fix: refuse a shim for any conditional export and bound shim filenames

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LwrJtHsao2FCEDH75EefzH

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<bool> =
+        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.

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) };
+    });
+  },
+};

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({{

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::{