fix(security): enforce workspace scoping and role checks on job/connection/webhook routes - #575
Merged
Merged
Conversation
…ction/webhook routes
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
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.
SECURITY
Fixes three linked cross-tenant authorization gaps confirmed by an internal security review. No working exploit payloads are included.
A) CRITICAL — cross-tenant Job (workflow) hijack
The id-addressed handlers in
routes/workflows.tsresolved the workflow/run without any workspace check, so any authenticated tenant could edit, clone, run, or delete another tenant's Job and read its runs/logs by guessing/enumerating a UUID:PATCH /api/jobs/:id,POST /api/jobs/:id/clone,DELETE /api/jobs/:id,POST /api/jobs/:id/runsGET /api/workflow-runs/:id,POST /api/workflow-runs/:id/retry,POST /api/workflow-runs/:id/cancel,GET /api/workflow-runs/:id/logsFix: added two resolver helpers mirroring the
routes/tasks.tspattern —requireWorkflowInWorkspace()andrequireWorkflowRunInWorkspace()(runs carry no workspace column, so ownership is derived from the parent workflow). Every id handler now resolves through them and returns 404 (not 403) for missing or foreign resources, before any mutation runs.B) MEDIUM — missing RBAC on mutating routes
Mutating routes across
workflows.ts,tasks-unified.ts, andwebhooks.tshad norequireRole, so aviewercould create/run/delete. Matched thetasks.tsconvention by addingpreHandler: [requireRole("member")]to:POST /api/jobs,PATCH/DELETE /api/jobs/:id,POST /api/jobs/:id/clone,POST /api/jobs/:id/runs,POST /api/workflow-runs/:id/retry|cancelPOST /api/tasks/:id/runs,POST/PATCH/DELETE /api/tasks/:id/triggersPOST /api/webhooks,PATCH/DELETE /api/webhooks/:id,POST /api/webhooks/:id/test(The unified task/trigger routes already scope the id via
resolveAnyTaskById(id, workspaceId); that stays intact — this only adds the role gate. A test locks in that the resolver is still called with the caller's workspace.)C) MEDIUM — connection-assignment IDOR
The connection-assignment routes in
routes/connections.tscalledconnectionService.getConnection(id)(or nothing at all, for the flat routes) with no workspace check, allowing cross-tenant credential-binding tamper:GET/POST /api/connections/:id/assignmentsPATCH/DELETE /api/connection-assignments/:id(these took only the assignment id — a pure IDOR)Fix: added a
requireOwnedConnection(req, reply, connectionId)guard (404 on missing/foreign, mirroring the sibling connection routes) and a read-onlyconnectionService.getAssignment(id)so the flat routes can resolve the owning connection before acting. Assignment mutations also now requiremember.Notes
requireRoleand the workspace checks already short-circuit viaisAuthDisabled()/ null workspaceId.workspaceId) remain accessible, consistent with the existingtasks.ts/webhooks.tshandlers.workflow-service.tsbusiness logic (other open PRs touch it); only read-only getters are consumed.persistent-agents.ts/persistent-agent-service.tsuntouched.Tests
Extended the existing route test files (
workflows.test.ts,connections.test.ts,webhooks.test.ts,tasks-unified.test.ts): assert 404 for cross-workspace id access on the (A)/(C) routes and 403 for avieweron the (B) mutations, verifying no service mutation is reached in either case.apps/apiunit suite: 2202 passedapps/apitypecheck (tsc --noEmit): cleanpnpm turbo typecheck: 12/12 passpnpm format:check: cleanFound via internal security review.