fix(core): fail builds loudly on env leakage - #47
Conversation
Env-leak detection was silently degrading: when an island bundle leaked a server-only variable, `assertNoEnvLeakage` caught it, but `x build` continued and emitted a non-interactive fallback island behind a routine build warning. Production ships a dead island that looks like a recovered build error, and CI/CD exits 0. Now `x build` aborts with EnvLeakageError (non-zero exit, visible to CI/CD) while the dev server keeps serving the fallback stub but logs a visually distinct SECURITY warning so it can't be mistaken for a hot-reload error. Tests moved to env-leak*.test.ts (own processes so the first browser bundle in each process is deterministic for react resolution).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 38 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe build pipeline now treats ChangesEnvironment Leakage Handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ProductionBuild
participant IslandBundler
participant DevServer
participant EnvLeakageError
ProductionBuild->>IslandBundler: bundle island
IslandBundler->>EnvLeakageError: detect leaked environment variable
EnvLeakageError-->>ProductionBuild: abort build
DevServer->>IslandBundler: bundle island
IslandBundler-->>DevServer: return fallback hydration
IslandBundler-->>DevServer: log SECURITY warning
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/env-leak.test.ts`:
- Around line 89-105: Update the rejected-build assertions near
clientDir/islandsDir to explicitly verify that client/leaky/index.html does not
exist. Keep the existing secret-content checks and broader output assertion,
adding the route-specific absence check so fallback output for /leaky cannot
pass unnoticed.
- Line 29: Replace native absolute-path interpolation in the generated fixture
imports with pathToFileURL(...).href so ESM specifiers are valid on all
platforms. Apply this at packages/core/src/env-leak.test.ts lines 29-29 and
54-54, and packages/core/src/env-leak-dev.test.ts lines 28-28 and 53-53, using
the existing ISLAND_PATH value at each generated import site.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ee4f9bca-1816-47e0-8495-4c8bd71234d6
📒 Files selected for processing (6)
.changeset/quiet-seas-serve.mdROADMAP.mdpackages/core/src/build.tspackages/core/src/env-leak-dev.test.tspackages/core/src/env-leak.test.tspackages/core/src/island-bundle.ts
| beforeAll(() => { | ||
| writeRoute( | ||
| LEAK_ROUTE, | ||
| `import { Island } from "${ISLAND_PATH}"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | grep -E 'packages/core/src/env-leak(-dev)?\.test\.ts$' || true
echo "== relevant snippets =="
for f in packages/core/src/env-leak.test.ts packages/core/src/env-leak-dev.test.ts; do
if [ -f "$f" ]; then
echo "--- $f ---"
nl -ba "$f" | sed -n '1,90p'
fi
done
echo "== imports and constants =="
rg -n "ISLAND_PATH|pathToFileURL|join\\(|writeFile|createTemp" packages/core/src/env-leak*.test.ts packages/core/src -S || true
echo "== package config relevant =="
rg -n '"type":|verbatimModuleSyntax|test' package.json packages/core/package.json README.md tsconfig*.json 2>/dev/null || trueRepository: abdelkabirouadoukou/x
Length of output: 353
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== deterministic JS/Esm specifier behavior probe =="
node - <<'JS'
const path = require('node:path');
const isWindowsLike = process.platform === 'win32';
const native = isWindowsLike ? 'C:\\temp\\fixture.island.tsx' : '/tmp/fixture.island.tsx';
const nodeUrl = require('node:url');
const fileUrl = new URL(`file://${native.replace(/\\/g, '/')}`).href;
console.log(JSON.stringify({
platform: process.platform,
native,
fileUrlFromReplacer: fileUrl,
specifierHasBackslash: native.includes('\\'),
specifierString: `import { Island } from "${native}";`,
fileUrlSpecifierString: `import { Island } from "${fileUrl}";`
}, null, 2));
try {
eval(`(async () => { const m = await import("${native.replace(/\\/g, '/')}"); console.log("eval dynamic import resolved", m.Ifo); })()`);
} catch (e) {
console.log("eval dynamic import error:", e.constructor.name, String(e.message).split('\n')[0]);
}
JS
echo "== Windows-style import parser behavior probe =="
node - <<'JS'
const cases = [
`import x from "C:\\temp\\module.tsx";`,
`import x from "C:/temp/module.tsx";`,
];
for (const src of cases) {
console.log("source:", src);
try {
console.log(JSON.stringify(src));
console.log(JSON.stringify(src.replace(/["']/g, m => m)));
} catch (e) {
console.log("error:", e.constructor.name, String(e.message).split('\n')[0]);
}
}
JSRepository: abdelkabirouadoukou/x
Length of output: 1517
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file counts/locations =="
git ls-files | grep -E 'packages/core/src/env-leak(-dev)?\.test\.ts$' || true
echo "== relevant snippets =="
for f in packages/core/src/env-leak.test.ts packages/core/src/env-leak-dev.test.ts; do
if [ -f "$f" ]; then
echo "--- $f ---"
awk '{printf "%6d\t%s\n", NR, $0}' "$f" | sed -n '1,90p'
fi
done
echo "== imports/constants/search =="
rg -n "ISLAND_PATH|pathToFileURL|join\\(|writeFile|createTemp|import { Island }" packages/core/src/env-leak*.test.ts packages/core/src -S || true
echo "== package config relevant =="
rg -n '"type":|verbatimModuleSyntax|test' package.json packages/core/package.json README.md tsconfig*.json 2>/dev/null || true
echo "== JavaScript import specifier semantics probe =="
node - <<'JS'
const fs = require('node:fs');
const os = require('node:os');
const path = require('node:path');
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'envleak-'));
const filePath = path.join(tmpDir, 'module.js');
const content = 'export const value = 123;';
fs.writeFileSync(filePath, content);
const nativeWindowsLike = filePath.replace(/\//g, '\\');
const fileUrl = new URL('file:///' + filePath).href;
for (const src of [
`import { value } from "${nativeWindowsLike}";`,
`import { value } from "${fileUrl}";`,
]) {
console.log('source:', src);
try {
const fn = new Function(src);
fn();
console.log('no syntax error');
} catch (e) {
console.log('syntax error:', e.constructor.name, String(e.message).split('\n')[0]);
}
}
fs.rmSync(tmpDir, { recursive: true, force: true });
JSRepository: abdelkabirouadoukou/x
Length of output: 7899
🌐 Web query:
BMD module specifier backslash Windows JavaScript string escape syntax error
💡 Result:
The issue you are encountering is a classic JavaScript string handling problem, often triggered when Windows file paths containing backslashes () are injected into strings or code templates [1][2][3]. In JavaScript (and many related shell/template environments), the backslash is the escape character [3]. When a Windows path like C:\Users is placed inside a double-quoted string, the engine attempts to interpret the backslash as the start of an escape sequence (e.g., \U is invalid, \v becomes a vertical tab) [1][2][3]. This causes the path to be "mangled" or corrupted, leading to file-not-found (ENOENT) errors [1][2]. Common scenarios and solutions include: 1. String Interpolation/Injection: If you are dynamically building strings that contain file paths, you must escape the backslashes [4][3]. A single backslash in a Windows path must become a double backslash (\) in your JavaScript source or template string [4][3]. Example: Instead of "C:\Users", use "C:\Users" [4][3]. 2. CLI/Command Templates: When passing paths to Node.js scripts via CLI arguments (e.g., node -e "..."), the shell often strips or misinterprets backslashes before Node.js even receives the string [1][2]. - Recommended Fix: Pass the path as a separate command-line argument using process.argv instead of embedding it directly in the script string [1]. - Example: node -e "const fs = require('fs'); console.log(fs.readFileSync(process.argv[4], 'utf8'))" "C:\path\to\file.txt" [1]. 3. Path Normalization: Node.js and many modern JavaScript tools natively support forward slashes (/) as path separators on Windows [1][2]. Converting your Windows paths to use forward slashes before using them in code or as module specifiers avoids the escape character issue entirely [1][2]. Regarding "BMD": Based on search results, BMD commonly refers to "Bot Maker for Discord" and its associated community mods, or "Both Module Definition" in webpack contexts [5][6][7][8][9]. There is no specific standard "BMD module specifier" syntax error; the error you are seeing is almost certainly a generic JavaScript path-escaping issue consistent with the environment you are working in [1][2]. Top results: [1], [2], [3]
Citations:
- 1: Excalidraw workflow: JSON validation command breaks on Windows due to backslash path escaping bmad-code-org/BMAD-METHOD#1468
- 2: Excalidraw workflow: JSON validation step breaks on Windows (bash path escaping) bmad-code-org/BMAD-METHOD#1470
- 3: https://stackoverflow.com/questions/41578002/all-backslashes-are-being-removed-from-local-file-path
- 4: Babel transform seems to be stripping backslash path separators (used on Windows) babel/babel#9765
- 5: https://p.rst.im/q/github.com/RatWasHere/bmods
- 6: https://github.com/devvyyxyz/bmd-action-maker
- 7: https://github.com/Galen-Yip/bmd-webpack-plugin
- 8: https://github.com/asmblah/bmd
- 9: https://github.com/qizzle/bmdm
🌐 Web query:
Node.js modules import specifier backslash Windows "Cannot use import statement"
💡 Result:
In Node.js, ECMAScript module (ESM) import specifiers must follow URL syntax, which uses forward slashes (/) as path separators, even on Windows [1][2]. Using backslashes () in an import specifier is invalid and will cause an error because Node.js resolves ESM import specifiers as URLs, which do not support backslashes as directory separators [2][3]. To correctly reference files on Windows in an ESM environment, you must use forward slashes in your import strings or programmatically generate the correct file URL [4][2]. Recommended Solutions: 1. Use Forward Slashes: For relative imports, always use forward slashes, regardless of the operating system [1][2]. import { myFunction } from './folder/my-module.js'; 2. Use fileURLToPath: If you need to construct paths dynamically (for example, when using dirname equivalents in ESM), use the node:url module to convert file URLs to platform-specific paths safely [4]. This ensures that backslashes are handled correctly by the underlying file system APIs [4]. Example of safe path construction: import { fileURLToPath } from 'node:url'; import { dirname, join } from 'node:path'; // Get the directory path of the current module const dirname = dirname(fileURLToPath(import.meta.url)); // Join paths using the platform-specific separator (backslash on Windows) const myModulePath = join(dirname, 'subfolder', 'module.js'); By using node:url and node:path utilities, you avoid hardcoding path separators and ensure compatibility across different operating systems [4][5].
Citations:
- 1: https://nodejs.org/api/esm.md
- 2: https://nodejs.org/api/esm.html
- 3: https://nodejs.org/docs/latest/api/esm.md
- 4: https://cirrus.twiddles.com/blog/2024/08/20/fixing-node-js-paths-on-windows/
- 5: https://nodejs.org/api/path.html
🌐 Web query:
Bun ESM import specifier backslash Windows "Cannot find module"
💡 Result:
In Bun, ES module (ESM) import specifiers must follow URL-like syntax, which requires the use of forward slashes (/) as path separators, even on Windows [1]. Using backslashes () in import specifiers is non-compliant with the ESM specification and will generally result in a "Cannot find module" error because Bun expects forward-slash-delimited paths [1][2]. If you are experiencing "Cannot find module" errors related to backslashes on Windows, please note the following: 1. Mandatory Forward Slashes: Unlike some older Windows-specific tools that might accept backslashes in paths, ESM import specifiers are URL-based and must strictly use forward slashes [1]. Ensure your import statements (e.g., import "./folder/file.js") use only forward slashes [1]. 2. Bun's Internal Resolution: Bun has addressed various bugs where backslashes were incorrectly inserted into import specifiers (e.g., during bundling) [3][4]. If your code uses correct forward-slash syntax but you still receive these errors, ensure you are running the latest version of Bun, as recent updates have included fixes for Windows-specific path normalization issues [3][2][4]. 3. Debugging: If you are dynamically generating paths, ensure you are normalizing them to forward slashes before passing them to an import statement. If you encounter this error while using features like package.json "imports" or "exports" fields, verify that those paths also use forward slashes, as Bun's resolver requires them to correctly map subpaths [1][2].
Citations:
- 1: https://github.com/oven-sh/bun/blob/88417471/test/js/bun/resolve/resolve.test.ts
- 2: bundler: emit posix-relative paths in the HTML-import manifest on Windows oven-sh/bun#34557
- 3: https://bun.com/blog/bun-v1.1.10
- 4: Pretty file path should only have forward slashes on Windows oven-sh/bun#9974
Use file URLs for generated fixture imports.
The generated modules interpolate native absolute paths into ESM import specifiers. On Windows, backslashes break import parsing or module resolution. Use pathToFileURL(ISLAND_PATH).href at all generated import sites: env-leak.test.ts:29, env-leak.test.ts:54, env-leak-dev.test.ts:28, and env-leak-dev.test.ts:53.
📍 Affects 2 files
packages/core/src/env-leak.test.ts#L29-L29(this comment)packages/core/src/env-leak.test.ts#L54-L54packages/core/src/env-leak-dev.test.ts#L28-L28packages/core/src/env-leak-dev.test.ts#L53-L53
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/env-leak.test.ts` at line 29, Replace native absolute-path
interpolation in the generated fixture imports with pathToFileURL(...).href so
ESM specifiers are valid on all platforms. Apply this at
packages/core/src/env-leak.test.ts lines 29-29 and 54-54, and
packages/core/src/env-leak-dev.test.ts lines 28-28 and 53-53, using the existing
ISLAND_PATH value at each generated import site.
The previous env-leak tests relied on real Bun.build to succeed, which intermittently trips a pre-existing Bun store race (EISDIR / "Unexpected reading file" on node_modules/.bun/react) when many test workers bundle browser targets at once — hit on both ubuntu/macos CI, not just locally. Now env-leak.test.ts stubs Bun.build so the leak-handling logic (prod build rejects; dev serves stub + distinct SECURITY warning; clean island bundles untouched) is asserted deterministically. Real bundling stays covered by build.test.ts and assertNoEnvLeakage is pinned by security.test.ts. Full core suite now passes repeatedly even under default parallelism.
1 participant
What
Env leaks (server-only
process.env/Bun.env/import.meta.envreferenced in client-shipped code) were caught byassertNoEnvLeakage, but the failure was silently swallowed:x buildkept going, emitted a non-interactive fallback island, logged only a routine[islands] build failedwarning, and exited 0. A dead island in prod that looks like a recovered error; CI/CD is blind to it.x buildaborts withEnvLeakageError(non-zero exit, visible to CI/CD). The dev server keeps serving the stub for fast iteration but logs a visually distinct🔒 SECURITY:warning so it can't be mistaken for a hot-reload error.The secret already never reached the client either way; this makes the failure loud and distinguishable from an ordinary
Bun.build()error.Changes
packages/core/src/build.ts— rethrowEnvLeakageErrorfrombundleRouteIslandspackages/core/src/island-bundle.ts— dev-mode🔒 SECURITYwarning (distinct formatting) + fallback stubpackages/core/src/env-leak.test.ts— prod: build fails loudly; secret never lands in outputpackages/core/src/env-leak-dev.test.ts— dev: stub + warning; clean island bundles normally.changeset/quiet-seas-serve.md— patchEnv-leak guardrow (stable, tested)Notes
Leak tests live in their own files so each process's first browser bundle is deterministic — on this machine repeat React
Bun.buildin one process intermittently hits a pre-existing Bun store race (EISDIR onnode_modules/.bun/react), shown independently with an env-agnostic decoy. Suite is green on CI; isolated leak tests pass deterministically locally.Summary by CodeRabbit
New Features
Bug Fixes
Tests