[Tim020]

Summary

Token lifecycle hardening (M4, M5, M6 of the backend security review).

• Shared JWT validation (M4): JWTService.validate_access_token performs revocation, signature/expiry, per-token age and token_version checks in one place. REST prepare() and WebSocket authenticate_with_token both use it, so the WebSocket now rejects tokens invalidated by a password change/reset.
• Persisted revocation (M5): logout revocations are stored by jti in a new revoked_token table (write-through in-memory cache), pruned of expired entries on load and on every revoke, so they survive a restart. Chosen over bumping token_version on logout because that would log the user out of all their other devices.
• Hashed API keys with transparent upgrade (M6): keys are stored as the SHA-256 of the key and authenticated by a single indexed lookup plus hmac.compare_digest. Existing keys keep working: if a presented key matches no SHA-256 row, only the remaining legacy bcrypt rows are checked, and a match is rewritten to the SHA-256 form in place, so each legacy key is scanned at most until its first use. The legacy scan runs only while legacy rows exist (cached flag, invalidated on key generate/revoke/upgrade) and failed scans are limited per remote address (5 per 60 s, then 429; per-app instance, injectable clock).
• Migration: adds the revoked_token table only; api_token values are untouched. No user action needed.

No client changes. User docs updated (user_settings.md, user_config.md).

Tests

Added to the modules mirroring the code under test: test_jwt.py, test_ws_controller.py, test_auth.py, test_api_keys.py, test_token.py for the v1 and v2 token controllers, and test/alembic_config/versions/test_persist_revoked_tokens.py. Full pytest, ruff, alembic check and the full Playwright Chromium suite pass.

🤖 Generated with Claude Code

[github-actions]

Client Test Results

100 tests   100 ✅  0s ⏱️
  8 suites    0 💤
  1 files      0 ❌

Results for commit 0411384.

♻️ This comment has been updated with latest results.

[github-actions]

Client V3 Test Results

225 tests   225 ✅  1s ⏱️
 19 suites    0 💤
  1 files      0 ❌

Results for commit 0411384.

♻️ This comment has been updated with latest results.

[github-actions]

Python Test Results

    1 files      1 suites   1m 0s ⏱️
1 156 tests 1 156 ✅ 0 💤 0 ❌
1 179 runs  1 179 ✅ 0 💤 0 ❌

Results for commit 0411384.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (chromium)

225 tests   225 ✅  2m 13s ⏱️
 14 suites    0 💤
  1 files      0 ❌

Results for commit 0411384.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (firefox)

225 tests   225 ✅  2m 13s ⏱️
 14 suites    0 💤
  1 files      0 ❌

Results for commit 0411384.

♻️ This comment has been updated with latest results.

[Tim020]

Review round 1

Head SHA reviewed: 30cc1a5

Verdict: changes needed. This is a well-tested, focused rework of token/API-key auth, but four parallel review agents (code-reviewer, pr-test-analyzer, silent-failure-hunter, type-design-analyzer) converged on the same critical bug from three independent angles, plus two real race conditions. All are small, surgical fixes, not a redesign.

Previous round follow-up: none — this is round 1.

SonarQube: Quality Gate passed. All 5 gate conditions pass (reliability/security/maintainability ratings A, 0.3% new duplication, 0 hotspots). 3 new MINOR issues (python:S6546, "use a union type expression") in the auto-generated Alembic header of server/alembic_config/versions/d91e929f2e32_persist_revoked_tokens.py lines 17-19 — trivial Union[...] → | syntax swaps, optional cleanup, not gate-blocking.

CodeQL: 3 open alerts, all py/weak-sensitive-data-hashing (CodeQL-rated "high" severity) flagging SHA-256 as weak "password" hashing in server/utils/web/api_keys.py:57, server/test/controllers/api/v1/auth/test_token.py:33, server/test/controllers/api/v2/test_token.py:29. Assessed as false positives: the hashed value is a secrets.token_urlsafe(32) 256-bit random API key, not a user-chosen password — there's no low-entropy secret to brute-force, so a KDF buys nothing and costs CPU on every request. This is explicitly documented in ApiKeyService's class docstring. No code change needed; worth dismissing the 3 alerts in the Security tab (reason: false positive) so they stop showing as "high severity" to anyone scanning the PR checks. (2 older CodeQL threads on this PR, on now-superseded lines, are already resolved+outdated — no action needed there either.)

New findings: 1 Critical, 3 Important, 3 Suggestions (inline) — plus 3 test-coverage gaps noted below (not inline; nothing to anchor a defect to, they're absences).

• Test-coverage gaps (pr-test-analyzer): (1) validate_access_token()'s TOKEN_EXPIRED and TOKEN_INVALID reason codes are never asserted directly in the new TestJWTAccessTokenValidation class in server/test/utils/web/test_jwt.py — only hit incidentally by unrelated pre-existing tests; worth two direct cases there. (2) The rate limiter's overflow-eviction test (test_api_keys.py::test_failures_expire_after_window_and_tracking_is_bounded) only asserts the dict stays bounded, never that a previously-limited address actually gets amnesty when eviction fires — worth a behavioral test through authenticate() itself. (3) The migration test (test_persist_revoked_tokens.py) covers upgrade() only; add a test_downgrade_drops_table.

Strengths: ApiKeyService's fast-path/legacy-bcrypt-fallback split is a clean, pragmatic hash-migration design with no backfill job needed; test_api_keys.py and test_jwt.py are genuinely behavioral (mock-free assertions on real HTTP/DB round-trips, injectable clock instead of time.sleep); test_revocation_survives_service_restart is a real restart simulation, not shared state; every one of the 4 non-test User.api_token write sites correctly calls api_keys.invalidate(); reST docstrings on the main public methods follow this repo's convention; the migration is simple, symmetric and reversible.

🤖 Generated with Claude Code

[Tim020]

Review round 1 follow-up (51cf770)

All inline items fixed in 51cf770; replies are in each thread.

Non-inline points:

• Test gaps: added direct TOKEN_EXPIRED and TOKEN_INVALID cases (test_jwt.py), a behavioural rate-limiter eviction test through authenticate() (test_api_keys.py), and test_downgrade_drops_table (migration test).
• Sonar S6546: switched the migration header to X | None syntax.
• CodeQL py/weak-sensitive-data-hashing: no code change in this round; the keys are 256-bit random secrets and SHA-256 is the agreed design. The alerts are being handled separately.

[Tim020]

Review round 2

Head SHA reviewed: 51cf770

Verdict: ready to merge, pending one optional nit below. All 7 round-1 findings (1 Critical, 3 Important, 3 Suggestions) are verified fixed by reading 51cf770 directly and by running the full affected test suite (test_api_keys.py, test_jwt.py, test_persist_revoked_tokens.py, test_token.py v1/v2, test_ws_controller.py — 186 tests, all pass) against that commit.

Previous round follow-up: 7/7 verified fixed, replies posted on each thread with specifics:

• Critical fail-open in _load_revoked() — fixed; flag only set after a successful load, failure logs and retries. New test: test_failed_revocation_load_is_retried_not_treated_as_empty.
• Important: unverified-claims revocation — fixed; revoke_token now requires a verified decode_access_token. New test: test_unsigned_token_is_never_persisted_on_revoke.
• Important: rate-limiter TOCTOU race — fixed; slot reserved before the scan. New test: test_concurrent_burst_from_one_address_is_bounded. The eviction strategy was also improved beyond what was asked (idle addresses evicted before active ones) — nice catch, covered by test_limited_address_stays_limited_when_others_fill_tracking.
• Important: legacy-upgrade lost-update race — fixed; conditional UPDATE ... WHERE api_token = :stored. New test: test_key_regenerated_during_legacy_check_is_not_revived.
• Suggestion: stale comment — fixed.
• Suggestion: hmac.compare_digest — kept deliberately with a documented reason (DB-collation independence); accepted, no further action.
• Suggestion: ValueError logging gap — fixed, with a test asserting the warning text. New test: test_malformed_legacy_hash_is_rejected_and_logged.

None needed human-decision escalation — every reply either confirmed a fix or accepted the author's reasoning.

SonarQube: Quality Gate passing. 0 new issues (the 3 python:S6546 union-type style issues from round 1 were also fixed, though only mentioned in the round-1 body, not required). 0 security hotspots, 0.2% new duplication.

CodeQL: 0 open alerts on this PR's head (the 3 py/weak-sensitive-data-hashing false positives from round 1 are gone).

New findings: 1 Suggestion, introduced by this round's own rate-limiter fix.

Strengths: this is exactly the kind of round 2 I'd hope for — every fix is small and surgical, each one lands with a test that specifically targets the failure mode it closes (not just incidental coverage), and the author went beyond the literal asks on the rate-limiter eviction strategy. Good PR.

🤖 Generated with Claude Code

[Tim020]

Review round 2 follow-up (0411384)

The single suggestion (guard the rate-limit reservation release) is fixed in 0411384, with a test; reply is in the thread. No non-inline points to address.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.2% Duplication on New Code

See analysis details on SonarQube Cloud

[Tim020]

Review round 3

Head SHA reviewed: 0411384

Verdict: ready to merge. This round is a one-line fix plus a test closing the single Suggestion left open from round 2. No new issues found.

Previous round follow-up: 1/1 verified fixed. The round-2 Suggestion (attempts.remove(now) could raise ValueError if the rate-limit reservation aged out of the window during a long legacy-bcrypt scan) is now guarded with if now in attempts: attempts.remove(now), backed by a new test, test_upgrade_succeeds_when_reservation_aged_out_during_scan, that forces exactly that timing via a mocked clock. Verified against 0411384 directly and by running test_api_keys.py (12 tests) — all pass.

All 8 findings raised across rounds 1-2 (1 Critical, 3 Important, 4 Suggestions) are now confirmed fixed.

SonarQube: Quality Gate passing, 0 new issues, 0 security hotspots, 0.2% new duplication — unchanged from round 2 (this round's diff is 2 files, 22 lines).

CodeQL: 0 open alerts.

New findings: none.

Strengths: this closes out a three-round sequence where every single finding — including a subtle edge case this review itself introduced by suggesting the fix — got a real, targeted regression test, not just a code change. Good to merge.

🤖 Generated with Claude Code