Uh oh!
There was an error while loading. Please reload this page.
fix(mcp): stop refusing tool results authored by another workspace member - #6610
Conversation
…mber `projectWorkflowToolOutput` required the resolved-secret provenance scope to carry the ACTING caller's userId, but the executor stamps that scope with the workflow AUTHOR: on the MCP bridge `execute-service.ts` sets `isClientSession: false` and `workflowUserId: workflow.userId`, so `execution-core.ts` resolves `personalEnvUserId` to the author, while the route builds its scope from `actorUserId`. Author and actor differ in the ordinary team configuration -- both attach and serve authorize on workspace membership only, and the workflow row the route selects does not even include `userId`. The refusal fired only after `executeWorkflowService` had returned, so every affected `tools/call` ran the workflow, wrote its log row, consumed an admission slot and resolved billing attribution, and then answered HTTP 500 / JSON-RPC -32603 'Tool execution failed'. MCP clients retry 500s, re-charging each time. On a public server the actor is pinned to `server.createdBy`, so a server whose creator is not the workflow author was permanently broken for every caller -- including anonymous ones and the creator -- with no recoverable setting. Provenance always carries a scope even for a secret-free workflow, so the failure did not depend on the workflow using secrets at all. The check was collateral of the #5273 rewrite that moved this bridge in-process: main's `projectWorkflowMcpModelContent` had no scope precondition, the sibling Copilot bridge added by the same commit has none, and the registry this route calls documents cross-scope provenance as an ANONYMIZATION signal, never a refusal. Comparing the tenant only is the minimal correct fix. Two alternatives were rejected. Forcing `anonymous: true` on the import is unnecessary and harmful: the registry's own `scopesMatch` already compares both fields, so a user-only difference anonymizes every entry and yields the opaque placeholder, whereas forcing it unconditionally would also strip named redaction from the author's own calls. Comparing against the workflow author instead would mean re-deriving the executor's `isClientSession ? sessionUserId : workflowUserId` rule inside a route -- the same duplication that produced this bug. Follow-up, not fixed here: this bridge imports the whole provenance bundle where main used the value-filtered `importCrossingProvenance`, so a very large env bundle can still hit `MAX_MATCHER_NODES` and produce the same billed 500. Tests pin cross-author success on both the public and the private API-key branches, that a cross-author secret redacts to `[REDACTED_SECRET]` while the author's own call keeps `{{OWNER_TOKEN}}`, and that a workspace mismatch still refuses. Each was verified to fail against the unfixed route or against the rejected alternatives.
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview
Reviewed by Cursor Bugbot for commit 86dcf86. Configure here. |
Greptile SummaryThis PR allows MCP tool results to cross the workflow-author/acting-caller boundary within the same workspace while preserving workspace isolation and cross-user secret-label anonymization.
Confidence Score: 5/5The PR appears safe to merge because same-workspace cross-author results remain subject to scope-aware secret projection and cross-workspace provenance is still rejected. The changed route removes only the user-identity precondition that conflicts with author-stamped provenance; completeness and workspace validation remain intact, and the registry anonymizes entries when the acting user differs from the provenance user.
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/mcp/serve/[serverId]/route.ts | Removes the incorrect actor-versus-author provenance check while preserving tenant validation and registry-based secret projection. |
| apps/sim/app/api/mcp/serve/[serverId]/route.test.ts | Adds focused regression and security-boundary tests for cross-author calls, secret anonymization, and cross-workspace refusal. |
Sequence Diagram
sequenceDiagram
participant Caller as MCP caller
participant Route as MCP serve route
participant Exec as Workflow executor
participant Registry as Secret trace registry
Exec-->>Route: Output + author-scoped provenance
Route->>Route: Validate complete provenance and workspace
Route->>Registry: Import trusted provenance
Registry->>Registry: Mark cross-user entries anonymous
Registry-->>Route: Project secrets to labels or opaque redactions
Route-->>Caller: Successful MCP tool result
Reviews (1): Last reviewed commit: "fix(mcp): stop refusing tool results aut..." | Re-trigger Greptile
Summary
tools/callon an MCP server returned HTTP 500 / JSON-RPC-32603whenever the caller was not the person who authored the workflow behind the tool. The provenance precondition inapps/sim/app/api/mcp/serve/[serverId]/route.tscomparedprovenance.scope?.userId !== scope.userId, but the executor stamps provenance with the workflow AUTHOR (personalEnvUserId, falling back tometadata.workflowUserIdon this non-session path) whilescopehere is the ACTING caller. Owner != actor is the ordinary team configuration, so the check refused normal traffic.server.createdBy, so any server whose creator differs from a tool's workflow author was permanently broken for every caller, including anonymous ones.userIdarm and keeps theworkspaceIdarm, so cross-tenant provenance is still refused. A TSDoc records whyuserIdis deliberately not compared, so the check is not "restored" later as hardening.ResolvedSecretTraceRegistrycompares both scope fields itself and marks every entry imported from another user anonymous, so a non-author sees[REDACTED_SECRET]rather than the author's secret name. That is covered by a test.Type of Change
Testing
bunx vitest run "app/api/mcp/serve/[serverId]/route.test.ts"— 41/41 pass. Reverting onlyroute.tsto staging makes exactly 3 fail (both non-author paths plus the anonymization projection), so the tests provably fail without the fix.bun run type-checkinapps/simis clean; biome is clean.Checklist