Fix OOB panic in redacting JS highlighter when ${} ends the input#31700
Conversation
When a template-literal interpolation ends exactly at the end of the
highlighted text, the string-scan loop in
QuickAndDirtyJavaScriptSyntaxHighlighter exits with i == 0 and an empty
remainder, and the redact_sensitive_information branch then slices
text[1..0]:
panic: range start index 1 out of range for slice of length 0
Reachable from any error printed with source context and redaction
enabled (bunfig.toml / .npmrc errors), e.g. a bunfig.toml containing:
logLevel = 3 # `${}
Guard the i == 0 case before forming the inner slice; mirror the fix
into the .zig porting reference. Expose the redacting highlighter
options through the internal-for-testing fmt binding so the regression
test can exercise them directly.
|
Warning Review limit reached
More reviews will be available in 5 minutes and 42 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
Comment |
|
Status: ready for review/merge — diff is green; the red build is four unrelated flaky/infra failures.
CI disposition (build 59873, final — 282 passed, 4 failed):
|
|
Updated 6:43 AM PT - Jun 2nd, 2026
❌ @robobun, your commit ed241e8 has 11 failures in
🧪 To try this PR locally: bunx bun-pr 31700That installs a local version of the PR into your bun-31700 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
When the previous token is a redacted keyword (token, email, _auth, ...) and the rest of the input is whitespace, the whitespace-skip loop drains text to empty and control falls through to `match text[0]`: panic: index out of bounds: the len is 0 but the index is 0 Hoist the emptiness check to right after the whitespace-skip loop; the existing check inside the '='/':' branch only fired when a separator was present. Mirror in the .zig porting reference.
There was a problem hiding this comment.
LGTM — the follow-up commit addresses the third OOB (token → empty-slice index) exactly as suggested, with matching tests and the fmt.zig mirror.
Extended reasoning...
Overview
Two narrowly-scoped bounds guards in QuickAndDirtyJavaScriptSyntaxHighlighter::fmt (src/bun_core/fmt.rs, mirrored in the src/bun_core/fmt.zig porting reference): an i == 0 early-break before slicing text[1..i] in the redact branch, and a hoisted text.is_empty() early-return after the redact-preamble whitespace-skip loop. A new highlight-javascript-redacted variant is plumbed through the test-only fmt_jsc binding so the redacting code path can be driven directly from highlighter.test.ts, which gains three new test.each blocks plus an end-to-end bunfig repro. Cargo.lock adds bstr to bun_bin (already a transitive dep — lockfile-only noise).
Security risks
None. The highlighter is a best-effort cosmetic colorizer for diagnostic output; the redaction logic it touches only masks values for display and is unchanged in behavior except for not panicking on degenerate tail inputs. The new binding is gated behind bun:internal-for-testing. No new inputs, parsing surfaces, or trust boundaries are introduced.
Level of scrutiny
Low–medium. This is defensive hardening of an error-printing path, traced back to a real Sentry crash, with a reproducible repro and regression tests. The two guards are pure additions (early-return / early-break) — they cannot change output for previously-non-crashing inputs. The follow-up commit (36317b2) implements exactly the fix I requested in the prior review pass, and the inline thread is resolved.
Other factors
The bug-hunting pass on the latest revision found nothing further. Targeted re-fuzzing now covers the keyword-gated paths that the original sweep missed. The one CI failure on d8aaf7d is the unrelated, known-flaky duckdb.test.ts third-party segfault. No CODEOWNERS rules apply to the touched files.
There was a problem hiding this comment.
LGTM — the third OOB (redacted keyword + trailing whitespace) is fixed exactly as suggested, mirrored in the Zig reference, and covered by the new tests.
Extended reasoning...
Overview
Two narrowly-scoped bounds-check guards in QuickAndDirtyJavaScriptSyntaxHighlighter::fmt (both src/bun_core/fmt.rs and the .zig porting reference): an i == 0 early-break before slicing text[1..i] in the redact branch, and a hoisted text.is_empty() early-return after the whitespace-skip loop. A new highlight-javascript-redacted variant is plumbed through the bun:internal-for-testing fmt binding so the redact path can be driven directly from tests, and highlighter.test.ts gains regression cases for both panics plus an end-to-end bunfig repro and a positive assertion that redaction still works. Cargo.lock picks up bstr for bun_bin.
Security risks
None. The change only adds early-exit guards to a cosmetic error-printing path; it does not weaken redaction (the new _authToken = "npm_123456" test confirms values are still masked). The new binding is gated behind bun:internal-for-testing.
Level of scrutiny
Low–medium. This is a crash fix in a best-effort syntax highlighter used for error messages, not core runtime logic. The guards are minimal, well-commented, and the failure mode they prevent (panic while printing a diagnostic) is strictly worse than any possible regression in highlight quality.
Other factors
My previous review flagged the third OOB; commit 36317b2 applied the suggested fix verbatim in both files and added the requested test cases, and the thread is resolved. The author re-fuzzed with keyword-prefixed inputs and reports clean. The current bug-hunting pass found nothing. The only CI failure noted was macOS lanes expiring (infra), with a retrigger commit on top — no code change since the fix landed.
2 participants
Crash
Sentry groups BUN-3C97 / BUN-3AA5 report a panic inside
QuickAndDirtyJavaScriptSyntaxHighlighter::fmt(src/bun_core/fmt.rs) while printing anerror message:
Investigating that signature turned up three bugs in the highlighter's scan loop — one
already fixed, two still live on main and fixed here:
The Sentry signature itself (
end 5 / len 4) matches Bun REPL dumps characters from memory (out-of-bounds read) and segfaults when inputting a string interpolation with a backslash in it #31434 exactly: a trailing\inside an unterminated
`${(the 4 bytes`${\) pushed the cursor to 5. That wasfixed by Fix out-of-bounds read in JS highlighter on trailing backslash in
${}#31435 (May 26); the reported events predate the fix reaching canary users.${...}ending the input (still crashing on current canary): when a templateinterpolation ends exactly at the end of the input, the string-scan loop exits with
i == 0andtextfully consumed. Theredact_sensitive_informationbranch then slicestext[1..i]→&[][1..0]:Redacted keyword followed by only whitespace (found in review): after a
RedactedKeyword(token,email,_auth, …), the whitespace-skip loop can draintextto empty, and control falls through tomatch text[0]:Repro (current canary, 1.4.0-canary.1)
The bunfig type error is printed with
redact_sensitive_information: trueand the offendingsource line highlighted; the line ends with
`${}, so the highlighter panics whileprinting the error message — the user sees a crash report instead of their config error.
Same shape is reachable from
.npmrcerror printing (also redacting). Bug 3 reproduces withhighlightJavaScriptRedacted("token ")via the testing binding.Fix
i == 0case before forming theinnerslice in the redact branch;i == 0can only happen via the interpolation sub-path with an empty remainder, so there is no
quoted content to inspect.
inside the
=/:branch only fired when a separator was present..zigporting reference.Also exposes the redacting highlighter options through the
bun:internal-for-testingfmtbinding (
highlight-javascript-redacted) so tests can drive the redact code path directly,matching how #31435 tests the non-redacting options.
Verification
USE_SYSTEM_BUN=1 bun test test/js/bun/util/highlighter.test.ts→ the end-to-end testcrashes with
panic: range start index 1 out of range for slice of length 0(exit 132),matching the live bug; the redacted-binding tests fail (binding doesn't exist yet).
bun bd test test/js/bun/util/highlighter.test.ts→ 16 pass, 0 fail.Fuzzed the patched highlighter through the real binary in both modes (plain + redacting):
class the scanner dispatches on (backtick/quotes,
${/},\, digits, identifierchars,
/*,//, newlines,=/:, and 2-, 3- and 4-byte UTF-8 chars) — ~8.3M inputs;RedactedKeywordand theprev_keyword-setting syntaxkeywords as prefixes (optionally inside
`${), × exhaustive tails of length ≤ 3(alphabet +
\t) × keyword-not-at-start variants, plus ~1M keyword-prefixed randominputs up to 33 chars;
No panics remain.