-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Referrers: Flip ReducedReferrerGranularity to enabled by default #26441
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The review process for this patch is being conducted in the Chromium project.
c3ea979
to
b4d16af
Compare
c22b707
to
f32a6e8
Compare
Hey @annevk, I'm wondering if you could take a look at the non-generated files in this PR. We'd like to land this change which is currently blocking the merge of w3c/webappsec-referrer-policy#142, but since we're changing an important default value in the spec, I want to make sure another vendor takes a look. There are not too many files in this PR to take a look at when you exclude all of the Edit: We've determined that the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks reasonable though I didn't look at all the things. Is this default referrer policy also reflected in fetch requests that end up in a service worker and is that tested?
One thing that seems broken and needs a follow-up is that spec.src.json
is not JSON. You cannot have comments in JSON.
Will bugs be filed against other browsers as part of the specification change? Or are they already filed? I lost track I'm afraid.
test(function () { | ||
assert_equals(doc.referrer, document.URL, "The document's referrer should be its creator document's address."); | ||
test(function() { | ||
assert_equals(doc.referrer, document.location.origin + '/', "The document's referrer should be its creator document's address's origin."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: no need for "address" here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review!
Nit: no need for "address" here.
Done; thanks for the suggestion
I think this looks reasonable though I didn't look at all the things. Is this default referrer policy also reflected in fetch requests that end up in a service worker and is that tested?
Yes, this is covered by tests changed in the linked CL, like fetch-event-referrer-policy.https.html.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, any feedback on the remainder of the comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maudnals Hey Maud, do you know offhand if Firefox and WebKit have tracking bugs specifically for changing the default referrer policy? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re JSON, I filed issue 26528 to track this: a quick codesearch shows it's not confined to the referrer policy tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned over here, Firefox does. I think Safari doesn't because they've already rolled out a superset of this change (discussion on that thread and privacycg/proposals#13). Ultimately this change allows everyone to align more to Safari, and bring Safari's behavior closer to the spec. We'll consider the additional referrer trimming that they do in the PrivacyCG is my understanding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought Safari didn't do a superset after all as they differed for subresources and navigations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I filed https://bugs.webkit.org/show_bug.cgi?id=218909 against WK. I think it'd be accurate to describe their behavior as "roughly a superset of strict-origin-when-cross-origin
, with a few rough edges": for instance, there are longstanding bugs where various HTML elements' referrerpolicy
attributes no-op, which I think are just too low-priority to attract attention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think it is slightly unclear how safari behaves in all cases, given the tracking bug above and at least discrepancies like https://bugs.webkit.org/show_bug.cgi?id=217758. I don't think this change is contentious, but I think the outcome of the referrer trimming discussion is where there are the more divergent views
f32a6e8
to
4b678d0
Compare
We rolled out ReducedReferrerGranularity, which changes the default referrer policy to strict-origin-when-cross-origin, to 100% in M85 stable. To clean up the experiment, we need to enable the behavior by default. This will take effect in M88; we'll follow up by removing the flag, the corresponding enterprise policy, and the corresponding field trial testing configuration. Changing this base::Feature's default value entails cleaning up the remaining tests that weren't within the field trial testing config's scope. These changes are mostly straightforward, involving updating expectations of full-URL referrers to expectations of the corresponding origins, but some tests require logic changes to make sure that they still cover the desired behavior. (For instance, multiple tests previously expected origins in order to test that a particular, arbitrary, non-default referrer policy took effect: to achieve a similar effect, this CL updates these tests to now expect full URLs and swaps in non-default full-URL-generating policies for the prior non-default origin-generating policies.) Launch approval: crbug.com/1019930 Spec change: w3c/webappsec-referrer-policy#142 Bug: 1014207, 1131688 Change-Id: Ib575af6a858641fb1fe2c8de73941f5702d88191 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2429247 Reviewed-by: Charlie Reis <[email protected]> Reviewed-by: Dominic Farolino <[email protected]> Reviewed-by: Kunihiko Sakamoto <[email protected]> Reviewed-by: Arthur Sonzogni <[email protected]> Reviewed-by: Jeremy Roman <[email protected]> Commit-Queue: David Van Cleve <[email protected]> Cr-Commit-Position: refs/heads/master@{#828098}
4b678d0
to
14be535
Compare
We rolled out ReducedReferrerGranularity, which changes the default
referrer policy to strict-origin-when-cross-origin, to 100% in M85
stable. To clean up the experiment, we need to enable the behavior by
default. This will take effect in M88; we'll follow up by removing the
flag, the corresponding enterprise policy, and the corresponding field
trial testing configuration.
Changing this base::Feature's default value entails cleaning up the
remaining tests that weren't within the field trial testing config's
scope. These changes are mostly straightforward, involving updating
expectations of full-URL referrers to expectations of the corresponding
origins, but some tests require logic changes to make sure that they
still cover the desired behavior. (For instance, multiple tests
previously expected origins in order to test that a particular,
arbitrary, non-default referrer policy took effect: to achieve a similar
effect, this CL updates these tests to now expect full URLs and swaps
in non-default full-URL-generating policies for the prior non-default
origin-generating policies.)
Launch approval: crbug.com/1019930
Spec change: w3c/webappsec-referrer-policy#142
Bug: 1014207, 1131688
Change-Id: Ib575af6a858641fb1fe2c8de73941f5702d88191
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2429247
Reviewed-by: Charlie Reis <[email protected]>
Reviewed-by: Dominic Farolino <[email protected]>
Reviewed-by: Kunihiko Sakamoto <[email protected]>
Reviewed-by: Arthur Sonzogni <[email protected]>
Reviewed-by: Jeremy Roman <[email protected]>
Commit-Queue: David Van Cleve <[email protected]>
Cr-Commit-Position: refs/heads/master@{#828098}