Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Validate Crypto::Bcrypt::Password is a valid hash #6467

Merged
merged 2 commits into from
Jan 7, 2020
Merged

Validate Crypto::Bcrypt::Password is a valid hash #6467

merged 2 commits into from
Jan 7, 2020

Conversation

miketheman
Copy link
Contributor

Resolves #5357

Signed-off-by: Mike Fiedler miketheman@gmail.com

@miketheman
Copy link
Contributor Author

#CodeTriage
Here's the fix for the issue reported by @crisward
The proposed change to the solution by @straight-shoota seemed likely, however since .split isn't being passed remove_empty=True, parts[0] is an empty string.

I figured it was better to use the value of 4 rather than to change the other index values.

@ysbaddaden
Copy link
Contributor

The pattern is more complex than that. Pass $$$ and you'll get more errors down the line for example.

straight-shoota
straight-shoota approved these changes Feb 11, 2019
View reviewed changes
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @ysbaddaden notes, this is by far not a full validation, but it's a first improvement.
I'm not sure if it should close #5357 though...

@straight-shoota
Copy link
Member

@miketheman Could you rebase on master to make sure it's all good?

@miketheman
Validate Crypto::Bcrypt::Password is a valid hash
5dae49e 
Resolves crystal-lang#5357

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
@miketheman
Copy link
Contributor Author

Rebased!

@Sija @miketheman
Update spec/std/crypto/bcrypt/password_spec.cr
4bc92a5 
Co-Authored-By: miketheman <miketheman@gmail.com>
asterite
asterite reviewed Mar 17, 2019
View reviewed changes
Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good, but I think that to_i can also fail (and maybe other cases?)

@straight-shoota straight-shoota added this to the 0.33.0 milestone Jan 7, 2020
@straight-shoota straight-shoota merged commit d84b708 into crystal-lang:master Jan 7, 2020
@straight-shoota
Copy link
Member

Merging this, further enhancements are always possible.

Thank you @miketheman ❤️

@miketheman miketheman deleted the miketheman/5357 branch January 7, 2020 18:50
@miketheman
Copy link
Contributor Author

Thanks @straight-shoota !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sija Sija Sija left review comments

@asterite asterite asterite left review comments

@jhass jhass jhass approved these changes

@straight-shoota straight-shoota straight-shoota approved these changes

Assignees
No one assigned
Labels
kind:feature topic:stdlib:crypto
Projects
None yet
Milestone
0.33.0
Development

Successfully merging this pull request may close these issues.

Crytpo::Bcrypt not handling invalid hash strings nicely
6 participants
@miketheman @ysbaddaden @straight-shoota @Sija @jhass @asterite