Fix: Prevent predictable token generation on entropy exhaustion (Issue #65)#128
Fix: Prevent predictable token generation on entropy exhaustion (Issue #65)#128diksha78dev wants to merge 1 commit into
Conversation
|
All contributors have signed the CLA. ✅ Thank you! |
📝 WalkthroughWalkthrough
Changesrand.Read error propagation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/service/token_service_test.go (1)
64-75: 💤 Low valueConsider adding test coverage for the error path.
The test currently verifies successful token generation but doesn't cover the case where
rand.Readfails. While mockingcrypto/randin Go is non-trivial, consider adding a test case that validates the error return behavior to ensure the security fix is fully covered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/token_service_test.go` around lines 64 - 75, Add additional test cases to TestTokenService_GenerateRandomString to verify error handling when rand.Read fails. Create separate test subtests or test functions that validate the error return path of the GenerateRandomString method, ensuring it properly handles and returns errors when random number generation fails. While mocking the crypto/rand package is challenging in Go, implement the error test case using available testing techniques to ensure the error handling logic in GenerateRandomString is validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/service/auth_service.go`:
- Around line 125-128: The error returned when GenerateRandomString fails
reveals user existence to attackers, breaking email enumeration protection.
Instead of returning the error directly when GenerateRandomString fails, log the
error internally and return the same nil response that is returned when a user
doesn't exist. This ensures attackers cannot distinguish between a non-existent
user and a token generation failure by observing different response behaviors.
Modify the error handling block after the GenerateRandomString call to log the
error and return nil instead of propagating the error.
---
Nitpick comments:
In `@internal/service/token_service_test.go`:
- Around line 64-75: Add additional test cases to
TestTokenService_GenerateRandomString to verify error handling when rand.Read
fails. Create separate test subtests or test functions that validate the error
return path of the GenerateRandomString method, ensuring it properly handles and
returns errors when random number generation fails. While mocking the
crypto/rand package is challenging in Go, implement the error test case using
available testing techniques to ensure the error handling logic in
GenerateRandomString is validated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f9af597-4344-4546-b308-f19d27ae1322
📒 Files selected for processing (3)
internal/service/auth_service.gointernal/service/token_helper.gointernal/service/token_service_test.go
| randomToken, err := s.tokenService.GenerateRandomString(32) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Email enumeration vulnerability: token generation error reveals user existence.
Returning the error directly when GenerateRandomString fails breaks the email enumeration protection. An attacker can distinguish between:
- User doesn't exist →
nilreturn (line 118) - User exists but token generation failed → error return (line 127)
This allows attackers to enumerate valid email addresses by observing different response behaviors.
🔒 Proposed fix to preserve email enumeration protection
// Create new reset token
randomToken, err := s.tokenService.GenerateRandomString(32)
if err != nil {
- return err
+ // Return nil to prevent email enumeration (same as user not found case)
+ // Log the error internally for monitoring
+ return nil
}📝 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.
| randomToken, err := s.tokenService.GenerateRandomString(32) | |
| if err != nil { | |
| return err | |
| } | |
| randomToken, err := s.tokenService.GenerateRandomString(32) | |
| if err != nil { | |
| // Return nil to prevent email enumeration (same as user not found case) | |
| // Log the error internally for monitoring | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/auth_service.go` around lines 125 - 128, The error returned
when GenerateRandomString fails reveals user existence to attackers, breaking
email enumeration protection. Instead of returning the error directly when
GenerateRandomString fails, log the error internally and return the same nil
response that is returned when a user doesn't exist. This ensures attackers
cannot distinguish between a non-existent user and a token generation failure by
observing different response behaviors. Modify the error handling block after
the GenerateRandomString call to log the error and return nil instead of
propagating the error.
|
I have read the CLA and agree to its terms. |
|
@diksha78dev handle the coderabbit suugestion |
|
@diksha78dev till when will it be ready |
2 participants
Closes #65.
Root Cause:
GenerateRandomStringsilently ignored theerrorreturned bycrypto/rand.Read. If the system exhausted its entropy pool,rand.Readwould fail, resulting in a predictable, zero-filled byte array being base64-encoded and used as secure tokens.Changes:
token_helper.goto return(string, error)and explicitly fail onrand.Readerrors.auth_service.goto handle and propagate the generation error.token_service_test.goto assert the new error signature.All tests pass. No unrelated formatting changes included.
Checklist
I have read the CLA and agree to its terms.on this PR.Summary by CodeRabbit