[ResponseOps] [Alerting] Handle invalid RRule params and prevent infinite looping #205650

Zacqary · 2025-01-06T19:50:29Z

Summary

Closes #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

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2025-01-06T19:50:32Z

Pinging @elastic/response-ops (Team:ResponseOps)

…-fix'

…/kibana into 205558-rrule-invalid-fix

…d-fix

src/platform/packages/shared/kbn-rrule/sanitize.ts

src/platform/packages/shared/kbn-rrule/rrule.ts

adcoelho · 2025-01-07T10:45:34Z

src/platform/packages/shared/kbn-rrule/rrule.ts

@@ -345,7 +306,7 @@ const getMonthOfRecurrences = function ({
 }: IterOptions) {
  const derivedByweekday = byweekday ?? ISO_WEEKDAYS;
  const currentMonth = refDT.month();
-  if (bymonth && !bymonth.includes(currentMonth)) return [];
+  if (bymonth && !bymonth.includes(currentMonth + 1)) return [];


What is this change about? I can't understand what getMonthOfRecurrences does

This change handles the difference between bymonth starting January at 1 and Moment.js starting January at 0. currentMonth is a Moment.js month, so we need to add 1 to it to accurately check if it matches bymonth.

getMonthOfRecurrences iterates over the month specified by refDT (a Moment object) to find all recurrences within that month. For example, if we pass Jan 7 2025 10:00:00, it will iterate over all of January 2025.

This is used for recurrences that happen at a monthly frequency or a yearly frequency.

This function previously wasn't being called with a bymonth parameter very often (yearly frequency was using a method to omit bymonth, which was a misguided approach that caused some infinite looping issues), so we never caught this bug.

js-jankisalvi

Tested locally and validation works as expected.

src/platform/packages/shared/kbn-rrule/rrule.ts

adcoelho · 2025-01-07T11:18:52Z

src/platform/packages/shared/kbn-rrule/rrule.ts

@@ -112,12 +66,17 @@ export class RRule {
      .toDate();

    const nextRecurrences: Moment[] = [];
+    let iters = 0;

    while (
      (!count && !until) ||
      (count && yieldedRecurrenceCount < count) ||
      (until && current.getTime() < new Date(until).getTime())


If until is already type Date why are we doing new Date(until).getTime()?

This is probably by mistake but at least it doesn't throw.

> const until = new Date('February 25, 2022 03:24:00') undefined > until 2022-02-25T02:24:00.000Z > const foobar = new Date(until) undefined > foobar 2022-02-25T02:24:00.000Z > foobar.getTime() 1645755840000 > until.getTime() 1645755840000

agree, until is already a sanitized date, so it is safe to use until.getTime()

src/platform/packages/shared/kbn-rrule/sanitize.ts

…d-fix

elasticmachine · 2025-01-07T18:38:57Z

💚 Build Succeeded

Buildkite Build
Commit: 10af5fe

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`alerting`	223	225	+2

History

ymao1 · 2025-01-07T18:50:58Z

src/platform/packages/shared/kbn-rrule/rrule.ts

@@ -74,19 +32,15 @@ type AllResult = Date[] & {
 };

 const ALL_LIMIT = 10000;
+const TIMEOUT_LIMIT = 100000;


Just a drive-by comment wondering if we really need to let it run 100,000 iterations before hard-exiting?

I am not familiar with this stuff at all but seems like if we could lower this and have it exit earlier, would be better. But if we need up to 100,000 iterations, then you can disregard :)

If the default limit on getting "all" recurrences of a schedule that has no end is 10k, we want to give it enough of a buffer that it might actually be able to find 10k recurrences.

Kibana isn't really hitting those limits right now but since 100k iterations takes a negligible amount of compute time to do, it seems like a decent ceiling to future-proof this.

kibanamachine · 2025-01-07T19:33:00Z

Starting backport for target branches: 8.16, 8.17, 8.x

https://github.com/elastic/kibana/actions/runs/12658052124

…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)

kibanamachine · 2025-01-07T19:38:36Z

💔 Some backports could not be created

Status	Branch	Result
❌	8.16	Backport failed because of merge conflicts
❌	8.17	Backport failed because of merge conflicts You might need to backport the following PRs to 8.17: - [AVC Banner] Updates the AVC Banner for 2025 (#205467) - [Security Solution] Fix timeline dynamic batching (#204034)
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Manual backport

To create the backport manually run:

node scripts/backport --pr 205650

Questions ?

Please refer to the Backport tool documentation

…t infinite looping (#205650) (#205803) # Backport This will backport the following commits from `main` to `8.x`: - [[ResponseOps] [Alerting] Handle invalid RRule params and prevent infinite looping (#205650)](#205650)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Zacqary Adam Xeper <[email protected]>

…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]>

…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 · 2025-01-08T01:10:37Z

💚 All backports created successfully

Status	Branch	Result
✅	8.17
✅	8.16

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…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/rrule.ts # packages/kbn-rrule/sanitize.test.ts # packages/kbn-rrule/sanitize.ts # packages/kbn-rrule/types.ts

…nt infinite looping (#205650) (#205830) # Backport This will backport the following commits from `main` to `8.17`: - [[ResponseOps] [Alerting] Handle invalid RRule params and prevent infinite looping (#205650)](#205650)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) \n\n### Questions ?\nPlease refer to the [Backport tool\ndocumentation](https://github.com/sqren/backport)\n\n\n\nCo-authored-by: Zacqary Adam Xeper <[email protected]>"}},{"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--> Co-authored-by: Zacqary Adam Xeper <[email protected]>

…nt infinite looping (#205650) (#205831) # Backport This will backport the following commits from `main` to `8.16`: - [[ResponseOps] [Alerting] Handle invalid RRule params and prevent infinite looping (#205650)](#205650)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) \n\n### Questions ?\nPlease refer to the [Backport tool\ndocumentation](https://github.com/sqren/backport)\n\n\n\nCo-authored-by: Zacqary Adam Xeper <[email protected]>"}},{"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--> --------- Co-authored-by: Zacqary Adam Xeper <[email protected]>

…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]>

fake-haris · 2025-01-10T18:23:17Z

Are there any user facing changes that I can test for this PR, if so how? Thank you!

…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]>

Zacqary added 2 commits January 6, 2025 13:29

[RRule] Handle invalid parameters

01d9d03

Add iteration limit

2615266

Zacqary added release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v9.0.0 Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework v8.18.0 labels Jan 6, 2025

Zacqary requested a review from a team as a code owner January 6, 2025 19:50

Zacqary added the backport:version Backport to applied version labels label Jan 6, 2025

kibanamachine and others added 7 commits January 6, 2025 20:22

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

d63be31

…-fix'

Fix lint

1c15cf4

Merge branch '205558-rrule-invalid-fix' of https://github.com/Zacqary…

25be647

…/kibana into 205558-rrule-invalid-fix

Fix bad merge

518ccff

Merge remote-tracking branch 'upstream/main' into 205558-rrule-invali…

2eee1c6

…d-fix

Merge branch 'main' into 205558-rrule-invalid-fix

0f1fb65

remove packages/kbn-repo-packages/package-map.json

1ca46d7

adcoelho reviewed Jan 7, 2025

View reviewed changes

src/platform/packages/shared/kbn-rrule/sanitize.ts Show resolved Hide resolved

adcoelho reviewed Jan 7, 2025

View reviewed changes

src/platform/packages/shared/kbn-rrule/rrule.ts Show resolved Hide resolved

adcoelho reviewed Jan 7, 2025

View reviewed changes

js-jankisalvi reviewed Jan 7, 2025

View reviewed changes

src/platform/packages/shared/kbn-rrule/rrule.ts Show resolved Hide resolved

adcoelho reviewed Jan 7, 2025

View reviewed changes

add sanitize.test.ts

be1a1be

adcoelho reviewed Jan 7, 2025

View reviewed changes

src/platform/packages/shared/kbn-rrule/sanitize.ts Outdated Show resolved Hide resolved

adcoelho approved these changes Jan 7, 2025

View reviewed changes

js-jankisalvi approved these changes Jan 7, 2025

View reviewed changes

add validation for options.interval

d3662ea

adcoelho force-pushed the 205558-rrule-invalid-fix branch from 91be1b6 to d3662ea Compare January 7, 2025 15:29

Merge remote-tracking branch 'upstream/main' into 205558-rrule-invali…

10af5fe

…d-fix

ymao1 added the v8.17.0 label Jan 7, 2025

ymao1 added v8.16.3 v8.17.1 and removed v8.17.0 labels Jan 7, 2025

ymao1 reviewed Jan 7, 2025

View reviewed changes

Zacqary merged commit b302109 into elastic:main Jan 7, 2025
15 checks passed

kibanamachine mentioned this pull request Jan 7, 2025

[8.x] [ResponseOps] [Alerting] Handle invalid RRule params and prevent infinite looping (#205650) #205803

Merged

mikecote mentioned this pull request Jan 8, 2025

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

Merged

mikecote mentioned this pull request Jan 8, 2025

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ResponseOps] [Alerting] Handle invalid RRule params and prevent infinite looping #205650

[ResponseOps] [Alerting] Handle invalid RRule params and prevent infinite looping #205650

Zacqary commented Jan 6, 2025 •

edited by kibanamachine

Loading

elasticmachine commented Jan 6, 2025

adcoelho Jan 7, 2025

Zacqary Jan 7, 2025 •

edited

Loading

js-jankisalvi left a comment

adcoelho Jan 7, 2025

js-jankisalvi Jan 7, 2025

elasticmachine commented Jan 7, 2025

ymao1 Jan 7, 2025

ymao1 Jan 7, 2025

Zacqary Jan 7, 2025

kibanamachine commented Jan 7, 2025

kibanamachine commented Jan 7, 2025

mikecote commented Jan 8, 2025

fake-haris commented Jan 10, 2025

[ResponseOps] [Alerting] Handle invalid RRule params and prevent infinite looping #205650

[ResponseOps] [Alerting] Handle invalid RRule params and prevent infinite looping #205650

Conversation

Zacqary commented Jan 6, 2025 • edited by kibanamachine Loading

Summary

Checklist

elasticmachine commented Jan 6, 2025

adcoelho Jan 7, 2025

Choose a reason for hiding this comment

Zacqary Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

js-jankisalvi left a comment

Choose a reason for hiding this comment

adcoelho Jan 7, 2025

Choose a reason for hiding this comment

js-jankisalvi Jan 7, 2025

Choose a reason for hiding this comment

elasticmachine commented Jan 7, 2025

💚 Build Succeeded

Metrics [docs]

Module Count

History

ymao1 Jan 7, 2025

Choose a reason for hiding this comment

ymao1 Jan 7, 2025

Choose a reason for hiding this comment

Zacqary Jan 7, 2025

Choose a reason for hiding this comment

kibanamachine commented Jan 7, 2025

kibanamachine commented Jan 7, 2025

💔 Some backports could not be created

Manual backport

Questions ?

mikecote commented Jan 8, 2025

💚 All backports created successfully

Questions ?

fake-haris commented Jan 10, 2025

Zacqary commented Jan 6, 2025 •

edited by kibanamachine

Loading

Zacqary Jan 7, 2025 •

edited

Loading