Repository navigation
Policy expression changes are silently ignored — migrate diffs RLS policies by name only #98
Description
Summary
DataProviderMigrate migrate diffs RLS policies by name only. If a policy's
using: / withCheck: expression changes in schema.yaml but the name stays the
same, the tool reports Schema is up to date — no operations needed and the
database keeps the old, wrong predicate — silently, forever.
This is not a cosmetic drift issue. It cost us a full production outage.
Impact (real incident, 2026-09-04)
migrations/schema.yaml declared:
- name: tenants_self_membership_select
operations: [Select]
roles: [app_user]
using: "app_tenant_id() is null and is_member(app_user_id(), id)"Production actually had the predecessor expression:
(current_setting('app.tenant_id'::text, true) IS NULL) AND is_member(app_user_id(), id)The corrected expression had been in schema.yaml since PR #275 and make migrate
had been run repeatedly against prod since. Every run said "up to date". The old
predicate is permanently false on any pooled connection (once any transaction on
a backend runs SET LOCAL app.tenant_id, the placeholder's reset value becomes '',
never NULL again), so GET /tenants/me returned [] for every user of the platform.
There was no signal anywhere: exit code 0, no warning, no drift report.
Reproduction
DataProviderMigrate migrate --schema s.yaml --output "$PG" --provider postgres
with a policypwhoseusing:is expression A.- Edit
s.yamlso policypkeeps its name but itsusing:becomes expression B. - Re-run the same command.
Expected: the tool detects the changed predicate and replaces the policy
(DROP POLICY + CREATE POLICY, gated behind --allow-destructive if you consider
the drop destructive).
Actual: Schema is up to date — no operations needed. The database still enforces
expression A.
--allow-destructive does not help — the diff is empty before phase filtering, so
there is nothing for it to permit.
Why the workaround is bad
The only way to land the fix through DataProvider was to rename the policy
(tenants_self_membership_select → ..._v2) so the differ saw it as a new object.
That leaves a dead policy in the database until a separate destructive pass, and it
puts a version suffix into a name that should be stable. Our house rules forbid
hand-written DDL, so there is no clean escape hatch.
Asks
- Diff policy
USING/WITH CHECKexpressions, not just names. Comparing
pg_get_expr(polqual, polrelid)after normalisation would catch this. - If exact expression comparison is too brittle (Postgres reformats predicates —
app_tenant_id()comes back aspublic.app_tenant_id(), literals get::text
casts), then at minimum emit a warning listing every policy whose stored
expression does not match the declaration. Silence is the part that hurt. - Same question applies to other body-bearing objects —
functions:bodyLql,
check constraints. If those are also name-only, they have the same failure mode.
Related, seen in the same run
Every migrate run ends with:
SCHEMA INTEGRITY CHECK FAILED
public.topup.status: default expected 'pending' but found 'pending'::text
The integrity checker is not normalising Postgres' ::text cast on a text column
default, so it reports a false positive on a converged schema. This trains operators
to ignore the tool's final line — which is exactly why the policy drift went unnoticed
for as long as it did. Worth fixing alongside.
Security note
DataProviderMigrate echoes the full connection string, password included, to
stdout on every run:
Output: Host=...;Port=5432;Database=postgres;Username=...;Password=<cleartext>;SslMode=Require;...
That lands in CI logs, terminal scrollback, and agent transcripts. Please mask the
password (or drop the line).
Versions: DataProviderMigrate (dotnet tool, current as of 2026-09-04), PostgreSQL 15
(Supabase), connecting through the Supavisor pooler.
Activity
MelbourneDeveloper commentedon Sep 30, 2026
Review from the NAP RLS rollout, based on the current local migration patch (source review; I have not independently executed these changes).
The move to predicate-aware ALTER/transactional replacement is the right direction. Two concrete comparison gaps remain in
Migration.Core/RlsPolicyPredicates.cs:Equivalentcurrently normalizes nested expressions, identifiers and binary operators, then compares every other AST node byToString(). PostgreSQL deparses text literals with casts and rewrites IN lists into ANY/ARRAY forms. NAP uses predicates such asrole in ('admin', 'member'),app_is_system() = 'true', and membership function calls. Please add real catalog round-trip tests using these expressions: unchanged declarations must produce no operations and pass integrity verification, while changing a role literal must still alter enforcement. The current new IS NULL/IS NOT NULL test does not exercise these forms. Avoid broad cast removal that could erase a meaningful change.SameScopeusesStringComparer.OrdinalIgnoreCasefor role names, while the PostgreSQL generator quotes role identifiers. Distinct quoted roles differing only by case can therefore compare equal, leaving the previous authorization target in place and masking the drift in verification. Add a role-change regression with distinct quoted role names and compare their actual database identities correctly. Also cover explicit PUBLIC versus the empty-role default.For the new replacement path, add tests for role/operation/permissive changes and a failed replacement proving the original policy survives rollback. Please validate row visibility and rejected writes under a non-BYPASSRLS role, as well as catalog text.
MelbourneDeveloper commentedon Sep 30, 2026
Second NAP review, 2026-09-30: reviewed
fix/nap-migration-blockersat local HEAD7f514fa, including the uncommittedRlsPolicyPredicates.cschanges (SHA-256 starts1b4674590b4a). Source review only; I have not independently run this version.The case-sensitive role comparison and explicit PUBLIC handling address the earlier source findings. Three issues remain:
NAP's actual text flags still do not converge.
NAP/migrations/schema.yamldeclaresapp_is_systemandapp_is_platform_adminas returning text, deliberately compared with'true'. Catalog predicates containapp_is_system() = 'true'::text.Equivalent/SameBooleanLiteral(lines 159–175, 224–238) cannot equate thatExpression.Castwith the declaration's string literal; text-cast handling currently exists only inside the IN/ANY case. The Boolean fallback therefore does not fix this NAP case. Add a real catalog round-trip with the actual text-returning helper, verify no repeat ALTER and successful integrity verification, and prove changing the literal still changes enforcement. Keep normalization type-aware.Policy changes are assigned to the wrong CLI phase.
DataProviderMigrate/Program.cs:IsRlsOperation(lines 405–413) includes neitherAlterRlsPolicyOperationnorReplaceRlsPolicyOperation. Consequently--phase rlsskips both, while--phase structuralapplies them. Include both operations in the RLS partition and exercise both CLI phases against an existing policy.Function normalization can erase changed arguments.
EquivalentFunctionline 218 usescurrent.ToString().Replace(currentName, desiredName, ...)on the entire call. For example, comparingpublic.can_access('public.can_access'::text)withcan_access('can_access'::text)can compare equal after replacement despite different literal arguments. Normalize the parsed function identifier only, and recursively compare arguments without rewriting their contents. Add this counterexample so a changed authorization argument never disappears as formatting.The new CLI policy regressions still use IS NULL/IS NOT NULL and catalog snapshots. Add non-BYPASSRLS visibility/write assertions, scope/role/permissive changes, and failure during replacement proving the original policy survives. The new generic transaction test runs SELECT 1; it does not establish policy-replacement rollback. Please keep this issue open until the actual NAP schema upgrade and unchanged rerun pass in the released tool.