Fix: remove catastrophic backtracking vulnerability

[davisjam]

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Change template substitution regex to exclude fields with whitespace.
This addresses possible O(n^2) catastrophic backtracking behavior.
The function behavior is unchanged.

Very unlikely to be exploited. For #10002.

Is there anything you'd like reviewers to focus on?

No.

[jsf-clabot]

All committers have signed the CLA.

[ljharb]

So it's just the \s* that's problematic here?

[ljharb]

term.trim()?

[davisjam]

Haha! I knew there had to be a function for that.

[davisjam]

So it's just the \s* that's problematic here?

A bit more complex than that. The original pattern contains a sub-sequence like this:

\s*[^{}]+\s*

Because the middle group's character class includes whitespace, it will match a superset of this pattern, which is easier to reason about:

\s*\s+\s*

If the input contains a long sequence of whitespace, the regex engine has to decide when to move from one \s to the next.
If the input doesn't match the pattern, the regex engine tries every subdivision of the whitespace looking for a match. It lays breadcrumbs each time it makes a decision to resolve ambiguity, and revisiting each breadcrumb is called backtracking. If there is a long sequence of whitespace, it has a lot of breadcrumbs to visit, and will get "stuck". This behavior is called catastrophic backtracking.

The blow-up in this case is O(n^2). In the worst case catastrophic backtracking can be O(2^n).

[davisjam]

It would be helpful if a developer can comment on whether this is a very low-risk security problem or just a potential problem if someone else were to copy/paste later.

If a real problem, I will follow up with Snyk.io to get a low-severity vulnerability ID assigned.

[ljharb]

It's an eslint rule config; imo it's the lowest nonzero risk you can get :-)

[not-an-aardvark]

I don't think this is realistically exploitable, because only an ESLint rule can provide a string to that regex, and an ESLint rule already can execute arbitrary code. So I'm fine with removing it for performance reasons, but I don't consider this to be a security issue in its current location.

[not-an-aardvark]

Looks good to me, thanks!

[platinumazure]

LGTM, this seems reasonable and doesn't make things much harder to read. Thanks for the PR!