From 36a3f3e60a3e34de8df3546e31443dcbe4e84dc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=C3=A1n=20de=20B=C3=BArca?= Date: Thu, 14 Sep 2023 12:04:11 -0700 Subject: [PATCH 1/3] Avoid infinite loop with invalid YEARLY recurrence rule. --- lib/ical/recur_iterator.js | 31 +++++++++++++++++++++++++------ test/recur_iterator_test.js | 19 +++++++++++++------ test/recur_test.js | 20 ++++++++++---------- 3 files changed, 48 insertions(+), 22 deletions(-) diff --git a/lib/ical/recur_iterator.js b/lib/ical/recur_iterator.js index a6e4b21e..9406e3d4 100644 --- a/lib/ical/recur_iterator.js +++ b/lib/ical/recur_iterator.js @@ -179,7 +179,14 @@ class RecurIterator { this.initialized = options.initialized || false; if (!this.initialized) { - this.init(); + try { + this.init(); + } catch (e) { + // Init may error if there are no possible recurrence instances from the + // rule, but we don't want to bubble this error up. Instead, we create + // an empty iterator. + this.completed = true; + } } } @@ -251,7 +258,17 @@ class RecurIterator { } if (this.rule.freq == "YEARLY") { - for (;;) { + // Some yearly recurrence rules may be specific enough to not actually + // occur on a yearly basis, e.g. the 29th day of February or the fifth + // Monday of a given month. The standard isn't clear on the intended + // behavior in these cases, but `libical` at least will iterate until it + // finds a matching year. + // CAREFUL: Some rules may specify an occurrence that can never happen, + // e.g. the first Monday of April so long as it falls on the 15th + // through the 21st. Detecting these is non-trivial, so ensure that we + // stop iterating at some point. + const untilYear = this.rule.until ? this.rule.until.year : 20000; + while (this.last.year <= untilYear) { this.expand_year_days(this.last.year); if (this.days.length > 0) { break; @@ -259,6 +276,10 @@ class RecurIterator { this.increment_year(this.rule.interval); } + if (this.days.length == 0) { + throw new Error("No possible occurrences"); + } + this._nextByYearDay(); } @@ -346,11 +367,10 @@ class RecurIterator { if ((this.rule.count && this.occurrence_number >= this.rule.count) || (this.rule.until && this.last.compare(this.rule.until) > 0)) { - - //XXX: right now this is just a flag and has no impact - // we can simplify the above case to check for completed later. this.completed = true; + } + if (this.completed) { return null; } @@ -360,7 +380,6 @@ class RecurIterator { return this.last; } - let valid; do { valid = 1; diff --git a/test/recur_iterator_test.js b/test/recur_iterator_test.js index 4ed19ce0..ee2fa938 100644 --- a/test/recur_iterator_test.js +++ b/test/recur_iterator_test.js @@ -143,13 +143,13 @@ suite('recur_iterator', function() { let start = ICAL.Time.fromString(options.dtStart); let recur = ICAL.Recur.fromString(ruleString); - if (options.throws) { - assert.throws(function() { - recur.iterator(start); - }); + + let iterator = recur.iterator(start); + + if (options.noInstance) { + assert.equal(iterator.next(), null); return; } - let iterator = recur.iterator(start); let inc = 0; let dates = []; @@ -688,7 +688,7 @@ suite('recur_iterator', function() { // Invalid rule. There's never a 31st of Feburary, check that this fails. testRRULE('FREQ=MONTHLY;INTERVAL=12;BYMONTHDAY=31', { dtStart: '2022-02-01T08:00:00', - throws: true, + noInstance: true, }); // monthly + by month @@ -963,6 +963,13 @@ suite('recur_iterator', function() { ] }); + // Invalid recurrence rule. The first Monday can never fall later than the + // 7th. + testRRULE('FREQ=YEARLY;BYMONTHDAY=15,16,17,18,19,20,21;BYDAY=1MO', { + dtStart: '2015-01-01T08:00:00', + noInstance: true, + }); + // Tycho brahe days - yearly, byYearDay with negative offsets testRRULE('FREQ=YEARLY;BYYEARDAY=1,2,4,6,11,12,20,42,48,49,-306,-303,' + '-293,-292,-266,-259,-258,-239,-228,-209,-168,-164,-134,-133,' + diff --git a/test/recur_test.js b/test/recur_test.js index a511cecb..2511d7c4 100644 --- a/test/recur_test.js +++ b/test/recur_test.js @@ -26,7 +26,7 @@ suite('recur', function() { }); } - function checkThrow(data, expectedMessage, dtstart, stack) { + function checkNoInstance(data, expectedMessage, dtstart, stack) { test(expectedMessage, function() { let recur = new ICAL.Recur(data); if (dtstart) { @@ -34,48 +34,48 @@ suite('recur', function() { } else { dtstart = ICAL.Time.epochTime.clone(); } - assert.throws(function() { - recur.iterator(dtstart); - }, expectedMessage); + + const iter = recur.iterator(dtstart); + assert.equal(iter.next(), null); }); } - checkThrow({ + checkNoInstance({ parts: { BYYEARDAY: [3, 4, 5], BYMONTH: [2] } }, 'Invalid BYYEARDAY rule'); - checkThrow({ + checkNoInstance({ parts: { BYWEEKNO: [3], BYMONTHDAY: [2] } }, 'BYWEEKNO does not fit to BYMONTHDAY'); - checkThrow({ + checkNoInstance({ freq: 'MONTHLY', parts: { BYWEEKNO: [30] } }, 'For MONTHLY recurrences neither BYYEARDAY nor BYWEEKNO may appear'); - checkThrow({ + checkNoInstance({ freq: 'WEEKLY', parts: { BYMONTHDAY: [20] } }, 'For WEEKLY recurrences neither BYMONTHDAY nor BYYEARDAY may appear'); - checkThrow({ + checkNoInstance({ freq: 'DAILY', parts: { BYYEARDAY: [200] } }, 'BYYEARDAY may only appear in YEARLY rules'); - checkThrow({ + checkNoInstance({ freq: 'MONTHLY', parts: { BYDAY: ['-6TH'] From 377646b84489a650ab25f0e19e75011eadce0867 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=C3=A1n=20de=20B=C3=BArca?= Date: Wed, 17 Apr 2024 15:51:03 -0700 Subject: [PATCH 2/3] Only catch invalid recurrence rule errors --- lib/ical/recur_iterator.js | 22 +++++++++++++++++----- test/recur_iterator_test.js | 9 ++++++++- test/recur_test.js | 20 ++++++++++---------- 3 files changed, 35 insertions(+), 16 deletions(-) diff --git a/lib/ical/recur_iterator.js b/lib/ical/recur_iterator.js index 9406e3d4..7dc90417 100644 --- a/lib/ical/recur_iterator.js +++ b/lib/ical/recur_iterator.js @@ -182,10 +182,15 @@ class RecurIterator { try { this.init(); } catch (e) { - // Init may error if there are no possible recurrence instances from the - // rule, but we don't want to bubble this error up. Instead, we create - // an empty iterator. - this.completed = true; + if (e instanceof InvalidRecurrenceRuleError) { + // Init may error if there are no possible recurrence instances from + // the rule, but we don't want to bubble this error up. Instead, we + // create an empty iterator. + this.completed = true; + } else { + // Propagate other errors to consumers. + throw e; + } } } } @@ -277,7 +282,7 @@ class RecurIterator { } if (this.days.length == 0) { - throw new Error("No possible occurrences"); + throw new InvalidRecurrenceRuleError(); } this._nextByYearDay(); @@ -1378,4 +1383,11 @@ class RecurIterator { return result; } } + +class InvalidRecurrenceRuleError extends Error { + constructor() { + super("Recurrence rule has no valid occurrences"); + } +} + export default RecurIterator; diff --git a/test/recur_iterator_test.js b/test/recur_iterator_test.js index ee2fa938..be486087 100644 --- a/test/recur_iterator_test.js +++ b/test/recur_iterator_test.js @@ -144,6 +144,13 @@ suite('recur_iterator', function() { let start = ICAL.Time.fromString(options.dtStart); let recur = ICAL.Recur.fromString(ruleString); + if (options.throws) { + assert.throws(function() { + recur.iterator(start); + }); + return; + } + let iterator = recur.iterator(start); if (options.noInstance) { @@ -688,7 +695,7 @@ suite('recur_iterator', function() { // Invalid rule. There's never a 31st of Feburary, check that this fails. testRRULE('FREQ=MONTHLY;INTERVAL=12;BYMONTHDAY=31', { dtStart: '2022-02-01T08:00:00', - noInstance: true, + throws: true, }); // monthly + by month diff --git a/test/recur_test.js b/test/recur_test.js index 2511d7c4..a511cecb 100644 --- a/test/recur_test.js +++ b/test/recur_test.js @@ -26,7 +26,7 @@ suite('recur', function() { }); } - function checkNoInstance(data, expectedMessage, dtstart, stack) { + function checkThrow(data, expectedMessage, dtstart, stack) { test(expectedMessage, function() { let recur = new ICAL.Recur(data); if (dtstart) { @@ -34,48 +34,48 @@ suite('recur', function() { } else { dtstart = ICAL.Time.epochTime.clone(); } - - const iter = recur.iterator(dtstart); - assert.equal(iter.next(), null); + assert.throws(function() { + recur.iterator(dtstart); + }, expectedMessage); }); } - checkNoInstance({ + checkThrow({ parts: { BYYEARDAY: [3, 4, 5], BYMONTH: [2] } }, 'Invalid BYYEARDAY rule'); - checkNoInstance({ + checkThrow({ parts: { BYWEEKNO: [3], BYMONTHDAY: [2] } }, 'BYWEEKNO does not fit to BYMONTHDAY'); - checkNoInstance({ + checkThrow({ freq: 'MONTHLY', parts: { BYWEEKNO: [30] } }, 'For MONTHLY recurrences neither BYYEARDAY nor BYWEEKNO may appear'); - checkNoInstance({ + checkThrow({ freq: 'WEEKLY', parts: { BYMONTHDAY: [20] } }, 'For WEEKLY recurrences neither BYMONTHDAY nor BYYEARDAY may appear'); - checkNoInstance({ + checkThrow({ freq: 'DAILY', parts: { BYYEARDAY: [200] } }, 'BYYEARDAY may only appear in YEARLY rules'); - checkNoInstance({ + checkThrow({ freq: 'MONTHLY', parts: { BYDAY: ['-6TH'] From feba263119aa4cca5e1302ef05eb1abd85414496 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=C3=A1n=20de=20B=C3=BArca?= Date: Thu, 18 Apr 2024 10:07:43 -0700 Subject: [PATCH 3/3] Add jsdoc and cover iterator "completed" value --- lib/ical/recur_iterator.js | 7 +++++++ test/recur_iterator_test.js | 1 + 2 files changed, 8 insertions(+) diff --git a/lib/ical/recur_iterator.js b/lib/ical/recur_iterator.js index 518a1a5e..3cecbb19 100644 --- a/lib/ical/recur_iterator.js +++ b/lib/ical/recur_iterator.js @@ -1385,6 +1385,13 @@ class RecurIterator { } } +/** + * An error indicating that a recurrence rule is invalid and produces no + * occurrences. + * + * @extends {Error} + * @class + */ class InvalidRecurrenceRuleError extends Error { constructor() { super("Recurrence rule has no valid occurrences"); diff --git a/test/recur_iterator_test.js b/test/recur_iterator_test.js index a870d0fd..34ada1af 100644 --- a/test/recur_iterator_test.js +++ b/test/recur_iterator_test.js @@ -155,6 +155,7 @@ suite('recur_iterator', function() { if (options.noInstance) { assert.equal(iterator.next(), null); + assert.ok(iterator.completed); return; }