diff --git a/config/patches/node-pty@1.1.0.patch b/config/patches/node-pty@1.1.0.patch index 34703560ef2..1cbc1052dc5 100644 --- a/config/patches/node-pty@1.1.0.patch +++ b/config/patches/node-pty@1.1.0.patch @@ -430,7 +430,7 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..383df0c9c48355547c65e6c9bbba593d } #endif diff --git a/src/win/conpty.cc b/src/win/conpty.cc -index 7b286d3d644c26141df516929703aa6e129df4b2..a1a63d6380ba5b0a669339435064b9f51e0706dc 100644 +index 7b286d3d644c26141df516929703aa6e129df4b2..e62eadb7b8959a687a546da5d6c1a7b99fde43c5 100644 --- a/src/win/conpty.cc +++ b/src/win/conpty.cc @@ -46,6 +46,11 @@ struct pty_baton { @@ -471,7 +471,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..a1a63d6380ba5b0a669339435064b9f5 envArg, // lpEnvironment mutableCwd.get(), // lpCurrentDirectory &siEx.StartupInfo, // lpStartupInfo -@@ -426,6 +441,31 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { +@@ -426,6 +441,39 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { throw errorWithCode(info, "Cannot create process"); } @@ -495,18 +495,50 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..a1a63d6380ba5b0a669339435064b9f5 + hJob = nullptr; + } + } -+ handle->hJob = hJob; -+ + // Safe to run now: either it is in the job, or we accepted that it is not. -+ ResumeThread(piClient.hThread); ++ if (ResumeThread(piClient.hThread) == static_cast(-1)) { ++ // Why fatal: a shell left suspended produces a pane that never prints and ++ // never exits, which is far harder to diagnose than a failed spawn. ++ if (hJob != nullptr) { ++ CloseHandle(hJob); ++ } ++ TerminateProcess(piClient.hProcess, 1); ++ CloseHandle(piClient.hProcess); ++ CloseHandle(piClient.hThread); ++ throw errorWithCode(info, "Cannot resume process"); ++ } + HANDLE hLibrary = LoadConptyDll(info, useConptyDll); bool fLoadedDll = hLibrary != nullptr; if (useConptyDll && fLoadedDll) -@@ -567,6 +607,72 @@ static Napi::Value PtyKill(const Napi::CallbackInfo& info) { +@@ -440,6 +488,10 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { + + // Update handle + handle->hShell = piClient.hProcess; ++ // Why here and not earlier: LoadConptyDll above can throw, and a baton that ++ // carries a job but never reaches SetupExitCallback has nothing left to ++ // close it. ++ handle->hJob = hJob; + + // Close the thread handle to avoid resource leak + CloseHandle(piClient.hThread); +@@ -567,6 +619,85 @@ static Napi::Value PtyKill(const Napi::CallbackInfo& info) { return env.Undefined(); } ++/** ++ * Orca: confirm a baton really is the pty the caller means. ++ * ++ * The winpty backend mints its own `pty` ids from a separate counter, and the ++ * JS layer stores both in the same field -- so a winpty terminal's id can ++ * collide with a live ConPTY baton here and terminate an unrelated pane's whole ++ * process tree. Matching the shell pid makes the id unforgeable. ++ */ ++static bool ownsShell(const pty_baton* handle, DWORD expectedShellPid) { ++ return handle != nullptr && handle->hJob != nullptr && handle->hShell != nullptr && ++ expectedShellPid != 0 && GetProcessId(handle->hShell) == expectedShellPid; ++} ++ +/** + * Orca: kill this pty's entire tree in one syscall. + * @@ -518,12 +550,12 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..a1a63d6380ba5b0a669339435064b9f5 + Napi::Env env(info.Env()); + Napi::HandleScope scope(env); + -+ if (info.Length() != 1 || !info[0].IsNumber()) { -+ throw Napi::Error::New(env, "Usage: pty.terminateJob(id)"); ++ if (info.Length() != 2 || !info[0].IsNumber() || !info[1].IsNumber()) { ++ throw Napi::Error::New(env, "Usage: pty.terminateJob(id, shellPid)"); + } + + const pty_baton* handle = get_pty_baton(info[0].As().Int32Value()); -+ if (handle == nullptr || handle->hJob == nullptr) { ++ if (!ownsShell(handle, info[1].As().Uint32Value())) { + return Napi::Boolean::New(env, false); + } + return Napi::Boolean::New(env, !!TerminateJobObject(handle->hJob, 1)); @@ -541,12 +573,12 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..a1a63d6380ba5b0a669339435064b9f5 + Napi::Env env(info.Env()); + Napi::HandleScope scope(env); + -+ if (info.Length() != 1 || !info[0].IsNumber()) { -+ throw Napi::Error::New(env, "Usage: pty.listJobProcessIds(id)"); ++ if (info.Length() != 2 || !info[0].IsNumber() || !info[1].IsNumber()) { ++ throw Napi::Error::New(env, "Usage: pty.listJobProcessIds(id, shellPid)"); + } + + const pty_baton* handle = get_pty_baton(info[0].As().Int32Value()); -+ if (handle == nullptr || handle->hJob == nullptr) { ++ if (!ownsShell(handle, info[1].As().Uint32Value())) { + return env.Null(); + } + @@ -576,7 +608,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..a1a63d6380ba5b0a669339435064b9f5 /** * Init */ -@@ -577,6 +683,8 @@ Napi::Object init(Napi::Env env, Napi::Object exports) { +@@ -577,6 +708,8 @@ Napi::Object init(Napi::Env env, Napi::Object exports) { exports.Set("resize", Napi::Function::New(env, PtyResize)); exports.Set("clear", Napi::Function::New(env, PtyClear)); exports.Set("kill", Napi::Function::New(env, PtyKill)); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 34821c1c7f1..d1dd69b4793 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -27,7 +27,7 @@ patchedDependencies: hash: 7333b3837f80a7fbd045964db6d76ba4fc118e49134bdbabb00585b6b7b60673 path: config/patches/lint-staged@16.4.0.patch node-pty@1.1.0: - hash: 65d30811c4f422be2637e01f1ad25c385ddc6b59d5c90309e5656964dda192b0 + hash: 76cf66a7ad9b69ebf9a717023ce41886aeaf1ee944d3fb3787cc06766899f9b6 path: config/patches/node-pty@1.1.0.patch importers: @@ -69,7 +69,7 @@ importers: version: 3.3.1 node-pty: specifier: ^1.1.0 - version: 1.1.0(patch_hash=65d30811c4f422be2637e01f1ad25c385ddc6b59d5c90309e5656964dda192b0) + version: 1.1.0(patch_hash=76cf66a7ad9b69ebf9a717023ce41886aeaf1ee944d3fb3787cc06766899f9b6) posthog-node: specifier: ^5.33.3 version: 5.33.3 @@ -12046,7 +12046,7 @@ snapshots: node-int64@0.4.0: {} - node-pty@1.1.0(patch_hash=65d30811c4f422be2637e01f1ad25c385ddc6b59d5c90309e5656964dda192b0): + node-pty@1.1.0(patch_hash=76cf66a7ad9b69ebf9a717023ce41886aeaf1ee944d3fb3787cc06766899f9b6): dependencies: node-addon-api: 7.1.1 diff --git a/src/main/windows/windows-pty-job.test.ts b/src/main/windows/windows-pty-job.test.ts index 7f982719f9c..7fcfdba455d 100644 --- a/src/main/windows/windows-pty-job.test.ts +++ b/src/main/windows/windows-pty-job.test.ts @@ -7,7 +7,7 @@ import { terminatePtyJob } from './windows-pty-job' -const ptyWithHandle = (id: unknown): IPty => ({ _pty: id }) as unknown as IPty +const ptyWithHandle = (id: unknown, pid = 4242): IPty => ({ _pty: id, pid }) as unknown as IPty afterEach(() => { __setConptyJobNativeForTests() @@ -19,7 +19,7 @@ describe('terminatePtyJob', () => { __setConptyJobNativeForTests(() => ({ terminateJob, listJobProcessIds: vi.fn() })) expect(terminatePtyJob(ptyWithHandle(7))).toBe('terminated') - expect(terminateJob).toHaveBeenCalledWith(7) + expect(terminateJob).toHaveBeenCalledWith(7, 4242) }) it.each([ @@ -47,6 +47,25 @@ describe('terminatePtyJob', () => { expect(terminatePtyJob(ptyWithHandle(id))).toBe('unavailable') }) + it('passes the shell pid so an id from another backend cannot match', () => { + // winpty mints `pty` ids from its own counter and the JS layer stores both + // in the same field, so an id alone can name a live ConPTY pane. The native + // side refuses on a pid mismatch; this pins that we always send the pid. + const terminateJob = vi.fn().mockReturnValue(true) + __setConptyJobNativeForTests(() => ({ terminateJob, listJobProcessIds: vi.fn() })) + + terminatePtyJob(ptyWithHandle(3, 9001)) + expect(terminateJob).toHaveBeenCalledWith(3, 9001) + }) + + it('reports unavailable when the shell pid is unusable', () => { + __setConptyJobNativeForTests(() => ({ + terminateJob: vi.fn().mockReturnValue(true), + listJobProcessIds: vi.fn() + })) + expect(terminatePtyJob(ptyWithHandle(3, 0))).toBe('unavailable') + }) + it('reports unavailable when this build has no job support', () => { // A node-pty rebuilt from unpatched sources exports neither symbol. __setConptyJobNativeForTests(() => null) diff --git a/src/main/windows/windows-pty-job.ts b/src/main/windows/windows-pty-job.ts index bd6211ecfad..5fe556e34f8 100644 --- a/src/main/windows/windows-pty-job.ts +++ b/src/main/windows/windows-pty-job.ts @@ -27,8 +27,8 @@ import { createRequire } from 'node:module' const requireFromMain = createRequire(__filename) type ConptyNative = { - terminateJob: (id: number) => boolean - listJobProcessIds: (id: number) => number[] | null + terminateJob: (id: number, shellPid: number) => boolean + listJobProcessIds: (id: number, shellPid: number) => number[] | null } let cachedNative: ConptyNative | null | undefined @@ -60,14 +60,20 @@ function loadConptyNative(): ConptyNative | null { } /** - * node-pty's per-terminal handle id. + * node-pty's per-terminal handle id, paired with the shell pid that proves it. * - * Not on the public `IPty` surface, so read defensively: a winpty fallback - * terminal and every POSIX terminal have none. + * Neither is on the public `IPty` surface, so read defensively. The pid is not + * belt-and-braces: the winpty backend mints `pty` ids from its own counter and + * the JS layer stores both in the same field, so an id alone can name a + * different, live ConPTY pane. The native side refuses on a pid mismatch. */ -function ptyHandleId(proc: IPty): number | null { +function ptyJobTarget(proc: IPty): { id: number; shellPid: number } | null { const id = (proc as unknown as { _pty?: unknown })._pty - return typeof id === 'number' && Number.isInteger(id) ? id : null + const shellPid = proc.pid + if (!Number.isInteger(id) || !Number.isInteger(shellPid) || (shellPid as number) <= 0) { + return null + } + return { id: id as number, shellPid: shellPid as number } } export type JobTerminationOutcome = 'terminated' | 'unavailable' @@ -81,13 +87,13 @@ export type JobTerminationOutcome = 'terminated' | 'unavailable' * to be misread as "nothing to kill". */ export function terminatePtyJob(proc: IPty): JobTerminationOutcome { - const id = ptyHandleId(proc) + const target = ptyJobTarget(proc) const native = nativeLoader() - if (id === null || !native) { + if (!target || !native) { return 'unavailable' } try { - return native.terminateJob(id) ? 'terminated' : 'unavailable' + return native.terminateJob(target.id, target.shellPid) ? 'terminated' : 'unavailable' } catch { return 'unavailable' } @@ -108,13 +114,13 @@ export function terminatePtyJob(proc: IPty): JobTerminationOutcome { * including children that detached from the console. */ export function listPtyJobProcessIds(proc: IPty): readonly number[] | null { - const id = ptyHandleId(proc) + const target = ptyJobTarget(proc) const native = nativeLoader() - if (id === null || !native) { + if (!target || !native) { return null } try { - return native.listJobProcessIds(id) + return native.listJobProcessIds(target.id, target.shellPid) } catch { return null }