RLS Allowing actor_user_id NULL Erases Audit Trails

Conclusion

The RLS policy permitting INSERTs into the audit log allowed actor_user_id IS NULL unconditionally. A compromised tenant account could use that gap to record its own actions as actor=NULL and erase itself from the audit trail.

• actor_user_id IS NULL was a condition that passed for every role
• The only role that should be allowed NULL is system_admin, which performs cross-tenant internal work
• Tenant roles (school_admin, teacher, and so on) should be rejected unless the value matches their own user_id exactly

The policy was rewritten to branch on role and reject NULL from tenant roles.

Symptom

INSERTs into the audit log were permitted by this policy.

CREATE POLICY audit_log_insert ON audit_log FOR INSERT
  WITH CHECK (
    (
      school_id IS NULL
      OR school_id = NULLIF(current_setting('app.current_school_id', true), '')::uuid
      OR current_setting('app.current_user_role', true) = 'system_admin'
    )
    AND (
      -- prevent actor_user_id spoofing: allow self, system_admin, or null
      actor_user_id IS NULL
      OR actor_user_id = NULLIF(current_setting('app.current_user_id', true), '')::uuid
      OR current_setting('app.current_user_role', true) = 'system_admin'
    )
  );

The comment says “prevent spoofing”, and indeed rewriting actor_user_id to someone else’s uuid was blocked. But actor_user_id IS NULL remained a condition that passed regardless of role.

That is what review flagged. If a tenant account (school_admin or teacher) is compromised, the attacker can insert into the audit log with NULL instead of their own user_id. The audit log exists to record who did what, so an action with a NULL actor is effectively unattributable. It was classified as a repudiation risk.

Cause

Allowing actor_user_id IS NULL was there for legitimate writes not tied to any tenant user — cross-tenant internal aggregation, inserts through the migrator.

But the policy’s condition was not split by role; it was a single monolithic “NULL passes” clause. The intent was to cover what system_admin does internally, yet a WITH CHECK expression applies equally to every session regardless of role. The intent was role-limited; the implementation never looked at the role.

The fix

Rewrite the actor_user_id side as a branch on role.

DROP POLICY IF EXISTS audit_log_insert ON audit_log;

CREATE POLICY audit_log_insert ON audit_log FOR INSERT
  WITH CHECK (
    (
      school_id IS NULL
      OR school_id = NULLIF(current_setting('app.current_school_id', true), '')::uuid
      OR current_setting('app.current_user_role', true) = 'system_admin'
    )
    AND (
      -- actor_user_id side:
      --   system_admin may supply NULL or any uuid (cross-tenant internal work)
      --   every other role: exact match on own user_id only (rejects NULL, prevents spoofing)
      current_setting('app.current_user_role', true) = 'system_admin'
      OR actor_user_id = NULLIF(current_setting('app.current_user_id', true), '')::uuid
    )
  );

system_admin passes unconditionally on the first OR; every other role can only pass the second condition, an exact match against the user_id set with SET LOCAL. Under SQL’s three-valued logic, NULL = uuid evaluates to NULL rather than true, so a tenant role supplying NULL for actor_user_id fails automatically. The monolithic “NULL passes” clause was re-embedded inside a role branch.

A verification gap found alongside it

In the PR that added this policy, CI did not fail the way it should have.

The setup that initialises the test database listed its migration files as constants.

const RLS_ENABLE_SQL = join(packageRoot, "migrations", "0001_enable_rls.sql");
const RLS_POLICIES_SQL = join(packageRoot, "migrations", "0002_rls_policies.sql");
const AUDIT_TRIGGER_SQL = join(packageRoot, "migrations", "0003_audit_trigger.sql");
const AUDIT_FK_SQL = join(packageRoot, "migrations", "0004_audit_fk.sql");

The migration adding the new policy existed in the repository but was not in that list. The test database kept the old policy allowing NULL, so the test meant to verify the new one — “a tenant role supplying NULL is rejected” — was in fact running against the old policy. The test code itself was correct; the database under test had simply never applied the new migration, so the behaviour it was written to check was never checked.

The fix was to add the migration to the list and to the execution order.

const AUDIT_LOG_ACTOR_NULL_SQL = join(
  packageRoot,
  "migrations",
  "0005_audit_log_actor_null_school_admin.sql",
);

With that, CI’s test database applies the new policy and the rejection of NULL from tenant roles is genuinely exercised.

Making it less likely to recur

When writing an RLS policy condition, you have to read the expression itself and confirm that “the target you intended” and “the target the implementation names” are the same. The single line actor_user_id IS NULL reads as a blanket allowance to every role unless you also read the comment describing the intent.

And fixing the policy is not the end of it. Whether the test database reflects a migration is a separate thing to confirm from whether the migration file exists. Adding a migration means checking, in the same change, that the test setup registers it for execution.