fix: input hardening + silent-failure hygiene (AGENTS.md doctrine)#39
Merged
Conversation
Second-wave audit (#33 Tier-3) batch. Verified per item; security/regression blockers from the first adversarial review fixed and re-reviewed. A — /fs/glob traversal: globFiles validated cwd but not the glob `pattern`. Reject pattern containing `..` OR isAbsolute(pattern) before constructing Bun.Glob (an absolute `/etc/*` otherwise disclosed real filenames via the response `path` field). Belt-and-suspenders: post-glob containment check with a proper `resolved + "/"` segment boundary. B — input length caps: - codex validators: requireString gained maxLen; session_id/turn_id/ tool_use_id <=128, tool_name/cwd/model <=512, prompt <=50000 — a single field can no longer consume the 1 MiB body and fan to all SSE clients. - sessions validatePromptBody: per-element `parts` validation (non-null object with non-empty string `type`); <=200 caps on model.*/agent; reject empty modelID. providerID '' is ACCEPTED (the dashboard sends '' for "no provider override" — rejecting it regressed the real send-prompt flow; matches main's permissive contract, stricter only on modelID). C — make silent swallows observable per AGENTS.md §"No silent failures": audit/log.ts disk-write failure now surfaced via the app.log fallback; banner/writer.ts returns a WriteBannerResult so the composition root logs a failed GLOBAL banner write (project write stays best-effort); dotenv warns when an EXISTING .env is unreadable (absent stays silent); audit/rotation.ts logs and bails early instead of a partial shift; dashboard.ts warns once if a required asset (main.js/styles.css/sw.js) is missing; remaining provably-irrelevant cleanup catches got accurate justifying comments (server shutdown, bus debug-trace, text stream teardown, filesystem statSync). +~45 tests. bun test 669/0, tsc clean. Two independent fresh-context reviews: first found 2 blockers (absolute-pattern bypass; validator regressed the dashboard) — both fixed with RED->GREEN proof tests; focused re-review APPROVE, blockers confirmed closed, no new regression. Refs #33
…-batch # Conflicts: # src/server/index.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves the hardening + silent-failure hygiene batch of #33 (Tier-3). One coherent PR (defensive input hardening + AGENTS.md §"No silent failures" compliance), not micro-PRs.
A —
/fs/globpattern traversalglobFilesvalidatedcwdbut not the globpattern. An absolute/etc/*(or../../etc/*) madeBun.Globyield real filenames disclosed via the responsepathfield. Now rejectspattern.includes("..") || isAbsolute(pattern)before constructing the glob; plus a post-glob containment check with a properresolved + "/"segment boundary. (Feature gated behindPILOT_ENABLE_GLOB_OPENER, off by default — still a real disclosure.)B — input length caps
requireStringgainedmaxLen;session_id/turn_id/tool_use_id ≤128,tool_name/cwd/model ≤512,prompt ≤50000— a single field can't consume the 1 MiB body and fan to every SSE client/audit.validatePromptBody: per-elementpartsvalidation;≤200caps onmodel.*/agent; reject emptymodelID.providerID: ""is accepted — the dashboard legitimately sends it for "no provider override"; rejecting it (the original spec) regressed the real send-prompt flow. The contract is now main-permissive on providerID, stricter only on the meaningfulmodelID.C — silent swallows made observable (AGENTS.md doctrine)
Per-item fix-vs-comment decision:
audit/log.tsdisk-write surfaced via the app.log fallback;banner/writer.tsreturns a result so the composition root logs a failed global banner write;dotenvwarns on an unreadable existing.env(absent stays silent);audit/rotation.tslogs + bails early (no partial shift);dashboard.tswarns if a required asset is missing; remaining provably-irrelevant cleanup catches got accurate justifying comments.Review trail (this is the one that needed two passes)
providerIDrejection regressed the dashboard's real model-switch send flow.main+ the actual dashboard call path.mainfor valid bodies, no new regression. The one remaining nit (an inaccurate "cross-platform" comment) is fixed in this PR.Testing
bun test669/0 ·bunx tsc --noEmitcleanany/class/console.log-in-server-path introducedIndependent off
main. Touchesserver/index.ts(banner-result wiring) — open #35 also touchesserver/index.ts(lifecycle); regions differ → 3-way mergeable; sequence either way.Refs #33 (Tier-3). With #34–38 + this, all six #33 PR groups are delivered.