fix(mcp): stop refusing tool results authored by another workspace member (#6610)

`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.
2026-09-24 15:45:35 +08:00 · 2026-08-12 01:36:23 -07:00
parent d96f3f95c8
commit d60b6c6273
2 changed files with 244 additions and 2 deletions
@@ -5,6 +5,8 @@
 */
 import {
  dbChainMockFns,
+  encryptionMock,
+  encryptionMockFns,
  hybridAuthMockFns,
  permissionsMock,
  permissionsMockFns,
@@ -31,6 +33,8 @@ const {
  fetchMock: vi.fn(),
 }))

+vi.mock('@/lib/core/security/encryption', () => encryptionMock)
+
 vi.mock('@/lib/billing/core/billing-attribution', () => ({
  BILLING_ATTRIBUTION_HEADER: 'x-sim-billing-attribution',
  assertBillingAttributionSnapshot: mockAssertBillingAttributionSnapshot,
@@ -99,6 +103,9 @@ describe('MCP Serve Route', () => {
    )
    mockAssertBillingAttributionSnapshot.mockImplementation((value: unknown) => value)
    mockSerializeBillingAttributionHeader.mockReturnValue('serialized-attribution')
+    encryptionMockFns.mockDecryptSecret.mockImplementation(async (encryptedValue: string) => ({
+      decrypted: `decrypted:${encryptedValue}`,
+    }))
  })

  afterEach(() => {
@@ -1121,6 +1128,232 @@ describe('MCP Serve Route', () => {
    expect(JSON.parse(body.result.content[0].text)).toEqual(['a', 'b'])
  })

+  it('serves a public tool whose workflow was authored by someone other than the actor', async () => {
+    dbChainMockFns.limit
+      .mockResolvedValueOnce([
+        {
+          id: 'server-1',
+          name: 'Public Server',
+          workspaceId: 'ws-1',
+          isPublic: true,
+          createdBy: 'owner-1',
+        },
+      ])
+      .mockResolvedValueOnce([{ toolName: 'tool_a', workflowId: 'wf-1' }])
+      .mockResolvedValueOnce([{ workspaceId: 'ws-1', deploymentVersionId: 'deployment-1' }])
+    mockExecuteWorkflowService.mockResolvedValueOnce({
+      ok: true,
+      executionId: 'exec-1',
+      workflowId: 'wf-1',
+      status: 'completed',
+      aborted: null,
+      output: { ok: true },
+      error: null,
+      hasResponseBlock: false,
+      resolvedSecretTraceProvenance: createResolvedSecretTraceProvenance('author-2'),
+    })
+
+    const req = new NextRequest('http://localhost:3000/api/mcp/serve/server-1', {
+      method: 'POST',
+      body: JSON.stringify({
+        jsonrpc: '2.0',
+        id: 1,
+        method: 'tools/call',
+        params: { name: 'tool_a', arguments: { q: 'test' } },
+      }),
+    })
+    const response = await POST(req, { params: Promise.resolve({ serverId: 'server-1' }) })
+    const body = await response.json()
+
+    expect(response.status).toBe(200)
+    expect(JSON.parse(body.result.content[0].text)).toEqual({ ok: true })
+  })
+
+  it('serves a private tool whose workflow was authored by someone other than the caller', async () => {
+    dbChainMockFns.limit
+      .mockResolvedValueOnce([
+        {
+          id: 'server-1',
+          name: 'Private Server',
+          workspaceId: 'ws-1',
+          isPublic: false,
+          createdBy: 'owner-1',
+        },
+      ])
+      .mockResolvedValueOnce([{ toolName: 'tool_a', workflowId: 'wf-1' }])
+      .mockResolvedValueOnce([{ workspaceId: 'ws-1', deploymentVersionId: 'deployment-1' }])
+    /**
+     * `vi.clearAllMocks()` does not drain `mockResolvedValueOnce` queues, and a public-server
+     * test never reaches `checkHybridAuth`, so an earlier queued auth would be consumed here.
+     */
+    hybridAuthMockFns.mockCheckHybridAuth.mockReset()
+    hybridAuthMockFns.mockCheckHybridAuth.mockResolvedValueOnce({
+      success: true,
+      userId: 'user-1',
+      authType: 'api_key',
+      apiKeyType: 'workspace',
+      workspaceId: 'ws-1',
+    })
+    mockGetUserEntityPermissions.mockResolvedValueOnce('write')
+    mockExecuteWorkflowService.mockResolvedValueOnce({
+      ok: true,
+      executionId: 'exec-1',
+      workflowId: 'wf-1',
+      status: 'completed',
+      aborted: null,
+      output: { ok: true },
+      error: null,
+      hasResponseBlock: false,
+      resolvedSecretTraceProvenance: createResolvedSecretTraceProvenance('author-2'),
+    })
+
+    const req = new NextRequest('http://localhost:3000/api/mcp/serve/server-1', {
+      method: 'POST',
+      headers: { 'X-API-Key': 'wsk_test_123' },
+      body: JSON.stringify({
+        jsonrpc: '2.0',
+        id: 1,
+        method: 'tools/call',
+        params: { name: 'tool_a', arguments: { q: 'test' } },
+      }),
+    })
+    const response = await POST(req, { params: Promise.resolve({ serverId: 'server-1' }) })
+    const body = await response.json()
+
+    expect(response.status).toBe(200)
+    expect(JSON.parse(body.result.content[0].text)).toEqual({ ok: true })
+  })
+
+  it('anonymizes an author-scoped secret label when the actor is not the author', async () => {
+    dbChainMockFns.limit
+      .mockResolvedValueOnce([
+        {
+          id: 'server-1',
+          name: 'Public Server',
+          workspaceId: 'ws-1',
+          isPublic: true,
+          createdBy: 'owner-1',
+        },
+      ])
+      .mockResolvedValueOnce([{ toolName: 'tool_a', workflowId: 'wf-1' }])
+      .mockResolvedValueOnce([{ workspaceId: 'ws-1', deploymentVersionId: 'deployment-1' }])
+    mockExecuteWorkflowService.mockResolvedValueOnce({
+      ok: true,
+      executionId: 'exec-1',
+      workflowId: 'wf-1',
+      status: 'completed',
+      aborted: null,
+      output: { leaked: 'decrypted:author-ciphertext' },
+      error: null,
+      hasResponseBlock: false,
+      resolvedSecretTraceProvenance: {
+        ...createResolvedSecretTraceProvenance('author-2'),
+        entries: [{ name: 'AUTHOR_TOKEN', encryptedValue: 'author-ciphertext' }],
+      },
+    })
+
+    const req = new NextRequest('http://localhost:3000/api/mcp/serve/server-1', {
+      method: 'POST',
+      body: JSON.stringify({
+        jsonrpc: '2.0',
+        id: 1,
+        method: 'tools/call',
+        params: { name: 'tool_a', arguments: { q: 'test' } },
+      }),
+    })
+    const response = await POST(req, { params: Promise.resolve({ serverId: 'server-1' }) })
+    const body = await response.json()
+
+    expect(response.status).toBe(200)
+    expect(JSON.parse(body.result.content[0].text)).toEqual({ leaked: '[REDACTED_SECRET]' })
+    expect(body.result.content[0].text).not.toContain('AUTHOR_TOKEN')
+  })
+
+  it('keeps the author-scoped secret label when the actor is the author', async () => {
+    dbChainMockFns.limit
+      .mockResolvedValueOnce([
+        {
+          id: 'server-1',
+          name: 'Public Server',
+          workspaceId: 'ws-1',
+          isPublic: true,
+          createdBy: 'owner-1',
+        },
+      ])
+      .mockResolvedValueOnce([{ toolName: 'tool_a', workflowId: 'wf-1' }])
+      .mockResolvedValueOnce([{ workspaceId: 'ws-1', deploymentVersionId: 'deployment-1' }])
+    mockExecuteWorkflowService.mockResolvedValueOnce({
+      ok: true,
+      executionId: 'exec-1',
+      workflowId: 'wf-1',
+      status: 'completed',
+      aborted: null,
+      output: { leaked: 'decrypted:owner-ciphertext' },
+      error: null,
+      hasResponseBlock: false,
+      resolvedSecretTraceProvenance: {
+        ...createResolvedSecretTraceProvenance('owner-1'),
+        entries: [{ name: 'OWNER_TOKEN', encryptedValue: 'owner-ciphertext' }],
+      },
+    })
+
+    const req = new NextRequest('http://localhost:3000/api/mcp/serve/server-1', {
+      method: 'POST',
+      body: JSON.stringify({
+        jsonrpc: '2.0',
+        id: 1,
+        method: 'tools/call',
+        params: { name: 'tool_a', arguments: { q: 'test' } },
+      }),
+    })
+    const response = await POST(req, { params: Promise.resolve({ serverId: 'server-1' }) })
+    const body = await response.json()
+
+    expect(response.status).toBe(200)
+    expect(JSON.parse(body.result.content[0].text)).toEqual({ leaked: '{{OWNER_TOKEN}}' })
+  })
+
+  it('refuses a tool result whose provenance was stamped for another workspace', async () => {
+    dbChainMockFns.limit
+      .mockResolvedValueOnce([
+        {
+          id: 'server-1',
+          name: 'Public Server',
+          workspaceId: 'ws-1',
+          isPublic: true,
+          createdBy: 'owner-1',
+        },
+      ])
+      .mockResolvedValueOnce([{ toolName: 'tool_a', workflowId: 'wf-1' }])
+      .mockResolvedValueOnce([{ workspaceId: 'ws-1', deploymentVersionId: 'deployment-1' }])
+    mockExecuteWorkflowService.mockResolvedValueOnce({
+      ok: true,
+      executionId: 'exec-1',
+      workflowId: 'wf-1',
+      status: 'completed',
+      aborted: null,
+      output: { ok: true },
+      error: null,
+      hasResponseBlock: false,
+      resolvedSecretTraceProvenance: createResolvedSecretTraceProvenance('owner-1', 'ws-other'),
+    })
+
+    const req = new NextRequest('http://localhost:3000/api/mcp/serve/server-1', {
+      method: 'POST',
+      body: JSON.stringify({
+        jsonrpc: '2.0',
+        id: 1,
+        method: 'tools/call',
+        params: { name: 'tool_a', arguments: { q: 'test' } },
+      }),
+    })
+    const response = await POST(req, { params: Promise.resolve({ serverId: 'server-1' }) })
+    const body = await response.json()
+
+    expect(response.status).toBe(500)
+    expect(body.error.code).toBe(-32603)
+  })
+
  it('rejects duplicate tool names instead of choosing an arbitrary workflow', async () => {
    dbChainMockFns.limit
      .mockResolvedValueOnce([
@@ -205,11 +205,20 @@ async function projectWorkflowToolOutput(
  provenance: unknown,
  scope: { userId: string; workspaceId: string }
 ): Promise<unknown> {
+  /**
+   * Deliberately compares the tenant only, never the user. The executor stamps provenance with
+   * the workflow AUTHOR (`personalEnvUserId` falls back to `metadata.workflowUserId` on this
+   * non-session path) while `scope` here is the ACTING caller, and a caller who did not author
+   * the workflow is the ordinary team configuration — demanding they match refuses every such
+   * call after the workflow has already run and been billed. Restoring a user comparison here
+   * is not hardening: the registry compares both scope fields itself and marks every entry
+   * imported from another user anonymous, so a non-author already sees the opaque redaction
+   * placeholder rather than the author's secret names.
+   */
  if (
    !isResolvedSecretTraceProvenanceV1(provenance) ||
    !provenance.complete ||
-    provenance.scope?.userId !== scope.userId ||
-    provenance.scope.workspaceId !== scope.workspaceId
+    provenance.scope?.workspaceId !== scope.workspaceId
  ) {
    throw new Error('MCP workflow execution provenance is unavailable')
  }