@mwithheld opened this Pull Request on September 3rd 2021 Contributor

Fix issue #15278 Secure Hash is not secure. Use passwordHelper->hash() which uses the Password::preferredAlgorithm() hash.

@justinvelluppillai commented on September 6th 2021 Contributor

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.

@mwithheld commented on September 7th 2021 Contributor

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

@justinvelluppillai commented on September 7th 2021 Contributor

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

@mwithheld commented on September 7th 2021 Contributor

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.

@mwithheld commented on September 7th 2021 Contributor

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.

@justinvelluppillai commented on September 8th 2021 Contributor

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?

@mwithheld commented on September 8th 2021 Contributor

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.

@sgiehl commented on September 8th 2021 Member

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

This Pull Request was closed on September 8th 2021
Powered by GitHub Issue Mirror