Aug 26 - Enhance security

app/routes/auth.py

@@ -31,6 +31,103 @@ auth_bp = Blueprint('auth', __name__)
 EMAIL_RE = re.compile(r'^[^@\s]+@[^@\s]+\.[^@\s]+$')
 
 
+# Fixed-length hex validator for the client-supplied recovery verifier.
+_HEX64_RE = re.compile(r'^[0-9a-f]{64}$')
+
+
+def _recovery_proof_key(user) -> bytes:
+    """
+    Return the HMAC key the recovery challenge-response is computed over.
+
+    Preferred: user.recovery_verifier — a 256-bit value derived client-side from
+    the recovery code alone. It is used for nothing else, so learning it grants
+    no decryption ability, and knowing enc_key_salt does not yield it.
+
+    Legacy fallback: user.enc_key_salt, for recovery codes created before the
+    verifier existed. This is weaker — enc_key_salt is also the vault-key PBKDF2
+    salt and is disclosed to the client on login, so anyone holding the master
+    password can forge a proof and pull the vault from /recovery/items without a
+    second factor. Accounts on this path are flagged via /recovery/status so the
+    settings UI can prompt the user to regenerate.
+    """
+    if user.recovery_verifier:
+        return user.recovery_verifier.encode()
+    return user.enc_key_salt.encode()
+
+
+class IncompleteReencryption(Exception):
+    """
+    Raised when a key-rotation payload does not cover every vault item the user
+    owns. Rotating enc_key_salt while some ciphertext is still under the old key
+    renders those items permanently undecryptable, so the whole transaction is
+    refused unless the caller explicitly opts into partial coverage.
+    """
+
+    def __init__(self, expected: int, received: int):
+        self.expected = expected
+        self.received = received
+        super().__init__(f'expected {expected} item(s), received {received}')
+
+
+def _apply_reencrypted_items(user_id: int, items, allow_partial: bool = False) -> tuple[int, int]:
+    """
+    Apply client-supplied re-encrypted ciphertext to the user's vault items.
+
+    Both the password-change and account-recovery flows rotate enc_key_salt,
+    which invalidates every ciphertext encrypted under the previous vault key.
+    The client is responsible for re-encrypting each item and sending it back;
+    any item missing from that payload is silently orphaned by the rotation.
+
+    This helper therefore counts what it actually wrote and compares it against
+    the number of items the user owns. On a shortfall it raises
+    IncompleteReencryption so the caller can roll back rather than commit a
+    rotation that destroys data.
+
+    allow_partial=True skips the guard. The web client only sets it after
+    showing the user exactly how many items will be lost and getting explicit
+    confirmation — it exists so a single corrupt item cannot lock someone out of
+    recovery entirely.
+
+    Returns (updated_count, total_count). Does not commit.
+    """
+    from app.models.vault_item import VaultItem
+
+    total = VaultItem.query.filter_by(user_id=user_id).count()
+
+    item_ids = [i.get('id') for i in (items or []) if i.get('id')]
+    existing = {
+        v.id: v
+        for v in VaultItem.query.filter(
+            VaultItem.user_id == user_id,
+            VaultItem.id.in_(item_ids),
+        ).all()
+    } if item_ids else {}
+
+    updated = 0
+    for item_data in (items or []):
+        item_id = item_data.get('id')
+        enc_data = item_data.get('enc_data', '')
+        iv = item_data.get('iv', '')
+        if not item_id or not enc_data or not iv:
+            continue
+        vault_item = existing.get(item_id)
+        if not vault_item:
+            continue
+        vault_item.enc_data = enc_data
+        vault_item.iv = iv
+        # Re-encrypt the name ciphertext if the client sent updated enc_name/iv_name.
+        if item_data.get('enc_name'):
+            vault_item.enc_name = item_data['enc_name']
+        if item_data.get('iv_name'):
+            vault_item.iv_name = item_data['iv_name']
+        updated += 1
+
+    if updated != total and not allow_partial:
+        raise IncompleteReencryption(expected=total, received=updated)
+
+    return updated, total
+
+
 @auth_bp.route('/register', methods=['POST'])
 @limiter.limit('10 per minute')
 def register():
@@ -175,13 +272,17 @@ def login():
     )
     db.session.commit()
 
-    # MFA gate: if enabled, issue a short-lived mfa_token instead of full tokens
+    # MFA gate: if enabled, issue a short-lived mfa_token instead of full tokens.
+    #
+    # enc_key_salt is deliberately NOT returned here. It is the PBKDF2 salt for
+    # the vault key, and releasing it to a caller that has only cleared the
+    # password factor is a partial authentication result. The client receives it
+    # from /mfa/verify once the second factor is satisfied.
     if user.totp_enabled:
         mfa_token = generate_mfa_token(user.id)
         return jsonify({
             'mfa_required': True,
             'mfa_token': mfa_token,
-            'enc_key_salt': user.enc_key_salt,
         }), 200
 
     tokens = generate_tokens(user.id)
@@ -437,6 +538,8 @@ def mfa_verify():
     return jsonify({
         'access_token': tokens['access_token'],
         'refresh_token': tokens['refresh_token'],
+        # Released here rather than at /login — both factors are now proven.
+        'enc_key_salt': user.enc_key_salt,
     }), 200
 
 
@@ -581,6 +684,9 @@ def change_password():
     items = data.get('items', [])  # [{id, enc_data, iv, enc_name?, iv_name?}, ...]
     sharing_private_key_enc = data.get('sharing_private_key_enc', '')
     sharing_private_key_iv = data.get('sharing_private_key_iv', '')
+    # Explicit opt-in to rotating the key while some items go un-re-encrypted.
+    # The client must have confirmed the resulting data loss with the user.
+    allow_partial = bool(data.get('allow_partial'))
 
     if not current_auth_hash or not new_auth_hash or not new_enc_key_salt:
         return jsonify({'error': 'current_auth_hash, new_auth_hash, and new_enc_key_salt are required'}), 400
@@ -600,33 +706,11 @@ def change_password():
         return jsonify({'error': 'Current password is incorrect'}), 401
 
     try:
-        from app.models.vault_item import VaultItem
-
-        # Bulk-update all vault item ciphertexts with new vault key encryption
-        item_ids = [i.get('id') for i in items if i.get('id')]
-        existing = {
-            v.id: v
-            for v in VaultItem.query.filter(
-                VaultItem.user_id == user.id,
-                VaultItem.id.in_(item_ids),
-            ).all()
-        } if item_ids else {}
-
-        for item_data in items:
-            item_id = item_data.get('id')
-            enc_data = item_data.get('enc_data', '')
-            iv = item_data.get('iv', '')
-            if not item_id or not enc_data or not iv:
-                continue
-            vault_item = existing.get(item_id)
-            if vault_item:
-                vault_item.enc_data = enc_data
-                vault_item.iv = iv
-                # Re-encrypt the name ciphertext if the client sent updated enc_name/iv_name.
-                if item_data.get('enc_name'):
-                    vault_item.enc_name = item_data['enc_name']
-                if item_data.get('iv_name'):
-                    vault_item.iv_name = item_data['iv_name']
+        # Refuse the rotation outright unless every item was re-encrypted —
+        # see _apply_reencrypted_items. Raises IncompleteReencryption otherwise.
+        updated, total = _apply_reencrypted_items(
+            user.id, items, allow_partial=allow_partial
+        )
 
         # Update credentials
         user.master_hash = hash_auth_token(new_auth_hash)
@@ -634,6 +718,7 @@ def change_password():
         # Clear recovery data — it was encrypted with the old vault key and is now invalid
         user.recovery_enc_salt = None
         user.recovery_iv = None
+        user.recovery_verifier = None
         # Re-encrypt sharing private key with new vault key if the client sent it.
         # Without this update, the old ciphertext would be undecryptable after key rotation.
         if sharing_private_key_enc and sharing_private_key_iv:
@@ -645,10 +730,39 @@ def change_password():
             action='auth.change_password',
             resource_type='user',
             resource_id=user.id,
-            detail=f'Master password changed; {len(existing)} vault item(s) re-encrypted; recovery code cleared',
+            detail=(
+                f'Master password changed; {updated}/{total} vault item(s) re-encrypted'
+                f'{" (PARTIAL — user confirmed data loss)" if updated != total else ""}; '
+                'recovery code cleared'
+            ),
             ip_address=client_ip(),
         )
         db.session.commit()
+    except IncompleteReencryption as exc:
+        db.session.rollback()
+        AuditLog.log(
+            user_id=g.current_user_id,
+            action='auth.change_password_failed',
+            resource_type='user',
+            resource_id=g.current_user_id,
+            detail=(
+                f'Password change refused — re-encryption payload covered '
+                f'{exc.received} of {exc.expected} vault item(s)'
+            ),
+            ip_address=client_ip(),
+        )
+        db.session.commit()
+        return jsonify({
+            'error': (
+                f'Password not changed: the re-encryption payload covered only '
+                f'{exc.received} of your {exc.expected} vault item(s). Completing '
+                'this would permanently lock the missing items. Reload the vault '
+                'and try again.'
+            ),
+            'code': 'incomplete_reencryption',
+            'expected': exc.expected,
+            'received': exc.received,
+        }), 409
     except Exception:
         db.session.rollback()
         _log.exception('change_password failed for user %s', g.current_user_id)
@@ -727,13 +841,20 @@ def recovery_setup():
     data = request.get_json(silent=True) or {}
     recovery_enc_salt = data.get('recovery_enc_salt', '').strip()
     recovery_iv = data.get('recovery_iv', '').strip()
+    # 64 lowercase hex chars, derived client-side from the recovery code alone.
+    recovery_verifier = (data.get('recovery_verifier') or '').strip().lower()
 
     if not recovery_enc_salt or not recovery_iv:
         return jsonify({'error': 'recovery_enc_salt and recovery_iv are required'}), 400
+    if not recovery_verifier or not _HEX64_RE.match(recovery_verifier):
+        return jsonify({
+            'error': 'recovery_verifier must be 64 hexadecimal characters'
+        }), 400
 
     user = db.session.get(User, g.current_user_id)
     user.recovery_enc_salt = recovery_enc_salt
     user.recovery_iv = recovery_iv
+    user.recovery_verifier = recovery_verifier
 
     AuditLog.log(
         user_id=user.id,
@@ -754,7 +875,13 @@ def recovery_setup():
 def recovery_status():
     """Return whether the user has a recovery code configured."""
     user = db.session.get(User, g.current_user_id)
-    return jsonify({'recovery_configured': bool(user.recovery_enc_salt)}), 200
+    return jsonify({
+        'recovery_configured': bool(user.recovery_enc_salt),
+        # True when the stored recovery code predates recovery_verifier and so
+        # still relies on the weaker enc_key_salt-keyed proof. The settings UI
+        # surfaces this as a prompt to regenerate.
+        'recovery_is_legacy': bool(user.recovery_enc_salt and not user.recovery_verifier),
+    }), 200
 
 
 @auth_bp.route('/recover', methods=['POST'])
@@ -771,7 +898,12 @@ def recover_account():
       5. Client POSTs everything here in one atomic payload.
 
     The server validates recovery_proof against the value stored in the DB
-    during /recovery/data — enc_key_salt is never sent in plaintext.
+    during /recovery/data — neither the recovery code nor the verifier is ever
+    sent in plaintext.
+
+    The re-encrypted `items` array must cover every vault item the user owns;
+    otherwise the rotation is refused with 409. Pass allow_partial=true to
+    override once the user has confirmed the resulting data loss.
     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.
@@ -783,6 +915,11 @@ def recover_account():
     new_enc_key_salt = data.get('new_enc_key_salt', '')
     client_proof = data.get('recovery_proof', '')
     items = data.get('items', [])
+    # Explicit opt-in to recovering with some items left un-re-encrypted.
+    # The client sets this only after telling the user how many items it could
+    # not decrypt and getting confirmation — without the escape hatch a single
+    # corrupt row would block recovery entirely.
+    allow_partial = bool(data.get('allow_partial'))
 
     if not all([email, new_auth_hash, new_enc_key_salt, client_proof]):
         return jsonify({'error': 'email, new_auth_hash, new_enc_key_salt, and recovery_proof are required'}), 400
@@ -813,47 +950,59 @@ def recover_account():
 
 
     try:
-        from app.models.vault_item import VaultItem
-
-        item_ids = [i.get('id') for i in items if i.get('id')]
-        existing = {
-            v.id: v
-            for v in VaultItem.query.filter(
-                VaultItem.user_id == user.id,
-                VaultItem.id.in_(item_ids),
-            ).all()
-        } if item_ids else {}
-
-        for item_data in items:
-            item_id = item_data.get('id')
-            enc_data = item_data.get('enc_data', '')
-            iv = item_data.get('iv', '')
-            if not item_id or not enc_data or not iv:
-                continue
-            vault_item = existing.get(item_id)
-            if vault_item:
-                vault_item.enc_data = enc_data
-                vault_item.iv = iv
-                if item_data.get('enc_name'):
-                    vault_item.enc_name = item_data['enc_name']
-                if item_data.get('iv_name'):
-                    vault_item.iv_name = item_data['iv_name']
+        # Refuse to rotate the key while items remain under the old one —
+        # see _apply_reencrypted_items. Raises IncompleteReencryption otherwise.
+        updated, total = _apply_reencrypted_items(
+            user.id, items, allow_partial=allow_partial
+        )
 
         user.master_hash = hash_auth_token(new_auth_hash)
         user.enc_key_salt = new_enc_key_salt
         # Recovery code is consumed — clear it so it cannot be reused.
         user.recovery_enc_salt = None
         user.recovery_iv = None
+        user.recovery_verifier = None
 
         AuditLog.log(
             user_id=user.id,
             action='auth.recovery_success',
             resource_type='user',
             resource_id=user.id,
-            detail=f'Account recovered; {len(existing)} vault item(s) re-encrypted; recovery code consumed',
+            detail=(
+                f'Account recovered; {updated}/{total} vault item(s) re-encrypted'
+                f'{" (PARTIAL — user confirmed data loss)" if updated != total else ""}; '
+                'recovery code consumed'
+            ),
             ip_address=client_ip(),
         )
         db.session.commit()
+    except IncompleteReencryption as exc:
+        # The challenge was already consumed above, so the client must restart
+        # from /recovery/data. That is the correct trade-off: better to repeat
+        # the flow than to commit a rotation that orphans ciphertext.
+        db.session.rollback()
+        AuditLog.log(
+            user_id=user.id,
+            action='auth.recovery_failed',
+            resource_type='user',
+            resource_id=user.id,
+            detail=(
+                f'Recovery refused — re-encryption payload covered '
+                f'{exc.received} of {exc.expected} vault item(s)'
+            ),
+            ip_address=client_ip(),
+        )
+        db.session.commit()
+        return jsonify({
+            'error': (
+                f'Recovery stopped: only {exc.received} of your {exc.expected} '
+                'vault item(s) could be re-encrypted. Continuing would permanently '
+                'lock the rest.'
+            ),
+            'code': 'incomplete_reencryption',
+            'expected': exc.expected,
+            'received': exc.received,
+        }), 409
     except Exception:
         db.session.rollback()
         _log.exception('recover_account failed for user %s', user.id)
@@ -876,11 +1025,15 @@ def recovery_data():
     Exposes: enc_key_salt, recovery_enc_salt, recovery_iv, and a one-time nonce.
 
     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)
+      - Client derives the recovery verifier from the recovery code:
+            PBKDF2(recovery_code, "passkeeper-recovery-verifier:" + email, 200k)
+      - Client computes: proof = HMAC-SHA256(key=verifier, msg=nonce)
       - 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.
+        verifying it on /recover and /recovery/items.
+
+    The response's `proof_scheme` field tells the client which key to use.
+    Accounts whose recovery code predates recovery_verifier get 'legacy' and
+    key the proof on the enc_key_salt decrypted out of the recovery blob.
 
     Returns 404 if no recovery code is configured (prevents user enumeration).
     The challenge is stored in the database (not the Flask session cookie) so
@@ -903,7 +1056,7 @@ def recovery_data():
     # enc_key_salt in plaintext.
     nonce = generate_recovery_nonce()
     expected_proof = _hmac.new(
-        user.enc_key_salt.encode(),
+        _recovery_proof_key(user),
         nonce.encode(),
         hashlib.sha256,
     ).hexdigest()
@@ -922,6 +1075,10 @@ def recovery_data():
         'recovery_enc_salt': user.recovery_enc_salt,
         'recovery_iv': user.recovery_iv,
         'nonce': nonce,
+        # Tells the client which value to key the HMAC proof with:
+        #   'verifier' → PBKDF2(recovery_code, "passkeeper-recovery-verifier:" + email)
+        #   'legacy'   → the enc_key_salt decrypted out of the recovery blob
+        'proof_scheme': 'verifier' if user.recovery_verifier else 'legacy',
     }), 200
 
 
@@ -932,11 +1089,14 @@ def recovery_items():
     Return encrypted vault items for recovery re-encryption (unauthenticated).
 
     Requires X-Recovery-Proof header containing the HMAC-SHA256 proof:
-        proof = HMAC-SHA256(key=enc_key_salt_bytes, msg=nonce_from_recovery_data)
+        proof = HMAC-SHA256(key=recovery_verifier, msg=nonce_from_recovery_data)
 
-    The enc_key_salt used as the HMAC key is NOT returned by /recovery/data;
-    the client must derive it by decrypting the recovery blob with the recovery
-    code. This ensures only the holder of the recovery code can compute the proof.
+    The verifier is derived client-side from the recovery code alone and is
+    never returned by any endpoint, so only the holder of the recovery code can
+    compute the proof. It is deliberately not enc_key_salt: that value is also
+    the vault-key PBKDF2 salt and is released to the client on login, so keying
+    the proof with it allowed anyone holding the master password to forge a
+    proof and pull the whole vault here — bypassing MFA.
 
     Replay prevention: the challenge is consumed (deleted) on success, then
     immediately re-issued with the same expected_proof but a new nonce and a