Skip to content

[WRONG BRANCH] fix(update): reject project-local npm commands #61

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
luvs01 wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

[WRONG BRANCH] fix(update): reject project-local npm commands #61

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 15 additions & 16 deletions src/update/npm-invocation.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Comment on lines +15 to 26

@coderabbitai coderabbitai Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -eu
file=$(git ls-files 'src/update/npm-invocation.mjs' | head -n 1)
printf '%s\n' "FILE=$file"
wc -l "$file"
printf '%s\n' '--- outline ---'
ast-grep outline "$file" --lang javascript || true
printf '%s\n' '--- relevant source ---'
cat -n "$file" | sed -n '1,120p'
printf '%s\n' '--- related references ---'
rg -n --glob '!node_modules' --glob '!dist' --glob '!build' 'isInside|isSamePath|APPDATA|npm-invocation|npm\.cmd|PATH' src package.json bun.lockb bun.lock 2>/dev/null || true
printf '%s\n' '--- runtime/tool availability ---'
command -v bun || true
bun --version 2>/dev/null || true

Repository: luvs01/opencodex

Length of output: 29297

🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- update tests and package metadata ---'
git ls-files | rg '(^|/)(test|tests|spec|__tests__)/|npm-invocation|package\.json$|bunfig|tsconfig'
printf '%s\n' '--- npm invocation references and tests ---'
rg -n -C 5 'resolveNpmCommand|npmInvocation|trusted PATH|APPDATA|node_modules.*bin' --glob '!node_modules' --glob '!dist' --glob '!build' .
printf '%s\n' '--- relevant updater call sites ---'
cat -n src/update/index.ts | sed -n '100,205p'
printf '%s\n' '--- existing path canonicalization patterns ---'
rg -n -C 4 'realpathSync|realpath|native\(' src --glob '*.ts' --glob '*.mjs' --glob '*.js'
printf '%s\n' '--- package/runtime declarations ---'
for f in package.json bunfig.toml tsconfig.json; do
  if [ -f "$f" ]; then echo "--- $f"; cat -n "$f" | sed -n '1,180p'; fi
done

Repository: luvs01/opencodex

Length of output: 50373

🌐 Web query:

Bun documentation realpathSync native node:fs support Windows symlink junction 8.3 path canonicalization

💡 Result:

Bun's implementation of node:fs provides full support for Windows symlinks, junctions, and 8.3 path canonicalization, though the behavior depends on the specific method used [1][2]. To achieve reliable path canonicalization on Windows—including resolving NTFS junctions, symlinks, and expanding 8.3 "short" path names (e.g., PROGRA~1) to their full, long-form equivalents—you should use fs.realpathSync.native (or fs.realpath.native) [3][1][2]. Key implementation details: 1. Native Resolution: fs.realpathSync.native and its asynchronous counterpart leverage the Win32 GetFinalPathNameByHandle API [4][5][3]. This native approach is required to correctly "see through" NTFS junctions and map 8.3 short names to their canonical long names [1][2]. 2. Compatibility: While the standard, pure-JavaScript fs.realpathSync exists for Node.js compatibility, it historically has limitations regarding complex Windows reparse points (like junctions) compared to the native binding [4][1]. Bun's implementation ensures realpathSync.native effectively handles these Windows-specific filesystem structures [2]. 3. Canonicalization: Because GetFinalPathNameByHandle is used, the returned paths may include the Windows extended-length path prefix (\\?\), which allows for long path support but requires that consumers handle this format correctly if passing paths to other applications [5][6]. 4. Best Practice: If your application requires robust path comparison or identity verification on Windows, prioritize fs.realpathSync.native over fs.realpathSync or path.resolve [4][3]. Bun maintains these behaviors to ensure that filesystem-dependent operations (such as bundler asset resolution and package installation) remain consistent even when complex filesystem links are involved [5][7].

Citations:

🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- focused npm invocation tests ---'
cat -n tests/update-npm-invocation.test.ts | sed -n '1,140p'
printf '%s\n' '--- package metadata ---'
if [ -f package.json ]; then sed -n '1,220p' package.json; fi
printf '%s\n' '--- focused updater call graph ---'
cat -n src/update/index.ts | sed -n '45,70p;120,205p'
printf '%s\n' '--- canonicalization usage in source ---'
rg -n -C 3 --glob 'src/**' 'realpathSync|realpath\.native|realpathSync\.native'
printf '%s\n' '--- Bun references and version constraints ---'
rg -n -C 3 --glob '!node_modules' --glob '!dist' --glob '!build' 'Bun|bun [0-9]|engines|runtime' package.json README.md src/update src/lib tests 2>/dev/null | head -n 240

Repository: luvs01/opencodex

Length of output: 14063

🏁 Script executed:

#!/bin/bash
set -eu
node - <<'JS'
const { win32 } = require("node:path");

function isInside(root, candidate) {
  const relative = win32.relative(win32.resolve(root), win32.resolve(candidate));
  return relative === "" || (
    relative !== ".." &&
    !relative.startsWith(`..${win32.sep}`) &&
    !win32.isAbsolute(relative)
  );
}

const cwd = "C:\\work\\repo";
const cases = [
  ["normal project bin", "C:\\work\\repo\\node_modules\\.bin"],
  ["junction alias to project bin (lexical path)", "C:\\Users\\Public\\repo-bin"],
  ["8.3 alias to project bin (lexical path)", "C:\\PROGRA~1\\repo-bin"],
  ["APPDATA npm", "C:\\Users\\alice\\AppData\\Roaming\\npm"],
];
for (const [label, entry] of cases) {
  console.log(JSON.stringify({ label, entry, insideLexically: isInside(cwd, entry) }));
}
JS

Repository: luvs01/opencodex

Length of output: 579

🌐 Web query:

Bun official docs node:fs realpathSync.native Windows support

💡 Result:

Bun supports fs.realpathSync.native on Windows [1][2]. While Bun generally aims to emulate Node.js behavior, its implementation of fs.realpathSync.native on Windows is specifically designed to provide compatibility. Internally, Bun's implementation for Windows aliases realpathSync.native to its internal fs.realpathNativeSync function [1][3][2]. According to Bun's source code, the implementation for realpathSync on Windows explicitly distinguishes itself from non-Windows platforms to handle path resolution correctly [1][2]. Specifically, Bun's internal code notes that while the standard fs.realpathSync (which is often ported 1:1 from Node.js) may be unable to resolve certain Windows-specific path structures like subst drives, the "native" version (fs.realpathSync.native) is specifically intended to provide the underlying location resolution capability [1][2]. In summary, fs.realpathSync.native is available and supported in Bun on Windows [1][2].

Citations:

Canonicalize Windows PATH directories before applying the trust boundary.

At src/update/npm-invocation.mjs:15-26, win32.resolve() and win32.relative() normalize text only. A junction, symlink, or 8.3 alias can make a PATH entry appear outside cwd while it targets cwd\node_modules\.bin; line 55 then accepts and executes its npm.cmd. Use Bun-supported realpathSync.native to canonicalize cwd, each existing PATH directory, and APPDATA\npm before isInside() and isSamePath(). Fail closed if canonicalization fails. Add a Windows regression test for an alias path.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/update/npm-invocation.mjs` around lines 15 - 26, Canonicalize Windows
paths before trust checks: update the PATH filtering flow and its
isInside/isSamePath inputs to use Bun-supported realpathSync.native for cwd,
each existing PATH directory, and APPDATA\npm. Treat any canonicalization
failure as untrusted and skip execution, while preserving existing path
comparisons after canonicalization. Add a Windows regression test covering a
junction, symlink, or 8.3 alias that resolves into cwd\node_modules\.bin.

Source: Path instructions


function cleanPathEntry(entry) {
Expand All @@ -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);
Expand All @@ -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);
Expand Down
23 changes: 23 additions & 0 deletions tests/update-npm-invocation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
};
Expand All @@ -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.
Expand Down
Loading