From 01b9ed9f8c560565d39108160d149d6b7f7bbb6a Mon Sep 17 00:00:00 2001 From: Greg Magolan Date: Fri, 5 Aug 2022 13:57:32 -0700 Subject: [PATCH] fix: set copy_data_to_bin default to True to mitigate ESM loader issue #362 --- README.md | 1 + docs/js_binary.md | 8 ++++---- examples/macro/BUILD.bazel | 2 -- js/private/js_binary.bzl | 20 ++++++++++++-------- 4 files changed, 17 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 451430060..fb24dcfcf 100644 --- a/README.md +++ b/README.md @@ -15,6 +15,7 @@ Known issues: - Doesn't support Remote Execution (RBE) due to https://github.com/bazelbuild/bazel/issues/10298. - Doesn't work with rules_docker due to https://github.com/bazelbuild/rules_pkg/issues/115#issuecomment-1137465914. - No examples yet for publishing npm packages. +- ESM imports escape the runfiles tree and the sandbox due to https://github.com/aspect-build/rules_js/issues/362 rules_js is just a part of what Aspect provides: diff --git a/docs/js_binary.md b/docs/js_binary.md index 08693a10f..5a08acbba 100644 --- a/docs/js_binary.md +++ b/docs/js_binary.md @@ -46,7 +46,7 @@ This rules requires that Bazel was run with | :------------- | :------------- | :------------- | :------------- | :------------- | | name | A unique name for this target. | Name | required | | | chdir | Working directory to run the binary or test in, relative to the workspace.

By default, js_binary runs in the root of the output tree.

To run in the directory containing the js_binary use

chdir = package_name()

(or if you're in a macro, use native.package_name())

WARNING: this will affect other paths passed to the program, either as arguments or in configuration files, which are workspace-relative.

You may need ../../ segments to re-relativize such paths to the new working directory. In a BUILD file you could do something like this to point to the output path:

python         js_binary(             ...             chdir = package_name(),             # ../.. segments to re-relative paths from the chdir back to workspace;             # add an additional 3 segments to account for running js_binary running             # in the root of the output tree             args = ["/".join([".."] * len(package_name().split("/")) + "$(rootpath //path/to/some:file)"],         )
| String | optional | "" | -| copy_data_to_bin | When True, data files and the entry_point file are copied to the Bazel output tree before being passed as inputs to runfiles.

This default to False. It should only be needed if the program manages to skirt the node fs patches and leave its runfiles and/or sandbox. In that case, setting this to True will prevent the program from following symlinks into the source tree and it will end up in the output tree instead where node_modules and other inputs the program may need will be.

Mocha, for example, has been observed to escape its runfiles & sandbox: https://github.com/aspect-build/rules_js/pull/353. | Boolean | optional | False | +| copy_data_to_bin | When True, data files and the entry_point file are copied to the Bazel output tree before being passed as inputs to runfiles.

Ideally, the default for this would be False as it is optimal, but there is a yet unresloved issue of ESM imports skirting the node fs patches and escaping the sandbox: https://github.com/aspect-build/rules_js/issues/362. This is hit in some test popular runners such as mocha, which use native import() statements (https://github.com/aspect-build/rules_js/pull/353).

A default of True will prevent program such as mocha from following symlinks into the source tree. They will escape the sandbox but they will end up in the output tree where node_modules and other inputs required will be available. With this in mind, the default will remain true until issue #362 is resolved. | Boolean | optional | True | | data | Runtime dependencies of the program.

The transitive closure of the data dependencies will be available in the .runfiles folder for this binary/test.

You can use the @bazel/runfiles npm library to access these files at runtime.

npm packages are also linked into the .runfiles/node_modules folder so they may be resolved directly from runfiles. | List of labels | optional | [] | | enable_runfiles | Whether runfiles are enabled in the current build configuration.

Typical usage of this rule is via a macro which automatically sets this attribute based on a config_setting rule. | Boolean | required | | | entry_point | The main script which is evaluated by node.js.

This is the module referenced by the require.main property in the runtime.

This must be a target that provides a single file or a DirectoryPathInfo from @aspect_bazel_lib//lib::directory_path.bzl.

See https://github.com/aspect-build/bazel-lib/blob/main/docs/directory_path.md for more info on creating a target that provides a DirectoryPathInfo. | Label | required | | @@ -58,7 +58,7 @@ This rules requires that Bazel was run with | log_level | Set the logging level.

Log from are written to stderr. They will be supressed on success when running as the tool of a js_run_binary when silent_on_success is True. In that case, they will be shown only on a build failure along with the stdout & stderr of the node tool being run. | String | optional | "error" | | node_options | Options to pass to the node.

https://nodejs.org/api/cli.html | List of strings | optional | [] | | patch_node_fs | Patch the to Node.js fs API (https://nodejs.org/api/fs.html) for this node program to prevent the program from following symlinks out of the execroot, runfiles and the sandbox.

When enabled, js_binary patches the Node.js sync and async fs API functions lstat, readlink, realpath, readdir and opendir so that the node program being run cannot resolve symlinks out of the execroot and the runfiles tree. When in the sandbox, these patches prevent the program being run from resolving symlinks out of the sandbox.

When disabled, node programs can leave the execroot, runfiles and sandbox by following symlinks which can lead to non-hermetic behavior. | Boolean | optional | True | -| preserve_symlinks_main | When True, the --preserve-symlinks-main flag is passed to node.

This prevents node from following entry script out of runfiles and the sandbox which can happen for some entry files such as .mjs where the fs node patches, which guard the runfiles and sandbox, are not used.

This flag was added in Node.js v10.2.0 (released 2018-05-23). If your node toolchain is configured to use a Node.js version older than this you'll need to set this attribute to False.

See https://nodejs.org/api/cli.html#--preserve-symlinks-main for more information. | Boolean | optional | True | +| preserve_symlinks_main | When True, the --preserve-symlinks-main flag is passed to node.

This prevents node from following an ESM entry script out of runfiles and the sandbox. This can happen for .mjs ESM entry points where the fs node patches, which guard the runfiles and sandbox, are not applied. See https://github.com/aspect-build/rules_js/issues/362 for more information. Once #362 is resolved, the default for this attribute can be set to False.

This flag was added in Node.js v10.2.0 (released 2018-05-23). If your node toolchain is configured to use a Node.js version older than this you'll need to set this attribute to False.

See https://nodejs.org/api/cli.html#--preserve-symlinks-main for more information. | Boolean | optional | True | @@ -80,7 +80,7 @@ Identical to js_binary, but usable under `bazel test`. | :------------- | :------------- | :------------- | :------------- | :------------- | | name | A unique name for this target. | Name | required | | | chdir | Working directory to run the binary or test in, relative to the workspace.

By default, js_binary runs in the root of the output tree.

To run in the directory containing the js_binary use

chdir = package_name()

(or if you're in a macro, use native.package_name())

WARNING: this will affect other paths passed to the program, either as arguments or in configuration files, which are workspace-relative.

You may need ../../ segments to re-relativize such paths to the new working directory. In a BUILD file you could do something like this to point to the output path:

python         js_binary(             ...             chdir = package_name(),             # ../.. segments to re-relative paths from the chdir back to workspace;             # add an additional 3 segments to account for running js_binary running             # in the root of the output tree             args = ["/".join([".."] * len(package_name().split("/")) + "$(rootpath //path/to/some:file)"],         )
| String | optional | "" | -| copy_data_to_bin | When True, data files and the entry_point file are copied to the Bazel output tree before being passed as inputs to runfiles.

This default to False. It should only be needed if the program manages to skirt the node fs patches and leave its runfiles and/or sandbox. In that case, setting this to True will prevent the program from following symlinks into the source tree and it will end up in the output tree instead where node_modules and other inputs the program may need will be.

Mocha, for example, has been observed to escape its runfiles & sandbox: https://github.com/aspect-build/rules_js/pull/353. | Boolean | optional | False | +| copy_data_to_bin | When True, data files and the entry_point file are copied to the Bazel output tree before being passed as inputs to runfiles.

Ideally, the default for this would be False as it is optimal, but there is a yet unresloved issue of ESM imports skirting the node fs patches and escaping the sandbox: https://github.com/aspect-build/rules_js/issues/362. This is hit in some test popular runners such as mocha, which use native import() statements (https://github.com/aspect-build/rules_js/pull/353).

A default of True will prevent program such as mocha from following symlinks into the source tree. They will escape the sandbox but they will end up in the output tree where node_modules and other inputs required will be available. With this in mind, the default will remain true until issue #362 is resolved. | Boolean | optional | True | | data | Runtime dependencies of the program.

The transitive closure of the data dependencies will be available in the .runfiles folder for this binary/test.

You can use the @bazel/runfiles npm library to access these files at runtime.

npm packages are also linked into the .runfiles/node_modules folder so they may be resolved directly from runfiles. | List of labels | optional | [] | | enable_runfiles | Whether runfiles are enabled in the current build configuration.

Typical usage of this rule is via a macro which automatically sets this attribute based on a config_setting rule. | Boolean | required | | | entry_point | The main script which is evaluated by node.js.

This is the module referenced by the require.main property in the runtime.

This must be a target that provides a single file or a DirectoryPathInfo from @aspect_bazel_lib//lib::directory_path.bzl.

See https://github.com/aspect-build/bazel-lib/blob/main/docs/directory_path.md for more info on creating a target that provides a DirectoryPathInfo. | Label | required | | @@ -92,7 +92,7 @@ Identical to js_binary, but usable under `bazel test`. | log_level | Set the logging level.

Log from are written to stderr. They will be supressed on success when running as the tool of a js_run_binary when silent_on_success is True. In that case, they will be shown only on a build failure along with the stdout & stderr of the node tool being run. | String | optional | "error" | | node_options | Options to pass to the node.

https://nodejs.org/api/cli.html | List of strings | optional | [] | | patch_node_fs | Patch the to Node.js fs API (https://nodejs.org/api/fs.html) for this node program to prevent the program from following symlinks out of the execroot, runfiles and the sandbox.

When enabled, js_binary patches the Node.js sync and async fs API functions lstat, readlink, realpath, readdir and opendir so that the node program being run cannot resolve symlinks out of the execroot and the runfiles tree. When in the sandbox, these patches prevent the program being run from resolving symlinks out of the sandbox.

When disabled, node programs can leave the execroot, runfiles and sandbox by following symlinks which can lead to non-hermetic behavior. | Boolean | optional | True | -| preserve_symlinks_main | When True, the --preserve-symlinks-main flag is passed to node.

This prevents node from following entry script out of runfiles and the sandbox which can happen for some entry files such as .mjs where the fs node patches, which guard the runfiles and sandbox, are not used.

This flag was added in Node.js v10.2.0 (released 2018-05-23). If your node toolchain is configured to use a Node.js version older than this you'll need to set this attribute to False.

See https://nodejs.org/api/cli.html#--preserve-symlinks-main for more information. | Boolean | optional | True | +| preserve_symlinks_main | When True, the --preserve-symlinks-main flag is passed to node.

This prevents node from following an ESM entry script out of runfiles and the sandbox. This can happen for .mjs ESM entry points where the fs node patches, which guard the runfiles and sandbox, are not applied. See https://github.com/aspect-build/rules_js/issues/362 for more information. Once #362 is resolved, the default for this attribute can be set to False.

This flag was added in Node.js v10.2.0 (released 2018-05-23). If your node toolchain is configured to use a Node.js version older than this you'll need to set this attribute to False.

See https://nodejs.org/api/cli.html#--preserve-symlinks-main for more information. | Boolean | optional | True | diff --git a/examples/macro/BUILD.bazel b/examples/macro/BUILD.bazel index 9b8b5a9ab..9933387f6 100644 --- a/examples/macro/BUILD.bazel +++ b/examples/macro/BUILD.bazel @@ -10,8 +10,6 @@ npm_link_all_packages(name = "node_modules") mocha_test( name = "test", srcs = ["test.js"], - # Work-around for mocha leaving the runfiles & sandbox - copy_data_to_bin = True, ) bzl_library( diff --git a/js/private/js_binary.bzl b/js/private/js_binary.bzl index 146af898e..412275e57 100644 --- a/js/private/js_binary.bzl +++ b/js/private/js_binary.bzl @@ -159,8 +159,10 @@ _ATTRS = { "preserve_symlinks_main": attr.bool( doc = """When True, the --preserve-symlinks-main flag is passed to node. - This prevents node from following entry script out of runfiles and the sandbox which can happen for some entry - files such as `.mjs` where the fs node patches, which guard the runfiles and sandbox, are not used. + This prevents node from following an ESM entry script out of runfiles and the sandbox. This can happen for `.mjs` + ESM entry points where the fs node patches, which guard the runfiles and sandbox, are not applied. + See https://github.com/aspect-build/rules_js/issues/362 for more information. Once #362 is resolved, + the default for this attribute can be set to False. This flag was added in Node.js v10.2.0 (released 2018-05-23). If your node toolchain is configured to use a Node.js version older than this you'll need to set this attribute to False. @@ -173,14 +175,16 @@ _ATTRS = { doc = """When True, data files and the entry_point file are copied to the Bazel output tree before being passed as inputs to runfiles. - This default to False. It should only be needed if the program manages to skirt the node fs patches and leave - its runfiles and/or sandbox. In that case, setting this to True will prevent the program from following symlinks - into the source tree and it will end up in the output tree instead where node_modules and other inputs the - program may need will be. + Ideally, the default for this would be False as it is optimal, but there is a yet unresloved issue of ESM imports + skirting the node fs patches and escaping the sandbox: https://github.com/aspect-build/rules_js/issues/362. + This is hit in some test popular runners such as mocha, which use native `import()` statements + (https://github.com/aspect-build/rules_js/pull/353). - Mocha, for example, has been observed to escape its runfiles & sandbox: https://github.com/aspect-build/rules_js/pull/353. + A default of True will prevent program such as mocha from following symlinks into the source tree. They will + escape the sandbox but they will end up in the output tree where node_modules and other inputs required will be + available. With this in mind, the default will remain true until issue #362 is resolved. """, - default = False, + default = True, ), "_launcher_template": attr.label( default = Label("//js/private:js_binary.sh.tpl"),