fix(server-core): validate workflow ownership on control endpoints#1318
Conversation
🦋 Changeset detectedLatest commit: dbd40e8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR enforces route-based ownership for suspend/cancel: route adapters attach the route workflow id to control request bodies, core handlers validate that the execution belongs to that workflow before acting, and tests plus a changeset document the change. ChangesWorkflow Ownership Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/server-core/src/handlers/workflow.handlers.ts (1)
639-641: ⚡ Quick winFail closed when route workflow id is missing.
typeof workflowId !== "string"currently returnstrue, which bypasses ownership validation. Prefer rejecting when__workflowIdis absent/invalid so control handlers stay safe even if adapter wiring regresses.Suggested hardening diff
async function isWorkflowExecutionOwnedByRoute( body: any, executionId: string, deps: ServerProviderDeps, ) { const workflowId = body?.__workflowId; - return ( - typeof workflowId !== "string" || - ( - await deps.workflowRegistry - .getWorkflow(workflowId) - ?.workflow.memory.getWorkflowState(executionId) - )?.workflowId === workflowId - ); + if (typeof workflowId !== "string" || workflowId.length === 0) { + return false; + } + + return ( + ( + await deps.workflowRegistry + .getWorkflow(workflowId) + ?.workflow.memory.getWorkflowState(executionId) + )?.workflowId === workflowId + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/handlers/workflow.handlers.ts` around lines 639 - 641, The current guard uses "typeof workflowId !== 'string'" which can be true in ways that bypass ownership checks; change the condition around workflowId/__workflowId to fail closed by treating missing or empty values as invalid (e.g., check for typeof workflowId !== 'string' || !workflowId || workflowId.trim() === '') so the function (the conditional that returns based on workflowId) will reject when __workflowId is absent/invalid and therefore will not bypass the ownership validation; update the boolean expression in the return that references workflowId to include the emptiness/falsy checks so ownership validation always runs for missing/invalid IDs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/server-core/src/handlers/workflow.handlers.ts`:
- Around line 639-641: The current guard uses "typeof workflowId !== 'string'"
which can be true in ways that bypass ownership checks; change the condition
around workflowId/__workflowId to fail closed by treating missing or empty
values as invalid (e.g., check for typeof workflowId !== 'string' || !workflowId
|| workflowId.trim() === '') so the function (the conditional that returns based
on workflowId) will reject when __workflowId is absent/invalid and therefore
will not bypass the ownership validation; update the boolean expression in the
return that references workflowId to include the emptiness/falsy checks so
ownership validation always runs for missing/invalid IDs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb33848a-1878-4003-8f17-24d4ad1a4a5c
📒 Files selected for processing (6)
.changeset/workflow-control-ownership.mdpackages/server-core/src/handlers/workflow.handlers.tspackages/server-core/src/handlers/workflow.stream-attach.handlers.spec.tspackages/server-elysia/src/routes/workflow.routes.tspackages/server-hono/src/routes/index.tspackages/serverless-hono/src/routes.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Follow-up commit
Validated locally with Biome, focused |
|
Hey @ruanchaves , |
2 participants
PR Checklist
Bugs / Features
What is the current behavior?
POST /workflows/:id/executions/:executionId/suspendandPOST /workflows/:id/executions/:executionId/cancelignore the workflow ID path parameter and act on the active execution byexecutionIdalone.That allows a wrong-route request to suspend or cancel a live execution that belongs to another workflow.
What is the new behavior?
The shared control handlers now verify that the route workflow owns the execution before acting on it.
The adapters pass the route workflow ID into the shared handler body, and wrong-route suspend/cancel requests are rejected instead of mutating the real execution.
Fixes #1316
Notes for reviewers
server-coreownership validation.workflow.stream-attach.handlers.spec.tsfile.@voltagent/server-coretest suite locally after the final patch, and manually rechecked the reported wrong-route repro behavior earlier in the investigation.packages/server-elysia/src/elysia-server-provider.spec.tsissue during earlier local validation ( [BUG] Elysia server provider test suite entirely broken — stale mocks after Node.js HTTP refactor #1204 ) and intentionally left it out to keep this PR minimal and scoped to [BUG] Workflow suspend/cancel endpoints ignore the workflow ID path parameter #1316.Summary by cubic
Validate workflow ownership on suspend and cancel, and harden request validation so wrong-route or malformed requests can’t control executions. Adapters tag control requests with the route workflow ID, and
@voltagent/server-coreverifies ownership before acting. Fixes #1316.@voltagent/server-core: enforce ownership in suspend/cancel; addedcreateWorkflowControlRequestBodyto validate JSON bodies and attach__workflowId.@voltagent/server-elysia,@voltagent/server-hono,@voltagent/serverless-hono: usecreateWorkflowControlRequestBodyto inject__workflowId;serverless-honoreturns 400 for invalid JSON.Written for commit dbd40e8. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Bug Fixes
Tests