Skip to content

Commit 61d594a

Browse files
ruedistedozyiotabcatclaude
authored
fix(autonat-v2): spurious exceptions (libp2p#3400)
* Fix spurious exceptions * chore: use isNetworkAddress instead of try catch * fix(protocol-autonat-v2): skip non-ip4/ip6 addresses without throwing Replace isNetworkAddress + getNetConfig pairs with tryGetNetConfig so each call site parses the multiaddr once, and skip connections whose remoteAddr is not ip4/ip6 (p2p-circuit, dns, etc.) since getNetworkSegment can only derive a stable segment from ip4/ip6 addresses. Adds tests covering: - non-network local addresses - non-network remote addresses (p2p-circuit) - DNS remote addresses (pass isNetworkAddress but not ip4/ip6) - non-network entries in peer.addresses when checking IPv6 support Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: dozyio <github@dozy.io> Co-authored-by: dozyio <37986489+dozyio@users.noreply.github.com> Co-authored-by: tabcat <tabcat00@proton.me> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
1 parent d57fe74 commit 61d594a

2 files changed

Lines changed: 137 additions & 3 deletions

File tree

packages/protocol-autonat-v2/src/client.ts

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { InvalidParametersError, ProtocolError, serviceCapabilities, serviceDependencies } from '@libp2p/interface'
22
import { peerSet } from '@libp2p/peer-collections'
3-
import { createScalableCuckooFilter, isGlobalUnicast, isPrivate, PeerQueue, repeatingTask, trackedMap, pbStream, getNetConfig } from '@libp2p/utils'
3+
import { createScalableCuckooFilter, isGlobalUnicast, isPrivate, PeerQueue, repeatingTask, trackedMap, pbStream, getNetConfig, tryGetNetConfig } from '@libp2p/utils'
44
import { anySignal } from 'any-signal'
55
import { setMaxListeners } from 'main-event'
66
import { DEFAULT_CONNECTION_THRESHOLD, DIAL_DATA_CHUNK_SIZE, MAX_DIAL_DATA_BYTES, MAX_INBOUND_STREAMS, MAX_MESSAGE_SIZE, MAX_OUTBOUND_STREAMS, TIMEOUT } from './constants.ts'
@@ -288,7 +288,12 @@ export class AutoNATv2Client implements Startable {
288288
return false
289289
}
290290

291-
const options = getNetConfig(addr.multiaddr)
291+
const options = tryGetNetConfig(addr.multiaddr)
292+
293+
if (options == null) {
294+
// skip non-network addresses
295+
return false
296+
}
292297

293298
if (options.type === 'ip6') {
294299
// do not send IPv6 addresses to peers without IPv6 addresses
@@ -402,9 +407,15 @@ export class AutoNATv2Client implements Startable {
402407
// if the remote peer has IPv6 addresses, we can probably send them an IPv6
403408
// address to verify, otherwise only send them IPv4 addresses
404409
const supportsIPv6 = peer.addresses.some(({ multiaddr }) => {
405-
return getNetConfig(multiaddr).type === 'ip6'
410+
return tryGetNetConfig(multiaddr)?.type === 'ip6'
406411
})
407412

413+
// only ip4/ip6 remote addrs can be mapped to a stable network segment
414+
const remoteAddrConfig = tryGetNetConfig(connection.remoteAddr)
415+
if (remoteAddrConfig?.type !== 'ip4' && remoteAddrConfig?.type !== 'ip6') {
416+
return
417+
}
418+
408419
// get multiaddrs this peer is eligible to verify
409420
const segment = this.getNetworkSegment(connection.remoteAddr)
410421
const results = this.getUnverifiedMultiaddrs(segment, supportsIPv6)

packages/protocol-autonat-v2/test/client.spec.ts

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -911,6 +911,129 @@ describe('autonat v2 - client', () => {
911911
.to.be.true('Did not verify external multiaddr')
912912
})
913913

914+
it('should skip non-network addresses', async () => {
915+
const peerId = peerIdFromPrivateKey(await generateKeyPair('Ed25519'))
916+
const nonNetworkAddress = multiaddr(`/p2p/${peerId.toString()}`)
917+
918+
addressManager.getAddressesWithMetadata.returns([{
919+
multiaddr: nonNetworkAddress,
920+
verified: false,
921+
type: 'observed',
922+
expires: 0
923+
}])
924+
925+
const connection = await stubPeerResponse({
926+
host: '123.123.123.123',
927+
messages: {}
928+
})
929+
930+
await service.client.verifyExternalAddresses(connection)
931+
await delay(100)
932+
933+
expect((connection.newStream as sinon.SinonStub).called)
934+
.to.be.false('Opened a stream for a non-network local address')
935+
expect(addressManager.confirmObservedAddr.called)
936+
.to.be.false('Attempted to verify a non-network address')
937+
})
938+
939+
it('should skip verification when the remote address is not a network address', async () => {
940+
const observedAddress = multiaddr('/ip4/123.123.123.123/tcp/28319')
941+
addressManager.getAddressesWithMetadata.returns([{
942+
multiaddr: observedAddress,
943+
verified: false,
944+
type: 'observed',
945+
expires: 0
946+
}])
947+
948+
const connection = await stubPeerResponse({
949+
host: '124.124.124.124',
950+
messages: {}
951+
})
952+
953+
// override with a p2p-circuit remote addr - no IP component, so no network
954+
// segment can be derived
955+
connection.remoteAddr = multiaddr(`/p2p/${connection.remotePeer.toString()}/p2p-circuit/p2p/${connection.remotePeer.toString()}`)
956+
957+
await service.client.verifyExternalAddresses(connection)
958+
await delay(100)
959+
960+
expect((connection.newStream as sinon.SinonStub).called)
961+
.to.be.false('Opened a stream for a non-network remote address')
962+
expect(addressManager.confirmObservedAddr.called).to.be.false()
963+
})
964+
965+
it('should skip verification when the remote address is not an IP address', async () => {
966+
const observedAddress = multiaddr('/ip4/123.123.123.123/tcp/28319')
967+
addressManager.getAddressesWithMetadata.returns([{
968+
multiaddr: observedAddress,
969+
verified: false,
970+
type: 'observed',
971+
expires: 0
972+
}])
973+
974+
const connection = await stubPeerResponse({
975+
host: '124.124.124.124',
976+
messages: {}
977+
})
978+
979+
// DNS remote addrs are network addresses but cannot be mapped to a stable
980+
// network segment
981+
connection.remoteAddr = multiaddr(`/dns4/example.com/tcp/443/p2p/${connection.remotePeer.toString()}`)
982+
983+
await service.client.verifyExternalAddresses(connection)
984+
await delay(100)
985+
986+
expect((connection.newStream as sinon.SinonStub).called)
987+
.to.be.false('Opened a stream for a DNS remote address')
988+
expect(addressManager.confirmObservedAddr.called).to.be.false()
989+
})
990+
991+
it('should skip non-network entries in peer addresses when checking IPv6 support', async () => {
992+
const observedAddress = multiaddr('/ip4/123.123.123.123/tcp/28319')
993+
addressManager.getAddressesWithMetadata.returns([{
994+
multiaddr: observedAddress,
995+
verified: false,
996+
type: 'observed',
997+
expires: 0
998+
}])
999+
1000+
const connection = await stubPeerResponse({
1001+
host: '124.124.124.124',
1002+
messages: {
1003+
[observedAddress.toString()]: {
1004+
dialResponse: {
1005+
addrIdx: 0,
1006+
status: DialResponse.ResponseStatus.OK,
1007+
dialStatus: DialStatus.OK
1008+
}
1009+
}
1010+
}
1011+
})
1012+
1013+
// override peer.addresses to include a non-network multiaddr - computing
1014+
// supportsIPv6 must not throw when it encounters it
1015+
peerStore.get.withArgs(connection.remotePeer).resolves({
1016+
id: connection.remotePeer,
1017+
addresses: [
1018+
{ multiaddr: multiaddr(`/p2p/${connection.remotePeer.toString()}`), isCertified: false },
1019+
{ multiaddr: multiaddr('/ip4/124.124.124.124/tcp/28319'), isCertified: true }
1020+
],
1021+
protocols: [
1022+
'/libp2p/autonat/2/dial-request',
1023+
'/libp2p/autonat/2/dial-back'
1024+
],
1025+
metadata: new Map(),
1026+
tags: new Map()
1027+
})
1028+
1029+
// should not throw - verification should proceed normally
1030+
await service.client.verifyExternalAddresses(connection)
1031+
await delay(100)
1032+
1033+
expect((connection.newStream as sinon.SinonStub).called)
1034+
.to.be.true('Did not attempt verification despite valid remote address')
1035+
})
1036+
9141037
it('should time out when verifying an observed address', async () => {
9151038
const observedAddress = multiaddr('/ip4/123.123.123.123/tcp/28319')
9161039
addressManager.getAddressesWithMetadata.returns([{

0 commit comments

Comments
 (0)