-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
BCryptPasswordEncoder handle null rawPassword #7981
Conversation
I believe that the method of BCryptPasswordEncoder's matches() should have a way to detect whether the rawPassword is null.
I believe that the method of BCryptPasswordEncoder's matches() should have a way to detect whether the rawPassword is null.
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, @ge1mina023! I've left some feedback inline.
@@ -119,6 +119,11 @@ public boolean matches(CharSequence rawPassword, String encodedPassword) { | |||
return false; | |||
} | |||
|
|||
if(rawPassword.toString() == null || rawPassword.toString.length() == 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.
Let's have this match the earlier check:
if(rawPassword.toString() == null || rawPassword.toString.length() == 0){ | |
if (rawPassword == null || rawPassword.length() == 0) { |
Also, for readability, we should probably place this at the beginning of the method, so that the method parameters are testing in the same order that they are defined.
logger.warn("Empty raw password"); | ||
return false; | ||
} | ||
|
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.
Would you also add a couple of tests to BCryptPasswordEncoderTests
? I think that doesntMatchNullEncodedValue
and doesntMatchEmptyEncodedValue
are probably good examples to follow.
@ge1mina023 in addition to the feedback I left above, would you please compile the code locally with ./gradle check and squash your commits? |
Closed in favor of #8330 |
Detecting the NullPointerException of rawPassword
I believe that the method of BCryptPasswordEncoder's matches() should have a way to detect whether the rawPassword is null.Because Spring Security will delete the password of authentication stored in the SecurityContext after I log in, so my other authenticate request will get a NullPointerException from BCryptPasswordEncoder's matches().There should have a way to detect the NullPointerException of rawPassword