Skip to content

fix(formatter): stop the error message regexes from retrying each position - #241

Open
musatoktas wants to merge 2 commits into
yoavbls:mainfrom
musatoktas:fix/187-redos-regexes
Open

musatoktas wants to merge 2 commits into
yoavbls:mainfrom
musatoktas:fix/187-redos-regexes

Conversation

@musatoktas

Copy link
Copy Markdown

Fixes #187

The regexes in #187 are slow on a line where a rule finds its start many times but cannot complete a match. The engine tries every start again, and each try reads to the end of the line, so the time grows with the square of the line length.

What changes

  • Each rule in errorMessagePrettifier.ts gets a last alternative after its start: (?<rest>.*). When a rule cannot match from a start, no later start on the same line can match either, because the content parts are .*? or .* (a match from a later start is also a match from the earlier one). So the rest group takes the rest of the line in one step. For the ['“]([^>]*)['”] (type|...) rule the rest is [^>]*, and for the last rule it is (?:(?!["]).)*: the same characters as their content.
  • createErrorMessagePrettifier keeps a match with a rest group as it is.
  • addMissingParentheses.ts:54: the content cannot go past a ( that starts another (name:. The match then ends at the same ), so the result is the same. In addMissingParentheses the old regex was not slow, because the function adds a ) for each unclosed (. This change only makes the regex itself linear.

Apart from that, the regexes and their captures do not change. I did not use negated character classes: TypeScript writes quoted property names with single quotes ({ 'first-name': string; }), and a class like [^']* changes the output for those (errorWithDashInObjectKeys, errorWithSpecialCharsInObjectKeys and the is missing the following properties case).

Checks

  • recheck 4: before, all 13 regexes from the issue table are vulnerable (polynomial). After, all 13 are safe.

  • The attack strings from the issue, through the prettifier, Node 20 on GitHub Actions:

    attack string main (ms) this branch (ms)
    string literal 3397 19
    declare module 7798 6
    missing properties 3465 5
    types and 3789 17
    type annotation 3817 4
    overload 3802 14
    module or file 2082 2
    type 6682 4
    quoted before a keyword 2357 2
    keyword 1409 1
    return or operator 3776 3
    quoted 4770 33
  • Same output: I compared main and this branch on 8,506 messages, with the test stub and with hoverCodeBlock. The messages are the 22 mocks and the 2,121 messages of TypeScript 5.9.3's diagnosticMessages.json, each filled 4 times with different types. There are 0 differences.

  • Tests: one new test per attack string, which must finish in under 500 ms. Without the source change, all 12 fail (1.4 s to 7.2 s each). With it, the 39 formatter tests and the 10 vscode-formatter tests pass, and format:check and the build pass.

This covers the 13 regexes in the issue table.

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.

ReDoS Vulnerable Regular Expressions - unlikely to be triggered

1 participant