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

Change to all-parametrised queries #81

Open
6 of 9 tasks
grigi opened this issue Jan 7, 2019 · 5 comments
Open
6 of 9 tasks

Change to all-parametrised queries #81

grigi opened this issue Jan 7, 2019 · 5 comments
Labels
enhancement New feature or request Next Release Issues tagged for next Major release target security Urgent

Comments

@grigi
Copy link
Member

grigi commented Jan 7, 2019

Is your feature request related to a problem? Please describe.
Two actually:

  1. Security as in SQL excaping issues. Until we are 100% parametrised, we have limited defence against SQL injection attacks.
  2. We can't handle non-text representative fields for updates or filtering until we change over to parametrised, as those fields don't parse well as text.

Describe the solution you'd like
Update PyPika to allow parametrised queries.

Describe alternatives you've considered
There isn't really. Build our own is too much work for negative gain.

Additional context
We can't guarantee that parameters will be presented in the SQL query in the order we specify, so I feel the simplest solution might be to send the parameters as per usual, but then have a to_parametrised_query() method that returns (str, query parameter objects) so we can use parametrised queries, and PyPika is then in charge of managing order.

Then we need to update our code to use the parameters.

Done:

  • Add parameter support to PyPika
  • Use parameters for Inserts
  • Use parameters for Deletes
  • Use parameters for Updates
  • Add BinaryField
  • Have an escaping strategy for filters using LIKE
  • Use parameters for Queryset Updates
  • Use parameters for Queryset Filters
  • Use parameters for Related matching
@grigi grigi added enhancement New feature or request security labels Jan 7, 2019
@grigi
Copy link
Member Author

grigi commented Jan 7, 2019

Started this at kayak/pypika#201

@arlyon
Copy link
Contributor

arlyon commented Jan 7, 2019

Just ran into a problem with the GIS work that this should solve.

Unless I am mistaken this will have the side-effect of allowing us to use pypika function objects in queries as well since pypika should properly handle converting them to raw sql. Practically everything in GIS is done via SQL functions and currently if you pass them in to the sql driver values list instead of having pypika "render" it properly it will wrap it in quotes / escape it.

Although re-running a query with a mixture of functions and raw sql parameters may cause problems because of the INSERT_CACHE on tortoise's end

@grigi
Copy link
Member Author

grigi commented Jan 7, 2019

Hmm, I think this will help for any non-text object. Pypika has a Function class in utils.py that you could subclass for SQL functions?
I think as long as you pass a value parameter for inserts only, I think it would work. But yes, for that to work PyPika and Tortoise would need to update.

I also think #72 and 'capabilities' would probably be needed to make the GIS implementation not feel hacky.

@grigi
Copy link
Member Author

grigi commented Jan 16, 2019

PyPika 0.22 is released with Parameter() syntax. we should update to use it where we can.
I had a look at filtering/updating, and it seems to require quite the refactor to do it.

@grigi grigi added the Urgent label Jan 28, 2019
@grigi grigi mentioned this issue Feb 26, 2019
72 tasks
grigi added a commit that referenced this issue Sep 1, 2019
Progress on #81

This PR adds capability to allow parameterd queries, and then connects it up for Updates(.save()) and Deletes.
(Not queryset updates, that is a separate issue).

This is a step towards hardening Tortoise ORM. It also SIGNIFICANTLY improves performance for full Updates, partial Updates and Deletes.
@grigi grigi added the Next Release Issues tagged for next Major release target label Nov 11, 2019
@grigi
Copy link
Member Author

grigi commented Apr 19, 2020

v0.16.6 fixed some SQL injection issues for MySQL.
We should focus on parametrizing Queryset Updates as they are at least contained in the UpdateQuery.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Next Release Issues tagged for next Major release target security Urgent
Projects
None yet
Development

No branches or pull requests

2 participants