fix(windows): reject a node-pty addon that predates the MSYS breakaway denial

The native-runtime gate asserted only that terminateJob, listJobProcessIds and
assignCurrentProcessToJob were exported. All three predate the Cygwin/MSYS
breakaway denial, so an addon built before it passes every gate,
isPtyJobOwnershipAvailable() returns true, and windows-pty-job.win32.test.ts
passes 6/6 -- while every Git Bash child is created outside its pane's job and
survives terminatePtyJob.

Read the resolved .node and require the wide msys-2.0.dll literal that
usesCygwinRuntime holds, the way stagedRelayAddonIsUnpatched() already tells a
patched windows-process-tree addon from a published one. An addon the caller
cannot name is refused rather than skipped: a gate that cannot see its subject
is not a gate.

Verified against real binaries on a Windows 11 host: the shared checkout's
pre-#19068 build errors, a build from current patched source passes, a missing
path errors.

Also closes the cross-host packaging skip. The export half has to load the
addon so it cannot run when the packaging host is not the target, which is how
a Windows release built elsewhere could ship this. The marker is a file read
and needs neither; an unrecognised layout warns rather than fails a release
that was packaging fine.
This commit is contained in:
Neil
2026-09-11 01:16:31 -07:00
committed by Neil
parent bb094c12c2
commit aa6c2a0bc5
7 changed files with 330 additions and 52 deletions
+6 -1
View File
@@ -15,7 +15,8 @@ const { verifyLinuxGlibcFloor } = require('./scripts/verify-linux-glibc-floor.cj
const { writeMacBuildCompatibility } = require('./scripts/mac-build-compatibility.cjs')
const { verifyPackagedPluginResources } = require('./scripts/verify-packaged-plugin-resources.cjs')
const {
verifyPackagedNodePtyJobOwnership
verifyPackagedNodePtyJobOwnership,
verifyPackagedConptyBreakawayMarker
} = require('./scripts/verify-packaged-node-pty-job-ownership.cjs')
const { verifySkillsCliRuntime } = require('./scripts/verify-skills-cli-runtime.cjs')
const { verifyStaticAppImagePackage } = require('./scripts/static-appimage-package-contract.cjs')
@@ -342,7 +343,11 @@ module.exports = {
if (process.platform === 'win32' && canExecuteTargetArch) {
verifyPackagedNodePtyJobOwnership(resourcesDir)
} else {
// The export check needs to load the addon, so it cannot run here. The
// MSYS breakaway marker is a file read, and skipping it is how a
// cross-host Windows release could ship the orphan bug.
console.log('[verify-packaged-node-pty] skipped cross-platform or cross-arch package')
verifyPackagedConptyBreakawayMarker(resourcesDir)
}
}
verifySkillsCliRuntime(join(resourcesDir, 'app.asar.unpacked', 'out'), resourcesDir, {
@@ -1,22 +1,41 @@
import { readFileSync } from 'node:fs'
import { mkdtempSync, readFileSync, writeFileSync } from 'node:fs'
import { createRequire } from 'node:module'
import { tmpdir } from 'node:os'
import { join } from 'node:path'
import { describe, expect, it } from 'vitest'
const require = createRequire(import.meta.url)
const { assertNodePtyJobOwnership } = require('./node-pty-job-ownership.cjs')
const { assertNodePtyJobOwnership, nodePtyAddonPath } = require('./node-pty-job-ownership.cjs')
const NODE_PTY_PATCH = readFileSync(
new URL('../patches/node-pty@1.1.0.patch', import.meta.url),
'utf8'
)
const PATCHED = {
dir: 'build/Release/',
module: {
listJobProcessIds: () => [],
terminateJob: () => true,
assignCurrentProcessToJob: () => true
}
const JOB_EXPORTS = {
listJobProcessIds: () => [],
terminateJob: () => true,
assignCurrentProcessToJob: () => true
}
const fixtureDir = mkdtempSync(join(tmpdir(), 'node-pty-job-ownership-'))
/** A stand-in addon; only the wide literal the gate reads has to be real. */
function writeAddon(name, { cygwinBreakawayDenied }) {
const path = join(fixtureDir, name)
writeFileSync(
path,
Buffer.concat([
Buffer.from('MZ fake addon '),
cygwinBreakawayDenied ? Buffer.from('msys-2.0.dll', 'utf16le') : Buffer.alloc(0)
])
)
return path
}
const CURRENT_ADDON = writeAddon('current.node', { cygwinBreakawayDenied: true })
const PRE_MSYS_ADDON = writeAddon('pre-msys.node', { cygwinBreakawayDenied: false })
const PATCHED = { dir: 'build/Release/', module: JOB_EXPORTS }
const PREBUILD = {
dir: 'prebuilds/win32-x64/',
module: {
@@ -28,6 +47,13 @@ const PREBUILD = {
}
}
const onWindows = (native, addonPath) => ({
platform: 'win32',
nativeName: 'conpty',
native,
addonPath
})
describe('assertNodePtyJobOwnership', () => {
it('keeps node-addon-api project paths absolute during Windows source builds', () => {
expect(NODE_PTY_PATCH).toContain(
@@ -41,21 +67,43 @@ describe('assertNodePtyJobOwnership', () => {
})
it('rejects the prebuild that shipped without the job exports', () => {
expect(() =>
assertNodePtyJobOwnership({ platform: 'win32', nativeName: 'conpty', native: PREBUILD })
).toThrow(/listJobProcessIds, terminateJob, assignCurrentProcessToJob/)
expect(() => assertNodePtyJobOwnership(onWindows(PREBUILD, CURRENT_ADDON))).toThrow(
/listJobProcessIds, terminateJob, assignCurrentProcessToJob/
)
})
it('names where the bad native came from, so the fix is obvious', () => {
expect(() =>
assertNodePtyJobOwnership({ platform: 'win32', nativeName: 'conpty', native: PREBUILD })
).toThrow(/prebuilds\/win32-x64/)
expect(() => assertNodePtyJobOwnership(onWindows(PREBUILD, CURRENT_ADDON))).toThrow(
/prebuilds\/win32-x64/
)
})
it('accepts a source build carrying the patch', () => {
expect(() =>
assertNodePtyJobOwnership({ platform: 'win32', nativeName: 'conpty', native: PATCHED })
).not.toThrow()
expect(() => assertNodePtyJobOwnership(onWindows(PATCHED, CURRENT_ADDON))).not.toThrow()
})
// The reason this gate reads the binary at all: every export above predates
// the Cygwin/MSYS breakaway denial, so a build that leaks every Git Bash
// child out of its pane's job satisfies all of them.
it('rejects a source build that predates the Cygwin/MSYS breakaway denial', () => {
expect(() => assertNodePtyJobOwnership(onWindows(PATCHED, PRE_MSYS_ADDON))).toThrow(
/predates the Cygwin\/MSYS job-breakaway denial/
)
})
it('tells that build apart by path, and says to rebuild', () => {
expect(() => assertNodePtyJobOwnership(onWindows(PATCHED, PRE_MSYS_ADDON))).toThrow(
/pre-msys\.node[\s\S]*Rebuild node-pty from source/
)
})
it.each([
['no path at all', undefined],
['a path that is not there', join(fixtureDir, 'absent.node')]
])('refuses rather than skip when the addon cannot be read: %s', (_case, addonPath) => {
expect(() => assertNodePtyJobOwnership(onWindows(PATCHED, addonPath))).toThrow(
/Cannot read node-pty's conpty native/
)
})
it.each([
@@ -65,3 +113,25 @@ describe('assertNodePtyJobOwnership', () => {
expect(() => assertNodePtyJobOwnership({ ...spec, native: PREBUILD })).not.toThrow()
})
})
describe('nodePtyAddonPath', () => {
it('resolves the addon against node-pty lib, which is the only base callers share', () => {
expect(
nodePtyAddonPath(
'/app/node_modules/node-pty/lib/utils.js',
{ dir: '../build/Release/' },
'conpty'
)
).toBe('/app/node_modules/node-pty/build/Release/conpty.node')
})
it('handles the bundled layout, where the addon sits beside lib', () => {
expect(
nodePtyAddonPath(
'/app/resources/node-pty/lib/utils.js',
{ dir: './build/Release/' },
'conpty'
)
).toBe('/app/resources/node-pty/lib/build/Release/conpty.node')
})
})
+6 -2
View File
@@ -13,7 +13,7 @@ import {
} from './windows-process-tree-gyp-rebuild.mjs'
const require = createRequire(import.meta.url)
const { assertNodePtyJobOwnership } = require('./node-pty-job-ownership.cjs')
const { assertNodePtyJobOwnership, nodePtyAddonPath } = require('./node-pty-job-ownership.cjs')
const { assertWindowsProcessTreeCreationTime } = require('./windows-process-tree-creation-time.cjs')
const scriptPath = import.meta.filename
const projectDir = resolve(import.meta.dirname, '../..')
@@ -298,7 +298,11 @@ function loadNodePtyNativeModule() {
// terminal is created, so require('node-pty') alone can miss ABI mismatches.
const native = loadNativeModule(nativeName)
assertNodePtyWindowsConptyRuntime(native?.dir)
assertNodePtyJobOwnership({ nativeName, native })
assertNodePtyJobOwnership({
nativeName,
native,
addonPath: nodePtyAddonPath(require.resolve('node-pty/lib/utils'), native, nativeName)
})
if (requiresPatchedNodePtySourceBuild() && !isNodePtyReleaseBuildDir(native?.dir)) {
throw new Error(
`node-pty resolved to ${native.dir}; expected build/Release so Orca's node-pty patch is active`
+82 -8
View File
@@ -1,25 +1,99 @@
'use strict'
const { readFileSync } = require('node:fs')
const { dirname, resolve } = require('node:path')
const NODE_PTY_JOB_EXPORTS = ['listJobProcessIds', 'terminateJob', 'assignCurrentProcessToJob']
function assertNodePtyJobOwnership({ nativeName, native, platform = process.platform }) {
/**
* The wide literal `usesCygwinRuntime` probes for in conpty.cc, as it sits in
* the compiled addon.
*
* Why sniff the binary rather than trust the exports: all three job exports
* predate the Cygwin/MSYS breakaway denial, so symbol presence cannot tell a
* current build from one whose per-PTY job still carries
* JOB_OBJECT_LIMIT_BREAKAWAY_OK. Measured on Windows 11: such a build passes
* every export check, reports isPtyJobOwnershipAvailable() true, and passes
* windows-pty-job.win32.test.ts 6/6, while every child of a Git Bash pane is
* created outside the pane's job and survives terminatePtyJob. See
* docs/reference/windows-msys-job-breakaway.md.
*
* Same shape as stagedRelayAddonIsUnpatched() in
* src/main/windows/windows-process-table.ts, which already tells a patched
* addon from a published one by a binary import name.
*/
const CYGWIN_BREAKAWAY_MARKER = Buffer.from('msys-2.0.dll', 'utf16le')
/**
* Absolute path of the addon `loadNativeModule` just resolved.
*
* `native.dir` is relative to node-pty's own `lib/`, which is the only base
* every caller shares -- the project install, a staged rebuild and the packaged
* resources tree all reach the addon through a different root.
*/
function nodePtyAddonPath(nodePtyUtilsPath, native, nativeName) {
return resolve(dirname(nodePtyUtilsPath), native.dir, `${nativeName}.node`)
}
function assertNodePtyJobOwnership({ nativeName, native, addonPath, platform = process.platform }) {
if (platform !== 'win32' || nativeName !== 'conpty') {
return
}
const exported = native?.module ?? native
const missing = NODE_PTY_JOB_EXPORTS.filter((name) => typeof exported?.[name] !== 'function')
if (missing.length === 0) {
if (missing.length > 0) {
throw new Error(
[
`node-pty's conpty native is missing ${missing.join(', ')}.`,
`Resolved from: ${native?.dir ?? 'unknown'}`,
'That build cannot own a PTY tree, so terminatePtyJob degrades to "unavailable"',
'and pane teardown falls back to guessing by PID ancestry.',
'Rebuild node-pty from source so config/patches/node-pty@1.1.0.patch applies.'
].join(' ')
)
}
assertCygwinBreakawayDenied(addonPath, native)
}
/**
* Why this refuses instead of skipping when the addon cannot be read: an
* unreadable binary is exactly the state that used to pass. `loadNativeModule`
* has already required this file, so "cannot read it" means the caller did not
* say which file it loaded, and a gate that cannot see its subject is not a
* gate.
*/
function assertCygwinBreakawayDenied(addonPath, native) {
let binary
try {
binary = readFileSync(addonPath)
} catch (error) {
throw new Error(
[
`Cannot read node-pty's conpty native at ${addonPath ?? '<no path given>'}`,
`(resolved from ${native?.dir ?? 'unknown'}): ${error.message}.`,
'Without the binary this cannot tell a current build from one that leaks',
'every MSYS pane child out of its job, so it refuses rather than assume.'
].join(' ')
)
}
if (binary.includes(CYGWIN_BREAKAWAY_MARKER)) {
return
}
throw new Error(
[
`node-pty's conpty native is missing ${missing.join(', ')}.`,
`Resolved from: ${native?.dir ?? 'unknown'}`,
'That build cannot own a PTY tree, so terminatePtyJob degrades to "unavailable"',
'and pane teardown falls back to guessing by PID ancestry.',
'Rebuild node-pty from source so config/patches/node-pty@1.1.0.patch applies.'
`node-pty's conpty native at ${addonPath} predates the Cygwin/MSYS job-breakaway denial.`,
'It exports the job functions, so it looks patched, but its per-PTY job still carries',
'JOB_OBJECT_LIMIT_BREAKAWAY_OK and every Git Bash child is created outside the job:',
'terminatePtyJob reports "terminated" and leaves the tree running.',
'Rebuild node-pty from source so the current config/patches/node-pty@1.1.0.patch applies',
'(a worktree sharing node_modules with its main checkout shares that stale addon).',
'See docs/reference/windows-msys-job-breakaway.md.'
].join(' ')
)
}
module.exports = { assertNodePtyJobOwnership }
module.exports = {
assertNodePtyJobOwnership,
assertCygwinBreakawayDenied,
nodePtyAddonPath
}
+10 -2
View File
@@ -550,14 +550,22 @@ function loadNativeModule(moduleName) {
}
if (moduleName === 'node-pty') {
projectRequire('node-pty')
const { assertNodePtyJobOwnership } = projectRequire(
const { assertNodePtyJobOwnership, nodePtyAddonPath } = projectRequire(
'./config/scripts/node-pty-job-ownership.cjs'
)
const { loadNativeModule } = projectRequire('node-pty/lib/utils')
const nativeName = getNodePtyNativeModuleName()
const native = loadNativeModule(nativeName)
assertNodePtyWindowsConptyRuntime(native.dir)
assertNodePtyJobOwnership({ nativeName, native })
assertNodePtyJobOwnership({
nativeName,
native,
addonPath: nodePtyAddonPath(
projectRequire.resolve('node-pty/lib/utils'),
native,
nativeName
)
})
if (requirePatchedNodePtySourceBuild && !isNodePtyReleaseBuildDir(native.dir)) {
throw new Error(
'node-pty resolved to ' +
@@ -1,11 +1,23 @@
const { existsSync } = require('node:fs')
const { createRequire } = require('node:module')
const { join } = require('node:path')
const { assertNodePtyJobOwnership } = require('./node-pty-job-ownership.cjs')
const {
assertNodePtyJobOwnership,
assertCygwinBreakawayDenied,
nodePtyAddonPath
} = require('./node-pty-job-ownership.cjs')
/** Where electron-builder lands the addon; the only layout `loadPackagedConpty` produces. */
function packagedConptyPath(resourcesDir) {
return join(resourcesDir, 'node_modules', 'node-pty', 'build', 'Release', 'conpty.node')
}
function loadPackagedConpty(resourcesDir) {
const packagedRequire = createRequire(join(resourcesDir, 'package.json'))
const { loadNativeModule } = packagedRequire('./node_modules/node-pty/lib/utils')
return loadNativeModule('conpty')
const utilsPath = packagedRequire.resolve('./node_modules/node-pty/lib/utils')
const { loadNativeModule } = packagedRequire(utilsPath)
const native = loadNativeModule('conpty')
return { native, addonPath: nodePtyAddonPath(utilsPath, native, 'conpty') }
}
function verifyPackagedNodePtyJobOwnership(resourcesDir, options = {}) {
@@ -14,12 +26,40 @@ function verifyPackagedNodePtyJobOwnership(resourcesDir, options = {}) {
return
}
const native = (options.loadNative ?? loadPackagedConpty)(resourcesDir)
assertNodePtyJobOwnership({ platform, nativeName: 'conpty', native })
const { native, addonPath } = (options.loadNative ?? loadPackagedConpty)(resourcesDir)
assertNodePtyJobOwnership({ platform, nativeName: 'conpty', native, addonPath })
if (!native.dir.replace(/\\/g, '/').includes('build/Release/')) {
throw new Error(`Packaged node-pty resolved to ${native.dir}; expected patched build/Release`)
}
console.log('[verify-packaged-node-pty] OK — packaged ConPTY owns process trees')
}
module.exports = { verifyPackagedNodePtyJobOwnership }
/**
* The half of the packaged check that survives a cross-host build.
*
* The export check has to load the addon, so it cannot run when the packaging
* host is not the target platform/arch -- and that skip is how a Windows
* release built elsewhere could ship a node-pty that leaks every MSYS pane
* child out of its job. Reading the binary needs neither.
*
* Absence is logged rather than thrown: an unrecognised layout must not fail a
* release that was packaging fine, and the export check still covers the
* same-host case. A binary that IS there and lacks the marker is fatal.
*/
function verifyPackagedConptyBreakawayMarker(resourcesDir, options = {}) {
// Deliberately no host-platform gate: the caller has already established that
// the *target* is Windows, and gating on the host is the very skip this
// closes.
const addonPath = (options.packagedConptyPath ?? packagedConptyPath)(resourcesDir)
if (!(options.exists ?? existsSync)(addonPath)) {
console.warn(
`[verify-packaged-node-pty] no addon at ${addonPath}; could not check the MSYS ` +
'job-breakaway denial for this cross-host package.'
)
return
}
assertCygwinBreakawayDenied(addonPath, { dir: addonPath })
console.log('[verify-packaged-node-pty] OK — packaged ConPTY denies MSYS job breakaway')
}
module.exports = { verifyPackagedNodePtyJobOwnership, verifyPackagedConptyBreakawayMarker }
@@ -1,11 +1,32 @@
import { mkdtempSync, writeFileSync } from 'node:fs'
import { createRequire } from 'node:module'
import { tmpdir } from 'node:os'
import { join } from 'node:path'
import { describe, expect, it, vi } from 'vitest'
const require = createRequire(import.meta.url)
const {
verifyPackagedNodePtyJobOwnership
verifyPackagedNodePtyJobOwnership,
verifyPackagedConptyBreakawayMarker
} = require('./verify-packaged-node-pty-job-ownership.cjs')
const fixtureDir = mkdtempSync(join(tmpdir(), 'packaged-node-pty-job-'))
function writeAddon(name, { cygwinBreakawayDenied }) {
const path = join(fixtureDir, name)
writeFileSync(
path,
Buffer.concat([
Buffer.from('MZ fake addon '),
cygwinBreakawayDenied ? Buffer.from('msys-2.0.dll', 'utf16le') : Buffer.alloc(0)
])
)
return path
}
const CURRENT_ADDON = writeAddon('current.node', { cygwinBreakawayDenied: true })
const PRE_MSYS_ADDON = writeAddon('pre-msys.node', { cygwinBreakawayDenied: false })
const PATCHED = {
dir: '../build/Release/',
module: {
@@ -15,31 +36,39 @@ const PATCHED = {
}
}
const packaged = (native, addonPath = CURRENT_ADDON) => ({
platform: 'win32',
loadNative: () => ({ native, addonPath })
})
describe('verifyPackagedNodePtyJobOwnership', () => {
it('accepts the packaged patched ConPTY binding', () => {
expect(() =>
verifyPackagedNodePtyJobOwnership('resources', {
platform: 'win32',
loadNative: () => PATCHED
})
).not.toThrow()
expect(() => verifyPackagedNodePtyJobOwnership('resources', packaged(PATCHED))).not.toThrow()
})
it('rejects a packaged upstream prebuild', () => {
expect(() =>
verifyPackagedNodePtyJobOwnership('resources', {
platform: 'win32',
loadNative: () => ({ dir: '../prebuilds/win32-x64/', module: {} })
})
verifyPackagedNodePtyJobOwnership(
'resources',
packaged({ dir: '../prebuilds/win32-x64/', module: {} })
)
).toThrow(/missing listJobProcessIds, terminateJob, assignCurrentProcessToJob/)
})
// A release built against a stale native cache ships the MSYS orphan bug
// while exporting every job function, so packaging has to read the binary.
it('rejects a packaged build that predates the Cygwin/MSYS breakaway denial', () => {
expect(() =>
verifyPackagedNodePtyJobOwnership('resources', packaged(PATCHED, PRE_MSYS_ADDON))
).toThrow(/predates the Cygwin\/MSYS job-breakaway denial/)
})
it('requires the patched source-build directory', () => {
expect(() =>
verifyPackagedNodePtyJobOwnership('resources', {
platform: 'win32',
loadNative: () => ({ ...PATCHED, dir: '../prebuilds/win32-x64/' })
})
verifyPackagedNodePtyJobOwnership(
'resources',
packaged({ ...PATCHED, dir: '../prebuilds/win32-x64/' })
)
).toThrow(/expected patched build\/Release/)
})
@@ -49,3 +78,51 @@ describe('verifyPackagedNodePtyJobOwnership', () => {
expect(loadNative).not.toHaveBeenCalled()
})
})
describe('verifyPackagedConptyBreakawayMarker', () => {
// This is the branch a Windows release built on another host takes, so it has
// to work without loading the addon.
it('fails a cross-host package whose addon predates the breakaway denial', () => {
expect(() =>
verifyPackagedConptyBreakawayMarker('resources', {
packagedConptyPath: () => PRE_MSYS_ADDON
})
).toThrow(/predates the Cygwin\/MSYS job-breakaway denial/)
})
it('passes a cross-host package built from current patched source', () => {
expect(() =>
verifyPackagedConptyBreakawayMarker('resources', {
packagedConptyPath: () => CURRENT_ADDON
})
).not.toThrow()
})
it('warns instead of failing a layout it does not recognise', () => {
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {})
expect(() =>
verifyPackagedConptyBreakawayMarker('resources', {
packagedConptyPath: () => join(fixtureDir, 'absent.node')
})
).not.toThrow()
expect(warn).toHaveBeenCalledWith(expect.stringContaining('could not check the MSYS'))
warn.mockRestore()
})
it('looks where electron-builder actually lands the addon', () => {
const exists = vi.fn().mockReturnValue(false)
verifyPackagedConptyBreakawayMarker(join('out', 'win-unpacked', 'resources'), { exists })
expect(exists).toHaveBeenCalledWith(
join(
'out',
'win-unpacked',
'resources',
'node_modules',
'node-pty',
'build',
'Release',
'conpty.node'
)
)
})
})