Skip to content

Commit 866dd93

Browse files
author
test
committed
fix(workflows): harden deployment auth and audit actor metadata
1 parent d3d15e6 commit 866dd93

File tree

7 files changed

+138
-59
lines changed

7 files changed

+138
-59
lines changed

apps/sim/app/api/workflows/[id]/deployments/[version]/route.test.ts

Lines changed: 26 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'
77

88
const {
99
mockActivateWorkflowVersion,
10+
mockAuthorizeWorkflowByWorkspacePermission,
1011
mockCreateSchedulesForDeploy,
1112
mockDbFrom,
1213
mockDbLimit,
@@ -22,6 +23,7 @@ const {
2223
mockValidateWorkflowAccess,
2324
} = vi.hoisted(() => ({
2425
mockActivateWorkflowVersion: vi.fn(),
26+
mockAuthorizeWorkflowByWorkspacePermission: vi.fn(),
2527
mockCreateSchedulesForDeploy: vi.fn(),
2628
mockDbFrom: vi.fn(),
2729
mockDbLimit: vi.fn(),
@@ -93,6 +95,11 @@ vi.mock('@/lib/mcp/workflow-mcp-sync', () => ({
9395
syncMcpToolsForWorkflow: (...args: unknown[]) => mockSyncMcpToolsForWorkflow(...args),
9496
}))
9597

98+
vi.mock('@/lib/workflows/utils', () => ({
99+
authorizeWorkflowByWorkspacePermission: (...args: unknown[]) =>
100+
mockAuthorizeWorkflowByWorkspacePermission(...args),
101+
}))
102+
96103
vi.mock('@/lib/audit/log', () => ({
97104
AuditAction: { WORKFLOW_DEPLOYMENT_ACTIVATED: 'WORKFLOW_DEPLOYMENT_ACTIVATED' },
98105
AuditResourceType: { WORKFLOW: 'WORKFLOW' },
@@ -127,6 +134,7 @@ describe('Workflow deployment version route', () => {
127134
success: true,
128135
deployedAt: '2024-01-17T12:00:00.000Z',
129136
})
137+
mockAuthorizeWorkflowByWorkspacePermission.mockResolvedValue({ allowed: true, status: 200 })
130138
})
131139

132140
it('uses write permission for metadata-only patch updates', async () => {
@@ -171,8 +179,10 @@ describe('Workflow deployment version route', () => {
171179
requireDeployment: false,
172180
action: 'write',
173181
})
174-
expect(mockValidateWorkflowAccess).toHaveBeenNthCalledWith(2, req, 'wf-1', {
175-
requireDeployment: false,
182+
expect(mockValidateWorkflowAccess).toHaveBeenCalledTimes(1)
183+
expect(mockAuthorizeWorkflowByWorkspacePermission).toHaveBeenCalledWith({
184+
workflowId: 'wf-1',
185+
userId: 'api-user',
176186
action: 'admin',
177187
})
178188
expect(mockSaveTriggerWebhooksForDeploy).toHaveBeenCalledWith(
@@ -206,14 +216,15 @@ describe('Workflow deployment version route', () => {
206216
})
207217

208218
it('returns admin auth failure before activation side effects', async () => {
209-
mockValidateWorkflowAccess
210-
.mockResolvedValueOnce({
211-
workflow: { id: 'wf-1', name: 'Test Workflow', workspaceId: 'ws-1' },
212-
auth: { success: true, userId: 'user-1', authType: 'session' },
213-
})
214-
.mockResolvedValueOnce({
215-
error: { message: 'Admin permission required', status: 403 },
216-
})
219+
mockValidateWorkflowAccess.mockResolvedValue({
220+
workflow: { id: 'wf-1', name: 'Test Workflow', workspaceId: 'ws-1' },
221+
auth: { success: true, userId: 'user-1', authType: 'session' },
222+
})
223+
mockAuthorizeWorkflowByWorkspacePermission.mockResolvedValue({
224+
allowed: false,
225+
status: 403,
226+
message: 'Admin permission required',
227+
})
217228

218229
const req = new NextRequest('http://localhost:3000/api/workflows/wf-1/deployments/3', {
219230
method: 'PATCH',
@@ -223,13 +234,14 @@ describe('Workflow deployment version route', () => {
223234
const response = await PATCH(req, { params: Promise.resolve({ id: 'wf-1', version: '3' }) })
224235

225236
expect(response.status).toBe(403)
226-
expect(mockValidateWorkflowAccess).toHaveBeenCalledTimes(2)
227-
expect(mockValidateWorkflowAccess).toHaveBeenNthCalledWith(1, req, 'wf-1', {
237+
expect(mockValidateWorkflowAccess).toHaveBeenCalledTimes(1)
238+
expect(mockValidateWorkflowAccess).toHaveBeenCalledWith(req, 'wf-1', {
228239
requireDeployment: false,
229240
action: 'write',
230241
})
231-
expect(mockValidateWorkflowAccess).toHaveBeenNthCalledWith(2, req, 'wf-1', {
232-
requireDeployment: false,
242+
expect(mockAuthorizeWorkflowByWorkspacePermission).toHaveBeenCalledWith({
243+
workflowId: 'wf-1',
244+
userId: 'user-1',
233245
action: 'admin',
234246
})
235247
expect(mockDbSelect).not.toHaveBeenCalled()

apps/sim/app/api/workflows/[id]/deployments/[version]/route.ts

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
createSchedulesForDeploy,
1515
validateWorkflowSchedules,
1616
} from '@/lib/workflows/schedules'
17+
import { authorizeWorkflowByWorkspacePermission } from '@/lib/workflows/utils'
1718
import { validateWorkflowAccess } from '@/app/api/workflows/middleware'
1819
import { createErrorResponse, createSuccessResponse } from '@/app/api/workflows/utils'
1920
import type { BlockState } from '@/stores/workflows/workflow/types'
@@ -116,20 +117,24 @@ export async function PATCH(
116117
}
117118

118119
const { name, description, isActive } = validation.data
120+
const auth = access.auth
121+
const workflowData = access.workflow
119122

120123
if (isActive) {
121-
const activationAccess = await validateWorkflowAccess(request, id, {
122-
requireDeployment: false,
124+
if (!auth?.userId) {
125+
return createErrorResponse('Unable to determine activating user', 400)
126+
}
127+
128+
const authorization = await authorizeWorkflowByWorkspacePermission({
129+
workflowId: id,
130+
userId: auth.userId,
123131
action: 'admin',
124132
})
125-
if (activationAccess.error) {
126-
return createErrorResponse(activationAccess.error.message, activationAccess.error.status)
133+
if (!authorization.allowed) {
134+
return createErrorResponse(authorization.message || 'Access denied', authorization.status)
127135
}
128136
}
129137

130-
const auth = access.auth
131-
const workflowData = access.workflow
132-
133138
const versionNum = Number(version)
134139
if (!Number.isFinite(versionNum)) {
135140
return createErrorResponse('Invalid version', 400)

apps/sim/app/api/workflows/[id]/route.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -422,7 +422,7 @@ describe('Workflow By ID API Route', () => {
422422
expect(auditMock.recordAudit).toHaveBeenCalledWith(
423423
expect.objectContaining({
424424
actorId: 'api-user-1',
425-
actorName: undefined,
425+
actorName: 'API Key Actor',
426426
actorEmail: undefined,
427427
})
428428
)

apps/sim/app/api/workflows/middleware.test.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,4 +170,39 @@ describe('validateWorkflowAccess', () => {
170170

171171
expect(result).toEqual({ workflow, auth })
172172
})
173+
174+
it('returns 404 for deployed access when workflow is missing', async () => {
175+
mockGetWorkflowById.mockResolvedValue(null)
176+
177+
const result = await validateWorkflowAccess(createRequest(), WORKFLOW_ID, {
178+
requireDeployment: true,
179+
})
180+
181+
expect(result).toEqual({
182+
error: {
183+
message: 'Workflow not found',
184+
status: 404,
185+
},
186+
})
187+
expect(mockCheckHybridAuth).not.toHaveBeenCalled()
188+
expect(mockAuthenticateApiKeyFromHeader).not.toHaveBeenCalled()
189+
})
190+
191+
it('returns 403 for deployed access when workflow has no workspace', async () => {
192+
mockGetWorkflowById.mockResolvedValue(createWorkflow({ workspaceId: null, isDeployed: true }))
193+
194+
const result = await validateWorkflowAccess(createRequest(), WORKFLOW_ID, {
195+
requireDeployment: true,
196+
})
197+
198+
expect(result).toEqual({
199+
error: {
200+
message:
201+
'This workflow is not attached to a workspace. Personal workflows are deprecated and cannot be accessed.',
202+
status: 403,
203+
},
204+
})
205+
expect(mockCheckHybridAuth).not.toHaveBeenCalled()
206+
expect(mockAuthenticateApiKeyFromHeader).not.toHaveBeenCalled()
207+
})
173208
})

apps/sim/app/api/workflows/middleware.ts

Lines changed: 32 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,30 @@ export interface WorkflowAccessOptions {
2323
allowInternalSecret?: boolean
2424
}
2525

26+
async function getValidatedWorkflow(workflowId: string): Promise<ValidationResult> {
27+
const workflow = await getWorkflowById(workflowId)
28+
if (!workflow) {
29+
return {
30+
error: {
31+
message: 'Workflow not found',
32+
status: 404,
33+
},
34+
}
35+
}
36+
37+
if (!workflow.workspaceId) {
38+
return {
39+
error: {
40+
message:
41+
'This workflow is not attached to a workspace. Personal workflows are deprecated and cannot be accessed.',
42+
status: 403,
43+
},
44+
}
45+
}
46+
47+
return { workflow }
48+
}
49+
2650
export async function validateWorkflowAccess(
2751
request: NextRequest,
2852
workflowId: string,
@@ -46,25 +70,11 @@ export async function validateWorkflowAccess(
4670
}
4771
}
4872

49-
const workflow = await getWorkflowById(workflowId)
50-
if (!workflow) {
51-
return {
52-
error: {
53-
message: 'Workflow not found',
54-
status: 404,
55-
},
56-
}
57-
}
58-
59-
if (!workflow.workspaceId) {
60-
return {
61-
error: {
62-
message:
63-
'This workflow is not attached to a workspace. Personal workflows are deprecated and cannot be accessed.',
64-
status: 403,
65-
},
66-
}
73+
const workflowResult = await getValidatedWorkflow(workflowId)
74+
if (workflowResult.error || !workflowResult.workflow) {
75+
return workflowResult
6776
}
77+
const workflow = workflowResult.workflow
6878

6979
const authorization = await authorizeWorkflowByWorkspacePermission({
7080
workflowId,
@@ -83,25 +93,11 @@ export async function validateWorkflowAccess(
8393
return { workflow, auth }
8494
}
8595

86-
const workflow = await getWorkflowById(workflowId)
87-
if (!workflow) {
88-
return {
89-
error: {
90-
message: 'Workflow not found',
91-
status: 404,
92-
},
93-
}
94-
}
95-
96-
if (!workflow.workspaceId) {
97-
return {
98-
error: {
99-
message:
100-
'This workflow is not attached to a workspace. Personal workflows are deprecated and cannot be accessed.',
101-
status: 403,
102-
},
103-
}
96+
const workflowResult = await getValidatedWorkflow(workflowId)
97+
if (workflowResult.error || !workflowResult.workflow) {
98+
return workflowResult
10499
}
100+
const workflow = workflowResult.workflow
105101

106102
if (requireDeployment) {
107103
if (!workflow.isDeployed) {
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
5+
import { describe, expect, it } from 'vitest'
6+
import { getAuditActorMetadata } from '@/lib/audit/actor-metadata'
7+
import { AuthType } from '@/lib/auth/hybrid'
8+
9+
describe('getAuditActorMetadata', () => {
10+
it('preserves actor metadata for API key auth when present', () => {
11+
expect(
12+
getAuditActorMetadata({
13+
success: true,
14+
userId: 'api-user',
15+
userName: 'API Key Actor',
16+
userEmail: 'api@example.com',
17+
authType: AuthType.API_KEY,
18+
})
19+
).toEqual({
20+
actorName: 'API Key Actor',
21+
actorEmail: 'api@example.com',
22+
})
23+
})
24+
25+
it('returns undefined metadata when auth is missing', () => {
26+
expect(getAuditActorMetadata(null)).toEqual({
27+
actorName: undefined,
28+
actorEmail: undefined,
29+
})
30+
})
31+
})

apps/sim/lib/audit/actor-metadata.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ export function getAuditActorMetadata(auth: AuthResult | null | undefined): {
44
actorName: string | undefined
55
actorEmail: string | undefined
66
} {
7-
if (auth?.authType !== 'session') {
7+
if (!auth) {
88
return {
99
actorName: undefined,
1010
actorEmail: undefined,

0 commit comments

Comments
 (0)