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

Jump to bottom

more private Referrer-Policy: use strict-origin-when-cross-origin #17382

Merged
merged 1 commit into from Jun 4, 2021
Merged

more private Referrer-Policy: use strict-origin-when-cross-origin #17382

merged 1 commit into from Jun 4, 2021

Conversation

Findus23
Copy link
Member

related to #17381, #15673 and matomo-org/matomo-nginx#61

At the moment we are sending a less private Referrer-Policy than the one that would be used if we didn't send one.

So now

  • the full referrer is sent to the same origin
  • just the domain is sent to another origin (as long as it is using HTTPS)
  • nothing is sent to HTTP sites
    (as long as $this->useStrictReferrerPolicy is not set in which case nothing is sent to other origins)

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@Findus23 Findus23 added the Needs Review PRs that need a code review label Mar 23, 2021
@Findus23 Findus23 mentioned this pull request Mar 23, 2021
Open
@diosmosis
Copy link
Member

Code looks good to me 👍, not sure we'd ever want to send the full matomo url referrer to another domain.

Any thoughts @tsteur, @flamisz, @sgiehl?

@tsteur
Copy link
Member

tsteur commented Mar 28, 2021

Hard to say if it would break anything. I remember #14482 broke the overlay when we tried various different settings earlier and we now worked around it with https://github.com/matomo-org/matomo/pull/14766/files

Generally sounds good to do this 👍 FYI seems it's not supported by IE11

And I suppose nothing changes when someone switches from HTTP to HTTPS? Eg when logging in then we don't look at the full referrer? not sure if something behaves different there maybe (didn't check in detail)

@flamisz
Copy link
Contributor

flamisz commented Mar 29, 2021

In the code we have the comment always send explicit default header, and looks like this is the new default header. I'm not sure about what could it break, but definitely the more secure and up-to-date solution.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 4, 2021

This issue is in "needs review" but there has been no activity for 7 days. ping @tsteur @sgiehl @diosmosis @flamisz

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Jun 4, 2021
@Findus23
Copy link
Member Author

Findus23 commented Jun 4, 2021

Maybe we could merge this quite early after a release, so there is a longer time to notice if this breaks something in a subtil way.

@diosmosis diosmosis added this to the 4.4.0 milestone Jun 4, 2021
@diosmosis
Copy link
Member

Sounds good to merge this early (cc @tsteur)

@diosmosis diosmosis merged commit 6b091d2 into 4.x-dev Jun 4, 2021
@diosmosis diosmosis deleted the strict-origin-when-cross-origin branch June 4, 2021 17:14
@mattab mattab changed the title use strict-origin-when-cross-origin Referrer-Policy more private Referrer-Policy: use strict-origin-when-cross-origin Jul 27, 2021
@tsteur tsteur mentioned this pull request Jul 27, 2021
Closed
tsteur added a commit that referenced this pull request Jul 30, 2021
@tsteur
Revert "use strict-origin-when-cross-origin Referrer-Policy (#17382)"
21180da 
This reverts commit 6b091d2.
@tsteur tsteur mentioned this pull request Jul 30, 2021
Merged
justinvelluppillai pushed a commit that referenced this pull request Jul 30, 2021
@tsteur
Revert "use strict-origin-when-cross-origin Referrer-Policy (#17382)" (…
492ed36 
…#17842)

This reverts commit 6b091d2.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Needs Review PRs that need a code review Stale The label used by the Close Stale Issues action
Projects
None yet
Milestone
4.4.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Findus23 @diosmosis @tsteur @flamisz