Bug 209761 – Experimental: Enforce SameSite=strict for domains classified as bounce trackers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[John Wilander]

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.

[John Wilander]

<rdar://problem/59394943>

[John Wilander]

Created attachment 394957 [details]
Patch

[John Wilander]

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.

[Kate Cheney]

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.

[John Wilander]

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

[John Wilander]

Created attachment 394993 [details]
Patch

[Brent Fulgham]

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! :-)

[John Wilander]

Thanks, Brent! I'll put it on the queue given that the ios-wk2 bot was happy with the previous patch.

[EWS]

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

[John Wilander]

Created attachment 395001 [details]
Patch for landing

[EWS]

Committed r259275: <https://trac.webkit.org/changeset/259275>

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