OIDC Test Connection: structured per-check diagnostic [shot:oidc-test-connection-structured-diagnostic] (bookshelf-tm38.15) #1396

Merged

zombor merged 4 commits from bd-bookshelf-tm38.15 into main 2026-08-08 16:01:54 +00:00

Conversation 3 Commits 4 Files changed 14 +981 -35

zombor commented 2026-08-08 02:25:29 +00:00

Owner

Copy link

Summary

• Expands the existing pass/fail OIDC Test Connection into a structured, per-check diagnostic mirroring Grimmory's green/yellow/red model: discovery document, authorization/token/userinfo endpoints, JWKS signing keys, requested scopes vs scopes_supported, PKCE (S256), end-session endpoint (single logout), and back-channel logout support.
• Each row reports pass/warn/fail/skip with a short reason. warn/skip never flip overall success — only a hard fail (discovery unreachable, missing required endpoints, or JWKS unreachable/empty) does, matching the "yellow = optional capability missing but OIDC still works" framing.
• The response always includes the exact computed redirect_uri — the one thing this diagnostic cannot verify itself (IdP-side registration), and the most common cause of a login failure even when every check passes.
• Rendered as a per-row list in the OIDC settings page, reusing the canonical .email-settings-row list shape and .badge pill styling (no bespoke classes, no inline style=).
• Docs: updated docs/content/docs/administering/oidc.md with a new "Test Connection diagnostic" section explaining each check and the redirect-URI hint.

Test plan

• make test — full unit suite green
• make coverage — 100% gate green (Docker + MySQL testcontainers)
• make lint — 0 issues, all policy checks green
• npx vitest run --coverage — full JS suite green, 100% coverage including new controller tests
• go build -tags e2e ./e2e/... — compiles; extended the existing OIDC journey It (journey_settings_test.go) to assert the structured diagnostic renders a failed "Discovery document" row against an unreachable issuer, and to capture a screenshot ([shot:...] marker in the PR title triggers CI auto-post)

Closes bead bookshelf-tm38.15 on merge.

## Summary - Expands the existing pass/fail OIDC Test Connection into a structured, per-check diagnostic mirroring Grimmory's green/yellow/red model: discovery document, authorization/token/userinfo endpoints, JWKS signing keys, requested scopes vs `scopes_supported`, PKCE (S256), end-session endpoint (single logout), and back-channel logout support. - Each row reports pass/warn/fail/skip with a short reason. `warn`/`skip` never flip overall success — only a hard `fail` (discovery unreachable, missing required endpoints, or JWKS unreachable/empty) does, matching the "yellow = optional capability missing but OIDC still works" framing. - The response always includes the exact computed `redirect_uri` — the one thing this diagnostic cannot verify itself (IdP-side registration), and the most common cause of a login failure even when every check passes. - Rendered as a per-row list in the OIDC settings page, reusing the canonical `.email-settings-row` list shape and `.badge` pill styling (no bespoke classes, no inline `style=`). - Docs: updated `docs/content/docs/administering/oidc.md` with a new "Test Connection diagnostic" section explaining each check and the redirect-URI hint. ## Test plan - [x] `make test` — full unit suite green - [x] `make coverage` — 100% gate green (Docker + MySQL testcontainers) - [x] `make lint` — 0 issues, all policy checks green - [x] `npx vitest run --coverage` — full JS suite green, 100% coverage including new controller tests - [x] `go build -tags e2e ./e2e/...` — compiles; extended the existing OIDC journey `It` (`journey_settings_test.go`) to assert the structured diagnostic renders a failed "Discovery document" row against an unreachable issuer, and to capture a screenshot (`[shot:...]` marker in the PR title triggers CI auto-post) Closes bead bookshelf-tm38.15 on merge.

zombor added 1 commit 2026-08-08 02:25:30 +00:00

feat(settings): structured per-check OIDC Test Connection diagnostic ... 8dbc667a71

Expand the OIDC Test Connection endpoint from a single pass/fail into a
structured, per-check diagnostic mirroring Grimmory's green/yellow/red model:
discovery document, authorization/token/userinfo endpoints, JWKS signing
keys, requested scopes vs discovery's scopes_supported, PKCE (S256), the
end-session endpoint (single logout), and back-channel logout support.
Each row reports pass/warn/fail/skip with a short reason, and the response
always includes the exact redirect_uri to register with the identity
provider — the one thing this diagnostic cannot verify itself, and the most
common cause of a login failure even when every check passes.

Renders as a per-row list in the OIDC settings page (reusing the existing
.email-settings-row list shape and .badge pill styling), driven by the
existing Stimulus oidc-settings controller.

Closes bead bookshelf-tm38.15.

zombor commented 2026-08-08 02:40:26 +00:00

Author

Owner

Copy link

[BLOCKER] internal/settings/oidc_settings.go:611-628 (checkOIDCJWKS) / internal/settings/wire.go:281 — jwks_uri from the discovery document is fetched with zero SSRF validation
TestOIDCConnectionResult validates the admin-supplied issuer_uri via validateIssuerURI/isRestrictedHost (blocks private/loopback/link-local/AWS-metadata literal IPs) before calling discoverMeta. But the discovery response itself is attacker-influenced content: meta.JWKSURI comes straight from the JSON the issuer host returns, and this PR wires a brand-new server-side fetch of that URL (users.FetchJWKS, via checkOIDCJWKS) with no host/scheme validation at all — not isRestrictedHost, not even a scheme check. An admin (or anyone who can get an admin to run Test Connection against an attacker-controlled/compromised issuer — this is not the saved/trusted config, it's whatever is currently typed into the issuer field per oidc_settings_controller.js's testConnection()) can serve a discovery document with "jwks_uri": "http://169.254.169.254/latest/meta-data/iam/security-credentials/..." or "http://127.0.0.1:9000/..." or any RFC1918 address, and pergamum's server will make that request. This is the exact SSRF class isRestrictedHost/validateIssuerURI was written to prevent for the issuer field (see the "admin-only SSRF via the test-connection handler" comments at oidc_settings.go:481,511) — but the protection was never extended to the derived jwks_uri, so it's trivially bypassed via a second hop. The response isn't fully blind either: checkOIDCJWKS returns distinguishable outcomes ("could not fetch signing keys" / "JWKS response contains no usable keys" / "N signing key(s) available") that give an oracle for probing reachability/behavior of internal hosts and ports from outside.
Fix: run meta.JWKSURI (and ideally AuthorizationEndpoint/TokenEndpoint/UserinfoEndpoint/EndSessionEndpoint, all of which are also unvalidated attacker-influenced discovery fields, even though only JWKS is fetched today) through the same validateIssuerURI/isRestrictedHost check before calling fetchJWKS, and fail the check with a generic "invalid signing-key endpoint" detail (no raw URL) rather than attempting the fetch.

[MINOR] internal/users/oidc_jwks.go:38-96 (DiscoverOIDCMeta / FetchJWKS, pre-existing/unchanged) — shared oidcHTTPClient has no CheckRedirect, so a validated-at-request-time https issuer can still redirect the discovery/JWKS fetch to an internal target via a 3xx response, bypassing the literal-IP check entirely (redirect target isn't re-validated). Not introduced by this PR, but this PR is what makes the JWKS leg of that client reachable from an unauthenticated-of-full-OAuth-flow admin action (Test Connection) instead of only from a completed login callback. Worth capping CheckRedirect (deny or re-validate each hop) while addressing the BLOCKER above, same call site.

[MINOR] internal/settings/oidc_settings.go:508-513 (isRestrictedHost, pre-existing) — the doc comment already flags that literal-IP-only checking doesn't catch DNS rebinding (hostname resolves to a private IP at request time, not URL-parse time). Not a regression from this PR, but since the fix for the BLOCKER above will likely reuse isRestrictedHost for jwks_uri, worth tracking as a follow-up: resolve-then-check (or use a custom DialContext that rejects private-IP connections at dial time) for full protection instead of syntactic-IP filtering.

REVIEW VERDICT: 1 blocker, 0 major, 2 minor

[BLOCKER] internal/settings/oidc_settings.go:611-628 (checkOIDCJWKS) / internal/settings/wire.go:281 — jwks_uri from the discovery document is fetched with zero SSRF validation `TestOIDCConnectionResult` validates the admin-supplied `issuer_uri` via `validateIssuerURI`/`isRestrictedHost` (blocks private/loopback/link-local/AWS-metadata literal IPs) before calling `discoverMeta`. But the discovery *response* itself is attacker-influenced content: `meta.JWKSURI` comes straight from the JSON the issuer host returns, and this PR wires a brand-new server-side fetch of that URL (`users.FetchJWKS`, via `checkOIDCJWKS`) with **no host/scheme validation at all** — not `isRestrictedHost`, not even a scheme check. An admin (or anyone who can get an admin to run Test Connection against an attacker-controlled/compromised issuer — this is *not* the saved/trusted config, it's whatever is currently typed into the issuer field per `oidc_settings_controller.js`'s `testConnection()`) can serve a discovery document with `"jwks_uri": "http://169.254.169.254/latest/meta-data/iam/security-credentials/..."` or `"http://127.0.0.1:9000/..."` or any RFC1918 address, and pergamum's server will make that request. This is the exact SSRF class `isRestrictedHost`/`validateIssuerURI` was written to prevent for the issuer field (see the "admin-only SSRF via the test-connection handler" comments at oidc_settings.go:481,511) — but the protection was never extended to the derived `jwks_uri`, so it's trivially bypassed via a second hop. The response isn't fully blind either: `checkOIDCJWKS` returns distinguishable outcomes ("could not fetch signing keys" / "JWKS response contains no usable keys" / "N signing key(s) available") that give an oracle for probing reachability/behavior of internal hosts and ports from outside. Fix: run `meta.JWKSURI` (and ideally `AuthorizationEndpoint`/`TokenEndpoint`/`UserinfoEndpoint`/`EndSessionEndpoint`, all of which are also unvalidated attacker-influenced discovery fields, even though only JWKS is fetched today) through the same `validateIssuerURI`/`isRestrictedHost` check before calling `fetchJWKS`, and fail the check with a generic "invalid signing-key endpoint" detail (no raw URL) rather than attempting the fetch. [MINOR] internal/users/oidc_jwks.go:38-96 (DiscoverOIDCMeta / FetchJWKS, pre-existing/unchanged) — shared `oidcHTTPClient` has no `CheckRedirect`, so a validated-at-request-time https issuer can still redirect the discovery/JWKS fetch to an internal target via a 3xx response, bypassing the literal-IP check entirely (redirect target isn't re-validated). Not introduced by this PR, but this PR is what makes the JWKS leg of that client reachable from an unauthenticated-of-full-OAuth-flow admin action (Test Connection) instead of only from a completed login callback. Worth capping `CheckRedirect` (deny or re-validate each hop) while addressing the BLOCKER above, same call site. [MINOR] internal/settings/oidc_settings.go:508-513 (isRestrictedHost, pre-existing) — the doc comment already flags that literal-IP-only checking doesn't catch DNS rebinding (hostname resolves to a private IP at request time, not URL-parse time). Not a regression from this PR, but since the fix for the BLOCKER above will likely reuse `isRestrictedHost` for `jwks_uri`, worth tracking as a follow-up: resolve-then-check (or use a custom `DialContext` that rejects private-IP connections at dial time) for full protection instead of syntactic-IP filtering. REVIEW VERDICT: 1 blocker, 0 major, 2 minor

zombor added 1 commit 2026-08-08 02:48:10 +00:00

fix(security): close SSRF via discovery-derived OIDC jwks_uri ... 956b93a215

The Test Connection diagnostic's checkOIDCJWKS fetched the JWKS URI
returned by the issuer's discovery response with no host/scheme
validation. Since the diagnostic fetches the issuer typed into the
admin form (not the trusted saved config), a malicious/compromised
issuer could point jwks_uri at an internal service (e.g. the cloud
metadata address or an RFC1918 host) and use the check's pass/fail
outcome as an SSRF oracle.

- internal/settings/oidc_settings.go: validate jwks_uri (https scheme,
  not a restricted-host IP literal) before ever fetching it, mirroring
  the existing issuer-URI validation. Rejection detail stays generic —
  never echoes the URI/host back to the caller.
- internal/users/oidc_jwks.go: the shared oidcHTTPClient (used for
  discovery, JWKS, token, and userinfo requests) now refuses to follow
  any redirect, closing the SSRF bypass a malicious 3xx response could
  otherwise use against pre-fetch host validation.
- Regression tests: jwks_uri pointing at loopback/metadata/RFC1918
  addresses is rejected without ever calling fetchJWKS, with no
  leakage of the rejected URI in the check detail; a JWKS redirect is
  not followed.

Filed bookshelf-qapga as a follow-up to port cover's dial-time
safe-transport pattern (DNS-rebinding protection) onto oidcHTTPClient.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016tRKybTpfjQ4SxmNdVFLHi

zombor referenced this pull request 2026-08-08 02:50:50 +00:00

feat(users): OIDC RP-initiated single logout [shot:settings] (bookshelf-tm38.9) #1398

zombor added 1 commit 2026-08-08 02:55:58 +00:00

fix(settings): satisfy 100% coverage gate for jwks_uri validation ... 70ea049ba3

- Remove the dead raw=="" branch in validateFetchableDiscoveryURL —
  unreachable because TestOIDCConnectionResult's completeness gate
  already rejects a blank jwks_uri before checkOIDCJWKS is called.
- Add a regression case for the url.Parse error branch (malformed
  percent-encoding in the discovery-derived jwks_uri).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016tRKybTpfjQ4SxmNdVFLHi

zombor commented 2026-08-08 03:04:08 +00:00

Author

Owner

Copy link

Security Re-Review — PR #1396 (bookshelf-tm38.15)

Re-reviewed the fix for the prior BLOCKER (unvalidated discovery-derived jwks_uri SSRF).

BLOCKER verification

Closed. checkOIDCJWKS (internal/settings/oidc_settings.go:215-235) calls validateFetchableDiscoveryURL(jwksURI) (oidc_settings.go:531) before invoking fetchJWKS. validateFetchableDiscoveryURL requires https scheme and rejects any literal-IP host via isRestrictedHost (private/loopback/link-local/multicast/unspecified + the 169.254.169.254 metadata address) — the exact literal-IP PoC (127.0.0.1, 169.254.169.254, 10.x) is rejected pre-fetch, confirmed by the DescribeTable regression test at oidc_test_connection_test.go:531-553, which also asserts fetchJWKSCalled stays false for each malicious entry.

• No oracle leak: the failure detail is the fixed generic string "could not fetch signing keys: jwks_uri is not an allowed address" — no URL/host/IP is echoed (oidc_settings.go:222), verified by the “does not leak the rejected URI or host” test (oidc_test_connection_test.go:555-568).
• Redirect bypass closed: oidcHTTPClient.CheckRedirect now unconditionally rejects every redirect (internal/users/oidc_jwks.go:813-818), with a regression test proving a JWKS redirect is not followed (oidc_jwks_test.go:841-860). None of discovery/JWKS/token/userinfo legitimately need a redirect, so this has no functional downside.
• Only jwks_uri is fetched from the discovery doc. The other discovery-derived diagnostic checks (checkOIDCEndSession, checkOIDCBackchannelLogout, checkOIDCEndpoints's userinfo check) only presence-check/echo the field back — they never issue a server-side fetch, so they don't need the same guard. Confirmed by reading buildOIDCChecks (oidc_settings.go:159-173) and each check function's body.
• DNS-rebinding deferral (qapga) is reasonable. isRestrictedHost only inspects url.Hostname() when it parses as a literal IP (net.ParseIP); a hostname that only resolves to a private/loopback IP at dial time is not caught here. That is a narrower, harder-to-exploit residual (attacker needs DNS control + the dial-time race), correctly separated from the demonstrated PoC (literal-IP jwks_uri), which this fix fully closes.

Other observations (non-blocking)

[MINOR] internal/users/oidc_jwks.go — the production OIDC login path (newOIDCJWKSCache → FetchJWKS, wired in internal/users/wire.go:182) fetches jwks_uri from the discovery document of the admin-saved, validateIssuerURI-checked issuer, but does not itself re-validate jwks_uri before fetch (only the Test Connection diagnostic path gained that guard in this PR). This is a narrower trust model — the issuer itself is trusted admin config and the RFC 8414 issuer-match check in DiscoverOIDCMeta limits document substitution — and out of scope for this PR's stated fix, but worth a follow-up bead to apply validateFetchableDiscoveryURL (or equivalent) symmetrically to the login-time JWKS fetch for defense in depth, since a compromised/malicious IdP could still steer that fetch.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Security Re-Review — PR #1396 (bookshelf-tm38.15) Re-reviewed the fix for the prior BLOCKER (unvalidated discovery-derived `jwks_uri` SSRF). ### BLOCKER verification **Closed.** `checkOIDCJWKS` (internal/settings/oidc_settings.go:215-235) calls `validateFetchableDiscoveryURL(jwksURI)` (oidc_settings.go:531) **before** invoking `fetchJWKS`. `validateFetchableDiscoveryURL` requires `https` scheme and rejects any literal-IP host via `isRestrictedHost` (private/loopback/link-local/multicast/unspecified + the 169.254.169.254 metadata address) — the exact literal-IP PoC (127.0.0.1, 169.254.169.254, 10.x) is rejected pre-fetch, confirmed by the `DescribeTable` regression test at oidc_test_connection_test.go:531-553, which also asserts `fetchJWKSCalled` stays `false` for each malicious entry. - **No oracle leak:** the failure detail is the fixed generic string `"could not fetch signing keys: jwks_uri is not an allowed address"` — no URL/host/IP is echoed (oidc_settings.go:222), verified by the “does not leak the rejected URI or host” test (oidc_test_connection_test.go:555-568). - **Redirect bypass closed:** `oidcHTTPClient.CheckRedirect` now unconditionally rejects every redirect (internal/users/oidc_jwks.go:813-818), with a regression test proving a JWKS redirect is not followed (oidc_jwks_test.go:841-860). None of discovery/JWKS/token/userinfo legitimately need a redirect, so this has no functional downside. - **Only jwks_uri is fetched from the discovery doc.** The other discovery-derived diagnostic checks (`checkOIDCEndSession`, `checkOIDCBackchannelLogout`, `checkOIDCEndpoints`'s userinfo check) only presence-check/echo the field back — they never issue a server-side fetch, so they don't need the same guard. Confirmed by reading `buildOIDCChecks` (oidc_settings.go:159-173) and each check function's body. - **DNS-rebinding deferral (qapga) is reasonable.** `isRestrictedHost` only inspects `url.Hostname()` when it parses as a literal IP (`net.ParseIP`); a hostname that only *resolves* to a private/loopback IP at dial time is not caught here. That is a narrower, harder-to-exploit residual (attacker needs DNS control + the dial-time race), correctly separated from the demonstrated PoC (literal-IP jwks_uri), which this fix fully closes. ### Other observations (non-blocking) [MINOR] internal/users/oidc_jwks.go — the production OIDC **login** path (`newOIDCJWKSCache` → `FetchJWKS`, wired in internal/users/wire.go:182) fetches `jwks_uri` from the discovery document of the admin-saved, `validateIssuerURI`-checked issuer, but does not itself re-validate `jwks_uri` before fetch (only the Test Connection diagnostic path gained that guard in this PR). This is a narrower trust model — the issuer itself is trusted admin config and the RFC 8414 issuer-match check in `DiscoverOIDCMeta` limits document substitution — and out of scope for this PR's stated fix, but worth a follow-up bead to apply `validateFetchableDiscoveryURL` (or equivalent) symmetrically to the login-time JWKS fetch for defense in depth, since a compromised/malicious IdP could still steer that fetch. REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor commented 2026-08-08 03:07:56 +00:00

Author

Owner

Copy link

Code re-review (post SSRF-fix) — PR #1396 / bookshelf-tm38.15 @ 70ea049b

Focused on the delta since the prior review: validateFetchableDiscoveryURL, the CheckRedirect on the shared oidcHTTPClient, and the dead-branch removal.

[MAJOR] internal/settings/oidc_settings.go:517-519 — doc comment overstates a security control that does not exist yet
The doc comment on validateFetchableDiscoveryURL states: "the real SSRF backstop for hostname-based and redirect-based attacks is the dial-time safe transport on the shared OIDC HTTP client." This is not true as of this commit: internal/users/oidc_jwks.go's oidcHTTPClient (checked at 70ea049b) is a plain &http.Client{Timeout: ..., CheckRedirect: ...} with no custom Transport/DialContext — there is no dial-time IP validation analogous to internal/cover's safeTransport/safeDialContext. That gap is exactly what the deferred follow-up bookshelf-qapga (correctly filed, correctly scoped) exists to close. A comment asserting the backstop already exists is a real risk in a security-sensitive function: a future reviewer or engineer skimming this comment could reasonably conclude hostname-based/DNS-rebinding SSRF is already mitigated and deprioritize qapga, when in fact only the redirect-bypass and literal-IP cases are closed today. Fix: reword to something like "hostname-based and DNS-rebinding SSRF is NOT yet covered by this check or by oidcHTTPClient (tracked in bookshelf-qapga); only redirect-following (CheckRedirect) and literal-IP scheme/host validation are enforced today."

Everything else checked out:

• checkOIDCJWKS (oidc_settings.go:335) calls validateFetchableDiscoveryURL and returns on error BEFORE fetchJWKS is ever invoked (line 338) — correct ordering, confirmed by the DescribeTable asserting fetchJWKSCalled stays false for loopback/metadata/RFC1918/http/malformed jwks_uri.
• Rejection detail is generic ("jwks_uri is not an allowed address") and never echoes the URI/host; a dedicated test asserts the metadata IP does not leak into the response.
• CheckRedirect on the shared oidcHTTPClient (internal/users/oidc_jwks.go:51-56) is safe for the real login flow: it's used for discovery (GET), JWKS (GET), token exchange (POST via oauth2.HTTPClient context value), and userinfo (GET) — none of pergamum's legitimate calls require following a redirect, and the app's own browser-facing 302s (login → /authorize, callback → app pages) go through http.ResponseWriter, not this client, so they're unaffected. New regression test in oidc_jwks_test.go exercises a redirecting JWKS server and asserts the redirect is not followed.
• The removed raw == "" branch in validateFetchableDiscoveryURL (commit 70ea049b) is genuinely unreachable via the public path — TestOIDCConnectionResult's completeness gate (meta.JWKSURI == "") rejects a blank jwks_uri before checkOIDCJWKS is ever called — verified against oidc_settings.go's buildOIDCChecks/TestOIDCConnectionResult call order. Not a coverage-gaming deletion.
• Tests are black-box (package settings_test / package users_test), assert behavior (not-called + no-leak) rather than just a failure result, and no .golangci.yml or scripts/check-coverage.sh exclusions were added in this diff.
• bookshelf-qapga is scoped honestly and correctly identifies the remaining gap (DNS-rebinding / hostname-based SSRF) — it is not being used to paper over an open BLOCKER; the redirect-bypass and literal-IP SSRF paths that were the prior review's actual BLOCKER are closed in this diff.

REVIEW VERDICT: 0 blocker, 1 major, 0 minor

## Code re-review (post SSRF-fix) — PR #1396 / bookshelf-tm38.15 @ 70ea049b Focused on the delta since the prior review: `validateFetchableDiscoveryURL`, the `CheckRedirect` on the shared `oidcHTTPClient`, and the dead-branch removal. [MAJOR] internal/settings/oidc_settings.go:517-519 — doc comment overstates a security control that does not exist yet The doc comment on `validateFetchableDiscoveryURL` states: "the real SSRF backstop for hostname-based and redirect-based attacks is the dial-time safe transport on the shared OIDC HTTP client." This is not true as of this commit: `internal/users/oidc_jwks.go`'s `oidcHTTPClient` (checked at 70ea049b) is a plain `&http.Client{Timeout: ..., CheckRedirect: ...}` with no custom `Transport`/`DialContext` — there is no dial-time IP validation analogous to `internal/cover`'s `safeTransport`/`safeDialContext`. That gap is exactly what the deferred follow-up `bookshelf-qapga` (correctly filed, correctly scoped) exists to close. A comment asserting the backstop already exists is a real risk in a security-sensitive function: a future reviewer or engineer skimming this comment could reasonably conclude hostname-based/DNS-rebinding SSRF is already mitigated and deprioritize `qapga`, when in fact only the redirect-bypass and literal-IP cases are closed today. Fix: reword to something like "hostname-based and DNS-rebinding SSRF is NOT yet covered by this check or by oidcHTTPClient (tracked in bookshelf-qapga); only redirect-following (CheckRedirect) and literal-IP scheme/host validation are enforced today." Everything else checked out: - `checkOIDCJWKS` (oidc_settings.go:335) calls `validateFetchableDiscoveryURL` and returns on error BEFORE `fetchJWKS` is ever invoked (line 338) — correct ordering, confirmed by the DescribeTable asserting `fetchJWKSCalled` stays false for loopback/metadata/RFC1918/http/malformed jwks_uri. - Rejection detail is generic ("jwks_uri is not an allowed address") and never echoes the URI/host; a dedicated test asserts the metadata IP does not leak into the response. - `CheckRedirect` on the shared `oidcHTTPClient` (internal/users/oidc_jwks.go:51-56) is safe for the real login flow: it's used for discovery (GET), JWKS (GET), token exchange (POST via `oauth2.HTTPClient` context value), and userinfo (GET) — none of pergamum's legitimate calls require following a redirect, and the app's own browser-facing 302s (login → /authorize, callback → app pages) go through `http.ResponseWriter`, not this client, so they're unaffected. New regression test in oidc_jwks_test.go exercises a redirecting JWKS server and asserts the redirect is not followed. - The removed `raw == ""` branch in `validateFetchableDiscoveryURL` (commit 70ea049b) is genuinely unreachable via the public path — `TestOIDCConnectionResult`'s completeness gate (`meta.JWKSURI == ""`) rejects a blank jwks_uri before `checkOIDCJWKS` is ever called — verified against oidc_settings.go's `buildOIDCChecks`/`TestOIDCConnectionResult` call order. Not a coverage-gaming deletion. - Tests are black-box (`package settings_test` / `package users_test`), assert behavior (not-called + no-leak) rather than just a failure result, and no `.golangci.yml` or `scripts/check-coverage.sh` exclusions were added in this diff. - `bookshelf-qapga` is scoped honestly and correctly identifies the remaining gap (DNS-rebinding / hostname-based SSRF) — it is not being used to paper over an open BLOCKER; the redirect-bypass and literal-IP SSRF paths that were the prior review's actual BLOCKER are closed in this diff. REVIEW VERDICT: 0 blocker, 1 major, 0 minor

zombor added 1 commit 2026-08-08 03:11:39 +00:00

fix(settings): correct misleading SSRF doc comment on validateFetchableDiscoveryURL ... 4a9e78d07b

Reword to accurately state what is enforced today (https-only + literal-IP
rejection via isRestrictedHost, plus CheckRedirect on the shared OIDC HTTP
client) instead of falsely claiming a dial-time safe transport backstop
exists. That backstop (which would also cover hostname-based/DNS-rebinding
SSRF) is not yet implemented; tracked in follow-up bookshelf-qapga.

zombor force-pushed bd-bookshelf-tm38.15 from 4a9e78d07b to 36aa12a00d 2026-08-08 15:55:11 +00:00 Compare

zombor merged commit d50addb2ee into main 2026-08-08 16:01:54 +00:00

zombor referenced this pull request from a commit 2026-08-08 16:01:58 +00:00

Merge pull request 'OIDC Test Connection: structured per-check diagnostic [shot:oidc-test-connection-structured-diagnostic] (bookshelf-tm38.15)' (#1396) from bd-bookshelf-tm38.15 into main

zombor referenced this pull request 2026-08-08 16:29:53 +00:00

fix(wfengine): close DrainWorker TOCTOU (bookshelf-81hq) #1388

zombor referenced this pull request from a commit 2026-08-09 02:12:38 +00:00

fix(security): dial-time SSRF guard on shared OIDC HTTP client (bookshelf-qapga)

zombor referenced this pull request 2026-08-09 02:12:47 +00:00

fix(security): dial-time SSRF guard on shared OIDC HTTP client (bookshelf-qapga) #1405

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!1396

No description provided.