3a41bb482d
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>
106 lines
4.2 KiB
Python
106 lines
4.2 KiB
Python
"""Magic link login (B-2.4).
|
|
|
|
Stateless tokens via ``itsdangerous`` (no DB column). Same pattern as
|
|
``src/services/email.py:generate_verification_token`` — token contains
|
|
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 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
|
|
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).
|
|
|
|
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:
|
|
"""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]:
|
|
"""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:
|
|
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
|