diff --git a/.changeset/rls-write-enforcement.md b/.changeset/rls-write-enforcement.md new file mode 100644 index 0000000000..9cfb02fb11 --- /dev/null +++ b/.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 `; 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). diff --git a/packages/plugins/plugin-security/src/objects/default-permission-sets.ts b/packages/plugins/plugin-security/src/objects/default-permission-sets.ts index c5efac90dd..89b4487853 100644 --- a/packages/plugins/plugin-security/src/objects/default-permission-sets.ts +++ b/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. ──── diff --git a/packages/plugins/plugin-security/src/security-plugin.test.ts b/packages/plugins/plugin-security/src/security-plugin.test.ts index 78cd67ad8f..2380c0b80d 100644 --- a/packages/plugins/plugin-security/src/security-plugin.test.ts +++ b/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; orgScoping?: boolean }) => { + const makeMiddlewareCtx = (overrides: { permissionSets: PermissionSet[]; objectFields?: string[]; schemaExtra?: Record; orgScoping?: boolean; findOneImpl?: (query: any) => any }) => { const fields: Record = {}; 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({ diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index fe22106b8d..02ab26d53f 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/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 `; 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