feat(db): fail-closed RLS backstop on quote tables and tenant-RLS coverage guard (PMS-683) #464

Merged

nrupard merged 1 commit from fix/PMS-683-rls-backstop into main

2026-07-24 17:53:55 +02:00

nrupard commented

2026-07-24 17:00:27 +02:00

Owner

What

Restores the fail-closed tenant_isolation RLS backstop on the tenant-scoped tables that slipped through the 024/038/039 catch-up loops, and adds a durable CI guard so it cannot regress. Scoped to the GUC-safe subset after a per-table query-path audit; the rest is deferred with an explicit, tested allowlist. This does NOT enable RLS on any table whose current query path would break under it.

Why

Migration 038_rls_fail_closed.sql makes tenant isolation fail closed at the database: every tenant table has RLS with a tenant_isolation policy keyed on the app.current_tenant GUC, and the request-serving pool (mokosh_app) is NOBYPASSRLS. Thirteen tables added after the loops never got that policy, so a forgotten WHERE tenant_id on them (including financial quotes and change_requests) has no DB backstop.

The critical constraint (why this is scoped, not a blanket loop)

Because mokosh_app is NOBYPASSRLS, enabling RLS on a table ACTIVELY constrains app queries: any query that does not set the tenant GUC via Database::begin_with_tenant fail-closes to zero rows. A blanket loop over all 13 would therefore break the features whose services do not set that GUC.

I audited every query path of all 13 tables:

GUC-safe (all access via begin_with_tenant): quotes, quote_sequences. All SQL is in src/modules/quotes/service.rs; the portal delegates to the same QuotesService, and data_transfer uses begin_with_tenant. Verified there is no other SQL touching these two tables anywhere in src/.
NOT GUC-safe (served by feature services holding the raw db.pool() and querying it with no GUC): the other 11, across dashboards, approvals, email_intake, saved_reports, workflows, and ticket_templates services. Enabling RLS on them now would fail-close their reads and writes in production. Their feature tests use boot() (the BYPASSRLS pool) so they would not even catch the breakage.

Converting those six services to begin_with_tenant (and routing tenant_intake_tokens's cross-tenant secret-hash resolve through the migrator pool) is the begin_with_tenant read-path migration (PMS-256 / PMS-285 lineage) and is out of scope for a defense-in-depth migration; doing it un-validated across financial and token paths would be higher risk than the gap it closes. It is left as follow-up.

Changes

migrations/094_rls_quotes_backstop.sql: enables + forces ROW LEVEL SECURITY and creates the fail-closed tenant_isolation policy on quotes and quote_sequences, mirroring the exact policy shape of 038/090/091. The header documents the scope decision and lists each of the 11 excluded tables with the service that blocks it.
tests/rls_coverage.rs: a new #[sqlx::test] guard that queries the migrated schema and fails if any public table with a tenant_id column lacks enabled+forced RLS plus a tenant_isolation policy, except a documented ALLOWED_WITHOUT_RLS allowlist holding exactly the 11 deferred tables. It asserts in both directions (no un-allowlisted table lacks RLS; no allowlisted table already has it), so the list can only shrink to empty and a newly added tenant table cannot silently skip RLS the way these 13 did. A schema-querying #[sqlx::test] was chosen over a file-parsing nushell check because the RLS is applied by dynamic information_schema loops that a file parser cannot evaluate, and it runs in the existing integration workflow which provides a fully migrated database.

Follow-up (not in this PR)

Enable RLS on the remaining 11 tables as each owning service is migrated to begin_with_tenant, removing the table from ALLOWED_WITHOUT_RLS in the same change: saved_dashboards, scheduled_dashboards (dashboards); ticket_approvals, change_requests (approvals); tenant_intake_tokens, email_intake_log (email_intake, plus the migrator-pool resolve for the cross-tenant token lookup); saved_reports, scheduled_reports (saved_reports); workflow_rules, workflow_rule_runs (workflows); ticket_templates.

Tests

There is no host cargo, so checks run through Docker (the CI mirrors).

just check-migrations: green (94 prefix unique across 93 files).
just pre-commit (mirrors CI check.yml): green. cargo fmt --check, clippy --all-targets -D warnings (compiles the new test), cargo check --all-targets, 293 unit tests passed / 0 failed, doc tests passed.
Migration 094 applied to a from-scratch fully-migrated schema: clean; post-migration exactly quotes and quote_sequences gain RLS and precisely the 11 documented tables remain uncovered.
tests/rls_coverage.rs (new guard): passed. tests/rls_isolation.rs: passed. tests/per_user_isolation.rs: 4 passed. These RLS-role tests run when #[sqlx::test] connects as the superuser (matching CI's DATABASE_URL). The quotes/quote_convert/quote_signoff suites passed (17/6/6) with RLS enabled.
One unrelated pre-existing failure observed locally: tests/readiness.rs::ready_returns_ok_when_db_reachable_and_infisical_unconfigured 503s because the readiness probe's hard 1s DB-ping timeout is exceeded (~1003 ms) on the dockerized dev Postgres. It fails the same way single-threaded in isolation, touches code this PR does not modify, and is independent of RLS on the quote tables.

## What Restores the fail-closed `tenant_isolation` RLS backstop on the tenant-scoped tables that slipped through the 024/038/039 catch-up loops, and adds a durable CI guard so it cannot regress. Scoped to the GUC-safe subset after a per-table query-path audit; the rest is deferred with an explicit, tested allowlist. This does NOT enable RLS on any table whose current query path would break under it. ## Why Migration `038_rls_fail_closed.sql` makes tenant isolation fail closed at the database: every tenant table has RLS with a `tenant_isolation` policy keyed on the `app.current_tenant` GUC, and the request-serving pool (`mokosh_app`) is NOBYPASSRLS. Thirteen tables added after the loops never got that policy, so a forgotten `WHERE tenant_id` on them (including financial `quotes` and `change_requests`) has no DB backstop. ## The critical constraint (why this is scoped, not a blanket loop) Because `mokosh_app` is NOBYPASSRLS, enabling RLS on a table ACTIVELY constrains app queries: any query that does not set the tenant GUC via `Database::begin_with_tenant` fail-closes to zero rows. A blanket loop over all 13 would therefore break the features whose services do not set that GUC. I audited every query path of all 13 tables: - GUC-safe (all access via `begin_with_tenant`): `quotes`, `quote_sequences`. All SQL is in `src/modules/quotes/service.rs`; the portal delegates to the same `QuotesService`, and `data_transfer` uses `begin_with_tenant`. Verified there is no other SQL touching these two tables anywhere in `src/`. - NOT GUC-safe (served by feature services holding the raw `db.pool()` and querying it with no GUC): the other 11, across `dashboards`, `approvals`, `email_intake`, `saved_reports`, `workflows`, and `ticket_templates` services. Enabling RLS on them now would fail-close their reads and writes in production. Their feature tests use `boot()` (the BYPASSRLS pool) so they would not even catch the breakage. Converting those six services to `begin_with_tenant` (and routing `tenant_intake_tokens`'s cross-tenant secret-hash resolve through the migrator pool) is the `begin_with_tenant` read-path migration (PMS-256 / PMS-285 lineage) and is out of scope for a defense-in-depth migration; doing it un-validated across financial and token paths would be higher risk than the gap it closes. It is left as follow-up. ## Changes - `migrations/094_rls_quotes_backstop.sql`: enables + forces ROW LEVEL SECURITY and creates the fail-closed `tenant_isolation` policy on `quotes` and `quote_sequences`, mirroring the exact policy shape of `038`/`090`/`091`. The header documents the scope decision and lists each of the 11 excluded tables with the service that blocks it. - `tests/rls_coverage.rs`: a new `#[sqlx::test]` guard that queries the migrated schema and fails if any public table with a `tenant_id` column lacks enabled+forced RLS plus a `tenant_isolation` policy, except a documented `ALLOWED_WITHOUT_RLS` allowlist holding exactly the 11 deferred tables. It asserts in both directions (no un-allowlisted table lacks RLS; no allowlisted table already has it), so the list can only shrink to empty and a newly added tenant table cannot silently skip RLS the way these 13 did. A schema-querying `#[sqlx::test]` was chosen over a file-parsing nushell check because the RLS is applied by dynamic `information_schema` loops that a file parser cannot evaluate, and it runs in the existing integration workflow which provides a fully migrated database. ## Follow-up (not in this PR) Enable RLS on the remaining 11 tables as each owning service is migrated to `begin_with_tenant`, removing the table from `ALLOWED_WITHOUT_RLS` in the same change: saved_dashboards, scheduled_dashboards (dashboards); ticket_approvals, change_requests (approvals); tenant_intake_tokens, email_intake_log (email_intake, plus the migrator-pool resolve for the cross-tenant token lookup); saved_reports, scheduled_reports (saved_reports); workflow_rules, workflow_rule_runs (workflows); ticket_templates. ## Tests There is no host cargo, so checks run through Docker (the CI mirrors). - `just check-migrations`: green (94 prefix unique across 93 files). - `just pre-commit` (mirrors CI check.yml): green. cargo fmt --check, clippy --all-targets -D warnings (compiles the new test), cargo check --all-targets, 293 unit tests passed / 0 failed, doc tests passed. - Migration 094 applied to a from-scratch fully-migrated schema: clean; post-migration exactly `quotes` and `quote_sequences` gain RLS and precisely the 11 documented tables remain uncovered. - `tests/rls_coverage.rs` (new guard): passed. `tests/rls_isolation.rs`: passed. `tests/per_user_isolation.rs`: 4 passed. These RLS-role tests run when `#[sqlx::test]` connects as the superuser (matching CI's DATABASE_URL). The `quotes`/`quote_convert`/`quote_signoff` suites passed (17/6/6) with RLS enabled. - One unrelated pre-existing failure observed locally: `tests/readiness.rs::ready_returns_ok_when_db_reachable_and_infisical_unconfigured` 503s because the readiness probe's hard 1s DB-ping timeout is exceeded (~1003 ms) on the dockerized dev Postgres. It fails the same way single-threaded in isolation, touches code this PR does not modify, and is independent of RLS on the quote tables.

nrupard added 1 commit

2026-07-24 17:00:28 +02:00

feat(db): enable RLS on quote tables and add tenant-RLS coverage guard

E2E / Playwright against staging (pull_request) Successful in 1m24s

Details

Check / fmt + clippy + build + tests (pull_request) Successful in 3m57s

Details

Integration / integration tests (pull_request) Successful in 11m53s

Details

Create release / Gate (release-branch merges only) (pull_request) Successful in 1s

Details

Create release / Create release from merged PR (pull_request) Has been skipped

Details

0c22cc0bf0

PMS-683: thirteen tenant-scoped tables were added after the 024/038/039 RLS catch-up loops and never got the fail-closed `tenant_isolation` policy, so a forgotten `WHERE tenant_id` on them has no database backstop.

An audit of the thirteen tables' query paths found that only `quotes` and `quote_sequences` are GUC-safe today: every access is in `src/modules/quotes/service.rs` through `begin_with_tenant` (the portal delegates to the same QuotesService; data_transfer uses `begin_with_tenant`). Migration 094 enables + forces ROW LEVEL SECURITY and the fail-closed `tenant_isolation` policy on those two tables, mirroring 038/090/091 exactly.

The other eleven tables (saved_dashboards, scheduled_dashboards, ticket_approvals, change_requests, tenant_intake_tokens, email_intake_log, saved_reports, scheduled_reports, workflow_rules, workflow_rule_runs, ticket_templates) are still served by feature services that hold the raw NOBYPASSRLS `mokosh_app` pool (`db.pool()`) and query it WITHOUT setting the `app.current_tenant` GUC. Because `mokosh_app` is NOBYPASSRLS, enabling RLS on those tables before their services move to `begin_with_tenant` would fail-close their reads and writes in production, so they are deferred to the `begin_with_tenant` read-path migration (PMS-256 / PMS-285 lineage). For `tenant_intake_tokens` that conversion must also route the cross-tenant secret-hash resolve through the BYPASSRLS migrator pool. The migration header documents each excluded table and why.

`tests/rls_coverage.rs` is a new `#[sqlx::test]` guard that queries the migrated schema and fails if any public table with a `tenant_id` column lacks enabled+forced RLS and a `tenant_isolation` policy, except a documented `ALLOWED_WITHOUT_RLS` allowlist holding exactly those eleven deferred tables. It keeps the allowlist honest in both directions, so a newly added tenant table cannot silently skip RLS the way these thirteen did, and the list shrinks to empty as the migration completes. A schema-querying `#[sqlx::test]` was chosen over a file-parsing nushell check because the RLS is applied by dynamic `information_schema` loops a file parser cannot evaluate, and the test runs in the existing integration workflow, which provides a fully migrated database.

#PMS-683

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>