fix(login): When searching for user credentials, terminate the loop i… #331
Conversation
…mmediately once valid credentials are found.
WalkthroughThe changes involve a modification to the Changes
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| if credentialsOwned { | ||
| break | ||
| } |
There was a problem hiding this comment.
Critical: Break statement is in the wrong loop scope
The current implementation breaks from the inner loop when a credential is found, but this could lead to incomplete validation. The code needs to verify that the user owns ALL credentials from the allowed list, not just one of them.
Here's the corrected implementation:
for _, allowedCredentialID := range session.AllowedCredentialIDs {
for _, credential = range credentials {
if bytes.Equal(credential.ID, allowedCredentialID) {
credentialsOwned = true
break
}
credentialsOwned = false
}
- if credentialsOwned {
- break
- }
+ if !credentialsOwned {
+ return nil, protocol.ErrBadRequest.WithDetails("User does not own all credentials from the allowedCredentialList")
+ }
}
-if !credentialsOwned {
- return nil, protocol.ErrBadRequest.WithDetails("User does not own all credentials from the allowedCredentialList")
-}This change:
- Continues to verify all allowed credentials
- Fails fast if any credential is not owned by the user
- Maintains the security requirement that the user must own ALL credentials in the allowed list
Committable suggestion skipped: line range outside the PR's diff.
fix(login): When searching for user credentials, terminate the loop immediately once valid credentials are found.