fix: improve cookie decryption handling by deleting invalid cookies after iteration#3988
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. WalkthroughDefers deletion of cookies that fail decryption by collecting their keys during iteration and removing them after the loop; adds a test that verifies mixed valid/invalid encrypted-cookie handling. Also adds/adjusts tests for idempotency locker and session behavior to avoid races and assert post-reset session state. Changes
Sequence Diagram(s)(Skipped — changes are a localized middleware iteration-safety fix and test updates; no new multi-component control flow to visualize.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 @ReneWerner87, 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 addresses a potential issue in 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. 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
|
![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 correctly addresses a potential panic that could occur when modifying cookies while iterating over them. By collecting invalid cookies and deleting them after the loop, you've made the decryption process more robust. The addition of a dedicated test case for mixed valid and invalid cookies is a great way to ensure this fix works as expected and prevents future regressions. I have one minor suggestion for a potential performance optimization.
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the encrypt cookie middleware where deleting invalid cookies during iteration could cause issues with map modification. The fix defers cookie deletion until after the iteration is complete by collecting cookies to delete in a slice first.
Key Changes:
- Modified the cookie decryption logic to collect invalid cookies during iteration and delete them afterwards
- Added a test case to verify correct handling of mixed valid and invalid cookies
- Minor optimization by reusing the
keyStringvariable instead of repeated string conversions
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| middleware/encryptcookie/encryptcookie.go | Implements deferred deletion pattern for invalid cookies to avoid mutation during iteration; adds slice to collect cookies for deletion and processes them after the loop |
| middleware/encryptcookie/encryptcookie_test.go | Adds comprehensive test for mixed valid/invalid cookie handling; includes minor import reordering for consistency |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3988 +/- ##
==========================================
- Coverage 91.60% 90.96% -0.65%
==========================================
Files 119 119
Lines 10262 10785 +523
==========================================
+ Hits 9401 9811 +410
- Misses 544 617 +73
- Partials 317 357 +40
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
middleware/idempotency/locker_test.go (1)
21-83: Consider refactoring into subtests for better organization.The inline test blocks with comments work fine, but using
t.Run()subtests would provide better test organization and clearer output reporting. This is entirely optional.Example refactoring approach
- // Test that a lock can be acquired - { + t.Run("lock can be acquired", func(t *testing.T) { err := l.Lock("a") require.NoError(t, err) - } + }) - // Test that the same lock cannot be acquired again while held - { + t.Run("same lock cannot be acquired while held", func(t *testing.T) { done := make(chan struct{}) // ... rest of test - } + })
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
middleware/idempotency/locker_test.gomiddleware/session/session_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding Go tests, always invoke
t.Parallel()at the start of each test and subtest to maximize concurrency
Files:
middleware/session/session_test.gomiddleware/idempotency/locker_test.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
middleware/session/session_test.gomiddleware/idempotency/locker_test.go
🧠 Learnings (9)
📓 Common learnings
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: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: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.
📚 Learning: 2024-06-30T00:38:06.580Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3051
File: middleware/session/session.go:215-216
Timestamp: 2024-06-30T00:38:06.580Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Applied to files:
middleware/session/session_test.go
📚 Learning: 2024-09-25T17:09:03.756Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:400-407
Timestamp: 2024-09-25T17:09:03.756Z
Learning: In the `Test_Session_Next` function in `middleware/session/middleware_test.go`, the variable `doNext` is properly synchronized with the `muNext` mutex for both read and write access.
Applied to files:
middleware/session/session_test.go
📚 Learning: 2025-12-07T15:07:23.885Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
Learning: Applies to **/*_test.go : When adding Go tests, always invoke `t.Parallel()` at the start of each test and subtest to maximize concurrency
Applied to files:
middleware/session/session_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:164-165
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the `Test_CSRF_WithSession_Middleware` function, calling `session.NewWithStore()` without arguments is acceptable, as the default configuration is sufficient.
Applied to files:
middleware/session/session_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/session.go:46-61
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In this codebase, the `sessionPool` only contains `Session` instances, so type assertions without additional checks are acceptable.
Applied to files:
middleware/session/session_test.go
📚 Learning: 2024-10-12T10:01:44.206Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-10-12T10:01:44.206Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Applied to files:
middleware/session/session_test.go
📚 Learning: 2025-09-14T00:10:40.547Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 0
File: :0-0
Timestamp: 2025-09-14T00:10:40.547Z
Learning: The `session.Release()` method in the `middleware/session` package is responsible for returning the Session back to `sync.Pool` via `sessionPool.Put(s)`. It also calls Reset() to clear session data. Users must call Release() when done with the session, even after calling Save().
Applied to files:
middleware/session/session_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/session_manager.go:30-43
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the session middleware, `session.FromContext(c)` returns `*session.Middleware`, whereas `m.session.Get(c)` returns `*session.Store`, so they are not directly interchangeable.
Applied to files:
middleware/session/session_test.go
⏰ 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: Compare
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: lint
🔇 Additional comments (3)
middleware/idempotency/locker_test.go (1)
45-49: Good practice to prevent goroutine leak.This explicit unlock is essential to clean up the goroutine that remains blocked at line 33 in the previous test block. Without this, the test would leak a goroutine.
middleware/session/session_test.go (2)
334-336: Clear explanation for omitting parallel execution.This comment correctly explains why
t.Parallel()cannot be used here. Registering types in the global gob registry (lines 357-358) would indeed cause race conditions with other parallel tests.
383-389: Good verification of session state after reset.This addition properly verifies the session lifecycle after
Reset(). The pattern of releasing the reset session and then obtaining a fresh one confirms that the reset operation works correctly and that subsequent sessions start in the expected fresh state.
No description provided.