From afc3250c488ffc59376def27242eb5dff9fafd0b Mon Sep 17 00:00:00 2001 From: Nguyen Ngo Date: Sun, 17 May 2026 18:29:13 -0400 Subject: [PATCH] 05/17 enhance codes --- app/__init__.py | 7 +- app/models/recovery_challenge.py | 91 +++++++++++++++++++ app/routes/auth.py | 79 +++++++++------- app/services/auth_service.py | 29 ++++-- extension/popup/popup.js | 29 ++++-- ...6a7b8c9d0_add_recovery_challenges_table.py | 63 +++++++++++++ 6 files changed, 246 insertions(+), 52 deletions(-) create mode 100644 app/models/recovery_challenge.py create mode 100644 migrations/versions/e5f6a7b8c9d0_add_recovery_challenges_table.py diff --git a/app/__init__.py b/app/__init__.py index e8b520e..e84d8dd 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -104,6 +104,7 @@ def create_app(config_name: str = 'development') -> Flask: from .models.shared_item import SharedItem from .models.emergency_access import EmergencyAccess from .models.audit_log import AuditLog + from .models.recovery_challenge import RecoveryChallenge @login_manager.user_loader def load_user(user_id): @@ -188,15 +189,17 @@ def create_app(config_name: str = 'development') -> Flask: with app.app_context(): try: from app.models.token_blacklist import TokenBlacklist + from app.models.recovery_challenge import RecoveryChallenge TokenBlacklist.cleanup_expired() + RecoveryChallenge.cleanup_expired() import logging logging.getLogger(__name__).debug( - '[PassKeeper] token_blacklist cleanup completed' + '[PassKeeper] token_blacklist + recovery_challenges cleanup completed' ) except Exception as exc: # pragma: no cover import logging logging.getLogger(__name__).warning( - '[PassKeeper] token_blacklist cleanup failed: %s', exc + '[PassKeeper] cleanup job failed: %s', exc ) scheduler = BackgroundScheduler(daemon=True) diff --git a/app/models/recovery_challenge.py b/app/models/recovery_challenge.py new file mode 100644 index 0000000..9fb4f86 --- /dev/null +++ b/app/models/recovery_challenge.py @@ -0,0 +1,91 @@ +from datetime import datetime, timezone, timedelta + +from sqlalchemy.dialects.mysql import INTEGER + +from app import db + +# Challenge TTL: the client has 15 minutes from /recovery/data to complete recovery. +CHALLENGE_TTL_MINUTES = 15 + + +class RecoveryChallenge(db.Model): + """ + Server-side state for the account-recovery challenge-response flow. + + Replaces the previous flask.session-based storage, which was not safe in + multi-worker Gunicorn deployments (each worker has its own in-process + session store, so a challenge written by worker A is invisible to worker B). + + A row is created (or replaced) by GET /recovery/data and consumed + (deleted) exactly once by POST /recover or GET /recovery/items. + Rows older than CHALLENGE_TTL_MINUTES are ignored and cleaned up by the + same APScheduler job that handles token_blacklist. + + Columns + ------- + user_id FK → users.id (CASCADE DELETE) + nonce Random 64-hex-char string issued to the client. + expected_proof HMAC-SHA256(key=enc_key_salt, msg=nonce), hex-encoded. + Precomputed at challenge creation so the server never needs + to touch enc_key_salt again during verification. + expires_at Absolute UTC timestamp after which this challenge is void. + """ + __tablename__ = 'recovery_challenges' + + id = db.Column(INTEGER(unsigned=True), autoincrement=True, primary_key=True) + user_id = db.Column( + INTEGER(unsigned=True), + db.ForeignKey('users.id', ondelete='CASCADE'), + nullable=False, + unique=True, # one active challenge per user at a time + index=True, + ) + nonce = db.Column(db.String(64), nullable=False) + expected_proof = db.Column(db.String(64), nullable=False) + expires_at = db.Column(db.DateTime, nullable=False) + + @classmethod + def create(cls, user_id: int, nonce: str, expected_proof: str) -> 'RecoveryChallenge': + """ + Upsert: delete any existing challenge for this user, then create a fresh one. + Caller must call db.session.commit() after this. + """ + # Remove stale challenge so the UNIQUE constraint never fires on re-issue. + cls.query.filter_by(user_id=user_id).delete() + challenge = cls( + user_id=user_id, + nonce=nonce, + expected_proof=expected_proof, + expires_at=datetime.now(timezone.utc).replace(tzinfo=None) + + timedelta(minutes=CHALLENGE_TTL_MINUTES), + ) + db.session.add(challenge) + return challenge + + @classmethod + def consume(cls, user_id: int) -> 'RecoveryChallenge | None': + """ + Fetch and delete the challenge for user_id in one operation. + Returns the challenge if it exists and has not expired, otherwise None. + Caller must call db.session.commit() after this. + """ + challenge = cls.query.filter_by(user_id=user_id).first() + if not challenge: + return None + if challenge.expires_at < datetime.now(timezone.utc).replace(tzinfo=None): + cls.query.filter_by(user_id=user_id).delete() + return None + # Delete before returning — one-time use. + cls.query.filter_by(user_id=user_id).delete() + return challenge + + @classmethod + def cleanup_expired(cls) -> None: + """Delete expired challenges — called by the APScheduler cleanup job.""" + cls.query.filter( + cls.expires_at <= datetime.now(timezone.utc).replace(tzinfo=None) + ).delete() + db.session.commit() + + def __repr__(self): + return f'' diff --git a/app/routes/auth.py b/app/routes/auth.py index 0761113..cca73a9 100644 --- a/app/routes/auth.py +++ b/app/routes/auth.py @@ -1,7 +1,7 @@ import re import time -from flask import Blueprint, request, jsonify, g, session +from flask import Blueprint, request, jsonify, g from app import db, limiter, client_ip from app.models.user import User from app.models.audit_log import AuditLog @@ -738,10 +738,13 @@ def recover_account(): 4. Client derives new credentials and re-encrypts all vault items. 5. Client POSTs everything here in one atomic payload. - The server validates recovery_proof against the value precomputed during - /recovery/data — enc_key_salt is never sent in plaintext. - The nonce is consumed on first use to prevent replay. + The server validates recovery_proof against the value stored in the DB + during /recovery/data — enc_key_salt is never sent in plaintext. + The challenge row is consumed (deleted) on first use to prevent replay. + Challenge state is stored in the database, not the Flask session, so the + flow works correctly across all Gunicorn workers. """ + from app.models.recovery_challenge import RecoveryChallenge data = request.get_json(silent=True) or {} email = (data.get('email') or '').strip().lower() new_auth_hash = data.get('new_auth_hash', '') @@ -754,18 +757,17 @@ def recover_account(): time.sleep(0.1) # timing mitigation - # Validate session binding. - expected_proof = session.get('recovery_expected_proof', '') - session_user_id = session.get('recovery_user_id') - - if not expected_proof or not session_user_id: - return jsonify({'error': 'No active recovery challenge. Call /recovery/data first.'}), 400 - user = User.query.filter_by(email=email).first() - if not user or not user.recovery_enc_salt or user.id != session_user_id: + if not user or not user.recovery_enc_salt: return jsonify({'error': 'No recovery code found for this account'}), 404 - if not verify_recovery_proof(expected_proof, client_proof): + # Consume the challenge — atomic read-and-delete from the DB. + # consume() returns None if the challenge is missing or expired. + challenge = RecoveryChallenge.consume(user.id) + if not challenge: + return jsonify({'error': 'No active recovery challenge. Call /recovery/data first.'}), 400 + + if not verify_recovery_proof(challenge.expected_proof, client_proof): AuditLog.log( user_id=user.id, action='auth.recovery_failed', @@ -777,10 +779,6 @@ def recover_account(): db.session.commit() return jsonify({'error': 'Invalid recovery code'}), 401 - # Consume the nonce — one-time use only. - session.pop('recovery_nonce', None) - session.pop('recovery_expected_proof', None) - session.pop('recovery_user_id', None) try: from app.models.vault_item import VaultItem @@ -847,12 +845,17 @@ def recovery_data(): The nonce is used for the HMAC-SHA256 challenge-response proof: - Client decrypts recovery_enc_salt → gets enc_key_salt bytes. - Client computes: proof = HMAC-SHA256(key=enc_key_salt_bytes, msg=nonce) - - Server stores expected proof in server-side session at challenge time, + - Server stores expected proof in the DB (recovery_challenges table), verifying it on /recover and /recovery/items without ever receiving enc_key_salt in plaintext. Returns 404 if no recovery code is configured (prevents user enumeration). + The challenge is stored in the database (not the Flask session cookie) so + it works correctly across all Gunicorn workers. """ + import hashlib, hmac as _hmac + from app.models.recovery_challenge import RecoveryChallenge + email = (request.args.get('email') or '').strip().lower() if not email: return jsonify({'error': 'email is required'}), 400 @@ -865,7 +868,6 @@ def recovery_data(): # enc_key_salt. The client must return HMAC-SHA256(enc_key_salt, nonce). # This proves it decrypted the recovery blob correctly without sending # enc_key_salt in plaintext. - import hashlib, hmac as _hmac nonce = generate_recovery_nonce() expected_proof = _hmac.new( user.enc_key_salt.encode(), @@ -873,10 +875,15 @@ def recovery_data(): hashlib.sha256, ).hexdigest() - # Store expected proof and bind it to the user — consumed on first use. - session['recovery_nonce'] = nonce - session['recovery_expected_proof'] = expected_proof - session['recovery_user_id'] = user.id + # Persist challenge in the DB — safe across all Gunicorn workers. + # RecoveryChallenge.create() deletes any previous challenge for this user + # before inserting, so a re-issued challenge always starts fresh. + RecoveryChallenge.create( + user_id=user.id, + nonce=nonce, + expected_proof=expected_proof, + ) + db.session.commit() return jsonify({ 'enc_key_salt': user.enc_key_salt, @@ -895,28 +902,32 @@ def recovery_items(): Requires X-Recovery-Proof header containing the HMAC-SHA256 proof: proof = HMAC-SHA256(key=enc_key_salt_bytes, msg=nonce_from_recovery_data) - The server validates the proof against the value precomputed and stored in - flask.session during /recovery/data — enc_key_salt is never sent in plaintext. + The server validates the proof against the value stored in the DB + during /recovery/data — enc_key_salt is never sent in plaintext. + The challenge row is NOT consumed here; it is consumed by the final + POST /recover call so the client can call this endpoint to preview + items before committing the full re-encryption. Items are returned as encrypted ciphertext blobs only. """ + from app.models.recovery_challenge import RecoveryChallenge + email = (request.args.get('email') or '').strip().lower() client_proof = request.headers.get('X-Recovery-Proof', '').strip() if not email or not client_proof: return jsonify({'error': 'email and X-Recovery-Proof header are required'}), 400 - # Validate session binding: proof must match what was issued to this session. - expected_proof = session.get('recovery_expected_proof', '') - session_user_id = session.get('recovery_user_id') - - if not expected_proof or not session_user_id: - return jsonify({'error': 'No active recovery challenge. Call /recovery/data first.'}), 400 - user = User.query.filter_by(email=email).first() - if not user or not user.recovery_enc_salt or user.id != session_user_id: + if not user or not user.recovery_enc_salt: return jsonify({'error': 'No recovery data found'}), 404 - if not verify_recovery_proof(expected_proof, client_proof): + # Validate against the DB-stored challenge — safe across all workers. + challenge = RecoveryChallenge.query.filter_by(user_id=user.id).first() + from datetime import datetime, timezone + if not challenge or challenge.expires_at < datetime.now(timezone.utc).replace(tzinfo=None): + return jsonify({'error': 'No active recovery challenge. Call /recovery/data first.'}), 400 + + if not verify_recovery_proof(challenge.expected_proof, client_proof): AuditLog.log( user_id=user.id, action='auth.recovery_items_denied', diff --git a/app/services/auth_service.py b/app/services/auth_service.py index 46289c2..7914039 100644 --- a/app/services/auth_service.py +++ b/app/services/auth_service.py @@ -129,18 +129,29 @@ def blacklist_token(token: str, token_type: str) -> None: if not jti: return exp = payload.get('exp') - expires_at = datetime.fromtimestamp(exp, tz=timezone.utc).replace(tzinfo=None) if exp else datetime.now(timezone.utc).replace(tzinfo=None) + timedelta(days=7) + expires_at = ( + datetime.fromtimestamp(exp, tz=timezone.utc).replace(tzinfo=None) + if exp + else datetime.now(timezone.utc).replace(tzinfo=None) + timedelta(days=7) + ) from app.models.token_blacklist import TokenBlacklist from app import db - # Avoid duplicate if already blacklisted - if not TokenBlacklist.query.filter_by(jti=jti).first(): - entry = TokenBlacklist( - jti=jti, - user_id=int(payload.get('sub', 0)), - expires_at=expires_at, - ) - db.session.add(entry) + from sqlalchemy.exc import IntegrityError + # INSERT directly — no SELECT-before-INSERT race. + # Two concurrent logouts of the same token would both try to insert, + # but the UNIQUE constraint on jti makes exactly one succeed. + # We catch IntegrityError and roll back gracefully; the token is + # already blacklisted so the outcome is correct either way. + entry = TokenBlacklist( + jti=jti, + user_id=int(payload.get('sub', 0)), + expires_at=expires_at, + ) + db.session.add(entry) + try: db.session.commit() + except IntegrityError: + db.session.rollback() # already blacklisted — safe to ignore # Cleanup is handled by the APScheduler background job in create_app(), # not here — keeps the logout/refresh hot path free of extra DB writes. except Exception: diff --git a/extension/popup/popup.js b/extension/popup/popup.js index 4b30dae..88d3483 100644 --- a/extension/popup/popup.js +++ b/extension/popup/popup.js @@ -235,6 +235,13 @@ function isMatch(item) { // ── API helpers ─────────────────────────────────────────────────────────────── +// Singleton promise for the in-flight token refresh. +// When a 401 triggers a refresh, all concurrent requests that also receive a +// 401 await this same promise instead of starting their own — preventing the +// second refresh from using an already-rotated (and therefore blacklisted) +// refresh token, which would cause an unexpected sign-out. +let _refreshPromise = null; + async function apiFetch(path, options = {}) { const { access_token } = await chrome.storage.session.get("access_token"); const headers = { @@ -246,7 +253,13 @@ async function apiFetch(path, options = {}) { let res = await fetch(`${API_BASE}${path}`, { ...options, headers }); if (res.status === 401) { - const refreshed = await tryRefreshToken(); + // Coalesce concurrent 401 retries onto a single refresh attempt. + if (!_refreshPromise) { + _refreshPromise = tryRefreshToken().finally(() => { + _refreshPromise = null; + }); + } + const refreshed = await _refreshPromise; if (!refreshed) { signOut(); return null; @@ -899,7 +912,9 @@ function renderList() { if (!item?.plain?.totp_uri) return; const code = await getTotpCode(item.plain.totp_uri).catch(() => null); if (code) { - navigator.clipboard.writeText(code); + // Use _copyWithAutoClear so the 2FA code is wiped from the clipboard + // after 30 s, consistent with password copy behaviour. + _copyWithAutoClear(code); btn.title = "Copied!"; btn.style.color = "#16a34a"; setTimeout(() => { @@ -1163,7 +1178,7 @@ async function saveCredential(data) { const res = await apiFetch("/api/vault", { method: "POST", body: JSON.stringify({ - name: "password", + name, // actual site name (plaintext fallback for legacy clients) item_type: "password", folder_id, enc_data, @@ -1232,9 +1247,9 @@ function generatePassword(length, useLower, useUpper, useNumbers, useSymbols) { if (useSymbols) required.push(GEN_SETS.symbols[_cryptoRandInt(GEN_SETS.symbols.length)]); - const arr = new Uint32Array(length); - crypto.getRandomValues(arr); - const rest = Array.from(arr).map((n) => pool[n % pool.length]); + // Use _cryptoRandInt() for each character to avoid modulo bias that arises + // when pool.length is not a power of 2. + const rest = Array.from({ length }, () => pool[_cryptoRandInt(pool.length)]); // Splice required chars into random positions and trim to length. const combined = [...rest]; @@ -1462,7 +1477,7 @@ async function addItemToVault() { const res = await apiFetch("/api/vault", { method: "POST", body: JSON.stringify({ - name: "password", + name, // actual site name (plaintext fallback for legacy clients) item_type: "password", folder_id, enc_data, diff --git a/migrations/versions/e5f6a7b8c9d0_add_recovery_challenges_table.py b/migrations/versions/e5f6a7b8c9d0_add_recovery_challenges_table.py new file mode 100644 index 0000000..89ce055 --- /dev/null +++ b/migrations/versions/e5f6a7b8c9d0_add_recovery_challenges_table.py @@ -0,0 +1,63 @@ +"""add recovery_challenges table + +Revision ID: e5f6a7b8c9d0 +Revises: d4e5f6a7b8c9 +Create Date: 2026-05-17 00:00:00.000000 + +Adds the recovery_challenges table, which stores the challenge-response +state for account recovery (nonce + expected HMAC proof) in the database +rather than the Flask session cookie. + +Why: Flask session cookies are per-worker in Gunicorn. A challenge written +by worker A is invisible to worker B, so the recovery flow would fail with +"No active recovery challenge" in any multi-worker deployment. + +The table has a UNIQUE constraint on user_id (one active challenge per user) +and an index on expires_at so the APScheduler cleanup job can efficiently +delete expired rows without a full scan. +""" +from alembic import op +import sqlalchemy as sa + + +revision = 'e5f6a7b8c9d0' +down_revision = 'd4e5f6a7b8c9' +branch_labels = None +depends_on = None + + +def upgrade(): + op.create_table( + 'recovery_challenges', + sa.Column('id', sa.Integer(), nullable=False, autoincrement=True), + sa.Column('user_id', sa.Integer(), nullable=False), + sa.Column('nonce', sa.String(64), nullable=False), + sa.Column('expected_proof', sa.String(64), nullable=False), + sa.Column('expires_at', sa.DateTime(), nullable=False), + sa.ForeignKeyConstraint( + ['user_id'], ['users.id'], + name='fk_recovery_challenges_user_id', + ondelete='CASCADE', + ), + sa.PrimaryKeyConstraint('id'), + sa.UniqueConstraint('user_id', name='uq_recovery_challenges_user_id'), + mysql_charset='utf8mb4', + ) + op.create_index( + 'ix_recovery_challenges_user_id', + 'recovery_challenges', + ['user_id'], + unique=True, + ) + op.create_index( + 'ix_recovery_challenges_expires_at', + 'recovery_challenges', + ['expires_at'], + unique=False, + ) + + +def downgrade(): + op.drop_index('ix_recovery_challenges_expires_at', table_name='recovery_challenges') + op.drop_index('ix_recovery_challenges_user_id', table_name='recovery_challenges') + op.drop_table('recovery_challenges')