Skip to content

Verify OAuth/OIDC state in callbacks to prevent login CSRF - #21

Open
dimafa wants to merge 2 commits into
BetterAndBetterII:mainfrom
dimafa:verify-oauth-state
Open

Verify OAuth/OIDC state in callbacks to prevent login CSRF#21
dimafa wants to merge 2 commits into
BetterAndBetterII:mainfrom
dimafa:verify-oauth-state

Conversation

@dimafa

@dimafa dimafa commented Jun 6, 2026

Copy link
Copy Markdown

What

Both login handlers generate a random state and store it in a cookie:

  • HandleGitHubLoginoauthstate cookie
  • HandleOIDCLoginoidc_state cookie

…but neither callback ever verifies it. HandleGitHubCallback and HandleOIDCCallback go straight to the token exchange without comparing r.FormValue("state") to the cookie.

Why it matters

This leaves both OAuth/OIDC callbacks open to login CSRF: an attacker can force a victim's browser to complete a login with the attacker's authorization code, logging the victim into the attacker's account (or attaching the attacker's identity), which can be used to seed content the attacker can later read, etc. State verification is the standard OAuth defense and the cookie is already being set — it just wasn't checked.

Change

In both callbacks, read the corresponding cookie and compare it to the returned state, redirecting to / on missing/mismatched state:

stateCookie, err := r.Cookie("oidc_state") // or "oauthstate"
if err != nil || stateCookie.Value == "" || r.FormValue("state") != stateCookie.Value {
    logrus.Error("invalid oidc state in callback")
    http.Redirect(w, r, "/", http.StatusTemporaryRedirect)
    return
}

Behavior is unchanged for legitimate logins (the login handler already sets the cookie and passes the same state to the IdP via AuthCodeURL). Builds clean (go build ./...).

Both HandleGitHubLogin and HandleOIDCLogin set a random state cookie
(oauthstate / oidc_state), but neither callback ever compared it against the
state returned by the IdP. That leaves the OAuth/OIDC callbacks open to login
CSRF (an attacker can force a victim's browser to complete a login with the
attacker's authorization code).

Verify r.FormValue("state") against the corresponding cookie in both
HandleGitHubCallback and HandleOIDCCallback, redirecting to / on mismatch.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces state parameter verification against the oauthstate and oidc_state cookies in the GitHub and OIDC OAuth callback handlers to prevent login CSRF. The review feedback recommends clearing these state cookies immediately after verification to prevent replay attacks or state reuse, and suggests explicitly setting the cookie path to / for the GitHub OAuth state cookie to avoid potential path mismatch issues.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread handlers/auth/auth.go
Comment on lines +196 to +201
stateCookie, err := r.Cookie("oauthstate")
if err != nil || stateCookie.Value == "" || r.FormValue("state") != stateCookie.Value {
logrus.Error("invalid oauth state in github callback")
http.Redirect(w, r, "/", http.StatusTemporaryRedirect)
return
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

It is highly recommended to clear the oauthstate cookie immediately after verifying it to prevent replay attacks or state reuse.

Additionally, please note that generateStateOauthCookie (defined on line 163) does not set an explicit Path on the cookie. This means the cookie defaults to the path of the login handler. If the callback handler is hosted on a different path hierarchy, the browser will not send the oauthstate cookie, causing the state verification to fail. You should update generateStateOauthCookie to set Path: "/" as well.

Suggested change
stateCookie, err := r.Cookie("oauthstate")
if err != nil || stateCookie.Value == "" || r.FormValue("state") != stateCookie.Value {
logrus.Error("invalid oauth state in github callback")
http.Redirect(w, r, "/", http.StatusTemporaryRedirect)
return
}
stateCookie, err := r.Cookie("oauthstate")
if err != nil || stateCookie.Value == "" || r.FormValue("state") != stateCookie.Value {
logrus.Error("invalid oauth state in github callback")
http.Redirect(w, r, "/", http.StatusTemporaryRedirect)
return
}
// Clear the state cookie to prevent reuse
http.SetCookie(w, &http.Cookie{
Name: "oauthstate",
Value: "",
Path: "/",
MaxAge: -1,
HttpOnly: true,
})

Comment thread handlers/auth/auth.go
Comment on lines +304 to +309
stateCookie, err := r.Cookie("oidc_state")
if err != nil || stateCookie.Value == "" || r.FormValue("state") != stateCookie.Value {
logrus.Error("invalid oidc state in callback")
http.Redirect(w, r, "/", http.StatusTemporaryRedirect)
return
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

It is highly recommended to clear the oidc_state cookie immediately after verifying it to prevent replay attacks or state reuse. Since HandleOIDCLogin sets the cookie with Path: "/", we can safely clear it using the same path.

Suggested change
stateCookie, err := r.Cookie("oidc_state")
if err != nil || stateCookie.Value == "" || r.FormValue("state") != stateCookie.Value {
logrus.Error("invalid oidc state in callback")
http.Redirect(w, r, "/", http.StatusTemporaryRedirect)
return
}
stateCookie, err := r.Cookie("oidc_state")
if err != nil || stateCookie.Value == "" || r.FormValue("state") != stateCookie.Value {
logrus.Error("invalid oidc state in callback")
http.Redirect(w, r, "/", http.StatusTemporaryRedirect)
return
}
// Clear the state cookie to prevent reuse
http.SetCookie(w, &http.Cookie{
Name: "oidc_state",
Value: "",
Path: "/",
MaxAge: -1,
HttpOnly: true,
})

- generateStateOauthCookie now sets Path: / (matches oidc_state) so the GitHub
  state cookie is reliably sent to the callback regardless of route hierarchy.
- Both callbacks clear the state cookie after a successful check, making the
  state single-use (no replay/reuse).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@dimafa

dimafa commented Jun 6, 2026

Copy link
Copy Markdown
Author

Thanks — both applied:

  • generateStateOauthCookie now sets Path: "/" (matching oidc_state) so the cookie reliably reaches the callback.
  • Both callbacks now clear the state cookie after a successful verification, making it single-use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant