[Tim020]

Summary

Low-severity hygiene hardening for the backend.

• New shared helper utils/web/json_body.get_json_object(): invalid JSON or a non-object body now returns 400 instead of a 500.
• require_admin now returns 401 only when not logged in, and 403 for a logged-in non-admin. Previously a non-admin got 401, which both clients' fetch interceptors treat as an expired session (refresh/logout), so 403 is also better UX. No client code or E2E asserted on this; backend tests were updated.
• RBAC grant/revoke: missing/mistyped fields, non-integer user, invalid role value and incomplete object keys now return 400 rather than 500.
• PATCH /user/settings: explicit allow-list of editable fields (replaces hasattr check).
• SessionStartController: already verifies session_id belongs to the requester and no longer dereferences user_session.user (present on dev); left as is, JSON parsing now uses the helper.
• Security headers in BaseController.set_default_headers: X-Content-Type-Options: nosniff, X-Frame-Options: SAMEORIGIN, Referrer-Policy: same-origin. CSP intentionally omitted: the SPA and docs use inline styles/scripts, and a policy loose enough not to break them adds little.

JSON helper applied to: auth/user.py (create, delete, login, token, change-password, admin reset), v2 users (POST, PATCH), settings PATCH, rbac grant/revoke, user/settings PATCH, sessions/start. Not converted: the show-content handlers (cast, acts, scenes, cues, characters, mics, tags, overrides, logging), which can adopt the helper incrementally.

Testing

• New test_security_hygiene.py (17 tests); 10 existing non-admin tests updated from 401 to 403.
• Full pytest: 1062 passed. ruff check/ruff format clean.
• Full Playwright suite (chromium): 225 passed, confirming the SPA loads with the new headers.
• No client source changed, so no client lint/typecheck/vitest needed.

🤖 Generated with Claude Code

[github-actions]

Client Test Results

98 tests   98 ✅  0s ⏱️
 7 suites   0 💤
 1 files     0 ❌

Results for commit dc2d203.

♻️ This comment has been updated with latest results.

[github-actions]

Client V3 Test Results

223 tests   223 ✅  1s ⏱️
 18 suites    0 💤
  1 files      0 ❌

Results for commit dc2d203.

♻️ This comment has been updated with latest results.

[github-actions]

Python Test Results

    1 files      1 suites   1m 9s ⏱️
1 102 tests 1 102 ✅ 0 💤 0 ❌
1 125 runs  1 125 ✅ 0 💤 0 ❌

Results for commit dc2d203.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (chromium)

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

Results for commit dc2d203.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (firefox)

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

Results for commit dc2d203.

♻️ This comment has been updated with latest results.

[Tim020]

Review round 1

Head SHA reviewed: 05ff95c2abcbc01386845a8a03dfc57c593923bc (the PR moved from 1c91d982 to this commit — a merge from dev plus a new test — partway through this review; findings below are checked against this current head, and the WS-message-size-metrics/--debug-default/client-v3 changes reviewed earlier are no longer part of this PR's diff, since they're now identical to dev)

Verdict: changes requested — the SonarQube quality gate is currently failing. The core security fixes are sound (the shows-DELETE auth bypass fix is real and well-tested), but there's a real new-code duplication regression to fix, plus a docstring that overclaims header coverage and one test with a gap.

Previous round follow-up: N/A — this is round 1, no prior threads exist.

SonarQube: quality gate FAILED. Source: anonymous SonarCloud public API. Freshness verified: analysis commit matches head SHA exactly, analysed shortly after the push (23:08:27Z).

Condition	Actual	Required	Result
New duplicated lines density	4.1%	≤ 3%	FAIL
New issues	0	—	pass
New security hotspots reviewed	100%	100%	pass
New reliability/security/maintainability rating	A	≤A	pass

The failing condition is driven almost entirely by server/controllers/api/v1/rbac.py (14 of 23 new-code duplicated lines; see inline comment below) — a 32-line block duplicated between the Grant and Revoke handlers. Two negligible 1-line hits in auth/user.py and v2/users/users.py are just the shared data = get_json_object(self) line matching elsewhere and aren't worth separate action. Fixing the rbac.py duplication should bring new-code duplication back under 3% and pass the gate.

New findings: 3 Important (all anchored inline below).

Suggestion (not anchorable — no diff lines to attach to): ~45 raw tornado.escape.json_decode(self.request.body) call sites remain across other files outside this PR's scope (show config controllers, user/overrides.py, logging.py, etc.) — these still 500 on malformed JSON instead of 400. Fine as an incremental migration; worth a follow-up issue so it doesn't get forgotten.

Strengths:

• The shows.py DELETE handler was missing @api_authenticated/@require_admin entirely at the PR base — any unauthenticated caller could delete a non-loaded show. The fix's decorator order matches every other admin-only endpoint, and the new tests assert the show still exists in the DB afterward (not just the status code), which is strong proof the fix actually works rather than being vacuous.
• require_admin returning 403 instead of 401 for a logged-in non-admin is a genuine bug fix, not just cosmetic: both clients' HTTP interceptors treat 401 as "token expired" and trigger a refresh/logout loop, which the old behaviour could have caused for non-admins hitting admin-only routes.
• EDITABLE_USER_SETTINGS replaces a hasattr()-based check that was effectively a mass-assignment risk; the allowlist was independently verified to match the model's editable columns exactly.
• --debug defaulting to False (was not IS_FROZEN, which meant any non-PyInstaller/docker deployment ran in debug by default) closes a real production exposure, and is backed by a regression test that parses main.py's AST so it can't silently vacuously pass.
• All the 401→403 test updates are edits to pre-existing assertions rather than new tests, which is good evidence they'd have failed before the fix.
• The WSMessageSizeMetricsMixin double-counting guard is correct: Tornado awaits each on_message future before reading the next frame, so the per-instance flag can't race.

🤖 Generated with Claude Code

[Tim020]

Review round 1 addressed. Fixed: static-route security headers and header helper (dc2d203), user settings allow-list test strengthened (dc2d203), RBAC grant/revoke duplication (6616d4f, Sonar passing). Follow-up noted, not in this PR: ~45 remaining raw json_decode call sites in show-config controllers, overrides and logging can adopt get_json_object incrementally. pytest 1102 passed, ruff clean, full chromium Playwright 225 passed.

🤖 Generated with Claude Code

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

[Tim020]

Review round 2

Head SHA reviewed: dc2d20370da0a0a3eb7731d7aa68ef6fa0fde0cb

Verdict: ready to merge. All three round-1 findings are genuinely fixed, no new issues found in the fix-up diff, and the SonarQube quality gate is green.

Previous round follow-up: 3 verified fixed, 0 still open, 0 fix-incomplete, 0 needs-human-decision.

• Security headers only covered BaseController, not static routes — verified fixed: new utils/web/security_headers.py (apply_security_headers + SecureStaticFileHandler) is used by both BaseController and the /assets/, /ui-old/assets/, /docs/ routes, with a new test covering both the 200 and 404 paths.
• User-settings allow-list test didn't prove non-editable attributes were skipped — verified fixed: the test now records every UserSettings.__setattr__ call during the PATCH and asserts metadata/registry/_user_id/nonexistent are never assigned; the legacy session.query() call is also gone.
• rbac.py Grant/Revoke duplication causing the Sonar gate failure — verified fixed: both handlers now extend a shared _RBACRoleChangeHandler and only override _apply_role; no duplication remains.

SonarQube: Quality gate passed. 0 new issues, 0 security hotspots, 0.0% new coverage gate n/a, 0.4% new-code duplication (well under the 3% limit, down from the 4.1% that failed round 1). Source: anonymous SonarCloud public API. Freshness verified: analysis matches head SHA, completed 2026-10-20T23:26:55Z.

New findings: none. I read the full fix-up diff (6 files: rbac.py, app_server.py, test_settings.py, the new test_security_headers.py, base_controller.py, the new security_headers.py) directly rather than relaunching the full agent suite, since it's a small, targeted diff — no new correctness, security, or test-coverage issues found. One thing checked and confirmed sound: controllers.StaticController (serving /favicon.ico) was already a BaseController subclass, so it was never part of the header gap and needs no change.

Strengths: the _RBACRoleChangeHandler/_apply_role refactor is a clean template-method fix that reads better than the original duplicated handlers, not just a gate-passing workaround. The __setattr__-recording test technique is a notably strong way to prove a "this must never be set" invariant rather than just checking a final value.

🤖 Generated with Claude Code