diff --git a/config/scripts/verify-computer-native.mjs b/config/scripts/verify-computer-native.mjs index 91c27e03a5b..dee0226b374 100644 --- a/config/scripts/verify-computer-native.mjs +++ b/config/scripts/verify-computer-native.mjs @@ -313,6 +313,16 @@ function verifyNativeArgumentGuardrails() { 'macOS typeText must try verified focused AX text replacement before synthetic keyboard fallback' ) } + if ( + !macos.includes('AgentLaunchArguments.parse(arguments)') || + !macos.includes('expectedProcessId: expectedPeerProcessId') || + macos.includes('/bin/ps') || + macos.includes('computer-sidecar.js') + ) { + failures.push( + 'macOS agent authorization must bind the kernel peer pid to the main-selected launch pid' + ) + } if (!macos.includes('even when the click uses an AX action path')) { failures.push( 'macOS clicks must activate the target window before accessibility or pointer input' diff --git a/native/computer-use-macos/Sources/OrcaComputerUseMacOS/main.swift b/native/computer-use-macos/Sources/OrcaComputerUseMacOS/main.swift index 87100b2faec..d3454ecb953 100644 --- a/native/computer-use-macos/Sources/OrcaComputerUseMacOS/main.swift +++ b/native/computer-use-macos/Sources/OrcaComputerUseMacOS/main.swift @@ -2583,12 +2583,14 @@ private final class AgentRuntime: NSObject, NSApplicationDelegate { private let socketPath: String private let token: String? + private let expectedPeerProcessId: pid_t private var listener: SocketListener? private var unclaimedSessionTimeout: DispatchWorkItem? - init(socketPath: String, token: String?) { + init(socketPath: String, token: String?, expectedPeerProcessId: pid_t) { self.socketPath = socketPath self.token = token + self.expectedPeerProcessId = expectedPeerProcessId } func applicationDidFinishLaunching(_ notification: Notification) { @@ -2601,6 +2603,7 @@ private final class AgentRuntime: NSObject, NSApplicationDelegate { let listener = try SocketListener( socketPath: socketPath, token: token, + expectedPeerProcessId: expectedPeerProcessId, onSessionClaimed: { DispatchQueue.main.async { timeout.cancel() @@ -3532,6 +3535,7 @@ private final class ButtonTarget: NSObject { private final class SocketListener: @unchecked Sendable { private let socketPath: String private let token: String? + private let expectedPeerProcessId: pid_t private let onSessionClaimed: () -> Void private let onSessionClosed: () -> Void private let provider = Provider() @@ -3544,11 +3548,13 @@ private final class SocketListener: @unchecked Sendable { init( socketPath: String, token: String?, + expectedPeerProcessId: pid_t, onSessionClaimed: @escaping () -> Void, onSessionClosed: @escaping () -> Void ) throws { self.socketPath = socketPath self.token = token + self.expectedPeerProcessId = expectedPeerProcessId self.onSessionClaimed = onSessionClaimed self.onSessionClosed = onSessionClosed try bindSocket() @@ -3641,7 +3647,10 @@ private final class SocketListener: @unchecked Sendable { onSessionClosed() } } - let authorizedPeer = peerProcessId(fd).map(isAuthorizedAgentPeer) == true + let authorizedPeer = isAuthorizedAgentPeer( + peerProcessId: peerProcessId(fd), + expectedProcessId: expectedPeerProcessId + ) let decoder = JSONDecoder() var registrationComplete = false while let line = readLine(from: fd) { @@ -3700,70 +3709,14 @@ private func peerProcessId(_ fd: Int32) -> pid_t? { return result == 0 && pid > 0 ? pid : nil } -private func isAuthorizedAgentPeer(_ pid: pid_t) -> Bool { - guard let command = processCommand(pid), - command.contains("/out/main/computer-sidecar.js") - || command.contains("/Contents/Resources/app.asar.unpacked/out/main/computer-sidecar.js") - else { - return false - } - if isTrustedOrcaApplication(pid) { - return true - } - guard let parentPid = parentProcessId(pid) else { return false } - return isTrustedOrcaApplication(parentPid) -} - -private func isTrustedOrcaApplication(_ pid: pid_t) -> Bool { - guard let app = NSRunningApplication(processIdentifier: pid), - let bundleId = app.bundleIdentifier - else { - return false - } - // Why: dev validation runs from per-worktree wrapper apps with stable - // Orca-owned bundle ids; the sidecar peer check must still authorize them. - return bundleId == "com.stablyai.orca" || - bundleId.hasPrefix("com.stablyai.orca.dev.") || - bundleId == "com.github.Electron" -} - -private func parentProcessId(_ pid: pid_t) -> pid_t? { - guard let output = processField(pid: pid, field: "ppid=") else { - return nil - } - let trimmed = output.trimmingCharacters(in: .whitespacesAndNewlines) - guard let parentPid = pid_t(trimmed), parentPid > 1 else { - return nil - } - return parentPid -} - -private func processCommand(_ pid: pid_t) -> String? { - return processField(pid: pid, field: "command=") -} - -private func processField(pid: pid_t, field: String) -> String? { - let process = Process() - let pipe = Pipe() - process.executableURL = URL(fileURLWithPath: "/bin/ps") - process.arguments = ["-p", "\(pid)", "-o", field] - process.standardOutput = pipe - process.standardError = Pipe() - do { - try process.run() - process.waitUntilExit() - guard process.terminationStatus == 0 else { return nil } - let data = pipe.fileHandleForReading.readDataToEndOfFile() - return String(data: data, encoding: .utf8) - } catch { - return nil - } -} - @MainActor -private func runAgent(socketPath: String, token: String?) { +private func runAgent(socketPath: String, token: String?, expectedPeerProcessId: pid_t) { let app = NSApplication.shared - let delegate = AgentRuntime(socketPath: socketPath, token: token) + let delegate = AgentRuntime( + socketPath: socketPath, + token: token, + expectedPeerProcessId: expectedPeerProcessId + ) app.delegate = delegate // Why: SCK is reliable once this code runs as a signed app with a real TCC identity. setenv("ORCA_COMPUTER_USE_SCK_SCREENSHOTS", "1", 1) @@ -3896,23 +3849,23 @@ private func writeAll(_ data: Data, to fd: Int32) -> Bool { let arguments = Array(CommandLine.arguments.dropFirst()) if arguments.first == "--agent" { - guard arguments.count >= 2 else { - fputs("usage: orca-computer-use-macos --agent --token-file \n", stderr) + guard let launchArguments = AgentLaunchArguments.parse(arguments) else { + fputs("usage: orca-computer-use-macos --agent --token-file --peer-pid \n", stderr) exit(2) } - let tokenFileIndex = arguments.firstIndex(of: "--token-file") - let token = tokenFileIndex.flatMap { index -> String? in - let valueIndex = index + 1 - guard valueIndex < arguments.count else { return nil } - let tokenPath = arguments[valueIndex] - return try? String(contentsOfFile: tokenPath, encoding: .utf8) - .trimmingCharacters(in: .whitespacesAndNewlines) - } + let token = try? String( + contentsOfFile: launchArguments.tokenFilePath, + encoding: .utf8 + ).trimmingCharacters(in: .whitespacesAndNewlines) guard let token, !token.isEmpty else { fputs("orca-computer-use-macos --agent requires a non-empty --token-file\n", stderr) exit(2) } - runAgent(socketPath: arguments[1], token: token) + runAgent( + socketPath: launchArguments.socketPath, + token: token, + expectedPeerProcessId: launchArguments.expectedPeerProcessId + ) } else if arguments.first == "--permissions" { runPermissionCheck() } else if arguments.first == "--permission" { diff --git a/native/computer-use-macos/Sources/OrcaComputerUseMacOSCore/AgentPeerAuthorization.swift b/native/computer-use-macos/Sources/OrcaComputerUseMacOSCore/AgentPeerAuthorization.swift new file mode 100644 index 00000000000..1199d27a8ef --- /dev/null +++ b/native/computer-use-macos/Sources/OrcaComputerUseMacOSCore/AgentPeerAuthorization.swift @@ -0,0 +1,31 @@ +public struct AgentLaunchArguments: Equatable, Sendable { + public let socketPath: String + public let tokenFilePath: String + public let expectedPeerProcessId: Int32 + + public static func parse(_ arguments: [String]) -> AgentLaunchArguments? { + guard arguments.count == 6, + arguments[0] == "--agent", + !arguments[1].isEmpty, + arguments[2] == "--token-file", + !arguments[3].isEmpty, + arguments[4] == "--peer-pid", + let expectedPeerProcessId = Int32(arguments[5]), + expectedPeerProcessId > 0 + else { + return nil + } + return AgentLaunchArguments( + socketPath: arguments[1], + tokenFilePath: arguments[3], + expectedPeerProcessId: expectedPeerProcessId + ) + } +} + +public func isAuthorizedAgentPeer( + peerProcessId: Int32?, + expectedProcessId: Int32 +) -> Bool { + expectedProcessId > 0 && peerProcessId == expectedProcessId +} diff --git a/native/computer-use-macos/Tests/OrcaComputerUseMacOSTests/AgentEntrypointSourceSafetyTests.swift b/native/computer-use-macos/Tests/OrcaComputerUseMacOSTests/AgentEntrypointSourceSafetyTests.swift index 389b83bac6b..3e7bcdbc431 100644 --- a/native/computer-use-macos/Tests/OrcaComputerUseMacOSTests/AgentEntrypointSourceSafetyTests.swift +++ b/native/computer-use-macos/Tests/OrcaComputerUseMacOSTests/AgentEntrypointSourceSafetyTests.swift @@ -34,6 +34,20 @@ final class AgentEntrypointSourceSafetyTests: XCTestCase { XCTAssertTrue(source.contains("event.flags = flags\n event.postToPid(pid)")) } + func testAgentUsesExactKernelPeerPidWithoutProcessInspection() throws { + let source = try agentEntrypointSource() + + XCTAssertTrue(source.contains( + """ + let authorizedPeer = isAuthorizedAgentPeer( + peerProcessId: peerProcessId(fd), + expectedProcessId: expectedPeerProcessId + """ + )) + XCTAssertFalse(source.contains("/bin/ps")) + XCTAssertFalse(source.contains("computer-sidecar.js")) + } + private func agentEntrypointSource() throws -> String { let testFile = URL(fileURLWithPath: #filePath) let packageRoot = testFile diff --git a/native/computer-use-macos/Tests/OrcaComputerUseMacOSTests/AgentPeerAuthorizationTests.swift b/native/computer-use-macos/Tests/OrcaComputerUseMacOSTests/AgentPeerAuthorizationTests.swift new file mode 100644 index 00000000000..092367e6c31 --- /dev/null +++ b/native/computer-use-macos/Tests/OrcaComputerUseMacOSTests/AgentPeerAuthorizationTests.swift @@ -0,0 +1,52 @@ +import XCTest +@testable import OrcaComputerUseMacOSCore + +final class AgentPeerAuthorizationTests: XCTestCase { + func testParsesExactAgentArguments() { + XCTAssertEqual( + AgentLaunchArguments.parse([ + "--agent", + "/private/tmp/provider.sock", + "--token-file", + "/private/tmp/provider.token", + "--peer-pid", + "4321", + ]), + AgentLaunchArguments( + socketPath: "/private/tmp/provider.sock", + tokenFilePath: "/private/tmp/provider.token", + expectedPeerProcessId: 4321 + ) + ) + } + + func testRejectsMissingReorderedOrExtraAgentArguments() { + XCTAssertNil(AgentLaunchArguments.parse([ + "--agent", "/tmp/provider.sock", "--token-file", "/tmp/provider.token", + ])) + XCTAssertNil(AgentLaunchArguments.parse([ + "--agent", "/tmp/provider.sock", "--peer-pid", "4321", + "--token-file", "/tmp/provider.token", + ])) + XCTAssertNil(AgentLaunchArguments.parse([ + "--agent", "/tmp/provider.sock", "--token-file", "/tmp/provider.token", + "--peer-pid", "4321", "--extra", + ])) + } + + func testRejectsInvalidPeerProcessIds() { + for value in ["", "0", "-1", "not-a-pid", "2147483648"] { + XCTAssertNil(AgentLaunchArguments.parse([ + "--agent", "/tmp/provider.sock", "--token-file", "/tmp/provider.token", + "--peer-pid", value, + ])) + } + } + + func testAuthorizesOnlyTheExpectedPeerProcess() { + XCTAssertTrue(isAuthorizedAgentPeer(peerProcessId: 4321, expectedProcessId: 4321)) + XCTAssertFalse(isAuthorizedAgentPeer(peerProcessId: 4322, expectedProcessId: 4321)) + XCTAssertFalse(isAuthorizedAgentPeer(peerProcessId: nil, expectedProcessId: 4321)) + XCTAssertFalse(isAuthorizedAgentPeer(peerProcessId: 4321, expectedProcessId: 0)) + } +} diff --git a/src/main/computer/computer-provider-supervisor-host.test.ts b/src/main/computer/computer-provider-supervisor-host.test.ts index 46848214f9f..c5951d87d13 100644 --- a/src/main/computer/computer-provider-supervisor-host.test.ts +++ b/src/main/computer/computer-provider-supervisor-host.test.ts @@ -27,7 +27,7 @@ describe('ComputerProviderSupervisorHost', () => { const macOS = macOSSupervisor() const host = new ComputerProviderSupervisorHost(macOS as never) const sent: unknown[] = [] - host.attach((message) => sent.push(message)) + host.attach((message) => sent.push(message), 4321) expect( host.handle({ @@ -40,7 +40,7 @@ describe('ComputerProviderSupervisorHost', () => { ).toBe(true) await vi.waitFor(() => expect(sent).toHaveLength(1)) - expect(macOS.start).toHaveBeenCalledTimes(1) + expect(macOS.start).toHaveBeenCalledWith(4321) expect(sent).toEqual([ { channel: COMPUTER_PROVIDER_SUPERVISOR_CHANNEL, @@ -61,7 +61,7 @@ describe('ComputerProviderSupervisorHost', () => { const desktop = desktopSupervisor() const host = new ComputerProviderSupervisorHost(macOS as never, desktop as never) const sent: unknown[] = [] - host.attach((message) => sent.push(message)) + host.attach((message) => sent.push(message), 4321) expect( host.handle({ @@ -112,7 +112,7 @@ describe('ComputerProviderSupervisorHost', () => { const host = new ComputerProviderSupervisorHost(macOSSupervisor() as never, desktop as never) const originalSend = vi.fn() const replacementSend = vi.fn() - host.attach(originalSend) + host.attach(originalSend, 4321) host.handle({ channel: COMPUTER_PROVIDER_SUPERVISOR_CHANNEL, kind: 'request', @@ -122,7 +122,7 @@ describe('ComputerProviderSupervisorHost', () => { }) host.shutdown() - host.attach(replacementSend) + host.attach(replacementSend, 5432) resolveExecution({ stdout: '{"ok":true}', stderr: '', error: null }) await new Promise((resolve) => setImmediate(resolve)) @@ -130,6 +130,28 @@ describe('ComputerProviderSupervisorHost', () => { expect(replacementSend).not.toHaveBeenCalled() }) + it('uses the replacement owner pid for a restarted macOS helper', async () => { + const macOS = macOSSupervisor() + const host = new ComputerProviderSupervisorHost(macOS as never) + const replacementSend = vi.fn() + host.attach(vi.fn(), 4321) + host.shutdown() + host.attach(replacementSend, 5432) + + expect( + host.handle({ + channel: COMPUTER_PROVIDER_SUPERVISOR_CHANNEL, + kind: 'request', + id: 5, + method: 'macos.start', + params: {} + }) + ).toBe(true) + await vi.waitFor(() => expect(replacementSend).toHaveBeenCalledTimes(1)) + + expect(macOS.start).toHaveBeenCalledWith(5432) + }) + it('rejects arbitrary executable and argument fields at the protocol boundary', () => { const macOS = macOSSupervisor() const host = new ComputerProviderSupervisorHost(macOS as never) @@ -140,7 +162,7 @@ describe('ComputerProviderSupervisorHost', () => { kind: 'request', id: 1, method: 'macos.start', - params: { executablePath: '/tmp/untrusted', args: ['--anything'] } + params: { executablePath: '/tmp/untrusted', args: ['--anything'], peerPid: 9999 } }) ).toBe(false) expect(macOS.start).not.toHaveBeenCalled() @@ -151,7 +173,7 @@ describe('ComputerProviderSupervisorHost', () => { const desktop = desktopSupervisor() const host = new ComputerProviderSupervisorHost(macOS as never, desktop as never) const send = vi.fn() - host.attach(send) + host.attach(send, 4321) host.shutdown() @@ -169,4 +191,15 @@ describe('ComputerProviderSupervisorHost', () => { expect(macOS.start).not.toHaveBeenCalled() expect(send).not.toHaveBeenCalled() }) + + it.each([0, -1, 1.5, Number.NaN, 0x80000000])( + 'rejects invalid owner pid %s before accepting requests', + (ownerProcessId) => { + const host = new ComputerProviderSupervisorHost(macOSSupervisor() as never) + + expect(() => host.attach(vi.fn(), ownerProcessId)).toThrow( + 'owner process did not report a valid pid' + ) + } + ) }) diff --git a/src/main/computer/computer-provider-supervisor-host.ts b/src/main/computer/computer-provider-supervisor-host.ts index b91e1a1789a..57b1cd76f23 100644 --- a/src/main/computer/computer-provider-supervisor-host.ts +++ b/src/main/computer/computer-provider-supervisor-host.ts @@ -15,6 +15,7 @@ type SupervisorResponsePayload = export class ComputerProviderSupervisorHost { private sender: SupervisorMessageSender | null = null + private ownerProcessId: number | null = null private ownerGeneration = 0 private readonly macOS: MacOSNativeProviderSupervisor private readonly desktop: DesktopScriptProviderSupervisor @@ -24,17 +25,26 @@ export class ComputerProviderSupervisorHost { this.desktop = desktop ?? new DesktopScriptProviderSupervisor() } - attach(sender: SupervisorMessageSender): void { + attach(sender: SupervisorMessageSender, ownerProcessId: number): void { + if (!Number.isInteger(ownerProcessId) || ownerProcessId <= 0 || ownerProcessId > 0x7fffffff) { + throw new Error('computer provider owner process did not report a valid pid') + } this.ownerGeneration++ this.sender = sender + this.ownerProcessId = ownerProcessId } handle(message: unknown): boolean { - if (!this.sender || !isComputerProviderSupervisorRequest(message)) { + if ( + !this.sender || + this.ownerProcessId === null || + !isComputerProviderSupervisorRequest(message) + ) { return false } const ownerGeneration = this.ownerGeneration - void this.dispatch(message).then( + const ownerProcessId = this.ownerProcessId + void this.dispatch(message, ownerProcessId).then( (result) => this.send(ownerGeneration, { id: message.id, ok: true, result }), (error) => this.send(ownerGeneration, { @@ -49,14 +59,18 @@ export class ComputerProviderSupervisorHost { shutdown(): void { this.ownerGeneration++ this.sender = null + this.ownerProcessId = null this.macOS.shutdown() this.desktop.shutdown() } - private async dispatch(request: ComputerProviderSupervisorRequest): Promise { + private async dispatch( + request: ComputerProviderSupervisorRequest, + ownerProcessId: number + ): Promise { switch (request.method) { case 'macos.start': - return this.macOS.start() + return this.macOS.start(ownerProcessId) case 'macos.claim': this.macOS.claim(request.params.sessionId) return { claimed: true } diff --git a/src/main/computer/computer-provider-supervisor-protocol.test.ts b/src/main/computer/computer-provider-supervisor-protocol.test.ts index cb1f9f262e6..f2184e7a603 100644 --- a/src/main/computer/computer-provider-supervisor-protocol.test.ts +++ b/src/main/computer/computer-provider-supervisor-protocol.test.ts @@ -18,6 +18,20 @@ function desktopRequest(request: Record): Record { + it('does not let the sidecar choose the macOS helper peer pid', () => { + const request = { + channel: COMPUTER_PROVIDER_SUPERVISOR_CHANNEL, + kind: 'request', + id: 1, + method: 'macos.start' + } + + expect(isComputerProviderSupervisorRequest({ ...request, params: {} })).toBe(true) + expect(isComputerProviderSupervisorRequest({ ...request, params: { peerPid: 4321 } })).toBe( + false + ) + }) + it('accepts exact desktop tool requests with validated nested element state', () => { expect( isComputerProviderSupervisorRequest( diff --git a/src/main/computer/computer-sidecar-process.ts b/src/main/computer/computer-sidecar-process.ts index 2b270fbc69a..0c743bf24fe 100644 --- a/src/main/computer/computer-sidecar-process.ts +++ b/src/main/computer/computer-sidecar-process.ts @@ -2,6 +2,7 @@ import { fork, type ChildProcess } from 'node:child_process' import { join } from 'node:path' import { ComputerProviderSupervisorHost } from './computer-provider-supervisor-host' import type { ComputerProviderSupervisorMessage } from './computer-provider-supervisor-protocol' +import { terminateComputerSidecarChild } from './computer-sidecar-termination' import { RuntimeClientError } from './runtime-client-error' export type ComputerSidecarMethod = @@ -36,38 +37,10 @@ type PendingRequest = { } const REQUEST_TIMEOUT_MS = 60_000 -export const COMPUTER_SIDECAR_FORCE_KILL_GRACE_MS = 5_000 // Why: stale children need an error listener without retaining their former owner. function ignoreStaleChildError(): void {} -function terminateSidecarChild(child: ChildProcess): void { - let exited = false - let forceKillTimer: NodeJS.Timeout | null = null - const onExit = (): void => { - exited = true - if (forceKillTimer) { - clearTimeout(forceKillTimer) - forceKillTimer = null - } - } - child.once('exit', onExit) - try { - child.kill('SIGTERM') - } catch {} - if (exited) { - return - } - forceKillTimer = setTimeout(() => { - forceKillTimer = null - child.off('exit', onExit) - try { - child.kill('SIGKILL') - } catch {} - }, COMPUTER_SIDECAR_FORCE_KILL_GRACE_MS) - forceKillTimer.unref() -} - export class ComputerSidecarProcess { private child: ChildProcess | null = null private childListenerCleanup: (() => void) | null = null @@ -116,12 +89,17 @@ export class ComputerSidecarProcess { this.pending.delete(id) } if (child) { - terminateSidecarChild(child) + terminateComputerSidecarChild(child) } } private send(method: ComputerSidecarMethod, params: unknown): Promise { - const child = this.ensureStarted() + let child: ChildProcess + try { + child = this.ensureStarted() + } catch (error) { + return Promise.reject(error) + } if (!child.send) { const error = new RuntimeClientError( 'accessibility_error', @@ -196,8 +174,23 @@ export class ComputerSidecarProcess { child.on('error', ignoreStaleChildError) } this.child = child - this.providerSupervisor.attach((message) => - this.sendComputerProviderSupervisorMessage(child, message) + const childProcessId = child.pid + if ( + typeof childProcessId !== 'number' || + !Number.isInteger(childProcessId) || + childProcessId <= 0 || + childProcessId > 0x7fffffff + ) { + const error = new RuntimeClientError( + 'accessibility_error', + 'computer sidecar process did not report a valid pid' + ) + this.failActiveChild(child, error) + throw error + } + this.providerSupervisor.attach( + (message) => this.sendComputerProviderSupervisorMessage(child, message), + childProcessId ) return child } @@ -250,7 +243,7 @@ export class ComputerSidecarProcess { this.child = null this.queueGeneration++ this.providerSupervisor.shutdown() - terminateSidecarChild(child) + terminateComputerSidecarChild(child) this.rejectPending(error) } diff --git a/src/main/computer/computer-sidecar-termination.ts b/src/main/computer/computer-sidecar-termination.ts new file mode 100644 index 00000000000..28c261216b0 --- /dev/null +++ b/src/main/computer/computer-sidecar-termination.ts @@ -0,0 +1,30 @@ +import type { ChildProcess } from 'node:child_process' + +export const COMPUTER_SIDECAR_FORCE_KILL_GRACE_MS = 5_000 + +export function terminateComputerSidecarChild(child: ChildProcess): void { + let exited = false + let forceKillTimer: NodeJS.Timeout | null = null + const onExit = (): void => { + exited = true + if (forceKillTimer) { + clearTimeout(forceKillTimer) + forceKillTimer = null + } + } + child.once('exit', onExit) + try { + child.kill('SIGTERM') + } catch {} + if (exited) { + return + } + forceKillTimer = setTimeout(() => { + forceKillTimer = null + child.off('exit', onExit) + try { + child.kill('SIGKILL') + } catch {} + }, COMPUTER_SIDECAR_FORCE_KILL_GRACE_MS) + forceKillTimer.unref() +} diff --git a/src/main/computer/macos-native-provider-supervisor.test.ts b/src/main/computer/macos-native-provider-supervisor.test.ts index 86e81537ec2..98cf3f00b61 100644 --- a/src/main/computer/macos-native-provider-supervisor.test.ts +++ b/src/main/computer/macos-native-provider-supervisor.test.ts @@ -48,7 +48,7 @@ describe('MacOSNativeProviderSupervisor', () => { it('registers the helper pid and private session state before returning', () => { const supervisor = new MacOSNativeProviderSupervisor((event) => events.push(event), deps) - const started = supervisor.start() + const started = supervisor.start(7654) expect(started).toEqual({ sessionId: 'uuid-2', @@ -61,7 +61,9 @@ describe('MacOSNativeProviderSupervisor', () => { '--agent', join('/private/tmp/orca-computer-use-session', 'provider.sock'), '--token-file', - join('/private/tmp/orca-computer-use-session', 'provider.token') + join('/private/tmp/orca-computer-use-session', 'provider.token'), + '--peer-pid', + '7654' ], { detached: true, stdio: 'ignore' } ) @@ -77,7 +79,7 @@ describe('MacOSNativeProviderSupervisor', () => { it('claims a live session and removes only its token file', async () => { const supervisor = new MacOSNativeProviderSupervisor((event) => events.push(event), deps) - const started = supervisor.start() + const started = supervisor.start(7654) supervisor.claim(started.sessionId) await vi.advanceTimersByTimeAsync(MACOS_HELPER_CLAIM_TIMEOUT_MS) @@ -92,7 +94,7 @@ describe('MacOSNativeProviderSupervisor', () => { it('reaps an unclaimed helper and removes private state on the parent deadline', async () => { const supervisor = new MacOSNativeProviderSupervisor((event) => events.push(event), deps) - supervisor.start() + supervisor.start(7654) await vi.advanceTimersByTimeAsync(MACOS_HELPER_CLAIM_TIMEOUT_MS) @@ -118,7 +120,7 @@ describe('MacOSNativeProviderSupervisor', () => { it('releases a claimed session with graceful-to-force escalation', async () => { const supervisor = new MacOSNativeProviderSupervisor((event) => events.push(event), deps) - const started = supervisor.start() + const started = supervisor.start(7654) supervisor.claim(started.sessionId) supervisor.release(started.sessionId) @@ -147,8 +149,8 @@ describe('MacOSNativeProviderSupervisor', () => { .mockReturnValueOnce('/private/tmp/orca-computer-use-first') .mockReturnValueOnce('/private/tmp/orca-computer-use-second') const supervisor = new MacOSNativeProviderSupervisor((event) => events.push(event), deps) - supervisor.start() - supervisor.start() + supervisor.start(7654) + supervisor.start(7654) supervisor.shutdown() @@ -172,7 +174,7 @@ describe('MacOSNativeProviderSupervisor', () => { it('reports unexpected helper exit and removes its private directory', () => { const supervisor = new MacOSNativeProviderSupervisor((event) => events.push(event), deps) - supervisor.start() + supervisor.start(7654) child.emit('exit', 13, null) @@ -190,7 +192,7 @@ describe('MacOSNativeProviderSupervisor', () => { it('reports a registered helper error and retains ownership until exit', () => { const supervisor = new MacOSNativeProviderSupervisor((event) => events.push(event), deps) - const started = supervisor.start() + const started = supervisor.start(7654) child.emit('error', new Error('helper handle failed')) @@ -212,7 +214,7 @@ describe('MacOSNativeProviderSupervisor', () => { child.pid = undefined const supervisor = new MacOSNativeProviderSupervisor((event) => events.push(event), deps) - expect(() => supervisor.start()).toThrow('helper process did not report a pid') + expect(() => supervisor.start(7654)).toThrow('helper process did not report a pid') expect(child.kill).toHaveBeenCalledWith('SIGKILL') expect(deps.rmSync).toHaveBeenCalledWith('/private/tmp/orca-computer-use-session', { @@ -229,7 +231,7 @@ describe('MacOSNativeProviderSupervisor', () => { }) const supervisor = new MacOSNativeProviderSupervisor((event) => events.push(event), deps) - expect(() => supervisor.start()).toThrow('token write failed') + expect(() => supervisor.start(7654)).toThrow('token write failed') expect(deps.spawn).not.toHaveBeenCalled() expect(deps.rmSync).toHaveBeenCalledWith('/private/tmp/orca-computer-use-session', { @@ -237,4 +239,18 @@ describe('MacOSNativeProviderSupervisor', () => { force: true }) }) + + it.each([0, -1, 1.5, Number.NaN, 0x80000000])( + 'rejects invalid expected peer pid %s before creating private state', + (expectedPeerProcessId) => { + const supervisor = new MacOSNativeProviderSupervisor((event) => events.push(event), deps) + + expect(() => supervisor.start(expectedPeerProcessId)).toThrow( + 'owner process did not report a valid pid' + ) + expect(deps.mkdtempSync).not.toHaveBeenCalled() + expect(deps.spawn).not.toHaveBeenCalled() + expect(events).toEqual([]) + } + ) }) diff --git a/src/main/computer/macos-native-provider-supervisor.ts b/src/main/computer/macos-native-provider-supervisor.ts index 321198b0a62..deae00f42e7 100644 --- a/src/main/computer/macos-native-provider-supervisor.ts +++ b/src/main/computer/macos-native-provider-supervisor.ts @@ -41,6 +41,10 @@ export type MacOSNativeProviderSupervisorDeps = { function ignoreUnownedChildError(): void {} +function isValidPeerProcessId(pid: number): boolean { + return Number.isInteger(pid) && pid > 0 && pid <= 0x7fffffff +} + export class MacOSNativeProviderSupervisor { private readonly sessions = new Map() private readonly deps: MacOSNativeProviderSupervisorDeps @@ -52,7 +56,13 @@ export class MacOSNativeProviderSupervisor { this.deps = deps ?? createDefaultDeps() } - start(): StartedSupervisedMacOSProvider { + start(expectedPeerProcessId: number): StartedSupervisedMacOSProvider { + if (!isValidPeerProcessId(expectedPeerProcessId)) { + throw new RuntimeClientError( + 'accessibility_error', + 'computer provider owner process did not report a valid pid' + ) + } const executablePath = this.deps.resolveExecutablePath() if (!executablePath) { throw new RuntimeClientError('accessibility_error', 'Orca Computer Use.app was not found') @@ -69,7 +79,14 @@ export class MacOSNativeProviderSupervisor { this.deps.writeFileSync(socketTokenPath, socketToken, { encoding: 'utf8', mode: 0o600 }) child = this.deps.spawn( executablePath, - ['--agent', socketPath, '--token-file', socketTokenPath], + [ + '--agent', + socketPath, + '--token-file', + socketTokenPath, + '--peer-pid', + String(expectedPeerProcessId) + ], { detached: true, stdio: 'ignore' } ) child.on('error', ignoreUnownedChildError) diff --git a/src/main/computer/sidecar-client.test.ts b/src/main/computer/sidecar-client.test.ts index 649c13b8923..8c26fe359d4 100644 --- a/src/main/computer/sidecar-client.test.ts +++ b/src/main/computer/sidecar-client.test.ts @@ -10,17 +10,23 @@ import { callComputerSidecarCapabilities, resetComputerSidecarForTest } from './sidecar-client' -import { COMPUTER_SIDECAR_FORCE_KILL_GRACE_MS } from './computer-sidecar-process' +import { COMPUTER_SIDECAR_FORCE_KILL_GRACE_MS } from './computer-sidecar-termination' -const { forkMock, supervisorHandleMock, supervisorSenderState, supervisorShutdownMock } = - vi.hoisted(() => ({ - forkMock: vi.fn(), - supervisorHandleMock: vi.fn(() => false), - supervisorSenderState: { - current: null as null | ((message: Record) => void) - }, - supervisorShutdownMock: vi.fn() - })) +const { + forkMock, + supervisorAttachedPids, + supervisorHandleMock, + supervisorSenderState, + supervisorShutdownMock +} = vi.hoisted(() => ({ + forkMock: vi.fn(), + supervisorAttachedPids: [] as number[], + supervisorHandleMock: vi.fn(() => false), + supervisorSenderState: { + current: null as null | ((message: Record) => void) + }, + supervisorShutdownMock: vi.fn() +})) vi.mock('child_process', () => ({ fork: forkMock @@ -28,8 +34,9 @@ vi.mock('child_process', () => ({ vi.mock('./computer-provider-supervisor-host', () => ({ ComputerProviderSupervisorHost: class { - attach(sender: (message: Record) => void): void { + attach(sender: (message: Record) => void, ownerProcessId: number): void { supervisorSenderState.current = sender + supervisorAttachedPids.push(ownerProcessId) } handle = supervisorHandleMock shutdown(): void { @@ -46,6 +53,10 @@ type SentRequest = { } class FakeChildProcess extends EventEmitter { + constructor(readonly pid: number | undefined) { + super() + } + killed = false killSignals: NodeJS.Signals[] = [] sent: SentRequest[] = [] @@ -83,9 +94,10 @@ describe('computer sidecar client', () => { children.length = 0 deferNextSendCallback = false supervisorSenderState.current = null + supervisorAttachedPids.length = 0 supervisorHandleMock.mockReturnValue(false) forkMock.mockImplementation(() => { - const child = new FakeChildProcess() + const child = new FakeChildProcess(4321 + children.length) child.deferSendCallback = deferNextSendCallback deferNextSendCallback = false children.push(child) @@ -107,6 +119,7 @@ describe('computer sidecar client', () => { 'computer sidecar capabilities timed out' ) const firstChild = children[0]! + expect(supervisorAttachedPids).toEqual([4321]) await vi.advanceTimersByTimeAsync(60_000) await firstRejection @@ -154,6 +167,7 @@ describe('computer sidecar client', () => { const secondCall = callComputerSidecarCapabilities() void secondCall.catch(() => undefined) expect(children).toHaveLength(2) + expect(supervisorAttachedPids).toEqual([4321, 4322]) const secondChild = children[1]! const secondRequest = secondChild.sent[0]! @@ -375,7 +389,7 @@ describe('computer sidecar client', () => { it('fails immediately when the forked sidecar has no IPC send channel', async () => { forkMock.mockImplementationOnce(() => { - const child = new FakeChildProcess() + const child = new FakeChildProcess(4321) child.send = undefined children.push(child) return child @@ -390,6 +404,21 @@ describe('computer sidecar client', () => { expect(children[0]!.listenerCount('error')).toBe(1) }) + it('rejects and reaps a forked sidecar without a valid pid', async () => { + forkMock.mockImplementationOnce(() => { + const child = new FakeChildProcess(undefined) + children.push(child) + return child + }) + + await expect(callComputerSidecarCapabilities()).rejects.toThrow( + 'computer sidecar process did not report a valid pid' + ) + + expect(children[0]!.killed).toBe(true) + expect(supervisorAttachedPids).toEqual([]) + }) + it('force-kills a sidecar that does not exit after the grace period', async () => { const call = callComputerSidecarCapabilities() const rejection = expect(call).rejects.toThrow('computer sidecar shut down')