search

balderdashy / waterline-sequel

A SQL generator for use in Waterline Adapters
MIT License
16 stars 61 forks source link

Injection safe #101

Closed Enteee closed 7 years ago

Enteee commented 7 years ago

Question

Who has to ensure that arguments for various aggregation functions such as .groupBy(), .min(), .max(), ... are safe against injections?

Context

I'm building currently a sails-hook which exposes REST routes for aggregations (repo). For example: /User/aggregate?groupBy=name should yield a list of user names. Therefore I'm using something like: User().find().groupBy(req.param('name')). Doing so is extremely dangerous because of this line. As the whole function processAggregates seems to be prone to injections I'm confused if this is a bug in waterline-sequel or if I have to ensure the correct use of the said functions.

sailsbot commented 7 years ago

@Enteee Thanks for posting, we'll take a look as soon as possible. In the meantime, if you haven’t already, please carefully read the issue contribution guidelines and double-check for any missing information above. In particular, please ensure that this issue is about a stability or performance bug with a documented feature; and make sure you’ve included detailed instructions on how to reproduce the bug from a clean install. Finally, don’t forget to include the version of Node.js you tested with, as well as your version of Sails or Waterline, and of any relevant standalone adapters/generators/hooks.

Thank you!

sailsbot commented 7 years ago

@Enteee,@sailsbot: Hello, I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message and simply close the issue if possible. On the other hand, if you are still waiting on a patch, please post a comment to keep the thread alive (with any new information you can provide).

If no further activity occurs on this thread within the next 3 days, the issue will automatically be closed.

Thanks so much for your help!

Enteee commented 7 years ago

question is still relevant.

sailsbot commented 7 years ago

@Enteee,@sailsbot: Hello, I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message and simply close the issue if possible. On the other hand, if you are still waiting on a patch, please post a comment to keep the thread alive (with any new information you can provide).

If no further activity occurs on this thread within the next 3 days, the issue will automatically be closed.

Thanks so much for your help!

wulfsolter commented 7 years ago

Ssshhh now @sailsbot

sailsbot commented 7 years ago

@Enteee,@sailsbot,@wulfsolter: Hello, I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message and simply close the issue if possible. On the other hand, if you are still waiting on a patch, please post a comment to keep the thread alive (with any new information you can provide).

If no further activity occurs on this thread within the next 3 days, the issue will automatically be closed.

Thanks so much for your help!

Enteee commented 7 years ago

yeah, automatically closing security related questions does not seem to be such a wise idea...

wulfsolter commented 7 years ago

@Enteee sails-postgresql uses pg which uses prepared statements. I don't use any other DBs, so YMMV, but Postgres is safe.

Enteee commented 7 years ago

@wulfsolter thank you for getting back to me. But I think your statement does not hold in my case, see links provided in the description.