Stop a migration when the read-only roles can still read a withheld table

packages/postgres/migrations/1792193305492_check-readonly-role-cant-read-withheld-tables.js

@@ -0,0 +1,74 @@
+// Checks that the read-only role used by dashboards and read-only database
+// access, and every role that reads through it, can't read the tables withheld
+// from it: `proxy_endpoints` and `incoming_webhooks`, which hold the platform's
+// vendor keys and the secrets that verify incoming webhooks, and `user_keys`
+// and `key_write_tickets`. The migrations that create or seal those tables
+// revoke the SELECT grants made to the role and its members by name. SELECT
+// granted to PUBLIC, or to another role a member also belongs to, still lets
+// them read, and revoking it isn't a migration's call. So `up` asks
+// `has_column_privilege` about every column of each table and stops, naming
+// the role and the column, if any of those roles can still read one.
+//
+// The roles checked are `readonly_role` and every role that inherits its
+// privileges or can SET ROLE to it, directly or through other roles. A member
+// holding only the ADMIN option reads nothing through the role (on Postgres 16
+// and later a non-superuser that creates `readonly_role` holds that membership), and
+// a table's owner reads it by owning it, so neither is checked. The check
+// covers what each role reads as itself; a member that can SET ROLE to some
+// other role reads as that role.
+//
+// `readonly_role` exists only where it was provisioned (staging and
+// production), so the check is skipped wherever it doesn't. The migration
+// changes nothing, so `down` has nothing to undo.
+
+exports.up = (pgm) => {
+  pgm.sql(`
+    DO $$
+    DECLARE
+      readable_by name;
+      readable_table name;
+      readable_column name;
+    BEGIN
+      IF EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'readonly_role') THEN
+        WITH RECURSIVE reader (oid) AS (
+          SELECT oid FROM pg_roles WHERE rolname = 'readonly_role'
+          UNION
+          SELECT membership.member
+          FROM pg_auth_members membership
+          JOIN reader ON reader.oid = membership.roleid
+          WHERE membership.inherit_option OR membership.set_option
+        )
+        SELECT reader_role.rolname, tbl.relname, col.attname
+          INTO readable_by, readable_table, readable_column
+          FROM reader
+          JOIN pg_roles reader_role ON reader_role.oid = reader.oid
+          CROSS JOIN pg_class tbl
+          JOIN pg_attribute col ON col.attrelid = tbl.oid
+          WHERE tbl.oid IN (
+              to_regclass('proxy_endpoints'),
+              to_regclass('incoming_webhooks'),
+              to_regclass('user_keys'),
+              to_regclass('key_write_tickets')
+            )
+            AND reader.oid <> tbl.relowner
+            AND col.attnum > 0
+            AND NOT col.attisdropped
+            AND has_column_privilege(reader.oid, tbl.oid, col.attnum, 'SELECT')
+          ORDER BY tbl.relname, reader_role.rolname, col.attnum
+          LIMIT 1;
+        IF FOUND THEN
+          RAISE EXCEPTION
+            '% can read %.%, which is withheld from readonly_role and the roles that read through it',
+            readable_by, readable_table, readable_column
+            USING HINT = format(
+              'Revoke the grant or role membership that lets %I read %I.%I, then run the migration again.',
+              readable_by, readable_table, readable_column
+            );
+        END IF;
+      END IF;
+    END
+    $$
+  `);
+};
+
+exports.down = () => {};

packages/realm-server/tests/readonly-role-withheld-tables-test.ts

@@ -0,0 +1,201 @@
+import QUnit from 'qunit';
+const { module, test } = QUnit;
+import { basename, dirname, join } from 'path';
+import { createRequire } from 'module';
+import { setupDB } from './helpers/index.ts';
+
+const require = createRequire(import.meta.url);
+const CHECK_MIGRATION = join(
+  dirname(require.resolve('@cardstack/postgres/package.json')),
+  'migrations',
+  '1792193305492_check-readonly-role-cant-read-withheld-tables.js',
+);
+
+const WITHHELD_TABLES = [
+  'proxy_endpoints',
+  'incoming_webhooks',
+  'user_keys',
+  'key_write_tickets',
+];
+
+// Roles and memberships are shared by every database on the server, so each
+// test removes the memberships it makes, whatever happens.
+const MEMBER = 'readonly_member_for_withheld_tables_test';
+const GROUP = 'readonly_group_for_withheld_tables_test';
+const LEAF = 'readonly_leaf_for_withheld_tables_test';
+const ADMIN_MEMBER = 'readonly_admin_member_for_withheld_tables_test';
+const OWNER = 'readonly_owner_for_withheld_tables_test';
+const OTHER_ROLE = 'other_role_for_withheld_tables_test';
+const OUTSIDER = 'outsider_for_withheld_tables_test';
+
+module(basename(import.meta.filename), function () {
+  module('the read-only role check', function (hooks) {
+    let dbAdapter: any;
+
+    setupDB(hooks, {
+      beforeEach: async (_dbAdapter) => {
+        dbAdapter = _dbAdapter;
+        // The role and its blanket grant as staging and production have them,
+        // with SELECT revoked on the withheld tables as the migrations that
+        // create or seal them leave it.
+        await createRole('readonly_role');
+        await dbAdapter.execute(
+          `GRANT SELECT ON ALL TABLES IN SCHEMA public TO readonly_role`,
+        );
+        await dbAdapter.execute(
+          `REVOKE SELECT ON TABLE ${WITHHELD_TABLES.join(', ')} FROM readonly_role`,
+        );
+      },
+    });
+
+    async function createRole(name: string) {
+      await dbAdapter.execute(
+        `DO $$
+         BEGIN
+           CREATE ROLE ${name};
+         EXCEPTION WHEN duplicate_object OR unique_violation THEN NULL;
+         END
+         $$`,
+      );
+    }
+
+    async function runUp() {
+      let statements: string[] = [];
+      require(CHECK_MIGRATION).up({
+        sql: (statement: string) => statements.push(statement),
+      });
+      for (let statement of statements) {
+        await dbAdapter.execute(statement);
+      }
+    }
+
+    async function withMemberships(
+      memberships: [role: string, member: string, options?: string][],
+      fn: () => Promise<void>,
+    ) {
+      for (let [role, member] of memberships) {
+        await createRole(role);
+        await createRole(member);
+      }
+      try {
+        for (let [role, member, options] of memberships) {
+          await dbAdapter.execute(
+            `GRANT ${role} TO ${member}${options ? ` WITH ${options}` : ''}`,
+          );
+        }
+        await fn();
+      } finally {
+        for (let [role, member] of [...memberships].reverse()) {
+          await dbAdapter.execute(`REVOKE ${role} FROM ${member}`);
+        }
+      }
+    }
+
+    test('passes when only a role outside the read-only role reads the tables', async function (assert) {
+      await createRole(OUTSIDER);
+      await dbAdapter.execute(
+        `GRANT SELECT ON TABLE ${WITHHELD_TABLES.join(', ')} TO ${OUTSIDER}`,
+      );
+
+      await runUp();
+
+      assert.ok(true, 'up completes');
+    });
+
+    test('stops when PUBLIC can read a column', async function (assert) {
+      await dbAdapter.execute(
+        `GRANT SELECT (api_key) ON TABLE proxy_endpoints TO PUBLIC`,
+      );
+
+      await assert.rejects(
+        runUp(),
+        /can read proxy_endpoints\.api_key, which is withheld from readonly_role/,
+      );
+    });
+
+    test('stops when a member reads a table through another role', async function (assert) {
+      await withMemberships(
+        [
+          ['readonly_role', MEMBER],
+          [OTHER_ROLE, MEMBER],
+        ],
+        async () => {
+          await dbAdapter.execute(
+            `GRANT SELECT ON TABLE user_keys TO ${OTHER_ROLE}`,
+          );
+
+          await assert.rejects(
+            runUp(),
+            new RegExp(
+              `${MEMBER} can read user_keys\\.\\w+, which is withheld`,
+            ),
+          );
+        },
+      );
+    });
+
+    test('stops when a role that inherits the read-only role through a group reads a table', async function (assert) {
+      await withMemberships(
+        [
+          ['readonly_role', GROUP],
+          [GROUP, LEAF],
+          [OTHER_ROLE, LEAF],
+        ],
+        async () => {
+          await dbAdapter.execute(
+            `GRANT SELECT (owner) ON TABLE key_write_tickets TO ${OTHER_ROLE}`,
+          );
+
+          await assert.rejects(
+            runUp(),
+            new RegExp(
+              `${LEAF} can read key_write_tickets\\.owner, which is withheld`,
+            ),
+          );
+        },
+      );
+    });
+
+    test('leaves a member that holds only the ADMIN option on the role', async function (assert) {
+      // The role that creates the read-only role holds this membership on
+      // Postgres 16 and later, and reads nothing through it.
+      await withMemberships(
+        [
+          [
+            'readonly_role',
+            ADMIN_MEMBER,
+            'ADMIN TRUE, INHERIT FALSE, SET FALSE',
+          ],
+          [OTHER_ROLE, ADMIN_MEMBER],
+        ],
+        async () => {
+          await dbAdapter.execute(
+            `GRANT SELECT ON TABLE ${WITHHELD_TABLES.join(', ')} TO ${OTHER_ROLE}`,
+          );
+
+          await runUp();
+
+          let [{ readable }] = await dbAdapter.execute(
+            `SELECT has_table_privilege('${ADMIN_MEMBER}', 'proxy_endpoints', 'SELECT') AS readable`,
+          );
+          assert.true(
+            readable,
+            'the member still reads through the other role',
+          );
+        },
+      );
+    });
+
+    test('leaves a member that owns a table reading it', async function (assert) {
+      await createRole(OWNER);
+      await dbAdapter.execute(
+        `ALTER TABLE incoming_webhooks OWNER TO ${OWNER}`,
+      );
+      await withMemberships([['readonly_role', OWNER]], async () => {
+        await runUp();
+
+        assert.ok(true, 'up completes');
+      });
+    });
+  });
+});