bundler: JS-escape file-loader asset paths at chunk assembly - #34188
bundler: JS-escape file-loader asset paths at chunk assembly#34188robobun wants to merge 3 commits into
Conversation
|
Updated 4:02 AM PT - Aug 16th, 2026
✅ @robobun, your commit c0a5a1ac0419ae36d201ae42a5eaf847346ca62c passed in 🧪 To try this PR locally: bunx bun-pr 34188That installs a local version of the PR into your bun-34188 --bun |
|
Warning Review limit reached
Next review available in: 21 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughChangesJavaScript path escaping
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 1
🤖 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 `@test/bundler/bundler_loader.test.ts`:
- Around line 440-443: Extend the special-character test loop in the
compile-mode coverage to include the missing carriage-return and U+2028 filename
cases, matching the corresponding bundle-mode cases above while preserving the
existing labels and assertions.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a7f6a639-8faa-4209-b67f-0efd52df2565
📒 Files selected for processing (2)
src/bundler/Chunk.rstest/bundler/bundler_loader.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
CI on a3c03c0: |
There was a problem hiding this comment.
The Windows count-pass fix in a3c03c0 looks correct — both passes now normalize \ → / before counting/writing escape bytes, and the new bun/loader-file-asset-naming-subdir test guards it on all platforms. I didn't find further issues, but this is a security-relevant change to bundler output assembly with byte-exact count/write accounting, so it's worth a human look.
Checked: js_string_extra_escape_bytes and memcpy_js_string_escaped agree byte-for-byte on all six escape classes; best_quote_char_for_string and print_import_record_path both tie-break to " for the all-ASCII 25-byte placeholder, so the double-quote-only escaping assumption holds for asset, chunk, and SCB references; shift.after.advance now reads the escaped bytes from remain before the reslice, so source-map columns account for inserted backslashes; file_path_buf reuse between the count and write loops is sequential with no borrow overlap.
Extended reasoning...
Overview
Fixes a code-injection bug in IntermediateOutput::code_with_source_map_shifts (src/bundler/Chunk.rs): file-loader asset paths (and --public-path prefixes) were spliced raw into JS chunk output at the site of the 25-byte unique-key placeholder, so a filename containing ", \, LF, CR, or U+2028/U+2029 would terminate the surrounding string literal and become executable source. The fix adds a count/write helper pair (js_string_extra_escape_bytes / memcpy_js_string_escaped) mirroring the existing count_closing_tags / memcpy_escaping_closing_tags pattern, gates it on chunk.content.is_javascript(), and adjusts source-map shift accounting to advance over the escaped bytes. Seven POSIX-only end-to-end tests plus one all-platform itBundled regression guard were added to test/bundler/bundler_loader.test.ts.
Follow-up on prior review
My earlier inline comment flagged that the count pass ran js_string_extra_escape_bytes on the raw dest_path while the write pass first applied platform_to_posix_in_place, causing a Windows-only over-count when asset naming templates contain a subdirectory. Commit a3c03c0 mirrors the write-pass normalization into the count pass so both see identical bytes, and adds an itBundled test with assetNaming: "assets/[name]-[hash].[ext]" that asserts no trailing NULs in the output — this runs on Windows CI. The fix is correct and the test covers the failure mode.
Security risks
This is itself a security fix — untrusted filenames reaching the file loader could inject arbitrary JS into bundled/compiled output. The fix is scoped correctly: it only escapes when the chunk is JS (CSS/HTML placeholder contexts are left for separate handling), and only escapes the six characters that can terminate or corrupt a double-quoted JS string literal. I verified the double-quote assumption holds: best_quote_char_for_string (js_printer/lib.rs:815) tie-breaks to " when all costs are zero, which is always the case for the all-hex-plus-letter-plus-digits placeholder, and print_import_record_path (js_printer/lib.rs:6495) uses the same function on the placeholder path text. So single-quote and backtick contexts don't arise for these substitutions.
Level of scrutiny
High. This touches the bundler's final chunk-assembly path, which every bun build output flows through, and the count-pass/write-pass byte accounting must match exactly (a mismatch is a debug panic and release-build output corruption — my earlier review caught one such case). The escaping logic itself is straightforward and self-contained, but the interaction with cheap_prefix_normalizer, platform_to_posix_in_place, relative_platform_buf, and source-map shift bookkeeping is subtle enough that a maintainer familiar with this file should confirm the escape_for_js gate is correct for every QueryKind × emit-context combination.
Other factors
Test coverage is thorough (four byte-class × bundle, two × compile, public-path prefix, all-platform subdir template). The two escape helpers are structurally parallel to the existing closing-tag helpers in the same impl block. No behavior change for paths without special characters (the count pass adds 0 and the write pass falls through to the 1-byte copy). CSS and HTML chunk substitution is explicitly left unchanged, which the PR description calls out as a separate lower-severity follow-up.
|
Heads-up: #38666 changes the same two passes in |
The JS printer emits file-loader asset paths as a unique-key placeholder inside a double-quoted string literal, and chunk assembly later replaces the placeholder with the final path via raw byte splice. A filename (or --public-path) containing '"', '\\', LF, CR, or U+2028/U+2029 therefore terminated the string literal and became executable source: a crafted asset filename ran arbitrary JS in the bundled output and inside 'bun build --compile' binaries, and a bare newline produced a binary that compiled cleanly (exit 0) but died with SyntaxError at startup. code_with_source_map_shifts now JS-string-escapes the substituted path when the containing chunk is JavaScript. Count and write passes stay exact; CSS/HTML chunk substitution is unchanged.
The write pass applies platform_to_posix_in_place (backslash to forward slash on Windows) before computing escape bytes, but the count pass used the raw path. When dest_path contains a Windows separator (e.g. with --asset-naming='assets/[name]-[hash].[ext]') the count pass would over-count by one escape byte per backslash, leaving trailing zero bytes in the output buffer (debug-assert panic / corrupt JS in release). Both passes now see the same posix-normalized bytes. Adds a cross-platform itBundled case with a subdir asset-naming template so Windows CI covers the substituted-path code path.
a3c03c0 to
c0a5a1a
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// Extra bytes needed to render `path` inside a double-quoted JS string | ||
| /// literal. The printer emits every unique-key placeholder inside `"..."`, | ||
| /// so only the characters that terminate or corrupt such a literal need | ||
| /// escaping here: `"`, `\`, LF, CR, and U+2028/U+2029. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Copy `path` into `dest`, escaping the bytes counted by | ||
| /// `js_string_extra_escape_bytes`. Returns bytes written. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // In JS chunks every placeholder lands inside a printer-emitted `"..."` | ||
| // literal; the substituted path must be JS-string-escaped so filename | ||
| // bytes cannot terminate the literal. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Same `\` → `/` normalization as the write pass so the | ||
| // escape-byte count below matches what will be emitted. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
I reviewed this PR again after the c0a5a1a fix and found no further bugs; the Windows count-pass/write-pass mismatch I flagged earlier is resolved and covered by bun/loader-file-asset-naming-subdir. Because this is a code-injection fix in the bundler's chunk-assembly hot path, and #38666 proposes routing the same substitution through bun_js_printer::write_pre_quoted_string_inner instead, a maintainer should pick which approach lands.
What was reviewed:
- Count pass and write pass now both
platform_to_posix_in_placebeforecheap_prefix_normalizer, sojs_string_extra_escape_bytesandmemcpy_js_string_escapedsee identical bytes anddebug_assert!(remain.is_empty())holds. best_quote_char_for_stringreturns"for the all-hex-ASCII placeholder (all costs zero → falls through to"), so escaping only the double-quote-literal terminators (",\, LF, CR, U+2028/2029) is sufficient.shift.after.advance(&remain[..written])now advances over the escaped output bytes, not the raw path — source-map shifts stay consistent with what was emitted.- Tests exercise
"/\n/\r/U+2028 filenames end-to-end (bundle + run + read-back),--compilefor the two representative cases,--public-pathwith"and\, and the subdir asset-naming case on all platforms.
Extended reasoning...
Overview
The PR fixes a code-injection vector in IntermediateOutput::code_with_source_map_shifts (src/bundler/Chunk.rs): when the bundler substitutes the 25-byte unique-key placeholder inside a JS chunk with the final asset/chunk path, the substituted bytes were copy_from_sliced raw into the surrounding "..." literal. A "/\/LF/CR/U+2028/U+2029 in the asset filename (via [name]/[dir] in the naming template) or in --public-path therefore escaped the string literal and became executable source in the emitted bundle or --compile binary. The fix adds a paired js_string_extra_escape_bytes / memcpy_js_string_escaped (mirroring the existing count_closing_tags / memcpy_escaping_closing_tags pattern) and applies them when chunk.content.is_javascript(). The count pass now also applies the same platform_to_posix_in_place normalization as the write pass so both agree byte-for-byte — this addresses the Windows over-count I flagged in my earlier review. Tests are added in test/bundler/bundler_loader.test.ts.
Security risks
This is the security-sensitive change: it closes an injection where filename bytes reached bundler output unescaped. The escape set is the minimal one that prevents literal termination in a double-quoted JS string; I confirmed against best_quote_char_for_string in src/js_printer/lib.rs that the placeholder (hex-and-decimal ASCII, no quotes) always selects " as the delimiter, so single-quote / backtick escaping is not needed here. CSS/HTML chunk substitution is intentionally left unescaped (different quoting context), which the PR calls out as separate lower-severity work.
Level of scrutiny
High. This touches the bundler's exact-size count-then-write buffer discipline (a mismatch panics in debug and emits trailing NULs in release), source-map shift accounting, and output correctness for every JS chunk with a file-loader/chunk/SCB reference. It is also security-relevant. That warrants a maintainer's eyes rather than an auto-approval.
Other factors
- Competing approach: robobun already noted #38666 routes the same substitution through the printer's own
write_pre_quoted_string_inner(which additionally handles the// @bunASCII-only mode and covers node/browser targets). A maintainer needs to decide whether to land this narrower fix now and rebase later, or wait for #38666. - My prior finding is addressed: the count pass now normalizes
\→/before counting escape bytes, verified on Windows per the author's reply, and the newbun/loader-file-asset-naming-subdirtest guards it on all platforms. - comment-cop bot flags on the four new multi-line comments look like false positives to me (they document the invariant, not justify a workaround), but a maintainer can judge.
- Test quality: the new tests spawn, drain both pipes concurrently, assert the emitted path round-trips to the copied asset on disk, and include the
--compileembedded-file lookup path. They are POSIX-gated where the filename bytes are unrepresentable on Windows, with the Windows-relevant case (subdir asset-naming) covered separately viaitBundled.
1 participant
Repro
A bare
\nin the asset filename produces a--compilebinary that builds cleanly (exit 0) but dies withSyntaxError: Unexpected EOFon startup. Plainbun build --target=bunemits the raw splice as well:Cause
The JS printer emits the file-loader import as an
E::Stringcontaining a 25-byte unique-key placeholder, which always prints as a double-quoted literal (best_quote_char_for_stringreturns"for the all-ASCII placeholder). At chunk assemblyIntermediateOutput::code_with_source_map_shiftsreplaces the placeholder with the final asset path (additional_output_files[..].dest_path, orfinal_rel_pathfor chunk/SCB references, plus the--public-pathprefix) via rawcopy_from_slice. No escaping sees the substituted bytes, so",\, LF, CR or U+2028/U+2029 in the path terminate the surrounding string literal and become source text.The asset path is derived from the input filename via the
[name]/[ext]/[dir]placeholders in the asset naming template, so any file the bundler reaches with thefileloader can supply these bytes.Fix
code_with_source_map_shiftsnow JS-string-escapes the substituted path when the chunk being assembled is JavaScript (chunk.content.is_javascript()). Ajs_string_extra_escape_bytes/memcpy_js_string_escapedpair mirrors the existingcount_closing_tags/memcpy_escaping_closing_tagshelpers so the count pass and write pass stay byte-exact; source-map shifts advance over the escaped bytes. Only the literal-terminating characters are escaped:",\, LF, CR, U+2028, U+2029. Ordinary paths produce unchanged output.CSS and HTML chunk substitution goes through the same function but is left untouched: those placeholders are not JS string literals and need different escaping (CSS
url()/ HTML attribute), which is a separate, lower-severity change.Verification
New tests in
test/bundler/bundler_loader.test.ts(POSIX-only, since Windows filenames cannot contain these bytes):bun build --target=bunwith asset filenames containing"…;process.exit(42);…",\n,\r, U+2028: the bundle runs, the emitted path round-trips to the copied asset on disk, and user code executes.bun build --compilewith the"-injection and\nfilenames: the standalone executable runs user code andBun.file(p).text()reads the embedded asset.--public-pathcontaining"and\: the prefix is escaped and the bundle runs, provingcheap_normalizer[0]is covered.All 7 fail on the released Bun (exit 42 /
SyntaxError/ broken output) and pass with this change.bundler_loader.test.ts(52 tests),bundler_edgecase.test.ts(114 tests) and the embedded-filebundler_compile.test.tssubset pass unchanged.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bundler_loader.test.ts