Skip to content

Commit a317a8b

Browse files
jacobheunjacobheun
authored
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
1 parent 8bed8f3 commit a317a8b

File tree

13 files changed

+88
-76
lines changed

13 files changed

+88
-76
lines changed

‎doc/API.md

+2-4
Original file line numberDiff line numberDiff line change
@@ -174,15 +174,13 @@ for (const [peerId, connections] of libp2p.connections) {
174174

175175
### dial
176176

177-
Dials to another peer in the network and establishes the connection.
178-
179177
`dial(peer, options)`
180178

181179
#### Parameters
182180

183181
| Name | Type | Description |
184182
|------|------|-------------|
185-
| peer | [`PeerInfo`][peer-info]\|[`PeerId`][peer-id]\|[`Multiaddr`][multiaddr]\|`string` | peer to dial |
183+
| 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 |
186184
| [options] | `Object` | dial options |
187185
| [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 |
188186

@@ -217,7 +215,7 @@ Dials to another peer in the network and selects a protocol to communicate with
217215

218216
| Name | Type | Description |
219217
|------|------|-------------|
220-
| peer | [`PeerInfo`][peer-info]\|[`PeerId`][peer-id]\|[`Multiaddr`][multiaddr]\|`string` | peer to dial |
218+
| 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 |
221219
| 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') |
222220
| [options] | `Object` | dial options |
223221
| [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

+1-1
Original file line numberDiff line numberDiff line change
@@ -83,10 +83,10 @@
8383
"cids": "^0.7.1",
8484
"delay": "^4.3.0",
8585
"dirty-chai": "^2.0.1",
86+
"interop-libp2p": "~0.0.1",
8687
"it-concat": "^1.0.0",
8788
"it-pair": "^1.0.0",
8889
"it-pushable": "^1.4.0",
89-
"interop-libp2p": "~0.0.1",
9090
"libp2p-bootstrap": "^0.10.3",
9191
"libp2p-delegated-content-routing": "^0.4.1",
9292
"libp2p-delegated-peer-routing": "^0.4.0",

‎src/dialer/index.js

+3-4
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ class Dialer {
7070
async connectToPeer (peer, options = {}) {
7171
const dialTarget = this._createDialTarget(peer)
7272
if (dialTarget.addrs.length === 0) {
73-
throw errCode(new Error('The dial request has no addresses'), 'ERR_NO_DIAL_MULTIADDRS')
73+
throw errCode(new Error('The dial request has no addresses'), codes.ERR_NO_VALID_ADDRESSES)
7474
}
7575
const pendingDial = this._pendingDials.get(dialTarget.id) || this._createPendingDial(dialTarget, options)
7676

@@ -136,7 +136,7 @@ class Dialer {
136136
*/
137137
_createPendingDial (dialTarget, options) {
138138
const dialAction = (addr, options) => {
139-
if (options.signal.aborted) throw errCode(new Error('already aborted'), 'ERR_ALREADY_ABORTED')
139+
if (options.signal.aborted) throw errCode(new Error('already aborted'), codes.ERR_ALREADY_ABORTED)
140140
return this.transportManager.dial(addr, options)
141141
}
142142

@@ -197,8 +197,7 @@ class Dialer {
197197
try {
198198
peer = PeerId.createFromCID(peer.getPeerId())
199199
} catch (err) {
200-
// Couldn't get the PeerId, just use the address
201-
return peer
200+
throw errCode(new Error('The multiaddr did not contain a valid peer id'), codes.ERR_INVALID_PEER)
202201
}
203202
}
204203

‎src/errors.js

+1
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ exports.codes = {
1212
ERR_CONNECTION_ENDED: 'ERR_CONNECTION_ENDED',
1313
ERR_CONNECTION_FAILED: 'ERR_CONNECTION_FAILED',
1414
ERR_NODE_NOT_STARTED: 'ERR_NODE_NOT_STARTED',
15+
ERR_ALREADY_ABORTED: 'ERR_ALREADY_ABORTED',
1516
ERR_NO_VALID_ADDRESSES: 'ERR_NO_VALID_ADDRESSES',
1617
ERR_DISCOVERED_SELF: 'ERR_DISCOVERED_SELF',
1718
ERR_DUPLICATE_TRANSPORT: 'ERR_DUPLICATE_TRANSPORT',

‎src/peer-store/index.js

+6-2
Original file line numberDiff line numberDiff line change
@@ -224,12 +224,16 @@ class PeerStore extends EventEmitter {
224224
}
225225

226226
/**
227-
* Returns the known multiaddrs for a given `PeerInfo`
227+
* Returns the known multiaddrs for a given `PeerInfo`. All returned multiaddrs
228+
* will include the encapsulated `PeerId` of the peer.
228229
* @param {PeerInfo} peer
229230
* @returns {Array<Multiaddr>}
230231
*/
231232
multiaddrsForPeer (peer) {
232-
return this.put(peer, true).multiaddrs.toArray()
233+
return this.put(peer, true).multiaddrs.toArray().map(addr => {
234+
if (addr.getPeerId()) return addr
235+
return addr.encapsulate(`/p2p/${peer.id.toB58String()}`)
236+
})
233237
}
234238
}
235239

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

+2-2
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ describe('DHT subsystem operates correctly', () => {
4343
remoteLibp2p.start()
4444
])
4545

46-
remAddr = remoteLibp2p.transportManager.getAddrs()[0]
46+
remAddr = libp2p.peerStore.multiaddrsForPeer(remotePeerInfo)[0]
4747
})
4848

4949
afterEach(() => Promise.all([
@@ -98,7 +98,7 @@ describe('DHT subsystem operates correctly', () => {
9898
await libp2p.start()
9999
await remoteLibp2p.start()
100100

101-
remAddr = remoteLibp2p.transportManager.getAddrs()[0]
101+
remAddr = libp2p.peerStore.multiaddrsForPeer(remotePeerInfo)[0]
102102
})
103103

104104
afterEach(() => Promise.all([

‎test/dialing/direct.node.js

+37-39
Original file line numberDiff line numberDiff line change
@@ -34,20 +34,25 @@ const Peers = require('../fixtures/peers')
3434
const { createPeerInfo } = require('../utils/creators/peer')
3535

3636
const listenAddr = multiaddr('/ip4/127.0.0.1/tcp/0')
37-
const unsupportedAddr = multiaddr('/ip4/127.0.0.1/tcp/9999/ws')
37+
const unsupportedAddr = multiaddr('/ip4/127.0.0.1/tcp/9999/ws/p2p/QmckxVrJw1Yo8LqvmDJNUmdAsKtSbiKWmrXJFyKmUraBoN')
3838

3939
describe('Dialing (direct, TCP)', () => {
4040
let remoteTM
4141
let localTM
42+
let peerStore
4243
let remoteAddr
4344

4445
before(async () => {
46+
const [remotePeerId] = await Promise.all([
47+
PeerId.createFromJSON(Peers[0])
48+
])
4549
remoteTM = new TransportManager({
4650
libp2p: {},
4751
upgrader: mockUpgrader
4852
})
4953
remoteTM.add(Transport.prototype[Symbol.toStringTag], Transport)
5054

55+
peerStore = new PeerStore()
5156
localTM = new TransportManager({
5257
libp2p: {},
5358
upgrader: mockUpgrader
@@ -56,7 +61,7 @@ describe('Dialing (direct, TCP)', () => {
5661

5762
await remoteTM.listen([listenAddr])
5863

59-
remoteAddr = remoteTM.getAddrs()[0]
64+
remoteAddr = remoteTM.getAddrs()[0].encapsulate(`/p2p/${remotePeerId.toB58String()}`)
6065
})
6166

6267
after(() => remoteTM.close())
@@ -66,15 +71,15 @@ describe('Dialing (direct, TCP)', () => {
6671
})
6772

6873
it('should be able to connect to a remote node via its multiaddr', async () => {
69-
const dialer = new Dialer({ transportManager: localTM })
74+
const dialer = new Dialer({ transportManager: localTM, peerStore })
7075

7176
const connection = await dialer.connectToPeer(remoteAddr)
7277
expect(connection).to.exist()
7378
await connection.close()
7479
})
7580

7681
it('should be able to connect to a remote node via its stringified multiaddr', async () => {
77-
const dialer = new Dialer({ transportManager: localTM })
82+
const dialer = new Dialer({ transportManager: localTM, peerStore })
7883

7984
const dialable = Dialer.getDialable(remoteAddr.toString())
8085
const connection = await dialer.connectToPeer(dialable)
@@ -83,7 +88,7 @@ describe('Dialing (direct, TCP)', () => {
8388
})
8489

8590
it('should fail to connect to an unsupported multiaddr', async () => {
86-
const dialer = new Dialer({ transportManager: localTM })
91+
const dialer = new Dialer({ transportManager: localTM, peerStore })
8792

8893
await expect(dialer.connectToPeer(unsupportedAddr))
8994
.to.eventually.be.rejectedWith(AggregateError)
@@ -140,6 +145,7 @@ describe('Dialing (direct, TCP)', () => {
140145
it('should abort dials on queue task timeout', async () => {
141146
const dialer = new Dialer({
142147
transportManager: localTM,
148+
peerStore,
143149
timeout: 50
144150
})
145151
sinon.stub(localTM, 'dial').callsFake(async (addr, options) => {
@@ -224,7 +230,7 @@ describe('Dialing (direct, TCP)', () => {
224230
remoteLibp2p.handle('/echo/1.0.0', ({ stream }) => pipe(stream, stream))
225231

226232
await remoteLibp2p.start()
227-
remoteAddr = remoteLibp2p.transportManager.getAddrs()[0]
233+
remoteAddr = remoteLibp2p.transportManager.getAddrs()[0].encapsulate(`/p2p/${remotePeerId.toB58String()}`)
228234
})
229235

230236
afterEach(async () => {
@@ -235,6 +241,28 @@ describe('Dialing (direct, TCP)', () => {
235241

236242
after(() => remoteLibp2p.stop())
237243

244+
it('should fail if no peer id is provided', async () => {
245+
libp2p = new Libp2p({
246+
peerInfo,
247+
modules: {
248+
transport: [Transport],
249+
streamMuxer: [Muxer],
250+
connEncryption: [Crypto]
251+
}
252+
})
253+
254+
sinon.spy(libp2p.dialer, 'connectToPeer')
255+
256+
try {
257+
await libp2p.dial(remoteLibp2p.transportManager.getAddrs()[0])
258+
} catch (err) {
259+
expect(err).to.have.property('code', ErrorCodes.ERR_INVALID_PEER)
260+
return
261+
}
262+
263+
expect.fail('dial should have failed')
264+
})
265+
238266
it('should use the dialer for connecting to a multiaddr', async () => {
239267
libp2p = new Libp2p({
240268
peerInfo,
@@ -267,10 +295,8 @@ describe('Dialing (direct, TCP)', () => {
267295
})
268296

269297
sinon.spy(libp2p.dialer, 'connectToPeer')
270-
const remotePeer = new PeerInfo(remoteLibp2p.peerInfo.id)
271-
remotePeer.multiaddrs.add(remoteAddr)
272298

273-
const connection = await libp2p.dial(remotePeer)
299+
const connection = await libp2p.dial(remotePeerInfo)
274300
expect(connection).to.exist()
275301
const { stream, protocol } = await connection.newStream('/echo/1.0.0')
276302
expect(stream).to.exist()
@@ -306,7 +332,7 @@ describe('Dialing (direct, TCP)', () => {
306332
}
307333
})
308334

309-
const connection = await libp2p.dial(`${remoteAddr.toString()}/p2p/${remotePeerInfo.id.toB58String()}`)
335+
const connection = await libp2p.dial(`${remoteAddr.toString()}`)
310336
expect(connection).to.exist()
311337
expect(connection.stat.timeline.close).to.not.exist()
312338
await libp2p.hangUp(connection.remotePeer)
@@ -337,33 +363,6 @@ describe('Dialing (direct, TCP)', () => {
337363
expect(libp2p.upgrader.protector.protect.callCount).to.equal(1)
338364
})
339365

340-
it('should coalesce parallel dials to the same peer (no id in multiaddr)', async () => {
341-
libp2p = new Libp2p({
342-
peerInfo,
343-
modules: {
344-
transport: [Transport],
345-
streamMuxer: [Muxer],
346-
connEncryption: [Crypto]
347-
}
348-
})
349-
const dials = 10
350-
351-
const dialResults = await Promise.all([...new Array(dials)].map((_, index) => {
352-
if (index % 2 === 0) return libp2p.dial(remoteLibp2p.peerInfo)
353-
return libp2p.dial(remoteLibp2p.peerInfo.multiaddrs.toArray()[0])
354-
}))
355-
356-
// All should succeed and we should have ten results
357-
expect(dialResults).to.have.length(10)
358-
for (const connection of dialResults) {
359-
expect(Connection.isConnection(connection)).to.equal(true)
360-
}
361-
362-
// We will have two connections, since the multiaddr dial doesn't have a peer id
363-
expect(libp2p.connectionManager._connections.size).to.equal(2)
364-
expect(remoteLibp2p.connectionManager._connections.size).to.equal(2)
365-
})
366-
367366
it('should coalesce parallel dials to the same peer (id in multiaddr)', async () => {
368367
libp2p = new Libp2p({
369368
peerInfo,
@@ -405,10 +404,9 @@ describe('Dialing (direct, TCP)', () => {
405404
const error = new Error('Boom')
406405
sinon.stub(libp2p.transportManager, 'dial').callsFake(() => Promise.reject(error))
407406

408-
const fullAddress = remoteAddr.encapsulate(`/p2p/${remoteLibp2p.peerInfo.id.toB58String()}`)
409407
const dialResults = await pSettle([...new Array(dials)].map((_, index) => {
410408
if (index % 2 === 0) return libp2p.dial(remoteLibp2p.peerInfo)
411-
return libp2p.dial(fullAddress)
409+
return libp2p.dial(remoteAddr)
412410
}))
413411

414412
// All should succeed and we should have ten results

‎test/dialing/direct.spec.js

+9-6
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ const { AbortError } = require('libp2p-interfaces/src/transport/errors')
2121
const { codes: ErrorCodes } = require('../../src/errors')
2222
const Constants = require('../../src/constants')
2323
const Dialer = require('../../src/dialer')
24+
const PeerStore = require('../../src/peer-store')
2425
const TransportManager = require('../../src/transport-manager')
2526
const Libp2p = require('../../src')
2627

@@ -29,13 +30,15 @@ const { MULTIADDRS_WEBSOCKETS } = require('../fixtures/browser')
2930
const mockUpgrader = require('../utils/mockUpgrader')
3031
const createMockConnection = require('../utils/mockConnection')
3132
const { createPeerId } = require('../utils/creators/peer')
32-
const unsupportedAddr = multiaddr('/ip4/127.0.0.1/tcp/9999/ws')
33+
const unsupportedAddr = multiaddr('/ip4/127.0.0.1/tcp/9999/ws/p2p/QmckxVrJw1Yo8LqvmDJNUmdAsKtSbiKWmrXJFyKmUraBoN')
3334
const remoteAddr = MULTIADDRS_WEBSOCKETS[0]
3435

3536
describe('Dialing (direct, WebSockets)', () => {
3637
let localTM
38+
let peerStore
3739

3840
before(() => {
41+
peerStore = new PeerStore()
3942
localTM = new TransportManager({
4043
libp2p: {},
4144
upgrader: mockUpgrader,
@@ -49,13 +52,13 @@ describe('Dialing (direct, WebSockets)', () => {
4952
})
5053

5154
it('should have appropriate defaults', () => {
52-
const dialer = new Dialer({ transportManager: localTM })
55+
const dialer = new Dialer({ transportManager: localTM, peerStore })
5356
expect(dialer.concurrency).to.equal(Constants.MAX_PARALLEL_DIALS)
5457
expect(dialer.timeout).to.equal(Constants.DIAL_TIMEOUT)
5558
})
5659

5760
it('should limit the number of tokens it provides', () => {
58-
const dialer = new Dialer({ transportManager: localTM })
61+
const dialer = new Dialer({ transportManager: localTM, peerStore })
5962
const maxPerPeer = Constants.MAX_PER_PEER_DIALS
6063
expect(dialer.tokens).to.have.length(Constants.MAX_PARALLEL_DIALS)
6164
const tokens = dialer.getTokens(maxPerPeer + 1)
@@ -64,14 +67,14 @@ describe('Dialing (direct, WebSockets)', () => {
6467
})
6568

6669
it('should not return tokens if non are left', () => {
67-
const dialer = new Dialer({ transportManager: localTM })
70+
const dialer = new Dialer({ transportManager: localTM, peerStore })
6871
sinon.stub(dialer, 'tokens').value([])
6972
const tokens = dialer.getTokens(1)
7073
expect(tokens.length).to.equal(0)
7174
})
7275

7376
it('should NOT be able to return a token twice', () => {
74-
const dialer = new Dialer({ transportManager: localTM })
77+
const dialer = new Dialer({ transportManager: localTM, peerStore })
7578
const tokens = dialer.getTokens(1)
7679
expect(tokens).to.have.length(1)
7780
expect(dialer.tokens).to.have.length(Constants.MAX_PARALLEL_DIALS - 1)
@@ -107,7 +110,7 @@ describe('Dialing (direct, WebSockets)', () => {
107110
})
108111

109112
it('should fail to connect to an unsupported multiaddr', async () => {
110-
const dialer = new Dialer({ transportManager: localTM })
113+
const dialer = new Dialer({ transportManager: localTM, peerStore })
111114

112115
await expect(dialer.connectToPeer(unsupportedAddr))
113116
.to.eventually.be.rejectedWith(AggregateError)

‎test/dialing/relay.node.js

+3-5
Original file line numberDiff line numberDiff line change
@@ -123,11 +123,10 @@ describe('Dialing (via relay, TCP)', () => {
123123
})
124124

125125
it('dialer should stay connected to an already connected relay on hop failure', async () => {
126-
const relayAddr = relayLibp2p.transportManager.getAddrs()[0]
127126
const relayIdString = relayLibp2p.peerInfo.id.toB58String()
127+
const relayAddr = relayLibp2p.transportManager.getAddrs()[0].encapsulate(`/p2p/${relayIdString}`)
128128

129129
const dialAddr = relayAddr
130-
.encapsulate(`/p2p/${relayIdString}`)
131130
.encapsulate(`/p2p-circuit/p2p/${dstLibp2p.peerInfo.id.toB58String()}`)
132131

133132
await srcLibp2p.dial(relayAddr)
@@ -142,16 +141,15 @@ describe('Dialing (via relay, TCP)', () => {
142141
})
143142

144143
it('destination peer should stay connected to an already connected relay on hop failure', async () => {
145-
const relayAddr = relayLibp2p.transportManager.getAddrs()[0]
146144
const relayIdString = relayLibp2p.peerInfo.id.toB58String()
145+
const relayAddr = relayLibp2p.transportManager.getAddrs()[0].encapsulate(`/p2p/${relayIdString}`)
147146

148147
const dialAddr = relayAddr
149-
.encapsulate(`/p2p/${relayIdString}`)
150148
.encapsulate(`/p2p-circuit/p2p/${dstLibp2p.peerInfo.id.toB58String()}`)
151149

152150
// Connect the destination peer and the relay
153151
const tcpAddrs = dstLibp2p.transportManager.getAddrs()
154-
await dstLibp2p.transportManager.listen([multiaddr(`/p2p-circuit${relayAddr}/p2p/${relayIdString}`)])
152+
await dstLibp2p.transportManager.listen([multiaddr(`/p2p-circuit${relayAddr}`)])
155153
expect(dstLibp2p.transportManager.getAddrs()).to.have.deep.members([...tcpAddrs, dialAddr.decapsulate('p2p')])
156154

157155
// Tamper with the our multiaddrs for the circuit message

0 commit comments

Comments
 (0)