Repository navigation
fix(workflows): record the owner, then scope control to them (#17014) - #17666
Conversation
`ActiveWorkflow.owner_id` and the WebSocket's `may_control` both existed and both were inert: no creation path set an owner, so every workflow answered `owner_id is None`. An owner check against None admits nobody, which made the WebSocket admin-only, while REST `control_workflow` had no check at all -- any signed-in user could pause, cancel or approve any workflow. Recording and checking now share one identity function, because a check that compares a different claim than the one recorded silently never matches. - `workflow_ownership.py` (new): `caller_id`, `is_owner_or_admin`, `require_owner_or_admin`, `find_workflow`. An absent owner means admin-only, not open -- fail closed. - Owner recorded on every creation path that has a caller: the REST route, chat-derived (via the manager), templates, and the advanced-workflow coordinator. `workflow_serializer` already passed one. - Scoped to owner-or-admin: `control_workflow`, `start_workflow`, `workflow_status`, `present_plan`, `approve_plan`, `pending_approval`. - `approve_plan`'s docstring claimed authorization "by checking session_id matches the workflow" -- client-supplied, so it authorised whoever asked, and deferred the real check to a gateway that does not perform it. - Executor no longer resolves secrets under `workflow.session_id`. A session id is not a user id; an unowned workflow now resolves no user secrets rather than another namespace's, and creation logs that it will. - `except HTTPException: raise` on the routes gaining a 403. Without it the route's `except Exception` returned 500 for a refusal -- and already did for the pre-existing 404, so "workflow not found" was an Internal Server Error. - `approval_support.py` (new): `routes.py` was at 599 of its 600-line ceiling, so the two approval helpers moved out rather than the ceiling moving up. 599 -> 573. Refs #17009, #2304, #16628
Found by enumerating the routes whose `except Exception` converts an `HTTPException` raised in the try body into 500. `present_plan` is one of five, and it is the one this change had just given an ownership check -- so its refusal would have surfaced as "Internal server error". The cross-user test did not cover `/present_plan`, which is why the gap survived the first pass: an unlisted route is an unchecked route. It is in the parametrized list now, and an AST assertion over the file confirms no route carrying an ownership check still swallows its 403. The remaining four swallows are pre-existing and got no 403 from this change, so they stay out of scope and are filed separately.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 21 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughWorkflow creation now records the authenticated caller as the owner. Workflow routes check owner or admin access, and secret resolution uses the recorded owner. Approval validation and outcome handling use shared helpers. ChangesWorkflow ownership and access
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The identified access-control failures are not established for supported workflow users. No actionable merge-blocking risk remains after normal checks. Security Architecture ReviewSecurity architecture risk: 🟠 High · up to Workflow access is more restricted overall, but the new owner check can treat a changeable username as equivalent to the identity recorded for a workflow. Where a workflow records a username, another account that later obtains that name may gain control of it. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
Auto-regenerated (py3.14) to match the backend and/or SLM backend schema so the required verify-generated-types gate(s) pass. Triggered by auto-fix-generated-types.yml.
mrveiss
left a comment
mrveiss
left a comment
There was a problem hiding this comment.
Non-author review of dac33c9b5b18, on scope rather than correctness — is the change's reach what the body claims?
The stated exception is genuinely fail-closed. Verified, not taken.
is_owner_or_admin (workflow_ownership.py:49) returns False for a non-admin when owner_id is falsy, with the reasoning in the docstring — "the alternative reading ('nobody owns it, so anybody may') is the hole #17014 is about". And api/workflow.py's routes are each Depends(check_admin_permission): :389 list, :429 details, :449 status, :526 approve, :572 execute. So the :292 exception is a missing capability, not a missing guard — today an owner who is not an admin cannot act there at all. That is the right direction to be incomplete in, and #17655 is the right home for the +3 lines against a 1027-line baseline.
One correction to my own method, before the finding
My first pass counted is_owner_or_admin per route file and got 0 everywhere — including the files that plainly enforce. The guard is called inside find_workflow (workflow_ownership.py), which raises 404 for missing and 403 for not-permitted, and the routes call that: 11 uses in workflow_automation/routes.py, 3 in advanced_workflow/routes.py. A lookup that cannot be performed without the check is a better design than a check at each handler, and my grep at the endpoint would have reported the opposite. Naming it because it is the grep the endpoint, miss the shared helper trap and I nearly published it.
Reach: three of the four untouched surfaces have their own enforcement. One does not.
| file | enforcement at this head |
|---|---|
api/workflow.py |
admin-only on every route — the stated exception |
api/workflow_permissions.py |
its own require_workflow_permission("view"/"grant"), documented "Only owners and admins" |
api/workflow_secrets.py |
scopes every query by owner_id = current_user.get("user_id") |
api/workflow_export.py:59 |
authenticated only — no ownership scoping |
export_workflow takes current_user purely to log it:
doc = await serializer.export_workflow(workflow_id) # no owner argument
if doc is None:
raise HTTPException(status_code=404, …)
logger.info("User %s exported workflow %s", current_user.get("user_id"), workflow_id)
return {"success": True, "export": doc.to_dict()}So any signed-in user can export any workflow by id, and the export is the full definition. That is #17014's shape on the disclosure side rather than the control side — any signed-in user reaches another user's workflow — and it is not the stated exception, which is an admin-only route.
I am raising it as a reach question rather than a defect in this PR, and the judgement is not mine to make: if #17014 is scoped to control, export is a sibling and belongs on its own issue; if it is scoped to another user's workflow, then the body's "one exception" is two and the second is open to any authenticated caller rather than admin-gated. Either answer is defensible; what should not happen is the number being quoted as one when someone later asks whether #17014 can close.
I did not check /validate or /import on that router — import sets owner_id from the caller (:134), which looks right, and validate takes no workflow id. Nor did I look at the frontend types or the WebSocket may_control path.
Otherwise
25 tests with a negative control, one identity function shared by the recording and the checking path so a workflow cannot be recorded under one identity and checked against another, and Refs not Closes with the exception stated. On scope, everything the body claims holds except the export surface above.
1 participant
Thinking Path
ActiveWorkflow.owner_idalready existed. The WebSocket'smay_controlalready admitted"an admin, or the owner the workflow records". Both were inert, because no creation path
ever set an owner — so every workflow answered
owner_id is None, an owner check againstNonematched nobody, and the WebSocket was admin-only by accident rather than by design.REST
control_workflowhad no check at all, so any signed-in user could pause, cancel orapprove any workflow.
That shape drove the design: the identity a workflow records and the identity a check
compares must come from the same function, or the check silently never matches and looks
correct while doing nothing. Hence one
caller_idused by both sides rather than each sitepicking its own claim out of the user dict.
The second decision was what an absent owner means. "Nobody owns it, so anybody may" is the
hole itself; this fails closed — no owner means admin-only.
Two things surfaced while implementing that changed the work:
A defect that would have silently defeated the fix. These routes wrap their body in
try: ... except Exception: raise HTTPException(500). A 403 raised inside would havesurfaced as "Internal server error" — and the pre-existing 404 already did, so "workflow
not found" has been returning 500. A refusal indistinguishable from a bug is also a refusal
no test can assert, so
except HTTPException: raisewas required, not cosmetic.A ceiling.
routes.pysat at 599 of its 600-line limit. The ownership checks had togo somewhere, so the approval helpers moved out instead of the ceiling moving up.
What Changed
New —
services/workflow_automation/workflow_ownership.py(98 lines):caller_id,is_owner_or_admin,require_owner_or_admin,find_workflow. One home for the rule.Owner recorded on every creation path that has a caller (AC1):
workflow_automation/routes.py:79routes.py:274→manager.py(newowner_idparameter)api/templates.py— route gainedcurrent_user; the router's admin gate returns a bool, so there was no user object to recordadvanced_workflow/coordinator.py→ its two routes inadvanced_workflow/routes.pyservices/workflow_serializer.py— already passed one; unchangedScoped to owner-or-admin (AC2):
control_workflow,start_workflow,workflow_status,present_plan,approve_plan,pending_approval.approve_plan's docstring claimed an authorization it did not perform — "validated bychecking session_id matches the workflow", which the client supplies. So it authorised
whoever asked and deferred the real check to an API gateway that does not perform one. The
docstring now states what the code does.
Executor (AC4):
workflow.owner_id or workflow.session_id→workflow.owner_id. Asession id is not a user id; the fallback made an opaque session string the key secrets were
resolved and redacted under, and every workflow reached it. An unowned workflow now
resolves no user secrets rather than another namespace's, and
manager.pylogs at creationthat it will be admin-only with no user secrets — once per workflow, where the choice is made.
AC3 needed no code.
ws_endpoint.may_controlalready admitted owners; it was waiting onan owner to exist. It gets tests.
New —
approval_support.py(45 lines): the two approval helpers, moved soroutes.pywent 599 → 573 rather than raising a ceiling.
Which reading of #17014 this PR satisfies — say it before anyone quotes "one exception"
#17014 is titled "workflow control cannot be scoped to its owner (REST control is open
to any signed-in user)", and its ACs name creation, the control/state/approval routes, the
WebSocket control check and the executor's secret resolution. This PR takes the control
reading. Under it, AC1 has one exception (below) and ACs 2-4 are delivered.
That matters because there is a second, broader reading — "another user's workflow is
protected" — and under that reading this PR is not sufficient, because
export_workflowauthenticates and never authorises: any signed-in user can export any workflow by id, receiving
every step's literal
command, itsstep_config, and the owner's identity(
api/workflow_export.py:59,services/workflow_serializer.py:271,295-309,150-153). Found byautobot-ai-87reviewing this PR; filed as #17669.It is deliberately not fixed here — different file, different verb (read, not control), and
folding an unreviewed security change into a reviewed PR is the wrong trade. But it cuts one way
worth stating: before #17014,
original_owner_idin that export payload was alwaysNone,because nothing recorded an owner. This PR populates it, so it gives that unguarded endpoint a
real new field to disclose. Still net-positive — an owner recorded and enforced on control beats
no owner and no control anywhere — but #17669 should land close behind, not drift.
So: do not quote "one stated exception" when asked whether #17014 can close. The honest
answer is "ACs 2-4 delivered, AC1 has one stated exception (#17655), and a read surface outside
these ACs is open (#17669)" — and whether that last clause blocks closure is a scope decision for
whoever owns the issue.
Deliberately not fixed here, stated rather than quiet
api/workflow.py:292is a sixth creation path. Recording an owner needs +3 linesminimum (two parameters, one import) and that file is a shrink-only baselined file at
1027. Doing it properly means splitting a 1027-line file — its own PR. I reverted my
change to it entirely rather than leave it half-done. It is fail-closed as it stands:
the route is admin-only, and an unowned workflow is admin-only to control, so there is no
authorisation hole — only an admin-created workflow that resolves no user secrets. AC1
therefore has one stated exception.
/active_workflowsand/completed_workflowsreturn every workflow to any signed-inuser. Same class, different remedy: filtering changes what the UI shows for existing
unowned workflows, which is a product decision rather than a security patch.
routes.pystill convertHTTPExceptionto 500 —create_workflow,get_active_workflows,get_completed_workflows,create_workflow_from_chat. Five alreadyhad the guard; this change added it to the six it gave a 403. Filed as fix(workflows): four routes convert their own HTTPException into a 500, so a deliberate 404 reads as a server error #17653, with the
lineage: fix(api): /agents/recommend swallows HTTPException(400) into a generic 500 #10662 fixed this same defect at one route in another file and closed, and the
class was never swept.
Filed rather than widened into this branch: #17653 (the swallow class), #17654 (the
list routes), #17655 (the
api/workflow.pysplit, which is what lets AC1's last path bedelivered).
AC1 therefore has one undelivered criterion, so this is
Refs #17014, notCloses.Partial delivery closes nothing.
Verification
[pre-push OK] pytest: all relevant tests passonworkflow_ownership_17014_test.py, and the full pre-commit run passed, including thePython file size guard and detect-secrets.
TestClientwithonly
get_current_userand the manager stood in — so the ownership lookup, the 403 and there-raise all execute.
(
control_workflowcross-user, the masked-404 assertion,workflow_statusandstart_workflowcross-user); restoring them returns 25 green. So the tests fail for thereason their titles claim.
test_planning_caller_wiring,test_chat_workflow_is_request_shaped,test_workflow_notification_persistenceandapi/ws_auth_17009_test.py— the callers that still pass two arguments keep working.routes.py599 → 573;executor.pyexactly 1104, its recordedceiling, after restructuring away a first version that reached 1117;
api/workflow.pyreverted to exactly 1027. No ceiling raised, no baseline entry added.
black,isort,flake8clean over all changed files.One defect in this change, self-caught and fixed in
72a42ac.present_planwas one ofthe routes given an ownership check and one of the routes whose
except ExceptionswallowsHTTPException— so its 403 came back as a 500: a working check that looked like a crash. Itsurvived the first pass because
/present_planwas missing from the parametrized cross-usertest, which is the whole lesson: an unlisted route is an unchecked route. It is in the list
now, and an AST assertion over the file confirms no route carrying an ownership check still
swallows its 403.
Local counts are labelled local; CI is the authority. The dependency-floor report in this
checkout states 91 of 139 comparable versions sit below their declared floor, so a local pass
here carries no information about CI's installed set.
Model Used
Claude Opus 5 (1M context)
Single-issue rationale
#17014 is a v0.9.0 release-blocking authorisation hole and the branch freeze was lifted for
it by name, for this issue alone. Nothing else is authorised to ride with it, and the three
adjacent defects this work surfaced were filed rather than folded in (#17653, #17654, #17655).
Refs #17009, #2304, #16628, #17653, #17654, #17655
Refs #17014