fix: add onResolve plugin to enforce dependencies come from BAZEL_BIN

[mattem]

Previously with preserveSymlinks set esbuild was unable to find transitive dependencies within the layout of node_modules that pnpm gives. This worked fine when using it under rules_nodejs, as that allowed us not to escape the sandbox and the layout was flat.

This allows us to avoid the user needing to hoist pretty much every runtime dependency, which could be a lot.
We can't just turn off preserveSymlinks though, otherwise esbuild will resolve dependencies elsewhere in our workspace, such as back to our source tree.

Building the following would now result in the error. Here we are intentionally commenting out react-dom

ts_project(
    name = "src",
    srcs = [
        "App.tsx",
        "index.tsx",
    ],
    declaration = True,
    supports_workers = False,
    transpiler = partial.make(
        swc_transpiler,
        swcrc = "//:swcrc",
    ),
    tsconfig = "//:tsconfig",
    deps = [
        "//:node_modules/react",
        #        "//:node_modules/react-dom",
    ],
)

esbuild(
    name = "bundle",
    srcs = [
        ":src",
    ],
    entry_point = "index.js",
)

$ bazel build //src:bundle --override_repository=aspect_rules_esbuild=/Users/matt/workspace/rules_esbuild
INFO: Analyzed target //src:bundle (77 packages loaded, 486 targets configured).
INFO: Found 1 target...
ERROR: /Users/matt/workspace/bazel-examples/ts_web_app/src/BUILD.bazel:27:8: Bundling Javascript src/index.js [esbuild] failed: (Exit 1): launcher.sh failed: error executing command bazel-out/darwin_arm64-opt-exec-2B5CBBC6/bin/external/esbuild_darwin-arm64/launcher.sh '--esbuild_args=src/bundle.args.json' '--metafile=src/bundle_metadata.json'

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
✘ [ERROR] [plugin Bazel Sandbox Guard] Failed to resolve import 'react-dom'. Ensure that it is a dependency of an upstream target

    ../../../../../../../../execroot/__main__/node_modules/.pnpm/react-dom@18.2.0_react@18.2.0/node_modules/react-dom/client.js:3:16:
      3 │ var m = require('react-dom');
        ╵                 ~~~~~~~~~~~

We could improve the error message, but that gets tricky when the import doesn't cleanly relate to a package name, eg removing react results in an import that's gone via a mapping in the package.json file (I guess?)

$ bazel build //src:bundle --override_repository=aspect_rules_esbuild=/Users/matt/workspace/rules_esbuild
INFO: Analyzed target //src:bundle (79 packages loaded, 494 targets configured).
INFO: Found 1 target...
ERROR: /Users/matt/workspace/bazel-examples/ts_web_app/src/BUILD.bazel:27:8: Bundling Javascript src/index.js [esbuild] failed: (Exit 1): launcher.sh failed: error executing command bazel-out/darwin_arm64-opt-exec-2B5CBBC6/bin/external/esbuild_darwin-arm64/launcher.sh '--esbuild_args=src/bundle.args.json' '--metafile=src/bundle_metadata.json'

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
✘ [ERROR] [plugin Bazel Sandbox Guard] Failed to resolve import './cjs/react.development.js'. Ensure that it is a dependency of an upstream target

    ../../../../../../../../execroot/__main__/node_modules/.pnpm/react@18.2.0/node_modules/react/index.js:6:27:
      6 │   module.exports = require('./cjs/react.development.js');
        ╵                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[alexeagle]

We can't just turn off preserveSymlinks though, otherwise esbuild will resolve dependencies elsewhere in our workspace, such as back to our source tree.

Isn't this what Greg's symlink guards is meant to prevent? It should make these resolutions hermetic.

[mattem]

Yes, but esbuild is Go, so Greg's work has no affect here. I'd imagine the same happens under swc

[gregmagolan]

🌮

[gregmagolan]

Yes, but esbuild is Go, so Greg's work has no affect here. I'd imagine the same happens under swc

If we used swc for bundling it would likely have the same issue

[JiaLiPassion]

@mattem, could we resolve the conflict and merge this one? @jbedard and I are working on the angular-ngc demo which need this one and the metafile PR you created, thank you.

[matthewjh]

@mattem: it seems like your onResolve plugin is going to error if a link leads out of the bazel-bin directory, but that ESBuild will still be free to pick up files in bazel-bin that are not in the sandbox (are not inputs to the action), which seems problematic. Do I understand that right? If so, this seems like a good initial step to unblock people, but not the final destination.

Another problem with preserveSymlinks = false for our needs in Bazel is that ESBuild leaves comments in the output that contain resolved file paths. With preserveSymlinks = false these can refer to paths like .../home/matt/.cache/bazel_matt/.../.../... which, across different users and checkouts, will generate downstream cache misses on actions that depend on the outputs.

It seems to me that one definitive solution to all these problems is to write a hook that will resolve the symlinks but only within the confines of sandbox dir. I guess this is tantamount to recreating the sandbox symlink guard logic. Or another way to look at it is that the hook will only allow a file path through if there is a symlink in the sandbox terminating at that path.

[gregmagolan]

This will become simpler once the "unresolved" symlinks features stabilizes in Bazel: bazelbuild/bazel#10298.

The end result will be symlinks in the sandbox and in runfiles will be relative to the sandbox and the runfiles so the symlink guards can be simplified to following the symlink chain as long as it stays within the sandbox.

[cgrindel]

Until someone can jump back on this PR, we will convert it to draft.

[gonzojive]

@mattem: it seems like your onResolve plugin is going to error if a link leads out of the bazel-bin directory, but that ESBuild will still be free to pick up files in bazel-bin that are not in the sandbox (are not inputs to the action), which seems problematic. Do I understand that right? If so, this seems like a good initial step to unblock people, but not the final destination.

I believe you're referring to the following code in the PR:

const bazelSandboxAwareOnResolvePlugin = {
  name: 'Bazel Sandbox Guard',
  setup(build) {
    const BAZEL_BINDIR = process.env.BAZEL_BINDIR

    build.onResolve({ filter: /.*/ }, args => {
      if (args.resolveDir.indexOf(BAZEL_BINDIR) === -1) {
        return {
          errors: [
            {
              text: `Failed to resolve import '${args.path}'. Ensure that it is a dependency of an upstream target`,
              location: null,
            }
          ]
        }
      }
    })
  }
}

What's the right way to do the check?

The esbuild rule could emit a file that lists of all the files in the sandbox, presumably, and the symlink guard can use that instead to check if a symlink destination is allowed.

    input_sources = depset(
        copy_files_to_bin_actions(ctx, [
            file
            for file in ctx.files.srcs + filter_files(entry_points)
            if not (file.path.endswith(".d.ts") or file.path.endswith(".tsbuildinfo"))
        ]) + other_inputs + node_toolinfo.tool_files + esbuild_toolinfo.tool_files,
        transitive = [js_lib_helpers.gather_files_from_js_providers(
            targets = ctx.attr.srcs + ctx.attr.deps,
            include_transitive_sources = True,
            include_declarations = False,
            include_npm_linked_packages = True,
        )],
    )

[gregmagolan]

"This won't work" ~Matt

[gonzojive]

I think #112 supersedes this PR, and @gregmagolan seems to be looking at that one.