Approval gates: the grant binds a run and a step index, not the step
- Dominant language
- Rust
- Stars
- 32.7k
- Forks
- 4.3k
- Avg merge
- 1d 13h
- Merged PRs (30d)
- 253
Description
Read through #2376 and PR #2377, and #2509. This is additive to all three and
narrower than either: it is about what the grant is bound to at resume time.
The arm-then-announce ordering and the fail-closed approver resolution in #2377
both look like the right calls.
As far as I can follow the resume path:
- `generate_approval_token` returns a bare `Uuid::new_v4()`. The doc comment is
explicit that `run_id` and `step_id` are accepted for logging context and are
not mixed in, so the token identifies the gate but commits to nothing about
it.
- The `workflow_approvals` row carries the token hash and `step_index`.
- On grant, the handler reads the run, reads the workflow by
`run.workflow_id`, and calls `execute_from_step` with the stored index.
So the definition executed at resume is whatever is in the database when the
grant lands, and the resume point is an integer offset into it. Nothing ties
the grant to the definition that produced the announcement.
Three things follow, roughly in order of how much they would bother me:
1. **Insert a step and the index moves.** A run suspends at index 3. Someone
edits the workflow and adds a step above it. The grant resumes at index 3 of
the new definition, which is now a different step. The approver approved one
thing and another runs.
2. **Edit the step in place and the grant still validates.** A `send_message`
destination changes, or a `call_webhook` URL does. The token still hashes to
the same row, the run is still `WaitingApproval`, and the grant path has
nothing to notice with.
3. **The announcement text is not bound to anything.** kind:46010 carries
`message`, which is free text from the definition. Even with no edit at all,
the sentence in the approver's needs-action feed is asserted rather than
derived, so there is no way to check afterwards that the approver was shown
a description of what actually ran.
I do not know whether something upstream refuses to update a workflow that has
runs in `WaitingApproval`. I could not find one, but I have only read the tree
from the outside and may have missed it. If that guard exists, 1 and 2 mostly
go away and 3 still stands.
If it does not, the fix looks small and sits inside the surface #2377 already
touches. Store a digest over the definition, or just over the resolved step, on
the `workflow_approvals` row at arm time. Carry it in the kind:46010 content.
On grant, recompute it against the reloaded definition and refuse the resume on
mismatch. A mismatch is not a stale approval to be tolerated, it is a request
to do something else, so refusing and requiring a fresh gate seems like the
right default.
That would also give #2509's `verdict_ref` firmer ground, since a checkable
claim about an action is only worth as much as the guarantee that the action is
the one that runs.
Happy to write it as a patch on top of #2377, or to leave it as a note if you
would rather keep that PR tight and take it as a follow-up.
Tom Farley, ScopeBlind
Contributor guide
Assessment
This issue has not been assessed yet.