Repository navigation
Conversation
The runtime process.env.TZ setter gated on value.length() < 32 before calling WTF::setTimeZoneOverride, so any zone id of 32+ characters was silently dropped. "America/Argentina/ComodRivadavia" is exactly 32 characters and a valid IANA link zone; assigning it mid-process left the previous zone in place while the env var itself updated. The launch-time path (JSGlobalObject__setTimeZone) and bun:jsc's setTimeZone have no such cap, and WTF::setTimeZoneOverride already validates the name through ICU's ucal_getCanonicalTimeZoneID and returns false for anything it rejects, so the length check is redundant. Fixes #5370
WalkthroughChangesTimezone override handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
Comment |
|
Updated 10:21 PM PT - Jul 26th, 2026
✅ @robobun, your commit b35ac52e22abe000e675dad38d618b6e66999767 passed in 🧪 To try this PR locally: bunx bun-pr 36045That installs a local version of the PR into your bun-36045 --bun |
|
Reproduced with |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Related but not duplicates: #35594 and #35003 are broader TZ-semantics changes (POSIX forms, revert-on-invalid) that happen to rewrite this function; both are still open. This PR is the minimal one-line fix for #5370 specifically (drop the length cap so 32+ char IANA ids like Note #35000 keeps the |
There was a problem hiding this comment.
LGTM — one-token deletion of a redundant length guard, aligning the runtime TZ setter with the two other paths that already call WTF::setTimeZoneOverride uncapped.
What was reviewed:
- Confirmed
JSGlobalObject__setTimeZone(launch-time) andbun:jscsetTimeZonealready callWTF::setTimeZoneOverridewith no length cap, so this only removes an inconsistency. - Test restores TZ in
finally, asserts the 32-char boundary, and checks a concrete offset (180) rather than the ICU-version-dependent canonical name.
Extended reasoning...
Overview
Removes the value.length() < 32 && short-circuit from applyTZFromString in src/jsc/bindings/JSEnvironmentVariableMap.cpp, so process.env.TZ = <zone> reaches WTF::setTimeZoneOverride for zone ids of 32+ characters. Adds a regression test in test/js/node/process/process.test.js using the 32-char IANA link zone America/Argentina/ComodRivadavia.
Security risks
None. The removed guard was a length pre-filter in front of a validating ICU call; WTF::setTimeZoneOverride already returns false for invalid zone ids (as demonstrated by bun:jsc's setTimeZone at src/jsc/modules/BunJSCModule.h:649, which throws on that false). No untrusted-size allocation, no new user-reachable surface.
Level of scrutiny
Low. This is a one-token deletion that brings the runtime setter in line with the two other TZ-setting call sites in the codebase (JSGlobalObject__setTimeZone at ZigGlobalObject.cpp:3356 and functionSetTimeZone in BunJSCModule.h), neither of which has a length cap. The pre-existing behavior of the sibling paths is the strongest evidence the guard was redundant.
Other factors
- The test asserts
.not.toBe("Asia/Tokyo")plus a concretegetTimezoneOffset() === 180instead of the exact canonical zone name — sensible, since ICU may canonicalize the link zone to eitherAmerica/Argentina/CatamarcaorAmerica/Catamarcaacross versions. Argentina has no DST, so 180 is stable. try/finallyrestoresprocess.env.TZ, matching the neighboringprocess.env.TZtest's pattern.- PR description shows fails-before (
USE_SYSTEM_BUN=1) / passes-after (bun bd test).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/process/process.test.js`:
- Around line 274-288: Update the test setup around origTimezone to capture the
original process.env.TZ value and whether the variable was unset, rather than
deriving it from Intl.DateTimeFormat(). In the finally block, restore the exact
original environment state by deleting process.env.TZ when it was initially
absent, otherwise assigning the captured value.
- Around line 282-286: Update the timezone assertions around the long variable
in the process environment test: replace the broad “not Asia/Tokyo” check with a
comparison between the resolved time zone and an explicit Intl.DateTimeFormat
configured with timeZone: long, allowing for ICU canonicalization. Keep the
existing offset assertion.
🪄 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: ffee27d1-ff23-4e3e-8324-4dc542a2e2bb
📒 Files selected for processing (2)
src/jsc/bindings/JSEnvironmentVariableMap.cpptest/js/node/process/process.test.js
There was a problem hiding this comment.
LGTM — one-line removal of a redundant length guard, with a regression test.
What was reviewed:
- Confirmed the two sibling paths (
JSGlobalObject__setTimeZonein ZigGlobalObject.cpp,setTimeZonein BunJSCModule.h) already callWTF::setTimeZoneOverridewith no length cap, so this brings the runtime setter in line. WTF::setTimeZoneOverridevalidates via ICU and returns false on rejection, so dropping the pre-filter doesn't lose any validation.- Test restores
process.env.TZinfinally, matching the existing TZ test right above it; uses.not.toBe("Asia/Tokyo")+ a concrete offset assertion since ICU canonicalizes the link zone name.
Extended reasoning...
Overview
Single-line change in src/jsc/bindings/JSEnvironmentVariableMap.cpp: removes value.length() < 32 && from the condition in applyTZFromString, so the runtime process.env.TZ setter passes any string to WTF::setTimeZoneOverride instead of silently dropping values ≥32 chars. Adds a 17-line regression test in test/js/node/process/process.test.js next to the existing process.env.TZ test.
Security risks
None. WTF::setTimeZoneOverride validates through ICU's ucal_getCanonicalTimeZoneID and returns false for anything it rejects; the two other in-tree callers (JSGlobalObject__setTimeZone at ZigGlobalObject.cpp:3356 and bun:jsc's setTimeZone at BunJSCModule.h:649) already invoke it with no length bound, so this is not a new input surface.
Level of scrutiny
Low. This is a targeted bug fix removing a guard that was demonstrably wrong (rejected valid IANA zone ids like America/Argentina/ComodRivadavia) and inconsistent with the launch-time and bun:jsc paths. No memory management, no new control flow, no API surface change.
Other factors
- Verification in the PR shows the new test fails under
USE_SYSTEM_BUN=1and passes with the debug build. - The test asserts
.not.toBe("Asia/Tokyo")for the resolved zone rather than an exact name because ICU canonicalizes the link zone (Bun →America/Argentina/Catamarca, Node →America/Catamarca); the concretegetTimezoneOffset() === 180assertion covers the actual behavior. - Test mutates
process.env.TZin-process but restores it infinally, exactly like the pre-existingprocess.env.TZtest immediately above. - Two other open PRs (#35594, #35003) touch the same guard as part of larger refactors; the author addressed this and this minimal fix is independently landable.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-27 and it conflicts with main. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
Repro
Launch-time
TZ=America/Argentina/ComodRivadavia bun -e ...already worked; only the mid-process setter was affected.Cause
applyTZFromStringinsrc/jsc/bindings/JSEnvironmentVariableMap.cppgated onvalue.length() < 32 && WTF::setTimeZoneOverride(value). The strict< 32dropped every value of 32+ characters before ICU ever saw it. The launch-time path (JSGlobalObject__setTimeZone) andbun:jsc'ssetTimeZonehave no such cap.WTF::setTimeZoneOverridealready validates through ICU'sucal_getCanonicalTimeZoneIDand returnsfalsefor anything it rejects, so the length pre-filter is redundant and only serves to reject valid long zone ids (and tzfile paths).Fix
Remove the
value.length() < 32 &&guard.Verification
no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/process/process.test.js