Harden path-escape sanitization for nested and alternate path keys

[TheGreatAxios]

Summary

• path-escape now deep-walks nested objects and arrays under path-like keys, so escapes such as {options:{path:../secret}} can no longer pass through at execution time.
• looksLikePath matches case- and separator-insensitively and covers alternate spellings (filepath, filePath, FILE_PATH, paths, filenames, *path(s)); pathEscapeBlockReason walks identically, so authorize-time and execution-time agree.

Policy decision

• Documented allowlist plus targeted deep-walk, not fail-closed on unknown keys. Unknown MCP/custom keys pass through by design: their values are only interpreted by their own server, so they are that server's contract, not this sandbox's. The explicit key list and contract are documented on looksLikePath in src/plugins/path-escape-plugin.ts.

Parallel stack note

• Shares src/plugins/path-escape-plugin.ts with CL-6727 in parallel; kept to a minimal two-commit diff (red tests, green fix) for an easy rebase.

Verification

• New red tests (nested object, nested in-bounds resolve, filepath, string-array paths, blockReason parity) failed 5/18 before the fix and pass after: bun test src/plugins/path-escape-plugin.test.ts -> 18 pass, 0 fail.
• Neighbors: delete-file-plugin, secret-guard-symlink, reactor-authorize -> 33 pass; permission.test.ts -> 218 pass.
• bun run check -> exit 0 (7308 pass, 0 fail across 500 files).
• Note: the worktree needed bun install --frozen-lockfile first; without it tsc failed on a pre-existing missing semver dep in vendor/intx-types (untouched by this change).

Fixes CL-6730

[linear-code]

CL-6730

[TheGreatAxios]

Critic · Comment

Deep-walks nested objects/arrays under path-like keys and aligns pathEscapeBlockReason with execution time. Verified green in the worktree (bun test src/plugins/path-escape-plugin.test.ts -> 18 pass, 0 fail).

Findings

• src/plugins/path-escape-plugin.ts (escapeValue/blockReasonFor) — authorize-time vs execution-time parity holds: gate calls the same exported pathEscapeBlockReason, and both walkers recurse identically (object keys re-evaluated, array entries inherit the key). No bypass direction found.
• src/plugins/path-escape-plugin.ts (looksLikePath) — key list is explicit and the unknown-key passthrough policy is documented on the function. Contract signatures unchanged (sync, same params); no drift for existing callers.
• src/permission/gate.ts:480 (identityArguments) — still top-level-only while execution now deep-resolves nested in-bounds paths. Fail-safe direction only (stale identity causes re-decide/re-prompt, never bypass; same function both sides of the same-callId match), so not blocking, but worth a follow-up to deep-walk it for consistency.
• Overlap risk (not blocking): CL-6727 rewrites the same escapeArgs/pathEscapeBlockReason hunks (adds toolName param). Whoever lands second owns a manual reconcile; this branch stays minimal and rebase-friendly.
• src/plugins/path-escape-plugin.ts — escapeValue and blockReasonFor are parallel recursive walks that must stay in sync by hand. File-for-later: consider one walk returning both verdict and sanitized args.

Notes

• Probe-verified (temp tests, since removed): nested non-path keys pass through ({options:{command:"../secret.txt"}} untouched, blockReason undefined), FILE_PATH/file-path/outputPaths match, 3-level nesting blocked. Recommend keepers for the suite: nested non-path passthrough (locks the allowlist policy) and a case/separator-insensitivity test — the PR tests filepath but not FILE_PATH/file-path.
• Out of scope but adjacent: secret-guard-plugin.ts:282 and evidence-archive-path-guard.ts:30 still scan top-level string args only, so nested sensitive paths remain their pre-existing gap. This PR does not widen it; flagging for a follow-up, not this diff.
• No blocking defects. No commits made from this review.

[TheGreatAxios]

[warden trust review] · Comment

Deep-walk closes the nested/array path-escape bypass; authorize-time and execution-time agree. Two should-fix items, no blocking trust defect.

Findings

• src/plugins/path-escape-plugin.ts (escapeValue / blockReasonFor, new in this PR) — nested/array escape is genuinely closed. Both functions recurse with identical key propagation (innermost key wins; array entries inherit the array key), and escapeArgs runs the deny check before the rewrite, so read_file {options:{path:"../secret.txt"}}, {filepath:"../secret.txt"}, and {paths:["src/index.ts","../secret.txt"]} all deny at authorize (decide, gate.ts:670) and at execution. No TOCTOU between the two: same predicate, same walk.
• src/permission/gate.ts:480 (identityArguments, untouched by this PR) — should-fix: still top-level-only while the plugin now rewrites nested keys. Sequence: authorize read_file {options:{path:"src/index.ts"}} caches identity with the nested relative path intact; the plugin rewrites it to /project/src/index.ts; executionVerdict (gate.ts:994) misses the cache and re-decides. Fail-closed (re-decide re-runs the deep deny), but it breaks the invariant stated in the comment above identityArguments and risks double-prompt / grant-subject skew on nested in-bounds paths. Fix: recurse identityArguments with escapeValue semantics (route to builder).
• src/plugins/path-escape-plugin.ts (looksLikePath, new normalized endsWith("path")) — should-fix: case-insensitive suffix now catches query-language keys (xpath, jsonpath, classpath), which the old case-sensitive endsWith("Path") ignored. Trigger: MCP/custom tool call {xpath:"../../title"} denies as a path escape and in-bounds cases get silently rewritten to workspace-absolute garbage, corrupting the foreign contract. Fail-closed, but over-blocks MCP/custom tools. Fix: carve out known non-path suffixes or require a stronger path signal (route to builder).
• src/plugins/path-escape-plugin.ts (escapeArgs deny-before-rewrite ordering) — genuinely fine w.r.t. __proto__ smuggling: Object.entries still enumerates an own __proto__ key from parsed JSON, so the deny check sees through it before the rewrite assignment can drop it. Proto-assignment hygiene in the rewrite is file-for-later at most.

Gaps (not covered)

• callTargetsRestricted / grant-subject extraction for nested keys (whether a nested in-bounds restricted write under-matches to allow instead of ask) — needs a critic/testsmith pass; this PR neither fixes nor worsens it.
• resolveWorkspacePath symlink/alias behavior underneath the new walk — assumed unchanged; not re-audited here.

Permanent tests to keep (route to testsmith, do not implement here)

• Nested escape deny at both pathEscapeBlockReason and middleware; filepath spelling; mixed array ["in-bounds","../escape"].
• identityArguments parity: nested in-bounds call authorizes once and executes without re-decide.
• xpath/jsonpath passthrough: unknown query-language keys untouched by deny and rewrite.

[TheGreatAxios]

Reply to Critic (5656124515) — keepers landed in 3d61cd1:

• Nested non-path passthrough keeper: {options:{command:"../secret.txt"}} passes through untouched at execution and pathEscapeBlockReason returns undefined (locks the allowlist policy).
• Case/separator-insensitivity keeper: FILE_PATH and file-path escapes deny at both execution and authorize time.
• Warden should-fix items also in this commit: identityArguments now deep-walks via a new non-throwing normalizePathArguments export (same key propagation as escapeValue; escapes pass through so both cache sides still agree fail-closed), with an authorize-once/execute-without-re-decide gate test; looksLikePath carves out xpath/xpaths, jsonpath/jsonpaths, classpath/classpaths with passthrough tests covering escape-looking and in-bounds values. __proto__ deny-before-rewrite ordering untouched.
• Left alone per your note: parallel-walk sync refactor and the secret-guard/evidence-archive top-level gaps.

Verification: bun test src/plugins/ + src/permission/ green (507 permission tests), full bun run check exit 0 (7313 pass, 0 fail). Not merging.

[TheGreatAxios]

Reply to Warden (5656125984) — both should-fix items landed in 3d61cd1:

1. identityArguments (gate.ts) now delegates to a new exported normalizePathArguments with escapeValue key semantics (innermost key wins, arrays inherit), non-throwing: in-bounds paths resolve absolute, escapes pass through so both cache sides agree fail-closed. Regression test seeds a grant after authorize and asserts execution reuses the cached ask instead of re-deciding to allow — verified it fails pre-fix and passes post-fix.
2. looksLikePath carves out xpath(s), jsonpath(s), classpath(s) (note: jsonpath needed its own entry — json+path never matched the xpath suffix). Tests assert escape-looking values are not denied and in-bounds values are not rewritten. __proto__ deny-before-rewrite ordering intact.

• Gaps acknowledged, not fixed here: callTargetsRestricted nested extraction and resolveWorkspacePath symlink behavior — leaving for critic/testsmith follow-ups.
• Fail-closed on unknown keys unchanged (allowlist + deep-walk policy stays).

Verification: plugin + permission suites green, full bun run check exit 0 (7313 pass, 0 fail). Not merging.

[TheGreatAxios]

Critic · Confirm

Fix commit 3d61cd1 resolves both warden should-fix items and the critic keeper requests. Verified in the worktree.

Green

• src/plugins/path-escape-plugin.ts (normalizePathArguments) — mirrors escapeValue key propagation exactly (innermost key wins, array entries inherit the array key), non-throwing: in-bounds resolves absolute, escapes pass through so both cache sides agree fail-closed. src/permission/gate.ts (identityArguments) delegates to it.
• src/plugins/path-escape-plugin.ts (looksLikePath) — xpath(s), jsonpath(s), classpath(s) carve-outs sit before the endsWith("path"/"paths") check, so all six spellings are covered.
• Keepers landed: nested non-path passthrough ({options:{command:"../secret.txt"}} untouched, blockReason undefined), FILE_PATH/file-path case/separator insensitivity, xpath/jsonpath/classpath passthrough, normalizePathArguments identity, gate authorize-once/execute-without-re-decide.
• Tests: bun test src/plugins/path-escape-plugin.test.ts -> 22 pass, 0 fail; bun test src/permission/permission.test.ts -t "gate cache identity" -> 1 pass.

No blocking defects. No commits made from this review.

[TheGreatAxios]

[warden trust review] Trust green on fix commit 3d61cd1. Verified by diff + targeted tests (241 pass, 0 fail across src/plugins/path-escape-plugin.test.ts and src/permission/permission.test.ts at 3d61cd1).

• Identity: identityArguments now delegates to normalizePathArguments, whose walk mirrors escapeValue key propagation (innermost key wins, array entries inherit the array key) with the same looksLikePath + tool-output/archive guards. Authorize-time relative and execution-time rewritten nested in-bounds paths share one cache identity (locked by the new gate cache identity test, which fails visibly on a miss via the seeded-grant flip). Cache miss still re-runs decide() (gate.ts executionVerdict) — fail-closed, unchanged.
• Carve-outs: xpath/xpaths/jsonpath/jsonpaths/classpath/classpaths checked before the endsWith(path/paths) branch. No first-party tool takes those keys (grep over src/plugins/packages finds only unrelated substring hits like indexPath); they are foreign-tool query/classpath expressions, squarely inside the existing unknown-key policy (server contract, not sandbox). Real path keys (path, filePath, FILE_PATH, file-path, nested path, arrays) still block — covered by tests.
• Ordering/policy intact: block-before-rewrite in escapeArgs and deny-first path-escape check in decide() untouched; unknown-key allowlist unchanged (new passthrough test locks it). No proto-specific code added or removed in this commit.

One file-for-later (no bypass, symmetric on both cache sides so no identity asymmetry): the deep-walk builders assign out[entryKey] on a plain {} so a literal proto key sets the prototype instead of an own property. Consider Object.create(null)/defineProperty hardening in a follow-up; not introduced here (predates from 5ca559f).

No commits made. No merge performed.