Skip to content

Commit 8ab978a

Browse files
committed
review pass
1 parent df3ba63 commit 8ab978a

53 files changed

Lines changed: 2156 additions & 404 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.github/workflows/test-build.yml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,12 @@ jobs:
126126
- name: Desktop bridge contract audit
127127
run: bun run check:desktop-bridge
128128

129+
# Complements the bridge audit above, which compares against a snapshot
130+
# this same PR is allowed to regenerate. This one derives every fact from
131+
# the source both sides execute, so it has no such blind spot.
132+
- name: Desktop IPC contract audit
133+
run: bun run check:desktop-ipc
134+
129135
- name: Shared utils enforcement audit
130136
run: bun run check:utils
131137

apps/desktop/README.md

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,21 +7,33 @@ A thin Electron shell around the hosted Sim web app. The renderer loads the conf
77
```
88
src/main/ # main process (bundled to dist/main.cjs)
99
index.ts # lifecycle + wiring
10+
ipc.ts # the single channel table: gate, version floor, handler
1011
config.ts # origin + settings store (userData/settings.json)
12+
app-routes.ts # Sim routes the shell navigates to (menu + tray share them)
13+
atomic-json-file.ts # crash-safe write for the encrypted userData stores
1114
navigation.ts # navigation classifier + openExternalSafe
1215
windows.ts # window.open policy (full app windows, MCP popup, blank children)
1316
window.ts # secure BrowserWindow, permissions, crash/hang recovery
1417
security-guards.ts# global web-contents guards, TLS policy
18+
csp.ts # Content-Security-Policy fallback header
1519
handoff.ts # 127.0.0.1 loopback login handoff + token redeem
1620
session-lifecycle.ts # sign-out teardown, 401 watcher, connect intercept
1721
load-health.ts # offline/error page, auto-retry, watchdog
1822
local-filesystem.ts # session-scoped read-only directory grants + localfs:// broker
23+
local-filesystem-grant-store.ts # those grants, encrypted at rest
24+
desktop-settings.ts # renderer-facing settings surface
1925
downloads.ts # will-download handling
2026
context-menu.ts # native right-click + spellcheck
2127
telemetry-policy.ts # third-party analytics blocking
2228
observability.ts # JSONL event log (userData/logs/desktop-events.log)
2329
updater.ts # electron-updater wiring, channels, downgrade/block guards
2430
menu.ts # role-based macOS menus
31+
tray.ts # tray icon, recent-chat menu, environment marker
32+
browser-agent/ # the agent browser: tab lifecycle, panel geometry, CDP driver
33+
terminal/ # the agent terminal: PTY sessions, tmux, shell integration
34+
browser-credentials/ # saved passwords, OS-auth gated, safeStorage at rest
35+
browser-sites/ # imported site directory, safeStorage at rest
36+
browser-import/ # one-shot import of profiles, cookies and passwords
2537
src/preload/ # contextBridge IPC bridge (bundled to dist/preload.cjs)
2638
static/ # bundled local pages (offline.html)
2739
e2e/ # Playwright _electron smoke suite
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
import { describe, expect, it } from 'vitest'
2+
import { newChatRoute, settingsRoute } from '@/main/app-routes'
3+
4+
describe('app routes', () => {
5+
it('derives the new-chat route from the last workspace route', () => {
6+
expect(newChatRoute('/workspace/ws1/w/wf2')).toBe('/workspace/ws1/home')
7+
expect(newChatRoute('/workspace/ws1/home?resource=r1')).toBe('/workspace/ws1/home')
8+
expect(newChatRoute('/account')).toBe('/workspace')
9+
expect(newChatRoute(undefined)).toBe('/workspace')
10+
expect(newChatRoute('//evil.example')).toBe('/workspace')
11+
})
12+
13+
it('derives the settings route from the last workspace route', () => {
14+
expect(settingsRoute('/workspace/ws1/w/wf2')).toBe('/workspace/ws1/settings/desktop')
15+
expect(settingsRoute('/account')).toBe('/workspace')
16+
expect(settingsRoute(undefined)).toBe('/workspace')
17+
expect(settingsRoute('//evil.example')).toBe('/workspace')
18+
})
19+
})
Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
import { isSafeInternalPath } from '@/main/config'
2+
3+
/**
4+
* Routes into the Sim web app that the shell navigates to on the user's
5+
* behalf, from a menu item or a tray item.
6+
*
7+
* They live here rather than in either caller because both the tray and the
8+
* application menu offer the same destinations. Keeping them in `tray.ts` made
9+
* `index.ts` import tray internals to wire up menu items that have nothing to
10+
* do with the tray, and the tray can be absent entirely.
11+
*/
12+
13+
/** Workspace id from the last visited route, or null when it carries none. */
14+
function workspaceIdFromRoute(lastRoute: string | undefined): string | null {
15+
if (isSafeInternalPath(lastRoute)) {
16+
const match = /^\/workspace\/([^/?#]+)/.exec(lastRoute)
17+
if (match) {
18+
return match[1]
19+
}
20+
}
21+
return null
22+
}
23+
24+
/**
25+
* Route for "New Chat": the home (chat) surface of the workspace the user was
26+
* last in, falling back to the workspace picker redirect when the last route
27+
* carries no workspace.
28+
*/
29+
export function newChatRoute(lastRoute: string | undefined): string {
30+
const workspaceId = workspaceIdFromRoute(lastRoute)
31+
return workspaceId ? `/workspace/${workspaceId}/home` : '/workspace'
32+
}
33+
34+
/**
35+
* Route for "Settings…": the Sim app's settings surface for the workspace the
36+
* user was last in, falling back to the workspace picker redirect.
37+
*/
38+
export function settingsRoute(lastRoute: string | undefined): string {
39+
const workspaceId = workspaceIdFromRoute(lastRoute)
40+
return workspaceId ? `/workspace/${workspaceId}/settings/desktop` : '/workspace'
41+
}
Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
import { mkdirSync, renameSync, writeFileSync } from 'node:fs'
2+
import { mkdir, rename, unlink, writeFile } from 'node:fs/promises'
3+
import { dirname } from 'node:path'
4+
5+
/** Owner-only, matching every store that keeps user data in userData. */
6+
const FILE_MODE = 0o600
7+
8+
/**
9+
* Distinct per call, not just per process.
10+
*
11+
* The pid keeps a second Sim process from sharing the path — the site
12+
* directory used a bare `.tmp` and could be clobbered by exactly that. The
13+
* counter covers the other half: these stores are read-modify-write with no
14+
* lock, so two overlapping writes to the SAME store in one process (a password
15+
* import racing a forget) would otherwise both truncate and write the one
16+
* temp file, and the first rename would publish a spliced blob. The vault
17+
* treats an unparseable file as empty, so that surfaces as every saved
18+
* password silently vanishing.
19+
*/
20+
let temporaryFileCounter = 0
21+
function temporaryPathFor(filePath: string): string {
22+
temporaryFileCounter += 1
23+
return `${filePath}.${process.pid}.${temporaryFileCounter}.tmp`
24+
}
25+
26+
/**
27+
* Crash-safe JSON writes for the small encrypted stores in userData.
28+
*
29+
* Every one of them (local-filesystem grants, the credential vault, the site
30+
* directory) had written this same temp-file-then-rename sequence by hand, and
31+
* they had already drifted: two scoped the temporary file by pid and the third
32+
* did not, so two Sim processes writing that store could clobber each other
33+
* through a shared `.tmp` path. Owning the sequence once removes the class.
34+
*/
35+
export async function writeJsonFileAtomically(filePath: string, value: unknown): Promise<void> {
36+
await mkdir(dirname(filePath), { recursive: true })
37+
const temporaryPath = temporaryPathFor(filePath)
38+
await writeFile(temporaryPath, JSON.stringify(value), { mode: FILE_MODE })
39+
await rename(temporaryPath, filePath)
40+
}
41+
42+
/**
43+
* The same sequence for a caller that cannot await.
44+
*
45+
* Only the settings store needs this: it flushes on `before-quit`, where the
46+
* event loop stops before a promise would settle. `indent` because that file
47+
* is one users open and edit by hand.
48+
*/
49+
export function writeJsonFileAtomicallySync(
50+
filePath: string,
51+
value: unknown,
52+
indent?: number
53+
): void {
54+
mkdirSync(dirname(filePath), { recursive: true })
55+
const temporaryPath = temporaryPathFor(filePath)
56+
writeFileSync(temporaryPath, JSON.stringify(value, null, indent), { mode: FILE_MODE })
57+
renameSync(temporaryPath, filePath)
58+
}
59+
60+
/**
61+
* Deletes a store file, treating "already gone" as success.
62+
*
63+
* Anything else rethrows: a store that reports a successful `clear()` after an
64+
* EACCES tells sign-out teardown the data is gone when it is still on disk.
65+
*/
66+
export async function removeFileIfPresent(filePath: string): Promise<void> {
67+
try {
68+
await unlink(filePath)
69+
} catch (error) {
70+
if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error
71+
}
72+
}

apps/desktop/src/main/browser-agent/driver.test.ts

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,19 +3,36 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'
33
vi.mock('electron', () => import('@/test/electron-mock'))
44

55
import { BrowserWindow } from 'electron'
6+
import * as driverModule from '@/main/browser-agent/driver'
7+
import * as session from '@/main/browser-agent/session'
68

79
type DriverModule = typeof import('@/main/browser-agent/driver')
810

9-
async function freshDriver(): Promise<DriverModule> {
10-
vi.resetModules()
11-
return await import('@/main/browser-agent/driver')
11+
/**
12+
* `initDriver` is a full reset of the driver's and the session's per-session
13+
* state, so a clean driver needs no module reload — which is what lets this
14+
* file use static imports instead of the `vi.resetModules()` the root
15+
* CLAUDE.md forbids. Tests needing real callbacks call `initDriver` again;
16+
* calling it twice is exactly the re-init case the reset exists for.
17+
*/
18+
function freshDriver(): DriverModule {
19+
driverModule.initDriver(
20+
{
21+
onPageState: vi.fn(),
22+
onTabsState: vi.fn(),
23+
onSessionStatus: vi.fn(),
24+
onFillAvailability: vi.fn(),
25+
},
26+
() => null
27+
)
28+
return driverModule
1229
}
1330

1431
describe('executeTool', () => {
1532
let driver: DriverModule
1633

1734
beforeEach(async () => {
18-
driver = await freshDriver()
35+
driver = freshDriver()
1936
})
2037

2138
it('returns ok:false instead of throwing for tool-level failures', async () => {
@@ -63,7 +80,6 @@ describe('executeTool', () => {
6380
)
6481
await driver.executeTool('browser_open_tab', {})
6582

66-
const session = await import('@/main/browser-agent/session')
6783
const contents = session.requireTab().view.webContents
6884
vi.mocked(contents.getURL).mockReturnValue(url)
6985
vi.mocked(contents.executeJavaScript).mockImplementation(() => new Promise<never>(() => {}))
@@ -115,7 +131,6 @@ describe('executeTool', () => {
115131
)
116132
await driver.executeTool('browser_open_tab', {})
117133

118-
const session = await import('@/main/browser-agent/session')
119134
const contents = session.requireTab().view.webContents
120135
vi.mocked(contents.executeJavaScript).mockImplementation(() => new Promise<never>(() => {}))
121136

@@ -146,7 +161,7 @@ describe('credential protection', () => {
146161
let driver: DriverModule
147162

148163
beforeEach(async () => {
149-
driver = await freshDriver()
164+
driver = freshDriver()
150165
})
151166

152167
/** Opens a tab on a real URL so injected page calls are not short-circuited. */
@@ -162,7 +177,6 @@ describe('credential protection', () => {
162177
() => win
163178
)
164179
await driver.executeTool('browser_open_tab', {})
165-
const session = await import('@/main/browser-agent/session')
166180
const contents = session.requireTab().view.webContents
167181
vi.mocked(contents.getURL).mockReturnValue('https://example.com/login')
168182
return contents

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,8 @@ export interface DriverCallbacks {
8585

8686
let driverCallbacks: DriverCallbacks | null = null
8787
let knownSessions: BrowserKnownSessionRegistry | null = null
88+
/** Kept so teardown can force its erasures past the settings write debounce. */
89+
let configStore: ConfigStore | null = null
8890

8991
/**
9092
* Page states auto-handled since the last tool result (dismissed dialogs,
@@ -190,6 +192,20 @@ export function initDriver(
190192
): void {
191193
driverCallbacks = callbacks
192194
knownSessions = config ? new BrowserKnownSessionRegistry(config) : null
195+
configStore = config ?? null
196+
// The rest of this module's state is per-session too. Left behind, a new
197+
// session inherits the previous one's pending notices, a takeover still
198+
// waiting on a user who is gone, and a fingerprint that suppresses its very
199+
// first tab push as a duplicate.
200+
pendingNotices = []
201+
takeoverActive = false
202+
takeoverDone = false
203+
lastTabsStateFingerprint = null
204+
// The serialization chain, too. A takeover from the previous session can sit
205+
// unresolved indefinitely, and its `takeoverDone` flag is reset above — so
206+
// leaving the old chain head in place would queue the new session's first
207+
// tool call behind a promise nothing can ever settle.
208+
toolQueue = Promise.resolve()
193209
initFillCoordinator({
194210
getActiveContents: () => session.activeTab()?.view.webContents ?? null,
195211
onAvailabilityChanged: (available) => callbacks.onFillAvailability(available),
@@ -264,6 +280,11 @@ export async function clearBrowserProfile(): Promise<void> {
264280
knownSessions?.clear()
265281
await session.clearProfileStorage()
266282
await clearCredentials()
283+
// Last, covering the pinned-tab list `clearProfileStorage` just emptied.
284+
// Settings writes coalesce, and an erasure that is still sitting in that
285+
// window when the process dies leaves the previous account's data on disk
286+
// after sign-out already told the user it was gone.
287+
configStore?.flush()
267288
}
268289

269290
function str(params: Record<string, unknown>, key: string): string | undefined {

apps/desktop/src/main/browser-agent/known-sessions.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,12 @@ export class BrowserKnownSessionRegistry {
170170
*/
171171
clear(): void {
172172
this.config.set('browserKnownSites', [])
173+
// Not left to the debounce. Ordinary writes here can afford to coalesce,
174+
// but this one is an erasure the user asked for: if the process dies in
175+
// the coalescing window — force quit, crash, OS shutdown — the previous
176+
// account's browsing trail is still on disk for whoever signs in next,
177+
// and sign-out has already reported success.
178+
this.config.flush()
173179
}
174180

175181
list(cookieSignals: BrowserCookieSignal[]): BrowserKnownSessionsState {

apps/desktop/src/main/browser-agent/panel.test.ts

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,26 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'
33
vi.mock('electron', () => import('@/test/electron-mock'))
44

55
import { BrowserWindow, WebContentsView } from 'electron'
6+
import * as panelModule from '@/main/browser-agent/panel'
67

78
type PanelModule = typeof import('@/main/browser-agent/panel')
89

9-
async function freshPanel(): Promise<PanelModule> {
10-
vi.resetModules()
11-
return await import('@/main/browser-agent/panel')
10+
/**
11+
* `initPanel` is a full reset of the module's session state, so a clean panel
12+
* needs no module reload — which is what lets this file use a static import
13+
* instead of the `vi.resetModules()` the root CLAUDE.md forbids.
14+
*
15+
* The reset happens here rather than being left to `showPanel`, so a test that
16+
* never shows a panel still starts from a clean one.
17+
*/
18+
function freshPanel(): PanelModule {
19+
panelModule.initPanel({
20+
getMainWindow: () => null,
21+
activeTab: () => null,
22+
ensureInitialTab: () => {},
23+
onViewDetached: () => {},
24+
})
25+
return panelModule
1226
}
1327

1428
const PANEL_RECT = { x: 400, y: 64, width: 600, height: 800 }
@@ -50,8 +64,8 @@ function snapshotSentAt(win: BrowserWindow): number | undefined {
5064
describe('panel occlusion', () => {
5165
let panel: PanelModule
5266

53-
beforeEach(async () => {
54-
panel = await freshPanel()
67+
beforeEach(() => {
68+
panel = freshPanel()
5569
})
5670

5771
it('keeps the page up until its replacement frame exists', async () => {

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,22 @@ let resizeBoundWindow: BrowserWindow | null = null
8686
const onHostResize = () => layout()
8787

8888
export function initPanel(panelHost: PanelHost): void {
89+
// A real reset, not a partial setter. Everything below is per-session state,
90+
// and this call IS the session boundary — anything left behind is inherited
91+
// by the next session: a stale owner window that rejects legitimate panel
92+
// updates, a lease timer polling for a panel that no longer exists, a
93+
// `lastApplied*` value that dedupes away the first layout of the new one.
94+
detachAttachedView()
95+
resetOcclusion()
96+
if (leaseTimer !== null) {
97+
clearInterval(leaseTimer)
98+
leaseTimer = null
99+
}
89100
host = panelHost
101+
panelBounds = null
102+
panelAnchor = null
103+
panelLeaseAt = 0
104+
panelOwnerWindow = null
90105
}
91106

92107
/**

0 commit comments

Comments
 (0)