Skip to content

Fix RLS context initialization in seed and validation scripts - #4335

Open
gilgardosh wants to merge 4 commits into
mainfrom
claude/staging-deployment-schema-error-2azulc
Open

Fix RLS context initialization in seed and validation scripts#4335
gilgardosh wants to merge 4 commits into
mainfrom
claude/staging-deployment-schema-error-2azulc

Conversation

@gilgardosh

Copy link
Copy Markdown
Collaborator

Summary

Fixes a critical bug where seed and validation scripts fail on deployed (non-superuser) database connections due to uninitialized Row-Level Security context. The issue was invisible locally because dev/CI connect as the postgres superuser, which bypasses RLS regardless of FORCE ROW LEVEL SECURITY.

Key Changes

  • New integration test (seed-admin-context-rls.integration.test.ts): Regression guard that runs seedAdminCore under a non-superuser role to catch RLS context issues before deployment
  • seed-demo-data.ts: Pin app.current_business_id session variable to the admin business ID before any domain table writes; validate the context is set before destructive reset
  • seed.ts: Derive the admin business ID deterministically and pin RLS context before inserting the admin entity (changed from relying on RETURNING id)
  • validate-demo-data.ts: Pin RLS context before reading any domain tables
  • fixture-loader.ts: Remove try-catch that was silently swallowing RLS context errors; let failures surface clearly
  • demo-staging-guide.md: Document the RLS context requirement, how to reproduce locally as a non-superuser, and the new test coverage

Implementation Details

  • All scripts now derive the admin business ID using makeUUID('business', 'Admin Business') — the same deterministic approach used by seedAdminCore and bootstrapNewClient
  • Context is set at session level (is_local = false) since scripts run statements outside transactions
  • The allow_bootstrap_root RLS policy permits inserting/updating the admin entity with id = get_current_business_id() even before it exists, enabling context to be pinned before the entity is created
  • The new test covers both success (context pinned) and failure (context unset) cases, with the failure case reproducing the exact P0001: No business context set error that occurred in production

https://claude.ai/code/session_01Kuz9izFq9MQLVae4NvVLah

The staging deploy fails at `yarn seed:staging-demo` with

    P0001: No business context set - authentication required
    where: PL/pgSQL function accounter_schema.get_current_business_id() line 11

The seed scripts have never been RLS-aware. Since
`2026-05-12T09-00-00.enable-rls-all-tables`, every domain table is
`ENABLE` + `FORCE ROW LEVEL SECURITY` with a `tenant_isolation` policy whose
predicate calls `get_current_business_id()` -- and that helper RAISES rather
than returning NULL when `app.current_business_id` is unset, so there is no
policy path (`allow_bootstrap_root` included) that tolerates a missing context.

It went unnoticed because dev and CI connect as the `postgres` superuser, which
bypasses RLS regardless of FORCE -- the point `helpers/rls-role.ts` already makes
in its header. A deployed database connects as a non-superuser, so the policies
are actually evaluated and the first `financial_entities` insert aborts.

The admin business id is deterministic (`makeUUID('business', 'Admin Business')`),
so the context can be pinned before the row it names exists -- exactly the
bootstrap dance `bootstrapNewClient` already performs. Every seeded row is owned
by that same business, so one context covers the whole run, and
`get_current_business_scope()` falls back to `ARRAY[get_current_business_id()]`,
so the scope GUC needs no separate handling.

- scripts/seed-demo-data.ts: pin the context session-level (steps 2-4 run outside
  any transaction, so a transaction-local setting would not survive), and assert
  seedAdminCore returns the id we pinned to.
- scripts/seed-demo-data.ts: verify the context *before* the destructive reset.
  That TRUNCATE is autocommitted, so failing after it left staging wiped rather
  than merely un-reseeded.
- validate-demo-data.ts: `yarn validate:demo`, the deploy's next step, reads the
  same RLS'd tables and would have failed identically.
- scripts/seed.ts: same omission. It took the admin id from `RETURNING id`, which
  is too late to pin a context, so derive it up front and insert it explicitly.
- fixture-loader.ts: stop swallowing a failed context set. Under RLS it turns one
  clear failure into a cascade of policy violations that name nothing useful.

Adds seed-admin-context-rls.integration.test.ts, which runs seedAdminCore under
the existing non-superuser role helper: it succeeds with the context pinned and
fails with P0001 without it. It lives under `src/__tests__/` so it runs in the
`integration` project on pull requests -- the `demo-seed` project only runs on
push to main, and both connect as superuser anyway.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kuz9izFq9MQLVae4NvVLah
@gilgardosh
gilgardosh deployed to accounter-fullstack August 31, 2026 15:58 — with GitHub Actions Active
@gilgardosh
gilgardosh deployed to accounter-fullstack August 31, 2026 15:58 — with GitHub Actions Active
@gilgardosh gilgardosh self-assigned this Aug 31, 2026
@gilgardosh
gilgardosh requested a lite review from Copilot August 31, 2026 16:01
@github-actions

Copy link
Copy Markdown
Contributor

The latest changes of this PR are not available as alpha, since there are no linked changesets for this PR.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kuz9izFq9MQLVae4NvVLah
@gilgardosh
gilgardosh deployed to accounter-fullstack August 31, 2026 16:03 — with GitHub Actions Active
@gilgardosh
gilgardosh deployed to accounter-fullstack August 31, 2026 16:03 — with GitHub Actions Active

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The changes consistently pin the RLS tenant context before any tenant-scoped operations and add a targeted non-superuser regression test that should prevent the production failure from recurring.

Pull request overview

This PR fixes demo seed and validation scripts failing on deployed (non-superuser) Postgres connections by explicitly pinning the RLS tenant context (app.current_business_id) before any tenant-scoped reads/writes, and adds an integration regression test that exercises seedAdminCore under a non-superuser role so CI catches missing-context failures.

Changes:

  • Pin app.current_business_id early in scripts/seed.ts, scripts/seed-demo-data.ts, and validate-demo-data.ts using the deterministic admin business UUID (makeUUID('business', 'Admin Business')).
  • Make root admin business insertion deterministic in scripts/seed.ts (explicit id, no RETURNING) so context can be pinned before the row exists.
  • Add seed-admin-context-rls.integration.test.ts to verify seedAdminCore succeeds with context pinned and fails with the expected P0001 when unset; also stop swallowing RLS-context failures in fixture-loader.ts.
File summaries
File Description
scripts/seed.ts Pins RLS context up-front and inserts the admin business with a deterministic UUID to avoid RETURNING reliance under RLS.
scripts/seed-demo-data.ts Pins RLS context before TRUNCATE/seed steps and validates deterministic admin ID consistency with seedAdminCore.
packages/server/src/demo-fixtures/validate-demo-data.ts Pins RLS context before any reads to prevent P0001 on non-superuser connections.
packages/server/src/tests/seed-admin-context-rls.integration.test.ts Adds a non-superuser RLS regression test covering both the “pinned succeeds” and “unset fails with P0001” cases.
packages/server/src/tests/helpers/fixture-loader.ts Removes best-effort try/catch so RLS context setup failures surface immediately.
packages/server/docs/demo-staging-guide.md Documents the RLS context requirement and how to reproduce the deployed behavior locally.
Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Run `yarn prettier:fix`: one paragraph in the demo staging guide was left
short of the 100-char proseWrap fill.

Cut the changeset down to the mechanism and the fix, dropping the detail
about which database role runs where.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kuz9izFq9MQLVae4NvVLah
@gilgardosh
gilgardosh deployed to accounter-fullstack August 31, 2026 17:17 — with GitHub Actions Active
@gilgardosh
gilgardosh deployed to accounter-fullstack August 31, 2026 17:17 — with GitHub Actions Active
@gilgardosh
gilgardosh requested a lite review from Copilot August 31, 2026 17:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new integration test should close the shared test DB pool to avoid open-handle hangs, and the production seed path should be made rerunnable/idempotent now that it uses deterministic IDs.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

packages/server/src/tests/seed-admin-context-rls.integration.test.ts:63

  • This test creates the shared test pool via TestDatabase/connectTestDb but never closes it in afterAll; relying on “global vitest setup” is inconsistent with other suites and can leave open handles when running this file in isolation. Close the shared pool via db.close() (or closeTestDb()) after dropping the RLS role.
  afterAll(async () => {
    if (pool) await dropRlsRole(pool);
    // Pool is managed by the global vitest setup — do not close it here.
  });
  • Files reviewed: 7/7 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread scripts/seed.ts
Addresses review feedback on #4335.

Pinning the RLS context requires knowing the admin id before the row exists,
so the id is now derived rather than returned by the database. That turned a
rerun from a silent second "Admin Business" row into a hard duplicate-key
failure -- and since this script runs outside a transaction, rerunning it is
how a partial failure is resumed. Both admin inserts now use
`ON CONFLICT (id) DO NOTHING`, matching `ensureBusinessForEntity`; the
self-owning UPDATE that follows already normalizes `owner_id`.

Also name `owner_id` in the `businesses` insert. The column is NOT NULL since
`2026-02-18T16-00-00.owner-id-not-null` with no default, so the insert could
not have succeeded on the current schema at all -- a separate pre-existing
break, but this statement is unusable without it. The admin business owns
itself, so the value is the entity id, as `seedAdminCore` does.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kuz9izFq9MQLVae4NvVLah
@gilgardosh
gilgardosh deployed to accounter-fullstack September 1, 2026 17:01 — with GitHub Actions Active
@gilgardosh
gilgardosh deployed to accounter-fullstack September 1, 2026 17:01 — with GitHub Actions Active
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants