Skip to content

fix: avoid catastrophic backtracking in number parsing for empty thousand separator (DEV-2120) - #1713

Merged
sequba merged 6 commits into
developfrom
fix/dev-2120-redos-number-parsing
Jul 28, 2026
Merged

fix: avoid catastrophic backtracking in number parsing for empty thousand separator (DEV-2120)#1713
sequba merged 6 commits into
developfrom
fix/dev-2120-redos-number-parsing

Conversation

@marcin-kordas-hoc

@marcin-kordas-hoc marcin-kordas-hoc commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

Entering a long run of digits ending in a non-digit character (e.g. 012345678901234567890123456789012345678901234567890123456789a) into a cell froze the page when formulas were enabled. Reported as #1520, where the non-digit character is a space. DEV-2120 / HOT-9767.

Root cause

NumberLiteralHelper builds its number-detection pattern by interpolating the configured separators. With the default thousandSeparator: '', the group (${thousandSeparator}\d{3,})* degenerates to (\d{3,})* placed immediately after \d+:

^([+-]?((\.\d+)|(\d+(\d{3,})*(\.\d*)?)))([eE][+-]?\d+)?$
             └──── nested quantifiers on the same class ────┘

For a long digit run that ultimately fails to match (trailing non-digit), the engine explores exponentially many ways to partition the digits between \d+ and the repeated \d{3,} group — classic catastrophic backtracking (ReDoS). Parse time roughly doubles every ~2 characters, so a 60-character input never returns.

The same pattern is reached from raw cell input (CellContentParser) and from string→number coercion during formula evaluation (ArithmeticHelper), so =VALUE("…") and arithmetic over such text hung too. Fixing the pattern builder covers all entry points.

Fix

Omit the thousand-separator group entirely when the separator is empty. The emitted pattern for a non-empty separator (,, , .) is byte-for-byte unchanged — a literal separator is a mandatory anchor between repetitions, so no ambiguous partition exists and those configs were never vulnerable.

Testing

Paired tests in handsontable/hyperformula-tests (branch fix/dev-2120-redos-number-parsing):

  • white-box guard that the default-config pattern contains no nested digit quantifier (deterministic regression tripwire — a synchronous ReDoS cannot be caught by a per-test timeout);
  • behavioral coverage: trailing letter, trailing non-letter symbol, separator matrix, long-integer value fidelity;
  • end-to-end via setCellContents (raw, percent, currency) and formula coercion (=VALUE(...));
  • the verbatim reproduction from #1520 (90 digits, a space, then 123) built through buildFromArray with the sheet layout from the issue, including the dependent =SUM(A1,B1) formula.

Reviewer notes

  • Why the white-box test (asserting numberPattern.source has no (\d{3,})*): a synchronous ReDoS cannot be caught by a Jest/Jasmine per-test timeout — the timer can't fire while the regex is stuck on the main thread — so asserting the emitted pattern shape is the one deterministic regression tripwire. The behavioral/e2e tests still cover actual behavior.
  • Verified against the reported input, not just a variant: with the NumberLiteralHelper change reverted, the new #1520 test hangs until killed (timeout 90 → exit 124); with the fix it finishes in ~20 ms. The default-config pattern goes from ^([+-]?((\.\d+)|(\d+(\d{3,})*(\.\d*)?)))([eE][+-]?\d+)?$ to ^([+-]?((\.\d+)|(\d+(\.\d*)?)))([eE][+-]?\d+)?$.
  • Why no input-length cap: the fix sits in the pattern builder, so it covers every entry point at once, and non-empty separators are provably linear (the literal separator anchors each repetition). A length cap would be complementary defense-in-depth — deliberately left out to keep this fix focused on the root cause.

Notes

Branch brought up to date with develop by merge (not rebase) to preserve review history.

Long-standing issue (reproduced on docs v17.1 and v18.0), not a v18 regression.

🤖 Generated with Claude Code


Note

Low Risk
Small, targeted regex construction change with unchanged behavior for non-empty thousand separators; low risk aside from edge cases in numeric string detection.

Overview
Fixes UI freezes when users enter a long digit string that fails number parsing (e.g. trailing letter or space before more digits), including the #1520 reproduction.

NumberLiteralHelper no longer emits the (\d{3,})* thousand-separator group when thousandSeparator is the default empty string. That degenerate pattern sat next to \d+ and caused catastrophic backtracking on near-miss inputs; the same helper is used for raw cell parsing and formula coercion (VALUE, arithmetic), so one regex change covers those paths. Configs with a non-empty thousand separator keep the previous pattern shape.

Reviewed by Cursor Bugbot for commit 6ad1637. Bugbot is set up for automated code reviews on this repo. Configure here.

@netlify

netlify Bot commented Jul 21, 2026

Copy link
Copy Markdown

Deploy Preview for hyperformula-dev-docs ready!

Name Link
🔨 Latest commit 6ad1637
🔍 Latest deploy log https://app.netlify.com/projects/hyperformula-dev-docs/deploys/6a68b362f8a4820008c42a9f
😎 Deploy Preview https://deploy-preview-1713--hyperformula-dev-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@qunabu

qunabu commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Task linked: HF-154 Agent-friendly docs

Comment thread scripts/extract-public-api.js Outdated
@marcin-kordas-hoc
marcin-kordas-hoc force-pushed the fix/dev-2120-redos-number-parsing branch from 6637a91 to 2f1ca3a Compare July 21, 2026 14:25
@github-actions

github-actions Bot commented Jul 21, 2026

Copy link
Copy Markdown

Performance comparison of head (6ad1637) vs base (d860eef)

                                     testName |    base |    head |  change
---------------------------------------------------------------------------
                                      Sheet A |  504.43 |  484.15 |  -4.02%
                                      Sheet B |  169.06 |  153.15 |  -9.41%
                                      Sheet T |  148.96 |  135.87 |  -8.79%
                                Column ranges |  483.92 |  460.59 |  -4.82%
                                Sorted lookup | 14359.5 | 13511.3 |  -5.91%
Sheet A:  change value, add/remove row/column |   15.55 |   13.96 | -10.23%
 Sheet B: change value, add/remove row/column |  145.04 |   125.9 | -13.20%
                   Column ranges - add column |  159.99 |  141.41 | -11.61%
                Column ranges - without batch |   482.8 |  429.96 | -10.94%
                        Column ranges - batch |  120.32 |  107.95 | -10.28%

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 2f1ca3a. Configure here.

Comment thread docs/.vuepress/plugins/md-companions/strip.js
…sand separator (DEV-2120)

When thousandSeparator is empty (the default), the number-detection pattern's
group `(sep\d{3,})*` degenerated to `(\d{3,})*` adjacent to `\d+`, producing
catastrophic regex backtracking (ReDoS) on a long run of digits ending in a
non-digit character. Entering such a value into a cell froze the page.

Omit the thousand-separator group entirely when the separator is empty. Behavior
for non-empty separators is unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@marcin-kordas-hoc
marcin-kordas-hoc force-pushed the fix/dev-2120-redos-number-parsing branch from 2f1ca3a to 8ec26f4 Compare July 21, 2026 14:55
@marcin-kordas-hoc
marcin-kordas-hoc requested a review from sequba July 21, 2026 17:01
Comment thread CHANGELOG.md Outdated

- Fixed the behavior of `MATCH`, `VLOOKUP`, `HLOOKUP`, and `XLOOKUP` functions when the search range contained empty cells. [#1697](https://github.com/handsontable/hyperformula/pull/1697)
- Fixed the `VLOOKUP`, `HLOOKUP`, and `XLOOKUP` functions to return `0` instead of an empty value when the matched cell in the result range is empty. [#1697](https://github.com/handsontable/hyperformula/pull/1697)
- Fixed the page freezing when entering a long string of digits ending in a non-digit character (e.g. `012...789a`) into a cell, caused by catastrophic regex backtracking in number parsing. [#1713](https://github.com/handsontable/hyperformula/pull/1713)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reference this task in changelog

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added in b136fec7a, wording widened in f022c9792 — the entry now cites #1520 next to the PR. Used the GitHub issue rather than the internal id, matching the rest of the file.

marcin-kordas-hoc and others added 4 commits July 28, 2026 07:08
The entry linked only the pull request. Add the reported bug it fixes,
matching the issue-first citation convention used elsewhere in the file.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…2120)

The entry cited issue #1520 but described the trigger as digits "ending in"
a non-digit character. In the reported repro the non-digit is followed by
three more digits, so it sits near the end rather than at it. Widen the
wording and add the reported shape as a second example.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread CHANGELOG.md Outdated
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.22%. Comparing base (d860eef) to head (6ad1637).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1713   +/-   ##
========================================
  Coverage    97.21%   97.22%           
========================================
  Files          178      178           
  Lines        15611    15612    +1     
  Branches      3429     3430    +1     
========================================
+ Hits         15177    15178    +1     
  Misses         426      426           
  Partials         8        8           
Files with missing lines Coverage Δ
src/NumberLiteralHelper.ts 94.11% <100.00%> (+0.36%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sequba
sequba merged commit ad142ba into develop Jul 28, 2026
36 checks passed
@sequba
sequba deleted the fix/dev-2120-redos-number-parsing branch July 28, 2026 14:12
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.

3 participants