Skip to content

Rebuild the same connection on a reset, not a weaker one - #135

Open
roed-math wants to merge 3 commits into
roed314:mainfrom
roed-math:security-reconnect-options
Open

Rebuild the same connection on a reset, not a weaker one#135
roed-math wants to merge 3 commits into
roed314:mainfrom
roed-math:security-reconnect-options

Conversation

@roed-math

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

Copy link
Copy Markdown

Third of five PRs from the August 3 security audit.

Stacked on #133 and #134, so the diff here contains their commits too;
merge those first and this one shrinks to its own commit. (GitHub will not take
a base branch that lives in the fork, hence the main base.)

The problem

PostgresDatabase.__init__ saves the connection overrides it is given in
_connect_kwargs and uses them for the first connection. reset_connection
then calls _new_connection() with no arguments, so the replacement is built
from the configuration alone. Everything the caller passed explicitly is
dropped:

  • sslmode, sslrootcert, sslcert, sslkey;
  • an alternate host, port, database or user;
  • libpq options;
  • connect_timeout and keepalive settings.

A connection that drops mid-session can therefore come back without TLS, or
pointed at a different server or database than the one whose schema psycodict
has cached. The 25 second statement_timeout for the webserver role had the
same shape of problem: it was set once, in the constructor, with a SET on the
connection it was replacing later. The read-only, superuser, knowls and userdb
capability flags were computed once and never rechecked.

The fix

  • _connection_options() is the single place where configuration and explicit
    overrides are merged, and _new_connection() takes no arguments — there is no
    longer a call shape that can forget the overrides.
  • _configure_session(conn) holds the per-session setup (adapters, encoding,
    session settings) and runs for every connection, first or replacement. It
    works directly on the connection rather than through _execute, since it runs
    while recovering from a failure in _execute.
  • PostgresDatabase(session_settings={...}) sets statement_timeout,
    lock_timeout, idle_in_transaction_session_timeout or application_name.
    The webserver's 25s timeout is now this setting's default for that role. The
    names are a closed set and the values go through set_config, which takes
    both as bound parameters.
  • _detect_capabilities(conn) is factored out of the constructor and rerun for
    every replacement: a reconnect can land on a standby or as a role with
    different grants, and a stale _read_only = False is wrong in the unsafe
    direction. It stays where it was in the constructor's order, after
    create/upgrade, since the read-only check concludes read-only when it can
    see no writable table.
  • A replacement is adopted only after _verify_identity confirms it reached the
    same database as the same role. The host is deliberately not checked, so a
    failover to a standby still works, and the TLS and endpoint options are
    preserved either way. On rejection the new connection is closed and the object
    keeps the old one.
  • The replaced connection is closed rather than left to the garbage collector.

Retry semantics are untouched: a standalone statement reconnects and retries
once, and a statement inside DelayCommit still raises rather than replaying a
transaction whose earlier statements went down with the connection.

listener() already passed _connect_kwargs to NotificationListener, which
merges them over the configured options the same way; its autocommit behavior
is unchanged. I did not apply session_settings to the listener's connection —
a statement_timeout on a connection that exists to wait for notifications
seemed more likely to be wrong than right, but say the word if you would rather
it inherited them.

Tests

New tests/test_reconnect.py (11 cases): the merged options carry
sslmode/sslrootcert/sslcert/sslkey/connect_timeout/options/keepalives
(checked on the merge, so no TLS server or certificate files are needed); a
forced reset and a reset triggered by a dropped connection each record exactly
the same options as the initial connect; session settings are present before and
after a reconnect, including the webserver default; an unknown session setting is
refused; a replacement reaching a different database is refused and the object
keeps its old connection; capability flags are recomputed; a standalone statement
is retried exactly once; a DelayCommit transaction is not replayed; and an
unreachable server does not loop.

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


Review round (2026-08-03)

  • Capability detection is atomic. _detect_capabilities now returns a
    _ConnectionCapabilities snapshot built from local variables and writes
    nothing to the database object; _apply_capabilities is the single mutation
    point. A candidate that fails a later probe — or at the commit — leaves the
    old connection, the registered objects and all four flags exactly as they
    were, and is closed.
  • Registered objects get a _connection_reset() hook, a no-op by default,
    called after the replacement connection and its capabilities are in place,
    so an override can just read them. The replaced connection is closed even if
    a hook raises, and a hook's failure is not swallowed. This is for consumers
    that cache what the old session was allowed to do; the LMFDB's
    KnowlBackend._rw_knowldb and PostgresUserTable._rw_userdb/_cols are the
    known cases, and they need a downstream change to use it — written up at
    ~/claude/handoffs/lmfdb-psycodict-security-upgrade.md rather than done here.
  • A connection whose per-session setup fails is closed instead of being left
    to the garbage collector holding a server slot; a failure in that close does
    not mask the original error.
  • Documentation corrected: session_settings apply to the main connection
    and its replacements, not to "every connection this database opens" — a
    notification listener deliberately does not inherit them (it does inherit the
    overrides that decide which server it reaches). And _verify_identity
    compares the database name and the role, which two different clusters can
    agree on; it is not proof of reaching the same server.
  • The webserver-default test now exercises the real resolver instead of
    monkeypatching it.

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

🤖 Generated with Claude Code

@read-the-docs-community

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

Copy link
Copy Markdown

roed314 and others added 3 commits August 3, 2026 13:34
A column type cannot be bound as a value -- PostgreSQL has no placeholder
for a type -- so create_table, add_column and header-driven table creation
interpolate it into DDL as SQL text.  The check guarding that
interpolation used regexp.match() against a set of unanchored patterns, so
any string beginning with a valid type passed validation with the rest
still attached, and psycopg runs a parameterless statement with the simple
query protocol, which executes every statement in it.

Centralize the check in validate_column_type(), which matches the complete
string with fullmatch(), restricts types to an ASCII character set that
excludes NUL, control characters and lookalikes, and returns the spelling
callers must emit.  Every type-bearing DDL path now emits that returned
spelling: _order_columns, _create_table, _create_table_from_header (and so
reload/reload_all with adjust_schema=True), add_column, and the temporary
table built when resorting ids.

The character-type pattern was also malformed -- a bracket typo in the
varchar length meant varchar(N) was only ever accepted as a prefix of
varchar -- so the char family is now described deliberately, with an
optional length and an optional collation.  Collation names stay
case-sensitive, since they are quoted identifiers: "C" is a collation and
"c" is not.  add_column validates before it mutates col_type, so a
rejected type leaves the table object alone.

InvalidColumnTypeError subclasses both ValueError and RuntimeError, the
latter being what an invalid type raised before.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
meta_indexes and meta_constraints hold what an index or constraint is
rebuilt from, sometimes long after it was created.  Between the two the
rows can be edited with plain SQL, exported to a file, carried to another
database and imported, or restored from the _hist tables, so
"this row must once have passed through create_index" is not something a
statement builder can rely on.  It was relied on: the access method,
column modifiers, storage-parameter names, check function and
partial-index predicate were formatted into the statement as text, and a
predicate is appended to CREATE INDEX, where a semicolon ends it.

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

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

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

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
reset_connection() called _new_connection() with no arguments, so a
replacement connection was built from the configuration alone.  The
overrides passed to PostgresDatabase(...) -- sslmode and the certificate
paths, an alternate host, port, database or user, libpq options, connect
and keepalive timeouts -- were kept in _connect_kwargs and used only for
the first connection.  A connection that dropped therefore came back
without them: possibly without TLS, possibly to a different server.  The
webserver statement timeout was set once in __init__ and went with the
session it was set on, and the read-only, superuser, knowls and userdb
capability flags were never recomputed.

Merge the options in one place, _connection_options(), used by every
connection this database opens; move per-session setup into
_configure_session(), applied to each new connection; and generalize the
webserver timeout into session_settings, a small closed set of settings
applied through set_config, which takes the name and value as bound
parameters.

A replacement is now adopted only after it is checked to have reached the
same database as the same role -- the host is deliberately not checked,
since a failover is a legitimate way for it to change -- and after the
capability flags are recomputed from it.  Both run directly on the new
connection rather than through _execute, which is what is being recovered
from.  The retry rules are untouched: a standalone statement is retried
once, and a statement inside DelayCommit still raises rather than
replaying a transaction whose earlier statements died with the connection.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@roed-math
roed-math force-pushed the security-reconnect-options branch from 10e7b36 to 9c17b7c Compare August 3, 2026 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants