fix: use constant time in user checks#1004
Merged
Merged
Conversation
dosubot
Bot
added
the
size:S
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAuthService now initializes a bcrypt dummy hash and returns construction errors. Login and basic authentication invoke a dummy password check when user lookup fails with ChangesDummy password authentication flow
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/service/auth_service.go (1)
165-168: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse the provided password for the dummy check.
Generating a random string for the dummy check consumes unnecessary entropy and slightly alters the execution time compared to hashing the actual password provided in the request. Accepting and comparing the user-provided password ensures the execution time identically matches a normal password validation failure.
internal/service/auth_service.go#L165-L168: update the definition tofunc (auth *AuthService) DummyPasswordCheck(providedPassword string)and replace[]byte(random)with[]byte(providedPassword).internal/controller/user_controller.go#L93-L93: pass the request password by callingcontroller.auth.DummyPasswordCheck(req.Password).internal/middleware/context_middleware.go#L247-L247: pass the basic auth password by callingm.auth.DummyPasswordCheck(password)(if addressing the major issue flagged here, update the gated call).🤖 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 165 - 168, Update AuthService.DummyPasswordCheck to accept providedPassword and compare the dummy hash against it instead of generating a random string. At internal/service/auth_service.go lines 165-168, change the method and comparison; at internal/controller/user_controller.go line 93, pass req.Password; and at internal/middleware/context_middleware.go line 247, pass password, including the gated call if applicable.
🤖 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/middleware/context_middleware.go`:
- Around line 246-249: Update the SearchUser error handling in the surrounding
middleware method so m.auth.DummyPasswordCheck() runs only when err matches
service.ErrUserNotFound. Preserve the existing wrapped error return for
not-found and all other search failures, while avoiding the CPU-intensive check
for database or connectivity errors.
In `@internal/service/auth_service.go`:
- Line 106: Update every NewAuthService call site, specifically the usages in
proxy_controller_test.go and user_controller_test.go, to receive and handle both
the returned service and error. Preserve each test’s existing setup while
propagating or asserting the constructor error appropriately so the tests
compile and fail clearly on initialization errors.
---
Nitpick comments:
In `@internal/service/auth_service.go`:
- Around line 165-168: Update AuthService.DummyPasswordCheck to accept
providedPassword and compare the dummy hash against it instead of generating a
random string. At internal/service/auth_service.go lines 165-168, change the
method and comparison; at internal/controller/user_controller.go line 93, pass
req.Password; and at internal/middleware/context_middleware.go line 247, pass
password, including the gated call if applicable.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 778a7065-caa3-49f3-9439-88de52f5d8ce
📒 Files selected for processing (3)
internal/controller/user_controller.gointernal/middleware/context_middleware.gointernal/service/auth_service.go
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 165-167: Update AuthService.DummyPasswordCheck to accept and hash
the supplied password instead of generating a random value; update
internal/service/auth_service.go lines 165-167 accordingly. Pass req.Password
from internal/controller/user_controller.go line 93 and password from
internal/middleware/context_middleware.go lines 248-250 into this method.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 093af0f1-87c9-4de8-bbc0-3eb77de336f1
📒 Files selected for processing (6)
internal/controller/proxy_controller_test.gointernal/controller/user_controller.gointernal/controller/user_controller_test.gointernal/middleware/context_middleware.gointernal/middleware/context_middleware_test.gointernal/service/auth_service.go
Rycochet
approved these changes
Jul 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Tests