Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: filter public ips #97

Merged
merged 5 commits into from
Dec 20, 2024
Merged

fix: filter public ips #97

merged 5 commits into from
Dec 20, 2024

Conversation

2color
Copy link
Member

@2color 2color commented Dec 19, 2024

What does this fix?

This fixes a regression introduced with the cached router.

Because the cachedRouter was wrapping sanitizeRouter, any providers that has no addresses, were thus getting populated straight from cache without going through sanitizeRouter.

This resulted in us returning providers with private IPs.

How does this fix

This PR ensures that whatever the cached router spews out, gets properly sanitised.

Open question

  • sanitizeRouter removes private IPs, but it doesn't remove records that have no IPs. In theory, there's might be an edge case where we find records that only contain private IPs, that are then removed by sanitizeRouter, leading frontends/consumers of this to issue a subsequent calls. This seems pretty unlikely.
    • Is it even possible for would a peer show up in dht.FindPeer with only private IPs (if you're connecting to Amino)?
    • We can also make sure of this by adding a filter in the sanitizeRouter, but it seems like this might be unnecessary work.

@2color 2color requested a review from lidel December 19, 2024 10:40
@2color 2color marked this pull request as ready for review December 19, 2024 10:44
@2color 2color changed the title fix: only return public ips fix: filter public ips Dec 19, 2024
@2color 2color merged commit 77a8147 into main Dec 20, 2024
8 checks passed
@2color 2color deleted the fix-private-ips branch December 20, 2024 08:10
@2color 2color mentioned this pull request Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant