Repository navigation
fix: sanitize Content-Disposition attachment filenames - #788
imuniqueshiv merged 5 commits into
Conversation
|
@Ayush-0918 is attempting to deploy a commit to the Shiv Raj Singh's projects Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
Bot
added
the
ECSoC26
|
Hi @Ayush-0918, thank you so much for your PR! 🎉 📋 PR Review Checklist
We appreciate the effort you've put in — let us know if you need any help getting this across the finish line! 💪 |
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughShared filename-sanitization helpers were added and tested, then integrated into meeting, graph snapshot, and transcript export controllers to sanitize ChangesFilename Sanitization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/controllers/transcriptController.js`:
- Around line 734-735: In the setHeader call around sanitizeFilenameForHeader,
remove the stale unsanitized transcript filename literal and ensure the
sanitized filename argument is properly terminated with a comma so the module
parses correctly.
In `@server/tests/fileUtils.test.js`:
- Around line 3-112: Run Prettier on the sanitizeFilenameForHeader test suite in
this file and commit the formatter’s output, including spacing, line wrapping,
and whitespace normalization; do not alter the test behavior or assertions.
In `@server/utils/fileUtils.js`:
- Around line 36-37: Update the filename sanitization chain in fileUtils to
remove only the quote character used to delimit the caller’s filename value,
while preserving apostrophes within names such as “Bob’s Notes.pdf”; narrow the
existing regex in the quote-removal step without changing other filename
processing.
- Around line 31-39: Update sanitizeFilenameForHeader and the
Content-Disposition response-header construction to support non-Latin-1
filenames: generate an ASCII-safe fallback for filename and add
filename*=UTF-8'' with the UTF-8 percent-encoded original name per RFC 8187.
Preserve sanitization of control characters and quotes, and add coverage for
Unicode filenames in the response header path.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 87fc0d14-a639-406c-a3f1-e29683ca90b7
⛔ Files ignored due to path filters (1)
server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
server/controllers/exportController.jsserver/controllers/graphSnapshotController.jsserver/controllers/transcriptController.jsserver/tests/fileUtils.test.jsserver/utils/fileUtils.js
|
@Ayush-0918 Could you please clear all the checks? |
- Fixed malformed setHeader() call in transcriptController.js - Implemented RFC 8187 compliant Content-Disposition headers for Unicode filenames - Preserved single quotes in fileUtils.js sanitization logic - Updated fileUtils.test.js with coverage for Unicode response headers - Formatted modified files with Prettier
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/tests/fileUtils.test.js (1)
136-154: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the composite
getContentDispositionHeadersecurity guarantees.This suite only exercises 3 happy-path cases. Given this function is the actual thing shipped into the response header, it's worth directly testing the properties this PR is meant to guarantee, none of which are currently covered end-to-end:
- A CRLF/header-injection payload (e.g.
"file.pdf\r\nX-Injected: x") round-tripped throughgetContentDispositionHeaderitself, not justsanitizeFilenameForHeader.null/undefinedinput —encodeURIComponent(null)coerces to the literal string"null", sofilename*=UTF-8''nullwould currently be produced.- A filename containing
',(,), or*in thefilename*segment.encodeURIComponentintentionally leaves these characters unescaped, but RFC 5987/8187attr-charexcludes them from the extended-value charset, sogetContentDispositionHeader's use of plainencodeURIComponentforencodedis not fully spec-compliant for these characters.Suggested additional test cases
it("should sanitize the fallback ascii filename", () => { expect(getContentDispositionHeader('bad"name.pdf')).toBe( `attachment; filename="badname.pdf"; filename*=UTF-8''bad%22name.pdf`, ); }); + + it("should not allow CRLF injection through the composite header", () => { + const header = getContentDispositionHeader( + "file.pdf\r\nX-Injected-Header: malicious", + ); + expect(header).not.toMatch(/[\r\n]/); + }); + + it("should handle null/undefined without emitting the literal string", () => { + expect(getContentDispositionHeader(null)).not.toContain("UTF-8''null"); + }); });As per the review-stack context,
server/utils/fileUtils.jsis part of this same layer ("Sanitizer contract and validation"); confirm whetherencodeURIComponent's unescaped' ( ) *characters andencodeURIComponent(null)producing"null"are acceptable for this PR's scope, since MDN's own RFC 5987 reference implementation explicitly patches for this gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/tests/fileUtils.test.js` around lines 136 - 154, Add end-to-end tests for getContentDispositionHeader covering CRLF/header-injection input, null and undefined values, and filenames containing apostrophe, parentheses, or asterisk characters. Verify the complete header output, including sanitized fallback and RFC 5987/8187-compliant filename* encoding; update the implementation used by getContentDispositionHeader if these cases currently produce unsafe or non-compliant output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/tests/fileUtils.test.js`:
- Around line 143-147: Add a CJK or emoji case to the
getContentDispositionHeader tests and verify the filename= fallback remains
ASCII while filename* retains the correctly encoded original name. Update
sanitizeFilenameForHeader or the surrounding getContentDispositionHeader logic
to ASCII-normalize or replace non-Latin-1 characters when necessary, while
preserving existing sanitization and Latin-1 behavior.
---
Nitpick comments:
In `@server/tests/fileUtils.test.js`:
- Around line 136-154: Add end-to-end tests for getContentDispositionHeader
covering CRLF/header-injection input, null and undefined values, and filenames
containing apostrophe, parentheses, or asterisk characters. Verify the complete
header output, including sanitized fallback and RFC 5987/8187-compliant
filename* encoding; update the implementation used by
getContentDispositionHeader if these cases currently produce unsafe or
non-compliant output.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8cd9f6c0-747d-415a-b6dc-e0b61c7728ac
📒 Files selected for processing (3)
server/controllers/transcriptController.jsserver/tests/fileUtils.test.jsserver/utils/fileUtils.js
🚧 Files skipped from review as they are similar to previous changes (2)
- server/utils/fileUtils.js
- server/controllers/transcriptController.js
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Hi @Ayush-0918, thank you so much for this solid contribution! 🎉🚀 Your PR #788 (fix: sanitize Content-Disposition attachment filenames) has been merged. ✅ Related issue: #786 (Sanitize Attachment This update fits nicely into the project and is genuinely appreciated — great work! We'd love to see you contribute again soon. 💙 ⭐ If you're enjoying MeetOnMemory, please consider starring the repository — it really helps us out! |
This branch was successfully deployed
2 participants
Pull Request
Description
This PR improves the security of attachment downloads by sanitizing filenames before they are included in the
Content-Dispositionresponse header. Unsafe characters that could lead to malformed headers or header injection are handled safely, while valid filenames continue to work as expected without changing the existing download behavior.Type of Change
Related Issue
Content-DispositionFilename #786Testing
The following scenarios were verified:
Additional testing performed:
Content-Dispositionheader.Screenshots
N/A
Checklist