diff --git a/.changeset/21771-write-door-unreadable-is-not-found.md b/.changeset/21771-write-door-unreadable-is-not-found.md new file mode 100644 index 00000000000..6c3cd615fd5 --- /dev/null +++ b/.changeset/21771-write-door-unreadable-is-not-found.md @@ -0,0 +1,27 @@ +--- +'@objectstack/plugin-security': minor +"@objectstack/spec": patch +--- + +fix(plugin-security)!: on the write doors, a row the caller cannot read answers what a nonexistent id answers + +Clause-②: no (narrowing) + + + +**BREAKING**: a by-id update or delete of a row the caller cannot read now answers `404 RECORD_NOT_FOUND`, with exactly the body an id that names no row gets, for every principal class. On the write doors, "hidden" and "gone" are now one answer to a caller who cannot read the row. It ships as `minor` under the launch-window convention for accept-set narrowings. No export is added or removed, and no error code is new. + +**What changed.** The answer used to depend on which gate saw the row first. Where a write-class row filter binds the caller, the by-id write pre-image check answered `403 PERMISSION_DENIED`. Where none binds it, a later gate answered with its own 403: `FORBIDDEN` from record sharing, or a parent-derived gate's code on attachments and comments. Meanwhile a nonexistent id answered `404`. So the write door could tell a hidden row apart from a missing one. The pre-image check now asks the read door's own question first, for the by-id write the caller addressed: a by-id read in the caller's context, every data middleware's visibility included. A row that read does not return gets the read door's not-found producer. A store fault propagates as raised, and a read-time policy refusal is not treated as absence. + +**What is refused now that was not.** A principal that no write-class row filter binds could have its by-id write admitted on a row the read door hides from it. One example is the uploader of an attachment, or the author of a comment, whose parent record they can no longer read. That write is now refused with the not-found answer, as it already was for every principal a row filter binds. + +**FROM → TO.** A by-id update or delete of a row hidden from the caller: FROM a `403` (`PERMISSION_DENIED`, `FORBIDDEN`, or a parent-derived gate's code) → TO `404 RECORD_NOT_FOUND`, the body a nonexistent id gets. + +**If you are affected.** A client that read a by-id write's `403` as "the row exists, but you may not change it" should read `404 RECORD_NOT_FOUND` the way the read door means it: no row you can see has this id. + +**Unchanged.** +- A caller who can read the row but may not write it keeps its 403. They already see the row. +- By-id writes the platform issues under the caller's context keep their previous answer, because the caller never named their target: the engine's cascade delete of a dependent row, a hook's write, and the referential clear of a lookup. +- Writes that are not routed by id are unchanged. + +`security/explain` follows enforcement. Its record verdict for an update or delete of a record the principal cannot read is now the missing-record shape: `visible: false`, with no decider. diff --git a/content/docs/permissions/attachments-access.mdx b/content/docs/permissions/attachments-access.mdx index 7b44a7a79ab..bc92cc38414 100644 --- a/content/docs/permissions/attachments-access.mdx +++ b/content/docs/permissions/attachments-access.mdx @@ -71,11 +71,12 @@ editable by design). A multi-delete requires *every* matched row to pass. | Code | Status | When | | --- | --- | --- | | `ATTACHMENT_DELETE_DENIED` | 403 | The caller can read the attachment, but is neither the uploader nor able to edit the parent record | -| `PERMISSION_DENIED` | 403 | The caller cannot read the parent record, so the attachment is not visible to them. This is the platform's not-visible refusal whichever layer gives it — the row-level write check that runs before the gate, or the gate itself for a caller that check does not cover — and it names neither the parent nor the attachment's link to it | +| `RECORD_NOT_FOUND` | 404 | A delete or update **by id** of an attachment the caller cannot read — its parent record is not readable to them. On the write doors a row the caller cannot read is a row that does not exist: the answer is exactly what an id that names no attachment gets, for every caller, so a hidden attachment and a missing one cannot be told apart. It applies even to the uploader once the parent is out of their sight, because the read door no longer returns the attachment to them either | +| `PERMISSION_DENIED` | 403 | The gate's own not-visible refusal, where the gate answers before that check. It names neither the parent nor the attachment's link to it | An update of another user's attachment follows the same rule — the uploader or -a parent editor — and a caller who cannot read the parent gets the same -not-visible refusal. +a parent editor — and a by-id update of an attachment the caller cannot read +gets the same not-found answer. The platform baseline also ships a parent-blind row-level delete floor (`owner_only_deletes`: you may delete only the rows you created, for members diff --git a/content/docs/permissions/permissions-matrix.mdx b/content/docs/permissions/permissions-matrix.mdx index 8cb920ad1a4..857b96665a1 100644 --- a/content/docs/permissions/permissions-matrix.mdx +++ b/content/docs/permissions/permissions-matrix.mdx @@ -360,7 +360,7 @@ flowchart TD | 7 | **Field-Level Security** | Which fields is the user allowed to see/edit? | -**Performance:** For reads, steps 2–6 are compiled into a query filter (owner-match ∪ materialized shares, AND-ed with RLS) at query time, not evaluated record-by-record. By-id writes are verified with a pre-image check: the target row is re-read through the write-scope filter before the mutation. This keeps security checks efficient even on tables with millions of rows. +**Performance:** For reads, steps 2–6 are compiled into a query filter (owner-match ∪ materialized shares, AND-ed with RLS) at query time, not evaluated record-by-record. By-id writes are verified with a pre-image check: the target row is re-read through the write-scope filter before the mutation, and a row the caller cannot read answers exactly what a missing id answers (`404 RECORD_NOT_FOUND`). This keeps security checks efficient even on tables with millions of rows. ## See also diff --git a/content/docs/protocol/kernel/error-handling.mdx b/content/docs/protocol/kernel/error-handling.mdx index e22695d79f9..19d80c88255 100644 --- a/content/docs/protocol/kernel/error-handling.mdx +++ b/content/docs/protocol/kernel/error-handling.mdx @@ -907,6 +907,12 @@ Content-Type: application/json } ``` +⚠️ **This is the answer for a row the caller can read.** A by-id update or delete of a row +the caller **cannot read** answers exactly what an id that names no row answers — +`404 RECORD_NOT_FOUND` — whichever rule hides the row, so the write door never tells +"hidden" apart from "gone". A caller who can read the row but may not write it gets the 403 +shown here. + ⚠️ **`FORBIDDEN`, not `PERMISSION_DENIED`.** A by-id write the sharing rules refuse carries `FORBIDDEN`; `PERMISSION_DENIED` is what the capability and identity guards carry. Both are 403s in the same envelope on this door, so branch on either — ⛔ but do not assume one code diff --git a/packages/plugins/plugin-audit/src/comment-access-hooks.ts b/packages/plugins/plugin-audit/src/comment-access-hooks.ts index e32cf4bb8db..b9af9e19816 100644 --- a/packages/plugins/plugin-audit/src/comment-access-hooks.ts +++ b/packages/plugins/plugin-audit/src/comment-access-hooks.ts @@ -200,6 +200,12 @@ function forbid(message: string, object?: string): never { * The code of the platform's not-visible refusal: the one plugin-security's * by-id write pre-image check throws when the caller's own read visibility * does not reach the target row (`PermissionDeniedError`, 403). + * + * [#21771] No longer the pre-image check's answer for a row the caller cannot + * read: under the write doors' ruling A that check asks the caller's read + * visibility for every principal, and answers the by-id update or delete the + * caller addressed with what a nonexistent id answers, before this gate runs. + * This refusal stays the gate's own, for a write the gate answers first. */ const NOT_VISIBLE_CODE: StandardErrorCode = 'PERMISSION_DENIED'; const NOT_VISIBLE_STATUS = 403; diff --git a/packages/plugins/plugin-auth/src/sys-user-self-service-route.test.ts b/packages/plugins/plugin-auth/src/sys-user-self-service-route.test.ts index 6c8410439a8..ebceb9539fe 100644 --- a/packages/plugins/plugin-auth/src/sys-user-self-service-route.test.ts +++ b/packages/plugins/plugin-auth/src/sys-user-self-service-route.test.ts @@ -350,7 +350,9 @@ describe('sys_user self-service — the four pins, each attributed to a layer', // with `refusedBy === 'object-gate'` — the object bit was false, so the row // scope never ran and this pin proved nothing about it. Now the pre-image // re-read HAPPENED, was scoped to the caller, and came back empty. - expect(r.preImageWheres).toEqual([{ $and: [{ id: PEER }, { id: ME }] }]); + // …and, the row being refused, the read question that keeps the 403: the + // member reads the peer's row, so it is not answered as a missing one. + expect(r.preImageWheres).toEqual([{ $and: [{ id: PEER }, { id: ME }] }, { id: PEER }]); expect(r.error?.name).toBe('PermissionDeniedError'); expect(r.error?.code).toBe('PERMISSION_DENIED'); }); diff --git a/packages/plugins/plugin-security/src/authz-matrix-gate.test.ts b/packages/plugins/plugin-security/src/authz-matrix-gate.test.ts index 0a44069c3c4..9e89a968819 100644 --- a/packages/plugins/plugin-security/src/authz-matrix-gate.test.ts +++ b/packages/plugins/plugin-security/src/authz-matrix-gate.test.ts @@ -205,7 +205,14 @@ async function readFilter(cell: any, roleCtx: any): Promise { */ async function writeFilter(cell: any, roleCtx: any): Promise { const plugin = new SecurityPlugin(); - const h = makeHarness({ ...cell, orgScoping: cell.orgScoping ?? true, findOneImpl: () => null }); + // The write-class re-read finds nothing; [#21771] the addressed write's + // read question (a plain by-id read) finds the row, so the matrix keeps + // measuring the WRITE filter alone. + const h = makeHarness({ + ...cell, + orgScoping: cell.orgScoping ?? true, + findOneImpl: (q: any) => (q?.where?.$and ? null : { id: 'r1' }), + }); await plugin.init(h.ctx); await plugin.start(h.ctx); const opCtx: any = { object: cell.objectName, operation: 'update', @@ -213,10 +220,11 @@ async function writeFilter(cell: any, roleCtx: any): Promise { }; let threw: any = null; try { await h.run(opCtx); } catch (e: any) { threw = e; } - if (h.findOne.mock.calls.length === 0) { + const reRead = h.findOne.mock.calls.find((call: any[]) => call[1]?.where?.$and); + if (!reRead) { return threw ? `CRUD_DENY:${threw?.name ?? 'err'}` : 'BYPASS(no-write-filter)'; } - return h.findOne.mock.calls[0][1].where.$and.slice(1); + return reRead[1].where.$and.slice(1); } /** diff --git a/packages/plugins/plugin-security/src/by-id-write-unreadable-not-found.test.ts b/packages/plugins/plugin-security/src/by-id-write-unreadable-not-found.test.ts new file mode 100644 index 00000000000..9f24cd14ae2 --- /dev/null +++ b/packages/plugins/plugin-security/src/by-id-write-unreadable-not-found.test.ts @@ -0,0 +1,251 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * On the write doors, a row the caller cannot READ is a row that does not + * exist — measured through a real `ObjectQL` over a real SQL driver, with the + * real `SecurityPlugin` middleware in front of it. + * + * ## What is pinned + * + * 1. The ADDRESSED by-id update and delete of a row the caller cannot read + * answer exactly what the same verbs answer for an id that exists nowhere: + * the read door's not-found, the same code, status and sentence (the one + * difference is the id the caller supplied). + * 2. A caller who can read the row but may not write it keeps `403 + * PERMISSION_DENIED` with the record-level sentence: they already see it. + * 3. A writable row the caller can read is still written. + * 4. By-id writes the platform issues UNDER the caller's context — the + * engine's cascade delete of a dependent row, and a hook's `ctx.api` + * write — are not addressed by the caller, so they keep the answer they + * had before: the record-level 403, which names nothing. A not-found there + * would carry an id the caller never supplied. + * + * The rig is one permission set over two objects: a public parent, and a + * child whose rows a caller reads when it owns them or they are flagged + * shared, and writes only when it owns them. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { PermissionSetSchema } from '@objectstack/spec/security'; +import { BUILTIN_OPERATION_MESSAGES } from '@objectstack/spec/system'; +import { recordNotFoundError } from '@objectstack/core'; + +import { SecurityPlugin } from './security-plugin.js'; +import { defaultPermissionSets } from './objects/default-permission-sets.js'; + +const SYS = { context: { isSystem: true } } as never; +const MEMBER_DEFAULT = defaultPermissionSets.find((p) => p.name === 'member_default')!; +const RECORD_SENTENCE = BUILTIN_OPERATION_MESSAGES.en.record_access_denied!; + +const PARENT = 'qa_nf_parent'; +const CHILD = 'qa_nf_child'; +const ME = 'usr_nf_member'; +const OTHER = 'usr_nf_other'; +const CALLER = { userId: ME, positions: ['qa_pos'], permissions: ['qa_nf_guard'], posture: 'MEMBER' }; + +interface Refusal { code?: string; status?: number; message: string; name?: string } +type Outcome = { kind: 'landed' } | ({ kind: 'refused' } & Refusal); + +interface Rig { + engine: ObjectQL; + update: (object: string, id: string, data?: Record) => Promise; + remove: (object: string, id: string) => Promise; + stored: (object: string, id: string) => Promise | null>; + teardown: () => Promise; +} +const rigs: Rig[] = []; +afterEach(async () => { + for (const rig of rigs.splice(0)) await rig.teardown(); +}); + +async function boot(): Promise { + const engine = new ObjectQL(); + engine.registerDriver( + new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }) as never, + true, + ); + await engine.init(); + engine.registerApp({ + id: 'com.objectstack.qa.by-id-write-unreadable-not-found', + name: 'By-id write: an unreadable row is a missing row', + version: '1.0.0', + type: 'plugin', + scope: 'system', + objects: [ + { + name: PARENT, + label: 'Parent', + sharingModel: 'public_read_write', + fields: { + id: { name: 'id', type: 'text', primaryKey: true }, + name: { name: 'name', type: 'text' }, + }, + }, + { + name: CHILD, + label: 'Child', + sharingModel: 'public_read_write', + fields: { + id: { name: 'id', type: 'text', primaryKey: true }, + name: { name: 'name', type: 'text' }, + owner: { name: 'owner', type: 'text' }, + shared: { name: 'shared', type: 'boolean' }, + parent: { name: 'parent', type: 'lookup', reference: PARENT, deleteBehavior: 'cascade' }, + }, + }, + ], + } as never); + await engine.syncSchemas(); + await engine.insert(PARENT, [ + { id: 'p_plain', name: 'plain' }, + { id: 'p_cascade', name: 'has a hidden child' }, + { id: 'p_hook', name: 'hooked' }, + ], SYS); + await engine.insert(CHILD, [ + { id: 'c_own', name: 'mine', owner: ME, shared: false }, + { id: 'c_hidden', name: 'theirs, private', owner: OTHER, shared: false }, + { id: 'c_shared', name: 'theirs, shared', owner: OTHER, shared: true }, + { id: 'c_kid', name: 'theirs, under the cascade parent', owner: OTHER, shared: false, parent: 'p_cascade' }, + ], SYS); + + const set = PermissionSetSchema.parse({ + name: 'qa_nf_guard', + objects: { + [PARENT]: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true }, + [CHILD]: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true }, + }, + rowLevelSecurity: [ + { name: 'child_read', object: CHILD, operation: 'select', using: 'record.owner == current_user.id || record.shared == true' }, + { name: 'child_update', object: CHILD, operation: 'update', using: 'record.owner == current_user.id' }, + { name: 'child_delete', object: CHILD, operation: 'delete', using: 'record.owner == current_user.id' }, + ], + }); + const services: Record = { + manifest: { register: vi.fn() }, + objectql: engine, + metadata: { + get: async (_type: string, name: string) => engine.getSchema(name) ?? null, + list: async () => [MEMBER_DEFAULT, set], + }, + }; + const ctx = { + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + registerService: vi.fn(), + hook: () => undefined, + getService: (name: string) => { + if (!(name in services)) throw new Error(`service not registered: ${name}`); + return services[name]; + }, + }; + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + await plugin.init(ctx as never); + await plugin.start(ctx as never); + + const rig: Rig = { + engine, + update: (object: string, id: string, data: Record = { name: 'renamed' }) => + outcome(engine.update(object, data, { where: { id }, context: { ...CALLER } } as never)), + remove: (object: string, id: string) => + outcome(engine.delete(object, { where: { id }, context: { ...CALLER } } as never)), + stored: async (object: string, id: string) => + ((await engine.findOne(object, { where: { id }, context: { isSystem: true } } as never)) ?? null) as Record | null, + teardown: async () => { try { await engine.destroy(); } catch { /* noop */ } }, + }; + rigs.push(rig); + return rig; +} + +function outcome(p: Promise): Promise { + return p.then( + () => ({ kind: 'landed' as const }), + (e: any) => ({ + kind: 'refused' as const, + code: e?.code, + status: e?.statusCode ?? e?.status, + message: String(e?.message), + name: e?.name, + }), + ); +} + +/** The whole refusal with the caller-supplied id written out of the sentence. */ +function withoutId(o: Outcome, id: string): Outcome { + return o.kind === 'refused' ? { ...o, message: o.message.split(id).join('ID') } : o; +} + +describe('a by-id write to a row the caller cannot read answers what a nonexistent id answers', () => { + for (const verb of ['update', 'delete'] as const) { + it(`${verb}: the hidden row and the missing id get one answer, the read door's not-found`, async () => { + const r = await boot(); + // The precondition, read off the read door: the caller does not see it. + expect(await r.engine.findOne(CHILD, { where: { id: 'c_hidden' }, context: { ...CALLER } } as never)).toBeNull(); + + const hidden = verb === 'update' ? await r.update(CHILD, 'c_hidden') : await r.remove(CHILD, 'c_hidden'); + const missing = verb === 'update' ? await r.update(CHILD, 'c_nowhere') : await r.remove(CHILD, 'c_nowhere'); + + expect(hidden).toMatchObject({ kind: 'refused', code: 'RECORD_NOT_FOUND', status: 404 }); + expect(hidden).toMatchObject({ message: (recordNotFoundError(CHILD, 'c_hidden') as Error).message }); + expect(withoutId(hidden, 'c_hidden')).toEqual(withoutId(missing, 'c_nowhere')); + // Refused means untouched. + expect((await r.stored(CHILD, 'c_hidden'))?.name).toBe('theirs, private'); + }); + + it(`${verb}: a caller who can read the row but may not write it keeps 403 PERMISSION_DENIED`, async () => { + const r = await boot(); + expect(await r.engine.findOne(CHILD, { where: { id: 'c_shared' }, context: { ...CALLER } } as never)).toBeTruthy(); + + const got = verb === 'update' ? await r.update(CHILD, 'c_shared') : await r.remove(CHILD, 'c_shared'); + + expect(got).toMatchObject({ kind: 'refused', code: 'PERMISSION_DENIED', status: 403, message: RECORD_SENTENCE }); + expect((await r.stored(CHILD, 'c_shared'))?.name).toBe('theirs, shared'); + }); + + it(`${verb}: a row the caller reads and may write is still written`, async () => { + const r = await boot(); + const got = verb === 'update' ? await r.update(CHILD, 'c_own') : await r.remove(CHILD, 'c_own'); + expect(got).toEqual({ kind: 'landed' }); + const after = await r.stored(CHILD, 'c_own'); + if (verb === 'update') expect(after?.name).toBe('renamed'); + else expect(after).toBeNull(); + }); + } +}); + +describe('a by-id write the platform issues under the caller\'s context keeps the answer it had', () => { + it('the cascade delete of a dependent row the caller cannot read refuses with the record-level 403, naming nothing', async () => { + const r = await boot(); + const got = await r.remove(PARENT, 'p_cascade'); + + expect(got).toMatchObject({ kind: 'refused', code: 'PERMISSION_DENIED', status: 403, message: RECORD_SENTENCE }); + expect(got.kind === 'refused' && got.message).not.toContain('c_kid'); + // One unit of work: neither the dependent nor the parent went. + expect(await r.stored(CHILD, 'c_kid')).toBeTruthy(); + expect(await r.stored(PARENT, 'p_cascade')).toBeTruthy(); + }); + + it('a hook\'s by-id write to a row the caller cannot read refuses with the record-level 403, not the not-found', async () => { + const r = await boot(); + r.engine.registerHook( + 'beforeUpdate', + async (hookCtx: any) => { + await hookCtx.api.object(CHILD).updateById('c_hidden', { name: 'written by the hook' }); + }, + { object: PARENT, packageId: 'com.objectstack.qa.by-id-write-unreadable-not-found' }, + ); + + const got = await r.update(PARENT, 'p_hook'); + + expect(got).toMatchObject({ kind: 'refused', code: 'PERMISSION_DENIED', status: 403, message: RECORD_SENTENCE }); + expect(got.kind === 'refused' && got.message).not.toContain('c_hidden'); + expect((await r.stored(CHILD, 'c_hidden'))?.name).toBe('theirs, private'); + expect((await r.stored(PARENT, 'p_hook'))?.name).toBe('hooked'); + }); + + it('the same caller addressing that row itself gets the not-found — the line is who addressed it', async () => { + const r = await boot(); + expect(await r.update(CHILD, 'c_hidden')).toMatchObject({ kind: 'refused', code: 'RECORD_NOT_FOUND', status: 404 }); + expect(await r.remove(CHILD, 'c_kid')).toMatchObject({ kind: 'refused', code: 'RECORD_NOT_FOUND', status: 404 }); + }); +}); diff --git a/packages/plugins/plugin-security/src/controlled-by-parent-sharing.test.ts b/packages/plugins/plugin-security/src/controlled-by-parent-sharing.test.ts index e86204c24d4..848ebab99d6 100644 --- a/packages/plugins/plugin-security/src/controlled-by-parent-sharing.test.ts +++ b/packages/plugins/plugin-security/src/controlled-by-parent-sharing.test.ts @@ -35,6 +35,7 @@ // nothing), and a master the caller owns. import { describe, it, expect, vi } from 'vitest'; +import { recordNotFoundError } from '@objectstack/core'; import { SecurityPlugin } from './security-plugin.js'; import { SharingService, type SharingEngine } from '@objectstack/plugin-sharing'; import { matchesFilterCondition } from '@objectstack/formula'; @@ -784,7 +785,10 @@ describe('[#7474] the six refusal legs answer with six envelopes, not one', () = const err = await refusalOf(h.updateContact('ct_deleted_concurrently')); expect(err.code).toBe('RECORD_NOT_FOUND'); expect(err.status).toBe(404); - expect(err.statusCode).toBe(404); + // [#21771] The addressed by-id write now asks the read door first, so a + // missing id gets the READ door's own not-found producer — which spells + // the status once — rather than the master gate's detail copy. + expect(err.message).toBe(recordNotFoundError('crm_contact', 'ct_deleted_concurrently').message); expect(err.message).toContain('ct_deleted_concurrently'); expect(err.message).not.toContain('requires edit access to its master record'); }); diff --git a/packages/plugins/plugin-security/src/explain-cross-class-refusal.test.ts b/packages/plugins/plugin-security/src/explain-cross-class-refusal.test.ts index d180214881d..cb6fb20016d 100644 --- a/packages/plugins/plugin-security/src/explain-cross-class-refusal.test.ts +++ b/packages/plugins/plugin-security/src/explain-cross-class-refusal.test.ts @@ -159,6 +159,8 @@ async function boot(makeDriver: () => Driver, predicate: string) { type Envelope = { code: string; status: number }; const DENIED: Envelope = { code: 'PERMISSION_DENIED', status: 403 }; const INVALID: Envelope = { code: 'INVALID_FILTER', status: 400 }; +/** [#21771] The read door's answer for an id it does not return. */ +const NOT_FOUND: Envelope = { code: 'RECORD_NOT_FOUND', status: 404 }; const envelopeOf = (e: unknown): Envelope => { const x = e as { code?: string; status?: number; statusCode?: number }; @@ -267,8 +269,10 @@ for (const [driverName, makeDriver, available] of DRIVERS) { expect(await outcome(w.request('update', 'r1'))).toBe('admitted'); expect((await w.explain('update', 'r1')).record).toEqual({ recordId: 'r1', visible: true, decidedBy: 'rls' }); - expect(await outcome(w.request('update', 'r2'))).toEqual(DENIED); - expect((await w.explain('update', 'r2')).record).toEqual({ recordId: 'r2', visible: false, decidedBy: 'rls' }); + // [#21771] r2 is a row the caller cannot read: the write door answers + // what a nonexistent id answers, and explain reports the missing shape. + expect(await outcome(w.request('update', 'r2'))).toEqual(NOT_FOUND); + expect((await w.explain('update', 'r2')).record).toEqual({ recordId: 'r2', visible: false }); }); }); } diff --git a/packages/plugins/plugin-security/src/explain-enforce-parity.test.ts b/packages/plugins/plugin-security/src/explain-enforce-parity.test.ts index 002ce6a91bf..62568039c0d 100644 --- a/packages/plugins/plugin-security/src/explain-enforce-parity.test.ts +++ b/packages/plugins/plugin-security/src/explain-enforce-parity.test.ts @@ -656,6 +656,8 @@ const withPrincipal = (source: PostureSource, f: (rig: PrincipalRig) => Promise< const REFUSED_INVALID: Enforced = { kind: 'refused', ...INVALID }; const REFUSED_DENIED: Enforced = { kind: 'refused', ...DENIED }; +/** [#21771] The read door's answer for an id it does not return, which a by-id write now gives for a row its caller cannot read. */ +const REFUSED_NOT_FOUND: Enforced = { kind: 'refused', code: 'RECORD_NOT_FOUND', status: 404 }; /** Rows over one row-level policy: every verdict position, for a predicate the find refuses and for the control. */ function rlsRows(card: string, label: string, predicate: string, refused: boolean): Row[] { @@ -706,7 +708,9 @@ function rlsRows(card: string, label: string, predicate: string, refused: boolea }, { card, shape: `${label}: record r2, update`, position: 'record.visible', - enforced: REFUSED_DENIED, + // r2 is outside the predicate, so the caller cannot read it (#21771); + // a predicate the find refuses still refuses before that question. + enforced: refused ? REFUSED_DENIED : REFUSED_NOT_FOUND, run: withRls(predicate, async (r) => ({ explain: await r.explain('update', 'r2'), enforce: await r.update('r2') })), }, { @@ -853,8 +857,10 @@ const TABLE: Row[] = [ }, { card: '#19963 control', shape: 'private OWD, an `own` writer, a row owned by someone else, update', position: 'record.visible', - // The sharing middleware's by-id write refusal. - enforced: { kind: 'refused', code: 'FORBIDDEN', status: 403 }, + // [#21771] The `own` writer cannot READ a row owned by someone else on a + // private object, so the by-id write answers the read door's not-found + // before the sharing middleware's write refusal is reached. + enforced: REFUSED_NOT_FOUND, run: withSharing({}, async (r) => ({ explain: await r.explain(SHARING_OWN, 'update', 'l_other'), enforce: await r.update(SHARING_OWN, 'l_other'), })), diff --git a/packages/plugins/plugin-security/src/explain-engine.ts b/packages/plugins/plugin-security/src/explain-engine.ts index 5a4b02f89a2..8384bb16609 100644 --- a/packages/plugins/plugin-security/src/explain-engine.ts +++ b/packages/plugins/plugin-security/src/explain-engine.ts @@ -364,6 +364,14 @@ export interface ExplainEngineDeps { * explanation for a `delete` must consult this rather than the update gate. */ canDeleteRecord?: (object: string, recordId: string, context: any) => Promise; + /** + * [#21771] Enforcement's own read question for an addressed by-id write: + * would the read door, asked by this principal, NOT return the record? + * `true` only for absence; a read-time policy refusal answers `false`, and a + * store fault propagates — exactly as the write path asks it, so the + * explanation cannot drift from the answer the write gets. + */ + recordAbsentToCaller?: (object: string, recordId: string, context: any) => Promise; } export interface ExplainInput { @@ -2048,6 +2056,25 @@ export async function explainAccess(deps: ExplainEngineDeps, input: ExplainInput }); recordVerdict = out.record; posture = out.posture; + // [#21771] Ruling A on the write doors: a row the principal cannot READ is + // a row that does not exist, so an update or delete of it answers what a + // nonexistent id answers. The record verdict says so in its own vocabulary + // — the missing-record shape, `visible: false` with no decider — wherever + // enforcement reaches that question: past the capability and object CRUD + // gates (which answer first, as they do here), for a principal with an + // identity, on a record that exists. (A system principal reads every row, + // so the question cannot change its verdict.) + if ( + (operation === 'update' || operation === 'delete') && + deps.recordAbsentToCaller && + context?.userId && + recordVerdict.decidedBy !== 'required_permissions' && + recordVerdict.decidedBy !== 'object_crud' && + !(recordVerdict.visible === false && recordVerdict.decidedBy === undefined) && + (await deps.recordAbsentToCaller(object, input.recordId, context)) + ) { + recordVerdict = { recordId: input.recordId, visible: false }; + } } const decision: ExplainDecision = { diff --git a/packages/plugins/plugin-security/src/explain-json-column-refusal.test.ts b/packages/plugins/plugin-security/src/explain-json-column-refusal.test.ts index 37385d34d64..5279666eb5b 100644 --- a/packages/plugins/plugin-security/src/explain-json-column-refusal.test.ts +++ b/packages/plugins/plugin-security/src/explain-json-column-refusal.test.ts @@ -318,7 +318,10 @@ for (const [driverName, makeDriver, available] of DRIVERS) { for (const { id } of c.rows as Array<{ id: string }>) { const visible = c.visible.includes(id); expect((await w.explain('read', id)).record, `read ${id}`).toEqual({ recordId: id, visible, decidedBy: 'rls' }); - expect((await w.explain('update', id)).record, `update ${id}`).toEqual({ recordId: id, visible, decidedBy: 'rls' }); + // [#21771] A row the caller cannot read answers, on the write side, + // what a nonexistent id answers: the missing-record shape. + expect((await w.explain('update', id)).record, `update ${id}`) + .toEqual(visible ? { recordId: id, visible, decidedBy: 'rls' } : { recordId: id, visible: false }); } }); } diff --git a/packages/plugins/plugin-security/src/get-writable-fields.test.ts b/packages/plugins/plugin-security/src/get-writable-fields.test.ts index bf461203ba1..65131294670 100644 --- a/packages/plugins/plugin-security/src/get-writable-fields.test.ts +++ b/packages/plugins/plugin-security/src/get-writable-fields.test.ts @@ -76,7 +76,11 @@ async function boot(sets: PermissionSet[], opts: { noBaseline?: boolean } = {}) registerMiddleware: (mw: any) => middlewares.push(mw), getSchema: (name: string) => SCHEMAS[name], findOne: vi.fn(async (_object: string, query: any) => - (query?.where?.id === LIVE_DELEGATOR ? { id: LIVE_DELEGATOR, email: 'boss@example.test' } : null)), + (query?.where?.id === LIVE_DELEGATOR + ? { id: LIVE_DELEGATOR, email: 'boss@example.test' } + // [#21771] An addressed by-id update asks the read door for its row; + // the double holds the one row the update names. + : query?.where?.id === PAYLOAD_VALUE.id ? { id: PAYLOAD_VALUE.id, title: 'x' } : null)), }, metadata: { get: async (_type: string, name: string) => SCHEMAS[name], diff --git a/packages/plugins/plugin-security/src/position-catalog-refusal.test.ts b/packages/plugins/plugin-security/src/position-catalog-refusal.test.ts index 652b9da0bfc..a93c653cd83 100644 --- a/packages/plugins/plugin-security/src/position-catalog-refusal.test.ts +++ b/packages/plugins/plugin-security/src/position-catalog-refusal.test.ts @@ -619,9 +619,14 @@ describe("walled posture, two organizations — the predicate reads the WRITER's )); const foreign = envelopeOf(foreignErr); const nowhere = envelopeOf(nowhereErr); - expect([foreign.code, foreign.status], position).toEqual(['PERMISSION_DENIED', 403]); - expect([nowhere.code, nowhere.status], position).toEqual(['PERMISSION_DENIED', 403]); - expect(resolveThrownHttpError(foreignErr).message, position).toBe(resolveThrownHttpError(nowhereErr).message); + // [#21771] Ruling A: a row the writer cannot read answers what a + // nonexistent id answers — the read door's not-found, for both ids. + expect([foreign.code, foreign.status], position).toEqual(['RECORD_NOT_FOUND', 404]); + expect([nowhere.code, nowhere.status], position).toEqual(['RECORD_NOT_FOUND', 404]); + // The one difference the not-found sentence carries is the id the + // writer itself supplied. + expect(resolveThrownHttpError(foreignErr).message.replace('upb_foreign', 'ID'), position) + .toBe(resolveThrownHttpError(nowhereErr).message.replace('up_nowhere', 'ID')); } const [row] = await h.engine.find('sys_user_position', { where: { id: 'upb_foreign' }, context: SYS }); expect(row?.position).toBe('qa_b_only'); diff --git a/packages/plugins/plugin-security/src/security-plugin.test.ts b/packages/plugins/plugin-security/src/security-plugin.test.ts index 8027f814948..87634f7e8f2 100644 --- a/packages/plugins/plugin-security/src/security-plugin.test.ts +++ b/packages/plugins/plugin-security/src/security-plugin.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi } from 'vitest'; import { assertEngineUpdateDispatch, assertEngineFindOnePredicate, type EngineFindOneQueryInput } from '@objectstack/metadata-core'; +import { recordNotFoundError } from '@objectstack/core'; import { SecurityPlugin } from './security-plugin.js'; import { PermissionEvaluator, crudBucketForOperation } from './permission-evaluator.js'; import { FieldMasker } from './field-masker.js'; @@ -502,12 +503,14 @@ describe('SecurityPlugin', () => { }); it('update without owner_id in the change-set is untouched by the guard', async () => { - const harness = await boot([memberSet]); + // [#21771] The addressed by-id update now asks the read door whether the + // caller can read `t1`, so the double answers that read with the row. + const harness = await boot([memberSet], () => ({ id: 't1', owner_id: 'u1' })); const opCtx: any = { object: 'task', operation: 'update', data: { id: 't1', name: 'renamed' }, context: memberCtx(), }; - await harness.run(opCtx); // must not throw, no pre-image read needed + await harness.run(opCtx); // must not throw — the guard reads no owner here }); it('update disowning via owner_id:undefined is denied (mongo $set null hazard)', async () => { @@ -548,7 +551,8 @@ describe('SecurityPlugin', () => { }); it('a prototype-chain owner_id (not own property) does not trip the guard', async () => { - const harness = await boot([memberSet]); + // [#21771] The double answers the addressed write's read question with the row. + const harness = await boot([memberSet], () => ({ id: 't1', owner_id: 'u1' })); const proto = { owner_id: 'attacker' }; const data: any = Object.create(proto); data.id = 't1'; @@ -873,7 +877,9 @@ describe('SecurityPlugin', () => { const harness = makeMiddlewareCtx({ permissionSets: [ownerPolicySet], objectFields: ownerFields, - findOneImpl: () => null, // row exists but filtered out by created_by → not visible + // The owner policies are WRITE-only (`update` / `delete`): the row is + // readable, and filtered out only by `created_by` on the write re-read. + findOneImpl: (q: any) => (q?.where?.$and ? null : { id: 'r1', created_by: 'u2', name: 'theirs' }), }); await plugin.init(harness.ctx); await plugin.start(harness.ctx); @@ -883,10 +889,36 @@ describe('SecurityPlugin', () => { context: memberCtx, }; await expect(harness.run(opCtx)).rejects.toMatchObject({ name: 'PermissionDeniedError' }); - expect(harness.findOne).toHaveBeenCalledTimes(1); + // [#21771] Two reads: the write re-read, then — the row being refused — + // the read question that keeps this caller's 403 (they can read it). + expect(harness.findOne).toHaveBeenCalledTimes(2); // 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' }); + expect(harness.findOne.mock.calls[1][1].where).toEqual({ id: 'r1' }); + }); + + it('[#21771] a row the caller cannot READ answers what a nonexistent id answers, not 403', async () => { + // Every read of `r1` under the caller's context comes back empty: the + // row is hidden from this caller, so the write door says what the read + // door says — the shared not-found producer, never the 403. + for (const operation of ['update', 'delete'] as const) { + 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, + ...(operation === 'update' ? { data: { id: 'r1', name: 'hijack' } } : {}), + options: { where: { id: 'r1' } }, + context: memberCtx, + }; + const err: any = await harness.run(opCtx).then(() => null, (e: unknown) => e); + const missing: any = recordNotFoundError('task', 'r1'); + expect({ code: err?.code, status: err?.status, message: err?.message }, operation) + .toEqual({ code: 'RECORD_NOT_FOUND', status: 404, message: missing.message }); + expect(err?.name, operation).not.toBe('PermissionDeniedError'); + } }); it('ALLOWS an update when the target row IS visible under the write filter (the owner)', async () => { @@ -941,14 +973,18 @@ describe('SecurityPlugin', () => { expect(harness.findOne).toHaveBeenCalledTimes(0); }); - it('SKIPS the check when no RLS policy applies (e.g. modifyAllRecords / admin) — no extra read', async () => { + it('SKIPS the write-class re-read when no RLS policy applies (e.g. modifyAllRecords / admin) — the read question only', async () => { const adminSet: PermissionSet = { name: 'admin_full_access', label: 'Admin', 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 }); + const harness = makeMiddlewareCtx({ + permissionSets: [adminSet], + objectFields: ownerFields, + findOneImpl: (q: any) => (q?.where?.$and ? null : { id: 'r1', name: 'x' }), + }); await plugin.init(harness.ctx); await plugin.start(harness.ctx); const opCtx: any = { @@ -957,7 +993,10 @@ describe('SecurityPlugin', () => { context: { userId: 'admin', roles: ['admin_full_access'], permissions: [] }, }; await expect(harness.run(opCtx)).resolves.toBeDefined(); - expect(harness.findOne).not.toHaveBeenCalled(); + // [#21771] Ruling A asks every principal class whether it can read the + // addressed row — one plain by-id read, and still no write-class re-read. + expect(harness.findOne).toHaveBeenCalledTimes(1); + expect(harness.findOne.mock.calls[0][1].where).toEqual({ id: 'r1' }); }); it('SKIPS the check for a multi-row predicate id ({$in}) — only single-id by-pk writes are guarded', async () => { @@ -1243,7 +1282,9 @@ describe('SecurityPlugin', () => { objectFields: ['id', 'organization_id', 'signed_token'], schemaExtra: { access: { default: 'private' }, requiredPermissions: ['manage_platform_settings'] }, orgScoping: true, - findOneImpl: () => null, // would DENY if the pre-image check ran + // The write-class re-read would DENY if it ran; the read question + // (#21771) finds the row the admin reads. + findOneImpl: (q: any) => (q?.where?.$and ? null : { id: 'r1', signed_token: 'old' }), }); await plugin.init(harness.ctx); await plugin.start(harness.ctx); @@ -1253,7 +1294,8 @@ describe('SecurityPlugin', () => { context: { userId: 'admin', tenantId: 'org-1', roles: ['admin_full_access'], permissions: [] }, }; await expect(harness.run(opCtx)).resolves.toBeDefined(); - expect(harness.findOne).not.toHaveBeenCalled(); + expect(harness.findOne).toHaveBeenCalledTimes(1); + expect(harness.findOne.mock.calls[0][1].where).toEqual({ id: 'r1' }); }); // ADR-0135 D4 / cloud#551 — `managedBy: 'better-auth'` identity tables get the diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index bf796ff10c8..59831b78fbd 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -1,6 +1,8 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. -import { Plugin, PluginContext, POSTURE_LADDER, isRowActive, buildEffectiveObjectPermissions } from '@objectstack/core'; +import { AsyncLocalStorage } from 'node:async_hooks'; +import { declaredHttpStatus } from '@objectstack/types'; +import { Plugin, PluginContext, POSTURE_LADDER, isRowActive, buildEffectiveObjectPermissions, recordNotFoundError } from '@objectstack/core'; import type { EffectiveObjectPermission, PermissionSet, RowLevelSecurityPolicy, TenantLayer0Verdict } from '@objectstack/spec/security'; import { describeHighPrivilegeBits, describeAnchorForbiddenBits, PUBLIC_FORM_SERVER_MANAGED_FIELDS } from '@objectstack/spec/security'; import type { AnchorBindingContext } from '@objectstack/spec/security'; @@ -15,7 +17,7 @@ import type { NumberComparandDoorFieldMeta } from '@objectstack/spec/data'; import { renderOperationMessage } from '@objectstack/spec/system'; // [#19989] The engine's own update-dispatch predicate, asked rather than // re-derived: step 3.6 needs to know which row the ENGINE will write. -import { resolveEngineUpdateDispatch, type EngineUpdateDispatchData } from '@objectstack/metadata-core'; +import { resolveEngineDeleteDispatch, resolveEngineUpdateDispatch, type EngineDeleteDispatchInput, type EngineUpdateDispatchData } from '@objectstack/metadata-core'; import { localWriteEpochSource, resolveWriteEpochSource, @@ -581,6 +583,37 @@ function callerHasOrganizationScope(context: any, posture: TenancyPosture): bool * (`organization_id = …` / `organization_id $in […]`) or `null`, so no legitimate * wall can collide with this test. */ +/** + * [#21771] Ruling A's read question, judged on the answer of a caller-context + * by-id read: is the row ABSENT to this caller? + * + * Three outcomes of the read, and only one is absence (the #7505 rule): + * + * - no row — absent: the row does not exist, or the read door hides it from + * this caller (row-level security, record sharing, a data middleware's + * visibility). Those two are the same answer by ruling, so they are one + * `true` here; + * - a declared 4xx refusal — the read itself was REFUSED (the object's read + * grant withheld, a predicate the driver will not compile). Not a hidden + * row, so `false`: the write keeps the answer it has today; + * - anything else — a store fault, which propagates as raised: an outage is + * neither absence nor a refusal, and reporting it as either would relabel it. + * + * Shared by the write path (step 2.7) and `security/explain`, so the two ask + * one question one way. + */ +async function absentUnderCallerRead(read: () => Promise): Promise { + let row: unknown; + try { + row = await read(); + } catch (e) { + const status = declaredHttpStatus(e); + if (status !== undefined && status < 500) return false; + throw e; + } + return row == null; +} + function isTenantWallDenial(filter: Record | null | undefined): boolean { if (!filter) return false; const keys = Object.keys(filter); @@ -1224,6 +1257,27 @@ export class SecurityPlugin implements Plugin { * that fallback, so the memo is never keyed on `undefined`. */ private epoch: WriteEpochSource = localWriteEpochSource(); + /** + * [#21771] The engine operations in flight through this plugin's middleware, + * as an async scope: the middleware runs the rest of the chain (the engine's + * hooks and driver call included) inside it, so an operation that starts + * while another is still in that chain reads `true` here. + * + * That is the line ruling A draws between the by-id write a caller ADDRESSED + * at a door and the by-id writes the platform issues on the caller's behalf + * under the caller's context: the engine's cascade delete of each dependent + * row, and a hook's `ctx.api` write. Only the addressed write is asked the + * read question ({@link addressedByIdWriteId}). A nested one keeps today's + * behaviour exactly: its target is a row the caller never named, so a + * not-found answer would carry an id the caller never supplied and misstate + * what happened to the write they did address. + * + * Owned here rather than stamped on the context: a context is shared by + * reference and copied by spread across those very sub-writes (the cascade's + * transaction context is a spread), so a stamp would leak in both directions. + * An async scope cannot outlive the chain it wraps. + */ + private readonly engineOperationScope = new AsyncLocalStorage(); /** * This plugin's report sink. Console-backed until a host injects one — see * {@link SecurityReportSink} and {@link CONSOLE_SECURITY_SINK} for the ruling @@ -2150,7 +2204,14 @@ export class SecurityPlugin implements Plugin { const writesExecutedByDataDoor = new WeakSet(); // Register security middleware - ql.registerMiddleware(async (opCtx: any, next: () => Promise) => { + ql.registerMiddleware(async (opCtx: any, chainNext: () => Promise) => { + // [#21771] Whether THIS operation was issued from inside another engine + // operation's pipeline (a cascade, a hook's `ctx.api` write), read before + // this operation opens a scope of its own; and the scoped `next` every + // exit below calls, so whatever the rest of the chain issues reads as + // nested. See {@link engineOperationScope}. + const nestedInOperation = this.engineOperationScope.getStore() === true; + const next = (): Promise => this.engineOperationScope.run(true, chainNext); // [insert-check commit a016f08b8a] (the original card no longer resolves) // The write `check` step 3.6 installs on the operation context // for the engine to run: on an insert, after `beforeInsert`; [#19950] on @@ -2953,8 +3014,23 @@ export class SecurityPlugin implements Plugin { ? await this.computeRlsFilter(delegatorSets, opCtx.object, rlsOperation, delegatorContext) : null; const writeParts = [writeFilter, delWriteFilter].filter(Boolean) as Record[]; + // [#21771] Ruling A on the write doors: a row the caller cannot READ + // is a row that does not exist. For the by-id write the caller + // addressed (see {@link addressedByIdWriteId}), "can you read it?" is + // asked before "may you write it?", and a row the read door would not + // return answers exactly what a nonexistent id answers — the read + // door's own producer, `recordNotFoundError`. Asked for EVERY + // principal class: before this the question was only implicit, inside + // the write-class re-read below, so a principal no write-class row + // filter binds was never asked it and got the gates' 403 for a hidden + // row and a 404 for a missing one. A caller who can read the row but + // may not write it keeps the 403 below; nothing is disclosed to them. + const addressedId = this.addressedByIdWriteId(opCtx, nestedInOperation, referentialFieldClearWrite); if (writeParts.length > 0) { let visible: unknown = null; + // A re-read that THREW is not an absent row: a store fault or a + // read-time policy refusal keeps today's denial below (#7505). + let reReadThrew = false; try { visible = await this.ql.findOne(opCtx.object, { where: { $and: [{ id: targetId }, ...writeParts] }, @@ -2964,6 +3040,15 @@ export class SecurityPlugin implements Plugin { // A read denial (e.g. no read permission) is itself a "cannot // touch this row" signal — fall through to the deny below. visible = null; + reReadThrew = true; + } + if ( + !visible && + !reReadThrew && + addressedId !== undefined && + (await this.callerCannotReadAddressedRow(opCtx, addressedId)) + ) { + throw recordNotFoundError(opCtx.object, addressedId as string | number); } if (!visible) { // [#7451] The refusal an ordinary business user is most likely to @@ -3002,6 +3087,16 @@ export class SecurityPlugin implements Plugin { developerMessage, ); } + } else if ( + addressedId !== undefined && + (await this.callerCannotReadAddressedRow(opCtx, addressedId)) + ) { + // [#21771] The principal no write-class row filter binds: the read + // question is the only row-level question asked here, so a row the + // read door hides (a data middleware's parent-derived visibility + // included) answers not-found before any later gate can tell it + // apart from a missing one. + throw recordNotFoundError(opCtx.object, addressedId as string | number); } } } @@ -5071,6 +5166,10 @@ export class SecurityPlugin implements Plugin { ...(sharing && typeof sharing.canDelete === 'function' ? { canDeleteRecord: async (o: string, rid: string, c: any) => sharing.canDelete(o, rid, await withWriteScope(o, c)) } : {}), + // [#21771] The by-id write path's read question, asked the same way: + // the caller-context by-id read, every data middleware included. + recordAbsentToCaller: (o: string, rid: string, c: any) => + absentUnderCallerRead(() => this.readRowById(o, rid, c)), }, { object, operation, context: targetContext, recordId }, ); @@ -7157,6 +7256,59 @@ export class SecurityPlugin implements Plugin { return masterGovernsRowWrites(schema); } + /** + * [#21771] The id of the row a by-id UPDATE or DELETE writes, when that write + * is the one the caller addressed — else `undefined`, and ruling A's read + * question is not asked. + * + * Three exclusions, each a measured constraint of the ruling's execution: + * + * - **a nested write** ({@link engineOperationScope}): a cascade or a hook + * issued it under the caller's context, so the caller addressed nothing; + * - **the referential FK clear** (`__referentialFieldClear`, server-derived, + * update only): the engine's own integrity write on a row the caller never + * named — the same carve-out step 2 makes for it; + * - **anything the engine does not route by id.** By-id versus predicate is + * the ENGINE's decision, read from its own dispatch predicates + * (`resolveEngineUpdateDispatch` / `resolveEngineDeleteDispatch`) and never + * re-derived: a falsy payload id is no row address to the engine, which + * then writes the row `where.id` names — or, with no `where.id`, takes the + * predicate path, where there is no single row to answer about at all. + * + * Update and delete only — the two verbs the ruling names. The lifecycle + * verbs keep the pre-image check exactly as it was. + */ + private addressedByIdWriteId(opCtx: any, nested: boolean, referentialFieldClear: boolean): unknown { + if (nested || referentialFieldClear) return undefined; + if (opCtx.operation === 'update') { + const data = opCtx.data; + if (data == null || typeof data !== 'object') return undefined; + const route = resolveEngineUpdateDispatch(data as EngineUpdateDispatchData, opCtx.options); + return route.kind === 'by-id' ? route.id : undefined; + } + if (opCtx.operation === 'delete') { + const route = resolveEngineDeleteDispatch(opCtx.options as EngineDeleteDispatchInput | undefined); + return route.kind === 'by-id' ? route.id : undefined; + } + return undefined; + } + + /** + * [#21771] Ruling A's read question for the addressed row: would the read + * door, asked by this caller, return it? Asked through the caller-context + * pre-image read the later gates already share ({@link getCallerPreImage}), + * so it is the read door's own question — every data middleware's read + * visibility included — and never a second visibility evaluator. + * + * Only ABSENCE is "cannot read". A read-time policy refusal (a declared 4xx + * envelope: the object's read grant withheld, a predicate the driver will not + * compile) is not a hidden row, so it answers `false` and the write keeps + * today's answer; a store fault is neither and propagates as raised (#7505). + */ + private async callerCannotReadAddressedRow(opCtx: any, id: unknown): Promise { + return absentUnderCallerRead(() => this.getCallerPreImage(opCtx, id)); + } + 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'); diff --git a/packages/plugins/plugin-security/src/store-fault-fail-closed.test.ts b/packages/plugins/plugin-security/src/store-fault-fail-closed.test.ts index 11d9be88cd7..ef62c0b8c5d 100644 --- a/packages/plugins/plugin-security/src/store-fault-fail-closed.test.ts +++ b/packages/plugins/plugin-security/src/store-fault-fail-closed.test.ts @@ -336,10 +336,13 @@ describe('[#7505] getCallerPreImage — the owner-anchor echo under a store faul // must stay indistinguishable: the pre-image is read under the CALLER's // context precisely so a non-reader learns nothing about ownership. A row // that is not there denies exactly like a row the caller cannot see. + // + // [#21771] …and since ruling A the addressed by-id write asks that read + // FIRST, so both now answer the read door's not-found, one answer still. const h = await boot({ rows: rows() }); const denied = await refusalOf(h.write(setOwner('tsk_missing', USER))); - expect(denied.code).toBe('PERMISSION_DENIED'); - expect(denied.message).toContain('requires the transfer grant'); + expect(denied.code).toBe('RECORD_NOT_FOUND'); + expect(denied.message).toContain('Record tsk_missing not found in crm_task'); }); it('FAULT: the outage propagates instead of being read as "you are not the owner"', async () => { @@ -373,23 +376,27 @@ describe('[#7505] getCallerPreImage — the owner-anchor echo under a store faul }); describe('[#7505] readRowById itself', () => { - it('a write that touches none of the probes is unaffected by a faulting store', async () => { - // The blast radius, asserted rather than assumed: an ordinary field-only - // update on an object with no provenance registry entry and no ownership - // write never reaches `readRowById`, so a store fault does not reach it - // either. Without this, "propagate the fault" could be satisfied by a - // change that refuses every write during an outage. + it('an addressed by-id write now asks the read question, so a faulting store reaches it — and propagates', async () => { + // The blast radius, asserted rather than assumed. Before ruling A + // (#21771) an ordinary field-only update on an object with no provenance + // registry entry and no ownership write never reached `readRowById`. Every + // addressed by-id write now asks the read door whether the caller can read + // its row, so a store fault reaches it — and the fault PROPAGATES as the + // engine raised it: never a 404 that tells an SDK to drop the id, never a + // 403 that relabels an outage as an authorization event. const h = await boot({ rows: { crm_task: [{ id: 'tsk_1', subject: 'Call back', owner_id: USER }] }, faultOn: 'crm_task', }); - await expect( - h.write({ - object: 'crm_task', - operation: 'update', - data: { id: 'tsk_1', subject: 'Renamed' }, - options: { where: { id: 'tsk_1' } }, - }), - ).resolves.toBe('admitted'); + expectPropagatedOutage( + await refusalOf( + h.write({ + object: 'crm_task', + operation: 'update', + data: { id: 'tsk_1', subject: 'Renamed' }, + options: { where: { id: 'tsk_1' } }, + }), + ), + ); }); }); diff --git a/packages/qa/dogfood/test/api-key-owner-revoke.dogfood.test.ts b/packages/qa/dogfood/test/api-key-owner-revoke.dogfood.test.ts index 7b965a2e918..c82a1b93b1e 100644 --- a/packages/qa/dogfood/test/api-key-owner-revoke.dogfood.test.ts +++ b/packages/qa/dogfood/test/api-key-owner-revoke.dogfood.test.ts @@ -206,17 +206,19 @@ describe('#8053: a member revokes their OWN sys_api_key', () => { // ── the "Not affected" list: still not affected ─────────────────────────── - it('[cross-owner] a member may NOT revoke the admin\'s key — 403, row unchanged, key still live', async () => { + it('[cross-owner] a member may NOT revoke the admin\'s key — refused as not found, row unchanged, key still live', async () => { // The row scope is the pre-existing `sys_api_key_self` RLS carve-out. This // is the assertion that proves the new grant is OWNER-scoped and not a // table-wide `update` on a credential table. const { id, raw } = await mintKey(adminToken, 'admins-key'); expect(await keyStillAuthenticates(raw)).toBe(true); + // The member cannot READ the admin's key either, and on the write doors a + // row the caller cannot read answers what a nonexistent id answers. const res = await stack.apiAs(memberToken, 'PATCH', `/data/sys_api_key/${id}`, { revoked: true }); - expect(res.status).toBe(403); + expect(res.status).toBe(404); const body: any = await res.json(); - expect(body.code ?? body.error?.code).toBe('PERMISSION_DENIED'); + expect(body.code ?? body.error?.code).toBe('RECORD_NOT_FOUND'); // Refused, not merely reported as refused: the admin's key is untouched. const { row } = await readKey(adminToken, id); diff --git a/packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts b/packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts index 50edae17cc1..c656d3b5687 100644 --- a/packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts +++ b/packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts @@ -327,9 +327,12 @@ describe('attachments permission matrix (#2755)', () => { // cannot read that record. The gate's own code is pinned on a parent the // caller CAN read (the next case). This used to accept either code because // which one arrived depended on whether the boot was org-bound. + // + // Since the write doors' ruling A that refusal IS the read door's answer: + // a row the caller cannot read answers what a nonexistent id answers. const denied = await stack.apiAs(memberBTok, 'DELETE', `/data/sys_attachment/${row.id}`); - expect(denied.status).toBe(403); - expect(((await denied.json()) as any).code).toBe('PERMISSION_DENIED'); + expect(denied.status).toBe(404); + expect(((await denied.json()) as any).code).toBe('RECORD_NOT_FOUND'); expect(await ql.findOne('sys_attachment', { where: { id: row.id }, context: SYS })).toBeTruthy(); // The uploader (admin) may delete it. diff --git a/packages/qa/dogfood/test/authored-row-write-scope.dogfood.test.ts b/packages/qa/dogfood/test/authored-row-write-scope.dogfood.test.ts index fb96ab185a5..8b7e92975b1 100644 --- a/packages/qa/dogfood/test/authored-row-write-scope.dogfood.test.ts +++ b/packages/qa/dogfood/test/authored-row-write-scope.dogfood.test.ts @@ -76,7 +76,6 @@ import { ObjectSchema, Field } from '@objectstack/spec/data'; import { bootStack, type VerifyStack } from '@objectstack/verify'; import { resolveAuthzContext } from '@objectstack/core'; import { SecurityPlugin, securityDefaultPermissionSets } from '@objectstack/plugin-security'; -import { BUILTIN_OPERATION_MESSAGES } from '@objectstack/spec/system'; // ── the two objects under probe ──────────────────────────────────────────── @@ -375,10 +374,12 @@ describe('[#7281] checkAuthoredRowWrite answers the declaration, not the caller const res = await stack.apiAs(bobToken, 'PATCH', `/data/${object}/${target}`, { body: 'should-not-land' }); expect(res.status, `${object}: a row outside the declaration must be refused`).toBeGreaterThanOrEqual(400); const envelope: any = await res.json().catch(() => ({})); + // On `private` Bob cannot READ the row, and a row the caller cannot read + // answers what a nonexistent id answers (the write doors' ruling A). expect( JSON.stringify(envelope), `${object}: the refusal carries a real error envelope, not a bare throw`, - ).toMatch(/FORBIDDEN|PERMISSION_DENIED/); + ).toMatch(object === CLOSED ? /RECORD_NOT_FOUND/ : /FORBIDDEN|PERMISSION_DENIED/); expect((await rowById(object, target))?.body, `${object}: the row is untouched`).toBe('seed'); } }); @@ -427,22 +428,21 @@ describe('[#7281] checkAuthoredRowWrite answers the declaration, not the caller const target = ids(CLOSED).theirsAdmitted; await expect(security.checkAuthoredRowWrite(CLOSED, target, 'update', bobCtx)).resolves.toBe('admit'); + // The contract question this case left open is now ruled (the write doors' + // ruling A): a by-id write does not land on a row the caller cannot read, + // and the answer is the read door's — what a nonexistent id answers. const res = await stack.apiAs(bobToken, 'PATCH', `/data/${CLOSED}/${target}`, { body: 'e2e-secret' }); - expect(res.status, 'still refused — the pre-image gate reads as the caller').toBe(403); + expect(res.status, 'still refused — the pre-image gate reads as the caller').toBe(404); const envelope: any = await res.json().catch(() => ({})); - expect(envelope?.code, 'ADR-0112 error code').toBe('PERMISSION_DENIED'); - // [#7451] Re-spelled against the catalog CONSTANT. The English developer - // sentence this used to match is now `developerMessage`, logged at the - // throw site and never shipped; what a caller reads is the localized - // `record_access_denied` entry (`en` here — this caller declares no - // locale). It still discriminates exactly as before: the sharing - // middleware's refusal is a different code AND a different sentence - // (`FORBIDDEN: insufficient privileges`), so matching the row gate's own - // catalog entry still proves which gate answered. + expect(envelope?.code, 'ADR-0112 error code').toBe('RECORD_NOT_FOUND'); + // The sentence is the read door's not-found for the id Bob supplied. It + // still discriminates which gate answered: the sharing middleware's + // refusal is a different code AND a different sentence (`FORBIDDEN: + // insufficient privileges`). expect( String(envelope?.error ?? ''), 'the row-level gate refused, NOT the sharing middleware (that shape would be `FORBIDDEN: insufficient privileges`)', - ).toBe(BUILTIN_OPERATION_MESSAGES.en.record_access_denied); + ).toBe(`Record ${target} not found in ${CLOSED}`); // And the developer half stays off the wire — this is the END-TO-END // measurement behind #7414's decision to log rather than ship it. REST's // `mapDataError` builds `{ error, code, object? }` and never reads @@ -461,7 +461,8 @@ describe('[#7281] checkAuthoredRowWrite answers the declaration, not the caller const res = await stack.apiAs(carolToken, 'PATCH', `/data/${object}/${target}`, { body: 'carol-should-not-land' }); expect(res.status, `${object}: Carol declares nothing and must be refused`).toBeGreaterThanOrEqual(400); const envelope: any = await res.json().catch(() => ({})); - expect(JSON.stringify(envelope)).toMatch(/FORBIDDEN|PERMISSION_DENIED/); + // Carol cannot read the `private` row: the read door's not-found (ruling A). + expect(JSON.stringify(envelope)).toMatch(object === CLOSED ? /RECORD_NOT_FOUND/ : /FORBIDDEN|PERMISSION_DENIED/); expect((await rowById(object, target))?.body, `${object}: the row is untouched`).toBe(before); } }); diff --git a/packages/qa/dogfood/test/flow-runas.dogfood.test.ts b/packages/qa/dogfood/test/flow-runas.dogfood.test.ts index c2685aa1166..7fc0dfa147d 100644 --- a/packages/qa/dogfood/test/flow-runas.dogfood.test.ts +++ b/packages/qa/dogfood/test/flow-runas.dogfood.test.ts @@ -125,7 +125,9 @@ describe('objectstack verify FLOW: runAs identity enforcement (#flow-runas)', () // The failure is the RLS refusal on the note, named node-first — not a // generic "flow failed", which would pass while the run died of anything. expect(body.error?.message).toContain(`Node '${failingNodeId}' failed`); - expect(body.error?.message).toMatch(/do not have access to this record/i); + // The member cannot READ the note, so the by-id write answers the read + // door's not-found (the write doors' ruling A), named node-first. + expect(body.error?.message).toMatch(/not found in runas_note/i); // Which node failed survives the envelope change — the reason `summary` // rides in `details` at all. const failed = body.error?.details?.summary?.nodes?.find((n) => n?.nodeId === failingNodeId); diff --git a/packages/qa/dogfood/test/parent-derived-write-refusal-not-visible.dogfood.test.ts b/packages/qa/dogfood/test/parent-derived-write-refusal-not-visible.dogfood.test.ts index ff64431beb4..3d7d7219a38 100644 --- a/packages/qa/dogfood/test/parent-derived-write-refusal-not-visible.dogfood.test.ts +++ b/packages/qa/dogfood/test/parent-derived-write-refusal-not-visible.dogfood.test.ts @@ -228,7 +228,7 @@ describe('parent-derived write refusal on an unreadable parent names nothing', ( for (const c of cases) { for (const verb of ['DELETE', 'PATCH'] as const) { - it(`${c.label} ${verb}: the outside-the-domain caller gets the pre-image check's not-visible refusal, naming nothing`, async () => { + it(`${c.label} ${verb}: the outside-the-domain caller gets the same answer as the org_member, naming nothing`, async () => { const mine = outsideRows[c.key]; const ref = insideRows[c.key]; const payload = verb === 'PATCH' ? c.patch : undefined; @@ -239,12 +239,16 @@ describe('parent-derived write refusal on an unreadable parent names nothing', ( const got = await answer(await outside.stack.apiAs(outside.token, verb, `/data/${c.object}/${mine.row.id}`, payload)); const reference = await answer(await inside.stack.apiAs(inside.token, verb, `/data/${c.object}/${ref.row.id}`, payload)); - // Pin 1 — the reference really is the pre-image check's refusal… - expect(reference.status, reference.text).toBe(403); - expect(reference.body.code).toBe('PERMISSION_DENIED'); - // …and the outside caller's answer is the same answer, field by field. + // Pin 1 — the reference is the by-id write pre-image check's answer + // for a row the caller cannot read, which since the write doors' + // ruling A is the read door's: what a nonexistent id answers… + expect(reference.status, reference.text).toBe(404); + expect(reference.body.code).toBe('RECORD_NOT_FOUND'); + // …and the outside caller's answer is the same answer, field by field + // — the one difference being the id each caller itself supplied. expect(got.status, got.text).toBe(reference.status); - expect(got.body).toEqual(reference.body); + expect(JSON.parse(got.text.split(String(mine.row.id)).join('ID'))) + .toEqual(JSON.parse(reference.text.split(String(ref.row.id)).join('ID'))); // Pin 2 — no parent identity anywhere in the body. const [parentObject, parentId] = mine.parent; diff --git a/packages/qa/dogfood/test/showcase-invoice-seed-isolation.dogfood.test.ts b/packages/qa/dogfood/test/showcase-invoice-seed-isolation.dogfood.test.ts index f0bee6c3a46..b5534debaf9 100644 --- a/packages/qa/dogfood/test/showcase-invoice-seed-isolation.dogfood.test.ts +++ b/packages/qa/dogfood/test/showcase-invoice-seed-isolation.dogfood.test.ts @@ -232,18 +232,21 @@ describe('showcase: seeded invoice/line owner isolation on the shipped contribut expect(foreign.owner).not.toBe(p.email); expect(foreign.status).not.toBe('paid'); const target = await lineUnder(foreign.id); + // The holder cannot read a line under a foreign invoice: on the write + // doors that row answers what a nonexistent id answers (ruling A). const r = await stack.apiAs(tok(), 'PATCH', `/data/showcase_invoice_line/${target.id}`, { quantity: 9 }); - expect(r.status, 'foreign line PATCH').toBe(403); - expect(((await r.json()) as any)?.code).toBe('PERMISSION_DENIED'); + expect(r.status, 'foreign line PATCH').toBe(404); + expect(((await r.json()) as any)?.code).toBe('RECORD_NOT_FOUND'); const after = await ql.findOne('showcase_invoice_line', { where: { id: target.id }, context: SYS }); expect(after?.quantity, 'the foreign line is unchanged').toBe(target.quantity); }); it('cannot re-own a foreign invoice to itself', async () => { const foreign = await invoice(p.foreignUnpaidInvoice); + // A foreign invoice is a row this holder cannot read (ruling A). const r = await stack.apiAs(tok(), 'PATCH', `/data/showcase_invoice/${foreign.id}`, { owner: p.email }); - expect(r.status).toBe(403); - expect(((await r.json()) as any)?.code).toBe('PERMISSION_DENIED'); + expect(r.status).toBe(404); + expect(((await r.json()) as any)?.code).toBe('RECORD_NOT_FOUND'); expect((await invoice(p.foreignUnpaidInvoice)).owner, 'owner unchanged').toBe(foreign.owner); }); diff --git a/packages/qa/dogfood/test/write-door-unreadable-is-not-found.dogfood.test.ts b/packages/qa/dogfood/test/write-door-unreadable-is-not-found.dogfood.test.ts new file mode 100644 index 00000000000..e5a83c3dcf8 --- /dev/null +++ b/packages/qa/dogfood/test/write-door-unreadable-is-not-found.dogfood.test.ts @@ -0,0 +1,330 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// On the write doors, a row the caller cannot READ is a row that does not +// exist — measured at the REST data door on the two parent-derived join +// objects (`sys_attachment`, `sys_comment`), whose read visibility a data +// middleware derives from the row's parent record. +// +// ── what is pinned, per principal class ─────────────────────────────────── +// +// Two classes: a session OUTSIDE the ownership floor's `org_member` domain +// (no write-class row filter binds it) and an `org_member` INSIDE it. For each, +// on update and delete, on both objects: +// +// 1. ⭐ the WHOLE answer for a row the caller cannot read — status, code, +// sentence, envelope — equals the answer for an id that exists nowhere. +// The one difference allowed is the id the caller itself supplied. +// 2. a caller who can read the row but may not write it gets 403, and never +// the not-found: they already see the row. Which gate answers is +// unchanged by the ruling — the by-id write pre-image check's +// `PERMISSION_DENIED` with the record-level sentence where the ownership +// floor binds the verb (inside the domain, on update), the parent-derived +// gate's own named refusal everywhere else. +// 3. a row the caller can read and may write is still written. +// +// Who may write is otherwise unchanged: every refused write leaves its row in +// place. +// +// ── the arming is the pin ───────────────────────────────────────────────── +// +// Each boot proves its principal's domain before anything is measured, as the +// sibling parent-derived refusal pin does: outside must NOT hold `org_member`, +// inside MUST, and both hold the delete grants, or every cell measures an RBAC +// refusal instead. + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { mkdtempSync } from 'node:fs'; +import { promises as fs } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import { defineStack } from '@objectstack/spec'; +import { BUILTIN_OPERATION_MESSAGES } from '@objectstack/spec/system'; +import { SecurityPlugin, securityDefaultPermissionSets } from '@objectstack/plugin-security'; +import { bootStack, type VerifyStack } from '@objectstack/verify'; +import { StorageServicePlugin } from '@objectstack/service-storage'; +import { AuditPlugin } from '@objectstack/plugin-audit'; +import { + AttCase, + AttSecret, + AttReadonly, + attFixtureBaselineSet, + attachmentManagerSet, +} from './fixtures/attachments-fixture.js'; +import { CmtOpen, CmtPrivate, CmtReadonly, commentManagerSet } from './fixtures/comments-fixture.js'; +import { armedWhen, assertArmed, principalArmed, resolveAuthzFor } from './armed.js'; + +const SYS = { isSystem: true } as const; +const RECORD_SENTENCE = BUILTIN_OPERATION_MESSAGES.en.record_access_denied!; + +const stackDefinition = defineStack({ + manifest: { + id: 'com.dogfood.write-door-unreadable-is-not-found', + version: '0.0.0', + type: 'app', + name: 'Write door: an unreadable row is a missing row', + description: + 'Open, private and read-only parents for sys_attachment and sys_comment: the by-id write answer for a row the caller cannot read, beside the answer for a missing id.', + }, + objects: [AttCase, AttSecret, AttReadonly, CmtOpen, CmtPrivate, CmtReadonly], +}); + +function security(): SecurityPlugin { + return new SecurityPlugin({ + defaultPermissionSets: [ + ...securityDefaultPermissionSets, + attFixtureBaselineSet, + attachmentManagerSet, + commentManagerSet, + ], + fallbackPermissionSet: attFixtureBaselineSet.name, + }); +} + +const GRANTS = [attachmentManagerSet.name, commentManagerSet.name]; + +interface Booted { + stack: VerifyStack; + rootDir: string; + ql: any; + adminId: string; + token: string; +} + +async function boot(orgContext: boolean, email: string): Promise { + const rootDir = mkdtempSync(join(tmpdir(), 'write-door-nf-')); + const stack = await bootStack(stackDefinition as never, { + orgContext, + security: security(), + extraPlugins: [ + new StorageServicePlugin({ adapter: 'local', local: { rootDir }, bindToSettings: false }), + new AuditPlugin(), + ], + }); + await stack.signIn(); + const token = await stack.signUp(email); + const ql = await stack.kernel.getServiceAsync('objectql'); + const adminId = (await ql.findOne('sys_user', { where: { email: 'admin@objectos.ai' }, context: SYS }))?.id; + const userId = (await ql.findOne('sys_user', { where: { email }, context: SYS }))?.id; + for (const name of GRANTS) { + const set = await ql.findOne('sys_permission_set', { where: { name }, context: SYS }); + expect(set?.id, `fixture permission set ${name} seeded`).toBeTruthy(); + await ql.insert('sys_user_permission_set', { user_id: userId, permission_set_id: set.id }, { context: { ...SYS } }); + } + return { stack, rootDir, ql, adminId, token }; +} + +async function uploadFile(stack: VerifyStack, token: string): Promise { + const auth = { Authorization: `Bearer ${token}` }; + const presign = await stack.api('/storage/upload/presigned', { + method: 'POST', + headers: { 'Content-Type': 'application/json', ...auth }, + body: JSON.stringify({ filename: 'mine.txt', mimeType: 'text/plain', size: 5, scope: 'attachments' }), + }); + expect(presign.status, 'presign').toBe(200); + const { data } = (await presign.json()) as any; + const put = await stack.raw(String(data.uploadUrl).replace(/^https?:\/\/[^/]+/, ''), { + method: 'PUT', + headers: data.headers ?? { 'content-type': 'text/plain' }, + body: 'hello', + }); + expect(put.status, 'raw PUT').toBeLessThan(300); + const complete = await stack.api('/storage/upload/complete', { + method: 'POST', + headers: { 'Content-Type': 'application/json', ...auth }, + body: JSON.stringify({ fileId: data.fileId }), + }); + expect(complete.status, 'complete').toBe(200); + return String(data.fileId); +} + +/** Rows the admin owns: one attachment and one comment on a parent only the admin reads, one of each on a parent everyone reads. */ +async function seed(b: Booted) { + const secret = await b.ql.insert('att_secret', { name: 'hidden parent', owner_id: b.adminId }, { context: { ...SYS } }); + const readonly = await b.ql.insert('att_readonly', { name: 'read-only parent', owner_id: b.adminId }, { context: { ...SYS } }); + const cmtSecret = await b.ql.insert('cmt_private', { name: 'hidden thread parent', owner_id: b.adminId }, { context: { ...SYS } }); + const cmtReadonly = await b.ql.insert('cmt_readonly', { name: 'read-only thread parent', owner_id: b.adminId }, { context: { ...SYS } }); + const attachment = (parentObject: string, parentId: string) => + b.ql.insert( + 'sys_attachment', + { + parent_object: parentObject, + parent_id: parentId, + file_id: `f_${parentId}`, + file_name: 'a.txt', + mime_type: 'text/plain', + size: 1, + uploaded_by: b.adminId, + }, + { context: { ...SYS } }, + ); + const comment = (threadObject: string, parentId: string) => + b.ql.insert( + 'sys_comment', + { thread_id: `${threadObject}:${parentId}`, body: 'the admin wrote this', author_id: b.adminId }, + { context: { ...SYS } }, + ); + return { + sys_attachment: { + hidden: String((await attachment('att_secret', secret.id)).id), + readable: String((await attachment('att_readonly', readonly.id)).id), + }, + sys_comment: { + hidden: String((await comment('cmt_private', cmtSecret.id)).id), + readable: String((await comment('cmt_readonly', cmtReadonly.id)).id), + }, + }; +} + +/** The caller's own attachment and comment, created through the door on parents it may edit. */ +async function seedOwn(b: Booted): Promise<{ sys_attachment: string; sys_comment: string }> { + const caseRow = await b.ql.insert('att_case', { name: 'open parent' }, { context: { ...SYS } }); + const fileId = await uploadFile(b.stack, b.token); + const att = await b.stack.apiAs(b.token, 'POST', '/data/sys_attachment', { + parent_object: 'att_case', + parent_id: caseRow.id, + file_id: fileId, + file_name: 'mine.txt', + mime_type: 'text/plain', + size: 5, + }); + expect(att.status, `own attachment: ${await att.clone().text()}`).toBeLessThan(300); + const openRow = await b.ql.insert('cmt_open', { name: 'open thread parent' }, { context: { ...SYS } }); + const cmt = await b.stack.apiAs(b.token, 'POST', '/data/sys_comment', { thread_id: `cmt_open:${openRow.id}`, body: 'mine' }); + expect(cmt.status, `own comment: ${await cmt.clone().text()}`).toBeLessThan(300); + const attId = (await b.ql.findOne('sys_attachment', { where: { file_id: fileId }, context: SYS }))?.id; + const cmtId = (await b.ql.findOne('sys_comment', { where: { thread_id: `cmt_open:${openRow.id}` }, context: SYS }))?.id; + return { sys_attachment: String(attId), sys_comment: String(cmtId) }; +} + +/** Status plus parsed body, the caller-supplied id written out of it. */ +async function answerOf(res: Response, id: string): Promise<{ status: number; body: any; text: string }> { + const text = await res.text(); + let body: any; + try { + body = JSON.parse(text.split(id).join('ID')); + } catch { + body = text; + } + return { status: res.status, body, text }; +} + +type Rows = Awaited>; +type Own = Awaited>; + +const OBJECTS = [ + { + object: 'sys_attachment' as const, + patch: { description: 'rewritten' }, + readerCode: { + outside: { DELETE: 'ATTACHMENT_DELETE_DENIED', PATCH: 'RECORD_NOT_ACCESSIBLE' }, + inside: { DELETE: 'ATTACHMENT_DELETE_DENIED', PATCH: 'PERMISSION_DENIED' }, + }, + }, + { + object: 'sys_comment' as const, + patch: { body: 'rewritten' }, + readerCode: { + outside: { DELETE: 'RECORD_NOT_ACCESSIBLE', PATCH: 'RECORD_NOT_ACCESSIBLE' }, + inside: { DELETE: 'RECORD_NOT_ACCESSIBLE', PATCH: 'PERMISSION_DENIED' }, + }, + }, +]; +const VERBS = ['DELETE', 'PATCH'] as const; + +describe('write door: a row the caller cannot read answers what a missing id answers', () => { + const classes: Record<'outside' | 'inside', { b?: Booted; rows?: Rows; own?: Own }> = { outside: {}, inside: {} }; + + beforeAll(async () => { + classes.outside.b = await boot(false, 'nf-outside@verify.test'); + classes.inside.b = await boot(true, 'nf-inside@verify.test'); + const outside = classes.outside.b; + const inside = classes.inside.b; + + await assertArmed([ + armedWhen({ + control: "a principal OUTSIDE the ownership floor's domain — no write-class row filter binds it", + disarmedBy: 'an org-bound boot of this half: the principal then holds org_member and the two classes collapse into one', + observe: () => resolveAuthzFor(outside.stack, outside.token), + armed: (ctx) => !ctx.positions.includes('org_member') && GRANTS.every((g) => ctx.permissions.includes(g)), + describe: (ctx) => `positions=${JSON.stringify(ctx.positions)} permissions=${JSON.stringify(ctx.permissions)}`, + }), + principalArmed({ + stack: inside.stack, + token: inside.token, + who: 'the org_member', + positions: ['org_member'], + permissions: GRANTS, + control: "a principal INSIDE the ownership floor's domain — the write-class row filter binds it", + disarmedBy: 'an org-less boot of this half: no org_member, and the inside class measures the outside one twice', + }), + ]); + + for (const k of ['outside', 'inside'] as const) { + classes[k].rows = await seed(classes[k].b!); + classes[k].own = await seedOwn(classes[k].b!); + } + }, 240_000); + + afterAll(async () => { + for (const k of ['outside', 'inside'] as const) { + const b = classes[k].b; + await b?.stack?.stop(); + if (b?.rootDir) await fs.rm(b.rootDir, { recursive: true, force: true }); + } + }); + + for (const who of ['outside', 'inside'] as const) { + for (const o of OBJECTS) { + for (const verb of VERBS) { + it(`${who} · ${o.object} · ${verb}: the hidden row's whole answer equals the missing id's`, async () => { + const { b, rows } = classes[who] as { b: Booted; rows: Rows }; + const hiddenId = rows[o.object].hidden; + const missingId = `nf_missing_${who}_${verb}`; + const payload = verb === 'PATCH' ? o.patch : undefined; + + // The precondition, read off the read door: the caller does not see it. + expect((await b.stack.apiAs(b.token, 'GET', `/data/${o.object}/${hiddenId}`)).status).toBe(404); + + const hidden = await answerOf(await b.stack.apiAs(b.token, verb, `/data/${o.object}/${hiddenId}`, payload), hiddenId); + const missing = await answerOf(await b.stack.apiAs(b.token, verb, `/data/${o.object}/${missingId}`, payload), missingId); + + expect(missing.status, missing.text).toBe(404); + expect(missing.body.code).toBe('RECORD_NOT_FOUND'); + expect({ status: hidden.status, body: hidden.body }, hidden.text).toEqual({ status: missing.status, body: missing.body }); + + // Refused means untouched. + const after = await b.ql.findOne(o.object, { where: { id: hiddenId }, context: SYS }); + expect(after, 'the refused write left the row in place').toBeTruthy(); + if (verb === 'PATCH') for (const [k, v] of Object.entries(o.patch)) expect(after[k]).not.toBe(v); + }); + + it(`${who} · ${o.object} · ${verb}: a reader who may not write still gets 403`, async () => { + const { b, rows } = classes[who] as { b: Booted; rows: Rows }; + const id = rows[o.object].readable; + expect((await b.stack.apiAs(b.token, 'GET', `/data/${o.object}/${id}`)).status).toBe(200); + + const got = await answerOf( + await b.stack.apiAs(b.token, verb, `/data/${o.object}/${id}`, verb === 'PATCH' ? o.patch : undefined), + id, + ); + expect(got.status, got.text).toBe(403); + expect(got.body.code).toBe(o.readerCode[who][verb]); + if (got.body.code === 'PERMISSION_DENIED') expect(got.body.error).toBe(RECORD_SENTENCE); + expect(await b.ql.findOne(o.object, { where: { id }, context: SYS })).toBeTruthy(); + }); + } + + it(`${who} · ${o.object}: a row the caller reads and may write is still written, then deleted`, async () => { + const { b, own } = classes[who] as { b: Booted; own: Own }; + const id = own[o.object]; + const patched = await b.stack.apiAs(b.token, 'PATCH', `/data/${o.object}/${id}`, o.patch); + expect(patched.status, await patched.clone().text()).toBeLessThan(300); + const after = await b.ql.findOne(o.object, { where: { id }, context: SYS }); + for (const [k, v] of Object.entries(o.patch)) expect(after[k]).toBe(v); + const deleted = await b.stack.apiAs(b.token, 'DELETE', `/data/${o.object}/${id}`); + expect(deleted.status, await deleted.clone().text()).toBeLessThan(300); + expect(await b.ql.findOne(o.object, { where: { id }, context: SYS })).toBeFalsy(); + }); + } + } +}); diff --git a/packages/services/service-storage/src/attachment-access-hooks.ts b/packages/services/service-storage/src/attachment-access-hooks.ts index 29e2ef0bb33..3e932d379cf 100644 --- a/packages/services/service-storage/src/attachment-access-hooks.ts +++ b/packages/services/service-storage/src/attachment-access-hooks.ts @@ -104,6 +104,12 @@ function forbid(code: string, message: string, object?: string): never { * The code of the platform's not-visible refusal: the one plugin-security's * by-id write pre-image check throws when the caller's own read visibility * does not reach the target row (`PermissionDeniedError`, 403). + * + * [#21771] No longer the pre-image check's answer for a row the caller cannot + * read: under the write doors' ruling A that check asks the caller's read + * visibility for every principal, and answers the by-id update or delete the + * caller addressed with what a nonexistent id answers, before this gate runs. + * This refusal stays the gate's own, for a write the gate answers first. */ const NOT_VISIBLE_CODE: StandardErrorCode = 'PERMISSION_DENIED'; const NOT_VISIBLE_STATUS = 403; diff --git a/packages/spec/src/migrations/entries/semantic/18.by-id-write-unreadable-row-not-found.ts b/packages/spec/src/migrations/entries/semantic/18.by-id-write-unreadable-row-not-found.ts new file mode 100644 index 00000000000..62d67327c33 --- /dev/null +++ b/packages/spec/src/migrations/entries/semantic/18.by-id-write-unreadable-row-not-found.ts @@ -0,0 +1,38 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import type { SemanticMigration } from '../../types.js'; + +// A write-door answer and verdict, not an authorable key: there is no D2 +// conversion and nothing for `objectstack migrate meta` to rewrite. The entry +// carries the changed answer to the one reader the ledger serves here — the +// upgrade guide — because a client that branched on the old 403 has no schema +// error to find it by. No backticks in `surface`: the upgrade guide renders it +// inside a code span and a table cell. +export const entry: SemanticMigration = { + id: 'by-id-write-unreadable-row-not-found', + surface: + 'the data write doors — a by-id update or delete of a row the caller cannot read, on every ' + + 'object and for every principal', + replacement: + 'read 404 `RECORD_NOT_FOUND` on a by-id update or delete as "no row you can see has this id" — ' + + 'the read door\'s meaning — and keep a 403 for a row the caller can read but may not write', + reason: + 'A WRITE-DOOR ANSWER, made one with the read door\'s. A by-id update or delete of a row the ' + + 'caller cannot read used to answer a 403 — `PERMISSION_DENIED` where a write-class row filter ' + + 'binds the caller, otherwise a later gate\'s own 403, such as `FORBIDDEN` from record sharing ' + + 'or a parent-derived gate\'s code on attachments and comments — while an id that names no row ' + + 'answered 404, so the write door told a hidden row apart from a missing one. The by-id write ' + + 'pre-image check now asks every principal whether it can read the row it addressed, through ' + + 'a by-id read in its own context that every data middleware\'s visibility applies to, and ' + + 'answers a row that read does not return with the read door\'s not-found: the same code, ' + + 'status and body a nonexistent id gets. It also refuses a by-id write a principal no row ' + + 'filter binds could previously land on a row hidden from it, such as an attachment\'s ' + + 'uploader or a comment\'s author whose parent record they can no longer read. A caller who ' + + 'can read the row but may not write it keeps its 403. Writes the platform issues under the ' + + 'caller\'s context — the engine\'s cascade delete, a hook\'s write, the referential clear of a ' + + 'lookup — keep their previous answer, and writes not routed by id are unchanged.', + acceptanceCriteria: + 'Every client that handles a by-id update or delete treats 404 `RECORD_NOT_FOUND` as "not ' + + 'found or not visible" and no longer reads a 403 there as proof the row exists; an operator ' + + 'who needs a user to write a row grants that user read access to it first.', +}; diff --git a/packages/spec/src/migrations/registry.ts b/packages/spec/src/migrations/registry.ts index 0a0c1fbdbef..3ee1b942cdf 100644 --- a/packages/spec/src/migrations/registry.ts +++ b/packages/spec/src/migrations/registry.ts @@ -8114,6 +8114,40 @@ const step18: MigrationStep = { + 'value, so no source rewrite ships and `objectstack migrate meta` has ' + 'nothing to visit.', }, + // A write-door answer and verdict, not an authorable key: there is no D2 + // conversion and nothing for `objectstack migrate meta` to rewrite. The entry + // carries the changed answer to the one reader the ledger serves here — the + // upgrade guide — because a client that branched on the old 403 has no schema + // error to find it by. No backticks in `surface`: the upgrade guide renders it + // inside a code span and a table cell. + { + id: 'by-id-write-unreadable-row-not-found', + surface: + 'the data write doors — a by-id update or delete of a row the caller cannot read, on every ' + + 'object and for every principal', + replacement: + 'read 404 `RECORD_NOT_FOUND` on a by-id update or delete as "no row you can see has this id" — ' + + 'the read door\'s meaning — and keep a 403 for a row the caller can read but may not write', + reason: + 'A WRITE-DOOR ANSWER, made one with the read door\'s. A by-id update or delete of a row the ' + + 'caller cannot read used to answer a 403 — `PERMISSION_DENIED` where a write-class row filter ' + + 'binds the caller, otherwise a later gate\'s own 403, such as `FORBIDDEN` from record sharing ' + + 'or a parent-derived gate\'s code on attachments and comments — while an id that names no row ' + + 'answered 404, so the write door told a hidden row apart from a missing one. The by-id write ' + + 'pre-image check now asks every principal whether it can read the row it addressed, through ' + + 'a by-id read in its own context that every data middleware\'s visibility applies to, and ' + + 'answers a row that read does not return with the read door\'s not-found: the same code, ' + + 'status and body a nonexistent id gets. It also refuses a by-id write a principal no row ' + + 'filter binds could previously land on a row hidden from it, such as an attachment\'s ' + + 'uploader or a comment\'s author whose parent record they can no longer read. A caller who ' + + 'can read the row but may not write it keeps its 403. Writes the platform issues under the ' + + 'caller\'s context — the engine\'s cascade delete, a hook\'s write, the referential clear of a ' + + 'lookup — keep their previous answer, and writes not routed by id are unchanged.', + acceptanceCriteria: + 'Every client that handles a by-id update or delete treats 404 `RECORD_NOT_FOUND` as "not ' + + 'found or not visible" and no longer reads a 403 there as proof the row exists; an operator ' + + 'who needs a user to write a row grants that user read access to it first.', + }, { id: 'cache-warmup-scheduled-strategy-retired', // No backticks in `surface` — build-upgrade-guide.ts renders it inside a code