fix: File metadata endpoint bypasses beforeFind / afterFind trigger authorization (GHSA-hwx8-q9cg-mqmc)#10106
Conversation
|
I will reformat the title to use the proper commit message syntax. |
|
🚀 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. |
📝 WalkthroughWalkthroughInvokes beforeFind/afterFind triggers around file metadata retrieval, standardizes trigger error handling to return a SCRIPT_FAILED 403, and adds a test verifying a throwing beforeFind blocks the metadata endpoint. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FilesRouter
participant CloudCode
participant Storage
Client->>FilesRouter: GET /files/<filename>/metadata
FilesRouter->>CloudCode: run beforeFind(Parse.File)
alt beforeFind throws
CloudCode-->>FilesRouter: throws error
FilesRouter-->>Client: 403 { code: SCRIPT_FAILED, error: message }
else beforeFind succeeds
FilesRouter->>Storage: getMetadata(filename)
alt metadata retrieval succeeds
Storage-->>FilesRouter: metadata
FilesRouter->>CloudCode: run afterFind(Parse.File, metadata)
CloudCode-->>FilesRouter: (possibly modified) metadata
FilesRouter-->>Client: 200 metadata
else metadata retrieval fails
Storage-->>FilesRouter: failure
FilesRouter-->>Client: 200 {}
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
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: 1
🧹 Nitpick comments (2)
src/Routers/FilesRouter.js (2)
735-740: MissingforceDownloadproperty in afterFind trigger call.For consistency with
getHandler(line 210-211), the afterFind trigger should include theforceDownloadproperty:{ file, forceDownload: false }While this may not be immediately relevant for metadata responses, maintaining consistency ensures predictable behavior if the trigger implementation evolves.
♻️ Suggested fix
await triggers.maybeRunFileTrigger( triggers.Types.afterFind, - { file }, + { file, forceDownload: false }, config, req.auth );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Routers/FilesRouter.js` around lines 735 - 740, The afterFind trigger call to triggers.maybeRunFileTrigger (with triggers.Types.afterFind, { file }, config, req.auth) is missing the forceDownload property; update the payload object passed to maybeRunFileTrigger to include forceDownload: false (i.e. { file, forceDownload: false }) so it matches getHandler's behavior and keeps trigger inputs consistent.
728-733: Consider using the beforeFind trigger result.The
getHandler(lines 186-189) uses the beforeFind trigger result to potentially update the filename:if (triggerResult?.file?._name) { filename = triggerResult?.file?._name; contentType = mime.getType(filename); }Currently, the metadata handler ignores the trigger result. If a beforeFind hook is expected to modify which file's metadata is retrieved, this would be inconsistent behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Routers/FilesRouter.js` around lines 728 - 733, The metadata handler currently calls triggers.maybeRunFileTrigger(triggers.Types.beforeFind, { file }, config, req.auth) but ignores its return; update the metadata route to inspect the returned triggerResult (same pattern used in getHandler) and, if triggerResult?.file?._name exists, set filename = triggerResult.file._name and recompute contentType = mime.getType(filename) before fetching metadata so any beforeFind modifications are honored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Routers/FilesRouter.js`:
- Around line 720-724: The metadata endpoint currently returns HTTP 200 with an
empty JSON when config is missing; change this to match getHandler's behavior by
returning HTTP 403 with an error message (e.g., "Invalid application ID.") when
config is falsy. Locate the config-check block in the metadata handler (the same
area where you saw "if (!config) { res.status(200); res.json({}); return; }")
and replace the response with res.status(403) and a JSON error body aligned to
getHandler's response format so both handlers behave consistently.
---
Nitpick comments:
In `@src/Routers/FilesRouter.js`:
- Around line 735-740: The afterFind trigger call to
triggers.maybeRunFileTrigger (with triggers.Types.afterFind, { file }, config,
req.auth) is missing the forceDownload property; update the payload object
passed to maybeRunFileTrigger to include forceDownload: false (i.e. { file,
forceDownload: false }) so it matches getHandler's behavior and keeps trigger
inputs consistent.
- Around line 728-733: The metadata handler currently calls
triggers.maybeRunFileTrigger(triggers.Types.beforeFind, { file }, config,
req.auth) but ignores its return; update the metadata route to inspect the
returned triggerResult (same pattern used in getHandler) and, if
triggerResult?.file?._name exists, set filename = triggerResult.file._name and
recompute contentType = mime.getType(filename) before fetching metadata so any
beforeFind modifications are honored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6cdf4a76-97a3-4cc0-95bf-71931b7dbde6
📒 Files selected for processing (2)
spec/CloudCode.spec.jssrc/Routers/FilesRouter.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Routers/FilesRouter.js (1)
717-757: PR title suggestion for changelog.The current title
fix: Mqmc-9doesn't convey the change's impact. Consider a more descriptive title:
fix(files): invoke beforeFind and afterFind triggers on file metadata endpointThis follows Angular commit convention and provides a meaningful changelog entry. Based on learnings from mtrezza: "For Parse Server PRs, always suggest an Angular commit convention PR title that would make a meaningful changelog entry for developers."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Routers/FilesRouter.js` around lines 717 - 757, The PR title is not descriptive; rename it to an Angular-style changelog entry such as "fix(files): invoke beforeFind and afterFind triggers on file metadata endpoint" to clearly reflect the change in FilesRouter.metadataHandler where FilesRouter._getFilenameFromParams is used and triggers.maybeRunFileTrigger is invoked before and after filesController.getMetadata; update the branch/commit titles accordingly so the changelog and reviewers see the intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Routers/FilesRouter.js`:
- Around line 728-733: The metadataHandler currently calls await
triggers.maybeRunFileTrigger(triggers.Types.beforeFind, { file }, config,
req.auth) but ignores its return value, causing filename/contentType not to
reflect hook remapping; update metadataHandler to capture the trigger result
(like getHandler does), check triggerResult?.file?._name and if present assign
filename = triggerResult.file._name and recompute contentType via
mime.getType(filename) before using filename to fetch metadata, ensuring
maybeRunFileTrigger's output is applied consistently with getHandler.
---
Nitpick comments:
In `@src/Routers/FilesRouter.js`:
- Around line 717-757: The PR title is not descriptive; rename it to an
Angular-style changelog entry such as "fix(files): invoke beforeFind and
afterFind triggers on file metadata endpoint" to clearly reflect the change in
FilesRouter.metadataHandler where FilesRouter._getFilenameFromParams is used and
triggers.maybeRunFileTrigger is invoked before and after
filesController.getMetadata; update the branch/commit titles accordingly so the
changelog and reviewers see the intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d19c8be-7f2b-4f46-9b4c-9b447f086c6e
📒 Files selected for processing (1)
src/Routers/FilesRouter.js
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Routers/FilesRouter.js (2)
744-749:afterFindreceives stale file object when filename is remapped.If
beforeFindremaps the filename (lines 734-736), thefileobject passed toafterFindstill contains the original name. In contrast,getHandlerrecreates the file object with the updated filename before callingafterFind(line 208).🔧 Suggested fix for consistency
if (triggerResult?.file?._name) { filename = triggerResult.file._name; + file = new Parse.File(filename, { base64: '' }); }Or alternatively, reuse the trigger's file directly:
if (triggerResult?.file?._name) { filename = triggerResult.file._name; + file = triggerResult.file; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Routers/FilesRouter.js` around lines 744 - 749, afterFind is being called with the original stale file object when beforeFind remaps the filename; update the code so maybeRunFileTrigger(triggers.Types.afterFind, ...) receives the remapped file instead of the original. Specifically, after the beforeFind logic (the code that may change filename), either assign the remapped result back to the local file variable or recreate the file object the same way getHandler does and pass that updated object to maybeRunFileTrigger; ensure the symbol passed to maybeRunFileTrigger is the post-remap file used elsewhere.
717-759: Suggested PR title update.The current title
fix: Mqmc-9doesn't convey the change's impact for the changelog. Consider:
fix(files): invoke beforeFind and afterFind triggers for file metadata endpointBased on learnings: For Parse Server PRs, always suggest an Angular commit convention PR title that would make a meaningful changelog entry for developers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Routers/FilesRouter.js` around lines 717 - 759, The PR title is non-descriptive; change it to an Angular-style conventional commit that reflects the code change around file metadata trigger invocation — for example use "fix(files): invoke beforeFind and afterFind triggers for file metadata endpoint"; update the PR title and the related commit message(s) (amend the last commit or create a new commit with that message) so the change involving FilesRouter.metadataHandler and the beforeFind/afterFind trigger calls is clearly represented in the changelog.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Routers/FilesRouter.js`:
- Around line 744-749: afterFind is being called with the original stale file
object when beforeFind remaps the filename; update the code so
maybeRunFileTrigger(triggers.Types.afterFind, ...) receives the remapped file
instead of the original. Specifically, after the beforeFind logic (the code that
may change filename), either assign the remapped result back to the local file
variable or recreate the file object the same way getHandler does and pass that
updated object to maybeRunFileTrigger; ensure the symbol passed to
maybeRunFileTrigger is the post-remap file used elsewhere.
- Around line 717-759: The PR title is non-descriptive; change it to an
Angular-style conventional commit that reflects the code change around file
metadata trigger invocation — for example use "fix(files): invoke beforeFind and
afterFind triggers for file metadata endpoint"; update the PR title and the
related commit message(s) (amend the last commit or create a new commit with
that message) so the change involving FilesRouter.metadataHandler and the
beforeFind/afterFind trigger calls is clearly represented in the changelog.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91bf2d22-4c75-4ffe-84c0-e2c54ea57b26
📒 Files selected for processing (1)
src/Routers/FilesRouter.js
mtrezza
changed the title
beforeFind / afterFind trigger authorization (GHSA-hwx8-q9cg-mqmc)
# [9.5.0-alpha.9](9.5.0-alpha.8...9.5.0-alpha.9) (2026-03-06) ### Bug Fixes * File metadata endpoint bypasses `beforeFind` / `afterFind` trigger authorization ([GHSA-hwx8-q9cg-mqmc](https://github.com/parse-community/parse-server/security/advisories/GHSA-hwx8-q9cg-mqmc)) ([#10106](#10106)) ([72e7707](72e7707))
|
🎉 This change has been released in version 9.5.0-alpha.9 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #10106 +/- ##
=======================================
Coverage 92.67% 92.67%
=======================================
Files 191 191
Lines 15856 15870 +14
Branches 180 180
=======================================
+ Hits 14694 14708 +14
Misses 1150 1150
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Pull Request
Issue
File metadata endpoint bypasses
beforeFind/afterFindtrigger authorization (GHSA-hwx8-q9cg-mqmc)Tasks
Summary by CodeRabbit
Bug Fixes
Tests