Fix OAuth login CSRF in Google account linking flow - #114
Open
jay79-boop wants to merge 1 commit into
Open
Conversation
/auth/google/callback trusted the `state` query param as-is: it was a base64url-encoded JSON blob (accountId, client, email, services) built entirely from client-supplied input on /auth/google/start, with nothing tying a callback back to the browser session that actually initiated it. Since /auth/google/callback is (necessarily) exempt from the app's normal session-cookie auth -- it has to be reachable when Google redirects the browser back -- this meant anyone could craft their own `state` (with an existing account's accountId) and get an admin to open `/auth/google/callback?code=<attacker's own authorization code>&state=<forged>` via a plain link. The server would exchange the attacker's code for the attacker's Google tokens and store them under the target account's id, silently swapping in Google credentials the admin doesn't control -- all without ever needing the admin's password or session. Fixed the same way /auth/codex/callback already does it elsewhere in this codebase: mint a random, unguessable state token server-side in /auth/google/start, hold the account-linking data in an in-memory map keyed by that token (never round-tripped through the client), and require the callback's `state` to match a live, single-use entry before doing anything. Unknown/forged/replayed state values are rejected before any token exchange happens. Added tests/server/routes-google.test.js covering: the minted state is opaque (not attacker-decodable JSON), a forged state is rejected without ever calling the token endpoint, and a valid state is single-use.
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
/auth/google/callbacktrusted thestatequery param as-is: it was a base64url-encoded JSON blob (accountId,client,email,services) built entirely from client-supplied input on/auth/google/start, with nothing tying a given callback back to the browser session that actually initiated it.Since
/auth/google/callbackis necessarily exempt from the app's normal session-cookie auth (it has to be reachable when Google redirects the browser back), this meant anyone could craft their ownstate— including one naming an existing account'saccountId— and get an admin to open:via a plain link (no JS needed, just a GET navigation). The server would exchange the attacker's authorization code for the attacker's own Google tokens and store them under the target account's id — silently swapping in Google credentials the admin doesn't control, without ever needing the admin's password or session token.
Fix
This codebase already has the correct pattern for exactly this problem in
/auth/codex/callback(lib/server/routes/codex.js): mint a random, unguessable state token server-side when the flow starts, hold the actual account-linking data in an in-memory map keyed by that token (never round-tripped through the client), and require the callback'sstateto match a live, single-use entry before doing anything.Applied the same pattern to
/auth/google/start//auth/google/callback:statesent to Google is now a random 16-byte hex token, not JSON.accountId/client/email/servicesare stored server-side in a map keyed by that token, with a TTL (kGoogleOauthStateTtlMs, matching the existing Codex TTL).Test plan
tests/server/routes-google.test.js:fetch(the token exchange) is never called