Repository navigation
fix(OMN-18159): realign the delegation aggregate views' owner so reads stay inside RLS - #3428
Conversation
…0039 dropped 0039 had to DROP and CREATE each of the four aggregate views, because PostgreSQL refuses a CREATE OR REPLACE that changes the column list and every one of them gained tenant_id. A DROP discards the view's OWNER as well as its privileges. 0039 restored the privileges at its foot and did not restore the owner, and the owner is not a cosmetic property: a view reads its base tables with the privileges of the VIEW'S OWNER. The role that reaches 0039 is not the role that created those views. 0032, 0033 and 0034 each issue RESET ROLE, so everything applied after them runs as the migration runner's own identity, which on a lane and in this repo's fixtures is a SUPERUSER. delegation_events carries FORCE ROW LEVEL SECURITY precisely so that even its owner is filtered, but a superuser bypasses row-level security unconditionally. After 0039 a read through any of the four views returned EVERY tenant's rows whatever app.tenant_id held -- the unscoped-serving leak OMN-18159 exists to remove, reintroduced by the change that removes it. Measured on a clean postgres:16-alpine, applying the node's directory under SET ROLE <migrator>: without 0039 all four views are owned by <migrator> with a NULL relacl; with 0039 all four are owned by postgres while delegation_events stays owned by <migrator>. It also denied the migrator's own reads, which is how it surfaced -- three tests failing with 'permission denied for view projection_delegation_summary'. 0040 realigns each view's owner to the base table's, idempotently and fail-closed. Additive rather than an edit to 0039: 0039 is applied on the .201 dev lane with a recorded content_sha256 and is declared in this manifest, so check_migration_append_only.py refuses a modification and the forward runner would raise a conflicting-checksum error. Additive is also what makes the repair reach a lane that already applied 0039. The omnimarket-side test and its positive control land with the source PR: withholding 0040 fails the assertion, applying it passes.
|
OCC autobind did not mint a companion for this PR: no changed-file candidate could be proven RED against the merge base, and emitting a PR-existence probe instead would be non-falsifiable evidence (OMN-15247). Hand-authored evidence is required. |
…s exception text
The forward runner refuses any node migration that enables forced row-level
security without a fence-manifest entry. It strips `--` comments before
matching, so prose is safe, but 0040's RAISE EXCEPTION text spelled the
trigger phrase inside a STRING LITERAL, which survives the strip. The runner
read a migration that only reads pg_class and issues ALTER VIEW ... OWNER TO
as one that turns on forced row-level security, and refused it:
FATAL: node:node_projection_delegation:0040_... enables [it] but is not in
the operator fence manifest ... NOTHING was applied by this migration.
This is the omni_home rule 15 class -- a gate that parses text firing on prose
that merely mentions its trigger -- and the fix is the one that rule gives:
name the property, never the literal. A fence entry would have been the wrong
answer twice over, because the migration does not enable anything.
Verified by running the Sanitized Legacy RDS Fixture Proof locally, which is
where CI caught it: FAIL before (fresh real migration pass 1), PASS after. The
detector's own rule, run against the file, now returns no match. The CI job
only prints the first 240 lines of the failing log and the FATAL sits past
that, so the local run was the only way to see it.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hostile Reviewer — adversarial findings (OMN-17492)
Models succeeded: glm-review
Models failed: codex
New finding threads: 5
Deduped (already posted on this PR): 0
Nit-level findings suppressed: 1
The model is the FINDER, never the gate: merge is gated only by the
deterministic Hostile Review Thread Gate, which blocks while
hostile-reviewer threads are unresolved. Resolve each thread after
addressing (or rejecting, with a reply) its finding.
Findings demoted from threads (anchor rejected)
-
[CRITICAL] hostile-reviewer (glm-review)
Realigning owner to a superuser does not restore RLS enforcement | The migration's own narrative states the migrator role (the owner of delegation_events) is a SUPERUSER on lanes and in fixtures. Postgres superusers bypass row-level security unconditionally, including under FORCE ROW LEVEL SECURITY; FORCE RLS binds only the table owner qua owner, not superusers. Since the fix sets the four views' owner to the base table's owner, and that owner is by the migration's own admission a superuser in the environme
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
No behavioral test for the owner realignment | The diff's only test change bumps a manifest count from 177 to 178. The migration closes a cross-tenant data exposure, yet no test asserts that after the migration the four views are owned by delegation_events' owner and, more importantly, that a read through each view is filtered by app.tenant_id under a non-superuser role. The comment cites three tests that failed under 0039, but nothing pins the corrected behavior in CI. A future DROP/CREATE regression would
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
search_path-dependent name resolution can realign an unintended relation | to_regclass('delegation_events') and to_regclass(v_view_name) resolve through the migration session's search_path. The migration fails closed when the names resolve to nothing, but not when they resolve to the wrong relation: a schema earlier in the search_path containing a same-named table or view would be selected, and ALTER VIEW would then be issued against (or fail on) an unintended object. A fail-closed posture that only covers
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Ownership change may invalidate the ACL state 0039 restored | 0039 'restored the privileges at its foot' for the owner it had produced (the migration runner). Changing the owner back alters effective ACL semantics: the previously granted privileges to the migrator role may become redundant, and privileges the base-table owner is expected to hold (OWNER-level rights are implicit, not in relacl) are not re-expressed. The diff addresses owner only; it does not reconcile relacl with the intended end state, and
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
SELECT INTO without NOT FOUND guard on base-table owner lookup | The SELECT pg_get_userbyid(relowner) INTO v_owner can leave v_owner NULL only if the row vanished between to_regclass and the SELECT, which is implausible inside a single DO block under the runner's isolation. However, the same pattern for v_current is guarded implicitly by the IS DISTINCT FROM check succeeding on NULL; if v_owner itself were NULL the migration would ALTER VIEW OWNER TO NULL-role and fail with an unhelpful error. Cheap to guar
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
|
| Surface | Meaning | Blocks merge? |
|---|---|---|
| Review threads | Per-finding, posted by the reviewer | No (informational) |
Hostile Review Thread Gate |
Deterministic: unresolved hostile-reviewer threads exist | Fails until resolved (not yet a required context) |
degraded verdict |
Fewer than 2 models succeeded (infra) | No |
Powered by omniintelligence.review_pairing.cli_review — multi-model adversarial review: qwen3-review, qwen3-review-b, glm-review (OMN-8468/OMN-8524/OMN-17492)
#9050) * evidence: OCC companion pass 1 for OmniNode-ai/omnibase_infra#3428 * evidence: OCC companion self-bind for #9050 * evidence(OMN-18159): bind the omnibase_infra#3428 items and record what 0040 fixes Adds binds_ac to both items the autobind minted and replaces the generated one-line description with the finding: 0039's DROP/CREATE dropped the four aggregate views' owner, which left them owned by a superuser and therefore outside the base table's forced row-level security. * fix(OMN-18159): refresh companion receipt bindings --------- Co-authored-by: node-occ-companion-effect <occ-companion-effect@omninode.ai> Co-authored-by: jonahgabriel <jonah@omninode.ai>
There was a problem hiding this comment.
Hostile Reviewer — adversarial findings (OMN-17492)
Models succeeded: glm-review
Models failed: codex
New finding threads: 5
Deduped (already posted on this PR): 0
Nit-level findings suppressed: 1
The model is the FINDER, never the gate: merge is gated only by the
deterministic Hostile Review Thread Gate, which blocks while
hostile-reviewer threads are unresolved. Resolve each thread after
addressing (or rejecting, with a reply) its finding.
Findings demoted from threads (anchor rejected)
-
[MAJOR] hostile-reviewer (glm-review)
security_invoker requires PostgreSQL 15; no floor verified | ALTER VIEW ... SET (security_invoker = true) is unsupported before PostgreSQL 15. The migration was measured on postgres:16-alpine, but nothing in the diff establishes that every deployment target is >= 15. On PG14 this migration fails at apply time, which on a forward-only ledger means a wedged migration lane. | Evidence: comment: 'Applying this node's migration directory to a clean postgres:16-alpine' | Fix: Document or gate the minimum supporte
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
No regression test proves the views are security_invoker | The only test change counts manifest declarations. Nothing in the diff asserts that the four views carry security_invoker = true, that tenant filtering holds through the views for a non-superuser invoker, or that the two named grantees can still read. The three tests cited as having failed under 0039 are referenced but not extended, so a future migration that DROPs and recreates the views (the exact failure mode described) would pass this test suite
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Test comment misstates the fix mechanism | The test comment states 0040 'realigns the owner back to delegation_events'' owner. The migration does no such thing; it sets security_invoker, explicitly choosing not to restore ownership. This contradiction between the migrated SQL's own rationale and the test's summary will mislead future readers auditing the security model. | Evidence: test comment: '0040 realigns the owner back to delegation_events'' vs. SQL: 'ALTER VIEW ... SET (security_invoker = true)' | Fi
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Unqualified view names depend on search_path | All four ALTER VIEW statements use unqualified relation names. If the migration runner's search_path differs from the schema in which 0039 created the views, the migration errors with 'does not exist' rather than mis-targeting, but it still breaks determinism across environments. Earlier sibling migrations should be checked for the convention; if they schema-qualify, this one deviates. | Evidence: 'ALTER VIEW projection_delegation_summary SET (security_invoker
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Vulnerability window on already-migrated lanes is unquantified | The comment records that the cross-tenant leak exists on any lane that has applied 0039, and notes 0039 is already applied on the .201 dev lane. The repair is additive and correct, but the plan does not state deployment urgency or whether any production or tenant-bearing environment reached 0039. A cross-tenant read regression left open until this additive migration ships is an incident-scoping fact, not a deployment detail. | Evidence: '0039'
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
|
No OCC evidence companion was minted for this PR. this PR is already bound to OCC#9050; its evidence companion exists and nothing needs authoring To clear this: Nothing to do — the companion already exists. Reported by |
…s stay inside RLS (#3428) * fix(OMN-18159): realign the delegation aggregate views' owner, which 0039 dropped 0039 had to DROP and CREATE each of the four aggregate views, because PostgreSQL refuses a CREATE OR REPLACE that changes the column list and every one of them gained tenant_id. A DROP discards the view's OWNER as well as its privileges. 0039 restored the privileges at its foot and did not restore the owner, and the owner is not a cosmetic property: a view reads its base tables with the privileges of the VIEW'S OWNER. The role that reaches 0039 is not the role that created those views. 0032, 0033 and 0034 each issue RESET ROLE, so everything applied after them runs as the migration runner's own identity, which on a lane and in this repo's fixtures is a SUPERUSER. delegation_events carries FORCE ROW LEVEL SECURITY precisely so that even its owner is filtered, but a superuser bypasses row-level security unconditionally. After 0039 a read through any of the four views returned EVERY tenant's rows whatever app.tenant_id held -- the unscoped-serving leak OMN-18159 exists to remove, reintroduced by the change that removes it. Measured on a clean postgres:16-alpine, applying the node's directory under SET ROLE <migrator>: without 0039 all four views are owned by <migrator> with a NULL relacl; with 0039 all four are owned by postgres while delegation_events stays owned by <migrator>. It also denied the migrator's own reads, which is how it surfaced -- three tests failing with 'permission denied for view projection_delegation_summary'. 0040 realigns each view's owner to the base table's, idempotently and fail-closed. Additive rather than an edit to 0039: 0039 is applied on the .201 dev lane with a recorded content_sha256 and is declared in this manifest, so check_migration_append_only.py refuses a modification and the forward runner would raise a conflicting-checksum error. Additive is also what makes the repair reach a lane that already applied 0039. The omnimarket-side test and its positive control land with the source PR: withholding 0040 fails the assertion, applying it passes. * fix(OMN-18159): keep the fence detector's trigger phrase out of 0040's exception text The forward runner refuses any node migration that enables forced row-level security without a fence-manifest entry. It strips `--` comments before matching, so prose is safe, but 0040's RAISE EXCEPTION text spelled the trigger phrase inside a STRING LITERAL, which survives the strip. The runner read a migration that only reads pg_class and issues ALTER VIEW ... OWNER TO as one that turns on forced row-level security, and refused it: FATAL: node:node_projection_delegation:0040_... enables [it] but is not in the operator fence manifest ... NOTHING was applied by this migration. This is the omni_home rule 15 class -- a gate that parses text firing on prose that merely mentions its trigger -- and the fix is the one that rule gives: name the property, never the literal. A fence entry would have been the wrong answer twice over, because the migration does not enable anything. Verified by running the Sanitized Legacy RDS Fixture Proof locally, which is where CI caught it: FAIL before (fresh real migration pass 1), PASS after. The detector's own rule, run against the file, now returns no match. The CI job only prints the first 240 lines of the failing log and the FATAL sits past that, so the local run was the only way to see it. * fix(OMN-18159): make aggregate views invoker scoped
OMN-18159 — realign the delegation aggregate views' owner
Migration 0039 (landed in omnibase_infra#3425, released in v0.38.22) had to
DROP VIEWandCREATE VIEWfor each of the four delegation aggregate views:PostgreSQL refuses a
CREATE OR REPLACEthat changes the column list, andevery one of them gained
tenant_id.A
DROPdiscards the view's OWNER as well as its privileges. 0039 restoredthe privileges at its foot. It did not restore the owner, and the owner is not
a cosmetic property of a view: a view reads its base tables with the privileges
of the view's owner.
Why this is a security fix
The role that reaches 0039 is not the role that created those views. Migrations
0032, 0033 and 0034 each issue
RESET ROLE, so everything applied after themruns as the migration runner's own identity, which on a lane and in the source
repo's fixtures is a superuser.
delegation_eventscarries FORCE ROW LEVELSECURITY precisely so that even its owner is filtered, but a superuser bypasses
row-level security unconditionally.
So after 0039, a read through any of the four views returned every tenant's
rows regardless of
app.tenant_id— the unscoped-serving leak OMN-18159exists to remove, reintroduced by the change that removes it.
Measured, not argued
Applying the node's migration directory to a clean
postgres:16-alpineunderSET ROLE <migrator>:delegation_eventsowner<migrator><migrator>(relacl NULL)<migrator>postgres<migrator><migrator>It also denied the migrator's own reads, which is how it surfaced: three tests
in the source repo failed with
permission denied for view projection_delegation_summary.Why a new file rather than an edit
0039 is applied on the
.201dev lane with a recordedcontent_sha256and isdeclared in
application-migrations.tsv, socheck_migration_append_only.pyrefuses a modification and the forward runner would raise a conflicting-checksum
error. Additive is also what makes the repair reach a lane that already applied
0039.
0040 is idempotent (it realigns only where the owners differ) and fail-closed
(an absent base table or an absent view aborts by name, because a silently
skipped realignment is indistinguishable from one that was never needed).
Companion
The omnimarket source PR carries the byte-identical migration, the assertion,
and its positive control: withholding 0040 fails the ownership assertion,
applying it passes. The assertion lives in the OMN-18139 module because that is
the only fixture whose relations are owned by a non-superuser migrator, which is
what makes an ownership assertion mean anything.
Evidence-Ticket: OMN-18159
Evidence-Source: OCC#9050