Fix basic auth options type issue #584
Conversation
swr1bm86
force-pushed
the
basic-auth-options-compliance
branch
2 times, most recently
from
347d688 to
785e4f9
Compare
|
ping @thornjad |
thornjad
left a comment
thornjad
left a comment
There was a problem hiding this comment.
Great catch and fix! And great job with the tests!
| var usernameEqual = secureCompare(options.username.toString(), credentials.name); | ||
| var passwordEqual = secureCompare(options.password.toString(), credentials.pass); |
There was a problem hiding this comment.
Should we also convert the credentials to strings? If the server were given a numeric password, as in the tests you added, the user will likely expect numeric to work. Or are we guaranteed to receive a string as credentials?
There was a problem hiding this comment.
from the definition of basic-auth which we use here to parse credentials, I think, yes, the credentials are guaranteed to be string :)
There was a problem hiding this comment.
Okay, perfect, that looks like it will always be a string! Could we just add in a comment here saying the same and/or why we need to use .toString() here? It could be added to the comment above the if, or a new comment within the if block?
There was a problem hiding this comment.
ping @thornjad , sorry for the late replay, the comment is added, pls review again :)
There was a problem hiding this comment.
Coule we also mention in the comment that credentials.name and credentials.pass will be strings, so we don't need to convert them to strings?
swr1bm86
force-pushed
the
basic-auth-options-compliance
branch
from
785e4f9 to
eaf82a5
Compare
| var usernameEqual = secureCompare(options.username.toString(), credentials.name); | ||
| var passwordEqual = secureCompare(options.password.toString(), credentials.pass); |
There was a problem hiding this comment.
Coule we also mention in the comment that credentials.name and credentials.pass will be strings, so we don't need to convert them to strings?
swr1bm86
force-pushed
the
basic-auth-options-compliance
branch
from
eaf82a5 to
1361eb9
Compare
swr1bm86
force-pushed
the
basic-auth-options-compliance
branch
from
1361eb9 to
cde6e5b
Compare
|
ping @thornjad |
swr1bm86
force-pushed
the
basic-auth-options-compliance
branch
from
cde6e5b to
bd36f7b
Compare
2 participants
Fixes #583