[Webkit-unassigned] [Bug 230906] Adopt platform UI for WebAuthn

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=230906

Brent Fulgham <bfulgham at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bfulgham at webkit.org
 Attachment #439610|review?                     |review-
              Flags|                            |

--- 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/WebAuthenticatorCoordinatorProxy.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/WebAuthenticatorCoordinatorProxy.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/WebAuthenticatorCoordinatorProxy.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/WebAuthenticatorCoordinatorProxy.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/WebAuthenticatorCoordinatorProxy.mm:153
> +        credentialCreationOptions.excludedCredentials = WTFMove(excludedCredentials);

Ditto the above.

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

I recommend adoptNS here to avoid future mistakes.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.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/WebAuthenticatorCoordinatorProxy.mm:196
> +        requestContext.securityKeyCredentialAssertionOptions = [allocASCPublicKeyCredentialAssertionOptionsInstance() initWithKind:ASCPublicKeyCredentialKindSecurityKey relyingPartyIdentifier:options.rpId challenge:challenge userVerificationPreference:userVerification allowedCredentials:allowedCredentials];

Ditto.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211004/2c56f338/attachment.htm>