🧹 chore: Return error during EncryptCookie failure#3842
Conversation
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 error handling mechanism within the Highlights
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
|
|
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. WalkthroughReplaced panic on cookie encryption failure with propagated error (using errors.Join to combine handler and encryption errors). Added tests to verify encryption error propagation and that encryption errors do not mask downstream errors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant Encryptor
participant ErrorHandler
rect rgb(220, 220, 230)
Note over Middleware,Client: Previous behavior (panic)
Client->>Middleware: Request with response cookie
Middleware->>Encryptor: Encrypt cookie
Encryptor-->>Middleware: encErr
Middleware->>Middleware: panic(encErr)
Middleware-X Client: execution terminated
end
rect rgb(220, 230, 220)
Note over Middleware,ErrorHandler: New behavior (propagate & join)
Client->>Middleware: Request with response cookie
Middleware->>Encryptor: Encrypt cookie
Encryptor-->>Middleware: encErr
Middleware->>ErrorHandler: return errors.Join(handlerErr?, encErr)
ErrorHandler->>Client: HTTP error response (e.g., 418) with combined error info
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ 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 error handling mechanism within the Highlights
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
|
There was a problem hiding this comment.
Pull Request Overview
This PR improves error handling in the encryptcookie middleware by replacing a panic with proper error propagation when cookie encryption fails.
- Replaced
panic(encErr)withreturn encErrin the encryption error path - Added comprehensive test coverage to verify error propagation through Fiber's error handler
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| middleware/encryptcookie/encryptcookie.go | Changed encryption error handling from panic to returning error |
| middleware/encryptcookie/encryptcookie_test.go | Added test to verify encryption errors propagate correctly to error handler |
Comments suppressed due to low confidence (1)
middleware/encryptcookie/encryptcookie.go:54
- When an encryption error occurs (line 45), the middleware returns
encErrbut discards the error fromc.Next()stored inerr(line 34). This means if both the handler and encryption fail, only the encryption error is returned. Consider returning both errors or at least logging the original error before returning the encryption error.
err := c.Next()
// Encrypt response cookies
for key := range c.Response().Header.Cookies() {
keyString := string(key)
if !isDisabled(keyString, cfg.Except) {
cookieValue := fasthttp.Cookie{}
cookieValue.SetKeyBytes(key)
if c.Response().Header.Cookie(&cookieValue) {
encryptedValue, encErr := cfg.Encryptor(keyString, string(cookieValue.Value()), cfg.Key)
if encErr != nil {
return encErr
}
cookieValue.SetValue(encryptedValue)
c.Response().Header.SetCookie(&cookieValue)
}
}
}
return err
There was a problem hiding this comment.
Code Review
This pull request significantly improves error handling within the encryptcookie middleware by replacing a panic with a returned error when cookie encryption fails. This change enhances server stability by preventing crashes. A corresponding test has been added to validate this new behavior. However, the pull request title and description are inaccurate and do not describe the changes made; they should be updated to reflect the work done on the encryptcookie middleware. I have also included a comment regarding a potential for error information to be lost in a specific edge case.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3842 +/- ##
==========================================
- Coverage 92.14% 92.10% -0.05%
==========================================
Files 115 115
Lines 9754 9754
==========================================
- Hits 8988 8984 -4
- Misses 486 490 +4
Partials 280 280
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 is a good refactoring that replaces a panic with a returned error in the encryptcookie middleware, making it more robust. The new test case validates this change effectively.
However, I've identified a potential issue where an error from the handler chain could be hidden by a subsequent cookie encryption error. I've left a comment with a suggested refactoring to address this by prioritizing the handler error. I've also suggested an additional test case to cover this scenario.
Also, the pull request description seems to be from a different change and should be updated to reflect the actual changes in this PR.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
middleware/encryptcookie/encryptcookie.go(1 hunks)middleware/encryptcookie/encryptcookie_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Apply formatting using gofumpt (Make target: format)
Optimize struct field alignment using betteralign (Make target: betteralign)
Modernize code using gopls modernize (Make target: modernize)
Files:
middleware/encryptcookie/encryptcookie.gomiddleware/encryptcookie/encryptcookie_test.go
🧠 Learnings (7)
📓 Common learnings
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-07-01T03:33:22.283Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.
Learnt from: gaby
Repo: gofiber/fiber 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.
Learnt from: gaby
Repo: gofiber/fiber 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.
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
📚 Learning: 2024-07-02T13:29:56.992Z
Learnt from: gaby
Repo: gofiber/fiber 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.
Applied to files:
middleware/encryptcookie/encryptcookie.gomiddleware/encryptcookie/encryptcookie_test.go
📚 Learning: 2024-07-01T03:44:03.672Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
Applied to files:
middleware/encryptcookie/encryptcookie.gomiddleware/encryptcookie/encryptcookie_test.go
📚 Learning: 2024-09-25T15:57:10.221Z
Learnt from: sixcolors
Repo: gofiber/fiber 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.
Applied to files:
middleware/encryptcookie/encryptcookie.go
📚 Learning: 2024-07-01T03:33:22.283Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-07-01T03:33:22.283Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.
Applied to files:
middleware/encryptcookie/encryptcookie_test.go
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
middleware/encryptcookie/encryptcookie_test.go
📚 Learning: 2024-07-26T21:00:12.902Z
Learnt from: sixcolors
Repo: gofiber/fiber 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.
Applied to files:
middleware/encryptcookie/encryptcookie_test.go
🧬 Code graph analysis (1)
middleware/encryptcookie/encryptcookie_test.go (5)
middleware/encryptcookie/utils.go (1)
GenerateKey(104-116)middleware/encryptcookie/encryptcookie.go (1)
New(9-56)app.go (3)
New(521-634)Config(113-424)ErrorHandler(59-59)middleware/encryptcookie/config.go (1)
Config(8-35)constants.go (1)
StatusTeapot(91-91)
🪛 GitHub Actions: golangci-lint
middleware/encryptcookie/encryptcookie_test.go
[error] 114-114: golangci-lint: unused-parameter: parameter 'key' seems to be unused, consider removing or renaming it as _ (revive)
🪛 GitHub Check: lint
middleware/encryptcookie/encryptcookie_test.go
[failure] 114-114:
unused-parameter: parameter 'key' seems to be unused, consider removing or renaming it as _ (revive)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: Compare
🔇 Additional comments (2)
middleware/encryptcookie/encryptcookie.go (1)
45-45: Excellent improvement to error handling.Changing from
panic(encErr)toreturn encErris the correct approach. This enables graceful error handling through Fiber's error handler instead of crashing the application, making the middleware more robust and production-ready.middleware/encryptcookie/encryptcookie_test.go (1)
6-6: LGTM!The
errorsimport is appropriately added for the new test.
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: 34462e6 | Previous: ff04aa0 | Ratio |
|---|---|---|---|
BenchmarkDecodeitem-4_middleware_cache |
343.1 ns/op 201.12 MB/s 0 B/op 0 allocs/op |
186.5 ns/op 369.91 MB/s 0 B/op 0 allocs/op |
1.84 |
BenchmarkDecodeitem-4_middleware_cache - ns/op |
343.1 ns/op |
186.5 ns/op |
1.84 |
This comment was automatically generated by workflow using github-action-benchmark.
Summary