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

Check if user handle is an empty string #148

Merged

Conversation

lykahb
Copy link
Contributor

@lykahb lykahb commented Apr 22, 2022

This fixes authentication on Safari #147

@ErinvanderVeen ErinvanderVeen self-requested a review April 25, 2022 07:11
@infinisil
Copy link
Member

This is strictly non-conforming behavior:

The user handle MUST NOT be empty, though it MAY be null.

@lykahb Can you report this to Apple? If it's a bug, they should fix it.

I think this change is fine if you add a code comment how this is a Safari-specific workaround, but not actually conform (giving the above spec sentence as a reference).

@lykahb
Copy link
Contributor Author

lykahb commented Apr 25, 2022

@infinisil Updated the comment, it includes the reference to Safari Webkit bug report now.

I think that the spec is not clear, though: the link is to the user handle entry under "5.4.3. User Account Parameters for Credential Generation (dictionary PublicKeyCredentialUserEntity)". The 5.2.2. Web Authentication Assertion does not explicitly say that the user handle may not be an empty string. It may also be worth opening an issue for the W3C spec to clarify the acceptable user handle values in the assertion.

@infinisil
Copy link
Member

infinisil commented Apr 25, 2022

@lykahb Nice, thanks for that webkit bug report! Good spot regarding the specification as well, I opened an issue for that, see the above link. Let's also link to the webauthn issue from the code comment.

@lykahb
Copy link
Contributor Author

lykahb commented Apr 26, 2022

Can anyone merge this if the changes look good? This would enable me to switch to using upstream again.

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Yup this looks good now, thanks!

@infinisil infinisil merged commit abc45eb into tweag:master Apr 26, 2022
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.

None yet

3 participants