Conversation
gaby
requested review from
ReneWerner87,
efectn and
sixcolors
and removed request for
Copilot
Summary of ChangesHello @gaby, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the security and robustness of file serving by implementing comprehensive filename sanitization for Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
WalkthroughAdded internal filename sanitization for response Attachment/Download headers to strip control characters and default empty names to "download". Augmented tests for attachment/download behavior and updated inline docs clarifying Request/Response/Value may return nil after context release. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4070 +/- ##
==========================================
- Coverage 91.08% 90.98% -0.10%
==========================================
Files 119 119
Lines 11221 11254 +33
==========================================
+ Hits 10221 10240 +19
- Misses 633 642 +9
- Partials 367 372 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces important security hardening by sanitizing filenames used in Content-Disposition headers for attachments and downloads, effectively preventing potential header injection vulnerabilities. The implementation is sound and the added tests provide good coverage for the new logic.
I have a few suggestions to further improve the implementation by making the sanitization more comprehensive and reducing code duplication. These changes would enhance robustness and maintainability.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
res.go (1)
330-340:⚠️ Potential issue | 🟠 Major
Downloadshould normalize provided filenames withfilepath.Baseto matchAttachmentbehavior.When a caller passes a display filename to
Download(e.g.,c.Download(file, "../../name.txt")), path separators are preserved in theContent-Dispositionheader. In contrast,Attachment(line 194) callsfilepath.Baseon the filename parameter before sanitization, which strips any path components. ThesanitizeFilenamefunction only removes control characters (\r,\n,\t,\0), not path separators. This inconsistency should be resolved by applyingfilepath.Baseto the explicitly provided filename inDownload, as is done inAttachment.
🧹 Nitpick comments (2)
res.go (2)
180-189: Consider stripping all control characters, not just four specific ones.The current implementation strips
\r,\n,\t, and NUL, which covers header injection. However, other C0 control characters (e.g.,0x01–0x1F) are also invalid/dangerous in filenames and Content-Disposition values. A broader filter would be more defensive:♻️ Suggested broader control character filter
func sanitizeFilename(filename string) string { return strings.Map(func(r rune) rune { - switch r { - case '\r', '\n', '\t', 0: + if r < 0x20 || r == 0x7F { return -1 - default: - return r } + return r }, filename) }This would also strip BEL, ESC, backspace, and other characters that have no legitimate use in filenames.
192-215: Duplicated Content-Disposition header construction betweenAttachmentandDownload.Lines 200–211 and 341–352 are nearly identical: ASCII check → quote → build disposition string → optional
filename*parameter. Consider extracting a shared helper (e.g.,buildContentDisposition(fname string)) to reduce duplication and ensure both paths stay consistent as the logic evolves.Also applies to: 330-354
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@res.go`:
- Around line 196-198: The Attachment code uses filepath.Base(filename[0]) then
sanitizeFilename(fname) but doesn't handle cases where Base returns "." or "/"
(or strings of only slashes), causing invalid Content-Disposition filenames;
update the Attachment handling to check the basename returned from filepath.Base
(and the resulting fname) for ".", "/", "" or strings made only of slashes and
replace them with a safe default like "attachment" or "file" before calling
sanitizeFilename (or sanitize after replacement) so sanitizeFilename is never
given "." or "/" as the filename.
🧹 Nitpick comments (1)
res.go (1)
194-215: Consider extracting the shared Content-Disposition construction logic.Both
Attachment(lines 200–211) andDownload(lines 338–349) contain identical logic for building theContent-Dispositionheader (ASCII check → quote → assembleattachment; filename=…→ optionalfilename*=UTF-8''…). Extracting this into a small helper (e.g.,buildContentDisposition(app *App, fname string)) would eliminate the duplication and reduce the risk of the two paths diverging in the future.♻️ Proposed refactor
+func buildContentDisposition(app *App, fname string) string { + var quoted string + if app.isASCII(fname) { + quoted = app.quoteString(fname) + } else { + quoted = app.quoteRawString(fname) + } + disp := `attachment; filename="` + quoted + `"` + if !app.isASCII(fname) { + disp += `; filename*=UTF-8''` + url.PathEscape(fname) + } + return disp +}Then in
Attachment:fname = sanitizeFilename(fname) r.Type(filepath.Ext(fname)) - app := r.c.app - var quoted string - if app.isASCII(fname) { - quoted = app.quoteString(fname) - } else { - quoted = app.quoteRawString(fname) - } - disp := `attachment; filename="` + quoted + `"` - if !app.isASCII(fname) { - disp += `; filename*=UTF-8''` + url.PathEscape(fname) - } - r.setCanonical(HeaderContentDisposition, disp) + r.setCanonical(HeaderContentDisposition, buildContentDisposition(r.c.app, fname))And similarly in
Download.Also applies to: 330-351
There was a problem hiding this comment.
Pull request overview
This PR hardens Content-Disposition handling for Attachment and Download by stripping control characters from the filename to prevent unsafe header values, and expands tests to validate expected sanitization behavior.
Changes:
- Added
sanitizeFilenameand applied it toDefaultRes.AttachmentandDefaultRes.Download, with a non-empty fallback name. - Updated attachment/download header expectations (notably removing CR/LF/TAB/NUL from encoded output).
- Expanded
ctx_test.gocoverage for spaces, nested paths, and multiple control-character placements; regenerated interface doc comments.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| res.go | Introduces filename sanitization and applies it to attachment/download Content-Disposition generation. |
| ctx_test.go | Adds/updates tests to assert control-character stripping and some additional filename cases. |
| ctx_interface_gen.go | Regenerates/updates interface comments related to released contexts. |
| { | ||
| name: "empty fallback", | ||
| filename: "\r\n\t\x00", | ||
| expected: `attachment; filename="download"`, |
There was a problem hiding this comment.
what is the purpose of default download filename
3 participants
Motivation
Content-Dispositionfilename parameter to avoid header injection and unsafe/ambiguous client dialogs.Description
sanitizeFilenameinres.gowhich strips\r,\n,\t, and NUL usingstrings.Mapand applied it toDefaultRes.AttachmentandDefaultRes.Downloadwith a fallback to"download"when the sanitized name is empty.Attachment/Downloadlogic to sanitize the base filename before determiningContent-Typeand building theContent-Dispositionheader, while preserving ASCII vs. non-ASCII quoting andfilename*behavior.ctx_interface_gen.go(minor interface comment updates) to match current generation output.ctx_test.go: added cases toTest_Ctx_Attachment(spaces and nested paths), extendedTest_Ctx_Attachment_SanitizesFilenameControls(additional control-placement cases), and addedTest_Ctx_Download_SanitizesFilenameControlsto assert sanitized headers and absence of raw control bytes.