diff --git a/config/reliability-gates.jsonc b/config/reliability-gates.jsonc index 2785645d621..01db646c217 100644 --- a/config/reliability-gates.jsonc +++ b/config/reliability-gates.jsonc @@ -3048,6 +3048,105 @@ ], "demotionRule": "Demote or quarantine if the unit gate flakes without a product bug or harness bug filed to the owner." }, + { + "id": "terminal-scroll.streaming-refocus-intent", + "title": "Streaming refocus preserves follow-output viewport intent", + "maturity": "experimental", + "protection": "partial", + "owner": "terminal-rendering", + "layer": "renderer-unit-and-electron-e2e", + "surfaces": [ + "terminal lifecycle", + "window focus recovery", + "hidden-to-visible resume", + "xterm write backlog", + "scrollback" + ], + "platforms": ["macos", "linux", "windows"], + "providers": ["local", "daemon", "ssh", "wsl", "remote-runtime"], + "coveredPlatforms": ["macos"], + "coveredProviders": ["local", "daemon"], + "coverageNotes": "Unit coverage proves provider-independent ordering for any PaneManager. Live Electron coverage exercises a local PTY through the daemon on macOS; SSH, WSL, remote-runtime, Linux, and Windows remain unproved for this exact race.", + "motivatingLinks": [ + "https://github.com/stablyai/orca/issues/11753", + "https://github.com/stablyai/orca/pull/11915" + ], + "invariant": "When output is queued during a focus or visibility transition, Orca records the pre-flush viewport intent before xterm parses backlog writes, so a follow-output terminal stays at the bottom and a pinned terminal keeps its prior position.", + "oracle": "Unit tests require exactly one intent sync before each queued-output flush. The Electron test injects a transient top-of-buffer xterm wobble during refocus and requires every presented scrollbar frame, including the final rendered output, to remain at the bottom.", + "commands": [ + "pnpm exec vitest run --config config/vitest.config.ts src/renderer/src/components/terminal-pane/terminal-visibility-resume.test.ts", + "pnpm exec electron-vite build --mode e2e", + "SKIP_BUILD=1 pnpm exec playwright test tests/e2e/terminal-streaming-refocus-viewport.spec.ts --config tests/playwright.config.ts --project electron-headless --workers=1 --repeat-each=5" + ], + "testFiles": [ + "src/renderer/src/components/terminal-pane/terminal-visibility-resume.test.ts", + "tests/e2e/terminal-streaming-refocus-viewport.spec.ts" + ], + "assertionRefs": [ + { + "file": "src/renderer/src/components/terminal-pane/terminal-visibility-resume.test.ts", + "assertions": [ + "window-wake recovery synchronizes viewport intent exactly once before flushing queued output", + "heavy visibility resume synchronizes intent exactly once before flushing queued output" + ] + }, + { + "file": "tests/e2e/terminal-streaming-refocus-viewport.spec.ts", + "assertions": [ + "phase-one scrollback is visibly ready at the bottom without a fixed sleep", + "no presented animation frame moves the scrollbar thumb away from the bottom during refocus", + "the final streamed marker renders with the visible scrollbar still at the bottom" + ] + } + ], + "evidenceRuns": [ + { + "date": "2026-08-01", + "runner": "local", + "platform": "macos", + "command": "pnpm exec vitest run --config config/vitest.config.ts src/renderer/src/components/terminal-pane/terminal-visibility-resume.test.ts", + "result": "passed", + "durationSeconds": 0.096, + "summary": "14 tests passed, including exact sync count and pre-flush ordering for wake and heavy visibility resume." + }, + { + "date": "2026-08-01", + "runner": "local", + "platform": "macos", + "command": "SKIP_BUILD=1 pnpm exec playwright test tests/e2e/terminal-streaming-refocus-viewport.spec.ts --config tests/playwright.config.ts --project electron-headless --workers=1 --repeat-each=5", + "result": "passed", + "durationSeconds": 35.2, + "summary": "Five consecutive Electron iterations passed after replacing fixed-time readiness and stale tab capture with deterministic viewport and pane-identity oracles." + } + ], + "runtimeBudget": { + "p95Seconds": 30, + "scope": "focused renderer unit test or one Electron E2E iteration" + }, + "flakeHistory": { + "status": "soaking", + "evidence": "Five consecutive local Electron iterations passed; CI soak history is still required before promotion." + }, + "redGreenEvidence": { + "status": "complete", + "evidence": "The controlled xterm viewport wobble reproduces the pinned-top failure with post-flush intent sampling and passes when intent is latched before the queued write flush." + }, + "performanceBudget": { + "required": true, + "evidence": "Wake recovery keeps one O(panes) intent pass before the existing bounded 64 KiB-per-pane flush. Heavy resume removes its second intent pass and keeps the existing bounded 256 KiB-per-pane flush; no polling, timers, subprocesses, IPC, output parsing, fit, or repaint work is added." + }, + "promotionCriteria": [ + "Collect stable CI soak history for the Electron race gate.", + "Run the live oracle on Linux and Windows terminal backends.", + "Add live SSH or remote-runtime coverage for queued output during refocus." + ], + "knownGaps": [ + "The deterministic wobble uses xterm private buffer state and must be updated if that internal contract changes.", + "Live Linux, Windows, SSH, WSL, and remote-runtime execution is not covered for this exact race.", + "The Electron gate proves follow-output behavior; adjacent scroll-intent coverage protects pinned viewport behavior." + ], + "demotionRule": "Demote or quarantine if the Electron oracle flakes without a product bug or harness bug filed to terminal-rendering." + }, { "id": "startup-upgrade.persisted-session-corpus", "title": "Current Orca preserves or recovers old production persisted sessions", diff --git a/electron.vite.config.ts b/electron.vite.config.ts index 02239421ae9..39f923cb215 100644 --- a/electron.vite.config.ts +++ b/electron.vite.config.ts @@ -206,6 +206,8 @@ export const electronViteConfig: UserConfig = { external: isExternalMainModule, input: { index: resolve('src/main/index.ts'), + // Why: sandboxed webview preloads cannot load Rollup helper chunks. + 'browser-window-close-preload': resolve('src/preload/browser-window-close.ts'), 'daemon-entry': resolve('src/main/daemon/daemon-entry.ts'), 'plugin-host-entry': resolve('src/main/plugins/plugin-host-entry.ts'), 'computer-sidecar': resolve('src/main/computer/sidecar-entry.ts'), diff --git a/src/main/browser/browser-manager.test.ts b/src/main/browser/browser-manager.test.ts index 286f172a645..b93a0358537 100644 --- a/src/main/browser/browser-manager.test.ts +++ b/src/main/browser/browser-manager.test.ts @@ -1512,6 +1512,38 @@ describe('browserManager', () => { expect(browserManager.getGuestWebContentsId('browser-destroyed-before-register')).toBeNull() }) + it('removes a destroyed primary guest from its tab registration maps', () => { + const guest = { + id: 306, + isDestroyed: vi.fn(() => false), + getType: vi.fn(() => 'webview'), + setBackgroundThrottling: guestSetBackgroundThrottlingMock, + setWindowOpenHandler: guestSetWindowOpenHandlerMock, + on: guestOnMock, + off: guestOffMock, + openDevTools: guestOpenDevToolsMock + } + webContentsFromIdMock.mockReturnValue(guest) + + browserManager.attachGuestPolicies(guest as never) + browserManager.registerGuest({ + browserPageId: 'browser-destroyed-after-register', + webContentsId: guest.id, + rendererWebContentsId + }) + + const destroyedHandler = guestOnMock.mock.calls.find( + ([event]) => event === 'destroyed' + )?.[1] as (() => void) | undefined + destroyedHandler?.() + + expect(browserManager.getGuestWebContentsId('browser-destroyed-after-register')).toBeNull() + const managerState = browserManager as unknown as { + tabIdByWebContentsId: Map + } + expect(managerState.tabIdByWebContentsId.has(guest.id)).toBe(false) + }) + it('fully unregisters stale guests discovered during authorization', () => { const guest = { id: 305, diff --git a/src/main/browser/browser-manager.ts b/src/main/browser/browser-manager.ts index 7dafca617f2..66f6ac653bf 100644 --- a/src/main/browser/browser-manager.ts +++ b/src/main/browser/browser-manager.ts @@ -640,7 +640,6 @@ export class BrowserManager { // Why: bot detectors probe APIs that differ in Electron webviews; inject overrides each load so manual browsing passes. const disposeAntiDetection = this.injectAntiDetection(guest) - // Why: disable throttling so background screenshots still get frames; else the compositor stalls and capture returns empty. guest.setBackgroundThrottling(false) const installClickedLinkRouting = (): void => { @@ -940,11 +939,15 @@ export class BrowserManager { private retireStaleGuestWebContents(previousWebContentsId: number): void { // Why: after a renderer-process swap, stop the dead guest id resolving to the live page so stale callbacks don't hit the wrong session. this.cleanupGuestPolicyAttachment(previousWebContentsId) - this.tabIdByWebContentsId.delete(previousWebContentsId) } private cleanupGuestPolicyAttachment(guestWebContentsId: number): void { - const isPrimaryGuest = this.tabIdByWebContentsId.has(guestWebContentsId) + const browserTabId = this.tabIdByWebContentsId.get(guestWebContentsId) + const isPrimaryGuest = browserTabId !== undefined + if (browserTabId && this.webContentsIdByTabId.get(browserTabId) === guestWebContentsId) { + this.webContentsIdByTabId.delete(browserTabId) + } + this.tabIdByWebContentsId.delete(guestWebContentsId) this.certificateTrustController?.onGuestRetired(guestWebContentsId) const policyCleanup = this.policyCleanupByGuestId.get(guestWebContentsId) if (policyCleanup) { diff --git a/src/main/persistence-async-write-syscalls.test.ts b/src/main/persistence-async-write-syscalls.test.ts new file mode 100644 index 00000000000..6a4491e8b38 --- /dev/null +++ b/src/main/persistence-async-write-syscalls.test.ts @@ -0,0 +1,496 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { mkdtempSync, readFileSync, readdirSync, rmSync, utimesSync, writeFileSync } from 'node:fs' +import type * as NodeFs from 'node:fs' +import type * as NodeFsPromises from 'node:fs/promises' +import { join } from 'node:path' +import { tmpdir } from 'node:os' + +const testState = { dir: '' } + +const fsCalls = vi.hoisted(() => { + const blocker = new Int32Array(new SharedArrayBuffer(4)) + const calls = { + recording: false, + dirPrefix: '', + /** When > 0, every recorded sync call parks the main thread this long — a stalled mount in miniature. */ + stallMs: 0, + syncCalls: [] as string[], + asyncCalls: [] as string[], + failAsync: null as ((fn: string, target: string) => NodeJS.ErrnoException | null) | null, + beforeAsync: null as ((fn: string, target: string) => void) | null, + waitAsync: null as ((fn: string, target: string) => Promise | null) | null, + reset(): void { + calls.syncCalls.length = 0 + calls.asyncCalls.length = 0 + calls.stallMs = 0 + calls.failAsync = null + calls.beforeAsync = null + calls.waitAsync = null + }, + inScope(target: unknown): target is string { + return ( + calls.recording && + typeof target === 'string' && + calls.dirPrefix !== '' && + target.startsWith(calls.dirPrefix) + ) + }, + recordSync(fn: string, target: unknown): void { + if (!calls.inScope(target)) { + return + } + calls.syncCalls.push(`${fn}:${target}`) + if (calls.stallMs > 0) { + // Atomics.wait blocks the thread the way an uninterruptible syscall does. + Atomics.wait(blocker, 0, 0, calls.stallMs) + } + }, + recordAsync(fn: string, target: unknown): NodeJS.ErrnoException | null { + if (!calls.inScope(target)) { + return null + } + calls.asyncCalls.push(`${fn}:${target}`) + calls.beforeAsync?.(fn, target) + return calls.failAsync?.(fn, target) ?? null + } + } + return calls +}) + +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal() + const patched: Record = { ...actual } + for (const name of Object.keys(actual)) { + const original = (actual as unknown as Record)[name] + if (!name.endsWith('Sync') || typeof original !== 'function') { + continue + } + const fn = original as (...args: unknown[]) => unknown + const wrapper = (...args: unknown[]): unknown => { + fsCalls.recordSync(name, args[0]) + return fn(...args) + } + patched[name] = Object.assign(wrapper, fn) + } + return { ...patched, default: patched } +}) + +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal() + const patched: Record = { ...actual } + for (const name of ['stat', 'access', 'rename', 'copyFile', 'rm', 'mkdir', 'open']) { + const fn = (actual as unknown as Record unknown>)[name] + patched[name] = async (...args: unknown[]): Promise => { + const failure = fsCalls.recordAsync(name, args[0]) + if (failure) { + throw failure + } + if (typeof args[0] === 'string') { + await fsCalls.waitAsync?.(name, args[0]) + } + return fn(...args) + } + } + return { ...patched, default: patched } +}) + +vi.mock('./ssh/ssh-config-parser', () => ({ + loadUserSshConfig: vi.fn(), + sshConfigHostsToTargets: vi.fn() +})) + +vi.mock('./telemetry/client', () => ({ track: vi.fn() })) + +vi.mock('./telemetry/cohort-classifier', () => ({ + getCohortAtEmit: vi.fn().mockReturnValue({ nth_repo_added: 2 }) +})) + +// Deterministic cipher so two stores driven through identical mutations produce identical bytes. +vi.mock('electron', () => ({ + app: { getPath: () => testState.dir }, + safeStorage: { + isEncryptionAvailable: () => true, + encryptString: (plaintext: string) => Buffer.from(`enc:${plaintext}`, 'utf-8'), + decryptString: (ciphertext: Buffer) => ciphertext.toString('utf-8').slice('enc:'.length) + } +})) + +const BACKUP_COUNT = 5 +const BACKUP_MIN_INTERVAL_MS = 60 * 60 * 1000 +const PAST_ROTATION_INTERVAL_MS = BACKUP_MIN_INTERVAL_MS * 2 +const SAVE_DEBOUNCE_MS = 1_000 + +const ROTATION_INTERLEAVE_CASES = [ + ['initial access', 'access', ''], + ['oldest removal', 'rm', '.bak.4'], + ['slot access', 'access', '.bak.0'], + ['slot rename', 'rename', '.bak.0'], + ['final copy', 'copyFile', ''] +] as const + +type TestStore = { + updateUI(updates: { sidebarWidth: number }): void + waitForPendingWrite(): Promise + flushOrThrow(): void +} + +async function createStore(dir: string): Promise { + testState.dir = dir + vi.resetModules() + const { Store, initDataPath } = await import('./persistence') + initDataPath() + return new Store() as unknown as TestStore +} + +function dataFile(dir: string): string { + return join(dir, 'orca-data.json') +} + +function seedStaleBackup(dir: string): void { + const path = `${dataFile(dir)}.bak.0` + writeFileSync(path, '{"stale":true}', 'utf-8') + const staleSeconds = (Date.now() - PAST_ROTATION_INTERVAL_MS) / 1000 + utimesSync(path, staleSeconds, staleSeconds) +} + +function ringSnapshot(dir: string): Record { + const snapshot: Record = {} + for (const name of readdirSync(dir).sort()) { + if (name === 'orca-data.json' || name.startsWith('orca-data.json.bak.')) { + snapshot[name] = readFileSync(join(dir, name), 'utf-8') + } + } + return snapshot +} + +describe('async persistence write path avoids synchronous fs syscalls', () => { + const dirs: string[] = [] + + function makeDir(): string { + const dir = mkdtempSync(join(tmpdir(), 'orca-async-write-')) + dirs.push(dir) + return dir + } + + beforeEach(() => { + fsCalls.recording = false + fsCalls.dirPrefix = '' + fsCalls.reset() + vi.useFakeTimers() + }) + + afterEach(() => { + fsCalls.recording = false + vi.useRealTimers() + while (dirs.length > 0) { + rmSync(dirs.pop() as string, { recursive: true, force: true }) + } + }) + + async function recordAsyncSave( + store: TestStore, + dir: string, + sidebarWidth: number + ): Promise { + store.updateUI({ sidebarWidth }) + fsCalls.dirPrefix = dir + fsCalls.recording = true + try { + vi.advanceTimersByTime(PAST_ROTATION_INTERVAL_MS) + await store.waitForPendingWrite() + } finally { + fsCalls.recording = false + } + } + + it('issues no sync fs syscall under the profile dir when rotation runs with an empty ring', async () => { + const dir = makeDir() + const store = await createStore(dir) + + await recordAsyncSave(store, dir, 301) + + expect(fsCalls.syncCalls).toEqual([]) + // Rotation must actually have happened, else the assertion above is vacuous. + expect(ringSnapshot(dir)['orca-data.json.bak.0']).toBe(readFileSync(dataFile(dir), 'utf-8')) + }) + + it('uses an awaited stat, not statSync, for the backup rotation interval check', async () => { + const dir = makeDir() + const store = await createStore(dir) + seedStaleBackup(dir) + + await recordAsyncSave(store, dir, 302) + + expect(fsCalls.syncCalls).toEqual([]) + expect(fsCalls.asyncCalls).toContain(`stat:${dataFile(dir)}.bak.0`) + }) + + it('issues no sync fs syscall while rotating a saturated ring', async () => { + const dir = makeDir() + const store = await createStore(dir) + + // One more save than the ring holds, so the oldest slot is evicted and every slot renames. + for (let i = 0; i <= BACKUP_COUNT; i++) { + await recordAsyncSave(store, dir, 310 + i) + } + + expect(fsCalls.syncCalls).toEqual([]) + const ring = ringSnapshot(dir) + expect(Object.keys(ring)).toContain(`orca-data.json.bak.${BACKUP_COUNT - 1}`) + expect(Object.keys(ring)).not.toContain(`orca-data.json.bak.${BACKUP_COUNT}`) + }) + + it('lets a main-thread timer keep firing while a rotating save is in flight', async () => { + // A recorded sync call stalls long enough for the heartbeat to catch it. + vi.useRealTimers() + const dir = makeDir() + const store = await createStore(dir) + seedStaleBackup(dir) + // Generous stall so ordinary scheduler/GC jitter can't reach the threshold on a loaded CI box. + const stallMs = 500 + + let lastTick = Date.now() + let worstGapMs = 0 + const heartbeat = setInterval(() => { + const now = Date.now() + worstGapMs = Math.max(worstGapMs, now - lastTick) + lastTick = now + }, 10) + + try { + store.updateUI({ sidebarWidth: 341 }) + fsCalls.dirPrefix = dir + fsCalls.stallMs = stallMs + fsCalls.recording = true + lastTick = Date.now() + await new Promise((resolve) => setTimeout(resolve, SAVE_DEBOUNCE_MS + 200)) + await store.waitForPendingWrite() + } finally { + fsCalls.recording = false + clearInterval(heartbeat) + } + + expect(worstGapMs).toBeLessThan(stallMs) + // The save really happened, so a small gap isn't just an absent write. + expect(ringSnapshot(dir)['orca-data.json.bak.0']).toBe(readFileSync(dataFile(dir), 'utf-8')) + }, 20_000) + + it('skips rotation when a sync flush rotated during the rotation-interval await', async () => { + // The acquired owner keeps rotation when the flush skips the ring. + const dir = makeDir() + const store = await createStore(dir) + seedStaleBackup(dir) + const staleBackup = readFileSync(`${dataFile(dir)}.bak.0`, 'utf-8') + + store.updateUI({ sidebarWidth: 361 }) + fsCalls.dirPrefix = dir + fsCalls.recording = true + let flushed = false + fsCalls.beforeAsync = (fn, target) => { + if (flushed || fn !== 'stat' || !target.endsWith('.bak.0')) { + return + } + flushed = true + store.updateUI({ sidebarWidth: 362 }) + store.flushOrThrow() + } + try { + vi.advanceTimersByTime(PAST_ROTATION_INTERVAL_MS) + await store.waitForPendingWrite() + } finally { + fsCalls.recording = false + fsCalls.beforeAsync = null + } + + expect(flushed).toBe(true) + const ring = ringSnapshot(dir) + expect(ring['orca-data.json.bak.0']).toBe(ring['orca-data.json']) + expect(ring['orca-data.json.bak.1']).toBe(staleBackup) + expect(ring['orca-data.json.bak.2']).toBeUndefined() + }) + + it('keeps the due check inside ownership when a second writer starts', async () => { + const dir = makeDir() + const store = await createStore(dir) + seedStaleBackup(dir) + const almostDueSeconds = (Date.now() - (BACKUP_MIN_INTERVAL_MS - SAVE_DEBOUNCE_MS - 100)) / 1000 + utimesSync(`${dataFile(dir)}.bak.0`, almostDueSeconds, almostDueSeconds) + const staleBackup = readFileSync(`${dataFile(dir)}.bak.0`, 'utf-8') + const statCall = `stat:${dataFile(dir)}.bak.0` + let releaseRotation!: () => void + const rotationRelease = new Promise((resolve) => { + releaseRotation = resolve + }) + let signalRotation!: () => void + const rotationStarted = new Promise((resolve) => { + signalRotation = resolve + }) + let held = false + fsCalls.waitAsync = (fn, target) => { + if (held || fn !== 'stat' || target !== `${dataFile(dir)}.bak.0`) { + return null + } + held = true + signalRotation() + return rotationRelease + } + + fsCalls.dirPrefix = dir + fsCalls.recording = true + store.updateUI({ sidebarWidth: 371 }) + vi.advanceTimersByTime(SAVE_DEBOUNCE_MS) + const firstWrite = store.waitForPendingWrite() + try { + await rotationStarted + store.updateUI({ sidebarWidth: 372 }) + store.flushOrThrow() + store.updateUI({ sidebarWidth: 373 }) + vi.advanceTimersByTime(SAVE_DEBOUNCE_MS) + await store.waitForPendingWrite() + expect(fsCalls.asyncCalls.filter((call) => call === statCall)).toHaveLength(1) + } finally { + releaseRotation() + await firstWrite + fsCalls.recording = false + fsCalls.waitAsync = null + } + const ring = ringSnapshot(dir) + expect(ring['orca-data.json.bak.0']).toBe(ring['orca-data.json']) + expect(ring['orca-data.json.bak.1']).toBe(staleBackup) + expect(ring['orca-data.json.bak.2']).toBeUndefined() + }) + + it.each(ROTATION_INTERLEAVE_CASES)( + 'keeps one rotation owner when a sync flush lands during %s', + async (_phase, expectedFn, targetSuffix) => { + const dir = makeDir() + const store = await createStore(dir) + seedStaleBackup(dir) + const staleBackup = readFileSync(`${dataFile(dir)}.bak.0`, 'utf-8') + const expectedTarget = `${dataFile(dir)}${targetSuffix}` + + store.updateUI({ sidebarWidth: 363 }) + fsCalls.dirPrefix = dir + fsCalls.recording = true + let flushed = false + fsCalls.beforeAsync = (fn, target) => { + if (flushed || fn !== expectedFn || target !== expectedTarget) { + return + } + flushed = true + store.updateUI({ sidebarWidth: 364 }) + store.flushOrThrow() + } + try { + vi.advanceTimersByTime(PAST_ROTATION_INTERVAL_MS) + await store.waitForPendingWrite() + } finally { + fsCalls.recording = false + fsCalls.beforeAsync = null + } + + expect(flushed).toBe(true) + const ring = ringSnapshot(dir) + expect(ring['orca-data.json.bak.0']).toBe(ring['orca-data.json']) + expect(ring['orca-data.json.bak.1']).toBe(staleBackup) + expect(ring['orca-data.json.bak.2']).toBeUndefined() + } + ) + + it('does not log for absent ring slots even when the mount rejects renames non-ENOENT', async () => { + // Probing keeps degraded mounts from logging one error per absent slot. + const dir = makeDir() + const store = await createStore(dir) + const errors = vi.spyOn(console, 'error').mockImplementation(() => {}) + fsCalls.failAsync = (fn, target) => + fn === 'rename' && target.includes('.bak.') + ? Object.assign(new Error('stale NFS file handle'), { code: 'ESTALE' }) + : null + + try { + await recordAsyncSave(store, dir, 351) + expect(errors).not.toHaveBeenCalled() + } finally { + errors.mockRestore() + } + }) + + it('logs when a ring slot that exists fails to rotate', async () => { + const dir = makeDir() + const store = await createStore(dir) + seedStaleBackup(dir) + const errors = vi.spyOn(console, 'error').mockImplementation(() => {}) + fsCalls.failAsync = (fn, target) => + fn === 'rename' && target.endsWith('.bak.0') + ? Object.assign(new Error('permission denied'), { code: 'EPERM' }) + : null + + try { + await recordAsyncSave(store, dir, 352) + expect(errors).toHaveBeenCalledWith( + '[persistence] Failed to rotate backup', + `${dataFile(dir)}.bak.0`, + '->', + `${dataFile(dir)}.bak.1`, + expect.objectContaining({ code: 'EPERM' }) + ) + } finally { + errors.mockRestore() + } + }) + + it('produces a .bak ring byte-identical to the sync path, at capacity and with ring holes', async () => { + const asyncDir = makeDir() + const syncDir = makeDir() + // Match generated IDs so only rotation behavior can differ. + const seedDir = makeDir() + const seedStore = await createStore(seedDir) + seedStore.updateUI({ sidebarWidth: 320 }) + seedStore.flushOrThrow() + const seed = readFileSync(dataFile(seedDir), 'utf-8') + for (const dir of [asyncDir, syncDir]) { + writeFileSync(dataFile(dir), seed, 'utf-8') + // Holes: slots 1 and 3 occupied, 0 and 2 missing — exercises the per-slot existence branch. + writeFileSync(`${dataFile(dir)}.bak.1`, '{"old":1}', 'utf-8') + writeFileSync(`${dataFile(dir)}.bak.3`, '{"old":3}', 'utf-8') + } + + const widths = [321, 322, 323, 324, 325, 326] + const asyncStore = await createStore(asyncDir) + for (const width of widths) { + asyncStore.updateUI({ sidebarWidth: width }) + vi.advanceTimersByTime(PAST_ROTATION_INTERVAL_MS) + await asyncStore.waitForPendingWrite() + } + + const syncStore = await createStore(syncDir) + for (const width of widths) { + syncStore.updateUI({ sidebarWidth: width }) + syncStore.flushOrThrow() + vi.advanceTimersByTime(PAST_ROTATION_INTERVAL_MS) + } + + const ring = ringSnapshot(asyncDir) + expect(ring).toEqual(ringSnapshot(syncDir)) + expect(Object.keys(ring)).toContain(`orca-data.json.bak.${BACKUP_COUNT - 1}`) + }) + + it('keeps the sync quit/crash fallback on synchronous syscalls', async () => { + const dir = makeDir() + const store = await createStore(dir) + seedStaleBackup(dir) + + store.updateUI({ sidebarWidth: 331 }) + fsCalls.dirPrefix = dir + fsCalls.recording = true + try { + store.flushOrThrow() + } finally { + fsCalls.recording = false + } + + expect(fsCalls.syncCalls).toContain(`statSync:${dataFile(dir)}.bak.0`) + expect(fsCalls.syncCalls).toContain(`existsSync:${dataFile(dir)}`) + expect(fsCalls.asyncCalls).toEqual([]) + }) +}) diff --git a/src/main/persistence.ts b/src/main/persistence.ts index c5e4c2a81a9..e5744973f12 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -11,7 +11,7 @@ import { statSync, realpathSync } from 'node:fs' -import { rename, mkdir, rm, copyFile, open } from 'node:fs/promises' +import { rename, mkdir, rm, copyFile, open, stat, access } from 'node:fs/promises' import { renameDurable, writeFileDurableSync } from './durable-file-write' import { join, dirname, isAbsolute, resolve, sep } from 'node:path' import { homedir } from 'node:os' @@ -589,6 +589,14 @@ function backupPath(dataFile: string, index: number): string { return `${dataFile}.bak.${index}` } +/** existsSync's non-blocking twin: existsSync is an access(F_OK) probe, so access() is the exact analogue. */ +async function exists(path: string): Promise { + return access(path).then( + () => true, + () => false + ) +} + function buildWorkspaceDirHistoryForUpdate( current: GlobalSettings, updates: Partial @@ -2742,6 +2750,8 @@ export class Store { private writeTimer: ReturnType | null = null private pendingWrite: Promise | null = null private writeGeneration = 0 + // Prevent a sync flush from interleaving a second rotation with awaited ring mutations. + private backupRotationInFlight = false // Why: after a profile transfer rewrites this file on disk, a late flush of stale in-memory state would resurrect the moved project. private writesFrozen = false // Content hash at last write, to skip no-op writes; derived from the payload with encrypted blobs normalized back to plaintext (see buildStateToSave), since encrypt() uses a random IV per call. @@ -2884,28 +2894,52 @@ export class Store { } } + // Why separate from the sync twin: a statSync here parks the Electron main thread in uninterruptible + // sleep on a stalled SMB/NFS profile mount. Error semantics are deliberately identical: rotate. + private async shouldRotateBackupsAsync(dataFile: string): Promise { + try { + const mtime = (await stat(backupPath(dataFile, 0))).mtimeMs + return Date.now() - mtime >= BACKUP_MIN_INTERVAL_MS + } catch { + return true + } + } + // Why: rotate current file into the .bak ring so load() can recover if a later primary write is truncated or corrupt. private async rotateBackupsAsync(dataFile: string): Promise { - if (!existsSync(dataFile)) { + if (this.backupRotationInFlight) { return } - await rm(backupPath(dataFile, BACKUP_COUNT - 1)).catch((err: unknown) => { - if (err && (err as NodeJS.ErrnoException).code !== 'ENOENT') { - console.error('[persistence] Failed to remove oldest backup:', err) + this.backupRotationInFlight = true + try { + if (!(await this.shouldRotateBackupsAsync(dataFile))) { + return } - }) - for (let i = BACKUP_COUNT - 2; i >= 0; i--) { - const src = backupPath(dataFile, i) - const dst = backupPath(dataFile, i + 1) - if (existsSync(src)) { - await rename(src, dst).catch((err) => { - console.error('[persistence] Failed to rotate backup', src, '->', dst, err) - }) + if (!(await exists(dataFile))) { + return } + await rm(backupPath(dataFile, BACKUP_COUNT - 1)).catch((err: unknown) => { + if (err && (err as NodeJS.ErrnoException).code !== 'ENOENT') { + console.error('[persistence] Failed to remove oldest backup:', err) + } + }) + for (let i = BACKUP_COUNT - 2; i >= 0; i--) { + const src = backupPath(dataFile, i) + const dst = backupPath(dataFile, i + 1) + // Why probe instead of rename-then-swallow-ENOENT: a degraded mount rejects a rename of an + // absent slot with ESTALE/EIO, which would log once per empty slot on every debounced save. + if (await exists(src)) { + await rename(src, dst).catch((err) => { + console.error('[persistence] Failed to rotate backup', src, '->', dst, err) + }) + } + } + await copyFile(dataFile, backupPath(dataFile, 0)).catch((err) => { + console.error('[persistence] Failed to snapshot current file to .bak.0:', err) + }) + } finally { + this.backupRotationInFlight = false } - await copyFile(dataFile, backupPath(dataFile, 0)).catch((err) => { - console.error('[persistence] Failed to snapshot current file to .bak.0:', err) - }) } private rotateBackupsSync(dataFile: string): void { @@ -3886,18 +3920,15 @@ export class Store { await rm(tmpFile).catch(() => {}) } } - // Why (issue #1158): rotate backups only after rename succeeded, and let a concurrent flush own rotation. + // Why (#1158): rotate only after the primary rename while this write still owns its generation. if (this.writeGeneration !== gen) { return } - const now = Date.now() - if (this.shouldRotateBackups(now, dataFile)) { - await this.rotateBackupsAsync(dataFile) - } + await this.rotateBackupsAsync(dataFile) } // Why: sync variant only for flush() at shutdown, where the process may exit before an async write completes. - private writeToDiskSync(opts: { force?: boolean } = {}): void { + private writeToDiskSync(opts: { force?: boolean; skipBackupRotation?: boolean } = {}): void { if (this.writesFrozen) { return } @@ -3931,7 +3962,7 @@ export class Store { } } const now = Date.now() - if (this.shouldRotateBackups(now, dataFile)) { + if (!opts.skipBackupRotation && this.shouldRotateBackups(now, dataFile)) { this.rotateBackupsSync(dataFile) } } @@ -3946,7 +3977,10 @@ export class Store { // Why: bump writeGeneration so an in-flight async write skips its rename and can't overwrite this sync write. this.writeGeneration++ this.pendingWrite = null - this.writeToDiskSync({ force: asyncWriteWasInFlight }) + this.writeToDiskSync({ + force: asyncWriteWasInFlight, + skipBackupRotation: this.backupRotationInFlight + }) } flushActiveViewPreferenceOrThrow(): void { diff --git a/src/main/ssh/ssh-auth-resolution.ts b/src/main/ssh/ssh-auth-resolution.ts index 025c8904870..c7e65e26d92 100644 --- a/src/main/ssh/ssh-auth-resolution.ts +++ b/src/main/ssh/ssh-auth-resolution.ts @@ -8,18 +8,26 @@ import { isOpenSshConfigBackedTarget } from './system-ssh-args' // Why: ssh2 only tries keys that are explicitly provided. Users with keys in // standard locations (e.g. ~/.ssh/id_ed25519) but no SSH agent running would -// fail to authenticate. Probing default paths matches VS Code's _findDefaultKeyFile. +// fail to authenticate. Probe the regular and FIDO2 OpenSSH default paths. const DEFAULT_KEY_NAMES = ['id_ed25519', 'id_rsa', 'id_ecdsa', 'id_dsa', 'id_xmss'] +const DEFAULT_SECURITY_KEY_NAMES = ['id_ed25519_sk', 'id_ecdsa_sk'] const DEFAULT_KEY_PATHS = DEFAULT_KEY_NAMES.map((name) => `~/.ssh/${name}`) +const DEFAULT_IDENTITY_PATHS = [...DEFAULT_KEY_NAMES, ...DEFAULT_SECURITY_KEY_NAMES].map( + (name) => `~/.ssh/${name}` +) const WINDOWS_OPENSSH_AGENT_PIPE = '\\\\.\\pipe\\openssh-ssh-agent' // Why: resolved IdentityFile paths are expanded before auth resolution, so they // won't match the ~/... form in DEFAULT_KEY_PATHS. -const EXPANDED_DEFAULT_KEY_PATHS = DEFAULT_KEY_PATHS.map(resolveSshConfigHomePath) +const EXPANDED_DEFAULT_KEY_PATHS = DEFAULT_IDENTITY_PATHS.map(resolveSshConfigHomePath) export type PrivateKeyFile = { path: string; contents: Buffer } +export function listDefaultIdentityFilePaths(): string[] { + return [...DEFAULT_IDENTITY_PATHS] +} + export function findDefaultKeyFile(): PrivateKeyFile | undefined { for (const keyPath of DEFAULT_KEY_PATHS) { const resolved = resolveSshConfigHomePath(keyPath) @@ -97,7 +105,10 @@ function resolveExplicitPrivateKeyPaths( return resolvedIdentities } -function resolvePrivateKeyPaths(target: SshTarget, resolved: SshResolvedConfig | null): string[] { +export function resolveIdentityFilePaths( + target: SshTarget, + resolved: Pick | null +): string[] { if (isOpenSshConfigBackedTarget(target) && resolved) { return resolved.identityFile } @@ -138,7 +149,7 @@ export function resolvePrivateKeys( target: SshTarget, resolved: SshResolvedConfig | null ): PrivateKeyFile[] { - const keyPaths = resolvePrivateKeyPaths(target, resolved) + const keyPaths = resolveIdentityFilePaths(target, resolved) if (keyPaths.length > 0 || resolved || target.identityFile) { return readPrivateKeys(keyPaths) } @@ -174,16 +185,6 @@ export function findEncryptedPrivateKeyPath(keys: PrivateKeyFile[]): string | un return undefined } -function resolveIdentityFilePaths(target: SshTarget, resolved: SshResolvedConfig | null): string[] { - if (isOpenSshConfigBackedTarget(target) && resolved) { - return resolved.identityFile - } - if (target.identityFile) { - return [target.identityFile] - } - return resolved?.identityFile ?? [] -} - export function resolveAgentConfigValue( agentSocket: string, target: SshTarget, diff --git a/src/main/ssh/ssh-connection-utils.test.ts b/src/main/ssh/ssh-connection-utils.test.ts index a5dd743fd43..5e06a9e7386 100644 --- a/src/main/ssh/ssh-connection-utils.test.ts +++ b/src/main/ssh/ssh-connection-utils.test.ts @@ -313,7 +313,7 @@ describe('findDefaultKeyFile', () => { expect(result!.contents).toEqual(Buffer.from('key-contents')) }) - it('probes keys in VS Code order: ed25519, rsa, ecdsa, dsa, xmss', () => { + it('probes regular and FIDO2 keys in stable default order', () => { const checkedPaths: string[] = [] mockExistsSync.mockImplementation((path: unknown) => { checkedPaths.push(String(path)) @@ -331,6 +331,34 @@ describe('findDefaultKeyFile', () => { ]) }) + it('keeps a regular default ahead of a malformed FIDO2 default', () => { + mockExistsSync.mockImplementation((path: unknown) => { + return ( + path === testHomePath('.ssh', 'id_rsa') || path === testHomePath('.ssh', 'id_ed25519_sk') + ) + }) + mockReadFileSync.mockImplementation((path: unknown) => { + if (String(path) === testHomePath('.ssh', 'id_ed25519_sk')) { + throw new Error('malformed FIDO2 key') + } + return Buffer.from('rsa-key') + }) + + expect(findDefaultKeyFile()).toEqual({ + path: '~/.ssh/id_rsa', + contents: Buffer.from('rsa-key') + }) + }) + + it('leaves FIDO2 defaults out of the ssh2 private-key fallback', () => { + mockExistsSync.mockImplementation((path: unknown) => { + return path === testHomePath('.ssh', 'id_ed25519_sk') + }) + + expect(findDefaultKeyFile()).toBeUndefined() + expect(mockReadFileSync).not.toHaveBeenCalled() + }) + it('skips unreadable key files and tries next', () => { mockExistsSync.mockImplementation((path: unknown) => { return path === testHomePath('.ssh', 'id_ed25519') || path === testHomePath('.ssh', 'id_rsa') diff --git a/src/main/ssh/ssh-connection.test.ts b/src/main/ssh/ssh-connection.test.ts index b73cc1e1db3..a3bcadf66dc 100644 --- a/src/main/ssh/ssh-connection.test.ts +++ b/src/main/ssh/ssh-connection.test.ts @@ -16,6 +16,7 @@ let execBehavior: 'callback' | 'pending' = 'callback' let pendingExecCallback: ((err: Error | undefined, channel: unknown) => void) | null = null let sftpBehavior: 'callback' | 'pending' = 'callback' let pendingSftpCallback: ((err: Error | undefined, channel: unknown) => void) | null = null +let notifyClientCreated: (() => void) | undefined type MockSshClient = { setNoDelay: ReturnType @@ -45,6 +46,8 @@ vi.mock('ssh2', () => { lastConnectConfig?: unknown constructor() { clientInstances.push(this) + notifyClientCreated?.() + notifyClientCreated = undefined } on(event: string, handler: (...args: unknown[]) => void) { const handlers = eventHandlers?.get(event) ?? new Set<(...args: unknown[]) => void>() @@ -168,6 +171,10 @@ import { } from './ssh-system-fallback' import { getRemoteHostPlatform } from './ssh-remote-platform' import type { SshTarget } from '../../shared/ssh-types' +import { + createOpenSshPrivateKeyFixture, + createOpenSshPublicKeyFixture +} from './ssh-security-key-identity.test-fixture' function createTarget(overrides?: Partial): SshTarget { return { @@ -278,6 +285,7 @@ describe('SshConnection', () => { pendingExecCallback = null sftpBehavior = 'callback' pendingSftpCallback = null + notifyClientCreated = undefined clientInstances = [] getOrcaControlSocketPathMock.mockReset() getOrcaControlSocketPathMock.mockReturnValue(null) @@ -462,10 +470,11 @@ describe('SshConnection', () => { const callbacks = createCallbacks() const conn = new SshConnection(createTarget(), callbacks) + const clientCreated = new Promise((resolve) => { + notifyClientCreated = resolve + }) const connectResult = conn.connect().catch((error: Error) => error) - for (let i = 0; i < 5 && clientInstances.length === 0; i++) { - await Promise.resolve() - } + await clientCreated expect(clientInstances).toHaveLength(1) await conn.disconnect() @@ -485,10 +494,11 @@ describe('SshConnection', () => { const callbacks = createCallbacks() const conn = new SshConnection(createTarget(), callbacks) + const clientCreated = new Promise((resolve) => { + notifyClientCreated = resolve + }) const connectResult = conn.connect().catch((error: Error) => error) - for (let i = 0; i < 5 && clientInstances.length === 0; i++) { - await Promise.resolve() - } + await clientCreated expect(clientInstances).toHaveLength(1) await conn.disconnect() @@ -1462,6 +1472,47 @@ describe('SshConnection', () => { ) }) + it('uses system SSH before ssh2 parses a security-key private key', async () => { + const directory = mkdtempSync(join(tmpdir(), 'orca-security-key-connect-')) + const keyPath = join(directory, 'id_ed25519_sk') + writeFileSync( + keyPath, + createOpenSshPrivateKeyFixture(['sk-ssh-ed25519@openssh.com'], { encrypted: true }) + ) + const conn = new SshConnection(createTarget({ identityFile: keyPath }), createCallbacks()) + + try { + await conn.connect() + + expect(conn.getState().status).toBe('connected') + expect(conn.usesSystemSshTransport()).toBe(true) + expect(clientInstances).toHaveLength(0) + expect(spawnSystemSshCommandMock).toHaveBeenCalledTimes(1) + } finally { + rmSync(directory, { recursive: true }) + } + }) + + it('uses system SSH for an agent-backed security-key public identity', async () => { + const directory = mkdtempSync(join(tmpdir(), 'orca-security-key-agent-connect-')) + const identityPath = join(directory, 'id_ed25519_sk') + writeFileSync( + `${identityPath}.pub`, + createOpenSshPublicKeyFixture('sk-ssh-ed25519@openssh.com') + ) + const conn = new SshConnection(createTarget({ identityFile: identityPath }), createCallbacks()) + + try { + await conn.connect() + + expect(conn.usesSystemSshTransport()).toBe(true) + expect(clientInstances).toHaveLength(0) + expect(spawnSystemSshCommandMock).toHaveBeenCalledTimes(1) + } finally { + rmSync(directory, { recursive: true }) + } + }) + it('falls back to system SSH when ssh2 hits a local network policy reachability error', async () => { connectBehavior = 'error' connectErrorMessage = 'connect EHOSTUNREACH 192.168.0.210:22 - Local (192.168.0.2:52112)' diff --git a/src/main/ssh/ssh-connection.ts b/src/main/ssh/ssh-connection.ts index 17f0a382251..37d661587ad 100644 --- a/src/main/ssh/ssh-connection.ts +++ b/src/main/ssh/ssh-connection.ts @@ -41,6 +41,10 @@ import { type SshConnectionCallbacks } from './ssh-connection-utils' import { getPassphrasePrivateKeyPath } from './ssh-private-key-authentication' +import { + requiresSystemSshForSecurityKey, + shouldUseSystemSshTransport +} from './ssh-transport-selection' import type { RemoteHostPlatform } from './ssh-remote-platform' import { resolveSftpTransferPathIfMapped, @@ -636,7 +640,14 @@ export class SshConnection { const resolved = await resolveWithSshG(this.target.configHost || this.target.label).catch( () => null ) - if (shouldUseSystemSshTransport(this.target, resolved)) { + const usesConfiguredSystemTransport = shouldUseSystemSshTransport(this.target, resolved) + const requiresSecurityKeyTransport = usesConfiguredSystemTransport + ? false + : await requiresSystemSshForSecurityKey(this.target, resolved) + if (!this.isCurrentConnectAttempt(connectGeneration)) { + throw this.createCancelledConnectAttemptError() + } + if (usesConfiguredSystemTransport || requiresSecurityKeyTransport) { await this.doSystemSshProbeWithControlMasterRetry(connectGeneration, resolved) return } @@ -1386,24 +1397,4 @@ export class SshConnection { } } -export function shouldUseSystemSshTransport( - target: SshTarget, - resolved: Pick | null -): boolean { - if (isOpenSshConfigBackedTarget(target) && resolved) { - return ( - process.env.ORCA_SSH_FORCE_SYSTEM_TRANSPORT === '1' || - resolved.proxyUseFdpass === true || - resolved.proxyCommand != null || - resolved.proxyJump != null - ) - } - return ( - process.env.ORCA_SSH_FORCE_SYSTEM_TRANSPORT === '1' || - target.proxyCommand != null || - target.jumpHost != null || - resolved?.proxyUseFdpass === true || - resolved?.proxyCommand != null || - resolved?.proxyJump != null - ) -} +export { shouldUseSystemSshTransport } from './ssh-transport-selection' diff --git a/src/main/ssh/ssh-security-key-identity.test-fixture.ts b/src/main/ssh/ssh-security-key-identity.test-fixture.ts new file mode 100644 index 00000000000..0a8bcbd58f4 --- /dev/null +++ b/src/main/ssh/ssh-security-key-identity.test-fixture.ts @@ -0,0 +1,42 @@ +function encodeUint32(value: number): Buffer { + const buffer = Buffer.alloc(4) + buffer.writeUInt32BE(value) + return buffer +} + +function sshString(value: string | Buffer): Buffer { + const contents = typeof value === 'string' ? Buffer.from(value, 'ascii') : value + return Buffer.concat([encodeUint32(contents.length), contents]) +} + +export function createOpenSshPrivateKeyFixture( + keyTypes: string[], + options: { encrypted?: boolean; cipher?: string; privateBlock?: Buffer; authTag?: Buffer } = {} +): Buffer { + const cipher = options.cipher ?? (options.encrypted ? 'aes256-ctr' : 'none') + const encrypted = cipher !== 'none' + const publicKeys = keyTypes.map((keyType) => sshString(sshString(keyType))) + const decoded = Buffer.concat([ + Buffer.from('openssh-key-v1\0', 'ascii'), + sshString(cipher), + sshString(encrypted ? 'bcrypt' : 'none'), + sshString(encrypted ? Buffer.from('fixture-kdf') : Buffer.alloc(0)), + encodeUint32(publicKeys.length), + ...publicKeys, + sshString(options.privateBlock ?? Buffer.from('fixture-private-block')), + options.authTag ?? Buffer.alloc(0) + ]) + const encoded = + decoded + .toString('base64') + .match(/.{1,70}/g) + ?.join('\n') ?? '' + return Buffer.from( + `-----BEGIN OPENSSH PRIVATE KEY-----\n${encoded}\n-----END OPENSSH PRIVATE KEY-----\n` + ) +} + +export function createOpenSshPublicKeyFixture(keyType: string): Buffer { + const encoded = sshString(keyType).toString('base64') + return Buffer.from(`${keyType} ${encoded} fixture-comment\n`) +} diff --git a/src/main/ssh/ssh-security-key-identity.test.ts b/src/main/ssh/ssh-security-key-identity.test.ts new file mode 100644 index 00000000000..c424bfa6891 --- /dev/null +++ b/src/main/ssh/ssh-security-key-identity.test.ts @@ -0,0 +1,309 @@ +import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { SshTarget } from '../../shared/ssh-types' +import { + isOpenSshSecurityKeyPrivateKey, + isOpenSshSecurityKeyPublicKey +} from './ssh-security-key-identity' +import { + createOpenSshPrivateKeyFixture, + createOpenSshPublicKeyFixture +} from './ssh-security-key-identity.test-fixture' +import { requiresSystemSshForSecurityKey } from './ssh-transport-selection' + +const { findSystemSshMock } = vi.hoisted(() => ({ findSystemSshMock: vi.fn() })) + +vi.mock('./system-ssh-binary', () => ({ findSystemSsh: findSystemSshMock })) + +vi.mock('node:os', async (importOriginal) => { + const actual = await importOriginal<{ homedir: () => string }>() + return { + ...actual, + homedir: () => process.env.ORCA_TEST_SSH_HOME || actual.homedir() + } +}) + +const ED25519_SECURITY_KEY = 'sk-ssh-ed25519@openssh.com' +const ECDSA_SECURITY_KEY = 'sk-ecdsa-sha2-nistp256@openssh.com' +const tempDirs: string[] = [] + +function createTarget(overrides: Partial = {}): SshTarget { + return { + id: 'target-1', + label: 'Test Server', + host: 'example.com', + port: 22, + username: 'deploy', + ...overrides + } +} + +beforeEach(() => { + findSystemSshMock.mockReset() + findSystemSshMock.mockReturnValue('/usr/bin/ssh') +}) + +async function writeKey(contents: Buffer, filename = 'security key'): Promise { + const directory = await mkdtemp(join(tmpdir(), 'orca-security-key-')) + tempDirs.push(directory) + const keyPath = join(directory, filename) + await writeFile(keyPath, contents) + return keyPath +} + +afterEach(async () => { + vi.unstubAllEnvs() + await Promise.all(tempDirs.splice(0).map((directory) => rm(directory, { recursive: true }))) +}) + +describe('isOpenSshSecurityKeyPrivateKey', () => { + it.each([ED25519_SECURITY_KEY, ECDSA_SECURITY_KEY])( + 'recognizes unencrypted %s keys', + (keyType) => { + expect(isOpenSshSecurityKeyPrivateKey(createOpenSshPrivateKeyFixture([keyType]))).toBe(true) + } + ) + + it('recognizes authenticated encrypted envelopes with a trailing tag', () => { + const key = createOpenSshPrivateKeyFixture([ED25519_SECURITY_KEY], { + cipher: 'aes256-gcm@openssh.com', + authTag: Buffer.alloc(16, 7) + }) + expect(isOpenSshSecurityKeyPrivateKey(key)).toBe(true) + }) + + it.each([ED25519_SECURITY_KEY, ECDSA_SECURITY_KEY])( + 'recognizes encrypted %s keys from the public section', + (keyType) => { + const key = createOpenSshPrivateKeyFixture([keyType], { encrypted: true }) + expect(isOpenSshSecurityKeyPrivateKey(key)).toBe(true) + } + ) + + it.each(['ssh-ed25519', 'ecdsa-sha2-nistp256', 'ssh-rsa'])( + 'leaves regular %s keys on ssh2', + (keyType) => { + expect(isOpenSshSecurityKeyPrivateKey(createOpenSshPrivateKeyFixture([keyType]))).toBe(false) + } + ) + + it('supports CRLF armored keys', () => { + const key = createOpenSshPrivateKeyFixture([ED25519_SECURITY_KEY]) + expect( + isOpenSshSecurityKeyPrivateKey(Buffer.from(key.toString().replaceAll('\n', '\r\n'))) + ).toBe(true) + }) + + it('recognizes OpenSSH envelopes without optional base64 padding', () => { + const key = createOpenSshPrivateKeyFixture([ED25519_SECURITY_KEY], { + privateBlock: Buffer.alloc(0) + }) + const unpadded = Buffer.from(key.toString().replace(/=+(?=\n-----END)/, '')) + expect(unpadded).not.toEqual(key) + expect(isOpenSshSecurityKeyPrivateKey(unpadded)).toBe(true) + }) + + it('does not match security-key text outside a valid public-key type', () => { + const key = createOpenSshPrivateKeyFixture(['ssh-ed25519'], { + privateBlock: Buffer.from(ED25519_SECURITY_KEY) + }) + expect(isOpenSshSecurityKeyPrivateKey(key)).toBe(false) + expect( + isOpenSshSecurityKeyPrivateKey(Buffer.from(`${ED25519_SECURITY_KEY} AAAA comment`)) + ).toBe(false) + }) + + it.each([ED25519_SECURITY_KEY, ECDSA_SECURITY_KEY])( + 'validates the %s type inside an OpenSSH public key blob', + (keyType) => { + expect(isOpenSshSecurityKeyPublicKey(createOpenSshPublicKeyFixture(keyType))).toBe(true) + } + ) + + it('recognizes public keys without optional base64 padding', () => { + const key = createOpenSshPublicKeyFixture(ECDSA_SECURITY_KEY) + const unpadded = Buffer.from(key.toString().replace(/=+(?=\s)/, '')) + expect(unpadded).not.toEqual(key) + expect(isOpenSshSecurityKeyPublicKey(unpadded)).toBe(true) + }) + + it('rejects regular or mismatched OpenSSH public key blobs', () => { + expect(isOpenSshSecurityKeyPublicKey(createOpenSshPublicKeyFixture('ssh-ed25519'))).toBe(false) + expect( + isOpenSshSecurityKeyPublicKey( + Buffer.from(`${ED25519_SECURITY_KEY} ${Buffer.from('ssh-ed25519').toString('base64')}`) + ) + ).toBe(false) + }) + + it('rejects malformed and truncated OpenSSH envelopes without throwing', () => { + const key = createOpenSshPrivateKeyFixture([ED25519_SECURITY_KEY]) + const malformedLength = Buffer.concat([ + Buffer.from('openssh-key-v1\0', 'ascii'), + Buffer.from([0xff, 0xff, 0xff, 0xff]) + ]).toString('base64') + const malformedKey = Buffer.from( + `-----BEGIN OPENSSH PRIVATE KEY-----\n${malformedLength}\n-----END OPENSSH PRIVATE KEY-----\n` + ) + expect(isOpenSshSecurityKeyPrivateKey(key.subarray(0, -20))).toBe(false) + expect(isOpenSshSecurityKeyPrivateKey(malformedKey)).toBe(false) + expect(isOpenSshSecurityKeyPrivateKey(Buffer.from('not a private key'))).toBe(false) + }) +}) + +describe('requiresSystemSshForSecurityKey', () => { + it('uses default FIDO2 identities only when config resolution is unavailable', async () => { + const directory = await mkdtemp(join(tmpdir(), 'orca-security-key-home-')) + tempDirs.push(directory) + await mkdir(join(directory, '.ssh')) + await writeFile( + join(directory, '.ssh', 'id_ed25519_sk'), + createOpenSshPrivateKeyFixture([ED25519_SECURITY_KEY]) + ) + vi.stubEnv('ORCA_TEST_SSH_HOME', directory) + + await expect(requiresSystemSshForSecurityKey(createTarget(), null)).resolves.toBe(true) + await expect( + requiresSystemSshForSecurityKey(createTarget(), { identityFile: [] }) + ).resolves.toBe(false) + }) + + it('keeps a regular default ahead of a dormant FIDO2 identity', async () => { + const directory = await mkdtemp(join(tmpdir(), 'orca-regular-key-home-')) + tempDirs.push(directory) + await mkdir(join(directory, '.ssh')) + await writeFile(join(directory, '.ssh', 'id_rsa'), createOpenSshPrivateKeyFixture(['ssh-rsa'])) + await writeFile( + join(directory, '.ssh', 'id_ed25519_sk'), + createOpenSshPrivateKeyFixture([ED25519_SECURITY_KEY]) + ) + vi.stubEnv('ORCA_TEST_SSH_HOME', directory) + + await expect(requiresSystemSshForSecurityKey(createTarget(), null)).resolves.toBe(false) + }) + + it('keeps password and agent fallback when default FIDO2 needs unavailable OpenSSH', async () => { + const directory = await mkdtemp(join(tmpdir(), 'orca-no-system-ssh-home-')) + tempDirs.push(directory) + await mkdir(join(directory, '.ssh')) + await writeFile( + join(directory, '.ssh', 'id_ed25519_sk'), + createOpenSshPrivateKeyFixture([ED25519_SECURITY_KEY]) + ) + vi.stubEnv('ORCA_TEST_SSH_HOME', directory) + findSystemSshMock.mockReturnValue(null) + + await expect(requiresSystemSshForSecurityKey(createTarget(), null)).resolves.toBe(false) + }) + + it('ignores an orphan regular sidecar before a valid default FIDO2 identity', async () => { + const directory = await mkdtemp(join(tmpdir(), 'orca-orphan-sidecar-home-')) + tempDirs.push(directory) + await mkdir(join(directory, '.ssh')) + await writeFile( + join(directory, '.ssh', 'id_ed25519.pub'), + createOpenSshPublicKeyFixture('ssh-ed25519') + ) + await writeFile( + join(directory, '.ssh', 'id_ed25519_sk'), + createOpenSshPrivateKeyFixture([ED25519_SECURITY_KEY]) + ) + vi.stubEnv('ORCA_TEST_SSH_HOME', directory) + + await expect(requiresSystemSshForSecurityKey(createTarget(), null)).resolves.toBe(true) + }) + + it('detects a manual target identity path with spaces', async () => { + const keyPath = await writeKey(createOpenSshPrivateKeyFixture([ED25519_SECURITY_KEY])) + await expect( + requiresSystemSshForSecurityKey(createTarget({ identityFile: keyPath }), null) + ).resolves.toBe(true) + }) + + it('checks every fresh resolved identity for config-backed targets', async () => { + const regularKey = await writeKey(createOpenSshPrivateKeyFixture(['ssh-ed25519']), 'regular') + const securityKey = await writeKey( + createOpenSshPrivateKeyFixture([ECDSA_SECURITY_KEY], { encrypted: true }), + 'security' + ) + const target = createTarget({ + source: 'ssh-config', + configHost: 'workbox', + identityFile: '/stale/security-key' + }) + + await expect( + requiresSystemSshForSecurityKey(target, { identityFile: [regularKey, securityKey] }) + ).resolves.toBe(true) + }) + + it.each([ED25519_SECURITY_KEY, ECDSA_SECURITY_KEY])( + 'detects an agent-backed %s identity from its public sidecar', + async (keyType) => { + const directory = await mkdtemp(join(tmpdir(), 'orca-security-key-agent-')) + tempDirs.push(directory) + const identityPath = join(directory, 'agent-key') + await writeFile(`${identityPath}.pub`, createOpenSshPublicKeyFixture(keyType)) + + await expect( + requiresSystemSshForSecurityKey(createTarget({ identityFile: identityPath }), null) + ).resolves.toBe(true) + } + ) + + it('ignores a stale FIDO2 sidecar beside a regular private identity', async () => { + const identityPath = await writeKey( + createOpenSshPrivateKeyFixture(['ssh-ed25519']), + 'regular-with-stale-sidecar' + ) + await writeFile(`${identityPath}.pub`, createOpenSshPublicKeyFixture(ED25519_SECURITY_KEY)) + + await expect( + requiresSystemSshForSecurityKey(createTarget({ identityFile: identityPath }), null) + ).resolves.toBe(false) + }) + + it('ignores stale imported identity paths when fresh config has regular keys', async () => { + const staleKey = await writeKey(createOpenSshPrivateKeyFixture([ED25519_SECURITY_KEY]), 'stale') + const regularKey = await writeKey(createOpenSshPrivateKeyFixture(['ssh-ed25519']), 'regular') + const target = createTarget({ + source: 'ssh-config', + configHost: 'workbox', + identityFile: staleKey + }) + + await expect( + requiresSystemSshForSecurityKey(target, { identityFile: [regularKey] }) + ).resolves.toBe(false) + }) + + it('keeps a manual target identity authoritative over resolved defaults', async () => { + const regularKey = await writeKey(createOpenSshPrivateKeyFixture(['ssh-ed25519']), 'manual') + const securityKey = await writeKey( + createOpenSshPrivateKeyFixture([ED25519_SECURITY_KEY]), + 'resolved' + ) + + await expect( + requiresSystemSshForSecurityKey( + createTarget({ source: 'manual', identityFile: regularKey }), + { identityFile: [securityKey] } + ) + ).resolves.toBe(false) + }) + + it('degrades to ssh2 when identity files are missing or malformed', async () => { + const malformedKey = await writeKey(Buffer.from('not a key'), 'malformed') + await expect( + requiresSystemSshForSecurityKey(createTarget({ identityFile: malformedKey }), null) + ).resolves.toBe(false) + await expect( + requiresSystemSshForSecurityKey( + createTarget({ identityFile: join(tmpdir(), 'missing-security-key') }), + null + ) + ).resolves.toBe(false) + }) +}) diff --git a/src/main/ssh/ssh-security-key-identity.ts b/src/main/ssh/ssh-security-key-identity.ts new file mode 100644 index 00000000000..bb1af581d7d --- /dev/null +++ b/src/main/ssh/ssh-security-key-identity.ts @@ -0,0 +1,107 @@ +const OPENSSH_PRIVATE_KEY_HEADER = '-----BEGIN OPENSSH PRIVATE KEY-----' +const OPENSSH_PRIVATE_KEY_FOOTER = '-----END OPENSSH PRIVATE KEY-----' +const OPENSSH_KEY_MAGIC = Buffer.from('openssh-key-v1\0', 'ascii') +export const MAX_SSH_IDENTITY_FILE_BYTES = 1024 * 1024 +const MAX_PUBLIC_KEYS = 64 +const SECURITY_KEY_TYPES = new Set([ + 'sk-ssh-ed25519@openssh.com', + 'sk-ecdsa-sha2-nistp256@openssh.com' +]) + +type SshString = { value: Buffer; nextOffset: number } + +function decodeBase64(value: string): Buffer | null { + if (!value || value.length % 4 === 1 || !/^[A-Za-z0-9+/]+={0,2}$/.test(value)) { + return null + } + const unpadded = value.replace(/=+$/, '') + const decoded = Buffer.from(unpadded, 'base64') + return decoded.toString('base64').replace(/=+$/, '') === unpadded ? decoded : null +} + +function readSshString(buffer: Buffer, offset: number): SshString | null { + if (offset < 0 || offset + 4 > buffer.length) { + return null + } + const length = buffer.readUInt32BE(offset) + const start = offset + 4 + const end = start + length + if (end < start || end > buffer.length) { + return null + } + return { value: buffer.subarray(start, end), nextOffset: end } +} + +function decodeOpenSshPrivateKey(contents: Buffer): Buffer | null { + if (contents.length > MAX_SSH_IDENTITY_FILE_BYTES) { + return null + } + const lines = contents.toString('ascii').trim().split(/\r?\n/) + if (lines[0] !== OPENSSH_PRIVATE_KEY_HEADER || lines.at(-1) !== OPENSSH_PRIVATE_KEY_FOOTER) { + return null + } + const encoded = lines.slice(1, -1).join('') + const decoded = decodeBase64(encoded) + if (!decoded) { + return null + } + return decoded.subarray(0, OPENSSH_KEY_MAGIC.length).equals(OPENSSH_KEY_MAGIC) ? decoded : null +} + +export function isOpenSshSecurityKeyPrivateKey(contents: Buffer): boolean { + const decoded = decodeOpenSshPrivateKey(contents) + if (!decoded) { + return false + } + + let offset = OPENSSH_KEY_MAGIC.length + for (let field = 0; field < 3; field++) { + const value = readSshString(decoded, offset) + if (!value) { + return false + } + offset = value.nextOffset + } + if (offset + 4 > decoded.length) { + return false + } + + const keyCount = decoded.readUInt32BE(offset) + offset += 4 + if (keyCount === 0 || keyCount > MAX_PUBLIC_KEYS) { + return false + } + + let hasSecurityKey = false + for (let keyIndex = 0; keyIndex < keyCount; keyIndex++) { + const publicKey = readSshString(decoded, offset) + if (!publicKey) { + return false + } + offset = publicKey.nextOffset + const keyType = readSshString(publicKey.value, 0) + if (!keyType) { + return false + } + hasSecurityKey ||= SECURITY_KEY_TYPES.has(keyType.value.toString('ascii')) + } + + const privateBlock = readSshString(decoded, offset) + return privateBlock !== null && hasSecurityKey +} + +export function isOpenSshSecurityKeyPublicKey(contents: Buffer): boolean { + if (contents.length > MAX_SSH_IDENTITY_FILE_BYTES) { + return false + } + const [declaredType, encoded] = contents.toString('ascii').trim().split(/\s+/, 3) + if (!declaredType || !encoded || !SECURITY_KEY_TYPES.has(declaredType)) { + return false + } + const decoded = decodeBase64(encoded) + if (!decoded) { + return false + } + const keyType = readSshString(decoded, 0) + return keyType?.value.toString('ascii') === declaredType +} diff --git a/src/main/ssh/ssh-system-fallback.test.ts b/src/main/ssh/ssh-system-fallback.test.ts index 36312762d08..03e3e8baf53 100644 --- a/src/main/ssh/ssh-system-fallback.test.ts +++ b/src/main/ssh/ssh-system-fallback.test.ts @@ -24,7 +24,6 @@ vi.mock('child_process', () => ({ import { buildSshArgs, - findSystemSsh, downloadFileViaSystemSsh, spawnSystemSsh, spawnSystemSshCommand, @@ -152,22 +151,6 @@ function createMockChildProcess(): EventEmitter & { return child } -describe('findSystemSsh', () => { - beforeEach(() => { - existsSyncMock.mockReset() - }) - - it('returns the first existing ssh path', () => { - mockSystemSshExists() - expect(findSystemSsh()).toBe(SYSTEM_SSH_PATH) - }) - - it('returns null when no ssh binary is found', () => { - existsSyncMock.mockReturnValue(false) - expect(findSystemSsh()).toBeNull() - }) -}) - describe('spawnSystemSsh', () => { let mockProc: { stdin: { @@ -827,6 +810,7 @@ describe('spawnSystemSsh', () => { it('throws when no system ssh is found', () => { existsSyncMock.mockReturnValue(false) + vi.stubEnv('PATH', '') expect(() => spawnSystemSsh(createTarget())).toThrow('No system ssh binary found') }) diff --git a/src/main/ssh/ssh-transport-selection.ts b/src/main/ssh/ssh-transport-selection.ts new file mode 100644 index 00000000000..fdbfb87075f --- /dev/null +++ b/src/main/ssh/ssh-transport-selection.ts @@ -0,0 +1,118 @@ +import { constants } from 'node:fs' +import { open, stat } from 'node:fs/promises' +import type { SshTarget } from '../../shared/ssh-types' +import type { SshResolvedConfig } from './ssh-config-parser' +import { listDefaultIdentityFilePaths, resolveIdentityFilePaths } from './ssh-auth-resolution' +import { resolveSshConfigHomePath } from './ssh-config-path-expansion' +import { + MAX_SSH_IDENTITY_FILE_BYTES, + isOpenSshSecurityKeyPrivateKey, + isOpenSshSecurityKeyPublicKey +} from './ssh-security-key-identity' +import { isOpenSshConfigBackedTarget } from './system-ssh-args' +import { findSystemSsh } from './system-ssh-binary' + +type TransportResolvedConfig = Pick< + SshResolvedConfig, + 'proxyUseFdpass' | 'proxyCommand' | 'proxyJump' +> + +const READ_CHUNK_BYTES = 64 * 1024 +const READ_OPEN_FLAGS = + constants.O_RDONLY | (process.platform === 'win32' ? 0 : constants.O_NONBLOCK) + +type IdentityInspection = { privateIdentityExists: boolean; requiresSystemSsh: boolean } + +async function readBoundedKeyFile(path: string): Promise { + let handle: Awaited> | undefined + try { + const pathStats = await stat(path) + if (!pathStats.isFile() || pathStats.size > MAX_SSH_IDENTITY_FILE_BYTES) { + return null + } + handle = await open(path, READ_OPEN_FLAGS) + const stats = await handle.stat() + if (!stats.isFile() || stats.size > MAX_SSH_IDENTITY_FILE_BYTES) { + return null + } + const chunks: Buffer[] = [] + let offset = 0 + while (offset <= MAX_SSH_IDENTITY_FILE_BYTES) { + const buffer = Buffer.alloc( + Math.min(READ_CHUNK_BYTES, MAX_SSH_IDENTITY_FILE_BYTES + 1 - offset) + ) + const { bytesRead } = await handle.read(buffer, 0, buffer.length, offset) + if (bytesRead === 0) { + break + } + offset += bytesRead + if (offset > MAX_SSH_IDENTITY_FILE_BYTES) { + return null + } + chunks.push(buffer.subarray(0, bytesRead)) + } + return Buffer.concat(chunks, offset) + } catch { + return null + } finally { + await handle?.close().catch(() => undefined) + } +} + +async function inspectIdentityPath(keyPath: string): Promise { + const resolvedPath = resolveSshConfigHomePath(keyPath) + const identity = await readBoundedKeyFile(resolvedPath) + if (identity !== null) { + return { + privateIdentityExists: true, + requiresSystemSsh: + isOpenSshSecurityKeyPublicKey(identity) || isOpenSshSecurityKeyPrivateKey(identity) + } + } + const publicIdentity = await readBoundedKeyFile(`${resolvedPath}.pub`) + return { + privateIdentityExists: false, + requiresSystemSsh: publicIdentity !== null && isOpenSshSecurityKeyPublicKey(publicIdentity) + } +} + +export function shouldUseSystemSshTransport( + target: SshTarget, + resolved: TransportResolvedConfig | null +): boolean { + if (isOpenSshConfigBackedTarget(target) && resolved) { + return ( + process.env.ORCA_SSH_FORCE_SYSTEM_TRANSPORT === '1' || + resolved.proxyUseFdpass === true || + resolved.proxyCommand != null || + resolved.proxyJump != null + ) + } + return ( + process.env.ORCA_SSH_FORCE_SYSTEM_TRANSPORT === '1' || + target.proxyCommand != null || + target.jumpHost != null || + resolved?.proxyUseFdpass === true || + resolved?.proxyCommand != null || + resolved?.proxyJump != null + ) +} + +export async function requiresSystemSshForSecurityKey( + target: SshTarget, + resolved: Pick | null +): Promise { + const configuredPaths = resolveIdentityFilePaths(target, resolved) + const usesDefaultPaths = configuredPaths.length === 0 && !resolved && !target.identityFile + const identityPaths = usesDefaultPaths ? listDefaultIdentityFilePaths() : configuredPaths + for (const keyPath of identityPaths) { + const inspection = await inspectIdentityPath(keyPath) + if (inspection.requiresSystemSsh) { + return !usesDefaultPaths || findSystemSsh() !== null + } + if (usesDefaultPaths && inspection.privateIdentityExists) { + return false + } + } + return false +} diff --git a/src/main/ssh/system-ssh-binary.test.ts b/src/main/ssh/system-ssh-binary.test.ts new file mode 100644 index 00000000000..a3be418d2fc --- /dev/null +++ b/src/main/ssh/system-ssh-binary.test.ts @@ -0,0 +1,106 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { accessSyncMock, existsSyncMock, statSyncMock } = vi.hoisted(() => ({ + accessSyncMock: vi.fn(), + existsSyncMock: vi.fn(), + statSyncMock: vi.fn() +})) + +vi.mock('node:fs', () => ({ + accessSync: accessSyncMock, + constants: { O_RDONLY: 0, O_NONBLOCK: 4, X_OK: 1 }, + existsSync: existsSyncMock, + statSync: statSyncMock +})) + +import { findSystemSsh } from './system-ssh-binary' + +describe('findSystemSsh', () => { + beforeEach(() => { + accessSyncMock.mockReset() + existsSyncMock.mockReset() + statSyncMock.mockReset() + statSyncMock.mockImplementation(() => { + throw new Error('missing') + }) + vi.unstubAllEnvs() + }) + + afterEach(() => { + vi.unstubAllEnvs() + }) + + it('returns the first existing fixed ssh path', () => { + const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('linux') + existsSyncMock.mockImplementation((path: string) => path === '/usr/bin/ssh') + + try { + expect(findSystemSsh()).toBe('/usr/bin/ssh') + } finally { + platformSpy.mockRestore() + } + }) + + it('returns null when no ssh binary is found', () => { + existsSyncMock.mockReturnValue(false) + expect(findSystemSsh()).toBeNull() + }) + + it('finds a PATH-installed ssh.exe on Windows', () => { + const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32') + vi.stubEnv('PATH', 'C:\\Git\\usr\\bin;C:\\Tools') + existsSyncMock.mockReturnValue(false) + statSyncMock.mockImplementation((path: string) => { + if (path === 'C:\\Git\\usr\\bin\\ssh.exe') { + return { isFile: () => true } + } + throw new Error('missing') + }) + + try { + expect(findSystemSsh()).toBe('C:\\Git\\usr\\bin\\ssh.exe') + } finally { + platformSpy.mockRestore() + } + }) + + it('finds an executable PATH-installed ssh on POSIX', () => { + const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('linux') + vi.stubEnv('PATH', '/nix/store/ssh/bin:/opt/tools') + existsSyncMock.mockReturnValue(false) + statSyncMock.mockImplementation((path: string) => { + if (path === '/nix/store/ssh/bin/ssh') { + return { isFile: () => true } + } + throw new Error('missing') + }) + + try { + expect(findSystemSsh()).toBe('/nix/store/ssh/bin/ssh') + expect(accessSyncMock).toHaveBeenCalledWith('/nix/store/ssh/bin/ssh', expect.any(Number)) + } finally { + platformSpy.mockRestore() + } + }) + + it('resolves Windows OpenSSH from the runtime system root', () => { + const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32') + vi.stubEnv('SystemRoot', 'D:\\Windows') + existsSyncMock.mockImplementation( + (path: string) => path === 'D:\\Windows\\System32\\OpenSSH\\ssh.exe' + ) + + try { + expect(findSystemSsh()).toBe('D:\\Windows\\System32\\OpenSSH\\ssh.exe') + } finally { + platformSpy.mockRestore() + } + }) + + it('keeps an explicit system ssh override authoritative', () => { + vi.stubEnv('ORCA_SYSTEM_SSH_PATH', 'C:\\Custom\\ssh.exe') + + expect(findSystemSsh()).toBe('C:\\Custom\\ssh.exe') + expect(existsSyncMock).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/ssh/system-ssh-binary.ts b/src/main/ssh/system-ssh-binary.ts index 579b483392c..bc8ff3ea516 100644 --- a/src/main/ssh/system-ssh-binary.ts +++ b/src/main/ssh/system-ssh-binary.ts @@ -1,9 +1,41 @@ -import { existsSync } from 'node:fs' +import { accessSync, constants, existsSync, statSync } from 'node:fs' +import { posix, win32 } from 'node:path' -const SYSTEM_SSH_PATHS = - process.platform === 'win32' - ? ['C:\\Windows\\System32\\OpenSSH\\ssh.exe', 'ssh.exe'] - : ['/usr/bin/ssh', '/usr/local/bin/ssh', '/opt/homebrew/bin/ssh'] +function systemSshPaths(platform: NodeJS.Platform): string[] { + if (platform !== 'win32') { + return ['/usr/bin/ssh', '/usr/local/bin/ssh', '/opt/homebrew/bin/ssh'] + } + const systemRoot = process.env.SystemRoot || process.env.WINDIR + return systemRoot ? [win32.join(systemRoot, 'System32', 'OpenSSH', 'ssh.exe')] : [] +} + +function findSshOnPath(platform: NodeJS.Platform): string | null { + const pathValue = process.env.PATH + if (!pathValue) { + return null + } + const pathApi = platform === 'win32' ? win32 : posix + const executable = platform === 'win32' ? 'ssh.exe' : 'ssh' + for (const entry of pathValue.split(pathApi.delimiter)) { + const directory = entry.trim().replace(/^"|"$/g, '') + if (!directory) { + continue + } + const candidate = pathApi.join(directory, executable) + try { + if (!statSync(candidate).isFile()) { + continue + } + if (platform !== 'win32') { + accessSync(candidate, constants.X_OK) + } + return candidate + } catch { + continue + } + } + return null +} /** * Find the system ssh binary path. Returns null if not found. @@ -12,10 +44,10 @@ export function findSystemSsh(): string | null { if (process.env.ORCA_SYSTEM_SSH_PATH) { return process.env.ORCA_SYSTEM_SSH_PATH } - for (const candidate of SYSTEM_SSH_PATHS) { + for (const candidate of systemSshPaths(process.platform)) { if (existsSync(candidate)) { return candidate } } - return null + return findSshOnPath(process.platform) } diff --git a/src/main/window/createMainWindow.test.ts b/src/main/window/createMainWindow.test.ts index fc38ca95e77..8a61f157584 100644 --- a/src/main/window/createMainWindow.test.ts +++ b/src/main/window/createMainWindow.test.ts @@ -74,6 +74,8 @@ import { } from './createMainWindow' import { ipcMain } from 'electron' import { shouldRecoverRendererAfterProcessGone } from '../crash-reporting/process-gone-classification' +import { BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD } from '../../shared/browser-window-close-policy' +import { fileURLToPath } from 'node:url' function withPlatform(platform: NodeJS.Platform, run: () => T): T { const original = process.platform @@ -248,6 +250,7 @@ describe('createMainWindow', () => { expect(allowBlankPrefs).toMatchObject({ disableHtmlFullscreenWindowResize: true, partition: 'persist:orca-browser', + preload: expect.stringMatching(/browser-window-close-preload\.js$/), sandbox: true }) @@ -262,6 +265,36 @@ describe('createMainWindow', () => { const guest = { marker: 'guest' } windowHandlers['did-attach-webview']({} as never, guest as never) expect(attachGuestPoliciesMock).toHaveBeenCalledWith(guest) + + const allowWindowCloseEvent = { preventDefault: vi.fn() } + const allowWindowCloseParams = { + src: 'data:text/html,', + preload: BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD + } + const allowWindowClosePrefs = { partition: 'persist:orca-browser' } + windowHandlers['will-attach-webview']( + allowWindowCloseEvent as never, + allowWindowClosePrefs as never, + allowWindowCloseParams as never + ) + expect(allowWindowCloseEvent.preventDefault).not.toHaveBeenCalled() + expect(allowWindowCloseParams.preload).toBeUndefined() + expect(allowWindowClosePrefs).not.toHaveProperty('preload') + + const allowWindowClosePathPrefs = { + partition: 'persist:orca-browser', + preload: fileURLToPath(BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD) + } + windowHandlers['will-attach-webview']( + { preventDefault: vi.fn() } as never, + allowWindowClosePathPrefs as never, + { src: 'data:text/html,', preload: '' } as never + ) + expect(allowWindowClosePathPrefs).not.toHaveProperty('preload') + + const cliGuest = { marker: 'cli-guest' } + windowHandlers['did-attach-webview']({} as never, cliGuest as never) + expect(attachGuestPoliciesMock).toHaveBeenLastCalledWith(cliGuest) }) it('sets platform-specific titlebar and frame options for every desktop platform', () => { diff --git a/src/main/window/createMainWindow.ts b/src/main/window/createMainWindow.ts index 519e1817051..49c3eb4c125 100644 --- a/src/main/window/createMainWindow.ts +++ b/src/main/window/createMainWindow.ts @@ -10,6 +10,7 @@ import { screen } from 'electron' import { join } from 'node:path' +import { fileURLToPath } from 'node:url' import { is } from '@electron-toolkit/utils' import type { Store } from '../persistence' import { getAppIconPath } from '../app-icon' @@ -18,6 +19,7 @@ import { browserSessionRegistry } from '../browser/browser-session-registry' import { translateMain } from '../i18n/main-i18n' import { normalizeBrowserNavigationUrl } from '../../shared/browser-url' import { ORCA_BROWSER_GUEST_WEB_PREFERENCES } from '../../shared/browser-guest-web-preferences' +import { BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD } from '../../shared/browser-window-close-policy' import { isCrashReportReason } from '../../shared/crash-reporting' import { DEFAULT_RENDERER_RECOVERY_MAX_RECOVERIES, @@ -441,6 +443,8 @@ export function createMainWindow( // so register it with the window's other navigation policy. registerPluginPanelNavigationGuard(mainWindow.webContents) + const browserWindowClosePreload = join(__dirname, 'browser-window-close-preload.js') + const browserWindowCloseAllowedPreloadPath = fileURLToPath(BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD) mainWindow.webContents.on('will-attach-webview', (event, webPreferences, params) => { const src = typeof params.src === 'string' ? params.src : '' const normalizedSrc = normalizeBrowserNavigationUrl(src) @@ -452,7 +456,18 @@ export function createMainWindow( return } - delete webPreferences.preload + const allowWindowClose = [params.preload, webPreferences.preload].some( + (preload) => + preload === BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD || + preload === browserWindowCloseAllowedPreloadPath + ) + delete params.preload + if (allowWindowClose) { + delete webPreferences.preload + } else { + // Why: preload runs in the page's main world before inline scripts can call window.close(). + webPreferences.preload = browserWindowClosePreload + } // Why: older Electron builds expose preloadURL alongside preload; delete both so the guest can't inherit the main preload bridge. delete (webPreferences as Record).preloadURL webPreferences.nodeIntegration = false diff --git a/src/preload/browser-window-close-installation.ts b/src/preload/browser-window-close-installation.ts new file mode 100644 index 00000000000..adf2fdb09bf --- /dev/null +++ b/src/preload/browser-window-close-installation.ts @@ -0,0 +1,15 @@ +export function installBrowserWindowCloseGuard(): void { + const ignoreWindowClose = (): void => {} + try { + Object.defineProperty(window, 'close', { + configurable: false, + enumerable: false, + writable: false, + value: ignoreWindowClose + }) + } catch { + try { + window.close = ignoreWindowClose + } catch {} + } +} diff --git a/src/preload/browser-window-close.test.ts b/src/preload/browser-window-close.test.ts new file mode 100644 index 00000000000..1b35a45f8df --- /dev/null +++ b/src/preload/browser-window-close.test.ts @@ -0,0 +1,20 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' + +import { installBrowserWindowCloseGuard } from './browser-window-close-installation' + +describe('browser window close preload', () => { + afterEach(() => { + vi.unstubAllGlobals() + }) + + it('installs a non-replaceable window.close no-op in the page world', () => { + const nativeClose = vi.fn() + vi.stubGlobal('window', { close: nativeClose }) + + installBrowserWindowCloseGuard() + + expect(window.close()).toBeUndefined() + expect(window.close).not.toBe(nativeClose) + expect(Reflect.set(window, 'close', nativeClose)).toBe(false) + }) +}) diff --git a/src/preload/browser-window-close.ts b/src/preload/browser-window-close.ts new file mode 100644 index 00000000000..7fb8201377f --- /dev/null +++ b/src/preload/browser-window-close.ts @@ -0,0 +1,7 @@ +import { installBrowserWindowCloseGuard } from './browser-window-close-installation' +import type { ContextBridge } from 'electron' + +// Why: raw require keeps the sandboxed preload standalone in the main-process CJS build. +const { contextBridge } = require('electron') as { contextBridge: ContextBridge } + +contextBridge.executeInMainWorld({ func: installBrowserWindowCloseGuard }) diff --git a/src/renderer/src/components/browser-pane/BrowserPane.tsx b/src/renderer/src/components/browser-pane/BrowserPane.tsx index 1a2c0279543..60cc98d8d7c 100644 --- a/src/renderer/src/components/browser-pane/BrowserPane.tsx +++ b/src/renderer/src/components/browser-pane/BrowserPane.tsx @@ -3693,6 +3693,7 @@ function BrowserPagePane({ container, inputLocked: inputLockedRef.current, webviewPartition, + allowWindowClose: browserTab.allowWindowClose === true, resolveContainer: () => ensureBrowserPageViewport(browserTab.id, workspaceId)?.container ?? null }) diff --git a/src/renderer/src/components/browser-pane/BrowserPane.webview-preferences.test.ts b/src/renderer/src/components/browser-pane/BrowserPane.webview-preferences.test.ts index 203fce6fdbf..496b23ec409 100644 --- a/src/renderer/src/components/browser-pane/BrowserPane.webview-preferences.test.ts +++ b/src/renderer/src/components/browser-pane/BrowserPane.webview-preferences.test.ts @@ -1,6 +1,7 @@ // @vitest-environment happy-dom import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { ORCA_BROWSER_GUEST_WEB_PREFERENCES_ATTRIBUTE } from '../../../../shared/browser-guest-web-preferences' +import { BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD } from '../../../../shared/browser-window-close-policy' const registryMocks = vi.hoisted(() => ({ destroyPersistentWebview: vi.fn(), @@ -103,4 +104,22 @@ describe('BrowserPane webview preferences', () => { expect(ensuredWebview?.webview.style.pointerEvents).toBe('none') expect(refreshedContainer.lastElementChild).toBe(ensuredWebview?.webview as unknown as Element) }) + + it('marks explicitly allowed CLI pages before the guest attaches', () => { + const container = createContainer('cli-page') + + const ensuredWebview = ensureBrowserPageWebview({ + browserTabId: 'browser-page-cli', + container, + inputLocked: false, + webviewPartition: 'persist:orca-browser', + allowWindowClose: true, + resolveContainer: () => container + }) + + expect(ensuredWebview?.webview.getAttribute('preload')).toBe( + BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD + ) + expect(BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD).toMatch(/^file:/) + }) }) diff --git a/src/renderer/src/components/browser-pane/browser-page-webview.ts b/src/renderer/src/components/browser-pane/browser-page-webview.ts index 1179d0da63b..b8c44d849ad 100644 --- a/src/renderer/src/components/browser-pane/browser-page-webview.ts +++ b/src/renderer/src/components/browser-pane/browser-page-webview.ts @@ -1,4 +1,5 @@ import { ORCA_BROWSER_GUEST_WEB_PREFERENCES_ATTRIBUTE } from '../../../../shared/browser-guest-web-preferences' +import { BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD } from '../../../../shared/browser-window-close-policy' import { destroyPersistentWebview, registerPersistentWebview, @@ -10,12 +11,14 @@ export function ensureBrowserPageWebview({ container, inputLocked, webviewPartition, + allowWindowClose, resolveContainer }: { browserTabId: string container: HTMLDivElement inputLocked: boolean webviewPartition: string + allowWindowClose?: boolean resolveContainer: () => HTMLDivElement | null }): { container: HTMLDivElement; created: boolean; webview: Electron.WebviewTag } | null { let webview = webviewRegistry.get(browserTabId) @@ -46,6 +49,10 @@ export function ensureBrowserPageWebview({ webview = document.createElement('webview') as Electron.WebviewTag webview.setAttribute('partition', webviewPartition) + if (allowWindowClose) { + // Why: main consumes and removes this marker in will-attach-webview before the guest sees any preload. + webview.setAttribute('preload', BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD) + } webview.setAttribute('allowpopups', '') // Why: Electron spreads the webpreferences keys verbatim, so the shared // camelCase attribute must stay intact for fullscreen containment to work. diff --git a/src/renderer/src/components/terminal-pane/terminal-visibility-resume.test.ts b/src/renderer/src/components/terminal-pane/terminal-visibility-resume.test.ts index 300a381c05a..17271179620 100644 --- a/src/renderer/src/components/terminal-pane/terminal-visibility-resume.test.ts +++ b/src/renderer/src/components/terminal-pane/terminal-visibility-resume.test.ts @@ -151,6 +151,50 @@ describe('resumeTerminalVisibility reveal repaint', () => { expect(manager.fitAllPanes).not.toHaveBeenCalled() }) + it('latches viewport intent before refocus recovery flushes streaming output', async () => { + const terminal = { name: 'streaming-terminal' } + const manager = createManager() + manager.getPanes.mockReturnValue([{ terminal }]) + const { syncTerminalScrollIntentFromViewport } = vi.mocked( + await import('@/lib/pane-manager/terminal-scroll-intent') + ) + const { flushTerminalOutput } = vi.mocked( + await import('@/lib/pane-manager/pane-terminal-output-scheduler') + ) + + recoverVisibleTerminalWindowWake({ + manager: manager as never as PaneManager, + isActive: true, + clearGlyphAtlases: false + }) + + expect(flushTerminalOutput).toHaveBeenCalledOnce() + expect(syncTerminalScrollIntentFromViewport).toHaveBeenCalledOnce() + expect(syncTerminalScrollIntentFromViewport.mock.invocationCallOrder[0]).toBeLessThan( + flushTerminalOutput.mock.invocationCallOrder[0] ?? Number.POSITIVE_INFINITY + ) + }) + + it('does not overwrite pre-reveal intent after queuing hidden output', async () => { + const terminal = { name: 'hidden-streaming-terminal' } + const manager = createManager() + manager.getPanes.mockReturnValue([{ terminal }]) + const { syncTerminalScrollIntentFromViewport } = vi.mocked( + await import('@/lib/pane-manager/terminal-scroll-intent') + ) + const { flushTerminalOutput } = vi.mocked( + await import('@/lib/pane-manager/pane-terminal-output-scheduler') + ) + + resumeTerminalVisibility(resumeArgs(manager, false)) + + expect(flushTerminalOutput).toHaveBeenCalledOnce() + expect(syncTerminalScrollIntentFromViewport).toHaveBeenCalledOnce() + expect(syncTerminalScrollIntentFromViewport.mock.invocationCallOrder[0]).toBeLessThan( + flushTerminalOutput.mock.invocationCallOrder[0] ?? Number.POSITIVE_INFINITY + ) + }) + it('resets each pane linkifier hover cache on window wake recovery so links recover without a scroll', () => { const first = { name: 'pane-a' } const second = { name: 'pane-b' } diff --git a/src/renderer/src/components/terminal-pane/terminal-visibility-resume.ts b/src/renderer/src/components/terminal-pane/terminal-visibility-resume.ts index 641c99258bc..5c6b8314545 100644 --- a/src/renderer/src/components/terminal-pane/terminal-visibility-resume.ts +++ b/src/renderer/src/components/terminal-pane/terminal-visibility-resume.ts @@ -143,6 +143,8 @@ export function recoverVisibleTerminalWindowWake({ }: RecoverVisibleTerminalWindowWakeArgs): void { // Why: macOS screensaver/display wake can leave xterm visible but with a // stale renderer/input surface; Orca's own hidden-state resume never runs. + // Why: backlog writes can expose transient viewport geometry while parsing. + syncTerminalViewportIntents(manager) for (const pane of manager.getPanes()) { requestTerminalBacklogRecovery(pane.terminal) flushTerminalOutput(pane.terminal, { maxChars: WINDOW_WAKE_FLUSH_CHARS }) @@ -154,7 +156,6 @@ export function recoverVisibleTerminalWindowWake({ resetTerminalLinkifierHoverState(pane.terminal) } } - syncTerminalViewportIntents(manager) manager.resumeRendering() // Why: wake re-attaches WebGL — same transient cell-metric wobble guard as the heavy resume. manager.fitAllRevealedPanes() @@ -192,7 +193,7 @@ function resumeTerminalVisibilityHeavy(manager: PaneManager, isActive: boolean): requestTerminalBacklogRecovery(pane.terminal) flushTerminalOutput(pane.terminal, { maxChars: VISIBLE_RESUME_FLUSH_CHARS }) } - syncTerminalViewportIntents(manager) + // Intent was latched by the caller before queued writes can expose transient geometry. // Resume WebGL immediately so the terminal shows its last-known state // on the first painted frame. macOS context creation is ~5 ms; on // Windows (ANGLE -> D3D11) it can be 100-500 ms but a deferred resume diff --git a/src/renderer/src/hooks/useIpcEvents.test.ts b/src/renderer/src/hooks/useIpcEvents.test.ts index 1d956f716cb..0b7ea6ad0af 100644 --- a/src/renderer/src/hooks/useIpcEvents.test.ts +++ b/src/renderer/src/hooks/useIpcEvents.test.ts @@ -1077,6 +1077,11 @@ describe('useIpcEvents browser tab create routing', () => { expect(acquireBrowserAutomationVisibility).toHaveBeenCalledWith('page-new') expect(acquireBrowserAutomationVisibility).not.toHaveBeenCalledWith('page-active') + expect(state.createBrowserTab).toHaveBeenCalledWith( + 'wt-1', + 'https://example.com', + expect.objectContaining({ allowWindowClose: true }) + ) expect(replyTabCreate).toHaveBeenCalledWith({ requestId: 'req-create', browserPageId: 'page-new' diff --git a/src/renderer/src/hooks/useIpcEvents.ts b/src/renderer/src/hooks/useIpcEvents.ts index 3c8b19068c2..2a72aae4918 100644 --- a/src/renderer/src/hooks/useIpcEvents.ts +++ b/src/renderer/src/hooks/useIpcEvents.ts @@ -2266,6 +2266,7 @@ export function useIpcEvents(): void { // Agent/automation opens stay in the background (activate:false) in the active browser group. const workspace = store.createBrowserTab(worktreeId, data.url, { title: data.url, + allowWindowClose: true, targetGroupId: data.activate ? undefined : activeBrowserUnifiedTab?.groupId, sessionProfileId: data.sessionProfileId, sessionPartition: data.sessionPartition, diff --git a/src/renderer/src/store/slices/browser.ts b/src/renderer/src/store/slices/browser.ts index 60a71a3eb47..42d0ddbce7b 100644 --- a/src/renderer/src/store/slices/browser.ts +++ b/src/renderer/src/store/slices/browser.ts @@ -57,6 +57,7 @@ import { buildValidWorktreeIdsForSessionHydration } from './degraded-repo-worktr type CreateBrowserTabOptions = { activate?: boolean title?: string + allowWindowClose?: boolean sessionProfileId?: string | null sessionPartition?: string | null // Place the new tab in a specific group (e.g. "Open Preview to the Side"); defaults to the worktree's active group. @@ -69,6 +70,7 @@ type CreateBrowserTabOptions = { type CreateBrowserPageOptions = { activate?: boolean title?: string + allowWindowClose?: boolean browserRuntimeEnvironmentId?: string | null } @@ -341,7 +343,8 @@ function buildBrowserPage( worktreeId: string, url: string, title?: string, - browserRuntimeEnvironmentId?: string | null + browserRuntimeEnvironmentId?: string | null, + allowWindowClose?: boolean ): BrowserPage { const normalizedUrl = normalizeUrl(url) return { @@ -357,6 +360,7 @@ function buildBrowserPage( canGoForward: false, loadError: null, createdAt: Date.now(), + ...(allowWindowClose !== undefined ? { allowWindowClose } : {}), ...(browserRuntimeEnvironmentId !== undefined ? { browserRuntimeEnvironmentId } : {}) } } @@ -554,7 +558,8 @@ export const createBrowserSlice: StateCreator = worktreeId, url, options?.title, - options?.browserRuntimeEnvironmentId + options?.browserRuntimeEnvironmentId, + options?.allowWindowClose ) // Why: with no explicit profile, inherit the user's default so a Settings preference applies to new tabs. const sessionProfileId = @@ -902,13 +907,15 @@ export const createBrowserSlice: StateCreator = activate: true, sessionProfileId, sessionPartition, - browserRuntimeEnvironmentId: firstPage.browserRuntimeEnvironmentId + browserRuntimeEnvironmentId: firstPage.browserRuntimeEnvironmentId, + allowWindowClose: firstPage.allowWindowClose }) for (const p of restPages) { get().createBrowserPage(restored.id, p.url, { activate: false, title: p.title, + allowWindowClose: p.allowWindowClose, browserRuntimeEnvironmentId: p.browserRuntimeEnvironmentId }) } @@ -987,7 +994,8 @@ export const createBrowserSlice: StateCreator = workspace.worktreeId, url, options?.title, - options?.browserRuntimeEnvironmentId + options?.browserRuntimeEnvironmentId, + options?.allowWindowClose ) set((s) => { @@ -1169,6 +1177,7 @@ export const createBrowserSlice: StateCreator = return get().createBrowserPage(workspaceId, pageToRestore.url, { title: pageToRestore.title, activate: true, + allowWindowClose: pageToRestore.allowWindowClose, browserRuntimeEnvironmentId: pageToRestore.browserRuntimeEnvironmentId }) }, diff --git a/src/shared/browser-window-close-policy.ts b/src/shared/browser-window-close-policy.ts new file mode 100644 index 00000000000..2ea28169ee7 --- /dev/null +++ b/src/shared/browser-window-close-policy.ts @@ -0,0 +1,2 @@ +// Why: this marker carries the CLI's explicit close policy across attach before the guest exists. +export const BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD = 'file:///__orca_window_close_allowed__' diff --git a/src/shared/types.ts b/src/shared/types.ts index 475470dd4c4..a03c2420985 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -982,6 +982,8 @@ export type BrowserPage = { canGoForward: boolean loadError: BrowserLoadError | null createdAt: number + // Why: CLI-created pages retain Chromium's native window.close behavior; ordinary embedded pages are guarded. + allowWindowClose?: boolean // Why: remote-owned worktrees can still host client-local fallback browser // pages until headless remote runtimes support real browser panes. browserRuntimeEnvironmentId?: string | null diff --git a/src/shared/workspace-session-schema.ts b/src/shared/workspace-session-schema.ts index da5fe4ef08e..f5d849d5270 100644 --- a/src/shared/workspace-session-schema.ts +++ b/src/shared/workspace-session-schema.ts @@ -225,6 +225,7 @@ const browserPageSchema = z.object({ canGoForward: z.boolean(), loadError: browserLoadErrorSchema.nullable(), createdAt: z.number(), + allowWindowClose: z.boolean().optional(), // Why: explicit null marks a browser page as client-local even when its // worktree is remote-owned; older sessions omit it and keep inferred runtime. browserRuntimeEnvironmentId: z.string().nullable().optional(), diff --git a/tests/e2e/browser-tab.spec.ts b/tests/e2e/browser-tab.spec.ts index 7ac10558047..bc8dd51e988 100644 --- a/tests/e2e/browser-tab.spec.ts +++ b/tests/e2e/browser-tab.spec.ts @@ -29,10 +29,11 @@ async function createBrowserTab( page: Parameters[0], worktreeId: string, url?: string, - title = 'New Browser Tab' + title = 'New Browser Tab', + allowWindowClose?: boolean ): Promise { return page.evaluate( - ({ targetWorktreeId, targetUrl, targetTitle }) => { + ({ targetWorktreeId, targetUrl, targetTitle, allowClose }) => { const store = window.__store if (!store) { return null @@ -44,12 +45,18 @@ async function createBrowserTab( targetUrl ?? state.browserDefaultUrl ?? 'about:blank', { title: targetTitle, - activate: true + activate: true, + allowWindowClose: allowClose } ) return { id: tab.id, pageId: tab.activePageId ?? null } }, - { targetWorktreeId: worktreeId, targetUrl: url, targetTitle: title } + { + targetWorktreeId: worktreeId, + targetUrl: url, + targetTitle: title, + allowClose: allowWindowClose + } ) } @@ -199,6 +206,71 @@ async function startBrowserLinkServer(): Promise<{ } } +async function startBrowserWindowCloseServer(): Promise<{ + url: string + sourceUrl: string + close: () => Promise +}> { + const server = createServer((request, response) => { + const origin = `http://127.0.0.1:${(server.address() as AddressInfo).port}` + const pathname = new URL(request.url ?? '/', origin).pathname + response.writeHead(200, { 'Content-Type': 'text/html; charset=utf-8' }) + if (pathname === '/source') { + response.end(` + + + Close link source + Open close page + + `) + return + } + response.end(` + + + Window close repro + +

Attempting close…

+ + + + `) + }) + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)) + const port = (server.address() as AddressInfo).port + return { + url: `http://127.0.0.1:${port}/window-close`, + sourceUrl: `http://127.0.0.1:${port}/source`, + close: () => closeServer(server) + } +} + +async function readBrowserWindowCloseStatus( + page: Parameters[0], + browserTabId: string +): Promise { + return page.evaluate(async (targetBrowserTabId) => { + const slot = document.querySelector(`[data-browser-overlay-tab-id="${targetBrowserTabId}"]`) + const webview = slot?.querySelector('webview') as Electron.WebviewTag | null + if (!webview) { + return 'webview missing' + } + try { + return (await webview.executeJavaScript( + 'document.querySelector("#s")?.textContent ?? "status missing"' + )) as string + } catch { + return 'guest lost' + } + }, browserTabId) +} + async function closeServer(server: Server): Promise { await new Promise((resolve, reject) => server.close((error) => { @@ -689,6 +761,88 @@ test.describe('Browser Tab', () => { } }) + test('blocked window.close in a link-created tab does not break tab switching', async ({ + orcaPage + }) => { + const closeServer = await startBrowserWindowCloseServer() + try { + const worktreeId = (await getActiveWorktreeId(orcaPage))! + const neighboringTab = await createBrowserTab( + orcaPage, + worktreeId, + 'about:blank', + 'Neighboring tab' + ) + const sourceTab = await createBrowserTab( + orcaPage, + worktreeId, + closeServer.sourceUrl, + 'Close link source' + ) + expect(neighboringTab?.id).toBeTruthy() + expect(sourceTab?.id).toBeTruthy() + + await clickBrowserLink(orcaPage, sourceTab!.id, '#window-close-link') + let closeTabId: string | null = null + await expect + .poll(async () => { + const tabs = await getBrowserTabs(orcaPage, worktreeId) + closeTabId = tabs.find((tab) => tab.url === closeServer.url)?.id ?? null + return closeTabId + }) + .not.toBeNull() + + await orcaPage.locator(`[data-tab-id="${neighboringTab!.id}"]`).click() + await expect.poll(async () => getActiveTabType(orcaPage), { timeout: 5_000 }).toBe('browser') + await expect + .poll(() => readBrowserWindowCloseStatus(orcaPage, closeTabId!), { timeout: 5_000 }) + .toContain('window.close() was blocked') + } finally { + await closeServer.close() + } + }) + + test('explicitly allowed browser tabs retain replaceable window.close', async ({ orcaPage }) => { + const closeServer = await startBrowserWindowCloseServer() + try { + const worktreeId = (await getActiveWorktreeId(orcaPage))! + const allowedTab = await createBrowserTab( + orcaPage, + worktreeId, + closeServer.sourceUrl, + 'Allowed close tab', + true + ) + expect(allowedTab?.id).toBeTruthy() + + await expect + .poll( + () => + orcaPage.evaluate(async (targetBrowserTabId) => { + const slot = document.querySelector( + `[data-browser-overlay-tab-id="${targetBrowserTabId}"]` + ) + const webview = slot?.querySelector('webview') as Electron.WebviewTag | null + if (!webview) { + return 'webview missing' + } + try { + return (await webview.executeJavaScript(`(() => { + window.close = () => 'replacement-called' + return window.close() === 'replacement-called' ? 'replacement-called' : 'blocked' + })()`)) as string + } catch { + return 'guest unavailable' + } + }, allowedTab!.id), + { timeout: 5_000 } + ) + .toBe('replacement-called') + } finally { + await closeServer.close() + } + }) + /** * User Prompt: * - Browser works and also retains state when switching tabs etc. diff --git a/tests/e2e/terminal-streaming-refocus-viewport.spec.ts b/tests/e2e/terminal-streaming-refocus-viewport.spec.ts new file mode 100644 index 00000000000..8376a3188a0 --- /dev/null +++ b/tests/e2e/terminal-streaming-refocus-viewport.spec.ts @@ -0,0 +1,240 @@ +import path from 'node:path' +import type { Page } from '@stablyai/playwright-test' +import { expect, test } from './helpers/orca-app' +import { ensureTerminalVisible, waitForActiveWorktree, waitForSessionReady } from './helpers/store' +import { + execInTerminal, + getTerminalContent, + waitForActivePaneHookDescriptor, + waitForActivePanePtyId, + waitForActiveTerminalManager +} from './helpers/terminal' +import { waitForTerminalPtyDataInjector } from './helpers/terminal-pty-injection' +import { nodeTerminalCommand } from './terminal-node-command' + +const STREAMING_FIXTURE_PATH = path.join( + process.cwd(), + 'tests/e2e/fixtures/streaming-scrollback-fixture.cjs' +) + +type RevealFrame = { + targetPresented: boolean + thumbTop: number | null + maxThumbTop: number | null +} + +async function closeFeatureTips(page: Page): Promise { + await page.evaluate(() => { + const store = window.__store + store?.getState().markFeatureTipsSeen(['orca-cli', 'cmd-j-palette', 'voice-dictation']) + if (store?.getState().activeModal === 'feature-tips') { + store.getState().closeModal() + } + }) +} + +async function waitForPhaseOneAtBottom(page: Page, tabId: string): Promise { + await expect + .poll( + () => + page.evaluate((targetTabId) => { + const pane = window.__paneManagers?.get(targetTabId)?.getPanes?.()[0] + const terminal = pane?.terminal + if (!terminal) { + return false + } + const buffer = terminal.buffer.active + let containsMarker = false + for (let line = buffer.baseY; line < buffer.baseY + terminal.rows; line += 1) { + if (buffer.getLine(line)?.translateToString(true).includes('STREAM_PHASE1_DONE')) { + containsMarker = true + break + } + } + const scrollbar = pane.container.querySelector( + '.xterm-scrollbar.xterm-vertical' + ) + const thumb = scrollbar?.querySelector('.xterm-slider') ?? null + return Boolean( + buffer.baseY > 0 && + buffer.viewportY === buffer.baseY && + containsMarker && + scrollbar && + thumb && + scrollbar.clientHeight - thumb.offsetHeight > 1 && + Math.abs(scrollbar.clientHeight - thumb.offsetHeight - thumb.offsetTop) <= 2 + ) + }, tabId), + { + timeout: 30_000, + message: 'phase-one output did not render with the visible viewport at the bottom' + } + ) + .toBe(true) +} + +async function injectQueuedWriteAndRefocus( + page: Page, + tabId: string, + paneKey: string +): Promise { + await page.evaluate( + ({ targetTabId, paneKey }) => { + const pane = window.__paneManagers?.get(targetTabId)?.getPanes?.()[0] + if (!pane) { + throw new Error('Hidden terminal pane unavailable') + } + const terminal = pane.terminal + // Why: fail loudly if xterm moves the private buffer path that models this wobble. + const bufferService = ( + terminal as typeof terminal & { + _core?: { + _bufferService?: { buffer?: { ydisp: number }; isUserScrolling: boolean } + } + } + )._core?._bufferService + const internalBuffer = bufferService?.buffer + if (!internalBuffer || !bufferService) { + throw new Error('xterm internal buffer unavailable') + } + const originalWrite = terminal.write + let wobbleApplied = false + terminal.write = ((data: string, callback?: () => void) => { + terminal.write = originalWrite + wobbleApplied = true + internalBuffer.ydisp = 0 + bufferService.isUserScrolling = true + if (terminal.buffer.active.viewportY !== 0) { + throw new Error('xterm viewport wobble was not observable') + } + originalWrite.call(terminal, data, callback) + }) as typeof terminal.write + const injector = ( + window as Window & { + __terminalPtyDataInjection?: { + inject: (paneKey: string, data: string) => boolean + } + } + ).__terminalPtyDataInjection + const rows = Array.from( + { length: 400 }, + (_, index) => `REFOCUS_STREAM_ROW_${String(index).padStart(4, '0')}_${'x'.repeat(80)}\n` + ).join('') + if (!injector?.inject(paneKey, `${rows}REFOCUS_STREAM_DONE\n`)) { + throw new Error('PTY data injector unavailable') + } + // Why: focus recovery must flush through terminal.write in this synchronous dispatch. + window.dispatchEvent(new Event('focus')) + if (!wobbleApplied) { + throw new Error('refocus did not flush the queued xterm write') + } + }, + { targetTabId: tabId, paneKey } + ) +} + +async function sampleRevealFrames(page: Page, targetTabId: string): Promise { + return page.evaluate( + (targetTabId) => + new Promise((resolve) => { + const frames: RevealFrame[] = [] + const startedAt = performance.now() + const isPresented = (element: Element | null): boolean => { + if (!(element instanceof HTMLElement)) { + return false + } + for ( + let current: HTMLElement | null = element; + current; + current = current.parentElement + ) { + const style = getComputedStyle(current) + if ( + style.display === 'none' || + style.visibility === 'hidden' || + style.opacity === '0' + ) { + return false + } + } + const rect = element.getBoundingClientRect() + return ( + rect.width > 0 && + rect.height > 0 && + rect.right > 0 && + rect.bottom > 0 && + rect.left < window.innerWidth && + rect.top < window.innerHeight + ) + } + const sample = (): void => { + const pane = window.__paneManagers?.get(targetTabId)?.getPanes?.()[0] + const targetXterm = pane?.container.querySelector('.xterm') ?? null + const scrollbar = + targetXterm?.querySelector('.xterm-scrollbar.xterm-vertical') ?? null + const thumb = scrollbar?.querySelector('.xterm-slider') ?? null + frames.push({ + targetPresented: isPresented(targetXterm), + thumbTop: thumb?.offsetTop ?? null, + maxThumbTop: scrollbar && thumb ? scrollbar.clientHeight - thumb.offsetHeight : null + }) + if (performance.now() - startedAt >= 700) { + resolve(frames) + return + } + requestAnimationFrame(sample) + } + sample() + }), + targetTabId + ) +} + +test.describe('terminal streaming refocus viewport', () => { + test('keeps follow-output at the bottom through a queued-write refocus wobble', async ({ + orcaPage + }) => { + await waitForSessionReady(orcaPage) + await closeFeatureTips(orcaPage) + await waitForActiveWorktree(orcaPage) + await ensureTerminalVisible(orcaPage) + await waitForActiveTerminalManager(orcaPage, 30_000) + const ptyId = await waitForActivePanePtyId(orcaPage) + const { paneKey } = await waitForActivePaneHookDescriptor(orcaPage) + const tabId = paneKey.slice(0, paneKey.indexOf(':')) + await waitForTerminalPtyDataInjector(orcaPage, paneKey) + await execInTerminal(orcaPage, ptyId, nodeTerminalCommand([STREAMING_FIXTURE_PATH])) + await waitForPhaseOneAtBottom(orcaPage, tabId) + + const framesPromise = sampleRevealFrames(orcaPage, tabId) + await injectQueuedWriteAndRefocus(orcaPage, tabId, paneKey) + const frames = await framesPromise + + expect(frames.filter((frame) => !frame.targetPresented)).toEqual([]) + expect( + frames.filter( + (frame) => + frame.thumbTop === null || + frame.maxThumbTop === null || + Math.abs(frame.maxThumbTop - frame.thumbTop) > 2 + ) + ).toEqual([]) + expect(frames.some((frame) => (frame.maxThumbTop ?? 0) > 1)).toBe(true) + expect( + frames.filter((frame) => (frame.maxThumbTop ?? 0) > 1 && (frame.thumbTop ?? 0) <= 1) + ).toEqual([]) + await expect + .poll(() => getTerminalContent(orcaPage), { timeout: 15_000 }) + .toContain('REFOCUS_STREAM_DONE') + const visibleScrollbar = orcaPage.locator('.xterm-scrollbar.xterm-vertical:visible').first() + await expect(visibleScrollbar).toBeVisible() + expect( + await visibleScrollbar.evaluate((scrollbar) => { + const thumb = scrollbar.querySelector('.xterm-slider') + return Boolean( + thumb && Math.abs(scrollbar.clientHeight - thumb.offsetHeight - thumb.offsetTop) <= 2 + ) + }) + ).toBe(true) + }) +})