Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

esbuild leaves the sandbox #58

Closed
gregmagolan opened this issue Aug 9, 2022 · 13 comments · Fixed by #160
Closed

esbuild leaves the sandbox #58

gregmagolan opened this issue Aug 9, 2022 · 13 comments · Fixed by #160
Labels
bug Something isn't working
Milestone

Comments

@gregmagolan
Copy link
Member

Since we turn preserveSymlinks off so that node_modules resolution works with the rules_js symlinks node_modules tree, esbuild will currently leave the sandbox by following symlinks. The rules_js node fs patches don't apply to the golang binary. There is currently no hook in esbuild where we can customize module resolution so that esbuild stays in the bazel sandbox.

If bundling .js files that come from transpiled sources, it is currently recommended to set the following configuration

config = {
        "resolveExtensions": [".js"],
    },

so that esbuild doesn't pick up and re-transpile the .ts sources it can find outside of the sandbox and seems to prefer (#57 (comment)).

@matthewjh
Copy link

@gregmagolan do you guys have a plan for how to deal with this? I remember you mentioning experimental_allow_unresolved_symlinks, but I'm not familiar with that and wonder what the long-term solution looks like.

Having some pretty major issues with the esbuild rules as-is, due to the sandbox escape. One is that, on developer machines, if an NPM package is missing from the inputs, it will probably be resolved from the PNPM-created node_modules folder in the workspace created by the developer running pnpm install. You only need one of these to pull in a bunch of transitive modules from the pnpm install rather than the rules_js-managed ones, and then end up with a load of weird bugs due to duplicated modules and different ctor refs breaking instanceOf checks. Thankfully, this should not be an issue on CI. There might be a simple config or even a lightweight plugin we can use for ESBuild to block this while the longer-term solution unfolds.

@gregmagolan
Copy link
Member Author

It's a hard one to fix. We'll probably have to make changes to esbuild itself to expose the hooks needed to prevent it from following symlinks out of the sandbox. experimental_allow_unresolved_symlinks won't affect esbuild's behaviour. Adding the hook to esbuild is the plan for now but its not high on our priority list at the moment since its non-trivial and we don't have funding for it.

@alexeagle
Copy link
Member

I wonder if we should file a bug on esbuild asking for what we want. Unlikely, but never hurts to ask?

@gonzojive
Copy link

gonzojive commented Jan 13, 2023

I wonder if this is the same issue causing building //lib in this repo to succeed when //location:location_ts_proto is built first but fails if //location:location_ts_proto hasn't yet been built.

esbuild(
    name = "lib",
    entry_point = "program.mjs",
    deps = [
        #"//location:location_ts_proto", # a real dependency of program.mjs that we leave out, and yet still esbuild succeeds.
        "//:node_modules/google-protobuf",
    ],
)

@gonzojive
Copy link

gonzojive commented Jan 15, 2023

Turning on preserve_symlinks does seem to resolve the issue for me. It is unclear to me precisely why preserving symlinks cannot be enabled. It seems like every tool like esbuild will need to be patched if preserve_symlinks is set to false when the tool is invoked.

In any case, I tried to modify the launcher.js with an onResolved plugin based on #32. I don't think the plugin hook has enough information to fix the problem. The resolved import path and set of files used to perform the resolution is not passed to the plugin, so it seems the plugin would have to implement its own import resolution (traversing node_modules, reasoning about file extensions, etc.).

esbuild seems to use its own file system library as sort of a virtual file system. It seems that could be modified to hide non-sandbox files from esbuild's resolver. https://pkg.go.dev/github.com/evanw/[email protected]/internal/fs Maybe a simple hook like isVisible func(path) bool would be sufficient for a Go plugin.

It seems the desired behavior (when preserveSymlinks=off) is to have a file system that consists of

  1. every sandbox file, directory, and symlink. I think this is $BAZEL_BINDIR/**
  2. every file that each of the symlinks from the sandbox resolves to

... naively, I would expect bazel to provide this functionality as part of the sandbox it sets up. However, it seems the sandbox environment allows reading files from any path on disk.


FYI, here some debug output from the onResolve plugin. You can see how the final resolved path isn't passed to the plugin.

resolved args {
  path: './program.mjs',
  importer: '',
  namespace: 'file',
  resolveDir: '/home/red/.cache/bazel/_bazel_red/9f975204b5973dd8a05172c6f645e273/sandbox/linux-sandbox/1867/execroot/__main__/bazel-out/k8-fastbuild/bin',
  kind: 'entry-point',
  pluginData: undefined
}; sandbox files = [{"path":"lib.args.json","isSymbolicLink":false,"realPath":"lib.args.json","realPathResolved":"/home/red/.cache/bazel/_bazel_red/9f975204b5973dd8a05172c6f645e273/sandbox/linux-sandbox/1867/execroot/__main__/bazel-out/k8-fastbuild/bin/lib.args.json"},{"path":"lib_sandbox.txt","isSymbolicLink":false,"realPath":"lib_sandbox.txt","realPathResolved":"/home/red/.cache/bazel/_bazel_red/9f975204b5973dd8a05172c6f645e273/sandbox/linux-sandbox/1867/execroot/__main__/bazel-out/k8-fastbuild/bin/lib_sandbox.txt"},{"path":"program.mjs","isSymbolicLink":false,"realPath":"program.mjs","realPathResolved":"/home/red/.cache/bazel/_bazel_red/9f975204b5973dd8a05172c6f645e273/sandbox/linux-sandbox/1867/execroot/__main__/bazel-out/k8-fastbuild/bin/program.mjs"}]
resolved args {
  path: './location/location_pb.mjs',
  importer: '/home/red/.cache/bazel/_bazel_red/9f975204b5973dd8a05172c6f645e273/execroot/__main__/bazel-out/k8-fastbuild/bin/program.mjs',
  namespace: 'file',
  resolveDir: '/home/red/.cache/bazel/_bazel_red/9f975204b5973dd8a05172c6f645e273/execroot/__main__/bazel-out/k8-fastbuild/bin',
  kind: 'import-statement',
  pluginData: undefined
}; sandbox files = [{"path":"lib.args.json","isSymbolicLink":false,"realPath":"lib.args.json","realPathResolved":"/home/red/.cache/bazel/_bazel_red/9f975204b5973dd8a05172c6f645e273/sandbox/linux-sandbox/1867/execroot/__main__/bazel-out/k8-fastbuild/bin/lib.args.json"},{"path":"lib_sandbox.txt","isSymbolicLink":false,"realPath":"lib_sandbox.txt","realPathResolved":"/home/red/.cache/bazel/_bazel_red/9f975204b5973dd8a05172c6f645e273/sandbox/linux-sandbox/1867/execroot/__main__/bazel-out/k8-fastbuild/bin/lib_sandbox.txt"},{"path":"program.mjs","isSymbolicLink":false,"realPath":"program.mjs","realPathResolved":"/home/red/.cache/bazel/_bazel_red/9f975204b5973dd8a05172c6f645e273/sandbox/linux-sandbox/1867/execroot/__main__/bazel-out/k8-fastbuild/bin/program.mjs"}]
resolved args {
  path: 'google-protobuf',
  importer: '/home/red/.cache/bazel/_bazel_red/9f975204b5973dd8a05172c6f645e273/execroot/__main__/bazel-out/k8-fastbuild/bin/location/location_pb.mjs',
  namespace: 'file',
  resolveDir: '/home/red/.cache/bazel/_bazel_red/9f975204b5973dd8a05172c6f645e273/execroot/__main__/bazel-out/k8-fastbuild/bin/location',
  kind: 'import-statement',
  pluginData: undefined
}; sandbox files = [{"path":"lib.args.json","isSymbolicLink":false,"realPath":"lib.args.json","realPathResolved":"/home/red/.cache/bazel/_bazel_red/9f975204b5973dd8a05172c6f645e273/sandbox/linux-sandbox/1867/execroot/__main__/bazel-out/k8-fastbuild/bin/lib.args.json"},{"path":"lib_sandbox.txt","isSymbolicLink":false,"realPath":"lib_sandbox.txt","realPathResolved":"/home/red/.cache/bazel/_bazel_red/9f975204b5973dd8a05172c6f645e273/sandbox/linux-sandbox/1867/execroot/__main__/bazel-out/k8-fastbuild/bin/lib_sandbox.txt"},{"path":"program.mjs","isSymbolicLink":false,"realPath":"program.mjs","realPathResolved":"/home/red/.cache/bazel/_bazel_red/9f975204b5973dd8a05172c6f645e273/sandbox/linux-sandbox/1867/execroot/__main__/bazel-out/k8-fastbuild/bin/program.mjs"}]

@gonzojive
Copy link

On second thought to the above comment, it seems the "onLoad" plugin callback might have enough information to catch sandbox escapes. https://esbuild.github.io/plugins/#on-load-arguments

@alexeagle
Copy link
Member

I think adding a capability to esbuild makes sense, if the maintainers accept it. That's how we get other tools to behave how we expect under Bazel.

I imagine another answer is just a more hermetic sandbox. You could try the docker strategy for all esbuild actions for example, or remote if you have access to a RBE cluster. Maybe we should try that and add to the docs.

gonzojive added a commit to gonzojive/rules_esbuild that referenced this issue Jan 15, 2023
This implementation uses an OnLoad plugin to catch when a file is loaded that is
not in an allowlist of files. The allowlist is all the files within the
BAZEL_BINDIR and all of the symlink targets of those files.

This may not prevent all sandbox escaping modes. The esbuild Go code may still
access unsandboxed files in the course of loading files that are in the sanbox.

Addresses aspect-build#58.
gonzojive added a commit to gonzojive/rules_esbuild that referenced this issue Jan 15, 2023
This implementation uses an OnLoad plugin to catch when a file is loaded that is
not in an allowlist of files. The allowlist is all the files within the
BAZEL_BINDIR and all of the symlink targets of those files.

This may not prevent all sandbox escaping modes. The esbuild Go code may still
access unsandboxed files in the course of loading files that are in the sanbox.

Addresses aspect-build#58 and requires
aspect-build/rules_js#793 to work properly.
gonzojive added a commit to gonzojive/rules_esbuild that referenced this issue Jan 15, 2023
This implementation uses an OnLoad plugin to catch when a file is loaded that is
not in an allowlist of files. The allowlist is all the files within the
BAZEL_BINDIR and all of the symlink targets of those files.

This may not prevent all sandbox escaping modes. The esbuild Go code may still
access unsandboxed files in the course of loading files that are in the sanbox.

Addresses aspect-build#58 and requires
aspect-build/rules_js#793 to work properly.
gonzojive added a commit to gonzojive/rules_esbuild that referenced this issue Jan 15, 2023
This implementation uses an OnLoad plugin to catch when a file is loaded that is
not in an allowlist of files. The allowlist is all the files within the
BAZEL_BINDIR and all of the symlink targets of those files.

This may not prevent all sandbox escaping modes. The esbuild Go code may still
access unsandboxed files in the course of loading files that are in the sanbox.

Addresses aspect-build#58 and requires
aspect-build/rules_js#793 to work properly.
@gonzojive
Copy link

I think #112 fixes this to some extent. I'm guessing there are some edge cases that it doesn't handle, like if esbuild loads a package.json file that's not in the sandbox even though the .js files for the package.json are.

@gregmagolan gregmagolan added funding needed Contribute to https://opencollective.com/aspect-build and removed need: funding labels Feb 6, 2023
@alexeagle alexeagle added this to the 1.0 milestone Mar 27, 2023
@gregmagolan gregmagolan removed the funding needed Contribute to https://opencollective.com/aspect-build label Apr 8, 2023
jbedard added a commit that referenced this issue Jun 7, 2023
By default esbuild uses any local tsconfig.json file. With sandboxing issues (#58)
this may unexpectedly cause `esbuild_bundle` to pickup any local tsconfig.json.
jbedard added a commit that referenced this issue Jun 8, 2023
By default esbuild uses any local tsconfig.json file. With sandboxing issues (#58)
this may unexpectedly cause `esbuild_bundle` to pickup any local tsconfig.json.
jbedard added a commit that referenced this issue Jun 8, 2023
By default esbuild uses any local tsconfig.json file. With sandboxing issues (#58)
this may unexpectedly cause `esbuild_bundle` to pickup any local tsconfig.json.

A custom tsconfig can now be set using `esbuild_bundle(tsconfig)`. By default an empty tsconfig is used to prevent unexpected sandboxing issues.
jbedard added a commit that referenced this issue Jun 8, 2023
By default esbuild uses any local tsconfig.json file. With sandboxing issues (#58)
this may unexpectedly cause `esbuild_bundle` to pickup any local tsconfig.json.

A custom tsconfig can now be set using `esbuild_bundle(tsconfig)`. By default an empty tsconfig is used to prevent unexpected sandboxing issues.
jbedard added a commit that referenced this issue Jun 8, 2023
By default esbuild uses any local tsconfig.json file. With sandboxing issues (#58)
this may unexpectedly cause `esbuild_bundle` to pickup any local tsconfig.json.

A custom tsconfig can now be set using `esbuild_bundle(tsconfig)`. By default an empty tsconfig is used to prevent unexpected sandboxing issues.
jbedard added a commit that referenced this issue Jun 9, 2023
By default esbuild uses any local tsconfig.json file. With sandboxing issues (#58)
this may unexpectedly cause `esbuild_bundle` to pickup any local tsconfig.json.

A custom tsconfig can now be set using `esbuild_bundle(tsconfig)`. By default an empty tsconfig is used to prevent unexpected sandboxing issues.
jbedard added a commit that referenced this issue Jun 9, 2023
By default esbuild uses any local tsconfig.json file. With sandboxing issues (#58)
this may unexpectedly cause `esbuild_bundle` to pickup any local tsconfig.json.

A custom tsconfig can now be set using `esbuild_bundle(tsconfig)`. By default an empty tsconfig is used to prevent unexpected sandboxing issues.
jbedard added a commit that referenced this issue Jun 13, 2023
By default esbuild uses any local tsconfig.json file. With sandboxing issues (#58)
this may unexpectedly cause `esbuild_bundle` to pickup any local tsconfig.json.

A custom tsconfig can now be set using `esbuild_bundle(tsconfig)`. By default an empty tsconfig is used to prevent unexpected sandboxing issues.
jbedard added a commit that referenced this issue Jun 13, 2023
By default esbuild uses any local tsconfig.json file. With sandboxing issues (#58)
this may unexpectedly cause `esbuild_bundle` to pickup any local tsconfig.json.

A custom tsconfig can now be set using `esbuild_bundle(tsconfig)`. By default an empty tsconfig is used to prevent unexpected sandboxing issues.
jbedard added a commit that referenced this issue Jun 13, 2023
By default esbuild uses any local tsconfig.json file. With sandboxing issues (#58)
this may unexpectedly cause `esbuild_bundle` to pickup any local tsconfig.json.

A custom tsconfig can now be set using `esbuild_bundle(tsconfig)`. By default an empty tsconfig is used to prevent unexpected sandboxing issues.
jbedard added a commit that referenced this issue Jun 13, 2023
By default esbuild uses any local tsconfig.json file. With sandboxing issues (#58)
this may unexpectedly cause `esbuild_bundle` to pickup any local tsconfig.json.

A custom tsconfig can now be set using `esbuild_bundle(tsconfig)`. By default an empty tsconfig is used to prevent unexpected sandboxing issues.
jbedard added a commit that referenced this issue Jun 13, 2023
By default esbuild uses any local tsconfig.json file. With sandboxing issues (#58)
this may unexpectedly cause `esbuild_bundle` to pickup any local tsconfig.json.

A custom tsconfig can now be set using `esbuild_bundle(tsconfig)`. By default an empty tsconfig is used to prevent unexpected sandboxing issues.
jbedard added a commit that referenced this issue Jun 13, 2023
By default esbuild uses any local tsconfig.json file. With sandboxing issues (#58)
this may unexpectedly cause `esbuild_bundle` to pickup any local tsconfig.json.

A custom tsconfig can now be set using `esbuild_bundle(tsconfig)`. By default an empty tsconfig is used to prevent unexpected sandboxing issues.
jbedard added a commit that referenced this issue Jun 13, 2023
By default esbuild uses any local tsconfig.json file. With sandboxing issues (#58)
this may unexpectedly cause `esbuild_bundle` to pickup any local tsconfig.json.

A custom tsconfig can now be set using `esbuild_bundle(tsconfig)`. By default an empty tsconfig is used to prevent unexpected sandboxing issues.
jbedard added a commit that referenced this issue Jun 13, 2023
By default esbuild uses any local tsconfig.json file. With sandboxing issues (#58)
this may unexpectedly cause `esbuild_bundle` to pickup any local tsconfig.json.

A custom tsconfig can now be set using `esbuild_bundle(tsconfig)`. By default an empty tsconfig is used to prevent unexpected sandboxing issues.
@vpanta
Copy link
Contributor

vpanta commented Jul 17, 2023

Just an addition here, I was able to make a plugin with onResolve which uses https://www.npmjs.com/package/resolve and https://www.npmjs.com/package/resolve.exports to remain in the sandbox. The downside is that this resolver will replace any other resolver used (only one can take affect), so all onResolve actions have to occur within it.

@Aghassi
Copy link
Contributor

Aghassi commented Jul 27, 2023

Just an addition here, I was able to make a plugin with onResolve which uses https://www.npmjs.com/package/resolve and https://www.npmjs.com/package/resolve.exports to remain in the sandbox. The downside is that this resolver will replace any other resolver used (only one can take affect), so all onResolve actions have to occur within it.

Do you have any examples of code you could share publicly? This would be great to have!

@vpanta
Copy link
Contributor

vpanta commented Sep 14, 2023

Just threw up #160 in case it's something that can be used to help.

@Aghassi the above PR contains what we ended up doing, depending on esbuild's own resolver (much simpler than doing our own resolution). Sorry for the delay, it's been a busy quarter.

jbedard added a commit that referenced this issue Nov 27, 2023
jbedard added a commit that referenced this issue Nov 27, 2023
jbedard added a commit that referenced this issue Nov 27, 2023
jbedard added a commit that referenced this issue Nov 27, 2023
@alexeagle
Copy link
Member

THere's a flag --experimental_use_hermetic_linux_sandbox which we should try here, maybe it neatly solves this already?

@gregmagolan
Copy link
Member Author

Fix for this (from #160) is included in the v0.17.0 release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

7 participants