diff --git a/src/update/npm-invocation.mjs b/src/update/npm-invocation.mjs index 38abb375f5..71062d00ef 100644 --- a/src/update/npm-invocation.mjs +++ b/src/update/npm-invocation.mjs @@ -12,21 +12,17 @@ function escapeCmdCommand(command) { return command.replace(CMD_META, "^$1"); } -/** - * Whether a PATH entry *is* the current directory. The hijack this guards against is - * cmd.exe resolving a bare `npm` out of the directory opencodex was launched from, so - * only that exact directory has to be skipped — every candidate we hand to spawn is an - * absolute path, which is what actually defeats the implicit cwd-first search. - * - * Deliberately not a subtree test: npm's default Windows global prefix is - * `%AppData%\npm` (`C:\Users\x\AppData\Roaming\npm`), so excluding everything under the - * cwd would fail closed for anyone whose shell sits in their home directory — a normal - * setup, not the untrusted-project case this hardening is for. - */ -function isCurrentDirectory(cwd, entry) { - const left = win32.resolve(entry); - const right = win32.resolve(cwd); - return left.toLowerCase() === right.toLowerCase(); +function isInside(root, candidate) { + const relative = win32.relative(win32.resolve(root), win32.resolve(candidate)); + return relative === "" || ( + relative !== ".." + && !relative.startsWith(`..${win32.sep}`) + && !win32.isAbsolute(relative) + ); +} + +function isSamePath(left, right) { + return win32.resolve(left).toLowerCase() === win32.resolve(right).toLowerCase(); } function cleanPathEntry(entry) { @@ -43,6 +39,9 @@ export function resolveNpmCommand( if (platform !== "win32") return "npm"; const exists = deps.exists ?? existsSync; const cwd = deps.cwd ?? process.cwd(); + const appDataNpm = env.APPDATA && win32.isAbsolute(env.APPDATA) + ? win32.join(env.APPDATA, "npm") + : null; const extensions = (env.PATHEXT ?? ".COM;.EXE;.BAT;.CMD") .split(";") .filter(Boolean); @@ -53,7 +52,7 @@ export function resolveNpmCommand( for (const entry of pathEntries) { if (!win32.isAbsolute(entry)) continue; - if (isCurrentDirectory(cwd, entry)) continue; + if (isInside(cwd, entry) && (!appDataNpm || !isSamePath(entry, appDataNpm))) continue; for (const extension of extensions) { const candidate = win32.join(entry, `npm${extension.toLowerCase()}`); if (exists(candidate)) return win32.resolve(candidate); diff --git a/tests/update-npm-invocation.test.ts b/tests/update-npm-invocation.test.ts index 6a09569060..1347b586bf 100644 --- a/tests/update-npm-invocation.test.ts +++ b/tests/update-npm-invocation.test.ts @@ -45,6 +45,7 @@ describe("Windows npm update invocation", () => { const appDataNpm = `${home}\\AppData\\Roaming\\npm\\npm.cmd`; const env = { PATH: `${home}\\AppData\\Roaming\\npm`, + APPDATA: `${home}\\AppData\\Roaming`, PATHEXT: ".CMD", SystemRoot: "C:\\Windows", }; @@ -55,6 +56,28 @@ describe("Windows npm update invocation", () => { })).toBe(appDataNpm); }); + test("ignores npm candidates in current-directory subtrees", () => { + const projectNpm = `${cwd}\\node_modules\\.bin\\npm.cmd`; + const env = { + PATH: `${cwd}\\node_modules\\.bin;C:\\Program Files\\nodejs`, + PATHEXT: ".CMD", + SystemRoot: "C:\\Windows", + }; + const existing = new Set([projectNpm, trustedNpm]); + + expect(resolveNpmCommand("win32", env, { + cwd, + exists: path => existing.has(path), + })).toBe(trustedNpm); + + const invocation = npmInvocation(["install", "-g", "pkg@latest"], "win32", env, { + cwd, + exists: path => existing.has(path), + }); + expect(invocation?.args.at(-1)).toContain("nodejs\\npm.cmd"); + expect(invocation?.args.at(-1)).not.toContain("node_modules\\.bin\\npm.cmd"); + }); + test("still skips the current directory when it is a PATH entry under the home tree", () => { // The narrower rule must not lose the actual defense: a PATH entry equal to the // launch directory stays excluded even though it sits inside the user's home.