util: styleText handles inspect.colors added after first call; getCallSites captures Error - #35409
util: styleText handles inspect.colors added after first call; getCallSites captures Error#35409robobun wants to merge 3 commits into
Conversation
…ites captures Error styleText() caches inspect.colors on first call but validated unknown format strings against the live inspect.colors, so a color added after the cache was populated passed validation then dereferenced an undefined cache entry. Fall back to the live inspect.colors entry on cache miss and populate the cache, restoring the behavior from before the cache was introduced. getCallSites() read Error via the global binding, so replacing globalThis.Error broke it. Capture Error at module load like child_process.ts already does.
|
Updated 6:50 AM PT - Jul 24th, 2026
✅ @robobun, your commit 8222b861764acc5ae46e825e9113d444fc585ed3 passed in 🧪 To try this PR locally: bunx bun-pr 35409That installs a local version of the PR into your bun-35409 --bun |
|
Status: diff is green. Gate (fail-before/pass-after on ASAN and release) passed, CI is red on unrelated flaky tests that this diff doesn't touch (terminal ConPTY on Windows, install registry hoisting on win-aarch64, security-scanner snapshot on Alpine, multi-run on darwin). Retriggered once with the same pattern on different tests. Ready for a maintainer to merge. |
WalkthroughChangesNode util runtime behavior
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/js/node/util/util.test.js`:
- Around line 452-458: Update the subprocess assertions in
test/js/node/util/util.test.js at lines 452-458 and 491-494: keep draining
stderr but remove exact empty-stderr assertions, and combine the parsed stdout
result with exitCode in a single toEqual assertion for the style test and
getCallSites() test respectively.
🪄 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: 8c7ed520-f688-43bb-bc66-120047785826
📒 Files selected for processing (2)
src/js/node/util.tstest/js/node/util/util.test.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted follow-up to #34434 fixing two real regressions. The inline nit (null-valued inspect.colors entry falling through to buildStyleEntry(null)) is not a regression and is a garbage-in edge case; fine to address or leave.
Extended reasoning...
Overview
Two small fixes in src/js/node/util.ts plus two subprocess tests:
styleText: on cache miss, look upinspect.colors[key]live and cache the built entry (factored intobuildStyleEntry), instead of validating-then-crashing onstyle.openSeq. Restores pre-#34434 behavior for user-added colors.getCallSites: captureErrorat module load viavar Error = globalThis.Error, matching the existing pattern insrc/js/node/child_process.ts:1919, so replacingglobalThis.Errordoesn't break it.
Security risks
None. No untrusted-input parsing, no auth/crypto/permissions. inspect.colors has __proto__: null (inspect.js:760), so the new inspect.colors[key] read is not a prototype-pollution vector. Error is not in globalsToPrefix, so the module-level var Error shadow is the correct hardening approach and doesn't collide with codegen rewriting.
Level of scrutiny
Low-to-medium. Small, mechanical follow-up to a recent PR; both fixes restore prior behavior. The buildStyleEntry extraction is a pure refactor of the loop body. The only other Error reference in the file (callbackifyOnRejected's new Error(...)) is unaffected by the shadow.
Other factors
- Tests follow harness conventions:
it.concurrent,bunEnv/bunExe, concurrent pipe drain, stderr/stdout asserted before exitCode. Subprocess isolation is necessary since both tests mutate process-global state. - Checked the fast-path branch (
!validateStream && typeof format === "string") — on cache miss it correctly falls through to the general loop that now handles the live lookup. - Checked
skipColorizeinteraction — the new path builds and caches the entry even when colorization is skipped, but the trailingif (skipColorize) return textdiscards the accumulated codes, matching existing behavior for cached entries. - The one inline nit (own-property with nullish value passes
validateOneOfthen hitsbuildStyleEntry(null)) is real but not a regression — pre-PR crashed on the same input one line later, and Node isn't robust to it either.
var Error = globalThis.Error captures a reference, not a copy; a user that deletes or overwrites Error.captureStackTrace still broke getCallSites. Snapshot the function itself, matching internal/util/inspect.js and internal/assert/assertion_error.ts.
There was a problem hiding this comment.
LGTM — small, well-tested follow-up to #34434. The author's rationale on my earlier nit (Node v26.3.0 also crashes on a null inspect.colors entry, and the pre-PR path crashed on the same input) is sound; leaving that as-is is the right call for compat.
What was reviewed:
buildStyleEntryextraction and the liveinspect.colors[key]lookup on cache miss — invalid names still hitvalidateOneOf, and theskipColorizepath is unaffected (built entry is discarded by the post-loopreturn text).var Error = globalThis.Errorshadows the module-wide binding; audited other in-file uses (new ErrorincallbackifyOnRejected,Error.prepareStackTrace/stackTraceLimitingetCallSites) — all benefit or are neutral. Matches the existingchild_process.tspattern exactly.- Subprocess tests follow harness conventions (bunEnv, concurrent pipe drain, stderr → stdout → exitCode ordering).
Extended reasoning...
Overview
Two narrow follow-up fixes in src/js/node/util.ts to bugs introduced by #34434, plus two subprocess regression tests in test/js/node/util/util.test.js:
styleText: the one-timegetStyleCache()snapshot meant a color key added toinspect.colorsafter the first call would passvalidateOneOf(which reads live own-property names) and then crash onstyle.openSeq. The fix factors the entry-builder intobuildStyleEntry(codes)and, on a cache miss, reads the liveinspect.colors[key]and caches it. The refactor is a pure extraction — the original body of thegetStyleCache()loop is byte-identical to the new helper.getCallSites: capturesErrorandError.captureStackTraceat module load soglobalThis.Error = undefined/delete Error.captureStackTraceno longer breaks it. Uses the exactvar Error = globalThis.Errorpattern already shipped insrc/js/node/child_process.ts:1919.
Security risks
None. This is a built-in-module tamper-resistance improvement (capturing globals at load) and a cache-miss fallback for a user-mutable colors table. No parsing of untrusted input, no auth/crypto/fs surface.
Level of scrutiny
Low-to-medium. The runtime change is ~15 net lines, most of which is moving existing code into a named helper. The var Error module-level shadow is the widest-reaching line, so I audited every in-file Error reference: the only other consumer is new Error(...) in callbackifyOnRejected, which now becomes tamper-resistant rather than regressing. getCallSites continues to read/write .prepareStackTrace/.stackTraceLimit on the same underlying constructor object, so JSC's stack-capture machinery still sees the assignments.
Other factors
- My earlier nit (a user-assigned
nullcolor entry falls throughvalidateOneOfand crashes inbuildStyleEntry) was declined with a verified rationale: Node v26.3.0 crashes on the same input, and the pre-PR code crashed one line later, so it is neither a regression nor a compat divergence. That resolution is reasonable. - CodeRabbit's suggestion to drop the
expect(stderr).toBe("")assertion was withdrawn after the author pointed outbunEnvsetsBUN_DEBUG_QUIET_LOGS=1and this pattern is the documented convention. - Tests use
it.concurrent, drain both pipes withPromise.all, and assert exit code last — matching REVIEW.md's subprocess-test guidance. PR description states fail-before was confirmed and the existing 210 tests still pass.
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/js/node/util/util.test.js`:
- Around line 482-484: Update the test setup around deleting
Error.captureStackTrace to assert immediately after the delete that the property
is absent, before setting globalThis.Error to undefined. Keep the existing
cached Error.captureStackTrace scenario unchanged and ensure the assertion
verifies the deletion precondition.
🪄 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: 4d76f0c0-1bf9-4dd4-a2c7-3c6a1a0eaca5
📒 Files selected for processing (2)
src/js/node/util.tstest/js/node/util/util.test.js
1 participant
What
Two follow-up fixes to #34434 in
src/js/node/util.ts.styleText: cache miss on user-added color
getStyleCache()snapshotsinspect.colorsonce, but the unknown-key path validates against the liveObjectGetOwnPropertyNames(inspect.colors)and then unconditionally readsstyle.openSeq. A color added after the cache was built passes validation and then crashes. Before #34434 the lookup was always live so user-added colors worked.Fix: on cache miss, read the live
inspect.colors[key]and build/cache the entry. Invalid names still hitvalidateOneOfand throwERR_INVALID_ARG_VALUE; mutated or deleted built-in entries continue to resolve from the cache, matching Node v26.3.0.getCallSites: bare global
ErrorNode's implementation goes through an internal binding and is unaffected by the
Errorglobal. We implement it overError.prepareStackTrace/captureStackTrace, so replacingglobalThis.Errorbroke it. CaptureErrorat module load, the same patternchild_process.tsalready uses.The
Object.freeze(Error)case still throws because we must assignError.prepareStackTrace; matching Node there would need a native call-site binding.The handoff also flagged the
Buffer.fromcall inhexToRgb, butBufferis already in the codegenglobalsToPrefixlist and is rewritten to the private intrinsic at build time, so it is unaffected byglobalThis.Buffertampering (verified).Tests
Added two subprocess tests in
test/js/node/util/util.test.jscovering the added-color path and the replaced-Errorpath. Both fail on main and pass with this change; the rest of the file (210 tests) andtest-util-getcallsites-preparestacktrace.jscontinue to pass.[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file