[PM-6631] Handle Fido2VerificationException during passkey attestation and assertion#59
Open
lizard-boy wants to merge 8 commits into
Open
[PM-6631] Handle Fido2VerificationException during passkey attestation and assertion#59lizard-boy wants to merge 8 commits into
lizard-boy wants to merge 8 commits into
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
Comment on lines
+50
to
+59
| Fido2NetLib.Objects.AssertionVerificationResult assertionVerificationResult = null; | ||
| try | ||
| { | ||
| assertionVerificationResult = await _fido2.MakeAssertionAsync( | ||
| assertionResponse, options, credentialPublicKey, (uint)credential.Counter, callback); | ||
| } | ||
| catch (Fido2VerificationException) | ||
| { | ||
| ThrowInvalidCredentialException(); | ||
| } |
There was a problem hiding this comment.
style: The catch block swallows the specific exception details. Consider logging the exception message or type for better diagnostics.
Comment on lines
62
to
63
| credential.Counter = (int)assertionVerificationResult.Counter; | ||
| await _webAuthnCredentialRepository.ReplaceAsync(credential); |
There was a problem hiding this comment.
logic: Ensure that assertionVerificationResult is not null before accessing its properties.
Comment on lines
+73
to
+76
| private void ThrowInvalidCredentialException() | ||
| { | ||
| throw new BadRequestException("Invalid credential."); | ||
| } |
There was a problem hiding this comment.
style: Consider making this method more flexible by allowing custom error messages or including more context in the exception.
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.
Type of change
Objective
In bitwarden#3615 we handled the
Fido2VerificationExceptionwhen asserting a WebAuthn credential for 2FA.In this PR, we address the
MakeNewCredentialAsyncmethods similarly, as well as theMakeAssertionAsyncwhen asserting a WebAuthn credential for login, which was missed in bitwarden#3615 .📓 We have https://bitwarden.atlassian.net/browse/PM-4172 in the backlog to consolidate the implementations, at which point we should consider an abstraction.
Code changes
BadRequestExceptioninstead of the unhandled exception returned previously. This will be handled on the client, as it is the pattern already established in the class for communicating assertion errors.falsealong with a log message. I did this instead of throwing aBadRequestExceptionas this is the pattern already established in this command for handling invalid data. I added a log here as returningfalsegives no indication of the root cause.falsealong with a log message. I did this instead of throwing aBadRequestExceptionas this is the pattern already established in this command for handling invalid data. I added a log here as returningfalsegives no indication of the root cause.Before you submit
dotnet format --verify-no-changes) (required)Greptile Summary
This pull request addresses exception handling for WebAuthn credential creation and assertion, focusing on improving error management in three key files:
AssertWebAuthnLoginCredentialCommand.csto handleFido2VerificationExceptionduring login credential assertionCreateWebAuthnLoginCredentialCommand.csfor WebAuthn credential creation, returning false and logging errors on exceptionUserService.csto catchFido2VerificationExceptionduring WebAuthn registration, logging the error and returning false on failureBadRequestExceptionthrowing inAssertWebAuthnLoginCredentialCommand.csfor consistent error handling