Skip to content

Audit the browser binaryPath gate; resolve context directories without following a link to a share - #1156

Merged
nedtwigg merged 4 commits into
mainfrom
audit-binary-path-gate
Oct 17, 2026
Merged

nedtwigg merged 4 commits into
mainfrom
audit-binary-path-gate

Conversation

@nedtwigg

Copy link
Copy Markdown
Member

Two local-security changes.

1. The browser binaryPath gate is an audited rule

docs/specs/dor-browser.md required the binaryPath check as a feature-spec Must, so the nightly audit had no mechanical check for it. It is now FAIL IF docs/specs/security-local.md -> "Spawned programs" #4. That rule covers the hosts' spawn sites, Playwright's in-process playwright-core load, dor's pinned executable, what the gate may accept, and the webview's own check before it sends or stores a path. It also says the path crosses from the webview realm, so a same-user writer is no reason to rate a break below WARNING. The webview writes a pane's saved params, which is the realm the gate fences.

  • dor-browser.md keeps the override variables and "a refused binaryPath is dropped, never fatal", and points at the new section for what the gate accepts.
  • The why moved to security-local.rationale.md -> "Spawned programs". A UNC path lets whoever controls the share supply the binary, and on Windows reaching \\host\share can send the user's NTLM credentials.
  • The unc-binary-path canary seed now names that rule and stays at WARNING. The rule is appended, so no other rule id moved.
  • The security-local.md word budget is ratcheted to 5000.

2. A context directory is resolved without following a link to a share

canonicalDirectory in lib/src/host/git-info.ts and openDirectory in standalone/sidecar/pty-core.js refused a UNC or device path by its text, then called realpath, and pty-core called stat before that. Both follow links. A local symlink or junction to \\server\share passed the text check, and resolving it already reached the share before any check of the result.

canonicalLocalDirectory in the new lib/src/host/local-directory.ts resolves one component at a time with lstat and readlink, which never follow a link. It refuses a link whose target is a share or device path before following it, caps at 40 links, and requires the result to be a directory. Both call sites use it:

  • git-info.ts directly.
  • pty-core through a new injected canonicalDirectory option, whose default opens nothing. The sidecar's main.js passes it from git-info.cjs, and VS Code's pty-host.js bundles it.

The "Terminal context directory actions" FAIL IF now names "a link whose target is one" as well.

What it does not cover:

  • A same-user process swapping a component between the walk and its use.
  • A mapped network drive letter, which is the user's own mapping and behaves as before.
  • Windows reparse points other than symlinks and junctions, which are walked as directories.
  • Canonical paths no longer correct letter case or 8.3 short names on Windows. This has no security effect.

Tests

Each new test went red with the line it pins reverted:

  • lib/src/host/local-directory.test.ts:
    • share and device link targets: a // target, a \\host\share target with a real directory of that name, a link mid-path, and a link to a link
    • relative, absolute, and chained links
    • a link loop, a file, a missing path, and relative input
  • lib/src/host/git-info.test.ts: never follows a link to a share.
  • standalone/sidecar/helper-terminal.test.js: openDirectory touches the path only through the resolver.
  • Wiring tests check that both hosts hand pty-core the shared resolver: standalone/sidecar/main-wiring.test.js and vscode-ext/test/pty-host-wiring.test.ts.

Commands run, all passing: spec, markdown, public-docs, loopback, and outbound lints; scripts/security-audit*.test.mjs (154/154); lib vitest run src/host and tsc -b; standalone tsc --noEmit and sidecar pnpm test on Node 24.18; vscode-ext vitest run.

This changes .github/audit/canaries/, so it needs a maintainer merge.

🤖 Generated with Claude Code

nedtwigg and others added 3 commits October 16, 2026 19:03
The executable gate on a browser provider's binaryPath was a feature-spec
rule in dor-browser.md, so the local audit met it only in its open sweep.
It is now a FAIL IF under security-local.md -> "Spawned programs": a host
spawning (or, for Playwright, loading playwright-core beside) a path the
gate refused, the gate accepting a UNC, device, relative, `..`, or
control-character path or another file name, or the webview sending or
storing one it refuses. The path crosses from the webview realm, which
also writes the saved pane params, so a same-user writer does not lower
its severity.

dor-browser.md keeps the drop-not-fatal behavior and the override
variables and points at the new rule; the gate's rationale moves to
security-local.rationale.md, with why a UNC path matters. The
unc-binary-path seed now names the rule it breaks.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The terminal context's Explore action and Workspace auto-naming's git
lookup checked the reported path for a UNC or device spelling, then
canonicalized it with realpath. realpath reaches each link's target while
resolving it, so a local symlink or junction to \\host\share passed the
string check and reached the share.

canonicalLocalDirectory (lib/src/host/local-directory.ts) now resolves one
component at a time with lstat and readlink, which never follow a link,
and refuses a link whose target is a share or device path before anything
follows it. git-info uses it directly; pty-core takes it as an injected
resolver from the sidecar (through git-info.cjs) and VS Code's PTY host,
and opens nothing without one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… plainly

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Oct 17, 2026 •

Copy link
Copy Markdown

Deploying mouseterm with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5266b3c
Status: ✅  Deploy successful!
Preview URL: https://64dd3efc.mouseterm.pages.dev
Branch Preview URL: https://audit-binary-path-gate.mouseterm.pages.dev

View logs

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three comments outside the diff still cite docs/specs/dor-browser.md → "Browser Host" as the place that says what may be spawned as a browser CLI. This PR moves that rule to docs/specs/security-local.md → "Spawned programs" #4. "Browser Host" now keeps only the override variables and the drop-on-refusal rule, so a reader who follows these pointers to the gate's edges finds nothing there. The heading still exists, so the spec lint stays green:

  • the header comment of lib/src/lib/agent-browser-binary.ts ("What may be spawned as a browser provider's CLI (docs/specs/dor-browser.md → "Browser Host")")
  • the comment above describe('isAllowedAgentBrowserBinary' in lib/src/lib/agent-browser-binary.test.ts ("so its edges are the test (docs/specs/dor-browser.md → "Browser Host")")
  • the comment above the predicates in dor-lib-common/src/browser-providers.ts ("What may be spawned as a browser provider's CLI (docs/specs/dor-browser.md → "Browser Host")")

Re-pointing each one at docs/specs/security-local.md → "Spawned programs" would fix it. I can push that as a commit if you'd like.

The rule moved to "Spawned programs"; "Browser Host" keeps only the
override variables and the drop-on-refusal rule.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@nedtwigg
nedtwigg merged commit 5c6e119 into main Oct 17, 2026
11 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants