Bug 234180 - [WebAuthn] Allow same-site, cross-origin iframe get()
Summary: [WebAuthn] Allow same-site, cross-origin iframe get()
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: j_pascoe@apple.com
Keywords: InRadar
Depends on: 234283 234309
Blocks: 222240
  Show dependency treegraph
Reported: 2021-12-10 15:02 PST by j_pascoe@apple.com
Modified: 2022-03-02 16:30 PST (History)
9 users (show)

See Also:

Patch (26.95 KB, patch)
2021-12-10 15:18 PST, j_pascoe@apple.com
no flags Details | Formatted Diff | Diff
Patch (34.64 KB, patch)
2021-12-13 13:38 PST, j_pascoe@apple.com
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description j_pascoe@apple.com 2021-12-10 15:02:51 PST
WebAuthn Level 2 specifies a feature policy: https://w3c.github.io/webauthn/#sctn-iframe-guidance, functionality to get credentials from a cross-origin iframe should be enabled if the iframe has the allow="publickey-credentials-get" attribute/value pair.

This patch implements this functionality only for same-site, cross-origin i-frames.
Comment 1 j_pascoe@apple.com 2021-12-10 15:03:16 PST
Comment 2 j_pascoe@apple.com 2021-12-10 15:18:38 PST
Created attachment 446826 [details]
Comment 3 Brent Fulgham 2021-12-13 09:43:03 PST
Comment on attachment 446826 [details]

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(destinationURL)) || 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.
Comment 4 j_pascoe@apple.com 2021-12-13 13:38:52 PST
Created attachment 447053 [details]
Comment 5 Brent Fulgham 2021-12-13 14:22:16 PST
Comment on attachment 447053 [details]

Looks great -- thank you for making those changes! r=me
Comment 6 EWS 2021-12-13 16:00:08 PST
Committed r286993 (?): <https://commits.webkit.org/r286993>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447053 [details].
Comment 7 WebKit Commit Bot 2021-12-13 18:40:50 PST
Re-opened since this is blocked by bug 234283