Bug 191523 - [WebAuthn] Implement AuthenticatorCancel
Summary: [WebAuthn] Implement AuthenticatorCancel
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on: 202559
Blocks: 181943
  Show dependency treegraph
 
Reported: 2018-11-11 18:00 PST by Jiewen Tan
Modified: 2019-10-23 12:25 PDT (History)
9 users (show)

See Also:


Attachments
Patch (104.92 KB, patch)
2019-10-16 23:02 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (125.23 KB, patch)
2019-10-17 15:44 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (126.73 KB, patch)
2019-10-17 15:59 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (130.47 KB, patch)
2019-10-18 11:46 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2018-11-11 18:00:42 PST
We need to implement AuthenticatorCancel for local authenticators and ways within WebKit/WebCore to invoke AuthenticatorCancel. Things like refreshing a page could go through this route.
Comment 1 Radar WebKit Bug Importer 2019-10-02 12:39:40 PDT
<rdar://problem/55920204>
Comment 2 Jiewen Tan 2019-10-16 23:02:48 PDT
Created attachment 381169 [details]
Patch
Comment 3 Chris Dumez 2019-10-17 08:56:11 PDT
Comment on attachment 381169 [details]
Patch

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

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:67
> +    if (!m_document || !m_document->frame() || !m_document->page()) {

Not needed. If you did not have a frame, you wouldn't have a page.

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:104
> +    if (!m_document || !m_document->frame() || !m_document->page()) {

Not needed. If you did not have a frame, you wouldn't have a page.

> Source/WebCore/Modules/webauthn/AuthenticatorCoordinator.cpp:143
> +    const auto& callerOrigin = document.securityOrigin();

we normally omit the "const" when we're using auto.

> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.cpp:102
> +    memset(serialized.data() + serialized.size(), 0, kHidPacketSize - serialized.size());

Looks to me like you're writing in memory that you don't own here? I get that the vector capacity is kHidPacketSize, but it is not its size.
You should swap your too lines, first resize() and then memset.

Also, grow() here would be more efficient than resize() since serialized.size() <= kHidPacketSize.

> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.cpp:151
> +    memset(serialized.data() + serialized.size(), 0, kHidPacketSize - serialized.size());

Same comment as above. ASAN would likely complain here.

> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.cpp:53
> +    if (!m_manager)

I think this can be:
if (m_manager)
    m_manager->cancelRequest(*this);

given how simple the method is.

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:167
> +void AuthenticatorManager::cancelRequest(const WebPageProxy& page, Optional<FrameIdentifier> subFrameId)

const Optional<FrameIdentifier>&

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:172
> +    if (subFrameId && (subFrameId != *(m_pendingRequestData.frameId)))

Too many parentheses.

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:174
> +    invokePendingCompletionHandler(ExceptionData { NotAllowedError, emptyString() });

Probably want a better error message than ""

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidConnection.h:68
> +    bool isInitialized() { return m_isInitialized; }

should be const.

> Source/WebKit/UIProcess/WebAuthentication/WebAuthenticationRequestData.h:49
> +    WTF::Optional<WebCore::FrameIdentifier> frameId;

FrameIdentifier are not unique at UIProcess level, or even at WebPageProxy level. So it looks very fragile to use this in UIProcess to identifier a request. A { WebCore::PageIdentifier + WebCore::FrameIdentifier } is globally unique though.

> Source/WebKit/UIProcess/WebAuthentication/fido/FidoAuthenticator.cpp:42
> +    if (!m_driver)

if (m_driver)
    m_driver->cancel();

> Source/WebKit/UIProcess/WebPageProxy.cpp:4119
> +    m_websiteDataStore->authenticatorManager().cancelRequest(*this, frame->isMainFrame() ? WTF::nullopt : Optional<FrameIdentifier>(frameID));

could use makeOptional(frameID)

Seems odd to not be using a frameID for the main frame, since it has one.

> Source/WebKit/UIProcess/WebProcessProxy.cpp:896
> +        // WebPageProxy takes care of the main frame.

Why? Why can't we have a simple code path no matter if the frame is a main frame or not?

> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:58
> +    auto messageId = setRequestCallback(WTFMove(handler));

Should this be using sendWithAsyncReply() ?

> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:68
> +    auto messageId = setRequestCallback(WTFMove(handler));

Should this be using sendWithAsyncReply() ?
Comment 4 Brent Fulgham 2019-10-17 08:59:41 PDT
Comment on attachment 381169 [details]
Patch

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

I think your handling of the 'getSerializedData' calls is wrong.

> Source/WebKit/ChangeLog:28
> +        Noted, CtapHidDriver is the only CtapDriver implements the cancel method. Since the message

Note: The CtapHidDriver is the only CtapDriver that implements the cancel method."

>> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:67
>> +    if (!m_document || !m_document->frame() || !m_document->page()) {
> 
> Not needed. If you did not have a frame, you wouldn't have a page.

We use this same test in a few places. I wonder if it would be better to have (perhaps come up with a better name for this predicate):

static bool documentHasValidState(Document* document) { return document &&  document->frame() && document->page(); }

Then just say:

if (!documentHasValidState(m_document)) { ...

>> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:104
>> +    if (!m_document || !m_document->frame() || !m_document->page()) {
> 
> Not needed. If you did not have a frame, you wouldn't have a page.

if (!documentHasValidState(m_document)) { ...

> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.cpp:103
> +    serialized.resize(kHidPacketSize);

You said that you switched from grow here, because it doesn't initialized POD types. But resize also shrinks the Vector to match the passed input, if it is smaller than the Vector's current size. Is that what you want?

Previously, this logic would make the serialized Vector larger by 'kHidPacketSize' bytes. Now, we change the size of Vector to be 'kHidPacketSize'.

I think you should still keep the 'grow', and after the 'grow' is executed, perform the memset to initialize the range of data.

> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.cpp:152
> +    serialized.resize(kHidPacketSize);

Ditto.

> Tools/TestWebKitAPI/Tests/WebCore/FidoHidMessageTest.cpp:63
> + * Byte 7-n:  Data block

Was the original comment wrong? You don't seem to have changed this test case at all, but the comment is now very different in terms of which bytes mean what.
Comment 5 Chris Dumez 2019-10-17 09:28:28 PDT
Comment on attachment 381169 [details]
Patch

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

>>> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:67
>>> +    if (!m_document || !m_document->frame() || !m_document->page()) {
>> 
>> Not needed. If you did not have a frame, you wouldn't have a page.
> 
> We use this same test in a few places. I wonder if it would be better to have (perhaps come up with a better name for this predicate):
> 
> static bool documentHasValidState(Document* document) { return document &&  document->frame() && document->page(); }
> 
> Then just say:
> 
> if (!documentHasValidState(m_document)) { ...

Really not a fan of this proposal. I'd rather we keep the bogus check.
Comment 6 Jiewen Tan 2019-10-17 15:43:21 PDT
Comment on attachment 381169 [details]
Patch

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

Thanks Chris and Brent for taking a look.

>> Source/WebKit/ChangeLog:28
>> +        Noted, CtapHidDriver is the only CtapDriver implements the cancel method. Since the message
> 
> Note: The CtapHidDriver is the only CtapDriver that implements the cancel method."

Fixed.

>>>> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:67
>>>> +    if (!m_document || !m_document->frame() || !m_document->page()) {
>>> 
>>> Not needed. If you did not have a frame, you wouldn't have a page.
>> 
>> We use this same test in a few places. I wonder if it would be better to have (perhaps come up with a better name for this predicate):
>> 
>> static bool documentHasValidState(Document* document) { return document &&  document->frame() && document->page(); }
>> 
>> Then just say:
>> 
>> if (!documentHasValidState(m_document)) { ...
> 
> Really not a fan of this proposal. I'd rather we keep the bogus check.

Okay, dropped the new check.

>>> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:104
>>> +    if (!m_document || !m_document->frame() || !m_document->page()) {
>> 
>> Not needed. If you did not have a frame, you wouldn't have a page.
> 
> if (!documentHasValidState(m_document)) { ...

Okay, dropped the new check.

>> Source/WebCore/Modules/webauthn/AuthenticatorCoordinator.cpp:143
>> +    const auto& callerOrigin = document.securityOrigin();
> 
> we normally omit the "const" when we're using auto.

Omitted.

>> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.cpp:102
>> +    memset(serialized.data() + serialized.size(), 0, kHidPacketSize - serialized.size());
> 
> Looks to me like you're writing in memory that you don't own here? I get that the vector capacity is kHidPacketSize, but it is not its size.
> You should swap your too lines, first resize() and then memset.
> 
> Also, grow() here would be more efficient than resize() since serialized.size() <= kHidPacketSize.

Fixed.

>> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.cpp:103
>> +    serialized.resize(kHidPacketSize);
> 
> You said that you switched from grow here, because it doesn't initialized POD types. But resize also shrinks the Vector to match the passed input, if it is smaller than the Vector's current size. Is that what you want?
> 
> Previously, this logic would make the serialized Vector larger by 'kHidPacketSize' bytes. Now, we change the size of Vector to be 'kHidPacketSize'.
> 
> I think you should still keep the 'grow', and after the 'grow' is executed, perform the memset to initialize the range of data.

Fixed as follow:
auto offset = serialized.size();
serialized.grow(kHidPacketSize);
memset(serialized.data() + offset, 0, kHidPacketSize - offset);

>> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.cpp:151
>> +    memset(serialized.data() + serialized.size(), 0, kHidPacketSize - serialized.size());
> 
> Same comment as above. ASAN would likely complain here.

Fixed.

>> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.cpp:152
>> +    serialized.resize(kHidPacketSize);
> 
> Ditto.

Fixed.

>> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.cpp:53
>> +    if (!m_manager)
> 
> I think this can be:
> if (m_manager)
>     m_manager->cancelRequest(*this);
> 
> given how simple the method is.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:167
>> +void AuthenticatorManager::cancelRequest(const WebPageProxy& page, Optional<FrameIdentifier> subFrameId)
> 
> const Optional<FrameIdentifier>&

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:172
>> +    if (subFrameId && (subFrameId != *(m_pendingRequestData.frameId)))
> 
> Too many parentheses.

Removed one.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:174
>> +    invokePendingCompletionHandler(ExceptionData { NotAllowedError, emptyString() });
> 
> Probably want a better error message than ""

It doesn't matter. Sites will never get this back. Anyway. changed to "Operation timed out.".

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidConnection.h:68
>> +    bool isInitialized() { return m_isInitialized; }
> 
> should be const.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/WebAuthenticationRequestData.h:49
>> +    WTF::Optional<WebCore::FrameIdentifier> frameId;
> 
> FrameIdentifier are not unique at UIProcess level, or even at WebPageProxy level. So it looks very fragile to use this in UIProcess to identifier a request. A { WebCore::PageIdentifier + WebCore::FrameIdentifier } is globally unique though.

I don't think GlobalFrameIdentifier is really helpful here as the request can only live within the lifetime of its corresponding web process, but I don't mind adding another safe guard.

>> Source/WebKit/UIProcess/WebAuthentication/fido/FidoAuthenticator.cpp:42
>> +    if (!m_driver)
> 
> if (m_driver)
>     m_driver->cancel();

Fixed.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:4119
>> +    m_websiteDataStore->authenticatorManager().cancelRequest(*this, frame->isMainFrame() ? WTF::nullopt : Optional<FrameIdentifier>(frameID));
> 
> could use makeOptional(frameID)
> 
> Seems odd to not be using a frameID for the main frame, since it has one.

Changed the method signature to: AuthenticatorManager::cancelRequest(const WebCore::PageIdentifier&, const Optional<WebCore::FrameIdentifier>&) to use GlobalFrameIdentifier.

>> Source/WebKit/UIProcess/WebProcessProxy.cpp:896
>> +        // WebPageProxy takes care of the main frame.
> 
> Why? Why can't we have a simple code path no matter if the frame is a main frame or not?

Just thought it will be redundant to do it twice as WebPageProxy::resetState is also calling the cancelRequest. It doesn't hurt to call it twice. So, let's simplify the code. Not sure if WebPageProxy::resetState can be omitted.

>> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:58
>> +    auto messageId = setRequestCallback(WTFMove(handler));
> 
> Should this be using sendWithAsyncReply() ?

Right, a good idea!

>> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:68
>> +    auto messageId = setRequestCallback(WTFMove(handler));
> 
> Should this be using sendWithAsyncReply() ?

Fixed.

>> Tools/TestWebKitAPI/Tests/WebCore/FidoHidMessageTest.cpp:63
>> + * Byte 7-n:  Data block
> 
> Was the original comment wrong? You don't seem to have changed this test case at all, but the comment is now very different in terms of which bytes mean what.

I guess this particular test predated WebAuthn/FIDO2 and Google folks forgot to update the comment.
Comment 7 Jiewen Tan 2019-10-17 15:44:22 PDT
Created attachment 381240 [details]
Patch
Comment 8 Jiewen Tan 2019-10-17 15:59:00 PDT
Created attachment 381244 [details]
Patch
Comment 9 Jiewen Tan 2019-10-18 11:46:01 PDT
Created attachment 381319 [details]
Patch
Comment 10 Brent Fulgham 2019-10-18 12:54:19 PDT
Comment on attachment 381319 [details]
Patch

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

Looks good! r=me

> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.cpp:104
> +    memset(serialized.data() + offset, 0, kHidPacketSize - offset);

Yes, this looks much better.

> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.cpp:154
> +    memset(serialized.data() + offset, 0, kHidPacketSize - offset);

Ditto.
Comment 11 Jiewen Tan 2019-10-18 12:59:53 PDT
Comment on attachment 381319 [details]
Patch

Thanks, Brent.
Comment 12 WebKit Commit Bot 2019-10-18 13:56:05 PDT
Comment on attachment 381319 [details]
Patch

Clearing flags on attachment: 381319

Committed r251295: <https://trac.webkit.org/changeset/251295>
Comment 13 WebKit Commit Bot 2019-10-18 13:56:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Truitt Savell 2019-10-23 08:19:24 PDT
It looks like the changes in https://trac.webkit.org/changeset/251295/webkit

broke http/wpt/webauthn/public-key-credential-get-success-hid.https.html
and caused it to be severely flakey. Diff makes it look like it is now flaking to a pass in the sub test. 

on Mac and iOS Debug wk2

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-get-success-hid.https.html

Diff:
--- /Volumes/Data/slave/mojave-debug-tests-wk2/build/layout-test-results/http/wpt/webauthn/public-key-credential-get-success-hid.https-expected.txt
+++ /Volumes/Data/slave/mojave-debug-tests-wk2/build/layout-test-results/http/wpt/webauthn/public-key-credential-get-success-hid.https-actual.txt
@@ -4,5 +4,5 @@
 PASS PublicKeyCredential's [[get]] with userVerification { preferred } in a mock hid authenticator. 
 PASS PublicKeyCredential's [[get]] with userVerification { discouraged } in a mock hid authenticator. 
 PASS PublicKeyCredential's [[get]] with mixed options in a mock hid authenticator. 
-PASS PublicKeyCredential's [[get]] with two consecutive requests. 
+FAIL PublicKeyCredential's [[get]] with two consecutive requests. assert_unreached: Should have rejected: This request has been cancelled by a new request. Reached unreachable code
Comment 15 Truitt Savell 2019-10-23 08:23:34 PDT
> and caused it to be severely flakey. Diff makes it look like it is now
> flaking to a pass in the sub test. 

Meant to type "Diff makes it look like it is now flaking to a fail in the sub test."
Comment 16 Russell Epstein 2019-10-23 11:53:27 PDT
(In reply to Truitt Savell from comment #14)
> It looks like the changes in https://trac.webkit.org/changeset/251295/webkit
> 
> broke http/wpt/webauthn/public-key-credential-get-success-hid.https.html
> and caused it to be severely flakey. Diff makes it look like it is now
> flaking to a pass in the sub test. 
> 
> on Mac and iOS Debug wk2
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-
> get-success-hid.https.html
> 
> Diff:
> ---
> /Volumes/Data/slave/mojave-debug-tests-wk2/build/layout-test-results/http/
> wpt/webauthn/public-key-credential-get-success-hid.https-expected.txt
> +++
> /Volumes/Data/slave/mojave-debug-tests-wk2/build/layout-test-results/http/
> wpt/webauthn/public-key-credential-get-success-hid.https-actual.txt
> @@ -4,5 +4,5 @@
>  PASS PublicKeyCredential's [[get]] with userVerification { preferred } in a
> mock hid authenticator. 
>  PASS PublicKeyCredential's [[get]] with userVerification { discouraged } in
> a mock hid authenticator. 
>  PASS PublicKeyCredential's [[get]] with mixed options in a mock hid
> authenticator. 
> -PASS PublicKeyCredential's [[get]] with two consecutive requests. 
> +FAIL PublicKeyCredential's [[get]] with two consecutive requests.
> assert_unreached: Should have rejected: This request has been cancelled by a
> new request. Reached unreachable code

Re-opening due to failing regression tests.
Comment 17 Jiewen Tan 2019-10-23 12:25:37 PDT
Bug 202563 will take care of the test failure.