fix(builtin): don't allow symlinks to escape or enter bazel managed n…

…ode_module folders

internal/node/launcher.sh

@@ -217,6 +217,22 @@ fi
 # Bazel always sets the PWD to execroot/my_wksp so we go up one directory.
 export BAZEL_PATCH_ROOT=$(dirname $PWD)
 
+# Set all bazel managed node_modules directories as guarded so no symlinks may
+# escape and no symlinks may enter
+if [ "$(basename ${BAZEL_PATCH_ROOT})" == "execroot" ]; then
+  # We are in execroot, linker node_modules is in the PWD
+  export BAZEL_PATCH_GUARDS="${PWD}/node_modules"
+else
+  # We in runfiles, linker node_modules is at the runfiles root
+  export BAZEL_PATCH_GUARDS="${BAZEL_PATCH_ROOT}/node_modules"
+fi
+if [[ -n "${BAZEL_NODE_MODULES_ROOT:-}" ]]; then
+  # If BAZEL_NODE_MODULES_ROOT is set, add it to the list of bazel patch guards
+  # Also, add the external/${BAZEL_NODE_MODULES_ROOT} which is the correct path under execroot
+  # and under runfiles it is the legacy external runfiles path
+  export BAZEL_PATCH_GUARDS="${BAZEL_PATCH_GUARDS},${BAZEL_PATCH_ROOT}/${BAZEL_NODE_MODULES_ROOT},${PWD}/external/${BAZEL_NODE_MODULES_ROOT}"
+fi
+
 # The EXPECTED_EXIT_CODE lets us write bazel tests which assert that
 # a binary fails to run. Otherwise any failure would make such a test
 # fail before we could assert that we expected that failure.

internal/node/node.bzl

@@ -79,7 +79,7 @@ def _compute_node_modules_root(ctx):
         ] if f])
     return node_modules_root
 
-def _write_require_patch_script(ctx):
+def _write_require_patch_script(ctx, node_modules_root):
     # Generates the JavaScript snippet of module roots mappings, with each entry
     # in the form:
     #   {module_name: /^mod_name\b/, module_root: 'path/to/mod_name'}
@@ -91,8 +91,6 @@ def _write_require_patch_script(ctx):
                 mapping = "{module_name: /^%s\\b/, module_root: '%s'}" % (escaped, mr)
                 module_mappings.append(mapping)
 
-    node_modules_root = _compute_node_modules_root(ctx)
-
     ctx.actions.expand_template(
         template = ctx.file._require_patch_template,
         output = ctx.outputs.require_patch_script,
@@ -175,7 +173,9 @@ def _nodejs_binary_impl(ctx):
             sources_depsets.append(d.files)
     sources = depset(transitive = sources_depsets)
 
-    _write_require_patch_script(ctx)
+    node_modules_root = _compute_node_modules_root(ctx)
+
+    _write_require_patch_script(ctx, node_modules_root)
     _write_loader_script(ctx)
 
     # Provide the target name as an environment variable avaiable to all actions for the
@@ -190,6 +190,13 @@ def _nodejs_binary_impl(ctx):
     # runfiles helpers to use.
     env_vars += "export BAZEL_WORKSPACE=%s\n" % ctx.workspace_name
 
+    # if BAZEL_NODE_MODULES_ROOT has not already been set by
+    # run_node, then set it to the computed value
+    env_vars += """if [[ -z "${BAZEL_NODE_MODULES_ROOT:-}" ]]; then
+  export BAZEL_NODE_MODULES_ROOT=%s
+fi
+""" % node_modules_root
+
     for k in ctx.attr.configuration_env_vars + ctx.attr.default_env_vars:
         # Check ctx.var first & if env var not in there then check
         # ctx.configuration.default_shell_env. The former will contain values from --define=FOO=BAR

internal/node/node_patches.js

@@ -61,9 +61,10 @@ Object.defineProperty(exports, "__esModule", { value: true });
 // es modules
 
 // tslint:disable-next-line:no-any
-exports.patcher = (fs = fs$1, root) => {
+exports.patcher = (fs = fs$1, root, guards) => {
     fs = fs || fs$1;
     root = root || '';
+    guards = guards || [];
     if (!root) {
         if (process.env.VERBOSE_LOGS) {
             console.error('fs patcher called without root path ' + __filename);
@@ -83,7 +84,7 @@ exports.patcher = (fs = fs$1, root) => {
     const origReadlinkSync = fs.readlinkSync.bind(fs);
     const origReaddir = fs.readdir.bind(fs);
     const origReaddirSync = fs.readdirSync.bind(fs);
-    const { isEscape, isOutPath } = exports.escapeFunction(root);
+    const { isEscape, isOutPath } = exports.escapeFunction(root, guards);
     // tslint:disable-next-line:no-any
     fs.lstat = (...args) => {
         let cb = args.length > 1 ? args[args.length - 1] : undefined;
@@ -483,23 +484,36 @@ exports.patcher = (fs = fs$1, root) => {
         }
     }
 };
-exports.escapeFunction = (root) => {
-    // ensure root is always absolute.
+exports.escapeFunction = (root, guards) => {
+    // ensure root & guards are always absolute.
     root = path.resolve(root);
+    guards = [...guards].map(g => path.resolve(g));
     function isEscape(linkTarget, linkPath) {
         if (!path.isAbsolute(linkPath)) {
             linkPath = path.resolve(linkPath);
         }
         if (!path.isAbsolute(linkTarget)) {
             linkTarget = path.resolve(linkTarget);
         }
+        if (isGuardPath(linkPath) || isGuardPath(linkTarget)) {
+            // don't escape out of guard paths and don't symlink into guard paths
+            return true;
+        }
         if (root) {
             if (isOutPath(linkTarget) && !isOutPath(linkPath)) {
+                // don't escape out of the root
                 return true;
             }
         }
         return false;
     }
+    function isGuardPath(str) {
+        for (const g of guards) {
+            if (str === g || str.startsWith(g + path.sep))
+                return true;
+        }
+        return false;
+    }
     function isOutPath(str) {
         return !root || (!str.startsWith(root + path.sep) && str !== root);
     }
@@ -644,12 +658,13 @@ var src_2 = src.subprocess;
  */
 
 // todo auto detect bazel env vars instead of adding a new one.
-const { BAZEL_PATCH_ROOT, NP_SUBPROCESS_BIN_DIR, VERBOSE_LOGS } = process.env;
+const { BAZEL_PATCH_ROOT, BAZEL_PATCH_GUARDS, NP_SUBPROCESS_BIN_DIR, VERBOSE_LOGS } = process.env;
 if (BAZEL_PATCH_ROOT) {
+    const guards = BAZEL_PATCH_GUARDS ? BAZEL_PATCH_GUARDS.split(',') : [];
     if (VERBOSE_LOGS)
         console.error(`bazel node patches enabled. root: ${BAZEL_PATCH_ROOT} symlinks in this directory will not escape`);
     const fs = fs$1;
-    src.fs(fs, BAZEL_PATCH_ROOT);
+    src.fs(fs, BAZEL_PATCH_ROOT, guards);
 }
 else if (VERBOSE_LOGS) {
     console.error(`bazel node patches disabled. set environment BAZEL_PATCH_ROOT`);

internal/providers/node_runtime_deps_info.bzl

@@ -15,6 +15,7 @@
 """Custom provider that mimics the Runfiles, but doesn't incur the expense of creating the runfiles symlink tree"""
 
 load("//internal/linker:link_node_modules.bzl", "add_arg", "write_node_modules_manifest")
+load("//internal/providers:npm_package_info.bzl", "NpmPackageInfo")
 
 NodeRuntimeDepsInfo = provider(
     doc = """Stores runtime dependencies of a nodejs_binary or nodejs_test
@@ -38,6 +39,23 @@ do the same.
     },
 )
 
+def _compute_node_modules_root(ctx):
+    """Computes the node_modules root (if any) from data & deps targets."""
+    node_modules_root = ""
+    deps = []
+    if hasattr(ctx.attr, "data"):
+        deps += ctx.attr.data
+    if hasattr(ctx.attr, "deps"):
+        deps += ctx.attr.deps
+    for d in deps:
+        if NpmPackageInfo in d:
+            possible_root = "/".join([d[NpmPackageInfo].workspace, "node_modules"])
+            if not node_modules_root:
+                node_modules_root = possible_root
+            elif node_modules_root != possible_root:
+                fail("All npm dependencies need to come from a single workspace. Found '%s' and '%s'." % (node_modules_root, possible_root))
+    return node_modules_root
+
 def run_node(ctx, inputs, arguments, executable, **kwargs):
     """Helper to replace ctx.actions.run
     This calls node programs with a node_modules directory in place"""
@@ -77,6 +95,7 @@ def run_node(ctx, inputs, arguments, executable, **kwargs):
                 env[var] = ctx.var[var]
             elif var in ctx.configuration.default_shell_env.keys():
                 env[var] = ctx.configuration.default_shell_env[var]
+    env["BAZEL_NODE_MODULES_ROOT"] = _compute_node_modules_root(ctx)
 
     ctx.actions.run(
         inputs = inputs + extra_inputs,

packages/node-patches/register.ts

@@ -19,14 +19,15 @@
  */
 const patcher = require('./src');
 // todo auto detect bazel env vars instead of adding a new one.
-const {BAZEL_PATCH_ROOT, NP_SUBPROCESS_BIN_DIR, VERBOSE_LOGS} = process.env;
+const {BAZEL_PATCH_ROOT, BAZEL_PATCH_GUARDS, NP_SUBPROCESS_BIN_DIR, VERBOSE_LOGS} = process.env;
 
 if (BAZEL_PATCH_ROOT) {
+  const guards = BAZEL_PATCH_GUARDS ? BAZEL_PATCH_GUARDS.split(',') : [];
   if (VERBOSE_LOGS)
     console.error(`bazel node patches enabled. root: ${
         BAZEL_PATCH_ROOT} symlinks in this directory will not escape`);
   const fs = require('fs');
-  patcher.fs(fs, BAZEL_PATCH_ROOT);
+  patcher.fs(fs, BAZEL_PATCH_ROOT, guards);
 } else if (VERBOSE_LOGS) {
   console.error(`bazel node patches disabled. set environment BAZEL_PATCH_ROOT`);
 }

packages/node-patches/src/fs.ts

@@ -29,9 +29,10 @@ type Dirent = any;
 const _fs = require('fs');
 
 // tslint:disable-next-line:no-any
-export const patcher = (fs: any = _fs, root: string) => {
+export const patcher = (fs: any = _fs, root: string, guards: string[]) => {
   fs = fs || _fs;
   root = root || '';
+  guards = guards || [];
   if (!root) {
     if (process.env.VERBOSE_LOGS) {
       console.error('fs patcher called without root path ' + __filename);
@@ -54,7 +55,7 @@ export const patcher = (fs: any = _fs, root: string) => {
   const origReaddir = fs.readdir.bind(fs);
   const origReaddirSync = fs.readdirSync.bind(fs);
 
-  const {isEscape, isOutPath} = escapeFunction(root);
+  const {isEscape, isOutPath} = escapeFunction(root, guards);
 
   const logged: {[k: string]: boolean} = {};
 
@@ -471,9 +472,10 @@ export const patcher = (fs: any = _fs, root: string) => {
   }
 };
 
-export const escapeFunction = (root: string) => {
-  // ensure root is always absolute.
+export const escapeFunction = (root: string, guards: string[]) => {
+  // ensure root & guards are always absolute.
   root = path.resolve(root);
+  guards = [...guards].map(g => path.resolve(g));
   function isEscape(linkTarget: string, linkPath: string) {
     if (!path.isAbsolute(linkPath)) {
       linkPath = path.resolve(linkPath);
@@ -483,14 +485,27 @@ export const escapeFunction = (root: string) => {
       linkTarget = path.resolve(linkTarget);
     }
 
+    if (isGuardPath(linkPath) || isGuardPath(linkTarget)) {
+      // don't escape out of guard paths and don't symlink into guard paths
+      return true;
+    }
+
     if (root) {
       if (isOutPath(linkTarget) && !isOutPath(linkPath)) {
+        // don't escape out of the root
         return true;
       }
     }
     return false;
   }
 
+  function isGuardPath(str) {
+    for (const g of guards) {
+      if (str === g || str.startsWith(g + path.sep)) return true;
+    }
+    return false;
+  }
+
   function isOutPath(str: string) {
     return !root || (!str.startsWith(root + path.sep) && str !== root);
   }

packages/node-patches/test/fs/escape.ts

@@ -22,7 +22,7 @@ import {escapeFunction} from '../../src/fs';
 describe('escape function', () => {
   it('isOutPath is correct', () => {
     const root = '/a/b';
-    const {isOutPath} = escapeFunction(root);
+    const {isOutPath} = escapeFunction(root, []);
 
     assert.ok(isOutPath('/a'));
     assert.ok(isOutPath('/a/c/b'));
@@ -32,7 +32,7 @@ describe('escape function', () => {
 
   it('isEscape is correct', () => {
     const root = '/a/b';
-    const {isEscape} = escapeFunction(root);
+    const {isEscape} = escapeFunction(root, []);
 
     assert.ok(isEscape('/a/c/boop', '/a/b/l'));
     assert.ok(isEscape('/a/c/boop', '/a/b'));
@@ -43,7 +43,7 @@ describe('escape function', () => {
 
   it('isEscape handles relative paths', () => {
     const root = './a/b';
-    const {isEscape} = escapeFunction(root);
+    const {isEscape} = escapeFunction(root, []);
 
     assert.ok(isEscape('./a/c/boop', './a/b/l'));
     assert.ok(isEscape('./a/c/boop', './a/b'));

packages/node-patches/test/fs/lstat.ts

@@ -38,7 +38,7 @@ describe('testing lstat', () => {
 
           const patchedFs = Object.assign({}, fs);
           patchedFs.promises = Object.assign({}, fs.promises);
-          patcher(patchedFs, path.join(fixturesDir));
+          patcher(patchedFs, path.join(fixturesDir), []);
 
           const linkPath = path.join(fixturesDir, 'a', 'link');
           assert.ok(
@@ -68,7 +68,7 @@ describe('testing lstat', () => {
 
           const patchedFs = Object.assign({}, fs);
           patchedFs.promises = Object.assign({}, fs.promises);
-          patcher(patchedFs, path.join(fixturesDir, 'a'));
+          patcher(patchedFs, path.join(fixturesDir, 'a'), []);
 
           const linkPath = path.join(fixturesDir, 'a', 'link');
 
@@ -114,7 +114,7 @@ describe('testing lstat', () => {
 
           const patchedFs = Object.assign({}, fs);
           patchedFs.promises = Object.assign({}, fs.promises);
-          patcher(patchedFs, path.join(fixturesDir, 'a'));
+          patcher(patchedFs, path.join(fixturesDir, 'a'), []);
 
           const linkPath = path.join(fixturesDir, 'b', 'link');
 

packages/node-patches/test/fs/opendir.ts

@@ -39,7 +39,7 @@ describe('testing opendir', () => {
 
           const patchedFs = Object.assign({}, fs);
           patchedFs.promises = Object.assign({}, fs.promises);
-          patcher(patchedFs, fixturesDir);
+          patcher(patchedFs, fixturesDir, []);
           (patchedFs as any).DEBUG = true;
 
           let dir;
@@ -74,7 +74,7 @@ describe('testing opendir', () => {
 
           const patchedFs = Object.assign({}, fs);
           patchedFs.promises = Object.assign({}, fs.promises);
-          patcher(patchedFs, path.join(fixturesDir, 'a'));
+          patcher(patchedFs, path.join(fixturesDir, 'a'), []);
           (patchedFs as any).DEBUG = true;
 
           let dir;
@@ -109,7 +109,7 @@ describe('testing opendir', () => {
 
           const patchedFs = Object.assign({}, fs);
           patchedFs.promises = Object.assign({}, fs.promises);
-          patcher(patchedFs, path.join(fixturesDir));
+          patcher(patchedFs, path.join(fixturesDir), []);
           (patchedFs as any).DEBUG = true;
 
           const dir = await util.promisify(patchedFs.opendir)(path.join(fixturesDir, 'a'));
@@ -140,7 +140,7 @@ describe('testing opendir', () => {
 
           const patchedFs = Object.assign({}, fs);
           patchedFs.promises = Object.assign({}, fs.promises);
-          patcher(patchedFs, path.join(fixturesDir, 'a'));
+          patcher(patchedFs, path.join(fixturesDir, 'a'), []);
           (patchedFs as any).DEBUG = true;
 
           const dir = await util.promisify(patchedFs.opendir)(path.join(fixturesDir, 'a'));

packages/node-patches/test/fs/readdir.ts

@@ -38,7 +38,7 @@ describe('testing readdir', () => {
 
           const patchedFs = Object.assign({}, fs);
           patchedFs.promises = Object.assign({}, fs.promises);
-          patcher(patchedFs, fixturesDir);
+          patcher(patchedFs, fixturesDir, []);
 
           let dirents = patchedFs.readdirSync(path.join(fixturesDir, 'a'), {
             withFileTypes: true,
@@ -77,7 +77,7 @@ describe('testing readdir', () => {
 
           const patchedFs = Object.assign({}, fs);
           patchedFs.promises = Object.assign({}, fs.promises);
-          patcher(patchedFs, path.join(fixturesDir, 'a'));
+          patcher(patchedFs, path.join(fixturesDir, 'a'), []);
 
           let dirents = patchedFs.readdirSync(path.join(fixturesDir, 'a'), {
             withFileTypes: true,

packages/node-patches/test/fs/readlink.ts

@@ -38,7 +38,7 @@ describe('testing readlink', () => {
           const patchedFs = Object.assign({}, fs);
           patchedFs.promises = Object.assign({}, fs.promises);
 
-          patcher(patchedFs, path.join(fixturesDir));
+          patcher(patchedFs, path.join(fixturesDir), []);
           const linkPath = path.join(fixturesDir, 'a', 'link');
 
           assert.deepStrictEqual(
@@ -70,7 +70,7 @@ describe('testing readlink', () => {
           const patchedFs = Object.assign({}, fs);
           patchedFs.promises = Object.assign({}, fs.promises);
 
-          patcher(patchedFs, path.join(fixturesDir, 'a'));
+          patcher(patchedFs, path.join(fixturesDir, 'a'), []);
           const linkPath = path.join(fs.realpathSync(fixturesDir), 'a', 'link');
 
           assert.throws(() => {