You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
is_public_http_url (crates/gitlawb-node/src/api/peers.rs:109) classifies a URL by its host string. When the host is an IP literal that works. When it is a name, nothing resolves it, so a hostname whose A or AAAA record points at 127.0.0.1, 169.254.169.254, or RFC1918 space passes every check we have.
POST /api/v1/peers/announce is unauthenticated, and the resulting http_url is used for outbound sync-notify fan-out, so the caller who chooses the name is not required to be anyone in particular.
There is a second half. Validation and connection resolve independently, so even a name that resolves to public space at announce time can resolve somewhere else when the request is actually made. A syntactic gate cannot see that by construction, which is why OWASP's SSRF guidance treats hostname-only validation as insufficient rather than as a partial win.
Why this is the unfinished third step
We have closed SSRF here twice, both times at the syntactic layer. #78 added the host-class gate and the peer prune; #93 and #140 stopped the gossip path following redirects into private space. Neither addressed what a name resolves to. Nothing in the tree records a decision that syntax-only is sufficient, and #333's description states the same conclusion from the other direction, deferring resolved-address validation to the fetch worker that will consume upstream_url.
Fix direction
reqwest::ClientBuilder::dns_resolver takes an Arc<R: Resolve>, and reqwest::dns carries no feature gate, so it is available in our default-features = false build of 0.12.28. A resolver that applies the address policy and drops non-public results closes the name case and the rebinding case together, because the check then happens on the addresses actually used to connect.
Two things to get right:
Reuse the address checks we already have rather than adding a dependency. Extract the IP-classification half of is_public_http_url into a function over IpAddr and call it from both the existing literal path and the resolver. IpAddr::is_global is still nightly-only, so std is not an option at our MSRV, and our hand-rolled ranges are already covered by tests.
Both clients, not one.build_http_client (crates/gitlawb-node/src/main.rs:1173) is the named shared builder, and sync.rs:149 builds a second client of its own. Both set redirect::Policy::none(). A resolver policy attached only to the first leaves the sync worker on the old behavior, which is the same shape as the bug fix(node,gossip): route gossip HTTP through the no-redirect client (#93) #140 fixed.
Keep the string checks. They stay useful as a cheap reject before any lookup happens, and defense in depth is the point; they are just not the boundary.
The narrower predicate gaps in that same function are tracked separately.
is_public_http_url(crates/gitlawb-node/src/api/peers.rs:109) classifies a URL by its host string. When the host is an IP literal that works. When it is a name, nothing resolves it, so a hostname whose A or AAAA record points at127.0.0.1,169.254.169.254, or RFC1918 space passes every check we have.POST /api/v1/peers/announceis unauthenticated, and the resultinghttp_urlis used for outbound sync-notify fan-out, so the caller who chooses the name is not required to be anyone in particular.There is a second half. Validation and connection resolve independently, so even a name that resolves to public space at announce time can resolve somewhere else when the request is actually made. A syntactic gate cannot see that by construction, which is why OWASP's SSRF guidance treats hostname-only validation as insufficient rather than as a partial win.
Why this is the unfinished third step
We have closed SSRF here twice, both times at the syntactic layer. #78 added the host-class gate and the peer prune; #93 and #140 stopped the gossip path following redirects into private space. Neither addressed what a name resolves to. Nothing in the tree records a decision that syntax-only is sufficient, and #333's description states the same conclusion from the other direction, deferring resolved-address validation to the fetch worker that will consume
upstream_url.Fix direction
reqwest::ClientBuilder::dns_resolvertakes anArc<R: Resolve>, andreqwest::dnscarries no feature gate, so it is available in ourdefault-features = falsebuild of 0.12.28. A resolver that applies the address policy and drops non-public results closes the name case and the rebinding case together, because the check then happens on the addresses actually used to connect.Two things to get right:
is_public_http_urlinto a function overIpAddrand call it from both the existing literal path and the resolver.IpAddr::is_globalis still nightly-only, so std is not an option at our MSRV, and our hand-rolled ranges are already covered by tests.build_http_client(crates/gitlawb-node/src/main.rs:1173) is the named shared builder, andsync.rs:149builds a second client of its own. Both setredirect::Policy::none(). A resolver policy attached only to the first leaves the sync worker on the old behavior, which is the same shape as the bug fix(node,gossip): route gossip HTTP through the no-redirect client (#93) #140 fixed.Keep the string checks. They stay useful as a cheap reject before any lookup happens, and defense in depth is the point; they are just not the boundary.
The narrower predicate gaps in that same function are tracked separately.