fix(unsafe-import): warning on unsafe-import using eval/require (#190)

* add unsafe-import probe for eval/require

* remove unecessary declaration & add eval/require both warning to unsafeimport probe

* update test & rename/simmplify new probe

* refacto test & rename/validate probe

* refacto update isrequire/isunsafecallee, remove new probe

* validateNode as array in isRequire probe & update test utils

* rebase refacto probe test

* update spec, update addDep, clean isRequire & isUnsafeCallee probes

* fix init dependencyAutoWarning=false & update docs

Author: tchapacan

.gitignore

@@ -106,3 +106,4 @@ dist
 temp.js
 temp/
 .vscode/
+.idea/

docs/unsafe-import.md

@@ -17,6 +17,7 @@ We analyze and trace several ways to require in Node.js (with CJS):
 - require.main.require
 - require.mainModule.require
 - require.resolve
+- `const XX = eval('require')('XX');` (dangerous import using eval)
 
 ## Example
 

src/ProbeRunner.js

@@ -42,12 +42,12 @@ export class ProbeRunner {
    * @type {Probe[]}
    */
   static Defaults = [
+    isRequire,
     isUnsafeCallee,
     isLiteral,
     isLiteralRegex,
     isRegexObject,
     isVariableDeclaration,
-    isRequire,
     isImportDeclaration,
     isMemberExpression,
     isAssignmentExpression,

src/SourceFile.js

@@ -21,6 +21,7 @@ export class SourceFile {
   inTryStatement = false;
   hasDictionaryString = false;
   hasPrefixedIdentifiers = false;
+  dependencyAutoWarning = false;
   varkinds = { var: 0, let: 0, const: 0 };
   idtypes = { assignExpr: 0, property: 0, variableDeclarator: 0, functionDeclaration: 0 };
   counter = {
@@ -52,7 +53,7 @@ export class SourceFile {
     }
   }
 
-  addDependency(name, location = null, unsafe = false) {
+  addDependency(name, location = null, unsafe = this.dependencyAutoWarning) {
     if (typeof name !== "string" || name.trim() === "") {
       return;
     }
@@ -64,6 +65,10 @@ export class SourceFile {
       inTry: this.inTryStatement,
       ...(location === null ? {} : { location })
     });
+
+    if (this.dependencyAutoWarning) {
+      this.addWarning("unsafe-import", dependencyName, location);
+    }
   }
 
   addWarning(name, value, location = rootLocation()) {

src/probes/isRequire.js

@@ -10,11 +10,9 @@ import {
   getCallExpressionIdentifier,
   getCallExpressionArguments
 } from "@nodesecure/estree-ast-utils";
-
-// Import Internal Dependencies
 import { ProbeSignals } from "../ProbeRunner.js";
 
-function validateNode(node, { tracer }) {
+function validateNodeRequire(node, { tracer }) {
   const id = getCallExpressionIdentifier(node);
   if (id === null) {
     return [false];
@@ -24,19 +22,46 @@ function validateNode(node, { tracer }) {
 
   return [
     data !== null && data.name === "require",
-    data?.identifierOrMemberExpr ?? void 0
+    id ?? void 0
+  ];
+}
+
+function validateNodeEvalRequire(node) {
+  const id = getCallExpressionIdentifier(node);
+
+  if (id !== "eval") {
+    return [false];
+  }
+  if (node.callee.type !== "CallExpression") {
+    return [false];
+  }
+
+  const args = getCallExpressionArguments(node.callee);
+
+  return [
+    args.length > 0 && args.at(0) === "require",
+    id
   ];
 }
 
+function teardown({ analysis }) {
+  analysis.dependencyAutoWarning = false;
+}
+
+
 function main(node, options) {
-  const { analysis } = options;
+  const { analysis, data: calleeName } = options;
   const { tracer } = analysis;
 
   if (node.arguments.length === 0) {
     return;
   }
   const arg = node.arguments.at(0);
 
+  if (calleeName === "eval") {
+    analysis.dependencyAutoWarning = true;
+  }
+
   switch (arg.type) {
     // const foo = "http"; require(foo);
     case "Identifier":
@@ -163,7 +188,7 @@ function walkRequireCallExpression(nodeToWalk, tracer) {
 
 export default {
   name: "isRequire",
-  validateNode,
+  validateNode: [validateNodeRequire, validateNodeEvalRequire],
   main,
   breakOnMatch: true,
   breakGroup: "import"

src/utils.js

@@ -7,15 +7,30 @@ export function notNullOrUndefined(value) {
   return value !== null && value !== void 0;
 }
 
-export function isUnsafeCallee(node) {
+function isEvalCallee(node) {
+  const identifier = getCallExpressionIdentifier(node, {
+    resolveCallExpression: false
+  });
+
+  return identifier === "eval";
+}
+
+function isFunctionCallee(node) {
   const identifier = getCallExpressionIdentifier(node);
 
-  // For Function we are looking for this: `Function("...")();`
-  // A double CallExpression
-  return [
-    identifier === "eval" || (identifier === "Function" && node.callee.type === "CallExpression"),
-    identifier
-  ];
+  return identifier === "Function" && node.callee.type === "CallExpression";
+}
+
+export function isUnsafeCallee(node) {
+  if (isEvalCallee(node)) {
+    return [true, "eval"];
+  }
+
+  if (isFunctionCallee(node)) {
+    return [true, "Function"];
+  }
+
+  return [false, null];
 }
 
 export function rootLocation() {

test/issues/179-UnsafeEvalRequire.spec.js

@@ -0,0 +1,27 @@
+// 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/179
+ */
+// CONSTANTS
+const kIncriminedCodeSample = `const stream = eval('require')('stream');`;
+const kWarningUnsafeImport = "unsafe-import";
+const kWarningUnsafeStatement = "unsafe-stmt";
+
+test("should detect unsafe-import and unsafe-statement", () => {
+  const sastAnalysis = runASTAnalysis(kIncriminedCodeSample);
+
+  assert.equal(sastAnalysis.warnings.at(0).value, "stream");
+  assert.equal(sastAnalysis.warnings.at(0).kind, kWarningUnsafeImport);
+  assert.equal(sastAnalysis.warnings.at(1).value, "eval");
+  assert.equal(sastAnalysis.warnings.at(1).kind, kWarningUnsafeStatement);
+  assert.equal(sastAnalysis.warnings.length, 2);
+  assert.equal(sastAnalysis.dependencies.has("stream"), true);
+  assert.equal(sastAnalysis.dependencies.get("stream").unsafe, true);
+  assert.equal(sastAnalysis.dependencies.size, 1);
+});