fix(authz): enforce credential and workspace boundaries (#6585)

* fix(authz): enforce credential and workspace boundaries

* fix(api): preserve fork policy in workflow detail

* fix(knowledge): preserve credential access guidance
This commit is contained in:
Vikhyath Mondreti
2026-08-11 19:35:58 -07:00
committed by GitHub
parent 1874ceccda
commit 261435cabe
14 changed files with 269 additions and 37 deletions
@@ -5,9 +5,10 @@ import { dbChainMockFns, resetDbChainMock } from '@sim/testing'
import type { NextRequest } from 'next/server'
import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
const { mockClearCache, mockDiscoverServerTools } = vi.hoisted(() => ({
const { mockClearCache, mockDiscoverServerTools, requiredRoles } = vi.hoisted(() => ({
mockClearCache: vi.fn(),
mockDiscoverServerTools: vi.fn(),
requiredRoles: [] as string[],
}))
vi.mock('@/lib/core/utils/with-route-handler', () => ({
@@ -15,21 +16,22 @@ vi.mock('@/lib/core/utils/with-route-handler', () => ({
}))
vi.mock('@/lib/mcp/middleware', () => ({
withMcpAuth:
() =>
(
withMcpAuth: (requiredRole: string) => {
requiredRoles.push(requiredRole)
return (
handler: (
request: NextRequest,
context: { userId: string; workspaceId: string; requestId: string },
routeContext: { params: Promise<{ id: string }> }
) => Promise<Response>
) =>
(request: NextRequest, routeContext: { params: Promise<{ id: string }> }) =>
handler(
request,
{ userId: 'user-1', workspaceId: 'workspace-1', requestId: 'request-1' },
routeContext
),
(request: NextRequest, routeContext: { params: Promise<{ id: string }> }) =>
handler(
request,
{ userId: 'user-1', workspaceId: 'workspace-1', requestId: 'request-1' },
routeContext
)
},
}))
vi.mock('@/lib/mcp/service', () => ({
@@ -72,6 +74,10 @@ describe('MCP server refresh route', () => {
resetDbChainMock()
})
it('requires workspace write permission because refresh persists workflow changes', () => {
expect(requiredRoles).toEqual(['write'])
})
it('preserves the service-persisted OAuth pending status', async () => {
mockDiscoverServerTools.mockRejectedValueOnce(new Error('OAuth authorization required'))
@@ -158,7 +158,7 @@ async function syncToolSchemasToWorkflows(
}
export const POST = withRouteHandler(
withMcpAuth<{ id: string }>('read')(
withMcpAuth<{ id: string }>('write')(
async (request: NextRequest, { userId, workspaceId, requestId }, { params }) => {
try {
const paramsValidation = mcpServerIdParamsSchema.safeParse(await params)
@@ -91,7 +91,7 @@ vi.mock('@/lib/workspaces/policy', () => ({
import { createFork } from '@/ee/workspace-forking/lib/create-fork'
const SOURCE = { id: 'src-ws', name: 'Parent' } as never
const SOURCE = { id: 'src-ws', name: 'Parent', allowPersonalApiKeys: false } as never
const POLICY = {
organizationId: null,
workspaceMode: 'personal',
@@ -210,6 +210,15 @@ describe('createFork storage headroom gate', () => {
)
})
it('preserves the source workspace personal API-key policy in the child', async () => {
const result = await createFork(forkParams())
expect(result.workspace.allowPersonalApiKeys).toBe(false)
expect(dbChainMockFns.values).toHaveBeenCalledWith(
expect.objectContaining({ allowPersonalApiKeys: false })
)
})
it('seeds identity mappings for copied FILES by storage key (a later sync must not re-offer them)', async () => {
mockPlanForkFileCopies.mockResolvedValue({
keyMap: new Map([['workspace/src-ws/a.png', 'workspace/child/a.png']]),
@@ -170,7 +170,7 @@ export async function createFork(params: CreateForkParams): Promise<CreateForkRe
organizationId: policy.organizationId,
workspaceMode: policy.workspaceMode,
billedAccountUserId: policy.billedAccountUserId,
allowPersonalApiKeys: true,
allowPersonalApiKeys: source.allowPersonalApiKeys,
forkedFromWorkspaceId: source.id,
createdAt: now,
updatedAt: now,
@@ -418,7 +418,7 @@ export async function createFork(params: CreateForkParams): Promise<CreateForkRe
organizationId: policy.organizationId,
workspaceMode: policy.workspaceMode,
billedAccountUserId: policy.billedAccountUserId,
allowPersonalApiKeys: true,
allowPersonalApiKeys: source.allowPersonalApiKeys,
forkedFromWorkspaceId: source.id,
},
workflowsCopied,
@@ -1,6 +1,7 @@
import { describe, expect, it } from 'vitest'
import {
executeWorkflowBodySchema,
getWorkflowResponseDataSchema,
updateWorkflowBodySchema,
workflowListItemSchema,
} from '@/lib/api/contracts/workflows'
@@ -119,4 +120,11 @@ describe('workflow contracts', () => {
})
expect(item.forkSyncExcluded).toBe(false)
})
it('detail response preserves forkSyncExcluded and defaults old responses', () => {
const forkPolicySchema = getWorkflowResponseDataSchema.pick({ forkSyncExcluded: true })
expect(forkPolicySchema.parse({ forkSyncExcluded: true }).forkSyncExcluded).toBe(true)
expect(forkPolicySchema.parse({}).forkSyncExcluded).toBe(false)
})
})
+1
View File
@@ -743,6 +743,7 @@ export const getWorkflowResponseDataSchema = z.object({
deployedAt: z.coerce.date().nullable(),
isPublicApi: z.boolean(),
locked: z.boolean(),
forkSyncExcluded: z.boolean().default(false),
runCount: z.number(),
lastRunAt: z.coerce.date().nullable(),
archivedAt: z.coerce.date().nullable(),
@@ -631,6 +631,33 @@ describe('knowledge_base trusted application delegation', () => {
})
})
it('preserves credential access guidance for connector creation', async () => {
mockCreateKnowledgeConnector.mockRejectedValueOnce(
new OrchestrationError(
'validation',
'Credential is not available to you in this workspace. Ask a credential administrator to grant access or select another credential.'
)
)
const result = await knowledgeBaseServerTool.execute(
{
operation: 'add_connector',
args: {
knowledgeBaseId: KNOWLEDGE_BASE.id,
connectorType: 'notion',
credentialId: 'credential-1',
},
},
CONTEXT
)
expect(result).toEqual({
success: false,
message:
'Credential is not available to you in this workspace. Ask a credential administrator to grant access or select another credential.',
})
})
it('preserves caller-actionable tag provenance conflicts', async () => {
mockDeleteKnowledgeTag.mockRejectedValueOnce(
new OrchestrationError(
@@ -15,6 +15,8 @@ const mocks = vi.hoisted(() => ({
deleteConnector: vi.fn(),
syncConnector: vi.fn(),
resolveBilling: vi.fn(),
getCredentialActorContext: vi.fn(),
canUseCredential: vi.fn(),
resolveTokenIdentity: vi.fn(),
refreshToken: vi.fn(),
validateConnectorConfig: vi.fn(),
@@ -56,6 +58,8 @@ vi.mock('@/lib/knowledge/orchestration/connectors', () => ({
}))
vi.mock('@/lib/credentials/access', () => ({
getCredentialActorContext: mocks.getCredentialActorContext,
canUseCredential: mocks.canUseCredential,
resolveCredentialTokenIdentity: mocks.resolveTokenIdentity,
}))
@@ -120,6 +124,17 @@ describe('knowledge connector application use cases', () => {
mocks.resolvePermission.mockResolvedValue('write')
mocks.resolveKnowledgeBase.mockResolvedValue(crossWorkspaceContext)
mocks.resolveConnector.mockResolvedValue(connectorContext)
mocks.getCredentialActorContext.mockResolvedValue({
credential: { id: 'credential-1', workspaceId: 'workspace-a' },
member: { role: 'member' },
hasWorkspaceAccess: true,
canWriteWorkspace: true,
isAdmin: false,
})
mocks.canUseCredential.mockImplementation(
(access: { hasWorkspaceAccess: boolean; member: unknown; isAdmin: boolean }) =>
access.hasWorkspaceAccess && (Boolean(access.member) || access.isAdmin)
)
mocks.resolveTokenIdentity.mockResolvedValue({ kind: 'oauth', userId: 'credential-owner' })
mocks.refreshToken.mockResolvedValue('access-token')
mocks.validateConnectorConfig.mockResolvedValue({ valid: true })
@@ -296,6 +311,7 @@ describe('knowledge connector application use cases', () => {
expect(mocks.resolvePermission.mock.invocationCallOrder[0]).toBeLessThan(
mocks.updateConnector.mock.invocationCallOrder[0]
)
expect(mocks.getCredentialActorContext).toHaveBeenCalledWith('credential-1', 'shared-user')
expect(mocks.resolveTokenIdentity).toHaveBeenCalledWith('credential-1', 'workspace-a')
expect(mocks.refreshToken).toHaveBeenCalledWith(
'credential-1',
@@ -305,6 +321,122 @@ describe('knowledge connector application use cases', () => {
expect(mocks.validateConnectorConfig).toHaveBeenCalledWith('access-token', { space: 'ENG' })
})
it('rejects connector creation when the writer cannot use the workspace credential', async () => {
const sameWorkspaceContext = {
...connectorContext,
workspaceId: 'workspace-a',
knowledgeBaseId: 'knowledge-a',
knowledgeBase: { id: 'knowledge-a', name: 'Workspace A docs' },
connector: { ...connectorContext.connector, knowledgeBaseId: 'knowledge-a' },
}
mocks.resolveKnowledgeBase.mockResolvedValueOnce(sameWorkspaceContext)
mocks.getCredentialActorContext.mockResolvedValueOnce({
credential: { id: 'credential-1', workspaceId: 'workspace-a' },
member: null,
hasWorkspaceAccess: true,
canWriteWorkspace: true,
isAdmin: false,
})
mocks.createConnector.mockImplementationOnce(
async (input: { resolveAccessToken: (credentialId: string) => Promise<string | null> }) => {
const accessToken = await input.resolveAccessToken('credential-1')
return accessToken
? { success: true, connector: sameWorkspaceContext.connector }
: {
success: false,
error: 'Credential has no access token. Please reconnect your account.',
errorCode: 'validation',
}
}
)
await expect(
createKnowledgeConnector.execute({
principal: delegatedPrincipal,
input: {
knowledgeBaseId: 'knowledge-a',
assertedWorkspaceId: 'workspace-a',
connectorType: 'confluence',
credentialId: 'credential-1',
sourceConfig: {},
syncIntervalMinutes: 1440,
resolveBillingAttribution: mocks.resolveBilling,
},
})
).rejects.toMatchObject({
code: 'validation',
message:
'Credential is not available to you in this workspace. Ask a credential administrator to grant access or select another credential.',
})
expect(mocks.getCredentialActorContext).toHaveBeenCalledWith('credential-1', 'shared-user')
expect(mocks.resolveTokenIdentity).not.toHaveBeenCalled()
expect(mocks.refreshToken).not.toHaveBeenCalled()
})
it('rejects source-config revalidation after credential membership is removed', async () => {
const sameWorkspaceContext = {
...connectorContext,
workspaceId: 'workspace-a',
knowledgeBaseId: 'knowledge-a',
knowledgeBase: { id: 'knowledge-a', name: 'Workspace A docs' },
connector: { ...connectorContext.connector, knowledgeBaseId: 'knowledge-a' },
}
mocks.resolveConnector.mockResolvedValueOnce(sameWorkspaceContext)
mocks.updateConnector.mockResolvedValueOnce({
success: true,
connector: { ...sameWorkspaceContext.connector, sourceConfig: { space: 'ENG' } },
})
await updateKnowledgeConnector.execute({
principal: delegatedPrincipal,
input: {
connectorId: 'connector-b',
assertedWorkspaceId: 'workspace-a',
updates: { sourceConfig: { space: 'ENG' } },
},
})
const orchestrationInput = mocks.updateConnector.mock.calls[0]?.[0] as {
validateSourceConfig?: (
connector: {
connectorType: string
credentialId: string
encryptedApiKey: null
},
sourceConfig: Record<string, unknown>
) => Promise<unknown>
}
if (!orchestrationInput.validateSourceConfig) {
throw new Error('Application command did not provide source-config validation')
}
mocks.getCredentialActorContext.mockResolvedValueOnce({
credential: { id: 'credential-1', workspaceId: 'workspace-a' },
member: null,
hasWorkspaceAccess: true,
canWriteWorkspace: true,
isAdmin: false,
})
await expect(
orchestrationInput.validateSourceConfig(
{
connectorType: 'confluence',
credentialId: 'credential-1',
encryptedApiKey: null,
},
{ space: 'ENG' }
)
).rejects.toMatchObject({
code: 'validation',
message:
'Credential is not available to you in this workspace. Ask a credential administrator to grant access or select another credential.',
})
expect(mocks.resolveTokenIdentity).not.toHaveBeenCalled()
expect(mocks.refreshToken).not.toHaveBeenCalled()
expect(mocks.validateConnectorConfig).not.toHaveBeenCalled()
})
it.each([
[
'create',
@@ -6,7 +6,11 @@ import { decryptApiKey } from '@/lib/api-key/crypto'
import type { BillingAttributionSnapshot } from '@/lib/billing/core/billing-attribution'
import { OrchestrationError } from '@/lib/core/orchestration/types'
import { generateRequestId } from '@/lib/core/utils/request'
import { resolveCredentialTokenIdentity } from '@/lib/credentials/access'
import {
canUseCredential,
getCredentialActorContext,
resolveCredentialTokenIdentity,
} from '@/lib/credentials/access'
import { defineAuthorizedKnowledgeUseCase } from '@/lib/knowledge/application/authorized-knowledge-use-case'
import { resolveKnowledgeAttributedUserId } from '@/lib/knowledge/application/billing'
import {
@@ -116,13 +120,32 @@ function requireConnectorWorkspaceId(context: ActiveKnowledgeResourceBaseContext
return context.workspaceId
}
async function resolveAuthorizedConnectorCredentialIdentity(input: {
credentialId: string
workspaceId: string
actingUserId: string
}) {
const access = await getCredentialActorContext(input.credentialId, input.actingUserId)
if (
!access.credential ||
access.credential.workspaceId !== input.workspaceId ||
!canUseCredential(access)
) {
throw new OrchestrationError(
'validation',
'Credential is not available to you in this workspace. Ask a credential administrator to grant access or select another credential.'
)
}
return resolveCredentialTokenIdentity(input.credentialId, input.workspaceId)
}
async function resolveConnectorCredentialAccessToken(input: {
credentialId: string
workspaceId: string
actingUserId: string
requestId: string
}): Promise<string | null> {
const identity = await resolveCredentialTokenIdentity(input.credentialId, input.workspaceId)
const identity = await resolveAuthorizedConnectorCredentialIdentity(input)
if (!identity) return null
return refreshAccessTokenIfNeeded(
input.credentialId,
@@ -163,10 +186,11 @@ async function validateConnectorSourceConfig(input: {
errorCode: 'validation',
}
}
const identity = await resolveCredentialTokenIdentity(
input.connector.credentialId,
input.workspaceId
)
const identity = await resolveAuthorizedConnectorCredentialIdentity({
credentialId: input.connector.credentialId,
workspaceId: input.workspaceId,
actingUserId: input.actingUserId,
})
if (!identity) {
return {
message: 'Credential is no longer usable in this workspace. Please reconnect it.',
+23 -6
View File
@@ -8,7 +8,7 @@
* raw `fetch`.
*/
import { queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing'
import { dbChainMockFns, queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing'
import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
const BLOCKED_ENDPOINT = 'http://169.254.170.2/v2/credentials/'
@@ -104,7 +104,7 @@ describe('revokeMcpOauthTokens — SSRF guard', () => {
})
it('routes metadata discovery through the SSRF-guarded fetch', async () => {
await revokeMcpOauthTokens('server-1')
await revokeMcpOauthTokens('server-1', 'workspace-1')
expect(mockDiscoverOAuthServerInfo).toHaveBeenCalledTimes(1)
const [, options] = mockDiscoverOAuthServerInfo.mock.calls[0]
@@ -112,13 +112,13 @@ describe('revokeMcpOauthTokens — SSRF guard', () => {
})
it('validates the attacker-controlled revocation_endpoint before issuing the request', async () => {
await revokeMcpOauthTokens('server-1')
await revokeMcpOauthTokens('server-1', 'workspace-1')
expect(mockValidateMcpServerSsrf).toHaveBeenCalledWith(BLOCKED_ENDPOINT)
})
it('never issues an outbound request to the blocked revocation endpoint', async () => {
await revokeMcpOauthTokens('server-1')
await revokeMcpOauthTokens('server-1', 'workspace-1')
const allCalls = [
...mockUndiciFetch.mock.calls,
@@ -131,7 +131,7 @@ describe('revokeMcpOauthTokens — SSRF guard', () => {
})
it('swallows the SSRF rejection — revocation is best-effort and never throws', async () => {
await expect(revokeMcpOauthTokens('server-1')).resolves.toBeUndefined()
await expect(revokeMcpOauthTokens('server-1', 'workspace-1')).resolves.toBeUndefined()
})
it('still issues the revocation POST when the endpoint resolves to a public IP', async () => {
@@ -143,7 +143,7 @@ describe('revokeMcpOauthTokens — SSRF guard', () => {
},
})
await revokeMcpOauthTokens('server-1')
await revokeMcpOauthTokens('server-1', 'workspace-1')
expect(mockValidateMcpServerSsrf).toHaveBeenCalledWith(publicEndpoint)
const revokeCalls = mockUndiciFetch.mock.calls.filter((call) => {
@@ -153,4 +153,21 @@ describe('revokeMcpOauthTokens — SSRF guard', () => {
expect(revokeCalls.length).toBeGreaterThan(0)
expect(revokeCalls[0][1]).toMatchObject({ method: 'POST' })
})
it('loads no OAuth tokens when the server is outside the authorized workspace', async () => {
resetDbChainMock()
queueTableRows(schemaMock.mcpServers, [])
await revokeMcpOauthTokens('server-1', 'workspace-other')
expect(mockLoadOauthRow).not.toHaveBeenCalled()
expect(mockDiscoverOAuthServerInfo).not.toHaveBeenCalled()
expect(dbChainMockFns.where).toHaveBeenCalledWith(
expect.objectContaining({
conditions: expect.arrayContaining([
expect.objectContaining({ type: 'eq', right: 'workspace-other' }),
]),
})
)
})
})
+11 -6
View File
@@ -4,7 +4,7 @@ import { db } from '@sim/db'
import { mcpServers } from '@sim/db/schema'
import { createLogger } from '@sim/logger'
import { toError } from '@sim/utils/errors'
import { eq } from 'drizzle-orm'
import { and, eq } from 'drizzle-orm'
import { decryptSecret } from '@/lib/core/security/encryption'
import { loadOauthRow } from '@/lib/mcp/oauth/storage'
import { createSsrfGuardedMcpFetch } from '@/lib/mcp/pinned-fetch'
@@ -15,12 +15,14 @@ const REVOKE_TIMEOUT_MS = 5000
/**
* Best-effort RFC 7009 revocation of tokens at the authorization server.
* Never throws — revocation is advisory and must not block disconnect/delete flows.
* The workspace scope is mandatory so a caller cannot revoke a server it has not
* already resolved inside its authorized workspace.
*/
export async function revokeMcpOauthTokens(mcpServerId: string): Promise<void> {
export async function revokeMcpOauthTokens(
mcpServerId: string,
workspaceId: string
): Promise<void> {
try {
const row = await loadOauthRow({ mcpServerId })
if (!row?.tokens) return
const [server] = await db
.select({
url: mcpServers.url,
@@ -28,10 +30,13 @@ export async function revokeMcpOauthTokens(mcpServerId: string): Promise<void> {
oauthClientSecret: mcpServers.oauthClientSecret,
})
.from(mcpServers)
.where(eq(mcpServers.id, mcpServerId))
.where(and(eq(mcpServers.id, mcpServerId), eq(mcpServers.workspaceId, workspaceId)))
.limit(1)
if (!server?.url) return
const row = await loadOauthRow({ mcpServerId })
if (!row?.tokens) return
const ssrfGuardedFetch = createSsrfGuardedMcpFetch()
const info = await discoverOAuthServerInfo(server.url, { fetchFn: ssrfGuardedFetch }).catch(
() => undefined
@@ -150,7 +150,7 @@ describe('MCP server lifecycle orchestration', () => {
})
)
// ...and revoke the now-orphaned OAuth tokens rather than leaving them stored and valid.
expect(mockRevokeOauthTokens).toHaveBeenCalledWith('server-1')
expect(mockRevokeOauthTokens).toHaveBeenCalledWith('server-1', 'workspace-1')
})
it('resets to disconnected when a create/upsert flips an existing OAuth server to headers', async () => {
@@ -196,7 +196,7 @@ describe('MCP server lifecycle orchestration', () => {
})
)
// ...and revoke the now-orphaned OAuth tokens.
expect(mockRevokeOauthTokens).toHaveBeenCalledWith('server-1')
expect(mockRevokeOauthTokens).toHaveBeenCalledWith('server-1', 'workspace-1')
})
it('evicts the deleted server from the connection pool (row is already gone from clearCache)', async () => {
@@ -211,6 +211,7 @@ describe('MCP server lifecycle orchestration', () => {
})
expect(result.success).toBe(true)
expect(mockRevokeOauthTokens).toHaveBeenCalledWith('server-1', 'workspace-1')
expect(mockEvictServerConnections).toHaveBeenCalledWith('server-1', expect.any(String))
})
})
@@ -195,7 +195,7 @@ export async function createMcpServer(
const oauthDisabled = existingServer.authType === 'oauth' && resolvedAuthType !== 'oauth'
const shouldClearOauth = urlChanged || credsChanged || isRevival || oauthDisabled
if (shouldClearOauth) await revokeMcpOauthTokens(serverId)
if (shouldClearOauth) await revokeMcpOauthTokens(serverId, params.workspaceId)
await db.transaction(async (tx) => {
if (shouldClearOauth) {
@@ -354,7 +354,7 @@ export async function updateMcpServer(
updateData.lastError = null
}
if (shouldClearOauth) await revokeMcpOauthTokens(params.serverId)
if (shouldClearOauth) await revokeMcpOauthTokens(params.serverId, params.workspaceId)
const server = await db.transaction(async (tx) => {
const [updated] = await tx
@@ -400,7 +400,7 @@ export async function deleteMcpServer(
params: Omit<PerformDeleteMcpServerParams, keyof ActorMetadata | 'source'>
): Promise<PerformMcpServerResult> {
try {
await revokeMcpOauthTokens(params.serverId)
await revokeMcpOauthTokens(params.serverId, params.workspaceId)
const [server] = await db
.delete(mcpServers)
.where(
@@ -25,6 +25,7 @@ export interface WorkspaceWithOwner {
organizationId: string | null
workspaceMode: WorkspaceMode
billedAccountUserId: string
allowPersonalApiKeys: boolean
archivedAt?: Date | null
}
@@ -95,6 +96,7 @@ export async function getWorkspaceWithOwner(
organizationId: workspace.organizationId,
workspaceMode: workspace.workspaceMode,
billedAccountUserId: workspace.billedAccountUserId,
allowPersonalApiKeys: workspace.allowPersonalApiKeys,
archivedAt: workspace.archivedAt,
})
.from(workspace)