@Findus23 opened this Pull Request on March 23rd 2021 Member

related to https://github.com/matomo-org/matomo/issues/17381, https://github.com/matomo-org/matomo/pull/15673 and https://github.com/matomo-org/matomo-nginx/pull/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
@diosmosis commented on March 28th 2021 Member

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

Any thoughts @tsteur, @flamisz, @sgiehl?

@tsteur commented on March 28th 2021 Member

Hard to say if it would break anything. I remember https://github.com/matomo-org/matomo/pull/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 commented on March 29th 2021 Contributor

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[bot] commented on June 4th 2021 Contributor

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

@Findus23 commented on June 4th 2021 Member

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 commented on June 4th 2021 Member

Sounds good to merge this early (cc @tsteur)

This Pull Request was closed on June 4th 2021
Powered by GitHub Issue Mirror