fix: Denial-of-service via unbounded query complexity in REST and GraphQL API (GHSA-cmj3-wx7h-ffvg)#10130
Merged
mtrezza merged 12 commits intoparse-community:alphafrom Mar 7, 2026
Conversation
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
|
🚀 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. 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. |
Contributor
✅ 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds requestComplexity options, validation, and enforcement for GraphQL (depth/fields) and REST (include/subquery depth/count); integrates a GraphQL complexity validation plugin, updates option definitions/docs/config, adds a server config check, and adds tests covering these behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as GraphQL Client
participant Server as ParseGraphQLServer
participant Plugin as Complexity Plugin
participant Validator as calculateQueryComplexity
participant Executor as Apollo Executor
Client->>Server: POST /graphql (query)
Server->>Plugin: requestDidStart / didResolveOperation
Plugin->>Plugin: Check auth (master/maintenance)
alt Master or Maintenance Key
Plugin-->>Server: Bypass validation
else Standard Auth
Plugin->>Validator: calculateQueryComplexity(operation, fragments)
Validator->>Validator: Traverse selectionSet and fragments
Validator-->>Plugin: { depth, fields }
Plugin->>Plugin: Compare vs config limits
alt Exceeds Limit
Plugin->>Client: Throw GraphQLError (400)
else Within Limit
Plugin->>Executor: Allow execution
Executor-->>Client: Query result
end
end
sequenceDiagram
participant Client as REST Client
participant RestQuery as RestQuery.execute()
participant IncludeVal as validateIncludeComplexity
participant SubqueryVal as checkSubqueryDepth
participant Storage as Database/Subquery Execution
Client->>RestQuery: REST request (includes / where with subqueries)
RestQuery->>RestQuery: handleIncludeAll
RestQuery->>IncludeVal: validateIncludeComplexity(context)
IncludeVal->>IncludeVal: Check auth and count/depth vs config
alt Exceeds Include Limits
IncludeVal->>Client: Reject with error (400)
else Within Limits
IncludeVal-->>RestQuery: Continue
end
RestQuery->>SubqueryVal: checkSubqueryDepth(context)
SubqueryVal->>SubqueryVal: Check auth and depth
alt Exceeds Subquery Depth
SubqueryVal->>Client: Reject with error (400)
else Within Limits
SubqueryVal-->>RestQuery: Create childContext(depth+1)
RestQuery->>Storage: Execute subquery with childContext
Storage-->>RestQuery: Subquery results
RestQuery-->>Client: Response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
spec/GraphQLQueryComplexity.spec.js (1)
42-55:buildDeepQueryhelper is defined but never used.The
buildDeepQueryfunction is defined to create deeply nested queries with synthetic fields (f0,f1, etc.), but none of the tests actually call it. The depth tests use inline query strings instead. Consider either using this helper in the depth tests or removing it to avoid dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/GraphQLQueryComplexity.spec.js` around lines 42 - 55, The helper function buildDeepQuery is dead code; either remove it or make the depth tests call it instead of using inline query strings. Edit the spec/GraphQLQueryComplexity.spec.js tests that assert complexity for various depths to call buildDeepQuery(depth) (using the same numeric depth values currently inlined) so the tests reuse buildDeepQuery, or delete the buildDeepQuery function if you prefer to keep the inline literals; update any references to f0/f1-style synthetic fields accordingly.
🤖 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/RequestComplexity.spec.js`:
- Around line 107-129: The test is brittle because reconfigureServer() may reuse
shared test config from prior specs; ensure the config is reset before asserting
defaults by clearing or resetting the test configuration prior to calling
reconfigureServer({}) — e.g., call Config.reset('test') or a provided cleanup
helper (or add a beforeEach that resets Config and/or stops/clears the server)
so that reconfigureServer and Config.get('test') return true defaults; update
the spec to invoke that reset (or add the reset in beforeEach) before the
"should apply full defaults when not configured" assertion.
In `@spec/RestQuery.spec.js`:
- Line 393: The test sets a global server config by calling reconfigureServer({
requestComplexity: { includeCount: 200 } }) which can leak into later specs;
wrap this change so it's reverted in teardown by capturing the previous
requestComplexity (via reconfigureServer or reading current config) and
restoring it in an afterEach / finally block, or replace the direct call with a
scoped helper that temporarily applies the override for this spec only;
reference reconfigureServer, requestComplexity, includeCount, and ensure
restoration runs even on failures (use afterEach or a try/finally).
In `@src/GraphQL/helpers/queryComplexity.js`:
- Around line 33-37: calculateQueryComplexity is walking every
OperationDefinition in requestContext.document which wrongly scores operations
that weren't selected; change the logic to only score the resolved operation
from requestContext.didResolveOperation (use didResolveOperation.operationName
or the resolvedOperation node) and locate the matching OperationDefinition in
requestContext.document (or use the resolved operation directly) and call
visitSelectionSet on that operation's selectionSet only; apply the same fix to
the other loop that iterates definitions (the block around the second
occurrence) so unused operations are ignored.
- Line 8: Several single-line if statements lack braces (e.g., "if
(!selectionSet) return;" and the other brace-less ifs at the locations flagged)
which fails linting; update each of those statements in
src/GraphQL/helpers/queryComplexity.js to use block braces (for example: if
(!selectionSet) { return; }) so the if bodies are wrapped in {}—ensure this
change is applied to the instances around the selectionSet check and the other
two flagged conditions at the same file (lines flagged: 8, 23, 52, 55) so lint
passes.
- Around line 21-28: The bug is that visitedFragments is shared across sibling
branches so repeated uses of the same fragment in different siblings are
skipped; change the recursion to track visited fragments per traversal path
instead of globally by cloning the Set when descending into a sibling branch or
when recursing from a FragmentSpread. Concretely, inside visitSelectionSet (and
where you handle selection.kind === 'FragmentSpread') stop mutating a single
global visitedFragments; create a new Set(visitedFragments) and pass that cloned
set into the recursive visitSelectionSet calls (while still marking the fragment
in the cloned set to avoid infinite recursion). This preserves loop protection
for recursion but allows the same fragment to be counted separately when reused
in different branches.
In `@src/Options/index.js`:
- Around line 428-433: The graphQLDepth and graphQLFields option entries lack
explicit :ENV: annotations so the generator creates awkward
PARSE_SERVER_REQUEST_COMPLEXITY_GRAPH_QLDEPTH/…_GRAPH_QLFIELDS names; add
explicit environment-variable names to those options in src/Options/index.js by
annotating graphQLDepth with ":ENV: GRAPHQL_DEPTH" and graphQLFields with ":ENV:
GRAPHQL_FIELDS" (so generated vars become PARSE_SERVER_GRAPHQL_DEPTH /
PARSE_SERVER_GRAPHQL_FIELDS), then run "npm run definitions" to propagate
changes to Definitions.js and docs.js.
---
Nitpick comments:
In `@spec/GraphQLQueryComplexity.spec.js`:
- Around line 42-55: The helper function buildDeepQuery is dead code; either
remove it or make the depth tests call it instead of using inline query strings.
Edit the spec/GraphQLQueryComplexity.spec.js tests that assert complexity for
various depths to call buildDeepQuery(depth) (using the same numeric depth
values currently inlined) so the tests reuse buildDeepQuery, or delete the
buildDeepQuery function if you prefer to keep the inline literals; update any
references to f0/f1-style synthetic fields accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89c7f221-8568-4608-b808-7d806ec1be49
📒 Files selected for processing (11)
resources/buildConfigDefinitions.jsspec/GraphQLQueryComplexity.spec.jsspec/RequestComplexity.spec.jsspec/RestQuery.spec.jssrc/Config.jssrc/GraphQL/ParseGraphQLServer.jssrc/GraphQL/helpers/queryComplexity.jssrc/Options/Definitions.jssrc/Options/docs.jssrc/Options/index.jssrc/RestQuery.js
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/GraphQL/helpers/queryComplexity.js (2)
23-31:⚠️ Potential issue | 🔴 CriticalCount repeated fragments per traversal path, not globally.
visitedFragmentsis still shared across sibling branches, so reusing the same fragment under different fields is only charged once. That undercounts wide queries and weakens the DoS protection this feature is meant to add.Proposed fix
} else if (selection.kind === 'FragmentSpread') { const name = selection.name.value; if (visitedFragments.has(name)) { continue; } - visitedFragments.add(name); const fragment = fragments[name]; if (fragment) { - visitSelectionSet(fragment.selectionSet, depth, visitedFragments); + const nextVisitedFragments = new Set(visitedFragments); + nextVisitedFragments.add(name); + visitSelectionSet(fragment.selectionSet, depth, nextVisitedFragments); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/GraphQL/helpers/queryComplexity.js` around lines 23 - 31, The bug is that visitedFragments is shared across sibling traversal paths causing fragments to be counted only once globally; in visitSelectionSet (and specifically where FragmentSpread is handled) pass a per-path copy of visitedFragments instead of the shared set—e.g., replace calls like visitSelectionSet(fragment.selectionSet, depth, visitedFragments) with visitSelectionSet(fragment.selectionSet, depth, new Set(visitedFragments)) so each traversal path tracks fragment visits independently (also apply the same cloning approach when recursing into sibling field/inline fragment branches in visitSelectionSet).
72-75:⚠️ Potential issue | 🟠 MajorOnly score the resolved operation.
didResolveOperationhas already selected the operation that will execute, but this still passes the full document intocalculateQueryComplexity(). A request with multiple operations can therefore be rejected because of an unused operation.Proposed fix
const { depth, fields } = calculateQueryComplexity( - requestContext.document, + { definitions: [requestContext.operation] }, fragments );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/GraphQL/helpers/queryComplexity.js` around lines 72 - 75, The code is passing the whole GraphQL document into calculateQueryComplexity causing unused operations to be scored; in the didResolveOperation handler replace requestContext.document with the resolved operation AST (requestContext.operation) so calculateQueryComplexity(requestContext.operation, fragments) computes complexity only for the selected operation (keep fragments as-is).
🧹 Nitpick comments (2)
spec/SecurityCheckGroups.spec.js (1)
44-51: Avoid asserting security checks by array index.Adding this check already shifted the numbering. Looking up the check by identity/title instead of position will keep these tests stable when more checks are inserted or reordered.
Also applies to: 74-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/SecurityCheckGroups.spec.js` around lines 44 - 51, Replace fragile index-based assertions in Spec file by finding checks by a stable identifier (e.g., title/name/id) and asserting their checkState equals CheckState.success; for example, use group.checks().find(c => c.title === '...') or similar instead of group.checks()[N].checkState() for all occurrences (the block with multiple expects using group.checks()[0], [1], [2], [4], [5], [6], [8], [9] and the similar block at lines 74-81), and update the test to throw a clear failure if the named check is not found before asserting its state.src/Security/CheckGroups/CheckGroupServerConfig.js (1)
148-149: Keep the checked keys in sync with the shared option definition.This list is duplicated here, so a future
RequestComplexityOptionsaddition can ship without security-check coverage and still report success. Consider deriving the keys from the shared option surface instead of hardcoding them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Security/CheckGroups/CheckGroupServerConfig.js` around lines 148 - 149, The check currently hardcodes RequestComplexity option keys (rc.includeDepth, rc.includeCount, rc.subqueryDepth, rc.graphQLDepth, rc.graphQLFields) which can drift from the shared RequestComplexityOptions; update the logic to derive the keys from the shared option definition (e.g., import or reference RequestComplexityOptions or the shared options object used elsewhere) and map those keys to rc[key] for the existence/`-1` check (replace the values array creation with something like Object.keys(RequestComplexityOptions).map(k => rc[k]) so future additions to RequestComplexityOptions are automatically covered).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/GraphQL/helpers/queryComplexity.js`:
- Around line 23-31: The bug is that visitedFragments is shared across sibling
traversal paths causing fragments to be counted only once globally; in
visitSelectionSet (and specifically where FragmentSpread is handled) pass a
per-path copy of visitedFragments instead of the shared set—e.g., replace calls
like visitSelectionSet(fragment.selectionSet, depth, visitedFragments) with
visitSelectionSet(fragment.selectionSet, depth, new Set(visitedFragments)) so
each traversal path tracks fragment visits independently (also apply the same
cloning approach when recursing into sibling field/inline fragment branches in
visitSelectionSet).
- Around line 72-75: The code is passing the whole GraphQL document into
calculateQueryComplexity causing unused operations to be scored; in the
didResolveOperation handler replace requestContext.document with the resolved
operation AST (requestContext.operation) so
calculateQueryComplexity(requestContext.operation, fragments) computes
complexity only for the selected operation (keep fragments as-is).
---
Nitpick comments:
In `@spec/SecurityCheckGroups.spec.js`:
- Around line 44-51: Replace fragile index-based assertions in Spec file by
finding checks by a stable identifier (e.g., title/name/id) and asserting their
checkState equals CheckState.success; for example, use group.checks().find(c =>
c.title === '...') or similar instead of group.checks()[N].checkState() for all
occurrences (the block with multiple expects using group.checks()[0], [1], [2],
[4], [5], [6], [8], [9] and the similar block at lines 74-81), and update the
test to throw a clear failure if the named check is not found before asserting
its state.
In `@src/Security/CheckGroups/CheckGroupServerConfig.js`:
- Around line 148-149: The check currently hardcodes RequestComplexity option
keys (rc.includeDepth, rc.includeCount, rc.subqueryDepth, rc.graphQLDepth,
rc.graphQLFields) which can drift from the shared RequestComplexityOptions;
update the logic to derive the keys from the shared option definition (e.g.,
import or reference RequestComplexityOptions or the shared options object used
elsewhere) and map those keys to rc[key] for the existence/`-1` check (replace
the values array creation with something like
Object.keys(RequestComplexityOptions).map(k => rc[k]) so future additions to
RequestComplexityOptions are automatically covered).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66973983-2f51-43d7-8744-731c7fafe5c4
📒 Files selected for processing (4)
spec/GraphQLQueryComplexity.spec.jsspec/SecurityCheckGroups.spec.jssrc/GraphQL/helpers/queryComplexity.jssrc/Security/CheckGroups/CheckGroupServerConfig.js
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/GraphQLQueryComplexity.spec.js
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/ParseGraphQLServer.spec.js`:
- Around line 9557-9566: After calling global.reconfigureServer and creating the
new ParseGraphQLServer instance, remount the fresh Parse Server instance so the
HTTP server the test client hits is replaced; specifically, ensure the old HTTP
server started in the outer beforeEach is stopped and the new parseServer (used
to construct parseGraphQLServer) is started/mounted (so requestComplexity: {
includeDepth: 10 } actually applies). Locate the reconfigureServer call and the
ParseGraphQLServer construction (parseServer, new ParseGraphQLServer(...)) and
add the appropriate teardown/start or helper call to replace the running HTTP
server with the newly configured parseServer before running the assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5547e2c5-a1c1-4856-8f32-313463001ad9
📒 Files selected for processing (1)
spec/ParseGraphQLServer.spec.js
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #10130 +/- ##
========================================
Coverage 92.70% 92.71%
========================================
Files 191 192 +1
Lines 15942 16051 +109
Branches 180 180
========================================
+ Hits 14779 14881 +102
- Misses 1151 1158 +7
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/RestQuery.js (1)
890-911: Implementation is correct, but note a potential user expectation mismatch.The
includeCountcheck validates againstthis.include.length, which includes all intermediate paths. For example,include=a.b.cis expanded internally to[['a'], ['a', 'b'], ['a', 'b', 'c']], counting as 3 includes rather than 1. This is technically correct since all intermediate pointers are resolved, but users specifyinginclude=a.b.c,x.y.zmight be surprised that it counts as 6 includes.Consider whether the documentation for
includeCountclarifies this behavior, or if the count should be based on user-specified paths instead of expanded paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/RestQuery.js` around lines 890 - 911, The includeCount check currently counts expanded intermediate include paths (this.include), causing user-surprise; change the check in _UnsafeRestQuery.prototype.validateIncludeComplexity to count user-intended include paths by deriving the set of maximal (non-prefix) paths from this.include before comparing to rc.includeCount—i.e., filter this.include to only keep paths that are not a prefix of any other path (join arrays to compare or compare lengths/element-wise), use that filtered length for the includeCount comparison and messages, and keep logging/throwing the same Parse.Error when exceeded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/RestQuery.js`:
- Around line 890-911: The includeCount check currently counts expanded
intermediate include paths (this.include), causing user-surprise; change the
check in _UnsafeRestQuery.prototype.validateIncludeComplexity to count
user-intended include paths by deriving the set of maximal (non-prefix) paths
from this.include before comparing to rc.includeCount—i.e., filter this.include
to only keep paths that are not a prefix of any other path (join arrays to
compare or compare lengths/element-wise), use that filtered length for the
includeCount comparison and messages, and keep logging/throwing the same
Parse.Error when exceeded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 96c24ae1-b3d3-48ac-8857-5891e5fc6e40
📒 Files selected for processing (3)
spec/ParseGraphQLServer.spec.jssrc/GraphQL/helpers/queryComplexity.jssrc/RestQuery.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/GraphQL/helpers/queryComplexity.js
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/RestQuery.js`:
- Around line 890-910: In validateIncludeComplexity (method
_UnsafeRestQuery.prototype.validateIncludeComplexity) deduplicate this.include
before enforcing rc.includeCount to avoid false rejections from
overlapping/duplicate paths (handleInclude and handleIncludeAll can leave
duplicates); create a Set of serialized include paths (e.g., serialize each
include entry consistently with path.join('.') or JSON.stringify(path)) and use
that Set.size for the includeCount check (and optionally for any subsequent
include-count-based logic), leaving this.include unchanged.
- Around line 461-475: The checkSubqueryDepth logic (in
_UnsafeRestQuery.prototype.checkSubqueryDepth) and the other places noted should
stop reading/writing context._subqueryDepth and instead use a private RestQuery
instance field (e.g., this._subqueryDepth) to track nesting; update all reads,
increments and resets (the occurrences referenced around the other check sites)
to use this internal property, ensure the auth checks and config use remain the
same, and do not expose or mutate the request context object so triggers still
receive the original context untouched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e540ec82-38d0-4cae-9e9d-cdfce167e98f
📒 Files selected for processing (4)
spec/GraphQLQueryComplexity.spec.jsspec/ParseGraphQLServer.spec.jssrc/GraphQL/helpers/queryComplexity.jssrc/RestQuery.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/GraphQL/helpers/queryComplexity.js
- spec/ParseGraphQLServer.spec.js
mtrezza
changed the title
parseplatformorg
pushed a commit
that referenced
this pull request
Mar 7, 2026
## [9.5.2-alpha.2](9.5.2-alpha.1...9.5.2-alpha.2) (2026-03-07) ### Bug Fixes * Denial-of-service via unbounded query complexity in REST and GraphQL API ([GHSA-cmj3-wx7h-ffvg](https://github.com/parse-community/parse-server/security/advisories/GHSA-cmj3-wx7h-ffvg)) ([#10130](#10130)) ([0ae9c25](0ae9c25))
Contributor
|
🎉 This change has been released in version 9.5.2-alpha.2 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Issue
Denial-of-service via unbounded query complexity in REST and GraphQL API (GHSA-cmj3-wx7h-ffvg)
Tasks
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Security