docs: document OAuth2 flow state cookies (SameSite=Lax) in cookies.md and security.md#207
Merged
veverkap merged 2 commits intoMay 3, 2026
Conversation
… and security.md The OAuth2Handler uses the same short-lived SameSite=Lax HttpOnly state cookies (oauth2_state, oauth2_verifier, 5-minute TTL) as OIDCHandler for the same reason — the provider redirects the browser back as a top-level cross-site navigation that SameSite=Strict would block. - Add 'OAuth2 flow cookies' section to docs/handler/cookies.md explaining the oauth2_state and oauth2_verifier cookies and the SameSite=Lax rationale - Update the 'Cookie security' bullet in docs/security.md to name all four flow state cookies (oidc_state, oidc_verifier, oauth2_state, oauth2_verifier) rather than only the OIDC pair Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Updates cross-cutting documentation to accurately reflect the OAuth2Handler flow cookies and their SameSite trade-offs, aligning Cookie Helpers and Security Notes with the existing implementation.
Changes:
- Document OAuth2 flow state cookies (
oauth2_state,oauth2_verifier) in Cookie Helpers, mirroring the OIDC rationale forSameSite=Lax. - Update Security Notes to include both OIDC and OAuth2 flow state cookies in the cookie security guidance.
Show a summary per file
| File | Description |
|---|---|
| docs/security.md | Expands cookie security guidance to explicitly include OAuth2 flow state cookies alongside OIDC. |
| docs/handler/cookies.md | Adds an “OAuth2 flow cookies” section describing cookie names and SameSite rationale for OAuth2Handler. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
Corrects the sentence to reflect the actual sequence in OAuth2Handler.Callback: cookies are cleared after the CSRF state is validated and the verifier value has been read, not after full PKCE validation (which happens server-side during the code exchange).
Closed
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.
Summary
When
OAuth2Handlerwas added, itsoauth2_stateandoauth2_verifierflow cookies were not reflected in the Cookie Helpers reference or the Security Notes page. This PR closes that gap.Changes
docs/handler/cookies.mdAdded a new OAuth2 flow cookies section explaining that
OAuth2Handlersets the sameSameSite=Lax,HttpOnly, 5-minute TTL state cookies (oauth2_state,oauth2_verifier) asOIDCHandler, and documents the same rationale: the provider redirect is a top-level cross-site navigation thatSameSite=Strictwould block.docs/security.mdUpdated the Cookie security bullet to name all four flow state cookies (
oidc_state,oidc_verifier,oauth2_state,oauth2_verifier) rather than only the OIDC pair, keeping the security guidance accurate for theOAuth2Handlerthat was added in the previous commit.Motivation
The prior commit (#204) added full
OAuth2Handlerdocumentation (routes, config, status codes, account linking) but missed updating the cross-cutting cookie and security reference pages. A developer readingdocs/security.mdordocs/handler/cookies.mdwould see that only OIDC usesSameSite=Laxcookies, leaving them unaware thatOAuth2Handlermakes the same trade-off.Verification
handler/oauth2.go:109–127(oauth2_state,oauth2_verifier,SameSite=Lax, 5-min TTL)handler/oidc.go:88–110(identical pattern)Greptile Summary
Documentation-only PR that fills a gap left when
OAuth2Handlerwas introduced: adds an OAuth2 flow cookies section todocs/handler/cookies.mdand widens the Cookie security bullet indocs/security.mdto name all four flow-state cookies. Both changes are accurate and consistent withhandler/oauth2.go(lines 109–127, 143–159, 173).Confidence Score: 5/5
Safe to merge — documentation only, no functional code changed.
No P0 or P1 findings. All documented claims (SameSite=Lax, HttpOnly, 5-min TTL, clearing order, PKCE server-side verification) are verified against the implementation in handler/oauth2.go. The only outstanding gap (missing OAuth2 PKCE section in security.md) was flagged in a prior review and is a P2 at most.
No files require special attention.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Browser participant App as goauth OAuth2Handler participant Provider as OAuth2 Provider Browser->>App: GET /oauth2/login App->>Browser: Set oauth2_state cookie (SameSite=Lax, HttpOnly, 5 min TTL) App->>Browser: Set oauth2_verifier cookie (SameSite=Lax, HttpOnly, 5 min TTL) App->>Browser: 302 Redirect to Provider AuthCodeURL with state and S256 PKCE challenge Browser->>Provider: GET /authorize (top-level cross-site navigation) Provider->>Browser: 302 Redirect to /oauth2/callback with code and state Browser->>App: GET /oauth2/callback (cookies sent via SameSite=Lax) App->>App: Validate state param vs oauth2_state cookie (CSRF check) App->>App: Read verifier from oauth2_verifier cookie App->>App: Clear both flow cookies App->>Provider: Exchange code plus PKCE verifier server-side Provider->>App: Access token App->>App: FetchUserInfo and issue JWT session cookies App->>Browser: 302 Redirect to login success URLComments Outside Diff (1)
docs/security.md, line 23-25 (link)This PR adds the OAuth2 cookie details to the Cookie security section, but
docs/security.mdstill has a dedicated## OIDC PKCEsection with no equivalent for the OAuth2 flow. The OAuth2 handler uses the same S256 PKCE pattern (oauth2.S256ChallengeOption(verifier)inoauth2.go:129) and validates the state parameter on every callback (lines 143–146), so a parallel## OAuth2 PKCEentry would keep the security reference complete.Prompt To Fix With AI
Reviews (2): Last reviewed commit: "docs: fix OAuth2 cookie-clearing timing ..." | Re-trigger Greptile