Skip to content

Commit e8c71d1

Browse files
committed
fix(desktop,cli): scope deployment capabilities to their origin
Changing the server left two device-global stores in place that grant the INCOMING deployment authority the user only handed the outgoing one: local filesystem grants (directories its agent may read, plus security-scoped bookmarks) and the agent browser's cookie jar (live third-party sessions its agent may drive). Sign-out clears exactly this pair; an origin change is the same boundary, so it now clears it too — awaited before the relaunch, since a quit racing an async clear could leave either behind. browserKnownSites goes with the jar it describes, so Sim is never left believing in sign-ins the profile no longer has. The CLI printed the redirect's filename straight to the terminal. It is read out of a Location the deployment chose, so percent-encoded ANSI or OSC survives decodeURIComponent as real control bytes and could forge CLI output; control characters are stripped and the name is bounded before it reaches the spinner. resolveDeploymentUrl took the first source naming an app URL. A machine with both a local checkout and a real deployment would be probed, printed, and opened at whichever enumerated first, silently — so disagreeing sources are now an error naming each candidate and asking for --url, the way resolveFeatureSetupDestination already refuses ambiguity.
1 parent 76e6979 commit e8c71d1

7 files changed

Lines changed: 179 additions & 30 deletions

File tree

apps/desktop/src/main/index.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -482,6 +482,10 @@ function main(): void {
482482
preloadPath,
483483
isPackaged: app.isPackaged,
484484
getParentWindow: getMainWindow,
485+
clearDeploymentScopedState: async () => {
486+
await localFilesystem.forgetAll()
487+
await clearAgentBrowserProfile()
488+
},
485489
relaunch: relaunchApp,
486490
})
487491

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -319,7 +319,7 @@ describe('registerIpcHandlers', () => {
319319
server: {
320320
open: vi.fn(),
321321
getConfiguration: vi.fn(() => ({ origin: APP, defaultOrigin: APP, isSimCloud: true })),
322-
setOrigin: vi.fn(() => ({ ok: true as const, origin: APP, unchanged: true })),
322+
setOrigin: vi.fn(async () => ({ ok: true as const, origin: APP, unchanged: true })),
323323
},
324324
}
325325
registerIpcHandlers(deps)

apps/desktop/src/main/ipc.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -306,7 +306,7 @@ export interface IpcDeps {
306306
server: {
307307
open: () => void
308308
getConfiguration: () => DesktopServerConfiguration
309-
setOrigin: (origin: string) => DesktopServerChangeResult
309+
setOrigin: (origin: string) => Promise<DesktopServerChangeResult>
310310
}
311311
}
312312

apps/desktop/src/main/server-window.test.ts

Lines changed: 44 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ function makeDeps(overrides: Partial<ServerWindowDeps> = {}): ServerWindowDeps {
3434
preloadPath: '/tmp/preload.cjs',
3535
isPackaged: false,
3636
getParentWindow: () => null,
37+
clearDeploymentScopedState: vi.fn(async () => {}),
3738
relaunch: vi.fn(),
3839
...overrides,
3940
}
@@ -64,8 +65,8 @@ describe('server window', () => {
6465
expect(createServerWindow(cloud).getConfiguration().isSimCloud).toBe(true)
6566
})
6667

67-
it('relaunches after storing a different origin', () => {
68-
const result = createServerWindow(deps).setOrigin('https://sim.other.example')
68+
it('relaunches after storing a different origin', async () => {
69+
const result = await createServerWindow(deps).setOrigin('https://sim.other.example')
6970

7071
expect(result).toEqual({ ok: true, origin: 'https://sim.other.example', unchanged: false })
7172
expect(deps.config.flush).toHaveBeenCalled()
@@ -75,28 +76,61 @@ describe('server window', () => {
7576
// The saved route carries the previous deployment's workspace id, and
7677
// resolveStartRoute only discards a route on a confirmed 403 — a fresh
7778
// partition answers 401, so a kept route would survive onto the new server.
78-
it('drops the saved route when the origin changes', () => {
79-
createServerWindow(deps).setOrigin('https://sim.other.example')
79+
it('drops the saved route when the origin changes', async () => {
80+
await createServerWindow(deps).setOrigin('https://sim.other.example')
8081

8182
expect(deps.config.set).toHaveBeenCalledWith('lastRoute', undefined)
8283
})
8384

84-
it('keeps the saved route when the origin is unchanged', () => {
85-
createServerWindow(deps).setOrigin(CURRENT)
85+
it('keeps the saved route when the origin is unchanged', async () => {
86+
await createServerWindow(deps).setOrigin(CURRENT)
8687

8788
expect(deps.config.set).not.toHaveBeenCalled()
8889
})
8990

9091
// Re-confirming the pre-filled URL is the common case here.
91-
it('does not relaunch when the origin is unchanged', () => {
92-
const result = createServerWindow(deps).setOrigin(CURRENT)
92+
it('does not relaunch when the origin is unchanged', async () => {
93+
const result = await createServerWindow(deps).setOrigin(CURRENT)
9394

9495
expect(result).toEqual({ ok: true, origin: CURRENT, unchanged: true })
9596
expect(deps.relaunch).not.toHaveBeenCalled()
9697
})
9798

98-
it('surfaces a rejected origin without relaunching', () => {
99-
const result = createServerWindow(deps).setOrigin('ftp://sim.example.com')
99+
// Filesystem grants and the agent browser's jar are device-global with no
100+
// origin key, so without this the incoming deployment inherits directory
101+
// access and live third-party sessions the user granted the outgoing one.
102+
it('clears deployment-scoped capabilities before relaunching', async () => {
103+
await createServerWindow(deps).setOrigin('https://sim.other.example')
104+
105+
expect(deps.clearDeploymentScopedState).toHaveBeenCalledTimes(1)
106+
expect(vi.mocked(deps.clearDeploymentScopedState).mock.invocationCallOrder[0]).toBeLessThan(
107+
vi.mocked(deps.relaunch).mock.invocationCallOrder[0]
108+
)
109+
})
110+
111+
it('does not clear them when the origin is unchanged', async () => {
112+
await createServerWindow(deps).setOrigin(CURRENT)
113+
114+
expect(deps.clearDeploymentScopedState).not.toHaveBeenCalled()
115+
})
116+
117+
// The origin is already persisted by this point, so a failed clear must not
118+
// strand the shell on the old server — but it is logged, not swallowed.
119+
it('still relaunches when the teardown fails', async () => {
120+
const failing = makeDeps({
121+
clearDeploymentScopedState: vi.fn(async () => {
122+
throw new Error('keychain unavailable')
123+
}),
124+
})
125+
126+
const result = await createServerWindow(failing).setOrigin('https://sim.other.example')
127+
128+
expect(result).toMatchObject({ ok: true, unchanged: false })
129+
expect(failing.relaunch).toHaveBeenCalledTimes(1)
130+
})
131+
132+
it('surfaces a rejected origin without relaunching', async () => {
133+
const result = await createServerWindow(deps).setOrigin('ftp://sim.example.com')
100134

101135
expect(result).toEqual({ ok: false, error: 'bad origin' })
102136
expect(deps.relaunch).not.toHaveBeenCalled()

apps/desktop/src/main/server-window.ts

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -36,17 +36,14 @@ const SERVER_WINDOW_PARTITION = 'server-selection'
3636
* `/workspace/<old-id>` on the new server. `resolveStartRoute` cannot rescue
3737
* that: it discards a route only on a confirmed 403, and a fresh partition has
3838
* no session, so the new server answers 401 and the stale route survives the
39-
* probe.
40-
*
41-
* The agent browser is deliberately untouched — both its cookie jar and the
42-
* `browserKnownSites` inference metadata that describes it. Sign-out clears the
43-
* pair because the ACCOUNT changed; pointing the shell at another deployment
44-
* does not imply that, and signing the operator out of every unrelated site in
45-
* the built-in browser is not a reasonable side effect of correcting a server
46-
* URL. They are kept together on purpose: clearing the metadata alone would
47-
* leave Sim blind to sign-ins that are still live in the profile.
39+
* probe. `browserKnownSites` describes the agent-browser profile that
40+
* {@link ServerWindowDeps.clearDeploymentScopedState} clears, and is dropped
41+
* with it so Sim is never left believing in sign-ins the profile no longer has.
4842
*/
49-
const ORIGIN_SCOPED_SETTINGS: readonly (keyof DesktopSettings)[] = ['lastRoute']
43+
const ORIGIN_SCOPED_SETTINGS: readonly (keyof DesktopSettings)[] = [
44+
'lastRoute',
45+
'browserKnownSites',
46+
]
5047

5148
export interface ServerWindowDeps {
5249
config: ConfigStore
@@ -56,6 +53,19 @@ export interface ServerWindowDeps {
5653
preloadPath: string
5754
isPackaged: boolean
5855
getParentWindow: () => BrowserWindow | null
56+
/**
57+
* Drops the capabilities the OUTGOING deployment was granted, before the new
58+
* one can inherit them.
59+
*
60+
* Local-filesystem grants and the agent browser's cookie jar live in
61+
* device-global stores with no origin key, and both are capabilities the user
62+
* handed to a specific Sim server: directories its agent may read, and live
63+
* third-party sessions its agent may drive. Carrying them across would let
64+
* the next deployment act with authority it was never given — which is why
65+
* sign-out clears exactly this pair. Awaited before the relaunch, since a
66+
* quit racing an async clear could leave either behind.
67+
*/
68+
clearDeploymentScopedState: () => Promise<void>
5969
/**
6070
* Relaunches the shell against the newly stored origin. A full restart rather
6171
* than an in-place swap: the origin decides the cookie partition, the update
@@ -72,7 +82,7 @@ export interface ServerWindowDeps {
7282
export interface ServerWindowHandle {
7383
open(): void
7484
getConfiguration(): DesktopServerConfiguration
75-
setOrigin(origin: string): DesktopServerChangeResult
85+
setOrigin(origin: string): Promise<DesktopServerChangeResult>
7686
}
7787

7888
/**
@@ -149,7 +159,7 @@ export function createServerWindow(deps: ServerWindowDeps): ServerWindowHandle {
149159
})
150160
}
151161

152-
const setOrigin = (raw: string): DesktopServerChangeResult => {
162+
const setOrigin = async (raw: string): Promise<DesktopServerChangeResult> => {
153163
const current = deps.config.getOrigin()
154164
const validated = deps.config.setOrigin(raw)
155165
if (!validated.ok) {
@@ -164,6 +174,14 @@ export function createServerWindow(deps: ServerWindowDeps): ServerWindowHandle {
164174
for (const key of ORIGIN_SCOPED_SETTINGS) {
165175
deps.config.set(key, undefined)
166176
}
177+
// Failing to clear must not strand the shell on the old origin — that is
178+
// already persisted — but it must be loud, because what survives is access
179+
// the next deployment did not earn.
180+
await deps.clearDeploymentScopedState().catch((error) => {
181+
logger.error('Could not clear deployment-scoped state before relaunch', {
182+
error: getErrorMessage(error),
183+
})
184+
})
167185
// setOrigin writes through immediately; the clears above are debounced like
168186
// every other setting. `before-quit` flushes too, but doing it here keeps
169187
// the write independent of the Electron quit sequence.

packages/sim-setup/src/desktop.test.ts

Lines changed: 61 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,14 @@
11
import { describe, expect, it, vi } from 'vitest'
2-
import { describeProbe, probeDownload, resolveDeploymentUrl } from './desktop'
2+
import { describeProbe, probeDownload, resolveDeploymentUrl, sanitizeForTerminal } from './desktop'
33
import { SetupError } from './errors'
44

55
const ASSET = 'https://github.com/simstudioai/sim/releases/download/v1.2.3/Sim-1.2.3-universal.dmg'
66

7-
function source(appUrl?: string) {
8-
return { values: appUrl ? new Map([['NEXT_PUBLIC_APP_URL', appUrl]]) : new Map<string, string>() }
7+
function source(appUrl?: string, label = 'configuration') {
8+
return {
9+
label,
10+
values: appUrl ? new Map([['NEXT_PUBLIC_APP_URL', appUrl]]) : new Map<string, string>(),
11+
}
912
}
1013

1114
function respond(status: number, headers: Record<string, string> = {}): typeof fetch {
@@ -37,6 +40,27 @@ describe('resolveDeploymentUrl', () => {
3740
expect(resolveDeploymentUrl([source()])).toBe('http://localhost:3000')
3841
})
3942

43+
// This command prints one URL as the one to trust and offers to open it, so
44+
// preferring whichever source happened to enumerate first would quietly send
45+
// an operator with a local checkout AND a real deployment to localhost.
46+
it('refuses to guess when sources name different deployments', () => {
47+
expect(() =>
48+
resolveDeploymentUrl([source('http://localhost:3000'), source('https://sim.example.com')])
49+
).toThrow(SetupError)
50+
})
51+
52+
it('accepts agreeing sources and an override that settles the ambiguity', () => {
53+
expect(
54+
resolveDeploymentUrl([source('https://sim.example.com'), source('https://sim.example.com')])
55+
).toBe('https://sim.example.com')
56+
expect(
57+
resolveDeploymentUrl(
58+
[source('http://localhost:3000'), source('https://sim.example.com')],
59+
'https://sim.example.com'
60+
)
61+
).toBe('https://sim.example.com')
62+
})
63+
4064
it('rejects a value that is not an http(s) URL', () => {
4165
expect(() => resolveDeploymentUrl([source('sim.example.com')])).toThrow(SetupError)
4266
expect(() => resolveDeploymentUrl([source('ftp://sim.example.com')])).toThrow(SetupError)
@@ -65,6 +89,24 @@ describe('probeDownload', () => {
6589
}
6690
})
6791

92+
// The end-to-end path that matters: a deployment can percent-encode ANSI in
93+
// the redirect, and decodeURIComponent turns it into real control bytes on
94+
// their way to the spinner.
95+
it('sanitizes a redirect filename before it reaches the terminal', async () => {
96+
const hostile = 'https://example.com/d/v1/Sim%1b%5b2K%1b%5b1Gforged.dmg'
97+
98+
const result = await probeDownload(
99+
'https://sim.example.com/x',
100+
respond(302, { location: hostile })
101+
)
102+
103+
expect(result).toEqual({
104+
status: 'ok',
105+
installerUrl: hostile,
106+
installerName: 'Sim[2K[1Gforged.dmg',
107+
})
108+
})
109+
68110
it('distinguishes no-release from a broken release feed', async () => {
69111
expect(await probeDownload('https://sim.example.com/x', respond(404))).toEqual({
70112
status: 'no-release',
@@ -96,6 +138,22 @@ describe('probeDownload', () => {
96138
})
97139
})
98140

141+
describe('sanitizeForTerminal', () => {
142+
// The name comes out of a redirect the deployment chose, so it is remote
143+
// input on its way to a TTY.
144+
it('strips control characters a deployment could smuggle through the redirect', () => {
145+
expect(sanitizeForTerminal('Sim\u001b[2K\u001b[1G forged.dmg')).toBe('Sim[2K[1G forged.dmg')
146+
expect(sanitizeForTerminal('a\u0000b\u007fc\u009fd')).toBe('abcd')
147+
expect(sanitizeForTerminal('Sim-1.2.3-universal.dmg')).toBe('Sim-1.2.3-universal.dmg')
148+
})
149+
150+
it('caps a name that would overrun the spinner line', () => {
151+
const capped = sanitizeForTerminal('x'.repeat(500))
152+
153+
expect(capped).toBe(`${'x'.repeat(120)}...`)
154+
})
155+
})
156+
99157
describe('describeProbe', () => {
100158
// Failure statuses are the ones an operator has to act on, so each must
101159
// arrive with something to try.

packages/sim-setup/src/desktop.ts

Lines changed: 38 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { getErrorMessage } from '@sim/utils/errors'
2+
import { truncate } from '@sim/utils/string'
23
import { openBrowser } from './cli-auth'
34
import { discoverConfigurationSources } from './configuration-sources'
45
import { SetupError } from './errors'
@@ -23,6 +24,21 @@ const REDIRECT_STATUSES: ReadonlySet<number> = new Set([301, 302, 307, 308])
2324
/** The env var every deployment sets to its own public origin. */
2425
const APP_URL_KEY = 'NEXT_PUBLIC_APP_URL'
2526

27+
/** Keeps a rendered artifact name from overrunning the spinner line. */
28+
const MAX_INSTALLER_NAME = 120
29+
30+
/**
31+
* Strips anything that could move the cursor, repaint, or retitle the terminal.
32+
*
33+
* The artifact name is read out of a redirect the deployment chose, so it is
34+
* remote input on its way to a TTY: percent-encoded ANSI or OSC bytes survive
35+
* `decodeURIComponent` as real control characters and would let a compromised
36+
* deployment forge CLI output. C0 (including ESC), DEL, and C1 all go.
37+
*/
38+
export function sanitizeForTerminal(value: string): string {
39+
return truncate(value.replace(/[\u0000-\u001f\u007f-\u009f]/g, ''), MAX_INSTALLER_NAME)
40+
}
41+
2642
export interface DesktopFlags {
2743
/** Overrides the deployment origin when the CLI runs away from the install. */
2844
url?: string
@@ -35,12 +51,31 @@ export interface DesktopFlags {
3551
* Read from every discovered source, not only the one `add` may write: an
3652
* operator running a Helm release or an external Compose project still needs
3753
* the URL, and reading it changes nothing.
54+
*
55+
* Sources that disagree are an error rather than a first-match win. This
56+
* command probes a URL, prints it as the one to trust, and offers to open it —
57+
* so silently preferring whichever source enumerated first would point an
58+
* operator with both a local checkout and a real deployment at localhost and
59+
* never say so. `resolveFeatureSetupDestination` refuses ambiguity the same way.
3860
*/
3961
export function resolveDeploymentUrl(
40-
sources: readonly { values?: Map<string, string> | null }[],
62+
sources: readonly { label?: string; values?: Map<string, string> | null }[],
4163
override?: string
4264
): string {
43-
const raw = override ?? sources.map((source) => source.values?.get(APP_URL_KEY)).find(Boolean)
65+
const discovered = sources.filter((source) => source.values?.get(APP_URL_KEY))
66+
const distinct = new Set(discovered.map((source) => source.values?.get(APP_URL_KEY)?.trim()))
67+
if (!override && distinct.size > 1) {
68+
throw new SetupError(
69+
`Found ${distinct.size} configurations naming different ${APP_URL_KEY} values.`,
70+
[
71+
...discovered.map(
72+
(source) => `${source.label ?? 'configuration'}: ${source.values?.get(APP_URL_KEY)}`
73+
),
74+
'Re-run with --url <deployment url> to say which one the desktop app should use.',
75+
]
76+
)
77+
}
78+
const raw = override ?? discovered[0]?.values?.get(APP_URL_KEY)
4479
if (!raw) {
4580
// A wizard-provisioned local install has the compose interpolation default
4681
// rather than an explicit value, so an absent key is not a misconfiguration.
@@ -94,7 +129,7 @@ export async function probeDownload(
94129
} catch {
95130
// Keep the raw Location; it is still the most useful thing to print.
96131
}
97-
return { status: 'ok', installerUrl: location, installerName: name }
132+
return { status: 'ok', installerUrl: location, installerName: sanitizeForTerminal(name) }
98133
}
99134
if (response.status === 404) return { status: 'no-release' }
100135
if (response.status === 502) return { status: 'feed-unavailable' }

0 commit comments

Comments
 (0)