From eac21368573745e4edd2b8f763caaf1816825f79 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Tue, 28 Jul 2026 12:31:37 +0800 Subject: [PATCH] fix(automation)!: refuse data ops for a run with no trigger user (#3760) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An effective `runAs:'user'` run that resolves no trigger user executed its data nodes UNSCOPED: it presented no principal, and the data security middleware skips when there is no principal, so the run read and wrote every row. `runAs:'user'` is an access-NARROWING declaration, and ADR-0049's standing rule is that failing to resolve one must never resolve to a grant. It now throws `UnscopedRunDataAccessError` from `resolveRunDataContext` — the single place every data node resolves its context. This was never really about schedules. The docs, the spec, the runtime warning and the lint all described a schedule-shaped problem, and the lint only matched that shape, but the runtime predicate is "no user". The commonest way to have no user is a record-change flow fired by a write that carried none: `isSystem` does not suppress trigger dispatch — only `skipTriggers` does, and three first-party paths set it — so plugin/service system writes, the approvals status mirror, and a `runAs:'system'` flow's own data node all dispatched record-change flows with `userId: undefined`. Ordinary users reach those writes routinely, so the fail-open was reachable by unprivileged input. Deliberately NOT implemented as "inherit the write's posture and run as isSystem". That reads like a relabel but is an escalation: the middleware's isSystem short-circuit (security-plugin.ts:722) fires before the package-managed-row, system-row, audience-anchor and delegated-admin gates (795/809/817/844), all of which a principal-less context still clears. Such a run cannot write sys_user_position today; as isSystem it could. - Lint `flow-schedule-runas-unscoped` -> `flow-runas-unscoped`, now FAILS the build and covers time_relative + api (ADR-0073 D5). It documented itself as "NEVER fails the build" — a gate that behaved as a comment. It still cannot cover record_change, which is undecidable at authoring time. - Three seed writes (seed-loader pass-2 back-fill, both app-plugin fallbacks) inlined a bare `{isSystem:true}` and so seeded with automation live. - #3712's user-less provenance path is subsumed: such runs are refused before the approvals lock is consulted, and a schedule reaches its own record via `runAs:'system'`. The flowRunId exemption stays live for runs with a user. - ADR-0073 amended: its "no untrusted-input path" severity claim is falsified, and its rejection of fail-closed expired when the example flows it cited were fixed to declare `runAs:'system'`. Both new gates were verified to go red against the pre-fix behavior, including the live dogfood stack, which previously pinned the fail-open explicitly. Refs #1888, #3712, #3749, #3456, ADR-0049, ADR-0073. Co-Authored-By: Claude --- .changeset/user-less-run-data-ops-refused.md | 77 ++++++++++ content/docs/automation/approvals.mdx | 4 +- .../docs/references/api/automation-api.mdx | 2 +- content/docs/references/automation/flow.mdx | 2 +- .../adr/0073-automation-execution-identity.md | 24 ++- ...g-surface-boundary-hook-flow-validation.md | 2 +- ...96-execution-surface-identity-admission.md | 2 +- packages/cli/src/commands/compile.ts | 36 ++++- .../cli/src/utils/lint-flow-patterns.test.ts | 55 ++++++- packages/cli/src/utils/lint-flow-patterns.ts | 84 +++++++--- .../src/seed-loader-deferred-failure.test.ts | 26 ++++ packages/metadata-protocol/src/seed-loader.ts | 8 +- ...cord-lock-schedule-run.integration.test.ts | 90 +++++++---- .../test/flow-runas-schedule.dogfood.test.ts | 74 +++++---- packages/runtime/src/app-plugin.ts | 16 +- .../src/builtin/crud-config-aliases.test.ts | 14 +- .../src/builtin/crud-output-var.test.ts | 7 +- .../src/builtin/crud-runas.test.ts | 144 ++++++++++++------ .../services/service-automation/src/engine.ts | 29 ++-- .../services/service-automation/src/index.ts | 7 +- .../src/runtime-identity.ts | 124 ++++++++++----- packages/spec/src/automation/flow.zod.ts | 15 +- .../src/formula-context.test.ts | 2 +- .../src/multilookup-context.test.ts | 2 +- .../src/record-change-integration.test.ts | 91 ++++++++++- 25 files changed, 721 insertions(+), 216 deletions(-) create mode 100644 .changeset/user-less-run-data-ops-refused.md diff --git a/.changeset/user-less-run-data-ops-refused.md b/.changeset/user-less-run-data-ops-refused.md new file mode 100644 index 0000000000..6e96c9ed48 --- /dev/null +++ b/.changeset/user-less-run-data-ops-refused.md @@ -0,0 +1,77 @@ +--- +"@objectstack/service-automation": major +"@objectstack/cli": major +"@objectstack/plugin-approvals": patch +"@objectstack/metadata-protocol": patch +"@objectstack/runtime": patch +"@objectstack/spec": patch +--- + +feat(automation)!: a flow run with no trigger user may no longer touch data (#3760) + +An effective `runAs:'user'` run that resolves **no trigger user** used to execute +its data nodes **UNSCOPED** — it presented no principal, and the data security +middleware skips when there is no principal, so the run read and wrote every row. +`runAs:'user'` is an access-*narrowing* declaration; failing to resolve it must +never resolve to a grant (ADR-0049). It now **refuses** the operation +(`UnscopedRunDataAccessError`), naming `runAs:'system'` as the fix. + +**This was never really about schedules.** The docs, the spec, the runtime +warning and the lint all described a schedule-shaped problem, and the lint only +ever matched that shape. But the runtime predicate is "no user", and the +commonest way to have no user is a **record-change flow fired by a write that +carried none**: `isSystem` does *not* suppress trigger dispatch — only +`skipTriggers` does, and exactly three first-party paths set it — so every +plugin/service system write, the approvals status mirror, and a `runAs:'system'` +flow's own data node dispatched record-change flows with `userId: undefined`. +Ordinary users reach those writes routinely (submitting for approval mirrors a +status onto the target record), so the fail-open was reachable by unprivileged +input and was the common case, not the rare one. + +Deliberately **not** implemented as "inherit the triggering write's posture and +run as `isSystem`". That reads like a relabel but is a privilege escalation: the +security middleware's `isSystem` short-circuit fires *before* its +package-managed-row, system-row, audience-anchor and delegated-admin gates, all +of which a principal-less context still has to clear. Such a run cannot write +`sys_user_position` today; as `isSystem` it could. "Unscoped" was never +equivalent to "system". + +**Breaking — how to migrate.** A flow that reacts to system writes and needs to +act beyond one user's grants declares `runAs: 'system'`, making the elevation +explicit and audit-attributable. Otherwise ensure the trigger supplies a user. +Flows that touch no data are unaffected (`runAs` is moot), and the failure is +isolated: the trigger already swallows flow errors, so the originating write +still succeeds. The engine warns at run *setup*, before any node executes. + +**#3712's user-less provenance path is subsumed, not broken.** That fix let a +run with no trigger user write its own approval-locked record by carrying a +provenance-only ObjectQL context (the run id, nothing else). Such a run can no +longer perform a data operation at all — presenting no principal is exactly what +made the write unscoped — so it is refused before the lock is consulted. The +capability survives via the explicit route: a schedule that must write records +declares `runAs:'system'`, which the lock hook exempts on its own `isSystem` +branch. The `flowRunId` exemption itself stays live and load-bearing for what +#3703 built it for — a `runAs:'user'` run that *does* have a user — where the +exemption is still provenance rather than privilege. + +Also in this change: + +- **`flow-schedule-runas-unscoped` → `flow-runas-unscoped`, and it now fails the + build.** It read as a gate and behaved as a comment — `os compile` documented + that the flow lint "NEVER fails the build" — which is close to no net at all + for the audience it protects, very often an AI generating flows in bulk. It now + also covers the other provably user-less triggers (`time_relative`, `api`), per + ADR-0073 D5. It still cannot cover `record_change`, which is undecidable at + authoring time — that is exactly why the runtime refusal exists. +- **Three seed writes stopped firing automation.** The seed loader's pass-2 + deferred-reference back-fill and both of `AppPlugin`'s basic-insert fallbacks + inlined a bare `{ isSystem: true }` instead of the shared seed options, so they + seeded with record-change automation live — the self-trigger vector + `skipTriggers` exists to prevent, on the writes that skipped it. +- **ADR-0073 amended.** Its severity rationale ("an unprivileged user cannot + trigger a schedule, so there is no untrusted-input path") is falsified, and its + rejection of fail-closed ("breaks legitimate scheduled CRUD — 2/3 example flows + relied on the default") expired when those flows were fixed to declare + `runAs:'system'`. Refusal is an interim posture, forward-compatible with the + ADR's `automation` principal: when that lands, the refusal point becomes the + place that resolves it. diff --git a/content/docs/automation/approvals.mdx b/content/docs/automation/approvals.mdx index c946dd18a9..4d09b3aa09 100644 --- a/content/docs/automation/approvals.mdx +++ b/content/docs/automation/approvals.mdx @@ -24,7 +24,9 @@ A flow declares `runAs` (ADR-0049), and for approvals this is the decision that - `runAs: 'user'` (default) — the flow's data operations run as the **submitter**, respecting their RLS. Good when the flow only touches records the submitter can already see. - `runAs: 'system'` — **elevated**, bypasses RLS. Needed when the flow must read/write records the submitter can't (e.g. post to a ledger, notify an approver who owns rows the submitter can't see). Declare it **explicitly** so the elevation is visible, not accidental. -A schedule-triggered escalation has no triggering user, so under the default `runAs: 'user'` its data operations run **unscoped** (elevated, RLS-bypassing) anyway — declare `system` to make that elevation explicit and intended rather than an implicit fail-open (the engine warns, and `os lint` flags the bare shape as `flow-schedule-runas-unscoped`). +A run that resolves **no** triggering user has nothing to scope to, so under the default `runAs: 'user'` its data operations are **refused** — declare `system` to make the elevation explicit and intended. `os lint` rejects the shapes it can prove at authoring time (`flow-runas-unscoped`, covering schedule / time-relative / api triggers), and the engine warns at run setup before refusing. + +This is **not** a schedule-only concern, and for approvals it is the common case: the approvals service mirrors a decision back onto the target record with a **system** write, which carries no user. Any record-change flow bound to that object then runs with no triggering user — so a flow left at the default `runAs: 'user'` is refused. Nothing can flag that at authoring time (whether a given write carries a user is only knowable at run time), so declare `runAs: 'system'` on record-change flows that react to approval outcomes. ### 3. The approval node diff --git a/content/docs/references/api/automation-api.mdx b/content/docs/references/api/automation-api.mdx index 7fca8f1d1c..9c3d278b38 100644 --- a/content/docs/references/api/automation-api.mdx +++ b/content/docs/references/api/automation-api.mdx @@ -108,7 +108,7 @@ const result = AutomationApiErrorCode.parse(data); | **nodes** | `{ id: string; type: string; label: string; config?: Record; … }[]` | ✅ | Flow nodes | | **edges** | `{ id: string; source: string; target: string; condition?: string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }; … }[]` | ✅ | Flow connections | | **active** | `boolean` | optional | Is active (Deprecated: use status) | -| **runAs** | `Enum<'system' \| 'user'>` | optional | Execution identity for the run: system = elevated (bypasses RLS), user = the triggering user (RLS-respecting). A schedule-triggered run has no trigger user, so under user it runs UNSCOPED (elevated) — declare system to make that explicit. | +| **runAs** | `Enum<'system' \| 'user'>` | optional | Execution identity for the run: system = elevated (bypasses RLS), user = the triggering user (RLS-respecting). A run with no trigger user has no identity to scope to, so under user its data operations are REFUSED — declare system to make the elevation explicit. This covers schedule/time-relative/api triggers AND any record-change flow fired by a write that carried no user. | | **errorHandling** | `{ strategy?: Enum<'fail' \| 'retry' \| 'continue'>; maxRetries?: integer; retryDelayMs?: integer; backoffMultiplier?: number; … }` | optional | Flow-level error handling configuration | | **protection** | `{ lock: Enum<'none' \| 'no-overlay' \| 'no-delete' \| 'full'>; reason: string; docsUrl?: string }` | optional | Package author protection block — lock policy for this flow. | | **_lock** | `Enum<'none' \| 'no-overlay' \| 'no-delete' \| 'full'>` | optional | Item-level lock — controls overlay & delete (ADR-0010). | diff --git a/content/docs/references/automation/flow.mdx b/content/docs/references/automation/flow.mdx index 8c375680ed..c91c5e36c1 100644 --- a/content/docs/references/automation/flow.mdx +++ b/content/docs/references/automation/flow.mdx @@ -58,7 +58,7 @@ const result = Flow.parse(data); | **nodes** | `{ id: string; type: string; label: string; config?: Record; … }[]` | ✅ | Flow nodes | | **edges** | `{ id: string; source: string; target: string; condition?: string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }; … }[]` | ✅ | Flow connections | | **active** | `boolean` | optional | Is active (Deprecated: use status) | -| **runAs** | `Enum<'system' \| 'user'>` | optional | Execution identity for the run: system = elevated (bypasses RLS), user = the triggering user (RLS-respecting). A schedule-triggered run has no trigger user, so under user it runs UNSCOPED (elevated) — declare system to make that explicit. | +| **runAs** | `Enum<'system' \| 'user'>` | optional | Execution identity for the run: system = elevated (bypasses RLS), user = the triggering user (RLS-respecting). A run with no trigger user has no identity to scope to, so under user its data operations are REFUSED — declare system to make the elevation explicit. This covers schedule/time-relative/api triggers AND any record-change flow fired by a write that carried no user. | | **errorHandling** | `{ strategy?: Enum<'fail' \| 'retry' \| 'continue'>; maxRetries?: integer; retryDelayMs?: integer; backoffMultiplier?: number; … }` | optional | Flow-level error handling configuration | | **protection** | `{ lock: Enum<'none' \| 'no-overlay' \| 'no-delete' \| 'full'>; reason: string; docsUrl?: string }` | optional | Package author protection block — lock policy for this flow. | | **_lock** | `Enum<'none' \| 'no-overlay' \| 'no-delete' \| 'full'>` | optional | Item-level lock — controls overlay & delete (ADR-0010). | diff --git a/docs/adr/0073-automation-execution-identity.md b/docs/adr/0073-automation-execution-identity.md index 28cd589750..b109bd74be 100644 --- a/docs/adr/0073-automation-execution-identity.md +++ b/docs/adr/0073-automation-execution-identity.md @@ -11,6 +11,22 @@ --- +## Amendment: user-less data ops are now REFUSED (2026-07-28, #3760) + +Two load-bearing claims below turned out to be wrong. Both were about *severity*, not about the model — D1–D5 stand unchanged, and D5 shipped in full. + +**1. "There is no untrusted-input path to the fail-open" — false.** The Severity section argues the risk is acute-mitigated because schedules are admin/AI-authored and "an unprivileged user cannot trigger a schedule". That is true of schedules and irrelevant to the actual exposure. The fail-open predicate is `runAs !== 'system' && !userId` — *any* run that resolves no user — while the lint only ever covered the schedule shape. The dominant real-world shape is a **record-change flow fired by a write that carried no user**: `isSystem` does **not** suppress trigger dispatch (only `skipTriggers` does, and exactly three first-party paths set it), so every plugin/service system write, the approvals status mirror, and a `runAs:'system'` flow's own data node all dispatch record-change flows with `userId: undefined`. Ordinary users reach those writes routinely — submitting for approval mirrors a status onto the target record. So the fail-open was reachable by unprivileged input, and was the common case rather than the rare one. + +**2. "Fail-closed … Rejected in #2308: breaks legitimate scheduled CRUD (2/3 example flows relied on the default)" — expired.** Those example flows were fixed as part of #2308 itself; every first-party schedule-triggered flow now declares `runAs: 'system'`. The stated cost of fail-closed no longer exists. + +**Consequently `runAs:'user'` + no trigger user now REFUSES the data operation** (`UnscopedRunDataAccessError`, thrown from `resolveRunDataContext` — the single place every data node resolves its context). This implements D5's ruling ("user-less `runAs:'user'` is a configuration error") at the only layer that can see the record-change case, since whether a triggering write carries a user is not knowable at authoring time. + +Note what this deliberately is **not**: it does not re-badge these runs as `isSystem`. The security middleware's `isSystem` short-circuit precedes its package-managed-row / system-row / audience-anchor / delegated-admin gates, all of which a principal-less context still has to clear — so "unscoped" was never equivalent to "system", and elevating would have *widened* these runs (e.g. letting them write `sys_user_position`) rather than preserving the status quo. + +This is an **interim** posture, not a replacement for D2. Refusal is strictly safer than today and forward-compatible: when the `automation` principal lands, the refusal point becomes the place that resolves it, and these runs go from *refused* to *RLS-enforced* with no third state. Ordering is unchanged — D2 still waits for a real consumer. + +--- + ## TL;DR 1. **[model] Automation is a first-class non-human identity, expressed as a built-in role** (the ADR-0068 idiom): the **environment's `automation` principal** — a Data-Plane identity living in that environment's own kernel/DB. A user-less run resolves to an `EvalUser` whose `id` is the env's stable automation principal and whose `roles` carry the `automation` role. There is **no anonymous run**. (Cross-environment, platform-wide automation is a **Control-Plane** concern — ADR-0002/0004 — out of scope; see D4.) @@ -52,7 +68,7 @@ Two findings sharpen the problem: This is a **footgun / hardening** issue, not an actively exploited hole, and the acute risk is **already mitigated**: -- Scheduled flows are **admin/AI-authored metadata**; an unprivileged user **cannot trigger a schedule**, so there is no untrusted-input path to the fail-open. +- ~~Scheduled flows are **admin/AI-authored metadata**; an unprivileged user **cannot trigger a schedule**, so there is no untrusted-input path to the fail-open.~~ **Falsified (#3760)** — see the Amendment. True of schedules, but schedules were never the boundary: a record-change flow fired by a user-less system write hits the identical fail-open, and unprivileged users reach those writes routinely. - **#2308 already shipped** the cheap mitigations: a build-time lint, a runtime warning, and fixing the example flows to explicit `runAs:'system'`. The bleeding is stopped. - Tenant isolation is **physical — environment-per-database** (ADR-0002): each tenant environment is its own kernel + DB. So the hard problem (cross-tenant RLS for an automation principal) **does not exist in this architecture** — the automation principal is a purely *intra-environment* Data-Plane identity with no cross-tenant data reach to scope. (The platform is also pre-launch / single-operator.) - The live automation surface is **tiny**, and — decisively — the existing scheduled flows (`stale_opportunity_sweep`, the app-todo sweeps) all want **full `system` elevation**, not the RLS-respecting middle. **The `automation` mode this ADR introduces has zero consumers in the current app set.** @@ -128,7 +144,7 @@ A scheduled / unauthenticated-webhook trigger has no user; `runAs:'user'` there **v1 — land now (no runtime machinery):** 1. **This decision record** — pins the model (D1–D4) + `runAs` posture semantics, so the AI authors flows against the right target and M2 has a contract. (Cheapest to land pre-scale, exactly the ADR-0068 v1 argument.) -2. **Author-time guardrail (D5)** — extend the #2308 `flow-schedule-runas-unscoped` lint to every user-less trigger type (api/webhook/queue), and make user-less `runAs:'user'` a **validation error** at compile. Small, non-breaking, and the real present value: it stops the AI from generating the wrong pattern before there is a large body of it. +2. **Author-time guardrail (D5)** — extend the #2308 `flow-runas-unscoped` lint to every user-less trigger type (api/webhook/queue), and make user-less `runAs:'user'` a **validation error** at compile. Small, non-breaking, and the real present value: it stops the AI from generating the wrong pattern before there is a large body of it. Runtime behavior is otherwise **unchanged** from #2308 (the audible warning stays). **We do not seed the roles, mint the principal, or touch `runAs` resolution in v1** — there is no consumer, so doing so would be inert/speculative (unlike ADR-0068 v1, whose seeded roles had a live `current_user` consumer). @@ -163,14 +179,14 @@ Runtime behavior is otherwise **unchanged** from #2308 (the audible warning stay - **Build the whole model now (seed roles + principal + runtime).** Rejected: zero current consumer, single-operator, acute risk already mitigated → the speculative over-build ADR-0049 warns against. - **Keep NULL-then-claim for automation.** Rejected: no claim event for perpetual automation, so attribution never converges; does nothing for authorization. - **Stop at the #2308 runtime warning.** Necessary but insufficient as the *end-state*: makes the fail-open audible without eliminating it, and leaves writes unattributed — hence this ADR fixes the *model* even though the *build* waits. -- **Fail-closed (deny user-less data ops).** Rejected in #2308: breaks legitimate scheduled CRUD (2/3 example flows relied on the default) and gives no attribution. +- **Fail-closed (deny user-less data ops).** ~~Rejected in #2308: breaks legitimate scheduled CRUD (2/3 example flows relied on the default) and gives no attribution.~~ **ADOPTED 2026-07-28 (#3760)** — see the Amendment above. The example flows this protected now declare `runAs:'system'`, so the stated cost expired; and the attribution objection does not apply, since refusing an operation attributes nothing either way (attribution remains D3's job, unchanged). - **Reuse `runAs:'system'` for scheduled (silent elevation).** Rejected: hides author intent; the ambient god-mode the four invariants warn against. ## Conformance checklist **v1 (now):** 1. **`@objectstack/spec`** — document the automation identity as an `EvalUser` (D1) and the three-posture `runAs` semantics (D2) in `FlowSchema.runAs` describe, **marked target-state** for `automation`. -2. **`@objectstack/cli`** — extend `flow-schedule-runas-unscoped` to all user-less trigger types; make user-less `runAs:'user'` a hard validation error (D5). +2. **`@objectstack/cli`** — extend `flow-runas-unscoped` to all user-less trigger types; make user-less `runAs:'user'` a hard validation error (D5). **M2 (gated on first consumer):** 3. **`plugin-security`** — seed the per-environment `automation` `sys_role` row (sibling to `bootstrap-declared-roles`); extend the non-human exclusion guards. diff --git a/docs/adr/0077-authoring-surface-boundary-hook-flow-validation.md b/docs/adr/0077-authoring-surface-boundary-hook-flow-validation.md index ea76a5fa8c..87bb41fdd7 100644 --- a/docs/adr/0077-authoring-surface-boundary-hook-flow-validation.md +++ b/docs/adr/0077-authoring-surface-boundary-hook-flow-validation.md @@ -89,7 +89,7 @@ The genuine overlap is `after-*` side effects (write succeeded → notify / audi ### 4. Loud-not-silent — the two new `os build` lints (the only code in v1) -Authored alongside the existing flow lints in `packages/cli/src/utils/lint-flow-patterns.ts` (which already ships `flow-schedule-runas-unscoped`, `flow-double-brace-interpolation`, etc.): +Authored alongside the existing flow lints in `packages/cli/src/utils/lint-flow-patterns.ts` (which already ships `flow-runas-unscoped`, `flow-double-brace-interpolation`, etc.): - **`flow-record-before-cannot-mutate`** — *error*. A flow bound to `record-before-*` that contains a `create_record`/`update_record` node targeting the **triggering object/record**, or otherwise reads as expecting to change the in-flight record. Message points to **hook** (rewrite) or **validation rule** (veto). - **`flow-record-before-cannot-veto`** — *error*. A `record-before-*` flow whose shape implies it intends to stop the write (e.g. a decision branch ending in an error/`end` node presented as rejection). Message: *"record-change flows cannot abort the triggering write — its errors are isolated by design; use a validation rule to reject, or a hook to throw."* diff --git a/docs/adr/0096-execution-surface-identity-admission.md b/docs/adr/0096-execution-surface-identity-admission.md index 52df303864..c13b572aba 100644 --- a/docs/adr/0096-execution-surface-identity-admission.md +++ b/docs/adr/0096-execution-surface-identity-admission.md @@ -123,7 +123,7 @@ find(object: string, identity: CallIdentity, options?: QueryOptions): Promise); - if (flowLint.length > 0 && !flags.json) { + const flowLintErrors = flowLint.filter((f) => f.severity === 'error'); + const flowLintWarnings = flowLint.filter((f) => f.severity !== 'error'); + if (flowLintWarnings.length > 0 && !flags.json) { console.log(''); - for (const fnd of flowLint) { + for (const fnd of flowLintWarnings) { printWarning(`${fnd.where}: ${fnd.message}`); console.log(chalk.dim(` ${fnd.hint}`)); console.log(chalk.dim(` rule: ${fnd.rule}`)); } } + if (flowLintErrors.length > 0) { + if (flags.json) { + this.log(JSON.stringify({ success: false, flowLintErrors }, null, 2)); + this.exit(1); + } + console.log(''); + printError(`Flow authoring check failed (${flowLintErrors.length} error${flowLintErrors.length > 1 ? 's' : ''})`); + for (const fnd of flowLintErrors) { + console.log(` • ${fnd.where}: ${fnd.message}`); + console.log(chalk.dim(` ${fnd.hint}`)); + console.log(chalk.dim(` rule: ${fnd.rule}`)); + } + this.exit(1); + } // 3d-bis. Liveness author-warning lint — close the spec-liveness loop on // the author side: an authored property the ledger marks dead-and- diff --git a/packages/cli/src/utils/lint-flow-patterns.test.ts b/packages/cli/src/utils/lint-flow-patterns.test.ts index b04b55f6ac..683844928a 100644 --- a/packages/cli/src/utils/lint-flow-patterns.test.ts +++ b/packages/cli/src/utils/lint-flow-patterns.test.ts @@ -11,14 +11,14 @@ import { FLOW_APPROVAL_REVISE_DEAD_END, FLOW_APPROVAL_REVISE_UNMARKED_BACKEDGE, FLOW_APPROVAL_REVISE_DISABLED, - FLOW_SCHEDULE_RUNAS_UNSCOPED, + FLOW_RUNAS_UNSCOPED, } from './lint-flow-patterns.js'; const CEL = (source: string) => ({ dialect: 'cel', source }); /** * A scheduled flow with a get_record node carrying `filter`. Declares * `runAs: 'system'` so it is the correct shape for a scheduled data flow and - * does not also trip the schedule-runAs lint (FLOW_SCHEDULE_RUNAS_UNSCOPED) — + * does not also trip the user-less runAs lint (FLOW_RUNAS_UNSCOPED) — * keeping these date-equality cases focused on the filter rule. */ const filterFlow = (filter: unknown) => ({ @@ -260,7 +260,7 @@ describe('lintFlowPatterns — approval revise loop (ADR-0044)', () => { }); }); -describe('lintFlowPatterns — schedule runAs unscoped (#1888 / ADR-0049)', () => { +describe('lintFlowPatterns — user-less runAs unscoped (#1888 / ADR-0049 / ADR-0073 D5 / #3760)', () => { /** A schedule-triggered flow that performs a data op, parameterized by runAs / detection signal. */ const scheduledDataFlow = (opts: { runAs?: 'system' | 'user'; @@ -283,17 +283,17 @@ describe('lintFlowPatterns — schedule runAs unscoped (#1888 / ADR-0049)', () = it('flags a schedule flow whose runAs is unset (defaults to user → unscoped)', () => { const fnds = lintFlowPatterns(scheduledDataFlow()); expect(fnds).toHaveLength(1); - expect(fnds[0].rule).toBe(FLOW_SCHEDULE_RUNAS_UNSCOPED); + expect(fnds[0].rule).toBe(FLOW_RUNAS_UNSCOPED); expect(fnds[0].where).toContain('nightly_sweep'); expect(fnds[0].message).toMatch(/default .*runAs:'user'/); - expect(fnds[0].message).toMatch(/UNSCOPED/); + expect(fnds[0].message).toMatch(/REFUSED/); expect(fnds[0].hint).toMatch(/runAs:'system'/); }); it("flags an EXPLICIT runAs:'user' on a schedule (incoherent — no user to scope to)", () => { const fnds = lintFlowPatterns(scheduledDataFlow({ runAs: 'user' })); expect(fnds).toHaveLength(1); - expect(fnds[0].rule).toBe(FLOW_SCHEDULE_RUNAS_UNSCOPED); + expect(fnds[0].rule).toBe(FLOW_RUNAS_UNSCOPED); expect(fnds[0].message).toMatch(/runAs:'user'/); }); @@ -309,10 +309,51 @@ describe('lintFlowPatterns — schedule runAs unscoped (#1888 / ADR-0049)', () = it('flags each data-op node type (get/create/update/delete)', () => { for (const t of ['get_record', 'create_record', 'update_record', 'delete_record']) { const fnds = lintFlowPatterns(scheduledDataFlow({ nodeType: t })); - expect(fnds.map((f) => f.rule), `node ${t}`).toContain(FLOW_SCHEDULE_RUNAS_UNSCOPED); + expect(fnds.map((f) => f.rule), `node ${t}`).toContain(FLOW_RUNAS_UNSCOPED); } }); + // #3760 — this rule FAILS the build. Every other rule in this file is + // advisory; this one flags metadata the runtime refuses to execute, so a + // warning would just be a slower way of finding out. If this assertion is ever + // relaxed the gate silently becomes a comment again — which is exactly the + // state #3760 found it in. + it('is a BLOCKING finding (severity: error), unlike every advisory rule here', () => { + const fnds = lintFlowPatterns(scheduledDataFlow()); + expect(fnds[0].severity).toBe('error'); + }); + + // ADR-0073 D5 — the rule was scoped to `schedule`, but a schedule is not the + // boundary: these triggers build an AutomationContext with no `userId` at all, + // so they hit the identical refusal. + it('flags the OTHER provably user-less triggers too — time-relative and api', () => { + const timeRelative = lintFlowPatterns( + scheduledDataFlow({ flowType: 'autolaunched', startConfig: { timeRelative: { object: 'task', field: 'due_at', offsetDays: -1 } } }), + ); + expect(timeRelative.map((f) => f.rule)).toContain(FLOW_RUNAS_UNSCOPED); + expect(timeRelative[0].message).toMatch(/time-relative/); + + const api = lintFlowPatterns(scheduledDataFlow({ flowType: 'api', startConfig: {} })); + expect(api.map((f) => f.rule)).toContain(FLOW_RUNAS_UNSCOPED); + expect(api[0].message).toMatch(/api/); + + const apiByTriggerType = lintFlowPatterns( + scheduledDataFlow({ flowType: 'autolaunched', startConfig: { triggerType: 'api' } }), + ); + expect(apiByTriggerType.map((f) => f.rule)).toContain(FLOW_RUNAS_UNSCOPED); + }); + + // The deliberate limit of this rule. A record-change flow hits the same + // refusal when its triggering write carried no user, but that is a RUNTIME + // property — approximating it here would fire on every record-change flow in + // existence. Pinned so nobody "fixes" the gap by guessing. + it('does NOT flag record_change — undecidable at authoring time, caught at run time', () => { + const fnds = lintFlowPatterns( + scheduledDataFlow({ flowType: 'record_change', startConfig: { triggerType: 'record_change', objectName: 'invoice' } }), + ); + expect(fnds.map((f) => f.rule)).not.toContain(FLOW_RUNAS_UNSCOPED); + }); + describe('does NOT flag (false-positive guards)', () => { it("a schedule flow that declares runAs:'system' (the correct shape)", () => { expect(lintFlowPatterns(scheduledDataFlow({ runAs: 'system' }))).toHaveLength(0); diff --git a/packages/cli/src/utils/lint-flow-patterns.ts b/packages/cli/src/utils/lint-flow-patterns.ts index 9bda03fa2a..ffef3821f7 100644 --- a/packages/cli/src/utils/lint-flow-patterns.ts +++ b/packages/cli/src/utils/lint-flow-patterns.ts @@ -3,10 +3,16 @@ /** * Build-time lint for flow authoring ANTI-PATTERNS — metadata that is valid * (passes schema + expression checks) but is semantically a footgun at runtime. - * These are emitted as WARNINGS: they guide the author (very often an AI + * Most are emitted as WARNINGS: they guide the author (very often an AI * generating templates) toward the robust pattern without failing the build on * a technically-legal construct. * + * A finding carrying `severity: 'error'` FAILS the build. That is reserved for + * shapes that are a *guaranteed* runtime failure rather than a risk — currently + * only {@link FLOW_RUNAS_UNSCOPED}, where the runtime refuses the data + * operation outright (#3760), so warning about it would just be a slower way of + * finding out. + * * #1874 — time-relative rules via record-change date-EQUALITY. A start-node * trigger condition like `end_date == daysFromNow(60)` on a `record-*` trigger * only fires if the record happens to be written on that exact day; the robust @@ -20,6 +26,14 @@ export interface FlowLintFinding { message: string; hint: string; rule: string; + /** + * `'error'` FAILS the build; `'warning'` (the default when absent) prints and + * continues. Most rules here flag a technically-legal footgun and stay + * advisory. A rule is only promoted to `'error'` when the shape it flags is a + * *guaranteed* runtime failure — then a warning would just be a slower way of + * finding out (#3760). + */ + severity?: 'error' | 'warning'; } type AnyRec = Record; @@ -53,7 +67,12 @@ export const FLOW_BARE_DOLLAR_REF = 'flow-bare-dollar-reference'; export const FLOW_APPROVAL_REVISE_DEAD_END = 'flow-approval-revise-dead-end'; export const FLOW_APPROVAL_REVISE_UNMARKED_BACKEDGE = 'flow-approval-revise-unmarked-backedge'; export const FLOW_APPROVAL_REVISE_DISABLED = 'flow-approval-revise-disabled'; -export const FLOW_SCHEDULE_RUNAS_UNSCOPED = 'flow-schedule-runas-unscoped'; +/** + * #3760 — renamed from `flow-schedule-runas-unscoped`. The old id named the + * *schedule*, which was never the boundary: the rule is about a trigger that + * resolves NO USER, and a schedule is only the most obvious such trigger. + */ +export const FLOW_RUNAS_UNSCOPED = 'flow-runas-unscoped'; /** Node types that perform a data operation — the ones `flow.runAs` governs (#1888). */ const DATA_NODE_TYPES = new Set(['get_record', 'create_record', 'update_record', 'delete_record']); @@ -69,6 +88,28 @@ function isScheduleTriggered(flow: AnyRec, startCfg: AnyRec): boolean { return startCfg.schedule != null; } +/** + * The trigger shapes that PROVABLY resolve no trigger user, with a human label + * for the diagnostic (ADR-0073 D5, #3760). `null` when the flow's trigger either + * supplies a user (`screen`) or may or may not, depending on who made the + * triggering write (`record_change`, `autolaunched`) — those are not decidable + * here and are caught at run time instead. + * + * Each entry is grounded in the trigger's own dispatch code, all of which build + * an `AutomationContext` with no `userId` field at all: + * - schedule — `trigger-schedule/src/schedule-trigger.ts` + * - time_relative — `trigger-schedule/src/time-relative-trigger.ts` + * - api — `trigger-api/src/api-trigger.ts` (webhook / queue) + */ +function userLessTriggerKind(flow: AnyRec, startCfg: AnyRec): string | null { + if (isScheduleTriggered(flow, startCfg)) return 'schedule'; + if (startCfg.timeRelative != null) return 'time-relative'; + if (typeof startCfg.triggerType === 'string' && startCfg.triggerType === 'time_relative') return 'time-relative'; + if (flow.type === 'api') return 'api'; + if (typeof startCfg.triggerType === 'string' && startCfg.triggerType === 'api') return 'api'; + return null; +} + /** * Node-config keys that name a capability the automation engine does NOT have. * There is no aggregate node, so a `script`/`loop`/… node carrying these keys is @@ -297,31 +338,38 @@ export function lintFlowPatterns(stack: AnyRec): FlowLintFinding[] { } } - // (a4) #1888 / ADR-0049 — a SCHEDULE-triggered flow has no trigger user at - // runtime, so an effective `runAs:'user'` (explicit, or unset → the spec - // default 'user') run executes its data nodes UNSCOPED (elevated, - // RLS-bypassing) rather than restricted — the data security middleware - // skips when there is no identity. An author who left `runAs` at the - // default expecting a restricted run gets a fail-open one. Only flagged - // when the flow actually performs a data operation (otherwise runAs is - // moot). The robust shape is an explicit `runAs:'system'`, which makes - // the elevation intentional + audit-attributable; a schedule cannot scope - // to a user because there is none. + // (a4) #1888 / ADR-0049 / ADR-0073 D5 — a trigger that resolves NO USER at + // runtime (schedule, time-relative, api/webhook/queue) combined with an + // effective `runAs:'user'` (explicit, or unset → the spec default) is a + // CONFIGURATION ERROR: there is no user to scope to. Since #3760 the + // runtime REFUSES the data operation rather than running it unscoped, so + // this shape is a guaranteed run-time failure — which is why it fails the + // build instead of warning. Only flagged when the flow actually performs + // a data operation (otherwise `runAs` is moot and the run is fine). + // + // This rule is necessary but NOT sufficient, and deliberately so: a + // record-change flow fired by a write that carried no user hits exactly + // the same refusal, but whether a given write carries a user is not + // knowable at authoring time. That case is caught at run time only + // (#3760) — do not try to approximate it here. const runAs = typeof flow.runAs === 'string' ? flow.runAs : 'user'; - if (isScheduleTriggered(flow, startCfg) && runAs !== 'system') { + const userLessKind = userLessTriggerKind(flow, startCfg); + if (userLessKind && runAs !== 'system') { const dataNode = nodes.find((n) => DATA_NODE_TYPES.has(typeof n.type === 'string' ? (n.type as string) : '')); if (dataNode) { const declared = typeof flow.runAs === 'string' ? `\`runAs:'${runAs}'\`` : `the default \`runAs:'user'\``; findings.push({ where: `flow '${flowName}' · runAs`, message: - `schedule-triggered flow runs as ${declared}, but a scheduled run has no trigger user — so its ` + - `data node '${dataNode.id}' (${dataNode.type}) executes UNSCOPED (elevated, RLS-bypassing), not ` + - `restricted to a user.`, + `${userLessKind}-triggered flow runs as ${declared}, but a ${userLessKind} run has no trigger ` + + `user — so its data node '${dataNode.id}' (${dataNode.type}) has no identity to scope to and ` + + `will be REFUSED at run time.`, hint: `Declare \`runAs:'system'\` to make the elevation explicit and intended (the run reads/writes ` + - `every record). A scheduled flow cannot scope to a user — there is none. (ADR-0049, #1888)`, - rule: FLOW_SCHEDULE_RUNAS_UNSCOPED, + `every record). A ${userLessKind} flow cannot scope to a user — there is none. ` + + `(ADR-0049, ADR-0073 D5, #1888, #3760)`, + rule: FLOW_RUNAS_UNSCOPED, + severity: 'error', }); } } diff --git a/packages/metadata-protocol/src/seed-loader-deferred-failure.test.ts b/packages/metadata-protocol/src/seed-loader-deferred-failure.test.ts index 81d9b7886e..bcce90aa71 100644 --- a/packages/metadata-protocol/src/seed-loader-deferred-failure.test.ts +++ b/packages/metadata-protocol/src/seed-loader-deferred-failure.test.ts @@ -122,6 +122,32 @@ const SEEDS = [ }, ] as any[]; +/** + * #3760 — the pass-2 back-fill is still SEEDING, so it must carry `skipTriggers` + * like every other seed write. It used to inline a bare `{ isSystem: true }`, + * and `isSystem` does NOT suppress record-change dispatch — only `skipTriggers` + * does. So the forward-reference patch pass re-fired "on update" automation over + * freshly seeded business rows: the exact self-trigger vector SEED_OPTIONS + * exists to prevent, on the one write that skipped it. + */ +describe('the deferred back-fill seeds with automation suppressed (#3760)', () => { + it("pass-2's reference update carries skipTriggers, like every other seed write", async () => { + const { engine } = createFaithfulEngine(); + const metadata = createMetadata(); + + await new SeedLoaderService(engine, metadata, createLogger()).load({ seeds: SEEDS, config: CONFIG }); + + const deferredUpdates = (engine.update as any).mock.calls.filter( + ([obj]: [string]) => obj === 'audit_department', + ); + expect(deferredUpdates.length, 'the pass-2 back-fill did not run').toBeGreaterThan(0); + for (const [, , opts] of deferredUpdates) { + expect(opts?.context?.skipTriggers, 'a seed write fired record-change automation').toBe(true); + expect(opts?.context?.isSystem).toBe(true); + } + }); +}); + describe('seed deferred back-fill failure is reported, not swallowed (framework#2805)', () => { it('a failing pass-2 reference update flips success=false and counts an error', async () => { const { engine, store } = createFaithfulEngine(); diff --git a/packages/metadata-protocol/src/seed-loader.ts b/packages/metadata-protocol/src/seed-loader.ts index 1303128a87..dcdfb765f2 100644 --- a/packages/metadata-protocol/src/seed-loader.ts +++ b/packages/metadata-protocol/src/seed-loader.ts @@ -785,10 +785,16 @@ export class SeedLoaderService implements ISeedLoaderService { if (recordId) { try { + // Use SEED_OPTIONS like every other seed write: this pass is still + // seeding, so it must carry `skipTriggers` too. Inlining a bare + // `{ isSystem: true }` here re-fired record-change automation on + // freshly seeded rows — `isSystem` does NOT suppress trigger + // dispatch, only `skipTriggers` does — which is exactly the + // self-trigger vector SEED_OPTIONS exists to prevent (#3760). await withTransientRetry(() => this.engine.update(deferred.objectName, { id: recordId, [deferred.field]: resolvedId, - }, { context: { isSystem: true } } as any)); + }, SeedLoaderService.SEED_OPTIONS as any)); // Update result stats const resultEntry = allResults.find(r => r.object === deferred.objectName); diff --git a/packages/plugins/plugin-approvals/src/record-lock-schedule-run.integration.test.ts b/packages/plugins/plugin-approvals/src/record-lock-schedule-run.integration.test.ts index 8b9bba0009..2ed2753614 100644 --- a/packages/plugins/plugin-approvals/src/record-lock-schedule-run.integration.test.ts +++ b/packages/plugins/plugin-approvals/src/record-lock-schedule-run.integration.test.ts @@ -1,17 +1,24 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * #3712 — a schedule-triggered run may write its own locked record. + * #3703 / #3712 / #3760 — the run that opened a pending approval may write its + * own locked record. * * #3703 exempted "the run that opened the pending request" from the approvals - * record lock, keyed on `flowRunId`. It worked for every run that resolved an - * identity and silently missed the one that doesn't: an effective - * `runAs:'user'` run with no trigger user — a schedule — passed NO ObjectQL - * context at all, so nothing carried the run id and the run still died on its - * own `RECORD_LOCKED`. + * record lock, keyed on `flowRunId`. #3712 extended that to the run that + * resolved no identity — an effective `runAs:'user'` run with no trigger user — + * by giving it a provenance-only ObjectQL context carrying just the run id. * - * That miss was a HAND-OFF gap, not a logic gap: every hop worked in isolation. - * So this test refuses to stub any hop. It runs the real + * #3760 then closed the fail-open that path depended on: a run with no trigger + * user may no longer perform a data operation at all, because presenting no + * principal is precisely what made the write UNSCOPED. So the user-less variant + * is now REFUSED rather than exempted, and a schedule reaches its own record the + * explicit way — `runAs:'system'`, which the hook exempts on its own + * `isSystem` branch. The `flowRunId` exemption remains live and load-bearing for + * what it was built for: a `runAs:'user'` run that DOES have a user. + * + * The original miss was a HAND-OFF gap, not a logic gap: every hop worked in + * isolation. So this test still refuses to stub any hop. It runs the real * `resolveRunDataContext` from the automation runtime, feeds its output to a * real {@link ObjectQL} engine, and lets the real lock hook decide — the same * three layers, in the same order, as a live deployment. @@ -99,10 +106,20 @@ function makeMemoryDriver() { return driver; } -/** The run context a schedule trigger produces: an event, and no user. */ -const SCHEDULE_RUN = (flowRunId: string) => ({ runAs: 'user' as const, flowRunId }); +/** + * A `runAs:'user'` run that HAS a trigger user — the live consumer of the + * `flowRunId` exemption, and the shape #3703 built it for (a record-change or + * screen flow that opened the approval and now writes its own target record). + */ +const OWNING_RUN = (flowRunId: string) => ({ runAs: 'user' as const, userId: 'u1', flowRunId }); + +/** What a schedule trigger produces: an event, and NO user. Refused since #3760. */ +const USER_LESS_RUN = (flowRunId: string) => ({ runAs: 'user' as const, flowRunId }); -describe('a schedule-triggered run and the approvals record lock (#3712)', () => { +/** The supported shape for a schedule that must write records (ADR-0049). */ +const SYSTEM_RUN = (flowRunId: string) => ({ runAs: 'system' as const, flowRunId }); + +describe('an owning run and the approvals record lock (#3703 / #3712 / #3760)', () => { let engine: ObjectQL; let oppId: string; @@ -128,22 +145,20 @@ describe('a schedule-triggered run and the approvals record lock (#3712)', () => const writeAs = (context: unknown) => engine.update('opportunity', { id: oppId, amount: 200 }, { context } as any); - it('lets the OWNING schedule run write its own target record', async () => { + it('lets the OWNING run write its own target record', async () => { // The full hand-off, unstubbed: the automation runtime resolves the run's // ObjectQL context, the engine turns it into hook provenance, the lock hook // matches it against the pending request it opened. - const dataCtx = resolveRunDataContext(SCHEDULE_RUN('run_1')); - expect(dataCtx, 'the run resolved no context — nothing could carry the run id').toEqual({ - flowRunId: 'run_1', - }); + const dataCtx = resolveRunDataContext(OWNING_RUN('run_1')); + expect(dataCtx, 'nothing could carry the run id').toMatchObject({ flowRunId: 'run_1' }); await expect(writeAs(dataCtx)).resolves.toBeDefined(); const row = await engine.findOne('opportunity', { where: { id: oppId } }); expect(row.amount).toBe(200); }); - it('still blocks a DIFFERENT schedule run', async () => { - await expect(writeAs(resolveRunDataContext(SCHEDULE_RUN('run_other')))) + it('still blocks a DIFFERENT run', async () => { + await expect(writeAs(resolveRunDataContext(OWNING_RUN('run_other')))) .rejects.toThrow(/RECORD_LOCKED/); }); @@ -156,13 +171,36 @@ describe('a schedule-triggered run and the approvals record lock (#3712)', () => await expect(writeAs(undefined)).rejects.toThrow(/RECORD_LOCKED/); }); - it('the exempted write presents NO principal — the lock was opened by provenance, not privilege', async () => { - // The exemption must not have been bought with elevation. Nothing the - // security middleware keys on is present, so the run's authorization is the - // same unscoped #1888 posture it had before this fix. - const dataCtx = resolveRunDataContext(SCHEDULE_RUN('run_1')) as Record; - for (const key of ['isSystem', 'userId', 'positions', 'permissions', 'tenantId']) { - expect(dataCtx, `the exemption rode in on '${key}'`).not.toHaveProperty(key); - } + it('the exemption is PROVENANCE, not privilege — the exempted write is not elevated', async () => { + // The exemption must not have been bought with elevation: the run that + // writes its own locked record is still a plain `runAs:'user'` principal, + // subject to the same RLS as the user who triggered it. Only `flowRunId` + // distinguishes it, and `flowRunId` grants nothing on its own. + const dataCtx = resolveRunDataContext(OWNING_RUN('run_1')) as Record; + expect(dataCtx.isSystem, 'the exemption rode in on isSystem').toBe(false); + expect(dataCtx.flowRunId).toBe('run_1'); + expect(dataCtx.userId).toBe('u1'); + }); + + // #3760 — the case #3712 solved by handing the lock a provenance-only context + // is gone at the root: such a run may not perform a data operation at all, + // because presenting no principal is exactly what made the write unscoped. It + // is refused BEFORE the lock is ever consulted. + it('a USER-LESS run never reaches the lock — it cannot perform a data op at all', async () => { + expect(() => resolveRunDataContext(USER_LESS_RUN('run_1'))) + .toThrow(/no trigger user could be resolved/); + }); + + // ...and the capability itself survives, via the explicit route: a schedule + // that must write records declares `runAs:'system'`, which the lock hook + // exempts on its own isSystem branch. Elevation is now declared rather than + // acquired by having no identity. + it("a schedule that declares runAs:'system' still writes its own target record", async () => { + const dataCtx = resolveRunDataContext(SYSTEM_RUN('run_1')); + expect(dataCtx).toMatchObject({ isSystem: true, flowRunId: 'run_1' }); + + await expect(writeAs(dataCtx)).resolves.toBeDefined(); + const row = await engine.findOne('opportunity', { where: { id: oppId } }); + expect(row.amount).toBe(200); }); }); diff --git a/packages/qa/dogfood/test/flow-runas-schedule.dogfood.test.ts b/packages/qa/dogfood/test/flow-runas-schedule.dogfood.test.ts index 40fd73ee84..eeb98aaf5a 100644 --- a/packages/qa/dogfood/test/flow-runas-schedule.dogfood.test.ts +++ b/packages/qa/dogfood/test/flow-runas-schedule.dogfood.test.ts @@ -1,29 +1,37 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. // -// FLOW runAs — the SCHEDULE (user-less) fail-open, exercised end-to-end through -// the real automation + security + data stack (#1888 follow-up, ADR-0049). +// FLOW runAs — the user-less run is REFUSED, exercised end-to-end through the +// real automation + security + data stack (#1888 → #3760, ADR-0049/ADR-0073). // // @proof: flow-runas-schedule // Sibling of flow-runas.dogfood.test.ts. That gate proves runAs switches identity -// for a USER-triggered run; this one pins the boundary case #1888 deliberately -// left open: a SCHEDULE-triggered run has NO trigger user, so an effective -// `runAs:'user'` (the default) resolves no identity → CRUD nodes pass no ObjectQL -// context → the security middleware SKIPS (it delegates auth to the auth layer) -// → the run executes UNSCOPED (effectively elevated), not restricted. +// for a USER-triggered run; this one pins the boundary case: a run with NO +// trigger user under an effective `runAs:'user'` (the default) resolves no +// identity, so its CRUD nodes would present no ObjectQL context → the security +// middleware SKIPS (it delegates auth to the auth layer) → the run would execute +// UNSCOPED (effectively elevated), not restricted. +// +// Until #3760 that is exactly what happened, and THIS FILE PINNED IT — asserting +// that a user-less run read and wrote an admin-owned record a member cannot +// touch. Its own note said a fail-closed change must turn these assertions RED +// and "force the product decision to be revisited deliberately". That is what +// #3760 did: `runAs:'user'` is an access-NARROWING declaration, so failing to +// resolve it must never resolve to a grant, and the operation is refused. +// +// A schedule was never the boundary — it is simply the most obvious user-less +// trigger. The commonest is a record-change flow fired by a write that carried +// no user (`isSystem` does not suppress trigger dispatch). The assertions below +// hold for every one of them; the schedule-shaped context is just the cheapest +// to drive here. // // We reuse the owner-isolated runas_note fixture and drive the flows directly // through the automation service with a USER-LESS context — exactly the shape the // schedule trigger builds ({ event:'schedule', params }, no userId) — proving: -// • runAs:'user' (user-less) → UNSCOPED: reads + writes the admin's note a -// member cannot touch, and the engine emits the [runAs] warning (the -// fail-open is now AUDIBLE, not silent); -// • runAs:'system'(user-less) → the explicit, attributable elevation (same -// access) with NO warning — the fix authors should declare. -// -// This is the live, revert-provable form of "passes static checks / silently -// elevated at runtime": if a future change makes the user-less case fail-closed -// (deny) or auto-elevate, the behavioral assertions here go RED and force the -// product decision to be revisited deliberately. +// • runAs:'user' (user-less) → REFUSED: the run fails and the admin's note is +// neither read nor written, with the engine's [runAs] warning emitted at run +// setup so the refusal is diagnosable; +// • runAs:'system'(user-less) → the explicit, attributable elevation, still +// working, with NO warning — the declaration authors should make. import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; import { bootStack, type VerifyStack } from '@objectstack/verify'; @@ -57,7 +65,7 @@ function captureRunAsWarnings() { }; } -describe('objectstack verify FLOW: schedule/user-less runAs fail-open (#flow-runas-schedule)', () => { +describe('objectstack verify FLOW: user-less runAs is refused (#flow-runas-schedule)', () => { let stack: VerifyStack; let adminToken: string; let memberToken: string; @@ -107,28 +115,38 @@ describe('objectstack verify FLOW: schedule/user-less runAs fail-open (#flow-run } }); - it("FAIL-OPEN (pinned): a user-less runAs:'user' run executes UNSCOPED — reads + writes the admin note, audibly", async () => { + it("FAIL-CLOSED (pinned): a user-less runAs:'user' run is REFUSED — never reads or writes the admin note", async () => { const id = await adminCreateNote('sched-user'); const warns = captureRunAsWarnings(); try { - // READ: user mode + no user → unscoped → finds the admin's note (a member can't). + // READ: user mode + no user → refused. Before #3760 this returned the + // admin's note, which a member cannot read. const read = await automation.execute('runas_user_read', scheduleContext(id)); - expect(read.success, `read run not successful: ${JSON.stringify(read)}`).toBe(true); + expect( + read.success, + 'a user-less user-mode run READ successfully — the #3760 fail-open is back (ADR-0049/#1888)', + ).toBe(false); const found = read.output?.found; expect( found && typeof found === 'object' ? (found as any).id : found, - 'a user-less user-mode run did NOT read unscoped — the fail-open behavior changed; revisit the product decision (ADR-0049/#1888)', - ).toBeTruthy(); + 'a user-less user-mode run read a record unscoped', + ).toBeFalsy(); - // WRITE: user mode + no user → unscoped → stamps the admin's note. + // WRITE: user mode + no user → refused, and — the assertion that actually + // matters — the admin's record is UNCHANGED. Before #3760 it read + // 'touched-user' here. const write = await automation.execute('runas_user_touch', scheduleContext(id)); - expect(write.success, `write run not successful: ${JSON.stringify(write)}`).toBe(true); - expect(await adminStatusOf(id)).toBe('touched-user'); + expect(write.success, 'a user-less user-mode run WROTE successfully — the fail-open is back').toBe(false); + expect( + await adminStatusOf(id), + "a user-less run stamped the admin's note — it wrote unscoped", + ).toBe('new'); - // ...and the fail-open is AUDIBLE — the engine warned (≥1 across the two runs). + // ...and the refusal is DIAGNOSABLE — the engine warned at run setup, + // before any node executed (≥1 across the two runs). expect( warns.count(), - 'expected the engine to warn that a user-less user-mode run executes unscoped', + 'expected the engine to warn that a user-less user-mode run will be refused', ).toBeGreaterThanOrEqual(1); } finally { warns.restore(); diff --git a/packages/runtime/src/app-plugin.ts b/packages/runtime/src/app-plugin.ts index 5a5397bb8c..ee2d52db05 100644 --- a/packages/runtime/src/app-plugin.ts +++ b/packages/runtime/src/app-plugin.ts @@ -12,6 +12,18 @@ import { QuickJSScriptRunner } from './sandbox/quickjs-runner.js'; import { hookBodyRunnerFactory, actionBodyRunnerFactory } from './sandbox/body-runner.js'; import { countServerTiming } from '@objectstack/observability'; +/** + * The write options every seed insert must use — mirrors + * `SeedLoaderService.SEED_OPTIONS`. `skipTriggers` is the load-bearing part: + * seed rows are pre-existing end-state data, not user events, so firing + * "on create" automation for them is semantically wrong and was the vector for + * a self-trigger loop that wedged first boot. `isSystem` alone does NOT suppress + * dispatch — only `skipTriggers` does — so the two basic-insert fallbacks below + * used to seed with automation live while the main path had it suppressed + * (#3760). + */ +const SEED_WRITE_OPTIONS = { context: { isSystem: true, skipTriggers: true, seedReplay: true } } as const; + /** * Optional per-project context attached when AppPlugin is instantiated by the * project kernel factory. Required for the `app:registered` / `app:unregistered` @@ -901,7 +913,7 @@ export class AppPlugin implements Plugin { ctx.logger.info(`[Seeder] Seeding ${dataset.records.length} records for ${dataset.object}`); for (const record of dataset.records) { try { - await ql.insert(dataset.object, record, { context: { isSystem: true } } as any); + await ql.insert(dataset.object, record, SEED_WRITE_OPTIONS as any); } catch (err: any) { ctx.logger.warn(`[Seeder] Failed to insert ${dataset.object} record:`, { error: err.message }); } @@ -915,7 +927,7 @@ export class AppPlugin implements Plugin { for (const dataset of normalizedDatasets) { for (const record of dataset.records) { try { - await ql.insert(dataset.object, record, { context: { isSystem: true } } as any); + await ql.insert(dataset.object, record, SEED_WRITE_OPTIONS as any); } catch (insertErr: any) { ctx.logger.warn(`[Seeder] Failed to insert ${dataset.object} record:`, { error: insertErr.message }); } diff --git a/packages/services/service-automation/src/builtin/crud-config-aliases.test.ts b/packages/services/service-automation/src/builtin/crud-config-aliases.test.ts index bcc9bb25f9..e3d2e9739c 100644 --- a/packages/services/service-automation/src/builtin/crud-config-aliases.test.ts +++ b/packages/services/service-automation/src/builtin/crud-config-aliases.test.ts @@ -12,6 +12,10 @@ * raw `filters` key reaching the executor directly (a flow that skipped the * load seam) is no longer honored, and the executor emits no alias warning * for it. This test documents that split — the PD #12 retirement path. + * + * Each run below passes a `userId`: a data-touching flow left at the spec + * default `runAs:'user'` is refused without a trigger user (#3760). Incidental + * to what these tests assert — supplied so the run reaches the CRUD node. */ import { describe, it, expect, beforeEach } from 'vitest'; import { normalizeStackInput } from '@objectstack/spec'; @@ -74,7 +78,7 @@ describe('CRUD config-key aliases: object→objectName (executor shim) + filters // Canonical `filter`; deprecated `object`. engine.registerFlow('gr', getRecordFlow({ object: 'crm_lead', filter: { id: 'L1' }, outputVariable: 'lead' })); - const res = await engine.execute('gr'); + const res = await engine.execute('gr', { userId: 'u1' }); expect(res.success).toBe(true); expect(calls).toHaveLength(1); @@ -86,7 +90,7 @@ describe('CRUD config-key aliases: object→objectName (executor shim) + filters // One-time per alias: a second run does not warn again. const before = warns.length; - await engine.execute('gr'); + await engine.execute('gr', { userId: 'u1' }); expect(warns.length).toBe(before); }); @@ -102,7 +106,7 @@ describe('CRUD config-key aliases: object→objectName (executor shim) + filters registerCrudNodes(engine, ctxWith(data, silentLogger())); engine.registerFlow('gr', getRecordFlow({ objectName: 'crm_lead', filters: { id: 'L1' } })); - await engine.execute('gr'); + await engine.execute('gr', { userId: 'u1' }); expect(calls[0].opts.where).toEqual({ id: 'L1' }); // filter preserved, not dropped }); @@ -115,7 +119,7 @@ describe('CRUD config-key aliases: object→objectName (executor shim) + filters const raw = getRecordFlow({ objectName: 'crm_lead', filters: { id: 'L1' }, outputVariable: 'lead' }); const converted = (normalizeStackInput({ flows: [raw] }).flows as any[])[0]; engine.registerFlow('gr', converted); - await engine.execute('gr'); + await engine.execute('gr', { userId: 'u1' }); expect(calls[0].opts.where).toEqual({ id: 'L1' }); }); @@ -127,7 +131,7 @@ describe('CRUD config-key aliases: object→objectName (executor shim) + filters registerCrudNodes(engine, ctxWith(data, collectingLogger(warns))); engine.registerFlow('gr', getRecordFlow({ objectName: 'crm_lead', filter: { id: 'L1' }, outputVariable: 'lead' })); - const res = await engine.execute('gr'); + const res = await engine.execute('gr', { userId: 'u1' }); expect(res.success).toBe(true); expect(warns).toHaveLength(0); diff --git a/packages/services/service-automation/src/builtin/crud-output-var.test.ts b/packages/services/service-automation/src/builtin/crud-output-var.test.ts index 097a48158e..a804261360 100644 --- a/packages/services/service-automation/src/builtin/crud-output-var.test.ts +++ b/packages/services/service-automation/src/builtin/crud-output-var.test.ts @@ -51,7 +51,10 @@ describe('create_record outputVariable (#1873)', () => { ], } as any); - const res = await engine.execute('promote'); + // A trigger user is required for a data-touching run left at the default + // `runAs:'user'` — a user-less one is refused (#3760). Unrelated to what this + // test asserts; supplied so the run reaches the CRUD node at all. + const res = await engine.execute('promote', { userId: 'u1' }); expect(res.success).toBe(true); expect(updates).toHaveLength(1); expect(updates[0].fields.promoted_topic).toBe('topic_1'); @@ -75,7 +78,7 @@ describe('create_record outputVariable (#1873)', () => { { id: 'e3', source: 'upd', target: 'end' }, ], } as any); - const res = await engine.execute('promote2'); + const res = await engine.execute('promote2', { userId: 'u1' }); expect(res.success).toBe(true); expect(updates[0].fields.ref).toBe('X'); }); diff --git a/packages/services/service-automation/src/builtin/crud-runas.test.ts b/packages/services/service-automation/src/builtin/crud-runas.test.ts index 516e595442..eaaa9ff77d 100644 --- a/packages/services/service-automation/src/builtin/crud-runas.test.ts +++ b/packages/services/service-automation/src/builtin/crud-runas.test.ts @@ -14,7 +14,12 @@ import { describe, it, expect } from 'vitest'; import { AutomationEngine } from '../engine.js'; import { registerCrudNodes } from './crud-nodes.js'; -import { resolveRunDataContext, runIsUnscopedUserMode, flowTouchesData } from '../runtime-identity.js'; +import { + resolveRunDataContext, + runIsUnscopedUserMode, + flowTouchesData, + UnscopedRunDataAccessError, +} from '../runtime-identity.js'; import type { AutomationContext } from '@objectstack/spec/contracts'; function makeLogger(): any { @@ -188,48 +193,66 @@ describe('resolveRunDataContext (#1888 unit)', () => { }); }); - it('returns undefined for a user-mode run with no user AND no run id', () => { - expect(resolveRunDataContext({ runAs: 'user' })).toBeUndefined(); - expect(resolveRunDataContext(undefined)).toBeUndefined(); + // #3760 — a user-mode run with NO user is REFUSED, not resolved. Previously + // this returned `undefined` (or, after #3712, a provenance-only envelope) and + // let the op proceed with no principal — which the data security middleware + // waves straight through, running it UNSCOPED. That was the #1888 fail-open. + it('THROWS for a user-mode run with no user (with or without a run id)', () => { + for (const ctx of [{ runAs: 'user' as const }, { runAs: 'user' as const, flowRunId: 'run_1' }, undefined]) { + expect(() => resolveRunDataContext(ctx)).toThrow(UnscopedRunDataAccessError); + } }); - // #3712 — a user-mode run with no user still HAS a run. It carries the run id - // and nothing else, so it becomes attributable without acquiring a principal. - it('maps a user-less run WITH a run id to a provenance-only context', () => { - const ctx = resolveRunDataContext({ runAs: 'user', flowRunId: 'run_1' }); - expect(ctx).toEqual({ flowRunId: 'run_1' }); - // Exhaustive on purpose: every field below is one the security middleware - // keys on. Any of them appearing here would change the run's authorization, - // which this fix must not do — the #1888 unscoped posture is unchanged. - expect(Object.keys(ctx!)).toEqual(['flowRunId']); - for (const key of ['isSystem', 'userId', 'positions', 'permissions', 'tenantId']) { - expect(ctx, `provenance-only context leaked '${key}' — it now presents a principal`) - .not.toHaveProperty(key); + it('the refusal names the fix, so the author can act on it without reading the source', () => { + let err: Error | undefined; + try { + resolveRunDataContext({ runAs: 'user', object: 'invoice', event: 'record-after-update', flowRunId: 'run_1' }); + } catch (e) { + err = e as Error; } + expect(err).toBeInstanceOf(UnscopedRunDataAccessError); + expect((err as UnscopedRunDataAccessError & { code: string }).code).toBe('AUTOMATION_UNSCOPED_RUN_DATA_ACCESS'); + expect(err!.message).toMatch(/runAs: 'system'/); + expect(err!.message).toMatch(/UNSCOPED/); + // Names WHERE, so a refusal in a busy log is traceable to a flow + record. + expect(err!.message).toContain("object 'invoice'"); + expect(err!.message).toContain("run 'run_1'"); + }); + + // The refusal is deliberately NOT an elevation to isSystem. The security + // middleware's isSystem short-circuit fires BEFORE its package-managed-row, + // system-row, audience-anchor and delegated-admin gates, so re-badging these + // runs as system would GRANT them powers a principal-less run never had. + it("refuses rather than elevating — runAs:'system' stays the only route to isSystem", () => { + expect(() => resolveRunDataContext({ runAs: 'user', flowRunId: 'r' })).toThrow(); + expect(resolveRunDataContext({ runAs: 'system', flowRunId: 'r' })).toEqual({ + isSystem: true, positions: [], permissions: [], flowRunId: 'r', + }); }); }); /** - * #1888 FOLLOW-UP — the user-less fail-open. A schedule-triggered run carries no - * trigger user, so an effective `runAs:'user'` (the default) resolves no identity - * → CRUD nodes present no principal → the data security middleware skips → the - * run executes UNSCOPED (effectively elevated). Denying would break legitimate - * scheduled CRUD and silently elevating would hide the author's intent, so the - * engine keeps the run working but makes the fail-open AUDIBLE: one clear warning - * per run, recommending `runAs:'system'` (ADR-0049). These tests pin both the - * (unchanged, non-breaking) data behavior AND the new warning. + * #1888 FOLLOW-UP, CLOSED BY #3760 — the user-less fail-open. A run with no + * trigger user and an effective `runAs:'user'` (the default) resolves no + * identity → CRUD nodes present no principal → the data security middleware + * skips → the run used to execute UNSCOPED (effectively elevated). + * + * #2308 made that AUDIBLE (a warning) but left it working, on the grounds that + * denying would break legitimate scheduled CRUD. That rationale expired: the + * example flows it protected now declare `runAs:'system'`, and #3760 showed the + * dominant shape is not a schedule at all but a record-change flow fired by a + * system write — reachable by ordinary users and not decidable at authoring + * time. So the run is now REFUSED. These tests pin the refusal, the warning that + * precedes it, and the fact that refusing is NOT elevating. */ /** - * The op ran with NO principal — the #1888 unscoped posture. Since #3712 such a - * run DOES carry a context, but a provenance-only one: the run id and nothing - * the security middleware keys on. Asserting "no context at all" would pin the - * transport instead of the property that matters. + * The op must never have reached the data engine at all. A refused run produces + * NO calls — asserting on a captured context would be vacuously true once the + * throw lands, so the meaningful assertion is that nothing was dispatched. */ -function expectUnscoped(ctx: any, op: string): void { - for (const key of ['isSystem', 'userId', 'positions', 'permissions', 'tenantId']) { - expect(ctx?.[key], `${op} should be unscoped — it presented '${key}'`).toBeUndefined(); - } +function expectNoDataOps(calls: Array<{ op: string }>, why: string): void { + expect(calls.map((c) => c.op), why).toEqual([]); } function recordingLogger(): { logger: any; warns: string[] } { @@ -240,34 +263,54 @@ function recordingLogger(): { logger: any; warns: string[] } { } const runAsWarns = (warns: string[]) => warns.filter((w) => w.includes('[runAs]')); -describe('schedule/user-less runs surface the unscoped fail-open (#1888 follow-up)', () => { - it('warns ONCE when a user-mode run has no trigger user and the flow touches data', async () => { +describe('user-less runs are refused, not run unscoped (#1888 / #3760)', () => { + it('REFUSES every data op when a user-mode run has no trigger user, and warns once first', async () => { const { logger, warns } = recordingLogger(); const engine = new AutomationEngine(logger); const { data, calls } = fakeData(); registerCrudNodes(engine, ctxWith(data)); engine.registerFlow('sched', allOpsFlow('sched')); // no runAs → default 'user' - // Simulate a schedule trigger's context: an event, but NO userId. + // Simulate a user-less trigger's context: an event, but NO userId. const res = await engine.execute('sched', { event: 'schedule', params: { jobId: 'j1' } }); - expect(res.success).toBe(true); - // Non-breaking: the run still executes, and every data op is UNSCOPED — it - // presents no principal, only its own run id (#3712). - expect(calls.length).toBeGreaterThan(0); - for (const c of calls) { - expectUnscoped(c.ctx, c.op); - expect(c.ctx?.flowRunId, `${c.op} carried no run provenance`).toBeTruthy(); - } + // The whole point: NOTHING reached the data engine. Before #3760 every op + // here ran, each one unscoped. + expectNoDataOps(calls, 'a user-less run must not reach the data engine at all'); + expect(res.success, 'the run must fail rather than silently do nothing').toBe(false); - // ...and the fail-open is AUDIBLE: exactly one runAs warning, naming the flow + the fix. + // ...and it was diagnosable BEFORE the failure: exactly one runAs warning at + // run setup, naming the flow + the fix. const w = runAsWarns(warns); expect(w).toHaveLength(1); expect(w[0]).toContain("flow 'sched'"); - expect(w[0]).toMatch(/UNSCOPED/); + expect(w[0]).toMatch(/REFUSED/); expect(w[0]).toMatch(/runAs:'system'/); }); + // The regression that motivated #3760: this is NOT a schedule. It is the + // ordinary record-change shape, fired by a write that carried no user (any + // isSystem plugin/service write — `isSystem` does not suppress dispatch). + // Nothing flags it at authoring time, so the runtime refusal is the only net. + it('refuses a RECORD-CHANGE run fired by a user-less write, exactly like a schedule', async () => { + const { logger, warns } = recordingLogger(); + const engine = new AutomationEngine(logger); + const { data, calls } = fakeData(); + registerCrudNodes(engine, ctxWith(data)); + engine.registerFlow('on_change', allOpsFlow('on_change')); // default runAs:'user' + + const res = await engine.execute('on_change', { + event: 'record-after-update', + object: 'invoice', + record: { id: 'inv_1' }, + // no userId — the triggering write was a system write + }); + + expectNoDataOps(calls, 'a record-change run with no trigger user must be refused too'); + expect(res.success).toBe(false); + expect(runAsWarns(warns)).toHaveLength(1); + }); + it("does NOT warn when a user-less run declares runAs:'system' (explicit elevation)", async () => { const { logger, warns } = recordingLogger(); const engine = new AutomationEngine(logger); @@ -397,16 +440,21 @@ describe("runAs:'user' resolves the triggering user's grants at run setup (#3356 for (const c of calls) expect(c.ctx.isSystem).toBe(true); }); - it('does NOT resolve when there is no trigger user (stays the unscoped fail-open)', async () => { + it('does NOT resolve when there is no trigger user — the run is refused instead (#3760)', async () => { const engine = new AutomationEngine(makeLogger()); const { data, calls } = fakeData(); registerCrudNodes(engine, ctxWith(data)); engine.registerFlow('sched', allOpsFlow('sched')); // default user, no userId let called = 0; engine.setUserGrantsResolver(() => { called++; return { positions: [], permissions: [] }; }); - await engine.execute('sched', { event: 'schedule' }); + const res = await engine.execute('sched', { event: 'schedule' }); + // There is no user to resolve grants FOR, so the resolver is never consulted… expect(called).toBe(0); - for (const c of calls) expectUnscoped(c.ctx, c.op); + // …and the run does not fall through to an unscoped op. (Asserting over + // `calls` alone would pass vacuously now that the list is empty, so pin the + // emptiness AND the failure explicitly.) + expect(calls).toHaveLength(0); + expect(res.success).toBe(false); }); it('fail-safe: a resolver error warns and keeps the bare user — never elevates', async () => { diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index a83506fe27..e3ff5e8fc5 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -1577,15 +1577,16 @@ export class AutomationEngine implements IAutomationService { * (ADR-0049 / #1888). The single construction point shared by `execute()` and * `executeWithoutRetry()`. * - * Also surfaces the user-less **fail-open** footgun (#1888 follow-up): a flow - * whose effective `runAs` is `'user'` but whose trigger carries no user — e.g. - * a schedule-triggered run — has no user to scope to, so its data nodes run - * UNSCOPED (the data security middleware skips when there is no identity). - * Denying would break legitimate scheduled CRUD and silently elevating would - * hide the author's intent, so the run proceeds — but we log a clear warning so - * the elevation is *audible* rather than silent. Authors should declare - * `runAs:'system'` to make scheduled elevation explicit (the build-time lint - * `flow-schedule-runas-unscoped` flags the same shape earlier). + * Also warns about the user-less case (#1888 follow-up, closed by #3760): a + * flow whose effective `runAs` is `'user'` but whose trigger resolved no user + * has no identity to scope to, so its data nodes would run UNSCOPED (the data + * security middleware skips when there is no identity). Those data ops are now + * REFUSED at `resolveRunDataContext`; the warning here fires at run SETUP, + * before any node executes, so the refusal is diagnosable rather than a + * surprise mid-flow. Authors declare `runAs:'system'` to make the elevation + * explicit (the build-time lint `flow-runas-unscoped` rejects the + * statically-decidable shapes earlier — but NOT the record-change shape, which + * is only knowable at run time). */ private async resolveRunContext(flow: FlowParsed, context?: AutomationContext, runId?: string): Promise { // `flowRunId` is stamped alongside `runAs` because it shares that field's @@ -1641,10 +1642,12 @@ export class AutomationEngine implements IAutomationService { if (runIsUnscopedUserMode(runContext) && flowTouchesData(flow)) { this.logger.warn( - `[runAs] flow '${flow.name}' executes with runAs:'user' but its trigger carries no user ` + - `(e.g. a schedule) — its data operations run UNSCOPED (elevated, RLS-bypassing), not ` + - `restricted. Declare runAs:'system' to make the elevation explicit and intended ` + - `(ADR-0049, #1888).`, + `[runAs] flow '${flow.name}' executes with runAs:'user' but its trigger resolved no user ` + + `— its data operations will be REFUSED (#3760). Running them would execute UNSCOPED ` + + `(elevated, RLS-bypassing) rather than restricted, which is the fail-open ADR-0049 ` + + `forbids. Declare runAs:'system' to make the elevation explicit and intended, or arrange ` + + `for the trigger to supply a user. Note a user-less trigger is NOT only a schedule: a ` + + `record-change flow fired by a system write carries no user either (ADR-0049, #1888).`, ); } diff --git a/packages/services/service-automation/src/index.ts b/packages/services/service-automation/src/index.ts index b8bb7db0b2..f93c537dc4 100644 --- a/packages/services/service-automation/src/index.ts +++ b/packages/services/service-automation/src/index.ts @@ -50,8 +50,11 @@ export type { AutomationServicePluginOptions } from './plugin.js'; // Run identity (ADR-0049 / #1888). Maps a flow run's effective `runAs` to the // ObjectQL `context` its data nodes pass — `system` → elevated/RLS-bypassing, -// `user` → the triggering user. Exported for hosts building custom data nodes. -export { resolveRunDataContext } from './runtime-identity.js'; +// `user` → the triggering user. A run that resolves NO principal is refused +// outright (#3760). Exported for hosts building custom data nodes: call +// `resolveRunDataContext` and let the error propagate, so a custom node inherits +// the same posture as the built-ins instead of re-opening the fail-open. +export { resolveRunDataContext, UnscopedRunDataAccessError } from './runtime-identity.js'; export type { RunDataContext, RunIdentityContext, RunProvenanceContext } from './runtime-identity.js'; // Built-in node executors (ADR-0018). These are seeded by AutomationServicePlugin diff --git a/packages/services/service-automation/src/runtime-identity.ts b/packages/services/service-automation/src/runtime-identity.ts index 6a90f6547c..7b1aef7204 100644 --- a/packages/services/service-automation/src/runtime-identity.ts +++ b/packages/services/service-automation/src/runtime-identity.ts @@ -31,9 +31,7 @@ export interface RunIdentityContext { } /** - * The PROVENANCE-ONLY envelope, for a run that resolves NO principal — an - * effective `runAs:'user'` run with no trigger user, a schedule being the - * canonical case (#3712). + * The PROVENANCE-ONLY envelope, for a run that resolves NO principal. * * It names the run that made the write and carries nothing else: no `userId`, * no `positions`, no `permissions`, not even `isSystem: false`. That absence is @@ -43,15 +41,58 @@ export interface RunIdentityContext { * `context.userId`, the empty-principal fall-open on * `positions`/`permissions`/`userId` (and the delegated-admin gate normalizes a * missing context to `{}` before testing it) — so this envelope is - * indistinguishable from passing no context at all. The run's authorization - * stays EXACTLY the documented #1888 unscoped fail-open it was before; only - * provenance rides along. + * indistinguishable from passing no context at all. + * + * Since #3760 a principal-less run may no longer reach a DATA node at all + * ({@link resolveRunDataContext} refuses), so this envelope is no longer the + * carrier of the old #1888 fail-open. It survives for the non-data provenance + * uses that motivated #3712 — a run id is still attributable without presenting + * an identity it does not have. */ export interface RunProvenanceContext { /** The run performing this operation. The whole envelope (#3712). */ flowRunId: string; } +/** + * Thrown when a run whose effective identity is the #1888 *unscoped* case tries + * to perform a data operation (#3760). + * + * The refusal is the point: an effective `runAs:'user'` with no resolvable + * trigger user used to execute its data nodes UNSCOPED — the data security + * middleware skips when there is no principal, so the run read and wrote EVERY + * row of EVERY tenant. `runAs:'user'` is an access-NARROWING declaration, and + * ADR-0049's standing rule is that failing to resolve a narrowing declaration + * must never resolve to a grant. So the operation is refused instead. + * + * Note this is strictly narrower than elevating to `isSystem`: the security + * middleware's `isSystem` short-circuit fires BEFORE the package-managed-row, + * system-row, audience-anchor and delegated-admin gates, so "unscoped" was + * never equivalent to "system" and quietly re-badging these runs as system + * would have WIDENED them. + */ +export class UnscopedRunDataAccessError extends Error { + readonly code = 'AUTOMATION_UNSCOPED_RUN_DATA_ACCESS'; + + constructor(context?: AutomationContext) { + const where = [ + context?.object ? `object '${context.object}'` : undefined, + context?.event ? `event '${context.event}'` : undefined, + context?.flowRunId ? `run '${context.flowRunId}'` : undefined, + ] + .filter(Boolean) + .join(', '); + super( + `[runAs] refusing a data operation${where ? ` (${where})` : ''}: this run's effective runAs is ` + + `'user' but no trigger user could be resolved, so the operation would execute UNSCOPED ` + + `(elevated, RLS-bypassing) rather than restricted to a user. Declare \`runAs: 'system'\` on the ` + + `flow to make the elevation explicit and intended, or arrange for the trigger to supply a user ` + + `(a write made with a system context carries none). (ADR-0049, #1888, #3760)`, + ); + this.name = 'UnscopedRunDataAccessError'; + } +} + /** What a flow's data nodes pass to ObjectQL as `options.context`. */ export type RunDataContext = RunIdentityContext | RunProvenanceContext; @@ -66,15 +107,22 @@ export type RunDataContext = RunIdentityContext | RunProvenanceContext; * enforces that user's row-level security. The run can never exceed the * triggering user's grants. Empty `positions` falls back to the platform's * baseline permission set, exactly like a fresh member's own REST request. - * - neither (an effective `runAs:'user'` run with no trigger user — a - * schedule) → a {@link RunProvenanceContext}: the run id and nothing else, - * so the middleware still sees no principal and the run still executes under - * the documented #1888 unscoped fail-open, while hooks can tell whose run - * made the write (#3712). `undefined` only when there is no run id either. + * - neither (an effective `runAs:'user'` run with no trigger user) → **throws** + * {@link UnscopedRunDataAccessError} (#3760). This used to return a + * provenance-only envelope and let the run proceed UNSCOPED — the #1888 + * fail-open. A schedule is only the most obvious source of a user-less run; + * the commonest is a record-change flow fired by a write that carried no + * user (any `isSystem` plugin/service write, the approvals status mirror, or + * a `runAs:'system'` flow's own data node — `isSystem` does NOT suppress + * trigger dispatch, only `skipTriggers` does). None of those are decidable at + * authoring time, which is why the refusal has to live here. * * The engine sets {@link AutomationContext.runAs} on the run context at setup; * this function is the single place that maps it to an ObjectQL context, shared - * by every data-touching node so the policy can't drift between node types. + * by every data-touching node so the policy can't drift between node types — + * which is exactly why the refusal belongs here and not in each executor. + * + * @throws {UnscopedRunDataAccessError} when the run resolves no principal. */ export function resolveRunDataContext(context: AutomationContext | undefined): RunDataContext | undefined { const flowRunId = context?.flowRunId; @@ -82,15 +130,18 @@ export function resolveRunDataContext(context: AutomationContext | undefined): R return { isSystem: true, positions: [], permissions: [], ...(flowRunId ? { flowRunId } : {}) }; } if (!context?.userId) { - // #3712 — no identity to present, but there IS a run. Carry the run id - // ALONE (see {@link RunProvenanceContext}): provenance without a principal, - // so the approvals record lock can recognise the owning run's write to its - // own target record (#3456) while the security middleware sees exactly what - // it saw before — nothing to key on. Manufacturing a *principal* here (even - // `{ isSystem: false, positions: [], permissions: [] }`) would be the wrong - // tool: it would tie this fix to the #1888 fail-open's fate instead of - // leaving that decision open. - return flowRunId ? { flowRunId } : undefined; + // #3760 — FAIL CLOSED. There is no identity to present, and presenting none + // means the data security middleware skips every principal gate and runs the + // operation unscoped. `runAs:'user'` asked for restriction; silently + // delivering elevation is the fail-open ADR-0049 forbids. Refuse instead. + // + // Deliberately NOT `{ isSystem: true }`: the middleware's isSystem + // short-circuit precedes the package-managed-row / system-row / + // audience-anchor / delegated-admin gates that a principal-less context + // still has to clear, so re-badging these runs as system would GRANT them + // powers they never had (e.g. writing sys_user_position) rather than + // preserving the status quo. + throw new UnscopedRunDataAccessError(context); } // `context` is now narrowed to a defined AutomationContext with a userId. const out: RunIdentityContext = { @@ -124,21 +175,24 @@ export function flowTouchesData(flow: { nodes?: ReadonlyArray<{ type?: string }> } /** - * True when a run's effective identity is the fail-open *unscoped* case: an - * effective `runAs:'user'` (explicit or defaulted) with NO resolvable trigger - * user — e.g. a schedule-triggered run, which has no user to scope to (#1888). + * True when a run has NO resolvable principal: an effective `runAs:'user'` + * (explicit or defaulted) with no trigger user (#1888). + * + * A schedule is the shape the docs have always led with, but it is not the + * common one. ANY run whose trigger resolved no user lands here — most often a + * record-change flow fired by a write that carried no user, since `isSystem` + * does not suppress trigger dispatch (only `skipTriggers` does). `time_relative` + * and `api` triggers likewise supply no user. * - * {@link resolveRunDataContext} resolves no principal for this case — the CRUD - * node passes either no `options.context` at all or a provenance-only one - * (#3712), neither of which presents an identity — and the data security - * middleware, which *skips* when there is no identity (delegating auth to the - * auth layer), runs the operation UNSCOPED (effectively elevated). An author - * who left `runAs` at the - * `'user'` default expecting a restricted run instead gets an unscoped one. The - * engine uses this predicate to surface the footgun at run time (a loud warning, - * not a silent elevation); the build-time lint `flow-schedule-runas-unscoped` - * catches it earlier, and declaring `runAs:'system'` makes the elevation - * explicit and intended (ADR-0049). + * Since #3760 such a run may not touch data at all: {@link resolveRunDataContext} + * throws {@link UnscopedRunDataAccessError} rather than handing the data engine + * a principal-less context that the security middleware would wave straight + * through. The engine uses this predicate to warn at run SETUP — before any node + * executes — that a data-touching run is going to be refused, so the failure is + * diagnosable rather than a surprise mid-flow. The build-time lint + * `flow-runas-unscoped` rejects the statically-decidable shapes at + * publish time. Declaring `runAs:'system'` makes the elevation explicit and + * intended (ADR-0049). */ export function runIsUnscopedUserMode(context: AutomationContext | undefined): boolean { return context?.runAs !== 'system' && !context?.userId; diff --git a/packages/spec/src/automation/flow.zod.ts b/packages/spec/src/automation/flow.zod.ts index 163592380d..56f3425c1b 100644 --- a/packages/spec/src/automation/flow.zod.ts +++ b/packages/spec/src/automation/flow.zod.ts @@ -265,15 +265,22 @@ export const FlowSchema = lazySchema(() => z.object({ // declared identity for the run's data operations and restores the caller's // context afterward: `system` runs elevated (a full-access, RLS-bypassing // system principal); `user` (default) runs as the triggering user, so CRUD - // nodes' ObjectQL reads/writes respect that user's row-level security. A - // schedule-triggered run with no user stays unscoped under `user` (there is no - // identity to scope to) — declare `system` to make elevation explicit. + // nodes' ObjectQL reads/writes respect that user's row-level security. + // + // A run under `user` that resolves NO trigger user has nothing to scope to, so + // its data operations are REFUSED (#3760). This is NOT a schedule-only case — + // it is any run whose trigger supplied no user, and the commonest by far is a + // record-change flow fired by a write that carried none (any `isSystem` + // plugin/service write; `isSystem` does not suppress trigger dispatch, only + // `skipTriggers` does). Declare `system` to make the elevation explicit. runAs: z .enum(['system', 'user']) .default('user') .describe( 'Execution identity for the run: system = elevated (bypasses RLS), user = the triggering user (RLS-respecting). ' + - 'A schedule-triggered run has no trigger user, so under user it runs UNSCOPED (elevated) — declare system to make that explicit.', + 'A run with no trigger user has no identity to scope to, so under user its data operations are REFUSED — ' + + 'declare system to make the elevation explicit. This covers schedule/time-relative/api triggers AND any ' + + 'record-change flow fired by a write that carried no user.', ), /** Error Handling Strategy */ diff --git a/packages/triggers/trigger-record-change/src/formula-context.test.ts b/packages/triggers/trigger-record-change/src/formula-context.test.ts index 8708802855..58e20a9d15 100644 --- a/packages/triggers/trigger-record-change/src/formula-context.test.ts +++ b/packages/triggers/trigger-record-change/src/formula-context.test.ts @@ -97,7 +97,7 @@ describe('record-change context hydrates read-time formula fields (#3426)', () = edges: [ { id: 'e1', source: 'start', target: 'stamp' }, { id: 'e2', source: 'stamp', target: 'end' } ], } as any); - const created = await data.insert('crm_lead', { first_name: 'Ada', last_name: 'Lovelace' }); + const created = await data.insert('crm_lead', { first_name: 'Ada', last_name: 'Lovelace' }, { context: { userId: 'u_trigger' } }); const id = Array.isArray(created) ? created[0]?.id : created?.id ?? created; await sleep(200); const row = await data.findOne('crm_lead', { where: { id } }); diff --git a/packages/triggers/trigger-record-change/src/multilookup-context.test.ts b/packages/triggers/trigger-record-change/src/multilookup-context.test.ts index 37f030fb1f..a7ea2fe483 100644 --- a/packages/triggers/trigger-record-change/src/multilookup-context.test.ts +++ b/packages/triggers/trigger-record-change/src/multilookup-context.test.ts @@ -85,7 +85,7 @@ describe('record-change context hydrates multi-lookup from input (#1872)', () => edges: [ { id: 'e1', source: 'start', target: 'stamp' }, { id: 'e2', source: 'stamp', target: 'end' } ], } as any); - const created = await data.insert('piece', { title: 'X', target_channels: ['ch_1', 'ch_2'] }); + const created = await data.insert('piece', { title: 'X', target_channels: ['ch_1', 'ch_2'] }, { context: { userId: 'u_trigger' } }); const id = Array.isArray(created) ? created[0]?.id : created?.id ?? created; await sleep(200); const row = await data.findOne('piece', { where: { id } }); diff --git a/packages/triggers/trigger-record-change/src/record-change-integration.test.ts b/packages/triggers/trigger-record-change/src/record-change-integration.test.ts index 1703b187ad..8c6e753aa1 100644 --- a/packages/triggers/trigger-record-change/src/record-change-integration.test.ts +++ b/packages/triggers/trigger-record-change/src/record-change-integration.test.ts @@ -180,6 +180,83 @@ const objectDef = (name: string) => ({ }, }); +/** + * #3760 — the fail-open this trigger was the dominant carrier of. + * + * `isSystem` does NOT suppress trigger dispatch (only `skipTriggers` does), and + * the trigger forwards `session.userId` with no fallback. So a write made with a + * system context — any plugin/service write, the approvals status mirror, a + * `runAs:'system'` flow's own data node — fires the record-change flows bound to + * that object with `userId: undefined`. A flow left at the spec default + * `runAs:'user'` then presented NO principal to ObjectQL, and the data security + * middleware skips when there is no principal: the flow read and wrote every row. + * + * Nothing flags this at authoring time and nothing can — whether a given write + * carries a user is only knowable at run time — so the runtime refusal is the + * only net. This test drives the real kernel end-to-end rather than the seam, so + * it fails if ANY link is re-opened: dispatch suppression, identity forwarding, + * or the refusal itself. + */ +describe('a system write must not fire a record-change flow UNSCOPED (#3760)', () => { + it("refuses the flow's data op when the triggering write carried isSystem and no user", async () => { + const kernel = new ObjectKernel({ logLevel: 'silent' }); + await kernel.use(new ObjectQLPlugin()); + await kernel.use(new AutomationServicePlugin()); + await kernel.use(new RecordChangeTriggerPlugin()); + await kernel.bootstrap(); + + const objectql = kernel.getService('objectql') as any; + const data = kernel.getService('data') as any; + const automation = kernel.getService('automation'); + + objectql.registerDriver(makeMemoryDriver(), true); + objectql.registry.registerObject(objectDef('sysw'), 'test', 'test'); + // No `runAs` — the spec default 'user'. This is the shape an author (very + // often an AI) writes without realising it can run without a user. + automation.registerFlow('sysw_stamp', stampFlow('sysw_stamp', 'sysw') as any); + + // A SYSTEM write: elevated, no userId, and NOT skipTriggers — so it still + // dispatches. This is the approvals-status-mirror shape. + const created = await data.insert( + 'sysw', + { status: 'new' }, + { context: { isSystem: true, positions: [], permissions: [] } }, + ); + const id = Array.isArray(created) ? created[0]?.id : created?.id ?? created; + await sleep(200); + + // The flow's update_record must NOT have landed. Before #3760 `stamp` was + // 'done' here — written by a run with no principal at all. + const row = await data.findOne('sysw', { where: { id } }); + expect(row?.stamp, 'a user-less run wrote to the record — the fail-open is back').toBeUndefined(); + }, 15000); + + it('the same flow still works normally when a real user made the write', async () => { + const kernel = new ObjectKernel({ logLevel: 'silent' }); + await kernel.use(new ObjectQLPlugin()); + await kernel.use(new AutomationServicePlugin()); + await kernel.use(new RecordChangeTriggerPlugin()); + await kernel.bootstrap(); + + const objectql = kernel.getService('objectql') as any; + const data = kernel.getService('data') as any; + const automation = kernel.getService('automation'); + + objectql.registerDriver(makeMemoryDriver(), true); + objectql.registry.registerObject(objectDef('sysw2'), 'test', 'test'); + automation.registerFlow('sysw2_stamp', stampFlow('sysw2_stamp', 'sysw2') as any); + + const created = await data.insert('sysw2', { status: 'new' }, { context: { userId: 'u_trigger' } }); + const id = Array.isArray(created) ? created[0]?.id : created?.id ?? created; + await sleep(200); + + // The refusal is scoped to the user-less case — it must not break the + // ordinary record-change flow, which is the overwhelming majority. + const row = await data.findOne('sysw2', { where: { id } }); + expect(row?.stamp).toBe('done'); + }, 15000); +}); + describe('record-change trigger — end-to-end (#1491)', () => { it('fires a record-after-create flow registered AFTER the trigger (engine.registerFlow path)', async () => { const kernel = new ObjectKernel({ logLevel: 'silent' }); @@ -202,7 +279,7 @@ describe('record-change trigger — end-to-end (#1491)', () => { triggerType: 'record_change', }); - const created = await data.insert('wid', { status: 'new' }); + const created = await data.insert('wid', { status: 'new' }, { context: { userId: 'u_trigger' } }); const id = Array.isArray(created) ? created[0]?.id : created?.id ?? created; await sleep(200); @@ -247,7 +324,7 @@ describe('record-change trigger — end-to-end (#1491)', () => { triggerType: 'record_change', }); - const created = await data.insert('wid2', { status: 'new' }); + const created = await data.insert('wid2', { status: 'new' }, { context: { userId: 'u_trigger' } }); const id = Array.isArray(created) ? created[0]?.id : created?.id ?? created; await sleep(200); @@ -276,14 +353,14 @@ describe('record-change trigger — end-to-end (#1491)', () => { }); // Create — the afterInsert leg fires; the flow mirrors status → mirror. - const created = await data.insert('wid3', { status: 'a' }); + const created = await data.insert('wid3', { status: 'a' }, { context: { userId: 'u_trigger' } }); const id = Array.isArray(created) ? created[0]?.id : created?.id ?? created; await sleep(200); expect((await data.findOne('wid3', { where: { id } }))?.mirror).toBe('a'); // Update — the afterUpdate leg of the SAME flow fires; mirror re-syncs. (The // flow's own write-back does not loop: the re-entrancy guard suppresses it.) - await data.update('wid3', { id, status: 'b' }); + await data.update('wid3', { id, status: 'b' }, { context: { userId: 'u_trigger' } }); await sleep(200); expect((await data.findOne('wid3', { where: { id } }))?.mirror).toBe('b'); }, 15000); @@ -306,20 +383,20 @@ describe('record-change trigger — end-to-end (#1491)', () => { // Create leg — a brand-new URGENT record: `previous == null` makes the // condition true, so the flow fires on afterInsert (the create-discrimination // pattern the docs/showcase advertise). - const urgent = await data.insert('wid5', { priority: 'urgent' }); + const urgent = await data.insert('wid5', { priority: 'urgent' }, { context: { userId: 'u_trigger' } }); const urgentId = Array.isArray(urgent) ? urgent[0]?.id : urgent?.id ?? urgent; await sleep(200); expect((await data.findOne('wid5', { where: { id: urgentId } }))?.alerted).toBe('yes'); // Create leg — a NON-urgent record: the condition is false, no fire. - const low = await data.insert('wid5', { priority: 'low' }); + const low = await data.insert('wid5', { priority: 'low' }, { context: { userId: 'u_trigger' } }); const lowId = Array.isArray(low) ? low[0]?.id : low?.id ?? low; await sleep(200); expect((await data.findOne('wid5', { where: { id: lowId } }))?.alerted).toBeFalsy(); // Update leg — escalate that low record to urgent: `previous.priority` was // 'low', so the transition guard fires the flow on afterUpdate. - await data.update('wid5', { id: lowId, priority: 'urgent' }); + await data.update('wid5', { id: lowId, priority: 'urgent' }, { context: { userId: 'u_trigger' } }); await sleep(200); expect((await data.findOne('wid5', { where: { id: lowId } }))?.alerted).toBe('yes'); }, 15000);