From b970c9b9e1a9bbbc5e4cde750456d2d6703f040a Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 21:05:49 +0000 Subject: [PATCH 1/2] fix(runtime): gate the paused-run screen read to the run's trigger identity (#7968) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `GET /automation/:name/runs/:runId/screen` served the paused run's ScreenSpec to any authenticated caller who knew a run id. A screen node's `defaults` and per-field `defaultValue` are interpolated against the live flow variables at suspend time, so a flow prefilling from its triggering record persists those values into the spec this route hands back — measured on a real screen flow over a `crm_lead` record: company in the title, email in the description, and email, phone and salary band as field defaults, answered 200 to a stranger explicitly refused the `sys_automation_run` read grant. Maintainer ruling 2026-08-12 (Option B): the route now requires the run's own trigger identity (`ExecutionLogEntry.trigger.userId`) OR read access to `sys_automation_run` as an operator override. The object grant ALONE is deliberately not the gate — it would refuse the screen to the very person the flow paused for, which is why #7900 audited this route out of its convergence. So the grant is the override half, and the over-block direction is pinned as hard as the under-block one. The `sys_automation_run` question is now one predicate (`mayReadRunState`) shared by both gates rather than a second copy of the resolution / feature-detection / fail-closed logic. Unchanged: the 404 for a run with no pending screen (the gate runs after the lookup, so every not-found path is byte-identical for every caller), the 501, the 401 anonymous floor, `resume`'s own authority checks, and which runs exist. Option A — the per-run `resumeAuthority` read gate — stays the recorded direction and is out of scope here. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01B3Kurx8qufrDzNjk4rag7V --- ...ation-screen-read-trigger-identity-gate.md | 38 ++ ...utomation-run-read-permission-gate.test.ts | 16 +- .../automation-screen-read-gate.test.ts | 512 ++++++++++++++++++ packages/runtime/src/domains/automation.ts | 202 +++++-- 4 files changed, 729 insertions(+), 39 deletions(-) create mode 100644 .changeset/automation-screen-read-trigger-identity-gate.md create mode 100644 packages/runtime/src/domains/automation-screen-read-gate.test.ts diff --git a/.changeset/automation-screen-read-trigger-identity-gate.md b/.changeset/automation-screen-read-trigger-identity-gate.md new file mode 100644 index 0000000000..c31079d18c --- /dev/null +++ b/.changeset/automation-screen-read-trigger-identity-gate.md @@ -0,0 +1,38 @@ +--- +"@objectstack/runtime": patch +--- + +fix(runtime): the paused-run screen read is gated to the run's trigger identity, or the `sys_automation_run` grant (#7968) + +`GET /api/v1/automation/:name/runs/:runId/screen` answered **any authenticated +caller who knew a run id** with the paused run's `ScreenSpec` — and that spec is +not inert with respect to record data. A screen node's `defaults` and per-field +`defaultValue` are interpolated against the live flow variables at suspend time, +so a flow that prefills from its triggering record persists those values into the +spec this route serves. + +Measured on a real screen flow over a `crm_lead` record: a caller with valid +auth, no relationship to the run, and explicitly refused the `sys_automation_run` +read grant received `200` with the lead's company in the screen title, its email +address in the description, and its email, phone and salary band as three field +defaults. Reaching it needed only a session plus a leaked or guessed run id. + +The route now requires **the identity that triggered the run** +(`ExecutionLogEntry.trigger.userId`) **OR** read access to `sys_automation_run` +as an operator override. A refused caller gets `403 PERMISSION_DENIED`. + +**Why not the object grant on its own** — the mechanism the sibling run-state +reads converged on in #7900: it would refuse the screen to the very person the +flow paused for. The pause exists because the flow is asking *that* caller to +fill a form in, so the grant is the override half here, never the whole question. +Operator tooling that already holds the `sys_automation_run` read grant is +unaffected, and so is the end user — including while the permission subsystem is +unreachable, since only the override half fails closed. + +**Unchanged**: which runs exist; `resume`'s own per-run `resumeAuthority` checks +(#3801 / #5561); the `404 No pending screen for run` answer, which still comes +back for an unknown or non-paused run id, for every caller, ahead of the gate; the +`501` a deployment without screen lookup returns; and the `401` anonymous floor. + +A deployment with no `plugin-security` (no object-permission system at all, so +`/data/sys_automation_run` is itself ungated) keeps answering as before. diff --git a/packages/runtime/src/domains/automation-run-read-permission-gate.test.ts b/packages/runtime/src/domains/automation-run-read-permission-gate.test.ts index 618e29899b..7b8ebbbce5 100644 --- a/packages/runtime/src/domains/automation-run-read-permission-gate.test.ts +++ b/packages/runtime/src/domains/automation-run-read-permission-gate.test.ts @@ -31,7 +31,10 @@ * the ruling names and not some second permission invented here. * 4. **THE AUDIT** — the routes that stay authenticated-only stay * authenticated-only, and ask the security service nothing. A future change - * to any of those verdicts has to come through this file. + * to any of those verdicts has to come through this file. (#7968 is that + * change, for one row: the paused-run `screen` read left this table when + * the maintainer gated it on the run's trigger identity instead — see the + * note in the table.) * * The three non-denials (system context, no security service, partial service) * are pinned too: each is a decision recorded on `refuseUngrantedRunRead`, and @@ -323,7 +326,16 @@ describe('#7900 — /automation run-state reads require the sys_automation_run r { path: 'approval_flow', why: 'getFlow — a flow definition, metadata-plane data' }, { path: 'actions', why: 'getActionDescriptors — the deployment action catalog' }, { path: '_status', why: 'getFlowRuntimeStates — per-flow enabled/bound state' }, - { path: 'approval_flow/runs/run_7/screen', why: 'the interactive runner\'s re-fetch' }, + // [#7968] `approval_flow/runs/run_7/screen` USED to be this table's + // fifth row. The audit's reason for leaving it here was right — the + // grant alone would refuse the end user the flow paused for — but + // "no grant" was not the same as "no gate": the route disclosed + // record-derived screen defaults to any authenticated caller with a + // run id. The 2026-08-12 ruling gates it on the run's own trigger + // identity, with this grant as an operator override, so it is no + // longer authenticated-only and no longer answers `explainCalls === + // 0` for a caller who is not the trigger identity. Its own file + // owns it now: `automation-screen-read-gate.test.ts`. ]; it.each(AUTHENTICATED_ONLY)('$path stays authenticated-only ($why)', async ({ path }) => { diff --git a/packages/runtime/src/domains/automation-screen-read-gate.test.ts b/packages/runtime/src/domains/automation-screen-read-gate.test.ts new file mode 100644 index 0000000000..8cfc47ada3 --- /dev/null +++ b/packages/runtime/src/domains/automation-screen-read-gate.test.ts @@ -0,0 +1,512 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #7968 — `GET /automation/:name/runs/:runId/screen` is gated to the run's own + * trigger identity, OR the `sys_automation_run` read grant as an operator + * override. + * + * Maintainer ruling, 2026-08-12 (Option B), acceptance verbatim: *"stranger + * with valid auth + run id ⇒ denied; triggering user ⇒ screen; holder of + * `sys_automation_run` read ⇒ screen."* + * + * ## What was measured before the gate + * + * A real screen flow, run through the real engine + * (`service-automation`: `registerScreenNodes` → `screen` node with + * `defaultValue: '{record.email}'` etc., triggered on a `crm_lead` record), + * suspends with this persisted `ScreenSpec` — record values interpolated into + * the title, the description and every field default: + * + * ``` + * { nodeId: 'collect', title: 'Confirm Acme Health', + * description: 'Contact for ceo@acme-health.example', + * fields: [ { name: 'email', defaultValue: 'ceo@acme-health.example' }, + * { name: 'phone', defaultValue: '+1-555-0100' }, + * { name: 'salary', defaultValue: 'L7 / 285000 USD' } ] } + * ``` + * + * Fed through this dispatcher, a caller with valid auth, no relationship to the + * run and NO `sys_automation_run` grant received it under `200 { success: + * true }` — the whole spec, every value. That payload is {@link REAL_SCREEN} + * here, verbatim, so the fixture is the observed disclosure rather than a + * plausible stand-in. + * + * ## Why the over-block direction is pinned as hard as the under-block one + * + * The obvious gate is the WRONG one, and that is the entire reason this card + * needed a ruling. Requiring the `sys_automation_run` grant — the mechanism + * #7900 converged the sibling run-state reads on — passes a naive "the stranger + * is denied" test **while refusing the end user the flow paused for**, i.e. + * while breaking the route's only purpose. So every admit case below asserts + * the screen ARRIVES WITH ITS RECORD-DERIVED VALUES INTACT, not merely that a + * 200 came back: an emptied or stripped screen is a broken route wearing a + * passing status code. + * + * ## Not in scope (recorded, not built) + * + * Option A — a per-run authority read gate derived from the suspension's own + * `resumeAuthority` / assignee state, the axis `resume` answers on (#3801 / + * #5561) — stays the recorded coherent end state and is ADR-0019-class design + * work. B does not preclude it: both refuse the same stranger and admit the + * same end user, so A can supersede this without re-litigating the acceptance. + */ + +import { describe, it, expect, vi } from 'vitest'; + +import { HttpDispatcher } from '../http-dispatcher.js'; +import type { HttpProtocolContext } from '../http-dispatcher.js'; +import { AUTOMATION_RUN_OBJECT } from './automation.js'; + +/** + * The disclosure itself, captured from a real engine run (see the file header). + * Every string in here is derived from the triggering `crm_lead` record. + */ +const REAL_SCREEN = { + nodeId: 'collect', + title: 'Confirm Acme Health', + description: 'Contact for ceo@acme-health.example', + fields: [ + { name: 'email', label: 'Email', type: 'text', required: false, defaultValue: 'ceo@acme-health.example' }, + { name: 'phone', label: 'Phone', type: 'text', required: false, defaultValue: '+1-555-0100' }, + { name: 'salary', label: 'Band', type: 'text', required: false, defaultValue: 'L7 / 285000 USD' }, + ], +} as const; + +/** + * Every record-derived string the spec above carries. A refusal is checked + * against this list POSITIVELY — "none of these appears in what the stranger + * received" — because "the response differs from the granted one" is also true + * of a 200 that leaked half the fields. + */ +const RECORD_DERIVED_VALUES = [ + 'Acme Health', + 'ceo@acme-health.example', + '+1-555-0100', + 'L7 / 285000 USD', +]; + +/** The run the screen belongs to, as the engine records it (`buildRunTrigger`, #7533). */ +const PAUSED_RUN = { + id: 'run_1', + flowName: 'lead_followup', + status: 'paused', + trigger: { type: 'record_change', userId: 'user_owner', object: 'crm_lead', recordId: 'lead_1' }, + steps: [{ nodeId: 'collect', nodeType: 'screen', status: 'paused' }], +} as const; + +/** One `explain` call, as the gate makes it. */ +interface ExplainCall { + request: { object: string; operation: string; userId?: string }; + context: unknown; +} + +interface Harness { + dispatcher: HttpDispatcher; + getSuspendedScreen: ReturnType; + getRun: ReturnType; + resume: ReturnType; + explainCalls: ExplainCall[]; +} + +interface Options { + /** The run `getRun` answers with — `null` for "no such run". */ + run?: unknown; + /** Omit `getRun` entirely: a service that cannot say who triggered a run. */ + withoutGetRun?: boolean; + /** What `getSuspendedScreen` answers — `null` is the nonexistent-run path. */ + screen?: unknown; + /** Omit `getSuspendedScreen`: the capability this deployment does not have. */ + withoutScreenLookup?: boolean; +} + +/** + * `security` is the deployment's security posture: `'granting'` / `'refusing'` + * are a service that answers, `'throwing'` one whose answer cannot be computed, + * `'partial'` an implementation that omits `explain`, `'absent'` a deployment + * with no `plugin-security` at all. + */ +function makeDispatcher( + security: 'granting' | 'refusing' | 'throwing' | 'partial' | 'absent', + options: Options = {}, +): Harness { + const explainCalls: ExplainCall[] = []; + const screen = 'screen' in options ? options.screen : REAL_SCREEN; + const getSuspendedScreen = vi.fn(async () => screen as unknown); + const getRun = vi.fn(async () => ('run' in options ? options.run : PAUSED_RUN) as unknown); + const resume = vi.fn(async () => ({ success: true, status: 'completed' }) as unknown); + + const explain = async ( + request: ExplainCall['request'], + context: unknown, + ): Promise<{ allowed: boolean; object: string; operation: string }> => { + explainCalls.push({ request, context }); + if (security === 'throwing') throw new Error('permission subsystem unavailable'); + return { allowed: security === 'granting', object: request.object, operation: request.operation }; + }; + + const automation: Record = { + handlerReady: true, + resume, + getFlow: async (name: string) => ({ name, nodes: [] }), + }; + if (!options.withoutScreenLookup) automation.getSuspendedScreen = getSuspendedScreen; + if (!options.withoutGetRun) automation.getRun = getRun; + + const services: Record = { automation }; + if (security === 'partial') { + services.security = { getReadableFields: async () => undefined }; + } else if (security !== 'absent') { + services.security = { explain }; + } + + const resolve = (name: string): unknown => services[name]; + const kernel = { + getService: resolve, + getServiceAsync: async (name: string) => resolve(name), + context: { getService: resolve }, + }; + return { + dispatcher: new HttpDispatcher(kernel as never), + getSuspendedScreen, + getRun, + resume, + explainCalls, + }; +} + +/** The identity the flow paused for — `PAUSED_RUN.trigger.userId`. */ +const TRIGGERING_USER = (): HttpProtocolContext => + ({ request: {}, executionContext: { userId: 'user_owner', positions: ['sales_rep'] } } as HttpProtocolContext); + +/** A stranger: valid auth, no relationship to this run. */ +const STRANGER = (): HttpProtocolContext => + ({ request: {}, executionContext: { userId: 'user_stranger', positions: ['intern'] } } as HttpProtocolContext); + +/** A platform-internal caller. */ +const SYSTEM_CTX = (): HttpProtocolContext => + ({ request: {}, executionContext: { userId: 'usr_system', isSystem: true } } as HttpProtocolContext); + +const SCREEN_PATH = 'lead_followup/runs/run_1/screen'; + +/** The payload out of the success envelope, whatever the envelope's shape. */ +const payloadOf = (response: unknown): any => { + const r = response as any; + return r?.data ?? r?.body?.data ?? r; +}; + +/** The semantic error code, from wherever the envelope parks it. */ +const codeOf = (response: unknown): unknown => { + const r = response as any; + return r?.body?.error?.code ?? r?.body?.error?.details?.code; +}; + +/** The whole response as text — for asserting a value is ABSENT from it. */ +const textOf = (response: unknown): string => JSON.stringify((response as any)?.body ?? response); + +/** + * The over-block assertion, in one place: the screen ARRIVED, whole, with every + * record-derived value still on it. + * + * Deep-equality against the entire captured spec plus a value-by-value check. + * A gate that admits the right caller and then hands them an empty `fields` + * array, or defaults stripped "to be safe", has broken the route while passing + * any status assertion — and the pause exists precisely so this caller can see + * these prefills. + */ +function expectScreenServedWhole(response: unknown): void { + expect((response as any).status).toBe(200); + const payload = payloadOf(response); + expect(payload).toEqual({ runId: 'run_1', screen: REAL_SCREEN }); + expect(payload.screen.fields).toHaveLength(3); + for (const value of RECORD_DERIVED_VALUES) expect(textOf(response)).toContain(value); +} + +describe('#7968 — the paused-run screen is gated to its trigger identity, or the run-state grant', () => { + describe('case 1 — stranger with valid auth + run id ⇒ denied', () => { + it('refuses with PERMISSION_DENIED AND 403 (ADR-0112, both halves)', async () => { + const h = makeDispatcher('refusing'); + const { response } = await h.dispatcher.handleAutomation( + SCREEN_PATH, 'GET', undefined, STRANGER(), undefined, + ); + + // BOTH halves, on purpose. This route's correct refusal (403) and + // its pre-existing not-found answer (404) are one status apart, so + // `status >= 400` cannot tell a working gate from a mistyped run + // id — and a bare `code` check cannot tell 403 from a 200 carrying + // a code field. + expect(codeOf(response)).toBe('PERMISSION_DENIED'); + expect((response as any).status).toBe(403); + }); + + it('does not disclose the record-derived values — asserted value by value', async () => { + const h = makeDispatcher('refusing'); + const { response } = await h.dispatcher.handleAutomation( + SCREEN_PATH, 'GET', undefined, STRANGER(), undefined, + ); + + // The disclosure this card is about, pinned POSITIVELY: each value + // the real run interpolated into the spec is absent from what the + // stranger received. "The response differs from the granted one" is + // also true of a 200 that leaked two fields out of three. + const body = textOf(response); + for (const value of RECORD_DERIVED_VALUES) expect(body).not.toContain(value); + expect(payloadOf(response)?.screen).toBeUndefined(); + }); + + it('answers nothing about the caller\'s authorization topology (#7450)', async () => { + const h = makeDispatcher('refusing'); + const { response } = await h.dispatcher.handleAutomation( + SCREEN_PATH, 'GET', undefined, STRANGER(), undefined, + ); + + const body = textOf(response); + expect(body).not.toContain('intern'); + expect(body).not.toContain('user_stranger'); + // It DOES name what would admit a caller — both halves, so the end + // user is not misdirected to ask for an operator grant. + expect(body).toContain(AUTOMATION_RUN_OBJECT); + expect(body).toContain('triggered the run'); + }); + + it('refuses a run whose trigger carries NO userId rather than matching on absence', async () => { + // A schedule-triggered run has no `trigger.userId`. An identity + // check written as `run.trigger?.userId === ec.userId` over two + // undefineds would admit everyone on exactly these runs — the + // failure mode this case exists to make impossible. + const h = makeDispatcher('refusing', { + run: { ...PAUSED_RUN, trigger: { type: 'schedule' } }, + }); + const { response } = await h.dispatcher.handleAutomation( + SCREEN_PATH, 'GET', undefined, STRANGER(), undefined, + ); + + expect(codeOf(response)).toBe('PERMISSION_DENIED'); + expect((response as any).status).toBe(403); + for (const value of RECORD_DERIVED_VALUES) expect(textOf(response)).not.toContain(value); + }); + }); + + describe('case 2 — the triggering user gets the screen (the OVER-BLOCK guard)', () => { + it('serves it whole to the identity the flow paused for, with NO grant at all', async () => { + // `'refusing'` is the load-bearing half of this case: this caller is + // refused the `sys_automation_run` grant, exactly like the stranger + // above. Gate on the grant — the #7900 mechanism, the obvious and + // wrong one — and this test goes red while every denial test stays + // green. That asymmetry is the whole reason the ruling was needed. + const h = makeDispatcher('refusing'); + const { response } = await h.dispatcher.handleAutomation( + SCREEN_PATH, 'GET', undefined, TRIGGERING_USER(), undefined, + ); + + expectScreenServedWhole(response); + }); + + it('reads the identity off THIS run, and does not consult the grant once it matches', async () => { + const h = makeDispatcher('refusing'); + await h.dispatcher.handleAutomation(SCREEN_PATH, 'GET', undefined, TRIGGERING_USER(), undefined); + + // The identity is the run's own (`ExecutionLogEntry.trigger.userId`), + // looked up for the run in the path… + expect(h.getRun).toHaveBeenCalledWith('run_1'); + // …and it is SUFFICIENT: the end user's access does not depend on + // the permission subsystem being reachable, or existing. + expect(h.explainCalls).toHaveLength(0); + }); + + it('still serves the end user when the permission subsystem is DOWN', async () => { + // `explain` throwing fails closed for the OVERRIDE half only. The + // person the flow paused for is not locked out of their own form by + // an operator-side outage. + const h = makeDispatcher('throwing'); + const { response } = await h.dispatcher.handleAutomation( + SCREEN_PATH, 'GET', undefined, TRIGGERING_USER(), undefined, + ); + + expectScreenServedWhole(response); + }); + + it('…while the same outage still refuses the stranger', async () => { + const h = makeDispatcher('throwing'); + const { response } = await h.dispatcher.handleAutomation( + SCREEN_PATH, 'GET', undefined, STRANGER(), undefined, + ); + + // The other side of that asymmetry: an unresolvable OVERRIDE is a + // denial, the stance plugin-security takes on an unresolvable + // posture (#3545). "Could not evaluate" never reads as "allowed". + expect(codeOf(response)).toBe('PERMISSION_DENIED'); + expect((response as any).status).toBe(403); + }); + }); + + describe('case 3 — a holder of the `sys_automation_run` read grant gets the screen', () => { + it('serves it whole to an operator who did NOT trigger the run', async () => { + const h = makeDispatcher('granting'); + const { response } = await h.dispatcher.handleAutomation( + SCREEN_PATH, 'GET', undefined, STRANGER(), undefined, + ); + + expectScreenServedWhole(response); + }); + + it('asks for `read` on sys_automation_run with the caller\'s own context', async () => { + const h = makeDispatcher('granting'); + await h.dispatcher.handleAutomation(SCREEN_PATH, 'GET', undefined, STRANGER(), undefined); + + // The SAME question #7900's gate asks, through the same predicate — + // one grant, not a second permission invented for this route. + expect(h.explainCalls).toHaveLength(1); + expect(h.explainCalls[0]!.request.object).toBe(AUTOMATION_RUN_OBJECT); + expect(h.explainCalls[0]!.request.operation).toBe('read'); + // No `userId` on the request: explaining ANOTHER user is an + // administrative act. The gate asks about the CALLER. + expect(h.explainCalls[0]!.request.userId).toBeUndefined(); + expect(h.explainCalls[0]!.context).toMatchObject({ userId: 'user_stranger' }); + }); + + it('serves an operator even when the run cannot say who triggered it', async () => { + // `getRun` is optional on `IAutomationService`. A service that + // cannot answer the identity question admits nobody on that half — + // it must not fall open, and it must not lock the operator out. + const h = makeDispatcher('granting', { withoutGetRun: true }); + const { response } = await h.dispatcher.handleAutomation( + SCREEN_PATH, 'GET', undefined, STRANGER(), undefined, + ); + expectScreenServedWhole(response); + + const denied = makeDispatcher('refusing', { withoutGetRun: true }); + const refusal = await denied.dispatcher.handleAutomation( + SCREEN_PATH, 'GET', undefined, STRANGER(), undefined, + ); + expect(codeOf(refusal.response)).toBe('PERMISSION_DENIED'); + expect((refusal.response as any).status).toBe(403); + }); + + it('treats a getRun THROW as unresolved identity, not as a match', async () => { + const h = makeDispatcher('refusing'); + h.getRun.mockRejectedValueOnce(new Error('run store unreachable')); + const { response } = await h.dispatcher.handleAutomation( + SCREEN_PATH, 'GET', undefined, TRIGGERING_USER(), undefined, + ); + + // Even for the real trigger identity: an identity that could not be + // read is not an identity that matched. + expect(codeOf(response)).toBe('PERMISSION_DENIED'); + expect((response as any).status).toBe(403); + expect(textOf(response)).not.toContain('ceo@acme-health.example'); + }); + }); + + describe('the not-found answer is untouched — for every caller', () => { + /** + * Deliberate, and the reason the gate runs AFTER `getSuspendedScreen`: + * the identity half is derived from the run, so gating first would have + * to fail closed on an unresolvable run and turn today's 404 into a 403 + * for everyone, honest typos included. + * + * The consequence is stated rather than hidden: a stranger can still + * tell a paused run id (403) from an unknown one (404). That is an + * existence oracle over run ids — strictly narrower than the record + * values it replaces — and closing it means answering 404 to the + * refused caller, a different design that is NOT what was ruled. These + * cases hold the distinction so a future change to it is deliberate. + */ + it.each([ + ['a stranger', STRANGER], + ['the triggering user', TRIGGERING_USER], + ])('answers 404 (not 403) to %s for a run with no pending screen', async (_label, ctx) => { + const h = makeDispatcher('refusing', { screen: null }); + const { response } = await h.dispatcher.handleAutomation( + 'lead_followup/runs/run_missing/screen', 'GET', undefined, ctx(), undefined, + ); + + expect((response as any).status).toBe(404); + expect(codeOf(response)).not.toBe('PERMISSION_DENIED'); + expect(textOf(response)).toContain('No pending screen for run'); + }); + + it('does not even ask the permission question when there is nothing to disclose', async () => { + const h = makeDispatcher('refusing', { screen: null }); + await h.dispatcher.handleAutomation( + 'lead_followup/runs/run_missing/screen', 'GET', undefined, STRANGER(), undefined, + ); + + expect(h.explainCalls).toHaveLength(0); + }); + + it('keeps the 501 for a deployment whose service cannot look screens up', async () => { + const h = makeDispatcher('refusing', { withoutScreenLookup: true }); + const { response } = await h.dispatcher.handleAutomation( + SCREEN_PATH, 'GET', undefined, STRANGER(), undefined, + ); + + expect((response as any).status).toBe(501); + }); + }); + + describe('the non-denials this gate inherits from the run-state policy', () => { + it('lets a SYSTEM context through without asking anything', async () => { + const h = makeDispatcher('refusing'); + const { response } = await h.dispatcher.handleAutomation( + SCREEN_PATH, 'GET', undefined, SYSTEM_CTX(), undefined, + ); + + expectScreenServedWhole(response); + expect(h.explainCalls).toHaveLength(0); + }); + + it('serves the read where no security service exists at all', async () => { + // No `plugin-security` ⇒ no object-permission system ⇒ + // `/data/sys_automation_run` is itself ungated. Refusing here would + // put the two doors in disagreement the other way. + const h = makeDispatcher('absent'); + const { response } = await h.dispatcher.handleAutomation( + SCREEN_PATH, 'GET', undefined, STRANGER(), undefined, + ); + expectScreenServedWhole(response); + }); + + it('degrades on a security service that omits `explain` rather than throwing', async () => { + const h = makeDispatcher('partial'); + const { response } = await h.dispatcher.handleAutomation( + SCREEN_PATH, 'GET', undefined, STRANGER(), undefined, + ); + + expect((response as any).status).not.toBe(500); + expectScreenServedWhole(response); + }); + + it('still refuses an ANONYMOUS caller at the #5519 floor, ahead of this gate', async () => { + const h = makeDispatcher('granting'); + const { response } = await h.dispatcher.handleAutomation( + SCREEN_PATH, 'GET', undefined, + { request: {}, executionContext: {} } as HttpProtocolContext, undefined, + ); + + // Authentication is still the first question, still answered as + // 401/UNAUTHENTICATED — a narrowing must not re-label the anonymous + // floor as an authorization failure. + expect(codeOf(response)).toBe('UNAUTHENTICATED'); + expect((response as any).status).toBe(401); + expect(h.explainCalls).toHaveLength(0); + }); + }); + + describe('scope — what this card does NOT change', () => { + it('leaves `resume`\'s own authority checks alone (#3801 / #5561)', async () => { + // The write sibling answers in the ENGINE, on the suspension's + // declared `resumeAuthority`. A stranger reaching it is the engine's + // question to answer, not this gate's — and Option A, which would + // put the READ on that same per-run axis, is explicitly out of scope + // here. + const h = makeDispatcher('refusing'); + const { response } = await h.dispatcher.handleAutomation( + 'lead_followup/runs/run_1/resume', 'POST', { inputs: {} }, STRANGER(), undefined, + ); + + expect((response as any).status).not.toBe(403); + expect(h.resume).toHaveBeenCalled(); + expect(h.explainCalls).toHaveLength(0); + }); + }); +}); diff --git a/packages/runtime/src/domains/automation.ts b/packages/runtime/src/domains/automation.ts index 28c6e30573..ddba59f2ef 100644 --- a/packages/runtime/src/domains/automation.ts +++ b/packages/runtime/src/domains/automation.ts @@ -129,6 +129,21 @@ const RUN_READ_DENY_CODE = 'PERMISSION_DENIED'; const RUN_READ_DENY_MESSAGE = `Reading automation run state requires read access to '${AUTOMATION_RUN_OBJECT}'.`; +/** + * [#7968] The screen route's own refusal text — same `code` and `status`, a + * different sentence, because a different question was asked. + * + * The two halves are BOTH named. A caller refused here is either the wrong + * person or an operator without the grant, and a message naming only the grant + * would tell the end user the flow paused for to go ask for operator tooling — + * the exact misdirection this route's gate exists to avoid. It still names no + * position, permission set or identity (#7450): what it lists is what would + * admit ANY caller, not what this one is missing. + */ +const SCREEN_READ_DENY_MESSAGE = + 'Reading a paused run\'s screen requires being the identity that triggered the run, ' + + `or read access to '${AUTOMATION_RUN_OBJECT}'.`; + /** * [#7900] Which `/automation` GET routes serve `sys_automation_run`-class data. * @@ -140,8 +155,10 @@ const RUN_READ_DENY_MESSAGE = * `/:name/runs` → listRuns, an `ExecutionLogEntry[]` * `/:name/runs/:runId` → getRun, an `ExecutionLogEntry` served verbatim * - * `/:name/runs/:runId/screen` is deliberately NOT here; the audit reason is on - * the route itself, below. + * `/:name/runs/:runId/screen` is deliberately NOT here — [#7968] it is gated, + * but on a DIFFERENT question (`refuseUnrelatedScreenRead`): the run's own + * trigger identity, with this grant as an operator override. Adding it here + * would apply the grant alone and lock out the end user the flow paused for. */ function isRunStateRead(parts: string[], method: string): boolean { if (method !== 'GET') return false; @@ -154,9 +171,14 @@ function isRunStateRead(parts: string[], method: string): boolean { * [#7900] Ask the SAME question the other door answers with: may this caller * READ `sys_automation_run`? * - * Returns a refusal result when the answer is no, `undefined` when the read may - * proceed — so the caller reads as a guard clause and no route can accidentally - * consume a "denied" as a value. + * The BOOLEAN the two gates in this file share. [#7968] split it out of + * {@link refuseUngrantedRunRead} when a second route needed the identical + * question under a different refusal sentence: the run-state reads refuse when + * the answer is no, while the screen route treats it as the OPERATOR OVERRIDE + * half of a two-half gate. Two refusals, one implementation — a second copy of + * the resolution/feature-detection/fail-closed logic would be a second policy + * that happens to agree today, which is the shape the #7900 ruling exists to + * remove. * * ## Why `explain`, and why nothing new was built * @@ -204,25 +226,38 @@ function isRunStateRead(parts: string[], method: string): boolean { * other way would mean re-deriving the middleware's own empty-set rule here, * which is the drift `ISecurityService` exists to prevent. */ -async function refuseUngrantedRunRead( +async function mayReadRunState( deps: DomainHandlerDeps, context: HttpProtocolContext, -): Promise { +): Promise { const ec = context?.executionContext; - if (ec?.isSystem === true) return undefined; + if (ec?.isSystem === true) return true; const security = await deps.resolveService(context, 'security').catch(() => undefined) as Partial | undefined; - if (!security || typeof security.explain !== 'function') return undefined; + if (!security || typeof security.explain !== 'function') return true; - let allowed = false; try { const decision = await security.explain({ object: AUTOMATION_RUN_OBJECT, operation: 'read' }, ec); - allowed = decision?.allowed === true; + return decision?.allowed === true; } catch { - allowed = false; + return false; } - if (allowed) return undefined; +} + +/** + * [#7900] The run-state read gate itself: {@link mayReadRunState} as a guard + * clause. + * + * Returns a refusal result when the answer is no, `undefined` when the read may + * proceed — so the caller reads as a guard clause and no route can accidentally + * consume a "denied" as a value. + */ +async function refuseUngrantedRunRead( + deps: DomainHandlerDeps, + context: HttpProtocolContext, +): Promise { + if (await mayReadRunState(deps, context)) return undefined; // The refusal names the GRANT it wants and nothing about the caller — no // positions, no permission-set names (#7450: a denial must not answer the @@ -233,6 +268,104 @@ async function refuseUngrantedRunRead( }; } +/** + * [#7968] The screen route's gate: **the run's own trigger identity, OR the + * `sys_automation_run` read grant as an operator override.** + * + * Maintainer ruling, 2026-08-12 (Option B). Acceptance, verbatim: *"stranger + * with valid auth + run id ⇒ denied; triggering user ⇒ screen; holder of + * `sys_automation_run` read ⇒ screen."* + * + * ## ⛔ Why this is NOT the grant check one route up + * + * The obvious gate — require the `sys_automation_run` grant, exactly as + * `/:name/runs/:runId` does — was **considered and ruled out for this route**, + * and the reason is the whole point of the card: it would **refuse the end user + * the flow paused for**. The pause exists because the flow is asking THIS + * caller to fill a form in; a screen served only to grant-holders is a screen + * served to everyone except its audience. So the grant is the OVERRIDE half + * here (operator tooling, support), never the whole question — and the + * over-block direction is pinned as hard as the under-block one + * (`automation-screen-read-gate.test.ts`). + * + * ## What the identity half reads, and why that field + * + * `ExecutionLogEntry.trigger.userId` — the caller whose request started the run, + * written by the engine's single `buildRunTrigger` chokepoint (#7533) at every + * site that records a run. It is the only identity the run itself carries, and + * it is the same axis `resume` answers on (`resumeAuthority`, #3801 / #5561), + * so read and write on one pause stay on one axis rather than the two unrelated + * permissions #7900 exists to remove. + * + * ⚠️ It is deliberately NOT the richer per-run authority question — "may this + * caller resume THIS suspension, per its declared `resumeAuthority`/assignee + * state". That is Option A, recorded as the coherent end state and ADR-0019 + * class design work; B does not preclude it, because both refuse the same + * stranger and admit the same end user. + * + * ## Order of operations — the 404 comes FIRST, on purpose + * + * Unlike the #7900 gate (which fires before the automation service is consulted + * at all), this one runs AFTER `getSuspendedScreen`, and that ordering is a + * decision with two reasons: + * + * 1. **A nonexistent run id must keep answering exactly as it does today.** + * The gate's identity half is derived from the run, so an unresolvable run + * would have to fail CLOSED — turning today's `404 No pending screen for + * run` into a 403 for every caller, including the honest ones who mistyped. + * Deciding only where there IS a screen to disclose keeps every 404 path + * byte-identical. + * 2. **It cannot be asked earlier anyway.** The identity is a property of the + * run, so the run must be looked up before the question exists. Nothing is + * disclosed by the lookup: a refused caller gets the refusal, never the + * spec. + * + * The consequence, stated rather than hidden: a stranger can still tell a + * paused run id (403) from an unknown one (404). That existence oracle is the + * price of leaving the not-found behaviour untouched, it is strictly narrower + * than the disclosure it replaces (an id, not the record's values), and closing + * it means answering 404 for the refused caller — a different, defensible + * design that is not what was ruled. + * + * ## The non-denials it inherits + * + * Everything {@link mayReadRunState} decides: a system context passes, a + * deployment with no `plugin-security` (or a partial one) passes, and an + * `explain` that throws fails CLOSED — but only the OVERRIDE half fails closed, + * so the triggering user still gets their own screen while the permission + * subsystem is unavailable. That asymmetry is the point of a two-half gate: the + * end user's access does not depend on operator infrastructure. + */ +async function refuseUnrelatedScreenRead( + deps: DomainHandlerDeps, + context: HttpProtocolContext, + automationService: Partial, + runId: string, +): Promise { + const ec = context?.executionContext; + if (ec?.isSystem === true) return undefined; + + // ── Half 1: the run's own trigger identity ─────────────────────────────── + // Best-effort: `getRun` is optional on `IAutomationService`, and a service + // that cannot answer who triggered a run simply does not admit anyone on + // this half — it never admits everyone. A throw is the same: unresolved, + // not granted. + const callerId = typeof ec?.userId === 'string' && ec.userId !== '' ? ec.userId : undefined; + if (callerId && typeof automationService.getRun === 'function') { + const run = await automationService.getRun(runId).catch(() => undefined); + const triggerUserId = (run as { trigger?: { userId?: unknown } } | null | undefined)?.trigger?.userId; + if (typeof triggerUserId === 'string' && triggerUserId === callerId) return undefined; + } + + // ── Half 2: the operator override, asked as ONE question with #7900 ────── + if (await mayReadRunState(deps, context)) return undefined; + + return { + handled: true, + response: deps.error(SCREEN_READ_DENY_MESSAGE, RUN_READ_DENY_STATUS, { code: RUN_READ_DENY_CODE }), + }; +} + /** * [#8055] A refusal thrown by `registerFlow` is the CALLER's metadata being * wrong — serve it as one. @@ -348,6 +481,8 @@ function flowDefinitionRefusal(err: any): unknown { * ⚑ run-state read — `sys_automation_run` grant (#7900) * POST /:name/runs/:runId/resume → resume a paused run (screen input / ADR-0019) * GET /:name/runs/:runId/screen → the screen a paused run awaits + * ⚑ run's trigger identity OR the + * `sys_automation_run` grant (#7968) */ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: string, method: string, body: any, context: HttpProtocolContext, query?: any): Promise { // [#5519] ANONYMOUS BASELINE — the same floor `/data`, `/meta`, `/ai` and @@ -750,35 +885,28 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str // GET /:name/runs/:runId/screen → the screen a paused run awaits // (refresh-safe re-fetch for the UI flow-runner). // - // [#7900 AUDIT — stays authenticated-only, with a reason] This is the - // one run-scoped read the `sys_automation_run` grant is NOT applied to, - // and the omission is a decision rather than an oversight, so it is - // recorded here rather than only in the PR: - // - // - It is the END USER's surface, not the operator's. The pause exists - // because the flow is asking THIS caller to fill a form in; the - // trigger response already handed them the same `screen` inline, and - // this route exists so a browser refresh does not lose it. Requiring - // an operator grant here would refuse the screen to the very person - // the flow paused for — a breakage, not the narrowing the ruling - // prices in ("operator tooling … now needs the grant"). - // - Its WRITE sibling one route up already answers on a different - // axis: `resume` is gated in the engine by the suspension's declared - // `resumeAuthority` (#3801 / #5561) — a per-run authority model, not - // an object grant. Read and write on the same pause answering to two - // unrelated permissions would be the incoherence this card is about. - // - // The residual is real and is NOT claimed closed: a `ScreenSpec` carries + // [#7968] GATED — the run's own trigger identity, OR the + // `sys_automation_run` read grant as an operator override. The audit + // that #7900 left here recorded, correctly, that the grant ALONE is the + // wrong gate for this route (it would refuse the end user the flow + // paused for) — and left the door authenticated-only as a result. The + // residual it named was measured and is real: a `ScreenSpec` carries // `defaults` / `defaultValue` interpolated against the live flow - // variables (`builtin/screen-nodes.ts`), so an authenticated caller who - // knows a run id can still read record-derived values through this door. - // Closing it wants the per-run authority read gate the resume path - // already has, which is a different mechanism from this card's ruling — - // filed as its own issue and linked from the PR. + // variables (`builtin/screen-nodes.ts`), so a real screen flow over + // `{record.email}` / `{record.phone}` answered those values to ANY + // authenticated caller who knew a run id. The ruling of 2026-08-12 + // closes it on the identity axis instead, keeping the end user in. + // Reasoning, the ordering, and what stays out of scope (Option A, the + // per-run `resumeAuthority` read gate): `refuseUnrelatedScreenRead`. if (parts[1] === 'runs' && parts[2] && parts[3] === 'screen' && m === 'GET') { if (typeof automationService.getSuspendedScreen === 'function') { const screen = await automationService.getSuspendedScreen(parts[2]); + // Deliberately AHEAD of the gate: no screen ⇒ nothing to + // disclose ⇒ every not-found answer stays exactly what it was, + // for every caller. See the gate's "order of operations". if (!screen) return { handled: true, response: deps.error('No pending screen for run', 404) }; + const refusal = await refuseUnrelatedScreenRead(deps, context, automationService, parts[2]); + if (refusal) return refusal; return { handled: true, response: deps.success({ runId: parts[2], screen }) }; } return { handled: true, response: deps.error('Screen lookup not supported', 501) }; From 7bc227239bb73b7c4be84210ca484acea0bb83b7 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 21:14:27 +0000 Subject: [PATCH 2/2] test(runtime): assert the screen route's routing claim on its answer, not on getRun being unused (#7968) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `should get the pending screen via GET /:name/runs/:runId/screen` claimed that the screen path is not swallowed by the `/:name/runs/:runId` branch below it, and asserted it as "getRun was never called". The #7968 gate reads the run to resolve its trigger identity, so that proxy no longer tracks the claim. Asserted on the answer instead: the caller gets the screen envelope (`{ runId, screen }`) and not the `ExecutionLogEntry` the run-detail branch serves verbatim — the mock's entry is `{ id, status }`, so the two are distinguishable by shape. The routing claim is now pinned more directly than before. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01B3Kurx8qufrDzNjk4rag7V --- packages/runtime/src/http-dispatcher.test.ts | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/packages/runtime/src/http-dispatcher.test.ts b/packages/runtime/src/http-dispatcher.test.ts index 38e275cc44..d49e7e829e 100644 --- a/packages/runtime/src/http-dispatcher.test.ts +++ b/packages/runtime/src/http-dispatcher.test.ts @@ -485,7 +485,19 @@ describe('HttpDispatcher', () => { expect(mockAutomationService.getSuspendedScreen).toHaveBeenCalledWith('run_1'); expect(result.response?.body?.data?.screen?.nodeId).toBe('collect'); // `screen` must NOT be swallowed by the getRun route below it. - expect(mockAutomationService.getRun).not.toHaveBeenCalled(); + // + // [#7968] Asserted on the ANSWER, not on `getRun` being unused: the + // screen route now reads the run to resolve its trigger identity + // for the read gate, so "getRun was never called" stopped being a + // proxy for "the path did not fall through". What the routing claim + // actually says is that the caller got the SCREEN envelope + // (`{ runId, screen }`) and not the `ExecutionLogEntry` the + // `/:name/runs/:runId` branch serves verbatim — which the mock + // makes distinguishable: that entry is `{ id: 'run_1', status: + // 'completed' }`, carrying neither key below. + expect(result.response?.body?.data?.runId).toBe('run_1'); + expect(result.response?.body?.data?.id).toBeUndefined(); + expect(result.response?.body?.data?.status).toBeUndefined(); }); it('should return 404 when the run is not awaiting a screen', async () => {