auth: security-review fixes (jti retry, return_to control chars) - #86
Merged
Conversation
…gout Migration 00015 adds a nullable sessions.oidc_sid (partial index for the revocation lookup). Callback stores the verified sid claim; IdPs without a sid keep working — the column is just NULL. SessionStore gains DeleteBySID and DeleteByOIDCSub (sqlc :execrows) as groundwork for the /auth/backchannel-logout endpoint (docs/specs/m3-backchannel-logout.md §2.1). Integration tests cover sid-less logins, per-sid revocation isolation, and sub-only revocation ending all of a user's sessions.
POST /auth/backchannel-logout accepts the IdP's form-encoded logout token, validates it per OIDC Back-Channel Logout 1.0 §2.4–2.6 (JWKS signature, iss, aud, iat freshness, optional exp, events member, nonce absence, sid-or-sub, jti replay via an in-process TTL cache), then ends the matching sessions: by oidc_sid when the token carries a sid, else every session of the sub. 200 also when nothing matched — an expired session is a successful logout, and a distinguishable answer would be an oracle for live sids. Rejections are 400 problem+json, logged with the reason but never the token. Accepted logouts log issuer, hashed sid/sub, and the session count. The endpoint is provider-agnostic and always registered; deployments enable the feature purely by registering the URL at their IdP (golden rule 8). Tests: the §3 validation table + replay against a new authtest JWKS/discovery double, handler-level tests over a fake session store (no-store headers, no-oracle 200, problem+json), and a real-Postgres end-to-end test through the router: cookie works → logout token lands → old cookie is 401, other sids and users untouched, sub-only ends all of a user's sessions.
docs/02 §6 gains a 'Logout — both directions' block (validation rules, revocation semantics, single-instance jti cache note); the Keycloak guide gains the client Logout settings (Backchannel logout URL + session required), a verification step, and two troubleshooting rows; README and config.example.yaml note that registering the URL at the IdP is the only knob — the endpoint is always on and provider-agnostic.
1. Back-channel logout no longer consumes a token's jti before revocation succeeds. The replay cache previously recorded the jti during validation, so a transient DB failure (answered 504 so the IdP retries) made every retry of the same token fail as a 'replay' — the logout was silently dropped and the session survived. The jti is now checked during validation and remembered only after the sessions were actually ended. 2. sanitizeReturnTo rejects control characters. Browsers strip ASCII tab/newline when parsing a Location URL (WHATWG preprocessing), so return_to=%09//evil.com passed the '//' prefix check yet reached the browser as protocol-relative //evil.com — an open redirect on the login flow. Any byte < 0x20 (and 0x7f) now falls back to '/'.
- Exempt /auth/backchannel-logout from the shared 60/min write limiter: all IdP notifications arrive from one IP and Keycloak never retries a 429, so an IdP-side mass logout would silently drop sessions' logouts. The route carries its own generous per-IP limit (600/min); forged tokens still die at signature validation. Router-level test posts a 70-token burst. - Apply the clock-skew allowance to the stale-iat check (max age + skew), so IdP/RP drift can't drop real logouts and the jti-cache TTL comment holds. - Verify logout-token signatures through a cooldown key set (go-jose over the discovered jwks_uri, promoted from indirect dependency): unknown-kid junk at the unauthenticated endpoint refetches the JWKS at most once per 30s, while cached keys keep verifying. Test counts JWKS hits across a burst. - Reject a present-but-malformed sid claim (non-string or empty) instead of silently escalating to sub-wide revocation. - Require exp: the final Back-Channel Logout 1.0 spec makes it REQUIRED (the optional-exp comment cited a stale draft); spec + docs/02 corrected. - Drop the manual aud check and Authenticator.clientID — go-oidc's Verify already enforces the audience. - Hoist writeProblem into internal/httpx, shared by the API layer and auth.
Member
Author
|
Supervisor review addressed in 7989ddb — all 7 findings, on this branch:
Gates rerun: |
This was referenced Aug 28, 2026
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
1 participant
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.
Part 4 of the M3 series — the "fix what's cheap" outcome of the spec §4 security review. Stacked on #85.
Finding 1 (medium, new code): failed back-channel logout was unretryable.
verifyLogoutTokenrecorded thejtiin the replay cache before the session delete ran. A transient DB error answered 504 ("retry later"), but the IdP's retry carries the same token — now rejected as a replayedjtifor longer than the token stays fresh, so the logout was silently dropped and the session survived. The cache now has separatecontains(checked in validation) andremember(called only after revocation succeeded). Test: 504 under injected DB failure → same token re-POSTed → 200 and sessions revoked; plus an endpoint-level replay test (200 then 400).Finding 2 (medium, pre-existing, spec §4 checklist "tabs"): open redirect via tab in
return_to.sanitizeReturnToblocked//and/\but let/\t/evil.com(return_to=%2F%09%2Fevil.com) through; Go writes the interior tab into the Location header verbatim and WHATWG URL preprocessing strips tabs/newlines, so browsers see protocol-relative//evil.comafter login. Control bytes (< 0x20, 0x7f) now reject to/. Table test covers tab/CR/LF/VT/DEL and the existing prefixes.Review summary and the remaining (filed, not fixed) items are in the PR series discussion / issues.
go test -race ./...,golangci-lint: green.Do not merge — supervisor review.