fix(auth): B-2.4 security review fixes — OAuth linking + magic link replay
Follow-up to commit 0513e67 addressing 2 critical OAuth account-takeover
vulnerabilities and 5 important issues found in the security review.
Critical fixes:
- C1: gate OAuth email-link on ``email_verified is True`` (strict bool)
in find_user_by_oauth + callback. Hostile Microsoft personal account
or Workspace tenant returning email_verified=False (or omitting the
claim) can no longer auto-link to an existing account. Callback shows
a friendly French flash + redirect to /login when the email exists
but the IdP didn't verify it.
- C2: refuse to overwrite an existing sso_subject in find_user_by_oauth.
A second IdP claiming the victim's email (Google after Microsoft, or
a hostile second Microsoft tenant) now raises PermissionError instead
of silently re-binding the User row, which would lock the legitimate
user out. Callback catches and flashes the error message in French.
Important fixes:
- I1: replace ``except Exception: pass`` in init_oauth_providers with an
idempotency pre-check on _oauth._clients. Real registration errors
(bad metadata URL, network failure) now surface as exceptions instead
of being silently swallowed at app boot.
- I2: single-use enforcement for magic-link tokens via in-process JTI
cache (_consumed_jtis dict). Replay within the 15-min validity window
now returns None. SECRET_KEY is now strictly required (no
default-dev-key fallback). Operator-facing comment documents that
/auth/magic-link/* should also be scrubbed from Cloudflare/Flask
access logs as defence in depth.
- I3: pre-check email collision in create_oauth_user_with_consent and
raise dedicated EmailAlreadyExistsError. Race against parallel /signup
in another tab between OAuth callback and finish-signup POST now
redirects to /login with a helpful French flash instead of burning 5
retry attempts and surfacing a 500.
- I4: oauth_signup_pending session blob now carries a created_at
timestamp; finish-signup rejects sessions older than 15 min with a
graceful expiry flash + redirect to /login.
- I5: init_oauth_providers logs an INFO when no providers are enabled
so operators can spot misconfigured deployments.
Tests: 16 → 21 (5 new):
- test_oauth_callback_refuses_link_when_email_not_verified (C1)
- test_oauth_callback_refuses_to_overwrite_existing_sso_subject (C2)
- test_finish_signup_handles_concurrent_account_creation (I3)
- test_finish_signup_expires_stale_oauth_session (I4)
- test_magic_link_token_is_single_use (I2)
Existing tests updated for new contract:
- test_oauth_callback_links_existing_user_by_email now sets
email_verified=True in the mock token (required by C1 gate).
- test_finish_signup_requires_cgu_and_confidentialite and
test_finish_signup_creates_user_and_4_consent_logs now seed
created_at in the session blob (required by I4 expiry check).
- test_magic_link_consume_logs_in_user_with_valid_token now also
asserts a second consume of the same token returns None and
redirects to /auth/magic-link with an invalid/expired flash.
Verified: 21/21 OAuth+magic-link tests pass; 16/16 email service tests
still pass (no regression in adjacent surface).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -5,13 +5,26 @@ Stateless tokens via ``itsdangerous`` (no DB column). Same pattern as
|
||||
the user_id; ``max_age`` is 15 minutes.
|
||||
|
||||
The compatibility-audit (C2) explicitly forbids new User columns
|
||||
(no ``magic_link_token``, no ``magic_link_sent_at``). Single-use enforcement
|
||||
is intentionally NOT implemented at this layer because the cost of a
|
||||
short-window replay (≤15 min, requires the user's email) is acceptable
|
||||
for the threat model — the user opened the email and clicked the link.
|
||||
If single-use becomes a hard requirement later, add an ip + sent_at index
|
||||
to a separate magic-link audit table without touching User.
|
||||
(no ``magic_link_token``, no ``magic_link_sent_at``). Single-use
|
||||
enforcement is implemented at the application layer via an in-process
|
||||
JTI cache (see ``_consumed_jtis`` below) — within a single gunicorn
|
||||
worker, a token can be consumed exactly once. Cross-worker uniqueness
|
||||
in a multi-worker deployment is best-effort and would require Redis or
|
||||
a small DB table; with the route's 10/min rate limit this is acceptable
|
||||
for B-2.4.
|
||||
|
||||
OPERATOR NOTE — log scrubbing:
|
||||
The magic-link token appears in the URL path (``/auth/magic-link/<token>``)
|
||||
and will therefore be captured by Cloudflare access logs, Flask's request
|
||||
log, and the user's browser history. The single-use cache here mitigates
|
||||
replay-from-logs within the 15-minute validity window, but operators
|
||||
should ALSO scrub ``/auth/magic-link/*`` from log retention as defence
|
||||
in depth (the operator action is documented in the security review;
|
||||
no application-side fix can fully address logs that have already been
|
||||
written elsewhere).
|
||||
"""
|
||||
import secrets
|
||||
import time
|
||||
from typing import Optional
|
||||
|
||||
from itsdangerous import URLSafeTimedSerializer, SignatureExpired, BadSignature
|
||||
@@ -20,21 +33,73 @@ from flask import current_app
|
||||
MAGIC_LINK_EXPIRY_SECONDS = 15 * 60 # 15 minutes
|
||||
_SALT = 'magic-link-login'
|
||||
|
||||
# In-process consumed-JTI cache: {jti: expires_at_unix_timestamp}.
|
||||
# Single-use enforcement against replay within the 15-min validity window.
|
||||
# Cache is best-effort: in a multi-worker gunicorn deployment a JTI
|
||||
# consumed on worker A would still be accepted on worker B. For production
|
||||
# multi-worker deployments, replace with Redis or a small DB table.
|
||||
# For B-2.4 with rate-limiting at 10/min on consume + 5/min on request,
|
||||
# this provides meaningful single-use enforcement within a worker.
|
||||
_consumed_jtis: dict = {}
|
||||
|
||||
|
||||
def _serializer() -> URLSafeTimedSerializer:
|
||||
"""Build a fresh serializer per call (cheap; reads SECRET_KEY from app config)."""
|
||||
secret_key = current_app.config.get('SECRET_KEY', 'default-dev-key')
|
||||
"""Build a fresh serializer per call (cheap; reads SECRET_KEY from app config).
|
||||
|
||||
Raises:
|
||||
RuntimeError: if SECRET_KEY is missing from app config. We refuse
|
||||
to fall back to a default key because that would let anyone
|
||||
forge magic-link tokens against any deployment that forgot
|
||||
to set SECRET_KEY.
|
||||
"""
|
||||
secret_key = current_app.config.get('SECRET_KEY')
|
||||
if not secret_key:
|
||||
raise RuntimeError(
|
||||
"SECRET_KEY must be configured for magic-link tokens"
|
||||
)
|
||||
return URLSafeTimedSerializer(secret_key, salt=_SALT)
|
||||
|
||||
|
||||
def _purge_expired_jtis() -> None:
|
||||
"""Drop entries past their expiry to bound memory."""
|
||||
now = time.time()
|
||||
for jti in [j for j, exp in _consumed_jtis.items() if exp < now]:
|
||||
_consumed_jtis.pop(jti, None)
|
||||
|
||||
|
||||
def generate_magic_link_token(user_id: int) -> str:
|
||||
"""Sign a magic-link token containing the user_id."""
|
||||
return _serializer().dumps(user_id)
|
||||
"""Generate a single-use magic-link token (15-min expiry, includes random JTI).
|
||||
|
||||
The JTI (JSON Token ID) is a random 16-byte URL-safe string embedded
|
||||
in the token payload. On consume, the JTI is added to the in-process
|
||||
``_consumed_jtis`` cache; subsequent consumes of the same token
|
||||
return None (single-use enforcement).
|
||||
"""
|
||||
jti = secrets.token_urlsafe(16)
|
||||
return _serializer().dumps({'uid': user_id, 'jti': jti})
|
||||
|
||||
|
||||
def consume_magic_link_token(token: str) -> Optional[int]:
|
||||
"""Return user_id if token is valid and unexpired, else None."""
|
||||
"""Verify + mark token as consumed. Returns user_id once; None on
|
||||
replay/expired/invalid/malformed.
|
||||
|
||||
Single-use enforcement: the JTI is added to ``_consumed_jtis`` on
|
||||
success; a second call with the same token returns None.
|
||||
"""
|
||||
try:
|
||||
return _serializer().loads(token, max_age=MAGIC_LINK_EXPIRY_SECONDS)
|
||||
payload = _serializer().loads(token, max_age=MAGIC_LINK_EXPIRY_SECONDS)
|
||||
except (SignatureExpired, BadSignature):
|
||||
return None
|
||||
|
||||
if not isinstance(payload, dict):
|
||||
return None
|
||||
user_id = payload.get('uid')
|
||||
jti = payload.get('jti')
|
||||
if not isinstance(user_id, int) or not isinstance(jti, str):
|
||||
return None
|
||||
|
||||
_purge_expired_jtis()
|
||||
if jti in _consumed_jtis:
|
||||
return None # replay — token already consumed
|
||||
_consumed_jtis[jti] = time.time() + MAGIC_LINK_EXPIRY_SECONDS
|
||||
return user_id
|
||||
|
||||
Reference in New Issue
Block a user