[webkit-reviews] review denied: [Bug 230906] Adopt platform UI for WebAuthn : [Attachment 440851] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has denied Garrett Davidson
<garrett_davidson at apple.com>'s request for review:
Bug 230906: Adopt platform UI for WebAuthn
https://bugs.webkit.org/show_bug.cgi?id=230906

Attachment 440851: Patch

https://bugs.webkit.org/attachment.cgi?id=440851&action=review




--- Comment #10 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 440851
  --> https://bugs.webkit.org/attachment.cgi?id=440851
Patch

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

r-, but fixing the requested changes should lead to an r+.

>
Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProx
y.mm:239
> +    NSData *challenge = toNSData(options.challenge).get();

This causes `challenge` to be assigned a deallocated NSData pointer because the
temporary RetainPtr<NSData> returned will be deallocated (and release the
NSData object with it).

You want to keep the return value as a RetainPtr<NSData>.  Here's one way:

     auto challenge = toNSData(options.challenge);

The code below will need to be changed to `challenge.get()`, but the compiler
will tell you if you missed any.

This must be fixed or we will introduce a use-after-free bug.

>
Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProx
y.mm:267
> +    NSWindow *window = m_webPageProxy.platformWindow();
> +    [proxy performAuthorizationRequestsForContext:requestContext.get()
withClearanceHandler:makeBlockPtr([weakThis = makeWeakPtr(*this), handler =
WTFMove(handler), window = WTFMove(window)](NSXPCListenerEndpoint
*daemonEnpoint, NSError *error) mutable {

The `window = WTFMove(window)` here is wrong as you're trying to move an
NSWindow * Objective-C object.

I think it would be better to wrap the NSWindow * object in a RetainPtr and
WTFMove that:

    RetainPtr<NSWindow> window = m_webPageProxy.platformWindow();

Note that we don't want to use adoptNS() here because we're not getting a +1
retained object back (under MRR).

This could also be written as this, but it makes the type more opaque:

    auto window = retainPtr(m_webPageProxy.platformWindow());

This must be fixed.

>
Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProx
y.mm:287
> +		   ASCPlatformPublicKeyCredentialRegistration
*registrationCredential = credential;

There is a new checked_objc_cast<> function in the new
<wtf/cocoa/TypeCastsCocoa.h> header since you started writing this patch.  It
would probably be a good idea to do that here to make sure `credentials` is the
type of object you expect it to be:

		auto *registrationCredential =
checked_objc_cast<ASCPlatformPublicKeyCredentialRegistration>(credential);

Note that if you nil-check the result of the cast, you can use
`dynamic_objc_cast<>` instead:

		auto *registrationCredential =
dynamic_objc_cast<ASCPlatformPublicKeyCredentialRegistration>(credential);
		if (!registrationCredential) {
		    /* return early with an error, possibly calling the
handler() function */
		}

Since this code deals with authentication, I would strongly suggest using one
or the other of these cast methods here and the other places where `credential`
is assigned a specific type below.

>
Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProx
y.mm:294
> +		   ASCSecurityKeyPublicKeyCredentialRegistration
*registrationCredential = credential;

Ditto.

>
Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProx
y.mm:301
> +		   ASCPlatformPublicKeyCredentialAssertion *assertionCredential
= credential;

Ditto.

>
Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProx
y.mm:310
> +		   ASCPlatformPublicKeyCredentialAssertion *assertionCredential
= credential;

Ditto.