Add state nonce to OAuth flow for login CSRF protection#32
Merged
Conversation
Security review follow-ups: - aai login OAuth: bind the browser hand-off to the loopback capture with a single-use random `state` nonce. build_start_url carries it as a query param on the (still path-exact) discovery_redirect_url; capture_callback only accepts a callback whose `state` matches (constant-time), rejecting forged callbacks with a 400 and continuing to wait. Stops a malicious local page from completing someone else's login (login CSRF / account confusion) by replaying an attacker-minted discovery token while `aai login` is waiting. - aai init: write the scaffolded .env (which holds the real API key) with 0600 instead of the umask default, so the key isn't world/group-readable on a shared host. Created 0600 from the start and re-tightened on --force overwrite. Tests cover the nonce round-trip, state mismatch/missing rejection, and the .env file mode (fresh + overwrite).
The loopback callback URL carries the single-use discovery_oauth token and the state nonce in its query string, so it would otherwise linger in the browser's history and address bar. The success page now drops the query from the current history entry via history.replaceState on load — no extra request to race the server shutdown (unlike a redirect). The token is already spent server-side by then, but keeping credentials out of browser history is OAuth-for-native-apps (RFC 8252) hygiene. The page reflects no query data, so there is nothing to inject.
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
2 participants
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
Implements login CSRF protection in the OAuth discovery flow by introducing a single-use state nonce that binds the browser hand-off to the loopback callback capture. This prevents a malicious page from completing someone else's login by replaying a discovery token while
aai loginis waiting.Key Changes
State nonce generation and validation:
flow.run_login_flow()now generates a cryptographically random 256-bit nonce (secrets.token_urlsafe(32)) and passes it through the entire OAuth flow:discovery.build_start_url(state)loopback.capture_callback(expected_state)using constant-time comparisonLoopback callback validation: Enhanced
loopback.capture_callback()to:expected_stateparametersecrets.compare_digest()for timing-safe comparisonDiscovery URL construction:
discovery.build_start_url()now embeds the state nonce as a query parameter on the redirect URL itself (?state=...), which Stytch preserves through the OAuth flowSuccess page security: Added
history.replaceState()to the success HTML to scrub the callback URL (containing the token and state) from browser history, per RFC 8252 (OAuth for native apps)File permissions hardening:
.envfiles created byscaffold.scaffold()are now written with 0600 permissions (owner read/write only) to prevent API keys from being world/group-readable on shared hosts. Existing.envfiles are also tightened whenaai init --forceoverwrites them.Implementation Details
https://claude.ai/code/session_0115sdYguJwA8exNeKU52CEb