From 65849e4fe6866ae7738f0aac3ea45f8692ff91a8 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 03:53:02 +0000 Subject: [PATCH] fix(plugin-security)!: split controlled_by_parent write refusals by true semantics (#7474) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `assertControlledByParentWrite` funnelled SIX distinct conditions through one `deny()` helper, so all six answered `403 PERMISSION_DENIED` with one sentence — "requires edit access to its master record". Three of them are genuine authorization verdicts. The other three are not verdicts at all, and the shared sentence was a false statement carrying a false remedy: "ask whoever owns the parent record" cannot fix a null master FK, a deleted row, or an object that declares `controlled_by_parent` with no `master_detail` relation to derive access from. Per the maintainer ruling of 2026-08-11 on #7474, the three genuine legs (no object-level `update` on the master / master row outside the write RLS / no `edit`-level share grant) keep `403 PERMISSION_DENIED` and their exact wording. The three non-verdict legs get envelopes of their own, all drawn from the existing ADR-0112 vocabulary — no new error code: - `controlled_by_parent` with no `master_detail` → 422 INVALID_METADATA - target detail row does not exist → 404 RECORD_NOT_FOUND - detail's master reference is empty → 422 MISSING_REQUIRED_FIELD Each new message opens with a prefix of its own rather than `[Security] Access denied`: that exact prefix is a MATCHER at both transports, so borrowing it would re-flatten the split back to 403 on the wire. The explanation lives in `message` and never in `details`, which is not a carrier the client can rely on (#7450). Throwing directly (instead of routing through a `never`-returning helper) also retires the non-null assertions the metadata-defect branch used to need — `deny()` returning `never` only by throwing was the load-bearing half of the same defect. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019sXg2v6khHim6XdmRoAXje --- .../split-controlled-by-parent-deny-legs.md | 27 ++ .../src/controlled-by-parent-sharing.test.ts | 256 +++++++++++++++++- .../plugins/plugin-security/src/errors.ts | 139 ++++++++++ .../plugin-security/src/security-plugin.ts | 63 ++++- 4 files changed, 466 insertions(+), 19 deletions(-) create mode 100644 .changeset/split-controlled-by-parent-deny-legs.md diff --git a/.changeset/split-controlled-by-parent-deny-legs.md b/.changeset/split-controlled-by-parent-deny-legs.md new file mode 100644 index 0000000000..ededcad2d0 --- /dev/null +++ b/.changeset/split-controlled-by-parent-deny-legs.md @@ -0,0 +1,27 @@ +--- +'@objectstack/plugin-security': minor +--- + +Split `controlled_by_parent` write refusals by true semantics: three of the six legs stop answering `403 PERMISSION_DENIED` + +A by-id write to a `controlled_by_parent` detail is refused for six distinct reasons, and all six used to answer with one envelope and one sentence — `403 PERMISSION_DENIED: … requires edit access to its master record`. Only three of them are authorization verdicts. The other three said something untrue and prescribed a remedy that could not work: "ask whoever owns the parent record" cannot fix a null master reference, a deleted row, or an object that declares `controlled_by_parent` with no `master_detail` relation to derive access from. + +Unchanged — the three genuine verdicts keep `403 PERMISSION_DENIED` and their exact wording: + +- the caller holds no object-level `update` on the master +- the master row lies outside the caller's write RLS +- the master carries no `edit`-level share grant + +Changed — the three non-verdict conditions now answer for what they are: + +| condition | before | after | +|---|---|---| +| `controlled_by_parent` declared with no `master_detail` relation | `403 PERMISSION_DENIED` | `422 INVALID_METADATA` | +| the target detail row does not exist | `403 PERMISSION_DENIED` | `404 RECORD_NOT_FOUND` | +| the detail's master reference is empty | `403 PERMISSION_DENIED` | `422 MISSING_REQUIRED_FIELD` | + +Each carries a message written for the app author, naming the object, the operation and the remedy. The metadata-defect case is the one that matters most: it is a precisely detectable authoring defect that was disguised as routine RBAC noise, so nobody ever investigated it — and a false 403 steers debugging, human or agent, toward permission changes when the truth is broken metadata. + +The 404 does not widen what a caller can learn. The detail row is probed under a system context, so a row hidden from the caller by row-level security is still found and falls through to the authorization legs; object-level CRUD and the row-level write pre-image check both run before this gate. Absence there is real absence. + +All codes come from the existing ADR-0112 vocabulary — no new error code is introduced. 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 ca585aac6e..84b83355d9 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 @@ -65,6 +65,24 @@ const CONTACT_SCHEMA = { }, }; +/** + * [#7474] The same detail, AUTHORED WRONG: `controlled_by_parent` with nothing + * to derive access from — no master_detail field, and no required lookup for + * `resolveCbpRelation` to fall back to. The gate's metadata-defect leg is the + * only thing between this object and a write that answers a false 403. + */ +const CONTACT_SCHEMA_NO_MASTER_DETAIL = { + name: 'crm_contact', + sharingModel: 'controlled_by_parent', + fields: { + id: { name: 'id', type: 'text' }, + name: { name: 'name', type: 'text' }, + // A lookup, but OPTIONAL — the third fallback `resolveCbpRelation` tries + // requires `required: true`, so this object resolves to no relation at all. + account: { name: 'account', type: 'lookup', reference: 'crm_account' }, + }, +}; + const SHARE_SCHEMA = { name: 'sys_record_share', isSystem: true, @@ -106,10 +124,10 @@ type Row = Record; * security plugin itself uses, so a filter this suite asserts on is a filter * that was really applied rather than one merely inspected. */ -function makeStore(rows: Record) { +function makeStore(rows: Record, brokenDetail = false) { const schemas: Record = { crm_account: ACCOUNT_SCHEMA, - crm_contact: CONTACT_SCHEMA, + crm_contact: brokenDetail ? CONTACT_SCHEMA_NO_MASTER_DETAIL : CONTACT_SCHEMA, sys_record_share: SHARE_SCHEMA, }; return { @@ -160,11 +178,24 @@ interface BootOptions { shareLevel?: 'read' | 'edit' | null; /** `'none'` boots a deployment WITHOUT plugin-sharing; `'throws'` a broken one. */ sharing?: 'real' | 'none' | 'throws'; + /** + * [#7474] `'no-master-detail'` swaps the detail's schema for the AUTHORING + * DEFECT the metadata-defect leg exists to report: `controlled_by_parent` + * declared on an object with no relation to derive access from. + */ + detail?: 'valid' | 'no-master-detail'; + /** [#7474] Replace the detail rows — e.g. one whose master FK is null. */ + contacts?: Row[]; + /** [#7474] Replace the caller's permission set (the master-CRUD / master-RLS legs). */ + sets?: PermissionSet[]; } async function boot(options: BootOptions = {}) { const shareLevel = options.shareLevel === undefined ? 'edit' : options.shareLevel; - const store = makeStore(fixtureRows(shareLevel)); + const fixture = fixtureRows(shareLevel); + if (options.contacts) fixture.crm_contact = options.contacts; + const store = makeStore(fixture, options.detail === 'no-master-detail'); + const sets = options.sets ?? [REP_SET]; let middleware: any; const ql = { @@ -179,7 +210,7 @@ async function boot(options: BootOptions = {}) { const services: Record = { manifest: { register: vi.fn() }, objectql: ql, - metadata: { get: async (n: string) => store.getSchema(n), list: async () => [REP_SET] }, + metadata: { get: async (n: string) => store.getSchema(n), list: async () => sets }, }; if ((options.sharing ?? 'real') === 'real') { // The REAL sharing service over the same store — the point of the fix is @@ -206,7 +237,7 @@ async function boot(options: BootOptions = {}) { }, }; const plugin = new SecurityPlugin({ - defaultPermissionSets: [REP_SET], + defaultPermissionSets: sets, fallbackPermissionSet: 'crm_rep', }); await plugin.init(ctx); @@ -266,6 +297,18 @@ async function boot(options: BootOptions = {}) { return out; }; + /** [#7474] The INSERT face — the one leg that reads the master FK off the body. */ + const insertContact = async (data: Row): Promise => { + const opCtx: any = { + object: 'crm_contact', + operation: 'insert', + data, + options: {}, + context: repContext(), + }; + await middleware(opCtx, async () => {}); + }; + return { store, ctx, @@ -274,6 +317,7 @@ async function boot(options: BootOptions = {}) { visibleContacts, analyticsVisibleContacts, updateContact, + insertContact, writableContacts, }; } @@ -467,3 +511,205 @@ describe('[#5815] getReadFilter enforces the same read scope as the engine middl expect(await h.analyticsVisibleContacts(delegated)).toEqual([]); }); }); + +// --------------------------------------------------------------------------- + +/** + * [#7474] SIX conditions refuse a write in `assertControlledByParentWrite`, and + * until the maintainer ruling of 2026-08-11 they answered with ONE sentence and + * ONE code: `403 PERMISSION_DENIED — requires edit access to its master record`. + * + * Three of them are genuine authorization verdicts and keep exactly that. Three + * are not verdicts at all — a broken `master_detail` declaration, a row that + * does not exist, a null master FK — and the shared sentence was a false + * statement with a false remedy: "ask whoever owns the parent record" cannot + * fix any of them. Worse for the app author, the metadata defect is a precisely + * detectable AUTHORING error that was wearing the costume of routine RBAC + * noise, which is the class of thing nobody ever investigates. + * + * ## Why these cases assert `code` and `status`, never just a throw + * + * The defect is the ENVELOPE, not the refusal: every one of these six threw + * before this change too. A `rejects.toThrow()` — or a message-only assertion — + * carries one bit where the defect has two, so it stays green on precisely the + * behaviour the issue reported. The minimum here is therefore the ADR-0112 pair + * (`code` + `status`), plus the message where the WORDING is the contract: the + * three authorization legs must keep their sentence verbatim, because that + * sentence is what a user reads and what consumers already pin. + * + * The pinned pairs are the split itself: + * + * | leg | status | code | + * |----------------------------------------|--------|-------------------------| + * | no object-level `update` on the master | 403 | PERMISSION_DENIED | + * | master row outside the write RLS | 403 | PERMISSION_DENIED | + * | no `edit`-level share grant | 403 | PERMISSION_DENIED | + * | controlled_by_parent, no master_detail | 422 | INVALID_METADATA | + * | target record not found | 404 | RECORD_NOT_FOUND | + * | detail has no master reference | 422 | MISSING_REQUIRED_FIELD | + */ +describe('[#7474] the six refusal legs answer with six envelopes, not one', () => { + /** The thrown error itself — `rejects.toThrow` cannot see `code` / `status`. */ + const refusalOf = async (run: Promise): Promise => { + try { + await run; + } catch (e) { + return e; + } + throw new Error('expected the write to be refused, but it resolved'); + }; + + /** REP_SET with the master's object-level `update` withheld. */ + const NO_MASTER_EDIT: PermissionSet = { + name: 'crm_rep', + label: 'CRM Rep', + objects: { + crm_account: { allowRead: true, allowCreate: true, allowEdit: false, allowDelete: true }, + crm_contact: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true }, + }, + } as unknown as PermissionSet; + + /** REP_SET plus a write RLS on the MASTER that matches no row in the fixture. */ + const MASTER_WRITE_RLS: PermissionSet = { + name: 'crm_rep', + label: 'CRM Rep', + objects: { + crm_account: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true }, + crm_contact: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true }, + }, + rowLevelSecurity: [ + { object: 'crm_account', operation: 'update', using: "name = 'No Such Corp'" }, + ], + } as unknown as PermissionSet; + + // ── the three GENUINE authorization verdicts — unchanged, verbatim ──────── + + it('403 PERMISSION_DENIED: the caller holds no object-level update on the master', async () => { + const h = await boot({ shareLevel: 'edit', sets: [NO_MASTER_EDIT] }); + const err = await refusalOf(h.updateContact('ct_own')); + expect(err.code).toBe('PERMISSION_DENIED'); + expect(err.statusCode).toBe(403); + // The sentence is contract on this leg — a user reads it, and consumers + // pin it. It must survive the split byte-for-byte. + expect(err.message).toContain("requires edit access to its master record"); + expect(err.message).toContain("no edit permission on master 'crm_account'"); + }); + + it('403 PERMISSION_DENIED: the master row is outside the caller\'s write RLS', async () => { + const h = await boot({ shareLevel: 'edit', sets: [MASTER_WRITE_RLS] }); + const err = await refusalOf(h.updateContact('ct_own')); + expect(err.code).toBe('PERMISSION_DENIED'); + expect(err.statusCode).toBe(403); + expect(err.message).toContain('requires edit access to its master record'); + expect(err.message).toContain('row-level security'); + }); + + it('403 PERMISSION_DENIED: the master carries no edit-level share grant', async () => { + const h = await boot({ shareLevel: 'read' }); + const err = await refusalOf(h.updateContact('ct_us')); + expect(err.code).toBe('PERMISSION_DENIED'); + expect(err.statusCode).toBe(403); + expect(err.message).toContain('requires edit access to its master record'); + expect(err.message).toContain('record sharing'); + }); + + // ── the three NON-VERDICT legs — split by true semantics ────────────────── + + it('422 INVALID_METADATA: controlled_by_parent declared with no master_detail relation', async () => { + const h = await boot({ shareLevel: 'edit', detail: 'no-master-detail' }); + const err = await refusalOf(h.updateContact('ct_own')); + expect(err.code).toBe('INVALID_METADATA'); + expect(err.status).toBe(422); + expect(err.statusCode).toBe(422); + // The remedy has to be IN the message: `details` is not a carrier the + // client can rely on (#7450), so the sentence is the whole fix-it. + expect(err.message).toContain('no master_detail relation'); + expect(err.message).toContain('Declare a required master_detail field'); + // …and it must NOT wear the 403's costume: that prefix is a MATCHER at both + // transports, so borrowing it would re-flatten this to PERMISSION_DENIED. + expect(err.message).not.toContain('[Security] Access denied'); + expect(err.message).not.toContain('requires edit access to its master record'); + }); + + it('404 RECORD_NOT_FOUND: the by-id write targets a detail row that does not exist', async () => { + const h = await boot({ shareLevel: 'edit' }); + 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); + expect(err.message).toContain('ct_deleted_concurrently'); + expect(err.message).not.toContain('requires edit access to its master record'); + }); + + it('404 is real absence, not an RLS-hidden row: the probe reads under a SYSTEM context', async () => { + // The one thing a 404 must never become is an oracle. `ct_eu` exists but + // its master is unreachable to the caller — the row is read as system, so + // it is FOUND here and falls through to the authorization leg. Both halves + // asserted together: this is what makes the 404 above mean "absent". + const h = await boot({ shareLevel: 'edit' }); + const hidden = await refusalOf(h.updateContact('ct_eu')); + expect(hidden.code).toBe('PERMISSION_DENIED'); + expect(hidden.statusCode).toBe(403); + expect(hidden.message).toContain('requires edit access to its master record'); + }); + + it('422 MISSING_REQUIRED_FIELD: the stored detail row has no master reference', async () => { + const h = await boot({ + shareLevel: 'edit', + contacts: [{ id: 'ct_orphan', name: 'Orphan contact', account: null }], + }); + const err = await refusalOf(h.updateContact('ct_orphan')); + expect(err.code).toBe('MISSING_REQUIRED_FIELD'); + expect(err.status).toBe(422); + expect(err.statusCode).toBe(422); + // The stored-row wording names the row, because the caller cannot fix this + // one by sending a different payload. + expect(err.message).toContain("record 'ct_orphan' has no value in 'account'"); + expect(err.message).not.toContain('requires edit access to its master record'); + }); + + it('422 MISSING_REQUIRED_FIELD: an insert that omits the master reference', async () => { + const h = await boot({ shareLevel: 'edit' }); + const err = await refusalOf(h.insertContact({ name: 'New contact' })); + expect(err.code).toBe('MISSING_REQUIRED_FIELD'); + expect(err.status).toBe(422); + // Same condition, same code, and the wording says which shape it is: the + // REQUEST omitted the FK, so the remedy is to send it. + expect(err.message).toContain("did not supply 'account'"); + expect(err.message).not.toContain('requires edit access to its master record'); + }); + + // ── the split itself ───────────────────────────────────────────────────── + + it('the non-verdict legs are DISTINGUISHABLE from the verdicts and from each other', async () => { + const authorization = await refusalOf( + (await boot({ shareLevel: 'read' })).updateContact('ct_us'), + ); + const metadataDefect = await refusalOf( + (await boot({ shareLevel: 'edit', detail: 'no-master-detail' })).updateContact('ct_own'), + ); + const missingRow = await refusalOf( + (await boot({ shareLevel: 'edit' })).updateContact('ct_gone'), + ); + const nullMaster = await refusalOf( + (await boot({ + shareLevel: 'edit', + contacts: [{ id: 'ct_orphan', name: 'Orphan contact', account: null }], + })).updateContact('ct_orphan'), + ); + + // Four conditions, four envelopes. Before the split these were one: + // `403 PERMISSION_DENIED` with a single sentence, which is exactly what an + // assertion set that only checked "it threw" could not see. + const envelope = (e: any) => `${e.status ?? e.statusCode}/${e.code}`; + expect([authorization, metadataDefect, missingRow, nullMaster].map(envelope)).toEqual([ + '403/PERMISSION_DENIED', + '422/INVALID_METADATA', + '404/RECORD_NOT_FOUND', + '422/MISSING_REQUIRED_FIELD', + ]); + // The two 422s share a status and must still be told apart by `code` — + // which is the field ADR-0112 makes the branch point. + expect(metadataDefect.code).not.toBe(nullMaster.code); + }); +}); diff --git a/packages/plugins/plugin-security/src/errors.ts b/packages/plugins/plugin-security/src/errors.ts index 9a677a78cc..43947a6612 100644 --- a/packages/plugins/plugin-security/src/errors.ts +++ b/packages/plugins/plugin-security/src/errors.ts @@ -41,6 +41,145 @@ export class PermissionDeniedError extends Error { } } +/** + * ## The three NON-VERDICT legs of `assertControlledByParentWrite` (#7474) + * + * That gate used to funnel SIX distinct conditions through one `deny()` helper, + * so all six answered `403 PERMISSION_DENIED` with one sentence — "requires + * edit access to its master record". Three of them are genuine authorization + * verdicts (no object-level `update` on the master / the master row outside the + * caller's write RLS / no `edit`-level share grant) and keep that answer + * verbatim. The other three are not verdicts at all, and the sentence was a + * false statement carrying a false remedy: "ask whoever owns the parent record" + * cannot fix a null FK, a deleted row, or a broken `master_detail` declaration. + * + * Maintainer ruling of 2026-08-11 on #7474 split them by true semantics. The + * classes below are that split; the codes come from ADR-0112's closed + * vocabulary rather than new spellings of conditions the catalog already names. + * + * ### Why each carries BOTH `status` and `statusCode` + * + * The two transports read different property names, and this gate throws on the + * DATA path, which reaches both: `@objectstack/rest`'s `mapDataError` passes a + * domain error through on `.status` alone, while the runtime dispatcher's + * `errorFromThrown` reads `.status` then falls back to `.statusCode`. Declaring + * one spelling would leave the other transport deriving a status from nothing — + * which is the defect this split exists to remove, reintroduced at the edge. + * + * ### Why none of them starts with `[Security] Access denied` + * + * That exact prefix is a MATCHER, not a house style: `isPermissionDeniedError` + * below, `mapDataError`, and `rest-server`'s sanitiser all read it as "this is a + * 403". A configuration defect or a missing row phrased with that opening would + * be re-flattened to `403 PERMISSION_DENIED` at the transport and the split + * would be invisible on the wire — so each class opens with a prefix of its + * own, and that is load-bearing. + * + * ### Why the explanation lives in `message`, never in `details` + * + * `details` is not a reliable carrier (#7450 rules it off the dispatcher wire + * entirely, and `mapDataError`'s 4xx passthrough keeps only `error`, `code` and + * `object`), so every one of these messages has to stand on its own. Each is + * written for the APP AUTHOR — the audience the false 403 was hiding the defect + * from — and each names the object, the operation, and the remedy. + */ + +/** + * `sharingModel: 'controlled_by_parent'` on an object with no `master_detail` + * relation to derive access from → `422 INVALID_METADATA`. + * + * This is a precisely detectable AUTHORING defect, not an access verdict: the + * object declares that its access is derived from a master and then gives the + * platform no master to derive it from. Disguised as a routine permission + * denial it was the class of thing an author never sees, because 403s on a + * detail object read as ordinary RBAC noise — which is exactly the "declared = + * enforced, and say so loudly" principle inverted. + * + * `INVALID_METADATA` at 422 is the shape `@objectstack/metadata-protocol` + * already uses for "your metadata is broken" (its publish/validate refusals), + * reused here rather than spelled a second way. 422 also keeps the message on + * the wire: the 5xx band drops a producer's prose unconditionally at both + * transports, and prose is the entire remedy for this condition. + */ +export class MasterDetailRelationMissingError extends Error { + readonly code = 'INVALID_METADATA'; + readonly status = 422; + readonly statusCode = 422; + constructor(object: string, operation: string) { + super( + `[Security] Configuration defect: ${operation} on '${object}' cannot be authorized because the object ` + + `declares sharingModel 'controlled_by_parent' but has no master_detail relation to derive access from. ` + + `Declare a required master_detail field pointing at the master object, or change sharingModel.`, + ); + this.name = 'MasterDetailRelationMissingError'; + } +} + +/** + * The by-id write names a detail row that does not exist → `404 + * RECORD_NOT_FOUND`. + * + * A concurrent delete answered `403 "requires edit access to its master"` + * before this: an answer that leaks less but says something untrue, and one an + * SDK cannot retry or reconcile against. + * + * The 404 does NOT widen what a caller can learn. The row is read under a + * SYSTEM context (`readRowById(…, { isSystem: true })`), so a row hidden from + * the caller by read RLS is still FOUND here and falls to the authorization + * legs; and the object-level CRUD gate (middleware step 2) plus the row-level + * write pre-image check (step 2.7) both run BEFORE this gate — so a caller who + * reaches it already holds `update` on the detail. Absence here is real + * absence. + */ +export class DetailRecordNotFoundError extends Error { + readonly code = 'RECORD_NOT_FOUND'; + readonly status = 404; + readonly statusCode = 404; + readonly recordId: unknown; + constructor(object: string, operation: string, recordId: unknown) { + super( + `[Security] Record not found: ${operation} on '${object}' targets record '${String(recordId)}', ` + + `which does not exist.`, + ); + this.name = 'DetailRecordNotFoundError'; + this.recordId = recordId; + } +} + +/** + * The detail carries no value in its master reference, so there is no master to + * derive access from → `422 MISSING_REQUIRED_FIELD`. + * + * Two shapes of one condition, and the wording says which (#5240 — one + * condition, one wording): on `insert` the REQUEST omitted the master FK, which + * the caller fixes by sending it; on any other by-id write the STORED row's FK + * is null, which is a data-integrity defect the caller cannot fix by asking for + * permissions. `MISSING_REQUIRED_FIELD` is the standard catalog's name for both + * — a controlled_by_parent detail without its master reference is precisely a + * required value that is absent. + */ +export class MasterReferenceMissingError extends Error { + readonly code = 'MISSING_REQUIRED_FIELD'; + readonly status = 422; + readonly statusCode = 422; + readonly field: string; + readonly recordId: unknown; + constructor(object: string, operation: string, field: string, recordId?: unknown) { + super( + recordId == null + ? `[Security] Missing master reference: ${operation} on '${object}' did not supply '${field}'. ` + + `A controlled_by_parent detail derives its access from its master, so '${field}' must carry a ` + + `master record id on every write.` + : `[Security] Missing master reference: ${operation} on '${object}' cannot be authorized because ` + + `record '${String(recordId)}' has no value in '${field}'. A controlled_by_parent detail derives ` + + `its access from its master, so that reference must be populated.`, + ); + this.name = 'MasterReferenceMissingError'; + this.field = field; + this.recordId = recordId; + } +} + export function isPermissionDeniedError(e: unknown): e is PermissionDeniedError { if (!e || typeof e !== 'object') return false; const anyE = e as any; diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 0525f11224..2f3fab9f75 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -66,7 +66,12 @@ import { import { matchesFilterCondition } from '@objectstack/formula'; import { FieldMasker } from './field-masker.js'; import { assertReadableQueryFields } from './predicate-guard.js'; -import { PermissionDeniedError } from './errors.js'; +import { + PermissionDeniedError, + MasterDetailRelationMissingError, + DetailRecordNotFoundError, + MasterReferenceMissingError, +} from './errors.js'; import { assertEngineOwnedWriteAllowed, type EngineOwnedSchemaLike } from './system-write-guard.js'; import { bootstrapPlatformAdmin } from './bootstrap-platform-admin.js'; import { @@ -4051,6 +4056,22 @@ export class SecurityPlugin implements Plugin { * * v1 scope: single-id writes. Bulk writes flow through the AST and are already * scoped by the controlled-by-parent READ filter (to readable masters). + * + * [#7474] SIX conditions refuse a write here, and they are NOT one verdict. + * Three are genuine authorization answers (no object-level `update` on the + * master, the master row outside the caller's write RLS, no `edit`-level + * share grant) and answer `403 PERMISSION_DENIED` with the sentence above. + * Three are not answers about access at all, and used to borrow that same + * sentence — telling a caller they lacked access to a record when the truth + * was a broken declaration, a deleted row, or a null FK, with a remedy + * ("ask whoever owns the parent record") that could not fix any of them: + * + * - `controlled_by_parent` with no `master_detail` relation → `422 + * INVALID_METADATA` ({@link MasterDetailRelationMissingError}) + * - the target row does not exist → `404 RECORD_NOT_FOUND` + * ({@link DetailRecordNotFoundError}) + * - the detail's master reference is empty → `422 MISSING_REQUIRED_FIELD` + * ({@link MasterReferenceMissingError}) */ private async assertControlledByParentWrite( permissionSets: PermissionSet[], @@ -4063,7 +4084,14 @@ export class SecurityPlugin implements Plugin { const sharingModel = schema?.sharingModel ?? schema?.security?.sharingModel; if (sharingModel !== 'controlled_by_parent') return; - const deny = (reason: string, recordId?: unknown) => { + // [#7474] The AUTHORIZATION verdicts — and ONLY those. Three of this gate's + // conditions really do mean "you may not write this detail because you may + // not edit its master", and they keep `403 PERMISSION_DENIED` and this + // exact sentence. The other three conditions (broken master_detail + // declaration / missing row / null master FK) are not verdicts at all and + // throw their own errors below — see `./errors.ts` for the ruling and the + // reasoning behind each code. + const denyMasterEdit = (reason: string, recordId?: unknown): never => { throw new PermissionDeniedError( `[Security] Access denied: ${operation} on '${object}' requires edit access to its master record (${reason})`, { operation, object, recordId }, @@ -4071,46 +4099,53 @@ export class SecurityPlugin implements Plugin { }; const rel = this.resolveCbpRelation(object); - if (!rel) deny('controlled_by_parent declared but no master_detail relation'); + // A metadata defect, not an access verdict: the object declares that its + // access is derived from a master and gives us no master to derive it from. + // Thrown rather than routed through a `never`-returning helper so the + // narrowing is real — the non-null assertions this branch used to need + // (`rel!.fk`) were the load-bearing half of the same defect. + if (!rel) throw new MasterDetailRelationMissingError(object, operation); // Resolve the master id: from the incoming body on insert, else from the // target row (read as system — we only need its FK value). let masterId: unknown; + let detailRecordId: unknown; if (operation === 'insert') { const data = opCtx.data; - masterId = data && typeof data === 'object' && !Array.isArray(data) ? (data as any)[rel!.fk] : undefined; + masterId = data && typeof data === 'object' && !Array.isArray(data) ? (data as any)[rel.fk] : undefined; } else { const targetId = this.extractSingleId(opCtx); if (targetId == null) return; // bulk write — scoped by the read filter on the AST + detailRecordId = targetId; const row = await this.readRowById(object, targetId, { isSystem: true }); - if (!row) deny('target record not found', targetId); - masterId = row![rel!.fk]; + if (!row) throw new DetailRecordNotFoundError(object, operation, targetId); + masterId = row[rel.fk]; } - if (masterId == null) deny('detail record has no master reference'); + if (masterId == null) throw new MasterReferenceMissingError(object, operation, rel.fk, detailRecordId); // Master edit access = CRUD update on the master AND the master row reachable // under BOTH halves of its own write gate (write RLS + record sharing). - if (!this.permissionEvaluator.checkObjectPermission('update', rel!.master, permissionSets)) { - deny(`no edit permission on master '${rel!.master}'`, masterId); + if (!this.permissionEvaluator.checkObjectPermission('update', rel.master, permissionSets)) { + denyMasterEdit(`no edit permission on master '${rel.master}'`, masterId); } - const masterWriteFilter = await this.computeRlsFilter(permissionSets, rel!.master, 'update', context); + const masterWriteFilter = await this.computeRlsFilter(permissionSets, rel.master, 'update', context); if (masterWriteFilter) { let visible: unknown = null; try { - visible = await this.ql.findOne(rel!.master, { + visible = await this.ql.findOne(rel.master, { where: { $and: [{ id: masterId }, masterWriteFilter] }, context, }); } catch { visible = null; } - if (!visible) deny(`master '${rel!.master}' not editable by this user (row-level security)`, masterId); + if (!visible) denyMasterEdit(`master '${rel.master}' not editable by this user (row-level security)`, masterId); } // [#5386] The OWD / record-share half — asked UNCONDITIONALLY, because the // RLS half above is skipped whole when the master authors no write policy, // which is exactly the common case this closes. - if (!(await this.resolveSharingCanEdit(rel!.master, String(masterId), context, permissionSets))) { - deny(`master '${rel!.master}' not editable by this user (record sharing)`, masterId); + if (!(await this.resolveSharingCanEdit(rel.master, String(masterId), context, permissionSets))) { + denyMasterEdit(`master '${rel.master}' not editable by this user (record sharing)`, masterId); } }