Intl: accept legacy IANA primary zones (CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET)#227
Intl: accept legacy IANA primary zones (CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET)#227robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 34 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR expands time zone validation in JavaScriptCore to recognize legacy IANA identifiers. The change modifies isValidTimeZoneNameFromICUTimeZone to accept eight additional timezone names (CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET) that lack the "/" separator, alongside the previously accepted UTC and GMT. ChangesLegacy timezone identifier validation
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
Preview Builds
|
Bump WebKit to oven-sh/WebKit#227, which lets `Intl.DateTimeFormat`/`Date.prototype.toLocaleString` accept the 8 non-'/' IANA primary time zones: CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET These are first-class `Zone`s in the IANA `etcetera` file, so they are primary identifiers — but JSC's `isValidTimeZoneNameFromICUTimeZone` dropped every non-'/' name except `UTC`/`GMT`. On Linux/Windows (bundled ICU 75) they were therefore rejected with RangeError: invalid time zone: CET macOS was unaffected (system libicucore has a different canonical table). Per test262 `intl402/DateTimeFormat/timezone-case-insensitive.js`, each of these identifiers must round-trip to itself through `resolvedOptions().timeZone`; this also makes them appear in `Intl.supportedValuesOf('timeZone')`. Fixes #30618
robobun
force-pushed
the
farm/eb3d4eac/intl-datetimeformat-legacy-zones
branch
from
65540d6 to
4a8a597
Compare
Bump WebKit to oven-sh/WebKit#227, which lets `Intl.DateTimeFormat`/`Date.prototype.toLocaleString` accept the 8 non-'/' IANA primary time zones: CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET These are first-class `Zone`s in the IANA `etcetera` file, so they are primary identifiers — but JSC's `isValidTimeZoneNameFromICUTimeZone` dropped every non-'/' name except `UTC`/`GMT`. On Linux/Windows (bundled ICU 75) they were therefore rejected with RangeError: invalid time zone: CET macOS was unaffected (system libicucore has a different canonical table). Per test262 `intl402/DateTimeFormat/timezone-case-insensitive.js`, each of these identifiers must round-trip to itself through `resolvedOptions().timeZone`; this also makes them appear in `Intl.supportedValuesOf('timeZone')`. Fixes #30618
Bump WebKit to oven-sh/WebKit#227, which lets `Intl.DateTimeFormat`/`Date.prototype.toLocaleString` accept the 8 non-'/' IANA primary time zones: CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET These are first-class `Zone`s in the IANA `etcetera` file, so they are primary identifiers — but JSC's `isValidTimeZoneNameFromICUTimeZone` dropped every non-'/' name except `UTC`/`GMT`. On Linux/Windows (bundled ICU 75) they were therefore rejected with RangeError: invalid time zone: CET macOS was unaffected (system libicucore has a different canonical table). Per test262 `intl402/DateTimeFormat/timezone-case-insensitive.js`, each of these identifiers must round-trip to itself through `resolvedOptions().timeZone`; this also makes them appear in `Intl.supportedValuesOf('timeZone')`. Fixes #30618
Bump WebKit to oven-sh/WebKit#227, which lets `Intl.DateTimeFormat`/`Date.prototype.toLocaleString` accept the 8 non-'/' IANA primary time zones: CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET These are first-class `Zone`s in the IANA `etcetera` file, so they are primary identifiers — but JSC's `isValidTimeZoneNameFromICUTimeZone` dropped every non-'/' name except `UTC`/`GMT`. On Linux/Windows (bundled ICU 75) they were therefore rejected with RangeError: invalid time zone: CET macOS was unaffected (system libicucore has a different canonical table). Per test262 `intl402/DateTimeFormat/timezone-case-insensitive.js`, each of these identifiers must round-trip to itself through `resolvedOptions().timeZone`; this also makes them appear in `Intl.supportedValuesOf('timeZone')`. Fixes #30618
robobun
force-pushed
the
farm/eb3d4eac/intl-datetimeformat-legacy-zones
branch
from
4a8a597 to
21eee43
Compare
Bump WebKit to oven-sh/WebKit#227, which lets `Intl.DateTimeFormat`/`Date.prototype.toLocaleString` accept the 8 non-'/' IANA primary time zones: CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET These are first-class `Zone`s in the IANA `etcetera` file, so they are primary identifiers — but JSC's `isValidTimeZoneNameFromICUTimeZone` dropped every non-'/' name except `UTC`/`GMT`. On Linux/Windows (bundled ICU 75) they were therefore rejected with RangeError: invalid time zone: CET macOS was unaffected (system libicucore has a different canonical table). Per test262 `intl402/DateTimeFormat/timezone-case-insensitive.js`, each of these identifiers must round-trip to itself through `resolvedOptions().timeZone`; this also makes them appear in `Intl.supportedValuesOf('timeZone')`. Fixes #30618
robobun
force-pushed
the
farm/eb3d4eac/intl-datetimeformat-legacy-zones
branch
from
21eee43 to
de04db9
Compare
Bump WebKit to oven-sh/WebKit#227, which lets `Intl.DateTimeFormat`/`Date.prototype.toLocaleString` accept the 8 non-'/' IANA primary time zones: CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET These are first-class `Zone`s in the IANA `etcetera` file, so they are primary identifiers — but JSC's `isValidTimeZoneNameFromICUTimeZone` dropped every non-'/' name except `UTC`/`GMT`. On Linux/Windows (bundled ICU 75) they were therefore rejected with RangeError: invalid time zone: CET macOS was unaffected (system libicucore has a different canonical table). Per test262 `intl402/DateTimeFormat/timezone-case-insensitive.js`, each of these identifiers must round-trip to itself through `resolvedOptions().timeZone`; this also makes them appear in `Intl.supportedValuesOf('timeZone')`. Fixes #30618
robobun
force-pushed
the
farm/eb3d4eac/intl-datetimeformat-legacy-zones
branch
from
69d4fc9 to
a4b920e
Compare
Bump WebKit to oven-sh/WebKit#227, which lets `Intl.DateTimeFormat`/`Date.prototype.toLocaleString` accept the 8 non-'/' IANA primary time zones: CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET These are first-class `Zone`s in the IANA `etcetera` file, so they are primary identifiers — but JSC's `isValidTimeZoneNameFromICUTimeZone` dropped every non-'/' name except `UTC`/`GMT`. On Linux/Windows (bundled ICU 75) they were therefore rejected with RangeError: invalid time zone: CET macOS was unaffected (system libicucore has a different canonical table). Per test262 `intl402/DateTimeFormat/timezone-case-insensitive.js`, each of these identifiers must round-trip to itself through `resolvedOptions().timeZone`; this also makes them appear in `Intl.supportedValuesOf('timeZone')`. Fixes #30618
robobun
force-pushed
the
farm/eb3d4eac/intl-datetimeformat-legacy-zones
branch
from
a4b920e to
47fe010
Compare
Bump WebKit to oven-sh/WebKit#227, which lets `Intl.DateTimeFormat`/`Date.prototype.toLocaleString` accept the 8 non-'/' IANA primary time zones: CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET These are first-class `Zone`s in the IANA `etcetera` file, so they are primary identifiers — but JSC's `isValidTimeZoneNameFromICUTimeZone` dropped every non-'/' name except `UTC`/`GMT`. On Linux/Windows (bundled ICU 75) they were therefore rejected with RangeError: invalid time zone: CET macOS was unaffected (system libicucore has a different canonical table). Per test262 `intl402/DateTimeFormat/timezone-case-insensitive.js`, each of these identifiers must round-trip to itself through `resolvedOptions().timeZone`; this also makes them appear in `Intl.supportedValuesOf('timeZone')`. Fixes #30618
robobun
force-pushed
the
farm/eb3d4eac/intl-datetimeformat-legacy-zones
branch
from
47fe010 to
9ea730a
Compare
Bump WebKit to oven-sh/WebKit#227, which lets `Intl.DateTimeFormat`/`Date.prototype.toLocaleString` accept the 8 non-'/' IANA primary time zones: CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET These are first-class `Zone`s in the IANA `etcetera` file, so they are primary identifiers — but JSC's `isValidTimeZoneNameFromICUTimeZone` dropped every non-'/' name except `UTC`/`GMT`. On Linux/Windows (bundled ICU 75) they were therefore rejected with RangeError: invalid time zone: CET macOS was unaffected (system libicucore has a different canonical table). Per test262 `intl402/DateTimeFormat/timezone-case-insensitive.js`, each of these identifiers must round-trip to itself through `resolvedOptions().timeZone`; this also makes them appear in `Intl.supportedValuesOf('timeZone')`. Fixes #30618
robobun
force-pushed
the
farm/eb3d4eac/intl-datetimeformat-legacy-zones
branch
from
9ea730a to
3b117bd
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward allowlist extension for 8 legacy IANA primary zones.
Extended reasoning...
Overview
This PR touches a single static helper, isValidTimeZoneNameFromICUTimeZone, in Source/JavaScriptCore/runtime/IntlObject.cpp. It extends the existing || chain for slash-less time zone names from 2 entries (UTC, GMT) to 10, adding the 8 legacy IANA etcetera primary zones (CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET). The accompanying comment is updated to explain why.
Security risks
None. The function is a filter applied to names that ICU itself enumerates as canonical (ucal_openTimeZoneIDEnumeration(UCAL_ZONE_TYPE_CANONICAL, ...)), not to arbitrary user input. The change is purely additive — it allows 8 additional well-known constant strings through to the primary-zone table. There is no parsing, no memory management, no auth/crypto/permissions surface.
Level of scrutiny
Low. This is a small, mechanical allowlist expansion that follows the exact pattern already in place for UTC/GMT. The 8 identifiers are documented IANA primary zones, the fix is backed by a test262 expectation (intl402/DateTimeFormat/timezone-case-insensitive.js), and the PR description includes a clear root-cause analysis plus before/after verification. Worst case if the list were wrong is that a zone ICU already supports becomes resolvable via Intl.DateTimeFormat — not a correctness or safety hazard.
Other factors
The bug-hunting system found no issues, there are no outstanding human review comments, and preview builds completed across multiple revisions. The diff is ~15 lines of constant string comparisons with no control-flow changes beyond adding braces around the existing branch.
robobun
force-pushed
the
farm/eb3d4eac/intl-datetimeformat-legacy-zones
branch
from
3b117bd to
a31c734
Compare
There was a problem hiding this comment.
LGTM — straightforward allowlist extension; the inline nit about the comment wording is non-blocking.
Extended reasoning...
Overview
This PR touches a single function, isValidTimeZoneNameFromICUTimeZone in Source/JavaScriptCore/runtime/IntlObject.cpp, extending the allowlist of slash-less IANA time zone identifiers from {UTC, GMT} to additionally include the eight legacy POSIX-style zones CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET. The change is ~15 lines, all of it new string-equality comparisons in an existing || chain plus an updated explanatory comment.
Security risks
None. This widens the set of accepted time zone identifier strings by eight fixed, hard-coded ASCII literals. There is no parsing of untrusted input beyond exact string equality, no auth/crypto/permissions surface, and no memory-safety concern (StringView equality on literals).
Level of scrutiny
Low. The change is mechanical and self-contained, follows the existing pattern in the function, and is directly mandated by test262 (intl402/DateTimeFormat/timezone-case-insensitive.js) and ECMA-402. The PR description includes a clear before/after verification showing the eight zones now round-trip through resolvedOptions().timeZone, and explains why the previous filter incorrectly excluded them while EST/MST/HST slipped through via ICU canonicalisation.
Other factors
The one finding from the bug-hunting pass is a documentation nit: the new comment claims these are "first-class Zones in the IANA etcetera file", which became stale after tzdata 2024b moved them to backward as Links. The code is correct regardless of how IANA currently classifies them — test262 and ECMA-402 require accepting these identifiers either way — so this is a non-blocking suggestion to make the comment more future-proof. No CODEOWNERS, no outstanding reviewer comments, and preview builds have been produced for every revision.
| // IANA time zone names usually include '/'. The following legacy identifiers | ||
| // are first-class `Zone`s in the IANA `etcetera` file (not backward links), | ||
| // so they are primary IANA zones and must be treated as valid: | ||
| // CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET | ||
| // plus UTC/GMT which are always valid. |
There was a problem hiding this comment.
🟡 The comment states these identifiers "are first-class Zones in the IANA etcetera file (not backward links)", but as of tzdata 2024b (Sep 2024) all eight were moved out of etcetera and are now Link entries in the backward file. The code change is still correct — test262/ECMA-402 require these to be accepted regardless — but consider rewording the rationale to e.g. "legacy POSIX-style IANA identifiers required by ECMA-402/test262" so the comment doesn't go stale against current IANA data.
Extended reasoning...
What the issue is
The newly added comment at IntlObject.cpp:1954-1958 (and the PR description) asserts that CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET "are first-class Zones in the IANA etcetera file (not backward links)". That was historically accurate, but it is no longer true at the time this PR was written.
Why it's stale
In tzdata 2024b (released 2024-09-04), the IANA tz maintainers moved all of these names — along with EST, HST, MST — out of etcetera and into the backward file as Link entries pointing at geographical zones. The 2024b NEWS entry reads:
Names present only for compatibility with UNIX System V (last released in the 1990s) have been moved to 'backward'. … The affected names are CET, CST6CDT, EET, EST, EST5EDT, HST, MET, MST, MST7MDT, PST8PDT, and WET.
Checking upstream confirms it: etcetera no longer contains any of the eight names, and backward contains all of them — e.g. Link Europe/Brussels CET, Link America/Chicago CST6CDT, Link America/Los_Angeles PST8PDT. So the comment is wrong on both counts: they are not in etcetera, and they are backward links.
Step-by-step proof
- The PR description links to https://github.com/eggert/tz/blob/main/etcetera as the source for the "first-class Zone" claim.
- Fetch that file today: it defines only
Etc/GMT,Etc/UTC,Etc/GMT±N,GMT, and a handful ofEtc/links. None ofCET,CST6CDT,EET,EST5EDT,MET,MST7MDT,PST8PDT,WETappear. - Fetch
backwardfrom the same repo: each of the eight appears as aLinkline targeting a geographical zone. - Therefore the literal statement in the new comment — "first-class
Zones in the IANAetceterafile (not backward links)" — is factually incorrect against current upstream tzdata.
Why this doesn't affect correctness of the code
The functional change — whitelisting these eight names in isValidTimeZoneNameFromICUTimeZone — is still right. ECMA-402 and test262 (intl402/DateTimeFormat/timezone-case-insensitive.js) require these identifiers to be accepted and to round-trip through resolvedOptions().timeZone regardless of whether IANA currently classifies them as Zones or Links, and ICU's canonical enumeration still surfaces them. Only the rationale in the comment is misleading; a future maintainer cross-referencing IANA could be confused about why the whitelist exists.
Suggested fix
Reword the comment to avoid asserting current IANA Zone-vs-Link status, e.g.:
// IANA time zone names usually include '/'. The following are legacy
// POSIX/System V-style identifiers (CET, CST6CDT, EET, EST5EDT, MET,
// MST7MDT, PST8PDT, WET) that ECMA-402 and test262 require us to accept
// as valid named time zones, plus UTC/GMT which are always valid.This keeps the rationale durable even as IANA reshuffles Zone/Link classifications.
Bump WebKit to oven-sh/WebKit#227, which lets `Intl.DateTimeFormat`/`Date.prototype.toLocaleString` accept the 8 non-'/' IANA primary time zones: CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET These are first-class `Zone`s in the IANA `etcetera` file, so they are primary identifiers — but JSC's `isValidTimeZoneNameFromICUTimeZone` dropped every non-'/' name except `UTC`/`GMT`. On Linux/Windows (bundled ICU 75) they were therefore rejected with RangeError: invalid time zone: CET macOS was unaffected (system libicucore has a different canonical table). Per test262 `intl402/DateTimeFormat/timezone-case-insensitive.js`, each of these identifiers must round-trip to itself through `resolvedOptions().timeZone`; this also makes them appear in `Intl.supportedValuesOf('timeZone')`. Fixes #30618
…ET, MST7MDT, PST8PDT, WET)
These identifiers are first-class `Zone`s in the IANA `etcetera` file
(not backward links), so they are primary IANA time zone identifiers.
ICU reports them as canonical primaries, but
`isValidTimeZoneNameFromICUTimeZone` dropped every non-`/` name except
`UTC`/`GMT`, so `new Intl.DateTimeFormat(undefined, { timeZone: "CET" })`
threw `RangeError` on Linux/Windows. macOS was unaffected (system
libicucore has a different canonical table).
Fixes oven-sh/bun#30618
robobun
force-pushed
the
farm/eb3d4eac/intl-datetimeformat-legacy-zones
branch
from
a31c734 to
62bd098
Compare
There was a problem hiding this comment.
LGTM — the allowlist extension is correct and required by test262; my earlier note on the comment wording is non-blocking.
Extended reasoning...
Overview
This PR extends isValidTimeZoneNameFromICUTimeZone in Source/JavaScriptCore/runtime/IntlObject.cpp to accept eight legacy POSIX-style IANA timezone identifiers (CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET) that lack a / separator. Previously only UTC and GMT passed the no-slash filter, causing Intl.DateTimeFormat to throw RangeError for these zones on platforms using bundled ICU. The diff is +17/−4 in a single static helper, consisting purely of additional == comparisons against string literals.
Security risks
None. The change only widens an allowlist of hardcoded ASCII timezone names that gate which ICU-enumerated zones enter the primary timezone table. There is no user-controlled input parsing, no memory management change, no auth/crypto/permissions surface. The worst case of a wrong entry here would be an extra timezone name appearing in Intl.supportedValuesOf('timeZone').
Level of scrutiny
Low. This is a mechanical allowlist extension backed by a clear spec requirement: test262 intl402/DateTimeFormat/timezone-case-insensitive.js mandates these identifiers round-trip through resolvedOptions().timeZone, and the PR description includes before/after verification showing the fix works. The function is a simple predicate over StringView; there are no edge cases beyond exact-match string equality, and existing behavior for already-accepted names (anything with /, Etc/*, UTC, GMT) is unchanged.
Other factors
I previously left a 🟡 inline note that the code comment's rationale ("first-class Zones in the IANA etcetera file") is stale against tzdata 2024b, where these names became Link entries in backward. That is purely a documentation-accuracy nit — the functional change is correct regardless of IANA's Zone/Link classification, since ICU still surfaces these names and ECMA-402 requires accepting them. The author has not reworded the comment in the latest push, but I do not consider that blocking. No other reviewers have raised concerns, and the bug-hunting system found no issues this run.
Bump WebKit to oven-sh/WebKit#227, which lets `Intl.DateTimeFormat`/`Date.prototype.toLocaleString` accept the 8 non-'/' IANA primary time zones: CET, CST6CDT, EET, EST5EDT, MET, MST7MDT, PST8PDT, WET These are first-class `Zone`s in the IANA `etcetera` file, so they are primary identifiers — but JSC's `isValidTimeZoneNameFromICUTimeZone` dropped every non-'/' name except `UTC`/`GMT`. On Linux/Windows (bundled ICU 75) they were therefore rejected with RangeError: invalid time zone: CET macOS was unaffected (system libicucore has a different canonical table). Per test262 `intl402/DateTimeFormat/timezone-case-insensitive.js`, each of these identifiers must round-trip to itself through `resolvedOptions().timeZone`; this also makes them appear in `Intl.supportedValuesOf('timeZone')`. Fixes #30618
1 participant
Problem
The 8 identifiers
CET,CST6CDT,EET,EST5EDT,MET,MST7MDT,PST8PDT,WETare first-classZones in the IANAetceterafile — not backward links. They are primary IANA time zone identifiers.ICU reports them as canonical primaries (
isSystem=1,canonical(CET) = CET, etc.), butisValidTimeZoneNameFromICUTimeZonedropped every non-/name exceptUTC/GMT, so these zones never enteredintlAvailableTimeZones(). They also could not be found viaintlAvailableTimeZoneIndex:toPrimaryIanaTimeZoneIdentifier("CET")returns"CET"unchanged (no backward link), and the primary table does not contain"CET", so the index-building code skips them too.intlAvailableNamedTimeZonetherefore returnsstd::nulloptandIntlDateTimeFormat::initializeDateTimeFormatthrows.EST,MST,HSThappen to be accepted (but only by the index path) because ICU canonicalises them toEtc/GMT+5,Etc/GMT+7,Etc/GMT+10, all of which are in the primary table.macOS was unaffected because it links against system
libicucore, which has a different canonical table.Per test262
intl402/DateTimeFormat/timezone-case-insensitive.js, each of these identifiers is expected to round-trip to itself throughresolvedOptions().timeZone.Fix
Whitelist the 8 names in
isValidTimeZoneNameFromICUTimeZone, so they enter the primary table and are found by the binary search in bothintlResolveTimeZoneIDandintlAvailableNamedTimeZone.Verification
Case-insensitive lookups still normalise to the canonical casing (
cet→CET), and the 8 zones now appear inIntl.supportedValuesOf("timeZone").Fixes oven-sh/bun#30618