Don't require new user passwords to be alphanumeric #273

Closed
wants to merge 2 commits into from

2 participants

@brianewing

Fixes #272. I wasn't quite sure if there are repercussions to this, so perhaps @chrismatthieu or someone can comment on the original reason to enforce alphanumeric passwords.

@contra

If you want to change the regex to throw an error if there are spaces I'll pull this in. Alphanumeric was a simple way of making passwords CLI-friendly. The password-change in the API should also follow these requirements.

@brianewing

I had a read through the child_process source and it doesn't provide any way to escape shell arguments.

Nodester should definitely be escaping user-provided shell arguments for everything, as forcing things to be alphanumeric is a large constraint, that isn't really necessary. I'll have a look at the Python implementation of shlex.quote() and friends and see what I can contribute.

Until there is proper shell escaping, passwords and usernames and anything else passed on the command line (I'm not familiar with Nodester's internals to know just what is) will have to be alphanumeric, as any symbol is potentially dangerous - eg password; rm -Rf --no-preserve-root /.

Could you list everywhere user input is used as shell args? I'll have a crack at this soon.

All of this said, it seems that the only place the user's password is used in plain text is when creating and changing the password. It's md5'd and put into Couch, and not used beyond that. Perhaps this pull request is still viable?

@contra

I meant for nodester-cli... we have to enforce no-spaces for passwords because passwords with spaces in them will break the way any CLI parses arguments. User input is never launched in processes, we aren't idiots.

@brianewing

I got the wrong end of the stick, then, apologies. Should this be a requirement of the API? The API handles any kind of password just fine, and the 'cheating' form on the site works perfectly. Creating an account with curl/etc works fine if you URL escape it, it's all good. The only place this is an issue is nodester-cli itself, heh.

Passwords with spaces should be wrapped in double quotes on your shell, or with an escaped space, and will then come into argv with the space, eg:
['node', 'test.js', 'create', 'brian', 'test password'].

I read nodester-cli's lib/commands.js and lib/user.js - it seems that this should work fine.
Isn't this a user issue of properly escaping the space?

@contra

I've never seen anyone allow spaces in passwords and that's the only standard we need to adhere to. I'll swap out the alphanumeric regex with a new one that enforces no spaces and also add the check in the API right now

@contra contra closed this Jan 18, 2012
@brianewing

Why enforce no spaces? There doesn't seem to be a reason to disallow spaces, and in fact I'm surprised when websites don't allow spaces. There is no technical reason why the API itself should disallow spaces, surely?

@contra

Fixed in b915b3e. We don't allow spaces because we don't want 50 issues open because people can't figure out why their password with spaces in it doesn't work on the CLI. We dealt with it before and a password policy cleaned it up.

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