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

[8.17] [ResponseOps] [Alerting] Handle invalid RRule params and prevent infinite looping (#205650) #205830

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Jan 8, 2025

Backport

This will backport the following commits from main to 8.17:

Questions ?

Please refer to the Backport tool documentation

\n\n### Questions ?\nPlease refer to the [Backport tool\ndocumentation](https://github.com/sqren/backport)\n\n\n\nCo-authored-by: Zacqary Adam Xeper "}},{"branch":"8.16","label":"v8.16.3","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.17","label":"v8.17.1","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT-->

…nite looping (elastic#205650)

## Summary

Closes elastic#205558

Updates the RRule library to correctly handle some scenarios with
invalid parameters that would either cause it to return strange
recurrence data or to infinitely loop. Specifically:

- On `RRule` object creation, removes and ignores any `bymonth`,
`bymonthday`, `byweekday`, or `byyearday` value that's out of bounds,
e.g. less than 0 or greater than the number of possible months, days,
weekdays, etc.
- Successfully ignores cases of `BYMONTH=2, BYMONTHDAY=30` (February
30th), an input that's complicated to invalidate but still won't ever
occur

Allowing these values to go unhandled led to unpredictable behavior. The
RRule library uses Moment.js to compare dates, but Moment.js months,
days, and other values generally start at `0` while RRule values start
at `1`. That led to several circumstances where we passed Moment.js a
value of `-1`, which Moment.js interpreted as moving to the
***previous*** year, month, or other period of time.

At worst, this could cause an infinite loop because the RRule library
was constantly iterating through the wrong year, never reaching the date
it was supposed to end on.

In addition to making the RRule library more able to handle these cases,
this PR also gives it a hard 100,000 iteration limit to prevent any
possible infinite loops we've missed.

Lastly, the Snooze Schedule APIs also come with additional validation to
hopefully prevent out of bounds dates from ever being set.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Janki Salvi <[email protected]>
Co-authored-by: Janki Salvi <[email protected]>
Co-authored-by: adcoelho <[email protected]>
(cherry picked from commit b302109)

# Conflicts:
#	packages/kbn-rrule/sanitize.test.ts
#	packages/kbn-rrule/sanitize.ts
#	packages/kbn-rrule/types.ts
@mikecote mikecote merged commit ebe9953 into elastic:8.17 Jan 8, 2025
11 checks passed
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
alerting 245 247 +2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants