🐛 bug: Fix EncryptCookie middleware panic#3958
Conversation
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
Summary of ChangesHello @dogukanoksuz, 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 resolves a panic condition within 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
|
|
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. WalkthroughOn decryption error, the encryptcookie middleware now preserves the cookie key but sets its value to an empty string (clears value) instead of deleting the cookie; successful decryption remains unchanged. A test was added to ensure invalid/unencrypted cookie values do not panic and result in a cleared cookie. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
There was a problem hiding this comment.
Code Review
This pull request addresses a panic in the EncryptCookie middleware that occurs when it encounters a non-encrypted cookie. The fix replaces the call to c.Request().Header.DelCookieBytes(key), which was causing the panic, with c.Request().Header.SetCookie(string(key), ""). This change effectively neutralizes invalid cookies by setting their value to an empty string, preventing the application crash. The change is simple, effective, and correctly works around the underlying issue. I have provided one suggestion for a minor performance improvement, but the current change is correct and resolves the reported issue.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
middleware/encryptcookie/encryptcookie.go (1)
24-34: Add test coverage for cookies with special characters in the name.The fix correctly replaces
DelCookieByteswithSetCookie(string(key), "")on decryption failure, which resolves the panic when processing cookies with special characters like colons. The behavioral change—setting cookies to empty strings instead of deleting them—is partially tested.However, the test suite lacks coverage for the specific scenario that triggered the panic: cookie names containing special characters (e.g., "sidebar:state"). Add a test case that sets a request cookie with special characters in the name and an invalid encrypted value to verify the middleware handles the error gracefully without panicking.
🧹 Nitpick comments (1)
middleware/encryptcookie/encryptcookie.go (1)
29-29: Use existingkeyStringvariable to avoid redundant conversion.Line 25 already converts the key to a string (
keyString := string(key)). Consider usingkeyStringhere instead of converting again:🔎 Proposed refactor
- c.Request().Header.SetCookie(string(key), "") + c.Request().Header.SetCookie(keyString, "")
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/encryptcookie/encryptcookie.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/encryptcookie/encryptcookie.go
🧠 Learnings (5)
📓 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-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: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: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: 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.
📚 Learning: 2024-10-08T19:06:06.583Z
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.
Applied to files:
middleware/encryptcookie/encryptcookie.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.go
📚 Learning: 2024-10-08T19:06:06.583Z
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.
Applied to files:
middleware/encryptcookie/encryptcookie.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
🧬 Code graph analysis (1)
middleware/encryptcookie/encryptcookie.go (1)
client/request.go (2)
Request(46-73)Header(708-710)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3958 +/- ##
==========================================
- Coverage 91.62% 91.56% -0.06%
==========================================
Files 119 119
Lines 10187 10187
==========================================
- Hits 9334 9328 -6
- Misses 540 545 +5
- Partials 313 314 +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:
|
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a panic in the EncryptCookie middleware that occurred when non-encrypted cookies (not in the exception list) were encountered during request processing. The fix changes the error handling behavior from attempting to delete the cookie to setting it to an empty value.
- Replaces
DelCookieByteswithSetCookieBytesto safely handle decryption failures - Prevents application crashes when encountering invalid or unencrypted cookies
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/encryptcookie/encryptcookie.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/encryptcookie/encryptcookie.go
🧠 Learnings (5)
📓 Common learnings
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: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: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.
📚 Learning: 2024-10-08T19:06:06.583Z
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.
Applied to files:
middleware/encryptcookie/encryptcookie.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.go
📚 Learning: 2024-10-08T19:06:06.583Z
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.
Applied to files:
middleware/encryptcookie/encryptcookie.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
🪛 GitHub Actions: Modernize Lint
middleware/encryptcookie/encryptcookie.go
[error] 29-29: Go compile error: c.Request().Header.SetCookieBytes undefined (type fasthttp.RequestHeader has no field or method SetCookieBytes).
🪛 GitHub Actions: Run govulncheck
middleware/encryptcookie/encryptcookie.go
[error] 29-29: govulncheck: loading packages: There are errors with the provided package patterns: /home/runner/work/fiber/fiber/middleware/encryptcookie/encryptcookie.go:29:25: c.Request().Header.SetCookieBytes undefined (type fasthttp.RequestHeader has no field or method SetCookieBytes)
🪛 GitHub Check: govulncheck-check
middleware/encryptcookie/encryptcookie.go
[failure] 29-29:
c.Request().Header.SetCookieBytes undefined (type fasthttp.RequestHeader has no field or method SetCookieBytes)
🪛 GitHub Check: modernize
middleware/encryptcookie/encryptcookie.go
[failure] 29-29:
c.Request().Header.SetCookieBytes undefined (type fasthttp.RequestHeader has no field or method SetCookieBytes)
🪛 GitHub Check: unit (1.25.x, ubuntu-latest)
middleware/encryptcookie/encryptcookie.go
[failure] 29-29:
c.Request().Header.SetCookieBytes undefined (type fasthttp.RequestHeader has no field or method SetCookieBytes)
[failure] 29-29:
c.Request().Header.SetCookieBytes undefined (type fasthttp.RequestHeader has no field or method SetCookieBytes)
⏰ 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-latest)
- GitHub Check: repeated
- GitHub Check: lint
- GitHub Check: Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
middleware/encryptcookie/encryptcookie.go (3)
29-29: ReusekeyStringto avoid duplicate string conversion.Line 25 already converts
keytokeyString, but line 29 performs the conversion again withstring(key). Reuse the existing variable for better efficiency.🔎 Proposed fix
- c.Request().Header.SetCookie(string(key), "") + c.Request().Header.SetCookie(keyString, "")
27-29: Consider logging or tracking decryption failures for security and debugging.Decryption errors are currently silently ignored, which could hide important issues such as cookie tampering attempts, key rotation problems, or malformed cookies. Unlike encryption errors (lines 47-48), which are properly returned, decryption failures provide no visibility.
Consider adding at minimum debug-level logging when decryption fails, or expose a metric/counter for monitoring. This would aid in debugging and security monitoring without breaking the current error handling flow.
Example implementation (add logging):
decryptedValue, err := cfg.Decryptor(keyString, string(value), cfg.Key) if err != nil { // Log decryption failure for debugging/security monitoring // Example: log.Debug().Err(err).Str("cookie", keyString).Msg("cookie decryption failed") c.Request().Header.SetCookie(keyString, "") } else { c.Request().Header.SetCookie(keyString, decryptedValue) }
31-31: Apply the same optimization here: reusekeyString.Similar to line 29, this line also performs a duplicate string conversion. For consistency and efficiency, use
keyStringhere as well.🔎 Proposed fix
- c.Request().Header.SetCookie(string(key), decryptedValue) + c.Request().Header.SetCookie(keyString, decryptedValue)
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/encryptcookie/encryptcookie.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/encryptcookie/encryptcookie.go
🧠 Learnings (5)
📓 Common learnings
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: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: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-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: 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.
📚 Learning: 2024-10-08T19:06:06.583Z
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.
Applied to files:
middleware/encryptcookie/encryptcookie.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.go
📚 Learning: 2024-10-08T19:06:06.583Z
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.
Applied to files:
middleware/encryptcookie/encryptcookie.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
🧬 Code graph analysis (1)
middleware/encryptcookie/encryptcookie.go (1)
client/request.go (2)
Request(46-73)Header(708-710)
⏰ 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). (5)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: lint
- GitHub Check: Compare
🔇 Additional comments (1)
middleware/encryptcookie/encryptcookie.go (1)
28-32: Consider reusing thekeyStringvariable to avoid redundant string conversion.On line 29,
string(key)is converted to a string for the second time;keyStringis already created on line 25 and should be reused here.Also note that setting failed decryption cookies to an empty string (rather than removing them) is intentional behavior per the middleware design—as confirmed by the comprehensive test coverage in
encryptcookie_test.go, which validates that invalid/malformed cookies result in empty values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
middleware/encryptcookie/encryptcookie_test.go (1)
110-140: LGTM! Test correctly validates the panic fix.The test properly verifies that invalid/unencrypted cookies don't crash the middleware and that the cookie value is cleared as expected. The test follows coding guidelines by calling
t.Parallel()at the start.Optional: Minor improvements to error handling and test robustness
Consider these small refinements:
- Handle the error from
resp.Body.Readinstead of ignoring it:- body := make([]byte, 64) - n, _ := resp.Body.Read(body) - require.Equal(t, "value=", string(body[:n])) + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.Equal(t, "value=", string(body))Add
ioto the imports if using this approach.
- Optionally, test with the specific cookie name from issue #3954 to demonstrate the exact scenario is fixed:
req.AddCookie(&http.Cookie{ - Name: "test", + Name: "sidebar:state", Value: "plaintext-unencrypted-value", })And update the handler to read the corresponding cookie:
app.Get("/", func(c fiber.Ctx) error { - return c.SendString("value=" + c.Cookies("test")) + return c.SendString("value=" + c.Cookies("sidebar:state")) })
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/encryptcookie/encryptcookie_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/encryptcookie/encryptcookie_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/encryptcookie/encryptcookie_test.go
🧠 Learnings (4)
📓 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-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: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: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: 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.
📚 Learning: 2024-10-08T19:06:06.583Z
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.
Applied to files:
middleware/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_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
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.
Applied to files:
middleware/encryptcookie/encryptcookie_test.go
🧬 Code graph analysis (1)
middleware/encryptcookie/encryptcookie_test.go (2)
middleware/encryptcookie/utils.go (1)
GenerateKey(98-110)middleware/encryptcookie/encryptcookie.go (1)
New(12-59)
| app := fiber.New() | ||
|
|
||
| app.Use(New(Config{ | ||
| Key: testKey, |
There was a problem hiding this comment.
the mentioned problem has no relation with sidebar:state key. it is happening on every unexpected and non encrypted cookie.
There was a problem hiding this comment.
Does that mean this test should then fail in the main branch?
i fixed the test locally
=== FAIL: middleware/encryptcookie Test_Middleware_Decrypt_Invalid_Cookie_Does_Not_Panic (0.00s)
encryptcookie_test.go:139:
Error Trace: /home/runner/work/fiber/fiber/middleware/encryptcookie/encryptcookie_test.go:139
Error: Received unexpected error:
EOF
Test: Test_Middleware_Decrypt_Invalid_Cookie_Does_Not_Panic
https://github.com/gofiber/fiber/actions/runs/20556624972/job/59041909729?pr=3958
body, err := io.ReadAll(resp.Body)
require.NoError(t, err)
require.Equal(t, "value=", string(body))and there is no difference with (“SetCookie”) or without (“DelCookieBytes”) your adjustment in the test
I would expect the test to fail without your code change, i.e. with “DelCookieBytes”
There was a problem hiding this comment.
same with the mentioned cookie key
|
While this works, it also means we try to decrypt every cookie. Even the ones not encrypted. |
|
@gaby maybe completely changing the middleware solves the root cause. middleware by itself tries every single cookie that exists in request. |
|
@dogukanoksuz Can you fix the linter issue. LGTM |
We will keep this in mind |
…valid_Cookie test
|
@gaby it should be fixed, can you check |
ReneWerner87
left a comment
There was a problem hiding this comment.
The adjustments are not entirely conclusive and require further steps before they can be moved to the main branch.
| decryptedValue, err := cfg.Decryptor(keyString, string(value), cfg.Key) | ||
| if err != nil { | ||
| c.Request().Header.DelCookieBytes(key) | ||
| c.Request().Header.SetCookie(string(key), "") |
There was a problem hiding this comment.
I don't think that's the fix.
Delete cookie analyzes the currently existing cookies.
SetCookie simply writes a new value without analyzing anything.
If the analysis is the problem and “sidebar:state” is a valid key, the analysis should be fixed instead of using a dirty solution everywhere the cookies are used, which only skips the analysis.
The “Except” feature can be used as a quick fix (https://docs.gofiber.io/next/middleware/encryptcookie#config).
| app := fiber.New() | ||
|
|
||
| app.Use(New(Config{ | ||
| Key: testKey, |
There was a problem hiding this comment.
Does that mean this test should then fail in the main branch?
i fixed the test locally
=== FAIL: middleware/encryptcookie Test_Middleware_Decrypt_Invalid_Cookie_Does_Not_Panic (0.00s)
encryptcookie_test.go:139:
Error Trace: /home/runner/work/fiber/fiber/middleware/encryptcookie/encryptcookie_test.go:139
Error: Received unexpected error:
EOF
Test: Test_Middleware_Decrypt_Invalid_Cookie_Does_Not_Panic
https://github.com/gofiber/fiber/actions/runs/20556624972/job/59041909729?pr=3958
body, err := io.ReadAll(resp.Body)
require.NoError(t, err)
require.Equal(t, "value=", string(body))and there is no difference with (“SetCookie”) or without (“DelCookieBytes”) your adjustment in the test
I would expect the test to fail without your code change, i.e. with “DelCookieBytes”
| app := fiber.New() | ||
|
|
||
| app.Use(New(Config{ | ||
| Key: testKey, |
There was a problem hiding this comment.
same with the mentioned cookie key
|
its related to the fasthttp core |
Description
Any non-encrypted cookie that doesn't set in except list for EncryptCookie middleware crashes the application.
Fixes #3954
Changes introduced
Changes are non-breaking