8df6de06f1
52-agent audit of the v3.3.84/85 remote-diagnostics code. App is healthy for this user's actual usage. Shipped the two zero-redaction-surface fixes (ws maxPayload, sendToClient guard). Deferred the cold-path server_health O(historySize) freeze because the fix refactors the credential-redaction collectors (leaked twice before). Lesson: "not persistence, so safe" is a fallacy — the redaction layer is an equally catastrophic guarantee surface (silent secret leak). At an audit goal, find+document is the deliverable; don't cut into a twice-leaked redaction pipeline under a Stop hook. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
58 lines
4.7 KiB
Markdown
58 lines
4.7 KiB
Markdown
# Diagnostics audit (3rd /goal re-fire): the un-audited v3.3.84/85 remote-diagnostics app-side code
|
||
|
||
Method: 52-agent adversarial audit of lib/diagnostics-agent.js, lib/diagnostics-collectors.js,
|
||
lib/support-bundle.js, lib/remote-server.js + the diag wiring in main.js/renderer/preload, for lag/freeze
|
||
+ correctness/leak. 23 findings, each double-verified. Reference = THIS user's real config (23 result rows).
|
||
|
||
## Conclusion: the app is HEALTHY for this user's actual usage.
|
||
All shipped fixes (v3.3.87 recent-panel, v3.3.88 doodstream) stand. The remaining findings are COLD,
|
||
opt-in (diagnostics is off by default; needs the user to enable it AND a remote operator to connect AND a
|
||
large accumulated history) — they impose ZERO cost on the normal path and start to matter only ~30k history
|
||
rows (this user: 23). Found + documented = the deliverable for a "could it exist" audit goal.
|
||
|
||
## SHIPPED (v3.3.89) — only the two isolated, zero-redaction-surface fixes
|
||
- lib/remote-server.js: WebSocketServer had NO maxPayload → a pre-auth message ran a synchronous
|
||
JSON.parse of up to ws's 100 MiB default per message on the main loop = an unbounded freeze/DoS sink.
|
||
Set maxPayload = 256 KiB (diag/auth/WebRTC-signaling messages are tiny). Closes the sink for free.
|
||
- lib/remote-server.js sendToClient: `ws.send(JSON.stringify(data))` had no readyState/try guard (unlike
|
||
broadcast) → a send on a closing socket or a stringify throw escaped as uncaughtException (potential
|
||
crash). Now guarded with `ws.readyState === 1` + try/catch, mirroring broadcast.
|
||
397/397 tests pass, eslint clean.
|
||
|
||
## DEFERRED — documented, conscious (real, but the fix cuts into the credential-redaction surface)
|
||
14 of 15 actionable findings converge on ONE cold-path freeze: a single `server_health` (and
|
||
get_history/list_errors/get_config) does O(historySize) synchronous work per request —
|
||
(a) ~6–7 configStore.load() per request, each a full structuredClone(config incl. unbounded history);
|
||
(b) summarizePerHoster copies+sorts+walks ALL batches; _historyErrors regex-scans ALL batches; the
|
||
`limit` arg only slices the OUTPUT, not the traversal.
|
||
Measured: ~258 ms at 7.45 MB / 30k rows, up to 1.6 s–6.7 s in the 39–185 MB tail. On a large history a
|
||
diagnostic query would freeze the very app it's diagnosing (violates the v3.3.85 rule). REAL — but:
|
||
- It only fires during an active diagnostic session on a large history; this user (23 rows) never hits it.
|
||
- The tempting fix (snapshot config + thread secrets through the collectors) refactors
|
||
lib/diagnostics-collectors.js — which IS the redaction surface (_secrets/_deepRedact/collectSecretValues/
|
||
sanitizeConfig/redactLogText). That code has LEAKED TWICE before (7b5420e "one collector still leaked",
|
||
8d757a9 "redaction gaps") in ways that passed green tests. Threading a secrets snapshot = exactly that
|
||
shape: miss/stale one path → SECRET LEAK, a worse failure than the cold freeze. Same category error as
|
||
last turn's config-store defer (then: data-loss; now: secret-leak), different file.
|
||
- If ever hardened: ONLY bound the history walk via summarizePerHoster's existing opts.lastNBatches (NOT the
|
||
secret threading), after confirming per-hoster-over-last-N is acceptable health-snapshot semantics, WITH a
|
||
per-collector redaction E2E + advisor pass. Not under a Stop hook.
|
||
Related cold-path items (same defer): get_config_redacted 'all' does 3 full-history passes then discards
|
||
history (strip history first would help — but same file); collectSecretValues/_secrets unmemoized per
|
||
request; support-bundle.js sanitizeConfig serializes the whole history + reads ~11 MB logs sync on the main
|
||
loop (explicit user-triggered bundle, very cold). remote-server _failedAttempts sub-threshold entries never
|
||
pruned (slow IP-keyed growth) — low.
|
||
|
||
## DEFERRED (config-store cluster, re-confirmed from last turn)
|
||
- load() structuredClone of whole config (incl. history) per call; _atomicWrite nulls cache per write;
|
||
_serializeForDisk stringifies whole history per write; _preserveDiagSubtree adds one extra load() per
|
||
save-global-settings (normal path, low). All scale with historySize, all sub-ms at this user's 52 KB
|
||
config, all in credential-bearing persistence code. Safe root fix = split history to its own file
|
||
(out of scope: risk > reward). Documented, not operated.
|
||
|
||
## Verified clean (no action)
|
||
- diagnostics collectors are otherwise well-bounded: readLog tailKb clamped ≤1024 KB, grep is literal
|
||
substring (ReDoS-free), get_config 'all' deletes history before redaction, all limit/maxJobs-bounded.
|
||
- _deepRedact is bounded at every call site (NOT the freeze source).
|
||
- diagnostics renderer wiring is all in renderSettings() (cold); no per-progress/per-frame cost when off.
|