Skip to content

fix(state): bind OAuth states to the browser that started the flow - #257

Merged
allen0099 merged 1 commit into
masterfrom
fix/state-binding-226
Sep 26, 2026
Merged

allen0099 merged 1 commit into
masterfrom
fix/state-binding-226

Conversation

@allen0099

Copy link
Copy Markdown
Owner

Closes #226.

Problem

create_state() / consume_state() took no binding, so any stored state completed the flow in any browser. That is the login CSRF RFC 6749 §10.12 says state must prevent: the attacker starts a flow, then sends the victim to the callback with the attacker's state and code. The victim ends up logged in to the attacker's account. STATE.md nevertheless described the states as CSRF protection.

Change

  • create_state(..., *, binding=None)
    • Stores the SHA-256 of the binding in a new StateData.binding_hash field. The binding itself is never stored.
    • An empty binding raises ValueError, since a missing cookie read as "" would bind everyone alike.
  • consume_state(state, *, binding=None)
    • Compares with hmac.compare_digest and raises InvalidStateError("State was issued to a different client") on a mismatch.
    • The check works in both directions: a bound state consumed without a binding is rejected, and so is an unbound state consumed with one. Neither side can drop the check by omitting its value.
    • The state is consumed either way.
    • A mismatch is logged at INFO with state_ref, like an unknown state. A second login in another tab overwrites the cookie, so a mismatch is not always an attack.
  • Compatibility: existing calls without binding behave as before. validate_state() / get_state_metadata() are unchanged: they don't consume and aren't a security check.
  • Docs (EN and zh-TW)
    • The intro says storage alone is not CSRF protection.
    • The quick start sets a random nonce as an HttpOnly, Secure, SameSite=Lax cookie in /login, checks it in /callback and deletes it afterwards.
    • The docs note samesite="none" for form_post and the two-tab case.
    • Method signatures, the consume_state table and the StateData block are updated. The zh-TW heading anchors follow the new English slugs (checked in the built HTML).
  • CHANGELOG gets a ### Security entry; CLAUDE.md gets a line.

Tests

  • tests/state/test_manager.py, parametrized over memory and Redis:
    • test_bound_state_is_accepted_with_its_binding (also checks binding_hash);
    • test_bound_state_is_rejected_for_another_client, parametrized with another binding and with none. It also checks that the state is burned afterwards;
    • test_unbound_state_is_rejected_with_a_binding;
    • test_empty_binding_is_rejected;
    • test_binding_is_not_stored_in_plain_text.
  • tests/state/test_login_csrf.py runs the quick start end to end. The initiating browser completes the flow, and the victim presenting the attacker's state gets 400 (it got 200 before this change).

Mutation checks:

  • With _binding_matches returning True unconditionally, the three rejection cases fail.
  • With the empty check removed, only test_empty_binding_is_rejected fails.

Local results:

  • ruff and mypy --strict pass.
  • Full suite against live Redis and Memcached: 926 passed.
  • Both docs builds pass --strict --clean.

Any stored state completed the flow in any browser, so an attacker could
start a flow and send the victim to the callback with the attacker's
state and code (login CSRF), although STATE.md described the states as
CSRF protection.

create_state(binding=...) stores the SHA-256 of a client secret, such as
a nonce also set as a cookie, and consume_state(state, binding=...)
raises InvalidStateError unless the same value is given. A bound state
consumed without a binding, or an unbound one consumed with a binding,
is rejected too, and the state is consumed either way. The STATE.md
quick start now sets and checks a binding cookie.

Closes #226
@allen0099 allen0099 added this to the 0.3.8 milestone Sep 26, 2026
@allen0099 allen0099 added the bug Something isn't working label Sep 26, 2026
@allen0099
allen0099 merged commit 3947810 into master Sep 26, 2026
11 checks passed
@allen0099
allen0099 deleted the fix/state-binding-226 branch September 26, 2026 21:33
@allen0099 allen0099 added the security Security vulnerability or hardening label Sep 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

StateManager: bind an OAuth state to the browser that started the flow

1 participant