Bug 232836 – [WebAuthn] challenge does not get passed to -[_WKWebAuthenticationPanel getAssertionWithChallenge:origin:options:completionHandler:]

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[[e-mail]]

rdar://85163927

[[e-mail]]

-[_WKWebAuthenticationPanel getAssertionWithChallenge:origin:options:completionHandler:] receives an empty challenge, causing _WKWebAuthenticationPanel to immediately close when using the new UNIFIED_ASC_AUTH_UI. 

This is likely because it’s not encoded/decoded by PublicKeyCredentialRequestOptions. Before this field was not used after xpc, but with the new UNIFIED_ASC_AUTH_UI it is.

[[e-mail]]

Created attachment 443592 [details]
Patch

[Brent Fulgham]

Comment on attachment 443592 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443592&action=review

> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:79
> +    BufferSource challenge; // challenge becomes challengeVector once it is passed to UIProcess.

I don't understand this. Do we meed to keep it as a Vector<uint8_t> in the UIProcess? Or do we only need that format when passing it into an API? Is this BufferSource just something given to us in an argument? Perhaps we should. convert directly to Vector when given this value, and not retain two copies (a BufferSource version, and a Vector<uint8_t> version).

[[e-mail]]

(In reply to Brent Fulgham from comment #3)
> Comment on attachment 443592 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443592&action=review
> 
> > Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:79
> > +    BufferSource challenge; // challenge becomes challengeVector once it is passed to UIProcess.
> 
> I don't understand this. Do we meed to keep it as a Vector<uint8_t> in the
> UIProcess? Or do we only need that format when passing it into an API? Is
> this BufferSource just something given to us in an argument? Perhaps we
> should. convert directly to Vector when given this value, and not retain two
> copies (a BufferSource version, and a Vector<uint8_t> version).

Yeah, the buffer source is from JS, directly from the IDL. Here, I'm copying what PublicKeyCredentialCreationOptions.h:55 does with id -> idVector. I don't know that we can directly create this as an vector as I'm not too familiar with the way it gets deserialized from the idl definition.

[Brent Fulgham]

Comment on attachment 443592 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443592&action=review

> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:150
> +    encoder.encodeFixedLengthData(challenge.data(), challenge.length(), 1);

So PublicKeyCredentialCreationOptions encodes the challenge (but not the challengeVector)...

> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:202
> +        return std::nullopt;

... and decodes the challengeVector (but not the challenge).

> Source/WebCore/Modules/webauthn/PublicKeyCredentialRequestOptions.h:63
> +    encoder.encodeFixedLengthData(challenge.data(), challenge.length(), 1);

And PublicKeyCredentialRequestOptions now encodes the challenge (but not the challengeVector)

> Source/WebCore/Modules/webauthn/PublicKeyCredentialRequestOptions.h:95
> +        return std::nullopt;

... and we decode the challengeVector (but not the challenge)

I think this is really hard to understand.

[Brent Fulgham]

Comment on attachment 443592 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443592&action=review

>>> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:79
>>> +    BufferSource challenge; // challenge becomes challengeVector once it is passed to UIProcess.
>> 
>> I don't understand this. Do we meed to keep it as a Vector<uint8_t> in the UIProcess? Or do we only need that format when passing it into an API? Is this BufferSource just something given to us in an argument? Perhaps we should. convert directly to Vector when given this value, and not retain two copies (a BufferSource version, and a Vector<uint8_t> version).
> 
> Yeah, the buffer source is from JS, directly from the IDL. Here, I'm copying what PublicKeyCredentialCreationOptions.h:55 does with id -> idVector. I don't know that we can directly create this as an vector as I'm not too familiar with the way it gets deserialized from the idl definition.

I really feel like we should have a clean way to serialize BufferSource across IPC. Let me check with Chris.

[Brent Fulgham]

Comment on attachment 443592 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443592&action=review

We should use an IPC::DataReference to hold/serialize the BufferSource contents. r- to make this change, as it will reduce the number of copies made during serialization. We should also do a follow-up patch to fix the bits of Jiewen's code where he did manual Vector<uint8_t> serialization.

>>>> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:79
>>>> +    BufferSource challenge; // challenge becomes challengeVector once it is passed to UIProcess.
>>> 
>>> I don't understand this. Do we meed to keep it as a Vector<uint8_t> in the UIProcess? Or do we only need that format when passing it into an API? Is this BufferSource just something given to us in an argument? Perhaps we should. convert directly to Vector when given this value, and not retain two copies (a BufferSource version, and a Vector<uint8_t> version).
>> 
>> Yeah, the buffer source is from JS, directly from the IDL. Here, I'm copying what PublicKeyCredentialCreationOptions.h:55 does with id -> idVector. I don't know that we can directly create this as an vector as I'm not too familiar with the way it gets deserialized from the idl definition.
> 
> I really feel like we should have a clean way to serialize BufferSource across IPC. Let me check with Chris.

Per Chris: We should be using an IPC::DataReference here, instead of BufferSource during serialization.

[Brent Fulgham]

Comment on attachment 443592 [details]
Patch

On second thought, let's handle all the weird BufferSource encode/decode in a separate patch.

[EWS]

Committed r285475 (244000@main): <https://commits.webkit.org/244000@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 443592 [details].

[Per Arne Vollan]

Reopening to attach new patch.

[Per Arne Vollan]

Created attachment 443749 [details]
Patch

[Per Arne Vollan]

Sorry, uploaded patch to incorrect bug.