🐛 bug: apply limits to msgp serialization#4065
Conversation
WalkthroughThis PR adds MessagePack serialization size limit enforcement across middleware components (cache, idempotency, session, redirect) by introducing per-field msgp limits, explicit bound checks during encode/decode, and metadata fields to track cache properties. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
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 refines the 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
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4065 +/- ##
==========================================
- Coverage 91.10% 91.05% -0.06%
==========================================
Files 119 119
Lines 11203 11207 +4
==========================================
- Hits 10207 10205 -2
- Misses 630 635 +5
- Partials 366 367 +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.
Code Review
This pull request aims to improve security by adding and adjusting msgp serialization limits. The changes are generally positive, especially the move to relax limits on large body fields while tightening them on smaller metadata fields. However, the process of regenerating the msgp code appears to have introduced several significant inconsistencies, likely due to bugs in the generator tool. These include mismatched size limits between encoding and decoding paths for the same fields, and a case where a limit is applied to a field within a struct that is ignored by msgp, rendering the limit ineffective. I've left specific comments on these issues. Addressing them is crucial to ensure the serialization is as robust and secure as intended. The updates to the benchmark tests are a welcome improvement.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
middleware/cache/manager_msgp.go (1)
406-414:⚠️ Potential issue | 🟠 MajorLimit check occurs after
WriteArrayHeader— writer left in corrupt state on violation.
en.WriteArrayHeaderat Line 406 has already flushed the array header to the stream before the limit is validated at Line 411. If the limit is exceeded, the writer contains a partial/malformed message (array header promising N elements with none following).The check should happen before the write:
Proposed fix
+ if uint32(len(z.headers)) > zc13959a0limitArrays { + err = msgp.ErrLimitExceeded + return + } err = en.WriteArrayHeader(uint32(len(z.headers))) if err != nil { err = msgp.WrapError(err, "headers") return } - if uint32(len(z.headers)) > zc13959a0limitArrays { - err = msgp.ErrLimitExceeded - return - }Since this is generated code, the fix likely needs to be applied to the msgp generator directives or templates, then regenerated.
middleware/idempotency/response_msgp.go (1)
120-128:⚠️ Potential issue | 🟠 MajorSame write-before-check pattern as in the cache file —
EncodeMsgwrites headers before validating limits.At Line 120,
WriteMapHeaderflushes the Headers map header to the stream. The limit check at Line 125 fires after the write. Similarly,WriteArrayHeaderat Line 135 precedes its limit check at Line 140. On limit violation, the writer is left with a partial/corrupt message.Proposed fix — move checks before writes
+ if uint32(len(z.Headers)) > zd44e2ac8limitMaps { + err = msgp.ErrLimitExceeded + return + } err = en.WriteMapHeader(uint32(len(z.Headers))) if err != nil { err = msgp.WrapError(err, "Headers") return } - if uint32(len(z.Headers)) > zd44e2ac8limitMaps { - err = msgp.ErrLimitExceeded - return - }And similarly for the inner array header:
+ if uint32(len(za0002)) > zd44e2ac8limitArrays { + err = msgp.ErrLimitExceeded + return + } err = en.WriteArrayHeader(uint32(len(za0002))) if err != nil { err = msgp.WrapError(err, "Headers", za0001) return } - if uint32(len(za0002)) > zd44e2ac8limitArrays { - err = msgp.ErrLimitExceeded - return - }As with the cache file, this needs to be fixed in the generator since these are generated files.
Also applies to: 135-143
🤖 Fix all issues with AI agents
In `@middleware/cache/manager_msgp.go`:
- Around line 607-610: In MarshalMsg, the code calls msgp.AppendArrayHeader(o,
uint32(len(z.headers))) before checking the array-size limit
zc13959a0limitArrays, which writes into the buffer (wasting Require's
allocation) and is inconsistent with decode-path checks; move the limit check to
before the AppendArrayHeader call (i.e., validate uint32(len(z.headers)) <=
zc13959a0limitArrays and return msgp.ErrLimitExceeded if not) and only then call
msgp.AppendArrayHeader to append the header; apply the same change pattern
wherever AppendArrayHeader is used with similar generated limit constants.
In `@middleware/idempotency/response_msgp.go`:
- Around line 181-190: In MarshalMsg, the code appends map/array headers before
checking limits; move the checks so that you validate len(z.Headers) against
zd44e2ac8limitMaps and len(za0002) against zd44e2ac8limitArrays before calling
msgp.AppendMapHeader and msgp.AppendArrayHeader respectively (i.e., check limits
first, return msgp.ErrLimitExceeded if exceeded, then append headers and
continue iterating over z.Headers in MarshalMsg).
- Around line 9-13: The generated msgp array limit (zd44e2ac8limitArrays =
16384) enforces a 16KB cap that contradicts the Body comment and the cache
middleware; either remove the msgp limit or align it with cache
(zd44e2ac8limitArrays = 65535) so storage-policy bounds are respected. Locate
the auto-generated msgp limit constant (zd44e2ac8limitArrays) and the
corresponding msgp directive in response_msgp.go and either delete the limit
directive/constant entirely (so msgp doesn't enforce a fixed ReadBytesLimit) or
change the constant value to 65535 to match the cache middleware, then
regenerate or adjust the generated code so the Body field remains bounded only
by storage policy.
In `@redirect_msgp.go`:
- Around line 229-247: The EncodeMsg function writes the array header before
checking the slice-length limit (zc920acdalimitArrays), which can leave the
writer in a partial/invalid state on error; move the limit check (uint32(len(z))
> zc920acdalimitArrays) to before the call to en.WriteArrayHeader so the
function returns msgp.ErrLimitExceeded without writing anything, keeping the
existing error wrapping behavior for individual element EncodeMsg calls and
matching the MarshalMsg pattern.
There was a problem hiding this comment.
Pull request overview
This PR updates msgp serialization/deserialization limits across Fiber’s cookie redirect messages, cache entries, and idempotency response payloads to better bound memory usage during decoding while removing/relaxing limits for large bodies that are governed by storage policy. It also regenerates msgp outputs accordingly and updates generated benchmarks.
Changes:
- Add msgp limit directives/struct tags (e.g., redirect arrays/maps; cache header/body related fields; idempotency header counts) and regenerate
*_msgp.gocode to enforce them during decode/unmarshal. - Remove/avoid msgp body caps for cache and idempotency bodies, relying on storage policy limits instead.
- Update generated benchmark loops and add targeted linter suppressions for msgp tags on unexported fields.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| redirect_msgp_test.go | Updates generated benchmarks to use b.ResetTimer() and explicit for i := 0; i < b.N; i++ loops. |
| redirect_msgp.go | Regenerated msgp code; introduces array/map size limits and adds marshal-side enforcement for redirect cookie payloads. |
| redirect.go | Adds msgp limit directive for redirect cookie message arrays/maps and documents rationale. |
| middleware/session/data.go | Adds documentation/comments about session size expectations. |
| middleware/idempotency/response_msgp.go | Regenerated msgp code; enforces header map/array count limits during decode/unmarshal. |
| middleware/idempotency/response.go | Adds limit=1024 to header field tag; clarifies body limit policy in comments. |
| middleware/cache/manager_msgp.go | Regenerated msgp code; uses ReadBytesLimit/ReadBytesHeader and explicit limit checks for cached header/value and related fields. |
| middleware/cache/manager.go | Adds msgp limit tags to unexported fields and //nolint:revive annotations to satisfy linting. |
|
@gaby can you check ai comments |
|
@copilot Should we add msgp limits anywhere else? |
Motivation
Description
item.bodyand idempotencyresponse.Body, deferring body-size enforcement to storage policy while retaining field-level limits for other fields.Headersand header counts) to1024where appropriate and tightened per-field byte limits (forctype,cencoding,cacheControl,expires,etag, and header key/value sizes).*_msgp.go) so they include size-constant checks and useReadBytesLimit/ReadBytesHeader/ReadBytesvariants and explicitmsgp.ErrLimitExceededchecks.//nolint:reviveannotations where msgp tags on unexported fields require linter exceptions.