Skip to content

build path.join called in require if args are string literals #212

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.

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

Merged
merged 5 commits into from
Jan 20, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
24 changes: 20 additions & 4 deletions src/probes/isRequire.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
getCallExpressionArguments
} from "@nodesecure/estree-ast-utils";
import { ProbeSignals } from "../ProbeRunner.js";
import path from "path";

function validateNodeRequire(node, { tracer }) {
const id = getCallExpressionIdentifier(node, {
Expand Down Expand Up @@ -120,10 +121,12 @@ function main(node, options) {

// require(Buffer.from("...", "hex").toString());
case "CallExpression": {
walkRequireCallExpression(arg, tracer)
.forEach((depName) => analysis.addDependency(depName, node.loc, true));
const { dependencies, triggerWarning } = walkRequireCallExpression(arg, tracer);
dependencies.forEach((depName) => analysis.addDependency(depName, node.loc, true));

analysis.addWarning("unsafe-import", null, node.loc);
if (triggerWarning) {
analysis.addWarning("unsafe-import", null, node.loc);
}

// We skip walking the tree to avoid anymore warnings...
return ProbeSignals.Skip;
Expand All @@ -136,6 +139,7 @@ function main(node, options) {

function walkRequireCallExpression(nodeToWalk, tracer) {
const dependencies = new Set();
let triggerWarning = true;

walk(nodeToWalk, {
enter(node) {
Expand Down Expand Up @@ -181,11 +185,23 @@ function walkRequireCallExpression(nodeToWalk, tracer) {
}
break;
}

case "path.join": {
if (!node.arguments.every((arg) => arg.type === "Literal" && typeof arg.value === "string")) {
break;
}

const constructedPath = path.posix.join(...node.arguments.map((arg) => arg.value));
dependencies.add(constructedPath);

triggerWarning = false;
break;
}
}
}
});

return [...dependencies];
return { dependencies, triggerWarning };
}

export default {
Expand Down
38 changes: 38 additions & 0 deletions test/issues/178-path-join-literal-args-is-not-unsafe.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Import Node.js Dependencies
import { test } from "node:test";
import assert from "node:assert";

// Import Internal Dependencies
import { runASTAnalysis } from "../../index.js";

/**
* @see https://github.com/NodeSecure/js-x-ray/issues/178
*/
const validTestCases = [
"const bin = require(path.join('..', './bin.js'));",
"const bin = require.resolve(path.join('..', './bin.js'));"
];

test("should not detect unsafe-import for path.join if every argument is a string literal", () => {
validTestCases.forEach((test) => {
const { warnings, dependencies } = runASTAnalysis(test);

assert.strictEqual(warnings.length, 0);
assert.ok(dependencies.has("../bin.js"));
});
});

const invalidTestCases = [
"const bin = require(path.join(__dirname, '..', './bin.js'));",
"const bin = require(path.join(3, '..', './bin.js'));",
"const bin = require.resolve(path.join(__dirname, '..', './bin.js'));",
"const bin = require.resolve(path.join(3, '..', './bin.js'));"
];

test("should detect unsafe-import of path.join if not every argument is a string literal", () => {
invalidTestCases.forEach((test) => {
const { warnings } = runASTAnalysis(test);

assert.strictEqual(warnings.length, 1);
});
});