fix(html): disable source maps in production mode for HTML bundles#28002
fix(html): disable source maps in production mode for HTML bundles#28002
Conversation
Source maps were unconditionally set to `.linked` in HTMLBundle.zig, causing `//# sourceMappingURL=...` to be appended to JS output and `.map` files to be publicly accessible even with `development: false`. Closes #28001 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Updated 6:51 PM PT - Mar 18th, 2026
✅ @Jarred-Sumner, your commit 1554f3852bbfb8ba1622720e317dc215189c84cd passed in 🧪 To try this PR locally: bunx bun-pr 28002That installs a local version of the PR into your bun-28002 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughSource map generation and serving are now conditional on server mode: enabled and emitted only in development, and disabled/not emitted in production. A regression test suite was added covering both production and development behaviors. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28001.test.ts`:
- Around line 94-100: The test currently only checks that the JS contains a
sourceMappingURL and that the response header 'sourcemap' exists; extend it to
actually fetch the referenced .map to ensure the sourcemap route is served:
parse the source map URL from jsText (or use jsResp.headers.get("sourcemap") if
present), construct an absolute URL against server.url (using jsSrc as base if
needed), perform a fetch to that .map URL (similar to how jsResp is fetched) and
assert the fetch returns a 200/OK response and non-empty body; update the
assertions around jsResp/jsText/jsSrc to include this additional reachable
check.
- Around line 30-39: The test currently only checks for absence of source map
markers but doesn't assert the JS bundle was actually served; add an explicit
assertion on jsResp (e.g., expect(jsResp.ok).toBe(true) or
expect(jsResp.status).toBe(200)) and/or assert jsText is non-empty before
checking sourceMappingURL and headers; do the same for the second block that
uses jsResp/jsText/mapResp (the other occurrence around the later assertions) so
a 404/500 won't incorrectly pass the regression test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7f25585e-456e-4653-8240-9cb9713c8faf
📒 Files selected for processing (2)
src/bun.js/api/server/HTMLBundle.zigtest/regression/issue/28001.test.ts
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
Add explicit assertions that JS bundles are served (200 + non-empty) in both test cases to prevent false passes. In the development mode test, also fetch the actual .map URL to verify it returns valid source map JSON. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
LGTM — straightforward fix to stop serving source maps in production mode, with good regression test coverage.
Extended reasoning...
Overview
This PR makes two small changes to HTMLBundle.zig: (1) sets config.source_map to .none instead of .linked when not in development mode, and (2) skips registering .sourcemap output files as static routes in production. A new regression test covers both production and development cases.
Security risks
This is a security improvement — it prevents source maps (which can expose original source code) from being publicly accessible in production. No new security risks are introduced.
Level of scrutiny
Low scrutiny needed. The change is 3 lines of Zig, follows the existing is_development / isDevelopment() pattern already used throughout the same function, and no CODEOWNERS file covers this path. The SourceMap header logic already gated on isDevelopment(), so this makes the source map generation and file serving consistent with that.
Other factors
The regression test is well-structured, testing both production (no source maps) and development (source maps present) scenarios. No prior reviewer comments or outstanding feedback exist. The fix is mechanical and self-contained with no architectural decisions involved.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28001.test.ts`:
- Around line 104-109: The test resolves the SourceMap header URL against
server.url which is brittle; instead resolve the SourceMap header against the
fetched script's URL (jsResp.url). Update the fetch call that constructs the map
URL (currently using new URL(sourceMapHeader, server.url)) to use new
URL(sourceMapHeader, jsResp.url) so source map references remain correct when
bundles live under subpaths; ensure variables referenced are sourceMapHeader,
jsResp, and mapResp.
- Around line 19-23: The test starts Bun.serve() into a local server variable
but only calls server.stop() on the happy path; replace those manual lifecycle
calls by wrapping each Bun.serve(...) instantiation with the using pattern so
the server is always disposed on error (i.e., change the non-development
Bun.serve() and the development-mode Bun.serve() fixtures to use using when
creating the server and remove the explicit server.stop() calls), locating the
changes around the server variable creation and where homepage is passed as the
route handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2ff9833f-af96-4b34-b91f-df932d12c3a4
📒 Files selected for processing (1)
test/regression/issue/28001.test.ts
…pt URL Use `using server = Bun.serve(...)` in fixture scripts for automatic cleanup on error. Resolve source map URL against jsResp.url instead of server.url for correctness with subpath routes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
LGTM — small, focused fix that correctly gates source map generation and serving on development mode, consistent with existing patterns in the file.
Extended reasoning...
Overview
This PR modifies two files: src/bun.js/api/server/HTMLBundle.zig (the core fix) and a new regression test. The Zig changes are minimal — one line makes config.source_map conditional on is_development (.linked vs .none), and a 3-line guard in onComplete skips registering .sourcemap output files as static routes in production mode.
Security risks
This PR improves security by preventing source maps from being publicly accessible in production mode. Source maps can expose original source code, which is a known information disclosure risk. There are no new security concerns introduced.
Level of scrutiny
The changes are small, mechanical, and follow established patterns already present in the same file. The surrounding code already conditionally handles development mode for minification, NODE_ENV, JSX development mode, SourceMap headers, and cache control headers — this fix simply extends that same pattern to source map generation and file serving. The logic is straightforward with no edge cases or ambiguity.
Other factors
- The regression test is comprehensive: it covers both production (no source maps) and development (source maps present and valid) modes, and includes assertions that the JS bundle itself was served (addressing CodeRabbit feedback from the first commit).
- CodeRabbit suggestions were addressed in commit 74a5f6a (resolved inline comments).
- No human reviewer comments or outstanding concerns.
- The PR may also fix two additional related issues (#19241, #26713) as noted by the issue finder bot.
There was a problem hiding this comment.
LGTM — straightforward conditional to disable source maps in production, following the existing dev/prod pattern in the same function.
Extended reasoning...
Overview
This PR changes two lines in HTMLBundle.zig and adds a regression test. The Zig changes make source map generation conditional on development mode (matching the existing pattern used for minification, NODE_ENV, and JSX settings in the same function) and skip registering .sourcemap output files as static routes in production.
Security risks
No security concerns — this change actually improves security posture by preventing source map files from being publicly accessible in production mode, which could otherwise expose original source code to end users.
Level of scrutiny
Low scrutiny needed. The change is small (2 lines of logic + 3-line guard), follows established patterns in the surrounding code, and the intent is clear from the bug report. The is_development variable used in the conditional is already computed and used for analogous dev/prod branching immediately above the changed line.
Other factors
- All CodeRabbit suggestions (using
usingfor cleanup, asserting JS bundle status, verifying source map reachability in dev mode, resolving againstjsResp.url) were addressed in follow-up commits. - The regression test covers both production and development scenarios with thorough assertions.
- The fix aligns with user expectations per issue #28001 and the duplicate #19241.
|
@Jarred-Sumner did Claude's code review get stuck? |
Summary
.linkedinHTMLBundle.zigline 289, causing//# sourceMappingURL=...comments to be appended to JS output and.mapfiles to be publicly accessible even whendevelopment: falseis setconfig.source_mapconditional:.linkedin development,.nonein productiononCompleteto skip registering.sourcemapoutput files as static routes in production modeCloses #28001
Test plan
test/regression/issue/28001.test.tswith two cases:sourceMappingURLin JS, noSourceMapheader,.mapfile returns 404)sourceMappingURL, hasSourceMapheader)USE_SYSTEM_BUN=1(confirming the bug exists)bun bd test(confirming the fix works)🤖 Generated with Claude Code