Centralise outbound URL validation in StrictHTTPClient#4246
Open
JorisHeadease wants to merge 2 commits into
Open
Centralise outbound URL validation in StrictHTTPClient#4246JorisHeadease wants to merge 2 commits into
JorisHeadease wants to merge 2 commits into
Conversation
Move core.ParsePublicURL from per-caller into StrictHTTPClient.Do so every outbound HTTP request gets HTTPS-only + no-IP + no-RFC2606-reserved-host validation in strict mode. Add CheckRedirect to re-validate every redirect target (10-hop cap matches net/http's default). Removes the duplicated validation in auth/openid4vci.Client, auth/client/iam.HTTPClient.ClientMetadata, and the scheme-only check in auth/services/oauth/relying_party.RequestRFC003AccessToken. All production callers route through *StrictHTTPClient. Behavior change: strict mode now rejects IP literals and RFC 2606 reserved hostnames on every outbound HTTP request, not just OpenID4VCI. Breaking API: oauth.NewRelyingParty no longer accepts a strictMode bool parameter; strict-mode behaviour is now controlled exclusively by the http/client.StrictMode flag set by the HTTP engine at startup. Closes #4244
Contributor
There was a problem hiding this comment.
Pull request overview
Centralizes SSRF-oriented outbound URL validation inside http/client.StrictHTTPClient so all outbound HTTP calls (and redirect targets) are consistently validated via core.ParsePublicURL, and removes duplicated per-caller checks across auth-related clients.
Changes:
- Validate every outbound request URL in
StrictHTTPClient.Doviacore.ParsePublicURL, gated by the globalhttp/client.StrictMode. - Enforce URL validation on redirects by wiring
CheckRedirect(with a 10-hop cap) into allStrictHTTPClientconstructors. - Remove per-caller strict-mode URL checks and adjust APIs/tests/docs accordingly (incl. dropping
strictModefromoauth.NewRelyingPartyandopenid4vci.NewClient).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
http/client/client.go |
Adds centralized ParsePublicURL validation and redirect re-validation via CheckRedirect. |
http/client/client_test.go |
Updates strict-mode error expectations and adds coverage for IP/reserved-host blocking + redirects. |
auth/openid4vci/client.go |
Removes per-client URL validation and drops strictMode parameter from NewClient. |
auth/openid4vci/client_test.go |
Updates construction of the OpenID4VCI client to match the new API. |
auth/client/iam/client.go |
Removes per-method URL validation for client metadata retrieval (now inherited from shared HTTP client). |
auth/services/oauth/relying_party.go |
Removes relying-party strictMode parameter/field; relies on shared HTTP client strict mode. |
auth/services/oauth/relying_party_test.go |
Removes tests that exercised relying-party-local scheme validation (now centralized). |
auth/auth.go |
Updates call sites for the changed openid4vci.NewClient and oauth.NewRelyingParty signatures. |
vcr/vcr_test.go |
Updates strict-mode error assertions to match new centralized validation errors. |
docs/pages/deployment/configuration.rst |
Documents stricter outbound URL validation (IP literals + RFC2606 reserved hosts) and redirect enforcement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Apply the capture-and-restore t.Cleanup pattern to tests that mutate the package-level StrictMode and DefaultCachingTransport globals so they no longer leak state into subsequent tests. Adds a withClientGlobals helper in http/client/client_test.go to avoid repeating the boilerplate. Also fixes a pre-existing DefaultCachingTransport leak in TestCaching.
JorisHeadease
requested review from
gerardsn,
reinkrul,
stevenvegt and
woutslakhorst
as code owners
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.
Closes #4244.
What changed
http/client.StrictHTTPClient.Donow validates every outbound URL viacore.ParsePublicURL, gated by the globalhttp/client.StrictModeflag.CheckRedirectis wired on all three constructors so every redirect target is re-validated (10-hop cap matchesnet/http's default).auth/openid4vci.Client,auth/client/iam.HTTPClient.ClientMetadata, and the scheme-only check inauth/services/oauth/relying_party.RequestRFC003AccessToken— all production callers route through*StrictHTTPClient.Behavior change
Strict mode now rejects IP literals (e.g.
https://10.0.0.1/) and RFC 2606 reserved hostnames (*.localhost,*.test,example.com/net/org, etc.) on every outbound HTTP request, not just OpenID4VCI. Previously only the HTTPS-scheme check was enforced for most callers. Deployments using IP hosts or reserved TLDs as Nuts service endpoints will need real DNS names. Redirects to such targets are also blocked.Breaking API change
oauth.NewRelyingPartyno longer accepts astrictMode boolparameter. Strict-mode behaviour is now controlled exclusively byhttp/client.StrictMode, which the HTTP engine sets at startup fromcore.ServerConfig.Strictmode.A backward-compatible stub was considered and rejected: silently ignoring the argument would create a footgun where callers pass
strictMode=truebut get whatever the global flag happens to be. External consumers can update by removing the argument from the call site.Out of scope
DNS rebinding and public hostnames resolving to private IPs (e.g.
10.0.0.1.nip.io,metadata.google.internal) are not defended —core.ParsePublicURLvalidates the URL string, not the resolved address. Closing this gap requires a customDialContextand should be tracked separately.