Validate column types against the whole string, not a prefix - #133
Open
roed-math wants to merge 1 commit into
Open
Validate column types against the whole string, not a prefix#133roed-math wants to merge 1 commit into
roed-math wants to merge 1 commit into
Conversation
This was referenced Aug 3, 2026
A column type cannot be bound as a value -- PostgreSQL has no placeholder for a type -- so create_table, add_column and header-driven table creation interpolate it into DDL as SQL text. The check guarding that interpolation used regexp.match() against a set of unanchored patterns, so any string beginning with a valid type passed validation with the rest still attached, and psycopg runs a parameterless statement with the simple query protocol, which executes every statement in it. Centralize the check in validate_column_type(), which matches the complete string with fullmatch(), restricts types to an ASCII character set that excludes NUL, control characters and lookalikes, and returns the spelling callers must emit. Every type-bearing DDL path now emits that returned spelling: _order_columns, _create_table, _create_table_from_header (and so reload/reload_all with adjust_schema=True), add_column, and the temporary table built when resorting ids. The character-type pattern was also malformed -- a bracket typo in the varchar length meant varchar(N) was only ever accepted as a prefix of varchar -- so the char family is now described deliberately, with an optional length and an optional collation. Collation names stay case-sensitive, since they are quoted identifiers: "C" is a collation and "c" is not. add_column validates before it mutates col_type, so a rejected type leaves the table object alone. InvalidColumnTypeError subclasses both ValueError and RuntimeError, the latter being what an invalid type raised before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
roed-math
force-pushed
the
security-datatype-validation
branch
from
August 3, 2026 08:33
293d208 to
66ca07b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First of five PRs from the August 3 security audit. It is the one that fixes a
concrete injection, so it is worth landing before the others.
The problem
A column type has to be interpolated into
CREATE TABLE/ALTER TABLEas SQLtext: PostgreSQL has no placeholder for a type. The check guarding that
interpolation used
regexp.match()against patterns that were not anchored atthe end, so any string beginning with a valid type validated with the rest
still attached, and
_order_columns()then interpolated the caller's originalstring. psycopg sends a parameterless statement with the simple query protocol,
which executes every statement in the string.
I confirmed this end to end against a disposable database on the unpatched tree:
db.create_table(name, [("c", <type with a second statement appended>)])returned normally and the appended statement had run.
The entry points that take a type from outside psycodict are
create_table,add_column, and the column types read from a data file header byreload(adjust_schema=True)andreload_all(adjust_schema=True). The last twomatter most in practice, since those types come from a file rather than from a
call site the administrator just typed. (
create_table_liketakes its typesfrom the source table's catalog entry, so it was not a way in; it goes through
the same validator now regardless.)
The character-type pattern was separately malformed — a bracket typo in the
varchar length (
\(1-9][0-9]*\)) meantvarchar(16)never matched it and wasonly ever accepted as a prefix of
varchar.The fix
psycodict.base.validate_column_type(typ),returning
(sql_spelling, storage_cost). It requires a string, stripssurrounding whitespace, rejects anything outside an ASCII character set that
excludes NUL, control characters and non-ASCII lookalikes, and matches with
fullmatch().argument:
_order_columns,_create_table,_create_table_from_header,add_column, and the temp table built when resorting ids._get_type_sortkeyand
_check_col_datatypeare now thin wrappers over the same validatorinstead of a parallel matching loop.
char,character,varchar,character varying, each with an optional length, and an optional collation)instead of by a typo'd pattern. Keywords match case-insensitively; collation
names do not, since they are quoted identifiers —
"C"is a collation and"c"is not, so the spelling that goes into the DDL is the spelling that wasvalidated.
SQLobjects,so the common case interpolates a constant rather than a caller-supplied
string.
add_columnvalidates before it mutatescol_type, and updates the in-memoryschema only after the DDL succeeds.
InvalidColumnTypeError. It subclassesValueError(thenatural type for a bad argument) and also
RuntimeError, which is what aninvalid type raised before, so existing
except RuntimeErrorcallers keepworking. Two existing tests assert
RuntimeErrorand are unchanged.Tests
New
tests/test_security.py(106 cases):grammar cannot drift away from PostgreSQL's and start rejecting a column an
existing database already has;
varchar(0),varchar(-1), a NUL byte, a Cyrillic lookalike — is rejected;create_table(both a column type and theid_type),add_column,reload(adjust_schema=True)andreload_all(adjust_schema=True)are eachdriven with a type carrying a statement that would create a uniquely named
marker table. Each test asserts the exception and that no marker exists,
that the target relation or column was not partially created, and that the
table object's
col_type/search_colswere not mutated.Full suite: 972 passed, 36 skipped against PostgreSQL 18.
Because the repository is at 1.0.0rc1, a patched release candidate before 1.0.0
final would be the natural home for this; I left the version alone.
🤖 Generated with Claude Code