diff --git a/.changeset/readonly-hook-api-write-lint.md b/.changeset/readonly-hook-api-write-lint.md new file mode 100644 index 0000000000..543d857e79 --- /dev/null +++ b/.changeset/readonly-hook-api-write-lint.md @@ -0,0 +1,14 @@ +--- +'@objectstack/lint': minor +--- + +Add `validateReadonlyHookWrites` — an author-time gate on a hook body writing a `readonly` field through `ctx.api`. + +A hook's `ctx.api` is a `ScopedContext` over the **triggering** operation's execution context, so `ctx.api.object('x').update({ someReadonlyField })` reaches the engine as an ordinary non-system caller and the update path strips the key. The call returns success, the step looks clean, and the column is simply always null — a failure only an end-to-end read-back detects. This completes the hook side of the flow-side gate that shipped as `flow-update-readonly-field`. + +Two new rule ids, wired through `REFERENCE_INTEGRITY_RULES` so they run on `os validate`, `os lint` and `os compile`: + +- `hook-api-update-readonly-field` — **error**. A literal `ctx.api.object('…').update()` / `.updateById()` writing a field the named object declares `readonly: true`. +- `hook-api-update-readonly-when-field` — **warning**. The same write against a `readonlyWhen` field, which strips per record state. + +The rule keys on the write **channel**, not on the field, so the correct and widely used pairing is untouched: a `beforeInsert`/`beforeUpdate` body stamping `ctx.input. = …` writes a server value that survives the strip and is **never** flagged. Also skipped, each for a stated reason: `ctx.api.sudo()` chains (elevated — the intended channel), `insert`/`create` (INSERT is engine-exempt), dynamic object names, non-literal payloads, objects this stack does not declare, fields the object does not declare, and `id` in an `update` payload (the row address, not a field write). diff --git a/content/docs/automation/hook-bodies.mdx b/content/docs/automation/hook-bodies.mdx index 01a7af2b6b..368b0d3a55 100644 --- a/content/docs/automation/hook-bodies.mdx +++ b/content/docs/automation/hook-bodies.mdx @@ -175,7 +175,7 @@ Static validation around a hook is asymmetric, and it is worth knowing exactly w - **Checked — read side.** `hook.condition` is validated at build time against the target object's fields by the expression validator (`@objectstack/lint`), including array-valued `hook.object` targets. A condition referencing a nonexistent field fails the lint. - **Checked — capability side.** `body.capabilities` gates which `ctx` APIs the body may call at all; the sandbox throws on an undeclared call. -- **Checked — write side, advisory and literal-only.** Since [#4271](https://github.com/objectstack-ai/objectstack/issues/4271), `body.source` is **parsed** (never executed, never type-checked) and the field names it writes are resolved against the target object's declarations. An unknown field raises `hook-body-write-unknown-field` — a **warning** carrying a did-you-mean suggestion, which never blocks a build. Action bodies get the same check on their `ctx.api` writes (`action-body-write-unknown-field`). Both run under `os validate`, `os lint` and `os compile`. +- **Checked — write side, literal-only.** Since [#4271](https://github.com/objectstack-ai/objectstack/issues/4271), `body.source` is **parsed** (never executed, never type-checked) and the field names it writes are resolved against the target object's declarations. An unknown field raises `hook-body-write-unknown-field` — a **warning** carrying a did-you-mean suggestion, which never blocks a build. Action bodies get the same check on their `ctx.api` writes (`action-body-write-unknown-field`). Both run under `os validate`, `os lint` and `os compile`. The *existence* question is advisory like this because the answer can depend on a package the build cannot see; the separate *writability* question — [writing a `readonly` field through `ctx.api`](#writing-a-readonly-field) — **does** gate, because both halves of that judgement are declared in the stack being checked. - **Checked — writes to a system column the object has no storage for.** Since [#8663](https://github.com/objectstack-ai/objectstack/issues/8663), a write to an injected system column is no longer exempted on the strength of its NAME alone. The registry injects `owner_id` / `organization_id` / the audit family onto an ADR-0015 [`external` object](/docs/data-modeling/external-datasources) exactly as onto a local one, but the remote database owns that schema and no column exists behind them. Writing one raises `hook-body-write-unprovisioned-anchor` (or `action-body-write-unprovisioned-anchor` / `flow-node-write-unprovisioned-anchor` on the other two surfaces) — a **warning** on all three, including the flow-node rule that otherwise gates, because the claim is about a remote schema the build cannot see. A column you **declare** yourself is untouched: on a federated object a declared `owner_id` maps a remote column you vouch for. Why it matters more than an ordinary typo: an undeclared name is refused upstream by the engine's own write-path validator (`INVALID_FIELD`), whereas the injected anchor is in the registered schema and passes it — so it is the one payload key that reaches the remote database raw, where a SQL remote aborts the **whole statement** with an untyped `no such column` and takes the correctly named fields of the same payload with it. - **Checked — writes that reach nothing at all.** Since [#4345](https://github.com/objectstack-ai/objectstack/issues/4345), an action body assigning to `ctx.record` raises `action-record-write-discarded`, also a warning. This one is **not** a field-resolution question: an action's `ctx.record` is a snapshot the runtime never writes back, so the assignment is discarded whether or not the field is declared — see [Signature conventions](#signature-conventions) below. @@ -211,11 +211,11 @@ An unknown field is **not** caught at runtime, and it does not fail quietly eith Neither outcome is the one you wanted, and the advisory warning is the earliest signal you get. -Because the checking is advisory and literal-only: +Because the existence check is advisory, and every write-side check here is literal-only: - **Treat `hook-body-write-unknown-field` as a build failure by convention.** It does not gate, but the rule is tuned for near-zero false positives — in practice a warning is a real typo. - **Check by hand what the parser cannot see.** Computed keys, spreads, aliased input and dynamic object names are invisible to the rule; for an array or `"*"` hook, every field must exist on every target. -- **Prefer a flow `update_record` node when the write set is fixed — and for *this* check most of all.** A flow node's writes are structured config: they diff field-by-field, render in the Console designer, and a write to a `readonly:true` field is a **gating error** (`flow-update-readonly-field`) that hooks have no counterpart for. Since [#4271](https://github.com/objectstack-ai/objectstack/issues/4271) the field-existence check gates there too — `flow-node-write-unknown-field` is an **error**, not the advisory warning a body gets, because a node's `fields` is a literal map next to a literal `objectName`: there is no parser in between that could have mis-extracted it, so a finding is a certainty rather than a best effort. +- **Prefer a flow `update_record` node when the write set is fixed — and for *this* check most of all.** A flow node's writes are structured config: they diff field-by-field, render in the Console designer, and since [#4271](https://github.com/objectstack-ai/objectstack/issues/4271) the field-existence check gates there too — `flow-node-write-unknown-field` is an **error**, not the advisory warning a body gets, because a node's `fields` is a literal map next to a literal `objectName`: there is no parser in between that could have mis-extracted it, so a finding is a certainty rather than a best effort. (The *writability* check now has a hook-side counterpart — see [Writing a `readonly` field](#writing-a-readonly-field) below — but it covers only the `ctx.api` channel.) - **Exercise the hook against a real object before shipping** — on SQL drivers the mistake surfaces on the first write; schemaless drivers won't tell you. ### Signature conventions @@ -245,6 +245,26 @@ Per-invocation budgets default to **250ms** (hooks) / **5000ms** (actions) of ** A body may write *other* objects — e.g. `await ctx.api.object('parent').update({ ... })` from a child's `afterInsert`/`afterUpdate` (requires `api.write`). The target's own hooks fire too: the nested write runs in a **fresh sandbox VM** while the calling body is suspended, and this composes to any depth. This is the natural "when a child changes, roll the total up to the parent" automation — it does **not** need a denormalized, hand-maintained mirror field. Because each body's budget is **CPU time** (ADR-0102), the caller is **not** charged for the nested write's own run — so the stock 250ms default comfortably covers deep rollup chains, and you rarely need to raise `timeoutMs` (the spec still permits up to 30_000ms for a genuinely CPU-heavy body). +### Writing a `readonly` field + +There is an asymmetry here that costs data if you learn it the hard way, so learn it here. A field declared `readonly: true` can still be **maintained by automation** — but only through two channels, and a nested `ctx.api` write is **not** one of them. + +`readonly` governs the *caller* surface. On UPDATE the engine strips read-only keys from the payload, but only the ones the **caller supplied** and only when the value is still the caller's. So: + +| How the body writes it | What happens | +|:---|:---| +| `ctx.input. = …` in `beforeInsert`/`beforeUpdate` | **Lands.** The stamp is a *server* value, not a caller-supplied one, so the strip leaves it alone. This is the recommended shape. | +| `ctx.api.object('x').update({ })` | **Silently dropped.** `ctx.api` is scoped to the *triggering* operation's context, so on any non-system trigger the payload is an ordinary caller payload and the key is stripped. The call still returns success. | +| `ctx.api.sudo().object('x').update({ })` | **Lands.** `sudo()` elevates to a system context, which the strip skips — the hook-side analogue of a flow's `runAs: 'system'`. Use it deliberately: it also bypasses the acting user's row and field permissions for that write. | +| `ctx.api.object('x').insert({ })` | **Lands.** INSERT is exempt — a create may legitimately seed read-only columns. | + +The dropped case is the dangerous one: nothing fails, the step reports success, and the column is simply always null. Because both halves of that judgement are declared in your own stack, it is checked at author time and **gates the build**: + +- `hook-api-update-readonly-field` — **error**. A body's literal `ctx.api.object('…').update()` / `.updateById()` writes a field the named object declares `readonly: true`. +- `hook-api-update-readonly-when-field` — **warning**. The same write against a `readonlyWhen` field, which strips per record *state*. Note that `readonlyWhen` also strips a `beforeUpdate`-derived value, so the own-hook stamp is **not** a workaround for it — `sudo()` is. + +Only literal object names and literal payload keys are seen; a `sudo()` chain, a dynamic object name, an object this stack does not declare, and `insert`/`create` are all skipped, so the rule has no opinion on them. The flow surface has carried the same gate as `flow-update-readonly-field` since [#3425](https://github.com/objectstack-ai/objectstack/issues/3425). + ### Errors from `ctx.api` A rejected `ctx.api` call gives your body the host error's `name` and `message`, plus two structured properties when the host supplied them: diff --git a/content/docs/automation/hooks.mdx b/content/docs/automation/hooks.mdx index 20f588800f..e9c76f88ff 100644 --- a/content/docs/automation/hooks.mdx +++ b/content/docs/automation/hooks.mdx @@ -35,12 +35,19 @@ Two structural reasons to prefer the flow when either could work: `update_record` node's `fields` is structural config that `os validate` checks — readonly targets, template dialects, declared expression slots. A hook body's write set is checked too, but only for the literal patterns a - parser can recognise and only as an advisory warning (see + parser can recognise (see [Hook & Action Bodies](/docs/automation/hook-bodies#write-set-checking)). Field existence is checked on both surfaces, at different strengths: a hook body gets a warning, an `update_record` node a **gating error** (`flow-node-write-unknown-field`) — its `fields` is a literal map next to a - literal `objectName`, so nothing could have mis-read it. + literal `objectName`, so nothing could have mis-read it. **Writability** is + now gated on both: writing a `readonly` field through `ctx.api` is + `hook-api-update-readonly-field`, the hook-side sibling of the flow node's + `flow-update-readonly-field` (see + [Writing a `readonly` field](/docs/automation/hook-bodies#writing-a-readonly-field)) — + the two questions differ because a `readonly` declaration and a literal + `ctx.api` update are both visible in the stack, while a field's existence may + depend on a package the build cannot see. - **A flow reviews as data.** The node graph diffs field-by-field and renders in the Console designer with per-node run history; a hook body reviews as code only. diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index 25924115b2..d20fa78693 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -124,6 +124,18 @@ export type { ReadonlyFlowWriteSeverity, } from './validate-readonly-flow-writes.js'; +export { + validateReadonlyHookWrites, + HOOK_API_UPDATE_READONLY_FIELD, + HOOK_API_UPDATE_READONLY_WHEN_FIELD, + READONLY_HOOK_WRITE_PATTERN_IDS, + READONLY_HOOK_WRITE_EXCLUSIONS, +} from './validate-readonly-hook-writes.js'; +export type { + ReadonlyHookWriteFinding, + ReadonlyHookWriteSeverity, +} from './validate-readonly-hook-writes.js'; + export { validateViewContainers, VIEW_CONTAINER_SHAPE } from './validate-view-containers.js'; export type { ViewContainerFinding, ViewContainerSeverity } from './validate-view-containers.js'; diff --git a/packages/lint/src/reference-integrity-suite.test.ts b/packages/lint/src/reference-integrity-suite.test.ts index 90c92dfdb0..0d0c75ab88 100644 --- a/packages/lint/src/reference-integrity-suite.test.ts +++ b/packages/lint/src/reference-integrity-suite.test.ts @@ -38,6 +38,10 @@ describe('reference-integrity suite — membership', () => { 'validateActionBodyWrites', 'validateFlowNodeWrites', 'validateReadonlyFlowWrites', + // [#13653] The hook-side half of the readonly write judgement: a body's + // `ctx.api` update to a declared-`readonly` field, placed beside the flow + // twin that asks the identical question one surface over. + 'validateReadonlyHookWrites', 'validateReactPageProps', ]); }); @@ -214,6 +218,22 @@ describe('reference-integrity suite — every member actually runs', () => { events: ['beforeInsert'], body: { language: 'js', source: "ctx.input.lead_score = 100;" }, }, + // validateReadonlyHookWrites (#13653): `locked` EXISTS on crm_lead and is + // static-`readonly`, so this is not an existence question — the engine + // strips the key from the ctx.api UPDATE payload on every non-system + // trigger and the call still returns success. A separate hook from + // `score_lead` on purpose, mirroring the `stamp`/`lock` flow-node split + // below: one body carrying both defects would let either rule go silent + // behind the other's finding. + { + name: 'lock_lead', + object: 'crm_lead', + events: ['afterUpdate'], + body: { + language: 'js', + source: "await ctx.api.object('crm_lead').update({ id: ctx.recordId, locked: true });", + }, + }, ], flows: [ { @@ -287,6 +307,7 @@ describe('reference-integrity suite — every member actually runs', () => { expect(rules).toContain('action-record-write-discarded'); expect(rules).toContain('flow-node-write-unknown-field'); expect(rules).toContain('flow-update-readonly-field'); + expect(rules).toContain('hook-api-update-readonly-field'); expect(rules).toContain('react-prop-missing-required'); }); diff --git a/packages/lint/src/reference-integrity-suite.ts b/packages/lint/src/reference-integrity-suite.ts index 0ea6ab732e..fe8ec8aed7 100644 --- a/packages/lint/src/reference-integrity-suite.ts +++ b/packages/lint/src/reference-integrity-suite.ts @@ -96,6 +96,7 @@ import { validateHookBodyWrites } from './validate-hook-body-writes.js'; import { validateActionBodyWrites } from './validate-action-body-writes.js'; import { validateFlowNodeWrites } from './validate-flow-node-writes.js'; import { validateReadonlyFlowWrites } from './validate-readonly-flow-writes.js'; +import { validateReadonlyHookWrites } from './validate-readonly-hook-writes.js'; import { validateReactPageProps } from './validate-react-page-props.js'; export type ReferenceIntegritySeverity = 'error' | 'warning'; @@ -275,6 +276,22 @@ export const REFERENCE_INTEGRITY_RULES: readonly ReferenceIntegrityRule[] = [ // build the other command would have stopped. Joining the suite is the whole // fix; the two hand-wired call sites are deleted with it (#4345 follow-up). { name: 'validateReadonlyFlowWrites', run: validateReadonlyFlowWrites }, + // [#13653] The SAME question as the member above, on the surface that had no + // answer for it: a hook body's `ctx.api.object('x').update({ readonlyField })`. + // A hook's `ctx.api` is a ScopedContext over the TRIGGERING operation's + // context, so on a non-system trigger the engine strips the key and the call + // still returns success — the flow rule's silent no-op, reached through JS + // instead of through `config.fields`. + // + // It gates for the flow member's reason and NOT for its neighbour + // `validateHookBodyWrites`': both halves of the judgement are declared in + // THIS stack (the field's `readonly`, the body's literal `ctx.api` update), + // so the finding does not depend on a package the build cannot see. What it + // must never touch is the `ctx.input` stamp — a before-hook writing a + // `readonly` field is CORRECT and widely used, because the strip drops only + // caller-supplied values (#5591) — so the rule keys on the write CHANNEL, + // and both directions are pinned in its tests. + { name: 'validateReadonlyHookWrites', run: validateReadonlyHookWrites }, // The `kind:'react'` page surface. Every prop a react block binds BY FIELD // NAME is resolved against the object it names (#4340) — ``, // ``, `` through the SAME diff --git a/packages/lint/src/validate-readonly-flow-writes.ts b/packages/lint/src/validate-readonly-flow-writes.ts index ef7334cfa1..6c192583b2 100644 --- a/packages/lint/src/validate-readonly-flow-writes.ts +++ b/packages/lint/src/validate-readonly-flow-writes.ts @@ -65,7 +65,7 @@ function asArray(v: unknown): AnyRec[] { return []; } -interface FieldReadonlyMeta { +export interface FieldReadonlyMeta { /** Static `readonly: true`. */ readonly: boolean; /** A non-empty `readonlyWhen` predicate is declared. */ @@ -77,8 +77,17 @@ interface FieldReadonlyMeta { * (array of `{name, readonly, readonlyWhen}` and name-keyed map). A field with * neither flag is recorded as `{false, false}` so callers can distinguish a * "known-writable field" from an "unknown field" (absent from the map). + * + * Exported for `validate-readonly-hook-writes.ts` (#13653), which asks the + * IDENTICAL question one surface over — "is this declared field writable + * through this channel?" — about a hook body's `ctx.api` update instead of a + * flow node's `config.fields`. Shared rather than copied for the reason #4330 + * collapsed five hand-copied lists: two readings of `readonly`/`readonlyWhen` + * that drift produce two rules that disagree about the same field, and the + * disagreement is silent. `IMPLICIT_FIELDS` in `validate-hook-body-writes.ts` + * is shared across its three surfaces on exactly this reasoning. */ -function buildReadonlyIndex(objects: AnyRec[]): Map> { +export function buildReadonlyIndex(objects: AnyRec[]): Map> { const idx = new Map>(); for (const obj of objects) { const name = typeof obj.name === 'string' ? obj.name : undefined; diff --git a/packages/lint/src/validate-readonly-hook-writes.test.ts b/packages/lint/src/validate-readonly-hook-writes.test.ts new file mode 100644 index 0000000000..2fd2205b6c --- /dev/null +++ b/packages/lint/src/validate-readonly-hook-writes.test.ts @@ -0,0 +1,428 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// Both directions of the #13653 judgement, pinned together on purpose. +// +// The rule's whole risk is that it over-fires. `readonly` + a `beforeInsert`/ +// `beforeUpdate` stamp is a CORRECT and widely used pairing (the strip drops +// only caller-supplied values, #5591), so a naive "readonly field appears in a +// write set" rule would fail the correct shape on day one and be switched off +// by the first author who met it. Every RED case below therefore has a GREEN +// twin that differs ONLY in the write channel. +import { describe, expect, it } from 'vitest'; + +import { HOOK_BODY_WRITE_PATTERNS } from './validate-hook-body-writes.js'; +import { + validateReadonlyHookWrites, + HOOK_API_UPDATE_READONLY_FIELD, + HOOK_API_UPDATE_READONLY_WHEN_FIELD, + READONLY_HOOK_WRITE_PATTERN_IDS, + READONLY_HOOK_WRITE_EXCLUSIONS, +} from './validate-readonly-hook-writes.js'; + +/** + * A stack shaped like the reference app's motivating case (#13653): a derived + * column another object's hook maintains. `last_activity_date` is the field + * whose `readonly` flag made the churn report count every account as silent. + * + * The six motivating fields live in `objectstack-ai/hotcrm`, which this repo's + * CI cannot reach, so the shape is reproduced here rather than referenced. + */ +const crmStack = (source: string, opts: { hookObject?: string } = {}) => ({ + objects: [ + { + name: 'crm_account', + fields: { + name: { type: 'text', label: 'Name' }, + // The outage field: automation must maintain it, users must not edit it. + last_activity_date: { type: 'datetime', label: 'Last activity', readonly: true }, + // The CORRECT pairing from the same app: `readonly` + an own-hook stamp. + name_normalized: { type: 'text', label: 'Normalized', readonly: true }, + // Writable by anyone - the control that proves the rule keys on the + // declaration and not merely on the channel. + notes: { type: 'text', label: 'Notes' }, + // Conditionally locked - a different shape with a different verdict. + credit_hold: { type: 'boolean', label: 'Credit hold', readonlyWhen: 'status == "closed"' }, + }, + }, + { + name: 'crm_case', + fields: { + subject: { type: 'text', label: 'Subject' }, + first_response_date: { type: 'datetime', label: 'First response', readonly: true }, + }, + }, + ], + hooks: [ + { + name: 'touch_account', + object: opts.hookObject ?? 'crm_case', + events: ['afterInsert'], + body: { language: 'js', source }, + }, + ], +}); + +describe('validateReadonlyHookWrites - RED: a ctx.api write to a readonly field', () => { + it('flags ctx.api.object(...).update() writing a static-readonly field', () => { + const findings = validateReadonlyHookWrites( + crmStack("await ctx.api.object('crm_account').update({ id: accountId, last_activity_date: now });"), + ); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(HOOK_API_UPDATE_READONLY_FIELD); + expect(findings[0].severity).toBe('error'); + expect(findings[0].where).toBe('hook "touch_account" > body'); + expect(findings[0].path).toBe('hooks[0].body.source'); + expect(findings[0].message).toContain("'last_activity_date'"); + expect(findings[0].message).toContain('crm_account'); + // The remedy must name BOTH legitimate channels, not only sudo - telling an + // author to elevate is a security-relevant instruction, and the own-hook + // stamp is the shape that needs no elevation at all. + expect(findings[0].hint).toContain('ctx.input.last_activity_date'); + expect(findings[0].hint).toContain('sudo'); + }); + + it('flags updateById, whose payload is argument 1', () => { + const findings = validateReadonlyHookWrites( + crmStack("await ctx.api.object('crm_account').updateById(accountId, { last_activity_date: now });"), + ); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(HOOK_API_UPDATE_READONLY_FIELD); + expect(findings[0].severity).toBe('error'); + }); + + it('flags a write to the hook OWN object, not just another object', () => { + // The card's motivating shape is "another object's hook", but the engine + // does not care whose object it is: a fresh ctx.api operation is a fresh + // non-elevated caller either way. + const findings = validateReadonlyHookWrites( + crmStack( + "await ctx.api.object('crm_case').update({ id: ctx.recordId, first_response_date: now });", + { hookObject: 'crm_case' }, + ), + ); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain("'first_response_date'"); + }); + + it('flags each distinct readonly field once, however many times it is written', () => { + const findings = validateReadonlyHookWrites( + crmStack( + "await ctx.api.object('crm_account').update({ last_activity_date: a }); " + + "await ctx.api.object('crm_account').update({ last_activity_date: b });", + ), + ); + expect(findings).toHaveLength(1); + }); + + it('reads the array `fields` authoring shape as well as the map shape', () => { + const findings = validateReadonlyHookWrites({ + objects: [ + { + name: 'crm_account', + fields: [ + { name: 'name', type: 'text' }, + { name: 'last_activity_date', type: 'datetime', readonly: true }, + ], + }, + ], + hooks: [ + { + name: 'touch', + object: 'crm_case', + events: ['afterInsert'], + body: { + language: 'js', + source: "await ctx.api.object('crm_account').update({ last_activity_date: now });", + }, + }, + ], + }); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(HOOK_API_UPDATE_READONLY_FIELD); + }); +}); + +describe('validateReadonlyHookWrites - GREEN: the correct readonly + before-hook pairing', () => { + // THE case this rule exists not to break. Zone 1 of the dispatch order and + // the card both single it out: a blanket write-set rule turns these into a + // noise gate on day one. + it('never flags a ctx.input stamp of a readonly field', () => { + expect( + validateReadonlyHookWrites( + crmStack("ctx.input.name_normalized = ctx.input.name.toLowerCase();", { + hookObject: 'crm_account', + }), + ), + ).toEqual([]); + }); + + it('never flags a ctx.input stamp written through element access or a compound operator', () => { + expect( + validateReadonlyHookWrites( + crmStack( + "ctx.input['last_activity_date'] = now; ctx.input.name_normalized ??= 'x';", + { hookObject: 'crm_account' }, + ), + ), + ).toEqual([]); + }); + + it('never flags Object.assign(ctx.input, ...) stamping readonly fields', () => { + expect( + validateReadonlyHookWrites( + crmStack("Object.assign(ctx.input, { last_activity_date: now, name_normalized: n });", { + hookObject: 'crm_account', + }), + ), + ).toEqual([]); + }); + + it('stays silent on a body that mixes a correct stamp with an unrelated api read', () => { + expect( + validateReadonlyHookWrites( + crmStack( + "const rows = await ctx.api.object('crm_account').find({}); " + + "ctx.input.name_normalized = rows.length;", + { hookObject: 'crm_account' }, + ), + ), + ).toEqual([]); + }); +}); + +describe('validateReadonlyHookWrites - GREEN: the elevated channel', () => { + // `ScopedContext.sudo()` sets isSystem, which the strip skips entirely - the + // hook-side analogue of a flow's runAs:'system'. The extractor's + // `api-crud-literal` matcher requires a literal `ctx.api` receiver, so a + // sudo chain yields no write at all. This test is what turns that from a + // reading of the extractor into a measured guarantee: if a future extractor + // change starts seeing through `.sudo()`, this rule would begin gating the + // one channel the platform recommends, and this case fails first. + it('never flags ctx.api.sudo().object(...).update()', () => { + expect( + validateReadonlyHookWrites( + crmStack("await ctx.api.sudo().object('crm_account').update({ last_activity_date: now });"), + ), + ).toEqual([]); + }); +}); + +describe('validateReadonlyHookWrites - GREEN: INSERT is engine-exempt', () => { + // A create may legitimately seed read-only columns: the engine's static + // readonly strip is deliberately absent from the insert path (#3043/#3413), + // which is the same reason the flow sibling never reads a create_record node. + it('never flags insert()', () => { + expect( + validateReadonlyHookWrites( + crmStack("await ctx.api.object('crm_account').insert({ last_activity_date: now });"), + ), + ).toEqual([]); + }); + + it('never flags create()', () => { + expect( + validateReadonlyHookWrites( + crmStack("await ctx.api.object('crm_account').create({ last_activity_date: now });"), + ), + ).toEqual([]); + }); +}); + +describe('validateReadonlyHookWrites - GREEN: nothing statically knowable is guessed', () => { + it('skips a dynamic object name', () => { + expect( + validateReadonlyHookWrites( + crmStack("await ctx.api.object(target).update({ last_activity_date: now });"), + ), + ).toEqual([]); + }); + + it('skips an object this stack does not declare', () => { + expect( + validateReadonlyHookWrites( + crmStack("await ctx.api.object('other_pkg_object').update({ last_activity_date: now });"), + ), + ).toEqual([]); + }); + + it('skips an object that declares no fields at all (external / introspected)', () => { + // An empty field map answers has(anything) === false, which would read as + // "no such field" for every key - the #4383 false-positive generator. + expect( + validateReadonlyHookWrites({ + objects: [{ name: 'ext_account', fields: {} }], + hooks: [ + { + name: 'touch', + object: 'crm_case', + events: ['afterInsert'], + body: { + language: 'js', + source: "await ctx.api.object('ext_account').update({ last_activity_date: now });", + }, + }, + ], + }), + ).toEqual([]); + }); + + it('leaves a field the object does not declare to the unknown-field rule', () => { + expect( + validateReadonlyHookWrites( + crmStack("await ctx.api.object('crm_account').update({ no_such_column: 1 });"), + ), + ).toEqual([]); + }); + + it('never flags a writable field', () => { + expect( + validateReadonlyHookWrites(crmStack("await ctx.api.object('crm_account').update({ notes: 'x' });")), + ).toEqual([]); + }); + + it("treats `id` in an update payload as the row ADDRESS, not a field write (#8141)", () => { + // The engine strips the address key and then deliberately does NOT log it: + // the caller did not forge it and lost nothing. Reporting it here would + // restate exactly the claim #8141 removed. + expect( + validateReadonlyHookWrites({ + objects: [{ name: 'crm_account', fields: { id: { type: 'text', readonly: true }, name: { type: 'text' } } }], + hooks: [ + { + name: 'touch', + object: 'crm_case', + events: ['afterInsert'], + body: { language: 'js', source: "await ctx.api.object('crm_account').update({ id: accountId });" }, + }, + ], + }), + ).toEqual([]); + }); + + it('stays silent on an unparseable body, leaving it to hook-body-source-unparseable', () => { + // A gating rule must not break a build off a partially recovered tree. + expect( + validateReadonlyHookWrites( + crmStack("await ctx.api.object('crm_account').update({ last_activity_date: now }); if ("), + ), + ).toEqual([]); + }); + + it('ignores an L1 handler hook and a non-js body', () => { + expect( + validateReadonlyHookWrites({ + objects: [{ name: 'crm_account', fields: { last_activity_date: { type: 'datetime', readonly: true } } }], + hooks: [ + { name: 'l1', object: 'crm_case', events: ['afterInsert'] }, + { + name: 'other_lang', + object: 'crm_case', + events: ['afterInsert'], + body: { language: 'cel', source: "ctx.api.object('crm_account').update({ last_activity_date: 1 })" }, + }, + ], + }), + ).toEqual([]); + }); + + it('returns nothing for a stack with no hooks', () => { + expect(validateReadonlyHookWrites({ objects: [] })).toEqual([]); + expect(validateReadonlyHookWrites({})).toEqual([]); + }); +}); + +describe('validateReadonlyHookWrites - readonlyWhen is a SECOND shape, not the same verdict', () => { + // #9107: readonlyWhen strips per record STATE, and it strips a + // beforeUpdate-derived value too. So the write is conditional, not certain - + // warning, exactly as the flow sibling grades it. + it('grades a readonlyWhen field as an advisory warning, not an error', () => { + const findings = validateReadonlyHookWrites( + crmStack("await ctx.api.object('crm_account').update({ credit_hold: true });"), + ); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(HOOK_API_UPDATE_READONLY_WHEN_FIELD); + expect(findings[0].severity).toBe('warning'); + // The own-hook stamp is NOT the remedy here, and the hint must not offer it. + expect(findings[0].hint).not.toContain('ctx.input.credit_hold'); + expect(findings[0].hint).toContain('sudo'); + }); + + it('reports a field carrying BOTH flags as the certain (static readonly) finding', () => { + const findings = validateReadonlyHookWrites({ + objects: [ + { + name: 'crm_account', + fields: { locked: { type: 'boolean', readonly: true, readonlyWhen: 'status == "closed"' } }, + }, + ], + hooks: [ + { + name: 'touch', + object: 'crm_case', + events: ['afterInsert'], + body: { language: 'js', source: "await ctx.api.object('crm_account').update({ locked: true });" }, + }, + ], + }); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(HOOK_API_UPDATE_READONLY_FIELD); + expect(findings[0].severity).toBe('error'); + }); +}); + +describe('READONLY_HOOK_WRITE_PATTERN_IDS - ledger partition', () => { + // The declared answer to "which shared hook write shapes does this rule + // judge?". Their union must BE the shared ledger, so a fifth pattern landing + // on the hook side fails here until someone classifies it - rather than + // being silently assumed into (or out of) a gating rule. + it('partitions the shared hook ledger exactly - no phantom, no unclassified id', () => { + const shared = HOOK_BODY_WRITE_PATTERNS.map((p) => p.id).sort(); + const classified = [ + ...READONLY_HOOK_WRITE_PATTERN_IDS, + ...READONLY_HOOK_WRITE_EXCLUSIONS.map((e) => e.id), + ].sort(); + expect(classified).toEqual(shared); + }); + + it('assigns each ledger shape to exactly one side', () => { + const excluded = READONLY_HOOK_WRITE_EXCLUSIONS.map((e) => e.id); + expect(READONLY_HOOK_WRITE_PATTERN_IDS.filter((id) => excluded.includes(id))).toEqual([]); + }); + + it('gives every exclusion a non-empty reason', () => { + for (const exclusion of READONLY_HOOK_WRITE_EXCLUSIONS) { + expect(exclusion.reason.length, `exclusion '${exclusion.id}' carries no reason`).toBeGreaterThan(0); + } + }); + + it('consumes only the ctx.api shape - every excluded shape stays green on a readonly field', () => { + // Drives the exclusion ledger rather than restating it: each excluded + // pattern's own canonical example is run against a stack where every field + // it writes is declared readonly. A rule that started consuming one of them + // would light up here. + for (const pattern of HOOK_BODY_WRITE_PATTERNS) { + if (READONLY_HOOK_WRITE_PATTERN_IDS.includes(pattern.id)) continue; + const fields = Object.fromEntries( + pattern.example.writes.map((w) => [w.field, { type: 'text', readonly: true }]), + ); + const objectNames = [ + ...new Set(pattern.example.writes.map((w) => w.object).filter((o): o is string => typeof o === 'string')), + ]; + const findings = validateReadonlyHookWrites({ + objects: [ + { name: 'crm_case', fields }, + ...objectNames.map((name) => ({ name, fields })), + ], + hooks: [ + { + name: 'probe', + object: 'crm_case', + events: ['beforeUpdate'], + body: { language: 'js', source: pattern.example.source }, + }, + ], + }); + expect(findings, `excluded pattern '${pattern.id}' produced a finding`).toEqual([]); + } + }); +}); diff --git a/packages/lint/src/validate-readonly-hook-writes.ts b/packages/lint/src/validate-readonly-hook-writes.ts new file mode 100644 index 0000000000..56d3e089b3 --- /dev/null +++ b/packages/lint/src/validate-readonly-hook-writes.ts @@ -0,0 +1,319 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// Build-time guardrail: an L2 hook body that writes a field the target object +// declares `readonly: true` THROUGH `ctx.api` is a SILENT NO-OP (#13653). +// +// `ctx.api` is a `ScopedContext` built over the TRIGGERING operation's +// execution context (`buildHookApi` in packages/objectql/src/engine.ts), so a +// `ctx.api.object('x').update({ ... })` issued while a user-context write is in +// flight arrives at the engine as an ordinary non-system caller. The update +// path then runs `stripReadonlyFields` under `if (!opCtx.context?.isSystem)` +// and deletes every caller-supplied `readonly` key. The engine logs +// +// Field 'last_activity_date' is read-only - ignoring incoming change +// +// ...and the call returns success. The column stays null for the life of the +// app, and only an end-to-end read-back ever notices. +// +// --- THE ASYMMETRY THIS RULE IS BUILT AROUND ------------------------------- +// +// `readonly` + a before-hook is a CORRECT and widely used pairing, and this +// rule must never touch it. `stripReadonlyFields` drops a key only when it is +// still `Object.is`-equal to what the CALLER supplied (`suppliedValues`, and +// #5591's own-value check), so a `beforeInsert`/`beforeUpdate` body stamping +// `ctx.input. = ...` writes a PLATFORM value that survives the strip +// untouched. That is the intended way to maintain a `readonly` column from a +// hook, and the reference app uses it on several fields. +// +// So the judgement is keyed on the WRITE CHANNEL, not on the field: +// +// - `ctx.input. = ...` / `Object.assign(ctx.input, ...)` -> the hook's +// own in-flight payload, a server stamp, survives the strip -> NEVER +// flagged. +// - `ctx.api.object('').update|updateById({ })` -> a fresh +// non-elevated operation whose payload IS caller-supplied -> flagged. +// +// A blanket "readonly field in any write set" rule would fail every correct +// before-hook stamp in the corpus on day one and be switched off by the first +// author who met it. Both directions are pinned in this rule's tests. +// +// --- SCOPE - deliberately narrow, so a finding is worth gating on ---------- +// +// - Only `update` / `updateById`. INSERT is engine-exempt from the +// author-declared static-`readonly` strip (`stripReadonlyForInsert`'s note +// in rule-validator.ts, #3043/#3413: "a create may legitimately seed +// read-only columns"), so `insert`/`create` are not no-ops and are never +// flagged. Exactly the reason the flow sibling skips `create_record`. +// +// - Only a NON-ELEVATED `ctx.api`. `ScopedContext.sudo()` returns a context +// with `isSystem: true`, which the strip skips entirely - the hook-side +// analogue of a flow's `runAs:'system'`, and the intended channel for +// "users cannot edit this, but automation maintains it". A `.sudo()` chain +// is structurally invisible to the extractor (its `api-crud-literal` +// matcher requires a literal `ctx.api` receiver, and `ctx.api.sudo()` is a +// CallExpression), so elevated writes cannot be flagged even by accident. +// Measured, not assumed - `validate-readonly-hook-writes.test.ts` pins it. +// +// - Only a LITERAL object name and a LITERAL payload key. A dynamic object +// (`ctx.api.object(name)`) or a non-literal payload yields no extraction at +// all, so nothing is guessed. +// +// - Only a field the named object DECLARES. A name that resolves to no field +// is `hook-body-write-unknown-field`'s question (a different failure with a +// different fix), and an object this stack does not declare - or declares +// with no fields at all (ADR-0015 `external`, datasource-introspected) - +// cannot be judged and is skipped. +// +// - `id` in an `update` payload is the write's ADDRESS, not a field write. +// The engine strips it and then deliberately does NOT log it (#8141: "this +// key is the write's address, and every claim the message makes about it is +// false"). Reporting it here would restate exactly the false claim that +// removed, so it is excluded on the payload-addressed method. +// +// --- SEVERITY - `error`, and why it differs from this file's neighbour ----- +// +// `validate-hook-body-writes.ts` is advisory-only and says so in its type. This +// rule gates. The two ask different questions and have different epistemics: +// +// - "Does this field exist?" can be wrong for reasons OUTSIDE the stack - a +// package this build cannot see may declare it. Its uncertainty is about +// whether the write is wrong AT ALL. +// - "Is this declared-`readonly` field writable through this channel?" is +// answered entirely from two facts THIS stack declares: the field's +// `readonly`, and the body's literal `ctx.api` update. Both halves are +// local and visible, which is the flow sibling's epistemic position +// (`flow-update-readonly-field`, `error`), not the unknown-field rule's. +// +// The honest caveat, measured rather than glossed: a hook has NO declared run +// identity. A flow declares `runAs`, which is what lets its rule call the strip +// a certainty; a hook inherits its context from whoever triggered the write, so +// this write is dropped whenever the triggering operation is non-system - the +// default, and the only path a user-reachable object can rely on - and lands on +// the system-triggered path. The residual case (a hook whose `ctx.api` write +// only ever runs under a system-triggered operation) is not a stable invariant: +// nothing declares or enforces it, and the first user-context write silently +// voids the stamp. Its remedy is the same `.sudo()` this rule points at, which +// makes the elevation explicit instead of accidental - so the flagged code is +// worth changing under BOTH readings, which is what makes gating defensible +// here where it would not be for an existence check. +// +// Measured field data before choosing to gate: 0 findings across every example +// app in this repo (`examples/app-crm`, `app-showcase`, `app-todo`) - the +// population is recorded in the PR. +// +// Wired via REFERENCE_INTEGRITY_RULES so it runs on `os validate`, `os lint` +// and `os compile` at once - never hand-wired into individual commands, which +// is the divergence that let `os lint` PASS a flow `os validate` refused. + +import { + extractHookBodyWriteSet, + type BodyWritePatternExclusion, +} from './validate-hook-body-writes.js'; +import { buildReadonlyIndex } from './validate-readonly-flow-writes.js'; + +export type ReadonlyHookWriteSeverity = 'error' | 'warning'; + +export interface ReadonlyHookWriteFinding { + severity: ReadonlyHookWriteSeverity; + rule: string; + /** Human-readable location, e.g. `hook "touch_account" > body`. */ + where: string; + /** Config path, e.g. `hooks[0].body.source`. */ + path: string; + message: string; + hint: string; +} + +// Rule ids (registry entries). +export const HOOK_API_UPDATE_READONLY_FIELD = 'hook-api-update-readonly-field'; +export const HOOK_API_UPDATE_READONLY_WHEN_FIELD = 'hook-api-update-readonly-when-field'; + +/** + * The `HOOK_BODY_WRITE_PATTERNS` shapes THIS rule consumes. + * + * Declared as data rather than implied by a branch, for the reason the sibling + * rules declare theirs: a write with no `object` is not a single thing (both + * `ctx.input` and `ctx.record` shapes carry none), so a future ledger addition + * must not be able to land silently in a branch never written for it. + */ +export const READONLY_HOOK_WRITE_PATTERN_IDS: readonly string[] = ['api-crud-literal']; + +/** Ledger shapes this rule leaves alone, each with its reason. */ +export const READONLY_HOOK_WRITE_EXCLUSIONS: readonly BodyWritePatternExclusion[] = [ + { + // The own-value half of the strip's discipline is #5591's fix; the id stays + // in this comment rather than in the string below, which reaches authors. + id: 'input-property-assign', + reason: + 'a `ctx.input. = ...` stamp is a SERVER value, not a caller-supplied one - stripReadonlyFields ' + + 'drops a key only while it is still Object.is-equal to what the caller supplied, so the stamp ' + + 'survives. readonly + a before-hook stamp is the CORRECT pairing this rule must never flag', + }, + { + id: 'input-object-assign', + reason: + 'Object.assign(ctx.input, { ... }) reaches the same in-flight payload as the property form and survives ' + + 'the strip for the same reason - the channel is what decides, not the syntax', + }, + { + id: 'record-property-assign', + reason: + 'a hook sandbox context has no ctx.record at all, so the expression throws at run time rather than ' + + 'silently no-op-ing - a loud failure on the first run is not this rule’s business (the action ' + + 'surface owns that shape)', + }, +]; + +const APPLICABLE_PATTERN_IDS: ReadonlySet = new Set(READONLY_HOOK_WRITE_PATTERN_IDS); + +/** + * `ctx.api` write methods whose payload is subject to the update-path strip. + * + * `insert` / `create` are absent BY DECISION, not by omission: the engine + * exempts INSERT from the author-declared static-`readonly` strip so a create + * may legitimately seed read-only columns (#3043/#3413), which is the same + * reason the flow sibling never looks at a `create_record` node. + */ +const STRIP_SUBJECT_METHODS: ReadonlySet = new Set(['update', 'updateById']); + +/** + * Methods whose payload carries the row ADDRESS rather than only field data. + * `ObjectRepository.update(data)` takes no separate id - it travels inside the + * payload - while `updateById(id, data)` addresses the row in argument 0. + */ +const PAYLOAD_ADDRESSED_METHODS: ReadonlySet = new Set(['update']); + +/** The address key excluded on {@link PAYLOAD_ADDRESSED_METHODS} (#8141). */ +const ADDRESS_KEY = 'id'; + +type AnyRec = Record; + +function isRec(v: unknown): v is AnyRec { + return !!v && typeof v === 'object' && !Array.isArray(v); +} + +/** Coerce an array-or-name-keyed-map collection to an array (name injected). */ +function asArray(v: unknown): AnyRec[] { + if (Array.isArray(v)) return v as AnyRec[]; + if (v && typeof v === 'object') { + return Object.entries(v as AnyRec).map(([name, def]) => ({ + name, + ...(def as AnyRec), + })); + } + return []; +} + +/** + * Validate L2 hook-body `ctx.api` writes against target-object readonly + * declarations. Pure `(stack) => Finding[]` (ADR-0019); safe on pre- or + * post-parse stacks. + */ +export function validateReadonlyHookWrites(stack: AnyRec): ReadonlyHookWriteFinding[] { + const findings: ReadonlyHookWriteFinding[] = []; + const hooks = asArray(stack.hooks); + if (hooks.length === 0) return findings; + + // Built lazily: a stack whose hooks are all L1/handler-based never pays it. + let roIndex: ReturnType | null = null; + + hooks.forEach((hook, hookIndex) => { + const body = hook.body; + if (!isRec(body) || body.language !== 'js') return; + const source = body.source; + if (typeof source !== 'string' || source.trim() === '') return; + + const extracted = extractHookBodyWriteSet(source); + // A body that did not parse yields whatever error recovery left readable, + // and this rule GATES - so a mis-extraction here would break a build over a + // write the author may never have made. The author is not left in silence: + // `validate-hook-body-writes.ts` reports the unparseable body itself + // (`hook-body-source-unparseable`), which is the finding that actually + // describes the problem. Skip rather than guess at error severity. + if (extracted.parseFailure) return; + + const writes = extracted.writes.filter( + (w) => + APPLICABLE_PATTERN_IDS.has(w.patternId) && + typeof w.object === 'string' && + w.method !== undefined && + STRIP_SUBJECT_METHODS.has(w.method), + ); + if (writes.length === 0) return; + + roIndex ??= buildReadonlyIndex(asArray(stack.objects)); + + const hookName = typeof hook.name === 'string' && hook.name ? hook.name : `#${hookIndex}`; + const where = `hook "${hookName}" > body`; + const path = `hooks[${hookIndex}].body.source`; + const reported = new Set(); + + for (const w of writes) { + const objectName = w.object as string; + const method = w.method as string; + + // The write's address, not a field write (#8141). + if (w.field === ADDRESS_KEY && PAYLOAD_ADDRESSED_METHODS.has(method)) continue; + + // An object this stack does not declare - or one that declares no fields + // at all - cannot be judged; an empty field map answers "no such field" + // for EVERY key, which is a false-positive generator (#4383). + const fieldMap = roIndex.get(objectName); + if (!fieldMap || fieldMap.size === 0) continue; + + // A field the object does not declare is `hook-body-write-unknown-field`'s + // question, never this one - the two must not double-report one key. + const meta = fieldMap.get(w.field); + if (!meta) continue; + + const dedupeKey = `${objectName} ${w.field}`; + if (reported.has(dedupeKey)) continue; + + const call = `ctx.api.object('${objectName}').${method}(...)`; + + if (meta.readonly) { + reported.add(dedupeKey); + findings.push({ + severity: 'error', + rule: HOOK_API_UPDATE_READONLY_FIELD, + where, + path, + // The static-`readonly` write-path strip is #2948; the id stays here, + // out of the message an author reads and cannot resolve. + message: + `body writes field '${w.field}' through ${call}, and object '${objectName}' declares it ` + + `readonly:true. A hook's ctx.api is a ScopedContext over the TRIGGERING operation's context, so ` + + `on every non-system trigger the engine strips readonly keys from that UPDATE payload - ` + + `the write never lands, while the call still returns success.`, + hint: + `If automation is meant to maintain '${w.field}', either stamp it on the record's OWN hook ` + + `(ctx.input.${w.field} = ... in beforeInsert/beforeUpdate survives the strip, and is the ` + + `recommended shape), or make the elevation explicit with ctx.api.sudo().object('${objectName}') ` + + `- deliberately, since sudo bypasses the acting user's row and field permissions for that write. ` + + `Otherwise drop readonly:true from '${w.field}'.`, + }); + } else if (meta.readonlyWhen) { + reported.add(dedupeKey); + findings.push({ + severity: 'warning', + rule: HOOK_API_UPDATE_READONLY_WHEN_FIELD, + where, + path, + // The conditional strip is #3042; that it also removes a + // beforeUpdate-derived value is #9107. Both ids stay in this comment. + message: + `body writes field '${w.field}' through ${call}, and object '${objectName}' declares it ` + + `readonlyWhen. On records whose predicate is TRUE that UPDATE strips the field, so this ` + + `write may silently not land depending on the record's state.`, + hint: + `readonlyWhen strips even a beforeUpdate-derived value, so an own-hook stamp is NOT a ` + + `workaround here. If automation must maintain '${w.field}' regardless of record state, write it ` + + `through ctx.api.sudo(). Otherwise confirm this call only targets records whose readonlyWhen ` + + `predicate is FALSE.`, + }); + } + } + }); + + return findings; +}