Fix critical + high severity issues from code audit (B1–B5) - #365
Merged
Conversation
Addresses the Critical and 4 High-severity findings from a full code audit of the daemon. Each was independently verified against source. - B1 (Critical): RCE via issue title in the tmux pane script. Untrusted titles were interpolated into a double-quoted `echo -e` with only quote escaping, so `$(…)`/backticks executed as shell. Extract buildPaneBanner() which single-quotes the name via shellEscape (also de-dups the two script branches). - B2 (High): Execution success path discarded committed-but-unpushed work. Now salvages (push + draft PR) instead of returning null into cleanup. - B3 (High): Reflected XSS via the window.__CRITTERS__ bootstrap. Escape script-breaking chars (<, >, &, U+2028/9) in the serialized JSON. - B4 (High): `clean --panes` could kill live critters on a transient health blip. Distinguish connection-refused (down) from timeout/non-200 (maybe alive) and fail closed. - B5 (High): Transient tracker error on a terminal success transition flipped a real success into the failure path. Wrap success outcomes defensively, mirroring the already-guarded failure path. Adds regression tests for B1, B3, B4. typecheck + lint clean, 850 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
resolvePhaseMcpConfig expands ~ via node:os homedir(); the test compared it to process.env.HOME. Bun caches homedir() on first call and ignores later process.env.HOME mutations, so when a sibling test changes HOME they diverge on Linux CI and this test fails (works on macOS by execution-order luck). Assert against the same homedir() the implementation uses. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This was referenced Jul 18, 2026
Merged
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
Fixes the Critical and 4 High-severity findings from a full code audit of the daemon. Each was independently verified against source before and after the fix.
src/cli/spawn.tsbuildPaneBanner()single-quotes the untrusted window name viashellEscape, so$(…)/backticks in a title can no longer execute as shell (also de-dups the two identical script branches)src/runner/execution.tsnullinto the cleanup-and-delete pathwindow.__CRITTERS__bootstrapsrc/dashboard/main-page.ts</>/&/U+2028-9 to\uXXXXin the serialized JSON so a path-segmentidentifiercan't break out of the inline<script>clean --paneskills live critters on a health blipsrc/cli-clean.tssrc/unified-spawner.tsapplySuccessOutcome()wraps the four success/merged/needs-changes transitions defensively, mirroring the already-guarded failure pathTests
New
src/__tests__/audit-fixes.test.ts: B1 (banner neutralizes$(…)/backticks/quotes), B3 (escapes</script>, JSON round-trips to original values), B4 (refused-vs-timeout classification, fail-closed default).typecheck clean · lint clean · 850 tests pass (842 existing + 8 new).
Notes
salvagePartialProgressgit-harness tests + code review rather than a dedicated unit test.🤖 Generated with Claude Code