New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make password reset use a secure hash to create reset token #17961
Conversation
Thanks @mwithheld. I'm not sure why the tests didn't run on this PR but to make them pass it looks like |
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 |
@mwithheld that command produces the following results for me, all related to that regex not finding the token from the email.
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. |
plugins/Goals/angularjs/manage-goals/manage-goals.controller.js
Outdated
Show resolved
Hide resolved
Yeah that is not part of my PR. It seems to be from rebasing on the 4.x-dev branch.
7f88a73 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
<#17961 (comment)>:
> @@ -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
<#17961 (review)>,
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mwithheld for your work on this PR!
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. matomo/plugins/Login/PasswordResetter.php Lines 248 to 262 in 7e82e80
|
Fix issue #15278 Secure Hash is not secure. Use passwordHelper->hash() which uses the Password::preferredAlgorithm() hash.