fix: Cloud Hooks and Cloud Jobs bypass readOnlyMasterKey write restriction (GHSA-vc89-5g3r-cmhh)

[mtrezza]

Pull Request

• Report security issues confidentially.
• Any contribution is under this license.

Issue

Cloud Hooks and Cloud Jobs bypass readOnlyMasterKey write restriction (GHSA-vc89-5g3r-cmhh).

Tasks

• 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

Bug Fixes

• Strengthened access control for read-only master keys. Attempting to create or modify hooks or run jobs with a read-only master key now properly returns a permission denied error.

Tests

• Added comprehensive test coverage verifying read-only master key access restrictions across hook and job operations.

[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

Adds read-only masterKey access controls to prevent write operations on hooks and cloud jobs. Enforces OPERATION_FORBIDDEN errors for POST/PUT requests while permitting read operations, with comprehensive test coverage validating both forbidden and allowed actions.

Changes

Cohort / File(s)	Summary
Authorization Guard Implementation src/Routers/FunctionsRouter.js, src/Routers/HooksRouter.js	Added read-only masterKey checks in handleCloudJob(), handlePost(), and handlePut() that throw standardized OPERATION_FORBIDDEN errors when isReadOnly is true, preventing write operations under read-only credentials.
Access Control Test Suite spec/rest.spec.js	Added comprehensive test cases for read-only masterKey context covering forbidden POST/PUT operations on hooks and jobs (returning OPERATION_FORBIDDEN with "Permission denied" messages), plus validation that GET operations succeed, totaling 135 new test lines.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	⚠️ Warning	The PR description includes the required Issue section with a specific security advisory reference, includes the Approach section requirements implicitly through code changes, and documents completed tasks (Add tests marked as done). However, the Issue closing field is incomplete and the Approach section lacks an explicit summary of the changes.	Complete the Issue section by replacing 'FILL_THIS_OUT' with the actual issue reference or advisory number, and add an explicit Approach section summarizing the changes (e.g., adding read-only masterKey guards in FunctionsRouter and HooksRouter).

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly describes the main security fix: preventing Cloud Hooks and Cloud Jobs from bypassing the readOnlyMasterKey write restriction, with the associated GHSA security advisory reference.

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

✨ Finishing Touches

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

[coderabbitai]

🧹 Nitpick comments (4)

src/Routers/HooksRouter.js (2)

93-99: Make the forbidden message explicit about delete operations

handlePut blocks both update and delete flows; the log message can be more precise.

Proposed wording tweak

-        "read-only masterKey isn't allowed to modify a hook.",
+        "read-only masterKey isn't allowed to update or delete a hook.",

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Routers/HooksRouter.js` around lines 93 - 99, The current read-only check
in handlePut uses createSanitizedError with a generic message; update the error
message produced when req.auth.isReadOnly is true to explicitly mention that
delete operations (in addition to updates) are forbidden by the read-only
masterKey—locate the check using req.auth.isReadOnly inside the handlePut flow
and change the string passed to createSanitizedError so it clearly states
"read-only masterKey isn't allowed to update or delete a hook" (or similar
explicit wording).

---

4-4: PR title suggestion for a clearer changelog entry

Suggested title: fix(security): block read-only masterKey writes for hooks and cloud jobs

Based on learnings: For Parse Server PRs, always suggest an Angular-style PR title (type(scope): description) that yields a meaningful changelog entry.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Routers/HooksRouter.js` at line 4, Update the pull request title to
follow Angular-style changelog convention by setting it to: "fix(security):
block read-only masterKey writes for hooks and cloud jobs" so the changelog
entry is meaningful and follows project standards.

spec/rest.spec.js (2)

1176-1297: Either assert logger output or remove loggerErrorSpy.calls.reset() in new tests

Right now resets are done repeatedly without assertions in these new cases, which adds noise.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@spec/rest.spec.js` around lines 1176 - 1297, Several new tests call
loggerErrorSpy.calls.reset() without asserting logger behavior; for each test
(e.g. the it blocks "should throw when trying to create a hook function",
"should throw when trying to create a hook trigger", "should throw when trying
to update a hook function", "should throw when trying to delete a hook
function", and "should throw when trying to run a job with readOnlyMasterKey")
either remove the unnecessary loggerErrorSpy.calls.reset() call or replace it
with an assertion that checks expected logger calls (e.g.
expect(loggerErrorSpy.calls.count()).toBe(...) or
expect(loggerErrorSpy).toHaveBeenCalledWith(...)) so the spy reset is not left
unused; update the corresponding test bodies around loggerErrorSpy.calls.reset()
accordingly to keep tests meaningful.

---

1216-1276: Use unique hook names (or cleanup) to avoid cross-test state leakage

The fixed function names can collide on retries/reruns in a non-pristine environment.

Example stabilization tweak

-    await request({
+    const functionName = `readOnlyUpdateTest_${Date.now()}`;
+    await request({
       url: `${Parse.serverURL}/hooks/functions`,
       method: 'POST',
       headers: {
         'X-Parse-Application-Id': Parse.applicationId,
         'X-Parse-Master-Key': Parse.masterKey,
         'Content-Type': 'application/json',
       },
-      body: { functionName: 'readOnlyUpdateTest', url: 'https://example.com/hook' },
+      body: { functionName, url: 'https://example.com/hook' },
     });
@@
-        url: `${Parse.serverURL}/hooks/functions/readOnlyUpdateTest`,
+        url: `${Parse.serverURL}/hooks/functions/${functionName}`,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@spec/rest.spec.js` around lines 1216 - 1276, The tests create hook functions
with fixed names (readOnlyUpdateTest, readOnlyDeleteTest) which can collide
between runs; modify the test to use unique hook names (e.g., append a timestamp
or random suffix) when calling request for creation and when targeting the
PUT/DELETE, or alternatively perform a cleanup step after the test to remove the
created hook using the true master key; update both occurrences of
'readOnlyUpdateTest' and 'readOnlyDeleteTest' in the spec to reference the
generated uniqueName (or call the cleanup request) so each test is isolated and
does not leak state across runs.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@spec/rest.spec.js`:
- Around line 1176-1297: Several new tests call loggerErrorSpy.calls.reset()
without asserting logger behavior; for each test (e.g. the it blocks "should
throw when trying to create a hook function", "should throw when trying to
create a hook trigger", "should throw when trying to update a hook function",
"should throw when trying to delete a hook function", and "should throw when
trying to run a job with readOnlyMasterKey") either remove the unnecessary
loggerErrorSpy.calls.reset() call or replace it with an assertion that checks
expected logger calls (e.g. expect(loggerErrorSpy.calls.count()).toBe(...) or
expect(loggerErrorSpy).toHaveBeenCalledWith(...)) so the spy reset is not left
unused; update the corresponding test bodies around loggerErrorSpy.calls.reset()
accordingly to keep tests meaningful.
- Around line 1216-1276: The tests create hook functions with fixed names
(readOnlyUpdateTest, readOnlyDeleteTest) which can collide between runs; modify
the test to use unique hook names (e.g., append a timestamp or random suffix)
when calling request for creation and when targeting the PUT/DELETE, or
alternatively perform a cleanup step after the test to remove the created hook
using the true master key; update both occurrences of 'readOnlyUpdateTest' and
'readOnlyDeleteTest' in the spec to reference the generated uniqueName (or call
the cleanup request) so each test is isolated and does not leak state across
runs.

In `@src/Routers/HooksRouter.js`:
- Around line 93-99: The current read-only check in handlePut uses
createSanitizedError with a generic message; update the error message produced
when req.auth.isReadOnly is true to explicitly mention that delete operations
(in addition to updates) are forbidden by the read-only masterKey—locate the
check using req.auth.isReadOnly inside the handlePut flow and change the string
passed to createSanitizedError so it clearly states "read-only masterKey isn't
allowed to update or delete a hook" (or similar explicit wording).
- Line 4: Update the pull request title to follow Angular-style changelog
convention by setting it to: "fix(security): block read-only masterKey writes
for hooks and cloud jobs" so the changelog entry is meaningful and follows
project standards.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 23b2291 and 2b6b385.

📒 Files selected for processing (3)

• spec/rest.spec.js
• src/Routers/FunctionsRouter.js
• src/Routers/HooksRouter.js

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.61%. Comparing base (bebf2fd) to head (2b6b385).
⚠️ Report is 3 commits behind head on alpha.

Additional details and impacted files

@@           Coverage Diff           @@
##            alpha   #10088   +/-   ##
=======================================
  Coverage   92.61%   92.61%           
=======================================
  Files         191      191           
  Lines       15776    15784    +8     
  Branches      180      180           
=======================================
+ Hits        14611    14619    +8     
  Misses       1153     1153           
  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]

fix: Cloud Hooks and Cloud Jobs bypass readOnlyMasterKey write restriction (GHSA-vc89-5g3r-cmhh)

[parseplatformorg]

🎉 This change has been released in version 9.4.1-alpha.3

[parseplatformorg]

🎉 This change has been released in version 9.4.1