fix: sandboxing and esm imports

.gitattributes

@@ -6,4 +6,5 @@ js/private/coverage/coverage.js linguist-generated=true
 js/private/devserver/js_run_devserver.mjs linguist-generated=true
 js/private/watch/aspect_watch_protocol.mjs linguist-generated=true
 js/private/watch/aspect_watch_protocol.d.mts linguist-generated=true
+js/private/fs.*.cjs linguist-generated=true
 js/private/js_image_layer.mjs linguist-generated=true

.prettierignore

@@ -6,6 +6,7 @@ examples/**/*-docs.md
 js/private/coverage/coverage.js
 js/private/devserver/js_run_devserver.mjs
 js/private/node-patches/fs.cjs
+js/private/node-patches/fs_stat.cjs
 js/private/watch/aspect_watch_protocol.mjs
 js/private/watch/aspect_watch_protocol.d.mts
 min/

MODULE.bazel

@@ -13,7 +13,7 @@ bazel_dep(name = "aspect_tools_telemetry", version = "0.2.8")
 bazel_dep(name = "bazel_features", version = "1.9.0")
 bazel_dep(name = "bazel_skylib", version = "1.5.0")
 bazel_dep(name = "platforms", version = "0.0.5")
-bazel_dep(name = "rules_nodejs", version = "6.3.0")
+bazel_dep(name = "rules_nodejs", version = "6.4.0")
 
 tel = use_extension("@aspect_tools_telemetry//:extension.bzl", "telemetry")
 use_repo(tel, "aspect_tools_telemetry_report")

js/private/js_binary.bzl

@@ -204,6 +204,13 @@ _ATTRS = {
         which can lead to non-hermetic behavior.""",
         default = True,
     ),
+    "patch_node_esm_loader": attr.bool(
+        doc = """Apply the internal lstat patch to prevent the program from following symlinks out of
+        the execroot, runfiles and the sandbox even when using the ESM loader.
+
+        This flag only has an effect when `patch_node_fs` is True.""",
+        default = True,
+    ),
     "include_sources": attr.bool(
         doc = """When True, `sources` from `JsInfo` providers in `data` targets are included in the runfiles of the target.""",
         default = True,
@@ -319,7 +326,10 @@ _ATTRS = {
     "_windows_constraint": attr.label(default = "@platforms//os:windows"),
     "_node_patches_files": attr.label_list(
         allow_files = True,
-        default = [Label("@aspect_rules_js//js/private/node-patches:fs.cjs")],
+        default = [
+            Label("@aspect_rules_js//js/private/node-patches:fs.cjs"),
+            Label("@aspect_rules_js//js/private/node-patches:fs_stat.cjs"),
+        ],
     ),
     "_node_patches": attr.label(
         allow_single_file = True,
@@ -564,11 +574,18 @@ def _create_launcher(ctx, log_prefix_rule_set, log_prefix_rule, fixed_args = [],
     )
 
 def _js_binary_impl(ctx):
+    # Only apply lstat patch if it's requested
+    JS_BINARY__PATCH_NODE_ESM_LOADER = "1" if ctx.attr.patch_node_esm_loader else "0"
+    fixed_env = {
+        "JS_BINARY__PATCH_NODE_ESM_LOADER": JS_BINARY__PATCH_NODE_ESM_LOADER,
+    }
+
     launcher = _create_launcher(
         ctx,
         log_prefix_rule_set = "aspect_rules_js",
         log_prefix_rule = "js_test" if ctx.attr.testonly else "js_binary",
         fixed_args = ctx.attr.fixed_args,
+        fixed_env = fixed_env,
     )
     runfiles = launcher.runfiles
 

js/private/node-patches/BUILD.bazel

@@ -4,10 +4,12 @@ write_source_files(
     name = "checked_in_compile",
     files = {
         "fs.cjs": "//js/private/node-patches/src:fs-generated.cjs",
+        "fs_stat.cjs": "//js/private/node-patches/src:fs_stat.cjs",
     },
 )
 
 exports_files([
     "fs.cjs",
+    "fs_stat.cjs",
     "register.cjs",
 ])

js/private/node-patches/fs.cjs

@@ -39,6 +39,7 @@ var __asyncGenerator = (this && this.__asyncGenerator) || function (thisArg, _ar
 Object.defineProperty(exports, "__esModule", { value: true });
 exports.patcher = patcher;
 exports.isSubPath = isSubPath;
+exports.resolvePathLike = resolvePathLike;
 exports.escapeFunction = escapeFunction;
 const path = require("path");
 const util = require("util");
@@ -65,7 +66,7 @@ const PATCHED_FS_METHODS = [
  * Function that patches the `fs` module to not escape the given roots.
  * @returns a function to undo the patches.
  */
-function patcher(roots) {
+function patcher(roots, useInternalLstatPatch = false) {
     if (fs._unpatched) {
         throw new Error('FS is already patched.');
     }
@@ -100,82 +101,93 @@ function patcher(roots) {
         .native;
     const { canEscape, isEscape } = escapeFunction(roots);
     // =========================================================================
+    // fsInternal.lstat (to patch ESM resolve's `realpathSync`!)
+    // =========================================================================
+    let unpatchEsm;
+    if (useInternalLstatPatch) {
+        const lstatEsmPatcher = new (require('./fs_stat.cjs').FsInternalStatPatcher)({ canEscape, isEscape }, guardedReadLink, guardedReadLinkSync, unguardedRealPath, unguardedRealPathSync);
+        lstatEsmPatcher.patch();
+        unpatchEsm = lstatEsmPatcher.revert.bind(lstatEsmPatcher);
+    }
+    // =========================================================================
     // fs.lstat
     // =========================================================================
-    fs.lstat = function lstat(...args) {
-        // preserve error when calling function without required callback
-        if (typeof args[args.length - 1] !== 'function') {
-            return origLstat(...args);
-        }
-        const cb = once(args[args.length - 1]);
-        // override the callback
-        args[args.length - 1] = function lstatCb(err, stats) {
-            if (err)
-                return cb(err);
-            if (!stats.isSymbolicLink()) {
+    if (!useInternalLstatPatch) {
+        fs.lstat = function lstat(...args) {
+            // preserve error when calling function without required callback
+            if (typeof args[args.length - 1] !== 'function') {
+                return origLstat(...args);
+            }
+            const cb = once(args[args.length - 1]);
+            // override the callback
+            args[args.length - 1] = function lstatCb(err, stats) {
+                if (err)
+                    return cb(err);
+                if (!stats.isSymbolicLink()) {
+                    // the file is not a symbolic link so there is nothing more to do
+                    return cb(null, stats);
+                }
+                args[0] = resolvePathLike(args[0]);
+                if (!canEscape(args[0])) {
+                    // the file can not escaped the sandbox so there is nothing more to do
+                    return cb(null, stats);
+                }
+                return guardedReadLink(args[0], guardedReadLinkCb);
+                function guardedReadLinkCb(str) {
+                    if (str != args[0]) {
+                        // there are one or more hops within the guards so there is nothing more to do
+                        return cb(null, stats);
+                    }
+                    // there are no hops so lets report the stats of the real file;
+                    // we can't use origRealPath here since that function calls lstat internally
+                    // which can result in an infinite loop
+                    return unguardedRealPath(args[0], unguardedRealPathCb);
+                    function unguardedRealPathCb(err, str) {
+                        if (err) {
+                            if (err.code === 'ENOENT') {
+                                // broken link so there is nothing more to do
+                                return cb(null, stats);
+                            }
+                            return cb(err);
+                        }
+                        return origLstat(str, cb);
+                    }
+                }
+            };
+            origLstat(...args);
+        };
+        fs.lstatSync = function lstatSync(...args) {
+            const stats = origLstatSync(...args);
+            if (!(stats === null || stats === void 0 ? void 0 : stats.isSymbolicLink())) {
                 // the file is not a symbolic link so there is nothing more to do
-                return cb(null, stats);
+                return stats;
             }
             args[0] = resolvePathLike(args[0]);
             if (!canEscape(args[0])) {
                 // the file can not escaped the sandbox so there is nothing more to do
-                return cb(null, stats);
+                return stats;
             }
-            return guardedReadLink(args[0], guardedReadLinkCb);
-            function guardedReadLinkCb(str) {
-                if (str != args[0]) {
-                    // there are one or more hops within the guards so there is nothing more to do
-                    return cb(null, stats);
-                }
+            const guardedReadLink = guardedReadLinkSync(args[0]);
+            if (guardedReadLink != args[0]) {
+                // there are one or more hops within the guards so there is nothing more to do
+                return stats;
+            }
+            try {
+                args[0] = unguardedRealPathSync(args[0]);
                 // there are no hops so lets report the stats of the real file;
-                // we can't use origRealPath here since that function calls lstat internally
+                // we can't use origRealPathSync here since that function calls lstat internally
                 // which can result in an infinite loop
-                return unguardedRealPath(args[0], unguardedRealPathCb);
-                function unguardedRealPathCb(err, str) {
-                    if (err) {
-                        if (err.code === 'ENOENT') {
-                            // broken link so there is nothing more to do
-                            return cb(null, stats);
-                        }
-                        return cb(err);
-                    }
-                    return origLstat(str, cb);
+                return origLstatSync(...args);
+            }
+            catch (err) {
+                if (err.code === 'ENOENT') {
+                    // broken link so there is nothing more to do
+                    return stats;
                 }
+                throw err;
             }
         };
-        origLstat(...args);
-    };
-    fs.lstatSync = function lstatSync(...args) {
-        const stats = origLstatSync(...args);
-        if (!(stats === null || stats === void 0 ? void 0 : stats.isSymbolicLink())) {
-            // the file is not a symbolic link so there is nothing more to do
-            return stats;
-        }
-        args[0] = resolvePathLike(args[0]);
-        if (!canEscape(args[0])) {
-            // the file can not escaped the sandbox so there is nothing more to do
-            return stats;
-        }
-        const guardedReadLink = guardedReadLinkSync(args[0]);
-        if (guardedReadLink != args[0]) {
-            // there are one or more hops within the guards so there is nothing more to do
-            return stats;
-        }
-        try {
-            args[0] = unguardedRealPathSync(args[0]);
-            // there are no hops so lets report the stats of the real file;
-            // we can't use origRealPathSync here since that function calls lstat internally
-            // which can result in an infinite loop
-            return origLstatSync(...args);
-        }
-        catch (err) {
-            if (err.code === 'ENOENT') {
-                // broken link so there is nothing more to do
-                return stats;
-            }
-            throw err;
-        }
-    };
+    }
     // =========================================================================
     // fs.realpath
     // =========================================================================
@@ -388,7 +400,9 @@ function patcher(roots) {
     let unpatchPromises;
     if (promisePropertyDescriptor) {
         const promises = {};
-        promises.lstat = util.promisify(fs.lstat);
+        if (!useInternalLstatPatch) {
+            promises.lstat = util.promisify(fs.lstat);
+        }
         // NOTE: node core uses the newer realpath function fs.promises.native instead of fs.realPath
         promises.realpath = util.promisify(fs.realpath.native);
         promises.readlink = util.promisify(fs.readlink);
@@ -765,6 +779,9 @@ function patcher(roots) {
         if (unpatchPromises) {
             unpatchPromises();
         }
+        if (unpatchEsm) {
+            unpatchEsm();
+        }
         // Re-sync the esm modules to revert to the unpatched module.
         esmModule.syncBuiltinESMExports();
     };

js/private/node-patches/fs_stat.cjs

@@ -0,0 +1,121 @@
+"use strict";
+// Patches Node's internal FS bindings, right before they would call into C++.
+// See full context in: https://github.com/aspect-build/rules_js/issues/362.
+// This is to ensure ESM imports don't escape accidentally via `realpathSync`.
+Object.defineProperty(exports, "__esModule", { value: true });
+exports.FsInternalStatPatcher = void 0;
+/// <reference path="./fs_stat_types.d.cts" />
+const binding_1 = require("internal/test/binding");
+const utils_1 = require("internal/fs/utils");
+const fs_cjs_1 = require("./fs.cjs");
+const internalFs = (0, binding_1.internalBinding)('fs');
+class FsInternalStatPatcher {
+    constructor(escapeFns, guardedReadLink, guardedReadLinkSync, unguardedRealPath, unguardedRealPathSync) {
+        this.escapeFns = escapeFns;
+        this.guardedReadLink = guardedReadLink;
+        this.guardedReadLinkSync = guardedReadLinkSync;
+        this.unguardedRealPath = unguardedRealPath;
+        this.unguardedRealPathSync = unguardedRealPathSync;
+        this._originalFsLstat = internalFs.lstat;
+    }
+    revert() {
+        internalFs.lstat = this._originalFsLstat;
+    }
+    patch() {
+        const statPatcher = this;
+        internalFs.lstat = function (path, bigint, reqCallback, throwIfNoEntry) {
+            if (reqCallback === internalFs.kUsePromises) {
+                return statPatcher._originalFsLstat.call(internalFs, path, bigint, reqCallback, throwIfNoEntry).then((stats) => {
+                    return new Promise((resolve, reject) => {
+                        statPatcher.eeguardStats(path, bigint, stats, throwIfNoEntry, (err, guardedStats) => {
+                            err || !guardedStats ? reject(err) : resolve(guardedStats);
+                        });
+                    });
+                });
+            }
+            else if (reqCallback === undefined) {
+                const stats = statPatcher._originalFsLstat.call(internalFs, path, bigint, undefined, throwIfNoEntry);
+                if (!stats) {
+                    return stats;
+                }
+                return statPatcher.eeguardStatsSync(path, bigint, throwIfNoEntry, stats);
+            }
+            else {
+                // Just re-use the promise path from above.
+                internalFs.lstat(path, bigint, internalFs.kUsePromises, throwIfNoEntry)
+                    .then((stats) => reqCallback.oncomplete(null, stats))
+                    .catch((err) => reqCallback.oncomplete(err));
+            }
+        };
+    }
+    eeguardStats(path, bigint, stats, throwIfNotFound, cb) {
+        const statsObj = (0, utils_1.getStatsFromBinding)(stats);
+        if (!statsObj.isSymbolicLink()) {
+            // the file is not a symbolic link so there is nothing more to do
+            return cb(null, stats);
+        }
+        path = (0, fs_cjs_1.resolvePathLike)(path);
+        if (!this.escapeFns.canEscape(path)) {
+            // the file can not escaped the sandbox so there is nothing more to do
+            return cb(null, stats);
+        }
+        return this.guardedReadLink(path, (str) => {
+            if (str != path) {
+                // there are one or more hops within the guards so there is nothing more to do
+                return cb(null, stats);
+            }
+            // there are no hops so lets report the stats of the real file;
+            // we can't use origRealPath here since that function calls lstat internally
+            // which can result in an infinite loop
+            return this.unguardedRealPath(path, (err, str) => {
+                if (err) {
+                    if (err.code === 'ENOENT') {
+                        // broken link so there is nothing more to do
+                        return cb(null, stats);
+                    }
+                    return cb(err);
+                }
+                // Forward request to original callback.
+                const req2 = new internalFs.FSReqCallback(bigint);
+                req2.oncomplete = (err, realStats) => cb(err, realStats);
+                return this._originalFsLstat.call(internalFs, str, bigint, req2, throwIfNotFound);
+            });
+        });
+    }
+    eeguardStatsSync(path, bigint, throwIfNoEntry, stats) {
+        // No stats available.
+        if (!stats) {
+            return stats;
+        }
+        const statsObj = (0, utils_1.getStatsFromBinding)(stats);
+        if (!statsObj.isSymbolicLink()) {
+            // the file is not a symbolic link so there is nothing more to do
+            return stats;
+        }
+        path = (0, fs_cjs_1.resolvePathLike)(path);
+        if (!this.escapeFns.canEscape(path)) {
+            // the file can not escaped the sandbox so there is nothing more to do
+            return stats;
+        }
+        const guardedReadLink = this.guardedReadLinkSync(path);
+        if (guardedReadLink != path) {
+            // there are one or more hops within the guards so there is nothing more to do
+            return stats;
+        }
+        try {
+            path = this.unguardedRealPathSync(path);
+            // there are no hops so lets report the stats of the real file;
+            // we can't use origRealPathSync here since that function calls lstat internally
+            // which can result in an infinite loop
+            return this._originalFsLstat.call(internalFs, path, bigint, undefined, throwIfNoEntry);
+        }
+        catch (err) {
+            if (err.code === 'ENOENT') {
+                // broken link so there is nothing more to do
+                return stats;
+            }
+            throw err;
+        }
+    }
+}
+exports.FsInternalStatPatcher = FsInternalStatPatcher;