diff --git a/CLAUDE.md b/CLAUDE.md index 33eaf93..6090a3b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -118,6 +118,8 @@ passkeeper/ │ ├── test_session_revocation.py # token_epoch revocation; deleted-account 401 │ ├── test_webauthn_uv.py # user verification required on both ceremonies │ ├── test_sharing_expiry.py # expires_days fails closed +│ ├── test_registration_privacy.py # register does not disclose account existence +│ ├── test_emergency_visibility.py # grantor sees requests + retrievals │ ├── test_deploy_config.py # nginx/gunicorn/systemd/extension packaging guards │ └── js/test_psl.js # PSL same-site matching (node, run in CI) ├── gunicorn.conf.py # worker class, timeouts, preload_app=False @@ -244,6 +246,7 @@ CREATE TABLE webauthn_credentials ( | `i9j0k1l2m3n4` | Add expires_at to shared_items | | `j0k1l2m3n4o5` | Add recovery_verifier (decouple recovery proof) | | `k1l2m3n4o5p6` | Add token_epoch (revoke sessions on pw change) | +| `l2m3n4o5p6q7` | Add emergency vault retrieval tracking | --- @@ -301,6 +304,20 @@ CREATE TABLE webauthn_credentials ( 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 +- **Registration privacy:** `POST /api/auth/register` returns an identical 202 + whether or not the address exists, and performs an equivalent Argon2id hash on + both branches so timing does not reinstate the oracle. Duplicate attempts are + audited under `auth.register_duplicate`. Fully closing this needs email + verification so the address owner is told — until then the oracle is removed + but the owner cannot be notified. +- **Emergency access visibility:** `accept`, `request` and `vault_retrieved` are + audited under BOTH parties' user_ids. `/api/auth/audit-log` filters by + `user_id`, so an entry written only under the acting user is invisible to the + other — which meant a grantee could request and retrieve a vault snapshot + without anything reaching the grantor. `vault_retrieved_at` / + `vault_retrieval_count` on `emergency_access` record every fetch. Retrieval is + intentionally NOT blocked after the first time (the grantor may be unable to + re-provision); the wait period is the gate, and the grantor can revoke. - **Audit logs:** never contain plaintext item names, shared item names, or vault data --- @@ -642,6 +659,8 @@ Audit log details **never** contain plaintext item names, shared item names, or - `password_changed_at` lives inside `plain` (encrypted) — never in the server schema - 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 +- Audit entries are only visible to the user_id they are written under — mirror cross-party events (use `_log_for_both` in `emergency.py`) or the other party never sees them +- `/register` must return the SAME body and status for new and existing addresses, and hash on both paths — returning early on duplicate reinstates a timing oracle - 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 - `extension/shared/psl.js` is GENERATED — never hand-edit; run `python scripts/update_psl.py`. It must load BEFORE content.js / popup.js / background.js in every manifest - Never reintroduce `endsWith("." + host)` host matching anywhere in the extension — `tests/test_deploy_config.py` fails the build if it reappears @@ -661,6 +680,7 @@ Audit log details **never** contain plaintext item names, shared item names, or | Module | Action | Trigger | | -------------- | ------------------------------------------------------ | ------------------------------ | | `auth.py` | `auth.register` | New account | +| `auth.py` | `auth.register_duplicate` | Register attempt on an existing address (no email in detail) | | `auth.py` | `auth.login` / `auth.login_failed` | Login success/fail | | `auth.py` | `auth.account_locked` | Failed login lockout | | `auth.py` | `auth.mfa_enable/disable/verify` | TOTP actions | @@ -676,6 +696,7 @@ Audit log details **never** contain plaintext item names, shared item names, or | `sharing.py` | `sharing_keys.create/update` | ECDH key setup | | `sharing.py` | `shared_item.create/delete/accept` | Sharing (detail: type + id) | | `emergency.py` | `emergency_access.*` | All EA state transitions | +| `emergency.py` | `emergency_access.accept/request/vault_retrieved` | Logged under BOTH grantor and grantee user_ids | | `webauthn.py` | `webauthn.register` | Passkey registered | | `webauthn.py` | `webauthn.auth_success` / `webauthn.auth_failed` | Passkey login attempt | | `webauthn.py` | `webauthn.rename` / `webauthn.delete` | Credential management | @@ -857,7 +878,9 @@ Features planned for future implementation. Ordered by priority within each cate - Autofill matching moved onto the Public Suffix List (registrable domains) - Share `expires_days` fails closed instead of silently meaning "never" - nginx `api_limit` corrected from 60r/m to 10r/s -- pytest suite (51 tests) + PSL node test + CI jobs; `gunicorn.conf.py`; +- Registration no longer discloses account existence (status, body and timing) +- Emergency access: requests and vault retrievals are now visible to the grantor +- pytest suite (66 tests) + PSL node test + CI jobs; `gunicorn.conf.py`; systemd watchdog removed ### High priority — user-facing diff --git a/app/models/emergency_access.py b/app/models/emergency_access.py index 146cf87..105c6a7 100644 --- a/app/models/emergency_access.py +++ b/app/models/emergency_access.py @@ -16,6 +16,10 @@ class EmergencyAccess(db.Model): pending → grantor calls /deny → ready (reset, grantee can request again) pending (wait_days elapsed) → grantable (grantee fetches vault) + Retrieval does not change `status`: the grant stays 'pending' so the grantor + keeps seeing it as active and can revoke it. What retrieval does change is + vault_retrieved_at / vault_retrieval_count, which the grantor's UI surfaces. + Zero-knowledge: enc_vault is a JSON array of vault items re-encrypted by the grantor using the ECDH shared secret (grantor private key + grantee public key). """ @@ -40,6 +44,12 @@ class EmergencyAccess(db.Model): # JSON string: [{ id, name, item_type, enc_data, iv }, ...] enc_vault = db.Column(db.Text, nullable=True) created_at = db.Column(db.DateTime, default=lambda: datetime.now(timezone.utc).replace(tzinfo=None), nullable=False) + # Retrieval tracking — makes grantee access to the snapshot visible to the + # grantor. Retrieval is not blocked after the first time (the grantor may be + # unable to re-provision, which is the entire premise of emergency access); + # the wait period is the gate, and these make use of it auditable. + vault_retrieved_at = db.Column(db.DateTime, nullable=True) + vault_retrieval_count = db.Column(db.Integer, default=0, nullable=False, server_default='0') @property def wait_elapsed(self): @@ -62,6 +72,10 @@ class EmergencyAccess(db.Model): self.request_initiated_at.isoformat() if self.request_initiated_at else None ), 'created_at': self.created_at.isoformat() if self.created_at else None, + 'vault_retrieved_at': ( + self.vault_retrieved_at.isoformat() if self.vault_retrieved_at else None + ), + 'vault_retrieval_count': self.vault_retrieval_count or 0, # True when enc_vault contains items in the old format (has a plaintext # 'name' field instead of enc_name/iv_name). Grantor should re-provision. 'enc_vault_is_legacy': self._enc_vault_is_legacy(), diff --git a/app/routes/auth.py b/app/routes/auth.py index 0545847..7b6079a 100644 --- a/app/routes/auth.py +++ b/app/routes/auth.py @@ -131,7 +131,7 @@ def _apply_reencrypted_items(user_id: int, items, allow_partial: bool = False) - @auth_bp.route('/register', methods=['POST']) -@limiter.limit('10 per minute') +@limiter.limit('5 per minute') def register(): data = request.get_json(silent=True) or {} email = (data.get('email') or '').strip().lower() @@ -147,8 +147,46 @@ def register(): if not enc_key_salt: return jsonify({'error': 'enc_key_salt is required'}), 400 + # ── Account-existence must not be observable ──────────────────────────── + # + # This used to answer 409 "Email already registered", which let anyone probe + # whether a given address has a PassKeeper account — a useful target list for + # phishing, and exactly the kind of thing a password manager should not leak. + # + # Both branches now return the identical 202 body. The wording sends the user + # to the sign-in page either way, which is the correct next step in both + # cases: registering an address that already exists is harmless because the + # user simply signs in with the password they already have. + # + # Timing has to match too. Creating an account runs Argon2id (deliberately + # slow); returning early without it would make "exists" measurably faster and + # reinstate the oracle through the side door. So the existing-account branch + # performs and discards an equivalent hash. + # + # NOTE: fully closing this needs email verification (roadmap item 4) so the + # address owner is told when someone tries to register it. Until then this + # removes the oracle but cannot notify the legitimate owner. + generic_response = jsonify({ + 'message': ( + 'If that email address was available, your account has been created. ' + 'Please sign in.' + ) + }), 202 + + time.sleep(0.1) # flatten timing across both branches + if User.query.filter_by(email=email).first(): - return jsonify({'error': 'Email already registered'}), 409 + hash_auth_token(auth_hash) # equalise work; result intentionally discarded + AuditLog.log( + user_id=0, # no account to attribute this to + action='auth.register_duplicate', + resource_type='user', + resource_id=None, + detail='Registration attempted for an address that already exists', + ip_address=client_ip(), + ) + db.session.commit() + return generic_response master_hash = hash_auth_token(auth_hash) user = User(email=email, master_hash=master_hash, enc_key_salt=enc_key_salt) @@ -165,7 +203,7 @@ def register(): ) db.session.commit() - return jsonify({'message': 'Account created successfully'}), 201 + return generic_response @auth_bp.route('/login', methods=['POST']) diff --git a/app/routes/emergency.py b/app/routes/emergency.py index d7de4b7..a294bb4 100644 --- a/app/routes/emergency.py +++ b/app/routes/emergency.py @@ -41,6 +41,39 @@ def list_emergency(): }), 200 +def _log_for_both(ea: EmergencyAccess, action: str, grantor_detail: str, + grantee_detail: str) -> None: + """ + Write the audit entry twice — once under each party's user_id. + + /api/auth/audit-log filters by user_id, so an entry written only under the + acting user is invisible to the other party. That meant a grantee could + request access and retrieve the vault snapshot without a single line of it + appearing in the grantor's own audit log or security dashboard — the person + whose vault it was had no way to see it had happened. + + Until email notifications exist (roadmap item 4), the grantor's audit log is + the only channel that reaches them, so it must carry these events. + """ + AuditLog.log( + user_id=ea.grantor_id, + action=action, + resource_type='emergency_access', + resource_id=ea.id, + detail=grantor_detail, + ip_address=client_ip(), + ) + if ea.grantee_id and ea.grantee_id != ea.grantor_id: + AuditLog.log( + user_id=ea.grantee_id, + action=action, + resource_type='emergency_access', + resource_id=ea.id, + detail=grantee_detail, + ip_address=client_ip(), + ) + + def _ea_as_grantee(ea: EmergencyAccess, grantor: 'User | None' = None) -> dict: if grantor is None: grantor = db.session.get(User, ea.grantor_id) @@ -150,14 +183,13 @@ def accept_emergency(ea_id): ea.status = 'accepted' ea.grantee_id = user.id + db.session.flush() - AuditLog.log( - user_id=g.current_user_id, - action='emergency_access.accept', - resource_type='emergency_access', - resource_id=ea.id, - detail=f'Accepted emergency access invitation from grantor_id={ea.grantor_id}', - ip_address=client_ip(), + _log_for_both( + ea, + 'emergency_access.accept', + grantor_detail=f'{ea.grantee_email} accepted your emergency access invitation', + grantee_detail=f'Accepted emergency access invitation from grantor_id={ea.grantor_id}', ) db.session.commit() @@ -221,14 +253,21 @@ def request_access(ea_id): ea.status = 'pending' ea.request_initiated_at = datetime.now(timezone.utc).replace(tzinfo=None) + db.session.flush() - AuditLog.log( - user_id=g.current_user_id, - action='emergency_access.request', - resource_type='emergency_access', - resource_id=ea.id, - detail=f'Requested emergency vault access from grantor_id={ea.grantor_id} (wait: {ea.wait_days}d)', - ip_address=client_ip(), + # The grantor has `wait_days` to notice and deny this. If it only appeared in + # the grantee's audit log they would never see it in time. + _log_for_both( + ea, + 'emergency_access.request', + grantor_detail=( + f'ACTION REQUIRED: {ea.grantee_email} requested emergency access to ' + f'your vault. It unlocks in {ea.wait_days} day(s) unless you deny it.' + ), + grantee_detail=( + f'Requested emergency vault access from grantor_id={ea.grantor_id} ' + f'(wait: {ea.wait_days}d)' + ), ) db.session.commit() @@ -291,13 +330,25 @@ def get_emergency_vault(ea_id): 'error': f'Wait period not yet elapsed ({days_left:.1f} day(s) remaining)' }), 403 - AuditLog.log( - user_id=g.current_user_id, - action='emergency_access.vault_retrieved', - resource_type='emergency_access', - resource_id=ea.id, - detail=f'Retrieved emergency vault from grantor_id={ea.grantor_id}', - ip_address=client_ip(), + # Record the retrieval. Access is deliberately not revoked afterwards — the + # grantor may be unable to re-provision, and a failed import must not strand + # the grantee — but every retrieval is counted and shown to the grantor, who + # can revoke the grant outright. + now = datetime.now(timezone.utc).replace(tzinfo=None) + is_first = ea.vault_retrieved_at is None + if is_first: + ea.vault_retrieved_at = now + ea.vault_retrieval_count = (ea.vault_retrieval_count or 0) + 1 + + _log_for_both( + ea, + 'emergency_access.vault_retrieved', + grantor_detail=( + f'{ea.grantee_email} retrieved your emergency vault snapshot ' + f'({"first" if is_first else f"retrieval #{ea.vault_retrieval_count}"}). ' + 'Remove the grant if this was not expected.' + ), + grantee_detail=f'Retrieved emergency vault from grantor_id={ea.grantor_id}', ) db.session.commit() diff --git a/app/static/css/app.css b/app/static/css/app.css index 3b06349..4b7a710 100644 --- a/app/static/css/app.css +++ b/app/static/css/app.css @@ -953,6 +953,25 @@ ul { border: 1px solid #ffb74d; } +/* Emergency access: a pending request or a retrieved snapshot is the one thing + in this list the grantor must not scroll past, so it gets the strongest + treatment available rather than the amber used for ordinary warnings. */ +.badge-danger { + background: #ffebee; + color: #b71c1c; + border: 1px solid #ef9a9a; +} + +.share-item.em-alert { + border-left: 3px solid #c62828; + background: #fff5f5; +} + +.share-meta.em-retrieved { + color: #b71c1c; + font-weight: 500; +} + .badge-info { background: #e3f2fd; color: #1565c0; diff --git a/app/static/js/vault.js b/app/static/js/vault.js index 73feb1a..457311c 100644 --- a/app/static/js/vault.js +++ b/app/static/js/vault.js @@ -2445,6 +2445,16 @@ const Vault = (() => { } } + /** + * Whole days left before a pending emergency request unlocks. Mirrors the + * server's wait_elapsed calculation in EmergencyAccess.wait_elapsed. + */ + function _emDaysRemaining(g) { + if (!g.request_initiated_at) return g.wait_days; + const elapsedMs = Date.now() - new Date(g.request_initiated_at).getTime(); + return Math.max(0, Math.ceil(g.wait_days - elapsedMs / 86400000)); + } + function renderEmergencyGrants(grants) { const ul = document.getElementById("em-grants-list"); if (!ul) return; @@ -2471,17 +2481,32 @@ const Vault = (() => { } } if (g.status === "pending") { + // The wait period is the only thing standing between a request and + // the grantee reading the vault, so make the countdown explicit + // rather than showing a bare status word. const waitInfo = g.wait_elapsed - ? "Wait period elapsed" - : "Access requested"; - actions = `${waitInfo} - `; + ? "⚠ Wait elapsed — access is available now" + : `⏳ Unlocks in ${_emDaysRemaining(g)} day(s)`; + actions = `${waitInfo} + `; } - return `
  • + + // Retrieval is not blocked after the wait elapses, so the grantor's + // signal that it happened is this badge plus their audit log. + const retrieved = g.vault_retrieval_count + ? `⚠ Vault retrieved ${ + g.vault_retrieval_count + }× · first on ${new Date( + g.vault_retrieved_at, + ).toLocaleDateString()} — remove this grant if unexpected` + : ""; + + return `
  • 🚨
    + ${retrieved}
    ${actions} diff --git a/app/templates/auth/login.html b/app/templates/auth/login.html index 72b2f5a..823bc55 100644 --- a/app/templates/auth/login.html +++ b/app/templates/auth/login.html @@ -7,8 +7,13 @@ block body_class %}auth-page{% endblock %} {% block body %} PassKeeper
    + diff --git a/migrations/versions/l2m3n4o5p6q7_add_emergency_retrieval_tracking.py b/migrations/versions/l2m3n4o5p6q7_add_emergency_retrieval_tracking.py new file mode 100644 index 0000000..4b2fcaf --- /dev/null +++ b/migrations/versions/l2m3n4o5p6q7_add_emergency_retrieval_tracking.py @@ -0,0 +1,49 @@ +"""add vault retrieval tracking to emergency_access + +Revision ID: l2m3n4o5p6q7 +Revises: k1l2m3n4o5p6 +Create Date: 2026-08-26 00:00:00.000000 + +Makes emergency-vault retrieval visible to the grantor. + +Previously GET /api/emergency//vault neither changed the record nor recorded +anything the grantor could see: the audit entry was written under the *grantee's* +user_id, and /api/auth/audit-log filters by user_id, so it never appeared in the +grantor's own log. Combined with the status never advancing past 'pending', a +grantee could re-fetch the snapshot indefinitely with nothing surfacing to the +person whose vault it was. + + vault_retrieved_at — when the snapshot was FIRST retrieved (NULL = never) + vault_retrieval_count — how many times, so repeated access is visible + +Retrieval is deliberately NOT blocked after the first time: the whole premise of +emergency access is that the grantor may be unable to re-provision, and a browser +crash mid-import must not permanently strand the grantee. The wait period remains +the gate; these columns plus the dual audit entries make use of that access +auditable, and the grantor can still revoke with DELETE at any point. +""" +from alembic import op +import sqlalchemy as sa + + +revision = 'l2m3n4o5p6q7' +down_revision = 'k1l2m3n4o5p6' +branch_labels = None +depends_on = None + + +def upgrade(): + op.add_column( + 'emergency_access', + sa.Column('vault_retrieved_at', sa.DateTime, nullable=True), + ) + op.add_column( + 'emergency_access', + sa.Column('vault_retrieval_count', sa.Integer, + nullable=False, server_default='0'), + ) + + +def downgrade(): + op.drop_column('emergency_access', 'vault_retrieval_count') + op.drop_column('emergency_access', 'vault_retrieved_at') diff --git a/tests/conftest.py b/tests/conftest.py index b157cf7..2046c88 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -56,8 +56,13 @@ def auth_headers(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 + """ + Register + log in. Returns (access_token, refresh_token). + + Registration answers 202 for both new and duplicate addresses so it cannot + be used to probe account existence — see test_registration_privacy.py. + """ + assert register(client, email, auth_hash, enc_key_salt).status_code == 202 res = login(client, email, auth_hash) assert res.status_code == 200, res.get_json() body = res.get_json() diff --git a/tests/test_emergency_visibility.py b/tests/test_emergency_visibility.py new file mode 100644 index 0000000..4b3eac7 --- /dev/null +++ b/tests/test_emergency_visibility.py @@ -0,0 +1,173 @@ +""" +Regression tests for emergency-access visibility (finding #9). + +Two problems, both leaving the grantor blind to activity on their own vault: + + 1. Audit entries were written only under the acting user's id, and + /api/auth/audit-log filters by user_id — so a grantee could request access + and retrieve the snapshot without a single line appearing in the grantor's + log or security dashboard. + 2. Retrieval left no trace on the record at all: status stayed 'pending' and + nothing counted, so repeated fetches were indistinguishable from none. + +Retrieval is deliberately still permitted after the first time — the grantor may +be unable to re-provision, which is the whole premise — so the fix is visibility +and revocability, not blocking. +""" +from datetime import datetime, timedelta, timezone + +from app import db +from app.models.audit_log import AuditLog +from app.models.emergency_access import EmergencyAccess +from tests.conftest import auth_headers, make_user + +GRANTOR = 'owner@example.com' +GRANTEE = 'trusted@example.com' + + +def _pair(client): + """Create grantor + grantee, returns (grantor_token, grantee_token).""" + g_token, _ = make_user(client, GRANTOR, 'HASH-O', 'SALT-O') + t_token, _ = make_user(client, GRANTEE, 'HASH-T', 'SALT-T') + return g_token, t_token + + +def _grant(client, grantor_token, grantee_token, wait_days=7): + res = client.post('/api/emergency', headers=auth_headers(grantor_token), + json={'grantee_email': GRANTEE, 'wait_days': wait_days}) + assert res.status_code == 201, res.get_json() + ea_id = res.get_json()['id'] + + assert client.post(f'/api/emergency/{ea_id}/accept', + headers=auth_headers(grantee_token)).status_code == 200 + assert client.post(f'/api/emergency/{ea_id}/provide', + headers=auth_headers(grantor_token), + json={'enc_vault': '[{"id":1,"enc_data":"X","iv":"Y","enc_name":"N","iv_name":"I"}]'} + ).status_code == 200 + return ea_id + + +def _grantor_log(client, token): + res = client.get('/api/auth/audit-log?limit=200', headers=auth_headers(token)) + assert res.status_code == 200 + return res.get_json()['entries'] + + +def _elapse_wait(ea_id): + """Backdate the request so the wait period has passed.""" + ea = db.session.get(EmergencyAccess, ea_id) + ea.request_initiated_at = ( + datetime.now(timezone.utc).replace(tzinfo=None) + - timedelta(days=ea.wait_days + 1) + ) + db.session.commit() + + +def test_access_request_appears_in_the_grantors_audit_log(client, app): + """The grantor has wait_days to notice and deny — they must be able to see it.""" + g_token, t_token = _pair(client) + ea_id = _grant(client, g_token, t_token) + + assert client.post(f'/api/emergency/{ea_id}/request', + headers=auth_headers(t_token)).status_code == 200 + + entries = _grantor_log(client, g_token) + requests = [e for e in entries if e['action'] == 'emergency_access.request'] + assert requests, 'the access request is invisible in the grantor audit log' + assert 'ACTION REQUIRED' in requests[0]['detail'] + assert GRANTEE in requests[0]['detail'] + + +def test_vault_retrieval_appears_in_the_grantors_audit_log(client, app): + g_token, t_token = _pair(client) + ea_id = _grant(client, g_token, t_token) + client.post(f'/api/emergency/{ea_id}/request', headers=auth_headers(t_token)) + _elapse_wait(ea_id) + + assert client.get(f'/api/emergency/{ea_id}/vault', + headers=auth_headers(t_token)).status_code == 200 + + entries = _grantor_log(client, g_token) + retrievals = [e for e in entries + if e['action'] == 'emergency_access.vault_retrieved'] + assert retrievals, 'vault retrieval is invisible in the grantor audit log' + assert GRANTEE in retrievals[0]['detail'] + + +def test_retrieval_is_counted_and_exposed_to_the_grantor(client, app): + g_token, t_token = _pair(client) + ea_id = _grant(client, g_token, t_token) + client.post(f'/api/emergency/{ea_id}/request', headers=auth_headers(t_token)) + _elapse_wait(ea_id) + + grants = client.get('/api/emergency', headers=auth_headers(g_token)).get_json()['grants'] + assert grants[0]['vault_retrieval_count'] == 0 + assert grants[0]['vault_retrieved_at'] is None + + for _ in range(3): + assert client.get(f'/api/emergency/{ea_id}/vault', + headers=auth_headers(t_token)).status_code == 200 + + grants = client.get('/api/emergency', headers=auth_headers(g_token)).get_json()['grants'] + assert grants[0]['vault_retrieval_count'] == 3, 'repeated retrieval not counted' + assert grants[0]['vault_retrieved_at'] is not None, 'first retrieval not timestamped' + + +def test_first_retrieval_timestamp_does_not_move(client, app): + """vault_retrieved_at records FIRST access, so it cannot be reset by re-fetching.""" + g_token, t_token = _pair(client) + ea_id = _grant(client, g_token, t_token) + client.post(f'/api/emergency/{ea_id}/request', headers=auth_headers(t_token)) + _elapse_wait(ea_id) + + client.get(f'/api/emergency/{ea_id}/vault', headers=auth_headers(t_token)) + first = db.session.get(EmergencyAccess, ea_id).vault_retrieved_at + client.get(f'/api/emergency/{ea_id}/vault', headers=auth_headers(t_token)) + assert db.session.get(EmergencyAccess, ea_id).vault_retrieved_at == first + + +def test_grantor_can_still_revoke_after_retrieval(client, app): + """Visibility is only useful if the grantor can act on it.""" + g_token, t_token = _pair(client) + ea_id = _grant(client, g_token, t_token) + client.post(f'/api/emergency/{ea_id}/request', headers=auth_headers(t_token)) + _elapse_wait(ea_id) + client.get(f'/api/emergency/{ea_id}/vault', headers=auth_headers(t_token)) + + assert client.delete(f'/api/emergency/{ea_id}', + headers=auth_headers(g_token)).status_code == 200 + assert client.get(f'/api/emergency/{ea_id}/vault', + headers=auth_headers(t_token)).status_code == 404 + + +def test_acceptance_is_visible_to_the_grantor(client, app): + g_token, t_token = _pair(client) + _grant(client, g_token, t_token) + + accepts = [e for e in _grantor_log(client, g_token) + if e['action'] == 'emergency_access.accept'] + assert accepts, 'grantee acceptance is invisible to the grantor' + + +def test_retrieval_before_the_wait_elapses_is_still_refused(client, app): + """The wait period remains the gate — none of this loosens it.""" + g_token, t_token = _pair(client) + ea_id = _grant(client, g_token, t_token) + client.post(f'/api/emergency/{ea_id}/request', headers=auth_headers(t_token)) + + res = client.get(f'/api/emergency/{ea_id}/vault', headers=auth_headers(t_token)) + assert res.status_code == 403 + assert db.session.get(EmergencyAccess, ea_id).vault_retrieval_count == 0 + + +def test_denying_a_request_stops_retrieval(client, app): + g_token, t_token = _pair(client) + ea_id = _grant(client, g_token, t_token) + client.post(f'/api/emergency/{ea_id}/request', headers=auth_headers(t_token)) + + assert client.post(f'/api/emergency/{ea_id}/deny', + headers=auth_headers(g_token)).status_code == 200 + _elapse_wait(ea_id) # even with time passed, the request was cancelled + + res = client.get(f'/api/emergency/{ea_id}/vault', headers=auth_headers(t_token)) + assert res.status_code == 403 diff --git a/tests/test_registration_privacy.py b/tests/test_registration_privacy.py new file mode 100644 index 0000000..b0ad175 --- /dev/null +++ b/tests/test_registration_privacy.py @@ -0,0 +1,111 @@ +""" +Regression tests for registration account-existence disclosure (finding #8). + +/register answered 409 "Email already registered", which let anyone probe +whether a given address has a PassKeeper account — a ready-made target list for +phishing, and precisely what /login goes out of its way not to reveal. + +Both branches must now be indistinguishable in status, body, and timing. +""" +import time + +from app import db +from app.models.audit_log import AuditLog +from app.models.user import User +from tests.conftest import login, register + + +def test_duplicate_registration_is_indistinguishable(client, app): + first = register(client, 'user@example.com', 'HASH-1', 'SALT-1') + second = register(client, 'user@example.com', 'HASH-2', 'SALT-2') + + assert first.status_code == second.status_code == 202 + assert first.get_json() == second.get_json(), ( + 'the response differs for an existing address, so it can be probed' + ) + + +def test_duplicate_registration_does_not_touch_the_existing_account(client, app): + """The generic response must not come at the cost of overwriting credentials.""" + register(client, 'user@example.com', 'HASH-1', 'SALT-1') + original = User.query.filter_by(email='user@example.com').first() + original_hash, original_salt = original.master_hash, original.enc_key_salt + + register(client, 'user@example.com', 'ATTACKER-HASH', 'ATTACKER-SALT') + + user = User.query.filter_by(email='user@example.com').first() + assert user.master_hash == original_hash, 'existing credentials overwritten' + assert user.enc_key_salt == original_salt + assert User.query.filter_by(email='user@example.com').count() == 1 + + # The original password must still be the one that works. + assert login(client, 'user@example.com', 'HASH-1').status_code == 200 + assert login(client, 'user@example.com', 'ATTACKER-HASH').status_code == 401 + + +def test_registration_response_does_not_name_the_cause(client, app): + register(client, 'user@example.com') + body = register(client, 'user@example.com').get_json() + text = ' '.join(str(v) for v in body.values()).lower() + + for leak in ('already', 'exists', 'taken', 'registered account', 'duplicate'): + assert leak not in text, f'response body leaks existence via {leak!r}: {body}' + + +def test_timing_does_not_disclose_existence(client, app): + """ + Creating an account runs Argon2id, which is deliberately slow. If the + duplicate branch returned early it would be measurably faster and the oracle + would survive in the timing even though the body is identical. + + Uses a loose bound: this asserts the expensive work happens on both paths, + not that timing is cryptographically uniform. + """ + register(client, 'taken@example.com') + + def elapsed(email): + start = time.perf_counter() + register(client, email) + return time.perf_counter() - start + + new_times = [elapsed(f'fresh{i}@example.com') for i in range(3)] + dup_times = [elapsed('taken@example.com') for _ in range(3)] + + new_avg = sum(new_times) / len(new_times) + dup_avg = sum(dup_times) / len(dup_times) + slower, faster = max(new_avg, dup_avg), min(new_avg, dup_avg) + + assert slower < faster * 4, ( + f'timing distinguishes the branches: new={new_avg:.4f}s dup={dup_avg:.4f}s' + ) + + +def test_duplicate_attempt_is_audited(client, app): + """Invisible to the prober, but the operator should still see the attempts.""" + register(client, 'user@example.com') + register(client, 'user@example.com') + + entry = (AuditLog.query.filter_by(action='auth.register_duplicate') + .order_by(AuditLog.id.desc()).first()) + assert entry is not None, 'duplicate registration attempt was not audited' + assert 'user@example.com' not in (entry.detail or ''), ( + 'audit detail should not need the probed address to be useful' + ) + + +def test_validation_errors_still_reported(client, app): + """Input validation does not reveal existence, so it stays specific.""" + assert register(client, 'not-an-email').status_code == 400 + assert client.post('/api/auth/register', json={'email': 'a@b.co'}).status_code == 400 + assert client.post('/api/auth/register', + json={'email': 'a@b.co', 'auth_hash': 'h'}).status_code == 400 + + +def test_new_registration_still_creates_a_usable_account(client, app): + """The privacy fix must not break the happy path.""" + assert register(client, 'fresh@example.com', 'HASH', 'SALT').status_code == 202 + assert User.query.filter_by(email='fresh@example.com').first() is not None + + res = login(client, 'fresh@example.com', 'HASH') + assert res.status_code == 200 + assert res.get_json()['enc_key_salt'] == 'SALT'