fix: path containment hardening for worktrees, fleet names, read guards and capture paths - #6678
9 commits merged into
Conversation
…orktree add Cause: resolve_worktree_path applied the per-repo .codewhale-worktrees/<repo>/ containment only to relative worktree_path values. A read-only role start that requested a worktree did not show the approval card. The base ref was passed to `git worktree add` without a `--` separator. Fix: absolute and relative worktree paths get the same containment, checked lexically and again after resolving the nearest existing ancestor so a symlink under the root cannot redirect the checkout. Any worktree request keeps the approval card. provision_worktree refuses a branch or base that starts with '-' and passes path and base after `--`; the sub-agent path also rejects such a base up front as invalid input. Existing tests that passed absolute temp paths now use paths under the worktree root. The blocking-call budget for worktree.rs rises by one: the added canonicalize runs in the same synchronous spawn path as the existing git and canonicalize calls. Tests: new create_isolated_worktree_keeps_absolute_paths_under_worktree_root, create_isolated_worktree_rejects_option_shaped_base_ref, approval cases in write_capable_or_unproven_starts_keep_the_approval_gate, and lane provision_refuses_option_shaped_base_ref / provision_accepts_a_path_that_looks_like_an_option. All fail with the fix reverted (lane: 2 failed; tui: 3 failed). - cargo test -p codewhale-lane --lib worktree: test result: ok. 10 passed; 0 failed - cargo test -p codewhale-tui --lib (worktree, approval, fleet::exact, pandoc, image_ocr, tools::file, plugins::builtin filters): test result: ok. 303 passed; 0 failed; 1 ignored Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Cause: FleetDocument::load_by_name, load_named_fleet, ReasoningRouterProfile::load_by_name and the TUI's load_fleet_document built `<root>/fleets/<name>.toml` from the part after the first '/' without checking that it was a single file name. Fix: one validator, validate_fleet_file_stem, in the workflow crate: the name must be a single normal path component with no separators, drive prefix, NUL or `..`. split_qualified_fleet_name now validates and is shared with the TUI loader, so every lookup (qualified origin, other-forms probe, NotFound fallback) runs after the check. load_named_fleet and the router loader call the same validator. The new InvalidName error carries no text from the name. Tests: fleet_names_cannot_leave_the_fleets_directory, router_names_cannot_leave_the_router_directory (workflow) and fleet_names_that_leave_the_fleets_directory_are_refused (tui). With the validator disabled they fail. - cargo test -p codewhale-workflow --lib -- named_fleet reasoning_router: test result: ok. 24 passed; 0 failed - tui fleet::exact is inside the focused run: test result: ok. 303 passed; 0 failed; 1 ignored Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Cause: pandoc_convert and image_ocr resolved the model's path without the read deny-list or the credential-store check that `read` applies, and pandoc ran without --sandbox. Fix: move read's guard sequence (deny-list on the raw spelling, resolve, credential check, deny-list on the resolved path) into one helper, file::resolve_guarded_read_path, used by read, read_file, pandoc_convert and image_ocr. pandoc always runs with `--sandbox`; a pandoc too old for the flag fails the call instead of running without it. Tests: pandoc_convert_refuses_deny_listed_sources, pandoc_convert_does_not_follow_include_directives and image_ocr_refuses_deny_listed_paths (direct and via a workspace symlink). All three fail with the fix reverted. - cargo test -p codewhale-tui --lib (focused filters incl. tools::file): test result: ok. 303 passed; 0 failed; 1 ignored Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Cause: screenshot and zoom accepted a caller-supplied `path` without a directory check, and zoom accepted an undeclared `source` argument that the server forwarded to the backend. Fix: one shared src/recordings.mjs owns recordingsDir() and recordingsOutputPath(): an explicit path must be an absolute .png/.jpg/.jpeg file inside the recordings directory, must not be a symlink, and its existing parent must not resolve outside the directory (checked before any directory is created). darwin, linux, win32 and harmonyos use it for screenshot and zoom and drop their private recordingsDir copies. Every zoom crops the backend's last raster; `source` is gone from the backends and the server strips it from screenshot/zoom arguments. The new module is added to the embedded bundle list. Existing capture tests now point CODEWHALE_CU_RECORDINGS_DIR at their temp dir. Tests: tests/recordings-path.test.mjs (inside/outside, `..`, non-image extensions, symlinked subdirectory and file) and a server test that screenshot/zoom never forward `source` (fails without the server change; the fake backend now records full arguments). - (cd crates/tui/plugins/computer-use && npm test): tests 388, pass 372, fail 0 - cargo test -p codewhale-tui --lib computer_use_embed_list: ok (in the focused run: test result: ok. 303 passed; 0 failed; 1 ignored) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Security bullets under [Unreleased] for the worktree, fleet name, read guard and Computer Use capture path changes; crates/tui/CHANGELOG.md regenerated with scripts/sync-changelog.sh (--check: up to date). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ories Cause: sync_one_skill joined the registry index key straight onto the skill cache directory and later removed, renamed and wrote under it, while the installed-skill path already ran validate_skill_name_segment. Fix: run validate_skill_name_segment on the key first and report a Failed outcome for a key that is not one path-safe segment. Test: registry_sync_refuses_a_key_that_is_not_a_single_segment (`../escape`, `..`, `a/b`, `/abs`); fails with the check disabled (1 failed). - cargo test -p codewhale-tui --lib -- skills::install: test result: ok. 21 passed; 0 failed Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95c01b493f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let stat = null; | ||
| try { stat = fs.lstatSync(resolved); } catch { /* does not exist yet */ } | ||
| if (stat?.isSymbolicLink()) throw badPath("output path must not be a symlink"); |
There was a problem hiding this comment.
Reject hard-linked capture targets
When an existing path inside the recordings directory is a hard link to a writable file outside it, lstatSync reports a regular file, so this check accepts it and the screenshot/zoom backend overwrites the external inode. This defeats the new containment boundary and can corrupt user files without requiring a symlink race; reject multiply linked targets or write to a new no-follow temporary file and atomically replace the directory entry.
Useful? React with 👍 / 👎.
| function inside(dir, file) { | ||
| const rel = path.relative(dir, file); | ||
| return rel !== "" && !rel.startsWith("..") && !path.isAbsolute(rel); |
There was a problem hiding this comment.
Permit in-root names beginning with two dots
For a valid path such as <recordings>/..hidden.png or <recordings>/..hidden/a.png, path.relative returns a string beginning with .., so this predicate rejects it even though it does not contain a parent-directory component and remains inside the recordings directory. Check for rel === ".." or a ..${path.sep} prefix instead of rejecting every name whose first two characters are dots.
Useful? React with 👍 / 👎.
| fn worktree_path_within_root(candidate: &Path, root: &Path) -> bool { | ||
| candidate.starts_with(root) | ||
| && canonicalize_existing_prefix(candidate).starts_with(canonicalize_existing_prefix(root)) |
There was a problem hiding this comment.
Anchor containment before resolving the worktree root
If .codewhale-worktrees/<repo> itself, or its .codewhale-worktrees parent, is a symlink to an outside directory, both sides of this comparison canonicalize through the same symlink, so the check returns true and git worktree add creates the checkout outside the intended root. Validate the root component against its canonical repository parent before using its canonical form as the containment boundary.
Useful? React with 👍 / 👎.
| let image_path = | ||
| crate::tools::file::resolve_guarded_read_path(context, path_str, "image_ocr")?; |
There was a problem hiding this comment.
Move the added read guards onto the blocking pool
On a slow or unavailable filesystem, this newly expanded guard runs synchronous canonicalization and credential/read-deny checks inline before image_ocr enters its existing spawn_blocking section, so a supposedly parallel tool call can stall a Tokio worker and the UI. Resolve and guard the path inside spawn_blocking (and do the same for the new pandoc_convert call site) rather than invoking this synchronous helper directly from execute.
AGENTS.md reference: AGENTS.md:L164-L170
Useful? React with 👍 / 👎.
| if (typeof file !== "string" || !path.isAbsolute(file) || file.includes("\0")) { | ||
| throw badPath("output path must be an absolute filename inside the recordings directory"); | ||
| } | ||
| if (!/\.(png|jpe?g)$/i.test(file)) throw badPath("output path must end in .png, .jpg or .jpeg"); |
There was a problem hiding this comment.
Match accepted capture extensions to backend encoders
The shared validator accepts all three extensions for every backend, but several backends always emit a fixed format: HarmonyOS always pulls a JPEG even for a .png path, while Windows screenshots and Windows/macOS zooms always save PNG even for .jpg or .jpeg. Those accepted requests therefore produce files whose bytes contradict their extension, breaking consumers that select a decoder or content type from the filename; either convert according to the requested suffix or restrict each backend to the formats it actually writes.
Useful? React with 👍 / 👎.
…in a test Cause: pandoc_convert always passes --sandbox, which pandoc only knows from 2.15. Distro packages such as Ubuntu 22.04's 2.9.2.1 then failed every call with pandoc's own "Unknown option --sandbox", and the doctor hint pointed Linux users at exactly those packages. The only test covering the flag needed pandoc installed and passed on any pandoc error, so CI without pandoc could not notice the flag going missing. The new resolve_guarded_read_path doc had also been placed between enforce_read_denylist's doc and its fn. Fix: build the argument list in pandoc_args() (--sandbox first, always) and probe `pandoc --version` once per process; below 2.15 the call fails with "pandoc 2.15 or newer is required (found X.Y)" and an upgrade link instead of running without the flag. The tool error and `codewhale doctor` hints now name 2.15 and the pandoc.org release package. Doc comments in file.rs are reattached to resolve_guarded_read_path, enforce_read_denylist and is_codewhale_credential_path. CHANGELOG notes the 2.15 requirement. Tests: pandoc_args_always_include_sandbox (no pandoc needed; fails with the flag removed: test result: FAILED. 0 passed; 1 failed) and pandoc_version_gate_matches_sandbox_release. - CARGO_BUILD_JOBS=4 cargo test -p codewhale-tui --lib -- tools::pandoc tools::file: test result: ok. 189 passed; 0 failed; 0 ignored - cargo fmt --all -- --check: clean; scripts/sync-changelog.sh --check: up to date - scripts/check-blocking-calls-budget.py: within budget Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… path tests Cause: browser-cdp.mjs and trajectory.mjs kept their own recordings directory (under the state dir) while the desktop backends used recordings.mjs (under the home dir), so captures could land in two places when CODEWHALE_CU_STATE_DIR was set. Only recordingsOutputPath itself was tested; no test drove a backend's screenshot or zoom with an outside path, so a backend falling back to the raw path would not have been caught. zoom checked for a previous raster before the output path. Fix: recordingsDir() in recordings.mjs is the only definition (default stateDir()/recordings, which is ~/.codewhale-cu/recordings unless the state dir is moved) and browser-cdp and trajectory import it. darwin, linux and win32 zoom validate the caller's output path before anything else runs. Tests: per-backend cases in tests/recordings-path.test.mjs for darwin, linux, win32 and harmonyos: screenshot and zoom with a path outside the recordings directory or a non-image extension fail with bad_args, write nothing and run no command. With the backends' recordingsOutputPath calls replaced by the raw path: 4 failed; with only zoom's call replaced: 3 failed. - node --test tests/recordings-path.test.mjs: tests 9, pass 9, fail 0 - (cd crates/tui/plugins/computer-use && npm test): tests 392, pass 376, fail 0, skipped 16 - scripts/check-bundled-plugin-claims.py: match Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
95c01b4 to
7eddbe5
Compare
The Windows test job failed tools::subagent::tests::create_isolated_worktree_keeps_absolute_paths_under_worktree_root: the worktree root comes from `git rev-parse --show-toplevel` (C:/...), while a canonicalized request carries the \\?\ verbatim prefix, so Path::starts_with saw different prefix components and refused a legitimate path under the root. The same mismatch let the "inside the parent checkout" guard silently never fire on Windows. resolve_worktree_path now takes the part of the request below the root via relative_to_root (strip_prefix off Windows; on Windows a separator-, verbatim-prefix- and ASCII-case-insensitive comparison that must end on a component boundary and refuses `..`), rebases it onto the root as git reported it, and keeps the existing lexical + symlink-resolved containment check. The parent-checkout guard uses the same comparison. New unit tests exercise Windows-shaped inputs on every platform. Verified (macOS): cargo test -p codewhale-tui --lib -- tools::subagent::worktree tools::subagent::tests::create_isolated_worktree tools::subagent::tests::unchanged_isolated_worktree -> test result: ok. 11 passed; 0 failed. Windows run is CI's to confirm. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| const outPath = recordingsOutputPath(args.path); | ||
| await probeSession(); | ||
| const dir = recordingsDir(); | ||
| fs.mkdirSync(dir, { recursive: true }); | ||
| const file = outPath || path.join(dir, `shot-${new Date().toISOString().replace(/[:.]/g, "-")}-${crypto.randomBytes(3).toString("hex")}.png`); | ||
| const file = outPath ?? path.join(dir, `shot-${new Date().toISOString().replace(/[:.]/g, "-")}-${crypto.randomBytes(3).toString("hex")}.png`); |
There was a problem hiding this comment.
🔴 JPEG screenshots lose Linux raster geometry
When screenshot writes an explicit JPEG path on Linux, pngSize returns null for the captured JPEG. Full screenshots lose their geometry, so coordinate actions cannot target them.
Learn more
Linux screenshots use scrot, grim, or ImageMagick import through takeShot. The new path validator accepts JPEG suffixes, while pngSize only extracts dimensions from PNG files. When the capture utility writes a JPEG for the requested suffix, dims is null. A full screenshot then reports null points and pixels, preventing the server from binding usable raster geometry.
Example: With an X11 desktop, request screenshot({path: '/home/alice/.codewhale-cu/recordings/shot.jpg'}). scrot writes a JPEG, but the screenshot receipt has no pixel dimensions instead of the captured screen's dimensions.
Recommended fix: Either restrict Linux screenshot output to .png or add a JPEG dimension reader and use it when reporting lastRaster; test a real JPEG capture and subsequent coordinate mapping.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| function inside(dir, file) { | ||
| const rel = path.relative(dir, file); | ||
| return rel !== "" && !rel.startsWith("..") && !path.isAbsolute(rel); |
There was a problem hiding this comment.
🟡 Valid recordings subdirectories rejected
When recordingsOutputPath receives a file under a directory named ..drafts, inside rejects its relative path. The directory is inside recordings, but the capture fails with bad_args.
| return rel !== "" && !rel.startsWith("..") && !path.isAbsolute(rel); | |
| return rel !== "" && rel !== ".." && !rel.startsWith(`..${path.sep}`) && !path.isAbsolute(rel); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (Object.hasOwn(args, "app_ref") || Object.hasOwn(args, "window_id")) throw unsupportedSelector("Windows screenshot does not support app_ref or window_id; omit them for a desktop screenshot"); | ||
| const { display = activeDisplay, region, path: outPath } = args; | ||
| const { display = activeDisplay, region } = args; | ||
| const outPath = recordingsOutputPath(args.path); |
There was a problem hiding this comment.
🟡 JPEG-named captures contain PNG data
When screenshot or zoom receives a .jpg path on Windows, it saves PNG bytes under the JPEG name. macOS zoom does the same, so extension-based consumers can misread these captures.
Learn more
The shared output validator now accepts PNG and JPEG names. Windows screenshot encoding saves with ImageFormat.Png, and zoom encoding also saves PNG regardless of suffix. macOS zoom forces PNG with sips -s format png. The returned file name can therefore claim a JPEG while carrying PNG bytes.
Example: A caller requests zoom({region:[0,0,100,100],path:'.../recordings/crop.jpg'}) on Windows. The file is PNG-encoded but named crop.jpg, which can fail a JPEG-only consumer.
Recommended fix: Enforce .png for PNG-only operations, or select the encoder based on the accepted output extension and test the produced file signature.
Was this helpful? React with 👍 or 👎 to provide feedback.
| ### Security | ||
|
|
||
| - Sub-agent worktrees stay under the per-repo `.codewhale-worktrees/<repo>/` | ||
| root: an absolute `worktree_path` is now held to the same containment as a | ||
| relative one, with symlinks resolved before the check. Any start that asks | ||
| for a worktree keeps the approval card, even for a read-only role. A | ||
| `worktree_base` starting with `-` is refused, and `git worktree add` now | ||
| receives its path and base after `--`. | ||
| - Fleet and reasoning-router names must be plain file names (optionally | ||
| `origin/name`); a name with path separators or `..` is refused before any | ||
| file is looked up, through one shared check in the workflow crate. | ||
| - `pandoc_convert` and `image_ocr` apply the same read deny-list and | ||
| credential-store checks as `read`, through one shared helper, and pandoc | ||
| always runs with `--sandbox`. This needs pandoc 2.15 or newer; an older | ||
| pandoc gets an upgrade message instead of a conversion. | ||
| - Computer Use: screenshot and zoom output paths must be `.png`/`.jpg`/`.jpeg` | ||
| files inside the recordings directory, and zoom always crops the last | ||
| captured raster instead of a caller-named source file. | ||
| - Skill registry sync refuses an index key that is not a single path-safe | ||
| name before it is used as a cache directory, the same check an installed | ||
| skill name already gets. |
| let out = Command::new(pandoc) | ||
| .arg("--version") | ||
| .stdin(Stdio::null()) | ||
| .stderr(Stdio::null()) | ||
| .output() | ||
| .ok()?; | ||
| parse_pandoc_version(&String::from_utf8_lossy(&out.stdout)) |
There was a problem hiding this comment.
| fn require_sandbox_support(pandoc: &str) -> Result<(), ToolError> { | ||
| static VERSION: OnceLock<Option<(u32, u32)>> = OnceLock::new(); | ||
| let version = *VERSION.get_or_init(|| { | ||
| let out = Command::new(pandoc) | ||
| .arg("--version") | ||
| .stdin(Stdio::null()) | ||
| .stderr(Stdio::null()) | ||
| .output() | ||
| .ok()?; | ||
| parse_pandoc_version(&String::from_utf8_lossy(&out.stdout)) | ||
| }); |
There was a problem hiding this comment.
| fs.mkdirSync(dir, { recursive: true }); | ||
| const realDir = fs.realpathSync(dir); |
There was a problem hiding this comment.
🟥 Recordings directory can escape through a symlink
When the recordings directory itself is a symlink, recordingsOutputPath validates children against its resolved target. A caller can choose an output inside that target through the configured recordings path, even when the target is outside the intended directory.
Was this helpful? React with 👍 or 👎 to provide feedback.
…e/0.10.1 crates/tui/src/tools/verify.rs (run_git_diff): #6671 routed the verify diff through tools::git::read_only_git_command (Git::review_command: no fsmonitor, hooks or clean filters) with --no-ext-diff --no-textconv; #6679 routed it through Git::review_command directly with the new Git::REVIEW_DIFF_ARGS (the same two flags plus --submodule=short and --ignore-submodules=dirty). Kept #6671's helper and availability check and #6679's REVIEW_DIFF_ARGS, so the verify diff gets both. crates/tui/src/skills/install.rs: tests module; kept both new tests (#6678 registry key segment check, #6679 download cap). crates/tui/src/tools/shell.rs: same flush() added by both with different comments; kept one comment. tools/shell/tests.rs add-only kept both sides. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Path containment hardening for v0.10.1. Each commit is a separate change with its own regression tests.
Refs #6561
Changes
tools/subagent/worktree.rs,subagent/mod.rs,crates/lane/src/worktree.rs)worktree_pathvalues now get the same containment as relative ones: the path must be under.codewhale-worktrees/<repo>/. The check runs on the lexical path and again after resolving the nearest existing ancestor, so a symlink under the root can't redirect the checkout.provision_worktreerefuses a branch or base that starts with-and passes path and base after--. The sub-agent path also rejects such a base early as invalid input.worktree.rsgoes up by one. The added canonicalize runs in the same synchronous spawn path as the existing git and canonicalize calls.crates/workflow,tui/src/fleet/exact.rs)validate_fleet_file_stemaccepts only a single normal path component: no separators, drive prefix, NUL or...split_qualified_fleet_namenow validates too and is shared with the TUI loader.load_named_fleetandReasoningRouterProfile::load_by_nameuse the same validator.InvalidNameerror doesn't echo any of the rejected name.pandoc_convertandimage_ocr(tools/file.rs,pandoc.rs,image_ocr.rs)readis now one helper,resolve_guarded_read_path: deny-list on the raw spelling, then resolve, then the credential check, then the deny-list on the resolved path.read,read_file,pandoc_convertandimage_ocrall use that helper.--sandbox, built bypandoc_args()and pinned by a unit test that needs no pandoc. The flag needs pandoc 2.15 or newer: an older pandoc gets "pandoc 2.15 or newer is required (found X.Y)" and an upgrade link, and the tool andcodewhale doctorinstall hints now name 2.15 and the pandoc.org package.crates/tui/plugins/computer-use)src/recordings.mjsholdsrecordingsDir()andrecordingsOutputPath(). An explicit output path must be an absolute.png/.jpg/.jpegfile inside the recordings directory, must not be a symlink, and its existing parent must resolve inside that directory. This is checked before any directory is created.browser-cdp.mjsandtrajectory.mjsnow use the samerecordingsDir(), so it is the only definition (defaultstateDir()/recordings).bad_args, write nothing and run no command.sourceargument is removed from the backends and stripped by the server.skills/install.rs):sync_one_skillnow runsvalidate_skill_name_segmenton the registry key before using it as a cache directory.Verification
Each new Rust regression test was run with its fix reverted and failed. The server-side
sourcetest also failed without the server change.cargo test -p codewhale-lane --lib worktree:test result: ok. 10 passed; 0 failed(with the fix reverted, 2 failed)cargo test -p codewhale-workflow --lib -- named_fleet reasoning_router:test result: ok. 24 passed; 0 failed(with the fix reverted, 2 failed)cargo test -p codewhale-tui --lib -- worktree read_only_role_starts write_capable_or_unproven_starts fleet::exact tools::pandoc tools::image_ocr tools::file plugins::builtin:test result: ok. 303 passed; 0 failed; 1 ignored. With the fixes reverted, the 7 new or extended tests: 7 failed.cargo test -p codewhale-tui --lib -- skills::install:test result: ok. 21 passed; 0 failed(with the fix reverted, 1 failed)(cd crates/tui/plugins/computer-use && npm test): tests 392, pass 376, fail 0, skipped 16. With the backends' path check replaced by the raw path, the 4 per-backend tests fail; with only zoom's check replaced, 3 fail.cargo test -p codewhale-tui --lib -- tools::pandoc tools::file:test result: ok. 189 passed; 0 failed. With--sandboxremoved frompandoc_args,pandoc_args_always_include_sandboxfails (0 passed; 1 failed).cargo clippy -p codewhale-workflow -p codewhale-lane --all-targets -- -D warnings: cleancargo fmt --all -- --check,scripts/sync-changelog.sh --check,check-blocking-calls-budget.pyandcheck-bundled-plugin-claims.pyall passI didn't run the full workspace suite or clippy on
codewhale-tuilocally because this machine is memory-limited. CI covers both.🤖 Generated with Claude Code