Harden relay SSRF validation with post-resolution IP filtering#278
Merged
kwsantiago merged 16 commits intomainfrom Feb 26, 2026
Merged
Harden relay SSRF validation with post-resolution IP filtering#278kwsantiago merged 16 commits intomainfrom
kwsantiago merged 16 commits intomainfrom
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
WalkthroughCentralizes relay URL validation and comprehensive internal-IP detection in keep-core, adds allow-ws / allow-internal features, gates relay acceptance and DNS-resolution results, filters internal addresses before TCP connect, and updates callers and tests across desktop, mobile, frost-net, nip46 crates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App/Node
participant Core as keep-core::relay
participant DNS as DNS Resolver
participant Net as keep-frost-net (cert_pin)
participant TCP as TCP Stack
participant TLS as TLS Handshake
App->>Core: validate_relay_url(relay_url)
alt validation fails
Core-->>App: Error (reject relay)
else validation passes
App->>DNS: resolve(host)
DNS-->>App: [IP1, IP2, ...]
App->>Core: is_internal_ip(IPx) for each IP
Core-->>App: internal/public classification
alt all resolved IPs internal
App-->>Net: return Transport error (relay resolves to internal addresses only)
else
App->>TCP: connect(non-internal IP)
TCP-->>App: connected
App->>TLS: perform TLS handshake / cert pin
TLS-->>App: handshake result
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
keep-desktop/src/app.rs (1)
307-317: Normalize/dedupe/cap loaded relays to match runtime constraints
filter_valid_relayscurrently only validates. Consider normalizing, deduplicating, and enforcingMAX_RELAYSduring load to keep disk-loaded behavior consistent with add-relay behavior.♻️ Suggested patch
fn filter_valid_relays(urls: Vec<String>) -> Vec<String> { - urls.into_iter() - .filter(|url| match validate_relay_url(url) { - Ok(()) => true, - Err(e) => { - tracing::warn!(url, "Dropping invalid relay loaded from disk: {e}"); - false - } - }) - .collect() + let mut out = Vec::new(); + for raw in urls { + let normalized = normalize_relay_url(&raw); + match validate_relay_url(&normalized) { + Ok(()) => { + if !out.contains(&normalized) { + out.push(normalized); + if out.len() >= MAX_RELAYS { + break; + } + } + } + Err(e) => { + tracing::warn!(url = %raw, "Dropping invalid relay loaded from disk: {e}"); + } + } + } + out }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-desktop/src/app.rs` around lines 307 - 317, filter_valid_relays currently only validates but doesn't normalize, dedupe, or cap loaded relays to the same runtime rules used by the add-relay flow; update filter_valid_relays to: normalize each url (e.g., lowercasing host, removing default ports) before validation via validate_relay_url, deduplicate while preserving order, and enforce the MAX_RELAYS limit (same policy as add-relay) by truncating the final Vec to MAX_RELAYS; ensure any dropped/duplicate entries are traced with tracing::warn similar to current behavior so behavior matches runtime add-relay constraints.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keep-core/src/relay.rs`:
- Around line 151-157: Add a check for the deprecated IPv6 site-local prefix
fec0::/10 in is_internal_v6: update the boolean expression that inspects
addr.segments() (in relay.rs) to include OR (segments[0] & 0xffc0) == 0xfec0
alongside the existing fc00/7 and fe80/10 checks so fec0::/10 is treated as
internal for SSRF protection.
---
Nitpick comments:
In `@keep-desktop/src/app.rs`:
- Around line 307-317: filter_valid_relays currently only validates but doesn't
normalize, dedupe, or cap loaded relays to the same runtime rules used by the
add-relay flow; update filter_valid_relays to: normalize each url (e.g.,
lowercasing host, removing default ports) before validation via
validate_relay_url, deduplicate while preserving order, and enforce the
MAX_RELAYS limit (same policy as add-relay) by truncating the final Vec to
MAX_RELAYS; ensure any dropped/duplicate entries are traced with tracing::warn
similar to current behavior so behavior matches runtime add-relay constraints.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
keep-core/src/relay.rskeep-desktop/src/app.rskeep-frost-net/src/cert_pin.rskeep-frost-net/src/node/mod.rskeep-mobile/src/network.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
keep-core/src/relay.rs (1)
154-165: Past concern aboutfec0::/10has been addressed.Line 161 now includes
(segments[0] & 0xffc0) == 0xfec0, which correctly blocks the deprecated IPv6 site-local range. This matches the fix suggested in the previous review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-core/src/relay.rs` around lines 154 - 165, is_internal_v6 currently checks embedded IPv4 via to_embedded_v4 and falls back to IPv6 segment tests; ensure the deprecated site-local range fec0::/10 is blocked by keeping the condition (segments[0] & 0xffc0) == 0xfec0 in the function is_internal_v6, alongside the existing checks ((segments[0] & 0xfe00) == 0xfc00, (segments[0] & 0xffc0) == 0xfe80, addr.is_loopback(), addr.is_unspecified(), addr.is_multicast()), and preserve the early return to is_internal_v4 for mapped v4 addresses via is_internal_v4 so embedded IPv4 handling remains unchanged.
🧹 Nitpick comments (4)
keep-nip46/src/bunker.rs (2)
319-324: Tests adapt to feature flag rather than asserting a fixed expectation.These tests check
validate_relay_url("ws://...")at runtime to decide the expected outcome. While pragmatic for a multi-config workspace, this means neither thews://-rejected nor thews://-accepted path is guaranteed to be exercised in any given test run. Ifallow-wsis accidentally enabled in a release profile, these tests won't catch it.Consider using
#[cfg(feature = "allow-ws")]and#[cfg(not(feature = "allow-ws"))]to write separate, unconditional assertions for each configuration, so both paths are always tested for their respective profile.♻️ Example: split into feature-gated test branches
#[test] fn test_parse_nostrconnect_rejects_non_wss_relay() { // ... URI construction ... - let ws_allowed = keep_core::relay::validate_relay_url("ws://relay.example.com").is_ok(); - if ws_allowed { - assert!(parse_nostrconnect_uri(&uri).is_ok()); - } else { + #[cfg(feature = "allow-ws")] + assert!(parse_nostrconnect_uri(&uri).is_ok()); + #[cfg(not(feature = "allow-ws"))] + { assert!(parse_nostrconnect_uri(&uri).is_err()); } }Also applies to: 537-543
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-nip46/src/bunker.rs` around lines 319 - 324, Tests currently branch at runtime using keep_core::relay::validate_relay_url("ws://...") to decide expectations; instead split into two feature-gated test branches so each configuration has an unconditional assertion: add one test block annotated #[cfg(feature = "allow-ws")] that asserts parse_nostrconnect_uri(&uri).is_ok() and another #[cfg(not(feature = "allow-ws"))] that asserts parse_nostrconnect_uri(&uri).is_err(); reference the existing functions validate_relay_url and parse_nostrconnect_uri when locating the test and remove the runtime conditional so each build/profile exercises the correct path.
83-86: Asymmetric error handling for invalid relays betweenparse_nostrconnect_uriandparse_bunker_url.
parse_nostrconnect_urisilently drops invalid relays (line 84:validate_relay_url(&value).is_ok()as a filter) and only errors if zero valid relays remain, whileparse_bunker_urlfails immediately on the first invalid relay (line 145-146:validate_relay_url(&value).map_err(...)?). This means a bunker URL with one bad relay among several good ones will be rejected entirely, but the same mix in a nostrconnect URI would succeed with only the valid relays.If this is intentional (e.g., nostrconnect comes from untrusted external clients, bunker URLs are user-configured), it's worth a brief doc comment on
parse_bunker_urlnoting the strict behavior so future maintainers understand the design choice.Also applies to: 141-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-nip46/src/bunker.rs` around lines 83 - 86, parse_bunker_url currently errors on the first invalid relay via validate_relay_url(...).map_err(...)? while parse_nostrconnect_uri filters out invalid relays and only errors if none remain; decide which behavior is desired and either (A) make parse_bunker_url permissive like parse_nostrconnect_uri by replacing the immediate map_err/? with the same is_ok filter + normalize_relay_url push and only return an error if relays.is_empty(), or (B) keep the strict behavior but add a clear doc comment above parse_bunker_url stating it will reject the entire URL on any invalid relay (reference validate_relay_url, parse_bunker_url, parse_nostrconnect_uri) so future maintainers understand the intentional asymmetry.keep-core/src/relay.rs (2)
435-504: Good test coverage for the newis_internal_ipfunction.Tests cover IPv4 private ranges, IPv6 ULA/link-local, mapped IPv4, IPv4-compatible IPv6, multicast, and 0/8. Consider adding a test for
fec0::1(the site-local range added at line 161) inis_internal_ip_blocks_privateto ensure end-to-end coverage of that fix.📝 Add fec0::1 test case
assert!(is_internal_ip(&"fe80::1".parse::<IpAddr>().unwrap())); + assert!(is_internal_ip(&"fec0::1".parse::<IpAddr>().unwrap())); assert!(is_internal_ip( &"::ffff:127.0.0.1".parse::<IpAddr>().unwrap() ));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-core/src/relay.rs` around lines 435 - 504, Add an assertion for the site-local IPv6 address in the existing test is_internal_ip_blocks_private: update that test to include assert!(is_internal_ip(&"fec0::1".parse::<IpAddr>().unwrap())); so the is_internal_ip function’s handling of the fec0::/10 site-local range (added around the logic referenced at line 161) is exercised end-to-end.
154-165: Consider blocking 6to4 (2002::/16) addresses that embed private IPv4.
is_internal_v6does not detect 6to4 addresses (2002::/16), which embed an IPv4 address in bits 16–47. An attacker could craft a DNS record returning e.g.2002:c0a8:0101::1(embeds192.168.1.1). Since this function is specifically documented for post-DNS-resolution filtering, extracting and checking the embedded IPv4 from 6to4 addresses would close this bypass vector.Similarly,
2001:db8::/32(documentation prefix, analogous to TEST-NET) is not blocked.🛡️ Proposed additions to is_internal_v6
fn is_internal_v6(addr: &std::net::Ipv6Addr) -> bool { if let Some(v4) = to_embedded_v4(addr) { return is_internal_v4(v4); } let segments = addr.segments(); + // 6to4 (2002::/16): extract embedded IPv4 from bits 16-47 + if segments[0] == 0x2002 { + let v4 = std::net::Ipv4Addr::new( + (segments[1] >> 8) as u8, + segments[1] as u8, + (segments[2] >> 8) as u8, + segments[2] as u8, + ); + return is_internal_v4(v4); + } + // Documentation prefix (RFC 3849) + if segments[0] == 0x2001 && segments[1] == 0x0db8 { + return true; + } (segments[0] & 0xfe00) == 0xfc00 || (segments[0] & 0xffc0) == 0xfe80 || (segments[0] & 0xffc0) == 0xfec0 || addr.is_loopback() || addr.is_unspecified() || addr.is_multicast() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-core/src/relay.rs` around lines 154 - 165, is_internal_v6 currently misses 6to4 (2002::/16) addresses and the documentation prefix 2001:db8::/32; update is_internal_v6 to treat 6to4 specially by detecting segments[0] == 0x2002, extracting the embedded IPv4 from segments[1] and segments[2] (combine those 32 bits into an Ipv4Addr) and then call is_internal_v4 on that extracted IPv4, and also add a check to return true for the documentation prefix by matching segments[0] == 0x2001 && segments[1] == 0x0db8 so these ranges are blocked post-DNS resolution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@keep-core/src/relay.rs`:
- Around line 154-165: is_internal_v6 currently checks embedded IPv4 via
to_embedded_v4 and falls back to IPv6 segment tests; ensure the deprecated
site-local range fec0::/10 is blocked by keeping the condition (segments[0] &
0xffc0) == 0xfec0 in the function is_internal_v6, alongside the existing checks
((segments[0] & 0xfe00) == 0xfc00, (segments[0] & 0xffc0) == 0xfe80,
addr.is_loopback(), addr.is_unspecified(), addr.is_multicast()), and preserve
the early return to is_internal_v4 for mapped v4 addresses via is_internal_v4 so
embedded IPv4 handling remains unchanged.
---
Nitpick comments:
In `@keep-core/src/relay.rs`:
- Around line 435-504: Add an assertion for the site-local IPv6 address in the
existing test is_internal_ip_blocks_private: update that test to include
assert!(is_internal_ip(&"fec0::1".parse::<IpAddr>().unwrap())); so the
is_internal_ip function’s handling of the fec0::/10 site-local range (added
around the logic referenced at line 161) is exercised end-to-end.
- Around line 154-165: is_internal_v6 currently misses 6to4 (2002::/16)
addresses and the documentation prefix 2001:db8::/32; update is_internal_v6 to
treat 6to4 specially by detecting segments[0] == 0x2002, extracting the embedded
IPv4 from segments[1] and segments[2] (combine those 32 bits into an Ipv4Addr)
and then call is_internal_v4 on that extracted IPv4, and also add a check to
return true for the documentation prefix by matching segments[0] == 0x2001 &&
segments[1] == 0x0db8 so these ranges are blocked post-DNS resolution.
In `@keep-nip46/src/bunker.rs`:
- Around line 319-324: Tests currently branch at runtime using
keep_core::relay::validate_relay_url("ws://...") to decide expectations; instead
split into two feature-gated test branches so each configuration has an
unconditional assertion: add one test block annotated #[cfg(feature =
"allow-ws")] that asserts parse_nostrconnect_uri(&uri).is_ok() and another
#[cfg(not(feature = "allow-ws"))] that asserts
parse_nostrconnect_uri(&uri).is_err(); reference the existing functions
validate_relay_url and parse_nostrconnect_uri when locating the test and remove
the runtime conditional so each build/profile exercises the correct path.
- Around line 83-86: parse_bunker_url currently errors on the first invalid
relay via validate_relay_url(...).map_err(...)? while parse_nostrconnect_uri
filters out invalid relays and only errors if none remain; decide which behavior
is desired and either (A) make parse_bunker_url permissive like
parse_nostrconnect_uri by replacing the immediate map_err/? with the same is_ok
filter + normalize_relay_url push and only return an error if relays.is_empty(),
or (B) keep the strict behavior but add a clear doc comment above
parse_bunker_url stating it will reject the entire URL on any invalid relay
(reference validate_relay_url, parse_bunker_url, parse_nostrconnect_uri) so
future maintainers understand the intentional asymmetry.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
keep-core/Cargo.tomlkeep-core/src/relay.rskeep-frost-net/Cargo.tomlkeep-mobile/src/network.rskeep-nip46/src/bunker.rs
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
keep-core/src/relay.rs (1)
435-452:⚠️ Potential issue | 🟡 Minor
fec0::1assertion missing fromis_internal_ip_blocks_private
is_internal_v6now correctly blocksfec0::/10(line 161), but the test doesn't exercise this path.✅ Proposed addition
assert!(is_internal_ip(&"fe80::1".parse::<IpAddr>().unwrap())); + assert!(is_internal_ip(&"fec0::1".parse::<IpAddr>().unwrap())); assert!(is_internal_ip( &"::ffff:127.0.0.1".parse::<IpAddr>().unwrap() ));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-core/src/relay.rs` around lines 435 - 452, The test is_internal_ip_blocks_private doesn't cover the IPv6 site-local range fec0::/10 even though is_internal_v6 blocks it; add an assertion in the is_internal_ip_blocks_private test that calls is_internal_ip(&"fec0::1".parse::<IpAddr>().unwrap()) to ensure the fec0::1 address is considered internal (use the existing pattern of assert!(is_internal_ip(...)) alongside the other IPv6 checks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keep-core/src/relay.rs`:
- Around line 136-138: The current conditional if !cfg!(feature = "allow-ws") &&
is_internal_host(host) disables SSRF checks when the allow-ws feature is
enabled; change the logic so the internal-host guard always runs: remove the
cfg!(feature = "allow-ws") gating from the is_internal_host(host) check (i.e.,
always return Err if is_internal_host(host) is true), and keep allow-ws only for
scheme/TLS handling elsewhere; if you need dev-only exceptions introduce a
distinct feature like allow-internal with a clear doc comment warning it must
not be enabled in production and use that for explicit test-only bypasses
instead of reusing allow-ws.
- Around line 167-180: The to_embedded_v4 function misses NAT64 Well-Known
Prefix 64:ff9b::/96, so update to_embedded_v4 to additionally detect the NAT64
WKP by checking IPv6 segments for [0x0064, 0xff9b] followed by zeros (or
equivalently check the first 12 octets match the /96 prefix) and return the
embedded IPv4 built from the last 4 octets; ensure this logic is used by
is_internal_v6 / is_internal_ip flow, and add the provided tests
(is_internal_ip_blocks_nat64_wkp) to validate 64:ff9b::a00:1 → 10.0.0.1 and
64:ff9b::7f00:1 → loopback cases.
---
Duplicate comments:
In `@keep-core/src/relay.rs`:
- Around line 435-452: The test is_internal_ip_blocks_private doesn't cover the
IPv6 site-local range fec0::/10 even though is_internal_v6 blocks it; add an
assertion in the is_internal_ip_blocks_private test that calls
is_internal_ip(&"fec0::1".parse::<IpAddr>().unwrap()) to ensure the fec0::1
address is considered internal (use the existing pattern of
assert!(is_internal_ip(...)) alongside the other IPv6 checks).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
keep-core/src/relay.rs (1)
154-165:⚠️ Potential issue | 🟡 Minor
fec0::/10is implemented but the companion test assertion is still missingLine 161 correctly adds
(segments[0] & 0xffc0) == 0xfec0to cover the deprecated site-local range. However, neitheris_internal_ip_blocks_private(lines 439–455) norrejects_ipv6_internal(lines 309–317) contains an assertion forfec0::1orwss://[fec0::1]/, leaving the guard untested.✅ Suggested additions
assert!(is_internal_ip(&"fe80::1".parse::<IpAddr>().unwrap())); + assert!(is_internal_ip(&"fec0::1".parse::<IpAddr>().unwrap())); assert!(is_internal_ip( &"::ffff:127.0.0.1".parse::<IpAddr>().unwrap() ));assert!(validate_relay_url("wss://[fe80::1]/").is_err()); assert!(validate_relay_url("wss://[ff02::1]/").is_err()); + assert!(validate_relay_url("wss://[fec0::1]/").is_err()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-core/src/relay.rs` around lines 154 - 165, Add test assertions to cover the newly handled deprecated site-local IPv6 block fec0::/10: update the test functions is_internal_ip_blocks_private and rejects_ipv6_internal to include an assertion that is_internal_v6 parses/returns true for "fec0::1" (or equivalent Ipv6Addr) and that the URL/reject logic rejects a websocket URL like "wss://[fec0::1]/". Locate tests referencing is_internal_v6, is_internal_ip_blocks_private, and rejects_ipv6_internal and add these two assertions so the fec0::/10 branch (segments[0] & 0xffc0 == 0xfec0) is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keep-core/src/relay.rs`:
- Around line 167-183: Add URL-level tests that exercise the full
is_internal_host → is_internal_v6 → to_embedded_v4 path by calling
validate_relay_url with hosts using IPv4-compatible (::x.x.x.x) and NAT64
Well-Known Prefix (64:ff9b::/96) IPv6 forms; update the existing
rejects_ipv6_internal test to include at least one URL for each form (e.g., host
literal ::127.0.0.1 and a NAT64-mapped address whose last 32 bits are an IPv4
like ::64:ff9b:7f00:1) so validate_relay_url fails for these addresses and the
to_embedded_v4 logic is exercised end-to-end. Ensure the tests reference the
validate_relay_url, is_internal_host, is_internal_v6, and to_embedded_v4 code
paths.
---
Duplicate comments:
In `@keep-core/src/relay.rs`:
- Around line 154-165: Add test assertions to cover the newly handled deprecated
site-local IPv6 block fec0::/10: update the test functions
is_internal_ip_blocks_private and rejects_ipv6_internal to include an assertion
that is_internal_v6 parses/returns true for "fec0::1" (or equivalent Ipv6Addr)
and that the URL/reject logic rejects a websocket URL like "wss://[fec0::1]/".
Locate tests referencing is_internal_v6, is_internal_ip_blocks_private, and
rejects_ipv6_internal and add these two assertions so the fec0::/10 branch
(segments[0] & 0xffc0 == 0xfec0) is exercised.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
keep-core/Cargo.tomlkeep-core/src/relay.rskeep-frost-net/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- keep-frost-net/Cargo.toml
- keep-core/Cargo.toml
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
keep-core/src/relay.rs (1)
136-138:⚠️ Potential issue | 🔴 CriticalRoot cause of CI failures: tests don't account for
allow-internalfeature flag.The
allow-internalfeature correctly bypasses the internal-host check for dev/testing scenarios, but the tests expecting internal addresses to be rejected (lines 284–293, 296–301, 304–306, 309–323, 332–338, 341–344, 346–349, 415–419, 422–435, 437–442, 480–490) don't have conditional logic to handle this feature flag. When CI runs withallow-internalenabled, all these tests fail becausevalidate_relay_urlreturnsOkinstead ofErr.The
rejects_non_wsstest (lines 260–264) demonstrates the correct pattern—add similar conditional handling to all internal-address rejection tests.🔧 Example fix pattern (apply to all affected tests)
#[test] fn rejects_internal_ipv4() { + if cfg!(feature = "allow-internal") { + // Internal addresses allowed when feature enabled + assert!(validate_relay_url("wss://127.0.0.1/").is_ok()); + return; + } assert!(validate_relay_url("wss://127.0.0.1/").is_err()); assert!(validate_relay_url("wss://127.0.0.2/").is_err()); // ... rest of assertions }Alternatively, use
#[cfg_attr(feature = "allow-internal", ignore)]to skip these tests entirely when the feature is enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-core/src/relay.rs` around lines 136 - 138, Tests that assert internal addresses are rejected fail when the allow-internal feature is enabled because validate_relay_url will return Ok; update each failing test (the ones asserting internal-host rejection) to follow the pattern used in rejects_non_wss by making the assertion conditional on cfg!(not(feature = "allow-internal")) or annotate the test with #[cfg_attr(feature = "allow-internal", ignore)] so they are skipped when allow-internal is enabled; locate tests referencing validate_relay_url and internal hosts (mentioned in the review) and apply one of these two patterns consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@keep-core/src/relay.rs`:
- Around line 136-138: Tests that assert internal addresses are rejected fail
when the allow-internal feature is enabled because validate_relay_url will
return Ok; update each failing test (the ones asserting internal-host rejection)
to follow the pattern used in rejects_non_wss by making the assertion
conditional on cfg!(not(feature = "allow-internal")) or annotate the test with
#[cfg_attr(feature = "allow-internal", ignore)] so they are skipped when
allow-internal is enabled; locate tests referencing validate_relay_url and
internal hosts (mentioned in the review) and apply one of these two patterns
consistently.
wksantiago
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
keep-frost-net/src/node/mod.rs (1)
328-337: Consider hoisting validator selection outside the loop.Since
ALLOW_INTERNAL_HOSTSis a compile-time constant, the validator function is the same for every iteration. Moving the selection outside the loop improves readability. The compiler likely optimizes this away, so this is purely a stylistic preference.♻️ Suggested refactor
+ let validate: fn(&str) -> Result<(), String> = if ALLOW_INTERNAL_HOSTS { + validate_relay_url_allow_internal + } else { + validate_relay_url + }; for relay in &relays { - let validate = if ALLOW_INTERNAL_HOSTS { - validate_relay_url_allow_internal - } else { - validate_relay_url - }; validate(relay).map_err(|e| { FrostNetError::Transport(format!("Rejected relay URL {relay}: {e}")) })?; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-frost-net/src/node/mod.rs` around lines 328 - 337, Move the validator selection out of the loop: choose the function once based on the compile-time constant ALLOW_INTERNAL_HOSTS (select either validate_relay_url_allow_internal or validate_relay_url) before iterating over relays, then call the selected validator inside the for relay in &relays loop and map errors to FrostNetError::Transport as before; this removes repeated branching per iteration and improves readability while keeping the existing validate(...).map_err(...) behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@keep-frost-net/src/node/mod.rs`:
- Around line 328-337: Move the validator selection out of the loop: choose the
function once based on the compile-time constant ALLOW_INTERNAL_HOSTS (select
either validate_relay_url_allow_internal or validate_relay_url) before iterating
over relays, then call the selected validator inside the for relay in &relays
loop and map errors to FrostNetError::Transport as before; this removes repeated
branching per iteration and improves readability while keeping the existing
validate(...).map_err(...) behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
keep-core/src/relay.rskeep-frost-net/Cargo.tomlkeep-frost-net/src/node/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- keep-frost-net/Cargo.toml
kwsantiago
approved these changes
Feb 26, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (2)
keep-desktop/src/app.rs (1)
348-355: Consider falling back to defaults when all saved bunker relays are filtered out.If a user's saved bunker relay config contains only URLs that fail validation (e.g., internal addresses now blocked),
filter_valid_relaysreturns an empty list. Theunwrap_or_else(default_bunker_relays)only applies when deserialization fails, not when filtering empties a valid list.This edge case might leave users with no bunker relays until they manually reconfigure. A fallback could improve UX:
🔧 Optional: Fall back to defaults when filtered list is empty
fn load_bunker_relays(keep_path: &std::path::Path) -> Vec<String> { let path = bunker_relay_config_path(keep_path); let urls: Vec<String> = std::fs::read_to_string(&path) .ok() .and_then(|s| serde_json::from_str(&s).ok()) .unwrap_or_else(default_bunker_relays); - filter_valid_relays(urls) + let filtered = filter_valid_relays(urls); + if filtered.is_empty() { + tracing::warn!("All saved bunker relays invalid, using defaults"); + default_bunker_relays() + } else { + filtered + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-desktop/src/app.rs` around lines 348 - 355, The current load_bunker_relays reads JSON and falls back to default_bunker_relays only on deserialization error, but after calling filter_valid_relays it may return an empty Vec when all saved URLs are invalid; update load_bunker_relays so that after deserializing and calling filter_valid_relays(urls) you check if the resulting Vec is empty and, if so, return default_bunker_relays() instead; reference functions: load_bunker_relays, bunker_relay_config_path, filter_valid_relays, and default_bunker_relays to locate and implement this empty-result fallback.keep-core/src/relay.rs (1)
480-497: Add test coverage for deprecated site-localfec0::/10range.The
is_internal_v6function now correctly includes thefec0::/10site-local check (line 178), but this test doesn't exercise that path. Addingfec0::1would ensure the deprecated-but-exploitable range is covered.✅ Suggested test addition
assert!(is_internal_ip(&"fc00::1".parse::<IpAddr>().unwrap())); assert!(is_internal_ip(&"fe80::1".parse::<IpAddr>().unwrap())); + assert!(is_internal_ip(&"fec0::1".parse::<IpAddr>().unwrap())); // deprecated site-local assert!(is_internal_ip( &"::ffff:127.0.0.1".parse::<IpAddr>().unwrap() ));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-core/src/relay.rs` around lines 480 - 497, The test is missing coverage for the deprecated IPv6 site-local range fec0::/10; update the is_internal_ip tests by adding an assertion that parses "fec0::1" into std::net::IpAddr and passes it to is_internal_ip (or exercise is_internal_v6 via is_internal_ip) to assert true so the fec0::/10 check is covered; locate the test function is_internal_ip_blocks_private and add the assert!(is_internal_ip(&"fec0::1".parse::<IpAddr>().unwrap())); line.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@keep-core/src/relay.rs`:
- Around line 480-497: The test is missing coverage for the deprecated IPv6
site-local range fec0::/10; update the is_internal_ip tests by adding an
assertion that parses "fec0::1" into std::net::IpAddr and passes it to
is_internal_ip (or exercise is_internal_v6 via is_internal_ip) to assert true so
the fec0::/10 check is covered; locate the test function
is_internal_ip_blocks_private and add the
assert!(is_internal_ip(&"fec0::1".parse::<IpAddr>().unwrap())); line.
In `@keep-desktop/src/app.rs`:
- Around line 348-355: The current load_bunker_relays reads JSON and falls back
to default_bunker_relays only on deserialization error, but after calling
filter_valid_relays it may return an empty Vec when all saved URLs are invalid;
update load_bunker_relays so that after deserializing and calling
filter_valid_relays(urls) you check if the resulting Vec is empty and, if so,
return default_bunker_relays() instead; reference functions: load_bunker_relays,
bunker_relay_config_path, filter_valid_relays, and default_bunker_relays to
locate and implement this empty-result fallback.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
keep-core/Cargo.tomlkeep-core/src/relay.rskeep-desktop/src/app.rskeep-frost-net/Cargo.tomlkeep-frost-net/src/cert_pin.rskeep-frost-net/src/node/mod.rskeep-mobile/src/network.rskeep-nip46/src/bunker.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- keep-frost-net/src/cert_pin.rs
- keep-mobile/src/network.rs
- keep-core/Cargo.toml
- keep-frost-net/src/node/mod.rs
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
is_internal_ip()post-DNS-resolution filter incert_pin.rsto prevent DNS rebinding attacks::priv), and NAT64 WKP (64:ff9b::priv) bypass vectorsallow-internalfeature flag decoupled fromallow-wsso test relays work without disabling SSRF guardsallow-wsinto production builds via workspacedev-dependenciesapp.rs,bunker.rs)keep-coreChanges
is_internal_host()with multicast, 0/8, site-local fec0::/10, IPv4-compatible IPv6, NAT64 WKP; addedis_internal_ip()for resolved addresses; 31 unit tests (+5 new)is_internal_ip()on DNS resultsallow-internalfeatureallow-wsandallow-internalas separatedev-dependenciesfeaturesallow-internalfeature flagkeep-corevalidation, remove duplicated logicTest plan
allow-internalfeature compiles and activates correctly for frost-net test buildsSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores