Merge pull request #417 from AikidoSec/patch-path

Avoid double wrapping path

library/helpers/isWindows.ts

@@ -0,0 +1,3 @@
+export function isWindows() {
+  return process.platform === "win32";
+}

library/sinks/Path.doubleWrapping.test.ts

@@ -0,0 +1,50 @@
+import { join, normalize, resolve } from "path/posix";
+import * as t from "tap";
+import { isWindows } from "../helpers/isWindows";
+import { isWrapped } from "../helpers/wrap";
+import { Path } from "./Path";
+import { createTestAgent } from "../helpers/createTestAgent";
+
+t.test(
+  "it works",
+  { skip: isWindows() ? "path is not the same as path/posix" : false },
+  async (t) => {
+    const agent = createTestAgent();
+
+    agent.start([new Path()]);
+
+    const { join, resolve, normalize } = require("path/posix");
+
+    // Path required after path/posix
+    require("path");
+
+    const checkForDoubleWrapping = [join, resolve, normalize];
+    for (const fn of checkForDoubleWrapping) {
+      if (isWrapped(fn) && isWrapped(fn.__original)) {
+        t.fail(`${fn.name} is double wrapped!`);
+      }
+    }
+  }
+);
+
+t.test(
+  "it works",
+  { skip: !isWindows() ? "path is not the same as path/win32" : false },
+  async (t) => {
+    const agent = createTestAgent();
+
+    agent.start([new Path()]);
+
+    const { join, resolve, normalize } = require("path/win32");
+
+    // Path required after path/win32
+    require("path");
+
+    const checkForDoubleWrapping = [join, resolve, normalize];
+    for (const fn of checkForDoubleWrapping) {
+      if (isWrapped(fn) && isWrapped(fn.__original)) {
+        t.fail(`${fn.name} is double wrapped!`);
+      }
+    }
+  }
+);

library/sinks/Path.test.ts

@@ -1,5 +1,6 @@
 import * as t from "tap";
 import { Context, runWithContext } from "../agent/Context";
+import { isWrapped } from "../helpers/wrap";
 import { Path } from "./Path";
 import { createTestAgent } from "../helpers/createTestAgent";
 
@@ -30,7 +31,8 @@ t.test("it works", async (t) => {
 
   agent.start([new Path()]);
 
-  const { join, resolve } = require("path");
+  // Path required before path/posix and path/win32
+  const { join, resolve, normalize } = require("path");
 
   function safeCalls() {
     t.same(join("test.txt"), "test.txt");
@@ -147,4 +149,11 @@ t.test("it works", async (t) => {
       "Zen has blocked a path traversal attack: path.normalize(...) originating from body.file.matches"
     );
   });
+
+  const checkForDoubleWrapping = [join, resolve, normalize];
+  for (const fn of checkForDoubleWrapping) {
+    if (isWrapped(fn) && isWrapped(fn.__original)) {
+      t.fail(`${fn.name} is double wrapped!`);
+    }
+  }
 });

library/sinks/Path.ts

@@ -3,9 +3,14 @@ import { Hooks } from "../agent/hooks/Hooks";
 import { wrapExport } from "../agent/hooks/wrapExport";
 import { WrapPackageInfo } from "../agent/hooks/WrapPackageInfo";
 import { Wrapper } from "../agent/Wrapper";
+import { isWindows } from "../helpers/isWindows";
 import { checkContextForPathTraversal } from "../vulnerabilities/path-traversal/checkContextForPathTraversal";
+import type * as path from "path";
 
 export class Path implements Wrapper {
+  private patchedPosix = false;
+  private patchedWin32 = false;
+
   private inspectPath(args: unknown[], operation: string) {
     const context = getContext();
 
@@ -34,19 +39,65 @@ export class Path implements Wrapper {
     return undefined;
   }
 
-  wrap(hooks: Hooks): void {
+  private wrapFunctions(exports: unknown, pkgInfo: WrapPackageInfo) {
     const functions = ["join", "resolve", "normalize"];
 
-    const onRequire = (exports: any, pkgInfo: WrapPackageInfo) => {
-      for (const func of functions) {
-        wrapExport(exports, func, pkgInfo, {
-          inspectArgs: (args) => this.inspectPath(args, func),
-        });
-      }
-    };
+    for (const func of functions) {
+      wrapExport(exports, func, pkgInfo, {
+        inspectArgs: (args) => this.inspectPath(args, func),
+      });
+    }
+  }
+
+  private wrapMainModule(exports: typeof path, pkgInfo: WrapPackageInfo) {
+    // If `path/win32` or `path/posix` was not required before `path`, we should wrap the functions in `path`
+    if (!this.patchedWin32 && !this.patchedPosix) {
+      this.wrapFunctions(exports, pkgInfo);
+    }
+
+    if (isWindows()) {
+      // `require("path").join` is the same as `require("path/win32").join`
+      this.patchedWin32 = true;
+    } else {
+      // `require("path").join` is the same as `require("path/posix").join`
+      this.patchedPosix = true;
+    }
+
+    this.wrapPosix(exports.posix, pkgInfo);
+    this.wrapWin32(exports.win32, pkgInfo);
+  }
+
+  private wrapPosix(exports: unknown, pkgInfo: WrapPackageInfo) {
+    if (this.patchedPosix) {
+      return;
+    }
+
+    this.wrapFunctions(exports, pkgInfo);
+
+    this.patchedPosix = true;
+  }
+
+  private wrapWin32(exports: unknown, pkgInfo: WrapPackageInfo) {
+    if (this.patchedWin32) {
+      return;
+    }
+
+    this.wrapFunctions(exports, pkgInfo);
+
+    this.patchedWin32 = true;
+  }
+
+  wrap(hooks: Hooks): void {
+    hooks
+      .addBuiltinModule("path")
+      .onRequire((exports, pkgInfo) => this.wrapMainModule(exports, pkgInfo));
+
+    hooks
+      .addBuiltinModule("path/posix")
+      .onRequire((exports, pkgInfo) => this.wrapPosix(exports, pkgInfo));
 
-    hooks.addBuiltinModule("path").onRequire(onRequire);
-    hooks.addBuiltinModule("path/posix").onRequire(onRequire);
-    hooks.addBuiltinModule("path/win32").onRequire(onRequire);
+    hooks
+      .addBuiltinModule("path/win32")
+      .onRequire((exports, pkgInfo) => this.wrapWin32(exports, pkgInfo));
   }
 }