feat: Max depth and max fields protection system #9920
Draft
+1,428
−1
Conversation
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
|
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! |
📝 WalkthroughWalkthroughQuery complexity limits are introduced for both GraphQL and REST APIs. New configuration options Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphQL as Apollo Server
participant Plugin as Complexity Plugin
participant Resolver
Client->>GraphQL: Query Request
Note over GraphQL: Validate Auth (master/maintenance?)
alt Master or Maintenance Key
GraphQL->>Resolver: Skip complexity check
else Normal Auth
GraphQL->>Plugin: Calculate query complexity
activate Plugin
Plugin->>Plugin: Traverse AST + fragments<br/>Count fields & depth
deactivate Plugin
alt Complexity within limits
GraphQL->>Resolver: Execute query
else Complexity exceeds limit
GraphQL->>Client: GraphQLError: Limit exceeded
end
end
Resolver->>Client: Query result
sequenceDiagram
participant Client
participant REST as REST Endpoint
participant Auth as Auth Check
participant Validator as Complexity Validator
participant Query as Query Handler
Client->>REST: Request with includes
REST->>Auth: Check credentials
alt Master key present
Auth->>Query: Proceed without validation
else Maintenance key or User
Auth->>Validator: Validate includes
activate Validator
Validator->>Validator: Count include fields<br/>Calculate depth
deactivate Validator
alt Within limits
Validator->>Query: Proceed
else Exceeds limits
Validator->>Client: Error: INVALID_QUERY
end
end
Query->>Client: Query result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
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. |
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
spec/ParseGraphQLQueryComplexity.spec.js(1 hunks)spec/RestQuery.spec.js(1 hunks)src/Config.js(3 hunks)src/GraphQL/ParseGraphQLServer.js(2 hunks)src/GraphQL/helpers/queryComplexity.js(1 hunks)src/Options/Definitions.js(2 hunks)src/Options/docs.js(1 hunks)src/Options/index.js(2 hunks)src/RestQuery.js(3 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.917Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9858
File: src/GraphQL/ParseGraphQLServer.js:176-178
Timestamp: 2025-09-21T15:43:32.265Z
Learning: The GraphQL playground feature in ParseGraphQLServer.js (applyPlayground method) is intended for development environments only, which is why it includes the master key in client-side headers.
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
spec/ParseGraphQLQueryComplexity.spec.jssrc/RestQuery.jsspec/RestQuery.spec.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
spec/ParseGraphQLQueryComplexity.spec.jsspec/RestQuery.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
spec/ParseGraphQLQueryComplexity.spec.jsspec/RestQuery.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
spec/ParseGraphQLQueryComplexity.spec.js
📚 Learning: 2025-11-08T13:46:04.917Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.917Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
Applied to files:
spec/ParseGraphQLQueryComplexity.spec.jssrc/Options/index.jssrc/Options/Definitions.jssrc/Config.jssrc/Options/docs.js
📚 Learning: 2025-09-21T15:43:32.265Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9858
File: src/GraphQL/ParseGraphQLServer.js:176-178
Timestamp: 2025-09-21T15:43:32.265Z
Learning: The GraphQL playground feature in ParseGraphQLServer.js (applyPlayground method) is intended for development environments only, which is why it includes the master key in client-side headers.
Applied to files:
spec/ParseGraphQLQueryComplexity.spec.jssrc/Options/Definitions.jssrc/Config.jssrc/Options/docs.js
📚 Learning: 2025-08-26T14:06:31.853Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.853Z
Learning: In the Parse Server codebase, when handling query objects in maybeRunAfterFindTrigger, objects without a where property that contain options like limit/skip should be treated as query JSON with an empty where clause using the spread pattern { where: {}, ...query }, not nested as { where: query }.
Applied to files:
src/RestQuery.jsspec/RestQuery.spec.js
📚 Learning: 2025-11-08T13:46:04.917Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.917Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Applied to files:
src/Options/Definitions.jssrc/Options/docs.js
📚 Learning: 2025-08-27T12:33:06.237Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:467-477
Timestamp: 2025-08-27T12:33:06.237Z
Learning: In the Parse Server codebase, maybeRunAfterFindTrigger is called in production with Parse.Query objects constructed via withJSON(), so the plain object query handling bug only affects tests, not production code paths.
Applied to files:
spec/RestQuery.spec.js
🪛 Biome (2.1.2)
src/Options/index.js
[error] 45-49: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 362-362: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
⏰ 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). (14)
- GitHub Check: Node 22
- GitHub Check: Node 18
- GitHub Check: Redis Cache
- GitHub Check: Node 20
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Docker Build
Comment on lines
+15
to
+49
| if (httpServer) { | ||
| await httpServer.close(); | ||
| } | ||
| parseServer = await global.reconfigureServer(options); | ||
| const expressApp = express(); | ||
| httpServer = http.createServer(expressApp); | ||
| expressApp.use('/parse', parseServer.app); | ||
| parseGraphQLServer = new ParseGraphQLServer(parseServer, { | ||
| graphQLPath: '/graphql', | ||
| playgroundPath: '/playground', | ||
| subscriptionsPath: '/subscriptions', | ||
| }); | ||
| parseGraphQLServer.applyGraphQL(expressApp); | ||
| await new Promise(resolve => httpServer.listen({ port: 13378 }, resolve)); | ||
|
|
||
| const httpLink = createHttpLink({ | ||
| uri: 'http://localhost:13378/graphql', | ||
| fetch: (...args) => import('node-fetch').then(({ default: fetch }) => fetch(...args)), | ||
| }); | ||
|
|
||
| apolloClient = new ApolloClient({ | ||
| link: httpLink, | ||
| cache: new InMemoryCache(), | ||
| defaultOptions: { | ||
| query: { | ||
| fetchPolicy: 'no-cache', | ||
| }, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| afterEach(async () => { | ||
| if (httpServer) { | ||
| await httpServer.close(); | ||
| } |
There was a problem hiding this comment.
Ensure the HTTP server is fully closed before reconfiguring
httpServer.close() is callback-based; awaiting it directly does not wait for the socket to shut down. That leaves the listener alive while you immediately spin up the next server on the same port, which can intermittently throw EADDRINUSE and flake the suite. Please wrap the close in a Promise (and reuse the same helper in afterEach) so we only proceed once the server has actually released the port.
async function reconfigureServer(options = {}) {
if (httpServer) {
- await httpServer.close();
+ await new Promise(resolve => httpServer.close(resolve));
+ httpServer = null;
}
@@
afterEach(async () => {
if (httpServer) {
- await httpServer.close();
+ await new Promise(resolve => httpServer.close(resolve));
+ httpServer = null;
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (httpServer) { | |
| await httpServer.close(); | |
| } | |
| parseServer = await global.reconfigureServer(options); | |
| const expressApp = express(); | |
| httpServer = http.createServer(expressApp); | |
| expressApp.use('/parse', parseServer.app); | |
| parseGraphQLServer = new ParseGraphQLServer(parseServer, { | |
| graphQLPath: '/graphql', | |
| playgroundPath: '/playground', | |
| subscriptionsPath: '/subscriptions', | |
| }); | |
| parseGraphQLServer.applyGraphQL(expressApp); | |
| await new Promise(resolve => httpServer.listen({ port: 13378 }, resolve)); | |
| const httpLink = createHttpLink({ | |
| uri: 'http://localhost:13378/graphql', | |
| fetch: (...args) => import('node-fetch').then(({ default: fetch }) => fetch(...args)), | |
| }); | |
| apolloClient = new ApolloClient({ | |
| link: httpLink, | |
| cache: new InMemoryCache(), | |
| defaultOptions: { | |
| query: { | |
| fetchPolicy: 'no-cache', | |
| }, | |
| }, | |
| }); | |
| } | |
| afterEach(async () => { | |
| if (httpServer) { | |
| await httpServer.close(); | |
| } | |
| if (httpServer) { | |
| await new Promise(resolve => httpServer.close(resolve)); | |
| httpServer = null; | |
| } | |
| parseServer = await global.reconfigureServer(options); | |
| const expressApp = express(); | |
| httpServer = http.createServer(expressApp); | |
| expressApp.use('/parse', parseServer.app); | |
| parseGraphQLServer = new ParseGraphQLServer(parseServer, { | |
| graphQLPath: '/graphql', | |
| playgroundPath: '/playground', | |
| subscriptionsPath: '/subscriptions', | |
| }); | |
| parseGraphQLServer.applyGraphQL(expressApp); | |
| await new Promise(resolve => httpServer.listen({ port: 13378 }, resolve)); | |
| const httpLink = createHttpLink({ | |
| uri: 'http://localhost:13378/graphql', | |
| fetch: (...args) => import('node-fetch').then(({ default: fetch }) => fetch(...args)), | |
| }); | |
| apolloClient = new ApolloClient({ | |
| link: httpLink, | |
| cache: new InMemoryCache(), | |
| defaultOptions: { | |
| query: { | |
| fetchPolicy: 'no-cache', | |
| }, | |
| }, | |
| }); | |
| } | |
| afterEach(async () => { | |
| if (httpServer) { | |
| await new Promise(resolve => httpServer.close(resolve)); | |
| httpServer = null; | |
| } |
🤖 Prompt for AI Agents
In spec/ParseGraphQLQueryComplexity.spec.js around lines 15 to 49, the test
calls await httpServer.close() but httpServer.close is callback-based and does
not return a Promise; wrap the close call in a Promise so the test waits for the
socket to fully release the port before reconfiguring or starting a new server,
and use the same Promise-wrapping helper in both the setup and afterEach
teardown to avoid EADDRINUSE flakes.
Comment on lines
+1103
to
+1138
| maxQueryComplexity: { | ||
| depth: 1, | ||
| paths: 1, | ||
| }, | ||
| }); | ||
|
|
||
| const simpleObject = new Parse.Object('SimpleObject'); | ||
| simpleObject.set('name', 'Test'); | ||
| simpleObject.set('value', 123); | ||
| await simpleObject.save(); | ||
|
|
||
| // Query without includes should not be affected by complexity limits | ||
| const query = new Parse.Query('SimpleObject'); | ||
| const results = await query.find(); | ||
|
|
||
| expect(results.length).toBeGreaterThan(0); | ||
| }); | ||
|
|
||
| it('should allow queries with empty includes array', async () => { | ||
| await reconfigureServer({ | ||
| maxQueryComplexity: { | ||
| depth: 1, | ||
| paths: 1, | ||
| }, | ||
| }); | ||
|
|
||
| const simpleObject = new Parse.Object('SimpleObject'); | ||
| simpleObject.set('name', 'Test'); | ||
| simpleObject.set('value', 123); | ||
| await simpleObject.save(); | ||
|
|
||
| // Query with empty includes should not be affected | ||
| const query = new Parse.Query('SimpleObject'); | ||
| const results = await query.find(); | ||
|
|
||
| expect(results.length).toBeGreaterThan(0); |
There was a problem hiding this comment.
Correct the maxQueryComplexity configuration typo
Both “Queries without includes” tests set paths: 1, but the server option looks for fields. With the typo we never exercise the fields limit branch these tests aim to cover. Please fix the key so the tests configure the intended limit.
maxQueryComplexity: {
depth: 1,
- paths: 1,
+ fields: 1,
},
@@
maxQueryComplexity: {
depth: 1,
- paths: 1,
+ fields: 1,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| maxQueryComplexity: { | |
| depth: 1, | |
| paths: 1, | |
| }, | |
| }); | |
| const simpleObject = new Parse.Object('SimpleObject'); | |
| simpleObject.set('name', 'Test'); | |
| simpleObject.set('value', 123); | |
| await simpleObject.save(); | |
| // Query without includes should not be affected by complexity limits | |
| const query = new Parse.Query('SimpleObject'); | |
| const results = await query.find(); | |
| expect(results.length).toBeGreaterThan(0); | |
| }); | |
| it('should allow queries with empty includes array', async () => { | |
| await reconfigureServer({ | |
| maxQueryComplexity: { | |
| depth: 1, | |
| paths: 1, | |
| }, | |
| }); | |
| const simpleObject = new Parse.Object('SimpleObject'); | |
| simpleObject.set('name', 'Test'); | |
| simpleObject.set('value', 123); | |
| await simpleObject.save(); | |
| // Query with empty includes should not be affected | |
| const query = new Parse.Query('SimpleObject'); | |
| const results = await query.find(); | |
| expect(results.length).toBeGreaterThan(0); | |
| maxQueryComplexity: { | |
| depth: 1, | |
| fields: 1, | |
| }, | |
| }); | |
| const simpleObject = new Parse.Object('SimpleObject'); | |
| simpleObject.set('name', 'Test'); | |
| simpleObject.set('value', 123); | |
| await simpleObject.save(); | |
| // Query without includes should not be affected by complexity limits | |
| const query = new Parse.Query('SimpleObject'); | |
| const results = await query.find(); | |
| expect(results.length).toBeGreaterThan(0); | |
| }); | |
| it('should allow queries with empty includes array', async () => { | |
| await reconfigureServer({ | |
| maxQueryComplexity: { | |
| depth: 1, | |
| fields: 1, | |
| }, | |
| }); | |
| const simpleObject = new Parse.Object('SimpleObject'); | |
| simpleObject.set('name', 'Test'); | |
| simpleObject.set('value', 123); | |
| await simpleObject.save(); | |
| // Query with empty includes should not be affected | |
| const query = new Parse.Query('SimpleObject'); | |
| const results = await query.find(); | |
| expect(results.length).toBeGreaterThan(0); |
🤖 Prompt for AI Agents
In spec/RestQuery.spec.js around lines 1103 to 1138, the test configuration for
maxQueryComplexity mistakenly uses the key "paths" instead of the expected
"fields", so the fields limit branch is never exercised; update the
maxQueryComplexity objects in these tests to use "fields: 1" (replacing "paths:
1") so the server receives the intended fields limit and the tests validate the
fields-related behavior.
Comment on lines
+9
to
+105
| const operationAST = getOperationAST(document); | ||
| if (!operationAST || !operationAST.selectionSet) { | ||
| return { depth: 0, fields: 0 }; | ||
| } | ||
|
|
||
| // Build fragment definition map | ||
| const fragments = {}; | ||
| if (document.definitions) { | ||
| document.definitions.forEach(def => { | ||
| if (def.kind === Kind.FRAGMENT_DEFINITION) { | ||
| fragments[def.name.value] = def; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| let maxDepth = 0; | ||
| let fields = 0; | ||
|
|
||
| function visitSelectionSet(selectionSet, depth) { | ||
| if (!selectionSet || !selectionSet.selections) { | ||
| return; | ||
| } | ||
|
|
||
| selectionSet.selections.forEach(selection => { | ||
| if (selection.kind === Kind.FIELD) { | ||
| fields++; | ||
| maxDepth = Math.max(maxDepth, depth); | ||
| if (selection.selectionSet) { | ||
| visitSelectionSet(selection.selectionSet, depth + 1); | ||
| } | ||
| } else if (selection.kind === Kind.INLINE_FRAGMENT) { | ||
| // Inline fragments don't add depth, just traverse their selections | ||
| visitSelectionSet(selection.selectionSet, depth); | ||
| } else if (selection.kind === Kind.FRAGMENT_SPREAD) { | ||
| const fragmentName = selection.name.value; | ||
| const fragment = fragments[fragmentName]; | ||
| // Note: Circular fragments are already prevented by GraphQL validation (NoFragmentCycles rule) | ||
| // so we don't need to check for cycles here | ||
| if (fragment && fragment.selectionSet) { | ||
| visitSelectionSet(fragment.selectionSet, depth); | ||
| } | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| visitSelectionSet(operationAST.selectionSet, 1); | ||
| return { depth: maxDepth, fields }; | ||
| } | ||
|
|
||
| /** | ||
| * Create a GraphQL complexity validation plugin for Apollo Server | ||
| * Computes depth and total field count directly from the parsed GraphQL document | ||
| * @param {Object} config - Parse Server config object | ||
| * @returns {Object} Apollo Server plugin | ||
| */ | ||
| export function createComplexityValidationPlugin(config) { | ||
| return { | ||
| requestDidStart: () => ({ | ||
| didResolveOperation: async (requestContext) => { | ||
| const { document } = requestContext; | ||
| const auth = requestContext.contextValue?.auth; | ||
|
|
||
| // Skip validation for master/maintenance keys | ||
| if (auth?.isMaster || auth?.isMaintenance) { | ||
| return; | ||
| } | ||
|
|
||
| // Skip if no complexity limits are configured | ||
| if (!config.maxGraphQLQueryComplexity) { | ||
| return; | ||
| } | ||
|
|
||
| // Skip if document is not available | ||
| if (!document) { | ||
| return; | ||
| } | ||
|
|
||
| const maxGraphQLQueryComplexity = config.maxGraphQLQueryComplexity; | ||
|
|
||
| // Calculate depth and fields in a single pass for performance | ||
| const { depth, fields } = calculateQueryComplexity(document); | ||
|
|
||
| // Validate fields (field count) | ||
| if (maxGraphQLQueryComplexity.fields && fields > maxGraphQLQueryComplexity.fields) { | ||
| throw new GraphQLError( | ||
| `Number of fields selected exceeds maximum allowed`, | ||
| ); | ||
| } | ||
|
|
||
| // Validate maximum depth | ||
| if (maxGraphQLQueryComplexity.depth && depth > maxGraphQLQueryComplexity.depth) { | ||
| throw new GraphQLError( | ||
| `Query depth exceeds maximum allowed depth`, | ||
| ); | ||
| } | ||
| }, | ||
| }), |
There was a problem hiding this comment.
Pass the resolved operation into complexity calculation
With multi-operation documents getOperationAST(document) returns null unless we supply the selected operationName. In that case the function exits early, so the complexity limits are never enforced. A client can therefore bundle a cheap “decoy” operation first in the document, set operationName to the expensive one, and bypass the limiter entirely. Please feed the resolved operation into the calculation—e.g. pass requestContext.request.operationName (or reuse requestContext.operation) to calculateQueryComplexity and update that helper to respect it—so multi-operation requests cannot skip validation.
🤖 Prompt for AI Agents
In src/GraphQL/helpers/queryComplexity.js around lines 9 to 105,
calculateQueryComplexity is called with only the parsed document which lets
multi-operation documents bypass validation because getOperationAST(document)
returns null unless the selected operationName is supplied; change the plugin to
pass the resolved operation (e.g. pass requestContext.request.operationName or
requestContext.operation) into calculateQueryComplexity and update the helper to
accept either an operationName or an OperationDefinition node and call
getOperationAST(document, operationName) (or use the provided
OperationDefinition directly) so the complexity calculation always targets the
actual requested operation; ensure the helper falls back safely and still
returns {depth:0, fields:0} for missing inputs.
Comment on lines
+399
to
+404
| maxGraphQLQueryComplexity: { | ||
| env: 'PARSE_SERVER_MAX_GRAPH_QLQUERY_COMPLEXITY', | ||
| help: | ||
| 'Maximum query complexity for GraphQL queries. Controls depth and number of operations.* Format: { depth: number, fields: number }* - depth: Maximum depth of nested field selections* - fields: Maximum number of operations (queries/mutations) in a single request* If both maxQueryComplexity and maxGraphQLQueryComplexity are provided, maxQueryComplexity values* must be lower than maxGraphQLQueryComplexity values to avoid validation conflicts.', | ||
| action: parsers.objectParser, | ||
| }, |
There was a problem hiding this comment.
Fix the maxGraphQLQueryComplexity fields description
The new help text says the fields limit controls the number of operations, but the implementation (and tests expecting “Number of fields selected exceeds maximum allowed”) enforces the number of field selections. That discrepancy will mislead operators configuring the option. Please update the copy to accurately describe the field-selection limit.
- 'Maximum query complexity for GraphQL queries. Controls depth and number of operations.* Format: { depth: number, fields: number }* - depth: Maximum depth of nested field selections* - fields: Maximum number of operations (queries/mutations) in a single request* If both maxQueryComplexity and maxGraphQLQueryComplexity are provided, maxQueryComplexity values* must be lower than maxGraphQLQueryComplexity values to avoid validation conflicts.',
+ 'Maximum query complexity for GraphQL queries. Controls depth and number of selected fields.* Format: { depth: number, fields: number }* - depth: Maximum depth of nested field selections* - fields: Maximum number of field selections in a single request* If both maxQueryComplexity and maxGraphQLQueryComplexity are provided, maxQueryComplexity values* must be lower than maxGraphQLQueryComplexity values to avoid validation conflicts.',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| maxGraphQLQueryComplexity: { | |
| env: 'PARSE_SERVER_MAX_GRAPH_QLQUERY_COMPLEXITY', | |
| help: | |
| 'Maximum query complexity for GraphQL queries. Controls depth and number of operations.* Format: { depth: number, fields: number }* - depth: Maximum depth of nested field selections* - fields: Maximum number of operations (queries/mutations) in a single request* If both maxQueryComplexity and maxGraphQLQueryComplexity are provided, maxQueryComplexity values* must be lower than maxGraphQLQueryComplexity values to avoid validation conflicts.', | |
| action: parsers.objectParser, | |
| }, | |
| maxGraphQLQueryComplexity: { | |
| env: 'PARSE_SERVER_MAX_GRAPH_QLQUERY_COMPLEXITY', | |
| help: | |
| 'Maximum query complexity for GraphQL queries. Controls depth and number of selected fields.* Format: { depth: number, fields: number }* - depth: Maximum depth of nested field selections* - fields: Maximum number of field selections in a single request* If both maxQueryComplexity and maxGraphQLQueryComplexity are provided, maxQueryComplexity values* must be lower than maxGraphQLQueryComplexity values to avoid validation conflicts.', | |
| action: parsers.objectParser, | |
| }, |
🤖 Prompt for AI Agents
In src/Options/Definitions.js around lines 399 to 404, the help text for
maxGraphQLQueryComplexity incorrectly states that "fields" limits the number of
operations; update the copy so it accurately reflects the implementation and
tests by describing "fields" as the maximum number of field selections in a
single request (e.g., "fields: Maximum number of field selections in a single
request") and adjust surrounding wording to reference field selections rather
than operations so operators are not misled.
Comment on lines
+74
to
+77
| * @property {QueryComplexityOptions} maxGraphQLQueryComplexity Maximum query complexity for GraphQL queries. Controls depth and number of operations.* Format: { depth: number, fields: number }* - depth: Maximum depth of nested field selections* - fields: Maximum number of operations (queries/mutations) in a single request* If both maxQueryComplexity and maxGraphQLQueryComplexity are provided, maxQueryComplexity values* must be lower than maxGraphQLQueryComplexity values to avoid validation conflicts. | ||
| * @property {Number} maxLimit Max value for limit option on queries, defaults to unlimited | ||
| * @property {Number|String} maxLogFiles Maximum number of logs to keep. If not set, no logs will be removed. This can be a number of files or number of days. If using days, add 'd' as the suffix. (default: null) | ||
| * @property {QueryComplexityOptions} maxQueryComplexity Maximum query complexity for REST API includes. Controls depth and number of include fields.* Format: { depth: number, fields: number }* - depth: Maximum depth of nested includes (e.g., foo.bar.baz = depth 3)* - fields: Maximum number of include fields (e.g., foo,bar,baz = 3 fields)* If both maxQueryComplexity and maxGraphQLQueryComplexity are provided, maxQueryComplexity values* must be lower than maxGraphQLQueryComplexity values to avoid validation conflicts. |
There was a problem hiding this comment.
Document the fields option as a field-count limit
The description currently says “Maximum number of operations (queries/mutations)”, but the implementation enforces the number of selected fields. That mismatch will lead operators to configure the option incorrectly and wonder why simple requests are rejected. Please update the wording to state that fields limits the number of selected fields (and for GraphQL clarify it’s field selections, not operations).
🤖 Prompt for AI Agents
In src/Options/docs.js around lines 74 to 77, update the documentation for both
maxGraphQLQueryComplexity and maxQueryComplexity so the "fields" entry is
described as a limit on the number of selected fields (field selections), not
the number of operations/queries/mutations; for GraphQL explicitly say it limits
field selections (not operations), and for REST includes say it limits the
number of include fields — replace the current wording that mentions "operations
(queries/mutations)" with phrasing that clearly states "maximum number of
selected fields / field selections."
Comment on lines
+354
to
+369
| /* Maximum query complexity for REST API includes. Controls depth and number of include fields. | ||
| * Format: { depth: number, fields: number } | ||
| * - depth: Maximum depth of nested includes (e.g., foo.bar.baz = depth 3) | ||
| * - fields: Maximum number of include fields (e.g., foo,bar,baz = 3 fields) | ||
| * If both maxQueryComplexity and maxGraphQLQueryComplexity are provided, maxQueryComplexity values | ||
| * must be lower than maxGraphQLQueryComplexity values to avoid validation conflicts. | ||
| */ | ||
| maxQueryComplexity: ?QueryComplexityOptions; | ||
| /* Maximum query complexity for GraphQL queries. Controls depth and number of operations. | ||
| * Format: { depth: number, fields: number } | ||
| * - depth: Maximum depth of nested field selections | ||
| * - fields: Maximum number of operations (queries/mutations) in a single request | ||
| * If both maxQueryComplexity and maxGraphQLQueryComplexity are provided, maxQueryComplexity values | ||
| * must be lower than maxGraphQLQueryComplexity values to avoid validation conflicts. | ||
| */ | ||
| maxGraphQLQueryComplexity: ?QueryComplexityOptions; |
There was a problem hiding this comment.
Clarify the meaning of fields in the public type
The public option comment repeats that fields controls “number of operations”, which conflicts with the actual behaviour (it caps field selections). This will mislead integrators using the type definitions. Please align the comment with the implementation and state that it limits the number of fields (include paths for REST / field selections for GraphQL).
🧰 Tools
🪛 Biome (2.1.2)
[error] 362-362: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
🤖 Prompt for AI Agents
In src/Options/index.js around lines 354 to 369, the JSDoc for
QueryComplexityOptions incorrectly describes "fields" as limiting the number of
"operations" for GraphQL; update both comment blocks so "fields" is described
consistently with the implementation — it limits the number of field selections
(for GraphQL) and include paths (for REST), not operations, and ensure both
maxQueryComplexity and maxGraphQLQueryComplexity descriptions use the same
wording and examples.
Comment on lines
+299
to
+314
| if (!this.auth.isMaster && !this.auth.isMaintenance && this.config.maxQueryComplexity && this.include && this.include.length > 0) { | ||
| const fieldsCount = this.include.length; | ||
|
|
||
| if (this.config.maxQueryComplexity.fields && fieldsCount > this.config.maxQueryComplexity.fields) { | ||
| throw new Parse.Error( | ||
| Parse.Error.INVALID_QUERY, | ||
| `Number of include fields exceeds maximum allowed` | ||
| ); | ||
| } | ||
|
|
||
| const depth = Math.max(...this.include.map(path => path.length)); | ||
| if (this.config.maxQueryComplexity.depth && depth > this.config.maxQueryComplexity.depth) { | ||
| throw new Parse.Error( | ||
| Parse.Error.INVALID_QUERY, | ||
| `Include depth exceeds maximum allowed` | ||
| ); |
There was a problem hiding this comment.
Count requested include paths, not internal expansion nodes
this.include.length is counting the internal expansion order, so include=foo.bar becomes ['foo'] and ['foo','bar'] and we report two fields instead of the single user request. That immediately trips the fields cap even when the client stays within the documented limit. Please derive the count from the actual include strings (e.g. the deduped paths set before we explode them into traversal order, or from restOptions.include after normalisation) so nested includes don’t get double-counted. Otherwise legitimate requests will start failing as soon as limits are enabled.
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
WIP: i need to check tests and finalize some checks
Give developers the option to strengthen the protection of the Parse Server REST and GraphQL APIs based on complexity factors such as fields and query depth.
Approach
Currently parse-server can't have default values because it's a breaking change.
Also if in a futur major release we introduce some large default values (Depth 10 + Fields 100 on rest) and (Depth 20 and fields 200 on GQL). The includeAll option should be then masterKey only
Tasks
Summary by CodeRabbit
New Features
maxGraphQLQueryComplexityandmaxQueryComplexityoptions.Tests