@peterhashair opened this Pull Request on September 23rd 2021 Contributor

Description:

Fixes: #16607
split nonce verifies into 3 parts. By extending the verifyNonce function pass 4th params with true or false, the default won't show details. I am not sure about the security policy around this one, I know some errors required generic error messages to prevent getting too many details, but for this case, I don't think will applied.

  • Token mismatch (InvalidNonceToken)
  • Referrer (InvalidNonceReferrer)
  • Origin (InvalidNonceOrigin)

  • deprecated getMessageExceptionNoAccess Login/controller
  • deprecated InvalidNonceOrHeadersOrReferrer in lang
  • move getMessageExceptionNoAccessWhenInsecureConnectionMayBeUsed from Login/controller to core/Nonce
  • fix missing 2 variables in translation.

Review

@Findus23 commented on October 4th 2021 Member

Honestly I meant in #16607 that we should log or output more precisely which one of the many checks in https://github.com/matomo-org/matomo/blob/679e73f1236969db0c2d767655cb84456a727d24/core/Nonce.php#L70 and https://github.com/matomo-org/matomo/blob/06d43857c48ada2fa7f1ad18a8309e8826c0e413/core/Url.php#L547 fails instead of leaving the user guessing and debugging Matomo code.

Adding the sentence by @Starker3 is also fine, but doesn't affect the core issue I meant.

@peterhashair unrelated, but it would be great if you could link issues in the PR description, so one can directly jump back to them instead of searching for the linked issue in the sidebar. And if you use something like fixes <a href='/1234'>#1234</a>, you don't need to manually link them either.

@peterhashair commented on October 5th 2021 Contributor

@Findus23 Sorry about the fixes issue number, add them back. If I understand correctly, we want to split the verifyNonce into details with their own error messages, instead of combining them into one error log?

@peterhashair commented on October 11th 2021 Contributor

@tsteur split the error message, but I don't think $expectedReferrerHost is used anywhere. Should we set a variable in config.ini.php that reflected that value?

@peterhashair commented on October 11th 2021 Contributor
  • deprecated getMessageExceptionNoAccess Login/controller
  • deprecated InvalidNonceOrHeadersOrReferrer in lang
  • move getMessageExceptionNoAccessWhenInsecureConnectionMayBeUsed from Login/controller to core/Nonce
  • fix missing 2 variables in translation.
@tsteur commented on October 12th 2021 Member

@peterhashair just tested it when Url::isSecureConnectionAssumedByPiwikButNotForcedYet== true

It now shows this
image

it doesn't look quite correct? I think previously it was appended to any error message.

@peterhashair commented on October 12th 2021 Contributor

@tsteur sorry, I thought that was a generic error. Changed it back, now should only appear when the error appears.

This Pull Request was closed on October 13th 2021
Powered by GitHub Issue Mirror