Bind SQL keys as parameters; allowlist raw-SQL table names (fixes injection)

[thorwhalen]

Closes #6. Follows up item 2 of the post-merge review on #4.

What was wrong

SqlBaseKvStore._mk_column_filter (used by __setitem__ / __delitem__, so by SqlDictStore) formatted keys, and for mapping keys also column names, into text(). A key containing ' made writes fail, and a key could widen the WHERE clause: in a local SQLite reproduction, deleting a crafted, absent key removed every row. The legacy raw-SQL paths in sql_base.py wrote table names into SQL text unchecked.

Change

• Keys are bound parameters. The filter is table.c[key_column] == key, or for a mapping key and_(table.c[col] == val, ...). That is how __getitem__ already matched keys, so reads and writes now compare keys the same way. Column names go through table.c, so only real columns can be named (SQLAlchemy quotes them); an unknown column raises a ValueError that lists the table's columns (not KeyError, so a typo isn't read as "key absent"). A name matching exactly one column case-insensitively still resolves, as unquoted SQL names did.
• Key edge cases, now explicit. An empty mapping key used to produce an empty WHERE and delete every row; it now raises ValueError. Tuple/list keys (which used to zip the characters of the column name into garbage SQL) raise a clear TypeError. None, float, UUID, datetime keys, which raised TypeError before, now compare like __getitem__ does (None → IS NULL).
• Legacy raw-SQL table names are allowlisted. SqlTableRowsCollection and iter_rows validate table names with validate_sql_identifier (Unicode letters, digits, _, $, not all digits, optional schema.table; informative ValueError otherwise). Table names can't be bound, and these paths may run on a plain DB-API connection with no quoting helper, which is why this uses an allowlist and not quoting. LIMIT/OFFSET values are coerced with operator.index.
• Unused text import dropped from base.py.

Tests

sqldol/tests/test_sql_injection.py: for 9 keys holding quotes, semicolons and --, insert, read back, overwrite, delete present, delete absent, and use as mapping-key values, each time checking the full table contents, so no other row may change. Plus normal str/int/dict keys, a test shaped like the known dependent's integer mapping-key delete, identifier allow/deny cases, and the legacy paths on a raw sqlite3 connection.

On the old code most of the key-path tests fail (the reviewer counted 38). With the fix, the whole suite passes locally (102 passed, doctests included, py3.10).

Review

An independent refute-review agent found no blockers. I applied its should-fix items in the second commit: ValueError for an unknown column, case-insensitive column resolution, a looser allowlist for MySQL-style digit-leading and Unicode names, two vacuous test assertions removed, and a TypeError for sequence keys. Not changed: __getitem__ still treats a mapping key as a key-column value while writes treat it as column conditions. That was already the case before this PR.

Dependents

fleet_dependents lists one dependent, raglab_app. Its call sites use str keys (token, name, guid), int keys (id, app_id) and __delitem__({"app_id": int, "user_id": int}) on Postgres. Every column it names exists in the reflected table, and each key type binds to the same comparison the old literal SQL made. The new test test_mapping_key_of_ints_on_integer_columns mirrors the mapping-key call. raglab_app's own tests don't exercise sqldol (its paths need a live Postgres).

Not changed (out of scope, noted in #6)

• SQLAlchemyPersister.table_columns (DESCRIBE {self.table}) formats the ORM class, not caller input, and can't execute under SQLAlchemy 2.x.
• SqlDbCollection.from_config_dict formats a connection URI, not a query.
• Pre-existing and unrelated: on a raw sqlite3 connection, SqlTableRowsCollection.count_rows calls .first() (a SQLAlchemy result method), and iter_rows never detects the end of the table because a SELECT's rowcount is -1.

🤖 Generated with Claude Code