Completions: a job callback may not answer a human approval - #29
Open
nishanthvonteddu wants to merge 1 commit into
Open
nishanthvonteddu wants to merge 1 commit into
nishanthvonteddu wants to merge 1 commit into
Conversation
`S16_COMPLETION_TOKEN` is deliberately separate from the control token so a
remote job service can report the work it was handed without also gaining
authority. auth.py says so: the holder "should be able to complete work it
was given without also being able to write subscriptions."
But `/completions` takes `event_type` from the caller and passes it
straight to `complete_waiting`, which matches on `(handle, event_type)`.
Parked handles are published by `GET /v1/agent/runs/{id}`, so a holder who
reads one could send `approval.received` and satisfy a `request_approval`
node -- answering, with a machine credential, a question that was asked of
a person.
`complete_waiting` now takes `refuse_skills`, and the route passes the
`human_gate` family. Which capability parks for a person stays a property
of the capability rather than a name this route memorises, matching how
the channel reply path already decides the same thing.
The refusal is a 403 and the node stays waiting, rather than a silent
no-op that would read like an unknown handle.
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
1 participant
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.
A holder of the completion token could satisfy a human approval gate.
What breaks
S16_COMPLETION_TOKENis deliberately separate from the control token.auth.pysays why: the holder "should be able to complete work it was given without also being able to write subscriptions."But
/completionstakesevent_typefrom the request body and passes it straight through:complete_waitingmatches on(handle, event_type). Nothing checks what is parked on that handle. So a job service that sendsevent_type: "approval.received"satisfies arequest_approvalnode — answering, with a machine credential, a question that was asked of a person.Handles are not secret:
GET /v1/agent/runs/{id}publishes them in each waiting node.How to see it break
Park a run on
request_approval, read the handle fromGET /v1/agent/runs/{id}, then:Before this change:
200, and the gate is released.The fix
complete_waitinggainsrefuse_skills; the route passes thehuman_gatefamily. Which capability parks for a person stays a property of the capability rather than a name this route memorises — the same way the channel reply path already decides it.The refusal is a 403 and the node stays waiting, rather than a silent no-op that would read like an unknown handle.
Test
test_a_job_callback_cannot_satisfy_a_human_approval— parks a real approval, attempts the completion with the completion token, asserts 403 and that the gate is still waiting. Fails before, passes after.