Summary
Several code paths build SQL by string interpolation of values that callers control (keys, and table names). Keys containing a single quote make writes fail, and a key can change the meaning of the WHERE clause, so store[k] = ... / del store[k] can touch rows other than the one named by k (a crafted key passed to del removed every row in a local SQLite reproduction). This was flagged in the post-merge review on #4.
Where
Values and column names interpolated into text() (the main issue)
-
sqldol/base.py, SqlBaseKvStore._mk_column_filter, used by __setitem__ and __delitem__ (so by SqlDictStore too):
str keys: text(f"{key_column} = '{key}'")int keys: text(f"{key_column} = {key}")dict keys: " AND ".join(f"{col} = '{val}'" ...), where both the column names and the values come from the caller- any other key: zips the characters of the (single,
str) key column name with the key, which is not a meaningful filter at all
SqlBaseKvReader.__getitem__ is not affected: it already uses self.table.c[col] == key, a bound parameter. So reads and writes currently compare keys differently.
Table names interpolated into raw SQL (legacy sql_base.py)
-
SqlTableRowsCollection._tmpl_count_rows_tmpl, _tmpl_describe_tmpl, _tmpl_iter_tmpl (... FROM {table_name})
-
iter_rows: f"SELECT * FROM {table_name} LIMIT {batch_size} OFFSET {offset}"
-
SqlDbReader.__getitem__(k) feeds k straight into the above as the table name.
Under SQLAlchemy 2.x a SQLAlchemy Connection refuses a plain string, so these paths only execute with a raw DB-API connection (e.g. sqlite3); the interpolation is still unsafe there.
Not SQL injection, noted for completeness
SQLAlchemyPersister.table_columns: f"DESCRIBE {self.table}" interpolates the ORM class, not caller input; it cannot run under SQLAlchemy 2.x anyway.SqlDbCollection.from_config_dict formats a connection URI with str.format; a password with URI-special characters would need escaping, but this is not a query.rows_iter / TableRows take a caller-supplied filt clause as-is, by design.
Fix plan
- Build the key filter from SQLAlchemy column expressions (
self.table.c[col] == value), i.e. bound parameters, the same way __getitem__ does. Unknown column names in a dict key raise a clear KeyError instead of being spliced into SQL; an empty dict key raises ValueError.
- In the legacy raw-SQL paths, validate table names against an identifier allowlist (letters, digits,
_, $, optionally schema.table) with an informative ValueError, and coerce LIMIT/OFFSET to integers.
- Regression tests that insert, read back, overwrite and delete keys containing quotes, semicolons and
--, and check no other row is affected.
Summary
Several code paths build SQL by string interpolation of values that callers control (keys, and table names). Keys containing a single quote make writes fail, and a key can change the meaning of the
WHEREclause, sostore[k] = .../del store[k]can touch rows other than the one named byk(a crafted key passed todelremoved every row in a local SQLite reproduction). This was flagged in the post-merge review on #4.Where
Values and column names interpolated into
text()(the main issue)sqldol/base.py,SqlBaseKvStore._mk_column_filter, used by__setitem__and__delitem__(so bySqlDictStoretoo):strkeys:text(f"{key_column} = '{key}'")intkeys:text(f"{key_column} = {key}")dictkeys:" AND ".join(f"{col} = '{val}'" ...), where both the column names and the values come from the callerstr) key column name with the key, which is not a meaningful filter at allSqlBaseKvReader.__getitem__is not affected: it already usesself.table.c[col] == key, a bound parameter. So reads and writes currently compare keys differently.Table names interpolated into raw SQL (legacy
sql_base.py)SqlTableRowsCollection._tmpl_count_rows_tmpl,_tmpl_describe_tmpl,_tmpl_iter_tmpl(... FROM {table_name})iter_rows:f"SELECT * FROM {table_name} LIMIT {batch_size} OFFSET {offset}"SqlDbReader.__getitem__(k)feedskstraight into the above as the table name.Under SQLAlchemy 2.x a SQLAlchemy
Connectionrefuses a plain string, so these paths only execute with a raw DB-API connection (e.g.sqlite3); the interpolation is still unsafe there.Not SQL injection, noted for completeness
SQLAlchemyPersister.table_columns:f"DESCRIBE {self.table}"interpolates the ORM class, not caller input; it cannot run under SQLAlchemy 2.x anyway.SqlDbCollection.from_config_dictformats a connection URI withstr.format; a password with URI-special characters would need escaping, but this is not a query.rows_iter/TableRowstake a caller-suppliedfiltclause as-is, by design.Fix plan
self.table.c[col] == value), i.e. bound parameters, the same way__getitem__does. Unknown column names in adictkey raise a clearKeyErrorinstead of being spliced into SQL; an emptydictkey raisesValueError._,$, optionallyschema.table) with an informativeValueError, and coerceLIMIT/OFFSETto integers.--, and check no other row is affected.