Skip to content

Commit 486f918

Browse files
committed
fix(desktop): address review findings (handoff callback gate, fail-closed guards)
- handoff: gate onCallback on a non-consuming state match so a format-valid but wrong/expired state no longer fires the callback (which surfaced a spurious "sign-in failed" UI while the genuine callback was still pending). The one-shot listener still survives wrong-state requests (self-DoS guard); consume() re-validates and tears it down on the real callback. - session: the agent-partition onBeforeRequest SSRF check now fails closed — an unexpected rejection cancels the request instead of leaving it suspended. - updater: the manual checkForUpdates() now has a .catch() that logs and shows an error dialog instead of silently swallowing network/manifest failures.
1 parent 8c38475 commit 486f918

4 files changed

Lines changed: 47 additions & 17 deletions

File tree

apps/desktop/src/main/browser-agent/session.ts

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -79,12 +79,19 @@ function configureAgentPartition(ses: Session): void {
7979
// document's check).
8080
ses.webRequest.onBeforeRequest((details, callback) => {
8181
if (details.resourceType === 'mainFrame' || details.resourceType === 'subFrame') {
82-
void checkAgentUrl(details.url).then((guard) => {
83-
if (!guard.ok) {
84-
logger.warn('Blocked agent document navigation to a private host')
85-
}
86-
callback({ cancel: !guard.ok })
87-
})
82+
void checkAgentUrl(details.url)
83+
.then((guard) => {
84+
if (!guard.ok) {
85+
logger.warn('Blocked agent document navigation to a private host')
86+
}
87+
callback({ cancel: !guard.ok })
88+
})
89+
.catch((error) => {
90+
// Fail closed: an unexpected rejection must cancel, never leave the
91+
// request suspended with no callback.
92+
logger.error('Agent SSRF check failed; cancelling request', { error })
93+
callback({ cancel: true })
94+
})
8895
return
8996
}
9097
callback({ cancel: isBlockedRequestUrl(details.url) })

apps/desktop/src/main/handoff.test.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,11 +97,13 @@ describe('createHandoffManager', () => {
9797
expect(received).toHaveLength(0)
9898

9999
// A format-valid but WRONG state must NOT tear down the one-shot listener
100-
// (self-DoS guard) — the server stays up for the genuine callback.
100+
// (self-DoS guard) and must NOT fire the callback — firing it would surface
101+
// a spurious "sign-in failed" UI while the genuine callback is still pending.
101102
const wrongState = 'z'.repeat(state.length)
102103
expect(
103104
(await fetch(`${base}/auth/callback?token=${VALID_TOKEN}&state=${wrongState}`)).status
104105
).toBe(200)
106+
expect(received).toHaveLength(0)
105107
expect(manager.consume(wrongState)).toBe(false)
106108

107109
// The genuine callback still lands while the server is up.

apps/desktop/src/main/handoff.ts

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,14 @@ export function createHandoffManager(
7070
}
7171
}
7272

73+
/** Constant-time, non-consuming check that a callback's state matches the live
74+
* pending handoff. Gates the callback so a wrong/expired state is ignored
75+
* without consuming the pending handoff or surfacing a failure. */
76+
const matchesPendingState = (state: string): boolean =>
77+
pending !== null &&
78+
now() - pending.createdAt <= HANDOFF_TTL_MS &&
79+
safeCompare(pending.state, state)
80+
7381
const startLoopback = async (): Promise<number | undefined> => {
7482
stopLoopback()
7583
const server = createServer((request, response) => {
@@ -87,11 +95,14 @@ export function createHandoffManager(
8795
response
8896
.writeHead(200, { 'Content-Type': 'text/html; charset=utf-8' })
8997
.end(CALLBACK_RESPONSE_HTML)
90-
// Do NOT tear the server down here: a request with a format-valid but
91-
// wrong state must not kill the one-shot listener before the genuine
92-
// callback arrives. `consume()` stops the server once the state actually
93-
// validates (via clear()); the TTL timer is the backstop otherwise.
94-
onCallback({ token, state })
98+
// Only a state match advances the flow. A format-valid but wrong/expired
99+
// state gets the generic 200 page and is otherwise ignored: the one-shot
100+
// listener survives for the genuine callback (TTL is the backstop) and no
101+
// spurious "sign-in failed" UI fires. `consume()` in the callback path
102+
// re-validates and tears the server down once the real state lands.
103+
if (matchesPendingState(state)) {
104+
onCallback({ token, state })
105+
}
95106
})
96107
loopbackServer = server
97108
try {

apps/desktop/src/main/updater.ts

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -237,9 +237,19 @@ export function checkForUpdatesInteractive(deps: UpdaterDeps): void {
237237
if (!autoUpdater) {
238238
return
239239
}
240-
void autoUpdater.checkForUpdates().then((result) => {
241-
if (!result || result.updateInfo.version === app.getVersion()) {
242-
void dialog.showMessageBox({ type: 'info', message: 'Sim is up to date' })
243-
}
244-
})
240+
void autoUpdater
241+
.checkForUpdates()
242+
.then((result) => {
243+
if (!result || result.updateInfo.version === app.getVersion()) {
244+
void dialog.showMessageBox({ type: 'info', message: 'Sim is up to date' })
245+
}
246+
})
247+
.catch((error) => {
248+
logger.error('Manual update check failed', { error })
249+
void dialog.showMessageBox({
250+
type: 'error',
251+
message: 'Could not check for updates',
252+
detail: 'Something went wrong reaching the update server. Try again later.',
253+
})
254+
})
245255
}

0 commit comments

Comments
 (0)