fix: do not trigger TZ resolution for VTIMEZONE observance DTSTART#497
Conversation
DTSTART inside a STANDARD or DAYLIGHT sub-component is a plain local wall-clock time that marks when a DST rule takes effect. It must never go through fallbackWithStackTimezone(), which looks up the enclosing VTIMEZONE and calls resolveVTimezoneToIana() — crashing when the year is < 1000 (e.g. Microsoft's conventional "00010325T020000"). Root cause fix (ical.js): add an explicit STANDARD/DAYLIGHT branch in dateParameter that constructs the Date directly, skipping TZ resolution. Defensive guard (lib/tz-utils.js): clamp the probe year to ≥ 1970 and pad to 4 digits so any future call path reaching resolveVTimezoneToIana with a small year cannot produce the malformed ISO string "1-01-15T12:00:00Z" that Temporal.Instant.from() rejects. Fixes jens-maus#495
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughParses DTSTARTs inside VTIMEZONE STANDARD/DAYLIGHT observances as local wall-clock Date objects instead of resolving them via the VTIMEZONE-to-IANA fallback; also clamps probe years to a minimum of 1970 and zero-pads years when building Temporal.Instant probes to avoid parsing errors for years < 1000. Tests and fixtures added. Changes
Sequence Diagram(s)sequenceDiagram
participant Parser as Parser\n(parseICS / dateParameter)
participant VTimeResolver as VTIMEZONE Resolver\n(fallbackWithStackTimezone / resolveVTimezoneToIana)
participant Temporal as Temporal\n(Temporal.Instant.from)
participant JSDate as JS Date\n(new Date / setFullYear)
Parser->>VTimeResolver: needs timezone info for DTSTART in VTIMEZONE
alt DTSTART inside STANDARD/DAYLIGHT and not Z
Parser->>JSDate: construct local wall-clock Date(year, month, day, ...)
JSDate-->>Parser: Date object (normalized via setFullYear)
else fallback path
VTimeResolver->>Temporal: build probe instants (clamp year ≥1970, zero-pad)
Temporal-->>VTimeResolver: probe instants -> offsets
VTimeResolver-->>Parser: resolved IANA tz / offset
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ical.js`:
- Line 384: The constructed Date for VTIMEZONE observances uses new Date(year,
monthIndex, day, hour, minute, second) which misinterprets years 0–99; after the
existing assignment to newDate in ical.js (the newDate variable created from
year, monthIndex, day, hour, minute, second), call newDate.setFullYear(year) to
preserve the literal year value (so years 0001–0099 remain correct) while
keeping the existing time components and behavior used by pickApplicableBlock.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b02c577e-af31-4633-b41d-552e4254860a
📒 Files selected for processing (5)
ical.jslib/tz-utils.jstest/advanced.test.jstest/fixtures/vtimezone-year-0001-dtstart.icstest/tz-utils.test.js
new Date(year, ...) maps years 0–99 to 1900–1999; add setFullYear() to keep the actual year (e.g. 0001) so pickApplicableBlock sorts observance blocks correctly.
|
@jens-maus Coderabbit seems satisfied. If you are fine with this changes, a new release would be nice after merging 🙂 |
Problem
ICS files originating from Outlook or Exchange often contain Microsoft-style VTIMEZONE blocks with year 0001 in DTSTART. These can also appear in other calendar systems (Nextcloud, Google Calendar, etc.) when events are imported or shared from Microsoft sources.
Year 0001 is Microsoft's convention for "this rule has always been in effect". This is valid per RFC 5545.
Since node-ical 0.25.x, parsing these files throws:
The generic DTSTART handler was treating observance DTSTARTs the same as event DTSTARTs — trying to resolve the enclosing VTIMEZONE to an IANA zone. That resolution builds a probe timestamp from the year (
parseInt('0001') → 1), producing the malformed ISO string1-01-15T12:00:00ZwhichTemporal.Instant.from()rejects.Fix
Root cause (
ical.js): DTSTART inside STANDARD/DAYLIGHT sub-components is now recognized as a local wall-clock time for the observance rule. It skips timezone resolution entirely, which is the correct behavior — these timestamps define when a DST rule takes effect, not an event in a timezone.Defensive guard (
lib/tz-utils.js): IfresolveVTimezoneToIanais ever called with a year < 1000, the probe year is clamped to 1970 and zero-padded to 4 digits. IANA zone data before 1970 is unreliable anyway (RFC 5545 itself only guarantees data since 1970), so this has no practical impact on zone matching.Tests
resolveVTimezoneToIanawithyear=1no longer throwsFixes #495
Summary by CodeRabbit
Bug Fixes
Tests