fix(automation): one chokepoint for the resume signal — output reopened the hole inputs had just closed (#3879)

.changeset/resume-signal-chokepoint.md

@@ -0,0 +1,51 @@
+---
+"@objectstack/spec": minor
+"@objectstack/service-automation": patch
+"@objectstack/runtime": patch
+---
+
+fix(automation): one chokepoint for the resume signal — `output` reopened the hole `inputs` had just closed (#3879)
+
+#3853 guarded `signal.variables` at the route. That closed one of **two**
+equivalent paths into the same variable map and left the other open:
+`signal.output` keys are merged under `${run.nodeId}.${key}`, and for a run
+parked on a `map` node `run.nodeId` **is** the map node — so
+
+```jsonc
+{ "output": { "$mapItemDone": true, "$mapItemOutput": { "result": "FORGED" } } }
+```
+
+writes exactly the `<mapNodeId>.$mapItemDone` the `inputs` guard had refused,
+making the map record a result for an item nobody decided. Demonstrated with a
+repro, then fixed.
+
+Scope: the #3853 map gate still held, so a batch whose pending item sits on an
+`approval` was refused before any of this — the **approval bypass stayed
+closed**. The residual was forging the recorded result of an item on an
+*ungated* pause.
+
+Two escapes with one shape is a design signal, not two bugs, so the fix is
+structural rather than a third patch:
+
+- **`applyResumeSignal` is the one place a resume signal reaches the variable
+ map.** Both fields are collected into a single write list (already in final,
+ prefixed form), checked, then applied — a new signal field is covered by
+ construction rather than by remembering.
+- **All-or-nothing**, and checked *before* the suspension is consumed: a
+ rejected signal applies nothing (not even legitimate keys sent alongside) and
+ the run stays parked, so the real continuation still lands.
+- **The engine owns the rule; the transport maps the verdict.** `resume` returns
+ `{ success: false, code: 'invalid_signal' }`; the route answers **400**. The
+ SDK and any future adapter inherit it — implemented in one transport it
+ protected exactly one transport, and one field of it.
+- Engine-built signals (the subflow output mapping, the map item handoff) are
+ exempt via a module-private symbol. Deliberately *not*
+ `RESUME_AUTHORITY_SERVICE`: that marker means "the owning service authorized
+ this decision", and a service still has no business writing engine internals.
+
+`AutomationResult.code` gains `'invalid_signal'` alongside `'forbidden'` — a
+`switch` over it needs a new arm; a plain read does not.
+
+Nothing changes for authoring: ordinary variables pass, `$` mid-name (`price$`)
+and dotted names (`collect.note`) included. Only names the engine reserves —
+`$…` or a `.$` segment — are refused.

content/docs/automation/flows.mdx

@@ -424,11 +424,16 @@ the item rather than the loop:
  decision is still open. Refused while that item is service-gated; the map
  moves on when the item completes through its owning service.
 
-Two related rules on the same route: **resume `inputs` may not write the
-engine's `$` namespace** (`$runId`, `$record`, `$flowName`,
-`<nodeId>.$mapItemDone`, …) — those are the engine's own handoff variables, and
-a caller who could set them could forge a map item's recorded result. Ordinary
-author-declared variables are unaffected; a reserved name answers **400**.
+A second rule on the same seam: **a resume signal may not write the engine's
+`$` namespace** (`$runId`, `$record`, `$flowName`, `<nodeId>.$mapItemDone`, …) —
+those are the engine's own handoff variables, and a caller who could set them
+could forge a map item's recorded result or re-point the run id an `approval` /
+`wait` node correlates on. It covers **both** signal fields: `inputs` land under
+their plain names, and `output` keys land under the *suspended node's* id —
+which for a map-parked run is the map node itself, i.e. the very same reserved
+key. A reserved name answers **400**, nothing is applied (not even legitimate
+keys sent alongside it), and the run stays parked. Ordinary author variables are
+unaffected, `$` mid-name (`price$`) included.
 
 Registering a pausing node of your own? Declare `resumeAuthority: 'service'` on
 its descriptor when the decision to continue belongs to your service rather

docs/adr/0019-approval-as-flow-node.md

@@ -277,3 +277,43 @@ segment) with a 400. Deliberately at the transport and not in the engine: `bubbl
 writes those keys in-process, and this is the same trust split the gate itself uses — strict at the
 untrusted boundary, unrestricted for the code that already holds the authority. Refuse rather than
 silently strip, so a mis-authored screen input fails at the door instead of many nodes downstream.
+
+> **Half of this was wrong — see the addendum below (#3879).** Guarding `inputs` at the transport left
+> `output` untouched, and `output` keys land under `${nodeId}.${key}`, which for a map-parked run is
+> the map node itself — the identical reserved key, forgeable through the other field. The *placement*
+> argument above was the error: "strict at the untrusted boundary" is right about where the rule
+> BINDS, not about where it LIVES. The rule moved into the engine, at the one place a signal touches
+> the variable map.
+
+## Addendum (2026-07-28, #3879) — one chokepoint for the resume signal, because guarding a field at a time failed twice
+
+The addendum above guarded `signal.variables` **at the route**. That closed one of two equivalent
+paths into the same variable map and left the other open: `signal.output` keys are merged under
+`${run.nodeId}.${key}`, and for a run parked on a `map` node `run.nodeId` **is** the map node — so
+`{ "output": { "$mapItemDone": true, "$mapItemOutput": … } }` writes exactly the
+`<mapNodeId>.$mapItemDone` the `inputs` guard had just refused. Demonstrated, then fixed.
+
+Note what the map gate (#3853) still bought: a batch whose pending item sits on an `approval` is
+refused before any of this, so the **approval bypass stayed closed**. The residual was forging the
+recorded result of an item on an *ungated* pause — map-state corruption, not a decision bypass.
+
+Two escapes with one shape is a design signal, not two bugs. The seam had **three** open-coded writers
+into one variable map (`output` prefixed, `variables` bare, and the engine's own map handoff), so
+"guard the field that was exploited" was always going to invite the next field. The fix is structural:
+
+- **`applyResumeSignal` is the one place a resume signal reaches the variable map.** Both fields are
+ collected into a single write list — already in final, prefixed form — checked, then applied. A new
+ signal field is covered by construction rather than by remembering.
+- **All-or-nothing.** A rejected signal applies nothing, not even legitimate keys sent alongside, and
+ the check runs *before* the suspension is consumed, so the run stays parked and the real
+ continuation still lands.
+- **The engine owns the rule; the transport maps the verdict.** `resume` returns
+ `{ success: false, code: 'invalid_signal' }` and the route answers 400. This corrects the placement
+ argument in the previous addendum: "strict at the untrusted boundary" is right about where a rule
+ BINDS, not where it LIVES — implemented in the transport it protected exactly one transport and one
+ field of it, and the SDK, any future adapter, and `output` all sat outside it.
+- **Engine-built signals are exempt via a module-private symbol** (`ENGINE_BUILT_SIGNAL`), stamped by
+ `bubbleToParent` and the subflow output mapping — the only writers that legitimately set the handoff
+ keys, and unreachable from a transport. Deliberately *not* `RESUME_AUTHORITY_SERVICE`: that marker
+ answers "the owning service authorized this decision", and a service still has no business writing
+ engine internals. Two different questions, two different markers.

packages/runtime/src/domains/automation.ts

@@ -216,38 +216,33 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str
  // `ApprovalService`, which records the decision and enforces the slate —
  // on a SYMBOL-keyed marker. Assembling the signal field-wise (never
  // spreading the body) keeps that unforgeable even if a caller invents
- // extra keys; a refused resume comes back `code: 'forbidden'` and is
- // answered 403 rather than a 200 carrying `success: false`.
+ // extra keys.
+ //
+ // Two REFUSAL codes come back from the engine and are answered as such
+ // rather than a 200 carrying `success: false` (which reads as "your
+ // resume ran and the flow failed"):
+ // forbidden → 403, the suspension is service-owned (#3801)
+ // invalid_signal → 400, the signal wrote the engine's `$` variable
+ // namespace (#3853 follow-up)
+ // Both are enforced in the ENGINE, at the one place a signal reaches the
+ // variable map — deliberately not re-implemented here. Guarding a field
+ // at a time in the transport is what let `output` reopen the hole
+ // `inputs` had just closed; every transport now inherits one rule.
  if (parts[1] === 'runs' && parts[2] && parts[3] === 'resume' && m === 'POST') {
  if (typeof automationService.resume === 'function') {
  const b = (body && typeof body === 'object') ? body : {};
  const inputs = (b.inputs ?? b.variables);
  const signal: any = {};
- if (inputs && typeof inputs === 'object') {
- // #3853: `inputs` land as BARE flow variables, and `$` is the
- // engine's own variable namespace (`$runId`, `$record`,
- // `$flowName`, `<nodeId>.$mapItemDone`/`$mapItemOutput`/
- // `$mapState`, …). A caller who could write those could forge
- // the map node's item handoff — recording a per-item result
- // for an approval nobody made — or re-point `$runId`, which is
- // how approval/wait nodes correlate external state back to a
- // run. Author-declared variables never live in that namespace,
- // so refuse rather than silently drop: a screen whose input is
- // quietly discarded fails much further downstream.
- const reserved = Object.keys(inputs).filter(k => k.startsWith('$') || k.includes('.$'));
- if (reserved.length) {
- return { handled: true, response: deps.error(
- `Resume inputs may not set engine-internal variables (${reserved.join(', ')}) — ` +
- `names starting with '$' (or containing '.$') are reserved by the flow engine`, 400) };
- }
- signal.variables = inputs;
- }
+ if (inputs && typeof inputs === 'object') signal.variables = inputs;
  if (b.output && typeof b.output === 'object') signal.output = b.output;
  if (typeof b.branchLabel === 'string') signal.branchLabel = b.branchLabel;
  const result = await automationService.resume(parts[2], signal);
  if (result?.success === false && result.code === 'forbidden') {
  return { handled: true, response: deps.error(result.error ?? 'Resume forbidden', 403) };
  }
+ if (result?.success === false && result.code === 'invalid_signal') {
+ return { handled: true, response: deps.error(result.error ?? 'Invalid resume signal', 400) };
+ }
  return { handled: true, response: deps.success(result) };
  }
  return { handled: true, response: deps.error('Resume not supported', 501) };

packages/runtime/src/http-dispatcher.test.ts

@@ -358,33 +358,34 @@ describe('HttpDispatcher', () => {
  expect(result.response?.body?.data?.success).toBe(false);
  });
 
- // #3853: `inputs` land as BARE flow variables, so a caller who could
- // write the engine's `$` namespace could forge the `map` node's item
- // handoff (recording a per-item result for an approval nobody made) or
- // re-point `$runId`, which is how approval/wait nodes correlate.
- it('should refuse resume inputs that write engine-internal variables', async () => {
- for (const inputs of [
- { 'signoffs.$mapItemDone': true, 'signoffs.$mapItemOutput': { forged: true } },
- { $runId: 'someone_elses_run' },
- { $record: { id: 'other' } },
- ]) {
- const result = await dispatcher.handleAutomation(
- 'flow_a/runs/run_1/resume', 'POST', { inputs }, { request: {} },
- );
- expect(result.response?.status).toBe(400);
- expect(result.response?.body?.error?.message).toMatch(/reserved by the flow engine/);
- }
- // Refused at the door — the engine is never asked.
- expect(mockAutomationService.resume).not.toHaveBeenCalled();
+ // #3853 follow-up: the reserved-name rule lives in the ENGINE, at the one
+ // place a signal reaches the variable map — the route only maps its
+ // verdict onto a status. (Guarding one body field at a time here is what
+ // let `output` reopen the hole `inputs` had just closed.)
+ it('should answer 400 when the engine rejects the signal as engine-internal', async () => {
+ mockAutomationService.resume.mockResolvedValue({
+ success: false, code: 'invalid_signal',
+ error: "Resume signal may not set engine-internal variables (signoffs.$mapItemDone) — " +
+ "names starting with '$' (or containing '.$') are reserved by the flow engine",
+ });
+ const result = await dispatcher.handleAutomation(
+ 'flow_a/runs/run_1/resume', 'POST',
+ { output: { $mapItemDone: true } }, { request: {} },
+ );
+ expect(result.response?.status).toBe(400);
+ expect(result.response?.body?.error?.message).toMatch(/reserved by the flow engine/);
  });
 
- it('should still accept ordinary screen inputs alongside the reserved-name guard', async () => {
+ // Both body fields reach the engine verbatim — it, not the route, decides.
+ it('should forward `output` and `inputs` unfiltered for the engine to judge', async () => {
  await dispatcher.handleAutomation(
  'flow_a/runs/run_1/resume', 'POST',
- { inputs: { new_assignee: 'ada', 'collect.note': 'hi', price$: 3 } }, { request: {} },
+ { inputs: { new_assignee: 'ada', 'collect.note': 'hi', price$: 3 }, output: { decision: 'ok' } },
+ { request: {} },
  );
  expect(mockAutomationService.resume).toHaveBeenCalledWith('run_1', {
  variables: { new_assignee: 'ada', 'collect.note': 'hi', price$: 3 },
+ output: { decision: 'ok' },
  });
  });