Aug 26 - Enhance security 4

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()

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

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'