🐛 bug: prevent panic when CBOR is not explicitly configured#4269
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReplace panic-based CBOR stubs with an exported sentinel error ChangesCBOR Error Handling
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/cbor_test.go (1)
93-98: ⚡ Quick winPrefer sentinel-based assertions over exact error-string matching.
These tests are less brittle if they assert
errors.Issemantics (require.ErrorIs) againsterrUnimplementedCBOR. Also consider asserting marshal output isnilwhen erroring.Proposed test refinement
func Test_UnimplementedCborMarshal_ReturnsError(t *testing.T) { t.Parallel() - _, err := UnimplementedCborMarshal(struct{ Name string }{Name: "test"}) - require.EqualError(t, err, "must explicitly setup CBOR, please check docs: https://docs.gofiber.io/next/guide/advance-format#cbor") + out, err := UnimplementedCborMarshal(struct{ Name string }{Name: "test"}) + require.ErrorIs(t, err, errUnimplementedCBOR) + require.Nil(t, out) } func Test_UnimplementedCborUnmarshal_ReturnsError(t *testing.T) { t.Parallel() var out any err := UnimplementedCborUnmarshal([]byte{0xa0}, &out) - require.EqualError(t, err, "must explicitly setup CBOR, please check docs: https://docs.gofiber.io/next/guide/advance-format#cbor") + require.ErrorIs(t, err, errUnimplementedCBOR) }Also applies to: 100-106
🤖 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/cbor_test.go` around lines 93 - 98, Replace the brittle exact-string assertion in Test_UnimplementedCborMarshal_ReturnsError by asserting error sentinel semantics: call UnimplementedCborMarshal and use require.ErrorIs(t, err, errUnimplementedCBOR) and also require.Nil(t, out) (or equivalent) to assert the returned marshal output is nil; update the parallel test and any other tests (e.g., the block around lines 100-106) that currently compare exact error strings to use ErrorIs with the errUnimplementedCBOR sentinel instead.
🤖 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/cbor_test.go`:
- Around line 93-98: Replace the brittle exact-string assertion in
Test_UnimplementedCborMarshal_ReturnsError by asserting error sentinel
semantics: call UnimplementedCborMarshal and use require.ErrorIs(t, err,
errUnimplementedCBOR) and also require.Nil(t, out) (or equivalent) to assert the
returned marshal output is nil; update the parallel test and any other tests
(e.g., the block around lines 100-106) that currently compare exact error
strings to use ErrorIs with the errUnimplementedCBOR sentinel instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d080c57c-fbb9-4411-a4f0-9abcb81d9f3b
📒 Files selected for processing (2)
binder/cbor.gobinder/cbor_test.go
There was a problem hiding this comment.
Pull request overview
This PR hardens Fiber’s CBOR binding/encoding defaults by removing panic-only “unimplemented” stubs and replacing them with a sentinel error, preventing attacker-controlled Content-Type: application/cbor from triggering a panic when CBOR isn’t explicitly configured.
Changes:
- Replace
panicinbinder/cbor.gounimplemented CBOR helpers with a sharederrUnimplementedCBORerror return. - Update
binder/cbor_test.goto assert error-return behavior instead of expecting panics.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| binder/cbor.go | Replaces CBOR “unimplemented” panic stubs with an error-return sentinel to avoid request-path panics. |
| binder/cbor_test.go | Updates tests to validate the new error-based behavior for unimplemented CBOR helpers. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4269 +/- ##
==========================================
+ Coverage 91.18% 91.25% +0.06%
==========================================
Files 129 130 +1
Lines 12757 12753 -4
==========================================
+ Hits 11633 11638 +5
+ Misses 709 702 -7
+ Partials 415 413 -2
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 replaces panics with error returns in the CBOR binder when the marshaler or unmarshaler is not configured, updating both the implementation and associated tests. Feedback suggests exporting the sentinel error as ErrUnimplementedCBOR with a "binder: " prefix for consistency and better usability. Additionally, the reviewer recommends using require.ErrorIs in tests for more robust verification and ensuring that the Reset method and binder pool utilize these default error-returning functions instead of nil to prevent potential panics in other parts of the codebase.
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/b797a3c2-a950-4324-bf9a-b4d330a82ee3 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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: e3e538c | Previous: 30b1caa | Ratio |
|---|---|---|---|
Benchmark_NewError (github.com/gofiber/fiber/v3) |
86.25 ns/op 24 B/op 1 allocs/op |
48.96 ns/op 24 B/op 1 allocs/op |
1.76 |
Benchmark_NewError (github.com/gofiber/fiber/v3) - ns/op |
86.25 ns/op |
48.96 ns/op |
1.76 |
Benchmark_Router_Handler_StrictRouting (github.com/gofiber/fiber/v3) |
222.1 ns/op 0 B/op 0 allocs/op |
119.6 ns/op 0 B/op 0 allocs/op |
1.86 |
Benchmark_Router_Handler_StrictRouting (github.com/gofiber/fiber/v3) - ns/op |
222.1 ns/op |
119.6 ns/op |
1.86 |
This comment was automatically generated by workflow using github-action-benchmark.
|
Addressed in c3aa326: updated the CBOR error text to use “set up” and changed the CBOR tests to assert the sentinel error with |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/74fbc4f6-1569-4541-bb44-1583c2fb9688 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
gaby
changed the title
|
@copilot the error should start with "binder: " |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/afa89cd6-2503-4d9d-8bfd-28efb0542d7a Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Motivation
Bind().Bodyreachable panic via an attacker-controlledContent-Type: application/cborheader.Description
binder/cbor.gowith anerrUnimplementedCBORerror and makeUnimplementedCborMarshal/UnimplementedCborUnmarshalreturn that error instead of callingpanic.binder/cbor_test.goto assert error-return behavior for the unimplemented CBOR helpers rather than expecting panics.