A Policy You Bypassed Is Not a Policy

A Policy You Bypassed Is Not a Policy

I read this codebase the way I read any Postgres system: policies first, grants second, indexes third, and the application's opinion of itself last. I came out with a compliment and a complaint — the same observation from two sides.

The compliment: the Row-Level Security in T2D3 OS is better than most. The organization_members policies wrap auth.uid() and is_global_admin() as initplan subselects instead of re-evaluating a volatile call per row. The helpers are SECURITY DEFINER STABLE. Newer org-native tables use a single FOR ALL policy with a matched USING and WITH CHECK on the same org predicate. There is a written RLS-exceptions register naming, per table, why a deviation exists and which route provides the access instead. Somebody here knows what they are doing.

The complaint: none of it protects a query that doesn't go through it.

RLS is a second authorization system, not a setting

This is not a Postgres subtlety; it is an architecture fact. RLS does not make your data safe. It makes the connections carrying a user identity safe. The service-role key is designed to bypass RLS — that is its job. The moment a code path picks up an admin client, the tenant wall stops being a policy in the database and becomes whatever predicate the author happened to type.

In this repo, that moment is not rare. Entire subsystems — skill handlers, cron routes, report builders, background workers — run almost exclusively on the service-role client. Comments in three separate modules say, in effect, RLS can't do it here, so we scope by hand. They are right that it can't. The question is what happens when a hand slips.

The comment that knew

The most instructive thing I found this month is not a bug. It is a comment.

In one skill handler, a fetcher carries the org filter and this note:

// Org-scoped (service-role client — RLS won't do it for us)

Exactly the right mental model, written down at the point of use. In the same file, three sibling fetchers doing the same shape of work — resolve a person, read their tasks, read their key results — omit that filter entirely. The tables they hit do have correct RLS: one requires a project role, the other org membership. Both are bypassed, because the client is service-role. One of those tables has no organization_id column at all; its only tenancy link is a join to projects that nobody performed.

The author understood the rule. The rule was enforced nowhere. That is the whole finding, and it generalizes: the identical shape appears in a merge-tag loader (three batch reads side by side, two carrying .eq(organization_id), one not), in a privacy-flag loader (is_private re-enforced by hand in one place with a comment explaining why, dropped in three others), and in a report path that justified its RLS bypass with "an earlier RLS select proved access" — which only covers rows filtered by the same key. Access is per-query, not per-page.

A convention that lives in comments has a half-life measured in pull requests.

Grants are the other half of RLS

Policies get the attention. Privileges decide whether a policy is ever consulted.

This repo found a real defect on its own: REVOKE ... FROM PUBLIC on a SECURITY DEFINER function does not reliably remove EXECUTE from anon and authenticated. The team documented it in a migration, wrote the correct idiom —

REVOKE ALL ON FUNCTION public.some_fn() FROM PUBLIC, anon, authenticated;
GRANT EXECUTE ON FUNCTION public.some_fn() TO service_role;

— and applied it in five places. One definer function with the incomplete form survived the sweep, and it is not a decorative one. And because the project relies on Postgres's default "EXECUTE to PUBLIC" for new functions and revokes per-function afterwards, instead of one ALTER DEFAULT PRIVILEGES ... REVOKE, two money-handling definer functions labelled "service-role only" in their own headers were authenticated-callable for two months.

For the migration template: CREATE OR REPLACE FUNCTION preserves the existing ACL — rewriting a function does not re-secure it. The only honest way to know who can call something is to ask the catalog (\dp, or has_function_privilege('anon','public.fn()','EXECUTE')), not to read the migration you think created it.

FOR ALL with USING and no WITH CHECK

The most repeated RLS foot-gun here is small enough to miss in review. Authors read USING as "who can see this row" — correct for SELECT — and write:

CREATE POLICY thing_own ON thing
  FOR ALL USING (user_id = auth.uid());

Postgres, given no WITH CHECK, reuses USING as the write check. The policy meant to say "you may read your own rows" now also says "you may insert any row that claims to be yours" — including rows whose other columns assert things the application never intended a user to assert: a status, a score, a completion flag. When two such policies sit on two tables that reference each other, the hole is in neither migration; it is in the seam between them, and reviewing either file alone misses it.

I would not fix these one at a time. Write the lint — any FOR ALL, FOR INSERT or FOR UPDATE policy must carry an explicit WITH CHECK — and run it over pg_policies in CI, not over the migrations directory.

Advisor output is a starting point, not a verdict

I want to be fair to the tooling and unfair to the reflex.

Several "duplicate index" flags in this schema are false positives, and falsely in an interesting way. The indexes are partial unique indexes — UNIQUE (program_id, person_id) WHERE person_id IS NOT NULL, a one-template-per-owner constraint gated on WHERE is_template, a one-verdict-per-asset rule. Not redundant copies of a broader index: business rules, expressed in the only place a business rule survives concurrency. A sweep that "cleans up duplicates" deletes the constraint and leaves the invariant to application code, where it becomes a count-then-insert race.

The team already understands this at the query layer: two places deliberately avoid PostgREST onConflict against a partial unique index — PostgREST cannot emit the WHERE predicate — and do insert-then-recover on 23505 instead. Correct handling, well commented. Advisors don't know your business rules.

What I got wrong

Roughly half of what was filed as S0/S1 did not survive refutation. Two of the corrections were mine.

I filed a credit-minting hole: an UPDATE policy on a credit-balance table with no WITH CHECK, letting a member write their own balance to any number. The policy defect is real, but its blast radius is not — the table it guards is retired and nothing reads it. The expiry cron documents it as unused and is a hard-coded no-op, every live spend path goes through differently-named successor tables, and the consume RPCs were already revoked from anon/authenticated. Minted balances buy nothing. It is orphan-table hygiene, not revenue loss. I had proven reachability in the schema and never proven it in the system.

The second is worse, because it is the mistake I lecture other people about. I read a policy off a migration and reported it live; a later migration had already fixed it. That class bit the review both ways — one finding refuted because a later migration did exactly the requested REVOKE, another reinstated because a performance-only sweep had rewritten a policy and faithfully preserved the hole inside it.

Migrations are a history, not a state. The schema at HEAD is the truth, and even that is a claim until you query the live catalog: this project has migrations that error on a clean apply and a prod database healthy only because four months of later migrations papered over them — the applied ledger and the repo have quietly diverged. Diff forward before calling April SQL live. Better: read pg_policies, not git log.

The gates I would actually add

Not fixes — fixes decay. Gates:

• CI lint over pg_policies: no FOR ALL policy without an explicit WITH CHECK; no unwrapped auth.uid() in a policy body. The August initplan sweep was hand-enumerated table by table and left islands behind — a sweep is a treadmill, a lint is a gate.
• CI check over pg_proc + pg_authid: no SECURITY DEFINER function with EXECUTE for anon or authenticated unless allowlisted with a named in-function guard. That is where the repo's own documented defect escaped.
• A type, not a comment: a branded ScopedClient vs AdminClient. Several of this month's cross-tenant findings would have been compile errors if "this function requires an already-authorized client" were a type instead of a sentence in a header.
• Migration lock discipline: no migration in this tree sets lock_timeout, and the one that tries sets it via ALTER ROLE — which applies only to future sessions, so the index build it was meant to protect ran uncapped. The self-identified hot-path index is a plain write-blocking CREATE INDEX. Against production row counts, that is a guess wearing a migration's clothes.

RLS here is good work. It is also, today, the authorization system for maybe half the queries touching tenant data. Until the other half is routed through a user-identity connection or enforced by something a human can't forget, the tenant boundary is a coding convention. Write it into a gate, not a comment.