bundler: don't panic on unterminated naming template placeholders - #36325
Conversation
path_template_print advanced with remain[end_len + 1..] after expanding a
known placeholder, but when the template has no closing ']' end_len was
remain.len(), so the slice started one past the end and panicked. This
was reachable from any of the naming/--entry-naming options with a
truncated placeholder like '[name'.
When no matching ']' is found, treat the '[' and the remainder as
literal text. Unknown placeholders ('[foo]') are now kept verbatim in
the output instead of silently dropping the '[', matching esbuild.
Fixes #7810
WalkthroughBundler naming template parsing now detects unterminated placeholders, preserves malformed and unknown placeholder output, and validates naming options in CLI and JavaScript API paths. Tests cover parser behavior, CLI errors, literal placeholders, and API failures. ChangesNaming template validation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — tight fix for a user-reachable panic on malformed naming templates.
What was reviewed:
- Traced
path_template_printon[name,foo[,[foo]-[name].[ext],[[name]],[name][ext— the newcount != 0guard subsumes the removedremain.is_empty()check and everyend_len + 1slice is now guarded byend_len < remain.len(). - Unknown-placeholder branch now emits
[placeholder]verbatim (matching esbuild) instead of dropping the[; checked no existing tests depend on the old corrupted output. - Tests: 5 CLI-backend panic repros + 3 wrong-filename repros in
bundler_naming.test.ts, plus a#[cfg(test)]unit test covering the same shapes and the empty/trailing-[edges.
Extended reasoning...
Overview
Fixes a slice-past-end panic in path_template_print (src/bundler/options.rs) when a naming template ends in a known placeholder name with no closing ] (e.g. naming: "[name"). The scan loop left end_len = remain.len() when no ] was found, and the known-placeholder branch then did &remain[end_len + 1..]. The fix adds an explicit count != 0 bail-out that writes the [ back and treats the tail as literal, and reworks the unknown-placeholder branch to emit [placeholder] verbatim and advance past the ] (previously it dropped the leading [ and left remain pointing at the ], so [foo]-[name] became foo]-index). The obsolete remain.is_empty() special case + TODO is removed — it is strictly subsumed by count != 0 (an empty remain never enters the scan loop, so count stays 1).
Security risks
None. This is output-path template formatting; the change only affects how literal [ bytes are echoed when the user's own template is malformed. The existing write_sanitized_parent_dirs traversal guard is untouched and still applies to [dir] expansion; the new literal pass-through just echoes back what the user typed.
Level of scrutiny
Low-to-moderate. The function is a small pure byte-writer over a user-supplied template string; the diff is ~15 lines of control flow with no new allocation, no unsafe, no FFI. The panic was a user-reachable crash (REVIEW.md: user-reachable failures must not panic), so the fix is required, and matching esbuild's pass-through behavior is the documented reference. I hand-traced the loop on [name, foo[, empty, [[name]] (nested), [name][ext, and [foo]-[name].[ext] — all produce the expected verbatim/substituted output, and every end_len + 1 slice is now reached only when count == 0 ⇒ end_len < remain.len(). The tightened debug_assert!(end_len < remain.len()) is correct since end_len = idx is a valid index when the loop broke.
Other factors
Test coverage is thorough: a Rust unit test exercises 12 template shapes directly against the printer, and 8 new itBundled cases run via backend: "cli" so a regression crashes the spawned bun build rather than the test process (per the PR description, all 5 panic cases SIGABRT on system bun and 3 produce wrong filenames — satisfying fail-before/pass-after). The only other caller of path_template_print is via PathTemplate{,Const}::print in generateChunksInParallel.rs, which passes well-formed built-in templates or the same user-supplied entry_naming/chunk_naming/asset_naming strings this fix targets. No prior review comments to address.
Add options::find_unterminated_placeholder and call it from the Bun.build config parser (JSBundler.rs) and the --entry/chunk/asset-naming CLI flag parser (Arguments.rs). An unmatched '[' now surfaces as a normal config error instead of reaching path_template_print. The printer's literal fallback from the previous commit stays as defense-in-depth. Tests updated: the CLI cases now assert the validation error, plus three Bun.build API subprocess tests covering naming / naming.chunk / naming.asset. UnknownPlaceholderIsLiteral is unchanged (balanced brackets pass validation and are emitted verbatim).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/bundler/options.rs`:
- Around line 2261-2289: Extend the delimiter-only test coverage in the existing
template printer and validator tests: assert that path_template_print preserves
a standalone closing bracket (b"]"), and assert that
find_unterminated_placeholder returns None for the same input. Keep the existing
behavior for other stray and well-formed bracket cases unchanged.
In `@test/bundler/bundler_naming.test.ts`:
- Around line 334-346: Expand the malformed-template matrix in the
unterminated-placeholder tests to cover --chunk-naming and --asset-naming
alongside --entry-naming. Add cases using each flag’s corresponding
configuration field and assert the diagnostic identifies the correct CLI flag,
including sibling entry points as required by the existing naming test
structure.
- Around line 328-333: In test/bundler/bundler_naming.test.ts at lines 328-333,
keep only the issue URL comment for `#7810`; at lines 365-367, replace the
existing prose with that same URL-only comment. Do not retain historical
implementation details.
🪄 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: 64e954d9-4129-4511-af2a-954a6b95c099
📒 Files selected for processing (4)
src/bundler/options.rssrc/runtime/api/JSBundler.rssrc/runtime/cli/Arguments.rstest/bundler/bundler_naming.test.ts
GitHub issue 7810 is a testcontainers bug, not this one; remove the reference from test comments. Add a lone-']' case to the printer/validator unit test and --chunk-naming / --asset-naming CLI tests alongside --entry-naming.
The string shorthand Bun.build({ naming: "..." }) now reports 'naming:'
instead of 'naming.entry:'. The object form still reports the dotted key.
API tests now assert the exact option prefix.
There was a problem hiding this comment.
LGTM — both earlier notes (stderr assertion, naming vs naming.entry in the string-shorthand error) are addressed.
What was reviewed:
path_template_printis now total: thecount != 0guard makes theend_len + 1slice unreachable when no]was found; unknown[foo]is kept verbatim (matches esbuild).find_unterminated_placeholderuses the same depth-counting as the printer; unit test asserts they agree on the boundary cases (],[, empty, well-formed).- CLI loop over
(flag, &mut slot)is behavior-identical to the three blocks it replaces and follows the file'sOutput::err_generic+Global::exit(1)convention. - API test now asserts the exact option prefix (
naming/naming.entry/naming.chunk/naming.asset) so a regression in the message wording fails.
Extended reasoning...
Overview
Fixes a Rust slice-bounds panic in path_template_print (src/bundler/options.rs) reachable from both bun build --entry-naming '[name' and Bun.build({ naming: '[name' }). Adds up-front validation at both entry points (Arguments.rs, JSBundler.rs) with a shared find_unterminated_placeholder helper, and makes the printer itself total so any future unvalidated caller falls back to literal output instead of panicking. The unknown-placeholder branch is also fixed to emit [foo] verbatim rather than the previous foo].
Security risks
None. Input is a config string; the fix converts a process-aborting panic into a catchable validation error, which is strictly a hardening. No new syscalls, no path handling changes beyond what the printer already did.
Level of scrutiny
Medium. It touches the bundler's output-path templating, but the change is narrowly scoped to bracket handling and is covered by both a Rust unit test (printer + validator on all delimiter edge cases including ], [, empty) and integration tests spawning subprocesses so a regressed panic fails the test rather than the runner. The PR description includes fail-before/pass-after evidence on ASAN and release.
Other factors
All prior review feedback is addressed: my two inline notes (drop the negative panic assertion → now expect(stderr).toBe(""); use naming: instead of naming.entry: for the string shorthand), CodeRabbit's three (cover ], cover --chunk-naming/--asset-naming, drop the wrong #7810 reference), and the comment-cop flags (long comments trimmed). The Arguments.rs refactor collapses three copy-pasted blocks into one loop and reuses the file's existing Output::err_generic + Global::exit(1) pattern. The one intentional behavior change — unknown [placeholder] kept literal — matches esbuild and has its own naming/UnknownPlaceholderIsLiteral test.
|
Build #84950 is failing on 18 unrelated flaky tests (spawn-streaming-stdout, jsonc, watch-many-dirs, fastutf8stream-reopen, fetch-tls-abortsignal-timeout, solc, node-http-pinned-write, proxy-stress-protocol, fetch-http2-client, multi-run, serve-body-leak, fetch-http3-client, coverage, streams-leak, regressions 14029/08965/09279/20144), all tagged The diff is green: 25 pass / 3 pre-existing todo locally on the ASAN debug build, 13/13 fail-before on bun 1.3.14. Ready for a maintainer to merge past the unrelated flake. |
| } | ||
|
|
||
| /// `Some((index_of_open_bracket, &template[index..]))` when a `[` has no matching `]`. | ||
| pub fn find_unterminated_placeholder(template: &[u8]) -> Option<(usize, &[u8])> { |
There was a problem hiding this comment.
this is a test only function and should be gated as such.
There was a problem hiding this comment.
It's the validator that produces the up-front error for both entry points (the second commit in this PR), not test-only:
src/runtime/api/JSBundler.rs:1002(thevalidateclosure forBun.build({ naming: ... }))src/runtime/cli/Arguments.rs:2380(--entry-naming/--chunk-naming/--asset-naming)
The #[cfg(test)] unit test further down calls it too, which is probably why it read as test-only from the diff hunk. Happy to restructure if you'd prefer it elsewhere (e.g. a PathTemplate::find_unterminated associated fn).
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ... # Conflicts: # src/jsc/bindings/BunDebugger.cpp
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ...
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ... # Conflicts: # src/js/internal/debugger.ts
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ...
2 participants
Repro
Same from the CLI via
--entry-naming='[name', and for any of[dir/[ext/[hash/[targetappearing at the tail ofnamingornaming.{entry,chunk,asset}.throw: falsedoes not help since the panic aborts the whole process.Cause
path_template_printinsrc/bundler/options.rsscans for a matching]and records its index inend_len. When none is found,end_lenstays atremain.len(). If the text between[and the end of the string happens to be a known placeholder name, the known-placeholder branch then advances withremain = &remain[end_len + 1..], one past the end.The unknown-placeholder branch used
&remain[end_len..]so it did not panic, but it wrote the placeholder text without the[it had already consumed, so"a[b"became"ab"and"[foo]-[name].[ext]"became"foo]-index.js".Fix
Validate up front.
options::find_unterminated_placeholderscans a template with the same depth-counting as the printer and returns the position of any[with no matching]. TheBun.buildconfig parser (JSBundler.rs) calls it fornaming/naming.{entry,chunk,asset}and throwsERR_INVALID_ARG_TYPEwith the placeholder text and byte position; the CLI flag parser (Arguments.rs) does the same for--entry-naming/--chunk-naming/--asset-namingand exits 1.Make the printer total. In
path_template_print, after the bracket scan, if no matching]was found (count != 0), write the[back and treat the remainder as literal text. For an unknown[placeholder]with a matching], write[placeholder]verbatim and advance past the](esbuild keeps unknown placeholders as-is). The known-placeholder branch'send_len + 1advance is now only reached whenend_len < remain.len(). The oldremain.is_empty()special case is subsumed and removed.Tests
test/bundler/bundler_naming.test.ts(all via subprocess so a regression crashes the spawned process, not the test runner):--entry-naming:[name,[dir,[ext,[hash,[target,a[b.js,[name]-[hash.jseach rejected with the--entry-naming: unterminated ...error; on main these either SIGABRT or silently write a wrong filename.--chunk-naming/--asset-naming: rejected with the matching flag name in the error.[nonexistent]-[name].[ext]passes validation and writes[nonexistent]-entry.js(unknown placeholder kept literal); on main it writesnonexistent]-entry.js.Bun.buildwithnaming: "[name",naming: { chunk: "[name]-[hash" },naming: { asset: "pre[post" }each reject with a catchablenaming.*: unterminated ...error; on main the first panics and the other two succeed.Plus a
#[cfg(test)]unit test inoptions.rscovering the printer's literal fallback and the validator directly.Fail-before: 13/13 fail on bun 1.3.14. Pass-after: 24 pass / 3 pre-existing todo on the debug build;
bun-build-api.test.ts -t naming|hashgreen.[review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 0
evidence per changed file