🐛 bug: prevent panic when MsgPack is not configured#4268
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughMsgPack binder functions now return exported error ChangesMsgPack Error Handling Refactor
Memory TTL Test Update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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.
🧹 Nitpick comments (1)
binder/msgpack_test.go (1)
92-94: ⚡ Quick winAssert the marshal return payload is
nilalongside the error.You’re already checking
ErrMsgpackNotConfigured; also verifying the[]byteresult isnilwill lock in the full contract and prevent subtle regressions.Suggested test tweak
- _, err := UnimplementedMsgpackMarshal(struct{ Name string }{Name: "test"}) + got, err := UnimplementedMsgpackMarshal(struct{ Name string }{Name: "test"}) + require.Nil(t, got) require.ErrorIs(t, err, ErrMsgpackNotConfigured)🤖 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 `@binder/msgpack_test.go` around lines 92 - 94, The test currently checks only the error from UnimplementedMsgpackMarshal; also assert that the returned []byte payload is nil to enforce the contract: after calling UnimplementedMsgpackMarshal(struct{ Name string }{Name: "test"}) assert err is ErrMsgpackNotConfigured and that the first return value is nil (e.g. using require.Nil or require.Equal(t, nil, payload)) so the test verifies both the error and that no bytes are returned.
🤖 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.
Nitpick comments:
In `@binder/msgpack_test.go`:
- Around line 92-94: The test currently checks only the error from
UnimplementedMsgpackMarshal; also assert that the returned []byte payload is nil
to enforce the contract: after calling UnimplementedMsgpackMarshal(struct{ Name
string }{Name: "test"}) assert err is ErrMsgpackNotConfigured and that the first
return value is nil (e.g. using require.Nil or require.Equal(t, nil, payload))
so the test verifies both the error and that no bytes are returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 15dc1650-64b4-4187-9618-0388c5b8bdc3
📒 Files selected for processing (2)
binder/msgpack.gobinder/msgpack_test.go
gaby
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4268 +/- ##
=======================================
Coverage 91.25% 91.25%
=======================================
Files 130 130
Lines 12753 12753
=======================================
Hits 11638 11638
Misses 702 702
Partials 413 413
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 PR hardens Fiber’s MsgPack integration by replacing the default “unconfigured MsgPack” panic stubs with safe error returns, preventing attacker-controlled Content-Type/Accept headers from crashing the process when MsgPack isn’t configured.
Changes:
- Introduces a new exported sentinel error and returns it from the MsgPack “unimplemented” marshal/unmarshal stubs instead of panicking.
- Updates MsgPack binder tests to assert error behavior rather than panic behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| binder/msgpack.go | Replaces panic-based MsgPack stubs with a typed exported error return. |
| binder/msgpack_test.go | Updates tests to validate the new non-panicking error behavior. |
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/10044016-bb25-46fc-ab97-bf87edb81a33 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
![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 refactors the MsgPack binder to return an error instead of panicking when the marshaler or unmarshaler is not configured. It introduces a new error variable and updates the test suite accordingly. Review feedback recommends renaming the error variable to ErrMsgPackNotConfigured for consistency, prefixing the error message with the package name, and maintaining consistent casing in documentation comments.
Applied in d8d581d: renamed the exported error to ErrMsgPackNotConfigured, updated the binder-prefixed error text, and fixed the MsgPack casing in the related comments. |
|
@copilot === FAIL: internal/memory Test_Memory (1.10s) === FAIL: internal/memory Test_Memory (1.10s) === FAIL: internal/memory Test_Memory (1.10s) |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/867088d4-f242-45b2-93db-4f968aad5e6f Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Fixed in 2fa8169. I replaced the fixed 1.1s sleep in |
Motivation
Content-Type/Acceptheaders select MsgPack paths while the app did not configure MsgPack, by converting panic stubs into safe error returns.Description
ErrMsgpackNotConfiguredand returning it fromUnimplementedMsgpackMarshalandUnimplementedMsgpackUnmarshalinbinder/msgpack.go.binder/msgpack_test.goto assert the new error behavior instead of expecting panics.