Skip to content

OAuth login: signed state cookie; bind accounts to the provider subject - #30

Merged
thorwhalen merged 2 commits into
mainfrom
security/28-oauth-state-and-links
Sep 22, 2026
Merged

thorwhalen merged 2 commits into
mainfrom
security/28-oauth-state-and-links

Conversation

@thorwhalen

@thorwhalen thorwhalen commented Sep 22, 2026 •

Copy link
Copy Markdown
Member

Closes #28

What

  1. State storage. Authlib's Starlette client keeps state, the nonce and the PKCE verifier in request.session, which needs a SessionMiddleware that enlace_auth never installed. Login could not complete as wired, and the existing tests mock both Authlib calls, so they never noticed. make_oauth_router now backs request.session with a signed cookie (enlace_oauth_state, itsdangerous with the platform signing key and salt oauth-state, Path=/auth, HttpOnly, SameSite=Lax, Secure when secure_cookies, Max-Age=600), but only when no real session is present. The cookie is cleared after a successful callback. A callback whose state was not issued to this browser, or was already used, is refused with 401 (login CSRF). Every callback spends its provider's pending states, whether it succeeds or fails.
  2. Account links. The identity is bound to the provider's stable subject: sub, or tid/oid for Entra ID, or GitHub's numeric id. It is stored as oauth_links[provider] on the user record. Rules (_login_refusal):
    • linked to this provider → the subject must match;
    • a password account with no link → refused, where before it was opened by email match;
    • created by this provider before links existed → allowed, and linked now;
    • anything else (another provider's account) → refused.
      New accounts are created with the link.

Not done (left for later)

  • No self-service "link my Google account to my password account" flow. The issue's option "require the password once" needs UI. Today such a login is refused with a message telling the user to sign in with their password.
  • Accounts are still keyed by email. A provider-side email change with the same subject creates a new account instead of finding the old one.

Impact

No login providers are configured on the live platform, so nothing deployed changes. No fleet dependent calls make_oauth_router. The two new parameters (state_cookie_name, state_max_age) are keyword-only and have defaults.

Tests

tests/test_oauth_state_and_links.py uses a real Authlib registry for a stub provider. Only fetch_access_token and userinfo are stubbed. It covers the round trip, a callback from another browser, a forged state, a single-use state, a forged unsigned cookie, and the linking rules. I checked with mutations that the tests catch the bugs: disabling the cookie session fails 9 of 10 tests, and disabling the link check fails 5. Full suite: 395 passed; ruff clean.

Independent refute-review (security): findings and fixes

  • Fixed: the state was single-use only when sign-in succeeded. An ?error= callback left it usable for a later ?code=. Every callback now spends its provider's states and rewrites the cookie, including on error responses.
  • Fixed (partly): in cookie tossing, a second enlace_oauth_state is planted at Path=/ and Starlette keeps the last one. Two cookies of that name are now refused. Residual, documented: a same-origin script can still plant a state for a browser that has none of its own. The state cannot be bound to the browser without a browser-held secret, and same-origin scripts are already a known platform-wide limit (SECURITY L11). A __Host- prefix would force Path=/, which sends the cookie to every proxied app.
  • Fixed: a password reset (admin, emailed link, CLI) now unlinks oauth_links. Before, recovery left an attacker's linked identity in place. A self-service change that knows the old password keeps the links.
  • Fixed: tid/oid identify the subject only for Entra issuers (login.microsoftonline.com, sts.windows.net). The legacy re-link now re-reads the record before writing. authlib>=1.4, because 1.3 never evicts states and the cookie could grow.
  • Accepted, documented: a legacy provider-created account is bound to the first subject that signs in after the upgrade (no worse than before). Path=/auth assumes no root path. A legacy account that later got a password is now refused for OAuth. The subject can flip if an Entra tenant changes which claims the ID token carries.
  • Suite: 400 passed; ruff clean.

🤖 Generated with Claude Code

thorwhalen and others added 2 commits September 22, 2026 16:33
…e provider subject

Authlib keeps the OAuth state, nonce and PKCE verifier in request.session, which
nothing in enlace_auth provided, so the flow could not complete as wired. The
router now backs request.session with a signed, HttpOnly, SameSite=Lax cookie
scoped to /auth (10-minute lifetime) whenever no SessionMiddleware is present,
and clears it after a successful callback. A callback whose state was not issued
to this browser is refused.

OAuth identities are now bound to the provider's stable subject (sub; tid/oid
for Entra ID; GitHub's id), recorded as oauth_links[provider]. A later login
must present the same subject; an existing password account, or an account
linked to or created by another provider, is refused instead of being opened
by an email match. Accounts this provider created before links existed are
linked on their next login.

Tests drive Authlib's real redirect/state code against a stub provider (only
the token exchange and userinfo calls are stubbed).

Closes #28

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Every callback, including refused or failed ones, spends its provider's
  pending states (the cookie is rewritten on the error response too), so an
  error callback no longer leaves the state usable for a later code.
- Two state cookies of the same name (cookie tossing) are refused.
- tid/oid identify the subject only for Entra ID issuers; elsewhere `sub`.
- The legacy re-link re-reads the record before writing, so it cannot revert a
  concurrent password change.
- A password reset (admin, emailed link, CLI) unlinks external sign-ins:
  recovery must evict whoever linked one.
- authlib >= 1.4 (1.3 never evicts old states, so the cookie could grow).
- Residual limits documented in the module docstring.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@thorwhalen
thorwhalen force-pushed the security/28-oauth-state-and-links branch from b2f7699 to 99abe0d Compare September 22, 2026 16:33
@thorwhalen

Copy link
Copy Markdown
Member Author

Dependents against this branch (rebased on 0.1.26): i2mint/enlace 308 passed; i2mint/enlace_docker 58 passed; tw_platform 576 passed, 4 failed (the pre-existing root-only test_grant_wrapper_refuses_before_doing_anything, same on main). Hosted CI green on the rebased head.

@thorwhalen
thorwhalen merged commit aab55a3 into main Sep 22, 2026
12 checks passed
@thorwhalen
thorwhalen deleted the security/28-oauth-state-and-links branch September 22, 2026 16:37
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.

OAuth login: state storage and account linking

1 participant