@kl-he
Description:
if an expectedReferrerHost is passed, the nonce-check will fail, if there is no referrer. This was introduced in #17228, as a fix when using GoogleAnalyticsImporter.
While using GoogleAnalyticsImporter, we are seeing a referrer being sent by the browser, so this this will cause the check to fail, as the empty referrer doesn't match google.com.
This fix allows empty referrers again, while also doing expectedReferrerHost and/or localhost, when referrer is specified.
Tests added as integration tests.
./console tests:run --testsuite integration --filter NonceTest
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
- [ ] Wording review done
- [ ] 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
@sgiehl
Hi @kl-he
Thanks for contributing this PR. That's very appreciated.
The code imho looks fine and it's awesome you also added some tests.
I'm not 100% sure about possible security implications of removing that check. In theory that would now allow to simply bypass the expected referrer check by suppressing the referrer header.
On the other side I guess there might be browsers or addons, that suppress that header on purpose for privacy reasons maybe...
Looking at the code before https://github.com/matomo-org/matomo/pull/17228, it looks like empty referrer were accepted as well. So might be fine to simply merge this one.
@tsteur Any thoughts or objections on that one?
@tsteur
@kl-he could you maybe explain the use case a bit more why you need the change? This will help understand things.
We are in the opposite situation as diosmosis was in, when #17228 was submitted.
We are not seeing a referrer being sent (tried with different browsers and distros), when authenticating the Google Analytics Importer plugin, resulting in a failed authorization attempt.
Without #17228 we would not have had a problem, as it allowed empty referrers, as @sgiehl correctly points out.
This pull-request will allow the empty referrers again, but still do the domain-check, if a referrer is passed.
If you go ahead with this pull-request, it might make sense to rename $expectedReferrerHost to $allowedReferrerHost.
@github-actions[bot]
If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.
@tsteur any further notes on this one? I'm otherwise tempted to merge it.
@sgiehl figured it's best to do a search on OWASP's thoughts on this so checked https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#verifying-origin-with-standard-headers and https://owasp.org/www-community/attacks/csrf
Maybe there's a redirect that gets rid of the referrer.
So it seems fine to merge. However, as suggested by @kl-he we could indeed rename the variable to like allowedReferrerHost and we should make clear in the comments for the method how it behaves and that this is expected behaviour that when no referrer is there that it's not checked.
If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.