Skip to content

Validate index and constraint definitions at import and at use - #134

Open
roed-math wants to merge 2 commits into
roed314:mainfrom
roed-math:security-metadata-validation
Open

Validate index and constraint definitions at import and at use#134
roed-math wants to merge 2 commits into
roed314:mainfrom
roed-math:security-metadata-validation

Conversation

@roed-math

@roed-math roed-math commented Aug 3, 2026

Copy link
Copy Markdown

Second of five PRs from the August 3 security audit.

Stacked on #133. The branch is cut from that one, so the diff here contains
its commit too; review #133 first and merge it first, and this diff shrinks to
its own single commit. (GitHub will not take a base branch that lives in the
fork, hence the main base.)

The problem

meta_indexes and meta_constraints record how to rebuild an index or
constraint. create_index and create_constraint validate their arguments, but
nothing revalidated a row on the way back out, and _create_index_statement /
_create_constraint_statement formatted the stored access method, column
modifiers, storage-parameter names, check function and partial-index predicate
into the statement as text.

Between creation and use those rows can be edited with plain SQL, exported to a
file and imported into another database, or restored from the _hist tables. A
predicate is appended to CREATE INDEX ... WHERE, where a semicolon ends the
statement — so a poisoned whereclause runs whatever follows it. I confirmed
this on the unpatched tree against a disposable database: restore_index on a
row whose predicate had a statement appended returned normally, and the appended
statement had run.

The fix

  • validate_index_definition and validate_constraint_definition in
    psycodict.base, returning normalized IndexDefinition / ConstraintDefinition
    tuples, applied at both ends:
    • at import, in _reload_meta and _revert_meta, inside the transaction that
      loaded the rows, so a rejected file or history version rolls back and leaves
      the previous metadata untouched;
    • at use, in _create_index_statement and _create_constraint_statement,
      which is what protects against a row edited in place, a row written by an
      older psycodict, or a future import path that forgets to check.
  • Column existence is checked only at use time, against the relation actually
    being built — a reload legitimately imports an index for a column the table is
    about to gain, so checking that at import time would break it.
  • Raw interpolation is gone from both statement builders, so validation is no
    longer the only defense: access methods, operator classes, storage-parameter
    names and CHECK functions are quoted identifiers, and ASC, DESC,
    NULLS FIRST, NULLS LAST are fixed SQL constants selected by a validated
    key. Storage-parameter values keep going through Literal, and are now
    range- and type-checked.
  • Partial-index predicates stay raw administrative SQL — psycodict does not
    parse them — but must remain predicates: no semicolon, comment, dollar-quoted
    string, control character, or length over 4096.
  • _operator_classes and _valid_storage_params move to base.py next to the
    validators and are re-exported from table.py for anything importing them.
  • _copy_from_meta is removed: an import path into meta_* with no validation
    and, as far as I can find, no callers in psycodict or LMFDB. Say the word if
    you would rather keep it and have it validate instead.
  • The assert in _meta_cols_types_jsonb_idx becomes a ValueError.

Compatibility

  • A CHECK constraint's function must now be in
    PostgresTable._valid_check_functions to be rebuilt, not only to be
    created. A database with CHECK constraints whose functions are not registered
    there will raise on restore, with a message saying so.
  • Constraint names are now validated on the same footing as index names, and
    create_constraint additionally refuses a name over PostgreSQL's 63-byte
    limit: it never truncated its generated names, so an over-long one used to be
    silently truncated by PostgreSQL and then fail to match its
    meta_constraints row. The length is checked only when a name is being
    created — psycodict builds the names it puts in DDL by appending _tmp or
    _oldN, and holding those to 63 bytes would fail every reload of a table
    whose index name is near the limit (create_index generates names right up
    to it).
  • Column names in a definition are checked for being usable strings, not for
    being identifiers: a column that exists is a column whatever it is called
    (the LMFDB has one named 2adic_index), and it is quoted wherever it is
    used. A column name carrying SQL is refused when the DDL is built, because it
    is not a column of the table.
  • The collation grammar accepts collation names psycodict has no business
    vetoing — C.UTF-8, ICU names like und-x-icu, de_DE@euro — and collated
    arrays. What it still refuses is a name that could end the quoted string
    early.
  • meta_indexes still stores modifiers exactly as the caller spelled them; the
    canonical spellings are what the DDL is built from.

Tests

34 new cases in tests/test_security.py:

  • a metadata file whose predicate carries a marker-table statement is rejected
    by reload_indexes, and afterwards the marker does not exist, the
    meta_indexes rows are exactly what they were, and the built index is the one
    that was there before;
  • eleven other invalid definitions through the same path (unknown access method,
    modifier carrying SQL, modifier valid only for another method, mismatched
    modifier/column lengths, unknown storage parameter, out-of-range and
    wrong-typed storage values, malformed and empty column lists, a column name
    carrying SQL);
  • rows poisoned directly in meta_* with SQL, then restore_index /
    restore_constraint, including an unapproved CHECK function;
  • poisoned meta_indexes_hist followed by revert_indexes;
  • positive coverage: every supported index shape still builds, a partial index
    still survives export → reload_indexesrestore_index, and predicates
    that are merely unusual (multi-line, LIKE 'a%', nested parentheses) are
    still accepted.

Full suite: 1006 passed, 36 skipped against PostgreSQL 18.


Review round (2026-08-03)

  • Identifier validation no longer imposes an ASCII naming convention. Every
    name is quoted, so x-y, index with spaces, idx_é and a name containing a
    semicolon all round-trip through create → drop → restore; what is refused is
    an empty name, a control character (a psycodict policy, so names stay
    printable in its logs and exports) and a name over PostgreSQL's 63-byte
    limit. The test that expected an SQL-looking index name to be rejected is
    replaced by one asserting it is quoted as a single identifier, executes
    nothing, and matches its catalog name exactly.
  • Derived names go through one byte-aware helper. derived_identifier(base, suffix) returns base + suffix when it fits — always, for existing names —
    and otherwise cuts the base on a UTF-8 character boundary and inserts a short
    SHA-256 digest before the suffix. Every _tmp / _oldN / _depN / _pkey
    path uses it, so no two paths can disagree about a derived name and none
    relies on server-side truncation. Generated index and constraint names are
    fitted (with the disambiguator's bytes reserved before the cut); explicit
    names are refused rather than silently changed.
  • Qualified CHECK functions are structural. schema.function is parsed into
    components and emitted as "schema"."function", never as one identifier.
  • gin_pending_list_limit has no client-side ceiling (its maximum derives
    from MAX_KILOBYTES and is architecture-dependent); only the documented lower
    bound is enforced, and the value still goes out as a Literal.
  • Boolean storage parameters are normalized. on, yes, t, 1 and
    PostgreSQL's other unambiguous spellings all become a Python bool, which is
    what both the DDL and meta_indexes get; floats are refused by type, since
    0.0 == False in Python.
  • New: a full approved-CHECK round trip — a schema-qualified immutable
    function, the constraint created and shown to be enforced, dropped and
    restored, exported and reloaded, and restored onto a _tmp table.

Full suite: 1109 passed, 36 skipped. Docs build clean under -W.

🤖 Generated with Claude Code

@read-the-docs-community

read-the-docs-community Bot commented Aug 3, 2026

Copy link
Copy Markdown

@roed-math
roed-math force-pushed the security-metadata-validation branch from 0bc0585 to 8bd2b5a Compare August 3, 2026 08:09
@roed-math

Copy link
Copy Markdown
Author

The LMFDB test suite check on this PR failed on a devmirror outage, not on the change:

psycopg.OperationalError: connection failed: connection to server at "35.225.45.113",
port 5432 failed: FATAL:  the database system is in recovery mode

The same commit is the base of #135, #136 and #137, whose Downstream runs started seven
minutes later and all passed. A re-run should be green (I do not have rights to trigger
one here).

Lint is red for the unrelated ruff-default reason described at the top; #138 fixes it.

@roed-math
roed-math force-pushed the security-metadata-validation branch from 8bd2b5a to a43947b Compare August 3, 2026 08:33
roed314 and others added 2 commits August 3, 2026 13:34
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>
meta_indexes and meta_constraints hold what an index or constraint is
rebuilt from, sometimes long after it was created.  Between the two the
rows can be edited with plain SQL, exported to a file, carried to another
database and imported, or restored from the _hist tables, so
"this row must once have passed through create_index" is not something a
statement builder can rely on.  It was relied on: the access method,
column modifiers, storage-parameter names, check function and
partial-index predicate were formatted into the statement as text, and a
predicate is appended to CREATE INDEX, where a semicolon ends it.

Add validate_index_definition and validate_constraint_definition, and
apply them in both places -- when rows are imported by _reload_meta and
_revert_meta, inside the transaction so a rejected file leaves the old
metadata intact, and again in _create_index_statement and
_create_constraint_statement when the definition becomes DDL.  Column
existence is checked only at the second point, against the relation being
built, since a reload legitimately imports an index for a column the
table is about to gain.

Remove the interpolation the validators were the only defense for: access
methods, operator classes, storage-parameter names and check functions are
now quoted identifiers, and ASC/DESC/NULLS FIRST/NULLS LAST are fixed SQL
constants selected by a validated key.  Predicates stay raw SQL, but must
remain predicates: no semicolons, comments, dollar quotes, control
characters or unreasonable lengths.

The index vocabulary (_operator_classes, _valid_storage_params) moves to
base.py next to the validators, and is re-exported from table.py.
_copy_from_meta, an import path into meta_* with no callers and no
validation, is removed, and the assert guarding _meta_cols_types_jsonb_idx
becomes a ValueError.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@roed-math
roed-math force-pushed the security-metadata-validation branch from a43947b to 04be6d8 Compare August 3, 2026 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants