matomo 15278 Make SecureHash() use a secure hash
Fix issue https://github.com/issues/matomo-org/matomo/15278 Secure Hash is not secure. Use passwordHelper->hash() which uses the Password::preferredAlgorithm() hash.
Thanks @mwithheld. I'm not sure why the tests didn't run on this PR but to make them pass it looks like
PasswordResetterTest needs updating to allow for the new characters possible in
passwordHelper->hash function. Eg replace
preg_match('/resetToken=[\s]*3D([a-zA-Z0-9=\s]+)<\/p>/', $body, $matches); with
preg_match('/resetToken=[\s]*3D([^<]+)<\/p>/', $body, $matches); or similar.
Good find -- I totally get it and will tweak that regex. I can see how to trigger the error manually, but can you tell me what tests brought this to your attention? I see no test failures when I run
./console tests:run /opt/bitnami/matomo/plugins/Login/tests/Integration/PasswordResetterTest.php, for example, and have so far been unable to run all tests (due to OmniFixture problems).
@mwithheld that command produces the following results for me, all related to that regex not finding the token from the email.
FAILURES! Tests: 7, Assertions: 9, Failures: 5.
So you are running the right tests, I'm not sure why they are passing if you are running them against the change you made.
Got it - my file sync to docker was not working the way I thought it was. I've reproduced the 5 failures, and confirmed your suggested change to the test fixes the issue. A commit is incoming.
Yeah that is not part of my PR. It seems to be from rebasing on the 4.x-dev branch. 7f88a735 Fix multiple conversion selection in edit goal form (#17974) I have reset my branch to after my first commit, the added the correct second commit. Of course, now my branch is behind 4.x-dev by a handful of commits.
On Tue, Sep 7, 2021 at 4:08 PM Justin Velluppillai @.***> wrote:
@.**** commented on this pull request.
In plugins/Goals/angularjs/manage-goals/manage-goals.controller.js https://github.com/matomo-org/matomo/pull/17961#discussion_r703910526:
@@ -50,7 +50,7 @@ }
self.goal.matchAttribute = matchAttribute;
self.goal.allowMultiple = allowMultiple;
self.goal.allowMultiple = allowMultiple == true ? "1" : "0";
has this change accidentally been included from another PR?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/matomo-org/matomo/pull/17961#pullrequestreview-748497514, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGCSOVQ6EZNIVAAY3Z7ZETUA2LQDANCNFSM5DK4QTMQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
It's ok if your branch is behind a little, it looks like it will still merge.
Your PR is not running tests because travis seems to be flagging it as "Abuse detected". Maybe you could try logging in to travis directly as suggested here https://travis-ci.community/t/some-pr-are-not-analyzed-with-message-abuse-detected/1191/8 and see if that fixes it for future PRs?
My pleasure to work with you :) I've logged into Travis and hope that cleans up the problem - would sure be helpful to see the test results.
I will revert this change, as we can't use a hash algorithm that has varying results for the same input with the current implementation. When resetting the password we are trying to regenerate the reset token and compare it with the given one. This currently fails, and thus the password reset is broken. Would have been visible in a UI tests, if tests would have run 🙈 See https://github.com/matomo-org/matomo/blob/7e82e80d7aa44714a412a0b6fd6cb53164b8ab6d/plugins/Login/PasswordResetter.php#L248-L262
- matomo no entry or class found for 'view.clearcompiledtemplates.enable'
- Notice - unserialize(): Error at offset 0 of 53988 bytes
- matomo secureHash is not secure
- matomo error when calling newVersionAvailable from CoreUpdater on Windows
- matomo signout confirmation
- matomo macOS 11 falsely detected as macOS 10.15
- matomo add write to adding_segment_requires_access
- matomo remove "refer us" feature