Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set user_handle to nil for non-string data type #392

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

elquimista
Copy link

Current code expects userHandle value in string data type and it seems to work fine at least for desktop web browsers. I tested with Yubikey 5C NFC and it returns an empty string '' for userHandle.
However, when I tested on mobile browsers (e.g., iOS Safari), it is returned with an empty object {} rather than an empty string, which causes an error in the backend code trying to encode a Hash object instead of a String object.

Screenshot 2023-06-07 at 6 25 10 PM

Because of this, I had to do a simple workaround temporarily in one of my client application.

I don't know if this suggestion is a right approach but at least it fixes my problem. Please let me know if there is a better approach. I am not an expert when it comes to webauthn.

@elquimista
Copy link
Author

@brauliomartinezlm - I see several PRs open with no response for a while. Is this going to be taken care of at all?

Copy link
Contributor

@santiagorodriguez96 santiagorodriguez96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @elquimista!!! Thank you so much for reporting this and taking your time to fix it! 🙌

Sorry for the late response.

Code looks good to me. Well done! 💪

lib/webauthn/authenticator_assertion_response.rb Outdated Show resolved Hide resolved
@@ -14,7 +14,7 @@ def self.from_client(response, relying_party: WebAuthn.configuration.relying_par
encoder = relying_party.encoder

user_handle =
if response["userHandle"] && String === response["userHandle"]
if response["userHandle"] && response["userHandle"].is_a? String
Copy link
Contributor

@santiagorodriguez96 santiagorodriguez96 Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that we have a syntax error here 😕

Suggested change
if response["userHandle"] && response["userHandle"].is_a? String
if response["userHandle"] && response["userHandle"].is_a?(String)

@bdewater
Copy link
Collaborator

bdewater commented Nov 10, 2023

🤔 returning {} for userHandle does not conform to the specification, it should be an ArrayBuffer according to AuthenticatorAssertionResponse interface.

I noticed in the fix you referenced that for other ArrayBuffers (authenticatorData, signature) in the AuthenticatorAssertionResponse you wrap these in a bufferToBase64url method call. Perhaps that is what it missing? That seems to line up with what the webauthn-json library is doing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants