From 6c1bef73c87fc2dbbabba96508eb803c3f17dbbb Mon Sep 17 00:00:00 2001 From: NguyenND Date: Wed, 26 Aug 2026 12:54:17 -0400 Subject: [PATCH] Aug 26 - Enhance security 2 --- .gitea/workflows/ci.yml | 40 +++- CLAUDE.md | 60 ++++- Deloy.md | 22 +- app/__init__.py | 13 +- app/config.py | 98 ++++++--- app/models/user.py | 8 + app/routes/auth.py | 46 ++-- app/routes/vault.py | 4 +- app/routes/webauthn.py | 41 +++- app/services/auth_service.py | 62 +++++- .../versions/k1l2m3n4o5p6_add_token_epoch.py | 50 +++++ pytest.ini | 7 + requirements-dev.txt | 4 + scripts/passkeeper-nginx.conf | 51 ++++- tests/conftest.py | 75 +++++++ tests/test_deploy_config.py | 109 +++++++++ tests/test_key_rotation.py | 208 ++++++++++++++++++ tests/test_mfa_gate.py | 143 ++++++++++++ tests/test_session_revocation.py | 148 +++++++++++++ tests/test_webauthn_uv.py | 83 +++++++ 20 files changed, 1193 insertions(+), 79 deletions(-) create mode 100644 migrations/versions/k1l2m3n4o5p6_add_token_epoch.py create mode 100644 pytest.ini create mode 100644 requirements-dev.txt create mode 100644 tests/conftest.py create mode 100644 tests/test_deploy_config.py create mode 100644 tests/test_key_rotation.py create mode 100644 tests/test_mfa_gate.py create mode 100644 tests/test_session_revocation.py create mode 100644 tests/test_webauthn_uv.py diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index 30fb472..45675cf 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -9,6 +9,7 @@ # syntax-check — ast.parse all Python files # migration-check — verify Alembic chain has single head # js-syntax — node syntax check on all JS files +# tests — pytest suite (in-memory SQLite, no MySQL needed) # build-extension — zip Chrome and Firefox extensions name: CI @@ -48,12 +49,23 @@ jobs: - name: Check all Python files parse cleanly run: | python3 - << 'EOF' - import ast, sys, pathlib + import ast, sys, pathlib, itertools + + # Root-level modules (wsgi, run, reset_db, gunicorn.conf) were not + # covered before, so a syntax error in the Gunicorn config or the WSGI + # entrypoint reached production without CI noticing. + paths = list(itertools.chain( + pathlib.Path('app').rglob('*.py'), + pathlib.Path('tests').rglob('*.py'), + pathlib.Path('scripts').rglob('*.py'), + pathlib.Path('migrations/versions').rglob('*.py'), + pathlib.Path('.').glob('*.py'), + )) failures = [] - for path in pathlib.Path('app').rglob('*.py'): + for path in paths: try: - ast.parse(path.read_text()) + ast.parse(path.read_text(encoding='utf-8')) except SyntaxError as e: failures.append(f"{path}: {e}") @@ -61,8 +73,7 @@ jobs: print(f"FAIL: {f}") if failures: sys.exit(1) - count = len(list(pathlib.Path('app').rglob('*.py'))) - print(f"OK: {count} Python files parsed cleanly") + print(f"OK: {len(paths)} Python files parsed cleanly") EOF # ── Alembic migration chain ────────────────────────────────────────────────── @@ -130,11 +141,28 @@ jobs: [ $FAILED -eq 0 ] && echo "OK: all JS files parsed cleanly" exit $FAILED + # ── Test suite ─────────────────────────────────────────────────────────────── + # Runs against in-memory SQLite (see app/config.py TestingConfig) so no MySQL + # service is needed on the host-mode runner. That means these tests cover + # application logic and flow, not MySQL-specific behaviour — schema changes + # still need a real `flask db upgrade` against MySQL before deploying. + tests: + name: Pytest + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + + - name: Install dependencies + run: pip3 install -r requirements.txt -r requirements-dev.txt --quiet --break-system-packages + + - name: Run test suite + run: python3 -m pytest tests/ -q + # ── Extension build ────────────────────────────────────────────────────────── build-extension: name: Build extension zip runs-on: ubuntu-latest - needs: [syntax-check, js-syntax] + needs: [syntax-check, js-syntax, tests] steps: - uses: actions/checkout@v3 diff --git a/CLAUDE.md b/CLAUDE.md index 9127f08..7f9a050 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -109,6 +109,16 @@ passkeeper/ │ ├── reencrypt_totp_secrets.py │ ├── backup_db.sh / backup.cron / passkeeper-logrotate │ ├── passkeeper-nginx.conf / passkeeper.service +├── tests/ # pytest suite — in-memory SQLite, no MySQL needed +│ ├── conftest.py # app/client fixtures + register/login helpers +│ ├── test_mfa_gate.py # enc_key_salt withheld until MFA; proof not forgeable +│ ├── test_key_rotation.py # re-encryption completeness guard +│ ├── test_session_revocation.py # token_epoch revocation; deleted-account 401 +│ ├── test_webauthn_uv.py # user verification required on both ceremonies +│ └── test_deploy_config.py # nginx/gunicorn/systemd invariants (502 guards) +├── gunicorn.conf.py # worker class, timeouts, preload_app=False +├── pytest.ini +├── requirements-dev.txt ├── reset_db.py ├── requirements.txt ├── wsgi.py / run.py @@ -135,7 +145,9 @@ CREATE TABLE users ( sharing_private_key_enc TEXT, sharing_private_key_iv VARCHAR(64), recovery_enc_salt VARCHAR(128), - recovery_iv VARCHAR(64) + recovery_iv VARCHAR(64), + recovery_verifier VARCHAR(64), -- HMAC key for the recovery challenge + token_epoch INT NOT NULL DEFAULT 0 -- session generation counter ); -- Vault Items @@ -225,6 +237,9 @@ CREATE TABLE webauthn_credentials ( | `f6a7b8c9d0e1` | Add totp_used_codes table (TOTP replay prevent.) | | `g7h8i9j0k1l2` | Add enc_name/iv_name to shared_items | | `h8i9j0k1l2m3` | Add webauthn_credentials table (passkeys) | +| `i9j0k1l2m3n4` | Add expires_at to shared_items | +| `j0k1l2m3n4o5` | Add recovery_verifier (decouple recovery proof) | +| `k1l2m3n4o5p6` | Add token_epoch (revoke sessions on pw change) | --- @@ -238,7 +253,14 @@ CREATE TABLE webauthn_credentials ( - **Shared item name:** `enc_name`/`iv_name` encrypted with ECDH shared key; server `item_name` = item type only - **Tags:** `plain.tags: string[]` inside `enc_data`; server never sees them - **Argon2id:** double-hashes `authHash` server-side; transparently rehashes on login if parameters are upgraded -- **JWT:** HS256, 15 min access / 7 day refresh, JTI blacklisted on logout +- **JWT:** HS256, 15 min access / 7 day refresh, JTI blacklisted on logout. + Every token carries an `epoch` claim checked against `users.token_epoch`; + changing the master password or recovering the account increments it, which + revokes every outstanding access AND refresh token. Tokens minted before the + claim existed decode as `epoch 0` and remain valid until the next change. +- **MFA gate:** `enc_key_salt` is NOT returned by `/login` when TOTP is enabled — + it is released by `/mfa/verify` once both factors are proven. Returning it + early let a password-only attacker forge a recovery proof (see below). - **MFA:** TOTP secret AES-256-GCM encrypted at rest; each code is single-use (replay prevented via `totp_used_codes` table, 120s TTL) - **Passkeys / WebAuthn:** server authentication via FIDO2; ZK model preserved — WebAuthn proves identity to the server but the vault key is still derived from the master password client-side; `sign_count` updated on every assertion for clone detection - **Sharing:** ECDH P-256 zero-knowledge re-encryption; item name also encrypted with shared key @@ -246,6 +268,24 @@ CREATE TABLE webauthn_credentials ( - **Decrypted vault data:** `chrome.storage.session` only — never to disk - **Clipboard auto-clear:** 30 s after any password/username copy (web + extension) - **Breach detection:** HIBP k-anonymity — only 5-char SHA-1 prefix transmitted +- **Recovery proof key:** `HMAC-SHA256(key=recovery_verifier, msg=nonce)`. + `recovery_verifier` = `PBKDF2(recoveryCode, "passkeeper-recovery-verifier:" + email, 200k)`, + derived client-side from the recovery code alone and used for nothing else. + It must NEVER be `enc_key_salt`: that value doubles as the vault-key PBKDF2 + salt and is disclosed to the client at login, so keying the proof with it + allowed anyone holding the master password to pull the entire vault from the + unauthenticated `/recovery/items` — bypassing MFA. Accounts whose recovery + code predates the column fall back to the legacy key and are flagged via + `recovery_is_legacy` on `/recovery/status`. +- **Key rotation completeness:** `change_password` and `/recover` refuse (409 + `incomplete_reencryption`) unless the client's `items` payload covers every + vault item the user owns — a short payload would rotate `enc_key_salt` and + leave the missing items permanently undecryptable. `/recover` accepts + `allow_partial: true` after the user confirms the loss (otherwise one corrupt + item locks them out forever); `change_password` has no such override. +- **Passkeys:** both ceremonies use `UserVerificationRequirement.REQUIRED` and + `require_user_verification=True`. A passkey replaces password AND TOTP, so + possession of an unlocked device must not be sufficient. - **Account recovery:** challenge-response via HMAC-SHA256; `enc_key_salt` NOT returned by `/recovery/data` — client must derive it by decrypting the recovery blob (proves possession of recovery code without transmitting it); challenge rotated on each `/recovery/items` call to prevent proof replay; recovery key derived with the user's email as a per-user PBKDF2 salt — legacy fixed salt `'passkeeper-recovery'` accepted transparently for codes created before this change - **folder_id ownership:** validated server-side on all create/update/import operations — user cannot assign items to another user's folder - **Audit logs:** never contain plaintext item names, shared item names, or vault data @@ -590,6 +630,12 @@ Audit log details **never** contain plaintext item names, shared item names, or - WebAuthn `attachment`: `"cross-platform"` for security keys; `"platform"` for device biometrics (default) - `enc_vault_is_legacy` check in `EmergencyAccess.to_dict()` is pure JSON inspection — no decryption - Never return `str(e)` from exception handlers — log with `_log.exception(...)` and return a generic user-facing message to avoid leaking DB schema details or query fragments +- `preload_app` must stay `False` in `gunicorn.conf.py` — APScheduler's thread does not survive `fork()`, so `--preload` silently disables the cleanup job +- nginx `proxy_read_timeout` must stay BELOW gunicorn `timeout`, else a slow request returns 502 instead of 504 +- `WatchdogSec` in the systemd unit requires `Type=notify` + `NotifyAccess=main`; without them systemd SIGKILLs the service on a loop +- `ExecReload` must not use `USR2` — it forks a second master and strands `$MAINPID` +- `ProductionConfig.validate()` is called from `create_app()`, NOT at class-definition time — running it in the class body made `import app.config` fail without production secrets, breaking tests and local tooling +- Always pass `user.token_epoch` to `generate_tokens()` — the `0` default exists only so stale call sites fail visibly - `SharingCrypto.decryptPrivateKey` imports the key with `extractable: false` — use raw `SubtleCrypto` calls when you need the key bytes (e.g. re-encryption on password change) --- @@ -785,6 +831,15 @@ page itself. Features planned for future implementation. Ordered by priority within each category. +### Recently completed (Aug 2026) + +- MFA bypass closed: `enc_key_salt` withheld until the second factor; recovery + proof rebased onto `recovery_verifier` +- Key-rotation completeness guard on `change_password` / `/recover` +- Session revocation via `token_epoch`; `require_jwt` now verifies the user exists +- Passkey ceremonies require user verification +- pytest suite (37 tests) + CI job; `gunicorn.conf.py`; systemd watchdog removed + ### High priority — user-facing **1. One-time share links** @@ -872,6 +927,7 @@ Extend `backup.cron` to run a weekly restore test into a throwaway DB, verifying 2. `syntax-check` — `ast.parse` all `app/` Python files 3. `migration-check` — single Alembic head, no duplicate revision IDs 4. `js-syntax` — `node -e "new Function(...)"` on all vault/extension JS files +4b. `tests` — pytest suite against in-memory SQLite (`TestingConfig`); gates `build-extension` 5. `build-extension` — produces `passkeeper-extension-chrome.zip` and `passkeeper-extension-firefox.zip` as artifacts (30-day retention) **Runner:** self-hosted host-mode runner on the production server. Requires `python3`, `pip3`, `node`, `zip` on the host. No Docker needed. \ No newline at end of file diff --git a/Deloy.md b/Deloy.md index a6b14ec..2fa8bec 100644 --- a/Deloy.md +++ b/Deloy.md @@ -266,12 +266,22 @@ atomically on any error. ## Step 8 — Test Gunicorn manually -Before installing the systemd service, verify Gunicorn can start the app: +Before installing the systemd service, verify Gunicorn can start the app. + +> **Do not add `--preload`.** `create_app()` starts an APScheduler thread for the +> hourly cleanup of `token_blacklist` / `recovery_challenges` / `totp_used_codes` +> / expired shares. Threads do not survive `fork()`, so under `--preload` the +> scheduler would live only in the arbiter — which serves no requests — and the +> cleanup would silently never run. `gunicorn.conf.py` pins `preload_app = False` +> for this reason. ```bash source /home/spuser/.venv/bin/activate cd /home/spuser/PassKeeper -gunicorn --workers 4 --bind 127.0.0.1:5000 --preload wsgi:app +gunicorn -c gunicorn.conf.py wsgi:app + +# Validate the config without starting the server: +gunicorn --check-config -c gunicorn.conf.py wsgi:app ``` You should see lines like: @@ -527,8 +537,12 @@ pip install -r requirements.txt export FLASK_APP=wsgi.py FLASK_ENV=production flask db upgrade -# Reload Gunicorn zero-downtime (sends USR2 to master) -sudo systemctl reload passkeeper +# Restart Gunicorn to pick up the new code. +# NOT `reload` — ExecReload sends HUP, which re-reads gunicorn.conf.py and +# recycles workers but does NOT reload changed Python source. Using reload after +# a code deploy leaves the old code running and looks like the deploy silently +# did nothing. +sudo systemctl restart passkeeper # Verify sudo systemctl status passkeeper diff --git a/app/__init__.py b/app/__init__.py index ddc923f..41323b0 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -42,7 +42,14 @@ def create_app(config_name: str = 'development') -> Flask: # IP keys spoof-resistant — a client cannot bypass per-IP limits by injecting # an arbitrary IP into the XFF header. app.wsgi_app = ProxyFix(app.wsgi_app, x_for=1, x_proto=1, x_host=1) - app.config.from_object(config[config_name]) + selected_config = config[config_name] + # Reject insecure production settings before the app is handed back, so no + # request is ever served under one. Deliberately not done at class-definition + # time — that made importing app.config impossible without production + # secrets, breaking tests and local tooling. + if hasattr(selected_config, 'validate'): + selected_config.validate() + app.config.from_object(selected_config) # Extensions db.init_app(app) @@ -230,7 +237,9 @@ def create_app(config_name: str = 'development') -> Flask: # WERKZEUG_RUN_MAIN == 'true' only in the child (the actual server), # so we skip the scheduler in the parent reloader to avoid duplicate jobs. import os - if not app.debug or os.environ.get('WERKZEUG_RUN_MAIN') == 'true': + if app.config.get('SCHEDULER_ENABLED', True) and ( + not app.debug or os.environ.get('WERKZEUG_RUN_MAIN') == 'true' + ): scheduler.start() # ── Production safety checks ─────────────────────────────────────────────── diff --git a/app/config.py b/app/config.py index 5108c91..006326a 100644 --- a/app/config.py +++ b/app/config.py @@ -78,6 +78,11 @@ class BaseConfig: # Set via .env: STATIC_VERSION=20260418 STATIC_VERSION = os.environ.get('STATIC_VERSION', '1') + # Background cleanup scheduler (token_blacklist / recovery_challenges / + # totp_used_codes / expired shares). Disabled under test so the suite does + # not spawn a daemon thread per app fixture. + SCHEDULER_ENABLED = True + # Session cookie defaults — applied in all environments. # SECURE is intentionally left out of BaseConfig so dev HTTP still works. # See ProductionConfig below for the full hardened set. @@ -90,6 +95,35 @@ class DevelopmentConfig(BaseConfig): RATELIMIT_ENABLED = False +class TestingConfig(BaseConfig): + """ + In-memory SQLite, no rate limiting, no background threads. + + SQLite is viable here because the only MySQL-specific construct in the + models is mysql.INTEGER(unsigned=True), which SQLAlchemy renders as a plain + INTEGER on other dialects. That means these tests cover application logic + and flow, NOT MySQL-specific behaviour (collation, ON UPDATE NOW(), unsigned + range) — schema changes still need a real migration run against MySQL. + """ + TESTING = True + DEBUG = False + SQLALCHEMY_DATABASE_URI = 'sqlite:///:memory:' + SQLALCHEMY_ENGINE_OPTIONS = {} + RATELIMIT_ENABLED = False + SCHEDULER_ENABLED = False + WTF_CSRF_ENABLED = False + SECRET_KEY = 'test-secret-not-used-in-production' + JWT_SECRET_KEY = 'test-jwt-secret-not-used-in-production' + TOTP_ENCRYPTION_KEY = '00' * 32 + CORS_ORIGINS = 'http://localhost' + WEBAUTHN_RP_ID = 'localhost' + WEBAUTHN_ORIGINS = ['http://localhost'] + # Keep Argon2 cheap so the suite is not dominated by password hashing. + ARGON2_TIME_COST = 1 + ARGON2_MEMORY_COST = 8 + ARGON2_PARALLELISM = 1 + + class ProductionConfig(BaseConfig): DEBUG = False RATELIMIT_ENABLED = True @@ -98,40 +132,48 @@ class ProductionConfig(BaseConfig): SESSION_COOKIE_HTTPONLY = True SESSION_COOKIE_SAMESITE = 'Lax' - # ── Critical security checks — fail loudly at startup, not silently at runtime ── - # These checks run at class definition time (i.e. at import / app startup). - # Any misconfiguration raises RuntimeError immediately so the process never - # serves a single request with an insecure configuration. + @classmethod + def validate(cls): + """ + Fail loudly on insecure production configuration. - _secret_key = os.environ.get('SECRET_KEY', '') - if not _secret_key or _secret_key in _INSECURE_SECRET_DEFAULTS: - raise RuntimeError( - '[PassKeeper] SECRET_KEY is not set or uses an insecure placeholder. ' - 'Generate a strong key with: python -c "import secrets; print(secrets.token_hex(32))" ' - 'and add SECRET_KEY= to your production .env file.' - ) - SECRET_KEY = _secret_key + Called from create_app() when this config is selected — NOT at class + definition time. Running it in the class body meant merely *importing* + app.config raised unless production secrets were present in the + environment, which broke the test suite and any local tooling that + imports the app (including `flask db upgrade` on a dev box). - _jwt_secret = os.environ.get('JWT_SECRET_KEY', '') - if not _jwt_secret or _jwt_secret in _INSECURE_SECRET_DEFAULTS: - raise RuntimeError( - '[PassKeeper] JWT_SECRET_KEY is not set or uses an insecure placeholder. ' - 'Generate a strong key with: python -c "import secrets; print(secrets.token_hex(32))" ' - 'and add JWT_SECRET_KEY= to your production .env file.' - ) - JWT_SECRET_KEY = _jwt_secret + The fail-loud property is preserved: create_app('production') raises + before the app is returned, so the process still never serves a request + under an insecure configuration. + """ + secret_key = os.environ.get('SECRET_KEY', '') + if not secret_key or secret_key in _INSECURE_SECRET_DEFAULTS: + raise RuntimeError( + '[PassKeeper] SECRET_KEY is not set or uses an insecure placeholder. ' + 'Generate a strong key with: python -c "import secrets; print(secrets.token_hex(32))" ' + 'and add SECRET_KEY= to your production .env file.' + ) - _cors = os.environ.get('CORS_ORIGINS', '') - if not _cors or _cors.strip() == '*': - raise RuntimeError( - '[PassKeeper] CORS_ORIGINS must be set to a specific origin in production ' - '(e.g. CORS_ORIGINS=https://pwkeeper.ngodanguyen.tech). ' - 'A wildcard "*" is not permitted in production.' - ) - CORS_ORIGINS = _cors + jwt_secret = os.environ.get('JWT_SECRET_KEY', '') + if not jwt_secret or jwt_secret in _INSECURE_SECRET_DEFAULTS: + raise RuntimeError( + '[PassKeeper] JWT_SECRET_KEY is not set or uses an insecure placeholder. ' + 'Generate a strong key with: python -c "import secrets; print(secrets.token_hex(32))" ' + 'and add JWT_SECRET_KEY= to your production .env file.' + ) + + cors = os.environ.get('CORS_ORIGINS', '') + if not cors or cors.strip() == '*': + raise RuntimeError( + '[PassKeeper] CORS_ORIGINS must be set to a specific origin in production ' + '(e.g. CORS_ORIGINS=https://pwkeeper.ngodanguyen.tech). ' + 'A wildcard "*" is not permitted in production.' + ) config = { 'development': DevelopmentConfig, + 'testing': TestingConfig, 'production': ProductionConfig, } \ No newline at end of file diff --git a/app/models/user.py b/app/models/user.py index 5062cb4..167b54f 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -58,6 +58,14 @@ class User(db.Model, UserMixin): # Each code is consumed (removed from the array) on use. # NULL means no backup codes have been generated yet. mfa_backup_codes = db.Column(db.Text, nullable=True) + # Session generation counter. Every issued JWT carries the value current at + # the time it was minted; require_jwt rejects tokens whose claim no longer + # matches. Incrementing this revokes every outstanding access and refresh + # token at once, which is what a master-password change must do — otherwise + # a stolen refresh token outlives the password it was obtained under. + # Tokens issued before this column existed decode with epoch 0 and stay + # valid until the next credential change. + token_epoch = db.Column(db.Integer, default=0, nullable=False, server_default='0') folders = db.relationship('Folder', backref='owner', lazy='dynamic', cascade='all, delete-orphan') vault_items = db.relationship('VaultItem', backref='owner', lazy='dynamic', cascade='all, delete-orphan') diff --git a/app/routes/auth.py b/app/routes/auth.py index edd9e00..0545847 100644 --- a/app/routes/auth.py +++ b/app/routes/auth.py @@ -4,7 +4,6 @@ import time from flask import Blueprint, request, jsonify, g -_log = logging.getLogger(__name__) from app import db, limiter, client_ip from app.models.user import User from app.models.audit_log import AuditLog @@ -12,6 +11,7 @@ from app.services.auth_service import ( hash_auth_token, verify_auth_token, generate_tokens, + load_user_for_token, generate_mfa_token, decode_token, blacklist_token, @@ -26,6 +26,8 @@ from app.services.auth_service import ( mark_totp_code_used, ) +_log = logging.getLogger(__name__) + auth_bp = Blueprint('auth', __name__) EMAIL_RE = re.compile(r'^[^@\s]+@[^@\s]+\.[^@\s]+$') @@ -285,7 +287,7 @@ def login(): 'mfa_token': mfa_token, }), 200 - tokens = generate_tokens(user.id) + tokens = generate_tokens(user.id, user.token_epoch) return jsonify({ 'access_token': tokens['access_token'], 'refresh_token': tokens['refresh_token'], @@ -322,9 +324,17 @@ def refresh(): except Exception: return jsonify({'error': 'Invalid or expired refresh token'}), 401 + # Same gate as require_jwt: the account must still exist and the token's + # epoch must still match. Without this a refresh token captured before a + # password change could keep minting fresh access tokens for its full + # 7-day lifetime, defeating the revocation entirely. + user = load_user_for_token(payload) + if user is None: + return jsonify({'error': 'Session is no longer valid. Please log in again.'}), 401 + # Rotate: blacklist old refresh token and issue fresh pair blacklist_token(refresh_token, 'refresh') - tokens = generate_tokens(int(payload['sub'])) + tokens = generate_tokens(user.id, user.token_epoch) return jsonify({ 'access_token': tokens['access_token'], 'refresh_token': tokens['refresh_token'], @@ -534,7 +544,7 @@ def mfa_verify(): ) db.session.commit() - tokens = generate_tokens(user.id) + tokens = generate_tokens(user.id, user.token_epoch) return jsonify({ 'access_token': tokens['access_token'], 'refresh_token': tokens['refresh_token'], @@ -684,9 +694,12 @@ 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')) + # NOTE: there is deliberately no allow_partial opt-in here. + # + # Recovery needs one, because refusing outright leaves a locked-out user with + # no way into their account. Changing the password has no such pressure — the + # current password keeps working — so accepting data loss is never the right + # answer, and the server refuses regardless of what the client asks for. 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 @@ -708,9 +721,7 @@ def change_password(): try: # 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 - ) + updated, total = _apply_reencrypted_items(user.id, items) # Update credentials user.master_hash = hash_auth_token(new_auth_hash) @@ -719,6 +730,10 @@ def change_password(): user.recovery_enc_salt = None user.recovery_iv = None user.recovery_verifier = None + # Revoke every token issued under the old password. Without this the + # "Please log in again" message below is advisory only — outstanding + # refresh tokens would stay valid for their full 7-day lifetime. + user.token_epoch = (user.token_epoch or 0) + 1 # 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: @@ -731,9 +746,8 @@ def change_password(): resource_type='user', resource_id=user.id, 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' + f'Master password changed; {updated}/{total} vault item(s) ' + 're-encrypted; recovery code cleared' ), ip_address=client_ip(), ) @@ -962,6 +976,10 @@ def recover_account(): user.recovery_enc_salt = None user.recovery_iv = None user.recovery_verifier = None + # Recovery resets the master password, so revoke prior sessions too — + # an attacker holding a stolen token must not survive the victim + # recovering their account. + user.token_epoch = (user.token_epoch or 0) + 1 AuditLog.log( user_id=user.id, @@ -1008,7 +1026,7 @@ def recover_account(): _log.exception('recover_account failed for user %s', user.id) return jsonify({'error': 'Account recovery failed. Please try again.'}), 500 - tokens = generate_tokens(user.id) + tokens = generate_tokens(user.id, user.token_epoch) return jsonify({ 'message': 'Account recovered successfully', 'access_token': tokens['access_token'], diff --git a/app/routes/vault.py b/app/routes/vault.py index 69ae9e8..2b0f002 100644 --- a/app/routes/vault.py +++ b/app/routes/vault.py @@ -2,13 +2,13 @@ import logging from flask import Blueprint, request, jsonify, g from app import db, limiter, client_ip - -_log = logging.getLogger(__name__) from app.models.vault_item import VaultItem, ItemType from app.models.folder import Folder from app.models.audit_log import AuditLog from app.services.auth_service import require_jwt +_log = logging.getLogger(__name__) + vault_bp = Blueprint('vault', __name__) VALID_TYPES = {t.value for t in ItemType} diff --git a/app/routes/webauthn.py b/app/routes/webauthn.py index 3866dcb..072dd32 100644 --- a/app/routes/webauthn.py +++ b/app/routes/webauthn.py @@ -27,6 +27,7 @@ Challenge storage: is stored client-side (signed, not encrypted — the challenge is not secret). """ import json +import logging from datetime import datetime, timezone import webauthn @@ -50,6 +51,8 @@ from app.services.auth_service import require_jwt, generate_tokens webauthn_bp = Blueprint('webauthn', __name__) +_log = logging.getLogger(__name__) + # Session key for the pending challenge bytes. _REG_CHALLENGE_KEY = 'webauthn_reg_challenge' _AUTH_CHALLENGE_KEY = 'webauthn_auth_challenge' @@ -113,7 +116,12 @@ def register_begin(): user_display_name=user.email, authenticator_selection=AuthenticatorSelectionCriteria( resident_key=ResidentKeyRequirement.PREFERRED, - user_verification=UserVerificationRequirement.PREFERRED, + # REQUIRED, not PREFERRED. A passkey here replaces BOTH the password + # and the TOTP second factor, so the authenticator must actually + # verify the human (biometric or PIN) rather than merely prove it is + # present. Under PREFERRED an authenticator is free to skip that, + # which reduced a full login to possession of an unlocked device. + user_verification=UserVerificationRequirement.REQUIRED, authenticator_attachment=authenticator_attachment, ), exclude_credentials=exclude_credentials, @@ -156,10 +164,18 @@ def register_complete(): expected_challenge=expected_challenge, expected_rp_id=_rp_id(), expected_origin=_origins(), - require_user_verification=False, + # Reject a credential created without user verification — otherwise + # the REQUIRED hint above is only a request, not a guarantee. + require_user_verification=True, ) - except (InvalidCBORData, InvalidRegistrationResponse, Exception) as e: - return jsonify({'error': f'Registration verification failed: {str(e)}'}), 400 + except (InvalidCBORData, InvalidRegistrationResponse) as e: + # Never echo str(e) to the client: py-webauthn messages quote raw + # attestation internals. Log the detail, return a generic message. + _log.warning('[PassKeeper] passkey registration rejected: %s', e) + return jsonify({'error': 'Could not verify this passkey. Please try again.'}), 400 + except Exception: + _log.exception('[PassKeeper] passkey registration failed unexpectedly') + return jsonify({'error': 'Could not verify this passkey. Please try again.'}), 400 # Persist the new credential. import base64 @@ -231,7 +247,8 @@ def authenticate_begin(): options = webauthn.generate_authentication_options( rp_id=_rp_id(), allow_credentials=allow_credentials, - user_verification=UserVerificationRequirement.PREFERRED, + # See register_begin — this assertion stands in for password + MFA. + user_verification=UserVerificationRequirement.REQUIRED, ) session[_AUTH_CHALLENGE_KEY] = webauthn.options_to_json(options) @@ -296,15 +313,21 @@ def authenticate_complete(): expected_origin=_origins(), credential_public_key=webauthn.base64url_to_bytes(credential.public_key), credential_current_sign_count=credential.sign_count, - require_user_verification=False, + # Enforced, not merely requested: the assertion must carry the UV + # flag or it is not sufficient to stand in for two factors. + require_user_verification=True, + ) + except Exception: + _log.warning( + '[PassKeeper] passkey assertion rejected for credential id=%s', + credential.id, exc_info=True, ) - except Exception as e: AuditLog.log( user_id=user.id, action='webauthn.auth_failed', resource_type='webauthn_credential', resource_id=credential.id, - detail=f'Passkey authentication failed: {str(e)[:200]}', + detail='Passkey authentication failed (assertion rejected)', ip_address=client_ip(), ) db.session.commit() @@ -315,7 +338,7 @@ def authenticate_complete(): credential.last_used_at = datetime.now(timezone.utc).replace(tzinfo=None) # Issue tokens. - tokens = generate_tokens(user.id) + tokens = generate_tokens(user.id, user.token_epoch) AuditLog.log( user_id=user.id, diff --git a/app/services/auth_service.py b/app/services/auth_service.py index 6c69540..90e83fc 100644 --- a/app/services/auth_service.py +++ b/app/services/auth_service.py @@ -84,14 +84,23 @@ def decrypt_totp_secret(ciphertext_b64: str, iv_b64: str) -> str: return aesgcm.decrypt(iv, ciphertext, None).decode() -def generate_tokens(user_id: int) -> dict: - """Return access_token and refresh_token JWTs, each with a unique jti.""" +def generate_tokens(user_id: int, token_epoch: int = 0) -> dict: + """ + Return access_token and refresh_token JWTs, each with a unique jti. + + token_epoch stamps the user's current session generation into both tokens. + require_jwt and /refresh compare it against users.token_epoch and reject on + mismatch, so incrementing that column revokes every outstanding token. + Always pass user.token_epoch — the 0 default exists only so old call sites + fail visibly in tests rather than silently minting unrevokable tokens. + """ now = datetime.now(timezone.utc).replace(tzinfo=None) secret = current_app.config['JWT_SECRET_KEY'] access_payload = { 'sub': str(user_id), 'type': 'access', 'jti': str(uuid.uuid4()), + 'epoch': int(token_epoch or 0), 'iat': now, 'exp': now + current_app.config['JWT_ACCESS_TOKEN_EXPIRES'], } @@ -99,6 +108,7 @@ def generate_tokens(user_id: int) -> dict: 'sub': str(user_id), 'type': 'refresh', 'jti': str(uuid.uuid4()), + 'epoch': int(token_epoch or 0), 'iat': now, 'exp': now + current_app.config['JWT_REFRESH_TOKEN_EXPIRES'], } @@ -172,8 +182,45 @@ def blacklist_token(token: str, token_type: str) -> None: pass # Never let blacklisting errors break the logout flow +def load_user_for_token(payload) -> 'object | None': + """ + Resolve the User a validated token refers to, or None if the token must be + rejected. + + Two checks beyond signature validity: + + 1. The user still exists. Routes immediately dereference the result of + db.session.get(User, ...); without this a valid token for a deleted + account produced an AttributeError on None and a 500. + 2. The token's epoch claim still matches users.token_epoch. A master-password + change increments that column, which revokes every token minted before it. + Tokens issued before the claim existed decode as 0 and match the column + default, so an upgrade does not sign existing sessions out. + """ + from app.models.user import User + from app import db + + try: + user_id = int(payload['sub']) + except (KeyError, TypeError, ValueError): + return None + + user = db.session.get(User, user_id) + if user is None: + return None + if int(payload.get('epoch', 0) or 0) != int(user.token_epoch or 0): + return None + return user + + def require_jwt(f): - """Decorator: validates Bearer token and sets g.current_user_id.""" + """ + Decorator: validates the Bearer token and sets g.current_user_id. + + Also sets g.current_user to the resolved User so handlers can reuse it + instead of issuing a second lookup (SQLAlchemy's identity map makes the + repeat cheap, but reusing it is clearer). + """ @wraps(f) def decorated(*args, **kwargs): auth_header = request.headers.get('Authorization', '') @@ -186,7 +233,14 @@ def require_jwt(f): return jsonify({'error': 'Token expired'}), 401 except jwt.PyJWTError: return jsonify({'error': 'Invalid token'}), 401 - g.current_user_id = int(payload['sub']) + + user = load_user_for_token(payload) + if user is None: + # Deleted account, or a token predating a credential change. + return jsonify({'error': 'Session is no longer valid. Please log in again.'}), 401 + + g.current_user = user + g.current_user_id = user.id return f(*args, **kwargs) return decorated diff --git a/migrations/versions/k1l2m3n4o5p6_add_token_epoch.py b/migrations/versions/k1l2m3n4o5p6_add_token_epoch.py new file mode 100644 index 0000000..ec2c133 --- /dev/null +++ b/migrations/versions/k1l2m3n4o5p6_add_token_epoch.py @@ -0,0 +1,50 @@ +"""add token_epoch to users + +Revision ID: k1l2m3n4o5p6 +Revises: j0k1l2m3n4o5 +Create Date: 2026-08-26 00:00:00.000000 + +Adds a monotonic session-generation counter so credential changes can revoke +every token issued before them. + +Previously, changing the master password left all outstanding access and refresh +tokens valid — a stolen refresh token kept working for its full 7-day lifetime +after the victim changed their password. The response said "Please log in again" +but nothing enforced it. + +Every JWT now carries an `epoch` claim. require_jwt (and /refresh) compare it +against users.token_epoch and reject on mismatch. change_password and /recover +increment the column, which invalidates every previously issued token at once. + +A counter rather than a timestamp: JWT `iat` has one-second granularity, so a +token minted in the same second as the password change could otherwise slip +through the comparison. + +Existing tokens predate the claim and decode with epoch 0, which matches the +server_default — so deploying this does not sign everyone out. The first +password change moves them to 1 and invalidates them as intended. +""" +from alembic import op +import sqlalchemy as sa + + +revision = 'k1l2m3n4o5p6' +down_revision = 'j0k1l2m3n4o5' +branch_labels = None +depends_on = None + + +def upgrade(): + op.add_column( + 'users', + sa.Column( + 'token_epoch', + sa.Integer, + nullable=False, + server_default='0', + ), + ) + + +def downgrade(): + op.drop_column('users', 'token_epoch') diff --git a/pytest.ini b/pytest.ini new file mode 100644 index 0000000..16788bb --- /dev/null +++ b/pytest.ini @@ -0,0 +1,7 @@ +[pytest] +testpaths = tests +python_files = test_*.py +# Fail on unraised warnings that indicate real problems, but keep the +# deliberately-bad signing key in test_forged_epoch_claim_is_rejected quiet. +filterwarnings = + ignore::UserWarning:jwt.api_jwt diff --git a/requirements-dev.txt b/requirements-dev.txt new file mode 100644 index 0000000..966a082 --- /dev/null +++ b/requirements-dev.txt @@ -0,0 +1,4 @@ +# Development / CI-only dependencies. +# Install alongside requirements.txt: pip install -r requirements.txt -r requirements-dev.txt +pytest>=8.0 +flake8>=7.0 diff --git a/scripts/passkeeper-nginx.conf b/scripts/passkeeper-nginx.conf index f00f1ee..496e80a 100644 --- a/scripts/passkeeper-nginx.conf +++ b/scripts/passkeeper-nginx.conf @@ -10,10 +10,25 @@ # - Content-Security-Policy (HTTP header level — authoritative over meta tag) # - Connection-level rate limiting zones for auth endpoints # - Buffer and timeout hardening +# +# Phase 6 (502 fixes): +# - proxy_connect_timeout / proxy_read_timeout / proxy_send_timeout made +# explicit. proxy_read_timeout MUST stay BELOW the `timeout` value in +# gunicorn.conf.py (90s): if Gunicorn kills the worker first the connection +# is severed mid-response and Nginx reports 502; if Nginx gives up first the +# client gets a clean 504 instead. +# - Security headers repeated inside the /static/ location block. Nginx drops +# ALL inherited add_header directives in any block that declares one of its +# own, so /static/'s Cache-Control header was silently stripping CSP, HSTS, +# X-Frame-Options and nosniff from every JS and CSS asset. # ── Rate limiting zones ──────────────────────────────────────────────────────── -# auth_limit: 10 req/s per IP for auth endpoints (login, register, MFA verify) -# api_limit: 60 req/s per IP for all other API endpoints +# NOTE the units: these are per MINUTE (r/m), not per second. api_limit is +# therefore 1 req/s sustained for the whole API, with burst=20 absorbing spikes. +# A vault page load fires several /api/* calls, so tightening these further will +# surface as 429s to normal users. +# auth_limit: 10 req/min per IP for auth endpoints (login, register, MFA verify) +# api_limit: 60 req/min per IP for all other API endpoints limit_req_zone $binary_remote_addr zone=auth_limit:10m rate=10r/m; limit_req_zone $binary_remote_addr zone=api_limit:10m rate=60r/m; @@ -27,14 +42,14 @@ server { include /etc/letsencrypt/options-ssl-nginx.conf; ssl_dhparam /etc/letsencrypt/ssl-dhparams.pem; - # Modern TLS only - ssl_protocols TLSv1.2 TLSv1.3; - ssl_prefer_server_ciphers on; + # options-ssl-nginx.conf (managed by Certbot) already sets ssl_protocols, + # ssl_prefer_server_ciphers, ssl_session_timeout and ssl_session_tickets. + # Repeating them here conflicts whenever Certbot updates its managed file. ssl_session_cache shared:SSL:10m; - ssl_session_timeout 1d; - ssl_session_tickets off; # ── Security headers ────────────────────────────────────────────────────── + # IMPORTANT: these are repeated verbatim inside the /static/ block below. + # Keep the two copies in sync whenever either is modified. # HSTS: enforce HTTPS for 1 year; include subdomains; allow preload submission add_header Strict-Transport-Security "max-age=31536000; includeSubDomains; preload" always; @@ -68,11 +83,31 @@ server { keepalive_timeout 15s; send_timeout 10s; + # ── Proxy timeouts ──────────────────────────────────────────────────────── + # proxy_read_timeout must stay BELOW gunicorn.conf.py `timeout` (90s) so + # Nginx is the side that gives up first and the client sees 504, not 502. + # proxy_connect_timeout is short — Gunicorn is on loopback. + proxy_connect_timeout 5s; + proxy_read_timeout 60s; + proxy_send_timeout 10s; + # ── Static files ────────────────────────────────────────────────────────── + # Every add_header from the server block MUST be repeated here. Nginx drops + # all inherited add_header directives in any location that declares one of + # its own, so without this the Cache-Control below silently strips CSP, + # HSTS, X-Frame-Options and nosniff from every static asset. location /static/ { alias /home/spuser/PassKeeper/app/static/; expires 30d; - add_header Cache-Control "public, immutable"; + add_header Cache-Control "public, immutable" always; + add_header Strict-Transport-Security "max-age=31536000; includeSubDomains; preload" always; + add_header X-Frame-Options "DENY" always; + add_header X-Content-Type-Options "nosniff" always; + add_header Referrer-Policy "strict-origin-when-cross-origin" always; + add_header Permissions-Policy "geolocation=(), camera=(), microphone=()" always; + add_header Content-Security-Policy + "default-src 'self'; script-src 'self'; style-src 'self'; img-src 'self' data:; font-src 'self'; connect-src 'self' https://api.pwnedpasswords.com; frame-ancestors 'none';" + always; } # ── Auth endpoints — stricter Nginx-level rate limit ────────────────────── diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..b157cf7 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,75 @@ +""" +Shared pytest fixtures. + +Runs the real app factory against in-memory SQLite. The server treats all +client-side crypto as opaque strings (auth_hash, enc_data, iv, enc_name), so +these tests can pass arbitrary values for them — no Web Crypto needed. The one +place real crypto matters is the recovery proof, which is plain HMAC-SHA256 and +is computed here exactly as recover.js does. +""" +import os +import sys + +import pytest + +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + +from app import create_app, db as _db # noqa: E402 + + +@pytest.fixture +def app(): + application = create_app('testing') + with application.app_context(): + _db.create_all() + yield application + _db.session.remove() + _db.drop_all() + + +@pytest.fixture +def client(app): + return app.test_client() + + +@pytest.fixture +def db(app): + return _db + + +# ── Helpers ────────────────────────────────────────────────────────────────── + +def register(client, email='user@example.com', auth_hash='AUTH-HASH-V1', + enc_key_salt='SALT-V1'): + return client.post('/api/auth/register', json={ + 'email': email, 'auth_hash': auth_hash, 'enc_key_salt': enc_key_salt, + }) + + +def login(client, email='user@example.com', auth_hash='AUTH-HASH-V1'): + return client.post('/api/auth/login', json={'email': email, 'auth_hash': auth_hash}) + + +def auth_headers(token): + return {'Authorization': f'Bearer {token}'} + + +def make_user(client, email='user@example.com', auth_hash='AUTH-HASH-V1', + enc_key_salt='SALT-V1'): + """Register + log in. Returns (access_token, refresh_token).""" + assert register(client, email, auth_hash, enc_key_salt).status_code == 201 + res = login(client, email, auth_hash) + assert res.status_code == 200, res.get_json() + body = res.get_json() + return body['access_token'], body['refresh_token'] + + +def add_item(client, token, name='password', enc_data='CT', iv='IV'): + """Create a vault item. `name` is the server-side type label.""" + res = client.post('/api/vault', headers=auth_headers(token), json={ + 'name': name, 'item_type': 'password', + 'enc_data': enc_data, 'iv': iv, + 'enc_name': 'ENCNAME', 'iv_name': 'IVNAME', + }) + assert res.status_code == 201, res.get_json() + return res.get_json()['id'] diff --git a/tests/test_deploy_config.py b/tests/test_deploy_config.py new file mode 100644 index 0000000..64e5209 --- /dev/null +++ b/tests/test_deploy_config.py @@ -0,0 +1,109 @@ +""" +Guards on deployment configuration that only bites in production. + +These are the settings whose failure mode is an intermittent 502 rather than a +stack trace, so nothing else catches them drifting apart. +""" +import re +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parent.parent +NGINX = (ROOT / 'scripts' / 'passkeeper-nginx.conf').read_text(encoding='utf-8') +UNIT = (ROOT / 'scripts' / 'passkeeper.service').read_text(encoding='utf-8') + + +@pytest.fixture(scope='module') +def gunicorn_conf(): + ns = {} + exec(compile((ROOT / 'gunicorn.conf.py').read_text(encoding='utf-8'), + 'gunicorn.conf.py', 'exec'), ns) + return ns + + +def _nginx_seconds(directive): + m = re.search(rf'^\s*{directive}\s+(\d+)s;', NGINX, re.M) + assert m, f'{directive} not found in passkeeper-nginx.conf' + return int(m.group(1)) + + +def test_nginx_gives_up_before_gunicorn_kills_the_worker(gunicorn_conf): + """ + The 502 invariant. If Gunicorn's timeout fires first the connection is + severed mid-response and Nginx reports 502; if Nginx times out first the + client gets a clean 504 instead. + """ + assert _nginx_seconds('proxy_read_timeout') < gunicorn_conf['timeout'] + + +def test_gunicorn_holds_keepalive_longer_than_nginx(gunicorn_conf): + """ + Nginx must be the side that closes an idle upstream connection. If Gunicorn + closes one as Nginx is reusing it, that request fails as a 502. + """ + assert gunicorn_conf['keepalive'] > _nginx_seconds('keepalive_timeout') + + +def test_preload_app_is_disabled(gunicorn_conf): + """ + create_app() starts an APScheduler thread, and threads do not survive + fork(). Under preload_app the scheduler would exist only in the arbiter, + which serves no requests, so the cleanup job would silently never run. + """ + assert gunicorn_conf['preload_app'] is False + + +def test_static_location_repeats_every_security_header(): + """ + Nginx drops ALL inherited add_header directives in any location that + declares one of its own. /static/ sets Cache-Control, so without explicit + copies every JS and CSS asset ships with no CSP, HSTS or X-Frame-Options. + """ + static = re.search(r'location /static/ \{(.*?)\n \}', NGINX, re.S) + assert static, 'no /static/ location block found' + body = static.group(1) + + for header in ('Strict-Transport-Security', 'X-Frame-Options', + 'X-Content-Type-Options', 'Referrer-Policy', + 'Permissions-Policy', 'Content-Security-Policy'): + assert header in body, f'/static/ is missing {header}' + + +def test_hibp_origin_is_allowed_in_every_csp(): + """The security dashboard's breach check needs this origin in connect-src.""" + policies = re.findall(r'connect-src[^;"]*', NGINX) + assert policies, 'no connect-src directive found' + for p in policies: + assert 'https://api.pwnedpasswords.com' in p, p + + +def test_unit_has_no_watchdog(): + """ + WatchdogSec without Type=notify meant systemd never received a keepalive, + declared the service hung, and SIGKILLed it on a loop — a repeating window + of 502s. Re-enabling it requires Type=notify AND NotifyAccess=main. + """ + active = [ln for ln in UNIT.splitlines() + if ln.strip().startswith('WatchdogSec')] + if active: + assert 'Type=notify' in UNIT and 'NotifyAccess=main' in UNIT, ( + 'WatchdogSec requires Type=notify + NotifyAccess=main or systemd ' + 'will kill the service on a loop' + ) + + +def test_unit_reload_does_not_use_usr2(): + """ + USR2 forks a second master without retiring the first, leaving systemd's + $MAINPID tracking a stale process. + """ + reload_line = next((ln for ln in UNIT.splitlines() + if ln.strip().startswith('ExecReload=')), '') + assert 'USR2' not in reload_line, reload_line + + +def test_unit_loads_the_gunicorn_config_file(): + assert 'gunicorn.conf.py' in UNIT, ( + 'the unit no longer references gunicorn.conf.py, so its tuning is dead code' + ) diff --git a/tests/test_key_rotation.py b/tests/test_key_rotation.py new file mode 100644 index 0000000..f1b42ed --- /dev/null +++ b/tests/test_key_rotation.py @@ -0,0 +1,208 @@ +""" +Regression tests for silent vault destruction (review finding #2). + +change_password and /recover both rotate enc_key_salt, which invalidates every +ciphertext under the previous vault key. The client re-encrypts each item and +sends it back — but nothing checked that the payload actually covered every +item. A short payload rotated the key anyway and left the missing items +permanently undecryptable, with no error and a success entry in the audit log. +""" +import hashlib +import hmac + +from app import db +from app.models.audit_log import AuditLog +from app.models.user import User +from app.models.vault_item import VaultItem +from tests.conftest import add_item, auth_headers, login, make_user + +VERIFIER = 'c' * 64 + + +def _payload(ids): + return [{'id': i, 'enc_data': f'NEW-{i}', 'iv': f'NEWIV-{i}'} for i in ids] + + +def _change_password(client, token, items, allow_partial=None): + body = { + 'current_auth_hash': 'AUTH-HASH-V1', + 'new_auth_hash': 'AUTH-HASH-V2', + 'new_enc_key_salt': 'SALT-V2', + 'items': items, + } + if allow_partial is not None: + body['allow_partial'] = allow_partial + return client.post('/api/auth/change-password', headers=auth_headers(token), json=body) + + +# -- change_password --------------------------------------------------------- + +def test_full_payload_rotates_everything(client, app): + token, _ = make_user(client) + ids = [add_item(client, token) for _ in range(3)] + + res = _change_password(client, token, _payload(ids)) + assert res.status_code == 200, res.get_json() + + for i in ids: + assert db.session.get(VaultItem, i).enc_data == f'NEW-{i}' + user = User.query.filter_by(email='user@example.com').first() + assert user.enc_key_salt == 'SALT-V2' + + +def test_short_payload_is_refused_and_nothing_changes(client, app): + """The exact data-loss bug: one item omitted from the re-encryption.""" + token, _ = make_user(client) + ids = [add_item(client, token) for _ in range(3)] + + res = _change_password(client, token, _payload(ids[:2])) # third omitted + assert res.status_code == 409, res.get_json() + body = res.get_json() + assert body['code'] == 'incomplete_reencryption' + assert (body['expected'], body['received']) == (3, 2) + + # Nothing may have been committed: salt unchanged, ciphertext untouched. + user = User.query.filter_by(email='user@example.com').first() + assert user.enc_key_salt == 'SALT-V1', 'key rotated despite refusal' + assert user.token_epoch == 0 + for i in ids: + assert db.session.get(VaultItem, i).enc_data == 'CT' + + # The old password must still work. + assert login(client).status_code == 200 + + +def test_empty_payload_against_populated_vault_is_refused(client, app): + token, _ = make_user(client) + for _ in range(4): + add_item(client, token) + + res = _change_password(client, token, []) + assert res.status_code == 409 + assert res.get_json()['received'] == 0 + assert User.query.filter_by(email='user@example.com').first().enc_key_salt == 'SALT-V1' + + +def test_refusal_is_audited(client, app): + token, _ = make_user(client) + ids = [add_item(client, token) for _ in range(2)] + _change_password(client, token, _payload(ids[:1])) + + entry = (AuditLog.query + .filter_by(action='auth.change_password_failed') + .order_by(AuditLog.id.desc()).first()) + assert entry is not None + assert '1 of 2' in entry.detail + + +def test_another_users_item_does_not_count_toward_coverage(client, app): + """A foreign id must not pad the payload up to the expected count.""" + token_a, _ = make_user(client, 'a@example.com', 'HASH-A', 'SALT-A') + token_b, _ = make_user(client, 'b@example.com', 'HASH-B', 'SALT-B') + a_ids = [add_item(client, token_a) for _ in range(2)] + b_id = add_item(client, token_b) + + res = client.post('/api/auth/change-password', headers=auth_headers(token_a), json={ + 'current_auth_hash': 'HASH-A', 'new_auth_hash': 'HASH-A2', + 'new_enc_key_salt': 'SALT-A2', + 'items': _payload([a_ids[0], b_id]), # b_id does not belong to user A + }) + assert res.status_code == 409 + assert (res.get_json()['expected'], res.get_json()['received']) == (2, 1) + assert db.session.get(VaultItem, b_id).enc_data == 'CT' + + +# -- /recover ---------------------------------------------------------------- + +def _setup_recovery(client, token): + assert client.post('/api/auth/recovery/setup', headers=auth_headers(token), json={ + 'recovery_enc_salt': 'BLOB', 'recovery_iv': 'IV', + 'recovery_verifier': VERIFIER, + }).status_code == 200 + + +def _proof(client, email='user@example.com'): + nonce = client.get(f'/api/auth/recovery/data?email={email}').get_json()['nonce'] + return hmac.new(VERIFIER.encode(), nonce.encode(), hashlib.sha256).hexdigest() + + +def _consume_challenge(client, proof): + """Fetch items exactly as recover.js does, which rotates the challenge.""" + return client.get('/api/auth/recovery/items?email=user@example.com', + headers={'X-Recovery-Proof': proof}) + + +def test_recover_refuses_short_payload(client, app): + token, _ = make_user(client) + ids = [add_item(client, token) for _ in range(3)] + _setup_recovery(client, token) + proof = _proof(client) + _consume_challenge(client, proof) + + res = client.post('/api/auth/recover', json={ + 'email': 'user@example.com', 'new_auth_hash': 'AUTH-HASH-V2', + 'new_enc_key_salt': 'SALT-V2', 'recovery_proof': proof, + 'items': _payload(ids[:2]), + }) + assert res.status_code == 409, res.get_json() + user = User.query.filter_by(email='user@example.com').first() + assert user.enc_key_salt == 'SALT-V1' + assert user.recovery_enc_salt == 'BLOB', 'recovery code consumed despite refusal' + + +def test_recover_allows_partial_when_explicitly_confirmed(client, app): + """ + The escape hatch exists because refusing outright would leave a locked-out + user with no way into their account at all. + """ + token, _ = make_user(client) + ids = [add_item(client, token) for _ in range(3)] + _setup_recovery(client, token) + proof = _proof(client) + _consume_challenge(client, proof) + + res = client.post('/api/auth/recover', json={ + 'email': 'user@example.com', 'new_auth_hash': 'AUTH-HASH-V2', + 'new_enc_key_salt': 'SALT-V2', 'recovery_proof': proof, + 'items': _payload(ids[:2]), 'allow_partial': True, + }) + assert res.status_code == 200, res.get_json() + + entry = (AuditLog.query.filter_by(action='auth.recovery_success') + .order_by(AuditLog.id.desc()).first()) + assert 'PARTIAL' in entry.detail, 'partial recovery not flagged in the audit log' + + +def test_recover_full_payload_succeeds_and_consumes_the_code(client, app): + token, _ = make_user(client) + ids = [add_item(client, token) for _ in range(2)] + _setup_recovery(client, token) + proof = _proof(client) + _consume_challenge(client, proof) + + res = client.post('/api/auth/recover', json={ + 'email': 'user@example.com', 'new_auth_hash': 'AUTH-HASH-V2', + 'new_enc_key_salt': 'SALT-V2', 'recovery_proof': proof, + 'items': _payload(ids), + }) + assert res.status_code == 200, res.get_json() + + user = User.query.filter_by(email='user@example.com').first() + assert user.enc_key_salt == 'SALT-V2' + assert user.recovery_enc_salt is None + assert user.recovery_verifier is None + assert login(client, auth_hash='AUTH-HASH-V2').status_code == 200 + + +def test_change_password_ignores_allow_partial(client, app): + """ + Recovery has a partial-completion escape hatch; changing the password must + not. The current password keeps working, so there is never a reason to + accept permanent data loss here — the server refuses even if a client asks. + """ + token, _ = make_user(client) + ids = [add_item(client, token) for _ in range(3)] + + res = _change_password(client, token, _payload(ids[:1]), allow_partial=True) + assert res.status_code == 409, 'server honoured allow_partial on change-password' + assert User.query.filter_by(email='user@example.com').first().enc_key_salt == 'SALT-V1' diff --git a/tests/test_mfa_gate.py b/tests/test_mfa_gate.py new file mode 100644 index 0000000..99ad60c --- /dev/null +++ b/tests/test_mfa_gate.py @@ -0,0 +1,143 @@ +""" +Regression tests for the MFA bypass (review finding #1). + +The bug had two halves: + a) /login returned enc_key_salt in the MFA-pending response, before the second + factor was verified. + b) The recovery challenge was keyed on enc_key_salt, so anyone holding it + could forge a proof and pull the whole encrypted vault from the + unauthenticated /recovery/items — bypassing MFA entirely. + +Chained, an attacker with only the master password could exfiltrate or take over +the account. These tests pin both halves shut. +""" +import hashlib +import hmac +import json + +import pyotp + +from app import db +from app.models.user import User +from app.services.auth_service import encrypt_totp_secret +from tests.conftest import add_item, auth_headers, login, make_user + + +def _enable_mfa(email='user@example.com'): + """Turn on TOTP directly in the DB and return the plaintext secret.""" + user = User.query.filter_by(email=email).first() + secret = pyotp.random_base32() + enc, iv = encrypt_totp_secret(secret) + user.totp_secret, user.totp_iv, user.totp_enabled = enc, iv, True + db.session.commit() + return secret + + +def test_login_withholds_enc_key_salt_until_mfa_is_verified(client, app): + make_user(client) + secret = _enable_mfa() + + res = login(client) + body = res.get_json() + + assert res.status_code == 200 + assert body['mfa_required'] is True + # The heart of finding #1a. + assert 'enc_key_salt' not in body, ( + 'enc_key_salt leaked before the second factor was verified' + ) + + res = client.post('/api/auth/mfa/verify', json={ + 'mfa_token': body['mfa_token'], 'totp_code': pyotp.TOTP(secret).now(), + }) + verified = res.get_json() + assert res.status_code == 200 + # Released only now, once both factors are proven. + assert verified['enc_key_salt'] == 'SALT-V1' + + +def test_login_without_mfa_still_returns_enc_key_salt(client): + """The withholding must apply only to the MFA-pending path.""" + make_user(client) + body = login(client).get_json() + assert 'mfa_required' not in body + assert body['enc_key_salt'] == 'SALT-V1' + + +# ── Finding #1b: the recovery proof must not be forgeable from enc_key_salt ── + +def _setup_recovery(client, token, verifier): + res = client.post('/api/auth/recovery/setup', headers=auth_headers(token), json={ + 'recovery_enc_salt': 'RECOVERY-BLOB', 'recovery_iv': 'RECOVERY-IV', + 'recovery_verifier': verifier, + }) + assert res.status_code == 200, res.get_json() + + +def test_recovery_proof_cannot_be_forged_from_enc_key_salt(client, app): + """ + The attack: password known, second factor not. Previously the attacker could + key the HMAC with enc_key_salt and walk away with every encrypted item. + """ + token, _ = make_user(client) + add_item(client, token) + verifier = 'a' * 64 + _setup_recovery(client, token, verifier) + + nonce = client.get('/api/auth/recovery/data?email=user@example.com').get_json()['nonce'] + + forged = hmac.new(b'SALT-V1', nonce.encode(), hashlib.sha256).hexdigest() + res = client.get('/api/auth/recovery/items?email=user@example.com', + headers={'X-Recovery-Proof': forged}) + assert res.status_code == 401, 'enc_key_salt still forges a valid recovery proof' + + +def test_recovery_proof_from_verifier_is_accepted(client, app): + """The legitimate holder of the recovery code must still get through.""" + token, _ = make_user(client) + add_item(client, token) + verifier = 'b' * 64 + _setup_recovery(client, token, verifier) + + data = client.get('/api/auth/recovery/data?email=user@example.com').get_json() + assert data['proof_scheme'] == 'verifier' + + proof = hmac.new(verifier.encode(), data['nonce'].encode(), hashlib.sha256).hexdigest() + res = client.get('/api/auth/recovery/items?email=user@example.com', + headers={'X-Recovery-Proof': proof}) + assert res.status_code == 200, res.get_json() + assert len(res.get_json()['items']) == 1 + + +def test_legacy_account_falls_back_to_enc_key_salt_proof(client, app): + """ + Recovery codes created before recovery_verifier must keep working, and be + reported as legacy so the UI can prompt a regeneration. + """ + token, _ = make_user(client) + add_item(client, token) + user = User.query.filter_by(email='user@example.com').first() + user.recovery_enc_salt, user.recovery_iv = 'BLOB', 'IV' + user.recovery_verifier = None # pre-migration state + db.session.commit() + + status = client.get('/api/auth/recovery/status', headers=auth_headers(token)).get_json() + assert status['recovery_configured'] is True + assert status['recovery_is_legacy'] is True + + data = client.get('/api/auth/recovery/data?email=user@example.com').get_json() + assert data['proof_scheme'] == 'legacy' + + proof = hmac.new(b'SALT-V1', data['nonce'].encode(), hashlib.sha256).hexdigest() + res = client.get('/api/auth/recovery/items?email=user@example.com', + headers={'X-Recovery-Proof': proof}) + assert res.status_code == 200 + + +def test_recovery_setup_rejects_malformed_verifier(client): + token, _ = make_user(client) + for bad in ('', 'short', 'g' * 64, 'A' * 63): + res = client.post('/api/auth/recovery/setup', headers=auth_headers(token), json={ + 'recovery_enc_salt': 'B', 'recovery_iv': 'IV', 'recovery_verifier': bad, + }) + assert res.status_code == 400, f'accepted malformed verifier {bad!r}' diff --git a/tests/test_session_revocation.py b/tests/test_session_revocation.py new file mode 100644 index 0000000..941749d --- /dev/null +++ b/tests/test_session_revocation.py @@ -0,0 +1,148 @@ +""" +Regression tests for session revocation (review finding #3). + +Changing the master password used to leave every outstanding access and refresh +token valid. The response said "Please log in again" but nothing enforced it, so +a stolen refresh token kept working for its full 7-day lifetime after the victim +changed the password it was obtained under. + +Every JWT now carries an `epoch` claim checked against users.token_epoch. + +Also covers the sub-finding that require_jwt never confirmed the user still +existed: a valid token for a deleted account dereferenced None and returned 500. +""" +import jwt as pyjwt + +from app import db +from app.models.user import User +from tests.conftest import add_item, auth_headers, login, make_user + + +def _rotate_password(client, token, ids): + """Complete a full, valid password change.""" + res = client.post('/api/auth/change-password', headers=auth_headers(token), json={ + 'current_auth_hash': 'AUTH-HASH-V1', + 'new_auth_hash': 'AUTH-HASH-V2', + 'new_enc_key_salt': 'SALT-V2', + 'items': [{'id': i, 'enc_data': f'NEW-{i}', 'iv': f'IV-{i}'} for i in ids], + }) + assert res.status_code == 200, res.get_json() + return res + + +def test_access_token_is_revoked_by_password_change(client, app): + token, _ = make_user(client) + ids = [add_item(client, token)] + + assert client.get('/api/vault', headers=auth_headers(token)).status_code == 200 + _rotate_password(client, token, ids) + + res = client.get('/api/vault', headers=auth_headers(token)) + assert res.status_code == 401, 'access token survived the password change' + + +def test_refresh_token_is_revoked_by_password_change(client, app): + """ + The more damaging half: a refresh token is valid for 7 days and can mint + fresh access tokens indefinitely. + """ + token, refresh = make_user(client) + ids = [add_item(client, token)] + _rotate_password(client, token, ids) + + res = client.post('/api/auth/refresh', json={'refresh_token': refresh}) + assert res.status_code == 401, 'refresh token survived the password change' + + +def test_epoch_increments_and_new_login_works(client, app): + token, _ = make_user(client) + ids = [add_item(client, token)] + _rotate_password(client, token, ids) + + user = User.query.filter_by(email='user@example.com').first() + assert user.token_epoch == 1 + + res = login(client, auth_hash='AUTH-HASH-V2') + assert res.status_code == 200 + new_token = res.get_json()['access_token'] + assert client.get('/api/vault', headers=auth_headers(new_token)).status_code == 200 + + +def test_recovery_also_revokes_prior_sessions(client, app): + """An attacker holding a token must not survive the victim recovering.""" + import hashlib + import hmac + + verifier = 'd' * 64 + token, _ = make_user(client) + ids = [add_item(client, token)] + assert client.post('/api/auth/recovery/setup', headers=auth_headers(token), json={ + 'recovery_enc_salt': 'BLOB', 'recovery_iv': 'IV', + 'recovery_verifier': verifier, + }).status_code == 200 + + nonce = client.get('/api/auth/recovery/data?email=user@example.com').get_json()['nonce'] + proof = hmac.new(verifier.encode(), nonce.encode(), hashlib.sha256).hexdigest() + client.get('/api/auth/recovery/items?email=user@example.com', + headers={'X-Recovery-Proof': proof}) + + res = client.post('/api/auth/recover', json={ + 'email': 'user@example.com', 'new_auth_hash': 'AUTH-HASH-V2', + 'new_enc_key_salt': 'SALT-V2', 'recovery_proof': proof, + 'items': [{'id': i, 'enc_data': f'NEW-{i}', 'iv': f'IV-{i}'} for i in ids], + }) + assert res.status_code == 200, res.get_json() + + assert client.get('/api/vault', headers=auth_headers(token)).status_code == 401 + # The tokens handed back by /recover must carry the NEW epoch and work. + fresh = res.get_json()['access_token'] + assert client.get('/api/vault', headers=auth_headers(fresh)).status_code == 200 + + +def test_token_for_deleted_account_is_401_not_500(client, app): + """Previously this dereferenced None inside the handler and returned 500.""" + token, _ = make_user(client) + user = User.query.filter_by(email='user@example.com').first() + db.session.delete(user) + db.session.commit() + + for path in ('/api/vault', '/api/auth/me', '/api/sharing/keys', '/api/emergency'): + res = client.get(path, headers=auth_headers(token)) + assert res.status_code == 401, f'{path} returned {res.status_code}' + + +def test_forged_epoch_claim_is_rejected(client, app): + """ + The epoch is inside the signed payload, so tampering invalidates the + signature. Re-signing with the wrong key must also fail. + """ + token, _ = make_user(client) + payload = pyjwt.decode(token, options={'verify_signature': False}) + payload['epoch'] = 99 + forged = pyjwt.encode(payload, 'not-the-real-signing-key', algorithm='HS256') + + assert client.get('/api/vault', headers=auth_headers(forged)).status_code == 401 + + +def test_tokens_predating_the_epoch_claim_still_work(client, app): + """ + Deploying this must not sign existing sessions out: tokens minted before the + claim existed decode with epoch 0, matching the column default. + """ + token, _ = make_user(client) + payload = pyjwt.decode(token, options={'verify_signature': False}) + del payload['epoch'] # simulate a pre-upgrade token + legacy = pyjwt.encode(payload, app.config['JWT_SECRET_KEY'], algorithm='HS256') + + assert client.get('/api/vault', headers=auth_headers(legacy)).status_code == 200 + + +def test_logout_still_revokes_via_blacklist(client, app): + """Epoch checking must not have displaced the existing jti blacklist.""" + token, refresh = make_user(client) + assert client.post('/api/auth/logout', headers=auth_headers(token), + json={'refresh_token': refresh}).status_code == 200 + + assert client.get('/api/vault', headers=auth_headers(token)).status_code == 401 + assert client.post('/api/auth/refresh', + json={'refresh_token': refresh}).status_code == 401 diff --git a/tests/test_webauthn_uv.py b/tests/test_webauthn_uv.py new file mode 100644 index 0000000..2c99410 --- /dev/null +++ b/tests/test_webauthn_uv.py @@ -0,0 +1,83 @@ +""" +Regression tests for passkey user verification (review finding #4). + +Both ceremonies used UserVerificationRequirement.PREFERRED with +require_user_verification=False, so an authenticator was free to skip the +biometric/PIN check. Since a passkey assertion here replaces BOTH the password +and the TOTP second factor, that reduced a full login to possession of an +unlocked device — enough to enumerate and delete vault items. + +A full ceremony needs a real authenticator, so these tests pin the negotiated +options (what the server asks the browser for) and the rejection path. The +enforcement half — require_user_verification=True passed to py-webauthn — is +asserted directly against the source. +""" +import inspect +import re + +from tests.conftest import auth_headers, make_user + +import app.routes.webauthn as webauthn_routes + + +def test_registration_options_require_user_verification(client, app): + token, _ = make_user(client) + res = client.post('/api/webauthn/register/begin', + headers=auth_headers(token), json={}) + assert res.status_code == 200, res.get_json() + body = res.get_json() + assert body['authenticatorSelection']['userVerification'] == 'required' + + +def test_authentication_options_require_user_verification(client, app): + make_user(client) + res = client.post('/api/webauthn/authenticate/begin', + json={'email': 'user@example.com'}) + assert res.status_code == 200, res.get_json() + assert res.get_json()['userVerification'] == 'required' + + +def test_verification_calls_enforce_user_verification(): + """ + Negotiating 'required' is only a request to the browser. The server must + also refuse an assertion that comes back without the UV flag set, or the + hint is decorative. + """ + src = inspect.getsource(webauthn_routes) + calls = re.findall(r'require_user_verification=(\w+)', src) + assert calls, 'no require_user_verification argument found' + assert all(v == 'True' for v in calls), ( + f'require_user_verification must be True everywhere, found: {calls}' + ) + + +def test_unknown_credential_is_rejected(client, app): + make_user(client) + client.post('/api/webauthn/authenticate/begin', json={'email': 'user@example.com'}) + res = client.post('/api/webauthn/authenticate/complete', + json={'id': 'bm9wZQ', 'rawId': 'bm9wZQ'}) + assert res.status_code == 401 + assert 'not recognised' in res.get_json()['error'] + + +def test_registration_failure_does_not_leak_exception_text(client, app): + """ + CLAUDE.md forbids returning str(e) to clients; this handler used to embed + the raw py-webauthn message, which quotes attestation internals. + """ + token, _ = make_user(client) + client.post('/api/webauthn/register/begin', headers=auth_headers(token), json={}) + + res = client.post('/api/webauthn/register/complete', + headers=auth_headers(token), json={'id': 'garbage'}) + assert res.status_code == 400 + error = res.get_json()['error'] + assert error == 'Could not verify this passkey. Please try again.', error + + +def test_register_complete_requires_a_pending_challenge(client, app): + token, _ = make_user(client) + res = client.post('/api/webauthn/register/complete', + headers=auth_headers(token), json={'id': 'x'}) + assert res.status_code == 400 + assert 'No pending registration challenge' in res.get_json()['error']