Bug 198227 – Resource Load Statistics: Downgrade document.referrer to the referrer's eTLD+1 if the page was navigated to with a prevalent resource referrer containing link decoration

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Wilander]

Reading document.referrer can be used to pass tracking information in cross-site navigations. We should downgrade document.referrer if the webpage was navigated to from a prevalent resource and the referrer has link decoration.

[Radar WebKit Bug Importer]

<rdar://problem/51117258>

[John Wilander]

Created attachment 370601 [details]
Patch

[John Wilander]

Created attachment 370605 [details]
Patch

[John Wilander]

WPE build fix.

[Alex Christensen]

Comment on attachment 370605 [details]
Patch

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

r=me with a few comments.

> Source/WebCore/page/CrossSiteNavigationDataTransfer.h:34
> +    enum Flag {

enum class Flag : uint8_t

> Source/WebKit/UIProcess/WebProcessPool.cpp:2528
> +    m_networkProcess->didCommitCrossSiteLoadWithDataTransfer(sessionID, fromDomain, toDomain, navigationDataTransfer, pageID, [] { });

I don't understand why this completion handler is necessary.  Could we remove it and have this message just be a notification?  That would make this code a lot simpler.

> LayoutTests/http/tests/resourceLoadStatistics/downgraded-referrer-for-navigation-with-link-query-from-prevalent-resource.html:14
> +        if (document.referrer === prevalentResourceOrigin + "/")

If this test is flaky, we should spin until this is true.  I hope it's not flaky, but I think it's currently based on a race condition.

[John Wilander]

Created attachment 370658 [details]
Patch for landing

[John Wilander]

Thank you so much for pitching in on a Saturday, Alex!

(In reply to Alex Christensen from comment #5)
> Comment on attachment 370605 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370605&action=review
> 
> r=me with a few comments.
> 
> > Source/WebCore/page/CrossSiteNavigationDataTransfer.h:34
> > +    enum Flag {
> 
> enum class Flag : uint8_t

Fixed.

> > Source/WebKit/UIProcess/WebProcessPool.cpp:2528
> > +    m_networkProcess->didCommitCrossSiteLoadWithDataTransfer(sessionID, fromDomain, toDomain, navigationDataTransfer, pageID, [] { });
> 
> I don't understand why this completion handler is necessary.  Could we
> remove it and have this message just be a notification?  That would make
> this code a lot simpler.

I put there in anticipation of building test infrastructure but the only one needed is for WebResourceLoadStatisticsStore::logCrossSiteLoadWithLinkDecoration() which is called from NetworkProcess::setCrossSiteLoadWithLinkDecorationForTesting(). I removed the others.

> > LayoutTests/http/tests/resourceLoadStatistics/downgraded-referrer-for-navigation-with-link-query-from-prevalent-resource.html:14
> > +        if (document.referrer === prevalentResourceOrigin + "/")
> 
> If this test is flaky, we should spin until this is true.  I hope it's not
> flaky, but I think it's currently based on a race condition.

Good call. Fixed.

[WebKit Commit Bot]

Comment on attachment 370658 [details]
Patch for landing

Clearing flags on attachment: 370658

Committed r245784: <https://trac.webkit.org/changeset/245784>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ehsan Akhgari [:ehsan]]

Do I understand correctly that this change only changes the script visible value of document.referrer and not the network visible value (e.g. the value that gets used here: https://html.spec.whatwg.org/#navigating-across-documents:the-document's-referrer)?

[John Wilander]

(In reply to Ehsan Akhgari [:ehsan] from comment #10)
> Do I understand correctly that this change only changes the script visible
> value of document.referrer and not the network visible value (e.g. the value
> that gets used here:
> https://html.spec.whatwg.org/#navigating-across-documents:the-document's-
> referrer)?

Yes, this only affects the JavaScript API, not the protocol part.