Skip to content

Conversation

Moumouls
Copy link
Member

@Moumouls Moumouls commented Oct 10, 2025

Pull Request

Issue

Closes: GHSA-9g8m-v378-pcg3

Approach

Use Object create null

Tasks

  • Add tests
  • Add changes to documentation (guides, repository pages, code comments)

Summary by CodeRabbit

  • New Features
    • Added capability to remove and return per-object state.
  • Refactor
    • Reworked state storage to use prototype-free objects for improved safety and reliability.
    • Adjusted internal logic to align with the new state structures.
  • Tests
    • Expanded test coverage for edge cases involving unusual class names and IDs.
  • Style
    • Standardized import and string literal quotes for consistency.

Copy link

🚀 Thanks for opening this pull request!

@parseplatformorg
Copy link
Contributor

parseplatformorg commented Oct 10, 2025

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

Copy link

coderabbitai bot commented Oct 10, 2025

📝 Walkthrough

Walkthrough

Prototype-safe state storage was introduced by replacing {} with Object.create(null) in SingleInstanceStateController and adding a new exported removeState(obj) API. Related tests for prototype pollution scenarios were added. Two other files received quote/style-only changes without behavior modifications.

Changes

Cohort / File(s) What changed
Prototype-safe state storage & API
src/SingleInstanceStateController.ts
Switched all state containers/maps to prototype-free objects via Object.create(null); adjusted init/clear paths accordingly; added exported removeState(obj): State | null; minor formatting and import-quote updates.
Prototype pollution tests
src/__tests__/SingleInstanceStateController-test.js
Added tests covering pollution attempts via className/id like "proto", "constructor", "prototype"; validated safe initialization, storage, retrieval, and cleanup behaviors.
Formatting only
src/LocalDatastoreController.default.ts, src/ObjectStateMutations.ts
Quote/style normalization (single→double quotes), minor reflow; no behavioral changes.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller
  participant StateCtrl as SingleInstanceStateController
  participant Store as objectState (prototype-free)

  rect rgb(235, 245, 255)
  note right of Store: All maps use Object.create(null)
  Caller->>StateCtrl: getState(obj)
  StateCtrl->>Store: ensure className/id containers
  Store-->>StateCtrl: State (created if missing)
  StateCtrl-->>Caller: State
  end

  rect rgb(240, 250, 240)
  Caller->>StateCtrl: removeState(obj)
  StateCtrl->>Store: delete state for className/id
  Store-->>StateCtrl: Removed State or null
  StateCtrl-->>Caller: State \| null
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.14% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title Check ❓ Inconclusive The title only references the security advisory identifier without describing the actual prototype pollution fix or the use of Object.create(null), making it too generic to convey the primary change to reviewers. Please update the title to clearly summarize the main change, for example “Prevent prototype pollution by using Object.create(null) in state controllers” or similar.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed The description follows the repository template by including the Pull Request header, the linked advisory in the Issue section, a concise Approach, and completed Tasks, satisfying the required sections and structure.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4a47f65 and 8cbe1fb.

📒 Files selected for processing (4)
  • src/LocalDatastoreController.default.ts (2 hunks)
  • src/ObjectStateMutations.ts (7 hunks)
  • src/SingleInstanceStateController.ts (4 hunks)
  • src/__tests__/SingleInstanceStateController-test.js (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/ObjectStateMutations.ts (3)
src/SingleInstanceStateController.ts (1)
  • setServerData (57-60)
src/UniqueInstanceStateController.ts (1)
  • setServerData (51-54)
src/ParseOp.ts (1)
  • RelationOp (300-453)
src/SingleInstanceStateController.ts (2)
src/ObjectStateMutations.ts (1)
  • State (14-20)
src/UniqueInstanceStateController.ts (3)
  • enqueueTask (109-112)
  • initializeState (15-32)
  • clearAllState (131-133)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: build (Node 18, 18.20.4)
  • GitHub Check: build (Node 22, 22.4.1)
  • GitHub Check: build (Node 20, 20.15.1)
🔇 Additional comments (8)
src/LocalDatastoreController.default.ts (1)

1-2: LGTM! Formatting-only changes.

The quote style updates (single to double quotes) are purely stylistic and have no functional impact. These changes align with the broader formatting consistency effort across the PR.

Also applies to: 32-32

src/ObjectStateMutations.ts (1)

1-8: LGTM! Formatting-only changes.

All modifications are stylistic: import quotes standardized to double quotes, and minor reformatting of function signatures and conditionals. No behavioral changes.

Also applies to: 32-43, 92-104, 120-158, 171-189, 202-211

src/__tests__/SingleInstanceStateController-test.js (2)

13-14: Minor formatting adjustment.

Trivial whitespace change in mockObject definition with no functional impact.


702-789: Excellent test coverage for prototype pollution protection.

The new test suite comprehensively validates the CVE-2025-57324 fix by covering:

  • Dangerous className values (__proto__, constructor, prototype)
  • Dangerous id values (__proto__)
  • Normal data storage/retrieval with dangerous names
  • Verification that pollution doesn't occur
  • Cleanup scenarios

The assertions correctly verify that {}.pollutedProperty and Object.prototype.pollutedProperty remain undefined, confirming no prototype chain pollution.

src/SingleInstanceStateController.ts (4)

1-10: LGTM! Import formatting standardization.

Import quotes standardized to double quotes, consistent with the broader PR formatting effort. No functional changes.


40-47: Well-implemented new API for per-object state removal.

The removeState function:

  • Provides a clean API for removing individual object state (vs. clearAllState)
  • Properly handles non-existent state by returning null
  • Returns the removed state, enabling cleanup verification
  • Is properly tested in the test suite (lines 638-678, 780-788)

The implementation correctly deletes from the nested map structure and maintains consistency with getState.


101-106: LGTM! Formatting improvements for readability.

Multiline formatting of function call arguments improves readability without changing behavior.

Also applies to: 117-121, 124-127


12-14: Prototype pollution fixes complete SingleInstanceStateController now uses Object.create(null) at init and reset; UniqueInstanceStateController already uses a WeakMap and is immune to prototype-pollution. No further changes required.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

codecov bot commented Oct 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (4a47f65) to head (8cbe1fb).

Additional details and impacted files
@@            Coverage Diff            @@
##             alpha     #2745   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           63        63           
  Lines         6185      6185           
  Branches      1456      1456           
=========================================
  Hits          6185      6185           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 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.

coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 10, 2025
@Moumouls
Copy link
Member Author

@mtrezza ready to review

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.

2 participants