fix(security+watcher): post-review hardening (path traversal, worktree regex, viewer watch leak) - #8
Merged
Conversation
…drain viewer watches Follow-up to the four PR reviews. Five independent improvements, none strictly required but each shipped well-defined feedback from the agents: - **Subagent IPCs path-traversal guard**: read-subagent-jsonl and start-subagent-watch took parentSessionId/agentId straight from the renderer and fed them to path.join. A compromised renderer (or a bug in cache payload) could traverse out of <projects>/<folder>. Add SAFE_ID_RE + a path.resolve()-based defence-in-depth check that the resolved jsonl path still lives under the cache row's folder. - **Worktree name hardening**: the regex now also rejects names matching `.` / `..` / leading `-` (the latter to keep git from parsing the name as a flag in some contexts). Parent repo must be absolute and not flag-like. - **Friendly git ENOENT error**: when git is absent from PATH, delete-worktree returned `spawn git ENOENT` to the renderer — opaque. Now returns "git not found on PATH — install git and retry". - **Viewer watch leak**: stopWatch closures registered in activeViewerWatches; hideAllViewers / showJsonlViewer / showSubagentTranscript drain via drainViewerWatches. Previously every expanded Agent block with a live tail kept polling indefinitely after the viewer was dismissed (until app shutdown). 32 tests still pass, 0 lint errors.
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.
Aggregates the security-relevant and correctness suggestions from the four code-reviewer agents that reviewed PR#47/#48/#49 today. None block functionality; each closes a small but real foothold.
Changes
1. Subagent IPC path-traversal guard
`read-subagent-jsonl` and `start-subagent-watch` took `parentSessionId`/`agentId` from the renderer and fed them to `path.join` without validation. A compromised renderer (or a bug in the cache payload) could traverse out of `/`. Adds `SAFE_ID_RE = /^[a-zA-Z0-9_-]+$/` plus a defence-in-depth check that the resolved jsonl path still lives under the cache row's folder.
2. Worktree name hardening
Adds `WORKTREE_NAME_BAD = /^-|^\.\.?$/` to reject `.` / `..` / leading-dash worktree names. Verifies `parentRepo` is absolute and not flag-like before passing to `git -C`.
3. Friendly git ENOENT error
When git isn't on PATH, `delete-worktree` previously returned `spawn git ENOENT` to the renderer. Now returns `git not found on PATH — install git and retry`.
4. Viewer watch leak drain
Every expanded Agent block with a live tail started an `fs.watchFile` poll but never stopped it when the viewer was dismissed (only on individual block collapse / completion event). Track watches in a module-level `activeViewerWatches` set; `hideAllViewers` / `showJsonlViewer` / `showSubagentTranscript` drain it via `drainViewerWatches`.
Verification
```
$ npm test
tests 32 / pass 32 / fail 0
$ npm run lint
✖ 206 problems (0 errors, 206 warnings)
```
206 warnings are pre-existing tech debt — no new lint errors.
Related