Fix autoFocus for all elements and ReDoS in devtools stack parsing #465
Fix autoFocus for all elements and ReDoS in devtools stack parsing #465everettbu wants to merge 5 commits into
Conversation
…35656)
autoFocus is a global HTML attribute per the spec, but React only
handled it for button, input, select, and textarea. This change treats
autoFocus as a global attribute so it works on any focusable element
including anchor tags, div with tabIndex, dialog, details, etc.
Changes:
- finalizeInitialChildren: default case now returns !!props.autoFocus
- commitMount: default case now calls .focus() when autoFocus is set
- Added tests for <a autoFocus> and <div tabIndex={0} autoFocus>
Fix two regex patterns in parseStackTrace.js vulnerable to catastrophic backtracking: 1. firefoxFrameRegExp: replace overlapping .*".+" with non-overlapping character classes [^@"]*(?:"[^"]*"[^@"]*)* 2. CHROME_STACK_REGEXP: simplify detection regex from /^\s*at .*(\S+:\d+|\(native\))/m to /^\s*at /m to eliminate the O(n^2) backtracking between .* and \S+
Feat/dimash
Greptile OverviewGreptile SummaryThis PR does two things:
Main merge-blockers are around correctness regressions from the updated heuristics:
Confidence Score: 3/5
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it('calls focus() on autoFocus anchor elements after they have been mounted to the DOM', async () => { | ||
| const originalFocus = HTMLElement.prototype.focus; | ||
|
|
||
| try { | ||
| let focusedElement; | ||
| let anchorFocusedAfterMount = false; | ||
|
|
||
| HTMLElement.prototype.focus = function () { | ||
| focusedElement = this; | ||
| anchorFocusedAfterMount = !!this.parentNode; | ||
| }; | ||
|
|
||
| const container = document.createElement('div'); | ||
| document.body.appendChild(container); | ||
| const root = ReactDOMClient.createRoot(container); | ||
| await act(() => { | ||
| root.render( | ||
| <div> | ||
| <h1>Auto-focus Test</h1> | ||
| <a href="https://react.dev" autoFocus={true}> | ||
| Link | ||
| </a> | ||
| <p>The above anchor should be focused after mount.</p> | ||
| </div>, | ||
| ); | ||
| }); | ||
|
|
||
| expect(anchorFocusedAfterMount).toBe(true); | ||
| expect(focusedElement.tagName).toBe('A'); | ||
| } finally { |
There was a problem hiding this comment.
Prototype focus override leaks DOM
These two new autoFocus tests append a container to document.body but never remove it (unlike neighboring tests, e.g. preserves focus), so they will leak DOM across the file and can affect later assertions that depend on document.body contents or focus behavior. Wrap them in a try/finally that also document.body.removeChild(container) (and ideally root.unmount() before removal) to keep test isolation deterministic.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-dom/src/__tests__/ReactDOM-test.js
Line: 389:418
Comment:
**Prototype focus override leaks DOM**
These two new autoFocus tests append a container to `document.body` but never remove it (unlike neighboring tests, e.g. `preserves focus`), so they will leak DOM across the file and can affect later assertions that depend on `document.body` contents or focus behavior. Wrap them in a `try/finally` that also `document.body.removeChild(container)` (and ideally `root.unmount()` before removal) to keep test isolation deterministic.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
In A safer pattern is to guard on focusability, e.g. Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js
Line: 933:935
Comment:
**autoFocus can throw on SVG**
In `commitMount`'s default branch, `domElement` is cast to `HTMLElement` and `.focus()` is called when `newProps.autoFocus` is truthy. For non-HTML host instances (e.g. SVG elements), `domElement` is an `SVGElement`, and `SVGElement.prototype.focus` is not guaranteed to exist in all supported environments. That can turn `autoFocus` into a runtime `TypeError` for `<svg autoFocus>` / `<circle autoFocus>`.
A safer pattern is to guard on focusability, e.g. `if (newProps.autoFocus && typeof (domElement: any).focus === 'function') { ... }` (or restrict this behavior to HTML elements only).
How can I resolve this? If you propose a fix, please make it concise. |
Additional Comments (2)
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-devtools-shared/src/backend/utils/parseStackTrace.js
Line: 97:103
Comment:
**Over-broad Chrome stack detection**
`CHROME_STACK_REGEXP` was loosened to `/^\s*at /m` (parseStackTraceFromString at `packages/react-devtools-shared/src/backend/utils/parseStackTrace.js:97-105`). This will classify *any* stack containing a line starting with `at ` as “Chrome”, even if it isn’t in the format `chromeFrameRegExp` can parse. In that case, `parseStackTraceFromChromeStack` will skip those frames entirely (because `chromeFrameRegExp.exec(...)` returns null), and the function will return fewer/empty frames where the previous Firefox fallback would have parsed something. This is a concrete regression for non-V8/custom stacks that include `"at "` lines but aren’t Chrome-formatted.
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js
Line: 648:666
Comment:
**autoFocus on <img> no-op**
`finalizeInitialChildren` now returns `!!props.autoFocus` for the `default` case (`packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js:648-666`), but `commitMount`’s new default handler is unreachable for `type === 'img'` (`commitMount` hits `case 'img'` and returns at `:862-894`). As a result, `<img autoFocus />` schedules an Update effect (because `finalizeInitialChildren` returns `true` for all `<img>`), but will never call `.focus()` in `commitMount`, so `autoFocus` silently doesn’t work for images despite now being treated as global.
How can I resolve this? If you propose a fix, please make it concise. |
|
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
github-actions
Bot
added
the
Resolution: Stale
|
Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you! |
2 participants
Mirror of facebook/react#35693
Original author: d1maash
Summary
This PR addresses two open issues:
1. Support autoFocus as a global HTML attribute
(#35656)
autoFocusis a [global HTMLattribute](https://developer.mozilla.org/en-US/docs/W
eb/HTML/Global_attributes/autofocus) per the spec,
but React only handled it for
button,input,select, andtextarea. This change treatsautoFocusas a global attribute so it works onany focusable element — anchor tags,
<div tabIndex={0}>,<dialog>,<details>, etc.Changes:
finalizeInitialChildren: default case now returns!!props.autoFocuscommitMount: default case now calls.focus()when
autoFocusis set<a autoFocus>and<div tabIndex={0} autoFocus>2. Fix ReDoS vulnerabilities in stack trace
parsing (#35490)
Two regex patterns in
parseStackTrace.jsarevulnerable to catastrophic backtracking:
firefoxFrameRegExp:(?:.*".+")?[^@]*contains overlapping quantifiers. Replaced with
[^@"]*(?:"[^"]*"[^@"]*)*using non-overlappingcharacter classes.
CHROME_STACK_REGEXP:.*(\S+:\d+|\(native\))causes O(n²) backtracking. Simplified to
/^\s*at /m.