fix(security)[P0]: enforce RLS on by-id writes — close member-edits-others'-records hole (#1985)

.changeset/rls-write-enforcement.md

@@ -0,0 +1,16 @@
+---
+"@objectstack/plugin-security": minor
+---
+
+fix(security): enforce row-level security on by-id writes — close the member-can-edit-others'-records hole (#1985).
+
+A single-id `update`/`delete` goes straight to `driver.update(object, id, …)` / `driver.delete(object, id)` and builds no query `ast`, so the RLS `where` filter the middleware injects on the read path was **never applied to by-id writes**. Combined with `member_default` granting `*: { edit, delete }` (scoped, by design, via the `owner_only_writes/deletes` RLS), this meant the owner predicate was silently bypassed: **any authenticated member could modify or delete another user's records** (verified end-to-end — a member PATCH'd an admin's record and the change persisted).
+
+Two coordinated changes:
+
+- **Enforce a pre-image authorization check.** Before a single-id `update`/`delete`, the security middleware computes the write-operation RLS filter and re-reads the target row with `{ id } AND <writeFilter>`; if the row isn't visible (someone else's, or RLS-hidden) it throws `PermissionDeniedError` (403). Reuses the existing RLS/tenant machinery, is recursion-safe (a `find` doesn't trigger the check), and is skipped when no RLS policy applies (e.g. admin sets, `modifyAllRecords`) so admins and unguarded objects are unchanged.
+- **Repoint owner scoping to a column that exists.** `owner_only_writes`/`owner_only_deletes` keyed on `owner_id`, which author-defined objects almost never declare — so the policy referenced a missing column and `computeRlsFilter` dropped it (the no-op that made the bypass invisible). Now keyed on `created_by`, the ownership column the engine stamps on every object.
+
+Result: a member may edit/delete the records they created, not others'; admins (and any set with `modifyAllRecords` or no RLS) are unrestricted. Objects that opt out of audit fields (`systemFields.audit: false`) have no `created_by` and now fail **closed** for member writes (grant `modifyAllRecords` or a per-object policy to allow). Objects modeling transferable ownership should override with a per-object owner policy.
+
+Verified live on app-crm (2 users): member→others' record PATCH/DELETE = 403 (unmutated); member→own = 200; admin→any = 200. Note: cross-tenant write isolation additionally depends on an organization being assigned at sign-up (tracked separately in #1985).

packages/plugins/plugin-security/src/objects/default-permission-sets.ts

@@ -295,17 +295,29 @@ export const defaultPermissionSets: PermissionSet[] = [
         operation: 'all',
         using: 'organization_id = current_user.organization_id',
       },
+      // Owner-scoped writes/deletes for rank-and-file members: you may modify
+      // and delete the records you created, not other users'. Keyed on
+      // `created_by` — the column the engine stamps on EVERY record — rather
+      // than `owner_id`, which author-defined objects almost never declare. The
+      // old `owner_id` key referenced a missing column on real objects, so
+      // `computeRlsFilter` dropped the policy and the scoping silently no-op'd
+      // (any member could edit/delete any record — #1985). These policies are
+      // ENFORCED on writes via the security middleware's pre-image check (a
+      // by-id update/delete never builds an RLS `where`, so the predicate is
+      // verified against the target row before the mutation). Objects that
+      // model transferable ownership with a dedicated owner field should
+      // override these with a per-object policy.
       {
         name: 'owner_only_writes',
         object: '*',
         operation: 'update',
-        using: 'owner_id = current_user.id',
+        using: 'created_by = current_user.id',
       },
       {
         name: 'owner_only_deletes',
         object: '*',
         operation: 'delete',
-        using: 'owner_id = current_user.id',
+        using: 'created_by = current_user.id',
       },
       // ── better-auth system tables that lack `organization_id` and would
       //    otherwise be left unprotected by the wildcard rule above. ────

packages/plugins/plugin-security/src/security-plugin.test.ts

@@ -97,13 +97,20 @@ describe('SecurityPlugin', () => {
   // wildcard `current_user.organization_id` RLS policies. Otherwise it
   // strips them so single-tenant deployments aren't filtered to nothing.
   // -------------------------------------------------------------------------
-  const makeMiddlewareCtx = (overrides: { permissionSets: PermissionSet[]; objectFields?: string[]; schemaExtra?: Record<string, any>; orgScoping?: boolean }) => {
+  const makeMiddlewareCtx = (overrides: { permissionSets: PermissionSet[]; objectFields?: string[]; schemaExtra?: Record<string, any>; orgScoping?: boolean; findOneImpl?: (query: any) => any }) => {
     const fields: Record<string, any> = {};
     for (const f of overrides.objectFields ?? ['id', 'organization_id', 'owner_id', 'name']) {
       fields[f] = { name: f };
     }
     const baseSchema: any = { name: 'task', fields, ...(overrides.schemaExtra ?? {}) };
     let middleware: any;
+    // The pre-image write-authorization check re-reads the target row via
+    // `ql.findOne(object, { where: { $and: [{ id }, writeFilter] }, … })`.
+    // `findOneImpl` lets a test decide whether that row is "visible" (owned /
+    // in-tenant) or filtered out (someone else's row → null → deny).
+    const findOne = vi.fn(async (_object: string, query: any) =>
+      overrides.findOneImpl ? overrides.findOneImpl(query) : null,
+    );
     const ql = {
       registerMiddleware: (mw: any) => {
         // Capture only the FIRST middleware (the security CRUD one);
@@ -112,6 +119,7 @@ describe('SecurityPlugin', () => {
         if (!middleware) middleware = mw;
       },
       getSchema: () => baseSchema,
+      findOne,
     };
     const metadata = {
       get: async () => baseSchema,
@@ -136,6 +144,7 @@ describe('SecurityPlugin', () => {
     };
     return {
       ctx,
+      findOne,
       run: async (opCtx: any) => {
         await middleware(opCtx, async () => {});
         return opCtx;
@@ -226,6 +235,98 @@ describe('SecurityPlugin', () => {
     expect(opCtx.ast.where).toBeUndefined();
   });
 
+  // ── Row-level WRITE authorization (pre-image check, #1985) ──────────────
+  // A by-id update/delete never builds an RLS `where`, so the owner/tenant
+  // predicate must be enforced by re-reading the target row before mutating.
+  describe('pre-image write authorization', () => {
+    const ownerPolicySet: PermissionSet = {
+      name: 'member_default',
+      label: 'Member',
+      isProfile: true,
+      objects: { '*': { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true } },
+      rowLevelSecurity: [
+        { name: 'owner_only_writes', object: '*', operation: 'update', using: 'created_by = current_user.id' },
+        { name: 'owner_only_deletes', object: '*', operation: 'delete', using: 'created_by = current_user.id' },
+      ],
+    } as any;
+    const memberCtx = { userId: 'u1', tenantId: 'org-1', roles: [], permissions: [] };
+    const ownerFields = ['id', 'created_by', 'name'];
+
+    it('DENIES an update when the target row is not visible under the write filter (not the owner)', async () => {
+      const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' });
+      const harness = makeMiddlewareCtx({
+        permissionSets: [ownerPolicySet],
+        objectFields: ownerFields,
+        findOneImpl: () => null, // row exists but filtered out by created_by → not visible
+      });
+      await plugin.init(harness.ctx);
+      await plugin.start(harness.ctx);
+      const opCtx: any = {
+        object: 'task', operation: 'update',
+        data: { id: 'r1', name: 'hijack' }, options: { where: { id: 'r1' } },
+        context: memberCtx,
+      };
+      await expect(harness.run(opCtx)).rejects.toMatchObject({ name: 'PermissionDeniedError' });
+      expect(harness.findOne).toHaveBeenCalledTimes(1);
+      // the re-read ANDs the row id with the owner write filter
+      const [, query] = harness.findOne.mock.calls[0];
+      expect(query.where.$and[0]).toEqual({ id: 'r1' });
+    });
+
+    it('ALLOWS an update when the target row IS visible under the write filter (the owner)', async () => {
+      const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' });
+      const harness = makeMiddlewareCtx({
+        permissionSets: [ownerPolicySet],
+        objectFields: ownerFields,
+        findOneImpl: () => ({ id: 'r1', created_by: 'u1', name: 'mine' }),
+      });
+      await plugin.init(harness.ctx);
+      await plugin.start(harness.ctx);
+      const opCtx: any = {
+        object: 'task', operation: 'delete',
+        options: { where: { id: 'r1' } },
+        context: memberCtx,
+      };
+      await expect(harness.run(opCtx)).resolves.toBeDefined();
+      expect(harness.findOne).toHaveBeenCalledTimes(1);
+    });
+
+    it('SKIPS the check when no RLS policy applies (e.g. modifyAllRecords / admin) — no extra read', async () => {
+      const adminSet: PermissionSet = {
+        name: 'admin_full_access', label: 'Admin', isProfile: true,
+        objects: { '*': { allowRead: true, allowEdit: true, allowDelete: true, modifyAllRecords: true, viewAllRecords: true } },
+        // no rowLevelSecurity
+      } as any;
+      const plugin = new SecurityPlugin({ fallbackPermissionSet: 'admin_full_access' });
+      const harness = makeMiddlewareCtx({ permissionSets: [adminSet], objectFields: ownerFields });
+      await plugin.init(harness.ctx);
+      await plugin.start(harness.ctx);
+      const opCtx: any = {
+        object: 'task', operation: 'update',
+        data: { id: 'r1', name: 'x' }, options: { where: { id: 'r1' } },
+        context: { userId: 'admin', roles: ['admin_full_access'], permissions: [] },
+      };
+      await expect(harness.run(opCtx)).resolves.toBeDefined();
+      expect(harness.findOne).not.toHaveBeenCalled();
+    });
+
+    it('SKIPS the check for a multi-row predicate id ({$in}) — only single-id by-pk writes are guarded', async () => {
+      const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' });
+      const harness = makeMiddlewareCtx({
+        permissionSets: [ownerPolicySet], objectFields: ownerFields, findOneImpl: () => null,
+      });
+      await plugin.init(harness.ctx);
+      await plugin.start(harness.ctx);
+      const opCtx: any = {
+        object: 'task', operation: 'update', multi: true,
+        data: { name: 'bulk' }, options: { multi: true, where: { id: { $in: ['r1', 'r2'] } } },
+        context: memberCtx,
+      };
+      await harness.run(opCtx);
+      expect(harness.findOne).not.toHaveBeenCalled();
+    });
+  });
+
   it('tenancy.enabled=false via systemFields.tenant=false — also skipped', async () => {
     const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' });
     const harness = makeMiddlewareCtx({

packages/plugins/plugin-security/src/security-plugin.ts

@@ -338,6 +338,67 @@ export class SecurityPlugin implements Plugin {
         }
       }
 
+      // 2.7. Row-level WRITE authorization (pre-image check).
+      //
+      // RLS is injected as a `where` filter on the read path (step 3, via
+      // `opCtx.ast`), but a single-id update/delete goes straight to
+      // `driver.update(object, id, …)` / `driver.delete(object, id)` — it builds
+      // no `ast`, so the row-level predicate is NEVER applied to by-id writes.
+      // The result (#1985): the CRUD check passes (member_default grants edit/
+      // delete) and the owner/tenant RLS that was supposed to scope the write is
+      // silently bypassed — any member could modify another user's record.
+      //
+      // Fix: before the mutation, compute the write-operation RLS filter and
+      // verify the TARGET row satisfies it. We re-read the row through the
+      // engine with `{ id } AND <writeFilter>`; a `find` does not re-enter this
+      // block, so there is no recursion, and read-side RLS/tenant scoping
+      // compose naturally. A `null` result means the row is either gone or
+      // RLS-hidden → deny. When `computeRlsFilter` returns `null` (no policy
+      // applies — e.g. an admin set with no RLS, or `modifyAllRecords`) the
+      // check is skipped and behaviour is unchanged.
+      if (
+        (opCtx.operation === 'update' || opCtx.operation === 'delete') &&
+        permissionSets.length > 0 &&
+        !!opCtx.context?.userId &&
+        this.ql
+      ) {
+        const targetId = this.extractSingleId(opCtx);
+        if (targetId != null) {
+          const writeFilter = await this.computeRlsFilter(
+            permissionSets,
+            opCtx.object,
+            opCtx.operation,
+            opCtx.context,
+          );
+          if (writeFilter) {
+            let visible: unknown = null;
+            try {
+              visible = await this.ql.findOne(opCtx.object, {
+                where: { $and: [{ id: targetId }, writeFilter] },
+                context: opCtx.context,
+              });
+            } catch {
+              // A read denial (e.g. no read permission) is itself a "cannot
+              // touch this row" signal — fall through to the deny below.
+              visible = null;
+            }
+            if (!visible) {
+              throw new PermissionDeniedError(
+                `[Security] Access denied: not permitted to ${opCtx.operation} this ` +
+                  `'${opCtx.object}' record (row-level security)`,
+                {
+                  operation: opCtx.operation,
+                  object: opCtx.object,
+                  roles,
+                  permissionSets: explicitPermissionSets,
+                  recordId: targetId,
+                },
+              );
+            }
+          }
+        }
+      }
+
       // 2.5. Field-Level Security write enforcement.
       //
       // The client-side masker (ObjectForm / inline grid) already hides
@@ -659,6 +720,28 @@ export class SecurityPlugin implements Plugin {
     return permissionSets;
   }
 
+  /**
+   * Resolve a single scalar primary-key id from an update/delete operation
+   * context, mirroring the engine's "single-id vs predicate" rule
+   * (`engine.ts` update/delete): only a scalar `data.id` or `where.id`
+   * identifies one row. An operator object (`{ $in: [...] }`, …) is a
+   * multi-row predicate and returns `null` (multi-row writes route through the
+   * `*Many` paths, out of scope for the by-id pre-image check).
+   */
+  private extractSingleId(opCtx: any): string | number | bigint | null {
+    const isScalar = (v: unknown): v is string | number | bigint =>
+      v !== null && (typeof v === 'string' || typeof v === 'number' || typeof v === 'bigint');
+    const data = opCtx?.data;
+    if (data && typeof data === 'object' && !Array.isArray(data) && isScalar(data.id)) {
+      return data.id;
+    }
+    const where = opCtx?.options?.where;
+    if (where && typeof where === 'object' && 'id' in where && isScalar((where as any).id)) {
+      return (where as any).id;
+    }
+    return null;
+  }
+
   /**
    * Compile the applicable RLS policies for (object, operation) into a single
    * `FilterCondition`, applying the field-existence safety net (wildcard