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

fix: prevent recursion in task regex #6639

Merged
merged 1 commit into from
Mar 3, 2023
Merged

Conversation

wdoconnell
Copy link
Contributor

@wdoconnell wdoconnell commented Mar 3, 2023

Users are hitting an error trying to save this data migration task in the Cloud2 UI. This PR fixes it by preventing a recursive loop caused by regex.

Before

Screen.Recording.2023-03-03.at.7.38.22.AM.mov

After

Screen.Recording.2023-03-03.at.7.33.29.AM.mov

Cause

The following regex on the "create task" page was intended to ensure that what the user selects for the task name and frequency in the panel on the left-hand side of the page takes precedence over any definition of the task name and frequency in the script.

script = script.replace(new RegExp('option\\s+task\\s+=\\s+{(.|\\s)*}'), '')

In other words, "Delete the part of the script that says: option task = { whatever is in between these brackets }".

The problem is that the capturing group (.|\s)* causes catastrophic backtracking. It's looking for a match of any number of appearances of any whitespace character (\s) OR any character (.), the overlap of which results in excessive recursion. What we need is a check for "any whitespace character or any non-whitespace character until the closing bracket."

The fix makes these changes:

  1. Instead of looking for "any character or any whitespace character" (these two conditions overlap), look for "any whitespace or non-whitespace character" before the closing bracket.
  2. Compile the expression when the script is loaded, rather than at runtime, since the regex is static.

Checklist

Authors and Reviewer(s), please verify the following:

  • A PR description, regardless of the triviality of this change, that communicates the value of this PR
  • Well-formatted conventional commit messages that provide context into the change
  • Documentation updated or issue created (provide link to issue/PR)
  • Signed CLA (if not already signed)
  • Feature flagged, if applicable - NO

@wdoconnell wdoconnell marked this pull request as ready for review March 3, 2023 13:22
@wdoconnell wdoconnell requested a review from a team as a code owner March 3, 2023 13:22
@wdoconnell wdoconnell requested review from a team and eatondustin1 March 3, 2023 13:22
@wdoconnell wdoconnell marked this pull request as draft March 3, 2023 14:07
|> range(start: -2m)`
)
// Fix bug with monaco error inserting an extra } here.
.monacoType('{backSpace}')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test wasn't working correctly - the monaco editor was adding an extra } at the end, and the old regex was deleting that too - likely caught by the greedy quantifier *. The new regex has a lazy quantifier (*?), so it will (correctly) only match the first closing bracket from option task.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@wdoconnell wdoconnell marked this pull request as ready for review March 3, 2023 15:18
Copy link
Contributor

@appletreeisyellow appletreeisyellow left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!! 🎉

|> range(start: -2m)`
)
// Fix bug with monaco error inserting an extra } here.
.monacoType('{backSpace}')

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@wdoconnell wdoconnell added this pull request to the merge queue Mar 3, 2023
Merged via the queue into master with commit 022199e Mar 3, 2023
@wdoconnell wdoconnell deleted the fix_task_recursion branch March 3, 2023 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants