chore(http): refuse redirects on spec-defined fetches, parse max-age strictly - #775
Merged
Conversation
…strictly Four outbound fetches build an SSRF-hardened client; only two set a redirect policy. The pinned dialer already makes a redirect unable to reach a new host, so this is consistency, not a hole — but a policy present at half the call sites is one relaxed dialer away from being one. Applied to the two that retrieve a spec-defined resource which must be AT the URL named: - backchannel_logout: Go re-issues a 302'd POST as a bodyless GET, so a redirecting RP was already not receiving the logout. Now visible. - clientmetadata: a hop away from the client_id URL serves a document for a different identifier. Already rejected incidentally by the client_id-must-match-its-URL check; now rejected deterministically, matching the sibling JWKS fetch. Set on a copy of the client rather than in place: fetchViaClient is also the injected-test-client seam, and mutating a caller's value is a side effect whichever path is hotter. The copy keeps production and the test seam on one policy. Left following redirects, with the reason recorded at both sites: webhook delivery and the admin test-endpoint probe. Those URLs are the operator's own, a same-host path redirect is followed today and its final status recorded in WebhookLog.HttpStatus, and changing that would alter recorded behaviour for existing deployments to close nothing. Separately, cacheTTL used fmt.Sscanf, which stops at the first non-digit and still reports success — "max-age=600junk" parsed as 600. strconv.Atoi rejects it. The clamp already made every outcome safe, so no live defect.
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
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.
Two hardening items from the same review as #774. Neither is a live
vulnerability — stated plainly so nobody reads more into it than is there.
F3 — redirect policy at 4 of 8 SSRF-client call sites
validators.safeHTTPClientpinsDialContextto the validated IP for everydial on the transport, so a redirect cannot reach a new host: it would
re-issue the request against the original address carrying a foreign Host
header. The gap is consistency, not exposure — but a policy present at half the
call sites is one relaxed dialer away from being a real one.
Applied to the two fetches that retrieve a spec-defined resource which must be
at the URL named:
internal/token/backchannel_logout.go— Go re-issues a 302'd POST as abodyless GET, so an RP whose
backchannel_logout_uriredirects was alreadysilently not processing the logout. This turns a success-that-did-nothing into
a visible non-2xx.
internal/clientmetadata/clientmetadata.go— a hop away from theclient_idURL serves a document for a different identifier. This was already rejected
incidentally by the
client_id-must-match-its-URL check; the change makes therejection deterministic and matches the sibling JWKS fetch.
Not applied, with the reason recorded in a comment at both sites:
events.webhookHTTPClientandadmin_webhooksTestEndpoint. Those URLs are theoperator's own, a same-host path redirect is followed today with its final
status recorded in
WebhookLog.HttpStatus, and refusing them would change thatrecorded status for existing deployments to close nothing. The test-endpoint
probe must also match delivery exactly, or it reports failures the live webhook
would not hit.
One implementation note
The policy is set on a copy of the client, not in place.
fetchViaClientisalso the
SetHTTPClientForTestseam, so setting the field directly would mutatea client the caller owns — and would mean the test seam no longer exercises the
production policy.
TestFetchViaClientDoesNotMutateTheCallersClientpins that.F5 —
cacheTTLaccepted trailing garbagefmt.Sscanf(v, "%d", &secs)stops at the first non-digit and still reportssuccess, so
max-age=600junkparsed as 600.strconv.Atoirejects it and thevalue falls back to the floor. The clamp already made every outcome safe, so
there is no live defect here — the parse just should not be half-reading a
header written by the party being validated.
Verification
TestFetchRefusesRedirectsand the newTestCacheTTLIsClampedrows confirmedfailing with the source reverted and the tests kept.
Stacked independently of #774 — both branch from
mainand touch disjoint files.