[webkit-reviews] review denied: [Bug 222240] [WebAuthn] Using WebAuthn within cross-origin iframe elements : [Attachment 453772] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied j_pascoe at apple.com
<j_pascoe at apple.com>'s request for review:
Bug 222240: [WebAuthn] Using WebAuthn within cross-origin iframe elements
https://bugs.webkit.org/show_bug.cgi?id=222240

Attachment 453772: Patch

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




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

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

I think this is close, but needs a little work before it's ready to land. r- to
adjust the naming and clean up some CredentialContainer:scope implementation.

> Source/WebCore/ChangeLog:11
> +	   same-site to cross-origin with consent.

I had to read this sentence a few times. I propose:

"""
This patch relaxes the requirement to perform a Web Authentication assertion
inside an i-frame with the "publickey-credentials-get" feature policy from
'same-site' to 'cross-origin with consent'.
"""

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:49
> +std::pair<WebAuthn::Scope, std::optional<SecurityOriginData>>
CredentialsContainer::scopeAndSingleParent()

Maybe "singleParent" should be called something like "AuthenticationOrigin", or
"RelyingParty"? I think we are authenticating in a frame (which is the relying
party) on behalf of the main frame (which is the site seeking to do something
based on the authentication succeeding in the frame).

Maybe we should call it  "scopeAndCrossOriginParent"?

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:64
> +	       parents.add(origin.data());

Collecting the set of parents really only makes sense if we have a plan to
select one over the others due to some criteria. Since we seem only care about
the first one we encounter, we should just store one:

if (!origin.isSameOriginAs(document->securityOrigin())) {
    isSameOrigin = false;
    if (!singleParent)
	singleParent = origin.data();
}

Or maybe:

if (isSameOrigin && !origin.isSameOriginAs(document->securityOrigin())) {
    isSameOrigin = false;
    singleParent = origin.data();
}

Or maybe:

if (!singleParent && !origin.isSameOriginAs(document->securityOrigin()))
    singleParent = origin.data()

... and get rid of the 'isSameOrigin' flag entirely.

And maybe "singleParent" should be called "crossOriginParent"

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.h:62
> +    std::pair<WebAuthn::Scope, std::optional<SecurityOriginData>>
scopeAndSingleParent();

It might be worth declaring a typedef for this std::pair-based type, since you
use it in a number of places.

> Source/WebCore/Modules/webauthn/AuthenticatorCoordinator.cpp:205
> +	   promise.reject(Exception { NotAllowedError, "The origin of the
document is not the same as multiple of its ancestors."_s });

What does "... not the same as multiple of its ancestors" mean here? I think
this would be clearer to say:

>
Source/WebKit/UIProcess/WebAuthentication/WebAuthenticatorCoordinatorProxy.cpp:
87
>		   }

I looks like the indentation of everything inside HAVE(UNIFIED_ASC_AUTH_UI)
needs to be fixed (I realize this predates your change, but we should fix it).