v3: Enforce key length for EncryptCookie middleware default functions #3056
Conversation
gaby
requested review from
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThe updates revolve around enhancing the Changes
Poem
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3056 +/- ##
==========================================
+ Coverage 83.04% 83.05% +0.01%
==========================================
Files 115 115
Lines 8315 8322 +7
==========================================
+ Hits 6905 6912 +7
- Misses 1077 1078 +1
+ Partials 333 332 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- docs/middleware/encryptcookie.md (3 hunks)
- docs/whats_new.md (1 hunks)
- middleware/encryptcookie/config.go (2 hunks)
- middleware/encryptcookie/encryptcookie_test.go (2 hunks)
- middleware/encryptcookie/utils.go (5 hunks)
Additional context used
GitHub Check: codecov/patch
middleware/encryptcookie/utils.go
[warning] 22-22: middleware/encryptcookie/utils.go#L22
Added line #L22 was not covered by tests
[warning] 37-37: middleware/encryptcookie/utils.go#L37
Added line #L37 was not covered by tests
[warning] 53-53: middleware/encryptcookie/utils.go#L53
Added line #L53 was not covered by tests
LanguageTool
docs/middleware/encryptcookie.md
[grammar] ~58-~58: Did you mean the verb “keeps”? For a parallel structure in formal text, use “it keeps”, because ‘random’ is an adjective.
Context: ...e values, so make sure it is random and keep it secret. For example, you can run `op...(PRP_IS_JJ_AND_VB)
docs/whats_new.md
[grammar] ~51-~51: It looks like there is a word missing here. Did you mean “listen to config”?
Context: ...ic.md) * app.Config properties moved to listen config * DisableStartupMessage * EnablePre...(LISTEN_TO_ME)
[style] ~55-~55: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...nablePrintRoutes * ListenerNetwork -> previously Network ### new methods * RegisterCus...(ADVERB_REPETITION_PREMIUM)
[uncategorized] ~195-~195: The official spelling of this programming framework is “Express.js”.
Context: ...king. ### new methods * AutoFormat -> ExpressJs like * Host -> ExpressJs like * Port ->...(NODE_JS)
[uncategorized] ~196-~196: The official spelling of this programming framework is “Express.js”.
Context: ... AutoFormat -> ExpressJs like * Host -> ExpressJs like * Port -> ExpressJs like * IsProxy...(NODE_JS)
[uncategorized] ~197-~197: The official spelling of this programming framework is “Express.js”.
Context: ...like * Host -> ExpressJs like * Port -> ExpressJs like * IsProxyTrusted * Reset * Schema ...(NODE_JS)
[uncategorized] ~200-~200: The official spelling of this programming framework is “Express.js”.
Context: ...ke * IsProxyTrusted * Reset * Schema -> ExpressJs like * SendStream -> ExpressJs like * S...(NODE_JS)
[uncategorized] ~201-~201: The official spelling of this programming framework is “Express.js”.
Context: ...chema -> ExpressJs like * SendStream -> ExpressJs like * SendString -> ExpressJs like * S...(NODE_JS)
[uncategorized] ~202-~202: The official spelling of this programming framework is “Express.js”.
Context: ...tream -> ExpressJs like * SendString -> ExpressJs like * String -> ExpressJs like * ViewB...(NODE_JS)
[uncategorized] ~203-~203: The official spelling of this programming framework is “Express.js”.
Context: ...endString -> ExpressJs like * String -> ExpressJs like * ViewBind -> instead of Bind ###...(NODE_JS)
[grammar] ~257-~257: Do not use the singular ‘a’ before the plural noun ‘conditions’.
Context: ...ache management, allowing you to define a custom conditions for invalidating cache entries. ### CO...(VB_A_JJ_NNS)
[style] ~261-~261: Consider rephrasing this to strengthen your wording.
Context: ...idating cache entries. ### CORS We've made some changes to the CORS middleware to improve its f...(MAKE_CHANGES)
[uncategorized] ~269-~269: Loose punctuation mark.
Context: ...updated fields: -Config.AllowOrigins: Now accepts a slice of strings, each re...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~270-~270: Loose punctuation mark.
Context: ... allowed origin. -Config.AllowMethods: Now accepts a slice of strings, each re...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~271-~271: Loose punctuation mark.
Context: ... allowed method. -Config.AllowHeaders: Now accepts a slice of strings, each re...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~272-~272: Loose punctuation mark.
Context: ...allowed header. -Config.ExposeHeaders: Now accepts a slice of strings, each re...(UNLIKELY_OPENING_PUNCTUATION)
Markdownlint
docs/middleware/encryptcookie.md
7-7: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
77-77: Column: 1
Hard tabs(MD010, no-hard-tabs)
78-78: Column: 1
Hard tabs(MD010, no-hard-tabs)
79-79: Column: 1
Hard tabs(MD010, no-hard-tabs)
80-80: Column: 1
Hard tabs(MD010, no-hard-tabs)
81-81: Column: 1
Hard tabs(MD010, no-hard-tabs)
92-92: Column: 1
Hard tabs(MD010, no-hard-tabs)
93-93: Column: 1
Hard tabs(MD010, no-hard-tabs)
96-96: Column: 1
Hard tabs(MD010, no-hard-tabs)
97-97: Column: 1
Hard tabs(MD010, no-hard-tabs)
98-98: Column: 1
Hard tabs(MD010, no-hard-tabs)
99-99: Column: 1
Hard tabs(MD010, no-hard-tabs)
85-85: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
103-103: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
120-120: null
Files should end with a single newline character(MD047, single-trailing-newline)
docs/whats_new.md
49-49: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
50-50: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
51-51: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
52-52: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
53-53: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
54-54: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
55-55: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
59-59: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
60-60: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
61-61: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
65-65: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
66-66: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
67-67: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
68-68: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
69-69: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
73-73: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
74-74: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
75-75: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
195-195: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
196-196: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
197-197: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
198-198: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
199-199: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
200-200: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
201-201: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
202-202: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
203-203: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
204-204: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
208-208: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
209-209: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
210-210: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
211-211: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
212-212: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
213-213: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
214-214: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
215-215: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
216-216: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
217-217: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
218-218: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
222-222: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
223-223: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
224-224: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
225-225: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
233-233: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
290-290: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
246-246: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
311-311: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
263-263: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
266-266: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
143-143: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
171-171: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
23-23: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
264-264: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
Additional comments not posted (12)
middleware/encryptcookie/config.go (3)
21-23: Update documentation forKeyinConfigstruct to reflect new key length requirements.The documentation now clearly states the required key lengths and suggests using the
GenerateKeyfunction to generate a valid key. This is a crucial update to ensure users are aware of the key length requirements when using the default encryption functions.
28-28: Document default encryption function.The
Encryptorfunction now defaults to using AES-256-GCM, which aligns with the enhanced security measures. It's good to see this explicitly mentioned in the documentation.
33-33: Document default decryption function.Similarly to the
Encryptor, theDecryptorfunction also defaults to using AES-256-GCM. This consistency in encryption and decryption methods is beneficial for maintaining secure data handling practices.middleware/encryptcookie/utils.go (2)
87-98: Review changes toGenerateKeyfunction.The
GenerateKeyfunction now accepts a key length parameter and correctly panics if an invalid length is provided. This update is crucial for ensuring that keys of appropriate lengths are generated for the supported AES modes.
37-37: Ensure error handling when reading the nonce.The error handling for reading the nonce in
EncryptCookieis properly implemented, providing a clear error message if the operation fails.Tools
GitHub Check: codecov/patch
[warning] 37-37: middleware/encryptcookie/utils.go#L37
Added line #L37 was not covered by testsdocs/middleware/encryptcookie.md (3)
19-20: Documentation update forGenerateKeyfunction.The function signature in the documentation has been updated to include the
lengthparameter, which is essential for generating keys of specific lengths. This change should help users understand how to use the function correctly.
58-60: Clarify key generation and usage in documentation.The documentation now includes important notes on generating and using keys, emphasizing the need for randomness and security. It also correctly warns against generating a new key on every application run, which could lead to security vulnerabilities.
Tools
LanguageTool
[grammar] ~58-~58: Did you mean the verb “keeps”? For a parallel structure in formal text, use “it keeps”, because ‘random’ is an adjective.
Context: ...e values, so make sure it is random and keep it secret. For example, you can run `op...(PRP_IS_JJ_AND_VB)
103-120: Document supported encryption algorithms and key requirements.The documentation clearly lists the key requirements for different AES modes, providing examples for generating keys of specific lengths. This is a valuable addition for users needing to comply with specific encryption standards.
Tools
Markdownlint
103-103: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
120-120: null
Files should end with a single newline character(MD047, single-trailing-newline)
docs/whats_new.md (1)
278-281: Update release notes to include changes toEncryptCookiemiddleware.The release notes have been updated to reflect the new support for specifying key lengths when using the
GenerateKeyfunction. This is an important update that should be communicated to users to highlight the enhanced flexibility and security of the middleware.middleware/encryptcookie/encryptcookie_test.go (3)
14-14: Approved the update totestKeyvariable.The update aligns with the new
GenerateKeyfunction signature, ensuring the key is suitable for AES-256 encryption.
194-225: Approved the comprehensive testing inTest_GenerateKey.This function effectively tests all valid key lengths and correctly expects panics for invalid ones, ensuring robust key generation aligned with AES standards.
228-590: Approved the addition of comprehensive benchmarks.The benchmarks are well-designed to assess the performance of the middleware under various scenarios, including parallel execution. This is valuable for understanding and optimizing the middleware's performance.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- middleware/encryptcookie/encryptcookie_test.go (2 hunks)
Additional comments not posted (3)
middleware/encryptcookie/encryptcookie_test.go (3)
15-15: Correct use ofGenerateKeyfunction with appropriate key length.This correctly follows the new function signature and the recommended key length for AES-256-GCM.
252-284: Comprehensive testing ofGenerateKeyfunction.The test efficiently checks both valid and invalid key lengths, ensuring the function behaves as expected. The use of parallel testing is appropriate and should improve test execution time.
286-610: Well-structured benchmarks forEncryptCookiemiddleware.The benchmarks cover a wide range of scenarios, including error handling and custom encryptors, providing valuable insights into the middleware's performance. The use of parallel benchmarks is excellent for simulating high-load environments.
Co-authored-by: Jason McNeil <sixcolors@mac.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
docs/middleware/encryptcookie.md (1)
Line range hint
7-7: Remove trailing spaces and replace hard tabs with spaces.Remove trailing spaces and replace hard tabs with spaces to adhere to markdownlint rules.
--- id: encryptcookie --- # Encrypt Cookie Encrypt Cookie is a middleware for [Fiber](https://github.com/gofiber/fiber) that secures your cookie values through encryption. :::note This middleware encrypts cookie values and not the cookie names. ::: ## Signatures ```go // Intitializes the middleware func New(config ...Config) fiber.Handler // GenerateKey returns a random string of 16, 24, or 32 bytes. // The length of the key determines the AES encryption algorithm used: // 16 bytes for AES-128, 24 bytes for AES-192, and 32 bytes for AES-256-GCM. func GenerateKey(length int) stringExamples
To use the Encrypt Cookie middleware, first, import the middleware package as part of the Fiber web framework:
import ( "github.com/gofiber/fiber/v3" "github.com/gofiber/fiber/v3/middleware/encryptcookie" )Once you've imported the middleware package, you can use it inside your Fiber app:
// Provide a minimal configuration app.Use(encryptcookie.New(encryptcookie.Config{ Key: "secret-thirty-2-character-string", })) // Retrieve the encrypted cookie value app.Get("/", func(c fiber.Ctx) error { return c.SendString("value=" + c.Cookies("test")) }) // Create an encrypted cookie app.Post("/", func(c fiber.Ctx) error { c.Cookie(&fiber.Cookie{ Name: "test", Value: "SomeThing", }) return nil }):::note
Key must be a 16, 24, or 32-byte encoded string. It is used to encrypt the values, so make sure it is random and keep it secret.
For example, you can runopenssl rand -base64 32or callencryptcookie.GenerateKey(32)to create a random key for you.
Make sure not to setKeytoencryptcookie.GenerateKey(32)because that will create a new key every run of the application.
:::Config
Property Type Description Default Next func(fiber.Ctx) boolA function to skip this middleware when returned true. nilExcept []stringArray of cookie keys that should not be encrypted. []Key stringA base64-encoded unique key to encode & decode cookies. Required. Key length should be 32 characters. (No default, required field) Encryptor func(decryptedString, key string) (string, error)A custom function to encrypt cookies. EncryptCookieDecryptor func(encryptedString, key string) (string, error)A custom function to decrypt cookies. DecryptCookieDefault Config
var ConfigDefault = Config{ Next: nil, Except: []string{}, Key: "", Encryptor: EncryptCookie, Decryptor: DecryptCookie, }Usage With Other Middlewares That Reads Or Modify Cookies
Place the
encryptcookiemiddleware before any other middleware that reads or modifies cookies. For example, if you are using the CSRF middleware, ensure that theencryptcookiemiddleware is placed before it. Failure to do so may prevent the CSRF middleware from reading the encrypted cookie.You may also choose to exclude certain cookies from encryption. For instance, if you are using the
CSRFmiddleware with a frontend framework like Angular, and the framework reads the token from a cookie, you should exclude that cookie from encryption. This can be achieved by adding the cookie name to the Except array in the configuration:app.Use(encryptcookie.New(encryptcookie.Config{ Key: "secret-thirty-2-character-string", Except: []string{csrf.ConfigDefault.CookieName}, // exclude CSRF cookie })) app.Use(csrf.New(csrf.Config{ KeyLookup: "header:" + csrf.HeaderName, CookieSameSite: "Lax", CookieSecure: true, CookieHTTPOnly: false, }))Encryption Algorithms
The default Encryptor and Decryptor functions use
AES-256-GCMfor encryption and decryption. If you need to useAES-128orAES-192instead, you can do so by changing the length of the key when callingencryptcookie.GenerateKey(length)or by providing a key of one of the following lengths:
- AES-128 requires a 16-byte key.
- AES-192 requires a 24-byte key.
- AES-256 requires a 32-byte key.
For example, to generate a key for AES-128:
key := encryptcookie.GenerateKey(16)And for AES-192:
key := encryptcookie.GenerateKey(24)Also applies to: 79-83, 94-101 </blockquote></details> </blockquote></details> <details> <summary>Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** <details> <summary>Commits</summary> Files that changed from the base of the PR and between a4d135c3a92bb05fafdfa4d799d014625a3501d0 and f95b116e23655dc5f01fa40af72edacd4fc89676. </details> <details> <summary>Files selected for processing (2)</summary> * docs/middleware/encryptcookie.md (3 hunks) * middleware/encryptcookie/encryptcookie_test.go (2 hunks) </details> <details> <summary>Additional context used</summary> <details> <summary>Learnings (3)</summary><blockquote> <details> <summary>Common learnings</summary><blockquote>Learnt from: gaby
PR: #3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in bothEncryptCookieandDecryptCookiefunctions have been added to ensure robust validation and prevent potential runtime errors.Learnt from: gaby
PR: #3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-07-01T03:33:22.283Z
Learning: Unit tests for key length enforcement inDecryptCookiehave been added to ensure consistency and security in the encryption processes.</blockquote></details> <details> <summary>docs/middleware/encryptcookie.md (2)</summary><blockquote>Learnt from: gaby
PR: #3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in bothEncryptCookieandDecryptCookiefunctions have been added to ensure robust validation and prevent potential runtime errors.Learnt from: gaby
PR: #3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-07-01T03:33:22.283Z
Learning: Unit tests for key length enforcement inDecryptCookiehave been added to ensure consistency and security in the encryption processes.</blockquote></details> <details> <summary>middleware/encryptcookie/encryptcookie_test.go (2)</summary><blockquote>Learnt from: gaby
PR: #3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in bothEncryptCookieandDecryptCookiefunctions have been added to ensure robust validation and prevent potential runtime errors.Learnt from: gaby
PR: #3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-07-01T03:33:22.283Z
Learning: Unit tests for key length enforcement inDecryptCookiehave been added to ensure consistency and security in the encryption processes.</blockquote></details> </blockquote></details> <details> <summary>LanguageTool</summary><blockquote> <details> <summary>docs/middleware/encryptcookie.md</summary><blockquote> [grammar] ~60-~60: Did you mean the verb “keeps”? For a parallel structure in formal text, use “it keeps”, because ‘random’ is an adjective. Context: ...e values, so make sure it is random and keep it secret. For example, you can run `op... (PRP_IS_JJ_AND_VB) </blockquote></details> </blockquote></details> <details> <summary>Markdownlint</summary><blockquote> <details> <summary>docs/middleware/encryptcookie.md</summary><blockquote> 7-7: Expected: 0 or 2; Actual: 1 Trailing spaces (MD009, no-trailing-spaces) --- 79-79: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 80-80: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 81-81: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 82-82: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 83-83: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 94-94: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 95-95: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 98-98: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 99-99: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 100-100: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 101-101: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 87-87: Expected: 1; Actual: 0; Below Headings should be surrounded by blank lines (MD022, blanks-around-headings) --- 105-105: Expected: 1; Actual: 0; Below Headings should be surrounded by blank lines (MD022, blanks-around-headings) --- 122-122: null Files should end with a single newline character (MD047, single-trailing-newline) </blockquote></details> </blockquote></details> </details> <details> <summary>Additional comments not posted (5)</summary><blockquote> <details> <summary>middleware/encryptcookie/encryptcookie_test.go (5)</summary><blockquote> `15-15`: **LGTM!** The `testKey` initialization looks good and ensures a 32-byte key is used for tests. --- `38-71`: **LGTM!** The test cases for invalid key lengths are comprehensive and cover various scenarios. --- `74-98`: **LGTM!** The test cases for invalid base64 strings are well-written and ensure proper error handling. --- `278-310`: **LGTM!** The test case for `GenerateKey` covers valid and invalid lengths and ensures proper key generation. --- `312-674`: **LGTM!** The benchmark functions are comprehensive and cover various scenarios, including parallel benchmarks. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Co-authored-by: Jason McNeil <sixcolors@mac.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- .github/workflows/test.yml (2 hunks)
- docs/middleware/encryptcookie.md (4 hunks)
- middleware/encryptcookie/config.go (2 hunks)
- middleware/encryptcookie/encryptcookie_test.go (5 hunks)
- middleware/encryptcookie/utils.go (5 hunks)
Files skipped from review due to trivial changes (1)
- .github/workflows/test.yml
Files skipped from review as they are similar to previous changes (1)
- middleware/encryptcookie/config.go
Additional context used
Learnings (4)
Common learnings
Learnt from: gaby PR: gofiber/fiber#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 PR: gofiber/fiber#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.middleware/encryptcookie/utils.go (2)
Learnt from: gaby PR: gofiber/fiber#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 PR: gofiber/fiber#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.docs/middleware/encryptcookie.md (2)
Learnt from: gaby PR: gofiber/fiber#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 PR: gofiber/fiber#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.middleware/encryptcookie/encryptcookie_test.go (2)
Learnt from: gaby PR: gofiber/fiber#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 PR: gofiber/fiber#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.
GitHub Check: codecov/patch
middleware/encryptcookie/utils.go
[warning] 39-39: middleware/encryptcookie/utils.go#L39
Added line #L39 was not covered by tests
Markdownlint
docs/middleware/encryptcookie.md
101-101: Column: 1
Hard tabs(MD010, no-hard-tabs)
108-108: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
122-122: null
Files should end with a single newline character(MD047, single-trailing-newline)
Additional comments not posted (21)
middleware/encryptcookie/utils.go (3)
13-13: Ensure the new sentinel error is used consistently.The new sentinel error
ErrInvalidKeyLengthimproves error handling consistency.
88-95: LGTM! But verify test coverage for key length validation.The
GenerateKeyfunction now generates keys of 16, 24, or 32 bytes, which are valid lengths for AES encryption. The validation ensures that only these lengths are accepted.Ensure the added validation is covered by unit tests.
Verification successful
Test coverage for key length validation in
GenerateKeyverified.The
encryptcookie_test.gofile contains tests that validate the key length check in theGenerateKeyfunction by ensuring it panics with invalid key lengths.
middleware/encryptcookie/encryptcookie_test.go: require.Panics(t, func() { GenerateKey(10) })middleware/encryptcookie/encryptcookie_test.go: require.Panics(t, func() { GenerateKey(20) })Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the key length validation in GenerateKey is covered by unit tests. # Test: Search for the test coverage for the key length check in GenerateKey. rg --type go 'panic(ErrInvalidKeyLength)'Length of output: 43
Script:
#!/bin/bash # Description: Verify that the GenerateKey function is covered by unit tests. # Test: Search for test cases related to the GenerateKey function. rg --type go 'GenerateKey'Length of output: 2233
52-56: LGTM! But verify test coverage for key length validation.The key length validation ensures that the key length is either 16, 24, or 32 bytes, which are valid lengths for AES-GCM.
Ensure the added validation is covered by unit tests.
Verification successful
Test coverage for key length validation in
DecryptCookieis verified.The tests in
encryptcookie_test.goinclude scenarios with different key lengths, ensuring that the key length validation logic is properly tested.
middleware/encryptcookie/encryptcookie_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the key length validation in DecryptCookie is covered by unit tests. # Test: Search for the test coverage for the key length check in DecryptCookie. rg --type go 'keyLen != 16 && keyLen != 24 && keyLen != 32'Length of output: 231
Script:
#!/bin/bash # Description: Find test functions that call DecryptCookie and inspect their inputs for key length validation. # Step 1: Find all test files. test_files=$(fd --type f --extension go --exec rg --files-with-matches 'func Test') # Step 2: Search for calls to DecryptCookie within these test files. rg --type go 'DecryptCookie' $test_filesLength of output: 4436
Script:
#!/bin/bash # Description: Inspect calls to DecryptCookie in encryptcookie_test.go for key length validation. # Step 1: Print the relevant lines from encryptcookie_test.go to inspect key lengths used in DecryptCookie calls. rg --type go -A 5 -B 5 'DecryptCookie' ./middleware/encryptcookie/encryptcookie_test.goLength of output: 1700
docs/middleware/encryptcookie.md (3)
19-22: LGTM! The documentation correctly reflects the new function signature.The
GenerateKeyfunction signature has been updated to accept a length parameter.
60-61: LGTM! The note correctly explains the key length requirements and provides examples for generating keys.The note about the key length requirements and how to generate keys is clear and accurate.
105-122: LGTM! The explanations and examples for using different key lengths for different AES algorithms are clear and accurate.The section on encryption algorithms provides clear and accurate information on how to use different key lengths for different AES algorithms.
Tools
Markdownlint
108-108: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
122-122: null
Files should end with a single newline character(MD047, single-trailing-newline)
middleware/encryptcookie/encryptcookie_test.go (15)
15-33: LGTM! The tests ensure that the middleware correctly handles empty and invalid keys.The function
Test_Middleware_Panicstests for panics when the key is empty or invalid, ensuring that the middleware correctly handles these scenarios.
36-69: LGTM! The tests ensure that the middleware correctly handles invalid key lengths.The function
Test_Middleware_InvalidKeystests for invalid key lengths during encryption and decryption, ensuring that the middleware correctly handles these scenarios.
72-95: LGTM! The tests ensure that the middleware correctly handles invalid base64 encoding.The function
Test_Middleware_InvalidBase64tests for invalid base64 encoding during encryption and decryption, ensuring that the middleware correctly handles these scenarios.
Line range hint
96-161: LGTM! The tests ensure that the middleware correctly encrypts and decrypts cookies.The function
Test_Middleware_Encrypt_Cookietests the encryption and decryption of cookies using the middleware, ensuring that the middleware correctly handles these scenarios.
Line range hint
162-185: LGTM! The tests ensure that the middleware correctly handles theNextconfiguration option.The function
Test_Encrypt_Cookie_Nexttests theNextconfiguration option of the middleware, ensuring that the middleware correctly handles this scenario.
Line range hint
187-230: LGTM! The tests ensure that the middleware correctly handles theExceptconfiguration option.The function
Test_Encrypt_Cookie_Excepttests theExceptconfiguration option of the middleware, ensuring that the middleware correctly handles this scenario.
Line range hint
232-278: LGTM! The tests ensure that the middleware correctly handles custom encryptor and decryptor functions.The function
Test_Encrypt_Cookie_Custom_Encryptortests the custom encryptor and decryptor configuration options of the middleware, ensuring that the middleware correctly handles these scenarios.
280-312: LGTM! The tests ensure that theGenerateKeyfunction correctly handles valid and invalid key lengths.The function
Test_GenerateKeytests theGenerateKeyfunction for valid and invalid key lengths, ensuring that the function correctly handles these scenarios.
314-359: LGTM! The benchmarks ensure that the middleware performs efficiently during encryption and decryption.The function
Benchmark_Middleware_Encrypt_Cookiebenchmarks the encryption and decryption of cookies using the middleware, ensuring that the middleware performs efficiently.
361-390: LGTM! The benchmarks ensure that the middleware performs efficiently with theNextconfiguration option.The function
Benchmark_Encrypt_Cookie_Nextbenchmarks theNextconfiguration option of the middleware, ensuring that the middleware performs efficiently with this configuration.
392-425: LGTM! The benchmarks ensure that the middleware performs efficiently with theExceptconfiguration option.The function
Benchmark_Encrypt_Cookie_Exceptbenchmarks theExceptconfiguration option of the middleware, ensuring that the middleware performs efficiently with this configuration.
427-480: LGTM! The benchmarks ensure that the middleware performs efficiently with custom encryptor and decryptor functions.The function
Benchmark_Encrypt_Cookie_Custom_Encryptorbenchmarks the custom encryptor and decryptor configuration options of the middleware, ensuring that the middleware performs efficiently with these configurations.
482-533: LGTM! The benchmarks ensure that the middleware performs efficiently during parallel execution of encryption and decryption.The function
Benchmark_Middleware_Encrypt_Cookie_Parallelbenchmarks the parallel execution of encryption and decryption of cookies using the middleware, ensuring that the middleware performs efficiently.
535-564: LGTM! The benchmarks ensure that the middleware performs efficiently with theNextconfiguration option during parallel execution.The function
Benchmark_Encrypt_Cookie_Next_Parallelbenchmarks the parallel execution of theNextconfiguration option of the middleware, ensuring that the middleware performs efficiently with this configuration.
566-599: LGTM! The benchmarks ensure that the middleware performs efficiently with theExceptconfiguration option during parallel execution.The function
Benchmark_Encrypt_Cookie_Except_Parallelbenchmarks the parallel execution of theExceptconfiguration option of the middleware, ensuring that the middleware performs efficiently with this configuration.
Description
EncryptCookiemiddleware.AES-GCMonly works with16,24,32bytes keys.lengthparam toencryptcookie.GenerateKey.encryptcookie.GenerateKey.EncryptCookiemiddleware.EncryptCookiemiddleware.+12.37%Related to #3048
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change