You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
SSO-2658: shouldRender (packages/forms/src/UIForm/utils/condition.js) evaluates UI-schema condition rules served by the backend with jsonLogic.apply and no cost limit. isSafeCondition only checks operator names, so a ~150 byte rule (a reduce whose accumulator is merged/cated with itself, or map nested ~9 levels deep) costs exponential time/memory on the main thread and freezes or crashes the tab of everyone opening the form.
What is the chosen solution to this problem?
Validate the condition structure before evaluating it (isBoundedCondition): cap node count (500) and depth (20), allow at most 2 nested iteration operators (map, filter, reduce, all, none, some), and forbid merge, cat and arrays of non-literal values inside a reduce logic (they grow the accumulator exponentially). Rejected conditions make the item not rendered, like unsafe conditions already do. Existing conditions keep working; new tests cover the exploit rules (they crash with OOM / RangeError without the fix) and bounded iteration cases.
Not addressed here: memoizing shouldRender across Widget/Fieldset renders (second recommendation of the finding).
Please check if the PR fulfills these requirements
The PR have used pnpm changeset to a request a release from the CI if wanted.
Root cause: shouldRender ran UI-schema conditions through jsonLogic.apply with
only an operator allow-list check. A ~150 byte rule (a reduce whose accumulator
is merged/cat with itself, or map nested ~9 levels) costs exponential CPU and
memory on the main thread, freezing or crashing the tab of whoever opens a form
served with such a schema.
Fix: validate the condition structure before evaluating it. Nodes count and depth
are capped, iteration operators (map, filter, reduce, all, none, some) can be
nested at most twice, and a reduce logic cannot grow its accumulator (merge, cat,
arrays of non-literal values). Rejected conditions make the item not rendered.
Recurrence is prevented by the tests reproducing the doubling reduce (merge, cat,
nested arrays) and deeply nested map rules, which crash/hang without the check.
This PR adds structural limits to backend-provided UI-schema conditions in @talend/react-forms, aiming to prevent expensive JSON Logic evaluation from freezing the browser.
Changes:
Caps condition size, depth, and iteration nesting, and rejects selected accumulator-growing expressions inside reducers.
Adds regression tests for expensive rules and compatibility tests for bounded iteration.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The reducer guard still allows accumulator-wide in scans that pass both validators and cause quadratic work during rendering, leaving a critical tab-freeze path.
Coercive comparisons still bypass the reducer scan guard: with input and initial accumulator both { var: 'items' }, the body { or: [{ '==': [{ var: 'accumulator' }, -1] }, { var: 'accumulator' }] } passes both validators. For zero-filled items, the comparison stringifies the entire array and or preserves it on every step, causing quadratic synchronous work and retaining a tab-freeze path; < permits the same bypass. Bound runtime operand sizes or prevent array-wide coercion in reducer operations, and add regression coverage for both comparison forms.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Standard var argument lists with literal paths and defaults are incorrectly rejected as computed paths, hiding widgets for previously valid conditions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GitHub Actions
Failure
What is the problem this PR is trying to solve?
SSO-2658:
shouldRender(packages/forms/src/UIForm/utils/condition.js) evaluates UI-schemaconditionrules served by the backend withjsonLogic.applyand no cost limit.isSafeConditiononly checks operator names, so a ~150 byte rule (areducewhose accumulator ismerged/cated with itself, ormapnested ~9 levels deep) costs exponential time/memory on the main thread and freezes or crashes the tab of everyone opening the form.What is the chosen solution to this problem?
Validate the condition structure before evaluating it (
isBoundedCondition): cap node count (500) and depth (20), allow at most 2 nested iteration operators (map,filter,reduce,all,none,some), and forbidmerge,catand arrays of non-literal values inside areducelogic (they grow the accumulator exponentially). Rejected conditions make the item not rendered, like unsafe conditions already do. Existing conditions keep working; new tests cover the exploit rules (they crash with OOM / RangeError without the fix) and bounded iteration cases.Not addressed here: memoizing
shouldRenderacrossWidget/Fieldsetrenders (second recommendation of the finding).Please check if the PR fulfills these requirements
pnpm changesetto a request a release from the CI if wanted.[ ] This PR introduces a breaking change