fix(core): fail builds loudly on env leakage

.changeset/quiet-seas-serve.md

@@ -0,0 +1,12 @@
+---
+"@thexjs/core": patch
+---
+
+Make env-leak detection fail production builds loudly instead of silently
+degrading. A leaked server-only variable was previously caught by
+`assertNoEnvLeakage`, but the build continued and emitted a non-interactive
+fallback island while logging only a routine warning — a dead island in
+production that looked like a recovered build error. Now `x build` aborts
+with an `EnvLeakageError` (non-zero exit, visible to CI/CD), while the dev
+server keeps serving but logs a visually distinct `SECURITY` warning instead
+of a generic build error so it can't be mistaken for a hot-reload hiccup.

ROADMAP.md

@@ -16,6 +16,7 @@ writing.
 | API routes (method dispatch, 405) | stable | yes | `createApp-request.test.ts` |
 | Server functions (dispatch, CSRF) | stable | yes | `server-functions.test.ts` |
 | Security headers / rate limiting | stable | yes | `security/security.test.ts`, `security/rate-limit-redis.test.ts` |
+| Env-leak guard (server-only env in client code) | stable | yes | `env-leak.test.ts` (prod fails loudly), `env-leak-dev.test.ts` (dev stub + 🔒 warning) |
 | Image proxy (SSRF allow-list) | stable | yes | `images/proxy.test.ts` |
 | Env validation (`@thexjs/env`) | stable | yes | `packages/env/src/index.test.ts` |
 | Build toolchain (`x build`, `.x/` output) | stable | yes | `build.test.ts`, `cli.test.ts` |

packages/core/src/build.ts

@@ -20,7 +20,7 @@ import {
   scanPages,
   scanRoutes,
 } from "./router";
-import { assertNoEnvLeakage } from "./security/env-isolation";
+import { EnvLeakageError, assertNoEnvLeakage } from "./security/env-isolation";
 import { registerServerFunctions } from "./server-functions";
 
 export type RouteMode = "static" | "server";
@@ -331,6 +331,13 @@ async function bundleRouteIslands(
       console.warn(`  [islands] build error: ${log.message}`);
     }
   } catch (err) {
+    // A leaked server-only env var is a security failure, not a routine build
+    // hiccup. Rethrow so `x build` fails loudly and CI/CD sees a non-zero
+    // exit — a build that silently ships a dead, non-interactive island is
+    // worse than no build at all. The guarantee (the secret never reaches the
+    // client) already holds; this makes the failure distinguishable from an
+    // ordinary Bun.build() error.
+    if (err instanceof EnvLeakageError) throw err;
     console.warn(`  [islands] build failed for ${routeFilePath}:`, err);
   }
 

packages/core/src/env-leak-dev.test.ts

@@ -0,0 +1,130 @@
+import { afterAll, beforeAll, describe, expect, spyOn, test } from "bun:test";
+import { mkdirSync, rmSync, writeFileSync } from "node:fs";
+import { join } from "node:path";
+import { buildIslandBundleInMemory } from "./island-bundle";
+
+/**
+ * Dev-mode env isolation lives in its own file so this process can make the
+ * dev bundle the FIRST browser-target Bun.build it runs. Bun 1.3.x has an
+ * intermittent in-process race where later browser bundles mis-resolve react
+ * from the node_modules module store ("Unexpected reading file … react/…"),
+ * so keeping these bundles to a fresh process makes the test deterministic.
+ */
+
+const FIXTURE_DIR = join(import.meta.dir, "__fixtures__/env-leak-dev");
+const ROUTES_DIR = join(FIXTURE_DIR, "src/routes");
+const ISLAND_PATH = join(import.meta.dir, "island.tsx");
+const LEAK_ROUTE = join(ROUTES_DIR, "leaky.tsx");
+const CLEAN_ROUTE = join(ROUTES_DIR, "clean.tsx");
+
+function writeRoute(path: string, body: string): void {
+  mkdirSync(join(path, ".."), { recursive: true });
+  writeFileSync(path, body);
+}
+
+beforeAll(() => {
+  writeRoute(
+    LEAK_ROUTE,
+    `import { Island } from "${ISLAND_PATH}";
+
+export function Leaky() {
+  const secret = process.env.API_SECRET_TOKEN;
+  return <button>{secret}</button>;
+}
+
+export const islands = { Leaky };
+
+export const mode = "static";
+
+export default function LeakPage() {
+  return (
+    <main>
+      <h1>Leak</h1>
+      <Island name="Leaky" client="idle">
+        <Leaky />
+      </Island>
+    </main>
+  );
+}
+`,
+  );
+  writeRoute(
+    CLEAN_ROUTE,
+    `import { Island } from "${ISLAND_PATH}";
+
+export function Counter() {
+  const count = 0;
+  return <button>{count}</button>;
+}
+
+export const islands = { Counter };
+
+export const mode = "static";
+
+export default function CleanPage() {
+  return (
+    <main>
+      <h1>Clean</h1>
+      <Island name="Counter" client="idle">
+        <Counter />
+      </Island>
+    </main>
+  );
+}
+`,
+  );
+});
+
+afterAll(() => {
+  rmSync(join(import.meta.dir, "__fixtures__/env-leak-dev"), { recursive: true, force: true });
+});
+
+describe("dev-mode island env isolation", () => {
+  test("a leaking island serves the fallback stub and logs a SECURITY warning without crashing", async () => {
+    const errorSpy = spyOn(console, "error");
+    try {
+      const code = await buildIslandBundleInMemory(
+        LEAK_ROUTE,
+        [],
+        ["Leaky"],
+        new Map(),
+        FIXTURE_DIR,
+      );
+
+      // Dev keeps serving so iteration continues, but only a stub. The
+      // stub's distinctive marker is the "not hydrated" attribute — the
+      // real React bundle also contains the ASCII word "fallback" in
+      // internals, so match the marker, not the word.
+      expect(code).toContain("data-island-hydrated");
+      expect(code).not.toContain("API_SECRET_TOKEN");
+
+      // The warning is visually distinct (🔒 + SECURITY) so it can't be
+      // mistaken for a routine hot-reload build error.
+      const warnings = errorSpy.mock.calls.map((c) => String(c[0])).join("\n");
+      expect(warnings).toContain("🔒");
+      expect(warnings).toContain("SECURITY:");
+      expect(warnings).toContain("API_SECRET_TOKEN");
+    } finally {
+      errorSpy.mockRestore();
+    }
+  });
+
+  test("a clean island bundles normally without a SECURITY warning", async () => {
+    const errorSpy = spyOn(console, "error");
+    try {
+      const code = await buildIslandBundleInMemory(
+        CLEAN_ROUTE,
+        [],
+        ["Counter"],
+        new Map(),
+        FIXTURE_DIR,
+      );
+      expect(code).not.toContain("data-island-hydrated");
+      expect(code).toContain("hydrateRoot");
+      expect(code.length).toBeGreaterThan(50);
+      expect(errorSpy.mock.calls.length).toBe(0);
+    } finally {
+      errorSpy.mockRestore();
+    }
+  });
+});

packages/core/src/env-leak.test.ts

@@ -0,0 +1,107 @@
+import { afterAll, beforeAll, describe, expect, test } from "bun:test";
+import { existsSync, mkdirSync, readFileSync, readdirSync, rmSync, writeFileSync } from "node:fs";
+import { join } from "node:path";
+import { build } from "./build";
+import { EnvLeakageError } from "./security/env-isolation";
+
+/**
+ * Env isolation is deliberately tested from a dedicated file: it bundles
+ * React islands via Bun.build, and the browser pre-Ill bundle path must be
+ * the first thing this process builds so an in-process React import can't
+ * perturb the isolated module store resolution.
+ */
+
+const FIXTURE_DIR = join(import.meta.dir, "__fixtures__/env-leak");
+const ROUTES_DIR = join(FIXTURE_DIR, "src/routes");
+const ISLAND_PATH = join(import.meta.dir, "island.tsx");
+const LEAK_ROUTE = join(ROUTES_DIR, "leaky.tsx");
+const CLEAN_ROUTE = join(ROUTES_DIR, "clean.tsx");
+const OUT_DIR = join(FIXTURE_DIR, "dist");
+
+function writeRoute(path: string, body: string): void {
+  mkdirSync(join(path, ".."), { recursive: true });
+  writeFileSync(path, body);
+}
+
+beforeAll(() => {
+  writeRoute(
+    LEAK_ROUTE,
+    `import { Island } from "${ISLAND_PATH}";
+
+export function Leaky() {
+  const secret = process.env.API_SECRET_TOKEN;
+  return <button>{secret}</button>;
+}
+
+export const islands = { Leaky };
+
+export const mode = "static";
+
+export default function LeakPage() {
+  return (
+    <main>
+      <h1>Leak</h1>
+      <Island name="Leaky" client="idle">
+        <Leaky />
+      </Island>
+    </main>
+  );
+}
+`,
+  );
+  writeRoute(
+    CLEAN_ROUTE,
+    `import { Island } from "${ISLAND_PATH}";
+
+export function Counter() {
+  const count = 0;
+  return <button>{count}</button>;
+}
+
+export const islands = { Counter };
+
+export const mode = "static";
+
+export default function CleanPage() {
+  return (
+    <main>
+      <h1>Clean</h1>
+      <Island name="Counter" client="idle">
+        <Counter />
+      </Island>
+    </main>
+  );
+}
+`,
+  );
+});
+
+afterAll(() => {
+  rmSync(join(import.meta.dir, "__fixtures__/env-leak"), { recursive: true, force: true });
+});
+
+describe("production build env isolation", () => {
+  test("a build with a leaking island fails loudly with EnvLeakageError", async () => {
+    await expect(build({ routesDir: ROUTES_DIR, outDir: OUT_DIR })).rejects.toThrow(
+      EnvLeakageError,
+    );
+
+    // The aborted build must not leave a silently-degraded stub claiming
+    // success: either the islands dir holds real (non-leaking) bundles, or —
+    // since the build aborted on the leak — no client output at all. Also,
+    // whatever bundle it did emit, the secret never reaches the client.
+    const clientDir = join(OUT_DIR, "client");
+    const islandsDir = join(clientDir, "_islands");
+    if (existsSync(islandsDir)) {
+      for (const entry of readdirSync(islandsDir)) {
+        const entryDir = join(islandsDir, entry);
+        for (const file of readdirSync(entryDir)) {
+          if (!file.endsWith(".js")) continue;
+          const js = readFileSync(join(entryDir, file), "utf-8");
+          expect(js).not.toContain("API_SECRET_TOKEN");
+        }
+      }
+    }
+    expect(existsSync(islandsDir) || !existsSync(join(clientDir, "index.html"))).toBe(true);
+  });
+});

packages/core/src/island-bundle.ts

@@ -1,6 +1,6 @@
 import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
 import { basename, join } from "node:path";
-import { assertNoEnvLeakage } from "./security/env-isolation";
+import { EnvLeakageError, assertNoEnvLeakage } from "./security/env-isolation";
 import { generateServerFunctionClient } from "./server-functions";
 
 export interface ActionModuleInfo {
@@ -156,6 +156,15 @@ export async function buildIslandBundleInMemory(
     assertNoEnvLeakage(code, entryPath);
     return wrapIslandBundle(code);
   } catch (err) {
+    // A leaked server-only env var isn't a routine build hiccup. Dev keeps
+    // serving the fallback stub (fast iteration wins), but the warning must
+    // be visually distinct so it can't be mistaken for a hot-reload error.
+    if (err instanceof EnvLeakageError) {
+      console.error(
+        `\x1b[1;31m🔒  SECURITY: [islands] ${err.message}\x1b[0m\n    Falling back to a non-interactive hydration stub — fix the env isolation (prefix with THEXJS_PUBLIC_ or move the access server-side) before relying on this page's client behavior.`,
+      );
+      return wrapIslandBundle(generateFallbackHydration(islandNames));
+    }
     console.warn(`  [islands] build failed for ${routeFilePath}:`, err);
     return wrapIslandBundle(generateFallbackHydration(islandNames));
   } finally {