diff --git a/.changeset/quiet-seas-serve.md b/.changeset/quiet-seas-serve.md new file mode 100644 index 0000000..460d18d --- /dev/null +++ b/.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. \ No newline at end of file diff --git a/ROADMAP.md b/ROADMAP.md index f8b92bc..b95f900 100644 --- a/ROADMAP.md +++ b/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; 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` | diff --git a/packages/core/src/build.ts b/packages/core/src/build.ts index d483081..8527de1 100644 --- a/packages/core/src/build.ts +++ b/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); } diff --git a/packages/core/src/env-leak.test.ts b/packages/core/src/env-leak.test.ts new file mode 100644 index 0000000..dc2f1d8 --- /dev/null +++ b/packages/core/src/env-leak.test.ts @@ -0,0 +1,184 @@ +import { afterAll, beforeAll, describe, expect, spyOn, test } from "bun:test"; +import { existsSync, mkdirSync, rmSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { build } from "./build"; +import { buildIslandBundleInMemory } from "./island-bundle"; +import { EnvLeakageError } from "./security/env-isolation"; + +/** + * Env isolation tests stub `Bun.build` so they exercise the leak-handling + * logic deterministically instead of depending on the real React bundler. + * + * Real bundling IS covered elsewhere (build.test.ts bundles islands end to + * end, security.test.ts pins `assertNoEnvLeakage`). Here we need to prove the + * *handling*: a leaking bundle is the very React/runtime that Bun resolves + * from its store, and repeated browser-target builds can intermittently hit a + * pre-existing Bun store race ("EISDIR / Unexpected reading file … + * node_modules/.bun/react…") when many test workers run concurrently. Stubbing + * removes that environmental flake while keeping the assertions exact. + */ + +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"); + +const LEAKY_BUNDLE_CODE = `const secret = process.env.API_SECRET_TOKEN; +function render() { return secret; }`; +const CLEAN_BUNDLE_CODE = `import { hydrateRoot } from "react-dom/client"; +const ok = 42; +hydrateRoot(document, null); +console.log(ok);`; + +let buildSpy: ReturnType; + +function fakeBuildFor(opts: { entrypoints?: (string | URL)[] }): { + success: boolean; + outputs: Array<{ kind: "entry-point"; text: () => Promise }>; + logs: unknown[]; +} { + const entry = String(opts.entrypoints?.[0] ?? ""); + const code = entry.includes("leaky") ? LEAKY_BUNDLE_CODE : CLEAN_BUNDLE_CODE; + return { + success: true, + outputs: [{ kind: "entry-point", text: async () => code }], + logs: [], + }; +} + +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 ; +} + +export const islands = { Leaky }; + +export const mode = "static"; + +export default function LeakPage() { + return ( +
+

Leak

+ + + +
+ ); +} +`, + ); + writeRoute( + CLEAN_ROUTE, + `import { Island } from "${ISLAND_PATH}"; + +export function Counter() { + const count = 0; + return ; +} + +export const islands = { Counter }; + +export const mode = "static"; + +export default function CleanPage() { + return ( +
+

Clean

+ + + +
+ ); +} +`, + ); + + buildSpy = spyOn(Bun, "build"); + buildSpy.mockImplementation(async (opts: Parameters[0]) => + fakeBuildFor(opts ?? { entrypoints: [] }), + ); +}); + +afterAll(() => { + buildSpy.mockRestore(); + 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"); + expect(existsSync(islandsDir) || !existsSync(join(clientDir, "index.html"))).toBe(true); + expect(existsSync(join(clientDir, "leaky/index.html"))).toBe(false); + }); +}); + +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(); + } + }); +}); diff --git a/packages/core/src/island-bundle.ts b/packages/core/src/island-bundle.ts index 70eb829..04e0182 100644 --- a/packages/core/src/island-bundle.ts +++ b/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 {