fix: Default ACL overwrites custom ACL on Parse.Object update

[YazanAlkhalil]

Summary

• Fixes the failing test from fix: Default ACL overwrites custom ACL on Parse.Object update #10003 by adding the missing addField: { '*': true } CLP to the test setup
• The test "should not overwrite ACL with defaultACL on update" was failing with Permission denied (code 119) because obj.set('field', 'value') adds a new field to the class, which requires addField CLP permission
• Without explicit addField in the CLP config, it defaults to {} (no access), causing the operation to be rejected before the ACL update logic is even reached

Closes #10003

Context

This PR includes the fix from #10003 (adding !this.query guard to prevent default ACL from overwriting custom ACL on update) along with the test fix. The original PR's code change in src/RestWrite.js is correct — only the test needed adjustment.

Test plan

• should not overwrite ACL with defaultACL on update — now passes
• should allow explicit ACL modification on update — still passes
• support defaultACL in schema — still passes
• All 35 ParseACL specs pass
• All 93 ACL-related tests across the suite pass

Summary by CodeRabbit

• Bug Fixes

  • Default access control lists are now applied only when creating objects and will no longer overwrite existing ACLs during updates, preventing unintended permission changes.
  • Explicit ACL changes made during updates are preserved and applied as specified.

• Tests

  • Added tests verifying ACL preservation on updates and correct behavior when ACLs are explicitly modified.

[parse-github-assistant]

🚀 Thanks for opening this pull request!

[parseplatformorg]

✅ Snyk checks have passed. No issues have been found so far.

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

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

[coderabbitai]

📝 Walkthrough

Walkthrough

Default ACLs are now applied only for create operations (no query present), not on updates. Two tests were added to ensure updates do not overwrite existing ACLs and that explicit ACL changes on update are respected.

Changes

Cohort / File(s)	Summary
ACL Update Behavior spec/ParseACL.spec.js, src/RestWrite.js	Apply default ACLs only when creating objects (no query). Added tests: one ensures defaultACL does not overwrite existing ACL on update; another verifies explicit ACL modifications on update are preserved.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description provides clear context, test plan, and explanation but does not follow the repository's template structure with Issue/Approach/Tasks sections.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Merge Conflict Detection	✅ Passed	✅ No merge conflicts detected when merging into alpha
Title check	✅ Passed	The title describes a bug fix related to default ACL overwriting custom ACL on update, which directly corresponds to the main code change in src/RestWrite.js that restricts default ACL application to create-only operations.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[mtrezza]

@coderabbitai what's the difference between this PR and #10003 where the CI failed? Analyze the diff and whether the fix is valid.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# Fetch PR `#10003` details
echo "=== PR `#10003` Details ==="
gh pr view 10003 --json title,author,body,state,baseRefName,headRefName,url

echo -e "\n=== PR `#10003` Diff ==="
gh pr diff 10003

echo -e "\n=== PR `#10061` Diff ==="
gh pr diff 10061

echo -e "\n=== PR `#10003` Status Checks ==="
gh pr checks 10003

Length of output: 10023

---

@mtrezza, here's the key difference between the two PRs:

Identical Code Changes ✓

Both PRs make the exact same fix to src/RestWrite.js - adding the !this.query && condition to ensure default ACL is only applied on object creation, not updates.

Critical Test Difference

PR #10003 (failed CI):

await schema.updateClass('TestObject', {}, {
  create: { '*': true },
  update: { '*': true },
  // Missing addField permission!
  ACL: { ... }
});

PR #10061 (this PR):

await schema.updateClass('TestObject', {}, {
  create: { '*': true },
  update: { '*': true },
  addField: { '*': true },  // ← Added
  ACL: { ... }
});

Why This Matters

In the first test, the line obj.set('field', 'value') adds a new field to the TestObject class. Without addField: { '*': true } in the Class-Level Permissions:

• The addField permission defaults to {} (no access)
• The operation fails with "Permission denied" (code 119)
• This happens before the ACL-update logic even runs
• So the test never actually exercised the code fix

Validation ✅

The fix in this PR is valid because:

1. The src/RestWrite.js code change is correct - it properly prevents defaultACL from overwriting custom ACL on updates
2. The test now has the proper addField CLP permission to allow adding a new field during the update
3. The second test doesn't need addField because it only modifies the ACL without changing class structure

The CI failures in PR #10003 were caused by incomplete test setup, not incorrect code logic.

---

🧠 Learnings used

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

---

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-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-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-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: 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()`.

---

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.

---

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.

---

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.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.57%. Comparing base (8cfc856) to head (449034d).
⚠️ Report is 2 commits behind head on alpha.

Additional details and impacted files

@@            Coverage Diff             @@
##            alpha   #10061      +/-   ##
==========================================
- Coverage   92.59%   92.57%   -0.02%     
==========================================
  Files         191      191              
  Lines       15603    15603              
  Branches      176      176              
==========================================
- Hits        14447    14445       -2     
- Misses       1144     1146       +2     
  Partials       12       12              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mtrezza]

@coderabbitai check this change for potential security issues, since we're dealing with ACL here.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# Get the relevant section of RestWrite.js around the change
echo "=== RestWrite.js - ACL handling section ==="
rg -A 30 -B 10 "defaultACL from class" src/RestWrite.js

echo -e "\n=== Full test file to understand scenarios ==="
cat spec/ParseACL.spec.js | tail -n 100

echo -e "\n=== Search for other ACL-related security checks in RestWrite.js ==="
rg -n "ACL|permission" src/RestWrite.js | head -n 50

Length of output: 5047

---

🏁 Script executed:

#!/bin/bash

# Check for ACL validation and manipulation protections
echo "=== ACL validation and manipulation checks ==="
rg -n -B 5 -A 10 "validatePermission" src/RestWrite.js | head -n 80

# Check how CLP interacts with ACL
echo -e "\n=== Class-level permissions enforcement ==="
rg -n -B 3 -A 8 "canAddField|canModifyFields|canCreate|canUpdate" src/RestWrite.js | head -n 60

# Check the full runBeforeSaveTrigger context to see when ACL is applied
echo -e "\n=== runBeforeSaveTrigger context (where ACL is applied) ==="
rg -n -B 15 -A 5 "add default ACL" src/RestWrite.js

# Check what happens when ACL is explicitly set to empty or null
echo -e "\n=== Search for empty ACL handling ==="
rg -n "ACL.*null|ACL.*undefined|ACL.*empty" src/RestWrite.js

Length of output: 1328

---

🏁 Script executed:

#!/bin/bash

# Search for permission validation in RestWrite
echo "=== Permission validation methods ==="
rg -n "function.*permission|Permission" src/RestWrite.js | head -n 30

# Look at the write method to understand the flow
echo -e "\n=== RestWrite execute/write flow ==="
sed -n '1,100p' src/RestWrite.js

# Check for ACL-related guards and validations
echo -e "\n=== ACL guards and validations ==="
rg -n -C 5 "INVALID_ACL|Cannot set.*ACL" src/RestWrite.js

# Look for where operations are checked against permissions
echo -e "\n=== Operation permission checks ==="
rg -n "hasClass" src/RestWrite.js | head -n 20

# Check the SchemaController for CLP enforcement
echo -e "\n=== CLP enforcement in SchemaController ==="
rg -l "classLevelPermissions" src/Controllers/ | head -n 5

Length of output: 5061

---

🏁 Script executed:

#!/bin/bash

# Check the permission validation flow in SchemaController
echo "=== SchemaController CLP validation ==="
rg -n -A 10 "validatePermission" src/Controllers/SchemaController.js | head -n 100

# Check RestWrite for permission checks before operations
echo -e "\n=== RestWrite permission check flow ==="
rg -n -B 3 -A 8 "getUserAndRoleACL|validatePermission|hasClass" src/RestWrite.js | head -n 120

# Look at the complete flow where the ACL default is applied
echo -e "\n=== Complete runBeforeSaveTrigger method context ==="
sed -n '240,400p' src/RestWrite.js

# Check if there are any tests that try to remove or manipulate ACL maliciously
echo -e "\n=== Search for ACL removal/manipulation tests ==="
rg -n "ACL.*null|setACL.*null|delete.*ACL" spec/ParseACL.spec.js spec/RestWrite.spec.js 2>/dev/null | head -n 20

Length of output: 8476

---

🏁 Script executed:

#!/bin/bash

# Check how empty ACL is handled in security checks
echo "=== Empty ACL handling in Auth and security ==="
rg -n "Object.keys.*ACL.*length" src/RestWrite.js src/Auth.js 2>/dev/null

# Check if CLP is enforced independently of ACL
echo -e "\n=== Database operation with validation (CLP enforcement) ==="
rg -n -B 5 -A 10 "create.*runOptions.*true|update.*runOptions.*true" src/RestWrite.js | head -n 60

# Look for ACL validation in Auth.js
echo -e "\n=== Auth.js ACL permission checks ==="
rg -n -C 5 "getClassLevelPermissions|testPermissions" src/Auth.js | head -n 80

# Check for objects without ACL protection scenarios
echo -e "\n=== Legacy objects and ACL handling ==="
rg -n -B 3 -A 8 "legacy.*ACL|ACL.*legacy" src/RestWrite.js src/Auth.js 2>/dev/null

Length of output: 896

---

@mtrezza, I've conducted a thorough security analysis of this change. Here's my assessment:

Security Analysis: ✅ No vulnerabilities introduced

What Changed

The fix adds !this.query && to restrict default ACL application to CREATE operations only, preventing it from overwriting custom ACLs on UPDATE operations.

Security Protections Still In Place

1. Class-Level Permissions (CLP) are enforced FIRST

   • Lines 252-273 show validation happens in database.create() / database.update() calls with validation=true
   • CLP enforcement occurs before default ACL logic even runs
   • This is the primary access control layer

2. The !this.query check cannot be bypassed

   • Set during RestWrite construction based on whether a query parameter exists
   • Not user-controllable or manipulable through API calls

3. Existing ACL security guards remain active

   • Line 1152-1153: Prevents ACL on Session objects
   • Line 1505-1506: Blocks invalid ACL structures (*unresolved)
   • Line 1514-1518: Prevents users from locking themselves out
   • Line 505: Objects with empty ACL {} are filtered out (locked)

4. Legacy object handling unchanged

   • Line 501-502: Objects without ACL are explicitly allowed for backwards compatibility

Why This Change Is Secure

Before (buggy behavior):

// Object has ACL: {user123: {read: true, write: true}}
obj.set('field', 'value');
obj.save(); // Default ACL would OVERWRITE the existing ACL ❌

After (fixed behavior):

// Object has ACL: {user123: {read: true, write: true}}
obj.set('field', 'value');
obj.save(); // Existing ACL is PRESERVED ✅

Edge Case Analysis

Q: What if an object exists without an ACL and gets updated?

This could happen for:

• Legacy objects created before default ACL was configured
• Objects where ACL was intentionally not set

A: Not a security issue because:

• CLP (primary access control) is still enforced
• Legacy object handling is intentional Parse Server design
• Administrators should use CLP for class-wide protection, not rely on retroactive ACL application

Conclusion

The fix is security-sound and actually improves security posture by:

• Preventing unintended ACL overwrites on updates
• Preserving explicit ACL configurations set by users
• Maintaining all existing access control mechanisms (CLP, ACL validation, self-lockout prevention)

No action required from a security perspective. The code change correctly implements the intended behavior while maintaining Parse Server's layered security model.

---

🧠 Learnings used

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

---

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.

---

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

[parseplatformorg]

🎉 This change has been released in version 9.3.0-alpha.6