[webkit-reviews] review denied: [Bug 234180] [WebAuthn] Allow same-site, cross-origin iframe get() : [Attachment 446826] 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 234180: [WebAuthn] Allow same-site, cross-origin iframe get()
https://bugs.webkit.org/show_bug.cgi?id=234180

Attachment 446826: Patch

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




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

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

I think this patch needs a bit of work before it's ready to land. Please adjust
the same-site test, and switch from a boolean to a two-state enum that
documents its purpose.

> Source/WebCore/Modules/webauthn/AuthenticatorCoordinator.cpp:184
> +	   if (domain !=
RegistrableDomain(parentDocument->securityOrigin().data()))

You should use "areRegistrableDomainsEqual" from the RegistrableDomains header.

We identify same-site in Document like this:

    // Only prevent cross-site navigations.
    RefPtr targetDocument = targetFrame.document();
    if (targetDocument &&
(targetDocument->securityOrigin().isSameOriginDomain(SecurityOrigin::create(des
tinationURL)) || areRegistrableDomainsEqual(targetDocument->url(),
destinationURL)))
	return false;

You should probably use something like that to be consistent. It should also
handle all the weird cases of data/Blob URLs and so forth properly.

> Source/WebCore/Modules/webauthn/WebAuthenticationUtils.h:55
> +WEBCORE_EXPORT Ref<ArrayBuffer> buildClientDataJson(ClientDataType /*type*/,
const BufferSource& challenge, const SecurityOrigin& /*origin*/, const bool
crossOrigin);

Instead of this boolean, we should create a two-state enum to represent this
state. Something like WebAuthn::CrossOrigin::Yes/No, or
WebAuthn::Scope::CrossOrigin/SameOrigin. There are many examples in the sources
of this pattern.