Bug 219900

<rdar://problem/72062985>

Created attachment 416278 [details] Patch

Comment on attachment 416278 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416278&action=review We're still limiting this to specific domains *in code*, right? So that manipulation of the database cannot be used to grant persistent third-party cookie access? > Source/WebCore/page/Quirks.cpp:1105 > + // We will match Sony login domains up when prompting the user and storing the domains. Does this mean if Sony calls the Storage Access API themselves, they'd trigger that matching and get the special prompt? > Source/WebCore/page/Quirks.cpp:1109 > + ResourceLoadObserver::shared().setDomainsWithCrossPageStorageAccess({{ domain, loginDomain }}, [&element, platformEvent, eventType, detail, relatedTarget] { domain instead of firstPartyDomain indicates that we would trigger the quirk even if the PlayStation site was embedded which we shouldn't do. We should always request storage access under the first party. > Source/WebCore/platform/network/NetworkStorageSession.cpp:219 > void NetworkStorageSession::grantCrossPageStorageAccess(const TopFrameDomain& topFrameDomain, const SubResourceDomain& resourceDomain) As you can see here, the rest of the code assumes that it's being sent the first party domain. The type system doesn't guarantee that so we have to make sure ourselves. Down the road I'd like to leverage the type system to get this enforced. > Source/WebCore/platform/network/NetworkStorageSession.cpp:417 > +Optional<RegistrableDomain> NetworkStorageSession::findAdditionalLoginDomain(const RegistrableDomain& subDomain, const RegistrableDomain& topDomain) I think the order of these parameters we mostly use is topDomain then subDomain. Please reverse since getting it wrong is very easy on the calling side without support from the type system. > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:348 > +static String buildList(const WebCore::RegistrableDomain subDomainOne, const WebCore::RegistrableDomain subDomainTwo) This is a very open ended function name. Could we make it more specific? Perhaps ...ForStorageAccessPrompt? > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:352 > + builder.append(String([NSString stringWithFormat:@"â and â"])); I don't understand this. Some Unicode getting garbled? > Source/WebKit/UIProcess/Cocoa/WKStorageAccessAlert.mm:39 > +static String buildList(const WebCore::RegistrableDomain subDomainOne, const WebCore::RegistrableDomain subDomainTwo) Same comment on function name. Also, this is a copy of the code. Could we write once? > Source/WebKit/UIProcess/Cocoa/WKStorageAccessAlert.mm:43 > + builder.append(String([NSString stringWithFormat:@"â and â"])); Same comment on weird characters.

(In reply to John Wilander from comment #3) > Comment on attachment 416278 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=416278&action=review > > We're still limiting this to specific domains *in code*, right? So that > manipulation of the database cannot be used to grant persistent third-party > cookie access? > I'm not sure I understand exactly what you mean. These domains have to be stored in the database in order to be persistent between sessions, I don't think we can avoid that. > > Source/WebCore/page/Quirks.cpp:1105 > > + // We will match Sony login domains up when prompting the user and storing the domains. > > Does this mean if Sony calls the Storage Access API themselves, they'd > trigger that matching and get the special prompt? > Yes, if they are requesting storage access for sony.com or sonyentertainmentnetwork.com under playstation.com they will get the special prompt. I did consider this, but my understanding was they were migrating away from needing 3rd party cookies entirely so I thought this was fine for the quirk. > > Source/WebCore/page/Quirks.cpp:1109 > > + ResourceLoadObserver::shared().setDomainsWithCrossPageStorageAccess({{ domain, loginDomain }}, [&element, platformEvent, eventType, detail, relatedTarget] { > > domain instead of firstPartyDomain indicates that we would trigger the quirk > even if the PlayStation site was embedded which we shouldn't do. We should > always request storage access under the first party. > Good point, I will change this to be first party only. > > Source/WebCore/platform/network/NetworkStorageSession.cpp:219 > > void NetworkStorageSession::grantCrossPageStorageAccess(const TopFrameDomain& topFrameDomain, const SubResourceDomain& resourceDomain) > > As you can see here, the rest of the code assumes that it's being sent the > first party domain. The type system doesn't guarantee that so we have to > make sure ourselves. Down the road I'd like to leverage the type system to > get this enforced. > Noted, I will fix the above code. > > Source/WebCore/platform/network/NetworkStorageSession.cpp:417 > > +Optional<RegistrableDomain> NetworkStorageSession::findAdditionalLoginDomain(const RegistrableDomain& subDomain, const RegistrableDomain& topDomain) > > I think the order of these parameters we mostly use is topDomain then > subDomain. Please reverse since getting it wrong is very easy on the calling > side without support from the type system. > OK, will reverse. > > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:348 > > +static String buildList(const WebCore::RegistrableDomain subDomainOne, const WebCore::RegistrableDomain subDomainTwo) > > This is a very open ended function name. Could we make it more specific? > Perhaps ...ForStorageAccessPrompt? > Good idea, will fix. > > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:352 > > + builder.append(String([NSString stringWithFormat:@"â and â"])); > > I don't understand this. Some Unicode getting garbled? > Strange, it is converting slanty quotes to these characters. I wonder if it is only the diff technology that is showing them like that? Will investigate. > > Source/WebKit/UIProcess/Cocoa/WKStorageAccessAlert.mm:39 > > +static String buildList(const WebCore::RegistrableDomain subDomainOne, const WebCore::RegistrableDomain subDomainTwo) > > Same comment on function name. Also, this is a copy of the code. Could we > write once? Probably, will adjust in the next upload. > > > Source/WebKit/UIProcess/Cocoa/WKStorageAccessAlert.mm:43 > > + builder.append(String([NSString stringWithFormat:@"â and â"])); > > Same comment on weird characters. :( Thanks for the comments!

(In reply to katherine_cheney from comment #4) > (In reply to John Wilander from comment #3) > > Comment on attachment 416278 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=416278&action=review > > > > We're still limiting this to specific domains *in code*, right? So that > > manipulation of the database cannot be used to grant persistent third-party > > cookie access? > > > > I'm not sure I understand exactly what you mean. These domains have to be > stored in the database in order to be persistent between sessions, I don't > think we can avoid that. These are special, quirks permissions. We have to make sure to restrict those permissions are enforced in code and not just in the database. If the database says that ThirdParty.example should have persistent third-party cookie access under news.example, the code should reject it. We have to make sure in code to restrict this functionality only to the specific sites we build quirks for. > > > Source/WebCore/page/Quirks.cpp:1105 > > > + // We will match Sony login domains up when prompting the user and storing the domains. > > > > Does this mean if Sony calls the Storage Access API themselves, they'd > > trigger that matching and get the special prompt? > > > > Yes, if they are requesting storage access for sony.com or > sonyentertainmentnetwork.com under playstation.com they will get the special > prompt. I did consider this, but my understanding was they were migrating > away from needing 3rd party cookies entirely so I thought this was fine for > the quirk. Should be fine but it's something to keep in mind since we want developers to be able to use our APIs without hitting quirks. > > > Source/WebCore/page/Quirks.cpp:1109 > > > + ResourceLoadObserver::shared().setDomainsWithCrossPageStorageAccess({{ domain, loginDomain }}, [&element, platformEvent, eventType, detail, relatedTarget] { > > > > domain instead of firstPartyDomain indicates that we would trigger the quirk > > even if the PlayStation site was embedded which we shouldn't do. We should > > always request storage access under the first party. > > > > Good point, I will change this to be first party only. > > > > Source/WebCore/platform/network/NetworkStorageSession.cpp:219 > > > void NetworkStorageSession::grantCrossPageStorageAccess(const TopFrameDomain& topFrameDomain, const SubResourceDomain& resourceDomain) > > > > As you can see here, the rest of the code assumes that it's being sent the > > first party domain. The type system doesn't guarantee that so we have to > > make sure ourselves. Down the road I'd like to leverage the type system to > > get this enforced. > > > > Noted, I will fix the above code. > > > > Source/WebCore/platform/network/NetworkStorageSession.cpp:417 > > > +Optional<RegistrableDomain> NetworkStorageSession::findAdditionalLoginDomain(const RegistrableDomain& subDomain, const RegistrableDomain& topDomain) > > > > I think the order of these parameters we mostly use is topDomain then > > subDomain. Please reverse since getting it wrong is very easy on the calling > > side without support from the type system. > > > > OK, will reverse. > > > > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:348 > > > +static String buildList(const WebCore::RegistrableDomain subDomainOne, const WebCore::RegistrableDomain subDomainTwo) > > > > This is a very open ended function name. Could we make it more specific? > > Perhaps ...ForStorageAccessPrompt? > > > > Good idea, will fix. > > > > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:352 > > > + builder.append(String([NSString stringWithFormat:@"â and â"])); > > > > I don't understand this. Some Unicode getting garbled? > > > > Strange, it is converting slanty quotes to these characters. I wonder if it > is only the diff technology that is showing them like that? Will investigate. Thanks! > > > Source/WebKit/UIProcess/Cocoa/WKStorageAccessAlert.mm:39 > > > +static String buildList(const WebCore::RegistrableDomain subDomainOne, const WebCore::RegistrableDomain subDomainTwo) > > > > Same comment on function name. Also, this is a copy of the code. Could we > > write once? > > Probably, will adjust in the next upload. > > > > > > Source/WebKit/UIProcess/Cocoa/WKStorageAccessAlert.mm:43 > > > + builder.append(String([NSString stringWithFormat:@"â and â"])); > > > > Same comment on weird characters. > > :( > > > Thanks for the comments! You're welcome! r=me with the above requested changes and requirements.

(In reply to John Wilander from comment #5) > (In reply to katherine_cheney from comment #4) > > (In reply to John Wilander from comment #3) > > > Comment on attachment 416278 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=416278&action=review > > > > > > We're still limiting this to specific domains *in code*, right? So that > > > manipulation of the database cannot be used to grant persistent third-party > > > cookie access? > > > > > > > I'm not sure I understand exactly what you mean. These domains have to be > > stored in the database in order to be persistent between sessions, I don't > > think we can avoid that. > > These are special, quirks permissions. We have to make sure to restrict > those permissions are enforced in code and not just in the database. If the > database says that ThirdParty.example should have persistent third-party > cookie access under news.example, the code should reject it. We have to make > sure in code to restrict this functionality only to the specific sites we > build quirks for. Oh! Yes, that is definitely enforced. It is not possible to manipulate the database to grant storage access for any other domains -- there are hard-coded checks for the Sony domains under playstation.com while reading data from the db into memory. > > Thanks for the comments! > > You're welcome! r=me with the above requested changes and requirements. thanks! Will fix before landing.

Comment on attachment 416278 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416278&action=review >>> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:352 >>> + builder.append(String([NSString stringWithFormat:@"â and â"])); >> >> I don't understand this. Some Unicode getting garbled? > > Strange, it is converting slanty quotes to these characters. I wonder if it is only the diff technology that is showing them like that? Will investigate. Did some investigating -- see https://bugs.webkit.org/show_bug.cgi?id=197370. It is just the diff technology that makes the characters look strange. They are curly quotes when they print in the user-facing Storage Access prompt.

Created attachment 416364 [details] Patch for landing

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

Comment on attachment 416364 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=416364&action=review > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:348 > +static String buildListForStorageAccessPrompt(const WebCore::RegistrableDomain subDomainOne, const WebCore::RegistrableDomain subDomainTwo) I think you left out some & characters here. > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:354 > + StringBuilder builder; > + builder.append(subDomainOne.string()); > + builder.append(String([NSString stringWithFormat:@"â and â"])); > + builder.append(subDomainTwo.string()); > + return builder.toString(); This should be doable with makeString rather than StringBuilder, which is more efficient. Also, there’s no reason to use NSString here. Could just write this: return makeString(subdomainOne.string(), rightDoubleQuotationMark, " and ", leftDoubleQuotationMark, subdomainTwo.string()); But also, I am concerned about the way we are building this string. Does not seem localizable. It’s a bit of a hack to end and then reopen the quotation mark like this.

Reopening to attach new patch.

Created attachment 416377 [details] Patch

Comment on attachment 416364 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=416364&action=review >> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:348 >> +static String buildListForStorageAccessPrompt(const WebCore::RegistrableDomain subDomainOne, const WebCore::RegistrableDomain subDomainTwo) > > I think you left out some & characters here. You're right, I removed this function entirely in my latest patch. >> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:354 >> + return builder.toString(); > > This should be doable with makeString rather than StringBuilder, which is more efficient. Also, there’s no reason to use NSString here. Could just write this: > > return makeString(subdomainOne.string(), rightDoubleQuotationMark, " and ", leftDoubleQuotationMark, subdomainTwo.string()); > > But also, I am concerned about the way we are building this string. Does not seem localizable. It’s a bit of a hack to end and then reopen the quotation mark like this. Good point. Let me know what you think of the latest patch I uploaded, I think it should fix the localizable string issue.

Created attachment 416429 [details] Patch

Created attachment 416437 [details] Patch

Comment on attachment 416437 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416437&action=review > Source/WebKit/UIProcess/Cocoa/WKStorageAccessAlert.mm:67 > +#if PLATFORM(MAC) > + NSString *alertTitle = [NSString stringWithFormat:WEB_UI_NSSTRING(@"Do you want to allow â%@â and â%@â to use cookies and website data while browsing â%@â?", @"Message for requesting cross-site cookie and website data access."), firstRequestingDomain.get(), secondRequestingDomain.get(), currentDomain.get()]; > +#else > + NSString *alertTitle = [NSString stringWithFormat:WEB_UI_NSSTRING(@"Allow â%@â and â%@â to use cookies and website data while browsing â%@â?", @"Message for requesting cross-site cookie and website data access."), firstRequestingDomain.get(), secondRequestingDomain.get(), currentDomain.get()]; > +#endif > + > + NSString *informativeText = [NSString stringWithFormat:WEB_UI_NSSTRING(@"This will allow â%@â and â%@â to track your activity.", @"Informative text for requesting cross-site cookie and website data access."), firstRequestingDomain.get(), secondRequestingDomain.get()]; I’m a bit puzzled about the use, here and elsewhere, of +[NSString stringWithFormat:] instead of +[NSString localizedStringWithFormat:] since these are uses of formatting for localization. It’s possible that this doesn’t matter in the case where the only format used is %@. I’m also sort of wishing that a little bit more of this code was cross-platform.

(In reply to Darin Adler from comment #16) > Comment on attachment 416437 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=416437&action=review > > > Source/WebKit/UIProcess/Cocoa/WKStorageAccessAlert.mm:67 > > +#if PLATFORM(MAC) > > + NSString *alertTitle = [NSString stringWithFormat:WEB_UI_NSSTRING(@"Do you want to allow â%@â and â%@â to use cookies and website data while browsing â%@â?", @"Message for requesting cross-site cookie and website data access."), firstRequestingDomain.get(), secondRequestingDomain.get(), currentDomain.get()]; > > +#else > > + NSString *alertTitle = [NSString stringWithFormat:WEB_UI_NSSTRING(@"Allow â%@â and â%@â to use cookies and website data while browsing â%@â?", @"Message for requesting cross-site cookie and website data access."), firstRequestingDomain.get(), secondRequestingDomain.get(), currentDomain.get()]; > > +#endif > > + > > + NSString *informativeText = [NSString stringWithFormat:WEB_UI_NSSTRING(@"This will allow â%@â and â%@â to track your activity.", @"Informative text for requesting cross-site cookie and website data access."), firstRequestingDomain.get(), secondRequestingDomain.get()]; > > I’m a bit puzzled about the use, here and elsewhere, of +[NSString > stringWithFormat:] instead of +[NSString localizedStringWithFormat:] since > these are uses of formatting for localization. It’s possible that this > doesn’t matter in the case where the only format used is %@. > It seems like WEB_UI_NSSTRING performs the localization of the string. I am not sure why but [NSString localizedStringWithFormat:] is used in very few places. > I’m also sort of wishing that a little bit more of this code was > cross-platform. me too. That is something to consider for a future upgrade to this code.

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

(In reply to katherine_cheney from comment #17) > It seems like WEB_UI_NSSTRING performs the localization of the string. Yes, that’s right. > I am > not sure why but [NSString localizedStringWithFormat:] is used in very few > places. I think it mostly affects numeric formatting, like swapping the use of "," and "." in countries where one is the thousands separator vs. the decimal separator. So probably no better than +[NSString stringWithFormat:] in most contexts. But I think I’d lean toward using it any time what we are formatting is a localized string for the user.