Skip to content

fix: ParseServerRESTController passes context by reference causing cross-request mutation#10291

Open
yog27ray wants to merge 3 commits intoparse-community:alphafrom
yog27ray:fix/direct-access-context-deep-copy
Open

fix: ParseServerRESTController passes context by reference causing cross-request mutation#10291
yog27ray wants to merge 3 commits intoparse-community:alphafrom
yog27ray:fix/direct-access-context-deep-copy

Conversation

@yog27ray
Copy link
Copy Markdown
Contributor

@yog27ray yog27ray commented Mar 23, 2026

Issue

When directAccess is enabled, ParseServerRESTController bypasses the HTTP layer and routes requests internally. Unlike the HTTP path — where request context is serialized to JSON and deserialized on the server side (naturally creating a fresh copy) — the direct access path passes options.context by reference.

This means the same context object instance is shared across all requests within the same session. When a beforeSave or afterSave hook mutates req.context (which is a common and documented pattern for passing data between hooks), those mutations leak back into the caller's original context object and into any subsequent requests that reuse the same context.

Reproduction

  1. Enable directAccess: true in Parse Server config
  2. Create a shared context object: const ctx = { counter: 0 }
  3. Pass it to multiple sequential or concurrent save operations via { context: ctx }
  4. In a beforeSave hook, mutate req.context (e.g., req.context.counter++)
  5. Observed: The original ctx object is mutated; subsequent requests see the mutated state
  6. Expected: Each request should receive its own isolated copy of the context, matching HTTP behavior

Impact

  • Context mutations in hooks bleed across unrelated requests
  • Sequential saves with the same context accumulate state unexpectedly
  • Concurrent requests can race on shared context, causing non-deterministic behavior
  • Behavior diverges from the HTTP code path, making directAccess a non-transparent optimization

Approach

Use structuredClone() to deep copy options.context before attaching it to the internal request object in ParseServerRESTController. This ensures each request gets its own isolated context — matching the behavior of the HTTP code path where JSON serialization/deserialization naturally creates independent copies.

Change

In src/ParseServerRESTController.js, line 123:

-            context: options.context || {},
+            context: structuredClone(options.context || {}),

Tasks

  • Add tests
  • Add changes to documentation (guides, repository pages, code comments)
  • Add security check
  • Add new Parse Error codes to Parse JS SDK

Summary by CodeRabbit

  • Bug Fixes

    • Prevented shared context objects from being mutated during request processing, ensuring isolation for sequential and concurrent REST API calls.
    • Requests now fail fast with a DataCloneError when provided context contains non-cloneable values.
  • Tests

    • Added tests validating deep-copying of context, concurrent request isolation, and failure on non-cloneable context.

…on leak across requests

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@parse-github-assistant
Copy link
Copy Markdown

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant bot changed the title fix: deep copy context in ParseServerRESTController to prevent mutation leak across requests fix: Deep copy context in ParseServerRESTController to prevent mutation leak across requests Mar 23, 2026
@parse-github-assistant
Copy link
Copy Markdown

parse-github-assistant bot commented Mar 23, 2026

🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review.

Tip

  • Keep pull requests small. Large PRs will be rejected. Break complex features into smaller, incremental PRs.
  • Use Test Driven Development. Write failing tests before implementing functionality. Ensure tests pass.
  • Group code into logical blocks. Add a short comment before each block to explain its purpose.
  • We offer conceptual guidance. Coding is up to you. PRs must be merge-ready for human review.
  • Our review focuses on concept, not quality. PRs with code issues will be rejected. Use an AI agent.
  • Human review time is precious. Avoid review ping-pong. Inspect and test your AI-generated code.

Note

Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect.

Caution

Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement.

@parseplatformorg
Copy link
Copy Markdown
Contributor

parseplatformorg commented Mar 23, 2026

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

Status Scan Engine 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.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 23, 2026

📝 Walkthrough

Walkthrough

Deep-clone the incoming REST request context before routing so cloud hooks receive isolated copies; add tests validating isolation across sequential and concurrent requests and that non-cloneable contexts produce a DataCloneError.

Changes

Cohort / File(s) Summary
REST Controller
src/ParseServerRESTController.js
Now deep-clones options.context via structuredClone() into a local requestContext before building the request. Cloning is wrapped in try/catch and rejects the request if cloning fails (propagates an error like DataCloneError).
Context Isolation Tests
spec/ParseServerRESTController.spec.js
Added three tests: (1) verifies deep-copy behavior so in-hook mutations (including nested changes and new fields) do not mutate the original shared context; (2) verifies concurrent requests using the same shared context observe isolated req.context states and the original context remains unchanged; (3) verifies requests fail with a DataCloneError when context contains non-cloneable values (e.g., a function).

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant REST as ParseServerRESTController
    participant Router
    participant Hook as CloudHook

    Client->>REST: POST /... with options.context (sharedObj)
    REST->>REST: try structuredClone(sharedObj) -> requestContext
    alt clone succeeds
        REST->>Router: tryRouteRequest({ ..., context: requestContext })
        Router->>Hook: invoke beforeSave(req.context)
        Hook-->>Router: returns (may mutate req.context)
        Router-->>REST: response
        REST-->>Client: 200 OK (original sharedObj unchanged)
    else clone fails
        REST-->>Client: reject Promise with DataCloneError
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description check ✅ Passed The description includes all essential sections from the template with comprehensive content: Issue (with detailed problem description and reproduction steps), Approach (with rationale and code change), and Tasks (with completion status marked).
Title check ✅ Passed The title accurately describes the main change: deep-cloning context in ParseServerRESTController to prevent cross-request mutation leaks.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.53%. Comparing base (7c8b213) to head (319e9d2).
⚠️ Report is 9 commits behind head on alpha.

Additional details and impacted files
@@           Coverage Diff           @@
##            alpha   #10291   +/-   ##
=======================================
  Coverage   92.53%   92.53%           
=======================================
  Files         192      192           
  Lines       16500    16504    +4     
  Branches      227      227           
=======================================
+ Hits        15268    15272    +4     
  Misses       1212     1212           
  Partials       20       20           

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

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/ParseServerRESTController.js`:
- Line 123: The structuredClone(options.context || {}) call can throw
synchronously; wrap that call in a try/catch inside the surrounding promise
callback and propagate errors to the outer rejection path instead of allowing a
sync throw to escape. Locate the structuredClone usage (referenced as
structuredClone and options.context) and change it to perform the clone inside a
try { const ctx = structuredClone(options.context || {}); use ctx } catch (err)
{ return reject(err); } (or call the appropriate error callback/next) so any
clone errors are explicitly rejected/handled.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 16bf7b4a-3327-4532-8474-f32a6b334672

📥 Commits

Reviewing files that changed from the base of the PR and between 61ff140 and 793e36d.

📒 Files selected for processing (2)
  • spec/ParseServerRESTController.spec.js
  • src/ParseServerRESTController.js

…t values

Moves structuredClone() before the async chain and wraps it in
try/catch so non-cloneable values properly reject the promise
instead of causing an unhandled rejection.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@yog27ray yog27ray changed the title fix: Deep copy context in ParseServerRESTController to prevent mutation leak across requests fix: ParseServerRESTController passes context by reference causing cross-request mutation Mar 24, 2026
@yog27ray
Copy link
Copy Markdown
Contributor Author

@mtrezza is there any further changes required in this PR?

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.

3 participants