From ff15c3af9ad96960b6d8f3be9c102904fca7bb05 Mon Sep 17 00:00:00 2001 From: anthonykim1 Date: Mon, 19 Oct 2026 20:25:18 -0700 Subject: [PATCH 01/11] Add ConptyReparentPseudoConsole support Add a conptyParentWindow fork option and IPty.setParentWindow(), backed by ConptyReparentPseudoConsole from conpty.dll. The host window becomes the owner of the console window returned by GetConsoleWindow(), so windows that console apps parent to it (for example the WAM sign-in picker) open above the host instead of behind it. --- src/interfaces.ts | 7 ++++ src/native.d.ts | 1 + src/terminal.test.ts | 3 ++ src/terminal.ts | 1 + src/unixTerminal.ts | 4 +++ src/win/conpty.cc | 52 ++++++++++++++++++++++++++++++ src/windowsPtyAgent.ts | 15 +++++++++ src/windowsTerminal.test.ts | 64 ++++++++++++++++++++++++++++++++++++- src/windowsTerminal.ts | 11 +++++++ typings/node-pty.d.ts | 23 +++++++++++++ 10 files changed, 180 insertions(+), 1 deletion(-) diff --git a/src/interfaces.ts b/src/interfaces.ts index 461b713ef..1327e03e3 100644 --- a/src/interfaces.ts +++ b/src/interfaces.ts @@ -40,6 +40,12 @@ export interface ITerminal { */ clear(): void; + /** + * Sets the top-level window (HWND) that hosts this terminal. This is a no-op unless on Windows + * with `useConptyDll`. + */ + setParentWindow(handle: number | bigint): void; + /** * Close, kill and destroy the socket. */ @@ -132,6 +138,7 @@ export interface IWindowsPtyForkOptions extends IBasePtyForkOptions { useConpty?: boolean; useConptyDll?: boolean; conptyInheritCursor?: boolean; + conptyParentWindow?: number | bigint; } export interface IPtyOpenOptions { diff --git a/src/native.d.ts b/src/native.d.ts index 03bfbebae..0a83a43fd 100644 --- a/src/native.d.ts +++ b/src/native.d.ts @@ -7,6 +7,7 @@ interface IConptyNative { connect(ptyId: number, commandLine: string, cwd: string, env: string[], useConptyDll: boolean, onExitCallback: (exitCode: number) => void): { pid: number }; resize(ptyId: number, cols: number, rows: number, useConptyDll: boolean): void; clear(ptyId: number, useConptyDll: boolean): void; + reparent(ptyId: number, hwnd: number | bigint, useConptyDll: boolean): void; kill(ptyId: number, useConptyDll: boolean): void; } diff --git a/src/terminal.test.ts b/src/terminal.test.ts index c9e55177e..5ec843371 100644 --- a/src/terminal.test.ts +++ b/src/terminal.test.ts @@ -32,6 +32,9 @@ class TestTerminal extends Terminal { public clear(): void { throw new Error('Method not implemented.'); } + public setParentWindow(handle: number | bigint): void { + throw new Error('Method not implemented.'); + } public destroy(): void { throw new Error('Method not implemented.'); } diff --git a/src/terminal.ts b/src/terminal.ts index 487e37565..f1a0c4f82 100644 --- a/src/terminal.ts +++ b/src/terminal.ts @@ -180,6 +180,7 @@ export abstract class Terminal implements ITerminal { public abstract resize(cols: number, rows: number, pixelSize?: { width: number, height: number }): void; public abstract clear(): void; + public abstract setParentWindow(handle: number | bigint): void; public abstract destroy(): void; public abstract kill(signal?: string): void; diff --git a/src/unixTerminal.ts b/src/unixTerminal.ts index 2776d501e..3d84feb2f 100644 --- a/src/unixTerminal.ts +++ b/src/unixTerminal.ts @@ -283,6 +283,10 @@ export class UnixTerminal extends Terminal { } + public setParentWindow(handle: number | bigint): void { + // Windows only + } + private _sanitizeEnv(env: IProcessEnv): void { // Make sure we didn't start our server from inside tmux. delete env['TMUX']; diff --git a/src/win/conpty.cc b/src/win/conpty.cc index 931c5362d..7f5d3640e 100644 --- a/src/win/conpty.cc +++ b/src/win/conpty.cc @@ -40,6 +40,9 @@ typedef void (__stdcall *PFNRELEASEPSEUDOCONSOLE)(HPCON hpc); #endif +// Only exported by the bundled conpty.dll; kernel32 has no equivalent. +typedef HRESULT (__stdcall *PFNREPARENTPSEUDOCONSOLE)(HPCON hpc, HWND newParent); + struct pty_baton { int id; HANDLE hIn; @@ -553,6 +556,54 @@ static Napi::Value PtyClear(const Napi::CallbackInfo& info) { return env.Undefined(); } +static Napi::Value PtyReparent(const Napi::CallbackInfo& info) { + Napi::Env env(info.Env()); + Napi::HandleScope scope(env); + + if (info.Length() != 3 || + !info[0].IsNumber() || + !(info[1].IsNumber() || info[1].IsBigInt()) || + !info[2].IsBoolean()) { + throw Napi::Error::New(env, "Usage: pty.reparent(id, hwnd, useConptyDll)"); + } + + int id = info[0].As().Int32Value(); + uint64_t hwndValue; + if (info[1].IsBigInt()) { + bool lossless = false; + hwndValue = info[1].As().Uint64Value(&lossless); + } else { + hwndValue = static_cast(info[1].As().Int64Value()); + } + const bool useConptyDll = info[2].As().Value(); + + // ConptyReparentPseudoConsole only exists in the bundled conpty.dll. + if (!useConptyDll) { + return env.Undefined(); + } + + std::lock_guard lock(g_ptyHandlesMutex); + const pty_baton* handle = get_pty_baton(lock, id); + + if (handle != nullptr) { + HANDLE hLibrary = LoadConptyDll(info, useConptyDll); + bool fLoadedDll = hLibrary != nullptr; + if (fLoadedDll) + { + PFNREPARENTPSEUDOCONSOLE const pfnReparentPseudoConsole = (PFNREPARENTPSEUDOCONSOLE)GetProcAddress( + (HMODULE)hLibrary, "ConptyReparentPseudoConsole"); + if (pfnReparentPseudoConsole) + { + // Makes newParent the owner of the console window returned by + // GetConsoleWindow(), so windows parented to it stack above the host. + pfnReparentPseudoConsole(handle->hpc, reinterpret_cast(static_cast(hwndValue))); + } + } + } + + return env.Undefined(); +} + static Napi::Value PtyKill(const Napi::CallbackInfo& info) { Napi::Env env(info.Env()); Napi::HandleScope scope(env); @@ -609,6 +660,7 @@ Napi::Object init(Napi::Env env, Napi::Object exports) { exports.Set("connect", Napi::Function::New(env, PtyConnect)); exports.Set("resize", Napi::Function::New(env, PtyResize)); exports.Set("clear", Napi::Function::New(env, PtyClear)); + exports.Set("reparent", Napi::Function::New(env, PtyReparent)); exports.Set("kill", Napi::Function::New(env, PtyKill)); return exports; }; diff --git a/src/windowsPtyAgent.ts b/src/windowsPtyAgent.ts index 73f83d6ec..14a682a4c 100644 --- a/src/windowsPtyAgent.ts +++ b/src/windowsPtyAgent.ts @@ -43,6 +43,7 @@ export class WindowsPtyAgent { private _closeTimeout: NodeJS.Timer | undefined; private _connectionTimeout: NodeJS.Timeout | undefined; private _exitCode: number | undefined; + private _isKilled: boolean = false; private _conoutSocketWorker: IConoutConnection; private _onError = new EventEmitter2(); @@ -193,7 +194,21 @@ export class WindowsPtyAgent { this._ptyNative.clear(this._pty, this._useConptyDll); } + /** + * Tells ConPTY which top-level window hosts the terminal (ConptyReparentPseudoConsole). The + * window becomes the owner of the console window returned by GetConsoleWindow(), so dialogs + * that console apps parent to it (e.g. WAM sign-in) stack above the host window. + */ + public setParentWindow(handle: number | bigint): void { + // The pseudoconsole is closed (and its HPCON freed) once the pty is killed + if (this._isKilled || this._exitCode !== undefined) { + return; + } + this._ptyNative.reparent(this._pty, handle, this._useConptyDll); + } + public kill(): void { + this._isKilled = true; // Prevent deferred connection from completing after kill this._clearConnectionTimeout(); this._pendingPtyInfo = undefined; diff --git a/src/windowsTerminal.test.ts b/src/windowsTerminal.test.ts index c76196512..e05db2ddd 100644 --- a/src/windowsTerminal.test.ts +++ b/src/windowsTerminal.test.ts @@ -283,8 +283,67 @@ if (process.platform === 'win32') { }); }); + describe('parent window', () => { + // Prints the console window and GetAncestor(GetConsoleWindow(), GA_ROOTOWNER), which is the + // parent MSAL/WAM uses for sign-in dialogs. + function rootOwnerArgs(delayMs: number, keepAliveSeconds: number = 0): string { + const script = [ + `Start-Sleep -Milliseconds ${delayMs}`, + `Add-Type -Namespace NodePtyTest -Name W -MemberDefinition '[DllImport("kernel32.dll")] public static extern IntPtr GetConsoleWindow(); [DllImport("user32.dll")] public static extern IntPtr GetAncestor(IntPtr h, uint f);'`, + `$c = [NodePtyTest.W]::GetConsoleWindow()`, + `Write-Host ('console=' + $c.ToInt64() + ' rootOwner=' + [NodePtyTest.W]::GetAncestor($c, 3).ToInt64() + ' end')`, + `Start-Sleep -Seconds ${keepAliveSeconds}` + ].join('; '); + return `-NoProfile -NonInteractive -EncodedCommand ${Buffer.from(script, 'utf16le').toString('base64')}`; + } + + function readWindows(term: WindowsTerminal): Promise<{ console: number, rootOwner: number }> { + return new Promise(resolve => { + let output = ''; + term.on('data', (data) => { + output += data; + const match = /console=(\d+) rootOwner=(\d+) end/.exec(output); + if (match) { + resolve({ console: parseInt(match[1], 10), rootOwner: parseInt(match[2], 10) }); + } + }); + }); + } + + it('should make conptyParentWindow the owner of the console window before the shell starts', async function (): Promise { + this.timeout(60000); + // Another pty's hidden top-level console window stands in for the host app's window. + const host = new WindowsTerminal('powershell.exe', rootOwnerArgs(0, 60), { useConptyDll }); + try { + const hostWindows = await readWindows(host); + const term = new WindowsTerminal('powershell.exe', rootOwnerArgs(0), { useConptyDll, conptyParentWindow: hostWindows.console }); + const windows = await readWindows(term); + assert.strictEqual(windows.rootOwner, useConptyDll ? hostWindows.console : windows.console); + term.kill(); + } finally { + host.kill(); + } + }); + + it('should change the owner when setParentWindow is called while the shell runs', async function (): Promise { + this.timeout(60000); + const host = new WindowsTerminal('powershell.exe', rootOwnerArgs(0, 60), { useConptyDll }); + try { + const hostWindows = await readWindows(host); + const term = new WindowsTerminal('powershell.exe', rootOwnerArgs(3000), { useConptyDll }); + // The first data event means the shell is running and conpty has been released. + term.once('data', () => term.setParentWindow(BigInt(hostWindows.console))); + const windows = await readWindows(term); + assert.strictEqual(windows.rootOwner, useConptyDll ? hostWindows.console : windows.console); + term.kill(); + } finally { + host.kill(); + } + }); + }); + describe('Regression for #921', () => { - it('should not crash with concurrent kills while resizing/clearing', function (done) { + it('should not crash with concurrent kills while resizing/clearing/reparenting', function (done) { this.timeout(60000); const N = 30; const terms: WindowsTerminal[] = []; @@ -307,6 +366,9 @@ if (process.platform === 'win32') { try { t.clear(); } catch (e) { /* already exited */ } + try { + t.setParentWindow(0); + } catch (e) { /* already exited */ } } }, 1); for (const t of terms) { diff --git a/src/windowsTerminal.ts b/src/windowsTerminal.ts index 3f88f4dee..9df5b669e 100644 --- a/src/windowsTerminal.ts +++ b/src/windowsTerminal.ts @@ -49,6 +49,11 @@ export class WindowsTerminal extends Terminal { // Create new termal. this._agent = new WindowsPtyAgent(file, args, parsedEnv, cwd, this._cols, this._rows, false, opt.useConptyDll, opt.conptyInheritCursor); + if (opt.conptyParentWindow !== undefined) { + // Applied before the shell is created (connect() waits for the conout worker), so the + // shell's first GetConsoleWindow() already sees its owner. + this._agent.setParentWindow(opt.conptyParentWindow); + } this._socket = this._agent.outSocket; // The socket 'close' handler that drives 'exit' is only attached after // ready_datapipe, which never fires when connect() fails. @@ -163,6 +168,12 @@ export class WindowsTerminal extends Terminal { }); } + public setParentWindow(handle: number | bigint): void { + // Not deferred: the pseudoconsole exists as soon as the agent is created, and the owner + // should be in place before the shell starts asking for it. + this._agent.setParentWindow(handle); + } + public destroy(): void { this._deferNoArgs(() => { this.kill(); diff --git a/typings/node-pty.d.ts b/typings/node-pty.d.ts index a6fc5dd0b..2a051ad61 100644 --- a/typings/node-pty.d.ts +++ b/typings/node-pty.d.ts @@ -109,6 +109,18 @@ declare module 'node-pty' { * @see https://docs.microsoft.com/en-us/windows/console/createpseudoconsole */ conptyInheritCursor?: boolean; + + /** + * (EXPERIMENTAL) + * + * The handle (HWND) of the top-level window that hosts this terminal. ConPTY makes it the owner + * of the console window returned by `GetConsoleWindow()`, so windows that console applications + * parent to it (for example `GetAncestor(GetConsoleWindow(), GA_ROOTOWNER)`, which MSAL/WAM + * sign-in uses) open on top of the host window instead of behind it. The owner is set before + * the process starts. Use {@link IPty.setParentWindow} when the terminal moves to another + * window. Only supported when `useConptyDll` is true; ignored otherwise. + */ + conptyParentWindow?: number | bigint; } /** @@ -173,6 +185,17 @@ declare module 'node-pty' { */ clear(): void; + /** + * (EXPERIMENTAL) + * + * Sets the handle (HWND) of the top-level window that hosts this terminal, see + * `IWindowsPtyForkOptions.conptyParentWindow`. Call it again whenever the terminal moves to a + * different window. This is a no-op unless on Windows with `useConptyDll`. + * @param handle The window handle, e.g. read from Electron's + * `BrowserWindow.getNativeWindowHandle()`. + */ + setParentWindow(handle: number | bigint): void; + /** * Writes data to the pty. * @param data The data to write. From 909b13ee48c186ed58c8d1689eb54689b8ada5c4 Mon Sep 17 00:00:00 2001 From: anthonykim1 Date: Mon, 19 Oct 2026 22:42:22 -0700 Subject: [PATCH 02/11] Address review: native kill guard, return value, parent re-enable - Null the HPCON in PtyKill and skip resize/clear/reparent once it is closed, instead of relying on JS-side bookkeeping. - Return whether ConptyReparentPseudoConsole succeeded, and reject BigInt handles that don't fit in 64 bits. - Apply conptyParentWindow inside WindowsPtyAgent, right after the pseudoconsole is created, and clean up if that throws. - Re-enable the parent window when the shell exits if a modal dialog that died with it left the window disabled. - Make IPty.setParentWindow optional in the public typings and show how to convert Electron's native window handle. --- src/interfaces.ts | 6 ++-- src/native.d.ts | 2 +- src/terminal.test.ts | 2 +- src/terminal.ts | 2 +- src/unixTerminal.ts | 3 +- src/win/conpty.cc | 65 +++++++++++++++++++++++++++++----- src/windowsPtyAgent.test.ts | 3 ++ src/windowsPtyAgent.ts | 24 ++++++++----- src/windowsTerminal.test.ts | 69 ++++++++++++++++++++++++++++++++----- src/windowsTerminal.ts | 11 ++---- typings/node-pty.d.ts | 13 ++++--- 11 files changed, 155 insertions(+), 45 deletions(-) diff --git a/src/interfaces.ts b/src/interfaces.ts index 1327e03e3..1939f8748 100644 --- a/src/interfaces.ts +++ b/src/interfaces.ts @@ -41,10 +41,10 @@ export interface ITerminal { clear(): void; /** - * Sets the top-level window (HWND) that hosts this terminal. This is a no-op unless on Windows - * with `useConptyDll`. + * Sets the top-level window (HWND) that hosts this terminal. Returns whether ConPTY applied it, + * which only happens on Windows with `useConptyDll`. */ - setParentWindow(handle: number | bigint): void; + setParentWindow(handle: number | bigint): boolean; /** * Close, kill and destroy the socket. diff --git a/src/native.d.ts b/src/native.d.ts index 0a83a43fd..fe65e29d9 100644 --- a/src/native.d.ts +++ b/src/native.d.ts @@ -7,7 +7,7 @@ interface IConptyNative { connect(ptyId: number, commandLine: string, cwd: string, env: string[], useConptyDll: boolean, onExitCallback: (exitCode: number) => void): { pid: number }; resize(ptyId: number, cols: number, rows: number, useConptyDll: boolean): void; clear(ptyId: number, useConptyDll: boolean): void; - reparent(ptyId: number, hwnd: number | bigint, useConptyDll: boolean): void; + reparent(ptyId: number, hwnd: number | bigint, useConptyDll: boolean): boolean; kill(ptyId: number, useConptyDll: boolean): void; } diff --git a/src/terminal.test.ts b/src/terminal.test.ts index 5ec843371..fe96bddd8 100644 --- a/src/terminal.test.ts +++ b/src/terminal.test.ts @@ -32,7 +32,7 @@ class TestTerminal extends Terminal { public clear(): void { throw new Error('Method not implemented.'); } - public setParentWindow(handle: number | bigint): void { + public setParentWindow(handle: number | bigint): boolean { throw new Error('Method not implemented.'); } public destroy(): void { diff --git a/src/terminal.ts b/src/terminal.ts index f1a0c4f82..735315aa5 100644 --- a/src/terminal.ts +++ b/src/terminal.ts @@ -180,7 +180,7 @@ export abstract class Terminal implements ITerminal { public abstract resize(cols: number, rows: number, pixelSize?: { width: number, height: number }): void; public abstract clear(): void; - public abstract setParentWindow(handle: number | bigint): void; + public abstract setParentWindow(handle: number | bigint): boolean; public abstract destroy(): void; public abstract kill(signal?: string): void; diff --git a/src/unixTerminal.ts b/src/unixTerminal.ts index 3d84feb2f..72b20700d 100644 --- a/src/unixTerminal.ts +++ b/src/unixTerminal.ts @@ -283,8 +283,9 @@ export class UnixTerminal extends Terminal { } - public setParentWindow(handle: number | bigint): void { + public setParentWindow(handle: number | bigint): boolean { // Windows only + return false; } private _sanitizeEnv(env: IProcessEnv): void { diff --git a/src/win/conpty.cc b/src/win/conpty.cc index 7f5d3640e..5c4938d16 100644 --- a/src/win/conpty.cc +++ b/src/win/conpty.cc @@ -50,6 +50,7 @@ struct pty_baton { HPCON hpc; HANDLE hShell = nullptr; + HWND parentWindow = nullptr; pty_baton(int _id, HANDLE _hIn, HANDLE _hOut, HPCON _hpc) : id(_id), hIn(_hIn), hOut(_hOut), hpc(_hpc) {}; }; @@ -74,6 +75,34 @@ struct ExitEvent { int exit_code = 0; }; +struct OwnedWindowSearch { + HWND owner; + bool found; +}; + +static BOOL CALLBACK FindVisibleOwnedWindow(HWND hwnd, LPARAM lParam) { + auto* search = reinterpret_cast(lParam); + if (GetWindow(hwnd, GW_OWNER) == search->owner && IsWindowVisible(hwnd)) { + search->found = true; + return FALSE; + } + return TRUE; +} + +// A dialog that is modal to the parent window disables it until the dialog closes. If the dialog's +// process exits first, nothing re-enables the parent, so do that once no visible owned window is +// left (an owned window that is still visible may be another live modal dialog). +static void EnableParentWindowIfOrphaned(HWND parentWindow) { + if (!IsWindow(parentWindow) || IsWindowEnabled(parentWindow)) { + return; + } + OwnedWindowSearch search = { parentWindow, false }; + EnumWindows(FindVisibleOwnedWindow, reinterpret_cast(&search)); + if (!search.found) { + EnableWindow(parentWindow, TRUE); + } +} + void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) { std::thread *th = new std::thread; // Don't use Napi::AsyncWorker which is limited by UV_THREADPOOL_SIZE. @@ -96,15 +125,20 @@ void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) { ExitEvent *exit_event = new ExitEvent; // Wait for process to complete. WaitForSingleObject(baton->hShell, INFINITE); + HWND parentWindow = nullptr; { std::lock_guard lock(g_ptyHandlesMutex); GetExitCodeProcess(baton->hShell, (LPDWORD)(&exit_event->exit_code)); CloseHandle(baton->hShell); + parentWindow = baton->parentWindow; const int id = baton->id; std::erase_if(ptyHandles, [id](const auto& ptyHandle) { return ptyHandle->id == id; }); } + if (parentWindow != nullptr) { + EnableParentWindowIfOrphaned(parentWindow); + } auto status = tsfn.BlockingCall(exit_event, callback); // In main thread switch (status) { @@ -498,7 +532,7 @@ static Napi::Value PtyResize(const Napi::CallbackInfo& info) { std::lock_guard lock(g_ptyHandlesMutex); const pty_baton* handle = get_pty_baton(lock, id); - if (handle != nullptr) { + if (handle != nullptr && handle->hpc != nullptr) { HANDLE hLibrary = LoadConptyDll(info, useConptyDll); bool fLoadedDll = hLibrary != nullptr; if (fLoadedDll) @@ -540,7 +574,7 @@ static Napi::Value PtyClear(const Napi::CallbackInfo& info) { std::lock_guard lock(g_ptyHandlesMutex); const pty_baton* handle = get_pty_baton(lock, id); - if (handle != nullptr) { + if (handle != nullptr && handle->hpc != nullptr) { HANDLE hLibrary = LoadConptyDll(info, useConptyDll); bool fLoadedDll = hLibrary != nullptr; if (fLoadedDll) @@ -570,8 +604,16 @@ static Napi::Value PtyReparent(const Napi::CallbackInfo& info) { int id = info[0].As().Int32Value(); uint64_t hwndValue; if (info[1].IsBigInt()) { + // Accept signed and unsigned 64-bit values, but never truncate. + const Napi::BigInt hwndBigInt = info[1].As(); bool lossless = false; - hwndValue = info[1].As().Uint64Value(&lossless); + hwndValue = hwndBigInt.Uint64Value(&lossless); + if (!lossless) { + hwndValue = static_cast(hwndBigInt.Int64Value(&lossless)); + } + if (!lossless) { + throw Napi::Error::New(env, "pty.reparent: hwnd does not fit in 64 bits"); + } } else { hwndValue = static_cast(info[1].As().Int64Value()); } @@ -579,13 +621,14 @@ static Napi::Value PtyReparent(const Napi::CallbackInfo& info) { // ConptyReparentPseudoConsole only exists in the bundled conpty.dll. if (!useConptyDll) { - return env.Undefined(); + return Napi::Boolean::New(env, false); } std::lock_guard lock(g_ptyHandlesMutex); - const pty_baton* handle = get_pty_baton(lock, id); + pty_baton* handle = get_pty_baton(lock, id); - if (handle != nullptr) { + bool applied = false; + if (handle != nullptr && handle->hpc != nullptr) { HANDLE hLibrary = LoadConptyDll(info, useConptyDll); bool fLoadedDll = hLibrary != nullptr; if (fLoadedDll) @@ -596,12 +639,16 @@ static Napi::Value PtyReparent(const Napi::CallbackInfo& info) { { // Makes newParent the owner of the console window returned by // GetConsoleWindow(), so windows parented to it stack above the host. - pfnReparentPseudoConsole(handle->hpc, reinterpret_cast(static_cast(hwndValue))); + const HWND newParent = reinterpret_cast(static_cast(hwndValue)); + applied = SUCCEEDED(pfnReparentPseudoConsole(handle->hpc, newParent)); + if (applied) { + handle->parentWindow = newParent; + } } } } - return env.Undefined(); + return Napi::Boolean::New(env, applied); } static Napi::Value PtyKill(const Napi::CallbackInfo& info) { @@ -631,6 +678,8 @@ static Napi::Value PtyKill(const Napi::CallbackInfo& info) { if (pfnClosePseudoConsole) { pfnClosePseudoConsole(handle->hpc); + // Closing frees the pseudoconsole, but the baton lives until the exit thread erases it. + handle->hpc = nullptr; } } // Defensive: if PtyConnect was never called (or failed before closing the diff --git a/src/windowsPtyAgent.test.ts b/src/windowsPtyAgent.test.ts index 9d6979500..af4056308 100644 --- a/src/windowsPtyAgent.test.ts +++ b/src/windowsPtyAgent.test.ts @@ -145,6 +145,7 @@ if (process.platform === 'win32') { false, false, false, + undefined, createTestAgentOptions(connection, 10) ); @@ -174,6 +175,7 @@ if (process.platform === 'win32') { false, false, false, + undefined, createTestAgentOptions(connection, 1000) ); @@ -200,6 +202,7 @@ if (process.platform === 'win32') { false, false, false, + undefined, createTestAgentOptions(connection, 1000) ); let errorCount = 0; diff --git a/src/windowsPtyAgent.ts b/src/windowsPtyAgent.ts index 14a682a4c..717590cc9 100644 --- a/src/windowsPtyAgent.ts +++ b/src/windowsPtyAgent.ts @@ -43,7 +43,6 @@ export class WindowsPtyAgent { private _closeTimeout: NodeJS.Timer | undefined; private _connectionTimeout: NodeJS.Timeout | undefined; private _exitCode: number | undefined; - private _isKilled: boolean = false; private _conoutSocketWorker: IConoutConnection; private _onError = new EventEmitter2(); @@ -71,6 +70,7 @@ export class WindowsPtyAgent { debug: boolean, private _useConptyDll: boolean = false, conptyInheritCursor: boolean = false, + conptyParentWindow?: number | bigint, options: IWindowsPtyAgentOptions = defaultWindowsPtyAgentOptions ) { if (!conptyNative) { @@ -94,6 +94,16 @@ export class WindowsPtyAgent { // as a terminal id. this._pty = term.pty; + if (conptyParentWindow !== undefined) { + // Before connect() starts the shell, so its first GetConsoleWindow() already has its owner. + try { + this._ptyNative.reparent(this._pty, conptyParentWindow, this._useConptyDll); + } catch (e) { + this._ptyNative.kill(this._pty, this._useConptyDll); + throw e; + } + } + // Create terminal pipe IPC channel and forward to a local unix socket. this._outSocket = new Socket(); this._outSocket.setEncoding('utf8'); @@ -197,18 +207,14 @@ export class WindowsPtyAgent { /** * Tells ConPTY which top-level window hosts the terminal (ConptyReparentPseudoConsole). The * window becomes the owner of the console window returned by GetConsoleWindow(), so dialogs - * that console apps parent to it (e.g. WAM sign-in) stack above the host window. + * that console apps parent to it (e.g. WAM sign-in) stack above the host window. Returns + * whether ConPTY applied the new owner. */ - public setParentWindow(handle: number | bigint): void { - // The pseudoconsole is closed (and its HPCON freed) once the pty is killed - if (this._isKilled || this._exitCode !== undefined) { - return; - } - this._ptyNative.reparent(this._pty, handle, this._useConptyDll); + public setParentWindow(handle: number | bigint): boolean { + return this._ptyNative.reparent(this._pty, handle, this._useConptyDll); } public kill(): void { - this._isKilled = true; // Prevent deferred connection from completing after kill this._clearConnectionTimeout(); this._pendingPtyInfo = undefined; diff --git a/src/windowsTerminal.test.ts b/src/windowsTerminal.test.ts index e05db2ddd..37157753f 100644 --- a/src/windowsTerminal.test.ts +++ b/src/windowsTerminal.test.ts @@ -284,32 +284,41 @@ if (process.platform === 'win32') { }); describe('parent window', () => { + const user32 = `Add-Type -Namespace NodePtyTest -Name W -MemberDefinition '[DllImport("kernel32.dll")] public static extern IntPtr GetConsoleWindow(); [DllImport("user32.dll")] public static extern IntPtr GetAncestor(IntPtr h, uint f); [DllImport("user32.dll")] public static extern bool EnableWindow(IntPtr h, bool e); [DllImport("user32.dll")] public static extern bool IsWindowEnabled(IntPtr h);'`; + + function scriptArgs(...lines: string[]): string { + return `-NoProfile -NonInteractive -EncodedCommand ${Buffer.from([user32, ...lines].join('; '), 'utf16le').toString('base64')}`; + } + // Prints the console window and GetAncestor(GetConsoleWindow(), GA_ROOTOWNER), which is the // parent MSAL/WAM uses for sign-in dialogs. function rootOwnerArgs(delayMs: number, keepAliveSeconds: number = 0): string { - const script = [ + return scriptArgs( `Start-Sleep -Milliseconds ${delayMs}`, - `Add-Type -Namespace NodePtyTest -Name W -MemberDefinition '[DllImport("kernel32.dll")] public static extern IntPtr GetConsoleWindow(); [DllImport("user32.dll")] public static extern IntPtr GetAncestor(IntPtr h, uint f);'`, `$c = [NodePtyTest.W]::GetConsoleWindow()`, `Write-Host ('console=' + $c.ToInt64() + ' rootOwner=' + [NodePtyTest.W]::GetAncestor($c, 3).ToInt64() + ' end')`, `Start-Sleep -Seconds ${keepAliveSeconds}` - ].join('; '); - return `-NoProfile -NonInteractive -EncodedCommand ${Buffer.from(script, 'utf16le').toString('base64')}`; + ); } - function readWindows(term: WindowsTerminal): Promise<{ console: number, rootOwner: number }> { + function readOutput(term: WindowsTerminal, pattern: RegExp): Promise { return new Promise(resolve => { let output = ''; term.on('data', (data) => { output += data; - const match = /console=(\d+) rootOwner=(\d+) end/.exec(output); + const match = pattern.exec(output); if (match) { - resolve({ console: parseInt(match[1], 10), rootOwner: parseInt(match[2], 10) }); + resolve(match); } }); }); } + async function readWindows(term: WindowsTerminal): Promise<{ console: number, rootOwner: number }> { + const match = await readOutput(term, /console=(\d+) rootOwner=(\d+) end/); + return { console: parseInt(match[1], 10), rootOwner: parseInt(match[2], 10) }; + } + it('should make conptyParentWindow the owner of the console window before the shell starts', async function (): Promise { this.timeout(60000); // Another pty's hidden top-level console window stands in for the host app's window. @@ -331,17 +340,59 @@ if (process.platform === 'win32') { try { const hostWindows = await readWindows(host); const term = new WindowsTerminal('powershell.exe', rootOwnerArgs(3000), { useConptyDll }); + let applied: boolean | undefined; // The first data event means the shell is running and conpty has been released. - term.once('data', () => term.setParentWindow(BigInt(hostWindows.console))); + term.once('data', () => applied = term.setParentWindow(BigInt(hostWindows.console))); const windows = await readWindows(term); + assert.strictEqual(applied, useConptyDll); assert.strictEqual(windows.rootOwner, useConptyDll ? hostWindows.console : windows.console); term.kill(); } finally { host.kill(); } }); - }); + it('should not apply setParentWindow once the pty is killed', async function (): Promise { + this.timeout(20000); + const term = new WindowsTerminal('cmd.exe', [], { useConptyDll }); + const exited = new Promise(resolve => term.once('exit', () => resolve())); + await new Promise(resolve => term.once('data', () => resolve())); + term.kill(); + assert.strictEqual(term.setParentWindow(0), false); + await exited; + assert.strictEqual(term.setParentWindow(0), false); + }); + + it('should reject handles that do not fit in 64 bits', function (done: Mocha.Done): void { + const term = new WindowsTerminal('cmd.exe', [], { useConptyDll }); + assert.throws(() => term.setParentWindow(BigInt('18446744073709551616')), /does not fit in 64 bits/); + term.on('exit', () => done()); + term.kill(); + }); + + it('should re-enable the parent window if the process exits while a dialog left it disabled', async function (): Promise { + this.timeout(60000); + const host = new WindowsTerminal('powershell.exe', rootOwnerArgs(0, 60), { useConptyDll }); + try { + const hostWindows = await readWindows(host); + // Disabling the root owner and exiting stands in for a modal dialog whose process dies. + const term = new WindowsTerminal('powershell.exe', scriptArgs( + `$o = [NodePtyTest.W]::GetAncestor([NodePtyTest.W]::GetConsoleWindow(), 3)`, + `[void][NodePtyTest.W]::EnableWindow($o, $false)`, + `Write-Host ('enabled=' + [NodePtyTest.W]::IsWindowEnabled($o) + ' end')` + ), { useConptyDll, conptyParentWindow: hostWindows.console }); + const exited = new Promise(resolve => term.once('exit', () => resolve())); + assert.strictEqual((await readOutput(term, /enabled=(\w+) end/))[1], 'False'); + await exited; + const checker = new WindowsTerminal('powershell.exe', scriptArgs( + `Write-Host ('enabled=' + [NodePtyTest.W]::IsWindowEnabled([IntPtr]${hostWindows.console}) + ' end')` + ), { useConptyDll }); + assert.strictEqual((await readOutput(checker, /enabled=(\w+) end/))[1], 'True'); + } finally { + host.kill(); + } + }); + }); describe('Regression for #921', () => { it('should not crash with concurrent kills while resizing/clearing/reparenting', function (done) { this.timeout(60000); diff --git a/src/windowsTerminal.ts b/src/windowsTerminal.ts index 9df5b669e..9e0148a93 100644 --- a/src/windowsTerminal.ts +++ b/src/windowsTerminal.ts @@ -48,12 +48,7 @@ export class WindowsTerminal extends Terminal { this._deferreds = []; // Create new termal. - this._agent = new WindowsPtyAgent(file, args, parsedEnv, cwd, this._cols, this._rows, false, opt.useConptyDll, opt.conptyInheritCursor); - if (opt.conptyParentWindow !== undefined) { - // Applied before the shell is created (connect() waits for the conout worker), so the - // shell's first GetConsoleWindow() already sees its owner. - this._agent.setParentWindow(opt.conptyParentWindow); - } + this._agent = new WindowsPtyAgent(file, args, parsedEnv, cwd, this._cols, this._rows, false, opt.useConptyDll, opt.conptyInheritCursor, opt.conptyParentWindow); this._socket = this._agent.outSocket; // The socket 'close' handler that drives 'exit' is only attached after // ready_datapipe, which never fires when connect() fails. @@ -168,10 +163,10 @@ export class WindowsTerminal extends Terminal { }); } - public setParentWindow(handle: number | bigint): void { + public setParentWindow(handle: number | bigint): boolean { // Not deferred: the pseudoconsole exists as soon as the agent is created, and the owner // should be in place before the shell starts asking for it. - this._agent.setParentWindow(handle); + return this._agent.setParentWindow(handle); } public destroy(): void { diff --git a/typings/node-pty.d.ts b/typings/node-pty.d.ts index 2a051ad61..4f7b66d55 100644 --- a/typings/node-pty.d.ts +++ b/typings/node-pty.d.ts @@ -119,6 +119,9 @@ declare module 'node-pty' { * sign-in uses) open on top of the host window instead of behind it. The owner is set before * the process starts. Use {@link IPty.setParentWindow} when the terminal moves to another * window. Only supported when `useConptyDll` is true; ignored otherwise. + * + * When the process exits, the window is re-enabled if a modal dialog that died with the + * process left it disabled. */ conptyParentWindow?: number | bigint; } @@ -190,11 +193,13 @@ declare module 'node-pty' { * * Sets the handle (HWND) of the top-level window that hosts this terminal, see * `IWindowsPtyForkOptions.conptyParentWindow`. Call it again whenever the terminal moves to a - * different window. This is a no-op unless on Windows with `useConptyDll`. - * @param handle The window handle, e.g. read from Electron's - * `BrowserWindow.getNativeWindowHandle()`. + * different window. Optional so existing implementations of `IPty` keep compiling. + * @param handle The window handle, for example Electron's + * `BrowserWindow.getNativeWindowHandle().readBigUInt64LE(0)`. + * @returns Whether ConPTY applied the new owner. This is always false unless on Windows with + * `useConptyDll`, and false once the pty has been killed or has exited. */ - setParentWindow(handle: number | bigint): void; + setParentWindow?(handle: number | bigint): boolean; /** * Writes data to the pty. From 0e662792b2a96535bca850a2df5fa25e5323d4a4 Mon Sep 17 00:00:00 2001 From: anthonykim1 Date: Tue, 20 Oct 2026 10:54:59 -0700 Subject: [PATCH 03/11] Don't mark conptyParentWindow and setParentWindow as experimental conpty.dll is no longer experimental, so the APIs added on top of it are documented as regular APIs. --- typings/node-pty.d.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/typings/node-pty.d.ts b/typings/node-pty.d.ts index 4f7b66d55..b66320b62 100644 --- a/typings/node-pty.d.ts +++ b/typings/node-pty.d.ts @@ -111,8 +111,6 @@ declare module 'node-pty' { conptyInheritCursor?: boolean; /** - * (EXPERIMENTAL) - * * The handle (HWND) of the top-level window that hosts this terminal. ConPTY makes it the owner * of the console window returned by `GetConsoleWindow()`, so windows that console applications * parent to it (for example `GetAncestor(GetConsoleWindow(), GA_ROOTOWNER)`, which MSAL/WAM @@ -189,8 +187,6 @@ declare module 'node-pty' { clear(): void; /** - * (EXPERIMENTAL) - * * Sets the handle (HWND) of the top-level window that hosts this terminal, see * `IWindowsPtyForkOptions.conptyParentWindow`. Call it again whenever the terminal moves to a * different window. Optional so existing implementations of `IPty` keep compiling. From f164f25215e4f09c3aa65c39c52e56aab36b3618 Mon Sep 17 00:00:00 2001 From: anthonykim1 Date: Tue, 20 Oct 2026 12:55:48 -0700 Subject: [PATCH 04/11] Address review: make re-enabling the parent window opt-in - Add conptyReenableParentWindowOnExit (defaults to false). Re-enabling the window on exit is a heuristic, so hosts have to opt in. - Look for visible owned windows through the whole owner chain, and compare window handles by their low 32 bits. - Run the recovery off the exit thread, since EnableWindow waits for the window's thread. - Validate handles before creating the pseudoconsole: numbers must be safe integers, and bigints must fit in 64 bits. - Only close the pseudoconsole once in kill(). - setParentWindow returns whether the request was sent; ConPTY applies it asynchronously. - The parent window tests wait for every pty to exit, since exiting while a conout worker still flushes its pipe can hang mocha. --- src/interfaces.ts | 5 +- src/native.d.ts | 2 +- src/win/conpty.cc | 55 +++++++---- src/windowsPtyAgent.test.ts | 3 + src/windowsPtyAgent.ts | 30 +++++- src/windowsTerminal.test.ts | 179 +++++++++++++++++++++++++----------- src/windowsTerminal.ts | 4 +- typings/node-pty.d.ts | 26 ++++-- 8 files changed, 215 insertions(+), 89 deletions(-) diff --git a/src/interfaces.ts b/src/interfaces.ts index 1939f8748..e2fd75d51 100644 --- a/src/interfaces.ts +++ b/src/interfaces.ts @@ -41,8 +41,8 @@ export interface ITerminal { clear(): void; /** - * Sets the top-level window (HWND) that hosts this terminal. Returns whether ConPTY applied it, - * which only happens on Windows with `useConptyDll`. + * Sets the top-level window (HWND) that hosts this terminal. Returns whether the request was sent + * to ConPTY, which only happens on Windows with `useConptyDll`. */ setParentWindow(handle: number | bigint): boolean; @@ -139,6 +139,7 @@ export interface IWindowsPtyForkOptions extends IBasePtyForkOptions { useConptyDll?: boolean; conptyInheritCursor?: boolean; conptyParentWindow?: number | bigint; + conptyReenableParentWindowOnExit?: boolean; } export interface IPtyOpenOptions { diff --git a/src/native.d.ts b/src/native.d.ts index fe65e29d9..bf75a0935 100644 --- a/src/native.d.ts +++ b/src/native.d.ts @@ -7,7 +7,7 @@ interface IConptyNative { connect(ptyId: number, commandLine: string, cwd: string, env: string[], useConptyDll: boolean, onExitCallback: (exitCode: number) => void): { pid: number }; resize(ptyId: number, cols: number, rows: number, useConptyDll: boolean): void; clear(ptyId: number, useConptyDll: boolean): void; - reparent(ptyId: number, hwnd: number | bigint, useConptyDll: boolean): boolean; + reparent(ptyId: number, hwnd: number | bigint, useConptyDll: boolean, reenableOnExit: boolean): boolean; kill(ptyId: number, useConptyDll: boolean): void; } diff --git a/src/win/conpty.cc b/src/win/conpty.cc index 5c4938d16..0a69ac18b 100644 --- a/src/win/conpty.cc +++ b/src/win/conpty.cc @@ -50,7 +50,8 @@ struct pty_baton { HPCON hpc; HANDLE hShell = nullptr; - HWND parentWindow = nullptr; + // Only set when the host opted in to re-enabling its window when the shell exits. + HWND parentWindowToReenable = nullptr; pty_baton(int _id, HANDLE _hIn, HANDLE _hOut, HPCON _hpc) : id(_id), hIn(_hIn), hOut(_hOut), hpc(_hpc) {}; }; @@ -75,6 +76,11 @@ struct ExitEvent { int exit_code = 0; }; +// Only the low 32 bits of a window handle are significant. +static bool IsSameWindow(HWND a, HWND b) { + return static_cast(reinterpret_cast(a)) == static_cast(reinterpret_cast(b)); +} + struct OwnedWindowSearch { HWND owner; bool found; @@ -82,16 +88,24 @@ struct OwnedWindowSearch { static BOOL CALLBACK FindVisibleOwnedWindow(HWND hwnd, LPARAM lParam) { auto* search = reinterpret_cast(lParam); - if (GetWindow(hwnd, GW_OWNER) == search->owner && IsWindowVisible(hwnd)) { - search->found = true; - return FALSE; + if (!IsWindowVisible(hwnd)) { + return TRUE; + } + // Walk the whole owner chain, since a dialog can be owned through a hidden proxy window. + HWND owner = GetWindow(hwnd, GW_OWNER); + for (int depth = 0; owner != nullptr && depth < 64; depth++) { + if (IsSameWindow(owner, search->owner)) { + search->found = true; + return FALSE; + } + owner = GetWindow(owner, GW_OWNER); } return TRUE; } // A dialog that is modal to the parent window disables it until the dialog closes. If the dialog's -// process exits first, nothing re-enables the parent, so do that once no visible owned window is -// left (an owned window that is still visible may be another live modal dialog). +// process exits first, nothing re-enables the parent. This is a heuristic: it re-enables the parent +// when it is disabled and no visible window is owned by it, so hosts must opt in. static void EnableParentWindowIfOrphaned(HWND parentWindow) { if (!IsWindow(parentWindow) || IsWindowEnabled(parentWindow)) { return; @@ -130,14 +144,15 @@ void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) { std::lock_guard lock(g_ptyHandlesMutex); GetExitCodeProcess(baton->hShell, (LPDWORD)(&exit_event->exit_code)); CloseHandle(baton->hShell); - parentWindow = baton->parentWindow; + parentWindow = baton->parentWindowToReenable; const int id = baton->id; std::erase_if(ptyHandles, [id](const auto& ptyHandle) { return ptyHandle->id == id; }); } if (parentWindow != nullptr) { - EnableParentWindowIfOrphaned(parentWindow); + // EnableWindow waits for the parent window's thread, so don't hold up the exit event. + std::thread([parentWindow] { EnableParentWindowIfOrphaned(parentWindow); }).detach(); } auto status = tsfn.BlockingCall(exit_event, callback); // In main thread @@ -594,11 +609,12 @@ static Napi::Value PtyReparent(const Napi::CallbackInfo& info) { Napi::Env env(info.Env()); Napi::HandleScope scope(env); - if (info.Length() != 3 || + if (info.Length() != 4 || !info[0].IsNumber() || !(info[1].IsNumber() || info[1].IsBigInt()) || - !info[2].IsBoolean()) { - throw Napi::Error::New(env, "Usage: pty.reparent(id, hwnd, useConptyDll)"); + !info[2].IsBoolean() || + !info[3].IsBoolean()) { + throw Napi::Error::New(env, "Usage: pty.reparent(id, hwnd, useConptyDll, reenableOnExit)"); } int id = info[0].As().Int32Value(); @@ -618,6 +634,7 @@ static Napi::Value PtyReparent(const Napi::CallbackInfo& info) { hwndValue = static_cast(info[1].As().Int64Value()); } const bool useConptyDll = info[2].As().Value(); + const bool reenableOnExit = info[3].As().Value(); // ConptyReparentPseudoConsole only exists in the bundled conpty.dll. if (!useConptyDll) { @@ -627,7 +644,7 @@ static Napi::Value PtyReparent(const Napi::CallbackInfo& info) { std::lock_guard lock(g_ptyHandlesMutex); pty_baton* handle = get_pty_baton(lock, id); - bool applied = false; + bool sent = false; if (handle != nullptr && handle->hpc != nullptr) { HANDLE hLibrary = LoadConptyDll(info, useConptyDll); bool fLoadedDll = hLibrary != nullptr; @@ -637,18 +654,18 @@ static Napi::Value PtyReparent(const Napi::CallbackInfo& info) { (HMODULE)hLibrary, "ConptyReparentPseudoConsole"); if (pfnReparentPseudoConsole) { - // Makes newParent the owner of the console window returned by - // GetConsoleWindow(), so windows parented to it stack above the host. + // Asks ConPTY to make newParent the owner of the console window returned by + // GetConsoleWindow(); ConPTY applies it asynchronously. const HWND newParent = reinterpret_cast(static_cast(hwndValue)); - applied = SUCCEEDED(pfnReparentPseudoConsole(handle->hpc, newParent)); - if (applied) { - handle->parentWindow = newParent; + sent = SUCCEEDED(pfnReparentPseudoConsole(handle->hpc, newParent)); + if (sent) { + handle->parentWindowToReenable = reenableOnExit ? newParent : nullptr; } } } } - return Napi::Boolean::New(env, applied); + return Napi::Boolean::New(env, sent); } static Napi::Value PtyKill(const Napi::CallbackInfo& info) { @@ -675,7 +692,7 @@ static Napi::Value PtyKill(const Napi::CallbackInfo& info) { PFNCLOSEPSEUDOCONSOLE const pfnClosePseudoConsole = (PFNCLOSEPSEUDOCONSOLE)GetProcAddress( (HMODULE)hLibrary, useConptyDll ? "ConptyClosePseudoConsole" : "ClosePseudoConsole"); - if (pfnClosePseudoConsole) + if (pfnClosePseudoConsole && handle->hpc != nullptr) { pfnClosePseudoConsole(handle->hpc); // Closing frees the pseudoconsole, but the baton lives until the exit thread erases it. diff --git a/src/windowsPtyAgent.test.ts b/src/windowsPtyAgent.test.ts index af4056308..8b4c687b5 100644 --- a/src/windowsPtyAgent.test.ts +++ b/src/windowsPtyAgent.test.ts @@ -146,6 +146,7 @@ if (process.platform === 'win32') { false, false, undefined, + false, createTestAgentOptions(connection, 10) ); @@ -176,6 +177,7 @@ if (process.platform === 'win32') { false, false, undefined, + false, createTestAgentOptions(connection, 1000) ); @@ -203,6 +205,7 @@ if (process.platform === 'win32') { false, false, undefined, + false, createTestAgentOptions(connection, 1000) ); let errorCount = 0; diff --git a/src/windowsPtyAgent.ts b/src/windowsPtyAgent.ts index 717590cc9..d88d07741 100644 --- a/src/windowsPtyAgent.ts +++ b/src/windowsPtyAgent.ts @@ -71,8 +71,14 @@ export class WindowsPtyAgent { private _useConptyDll: boolean = false, conptyInheritCursor: boolean = false, conptyParentWindow?: number | bigint, + private _reenableParentWindowOnExit: boolean = false, options: IWindowsPtyAgentOptions = defaultWindowsPtyAgentOptions ) { + // Validate before allocating the pseudoconsole, so bad input leaves nothing behind. + if (conptyParentWindow !== undefined) { + validateWindowHandle(conptyParentWindow); + } + if (!conptyNative) { conptyNative = loadNativeModule('conpty').module; } @@ -95,9 +101,9 @@ export class WindowsPtyAgent { this._pty = term.pty; if (conptyParentWindow !== undefined) { - // Before connect() starts the shell, so its first GetConsoleWindow() already has its owner. + // Requested before connect() launches the shell, like Windows Terminal does. try { - this._ptyNative.reparent(this._pty, conptyParentWindow, this._useConptyDll); + this._ptyNative.reparent(this._pty, conptyParentWindow, this._useConptyDll, this._reenableParentWindowOnExit); } catch (e) { this._ptyNative.kill(this._pty, this._useConptyDll); throw e; @@ -208,10 +214,11 @@ export class WindowsPtyAgent { * Tells ConPTY which top-level window hosts the terminal (ConptyReparentPseudoConsole). The * window becomes the owner of the console window returned by GetConsoleWindow(), so dialogs * that console apps parent to it (e.g. WAM sign-in) stack above the host window. Returns - * whether ConPTY applied the new owner. + * whether the request was sent to ConPTY, which applies it asynchronously. */ public setParentWindow(handle: number | bigint): boolean { - return this._ptyNative.reparent(this._pty, handle, this._useConptyDll); + validateWindowHandle(handle); + return this._ptyNative.reparent(this._pty, handle, this._useConptyDll, this._reenableParentWindowOnExit); } public kill(): void { @@ -371,3 +378,18 @@ function repeatText(text: string, count: number): string { function xOr(arg1: boolean, arg2: boolean): boolean { return ((arg1 && !arg2) || (!arg1 && arg2)); } + +/** + * Throws unless the handle is a safe integer or a bigint that fits in 64 bits. Signed and unsigned + * bigints are both accepted, since a window handle may be sign-extended. + */ +function validateWindowHandle(handle: number | bigint): void { + if (typeof handle === 'bigint') { + if (handle < BigInt('-9223372036854775808') || handle > BigInt('18446744073709551615')) { + throw new Error('Window handle does not fit in 64 bits'); + } + } else if (!Number.isSafeInteger(handle)) { + // Larger numbers can't hold an exact handle. + throw new Error('Window handle must be a safe integer or a bigint'); + } +} diff --git a/src/windowsTerminal.test.ts b/src/windowsTerminal.test.ts index 37157753f..5be964753 100644 --- a/src/windowsTerminal.test.ts +++ b/src/windowsTerminal.test.ts @@ -6,6 +6,7 @@ import * as fs from 'fs'; import * as assert from 'assert'; import { WindowsTerminal } from './windowsTerminal'; +import { IWindowsPtyForkOptions } from './interfaces'; import * as path from 'path'; import * as psList from 'ps-list'; @@ -284,7 +285,7 @@ if (process.platform === 'win32') { }); describe('parent window', () => { - const user32 = `Add-Type -Namespace NodePtyTest -Name W -MemberDefinition '[DllImport("kernel32.dll")] public static extern IntPtr GetConsoleWindow(); [DllImport("user32.dll")] public static extern IntPtr GetAncestor(IntPtr h, uint f); [DllImport("user32.dll")] public static extern bool EnableWindow(IntPtr h, bool e); [DllImport("user32.dll")] public static extern bool IsWindowEnabled(IntPtr h);'`; + const user32 = `Add-Type -Namespace NodePtyTest -Name W -MemberDefinition '[DllImport("kernel32.dll")] public static extern IntPtr GetConsoleWindow(); [DllImport("user32.dll")] public static extern IntPtr GetAncestor(IntPtr h, uint f); [DllImport("user32.dll")] public static extern bool EnableWindow(IntPtr h, bool e); [DllImport("user32.dll")] public static extern bool IsWindowEnabled(IntPtr h); [DllImport("user32.dll", CharSet = CharSet.Unicode)] public static extern IntPtr CreateWindowExW(int ex, string cls, string title, int style, int x, int y, int w, int h, IntPtr owner, IntPtr menu, IntPtr inst, IntPtr param);'`; function scriptArgs(...lines: string[]): string { return `-NoProfile -NonInteractive -EncodedCommand ${Buffer.from([user32, ...lines].join('; '), 'utf16le').toString('base64')}`; @@ -301,6 +302,44 @@ if (process.platform === 'win32') { ); } + // Disabling the root owner and exiting stands in for a modal dialog whose process dies. + function disableRootOwnerArgs(): string { + return scriptArgs( + `$o = [NodePtyTest.W]::GetAncestor([NodePtyTest.W]::GetConsoleWindow(), 3)`, + `[void][NodePtyTest.W]::EnableWindow($o, $false)`, + `Write-Host ('enabled=' + [NodePtyTest.W]::IsWindowEnabled($o) + ' end')` + ); + } + + const started: Array<{ exited: Promise, kill: () => void }> = []; + + function startPty(file: string, args: string | string[], options: IWindowsPtyForkOptions = {}): WindowsTerminal { + const term = new WindowsTerminal(file, args, { useConptyDll, ...options }); + let hasExited = false; + const exited = new Promise(resolve => term.once('exit', () => { + hasExited = true; + resolve(); + })); + started.push({ + exited, + kill: () => { + if (!hasExited) { + term.kill(); + } + } + }); + return term; + } + + // Wait for every pty to exit, even after a failed test: exiting while a conout worker still + // flushes its pipe can hang mocha. + afterEach(async function (): Promise { + this.timeout(30000); + const ptys = started.splice(0); + ptys.forEach(pty => pty.kill()); + await Promise.all(ptys.map(pty => pty.exited)); + }); + function readOutput(term: WindowsTerminal, pattern: RegExp): Promise { return new Promise(resolve => { let output = ''; @@ -315,46 +354,54 @@ if (process.platform === 'win32') { } async function readWindows(term: WindowsTerminal): Promise<{ console: number, rootOwner: number }> { - const match = await readOutput(term, /console=(\d+) rootOwner=(\d+) end/); + const match = await readOutput(term, /console=(-?\d+) rootOwner=(-?\d+) end/); return { console: parseInt(match[1], 10), rootOwner: parseInt(match[2], 10) }; } + // Another pty's hidden top-level console window stands in for the host app's window. + async function startHostWindow(): Promise { + return (await readWindows(startPty('powershell.exe', rootOwnerArgs(0, 60)))).console; + } + + // Re-enabling happens off the exit path, so wait up to waitMs for the window to be enabled. + async function readEnabled(hwnd: number, waitMs: number): Promise { + const checker = startPty('powershell.exe', scriptArgs( + `$h = [IntPtr]${hwnd}`, + `for ($i = 0; $i -lt ${waitMs / 100} -and -not [NodePtyTest.W]::IsWindowEnabled($h); $i++) { Start-Sleep -Milliseconds 100 }`, + `Write-Host ('enabled=' + [NodePtyTest.W]::IsWindowEnabled($h) + ' end')` + )); + return (await readOutput(checker, /enabled=(\w+) end/))[1]; + } + + async function disableParentAndExit(parent: number, options: IWindowsPtyForkOptions): Promise { + const term = startPty('powershell.exe', disableRootOwnerArgs(), { conptyParentWindow: parent, ...options }); + const exited = new Promise(resolve => term.once('exit', () => resolve())); + assert.strictEqual((await readOutput(term, /enabled=(\w+) end/))[1], 'False'); + await exited; + } + it('should make conptyParentWindow the owner of the console window before the shell starts', async function (): Promise { this.timeout(60000); - // Another pty's hidden top-level console window stands in for the host app's window. - const host = new WindowsTerminal('powershell.exe', rootOwnerArgs(0, 60), { useConptyDll }); - try { - const hostWindows = await readWindows(host); - const term = new WindowsTerminal('powershell.exe', rootOwnerArgs(0), { useConptyDll, conptyParentWindow: hostWindows.console }); - const windows = await readWindows(term); - assert.strictEqual(windows.rootOwner, useConptyDll ? hostWindows.console : windows.console); - term.kill(); - } finally { - host.kill(); - } + const hostWindow = await startHostWindow(); + const windows = await readWindows(startPty('powershell.exe', rootOwnerArgs(0), { conptyParentWindow: hostWindow })); + assert.strictEqual(windows.rootOwner, useConptyDll ? hostWindow : windows.console); }); it('should change the owner when setParentWindow is called while the shell runs', async function (): Promise { this.timeout(60000); - const host = new WindowsTerminal('powershell.exe', rootOwnerArgs(0, 60), { useConptyDll }); - try { - const hostWindows = await readWindows(host); - const term = new WindowsTerminal('powershell.exe', rootOwnerArgs(3000), { useConptyDll }); - let applied: boolean | undefined; - // The first data event means the shell is running and conpty has been released. - term.once('data', () => applied = term.setParentWindow(BigInt(hostWindows.console))); - const windows = await readWindows(term); - assert.strictEqual(applied, useConptyDll); - assert.strictEqual(windows.rootOwner, useConptyDll ? hostWindows.console : windows.console); - term.kill(); - } finally { - host.kill(); - } + const hostWindow = await startHostWindow(); + const term = startPty('powershell.exe', rootOwnerArgs(3000)); + let sent: boolean | undefined; + // The first data event means the shell is running and conpty has been released. + term.once('data', () => sent = term.setParentWindow(BigInt(hostWindow))); + const windows = await readWindows(term); + assert.strictEqual(sent, useConptyDll); + assert.strictEqual(windows.rootOwner, useConptyDll ? hostWindow : windows.console); }); - it('should not apply setParentWindow once the pty is killed', async function (): Promise { + it('should not send setParentWindow once the pseudoconsole is closed', async function (): Promise { this.timeout(20000); - const term = new WindowsTerminal('cmd.exe', [], { useConptyDll }); + const term = startPty('cmd.exe', []); const exited = new Promise(resolve => term.once('exit', () => resolve())); await new Promise(resolve => term.once('data', () => resolve())); term.kill(); @@ -363,34 +410,60 @@ if (process.platform === 'win32') { assert.strictEqual(term.setParentWindow(0), false); }); - it('should reject handles that do not fit in 64 bits', function (done: Mocha.Done): void { - const term = new WindowsTerminal('cmd.exe', [], { useConptyDll }); - assert.throws(() => term.setParentWindow(BigInt('18446744073709551616')), /does not fit in 64 bits/); - term.on('exit', () => done()); - term.kill(); + it('should reject handles that are not safe integers or bigints that fit in 64 bits', () => { + const invalid: Array<[number | bigint, RegExp]> = [ + [BigInt('18446744073709551616'), /does not fit in 64 bits/], + [BigInt('-9223372036854775809'), /does not fit in 64 bits/], + [1.5, /must be a safe integer/], + [Number.MAX_SAFE_INTEGER + 1, /must be a safe integer/] + ]; + // Validated before the pseudoconsole is created, so nothing is left behind + for (const [handle, error] of invalid) { + assert.throws(() => startPty('cmd.exe', [], { conptyParentWindow: handle }), error); + } + const term = startPty('cmd.exe', []); + for (const [handle, error] of invalid) { + assert.throws(() => term.setParentWindow(handle), error); + } }); - it('should re-enable the parent window if the process exits while a dialog left it disabled', async function (): Promise { + it('should re-enable the parent window on exit when asked, if a dialog left it disabled', async function (): Promise { + if (!useConptyDll) { + this.skip(); + } this.timeout(60000); - const host = new WindowsTerminal('powershell.exe', rootOwnerArgs(0, 60), { useConptyDll }); - try { - const hostWindows = await readWindows(host); - // Disabling the root owner and exiting stands in for a modal dialog whose process dies. - const term = new WindowsTerminal('powershell.exe', scriptArgs( - `$o = [NodePtyTest.W]::GetAncestor([NodePtyTest.W]::GetConsoleWindow(), 3)`, - `[void][NodePtyTest.W]::EnableWindow($o, $false)`, - `Write-Host ('enabled=' + [NodePtyTest.W]::IsWindowEnabled($o) + ' end')` - ), { useConptyDll, conptyParentWindow: hostWindows.console }); - const exited = new Promise(resolve => term.once('exit', () => resolve())); - assert.strictEqual((await readOutput(term, /enabled=(\w+) end/))[1], 'False'); - await exited; - const checker = new WindowsTerminal('powershell.exe', scriptArgs( - `Write-Host ('enabled=' + [NodePtyTest.W]::IsWindowEnabled([IntPtr]${hostWindows.console}) + ' end')` - ), { useConptyDll }); - assert.strictEqual((await readOutput(checker, /enabled=(\w+) end/))[1], 'True'); - } finally { - host.kill(); + const hostWindow = await startHostWindow(); + await disableParentAndExit(hostWindow, { conptyReenableParentWindowOnExit: true }); + assert.strictEqual(await readEnabled(hostWindow, 5000), 'True'); + }); + + it('should leave the parent window disabled on exit unless asked to re-enable it', async function (): Promise { + if (!useConptyDll) { + this.skip(); } + this.timeout(60000); + const hostWindow = await startHostWindow(); + await disableParentAndExit(hostWindow, {}); + assert.strictEqual(await readEnabled(hostWindow, 3000), 'False'); + }); + + it('should not re-enable the parent window while it still owns a visible window', async function (): Promise { + if (!useConptyDll) { + this.skip(); + } + this.timeout(60000); + const hostWindow = await startHostWindow(); + // A visible off-screen window owned through a hidden proxy window, like a modal dialog + // shown by another process. + const dialog = startPty('powershell.exe', scriptArgs( + `$proxy = [NodePtyTest.W]::CreateWindowExW(0x80, 'STATIC', 'proxy', 0x80000000, 0, 0, 1, 1, [IntPtr]${hostWindow}, [IntPtr]::Zero, [IntPtr]::Zero, [IntPtr]::Zero)`, + `$dialog = [NodePtyTest.W]::CreateWindowExW(0x08000080, 'STATIC', 'dialog', 0x90000000, -32000, -32000, 1, 1, $proxy, [IntPtr]::Zero, [IntPtr]::Zero, [IntPtr]::Zero)`, + `Write-Host ('dialog=' + $dialog.ToInt64() + ' end')`, + `Start-Sleep -Seconds 60` + )); + assert.notStrictEqual((await readOutput(dialog, /dialog=(-?\d+) end/))[1], '0'); + await disableParentAndExit(hostWindow, { conptyReenableParentWindowOnExit: true }); + assert.strictEqual(await readEnabled(hostWindow, 3000), 'False'); }); }); describe('Regression for #921', () => { diff --git a/src/windowsTerminal.ts b/src/windowsTerminal.ts index 9e0148a93..eaeb8f1b2 100644 --- a/src/windowsTerminal.ts +++ b/src/windowsTerminal.ts @@ -48,7 +48,7 @@ export class WindowsTerminal extends Terminal { this._deferreds = []; // Create new termal. - this._agent = new WindowsPtyAgent(file, args, parsedEnv, cwd, this._cols, this._rows, false, opt.useConptyDll, opt.conptyInheritCursor, opt.conptyParentWindow); + this._agent = new WindowsPtyAgent(file, args, parsedEnv, cwd, this._cols, this._rows, false, opt.useConptyDll, opt.conptyInheritCursor, opt.conptyParentWindow, opt.conptyReenableParentWindowOnExit); this._socket = this._agent.outSocket; // The socket 'close' handler that drives 'exit' is only attached after // ready_datapipe, which never fires when connect() fails. @@ -165,7 +165,7 @@ export class WindowsTerminal extends Terminal { public setParentWindow(handle: number | bigint): boolean { // Not deferred: the pseudoconsole exists as soon as the agent is created, and the owner - // should be in place before the shell starts asking for it. + // should be requested before the shell starts asking for it. return this._agent.setParentWindow(handle); } diff --git a/typings/node-pty.d.ts b/typings/node-pty.d.ts index b66320b62..01d75d9db 100644 --- a/typings/node-pty.d.ts +++ b/typings/node-pty.d.ts @@ -114,14 +114,23 @@ declare module 'node-pty' { * The handle (HWND) of the top-level window that hosts this terminal. ConPTY makes it the owner * of the console window returned by `GetConsoleWindow()`, so windows that console applications * parent to it (for example `GetAncestor(GetConsoleWindow(), GA_ROOTOWNER)`, which MSAL/WAM - * sign-in uses) open on top of the host window instead of behind it. The owner is set before - * the process starts. Use {@link IPty.setParentWindow} when the terminal moves to another - * window. Only supported when `useConptyDll` is true; ignored otherwise. - * - * When the process exits, the window is re-enabled if a modal dialog that died with the - * process left it disabled. + * sign-in uses) open on top of the host window instead of behind it. The owner is requested + * before the process is launched. Use {@link IPty.setParentWindow} when the terminal moves to + * another window. Must be a safe integer, or a bigint that fits in 64 bits. Only supported when + * `useConptyDll` is true; ignored otherwise. */ conptyParentWindow?: number | bigint; + + /** + * Whether to re-enable the parent window when the process exits, if the window is still + * disabled and no visible window is owned by it, directly or through other owned windows. The + * parent window is the last one set with `conptyParentWindow` or {@link IPty.setParentWindow}. + * This recovers from a modal dialog that died with its process, which otherwise leaves the + * parent window disabled for good. It's a heuristic that can't tell why the window is disabled, + * so only set it if the host never disables its window without showing a window it owns. Only + * supported when `useConptyDll` is true. Defaults to false. + */ + conptyReenableParentWindowOnExit?: boolean; } /** @@ -192,8 +201,9 @@ declare module 'node-pty' { * different window. Optional so existing implementations of `IPty` keep compiling. * @param handle The window handle, for example Electron's * `BrowserWindow.getNativeWindowHandle().readBigUInt64LE(0)`. - * @returns Whether ConPTY applied the new owner. This is always false unless on Windows with - * `useConptyDll`, and false once the pty has been killed or has exited. + * @returns Whether the request was sent to ConPTY, which applies it asynchronously. This is + * always false unless on Windows with `useConptyDll`, and false once the pseudoconsole is + * closed. */ setParentWindow?(handle: number | bigint): boolean; From 327e24c997a6039dabe49abef73dc23d867ae1f4 Mon Sep 17 00:00:00 2001 From: anthonykim1 Date: Tue, 20 Oct 2026 13:43:40 -0700 Subject: [PATCH 05/11] Don't call the parent window re-enable a heuristic It's an exact check: the window is still disabled and no visible window is owned by it. What it can't tell is why the window is disabled, which is why hosts opt in. --- src/win/conpty.cc | 4 ++-- typings/node-pty.d.ts | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/win/conpty.cc b/src/win/conpty.cc index 0a69ac18b..1439dc278 100644 --- a/src/win/conpty.cc +++ b/src/win/conpty.cc @@ -104,8 +104,8 @@ static BOOL CALLBACK FindVisibleOwnedWindow(HWND hwnd, LPARAM lParam) { } // A dialog that is modal to the parent window disables it until the dialog closes. If the dialog's -// process exits first, nothing re-enables the parent. This is a heuristic: it re-enables the parent -// when it is disabled and no visible window is owned by it, so hosts must opt in. +// process exits first, nothing re-enables the parent, so re-enable it when it is still disabled and +// no visible window is owned by it. This can't tell why the window is disabled, so hosts opt in. static void EnableParentWindowIfOrphaned(HWND parentWindow) { if (!IsWindow(parentWindow) || IsWindowEnabled(parentWindow)) { return; diff --git a/typings/node-pty.d.ts b/typings/node-pty.d.ts index 01d75d9db..cafa70bcf 100644 --- a/typings/node-pty.d.ts +++ b/typings/node-pty.d.ts @@ -126,9 +126,9 @@ declare module 'node-pty' { * disabled and no visible window is owned by it, directly or through other owned windows. The * parent window is the last one set with `conptyParentWindow` or {@link IPty.setParentWindow}. * This recovers from a modal dialog that died with its process, which otherwise leaves the - * parent window disabled for good. It's a heuristic that can't tell why the window is disabled, - * so only set it if the host never disables its window without showing a window it owns. Only - * supported when `useConptyDll` is true. Defaults to false. + * parent window disabled for good. The check can't tell why the window is disabled, so only set + * it if the host never disables its window without showing a window it owns. Only supported + * when `useConptyDll` is true. Defaults to false. */ conptyReenableParentWindowOnExit?: boolean; } From 95ed9a49a64e106770995760d902ccb9aecc8823 Mon Sep 17 00:00:00 2001 From: anthonykim1 Date: Tue, 20 Oct 2026 14:36:15 -0700 Subject: [PATCH 06/11] Keep setParentWindow as the only new API Remove conptyParentWindow. setParentWindow() isn't deferred, and the shell is only launched once the conout worker is ready, so calling it right after spawn() sends the request before the shell starts. Remove conptyReenableParentWindowOnExit and re-enabling the parent window on exit. That moves to a follow-up issue, so this PR stays focused on the parenting change. --- src/interfaces.ts | 2 - src/native.d.ts | 2 +- src/win/conpty.cc | 62 ++-------------------------- src/windowsPtyAgent.test.ts | 6 --- src/windowsPtyAgent.ts | 19 +-------- src/windowsTerminal.test.ts | 82 ++++--------------------------------- src/windowsTerminal.ts | 2 +- typings/node-pty.d.ts | 36 ++++------------ 8 files changed, 24 insertions(+), 187 deletions(-) diff --git a/src/interfaces.ts b/src/interfaces.ts index e2fd75d51..d69b947bb 100644 --- a/src/interfaces.ts +++ b/src/interfaces.ts @@ -138,8 +138,6 @@ export interface IWindowsPtyForkOptions extends IBasePtyForkOptions { useConpty?: boolean; useConptyDll?: boolean; conptyInheritCursor?: boolean; - conptyParentWindow?: number | bigint; - conptyReenableParentWindowOnExit?: boolean; } export interface IPtyOpenOptions { diff --git a/src/native.d.ts b/src/native.d.ts index bf75a0935..fe65e29d9 100644 --- a/src/native.d.ts +++ b/src/native.d.ts @@ -7,7 +7,7 @@ interface IConptyNative { connect(ptyId: number, commandLine: string, cwd: string, env: string[], useConptyDll: boolean, onExitCallback: (exitCode: number) => void): { pid: number }; resize(ptyId: number, cols: number, rows: number, useConptyDll: boolean): void; clear(ptyId: number, useConptyDll: boolean): void; - reparent(ptyId: number, hwnd: number | bigint, useConptyDll: boolean, reenableOnExit: boolean): boolean; + reparent(ptyId: number, hwnd: number | bigint, useConptyDll: boolean): boolean; kill(ptyId: number, useConptyDll: boolean): void; } diff --git a/src/win/conpty.cc b/src/win/conpty.cc index 1439dc278..1fa7189a1 100644 --- a/src/win/conpty.cc +++ b/src/win/conpty.cc @@ -50,8 +50,6 @@ struct pty_baton { HPCON hpc; HANDLE hShell = nullptr; - // Only set when the host opted in to re-enabling its window when the shell exits. - HWND parentWindowToReenable = nullptr; pty_baton(int _id, HANDLE _hIn, HANDLE _hOut, HPCON _hpc) : id(_id), hIn(_hIn), hOut(_hOut), hpc(_hpc) {}; }; @@ -76,47 +74,6 @@ struct ExitEvent { int exit_code = 0; }; -// Only the low 32 bits of a window handle are significant. -static bool IsSameWindow(HWND a, HWND b) { - return static_cast(reinterpret_cast(a)) == static_cast(reinterpret_cast(b)); -} - -struct OwnedWindowSearch { - HWND owner; - bool found; -}; - -static BOOL CALLBACK FindVisibleOwnedWindow(HWND hwnd, LPARAM lParam) { - auto* search = reinterpret_cast(lParam); - if (!IsWindowVisible(hwnd)) { - return TRUE; - } - // Walk the whole owner chain, since a dialog can be owned through a hidden proxy window. - HWND owner = GetWindow(hwnd, GW_OWNER); - for (int depth = 0; owner != nullptr && depth < 64; depth++) { - if (IsSameWindow(owner, search->owner)) { - search->found = true; - return FALSE; - } - owner = GetWindow(owner, GW_OWNER); - } - return TRUE; -} - -// A dialog that is modal to the parent window disables it until the dialog closes. If the dialog's -// process exits first, nothing re-enables the parent, so re-enable it when it is still disabled and -// no visible window is owned by it. This can't tell why the window is disabled, so hosts opt in. -static void EnableParentWindowIfOrphaned(HWND parentWindow) { - if (!IsWindow(parentWindow) || IsWindowEnabled(parentWindow)) { - return; - } - OwnedWindowSearch search = { parentWindow, false }; - EnumWindows(FindVisibleOwnedWindow, reinterpret_cast(&search)); - if (!search.found) { - EnableWindow(parentWindow, TRUE); - } -} - void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) { std::thread *th = new std::thread; // Don't use Napi::AsyncWorker which is limited by UV_THREADPOOL_SIZE. @@ -139,21 +96,15 @@ void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) { ExitEvent *exit_event = new ExitEvent; // Wait for process to complete. WaitForSingleObject(baton->hShell, INFINITE); - HWND parentWindow = nullptr; { std::lock_guard lock(g_ptyHandlesMutex); GetExitCodeProcess(baton->hShell, (LPDWORD)(&exit_event->exit_code)); CloseHandle(baton->hShell); - parentWindow = baton->parentWindowToReenable; const int id = baton->id; std::erase_if(ptyHandles, [id](const auto& ptyHandle) { return ptyHandle->id == id; }); } - if (parentWindow != nullptr) { - // EnableWindow waits for the parent window's thread, so don't hold up the exit event. - std::thread([parentWindow] { EnableParentWindowIfOrphaned(parentWindow); }).detach(); - } auto status = tsfn.BlockingCall(exit_event, callback); // In main thread switch (status) { @@ -609,12 +560,11 @@ static Napi::Value PtyReparent(const Napi::CallbackInfo& info) { Napi::Env env(info.Env()); Napi::HandleScope scope(env); - if (info.Length() != 4 || + if (info.Length() != 3 || !info[0].IsNumber() || !(info[1].IsNumber() || info[1].IsBigInt()) || - !info[2].IsBoolean() || - !info[3].IsBoolean()) { - throw Napi::Error::New(env, "Usage: pty.reparent(id, hwnd, useConptyDll, reenableOnExit)"); + !info[2].IsBoolean()) { + throw Napi::Error::New(env, "Usage: pty.reparent(id, hwnd, useConptyDll)"); } int id = info[0].As().Int32Value(); @@ -634,7 +584,6 @@ static Napi::Value PtyReparent(const Napi::CallbackInfo& info) { hwndValue = static_cast(info[1].As().Int64Value()); } const bool useConptyDll = info[2].As().Value(); - const bool reenableOnExit = info[3].As().Value(); // ConptyReparentPseudoConsole only exists in the bundled conpty.dll. if (!useConptyDll) { @@ -642,7 +591,7 @@ static Napi::Value PtyReparent(const Napi::CallbackInfo& info) { } std::lock_guard lock(g_ptyHandlesMutex); - pty_baton* handle = get_pty_baton(lock, id); + const pty_baton* handle = get_pty_baton(lock, id); bool sent = false; if (handle != nullptr && handle->hpc != nullptr) { @@ -658,9 +607,6 @@ static Napi::Value PtyReparent(const Napi::CallbackInfo& info) { // GetConsoleWindow(); ConPTY applies it asynchronously. const HWND newParent = reinterpret_cast(static_cast(hwndValue)); sent = SUCCEEDED(pfnReparentPseudoConsole(handle->hpc, newParent)); - if (sent) { - handle->parentWindowToReenable = reenableOnExit ? newParent : nullptr; - } } } } diff --git a/src/windowsPtyAgent.test.ts b/src/windowsPtyAgent.test.ts index 8b4c687b5..9d6979500 100644 --- a/src/windowsPtyAgent.test.ts +++ b/src/windowsPtyAgent.test.ts @@ -145,8 +145,6 @@ if (process.platform === 'win32') { false, false, false, - undefined, - false, createTestAgentOptions(connection, 10) ); @@ -176,8 +174,6 @@ if (process.platform === 'win32') { false, false, false, - undefined, - false, createTestAgentOptions(connection, 1000) ); @@ -204,8 +200,6 @@ if (process.platform === 'win32') { false, false, false, - undefined, - false, createTestAgentOptions(connection, 1000) ); let errorCount = 0; diff --git a/src/windowsPtyAgent.ts b/src/windowsPtyAgent.ts index d88d07741..e1068bca4 100644 --- a/src/windowsPtyAgent.ts +++ b/src/windowsPtyAgent.ts @@ -70,15 +70,8 @@ export class WindowsPtyAgent { debug: boolean, private _useConptyDll: boolean = false, conptyInheritCursor: boolean = false, - conptyParentWindow?: number | bigint, - private _reenableParentWindowOnExit: boolean = false, options: IWindowsPtyAgentOptions = defaultWindowsPtyAgentOptions ) { - // Validate before allocating the pseudoconsole, so bad input leaves nothing behind. - if (conptyParentWindow !== undefined) { - validateWindowHandle(conptyParentWindow); - } - if (!conptyNative) { conptyNative = loadNativeModule('conpty').module; } @@ -100,16 +93,6 @@ export class WindowsPtyAgent { // as a terminal id. this._pty = term.pty; - if (conptyParentWindow !== undefined) { - // Requested before connect() launches the shell, like Windows Terminal does. - try { - this._ptyNative.reparent(this._pty, conptyParentWindow, this._useConptyDll, this._reenableParentWindowOnExit); - } catch (e) { - this._ptyNative.kill(this._pty, this._useConptyDll); - throw e; - } - } - // Create terminal pipe IPC channel and forward to a local unix socket. this._outSocket = new Socket(); this._outSocket.setEncoding('utf8'); @@ -218,7 +201,7 @@ export class WindowsPtyAgent { */ public setParentWindow(handle: number | bigint): boolean { validateWindowHandle(handle); - return this._ptyNative.reparent(this._pty, handle, this._useConptyDll, this._reenableParentWindowOnExit); + return this._ptyNative.reparent(this._pty, handle, this._useConptyDll); } public kill(): void { diff --git a/src/windowsTerminal.test.ts b/src/windowsTerminal.test.ts index 5be964753..5fa497339 100644 --- a/src/windowsTerminal.test.ts +++ b/src/windowsTerminal.test.ts @@ -285,7 +285,7 @@ if (process.platform === 'win32') { }); describe('parent window', () => { - const user32 = `Add-Type -Namespace NodePtyTest -Name W -MemberDefinition '[DllImport("kernel32.dll")] public static extern IntPtr GetConsoleWindow(); [DllImport("user32.dll")] public static extern IntPtr GetAncestor(IntPtr h, uint f); [DllImport("user32.dll")] public static extern bool EnableWindow(IntPtr h, bool e); [DllImport("user32.dll")] public static extern bool IsWindowEnabled(IntPtr h); [DllImport("user32.dll", CharSet = CharSet.Unicode)] public static extern IntPtr CreateWindowExW(int ex, string cls, string title, int style, int x, int y, int w, int h, IntPtr owner, IntPtr menu, IntPtr inst, IntPtr param);'`; + const user32 = `Add-Type -Namespace NodePtyTest -Name W -MemberDefinition '[DllImport("kernel32.dll")] public static extern IntPtr GetConsoleWindow(); [DllImport("user32.dll")] public static extern IntPtr GetAncestor(IntPtr h, uint f);'`; function scriptArgs(...lines: string[]): string { return `-NoProfile -NonInteractive -EncodedCommand ${Buffer.from([user32, ...lines].join('; '), 'utf16le').toString('base64')}`; @@ -302,15 +302,6 @@ if (process.platform === 'win32') { ); } - // Disabling the root owner and exiting stands in for a modal dialog whose process dies. - function disableRootOwnerArgs(): string { - return scriptArgs( - `$o = [NodePtyTest.W]::GetAncestor([NodePtyTest.W]::GetConsoleWindow(), 3)`, - `[void][NodePtyTest.W]::EnableWindow($o, $false)`, - `Write-Host ('enabled=' + [NodePtyTest.W]::IsWindowEnabled($o) + ' end')` - ); - } - const started: Array<{ exited: Promise, kill: () => void }> = []; function startPty(file: string, args: string | string[], options: IWindowsPtyForkOptions = {}): WindowsTerminal { @@ -331,8 +322,8 @@ if (process.platform === 'win32') { return term; } - // Wait for every pty to exit, even after a failed test: exiting while a conout worker still - // flushes its pipe can hang mocha. + // Kill and wait for every pty, even after a failed test: ptys left running can keep mocha + // from exiting. afterEach(async function (): Promise { this.timeout(30000); const ptys = started.splice(0); @@ -363,27 +354,13 @@ if (process.platform === 'win32') { return (await readWindows(startPty('powershell.exe', rootOwnerArgs(0, 60)))).console; } - // Re-enabling happens off the exit path, so wait up to waitMs for the window to be enabled. - async function readEnabled(hwnd: number, waitMs: number): Promise { - const checker = startPty('powershell.exe', scriptArgs( - `$h = [IntPtr]${hwnd}`, - `for ($i = 0; $i -lt ${waitMs / 100} -and -not [NodePtyTest.W]::IsWindowEnabled($h); $i++) { Start-Sleep -Milliseconds 100 }`, - `Write-Host ('enabled=' + [NodePtyTest.W]::IsWindowEnabled($h) + ' end')` - )); - return (await readOutput(checker, /enabled=(\w+) end/))[1]; - } - - async function disableParentAndExit(parent: number, options: IWindowsPtyForkOptions): Promise { - const term = startPty('powershell.exe', disableRootOwnerArgs(), { conptyParentWindow: parent, ...options }); - const exited = new Promise(resolve => term.once('exit', () => resolve())); - assert.strictEqual((await readOutput(term, /enabled=(\w+) end/))[1], 'False'); - await exited; - } - - it('should make conptyParentWindow the owner of the console window before the shell starts', async function (): Promise { + it('should make the window the owner of the console window when set right after spawn', async function (): Promise { this.timeout(60000); const hostWindow = await startHostWindow(); - const windows = await readWindows(startPty('powershell.exe', rootOwnerArgs(0), { conptyParentWindow: hostWindow })); + const term = startPty('powershell.exe', rootOwnerArgs(0)); + // Not deferred, and the shell is only launched later, so this is sent before the shell starts. + assert.strictEqual(term.setParentWindow(hostWindow), useConptyDll); + const windows = await readWindows(term); assert.strictEqual(windows.rootOwner, useConptyDll ? hostWindow : windows.console); }); @@ -417,54 +394,11 @@ if (process.platform === 'win32') { [1.5, /must be a safe integer/], [Number.MAX_SAFE_INTEGER + 1, /must be a safe integer/] ]; - // Validated before the pseudoconsole is created, so nothing is left behind - for (const [handle, error] of invalid) { - assert.throws(() => startPty('cmd.exe', [], { conptyParentWindow: handle }), error); - } const term = startPty('cmd.exe', []); for (const [handle, error] of invalid) { assert.throws(() => term.setParentWindow(handle), error); } }); - - it('should re-enable the parent window on exit when asked, if a dialog left it disabled', async function (): Promise { - if (!useConptyDll) { - this.skip(); - } - this.timeout(60000); - const hostWindow = await startHostWindow(); - await disableParentAndExit(hostWindow, { conptyReenableParentWindowOnExit: true }); - assert.strictEqual(await readEnabled(hostWindow, 5000), 'True'); - }); - - it('should leave the parent window disabled on exit unless asked to re-enable it', async function (): Promise { - if (!useConptyDll) { - this.skip(); - } - this.timeout(60000); - const hostWindow = await startHostWindow(); - await disableParentAndExit(hostWindow, {}); - assert.strictEqual(await readEnabled(hostWindow, 3000), 'False'); - }); - - it('should not re-enable the parent window while it still owns a visible window', async function (): Promise { - if (!useConptyDll) { - this.skip(); - } - this.timeout(60000); - const hostWindow = await startHostWindow(); - // A visible off-screen window owned through a hidden proxy window, like a modal dialog - // shown by another process. - const dialog = startPty('powershell.exe', scriptArgs( - `$proxy = [NodePtyTest.W]::CreateWindowExW(0x80, 'STATIC', 'proxy', 0x80000000, 0, 0, 1, 1, [IntPtr]${hostWindow}, [IntPtr]::Zero, [IntPtr]::Zero, [IntPtr]::Zero)`, - `$dialog = [NodePtyTest.W]::CreateWindowExW(0x08000080, 'STATIC', 'dialog', 0x90000000, -32000, -32000, 1, 1, $proxy, [IntPtr]::Zero, [IntPtr]::Zero, [IntPtr]::Zero)`, - `Write-Host ('dialog=' + $dialog.ToInt64() + ' end')`, - `Start-Sleep -Seconds 60` - )); - assert.notStrictEqual((await readOutput(dialog, /dialog=(-?\d+) end/))[1], '0'); - await disableParentAndExit(hostWindow, { conptyReenableParentWindowOnExit: true }); - assert.strictEqual(await readEnabled(hostWindow, 3000), 'False'); - }); }); describe('Regression for #921', () => { it('should not crash with concurrent kills while resizing/clearing/reparenting', function (done) { diff --git a/src/windowsTerminal.ts b/src/windowsTerminal.ts index eaeb8f1b2..bc28e9acc 100644 --- a/src/windowsTerminal.ts +++ b/src/windowsTerminal.ts @@ -48,7 +48,7 @@ export class WindowsTerminal extends Terminal { this._deferreds = []; // Create new termal. - this._agent = new WindowsPtyAgent(file, args, parsedEnv, cwd, this._cols, this._rows, false, opt.useConptyDll, opt.conptyInheritCursor, opt.conptyParentWindow, opt.conptyReenableParentWindowOnExit); + this._agent = new WindowsPtyAgent(file, args, parsedEnv, cwd, this._cols, this._rows, false, opt.useConptyDll, opt.conptyInheritCursor); this._socket = this._agent.outSocket; // The socket 'close' handler that drives 'exit' is only attached after // ready_datapipe, which never fires when connect() fails. diff --git a/typings/node-pty.d.ts b/typings/node-pty.d.ts index cafa70bcf..bdc012175 100644 --- a/typings/node-pty.d.ts +++ b/typings/node-pty.d.ts @@ -109,28 +109,6 @@ declare module 'node-pty' { * @see https://docs.microsoft.com/en-us/windows/console/createpseudoconsole */ conptyInheritCursor?: boolean; - - /** - * The handle (HWND) of the top-level window that hosts this terminal. ConPTY makes it the owner - * of the console window returned by `GetConsoleWindow()`, so windows that console applications - * parent to it (for example `GetAncestor(GetConsoleWindow(), GA_ROOTOWNER)`, which MSAL/WAM - * sign-in uses) open on top of the host window instead of behind it. The owner is requested - * before the process is launched. Use {@link IPty.setParentWindow} when the terminal moves to - * another window. Must be a safe integer, or a bigint that fits in 64 bits. Only supported when - * `useConptyDll` is true; ignored otherwise. - */ - conptyParentWindow?: number | bigint; - - /** - * Whether to re-enable the parent window when the process exits, if the window is still - * disabled and no visible window is owned by it, directly or through other owned windows. The - * parent window is the last one set with `conptyParentWindow` or {@link IPty.setParentWindow}. - * This recovers from a modal dialog that died with its process, which otherwise leaves the - * parent window disabled for good. The check can't tell why the window is disabled, so only set - * it if the host never disables its window without showing a window it owns. Only supported - * when `useConptyDll` is true. Defaults to false. - */ - conptyReenableParentWindowOnExit?: boolean; } /** @@ -196,11 +174,15 @@ declare module 'node-pty' { clear(): void; /** - * Sets the handle (HWND) of the top-level window that hosts this terminal, see - * `IWindowsPtyForkOptions.conptyParentWindow`. Call it again whenever the terminal moves to a - * different window. Optional so existing implementations of `IPty` keep compiling. - * @param handle The window handle, for example Electron's - * `BrowserWindow.getNativeWindowHandle().readBigUInt64LE(0)`. + * Sets the handle (HWND) of the top-level window that hosts this terminal. ConPTY makes it the + * owner of the console window returned by `GetConsoleWindow()`, so windows that console + * applications parent to it (for example `GetAncestor(GetConsoleWindow(), GA_ROOTOWNER)`, which + * MSAL/WAM sign-in uses) open on top of the host window instead of behind it. Calling it right + * after spawning sends the request before the process is launched. Call it again whenever the + * terminal moves to a different window. Optional so existing implementations of `IPty` keep + * compiling. + * @param handle The window handle, as a safe integer or a bigint that fits in 64 bits, for + * example Electron's `BrowserWindow.getNativeWindowHandle().readBigUInt64LE(0)`. * @returns Whether the request was sent to ConPTY, which applies it asynchronously. This is * always false unless on Windows with `useConptyDll`, and false once the pseudoconsole is * closed. From 271faf38a6ee1f80f570cfc96d60367ed4a5bb5f Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Wed, 21 Oct 2026 13:27:55 -0700 Subject: [PATCH 07/11] fix(win): collect console processes before closing ConPTY Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/windowsPtyAgent.ts | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/windowsPtyAgent.ts b/src/windowsPtyAgent.ts index e1068bca4..aef34bf04 100644 --- a/src/windowsPtyAgent.ts +++ b/src/windowsPtyAgent.ts @@ -43,6 +43,7 @@ export class WindowsPtyAgent { private _closeTimeout: NodeJS.Timer | undefined; private _connectionTimeout: NodeJS.Timeout | undefined; private _exitCode: number | undefined; + private _killStarted = false; private _conoutSocketWorker: IConoutConnection; private _onError = new EventEmitter2(); @@ -205,6 +206,11 @@ export class WindowsPtyAgent { } public kill(): void { + if (this._killStarted) { + return; + } + this._killStarted = true; + // Prevent deferred connection from completing after kill this._clearConnectionTimeout(); this._pendingPtyInfo = undefined; @@ -213,7 +219,7 @@ export class WindowsPtyAgent { if (!this._useConptyDll) { this._inSocket.readable = false; this._outSocket.readable = false; - this._getConsoleProcessList().then(consoleProcessList => { + const kill = (consoleProcessList: number[]): void => { consoleProcessList.forEach((pid: number) => { try { process.kill(pid); @@ -221,9 +227,16 @@ export class WindowsPtyAgent { // Ignore if process cannot be found (kill ESRCH error) } }); - }); - this._ptyNative.kill(this._pty, this._useConptyDll); - this._conoutSocketWorker.dispose(); + this._ptyNative.kill(this._pty, this._useConptyDll); + this._conoutSocketWorker.dispose(); + }; + if (this._innerPid <= 0) { + kill([]); + } else { + // Get the process list before closing the pseudoconsole. Closing it lets process IDs be + // reused, so the asynchronous helper could otherwise return unrelated processes. + this._getConsoleProcessList().then(kill, () => kill([])); + } } else { // Close the input write handle to signal the end of session. this._inSocket.destroy(); From fae2ee92560e82feca3b999f907d2f8a4b3622aa Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Wed, 21 Oct 2026 13:46:55 -0700 Subject: [PATCH 08/11] Revert "fix(win): collect console processes before closing ConPTY" This reverts commit 271faf38a6ee1f80f570cfc96d60367ed4a5bb5f. --- src/windowsPtyAgent.ts | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/src/windowsPtyAgent.ts b/src/windowsPtyAgent.ts index aef34bf04..e1068bca4 100644 --- a/src/windowsPtyAgent.ts +++ b/src/windowsPtyAgent.ts @@ -43,7 +43,6 @@ export class WindowsPtyAgent { private _closeTimeout: NodeJS.Timer | undefined; private _connectionTimeout: NodeJS.Timeout | undefined; private _exitCode: number | undefined; - private _killStarted = false; private _conoutSocketWorker: IConoutConnection; private _onError = new EventEmitter2(); @@ -206,11 +205,6 @@ export class WindowsPtyAgent { } public kill(): void { - if (this._killStarted) { - return; - } - this._killStarted = true; - // Prevent deferred connection from completing after kill this._clearConnectionTimeout(); this._pendingPtyInfo = undefined; @@ -219,7 +213,7 @@ export class WindowsPtyAgent { if (!this._useConptyDll) { this._inSocket.readable = false; this._outSocket.readable = false; - const kill = (consoleProcessList: number[]): void => { + this._getConsoleProcessList().then(consoleProcessList => { consoleProcessList.forEach((pid: number) => { try { process.kill(pid); @@ -227,16 +221,9 @@ export class WindowsPtyAgent { // Ignore if process cannot be found (kill ESRCH error) } }); - this._ptyNative.kill(this._pty, this._useConptyDll); - this._conoutSocketWorker.dispose(); - }; - if (this._innerPid <= 0) { - kill([]); - } else { - // Get the process list before closing the pseudoconsole. Closing it lets process IDs be - // reused, so the asynchronous helper could otherwise return unrelated processes. - this._getConsoleProcessList().then(kill, () => kill([])); - } + }); + this._ptyNative.kill(this._pty, this._useConptyDll); + this._conoutSocketWorker.dispose(); } else { // Close the input write handle to signal the end of session. this._inSocket.destroy(); From cd33f21cf9e6a60af9f9a10d22bd8087ab152d7b Mon Sep 17 00:00:00 2001 From: anthonykim1 Date: Wed, 21 Oct 2026 15:15:15 -0700 Subject: [PATCH 09/11] Address review: accept only unsigned, pointer-sized window handles HWNDs are pointers, so negative handles are now rejected and the handle is converted to an HWND through uintptr_t in C++, which also covers 32-bit builds. Numbers must be non-negative safe integers, since Node-API's integer getters silently truncate fractions, map NaN and Infinity to 0 and lose precision above 2^53. Remove the JS check, so the native binding is the only place that validates the handle. --- src/win/conpty.cc | 51 +++++++++++++++++++++++++------------ src/windowsPtyAgent.ts | 16 ------------ src/windowsTerminal.test.ts | 38 ++++++++++++++------------- typings/node-pty.d.ts | 5 ++-- 4 files changed, 59 insertions(+), 51 deletions(-) diff --git a/src/win/conpty.cc b/src/win/conpty.cc index 1fa7189a1..aa2328bdf 100644 --- a/src/win/conpty.cc +++ b/src/win/conpty.cc @@ -15,6 +15,7 @@ #include #include // PathCombine, PathIsRelative #include +#include #include #include #include @@ -556,32 +557,51 @@ static Napi::Value PtyClear(const Napi::CallbackInfo& info) { return env.Undefined(); } +// A window handle is a pointer, so it's only valid as a non-negative integer that fits in one. +static bool TryGetWindowHandle(const Napi::Value& value, HWND* hwnd) { + uint64_t handle; + if (value.IsBigInt()) { + // Not lossless when negative or above 2^64 - 1. + bool lossless = false; + handle = value.As().Uint64Value(&lossless); + if (!lossless) { + return false; + } + } else if (value.IsNumber()) { + // Node-API's integer getters silently truncate fractions, map NaN and Infinity to 0 and lose + // precision above 2^53, so validate the double itself. + constexpr double maxSafeInteger = 9007199254740991.0; // Number.MAX_SAFE_INTEGER + const double number = value.As().DoubleValue(); + if (!(number >= 0 && number <= maxSafeInteger) || std::trunc(number) != number) { + return false; + } + handle = static_cast(number); + } else { + return false; + } + // Rejects handles that don't round-trip through a pointer, i.e. above 2^32 - 1 in 32-bit builds. + const uintptr_t pointer = static_cast(handle); + if (pointer != handle) { + return false; + } + *hwnd = reinterpret_cast(pointer); + return true; +} + static Napi::Value PtyReparent(const Napi::CallbackInfo& info) { Napi::Env env(info.Env()); Napi::HandleScope scope(env); if (info.Length() != 3 || !info[0].IsNumber() || - !(info[1].IsNumber() || info[1].IsBigInt()) || !info[2].IsBoolean()) { throw Napi::Error::New(env, "Usage: pty.reparent(id, hwnd, useConptyDll)"); } int id = info[0].As().Int32Value(); - uint64_t hwndValue; - if (info[1].IsBigInt()) { - // Accept signed and unsigned 64-bit values, but never truncate. - const Napi::BigInt hwndBigInt = info[1].As(); - bool lossless = false; - hwndValue = hwndBigInt.Uint64Value(&lossless); - if (!lossless) { - hwndValue = static_cast(hwndBigInt.Int64Value(&lossless)); - } - if (!lossless) { - throw Napi::Error::New(env, "pty.reparent: hwnd does not fit in 64 bits"); - } - } else { - hwndValue = static_cast(info[1].As().Int64Value()); + HWND newParent; + if (!TryGetWindowHandle(info[1], &newParent)) { + throw Napi::Error::New(env, "pty.reparent: hwnd must be a non-negative safe integer or a bigint that fits in a pointer"); } const bool useConptyDll = info[2].As().Value(); @@ -605,7 +625,6 @@ static Napi::Value PtyReparent(const Napi::CallbackInfo& info) { { // Asks ConPTY to make newParent the owner of the console window returned by // GetConsoleWindow(); ConPTY applies it asynchronously. - const HWND newParent = reinterpret_cast(static_cast(hwndValue)); sent = SUCCEEDED(pfnReparentPseudoConsole(handle->hpc, newParent)); } } diff --git a/src/windowsPtyAgent.ts b/src/windowsPtyAgent.ts index e1068bca4..901e7644b 100644 --- a/src/windowsPtyAgent.ts +++ b/src/windowsPtyAgent.ts @@ -200,7 +200,6 @@ export class WindowsPtyAgent { * whether the request was sent to ConPTY, which applies it asynchronously. */ public setParentWindow(handle: number | bigint): boolean { - validateWindowHandle(handle); return this._ptyNative.reparent(this._pty, handle, this._useConptyDll); } @@ -361,18 +360,3 @@ function repeatText(text: string, count: number): string { function xOr(arg1: boolean, arg2: boolean): boolean { return ((arg1 && !arg2) || (!arg1 && arg2)); } - -/** - * Throws unless the handle is a safe integer or a bigint that fits in 64 bits. Signed and unsigned - * bigints are both accepted, since a window handle may be sign-extended. - */ -function validateWindowHandle(handle: number | bigint): void { - if (typeof handle === 'bigint') { - if (handle < BigInt('-9223372036854775808') || handle > BigInt('18446744073709551615')) { - throw new Error('Window handle does not fit in 64 bits'); - } - } else if (!Number.isSafeInteger(handle)) { - // Larger numbers can't hold an exact handle. - throw new Error('Window handle must be a safe integer or a bigint'); - } -} diff --git a/src/windowsTerminal.test.ts b/src/windowsTerminal.test.ts index 5fa497339..a491494bc 100644 --- a/src/windowsTerminal.test.ts +++ b/src/windowsTerminal.test.ts @@ -285,19 +285,19 @@ if (process.platform === 'win32') { }); describe('parent window', () => { - const user32 = `Add-Type -Namespace NodePtyTest -Name W -MemberDefinition '[DllImport("kernel32.dll")] public static extern IntPtr GetConsoleWindow(); [DllImport("user32.dll")] public static extern IntPtr GetAncestor(IntPtr h, uint f);'`; + const user32 = `Add-Type -Namespace NodePtyTest -Name W -MemberDefinition '[DllImport("kernel32.dll")] public static extern IntPtr GetConsoleWindow(); [DllImport("user32.dll")] public static extern IntPtr GetAncestor(IntPtr h, uint f); public static ulong U(IntPtr h) { return unchecked((ulong)h.ToInt64()); }'`; function scriptArgs(...lines: string[]): string { return `-NoProfile -NonInteractive -EncodedCommand ${Buffer.from([user32, ...lines].join('; '), 'utf16le').toString('base64')}`; } // Prints the console window and GetAncestor(GetConsoleWindow(), GA_ROOTOWNER), which is the - // parent MSAL/WAM uses for sign-in dialogs. + // parent MSAL/WAM uses for sign-in dialogs. Handles are pointers, so print them unsigned. function rootOwnerArgs(delayMs: number, keepAliveSeconds: number = 0): string { return scriptArgs( `Start-Sleep -Milliseconds ${delayMs}`, `$c = [NodePtyTest.W]::GetConsoleWindow()`, - `Write-Host ('console=' + $c.ToInt64() + ' rootOwner=' + [NodePtyTest.W]::GetAncestor($c, 3).ToInt64() + ' end')`, + `Write-Host ('console=' + [NodePtyTest.W]::U($c) + ' rootOwner=' + [NodePtyTest.W]::U([NodePtyTest.W]::GetAncestor($c, 3)) + ' end')`, `Start-Sleep -Seconds ${keepAliveSeconds}` ); } @@ -344,22 +344,24 @@ if (process.platform === 'win32') { }); } - async function readWindows(term: WindowsTerminal): Promise<{ console: number, rootOwner: number }> { - const match = await readOutput(term, /console=(-?\d+) rootOwner=(-?\d+) end/); - return { console: parseInt(match[1], 10), rootOwner: parseInt(match[2], 10) }; + async function readWindows(term: WindowsTerminal): Promise<{ console: bigint, rootOwner: bigint }> { + const match = await readOutput(term, /console=(\d+) rootOwner=(\d+) end/); + return { console: BigInt(match[1]), rootOwner: BigInt(match[2]) }; } // Another pty's hidden top-level console window stands in for the host app's window. - async function startHostWindow(): Promise { + async function startHostWindow(): Promise { return (await readWindows(startPty('powershell.exe', rootOwnerArgs(0, 60)))).console; } it('should make the window the owner of the console window when set right after spawn', async function (): Promise { this.timeout(60000); const hostWindow = await startHostWindow(); + // Pass a number when it's exact, so both the number and the bigint path are covered. + const handle = hostWindow <= BigInt(Number.MAX_SAFE_INTEGER) ? Number(hostWindow) : hostWindow; const term = startPty('powershell.exe', rootOwnerArgs(0)); // Not deferred, and the shell is only launched later, so this is sent before the shell starts. - assert.strictEqual(term.setParentWindow(hostWindow), useConptyDll); + assert.strictEqual(term.setParentWindow(handle), useConptyDll); const windows = await readWindows(term); assert.strictEqual(windows.rootOwner, useConptyDll ? hostWindow : windows.console); }); @@ -370,7 +372,7 @@ if (process.platform === 'win32') { const term = startPty('powershell.exe', rootOwnerArgs(3000)); let sent: boolean | undefined; // The first data event means the shell is running and conpty has been released. - term.once('data', () => sent = term.setParentWindow(BigInt(hostWindow))); + term.once('data', () => sent = term.setParentWindow(hostWindow)); const windows = await readWindows(term); assert.strictEqual(sent, useConptyDll); assert.strictEqual(windows.rootOwner, useConptyDll ? hostWindow : windows.console); @@ -387,16 +389,18 @@ if (process.platform === 'win32') { assert.strictEqual(term.setParentWindow(0), false); }); - it('should reject handles that are not safe integers or bigints that fit in 64 bits', () => { - const invalid: Array<[number | bigint, RegExp]> = [ - [BigInt('18446744073709551616'), /does not fit in 64 bits/], - [BigInt('-9223372036854775809'), /does not fit in 64 bits/], - [1.5, /must be a safe integer/], - [Number.MAX_SAFE_INTEGER + 1, /must be a safe integer/] + it('should reject handles that are not non-negative integers that fit in a pointer', () => { + const invalid: Array = [ + -1, + BigInt('-1'), + 1.5, + NaN, + Number.MAX_SAFE_INTEGER + 1, + BigInt('18446744073709551616') ]; const term = startPty('cmd.exe', []); - for (const [handle, error] of invalid) { - assert.throws(() => term.setParentWindow(handle), error); + for (const handle of invalid) { + assert.throws(() => term.setParentWindow(handle), /hwnd must be a non-negative safe integer or a bigint that fits in a pointer/); } }); }); diff --git a/typings/node-pty.d.ts b/typings/node-pty.d.ts index bdc012175..102ceab65 100644 --- a/typings/node-pty.d.ts +++ b/typings/node-pty.d.ts @@ -181,8 +181,9 @@ declare module 'node-pty' { * after spawning sends the request before the process is launched. Call it again whenever the * terminal moves to a different window. Optional so existing implementations of `IPty` keep * compiling. - * @param handle The window handle, as a safe integer or a bigint that fits in 64 bits, for - * example Electron's `BrowserWindow.getNativeWindowHandle().readBigUInt64LE(0)`. + * @param handle The window handle. HWNDs are pointers, so this must be a non-negative safe + * integer or a bigint that fits in a pointer, for example Electron's + * `BrowserWindow.getNativeWindowHandle().readBigUInt64LE(0)`. Anything else throws. * @returns Whether the request was sent to ConPTY, which applies it asynchronously. This is * always false unless on Windows with `useConptyDll`, and false once the pseudoconsole is * closed. From 447be77be40c8119f24047d74469f9fcf2dcc919 Mon Sep 17 00:00:00 2001 From: anthonykim1 Date: Wed, 21 Oct 2026 17:17:58 -0700 Subject: [PATCH 10/11] Reword the window handle comment and test name Only Int64Value() loses precision past Number.MAX_SAFE_INTEGER; the 32-bit getters wrap instead. The invalid handle test also rejects unsafe integers, so say safe integers in its name. --- src/win/conpty.cc | 4 ++-- src/windowsTerminal.test.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/win/conpty.cc b/src/win/conpty.cc index aa2328bdf..529d1cb23 100644 --- a/src/win/conpty.cc +++ b/src/win/conpty.cc @@ -568,8 +568,8 @@ static bool TryGetWindowHandle(const Napi::Value& value, HWND* hwnd) { return false; } } else if (value.IsNumber()) { - // Node-API's integer getters silently truncate fractions, map NaN and Infinity to 0 and lose - // precision above 2^53, so validate the double itself. + // Napi::Number::Int64Value() silently truncates fractions, maps NaN and Infinity to 0 and + // loses precision past Number.MAX_SAFE_INTEGER, so validate the double itself. constexpr double maxSafeInteger = 9007199254740991.0; // Number.MAX_SAFE_INTEGER const double number = value.As().DoubleValue(); if (!(number >= 0 && number <= maxSafeInteger) || std::trunc(number) != number) { diff --git a/src/windowsTerminal.test.ts b/src/windowsTerminal.test.ts index a491494bc..1ab9bacce 100644 --- a/src/windowsTerminal.test.ts +++ b/src/windowsTerminal.test.ts @@ -389,7 +389,7 @@ if (process.platform === 'win32') { assert.strictEqual(term.setParentWindow(0), false); }); - it('should reject handles that are not non-negative integers that fit in a pointer', () => { + it('should reject handles that are not non-negative safe integers or bigints that fit in a pointer', () => { const invalid: Array = [ -1, BigInt('-1'), From f6991751bd86b4e40437917f6ae5f9a44738f6bb Mon Sep 17 00:00:00 2001 From: anthonykim1 Date: Thu, 22 Oct 2026 07:55:55 -0700 Subject: [PATCH 11/11] Address review: take the window handle as a Buffer setParentWindow() now takes the HWND as a Buffer the size of an HWND, in native byte order, which is how Electron's BrowserWindow.getNativeWindowHandle() returns it, so hosts can pass that Buffer directly. The native side copies it into an HWND, which replaces the number and bigint parsing. --- src/interfaces.ts | 2 +- src/native.d.ts | 2 +- src/terminal.test.ts | 2 +- src/terminal.ts | 2 +- src/unixTerminal.ts | 2 +- src/win/conpty.cc | 33 ++++++++------------------------- src/windowsPtyAgent.ts | 2 +- src/windowsTerminal.test.ts | 34 +++++++++++++++++----------------- src/windowsTerminal.ts | 2 +- typings/node-pty.d.ts | 8 ++++---- 10 files changed, 36 insertions(+), 53 deletions(-) diff --git a/src/interfaces.ts b/src/interfaces.ts index d69b947bb..44b4c49b2 100644 --- a/src/interfaces.ts +++ b/src/interfaces.ts @@ -44,7 +44,7 @@ export interface ITerminal { * Sets the top-level window (HWND) that hosts this terminal. Returns whether the request was sent * to ConPTY, which only happens on Windows with `useConptyDll`. */ - setParentWindow(handle: number | bigint): boolean; + setParentWindow(handle: Buffer): boolean; /** * Close, kill and destroy the socket. diff --git a/src/native.d.ts b/src/native.d.ts index fe65e29d9..9bd646376 100644 --- a/src/native.d.ts +++ b/src/native.d.ts @@ -7,7 +7,7 @@ interface IConptyNative { connect(ptyId: number, commandLine: string, cwd: string, env: string[], useConptyDll: boolean, onExitCallback: (exitCode: number) => void): { pid: number }; resize(ptyId: number, cols: number, rows: number, useConptyDll: boolean): void; clear(ptyId: number, useConptyDll: boolean): void; - reparent(ptyId: number, hwnd: number | bigint, useConptyDll: boolean): boolean; + reparent(ptyId: number, hwnd: Buffer, useConptyDll: boolean): boolean; kill(ptyId: number, useConptyDll: boolean): void; } diff --git a/src/terminal.test.ts b/src/terminal.test.ts index fe96bddd8..9d6fe704d 100644 --- a/src/terminal.test.ts +++ b/src/terminal.test.ts @@ -32,7 +32,7 @@ class TestTerminal extends Terminal { public clear(): void { throw new Error('Method not implemented.'); } - public setParentWindow(handle: number | bigint): boolean { + public setParentWindow(handle: Buffer): boolean { throw new Error('Method not implemented.'); } public destroy(): void { diff --git a/src/terminal.ts b/src/terminal.ts index 735315aa5..7e268521a 100644 --- a/src/terminal.ts +++ b/src/terminal.ts @@ -180,7 +180,7 @@ export abstract class Terminal implements ITerminal { public abstract resize(cols: number, rows: number, pixelSize?: { width: number, height: number }): void; public abstract clear(): void; - public abstract setParentWindow(handle: number | bigint): boolean; + public abstract setParentWindow(handle: Buffer): boolean; public abstract destroy(): void; public abstract kill(signal?: string): void; diff --git a/src/unixTerminal.ts b/src/unixTerminal.ts index 72b20700d..1670d8e29 100644 --- a/src/unixTerminal.ts +++ b/src/unixTerminal.ts @@ -283,7 +283,7 @@ export class UnixTerminal extends Terminal { } - public setParentWindow(handle: number | bigint): boolean { + public setParentWindow(handle: Buffer): boolean { // Windows only return false; } diff --git a/src/win/conpty.cc b/src/win/conpty.cc index 529d1cb23..7447f8f47 100644 --- a/src/win/conpty.cc +++ b/src/win/conpty.cc @@ -15,7 +15,7 @@ #include #include // PathCombine, PathIsRelative #include -#include +#include #include #include #include @@ -557,34 +557,17 @@ static Napi::Value PtyClear(const Napi::CallbackInfo& info) { return env.Undefined(); } -// A window handle is a pointer, so it's only valid as a non-negative integer that fits in one. +// The handle is the bytes of an HWND in native byte order, which is how Electron's +// BrowserWindow.getNativeWindowHandle() returns it. static bool TryGetWindowHandle(const Napi::Value& value, HWND* hwnd) { - uint64_t handle; - if (value.IsBigInt()) { - // Not lossless when negative or above 2^64 - 1. - bool lossless = false; - handle = value.As().Uint64Value(&lossless); - if (!lossless) { - return false; - } - } else if (value.IsNumber()) { - // Napi::Number::Int64Value() silently truncates fractions, maps NaN and Infinity to 0 and - // loses precision past Number.MAX_SAFE_INTEGER, so validate the double itself. - constexpr double maxSafeInteger = 9007199254740991.0; // Number.MAX_SAFE_INTEGER - const double number = value.As().DoubleValue(); - if (!(number >= 0 && number <= maxSafeInteger) || std::trunc(number) != number) { - return false; - } - handle = static_cast(number); - } else { + if (!value.IsBuffer()) { return false; } - // Rejects handles that don't round-trip through a pointer, i.e. above 2^32 - 1 in 32-bit builds. - const uintptr_t pointer = static_cast(handle); - if (pointer != handle) { + const auto buffer = value.As>(); + if (buffer.Length() != sizeof(HWND)) { return false; } - *hwnd = reinterpret_cast(pointer); + std::memcpy(hwnd, buffer.Data(), sizeof(*hwnd)); return true; } @@ -601,7 +584,7 @@ static Napi::Value PtyReparent(const Napi::CallbackInfo& info) { int id = info[0].As().Int32Value(); HWND newParent; if (!TryGetWindowHandle(info[1], &newParent)) { - throw Napi::Error::New(env, "pty.reparent: hwnd must be a non-negative safe integer or a bigint that fits in a pointer"); + throw Napi::Error::New(env, "pty.reparent: hwnd must be a Buffer the size of an HWND"); } const bool useConptyDll = info[2].As().Value(); diff --git a/src/windowsPtyAgent.ts b/src/windowsPtyAgent.ts index 901e7644b..5b4f366c0 100644 --- a/src/windowsPtyAgent.ts +++ b/src/windowsPtyAgent.ts @@ -199,7 +199,7 @@ export class WindowsPtyAgent { * that console apps parent to it (e.g. WAM sign-in) stack above the host window. Returns * whether the request was sent to ConPTY, which applies it asynchronously. */ - public setParentWindow(handle: number | bigint): boolean { + public setParentWindow(handle: Buffer): boolean { return this._ptyNative.reparent(this._pty, handle, this._useConptyDll); } diff --git a/src/windowsTerminal.test.ts b/src/windowsTerminal.test.ts index 1ab9bacce..5aa6c843a 100644 --- a/src/windowsTerminal.test.ts +++ b/src/windowsTerminal.test.ts @@ -354,14 +354,20 @@ if (process.platform === 'win32') { return (await readWindows(startPty('powershell.exe', rootOwnerArgs(0, 60)))).console; } + // An HWND the way setParentWindow takes it, like Electron's getNativeWindowHandle(): its bytes + // in native byte order. The tests run in 64-bit processes. + function toHandle(hwnd: bigint): Buffer { + const handle = Buffer.alloc(8); + handle.writeBigUInt64LE(hwnd); + return handle; + } + it('should make the window the owner of the console window when set right after spawn', async function (): Promise { this.timeout(60000); const hostWindow = await startHostWindow(); - // Pass a number when it's exact, so both the number and the bigint path are covered. - const handle = hostWindow <= BigInt(Number.MAX_SAFE_INTEGER) ? Number(hostWindow) : hostWindow; const term = startPty('powershell.exe', rootOwnerArgs(0)); // Not deferred, and the shell is only launched later, so this is sent before the shell starts. - assert.strictEqual(term.setParentWindow(handle), useConptyDll); + assert.strictEqual(term.setParentWindow(toHandle(hostWindow)), useConptyDll); const windows = await readWindows(term); assert.strictEqual(windows.rootOwner, useConptyDll ? hostWindow : windows.console); }); @@ -372,7 +378,7 @@ if (process.platform === 'win32') { const term = startPty('powershell.exe', rootOwnerArgs(3000)); let sent: boolean | undefined; // The first data event means the shell is running and conpty has been released. - term.once('data', () => sent = term.setParentWindow(hostWindow)); + term.once('data', () => sent = term.setParentWindow(toHandle(hostWindow))); const windows = await readWindows(term); assert.strictEqual(sent, useConptyDll); assert.strictEqual(windows.rootOwner, useConptyDll ? hostWindow : windows.console); @@ -384,23 +390,16 @@ if (process.platform === 'win32') { const exited = new Promise(resolve => term.once('exit', () => resolve())); await new Promise(resolve => term.once('data', () => resolve())); term.kill(); - assert.strictEqual(term.setParentWindow(0), false); + assert.strictEqual(term.setParentWindow(Buffer.alloc(8)), false); await exited; - assert.strictEqual(term.setParentWindow(0), false); + assert.strictEqual(term.setParentWindow(Buffer.alloc(8)), false); }); - it('should reject handles that are not non-negative safe integers or bigints that fit in a pointer', () => { - const invalid: Array = [ - -1, - BigInt('-1'), - 1.5, - NaN, - Number.MAX_SAFE_INTEGER + 1, - BigInt('18446744073709551616') - ]; + it('should reject handles that are not buffers the size of an HWND', () => { + const invalid: unknown[] = [Buffer.alloc(4), Buffer.alloc(9), Buffer.alloc(0), 1, BigInt(1), '0x1234', undefined]; const term = startPty('cmd.exe', []); for (const handle of invalid) { - assert.throws(() => term.setParentWindow(handle), /hwnd must be a non-negative safe integer or a bigint that fits in a pointer/); + assert.throws(() => term.setParentWindow(handle as Buffer), /hwnd must be a Buffer the size of an HWND/); } }); }); @@ -412,6 +411,7 @@ if (process.platform === 'win32') { let ready = 0; let exited = 0; let spamInterval: NodeJS.Timeout | undefined; + const noWindow = Buffer.alloc(8); const cleanup = (err?: Error): void => { if (spamInterval) { clearInterval(spamInterval); @@ -429,7 +429,7 @@ if (process.platform === 'win32') { t.clear(); } catch (e) { /* already exited */ } try { - t.setParentWindow(0); + t.setParentWindow(noWindow); } catch (e) { /* already exited */ } } }, 1); diff --git a/src/windowsTerminal.ts b/src/windowsTerminal.ts index bc28e9acc..c01be4221 100644 --- a/src/windowsTerminal.ts +++ b/src/windowsTerminal.ts @@ -163,7 +163,7 @@ export class WindowsTerminal extends Terminal { }); } - public setParentWindow(handle: number | bigint): boolean { + public setParentWindow(handle: Buffer): boolean { // Not deferred: the pseudoconsole exists as soon as the agent is created, and the owner // should be requested before the shell starts asking for it. return this._agent.setParentWindow(handle); diff --git a/typings/node-pty.d.ts b/typings/node-pty.d.ts index 102ceab65..f46c02968 100644 --- a/typings/node-pty.d.ts +++ b/typings/node-pty.d.ts @@ -181,14 +181,14 @@ declare module 'node-pty' { * after spawning sends the request before the process is launched. Call it again whenever the * terminal moves to a different window. Optional so existing implementations of `IPty` keep * compiling. - * @param handle The window handle. HWNDs are pointers, so this must be a non-negative safe - * integer or a bigint that fits in a pointer, for example Electron's - * `BrowserWindow.getNativeWindowHandle().readBigUInt64LE(0)`. Anything else throws. + * @param handle The window handle (HWND) as its bytes in native byte order, the way Electron's + * `BrowserWindow.getNativeWindowHandle()` returns it. Throws unless it's a Buffer the size of + * an HWND (8 bytes in a 64-bit process). * @returns Whether the request was sent to ConPTY, which applies it asynchronously. This is * always false unless on Windows with `useConptyDll`, and false once the pseudoconsole is * closed. */ - setParentWindow?(handle: number | bigint): boolean; + setParentWindow?(handle: Buffer): boolean; /** * Writes data to the pty.