Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid infinite loop with invalid YEARLY recurrence rule #621

Merged
merged 4 commits into from
Apr 18, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 25 additions & 6 deletions lib/ical/recur_iterator.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
leftmostcat marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

Expand Down Expand Up @@ -251,14 +258,28 @@ 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;
}
this.increment_year(this.rule.interval);
}

if (this.days.length == 0) {
throw new Error("No possible occurrences");
}

this._nextByYearDay();
}

Expand Down Expand Up @@ -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;
}

Expand All @@ -360,7 +380,6 @@ class RecurIterator {
return this.last;
}


let valid;
do {
valid = 1;
Expand Down
19 changes: 13 additions & 6 deletions test/recur_iterator_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also assert that completed is set, while you are at it.

return;
}
let iterator = recur.iterator(start);

let inc = 0;
let dates = [];
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,' +
Expand Down
20 changes: 10 additions & 10 deletions test/recur_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,56 +26,56 @@ 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) {
dtstart = ICAL.Time.fromString(dtstart);
} 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');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spec says that The BYYEARDAY rule part MUST NOT be specified when the FREQ rule part is set to DAILY, WEEKLY, or MONTHLY., so this combination should normally be an error instead of just not producing instances.

I guess that brings up the bigger question of should we just ignore such errors and be lenient, or should we throw. What about using ICAL.design.strict to determine if it should throw or silently show no instances?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The robustness principle suggests to me that we should only error if we reach a point where we simply can't interpret the file any longer. I'd be okay with using strict as a guard here if that makes sense to you. Probably the best thing to do in this instance is to favor the FREQ property and any invalid parts that follow are silently dropped, resulting in occurrences that follow whatever can be validly interpreted. Unfortunately, the spec provides rules for how to create valid iCalendar objects, but no real guidance on how to deal with invalid ones.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, generally I'd like ical.js to be able to parse everything and be lenient. The strict mode was meant for the case where you might use ical.js as a validator and would want it to bail. Let's put this behind the strict mode check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't quite sure how to access whether we were in strict mode from the recurrence iter, so I've implemented the infinite loop prevention without modifying any throwing behavior. If that behavior is desire, it may be best to address that in a followup.


checkThrow({
checkNoInstance({
freq: 'MONTHLY',
parts: {
BYDAY: ['-6TH']
Expand Down