🧹 chore: Remove SHA-1/MD5 support in BasicAuth middleware #3634
Conversation
WalkthroughSupport for SHA1 and MD5 password hashes has been removed from the BasicAuth middleware. Documentation and tests have been updated to reflect that only SHA256, SHA512, and bcrypt password hash formats are now supported. Code handling and test cases for SHA1 and MD5 have been deleted. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
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 (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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 prepared this pull request to enhance the security of the BasicAuth middleware by removing support for the cryptographically weak SHA-1 and MD5 hashing algorithms. This change ensures that only stronger, more secure hashing methods are used for password authentication. Alongside the core logic update, I've also clarified the documentation to explicitly state that the Authorizer function receives a hashed password, and I've cleaned up associated test cases.
Highlights
- Security Enhancement: Deprecated SHA-1 and MD5 hashing algorithms have been removed from the BasicAuth middleware, improving the security posture by enforcing stronger hashing standards.
- Documentation Update: The documentation for BasicAuth, including
basicauth.mdandwhats_new.md, has been updated to reflect the removal of SHA-1 and MD5 support and to clarify that theAuthorizerfunction now explicitly receives a hashed password. - Codebase Cleanup: Related test cases and helper functions for SHA-1 and MD5 have been removed from
basicauth_test.go, and theconfig.gofile no longer imports or processes these outdated hash types.
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. ↩
There was a problem hiding this comment.
Code Review
This pull request correctly removes support for the insecure SHA-1 and MD5 hashing algorithms from the BasicAuth middleware, which is an important security enhancement. The changes are consistently applied throughout the code, tests, and documentation. However, I've found a critical issue in the updated documentation for the Authorizer function. Both the code comments and the markdown documentation incorrectly state that the Authorizer receives a hashed password, when it actually receives the plaintext password provided by the client. This is a significant documentation bug that could lead developers to implement insecure custom authorizers. I've left detailed comments on the specific lines with suggestions for correction.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3634 +/- ##
==========================================
+ Coverage 91.78% 91.82% +0.03%
==========================================
Files 113 113
Lines 11444 11426 -18
==========================================
- Hits 10504 10492 -12
+ Misses 675 671 -4
+ Partials 265 263 -2
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:
|
gaby
changed the title
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
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 removes support for insecure SHA-1 and MD5 hashing algorithms from the BasicAuth middleware as part of security hardening. The change maintains support for more secure alternatives like SHA-256, SHA-512, and bcrypt while dropping the weaker cryptographic functions.
Key changes:
- Remove SHA-1 and MD5 parsing logic from the password hash detection code
- Clean up related test helper functions and test cases
- Update documentation to reflect the removed hash algorithm support
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| middleware/basicauth/config.go | Removes SHA-1/MD5 imports and password parsing logic for these weak hash algorithms |
| middleware/basicauth/basicauth_test.go | Removes SHA-1/MD5 test helper functions and eliminates related test cases |
| docs/whats_new.md | Updates documentation to remove references to SHA-1/MD5 support |
| docs/middleware/basicauth.md | Updates middleware documentation to reflect removed hash algorithm support |
| @@ -66,8 +66,7 @@ Getting the username and password | |||
| Passwords must be supplied in pre-hashed form. The middleware detects the | |||
| hashing algorithm from a prefix: | |||
There was a problem hiding this comment.
The documentation should clarify that the Authorizer function receives the hashed password, not the plaintext password, as mentioned in the PR description. Consider adding a note about this important detail for developers implementing custom authorization logic.
| hashing algorithm from a prefix: | |
| hashing algorithm from a prefix. The `Authorizer` function receives the hashed password, not the plaintext password. Developers should ensure their custom logic accounts for this. |
Copilot uses AI. Check for mistakes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
docs/whats_new.md (1)
1948-1954: Clarify thatAuthorizernow receives hashed passwordThe text introduces the new requirement that passwords must be pre-hashed, but it doesn’t explicitly state that the
Authorizercallback is now invoked with that hashed value (per the implementation change in PR #3634). This is an essential behavioural change and must be called out here to prevent existing handlers from blindly comparing plaintext strings.Please add one sentence such as:
“Note: the
Authorizerfunction now receives the hashed password, not the plaintext sent by the client.”docs/middleware/basicauth.md (2)
66-74: Mismatch between documentation and runtime behaviour for hash detectionThe bullet list removes
{SHA}/{MD5}– good – but it still claims that absence of a prefix allows either hex or base64 encoded SHA-256.
Implementation (parseHashedPassword) only accepts the 64-byte hex form; base64 without a prefix is rejected and yields “invalid hash” (see middleware/basicauth/config.go).Diff suggestion:
-If no prefix is present the value is interpreted as a SHA-256 digest encoded in -hex or base64. +If no prefix is present the value is interpreted as a **hex-encoded** SHA-256 +digest (64 hexadecimal characters).This avoids confusing integrators who would otherwise provide an unsupported format.
88-95:StorePassword/Authorizerwording still says plaintextAfter PR #3634 the middleware hashes the provided password before:
- calling the custom
Authorizer- optionally storing it with
StorePasswordThe table still states “plaintext password”. Update both descriptions to “hashed password” (or “password hash”) to reflect actual behaviour and avoid security-sensitive mistakes.
🧹 Nitpick comments (1)
docs/middleware/basicauth.md (1)
52-58: ExampleAuthorizershould compare hashes, not usernames onlyThe custom
Authorizerin the example returnstruefor any request where the
user isjohnoradmin, regardless of the password. That defeats the
purpose of authentication and is dangerous to copy-paste.Replace with a realistic check, e.g.:
Authorizer: func(user, passHash string, _ fiber.Ctx) bool { expected, ok := map[string]string{ "john": "{SHA256}eZ75K…", "admin": "$2a$10$gTYwC…", }[user] return ok && passHash == expected },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/middleware/basicauth.md(1 hunks)docs/whats_new.md(1 hunks)middleware/basicauth/basicauth_test.go(0 hunks)middleware/basicauth/config.go(0 hunks)
💤 Files with no reviewable changes (2)
- middleware/basicauth/basicauth_test.go
- middleware/basicauth/config.go
🧰 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/whats_new.mddocs/middleware/basicauth.md
🧠 Learnings (3)
📓 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: ReneWerner87
PR: gofiber/fiber#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: 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.
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.
docs/whats_new.md (8)
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: sixcolors
PR: #3446
File: docs/middleware/logger.md:44-44
Timestamp: 2025-05-13T00:19:16.407Z
Learning: In documentation files for the Fiber framework, code examples are often partial and don't repeat import statements that were shown in earlier examples, focusing instead on demonstrating specific usage patterns.
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: 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: 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.
docs/middleware/basicauth.md (6)
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: 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: 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: 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.
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.
⏰ 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: repeated
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: Compare
- GitHub Check: Analyse
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 15ccbcf | Previous: 2e544ae | Ratio |
|---|---|---|---|
Benchmark_Ctx_Links - B/op |
1 B/op |
0 B/op |
+∞ |
This comment was automatically generated by workflow using github-action-benchmark.
Summary