Close three escapes in the Claude tool gate - #8
Merged
Conversation
Each is reproduced by a test that fails without the corresponding fix. 1. A `..` segment after a symlink smuggled an edit out of the worktree. `resolvesOutsideWorktree` collapses the path with `path.resolve` before it probes the filesystem, so `sub/esc/../stolen` (where `sub/esc` is a committed symlink) becomes `sub/stolen` — the symlink is erased from the probe and the write reads as in-tree, while POSIX resolves left to right and lands it in the symlink target's parent. The shell surface already refuses `..` outright via PARENT_TRAVERSAL; the edit surface applied no `..` screen at all. Apply the same coarse screen there under a strict scope or a task context. 2. The engine-owned `.weft/tasks` guard tested the RAW tool argument, so any non-canonical spelling of the same file (`.weft/foo/../tasks/x`, `.weft//tasks/x`) missed the regex and then normalized straight back onto the store two lines later. Screen the normalized `workspacePath` as well. 3. An absolute destination ATTACHED to its redirection (`>/etc/x`, `2>>/etc/x`) escaped both strict-scope screens: `OUT_OF_TREE_PATH` required the `/` to follow start-of-string, whitespace, `=` or a quote, and `commandEscapesWorktree` strips the redirection operator and then skips any token starting with `/`. The command reached the approval broker instead of being denied up front, so an auto-approving policy was the only thing left. Add the shell's own operators to the leading class; `:` stays out so `curl http://host/p` still passes.
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
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.
Three ways an agent step could write outside the boundary
createToolGateis supposed to enforce. Each fix is paired with a test that fails without it (verified by revertinggate.tsalone: exactly three failures, zero with the fix).Found while evaluating Weft — a
reviewrun over its own sources surfaced the first two; the third came out of reading the surrounding screens.1. A
..segment after a symlink smuggled an edit out of the worktreeresolvesOutsideWorktreecollapses the path withpath.resolvebefore it probes the filesystem. Withsub/esca committed symlink to an outside directory,sub/esc/../stolenbecomessub/stolen— the symlink is erased from the probe,realpathon the surviving ancestor reports in-tree, and the function returnsfalse. POSIX resolves left to right, so the write actually lands in the symlink target's parent, invisible to patch capture.The shell surface already refuses
..outright viaPARENT_TRAVERSAL, with a comment explaining that a false deny beats an unquarantined write. The edit surface applied no..screen at all. This applies the same coarse screen there, under a strict scope or a task context.2. The
.weft/tasksguard was tested against the raw tool argumentThe regex ran on
target(the argument verbatim, pereditTargetPath), so.weft/foo/../tasks/x.jsonand.weft//tasks/x.jsonmissed it — and thenworkspacePathnormalized them straight back onto the store two lines later. A task-aware agent could edit the engine-owned store directly, bypassing thetaskOperationssettlement channel.Now the normalized path is screened as well. The test asserts three spellings of the same file, all denied.
3. An absolute destination attached to its redirection escaped both strict-scope screens
printf x >/etc/cron.d/pwnpassed everything:OUT_OF_TREE_PATHrequires the/to follow start-of-string, whitespace,=or a quote —>is not in the class;commandEscapesWorktreestrips the redirection operator and thencontinues on any token starting with/.So the command was never denied up front as the comment above it documents, and instead reached
hitl.onPermission, where an auto-approving policy — the case that comment anticipates — is the only thing left. Defense-in-depth miss rather than a silent allow, but the strict scope's stated guarantee was gone.Fixed by adding the shell's own operators to the leading class.
:is deliberately left out so ordinary URLs (curl http://host/p) still pass; the test pins that.Verification
vitest run— 923 passed, 1 skipped, 53 filespnpm typecheck— cleanbiome checkon both changed files — cleanpnpm lintis red onmainfor seven pre-existing findings inapps/ui,packages/design-system,packages/gateandpackages/host; untouched here to keep the diff to the boundary.