Skip to content

Commit abbf8c9

Browse files
author
test
committed
fix(workflows): enforce deployment patch auth ordering
1 parent 965e9ac commit abbf8c9

File tree

2 files changed

+96
-9
lines changed

2 files changed

+96
-9
lines changed

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

Lines changed: 80 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,30 @@ describe('Workflow deployment version route', () => {
129129
})
130130
})
131131

132+
it('uses write permission for metadata-only patch updates', async () => {
133+
mockValidateWorkflowAccess.mockResolvedValue({
134+
workflow: { id: 'wf-1', name: 'Test Workflow', workspaceId: 'ws-1' },
135+
auth: { success: true, userId: 'user-1', authType: 'session' },
136+
})
137+
mockDbReturning.mockResolvedValue([{ id: 'dep-3', name: 'Renamed', description: 'Updated' }])
138+
139+
const req = new NextRequest('http://localhost:3000/api/workflows/wf-1/deployments/3', {
140+
method: 'PATCH',
141+
headers: { 'content-type': 'application/json' },
142+
body: JSON.stringify({ name: 'Renamed', description: 'Updated' }),
143+
})
144+
const response = await PATCH(req, { params: Promise.resolve({ id: 'wf-1', version: '3' }) })
145+
146+
expect(response.status).toBe(200)
147+
expect(mockValidateWorkflowAccess).toHaveBeenCalledTimes(1)
148+
expect(mockValidateWorkflowAccess).toHaveBeenCalledWith(req, 'wf-1', {
149+
requireDeployment: false,
150+
action: 'write',
151+
})
152+
expect(mockDbUpdate).toHaveBeenCalledTimes(1)
153+
expect(mockActivateWorkflowVersion).not.toHaveBeenCalled()
154+
})
155+
132156
it('allows API-key auth for activation using hybrid auth userId', async () => {
133157
mockValidateWorkflowAccess.mockResolvedValue({
134158
workflow: { id: 'wf-1', name: 'Test Workflow', workspaceId: 'ws-1' },
@@ -143,7 +167,11 @@ describe('Workflow deployment version route', () => {
143167
const response = await PATCH(req, { params: Promise.resolve({ id: 'wf-1', version: '3' }) })
144168

145169
expect(response.status).toBe(200)
146-
expect(mockValidateWorkflowAccess).toHaveBeenCalledWith(req, 'wf-1', {
170+
expect(mockValidateWorkflowAccess).toHaveBeenNthCalledWith(1, req, 'wf-1', {
171+
requireDeployment: false,
172+
action: 'write',
173+
})
174+
expect(mockValidateWorkflowAccess).toHaveBeenNthCalledWith(2, req, 'wf-1', {
147175
requireDeployment: false,
148176
action: 'admin',
149177
})
@@ -158,4 +186,55 @@ describe('Workflow deployment version route', () => {
158186
})
159187
)
160188
})
189+
190+
it('returns write auth failure before parsing or updating metadata', async () => {
191+
mockValidateWorkflowAccess.mockResolvedValue({
192+
error: { message: 'Write permission required', status: 403 },
193+
})
194+
195+
const jsonSpy = vi.fn().mockResolvedValue({ name: 'Renamed' })
196+
const req = {
197+
json: jsonSpy,
198+
} as unknown as NextRequest
199+
200+
const response = await PATCH(req, { params: Promise.resolve({ id: 'wf-1', version: '3' }) })
201+
202+
expect(response.status).toBe(403)
203+
expect(jsonSpy).not.toHaveBeenCalled()
204+
expect(mockDbUpdate).not.toHaveBeenCalled()
205+
expect(mockActivateWorkflowVersion).not.toHaveBeenCalled()
206+
})
207+
208+
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+
})
217+
218+
const req = new NextRequest('http://localhost:3000/api/workflows/wf-1/deployments/3', {
219+
method: 'PATCH',
220+
headers: { 'content-type': 'application/json' },
221+
body: JSON.stringify({ isActive: true }),
222+
})
223+
const response = await PATCH(req, { params: Promise.resolve({ id: 'wf-1', version: '3' }) })
224+
225+
expect(response.status).toBe(403)
226+
expect(mockValidateWorkflowAccess).toHaveBeenCalledTimes(2)
227+
expect(mockValidateWorkflowAccess).toHaveBeenNthCalledWith(1, req, 'wf-1', {
228+
requireDeployment: false,
229+
action: 'write',
230+
})
231+
expect(mockValidateWorkflowAccess).toHaveBeenNthCalledWith(2, req, 'wf-1', {
232+
requireDeployment: false,
233+
action: 'admin',
234+
})
235+
expect(mockDbSelect).not.toHaveBeenCalled()
236+
expect(mockSaveTriggerWebhooksForDeploy).not.toHaveBeenCalled()
237+
expect(mockCreateSchedulesForDeploy).not.toHaveBeenCalled()
238+
expect(mockActivateWorkflowVersion).not.toHaveBeenCalled()
239+
})
161240
})

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

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,14 @@ export async function PATCH(
100100
const { id, version } = await params
101101

102102
try {
103+
const access = await validateWorkflowAccess(request, id, {
104+
requireDeployment: false,
105+
action: 'write',
106+
})
107+
if (access.error) {
108+
return createErrorResponse(access.error.message, access.error.status)
109+
}
110+
103111
const body = await request.json()
104112
const validation = patchBodySchema.safeParse(body)
105113

@@ -109,14 +117,14 @@ export async function PATCH(
109117

110118
const { name, description, isActive } = validation.data
111119

112-
// Activation requires admin permission, other updates require write
113-
const requiredPermission = isActive ? 'admin' : 'write'
114-
const access = await validateWorkflowAccess(request, id, {
115-
requireDeployment: false,
116-
action: requiredPermission,
117-
})
118-
if (access.error) {
119-
return createErrorResponse(access.error.message, access.error.status)
120+
if (isActive) {
121+
const activationAccess = await validateWorkflowAccess(request, id, {
122+
requireDeployment: false,
123+
action: 'admin',
124+
})
125+
if (activationAccess.error) {
126+
return createErrorResponse(activationAccess.error.message, activationAccess.error.status)
127+
}
120128
}
121129

122130
const auth = access.auth

0 commit comments

Comments
 (0)