security: reject query response_mode for token flows; harden GET /logout against CSRF (#9, #10)#589
Merged
lakhansamani merged 2 commits intomainfrom Apr 7, 2026
Merged
Conversation
…out against CSRF (#9, #10) #9 — Tokens leaked into URL query string The /authorize handler accepted response_mode=query for any response_type, including implicit (response_type=token) and hybrid flows that issue tokens directly. Tokens in the query string get logged in proxy access logs, server access logs, and the browser history bar — a real-world credential leak path. Fix: validateAuthorizeRequest now rejects response_mode=query when response_type is not "code". OAuth 2.0 Multiple Response Type Encoding Practices §3.0 forbids this combination. Returns the standard "invalid_request" error so RP libraries handle it correctly. Permitted combinations: response_type=code → query, fragment, form_post response_type=token / id_token → fragment, form_post #10 — GET /logout was a CSRF logout vector GET /logout terminated the session purely on cookie presence. An attacker on a third-party page could place <img src="https://auth.example.com/logout"> and silently sign out any logged-in visitor. Removing GET entirely was not an option because OIDC RP-Initiated Logout 1.0 requires it on the end_session_endpoint and external OIDC clients depend on it. Fix: keep GET /logout but make it safe. - GET with id_token_hint (and the hint parses against our signing key) → proceed with logout. An <img> tag cannot synthesise a valid signed ID token, so this gate defeats the CSRF vector. - GET without (or with an invalid) id_token_hint → render an HTML confirmation page. Only the POST that follows actually deletes the session. - POST /logout → unchanged. The CSRF middleware (Branch 4) enforces Origin/Referer for all POSTs. The HTML confirmation page is rendered through html/template so any reflected redirect_uri / state / post_logout_redirect_uri values are properly escaped. Cache-Control: no-store is set on the page itself. End_session_endpoint advertisement in /.well-known/openid-configuration was already present and is unchanged. First-party SDKs (authorizer-js, authorizer-go, web/app, web/dashboard) all use the GraphQL `logout` mutation and are unaffected.
The inline html/template literal in logout.go was awkward to maintain and inconsistent with how the other authorize_* templates are organised. Move it into web/templates/logout_confirm.tmpl so it loads through the same gin LoadHTMLGlob path the existing form_post and web_message templates use, and so frontend folks can iterate on the markup/styles without touching Go code. The handler now renders via gc.HTML(http.StatusOK, "logout_confirm.tmpl", ...) following the same pattern as authorizeFormPostTemplate / authorizeWebMessageTemplate. Escaping behaviour is unchanged — gin's HTML render uses html/template under the hood, so the reflected redirect_uri / post_logout_redirect_uri / state values stay safely escaped.
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.
Summary
#9 — Tokens leaked into URL query string
The
/authorizehandler acceptedresponse_mode=queryfor anyresponse_type, including implicit (response_type=token) and hybrid flows that issue tokens directly. Tokens in the query string get logged in proxy access logs, server access logs, and the browser history bar.#10 —
GET /logoutwas a CSRF logout vectorGET /logoutterminated the session purely on cookie presence. An attacker on a third-party page could place<img src="https://auth.example.com/logout">and silently sign out any logged-in visitor. Removing GET entirely was not an option because OIDC RP-Initiated Logout 1.0 requires it on theend_session_endpoint.Changes
validateAuthorizeRequestnow rejectsresponse_mode=querywhenresponse_typeis notcode(per OAuth 2.0 Multiple Response Type Encoding Practices §3.0).LogoutHandlerdifferentiates GET vs POST:id_token_hint→ proceed with logout. An<img>tag cannot synthesise a valid signed ID token.id_token_hint→ render an HTML confirmation page. Only the POST that follows actually deletes the session.html/templateso any reflectedredirect_uri/state/post_logout_redirect_urivalues are properly escaped.Cache-Control: no-storeis set on the page.Notes
end_session_endpointadvertisement in/.well-known/openid-configurationwas already present and is unchanged.authorizer-js,authorizer-go,web/app,web/dashboard) all use the GraphQLlogoutmutation and are unaffected.Test plan
go build ./...TEST_DBS=sqlite go test -run "TestLogout|TestAuthorize|TestSignup|TestLogin" ./internal/integration_tests/<img src="/logout">no longer terminates the session/authorize?response_type=token&response_mode=query&...returnsinvalid_request