Domains that are classified as bounce trackers and hit a certain threshold of unique top frame redirects should have their cookies rewritten as SameSite=strict.
<rdar://problem/59394943>
Created attachment 394957 [details] Patch
Ugh. The tests are failing on mac-wk2 because that bot is on a version of macOS which lacks the CFNetwork APIs. I'll update the expectations file accordingly before landing.
Comment on attachment 394957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394957&action=review > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1653 > + return; Probably need to call the completionHandler here in case of a database I/O error. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1718 > ASSERT_NOT_REACHED(); Nice catch! > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2068 > + result = ensureResourceStatisticsForRegistrableDomain(redirectDomain); This is not necessary, it is done already in ResourceLoadStatisticsDatabaseStore::ensureAndMakeDomainList. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2280 > + Nice! :) > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:455 > + RELEASE_LOG_INFO_IF(debugLoggingEnabled(), ITPDebug, "Did set %" PUBLIC_LOG_STRING " as making a top frame redirect to %" PUBLIC_LOG_STRING ".", sourceDomain.string().utf8().data(), targetDomain.string().utf8().data()); Did you mean to add logging even if the if-statement is false? > LayoutTests/http/tests/resourceLoadStatistics/enforce-samesite-strict-based-on-top-frame-unique-redirects-to-database.html:33 > + finishJSTest(); setEnableFeature(false, finishJSTest)? > LayoutTests/http/tests/resourceLoadStatistics/enforce-samesite-strict-based-on-top-frame-unique-redirects-to-database.html:49 > + finishJSTest(); Ditto. > LayoutTests/http/tests/resourceLoadStatistics/enforce-samesite-strict-based-on-top-frame-unique-redirects-to.html:49 > + finishJSTest(); Ditto > LayoutTests/http/tests/resourceLoadStatistics/enforce-samesite-strict-based-on-top-frame-unique-redirects-to.html:74 > + finishJSTest(); Ditto.
(In reply to katherine_cheney from comment #4) > Comment on attachment 394957 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394957&action=review > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1653 > > + return; > > Probably need to call the completionHandler here in case of a database I/O > error. Good catch! Will fix. > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1718 > > ASSERT_NOT_REACHED(); > > Nice catch! Yeah, I was working my way thorough existing functionality to design mine and came across this copy pasta. > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2068 > > + result = ensureResourceStatisticsForRegistrableDomain(redirectDomain); > > This is not necessary, it is done already in > ResourceLoadStatisticsDatabaseStore::ensureAndMakeDomainList. I kind of suspected that since we hadn't seen issues in testing. I'll just remove the comment then. > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2280 > > + > > Nice! :) Yes, I find this part especially nice. > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:455 > > + RELEASE_LOG_INFO_IF(debugLoggingEnabled(), ITPDebug, "Did set %" PUBLIC_LOG_STRING " as making a top frame redirect to %" PUBLIC_LOG_STRING ".", sourceDomain.string().utf8().data(), targetDomain.string().utf8().data()); > > Did you mean to add logging even if the if-statement is false? Yes, since the two if statements are only false if the entry already exists and I wanted the log to show the event for any developer looking. > > LayoutTests/http/tests/resourceLoadStatistics/enforce-samesite-strict-based-on-top-frame-unique-redirects-to-database.html:33 > > + finishJSTest(); > > setEnableFeature(false, finishJSTest)? Good catch! > > LayoutTests/http/tests/resourceLoadStatistics/enforce-samesite-strict-based-on-top-frame-unique-redirects-to-database.html:49 > > + finishJSTest(); > > Ditto. > > > LayoutTests/http/tests/resourceLoadStatistics/enforce-samesite-strict-based-on-top-frame-unique-redirects-to.html:49 > > + finishJSTest(); > > Ditto > > > LayoutTests/http/tests/resourceLoadStatistics/enforce-samesite-strict-based-on-top-frame-unique-redirects-to.html:74 > > + finishJSTest(); > > Ditto. Thanks for the review! I'll fix all the relevant things. (And thanks for the help on the DB query stuff.)
Created attachment 394993 [details] Patch
Comment on attachment 394993 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394993&action=review > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1740 > + RELEASE_LOG_ERROR_IF_ALLOWED(m_sessionID, "%p - ResourceLoadStatisticsDatabaseStore::clearUserInteraction failed to bind, error message: %{private}s", this, m_database.lastErrorMsg()); Oops! :-)
Thanks, Brent! I'll put it on the queue given that the ios-wk2 bot was happy with the previous patch.
ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.
Created attachment 395001 [details] Patch for landing
Committed r259275: <https://trac.webkit.org/changeset/259275> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395001 [details].