fix: Streaming file download bypasses afterFind file trigger authorization (GHSA-hpm8-9qx6-jvwv)#10361
Conversation
|
🚀 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. |
✅ 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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds per-request auth resolution for file requests and ensures Parse.File Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FilesRouter
participant Auth
participant Triggers
participant FilesController
Client->>FilesRouter: GET /files/:id (Range?, X-Parse-Session-Token)
FilesRouter->>Auth: _resolveAuth(req, config)
Auth-->>FilesRouter: fileAuth (user session or null)
FilesRouter->>Triggers: run afterFind(Parse.File, fileAuth)
alt afterFind denies access
Triggers-->>FilesRouter: throw OPERATION_FORBIDDEN / error
FilesRouter-->>Client: 403 Forbidden
else afterFind allows / modifies response
Triggers-->>FilesRouter: { responseHeaders, forceDownload? }
FilesRouter->>FilesController: handleFileStream(fileId, headers, range)
FilesController-->>Client: 206 Partial Content or 200 OK (stream)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #10361 +/- ##
==========================================
- Coverage 92.52% 92.52% -0.01%
==========================================
Files 192 192
Lines 16554 16566 +12
Branches 231 231
==========================================
+ Hits 15316 15327 +11
- Misses 1216 1217 +1
Partials 22 22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Routers/FilesRouter.js (1)
211-225:⚠️ Potential issue | 🟠 MajorThe ranged-download path still drops the
afterFindresult.This now enforces validators, but the hook result is discarded. At minimum,
forceDownloadandresponseHeadersmutations are ignored on streamed downloads, so the ranged path still behaves differently from the buffered branch.Based on learnings: In parse-community/parse-server, the `responseHeaders` object exposed to `afterFind` file hooks (Parse.Cloud.afterFind(Parse.File, ...)) is intentionally fully mutable by the server administrator.🔧 Apply the hook result before starting the stream
if (isFileStreamable(req, filesController)) { - await triggers.maybeRunFileTrigger( + const afterFind = await triggers.maybeRunFileTrigger( triggers.Types.afterFind, { file, forceDownload: false, responseHeaders: { ...defaultResponseHeaders } }, config, fileAuth ); - for (const [key, value] of Object.entries(defaultResponseHeaders)) { + if (afterFind?.file?._name && afterFind.file._name !== filename) { + filename = afterFind.file._name; + contentType = mime.getType(filename); + } + if (afterFind?.forceDownload) { + res.set('Content-Disposition', `attachment;filename=${afterFind.file._name}`); + } + for (const [key, value] of Object.entries(afterFind?.responseHeaders ?? defaultResponseHeaders)) { res.set(key, value); } filesController.handleFileStream(config, filename, req, res, contentType).catch(() => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Routers/FilesRouter.js` around lines 211 - 225, The ranged-download branch currently calls triggers.maybeRunFileTrigger but ignores its result; change to await and capture the result of triggers.maybeRunFileTrigger (called with triggers.Types.afterFind, { file, forceDownload: false, responseHeaders: { ...defaultResponseHeaders } }, config, fileAuth), then apply any mutations from the returned object—merge/overwrite res headers from result.responseHeaders and use result.forceDownload when invoking filesController.handleFileStream; finally call filesController.handleFileStream(config, filename, req, res, contentType) (or pass a param to enforce download if handleFileStream supports it) after applying headers and keep the existing .catch error handling.
🤖 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 124-137: The helper static method _resolveAuth currently swallows
all exceptions from auth.getAuthForSessionToken and returns null, which hides
invalid/expired token errors; change it so that only a missing session token
returns null and any errors thrown by auth.getAuthForSessionToken are not
downgraded — either rethrow the caught error or propagate it (do not return null
on catch). Update static async _resolveAuth to call auth.getAuthForSessionToken
and let its errors bubble up (or rethrow them) so callers can surface
authentication failures instead of treating them as anonymous access.
---
Outside diff comments:
In `@src/Routers/FilesRouter.js`:
- Around line 211-225: The ranged-download branch currently calls
triggers.maybeRunFileTrigger but ignores its result; change to await and capture
the result of triggers.maybeRunFileTrigger (called with
triggers.Types.afterFind, { file, forceDownload: false, responseHeaders: {
...defaultResponseHeaders } }, config, fileAuth), then apply any mutations from
the returned object—merge/overwrite res headers from result.responseHeaders and
use result.forceDownload when invoking filesController.handleFileStream; finally
call filesController.handleFileStream(config, filename, req, res, contentType)
(or pass a param to enforce download if handleFileStream supports it) after
applying headers and keep the existing .catch error handling.
🪄 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: 91d39f66-9744-4560-a4af-ffd3e53bfe21
📒 Files selected for processing (2)
spec/vulnerabilities.spec.jssrc/Routers/FilesRouter.js
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
mtrezza
changed the title
mtrezza
force-pushed
the
fix/GHSA-hpm8-9qx6-jvwv-v9
branch
from
71b24f2 to
f79d7ed
Compare
## [9.7.1-alpha.1](9.7.0...9.7.1-alpha.1) (2026-03-30) ### Bug Fixes * Streaming file download bypasses afterFind file trigger authorization ([GHSA-hpm8-9qx6-jvwv](GHSA-hpm8-9qx6-jvwv)) ([#10361](#10361)) ([a0b0c69](a0b0c69))
|
🎉 This change has been released in version 9.7.1-alpha.1 |
2 participants
Issue
Streaming file download bypasses afterFind file trigger authorization (GHSA-hpm8-9qx6-jvwv)
Summary by CodeRabbit
Bug Fixes
Tests