Repository navigation
Conversation
…D[+-]hh')
JSC's DateCache resolves the default zone by name through
intlResolveTimeZoneID and collapses to UTC when that lookup fails, so
TZ values libc and Node honor (':/etc/localtime', 'MSK-3', 'UTC+5',
'/usr/share/zoneinfo/...') produced a 0 offset. Normalize them before
handing off to JSC: strip a leading ':', realpath absolute tzfile paths
and take the '/zoneinfo/' suffix, and map the POSIX 'std offset' form
to the matching Etc/GMT zone. An unrecognized value now clears the
override so a stale zone does not persist.
Fixes #8060
|
Warning Review limit reached
Next review available in: 19 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Updated 9:41 AM PT - Jul 25th, 2026
⏳ @robobun, your commit e145bce is still building in
|
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
…tests host-zone agnostic
There was a problem hiding this comment.
No new findings this pass — the earlier feedback on test hermeticity and angle-bracket coverage has been addressed. Deferring to a human for the final call given this is ~100 lines of new C++ in the JSC bindings with a hand-rolled POSIX TZ parser, a realpath() filesystem probe, and a deliberate semantic change (unrecognized TZ now clears the prior override rather than leaving it in place).
What was reviewed:
- Traced
posixStdOffsetToEtcGMTagainst every accept/reject case in the test matrix; the<...>branch andnotFoundreject path are now both covered. - Confirmed
bun:jsc'ssetTimeZone(BunJSCModule.h:629) is a separate path that still callsWTF::setTimeZoneOverridedirectly and throws — unchanged by this PR as the description claims. - Checked all Rust callers of
JSGlobalObject__setTimeZone(run/test/repl commands) — all are startup-TZ-from-env, so the widened acceptance is intentional there. - Verified the reworked fall-through tests are now POSIX-gated with the OS
TZpinned toEtc/UTC, eliminating the host-zone dependency flagged earlier.
Extended reasoning...
Overview
The PR normalizes process.env.TZ values before handing them to JSC's WTF::setTimeZoneOverride, so POSIX conventions that ICU cannot canonicalize by name (leading :, absolute tzfile paths like :/etc/localtime, and STD[+|-]hh offset specs) resolve to a real IANA zone instead of collapsing to UTC. The change is ~100 lines of new C++ in JSEnvironmentVariableMap.cpp (setTimeZoneFromEnvValue + posixStdOffsetToEtcGMT), a one-line rewire in ZigGlobalObject.cpp so the startup path shares it, a header export, and a new 148-line test file.
Security risks
Low. The only new attack-surface-adjacent piece is realpath() on a user-controlled absolute path, but TZ is already user-controlled by definition, the call is read-only, the buffer is a stack char[PATH_MAX] (which is what realpath requires), and the result is fed through intlResolveTimeZoneID before being trusted. The parser operates on StringView with bounds-checked indexing throughout; I traced the digit loop and i - hstart > 2 cap and did not find an overflow path. No injection, auth, or data-exposure surface.
Level of scrutiny
This warrants a human look. It is not a mechanical change: there is a hand-rolled parser, an #if OS(UNIX) filesystem probe, and a deliberate behavioral change — an unrecognized TZ value now clears the WTF override (commit(emptyString())) where the old code left the previous override in place. That change is intentional per the PR description and covered by the last test, but it is the kind of user-visible semantic shift a maintainer should sign off on. The Etc/GMT±h mapping's asymmetric range (+1..+12 / -1..-14) and the decision to run the POSIX-offset mapping on Windows too (where Node ignores TZ entirely) are also design calls worth a human eye.
Other factors
All four items I raised in earlier passes are resolved in the current revision: the fall-through tests are now skipIf(!isPosix) with the child's OS TZ pinned to Etc/UTC and runtime assignment (so ICU's host-zone fallback is deterministic), the Windows registry-zone edge case is avoided by dropping the not.toBe(a) assertion in favor of expect(b).toBe(host) under the POSIX guard, and the angle-bracket branch has both accept (<+03>-3) and reject (<+03-3) coverage. The bug-hunting system found nothing new this run. I confirmed bun:jsc's setTimeZone still goes through its own strict path in BunJSCModule.h and is unaffected, and that the three Rust-side callers of JSGlobalObject__setTimeZone are all startup-TZ-from-env, matching the PR's intent. The old value.length() < 32 guard is dropped, which is fine — setTimeZoneOverride bounds-checks internally and long IANA names like America/Argentina/ComodRivadavia were previously being rejected by it.
|
CI status: the new |
|
Two more runtime-only rejections in the same family, observed on 1.4.0-canary (launch-time Reading the diff, Unrelated to the above: #36045 drops the |
|
Closing: #46698 replaces this PR. It keeps the two parts of this one that hold on the current WebKit: the |
Fixes #8060. Fixes #3649.
Repro
Cause
JSC's
DateCacheresolves the default time zone purely by name: it feeds the WTF override (or ICU'sucal_getHostTimeZonestring) throughintlResolveTimeZoneID, and when that lookup fails it falls back to a default-constructedTimeZone, i.e. UTC. V8 hits the same ICU path but keeps theSimpleTimeZoneICU builds from libc'stimezoneoffset, so Node still reports the rightgetTimezoneOffset()for POSIX TZ specs even whenIntlcannot name the zone.Bun's startup and
process.env.TZsetter passed the raw value straight toWTF::setTimeZoneOverride, which only accepts names ICU'sucal_getCanonicalTimeZoneIDrecognises. So:/etc/localtime,/usr/share/zoneinfo/..., andSTD[+-]hhPOSIX specs all collapsed to UTC.Fix
Normalize the TZ value on the Bun side before installing the override (
Bun::setTimeZoneFromEnvValue, shared by the startupJSGlobalObject__setTimeZonepath and the runtimeprocess.env.TZsetter)::(the glibc convention);realpath()an absolute path and take the/zoneinfo/suffix so:/etc/localtimeresolves to the IANA zone it points at;intlResolveTimeZoneIDstill rejects the name, try ICU's primary-IANA canonical form;std[+|-]hhand map it to the matchingEtc/GMTzone (sign preserved, since both conventions are hours west of UTC).An unrecognised value clears the override so the previous zone does not persist past a bad assignment.
bun:jsc'ssetTimeZone()stays strict and still throws on non-IANA names.The
STD[+-]hhmapping runs on all platforms, soTZ=MSK-3is honoured on Windows too; Node ignoresTZon Windows entirely, so this is intentionally more permissive there.Known limitation
Node honours whole-hour POSIX offsets out to ±24h via a
SimpleTimeZone, but Bun maps throughEtc/GMT±hwhich only exists for+1..+12/-1..-14.TZ=XXX+13therefore still falls through. Extending the range would mean plumbing an offset-basedTimeZoneinto JSC'sDateCache, which is out of scope here.Verification
test/js/node/process/process-env-tz.test.tscovers startup and runtime assignment for all of the above, including a symlink that stands in for a container-mounted/etc/localtime. 11 of the 20 cases fail on main and all pass with this change; the existingprocess.env.TZandbun:jscsetTimeZonetests continue to pass.