Repository navigation
fix(postgres): close soundness holes around user-defined objects - #119
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
There was a problem hiding this comment.
Important
The projection rule for aggregates still misses two ways a row gets a column named like an aggregate. In both, sum(x) is claimed one row where Postgres returns a value per row. There are also two precision regressions that are worth a look. Details are inline.
Reviewed changes
I read the full diff of the eight commits: the inference rules, the hardened catalog queries and their lint, the loader's fingerprint and describer, and the new oracle cases.
IS [NOT] DISTINCT FROM: this is now read as the=call it runs, and is non-null only where that=resolves to the built-in or a side is a bareNULL.- Unique key pinning: the untyped and exact-type shortcuts now need the resolved
=to be the built-in one, or the btree equality of the unique index's opfamily (a new trust-querykeyrow, plusCatalog.declared_types). - Column projection (
f(row)read asrow.f): the newExpr::NotRowkeeps a one-argument call non-null only over a type known to be no row.aggregate_for_certaindrops the aggregate claim ofsum/max/… where a row may have a column of that name. This uses the new trust-query rowsrows,scalarsandcolumn, plusrow_fields. - Catalog queries: every query now names only
pg_catalogobjects, with typed literals and nounnest/array_agg. Arrays are read by subscript overgenerate_serieswith new count parameters, and every call site is updated. A parse-tree lint (catalog_queries.rs) and a live test against hostile objects (tests/catalog-queries.test.ts) enforce the rules. - Loader: the schema fingerprint is built with
pg_catalog's own||and withtextin(…out(…)). The describer connects withfetch_types: falseand serializes and parses catalog arrays itself.
claude-opus-5-5 ｜ 𝕏
IS [NOT] DISTINCT FROM compares two non-null values with =, and is NULL where that = answers NULL, as a user's of the name may. It is now non-null only where = resolves to the built-in for its operands' types, or one side is a NULL, where no = runs.
An untyped value beside a unique column, or one of its type, pinned the key whatever = it resolved to: a user's = on a domain, or one the search path puts first. Such a comparison now pins where = resolves to a built-in, or for a type of a user's where the trust query finds that = to be the btree equality of every unique index's key of that type.
Under a search path that puts a user's schema before pg_catalog, a user's `=` or `||` answered the catalog queries, and an overload in pg_catalog itself or a user's cast could beat a polymorphic call or an I/O conversion. Every operator, function, type and table is now pg_catalog's by name, every call an exact match of a non-polymorphic built-in, and a test reads each query's parse tree to keep it so.
postgres.js reads the array types at connect with bare operators, which a user's `=` answers under a search path that puts their schema first; arrays then travelled as text both ways. The describer now writes and reads the catalog queries' arrays itself, and a test runs every catalog query beside hostile copies of what it names.
Under a search path that puts a user's schema first, a user's `||` that answers NULL left every row of the fingerprint query NULL, so the fingerprint stopped moving and .reylo/ went stale. The query now names only pg_catalog's operators, functions and types, and its value on a database with none of the user's is unchanged.
colinhacks
force-pushed
the
fix-trust-followups
branch
from
October 25, 2026 04:10
2336e65 to
d2eea70
Compare
Contributor
There was a problem hiding this comment.
✅ No new issues found. All four findings from the last review are fixed, and each has an oracle case.
Reviewed changes
This covers the six commits since 2336e65.
- Alias column lists:
row_fieldsnow reads thealiasfield of FROM items and joins. So(…) q (bool_and),t AS x (bit_and)and(… JOIN …) AS j (min)count as row columns. - OUT, INOUT and TABLE parameters: the trust query's
columnrow now also readsproargnameswhere the mode iso,bort. Sosum(r)overg() rwithRETURNS TABLE (sum int)ismany. - Enum keys: the
keyrow falls back to the built-in=(anyenum, anyenum)for an enum, or a domain over one. It does so only when no user=takes the enum (exactly, through a domain, or through a polymorphic pseudo-type) and no implicit cast converts it. A user=(mood, text)still blocks the pin. - Typed columns for aggregates:
Columnsnow keeps the type oids thatCOLUMNS_QUERYalready returned.Level::column_typegives an argument's type in the scalar-subquery, jsonshape andaggregatingpaths, which ends the precision loss forcoalesce(max(t.n), 0). - Operator classes:
UNIQUE_KEYS_QUERYnow drops a unique index if any part's operator class uses a different strategy-3 equality than its type's default btree class. This coversrecord_image_opsand a user's operator class overnumeric. - No implicit operand conversions:
o.oprcodeis now cast tooid, and the"char"literals are typed. A new live test fails on any implicit coercion node in a catalog query'sdebug_print_parsetree. The new casetrust_catalog_exact_operandsuses a superuser'spg_catalog.=(oid, regproc). - Artifacts: all three
engine.wasmfiles are rebuilt.
claude-opus-5-5 ｜ 𝕏
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The rest of the soundness work around user-defined operators, functions, casts and search paths that #112 started. An adversarial review of that change turned up holes
mainalready had; this closes them, and hardens every catalog query the way #112 hardened the trust query.Inference
IS [NOT] DISTINCT FROMis read as the call of=it runs: a user=that answers NULL for two non-null values makes it NULL, so it is non-null only where=resolves to the built-in (or a side is a NULL literal, where no=runs). New casetrust_distinct.ValueType::Adapts) and exact-type (t == column_type) shortcuts claimed at most one row forwhere id = 5::int8orwhere id = :peven when a user=(int8, int8)placed first by the search path answers true for every row. The resolved=must now be the built-in or the btree equality of the unique index's operator family, which keeps acitextkey'swhere email = :eat most one row, and an enum key keeps it under the built-in=(anyenum, anyenum)where no user=reaches the enum. A unique index whose operator class compares more finely than the type's default btree equality (record_image_ops, which holds(1.0)and(1.00)apart) pins nothing. New casestrust_key_user_equalityandtrust_key_operator_class.length(t)ist.length(Postgres's projection syntax), solength(t),md5(t),abs(t)could be NULL where they were typed non-null, andsum(s)over a table with a columnsumreturns a row per row where the cardinality said one. A one-argument call keeps its claim only where its argument can't be a row, its type is known to be no row, or nothing gives a row a field of that name: no column the statement shows (alias column lists included), no attribute of a catalog row type, and no OUT, INOUT or TABLE parameter of a function.sum(col),count(*)andcoalesce(sum(n), 0)keep theirs, in scalar subqueries and JSON too, where a column's type is now known. New cases intrust_projectionandtrust_out_parameters.Catalog queries
pg_catalog's objects:OPERATOR(pg_catalog.…), qualified functions, types and tables, typed literals, no conversion through text that aCREATE CASTcould take, no polymorphic function that an overload inpg_catalogcould beat, and no construct that looks=up by name. Before, a user's=or||undersearch_path = public, pg_catalogsilently emptied or errored these queries (a BEFORE trigger went unseen, so aninsert … returningclaimed one row), and under a hostile||the fingerprint stopped changing. A lint test (catalog_queries.rs) fails on any unqualified name,tests/catalog-queries.test.tsreplays every query against about 600 hostile objects and asserts the same rows, and another test prepares every catalog query withdebug_print_parseand fails on any implicit conversion in its parse tree (it found one: anoidcompared with aregprocin the trust query). Array parameters are bound by count, so several queries take a new count parameter.fetch_types: falseand reads arrays itself: postgres.js's own type query on connect compares with a bare=, which a user's=captured, failing every array parameter.The whole oracle passes with
--strict-coverage(153 cases), and no existingexpected.txtline changed. A known limit: a superuser's overloadpg_catalog.generate_series(int4, int4, text default '')makes the queries' exactgenerate_series(int4, int4)call ambiguous, so the analysis fails with the server's error rather than returning a wrong type, as a defaulted overload ofunnestdid before.