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

Support for known array parameters #1

Closed
avaly opened this issue May 18, 2015 · 4 comments
Closed

Support for known array parameters #1

avaly opened this issue May 18, 2015 · 4 comments

Comments

@avaly
Copy link

avaly commented May 18, 2015

This module assumes that any query parameter is not supposed to be an array. However some applications might actually require to use an array parameter. This type of parameter is invalidated by this module while cleaning other potentially invalid parameters.

Example:

GET /?query=foo&query=bar&filters[]=123&filters[]=456

I would like a way to notify the plugin to clean only query or to exclude filters from the cleaning.

@analog-nico
Copy link
Owner

Hi @avaly good point! I was also thinking about a parameter whitelist. However, I am not sure how the whitelist should look like. Of course, it would contain the names of the parameters HPP will not touch. But that might not be enough: Probably for some routes a specific parameter should be whitelisted but for other routes the parameter with the same name should not be whitelisted.

For you example above the following would make sense:

app.use(hpp({
    whitelist: [ 'filters' ]
}));

For configuring different routes differently we could extend the whitelist data structure:

app.use(hpp({
    whitelist: [
        { routes: [ '/search' ], parameters: [ 'filters' ] },
        { routes: [ '/find' ], parameters: [ 'categories' ] }
    ]
}));

Or instead we could use native filtering:

app.use(hpp()); // To secure all other routes.
app.use('/search', hpp({ whitelist: [ 'filters' ] }));
app.use('/find', hpp({ whitelist: [ 'categories' ] }));

I prefer the last solution. However, maybe the routes are not the only distinguishing aspects:

  1. Should parsing the req.query and req.body have different whitelists?
  2. Should a different whitelist apply e.g. if the client sends certain headers?

Thoughts?

@avaly
Copy link
Author

avaly commented May 18, 2015

I agree that the whitelist should be route-specific. I prefer your last example there with the native route filtering.

  1. For a big enough application the whitelist could also be query/body-specific as well.
  2. I don't see the use case for such a feature.

@analog-nico
Copy link
Owner

Thanks, makes sense.

I just realized the distinction between query and body could be made like this:

app.use('/search', { whitelist: [ 'filters' ], checkBody: false });
app.use('/search', { checkQuery: false });

This way filters is only whitelisted when passed to req.query. This maybe isn't the nicest solution but it is enough until the first user actually makes that distinction very often and hopefully suggests something more elegant in a new issue.

OK, I will implement the whitelist as discussed shortly.

@analog-nico
Copy link
Owner

I just published version 0.1.2 to npm.

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