diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index 876c250..cdf296a 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -40,7 +40,12 @@ on: env: # Pin Node + pnpm versions for reproducibility. - NODE_VERSION: "20" + # Node 22 is the floor: pnpm 11.23.0 (the packageManager pin) needs >= v22.13 + # and dies on Node 20 with ERR_UNKNOWN_BUILTIN_MODULE (node:sqlite). + NODE_VERSION: "22" + # Display label for the pnpm step only. The INSTALLED pnpm version is the + # packageManager pin in package.json - pnpm/action-setup rejects a second + # pin (workflow version: + package.json packageManager:) and exits 1. PNPM_VERSION: "11" jobs: @@ -61,7 +66,6 @@ jobs: - name: Setup pnpm ${{ env.PNPM_VERSION }} uses: pnpm/action-setup@v4 with: - version: ${{ env.PNPM_VERSION }} run_install: false - name: Get pnpm store dir @@ -78,6 +82,12 @@ jobs: - name: Install dependencies run: pnpm install --frozen-lockfile + # Consumers resolve @coreci/db and @coreci/secrets through their + # published dist/*.d.ts, so the packages must be built (pnpm runs + # them in dependency order) before typecheck and before the tests. + - name: Build (packages - dist for the type declarations) + run: pnpm --filter "./packages/*" -r build + - name: Typecheck run: pnpm typecheck @@ -87,7 +97,7 @@ jobs: - name: Build (mcp package — dist for the smoke imports) run: pnpm --filter @coreci/mcp build - - name: [G-018,R-008] Import guard (no @coreci/llm-mock in prod source) + - name: "[G-018,R-008] Import guard (no @coreci/llm-mock in prod source)" run: pnpm check:llm-mock-guard - name: Unit + integration tests (PGlite) @@ -169,7 +179,6 @@ jobs: - name: Setup pnpm ${{ env.PNPM_VERSION }} uses: pnpm/action-setup@v4 with: - version: ${{ env.PNPM_VERSION }} run_install: false - name: Cache pnpm store @@ -182,6 +191,12 @@ jobs: - name: Install dependencies run: pnpm install --frozen-lockfile + # Consumers resolve @coreci/db and @coreci/secrets through their + # published dist/*.d.ts, so the packages must be built (pnpm runs + # them in dependency order) before typecheck and before the tests. + - name: Build (packages - dist for the type declarations) + run: pnpm --filter "./packages/*" -r build + - name: Install the postgres client (the runner image carries no psql) run: | apt-get update -qq && apt-get install -y -qq postgresql-client @@ -193,27 +208,27 @@ jobs: echo "DATABASE_URL=postgres://coreci_app:coreci_app_ci@$HOST:5432/coreci_ci" >> "$GITHUB_ENV" echo "CI_MIGRATOR_DATABASE_URL=postgres://migrator:migrator_ci@$HOST:5432/coreci_ci" >> "$GITHUB_ENV" - - name: [R-009] Setup CI roles (coreci_app NOBYPASSRLS, migrator BYPASSRLS) + - name: "[R-009] Setup CI roles (coreci_app NOBYPASSRLS, migrator BYPASSRLS)" run: | psql -h "$CI_DB_HOST" -U postgres -d postgres -f packages/db/scripts/setup-ci-roles.sql - - name: [G-022] Run migrations as migrator (BYPASSRLS) + - name: "[G-022] Run migrations as migrator (BYPASSRLS)" run: | DATABASE_URL="$CI_MIGRATOR_DATABASE_URL" pnpm --filter @coreci/db migrate - - name: [G-022] Build mcp (dist for smoke imports) + - name: "[G-022] Build mcp (dist for smoke imports)" run: pnpm --filter @coreci/mcp build - - name: [G-022] Full M1 + M2 test suite against real Postgres 16 + - name: "[G-022] Full M1 + M2 test suite against real Postgres 16" # The tests read DB_MODE=pg + DATABASE_URL (coreci_app role, RLS # enforced). The pen test's WITH CHECK assertion (R-009) is REAL here # — a cross-tenant INSERT is rejected by the RLS policy. run: pnpm test - - name: [G-022] MCP conformance + LLM smoke (Track A only — no PAT in pg job) + - name: "[G-022] MCP conformance + LLM smoke (Track A only — no PAT in pg job)" run: pnpm test:conformance - - name: [G-022] DB pen test (real RLS WITH CHECK enforcement, R-009) + - name: "[G-022] DB pen test (real RLS WITH CHECK enforcement, R-009)" run: pnpm --filter @coreci/db test:pen - name: Upload coverage artifacts diff --git a/apps/control-plane/app/layout.tsx b/apps/control-plane/app/layout.tsx new file mode 100644 index 0000000..c482c82 --- /dev/null +++ b/apps/control-plane/app/layout.tsx @@ -0,0 +1,27 @@ +/** + * / - App Router root layout (coreci-chat#10). + * + * Next.js 15 requires a root layout that renders and above + * every route. Without one, `next build` fails with "login/page.tsx does not + * have a root layout", so apps/control-plane has never been buildable - the + * pipeline never built the app, so nothing caught it. + * + * This is the build-enabling minimum only. The shell content (theme, + * providers, session wrapper) is an M3 scope call for the line head and is + * deliberately NOT decided here - it rides the M3 kickoff verdict on + * coreci-chat#1. + */ + +import type { ReactNode } from "react"; + +export const metadata = { + title: "CoreCI Chat - control plane", +}; + +export default function RootLayout({ children }: { children: ReactNode }) { + return ( + + {children} + + ); +} diff --git a/apps/control-plane/lib/db.ts b/apps/control-plane/lib/db.ts index 44a06f8..e64abdf 100644 --- a/apps/control-plane/lib/db.ts +++ b/apps/control-plane/lib/db.ts @@ -31,6 +31,19 @@ export async function getDb(): Promise { } async function runMigrations(db: DbClient): Promise { + // The runtime role owns no tables. The migrations are applied by the + // migrator role (BYPASSRLS) - `pnpm migrate` in CI, the deploy step in prod - + // and the app role cannot DDL: the migration files CREATE tables, indexes and + // RLS policies, and those need ownership. The app role only gets DML. So the + // boot-time re-run is skipped when the schema is already there; it stays the + // dev convenience it was meant to be (a fresh PGlite database has no schema + // yet). Without this, the real-Postgres CI job red on "permission denied for + // schema public" as soon as the control-plane suites booted the app. + const { rows } = await db.query<{ applied: boolean }>( + "SELECT EXISTS (SELECT 1 FROM information_schema.tables WHERE table_schema = 'public' AND table_name = 'tenants') AS applied", + ); + if (rows[0]?.applied) return; + // Migrations live in packages/db/migrations. Resolve via the symlinked // @coreci/db workspace package (apps/control-plane/node_modules/@coreci/db // → packages/db). Falls back to a monorepo-relative path in dev. diff --git a/apps/control-plane/tests/mcp-routes.test.ts b/apps/control-plane/tests/mcp-routes.test.ts index 608eb23..eed70ee 100644 --- a/apps/control-plane/tests/mcp-routes.test.ts +++ b/apps/control-plane/tests/mcp-routes.test.ts @@ -135,10 +135,15 @@ beforeAll(async () => { }); beforeEach(async () => { - // Wipe mcp_adapters between tests (disable RLS to mutate, re-enable). - await db.query("ALTER TABLE mcp_adapters DISABLE ROW LEVEL SECURITY"); - await db.query("DELETE FROM mcp_adapters"); - await db.query("ALTER TABLE mcp_adapters ENABLE ROW LEVEL SECURITY"); + // Wipe mcp_adapters between tests. The runtime role (coreci_app) owns no + // tables, so it cannot ALTER the table's RLS - that is why the old wipe + // died with "must be owner of table mcp_adapters" and took all 15 tests + // with it (coreci-chat#12). The wipe is a tenant-scoped DELETE inside + // withTenant: the app role holds the DELETE privilege and the RLS policy + // scopes the rows to the session's tenant. + await withTenant(admin!.tenantId, async (c: ScopedClient) => { + await c.query("DELETE FROM mcp_adapters"); + }); // Reset the stream manager between tests so contexts don't leak. const { streamManager } = await getMcpRuntime(); streamManager.reset(); diff --git a/packages/auth/src/provisioning.ts b/packages/auth/src/provisioning.ts index f5df260..16de3af 100644 --- a/packages/auth/src/provisioning.ts +++ b/packages/auth/src/provisioning.ts @@ -80,41 +80,51 @@ export async function provisionTenant( createdUser = true; } - // 3. Resolve-or-create the membership (tenant-scoped). - const membership = await resolveMembership(db, tenant.id, user.id); - if (!membership) { - const role: "admin" | "operator" | "viewer" = createdTenant ? "admin" : "viewer"; - await withTenant(tenant.id, async (c: ScopedClient) => { - await c.query( - `INSERT INTO tenant_memberships (tenant_id, user_id, role) - VALUES ($1, $2, $3)`, - [tenant.id, user.id, role], - ); - // Audit: provision event (only when we actually created something). - await appendAudit(c, { - tenantId: tenant.id, - eventType: "provision", - payload: { - workosOrgId, - workosUserId, - email, - role, - createdTenant, - createdUser, - }, - userId: user.id, - }); + // 3. Resolve-or-create the membership (tenant-scoped). The lookup runs + // INSIDE the tenant scope on purpose: under RLS a statement outside a + // scope sees zero rows, so resolving outside always missed an existing + // membership and re-inserted it (the unique constraint on + // (tenant_id, user_id) then rejected the second call). It also keeps the + // statement off the empty-scope crash path: casting an unset scope to + // uuid raised `invalid input syntax for type uuid: ""` under real + // Postgres (coreci-chat#13). + const role: "admin" | "operator" | "viewer" = createdTenant ? "admin" : "viewer"; + const membership = await withTenant(tenant.id, async (c: ScopedClient) => { + const found = await resolveMembership(c, tenant.id, user.id); + if (found) return { role: found.role, created: false }; + await c.query( + `INSERT INTO tenant_memberships (tenant_id, user_id, role) + VALUES ($1, $2, $3)`, + [tenant.id, user.id, role], + ); + // Audit: provision event (only when we actually created something). + await appendAudit(c, { + tenantId: tenant.id, + eventType: "provision", + payload: { + workosOrgId, + workosUserId, + email, + role, + createdTenant, + createdUser, + }, + userId: user.id, }); + return { role, created: true }; + }); + + if (membership.created) { return { tenantId: tenant.id, userId: user.id, - role, + role: membership.role, createdTenant, createdUser, }; } - // Both exist + membership exists → idempotent. No audit (nothing changed). + // Both exist + membership exists -> idempotent. No audit (nothing changed). return { tenantId: tenant.id, userId: user.id, @@ -123,7 +133,6 @@ export async function provisionTenant( createdUser: false, }; } - async function resolveTenantByOrg(db: ProvisionDb, workosOrgId: string): Promise { const res = await db.query( "SELECT id, name FROM tenants WHERE workos_org_id = $1", diff --git a/packages/db/migrations/0004_rls_scope_hardening.sql b/packages/db/migrations/0004_rls_scope_hardening.sql new file mode 100644 index 0000000..12b7aeb --- /dev/null +++ b/packages/db/migrations/0004_rls_scope_hardening.sql @@ -0,0 +1,65 @@ +-- 0004_rls_scope_hardening.sql - RLS tenant-scope hardening (coreci-chat#13). +-- +-- The tenant-scoped policies created by 0001_init.sql and 0003_mcp_adapters.sql +-- cast the session's tenant scope straight to uuid: +-- +-- USING (tenant_id = current_setting('app.tenant_id', true)::uuid) +-- +-- `current_setting(name, missing_ok := true)` returns NULL when the setting was +-- never set in this session - but it returns an EMPTY STRING, not NULL, once +-- the session holds a placeholder for it. Postgres creates that placeholder the +-- first time a transaction-local `set_config('app.tenant_id', ..., true)` runs +-- (that is withTenant), and when that transaction ends the placeholder's value +-- is ''. So on any pooled connection that has already run a withTenant +-- transaction, the NEXT statement that touches a tenant-scoped table casts '' +-- to uuid and the database raises: +-- +-- invalid input syntax for type uuid: "" +-- +-- That is the crash the control-plane dashboard suite hit under real Postgres +-- (run 55595, job 57855: 16 assertions, provisioning.ts:177 via :84). PGlite +-- never showed it because PGlite 0.5.7 does not enforce RLS on SELECT, so the +-- policy expression is never evaluated there. +-- +-- The fix keeps the documented semantics ("a query outside withTenant sees +-- zero tenant-scoped rows") and removes the crash: nullif(..., '') maps the +-- empty scope to NULL, so the comparison is NULL - not true - and the row is +-- filtered out instead of the statement dying. +-- +-- DROP + CREATE keeps this file idempotent: re-running it replaces the +-- policies instead of failing on "policy already exists". The migrator role +-- owns the tables, so it can drop and recreate them. +-- +-- Cross-tenant behaviour is unchanged: a row whose tenant_id differs from the +-- session's scope still fails the WITH CHECK clause - the R-009 pen test +-- asserts exactly that. + +DROP POLICY IF EXISTS tenant_isolation_tenant_memberships ON tenant_memberships; +CREATE POLICY tenant_isolation_tenant_memberships ON tenant_memberships + USING (tenant_id = nullif(current_setting('app.tenant_id', true), '')::uuid) + WITH CHECK (tenant_id = nullif(current_setting('app.tenant_id', true), '')::uuid); + +DROP POLICY IF EXISTS tenant_isolation_invitations ON invitations; +CREATE POLICY tenant_isolation_invitations ON invitations + USING (tenant_id = nullif(current_setting('app.tenant_id', true), '')::uuid) + WITH CHECK (tenant_id = nullif(current_setting('app.tenant_id', true), '')::uuid); + +DROP POLICY IF EXISTS tenant_isolation_targets ON targets; +CREATE POLICY tenant_isolation_targets ON targets + USING (tenant_id = nullif(current_setting('app.tenant_id', true), '')::uuid) + WITH CHECK (tenant_id = nullif(current_setting('app.tenant_id', true), '')::uuid); + +DROP POLICY IF EXISTS tenant_isolation_byom_endpoints ON byom_endpoints; +CREATE POLICY tenant_isolation_byom_endpoints ON byom_endpoints + USING (tenant_id = nullif(current_setting('app.tenant_id', true), '')::uuid) + WITH CHECK (tenant_id = nullif(current_setting('app.tenant_id', true), '')::uuid); + +DROP POLICY IF EXISTS tenant_isolation_audit_log ON audit_log; +CREATE POLICY tenant_isolation_audit_log ON audit_log + USING (tenant_id = nullif(current_setting('app.tenant_id', true), '')::uuid) + WITH CHECK (tenant_id = nullif(current_setting('app.tenant_id', true), '')::uuid); + +DROP POLICY IF EXISTS tenant_isolation_mcp_adapters ON mcp_adapters; +CREATE POLICY tenant_isolation_mcp_adapters ON mcp_adapters + USING (tenant_id = nullif(current_setting('app.tenant_id', true), '')::uuid) + WITH CHECK (tenant_id = nullif(current_setting('app.tenant_id', true), '')::uuid); diff --git a/packages/db/scripts/setup-ci-roles.sql b/packages/db/scripts/setup-ci-roles.sql index d513055..04f1ed2 100644 --- a/packages/db/scripts/setup-ci-roles.sql +++ b/packages/db/scripts/setup-ci-roles.sql @@ -37,6 +37,16 @@ BEGIN END IF; END $$; +-- Create the CI database FIRST (the service container's default is `postgres`; +-- the migrations and the test run happen in `coreci_ci`). The grants below are +-- per-database, so they have to be applied in the database the tests use. +SELECT 'CREATE DATABASE coreci_ci OWNER migrator' +WHERE NOT EXISTS (SELECT FROM pg_database WHERE datname = 'coreci_ci')\gexec +-- Switch to that database. The script connects to `postgres` first (the roles +-- are cluster-level and the database does not exist yet); schema and table +-- privileges granted in `postgres` do not reach the tables in `coreci_ci`. +\connect coreci_ci + -- Grant schema + table privileges. The migrator creates tables; the app -- role gets DML (INSERT/SELECT/UPDATE/DELETE) on the tables it owns. RLS -- policies enforce tenant scoping (the WITH CHECK clause blocks cross-tenant @@ -47,9 +57,11 @@ GRANT CREATE ON SCHEMA public TO migrator; -- The app role gets DML on all current + future tables in public. The -- migrations CREATE TABLE with no explicit owner (migrator owns them); the -- app role connects and queries/inserts under RLS. -ALTER DEFAULT PRIVILEGES IN SCHEMA public +-- The default privileges are set FOR ROLE migrator: set as another +-- role they would not apply to the tables the migrations create. +ALTER DEFAULT PRIVILEGES FOR ROLE migrator IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON TABLES TO coreci_app; -ALTER DEFAULT PRIVILEGES IN SCHEMA public +ALTER DEFAULT PRIVILEGES FOR ROLE migrator IN SCHEMA public GRANT USAGE ON SEQUENCES TO coreci_app; -- For tables created by migrations BEFORE this grant took effect, apply @@ -64,8 +76,3 @@ BEGIN END LOOP; END $$; --- Create the CI database (the service container's default is `postgres`; --- the CI workflow creates a separate `coreci_ci` database for the test run). --- This is optional — the workflow may set DATABASE_URL to point at any DB. -SELECT 'CREATE DATABASE coreci_ci OWNER migrator' -WHERE NOT EXISTS (SELECT FROM pg_database WHERE datname = 'coreci_ci')\gexec \ No newline at end of file diff --git a/packages/db/src/create-db.ts b/packages/db/src/create-db.ts index cc07213..422168f 100644 --- a/packages/db/src/create-db.ts +++ b/packages/db/src/create-db.ts @@ -49,13 +49,44 @@ export async function createDb(opts: CreateDbOptions = {}): Promise { client.release(); } }, + // One connection for the whole transaction (coreci-chat#14): a pool + // hands out a different connection per statement, so a transaction-local + // setting (the tenant scope SET LOCAL app.tenant_id) would otherwise + // be set on one connection and read on another. + async transaction(fn: (client: DbClient) => Promise): Promise { + const client = await pool.connect(); + const tx: DbClient = { + async query>(text: string, params?: unknown[]) { + const res = await client.query(text, params); + return { rows: res.rows as R[], rowCount: res.rowCount ?? 0 }; + }, + async exec(text: string) { + await client.query(text); + }, + }; + try { + await client.query("BEGIN"); + const out = await fn(tx); + await client.query("COMMIT"); + return out; + } catch (err) { + try { + await client.query("ROLLBACK"); + } catch { + /* ignore rollback failure; the original error is more important */ + } + throw err; + } finally { + client.release(); + } + }, }; } // PGlite (dev/test). Real Postgres in WASM, supports RLS + triggers. const { PGlite } = await import("@electric-sql/pglite"); const pg = await PGlite.create(); - return { + const single: DbClient = { async query>(text: string, params?: unknown[]) { const res = await pg.query(text, params); return { rows: res.rows as T[], rowCount: (res.affectedRows ?? res.rows.length) }; @@ -64,4 +95,24 @@ export async function createDb(opts: CreateDbOptions = {}): Promise { await pg.exec(text); }, }; + return { + ...single, + // PGlite is a single in-process connection: the transaction and the + // tenant scope always land on it. + async transaction(fn: (client: DbClient) => Promise): Promise { + await pg.query("BEGIN"); + try { + const out = await fn(single); + await pg.query("COMMIT"); + return out; + } catch (err) { + try { + await pg.query("ROLLBACK"); + } catch { + /* ignore rollback failure; the original error is more important */ + } + throw err; + } + }, + }; } \ No newline at end of file diff --git a/packages/db/src/db-client.ts b/packages/db/src/db-client.ts index bc59f4d..e91c3c2 100644 --- a/packages/db/src/db-client.ts +++ b/packages/db/src/db-client.ts @@ -16,4 +16,14 @@ export interface DbClient { query>(text: string, params?: unknown[]): Promise>; /** Run multi-statement SQL (DDL, migrations). No params. Use for migrations only. */ exec(text: string): Promise; + /** + * Run `fn` inside one transaction on ONE connection. + * + * A pooled client can hand out a different connection per statement, so a + * transaction-local setting (the tenant scope `SET LOCAL app.tenant_id`) + * only holds if every statement of the transaction shares a connection. + * Optional: a client that cannot pin a connection omits it, and withTenant + * keeps its own BEGIN/COMMIT path as the fallback. + */ + transaction?(fn: (client: DbClient) => Promise): Promise; } \ No newline at end of file diff --git a/packages/db/src/withTenant.ts b/packages/db/src/withTenant.ts index 6f6e42e..3fa5eae 100644 --- a/packages/db/src/withTenant.ts +++ b/packages/db/src/withTenant.ts @@ -6,7 +6,9 @@ * outside withTenant() returns zero rows (the RLS policy evaluates NULL = X → false). * * In PGlite (single-connection), BEGIN/COMMIT work on the single connection. - * In pg (pool), withTenant acquires a dedicated client for the transaction. + * In pg (pool), withTenant runs the transaction on a dedicated client that the + * client pins for the whole transaction (`DbClient.transaction`), so the tenant + * scope and every statement of the transaction share one connection. * * For simplicity in M1, this implementation holds a single client (set via * `setDbClient`). The control plane's API gateway will call setDbClient at boot. @@ -47,23 +49,44 @@ export async function withTenant( fn: (client: ScopedClient) => Promise, ): Promise { const c = client(); - const scoped: ScopedClient = { - async query(text, params) { - return c.query(text, params); - }, - }; try { - await c.query("BEGIN"); - await c.query("SELECT set_config('app.tenant_id', $1, true)", [tenantId]); - const result = await fn(scoped); - await c.query("COMMIT"); - return result; - } catch (err) { - try { - await c.query("ROLLBACK"); - } catch { - /* ignore rollback failure; original error is more important */ + if (typeof c.transaction === "function") { + // The client can pin ONE connection for the whole transaction, so the + // tenant scope (SET LOCAL) and every statement in `fn` share it. A pool + // otherwise hands out a different connection per statement, and the + // scope is then set on one connection and read on another + // (coreci-chat#14). + return await c.transaction(async (tx) => { + await tx.query("SELECT set_config('app.tenant_id', $1, true)", [tenantId]); + return fn({ + async query(text, params) { + return tx.query(text, params); + }, + }); + }); } + // Fallback for a client that cannot pin a connection: the scope and the + // statements still share the client's own connection. + const scoped: ScopedClient = { + async query(text, params) { + return c.query(text, params); + }, + }; + try { + await c.query("BEGIN"); + await c.query("SELECT set_config('app.tenant_id', $1, true)", [tenantId]); + const result = await fn(scoped); + await c.query("COMMIT"); + return result; + } catch (err) { + try { + await c.query("ROLLBACK"); + } catch { + /* ignore rollback failure; original error is more important */ + } + throw err; + } + } catch (err) { throw new TenantContextError( `withTenant(${tenantId}) failed: ${err instanceof Error ? err.message : String(err)}`, err, diff --git a/packages/db/tests/pen/cross-tenant.test.ts b/packages/db/tests/pen/cross-tenant.test.ts index fff2b27..281bcf7 100644 --- a/packages/db/tests/pen/cross-tenant.test.ts +++ b/packages/db/tests/pen/cross-tenant.test.ts @@ -38,6 +38,9 @@ const T1 = "00000000-0000-0000-0000-000000000001"; const T2 = "00000000-0000-0000-0000-000000000002"; const U1 = "00000000-0000-0000-0000-000000000011"; const U2 = "00000000-0000-0000-0000-000000000012"; +/** Hostnames seeded by this file. No other suite uses them. */ +const T1_HOST = "t1-host"; +const T2_HOST = "t2-host"; /** DB_MODE env: 'pg' → real Postgres 16 (CI); unset → PGlite (dev). [G-022] */ const isPgMode = process.env.DB_MODE === "pg"; @@ -65,9 +68,10 @@ describe("cross-tenant isolation (REQ-039 pen test)", () => { // outside a tenant scope — RLS WITH CHECK rejects it). } // Seed two tenants + users + memberships + one target each. - // In PG mode (coreci_app role), the tenants/users/memberships tables are - // NOT tenant-scoped (they're the bootstrap tables), so direct inserts - // work. The targets table IS tenant-scoped — seed via withTenant so the + // In PG mode (coreci_app role) the tenants and users tables are the + // bootstrap tables and are not tenant-scoped, so direct inserts work. + // tenant_memberships IS tenant-scoped (the M1 migration enables AND forces + // RLS on it) and so is targets — both are seeded via withTenant so the // RLS WITH CHECK passes. await db.query( `INSERT INTO tenants (id, name) VALUES ($1,'T1'), ($2,'T2') ON CONFLICT DO NOTHING`, @@ -77,10 +81,38 @@ describe("cross-tenant isolation (REQ-039 pen test)", () => { `INSERT INTO users (id, email) VALUES ($1,'u1@t1.test'), ($2,'u2@t2.test') ON CONFLICT DO NOTHING`, [U1, U2], ); - await db.query( - `INSERT INTO tenant_memberships (tenant_id, user_id, role) VALUES ($1,$2,'admin'), ($3,$4,'admin') ON CONFLICT DO NOTHING`, - [T1, U1, T2, U2], - ); + if (isPgMode) { + await withTenant(T1, async (c) => { + await c.query( + `INSERT INTO tenant_memberships (tenant_id, user_id, role) VALUES ($1,$2,'admin') ON CONFLICT DO NOTHING`, + [T1, U1], + ); + }); + await withTenant(T2, async (c) => { + await c.query( + `INSERT INTO tenant_memberships (tenant_id, user_id, role) VALUES ($1,$2,'admin') ON CONFLICT DO NOTHING`, + [T2, U2], + ); + }); + } else { + await db.query( + `INSERT INTO tenant_memberships (tenant_id, user_id, role) VALUES ($1,$2,'admin'), ($3,$4,'admin') ON CONFLICT DO NOTHING`, + [T1, U1, T2, U2], + ); + } + // The seed must be idempotent. This file runs TWICE in the CI postgres + // job — once inside `pnpm test`, once in the `test:pen` step — against + // ONE database, and `targets` has no unique constraint on (tenant_id, + // hostname), so `ON CONFLICT DO NOTHING` does NOT dedupe: the second run + // would add a second 't1-host' row and the row counts below would grow. + // Delete this file's own rows first (the app role holds DELETE; RLS + // scopes the delete to the tenant). + await withTenant(T1, async (c) => { + await c.query("DELETE FROM targets WHERE hostname = $1", [T1_HOST]); + }); + await withTenant(T2, async (c) => { + await c.query("DELETE FROM targets WHERE hostname = $1", [T2_HOST]); + }); // Seed targets via withTenant (RLS WITH CHECK requires the row's // tenant_id to match the current app.tenant_id). await withTenant(T1, async (c) => { @@ -107,9 +139,14 @@ describe("cross-tenant isolation (REQ-039 pen test)", () => { ); return res.rows; }); - expect(rows).toHaveLength(1); - expect(rows[0]?.hostname).toBe("t1-host"); + // The row COUNT is not this test's to assert: the CI postgres job runs + // every workspace suite against ONE database and the relay-ws suites + // register targets for these same two tenant ids. The ISOLATION property + // is what is asserted here: this tenant's own seeded row is visible, + // every visible row belongs to T1, and T2's seeded row is not among them. + expect(rows.filter((r) => r.hostname === T1_HOST)).toHaveLength(1); expect(rows.every((r) => r.tenant_id === T1)).toBe(true); + expect(rows.some((r) => r.hostname === T2_HOST)).toBe(false); }); it("T2 sees only T2 targets, not T1", async () => { @@ -120,8 +157,9 @@ describe("cross-tenant isolation (REQ-039 pen test)", () => { ); return res.rows; }); - expect(rows).toHaveLength(1); - expect(rows[0]?.hostname).toBe("t2-host"); + expect(rows.filter((r) => r.hostname === T2_HOST)).toHaveLength(1); + expect(rows.every((r) => r.tenant_id === T2)).toBe(true); + expect(rows.some((r) => r.hostname === T1_HOST)).toBe(false); }); it("a query scoped to T1 cannot read T2's targets by ID", async () => { @@ -135,13 +173,35 @@ describe("cross-tenant isolation (REQ-039 pen test)", () => { ); return res.rows; }); - expect(rows).toHaveLength(1); - expect(rows[0]?.hostname).toBe("t1-host"); + expect(rows.filter((r) => r.hostname === T1_HOST)).toHaveLength(1); + expect(rows.some((r) => r.hostname === T2_HOST)).toBe(false); // In prod (real Postgres), RLS would block even a bare `SELECT * FROM targets` // without the WHERE clause. PGlite 0.5.7 doesn't enforce RLS on SELECT, // so the application-layer WHERE is the primary enforcement in dev/test. }); + it("RLS hides the other tenant's rows from a bare SELECT (real Postgres only)", async () => { + // PGlite 0.5.7 does not enforce RLS on SELECT, so this assertion only + // runs against real Postgres. This is the R-009 property in its purest + // form: inside withTenant(T1), with NO application-layer WHERE clause, + // the DATABASE returns only T1's rows. It also proves the tenant scope + // reached the connection that ran the statement (coreci-chat#14): a + // scope set on another connection would show no rows at all. + if (!isPgMode) { + expect(true).toBe(true); // PGlite: RLS on SELECT is not enforced + return; + } + const rows = await withTenant(T1, async (c) => { + const res = await c.query<{ tenant_id: string; hostname: string }>( + "SELECT tenant_id, hostname FROM targets", + ); + return res.rows; + }); + expect(rows.length).toBeGreaterThan(0); + expect(rows.every((r) => r.tenant_id === T1)).toBe(true); + expect(rows.some((r) => r.hostname === T2_HOST)).toBe(false); + }); + it("a query OUTSIDE withTenant() has no active tenant context", async () => { const ctx = await getTenantContext(); expect(ctx).toBe(null); // no active tenant context → prod RLS returns nothing diff --git a/packages/db/tests/withTenant.test.ts b/packages/db/tests/withTenant.test.ts new file mode 100644 index 0000000..1a59088 --- /dev/null +++ b/packages/db/tests/withTenant.test.ts @@ -0,0 +1,128 @@ +/** + * withTenant — the tenant scope must live on ONE connection (coreci-chat#14). + * + * In pg mode the client is a POOL: a pool hands out a different connection + * per statement, so `SET LOCAL app.tenant_id` (transaction-local) only holds + * if the scope and the statements share a connection. A client that can pin + * one connection exposes `transaction()`; withTenant uses it when it is there + * and keeps its BEGIN/COMMIT path as the fallback for clients that cannot. + */ + +import { describe, it, expect } from "vitest"; +import { setDbClient, withTenant, TenantContextError } from "../src/withTenant.js"; +import type { DbClient } from "../src/db-client.js"; + +type Recorder = { + client: DbClient; + /** statements that went through the pooled client (where a scope gets lost) */ + pooled: string[]; + /** statements that ran inside the pinned transaction */ + pinned: string[]; +}; + +function recorder(withTransaction: boolean): Recorder { + const pooled: string[] = []; + const pinned: string[] = []; + + const pooledClient: DbClient = { + async query>(text: string) { + pooled.push(text); + return { rows: [] as T[], rowCount: 0 }; + }, + async exec() { + /* not used here */ + }, + }; + + const client: DbClient = withTransaction + ? { + ...pooledClient, + async transaction(fn: (c: DbClient) => Promise): Promise { + pinned.push("BEGIN"); + const tx: DbClient = { + async query>(text: string) { + pinned.push(text); + return { rows: [] as R[], rowCount: 0 }; + }, + async exec() { + /* not used here */ + }, + }; + try { + const out = await fn(tx); + pinned.push("COMMIT"); + return out; + } catch (err) { + pinned.push("ROLLBACK"); + throw err; + } + }, + } + : pooledClient; + + return { client, pooled, pinned }; +} + +describe("withTenant — tenant scope pinning (coreci-chat#14)", () => { + it("runs the scope and the statements on the transaction's connection", async () => { + const r = recorder(true); + setDbClient(r.client); + + const out = await withTenant("tenant-a", async (c) => { + await c.query("SELECT 1"); + return "ok"; + }); + + expect(out).toBe("ok"); + expect(r.pinned).toEqual([ + "BEGIN", + "SELECT set_config('app.tenant_id', $1, true)", + "SELECT 1", + "COMMIT", + ]); + // Nothing touched the pooled client: a pool statement would have taken a + // different connection and lost the scope. + expect(r.pooled).toEqual([]); + }); + + it("falls back to BEGIN/COMMIT when the client cannot pin a connection", async () => { + const r = recorder(false); + setDbClient(r.client); + + await withTenant("tenant-a", async (c) => { + await c.query("SELECT 1"); + }); + + expect(r.pooled).toEqual([ + "BEGIN", + "SELECT set_config('app.tenant_id', $1, true)", + "SELECT 1", + "COMMIT", + ]); + expect(r.pinned).toEqual([]); + }); + + it("rolls the transaction back and wraps the failure in TenantContextError", async () => { + const r = recorder(true); + setDbClient(r.client); + + await expect( + withTenant("tenant-a", async () => { + throw new Error("boom"); + }), + ).rejects.toBeInstanceOf(TenantContextError); + expect(r.pinned).toContain("ROLLBACK"); + }); + + it("rolls the fallback path back too", async () => { + const r = recorder(false); + setDbClient(r.client); + + await expect( + withTenant("tenant-a", async () => { + throw new Error("boom"); + }), + ).rejects.toBeInstanceOf(TenantContextError); + expect(r.pooled).toContain("ROLLBACK"); + }); +});