[Tim020]

Summary

• Client log endpoints (/api/v1/logs, /logs/batch) now require authentication, cap batch size (100 entries, 413 above), message length (4 KB, truncated), and extra size (4 KB, replaced), coerce non-string fields safely, escape newlines in messages, and apply a per-IP rate limit (600 entries/min, 429 + Retry-After). The limiter lives on the application object (no module state).
• Filesystem path settings (db_path, log_path, db_log_path, client_log_path, compiled_script_path, draft_script_path) are only returned by GET /settings and GET /settings/raw to admins, and are never included in the SETTINGS_CHANGED broadcast. Other settings still work pre-login. /settings/raw is used by the admin Settings page in both clients, so it is filtered rather than removed.
• /debug is admin-only; /health, /api/v1/debug and /debug/metrics (Prometheus scrape) are unchanged.
• Both clients: the remote logger no longer posts without a token; entries logged before login are kept (bounded to 50) and sent after login.
• Docs updated (docs/pages/user_config.md).

Tests

• New/updated pytest coverage for log auth, limits, truncation, coercion, rate limiting, path omission for anonymous/non-admin/admin, broadcast payload, and /debug access. Full backend suite, lint/typecheck/vitest for both clients, and the full Playwright suite (chromium) pass.

🤖 Generated with Claude Code

[github-actions]

Client Test Results

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

Results for commit a7db686.

♻️ This comment has been updated with latest results.

[github-actions]

Python Test Results

    1 files      1 suites   1m 20s ⏱️
1 122 tests 1 122 ✅ 0 💤 0 ❌
1 145 runs  1 145 ✅ 0 💤 0 ❌

Results for commit a7db686.

♻️ This comment has been updated with latest results.

[github-actions]

Client V3 Test Results

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

Results for commit a7db686.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (firefox)

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

Results for commit a7db686.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (chromium)

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

Results for commit a7db686.

♻️ This comment has been updated with latest results.

[Tim020]

Review round 1

Head SHA reviewed: a578c9261c5b7dc9cde7c5a4a2bcc1a51bc6ac42

Verdict: changes needed. The core hardening (auth on log endpoints, batch/message/extra limits, rate limiting, /debug admin-gating, and the sensitive-flag approach for settings disclosure) is sound and well-tested. However, two gaps mean the PR doesn't fully deliver what its own description claims, and one robustness gap undermines the "harden against untrusted client input" goal:

Critical

1. The settings file-watcher broadcast still leaks every sensitive path to all clients. server/digi_server/settings.py, Settings.auto_reload_changes() (lines 489–504, unchanged by this PR, wired as the file-watcher callback at line 202) builds settings_json by hand (settings_json[key] = value.get_value() for every key, no sensitive filtering) and calls client.write_message({"OP": "SETTINGS_CHANGED", ...}) on every connected client (self._application.clients) — not just admins. This is a second SETTINGS_CHANGED broadcast path, separate from Settings.set() (which correctly uses await self.as_json(), the one the PR's own test_broadcast_payload_has_no_paths test exercises). Whenever conf/digiscript.json changes on disk (e.g. an operator or another process edits it, or Settings.set()'s own self._save() round-trips through the file watcher), every logged-out/non-admin WebSocket client receives db_path, log_path, db_log_path, client_log_path, compiled_script_path and draft_script_path — exactly the disclosure this PR is meant to close, and the PR description's claim that paths are "never included in the SETTINGS_CHANGED broadcast" is only true for one of the two broadcast call sites. (Independently confirmed by two review agents plus direct inspection of the current code; not anchorable as an inline comment since this method isn't touched by the diff.)

   • Fix: route auto_reload_changes()'s payload through the same sensitive-filtering logic as_json() uses (e.g. a shared sync helper), and add a regression test that triggers a reload with a fake/mock client and asserts no sensitive keys reach write_message.

2. json.loads/escape.json_decode can raise RecursionError on deeply-nested JSON, which isn't caught by the except ValueError: around it in either log endpoint — turning a ~400KB authenticated POST (any logged-in user, not just admin) into an uncaught 500 with a traceback, instead of the 400 the handler intends. Verified directly: json.loads('['*200000 + '1' + ']'*200000) raises RecursionError, which is a RuntimeError subclass, not a ValueError. Both ClientLoggingController.post() (server/controllers/api/v1/logging.py:204-209) and ClientLoggingBatchController.post() (lines 164-169) have this exact pattern. This is precisely the "obscure input turns into a 500" failure mode a hardening PR should be closing, not opening. (Not anchorable inline — the affected try/except blocks are pre-existing code this PR doesn't touch.)

   • Fix: catch (ValueError, RecursionError) around both escape.json_decode calls, with a regression test using deeply-nested input.

Important

3. Neither client re-fetches settings after an in-session login, so admins don't see the path settings until a full page reload. Settings are fetched once at startup (before a token exists), so with this PR's change they come back without the sensitive path keys. Verified in both client-v3/src/stores/user.ts login() (lines 41-73) and client/src/store/modules/user/user.ts USER_LOGIN (lines 103-131) — neither dispatches getSettings()/getRawSettings() (resp. GET_SETTINGS) after a successful login. An admin who opens the app logged out and logs in will see the Settings tab missing Database Path, both log paths, Compiled Script Path and Draft Script Path until they reload the page (or someone else triggers a SETTINGS_CHANGED broadcast). Before this PR, the pre-login fetch returned everything, so this is a regression this PR introduces as a side effect of the new filtering. (Not anchorable inline — these login flows aren't touched by the diff.)
   • Fix: re-fetch settings/raw-settings on successful login in both clients (and consider clearing them on logout, since the Vue 2 client persists its store via vuex-persistedstate, so a shared device's next user could otherwise still read a previous admin's cached path settings).

4 inline comments below cover a silent-drop/observability gap in the batch entry loop (and a worse version of the same gap in the single-entry endpoint), a misleading _clean_extra docstring plus a spoofable log sentinel, and several smaller type-design/comment-polish suggestions on the new ClientLogRateLimiter.

Test coverage gaps (not individually anchored; see test-coverage review for detail)

• No test proves the rate limiter's per-IP isolation — both existing tests only ever exercise one active IP at a time, so a bug that keyed the limiter globally instead of per-IP would pass every current test.
• No batch test mixes one oversized/malformed entry with a normal sibling to prove per-entry limits are applied independently (not via a shared/mutated buffer).
• Zero Vitest coverage in either client for the new queue-cap (shift() at MAX_QUEUE_SIZE) and pre-login-queue-then-flush-after-login logic in logger.ts — this is exactly the kind of client-side ordering behavior pytest/E2E won't catch, and it's called out as a feature in the PR description.
• The empty-batch ({"batch": []}) boundary is untested.

SonarQube

Quality gate: PASSED. Source: anonymous SonarCloud API (dreamteamprod_DigiScript, analysis confirmed fresh for head commit a578c926, analysed 2026-10-20T23:40:55Z). 0 new issues, 0 security hotspots, duplication 1.9% (threshold 3%), all new-code ratings at best grade. new_coverage wasn't evaluated because the diff has 0 coverable new lines per Sonar's analyzer — not itself evidence of a coverage gap, just that Sonar found nothing numeric to measure here. Nothing required to pass the gate.

New findings summary

• Critical: 2 (both non-line-anchorable; see above)
• Important: 2 (1 non-line-anchorable; 1 inline — silent batch-entry drop)
• Suggestion: 5 inline (docstring/sentinel clarity, rate-limiter type design ×2, settings comment duplication, test assertion strength)

Strengths

• The auth/limit/truncation/rate-limit hardening on the log endpoints is well-structured, and the rate limiter's core fixed-window algorithm, lazy per-application init, and single-threaded-safety reasoning all check out correctly on inspection (no races, no shared module state across test workers).
• The sensitive kwarg approach for settings (added in response to this PR's own earlier review round) is a clean, minimal design, consistently applied to as_json/raw_json//api/v1/settings//api/v1/settings/raw.
• /debug admin-gating matches the decorator ordering used elsewhere (auth/user.py, db_backups.py), and the three debug-adjacent routes are correctly separate, non-conflicting registrations.
• Good boundary-value testing for the batch size cap, and the truncation/oversized-extra tests verify the absence of leaked raw content, not just that some replacement happened.
• Frontend queue-then-flush-after-login logic is correct and symmetric between both clients, and doesn't drop or splice entries prematurely on the new early-return path.

🤖 Generated with Claude Code

[Tim020]

Review round 1 addressed in a7db686.

• Critical 1 (file-watcher broadcast leaked paths): auto_reload_changes now builds its payload via a shared helper that omits sensitive settings; regression test drives a reload with a fake client.
• Critical 2 (deeply nested JSON): both log endpoints catch RecursionError alongside ValueError and return 400; _clean_extra also tolerates it. Test added for both endpoints.
• Important 3 (settings not refetched after login): both clients now re-fetch raw settings after a successful login, and also after logout so the admin-only values are dropped from the cache (the Vue 2 store is persisted).
• Inline items: see the resolved threads.
• Test gaps: added per-IP isolation, mixed batch, empty batch tests, and Vitest coverage for the logger queue cap and flush-after-login in both clients.

Sonar gate to be confirmed on this head.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

[Tim020]

Review round 2

Head SHA reviewed: a7db686a4d56c1dc12bcb9b9ba8aa2dceed2a327

Verdict: ready to merge, pending a final look from a human maintainer. Every finding from round 1 — both Critical items, the Important item, all 7 inline comments, and every noted test-coverage gap — has been addressed, and I independently verified each fix by reading the resulting code and by actually running the affected test suites (not just trusting the commit message).

Previous round follow-up

All 10 round 1 findings verified fixed in a7db686:

• Critical, verified fixed: Settings.auto_reload_changes() now routes through the new shared Settings._values(include_sensitive) helper (the same one as_json() uses), so the file-watcher broadcast no longer leaks sensitive paths to non-admin clients. New test test_file_reload_broadcast_has_no_paths drives a reload with a fake client and asserts this directly.
• Critical, verified fixed: both log endpoints now catch RecursionError alongside ValueError around escape.json_decode (and _clean_extra's json.dumps call does the same for defense in depth), so deeply-nested JSON gets a 400 instead of an uncaught 500. I reproduced the original crash and reran it against the fix — test_deeply_nested_json_returns_400 covers both endpoints and passes.
• Important, verified fixed: both clients now re-fetch raw settings after a successful in-session login (and again after logout, so a shared/persisted Vue 2 store drops admin-only values from its cache).
• 7/7 inline comments, verified fixed (see individual replies on each thread): single-entry endpoint now validates its body is an object; non-dict batch entries are counted and logged once per request instead of vanishing silently; the _clean_extra docstring now documents all three outcomes and its extra_omitted marker is applied after the client/request merge so it can't be forged; the rate limiter validates limit/window > 0, uses a typed _Window NamedTuple, and check() returns Optional[float] instead of an overloaded 0.0/positive sentinel; both the pruning comment and the settings sensitive-flag documentation were corrected and de-duplicated.
• All flagged test-coverage gaps, verified fixed: new tests for rate-limiter per-IP isolation (test_rate_limiter_budgets_are_per_ip), mixed-batch independence (test_oversize_entry_does_not_affect_sibling_entries), the empty-batch boundary (test_empty_batch_accepted), and new Vitest coverage for the queue-cap/flush-after-login logic in both client-v3/src/js/logger.test.ts and client/src/js/logger.test.ts.

None still open, none disputed, none needing a human decision.

SonarQube

Quality gate: PASSED for this head (check run SonarCloud Code Analysis completed/success; bot comment posted 2026-10-21T00:06:34Z, after the push). 0 new issues, 0 security hotspots, duplication 1.0% on new code (down from 1.9%, well under the 3% threshold). Nothing required to pass the gate.

New findings

None. I re-read the full incremental diff (git diff a578c926..a7db686 for the touched files) and additionally ran, in a scratch worktree at the new head: the full backend suite for the touched files (pytest test/controllers/api/v1/test_logging.py test/controllers/api/v1/test_settings.py test/digi_server/test_digi_server.py — 47 passed) and both clients' new logger.test.ts (2/2 passed each, after installing deps). No regressions, no new issues introduced by the fixes themselves.

Strengths

• Exceptionally responsive round: every single finding from round 1 — including the two findings that weren't even postable as inline comments, because they pointed at pre-existing code outside the diff — was traced back to its root cause and fixed, not just patched at the symptom.
• The _values() extraction is a good refactor: it gives as_json() and auto_reload_changes() one shared, correctly-filtered source of truth instead of two parallel implementations that could drift again.
• The extra_omitted fix (server-set key applied after the client/request merge) is the right shape for "can't be forged by the data it's describing" — and it shipped with a test that tries to forge it.
• Test additions throughout are behavioral, not just coverage-padding: test_file_reload_broadcast_has_no_paths uses a real fake client against the real broadcast method, test_oversize_entry_does_not_affect_sibling_entries actually proves independence rather than asserting a status code.

🤖 Generated with Claude Code