fix: RequestComplexity opt in and new defaults#10204
fix: RequestComplexity opt in and new defaults#10204Moumouls wants to merge 4 commits intoparse-community:alphafrom
Conversation
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. |
✅ 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughDeprecates the requestComplexity option by removing its empty-object default (now undefined), adding deprecation entry DEPPS18 (window 9.6.0 → 10.0.0) to Deprecator, changing the security check to skip when unset, and updating tests to expect undefined + a deprecation log while retaining explicit-empty-object behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
DEPRECATIONS.md (1)
24-24: Suggested PR title for changelog quality:fix(security): make requestComplexity opt-in and deprecate future default limitsThis follows the Angular-style convention and makes the impact clearer for release notes.
Based on learnings: For Parse Server PRs, always suggest an Angular-style PR title in the format
type(scope): description.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DEPRECATIONS.md` at line 24, Update the PR title to follow Angular-style convention as suggested: use "fix(security): make requestComplexity opt-in and deprecate future default limits" so release notes are clearer; reference the DEPRECATIONS.md entry and the config option name requestComplexity when updating the PR title and any related changelog entry to ensure consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@DEPRECATIONS.md`:
- Line 24: Update the deprecation table row for DEPPS18 (Config option
`requestComplexity`) by replacing the placeholder "TBD" in the Issue column with
the concrete PR/issue reference for traceability; use the PR number "#10204" (or
the full repository issue/PR URL) so the row reads with that link instead of
"TBD" to ensure DEPPS18 is properly traced to the change.
In `@src/Security/CheckGroups/CheckGroupServerConfig.js`:
- Around line 145-147: The early return when requestComplexity is not present
(the block with "if (!rc) { return; }" referencing the rc variable and
requestComplexity) causes the check to be reported as passed; change this to
explicitly mark the check as skipped or failed instead of returning—e.g., set
the check result state (skip/failed) via the module's reporting API (use the
same result object/manipulator used elsewhere in CheckGroupServerConfig.js) or
call the existing helper like skipCheck/markFailed with a message like
"requestComplexity not configured" so the report does not show this as a
successful check.
---
Nitpick comments:
In `@DEPRECATIONS.md`:
- Line 24: Update the PR title to follow Angular-style convention as suggested:
use "fix(security): make requestComplexity opt-in and deprecate future default
limits" so release notes are clearer; reference the DEPRECATIONS.md entry and
the config option name requestComplexity when updating the PR title and any
related changelog entry to ensure consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d89c64d-4335-4cac-863a-f54f556df31d
📒 Files selected for processing (5)
DEPRECATIONS.mdspec/RequestComplexity.spec.jssrc/Deprecator/Deprecations.jssrc/Options/Definitions.jssrc/Security/CheckGroups/CheckGroupServerConfig.js
| if (!rc) { | ||
| throw 1; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Avoid reporting disabled request-complexity protection as a passed security check.
At Line 146, return causes this check to be marked as success (✅) rather than skipped/failed, so reports can incorrectly imply limits are enabled when requestComplexity is unset.
🔧 Minimal fix at the root cause
check: () => {
const rc = config.requestComplexity;
if (!rc) {
- return;
+ throw 1;
}
const values = [rc.includeDepth, rc.includeCount, rc.subqueryDepth, rc.graphQLDepth, rc.graphQLFields];
if (values.some(v => v === -1)) {
throw 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.
| if (!rc) { | |
| throw 1; | |
| return; | |
| } | |
| check: () => { | |
| const rc = config.requestComplexity; | |
| if (!rc) { | |
| throw 1; | |
| } | |
| const values = [rc.includeDepth, rc.includeCount, rc.subqueryDepth, rc.graphQLDepth, rc.graphQLFields]; | |
| if (values.some(v => v === -1)) { | |
| throw 1; | |
| } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Security/CheckGroups/CheckGroupServerConfig.js` around lines 145 - 147,
The early return when requestComplexity is not present (the block with "if (!rc)
{ return; }" referencing the rc variable and requestComplexity) causes the check
to be reported as passed; change this to explicitly mark the check as skipped or
failed instead of returning—e.g., set the check result state (skip/failed) via
the module's reporting API (use the same result object/manipulator used
elsewhere in CheckGroupServerConfig.js) or call the existing helper like
skipCheck/markFailed with a message like "requestComplexity not configured" so
the report does not show this as a successful check.
There was a problem hiding this comment.
we should authorize the undefined option in PS9 it will be changed to throw on PS10
There was a problem hiding this comment.
@Moumouls, understood — the return is intentional for PS9. Since requestComplexity is opt-in during this transition period, returning early (instead of throwing) correctly avoids flagging the check as failed for operators who haven't configured the option yet. The deprecation entry (DEPPS18) and updated help text handle communicating the upcoming PS10 default. My suggestion to revert to throw 1 was incorrect in this context — apologies for the noise.
✏️ Learnings added
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 10204
File: src/Security/CheckGroups/CheckGroupServerConfig.js:145-147
Timestamp: 2026-03-14T21:39:29.537Z
Learning: In Parse Server's `src/Security/CheckGroups/CheckGroupServerConfig.js`, the "Request complexity limits enabled" security check intentionally uses `return` (not `throw 1`) when `requestComplexity` is `undefined` in PS9. This is by design: `requestComplexity` is opt-in in the current major version and will only become a hard-failing security check (throw 1 when undefined) in PS10. The deprecation DEPPS18 in `src/Deprecator/Deprecations.js` communicates this upcoming default change to operators.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 10130
File: src/RestQuery.js:890-910
Timestamp: 2026-03-07T22:42:53.791Z
Learning: In Parse Server's RestQuery.js, `validateIncludeComplexity` intentionally counts raw `this.include.length` (not deduplicated paths) for the `includeCount` limit. This is by design: the limit caps total request complexity, not unique paths. Deduplicating before the check would weaken the protection. Note: user-supplied include strings are already deduplicated at construction via `pathSet`, but `handleIncludeAll` may add array references with reference-inequality duplicates — that is a pre-existing concern separate from this security check.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 10175
File: src/LiveQuery/ParseLiveQueryServer.ts:919-952
Timestamp: 2026-03-10T18:04:39.708Z
Learning: In Parse Server LiveQuery (`src/LiveQuery/ParseLiveQueryServer.ts`), the `addProtectedFields` method in `DatabaseController` uses **intersection** logic: it computes a protected set for each applicable ACL group (`*`, `userId`, `role:X`) and intersects them. Role entries serve as **exemptions** (they narrow/reduce the protected set for that role), not additions. Consequently, passing `userRoles: []` when building the `auth` object for the protected-fields WHERE-clause check is deliberately more restrictive (not less): without role sets, only the `'*'` set applies, which is the maximum protection. The only correctness gap is a rare role-only config (`{ 'role:X': ['field'] }` with no `'*'` entry), where omitting roles yields an empty intersection and the field is unprotected for the role member — but unprivileged users are unaffected, so this is not a security escalation. The same pattern exists pre-existing in `_filterSensitiveData`. This is intentional and documented with tests.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 10149
File: src/Controllers/DatabaseController.js:1865-1873
Timestamp: 2026-03-08T21:51:35.204Z
Learning: In `src/Controllers/DatabaseController.js` (`performInitialization`), the `ensureAuthDataUniqueness` index creation is intentionally non-fatal (warn-only, no rethrow). Unlike username/email uniqueness which has been enforced since Parse Server's inception, authData uniqueness is new. Existing production databases may have duplicate authData entries caused by the race condition this PR fixes, so crashing on startup would be a breaking change. The application-level `ensureUniqueAuthDataId` check in `RestWrite.js` still runs as a fallback, and the warning log gives operators visibility to resolve duplicates manually before the index can be created successfully.
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-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: 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: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: 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: 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: 10117
File: benchmark/performance.js:713-737
Timestamp: 2026-03-06T21:30:03.253Z
Learning: In Parse Server LiveQuery, use liveQuery: { classNames: [...] } in the server config to specify which classes emit pub/sub events (server-side producers). Use ParseServer.createLiveQueryServer(httpServer, { appId, masterKey, serverURL }) to start the WebSocket server that forwards events to subscribed clients (event consumers); this function does not require a classNames parameter, as it simply matches incoming events against active client subscriptions.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 10117
File: benchmark/performance.js:52-52
Timestamp: 2026-03-06T21:30:10.326Z
Learning: In reviews of JavaScript files, ensure separation of concerns for LiveQuery in Parse Server: do not rely on the LiveQuery server to perform className filtering. The filtering and class event emission should occur in the ParseServer configuration (classNames within liveQuery) or related server logic, while ParseServer.createLiveQueryServer(httpServer, { appId, masterKey, serverURL }) should only establish the WebSocket-based delivery of already-filtered events. Treat classNames filtering as the responsibility of ParseServer, not the LiveQuery server, and verify that LiveQuery code receives pre-filtered events and does not duplicate filtering logic.
|
made with Cursor, composer 1.5 @mtrezza |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #10204 +/- ##
==========================================
- Coverage 92.55% 92.16% -0.39%
==========================================
Files 192 192
Lines 16281 16281
Branches 199 199
==========================================
- Hits 15069 15006 -63
- Misses 1195 1254 +59
- Partials 17 21 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I didn't see your PR and already merged the fixes. I'm also unsure it works like what's suggested in this PR, because we already established a convention that disabled means I've added the deprecation in #10207 with the new defaults you proposed here. In case you wonder, there's now also a new |
3 participants
Pull Request
Issue
Approach
Tasks
Summary by CodeRabbit
Deprecations
Documentation
Tests