Skip to content

fix(react-doctor): harden rules against prototype-pollution FPs and adopt-config noise#177

Merged
aidenybai merged 5 commits into
mainfrom
fix-real-world-fps
May 8, 2026
Merged

fix(react-doctor): harden rules against prototype-pollution FPs and adopt-config noise#177
aidenybai merged 5 commits into
mainfrom
fix-real-world-fps

Conversation

@aidenybai
Copy link
Copy Markdown
Member

@aidenybai aidenybai commented May 8, 2026

Summary

Audited every OBJECT[someAstName] lookup in the rule plugin and surrounding utils — converted plain-object indexing to Map.get / Object.hasOwn so AST-derived names that shadow Object.prototype properties (constructor, toString, hasOwnProperty, …) can't fall through to the native value and false-positive. Same audit pass picked up four other shipped regressions: a shallow effect-needs-cleanup walker, an over-broad Tailwind palette regex, version-gating that silently dropped rules on R17/18 + unknown React versions, and an adoptExistingLintConfig path that emitted misleading stderr warnings on Next.js / CRA scaffolds.

Fixes

  • Prototype pollution → false positives (the original no-legacy-class-lifecycles constructor bug, reproduced in 5 sibling rules):
    • no-legacy-class-lifecycles, no-react19-deprecated-apis, no-react-dom-deprecated-apis, rn-no-deprecated-modules, no-side-tab-border, no-prevent-defaultRecord<string, X> lookups → Map.get
    • run-oxlint category/help maps + run-knip issue-type map — Object.hasOwn guards via new lookupOwnString helper / Map.get
  • effect-needs-cleanup shallow walker (~36% FP rate measured against react-grab / excalidraw / popover patterns): effectHasCleanupRelease now scans every ReturnStatement in the effect's own scope via walkInsideStatementBlocks (stops at function boundaries so cleanup nested in a callback still fires).
  • design-no-default-tailwind-palette over-matching: regex anchored to canonical Tailwind stops (50950) instead of \d{2,3}, so Radix Colors utilities (text-gray-11, bg-slate-2) aren't flagged as the Tailwind template default.
  • filterRulesByReactMajor directional gating: deprecation-warning rules now fire on every detected major (R17/18 users planning the R19 upgrade are exactly the audience that benefits). When reactMajorVersion is null (custom resolver, workspace:*, mid-clone state) we optimistically assume the latest React major and apply every rule, including prefer-newer-api ones.
  • Adopt-config stderr noise: new canOxlintExtendConfig pre-screens .eslintrc.json for bare-package extends ("next", "plugin:foo/bar") that oxlint can't resolve, dropping them silently before the parser crashes. Handles JSONC (// and /* */ comments) so real-world configs parse correctly.

Tests

20 new regression tests:

  • tests/regressions/proto-pollution-defenses.test.ts — 6 tests (RN, design, prevent-default, with negative + positive each)
  • tests/can-oxlint-extend-config.test.ts — 9 unit tests (every extends shape + JSONC line/block comments + comments inside string literals)
  • tests/regressions/react-19-migration-rules.test.ts — +5 (constructor / proto-name member access, R17/18 fail-open, null fallback)
  • tests/regressions/state-rules.test.ts — +4 (conditional cleanup, try/finally cleanup, nested-fn negative test, null version fires prefer-use-effect-event)
  • tests/regressions/react-ui-rules.test.ts — +1 (Radix Colors stops not flagged)
  • tests/diagnose.test.ts — flipped null-version assertion to match new policy

Total: 668 tests passing, typecheck/lint/format clean against latest main (oxlint 1.63.0).

Test plan

  • pnpm typecheck — clean
  • pnpm lint — 0 warnings, 0 errors
  • pnpm test — 668/668 passing
  • pnpm format — clean
  • Re-bench against ~/Developer/{ami,same,expect,react-grab} — zero stderr noise, every spot-checked diagnostic is a true positive, no precision regressions

Made with Cursor


Note

Medium Risk
Changes lint rule gating and several rule detectors, which can materially change diagnostic output (and CI outcomes) across projects. Low runtime/security risk, but moderate risk of behavior regressions in linting heuristics and config adoption paths.

Overview
Improves linting correctness and stability by replacing multiple AST-derived Record[...] lookups with Map.get()/Object.hasOwn guards to prevent Object.prototype name collisions (e.g. constructor, toString) from causing false positives or bad messages across several rules and diagnostic/category/help lookups.

Adjusts rule behavior: React-major version gating now fails open when React version is unknown (null) and keeps deprecation-warning rules enabled across detected majors; effect-needs-cleanup now scans all relevant return statements within the effect scope to avoid conditional-cleanup false positives; and design-no-default-tailwind-palette narrows its regex to canonical Tailwind palette stops to avoid flagging custom scales.

Reduces oxlint config-adoption noise by adding canOxlintExtendConfig to pre-screen .eslintrc.json (including JSONC comments) and skip extends that oxlint cannot resolve, plus adds/updates regression tests and consolidates repeated test helpers.

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

…dopt-config noise

Audited every `OBJECT[someAstName]` lookup driven by user code (AST
identifier names, JSX tag names, inline style keys, knip issue types,
oxlint rule names) and converted plain-object indexing to `Map.get` /
`Object.hasOwn` guards. The original `no-legacy-class-lifecycles`
constructor false positive (`messages["constructor"]` falling through
to `Object.prototype.constructor`) had silent siblings in
`no-react19-deprecated-apis`, `no-react-dom-deprecated-apis`,
`rn-no-deprecated-modules`, `no-side-tab-border`, `no-prevent-default`,
and the diagnostic category/help maps in `run-oxlint` / `run-knip`.

Other fixes in the same audit pass:

- `effect-needs-cleanup` now scans every `ReturnStatement` in the
  effect's own scope (via `walkInsideStatementBlocks`) instead of only
  the top-level last statement. The shallow check false-positived on
  the very common conditional-cleanup shape (`if (enabled) { ...
  return () => unsub(); }`), measured at ~36% FPs across react-grab /
  excalidraw / popover patterns. The walker stops at function
  boundaries so cleanup nested inside a nested callback still fires.
- `design-no-default-tailwind-palette` regex anchored to canonical
  Tailwind stops (`50` … `950`) instead of `\d{2,3}`, so Radix Colors
  utilities (`text-gray-11`, `bg-slate-2`) are not flagged as
  "Tailwind template default."
- `filterRulesByReactMajor`: `deprecation-warning` rules now fire on
  every detected major (R17/18 users planning the R19 upgrade are
  exactly the audience that benefits). When `reactMajorVersion` is
  null (custom resolver, `workspace:*`, mid-clone state) we
  optimistically assume the latest React major and apply every rule,
  including `prefer-newer-api` ones.
- `runOxlint` pre-screens `.eslintrc.json` files via
  `canOxlintExtendConfig` before forwarding them to oxlint's `extends`.
  Configs whose `extends` is only bare-package refs (`"next"`,
  `"plugin:foo/bar"`) cannot be resolved and previously emitted a
  misleading "could not adopt existing lint config" stderr warning.
  The pre-screen also handles JSONC (`//` and `/* */` comments) so
  real-world Next.js / CRA / TypeScript scaffolds parse correctly.

Adds 20 new regression tests (6 prototype-pollution defenses, 9
extends pre-screen / JSONC, 5 R19 + cleanup behaviors). Validated
against ami / same / expect / react-grab: zero stderr noise on
baseline + head, every spot-checked diagnostic is a true positive.

Co-authored-by: Cursor <cursoragent@cursor.com>
@vercel
Copy link
Copy Markdown

vercel Bot commented May 8, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
react-doctor-website Ready Ready Preview, Comment May 8, 2026 0:40am

`JSON.parse("null")` returns a literal `null` and `parsed.extends`
threw a TypeError out of the pre-screen instead of returning safely.
Same hazard for arrays and primitives (`true`, `42`, `"a"`) which are
all valid top-level JSON values. Added an `isPlainObject` type guard
before the property access; added a regression test.

Co-authored-by: Cursor <cursoragent@cursor.com>
…rs into shared _helpers.ts

Each regression suite had its own near-identical `collectRuleHits` —
same `runOxlint(...)` call, same diagnostic filter/map. Five copies
across `proto-pollution-defenses`, `state-rules`, `react-ui-rules`,
`react-19-migration-rules`, and `prop-stack-barrier`, drifting in
small ways (default reactMajorVersion, framework parameter,
hasReactCompiler).

Replaced with a single options-bag-style helper in
`tests/regressions/_helpers.ts`:

  collectRuleHits(dir, "rule-id", { reactMajorVersion: 18 })

Defaults match the most common shape (React 19, framework "unknown")
so the typical call stays a 2-arg `(dir, "rule-id")`. Net `-45` lines.
All 669 tests pass unchanged.

Co-authored-by: Cursor <cursoragent@cursor.com>
Copy link
Copy Markdown

@cursor cursor Bot left a comment

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 and found 1 potential issue.

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 6a715b3. Configure here.

Comment thread packages/react-doctor/src/utils/can-oxlint-extend-config.ts Outdated
…p review

Spot-checks during the post-DRY review caught three more sites the
first pass missed:

1. `validateRuleRegistration` in `run-oxlint.ts` used `key in MAP` to
   detect missing rule metadata. A rule literally named `constructor`
   would be silently treated as registered. `Object.hasOwn` instead.

2. `tryDisableFailedPlugin` in `run-knip.ts` used `failedPlugin in
   parsedConfig` for the same reason. Same `Object.hasOwn` swap. Both
   are dev-time / very-narrow paths but the inconsistency itself was
   the smell.

3. The shared `collectRuleHits` helper added in the previous commit
   collapsed `{ reactMajorVersion: null }` to `19` via `??`. The null
   path is exactly what the unresolvable-version regression tests
   want to exercise, so the silent coercion would have made those
   tests test the wrong code path. Switch to an
   `Object.hasOwn(options, "reactMajorVersion")` gate so explicit
   `null` round-trips intact while omission still defaults to 19.

Co-authored-by: Cursor <cursoragent@cursor.com>
…nfig

Bugbot caught the duplication: the local `isPlainObject` in
`can-oxlint-extend-config.ts` was a less-strict copy of the existing
`utils/is-plain-object.ts` (the shared one also rejects objects with
non-`Object.prototype` prototypes — which is fine for `JSON.parse`
output but keeps the guard consistent with the rest of the codebase).
Drop the local copy and import the shared helper.

Co-authored-by: Cursor <cursoragent@cursor.com>
@aidenybai aidenybai merged commit 01c38a7 into main May 8, 2026
6 checks passed
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.

1 participant