fix(automation): one chokepoint for the resume signal — output reopened the hole inputs had just closed (#3879) - #3880
Merged
Conversation
…ened 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 { "output": { "$mapItemDone": true, "$mapItemOutput": … } } 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. The seam had three open-coded writers into one variable map (`output` prefixed, `variables` bare, the engine's own map handoff), so "guard the field that was exploited" was always going to invite the next field. Structural fix: - applyResumeSignal is the ONE place a resume signal reaches the variable map. Both fields become a single write list (already in final, prefixed form), checked, then applied — a new signal field is covered by construction. - All-or-nothing, and checked before the suspension is consumed: a rejected signal applies nothing and the run stays parked, so the real continuation still lands. - The engine owns the rule; the transport maps the verdict. resume returns code 'invalid_signal', the route answers 400. The SDK and any future adapter inherit it. This corrects #3853's placement argument: "strict at the untrusted boundary" is right about where a rule BINDS, not where it LIVES. - Engine-built signals (subflow output mapping, map item handoff) are exempt via a module-private symbol — deliberately not RESUME_AUTHORITY_SERVICE, which answers a different question and does not license writing internals. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013gvN32u1EiuvY9uQEMJiMR
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
github-actions
Bot
added
documentation
Contributor
📓 Docs Drift CheckThis PR changes 3 package(s): 113 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
os-zhuang
marked this pull request as ready for review
July 28, 2026 14:30
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3879. Third pass on the same seam (#3801 → #3853 → here), and the reason it needed a third is the point of the PR.
The hole
#3853 guarded
signal.variablesat the route and passedsignal.outputthrough unchecked. Butoutputkeys are merged under the suspended node's id (engine.ts:2214):and for a run parked on a
mapnoderun.nodeIdis the map node — so the same forgery lands through the other field:POST .../runs/{parentRunId}/resume { "output": { "$mapItemDone": true, "$mapItemOutput": { "result": "FORGED via output" } } } → { started: 2, results: [ { "result": "FORGED via output" } ] }Scope, stated precisely. The #3853 map gate still held: a batch whose pending item sits on an
approvalis refused before any of this, so the approval bypass stayed closed. The residual was forging the recorded result of an item on an ungatedscreen/waitpause — map-state corruption, not a decision bypass.outputalso can't reach bare$runId/$record(it is always node-prefixed).Why this isn't a third patch
Two escapes with one shape is a design signal. The seam had three open-coded writers into one variable map —
outputprefixed,variablesbare, and the engine's own map handoff — so "guard the field that was exploited" was always going to invite the next field.applyResumeSignalis 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.resumereturns{ success: false, code: 'invalid_signal' }; the route answers 400 and its own copy of the check is deleted. The SDK and any future adapter inherit it — implemented in one transport it protected exactly one transport, and one field of it.ENGINE_BUILT_SIGNAL), stamped bybubbleToParentand the subflow output mapping — the only legitimate writers of the handoff keys, unreachable from a transport. Deliberately notRESUME_AUTHORITY_SERVICE: that marker answers "the owning service authorized this decision", and a service still has no business writing engine internals. Two questions, two markers.This corrects #3853's placement argument in the ADR: "strict at the untrusted boundary" is right about where a rule binds, not where it lives.
Consumer impact
AutomationResult.codegains'invalid_signal'alongside'forbidden'— aswitchover it needs a new arm; a plain read doesn't. Authoring is unchanged: ordinary variables pass,$mid-name (price$) and dotted names (collect.note) included; only$…or a.$segment is refused.Tests
resume-authority-gate.test.ts(19): the forgery is refused through both fields viait.each— the point being that they are one rule, not two — with$mapStateunchanged and the pause still live; the legitimate engine bubble still writes the handoff (proving the guard didn't break the mechanism it protects); a bare$runIdrewrite on an ordinary screen resume is refused and the legitimate key alongside it is not applied (all-or-nothing); ordinary author names still pass.http-dispatcher.test.ts(200) now asserts the route forwards both fields unfiltered and mapsinvalid_signal→ 400 — the rule is tested where it lives, not twice.Verified:
service-automation(35 files),plugin-approvals(12),spec(262),runtime(50 files / 716 tests) green;eslint --no-inline-configclean;check:docs,check:api-surface,check:doc-authoring,tsc --noEmiton spec all clean.Separately
While verifying this I checked the run read routes:
GET /automation/:name/runs/:runIdand.../screenhave no per-run authorization — access stops at environment membership, so any member can read any run's screen or step log by id. I have not judged whether that's acceptable (run ids are uuids; membership may be the intended boundary), so it is not claimed as a vulnerability here and nothing in this PR changes it. Flagging for a product call.Generated by Claude Code