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

Doesn't Seem To Differentiate Post vs Get vs URL #3

Closed
remyguercio opened this issue Dec 7, 2011 · 10 comments
Closed

Doesn't Seem To Differentiate Post vs Get vs URL #3

remyguercio opened this issue Dec 7, 2011 · 10 comments

Comments

@remyguercio
Copy link

If I pass post and get parameters to an app.post and try and assert values it uses the get parameters.

curl -d 'password=1234567' http://localhost:3000/backend/check/?password=fjf
Password can't be shorter than 6 characters and it throws an exception.

Maybe use a similar syntax to req.param('password', req.body) with req.assert('password)?

@ctavan
Copy link
Member

ctavan commented Dec 7, 2011

I agree, if req.param() provided a way to specify from which source to extract the parameters then req.assert() should be able to handle that as well. However I can't really find the place where you got the syntax req.param('password', req.body) from. To my knowledge in express.js url-parameters take precedence over get-parameters which take precedence over post-parameters if one uses req.param(), see https://github.com/visionmedia/express/blob/master/lib/request.js#L244-258

If you do not expect any get-parameters you could use

req.query = null;

as a workaround to unset all query-parameters before doing the asserts.

So currently req.assert() somehow just behaves like the express.js-provided req.param().

Any thoughts?

@remyguercio
Copy link
Author

Yeah sorry my bad, misunderstood what I was looking at. (I guess I was trying to be optimistic.)
I've been using something similar to req.query.email = req.params.email = req.body.email; as a workaround, but it seems that req.assert() could work in a way where you pass it req.body, req.params, req.query or post, get, route and it uses that.

To me req.param() seems like a rather large security hole in it's current form for unsuspecting programmers. Ask for variables via post have them given to req.param via get. But that is just me.

@ctavan
Copy link
Member

ctavan commented Dec 7, 2011

Yeah, I admit req.param() may be a little confusing, but as express-validator is a middleware for express.js I want to stick with the way, express.js behaves.

You can still use plain node-validator to validate just the bits that you want so you don't get confused by strange express.js magic ;)

Maybe you might consider filing an issue for express.js that addresses your concerns?

@ctavan
Copy link
Member

ctavan commented Dec 7, 2011

Another idea would be to add something like

req.query.assert('email')
req.body.assert('email')
etc.

But I'm not sure how useful that would be if the application later still can use req.param('email')....

@remyguercio
Copy link
Author

Yeah I may go to just regular node-validator and sadly drop this, but I do understand sticking with the way expressjs handles things.

Posted here expressjs/express#622

@ctavan
Copy link
Member

ctavan commented Dec 7, 2011

Cool, thanks for the upstream issue. Closing this one.

@ctavan ctavan closed this as completed Dec 7, 2011
@ctavan ctavan reopened this Dec 7, 2011
@ctavan
Copy link
Member

ctavan commented Dec 7, 2011

Hmm, after I've read the issue I reopen the issue here. I'll think about some way to handle that stuff nicer in express-validator.

@remyguercio
Copy link
Author

The only thing I can think of is having it where you either pass the req.body etc. or just saying post, route, or get as a second parameter. If that is the case I can just req.body.example afterwards to get my sanitized or asserted variable. Personally my favorite thing is the easy error handling with assertions, because with my way I could basically just use node-validator I just don't get the nice assertions.

@ctavan
Copy link
Member

ctavan commented Jun 6, 2013

#32 provides a special method checkBody for checking the body only, similar methods could be added for checkQuery and checkParams. If anyone has the need for these feel free to open a pull request for it.

@ctavan ctavan closed this as completed Jun 6, 2013
@lock
Copy link

lock bot commented Jun 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants