Bug 218721 – Implement a proof-of-concept for First Party Sets

• Component: WebKit Misc.
• Status: NEW

[Kate Cheney]

First Party Sets are being talked about in the Privacy Community Group (see https://github.com/privacycg/first-party-sets). We should create a draft implementation of functionality that WebKit may use First Party Sets for. 

There is no specification for First Party Sets, and this is not an official implementation of First Party Sets.

[Radar WebKit Bug Importer]

<rdar://problem/71204559>

[Kate Cheney]

Created attachment 413621 [details]
Patch

[Kate Cheney]

Created attachment 413626 [details]
Patch

[Sam Weinig]

Comment on attachment 413626 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        Add a draft implementation of functionality that WebKit may use First
> +        Party Sets for. First Party Sets are being talked about in public
> +        standards groups, see https://github.com/privacycg/first-party-sets.

It doesn't seem like this implements the spec. There is no logic for .well-known/first-party-set nor the Sec-First-Party-Set header as far as I can tell. What subset of the spec is this intended to implement?

> Source/WebCore/Modules/firstpartysets/FirstPartySets.h:47
> +    static HashMap<RegistrableDomain, HashSet<RegistrableDomain>>& firstPartySetsTestOverride();
> +    static HashMap<RegistrableDomain, HashSet<RegistrableDomain>>& firstPartySetsWithQuirk();

The use of globals here seems wrong. Seems like this data should live on something like the NetworkStorageSession.

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:894
> +#if ENABLE(RESOURCE_LOAD_STATISTICS)

Seems like if this is going to be #ifdefed, it should have its own ENABLE macro.

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.h:212
> +typedef void (*WKWebsiteDataStoreSetFirstPartySetsTestingOverrideFunction)(void* functionContext);
> +WK_EXPORT void WKWebsiteDataStoreSetFirstPartySetsTestingOverride(WKWebsiteDataStoreRef dataStoreRef, const WKStringRef subDomain, const WKStringRef topFrameDomain, void* context, WKWebsiteDataStoreSetFirstPartySetsTestingOverrideFunction completionHandler);

I think rather than add this functionality for testing only, you should consider what might be useful as API/SPI here that could go here instead. For instance, it would probably be useful to give browsers implemented on top of WebKit the ability to use existing static lists of domains owned by the same entities.

[Kate Cheney]

(In reply to Sam Weinig from comment #4)
> Comment on attachment 413626 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=413626&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        Add a draft implementation of functionality that WebKit may use First
> > +        Party Sets for. First Party Sets are being talked about in public
> > +        standards groups, see https://github.com/privacycg/first-party-sets.
> 
> It doesn't seem like this implements the spec. There is no logic for
> .well-known/first-party-set nor the Sec-First-Party-Set header as far as I
> can tell. What subset of the spec is this intended to implement?
> 

There is no spec for First Party Sets. The link I dropped in the description is an explainer that provides support for the concept of related domains having same-site privileges in privacy mechanisms. And, this is not an official implementation of First Party Sets, it is functionality we think FPS could be used for.

> > Source/WebCore/Modules/firstpartysets/FirstPartySets.h:47
> > +    static HashMap<RegistrableDomain, HashSet<RegistrableDomain>>& firstPartySetsTestOverride();
> > +    static HashMap<RegistrableDomain, HashSet<RegistrableDomain>>& firstPartySetsWithQuirk();
> 
> The use of globals here seems wrong. Seems like this data should live on
> something like the NetworkStorageSession.
> 

Ok, I can move the logic there.

> > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:894
> > +#if ENABLE(RESOURCE_LOAD_STATISTICS)
> 
> Seems like if this is going to be #ifdefed, it should have its own ENABLE
> macro.
> 

Ok, you're right.

> > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.h:212
> > +typedef void (*WKWebsiteDataStoreSetFirstPartySetsTestingOverrideFunction)(void* functionContext);
> > +WK_EXPORT void WKWebsiteDataStoreSetFirstPartySetsTestingOverride(WKWebsiteDataStoreRef dataStoreRef, const WKStringRef subDomain, const WKStringRef topFrameDomain, void* context, WKWebsiteDataStoreSetFirstPartySetsTestingOverrideFunction completionHandler);
> 
> I think rather than add this functionality for testing only, you should
> consider what might be useful as API/SPI here that could go here instead.
> For instance, it would probably be useful to give browsers implemented on
> top of WebKit the ability to use existing static lists of domains owned by
> the same entities.

That could be useful, but for the sake of the API review process I think if we do implement it, it should be in its own patch. We also don't know of any use cases for now where that would be useful. On the other hand, an SPI for setting FPS for layout tests will definitely be useful in the future (like for https://bugs.webkit.org/show_bug.cgi?id=218471).

[Kate Cheney]

Created attachment 413637 [details]
Patch

[Brent Fulgham]

Comment on attachment 413637 [details]
Patch

Looks good. I wonder if we could figure out a way to align App-Bound domains and FPS (idea for future work).

[Kate Cheney]

Created attachment 413641 [details]
Patch

[Kate Cheney]

Thanks! I noticed and removed a stray function declaration before cq+.

[Sam Weinig]

Comment on attachment 413641 [details]
Patch

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

> Source/WebCore/platform/network/NetworkStorageSession.cpp:378
> +#if ENABLE(FIRST_PARTY_SETS)
> +HashMap<RegistrableDomain, HashSet<RegistrableDomain>>& NetworkStorageSession::firstPartySets()
> +{
> +    static NeverDestroyed<HashMap<RegistrableDomain, HashSet<RegistrableDomain>>> map;
> +    return map.get();
> +}
> +
> +bool NetworkStorageSession::canRequestStorageAccessForLoginPurposesWithoutPriorUserInteraction(const RegistrableDomain& subFrameDomain, const RegistrableDomain& topFrameDomain)
> +{
> +    auto loginDomains = loginDomainsForFirstParty(topFrameDomain);
> +    if (loginDomains)
> +        return loginDomains.value().contains(subFrameDomain);
> +    return false;
> +}
> +
> +Optional<HashSet<RegistrableDomain>> NetworkStorageSession::loginDomainsForFirstParty(const RegistrableDomain& firstPartyDomain)
> +{
> +    auto it = firstPartySets().find(firstPartyDomain);
> +    if (it != firstPartySets().end())
> +        return it->value;
> +    return WTF::nullopt;
> +}
> +#endif

I think you misunderstood. I was objecting to the use of globals more than the specific file the code was in. This is still global state where it really should not be.