search

cvburgess / SQLDataSource

SQL DataSource for Apollo GraphQL projects
MIT License
236 stars 39 forks source link

Knex QueryBuilder toString is unreliable for producing cache keys for PG. #86

Open willrust opened 1 year ago

willrust commented 1 year ago

I believe this is related to knex issue 3553, but wanted to surface it here because of a potential security issue that could result.

For PG, query.toString() will omit bigint values, yet the actual query can succeed.

Here's an example of the resulting string where a bigint id is used: select "id", "username" from "users" where "id" = '' limit 1 Notice that id = ''. So in this case, the first user queried will be returned for subsequent queries as the cache key is always the same.

Also, the Knex docs suggest that toString() should be used for debugging, but we're using it for something quite important.

cvburgess commented 1 year ago

Great callout @willrust - when I created this utility for my team we used UUIDs for IDs across our app, never integers of any kind.

If you can suggest a better path forward I would gladly welcome a PR.

Personally, for security, I implement RLS so the only records selected belong to the user currently authenticated. I don't have a great idea for this particular scenario.