diff --git a/packages/core/src/update/command-runner.ts b/packages/core/src/update/command-runner.ts index 333adeb..1c024d5 100644 --- a/packages/core/src/update/command-runner.ts +++ b/packages/core/src/update/command-runner.ts @@ -261,7 +261,16 @@ export function findExecutable ( } const SHIM_INVOCATION_RE = /(?:^|[&;])\s*@?(?:"(?:%_prog%|%dp0%\\node\.exe|node(?:\.exe)?)"|node(?:\.exe)?)\s+/i -const SHIM_ENTRYPOINT_RE = /node_modules[\\/]([^"\r\n']+?\.(?:js|cjs))/i +/** + * Capture the quoted `%dp0%`-relative entrypoint target of an invocation line. + * npm's cmd-shim writes the target relative to the shim directory in two + * layouts: `%dp0%\node_modules\\...` for a shim that sits next to + * `node_modules` (the bundled `npm.cmd`/`npx.cmd`), and `%dp0%\..\...` + * for a shim inside `node_modules\.bin` (every dependency bin, and pnpm when + * it was installed through npm). Legacy shims use `%~dp0\` instead of + * `%dp0%\`. + */ +const SHIM_TARGET_RE = /%(?:dp0%|~dp0)\\([^"\r\n]+?\.(?:js|cjs))(?=")/i /** * Parse an npm-generated Windows `.cmd`/`.bat` shim and return the absolute @@ -298,24 +307,23 @@ export function deriveShimEntrypoint (shimPath: string, platform: NodeJS.Platfor return undefined } const isWindows = platform === 'win32' + const shimDir = path.win32.dirname(shimPath) const shimName = path.win32.basename(shimPath, path.win32.extname(shimPath)) const namesEqual = (left: string, right: string): boolean => isWindows ? left.toLowerCase() === right.toLowerCase() : left === right const invocationLines = content.split(/\r?\n/).filter((line) => SHIM_INVOCATION_RE.test(line) && /%\*/.test(line)) for (const invocation of invocationLines) { - if (!/node_modules[\\/]/.test(invocation) || !/\.(?:js|cjs)["\s]/i.test(invocation)) continue - // Match a `node_modules\<...>...\*.js|cjs` target (npm's cmd-shim emits the - // entrypoint relative to the shim directory via %dp0%). Since every - // character inside the entrypoint path is constrained to a Windows path we - // strip quotes and whitespace around it. - const match = SHIM_ENTRYPOINT_RE.exec(invocation) + // Extract the quoted %dp0%-relative `.js|cjs` target npm's cmd-shim emits + // on the invocation line, then anchor it under the node_modules root the + // shim's layout implies. + const match = SHIM_TARGET_RE.exec(invocation) if (!match) continue - const relative = match[1] - if (relative.length === 0 || /\.\./.test(relative)) continue - const packageEntry = verifyPackageBinOwnership(shimPath, shimName, relative, namesEqual, isWindows) + const resolved = resolveShimTargetRoot(shimDir, match[1]) + if (!resolved) continue + const packageEntry = verifyPackageBinOwnership(resolved.modulesRoot, resolved.inModules, shimName, namesEqual) if (!packageEntry) continue const entrypoint = path.win32 - .resolve(path.win32.dirname(shimPath), 'node_modules', relative) + .resolve(resolved.modulesRoot, ...resolved.inModules) .split('\\') .join(path.sep) if (!existsSync(entrypoint)) continue @@ -325,28 +333,54 @@ export function deriveShimEntrypoint (shimPath: string, platform: NodeJS.Platfor } /** - * Prove that the package containing `relative` (a path under - * `node_modules\...` captured from a shim invocation line) declares a `bin` + * Anchor a shim's captured `%dp0%`-relative target under the node_modules root + * its layout implies: a shim next to `node_modules` references + * `node_modules\\...`, while a shim inside `node_modules\.bin` + * references `..\\...` one level up. Any traversal beyond that root (or + * any other shape) is rejected, so the derived entrypoint always stays inside + * the shim's own node_modules tree. + */ +function resolveShimTargetRoot ( + shimDir: string, + relative: string +): { modulesRoot: string; inModules: string[] } | undefined { + const segments = relative.replace(/\//g, '\\').split('\\') + const isSafe = (rest: string[]): boolean => + rest.length > 0 && rest.every((segment) => segment.length > 0 && segment !== '.' && segment !== '..') + if (segments[0]?.toLowerCase() === 'node_modules') { + const inModules = segments.slice(1) + if (!isSafe(inModules)) return undefined + return { modulesRoot: path.win32.resolve(shimDir, 'node_modules'), inModules } + } + if (segments[0] === '..' && path.win32.basename(shimDir).toLowerCase() === '.bin') { + const inModules = segments.slice(1) + if (!isSafe(inModules)) return undefined + return { modulesRoot: path.win32.dirname(shimDir), inModules } + } + return undefined +} + +/** + * Prove that the package at `inModules` (segments under the shim's + * node_modules root, captured from a shim invocation line) declares a `bin` * named after the shim and pointing at that exact entrypoint. Returns the bin * entry name on success, undefined on any failure (fail-closed). */ function verifyPackageBinOwnership ( - shimPath: string, + modulesRoot: string, + inModules: readonly string[], shimName: string, - relative: string, - namesEqual: (left: string, right: string) => boolean, - isWindows: boolean + namesEqual: (left: string, right: string) => boolean ): string | undefined { - const segments = relative.split(/[\\/]+/) - if (segments.length === 0 || segments[0].length === 0) return undefined + if (inModules.length === 0 || inModules[0].length === 0) return undefined // The package directory is one segment, or two for a scoped package. - const packageSegments = segments[0].startsWith('@') ? 2 : 1 - if (segments.length <= packageSegments) return undefined - const packageDir = segments.slice(0, packageSegments).join('\\') - const inPackageRelative = segments.slice(packageSegments).join('\\') + const packageSegments = inModules[0].startsWith('@') ? 2 : 1 + if (inModules.length <= packageSegments) return undefined + const packageDir = inModules.slice(0, packageSegments).join('\\') + const inPackageRelative = inModules.slice(packageSegments).join('\\') const manifestPath = path.win32 - .resolve(path.win32.dirname(shimPath), 'node_modules', packageDir, 'package.json') + .resolve(modulesRoot, packageDir, 'package.json') .split('\\') .join(path.sep) let manifest: unknown diff --git a/packages/core/src/update/fallback-result-protocol.ts b/packages/core/src/update/fallback-result-protocol.ts index 2696f54..0b5a18a 100644 --- a/packages/core/src/update/fallback-result-protocol.ts +++ b/packages/core/src/update/fallback-result-protocol.ts @@ -3,15 +3,20 @@ import { chmod, lstat, open, rename, rm, writeFile } from 'node:fs/promises' import { randomUUID } from 'node:crypto' import path from 'node:path' -/** O_NOFOLLOW is unavailable on some platforms (Windows); fall back to a plain open there. */ +/** + * O_NOFOLLOW is unavailable on Windows (the flag degrades to a plain open + * there), so readValidatedFallbackChildResult pairs the flag with an lstat + * symlink rejection plus an fd-vs-path identity comparison that work on + * every platform. + */ const NO_FOLLOW_FLAG = (constants as { O_NOFOLLOW?: number }).O_NOFOLLOW ?? 0 /** * Identity of a parent-owned containment directory, recorded at creation time * (before any other process can observe or replace the path). POSIX carries - * device+inode; Windows filesystems expose neither, so the recorded identity - * is empty there and containment relies on the inherited ACLs of the private - * temporary workspace plus the other trust boundaries. + * device+inode; on Windows Node's fs exposes the volume serial number as + * `dev` and the NTFS file index as `ino`, so the identity is recorded on + * every platform and a swapped directory is detectable everywhere. */ export interface ContainmentDirectoryIdentity { /** Directory path recorded by the parent (absolute). */ @@ -30,9 +35,6 @@ export interface ContainmentDirectoryIdentity { */ export async function recordContainmentDirectoryIdentity (directory: string): Promise { const resolved = path.resolve(directory) - if (process.platform === 'win32') { - return { directory: resolved } - } try { const stat = await lstat(resolved) return { directory: resolved, dev: stat.dev, ino: stat.ino } @@ -51,9 +53,10 @@ export function containmentDirectoryMatches (recorded: ContainmentDirectoryIdent const resolved = path.resolve(currentDirectory) if (path.resolve(recorded.directory) !== resolved) return false if (recorded.dev === undefined || recorded.ino === undefined) { - // Windows (or an unmeasurable POSIX identity): containment is enforced - // lexically only, as before. - return true + // An unmeasurable identity (lstat failed at recording time, or a legacy + // persisted identity) can never be re-verified, so containment fails + // closed instead of degrading to a lexical-only check. + return false } try { const stat = lstatSync(resolved) @@ -204,14 +207,16 @@ export async function readValidatedFallbackChildResult ( // result path still lexically matches, closing the ancestor-swap escape. const recorded = directories.find((identity) => path.resolve(identity.directory) === path.resolve(lexicalParent)) if (!recorded || !containmentDirectoryMatches(recorded, lexicalParent)) return undefined - // Open the expected path exactly once with no-follow semantics: on POSIX - // O_NOFOLLOW makes a symlink planted at the path fail instead of being - // followed. Every further check (file kind, ownership, mode, size, bytes) - // inspects only this already-open descriptor, so swapping the path after - // validation cannot change what this call reads. Windows has no - // O_NOFOLLOW equivalent and does not expose POSIX ownership/mode bits - // here; isolation rests on inherited temporary-directory ACLs plus the + // Windows has no O_NOFOLLOW, so a symlink planted at the result path is + // rejected before the open, and the opened descriptor is then verified to + // still describe the same file the pre-open lstat saw (fd-vs-path + // identity), which also closes the swap-between-lstat-and-open window on + // every platform. On POSIX O_NOFOLLOW additionally makes the open itself + // fail on a symlink. The ownership/mode checks below are POSIX-only; + // Windows isolation rests on inherited temporary-directory ACLs plus the // containment and nonce validation. + const preOpen = lstatSync(resultPath) + if (preOpen.isSymbolicLink()) return undefined const handle = await open(resultPath, constants.O_RDONLY | NO_FOLLOW_FLAG) let text: string try { @@ -222,6 +227,7 @@ export async function readValidatedFallbackChildResult ( if (!containmentDirectoryMatches(recorded, lexicalParent)) return undefined const stat = await handle.stat() if (!stat.isFile()) return undefined + if (stat.ino !== preOpen.ino || stat.dev !== preOpen.dev) return undefined // Ownership assumption: the envelope must have been written by this user. if (typeof process.getuid === 'function' && stat.uid !== process.getuid()) return undefined // The result is private state: reject anything group- or world-readable diff --git a/packages/core/src/update/native-payload.ts b/packages/core/src/update/native-payload.ts index c25152e..812f827 100644 --- a/packages/core/src/update/native-payload.ts +++ b/packages/core/src/update/native-payload.ts @@ -270,13 +270,40 @@ function captureArchivePayload (compressedArchive: Buffer, scope: ArchivePayload } } +/** + * Windows readlink returns the stored symlink target with native separators + * while tar linknames keep POSIX separators. Separators are normalized only + * for ordinary Win32 paths, where `\` and `/` are interchangeable. + * Device-namespace targets (`\\?\`, `\\.\`, `\??\`, including + * slash-spelled variants) are digested verbatim: extended paths disable + * Win32 normalization, so forward slashes inside them are not equivalent + * separators and must not be rewritten. Distinct targets can never collide + * on POSIX, where the target is digested verbatim. + */ +function symlinkTargetForDigest (target: string): string { + if (process.platform !== 'win32') return target + if (isWindowsDeviceNamespaceTarget(target)) return target + return target.replace(/\\/g, '/') +} + +/** + * True for Win32 device-namespace paths (`\\?\`, `\\.\`, `\??\`) in + * either separator spelling. Forward slashes inside these paths are not + * equivalent separators, so such targets must never be normalized. + * Exported for unit testing the classification; the digest is the only caller. + */ +export function isWindowsDeviceNamespaceTarget (target: string): boolean { + const normalized = target.replace(/\//g, '\\') + return normalized.startsWith('\\\\?\\') || normalized.startsWith('\\\\.\\') || normalized.startsWith('\\??\\') +} + function digestEntries (entries: Map): string | undefined { if (entries.size === 0) return undefined const hash = createHash('sha256') for (const [relative, entry] of [...entries].sort(([left], [right]) => left.localeCompare(right))) { hash.update(entry.kind).update('\0').update(relative).update('\0') if (entry.kind === 'file') hash.update(String(entry.content.length)).update('\0').update(entry.content) - else if (entry.kind === 'symlink') hash.update(entry.target) + else if (entry.kind === 'symlink') hash.update(symlinkTargetForDigest(entry.target)) hash.update('\0') } return hash.digest('hex') diff --git a/packages/core/test/unit/update/command-runner.test.ts b/packages/core/test/unit/update/command-runner.test.ts index 3ca94e2..b63682c 100644 --- a/packages/core/test/unit/update/command-runner.test.ts +++ b/packages/core/test/unit/update/command-runner.test.ts @@ -204,6 +204,65 @@ describe('update command runner', () => { } }) + it('derives the entrypoint of a node_modules/.bin cmd shim (npm dependency layout, cross-platform)', () => { + const root = mkdtempSync(path.join(os.tmpdir(), 'nsolid-plugin-shim-bin-')) + const entrypoint = path.join(root, 'node_modules', 'pnpm', 'bin', 'pnpm.cjs') + const shim = path.join(root, 'node_modules', '.bin', 'pnpm.cmd') + mkdirSync(path.dirname(entrypoint), { recursive: true }) + mkdirSync(path.dirname(shim), { recursive: true }) + writeFileSync(entrypoint, '#!/usr/bin/env node\n') + writeFileSync(path.join(root, 'node_modules', 'pnpm', 'package.json'), JSON.stringify({ name: 'pnpm', bin: { pnpm: 'bin/pnpm.cjs' } })) + // npm's cmd-shim writes the target relative to the shim directory, which + // for a node_modules/.bin shim is one `..` level up (this is the layout + // pnpm/action-setup produces on Windows runners, where only pnpm.cmd — + // no pnpm.exe — exists on PATH). + writeFileSync(shim, [ + '@ECHO off', + 'GOTO start', + ':find_dp0', + 'SET dp0=%~dp0', + 'EXIT /b', + ':start', + 'SETLOCAL', + 'CALL :find_dp0', + '', + 'IF EXIST "%dp0%\\node.exe" (', + ' SET "_prog=%dp0%\\node.exe"', + ') ELSE (', + ' SET "_prog=node"', + ' SET PATHEXT=%PATHEXT:;.JS;=;%', + ')', + '', + 'endLocal & goto #_undefined_# 2>NUL || title %COMSPEC% & set PATHEXT=%PATHEXT:;.JS;=;% & "%_prog%" "%dp0%\\..\\pnpm\\bin\\pnpm.cjs" %*', + '', + ].join('\r\n')) + try { + assert.equal(deriveShimEntrypoint(shim, 'win32'), entrypoint) + assert.deepEqual(resolveExecutableIdentity('pnpm', { Path: path.join(root, 'node_modules', '.bin'), PATHEXT: '.cmd' }, 'win32'), { + kind: 'node', + executable: process.execPath, + entrypoint, + }) + } finally { + rmSync(root, { recursive: true, force: true }) + } + }) + + it('rejects a .bin cmd shim whose target escapes the node_modules root', () => { + const root = mkdtempSync(path.join(os.tmpdir(), 'nsolid-plugin-shim-bin-')) + const evil = path.join(root, 'evil', 'dummy.js') + const shim = path.join(root, 'node_modules', '.bin', 'pnpm.cmd') + mkdirSync(path.dirname(evil), { recursive: true }) + mkdirSync(path.dirname(shim), { recursive: true }) + writeFileSync(evil, '#!/usr/bin/env node\n') + writeFileSync(shim, 'endLocal & goto #_undefined_# 2>NUL || title %COMSPEC% & "%_prog%" "%dp0%\\..\\..\\evil\\dummy.js" %*\r\n') + try { + assert.equal(deriveShimEntrypoint(shim, 'win32'), undefined) + } finally { + rmSync(root, { recursive: true, force: true }) + } + }) + it('derives a scoped package shim (bin object) declared by the owning package (cross-platform)', () => { const root = mkdtempSync(path.join(os.tmpdir(), 'nsolid-plugin-shim-')) const entrypoint = path.join(root, 'node_modules', '@anthropic-ai', 'claude-code', 'cli.js') diff --git a/packages/core/test/unit/update/fallback-result-protocol.test.ts b/packages/core/test/unit/update/fallback-result-protocol.test.ts index 517b630..661053f 100644 --- a/packages/core/test/unit/update/fallback-result-protocol.test.ts +++ b/packages/core/test/unit/update/fallback-result-protocol.test.ts @@ -38,7 +38,6 @@ describe('fallback child result protocol', () => { /** Identity of the beforeEach-created directory, mirroring production recording. */ function containment (): ContainmentDirectoryIdentity { - if (process.platform === 'win32') return { directory: path.resolve(directory) } const stat = lstatSync(path.resolve(directory)) return { directory: path.resolve(directory), dev: stat.dev, ino: stat.ino } } diff --git a/packages/core/test/unit/update/native-payload.test.ts b/packages/core/test/unit/update/native-payload.test.ts index 589a8a9..0058afe 100644 --- a/packages/core/test/unit/update/native-payload.test.ts +++ b/packages/core/test/unit/update/native-payload.test.ts @@ -4,7 +4,7 @@ import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node import os from 'node:os' import path from 'node:path' import { gzipSync } from 'node:zlib' -import { gitArchivePayloadDigest, nativePayloadTreeDigest, plannedPayloadIdentityFromArchive, plannedPayloadIdentityFromTree } from '../../../src/update/native-payload.js' +import { gitArchivePayloadDigest, isWindowsDeviceNamespaceTarget, nativePayloadTreeDigest, plannedPayloadIdentityFromArchive, plannedPayloadIdentityFromTree } from '../../../src/update/native-payload.js' const CODEX_PROFILE = 'codex-installed-v1' as const @@ -246,6 +246,42 @@ describe('native payload identity', () => { } }) + it('normalizes Windows symlink target separators without collapsing device-prefixed forms', { skip: process.platform !== 'win32' }, () => { + const digestWithTarget = (target: string): string | undefined => { + const root = mkdtempSync(path.join(os.tmpdir(), 'nsolid-native-win-target-')) + try { + materializePayload(root, cleanPayloadFiles()) + // Explicit 'file' type: an omitted type makes symlinkSync stat the + // target to guess file-vs-dir, and a UNC target stat surfaces as + // UNKNOWN instead of the tolerated ENOENT on Windows. + symlinkSync(target, path.join(root, 'skills', 'example', 'asset.bin'), 'file') + return nativePayloadTreeDigest(root) + } finally { + rmSync(root, { recursive: true, force: true }) + } + } + // Forward- and backslash-separated forms resolve identically on Windows. + assert.equal(digestWithTarget('../shared/asset.bin'), digestWithTarget('..\\shared\\asset.bin')) + // Same for absolute UNC paths outside the device namespace. + assert.equal(digestWithTarget('\\\\server\\share\\asset.bin'), digestWithTarget('//server/share/asset.bin')) + // Genuinely different destinations stay distinct. (Absolute device-namespace + // spellings are intentionally not compared here: Windows readlink + // canonicalizes absolute targets to NT form on read, so distinct spellings + // of the same destination digest identically — which is correct. The + // device-namespace classification itself is pinned by the cross-platform + // unit test below.) + assert.notEqual(digestWithTarget('C:\\payload\\asset.bin'), digestWithTarget('C:\\payload\\other.bin')) + }) + + it('classifies Windows device-namespace symlink targets for verbatim digesting (cross-platform)', () => { + for (const device of ['\\\\?\\C:\\x', '//?/C:/x', '\\\\.\\COM1', '\\??\\C:\\x', '\\\\?\\UNC\\s\\s']) { + assert.equal(isWindowsDeviceNamespaceTarget(device), true, device) + } + for (const ordinary of ['..\\a\\b', '../a/b', 'C:\\a\\b', '\\\\server\\share\\x', 'asset.bin']) { + assert.equal(isWindowsDeviceNamespaceTarget(ordinary), false, ordinary) + } + }) + it('keeps reserved-name symlinks and directories significant on the installed side', () => { const root = mkdtempSync(path.join(os.tmpdir(), 'nsolid-native-f4-reserved-kind-')) try { diff --git a/packages/core/test/unit/update/plan-projection.test.ts b/packages/core/test/unit/update/plan-projection.test.ts index 773eabf..5496ba1 100644 --- a/packages/core/test/unit/update/plan-projection.test.ts +++ b/packages/core/test/unit/update/plan-projection.test.ts @@ -66,13 +66,16 @@ describe('public plan step projection', () => { const projected = publicPlanSteps(steps) const command = projected[0] assert.ok(command?.kind === 'command') + // Redaction normalizes to forward slashes on every platform + // (plan-projection.ts), so the expectations use literals rather than + // path.join, which would emit native separators on Windows. assert.deepEqual(command.args, [ - 'exec', '--yes', `--package=${path.join('', 'nsolid-plugin-1.0.1.tgz')}`, - '--', 'nsolid-plugin-refresh-owned', '--transaction', path.join('', 'transaction.json'), + 'exec', '--yes', '--package=/nsolid-plugin-1.0.1.tgz', + '--', 'nsolid-plugin-refresh-owned', '--transaction', '/transaction.json', ]) const filesystem = projected[1] assert.ok(filesystem?.kind === 'filesystem') - assert.deepEqual(filesystem.paths, [path.join('', 'skill.md'), '/home/user/.config/opencode/skills/kept']) + assert.deepEqual(filesystem.paths, ['/skill.md', '/home/user/.config/opencode/skills/kept']) }) it('projects validation steps verbatim', () => {