fix: embed PKCE code_verifier in signed OAuth state parameter#72
Conversation
The "Missing code verifier" error kept recurring because storing the PKCE code_verifier in cookies (session) is inherently fragile — cookies can be dropped by browser restarts, privacy extensions, or SameSite enforcement. Instead, bundle the code_verifier into the OAuth state parameter using itsdangerous.URLSafeTimedSerializer. Google echoes state back in the redirect URL, guaranteeing delivery regardless of cookie behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces session-based OAuth state management with a signed-state approach using itsdangerous.URLSafeTimedSerializer. This change embeds the PKCE code_verifier and CSRF nonce directly into the OAuth state parameter to improve reliability for users with restricted cookie settings. However, the review identifies a critical security regression: by removing the session-bound check, the implementation is now vulnerable to login CSRF and account linking attacks. Additionally, the code exposes sensitive stack traces to users and contains duplicated logic for state signing that should be refactored into a helper function.
| state_data, code, error = _validate_oauth_callback() | ||
| if error: | ||
| return error |
There was a problem hiding this comment.
The new signed-state approach removes the session-based CSRF protection for the OAuth callback. While the state is signed, it is no longer bound to the specific user session that initiated the request. An attacker could initiate a flow, obtain a valid signed_state, and trick a victim into completing the flow, potentially leading to account linking or login CSRF. To maintain security while still embedding the code_verifier in the state, you should include a unique nonce in the payload and verify it against a value stored in the user's session (e.g., session['oauth_state']) during the callback. If the goal is to support users with blocked cookies, be aware that this change significantly weakens the security of the authentication flow.
| except Exception as e: | ||
| import traceback | ||
|
|
||
| return f"<pre>Error: {e}\n\n{traceback.format_exc()}</pre>", HTTPStatus.INTERNAL_SERVER_ERROR |
There was a problem hiding this comment.
Exposing full stack traces to the user is a security risk (Information Exposure). It can reveal internal file paths, library versions, and code logic that an attacker could exploit. Consider logging the traceback on the server and returning a generic error message to the client.
| signed_state = URLSafeTimedSerializer(app.config["SECRET_KEY"]).dumps(reauth_payload) | ||
| parsed = urlparse(authorization_url) | ||
| params = parse_qs(parsed.query) | ||
| params["state"] = [signed_state] | ||
| authorization_url = urlunparse(parsed._replace(query=urlencode(params, doseq=True))) |
There was a problem hiding this comment.
The logic for signing the state and updating the authorization URL is duplicated here from the auth_google function. Additionally, the URLSafeTimedSerializer is instantiated multiple times across the file. Consider refactoring this into a helper function to improve maintainability and reduce the risk of inconsistent implementations.
1 participant
Summary
code_verifierout of cookie-based sessions and into the OAuthstateparameter, signed withitsdangerous.URLSafeTimedSerializerstateback in the redirect URL — guaranteed to survive regardless of cookie behavior, privacy extensions, or browser restarts_validate_oauth_callback()helper to keep ruff complexity under limitsTest plan
pytest tests/ -v— 40 passedruff check— cleanruff format --check— clean🤖 Generated with Claude Code