From 56c6d7d54f7b346bfe1bbd9cf4927f1d143f3529 Mon Sep 17 00:00:00 2001 From: Jordan Mele Date: Tue, 2 Jan 2024 16:43:19 +1100 Subject: [PATCH 1/6] Limit exposure of implementation detail env vars to NodeJS --- internal/node/launcher.sh | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/internal/node/launcher.sh b/internal/node/launcher.sh index df7ce89f..07d7b503 100755 --- a/internal/node/launcher.sh +++ b/internal/node/launcher.sh @@ -13,6 +13,9 @@ # See the License for the specific language governing permissions and # limitations under the License. +# Capture initial env var state +readarray -d '' INIT_ENV_ARGS < <(env -0) + # --- begin runfiles.bash initialization v2 --- # Copy-pasted from the Bazel Bash runfiles library v2. set -uo pipefail; f=build_bazel_rules_nodejs/third_party/github.com/bazelbuild/bazel/tools/bash/runfiles/runfiles.bash @@ -390,14 +393,29 @@ if [[ -n "$NODE_WORKING_DIR" ]]; then fi set +e +# Prepare NodeJS args plus a cleaned environment +spawn_args=( + -i + "${INIT_ENV_ARGS[@]}" + # Arguments for child NodeJS processes + ${NODE_REPOSITORY_ARGS+"NODE_REPOSITORY_ARGS=${NODE_REPOSITORY_ARGS}"} + # For coverage support + ${NODE_V8_COVERAGE+"NODE_V8_COVERAGE=${NODE_V8_COVERAGE}"} + "${node}" + ${LAUNCHER_NODE_OPTIONS[@]+"${LAUNCHER_NODE_OPTIONS[@]}"} + ${USER_NODE_OPTIONS[@]+"${USER_NODE_OPTIONS[@]}"} + "${MAIN}" + ${ARGS[@]+"${ARGS[@]}"} +) + if [[ -n "${STDOUT_CAPTURE}" ]] && [[ -n "${STDERR_CAPTURE}" ]]; then - "${node}" ${LAUNCHER_NODE_OPTIONS[@]+"${LAUNCHER_NODE_OPTIONS[@]}"} ${USER_NODE_OPTIONS[@]+"${USER_NODE_OPTIONS[@]}"} "${MAIN}" ${ARGS[@]+"${ARGS[@]}"} <&0 >$STDOUT_CAPTURE 2>$STDERR_CAPTURE & + env "${spawn_args[@]}" <&0 >$STDOUT_CAPTURE 2>$STDERR_CAPTURE & elif [[ -n "${STDOUT_CAPTURE}" ]]; then - "${node}" ${LAUNCHER_NODE_OPTIONS[@]+"${LAUNCHER_NODE_OPTIONS[@]}"} ${USER_NODE_OPTIONS[@]+"${USER_NODE_OPTIONS[@]}"} "${MAIN}" ${ARGS[@]+"${ARGS[@]}"} <&0 >$STDOUT_CAPTURE & + env "${spawn_args[@]}" <&0 >$STDOUT_CAPTURE & elif [[ -n "${STDERR_CAPTURE}" ]]; then - "${node}" ${LAUNCHER_NODE_OPTIONS[@]+"${LAUNCHER_NODE_OPTIONS[@]}"} ${USER_NODE_OPTIONS[@]+"${USER_NODE_OPTIONS[@]}"} "${MAIN}" ${ARGS[@]+"${ARGS[@]}"} <&0 2>$STDERR_CAPTURE & + env "${spawn_args[@]}" <&0 2>$STDERR_CAPTURE & else - "${node}" ${LAUNCHER_NODE_OPTIONS[@]+"${LAUNCHER_NODE_OPTIONS[@]}"} ${USER_NODE_OPTIONS[@]+"${USER_NODE_OPTIONS[@]}"} "${MAIN}" ${ARGS[@]+"${ARGS[@]}"} <&0 & + env "${spawn_args[@]}" <&0 & fi readonly child=$! From 8704afe02091dc1d9cde74dc9b6d5aa0999475b3 Mon Sep 17 00:00:00 2001 From: Jordan Mele Date: Thu, 18 Jan 2024 10:36:16 +1100 Subject: [PATCH 2/6] Fix for `NM_SYMLINKS` --- internal/node/launcher.sh | 7 ++++++- internal/node/node.bzl | 5 +---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/internal/node/launcher.sh b/internal/node/launcher.sh index 07d7b503..805201eb 100755 --- a/internal/node/launcher.sh +++ b/internal/node/launcher.sh @@ -95,6 +95,9 @@ export RUNFILES TEMPLATED_env_vars +# Metadata for recreation of node_modules symlinks if required +export NM_SYMLINKS="$(mktemp -d)/nm-symlinks.json" + # Note: for debugging it is useful to see what files are actually present # This redirects to stderr so it doesn't interfere with Bazel's worker protocol # find . -name thingImLookingFor 1>&2 @@ -395,7 +398,9 @@ set +e # Prepare NodeJS args plus a cleaned environment spawn_args=( - -i + --ignore-environment + # Expose `NM_SYMLINKS` so persistent workers can recreate side effects if required + ${NM_SYMLINKS+"NM_SYMLINKS=${NM_SYMLINKS}"} "${INIT_ENV_ARGS[@]}" # Arguments for child NodeJS processes ${NODE_REPOSITORY_ARGS+"NODE_REPOSITORY_ARGS=${NODE_REPOSITORY_ARGS}"} diff --git a/internal/node/node.bzl b/internal/node/node.bzl index 46f28400..e6b7389e 100755 --- a/internal/node/node.bzl +++ b/internal/node/node.bzl @@ -191,10 +191,7 @@ def _nodejs_binary_impl(ctx): _write_loader_script(ctx) - # Provide the target name as an environment variable avaiable to all actions for the - # runfiles helpers to use. - env_vars = "export BAZEL_TARGET=%s\n" % ctx.label - env_vars += """export NM_SYMLINKS="$(mktemp -d)/nm-symlinks.json"\n""" + env_vars = "" # Add all env vars from the ctx attr for [key, value] in ctx.attr.env.items(): From 41be7fffcdbf0e780860ef05a70b85b2088a8f2c Mon Sep 17 00:00:00 2001 From: Jordan Mele Date: Thu, 18 Jan 2024 10:48:58 +1100 Subject: [PATCH 3/6] No bash profile, no shell specific vars --- internal/node/launcher.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/node/launcher.sh b/internal/node/launcher.sh index 805201eb..3e27e11d 100755 --- a/internal/node/launcher.sh +++ b/internal/node/launcher.sh @@ -1,4 +1,4 @@ -#!/usr/bin/env bash +#!/usr/bin/env bash --noprofile # Copyright 2017 The Bazel Authors. All rights reserved. # # Licensed under the Apache License, Version 2.0 (the "License"); @@ -13,8 +13,8 @@ # See the License for the specific language governing permissions and # limitations under the License. -# Capture initial env var state -readarray -d '' INIT_ENV_ARGS < <(env -0) +# Capture initial env var state, sans some shell vars +readarray -d '' INIT_ENV_ARGS < <(env -0 -u SHLVL -u _ -u PWD) # --- begin runfiles.bash initialization v2 --- # Copy-pasted from the Bazel Bash runfiles library v2. From bb420f6abc0549162600bec17f3056549964ee52 Mon Sep 17 00:00:00 2001 From: Jordan Mele Date: Thu, 18 Jan 2024 10:57:18 +1100 Subject: [PATCH 4/6] Disabling bash profile unnecessary --- internal/node/launcher.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/node/launcher.sh b/internal/node/launcher.sh index 3e27e11d..e1c9f7c7 100755 --- a/internal/node/launcher.sh +++ b/internal/node/launcher.sh @@ -1,4 +1,4 @@ -#!/usr/bin/env bash --noprofile +#!/usr/bin/env bash # Copyright 2017 The Bazel Authors. All rights reserved. # # Licensed under the Apache License, Version 2.0 (the "License"); From cc7ea12d948c32ae9dac8917e5d70ceed3f22d55 Mon Sep 17 00:00:00 2001 From: Jordan Mele Date: Thu, 8 Feb 2024 22:15:22 +1100 Subject: [PATCH 5/6] Fixes --- internal/node/launcher.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/node/launcher.sh b/internal/node/launcher.sh index e1c9f7c7..e6ce6176 100755 --- a/internal/node/launcher.sh +++ b/internal/node/launcher.sh @@ -406,6 +406,10 @@ spawn_args=( ${NODE_REPOSITORY_ARGS+"NODE_REPOSITORY_ARGS=${NODE_REPOSITORY_ARGS}"} # For coverage support ${NODE_V8_COVERAGE+"NODE_V8_COVERAGE=${NODE_V8_COVERAGE}"} + # For NodeJS patches + ${RUNFILES_DIR+"RUNFILES_DIR=${RUNFILES_DIR}"} + ${RUNFILES+"RUNFILES=${RUNFILES}"} + ${BAZEL_PATCH_ROOTS+"BAZEL_PATCH_ROOTS=${BAZEL_PATCH_ROOTS}"} "${node}" ${LAUNCHER_NODE_OPTIONS[@]+"${LAUNCHER_NODE_OPTIONS[@]}"} ${USER_NODE_OPTIONS[@]+"${USER_NODE_OPTIONS[@]}"} From 2e832df27a6adabdca31768c91fe905e8f7ab392 Mon Sep 17 00:00:00 2001 From: Jordan Mele Date: Thu, 8 Feb 2024 22:24:21 +1100 Subject: [PATCH 6/6] Use `bash` from `PATH` in `pkg_npm` --- internal/pkg_npm/pkg_npm.bzl | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/pkg_npm/pkg_npm.bzl b/internal/pkg_npm/pkg_npm.bzl index 56c758a4..ea8ca6cd 100755 --- a/internal/pkg_npm/pkg_npm.bzl +++ b/internal/pkg_npm/pkg_npm.bzl @@ -258,6 +258,9 @@ def create_package(ctx, deps_files, nested_packages): inputs = inputs, outputs = [package_dir], arguments = [args], + # Without this, `PATH` is unset and OS specified defaults are used. + # e.g. Bash 3.2 on macoS Sonoma (via `/bin:/usr/bin`) + use_default_shell_env = True, ) _create_npm_scripts(ctx) @@ -290,6 +293,9 @@ def _create_npm_scripts(ctx): "no-remote-exec": "", "no-sandbox": "", }, + # Without this, `PATH` is unset and OS specified defaults are used. + # e.g. Bash 3.2 on macoS Sonoma (via `/bin:/usr/bin`) + use_default_shell_env = True, ) def _pkg_npm(ctx):