fix(builtin): don't allow symlinks to escape or enter bazel managed node_module folders #1800

gregmagolan · 2020-04-07T01:54:42Z

No description provided.

alexeagle

wow, I kinda understand. I wish @soldair was available for review but I think it's good.

Would be nice to have that added test coverage but I guess there's integration test coverage coming from some downstream usecase like jest?

alexeagle · 2020-04-08T23:54:44Z

internal/node/launcher.sh

+# 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"


seems like this doesn't work if your node_modules is in a subdirectory, which we support - but I think that's what BAZEL_NODE_MODULES_ROOT is doing below? maybe this needs a comment that we do this in a catch-all case where we don't have a BAZEL_NODE_MODULES_ROOT - but then why does that happen?

BAZEL_NODE_MODULES_ROOT is the path to the external workspace such as npm/node_modules.
This path is actually the path to the linker symlink node_modules under execroot which should work regardless of where node_modules is.

BAZEL_NODE_MODULES_ROOT is always set in templated_args above:

env_vars += """if [[ -z "${BAZEL_NODE_MODULES_ROOT:-}" ]]; then export BAZEL_NODE_MODULES_ROOT=%s fi """ % node_modules_root

so below is just a sanity check

buildsize · 2020-04-09T03:22:48Z

File name	Previous Size	New Size	Change
release.tar.gz	956.81 KB	957.8 KB	1021 bytes (0%)

buildsize · 2020-04-09T03:22:48Z

File name	Previous Size	New Size	Change
release.tar.gz	968.29 KB	969.24 KB	973 bytes (0%)

internal/node/node_patches.js

Snapshot for bazel-contrib/rules_nodejs#1800, taken from https://github.com/aspect-dev/rules_nodejs-builds/tree/labs. Contains fixes related to the symlink behaviour inside of bazel. Without it, webpack needs to be configured to be aware of symlinks and preserve the paths.

Issue bazel-contrib#1803 was fixed by bazel-contrib#1800 which is already landed. bazel-contrib#1805 also fixes via a different mechanism and will also land soon.

Issue bazel-contrib#1813 was fixed by bazel-contrib#1800 which is already landed. bazel-contrib#1805 also fixes via a different mechanism and will also land soon.

Issue #1813 was fixed by #1800 which is already landed. #1805 also fixes via a different mechanism and will also land soon.

…ode_module folders

Snapshot for bazel-contrib/rules_nodejs#1800, taken from https://github.com/aspect-dev/rules_nodejs-builds/tree/labs. Contains fixes related to the symlink behaviour inside of bazel. Without it, webpack needs to be configured to be aware of symlinks and preserve the paths.

soldair · 2020-04-14T17:16:52Z

\o sorry for the delay. i somehow missed most of my communication last week

Snapshot for bazel-contrib/rules_nodejs#1800, taken from https://github.com/aspect-dev/rules_nodejs-builds/tree/labs. Contains fixes related to the symlink behaviour inside of bazel. Without it, webpack needs to be configured to be aware of symlinks and preserve the paths.

…mmits) Squashed commits: [241a8bc] build: use ts_library macro with common defaults [12b3f3c] build: use @bazel/bazelisk [22792f3] ci: update required status checks [d7dbbab] build: use no-remote-exec tag so test still runs in sandbox Turns out there is a linker bug with no sandbox. [3eeab18] build: run tests depending on webdriver-manager locally [2cf4d32] docs: add bazel jasmine_node_test debug information [c0d3912] build: update to rules_nodejs 1.6.0 [4f0cb97] build: add webdriver-manager update & ngcc to postinstall This is required so that yarn_install can add all generated & downloaded files to the generated bazel filegroups [be1f0ed] ci: use xlarge for bazel tests [27f94da] test: remove non-bazel test setup [d085d5e] build: update yarn lock [3bc6c54] test(@angular/cli): add npm_integration_test [c83a90f] build: add missing npm_package_archive targets [8f72222] test(@angular-devkit/build-ng-packagr): build and test with Bazel [c56b83e] test(@angular-devkit/build-angular): build and test with Bazel [a7e827c] test(@angular-devkit/build-webpack): build and test with Bazel [4e7bcd3] build: use rules_nodejs snapshot Snapshot for bazel-contrib/rules_nodejs#1800, taken from https://github.com/aspect-dev/rules_nodejs-builds/tree/labs. Contains fixes related to the symlink behaviour inside of bazel. Without it, webpack needs to be configured to be aware of symlinks and preserve the paths. [4507c05] build: don't copy root package test folders [8b79f9d] fix(@ngtools/webpack): remove internal markers With these in, we can't access the properties from other Bazel targets. [e1bc6e1] fix(@ngtools/webpack): export VirtualFileSystemDecorator type We shouldn't need to export this, but webpack-rollup-loader uses it. [ffce320] refactor: rename toplevel BUILD to BUILD.bazel [90e5429] build: also validade BUILD.bazel files

gregmagolan requested review from alexeagle and soldair as code owners April 7, 2020 01:54

googlebot added the cla: yes label Apr 7, 2020

gregmagolan force-pushed the node_symlinks_fixes branch from 88026cc to 4b648d5 Compare April 7, 2020 02:50

gregmagolan added the package:builtin label Apr 7, 2020

gregmagolan force-pushed the node_symlinks_fixes branch from 4b648d5 to 94eeb00 Compare April 7, 2020 04:47

gregmagolan mentioned this pull request Apr 7, 2020

fix(builtin): linker no longer makes node_modules symlink to the root of the workspace output tree #1797

Merged

gregmagolan force-pushed the node_symlinks_fixes branch from 94eeb00 to 37038c0 Compare April 8, 2020 01:35

alexeagle approved these changes Apr 8, 2020

View reviewed changes

gregmagolan force-pushed the node_symlinks_fixes branch from 37038c0 to c1f2f29 Compare April 9, 2020 00:14

gregmagolan mentioned this pull request Apr 9, 2020

fix(builtin): always symlink node_modules at execroot/my_wksp/node_modules even when running in runfiles #1805

Merged

gregmagolan force-pushed the node_symlinks_fixes branch 2 times, most recently from 10bd951 to ceebe1a Compare April 9, 2020 03:18

jbedard reviewed Apr 9, 2020

View reviewed changes

internal/node/node_patches.js Outdated Show resolved Hide resolved

gregmagolan force-pushed the node_symlinks_fixes branch 3 times, most recently from 00a4c20 to 23c1500 Compare April 9, 2020 16:25

This was referenced Apr 9, 2020

Jest's expect toBeInstanceOf fails in rules_nodejs #1813

Closed

test(builtin): add test for issue 1813 #1814

Merged

gregmagolan force-pushed the node_symlinks_fixes branch from 23c1500 to 330bebf Compare April 9, 2020 17:02

gregmagolan force-pushed the node_symlinks_fixes branch from 330bebf to 3212252 Compare April 9, 2020 19:13

gregmagolan force-pushed the node_symlinks_fixes branch from 3212252 to c380f0a Compare April 9, 2020 19:21

gregmagolan force-pushed the node_symlinks_fixes branch 2 times, most recently from 46c0343 to a5e6da4 Compare April 9, 2020 20:42

gregmagolan force-pushed the node_symlinks_fixes branch from a5e6da4 to e55d3c3 Compare April 9, 2020 22:03

gregmagolan added a commit that referenced this pull request Apr 9, 2020

test(builtin): add test for issue 1813 (#1814)

2822a8c

Issue #1813 was fixed by #1800 which is already landed. #1805 also fixes via a different mechanism and will also land soon.

gregmagolan force-pushed the node_symlinks_fixes branch 6 times, most recently from 9773808 to 0209cff Compare April 10, 2020 00:04

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

0c16611

…ode_module folders

gregmagolan force-pushed the node_symlinks_fixes branch from 0209cff to 0c16611 Compare April 10, 2020 00:10

gregmagolan merged commit 4554ce7 into bazel-contrib:master Apr 10, 2020

gregmagolan mentioned this pull request Apr 10, 2020

fix(builtin): don't allow symlinks to escape into duplicate node_modules tree in runfiles #1505

Closed

12 tasks

gregmagolan mentioned this pull request Apr 11, 2020

fix(builtin): fix node patches lstat short-circuit logic #1818

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(builtin): don't allow symlinks to escape or enter bazel managed node_module folders #1800

fix(builtin): don't allow symlinks to escape or enter bazel managed node_module folders #1800

gregmagolan commented Apr 7, 2020 •

edited

Loading

alexeagle left a comment

alexeagle Apr 8, 2020

gregmagolan Apr 9, 2020

gregmagolan Apr 9, 2020 •

edited

Loading

buildsize bot commented Apr 9, 2020 •

edited

Loading

buildsize bot commented Apr 9, 2020

soldair commented Apr 14, 2020

fix(builtin): don't allow symlinks to escape or enter bazel managed node_module folders #1800

fix(builtin): don't allow symlinks to escape or enter bazel managed node_module folders #1800

Conversation

gregmagolan commented Apr 7, 2020 • edited Loading

alexeagle left a comment

Choose a reason for hiding this comment

alexeagle Apr 8, 2020

Choose a reason for hiding this comment

gregmagolan Apr 9, 2020

Choose a reason for hiding this comment

gregmagolan Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

buildsize bot commented Apr 9, 2020 • edited Loading

buildsize bot commented Apr 9, 2020

soldair commented Apr 14, 2020

gregmagolan commented Apr 7, 2020 •

edited

Loading

gregmagolan Apr 9, 2020 •

edited

Loading

buildsize bot commented Apr 9, 2020 •

edited

Loading