refactor(probe/isRequire): use new class RequireCallExpressionWalker (#…

…231) * refactor: create and use RequireCallExpressionWalker class in isRequire probe * test: should detect unsafe-import when detect obfuscated atob value
NodeSecure · Feb 2, 2024 · 045f704 · 045f704
1 parent e711bb2
commit 045f704
Show file tree

Hide file tree

Showing 4 changed files with 104 additions and 83 deletions.
diff --git a/src/ProbeRunner.js b/src/ProbeRunner.js
@@ -7,7 +7,7 @@ import isLiteral from "./probes/isLiteral.js";
 import isLiteralRegex from "./probes/isLiteralRegex.js";
 import isRegexObject from "./probes/isRegexObject.js";
 import isVariableDeclaration from "./probes/isVariableDeclaration.js";
-import isRequire from "./probes/isRequire.js";
+import isRequire from "./probes/isRequire/isRequire.js";
 import isImportDeclaration from "./probes/isImportDeclaration.js";
 import isMemberExpression from "./probes/isMemberExpression.js";
 import isArrayExpression from "./probes/isArrayExpression.js";

diff --git a/src/probes/isRequire/RequireCallExpressionWalker.js b/src/probes/isRequire/RequireCallExpressionWalker.js
@@ -0,0 +1,93 @@
+// Import Node.js Dependencies
+import path from "node:path";
+
+// Import Third-party Dependencies
+import { Hex } from "@nodesecure/sec-literal";
+import { walk as doWalk } from "estree-walker";
+import {
+  arrayExpressionToString,
+  getMemberExpressionIdentifier,
+  getCallExpressionArguments
+} from "@nodesecure/estree-ast-utils";
+
+export class RequireCallExpressionWalker {
+  constructor(tracer) {
+    this.tracer = tracer;
+    this.dependencies = new Set();
+    this.triggerWarning = true;
+  }
+
+  walk(nodeToWalk) {
+    this.dependencies = new Set();
+    this.triggerWarning = true;
+
+    // we need the `this` context of doWalk.enter
+    const self = this;
+    doWalk(nodeToWalk, {
+      enter(node) {
+        if (node.type !== "CallExpression" || node.arguments.length === 0) {
+          return;
+        }
+
+        const rootArgument = node.arguments.at(0);
+        if (rootArgument.type === "Literal" && Hex.isHex(rootArgument.value)) {
+          self.dependencies.add(Buffer.from(rootArgument.value, "hex").toString());
+          this.skip();
+
+          return;
+        }
+
+        const fullName = node.callee.type === "MemberExpression" ?
+          [...getMemberExpressionIdentifier(node.callee)].join(".") :
+          node.callee.name;
+        const tracedFullName = self.tracer.getDataFromIdentifier(fullName)?.identifierOrMemberExpr ?? fullName;
+        switch (tracedFullName) {
+          case "atob":
+            self.#handleAtob(node);
+            break;
+          case "Buffer.from":
+            self.#handleBufferFrom(node);
+            break;
+          case "require.resolve":
+            self.#handleRequireResolve(rootArgument);
+            break;
+          case "path.join":
+            self.#handlePathJoin(node);
+            break;
+        }
+      }
+    });
+
+    return { dependencies: this.dependencies, triggerWarning: this.triggerWarning };
+  }
+
+  #handleAtob(node) {
+    const nodeArguments = getCallExpressionArguments(node, { tracer: this.tracer });
+    if (nodeArguments !== null) {
+      this.dependencies.add(Buffer.from(nodeArguments.at(0), "base64").toString());
+    }
+  }
+
+  #handleBufferFrom(node) {
+    const [element] = node.arguments;
+    if (element.type === "ArrayExpression") {
+      const depName = [...arrayExpressionToString(element)].join("").trim();
+      this.dependencies.add(depName);
+    }
+  }
+
+  #handleRequireResolve(rootArgument) {
+    if (rootArgument.type === "Literal") {
+      this.dependencies.add(rootArgument.value);
+    }
+  }
+
+  #handlePathJoin(node) {
+    if (!node.arguments.every((arg) => arg.type === "Literal" && typeof arg.value === "string")) {
+      return;
+    }
+    const constructedPath = path.posix.join(...node.arguments.map((arg) => arg.value));
+    this.dependencies.add(constructedPath);
+    this.triggerWarning = false;
+  }
+}
diff --git a/src/probes/isRequire.js → src/probes/isRequire/isRequire.js b/src/probes/isRequire.js → src/probes/isRequire/isRequire.js
@@ -1,19 +1,13 @@
 /* eslint-disable consistent-return */
 
-// Import Node.js Dependencies
-import path from "node:path";
-
-// Import Third-party Dependencies
-import { Hex } from "@nodesecure/sec-literal";
-import { walk } from "estree-walker";
 import {
   concatBinaryExpression,
   arrayExpressionToString,
-  getMemberExpressionIdentifier,
   getCallExpressionIdentifier,
   getCallExpressionArguments
 } from "@nodesecure/estree-ast-utils";
-import { ProbeSignals } from "../ProbeRunner.js";
+import { ProbeSignals } from "../../ProbeRunner.js";
+import { RequireCallExpressionWalker } from "./RequireCallExpressionWalker.js";
 
 function validateNodeRequire(node, { tracer }) {
   const id = getCallExpressionIdentifier(node, {
@@ -53,7 +47,6 @@ function teardown({ sourceFile }) {
   sourceFile.dependencyAutoWarning = false;
 }
 
-
 function main(node, options) {
   const { sourceFile, data: calleeName } = options;
   const { tracer } = sourceFile;
@@ -123,7 +116,8 @@ function main(node, options) {
 
     // require(Buffer.from("...", "hex").toString());
     case "CallExpression": {
-      const { dependencies, triggerWarning } = walkRequireCallExpression(arg, tracer);
+      const walker = new RequireCallExpressionWalker(tracer);
+      const { dependencies, triggerWarning } = walker.walk(arg);
       dependencies.forEach((depName) => sourceFile.addDependency(depName, node.loc, true));
 
       if (triggerWarning) {
@@ -139,73 +133,6 @@ function main(node, options) {
   }
 }
 
-function walkRequireCallExpression(nodeToWalk, tracer) {
-  const dependencies = new Set();
-  let triggerWarning = true;
-
-  walk(nodeToWalk, {
-    enter(node) {
-      if (node.type !== "CallExpression" || node.arguments.length === 0) {
-        return;
-      }
-
-      const rootArgument = node.arguments.at(0);
-      if (rootArgument.type === "Literal" && Hex.isHex(rootArgument.value)) {
-        dependencies.add(Buffer.from(rootArgument.value, "hex").toString());
-
-        return this.skip();
-      }
-
-      const fullName = node.callee.type === "MemberExpression" ?
-        [...getMemberExpressionIdentifier(node.callee)].join(".") :
-        node.callee.name;
-      const tracedFullName = tracer.getDataFromIdentifier(fullName)?.identifierOrMemberExpr ?? fullName;
-
-      switch (tracedFullName) {
-        case "atob": {
-          const nodeArguments = getCallExpressionArguments(node, { tracer });
-          if (nodeArguments !== null) {
-            dependencies.add(
-              Buffer.from(nodeArguments.at(0), "base64").toString()
-            );
-          }
-
-          break;
-        }
-        case "Buffer.from": {
-          const [element] = node.arguments;
-
-          if (element.type === "ArrayExpression") {
-            const depName = [...arrayExpressionToString(element)].join("").trim();
-            dependencies.add(depName);
-          }
-          break;
-        }
-        case "require.resolve": {
-          if (rootArgument.type === "Literal") {
-            dependencies.add(rootArgument.value);
-          }
-          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, triggerWarning };
-}
-
 export default {
   name: "isRequire",
   validateNode: [validateNodeRequire, validateNodeEvalRequire],

diff --git a/test/probes/isRequire.spec.js b/test/probes/isRequire.spec.js
@@ -4,7 +4,7 @@ import assert from "node:assert";
 
 // Import Internal Dependencies
 import { getSastAnalysis, parseScript } from "../utils/index.js";
-import isRequire from "../../src/probes/isRequire.js";
+import isRequire from "../../src/probes/isRequire/isRequire.js";
 
 test("it should ignore require CallExpression with no (zero) arguments", () => {
   const str = `
@@ -360,17 +360,18 @@ test("(require CallExpression): it should detect MemberExpression require.resolv
 test("(require CallExpression): it should detect obfuscated atob value", () => {
   const str = `
     const myFunc = atob;
-    const ff = myFunc('b3' + 'M=');
-    const dep = require(ff);
+    const dep = require(myFunc('b3' + 'M='));
   `;
+
   const ast = parseScript(str);
   const sastAnalysis = getSastAnalysis(str, isRequire)
     .execute(ast.body);
 
-  assert.strictEqual(sastAnalysis.warnings().length, 0);
+  assert.strictEqual(sastAnalysis.warnings().length, 1);
+  const warning = sastAnalysis.getWarning("unsafe-import");
+  assert.strictEqual(warning.kind, "unsafe-import");
 
   const dependencies = sastAnalysis.dependencies();
   assert.strictEqual(dependencies.size, 1);
   assert.ok(dependencies.has("os"));
 });
-