Skip to content

Take the OSC 7 host from the URL parser except for case and localhost - #599

Merged
nedtwigg merged 4 commits into
mainfrom
fix/osc7-host-control-chars
Sep 9, 2026
Merged

nedtwigg merged 4 commits into
mainfrom
fix/osc7-host-control-chars

Conversation

@dormouse-bot

@dormouse-bot dormouse-bot commented Sep 9, 2026 •

Copy link
Copy Markdown
Collaborator

cwdFromOsc7 took the OSC 7 host from a raw regex slice of the URI, and that slice disagreed with the URL parser in two ways, each of which let a host be wrong in the same direction: a local CWD reading as remote, cwdIdentity splitting two panes in the same directory, and a misleading pane header.

A ? or # tail landed in the host. [^/]* runs past the delimiters new URL honours and swallows the tail to the next / or end of string. This is what the nightly security audit (#598) caught as VERDICT: FAIL on application-security, for the docs/specs/security-local.md -> "Terminal output" clause about a retained value being bounded and control-stripped before storage: its payload file://ok.example?a%1Bb%5D0;PWNED%07 produced a host carrying a literal ESC and BEL with isRemote: true. The parser did not reject it because the tail is outside what it treats as the host. The same slice mangled benign input — file://localhost?x/repo/app yielded host: 'localhost?x', rendering localhost?x:/.

Input the parser normalizes rather than rejects still diverged after that anchor. file://loc%E2%80%8Balhost/repo/app parses cleanly to hostname: '' — the file-scheme spelling of localhost — while the slice returned a ZWSP-bearing loc​alhost that the control strip does not touch, since ZWSP is not C0/C1. So a zero-width localhost rendered as localhost:/repo/app and read as remote. %C2%AD (soft hyphen) does the same, and file://%F0%9F%92%A9/x kept the raw emoji where the parser gives xn--ls8h.

The fix is one rule covering both: the host is the parser's mapped hostname, and the raw slice wins only for the two things that mapping erases — case (Prod-Box, the reason the slice exists) and the literal localhost spelling the file scheme flattens to the empty string. Anything else is unnormalized input, so the parser's answer is the truthful one. prod-box, the bare file://prod-box form, file://[::1]/x, and file://localhost/C:/... are unchanged; a non-ASCII host now displays punycoded, which is what it resolves as.

boundedCwdValue stays on the host expression so the value is sanitized where it is built rather than resting on the parser rejecting every control character a host could decode to.

Blast radius of the original defect was limited: React renders these labels as text nodes, so there was no markup execution and no re-injection into a PTY, and neither dor ls nor the remote directory sends host across a boundary. It was header and archive-record corruption, not code execution.

Verification and scope notes

Assertions in the existing bounds every CWD source and strips control characters case in lib/src/lib/terminal-state.test.ts pin the audit payload (host: 'ok.example'), the query and fragment tails, the ZWSP and soft-hyphen localhosts (isRemote: false), the punycoded emoji, and the two preserved forms (Prod-Box, localhost). Reverting the reconciliation to return sliced makes them fail (88 tests | 1 failed), so they are load-bearing.

pnpm test in lib/ (typecheck plus vitest): 175 files, 2788 tests, all passing. pnpm lint:specs passes; docs/specs/terminal-state.md gains the host-derivation rule and its budget is ratcheted in the same commit, per the AGENTS.md convention.

The control-character strip on host is not independently reachable under Node's or Chromium's WHATWG parser: a percent-encoded control inside the host proper is a forbidden host code point, so file://ok%1Bexample/repo fails the parse outright and cwdFromOsc7 returns null before the slice runs. The length cap is likewise covered, since rawUri is bounded before the parse and a percent-decode only shrinks. The strip stays as the explicit sanitization point for a retained value, not as a live fix, which is why no test pins it.

Archived CwdState records written before this commit can still carry an unnormalized host; that is existing persisted data, outside the parse-boundary rule this fixes, and left alone to keep the change atomic.

cwdFromOsc7 bounded rawUri before parsing, but extractFileUriHost
percent-decodes its slice of that URI afterwards, so an encoded control
character came back into CwdState.host with nothing to re-strip it. The
sibling path on the adjacent line is already re-bounded after its own
decode; host now is too.

Refs #598
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 9, 2026 •

Copy link
Copy Markdown

Deploying mouseterm with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1290597
Status: ✅  Deploy successful!
Preview URL: https://c5f48f08.mouseterm.pages.dev
Branch Preview URL: https://fix-osc7-host-control-chars.mouseterm.pages.dev

View logs

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Feedback on work in progress — not a merge verdict. Mark the PR ready when you want the full review.

The strip is in the right place, but it leaves the parse bug that put the payload in host to begin with. extractFileUriHost slices [^/]*, which swallows the ?/# tail to end of string, so after this fix host still retains arbitrary attacker-controlled printable text — only the control characters are gone. file://localhost?x/repo/app yields host: 'localhost?x', and that value is what isRemoteFileHost compares against 'localhost' (so a local CWD reads as remote), what cwdIdentity concatenates into the grouping key (so two panes in the same directory stop grouping), and what the pane header renders as localhost?x:/.

Anchoring the slice at the delimiters new URL already honours removes it at the source and makes the regex agree with the parsed.hostname fallback it sits beside — [^/?#]* returns ok.example for the audit's payload and localhost for the case above, with prod-box and the bare file://prod-box form unchanged. boundedCwdValue is still worth keeping on the result, since a percent-encoded control inside the host proper survives the slice.

The new assertion pins the current shape as intended ('ok.example?ab]0;PWNED'). If the tail shouldn't be in host at all, that expectation should be 'ok.example' rather than a freeze of the behavior the payload exploits.

function extractFileUriHost(uri: string): string | undefined {
  const match = uri.match(/^file:\/\/([^/?#]*)(?:[/?#]|$)/i);

(That block targets extractFileUriHost in lib/src/lib/terminal-state.ts, outside this diff, so it's here rather than inline.)

extractFileUriHost sliced [^/]*, so a query or fragment tail was swallowed into the host: file://localhost?x/repo/app yielded host 'localhost?x', which read as remote, changed the grouping key, and rendered in the pane header. Anchoring at [/?#] makes the slice agree with the parsed.hostname fallback beside it and keeps the audit payload out of host entirely.
@dormouse-bot

Copy link
Copy Markdown
Collaborator Author

Fixed at the source, as suggested — extractFileUriHost now matches /^file:\/\/([^/?#]*)(?:[/?#]|$)/i, so the ?/# tail never reaches host. The test expectation moved off the frozen behavior: the audit payload now asserts host: 'ok.example', and file://localhost?x/repo/app asserts host: 'localhost' with isRemote: false, plus a fragment case. Restoring [^/]* makes them fail. pnpm test in lib/: 175 files, 2788 tests, all green.

One correction to the reasoning for keeping boundedCwdValue on the result: a percent-encoded control inside the host proper does not survive the slice, because it never gets that far. For the file: scheme the WHATWG parser percent-decodes the host and rejects forbidden host code points, so file://ok%1Bexample/repo fails new URL and cwdFromOsc7 returns null before extractFileUriHost runs (same for %7F, %09, and %C2%9B). The length cap is redundant too — rawUri is bounded before the parse and a percent-decode only shrinks.

I kept the call anyway, with the comment rewritten to say why: the host is retained, rendered, and part of cwdIdentity, so it is sanitized where it is built rather than resting on a parser's host validation staying that strict. No test pins it, since asserting an unreachable branch would just be a second freeze of parser behavior.

@nedtwigg
nedtwigg marked this pull request as ready for review September 9, 2026 16:46

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

extractFileUriHost still diverges from the parser on input the URL host parser normalizes rather than rejects, so the failure shape this PR fixes stays reachable through an invisible character instead of a ?. file://loc%E2%80%8Balhost/repo/app parses cleanly — new URL(...).hostname is '', the file-scheme spelling of localhost — while the slice returns loc​alhost, which the control-character strip doesn't touch (ZWSP is not C0/C1). So isRemote comes back true, cwdIdentity splits that pane from the real localhost pane in the same directory, and the header renders localhost:/repo/app because the ZWSP has no width. %C2%AD (soft hyphen) does the same, and file://%F0%9F%92%A9/x keeps the raw emoji where the parser gives xn--ls8h.

This is pre-existing and outside the audit clause — the rule names bounding and control-stripping, which boundedCwdValue still does — so it doesn't block the fix. It does undercut the description's claim that the anchored slice "makes the regex agree with the parsed.hostname fallback it sits beside": they now agree on delimiters, not on IDNA mapping. Worth either narrowing that claim or deciding whether the slice should defer to parsed.hostname when the two differ by more than case, since case is the only difference the slice exists to preserve.

Separately, the title names the part of the change that turned out to be unreachable. The defect fixed is the host slice; the strip is defensive, as the description itself establishes. Something like "Anchor the OSC 7 host slice at the ? and # delimiters" would describe what merges.

The anchored slice agreed with the parser on delimiters but not on IDNA
mapping, so `file://loc<ZWSP>alhost/repo/app` still produced a zero-width
`localhost` with isRemote true — the same failure shape, reached through an
ignorable code point instead of a `?` tail. The slice now wins only for the
two things the parser erases: case, and the file-scheme `localhost` it
flattens to the empty string.
@dormouse-bot dormouse-bot changed the title Strip control characters from the OSC 7 host after its percent-decode Take the OSC 7 host from the URL parser except for case and localhost Sep 9, 2026
@dormouse-bot

Copy link
Copy Markdown
Collaborator Author

Confirmed and fixed rather than narrowed, since the divergence produces the same failure this PR exists to remove. Against Node's parser, file://loc%E2%80%8Balhost/repo/app gives hostname: '' while the slice returns loc<ZWSP>alhost; %C2%AD behaves the same, and file://%F0%9F%92%A9/x gives xn--ls8h against a raw emoji.

fileUriHost now takes the parser's mapped hostname and lets the raw slice win only where the mapping erases something: case, and the literal localhost the file scheme flattens to the empty string. Everything else is unnormalized input the parser has already resolved. So the two agree by construction now, not by coincidence of delimiters, and the description's claim is true as stated instead of needing a hedge.

Behavior changes only where the two disagreed: a ZWSP or soft-hyphen localhost resolves to localhost (isRemote: false, no host in the header) and a non-ASCII host displays punycoded, which is what it resolves as. prod-box, Prod-Box, file://prod-box, file://[::1]/x, and file://localhost/C:/... are unchanged, each pinned in bounds every CWD source and strips control characters. Reverting the reconciliation to return sliced makes that case fail, and pnpm test in lib/ is green at 2788 tests.

docs/specs/terminal-state.md gains the derivation rule — it is the sentence whose deletion would let the slice drift back — with its word budget ratcheted in the same commit.

Title retaken on what merges: Take the OSC 7 host from the URL parser except for case and localhost. It covers the anchor and the mapping together, and drops the strip, which the description already establishes as defensive rather than the defect fixed.

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The reconciliation holds: every divergence I could construct — percent-encoded case (file://%50rod-box), IPv6 compression (file://[0:0:0:0:0:0:0:1]), the backslash the regex doesn't treat as a delimiter (file://Prod-Box\repo), a leading space that defeats the ^file:// anchor — falls back to parsedHostname rather than to a slice the parser disagrees with, so the ZWSP class of input is closed rather than relocated.

One inconsistency the change leaves behind: the OSC 7 row of the Supported OSC Inputs table in docs/specs/terminal-state.md still ends with "path decoded, host preserved". Host-preserved is exactly the rule this PR replaces, and the cell now contradicts the derivation rule added six lines below it — a reader who stops at the table gets the pre-PR behavior. The cell wants "path decoded, host derived from the parser", leaving the rule in the paragraph that owns it.

That row sits outside the diff hunk, so it isn't inline-able.

@dormouse-bot

Copy link
Copy Markdown
Collaborator Author

Fixed — the OSC 7 row now reads "path decoded, host derived from the parser", so the table states the same rule as the derivation paragraph below it instead of the pre-PR behavior. The rule itself stays in the paragraph that owns it. node scripts/spec-lint.mjs passes at the current budget, so no ratchet was needed.

@nedtwigg
nedtwigg merged commit 77b3288 into main Sep 9, 2026
10 checks passed
@nedtwigg
nedtwigg deleted the fix/osc7-host-control-chars branch September 9, 2026 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants