Fixes T15371
Details
Details
- Reviewers
aklapper - Group Reviewers
O1: Blessed Committers - Maniphest Tasks
- T15371: RuntimeException in preg_replace_callback: Text disappears due to catastrophic backtracking regex in Remarkup parsing
- Commits
- rP9c73d62c4466: Rewrite regex for project names to be not prone to catastrophic backtracking
- Save the text {{#translation:}} in remarkup and see that it renders.
- Create a project or projects with the hashtags a, b, ab, foo, f.o.o.
- Observe that both before and after this patch you can link to all of them by hashtag.
- Create a project or projects with the hashtags a., .b, .foo, foo..
- Observe that both before and after this patch you can link to none of them by hashtag.
Diff Detail
Diff Detail
- Repository
- rP Phorge
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
https://devina.io/redos-checker says the new regex is still prone to polynomial backtracking, but that can happen only on extremely contrived input, whereas the old one was exponential on a common case.
Comment Actions
(I still need to understand the old intentions on the old regex, and the new one, but it seems much magically readable, thanks)
| src/applications/project/remarkup/ProjectRemarkupRule.php | ||
|---|---|---|
| 49 | (pls restore this super-supa-important newline at line 49 lol) | |
Comment Actions
Big thanks for digging deep into that regex (on which I gave up).
The proposed code change seems correct to me:
- Given an existing project tag #whatever, I entered various strings like {{{#whatever:}}}, {{{#whatever_}}}, {{#whatever:}}, {{#whatever}}, {{#whatever_}}, {#whatever}, {#whatever:}, {#whatever } and always got something reasonable™ rendered in the task comment preview without any errors or exceptions in the error log.
- Entering the existing project tag #projectwithcolon: (note the colon at the end) renders an additional (second) colon as suffix but this also already happens in current git master without this very patch applied thus not a regression.
Please feel free to arc land after a git rebase master (as this patch seems to be against master branch and not in a dedicated branch).