review: post-merge fixes for phases 1, 6, 2, 5#6
Merged
Conversation
…aude config Two issues from the post-merge fresh-eyes review of phase 6: - The /spawn-sub-agent endpoint passed a user-supplied workingDir straight into child_process.spawn's cwd with no validation. An authenticated client could point claude at /etc, /var, or anywhere else the server uid can reach. Now we resolve the path, require it to live under $HOME (or DISPATCH_PROJECT_ROOTS), reject sensitive credential subdirs, and confirm it exists as a directory before spawning. - ensureRecommendedMCPs ran as a top-level module side-effect on import and would happily materialize a fresh ~/.claude.json containing only Dispatch's two MCPs for users who had never run claude. The bootstrap now refuses to create the file from nothing and defers via setImmediate so the import chain in server/index.js is not blocked by file IO.
…tasks path safety Three security fixes from the post-merge fresh-eyes review of phase 5: - Preview proxy used to forward any port from 1-65535 to 127.0.0.1, so any authenticated user could probe localhost services (mysql, postgres, redis, the cdp port itself, etc). Now defaults to a curated allowlist of common dev-server ports, blocks well-known infra ports outright, and exposes DISPATCH_PREVIEW_PORTS / DISPATCH_PREVIEW_ALLOW_ANY_HIGH_PORT for users who need something else. - Chrome screencast attaches to the operator's real browser over CDP and was reachable by any logged-in user. The Input.* dispatch path let any client drive the host browser (steal cookies via Runtime.evaluate, navigate to attacker URLs, etc). Now disabled by default; opt in via DISPATCH_CHROME_VIEW_ENABLED=true for view-only and additionally DISPATCH_CHROME_VIEW_ALLOW_INPUT=true for take-control. The HTTP /status and /tabs helpers report the disabled state cleanly so the client UI can render an explanation instead of a stack trace. - /api/tasks built the JSONL path from query params with no traversal guard. Now rejects path separators, null bytes, and '..' on projectName + sessionId, then prefix-checks the resolved path against ~/.claude/projects.
…y context Two phase 5 frontend follow-ups from the post-merge review: - PreviewModal and BrowserModal shipped in phase 5 but were never imported anywhere, so mobile users on the preview/browser tabs got the desktop pane shoved into the tab slot with no fullscreen treatment or close button. MainContent now mounts both modals, gated by isMobile + activeTab; the inline panes stay desktop-only. Closing a modal returns to the chat tab. - WorktreeList accepted activeSessions/processingSessions/blockedSessions /worktreeSessionMap props but its parent (SidebarProjectItem) is on the no-edit churn list, so the props were never passed and every dot stayed grey. New SessionActivityContext lives at the AppContent root and delivers the same protection sets to WorktreeList without prop drilling. worktreeSessionMap is best-effort for now: only the currently selected worktree's session can be attributed; cross-worktree resolution needs a server endpoint, tracked in docs/follow-ups.md.
Captures every NICE-TO-HAVE finding from the four phase reviews so the items aren't lost. Entries are grouped by phase with file:line citations; each is small enough to land in a polish PR or fold into the next phase.
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
Fresh-eyes Opus review of the four phases that landed earlier today (PRs #1 / #2 / #3 / #4). Critical findings were fixed in this branch; nice-to-haves were captured in
docs/follow-ups.mdfor the next polish PR.This PR is additive only — none of the three churn-restricted files (
server/projects.js,server/index.js,src/components/sidebar/view/subcomponents/SidebarProjectItem.tsx) were touched. No raw Tailwind colour classes added.Findings table
workingDirin/spawn-sub-agentwas unvalidated → authority escalation$HOME(orDISPATCH_PROJECT_ROOTS), rejects sensitive subdirsensureRecommendedMCPs()materializes~/.claude.jsonfrom nothingsetImmediate127.0.0.1DISPATCH_CHROME_VIEW_ENABLED+..._ALLOW_INPUTprojectName/sessionIdjoined to disk path with no traversal guard../ separators / null bytes; resolved-path prefix checkPreviewModal,BrowserModalshipped but never importedMainContent, gated onisMobile && activeTabSidebarProjectItemis no-editSessionActivityContextdelivers state without prop drilling;worktreeSessionMapis best-effort (cross-worktree resolution tracked in follow-ups)Review checklist (cross-cuts all four phases reviewed)
grepclean)docs/screenshots/phase-5/npm run buildsucceeds (bundle delta +0.1% vs main, well under 5%)npm test— no test script defined inpackage.json; noted in follow-upsnpm run typecheckclean)Files changed
server/routes/mcp-bootstrap.js— workingDir validation, side-effect guardserver/routes/preview-proxy.js— port allowlistserver/routes/chrome-screencast.js— operator scoping for view + inputserver/routes/tasks.js— path-traversal guardsrc/components/main-content/view/MainContent.tsx— mount mobile modalssrc/components/app/AppContent.tsx— provideSessionActivityContextsrc/components/sidebar/worktrees/WorktreeList.tsx— consume context as fallbacksrc/contexts/SessionActivityContext.tsx— newdocs/follow-ups.md— new (12 nice-to-haves, grouped by phase)Test plan
npm run typecheck— cleannpm run lint— cleannpm run build— clean (2533 KB main chunk; +3 KB vs pre-fix, +0.1%)npm run devonce merged: confirm preview iframe still loads, chrome screencast cleanly returns 503/disabled, mobile preview/browser open as fullscreen modals.🤖 Generated with Claude Code