Say who a new relation is readable by, instead of assuming - #137
Open
roed-math wants to merge 5 commits into
Open
Say who a new relation is readable by, instead of assuming#137roed-math wants to merge 5 commits into
roed-math wants to merge 5 commits into
Conversation
roed-math
force-pushed
the
security-grant-policy
branch
2 times, most recently
from
August 3, 2026 07:49
f841b14 to
8493aa7
Compare
roed-math
force-pushed
the
security-grant-policy
branch
2 times, most recently
from
August 3, 2026 08:16
e210ce0 to
a56d49b
Compare
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>
reset_connection() called _new_connection() with no arguments, so a replacement connection was built from the configuration alone. The overrides passed to PostgresDatabase(...) -- sslmode and the certificate paths, an alternate host, port, database or user, libpq options, connect and keepalive timeouts -- were kept in _connect_kwargs and used only for the first connection. A connection that dropped therefore came back without them: possibly without TLS, possibly to a different server. The webserver statement timeout was set once in __init__ and went with the session it was set on, and the read-only, superuser, knowls and userdb capability flags were never recomputed. Merge the options in one place, _connection_options(), used by every connection this database opens; move per-session setup into _configure_session(), applied to each new connection; and generalize the webserver timeout into session_settings, a small closed set of settings applied through set_config, which takes the name and value as bound parameters. A replacement is now adopted only after it is checked to have reached the same database as the same role -- the host is deliberately not checked, since a failover is a legitimate way for it to change -- and after the capability flags are recomputed from it. Both run directly on the new connection rather than through _execute, which is what is being recovered from. The retry rules are untouched: a standalone statement is retried once, and a statement inside DelayCommit still raises rather than replaying a transaction whose earlier statements died with the connection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A search table's name is used in three namespaces at once: as a PostgreSQL identifier, as the key its table object is reached by on the database, and as the stem of the files copy_to writes. Nothing checked it against any of them. A name containing a path separator or a ".." component sent an export out of the folder it was asked for, and because table objects were stored in the database object's instance dictionary, a table called conn, config or tablenames -- from create_table, or from a meta_tables row edited with plain SQL -- took the place of that attribute. Add validate_search_table_name and apply it in create_table, create_table_like (through create_table), rename_table and refresh_tables, so a name is checked whether it came from a caller or out of meta_tables. Move table objects into db.tables, reached through __getitem__ and a __getattr__ that only runs after ordinary attribute lookup has failed, so that what the database object really has always wins; tablenames becomes a sorted property of that mapping, which cannot drift from it. Route the filenames copy_to generates through safe_child_path, which resolves both paths -- so a symlink inside the folder does not lead out of it -- and refuses anything that is not underneath. The asserts in rename_table and in reload_all's meta-file check become explicit errors: they validate input, and disappear under python -O. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Creating a search table granted SELECT on it to lmfdb and webserver, and SELECT/INSERT on its counts and stats tables, wherever those roles happened to exist; the metadata tables did the same and a reload reapplied it. That is the LMFDB's deployment rather than a fact about a PostgreSQL database: a table created for anything at all came into existence readable by two roles the administrator may never have heard of, and a missing role was passed over with a warning, so a policy could appear to be applied when it had not been. Add psycodict.grants.GrantPolicy, which states the privileges by relation kind -- search, counts, stats, meta, meta_hist, meta_format, backup -- and pass one to PostgresDatabase. The default grants nothing. LMFDBGrantPolicy() reproduces what psycodict did before, for the deployments that want it, and LMFDB should pass it explicitly. A policy that names a role the cluster does not have raises unless it asks for missing_role="skip". Apply it through a single helper at every point a relation is created or swapped, including the _oldN table a reload leaves behind: PostgreSQL carries privileges along with a rename, so a backup holding a copy of the live data stayed exactly as readable as the table it used to be. The policy is authoritative for the roles it names -- the managed actions are revoked from them before granting -- so a relation ends up with the policy rather than the policy plus whatever it inherited. Roles a policy does not name are never touched. grant_select and its siblings keep working but require their users argument: a public method for granting to roles you name is useful, one that quietly grants to lmfdb and webserver is not. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
roed-math
force-pushed
the
security-grant-policy
branch
from
August 3, 2026 08:33
a56d49b to
0182800
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
2 participants
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.
Last of five PRs from the August 3 security audit.
Stacked on #133, #134, #135 and #136, so the diff here contains their
commits too; merge those first and this one shrinks to its own commit.
The problem
Creating a search table granted
SELECTon it tolmfdbandwebserver, andSELECT/INSERTon its counts and stats tables, wherever those roles happenedto exist. The metadata tables did the same, and a reload reapplied it. That is
the LMFDB's deployment, not a fact about a PostgreSQL database: a table created
for anything at all came into existence readable by two roles the administrator
may never have heard of. A missing role was passed over with a warning, so a
policy could appear to have been applied when it had not been. And because
PostgreSQL carries privileges along with a rename, the
_oldNtable a reloadleaves behind — a copy of the live data — stayed exactly as readable as the
table it used to be.
The fix
psycodict.grants.GrantPolicystates privileges by relation kind (search,counts,stats,meta,meta_hist,meta_format,backup), passed asPostgresDatabase(grant_policy=...).LMFDBGrantPolicy()reproduces the oldbehavior for deployments that want it.
half applied silently;
missing_role="skip"warns and carries on, which iswhat
LMFDBGrantPolicyuses since development databases rarely have thoseroles. The roles are checked once at connection time, so a mistaken policy is
refused before it can interrupt a bootstrap or a reload halfway through, and
the per-relation failure still happens inside the creation transaction, so
the relation is rolled back rather than left ungranted.
_apply_grant_policy(table_name, kind), is used at every point arelation is created or swapped: metadata bootstrap, the format stamp,
search/counts/stats creation,
create_table_like, the reload swap, andcreate_oldstats. There are no remaininggrant_selectcalls with hiddendefaults.
PostgreSQL gives a clone no privileges of its own, so under the default
policy the replacement is reachable only by its owner even when the table it
replaced was readable by an application role. That is what the policy asks
for, but it should not be discovered by a website going dark, so the swap
logs a warning naming the roles that lost access and pointing at
grant_policy._swap_in_tmpknows which relation is the search table and which are its companions; the
suffix reading it used to do (also what the old code did) would have granted
a counts table's INSERT to a search table ending in
_stats, which theLMFDB has.
revokes the four managed actions from those roles before granting, so a
relation ends up with the policy rather than the policy plus whatever it
inherited — which is what makes the backup case work. Roles a policy does not
name are never touched. Documented in DataManagement.md.
grant_select,grant_insert,grant_updateandgrant_deletekeepworking, but their
usersargument is now required (it was a mutable listdefault naming
lmfdb/webserver).Downstream
LMFDB needs one line to keep its current permissions —
in
lmfdb/lmfdb_database.py,LMFDBDatabase.__init__:I have not touched the LMFDB repository; say the word and I will open that PR
too. Without it, tables created by LMFDB after this lands get no grants —
existing tables are unaffected, since a policy is applied when psycodict creates
or swaps a relation, not on connection.
Tests
8 new cases in
tests/test_security.py: under the default policy a new searchtable, its
_countsand_stats, and the meta tables have no non-owner grantsat all; an explicit policy grants exactly what it names and nothing to a kind it
does not mention (using two disposable roles, skipped where the test user may
not create roles); the LMFDB policy's contents are what they were, including
nothing for backups; a missing role under the strict setting raises and leaves
no table, no
meta_tablesrow and no relation behind; the permissive settingwarns and creates; after a reload the live table has the policy's grants and the
_oldNbackup has none; a policy rejects unknown kinds, unknown actions, a barestring of roles, a bad
missing_role, and a role name that is not anidentifier; and the manual grant methods refuse to be called without roles.
Full suite: 1091 passed, 36 skipped against PostgreSQL 18. The wheel builds with
the new module included.
🤖 Generated with Claude Code