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

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham 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 439610: Patch

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




--- Comment #4 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 439610
  --> https://bugs.webkit.org/attachment.cgi?id=439610
Patch

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

I think this patch looks good, but I think it has some memory management
issues. I'm asking David to do a review -- he can give us the right steps to
take.

>
Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProx
y.mm:87
> +		   transportString = @"usb";

Nit: Seems like these string conversion things should live where
AuthenticatorTransport is defined.

Not needed for this patch, though.

>
Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProx
y.mm:127
> +    ASCCredentialRequestContext *requestContext =
[allocASCCredentialRequestContextInstance() initWithRequestTypes:requestTypes];

You should adoptNS here to avoid the possibility of leaking this (e.g., if we
add new code someday that does an early return or something).

>
Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProx
y.mm:130
> +    ASCPublicKeyCredentialCreationOptions *credentialCreationOptions =
[allocASCPublicKeyCredentialCreationOptionsInstance() init];

This should be wrapped in 'adoptNS', since the refcount is increased when you
assign to the requestContext member, and we will leak if this isn't cleaned up.

>
Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProx
y.mm:144
> +    credentialCreationOptions.supportedAlgorithmIdentifiers =
WTFMove(supportedAlgorithmIdentifiers);

I don't think this will do what you want. You are just moving a pointer, and
the refcount will now be +1  (assuming supportedAlgorithmIdentifiers is
refcounted).

I think you need to adoptNS the NSMutableArray, so it gets cleaned up.

>
Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProx
y.mm:153
> +	   credentialCreationOptions.excludedCredentials =
WTFMove(excludedCredentials);

Ditto the above.

>
Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProx
y.mm:187
> +    ASCCredentialRequestContext *requestContext =
[allocASCCredentialRequestContextInstance() initWithRequestTypes:requestTypes];

I recommend adoptNS here to avoid future mistakes.

>
Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProx
y.mm:193
> +	   requestContext.platformKeyCredentialAssertionOptions =
[allocASCPublicKeyCredentialAssertionOptionsInstance()
initWithKind:ASCPublicKeyCredentialKindPlatform
relyingPartyIdentifier:options.rpId challenge:challenge
userVerificationPreference:userVerification
allowedCredentials:allowedCredentials];

I think this will leak.

>
Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProx
y.mm:196
> +	   requestContext.securityKeyCredentialAssertionOptions =
[allocASCPublicKeyCredentialAssertionOptionsInstance()
initWithKind:ASCPublicKeyCredentialKindSecurityKey
relyingPartyIdentifier:options.rpId challenge:challenge
userVerificationPreference:userVerification
allowedCredentials:allowedCredentials];

Ditto.