harden: add path validation in shuji.js - #243
Conversation
The application constructs output directories and writes files based on user-supplied input file paths and filenames extracted from source map content
📝 WalkthroughWalkthrough
ChangesSafe source writing
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Warning |
|
![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 `@lib/write-sources.js`:
- Around line 24-30: The path traversal check using resolved lexical paths does
not prevent symlinked directories from redirecting writes outside outdir. Update
the output-writing flow around resolvedOutdir, resolvedFilepath,
fs.ensureDirSync, and fs.writeFileSync to validate real parent paths and reject
symlink targets escaping outdir, then use a no-follow or atomic write approach
to avoid check-to-write races. Add a regression test covering a symlinked output
subdirectory.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 00ef69ea-ace2-4f51-b898-d75da80a0f87
📒 Files selected for processing (1)
lib/write-sources.js
| // Prevent path traversal: ensure the resolved output stays within outdir | ||
| const resolvedOutdir = path.resolve(outdir); | ||
| const resolvedFilepath = path.resolve(outputFilepath); | ||
| if (!resolvedFilepath.startsWith(resolvedOutdir + path.sep) && resolvedFilepath !== resolvedOutdir) { | ||
| console.error(`Skipping unsafe path "${filename}"`); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
mkdir -p "$tmp/out" "$tmp/outside"
ln -s "$tmp/outside" "$tmp/out/link"
OUTDIR="$tmp/out" OUTSIDE="$tmp/outside" node <<'NODE'
const fs = require('node:fs');
const path = require('node:path');
const outdir = process.env.OUTDIR;
const outside = process.env.OUTSIDE;
const outputFilepath = path.join(outdir, 'link', 'escaped.js');
const resolvedOutdir = path.resolve(outdir);
const resolvedFilepath = path.resolve(outputFilepath);
const accepted =
resolvedFilepath.startsWith(resolvedOutdir + path.sep) ||
resolvedFilepath === resolvedOutdir;
if (!accepted) throw new Error('Fixture did not pass the lexical check');
fs.writeFileSync(outputFilepath, 'escaped');
if (!fs.existsSync(path.join(outside, 'escaped.js'))) {
throw new Error('Symlink escape was not reproduced');
}
console.log('Symlink escape reproduced');
NODERepository: paazmaya/shuji
Length of output: 178
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^|/)(write-sources\.js|write-sources_test\.js)$' || true
echo "== lib/write-sources.js =="
if [ -f lib/write-sources.js ]; then
cat -n lib/write-sources.js
fi
echo "== tests/lib/write-sources_test.js =="
if [ -f tests/lib/write-sources_test.js ]; then
cat -n tests/lib/write-sources_test.js
fi
echo "== usages of writeSources/write-source path =="
rg -n "writeSources|write-sources|outputFilepath|outdir|fs.mkdtemp|mkdtemp|realpath|fs\\.writeFile|writeFileSync" . --glob '!node_modules' --glob '!dist' --glob '!build' || true
echo "== package node version hints =="
[ -f package.json ] && sed -n '1,120p' package.json | rg -n '"(engines|type|main|scripts)"|node:' || trueRepository: paazmaya/shuji
Length of output: 5201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package.json relevant fields =="
sed -n '1,80p' package.json | cat -n
echo "== bin/shuji.js relevant fields =="
sed -n '1,220p' bin/shuji.js | cat -n
echo "== fs-extra usage/import =="
rg -n "fs-extra|ensureDirSync|writeFileSync|realpath|mkdtemp" lib bin tests npm-shrinkwrap.json --glob '!node_modules' || true
echo "== local safe-path lexical check semantics probe =="
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
mkdir -p "$tmp/out" "$tmp/outside/subdir"
node <<'NODE'
const path = require('node:path');
const cases = [
['./out/subdir/escaped.js', './out/safe.js'],
['link/escaped.js', 'safe.js'],
['../outside/subdir/escaped.js', 'safe.js'],
['..', 'safe.js'],
];
for (const [base, expected] of cases) {
const filepath = path.join(base);
const resolvedFilepath = path.resolve(filepath);
const resolvedOutdir = path.resolve(base.endsWith('/unsafe.log') ? base.slice(0, -'/unsafe.log'.length) : base);
const accepted = resolvedFilepath.startsWith(resolvedOutdir + path.sep) || resolvedFilepath === resolvedOutdir;
console.log(JSON.stringify({ input: filepath, resolvedFilepath, accepted, expected }));
}
NODE
echo "== fs.realpath built-in availability =="
node -e "const fs=require('node:fs'); console.log(typeof fs.realpath)"Repository: paazmaya/shuji
Length of output: 8043
🌐 Web query:
Node.js path.resolve does not resolve symlinks documentation
💡 Result:
The Node.js path.resolve method does not resolve symlinks [1][2]. Its documentation defines the method as a utility that processes a sequence of paths or path segments into an absolute path by performing string manipulation and normalization (such as resolving.. and. segments) [3][4]. Because path.resolve performs these operations purely as string-based path transformations based on the current working directory, it does not interact with the actual filesystem to detect or expand symbolic links [1][2]. If you need to resolve a path while accounting for symlinks, you should use fs.realpath or fs.realpathSync [5][2], which are designed to return the canonicalized absolute path by resolving symlinks and filesystem references [2].
Citations:
- 1: Path.resolve reports the wrong path for junctioned paths in windows. nodejs/node#22592
- 2: https://stackoverflow.com/questions/26372371/resolve-filesystem-paths-possibly-with-symlinks-with-node-js
- 3: https://nodejs.org/api/path.html
- 4: https://nodejs.org/docs/latest-v23.x/api/path.html
- 5: https://nodejs.org/api/fs.html
Prevent symlink-based writing outside outdir.
path.resolve() only normalizes path segments, so filename = "link/escaped.js" can pass the check when outdir/link points outside outdir. The later fs.ensureDirSync() and fs.writeFileSync(outputFilepath, ...) can follow the link and write outside the output directory. Reject symlink targets that escape outdir and compare real-parent paths. If filesystem state can change between the check and the write, also require a no-follow or atomic write path. Add a regression test for a symlinked output subdirectory.
🤖 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 `@lib/write-sources.js` around lines 24 - 30, The path traversal check using
resolved lexical paths does not prevent symlinked directories from redirecting
writes outside outdir. Update the output-writing flow around resolvedOutdir,
resolvedFilepath, fs.ensureDirSync, and fs.writeFileSync to validate real parent
paths and reject symlink targets escaping outdir, then use a no-follow or atomic
write approach to avoid check-to-write races. Add a regression test covering a
symlinked output subdirectory.
1 participant
Summary
Harden input handling in
bin/shuji.js(flagged by multi_agent_ai).Vulnerability
V-001bin/shuji.js:107Description: The application constructs output directories and writes files based on user-supplied input file paths and filenames extracted from source map content. If a malicious source map contains path traversal sequences (e.g., '../../../etc/cron.d/backdoor') in its 'sources' field, the writeSources function may write files outside the intended output directory. The lib/write-sources.js file was not available for review, so full path sanitization cannot be confirmed.
Threat Model Context
This is a Node.js library - vulnerabilities affect downstream consumers who use this package.
Changes
lib/write-sources.jsBehavior Preservation
The change is scoped to 1 file on the vulnerable path; it only tightens handling of untrusted input and leaves valid inputs unaffected.
This patch removes an exploit primitive — a code pattern that, while not independently exploitable today, could be chained with other weaknesses by automated exploit-development tooling. Proactive removal of such primitives raises the bar against increasingly capable automated attack tools.
Automated security fix by OrbisAI Security
Summary by CodeRabbit