parse-server icon indicating copy to clipboard operation
parse-server copied to clipboard

feat: Add Parse Server options `maxIncludeQueryComplexity`, `maxGraphQLQueryComplexity` to limit query complexity for performance protection

Open Moumouls opened this issue 1 month ago โ€ข 15 comments

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

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

Summary by CodeRabbit

  • New Features

    • Configurable query complexity limits for GraphQL and REST include queries (depth and fields/count); enforcement blocks excessive requests (including includeAll) and allows master/maintenance-key bypass; GraphQL complexity check runs in request pipeline.
  • Documentation

    • Public options, types, and docs updated to expose new complexity settings and cross-option validation guidance.
  • Tests

    • Comprehensive test suites covering GraphQL and REST complexity rules, fragments/includes, bypass keys, combined constraints, and no-limit scenarios.

โœ๏ธ Tip: You can customize this high-level summary in your review settings.

Moumouls avatar Nov 08 '25 23:11 Moumouls

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

๐Ÿš€ Thanks for opening this pull request!

๐Ÿ“ Walkthrough

Walkthrough

Adds configurable query-complexity limits for GraphQL and REST includes, validates their cross-relations in Config, enforces GraphQL complexity via an Apollo plugin (AST traversal with fragment/cycle handling and auth bypass), blocks/validates REST includes (including includeAll and wildcard), and adds comprehensive tests for both features.

Changes

Cohort / File(s) Summary
Configuration & Types
src/Config.js, src/Options/index.js, src/Options/Definitions.js, src/Options/docs.js, types/Options/index.d.ts
Introduce maxGraphQLQueryComplexity and maxIncludeQueryComplexity option shapes, typings, and docs; add Config.validateQueryComplexityOptions and wire validation into option processing; update public option types.
GraphQL Complexity Validation
src/GraphQL/helpers/queryComplexity.js, src/GraphQL/ParseGraphQLServer.js
Add AST traversal to compute depth and field counts (handles Field, InlineFragment, FragmentSpread and cycle detection); implement createComplexityValidationPlugin(config) and conditionally attach it to ApolloServer; plugin skips checks for master/maintenance auth or when unset and throws GraphQLError on violations.
REST Include Validation
src/RestQuery.js
Block includeAll and wildcard * includes for non-master/maintenance users when configured; validate include count and depth after parsing and throw INVALID_QUERY with explicit messages on violations.
Tests
spec/ParseGraphQLQueryComplexity.spec.js, spec/RestQuery.spec.js
Add comprehensive tests covering GraphQL fields/depth limits (named/inline/cyclic fragments, combined constraints, bypass keys) and REST include count/depth, includeAll/wildcard blocking, and bypass scenarios.

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

  • Areas needing extra attention:
    • correctness and early-exit behavior of AST traversal, fragment resolution, and cycle detection in src/GraphQL/helpers/queryComplexity.js
    • plugin lifecycle hooks and master/maintenance bypass logic in src/GraphQL/ParseGraphQLServer.js
    • REST include parsing, includeAll/wildcard blocking, and exact error messages in src/RestQuery.js
    • cross-option validation in src/Config.js
    • reliability and flakiness risk in new tests (spec/*)

Possibly related issues

  • parse-community/parse-server#9939 โ€” discusses includeAll/blocking behavior for includes; this PR implements includeAll and wildcard blocking under maxIncludeQueryComplexity.

Possibly related PRs

  • parse-community/parse-server#9819 โ€” modifies Apollo Server plugin composition and GraphQL wiring; related to conditional plugin attachment here.
  • parse-community/parse-server#9693 โ€” introduces/adds query-complexity option types and related typings; closely related to the options and type updates in this PR.

Suggested reviewers

  • mtrezza

Pre-merge checks and finishing touches

โŒ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage โš ๏ธ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
โœ… Passed checks (2 passed)
Check name Status Explanation
Title check โœ… Passed The title accurately and concisely describes the main feature: adding Parse Server options for query complexity limits to protect performance.
Description check โœ… Passed The description includes the required issue context and approach, with completed tests and identified pending tasks; follows the template structure with clear objectives.
โœจ Finishing touches
  • [ ] ๐Ÿ“ Generate docstrings
๐Ÿงช Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

โค๏ธ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Nov 08 '25 23:11 coderabbitai[bot]

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

Status Scanner Critical High Medium Low Total (0)
:white_check_mark: Open Source Security 0 0 0 0 0 issues

:computer: Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

parseplatformorg avatar Nov 08 '25 23:11 parseplatformorg

@coderabbitai full review

Moumouls avatar Nov 08 '25 23:11 Moumouls

โœ… Actions performed

Full review triggered.

coderabbitai[bot] avatar Nov 08 '25 23:11 coderabbitai[bot]

Codecov Report

:x: Patch coverage is 86.15385% with 9 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 93.05%. Comparing base (d4c6de0) to head (996bdc2). :warning: Report is 1 commits behind head on alpha.

Files with missing lines Patch % Lines
src/Config.js 33.33% 4 Missing :warning:
src/GraphQL/helpers/queryComplexity.js 90.90% 4 Missing :warning:
src/RestQuery.js 90.90% 1 Missing :warning:
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              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Nov 09 '25 18:11 codecov[bot]

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.

 maxGraphQLQueryComplexity: {
          depth: 50,
          fields: 250,
        },
maxIncludeQueryComplexity: {
          depth: 10,
          count: 50,
        },

Moumouls avatar Nov 09 '25 19:11 Moumouls

This is not a breaking change, correct? We'll make it a breaking change with https://github.com/parse-community/parse-server/issues/9928.

mtrezza avatar Nov 09 '25 19:11 mtrezza

exactly @mtrezza no breaking change currently

Moumouls avatar Nov 09 '25 19:11 Moumouls

@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 ?

Moumouls avatar Nov 22 '25 09:11 Moumouls

I think we could go ahead and merge and add this as a caveat. I've opened https://github.com/parse-community/parse-server/issues/9939 to track the feature improvement. If you could just fix the conflict and https://github.com/parse-community/parse-server/pull/9920#discussion_r2534416752.

mtrezza avatar Nov 22 '25 11:11 mtrezza

it's fixed @mtrezza we will have a merge conflict also between this one and apollo server update

Moumouls avatar Nov 22 '25 14:11 Moumouls

@Moumouls if we set default values at some point with https://github.com/parse-community/parse-server/issues/9928, but the value 0 means no depth is allowed, what should a user set to disable the limits? undefined may be complicated as it's almost the same as not defined, and I don't think we make such nuanced distinctions for other options. It would be good to consider this already in this PR, so we don't have to make large changes later.

Should we define a value < 0 as disabling the options?

maxIncludeQueryComplexity: {
  depth: -1,
  count: -1,
}

mtrezza avatar Nov 24 '25 14:11 mtrezza

@Moumouls Friendly ping so we can merge this.

mtrezza avatar Nov 28 '25 12:11 mtrezza

@Moumouls we're preparing all PS9 breaking change PRs for merge. Although this isn't a breaking change, we may want to consider https://github.com/parse-community/parse-server/pull/9920#issuecomment-3571211062 to make a future introduction of a default limit easier. We can in fact merge this PR with a default limit already for the PS9 release, so we don't have to worry about adding a default in the future. If we can't find a solution here, then we can merge the feature anytime after the PS9 release and add the default with PS10. Maybe @coratgerl wants to take a look at this as well.

mtrezza avatar Dec 12 '25 02:12 mtrezza

I discussed with @Moumouls about a potential improvement, either in this PR or in a future one, depending on the scop to use the validationRules object from Apollo Server for this type of validation.

coratgerl avatar Dec 12 '25 07:12 coratgerl

@coratgerl The main question now is whether we can get the PR ready with a default value and add it as breaking change in PS9 release, or add it without default value after PS9 release. For now I've removed this PR from the required PRs for PS9. If you guys can find a solution for https://github.com/parse-community/parse-server/pull/9920#issuecomment-3571211062 in time then we merge with a default value, otherwise we just disregard the PR for PS9 release and we can merge without default value when https://github.com/parse-community/parse-server/pull/9920#issuecomment-3571211062 is solved.

mtrezza avatar Dec 12 '25 14:12 mtrezza

@coderabbitai could you suggest good default value for new parameters introduced in this PR

Moumouls avatar Dec 12 '25 16:12 Moumouls

[!TIP] For best results, initiate chat on the files or code changes.

@Moumouls Based on the discussions in this PR and the constraint that REST limits must be lower than GraphQL limits, here are my recommendations for default values:

Recommended Default Values

For maxIncludeQueryComplexity (REST API):

maxIncludeQueryComplexity: {
  depth: 10,
  count: 100
}

For maxGraphQLQueryComplexity:

maxGraphQLQueryComplexity: {
  depth: 20,
  fields: 200
}

Rationale:

  1. Conservative but practical: These values align with the illustrative defaults mentioned in the PR description and should accommodate most legitimate use cases while preventing abuse.

  2. Follows the constraint: REST values (depth: 10, count: 100) are lower than GraphQL values (depth: 20, fields: 200), satisfying the validation rules.

  3. Depth rationale:

    • REST depth of 10: Most REST queries with includes rarely need more than 3-5 levels of nesting. A depth of 10 provides a comfortable buffer.
    • GraphQL depth of 20: GraphQL queries can be more complex by nature; 20 levels provides flexibility while preventing deeply nested attacks.
  4. Count/Fields rationale:

    • REST count of 100: Allows for complex include queries with many relations without being unlimited.
    • GraphQL fields of 200: GraphQL queries can select many fields across objects; 200 provides generous limits.
  5. Regarding the -1 convention (addressing @mtrezza's concern): I recommend using -1 to explicitly disable limits when defaults are introduced:

    maxIncludeQueryComplexity: {
      depth: -1,  // disables depth limit
      count: -1   // disables count limit
    }
    

    This is a common convention in many systems and would make the future migration clearer.

Alternative Approach (More Generous):

If you prefer more generous defaults to minimize potential breakage:

maxIncludeQueryComplexity: {
  depth: 10,
  count: 150
}

maxGraphQLQueryComplexity: {
  depth: 50,
  fields: 300
}

These higher values match closer to @Moumouls's earlier suggestion and would be even safer for existing applications while still providing DoS protection.


๐Ÿง  Learnings used
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-12-02T06:55:53.808Z
Learning: When reviewing Parse Server PRs that add or modify Parse Server options, always verify that changes are properly reflected in three files: src/Options/index.js (where changes originate), src/Options/Definitions.js, and src/Options/docs.js. The correct workflow is: make changes in index.js first, then run `npm run definitions` to automatically replicate the changes to Definitions.js and docs.js.

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

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

Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-12-02T08:00:20.138Z
Learning: For Parse Server 9 release (PR #9938 and related), the parse/push-adapter dependency must be upgraded to version >= 8.0.0, not 7.0.0. Version 8.x drops support for Node 18.

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.

coderabbitai[bot] avatar Dec 12 '25 16:12 coderabbitai[bot]

This is the last unmerged PR for Parse Server 9 release. We can merge as breaking, considering https://github.com/parse-community/parse-server/issues/9928. If you run into complexities, no worries, we'll go ahead with PS9 release and merge this at a later point as non-breaking.

mtrezza avatar Dec 12 '25 20:12 mtrezza