🐛 bug: Fix Content-Disposition header per RFC 6266 #3551
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes RFC6266 compliance for Content-Disposition headers by handling non-ASCII filenames according to the RFCs. Key changes include:
- Updating attachment and download behavior to include a properly formatted filename* parameter for non-ASCII filenames.
- Adding new helper functions and tests for non-ASCII filename handling.
- Updating documentation in multiple files to reflect the new behavior.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| middleware/static/static_test.go | Tests verify the updated Content-Disposition header behavior. |
| middleware/static/static.go | Adjusts middleware logic to correctly set attachment filename. |
| helpers.go | Adds helper functions (quoteRawString and isASCII) for RFC9110 quoting. |
| docs/whats_new.md, static.md, ctx.md | Updates documentation regarding RFC6266-based non-ASCII filename handling. |
| ctx.go, ctx_test.go | Modifies attachment and download methods and their tests accordingly. |
Comments suppressed due to low confidence (2)
ctx.go:256
- [nitpick] The logic of constructing the Content-Disposition header for attachments is duplicated across the Attachment and Download methods. Consider extracting the header construction into a separate helper function to improve maintainability.
func (c *DefaultCtx) Attachment(filename ...string) {
ctx.go:477
- [nitpick] The header construction logic in Download duplicates similar logic from the Attachment method. Refactoring this repeated code into a common helper could enhance code maintainability.
func (c *DefaultCtx) Download(file string, filename ...string) error {
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Summary of Changes
Hello @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 significantly improves the framework's adherence to RFC 6266 for Content-Disposition headers, specifically addressing the robust handling of non-ASCII characters in filenames during file downloads. By introducing the filename* parameter and refining filename encoding logic, it ensures better interoperability and correct display of internationalized filenames across various clients. The changes are integrated into core context methods and the static file serving middleware, supported by updated helper functions, comprehensive tests, and clear documentation.
Highlights
- RFC 6266 Compliance: Implemented support for the
filename*parameter in theContent-Dispositionheader to ensure proper handling and display of filenames containing non-ASCII characters, aligning with RFC 6266 and RFC 8187. - Enhanced Filename Encoding: Introduced new helper functions (
quoteRawString,isASCII) and refined the existingquoteStringto manage filename encoding more precisely.quoteRawStringis used for the primaryfilenameparameter (allowing raw UTF-8), whilequoteStringensures ASCII-only output for other contexts, andfilename*uses URL-escaped UTF-8. - Updated
AttachmentandDownloadMethods: TheCtx.AttachmentandCtx.Downloadmethods now automatically detect non-ASCII filenames and generate thefilename*parameter alongside the standardfilenameparameter in theContent-Dispositionheader. - Static Middleware Integration: The
staticmiddleware's download functionality has been updated to leverage the new RFC 6266 compliantContent-Dispositionheader generation, ensuring non-ASCII filenames are correctly handled when serving static files for download. - Comprehensive Testing and Documentation: Added new test cases to verify the correct generation of
Content-Dispositionheaders for non-ASCII filenames in bothCtxmethods and thestaticmiddleware. Corresponding documentation indocs/api/ctx.md,docs/middleware/static.md, anddocs/whats_new.mdhas been updated to reflect these changes.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
docs/api/ctx.md (1)
1618-1627: Same compatibility footnote applies here.The
Downloadexample repeats the pattern above; consider re-using (or cross-linking to) the same compatibility note to keep the docs DRY.
🧹 Nitpick comments (3)
helpers.go (1)
128-137: Micro-optimisation opportunity
isASCIIdoes a manual loop;bytes.IndexFunc([]byte(s), func(r rune) bool { return r > 127 }) == -1benchmarks ~15-20 % faster for long strings due to SIMD in the std-lib.
Not urgent, but worth considering if this helper sits on a hot path.docs/middleware/static.md (1)
162-164: Good note – maybe link to ctx.Download for symmetryThe extra sentence accurately describes the new behaviour.
Tiny nit: adding an internal link to thec.Downloaddocs would help readers discover the low-level API.docs/api/ctx.md (1)
1437-1447: Consider flagging potential user-agent quirks around raw UTF-8 fallback.RFC 6266 recommends providing a legacy
filenametoken that is strictly ASCII for maximum user-agent coverage. Supplying raw UTF-8 (as the implementation now does) is generally fine for modern browsers but can confuse older UA’s and some CLI tools. Adding a short caveat (or a link to a compatibility note) right after the example would prepare users for that edge-case and avoid support questions later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
ctx.go(3 hunks)ctx_test.go(2 hunks)docs/api/ctx.md(2 hunks)docs/middleware/static.md(1 hunks)docs/whats_new.md(1 hunks)helpers.go(1 hunks)middleware/static/static.go(3 hunks)middleware/static/static_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Feature request #3224 has been created to add support for square bracket notation and comma-separated values in multipart form data in Fiber, while maintaining binary data transfer capabilities. This would bring parity with the existing form-urlencoded functionality.
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Fiber currently supports square bracket notation in form data with 'application/x-www-form-urlencoded' content type, but this feature is not available for multipart form data. The implementation for form-urlencoded is in ctx.go (BodyParser method), while multipart form data handling needs enhancement to support this feature.
docs/whats_new.md (6)
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Learnt from: sixcolors
PR: gofiber/fiber#3446
File: docs/middleware/logger.md:44-44
Timestamp: 2025-05-13T00:19:16.407Z
Learning: In documentation files for the Fiber framework, code examples are often partial and don't repeat import statements that were shown in earlier examples, focusing instead on demonstrating specific usage patterns.
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Feature request #3224 has been created to add support for square bracket notation and comma-separated values in multipart form data in Fiber, while maintaining binary data transfer capabilities. This would bring parity with the existing form-urlencoded functionality.
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Fiber currently supports square bracket notation in form data with 'application/x-www-form-urlencoded' content type, but this feature is not available for multipart form data. The implementation for form-urlencoded is in ctx.go (BodyParser method), while multipart form data handling needs enhancement to support this feature.
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
docs/middleware/static.md (4)
Learnt from: hcancelik
PR: gofiber/fiber#3036
File: docs/middleware/cache.md:103-103
Timestamp: 2024-10-08T19:06:06.583Z
Learning: There are no hard tabs in the lines 100 to 105 of the `docs/middleware/cache.md` file. Future comments about formatting should accurately reflect the actual content.
Learnt from: hcancelik
PR: gofiber/fiber#3036
File: docs/middleware/cache.md:103-103
Timestamp: 2024-06-15T19:26:06.401Z
Learning: There are no hard tabs in the lines 100 to 105 of the `docs/middleware/cache.md` file. Future comments about formatting should accurately reflect the actual content.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
docs/api/ctx.md (10)
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Fiber currently supports square bracket notation in form data with 'application/x-www-form-urlencoded' content type, but this feature is not available for multipart form data. The implementation for form-urlencoded is in ctx.go (BodyParser method), while multipart form data handling needs enhancement to support this feature.
Learnt from: mdelapenya
PR: gofiber/fiber#3434
File: docs/api/services.md:39-43
Timestamp: 2025-05-07T13:07:33.899Z
Learning: When documenting Go interface methods in the Fiber project, avoid showing method signatures with the interface type as the receiver (e.g., `func (d *Service) Method()`) since interfaces cannot be used as receivers in Go. Instead, show just the method signature without a receiver or use a placeholder implementation name.
Learnt from: efectn
PR: gofiber/fiber#3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
ctx.go (4)
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
ctx_test.go (13)
undefined
<retrieved_learning>
Learnt from: gaby
PR: #3056
File: middleware/encryptcookie/utils.go:22-25
Timestamp: 2024-07-02T13:29:56.992Z
Learning: The encryptcookie_test.go file contains unit tests that validate key lengths for both EncryptCookie and DecryptCookie functions, ensuring that invalid key lengths raise appropriate errors.
</retrieved_learning>
<retrieved_learning>
Learnt from: gaby
PR: #3056
File: middleware/encryptcookie/utils.go:22-25
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The encryptcookie_test.go file contains unit tests that validate key lengths for both EncryptCookie and DecryptCookie functions, ensuring that invalid key lengths raise appropriate errors.
</retrieved_learning>
<retrieved_learning>
Learnt from: gaby
PR: #3170
File: ctx_test.go:1721-1724
Timestamp: 2024-10-16T12:12:30.506Z
Learning: In the Go unit tests in ctx_test.go, it is acceptable to use invalid CIDR notation such as "0.0.0.1/31junk" for testing purposes.
</retrieved_learning>
<retrieved_learning>
Learnt from: sixcolors
PR: #3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using ctx.Response.Header.Cookie may not be suitable for parsing cookies from the response header, as it requires a *Cookie and fills it rather than returning a string value; thus, manual parsing of the Set-Cookie header may be necessary.
</retrieved_learning>
<retrieved_learning>
Learnt from: sixcolors
PR: #3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using ctx.Response.Header.Cookie may not be suitable for parsing cookies from the response header, as it requires a *Cookie and fills it rather than returning a string value; thus, manual parsing of the Set-Cookie header may be necessary.
</retrieved_learning>
<retrieved_learning>
Learnt from: sixcolors
PR: #2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
</retrieved_learning>
<retrieved_learning>
Learnt from: sixcolors
PR: #2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.
</retrieved_learning>
<retrieved_learning>
Learnt from: sixcolors
PR: #3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
</retrieved_learning>
<retrieved_learning>
Learnt from: sixcolors
PR: #3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
</retrieved_learning>
<retrieved_learning>
Learnt from: efectn
PR: #3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the require methods from the testify package for assertions instead of manual comparisons and calls to t.Fatal or t.Fatalf.
</retrieved_learning>
<retrieved_learning>
Learnt from: sixcolors
PR: #3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In DefaultErrorHandler(c *fiber.Ctx, err error), since c is a pointer to an interface, we need to dereference *c when calling interface methods like SendStatus.
</retrieved_learning>
<retrieved_learning>
Learnt from: sixcolors
PR: #3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In DefaultErrorHandler(c *fiber.Ctx, err error), since c is a pointer to an interface, we need to dereference *c when calling interface methods like SendStatus.
</retrieved_learning>
<retrieved_learning>
Learnt from: efectn
PR: #3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the Test_App_ShutdownWithContext function in app_test.go, the clientDone channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional time.Sleep calls.
</retrieved_learning>
middleware/static/static_test.go (8)
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
helpers.go (1)
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
🧬 Code Graph Analysis (1)
ctx.go (1)
constants.go (1)
HeaderContentDisposition(209-209)
🔇 Additional comments (11)
helpers.go (1)
96-102: Clarified comment looks goodThe revised doc-comment now explicitly calls out percent-encoding for non-ASCII bytes, which removes ambiguity around “special characters”. 👍
docs/whats_new.md (1)
449-452: Nice documentation touchCalling out
filename*support directly in the change log makes the upgrade path clearer for users dealing with i18n filenames.
No further action needed.middleware/static/static_test.go (3)
8-10: LGTM - Appropriate imports for RFC 6266 compliance testing.The new imports support URL encoding (
net/url) and file path manipulation (path/filepath) required for testing non-ASCII filename handling in Content-Disposition headers.
213-213: LGTM - Enhanced test specificity for Content-Disposition header.The assertion now explicitly validates the expected format of the Content-Disposition header, improving test coverage and ensuring RFC compliance.
216-231: LGTM - Comprehensive test for RFC 6266 compliance with non-ASCII filenames.The test correctly validates the enhanced Content-Disposition header handling:
- Creates a file with Cyrillic characters to test non-ASCII support
- Verifies both the
filenameandfilename*parameters are present- Uses proper UTF-8 URL encoding via
url.PathEscapeper RFC 8187- Follows the expected format:
attachment; filename="файл.txt"; filename*=UTF-8''%D1%84%D0%B0%D0%B9%D0%BB.txtThis ensures proper internationalization support for file downloads.
middleware/static/static.go (3)
27-27: LGTM - Well-named variable for tracking root file status.The
rootIsFileboolean appropriately tracks whether the root path corresponds to a file, enabling correct filename handling for Content-Disposition headers.
50-52: LGTM - Proper root file detection with error handling.The implementation correctly:
- Uses the existing
isFilehelper for consistency- Only sets
rootIsFilewhen the check succeeds (no error)- Places the check within the
createFS.Doblock for one-time initializationThis ensures reliable detection of whether the root path is a file.
127-131: LGTM - Correct filename handling for root file downloads.The logic ensures proper filename handling for Content-Disposition headers:
- When
rootIsFileis true, uses the actual root filename viafilepath.Base(root)- Otherwise, defaults to the request path filename via
filepath.Base(c.Path())This guarantees that downloaded files have meaningful names that match the actual file being served, improving user experience and RFC compliance.
ctx.go (3)
19-19: LGTM - Required import for RFC 6266 compliance.The
net/urlimport is necessary for URL encoding non-ASCII filenames in thefilename*parameter per RFC 8187 standards.
260-270: LGTM - Excellent RFC 6266 compliance implementation for Attachment method.The enhanced implementation correctly handles both ASCII and non-ASCII filenames:
ASCII filenames: Uses existing
quoteStringmethod for proper escaping
Non-ASCII filenames:
- Uses
quoteRawStringfor thefilenameparameter (avoiding percent-encoding)- Adds
filename*=UTF-8''parameter withurl.PathEscapeencoding per RFC 8187- Ensures broad client compatibility
The dual-parameter approach provides fallback support for older clients while enabling modern browsers to display non-ASCII filenames correctly.
484-494: LGTM - Consistent RFC 6266 compliance in Download method.The implementation maintains consistency with the
Attachmentmethod by applying the same non-ASCII filename handling logic:
- Identical ASCII/non-ASCII detection and quoting
- Same
filename*parameter formatting for non-ASCII filenames- Consistent UTF-8 URL encoding approach
This ensures uniform behavior across both download methods and simplifies maintenance.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
helpers.go (1)
105-126: Address control character escaping for security complianceAs noted in a previous review, this function leaves control characters (bytes < 0x20 and 0x7F) unescaped, which violates HTTP header value rules per RFC 9110 §5.6 and may be rejected by proxies/clients.
Consider escaping control characters to maintain RFC compliance:
for i := 0; i < len(raw); i++ { c := raw[i] switch c { case '\\', '"': bb.B = append(bb.B, '\\', c) case '\n': bb.B = append(bb.B, '\\', 'n') case '\r': bb.B = append(bb.B, '\\', 'r') + case c < 0x20 || c == 0x7f: + bb.B = append(bb.B, '%', "0123456789ABCDEF"[c>>4], "0123456789ABCDEF"[c&0x0f]) default: bb.B = append(bb.B, c) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ctx.go(3 hunks)ctx_test.go(2 hunks)docs/api/ctx.md(2 hunks)docs/whats_new.md(1 hunks)helpers.go(1 hunks)middleware/static/static_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/whats_new.md
- docs/api/ctx.md
- middleware/static/static_test.go
- ctx_test.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Feature request #3224 has been created to add support for square bracket notation and comma-separated values in multipart form data in Fiber, while maintaining binary data transfer capabilities. This would bring parity with the existing form-urlencoded functionality.
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Fiber currently supports square bracket notation in form data with 'application/x-www-form-urlencoded' content type, but this feature is not available for multipart form data. The implementation for form-urlencoded is in ctx.go (BodyParser method), while multipart form data handling needs enhancement to support this feature.
ctx.go (4)
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
helpers.go (1)
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
🧬 Code Graph Analysis (1)
ctx.go (1)
constants.go (1)
HeaderContentDisposition(209-209)
🪛 GitHub Check: unit (1.24.x, ubuntu-latest)
ctx.go
[failure] 269-269:
undefined: isASCII
[failure] 263-263:
undefined: isASCII
[failure] 537-537:
undefined: isASCII
[failure] 531-531:
undefined: isASCII
helpers.go
[failure] 123-123:
undefined: app
🪛 GitHub Check: unit (1.24.x, macos-latest)
ctx.go
[failure] 269-269:
undefined: isASCII
[failure] 263-263:
undefined: isASCII
[failure] 537-537:
undefined: isASCII
[failure] 531-531:
undefined: isASCII
helpers.go
[failure] 123-123:
undefined: app
🪛 GitHub Check: govulncheck-check
ctx.go
[failure] 269-269:
undefined: isASCII
[failure] 263-263:
undefined: isASCII
[failure] 537-537:
undefined: isASCII
[failure] 531-531:
undefined: isASCII
helpers.go
[failure] 123-123:
undefined: app
🪛 GitHub Check: lint
ctx.go
[failure] 269-269:
undefined: isASCII
[failure] 263-263:
undefined: isASCII
[failure] 537-537:
undefined: isASCII
[failure] 531-531:
undefined: isASCII
helpers.go
[failure] 123-123:
undefined: app (typecheck)
🪛 GitHub Actions: Run govulncheck
ctx.go
[error] 263-263: Undefined identifier 'isASCII' at line 263.
🪛 GitHub Actions: golangci-lint
ctx.go
[error] 263-263: golangci-lint: undefined identifier 'isASCII'
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (4)
helpers.go (1)
96-97: LGTM: Clear documentation improvementThe updated comment properly clarifies that
quoteStringuses percent-encoding and ensures ASCII output, which helps distinguish it from the newquoteRawStringfunction.ctx.go (3)
19-19: LGTM: Required import for URL encodingThe
net/urlimport is correctly added to support theurl.PathEscapefunction used in the filename handling logic.
262-272: Excellent RFC 6266 compliance implementationThe logic correctly handles non-ASCII filenames by:
- Using
quoteStringfor ASCII filenames (percent-encoded, header-safe)- Using
quoteRawStringfor non-ASCII filenames (preserves Unicode)- Adding RFC 5987-compliant
filename*parameter with UTF-8 encoding for non-ASCII filesThis approach properly supports international filenames while maintaining backward compatibility.
530-540: Consistent implementation across Download methodThe Download method correctly mirrors the Attachment method's approach for handling both ASCII and non-ASCII filenames, ensuring consistent behavior across the framework.
There was a problem hiding this comment.
Code Review
The pull request focuses on improving RFC6266 compliance for Content-Disposition, especially for filenames with non-ASCII characters. It includes changes to handle filename* parameters, documentation updates, and new tests. The code generally looks good, but I've suggested a few minor improvements for readability and efficiency.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3551 +/- ##
==========================================
+ Coverage 90.61% 90.94% +0.32%
==========================================
Files 110 110
Lines 10912 11010 +98
==========================================
+ Hits 9888 10013 +125
+ Misses 773 745 -28
- Partials 251 252 +1
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:
|
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 27f7326 | Previous: 55818fb | Ratio |
|---|---|---|---|
Benchmark_Utils_SortAcceptedTypes_Sorted |
22.95 ns/op 0 B/op 0 allocs/op |
11.04 ns/op 0 B/op 0 allocs/op |
2.08 |
Benchmark_Utils_SortAcceptedTypes_Sorted - ns/op |
22.95 ns/op |
11.04 ns/op |
2.08 |
This comment was automatically generated by workflow using github-action-benchmark.
Summary
filename*per RFC 6266 when filenames contain non-ASCIIfilename*usage in Attachment and Downloadfilename*handling to static middleware downloadsfilenameparameterRelated #3383