Bug 213889

<rdar://problem/64804671>

Created attachment 403378 [details] Patch

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

Comment on attachment 403378 [details] Patch Cancelling review while investigating failing API tests.

Created attachment 403395 [details] Patch

(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?

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?

Comment on attachment 403395 [details] Patch I think this patch is good, but I have a couple of suggestions I hope you'll consider.

(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).

Created attachment 403412 [details] Patch for landing

(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.

(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.

(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.

Committed r263874: <https://trac.webkit.org/changeset/263874> All reviewed patches have been landed. Closing bug and clearing flags on attachment 403412 [details].