fix: LiveQuery protected field leak via shared mutable state across concurrent subscribers (GHSA-m983-v2ff-wq65)#10330
Conversation
|
🚀 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
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. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
📝 WalkthroughWalkthroughAdds LiveQuery tests for concurrent-subscriber scenarios around protected fields and afterLiveQueryEvent; updates ParseLiveQueryServer to deep-clone per-request payloads and to unconditionally clear protectedFields for master-key clients to prevent shared-state leakage. Changes
Sequence Diagram(s)sequenceDiagram
participant DB as Database
participant LQ as ParseLiveQueryServer
participant TR as afterLiveQueryEventTrigger
participant C1 as Client (master)
participant C2 as Client (regular)
DB->>LQ: emit change (create/update/delete)
LQ->>LQ: for each subscription requestId\ndeep-clone payload -> localObject
alt trigger configured
LQ->>TR: call afterLiveQueryEvent with localObject
TR-->>LQ: may mutate localObject (per-request)
end
LQ->>LQ: toJSONwithObjects(localObject) and _filterSensitiveData(localObject, client)
LQ->>C1: push filtered localObject (master sees protected fields)
LQ->>C2: push filtered localObject (regular does not see protected fields)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #10330 +/- ##
=======================================
Coverage 92.52% 92.52%
=======================================
Files 192 192
Lines 16517 16519 +2
Branches 227 229 +2
=======================================
+ Hits 15283 15285 +2
Misses 1214 1214
Partials 20 20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@spec/vulnerabilities.spec.js`:
- Around line 3409-3423: The race test uses the helper createSubscribedClient
and derives the "injected" marker from req.sessionToken || 'no-session', which
makes both anonymous clients produce the same marker; change
createSubscribedClient to tag each client with a unique client-specific marker
(e.g., include a generated id or suffix in the payload subscriber metadata) so
the server sees distinct values per client, and update the assertions that
currently check payloads with toBeDefined() to instead assert that the two
received payload markers are both defined and not equal (i.e., payloadA !==
payloadB), referencing createSubscribedClient, the injected variable, and the
code handling req.sessionToken to locate and modify the test and assertions.
🪄 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: f7adc915-ac84-4ea0-96a0-0e88a56d087f
📒 Files selected for processing (2)
spec/vulnerabilities.spec.jssrc/LiveQuery/ParseLiveQueryServer.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
spec/vulnerabilities.spec.js (1)
3672-3726: Cover the concurrentoriginalclone path too.This only proves per-subscriber isolation for
req.objecton acreateevent. The server change also cloneslocalOriginalParseObjectand sends it through the same trigger/filter path insrc/LiveQuery/ParseLiveQueryServer.ts:321-328andsrc/LiveQuery/ParseLiveQueryServer.ts:407-442, so a regression there would still pass. Prefer anupdate/leavecase that mutates and asserts bothobjectandoriginalper subscriber.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/vulnerabilities.spec.js` around lines 3672 - 3726, Add a test that exercises the concurrent "original" clone path by using an update (or leave) LiveQuery event where the afterLiveQueryEvent handler mutates both req.object and req.original (or localOriginalParseObject) differently per installationId; use Parse.Cloud.afterLiveQueryEvent (same as existing test) and createSubscribedClient to create two subscribers, perform an update on the object, then assert each subscriber sees both object and original mutated to their own installation-specific values (e.g., injectedObject vs injectedOriginal) to ensure per-subscriber isolation of the original clone path handled in ParseLiveQueryServer (localOriginalParseObject).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spec/vulnerabilities.spec.js`:
- Around line 3424-3425: The subscription ACK wait using client.subscribe(...)
then awaiting new Promise(resolve => sub.on('open', resolve)) can hang
indefinitely; replace that await with a bounded wait that races the 'open' event
against a short timeout (e.g., a few hundred milliseconds) and rejects or fails
deterministically on timeout; ensure you reference the subscription returned by
client.subscribe and the event listener on sub.on('open', ...), remove the
listener and close/unsubscribe the subscription on timeout to avoid leaks, and
make the test assert or throw when the timeout wins so failures surface
immediately.
---
Nitpick comments:
In `@spec/vulnerabilities.spec.js`:
- Around line 3672-3726: Add a test that exercises the concurrent "original"
clone path by using an update (or leave) LiveQuery event where the
afterLiveQueryEvent handler mutates both req.object and req.original (or
localOriginalParseObject) differently per installationId; use
Parse.Cloud.afterLiveQueryEvent (same as existing test) and
createSubscribedClient to create two subscribers, perform an update on the
object, then assert each subscriber sees both object and original mutated to
their own installation-specific values (e.g., injectedObject vs
injectedOriginal) to ensure per-subscriber isolation of the original clone path
handled in ParseLiveQueryServer (localOriginalParseObject).
🪄 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: 1ccd0e94-58a1-4ac1-933f-1c3126d68327
📒 Files selected for processing (1)
spec/vulnerabilities.spec.js
mtrezza
changed the title
# [9.7.0-alpha.9](9.7.0-alpha.8...9.7.0-alpha.9) (2026-03-27) ### Bug Fixes * LiveQuery protected field leak via shared mutable state across concurrent subscribers ([GHSA-m983-v2ff-wq65](GHSA-m983-v2ff-wq65)) ([#10330](#10330)) ([776c71c](776c71c))
|
🎉 This change has been released in version 9.7.0-alpha.9 |
2 participants
Issue
LiveQuery protected field leak via shared mutable state across concurrent subscribers (GHSA-m983-v2ff-wq65)