Bug 213889 – Custom URL schemes should be treated as app-bound

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

We should treat loads using custom URL schemes as app-bound

[Kate Cheney]

<rdar://problem/64804671>

[Kate Cheney]

Created attachment 403378 [details]
Patch

[John Wilander]

If possible, it would be nice to test that this'll work with the ITP relaxation for app-bound domains too.

[Kate Cheney]

Comment on attachment 403378 [details]
Patch

Cancelling review while investigating failing API tests.

[Kate Cheney]

Created attachment 403395 [details]
Patch

[Kate Cheney]

(In reply to John Wilander from comment #3)
> If possible, it would be nice to test that this'll work with the ITP
> relaxation for app-bound domains too.

Do you have a specific example in mind where you think it might not work with ITP relaxation?

[Brent Fulgham]

Comment on attachment 403395 [details]
Patch

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

> Source/WebKit/ChangeLog:14
> +        count of 10 app-bound domains.

I assume they can be interleaved with the domains, right? No restriction on ordering?

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:442
> +                if (length > 0 && [data characterAtIndex:length - 1] == ':') {

You might be able to just do:

if ([data endsWithString: @":"]) {
    auto appBoundScheme = String([data subStringToIndex:[data length] - 1]);
    if (!appBoundScheme.isEmpty())
        appBoundSchemes().add(appBoundScheme);
}

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:445
> +                        appBoundSchemes().add(appBoundScheme);

Do we want to continue and possibly add this to the set of appBoundDomains, too? Or should we 'continue' here?

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:502
> +        auto isAppBound = schemeIsAppBound || domains.contains(WebCore::RegistrableDomain(requestURL));

This might be a little more legible as a static function:

static NavigatingToAppBoundDomain schemeOrDomainIsAppBound(const URL& requestURL, const <<mumble>>& domains, const <<mumble>>& schemes) {
    auto protocol = requestURL.protocol().toString();
    auto schemeIsAppBound = !protocol.isNull() && schemes.contains(protocol);
    auto domainIsAppBound = domains.contains(WebCore::RegistrableDomain(requestURL));
    return schemeIsAppBound || domainIsAppBound ? NavigatingToAppBoundDomain::Yes : NavigatingToAppBoundDomain::No;
}

> Tools/TestWebKitAPI/Info.plist:8
> +		<string>:</string>

Ha! Good test!

> Tools/TestWebKitAPI/Info.plist:28
> +		<string>should-not-be-included:</string>

Should we have an empty <string></string> test?

[Brent Fulgham]

Comment on attachment 403395 [details]
Patch

I think this patch is good, but I have a couple of suggestions I hope you'll consider.

[Kate Cheney]

(In reply to Brent Fulgham from comment #7)
> Comment on attachment 403395 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403395&action=review
> 
> > Source/WebKit/ChangeLog:14
> > +        count of 10 app-bound domains.
> 
> I assume they can be interleaved with the domains, right? No restriction on
> ordering?
> 

Yes. No restrictions, its first-come, first-serve until all 10 spots are filled.

> > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:442
> > +                if (length > 0 && [data characterAtIndex:length - 1] == ':') {
> 
> You might be able to just do:
> 
> if ([data endsWithString: @":"]) {
>     auto appBoundScheme = String([data subStringToIndex:[data length] - 1]);
>     if (!appBoundScheme.isEmpty())
>         appBoundSchemes().add(appBoundScheme);
> }

Good idea, using the endsWithString makes it more readable, I'll change this.

> 
> > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:445
> > +                        appBoundSchemes().add(appBoundScheme);
> 
> Do we want to continue and possibly add this to the set of appBoundDomains,
> too? Or should we 'continue' here?
> 

Very good catch, we do not want to add this to the set of domains if a custom scheme also happens to be a domain.

> > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:502
> > +        auto isAppBound = schemeIsAppBound || domains.contains(WebCore::RegistrableDomain(requestURL));
> 
> This might be a little more legible as a static function:
> 
> static NavigatingToAppBoundDomain schemeOrDomainIsAppBound(const URL&
> requestURL, const <<mumble>>& domains, const <<mumble>>& schemes) {
>     auto protocol = requestURL.protocol().toString();
>     auto schemeIsAppBound = !protocol.isNull() && schemes.contains(protocol);
>     auto domainIsAppBound =
> domains.contains(WebCore::RegistrableDomain(requestURL));
>     return schemeIsAppBound || domainIsAppBound ?
> NavigatingToAppBoundDomain::Yes : NavigatingToAppBoundDomain::No;
> }
> 

Agreed, will change.

> > Tools/TestWebKitAPI/Info.plist:8
> > +		<string>:</string>
> 
> Ha! Good test!
> 

:-)

> > Tools/TestWebKitAPI/Info.plist:28
> > +		<string>should-not-be-included:</string>
> 
> Should we have an empty <string></string> test?

This case is covered already in the Info.plist! (It is not shown in the diff).

[Kate Cheney]

Created attachment 403412 [details]
Patch for landing

[John Wilander]

(In reply to katherine_cheney from comment #6)
> (In reply to John Wilander from comment #3)
> > If possible, it would be nice to test that this'll work with the ITP
> > relaxation for app-bound domains too.
> 
> Do you have a specific example in mind where you think it might not work
> with ITP relaxation?

I'm thinking a test that checks that a third-party with a custom scheme gets access to its cookies if it and the first party are both in the app-bound set.

[John Wilander]

(In reply to John Wilander from comment #11)
> (In reply to katherine_cheney from comment #6)
> > (In reply to John Wilander from comment #3)
> > > If possible, it would be nice to test that this'll work with the ITP
> > > relaxation for app-bound domains too.
> > 
> > Do you have a specific example in mind where you think it might not work
> > with ITP relaxation?
> 
> I'm thinking a test that checks that a third-party with a custom scheme gets
> access to its cookies if it and the first party are both in the app-bound
> set.

ITP only looks at RegistrableDomain but there might be something in how we extract RegistrableDomains from URLs that makes this not work.

[Kate Cheney]

(In reply to John Wilander from comment #12)
> (In reply to John Wilander from comment #11)
> > (In reply to katherine_cheney from comment #6)
> > > (In reply to John Wilander from comment #3)
> > > > If possible, it would be nice to test that this'll work with the ITP
> > > > relaxation for app-bound domains too.
> > > 
> > > Do you have a specific example in mind where you think it might not work
> > > with ITP relaxation?
> > 
> > I'm thinking a test that checks that a third-party with a custom scheme gets
> > access to its cookies if it and the first party are both in the app-bound
> > set.
> 
> ITP only looks at RegistrableDomain but there might be something in how we
> extract RegistrableDomains from URLs that makes this not work.

I see, good point. This should still work fine because the app-bound domains HashSet does not change at all. But, it might still worth be writing a test case for in a followup patch.

[EWS]

Committed r263874: <https://trac.webkit.org/changeset/263874>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403412 [details].