feat: Add Parse Server options maxIncludeQueryComplexity, maxGraphQLQueryComplexity to limit query complexity for performance protection
#9920
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! |
📝 WalkthroughWalkthroughAdds configurable query-complexity limits for GraphQL and REST includes, validates their cross-relations in Config, enforces GraphQL complexity via an Apollo plugin (with fragment/cycle handling and auth bypass), blocks/validates REST includes, and adds test suites for both features. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Apollo as ApolloServer
participant Plugin as ComplexityPlugin
participant Resolver
Note over Apollo: GraphQL request lifecycle
Client->>Apollo: POST /graphql (document, headers)
Apollo->>Apollo: Check auth (master/maintenance?)
alt master/maintenance key
Apollo->>Resolver: Execute query (no complexity check)
else normal request
Apollo->>Plugin: didResolveOperation(document)
activate Plugin
Plugin->>Plugin: build fragment map, traverse AST, count depth & fields
Plugin-->>Apollo: ok or throw GraphQLError(403)
deactivate Plugin
alt within limits
Apollo->>Resolver: Execute query
Resolver-->>Client: 200 + data
else exceeds limits
Apollo-->>Client: 403 GraphQLError (limit exceeded)
end
end
sequenceDiagram
participant Client
participant REST as REST Endpoint
participant Auth as Auth Check
participant Validator as IncludeValidator
participant Handler as QueryHandler
Client->>REST: GET /classes/Thing?include=...
REST->>Auth: Validate credentials
alt master key
Auth->>Handler: Proceed (no include validation)
else normal/maintenance
Auth->>Validator: Validate include count & depth
activate Validator
Validator->>Validator: count include fields, compute max depth
Validator-->>Auth: ok or INVALID_QUERY error
deactivate Validator
alt within limits
Auth->>Handler: Execute query and return results
else exceeds limits
REST-->>Client: 400 INVALID_QUERY
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
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 |
✅ 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. |
|
@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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #9920 +/- ##
==========================================
- Coverage 93.08% 93.05% -0.03%
==========================================
Files 188 189 +1
Lines 15298 15363 +65
Branches 177 177
==========================================
+ Hits 14240 14296 +56
- Misses 1046 1055 +9
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/Options/index.js (1)
354-369: Document the right counters for REST vs GraphQL complexityThe REST comment still describes a
{ depth, count }format while the shared type exposesfields, and the GraphQL comment says “fields = number of operations”, which contradicts the actual limiter (it counts field selections). This inconsistency will send users in circles. Please align the prose with the real counters—REST usescountfor include paths, GraphQL usesfieldsfor selected fields—and mention thatfieldslimits field selections, not operations.- /* Maximum query complexity for REST API includes. Controls depth and number of include fields. - * Format: { depth: number, count: number } - * - depth: Maximum depth of nested includes (e.g., foo.bar.baz = depth 3) - * - count: Maximum number of include fields (e.g., foo,bar,baz = 3 fields) + /* Maximum query complexity for REST API includes. Controls include depth and include count. + * Format: { depth?: number, count?: number } + * - depth: Maximum depth of nested include paths (e.g., foo.bar.baz = depth 3) + * - count: Maximum number of include paths across the query (e.g., foo,bar,baz = 3) … - /* 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 + /* Maximum query complexity for GraphQL queries. Controls depth and total field selections. + * Format: { depth?: number, fields?: number } + * - depth: Maximum depth of nested field selections + * - fields: Maximum number of field selections within the resolved operationspec/ParseGraphQLQueryComplexity.spec.js (1)
16-31: Close the HTTP server with a promise before reconfiguringLine 19 (and again Line 54):
httpServer.close()is callback-based; awaiting it directly never waits for the port to free, so the very nextlistenraces and we can hitEADDRINUSE. Please wrap the close call in a Promise (and clearhttpServer) before spinning up the next server, and reuse the same helper inafterEach.- async function reconfigureServer(options = {}) { - if (httpServer) { - await httpServer.close(); - } + async function shutdownHttpServer() { + if (!httpServer) { + return; + } + await new Promise(resolve => httpServer.close(resolve)); + httpServer = null; + } + + async function reconfigureServer(options = {}) { + await shutdownHttpServer(); … - if (httpServer) { - await httpServer.close(); - } + await shutdownHttpServer();Also applies to: 52-55
spec/RestQuery.spec.js (1)
1107-1133: Use the actual include-count key in the “no includes” testsLines 1110 & 1131 still configure
{ paths: 1 }, so we never exercise the include-count limiter branch these tests are meant to cover. Change those objects to use the realcountproperty so the server actually enforces the limit during the test run.maxIncludeQueryComplexity: { depth: 1, - paths: 1, + count: 1, }, … maxIncludeQueryComplexity: { depth: 1, - paths: 1, + count: 1, },src/Options/Definitions.js (1)
402-402: Duplicate: Fix the fields description.The description of the
fieldsproperty is misleading as noted in a previous review. It currently states "Maximum number of operations (queries/mutations)" but the implementation enforces the number of field selections, not operations.
🧹 Nitpick comments (1)
src/Options/Definitions.js (1)
416-421: Consider consistent property naming across complexity options.The
maxIncludeQueryComplexityoption uses acountproperty whilemaxGraphQLQueryComplexityuses afieldsproperty, even though both measure similar concepts (the number of items being selected or included). This inconsistency may cause confusion.Consider renaming one to match the other for API consistency, such as using
fieldsfor both orcountfor both. If the different names are intentional to reflect different semantics (GraphQL field selections vs REST include counts), this distinction should be clearly documented.
⚠️ Note: This is generated code. Changes must be made insrc/Options/index.js.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
spec/ParseGraphQLQueryComplexity.spec.js(1 hunks)spec/RestQuery.spec.js(1 hunks)src/Config.js(3 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)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/RestQuery.js
- src/Options/docs.js
🧰 Additional context used
🧠 Learnings (12)
📓 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.
📚 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.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: 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.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-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
spec/ParseGraphQLQueryComplexity.spec.jsspec/RestQuery.spec.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.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:
spec/RestQuery.spec.jssrc/Options/Definitions.jssrc/Options/index.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
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
spec/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: 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/RestQuery.spec.jssrc/Options/Definitions.jssrc/Config.jssrc/Options/index.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/index.js
🧬 Code graph analysis (4)
spec/ParseGraphQLQueryComplexity.spec.js (1)
spec/helper.js (1)
reconfigureServer(180-214)
spec/RestQuery.spec.js (3)
spec/helper.js (2)
reconfigureServer(180-214)Parse(4-4)src/RestQuery.js (3)
query(372-381)query(382-382)results(800-800)src/rest.js (1)
query(120-130)
src/Options/Definitions.js (1)
resources/buildConfigDefinitions.js (1)
parsers(12-12)
src/Options/index.js (2)
resources/buildConfigDefinitions.js (3)
type(121-121)type(159-159)type(216-216)src/triggers.js (1)
type(819-819)
🪛 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)
🔇 Additional comments (1)
src/Options/Definitions.js (1)
399-404: The core claim about an undefined option reference is incorrect.The review comment states that line 402 references a non-existent
maxQueryComplexityoption. However, searching the entire codebase confirmsmaxQueryComplexitydoes not appear anywhere. More importantly, line 402 actually references onlymaxIncludeQueryComplexityandmaxGraphQLQueryComplexity—both of which are properly defined options in the codebase and validated insrc/Config.js.A separate but valid concern exists: The environment variable
PARSE_SERVER_MAX_GRAPH_QLQUERY_COMPLEXITYuses inconsistent naming. Other GraphQL-related environment variables in the same file useGRAPHQL(e.g.,PARSE_SERVER_GRAPHQL_PATH,PARSE_SERVER_GRAPHQL_PUBLIC_INTROSPECTION,PARSE_SERVER_MOUNT_GRAPHQL), while two variables oddly useGRAPH_QL(PARSE_SERVER_GRAPH_QLSCHEMAandPARSE_SERVER_MAX_GRAPH_QLQUERY_COMPLEXITY). This is worth addressing, though it's a separate issue from what the review comment claimed.Likely an incorrect or invalid review comment.
…oumouls/parse-server into moumouls/include-prevention-complexity
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
spec/ParseGraphQLQueryComplexity.spec.js (1)
18-55: Await the HTTP server shutdown before restarting.
httpServer.close()is callback-based;await httpServer.close()returns immediately and the port can still be bound when the next test starts, causing intermittentEADDRINUSE. Wrap the close in a Promise (and mirror the change inafterEach) so we only continue once the listener is actually closed:if (httpServer) { - await httpServer.close(); + await new Promise(resolve => httpServer.close(resolve)); + httpServer = null; }Apply the same promise wrapper inside
afterEach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
spec/ParseGraphQLQueryComplexity.spec.js(1 hunks)src/GraphQL/helpers/queryComplexity.js(1 hunks)src/Options/docs.js(1 hunks)src/Options/index.js(2 hunks)types/Options/index.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/GraphQL/helpers/queryComplexity.js
🧰 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.
📚 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:
src/Options/docs.jssrc/Options/index.jsspec/ParseGraphQLQueryComplexity.spec.jstypes/Options/index.d.ts
📚 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/docs.jssrc/Options/index.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:
src/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/Options/index.js
📚 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.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.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.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-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
spec/ParseGraphQLQueryComplexity.spec.js
🧬 Code graph analysis (3)
src/Options/index.js (2)
resources/buildConfigDefinitions.js (3)
type(121-121)type(159-159)type(216-216)src/triggers.js (1)
type(819-819)
spec/ParseGraphQLQueryComplexity.spec.js (1)
spec/helper.js (1)
reconfigureServer(180-214)
types/Options/index.d.ts (6)
src/Adapters/Analytics/AnalyticsAdapter.js (1)
AnalyticsAdapter(6-23)src/Adapters/Logger/LoggerAdapter.js (1)
LoggerAdapter(9-18)src/Adapters/Cache/CacheAdapter.js (1)
CacheAdapter(6-32)src/Adapters/Email/MailAdapter.js (1)
MailAdapter(8-23)src/Adapters/PubSub/PubSubAdapter.js (1)
PubSubAdapter(6-15)src/Adapters/WebSocketServer/WSSAdapter.js (1)
WSSAdapter(17-57)
🪛 Biome (2.1.2)
src/Options/index.js
[error] 49-53: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 366-366: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
[error] 373-373: 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). (15)
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: Node 18
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: Node 22
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: Node 20
- GitHub Check: Redis Cache
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: Docker Build
- GitHub Check: Benchmarks
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Options/Definitions.js (1)
399-404: Thefieldsdescription still incorrectly states "operations" instead of "field selections".This issue was flagged in a previous review and marked as addressed, but the incorrect description remains. Line 402 still describes
fieldsas limiting "operations (queries/mutations)" when it should describe limiting the number of field selections in a single request.Since this is generated code, the fix must be applied in
src/Options/index.jsand thennpm run definitionsmust be executed to regenerate this file.Based on learnings.
🧹 Nitpick comments (1)
src/Options/Definitions.js (1)
400-400: Verify the environment variable naming convention.The environment variable
PARSE_SERVER_MAX_GRAPH_QLQUERY_COMPLEXITYhas inconsistent underscore placement. Based on other GraphQL-related variables in this file (e.g.,PARSE_SERVER_GRAPHQL_PATHon line 298), it should likely bePARSE_SERVER_MAX_GRAPHQL_QUERY_COMPLEXITYwithGRAPHQLas a single word.Since this is generated code, verify and correct the naming in
src/Options/index.js, then regenerate by runningnpm run definitions.Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Options/Definitions.js(1 hunks)src/Options/docs.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Options/docs.js
🧰 Additional context used
🧠 Learnings (4)
📓 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: 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.
📚 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:
src/Options/Definitions.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.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:
src/Options/Definitions.js
|
I suggest that developers set the parameters sufficiently high to prevent client issues, since the goal is to prevent major abuse, not to match the most complex query in the codebase. |
|
This is not a breaking change, correct? We'll make it a breaking change with #9928. |
|
exactly @mtrezza no breaking change currently |
mtrezza
changed the title
maxIncludeQueryComplexity, maxGraphQLQueryComplexity to limit query complexity for performance protection
|
@mtrezza i currently don't see a proper time window on my side to ship this correctly asap, i suggest to go ahead, it will be an opt in in PS9 and a breaking change for PS10, what do you think ? |
|
I think we could go ahead and merge and add this as a caveat. I've opened #9939 to track the feature improvement. If you could just fix the conflict and #9920 (comment). |
…xity # Conflicts: # src/GraphQL/ParseGraphQLServer.js
|
it's fixed @mtrezza we will have a merge conflict also between this one and apollo server update |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Options/index.js (1)
1-1: Consider a more concise PR title with scope for the changelog.The current title is quite long. For a clearer changelog entry, consider:
feat(security): add query complexity limits for REST and GraphQL APIsThis follows Angular convention with a scope and is more concise while still clearly conveying the feature's impact.
Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/GraphQL/ParseGraphQLServer.js(2 hunks)src/Options/Definitions.js(1 hunks)src/Options/docs.js(1 hunks)src/Options/index.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/GraphQL/ParseGraphQLServer.js
- src/Options/docs.js
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
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: 0
File: :0-0
Timestamp: 2025-11-17T15:02:48.786Z
Learning: For Parse Server PRs, always suggest an Angular commit convention PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion on every commit. The format should be: type(scope): description. Common types include feat, fix, perf, refactor, docs, test, chore. The scope should identify the subsystem (e.g., graphql, rest, push, security). The description should be action-oriented and clearly convey the change's impact to developers.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
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.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:24.824Z
Learning: For Parse Server PRs, always suggest an Angular-style PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion with every new commit to the PR.
📚 Learning: 2025-11-08T13:46:04.940Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
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:
src/Options/Definitions.jssrc/Options/index.js
📚 Learning: 2025-11-08T13:46:04.940Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
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/index.js
📚 Learning: 2025-11-17T15:02:48.786Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:48.786Z
Learning: For Parse Server PRs, always suggest an Angular commit convention PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion on every commit. The format should be: type(scope): description. Common types include feat, fix, perf, refactor, docs, test, chore. The scope should identify the subsystem (e.g., graphql, rest, push, security). The description should be action-oriented and clearly convey the change's impact to developers.
Applied to files:
src/Options/Definitions.jssrc/Options/index.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:
src/Options/Definitions.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/Options/index.js
🧬 Code graph analysis (1)
src/Options/Definitions.js (1)
resources/buildConfigDefinitions.js (1)
parsers(12-12)
🪛 Biome (2.1.2)
src/Options/index.js
[error] 49-53: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 368-368: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
[error] 377-377: 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). (15)
- GitHub Check: Redis Cache
- GitHub Check: Node 18
- GitHub Check: Node 20
- GitHub Check: Node 22
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: Benchmarks
- GitHub Check: Docker Build
🔇 Additional comments (2)
src/Options/Definitions.js (1)
1-5: Note: This is auto-generated code.Based on lines 1-5, this file is generated by
resources/buildConfigDefinitions.jsfromsrc/Options/index.js. Any issues should be fixed in the source file and then regenerated by runningnpm run definitions.src/Options/index.js (1)
46-53: LGTM! Type definitions are well-structured.The separate type definitions for
GraphQLQueryComplexityOptionsandIncludeComplexityOptionscorrectly reflect their different runtime shapes, and all fields are properly marked optional to match actual usage patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Options/index.js (1)
358-375: Option docs and HTML formatting for query complexity look good; minor follow-upsThe new
maxIncludeQueryComplexity/maxGraphQLQueryComplexityJSDoc blocks:
- Use the required HTML formatting (
<br><br>,<ul><li>) so they are parsable by the docs generator.- Correctly describe
depthandcount/fieldsin terms of include paths vs field selections and mention the cross‑option constraint, matching the validator behavior.Two optional polish ideas:
- Consider explicitly noting that
depth/count/fieldsare individually optional in the prose (since the types allow partial objects), e.g. “Supported keys:depth,count…”.- Suggested Angular-style PR title for the changelog:
feat(graphql,rest): add query complexity limits for GraphQL and REST includes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Options/Definitions.js(1 hunks)src/Options/docs.js(1 hunks)src/Options/index.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Options/docs.js
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
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: 0
File: :0-0
Timestamp: 2025-11-17T15:02:48.786Z
Learning: For Parse Server PRs, always suggest an Angular commit convention PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion on every commit. The format should be: type(scope): description. Common types include feat, fix, perf, refactor, docs, test, chore. The scope should identify the subsystem (e.g., graphql, rest, push, security). The description should be action-oriented and clearly convey the change's impact to developers.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
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.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:24.824Z
Learning: For Parse Server PRs, always suggest an Angular-style PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion with every new commit to the PR.
📚 Learning: 2025-11-08T13:46:04.940Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
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:
src/Options/Definitions.jssrc/Options/index.js
📚 Learning: 2025-11-08T13:46:04.940Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
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/index.js
📚 Learning: 2025-11-17T15:02:48.786Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:48.786Z
Learning: For Parse Server PRs, always suggest an Angular commit convention PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion on every commit. The format should be: type(scope): description. Common types include feat, fix, perf, refactor, docs, test, chore. The scope should identify the subsystem (e.g., graphql, rest, push, security). The description should be action-oriented and clearly convey the change's impact to developers.
Applied to files:
src/Options/Definitions.jssrc/Options/index.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:
src/Options/Definitions.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/Options/index.js
📚 Learning: 2025-11-17T15:02:24.824Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:24.824Z
Learning: For Parse Server PRs, always suggest an Angular-style PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion with every new commit to the PR.
Applied to files:
src/Options/index.js
🧬 Code graph analysis (2)
src/Options/Definitions.js (1)
resources/buildConfigDefinitions.js (1)
parsers(12-12)
src/Options/index.js (2)
src/triggers.js (1)
type(821-821)resources/buildConfigDefinitions.js (3)
type(121-121)type(159-159)type(216-216)
🪛 Biome (2.1.2)
src/Options/index.js
[error] 49-53: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 367-367: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
[error] 375-375: 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). (15)
- GitHub Check: Node 18
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Node 22
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: Docker Build
- GitHub Check: Redis Cache
- GitHub Check: Node 20
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: Benchmarks
- GitHub Check: MongoDB 7, ReplicaSet
🔇 Additional comments (2)
src/Options/index.js (1)
46-53: Flow type aliases correctly model complexity options; Biome warnings are benign
GraphQLQueryComplexityOptions/IncludeComplexityOptionswith optionaldepth/fields/countalign with the runtime behavior (supporting partial configs) and resolve earlier type-shape issues for these options. The Biome parse errors abouttypealiases and?are just the TS parser misreading this Flow-typed file; there’s nothing to change code-wise, but you may want to exclude this file or configure Biome to treat it as Flow.src/Options/Definitions.js (1)
399-410: Definitions for new complexity options are consistent with Options/index.jsThe
maxGraphQLQueryComplexity/maxIncludeQueryComplexityentries correctly:
- Use the intended env vars (
PARSE_SERVER_MAX_GRAPH_QLQUERY_COMPLEXITY,PARSE_SERVER_MAX_INCLUDE_QUERY_COMPLEXITY).- Mirror the HTML-formatted help text and semantics from
src/Options/index.js, including the field-selection vs include-path distinction and the “include < graphQL” constraint.- Parse via
objectParser, matching how other structured options are handled.This confirms
npm run definitionshas been run and keeps options/docs in sync; no changes needed here. (README coverage can follow later with the planned docs task.)
| <li>`depth`: Maximum depth of nested includes (e.g., foo.bar.baz = depth 3)</li> | ||
| <li>`count`: Maximum number of include fields (e.g., foo,bar,baz = 3 fields)</li> | ||
| </ul> | ||
| If both `maxIncludeQueryComplexity` and `maxGraphQLQueryComplexity` are provided, `maxIncludeQueryComplexity` values must be lower than `maxGraphQLQueryComplexity` values to avoid validation conflicts. */ |
There was a problem hiding this comment.
Add note about includeAll not usable
|
@Moumouls if we set default values at some point with #9928, but the value Should we define a value < 0 as disabling the options? |
|
@Moumouls Friendly ping so we can merge this. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
spec/RestQuery.spec.js (1)
1019-1188: Fixpathstypo in configs and consider tightening the “queries without includes” testsThe includeAll /
include('*')and “no limits configured” blocks look good: they correctly assert that:
- includeAll /
include('*')are rejected withINVALID_QUERYwhen any include complexity limit is set,- but still work with master key and when no complexity limits are configured.
Two issues in the “Queries without includes” section though:
- Config key typo (
pathsvscount)Both tests configure:
maxIncludeQueryComplexity: { depth: 1, paths: 1, },but the option surface and validator use
count, notpaths. This means the tests never exercise the count branch and are inconsistent with the public option shape.Change
pathstocountin both configs:maxIncludeQueryComplexity: { depth: 1, - paths: 1, + count: 1, },
- Two tests currently cover the same scenario
'should allow queries without includes regardless of complexity limits'and'should allow queries with empty includes array'currently execute identical logic (both justnew Parse.Query('SimpleObject').find()without any includes), so they don’t differentiate “no includes” from “empty includes”.If you want distinct coverage, you could, for example, turn the second test into an explicit REST-level call with
include=or use the SDK’s empty-include form so it truly hits the “empty includes” code path; otherwise, consider dropping or renaming it to avoid confusion. This is optional and doesn’t affect runtime behavior.Also applies to: 1191-1230, 1232-1267
🧹 Nitpick comments (3)
src/Options/index.js (1)
358-375: Surface includeAll / include("*") restrictions in the option docs and keep generated docs in syncThe option comments describe depth / count / fields and the cross-limit rule, but they don’t mention that when
maxIncludeQueryComplexityis configured, wildcard includes viaincludeAll()andinclude('*')are rejected for non-master / non-maintenance requests, as exercised inREST Query Complexitytests. That’s important behavior for operators.Consider extending the
maxIncludeQueryComplexitycomment with a short HTML-formatted note about wildcard includes being disabled under these limits (and that explicit include paths should be used instead), then runnpm run definitionssosrc/Options/docs.jsandsrc/Options/Definitions.jspick up the updated wording as well.Example diff for this file only:
/* Maximum query complexity for REST API includes. Controls depth and number of include fields. <br><br> Format: `{ depth: number, count: number }` <ul> <li>`depth`: Maximum depth of nested includes (e.g., foo.bar.baz = depth 3)</li> <li>`count`: Maximum number of include fields (e.g., foo,bar,baz = 3 fields)</li> - </ul> + </ul> + <br> + When this option is configured, wildcard includes via `includeAll()` or `include("*")` are disabled for non-maintenance and non-master key requests to prevent uncontrolled include expansion. Use explicit include paths instead. + <br><br> If both `maxIncludeQueryComplexity` and `maxGraphQLQueryComplexity` are provided, `maxIncludeQueryComplexity` values must be lower than `maxGraphQLQueryComplexity` values to avoid validation conflicts. */ maxIncludeQueryComplexity: ?IncludeComplexityOptions;Also, per the changelog guidelines, a concise Angular-style PR title that reflects this feature could be:
feat(security): add query complexity limits for GraphQL and REST includessrc/Options/docs.js (1)
74-75: Docs for new complexity options are aligned; extend via generator if you add includeAll noteThese
@propertyentries formaxGraphQLQueryComplexityandmaxIncludeQueryComplexitycorrectly describe:
- the option shapes (
{ depth, fields }vs{ depth, count }),- what each dimension controls, and
- the cross-limit constraint between REST includes and GraphQL complexity.
If you adopt the suggested includeAll /
include("*")note insrc/Options/index.js, please rely onnpm run definitionsto regenerate this file instead of editing it directly, so the JSDoc and generated docs stay in sync.src/Options/Definitions.js (1)
399-410: Generated definitions for new options look correct; keep them driven fromOptions/index.jsThe
maxGraphQLQueryComplexityandmaxIncludeQueryComplexityentries have:
- sensible env var names,
- help text consistent with the Flow option docs (depth vs fields / count and the cross-limit rule),
- and use
objectParser, which matches the{ depth, fields/count }structure.Given this file is generated, any further wording tweaks (for example, if you add an includeAll /
include("*")restriction note insrc/Options/index.js) should be made inOptions/index.jsand then propagated here vianpm run definitions, not edited by hand. No changes needed to this snippet itself.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
spec/RestQuery.spec.js(1 hunks)src/Options/Definitions.js(1 hunks)src/Options/docs.js(1 hunks)src/Options/index.js(2 hunks)src/RestQuery.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/RestQuery.js
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
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: 0
File: :0-0
Timestamp: 2025-11-17T15:02:48.786Z
Learning: For Parse Server PRs, always suggest an Angular commit convention PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion on every commit. The format should be: type(scope): description. Common types include feat, fix, perf, refactor, docs, test, chore. The scope should identify the subsystem (e.g., graphql, rest, push, security). The description should be action-oriented and clearly convey the change's impact to developers.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
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.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:24.824Z
Learning: For Parse Server PRs, always suggest an Angular-style PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion with every new commit to the PR.
📚 Learning: 2025-11-08T13:46:04.940Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
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:
src/Options/docs.jsspec/RestQuery.spec.jssrc/Options/Definitions.jssrc/Options/index.js
📚 Learning: 2025-11-08T13:46:04.940Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
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/docs.jssrc/Options/Definitions.jssrc/Options/index.js
📚 Learning: 2025-11-17T15:02:48.786Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:48.786Z
Learning: For Parse Server PRs, always suggest an Angular commit convention PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion on every commit. The format should be: type(scope): description. Common types include feat, fix, perf, refactor, docs, test, chore. The scope should identify the subsystem (e.g., graphql, rest, push, security). The description should be action-oriented and clearly convey the change's impact to developers.
Applied to files:
src/Options/docs.jssrc/Options/Definitions.jssrc/Options/index.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:
src/Options/docs.jssrc/Options/Definitions.js
📚 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/RestQuery.spec.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:
spec/RestQuery.spec.jssrc/Options/index.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/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/RestQuery.spec.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
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
spec/RestQuery.spec.js
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
spec/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/RestQuery.spec.js
📚 Learning: 2025-11-17T15:02:24.824Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:24.824Z
Learning: For Parse Server PRs, always suggest an Angular-style PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion with every new commit to the PR.
Applied to files:
src/Options/index.js
🧬 Code graph analysis (2)
spec/RestQuery.spec.js (3)
spec/helper.js (2)
reconfigureServer(180-214)Parse(4-4)spec/ParseQuery.FullTextSearch.spec.js (1)
Parse(4-4)src/RestQuery.js (3)
query(373-382)query(383-383)results(801-801)
src/Options/Definitions.js (1)
resources/buildConfigDefinitions.js (1)
parsers(12-12)
🪛 Biome (2.1.2)
src/Options/index.js
[error] 49-53: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 367-367: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
[error] 375-375: 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). (15)
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: Node 18
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Node 22
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: Node 20
- GitHub Check: Redis Cache
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: Docker Build
- GitHub Check: Benchmarks
🔇 Additional comments (2)
src/Options/index.js (1)
46-53: GraphQL / include complexity option types look consistent with runtime behavior
GraphQLQueryComplexityOptionsandIncludeComplexityOptionscorrectly model the two dimensions used at runtime (depth+fieldsvsdepth+count), and marking all properties optional matches how the validator accepts partial configs in tests and Config. No changes needed here.spec/RestQuery.spec.js (1)
769-1016: Depth and combined depth+count coverage is thorough and matches the option semanticsThe
maxIncludeQueryComplexity.depthand “Combined depth and fields validation” suites exercise:
- Allowed vs rejected depth-only queries (
depth: 2vsdepth: 1withpost.author).- Multiple sibling nested paths under the same depth budget (
post.author+post.category).- Master-key and maintenance-key bypasses for over-depth includes.
- A scenario where depth is generous but
countis tight, ensuring the count limit alone can reject a query.This aligns well with how the
depthandcountoptions are documented and implemented. No changes needed here.
| it('should allow queries with master key even when exceeding fields limit', async () => { | ||
| await reconfigureServer({ | ||
| maxIncludeQueryComplexity: { | ||
| count: 2, | ||
| }, | ||
| }); | ||
|
|
||
| // Create test objects with relationships | ||
| const user = new Parse.User(); | ||
| user.setUsername('testuser3'); | ||
| user.setPassword('password'); | ||
| await user.signUp(null, { useMasterKey: true }); | ||
|
|
||
| const post = new Parse.Object('Post'); | ||
| post.set('title', 'Test Post'); | ||
| post.set('author', user); | ||
| await post.save(null, { useMasterKey: true }); | ||
|
|
||
| const comment = new Parse.Object('Comment'); | ||
| comment.set('text', 'Test Comment'); | ||
| comment.set('post', post); | ||
| await comment.save(null, { useMasterKey: true }); | ||
|
|
||
| // Query with include that exceeds limit but using master key | ||
| const query = new Parse.Query('Comment'); | ||
| query.include('post'); | ||
| query.include('post.author'); | ||
| const results = await query.find({ useMasterKey: true }); | ||
|
|
||
| expect(results.length).toBeGreaterThan(0); | ||
| }); | ||
|
|
||
| it('should allow queries with maintenance key even when exceeding fields limit', async () => { | ||
| await reconfigureServer({ | ||
| maintenanceKey: 'maintenanceKey456', | ||
| maxIncludeQueryComplexity: { | ||
| count: 2, | ||
| }, | ||
| }); | ||
|
|
||
| // Create test objects with relationships using Parse SDK | ||
| const user = new Parse.User(); | ||
| user.setUsername('testuser4'); | ||
| user.setPassword('password'); | ||
| await user.signUp(); | ||
|
|
||
| const post = new Parse.Object('Post'); | ||
| post.set('title', 'Test Post'); | ||
| post.set('author', user); | ||
| await post.save(); | ||
|
|
||
| const comment = new Parse.Object('Comment'); | ||
| comment.set('text', 'Test Comment'); | ||
| comment.set('post', post); | ||
| await comment.save(); | ||
|
|
||
| // Query with include that exceeds limit but using maintenance key via REST API | ||
| const headers = { | ||
| 'X-Parse-Application-Id': 'test', | ||
| 'X-Parse-REST-API-Key': 'rest', | ||
| 'X-Parse-Maintenance-Key': 'maintenanceKey456', | ||
| }; | ||
| const response = await request({ | ||
| headers, | ||
| url: `http://localhost:8378/1/classes/Comment?include=post,post.author`, | ||
| json: true, | ||
| }); | ||
|
|
||
| expect(response.data.results.length).toBeGreaterThan(0); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Master / maintenance count tests never exceed the configured limit
In both:
it('should allow queries with master key even when exceeding fields limit', …)it('should allow queries with maintenance key even when exceeding fields limit', …)
you configure:
maxIncludeQueryComplexity: {
count: 2,
},but the queries only include:
query.include('post');
query.include('post.author');which is 2 include paths, i.e. at or below the limit. These tests would succeed even for non-privileged requests, so they don’t actually verify the intended master / maintenance bypass for an over-limit query.
To exercise the bypass, mirror the earlier “reject queries exceeding fields limit” case by adding a 3rd include (e.g. reply) and wiring the extra relation:
@@
- const comment = new Parse.Object('Comment');
- comment.set('text', 'Test Comment');
- comment.set('post', post);
- await comment.save(null, { useMasterKey: true });
+ const reply = new Parse.Object('Comment');
+ reply.set('text', 'Test Reply');
+ await reply.save(null, { useMasterKey: true });
+
+ const comment = new Parse.Object('Comment');
+ comment.set('text', 'Test Comment');
+ comment.set('post', post);
+ comment.set('reply', reply);
+ await comment.save(null, { useMasterKey: true });
@@
- query.include('post');
- query.include('post.author');
+ query.include('post');
+ query.include('post.author');
+ query.include('reply');
@@
- const results = await query.find({ useMasterKey: true });
+ const results = await query.find({ useMasterKey: true });and analogously for the maintenance-key test:
@@
- const comment = new Parse.Object('Comment');
- comment.set('text', 'Test Comment');
- comment.set('post', post);
- await comment.save();
+ const reply = new Parse.Object('Comment');
+ reply.set('text', 'Test Reply');
+ await reply.save();
+
+ const comment = new Parse.Object('Comment');
+ comment.set('text', 'Test Comment');
+ comment.set('post', post);
+ comment.set('reply', reply);
+ await comment.save();
@@
- url: `http://localhost:8378/1/classes/Comment?include=post,post.author`,
+ url: `http://localhost:8378/1/classes/Comment?include=post,post.author,reply`,This way, only master / maintenance requests succeed for an over-limit include set, and the tests validate the bypass path.
3 participants
Pull Request
Issue
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
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.