@kl-he opened this Pull Request on November 29th 2022 First_time_contributor

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

@sgiehl commented on December 1st 2022 Member

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 commented on December 2nd 2022 Member

@kl-he could you maybe explain the use case a bit more why you need the change? This will help understand things.

@kl-he commented on December 5th 2022

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] commented on December 20th 2022 Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@sgiehl commented on December 21st 2022 Member

@tsteur any further notes on this one? I'm otherwise tempted to merge it.

@tsteur commented on December 21st 2022 Member

@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.

@sgiehl commented on December 23rd 2022 Member

@kl-he could you maybe apply those suggestions

@kl-he commented on December 27th 2022

@sgiehl Done

@github-actions[bot] commented on January 19th 2023 Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

Powered by GitHub Issue Mirror