[WRONG BRANCH] fix(update): reject project-local npm commands - #61
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
changed the title
⏳ DRAFT
What to do
Its title has been prefixed with |
📝 WalkthroughWalkthroughWindows npm resolution now excludes PATH entries inside the current working directory. It allows ChangesWindows npm PATH filtering
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/update/npm-invocation.mjs`:
- Around line 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.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c6004453-1722-4211-8c67-da1642505158
📒 Files selected for processing (2)
src/update/npm-invocation.mjstests/update-npm-invocation.test.ts
| 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(); | ||
| } |
There was a problem hiding this comment.
🔒 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 || trueRepository: 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
doneRepository: 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:
- 1: https://github.com/oven-sh/bun/blob/88417471/src/js/node/fs.ts
- 2: windows: run Bun inside an AppContainer (lowbox token) oven-sh/bun#33119
- 3: ardenden/ts-semver-checks@676be7b
- 4: Windows: fs.realpathSync fails with ENOENT when CWD is an NTFS junction can1357/oh-my-pi#935
- 5: bundler: canonicalize the asset source directory for [dir] only when string relativization falls outside root oven-sh/bun#34558
- 6: Bun v1.1.13 Windows x86_64 crashes when being called by another program using UNC paths oven-sh/bun#11953
- 7: install: remove overly strict bin linking assertions oven-sh/bun#29011
🏁 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 240Repository: 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) }));
}
JSRepository: 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:
- 1: https://github.com/oven-sh/bun/blob/main/src/js/node/fs.ts
- 2: https://github.com/oven-sh/bun/blob/88417471/src/js/node/fs.ts
- 3: https://github.com/oven-sh/bun/blob/1cc83768/src/bun.js/node/node_fs_binding.zig
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
1 participant
Motivation
npm.cmdfrom attacker-controlled project-local PATH entries (for examplenode_modules\\.bin) during update/version checks and installs.%APPDATA%\\npmglobal prefix when the current working directory is an ancestor.Description
isInside()subtree test and addisSamePath()plusappDataNpmdetection to distinguish the AppData npm prefix from untrusted project subtrees.resolveNpmCommandto skip absolute PATH entries that are inside the launchcwd(unless the entry is the AppData npm prefix) and otherwise resolve the first existingnpm*candidate as before.npmInvocationunchanged and add a focused regression test to ensure project-localnode_modules\\.bin\\npm.cmdis ignored in favor of a trusted absolute PATH candidate.Testing
bun test tests/update-npm-invocation.test.tsand the file's tests all passed.bun run typecheckandbun run privacy:scan, both of which succeeded.bun run test; the targeted regression passed, but the full run could not complete due to unrelated GUI dependency fetch errors and timeouts in other integration tests.Summary by CodeRabbit
%APPDATA%\npmlocation when configured with an absolute path.