Skip to content

Improve the validation of auth_pass#613

Closed
jcppman wants to merge 2 commits into
redis:masterfrom
jcppman:master
Closed

Improve the validation of auth_pass#613
jcppman wants to merge 2 commits into
redis:masterfrom
jcppman:master

Conversation

@jcppman

@jcppman jcppman commented Jun 26, 2014

Copy link
Copy Markdown

If auth_pass is set, but the value is falsy (e.g: an empty string), do_auth will not be evaluated, so the callback of client.auth(pass, callback) will never be called. Instead of check if(auth_pass), now we use if(auth_pass !== undefined) to prevent such situation.

Although pass the empty string or other falsy sutff as a password make no sense, but we should leave this validation to the redis and at least let the callback of client.auth(pass, callback) be evaluated.

If auth_pass is set, but the value is falsy like '', do_auth will not be
evaluated, so the callback will never be called. Instead of check
if(auth_pass), now we use if(auth_pass !== undefined) to prevent such
situation.
@brycebaril

Copy link
Copy Markdown
Contributor

How about != null to coalesce null and undefined

@jcppman

jcppman commented Jul 9, 2014

Copy link
Copy Markdown
Author

Sorry, I was wrong, maybe adding the validation here cannot actually solve the problem (at least not enough).

There's still a problem, if someone pass null or undefined into RedisClient.auth, the callback will never be called (I know, it sounds stupid, but somehow it might happen). So the actual problem is that RedisClient.auth will set auth_pass to a falsy value, then the callback will never be called. Maybe the RedisClient.auth need some code to check the input? Like this:

var auth_pass = args[0];
if (auth_pass != null) {
  // set this.auth_pass, this.auth_callback and doing rest of the job 
} else {
  // call the callback with err like INVALID_PASS or some
}

What do you think?

@brycebaril

Copy link
Copy Markdown
Contributor

At some level this is starting to feel over-protective.

It may be that this is in the realm of "let the user shoot themself in the foot" -- the library simply can't protect everyone from every possible type of mistake.

Use != instead of !== to coalesce null and undefined
@jcppman

jcppman commented Jul 9, 2014

Copy link
Copy Markdown
Author

Yeah, I understand that it is not proper to protect library user in all these kind of mistakes.

But if a callback is provided, maybe it should always be called, no matter the method is successfully evaluated or not.

This is just my personal opinion, if it is not correct/proper, I am sorry!

@jcppman

jcppman commented Jul 9, 2014

Copy link
Copy Markdown
Author

Anyway, this is just a matter of taste, I have pushed the new commit, if you think this is quite enough, feel free to close this request :)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants