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
Fixes #7283: make BasicCredentials constructor safe #7284
Fixes #7283: make BasicCredentials constructor safe #7284
Conversation
case ix => apply(userPass.substring(0, ix), userPass.substring(ix + 1), charset) | ||
@deprecated("Use fromString instead", "0.23.24") | ||
def apply(token: String): BasicCredentials = | ||
fromString(token).get |
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.
If I'm not wrong, previously there was an IllegalArgumentException
, and now it will be NoSuchElementException
. Although the difference is subtle, shouldn't we preserve the behaviour?
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.
Good point!
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.
Fixed this one.
userPass.indexOf(':') match { | ||
case -1 => apply(userPass, "", charset) | ||
case ix => apply(userPass.substring(0, ix), userPass.substring(ix + 1), charset) | ||
private def fromString0(token: String): Try[BasicCredentials] = |
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.
Do you have any suggestions for a better name?
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.
Personally I use Impl
but it's just a private method, I don't really care :)
private def fromString0(token: String): Try[BasicCredentials] = | |
private def fromStringImpl(token: String): Try[BasicCredentials] = |
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 for iterating!
userPass.indexOf(':') match { | ||
case -1 => apply(userPass, "", charset) | ||
case ix => apply(userPass.substring(0, ix), userPass.substring(ix + 1), charset) | ||
private def fromString0(token: String): Try[BasicCredentials] = |
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.
Personally I use Impl
but it's just a private method, I don't really care :)
private def fromString0(token: String): Try[BasicCredentials] = | |
private def fromStringImpl(token: String): Try[BasicCredentials] = |
This PR provides a fix for #7283 by replacing the existing
BasicCredentials.apply
constructor withBasicCredentials.fromString
and using it inBasicCredentials.unapply
to make pattern matching safe too.