🔥 feat: Support hashed BasicAuth passwords #3631
Conversation
WalkthroughThe BasicAuth middleware was updated to require and support hashed passwords (SHA-256, SHA-512, SHA-1, MD5, bcrypt) instead of plaintext. Documentation and tests were revised to reflect these changes, including new helper and test functions for various hash formats. The configuration now interprets password hashes and verifies credentials accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant Config
Client->>Middleware: Sends request with Authorization header
Middleware->>Config: Looks up user verifier by username
Config-->>Middleware: Returns password verifier function
Middleware->>Middleware: Calls verifier with provided password
alt Password valid
Middleware-->>Client: Allow request to proceed
else Password invalid
Middleware-->>Client: Respond with 401 Unauthorized
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Summary of Changes
Hello @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!
I've implemented a significant security enhancement to the BasicAuth middleware. This change moves away from plaintext password storage and verification, instead requiring and supporting various hashed password formats like SHA-256 and bcrypt. This makes the BasicAuth component more robust and secure by design.
Highlights
- Enhanced Security for BasicAuth: The BasicAuth middleware now exclusively accepts pre-hashed passwords (e.g., SHA-256, bcrypt) instead of plaintext, greatly improving credential security.
- Multi-Algorithm Hashing Support: I've added support for multiple hashing algorithms, including SHA-256 (default), bcrypt, SHA-512, SHA-1, and MD5, identified by specific prefixes.
- Internalized Hashing Logic: The external go-htpasswd dependency has been removed, and the password verification logic for various hash types is now handled internally within the middleware.
- Updated Documentation: The documentation has been updated to clearly reflect the new requirement for hashed passwords and provide examples of their usage.
- Comprehensive Test Coverage: All relevant tests have been updated to incorporate the new hashed password verification, ensuring the feature's correctness and reliability.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3631 +/- ##
==========================================
+ Coverage 91.14% 91.76% +0.61%
==========================================
Files 113 113
Lines 11388 11410 +22
==========================================
+ Hits 10380 10470 +90
+ Misses 750 675 -75
- Partials 258 265 +7
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 introduces support for hashed passwords in the BasicAuth middleware, which is a great security improvement. The implementation correctly handles bcrypt and various SHA hashes. My review focuses on further improving security by removing weak hashing algorithms, enhancing documentation clarity, and improving test code quality and maintainability. The most critical issue is the support for MD5 and SHA-1, which are insecure for password hashing.
| Authorizer: func(user, pass string, c fiber.Ctx) bool { | ||
| if user == "john" && pass == "doe" { | ||
| return true | ||
| } | ||
| if user == "admin" && pass == "123456" { | ||
| return true | ||
| } | ||
| return false | ||
| // custom validation logic | ||
| return (user == "john" || user == "admin") | ||
| }, |
There was a problem hiding this comment.
The example for Authorizer is a bit confusing as it doesn't use the pass parameter, which would result in a linter warning in a real Go program (pass declared and not used).
To make the example clearer and more practical, I suggest modifying it to demonstrate a simple custom validation that uses both the username and password.
| Authorizer: func(user, pass string, c fiber.Ctx) bool { | |
| if user == "john" && pass == "doe" { | |
| return true | |
| } | |
| if user == "admin" && pass == "123456" { | |
| return true | |
| } | |
| return false | |
| // custom validation logic | |
| return (user == "john" || user == "admin") | |
| }, | |
| Authorizer: func(user, pass string, c fiber.Ctx) bool { | |
| // custom validation logic | |
| if user == "admin" && pass == "supersecret" { | |
| return true | |
| } | |
| return false | |
| }, |
There was a problem hiding this comment.
Pull Request Overview
This PR implements support for hashed password authentication in the BasicAuth middleware, replacing plaintext password storage with secure hashing mechanisms. The change enhances security by requiring pre-hashed passwords and supports multiple hash algorithms including SHA-256, SHA-512, SHA-1, MD5, and bcrypt.
Key changes:
- Replaced plaintext password comparison with hash verification using multiple algorithms
- Added default SHA-256 hashing when no prefix is provided
- Updated all tests to use hashed passwords instead of plaintext
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| middleware/basicauth/config.go | Implements hash parsing and verification logic with support for multiple algorithms |
| middleware/basicauth/basicauth_test.go | Updates all test cases to use hashed passwords and adds comprehensive hash variant testing |
| docs/whats_new.md | Documents the breaking change requiring hashed passwords |
| docs/middleware/basicauth.md | Updates documentation with hash format examples and requirements |
| for u, hpw := range cfg.Users { | ||
| v, err := parseHashedPassword(hpw) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
Using panic() during configuration parsing can crash the entire application. Consider returning an error from configDefault() instead and handling it gracefully in the calling code.
Copilot uses AI. Check for mistakes.
| b, err := hex.DecodeString(h) | ||
| if err != nil || len(b) != sha256.Size { | ||
| if b, err = base64.StdEncoding.DecodeString(h); err != nil { | ||
| return nil, fmt.Errorf("decode SHA256 password: %w", err) | ||
| } | ||
| if len(b) != sha256.Size { | ||
| return nil, errors.New("decode SHA256 password: invalid length") | ||
| } | ||
| } | ||
| return func(p string) bool { | ||
| sum := sha256.Sum256([]byte(p)) | ||
| return subtle.ConstantTimeCompare(sum[:], b) == 1 | ||
| }, nil |
There was a problem hiding this comment.
The fallback logic allows both hex and base64 decoding without clear validation. This could accept malformed input that happens to decode successfully. Consider being more explicit about the expected format or adding stricter validation.
| b, err := hex.DecodeString(h) | |
| if err != nil || len(b) != sha256.Size { | |
| if b, err = base64.StdEncoding.DecodeString(h); err != nil { | |
| return nil, fmt.Errorf("decode SHA256 password: %w", err) | |
| } | |
| if len(b) != sha256.Size { | |
| return nil, errors.New("decode SHA256 password: invalid length") | |
| } | |
| } | |
| return func(p string) bool { | |
| sum := sha256.Sum256([]byte(p)) | |
| return subtle.ConstantTimeCompare(sum[:], b) == 1 | |
| }, nil | |
| // Validate if the input is a valid hex string | |
| if _, err := hex.DecodeString(h); err == nil { | |
| b, err := hex.DecodeString(h) | |
| if err != nil || len(b) != sha256.Size { | |
| return nil, errors.New("decode SHA256 password: invalid hex format or length") | |
| } | |
| return func(p string) bool { | |
| sum := sha256.Sum256([]byte(p)) | |
| return subtle.ConstantTimeCompare(sum[:], b) == 1 | |
| }, nil | |
| } | |
| // Validate if the input is a valid base64 string | |
| if _, err := base64.StdEncoding.DecodeString(h); err == nil { | |
| b, err := base64.StdEncoding.DecodeString(h) | |
| if err != nil || len(b) != sha256.Size { | |
| return nil, errors.New("decode SHA256 password: invalid base64 format or length") | |
| } | |
| return func(p string) bool { | |
| sum := sha256.Sum256([]byte(p)) | |
| return subtle.ConstantTimeCompare(sum[:], b) == 1 | |
| }, nil | |
| } | |
| // If neither hex nor base64 validation passes, return an error | |
| return nil, errors.New("decode SHA256 password: invalid format") |
Copilot uses AI. Check for mistakes.
| {"bcrypt", string(bcryptHash)}, | ||
| {"sha512", sha512Hash(pass)}, | ||
| {"sha256", sha256Hash(pass)}, | ||
| {"sha256-hex", func() string { h := sha256.Sum256([]byte(pass)); return hex.EncodeToString(h[:]) }()}, |
There was a problem hiding this comment.
[nitpick] The inline function makes the test case definition hard to read. Consider extracting this to a helper function like the other hash functions or computing it before the test cases array.
| {"sha256-hex", func() string { h := sha256.Sum256([]byte(pass)); return hex.EncodeToString(h[:]) }()}, | |
| {"sha256-hex", sha256HexHash(pass)}, |
Copilot uses AI. Check for mistakes.
| {"bcrypt", string(bcryptHash)}, | ||
| {"sha512", sha512Hash(pass)}, | ||
| {"sha256", sha256Hash(pass)}, | ||
| {"sha256-hex", func() string { h := sha256.Sum256([]byte(pass)); return hex.EncodeToString(h[:]) }()}, |
There was a problem hiding this comment.
[nitpick] Duplicate inline function logic from the previous test. Consider creating a reusable helper function or variable to avoid code duplication.
| {"sha256-hex", func() string { h := sha256.Sum256([]byte(pass)); return hex.EncodeToString(h[:]) }()}, | |
| {"sha256-hex", sha256HexHash(pass)}, |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
middleware/basicauth/config.go (3)
4-5: Security concern: Support for weak hash algorithms MD5 and SHA1These algorithms are cryptographically weak and vulnerable to attacks. While marked for compatibility, their use should be strongly discouraged in the documentation.
117-120: Consider returning an error instead of panicUsing panic() during configuration parsing can crash the entire application. Consider returning an error from configDefault() and handling it gracefully in the calling code.
187-199: Simplify the default case logic for better readabilityThe nested logic for parsing hex and base64 encoded hashes is difficult to follow and hides the original error from hex.DecodeString. Consider refactoring to handle hex and base64 decoding separately.
docs/whats_new.md (1)
1057-1057: Add security warnings for MD5 and SHA-1 hash supportThe documentation mentions support for
{MD5}and{SHA}(SHA-1) without security warnings. Please clarify these are supported only for backward compatibility and add strong recommendations against their use.Also applies to: 1950-1950
middleware/basicauth/basicauth_test.go (1)
400-400: Extract inline SHA256 hex generationThe inline anonymous functions make test cases harder to read. Consider creating a helper function or using hex.EncodeToString directly.
+func sha256HexHash(p string) string { + sum := sha256.Sum256([]byte(p)) + return hex.EncodeToString(sum[:]) +} // In test cases: -{"sha256-hex", func() string { h := sha256.Sum256([]byte(pass)); return hex.EncodeToString(h[:]) }()}, +{"sha256-hex", sha256HexHash(pass)},Also applies to: 432-432
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/middleware/basicauth.md(3 hunks)docs/whats_new.md(2 hunks)middleware/basicauth/basicauth_test.go(14 hunks)middleware/basicauth/config.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
docs/**
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/middleware/basicauth.mddocs/whats_new.md
🧠 Learnings (5)
📓 Common learnings
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Feature request #3224 has been created to add support for square bracket notation and comma-separated values in multipart form data in Fiber, while maintaining binary data transfer capabilities. This would bring parity with the existing form-urlencoded functionality.
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: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
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.
Learnt from: gaby
PR: gofiber/fiber#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.
middleware/basicauth/config.go (6)
Learnt from: sixcolors
PR: #3016
File: middleware/session/store.go:257-331
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the GetByID method, when handling a decode error, use decodeErr in the error message to accurately report the decoding failure.
Learnt from: sixcolors
PR: #3016
File: middleware/session/store.go:257-331
Timestamp: 2024-10-02T22:57:30.345Z
Learning: In the GetByID method, when handling a decode error, use decodeErr in the error message to accurately report the decoding failure.
Learnt from: mdelapenya
PR: #3434
File: app.go:623-636
Timestamp: 2025-05-08T08:14:37.302Z
Learning: In the gofiber/fiber framework, service startup failures should panic rather than allowing the application to continue running with degraded functionality, as this is the agreed-upon design decision.
Learnt from: sixcolors
PR: #3625
File: middleware/session/config.go:57-58
Timestamp: 2025-07-27T17:28:53.403Z
Learning: In the session middleware Config struct, the Extractor field uses function closures (like FromCookie(key)), making it impossible to introspect extractor parameters at runtime for validation purposes without complex reflection techniques.
Learnt from: sixcolors
PR: #3016
File: middleware/session/config.go:16-26
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the session middleware Config struct, Store is backed by fiber.Storage; they are different entities serving distinct purposes in session management.
Learnt from: sixcolors
PR: #3016
File: middleware/session/config.go:16-26
Timestamp: 2024-09-25T16:17:00.969Z
Learning: In the session middleware Config struct, Store is backed by fiber.Storage; they are different entities serving distinct purposes in session management.
docs/middleware/basicauth.md (3)
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 both EncryptCookie and DecryptCookie functions have been added to ensure robust validation and prevent potential runtime errors.
Learnt from: gaby
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: mdelapenya
PR: #3434
File: docs/api/services.md:39-43
Timestamp: 2025-05-07T13:07:33.899Z
Learning: When documenting Go interface methods in the Fiber project, avoid showing method signatures with the interface type as the receiver (e.g., func (d *Service) Method()) since interfaces cannot be used as receivers in Go. Instead, show just the method signature without a receiver or use a placeholder implementation name.
docs/whats_new.md (8)
Learnt from: gaby
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
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: hcancelik
PR: #3036
File: docs/middleware/cache.md:103-103
Timestamp: 2024-06-15T19:26:06.401Z
Learning: There are no hard tabs in the lines 100 to 105 of the docs/middleware/cache.md file. Future comments about formatting should accurately reflect the actual content.
Learnt from: hcancelik
PR: #3036
File: docs/middleware/cache.md:103-103
Timestamp: 2024-10-08T19:06:06.583Z
Learning: There are no hard tabs in the lines 100 to 105 of the docs/middleware/cache.md file. Future comments about formatting should accurately reflect the actual content.
Learnt from: ckoch786
PR: #3230
File: docs/whats_new.md:944-951
Timestamp: 2024-12-15T19:56:45.935Z
Learning: Detailed usage examples and explanations for new methods like RemoveRoute and RemoveRouteByName are documented in docs/api/app.md, so it's unnecessary to duplicate them in docs/whats_new.md.
Learnt from: sixcolors
PR: #3598
File: docs/middleware/csrf.md:37-42
Timestamp: 2025-07-19T14:06:29.884Z
Learning: In Fiber v3, the CookieSameSite constants use lowercase values: CookieSameSiteLaxMode = "lax", CookieSameSiteStrictMode = "strict", CookieSameSiteNoneMode = "none". Documentation examples should use lowercase string values or the typed constants, not capitalized strings like "Lax".
Learnt from: sixcolors
PR: #3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
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.
Learnt from: sixcolors
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.
middleware/basicauth/basicauth_test.go (28)
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 both EncryptCookie and DecryptCookie functions have been added to ensure robust validation and prevent potential runtime errors.
Learnt from: gaby
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: sixcolors
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.
Learnt from: sixcolors
PR: #3051
File: middleware/session/session.go:215-216
Timestamp: 2024-10-08T19:06:06.583Z
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.
Learnt from: gaby
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
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
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
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: sixcolors
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.
Learnt from: sixcolors
PR: #2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Learnt from: efectn
PR: #3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the require methods from the testify package for assertions instead of manual comparisons and calls to t.Fatal or t.Fatalf.
Learnt from: sixcolors
PR: #3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Learnt from: sixcolors
PR: #3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
Learnt from: sixcolors
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.
Learnt from: sixcolors
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.
Learnt from: sixcolors
PR: #3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
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.
Learnt from: sixcolors
PR: #3016
File: middleware/session/config.go:16-26
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the session middleware Config struct, Store is backed by fiber.Storage; they are different entities serving distinct purposes in session management.
Learnt from: sixcolors
PR: #3016
File: middleware/session/config.go:16-26
Timestamp: 2024-09-25T16:17:00.969Z
Learning: In the session middleware Config struct, Store is backed by fiber.Storage; they are different entities serving distinct purposes in session management.
Learnt from: sixcolors
PR: #3016
File: middleware/csrf/csrf_test.go:164-165
Timestamp: 2024-09-25T17:08:07.693Z
Learning: In the Test_CSRF_WithSession_Middleware function, calling session.NewWithStore() without arguments is acceptable, as the default configuration is sufficient.
Learnt from: gaby
PR: #3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the middleware/adaptor/adaptor.go file of the Fiber framework, when updating context handling, replacing c.Context() with c.RequestCtx() is appropriate to access the fasthttp.RequestCtx.
Learnt from: efectn
PR: #3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the Test_App_ShutdownWithContext function in app_test.go, the clientDone channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional time.Sleep calls.
Learnt from: sixcolors
PR: #3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In DefaultErrorHandler(c *fiber.Ctx, err error), since c is a pointer to an interface, we need to dereference *c when calling interface methods like SendStatus.
Learnt from: sixcolors
PR: #3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In DefaultErrorHandler(c *fiber.Ctx, err error), since c is a pointer to an interface, we need to dereference *c when calling interface methods like SendStatus.
Learnt from: ReneWerner87
PR: #3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the Test method in app.go.
Learnt from: gaby
PR: #3170
File: ctx_test.go:1721-1724
Timestamp: 2024-10-16T12:12:30.506Z
Learning: In the Go unit tests in ctx_test.go, it is acceptable to use invalid CIDR notation such as "0.0.0.1/31junk" for testing purposes.
Learnt from: sixcolors
PR: #3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-09-25T17:05:06.991Z
Learning: When testing session IdleTimeout expiration, it's acceptable to use time.Sleep to simulate the passage of time in tests.
Learnt from: sixcolors
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.
Learnt from: sixcolors
PR: #3625
File: middleware/session/config.go:57-58
Timestamp: 2025-07-27T17:28:53.403Z
Learning: In the session middleware Config struct, the Extractor field uses function closures (like FromCookie(key)), making it impossible to introspect extractor parameters at runtime for validation purposes without complex reflection techniques.
🔇 Additional comments (4)
middleware/basicauth/basicauth_test.go (4)
21-39: Well-structured helper functionsGood implementation of helper functions to reduce code duplication across tests. This addresses the previous concern about repeated SHA256 hash creation logic.
61-63: Proper error handling for bcryptGood job adding error handling for bcrypt.GenerateFromPassword using require.NoError.
355-386: Comprehensive test coverage for parseHashedPasswordExcellent test coverage that validates all supported hash formats and includes both positive and negative test cases.
388-449: Excellent integration test coverageThe HashVariants tests provide comprehensive coverage of the BasicAuth middleware with all supported hash formats, testing both successful and failed authentication scenarios.
| - `"{SHA512}"`, `"{SHA256}"`, or `"{SHA}"` followed by a base64 encoded digest | ||
| - `"{MD5}"` followed by a base64 encoded digest | ||
| - standard bcrypt strings beginning with `$2` |
There was a problem hiding this comment.
Add security warnings for weak hash algorithms
The documentation mentions {SHA} (SHA-1) and {MD5} without any security warnings. Please add a clear warning that these algorithms are insecure and supported only for backward compatibility. Recommend using bcrypt or SHA-256/SHA-512 instead.
- `"{SHA512}"`, `"{SHA256}"`, or `"{SHA}"` followed by a base64 encoded digest
- `"{MD5}"` followed by a base64 encoded digest
+ `"{SHA512}"`, `"{SHA256}"` followed by a base64 encoded digest
+ `"{SHA}"` (SHA-1) followed by a base64 encoded digest (**insecure, use only for backward compatibility**)
+ `"{MD5}"` followed by a base64 encoded digest (**insecure, use only for backward compatibility**)
- standard bcrypt strings beginning with `$2`
+ standard bcrypt strings beginning with `$2` (**recommended**)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `"{SHA512}"`, `"{SHA256}"`, or `"{SHA}"` followed by a base64 encoded digest | |
| - `"{MD5}"` followed by a base64 encoded digest | |
| - standard bcrypt strings beginning with `$2` | |
| - `"{SHA512}"`, `"{SHA256}"` followed by a base64 encoded digest | |
| - `"{SHA}"` (SHA-1) followed by a base64 encoded digest (**insecure, use only for backward compatibility**) | |
| - `"{MD5}"` followed by a base64 encoded digest (**insecure, use only for backward compatibility**) | |
| - standard bcrypt strings beginning with `$2` (**recommended**) |
🤖 Prompt for AI Agents
In docs/middleware/basicauth.md around lines 69 to 71, add a clear security
warning that the `{SHA}` (SHA-1) and `{MD5}` hash algorithms are insecure and
only supported for backward compatibility. Update the documentation to recommend
using stronger algorithms like bcrypt or SHA-256/SHA-512 instead, emphasizing
best security practices.
|
cli migration gofiber/cli#150 |
Summary