fix(builtin): fix node patches subprocess sandbox propogation (#2017)

Greg Magolan · web-flow · commit 0bd9b7e7201c · 2020-07-09T10:45:47.000-07:00
Two fixes here.

Set --preserve-symlinks --preserve-symlinks-main in the generated _node_bin/node which comes from ${process.env.NODE_REPOSITORY_ARGS}. This will prevent the --require /path/to/node_patches.js from being resolved to its location outside of the sandbox.

However, if preserve_symlinks=false, which is still an option, the 2nd fix is to honour the value of NP_SUBPROCESS_NODE_DIR when running node_patches.js for the subprocess. Even if node_patches.js is resolved to outside of the sandbox for the subprocess and run from there, it will still look for _node_bin/node within the sandbox.
diff --git a/internal/node/launcher.sh b/internal/node/launcher.sh
@@ -165,9 +165,9 @@ readonly lcov_merger_script=$(rlocation "TEMPLATED_lcov_merger_script")
 source $repository_args
 
 ARGS=()
-LAUNCHER_NODE_OPTIONS=()
+LAUNCHER_NODE_OPTIONS=($NODE_REPOSITORY_ARGS)
 USER_NODE_OPTIONS=()
-ALL_ARGS=(TEMPLATED_args $NODE_REPOSITORY_ARGS "$@")
+ALL_ARGS=(TEMPLATED_args "$@")
 STDOUT_CAPTURE=""
 STDERR_CAPTURE=""
 EXIT_CODE_CAPTURE=""
@@ -176,7 +176,7 @@ RUN_LINKER=true
 NODE_PATCHES=true
 # TODO(alex): change the default to false
 PATCH_REQUIRE=true
-for ARG in "${ALL_ARGS[@]:-}"; do
+for ARG in ${ALL_ARGS[@]+"${ALL_ARGS[@]}"}; do
   case "$ARG" in
     # Supply custom linker arguments for first-party dependencies
     --bazel_node_modules_manifest=*) MODULES_MANIFEST="${ARG#--bazel_node_modules_manifest=}" ;;
@@ -316,13 +316,13 @@ _int() {
 set +e
 
 if [[ -n "${STDOUT_CAPTURE}" ]] && [[ -n "${STDERR_CAPTURE}" ]]; then
-  "${node}" "${LAUNCHER_NODE_OPTIONS[@]:-}" "${USER_NODE_OPTIONS[@]:-}" "${MAIN}" ${ARGS[@]+"${ARGS[@]}"} <&0 >$STDOUT_CAPTURE 2>$STDERR_CAPTURE &
+  "${node}" ${LAUNCHER_NODE_OPTIONS[@]+"${LAUNCHER_NODE_OPTIONS[@]}"} ${USER_NODE_OPTIONS[@]+"${USER_NODE_OPTIONS[@]}"} "${MAIN}" ${ARGS[@]+"${ARGS[@]}"} <&0 >$STDOUT_CAPTURE 2>$STDERR_CAPTURE &
 elif [[ -n "${STDOUT_CAPTURE}" ]]; then
-  "${node}" "${LAUNCHER_NODE_OPTIONS[@]:-}" "${USER_NODE_OPTIONS[@]:-}" "${MAIN}" ${ARGS[@]+"${ARGS[@]}"} <&0 >$STDOUT_CAPTURE &
+  "${node}" ${LAUNCHER_NODE_OPTIONS[@]+"${LAUNCHER_NODE_OPTIONS[@]}"} ${USER_NODE_OPTIONS[@]+"${USER_NODE_OPTIONS[@]}"} "${MAIN}" ${ARGS[@]+"${ARGS[@]}"} <&0 >$STDOUT_CAPTURE &
 elif [[ -n "${STDERR_CAPTURE}" ]]; then
-  "${node}" "${LAUNCHER_NODE_OPTIONS[@]:-}" "${USER_NODE_OPTIONS[@]:-}" "${MAIN}" ${ARGS[@]+"${ARGS[@]}"} <&0 2>$STDERR_CAPTURE &
+  "${node}" ${LAUNCHER_NODE_OPTIONS[@]+"${LAUNCHER_NODE_OPTIONS[@]}"} ${USER_NODE_OPTIONS[@]+"${USER_NODE_OPTIONS[@]}"} "${MAIN}" ${ARGS[@]+"${ARGS[@]}"} <&0 2>$STDERR_CAPTURE &
 else
-  "${node}" "${LAUNCHER_NODE_OPTIONS[@]:-}" "${USER_NODE_OPTIONS[@]:-}" "${MAIN}" ${ARGS[@]+"${ARGS[@]}"} <&0 &
+  "${node}" ${LAUNCHER_NODE_OPTIONS[@]+"${LAUNCHER_NODE_OPTIONS[@]}"} ${USER_NODE_OPTIONS[@]+"${USER_NODE_OPTIONS[@]}"} "${MAIN}" ${ARGS[@]+"${ARGS[@]}"} <&0 &
 fi
 
 readonly child=$!
diff --git a/internal/node/node_patches.js b/internal/node/node_patches.js
@@ -551,39 +551,41 @@ Object.defineProperty(exports, "__esModule", { value: true });
 // but adds support to ensure the registered loader is included in all nested executions of nodejs.
 
 
-exports.patcher = (requireScriptName, binDir) => {
+exports.patcher = (requireScriptName, nodeDir) => {
     requireScriptName = path.resolve(requireScriptName);
-    const dir = path.dirname(requireScriptName);
+    nodeDir = nodeDir || path.join(path.dirname(requireScriptName), '_node_bin');
     const file = path.basename(requireScriptName);
-    const nodeDir = path.join(binDir || dir, '_node_bin');
-    if (!process.env.NP_PATCHED_NODEJS) {
-        // TODO: WINDOWS.
-        try {
-            fs$1.mkdirSync(nodeDir, { recursive: true });
-        }
-        catch (e) {
-            // with node versions that don't have recursive mkdir this may throw an error.
-            if (e.code !== 'EEXIST') {
-                throw e;
-            }
+    try {
+        fs$1.mkdirSync(nodeDir, { recursive: true });
+    }
+    catch (e) {
+        // with node versions that don't have recursive mkdir this may throw an error.
+        if (e.code !== 'EEXIST') {
+            throw e;
         }
-        if (process.platform == 'win32') {
-            fs$1.writeFileSync(path.join(nodeDir, 'node.bat'), `@if not defined DEBUG_HELPER @ECHO OFF
-set NP_PATCHED_NODEJS=${nodeDir}
+    }
+    if (process.platform == 'win32') {
+        const nodeEntry = path.join(nodeDir, 'node.bat');
+        if (!fs$1.existsSync(nodeEntry)) {
+            fs$1.writeFileSync(nodeEntry, `@if not defined DEBUG_HELPER @ECHO OFF
+set NP_SUBPROCESS_NODE_DIR=${nodeDir}
 set Path=${nodeDir};%Path%
-"${process.execPath}" --require "${requireScriptName}" %*
-        `);
+"${process.execPath}" ${process.env.NODE_REPOSITORY_ARGS} --require "${requireScriptName}" %*
+`);
         }
-        else {
-            fs$1.writeFileSync(path.join(nodeDir, 'node'), `#!/bin/bash
-export NP_PATCHED_NODEJS="${nodeDir}"
+    }
+    else {
+        const nodeEntry = path.join(nodeDir, 'node');
+        if (!fs$1.existsSync(nodeEntry)) {
+            fs$1.writeFileSync(nodeEntry, `#!/bin/bash
+export NP_SUBPROCESS_NODE_DIR="${nodeDir}"
 export PATH="${nodeDir}":\$PATH
 if [[ ! "\${@}" =~ "${file}" ]]; then
-  exec ${process.execPath} --require "${requireScriptName}" "$@"
+  exec ${process.execPath} ${process.env.NODE_REPOSITORY_ARGS} --require "${requireScriptName}" "$@"
 else
-  exec ${process.execPath} "$@"
+  exec ${process.execPath} ${process.env.NODE_REPOSITORY_ARGS} "$@"
 fi
-  `, { mode: 0o777 });
+`, { mode: 0o777 });
         }
     }
     if (!process.env.PATH) {
@@ -659,8 +661,7 @@ var src_2 = src.subprocess;
  * @fileoverview Description of this file.
  */
 
-// todo auto detect bazel env vars instead of adding a new one.
-const { BAZEL_PATCH_ROOT, BAZEL_PATCH_GUARDS, NP_SUBPROCESS_BIN_DIR, VERBOSE_LOGS } = process.env;
+const { BAZEL_PATCH_ROOT, BAZEL_PATCH_GUARDS, NP_SUBPROCESS_NODE_DIR, VERBOSE_LOGS } = process.env;
 if (BAZEL_PATCH_ROOT) {
     const guards = BAZEL_PATCH_GUARDS ? BAZEL_PATCH_GUARDS.split(',') : [];
     if (VERBOSE_LOGS)
@@ -671,7 +672,7 @@ if (BAZEL_PATCH_ROOT) {
 else if (VERBOSE_LOGS) {
     console.error(`bazel node patches disabled. set environment BAZEL_PATCH_ROOT`);
 }
-src.subprocess(__filename, NP_SUBPROCESS_BIN_DIR);
+src.subprocess(__filename, NP_SUBPROCESS_NODE_DIR);
 
 var register = {
 
diff --git a/internal/node/node_repositories.bzl b/internal/node/node_repositories.bzl
@@ -439,13 +439,10 @@ def _prepare_node(repository_ctx):
         preserve_symlinks_main_support = check_version(repository_ctx.attr.node_version, "10.2.0")
         if preserve_symlinks_main_support:
             node_args = "--preserve-symlinks --preserve-symlinks-main"
-            node_repo_args = "\"--node_options=--preserve-symlinks --node_options=--preserve-symlinks-main\""
         else:
             node_args = "--preserve-symlinks"
-            node_repo_args = "--node_options=--preserve-symlinks"
     else:
         node_args = ""
-        node_repo_args = ""
 
     # The entry points for node for osx/linux and windows
     if not is_windows:
@@ -476,8 +473,8 @@ CALL "%SCRIPT_DIR%\\{node}" {args} %*
 # Immediately exit if any command fails.
 set -e
 # Generated by node_repositories.bzl
-export NODE_REPOSITORY_ARGS={args}
-""".format(args = node_repo_args), executable = True)
+export NODE_REPOSITORY_ARGS="{args}"
+""".format(args = node_args), executable = True)
 
     # The entry points for npm for osx/linux and windows
     # Runs npm using appropriate node entry point
diff --git a/internal/node/test/empty_args_fail.js b/internal/node/test/empty_args_fail.js
@@ -6,5 +6,6 @@ var theArgs = process.argv.slice(2);
 
 if (theArgs.length != 0) {
   // Non-zero exit code if the argument list is not empty
+  console.error(theArgs)
   process.exit(42)
 }
diff --git a/packages/node-patches/register.ts b/packages/node-patches/register.ts
@@ -18,8 +18,7 @@
  * @fileoverview Description of this file.
  */
 const patcher = require('./src');
-// todo auto detect bazel env vars instead of adding a new one.
-const {BAZEL_PATCH_ROOT, BAZEL_PATCH_GUARDS, NP_SUBPROCESS_BIN_DIR, VERBOSE_LOGS} = process.env;
+const {BAZEL_PATCH_ROOT, BAZEL_PATCH_GUARDS, NP_SUBPROCESS_NODE_DIR, VERBOSE_LOGS} = process.env;
 
 if (BAZEL_PATCH_ROOT) {
   const guards = BAZEL_PATCH_GUARDS ? BAZEL_PATCH_GUARDS.split(',') : [];
@@ -32,4 +31,4 @@ if (BAZEL_PATCH_ROOT) {
   console.error(`bazel node patches disabled. set environment BAZEL_PATCH_ROOT`);
 }
 
-patcher.subprocess(__filename, NP_SUBPROCESS_BIN_DIR);
+patcher.subprocess(__filename, NP_SUBPROCESS_NODE_DIR);
diff --git a/packages/node-patches/src/subprocess.ts b/packages/node-patches/src/subprocess.ts
@@ -3,39 +3,41 @@
 const fs = require('fs');
 const path = require('path');
 
-export const patcher = (requireScriptName: string, binDir?: string) => {
+export const patcher = (requireScriptName: string, nodeDir?: string) => {
   requireScriptName = path.resolve(requireScriptName);
-  const dir = path.dirname(requireScriptName);
+  nodeDir = nodeDir || path.join(path.dirname(requireScriptName), '_node_bin');
   const file = path.basename(requireScriptName);
-  const nodeDir = path.join(binDir || dir, '_node_bin');
 
-  if (!process.env.NP_PATCHED_NODEJS) {
-    // TODO: WINDOWS.
-    try {
-      fs.mkdirSync(nodeDir, {recursive: true});
-    } catch (e) {
-      // with node versions that don't have recursive mkdir this may throw an error.
-      if (e.code !== 'EEXIST') {
-        throw e;
-      }
+  try {
+    fs.mkdirSync(nodeDir, {recursive: true});
+  } catch (e) {
+    // with node versions that don't have recursive mkdir this may throw an error.
+    if (e.code !== 'EEXIST') {
+      throw e;
     }
-    if (process.platform == 'win32') {
-      fs.writeFileSync(path.join(nodeDir, 'node.bat'), `@if not defined DEBUG_HELPER @ECHO OFF
-set NP_PATCHED_NODEJS=${nodeDir}
+  }
+  if (process.platform == 'win32') {
+    const nodeEntry = path.join(nodeDir, 'node.bat');
+    if (!fs.existsSync(nodeEntry)) {
+      fs.writeFileSync(nodeEntry, `@if not defined DEBUG_HELPER @ECHO OFF
+set NP_SUBPROCESS_NODE_DIR=${nodeDir}
 set Path=${nodeDir};%Path%
-"${process.execPath}" --require "${requireScriptName}" %*
-        `)
-    } else {
+"${process.execPath}" ${process.env.NODE_REPOSITORY_ARGS} --require "${requireScriptName}" %*
+`);
+    }
+  } else {
+    const nodeEntry = path.join(nodeDir, 'node');
+    if (!fs.existsSync(nodeEntry)) {
       fs.writeFileSync(
-          path.join(nodeDir, 'node'), `#!/bin/bash
-export NP_PATCHED_NODEJS="${nodeDir}"
+          nodeEntry, `#!/bin/bash
+export NP_SUBPROCESS_NODE_DIR="${nodeDir}"
 export PATH="${nodeDir}":\$PATH
 if [[ ! "\${@}" =~ "${file}" ]]; then
-  exec ${process.execPath} --require "${requireScriptName}" "$@"
+  exec ${process.execPath} ${process.env.NODE_REPOSITORY_ARGS} --require "${requireScriptName}" "$@"
 else
-  exec ${process.execPath} "$@"
+  exec ${process.execPath} ${process.env.NODE_REPOSITORY_ARGS} "$@"
 fi
-  `,
+`,
           {mode: 0o777});
     }
   }
diff --git a/packages/node-patches/test/subprocess/test.ts b/packages/node-patches/test/subprocess/test.ts
@@ -1,27 +1,22 @@
 import * as assert from 'assert';
 import * as cp from 'child_process';
-import * as fs from 'fs';
 import * as os from 'os';
 import * as path from 'path';
 import * as rimraf from 'rimraf';
 
-import {patcher} from '../../src/subprocess';
-
 const requireScript = path.resolve(path.join(__dirname, '..', '..', 'register.js'));
 
-const tmp = path.join(os.tmpdir(), 'node-patches-test-tmp');
+const nodeDir = path.join(os.tmpdir(), 'node-patches-test-tmp', '_node_bin');
 
 // tslint:disable-next-line:no-any
 function assertPatched(prefix: string, arr: any) {
-  const binDir = path.join(tmp, '_node_bin');
-
   const [execPath, execArgv, argv, pathEnv] = arr;
 
   assert.deepStrictEqual(
-      path.join(binDir, 'node'), execPath,
+      path.join(nodeDir, 'node'), execPath,
       prefix + ' exec path has been rewritten to subprocess proxy');
   assert.deepStrictEqual(
-      path.join(binDir, 'node'), argv[0],
+      path.join(nodeDir, 'node'), argv[0],
       prefix + ' argv[0] has been rewritten to subprocess proxy');
 
   let found = false;
@@ -38,23 +33,19 @@ function assertPatched(prefix: string, arr: any) {
   // something.
 
   assert.deepStrictEqual(
-      binDir, pathEnv.split(path.delimiter)[0],
+      nodeDir, pathEnv.split(path.delimiter)[0],
       prefix + ' the highest priority directory in the PATH must be the node shim dir.');
 }
 
 describe('spawning child processes', () => {
-  before(() => {
-    fs.mkdirSync(tmp, {recursive: true});
-  });
-
   it('get patched if run as shell script.', () => {
-    const res = cp.execSync(`NP_SUBPROCESS_BIN_DIR=${tmp} node -r ${
+    const res = cp.execSync(`NP_SUBPROCESS_NODE_DIR=${nodeDir} node -r ${
         requireScript} -e 'console.log(JSON.stringify([process.execPath,process.execArgv,process.argv,process.env.PATH]))'`);
     assertPatched('', JSON.parse(res + ''));
   });
 
   it('overwrites spawn related variables correctly.', () => {
-    const res = cp.execSync(`NP_SUBPROCESS_BIN_DIR=${tmp} node -r ${requireScript} ${
+    const res = cp.execSync(`NP_SUBPROCESS_NODE_DIR=${nodeDir} node -r ${requireScript} ${
         path.join(__dirname, 'worker-threads-script.js')}`);
 
     const {mainThread, worker} = JSON.parse(res + '');
@@ -64,7 +55,7 @@ describe('spawning child processes', () => {
   });
 
   it('can spawn node from the shell', () => {
-    const res = cp.execSync(`NP_SUBPROCESS_BIN_DIR=${tmp} node -r ${requireScript} ${
+    const res = cp.execSync(`NP_SUBPROCESS_NODE_DIR=${nodeDir} node -r ${requireScript} ${
         path.join(__dirname, 'shell-script.js')}`);
     // TODO: this is broken if no environment is passed and a new bash is executed
     // reading only the rc files to build the environment.
@@ -77,7 +68,7 @@ describe('spawning child processes', () => {
 
   it('can spawn node from spawn', () => {
     const res = cp.execSync(
-        `NP_SUBPROCESS_BIN_DIR=${tmp} node -r ${requireScript} ${
+        `NP_SUBPROCESS_NODE_DIR=${nodeDir} node -r ${requireScript} ${
             path.join(__dirname, 'spawn-script.js')}`,
         {env: process.env});
 
@@ -86,6 +77,6 @@ describe('spawning child processes', () => {
   });
 
   after(() => {
-    rimraf.sync(tmp);
+    rimraf.sync(nodeDir);
   });
 });

Original file line number	Diff line number	Diff line change
`@@ -6,5 +6,6 @@ var theArgs = process.argv.slice(2);`
`6`	`6`
`7`	`7`	`if (theArgs.length != 0) {`
`8`	`8`	`// Non-zero exit code if the argument list is not empty`
	`9`	`+ console.error(theArgs)`
`9`	`10`	`process.exit(42)`
`10`	`11`	`}`