fix: dont allow multiaddr dials without a peer id (#558)

* fix: require peer ids when dialing multiaddrs

* chore: fix lint

* docs: add more info about multiaddr peer ids

doc/API.md

@@ -174,15 +174,13 @@ for (const [peerId, connections] of libp2p.connections) {
 
 ### dial
 
-Dials to another peer in the network and establishes the connection.
-
 `dial(peer, options)`
 
 #### Parameters
 
 | Name | Type | Description |
 |------|------|-------------|
-| peer | [`PeerInfo`][peer-info]\|[`PeerId`][peer-id]\|[`Multiaddr`][multiaddr]\|`string` | peer to dial |
+| peer | [`PeerInfo`][peer-info]\|[`PeerId`][peer-id]\|[`Multiaddr`][multiaddr]\|`string` | The peer to dial. If a [`Multiaddr`][multiaddr] or its string is provided, it **must** include the peer id |
 | [options] | `Object` | dial options |
 | [options.signal] | [`AbortSignal`](https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal) | An `AbortSignal` instance obtained from an [`AbortController`](https://developer.mozilla.org/en-US/docs/Web/API/AbortController) that can be used to abort the connection before it completes |
 
@@ -217,7 +215,7 @@ Dials to another peer in the network and selects a protocol to communicate with
 
 | Name | Type | Description |
 |------|------|-------------|
-| peer | [`PeerInfo`][peer-info]\|[`PeerId`][peer-id]\|[`Multiaddr`][multiaddr]\|`string` | peer to dial |
+| peer | [`PeerInfo`][peer-info]\|[`PeerId`][peer-id]\|[`Multiaddr`][multiaddr]\|`string` | The peer to dial. If a [`Multiaddr`][multiaddr] or its string is provided, it **must** include the peer id |
 | protocols | `String|Array<String>` |  A list of protocols (or single protocol) to negotiate with. Protocols are attempted in order until a match is made. (e.g '/ipfs/bitswap/1.1.0') |
 | [options] | `Object` | dial options |
 | [options.signal] | [`AbortSignal`](https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal) | An `AbortSignal` instance obtained from an [`AbortController`](https://developer.mozilla.org/en-US/docs/Web/API/AbortController) that can be used to abort the connection before it completes |

package.json

@@ -83,10 +83,10 @@
     "cids": "^0.7.1",
     "delay": "^4.3.0",
     "dirty-chai": "^2.0.1",
+    "interop-libp2p": "~0.0.1",
     "it-concat": "^1.0.0",
     "it-pair": "^1.0.0",
     "it-pushable": "^1.4.0",
-    "interop-libp2p": "~0.0.1",
     "libp2p-bootstrap": "^0.10.3",
     "libp2p-delegated-content-routing": "^0.4.1",
     "libp2p-delegated-peer-routing": "^0.4.0",

src/dialer/index.js

@@ -70,7 +70,7 @@ class Dialer {
   async connectToPeer (peer, options = {}) {
     const dialTarget = this._createDialTarget(peer)
     if (dialTarget.addrs.length === 0) {
-      throw errCode(new Error('The dial request has no addresses'), 'ERR_NO_DIAL_MULTIADDRS')
+      throw errCode(new Error('The dial request has no addresses'), codes.ERR_NO_VALID_ADDRESSES)
     }
     const pendingDial = this._pendingDials.get(dialTarget.id) || this._createPendingDial(dialTarget, options)
 
@@ -136,7 +136,7 @@ class Dialer {
    */
   _createPendingDial (dialTarget, options) {
     const dialAction = (addr, options) => {
-      if (options.signal.aborted) throw errCode(new Error('already aborted'), 'ERR_ALREADY_ABORTED')
+      if (options.signal.aborted) throw errCode(new Error('already aborted'), codes.ERR_ALREADY_ABORTED)
       return this.transportManager.dial(addr, options)
     }
 
@@ -197,8 +197,7 @@ class Dialer {
       try {
         peer = PeerId.createFromCID(peer.getPeerId())
       } catch (err) {
-        // Couldn't get the PeerId, just use the address
-        return peer
+        throw errCode(new Error('The multiaddr did not contain a valid peer id'), codes.ERR_INVALID_PEER)
       }
     }
 

src/errors.js

@@ -12,6 +12,7 @@ exports.codes = {
   ERR_CONNECTION_ENDED: 'ERR_CONNECTION_ENDED',
   ERR_CONNECTION_FAILED: 'ERR_CONNECTION_FAILED',
   ERR_NODE_NOT_STARTED: 'ERR_NODE_NOT_STARTED',
+  ERR_ALREADY_ABORTED: 'ERR_ALREADY_ABORTED',
   ERR_NO_VALID_ADDRESSES: 'ERR_NO_VALID_ADDRESSES',
   ERR_DISCOVERED_SELF: 'ERR_DISCOVERED_SELF',
   ERR_DUPLICATE_TRANSPORT: 'ERR_DUPLICATE_TRANSPORT',

src/peer-store/index.js

@@ -224,12 +224,16 @@ class PeerStore extends EventEmitter {
   }
 
   /**
-   * Returns the known multiaddrs for a given `PeerInfo`
+   * Returns the known multiaddrs for a given `PeerInfo`. All returned multiaddrs
+   * will include the encapsulated `PeerId` of the peer.
    * @param {PeerInfo} peer
    * @returns {Array<Multiaddr>}
    */
   multiaddrsForPeer (peer) {
-    return this.put(peer, true).multiaddrs.toArray()
+    return this.put(peer, true).multiaddrs.toArray().map(addr => {
+      if (addr.getPeerId()) return addr
+      return addr.encapsulate(`/p2p/${peer.id.toB58String()}`)
+    })
   }
 }
 

test/content-routing/dht/operation.node.js

@@ -43,7 +43,7 @@ describe('DHT subsystem operates correctly', () => {
         remoteLibp2p.start()
       ])
 
-      remAddr = remoteLibp2p.transportManager.getAddrs()[0]
+      remAddr = libp2p.peerStore.multiaddrsForPeer(remotePeerInfo)[0]
     })
 
     afterEach(() => Promise.all([
@@ -98,7 +98,7 @@ describe('DHT subsystem operates correctly', () => {
       await libp2p.start()
       await remoteLibp2p.start()
 
-      remAddr = remoteLibp2p.transportManager.getAddrs()[0]
+      remAddr = libp2p.peerStore.multiaddrsForPeer(remotePeerInfo)[0]
     })
 
     afterEach(() => Promise.all([

test/dialing/direct.node.js

@@ -34,20 +34,25 @@ const Peers = require('../fixtures/peers')
 const { createPeerInfo } = require('../utils/creators/peer')
 
 const listenAddr = multiaddr('/ip4/127.0.0.1/tcp/0')
-const unsupportedAddr = multiaddr('/ip4/127.0.0.1/tcp/9999/ws')
+const unsupportedAddr = multiaddr('/ip4/127.0.0.1/tcp/9999/ws/p2p/QmckxVrJw1Yo8LqvmDJNUmdAsKtSbiKWmrXJFyKmUraBoN')
 
 describe('Dialing (direct, TCP)', () => {
   let remoteTM
   let localTM
+  let peerStore
   let remoteAddr
 
   before(async () => {
+    const [remotePeerId] = await Promise.all([
+      PeerId.createFromJSON(Peers[0])
+    ])
     remoteTM = new TransportManager({
       libp2p: {},
       upgrader: mockUpgrader
     })
     remoteTM.add(Transport.prototype[Symbol.toStringTag], Transport)
 
+    peerStore = new PeerStore()
     localTM = new TransportManager({
       libp2p: {},
       upgrader: mockUpgrader
@@ -56,7 +61,7 @@ describe('Dialing (direct, TCP)', () => {
 
     await remoteTM.listen([listenAddr])
 
-    remoteAddr = remoteTM.getAddrs()[0]
+    remoteAddr = remoteTM.getAddrs()[0].encapsulate(`/p2p/${remotePeerId.toB58String()}`)
   })
 
   after(() => remoteTM.close())
@@ -66,15 +71,15 @@ describe('Dialing (direct, TCP)', () => {
   })
 
   it('should be able to connect to a remote node via its multiaddr', async () => {
-    const dialer = new Dialer({ transportManager: localTM })
+    const dialer = new Dialer({ transportManager: localTM, peerStore })
 
     const connection = await dialer.connectToPeer(remoteAddr)
     expect(connection).to.exist()
     await connection.close()
   })
 
   it('should be able to connect to a remote node via its stringified multiaddr', async () => {
-    const dialer = new Dialer({ transportManager: localTM })
+    const dialer = new Dialer({ transportManager: localTM, peerStore })
 
     const dialable = Dialer.getDialable(remoteAddr.toString())
     const connection = await dialer.connectToPeer(dialable)
@@ -83,7 +88,7 @@ describe('Dialing (direct, TCP)', () => {
   })
 
   it('should fail to connect to an unsupported multiaddr', async () => {
-    const dialer = new Dialer({ transportManager: localTM })
+    const dialer = new Dialer({ transportManager: localTM, peerStore })
 
     await expect(dialer.connectToPeer(unsupportedAddr))
       .to.eventually.be.rejectedWith(AggregateError)
@@ -140,6 +145,7 @@ describe('Dialing (direct, TCP)', () => {
   it('should abort dials on queue task timeout', async () => {
     const dialer = new Dialer({
       transportManager: localTM,
+      peerStore,
       timeout: 50
     })
     sinon.stub(localTM, 'dial').callsFake(async (addr, options) => {
@@ -224,7 +230,7 @@ describe('Dialing (direct, TCP)', () => {
       remoteLibp2p.handle('/echo/1.0.0', ({ stream }) => pipe(stream, stream))
 
       await remoteLibp2p.start()
-      remoteAddr = remoteLibp2p.transportManager.getAddrs()[0]
+      remoteAddr = remoteLibp2p.transportManager.getAddrs()[0].encapsulate(`/p2p/${remotePeerId.toB58String()}`)
     })
 
     afterEach(async () => {
@@ -235,6 +241,28 @@ describe('Dialing (direct, TCP)', () => {
 
     after(() => remoteLibp2p.stop())
 
+    it('should fail if no peer id is provided', async () => {
+      libp2p = new Libp2p({
+        peerInfo,
+        modules: {
+          transport: [Transport],
+          streamMuxer: [Muxer],
+          connEncryption: [Crypto]
+        }
+      })
+
+      sinon.spy(libp2p.dialer, 'connectToPeer')
+
+      try {
+        await libp2p.dial(remoteLibp2p.transportManager.getAddrs()[0])
+      } catch (err) {
+        expect(err).to.have.property('code', ErrorCodes.ERR_INVALID_PEER)
+        return
+      }
+
+      expect.fail('dial should have failed')
+    })
+
     it('should use the dialer for connecting to a multiaddr', async () => {
       libp2p = new Libp2p({
         peerInfo,
@@ -267,10 +295,8 @@ describe('Dialing (direct, TCP)', () => {
       })
 
       sinon.spy(libp2p.dialer, 'connectToPeer')
-      const remotePeer = new PeerInfo(remoteLibp2p.peerInfo.id)
-      remotePeer.multiaddrs.add(remoteAddr)
 
-      const connection = await libp2p.dial(remotePeer)
+      const connection = await libp2p.dial(remotePeerInfo)
       expect(connection).to.exist()
       const { stream, protocol } = await connection.newStream('/echo/1.0.0')
       expect(stream).to.exist()
@@ -306,7 +332,7 @@ describe('Dialing (direct, TCP)', () => {
         }
       })
 
-      const connection = await libp2p.dial(`${remoteAddr.toString()}/p2p/${remotePeerInfo.id.toB58String()}`)
+      const connection = await libp2p.dial(`${remoteAddr.toString()}`)
       expect(connection).to.exist()
       expect(connection.stat.timeline.close).to.not.exist()
       await libp2p.hangUp(connection.remotePeer)
@@ -337,33 +363,6 @@ describe('Dialing (direct, TCP)', () => {
       expect(libp2p.upgrader.protector.protect.callCount).to.equal(1)
     })
 
-    it('should coalesce parallel dials to the same peer (no id in multiaddr)', async () => {
-      libp2p = new Libp2p({
-        peerInfo,
-        modules: {
-          transport: [Transport],
-          streamMuxer: [Muxer],
-          connEncryption: [Crypto]
-        }
-      })
-      const dials = 10
-
-      const dialResults = await Promise.all([...new Array(dials)].map((_, index) => {
-        if (index % 2 === 0) return libp2p.dial(remoteLibp2p.peerInfo)
-        return libp2p.dial(remoteLibp2p.peerInfo.multiaddrs.toArray()[0])
-      }))
-
-      // All should succeed and we should have ten results
-      expect(dialResults).to.have.length(10)
-      for (const connection of dialResults) {
-        expect(Connection.isConnection(connection)).to.equal(true)
-      }
-
-      // We will have two connections, since the multiaddr dial doesn't have a peer id
-      expect(libp2p.connectionManager._connections.size).to.equal(2)
-      expect(remoteLibp2p.connectionManager._connections.size).to.equal(2)
-    })
-
     it('should coalesce parallel dials to the same peer (id in multiaddr)', async () => {
       libp2p = new Libp2p({
         peerInfo,
@@ -405,10 +404,9 @@ describe('Dialing (direct, TCP)', () => {
       const error = new Error('Boom')
       sinon.stub(libp2p.transportManager, 'dial').callsFake(() => Promise.reject(error))
 
-      const fullAddress = remoteAddr.encapsulate(`/p2p/${remoteLibp2p.peerInfo.id.toB58String()}`)
       const dialResults = await pSettle([...new Array(dials)].map((_, index) => {
         if (index % 2 === 0) return libp2p.dial(remoteLibp2p.peerInfo)
-        return libp2p.dial(fullAddress)
+        return libp2p.dial(remoteAddr)
       }))
 
       // All should succeed and we should have ten results

test/dialing/direct.spec.js

@@ -21,6 +21,7 @@ const { AbortError } = require('libp2p-interfaces/src/transport/errors')
 const { codes: ErrorCodes } = require('../../src/errors')
 const Constants = require('../../src/constants')
 const Dialer = require('../../src/dialer')
+const PeerStore = require('../../src/peer-store')
 const TransportManager = require('../../src/transport-manager')
 const Libp2p = require('../../src')
 
@@ -29,13 +30,15 @@ const { MULTIADDRS_WEBSOCKETS } = require('../fixtures/browser')
 const mockUpgrader = require('../utils/mockUpgrader')
 const createMockConnection = require('../utils/mockConnection')
 const { createPeerId } = require('../utils/creators/peer')
-const unsupportedAddr = multiaddr('/ip4/127.0.0.1/tcp/9999/ws')
+const unsupportedAddr = multiaddr('/ip4/127.0.0.1/tcp/9999/ws/p2p/QmckxVrJw1Yo8LqvmDJNUmdAsKtSbiKWmrXJFyKmUraBoN')
 const remoteAddr = MULTIADDRS_WEBSOCKETS[0]
 
 describe('Dialing (direct, WebSockets)', () => {
   let localTM
+  let peerStore
 
   before(() => {
+    peerStore = new PeerStore()
     localTM = new TransportManager({
       libp2p: {},
       upgrader: mockUpgrader,
@@ -49,13 +52,13 @@ describe('Dialing (direct, WebSockets)', () => {
   })
 
   it('should have appropriate defaults', () => {
-    const dialer = new Dialer({ transportManager: localTM })
+    const dialer = new Dialer({ transportManager: localTM, peerStore })
     expect(dialer.concurrency).to.equal(Constants.MAX_PARALLEL_DIALS)
     expect(dialer.timeout).to.equal(Constants.DIAL_TIMEOUT)
   })
 
   it('should limit the number of tokens it provides', () => {
-    const dialer = new Dialer({ transportManager: localTM })
+    const dialer = new Dialer({ transportManager: localTM, peerStore })
     const maxPerPeer = Constants.MAX_PER_PEER_DIALS
     expect(dialer.tokens).to.have.length(Constants.MAX_PARALLEL_DIALS)
     const tokens = dialer.getTokens(maxPerPeer + 1)
@@ -64,14 +67,14 @@ describe('Dialing (direct, WebSockets)', () => {
   })
 
   it('should not return tokens if non are left', () => {
-    const dialer = new Dialer({ transportManager: localTM })
+    const dialer = new Dialer({ transportManager: localTM, peerStore })
     sinon.stub(dialer, 'tokens').value([])
     const tokens = dialer.getTokens(1)
     expect(tokens.length).to.equal(0)
   })
 
   it('should NOT be able to return a token twice', () => {
-    const dialer = new Dialer({ transportManager: localTM })
+    const dialer = new Dialer({ transportManager: localTM, peerStore })
     const tokens = dialer.getTokens(1)
     expect(tokens).to.have.length(1)
     expect(dialer.tokens).to.have.length(Constants.MAX_PARALLEL_DIALS - 1)
@@ -107,7 +110,7 @@ describe('Dialing (direct, WebSockets)', () => {
   })
 
   it('should fail to connect to an unsupported multiaddr', async () => {
-    const dialer = new Dialer({ transportManager: localTM })
+    const dialer = new Dialer({ transportManager: localTM, peerStore })
 
     await expect(dialer.connectToPeer(unsupportedAddr))
       .to.eventually.be.rejectedWith(AggregateError)

test/dialing/relay.node.js

@@ -123,11 +123,10 @@ describe('Dialing (via relay, TCP)', () => {
   })
 
   it('dialer should stay connected to an already connected relay on hop failure', async () => {
-    const relayAddr = relayLibp2p.transportManager.getAddrs()[0]
     const relayIdString = relayLibp2p.peerInfo.id.toB58String()
+    const relayAddr = relayLibp2p.transportManager.getAddrs()[0].encapsulate(`/p2p/${relayIdString}`)
 
     const dialAddr = relayAddr
-      .encapsulate(`/p2p/${relayIdString}`)
       .encapsulate(`/p2p-circuit/p2p/${dstLibp2p.peerInfo.id.toB58String()}`)
 
     await srcLibp2p.dial(relayAddr)
@@ -142,16 +141,15 @@ describe('Dialing (via relay, TCP)', () => {
   })
 
   it('destination peer should stay connected to an already connected relay on hop failure', async () => {
-    const relayAddr = relayLibp2p.transportManager.getAddrs()[0]
     const relayIdString = relayLibp2p.peerInfo.id.toB58String()
+    const relayAddr = relayLibp2p.transportManager.getAddrs()[0].encapsulate(`/p2p/${relayIdString}`)
 
     const dialAddr = relayAddr
-      .encapsulate(`/p2p/${relayIdString}`)
       .encapsulate(`/p2p-circuit/p2p/${dstLibp2p.peerInfo.id.toB58String()}`)
 
     // Connect the destination peer and the relay
     const tcpAddrs = dstLibp2p.transportManager.getAddrs()
-    await dstLibp2p.transportManager.listen([multiaddr(`/p2p-circuit${relayAddr}/p2p/${relayIdString}`)])
+    await dstLibp2p.transportManager.listen([multiaddr(`/p2p-circuit${relayAddr}`)])
     expect(dstLibp2p.transportManager.getAddrs()).to.have.deep.members([...tcpAddrs, dialAddr.decapsulate('p2p')])
 
     // Tamper with the our multiaddrs for the circuit message