fix(javascript): refuse Webpack loaders that can fall off the end (#6007)

* fix(javascript): refuse Webpack loaders that can fall off the end

The bootstrap integrity proof accepted any loader whose explicit returns
all yielded the module's exports. A loader with no returns, or with only
the cache-hit return, passed vacuously even though its miss path returns
undefined, so a bundled `n(45840).BrowserWindow` became an exact Electron
window.

Require that the loader body cannot complete normally before the proof
checks its returns, so the bundled-call identity stays unresolved while
the literal require("electron") reference is preserved.

Fixes #5958

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test(javascript): cover finally completion in Webpack loader proofs

Group the loader completion-path cases in their own suite and add
focused finally cases: a completing finally keeps a falling-through
catch unproven, a finally returning another value stays unresolved, and
a finally returning the exports proves the loader.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test(javascript): keep IPC checks for unresolved Webpack loader cases

Share the unresolved-identity assertion so the completion-path cases
also reject directly attributed IPC observations.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

src/domain/javascript/electronBundledRequireIdentity.test.ts

@@ -18,6 +18,18 @@ const CONSUMER =
 const electronOf = (source: string) =>
   analyzeJavaScriptStaticSource(source).electron;
 
+const expectUnresolved = (source: string) => {
+  const electron = electronOf(source);
+  expect(electron.browser_windows).toEqual([]);
+  // Only direct observations come from this identity; the separate
+  // wrapper-substitution path keeps its own resolution basis.
+  expect(
+    electron.ipc.filter(
+      ({ resolution_basis: basis }) => basis !== "wrapper-substitution",
+    ),
+  ).toEqual([]);
+};
+
 describe("Electron identity through Webpack externals", () => {
   it("follows the default-export getter produced by webpack n.n", () => {
     const source = bootstrap(
@@ -142,15 +154,7 @@ describe("Electron identity through Webpack externals", () => {
       bootstrap(EXTERNAL, CONSUMER.replace("n(45840)", "n(45841)")),
     ],
   ])("keeps %s unresolved", (_name, source) => {
-    const electron = electronOf(source);
-    expect(electron.browser_windows).toEqual([]);
-    // Only direct observations come from this identity; the separate
-    // wrapper-substitution path keeps its own resolution basis.
-    expect(
-      electron.ipc.filter(
-        ({ resolution_basis: basis }) => basis !== "wrapper-substitution",
-      ),
-    ).toEqual([]);
+    expectUnresolved(source);
   });
 });
 
@@ -214,15 +218,7 @@ describe("Webpack externals with a tampered runtime", () => {
       ),
     ],
   ])("keeps %s unresolved", (_name, source) => {
-    const electron = electronOf(source);
-    expect(electron.browser_windows).toEqual([]);
-    // Only direct observations come from this identity; the separate
-    // wrapper-substitution path keeps its own resolution basis.
-    expect(
-      electron.ipc.filter(
-        ({ resolution_basis: basis }) => basis !== "wrapper-substitution",
-      ),
-    ).toEqual([]);
+    expectUnresolved(source);
   });
 });
 
@@ -287,15 +283,7 @@ describe("Webpack externals with a tampered loader or module object", () => {
       ),
     ],
   ])("keeps %s unresolved", (_name, source) => {
-    const electron = electronOf(source);
-    expect(electron.browser_windows).toEqual([]);
-    // Only direct observations come from this identity; the separate
-    // wrapper-substitution path keeps its own resolution basis.
-    expect(
-      electron.ipc.filter(
-        ({ resolution_basis: basis }) => basis !== "wrapper-substitution",
-      ),
-    ).toEqual([]);
+    expectUnresolved(source);
   });
 
   it.each([
@@ -331,3 +319,60 @@ describe("Webpack externals with a tampered loader or module object", () => {
     ]);
   });
 });
+
+describe("Webpack externals through the loader's completion paths", () => {
+  const MISS_RETURN = "return m[t](o,o.exports,n),o.exports}";
+  const missPath = (tail: string) =>
+    bootstrap(EXTERNAL, CONSUMER).replace(MISS_RETURN, tail);
+
+  // Falling off the end returns undefined, not the dispatched exports.
+  it.each([
+    ["returns only cached exports", missPath("m[t](o,o.exports,n)}")],
+    [
+      "has no returns",
+      bootstrap(EXTERNAL, CONSUMER).replace(
+        `var r=c[t];if(void 0!==r)return r.exports;var o=c[t]={exports:{}};${MISS_RETURN}`,
+        "var o=c[t]={exports:{}};m[t](o,o.exports,n)}",
+      ),
+    ],
+    [
+      "returns from one branch",
+      missPath("m[t](o,o.exports,n);if(t)return o.exports}"),
+    ],
+    [
+      "returns from a try whose catch falls through",
+      missPath("try{return m[t](o,o.exports,n),o.exports}catch(x){}}"),
+    ],
+    [
+      "falls through a catch past a completing finally",
+      missPath(
+        "try{return m[t](o,o.exports,n),o.exports}catch(x){}finally{t}}",
+      ),
+    ],
+    [
+      "returns another value from a finally",
+      missPath(
+        "try{return m[t](o,o.exports,n),o.exports}finally{return local}}",
+      ),
+    ],
+  ])("keeps a loader that %s unresolved", (_name, source) => {
+    expectUnresolved(source);
+  });
+
+  it.each([
+    [
+      "returns exports from both branches",
+      missPath(
+        "m[t](o,o.exports,n);if(t)return o.exports;else return o.exports}",
+      ),
+    ],
+    [
+      "returns exports from a finally after a falling-through catch",
+      missPath("try{m[t](o,o.exports,n)}catch(x){}finally{return o.exports}}"),
+    ],
+  ])("still resolves a loader that %s", (_name, source) => {
+    expect(electronOf(source).browser_windows).toMatchObject([
+      { preload_path: "./preload.js" },
+    ]);
+  });
+});

src/domain/javascript/javascriptBootstrapIntegrity.ts

@@ -30,11 +30,12 @@ interface LoaderShape {
 
 /**
  * Prove that `loader(key)` returns the exports of the factory declared at
- * `table[key]`: the loader returns only the dispatched module's exports or a
- * cache entry it created for that module, the table has only literal keys,
- * and the table and cache are referenced only by the loader or through the
- * runtime's `loader.m`/`loader.c` exposure. Literal-key writes through that
- * exposure replace their slot. Returns undefined when any of this is unproven.
+ * `table[key]`: every normal completion of the loader returns the dispatched
+ * module's exports or a cache entry it created for that module, the table has
+ * only literal keys, and the table and cache are referenced only by the loader
+ * or through the runtime's `loader.m`/`loader.c` exposure. Literal-key writes
+ * through that exposure replace their slot. Returns undefined when any of this
+ * is unproven.
  */
 export const proveJavaScriptBootstrapIntegrity = (
   call: t.CallExpression,
@@ -74,12 +75,14 @@ const loaderShape = (
   const parameterId = t.isIdentifier(parameter)
     ? bindingOf(state, parameter)
     : undefined;
-  // A rewritten key would dispatch a different slot than the caller named.
+  // A rewritten key would dispatch a different slot than the caller named,
+  // and falling off the end returns undefined instead of the exports.
   if (
     parameterId === undefined ||
     state.bindingsById.get(parameterId)?.definitions.length !== 1 ||
     loader.async ||
-    loader.generator
+    loader.generator ||
+    completesNormally(loader.body)
   )
     return undefined;
   const dispatches = new Set<t.Node>();
@@ -129,6 +132,35 @@ const loaderShape = (
     : undefined;
 };
 
+/**
+ * Whether control may reach the end of `statement`. Loops, switches and labels
+ * can absorb a `break`, so they are assumed to complete.
+ */
+const completesNormally = (statement: t.Statement): boolean => {
+  if (t.isReturnStatement(statement) || t.isThrowStatement(statement))
+    return false;
+  if (t.isBlockStatement(statement))
+    return statement.body.every(completesNormally);
+  if (t.isIfStatement(statement))
+    return (
+      statement.alternate === null ||
+      statement.alternate === undefined ||
+      completesNormally(statement.consequent) ||
+      completesNormally(statement.alternate)
+    );
+  if (t.isTryStatement(statement))
+    return (
+      (statement.finalizer === null ||
+        statement.finalizer === undefined ||
+        completesNormally(statement.finalizer)) &&
+      (completesNormally(statement.block) ||
+        (statement.handler !== null &&
+          statement.handler !== undefined &&
+          completesNormally(statement.handler.body)))
+    );
+  return true;
+};
+
 /** `table[key](module, ...)` or `table[key].call(thisArg, module, ...)`. */
 const tableDispatch = (
   call: t.CallExpression,

tests/acceptance/applications/webpackLoaderReturnsCli.test.ts

@@ -0,0 +1,62 @@
+import { expect } from "vitest";
+
+import { parseEvidence } from "../../../src/domain/evidence.js";
+import { javascriptApplicationAnalysisResultSchema } from "../../../src/domain/javascript/javascriptApplicationAnalysis.js";
+import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js";
+import { cliTest } from "../../support/cli/cliFixture.js";
+import { writeFixtureFiles } from "../../support/javascriptApplicationFixture.js";
+
+const CACHE_HIT = `
+    var cached = cache[key];
+    if (cached !== undefined) return cached.exports;`;
+
+const loaderSource = (cacheHit: string, missReturn: string) => `(function () {
+  var modules = {
+    45840: function (module) { module.exports = require("electron"); },
+    1: function (module, exports, n) { new (n(45840).BrowserWindow)({}); }
+  }, cache = {};
+  function n(key) {${cacheHit}
+    var module = (cache[key] = { exports: {} });
+    modules[key](module, module.exports, n);${missReturn}
+  }
+  n(1);
+})();`;
+
+cliTest.for([
+  [
+    "the canonical loader",
+    loaderSource(CACHE_HIT, "\n    return module.exports;"),
+    1,
+  ],
+  ["a loader that returns only cached exports", loaderSource(CACHE_HIT, ""), 0],
+  ["a loader without returns", loaderSource("", ""), 0],
+] as const)(
+  "attributes the BrowserWindow to Electron only through %s",
+  async ([, source, windows], { cli }) => {
+    const root = await createTestTempDirectory("rea-webpack-loader-returns-");
+    await writeFixtureFiles(root, {
+      "main.js": source,
+      "package.json": JSON.stringify({ name: "fixture", main: "main.js" }),
+    });
+    const response = await cli.run({
+      arguments: ["analyze-javascript-application", root, "--format", "json"],
+      environment: { REA_LOG_LEVEL: "silent" },
+    });
+    expect(response.exitCode).toBe(0);
+    const result = javascriptApplicationAnalysisResultSchema.parse(
+      parseEvidence(response.json).normalized_result,
+    );
+    expect(result.summary.browser_windows).toBe(windows);
+    // The literal external stays observed even when the loader is unproven.
+    expect(
+      result.graph.nodes.flatMap(({ observations }) =>
+        observations.map(({ properties }) => properties),
+      ),
+    ).toContainEqual(
+      expect.objectContaining({
+        reference_kind: "require",
+        specifier: "electron",
+      }),
+    );
+  },
+);