Sorry, something went wrong.
|
I will reformat the title to use the proper commit message syntax. |
Sorry, something went wrong.
|
🚀 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. Tip
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. |
Sorry, something went wrong.
✅ 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. |
Sorry, something went wrong.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting. Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds per-class bulk GraphQL mutations (createMany, updateMany, deleteMany) with alias support, input-length/batch-limit validation, ordered per-item results via Promise.allSettled, standardized per-item bulk error payload/type, shared batch-limit helpers, and expanded tests covering schema, controller validation, runtime, and error normalization. Changes
Sequence Diagram
sequenceDiagram
actor Client
participant GraphQL_Server as "GraphQL Server"
participant Validation as "Validation Layer"
participant Parse_Core as "Parse Core / DB"
participant ErrorHandler as "Error Handler"
Client->>GraphQL_Server: Send bulk mutation (createMany/updateMany/deleteMany)
GraphQL_Server->>Validation: assertBulkInputLength / isBatchRequestLimitExceeded
alt Limit exceeded
Validation-->>GraphQL_Server: throw batch limit error
GraphQL_Server-->>Client: Return error response
else Within limit
GraphQL_Server->>Parse_Core: Execute per-item operations (Promise.allSettled)
Parse_Core->>Parse_Core: Run per-item hooks (beforeSave/beforeDelete) and persist
Parse_Core-->>GraphQL_Server: Per-item fulfilled/rejected results
GraphQL_Server->>ErrorHandler: bulkErrorPayloadFromReason for rejections
ErrorHandler-->>GraphQL_Server: Normalized {code,message} items
GraphQL_Server-->>Client: Ordered results array with per-item success/error entries
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches 🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
|
@coderabbitai does all security measures are correctly implemented to prevent abuse, do you see security gaps ? |
Sorry, something went wrong.
|
✅ Actions performed
Full review triggered. |
Sorry, something went wrong.
|
Tip For best results, initiate chat on the files or code changes. Let me examine the code changes in this PR to give you a thorough security assessment. 🏁 Script executed: #!/bin/bash
# Look at the changed files in this PR related to GraphQL batch mutations
fd -e js -e ts . --search-path src/GraphQL | xargs grep -l -i "many\|batch\|createMany\|updateMany\|deleteMany" 2>/dev/null | head -20
echo "---"
# Search for batch limit config
rg -n "batchLimit\|batch_limit\|maxBatch\|max_batch\|BatchSize\|batchSize" --type js --type ts -C 3
echo "---"
# Search for the new mutations
rg -n "createMany\|updateMany\|deleteMany" --type js --type ts -C 5 | head -200
echo "---"
# Look at security policy for batch
rg -n "settled\|allSettled\|Promise.all" --type js --type ts -C 3 | grep -i "batch\|many\|graphql" -A 3 -B 3 | head -100
Length of output: 173 🏁 Script executed: #!/bin/bash
echo "=== parseClassMutations.js ==="
cat src/GraphQL/loaders/parseClassMutations.js
echo ""
echo "=== mutation.js transformer ==="
cat src/GraphQL/transformers/mutation.js
echo ""
echo "=== defaultGraphQLTypes.js (relevant parts) ==="
grep -n "many\|batch\|limit\|Many\|Batch\|Limit" src/GraphQL/loaders/defaultGraphQLTypes.js -A 5 -B 2 | head -150
echo ""
echo "=== parseClassTypes.js (relevant parts) ==="
grep -n "many\|batch\|limit\|Many\|Batch\|Limit" src/GraphQL/loaders/parseClassTypes.js -A 5 -B 2 | head -100
echo ""
echo "=== ParseServer options for batch/graphql ==="
rg -n "graphQLBatch\|batchLimit\|maxBatch\|graphqlBatch\|graphql.*batch\|batch.*graphql\|maxObjects\|objectLimit" --type js --type ts -C 3 | head -100
echo ""
echo "=== ParseServerOptions or Definitions ==="
fd -e js -e ts "Definitions\|Options\|Config" --search-path src | xargs grep -l "graphql\|GraphQL" 2>/dev/null | head -10
echo ""
echo "=== ACL and CLP checks in mutations ==="
grep -n "ACL\|CLP\|classLevelPermissions\|requiresAuthentication\|auth\|masterKey" src/GraphQL/loaders/parseClassMutations.js | head -50
echo ""
echo "=== Rate limiting ==="
rg -n "rateLimit\|rate_limit\|throttle" --type js --type ts -C 3 | head -60
Length of output: 41577 🏁 Script executed: #!/bin/bash
echo "=== batchRequestLimit file ==="
fd "batchRequestLimit" | head -5
fd "batchRequestLimit" | xargs cat 2>/dev/null
echo ""
echo "=== ParseServerOptions - looking for batch-related config ==="
fd "Options" --search-path src | head -10
fd "Definitions" --search-path src | head -5
echo ""
echo "=== OptionsDefinitions or ParseServerOptions ==="
fd -e js -e ts "OptionsDefinitions" | xargs cat 2>/dev/null | grep -A 5 -B 2 -i "batch\|bulk\|graphql" | head -100
echo ""
echo "=== default value for batch limit ==="
rg -n "DEFAULT\|default.*[Ll]imit\|[Ll]imit.*default\|batchRequestLimit\|100\|25\|50" src/batchRequestLimit* 2>/dev/null | head -40
echo ""
echo "=== masterKey bypass for batch limit ==="
rg -n "masterKey\|isMaster\|useMasterKey" src/batchRequestLimit* 2>/dev/null | head -20
echo ""
echo "=== error info leakage - bulkErrorFromReason ==="
grep -n "bulkErrorFromReason\|INTERNAL_SERVER_ERROR\|message\|reason" src/GraphQL/loaders/parseClassMutations.js | head -20
echo ""
echo "=== duplicate IDs check in deleteMany ==="
grep -n "duplicate\|Set\|unique\|dedup" src/GraphQL/loaders/parseClassMutations.js | head -20
echo ""
echo "=== tests for batch mutations ==="
fd -e js -e ts -e spec . | xargs grep -l "createMany\|updateMany\|deleteMany\|batchRequestLimit" 2>/dev/null | head -10
echo ""
echo "=== Test file content for batch security ==="
fd -e js -e spec "batch\|graphql.*many\|many.*graphql\|batchRequest" | head -5
Length of output: 2235 🏁 Script executed: #!/bin/bash
echo "=== OptionsDefinitions batch/requestComplexity section ==="
grep -n "batchRequestLimit\|requestComplexity\|batch" src/Options/Definitions.js | head -40
echo ""
echo "=== RequestComplexity spec ==="
cat spec/RequestComplexity.spec.js | grep -A 20 -B 5 "batch\|graphql\|many\|createMany\|updateMany\|deleteMany" | head -150
echo ""
echo "=== ParseGraphQLServer.spec - batch mutation tests ==="
grep -n "createMany\|updateMany\|deleteMany\|batchRequestLimit\|allowCreateMany\|allowUpdateMany\|allowDeleteMany" spec/ParseGraphQLServer.spec.js | head -60
echo ""
echo "=== Check if batchRequestLimit is in OptionsDefinitions ==="
grep -n "batchRequestLimit\|requestComplexity" src/Options/Definitions.js | head -20
echo ""
echo "=== Check for concurrent connection / memory exhaustion protection ==="
rg -n "concurrency\|semaphore\|throttle\|queue\|chunk\|MAX_CONCURRENT\|CONCURRENCY" --type js --type ts -C 3 | head -60
echo ""
echo "=== Check how errors are surfaced from non-Parse errors in bulkErrorFromReason ==="
grep -n -A 10 "bulkErrorFromReason" src/GraphQL/loaders/parseClassMutations.js | head -30
echo ""
echo "=== Check if there's a max input size / GraphQL depth limiting ==="
rg -n "depthLimit\|depth_limit\|maxDepth\|complexityLimit\|queryDepth\|queryComplexity" --type js --type ts -C 3 | head -40
echo ""
echo "=== Check how allowDeleteMany is defaulted - verify no unintended exposure ==="
grep -n "allowDeleteMany\|allowUpdateMany\|allowCreateMany\|isDeleteManyEnabled\|isUpdateManyEnabled\|isCreateManyEnabled" src/GraphQL/loaders/parseClassMutations.js | head -30
Length of output: 7362 @Moumouls, here is a thorough security assessment of the batch mutations introduced in this PR. ✅ Security Measures That Are Correctly Implemented
🔴 Security Gaps Found1. Default Batch Limit is Unlimited (-1)In src/batchRequestLimit.js: function getBatchRequestLimit(config) {
return config?.requestComplexity?.batchRequestLimit ?? -1;
}
The default is -1 — meaning no limit at all unless the operator explicitly configures one. A single createMany call with 100,000 items is accepted by default. This is a denial-of-service risk (memory, DB connections, CPU). Consider:
2. No Concurrency Cap — All Items Dispatched in ParallelAll three mutations use: const settled = await Promise.allSettled(
fieldsList.map(fieldSet => (async () => { ... })())
);
Even if batchRequestLimit is set to e.g. 100, all 100 operations are fired simultaneously. There is no chunking, semaphore, or concurrency limit. This can:
Consider processing items in chunks (e.g., 10 at a time) or using a concurrency limiter like p-limit. 3. Raw Error Message Leakage in bulkErrorFromReasonconst bulkErrorFromReason = reason => {
if (reason instanceof Parse.Error) {
return { code: reason.code, message: reason.message };
}
const message =
reason && typeof reason.message === 'string' ? reason.message : 'Internal server error';
return { code: Parse.Error.INTERNAL_SERVER_ERROR, message };
};
For non-Parse.Error exceptions (e.g., database adapter errors, network errors, ORM internals), the raw .message string is returned directly to the client. This can expose:
The outer catch uses parseGraphQLSchema.handleError(e) which sanitizes — but errors caught inside Promise.allSettled bypass that sanitizer. The fix should either use a fixed message or re-wrap through the existing sanitizer: // Safer approach
return { code: Parse.Error.INTERNAL_SERVER_ERROR, message: 'Internal server error' };
4. Duplicate IDs Not Deduplicated in deleteManyIn deleteMany, the ids array is processed as-is. Sending duplicate IDs (e.g., ids: ["abc", "abc"]) will:
This is not a critical vulnerability since the second attempt fails gracefully, but it's unexpected behavior that should be documented or guarded against, since it can also be used to probe object existence. 5. deleteMany / updateMany Are Implicitly Enabled When destroy/update Is Enabledconst isDeleteManyEnabled = deleteManyExplicit !== undefined ? deleteManyExplicit : isDestroyEnabled;
const allowDeleteMany = isDeleteManyEnabled && (isDestroyEnabled || deleteManyExplicit === true);
Any operator who already exposed single delete/update mutations automatically opts into bulk delete/update without any action on their part. Given the amplified attack surface of bulk operations, this should arguably be an explicit opt-in (false by default), or the documentation should prominently highlight this behavior. Summary Table
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.
|
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 92.39130% with 14 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## alpha #10341 +/- ##
==========================================
- Coverage 92.55% 92.54% -0.01%
==========================================
Files 192 193 +1
Lines 16517 16692 +175
Branches 229 229
==========================================
+ Hits 15287 15448 +161
- Misses 1210 1224 +14
Partials 20 20
☔ View full report in Codecov by Sentry.
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6) spec/ParseGraphQLServer.spec.js (3)12162-12222: Strengthen the partial-failure create assertion to verify rejected items are not persisted.
This test currently proves the successful item exists, but it doesn’t prove the rejected "FAIL" item was not created.
♻️ Suggested assertion hardening11979-12015: Add a negative-control auth test for batch-limit bypass.
You validate master-key bypass, but a security regression test should also assert that non-master auth still enforces batchRequestLimit (for example JS key/session, and maintenance key if policy says master-only bypass).
🤖 Prompt for AI Agents11793-11835: Consider consolidating duplicated BulkTest bootstrap logic.
Schema reset/recreate is repeated in both beforeEach and reconfigureGraphQLWithBatchLimit2AndOpenClient; centralizing this into one helper would reduce drift and test maintenance cost.
🤖 Prompt for AI Agents37-52: Inconsistent error creation pattern.
The empty-input case uses createSanitizedError (line 39) while the batch-limit exceeded case uses new Parse.Error directly (line 47). Both should likely use the same pattern for consistency. If createSanitizedError is needed for security reasons, it should also be applied to the batch limit error.
♻️ Suggested fix for consistency508-521: Optional: Consider extracting shared result mapping logic.
The result mapping logic (lines 508-521, 667-680, 781-794) is duplicated across all three bulk mutations. A shared helper could reduce duplication:
Also applies to: 667-680, 781-794
🤖 Prompt for AI Agents435-506: Consider adding concurrency control for database operations.
Promise.allSettled executes all operations concurrently without throttling. Even within the batch limit, large batches could overwhelm the database with simultaneous writes. Consider using a concurrency-limited execution pattern (e.g., p-limit or chunked sequential execution) if batch limits can be set high.
Additionally, the inner cloneArgs at line 438 appears redundant since fieldsList is already cloned at line 428. This creates unnecessary object allocations per item.
♻️ Suggested fix to remove redundant cloningFix all unresolved CodeRabbit comments on this PR:
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 106ad4bd-4221-4cf9-a5fe-e8118dec6ca2
📥 CommitsReviewing files that changed from the base of the PR and between 705855c and 7813f36.
📒 Files selected for processing (9)
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3) spec/Utils.spec.js (1)292-323: Add coverage for primitive/non-Error rejection reasons
bulkErrorPayloadFromReason accepts unknown, but tests only cover Parse.Error and native Error. Add at least one primitive/nullish case to prevent regressions in Promise rejection normalization.
Proposed test additions11801-11873: Consolidate duplicated GraphQL reconfigure/client bootstrap helpers.
reconfigureGraphQLWithBatchLimit2AndOpenClient and reconfigureGraphQLWithUnsanitizedErrorsAndOpenClient duplicate most setup/teardown logic. Extract one parameterized helper to reduce drift.
♻️ Proposed refactor12155-12195: Pin deleteMany duplicate-ID behavior with an explicit test.
deleteManyBulkTest tests success and partial failure, but not repeated IDs in the same request. Add one test so behavior is deterministic and protected against probing-style regressions.
🤖 Prompt for AI AgentsFix all unresolved CodeRabbit comments on this PR:
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f03fa34-7ccb-4880-9356-a512ca2ec8e1
📥 CommitsReviewing files that changed from the base of the PR and between 7813f36 and 4196811.
📒 Files selected for processing (4)
Sorry, something went wrong.
There was a problem hiding this comment.
292-349: Consider adding an explicit default-config test for sanitization fallback.
You cover explicit true/false; adding config omitted (undefined) would lock in the default secure behavior and prevent regressions.
➕ Optional test additionConfiguration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7144027-46d7-42b7-9293-84d6e32b144d
📥 CommitsReviewing files that changed from the base of the PR and between 3cc83d9 and 8f24cdb.
📒 Files selected for processing (2)
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1) spec/ParseGraphQLServer.spec.js (1)12323-12428: ⚠️ Potential issue | 🟠 Major
These "plain Error" assertions still miss the generic bulk-error contract.
By asserting SCRIPT_FAILED on Line 12371 and Line 12423, these tests are only locking in the wrapped-hook path. They do not protect the GraphQL response shape for true non-Parse.Error per-item failures, which is the leakage-sensitive branch this PR was missing. Please add a case that actually reaches that branch, or update these expectations if hook-thrown Errors are supposed to surface as INTERNAL_SERVER_ERROR instead.
Based on learnings: For GraphQL bulk mutations in parse-community/parse-server, per-item failures must respect enableSanitizedErrorResponse at the GraphQL layer. When enableSanitizedErrorResponse is true (default), a plain Error thrown from hooks should be returned as { code: INTERNAL_SERVER_ERROR, message: 'Internal server error' }; when false, the original Error.message should be returned.
🤖 Prompt for AI AgentsFix all unresolved CodeRabbit comments on this PR:
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b926f70-2fe0-46a5-9689-02b8e00112cc
📥 CommitsReviewing files that changed from the base of the PR and between 8f24cdb and 8b41f8d.
📒 Files selected for processing (1)
Sorry, something went wrong.
|
@coderabbitai why you do not approve ? |
Sorry, something went wrong.
|
Tip For best results, initiate chat on the files or code changes. @Moumouls, there are two reasons:
If you'd like to proceed with approval after addressing these, post @coderabbitai approve as a top-level comment and I'll take care of it. 🧠 Learnings usedLearnt 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: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2026-03-23T22:11:01.903Z
Learning: In Parse Server PRs (parse-community/parse-server), when the commit/PR type prefix is `fix:`, the PR title must describe the bug (the broken behavior), not the fix (the expected or corrected behavior). For example, instead of "fix: Strip undefined values in ParseServerRESTController to match HTTP mode" (describes the fix), prefer "fix: directAccess update with undefined field values persists them as null in MongoDB" (describes the broken behavior).
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 10341
File: spec/ParseGraphQLServer.spec.js:12265-12321
Timestamp: 2026-03-28T17:36:49.599Z
Learning: In parse-community/parse-server GraphQL bulk mutations, per-item failures must follow enableSanitizedErrorResponse: when true (default), a plain `Error` thrown from hooks is returned as `{ code: INTERNAL_SERVER_ERROR, message: 'Internal server error' }`; when false, the original `Error.message` is returned. Tests should cover both behaviors at the GraphQL layer.
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: 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: mtrezza
Repo: parse-community/parse-server PR: 10320
File: spec/ParseGraphQLServer.spec.js:8661-8666
Timestamp: 2026-03-26T02:21:36.416Z
Learning: In parse-community/parse-server tests, swallowing errors with `.catch(() => {})` during schema cleanup is acceptable when the only expected failure is "class does not exist". Specifically, in spec/ParseGraphQLServer.spec.js beforeEach blocks that call `new Parse.Schema('SomeClass').purge()` and `.delete()`, using `.catch(() => {})` is an intentional pattern to keep test setup/teardown resilient and non-flaky. Do not flag this as an issue in future reviews.
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: 10161
File: src/Adapters/Storage/Postgres/PostgresStorageAdapter.js:1743-1750
Timestamp: 2026-03-09T21:11:01.966Z
Learning: In `src/Adapters/Storage/Postgres/PostgresStorageAdapter.js` (`updateObjectsByQuery`), the nested sub-key `c` (derived from dot-notation originalUpdate keys) is directly interpolated into SQL string literals in the Object Increment path (`->>'${c}'` and `CONCAT('{"${c}":', ...)`). This is a confirmed SQL injection vulnerability tracked as GHSA-gqpp-xgvh-9h7h and will be fixed in a separate dedicated PR. The maintainer (mtrezza) prefers a simpler fix — either single-quote escaping (`c.replace(/'/g, "''")`) or strict key name validation (e.g., `/^[A-Za-z0-9_]+$/`) — over restructuring with `jsonb_build_object`, to minimize SQL structure changes. Do not suggest `jsonb_build_object` for this fix.
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.
|
Sorry, something went wrong.
|
@coderabbitai batchRequestLimit -1 is expected and will be changed in next major parse-server version |
Sorry, something went wrong.
|
✅ Actions performed
Comments resolved and changes approved. |
Sorry, something went wrong.
|
Tip For best results, initiate chat on the files or code changes. @Moumouls, understood — that's consistent with the existing requestComplexity opt-in policy for PS9, where limits become mandatory defaults in PS10. I'll retract the 🔴 High severity flag for the -1 default. (｡•ᴗ•｡)🐇 ✏️ Learnings addedLearnt from: Moumouls
URL: https://github.com/parse-community/parse-server/pull/10341
Timestamp: 2026-03-28T19:24:55.356Z
Learning: In parse-community/parse-server (PR `#10341`), `requestComplexity.batchRequestLimit` intentionally defaults to `-1` (unlimited) in PS9. This is by design: batch request limiting is opt-in in the current major version and will become a hard default (safe non-unlimited value) in PS10, consistent with the same opt-in policy already established for other `requestComplexity` sub-options (e.g., `includeLimit`). Do not flag the `-1` default as a security gap for PS9.
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 usedLearnt from: Moumouls
Repo: parse-community/parse-server PR: 10204
File: src/Security/CheckGroups/CheckGroupServerConfig.js:145-147
Timestamp: 2026-03-14T21:39:34.558Z
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.
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: 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: 0
File: :0-0
Timestamp: 2026-03-23T22:11:01.903Z
Learning: In Parse Server PRs (parse-community/parse-server), when the commit/PR type prefix is `fix:`, the PR title must describe the bug (the broken behavior), not the fix (the expected or corrected behavior). For example, instead of "fix: Strip undefined values in ParseServerRESTController to match HTTP mode" (describes the fix), prefer "fix: directAccess update with undefined field values persists them as null in MongoDB" (describes the broken behavior).
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: 10149
File: src/Adapters/Storage/Mongo/MongoStorageAdapter.js:844-868
Timestamp: 2026-03-09T01:30:19.872Z
Learning: In parse-community/parse-server (PR `#10149`), `ensureAuthDataUniqueness(provider)` on both MongoStorageAdapter and PostgresStorageAdapter is called **only once at server startup** from `DatabaseController.performInitialization()` (src/Controllers/DatabaseController.js line 1867). It is NOT called on any request path. The request-path function is `RestWrite.prototype.ensureUniqueAuthDataId` (src/RestWrite.js), which performs a query-based duplicate check — these are two completely different functions. Memoization/Set caching on `ensureAuthDataUniqueness` is unnecessary; DB-level idempotency (MongoDB error codes 85/86; Postgres `IF NOT EXISTS`) is sufficient.
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 10341
File: spec/ParseGraphQLServer.spec.js:12265-12321
Timestamp: 2026-03-28T17:36:49.599Z
Learning: In parse-community/parse-server GraphQL bulk mutations, per-item failures must follow enableSanitizedErrorResponse: when true (default), a plain `Error` thrown from hooks is returned as `{ code: INTERNAL_SERVER_ERROR, message: 'Internal server error' }`; when false, the original `Error.message` is returned. Tests should cover both behaviors at the GraphQL layer.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 10320
File: spec/ParseGraphQLServer.spec.js:8661-8666
Timestamp: 2026-03-26T02:21:36.416Z
Learning: In parse-community/parse-server tests, swallowing errors with `.catch(() => {})` during schema cleanup is acceptable when the only expected failure is "class does not exist". Specifically, in spec/ParseGraphQLServer.spec.js beforeEach blocks that call `new Parse.Schema('SomeClass').purge()` and `.delete()`, using `.catch(() => {})` is an intentional pattern to keep test setup/teardown resilient and non-flaky. Do not flag this as an issue in future reviews.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 10308
File: src/Adapters/Storage/Postgres/PostgresStorageAdapter.js:1995-1998
Timestamp: 2026-03-24T23:49:11.335Z
Learning: In parse-community/parse-server's `PostgresStorageAdapter.js`, the `find`, `count`, and `distinct` methods all broadly suppress `PostgresMissingColumnError` (code `42703`) and return empty results (`[]` or `0`) rather than throwing. This is intentional design: the storage adapter should not know about specific Parse field semantics (e.g., `_tombstone`), and the pattern is consistent with how `PostgresRelationDoesNotExistError` (missing table → `[]`) is already handled. Do not suggest scoping the `42703` suppression to specific field names or query shapes — that would violate separation of concerns and break future internal fields that lack a physical Postgres column.
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: 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-12-02T08:00:08.440Z
Learning: For Parse Server 9 release preparation, the parse/push-adapter dependency should be upgraded to version >= 8.0.0, not 7.x, as version 8.x is required despite dropping Node 18 support (which aligns with Parse Server 9's removal of EOL Node versions).
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 10082
File: package.json:3-3
Timestamp: 2026-02-28T15:22:57.025Z
Learning: In the parse-community/parse-server repository, manual changes to the npm package version in package.json should never be approved. The package version is managed and bumped automatically by semantic-release during the release workflow.
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.
|
Sorry, something went wrong.
|
Sorry, something went wrong.
Pull Request
Issue
Support batch OPS on GQL api
Approach
Introduce 3 new mutations, with promise all settled strategy and batch request limit security policy
made with Cursor Composer 2
Tasks
Summary by CodeRabbit
New Features
Behavior / Limits
Validation
Tests