Repository navigation
Conversation
A long line in bunfig.toml or .npmrc is cut to about 120 bytes around the error when the diagnostic is created. The printer redacts a secret by the key in front of it, so a cut that drops the key prints the value. Keep the whole line when the message has redact_sensitive_information set. Other diagnostics keep the window.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 2 days.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for the next 2 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 3 minutes for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
LGTM — small, well-scoped fix: redacted diagnostics now keep the whole source line so the key-based masking in the printer can still see token = when the error lands far to its right, and the new test exercises both the color and no-color output paths.
Extended reasoning...
Overview
The diff touches src/ast/lib.rs (+~45 lines) and adds one test in test/cli/install/redacted-config-logs.test.ts. A private LineText { Windowed, Whole } enum is threaded through Location::init_or_null_tracked / init_or_null_impl and a new Log::tracked_range_data_with; the only behavioral change is that the ~120-byte excerpt window is skipped when line_text == Whole, which is selected via LineText::for_msg(redact_sensitive_information) at the two message-construction sites that carry the redact flag (add_formatted_msg, which backs add_error_fmt_opts, and add_error_opts). The old tracked_range_data delegates with Windowed, so every other caller is byte-for-byte unchanged.
Security risks
This is a data-exposure fix, not a new exposure surface. I grepped every site that sets redact_sensitive_information: true (src/ini/lib.rs, src/bunfig/bunfig.rs, src/install_types/NodeLinker.rs, src/parsers/toml.rs) — all of them go through add_error_opts or add_error_fmt_opts, both of which now pick Whole. The printer path (Data::write_format -> fmt_javascript / redacted_source) is untouched; it already redacts quoted values keyed on the preceding identifier, and in colored mode writes the quote, the * run, and the closing quote contiguously, so the test's "****..." assertion holds in both env variants. The only cost is that line_text for a redacted message is now an owned copy of a full config-file line instead of a bounded ~120 bytes; memory_cost already sums the actual line_text length so nothing else relied on the bound.
Level of scrutiny
Moderate: the change is small and mechanical, the new parameter is plumbed through a closed set of private functions, and the else branch in tracked_range_data_with that ignores line_text is harmless because init_or_null(None, ..) returns None with no line text at all. The caret indent in the printer is column - 1, which is exactly right for an un-windowed line, so no alignment regression is introduced for the Whole case. The changed files are not covered by .github/CODEOWNERS.
Other factors
The added test spawns bun install against a bunfig whose key sits more than 40 bytes before the error with more than 80 bytes of trailing comment, i.e. precisely the shape that defeats the old window, and asserts the secret is absent while the masked value and the parse error are present, under both NO_COLOR=1 and FORCE_COLOR=1. It follows the sibling test's structure, uses test.concurrent, tempDir, spread bunEnv, drains pipes concurrently, and asserts output before exit code. The single inline finding is a style nit ("*".repeat in the expected-mask string) with no functional bearing.
|
Updated 12:28 PM PT - Sep 18th, 2026
✅ @robobun, your commit 1acbdcb023a6983b67f29be64fe892cef5fc765b passed in 🧪 To try this PR locally: bunx bun-pr 43316That installs a local version of the PR into your bun-43316 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked caret alignment for Whole lines — Data::write_format indents by column - 1 from the line start, so an untrimmed line is the case where the caret lands exactly right. I also looked at whether an unbounded line_text on the redaction paths could blow up output: it is only reachable from bunfig.toml/.npmrc diagnostics and the printer in this checkout writes the line as-is, so the cost is one config line per message.
Extended reasoning...
The diff threads a LineText mode through Location::init_or_null_impl and picks Whole on the two add_* paths that carry redact_sensitive_information. The stored column was never adjusted for the 40-byte left trim, so the printer's column - 1 indent is correct for a whole line and (pre-existing) off for a windowed one; this PR does not make that worse. The Whole lines are owned Vec<u8> copies of a single line from a local config file, only materialized on error paths, which is not a meaningful memory or output concern. The prior nit about "*".repeat was addressed in the follow-up commit (the test now uses Buffer.alloc(secret.length, "*")).
There was a problem hiding this comment.
LGTM — both points from the earlier passes are addressed in 981ec74 (the masked literal is built with Buffer.alloc, and a second case now exercises Log::add_error_opts), and the fix is small and self-contained.
What was reviewed:
- Every site in
src/that setsredact_sensitive_information: true(bunfig, ini/.npmrc, NodeLinker, fmt_jsc) reaches the logger through eitheradd_error_fmt_opts->add_formatted_msgoradd_error_opts; both now passLineText::for_msg(...), so no redacting path still gets the 120-byte window. init_or_null_tracked/tracked_range_dataare private tosrc/ast/lib.rs; the externalLocation::init_or_nullcallers (parser, lockfile, linker) never carry the redaction flag, so keeping them onWindowedis correct.- In
Wholemodecolumnis the untrimmed column andwrite_formatindents the caret bycolumn - 1, so caret alignment is unchanged for whole lines. - The new test asserts the secret is absent and the masked literal present under both
NO_COLORandFORCE_COLOR, covering theredacted_sourceandfmt_javascriptprinter branches.
Extended reasoning...
Overview
The PR touches one Rust file (src/ast/lib.rs) and one test file. It adds a private two-variant enum LineText { Windowed, Whole }, threads it through Location::init_or_null_tracked -> init_or_null_impl and a new Log::tracked_range_data_with, and gates the existing ~120-byte line-window on Windowed. The two logger entry points that carry redact_sensitive_information (add_formatted_msg, reached via add_error_fmt_opts, and add_error_opts) select Whole when the flag is set. All other tracked_range_data callers delegate with Windowed, so their behavior is unchanged. The removed comment about full_line being bounded is accurate to delete since Whole mode now copies the full line.
Security risks
This is a secret-exposure fix: previously, when a bunfig.toml/.npmrc key like token = fell outside the 40-byte lookbehind window, the redaction highlighter could not identify the string as sensitive and printed it in clear. Keeping the whole line makes the redaction strictly more complete; there is no path on which the new code prints more of a secret than before. I grepped every redact_sensitive_information: true site in src/ (bunfig.rs, ini/lib.rs, install_types/NodeLinker.rs, jsc/fmt_jsc.rs) and each goes through one of the two updated entry points, so the class is covered rather than only the reported repro. The one residual consideration is that a redacted line is no longer bounded to ~120 bytes at creation time; the PR description says the printer side bounds output separately, and in practice these are hand-written config lines, so this is not a meaningful memory or DoS concern.
Level of scrutiny
Moderate. The diff is small and mechanical, but it is a refactor of a shared diagnostic helper, so I checked that the Windowed path is byte-for-byte the old behavior (it is: same condition, same lo/hi computation, same UTF-8 boundary adjustment) and that Whole mode does not desynchronize the caret. column comes from data.column_count regardless of windowing, and write_format indents by column - 1, which is correct for an untrimmed line. The signature changes are all to private functions within src/ast/lib.rs, so there are no stale external call sites.
Other factors
Both nits I raised on earlier pushes are addressed in the latest commit: the expected mask now uses Buffer.alloc, and a second test case (install = { registry = { token = ... }, cafile = 1, ... }) reaches Log::add_error_opts, so reverting either LineText::for_msg arm would now fail a test. The test runs under both NO_COLOR and FORCE_COLOR, drains stdout/stderr/exited concurrently, asserts output before exit code, and uses tempDir/await using per harness conventions. The bug hunt exited on dry_streak with no findings, and there are no outstanding third-party objections in the timeline (the github-actions inline comments were followed by commits from the author). I did not run the debug build for this run; the approval rests on reading the diff and tracing every redaction site.
Keep the window for every diagnostic. For a message with redact_sensitive_information set, run redacted_source over the whole line first, so the key in front of a secret is seen before the window can cut it away. The masked line has the same byte length, so the window bounds and the caret do not move.
robobun
changed the title
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I checked the mask-before-window shape now in init_or_null_impl: redacted_source emits one * per masked byte, so offset_in_line computed on the unmasked source.contents still lines up with the masked buffer and the caret, and the * bytes are ASCII so the UTF-8 boundary loops are unaffected. Every redact_sensitive_information: true producer (bunfig, ini, NodeLinker) routes through add_error_opts or add_error_fmt_opts, both of which now thread the flag. One note: the PR description still describes a LineText { Windowed, Whole } enum and an unbounded line_text, but the code masks the full line and then applies the existing ~120-byte window, so the description is stale.
Extended reasoning...
The current diff replaces the earlier enum approach with a bool threaded through Location::init_or_null_tracked / init_or_null_impl and a new Log::tracked_range_data_with; when set, the whole source line is masked via bun_core::fmt::redacted_source into an owned Cow before the 40/80-byte window is applied. I verified the masking is length-preserving (splat_byte_all(b'*', len) for exactly the secret's byte span, write_bytes for everything else), so the window offsets and column - 1 caret indent computed from the unmasked source remain valid, and the is_utf8_char_boundary adjustments operate on ASCII * inside masked spans. All producers that set the flag reach one of the two updated entry points, so no sibling path still windows an unmasked line. The test covers both entry points (TOML syntax error via add_formatted_msg, bunfig validation error via add_error_opts) under NO_COLOR and FORCE_COLOR with exact excerpt regexes. The remaining inline finding (multi-line TOML strings not matched by starts_with_redacted_item) is pre-existing and security-relevant, so a human should weigh whether it belongs in this PR; the description also no longer matches the implementation.
This branch has not been deployed
1 participant
Problem
bunfig.tomlprints part of a secret when the line is longer than about 120 bytes. For the repro in A syntax error in bunfig.toml or .npmrc can print part of a secret when the line is longer than 120 bytes #43311,bun installprints2 | ETSECRETSECRET...SECRET" ] # xxxundererror: Expected a newline or end of file after a key/value pair.Location::init_or_null_impl(src/ast/lib.rs:812). It keeps 40 bytes before the error and 80 after it, when the diagnostic is created. The redaction from Redact secrets inbunfig.tomlandnpmrclogs #14919 runs later, in the printer, and finds a secret by the key in front of it (token,_authToken,_auth,password,_password,email). When the key is more than 40 bytes left of the error, the window drops it, and the printer sees a plain string.Fix
redact_sensitive_informationset,init_or_null_implrunsbun_core::fmt::redacted_sourceover the whole line before the window. The window itself stays the same for every diagnostic.redacted_sourcebecomespub.line_textstays bounded to about 120 bytes, andBuildMessage.position.lineTextfor a config error now holds a masked line. The printer still runs its own redaction on the excerpt.token = 12345) now prints as******(one star per byte), the same as without colors. Before it printed a yellow***.test/cli/install/redacted-config-logs.test.ts(two new cases, one per logger entry point, both fail on 1.4.3). Alsoparse-error-column.test.ts,npmrc.test.ts,bun-run-bunfig.test.tson the debug build.Background
Locationcarries a copy of its line's text, taken when the diagnostic is created. The printer (Data::write_format) drawsN | <text>and a caret under it without a second look at the source.redact_sensitive_informationis a flag on aMsg. The TOML parser sets it forbunfig.toml, the INI parser for.npmrc. The two logger entry points that carry it,add_formatted_msgandadd_error_opts, now pass it toinit_or_null_impl.Notes
#43311 was filed during the work on #43313, which found the cut while it bounded the printed excerpt.
Repro:
Before (1.4.3): the last 38 bytes of the token print. After:
2 | **************************************" ] # xxx...with the caret at column 84. The excerpt still starts inside the value, but the value is masked.A first revision kept the whole line for a redacted message instead of the window. Review preferred masking at creation: the window stays unconditional,
line_textnever holds the plaintext, and #43313 can drop its print-side exemption..npmrc: INI errors are placed at the value, so the 40 bytes before the error include the key. No.npmrcleak of this kind was found. The same code path is fixed.Not covered here:
--configis parsed throughjson_parser::parse_ts_config(src/bunfig/bunfig.rs:1141), and the parser's own messages do not setredact_sensitive_information.token = """...""") is not masked by either redactor. The quote scanner stops at the second quote. Tracked in A triple-quoted secret in bunfig.toml is printed in the clear by a config diagnostic #43334.Suites run with the debug build:
test/cli/install/redacted-config-logs.test.ts(18 pass),test/js/bun/transpiler/parse-error-column.test.ts(7 pass),test/cli/install/npmrc.test.ts(47 pass),test/cli/install/bun-run-bunfig.test.ts(28 pass).