Close three escapes in the Claude tool gate

packages/provider-claude/src/gate.ts

@@ -23,8 +23,14 @@ import {
 /** Sent back to the agent for every denial on a read-only step, edits and shell writes alike. */
 export const READ_ONLY_MESSAGE = "this is a read-only step";
 
-/** An absolute or home-anchored path anywhere in a command (except /dev/null). */
-const OUT_OF_TREE_PATH = /(?:^|[\s='"`])(?:\/(?!dev\/null\b)|~\/|\$HOME\b)/;
+/**
+ * An absolute or home-anchored path anywhere in a command (except /dev/null).
+ * The leading class must include the shell's own operators, not just whitespace:
+ * a destination can be ATTACHED to its redirection (`>/etc/x`, `2>>/etc/x`) or
+ * follow a pipe or separator, and those spellings are the same write as the
+ * spaced form. `:` stays out so ordinary URLs (`curl http://host/p`) do not trip it.
+ */
+const OUT_OF_TREE_PATH = /(?:^|[\s='"`><|;&(])(?:\/(?!dev\/null\b)|~\/|\$HOME\b)/;
 
 /**
  * A `..` path segment anywhere in a command: relative traversal climbs out of the
@@ -105,6 +111,18 @@ export interface ToolGateOptions {
   onEdit: (path: string) => void;
 }
 
+/** The engine-owned workflow task store, in either separator spelling. */
+const TASK_STORE_PATH = /(?:^|[/\\])\.weft[/\\]tasks(?:[/\\]|$)/;
+
+function isTaskStorePath(value: string): boolean {
+  return TASK_STORE_PATH.test(value);
+}
+
+/** A `..` path SEGMENT (not a `..` inside a name, and not a `HEAD..main` range). */
+function hasParentSegment(value: string): boolean {
+  return value.split(/[/\\]/).includes("..");
+}
+
 /**
  * Resolve a tool's path argument against the step cwd and normalize it to a
  * posix-relative path — the form write-scope globs are written in. Paths outside
@@ -139,7 +157,7 @@ export function createToolGate({ req, onEdit }: ToolGateOptions): CanUseTool {
     if (EDIT_TOOLS.has(base)) {
       if (!allowEdits) return deny(READ_ONLY_MESSAGE);
       const target = editTargetPath(input);
-      if (target !== undefined && /(?:^|[/\\])\.weft[/\\]tasks(?:[/\\]|$)/.test(target)) {
+      if (target !== undefined && isTaskStorePath(target)) {
         return deny("workflow tasks are engine-owned; return taskOperations instead of editing the store");
       }
       if (target === undefined) {
@@ -161,6 +179,20 @@ export function createToolGate({ req, onEdit }: ToolGateOptions): CanUseTool {
         return allow;
       }
       const path = workspacePath(req.cwd, target);
+      // The guard above sees the RAW argument; a non-canonical spelling of the same
+      // file (`.weft/foo/../tasks/x`, `.weft//tasks/x`) misses it and normalizes back
+      // onto the store here. Screen the normalized path too.
+      if (isTaskStorePath(path)) {
+        return deny("workflow tasks are engine-owned; return taskOperations instead of editing the store");
+      }
+      // `resolvesOutsideWorktree` collapses `..` LEXICALLY (`path.resolve`), so a `..`
+      // placed after a symlink (`sub/esc/../out`) erases the symlink from the probe and
+      // the escape reads as in-tree. The shell surface already refuses `..` outright via
+      // PARENT_TRAVERSAL; the edit surface needs the same coarse screen for the same
+      // reason — a false deny beats an unquarantined write.
+      if ((req.taskContext || scope?.mode === "strict") && hasParentSegment(target)) {
+        return deny(`${path} contains a ".." segment, which cannot be resolved safely`);
+      }
       // A task-aware agent's edit capability is confined to the worktree even
       // under a WARN scope. Otherwise a lexically in-scope path can traverse a
       // committed symlink into the integration checkout's .weft/tasks store and

packages/provider-claude/test/claude.test.ts

@@ -926,6 +926,95 @@ describe("the tool gate", () => {
     }
   });
 
+  test("a `..` segment after a symlink cannot smuggle an edit out of the worktree", async () => {
+    const cwd = await mkdtemp(join(tmpdir(), "weft-gate-"));
+    const outside = await mkdtemp(join(tmpdir(), "weft-outside-"));
+    await mkdir(join(cwd, "sub"));
+    await symlink(outside, join(cwd, "sub", "esc"), "dir");
+    try {
+      const options = await gateContext(
+        request({ cwd, tools: { allowEdits: true }, writeScope: { paths: ["**"], mode: "strict" } }),
+      );
+      // Deliberately NOT join(): join() collapses the `..` lexically, which is exactly
+      // what resolvesOutsideWorktree used to do — erasing the symlink from the probe.
+      // POSIX resolves left to right, so this lands in the OUTSIDE directory's parent.
+      const edit = await ask(options, "Edit", {
+        file_path: `${cwd}/sub/esc/../stolen`,
+        old_string: "a",
+        new_string: "b",
+      });
+      expect(edit.behavior).toBe("deny");
+      // The traversal-free spelling of an in-tree file stays allowed.
+      expect(
+        (await ask(options, "Edit", { file_path: `${cwd}/sub/notes.txt`, old_string: "a", new_string: "b" }))
+          .behavior,
+      ).toBe("allow");
+    } finally {
+      await rm(cwd, { recursive: true, force: true, maxRetries: 5, retryDelay: 50 });
+      await rm(outside, { recursive: true, force: true, maxRetries: 5, retryDelay: 50 });
+    }
+  });
+
+  test("the engine-owned task store is screened on the normalized path, not the raw argument", async () => {
+    const options = await gateContext(
+      request({
+        tools: { allowEdits: true },
+        writeScope: { paths: ["**"], mode: "warn" },
+        protectedPaths: [`${CWD}/.weft/tasks`],
+        taskContext: {
+          workflowId: "review",
+          workflowName: "review",
+          runId: "run-1",
+          step: "review",
+          provider: "claude",
+          mode: "write",
+        },
+      }),
+    );
+    // Every spelling of the same file is the same write.
+    for (const spelling of [
+      `${CWD}/.weft/tasks/review/task-deadbeef.json`,
+      `${CWD}/.weft/foo/../tasks/review/task-deadbeef.json`,
+      `${CWD}/.weft//tasks/review/task-deadbeef.json`,
+    ]) {
+      expect(await ask(options, "Edit", { file_path: spelling }), spelling).toMatchObject({
+        behavior: "deny",
+      });
+    }
+  });
+
+  test("a strict scope denies an absolute destination ATTACHED to its redirection", async () => {
+    const brokered: PermissionRequest[] = [];
+    const options = await gateContext(
+      request({
+        tools: { allowEdits: true },
+        writeScope: { paths: ["**"], mode: "strict" },
+        hitl: {
+          onPermission: async (r: PermissionRequest) => {
+            brokered.push(r);
+            return { behavior: "allow" } as PermissionDecision;
+          },
+          onAsk: async () => ({}),
+        },
+      }),
+    );
+    // The spaced form was already denied; the attached forms are the same write.
+    for (const command of [
+      "printf x > /etc/cron.d/pwn",
+      "printf x >/etc/cron.d/pwn",
+      "printf x >>/etc/cron.d/pwn",
+      "printf x 2>/etc/cron.d/pwn",
+    ]) {
+      expect((await ask(options, "Bash", { command })).behavior, command).toBe("deny");
+    }
+    // Denied up front, never handed to the approval broker.
+    expect(brokered).toHaveLength(0);
+    // A URL is not a filesystem destination and must still pass the screen.
+    expect((await ask(options, "Bash", { command: "curl -s http://example.com/p" })).behavior).not.toBe(
+      "deny",
+    );
+  });
+
   test("tools.deny removes a tool outright", async () => {
     const options = await gateContext(request({ tools: { allowEdits: true, deny: ["WebFetch"] } }));
     const denial = await ask(options, "WebFetch", { url: "https://example.com" });