From de7d2ef02f5080f5ed537fe11a15aa01433b454c Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 22 Jul 2026 13:27:41 -0700 Subject: [PATCH] fix(daemon): make login-session PTY probe reliable --- .../macos-login-session-pty-probe.test.ts | 128 ++++++++++++++++++ .../macos-login-session-pty-probe.ts | 76 +++++++++++ .../providers/macos-tcc-login-shell.test.ts | 45 +++--- src/main/providers/macos-tcc-login-shell.ts | 90 +++--------- 4 files changed, 245 insertions(+), 94 deletions(-) create mode 100644 src/main/providers/macos-login-session-pty-probe.test.ts create mode 100644 src/main/providers/macos-login-session-pty-probe.ts diff --git a/src/main/providers/macos-login-session-pty-probe.test.ts b/src/main/providers/macos-login-session-pty-probe.test.ts new file mode 100644 index 00000000000..8985c3aa01d --- /dev/null +++ b/src/main/providers/macos-login-session-pty-probe.test.ts @@ -0,0 +1,128 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { execFileMock, existsSyncMock, stdinEndMock } = vi.hoisted(() => ({ + execFileMock: vi.fn(), + existsSyncMock: vi.fn(), + stdinEndMock: vi.fn() +})) + +vi.mock('node:child_process', () => ({ execFile: execFileMock })) +vi.mock('node:fs', () => ({ existsSync: existsSyncMock })) + +import { runMacosLoginSessionPtyProbe } from './macos-login-session-pty-probe' + +type ExecFileCallback = (error: Error | null, stdout: string, stderr: string) => void + +describe('runMacosLoginSessionPtyProbe', () => { + beforeEach(() => { + existsSyncMock.mockReturnValue(true) + execFileMock.mockReset() + stdinEndMock.mockReset() + }) + + it('runs login under expect-owned PTY and requires its marker plus a clean exit', async () => { + execFileMock.mockImplementation( + (_file: string, _args: string[], _options: unknown, callback: ExecFileCallback) => { + callback(null, '^D\b\bORCA_LOGIN_PREFLIGHT_OK', '') + return { stdin: { end: stdinEndMock } } + } + ) + + await expect(runMacosLoginSessionPtyProbe('ada', '/Users/ada', 4_000, 1_024)).resolves.toEqual({ + ok: true, + conclusive: true, + reason: 'accepted' + }) + expect(execFileMock).toHaveBeenCalledWith( + '/usr/bin/expect', + [ + '-c', + expect.stringContaining( + 'spawn -noecho /usr/bin/login -flpq $env(ORCA_LOGIN_PROBE_USERNAME)' + ) + ], + expect.objectContaining({ + cwd: '/Users/ada', + env: expect.objectContaining({ ORCA_LOGIN_PROBE_USERNAME: 'ada' }), + killSignal: 'SIGKILL', + maxBuffer: 1_024, + timeout: 4_000 + }), + expect.any(Function) + ) + expect(stdinEndMock).toHaveBeenCalledOnce() + }) + + it('treats a natural exit without the marker as a conclusive rejection', async () => { + execFileMock.mockImplementation( + (_file: string, _args: string[], _options: unknown, callback: ExecFileCallback) => { + callback(null, 'Login incorrect\r\nlogin: ', '') + return { stdin: { end: stdinEndMock } } + } + ) + + await expect(runMacosLoginSessionPtyProbe('ada', '/Users/ada', 4_000, 1_024)).resolves.toEqual({ + ok: false, + conclusive: true, + reason: 'rejected' + }) + }) + + it('keeps a timeout or output overflow inconclusive', async () => { + execFileMock.mockImplementation( + (_file: string, _args: string[], _options: unknown, callback: ExecFileCallback) => { + callback(Object.assign(new Error('killed'), { killed: true }), '', '') + return { stdin: { end: stdinEndMock } } + } + ) + await expect(runMacosLoginSessionPtyProbe('ada', '/Users/ada', 4_000, 1_024)).resolves.toEqual({ + ok: false, + conclusive: false, + reason: 'timeout' + }) + + execFileMock.mockImplementation( + (_file: string, _args: string[], _options: unknown, callback: ExecFileCallback) => { + callback( + Object.assign(new Error('stdout maxBuffer length exceeded'), { + code: 'ERR_CHILD_PROCESS_STDIO_MAXBUFFER' + }), + '', + '' + ) + return { stdin: { end: stdinEndMock } } + } + ) + await expect(runMacosLoginSessionPtyProbe('ada', '/Users/ada', 4_000, 1_024)).resolves.toEqual({ + ok: false, + conclusive: false, + reason: 'error' + }) + }) + + it('does not mistake an expect wrapper failure for a PAM rejection', async () => { + execFileMock.mockImplementation( + (_file: string, _args: string[], _options: unknown, callback: ExecFileCallback) => { + callback(Object.assign(new Error('expect Tcl error'), { code: 1 }), '', '') + return { stdin: { end: stdinEndMock } } + } + ) + + await expect(runMacosLoginSessionPtyProbe('ada', '/Users/ada', 4_000, 1_024)).resolves.toEqual({ + ok: false, + conclusive: false, + reason: 'error' + }) + }) + + it('fails safe without spawning when expect is unavailable', async () => { + existsSyncMock.mockReturnValue(false) + + await expect(runMacosLoginSessionPtyProbe('ada', '/Users/ada', 4_000, 1_024)).resolves.toEqual({ + ok: false, + conclusive: false, + reason: 'error' + }) + expect(execFileMock).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/providers/macos-login-session-pty-probe.ts b/src/main/providers/macos-login-session-pty-probe.ts new file mode 100644 index 00000000000..2a514c1a1a6 --- /dev/null +++ b/src/main/providers/macos-login-session-pty-probe.ts @@ -0,0 +1,76 @@ +import { execFile, type ExecFileException } from 'node:child_process' +import { existsSync } from 'node:fs' + +const MACOS_EXPECT_PATH = '/usr/bin/expect' +const LOGIN_PREFLIGHT_MARKER = 'ORCA_LOGIN_PREFLIGHT_OK' +const LOGIN_PROBE_USERNAME_ENV = 'ORCA_LOGIN_PROBE_USERNAME' +// Why: expect owns the PTY without adding a long-lived native handle to the daemon. +const EXPECT_LOGIN_PROBE_SCRIPT = + 'log_user 1; ' + + 'spawn -noecho /usr/bin/login -flpq $env(ORCA_LOGIN_PROBE_USERNAME) /usr/bin/printf ORCA_LOGIN_PREFLIGHT_OK; ' + + 'send "\\004"; expect eof; wait; exit 0' + +export type LoginPreflightOutcome = { + ok: boolean + conclusive: boolean + reason: 'accepted' | 'rejected' | 'timeout' | 'error' +} + +export function classifyLoginPreflightError(error: ExecFileException): LoginPreflightOutcome { + // Why: a probe killed by our bound proves nothing about PAM and must not stick. + if (error.killed || error.code === 'ETIMEDOUT') { + return { ok: false, conclusive: false, reason: 'timeout' } + } + // Why: a natural nonzero exit is login(1)'s conclusive rejection verdict. + if (typeof error.code === 'number') { + return { ok: false, conclusive: true, reason: 'rejected' } + } + return { ok: false, conclusive: false, reason: 'error' } +} + +/** Runs the login-session oracle under a real PTY when the pipe probe cannot decide. */ +export function runMacosLoginSessionPtyProbe( + username: string, + accountHome: string, + timeoutMs: number, + maxOutputBytes: number +): Promise { + if (!existsSync(MACOS_EXPECT_PATH)) { + return Promise.resolve({ ok: false, conclusive: false, reason: 'error' }) + } + return new Promise((resolve) => { + try { + const child = execFile( + MACOS_EXPECT_PATH, + ['-c', EXPECT_LOGIN_PROBE_SCRIPT], + { + cwd: accountHome, + encoding: 'utf8', + env: { ...process.env, [LOGIN_PROBE_USERNAME_ENV]: username }, + killSignal: 'SIGKILL', + maxBuffer: maxOutputBytes, + timeout: timeoutMs + }, + (error, stdout) => { + if (error !== null) { + // Why: a nonzero expect exit can be its own Tcl/PTY failure, not PAM authority. + resolve( + error.killed || error.code === 'ETIMEDOUT' + ? { ok: false, conclusive: false, reason: 'timeout' } + : { ok: false, conclusive: false, reason: 'error' } + ) + return + } + resolve( + stdout.includes(LOGIN_PREFLIGHT_MARKER) + ? { ok: true, conclusive: true, reason: 'accepted' } + : { ok: false, conclusive: true, reason: 'rejected' } + ) + } + ) + child.stdin?.end() + } catch { + resolve({ ok: false, conclusive: false, reason: 'error' }) + } + }) +} diff --git a/src/main/providers/macos-tcc-login-shell.test.ts b/src/main/providers/macos-tcc-login-shell.test.ts index 09c81413fbc..dd03a59cc86 100644 --- a/src/main/providers/macos-tcc-login-shell.test.ts +++ b/src/main/providers/macos-tcc-login-shell.test.ts @@ -1,15 +1,23 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type * as LoginSessionPtyProbe from './macos-login-session-pty-probe' -const { existsSyncMock, userInfoMock, execFileMock, stdinEndMock } = vi.hoisted(() => ({ - existsSyncMock: vi.fn(), - userInfoMock: vi.fn(), - execFileMock: vi.fn(), - stdinEndMock: vi.fn() -})) +const { existsSyncMock, userInfoMock, execFileMock, stdinEndMock, ptyProbeMock } = vi.hoisted( + () => ({ + existsSyncMock: vi.fn(), + userInfoMock: vi.fn(), + execFileMock: vi.fn(), + stdinEndMock: vi.fn(), + ptyProbeMock: vi.fn() + }) +) vi.mock('node:fs', () => ({ existsSync: existsSyncMock })) vi.mock('node:os', () => ({ userInfo: userInfoMock })) vi.mock('node:child_process', () => ({ execFile: execFileMock })) +vi.mock('./macos-login-session-pty-probe', async (importOriginal) => ({ + ...(await importOriginal()), + runMacosLoginSessionPtyProbe: ptyProbeMock +})) import { prepareMacosTccLoginShell, @@ -40,6 +48,7 @@ describe('wrapShellSpawnForMacosTccAttribution', () => { return { stdin: { end: stdinEndMock } } } ) + ptyProbeMock.mockResolvedValue({ ok: true, conclusive: true, reason: 'accepted' }) resetMacosLoginShellPreflightForTests() }) @@ -362,6 +371,7 @@ describe('probeMacosLoginSessionAlive', () => { return { stdin: { end: stdinEndMock } } } ) + ptyProbeMock.mockResolvedValue({ ok: true, conclusive: true, reason: 'accepted' }) resetMacosLoginShellPreflightForTests() }) @@ -413,6 +423,7 @@ describe('probeMacosLoginSessionAlive', () => { return { stdin: { end: stdinEndMock } } } ) + ptyProbeMock.mockResolvedValue({ ok: false, conclusive: false, reason: 'timeout' }) const outcome = await probeMacosLoginSessionAlive() expect(outcome).toEqual({ ok: false, conclusive: false, reason: 'timeout' }) expect(wrapShellSpawnForMacosTccAttribution('/bin/zsh', ['-l']).file).toBe('/usr/bin/login') @@ -430,19 +441,15 @@ describe('probeMacosLoginSessionAlive', () => { it('escalates an inconclusive pipe probe to a PTY probe and accepts its verdict', async () => { setPlatform('darwin') execFileMock.mockImplementation( - (file: string, _args: string[], _options: unknown, callback: ExecFileCallback) => { - if (file === '/usr/bin/script') { - // PTYs echo control sequences around the marker. - callback(null, '[?1049h\r\nORCA_LOGIN_PREFLIGHT_OK\r\n', '') - } else { - callback(Object.assign(new Error('killed'), { killed: true }), '', '') - } + (_file: string, _args: string[], _options: unknown, callback: ExecFileCallback) => { + callback(Object.assign(new Error('killed'), { killed: true }), '', '') return { stdin: { end: stdinEndMock } } } ) const outcome = await probeMacosLoginSessionAlive() expect(outcome).toEqual({ ok: true, conclusive: true, reason: 'accepted' }) - expect(execFileMock.mock.calls.map((c) => c[0])).toEqual(['/usr/bin/login', '/usr/bin/script']) + expect(execFileMock).toHaveBeenCalledOnce() + expect(ptyProbeMock).toHaveBeenCalledWith('ada', '/Users/ada', 4_000, 1_024) expect(wrapShellSpawnForMacosTccAttribution('/bin/zsh', ['-l']).file).toBe('/usr/bin/login') }) @@ -450,15 +457,12 @@ describe('probeMacosLoginSessionAlive', () => { setPlatform('darwin') await prepareMacosTccLoginShell() execFileMock.mockImplementation( - (file: string, _args: string[], _options: unknown, callback: ExecFileCallback) => { - if (file === '/usr/bin/script') { - callback(Object.assign(new Error('login incorrect'), { code: 1 }), '', '') - } else { - callback(Object.assign(new Error('killed'), { killed: true }), '', '') - } + (_file: string, _args: string[], _options: unknown, callback: ExecFileCallback) => { + callback(Object.assign(new Error('killed'), { killed: true }), '', '') return { stdin: { end: stdinEndMock } } } ) + ptyProbeMock.mockResolvedValue({ ok: false, conclusive: true, reason: 'rejected' }) const outcome = await probeMacosLoginSessionAlive() expect(outcome).toEqual({ ok: false, conclusive: true, reason: 'rejected' }) expect(wrapShellSpawnForMacosTccAttribution('/bin/zsh', ['-l']).file).toBe('/bin/zsh') @@ -473,6 +477,7 @@ describe('probeMacosLoginSessionAlive', () => { return { stdin: { end: stdinEndMock } } } ) + ptyProbeMock.mockResolvedValue({ ok: false, conclusive: false, reason: 'timeout' }) const outcome = await probeMacosLoginSessionAlive() expect(outcome).toEqual({ ok: false, conclusive: false, reason: 'timeout' }) // Inconclusive must not disturb the cached acceptance. diff --git a/src/main/providers/macos-tcc-login-shell.ts b/src/main/providers/macos-tcc-login-shell.ts index c01e4a74664..76ef1343825 100644 --- a/src/main/providers/macos-tcc-login-shell.ts +++ b/src/main/providers/macos-tcc-login-shell.ts @@ -1,11 +1,17 @@ -import { execFile, type ExecFileException } from 'node:child_process' +import { execFile } from 'node:child_process' import { existsSync } from 'node:fs' import { userInfo } from 'node:os' +import { + classifyLoginPreflightError, + runMacosLoginSessionPtyProbe, + type LoginPreflightOutcome +} from './macos-login-session-pty-probe' + +export type { LoginPreflightOutcome } from './macos-login-session-pty-probe' const MACOS_LOGIN_PATH = '/usr/bin/login' const MACOS_ENV_PATH = '/usr/bin/env' const MACOS_PRINTF_PATH = '/usr/bin/printf' -const MACOS_SCRIPT_PATH = '/usr/bin/script' const LOGIN_PREFLIGHT_TIMEOUT_MS = 500 // Why: the death-watch probe runs off the spawn path, so it can afford a bound // that outlasts a PAM stack answering slowly rather than misreading it as a hang. @@ -27,12 +33,6 @@ const DISABLE_ENV_VAR = 'ORCA_DISABLE_MACOS_LOGIN_SHELL' * reject) that may be cached; an inconclusive probe (our own timeout/SIGKILL, * maxBuffer, or spawn error) proves nothing about PAM and must not stick. */ -export type LoginPreflightOutcome = { - ok: boolean - conclusive: boolean - reason: 'accepted' | 'rejected' | 'timeout' | 'error' -} - let cachedLoginPreflightResult: boolean | null = null let loginPreflightInFlight: Promise | null = null let transientLoginPreflightFailure: { failureCount: number; retryAtMs: number } | null = null @@ -49,21 +49,6 @@ function loginPreflightRetryDelayMs(failureCount: number): number { ) } -function classifyPreflightError(error: ExecFileException): LoginPreflightOutcome { - // Why: our SIGKILL timeout cap (and maxBuffer, which also kills) is an - // environmental slow-path, not a PAM verdict — retry, don't cache (F1). - if (error.killed || error.code === 'ETIMEDOUT') { - return { ok: false, conclusive: false, reason: 'timeout' } - } - // A numeric exit code means login(1) ran to completion and rejected the user - // (it exits immediately on EOF-driven rejection); that verdict is cacheable. - if (typeof error.code === 'number') { - return { ok: false, conclusive: true, reason: 'rejected' } - } - // Spawn/EOF/other failure: inconclusive, fail open for this spawn but retry. - return { ok: false, conclusive: false, reason: 'error' } -} - // Fidelity limit: the probe runs over pipes while production shells run under a // real PTY, so a tty-sensitive PAM stack could diverge. It fails safe — a probe // pass with a prod failure only degrades to today's direct spawn (no wrapper). @@ -99,7 +84,7 @@ function runLoginPreflight( ) return } - resolve(classifyPreflightError(error)) + resolve(classifyLoginPreflightError(error)) } ) // Why: login(1) must see immediate EOF, not an interactive pipe, so a PAM @@ -111,54 +96,6 @@ function runLoginPreflight( }) } -// Why: a dead login session's PAM stack may only misbehave under a real tty -// (the pipe-vs-PTY fidelity limit above), so death detection can escalate to -// running login(1) under script(1)'s PTY when the pipe probe is inconclusive. -function runPtyLoginProbe( - username: string, - accountHome: string, - timeoutMs: number -): Promise { - return new Promise((resolve) => { - try { - const child = execFile( - MACOS_SCRIPT_PATH, - [ - '-q', - '/dev/null', - MACOS_LOGIN_PATH, - '-flpq', - username, - MACOS_PRINTF_PATH, - LOGIN_PREFLIGHT_MARKER - ], - { - cwd: accountHome, - encoding: 'utf8', - killSignal: 'SIGKILL', - maxBuffer: LOGIN_PREFLIGHT_MAX_BUFFER_BYTES, - timeout: timeoutMs - }, - (error, stdout) => { - if (error === null) { - // Why includes(): the PTY echoes control sequences around the marker. - resolve( - stdout.includes(LOGIN_PREFLIGHT_MARKER) - ? { ok: true, conclusive: true, reason: 'accepted' } - : { ok: false, conclusive: true, reason: 'rejected' } - ) - return - } - resolve(classifyPreflightError(error)) - } - ) - child.stdin?.end() - } catch { - resolve({ ok: false, conclusive: false, reason: 'error' }) - } - }) -} - function cachedOutcome(): LoginPreflightOutcome | null { if (cachedLoginPreflightResult === null) { return null @@ -276,8 +213,13 @@ export async function probeMacosLoginSessionAlive(): Promise