fix: prevent ReDoS in no-reversed-media-syntax - #693
Conversation
|
|
lumirlumir
left a comment
There was a problem hiding this comment.
Thanks for the PR, but I think this solution is too verbose and does more than what we really want to fix in this PR. I’d say let’s pinpoint the problematic regex pattern and fix it surgically, as you mentioned in the issue, such as replacing \([\s\S]*\) with a non-backtracking form like \([^()]*\), or something similar.
Also, please follow our AI Usage Policy and update the PR description to match our template.
The `label` group nested `\([\s\S]*\)` inside a `*`, overlapping the other alternatives, so text with many parenthesised spans and no `)[` backtracked exponentially. Bounding the nested group to `\([^()]*\)` makes each character's parse unambiguous. A label may now contain at most one level of nested parentheses. Fixes eslint#690
26a7d2d to
7056183
Compare
| `Text (unclosed ${"and (x) ".repeat(30)}end`, | ||
| "(".repeat(10000), | ||
| // A label may contain at most one level of nested parentheses. | ||
| "(x (a (b)) y)[url]", |
There was a problem hiding this comment.
Nice catch on the regression.
As mentioned in the PR description, if there’s a way to keep reporting this pattern, I’d be in favor of continuing to report it.
Since the rule expects that case to be transformed into [x (a (b)) y](url) (with the [ and ( patterns swapped), and that’s still valid Markdown Link syntax.
There was a problem hiding this comment.
Happy to keep reporting it — but I want to check which trade-off you'd prefer, because JS regex has no recursion, so any pure-pattern fix has some fixed nesting bound. The question is only where we put it.
Option A — move the bound one level deeper. Nest the group once more:
// label's third alternative: \((?:\\.|[^()\\]|\([^()]*\))*\)
/(?<=(?<!\\)(?:\\{2})*)\((?<label>(?:\\.|[^()\\]|\((?:\\.|[^()\\]|\([^()]*\))*\))*)\)\[(?<url>(?:\\.|[^\]\\\r\n])*)\](?!\()/guThe alternatives stay disjoint, so this is still backtracking-free — "(".repeat(100_000) runs in ~1.3ms, the #690 input in <0.01ms, and 20,000 nested (x) groups in ~0.6ms.
| input | current PR | Option A |
|---|---|---|
(x (a) y)[url] |
reported | reported |
(x (a (b)) y)[url] |
not reported | reported |
(x (a (b (c))) y)[url] |
not reported | not reported |
So it fixes the case you flagged, but the cliff just moves from depth 2 to depth 3, and each extra level makes the pattern noticeably harder to read.
Option B — match the label with a balanced-paren scan instead of encoding nesting in the regex. That handles arbitrary depth in linear time, but it's the more verbose direction you steered me away from in the first review, so I don't want to go there without your say-so.
My read is that depth 3+ in a reversed link label is rare in real Markdown, so Option A buys most of the benefit for a two-token change — but you know the rule's users better than I do. Which would you like? If Option A, I'll push it and turn "(x (a (b)) y)[url]" into an invalid case, with a valid case pinning the new depth-3 limit (or drop that pin entirely if you'd rather
not commit to it).
There was a problem hiding this comment.
Thanks for the detailed summary! I now understand the regex-based limitations clearly.
My read is that depth 3+ in a reversed link label is rare in real Markdown
As you said, I also agree that depth 3+ is rare in Markdown documents, so I think it’s fine to keep the current solution.
Just to clarify the nested-depth limitation, could we add a small note to https://github.com/eslint/markdown/blob/main/docs/rules/no-reversed-media-syntax.md explaining that 3+ nested depth patterns are not supported, with a small example?
It would be helpful if this section could be placed under ## Options as ## Known Limitations, similar to https://eslint.org/docs/latest/rules/no-throw-literal#known-limitations.
Co-authored-by: lumir <yonghyeon0324@gmail.com>
Prerequisites checklist
AI acknowledgment
What is the purpose of this pull request?
Fix the catastrophic backtracking in
no-reversed-media-syntaxreported in #690.What changes did you make? (Give an overview)
Bounded the nested group in
reversedPatternfrom\([\s\S]*\)to\([^()]*\), as suggested in review.The
labelgroup is(?: \\. | [^()\\] | \([\s\S]*\) )*. The third alternative could match text the first two could also match, so when the overall match failed the engine explored every way of splitting the input between the outer*and the inner[\s\S]*. Bounding the nested group removes that overlap: each character now has exactly one possible parse.Timings for the reproduction in #690:
and (x)Growth is now linear — 50,000 repeats parse in 2.4ms, and
"(".repeat(50000)in 0.7ms.Added two
validcases from the issue as regression tests.Related Issues
Fixes #690
Is there anything you'd like reviewers to focus on?
This narrows what a label can contain, which is a behavior change.
\([\s\S]*\)accepted arbitrarily deep nesting;\([^()]*\)accepts one level:All 93 existing tests pass unchanged, so nothing currently covered regresses. I added
"(x (a (b)) y)[url]"as avalidcase to document the new limit — happy to remove it if you would rather not pin that, or to extend the pattern by one more level if two-deep labels are worth supporting.I verified equivalence against the old pattern by differential testing over 500,000 random inputs built from parentheses, brackets, backslashes, spaces, line breaks and astral characters; the only divergences are the nesting-depth cases above.