Centralise outbound URL validation in StrictHTTPClient by JorisHeadease · Pull Request #4246 · nuts-foundation/nuts-node

JorisHeadease · 2026-05-11T16:54:46Z

Closes #4244.

What changed

http/client.StrictHTTPClient.Do and its CheckRedirect policy now validate every outbound URL and every redirect target via core.ParsePublicURLAllowIP (HTTPS-only + no RFC 2606 reserved hostnames in strict mode), instead of each caller duplicating that check.

Per-caller URL validation removed from:

auth/openid4vci.Client
auth/client/iam.HTTPClient.ClientMetadata
auth/client/iam.OpenID4VPClient (6 call sites: PostError, PostAuthorizationResponse, PresentationDefinition, RequestObjectByGet, RequestObjectByPost, AccessToken)
auth/oauth.FetchMetadata / wellKnownCandidates / IssuerIdToWellKnown
auth/services/oauth.relyingParty.RequestRFC003AccessToken (scheme-only check)

All of these routed through *StrictHTTPClient already, so the checks were purely duplicated.

IP-literal hosts are deliberately exempt from this check

core.ParsePublicURL's "hostname is IP" rejection is not part of the centralized check. Blocking IP-literal URLs at this layer would make http.client.allowedinternalcidrs (#4420) unreachable for IP-literal targets — that config option exists precisely to let internal flows target a private address, and the guard that honors it is the dial-time SSRF guard (denyNonPublicAddr), not a URL-string check. So IP addresses are left entirely to that guard, which already runs on every connection (including redirect hops) and is allowlist/denylist-aware.

A new core.ParsePublicURLAllowIP behaves like core.ParsePublicURL except it never rejects an IP-literal host, for this reason.

Breaking API change

oauth.NewRelyingParty, openid4vci.NewClient, oauth.FetchMetadata, oauth.IssuerIdToWellKnown no longer accept a strictMode/strictmode parameter. iam.ClientConfig.StrictMode and iam.OpenID4VPClient's equivalent field are removed. Strict-mode behavior is controlled exclusively by http/client.StrictMode, set at startup from core.ServerConfig.Strictmode.

Side effect: fixes a live gap

auth.Auth.strictMode was a dead field — declared, threaded into iam.ClientConfig.StrictMode and openid4vci.NewClient, but never assigned, so it was always false. That meant the openid4vci and IAM client-metadata URL-validation paths never actually enforced strict-mode SSRF rules regardless of the real strictmode config value. Removing it in favor of the correctly-wired http/client.StrictMode global fixes that.

Out of scope

DNS rebinding and IP-literal SSRF are handled by #4420's dial guard, already merged. This PR only removes the now-redundant string-level validation duplicated across callers.

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

Copilot

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.Do via core.ParsePublicURL, gated by the global http/client.StrictMode.
Enforce URL validation on redirects by wiring CheckRedirect (with a 10-hop cap) into all StrictHTTPClient constructors.
Remove per-caller strict-mode URL checks and adjust APIs/tests/docs accordingly (incl. dropping strictMode from oauth.NewRelyingParty and openid4vci.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.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

reinkrul · 2026-05-22T08:28:11Z

We need to check: I think CRLs are sometimes published on plain HTTP endpoints (instead of HTTPS).

@JorisHeadease could you check that?

…alise-url-validation ; Conflicts: ; auth/openid4vci/client.go ; auth/openid4vci/client_test.go ; vcr/vcr_test.go

StrictHTTPClient.Do and its CheckRedirect now run core.ParsePublicURLAllowIP (HTTPS-only + RFC 2606 reserved-hostname rejection in strict mode) on every outbound request and every redirect hop, instead of each caller duplicating that check. Removes the duplicated validation from auth/openid4vci.Client, auth/client/iam.HTTPClient.ClientMetadata, auth/client/iam.OpenID4VPClient, auth/oauth.FetchMetadata/IssuerIdToWellKnown, and auth/services/oauth.relyingParty, dropping the strictMode parameter each of them threaded through solely for that purpose. IP-literal hosts are deliberately left unchecked by this centralised validation: http/client's dial-time SSRF guard (added in #4420) already polices connecting addresses and honors http.client.allowedinternalcidrs/ deniedcidrs, which a blanket string-level IP check has no visibility into and would make unreachable for IP-literal targets. core.ParsePublicURLAllowIP is a new variant of ParsePublicURL that keeps the HTTPS/reserved-hostname checks but exempts IP literals for this reason. auth.Auth.strictMode was a dead field (never assigned, always false), so the openid4vci and IAM client-metadata paths that read it were effectively never strict-mode-validated at the URL layer regardless of the real strictmode setting. Removing it in favour of the correctly-wired http/client.StrictMode global fixes that as a side effect. Closes #4244.

reinkrul · 2026-08-17T13:01:55Z

CRL fetching (pki/validator.go) doesn't go through StrictHTTPClient at all — it uses http.DefaultTransport directly (see #4422), so it's unaffected by this PR's centralized URL validation. #4422 tracks bringing it onto the shared client with an HTTP-allowed variant; not addressed here to keep this PR to pure deduplication.

qltysh · 2026-08-17T13:02:30Z

4 new issues

Tool	Category	Rule	Count
qlty	Structure	Function with many returns (count = 7): Configure	4

qltysh · 2026-08-17T13:04:29Z

Coverage Impact

This PR will not change total coverage.

Modified Files with Diff Coverage (9)

File	% Diff	Uncovered Line #s
auth/auth.go	100.0%
core/url.go	100.0%
http/client/client.go	100.0%
auth/oauth/types.go	100.0%
auth/oauth/metadata.go	100.0%
auth/client/iam/openid4vp.go	100.0%
auth/services/oauth/relying_party.go	50.0%	58
auth/openid4vci/client.go	100.0%
auth/client/iam/client.go	100.0%
Total	97.2%

🤖 Increase coverage with AI coding...

In the `refactor/4244-centralise-url-validation` branch, add test coverage for this new code:

- `auth/services/oauth/relying_party.go` -- Line 58

🚦 See full report on Qlty Cloud »

🛟 Help

Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.
Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.
File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)
- Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

JorisHeadease requested a review from Copilot May 11, 2026 17:17

Copilot started reviewing on behalf of JorisHeadease May 11, 2026 17:18 View session

Copilot AI reviewed May 11, 2026

View reviewed changes

Comment thread http/client/client_test.go Outdated

Comment thread http/client/client_test.go Outdated

Comment thread vcr/vcr_test.go Outdated

JorisHeadease requested a review from Copilot May 11, 2026 17:48

Copilot started reviewing on behalf of JorisHeadease May 11, 2026 17:49 View session

Copilot AI reviewed May 11, 2026

View reviewed changes

JorisHeadease marked this pull request as ready for review May 11, 2026 18:00

JorisHeadease requested review from gerardsn, reinkrul, stevenvegt and woutslakhorst as code owners May 11, 2026 18:00

reinkrul assigned reinkrul and unassigned reinkrul May 22, 2026

reinkrul added 2 commits August 17, 2026 14:58

Merge remote-tracking branch 'origin/master' into refactor/4244-centr…

aefb460

…alise-url-validation ; Conflicts: ; auth/openid4vci/client.go ; auth/openid4vci/client_test.go ; vcr/vcr_test.go

reinkrul requested a review from Dirklectisch as a code owner August 17, 2026 13:00

reinkrul approved these changes Aug 17, 2026

View reviewed changes

reinkrul merged commit ae207e7 into master Aug 17, 2026
13 checks passed

reinkrul deleted the refactor/4244-centralise-url-validation branch August 17, 2026 13:17

Navigation Menu

Centralise outbound URL validation in StrictHTTPClient - #4246

Centralise outbound URL validation in StrictHTTPClient#4246
reinkrul merged 4 commits into
masterfrom
refactor/4244-centralise-url-validation

JorisHeadease commented May 11, 2026 •

edited by reinkrul

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

reinkrul commented May 22, 2026 •

edited

Loading

Uh oh!

reinkrul commented Aug 17, 2026

Uh oh!

qltysh Bot commented Aug 17, 2026

Uh oh!

qltysh Bot commented Aug 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

JorisHeadease commented May 11, 2026 • edited by reinkrul Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

IP-literal hosts are deliberately exempt from this check

Breaking API change

Side effect: fixes a live gap

Out of scope

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

reinkrul commented May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reinkrul commented Aug 17, 2026

Uh oh!

qltysh Bot commented Aug 17, 2026

4 new issues

Uh oh!

qltysh Bot commented Aug 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JorisHeadease commented May 11, 2026 •

edited by reinkrul

Loading

reinkrul commented May 22, 2026 •

edited

Loading