Skip to content

Commit 1ea5e42

Browse files
devversionjbedard
authored andcommitted
feat: add experimental js_binary(patch_node_esm_loader)
1 parent 2c87ac1 commit 1ea5e42

27 files changed

+472
-35
lines changed

.gitattributes

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,4 +6,5 @@ js/private/coverage/coverage.js linguist-generated=true
66
js/private/devserver/js_run_devserver.mjs linguist-generated=true
77
js/private/watch/aspect_watch_protocol.mjs linguist-generated=true
88
js/private/watch/aspect_watch_protocol.d.mts linguist-generated=true
9+
js/private/fs.*.cjs linguist-generated=true
910
js/private/js_image_layer.mjs linguist-generated=true

.prettierignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ examples/**/*-docs.md
66
js/private/coverage/coverage.js
77
js/private/devserver/js_run_devserver.mjs
88
js/private/node-patches/fs.cjs
9+
js/private/node-patches/fs_stat.cjs
910
js/private/watch/aspect_watch_protocol.mjs
1011
js/private/watch/aspect_watch_protocol.d.mts
1112
min/

MODULE.bazel

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ bazel_dep(name = "aspect_tools_telemetry", version = "0.2.8")
1313
bazel_dep(name = "bazel_features", version = "1.9.0")
1414
bazel_dep(name = "bazel_skylib", version = "1.5.0")
1515
bazel_dep(name = "platforms", version = "0.0.5")
16-
bazel_dep(name = "rules_nodejs", version = "6.3.0")
16+
bazel_dep(name = "rules_nodejs", version = "6.4.0")
1717

1818
tel = use_extension("@aspect_tools_telemetry//:extension.bzl", "telemetry")
1919
use_repo(tel, "aspect_tools_telemetry_report")

js/private/js_binary.bzl

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,13 @@ _ATTRS = {
204204
which can lead to non-hermetic behavior.""",
205205
default = True,
206206
),
207+
"patch_node_esm_loader": attr.bool(
208+
doc = """Apply the internal lstat patch to prevent the program from following symlinks out of
209+
the execroot, runfiles and the sandbox even when using the ESM loader.
210+
211+
This flag only has an effect when `patch_node_fs` is True.""",
212+
default = False,
213+
),
207214
"include_sources": attr.bool(
208215
doc = """When True, `sources` from `JsInfo` providers in `data` targets are included in the runfiles of the target.""",
209216
default = True,
@@ -319,7 +326,10 @@ _ATTRS = {
319326
"_windows_constraint": attr.label(default = "@platforms//os:windows"),
320327
"_node_patches_files": attr.label_list(
321328
allow_files = True,
322-
default = [Label("@aspect_rules_js//js/private/node-patches:fs.cjs")],
329+
default = [
330+
Label("@aspect_rules_js//js/private/node-patches:fs.cjs"),
331+
Label("@aspect_rules_js//js/private/node-patches:fs_stat.cjs"),
332+
],
323333
),
324334
"_node_patches": attr.label(
325335
allow_single_file = True,
@@ -564,11 +574,18 @@ def _create_launcher(ctx, log_prefix_rule_set, log_prefix_rule, fixed_args = [],
564574
)
565575

566576
def _js_binary_impl(ctx):
577+
# Only apply lstat patch if it's requested
578+
JS_BINARY__PATCH_NODE_ESM_LOADER = "1" if ctx.attr.patch_node_esm_loader else "0"
579+
fixed_env = {
580+
"JS_BINARY__PATCH_NODE_ESM_LOADER": JS_BINARY__PATCH_NODE_ESM_LOADER,
581+
}
582+
567583
launcher = _create_launcher(
568584
ctx,
569585
log_prefix_rule_set = "aspect_rules_js",
570586
log_prefix_rule = "js_test" if ctx.attr.testonly else "js_binary",
571587
fixed_args = ctx.attr.fixed_args,
588+
fixed_env = fixed_env,
572589
)
573590
runfiles = launcher.runfiles
574591

js/private/node-patches/BUILD.bazel

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,12 @@ write_source_files(
44
name = "checked_in_compile",
55
files = {
66
"fs.cjs": "//js/private/node-patches/src:fs-generated.cjs",
7+
"fs_stat.cjs": "//js/private/node-patches/src:fs_stat.cjs",
78
},
89
)
910

1011
exports_files([
1112
"fs.cjs",
13+
"fs_stat.cjs",
1214
"register.cjs",
1315
])

js/private/node-patches/fs.cjs

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ var __asyncGenerator = (this && this.__asyncGenerator) || function (thisArg, _ar
3939
Object.defineProperty(exports, "__esModule", { value: true });
4040
exports.patcher = patcher;
4141
exports.isSubPath = isSubPath;
42+
exports.resolvePathLike = resolvePathLike;
4243
exports.escapeFunction = escapeFunction;
4344
const path = require("path");
4445
const util = require("util");
@@ -65,7 +66,7 @@ const PATCHED_FS_METHODS = [
6566
* Function that patches the `fs` module to not escape the given roots.
6667
* @returns a function to undo the patches.
6768
*/
68-
function patcher(roots) {
69+
function patcher(roots, useInternalLstatPatch = false) {
6970
if (fs._unpatched) {
7071
throw new Error('FS is already patched.');
7172
}
@@ -100,6 +101,15 @@ function patcher(roots) {
100101
.native;
101102
const { canEscape, isEscape } = escapeFunction(roots);
102103
// =========================================================================
104+
// fsInternal.lstat (to patch ESM resolve's `realpathSync`!)
105+
// =========================================================================
106+
let unpatchEsm;
107+
if (useInternalLstatPatch) {
108+
const lstatEsmPatcher = new (require('./fs_stat.cjs').FsInternalStatPatcher)({ canEscape, isEscape }, guardedReadLink, guardedReadLinkSync, unguardedRealPath, unguardedRealPathSync);
109+
lstatEsmPatcher.patch();
110+
unpatchEsm = lstatEsmPatcher.revert.bind(lstatEsmPatcher);
111+
}
112+
// =========================================================================
103113
// fs.lstat
104114
// =========================================================================
105115
fs.lstat = function lstat(...args) {
@@ -388,7 +398,9 @@ function patcher(roots) {
388398
let unpatchPromises;
389399
if (promisePropertyDescriptor) {
390400
const promises = {};
391-
promises.lstat = util.promisify(fs.lstat);
401+
if (!useInternalLstatPatch) {
402+
promises.lstat = util.promisify(fs.lstat);
403+
}
392404
// NOTE: node core uses the newer realpath function fs.promises.native instead of fs.realPath
393405
promises.realpath = util.promisify(fs.realpath.native);
394406
promises.readlink = util.promisify(fs.readlink);
@@ -765,6 +777,9 @@ function patcher(roots) {
765777
if (unpatchPromises) {
766778
unpatchPromises();
767779
}
780+
if (unpatchEsm) {
781+
unpatchEsm();
782+
}
768783
// Re-sync the esm modules to revert to the unpatched module.
769784
esmModule.syncBuiltinESMExports();
770785
};
Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
"use strict";
2+
// Patches Node's internal FS bindings, right before they would call into C++.
3+
// See full context in: https://github.com/aspect-build/rules_js/issues/362.
4+
// This is to ensure ESM imports don't escape accidentally via `realpathSync`.
5+
Object.defineProperty(exports, "__esModule", { value: true });
6+
exports.FsInternalStatPatcher = void 0;
7+
/// <reference path="./fs_stat_types.d.cts" />
8+
const binding_1 = require("internal/test/binding");
9+
const utils_1 = require("internal/fs/utils");
10+
const fs_cjs_1 = require("./fs.cjs");
11+
const internalFs = (0, binding_1.internalBinding)('fs');
12+
class FsInternalStatPatcher {
13+
constructor(escapeFns, guardedReadLink, guardedReadLinkSync, unguardedRealPath, unguardedRealPathSync) {
14+
this.escapeFns = escapeFns;
15+
this.guardedReadLink = guardedReadLink;
16+
this.guardedReadLinkSync = guardedReadLinkSync;
17+
this.unguardedRealPath = unguardedRealPath;
18+
this.unguardedRealPathSync = unguardedRealPathSync;
19+
this._originalFsLstat = internalFs.lstat;
20+
}
21+
revert() {
22+
internalFs.lstat = this._originalFsLstat;
23+
}
24+
patch() {
25+
const statPatcher = this;
26+
internalFs.lstat = function (path, bigint, reqCallback, throwIfNoEntry) {
27+
const currentStack = new Error().stack;
28+
const needsGuarding = currentStack &&
29+
(currentStack.includes('finalizeResolution (node:internal/modules/esm/resolve') &&
30+
!currentStack.includes('eeguardStats'));
31+
if (!needsGuarding) {
32+
return statPatcher._originalFsLstat.call(internalFs, path, bigint, reqCallback, throwIfNoEntry);
33+
}
34+
if (reqCallback === internalFs.kUsePromises) {
35+
return statPatcher._originalFsLstat.call(internalFs, path, bigint, reqCallback, throwIfNoEntry).then((stats) => {
36+
return new Promise((resolve, reject) => {
37+
statPatcher.eeguardStats(path, bigint, stats, throwIfNoEntry, (err, guardedStats) => {
38+
err || !guardedStats ? reject(err) : resolve(guardedStats);
39+
});
40+
});
41+
});
42+
}
43+
else if (reqCallback === undefined) {
44+
const stats = statPatcher._originalFsLstat.call(internalFs, path, bigint, undefined, throwIfNoEntry);
45+
if (!stats) {
46+
return stats;
47+
}
48+
return statPatcher.eeguardStatsSync(path, bigint, throwIfNoEntry, stats);
49+
}
50+
else {
51+
// Just re-use the promise path from above.
52+
internalFs.lstat(path, bigint, internalFs.kUsePromises, throwIfNoEntry)
53+
.then((stats) => reqCallback.oncomplete(null, stats))
54+
.catch((err) => reqCallback.oncomplete(err));
55+
}
56+
};
57+
}
58+
eeguardStats(path, bigint, stats, throwIfNotFound, cb) {
59+
const statsObj = (0, utils_1.getStatsFromBinding)(stats);
60+
if (!statsObj.isSymbolicLink()) {
61+
// the file is not a symbolic link so there is nothing more to do
62+
return cb(null, stats);
63+
}
64+
path = (0, fs_cjs_1.resolvePathLike)(path);
65+
if (!this.escapeFns.canEscape(path)) {
66+
// the file can not escaped the sandbox so there is nothing more to do
67+
return cb(null, stats);
68+
}
69+
return this.guardedReadLink(path, (str) => {
70+
if (str != path) {
71+
// there are one or more hops within the guards so there is nothing more to do
72+
return cb(null, stats);
73+
}
74+
// there are no hops so lets report the stats of the real file;
75+
// we can't use origRealPath here since that function calls lstat internally
76+
// which can result in an infinite loop
77+
return this.unguardedRealPath(path, (err, str) => {
78+
if (err) {
79+
if (err.code === 'ENOENT') {
80+
// broken link so there is nothing more to do
81+
return cb(null, stats);
82+
}
83+
return cb(err);
84+
}
85+
// Forward request to original callback.
86+
const req2 = new internalFs.FSReqCallback(bigint);
87+
req2.oncomplete = (err, realStats) => cb(err, realStats);
88+
return this._originalFsLstat.call(internalFs, str, bigint, req2, throwIfNotFound);
89+
});
90+
});
91+
}
92+
eeguardStatsSync(path, bigint, throwIfNoEntry, stats) {
93+
// No stats available.
94+
if (!stats) {
95+
return stats;
96+
}
97+
const statsObj = (0, utils_1.getStatsFromBinding)(stats);
98+
if (!statsObj.isSymbolicLink()) {
99+
// the file is not a symbolic link so there is nothing more to do
100+
return stats;
101+
}
102+
path = (0, fs_cjs_1.resolvePathLike)(path);
103+
if (!this.escapeFns.canEscape(path)) {
104+
// the file can not escaped the sandbox so there is nothing more to do
105+
return stats;
106+
}
107+
const guardedReadLink = this.guardedReadLinkSync(path);
108+
if (guardedReadLink != path) {
109+
// there are one or more hops within the guards so there is nothing more to do
110+
return stats;
111+
}
112+
try {
113+
path = this.unguardedRealPathSync(path);
114+
// there are no hops so lets report the stats of the real file;
115+
// we can't use origRealPathSync here since that function calls lstat internally
116+
// which can result in an infinite loop
117+
return this._originalFsLstat.call(internalFs, path, bigint, undefined, throwIfNoEntry);
118+
}
119+
catch (err) {
120+
if (err.code === 'ENOENT') {
121+
// broken link so there is nothing more to do
122+
return stats;
123+
}
124+
throw err;
125+
}
126+
}
127+
}
128+
exports.FsInternalStatPatcher = FsInternalStatPatcher;

js/private/node-patches/register.cjs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ const {
55
JS_BINARY__LOG_PREFIX,
66
JS_BINARY__NODE_WRAPPER,
77
JS_BINARY__PATCH_NODE_FS,
8+
JS_BINARY__PATCH_NODE_ESM_LOADER,
89
} = process.env
910

1011
// Keep a count of how many times these patches are applied; this should reflect the depth
@@ -41,5 +42,8 @@ if (
4142
`DEBUG: ${JS_BINARY__LOG_PREFIX}: node fs patches will be applied with roots: ${roots}`
4243
)
4344
}
44-
patchfs(roots)
45+
const useLstatPatch =
46+
JS_BINARY__PATCH_NODE_ESM_LOADER &&
47+
JS_BINARY__PATCH_NODE_ESM_LOADER != '0'
48+
patchfs(roots, useLstatPatch)
4549
}

js/private/node-patches/src/BUILD.bazel

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,18 +4,24 @@ typescript_bin.tsc(
44
name = "compile",
55
srcs = [
66
"fs.cts",
7+
"fs_stat.cts",
8+
"fs_stat_types.d.cts",
79
"tsconfig.json",
810
"//:node_modules/@types/node",
911
],
1012
outs = [
1113
"fs.cjs",
14+
"fs_stat.cjs",
1215
],
1316
args = [
1417
"-p",
1518
"tsconfig.json",
1619
],
1720
chdir = package_name(),
18-
visibility = ["//js/private/test/node-patches:__pkg__"],
21+
visibility = [
22+
"//js/private/node-patches:__pkg__",
23+
"//js/private/test/node-patches:__pkg__",
24+
],
1925
)
2026

2127
genrule(

js/private/node-patches/src/fs.cts

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,10 @@ const PATCHED_FS_METHODS: ReadonlyArray<keyof typeof FsType> = [
5353
* Function that patches the `fs` module to not escape the given roots.
5454
* @returns a function to undo the patches.
5555
*/
56-
export function patcher(roots: string[]): () => void {
56+
export function patcher(
57+
roots: string[],
58+
useInternalLstatPatch: boolean = false
59+
): () => void {
5760
if (fs._unpatched) {
5861
throw new Error('FS is already patched.')
5962
}
@@ -101,6 +104,25 @@ export function patcher(roots: string[]): () => void {
101104

102105
const { canEscape, isEscape } = escapeFunction(roots)
103106

107+
// =========================================================================
108+
// fsInternal.lstat (to patch ESM resolve's `realpathSync`!)
109+
// =========================================================================
110+
let unpatchEsm: Function | undefined
111+
if (useInternalLstatPatch) {
112+
const lstatEsmPatcher =
113+
new (require('./fs_stat.cjs').FsInternalStatPatcher)(
114+
{ canEscape, isEscape },
115+
guardedReadLink,
116+
guardedReadLinkSync,
117+
unguardedRealPath,
118+
unguardedRealPathSync
119+
)
120+
121+
lstatEsmPatcher.patch()
122+
123+
unpatchEsm = lstatEsmPatcher.revert.bind(lstatEsmPatcher)
124+
}
125+
104126
// =========================================================================
105127
// fs.lstat
106128
// =========================================================================
@@ -461,7 +483,9 @@ export function patcher(roots: string[]): () => void {
461483

462484
if (promisePropertyDescriptor) {
463485
const promises: typeof fs.promises = {}
464-
promises.lstat = util.promisify(fs.lstat)
486+
if (!useInternalLstatPatch) {
487+
promises.lstat = util.promisify(fs.lstat)
488+
}
465489
// NOTE: node core uses the newer realpath function fs.promises.native instead of fs.realPath
466490
promises.realpath = util.promisify(fs.realpath.native)
467491
promises.readlink = util.promisify(fs.readlink)
@@ -886,6 +910,10 @@ export function patcher(roots: string[]): () => void {
886910
unpatchPromises()
887911
}
888912

913+
if (unpatchEsm) {
914+
unpatchEsm()
915+
}
916+
889917
// Re-sync the esm modules to revert to the unpatched module.
890918
esmModule.syncBuiltinESMExports()
891919
}
@@ -910,7 +938,7 @@ function stringifyPathLike(p: PathLike): string {
910938
}
911939
}
912940

913-
function resolvePathLike(p: PathLike): string {
941+
export function resolvePathLike(p: PathLike): string {
914942
return path.resolve(stringifyPathLike(p))
915943
}
916944

0 commit comments

Comments
 (0)