Skip to content

Commit aff7376

Browse files
authored
fix(tasks): require owner permission to cancel a task via REST (#308)
## Problem `POST /tasks/{task_id}/cancel` authorizes `AuthorizedOperationType.update`. Editors hold `update`, so a task editor who is not the owner can cancel a task through the REST route. Cancellation is intended to be **owner-only**. The RPC `task/cancel` path already authorizes the owner-only `cancel` action, so the two entry points to the same operation disagreed. ## Fix Authorize `cancel` on the REST route, matching the RPC path and the schema. The sibling lifecycle routes (`complete`/`fail`/`terminate`/`timeout`) intentionally keep `update` — those are state transitions, not the owner-only cancel. ## Tests Adds a unit test pinning the REST cancel route to the `cancel` action (mirrors the existing per-RPC operation-routing tests). End-to-end behavioral coverage lives in the authorization e2e suite. <!-- greptile_comment --> <h3>Greptile Summary</h3> This PR fixes a privilege escalation bug in the REST `POST /tasks/{task_id}/cancel` endpoint where it previously authorized `update` (allowing editors), but cancellation is owner-only. The fix aligns the REST route with the RPC `task/cancel` path by switching to `AuthorizedOperationType.cancel`. - **`tasks.py`**: Changes `cancel_task`'s `DAuthorizedId` call from `AuthorizedOperationType.update` to `AuthorizedOperationType.cancel`, adding a clarifying comment about the intent. - **`test_tasks_authz.py`**: Adds `TestRestCancelRouteRequiresOwner` — a unit test that introspects the function signature to assert the dependency checks `cancel`, mirroring the existing per-RPC routing tests. <details><summary><h3>Confidence Score: 5/5</h3></summary> Safe to merge — the change is a one-line auth operation swap on a single route, and the new test directly pins the expected behavior. The change is minimal and surgical: one enum value replaced in one route, with a well-scoped unit test that introspects the function signature at test time to verify the correct operation is wired. The sibling lifecycle routes (complete/fail/terminate/timeout) are intentionally left on update and are unaffected. The RPC path already used cancel and is also untouched. No edge cases, no new dependencies, no schema changes. No files require special attention. </details> <h3>Important Files Changed</h3> | Filename | Overview | |----------|----------| | agentex/src/api/routes/tasks.py | Single-line auth fix: cancel_task's DAuthorizedId now uses AuthorizedOperationType.cancel instead of update; comment explains the intentional difference from sibling lifecycle routes. | | agentex/tests/unit/api/test_tasks_authz.py | New TestRestCancelRouteRequiresOwner class introspects the cancel_task signature at test time and asserts the dependency resolves to the cancel operation; mirrors existing RPC routing test patterns. | </details> <details><summary><h3>Sequence Diagram</h3></summary> ```mermaid sequenceDiagram participant Client participant REST as REST POST /tasks/{id}/cancel participant RPC as RPC task/cancel participant Authz as Authorization Service Note over REST,Authz: Before this PR (bug) Client->>REST: "POST /tasks/{id}/cancel" REST->>Authz: check(task, update) editors allowed Authz-->>REST: authorized (editor or owner) REST-->>Client: task canceled Note over REST,Authz: After this PR (fix) Client->>REST: "POST /tasks/{id}/cancel" REST->>Authz: check(task, cancel) owner-only Authz-->>REST: authorized (owner only) REST-->>Client: task canceled Client->>RPC: task/cancel RPC->>Authz: check(task, cancel) owner-only (unchanged) Authz-->>RPC: authorized (owner only) RPC-->>Client: task canceled ``` </details> <sub>Reviews (1): Last reviewed commit: ["fix(tasks): require owner permission to ..."](a186b78) | [Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=37712273)</sub> <!-- /greptile_comment -->
1 parent d65011a commit aff7376

2 files changed

Lines changed: 33 additions & 1 deletion

File tree

agentex/src/api/routes/tasks.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,10 @@ async def fail_task(
258258
description="Mark a running task as canceled.",
259259
)
260260
async def cancel_task(
261-
task_id: DAuthorizedId(AgentexResourceType.task, AuthorizedOperationType.update),
261+
# Cancel is owner-only — unlike the editor-allowed lifecycle transitions
262+
# (complete/fail/terminate/timeout) which authorize ``update`` — so it
263+
# checks the ``cancel`` action, matching the RPC task/cancel path.
264+
task_id: DAuthorizedId(AgentexResourceType.task, AuthorizedOperationType.cancel),
262265
task_use_case: DTaskUseCase,
263266
request: TaskStatusReasonRequest | None = None,
264267
) -> Task:

agentex/tests/unit/api/test_tasks_authz.py

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,35 @@ async def test_unwired_method_fails_closed_with_not_implemented(self):
209209
await _authorize_rpc_request(request, authorization, task_service)
210210

211211

212+
@pytest.mark.unit
213+
@pytest.mark.asyncio
214+
class TestRestCancelRouteRequiresOwner:
215+
"""The REST ``POST /tasks/{id}/cancel`` route authorizes the owner-only
216+
``cancel`` action — matching the RPC task/cancel path, and distinct from the
217+
editor-allowed ``update`` used by the sibling lifecycle routes
218+
(complete/fail/terminate/timeout)."""
219+
220+
async def test_rest_cancel_route_checks_cancel_operation(self):
221+
import inspect
222+
223+
from src.api.routes.tasks import cancel_task
224+
225+
annotation = inspect.signature(cancel_task).parameters["task_id"].annotation
226+
dep = _dep_callable(annotation)
227+
228+
authorization = MagicMock()
229+
authorization.check = AsyncMock(return_value=True)
230+
repos = (MagicMock(), MagicMock(), MagicMock())
231+
232+
result = await dep(authorization, *repos, "task-c")
233+
234+
assert result == "task-c"
235+
assert (
236+
authorization.check.await_args.kwargs["operation"]
237+
== AuthorizedOperationType.cancel
238+
)
239+
240+
212241
def test_cancel_operation_wire_format_matches_agentex_auth_contract():
213242
"""Cross-repo enum contract: the literal string ``"cancel"`` is the wire
214243
format shared with agentex-auth's mirror enum. Diverging strings would

0 commit comments

Comments
 (0)