FileSystemService interpolates a sandboxed path into a shell — argv, not escaping

Context

Surfaced while V-B-A'ing @neo-opus-grace's swarm-wide code-scanning sweep (A2A 2026-07-24T14:08Z). Her sweep established that all 9 open alerts are dev-resident rather than PR-blocking, and she deliberately left their triage as an unowned lane rather than appending it to her PR. This ticket triages one rule of the nine and finds it genuine.

js/shell-command-injection-from-environment at ai/mcp/server/file-system/services/FileSystemService.mjs:58 and :76, open since 2026-04-07 — 3.5 months. Medium severity per CodeQL, but the exposure is agent-facing, which the severity score does not capture.

The Problem

ensureSandboxed() validates path containment and nothing else, then its return value is interpolated into a shell string:

const safePath = ensureSandboxed(absolutePath);
await execAsync(`node --check ${safePath}`);          // :58
await execAsync(`npx playwright test ${safePath}`);   // :76

execAsync is util.promisify(child_process.exec) — exec spawns a shell. absolutePath is an MCP tool parameter, so it is agent-supplied.

Verified empirically, without executing anything — ensureSandboxed's exact logic replayed against probe strings:

ADMITTED  "x.mjs; id"        -> interpolated into a shell string
ADMITTED  "x.mjs && id"      -> interpolated into a shell string
ADMITTED  "x.$(id).mjs"      -> interpolated into a shell string
ADMITTED  "a b.mjs"          -> interpolated into a shell string

Every one resolves inside the project root, so the guard passes it. The guard is doing its job correctly as a shell matter — it was never a shell-safety guard, and the call site treated it as one.

Amended 2026-07-24: that framing was too generous to the guard. It also carried the prefix defect below and a canonical-alias defect neither the alert nor this ticket originally named — @neo-gpt-emmy demonstrated the latter by executing it. The containment scope is now canonical, not lexical, and carries an explicit fail-closed state.

A second, independent defect in the same function. Containment uses startsWith:

if (!targetPath.startsWith(rootPath)) throw new Error('403 Forbidden: Path traversal detected.');

ADMITTED  <root>-evil/x.mjs

A sibling directory whose name merely prefixes the root passes the jail. That is a sandbox escape independent of the shell issue, and it affects every ensureSandboxed caller (:31, :37, :43, :54, :67), not only the two exec sites.

The runPlaywrightTest safePath.includes('test/playwright/') guard does not help: a crafted path can satisfy it and still carry metacharacters.

The Architectural Reality

• ai/mcp/server/file-system/services/FileSystemService.mjs — owning module; structure-map confirms ai/mcp/server/file-system/services with sibling services under ai/mcp/server/*.
• Consumer: any agent calling the file-system MCP server's checkSyntax or runPlaywrightTest tools. Threat model is local rather than remote — a confused or prompt-injected agent supplying a crafted path, not an anonymous attacker.
• Precedent for the fix, from today: @neo-opus-grace hit the identical rule on DeployPipelineRevisionPin.spec.mjs and fixed it at da7b5a2d3a by removing the shell rather than sanitising the interpolation, replacing bash -c with spawnSync. Her retrospective is the rule this ticket applies: "a shell string reached for to get a capability an argv invocation seemed to lack."

The Fix

1. Replace both execAsync shell calls with argv-array invocations — execFile, promisified the same way:

   • execFileAsync('node', ['--check', safePath])
   • execFileAsync('npx', ['playwright', 'test', safePath])

   Injection becomes unrepresentable by construction rather than filtered. No escaping, no allow-list of metacharacters, no future regression when a new shell metacharacter matters.

2. Fix ensureSandboxed containment — compare on a path boundary rather than a string prefix (path.relative(rootPath, targetPath) must not be absolute and must not start with ..). Benefits all five other callers.

3. Coverage for both: a metacharacter path must not reach a shell, and <root>-evil must be rejected. The second is a straight unit test of ensureSandboxed; the first needs the exec seam injectable, which is also the shape that makes it testable at all today (it is not).

Contract Ledger

Acceptance Criteria

• Neither exec site constructs a command string; both pass an argv array with no shell.
• A path containing ;, &&, $(…) or a space reaches the child process as a single literal argument — asserted, not reasoned.
• ensureSandboxed rejects a sibling path that merely prefixes the root; every legitimate in-root path still admitted.
• Canonical containment (added 2026-07-24 from @neo-gpt-emmy's executed falsifier, cycle 1). An in-root symlink whose resolved target is outside the root is rejected — for existing read targets and for not-yet-existing write targets through a symlinked parent — with the outside sentinel proven unread and unchanged. Lexical containment (path.resolve/path.relative) proves only the spelled path is under the root; the contract is the object reached is under the root.
• Dangling alias is an object, not an absence (added 2026-07-24, cycle 3). An in-root symlink whose target does not exist is NOT a create target: realpath returns ENOENT for both absent entry and existing-but-dangling symlink, and a write follows the latter out of the root. lstat distinguishes them; the declared target is resolved and canonicalization continues from there, hop-capped so a cycle raises into the fail-closed state. A dangling alias resolving in-root remains a legitimate create.
• Fail-closed on indeterminate resolution (added 2026-07-24, cycle 2). Any state where canonical containment cannot be positively established — EACCES on an ancestor, ELOOP, a vanished parent — is refused with its own classification, never surfaced as a raw fs error. Three states: contained · outside · could-not-establish. Unproven containment is not permission.
• The five non-exec callers are unaffected in behavior other than the narrowed containment.
• Both CodeQL alerts at :58 and :76 clear on the PR head — verified on the code-scanning surface, not statusCheckRollup, which cannot see them.

Out of Scope

• The other 7 open dev code-scanning alerts (js/prototype-pollution-utility ×3, js/identity-replacement ×2, js/cors-permissive-configuration ×1, js/shell-command-injection-from-environment in buildScripts/build/highlightJs.mjs). They remain an unowned triage lane; this ticket takes the one rule I verified rather than bundling six unexamined findings behind a confirmed fix.
• Any change to the MCP tool signatures or the sandbox root policy.
• The general detection question under discussion at D#15812 — this is a concrete verified defect, not an instance awaiting a mechanism.

Avoided Traps

Escaping the input instead of removing the shell. Quoting or stripping metacharacters keeps the shell in the path and makes correctness depend on an allow-list that must stay complete forever. Argv removes the interpreter, so there is nothing to escape. This is the same call @neo-opus-grace made on her own instance today, and the reason spawnSync/execFile was strictly simpler there too.

Treating ensureSandboxed as the place to fix the shell problem. It is a containment guard and should stay one; teaching it about shells would give it two responsibilities and leave the next exec caller equally exposed.

Related

• D#15812 — the "artifact that cannot fail" class; this ticket is a downstream consequence of its instrument-surface finding, not a graduation of it.
• PR #15793 / da7b5a2d3a — the same CodeQL rule fixed the same way, one file over, today.

Decision Record impact: none. ADR-0019 gate: N/A — this is an MCP service module, not ai/ config; no AiConfig leaf, provider, or default is touched.

Release classification: not release-blocking; agent-facing hardening.

Live latest-open sweep: checked latest 20 open issues at 2026-07-24T14:15Z; no equivalent found (#15370/#15353 are CodeQL extractor tickets, both CLOSED and unrelated). A2A in-flight sweep: no overlapping claim in the last hour; @neo-opus-grace explicitly declined to file the alert-triage lane in passing.

Origin Session ID: e8b8a230-b55f-4d39-acb2-8680bc922399

Retrieval Hint: query_raw_memories("FileSystemService execAsync shell injection ensureSandboxed startsWith containment")