diff --git a/.changeset/27472-owd-unset-external-hint.md b/.changeset/27472-owd-unset-external-hint.md new file mode 100644 index 00000000000..f2aa888da53 --- /dev/null +++ b/.changeset/27472-owd-unset-external-hint.md @@ -0,0 +1,17 @@ +--- +'@objectstack/lint': patch +--- + +fix(lint): `security-owd-unset`'s fix line on a writable external object names the ways out `security-external-owner-unmapped` names, instead of recommending `'private'` + +Clause-②: no +Metadata-protocol: none + +`security-owd-unset` refuses a custom object with no `sharingModel`, and its fix line recommended `sharingModel: 'private'` for every such object. On a writable external object (`external.writable: true`) whose ownership column no remote row serves, `'private'` is the shape `security-external-owner-unmapped` warns refuses every member edit: the author followed one rule's advice into the other's warning. + +- **Now, on a writable binding.** For exactly the objects `security-external-owner-unmapped` judges (`external.writable: true`, with no declared `owner_id`, or `created_by` on an object without `owner_id`), the fix line reads: declare `sharingModel: 'public_read_write'` and decide who may edit in a `beforeUpdate` hook, or declare the ownership column in `fields` and have the driver return the remote owner under that name, and declare `'private'` or `'public_read'`; `'controlled_by_parent'` for master-detail children. Both clauses are that rule's own wording, including its shorter one when an `external.columnMap` entry already targets the column. +- **Now, on a read-only binding.** An external object that no member writes (`external.writable` not `true`, or a datasource this stack declares with `external.allowWrites: false`) and whose `owner_id` no declared field or `external.columnMap` entry names is told to declare `sharingModel: 'public_read'`: on `'private'` a member reads none of its rows. +- **Unchanged.** The severity (`error`) and the verdict line. Every other custom object, and an external object whose owner is provable, keeps the `'private'` recommendation. +- `os explain security-owd-unset` explains both external cases and points at `os explain security-external-owner-unmapped`. + +The same fix line reaches the runtime publish gate's 422 issue for an `object` write. That gate sees no `datasources`, so a writable binding on an `allowWrites: false` datasource gets the writable-binding line there; neither door recommends `'private'` for it. diff --git a/packages/lint/src/rule-explanations.ts b/packages/lint/src/rule-explanations.ts index f202847c348..c4c88a2f6c3 100644 --- a/packages/lint/src/rule-explanations.ts +++ b/packages/lint/src/rule-explanations.ts @@ -114,6 +114,18 @@ const SECURITY_OWD_UNSET_EXPLANATION: RuleExplanation = { 'default); \'public_read\' — everyone reads, the owner writes; \'public_read_write\' — everyone ' + 'reads and writes; \'controlled_by_parent\' — access derives from the master record (a ' + 'master-detail child).', + 'An external object with no provable owner is the exception to that recommendation. On a writable ' + + 'binding (`external.writable: true`) the remote row carries no `owner_id` or `created_by` the platform can read, so on \'private\' every ' + + 'member edit is refused, and where the object carries `owner_id` a member reads no row at all ' + + '(`os explain security-external-owner-unmapped`). There the fix line names that rule\'s two ways out ' + + 'instead: \'public_read_write\' with a `beforeUpdate` hook that decides who may edit, or the ownership ' + + 'column (`owner_id`, or `created_by` on an object without one) declared in `fields` and returned by ' + + 'the driver, after which \'private\' or \'public_read\' admits edits by the user that column names. ' + + 'A read-only external binding (`external.writable` not `true`, or a datasource declared with ' + + '`external.allowWrites: false`) whose `owner_id` no declared field or `external.columnMap` entry names ' + + 'is told to declare \'public_read\' instead: no member writes it, and on \'private\' a member reads none ' + + 'of its rows. An external object whose owner is provable (a declared field, or on a read-only binding ' + + 'a `columnMap` entry onto `owner_id`) keeps the usual fix line.', 'System objects (`isSystem: true`, or a name starting with `sys_`) are not judged: the platform ' + 'owns their posture.', ], diff --git a/packages/lint/src/validate-external-owner-sharing.test.ts b/packages/lint/src/validate-external-owner-sharing.test.ts index fd188848dce..a36f2e8a788 100644 --- a/packages/lint/src/validate-external-owner-sharing.test.ts +++ b/packages/lint/src/validate-external-owner-sharing.test.ts @@ -4,9 +4,13 @@ import { describe, expect, it } from 'vitest'; import { defineStack } from '@objectstack/spec'; import { runAuthoringRules, splitBySeverity } from './authoring-rules.js'; import { + PUBLIC_READ_WRITE_WAY_OUT, + READ_ONLY_EXTERNAL_WAY_OUT, SECURITY_EXTERNAL_OWNER_UNMAPPED, + declareOwnershipColumnWayOut, validateExternalOwnerSharing, } from './validate-external-owner-sharing.js'; +import { SECURITY_OWD_UNSET, validateSecurityPosture } from './validate-security-posture.js'; /** * The two external objects of `packages/runtime/src/plugin-driver-external-object.spike.test.ts`, @@ -220,3 +224,88 @@ describe('security-external-owner-unmapped — the fix line', () => { } }); }); + +// [#27472] `security-owd-unset` refuses a custom object with no sharingModel, and its fix line used +// to recommend 'private' everywhere: on a writable external object that is the shape this rule warns +// refuses every member edit. Its fix line for the objects this rule judges is built from this +// rule's own two ways out; every other object keeps the 'private' recommendation. +describe("security-owd-unset's fix line on an external object", () => { + const owdUnsetHint = (stack: Record, where: string) => { + const findings = validateSecurityPosture(stack).filter((f) => f.rule === SECURITY_OWD_UNSET && f.where === where); + expect(findings, where).toHaveLength(1); + return findings[0]!.hint ?? ''; + }; + const OLD_HINT = + "declare sharingModel: 'private' (owner + shares; recommended), 'public_read', " + + "'public_read_write', or 'controlled_by_parent' (master-detail children)"; + + it("spike_owned gets this rule's ways out and no 'private' recommendation", () => { + const hint = owdUnsetHint(SPIKE_STACK, 'object "spike_owned"'); + expect(hint).toContain(PUBLIC_READ_WRITE_WAY_OUT); + expect(hint).toContain(declareOwnershipColumnWayOut('owner_id')); + expect(hint).not.toContain('recommended'); + expect(hint).not.toContain('\n'); + // Severity and verdict are the rule's own and do not move. + const [finding] = validateSecurityPosture(SPIKE_STACK).filter((f) => f.where === 'object "spike_owned"'); + expect(finding).toMatchObject({ severity: 'error', path: 'objects[1].sharingModel' }); + expect(finding!.message).toBe( + "custom object declares no sharingModel (OWD); the runtime falls back to 'private', " + + 'but the baseline must be an authored decision', + ); + }); + + it('a managed custom object keeps the private recommendation', () => { + expect(owdUnsetHint({ objects: [{ name: 'leave_request', label: 'Leave Request' }] }, 'object "leave_request"')).toBe( + OLD_HINT, + ); + }); + + it('names created_by on an external object without owner_id', () => { + const hint = owdUnsetHint({ objects: [ext({ ownership: 'none' })] }, 'object "remote_row"'); + expect(hint).toContain(declareOwnershipColumnWayOut('created_by')); + expect(hint).not.toContain('recommended'); + }); + + it('words the mapped case as this rule does: only the declaration is missing', () => { + const hint = owdUnsetHint({ objects: [ext({}, { columnMap: { assignee: 'owner_id' } })] }, 'object "remote_row"'); + expect(hint).toContain(declareOwnershipColumnWayOut('owner_id', true)); + expect(hint).toContain(PUBLIC_READ_WRITE_WAY_OUT); + expect(hint).not.toContain('recommended'); + }); + + // [#27425] On 'private', an external object whose owner_id is the platform's unprovable anchor + // reads no row for a member, whatever its writability: a read-only binding is told 'public_read'. + it("recommends 'public_read' on a read-only binding with an unprovable owner", () => { + const readOnlyDatasource = { name: 'remote', schemaMode: 'external', external: { allowWrites: false } }; + const { writable: _writable, ...noWritable } = ext().external; + for (const [label, stack] of [ + ['writable: false', { objects: [ext({}, { writable: false })] }], + ['writable unset', { objects: [{ ...ext(), external: noWritable }] }], + ['allowWrites: false', { datasources: [readOnlyDatasource], objects: [ext()] }], + ] as const) { + const hint = owdUnsetHint(stack, 'object "remote_row"'); + expect(hint, label).toBe(READ_ONLY_EXTERNAL_WAY_OUT); + expect(hint, label).not.toContain('recommended'); + } + }); + + it('keeps the private recommendation where the owner is provable', () => { + const ownerField = { ...NAME_FIELD, owner_id: { type: 'lookup', reference: 'sys_user', label: 'Owner' } }; + for (const [label, stack] of [ + ['declared owner_id, writable', { objects: [ext({ fields: ownerField })] }], + ['declared owner_id, read-only', { objects: [ext({ fields: ownerField }, { writable: false })] }], + ['columnMap onto owner_id, read-only', { objects: [ext({}, { writable: false, columnMap: { assignee: 'owner_id' } })] }], + ] as const) { + expect(owdUnsetHint(stack, 'object "remote_row"'), label).toBe(OLD_HINT); + } + }); + + it('either way out it names, once taken, clears this rule too', () => { + const ownerField = { ...NAME_FIELD, owner_id: { type: 'lookup', reference: 'sys_user', label: 'Owner' } }; + for (const obj of [ext({ sharingModel: 'public_read_write' }), ext({ sharingModel: 'private', fields: ownerField })]) { + const stack = { objects: [obj] }; + expect(validateSecurityPosture(stack).map((f) => f.rule)).toEqual([]); + expect(validateExternalOwnerSharing(stack)).toEqual([]); + } + }); +}); diff --git a/packages/lint/src/validate-external-owner-sharing.ts b/packages/lint/src/validate-external-owner-sharing.ts index 3fb0ab1680c..f0a7b40acc6 100644 --- a/packages/lint/src/validate-external-owner-sharing.ts +++ b/packages/lint/src/validate-external-owner-sharing.ts @@ -37,9 +37,11 @@ * column to `owner_id` changes nothing, and the driver must return the field * itself. So the author's vouch this rule reads is a DECLARED field — the same * provenance the runtime guards read (`resolveInjectedColumnProvenance`, - * `@objectstack/spec/data`) — never a `columnMap` entry alone. A declared - * `owner_id` also scopes reads by owner; the platform's own injected one does - * not (plugin-sharing treats it as unstored and leaves reads unscoped). + * `@objectstack/spec/data`) — never a `columnMap` entry alone. Reads follow + * the same anchor: a declared `owner_id` scopes them by owner, while the + * platform's own injected one, with no `columnMap` entry naming it, proves no + * owner, so on an owner-scoped model a member's read scope admits no row + * (plugin-sharing `hasPhantomOwnerAnchor`, #27425) and an edit answers 404. * * ## The two arms * @@ -99,9 +101,37 @@ type AnyRec = Record; const OWNER_COLUMN = 'owner_id'; const CREATOR_COLUMN = 'created_by'; -const PUBLIC_READ_WRITE_WAY_OUT = +/** + * The two ways out this rule names, as fix-line clauses. `security-owd-unset` + * words its fix line for the same objects from these, so an author following + * either rule's advice lands on a shape the other does not refuse. + */ +export const PUBLIC_READ_WRITE_WAY_OUT = "declare sharingModel: 'public_read_write' and decide who may edit in a beforeUpdate hook"; +/** + * The declaration way out, for the column {@link externalOwnershipGap} names. + * `mapped`: an `external.columnMap` entry already targets the column, so what + * is missing is only the declared field. + */ +export function declareOwnershipColumnWayOut(column: string, mapped = false): string { + const role = column === OWNER_COLUMN ? 'owner' : 'creator'; + if (mapped) return `declare ${column} in fields (a plugin driver must return the remote ${role} under that name itself)`; + return ( + `declare ${column} in fields and have the driver return the remote ${role} under that name ` + + '(a built-in SQL driver does it through external.columnMap; a plugin driver must return it itself)' + ); +} + +/** + * The fix-line clause `security-owd-unset` gives a read-only external binding + * whose owner anchor is unprovable ({@link hasUnprovableOwnerAnchor}): no + * member writes it, and on 'private' a member reads none of its rows. + */ +export const READ_ONLY_EXTERNAL_WAY_OUT = + "declare sharingModel: 'public_read' (no member writes a read-only binding, and on 'private' a member " + + 'reads no row: no declared field or external.columnMap entry names the remote owner_id)'; + /** Does `external.columnMap` (remote column → local field) map some remote column to `field`? */ function columnMapTargets(external: AnyRec, field: string): boolean { const map = external.columnMap; @@ -129,14 +159,13 @@ function undeclaredOwnershipColumn(obj: AnyRec): { column: string; injected: boo * declares no ownership column a remote row serves. Returns the findings * (empty = clean); the CLI folds them in as advisory warnings. */ -export function validateExternalOwnerSharing(stack: AnyRec): ExternalOwnerSharingFinding[] { - const out: ExternalOwnerSharingFinding[] = []; - if (!stack || typeof stack !== 'object') return out; - const objects = recordsOf(stack.objects); - // ADR-0015's datasource half of the double write opt-in: a datasource this - // stack declares with `external.allowWrites: false` (also the parsed - // default) lets no write through, so no ownership check is what refuses. - const readOnlyDatasources = new Set( +/** + * ADR-0015's datasource half of the double write opt-in: the datasources this + * stack declares with `external.allowWrites: false` (also the parsed default) + * let no write through, so no ownership check is what refuses. + */ +export function readOnlyExternalDatasources(stack: AnyRec): ReadonlySet { + return new Set( recordsOf(stack.datasources) .filter((ds) => { const ext = ds.external; @@ -144,25 +173,61 @@ export function validateExternalOwnerSharing(stack: AnyRec): ExternalOwnerSharin }) .map((ds) => ds.name), ); +} + +/** + * The ownership column whose missing storage refuses every member write on + * this object under an owner-keyed sharing model — or `undefined` when the + * object is not a writable external binding, sits on a datasource that lets + * no write through, or declares the column the check reads. `mapped`: an + * `external.columnMap` entry targets the column, so only the declaration is + * missing. The sharing model itself is the caller's to judge. + */ +export function externalOwnershipGap( + obj: AnyRec, + readOnlyDatasources: ReadonlySet, +): { column: string; injected: boolean; mapped: boolean } | undefined { + const external = obj.external; + if (!external || typeof external !== 'object' || Array.isArray(external)) return undefined; + if ((external as AnyRec).writable !== true) return undefined; + if (readOnlyDatasources.has(obj.datasource)) return undefined; + const undeclared = undeclaredOwnershipColumn(obj); + if (!undeclared) return undefined; + return { ...undeclared, mapped: columnMapTargets(external as AnyRec, undeclared.column) }; +} + +/** + * [#27425] The lint's reading of plugin-sharing's `hasPhantomOwnerAnchor`: an + * external object whose `owner_id` is the platform's injected anchor (no + * declared field) and no `external.columnMap` entry targets it. No owner is + * provable for such an object, so record sharing admits no row by ownership on + * an owner-scoped model, whatever the binding's writability. + */ +export function hasUnprovableOwnerAnchor(obj: AnyRec): boolean { + const external = obj.external; + if (!external || typeof external !== 'object' || Array.isArray(external)) return false; + if (columnMapTargets(external as AnyRec, OWNER_COLUMN)) return false; + return resolveInjectedColumnProvenance(obj, OWNER_COLUMN) === 'injected-unprovisioned'; +} + +export function validateExternalOwnerSharing(stack: AnyRec): ExternalOwnerSharingFinding[] { + const out: ExternalOwnerSharingFinding[] = []; + if (!stack || typeof stack !== 'object') return out; + const objects = recordsOf(stack.objects); + const readOnlyDatasources = readOnlyExternalDatasources(stack); objects.forEach((obj, i) => { - const external = obj.external; - if (!external || typeof external !== 'object' || Array.isArray(external)) return; - if ((external as AnyRec).writable !== true) return; - if (readOnlyDatasources.has(obj.datasource)) return; if (effectiveSharingModelOf(obj) === 'public') return; - const undeclared = undeclaredOwnershipColumn(obj); + const undeclared = externalOwnershipGap(obj, readOnlyDatasources); if (!undeclared) return; - const { column } = undeclared; + const { column, mapped } = undeclared; const name = typeof obj.name === 'string' ? obj.name : `(object ${i})`; const model = typeof obj.sharingModel === 'string' ? `'${obj.sharingModel}'` : "'private' (the default)"; const check = column === OWNER_COLUMN ? 'record sharing reads owner_id' : 'with no owner_id, the ownership floor reads created_by == the caller'; - const mapped = columnMapTargets(external as AnyRec, column); - const role = column === OWNER_COLUMN ? 'owner' : 'creator'; // [#27425] On `private`, record sharing also scopes READS by owner_id, and // an owner it cannot read admits no row: the edit's pre-image read finds // none, so the refusal is a 404 rather than the write check's 403. @@ -170,9 +235,7 @@ export function validateExternalOwnerSharing(stack: AnyRec): ExternalOwnerSharin column === OWNER_COLUMN && effectiveSharingModelOf(obj) === 'private' ? "(404: the member's read scope admits no row)" : '(403)'; - const declareWayOut = - `declare ${column} in fields and have the driver return the remote ${role} under that name ` + - '(a built-in SQL driver does it through external.columnMap; a plugin driver must return it itself)'; + const declareWayOut = declareOwnershipColumnWayOut(column, mapped); out.push({ severity: 'warning', @@ -191,8 +254,7 @@ export function validateExternalOwnerSharing(stack: AnyRec): ExternalOwnerSharin `${check} before every edit and delete, so every member write is refused ${refusal} unless an app-authored ` + 'row-level policy admits the row', hint: mapped - ? `Declare ${column} in fields (a plugin driver must return the remote ${role} under that name itself), ` + - `or ${PUBLIC_READ_WRITE_WAY_OUT}` + ? `D${declareWayOut.slice(1)}, or ${PUBLIC_READ_WRITE_WAY_OUT}` : `Either ${PUBLIC_READ_WRITE_WAY_OUT}, or ${declareWayOut}`, }); }); diff --git a/packages/lint/src/validate-security-posture.test.ts b/packages/lint/src/validate-security-posture.test.ts index eb573fa4809..f3aa680bc2b 100644 --- a/packages/lint/src/validate-security-posture.test.ts +++ b/packages/lint/src/validate-security-posture.test.ts @@ -145,6 +145,10 @@ describe('validateSecurityPosture (ADR-0090 D7)', () => { const text = explanation!.paragraphs.join('\n'); expect(text).toContain('ADR-0090 D1'); expect(text).toContain("read and edit every other user's records"); + // [#27472] The writable-external exception to the 'private' recommendation, and the rule it defers to. + expect(text).toContain('external.writable: true'); + expect(text).toContain('os explain security-external-owner-unmapped'); + expect(text).toContain("is told to declare 'public_read' instead"); expect(finding.message).not.toContain('incident'); }); @@ -1792,6 +1796,8 @@ describe('validateSecurityPosture — reads only keys the spec declares (meta-te 'cbpDetails', // #26591: the resolving details by name — JS Map methods. 'policies', // one set's `rowLevelSecurity` records — `.length`, a JS property. 'predicate', // a policy's `using` / `check` text, a string — `.trim`. + // #27472: `externalOwnershipGap`'s answer — `.column`, a field of that helper's own return value. + 'ownershipGap', ]); expect(receivers.filter((r) => !tabled.has(r) && !PLUMBING.has(r))).toEqual([]); }); diff --git a/packages/lint/src/validate-security-posture.ts b/packages/lint/src/validate-security-posture.ts index 7122b8ad13d..b5bf9506e68 100644 --- a/packages/lint/src/validate-security-posture.ts +++ b/packages/lint/src/validate-security-posture.ts @@ -125,6 +125,14 @@ import { controlledByParentOwnerMayEdit, isControlledByParentMasterField, refere import { describeAnchorForbiddenBits, type AnchorBindingContext } from '@objectstack/spec/security'; import { indexObjectGraph, nearestName, packagesOf, recordsOf, type ObjectGraph } from './object-graph.js'; import { injectedColumnsFor } from './system-fields.js'; +import { + PUBLIC_READ_WRITE_WAY_OUT, + READ_ONLY_EXTERNAL_WAY_OUT, + declareOwnershipColumnWayOut, + externalOwnershipGap, + hasUnprovableOwnerAnchor, + readOnlyExternalDatasources, +} from './validate-external-owner-sharing.js'; export const SECURITY_OWD_UNSET = 'security-owd-unset'; export const SECURITY_OWD_ALIAS = 'security-owd-alias'; @@ -794,6 +802,7 @@ export function validateSecurityPosture(stack: AnyRec, opts?: { nowMs?: number } declaredCapabilities.length > 0 ? { declaredCapabilities } : undefined; // ── D1/D4: per-object OWD posture ─────────────────────────────────── + const readOnlyDatasources = readOnlyExternalDatasources(stack); for (let i = 0; i < objects.length; i++) { const obj = objects[i]; if (!obj || typeof obj !== 'object') continue; @@ -803,6 +812,18 @@ export function validateSecurityPosture(stack: AnyRec, opts?: { nowMs?: number } if (!isSystemObject(obj)) { if (owd == null) { + // [#27472] 'private' is never recommended where it refuses the member: + // - a writable external binding whose ownership column no remote row + // serves gets `security-external-owner-unmapped`'s two ways out; + // - any other external object whose owner anchor is unprovable (a + // read-only binding, or one on a datasource that lets no write + // through) gets 'public_read': on 'private' a member reads no row + // (#27425), whatever the binding's writability. The publish gate + // sees no datasources, so there an allowWrites: false datasource + // reads as writable; both doors still never say 'private'. + // Every other object, an external one that declares its owner + // included, keeps the 'private' recommendation. + const ownershipGap = externalOwnershipGap(obj, readOnlyDatasources); findings.push({ severity: 'error', rule: SECURITY_OWD_UNSET, @@ -815,9 +836,14 @@ export function validateSecurityPosture(stack: AnyRec, opts?: { nowMs?: number } message: `custom object declares no sharingModel (OWD); the runtime falls back to 'private', ` + `but the baseline must be an authored decision`, - hint: - `declare sharingModel: 'private' (owner + shares; recommended), 'public_read', ` + - `'public_read_write', or 'controlled_by_parent' (master-detail children)`, + hint: ownershipGap + ? `${PUBLIC_READ_WRITE_WAY_OUT}, or ` + + `${declareOwnershipColumnWayOut(ownershipGap.column, ownershipGap.mapped)} ` + + `and declare sharingModel: 'private' or 'public_read'; 'controlled_by_parent' for master-detail children` + : hasUnprovableOwnerAnchor(obj) + ? READ_ONLY_EXTERNAL_WAY_OUT + : `declare sharingModel: 'private' (owner + shares; recommended), 'public_read', ` + + `'public_read_write', or 'controlled_by_parent' (master-detail children)`, }); } else if (typeof owd === 'string' && OWD_ALIAS_FIX[owd]) { // Reachable ONLY through the unparsed doors (`os lint` on a raw