Bound catastrophic regex backtracking in user-authored patterns - #5037
builder-io-integration[bot] wants to merge 6 commits into
Conversation
…he browser An agent asked to make "Full Name" accept at least two words wrote `^([A-Za-z]+\s?)+$`. It compiles cleanly and backtracks exponentially: 748 ms on a 26-character value, doubling with every further character. The Forms editor tab stopped responding and Chrome offered to kill the page. The same pattern is re-checked in the submit handler, so it pegs the server event loop too, and no JavaScript timeout can interrupt a match once V8 is inside it. Add analyzeRegexSource / compileUserRegex / testUserRegex to @agent-native/core/shared, which refuse patterns shaped like this rather than trying to time them out, and return a tri-state so "was not evaluated" stays distinguishable from "did not match". Forms rejects an unsafe pattern at the authoring gate shared by create-form, update-form and patch-form-fields, naming a safe rewrite so the agent can correct itself; the fill page, submit handler and public SSR runtime bound patterns already stored. The same defect was present in Calendar booking fields, whose existing input/pattern length caps are ineffective against it, and in the Slides regex-replace edit op. Also teach guard:i18n-changed-copy the export-default messagesByLocale wrapper shape, which apps using it could not otherwise satisfy.
builder-io-integration
Bot
changed the title
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@builderio-bot look at the latest PR feedback and fix anything you agree with. Be skeptical. Reply in each open inline thread with exactly one of:
Or resolve the thread in GitHub. Outdated threads after new commits do not need a new reply. Get CI green and keep the branch mergeable. |
…77ac63c899f04da0a544
Six false negatives, each runtime-validated as catastrophic before fixing:
- The probe alphabet was a fixed list, so any pattern over characters it omitted
produced empty character sets and read as unambiguous. `^(A+)+$` and `^(x|x)+$`
were cleared; `^(a+)+$` only failed because "a" happened to be in the list.
The alphabet is now derived from the pattern itself, and an atom the probes
cannot describe is "unknown" and overlaps everything rather than being treated
as disjoint.
- Flags are now part of the verdict. `^(a|A)+$` is unambiguous on its own and
catastrophic under `i`; the Slides regex-replace op analyzed the source while
running it with flags.
- Ambiguity checks keyed on infinite quantifiers only, so `^(a{1,10})+$` evaded
them. They now key on variable length.
- Overlapping alternatives were only compared when both sides were a single
atom, clearing `^(a|aa)+$`. They are now compared on leading characters and
minimum length, which still admits `(cat|car)+`.
- Three chained repetitions over the same characters backtrack cubically and
exceed the input cap: `^(a+)(a+)(a+)$` takes over 20s at 4096 characters. Runs
of three or more are now rejected; pairs are quadratic and stay admitted, so
the standard email pattern is unaffected.
- The public form runtime skipped the pattern check for values over the cap, so
an unchecked value looked valid in the browser while the server rejected it.
It now reports the value as unchecked.
Corpus expanded to 16 catastrophic and 17 legitimate patterns; every cleared
pattern verified at 0 ms against adversarial input at the full 4096-char cap.
There was a problem hiding this comment.
Builder reviewed your changes and found 6 potential issues 🔴
Review Details
Incremental Code Review Summary
The latest commit materially addresses the six previously reported issues: the analyzer now collects pattern literals, fails closed on unknown character sets, recognizes variable-length alternatives and chained repetitions, and receives Slides flags; public SSR now reports over-limit values instead of silently skipping them. Those prior review threads were verified fixed and resolved.
New review passes still find remaining ReDoS bypasses in the expanded heuristic. This remains high risk because these patterns can execute synchronously in Forms/Calendar validation or over unrestricted Slides content.
New Findings
- 🔴 HIGH: Finite outer repetition groups are skipped, allowing patterns such as
^(a+){10}$to reach synchronous evaluation. - 🔴 HIGH: Duplicate multi-character alternatives can still create exponential ambiguity.
- 🔴 HIGH: Slides dotAll (
s) semantics are not preserved during probing. - 🔴 HIGH: Unicode case folding and Unicode property escapes can disagree with the actual runtime regex when
uis used. - 🟡 MEDIUM: Public SSR rejects optional empty fields with unsafe patterns even though the client and server submission paths skip validation for absent values.
The new regression corpus and focused tests are valuable, and the core tri-state contract remains sound. The remaining bypasses should be closed before merge.
🧪 Browser testing: Will run after this review (PR touches UI code)
| for (const atom of branch) { | ||
| if (atom.kind !== "group") continue; | ||
| if (isUnbounded(atom)) { |
There was a problem hiding this comment.
🔴 Analyze finite outer repeated groups before executing them
walk only sends a group to analyzeRepeatedGroup when its outer quantifier is unbounded. A finite repeat can still have a combinatorial number of partitions within the 4,096-character input cap; for example, ^(a+){10}$ is currently classified safe and can take seconds on a short non-matching input. Analyze repeated groups with finite bounds when their bodies are variable or ambiguous, or fail closed for these cases.
Additional Info
Found by 2/3 review agents; independently runtime-validated.
| const ambiguous = | ||
| minLength(a) !== minLength(b) || | ||
| a.some(isVariableLength) || | ||
| b.some(isVariableLength) || | ||
| (consuming(a).length === 1 && consuming(b).length === 1); |
There was a problem hiding this comment.
🔴 Reject duplicate multi-character alternatives in repeated groups
The ambiguity check permits equal-length fixed alternatives unless both branches are single atoms. This misses duplicate multi-character branches such as ^(ab|ab)+$, which provide indistinguishable choices on every iteration and can backtrack exponentially on a non-match. Detect equal-language or overlapping multi-atom alternatives conservatively before execution.
Additional Info
Found by 1/3 review agents; independently runtime-validated.
| // Only case folding changes which characters two atoms share; the rest affect | ||
| // anchoring or iteration, and `y`/`g` would break the single-character probes. | ||
| const probeFlags = flags.includes("i") ? "i" : ""; |
There was a problem hiding this comment.
🔴 Preserve dotAll semantics during overlap analysis
Only i is retained in probeFlags, but Slides passes s through to the actual RegExp and matchAll. With dotAll, . can overlap newline-consuming alternatives that the analyzer treats as disjoint, allowing a catastrophic pattern to block the Slides request. Preserve matching-affecting flags such as s, or fail closed for unsupported flag combinations.
Additional Info
Found by 2/3 review agents; independently runtime-validated.
| } | ||
| // Only case folding changes which characters two atoms share; the rest affect | ||
| // anchoring or iteration, and `y`/`g` would break the single-character probes. | ||
| const probeFlags = flags.includes("i") ? "i" : ""; |
There was a problem hiding this comment.
🔴 Preserve Unicode case-folding semantics during analysis
The analyzer drops u from probe flags even when the executed Slides regex uses iu. Unicode case folding can make alternatives overlap only under those flags (for example ^(ſ|s)+Z$ with iu), so the source can pass analysis while the runtime backtracks catastrophically. Preserve u with i or reject flag combinations whose matching semantics are not modeled.
Additional Info
Found by 1/3 review agents; independently runtime-validated.
| } else if (ch === "\\") { | ||
| source = state.source.slice(state.index, state.index + 2); | ||
| state.index += 2; | ||
| kind = /^\\\d$/.test(source) ? "backref" : "escape"; |
There was a problem hiding this comment.
🔴 Parse or reject Unicode property escapes
The parser treats every escape as two characters, so under u, \p{L} is parsed as \p followed by literal {L} rather than a Unicode property class. A pattern such as ^(\p{L}+)+$ can therefore be classified safe while the actual JavaScript regex has nested-overlap backtracking. Parse multi-character property escapes correctly or fail closed whenever unsupported escape syntax is present.
Additional Info
Found by 1/3 review agents; independently runtime-validated.
| return (f.validation.message || f.label + " must be at most " + f.validation.max); | ||
| if (f.validation.pattern && typeof v === "string" && !new RegExp(f.validation.pattern).test(v)) | ||
| return (f.validation.message || f.label + " is invalid"); | ||
| if (f.validation.unsafePattern) |
There was a problem hiding this comment.
🟡 Do not reject optional empty fields solely for an unsafe pattern
The inline SSR runtime checks unsafePattern before determining whether the field has a submitted value. For an optional untouched text field, the SSR page therefore returns an uncheckable-pattern error, while the React client and server submission validator skip pattern checks for absent values. Move this branch inside the present-string validation path or make all surfaces consistently handle optional empty fields.
Additional Info
Found by 2/3 review agents; independently validated against the existing client/server behavior.
|
There was a problem with your request, please try again later. Error id: |
`pnpm fmt:check` fails on main for this changelog file, which blocks every PR that merges it. Whitespace only.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…da0a544 # Conflicts: # scripts/guard-i18n-changed-copy.test.ts # scripts/guard-i18n-changed-copy.ts
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🟡
Review Details
Incremental Code Review Summary
The latest head still contains the six previously reported issues, which remain open and were intentionally not reposted. New review found two medium-severity regressions. Slides now analyzes agent-supplied regex-replace patterns directly, bypassing the shared 512-character source limit; the analyzer itself can therefore become expensive before slide matching begins. Separately, applying the new safety verdict inside assertValidFields also affects legacy persisted Forms rows during submission/upload configuration parsing, so old forms with unsafe patterns can fail with a generic 500 before the intended unevaluated response is reached.
The core approach and focused regression suites remain useful, but these new integration boundaries should be fixed before merge. A low-severity test-helper concern was also identified: the synchronous elapsed-time helper cannot interrupt a truly hanging regex; it is included for tracking but may be filtered by the review service.
Risk remains high because these paths process agent- or user-authored expressions synchronously.
🧪 Browser testing: Will run after this review (PR touches UI code)
| // Name the mistake so the agent rewrites the pattern instead of retrying it. | ||
| // The flags are part of the verdict: `^(a|A)+$` is unambiguous on its own and | ||
| // catastrophic under `i`. | ||
| const verdict = analyzeRegexSource(edit.pattern, flags); |
There was a problem hiding this comment.
🟡 Bound regex-replace source before running the analyzer
regex-replace accepts an unbounded pattern and calls analyzeRegexSource directly, bypassing the 512-character limit enforced by compileUserRegex. A large syntactically valid pattern can make probe collection and adjacent-run analysis quadratic before slide content is even inspected, stalling the Slides request. Enforce the shared source bound before analysis, or reuse a bounded compiler while preserving the requested flags.
Additional Info
Found by 1/2 review agents; independently measured on the current implementation.
| // — what an LLM reaches for to mean "at least two words" — backtracks | ||
| // exponentially and freezes both the respondent's tab and the submit | ||
| // handler's event loop. Reject it here so it never reaches the column. | ||
| const compiled = compileUserRegex(v.pattern); |
There was a problem hiding this comment.
🟡 Allow legacy unsafe patterns to reach execution-time validation
The new safety check in assertValidFields also runs when public submission/upload handlers parse persisted form fields. A form saved before this gate with the reported unsafe pattern now fails configuration parsing with a generic 500 before validateSubmissionField can return its intended explicit unevaluated error. Keep authoring-time rejection separate from legacy structural parsing so existing respondents receive a controlled validation response instead of every submission failing as invalid form configuration.
Additional Info
Found by 1/2 review agents; verified against the legacy execution path.
2 participants
Summary
Adds a shared
bounded-regexutility that detects and refuses regex patterns shaped for catastrophic (super-linear) backtracking, and wires it into every place Forms/Calendar/Slides evaluates a user- or agent-authored regex, fixing a tab-freezing hang.Factory item:
49878fbab5aae6e665ddf4f2ed8cf959d016fe863d9a039a3f088085da7e5353Source Slack thread: https://slack.com/app_redirect?team=T0GCV21GE&channel=C0ATH3CCZT4&message_ts=1789450650.422179
Problem
In the Forms editor, an agent asked to make "Full Name" require at least two words wrote the pattern
^([A-Za-z]+\s?)+$. That pattern is short, compiles cleanly, but backtracks exponentially: a 26-character non-matching value already costs ~750ms, doubling with every additional character. Because the same pattern was applied vianew RegExp(source).test(value)with no timeout mechanism available in JavaScript, this froze the Forms editor tab (CPU spike, Chrome "Page Unresponsive" dialog) and, since the same pattern was re-checked on submit, could also peg the server's event loop. Existing mitigations (capping pattern length to 200 chars, capping input to 1000 chars) did not help — the blowup is reached well inside those caps.Solution
Introduced
analyzeRegexSource,compileUserRegex, andtestUserRegexin@agent-native/core/shared.analyzeRegexSourceparses the pattern's structure and recognizes known ambiguity signatures that cause super-linear backtracking (nested/adjacent overlapping repetition, nullable parts under an unbounded repeat, overlapping single-atom alternatives) and refuses to run those patterns. Safe patterns are still evaluated against a capped input length. Results are tri-state (match/no-match/unevaluated) so a refused-to-run pattern can never be silently read as "validation passed" or "value failed."Key Changes
packages/core/src/shared/bounded-regex.ts: exportsanalyzeRegexSource,compileUserRegex,testUserRegex,MAX_USER_REGEX_LENGTH,MAX_USER_REGEX_INPUT_LENGTH, plus a hand-rolled regex-shape parser/analyzer and spec coverage (bounded-regex.spec.ts).templates/forms/shared/field-schema.ts,templates/forms/server/lib/validate-fields.ts):assertValidFieldsnow rejects unsafe patterns at save time viacompileUserRegex, with an error message suggesting a safe rewrite (e.g.^\S+(\s+\S+)+$for "at least two words").templates/forms/server/lib/submission-validation.ts,templates/forms/app/pages/FormFillPage.tsx): usetestUserRegexinstead of rawnew RegExp(...).test(...), handling theunevaluatedcase explicitly rather than silently accepting/rejecting.templates/forms/server/lib/public-form-ssr.ts): newpublicValidationhelper strips an unsafepatternbefore shipping field validation to the anonymous respondent's browser, replacing it with anunsafePatternmarker so the inline runtime can show a "can't be checked" message instead of running the pattern.templates/calendar/app/components/booking/BookingForm.tsx,templates/calendar/server/handlers/bookings.ts): replaced ad-hoc length caps and try/catchRegExpusage withtestUserRegex, distinguishing "no match" from "uncheckable" on both client and server.templates/slides/server/lib/slide-content-patch.ts):applyRegexReplacenow callsanalyzeRegexSourcebefore runningmatchAll, throwing a descriptive error so the agent rewrites the pattern instead of hanging on it.fieldPatternUncheckable(Calendar) anduncheckablePattern(Forms) strings across all locales.templates/calendar/server/lib/booking-custom-field-pattern.spec.tsandtemplates/forms/server/lib/validation-pattern-redos.spec.ts, both time-bounded so a reintroduced catastrophic pattern fails fast instead of hanging the suite.scripts/guard-i18n-changed-copy.tsnow also recognizes theexport default messagesByLocale[...]wrapper shape (used by calendar/brain templates) as forwarding an inline locale update, with accompanying tests.bounded-user-regex.mddocumenting the new@agent-native/coreshared utilities.To clone this PR locally use the Github CLI with command
gh pr checkout 5037You can tag me at @BuilderIO for anything you want me to fix or change