From 66ca07b5dd7ce0d28e8f256efce8e65a03d76304 Mon Sep 17 00:00:00 2001 From: David Roe Date: Mon, 3 Aug 2026 02:56:46 -0400 Subject: [PATCH 1/5] Validate column types against the whole string, not a prefix 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 --- CHANGELOG.md | 18 +++ psycodict/base.py | 165 ++++++++++++++++--- psycodict/database.py | 8 + psycodict/table.py | 20 +-- tests/test_security.py | 354 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 534 insertions(+), 31 deletions(-) create mode 100644 tests/test_security.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b6bff4..56bd2fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -161,6 +161,24 @@ hardening standalone use; the highlights: - `postgresql_dbname` was the one option whose default ignored the `defaults` dictionary passed to `Configuration`. (#119) +### Security + +- **Column types are validated against the whole string, not a prefix.** The + type of a column has to be interpolated into `CREATE TABLE` and `ALTER TABLE` + as SQL text, since PostgreSQL has no placeholder for a type. The check that + guarded that interpolation matched a prefix, so a type beginning with a valid + type — `text; ...`, `varchar(16); ...` — was accepted with the rest attached + and executed. `create_table`, `create_table_like`, `add_column` and the + column types read from the header of a reloaded data file + (`reload(adjust_schema=True)`, `reload_all(adjust_schema=True)`) were all + affected. Types are now validated by `psycodict.base.validate_column_type`, + which matches the complete string and returns the spelling that callers must + emit; the character-type grammar it replaces was also malformed, so + `varchar(N)`, `char(N)` and `character varying(N)` are now described + deliberately rather than accepted as prefixes of `varchar`. An invalid type + raises `InvalidColumnTypeError`, a subclass of both `ValueError` and the + `RuntimeError` raised before. + ### Infrastructure - A test suite of 500+ tests and a continuous-integration workflow covering the diff --git a/psycodict/base.py b/psycodict/base.py index a7ade0e..1806dc1 100644 --- a/psycodict/base.py +++ b/psycodict/base.py @@ -106,17 +106,136 @@ types_whitelist[elt + "[]"] = -1 +# Types that carry a length, a precision or a collation cannot be listed +# exhaustively, so they are described by a grammar. Every pattern below is +# matched with fullmatch(): a type is accepted only if the *whole* string is a +# type, so a valid prefix such as "text" cannot carry a suffix of arbitrary SQL +# into the DDL these types are interpolated into. +# +# Keywords are matched case-insensitively through scoped (?i:...) groups rather +# than by lowercasing the input, because a collation name is a quoted +# identifier and therefore case-sensitive: "C" is a collation, "c" is not. + +# Collations psycodict accepts: the two collations every PostgreSQL server has, +# the two SQL-standard names, and locale names of the usual ll_CC[.charset] +# shape. +_COLLATION_NAME = ( + r'(?:C|POSIX|default|ucs_basic' + r'|[A-Za-z]{2}(?:_[A-Za-z]{2,3})?(?:\.[A-Za-z0-9-]+)?(?:@[A-Za-z0-9]+)?)' +) +_COLLATE = r'(?:\s+(?i:collate)\s+"' + _COLLATION_NAME + r'")?' + param_types_whitelist = { - r"^(bit( varying)?|varbit)\s*\([1-9][0-9]*\)$": -1, - r'(text|(char(acter)?|character varying|varchar(\s*\(1-9][0-9]*\))?))(\s+collate "(c|posix|[a-z][a-z]_[a-z][a-z](\.[a-z0-9-]+)?)")?': -1, - r"^interval(\s+year|month|day|hour|minute|second|year to month|day to hour|day to minute|day to second|hour to minute|hour to second|minute to second)?(\s*\([0-6]\))?$": 16, - r"^timestamp\s*\([0-6]\)(\s+with(out)? time zone)?$": 8, - r"^time\s*\(([0-9]|10)\)(\s+without time zone)?$": 8, - r"^time\s*\(([0-9]|10)\)\s+with time zone$": 12, - r"^(numeric|decimal)\s*\([1-9][0-9]*(,\s*(0|[1-9][0-9]*))?\)$": -1, + # text, optionally collated + r"(?i:text)" + _COLLATE: -1, + # the char family: an optional length, optionally collated + r"(?i:character\s+varying|varchar|character|char)" + r"(?:\s*\([1-9][0-9]*\))?" + _COLLATE: -1, + # bit strings, which unlike the char family require a length here + r"(?i:bit\s+varying|varbit|bit)\s*\([1-9][0-9]*\)": -1, + # interval, with an optional field specification and an optional precision + r"(?i:interval)" + r"(?:\s+(?i:year\s+to\s+month|day\s+to\s+hour|day\s+to\s+minute" + r"|day\s+to\s+second|hour\s+to\s+minute|hour\s+to\s+second" + r"|minute\s+to\s+second|year|month|day|hour|minute|second))?" + r"(?:\s*\([0-6]\))?": 16, + r"(?i:timestamp)\s*\([0-6]\)(?:\s+(?i:with|without)\s+(?i:time\s+zone))?": 8, + # PostgreSQL caps time precision at 6 but only warns above it; the wider + # range here is the one psycodict has always accepted. + r"(?i:time)\s*\((?:[0-9]|10)\)(?:\s+(?i:without\s+time\s+zone))?": 8, + r"(?i:time)\s*\((?:[0-9]|10)\)\s+(?i:with\s+time\s+zone)": 12, + r"(?i:numeric|decimal)\s*\([1-9][0-9]*(?:,\s*(?:0|[1-9][0-9]*))?\)": -1, } param_types_whitelist = {re.compile(s): cost for (s, cost) in param_types_whitelist.items()} +# The only characters a column type may contain: letters and digits, the +# punctuation used by lengths, precisions, array markers and quoted collation +# names, and spaces between words. Checking this first rejects NUL bytes, +# control characters, non-ASCII lookalikes, semicolons and comment markers with +# a clear message, and makes the case-folded copy used for the lookup below an +# ASCII-only transformation of the string that is actually emitted. +_TYPE_CHARSET = re.compile(r'[A-Za-z0-9_ ,.()\[\]"-]*') + +# Preconstructed SQL for the fixed types, so that the common case interpolates +# a constant chosen from a closed mapping rather than a caller-supplied string. +_FIXED_TYPE_SQL = {typ: SQL(typ) for typ in types_whitelist} + + +class InvalidColumnTypeError(ValueError, RuntimeError): + """ + Raised for a column type psycodict will not put into a statement. + + A ``ValueError``, since an unusable type is a bad argument, and also a + ``RuntimeError``, which is what psycodict raised for an invalid type + before 1.0.0 and what existing callers may catch. + """ + + +def validate_column_type(typ): + """ + Check that ``typ`` is a PostgreSQL column type psycodict is willing to + create, and return the spelling that callers must put into DDL. + + Validation is centralized here because a column type is interpolated into + ``CREATE TABLE`` and ``ALTER TABLE`` statements as SQL text rather than + bound as a value: PostgreSQL has no placeholder for a type. Callers must + emit the returned spelling and never the string they passed in, since the + two are equal only for input that needed no normalization. + + INPUT: + + - ``typ`` -- a string, e.g. ``'bigint'``, ``'numeric(10, 2)'`` or + ``'text COLLATE "C"'``. Surrounding whitespace is ignored. + + OUTPUT: + + A pair ``(sql_spelling, storage_cost)``. ``storage_cost`` is the width of + the type in bytes, or -1 if it is variable, and is used to order columns + when creating a table. + + Raises ``InvalidColumnTypeError`` (a ``ValueError``) on anything else, + including a type that merely starts with a valid type. + """ + if not isinstance(typ, str): + raise InvalidColumnTypeError("Column type must be a string, not %s" % type(typ).__name__) + typ = typ.strip() + if not typ: + raise InvalidColumnTypeError("Column type must not be empty") + if not _TYPE_CHARSET.fullmatch(typ): + bad = next(c for c in typ if not _TYPE_CHARSET.fullmatch(c)) + raise InvalidColumnTypeError( + "%r is not a valid type: it contains the character %r" + % (typ, bad) + ) + fixed = types_whitelist.get(typ.lower()) + if fixed is not None: + # Emit the canonical spelling from the closed mapping rather than the + # caller's casing. + return typ.lower(), fixed + for regexp, cost in param_types_whitelist.items(): + if regexp.fullmatch(typ): + return typ, cost + raise InvalidColumnTypeError("%s is not a valid type" % (typ,)) + + +def column_type_sql(typ): + """ + The SQL fragment for a column type, validated by + :func:`validate_column_type`. + + INPUT: + + - ``typ`` -- a string giving a PostgreSQL column type + + OUTPUT: + + A ``psycopg.sql.SQL`` fragment naming the type, ready to be interpolated + into a ``CREATE TABLE`` or ``ALTER TABLE`` statement. + """ + spelling, _ = validate_column_type(typ) + fixed = _FIXED_TYPE_SQL.get(spelling) + return SQL(spelling) if fixed is None else fixed + ################################################################## # meta_* infrastructure # ################################################################## @@ -1112,9 +1231,14 @@ def _clone(self, table, tmp_table): self._execute(creator) def _check_col_datatype(self, typ): - if typ.lower() not in types_whitelist: - if not any(regexp.match(typ.lower()) for regexp in param_types_whitelist): - raise RuntimeError("%s is not a valid type" % (typ)) + """ + The spelling of the column type ``typ`` to use in DDL, or ``ValueError``. + + A thin method wrapper around :func:`validate_column_type`; callers must + build their SQL from the returned spelling rather than from ``typ``. + """ + spelling, _ = validate_column_type(typ) + return spelling def _pairs_to_dict(self, L): """ @@ -1132,12 +1256,8 @@ def _get_type_sortkey(self, typ): Returns the negated storage cost, together with the type Used to sort columns when creating a table for smaller storage footprint """ - if typ.lower() in types_whitelist: - return -types_whitelist[typ.lower()], typ - for regexp, cost in param_types_whitelist.items(): - if regexp.match(typ.lower()): - return -cost, typ - raise RuntimeError("%s is not a valid type" % (typ)) + spelling, cost = validate_column_type(typ) + return -cost, spelling def _order_columns(self, coldict, addid="bigint"): """ @@ -1151,14 +1271,15 @@ def _order_columns(self, coldict, addid="bigint"): coldict[addid] = [] coldict[addid].append("id") allcols = [] - # Note that _get_typlen checks that the type is valid - dictorder = sorted(coldict, key=self._get_type_sortkey) + # Validate every type before any of them reaches the statement, so that + # an invalid one raises rather than being interpolated: the type has to + # go in as SQL text (PostgreSQL has no placeholder for a type), and only + # the spelling the validator returns is safe to emit. + validated = {typ: validate_column_type(typ) for typ in coldict} + dictorder = sorted(coldict, key=lambda typ: (-validated[typ][1], validated[typ][0])) for typ in dictorder: for col in sorted(coldict[typ]): - # We have whitelisted the types, so it's okay to use string formatting - # to insert them into the SQL command. - # This is useful so that we can specify the collation in the type - allcols.append(SQL("{0} " + typ).format(Identifier(col))) + allcols.append(SQL("{0} {1}").format(Identifier(col), column_type_sql(typ))) return allcols def _create_table(self, name, columns, addid="bigint", tablespace=None): diff --git a/psycodict/database.py b/psycodict/database.py index 5349f1a..41842d0 100644 --- a/psycodict/database.py +++ b/psycodict/database.py @@ -1116,6 +1116,14 @@ def create_table( - boolean -- true or false. - jsonb -- data iteratively built from numerics, strings, booleans, nulls, lists and dictionaries. - timestamp -- 8-byte date and time with no timezone. + + A type has to be interpolated into the ``CREATE TABLE`` statement as + SQL text, since PostgreSQL has no placeholder for a type, so types are + restricted to the ones ``psycodict.base.validate_column_type`` accepts: + the fixed types above, arrays of them, and lengths, precisions and + collations (``numeric(10, 2)``, ``varchar(16)``, ``text COLLATE "C"``). + Anything else raises ``InvalidColumnTypeError`` before any statement + runs. """ if name in self.tablenames: raise ValueError("%s already exists" % name) diff --git a/psycodict/table.py b/psycodict/table.py index c7b9b5f..665c7b7 100644 --- a/psycodict/table.py +++ b/psycodict/table.py @@ -26,6 +26,7 @@ from .utils import DelayCommit, IdentifierWrapper, LockError from .base import ( _meta_cols_types_jsonb_idx, + column_type_sql, jsonb_idx, ) from .statstable import PostgresStatsTable @@ -1739,10 +1740,10 @@ def resort(self, suffix="", sort=None): "CREATE TEMP SEQUENCE {0} MINVALUE 0 START 0 CACHE 10000" ).format(tmp_seq)) - id_type = self.col_type["id"] + id_type = column_type_sql(self.col_type["id"]) self._execute(SQL( - "CREATE TEMP TABLE {0} (oldid %s, newid %s NOT NULL DEFAULT nextval('{1}')) ON COMMIT DROP" % (id_type, id_type) - ).format(tmp_table, tmp_seq)) + "CREATE TEMP TABLE {0} (oldid {2}, newid {3} NOT NULL DEFAULT nextval('{1}')) ON COMMIT DROP" + ).format(tmp_table, tmp_seq, id_type, id_type)) self._execute(SQL( "ALTER SEQUENCE {0} OWNED BY {1}.newid" @@ -3025,16 +3026,17 @@ def add_column(self, name, datatype, description=None, label=False, force_descri logid = self._check_locks("add_column") aborted = True try: - self._check_col_datatype(datatype) - self.col_type[name] = datatype + # Validate before touching the in-memory schema, and emit the + # spelling the validator returns rather than the argument: an + # invalid type must leave both the table and this object alone. + datatype = self._check_col_datatype(datatype) table = self.search_table with DelayCommit(self, silence=True): - # Since we have run the datatype through the whitelist, - # the following string substitution is safe - modifier = SQL("ALTER TABLE {0} ADD COLUMN {1} %s" % datatype).format( - Identifier(table), Identifier(name) + modifier = SQL("ALTER TABLE {0} ADD COLUMN {1} {2}").format( + Identifier(table), Identifier(name), column_type_sql(datatype) ) self._execute(modifier) + self.col_type[name] = datatype if name != "id": self.search_cols.insert(bisect(self.search_cols, name), name) if label: diff --git a/tests/test_security.py b/tests/test_security.py new file mode 100644 index 0000000..2d6970d --- /dev/null +++ b/tests/test_security.py @@ -0,0 +1,354 @@ +# -*- coding: utf-8 -*- +""" +Input that must never reach a statement. + +Most of psycodict's SQL is composed from placeholders and quoted identifiers, +which are safe by construction. A column type is the exception: PostgreSQL has +no placeholder for a type, so a type has to be interpolated into ``CREATE +TABLE`` and ``ALTER TABLE`` as SQL text. Everything here checks the validator +that guards that interpolation, and checks it through the public entry points +that take a type from outside psycodict: ``create_table``, ``add_column`` and +the header line of a reloaded data file. + +The negative tests do not stop at "an exception was raised". Each one appends +a statement creating a uniquely named marker table to the injected type and +then asserts that no marker exists, that the intended relation or column was +not partially created, and that the Python objects were not mutated -- an +exception raised after the damage was done would satisfy the first check alone. +""" +import uuid + +import pytest + +from psycopg.sql import SQL + +from psycodict.base import ( + InvalidColumnTypeError, + types_whitelist, + validate_column_type, +) + +import conftest + + +# Types psycodict accepts. This list is the compatibility contract of the +# grammar: a change that drops one of these breaks existing databases, whose +# column types are read back out of the catalog and revalidated on every +# reload. +VALID_TYPES = [ + "text", + "bigint", + "integer", + "boolean", + "jsonb", + "numeric", + "double precision", + "text[]", + "numeric[]", + "varchar", + "varchar(16)", + "character varying", + "character varying(64)", + "char", + "char(8)", + "numeric(10)", + "numeric(10, 2)", + "numeric(10,2)", + "bit(8)", + "bit varying(8)", + "timestamp", + "timestamp(6) with time zone", + "timestamp(0) without time zone", + "time(3)", + "time(3) with time zone", + "interval", + "interval day to second(6)", + "interval year", + 'text COLLATE "C"', + 'varchar(32) COLLATE "POSIX"', + # keywords are case-insensitive + "TEXT", + "Bigint", + 'text collate "C"', + # surrounding whitespace is not significant + " text ", +] + +# Locale collations exist only on a server whose locales include them, so they +# are checked against the grammar but not created. +VALID_TYPES_ANY_SERVER = VALID_TYPES + [ + 'character varying COLLATE "en_US.utf8"', + 'text COLLATE "de_DE.UTF-8"', +] + +# Types psycodict must refuse. The first group is the injection the validator +# exists to stop: a valid type followed by more SQL. The rest are malformed +# lengths and precisions, and characters that have no business in a type. +INVALID_TYPES = [ + "text; SELECT 1; --", + "text /* comment */", + "varchar(16); SELECT 1", + "numeric(10,2)); SELECT 1; --", + "text\nDROP TABLE anything", + "text\x00", + "text\x00; SELECT 1", + "varchar(0)", + "varchar(-1)", + "text)", + "text, x integer", + "bigint DEFAULT nextval('s')", + 'text COLLATE "C"; SELECT 1', + 'text COLLATE "C" || pg_sleep(10)', + "no_such_type", + "", + " ", + # a lookalike: the second character is a Cyrillic 'е' + "tеxt", +] + + +def marker_name(): + """ + A table name that only an executed injection could bring into existence. + """ + return "marker_%s" % uuid.uuid4().hex[:12] + + +def injected(typ, marker): + """ + ``typ`` extended so that executing it as DDL would create ``marker``. + + The type is interpolated into ``CREATE TABLE t (col , ...)``, so + closing the parenthesis ends that statement and what follows is a statement + of its own. psycopg sends a parameterless statement with the simple query + protocol, which runs every statement in the string. + """ + return "%s); CREATE TABLE %s (x integer); --" % (typ, marker) + + +def table_exists(db, name): + return bool( + db._execute( + SQL("SELECT 1 FROM information_schema.tables WHERE table_name = %s"), + [name], + ).fetchone() + ) + + +def column_exists(db, table, column): + return bool( + db._execute( + SQL( + "SELECT 1 FROM information_schema.columns " + "WHERE table_name = %s AND column_name = %s" + ), + [table, column], + ).fetchone() + ) + + +################################################################## +# the validator itself # +################################################################## + + +@pytest.mark.parametrize("typ", VALID_TYPES_ANY_SERVER) +def test_valid_types_are_accepted(typ): + spelling, cost = validate_column_type(typ) + assert spelling.strip() == spelling + assert isinstance(cost, int) + + +@pytest.mark.parametrize("typ", INVALID_TYPES) +def test_invalid_types_are_rejected(typ): + with pytest.raises(InvalidColumnTypeError): + validate_column_type(typ) + + +@pytest.mark.parametrize("typ", ["text", "bigint", "numeric(10, 2)", 'text COLLATE "C"']) +def test_appended_sql_is_rejected(typ): + """ + A valid type followed by a statement is not a valid type. + + This is the regression test for the vulnerability: the validator used to + match a prefix, so every type in this list passed with the injection still + attached and was then interpolated into DDL. + """ + with pytest.raises(InvalidColumnTypeError): + validate_column_type(injected(typ, marker_name())) + + +@pytest.mark.parametrize("typ", [None, 17, b"text", ["text"]]) +def test_non_strings_are_rejected(typ): + with pytest.raises(InvalidColumnTypeError): + validate_column_type(typ) + + +def test_invalid_column_type_error_is_a_value_error(): + # ValueError is the documented type; RuntimeError is what psycodict raised + # before 1.0.0, and callers catching it must keep working. + assert issubclass(InvalidColumnTypeError, ValueError) + assert issubclass(InvalidColumnTypeError, RuntimeError) + + +def test_the_validated_spelling_is_what_callers_must_emit(): + # Case-insensitive keywords are canonicalized, but a collation name is a + # quoted identifier: "C" is a collation and "c" is not, so the case of the + # spelling the validator returns has to be the case it validated. + assert validate_column_type("TEXT")[0] == "text" + assert validate_column_type(" Bigint ")[0] == "bigint" + assert validate_column_type('text COLLATE "C"')[0] == 'text COLLATE "C"' + assert validate_column_type("numeric(10, 2)")[0] == "numeric(10, 2)" + + +def test_every_fixed_type_validates_to_itself(): + for typ in types_whitelist: + assert validate_column_type(typ) == (typ, types_whitelist[typ]) + + +################################################################## +# create_table # +################################################################## + + +def test_create_table_rejects_an_injected_type(db): + marker = marker_name() + name = "test_%s" % uuid.uuid4().hex[:12] + with pytest.raises(InvalidColumnTypeError): + db.create_table(name, [("n", "integer"), ("c", injected("text", marker))], "n") + assert not table_exists(db, marker) + assert not table_exists(db, name) + assert name not in db.tablenames + + +def test_create_table_rejects_an_injected_id_type(db): + marker = marker_name() + name = "test_%s" % uuid.uuid4().hex[:12] + with pytest.raises(InvalidColumnTypeError): + db.create_table(name, [("n", "integer")], "n", id_type=injected("bigint", marker)) + assert not table_exists(db, marker) + assert not table_exists(db, name) + + +@pytest.mark.parametrize("typ", VALID_TYPES) +def test_create_table_accepts_every_valid_type(db, typ): + """ + Every type the validator accepts is a type PostgreSQL accepts. + + A grammar that drifts from the server's is as much a bug as a permissive + one: it would reject a column that an existing database already has. + """ + name = "test_%s" % uuid.uuid4().hex[:12] + db.create_table(name, [("n", "integer"), ("c", typ)], "n") + try: + assert column_exists(db, name, "c") + finally: + db.drop_table(name, force=True) + + +################################################################## +# add_column # +################################################################## + + +def test_add_column_rejects_an_injected_type(db, empty_table): + marker = marker_name() + before_cols = list(empty_table.search_cols) + before_types = dict(empty_table.col_type) + with pytest.raises(InvalidColumnTypeError): + empty_table.add_column("bad", injected("text", marker)) + assert not table_exists(db, marker) + assert not column_exists(db, empty_table.search_table, "bad") + # The in-memory schema must not have been updated by the failed call. + assert empty_table.search_cols == before_cols + assert empty_table.col_type == before_types + assert "bad" not in empty_table.col_type + + +def test_add_column_still_adds_a_valid_column(db, empty_table): + empty_table.add_column("extra", 'text COLLATE "C"') + assert column_exists(db, empty_table.search_table, "extra") + assert "extra" in empty_table.search_cols + empty_table.insert_many([{"n": 1, "extra": "hello"}]) + assert empty_table.lucky({"n": 1}, "extra") == "hello" + + +################################################################## +# types read from the header of a data file # +################################################################## + + +def _write_search_file(path, cols, types, rows=(), sep="|"): + with open(path, "w") as F: + F.write(sep.join(cols) + "\n") + F.write(sep.join(types) + "\n") + F.write("\n") + for row in rows: + F.write(sep.join(str(x) for x in row) + "\n") + + +def test_reload_with_adjust_schema_rejects_an_injected_header_type(db, filled_table, tmp_path): + marker = marker_name() + name = filled_table.search_table + searchfile = str(tmp_path / "search.txt") + _write_search_file( + searchfile, + ["id", "n", "label"], + ["bigint", "integer", injected("text", marker)], + [(1, 1, "one")], + ) + with pytest.raises(InvalidColumnTypeError): + filled_table.reload(searchfile, adjust_schema=True) + assert not table_exists(db, marker) + assert not table_exists(db, name + "_tmp") + # The live table still holds its original rows. + assert db[name].count() == 200 + + +def test_reload_with_adjust_schema_accepts_a_valid_header_type(db, filled_table, tmp_path): + name = filled_table.search_table + searchfile = str(tmp_path / "search.txt") + _write_search_file( + searchfile, + ["id", "n", "label"], + ["bigint", "integer", "text"], + [(1, 1, "one"), (2, 2, "two")], + ) + filled_table.reload(searchfile, adjust_schema=True) + table = db[name] + assert table.count() == 2 + assert table.lucky({"n": 2}, "label") == "two" + + +def test_reload_all_with_adjust_schema_rejects_an_injected_header_type(db, filled_table, tmp_path): + """ + The same check on the path that creates a table nobody has seen before. + + ``reload_all(adjust_schema=True)`` builds a table out of a data folder: the + column types come from the header of a file on disk, which is exactly the + input this validation exists for. + """ + marker = marker_name() + folder = tmp_path / "data" + db.copy_to([filled_table.search_table], str(folder)) + + new_name = "test_%s" % uuid.uuid4().hex[:12] + old_name = filled_table.search_table + for path in sorted(folder.glob(old_name + "*")): + path.rename(folder / path.name.replace(old_name, new_name, 1)) + # The meta row names the table it describes, and reload_all checks it. + metafile = folder / (new_name + "_meta.txt") + metafile.write_text(metafile.read_text().replace(old_name, new_name, 1)) + searchfile = folder / (new_name + ".txt") + lines = searchfile.read_text().split("\n") + lines[1] = lines[1].replace("text", injected("text", marker), 1) + searchfile.write_text("\n".join(lines)) + + with pytest.raises(InvalidColumnTypeError): + db.reload_all(str(folder), adjust_schema=True) + assert not table_exists(db, marker) + assert not table_exists(db, new_name) + assert new_name not in db.tablenames + # and the table the folder was exported from is untouched + assert db[old_name].count() == 200 + assert conftest # the fixtures above come from conftest From a43947b6c8aa6ae4b559dc918d1a1dff7d9cf6dc Mon Sep 17 00:00:00 2001 From: David Roe Date: Mon, 3 Aug 2026 03:12:45 -0400 Subject: [PATCH 2/5] Validate index and constraint definitions at import and at use 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 --- CHANGELOG.md | 26 ++ DataManagement.md | 19 ++ SECURITY.md | 10 + psycodict/base.py | 605 +++++++++++++++++++++++++++++++++++++++-- psycodict/table.py | 216 ++++++++------- tests/test_security.py | 349 +++++++++++++++++++++++- 6 files changed, 1098 insertions(+), 127 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 56bd2fd..6cd46d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -179,6 +179,32 @@ hardening standalone use; the highlights: raises `InvalidColumnTypeError`, a subclass of both `ValueError` and the `RuntimeError` raised before. +- **Index and constraint definitions are validated when imported and again + when used.** `meta_indexes` and `meta_constraints` hold what an index or + constraint is rebuilt from, sometimes years and several psycodict versions + after it was created, and in between the rows can be edited with plain SQL, + exported to a file, carried to another database and imported, or restored + from history. `create_index` and `create_constraint` checked their arguments, + but nothing rechecked a row on the way back out, and the access method, + column modifiers, storage-parameter names, check function and partial-index + predicate were formatted into the statement as text. A poisoned + `meta_indexes` row could therefore run its own statements the next time the + index was restored. Definitions are now checked by + `validate_index_definition` / `validate_constraint_definition` at import + (`reload_indexes`, `reload_constraints`, the `revert_*` methods) and again + immediately before the DDL is built, and everything they are built from is + either a quoted identifier or fixed SQL selected by a validated key. An + invalid definition raises `InvalidDefinitionError` (a `ValueError`). Names + are held to what they must be rather than to a convention: a column is quoted + wherever it is used and any string can be one, and a relation name is not + length-checked at DDL time because psycodict makes the names it uses by + appending `_tmp` or `_oldN` to one that already exists. Because + the import runs inside the reload transaction, the metadata that was there + before survives intact. A partial-index predicate remains raw administrative + SQL, but may no longer contain a semicolon, a comment, a dollar-quoted string + or control characters. The unused, unvalidated `_copy_from_meta` helper is + gone. + ### Infrastructure - A test suite of 500+ tests and a continuous-integration workflow covering the diff --git a/DataManagement.md b/DataManagement.md index 1999990..653e630 100644 --- a/DataManagement.md +++ b/DataManagement.md @@ -48,6 +48,25 @@ psycodict keeps its own bookkeeping in a handful of tables that live alongside y * **`meta_tables_hist`**, **`meta_indexes_hist`**, **`meta_constraints_hist`** — versioned history of the three tables above, so that `reload_meta`/`revert_meta` can roll a table's metadata forward and back. * **`meta_format`** — a single-row `(version, min_compat)` stamp of the metadata *format*: the layout of the `meta_*` tables, versioned by an integer aligned with psycodict's major version (`META_FORMAT`). Every connection checks it. The same format connects silently; an older but compatible format connects with a warning and operates at that older format (newer features unavailable) so a not-yet-migrated or read-only database — the LMFDB devmirror, say — keeps working; a newer format connects when its stamped `min_compat` admits this psycodict, and is otherwise refused. A database that has meta tables but no `meta_format` is the unstamped 0.x baseline (format 0); one with no meta tables at all is fresh and must be connected to with `PostgresDatabase(create=True)`, which bootstraps all of the above. Migrating to a newer format is deliberate — `db.upgrade_metadata()` or `PostgresDatabase(upgrade=True)`, never a side effect of connecting. See [MetadataFormats.md](MetadataFormats.md) for the full policy (including why a pre-1.0 psycodict must not be pointed at a migrated database). +Rows of `meta_indexes` and `meta_constraints` become DDL when an index or +constraint is rebuilt, so psycodict validates them both when they are imported +(`reload_indexes`, `reload_constraints`, `revert_indexes`, `revert_constraints`) +and again immediately before the `CREATE INDEX` or `ALTER TABLE` statement is +built. The access method, the column modifiers, the storage-parameter names and +the check function of a `CHECK` constraint must be ones psycodict recognizes, +and are emitted as quoted identifiers or fixed SQL rather than as the stored +text. A metadata file is therefore no more trusted than the database it is +loaded into: an invalid definition raises, and because the import runs in a +transaction, the metadata that was there before is left untouched. A `CHECK` +constraint's function must be listed in `PostgresTable._valid_check_functions` +both to create it and to rebuild it. + +The predicate of a partial index (`create_index(..., where=...)`) stays raw SQL +— psycodict does not parse it — but it is checked for the things that would let +it stop being a predicate: a semicolon, a comment, a dollar-quoted string, a +control character or an unreasonable length. It is administrative input, not +website input. + ## Row-level writes These mutate the live table directly. They are convenient for small edits; for anything large prefer the [file operations](#bulk-file-operations). Each records an entry in the change log and, on success, updates the maintained row `total`. diff --git a/SECURITY.md b/SECURITY.md index 090efc5..e066bc9 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -45,6 +45,16 @@ with untrusted input. Injection caused solely by deliberately passing untrusted input through `$raw` is not considered a psycodict vulnerability. A way to inject SQL through an interface that is intended to be safe is in scope. +The `where=` predicate of a partial index (`create_index`) is likewise an +administrative SQL expression: psycodict does not parse it, and it must not be +built from untrusted input. It is validated to the extent that it must remain a +predicate — it may not contain a statement terminator, a comment or a +dollar-quoted string — so that a predicate stored in `meta_indexes` cannot turn +a later `CREATE INDEX` into two statements. Index and constraint definitions +read back out of the `meta_*` tables, out of an exported metadata file or out +of the history tables are validated before they are turned into DDL, so a +poisoned metadata row is in scope. + Problems in PostgreSQL, psycopg, or another dependency should normally be reported to that project unless psycodict uses the dependency unsafely. Ordinary bugs without a security impact may be reported through the public diff --git a/psycodict/base.py b/psycodict/base.py index 1806dc1..5d899d8 100644 --- a/psycodict/base.py +++ b/psycodict/base.py @@ -16,7 +16,7 @@ import re import sys import time -from collections import defaultdict +from collections import defaultdict, namedtuple from psycopg import ( ClientCursor, @@ -116,21 +116,21 @@ # than by lowercasing the input, because a collation name is a quoted # identifier and therefore case-sensitive: "C" is a collation, "c" is not. -# Collations psycodict accepts: the two collations every PostgreSQL server has, -# the two SQL-standard names, and locale names of the usual ll_CC[.charset] -# shape. -_COLLATION_NAME = ( - r'(?:C|POSIX|default|ucs_basic' - r'|[A-Za-z]{2}(?:_[A-Za-z]{2,3})?(?:\.[A-Za-z0-9-]+)?(?:@[A-Za-z0-9]+)?)' -) +# A collation name, as it appears inside the double quotes of a COLLATE clause. +# Deliberately permissive about which collations exist -- "C", "POSIX", +# "en_US.utf8", "C.UTF-8", "und-x-icu" and every other ICU name are all real, +# and psycodict has no business deciding which a server has -- but restricted +# to characters that cannot end the quoted name early. +_COLLATION_NAME = r"[A-Za-z0-9][A-Za-z0-9_.@+-]*" _COLLATE = r'(?:\s+(?i:collate)\s+"' + _COLLATION_NAME + r'")?' param_types_whitelist = { - # text, optionally collated - r"(?i:text)" + _COLLATE: -1, - # the char family: an optional length, optionally collated + # text, optionally an array, optionally collated + r"(?i:text)(?:\[\])?" + _COLLATE: -1, + # the char family: an optional length, an optional array marker, optionally + # collated r"(?i:character\s+varying|varchar|character|char)" - r"(?:\s*\([1-9][0-9]*\))?" + _COLLATE: -1, + r"(?:\s*\([1-9][0-9]*\))?(?:\[\])?" + _COLLATE: -1, # bit strings, which unlike the char family require a length here r"(?i:bit\s+varying|varbit|bit)\s*\([1-9][0-9]*\)": -1, # interval, with an optional field specification and an optional precision @@ -154,7 +154,7 @@ # control characters, non-ASCII lookalikes, semicolons and comment markers with # a clear message, and makes the case-folded copy used for the lookup below an # ASCII-only transformation of the string that is actually emitted. -_TYPE_CHARSET = re.compile(r'[A-Za-z0-9_ ,.()\[\]"-]*') +_TYPE_CHARSET = re.compile(r'[A-Za-z0-9_ ,.@()\[\]"-]*') # Preconstructed SQL for the fixed types, so that the common case interpolates # a constant chosen from a closed mapping rather than a caller-supplied string. @@ -368,7 +368,8 @@ def _meta_cols_types_jsonb_idx(meta_name, fmt=None): should pass the connection's format, ``self._db._meta_format``, so that their SQL matches the columns the database actually has. """ - assert meta_name in ["meta_tables", "meta_indexes", "meta_constraints"] + if meta_name not in ("meta_tables", "meta_indexes", "meta_constraints"): + raise ValueError("Unknown metadata table %r" % (meta_name,)) if meta_name == "meta_tables": meta_cols = _meta_tables_cols meta_types = _meta_tables_types @@ -398,6 +399,475 @@ def _meta_table_name(meta_name): return table_name +################################################################## +# index and constraint definitions # +################################################################## + +# An index or constraint definition lives in meta_indexes or meta_constraints +# between the call that creates it and the DDL that rebuilds it, which may be +# years and several psycodict versions later. In between it can be edited with +# plain SQL, exported to a file, carried to another database and imported, or +# restored from the _hist tables, so "it must once have passed through +# create_index" is not something a statement builder can rely on. The +# validators here are therefore applied at both ends: when a definition is +# imported, and again immediately before it is turned into DDL. + +# The index access methods psycodict creates indexes with, mapped to the +# non-default operator classes each one accepts. +_operator_classes = { + "brin": ["inet_minmax_ops"], + "btree": [ + "bpchar_pattern_ops", + "cidr_ops", + "record_image_ops", + "text_pattern_ops", + "varchar_ops", + "varchar_pattern_ops", + ], + "gin": ["jsonb_path_ops", "array_ops"], + "gist": ["inet_ops"], + "hash": [ + "bpchar_pattern_ops", + "cidr_ops", + "text_pattern_ops", + "varchar_ops", + "varchar_pattern_ops", + ], + "spgist": ["kd_point_ops"], +} + +# Valid storage parameters by access method, used in creating indexes. +_valid_storage_params = { + "brin": ["pages_per_range", "autosummarize"], + "btree": ["fillfactor"], + "gin": ["fastupdate", "gin_pending_list_limit"], + "gist": ["fillfactor", "buffering"], + "hash": ["fillfactor"], + "spgist": ["fillfactor"], +} + +# What each storage parameter's value may be: an inclusive integer range, or a +# closed set of the strings/booleans PostgreSQL accepts. A value that is not +# of the expected kind is rejected rather than passed to the server, so that a +# metadata row cannot smuggle anything into the WITH clause. +_storage_param_values = { + "fillfactor": range(10, 101), + "pages_per_range": range(1, 131073), + "gin_pending_list_limit": range(64, 2097153), + "autosummarize": (True, False), + "fastupdate": (True, False), + "buffering": ("auto", "on", "off"), +} + +# The column modifiers that are not operator classes. Each maps to the SQL it +# is emitted as, so that the statement is built from constants rather than from +# the stored string, and to its slot: an index column takes at most one +# operator class, one direction and one null placement, and PostgreSQL wants +# them in that order. +_index_modifiers = { + "asc": ("direction", SQL("ASC")), + "desc": ("direction", SQL("DESC")), + "nulls first": ("nulls", SQL("NULLS FIRST")), + "nulls last": ("nulls", SQL("NULLS LAST")), +} + +# PostgreSQL truncates an identifier at 63 bytes, which would make a name and +# its _tmp variant indistinguishable, so psycodict refuses the longer name +# instead. +MAX_IDENTIFIER_LENGTH = 63 + +_RELATION_NAME = re.compile(r"[A-Za-z_][A-Za-z0-9_$]*") + +# A partial index predicate is raw SQL by design (see create_index), but it is +# appended to CREATE INDEX, where a statement terminator or a comment would let +# a metadata row carry a second statement along with it. +_MAX_PREDICATE_LENGTH = 4096 + + +_valid_constraint_types = ("UNIQUE", "CHECK", "NOT NULL") + + +def _type_name(value): + """ + The name of a value's type, for error messages. + + A helper because ``type`` is the name of a parameter in the validators + below, following the column of ``meta_indexes`` it holds. + """ + return value.__class__.__name__ + + +class InvalidDefinitionError(ValueError): + """ + Raised for an index or constraint definition psycodict will not build DDL + from, whether it came from a caller, a metadata file or a ``meta_*`` row. + """ + + +def validate_relation_name(name, kind="Relation", max_length=None): + """ + Check that ``name`` can be used as a PostgreSQL relation name. + + INPUT: + + - ``name`` -- the name of a table, index or constraint + - ``kind`` -- what the name names, used in the error message + - ``max_length`` -- a byte length to hold the name to, for a name psycodict + is being asked to create. Not applied by default: psycodict builds the + names it uses in DDL by appending ``_tmp`` or ``_oldN`` to an existing + one, and an index created at the 63-byte limit would then fail every + reload rather than being truncated by PostgreSQL as it always was. + + OUTPUT: + + ``name`` itself. Names are quoted with ``Identifier`` wherever they are + used, so this is not what stops injection; it stops a name that no + ``Identifier`` could round-trip, or that came from somewhere it should not + have. + """ + if not isinstance(name, str): + raise InvalidDefinitionError( + "%s name must be a string, not %s" % (kind, type(name).__name__) + ) + if not _RELATION_NAME.fullmatch(name): + raise InvalidDefinitionError( + "%s name %r must consist of letters, digits and underscores, and " + "must not start with a digit" % (kind, name) + ) + if max_length is not None and len(name.encode("utf-8")) > max_length: + raise InvalidDefinitionError( + "%s name %r is longer than PostgreSQL's %s byte limit" + % (kind, name, max_length) + ) + return name + + +def validate_column_name(name): + """ + Check a column name an index or constraint definition refers to. + + Columns are quoted with ``Identifier`` wherever they are used, and a column + that exists is a column whatever it is called -- the LMFDB has one called + ``2adic_index`` -- so this checks only that the name is a string psycodict + can put in a statement at all. + """ + if not isinstance(name, str): + raise InvalidDefinitionError( + "Column name must be a string, not %s" % _type_name(name) + ) + if not name: + raise InvalidDefinitionError("Column name must not be empty") + for char in name: + if ord(char) < 0x20 or 0x7F <= ord(char) <= 0x9F: + raise InvalidDefinitionError( + "Column name %r contains the control character %r" % (name, char) + ) + return name + + +def validate_index_predicate(predicate): + """ + Check the predicate of a partial index. + + The predicate is administrative raw SQL: psycodict does not parse it, and + ``create_index`` documents that it is trusted input. What this rules out + is a predicate that does not stay a predicate -- one that ends the + ``CREATE INDEX`` statement it is appended to, or comments out the rest of + it -- so that a poisoned ``meta_indexes`` row cannot turn a restore into + two statements. + + INPUT: + + - ``predicate`` -- a string giving the ``WHERE`` clause of a partial index + + OUTPUT: + + The predicate, stripped of surrounding whitespace. + + This is deliberately conservative: a predicate that needs a semicolon, a + comment or a dollar-quoted string is rejected rather than analyzed. + """ + if not isinstance(predicate, str): + raise InvalidDefinitionError( + "Index predicate must be a string, not %s" % type(predicate).__name__ + ) + stripped = predicate.strip() + if not stripped: + raise InvalidDefinitionError("Index predicate must not be empty") + if len(stripped) > _MAX_PREDICATE_LENGTH: + raise InvalidDefinitionError( + "Index predicate is longer than %s characters" % _MAX_PREDICATE_LENGTH + ) + bad = { + "\x00": "a NUL character", + ";": "a semicolon", + "--": "a comment", + "/*": "a comment", + "*/": "a comment", + "$$": "a dollar-quoted string", + } + for token, description in bad.items(): + if token in stripped: + raise InvalidDefinitionError( + "Index predicate %r contains %s, which is not allowed: the " + "predicate is appended to CREATE INDEX and must not be able " + "to end the statement" % (predicate, description) + ) + for char in stripped: + if char not in "\t\n\r" and (ord(char) < 0x20 or 0x7F <= ord(char) <= 0x9F): + raise InvalidDefinitionError( + "Index predicate %r contains the control character %r" + % (predicate, char) + ) + # $tag$ ... $tag$ quoting, which the checks above would otherwise miss + if re.search(r"\$[A-Za-z_][A-Za-z0-9_]*\$", stripped): + raise InvalidDefinitionError( + "Index predicate %r contains a dollar-quoted string, which is not " + "allowed" % (predicate,) + ) + return stripped + + +def index_modifier_sql(modifier, type): + """ + The SQL for one modifier of one index column. + + INPUT: + + - ``modifier`` -- a modifier normalized by :func:`validate_index_definition` + - ``type`` -- the access method of the index + + OUTPUT: + + A fixed ``SQL`` constant for a direction or null placement, and a quoted + identifier for an operator class. Nothing here is built by formatting the + stored string into SQL text. + """ + if modifier in _index_modifiers: + return _index_modifiers[modifier][1] + if modifier in _operator_classes.get(type, ()): + return Identifier(modifier) + raise InvalidDefinitionError("Invalid modifier %r for a %s index" % (modifier, type)) + + +# The fields are named for what they are rather than for the meta_* columns +# they come from: the column is called "type" in both tables, which says less +# than access_method and constraint_type do. +IndexDefinition = namedtuple( + "IndexDefinition", + ["name", "table", "access_method", "columns", "modifiers", "storage_params", + "whereclause"], +) + +ConstraintDefinition = namedtuple( + "ConstraintDefinition", + ["name", "table", "constraint_type", "columns", "check_func"], +) + + +def _validate_columns(columns, valid_columns, kind): + """ + Check the column list of an index or constraint definition. + + ``valid_columns`` is the set of columns of the relation the definition + applies to, or None when it is unknown -- at import time the relation the + definition will be built on may not exist yet, so the columns are checked + for shape there and for existence at use time. + """ + if isinstance(columns, str) or not isinstance(columns, (list, tuple)): + raise InvalidDefinitionError( + "%s columns must be a list, not %s" % (kind, type(columns).__name__) + ) + if not columns: + raise InvalidDefinitionError("%s must have at least one column" % kind) + columns = list(columns) + for col in columns: + validate_column_name(col) + if valid_columns is not None and col not in valid_columns: + raise InvalidDefinitionError( + "%s refers to %s, which is not a column of the table" % (kind, col) + ) + return columns + + +def validate_index_definition( + name, table, type, columns, modifiers, storage_params, whereclause=None, + valid_columns=None, +): + """ + Check an index definition and return it normalized. + + INPUT: + + - ``name``, ``table`` -- the names of the index and of the relation it is + built on. ``name`` may be None when the caller has not generated it yet + (``create_index`` derives it from the columns it is validating here). + - ``type`` -- the access method, one of the keys of ``_operator_classes`` + - ``columns`` -- a nonempty list of column names + - ``modifiers`` -- a list, of the same length as ``columns``, of lists of + modifiers for each column: an operator class valid for ``type``, a + direction and a null placement + - ``storage_params`` -- a dictionary of storage parameters valid for + ``type`` + - ``whereclause`` -- the predicate of a partial index, or None + - ``valid_columns`` -- the columns of the relation, if known; when given, + every column of the index must be one of them + + OUTPUT: + + An ``IndexDefinition``. Its ``modifiers`` are canonicalized to the + spellings in ``_operator_classes`` and ``_index_modifiers`` and sorted into + the order PostgreSQL expects, so the statement builder never emits a string + that came out of the metadata. + """ + if name is not None: + validate_relation_name(name, "Index") + validate_relation_name(table, "Table") + if type not in _operator_classes: + raise InvalidDefinitionError( + "Unrecognized index type %r; psycodict supports %s" + % (type, ", ".join(sorted(_operator_classes))) + ) + columns = _validate_columns(columns, valid_columns, "Index") + + if modifiers is None: + modifiers = [[]] * len(columns) + if isinstance(modifiers, str) or not isinstance(modifiers, (list, tuple)): + raise InvalidDefinitionError( + "Index modifiers must be a list, not %s" % _type_name(modifiers) + ) + if len(modifiers) != len(columns): + raise InvalidDefinitionError( + "Index has %s columns but %s modifier lists" + % (len(columns), len(modifiers)) + ) + normalized_modifiers = [] + for mods in modifiers: + if mods is None: + mods = [] + if isinstance(mods, str) or not isinstance(mods, (list, tuple)): + raise InvalidDefinitionError( + "Index modifiers for a column must be a list, not %s" % _type_name(mods) + ) + slots = {} + for mod in mods: + if not isinstance(mod, str): + raise InvalidDefinitionError( + "Index modifier must be a string, not %s" % _type_name(mod) + ) + key = " ".join(mod.lower().split()) + if key in _index_modifiers: + slot, _ = _index_modifiers[key] + elif key in _operator_classes[type]: + slot = "opclass" + else: + raise InvalidDefinitionError( + "Invalid modifier %r for a %s index" % (mod, type) + ) + if slot in slots: + raise InvalidDefinitionError( + "Index column has two %s modifiers: %r and %r" + % (slot, slots[slot], key) + ) + slots[slot] = key + normalized_modifiers.append( + [slots[slot] for slot in ("opclass", "direction", "nulls") if slot in slots] + ) + + if storage_params is None: + storage_params = {} + if not isinstance(storage_params, dict): + raise InvalidDefinitionError( + "Index storage parameters must be a dictionary, not %s" + % _type_name(storage_params) + ) + for key, val in storage_params.items(): + if key not in _valid_storage_params[type]: + raise InvalidDefinitionError( + "Invalid storage parameter %r for a %s index" % (key, type) + ) + allowed = _storage_param_values[key] + if isinstance(allowed, range): + # bool is a subclass of int, and WITH (fillfactor = true) is not a + # thing, so it has to be excluded explicitly + if isinstance(val, bool) or not isinstance(val, int): + raise InvalidDefinitionError( + "Storage parameter %s must be an integer, not %s" + % (key, _type_name(val)) + ) + if val not in allowed: + raise InvalidDefinitionError( + "Storage parameter %s must be between %s and %s, not %s" + % (key, allowed[0], allowed[-1], val) + ) + elif val not in allowed: + raise InvalidDefinitionError( + "Storage parameter %s must be one of %s, not %r" + % (key, ", ".join(str(x) for x in allowed), val) + ) + + if whereclause is not None: + whereclause = validate_index_predicate(whereclause) + + return IndexDefinition( + name, table, type, columns, normalized_modifiers, dict(storage_params), + whereclause, + ) + + +def validate_constraint_definition( + name, table, type, columns, check_func, valid_columns=None, + valid_check_functions=(), +): + """ + Check a constraint definition and return it normalized. + + INPUT: + + - ``name``, ``table`` -- the names of the constraint and of the relation it + applies to + - ``type`` -- ``"UNIQUE"``, ``"CHECK"`` or ``"NOT NULL"`` + - ``columns`` -- a nonempty list of column names; ``NOT NULL`` takes one + - ``check_func`` -- for a CHECK constraint, the name of the function it + calls, which must be one of ``valid_check_functions``; None otherwise + - ``valid_columns`` -- the columns of the relation, if known + - ``valid_check_functions`` -- the approved check functions, normally + ``PostgresTable._valid_check_functions`` + + OUTPUT: + + A ``ConstraintDefinition``. + """ + if name is not None: + # None while ``create_constraint`` is still deriving the name from the + # columns it is validating here. + validate_relation_name(name, "Constraint") + validate_relation_name(table, "Table") + if not isinstance(type, str) or type not in _valid_constraint_types: + raise InvalidDefinitionError( + "Unrecognized constraint type %r; psycodict supports %s" + % (type, ", ".join(_valid_constraint_types)) + ) + columns = _validate_columns(columns, valid_columns, "Constraint") + if type == "NOT NULL" and len(columns) != 1: + raise InvalidDefinitionError( + "A NOT NULL constraint has one column, not %s" % len(columns) + ) + if (check_func is None) == (type == "CHECK"): + raise InvalidDefinitionError( + "A check function belongs to a CHECK constraint and only to one" + ) + if check_func is not None: + if check_func not in valid_check_functions: + raise InvalidDefinitionError( + "%r is not an approved check function; add it to " + "PostgresTable._valid_check_functions to allow it" + % (check_func,) + ) + validate_relation_name(check_func, "Check function") + return ConstraintDefinition(name, table, type, columns, check_func) + + class PostgresBase(): """ A base class for various objects that interact with Postgres. @@ -990,6 +1460,26 @@ def _column_types(self, table_name, data_types=None): has_id = True return sorted(col_list), col_type, has_id + def _relation_columns(self, table): + """ + The set of column names of ``table``, or None if it has none. + + Used to check an index or constraint definition against the relation it + will be built on at the moment it is built. None -- for a relation + that does not exist yet, such as the ``_tmp`` table of a reload that has + not created it -- means "unknown", and leaves the columns unchecked + here so that PostgreSQL gives its own error rather than a misleading + one about columns. + """ + cur = self._execute( + SQL("SELECT column_name FROM information_schema.columns WHERE table_name = %s"), + [table], + silent=True, + commit=False, + ) + columns = {rec[0] for rec in cur} + return columns or None + def _copy_to_select(self, select, filename, header="", sep="|", silent=False): """ Using COPY ... TO STDOUT, exports the data from a select statement. @@ -1512,19 +2002,63 @@ def _meta_file_columns(self, meta_name, filename, sep="|"): ) return db_cols[:width] - def _copy_from_meta(self, meta_name, filename, sep="|"): - # Take the column list from the file's width, so files exported from - # an older metadata format keep loading after the database migrates - # (columns the file predates are left NULL). - meta_cols = self._meta_file_columns(meta_name, filename, sep) - if meta_cols is None: + def _validate_meta_rows(self, meta_name, meta_cols, rows, source): + """ + Check index or constraint definitions that have just been loaded. + + INPUT: + + - ``meta_name`` -- ``"meta_indexes"``, ``"meta_constraints"`` or + ``"meta_tables"`` + - ``meta_cols`` -- the columns the rows carry, in order + - ``rows`` -- the rows, as returned by the database (jsonb columns + already decoded) + - ``source`` -- where they came from, for the error message + + Must be called inside the transaction that loaded the rows, so that + raising leaves neither the new definitions nor the deletion of the old + ones behind. + + The columns of the definitions are not checked against the table here: + an index may legitimately name a column that a reload is about to add, + and the relation the definition will be built on need not exist yet. + Column existence is checked when the definition becomes DDL. + """ + if meta_name not in ("meta_indexes", "meta_constraints"): return - try: - with open(filename) as F: - self._copy_from_stdin(F, meta_name, meta_cols, sep) - except Exception: - self.conn.rollback() - raise + for row in rows: + record = dict(zip(meta_cols, row)) + try: + if meta_name == "meta_indexes": + validate_index_definition( + record["index_name"], + record["table_name"], + record["type"], + record["columns"], + record["modifiers"], + record["storage_params"], + record.get("whereclause"), + ) + else: + validate_constraint_definition( + record["constraint_name"], + record["table_name"], + record["type"], + record["columns"], + record["check_func"], + # a PostgresTable attribute: metadata for a search + # table is always reloaded through its table object + valid_check_functions=getattr(self, "_valid_check_functions", ()), + ) + except ValueError as err: + raise InvalidDefinitionError( + "%s in %s is not a definition psycodict can build: %s" + % ( + record.get("index_name") or record.get("constraint_name"), + source, + err, + ) + ) def _get_current_meta_version(self, meta_name, search_table): # the column which will match search_table @@ -1596,12 +2130,20 @@ def _reload_meta(self, meta_name, filename, search_table, sep="|"): place_holder = SQL(", ").join(Placeholder() * len(cols)) query = SQL("INSERT INTO {} ({}) VALUES ({})").format(meta_name_hist_sql, cols_sql, place_holder) + imported = [] for row in rows: + imported.append(row) row = [ Json(elt) if i in jsonb_idx else elt for i, elt in enumerate(row) ] self._execute(query, row + [version]) + # Validate what was imported, inside the transaction: a file that + # carries a definition psycodict would not build raises here, and + # the surrounding DelayCommit rolls back both the DELETE above and + # the rows just loaded, leaving the old metadata in place. + self._validate_meta_rows(meta_name, meta_cols, imported, filename) + def _revert_meta(self, meta_name, search_table, version=None): meta_cols, _, jsonb_idx = _meta_cols_types_jsonb_idx(meta_name, self._db._meta_format) # the column which will match search_table @@ -1643,7 +2185,18 @@ def _revert_meta(self, meta_name, search_table, version=None): query_hist = SQL("INSERT INTO {} ({}) VALUES ({})").format( meta_name_hist_sql, cols_sql, place_holder ) + restored = [] for row in rows: + restored.append(row) row = [Json(elt) if i in jsonb_idx else elt for i, elt in enumerate(row)] self._execute(query, row) self._execute(query_hist, row + [currentversion + 1]) + + # History is as untrusted as a file: the rows in it were written by + # whatever psycodict version was running at the time, and can be + # edited in place like any other table. Validating here, inside + # the DelayCommit, means a poisoned version cannot be reverted to. + self._validate_meta_rows( + meta_name, meta_cols, restored, + "%s_hist version %s" % (meta_name, version), + ) diff --git a/psycodict/table.py b/psycodict/table.py index 665c7b7..f54bdbd 100644 --- a/psycodict/table.py +++ b/psycodict/table.py @@ -27,43 +27,25 @@ from .base import ( _meta_cols_types_jsonb_idx, column_type_sql, + index_modifier_sql, jsonb_idx, + validate_constraint_definition, + validate_index_definition, + validate_index_predicate, + validate_relation_name, +) +# The index vocabulary lives in base.py, next to the validators that check +# definitions against it; these names are re-exported for callers that have +# always imported them from here. +from .base import ( + MAX_IDENTIFIER_LENGTH, + _operator_classes, + _valid_storage_params, + _valid_constraint_types, ) from .statstable import PostgresStatsTable - -# the non-default operator classes, used in creating indexes -_operator_classes = { - "brin": ["inet_minmax_ops"], - "btree": [ - "bpchar_pattern_ops", - "cidr_ops", - "record_image_ops", - "text_pattern_ops", - "varchar_ops", - "varchar_pattern_ops", - ], - "gin": ["jsonb_path_ops", "array_ops"], - "gist": ["inet_ops"], - "hash": [ - "bpchar_pattern_ops", - "cidr_ops", - "text_pattern_ops", - "varchar_ops", - "varchar_pattern_ops", - ], - "spgist": ["kd_point_ops"], -} - -# Valid storage parameters by type, used in creating indexes -_valid_storage_params = { - "brin": ["pages_per_range", "autosummarize"], - "btree": ["fillfactor"], - "gin": ["fastupdate", "gin_pending_list_limit"], - "gist": ["fillfactor", "buffering"], - "hash": ["fillfactor"], - "spgist": ["fillfactor"], -} +assert _operator_classes and _valid_storage_params ################################################################## @@ -348,17 +330,26 @@ def _get_tablespace(self): def _create_index_statement(self, name, table, type, columns, modifiers, storage_params, whereclause=None): """ Utility function for making the create index SQL statement. - """ - # We whitelisted the type, modifiers and storage parameters - # when creating the index so the following is safe from SQL injection - if storage_params: - # The keys of storage_params have been whitelisted; the values are - # inlined as literals because DDL statements cannot take bound - # parameters under psycopg3's server-side binding. + + The definition is validated here rather than only where it was created, + because it may have reached this point out of ``meta_indexes``, a + metadata file or the history table since then. Everything the + statement is built from is either quoted as an identifier or an ``SQL`` + constant selected by a validated key, so validation is not the only + thing standing between a metadata row and the statement. + """ + validate_relation_name(name, "Index") + definition = validate_index_definition( + name, table, type, columns, modifiers, storage_params, whereclause, + valid_columns=self._relation_columns(table), + ) + if definition.storage_params: + # Values are inlined as literals because DDL statements cannot take + # bound parameters under psycopg3's server-side binding. storage_params = SQL(" WITH ({0})").format( SQL(", ").join( - SQL("{0} = {{0}}".format(param)).format(Literal(val)) - for param, val in storage_params.items() + SQL("{0} = {1}").format(Identifier(param), Literal(val)) + for param, val in definition.storage_params.items() ) ) else: @@ -366,21 +357,26 @@ def _create_index_statement(self, name, table, type, columns, modifiers, storage tablespace = self._tablespace_clause() # A partial index restricts the rows it covers to those matching a # predicate. The clause is raw SQL supplied by an administrator (see - # create_index), so it is inlined directly, like the whitelisted type - # and modifiers above; it must come last, after WITH and TABLESPACE. - if whereclause: - where = SQL(" WHERE " + whereclause) + # create_index); validate_index_predicate has checked that it cannot + # end the statement. It must come last, after WITH and TABLESPACE. + if definition.whereclause: + where = SQL(" WHERE ") + SQL(definition.whereclause) else: where = SQL("") - modifiers = [" " + " ".join(mods) if mods else "" for mods in modifiers] - # The inner % operator is on strings prior to being wrapped by SQL: modifiers have been whitelisted. columns = SQL(", ").join( - SQL("{0}%s" % mods).format(Identifier(col)) - for col, mods in zip(columns, modifiers) + SQL(" ").join([Identifier(col)] + [index_modifier_sql(mod, definition.access_method) for mod in mods]) + for col, mods in zip(definition.columns, definition.modifiers) + ) + creator = SQL("CREATE INDEX {0} ON {1} USING {2} ({3}){4}{5}{6}") + return creator.format( + Identifier(definition.name), + Identifier(definition.table), + Identifier(definition.access_method), + columns, + storage_params, + tablespace, + where, ) - # The inner % operator is on strings prior to being wrapped by SQL: type has been whitelisted. - creator = SQL("CREATE INDEX {0} ON {1} USING %s ({2}){3}{4}{5}" % (type)) - return creator.format(Identifier(name), Identifier(table), columns, storage_params, tablespace, where) def _create_counts_indexes(self, suffix="", warning_only=False): """ @@ -494,6 +490,10 @@ def create_index(self, columns, type="btree", modifiers=None, name=None, storage now = time.time() if type not in _operator_classes: raise ValueError("Unrecognized index type") + if where is not None: + # Checked here as well as in _create_index_statement so that an + # unusable predicate is refused before a name is generated for it. + where = validate_index_predicate(where) if where is not None and self._db._meta_format < 1: raise ValueError( "Partial indexes need metadata format 1, but this database " @@ -514,29 +514,22 @@ def mod(col): modifiers = [mod(col) for col in columns] else: modifiers = [[]] * len(columns) - else: - if len(modifiers) != len(columns): - raise ValueError("modifiers must have same length as columns") - for mods in modifiers: - for mod in mods: - if ( - mod.lower() - not in ["asc", "desc", "nulls first", "nulls last"] - + _operator_classes[type] - ): - raise ValueError("Invalid modifier %s" % (mod,)) if storage_params is None: if type in ["btree", "hash", "gist", "spgist"]: storage_params = {"fillfactor": 100} else: storage_params = {} - else: - for key in storage_params: - if key not in _valid_storage_params[type]: - raise ValueError("Invalid storage parameter %s" % key) - for col in columns: - if col != "id" and col not in self.search_cols: - raise ValueError("%s not a column" % (col)) + # The definition is checked in full here, before a name is generated + # and before anything is written to meta_indexes; _create_index_statement + # checks it again when it builds the DDL. + definition = validate_index_definition( + name, self.search_table, type, columns, modifiers, storage_params, + where, valid_columns=set(self.search_cols) | {"id"}, + ) + # meta_indexes keeps the modifiers as the caller spelled them; the + # canonical spellings the validator returns are what the DDL is built + # from, here and on every later restore. + columns = definition.columns if name is None: # Postgres has a maximum name length of 64 bytes # It will truncate if longer, but that causes suffixes of _tmp to be indistinguishable. @@ -775,24 +768,37 @@ def list_constraints(self, verbose=False): if not verbose: return output - @staticmethod - def _create_constraint_statement(name, table, type, columns, check_func): + def _create_constraint_statement(self, name, table, type, columns, check_func): """ Utility function for making the create constraint SQL statement. + + Like ``_create_index_statement``, this validates the definition it is + given: a constraint is rebuilt from ``meta_constraints`` long after + ``create_constraint`` checked it, and the row may have been imported or + edited in between. The check function is quoted as an identifier + rather than formatted into the statement. """ - # We whitelisted the type and check function so the following is safe - cols = SQL(", ").join(Identifier(col) for col in columns) - # from SQL injection - if type == "NOT NULL": - return SQL("ALTER TABLE {0} ALTER COLUMN {1} SET NOT NULL").format(Identifier(table), cols) - elif type == "UNIQUE": + definition = validate_constraint_definition( + name, table, type, columns, check_func, + valid_columns=self._relation_columns(table), + valid_check_functions=self._valid_check_functions, + ) + cols = SQL(", ").join(Identifier(col) for col in definition.columns) + if definition.constraint_type == "NOT NULL": + return SQL("ALTER TABLE {0} ALTER COLUMN {1} SET NOT NULL").format( + Identifier(definition.table), cols + ) + elif definition.constraint_type == "UNIQUE": return SQL( "ALTER TABLE {0} ADD CONSTRAINT {1} UNIQUE ({2}) WITH (fillfactor=100)" - ).format(Identifier(table), Identifier(name), cols) - elif type == "CHECK": - return SQL( - "ALTER TABLE {0} ADD CONSTRAINT {1} CHECK (%s({2}))" % check_func - ).format(Identifier(table), Identifier(name), cols) + ).format(Identifier(definition.table), Identifier(definition.name), cols) + else: + return SQL("ALTER TABLE {0} ADD CONSTRAINT {1} CHECK ({2}({3}))").format( + Identifier(definition.table), + Identifier(definition.name), + Identifier(definition.check_func), + cols, + ) @staticmethod def _drop_constraint_statement(name, table, type, columns): @@ -808,8 +814,13 @@ def _drop_constraint_statement(name, table, type, columns): Identifier(table), Identifier(name) ) - _valid_constraint_types = ["UNIQUE", "CHECK", "NOT NULL"] - _valid_check_functions = [] # defined in utils.psql + # The constraint types psycodict creates, and the functions a CHECK + # constraint may call. A check function is emitted as a quoted identifier, + # but it also has to be one an administrator has approved here: it is a + # call psycodict makes on the table's behalf, not something a metadata row + # gets to choose. + _valid_constraint_types = list(_valid_constraint_types) + _valid_check_functions = [] # e.g. those defined in LMFDB's utils.psql def create_constraint(self, columns, type, name=None, check_func=None): """ @@ -829,23 +840,21 @@ def create_constraint(self, columns, type, name=None, check_func=None): to prevent SQL injection attacks """ now = time.time() + if not isinstance(type, str): + raise ValueError("Unrecognized constraint type") type = type.upper() if isinstance(columns, str): columns = [columns] - if type not in self._valid_constraint_types: - raise ValueError("Unrecognized constraint type") - if check_func is not None and check_func not in self._valid_check_functions: - # If the following line fails, add the desired function to the list defined above - raise ValueError("%s not in list of approved check functions (edit db_backend to add)") - if (check_func is None) == (type == "CHECK"): - raise ValueError("check_func should specified just for CHECK constraints") - if type == "NOT NULL" and len(columns) != 1: - raise ValueError("NOT NULL only supports one column") - if all(col == "id" for col in columns): + if not isinstance(columns, (list, tuple)) or all(col == "id" for col in columns): raise ValueError("Must specify non-id columns") - for col in columns: - if col != "id" and col not in self.search_cols: - raise ValueError("%s not a column" % (col)) + # Checked in full here, before anything is written to meta_constraints, + # and again in _create_constraint_statement when the definition becomes + # DDL. The name is checked there: it may still be generated below. + validate_constraint_definition( + None, self.search_table, type, columns, check_func, + valid_columns=set(self.search_cols) | {"id"}, + valid_check_functions=self._valid_check_functions, + ) if name is None: # Postgres has a maximum name length of 64 bytes # It will truncate if longer, but that causes suffixes of _tmp to be indistinguishable. @@ -856,10 +865,17 @@ def create_constraint(self, columns, type, name=None, check_func=None): else: name = "_".join([self.search_table] + ["c"] + ["".join(col[0] for col in columns)]) + # A name PostgreSQL would truncate is refused here rather than created: + # meta_constraints would keep the full name, and every later drop or + # restore would look for a constraint that is not called that. Only + # names being created are held to this; a name already recorded is not, + # since psycodict appends _tmp and _oldN to it. + validate_relation_name(name, "Constraint", max_length=MAX_IDENTIFIER_LENGTH) + with DelayCommit(self, silence=True): self._check_index_name(name, "Constraint") # also works for constraints table = self.search_table - creator = self._create_constraint_statement(name, table, type, columns, check_func) + creator = self._create_constraint_statement(name, table, type, list(columns), check_func) self._execute(creator) inserter = SQL( "INSERT INTO meta_constraints " diff --git a/tests/test_security.py b/tests/test_security.py index 2d6970d..f2b2827 100644 --- a/tests/test_security.py +++ b/tests/test_security.py @@ -20,13 +20,16 @@ import pytest -from psycopg.sql import SQL +from psycopg.sql import SQL, Identifier from psycodict.base import ( InvalidColumnTypeError, + InvalidDefinitionError, types_whitelist, validate_column_type, + validate_index_predicate, ) +from psycodict.encoding import Json import conftest @@ -352,3 +355,347 @@ def test_reload_all_with_adjust_schema_rejects_an_injected_header_type(db, fille # and the table the folder was exported from is untouched assert db[old_name].count() == 200 assert conftest # the fixtures above come from conftest + + +################################################################## +# index and constraint definitions out of meta_* # +################################################################## + + +def meta_index_rows(db, table): + """ + This table's rows of meta_indexes, as dictionaries. + """ + cur = db._execute( + SQL( + "SELECT index_name, type, columns, modifiers, storage_params, whereclause " + "FROM meta_indexes WHERE table_name = %s ORDER BY index_name" + ), + [table], + ) + keys = ["index_name", "type", "columns", "modifiers", "storage_params", "whereclause"] + return [dict(zip(keys, row)) for row in cur] + + +def poison_meta_index(db, table, target, **columns): + """ + Edit a meta_indexes row in place, the way a direct SQL edit would. + """ + assignments = SQL(", ").join( + SQL("{0} = %s").format(Identifier(col)) for col in columns + ) + db._execute( + SQL("UPDATE meta_indexes SET {0} WHERE table_name = %s AND index_name = %s").format( + assignments + ), + [Json(val) if col in ("columns", "modifiers", "storage_params") else val + for col, val in columns.items()] + [table, target], + ) + + +def indexes_file_with(tmp_path, table, **replacements): + """ + Export ``table``'s indexes and rewrite fields of the exported row. + + Returns the path of the modified file. + """ + path = tmp_path / "indexes.txt" + table.copy_to_indexes(str(path)) + text = path.read_text() + for old, new in replacements.items(): + text = text.replace(old, new) + path.write_text(text) + return str(path) + + +def test_reload_indexes_rejects_a_predicate_carrying_a_statement(db, empty_table, tmp_path): + """ + A partial index predicate is raw SQL by design, and it is appended to + CREATE INDEX; a metadata file must not be able to use it to append a + statement of its own. + """ + marker = marker_name() + empty_table.create_index(["n"], where="n > 0") + name = empty_table.search_table + "_n" + before = meta_index_rows(db, empty_table.search_table) + built_before = set(empty_table._list_built_indexes()) + + path = indexes_file_with( + tmp_path, empty_table, + **{"n > 0": "n > 0; CREATE TABLE %s (x integer); --" % marker} + ) + with pytest.raises(ValueError): + empty_table.reload_indexes(path) + + assert not table_exists(db, marker) + # The rejected import rolled back: the old definitions are still there ... + assert meta_index_rows(db, empty_table.search_table) == before + # ... and the built index was neither dropped nor replaced. + assert set(empty_table._list_built_indexes()) == built_before + assert "WHERE (n > 0)" in db._execute( + SQL("SELECT indexdef FROM pg_indexes WHERE indexname = %s"), [name] + ).fetchone()[0] + + +@pytest.mark.parametrize( + "replacement", + [ + # unknown access method + {"btree": "btree; CREATE TABLE x (i int); --"}, + {"btree": "nosuchmethod"}, + # a modifier carrying SQL + {'[[]]': '[["DESC) ; CREATE TABLE x (i int); --"]]'}, + # a modifier that is valid for another access method + {'[[]]': '[["jsonb_path_ops"]]'}, + # more modifier lists than columns + {'[[]]': '[[], []]'}, + # unknown storage parameter + {'{"fillfactor": 100}': '{"nosuchparam": 100}'}, + # a storage parameter value out of range, and one of the wrong type + {'{"fillfactor": 100}': '{"fillfactor": 1000}'}, + {'{"fillfactor": 100}': '{"fillfactor": "100); CREATE TABLE x (i int); --"}'}, + # malformed columns + {'["n"]': '"n"'}, + {'["n"]': '[]'}, + ], +) +def test_reload_indexes_rejects_invalid_definitions(db, empty_table, tmp_path, replacement): + empty_table.create_index(["n"]) + before = meta_index_rows(db, empty_table.search_table) + path = indexes_file_with(tmp_path, empty_table, **replacement) + with pytest.raises(ValueError): + empty_table.reload_indexes(path) + assert meta_index_rows(db, empty_table.search_table) == before + + +def test_a_column_name_carrying_sql_cannot_build_a_statement(db, empty_table, tmp_path): + """ + Column names are quoted wherever they are used, and any string can be a + real column name (the LMFDB has 2adic_index), so a name carrying SQL is not + refused for its shape -- it is refused when the definition is turned into + DDL, because it is not a column of the table. + """ + marker = marker_name() + empty_table.create_index(["n"]) + name = empty_table.search_table + "_n" + poison = 'n"); CREATE TABLE %s (x integer); --' % marker + poison_meta_index(db, empty_table.search_table, name, columns=[poison]) + empty_table.drop_index(name, permanent=False) + with pytest.raises(InvalidDefinitionError): + empty_table.restore_index(name) + assert not table_exists(db, marker) + assert name not in empty_table._list_built_indexes() + + +def test_reload_constraints_rejects_invalid_definitions(db, empty_table, tmp_path): + empty_table.create_constraint(["label"], "unique") + path = tmp_path / "constraints.txt" + empty_table.copy_to_constraints(str(path)) + original = path.read_text() + + for old, new in [ + ("UNIQUE", "UNIQUE; CREATE TABLE x (i int); --"), + ("UNIQUE", "FOREIGN KEY"), + # a check function on a constraint that is not a CHECK + ('["label"]|', '["label"]|nosuchfunc'), + ]: + path.write_text(original.replace(old, new, 1)) + with pytest.raises(ValueError): + empty_table.reload_constraints(str(path)) + # the constraint is still recorded and still enforced + assert list(empty_table.list_constraints()) == [empty_table.search_table + "_c_label"] + + +def test_restore_index_rejects_metadata_poisoned_in_place(db, empty_table): + """ + A row can be edited with plain SQL after create_index validated it, so the + check has to happen again when the row is turned back into DDL. + """ + marker = marker_name() + empty_table.create_index(["n"]) + name = empty_table.search_table + "_n" + empty_table.drop_index(name, permanent=False) + + poison_meta_index( + db, empty_table.search_table, name, + whereclause="n > 0; CREATE TABLE %s (x integer); --" % marker, + ) + with pytest.raises(ValueError): + empty_table.restore_index(name) + assert not table_exists(db, marker) + assert name not in empty_table._list_built_indexes() + + +@pytest.mark.parametrize( + "poison", + [ + {"type": "btree; CREATE TABLE x (i int); --"}, + {"modifiers": [["DESC); CREATE TABLE x (i int); --"]]}, + {"storage_params": {"fillfactor); CREATE TABLE x (i int); --": 100}}, + {"columns": ["nosuchcolumn"]}, + {"columns": "n"}, + {"index_name": "n; CREATE TABLE x (i int); --"}, + ], +) +def test_restore_index_rejects_poisoned_fields(db, empty_table, poison): + empty_table.create_index(["n"]) + name = empty_table.search_table + "_n" + empty_table.drop_index(name, permanent=False) + poison_meta_index(db, empty_table.search_table, name, **poison) + lookup = poison.get("index_name", name) + with pytest.raises(ValueError): + empty_table.restore_index(lookup) + assert lookup not in empty_table._list_built_indexes() + + +def test_restore_constraint_rejects_an_unapproved_check_function(db, empty_table): + empty_table.create_constraint(["label"], "unique") + name = empty_table.search_table + "_c_label" + empty_table.drop_constraint(name) + db._execute( + SQL( + "UPDATE meta_constraints SET type = %s, check_func = %s " + "WHERE table_name = %s AND constraint_name = %s" + ), + ["CHECK", "nosuchfunc", empty_table.search_table, name], + ) + with pytest.raises(ValueError): + empty_table.restore_constraint(name) + assert name not in empty_table._list_built_constraints() + + +def test_revert_indexes_rejects_poisoned_history(db, empty_table, tmp_path): + """ + The history table is as editable as any other, so a revert validates too. + """ + marker = marker_name() + empty_table.create_index(["n"], where="n > 0") + path = tmp_path / "indexes.txt" + empty_table.copy_to_indexes(str(path)) + # two versions, so that there is something to revert to + empty_table.reload_indexes(str(path)) + empty_table.reload_indexes(str(path)) + + db._execute( + SQL( + "UPDATE meta_indexes_hist SET whereclause = %s " + "WHERE table_name = %s AND version = 0" + ), + ["n > 0; CREATE TABLE %s (x integer); --" % marker, empty_table.search_table], + ) + before = meta_index_rows(db, empty_table.search_table) + with pytest.raises(ValueError): + empty_table.revert_indexes(version=0) + assert not table_exists(db, marker) + assert meta_index_rows(db, empty_table.search_table) == before + + +################################################################## +# the definitions psycodict does accept still work # +################################################################## + + +def test_create_index_still_accepts_every_supported_shape(db, empty_table): + empty_table.create_index(["n"]) + empty_table.create_index(["label"], modifiers=[["text_pattern_ops"]]) + empty_table.create_index(["n", "label"], modifiers=[["DESC"], ["nulls first"]]) + empty_table.create_index(["data"], type="gin") + empty_table.create_index(["x"], storage_params={"fillfactor": 90}) + empty_table.create_index(["num"], where="num > 0") + recorded = empty_table.list_indexes() + built = set(empty_table._list_built_indexes()) + assert len(recorded) == 6 + assert set(recorded) <= built + + +def test_a_partial_index_survives_export_and_restore(db, empty_table, tmp_path): + empty_table.create_index(["n"], where="n > 0 AND n < 100") + name = empty_table.search_table + "_n" + path = str(tmp_path / "indexes.txt") + empty_table.copy_to_indexes(path) + empty_table.drop_index(name) + empty_table.reload_indexes(path) + empty_table.restore_index(name) + indexdef = db._execute( + SQL("SELECT indexdef FROM pg_indexes WHERE indexname = %s"), [name] + ).fetchone()[0] + assert "WHERE" in indexdef and "n > 0" in indexdef + + +def test_predicates_that_are_merely_unusual_are_still_accepted(): + for predicate in [ + "n > 0", + "n IS NOT NULL", + "label LIKE 'a%'", + "(n > 0 AND label IS NOT NULL) OR num = 3", + "n > 0\n AND num < 10", + ]: + assert validate_index_predicate(predicate) == predicate.strip() + + +@pytest.mark.parametrize( + "predicate", + [ + "n > 0; DROP TABLE anything", + "n > 0 -- comment", + "n > 0 /* comment */", + "n > 0 $$ x $$", + "n > 0 $tag$ x $tag$", + "n > 0\x00", + "", + " ", + "x" * 5000, + ], +) +def test_predicates_that_could_end_the_statement_are_rejected(predicate): + with pytest.raises(InvalidDefinitionError): + validate_index_predicate(predicate) + + +def test_a_long_index_name_still_survives_a_reload(db, empty_table): + """ + psycodict makes the names it puts in DDL by appending _tmp or _oldN to one + it already has, and create_index generates names right up to PostgreSQL's + 63-byte limit. Holding the suffixed name to that limit would fail every + reload of a table with a long index name. + """ + name = ("i_%s" % uuid.uuid4().hex) * 2 + name = name[:63] + empty_table.create_index(["n"], name=name) + empty_table.drop_index(name, permanent=False) + empty_table.restore_index(name, suffix="") + assert name in empty_table._list_built_indexes() + + +def test_an_index_on_a_column_named_like_a_number_is_restorable(db, table_factory): + """ + A column that exists is a column whatever it is called: the LMFDB has one + named 2adic_index, which is not an identifier psycodict could have created + unquoted. + """ + table = table_factory( + columns=[("2adic_index", "integer"), ("label", "text")], sort=["label"] + ) + table.create_index(["2adic_index"], name="idx_%s" % uuid.uuid4().hex[:8]) + name = list(table.list_indexes())[0] + table.drop_index(name, permanent=False) + table.restore_index(name) + assert name in table._list_built_indexes() + + +@pytest.mark.parametrize( + "typ", + [ + 'text COLLATE "C.UTF-8"', + 'text COLLATE "und-x-icu"', + 'varchar(5) COLLATE "de_DE@euro"', + 'text[] COLLATE "C"', + ], +) +def test_collations_psycodict_does_not_get_to_veto(typ): + """ + Which collations exist is the server's business; the grammar's business is + that the name cannot end the quoted string early. + """ + spelling, _ = validate_column_type(typ) + assert spelling == typ From 10e7b36ae1cf3207fa6c353d1dcd385b0823ac87 Mon Sep 17 00:00:00 2001 From: David Roe Date: Mon, 3 Aug 2026 03:20:45 -0400 Subject: [PATCH 3/5] Rebuild the same connection on a reset, not a weaker one 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 --- CHANGELOG.md | 20 +++ psycodict/database.py | 298 +++++++++++++++++++++++++++++----------- tests/test_reconnect.py | 223 ++++++++++++++++++++++++++++++ 3 files changed, 463 insertions(+), 78 deletions(-) create mode 100644 tests/test_reconnect.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 6cd46d4..73c00b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -205,6 +205,26 @@ hardening standalone use; the highlights: or control characters. The unused, unvalidated `_copy_from_meta` helper is gone. +- **A reconnect reproduces the connection it replaces.** `reset_connection` + rebuilt the connection from the configuration alone, dropping the overrides + passed to `PostgresDatabase(...)` — TLS mode and certificates, an alternate + host, port, database or user, libpq `options`, connect and keepalive + timeouts. A connection that dropped could come back weaker than the one it + replaced, or somewhere else entirely. Options now come from one place + (`_connection_options`), used for the first connection and every replacement; + session setup moved into `_configure_session`, so the statement timeout and + friends are reapplied rather than lost with the session they were set on; and + a replacement is checked to have reached the same database as the same role, + with the read-only, superuser, knowls and userdb capability flags recomputed + from it instead of carried over. Retry behavior is unchanged: a standalone + statement is retried once, and a statement inside `DelayCommit` still raises + rather than replaying a transaction whose earlier statements are gone. +- **`PostgresDatabase(session_settings=...)`** applies `statement_timeout`, + `lock_timeout`, `idle_in_transaction_session_timeout` or `application_name` + to every connection the database opens. The 25 second statement timeout for + the `webserver` role is now the default value of this setting rather than a + one-off `SET` in the constructor. + ### Infrastructure - A test suite of 500+ tests and a continuous-integration workflow covering the diff --git a/psycodict/database.py b/psycodict/database.py index 41842d0..3fb8872 100644 --- a/psycodict/database.py +++ b/psycodict/database.py @@ -159,7 +159,16 @@ class PostgresDatabase(PostgresBase): MetadataFormats.md). Without it, a database using an older but compatible metadata format connects with a warning and operates at the older format. - - ``**kwargs`` -- passed on to psycopg's connect method + - ``session_settings`` -- a dictionary of PostgreSQL session settings + (``statement_timeout``, ``lock_timeout``, + ``idle_in_transaction_session_timeout``, ``application_name``) applied to + every connection this database opens, including replacements made after + a connection drops. Defaults to a 25 second statement timeout when + connecting as ``webserver``, and to nothing otherwise. + - ``**kwargs`` -- passed on to psycopg's connect method. These override the + configured options for every connection this database opens, so TLS + settings, an alternate host or database, and timeouts given here survive + a reconnect. ATTRIBUTES: @@ -191,38 +200,227 @@ class PostgresDatabase(PostgresBase): # Override the following to use a different class for search tables _search_table_class_ = PostgresSearchTable - def _new_connection(self, **kwargs): + # The session settings psycodict will apply, and what it applies to a + # webserver connection when the caller asks for nothing else. Settings are + # applied with set_config, which takes the name and the value as bound + # parameters; the closed set here is what keeps a caller from setting + # something unrelated through the same door. + _allowed_session_settings = ( + "statement_timeout", + "lock_timeout", + "idle_in_transaction_session_timeout", + "application_name", + ) + _webserver_session_settings = {"statement_timeout": "25s"} + + def _resolve_session_settings(self, session_settings): """ - Create a new connection to the postgres database. + The session settings to apply to every connection of this database. + + INPUT: + + - ``session_settings`` -- a dictionary of PostgreSQL settings, or None + to take the default for the connecting role + + A connection as ``webserver`` has always been given a 25 second + statement timeout; that is now the default rather than a special case + applied once, so it survives a reconnect and can be overridden. + """ + if session_settings is None: + user = self._connection_options().get("user") + session_settings = ( + self._webserver_session_settings if user == "webserver" else {} + ) + unknown = set(session_settings) - set(self._allowed_session_settings) + if unknown: + raise ValueError( + "psycodict will not set the session setting(s) %s; it applies " + "%s" % (", ".join(sorted(unknown)), + ", ".join(self._allowed_session_settings)) + ) + return dict(session_settings) + + def _connection_options(self): + """ + The options every connection this database opens is made with. + + The configured ``postgresql`` options, overridden by the keyword + arguments the constructor was given. There is one of these rather than + a ``_new_connection(**kwargs)`` because the overrides carry the + connection's security: TLS mode and certificates, the host and database + actually connected to, libpq ``options``, timeouts. A reconnect that + rebuilt the options without them would come back on a connection weaker + than the one it replaced -- or to a different server. """ options = dict(self.config.options["postgresql"]) - # overrides the options passed as keyword arguments - for key, value in kwargs.items(): - options[key] = value - self._user = options["user"] + options.update(self._connect_kwargs) + return options + + def _new_connection(self): + """ + Open a connection with this database's options, and configure it. + + Every connection the database makes -- the first one and every + replacement -- goes through here, so they are made alike. + """ + options = self._connection_options() logging.info( "Connecting to PostgresSQL server as: user=%s host=%s port=%s dbname=%s..." - % (options["user"], options["host"], options["port"], options["dbname"]) + % ( + options.get("user"), + options.get("host"), + options.get("port"), + options.get("dbname"), + ) ) connection = connect(**options) logging.info("Done!\n connection = %s" % connection) + self._configure_session(connection) + return connection + + def _configure_session(self, conn): + """ + Apply psycodict's per-session setup to a fresh connection. + + Everything a session needs beyond the connection itself: the type + adapters, and the session settings (statement, lock and + idle-in-transaction timeouts) that protect the server from this + connection. A replacement connection that skipped this would be a + connection without those protections. + + Runs directly on ``conn`` rather than through ``_execute``, because it + is called while recovering from a failure in ``_execute``. + """ # The following function controls how Python classes are converted to # strings for passing to Postgres, and how the results are decoded upon # extraction from the database. # Note that it has some global effects, since register_adapter # is not limited to just one connection - setup_connection(connection) - return connection + setup_connection(conn) + for name, value in self._session_settings.items(): + # set_config takes both as bound values, so nothing is interpolated + conn.execute("SELECT set_config(%s, %s, false)", [name, str(value)]) + conn.commit() + + def _detect_capabilities(self, conn): + """ + What this connection is allowed to do, read from the server. + + Sets ``_read_only``, ``_super_user``, ``_read_and_write_knowls`` and + ``_read_and_write_userdb``. Called for the initial connection and + again for every replacement: a reconnect can land on a standby, or as a + role with different grants, and the cached answers from the previous + session would then be wrong in the unsafe direction. + + Like ``_configure_session``, this runs directly on ``conn``. + """ + user = conn.info.user + + def query(sql, args): + return conn.execute(sql, args).fetchall() + + if query("SELECT pg_is_in_recovery()", [])[0][0]: + self._read_only = True + else: + # Check if there is a table where we can insert/update + privileges = ["INSERT", "UPDATE"] + rows = query( + "SELECT count(*) FROM information_schema.role_table_grants " + "WHERE grantee = %s AND table_schema = %s " + "AND privilege_type IN (" + ",".join(["%s"] * len(privileges)) + ")", + [user, "public"] + privileges, + ) + self._read_only = rows[0][0] == 0 + + self._super_user = query("SELECT current_setting('is_superuser')", [])[0][0] == "on" + + if self._read_only: + self._read_and_write_knowls = False + self._read_and_write_userdb = False + elif self._super_user: + self._read_and_write_knowls = True + self._read_and_write_userdb = True + else: + privileges = ["INSERT", "SELECT", "UPDATE"] + knowls_tables = ["kwl_knowls"] + rows = sorted(query( + "SELECT table_name, privilege_type " + "FROM information_schema.role_table_grants " + "WHERE grantee = %s AND table_name IN (" + + ",".join(["%s"] * len(knowls_tables)) + + ") AND privilege_type IN (" + + ",".join(["%s"] * len(privileges)) + + ")", + [user] + knowls_tables + privileges, + )) + self._read_and_write_knowls = rows == sorted( + [(table, priv) for table in knowls_tables for priv in privileges] + ) + + rows = sorted(query( + "SELECT privilege_type FROM information_schema.role_table_grants " + "WHERE grantee = %s AND table_schema = %s " + "AND table_name = %s AND privilege_type IN (" + + ",".join(["%s"] * len(privileges)) + + ")", + [user, "userdb", "users"] + privileges, + )) + self._read_and_write_userdb = rows == sorted([(priv,) for priv in privileges]) + conn.commit() + + logging.info("User: %s", user) + logging.info("Read only: %s", self._read_only) + logging.info("Super user: %s", self._super_user) + logging.info("Read/write to userdb: %s", self._read_and_write_userdb) + logging.info("Read/write to knowls: %s", self._read_and_write_knowls) + + def _verify_identity(self, conn): + """ + Check that a replacement connection reached the same place as the one + it replaces. + + The database and the authenticated role are what psycodict's cached + view of the schema, and every permission decision made from it, are + about. A reconnect that silently landed elsewhere -- a DNS change, a + connection-service file edited underneath a long-running process -- must + not be adopted; the host is deliberately not checked, since a failover + to a standby is a legitimate way for it to change. + """ + found = (conn.info.dbname, conn.info.user) + if found != self._identity: + raise RuntimeError( + "Refusing to reconnect: expected to reach database %s as %s, " + "but the new connection reached database %s as %s" + % (self._identity + found) + ) def reset_connection(self): """ - Resets the connection + Replace the connection, and everything that was set up on it. + + The replacement is made with the same options, configured with the same + session settings, and checked to have reached the same database as the + same role; only then does it replace the old one on this object and on + every table registered with it. Capability flags are recomputed from + it, since they describe the session and not the object. """ logging.info("Connection broken (status %s); resetting...", self.conn.closed) + old = self.conn conn = self._new_connection() + try: + self._verify_identity(conn) + self._detect_capabilities(conn) + except Exception: + conn.close() + raise # Note that self is the first entry in self._objects for obj in self._objects: obj.conn = conn + if old is not None and not old.closed: + try: + old.close() + except Exception: + logging.info("Could not close the replaced connection", exc_info=True) def _register_object(self, obj): """ @@ -231,7 +429,8 @@ def _register_object(self, obj): obj.conn = self.conn self._objects.append(obj) - def __init__(self, config=None, secretsfile=None, create=False, upgrade=False, **kwargs): + def __init__(self, config=None, secretsfile=None, create=False, upgrade=False, + session_settings=None, **kwargs): if config is None: from .config import Configuration config = Configuration() @@ -245,10 +444,12 @@ def __init__(self, config=None, secretsfile=None, create=False, upgrade=False, * # listener() opens (otherwise it could subscribe on a different server # or database than the sender writes to). self._connect_kwargs = dict(kwargs) - self.conn = self._new_connection(**kwargs) + self._session_settings = self._resolve_session_settings(session_settings) + self.conn = self._new_connection() + self._user = self.conn.info.user + # What a replacement connection has to reach to be adopted. + self._identity = (self.conn.info.dbname, self.conn.info.user) PostgresBase.__init__(self, "db_all", self) - if self._user == "webserver": - self._execute(SQL("SET SESSION statement_timeout = '25s'")) if create: # Create any missing metadata tables before the read-only detection @@ -262,69 +463,10 @@ def __init__(self, config=None, secretsfile=None, create=False, upgrade=False, * # format (and without the older-format warning) in one call. self.upgrade_metadata() - if self._execute(SQL("SELECT pg_is_in_recovery()")).fetchone()[0]: - self._read_only = True - else: - # Check if there is a table where we can insert/update - privileges = ["INSERT", "UPDATE"] - cur = self._execute( - SQL( - "SELECT count(*) FROM information_schema.role_table_grants " - + "WHERE grantee = %s AND table_schema = %s " - + "AND privilege_type IN (" - + ",".join(["%s"] * len(privileges)) - + ")" - ), - [self._user, "public"] + privileges, - ) - self._read_only = cur.fetchone()[0] == 0 - - self._super_user = (self._execute(SQL("SELECT current_setting('is_superuser')")).fetchone()[0] == "on") - - if self._read_only: - self._read_and_write_knowls = False - self._read_and_write_userdb = False - elif self._super_user and not self._read_only: - self._read_and_write_knowls = True - self._read_and_write_userdb = True - else: - privileges = ["INSERT", "SELECT", "UPDATE"] - knowls_tables = ["kwl_knowls"] - cur = sorted(self._execute( - SQL( - "SELECT table_name, privilege_type " - + "FROM information_schema.role_table_grants " - + "WHERE grantee = %s AND table_name IN (" - + ",".join(["%s"] * len(knowls_tables)) - + ") AND privilege_type IN (" - + ",".join(["%s"] * len(privileges)) - + ")" - ), - [self._user] + knowls_tables + privileges, - )) - # print cur - # print sorted([(table, priv) for table in knowls_tables for priv in privileges]) - self._read_and_write_knowls = cur == sorted( - [(table, priv) for table in knowls_tables for priv in privileges] - ) - - cur = sorted(self._execute( - SQL( - "SELECT privilege_type FROM information_schema.role_table_grants " - + "WHERE grantee = %s AND table_schema = %s " - + "AND table_name=%s AND privilege_type IN (" - + ",".join(["%s"] * len(privileges)) - + ")" - ), - [self._user, "userdb", "users"] + privileges, - )) - self._read_and_write_userdb = cur == sorted([(priv,) for priv in privileges]) - - logging.info("User: %s", self._user) - logging.info("Read only: %s", self._read_only) - logging.info("Super user: %s", self._super_user) - logging.info("Read/write to userdb: %s", self._read_and_write_userdb) - logging.info("Read/write to knowls: %s", self._read_and_write_knowls) + # After create/upgrade: the read-only detection below concludes + # read-only when it can see no table it may write to, which a database + # whose meta tables have just been bootstrapped now has. + self._detect_capabilities(self.conn) # Refuse to run against a database that still uses the removed # search/extras table split diff --git a/tests/test_reconnect.py b/tests/test_reconnect.py new file mode 100644 index 0000000..090161b --- /dev/null +++ b/tests/test_reconnect.py @@ -0,0 +1,223 @@ +# -*- coding: utf-8 -*- +""" +What a replacement connection has to be. + +psycodict replaces its connection when a statement fails on a dead one. The +replacement is not just any connection to the same server: it has to be made +with the same options (TLS, host, database, timeouts), carry the same session +settings, reach the same database as the same role, and have its capability +flags recomputed rather than inherited. These tests hold each of those, and +also that reconnecting does not quietly replay an interrupted transaction. + +They open their own ``PostgresDatabase`` objects rather than using the shared +``db`` fixture, since they close connections on purpose. +""" +import psycopg +import pytest + +from psycopg.sql import SQL + +import psycodict.database +from psycodict.database import PostgresDatabase +from psycodict.utils import DelayCommit + + +@pytest.fixture +def recorded_connect(monkeypatch): + """ + Record the options every ``psycopg.connect`` call is made with. + """ + calls = [] + real_connect = psycodict.database.connect + + def recording(**options): + calls.append(dict(options)) + return real_connect(**options) + + monkeypatch.setattr(psycodict.database, "connect", recording) + return calls + + +@pytest.fixture +def own_db(config): + """ + A database object the test may break, closed when it ends. + """ + created = [] + + def make(**kwargs): + database = PostgresDatabase(config=config, **kwargs) + created.append(database) + return database + + yield make + + for database in created: + if not database.conn.closed: + database.conn.close() + + +def setting(database, name): + return database._execute( + SQL("SELECT current_setting(%s)"), [name] + ).fetchone()[0] + + +################################################################## +# connection options # +################################################################## + + +def test_connection_options_carry_the_explicit_overrides(db, monkeypatch): + """ + The overrides passed to the constructor are part of every connection's + options, not just the first one's. + + Checked on the merge rather than by connecting, so that the security + options here need no server that offers TLS and no certificate on disk. + """ + overrides = { + "sslmode": "verify-full", + "sslrootcert": "/example/ca.pem", + "sslcert": "/example/client.pem", + "sslkey": "/example/client.key", + "connect_timeout": 7, + "options": "-c search_path=public", + "keepalives_idle": 60, + } + monkeypatch.setattr(db, "_connect_kwargs", overrides) + options = db._connection_options() + for key, value in overrides.items(): + assert options[key] == value + # and the configured options are still there underneath + assert options["dbname"] == db.conn.info.dbname + + +def test_a_reconnect_uses_the_same_options_as_the_first_connection(own_db, recorded_connect): + database = own_db( + connect_timeout=7, + options="-c search_path=public", + application_name="psycodict-reconnect-test", + ) + database.reset_connection() + + assert len(recorded_connect) == 2 + initial, replacement = recorded_connect + assert initial == replacement + assert initial["connect_timeout"] == 7 + assert initial["options"] == "-c search_path=public" + assert initial["application_name"] == "psycodict-reconnect-test" + + +def test_a_reconnect_after_a_dropped_connection_keeps_the_options(own_db, recorded_connect): + database = own_db(connect_timeout=7, application_name="psycodict-drop-test") + database.conn.close() + # a standalone statement: the connection is replaced and the statement retried + assert database._execute(SQL("SELECT 1")).fetchone()[0] == 1 + + assert len(recorded_connect) == 2 + assert recorded_connect[0] == recorded_connect[1] + + +################################################################## +# session settings # +################################################################## + + +def test_session_settings_are_applied_and_survive_a_reconnect(own_db): + database = own_db(session_settings={"statement_timeout": "7s", "lock_timeout": "3s"}) + assert setting(database, "statement_timeout") == "7s" + assert setting(database, "lock_timeout") == "3s" + + database.conn.close() + assert setting(database, "statement_timeout") == "7s" + assert setting(database, "lock_timeout") == "3s" + + +def test_a_webserver_connection_keeps_its_statement_timeout(own_db, monkeypatch): + """ + The 25 second timeout for the webserver role used to be set once, in the + constructor, and was lost with the connection it was set on. + """ + monkeypatch.setattr( + PostgresDatabase, + "_resolve_session_settings", + lambda self, given: dict(PostgresDatabase._webserver_session_settings), + ) + database = own_db() + assert setting(database, "statement_timeout") == "25s" + database.conn.close() + assert setting(database, "statement_timeout") == "25s" + + +def test_unknown_session_settings_are_refused(own_db): + with pytest.raises(ValueError, match="session setting"): + own_db(session_settings={"log_statement": "all"}) + + +################################################################## +# identity and capabilities # +################################################################## + + +def test_a_replacement_connection_reaching_elsewhere_is_refused(own_db): + database = own_db() + conn = database.conn + # what the object believes it is connected to no longer matches the server + database._identity = ("some_other_database", database.conn.info.user) + with pytest.raises(RuntimeError, match="Refusing to reconnect"): + database.reset_connection() + # the old connection was not replaced by the rejected one + assert database.conn is conn + + +def test_capabilities_are_recomputed_on_reconnect(own_db): + database = own_db() + assert not database._read_only + # a stale flag from the previous session must not be carried over + database._read_only = True + database._super_user = False + database.reset_connection() + assert not database._read_only + assert database._super_user == ( + database.conn.execute("SELECT current_setting('is_superuser')").fetchone()[0] == "on" + ) + + +################################################################## +# retry semantics # +################################################################## + + +def test_a_standalone_statement_is_retried_once(own_db, recorded_connect): + database = own_db() + database.conn.close() + assert database._execute(SQL("SELECT 1")).fetchone()[0] == 1 + # one replacement, not a loop of them + assert len(recorded_connect) == 2 + + +def test_a_transaction_is_not_silently_replayed(own_db): + """ + Inside DelayCommit the statements before the failure are gone with the + connection; retrying only the last one would commit a fragment of the + transaction as though the rest had succeeded. + """ + database = own_db() + with pytest.raises(psycopg.OperationalError): + with DelayCommit(database): + database._execute(SQL("SELECT 1")) + database.conn.close() + database._execute(SQL("SELECT 2")) + + +def test_a_server_that_stays_unreachable_does_not_loop(own_db, monkeypatch): + database = own_db() + database.conn.close() + + def dead(**options): + raise psycodict.database.DatabaseError("server is not there") + + monkeypatch.setattr(psycodict.database, "connect", dead) + with pytest.raises(psycopg.DatabaseError, match="server is not there"): + database._execute(SQL("SELECT 1")) From 789696b048b10d8addcb1f8b4b7686906a49d466 Mon Sep 17 00:00:00 2001 From: David Roe Date: Mon, 3 Aug 2026 03:29:48 -0400 Subject: [PATCH 4/5] Give search table names a grammar and table objects a registry 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 --- CHANGELOG.md | 22 ++++ DataManagement.md | 2 +- psycodict/base.py | 76 ++++++++++++++ psycodict/database.py | 158 ++++++++++++++++++++++------- psycodict/table.py | 2 +- psycodict/utils.py | 31 ++++++ tests/test_security.py | 221 +++++++++++++++++++++++++++++++++++++++++ 7 files changed, 473 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 73c00b3..49e0ed8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -225,6 +225,28 @@ hardening standalone use; the highlights: the `webserver` role is now the default value of this setting rather than a one-off `SET` in the constructor. +- **Search table names have one grammar, and table objects have their own + registry.** A table name is used as a PostgreSQL identifier, as the key the + table object is reached by, and as the stem of the files `copy_to` writes, + but nothing checked it against any of those uses. A name with 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 named `conn`, `config` or `tablenames` — from + `create_table` or from a `meta_tables` row edited with plain SQL — replaced + that attribute. Names are now checked by `validate_search_table_name` + (lowercase letters, digits and underscores, starting with a letter, at most + 63 bytes, not ending in a suffix psycodict appends to a table's own name, not + the name of something else on the database object) in `create_table`, + `create_table_like` and `rename_table`; names already in `meta_tables` are + held to the identifier rule alone, since an existing database is entitled to + a name that predates the convention; table objects live in `db.tables`; and + the files an export generates go through + `safe_child_path`, which resolves symlinks and refuses anything that is not + under the requested folder. *Migration:* `db.tablenames` is now a sorted + property derived from `db.tables` rather than a list to mutate, and + attribute access (`db.mytable`) is unchanged. `reload_all` and `rename_table` + raise `ValueError` where they used to `assert`. + ### Infrastructure - A test suite of 500+ tests and a continuous-integration workflow covering the diff --git a/DataManagement.md b/DataManagement.md index 653e630..06b1f5c 100644 --- a/DataManagement.md +++ b/DataManagement.md @@ -21,7 +21,7 @@ db.create_table( ) ``` - * **`name`** must contain an underscore. The call also creates the companion tables `name_counts` and `name_stats`, and writes one row to `meta_tables`. + * **`name`** must be lowercase letters, digits and underscores, start with a letter, be at most 63 bytes, and not end in one of the suffixes psycodict appends to a search table's own name (`_counts`, `_stats`, `_pkey`, `_tmp`, `_oldN`, `_depN`) or collide with an attribute of the database object. By convention it contains an underscore. The name is used as a PostgreSQL identifier, as the key the table object is reached by (`db.name`), and as the stem of the files `copy_to` writes. A name already in `meta_tables` is held to less than this — an existing database is entitled to a name that predates the convention, and the LMFDB has a search table called `hgcwa_per_group_stats` — but it must still be an identifier: a row edited to something with a path separator, a `..` component or whitespace in it makes `refresh_tables` fail rather than be acted on. The call also creates the companion tables `name_counts` and `name_stats`, and writes one row to `meta_tables`. * **`search_columns`** takes two forms: a dictionary whose keys are Postgres types and whose values are lists of column names (a bare string is allowed for a single column), or a list of `(column, type)` pairs. The two forms are interchangeable. * **the `id` column** is added automatically as a `bigint` primary key unless you already list one; use `id_type=` to choose a different integer type. Columns are physically laid out ordered by type (widest alignment first) for storage efficiency, so the on-disk column order is not your declaration order — but every file operation is header-driven, so this never matters to you. * **`label_col`** names the column used by `lookup`; it must be one of the search columns, or `None`. diff --git a/psycodict/base.py b/psycodict/base.py index 5d899d8..fdc188e 100644 --- a/psycodict/base.py +++ b/psycodict/base.py @@ -565,6 +565,82 @@ def validate_column_name(name): return name +# A search table's name is used in three namespaces at once: as a PostgreSQL +# identifier, as the key a table object is reached by on the database, and as +# the stem of the files an export writes. The grammar below is what all three +# can agree on -- and it is the LMFDB's existing convention. +_SEARCH_TABLE_NAME = re.compile(r"[a-z][a-z0-9_]*") + +# Suffixes psycodict appends to a search table's name to make the names of its +# companion relations and of the temporary and backup tables it swaps through. +# A search table called foo_counts would collide with the counts table of a +# search table called foo. +_RESERVED_TABLE_SUFFIXES = ("_counts", "_stats", "_pkey", "_tmp") +_RESERVED_TABLE_SUFFIX_PATTERNS = (r".*_old[0-9]+", r".*_dep[0-9]+") + + +def validate_search_table_name(name, reserved=(), strict=True): + """ + Check that ``name`` can be used as the name of a search table. + + INPUT: + + - ``name`` -- the proposed or recorded name + - ``reserved`` -- names that are taken for another purpose on the database + object (its attributes and methods), which a new table may not shadow + - ``strict`` -- whether to apply the conventions a *new* name must follow, + as opposed to the rules that keep an existing one safe to use + + OUTPUT: + + ``name`` itself. + + A search table's name is used in more places than a relation name: as an + identifier, as the key of a table object on the database, and as the stem + of the files ``copy_to`` generates. A name containing a path separator or + a ``..`` component would send an export outside the directory it was asked + for, so that much is checked of every name, however it arrives. + + The rest -- lowercase spelling, and staying clear of the suffixes psycodict + appends to a search table's own name -- is a convention for names psycodict + is being asked to create. It is not applied to a name a database already + has, since that database is a fact: the LMFDB, for one, has a search table + called ``hgcwa_per_group_stats``, and refusing to connect to a database on + account of a name that has worked for years would be a worse failure than + the one being prevented. + """ + validate_relation_name(name, "Search table") + if not strict: + return name + # A search table's name is one psycodict appends to -- _counts, _tmp, _oldN + # -- so a new one has to leave room, and a name PostgreSQL would truncate + # would not match the meta_tables row recording it. + validate_relation_name(name, "Search table", max_length=MAX_IDENTIFIER_LENGTH) + if not _SEARCH_TABLE_NAME.fullmatch(name): + raise InvalidDefinitionError( + "Search table name %r must be lowercase letters, digits and " + "underscores, starting with a letter" % (name,) + ) + for suffix in _RESERVED_TABLE_SUFFIXES: + if name.endswith(suffix): + raise InvalidDefinitionError( + "Search table name %r ends with %s, which psycodict appends to " + "a search table's own name" % (name, suffix) + ) + for pattern in _RESERVED_TABLE_SUFFIX_PATTERNS: + if re.fullmatch(pattern, name): + raise InvalidDefinitionError( + "Search table name %r ends with a suffix psycodict appends to " + "the tables it swaps through" % (name,) + ) + if name in reserved: + raise InvalidDefinitionError( + "Search table name %r is the name of something else on the " + "database object" % (name,) + ) + return name + + def validate_index_predicate(predicate): """ Check the predicate of a partial index. diff --git a/psycodict/database.py b/psycodict/database.py index 3fb8872..0ec5e21 100644 --- a/psycodict/database.py +++ b/psycodict/database.py @@ -36,12 +36,14 @@ from .base import ( PostgresBase, META_FORMAT, + InvalidDefinitionError, _meta_tables_cols, _meta_tables_defaults, _meta_cols_types_jsonb_idx, + validate_search_table_name, ) from .searchtable import PostgresSearchTable -from .utils import DelayCommit +from .utils import DelayCommit, safe_child_path # The registry of metadata-format migrations. Entry N describes the step # from format N-1 to format N; MetadataFormats.md has the checklist a new @@ -176,11 +178,16 @@ class PostgresDatabase(PostgresBase): - ``server_side_counter`` -- an integer tracking how many buffered connections have been created - ``conn`` -- the psycopg connection object - - ``tablenames`` -- a list of tablenames in the database, as strings + - ``tables`` -- the search tables, by name + - ``tablenames`` -- the names of the search tables, sorted - ``meta_format`` -- the metadata format this connection operates at (see MetadataFormats.md) - Also, each tablename will be stored as an attribute, so that db.ec_curvedata works for example. + Each table is also reachable as an attribute, so that db.ec_curvedata works + for example. The tables themselves live in ``db.tables``, not in the + instance dictionary: a table name comes out of meta_tables and must not be + able to take the place of the connection or of a method, so anything the + database object really has wins over a table of the same name. These table objects are snapshots: if another process later changes the schema (adding or dropping columns or tables), call ``refresh_tables`` to update them @@ -502,7 +509,11 @@ def __init__(self, config=None, secretsfile=None, create=False, upgrade=False, # sides have. self._meta_format = min(stored, META_FORMAT) - self.tablenames = [] + # The search tables, by name. A dedicated mapping rather than the + # instance dictionary, so that a table name -- which comes out of + # meta_tables and can therefore be anything a row holds -- cannot take + # the place of the connection, the configuration or a method. + self.tables = {} self.refresh_tables() def refresh_tables(self): @@ -550,18 +561,18 @@ def refresh_tables(self): current = set() for tabledata in cur: tablename = tabledata[0] + # meta_tables is data like any other: a row can be inserted with + # plain SQL, so the name is checked here and not only where + # create_table put it there. + self._validate_table_name(tablename, source="meta_tables") current.add(tablename) - if tablename in self.tablenames: - self.__dict__[tablename]._refresh(tabledata[1:], data_types) + if tablename in self.tables: + self.tables[tablename]._refresh(tabledata[1:], data_types) else: tabledata += (data_types,) - table = self._search_table_class_(self, *tabledata) - self.__dict__[tablename] = table - self.tablenames.append(tablename) - for tablename in [name for name in self.tablenames if name not in current]: - delattr(self, tablename) - self.tablenames.remove(tablename) - self.tablenames.sort() + self.tables[tablename] = self._search_table_class_(self, *tabledata) + for tablename in [name for name in self.tables if name not in current]: + del self.tables[tablename] def __repr__(self): return "Interface to Postgres database" @@ -692,15 +703,76 @@ def _is_alive(self): pass return False + @property + def tablenames(self): + """ + The names of the search tables, sorted. + """ + return sorted(self.tables) + + def _validate_table_name(self, name, source=None): + """ + Check a search table name. + + INPUT: + + - ``name`` -- the proposed or recorded name + - ``source`` -- where it came from, if not from a caller, which also + means the name is one this database already has + + A name psycodict is asked to create must follow the conventions in + full. A name read out of ``meta_tables`` is checked for the things + that would make it unsafe to use -- a path separator, a ``..`` + component, whitespace, anything that is not an identifier -- but not + for the conventions, which an existing database is entitled to have + broken long before this check existed. + """ + try: + return validate_search_table_name( + name, reserved=self._reserved_names(), strict=source is None + ) + except ValueError as err: + if source is None: + raise + raise InvalidDefinitionError("%s (from %s)" % (err, source)) + + def _reserved_names(self): + """ + The names a search table may not have, because the database object + already uses them for something else. + """ + return set(dir(type(self))) | set(self.__dict__) + def __getitem__(self, name): """ Accesses a PostgresSearchTable object by name. """ - if name in self.tablenames: - return getattr(self, name) - else: + try: + return self.tables[name] + except KeyError: raise ValueError("%s is not a search table" % name) + def __getattr__(self, name): + """ + Accesses a PostgresSearchTable object as an attribute. + + Table objects live in ``self.tables`` rather than in the instance + dictionary, so that a table can never take the place of the + connection, the configuration or a method. This is only reached when + ordinary attribute lookup has already failed, so anything the database + object really has wins over a table of the same name. + """ + try: + tables = self.__dict__["tables"] + except KeyError: + # before __init__ has made the registry; without this an attribute + # touched during construction would recurse + raise AttributeError(name) + try: + return tables[name] + except KeyError: + raise AttributeError(name) + def table_sizes(self): """ Returns a dictionary containing information on the sizes of the search tables. @@ -1221,7 +1293,12 @@ def create_table( INPUT: - - ``name`` -- the name of the table, which must include an underscore. See existing names for consistency. + - ``name`` -- the name of the table: lowercase letters, digits and + underscores, starting with a letter, at most 63 bytes, and not ending + in one of the suffixes psycodict appends to a search table's own name + (``_counts``, ``_stats``, ``_pkey``, ``_tmp``, ``_oldN``, ``_depN``). + By convention it includes an underscore; see existing names for + consistency. - ``search_columns`` -- either a dictionary whose keys are valid postgres types and whose values are lists of column names (or just a string if only one column has the specified type); or a list of pairs (col, type). @@ -1267,6 +1344,7 @@ def create_table( Anything else raises ``InvalidColumnTypeError`` before any statement runs. """ + self._validate_table_name(name) if name in self.tablenames: raise ValueError("%s already exists" % name) now = time.time() @@ -1379,9 +1457,7 @@ def create_table( new_table.restore_pkeys() new_table.description(table_description) new_table.column_description(description=col_description) - self.__dict__[name] = new_table - self.tablenames.append(name) - self.tablenames.sort() + self.tables[name] = new_table self._log_db_change( "create_table", tablename=name, @@ -1434,8 +1510,7 @@ def drop_table(self, name, force=False): for tbl in [name, name + "_counts", name + "_stats"]: self._execute(SQL("DROP TABLE {0}").format(Identifier(tbl))) print("Dropped {0}".format(tbl)) - self.tablenames.remove(name) - delattr(self, name) + del self.tables[name] self._notify_schema_change(name) # rides this transaction def rename_table(self, old_name, new_name): @@ -1447,8 +1522,11 @@ def rename_table(self, old_name, new_name): - ``old_name`` -- the current name of the table, as a string - ``new_name`` -- the new name of the table, as a string """ - assert old_name != new_name - assert new_name not in self.tablenames + if old_name == new_name: + raise ValueError("The new name is the same as the old one") + self._validate_table_name(new_name) + if new_name in self.tablenames: + raise ValueError("%s already exists" % new_name) with DelayCommit(self, silence=True): table = self[old_name] # first rename indexes and constraints @@ -1532,14 +1610,11 @@ def rename_table(self, old_name, new_name): [new_name], ).fetchone() table = self._search_table_class_(self, *tabledata) - self.__dict__[new_name] = table - # Also drop the old attribute (as drop_table does), so that + self.tables[new_name] = table + # Also drop the old name (as drop_table does), so that # db. does not keep handing out a table object whose # postgres table no longer exists. - self.__dict__.pop(old_name, None) - self.tablenames.append(new_name) - self.tablenames.remove(old_name) - self.tablenames.sort() + self.tables.pop(old_name, None) # A rename touches both names: the old one is gone, the new one # appeared. Announce both so a listener can drop the stale # metadata and pick up the new table (rides this transaction). @@ -1570,12 +1645,16 @@ def copy_to(self, search_tables, data_folder, fail_on_error=True, **kwds): for tablename in search_tables: if tablename in self.tablenames: table = self[tablename] - searchfile = data_folder / (tablename + ".txt") - statsfile = data_folder / (tablename + "_stats.txt") - countsfile = data_folder / (tablename + "_counts.txt") - indexesfile = data_folder / (tablename + "_indexes.txt") - constraintsfile = data_folder / (tablename + "_constraints.txt") - metafile = data_folder / (tablename + "_meta.txt") + # These names are generated from the table name rather than + # given by the caller, so they go through safe_child_path: + # whatever a name in meta_tables turns out to be, the files an + # export writes stay in the folder it was asked for. + searchfile = safe_child_path(data_folder, tablename + ".txt") + statsfile = safe_child_path(data_folder, tablename + "_stats.txt") + countsfile = safe_child_path(data_folder, tablename + "_counts.txt") + indexesfile = safe_child_path(data_folder, tablename + "_indexes.txt") + constraintsfile = safe_child_path(data_folder, tablename + "_constraints.txt") + metafile = safe_child_path(data_folder, tablename + "_meta.txt") table.copy_to( searchfile=searchfile, countsfile=countsfile, @@ -1680,7 +1759,12 @@ def reload_all( if len(rows) != 1: raise RuntimeError("Expected only one row in {0}") meta = dict(zip(_meta_tables_cols, rows[0])) - assert meta["name"] == tablename + if meta["name"] != tablename: + raise ValueError( + "The meta file %s describes the table %s, but its " + "name says %s" + % (metafile, meta["name"], tablename) + ) with search_table_file.open("r") as F: search_columns_pairs = self._read_header_lines(F, sep=sep) diff --git a/psycodict/table.py b/psycodict/table.py index f54bdbd..d39b337 100644 --- a/psycodict/table.py +++ b/psycodict/table.py @@ -2222,7 +2222,7 @@ def reload_final_swap(self, tables=None, metafile=None, ordered=False, sep="|"): [self.search_table], ).fetchone() table = self._db._search_table_class_(self._db, *tabledata) - self._db.__dict__[self.search_table] = table + self._db.tables[self.search_table] = table def drop_tmp(self): """ diff --git a/psycodict/utils.py b/psycodict/utils.py index a473c37..e2ed716 100644 --- a/psycodict/utils.py +++ b/psycodict/utils.py @@ -10,6 +10,7 @@ import sys import re from collections import defaultdict +from pathlib import Path from psycopg.sql import SQL, Identifier, Placeholder @@ -270,6 +271,36 @@ def reraise(exc_type, exc_value, exc_traceback=None): raise exc_value.with_traceback(exc_traceback) raise exc_value +def safe_child_path(root, filename): + """ + The path of ``filename`` inside ``root``, if it really is inside it. + + INPUT: + + - ``root`` -- a directory the caller has chosen + - ``filename`` -- a name generated by psycodict, typically from a table + name + + OUTPUT: + + The resolved path, or ``ValueError`` if it is not under ``root``. + + Used where psycodict builds a filename out of database content rather than + out of something the caller passed: an export names its files after the + tables it writes, and a table whose name contained a path separator or a + ``..`` component would otherwise send the export somewhere else. Both + paths are resolved first, so a symlink inside ``root`` that points out of + it does not count as being inside. + """ + root = Path(root).resolve() + target = (root / filename).resolve() + if not target.is_relative_to(root): + raise ValueError( + "%s is not a path inside %s" % (target, root) + ) + return target + + def range_formatter(x): """ Format a query-language range constraint for human display: diff --git a/tests/test_security.py b/tests/test_security.py index f2b2827..67cfca2 100644 --- a/tests/test_security.py +++ b/tests/test_security.py @@ -16,6 +16,7 @@ not partially created, and that the Python objects were not mutated -- an exception raised after the damage was done would satisfy the first check alone. """ +import os.path import uuid import pytest @@ -30,6 +31,7 @@ validate_index_predicate, ) from psycodict.encoding import Json +from psycodict.utils import safe_child_path import conftest @@ -699,3 +701,222 @@ def test_collations_psycodict_does_not_get_to_veto(typ): """ spelling, _ = validate_column_type(typ) assert spelling == typ +################################################################## +# search table names, and the files they generate # +################################################################## + + +BAD_TABLE_NAMES = [ + "/tmp/escape", + "../escape", + "foo/bar", + "foo\\bar", + ".", + "..", + "", + " tab", + "tab ", + "foo bar", + "foo;bar", + 'foo"bar', + "Foo_bar", # a search table name is lowercase + "1foo", # and starts with a letter + "foo\x00bar", + "tablе_x", # Cyrillic lookalike + # names psycodict itself makes out of a search table's name + "foo_tmp", + "foo_old1", + "foo_dep2", + "foo_counts", + "foo_stats", + "foo_pkey", + "foo_dep3", + # names the database object already uses + "conn", + "config", + "tables", + "tablenames", + "_execute", + "reset_connection", + # over PostgreSQL's identifier limit + "t" + "o" * 63, +] + + +@pytest.mark.parametrize("name", BAD_TABLE_NAMES) +def test_create_table_rejects_unusable_names(db, name): + with pytest.raises(ValueError): + db.create_table(name, [("n", "integer")], "n") + assert name not in db.tablenames + + +@pytest.mark.parametrize("name", BAD_TABLE_NAMES) +def test_rename_table_rejects_unusable_names(db, empty_table, name): + with pytest.raises(ValueError): + db.rename_table(empty_table.search_table, name) + # the table is still there under its own name + assert empty_table.search_table in db.tablenames + + +def test_core_attributes_survive_a_table_named_after_them(db, empty_table): + """ + A table object cannot take the place of the connection or a method. + + Table objects used to be stored in the database object's instance + dictionary, where a table called `conn` would have replaced the connection. + """ + conn, config = db.conn, db.config + db.tables["conn"] = empty_table + db.tables["_execute"] = empty_table + try: + assert db.conn is conn + assert db.config is config + assert callable(db._execute) + # the real attributes win; the table is still reachable by subscript + assert db["conn"] is empty_table + finally: + del db.tables["conn"] + del db.tables["_execute"] + + +def test_refresh_tables_rejects_a_poisoned_meta_tables_row(db, empty_table, tmp_path): + """ + meta_tables is a table like any other, and a name in it becomes an + identifier, an attribute name and the stem of an export's filenames. + """ + conn, config = db.conn, db.config + tablenames = db.tablenames + escape = str(tmp_path / "escaped") + db._execute( + SQL("UPDATE meta_tables SET name = %s WHERE name = %s"), + [escape, empty_table.search_table], + ) + try: + with pytest.raises(ValueError): + db.refresh_tables() + assert db.conn is conn + assert db.config is config + assert callable(db._execute) + assert not os.path.exists(escape) + finally: + db._execute( + SQL("UPDATE meta_tables SET name = %s WHERE name = %s"), + [empty_table.search_table, escape], + ) + db.refresh_tables() + assert db.tablenames == tablenames + + +def test_attribute_and_subscript_access_agree(db, empty_table): + name = empty_table.search_table + assert getattr(db, name) is db[name] + assert name in db.tablenames + + +def test_a_dropped_table_is_gone_from_both_kinds_of_access(db, table_factory): + table = table_factory() + name = table.search_table + assert getattr(db, name) is db[name] + db.drop_table(name, force=True) + assert name not in db.tablenames + assert not hasattr(db, name) + with pytest.raises(ValueError): + db[name] + + +def test_a_renamed_table_moves_in_both_kinds_of_access(db, table_factory, tmp_path): + table = table_factory() + old = table.search_table + new = "test_%s" % uuid.uuid4().hex[:12] + db.rename_table(old, new) + try: + assert new in db.tablenames + assert old not in db.tablenames + assert getattr(db, new) is db[new] + assert not hasattr(db, old) + finally: + db.drop_table(new, force=True) + + +def test_generated_export_paths_stay_in_the_export_folder(tmp_path): + root = tmp_path / "export" + root.mkdir() + assert safe_child_path(root, "table.txt") == (root / "table.txt").resolve() + for escape in ["../escape.txt", "/tmp/escape.txt", "a/../../escape.txt"]: + with pytest.raises(ValueError): + safe_child_path(root, escape) + + +def test_a_symlink_out_of_the_export_folder_is_not_inside_it(tmp_path): + root = tmp_path / "export" + root.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + (root / "link").symlink_to(outside) + with pytest.raises(ValueError): + safe_child_path(root, "link/escape.txt") + + +def test_copy_to_writes_only_inside_the_folder(db, filled_table, tmp_path): + folder = tmp_path / "data" + db.copy_to([filled_table.search_table], str(folder)) + written = list(folder.glob("*")) + assert written + for path in written: + assert path.resolve().parent == folder.resolve() + + +def test_a_name_an_existing_database_has_is_not_second_guessed(db, empty_table): + """ + The conventions apply to a name psycodict is asked to create, not to one a + database already has: the LMFDB has a search table called + hgcwa_per_group_stats, and refusing to connect over that would be a worse + failure than the one being prevented. + """ + name = empty_table.search_table + db._execute( + SQL("UPDATE meta_tables SET name = %s WHERE name = %s"), + [name + "_stats2", name], + ) + db._execute( + SQL("ALTER TABLE {0} RENAME TO {1}").format( + Identifier(name), Identifier(name + "_stats2") + ), + ) + try: + # renaming the row to something ending in _stats would be refused for a + # new table, but is read back without complaint + db.refresh_tables() + assert name + "_stats2" in db.tablenames + finally: + db._execute( + SQL("ALTER TABLE {0} RENAME TO {1}").format( + Identifier(name + "_stats2"), Identifier(name) + ), + ) + db._execute( + SQL("UPDATE meta_tables SET name = %s WHERE name = %s"), + [name, name + "_stats2"], + ) + db.refresh_tables() + + +def test_a_name_that_could_escape_the_export_folder_is_refused_anywhere(db, empty_table, tmp_path): + """ + What is checked of every name, however it arrives, is what would make it + unsafe to use. + """ + name = empty_table.search_table + for bad in [str(tmp_path / "escape"), "../escape", "foo/bar", "foo bar"]: + db._execute( + SQL("UPDATE meta_tables SET name = %s WHERE name = %s"), [bad, name] + ) + try: + with pytest.raises(ValueError): + db.refresh_tables() + finally: + db._execute( + SQL("UPDATE meta_tables SET name = %s WHERE name = %s"), [name, bad] + ) + db.refresh_tables() + assert name in db.tablenames From 0182800637e0fa6d1083eeaef7762a3d304e9d53 Mon Sep 17 00:00:00 2001 From: David Roe Date: Mon, 3 Aug 2026 03:37:29 -0400 Subject: [PATCH 5/5] Say who a new relation is readable by, instead of assuming 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 --- CHANGELOG.md | 27 ++++++ DataManagement.md | 13 +++ docs/api/grants.md | 5 + docs/api/index.md | 3 + psycodict/database.py | 150 ++++++++++++++++++++++++----- psycodict/grants.py | 187 +++++++++++++++++++++++++++++++++++++ psycodict/statstable.py | 2 +- psycodict/table.py | 23 ++++- tests/test_security.py | 202 ++++++++++++++++++++++++++++++++++++++++ 9 files changed, 585 insertions(+), 27 deletions(-) create mode 100644 docs/api/grants.md create mode 100644 psycodict/grants.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 49e0ed8..6d12c96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -247,6 +247,33 @@ hardening standalone use; the highlights: attribute access (`db.mytable`) is unchanged. `reload_all` and `rename_table` raise `ValueError` where they used to `assert`. +- **Table creation no longer grants privileges to hard-coded roles.** 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, so a new table holding anything at all was readable by two roles + the administrator may never have heard of. Privileges are now stated by a + `psycodict.grants.GrantPolicy`, passed as + `PostgresDatabase(grant_policy=...)` and applied through one helper wherever + psycodict creates or swaps a relation. *Migration:* the default policy grants + nothing; LMFDB and any other deployment that wants the old behavior passes + `LMFDBGrantPolicy()`. A policy that names a role the cluster does not have + raises by default (`LMFDBGrantPolicy` warns and skips, as before), and + `grant_select`, `grant_insert`, `grant_update` and `grant_delete` now require + their `users` argument instead of defaulting to those roles. +- **A reload warns when it takes access away.** A reload swaps in a clone, and + 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 psycodict + logs a warning naming the roles that lost access. +- **A reload's backup table is no longer left as readable as the table it + replaced.** PostgreSQL carries privileges along with a rename, so the `_oldN` + table — a copy of the live data — inherited the live table's grants. The + policy is now applied to it as its own relation kind, which grants nothing + under `LMFDBGrantPolicy`. + ### Infrastructure - A test suite of 500+ tests and a continuous-integration workflow covering the diff --git a/DataManagement.md b/DataManagement.md index 06b1f5c..c0755e8 100644 --- a/DataManagement.md +++ b/DataManagement.md @@ -8,6 +8,19 @@ Everything here is a method on either the database object (`db`) or a search tab A note on permissions: most write methods first check `db._read_only`, and several branches of behavior depend on whether your table's statistics class has `saving` turned on (see [Statistics and counts](#statistics-and-counts)). A bare psycodict install has `saving = False`. +Permissions on the relations psycodict *creates* are a separate matter, decided by the database's grant policy (`psycodict.grants`). The default policy grants nothing, so a new search table, its `_counts` and `_stats` companions and the meta tables are reachable only by their owner. A deployment that wants the LMFDB's roles asks for them: + +```python +from psycodict.grants import LMFDBGrantPolicy +db = PostgresDatabase(grant_policy=LMFDBGrantPolicy()) +``` + +which grants `SELECT` to `lmfdb` and `webserver` and `INSERT` on the counts and stats tables to `webserver`, as psycodict used to do unconditionally. A policy names a role that must exist: by default a policy naming a role the cluster does not have raises — at connection time, rather than partway through the first thing that creates a relation — and `missing_role="skip"` (what `LMFDBGrantPolicy` uses, since development databases rarely have those roles) warns and carries on. + +The policy is authoritative for the roles it names. Applying it revokes `SELECT`, `INSERT`, `UPDATE` and `DELETE` from those roles before granting what the policy says, so a relation psycodict creates or swaps ends up with the policy rather than with the policy plus whatever it inherited; roles the policy does not name are never touched. This matters most for the `_oldN` table a [reload](#reload) leaves behind: PostgreSQL carries privileges along with a rename, so the backup would otherwise stay as readable as the live table it used to be, while holding a copy of the same production data. The `backup` relation kind decides what it gets instead — nothing, under `LMFDBGrantPolicy`. + +The reverse is worth knowing too: a reload swaps in a clone, and PostgreSQL creates a clone with no privileges of its own, so under the default policy the replacement is reachable only by its owner even if the table it replaced was readable by an application role. psycodict warns when a swap takes access away like this, naming the roles that lost it. `db.grant_select` and its siblings remain, for granting something to roles you name explicitly. + ## Creating tables ### `create_table` diff --git a/docs/api/grants.md b/docs/api/grants.md new file mode 100644 index 0000000..4e0e4e7 --- /dev/null +++ b/docs/api/grants.md @@ -0,0 +1,5 @@ +# psycodict.grants + +```{eval-rst} +.. automodule:: psycodict.grants +``` diff --git a/docs/api/index.md b/docs/api/index.md index 70bfa38..08e4d2d 100644 --- a/docs/api/index.md +++ b/docs/api/index.md @@ -18,6 +18,8 @@ Generated from the docstrings. The map of the library: PostgreSQL, including the file formats used by the bulk operations. - {mod}`psycodict.config` — configuration discovery and parsing (`config.ini`, command-line arguments, `$PSYCODICT_CONFIG`). +- {mod}`psycodict.grants` — {class}`~psycodict.grants.GrantPolicy`, the + privileges the relations psycodict creates come into existence with. - {mod}`psycodict.utils` — {class}`~psycodict.utils.DelayCommit` and other helpers shared across the library. - {mod}`psycodict.notifications` — LISTEN/NOTIFY support: schema-change @@ -36,6 +38,7 @@ table statstable encoding config +grants utils notifications dbdiff diff --git a/psycodict/database.py b/psycodict/database.py index 0ec5e21..39ca9cb 100644 --- a/psycodict/database.py +++ b/psycodict/database.py @@ -42,6 +42,7 @@ _meta_cols_types_jsonb_idx, validate_search_table_name, ) +from .grants import GRANT_ACTIONS, GrantPolicy from .searchtable import PostgresSearchTable from .utils import DelayCommit, safe_child_path @@ -437,7 +438,7 @@ def _register_object(self, obj): self._objects.append(obj) def __init__(self, config=None, secretsfile=None, create=False, upgrade=False, - session_settings=None, **kwargs): + session_settings=None, grant_policy=None, **kwargs): if config is None: from .config import Configuration config = Configuration() @@ -452,12 +453,25 @@ def __init__(self, config=None, secretsfile=None, create=False, upgrade=False, # or database than the sender writes to). self._connect_kwargs = dict(kwargs) self._session_settings = self._resolve_session_settings(session_settings) + # What the relations psycodict creates are readable and writable by. + # The default grants nothing: a deployment that wants the LMFDB's roles + # asks for them (see psycodict.grants). + self.grant_policy = GrantPolicy() if grant_policy is None else grant_policy self.conn = self._new_connection() self._user = self.conn.info.user # What a replacement connection has to reach to be adopted. self._identity = (self.conn.info.dbname, self.conn.info.user) PostgresBase.__init__(self, "db_all", self) + # Check the policy's roles here, rather than when the first relation is + # created: a policy naming a role this cluster does not have is a + # mistake worth hearing about before it interrupts a bootstrap or a + # reload halfway through. + if self.grant_policy.roles: + self._existing_roles( + self.grant_policy.roles, missing=self.grant_policy.missing_role + ) + if create: # Create any missing metadata tables before the read-only detection # below, which concludes read-only when no tables are visible @@ -602,34 +616,122 @@ def _log_db_change(self, operation, tablename=None, logid=None, aborted=False, * """ pass - def _existing_roles(self, users): + def _existing_roles(self, users, missing="error"): """ - Filters a list of role names down to those that exist in the cluster. + The roles among ``users`` that the cluster actually has. + + INPUT: + + - ``users`` -- role names + - ``missing`` -- ``"error"`` to refuse when one of them does not exist, + ``"skip"`` to warn and leave it out - Missing roles produce a warning rather than an error, so that table - creation works on clusters without the LMFDB roles (lmfdb, webserver). + A policy that names a role the cluster does not have has not been + applied, whatever it says; the default is to say so rather than to + leave the caller believing otherwise. """ existing = {rec[0] for rec in self._execute(SQL("SELECT rolname FROM pg_roles"))} - missing = [user for user in users if user not in existing] - if missing: + absent = [user for user in users if user not in existing] + if absent: + if missing == "error": + raise ValueError( + "Postgres role(s) %s do not exist, so the grant policy " + "cannot be applied; create them, correct the policy, or " + "use a policy with missing_role='skip'" + % (", ".join(absent),) + ) logging.warning( "Postgres role(s) %s do not exist; skipping grants", - ", ".join(missing), + ", ".join(absent), ) return [user for user in users if user in existing] - def _grant(self, action, table_name, users): + def _grant(self, action, table_name, users, missing="error"): """ Utility function for granting permissions on tables. """ action = action.upper() - if action not in ["SELECT", "INSERT", "UPDATE", "DELETE"]: + if action not in GRANT_ACTIONS: raise ValueError("%s is not a valid action" % action) - grantor = SQL("GRANT %s ON TABLE {0} TO {1}" % action) - for user in self._existing_roles(users): + grantor = SQL("GRANT " + action + " ON TABLE {0} TO {1}") + for user in self._existing_roles(users, missing=missing): self._execute(grantor.format(Identifier(table_name), Identifier(user)), silent=True) - def grant_select(self, table_name, users=["lmfdb", "webserver"]): + def _apply_grant_policy(self, table_name, kind): + """ + Give a relation psycodict has just created the privileges the policy + says a relation of its kind has. + + INPUT: + + - ``table_name`` -- the relation + - ``kind`` -- one of ``psycodict.grants.RELATION_KINDS`` + + The policy is authoritative for the roles it names: the actions it + manages are revoked from all of them first, so a relation ends up with + what the policy says rather than with that plus whatever it inherited. + A rename carries privileges with it, which is how a backup table would + otherwise keep the live table's, so this matters most there. Roles the + policy does not name are not touched at all. + """ + policy = self.grant_policy + if not policy.roles: + # the default policy: nothing to grant, and nothing to look up + return + roles = self._existing_roles(policy.roles, missing=policy.missing_role) + if not roles: + return + self._execute( + SQL("REVOKE {0} ON TABLE {1} FROM {2}").format( + SQL(", ").join(SQL(action) for action in GRANT_ACTIONS), + Identifier(table_name), + SQL(", ").join(Identifier(role) for role in roles), + ), + silent=True, + ) + for action, granted in policy.for_kind(kind).items(): + wanted = [role for role in granted if role in roles] + if wanted: + self._grant(action, table_name, wanted, missing=policy.missing_role) + + def _grantees(self, table_name): + """ + The roles that have been granted something on a relation, other than + the grantor's own privileges on it. + """ + cur = self._execute( + SQL( + "SELECT DISTINCT grantee FROM information_schema.role_table_grants " + "WHERE table_name = %s AND grantee <> grantor" + ), + [table_name], + silent=True, + ) + return {rec[0] for rec in cur} + + def _warn_if_access_was_lost(self, table_name, previous_name): + """ + Say so when a swap has replaced a relation others could read with one + they cannot. + + A reload swaps in a clone, which PostgreSQL creates with no privileges + of its own, so under a policy that says nothing about the relation the + replacement is reachable only by its owner -- an application role that + could read the table a moment ago now cannot. That is what the policy + asks for, but it should not be discovered by a website going dark. + """ + lost = self._grantees(previous_name) - self._grantees(table_name) + if lost: + logging.warning( + "%s was replaced by a copy that %s cannot access; the grant " + "policy in force grants them nothing on it. Pass a grant_policy " + "to PostgresDatabase (LMFDBGrantPolicy() reproduces psycodict's " + "pre-1.0 grants) if they should keep it.", + table_name, + ", ".join(sorted(lost)), + ) + + def grant_select(self, table_name, users): """ Grant users the ability to run SELECT statements on a given table @@ -637,10 +739,14 @@ def grant_select(self, table_name, users=["lmfdb", "webserver"]): - ``table_name`` -- a string, the name of the table - ``users`` -- a list of users to grant this permission + + Note that this grants exactly what it is asked to, to exactly the roles + it is given: what a relation psycodict creates starts out with is the + grant policy's business (see :class:`psycodict.grants.GrantPolicy`). """ self._grant("SELECT", table_name, users) - def grant_insert(self, table_name, users=["webserver"]): + def grant_insert(self, table_name, users): """ Grant users the ability to run INSERT statements on a given table @@ -651,7 +757,7 @@ def grant_insert(self, table_name, users=["webserver"]): """ self._grant("INSERT", table_name, users) - def grant_update(self, table_name, users=["webserver"]): + def grant_update(self, table_name, users): """ Grant users the ability to run UPDATE statements on a given table @@ -662,7 +768,7 @@ def grant_update(self, table_name, users=["webserver"]): """ self._grant("UPDATE", table_name, users) - def grant_delete(self, table_name, users=["webserver"]): + def grant_delete(self, table_name, users): """ Grant users the ability to run DELETE statements on a given table @@ -861,7 +967,7 @@ def _meta_creator(self, meta_name, hist=False, fmt=None): parts.append(part) tbl = meta_name + ("_hist" if hist else "") self._execute(SQL("CREATE TABLE {0} ({1})").format(Identifier(tbl), SQL(", ").join(parts))) - self.grant_select(tbl) + self._apply_grant_policy(tbl, "meta_hist" if hist else "meta") @property def meta_format(self): @@ -1047,7 +1153,7 @@ def _stamp_meta_format(self, version): )) self._execute(SQL("INSERT INTO meta_format (version, min_compat) VALUES (%s, %s)"), [version, min_compat]) - self.grant_select("meta_format") + self._apply_grant_policy("meta_format", "meta_format") self._execute(SQL("DROP TABLE IF EXISTS meta_version")) print("Stamped metadata format %s (min_compat %s)" % (version, min_compat)) @@ -1400,7 +1506,7 @@ def create_table( with DelayCommit(self, silence=True): self._create_table(name, search_columns, addid=id_type, tablespace=tablespace) - self.grant_select(name) + self._apply_grant_policy(name, "search") tablespace = self._tablespace_clause(tablespace) creator = SQL( "CREATE TABLE {0} " @@ -1409,8 +1515,7 @@ def create_table( ) creator = creator.format(Identifier(name + "_counts"), tablespace) self._execute(creator) - self.grant_select(name + "_counts") - self.grant_insert(name + "_counts") + self._apply_grant_policy(name + "_counts", "counts") creator = SQL( "CREATE TABLE {0} " '(cols jsonb, stat text COLLATE "C", value numeric, ' @@ -1418,8 +1523,7 @@ def create_table( ) creator = creator.format(Identifier(name + "_stats"), tablespace) self._execute(creator) - self.grant_select(name + "_stats") - self.grant_insert(name + "_stats") + self._apply_grant_policy(name + "_stats", "stats") # FIXME use global constants ? # include_nones is written explicitly rather than left to the # column default: existing databases keep the DDL default their diff --git a/psycodict/grants.py b/psycodict/grants.py new file mode 100644 index 0000000..dd595af --- /dev/null +++ b/psycodict/grants.py @@ -0,0 +1,187 @@ +# -*- coding: utf-8 -*- +""" +Who gets to read and write the relations psycodict creates. + +Creating a search table creates several relations -- the table itself, its +counts and stats tables -- and a reload creates and swaps more. Something has +to decide what privileges those come into existence with, and psycodict used to +decide it in the code: SELECT to ``lmfdb`` and ``webserver``, INSERT to +``webserver``, wherever those roles happened to exist. That is the LMFDB's +deployment, not a fact about a PostgreSQL database, and it meant a new table +holding whatever you put in it was readable by two roles you may never have +heard of. + +A :class:`GrantPolicy` states those privileges explicitly, by relation kind. +The default policy grants nothing, so a relation is reachable only by its owner +and by whatever the cluster's own defaults give away; :func:`LMFDBGrantPolicy` +reproduces what psycodict used to do, for the deployments that want it:: + + db = PostgresDatabase(grant_policy=LMFDBGrantPolicy()) + +A policy is authoritative for the relations psycodict applies it to: it revokes +the actions it manages from the roles it names before granting, so the result +is the policy and not the policy plus whatever was there before. +""" +from dataclasses import dataclass, field + +from .base import InvalidDefinitionError, MAX_IDENTIFIER_LENGTH + +# The actions a policy can grant. A policy manages exactly these: privileges +# outside this set (TRUNCATE, REFERENCES, TRIGGER) are left alone. +GRANT_ACTIONS = ("SELECT", "INSERT", "UPDATE", "DELETE") + +# The kinds of relation psycodict creates, which is what a policy is written in +# terms of -- a policy should not have to know that the counts table of foo is +# called foo_counts. +RELATION_KINDS = ( + "search", # a search table + "counts", # its counts table + "stats", # its stats table + "meta", # meta_tables, meta_indexes, meta_constraints + "meta_hist", # their _hist counterparts + "meta_format", # the metadata format stamp + "backup", # the _oldN table a reload leaves behind +) + +# What to do about a role a policy names that the cluster does not have. +MISSING_ROLE_ACTIONS = ("error", "skip") + + +def _validate_role_name(name): + """ + Check a role name a policy names. + + Roles are quoted with ``Identifier`` wherever they are used, and PostgreSQL + allows more in a role name than psycodict allows in a table name, so this + checks only what would make the name unusable. + """ + if not isinstance(name, str) or not name: + raise InvalidDefinitionError("A role name must be a non-empty string") + if len(name.encode("utf-8")) > MAX_IDENTIFIER_LENGTH: + raise InvalidDefinitionError( + "Role name %r is longer than PostgreSQL's %s byte limit" + % (name, MAX_IDENTIFIER_LENGTH) + ) + for char in name: + if ord(char) < 0x20 or 0x7F <= ord(char) <= 0x9F: + raise InvalidDefinitionError( + "Role name %r contains the control character %r" % (name, char) + ) + return name + + +@dataclass(frozen=True) +class GrantPolicy: + """ + The privileges psycodict grants on the relations it creates. + + INPUT: + + - ``grants`` -- a mapping from relation kind (see ``RELATION_KINDS``) to a + mapping from action (see ``GRANT_ACTIONS``) to the roles that get it. + Kinds left out get nothing. + - ``missing_role`` -- what to do when the policy names a role the cluster + does not have: ``"error"`` (the default) refuses, so that a policy is + never half-applied without saying so; ``"skip"`` warns and carries on, + which is what a development machine without the deployment's roles + wants. + + EXAMPLES:: + + >>> GrantPolicy({"search": {"SELECT": ("readonly",)}}) + GrantPolicy(grants={'search': {'SELECT': ('readonly',)}}, missing_role='error') + """ + + grants: dict = field(default_factory=dict) + missing_role: str = "error" + + def __post_init__(self): + if self.missing_role not in MISSING_ROLE_ACTIONS: + raise ValueError( + "missing_role must be one of %s, not %r" + % (", ".join(MISSING_ROLE_ACTIONS), self.missing_role) + ) + normalized = {} + for kind, actions in self.grants.items(): + if kind not in RELATION_KINDS: + raise ValueError( + "Unknown relation kind %r; psycodict creates %s" + % (kind, ", ".join(RELATION_KINDS)) + ) + normalized[kind] = {} + for action, roles in actions.items(): + if action.upper() not in GRANT_ACTIONS: + raise ValueError( + "Unknown action %r; a policy grants %s" + % (action, ", ".join(GRANT_ACTIONS)) + ) + if isinstance(roles, str): + raise ValueError( + "The roles granted %s on a %s relation must be a " + "sequence of role names, not the string %r" + % (action, kind, roles) + ) + for role in roles: + _validate_role_name(role) + normalized[kind][action.upper()] = tuple(roles) + # frozen dataclass: this is the one place the fields are set + object.__setattr__(self, "grants", normalized) + + @property + def roles(self): + """ + Every role this policy mentions. + + These are the roles whose privileges psycodict manages: applying the + policy revokes the managed actions from them first, so that what a + relation ends up with is the policy rather than the policy plus + whatever it inherited. Roles the policy does not mention are not + touched. + """ + return sorted({ + role + for actions in self.grants.values() + for roles in actions.values() + for role in roles + }) + + def for_kind(self, kind): + """ + The ``{action: roles}`` this policy gives a relation of ``kind``. + """ + if kind not in RELATION_KINDS: + raise ValueError("Unknown relation kind %r" % (kind,)) + return self.grants.get(kind, {}) + + +def LMFDBGrantPolicy(missing_role="skip"): + """ + The permissions psycodict granted before they were a policy. + + SELECT to ``lmfdb`` and ``webserver`` on search, counts, stats and metadata + relations, and INSERT to ``webserver`` on counts and stats, which the + website needs in order to record the counts it computes. Backup (``_oldN``) + tables get nothing: they hold the data the live table held before a reload, + and a rename carries the live table's privileges over to them, so the + policy revokes what the live table had rather than leaving a copy of + production readable by the application roles. + + INPUT: + + - ``missing_role`` -- defaults to ``"skip"``, since a development database + typically has neither role; pass ``"error"`` on a deployment that should + have both. + """ + read = ("lmfdb", "webserver") + write = ("webserver",) + return GrantPolicy( + { + "search": {"SELECT": read}, + "counts": {"SELECT": read, "INSERT": write}, + "stats": {"SELECT": read, "INSERT": write}, + "meta": {"SELECT": read}, + "meta_hist": {"SELECT": read}, + "meta_format": {"SELECT": read}, + }, + missing_role=missing_role, + ) diff --git a/psycodict/statstable.py b/psycodict/statstable.py index 87ccabd..4c4640e 100644 --- a/psycodict/statstable.py +++ b/psycodict/statstable.py @@ -2151,7 +2151,7 @@ def create_oldstats(self, filename): with DelayCommit(self, silence=True): creator = SQL('CREATE TABLE {0} (_id text COLLATE "C", data jsonb)').format(Identifier(name)) self._execute(creator) - self._db.grant_select(name) + self._db._apply_grant_policy(name, "stats") with open(filename) as F: try: self._copy_from_stdin(F, self.search_table + "_oldstats") diff --git a/psycodict/table.py b/psycodict/table.py index d39b337..c7c10da 100644 --- a/psycodict/table.py +++ b/psycodict/table.py @@ -1837,10 +1837,27 @@ def _swap_in_tmp(self, tables): with DelayCommit(self, silence=True): self._swap(tables, "", "_old" + str(backup_number)) self._swap(tables, "_tmp", "") + # Which relation is which is known here, so the policy is looked up + # by it rather than guessed from the name: a search table may end + # in _stats (the LMFDB has one), and granting it a counts table's + # privileges would be wrong. + kinds = { + self.search_table: "search", + self.stats.counts: "counts", + self.stats.stats: "stats", + } for table in tables: - self._db.grant_select(table) - if table.endswith("_counts") or table.endswith("_stats"): - self._db.grant_insert(table) + backup = table + "_old" + str(backup_number) + # The replacement gets the privileges of the kind of relation + # it is, and the table it replaced -- now a backup holding what + # was live a moment ago -- gets the backup kind's, since a + # rename brought the live table's privileges along with it. + self._db._apply_grant_policy(table, kinds.get(table, "search")) + self._db._apply_grant_policy(backup, "backup") + # The replacement was built by _clone, which copies no + # privileges, so under a policy that grants nothing this swap + # can quietly take away access the live table had. + self._db._warn_if_access_was_lost(table, backup) print( "Swapped temporary tables for %s into place in %s secs\nNew backup at %s" % ( diff --git a/tests/test_security.py b/tests/test_security.py index 67cfca2..40a2222 100644 --- a/tests/test_security.py +++ b/tests/test_security.py @@ -31,6 +31,7 @@ validate_index_predicate, ) from psycodict.encoding import Json +from psycodict.grants import GrantPolicy, LMFDBGrantPolicy from psycodict.utils import safe_child_path import conftest @@ -920,3 +921,204 @@ def test_a_name_that_could_escape_the_export_folder_is_refused_anywhere(db, empt ) db.refresh_tables() assert name in db.tablenames +################################################################## +# what a new relation is readable by # +################################################################## + + +def role_grants(db, table): + """ + The (grantee, privilege) pairs on a table, excluding the owner's own. + """ + cur = db._execute( + SQL( + "SELECT grantee, privilege_type FROM information_schema.role_table_grants " + "WHERE table_name = %s AND grantee <> grantor" + ), + [table], + ) + return sorted((grantee, privilege) for grantee, privilege in cur) + + +@pytest.fixture +def roles(db): + """ + Two disposable roles, or a skip where the test user may not make them. + """ + names = ["psycodict_test_reader", "psycodict_test_writer"] + try: + for name in names: + db._execute(SQL("CREATE ROLE {0}").format(Identifier(name))) + db.conn.commit() + except Exception: + db.conn.rollback() + pytest.skip("this test user may not create roles") + yield names + for name in names: + db._execute(SQL("DROP OWNED BY {0}").format(Identifier(name))) + db._execute(SQL("DROP ROLE {0}").format(Identifier(name))) + db.conn.commit() + + +def test_the_default_policy_grants_nothing(db, table_factory): + """ + A new table is not readable by anyone merely because a role exists. + """ + table = table_factory() + name = table.search_table + for relation in [name, name + "_counts", name + "_stats"]: + assert role_grants(db, relation) == [] + for relation in ["meta_tables", "meta_indexes", "meta_constraints", "meta_format"]: + assert role_grants(db, relation) == [] + + +def test_an_explicit_policy_grants_exactly_what_it_says(db, table_factory, roles, monkeypatch): + reader, writer = roles + monkeypatch.setattr( + db, + "grant_policy", + GrantPolicy({ + "search": {"SELECT": (reader,)}, + "counts": {"SELECT": (reader,), "INSERT": (writer,)}, + }), + ) + table = table_factory() + name = table.search_table + assert role_grants(db, name) == [(reader, "SELECT")] + assert role_grants(db, name + "_counts") == [(reader, "SELECT"), (writer, "INSERT")] + # a kind the policy says nothing about gets nothing + assert role_grants(db, name + "_stats") == [] + + +def test_the_lmfdb_policy_reproduces_the_old_behavior(db, roles, monkeypatch): + policy = LMFDBGrantPolicy() + assert policy.for_kind("search") == {"SELECT": ("lmfdb", "webserver")} + assert policy.for_kind("counts") == { + "SELECT": ("lmfdb", "webserver"), + "INSERT": ("webserver",), + } + # and nothing for the tables a reload leaves behind + assert policy.for_kind("backup") == {} + + +def test_a_missing_role_is_refused_and_nothing_is_created(db, monkeypatch): + name = "test_%s" % uuid.uuid4().hex[:12] + monkeypatch.setattr( + db, + "grant_policy", + GrantPolicy({"search": {"SELECT": ("no_such_role",)}}, missing_role="error"), + ) + with pytest.raises(ValueError, match="do not exist"): + db.create_table(name, [("n", "integer")], "n") + # the whole creation rolled back + assert not table_exists(db, name) + assert name not in db.tablenames + assert not db._execute( + SQL("SELECT 1 FROM meta_tables WHERE name = %s"), [name] + ).fetchone() + + +def test_a_missing_role_can_be_skipped_instead(db, table_factory, monkeypatch): + monkeypatch.setattr( + db, + "grant_policy", + GrantPolicy({"search": {"SELECT": ("no_such_role",)}}, missing_role="skip"), + ) + table = table_factory() + assert table.search_table in db.tablenames + assert role_grants(db, table.search_table) == [] + + +def test_a_reload_leaves_the_live_table_with_the_policy_and_the_backup_without( + db, filled_table, roles, monkeypatch, tmp_path +): + """ + A rename carries privileges with it, so the table a reload pushes aside + would keep the live table's readers unless the policy is applied to it too. + """ + reader, _ = roles + monkeypatch.setattr( + db, + "grant_policy", + GrantPolicy({"search": {"SELECT": (reader,)}}), + ) + name = filled_table.search_table + db._apply_grant_policy(name, "search") + assert role_grants(db, name) == [(reader, "SELECT")] + + searchfile = str(tmp_path / "search.txt") + filled_table.copy_to(searchfile) + filled_table.reload(searchfile) + + table = db[name] + assert table.count() == 200 + # the replacement is readable by exactly what the policy says + assert role_grants(db, name) == [(reader, "SELECT")] + # and the backup, which holds what was live a moment ago, is not + assert table._table_exists(name + "_old1") + assert role_grants(db, name + "_old1") == [] + for relation in [name + "_old1", name + "_counts_old1", name + "_stats_old1"]: + if table._table_exists(relation): + db._execute(SQL("DROP TABLE {0}").format(Identifier(relation))) + + +def test_a_policy_rejects_what_it_cannot_apply(): + with pytest.raises(ValueError, match="relation kind"): + GrantPolicy({"nosuchkind": {"SELECT": ("r",)}}) + with pytest.raises(ValueError, match="action"): + GrantPolicy({"search": {"TRUNCATE": ("r",)}}) + with pytest.raises(ValueError, match="missing_role"): + GrantPolicy({}, missing_role="ignore") + with pytest.raises(ValueError, match="sequence of role names"): + GrantPolicy({"search": {"SELECT": "lmfdb"}}) + with pytest.raises(ValueError, match="role name"): + GrantPolicy({"search": {"SELECT": ("",)}}) + with pytest.raises(ValueError, match="control character"): + GrantPolicy({"search": {"SELECT": ("role\x00",)}}) + + +def test_a_role_name_carrying_sql_cannot_build_a_statement(db, monkeypatch): + """ + Role names are quoted, and which role names are legal is PostgreSQL's + business, so a name carrying SQL is not refused for its shape -- it is + refused because no role is called that. + """ + marker = marker_name() + role = 'r"; CREATE TABLE %s (x integer); --' % marker + monkeypatch.setattr( + db, "grant_policy", GrantPolicy({"search": {"SELECT": (role,)}}) + ) + name = "test_%s" % uuid.uuid4().hex[:12] + with pytest.raises(ValueError, match="do not exist"): + db.create_table(name, [("n", "integer")], "n") + assert not table_exists(db, marker) + assert not table_exists(db, name) + + +def test_the_manual_grant_methods_need_explicit_roles(db, empty_table): + with pytest.raises(TypeError): + db.grant_select(empty_table.search_table) + with pytest.raises(TypeError): + db.grant_insert(empty_table.search_table) + + +def test_a_reload_that_takes_access_away_says_so(db, filled_table, roles, caplog, tmp_path): + """ + A reload swaps in a clone, which PostgreSQL creates with no privileges, so + a policy that grants nothing silently takes away what the live table had. + It should not be discovered by a website going dark. + """ + reader, _ = roles + name = filled_table.search_table + db.grant_select(name, [reader]) + assert role_grants(db, name) == [(reader, "SELECT")] + + searchfile = str(tmp_path / "search.txt") + filled_table.copy_to(searchfile) + with caplog.at_level("WARNING"): + filled_table.reload(searchfile) + assert reader in caplog.text + assert "grant_policy" in caplog.text + for relation in [name + "_old1", name + "_counts_old1", name + "_stats_old1"]: + if db._table_exists(relation): + db._execute(SQL("DROP TABLE {0}").format(Identifier(relation)))