From 85ac33ec73be2a710dadb53aea3bbe2e5738999c Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sat, 22 Aug 2026 12:54:35 +0000 Subject: [PATCH] fix: refuse a shim for any conditional export and bound shim filenames Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01LwrJtHsao2FCEDH75EefzH --- backend/tests/bun_jobs.rs | 59 ++++++++ backend/windmill-worker/node_externals.bun.js | 127 ++++++++++++++++++ backend/windmill-worker/src/bun_executor.rs | 107 +-------------- backend/windmill-worker/src/lib.rs | 4 +- 4 files changed, 192 insertions(+), 105 deletions(-) create mode 100644 backend/windmill-worker/node_externals.bun.js diff --git a/backend/tests/bun_jobs.rs b/backend/tests/bun_jobs.rs index 85e017a180..9a88864f80 100644 --- a/backend/tests/bun_jobs.rs +++ b/backend/tests/bun_jobs.rs @@ -2515,6 +2515,65 @@ export function main(name: string) { Ok(()) } +/// A shimmed package is reached by `require()`, so the shim is only safe when nothing can make +/// node's `require` land on a different entry than the `import` the classifier looked at. Bun's +/// own conditions, `import`/`require`, and `--conditions` in the job's NODE_OPTIONS can each do +/// that, so any condition at all disqualifies a manifest. +#[test] +fn test_node_externals_refuses_conditional_exports() { + use std::process::Command; + use windmill_worker::{BUN_PATH, NODE_EXTERNALS_PLUGIN}; + + let temp_dir = tempfile::tempdir().unwrap(); + let dir = temp_dir.path(); + let plugin = NODE_EXTERNALS_PLUGIN.replace("JOB_DIR", &dir.to_string_lossy()); + std::fs::write(dir.join("externals.js"), plugin).unwrap(); + std::fs::write( + dir.join("probe.js"), + r#" +import { entryDependsOnCondition } from "./externals.js"; +const manifests = JSON.parse(await Bun.file("manifests.json").text()); +console.log(JSON.stringify(manifests.map((m) => entryDependsOnCondition(m)))); +"#, + ) + .unwrap(); + // Every entry a shimmable package may have, then every way one can vary by condition. + let manifests = serde_json::json!([ + serde_json::Value::Null, + "./index.js", + { ".": "./index.js" }, + { ".": { "default": "./index.js" }, "./sub": "./sub.js" }, + { ".": { "bun": "./bun.js", "default": "./index.js" } }, + { ".": { "import": "./esm.js", "require": "./cjs.js" } }, + { ".": { "development": "./dev.js", "default": "./index.js" } }, + { "./sub": { "node": "./node.js", "default": "./index.js" } }, + { ".": [{ "browser": "./browser.js" }, "./index.js"] }, + ]); + std::fs::write( + dir.join("manifests.json"), + serde_json::to_string(&manifests).unwrap(), + ) + .unwrap(); + + let output = Command::new(BUN_PATH.as_str()) + .args(["run", "probe.js"]) + .current_dir(dir) + .output() + .expect("failed to run bun"); + assert!( + output.status.success(), + "probe failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let verdicts: Vec = + serde_json::from_slice(String::from_utf8_lossy(&output.stdout).trim().as_bytes()).unwrap(); + assert_eq!( + verdicts, + vec![false, false, false, false, true, true, true, true, true] + ); +} + /// Tests for RELATIVE_BUN_BUILDER (loader_builder.bun.js) /// These tests verify Bun's behavior for import scanning and package.json generation. /// Purpose: Catch regressions when upgrading Bun versions. diff --git a/backend/windmill-worker/node_externals.bun.js b/backend/windmill-worker/node_externals.bun.js new file mode 100644 index 0000000000..0acf8feb5f --- /dev/null +++ b/backend/windmill-worker/node_externals.bun.js @@ -0,0 +1,127 @@ +import { dirname } from "node:path"; +import { mkdirSync, readFileSync, readdirSync, writeFileSync } from "node:fs"; + +const jobDir = "JOB_DIR"; +const nodeModulesDir = jobDir + "/node_modules"; +const cjsShimDir = jobDir + "/.wm_node_cjs"; + +let installedPackages = []; +try { + installedPackages = readdirSync(nodeModulesDir); +} catch (e) {} + +// The shim reaches the package through require(), the classifier through bun's resolution, and the +// two only ever agree on a manifest that cannot select a different entry for either of them. Which +// conditions are live is not knowable here: bun applies its own, `import`/`require` split node's +// two ways in, and `--conditions` in the job's NODE_OPTIONS adds arbitrary more at runtime. So a +// condition of any name disqualifies the package. Subpaths and `default` always select the same +// entry, and a package with no `exports` at all resolves through `main`, which conditions never +// reach. +export function entryDependsOnCondition(exports) { + if (Array.isArray(exports)) { + return exports.some(entryDependsOnCondition); + } + if (exports && typeof exports === "object") { + return Object.keys(exports).some( + (key) => + (key !== "default" && !key.startsWith(".")) || + entryDependsOnCondition(exports[key]) + ); + } + return false; +} + +const shimmable = new Map(); +function isShimmableCjs(specifier) { + if (!shimmable.has(specifier)) { + shimmable.set(specifier, classifyAsCjs(specifier)); + } + return shimmable.get(specifier); +} + +function classifyAsCjs(specifier) { + const segments = specifier.split("/"); + const pkg = specifier.startsWith("@") + ? segments.slice(0, 2).join("/") + : segments[0]; + try { + const manifest = JSON.parse( + readFileSync(nodeModulesDir + "/" + pkg + "/package.json", "utf8") + ); + if (manifest.bun !== undefined || entryDependsOnCondition(manifest.exports)) { + return false; + } + const file = Bun.resolveSync(specifier, jobDir); + if (file.endsWith(".cjs") || file.endsWith(".node")) { + return true; + } + if (file.endsWith(".js")) { + // Same nearest-package.json walk node does to decide how to load a bare .js + for (let dir = dirname(file); dir !== dirname(dir); dir = dirname(dir)) { + try { + return ( + JSON.parse(readFileSync(dir + "/package.json", "utf8")).type !== + "module" + ); + } catch (e) {} + } + } + return false; + } catch (e) { + console.log( + "could not inspect '" + + specifier + + "' to pick its module format, leaving it external: " + + e + ); + return false; + } +} + +const cjsShims = new Map(); +function cjsShim(specifier) { + if (!cjsShims.has(specifier)) { + // Truncated so that a deep subpath cannot flatten into a basename over the filesystem's + // limit; the hash is what keeps two specifiers from sharing a shim. + const shim = + cjsShimDir + + "/" + + specifier.replace(/[^a-zA-Z0-9]/g, "_").slice(0, 64) + + "_" + + Bun.hash(specifier).toString(36) + + ".cjs"; + mkdirSync(cjsShimDir, { recursive: true }); + // The local binding is load-bearing: bun collapses a bare `module.exports = require(x)` back + // into a passthrough external import, which is the shape that breaks node. + writeFileSync( + shim, + "const mod = require(" + JSON.stringify(specifier) + ");\nmodule.exports = mod;\n" + ); + cjsShims.set(specifier, shim); + } + return cjsShims.get(specifier); +} + +// Node only sees the named exports of a CommonJS dependency that cjs-module-lexer finds +// statically, which fails on packages such as lodash, so leaving those as plain externals breaks +// `import { x } from "pkg"` and `import * as pkg from "pkg"`. A generated CommonJS shim makes bun +// synthesize the interop while the package itself is still required at runtime. Only a package +// proven CommonJS gets one: requiring an ESM entry throws on the node versions without +// require(esm), so everything else keeps the plain external it would have had. +export const nodeExternals = { + name: "windmill-node-externals", + setup(build) { + build.onResolve({ filter: /^[^./]/ }, (args) => { + if (args.importer.replace(/\\/g, "/").includes("/.wm_node_cjs/")) { + return { path: args.path, external: true }; + } + if (!installedPackages.includes(args.path.split("/")[0])) { + return undefined; + } + if (!isShimmableCjs(args.path)) { + return { path: args.path, external: true }; + } + return { path: cjsShim(args.path) }; + }); + }, +}; diff --git a/backend/windmill-worker/src/bun_executor.rs b/backend/windmill-worker/src/bun_executor.rs index 2515a3a0c0..54b51e5d37 100644 --- a/backend/windmill-worker/src/bun_executor.rs +++ b/backend/windmill-worker/src/bun_executor.rs @@ -71,6 +71,8 @@ pub const RELATIVE_BUN_LOADER: &str = include_str!("../loader.bun.windows.js"); pub const RELATIVE_BUN_BUILDER: &str = include_str!("../loader_builder.bun.js"); +pub const NODE_EXTERNALS_PLUGIN: &str = include_str!("../node_externals.bun.js"); + const NSJAIL_CONFIG_RUN_BUN_CONTENT: &str = include_str!("../nsjail/run.bun.config.proto"); pub const BUN_LOCK_SPLIT: &str = "\n//bun.lock\n"; @@ -870,6 +872,7 @@ pub async fn build_loader( .replace("TEMP_SCRIPT_REFS_PLACEHOLDER", &temp_refs_json); if mode == LoaderMode::Node { + let node_externals = NODE_EXTERNALS_PLUGIN.replace("JOB_DIR", &job_dir_js); write_file( &job_dir, "node_builder.ts", @@ -877,109 +880,7 @@ pub async fn build_loader( r#" {loader} -import {{ readdir }} from "node:fs/promises"; -import {{ dirname }} from "node:path"; -import {{ mkdirSync, readFileSync, writeFileSync }} from "node:fs"; - -let fileNames = [] -try {{ - fileNames = await readdir("{job_dir_js}/node_modules") -}} catch (e) {{ -}} - -const nodeModulesDir = "{job_dir_js}/node_modules"; -const cjsShimDir = "{job_dir_js}/.wm_node_cjs"; - -// The shim reaches the package through require(), and the classifier through bun's resolution, so -// a manifest whose entry depends on which condition asks for it would let the two disagree: "bun" -// is applied by bun and never by node, and "import"/"require" split node's own two ways in. Only a -// package that resolves to one entry either way can be classified from bun's resolution at all. -function entryDependsOnCondition(exports) {{ - if (Array.isArray(exports)) {{ - return exports.some(entryDependsOnCondition); - }} - if (exports && typeof exports === "object") {{ - return Object.keys(exports).some((key) => - key === "bun" || key === "bun-macro" || key === "import" || key === "require" - || entryDependsOnCondition(exports[key])); - }} - return false; -}} - -// Node only sees the named exports of a CommonJS dependency that cjs-module-lexer finds -// statically, which fails on packages such as lodash, so leaving those as plain externals breaks -// `import {{ x }} from "pkg"` and `import * as pkg from "pkg"`. A generated CommonJS shim makes -// bun synthesize the interop while the package itself is still required at runtime. Only a package -// proven CommonJS gets one: requiring an ESM entry throws on the node versions without -// require(esm), so everything else keeps the plain external it had before. -const shimmable = new Map(); -function isShimmableCjs(specifier) {{ - if (!shimmable.has(specifier)) {{ - shimmable.set(specifier, classifyAsCjs(specifier)); - }} - return shimmable.get(specifier); -}} - -function classifyAsCjs(specifier) {{ - const segments = specifier.split("/"); - const pkg = specifier.startsWith("@") ? segments.slice(0, 2).join("/") : segments[0]; - try {{ - const manifest = JSON.parse(readFileSync(nodeModulesDir + "/" + pkg + "/package.json", "utf8")); - if (manifest.bun !== undefined || entryDependsOnCondition(manifest.exports)) {{ - return false; - }} - const file = Bun.resolveSync(specifier, "{job_dir_js}"); - if (file.endsWith(".cjs") || file.endsWith(".node")) {{ - return true; - }} - if (file.endsWith(".js")) {{ - // Same nearest-package.json walk node does to decide how to load a bare .js - for (let dir = dirname(file); dir !== dirname(dir); dir = dirname(dir)) {{ - try {{ - return JSON.parse(readFileSync(dir + "/package.json", "utf8")).type !== "module"; - }} catch (e) {{}} - }} - }} - return false; - }} catch (e) {{ - console.log("could not inspect '" + specifier - + "' to pick its module format, leaving it external: " + e); - return false; - }} -}} - -const cjsShims = new Map(); -function cjsShim(specifier) {{ - if (!cjsShims.has(specifier)) {{ - const shim = cjsShimDir + "/" + specifier.replace(/[^a-zA-Z0-9]/g, "_") - + "_" + Bun.hash(specifier).toString(36) + ".cjs"; - mkdirSync(cjsShimDir, {{ recursive: true }}); - // The local binding is load-bearing: bun collapses a bare `module.exports = require(x)` - // back into a passthrough external import, which is the shape that breaks node. - writeFileSync(shim, "const mod = require(" + JSON.stringify(specifier) + ");\nmodule.exports = mod;\n"); - cjsShims.set(specifier, shim); - }} - return cjsShims.get(specifier); -}} - -const nodeExternals = {{ - name: "windmill-node-externals", - setup(build) {{ - build.onResolve({{ filter: /^[^./]/ }}, (args) => {{ - if (args.importer.replace(/\\/g, "/").includes("/.wm_node_cjs/")) {{ - return {{ path: args.path, external: true }}; - }} - if (!fileNames.includes(args.path.split("/")[0])) {{ - return undefined; - }} - if (!isShimmableCjs(args.path)) {{ - return {{ path: args.path, external: true }}; - }} - return {{ path: cjsShim(args.path) }}; - }}); - }}, -}}; - +{node_externals} let result; try {{ result = await Bun.build({{ diff --git a/backend/windmill-worker/src/lib.rs b/backend/windmill-worker/src/lib.rs index e3e7868548..cfe7d7b401 100644 --- a/backend/windmill-worker/src/lib.rs +++ b/backend/windmill-worker/src/lib.rs @@ -108,8 +108,8 @@ pub use worker::*; pub use bun_executor::{ build_loader, compute_bundle_local_and_remote_path, ensure_bundle_output_exists, generate_bun_bundle, get_common_bun_proc_envs, install_bun_lockfile, prebundle_bun_script, - prepare_job_dir, LoaderMode, BUN_DEDICATED_WORKER_ARGS, RELATIVE_BUN_BUILDER, - RELATIVE_BUN_LOADER, + prepare_job_dir, LoaderMode, BUN_DEDICATED_WORKER_ARGS, NODE_EXTERNALS_PLUGIN, + RELATIVE_BUN_BUILDER, RELATIVE_BUN_LOADER, }; #[cfg(any(feature = "private", test))] pub use bun_executor::{