Skip to content
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

Merged
merged 1 commit into from Nov 17, 2020

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Nov 7, 2020

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 <creis@chromium.org>
Reviewed-by: Dominic Farolino <dom@chromium.org>
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: David Van Cleve <davidvc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828098}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a 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.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-2429247 branch 4 times, most recently from c3ea979 to b4d16af Compare November 7, 2020 03:59
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-2429247 branch 7 times, most recently from c22b707 to f32a6e8 Compare November 10, 2020 19:29
@domfarolino
Copy link
Member

domfarolino commented Nov 11, 2020

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 */gen/* ones under //referrer-policy, so hopefully this isn't too big an ask :)


Edit: We've determined that the wpt-chrome-dev-stability bot failure is unrelated, so no worries there.

Copy link
Member

@annevk annevk left a 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.");
Copy link
Member

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.

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.

Copy link
Member

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?

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!

Copy link

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

Copy link
Member

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

Copy link
Member

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?

Copy link

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.

Copy link
Member

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

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 <creis@chromium.org>
Reviewed-by: Dominic Farolino <dom@chromium.org>
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: David Van Cleve <davidvc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828098}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants