Suppressing unexpected error with `WebAuthn::PublicKeyCredentialWithAttestation#verify`

[soartec-lab]

WebAuthn::PublicKeyCredentialWithAttestation#verify causes NoMethodError if challenge is nil. as below:

undefined method `end_with?' for nil:NilClass

if !str.end_with?("=") && str.length % 4 != 0
           ^^^^^^^^^^
base64.rb:102:in `urlsafe_decode64'

Since challenge is assumed to refer to a value stored in temporary storage such as session, there may be cases where it does not exist by accident. This change will make the error messages that occur when the above problem occurs more user-friendly and will help users of this library understand the errors.

[santiagorodriguez96]

I wonder how it is possible in a real scenario for the challenge to be nil. Did you run into this issue in a prod environment?

Code looks good to me 🙂

[soartec-lab]

@santiagorodriguez96

Thank you for review.

Yes, this issue is an approach to a phenomenon that occurs when operating a production environment.

For example, if you are using external storage for session store. When using an in-memory data store, data may be deleted if the limit is reached. Also, since external data stores are accessed via the network, errors are expected.

This is to help troubleshoot unforeseen circumstances and is not something that occurs in routine scenarios. However, since this is a function that handles authentication, I would like to troubleshoot this issue smoothly.

[soartec-lab]

The Rubocopoffence was occurring so I fixed it below:

https://github.com/cedarcode/webauthn-ruby/pull/413/commits/bf16de52f70ab859f5349c628dcddfabcf8607f2

If necessary, squash all commits after all reviews are complete.

[santiagorodriguez96]

I'm just realizing that we should do this same check in lib/webauthn/public_key_credential_with_assertion.rb. Do you mind?

[soartec-lab]

@santiagorodriguez96

of course. Thank you for letting me know.

I fixed it with the commit bellow:

https://github.com/cedarcode/webauthn-ruby/pull/413/commits/9f86fe4ac0cc4ea2ef588883188bcdccec3e20ab

WebAuthn::PublicKeyCredentialWithAssertion and WebAuthn::PublicKeyCredentialWithAttestation both inherit from WebAuthn::PublicKeyCredential, so the checking process has been moved to WebAuthn::PublicKeyCredential. At the same time, we have changed the argument of WebAuthn::PublicKeyCredential#verify to receive challenge.

[soartec-lab]

@santiagorodriguez96

Thank you very much for giving me a good time with this PR. I am replying to your comment, so please check it as well.

Also, does this PR commit require squash? Looking at other PRs, it seems that squash of commits is not necessary, but if it is necessary, please let me know.

[santiagorodriguez96]

@santiagorodriguez96

Thank you very much for giving me a good time with this PR. I am replying to your comment, so please check it as well.

The same goes for you! Sorry for the back and forth, I'm lately getting back to maintaining the gem, so I'm still a little bit rough 🙂

Also, does this PR commit require squash? Looking at other PRs, it seems that squash of commits is not necessary, but if it is necessary, please let me know.

I can just Squash and merge and it should be good! 🙂