fix(@libp2p/mdns): do not send TXT records that are too long #2014
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The data field of mDNS TXT records has a hard limit of 255 characters - some multiaddrs can be longer than this so filter them out before sending. We should also only be sending link-local, non-loopback addresses in mDNS responses so filter those out too.
SgtPooki
approved these changes
Sep 1, 2023
| return | ||
| } | ||
| _onMdnsWarning (err: Error): void { | ||
| log.error('mdns warning', err) |
There was a problem hiding this comment.
Should this be .error or .info?
| // TXT record fields have a max data length of 255 bytes | ||
| // see 6.1 - https://www.ietf.org/rfc/rfc6763.txt | ||
| if (data.length > 255) { | ||
| log('multiaddr %a is too long to use in mDNS query response', addr) |
There was a problem hiding this comment.
This will likely lead to a lot of circuit relay webtransport multiaddrs failing, won't it?
There was a problem hiding this comment.
Not failing, just being omitted from the mDNS response. go-libp2p just excludes all circuit addresses so I'm not sure if it's a big deal or not. My feeling is that mDNS is really for tcp/quic sort of addresses.
| @@ -141,15 +142,6 @@ describe('MulticastDNS', () => { | |||
| await stop(mdnsC) | |||
| }) | |||
|
|
|||
| it('should start and stop with go-libp2p-mdns compat', async () => { | |||
There was a problem hiding this comment.
Historically go-libp2p-mdns didn't speak spec-compliant mDNS so we had a compatibility layer.
go-libp2p-mdns has since been updated to behave better so the compatibility layer was removed, this test should have gone too.
|
|
||
| expect(pB.toString()).to.eql(id.toString()) | ||
|
|
||
| ;[ |
This was referenced Jan 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The data field of mDNS TXT records has a hard limit of 255 characters - some multiaddrs can be longer than this so filter them out before sending, otherwise remote peers can fail to parse our mDNS response.
We should also only be sending link-local addresses in mDNS responses so filter any non-link-local addresses out too, though go-libp2p includes loopback addresses even though it probably shouldn't (according to the mDNS spec which conflicts with our mDNS Peer Discovery spec) so we continue to do so as well.
Fixes #2012