🔥 feat: Add MaxBytes to favicon middleware #4016
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe favicon middleware adds a configurable Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer/Test
participant Middleware as FaviconMiddleware
participant FS as FileSystem / OS
participant Cache as In-memory Cache
Dev->>Middleware: create middleware with Config{MaxBytes}
Middleware->>FS: open favicon file / read from FileSystem
FS-->>Middleware: io.Reader
Middleware->>Middleware: readLimited(reader, MaxBytes)
alt size <= MaxBytes
Middleware->>Cache: store favicon bytes, content-type, len
Middleware-->>Dev: middleware created successfully
else size > MaxBytes
Middleware-->>Dev: panic during setup (size exceeded)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
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 favicon middleware by introducing a configurable maximum size limit for favicon assets. This change addresses potential security and performance issues by preventing the loading of excessively large files, thereby safeguarding against unbounded reads and simplifying the file size validation process. The update streamlines the internal file reading mechanism and ensures that the middleware operates more securely and efficiently. 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. 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
|
gaby
changed the title
![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 effectively addresses a potential security vulnerability by introducing a size limit for favicon assets, preventing unbounded memory allocation. The implementation adds a MaxBytes configuration, a readLimited helper for safe file reading, and updates documentation and tests accordingly. The changes are well-structured and clear. I have one suggestion to refactor a small piece of duplicated code to improve maintainability. Overall, this is a valuable and well-executed enhancement.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4016 +/- ##
==========================================
- Coverage 91.17% 91.14% -0.04%
==========================================
Files 119 119
Lines 10946 10963 +17
==========================================
+ Hits 9980 9992 +12
- Misses 609 613 +4
- Partials 357 358 +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.
Pull request overview
This pull request adds a configurable size limit to the favicon middleware to prevent unbounded reads when loading favicon files from disk, improving safety and resource management.
Changes:
- Added
MaxBytesconfiguration field with a default of 1 MiB (1,048,576 bytes) - Introduced
readLimitedhelper function that enforces the size limit usingio.LimitReader - Replaced direct file read operations with bounded reads that fail fast on oversized files
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| middleware/favicon/config.go | Adds MaxBytes field to Config struct with default value and validation in configDefault |
| middleware/favicon/favicon.go | Implements readLimited helper and refactors file loading to use bounded reads for both FileSystem and os.Open paths |
| middleware/favicon/favicon_test.go | Adds test case to verify panic behavior when file exceeds MaxBytes limit |
| docs/middleware/favicon.md | Documents the new MaxBytes configuration option with type, description, and default value |
| docs/whats_new.md | Adds Favicon section describing the new size limit feature in both the changes overview and migration guide TOC |
![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
🤖 Fix all issues with AI agents
In `@middleware/favicon/favicon_test.go`:
- Around line 62-68: The test error messages contain a typo ("cache" instead of
"catch"); update the t.Error calls in Test_Middleware_Favicon_MaxBytes and the
similar message in Test_Middleware_Favicon_Not_Found to read "should catch
panic" (or equivalent) so the assertion text correctly describes the
expectation; search for the string "should cache panic" in those test functions
and replace it with "should catch panic".
Motivation
Description
MaxBytes int64tofavicon.Configwith a default of1024 * 1024and apply defaulting inconfigDefault.os.Stat/fs.Statpre-checks and directos.ReadFile/io.ReadAlluses with a singlereadLimitedhelper that reads viaio.LimitReaderand returns an error if the file exceedsMaxBytes.Newto always use the bounded reader path whenConfig.Fileis set and remove the extra size-check panics.docs/middleware/favicon.mdto document theMaxBytesoption and add an entry todocs/whats_new.mdnoting the favicon size limit change.