fix(security): reject non-public peer URLs + prune poisoned peers#78
Merged
Conversation
The unauthenticated POST /api/v1/peers/announce only validated the http(s) scheme, so anyone could register loopback/private 'peers' (localhost:5432, localhost:22, internal admin URLs). On every push the node fans out signed sync-notify POSTs to all peers — turning this into an SSRF probe and burying the real peers under junk so node-origin repos stopped replicating. Observed on node.gitlawb.com: 126/194 peer rows were injected junk (fake DIDs z6MkScan…/z6MkAdmin…, localhost targets). - is_public_http_url(): reject non-http(s), loopback, unspecified, private (RFC1918 + ULA), link-local, and localhost/.local/.internal hosts. Applied at announce time. - db.prune_non_public_peers(): boot-time sweep removing already-injected rows, run from main() alongside prune_self_peers — self-heals poisoned tables on deploy. - tests for the validator (accept public, reject loopback/private/internal, incl. IPv6 [::1]). Co-Authored-By: OpenClaude <openclaude@gitlawb.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPeer announcement now validates that announced peer URLs are safe public HTTP endpoints using ChangesPeer URL Validation and SSRF Mitigation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
Thanks for the contribution. A couple of things will help us review this faster:
See CONTRIBUTING.md. Update the PR and these notes will clear automatically. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/gitlawb-node/src/api/peers.rs (1)
391-410: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd test coverage for IPv4-mapped IPv6 and CGNAT ranges.
The test suite should cover the IPv4-mapped IPv6 bypass vectors and the CGNAT range (100.64.0.0/10) once those checks are implemented.
🧪 Suggested additional test cases
"http://[::1]:7545", + "http://[::ffff:127.0.0.1]:7545", // IPv4-mapped loopback + "http://[::ffff:10.0.0.1]:7545", // IPv4-mapped private + "http://[::ffff:192.168.1.1]:7545", // IPv4-mapped private + "http://100.64.0.1:7545", // CGNAT range + "http://100.127.255.254:7545", // CGNAT range upper bound "http://gitlawb-node.internal:7545",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/api/peers.rs` around lines 391 - 410, The rejects_loopback_private_and_internal test function is missing test coverage for IPv4-mapped IPv6 addresses and CGNAT range (100.64.0.0/10) addresses. Add additional test cases to the bad array that includes IPv4-mapped IPv6 format URLs (such as http://[::ffff:192.168.1.1]:7545) and CGNAT range URLs (such as http://100.64.0.1:7545) to ensure the is_public_http_url function correctly rejects these address ranges.crates/gitlawb-node/src/db/mod.rs (1)
1610-1623: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider batching deletes for large poisoned peer sets.
The current implementation issues one
DELETEper poisoned peer. For the reported 126 junk rows, this means 126 round-trips. A batched approach would reduce this, though it's low priority since this runs once at boot and subsequent runs should find few/no rows.♻️ Optional: batch delete with collected DIDs
pub async fn prune_non_public_peers(&self) -> Result<u64> { let peers = self.list_peers().await?; - let mut removed = 0u64; - for p in peers { - if !crate::api::peers::is_public_http_url(&p.http_url) { - sqlx::query("DELETE FROM peers WHERE did = $1") - .bind(&p.did) - .execute(&self.pool) - .await?; - removed += 1; - } + let bad_dids: Vec<&str> = peers + .iter() + .filter(|p| !crate::api::peers::is_public_http_url(&p.http_url)) + .map(|p| p.did.as_str()) + .collect(); + if bad_dids.is_empty() { + return Ok(0); } - Ok(removed) + let result = sqlx::query("DELETE FROM peers WHERE did = ANY($1)") + .bind(&bad_dids) + .execute(&self.pool) + .await?; + Ok(result.rows_affected()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/db/mod.rs` around lines 1610 - 1623, The prune_non_public_peers function executes individual DELETE queries inside the loop for each non-public peer, resulting in multiple database round-trips. Instead, collect all the DIDs that need to be deleted into a Vec during the iteration, and after the loop completes, execute a single DELETE query using an IN clause with all collected DIDs. This will reduce the number of database operations from N to 1 for N deletions, improving performance when pruning large sets of non-public peers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gitlawb-node/src/api/peers.rs`:
- Around line 69-86: The IPv6 validation logic in the match statement for
std::net::IpAddr::V6 currently only checks the first segment for ULA and
link-local prefixes, but this allows IPv4-mapped IPv6 addresses (like
::ffff:127.0.0.1) to bypass private IP checks since the private IPv4 address is
embedded in the later segments. Additionally, the IPv4 validation in the
std::net::IpAddr::V4 branch uses is_private() which does not include the CGNAT
range (100.64.0.0/10). To fix this, add a check in the V6 branch to detect
IPv4-mapped IPv6 addresses by verifying if the first six segments equal 0 and
the seventh segment equals 0xffff, then extract and validate the embedded IPv4
address using the same checks as the V4 branch. Also extend the IPv4 branch to
include a CGNAT range check for addresses between 100.64.0.0 and
100.127.255.255.
---
Nitpick comments:
In `@crates/gitlawb-node/src/api/peers.rs`:
- Around line 391-410: The rejects_loopback_private_and_internal test function
is missing test coverage for IPv4-mapped IPv6 addresses and CGNAT range
(100.64.0.0/10) addresses. Add additional test cases to the bad array that
includes IPv4-mapped IPv6 format URLs (such as http://[::ffff:192.168.1.1]:7545)
and CGNAT range URLs (such as http://100.64.0.1:7545) to ensure the
is_public_http_url function correctly rejects these address ranges.
In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 1610-1623: The prune_non_public_peers function executes individual
DELETE queries inside the loop for each non-public peer, resulting in multiple
database round-trips. Instead, collect all the DIDs that need to be deleted into
a Vec during the iteration, and after the loop completes, execute a single
DELETE query using an IN clause with all collected DIDs. This will reduce the
number of database operations from N to 1 for N deletions, improving performance
when pruning large sets of non-public peers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8aa581d1-b4eb-4ac7-b154-d659c417350f
📒 Files selected for processing (3)
crates/gitlawb-node/src/api/peers.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/main.rs
beardthelion
added
kind:security
beardthelion
requested changes
Jun 22, 2026
Collaborator
There was a problem hiding this comment.
The peer-validation approach is right and the boot-time prune is a good call. Two things block it: it doesn't compile, and input validation alone doesn't close the SSRF here, a redirect walks right through it. Findings highest-first.
Findings
- [P1] Strip the mis-based storage wiring from
main.rs, it doesn't compile.crates/gitlawb-node/src/main.rs:19,167carriesmod storage;/storage::build()/ theRepoStore::newarg change from the storage refactor without thestoragemodule itself, so the build fails withfailed to resolve mod 'storage': .../storage.rs does not exist. The only main.rs change this PR needs is theprune_non_public_peersblock at:104. Drop the rest, or rebase once the storage work has landed. - [P1] Reject redirects on the peer-facing clients, otherwise the validation is bypassable.
crates/gitlawb-node/src/main.rs:145,crates/gitlawb-node/src/sync.rs:73, and the barereqwest::getatcrates/gitlawb-node/src/api/peers.rs:366all use reqwest's default redirect policy (Policy::limited(10)), with noredirect::Policyset anywhere. A peer that passesis_public_http_urlwith a genuine public host can answer302 Location: http://127.0.0.1/(or169.254.169.254), and the node follows it to the internal target, replaying its signedSignatureheaders on the way. This pre-dates the PR, but it means announce-time validation can't stand alone. Setredirect::Policy::none()on the peer-facing clients (this fails closed on a peer that 301s http to https, so peers should announce their final https URL). Closing DNS rebinding on top of that means a connect-time guard, which isn't a one-liner here: a customdns::Resolveimpl that resolves the host and rejects any resolved address in the private/loopback/link-local/CGNAT ranges. The git clone/fetch subprocess follows redirects independently, worth pinning as well. - [P2] Validate inside
upsert_peer, not just the announce handler. Thepeerstable has two writers: the gated announce (crates/gitlawb-node/src/api/peers.rs:162) and the bootstrap announce-back atcrates/gitlawb-node/src/main.rs:538, which inserts a bootstrap peer's self-reportednode_urlwith no check. That second path re-poisons the table seconds after the boot prune runs (a malicious or misconfigured bootstrap peer can return an internal URL). Moving the check intoupsert_peercovers both writers. While there, tighten the predicate:http://[::ffff:127.0.0.1]and other IPv4-mapped/compatible IPv6,http://[::ffff:169.254.169.254],http://100.64.0.1(CGNAT), andhttp://localhost.(trailing dot) all register as public today. Checkis_loopback()/is_unspecified()on the parsed IP first (so[::1]stays caught), then fold IPv4-mapped/compatible v6 to v4 (to_ipv4_mapped(), falling back toto_ipv4()) and run the existing v4 private/link-local rules plus CGNAT100.64.0.0/10on the result; strip a trailing dot before the host comparison. Do the loopback check before folding,to_ipv4()maps::1to0.0.0.1which would otherwise re-open loopback. (is_global()would read cleaner but it's unstable behindfeature(ip), so on stable the explicit checks are the way.) - [P2] Make the boot prune all-or-nothing.
crates/gitlawb-node/src/db/mod.rs:1618issues oneDELETEper non-public row inside the loop with?, so the first transient failure abandons every remaining poisoned row while the caller logs it as non-fatal. Collect the failures and continue rather than?-ing out (or, if your sqlx/Postgres setup supports it, a singleDELETE FROM peers WHERE did = ANY($1)) so the sweep is complete.
Two related findings are out of scope for this PR but the same class, filed separately so they don't get lost: webhook delivery is an unvalidated SSRF sink with no ownership check (#81), and POST /api/v1/sync/trigger is unauthenticated by default (#82).
Contributor
Author
|
closes #85 |
…on gaps Resolves the P1/P2 review feedback on the peer-announce SSRF fix: - main.rs: revert the unrelated, incomplete `storage::build()` refactor back to the tigris init from main (the `storage` module never existed, breaking the build). Keeps only the prune_non_public_peers boot call. - Redirect bypass: set redirect::Policy::none() on all peer-facing reqwest clients (shared http_client, sync worker, ping_peer) so a peer cannot answer `302 -> http://127.0.0.1/` and bypass the public-URL check. ping_peer now reuses the shared no-redirect client instead of bare reqwest::get. - Move the public-URL guard into db::upsert_peer so the bootstrap announce-back path (main.rs) can no longer re-poison the table after the prune. - is_public_http_url: strip trailing-dot FQDNs (`localhost.`), fold IPv4-mapped/compatible IPv6 (`::ffff:127.0.0.1`, `::127.0.0.1`) to IPv4 before range checks, and reject CGNAT (100.64.0.0/10). Added tests for each. - prune_non_public_peers: collect bad DIDs then DELETE ... WHERE did = ANY($1) in one statement, so a transient error no longer abandons remaining rows. Follow-up (separate issue): DNS-resolution rebinding guard rejecting peer hostnames that resolve to private IPs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/gitlawb-node/src/api/peers.rs (3)
64-68: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject
*.localhosthostnames as local too.
node.localhostcurrently passes because only exactlocalhostis blocked. Treat the whole.localhostspecial-use suffix like loopback before accepting peer URLs.Suggested patch
if host.is_empty() || host == "localhost" + || host.ends_with(".localhost") || host.ends_with(".local") || host.ends_with(".internal") {// Trailing-dot FQDN of an internal host. "http://localhost./", + "http://node.localhost/", + "http://node.localhost./", // CGNAT (100.64.0.0/10).Also applies to: 441-452
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/api/peers.rs` around lines 64 - 68, The localhost validation in this code block only rejects the exact hostname "localhost" using the equality check host == "localhost", which allows subdomains like "node.localhost" to pass through. Add an additional condition using host.ends_with(".localhost") to the existing if statement that already checks for ".local" and ".internal" suffixes, so that any hostname ending with the special-use .localhost suffix is properly treated as a local address and rejected.
95-112: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDo not classify special-use IP ranges as public peers.
The current literal-IP denylist still accepts non-global ranges such as TEST-NET
203.0.113.0/24— Line 421 explicitly expects one to pass — plus other special-use IPv4/IPv6 ranges like benchmarking, multicast, reserved, and documentation blocks. Those rows are not reachable public peers and can still poison/crowd the peer table; use a complete “globally routable” helper and move203.0.113.10into rejection coverage.Also applies to: 421-421
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/api/peers.rs` around lines 95 - 112, The current IP validation in the match statement for std::net::IpAddr::V4 and std::net::IpAddr::V6 only checks for a limited set of special-use ranges (private, link-local, CGNAT for IPv4 and unique-local, link-local for IPv6) but misses other special-use ranges like TEST-NET (203.0.113.0/24), benchmarking, multicast, reserved, and documentation blocks. Replace the manual range checks with Rust's built-in is_global() method for both IPv4 and IPv6 addresses, which comprehensively validates that an address is globally routable and rejects all special-use ranges. This ensures that non-routable addresses are properly excluded from the peer table.
73-115: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftValidate resolved peer addresses at dial time.
Non-IP hostnames skip the IP checks and return
true, thenstate.http_client.get(&url).send()resolves them later. A public-looking DNS name can point or rebind to127.0.0.1/RFC1918 and still bypass this SSRF guard. Add a peer dial path that resolves and rejects every non-publicSocketAddrimmediately before connecting, ideally pinning the checked address for the request rather than re-resolving the hostname.Also applies to: 393-399
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/api/peers.rs` around lines 73 - 115, The current validation only checks IP address literals and skips validation for hostnames (non-IP addresses), returning true immediately and allowing them to proceed. This creates an SSRF vulnerability since hostnames will be resolved later during the HTTP client request and could resolve to loopback or private addresses. Create a dedicated peer dial function that resolves hostnames to their socket addresses at connection time, applies the same IP validation checks (loopback, unspecified, private range, link-local, CGNAT, IPv6 unique-local, and IPv6 link-local) to each resolved address before connecting, and pins the resolved address for the request to prevent DNS rebinding attacks. Ensure both the initial validation function and the new dial path work together to comprehensively block non-public addresses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gitlawb-node/src/sync.rs`:
- Around line 76-80: The fallback in the reqwest client builder creation is not
preserving the redirect security policy. In the `.unwrap_or_else(|_|
reqwest::Client::new())` call, replace the fallback behavior to either panic on
builder failure or reconstruct the client with the same `Policy::none()`
redirect policy instead of using `reqwest::Client::new()` which reverts to
default (insecure) redirect handling. This ensures that if the client builder
fails, the SSRF protection is maintained rather than silently reverting to a
less secure default.
---
Outside diff comments:
In `@crates/gitlawb-node/src/api/peers.rs`:
- Around line 64-68: The localhost validation in this code block only rejects
the exact hostname "localhost" using the equality check host == "localhost",
which allows subdomains like "node.localhost" to pass through. Add an additional
condition using host.ends_with(".localhost") to the existing if statement that
already checks for ".local" and ".internal" suffixes, so that any hostname
ending with the special-use .localhost suffix is properly treated as a local
address and rejected.
- Around line 95-112: The current IP validation in the match statement for
std::net::IpAddr::V4 and std::net::IpAddr::V6 only checks for a limited set of
special-use ranges (private, link-local, CGNAT for IPv4 and unique-local,
link-local for IPv6) but misses other special-use ranges like TEST-NET
(203.0.113.0/24), benchmarking, multicast, reserved, and documentation blocks.
Replace the manual range checks with Rust's built-in is_global() method for both
IPv4 and IPv6 addresses, which comprehensively validates that an address is
globally routable and rejects all special-use ranges. This ensures that
non-routable addresses are properly excluded from the peer table.
- Around line 73-115: The current validation only checks IP address literals and
skips validation for hostnames (non-IP addresses), returning true immediately
and allowing them to proceed. This creates an SSRF vulnerability since hostnames
will be resolved later during the HTTP client request and could resolve to
loopback or private addresses. Create a dedicated peer dial function that
resolves hostnames to their socket addresses at connection time, applies the
same IP validation checks (loopback, unspecified, private range, link-local,
CGNAT, IPv6 unique-local, and IPv6 link-local) to each resolved address before
connecting, and pins the resolved address for the request to prevent DNS
rebinding attacks. Ensure both the initial validation function and the new dial
path work together to comprehensively block non-public addresses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d647f2bd-d835-4037-a18a-b1777ddcb130
📒 Files selected for processing (4)
crates/gitlawb-node/src/api/peers.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/sync.rs
… failure - peers.rs: collapse the CGNAT/private check onto one line (cargo fmt --check). - sync.rs: replace the unwrap_or_else(reqwest::Client::new()) fallback with .expect(). The default client follows redirects, so the fallback silently reintroduced the SSRF vector Policy::none() exists to close (CodeRabbit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
coderabbitai please review |
This was referenced Jun 24, 2026
kevincodex1
pushed a commit
that referenced
this pull request
Jul 3, 2026
…) (#140) gossip_task built its own reqwest::Client::new(), whose default redirect policy follows up to 10 redirects, and used it for the bootstrap announce and the periodic peer /health ping. A peer host (public at announce time, per is_public_http_url) could answer /health with 302 Location: http://169.254.169.254/... and the node would follow it into cloud metadata or internal addresses. Blind SSRF: only request status feeds mark_peer_ping. Reuse the shared state.http_client (Policy::none, already on AppState), matching sync.rs and the announce fan-out hardened in #78. Extract ping_peer_health so the redirect-not-followed behavior is covered by a mockito test (302 to an internal target registered expect(0), asserted not hit).
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.
The unauthenticated POST /api/v1/peers/announce only validated the http(s) scheme, so anyone could register loopback/private 'peers' (localhost:5432, localhost:22, internal admin URLs). On every push the node fans out signed sync-notify POSTs to all peers — turning this into an SSRF probe and burying the real peers under junk so node-origin repos stopped replicating. Observed on node.gitlawb.com: 126/194 peer rows were injected junk (fake DIDs z6MkScan…/z6MkAdmin…, localhost targets).
Closes #85
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes / Security
Tests