-
Notifications
You must be signed in to change notification settings - Fork 326
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
Fix admin vulnerability to Brute Force #3492
Fix admin vulnerability to Brute Force #3492
Conversation
…dmin API login attempts"
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.
Seems straightforward.
Here's a suggested tweak for
I've just re-used HBErrorCode 100 on the proof of concept, we'd need to give this particular error its own code to use the Error Page. We might also want to do something similar for the |
On the actual error message, I think |
I'd rebuild the UI for this, it is not a brew that is being accessed, but the admin page. But yeah i can work with that, good point. |
There are some spelling errors and awkward sentences that I think could be improved. I'm no wordsmith (I use 10 words where will 1 will do), but here is my take:
|
package.json
Outdated
@@ -100,6 +100,7 @@ | |||
"expr-eval": "^2.0.2", | |||
"express": "^4.21.0", | |||
"express-async-handler": "^1.2.0", | |||
"express-rate-limit": "^7.2.0", |
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.
We can remove this package now if it's not being used.
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.
You added it to the project for the 429 issues, right?
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.
Took it back out because it didn't make a difference.
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.
I think this is the only thing. Remove that, fix any conflicts, and we can merge. I put my approval so you or I can merge.
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.
I will remove it as soon as i get home, but i won't be able to merge without another review..
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.
Just some remarks on messaging and a little bit of whitespace cleanup.
Marking 'approved' so as to not hold it up.
…nto fix-vulnerability-admin-pages
Small PR, works great, thank you for the reviews and helpful comments! Specially @Gazook89, i know i am not the best at writing messages. |
Added express-rate-limit package and implemented rate limiting for admin API login attempts
Have NOT tested this as of yet, feel free to do so. Should be a limit of 10 attempts per day.