Bug 237380

Summary: [WebAuthn] Completion handler is not called when WebAuthn invoked without proper entitlements
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit Misc.Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, kevin.flanagan, pascoe, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch for landing none

Description Brent Fulgham 2022-03-02 11:08:36 PST 
WebAuthn is not permitted outside of Web Browser applications. When an application that lacks the full web browser entitlement attempts to invoke WebAuthn flows, we do an early return. However, the completion handler for this flow is bypassed, preventing applications from being informed of this problem.
Comment 1 Radar WebKit Bug Importer 2022-03-02 11:08:57 PST 
<rdar://problem/89700242>
Comment 2 Brent Fulgham 2022-03-02 11:12:02 PST 
Created attachment 453640 [details]
Patch
Comment 3 Brent Fulgham 2022-03-02 11:33:26 PST 
Created attachment 453643 [details]
Patch
Comment 4 Brent Fulgham 2022-03-02 12:06:08 PST 
Created attachment 453646 [details]
Patch
Comment 5 pascoe@apple.com 2022-03-02 12:20:08 PST 
Comment on attachment 453646 [details]
Patch

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

This looks good to me.

> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:53
> +#define WEBAUTHN_RELEASE_LOG(fmt, ...) RELEASE_LOG(WebAuthn, "%p - [webPageID=%" PRIu64 ", webFrameID=%" PRIu64 "] WebAuthenticatorCoordinator::" fmt, this, PAGE_ID, FRAME_ID, ##__VA_ARGS__)

Will fmt be concatenated to the format string here: "...WebAuthenticatorCoordinator::" fmt, this, PA..." ?
Comment 6 pascoe@apple.com 2022-03-02 12:22:02 PST 
(In reply to j_pascoe@apple.com from comment #5)
> Will fmt be concatenated to the format string here:
> "...WebAuthenticatorCoordinator::" fmt, this, PA..." ?

I've answered my own question here, it does.
Comment 7 Chris Dumez 2022-03-02 13:28:30 PST 
Comment on attachment 453646 [details]
Patch

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

r=me

> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:88
> +        WEBAUTHN_RELEASE_LOG("makeCredential: The 'navigator.credentials.create' API is only permitted in applications with the 'com.apple.developer.web-browser' entitlement.");

Is this an error? If so, could we add a WEBAUTHN_RELEASE_LOG_ERROR (which calls RELEASE_LOG_ERROR internally) and call that instead?

If this is something we should pay attention to, making them stand out in the logs could be useful. Same comment for other logging you added in this file.

> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:138
> +        RELEASE_LOG(WebAuthn, "%p - [webPageID=%" PRIu64 "] WebAuthenticatorCoordinator::isUserVerifyingPlatformAuthenticatorAvailable: WebAuthn is only permitted in applications with the 'com.apple.developer.web-browser' entitlement.", this, PAGE_ID);

Why is this one not using WEBAUTHN_RELEASE_LOG() ?
Comment 8 Brent Fulgham 2022-03-02 13:33:02 PST 
Comment on attachment 453646 [details]
Patch

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

>> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:53
>> +#define WEBAUTHN_RELEASE_LOG(fmt, ...) RELEASE_LOG(WebAuthn, "%p - [webPageID=%" PRIu64 ", webFrameID=%" PRIu64 "] WebAuthenticatorCoordinator::" fmt, this, PAGE_ID, FRAME_ID, ##__VA_ARGS__)
> 
> Will fmt be concatenated to the format string here: "...WebAuthenticatorCoordinator::" fmt, this, PA..." ?

Yes!

>> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:88
>> +        WEBAUTHN_RELEASE_LOG("makeCredential: The 'navigator.credentials.create' API is only permitted in applications with the 'com.apple.developer.web-browser' entitlement.");
> 
> Is this an error? If so, could we add a WEBAUTHN_RELEASE_LOG_ERROR (which calls RELEASE_LOG_ERROR internally) and call that instead?
> 
> If this is something we should pay attention to, making them stand out in the logs could be useful. Same comment for other logging you added in this file.

Sure -- all of these are ERROR-level, since they indicate misuse of the APIs and places where developer will be confused if we don't help surface things.

I'll make that change.

>> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:138
>> +        RELEASE_LOG(WebAuthn, "%p - [webPageID=%" PRIu64 "] WebAuthenticatorCoordinator::isUserVerifyingPlatformAuthenticatorAvailable: WebAuthn is only permitted in applications with the 'com.apple.developer.web-browser' entitlement.", this, PAGE_ID);
> 
> Why is this one not using WEBAUTHN_RELEASE_LOG() ?

Oh! We don't have a calling frame here, since this is an implementation detail used outside of normal web flows. I can't use the same macro because that one expects a frame pointer.

But, I will make it RELEASE_LOG_ERROR as well.

And frankly, making a macro for it might be a little cleaner and more consistent, so I'll do that.
Comment 9 Brent Fulgham 2022-03-02 13:36:05 PST 
Created attachment 453655 [details]
Patch for landing
Comment 10 EWS 2022-03-02 14:08:46 PST 
Committed r290755 (247999@main): <https://commits.webkit.org/247999@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453655 [details].
Comment 11 pascoe@apple.com 2022-05-19 10:57:08 PDT 
*** Bug 240666 has been marked as a duplicate of this bug. ***