Skip to content
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

Security issue: Missing input validation #3

Closed
yonjah opened this issue Jul 2, 2018 · 6 comments
Closed

Security issue: Missing input validation #3

yonjah opened this issue Jul 2, 2018 · 6 comments

Comments

@yonjah
Copy link

yonjah commented Jul 2, 2018

Your still missing input validation on register route -
https://github.com/ylorenzana/node-express-api-auth/blob/master/server/routes/users.js#L12

@ylorenzana
Copy link
Owner

ylorenzana commented Jul 2, 2018

This is on purpose. I believe the validation should be done client side for registration, avoiding unnecessary requests to the server when the input is invalid. The User schema enforces the email and password values to be strings, so it is safe from noSQL injection as it will not accept objects.

Closing this issue unless there are other reasons to validate the input server side.

@yonjah
Copy link
Author

yonjah commented Jul 3, 2018

Client side validation has nothing to do with security. As you mentioned it's only role is better UX.
You need to have both as they work together. Server side validation for security, and client side validation for UX.
Their is also some validations that are hard to do on the client, but they are a bit too advance to talk about at the current state of the implementation.

I think you still don't understand how injections work.
You defined a schema for the session token to be a String as well it didn't stop any injections. It's just not the right level for this checks. If you don't do it yourself you can't be sure it will ever be safe.

It will be much better if you took the time to really learn those issue in depth but until then just put it as a general rule to always validate user input on the server

@ylorenzana
Copy link
Owner

The difference for the token schema is it wasn't using the input for the save() function, so the fact that it was enforcing a String type didn't matter. In this case, the input for the registration is only being used for creating a new User and for saving it, so if the input is not a string, mongoose will throw an error saying it received a value of the wrong type. Isn't that validation? This ties in with the other issue you opened regarding the checking the input for the email is an actual email, which I agree with you it's something that must be added and I'll be working on that shortly.

@yonjah
Copy link
Author

yonjah commented Jul 3, 2018

The difference is that in this particular case mongoose might be able to detect something is not right.
But it does not mean it is fool proof and it does not mean it will work for every possible injection.

It also doesn't mean that future changes to both your code and mongoose wont introduce security issues because you've been too lazy to validate the input yourself.

You know this value came from untrusted source. You need to make sure it is valid before you pass it on.

It is so simple to do there is no reason to be lazy here.

@ylorenzana
Copy link
Owner

All right I'll add another check before saving the user. I am working on the CSRF token implementation right now, I hope you're willing to give it a look once I push the commits.

P.S. wasn't being lazy, just thought the check would be redundant.

@yonjah
Copy link
Author

yonjah commented Jul 3, 2018

Sure ping me once it's done and I'll have a look.

The main issue with CSRF is that you need to have some client logic to get the token and send it with every POST request so you might want to add an example page to show how it supposed to be used on the client side

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

No branches or pull requests

2 participants