Bug 237380 – [WebAuthn] Completion handler is not called when WebAuthn invoked without proper entitlements

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

[Brent Fulgham]

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.

[Radar WebKit Bug Importer]

<rdar://problem/89700242>

[Brent Fulgham]

Created attachment 453640 [details]
Patch

[Brent Fulgham]

Created attachment 453643 [details]
Patch

[Brent Fulgham]

Created attachment 453646 [details]
Patch

[[e-mail]]

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..." ?

[[e-mail]]

(In reply to [e-mail] 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.

[Chris Dumez]

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() ?

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 453655 [details]
Patch for landing

[EWS]

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].

[[e-mail]]

*** Bug 240666 has been marked as a duplicate of this bug. ***