3 ms·

Isn't using f-strings to build SQL queries a large footgun? I can imagine a junior developer introducing a SQL injection by not completely grasping the sqlbind

Isn't using f-strings to build SQL queries a large footgun? I can imagine a junior developer introducing a SQL injection by not completely grasping the sqlbind API. Using the quickstart example, visually both lines are very similar and both are functional, but one of them may introduce a SQL injection: sql = f'SELECT * FROM table WHERE field1 = {q/value1} AND field2 > {q/value2}' sql = f'SELECT * FROM table WHERE field1 = {value1} AND field2 > {value2}'

hombre_fatal 3y ago
I don’t use Python so can’t be bothered to read the source but the general solution here is to make regular string interpolation fail. e.g. You must use the Q mechanism and doesn’t work if you forget it.
- dougk 3y ago
  f-strings are part of the language so you need to change the interpreter to make that fail. That will not play nice with the rest of the codebase.
gpapilion 3y ago
Any string interpolation would open you up to sql injection without a lot of care. It generally recommended to used prepared statements. I guess the q part is the query parameter for the prepared statement. I’d still be a bit antsy here given the ease of a mistake. I just don’t get what it’s providing over using my db’s module. Usually the dialect is close but different enough to require changes to queries.
Retr0id 3y ago
Proposed safe(r) API via decorated methods: QParams = sqlbind.Dialect.some_dialect @QParams def make_my_query(value1: str, value2: int): return f'SELECT * FROM table WHERE field1 = {value1} AND field2 > {value2}' data = connection.execute(*make_my_query(foo, bar)) Obviously this wouldn't work as-is, QParams would need to be modified to support this. The decorator would wrap the method, sanitising the args before they're even passed to the wrapped function. Edit: actually, I might've misunderstood what sqlbind is doing internally, maybe this approach doesn't quite make sense Edit 2: Like so: https://github.com/baverman/sqlbind/issues/1 https://github.com/baverman/sqlbind/issues/1
- chrisjc 3y ago
  If I understand your code correctly, you are bypassing DBAPI's binding functionality and opening yourself up for SQL injection. QParams = sqlbind.Dialect.some_dialect @QParams def make_my_query(value1: str, value2: int): # SELECT * FROM table WHERE field1 = ? AND field2 > ? return f'SELECT * FROM table WHERE field1 = {value1} AND field2 > {value2}' data = connection.execute(*make_my_query(foo, bar), [foo, bar])
  - Retr0id 3y ago
    Where's the injection? (you might want to take a look at the example in the issue I opened, it's more complete - I'm not bypassing the DBAPI, I'm working with it)
    - chrisjc 3y ago
      Yup. Your code in github explains what you're suggesting better. My bad.
chrisjc 3y ago
It's a little difficult to parse through the README, but I believe placeholders and placeholder structure is being rendered in the SQL, not placeholder values. ps = f'SELECT * FROM table WHERE field1 = {q/value1} AND field2 > {q/value2}' # SELECT * FROM table WHERE field1 = ? AND field2 > ? However, I would imagine that if any external input is passed through to this framework, then there might still be the possibility of SQL injection attacks passing through this framework and ending up in the prepared statement SQL.
bvrmn 3y ago
It's my biggest concern as well. For real world application I tend to use out of line filters: filters = [q.field == value1, q.field2 > value2] sql = f'SELECT * FROM table {WHERE(*filters)}' It's a little harder to misuse.

hombre_fatal 3y ago

I don’t use Python so can’t be bothered to read the source but the general solution here is to make regular string interpolation fail. e.g. You must use the Q mechanism and doesn’t work if you forget it.

dougk 3y ago
f-strings are part of the language so you need to change the interpreter to make that fail. That will not play nice with the rest of the codebase.

gpapilion 3y ago

Any string interpolation would open you up to sql injection without a lot of care. It generally recommended to used prepared statements. I guess the q part is the query parameter for the prepared statement. I’d still be a bit antsy here given the ease of a mistake. I just don’t get what it’s providing over using my db’s module. Usually the dialect is close but different enough to require changes to queries.

Retr0id 3y ago

Proposed safe(r) API via decorated methods: QParams = sqlbind.Dialect.some_dialect @QParams def make_my_query(value1: str, value2: int): return f'SELECT * FROM table WHERE field1 = {value1} AND field2 > {value2}' data = connection.execute(*make_my_query(foo, bar)) Obviously this wouldn't work as-is, QParams would need to be modified to support this. The decorator would wrap the method, sanitising the args before they're even passed to the wrapped function. Edit: actually, I might've misunderstood what sqlbind is doing internally, maybe this approach doesn't quite make sense Edit 2: Like so: https://github.com/baverman/sqlbind/issues/1 https://github.com/baverman/sqlbind/issues/1

chrisjc 3y ago
If I understand your code correctly, you are bypassing DBAPI's binding functionality and opening yourself up for SQL injection. QParams = sqlbind.Dialect.some_dialect @QParams def make_my_query(value1: str, value2: int): # SELECT * FROM table WHERE field1 = ? AND field2 > ? return f'SELECT * FROM table WHERE field1 = {value1} AND field2 > {value2}' data = connection.execute(*make_my_query(foo, bar), [foo, bar])
- Retr0id 3y ago
  Where's the injection? (you might want to take a look at the example in the issue I opened, it's more complete - I'm not bypassing the DBAPI, I'm working with it)
  - chrisjc 3y ago
    Yup. Your code in github explains what you're suggesting better. My bad.

chrisjc 3y ago

It's a little difficult to parse through the README, but I believe placeholders and placeholder structure is being rendered in the SQL, not placeholder values. ps = f'SELECT * FROM table WHERE field1 = {q/value1} AND field2 > {q/value2}' # SELECT * FROM table WHERE field1 = ? AND field2 > ? However, I would imagine that if any external input is passed through to this framework, then there might still be the possibility of SQL injection attacks passing through this framework and ending up in the prepared statement SQL.

bvrmn 3y ago

It's my biggest concern as well. For real world application I tend to use out of line filters: filters = [q.field == value1, q.field2 > value2] sql = f'SELECT * FROM table {WHERE(*filters)}' It's a little harder to misuse.