fix(desktop,cli): fail closed on origin change, widen terminal sanitizer

The capability teardown could partially fail and still let the shell move.
Sequential awaits meant a filesystem-grant rejection skipped the browser-profile
clear entirely, and the new origin was already persisted by then, so the
incoming deployment inherited whatever survived — and startup restores it.

Now the two stores clear independently via allSettled and report which ones
survived, and the whole teardown runs BEFORE anything is written. A store that
cannot be emptied refuses the change outright and names what it could not
clear. Nothing is persisted at that point, so refusing leaves the shell exactly
where it was rather than half-applying. Validation moved up front for the same
reason: a typo now costs no teardown.

The terminal sanitizer matched only C0/DEL/C1 by range, so percent-encoded bidi
overrides and isolates survived decodeURIComponent and could still reorder what
the reader sees without emitting one control byte. Matched by Unicode class
instead — Cc covers the cursor controls, Cf covers the bidi ones.

Configuration discovery compared raw strings, so a trailing slash, a default
port, a host-case difference, or an ignored path read as two different servers
and demanded a --url override to settle an ambiguity that did not exist. Now
compared on the parsed origin, which is what the command ends up using.

Author: waleedlatif1

apps/desktop/src/main/index.ts

@@ -483,8 +483,23 @@ function main(): void {
     isPackaged: app.isPackaged,
     getParentWindow: getMainWindow,
     clearDeploymentScopedState: async () => {
-      await localFilesystem.forgetAll()
-      await clearAgentBrowserProfile()
+      // allSettled, not sequential awaits: these are independent stores, and a
+      // rejection from the first must not skip the second — leaving the store
+      // that would have cleared fine still holding the outgoing deployment's
+      // access. Each failure is named so the picker can say what survived.
+      const stores = [
+        { label: 'local file access', clear: () => localFilesystem.forgetAll() },
+        { label: 'built-in browser sessions', clear: () => clearAgentBrowserProfile() },
+      ]
+      const outcomes = await Promise.allSettled(stores.map((store) => store.clear()))
+      return outcomes.flatMap((outcome, index) => {
+        if (outcome.status === 'fulfilled') return []
+        logger.error('Could not clear deployment-scoped state', {
+          store: stores[index].label,
+          error: getErrorMessage(outcome.reason),
+        })
+        return [stores[index].label]
+      })
     },
     relaunch: relaunchApp,
   })

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

@@ -34,7 +34,7 @@ function makeDeps(overrides: Partial<ServerWindowDeps> = {}): ServerWindowDeps {
     preloadPath: '/tmp/preload.cjs',
     isPackaged: false,
     getParentWindow: () => null,
-    clearDeploymentScopedState: vi.fn(async () => {}),
+    clearDeploymentScopedState: vi.fn(async (): Promise<readonly string[]> => []),
     relaunch: vi.fn(),
     ...overrides,
   }
@@ -114,25 +114,46 @@ describe('server window', () => {
     expect(deps.clearDeploymentScopedState).not.toHaveBeenCalled()
   })
 
-  // The origin is already persisted by this point, so a failed clear must not
-  // strand the shell on the old server — but it is logged, not swallowed.
-  it('still relaunches when the teardown fails', async () => {
+  // Fail closed. A store that could not be emptied is access the incoming
+  // deployment would inherit and that startup would restore, so the change is
+  // refused outright — and because nothing is persisted until the teardown
+  // succeeds, refusing leaves the shell exactly where it was.
+  it('refuses the change when a store could not be cleared', async () => {
     const failing = makeDeps({
+      clearDeploymentScopedState: vi.fn(async () => ['local file access']),
+    })
+
+    const result = await createServerWindow(failing).setOrigin('https://sim.other.example')
+
+    expect(result).toMatchObject({ ok: false })
+    expect(result).toHaveProperty('error', expect.stringContaining('local file access'))
+    expect(failing.relaunch).not.toHaveBeenCalled()
+    expect(failing.config.setOrigin).not.toHaveBeenCalled()
+    expect(failing.config.getOrigin()).toBe(CURRENT)
+  })
+
+  it('refuses the change when the teardown throws outright', async () => {
+    const throwing = makeDeps({
       clearDeploymentScopedState: vi.fn(async () => {
         throw new Error('keychain unavailable')
       }),
     })
 
-    const result = await createServerWindow(failing).setOrigin('https://sim.other.example')
+    const result = await createServerWindow(throwing).setOrigin('https://sim.other.example')
 
-    expect(result).toMatchObject({ ok: true, unchanged: false })
-    expect(failing.relaunch).toHaveBeenCalledTimes(1)
+    expect(result).toMatchObject({ ok: false })
+    expect(throwing.relaunch).not.toHaveBeenCalled()
+    expect(throwing.config.getOrigin()).toBe(CURRENT)
   })
 
-  it('surfaces a rejected origin without relaunching', async () => {
+  // Validated up front with the shell's own rule, before anything is torn down
+  // or written, so a typo costs nothing.
+  it('surfaces a rejected origin without tearing anything down', async () => {
     const result = await createServerWindow(deps).setOrigin('ftp://sim.example.com')
 
-    expect(result).toEqual({ ok: false, error: 'bad origin' })
+    expect(result).toMatchObject({ ok: false })
+    expect(result).toHaveProperty('error', expect.stringContaining('HTTPS'))
+    expect(deps.clearDeploymentScopedState).not.toHaveBeenCalled()
     expect(deps.relaunch).not.toHaveBeenCalled()
     expect(deps.config.getOrigin()).toBe(CURRENT)
   })

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

@@ -3,7 +3,7 @@ import { createLogger } from '@sim/logger'
 import { getErrorMessage } from '@sim/utils/errors'
 import { app, BrowserWindow, nativeTheme, session } from 'electron'
 import type { ConfigStore, DesktopSettings } from '@/main/config'
-import { isSimCloudOrigin } from '@/main/config'
+import { canonicalOrigin, isSimCloudOrigin, validateOriginInput } from '@/main/config'
 import {
   backgroundColorFor,
   createSecureWebPreferences,
@@ -54,18 +54,21 @@ export interface ServerWindowDeps {
   isPackaged: boolean
   getParentWindow: () => BrowserWindow | null
   /**
-   * Drops the capabilities the OUTGOING deployment was granted, before the new
-   * one can inherit them.
+   * Drops the capabilities the OUTGOING deployment was granted, and reports
+   * what it could not drop.
    *
    * Local-filesystem grants and the agent browser's cookie jar live in
    * device-global stores with no origin key, and both are capabilities the user
    * handed to a specific Sim server: directories its agent may read, and live
    * third-party sessions its agent may drive. Carrying them across would let
    * the next deployment act with authority it was never given — which is why
-   * sign-out clears exactly this pair. Awaited before the relaunch, since a
-   * quit racing an async clear could leave either behind.
+   * sign-out clears exactly this pair.
+   *
+   * Returns the human-readable name of each store that survived; empty means
+   * everything is gone. Reporting rather than throwing is what lets one store's
+   * failure not hide another's, and lets the caller refuse to move.
    */
-  clearDeploymentScopedState: () => Promise<void>
+  clearDeploymentScopedState: () => Promise<readonly string[]>
   /**
    * Relaunches the shell against the newly stored origin. A full restart rather
    * than an in-place swap: the origin decides the cookie partition, the update
@@ -160,35 +163,50 @@ export function createServerWindow(deps: ServerWindowDeps): ServerWindowHandle {
   }
 
   const setOrigin = async (raw: string): Promise<DesktopServerChangeResult> => {
-    const current = deps.config.getOrigin()
-    const validated = deps.config.setOrigin(raw)
+    const validated = validateOriginInput(raw)
     if (!validated.ok) {
       return validated
     }
-    if (validated.origin === current) {
-      // Nothing moved, so nothing is torn down. Relaunching anyway would make
+    // Same canonicalization the store applies, so the comparison below matches
+    // what would actually be written.
+    const origin = canonicalOrigin(validated.origin)
+    const current = deps.config.getOrigin()
+    if (origin === current) {
+      // Nothing moves, so nothing is torn down. Relaunching anyway would make
       // "confirm the URL I already use" restart the app for no reason.
-      return { ok: true, origin: validated.origin, unchanged: true }
+      return { ok: true, origin, unchanged: true }
+    }
+
+    // Fail closed, and clear BEFORE persisting. If a store cannot be emptied,
+    // the shell must not move: the incoming deployment would otherwise inherit
+    // folder grants and authenticated browser sessions the outgoing one was
+    // given, and they are restored on the next startup. Nothing has been
+    // written at this point, so refusing leaves the shell exactly where it was
+    // rather than stranding it on a half-applied change.
+    const surviving = await deps.clearDeploymentScopedState().catch((error) => {
+      logger.error('Deployment-scoped teardown threw', { error: getErrorMessage(error) })
+      return ['local file access and built-in browser sessions']
+    })
+    if (surviving.length > 0) {
+      logger.error('Refusing to change server; deployment-scoped state survived', { surviving })
+      return {
+        ok: false,
+        error: `Could not clear ${surviving.join(' or ')} from the current server, so the server was not changed. Try again, or sign out first.`,
+      }
     }
-    logger.info('Server origin changed; relaunching', { from: current, to: validated.origin })
+
+    logger.info('Server origin changed; relaunching', { from: current, to: origin })
+    deps.config.setOrigin(raw)
     for (const key of ORIGIN_SCOPED_SETTINGS) {
       deps.config.set(key, undefined)
     }
-    // Failing to clear must not strand the shell on the old origin — that is
-    // already persisted — but it must be loud, because what survives is access
-    // the next deployment did not earn.
-    await deps.clearDeploymentScopedState().catch((error) => {
-      logger.error('Could not clear deployment-scoped state before relaunch', {
-        error: getErrorMessage(error),
-      })
-    })
     // setOrigin writes through immediately; the clears above are debounced like
     // every other setting. `before-quit` flushes too, but doing it here keeps
     // the write independent of the Electron quit sequence.
     deps.config.flush()
     close()
     deps.relaunch()
-    return { ok: true, origin: validated.origin, unchanged: false }
+    return { ok: true, origin, unchanged: false }
   }
 
   return { open, getConfiguration, setOrigin }

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

@@ -49,6 +49,19 @@ describe('resolveDeploymentUrl', () => {
     ).toThrow(SetupError)
   })
 
+  // These spell one deployment four ways. Treating them as a conflict would
+  // demand a --url override to settle an ambiguity that does not exist.
+  it('treats equivalent spellings of one deployment as agreement', () => {
+    expect(
+      resolveDeploymentUrl([
+        source('https://sim.example.com'),
+        source('https://sim.example.com/'),
+        source('https://SIM.example.com'),
+        source('https://sim.example.com:443/workspace'),
+      ])
+    ).toBe('https://sim.example.com')
+  })
+
   it('accepts agreeing sources and an override that settles the ambiguity', () => {
     expect(
       resolveDeploymentUrl([source('https://sim.example.com'), source('https://sim.example.com')])
@@ -147,6 +160,16 @@ describe('sanitizeForTerminal', () => {
     expect(sanitizeForTerminal('Sim-1.2.3-universal.dmg')).toBe('Sim-1.2.3-universal.dmg')
   })
 
+  // Bidi overrides and isolates reorder what the reader sees without emitting a
+  // single control byte, so a range-based C0/C1 filter lets them straight
+  // through — `gpj.dmg` can be made to render as `dmg.jpg`.
+  it('strips bidi controls, not just cursor controls', () => {
+    expect(sanitizeForTerminal('Sim\u202e gmd.eno\u202c.dmg')).toBe('Sim gmd.eno.dmg')
+    for (const control of ['\u200e', '\u200f', '\u202a', '\u202d', '\u2066', '\u2069', '\u061c']) {
+      expect(sanitizeForTerminal(`a${control}b`)).toBe('ab')
+    }
+  })
+
   it('caps a name that would overrun the spinner line', () => {
     const capped = sanitizeForTerminal('x'.repeat(500))
 

packages/sim-setup/src/desktop.ts

@@ -24,19 +24,35 @@ const REDIRECT_STATUSES: ReadonlySet<number> = new Set([301, 302, 307, 308])
 /** The env var every deployment sets to its own public origin. */
 const APP_URL_KEY = 'NEXT_PUBLIC_APP_URL'
 
+/**
+ * The deployment a configured URL names, for comparison only. Unparseable
+ * values fall back to their raw text so they compare equal to themselves and
+ * unequal to everything else.
+ */
+function deploymentKey(value: string): string {
+  try {
+    return new URL(value).origin.toLowerCase()
+  } catch {
+    return value
+  }
+}
+
 /** Keeps a rendered artifact name from overrunning the spinner line. */
 const MAX_INSTALLER_NAME = 120
 
 /**
- * Strips anything that could move the cursor, repaint, or retitle the terminal.
+ * Strips anything that could move the cursor, repaint or retitle the terminal,
+ * or reorder what the reader sees.
  *
  * The artifact name is read out of a redirect the deployment chose, so it is
- * remote input on its way to a TTY: percent-encoded ANSI or OSC bytes survive
- * `decodeURIComponent` as real control characters and would let a compromised
- * deployment forge CLI output. C0 (including ESC), DEL, and C1 all go.
+ * remote input on its way to a TTY, and `decodeURIComponent` turns percent-
+ * encoded bytes into the real characters. Matched by Unicode class rather than
+ * by hand-listed ranges: `Cc` covers C0 (including ESC), DEL, and C1, while
+ * `Cf` covers the bidi overrides and isolates that would otherwise survive and
+ * let a name render in an order it is not written in.
  */
 export function sanitizeForTerminal(value: string): string {
-  return truncate(value.replace(/[\u0000-\u001f\u007f-\u009f]/g, ''), MAX_INSTALLER_NAME)
+  return truncate(value.replace(/[\p{Cc}\p{Cf}]/gu, ''), MAX_INSTALLER_NAME)
 }
 
 export interface DesktopFlags {
@@ -62,20 +78,30 @@ export function resolveDeploymentUrl(
   sources: readonly { label?: string; values?: Map<string, string> | null }[],
   override?: string
 ): string {
-  const discovered = sources.filter((source) => source.values?.get(APP_URL_KEY))
-  const distinct = new Set(discovered.map((source) => source.values?.get(APP_URL_KEY)?.trim()))
-  if (!override && distinct.size > 1) {
+  const discovered = sources.flatMap((source) => {
+    const value = source.values?.get(APP_URL_KEY)?.trim()
+    return value ? [{ label: source.label ?? 'configuration', value }] : []
+  })
+  // Compared on the parsed origin, which is what the command ultimately uses:
+  // a trailing slash, a default port, a different host case, or an ignored path
+  // all name the same deployment, and calling those a conflict would demand a
+  // --url override to resolve an ambiguity that does not exist. A value that
+  // will not parse is its own bucket so it still reaches the error below,
+  // which says something more useful than "these disagree".
+  const byDeployment = new Map<string, string>()
+  for (const { value } of discovered) {
+    byDeployment.set(deploymentKey(value), value)
+  }
+  if (!override && byDeployment.size > 1) {
     throw new SetupError(
-      `Found ${distinct.size} configurations naming different ${APP_URL_KEY} values.`,
+      `Found ${byDeployment.size} configurations naming different ${APP_URL_KEY} values.`,
       [
-        ...discovered.map(
-          (source) => `${source.label ?? 'configuration'}: ${source.values?.get(APP_URL_KEY)}`
-        ),
+        ...discovered.map(({ label, value }) => `${label}: ${value}`),
         'Re-run with --url <deployment url> to say which one the desktop app should use.',
       ]
     )
   }
-  const raw = override ?? discovered[0]?.values?.get(APP_URL_KEY)
+  const raw = override ?? byDeployment.values().next().value
   if (!raw) {
     // A wizard-provisioned local install has the compose interpolation default
     // rather than an explicit value, so an absent key is not a misconfiguration.