Skip to content

Commit e77c8fb

Browse files
PlaneInABottleGitHub Copilot
PlaneInABottle
and
GitHub Copilot
committed
fix: harden workflow API auth routes
Co-authored-by: GitHub Copilot <github-copilot[bot]@users.noreply.github.com>
1 parent 3fbf489 commit e77c8fb

File tree

7 files changed

+269
-81
lines changed

7 files changed

+269
-81
lines changed

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

Lines changed: 86 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -5,16 +5,43 @@
55
import { NextRequest } from 'next/server'
66
import { beforeEach, describe, expect, it, vi } from 'vitest'
77

8-
const mockValidateWorkflowAccess = vi.fn()
9-
const mockDbSelect = vi.fn()
10-
const mockDbFrom = vi.fn()
11-
const mockDbWhere = vi.fn()
12-
const mockDbLimit = vi.fn()
13-
const mockDbOrderBy = vi.fn()
14-
const mockDeployWorkflow = vi.fn()
15-
const mockUndeployWorkflow = vi.fn()
16-
const mockCleanupWebhooksForWorkflow = vi.fn()
17-
const mockRemoveMcpToolsForWorkflow = vi.fn()
8+
const {
9+
mockCleanupWebhooksForWorkflow,
10+
mockDbLimit,
11+
mockDbOrderBy,
12+
mockDbFrom,
13+
mockDbSelect,
14+
mockDbSet,
15+
mockDbUpdate,
16+
mockDbWhere,
17+
mockCreateSchedulesForDeploy,
18+
mockDeployWorkflow,
19+
mockLoadWorkflowFromNormalizedTables,
20+
mockRemoveMcpToolsForWorkflow,
21+
mockSaveTriggerWebhooksForDeploy,
22+
mockSyncMcpToolsForWorkflow,
23+
mockUndeployWorkflow,
24+
mockValidatePublicApiAllowed,
25+
mockValidateWorkflowAccess,
26+
} = vi.hoisted(() => ({
27+
mockCleanupWebhooksForWorkflow: vi.fn(),
28+
mockDbLimit: vi.fn(),
29+
mockDbOrderBy: vi.fn(),
30+
mockDbFrom: vi.fn(),
31+
mockDbSelect: vi.fn(),
32+
mockDbSet: vi.fn(),
33+
mockDbUpdate: vi.fn(),
34+
mockDbWhere: vi.fn(),
35+
mockCreateSchedulesForDeploy: vi.fn(),
36+
mockDeployWorkflow: vi.fn(),
37+
mockLoadWorkflowFromNormalizedTables: vi.fn(),
38+
mockRemoveMcpToolsForWorkflow: vi.fn(),
39+
mockSaveTriggerWebhooksForDeploy: vi.fn(),
40+
mockSyncMcpToolsForWorkflow: vi.fn(),
41+
mockUndeployWorkflow: vi.fn(),
42+
mockValidatePublicApiAllowed: vi.fn(),
43+
mockValidateWorkflowAccess: vi.fn(),
44+
}))
1845

1946
vi.mock('@sim/logger', () => ({
2047
createLogger: () => ({ info: vi.fn(), warn: vi.fn(), error: vi.fn() }),
@@ -33,19 +60,23 @@ vi.mock('@/lib/core/utils/request', () => ({
3360
}))
3461

3562
vi.mock('@sim/db', () => ({
36-
db: { select: mockDbSelect },
63+
db: { select: mockDbSelect, update: mockDbUpdate },
3764
workflow: { variables: 'variables', id: 'id' },
3865
workflowDeploymentVersion: { state: 'state', workflowId: 'workflowId', isActive: 'isActive', createdAt: 'createdAt', id: 'id' },
3966
}))
4067

41-
vi.mock('drizzle-orm', () => ({
42-
and: vi.fn(),
43-
desc: vi.fn(),
44-
eq: vi.fn(),
45-
}))
68+
vi.mock('drizzle-orm', async (importOriginal) => {
69+
const actual = await importOriginal<typeof import('drizzle-orm')>()
70+
return {
71+
...actual,
72+
and: vi.fn(),
73+
desc: vi.fn(),
74+
eq: vi.fn(),
75+
}
76+
})
4677

4778
vi.mock('@/lib/workflows/persistence/utils', () => ({
48-
loadWorkflowFromNormalizedTables: vi.fn().mockResolvedValue(null),
79+
loadWorkflowFromNormalizedTables: (...args: unknown[]) => mockLoadWorkflowFromNormalizedTables(...args),
4980
deployWorkflow: (...args: unknown[]) => mockDeployWorkflow(...args),
5081
undeployWorkflow: (...args: unknown[]) => mockUndeployWorkflow(...args),
5182
}))
@@ -56,19 +87,19 @@ vi.mock('@/lib/workflows/comparison', () => ({
5687

5788
vi.mock('@/lib/workflows/schedules', () => ({
5889
cleanupDeploymentVersion: vi.fn(),
59-
createSchedulesForDeploy: vi.fn(),
90+
createSchedulesForDeploy: (...args: unknown[]) => mockCreateSchedulesForDeploy(...args),
6091
validateWorkflowSchedules: vi.fn().mockReturnValue({ isValid: true }),
6192
}))
6293

6394
vi.mock('@/lib/webhooks/deploy', () => ({
6495
cleanupWebhooksForWorkflow: (...args: unknown[]) => mockCleanupWebhooksForWorkflow(...args),
6596
restorePreviousVersionWebhooks: vi.fn(),
66-
saveTriggerWebhooksForDeploy: vi.fn(),
97+
saveTriggerWebhooksForDeploy: (...args: unknown[]) => mockSaveTriggerWebhooksForDeploy(...args),
6798
}))
6899

69100
vi.mock('@/lib/mcp/workflow-mcp-sync', () => ({
70101
removeMcpToolsForWorkflow: (...args: unknown[]) => mockRemoveMcpToolsForWorkflow(...args),
71-
syncMcpToolsForWorkflow: vi.fn(),
102+
syncMcpToolsForWorkflow: (...args: unknown[]) => mockSyncMcpToolsForWorkflow(...args),
72103
}))
73104

74105
vi.mock('@/lib/audit/log', () => ({
@@ -77,7 +108,12 @@ vi.mock('@/lib/audit/log', () => ({
77108
recordAudit: vi.fn(),
78109
}))
79110

80-
import { POST, DELETE } from '@/app/api/workflows/[id]/deploy/route'
111+
vi.mock('@/ee/access-control/utils/permission-check', () => ({
112+
PublicApiNotAllowedError: class PublicApiNotAllowedError extends Error {},
113+
validatePublicApiAllowed: (...args: unknown[]) => mockValidatePublicApiAllowed(...args),
114+
}))
115+
116+
import { DELETE, PATCH, POST } from '@/app/api/workflows/[id]/deploy/route'
81117

82118
describe('Workflow deploy route', () => {
83119
beforeEach(() => {
@@ -87,8 +123,20 @@ describe('Workflow deploy route', () => {
87123
mockDbWhere.mockReturnValue({ limit: mockDbLimit, orderBy: mockDbOrderBy })
88124
mockDbOrderBy.mockReturnValue({ limit: mockDbLimit })
89125
mockDbLimit.mockResolvedValue([])
126+
mockDbUpdate.mockReturnValue({ set: mockDbSet })
127+
mockDbSet.mockReturnValue({ where: mockDbWhere })
90128
mockCleanupWebhooksForWorkflow.mockResolvedValue(undefined)
129+
mockCreateSchedulesForDeploy.mockResolvedValue({ success: true })
130+
mockLoadWorkflowFromNormalizedTables.mockResolvedValue({
131+
blocks: { 'block-1': { id: 'block-1', type: 'start_trigger', name: 'Start' } },
132+
edges: [],
133+
loops: {},
134+
parallels: {},
135+
})
136+
mockSaveTriggerWebhooksForDeploy.mockResolvedValue({ success: true, warnings: [] })
91137
mockRemoveMcpToolsForWorkflow.mockResolvedValue(undefined)
138+
mockSyncMcpToolsForWorkflow.mockResolvedValue(undefined)
139+
mockValidatePublicApiAllowed.mockResolvedValue(undefined)
92140
})
93141

94142
it('allows API-key auth for deploy using hybrid auth userId', async () => {
@@ -136,4 +184,21 @@ describe('Workflow deploy route', () => {
136184
expect(data.isDeployed).toBe(false)
137185
expect(mockUndeployWorkflow).toHaveBeenCalledWith({ workflowId: 'wf-1' })
138186
})
187+
188+
it('checks public API restrictions against hybrid auth userId', async () => {
189+
mockValidateWorkflowAccess.mockResolvedValue({
190+
workflow: { id: 'wf-1', name: 'Test Workflow', workspaceId: 'ws-1' },
191+
auth: { success: true, userId: 'api-user', authType: 'api_key' },
192+
})
193+
194+
const req = new NextRequest('http://localhost:3000/api/workflows/wf-1/deploy', {
195+
method: 'PATCH',
196+
headers: { 'content-type': 'application/json', 'x-api-key': 'test-key' },
197+
body: JSON.stringify({ isPublicApi: true }),
198+
})
199+
const response = await PATCH(req, { params: Promise.resolve({ id: 'wf-1' }) })
200+
201+
expect(response.status).toBe(200)
202+
expect(mockValidatePublicApiAllowed).toHaveBeenCalledWith('api-user')
203+
})
139204
})

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

Lines changed: 42 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { db, workflow, workflowDeploymentVersion } from '@sim/db'
22
import { createLogger } from '@sim/logger'
33
import { and, desc, eq } from 'drizzle-orm'
44
import type { NextRequest } from 'next/server'
5+
import type { AuthResult } from '@/lib/auth/hybrid'
56
import { AuditAction, AuditResourceType, recordAudit } from '@/lib/audit/log'
67
import { generateRequestId } from '@/lib/core/utils/request'
78
import { removeMcpToolsForWorkflow, syncMcpToolsForWorkflow } from '@/lib/mcp/workflow-mcp-sync'
@@ -30,22 +31,57 @@ const logger = createLogger('WorkflowDeployAPI')
3031
export const dynamic = 'force-dynamic'
3132
export const runtime = 'nodejs'
3233

34+
type LifecycleAdminAccessResult = {
35+
error: { message: string; status: number } | null | undefined
36+
auth: AuthResult | null | undefined
37+
session:
38+
| Awaited<ReturnType<typeof validateWorkflowPermissions>>['session']
39+
| null
40+
| undefined
41+
workflow:
42+
| Awaited<ReturnType<typeof validateWorkflowPermissions>>['workflow']
43+
| Awaited<ReturnType<typeof validateWorkflowAccess>>['workflow']
44+
| null
45+
| undefined
46+
}
47+
3348
async function validateLifecycleAdminAccess(
3449
request: NextRequest,
3550
workflowId: string,
3651
requestId: string
37-
) {
52+
): Promise<LifecycleAdminAccessResult> {
3853
const hybridAccess = await validateWorkflowAccess(request, workflowId, {
3954
requireDeployment: false,
4055
action: 'admin',
4156
})
4257

4358
if (hybridAccess.error) {
44-
return hybridAccess
59+
return {
60+
error: hybridAccess.error,
61+
auth: hybridAccess.auth,
62+
session: null,
63+
workflow: hybridAccess.workflow,
64+
}
4565
}
4666

4767
if (hybridAccess.auth?.authType === 'session') {
48-
return await validateWorkflowPermissions(workflowId, requestId, 'admin')
68+
const sessionAccess = await validateWorkflowPermissions(workflowId, requestId, 'admin')
69+
const auth: AuthResult | null = sessionAccess.session?.user?.id
70+
? {
71+
success: true,
72+
userId: sessionAccess.session.user.id,
73+
userName: sessionAccess.session.user.name,
74+
userEmail: sessionAccess.session.user.email,
75+
authType: 'session',
76+
}
77+
: null
78+
79+
return {
80+
error: sessionAccess.error,
81+
auth,
82+
session: sessionAccess.session,
83+
workflow: sessionAccess.workflow,
84+
}
4985
}
5086

5187
return {
@@ -338,7 +374,7 @@ export async function PATCH(request: NextRequest, { params }: { params: Promise<
338374
const { id } = await params
339375

340376
try {
341-
const { error, session } = await validateLifecycleAdminAccess(request, id, requestId)
377+
const { auth, error, session } = await validateLifecycleAdminAccess(request, id, requestId)
342378
if (error) {
343379
return createErrorResponse(error.message, error.status)
344380
}
@@ -354,8 +390,9 @@ export async function PATCH(request: NextRequest, { params }: { params: Promise<
354390
const { validatePublicApiAllowed, PublicApiNotAllowedError } = await import(
355391
'@/ee/access-control/utils/permission-check'
356392
)
393+
const actorUserId = session?.user?.id ?? auth?.userId
357394
try {
358-
await validatePublicApiAllowed(session?.user?.id)
395+
await validatePublicApiAllowed(actorUserId)
359396
} catch (err) {
360397
if (err instanceof PublicApiNotAllowedError) {
361398
return createErrorResponse('Public API access is disabled', 403)

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

Lines changed: 31 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,16 +5,29 @@
55
import { NextRequest } from 'next/server'
66
import { beforeEach, describe, expect, it, vi } from 'vitest'
77

8-
const mockValidateWorkflowAccess = vi.fn()
9-
const mockDbSelect = vi.fn()
10-
const mockDbFrom = vi.fn()
11-
const mockDbWhere = vi.fn()
12-
const mockDbLimit = vi.fn()
13-
const mockDbUpdate = vi.fn()
14-
const mockDbSet = vi.fn()
15-
const mockDbWhereUpdate = vi.fn()
16-
const mockSaveWorkflowToNormalizedTables = vi.fn()
17-
const mockSyncMcpToolsForWorkflow = vi.fn()
8+
const {
9+
mockDbFrom,
10+
mockDbLimit,
11+
mockDbSelect,
12+
mockDbSet,
13+
mockDbUpdate,
14+
mockDbWhere,
15+
mockDbWhereUpdate,
16+
mockSaveWorkflowToNormalizedTables,
17+
mockSyncMcpToolsForWorkflow,
18+
mockValidateWorkflowAccess,
19+
} = vi.hoisted(() => ({
20+
mockDbFrom: vi.fn(),
21+
mockDbLimit: vi.fn(),
22+
mockDbSelect: vi.fn(),
23+
mockDbSet: vi.fn(),
24+
mockDbUpdate: vi.fn(),
25+
mockDbWhere: vi.fn(),
26+
mockDbWhereUpdate: vi.fn(),
27+
mockSaveWorkflowToNormalizedTables: vi.fn(),
28+
mockSyncMcpToolsForWorkflow: vi.fn(),
29+
mockValidateWorkflowAccess: vi.fn(),
30+
}))
1831
const mockFetch = vi.fn()
1932

2033
vi.stubGlobal('fetch', mockFetch)
@@ -49,10 +62,14 @@ vi.mock('@sim/db', () => ({
4962
},
5063
}))
5164

52-
vi.mock('drizzle-orm', () => ({
53-
and: vi.fn(),
54-
eq: vi.fn(),
55-
}))
65+
vi.mock('drizzle-orm', async (importOriginal) => {
66+
const actual = await importOriginal<typeof import('drizzle-orm')>()
67+
return {
68+
...actual,
69+
and: vi.fn(),
70+
eq: vi.fn(),
71+
}
72+
})
5673

5774
vi.mock('@/lib/workflows/persistence/utils', () => ({
5875
saveWorkflowToNormalizedTables: (...args: unknown[]) => mockSaveWorkflowToNormalizedTables(...args),

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

Lines changed: 42 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,35 @@
55
import { NextRequest } from 'next/server'
66
import { beforeEach, describe, expect, it, vi } from 'vitest'
77

8-
const mockValidateWorkflowAccess = vi.fn()
9-
const mockDbSelect = vi.fn()
10-
const mockDbFrom = vi.fn()
11-
const mockDbWhere = vi.fn()
12-
const mockDbLimit = vi.fn()
13-
const mockSaveTriggerWebhooksForDeploy = vi.fn()
14-
const mockCreateSchedulesForDeploy = vi.fn()
15-
const mockActivateWorkflowVersion = vi.fn()
16-
const mockSyncMcpToolsForWorkflow = vi.fn()
8+
const {
9+
mockActivateWorkflowVersion,
10+
mockCreateSchedulesForDeploy,
11+
mockDbFrom,
12+
mockDbLimit,
13+
mockDbReturning,
14+
mockDbSelect,
15+
mockDbSet,
16+
mockDbUpdate,
17+
mockDbWhere,
18+
mockDbWhereUpdate,
19+
mockSaveTriggerWebhooksForDeploy,
20+
mockSyncMcpToolsForWorkflow,
21+
mockValidateWorkflowAccess,
22+
} = vi.hoisted(() => ({
23+
mockActivateWorkflowVersion: vi.fn(),
24+
mockCreateSchedulesForDeploy: vi.fn(),
25+
mockDbFrom: vi.fn(),
26+
mockDbLimit: vi.fn(),
27+
mockDbReturning: vi.fn(),
28+
mockDbSelect: vi.fn(),
29+
mockDbSet: vi.fn(),
30+
mockDbUpdate: vi.fn(),
31+
mockDbWhere: vi.fn(),
32+
mockDbWhereUpdate: vi.fn(),
33+
mockSaveTriggerWebhooksForDeploy: vi.fn(),
34+
mockSyncMcpToolsForWorkflow: vi.fn(),
35+
mockValidateWorkflowAccess: vi.fn(),
36+
}))
1737

1838
vi.mock('@sim/logger', () => ({
1939
createLogger: () => ({ info: vi.fn(), warn: vi.fn(), error: vi.fn() }),
@@ -30,7 +50,7 @@ vi.mock('@/lib/core/utils/request', () => ({
3050
vi.mock('@sim/db', () => ({
3151
db: {
3252
select: mockDbSelect,
33-
update: vi.fn(() => ({ set: vi.fn(() => ({ where: vi.fn(() => ({ returning: vi.fn() })) })) }),
53+
update: mockDbUpdate,
3454
},
3555
workflowDeploymentVersion: {
3656
id: 'id',
@@ -43,10 +63,14 @@ vi.mock('@sim/db', () => ({
4363
},
4464
}))
4565

46-
vi.mock('drizzle-orm', () => ({
47-
and: vi.fn(),
48-
eq: vi.fn(),
49-
}))
66+
vi.mock('drizzle-orm', async (importOriginal) => {
67+
const actual = await importOriginal<typeof import('drizzle-orm')>()
68+
return {
69+
...actual,
70+
and: vi.fn(),
71+
eq: vi.fn(),
72+
}
73+
})
5074

5175
vi.mock('@/lib/webhooks/deploy', () => ({
5276
restorePreviousVersionWebhooks: vi.fn(),
@@ -91,6 +115,10 @@ describe('Workflow deployment version route', () => {
91115
},
92116
])
93117
.mockResolvedValueOnce([{ id: 'dep-2' }])
118+
mockDbUpdate.mockReturnValue({ set: mockDbSet })
119+
mockDbSet.mockReturnValue({ where: mockDbWhereUpdate })
120+
mockDbWhereUpdate.mockReturnValue({ returning: mockDbReturning })
121+
mockDbReturning.mockResolvedValue([])
94122
mockSaveTriggerWebhooksForDeploy.mockResolvedValue({ success: true, warnings: [] })
95123
mockCreateSchedulesForDeploy.mockResolvedValue({ success: true })
96124
mockActivateWorkflowVersion.mockResolvedValue({

0 commit comments

Comments
 (0)