Repository navigation
Take the OSC 7 host from the URL parser except for case and localhost - #599
Conversation
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
Deploying mouseterm with
|
| 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 |
dormouse-bot
left a comment
There was a problem hiding this comment.
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.
|
Fixed at the source, as suggested — One correction to the reasoning for keeping I kept the call anyway, with the comment rewritten to say why: the host is retained, rendered, and part of |
dormouse-bot
left a comment
There was a problem hiding this comment.
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 localhost, 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.
|
Confirmed and fixed rather than narrowed, since the divergence produces the same failure this PR exists to remove. Against Node's parser,
Behavior changes only where the two disagreed: a ZWSP or soft-hyphen
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
left a comment
There was a problem hiding this comment.
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.
|
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. |
cwdFromOsc7took 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,cwdIdentitysplitting two panes in the same directory, and a misleading pane header.A
?or#tail landed in the host.[^/]*runs past the delimitersnew URLhonours and swallows the tail to the next/or end of string. This is what the nightly security audit (#598) caught asVERDICT: FAILonapplication-security, for thedocs/specs/security-local.md-> "Terminal output" clause about a retained value being bounded and control-stripped before storage: its payloadfile://ok.example?a%1Bb%5D0;PWNED%07produced ahostcarrying a literal ESC and BEL withisRemote: 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/appyieldedhost: 'localhost?x', renderinglocalhost?x:/.Input the parser normalizes rather than rejects still diverged after that anchor.
file://loc%E2%80%8Balhost/repo/appparses cleanly tohostname: ''— the file-scheme spelling of localhost — while the slice returned a ZWSP-bearinglocalhostthat the control strip does not touch, since ZWSP is not C0/C1. So a zero-widthlocalhostrendered aslocalhost:/repo/appand read as remote.%C2%AD(soft hyphen) does the same, andfile://%F0%9F%92%A9/xkept the raw emoji where the parser givesxn--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 literallocalhostspelling 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 barefile://prod-boxform,file://[::1]/x, andfile://localhost/C:/...are unchanged; a non-ASCII host now displays punycoded, which is what it resolves as.boundedCwdValuestays 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 lsnor the remote directory sendshostacross 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 characterscase inlib/src/lib/terminal-state.test.tspin 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 toreturn slicedmakes them fail (88 tests | 1 failed), so they are load-bearing.pnpm testinlib/(typecheck plus vitest): 175 files, 2788 tests, all passing.pnpm lint:specspasses;docs/specs/terminal-state.mdgains the host-derivation rule and its budget is ratcheted in the same commit, per the AGENTS.md convention.The control-character strip on
hostis 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, sofile://ok%1Bexample/repofails the parse outright andcwdFromOsc7returnsnullbefore the slice runs. The length cap is likewise covered, sincerawUriis 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
CwdStaterecords written before this commit can still carry an unnormalizedhost; that is existing persisted data, outside the parse-boundary rule this fixes, and left alone to keep the change atomic.