Skip to content

Refuse a .. step behind a backslash in a browser binary path - #1479

Open
dormouse-bot wants to merge 1 commit into
mainfrom
fix/browser-binary-dotdot-backslash
Open

dormouse-bot wants to merge 1 commit into
mainfrom
fix/browser-binary-dotdot-backslash

Conversation

@dormouse-bot

Copy link
Copy Markdown
Collaborator

The browser-binary gate accepted /opt\..\x/agent-browser.exe after #1449. On a Windows host that path is rooted on the current drive, \ is a separator, and the .. is a real parent step, which docs/specs/security-local.md S168 says the gate refuses. This PR makes the .. check split on both separators again, while the file-name check keeps #1449's per-form split, so /r/x\agent-browser stays refused on POSIX.

#1449 split a POSIX-form candidate (leading /) on / alone, which correctly treats \ as a file-name character for the file-name check. It also fed the .. check, though, and the same gate runs on Windows hosts (VS Code, standalone), where a leading-/ path is not POSIX. The path reaches the gate from the webview, as a binaryPath or a persisted pane param, before the host spawns it. The only side effect on POSIX is that a directory named literally a\..\b is refused, which no install produces.

The new rejected cases in lib/src/lib/agent-browser-binary.test.ts (/opt\..\x/agent-browser.exe and the Playwright twin) fail on main and pass here. The unc-binary-path audit canary is re-seeded over the moved lines, with its mutation unchanged, and scripts/security-audit-canary.test.mjs passes.

#1449 split a POSIX-form candidate on `/` alone, so the `..` check stopped
seeing `\..\` in it. A Windows host reads `/opt\..\x/agent-browser.exe` as
rooted on the current drive with `\` as a separator, so the gate accepted a
parent step S168 says it refuses. The `..` check now splits on both
separators; the file-name check keeps the per-form split.

Re-seeds the unc-binary-path canary over the moved lines.
@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying mouseterm with  Cloudflare Pages  Cloudflare Pages

Latest commit: a9f7d7b
Status: ✅  Deploy successful!
Preview URL: https://faa94543.mouseterm.pages.dev
Branch Preview URL: https://fix-browser-binary-dotdot-ba.mouseterm.pages.dev

View logs

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

CI is red only on Argos: argos/storybook-chromium and argos/storybook-webkit each report "1 changed — waiting for your decision". This PR changes no component or story, only the binary-path gate in dor-lib-common/src/browser-providers.ts, its unit test, and an audit canary. The same Argos status is on #1474, #1475, and #1476, so the changed snapshot most likely comes from outside this diff. A maintainer needs to accept or reject it in Argos. Every other check passed at a9f7d7b.

This branch has not been deployed

No deployments
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.

1 participant