From 3344b679494eb5b685dfeb958e528d4068beb878 Mon Sep 17 00:00:00 2001 From: waleed Date: Fri, 3 Jul 2026 14:17:41 -0700 Subject: [PATCH 1/2] fix(tables): verify workflow belongs to table's workspace before binding Add a check that a table workflow group's workflowId resolves to an active workflow in the table's own workspace, in both the create and update handlers, before it is persisted. --- .../api/table/[tableId]/groups/route.test.ts | 209 ++++++++++++++++++ .../app/api/table/[tableId]/groups/route.ts | 31 +++ 2 files changed, 240 insertions(+) create mode 100644 apps/sim/app/api/table/[tableId]/groups/route.test.ts diff --git a/apps/sim/app/api/table/[tableId]/groups/route.test.ts b/apps/sim/app/api/table/[tableId]/groups/route.test.ts new file mode 100644 index 00000000000..674298e8c66 --- /dev/null +++ b/apps/sim/app/api/table/[tableId]/groups/route.test.ts @@ -0,0 +1,209 @@ +/** + * @vitest-environment node + */ +import { hybridAuthMockFns, workflowAuthzMockFns } from '@sim/testing' +import { NextRequest } from 'next/server' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { TableDefinition } from '@/lib/table' + +const { mockCheckAccess, mockAddWorkflowGroup, mockUpdateWorkflowGroup } = vi.hoisted(() => ({ + mockCheckAccess: vi.fn(), + mockAddWorkflowGroup: vi.fn(), + mockUpdateWorkflowGroup: vi.fn(), +})) + +vi.mock('@/app/api/table/utils', async () => { + const { NextResponse } = await import('next/server') + return { + accessError: (result: { status: number }) => + NextResponse.json({ error: 'denied' }, { status: result.status }), + checkAccess: mockCheckAccess, + normalizeColumn: (column: unknown) => column, + } +}) + +vi.mock('@/lib/table/workflow-groups/service', () => ({ + addWorkflowGroup: mockAddWorkflowGroup, + updateWorkflowGroup: mockUpdateWorkflowGroup, + deleteWorkflowGroup: vi.fn(), +})) + +import { PATCH, POST } from '@/app/api/table/[tableId]/groups/route' + +function buildTable(overrides: Partial = {}): TableDefinition { + return { + id: 'tbl_1', + name: 'People', + description: null, + schema: { columns: [] }, + metadata: null, + rowCount: 0, + maxRows: 100, + workspaceId: 'workspace-1', + createdBy: 'user-1', + archivedAt: null, + createdAt: new Date('2024-01-01'), + updatedAt: new Date('2024-01-01'), + ...overrides, + } +} + +function callPost(body: Record, tableId = 'tbl_1') { + const req = new NextRequest(`http://localhost:3000/api/table/${tableId}/groups`, { + method: 'POST', + body: JSON.stringify(body), + headers: { 'Content-Type': 'application/json' }, + }) + return POST(req, { params: Promise.resolve({ tableId }) }) +} + +function callPatch(body: Record, tableId = 'tbl_1') { + const req = new NextRequest(`http://localhost:3000/api/table/${tableId}/groups`, { + method: 'PATCH', + body: JSON.stringify(body), + headers: { 'Content-Type': 'application/json' }, + }) + return PATCH(req, { params: Promise.resolve({ tableId }) }) +} + +const baseGroup = { + id: 'grp_1', + workflowId: 'wf_1', + outputs: [{ blockId: 'block_1', path: 'result', columnName: 'result' }], +} + +const baseOutputColumns = [{ name: 'result', type: 'string', workflowGroupId: 'grp_1' }] + +describe('POST /api/table/[tableId]/groups', () => { + beforeEach(() => { + vi.clearAllMocks() + hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({ + success: true, + userId: 'user-1', + authType: 'session', + }) + mockCheckAccess.mockResolvedValue({ ok: true, table: buildTable() }) + workflowAuthzMockFns.mockGetActiveWorkflowContext.mockResolvedValue({ + workflow: { id: 'wf_1' }, + workspaceId: 'workspace-1', + workspaceOrganizationId: null, + }) + mockAddWorkflowGroup.mockResolvedValue({ + schema: { columns: baseOutputColumns, workflowGroups: [baseGroup] }, + }) + }) + + it('rejects a workflowId belonging to a different workspace', async () => { + workflowAuthzMockFns.mockGetActiveWorkflowContext.mockResolvedValue({ + workflow: { id: 'wf_1' }, + workspaceId: 'other-workspace', + workspaceOrganizationId: null, + }) + const res = await callPost({ + workspaceId: 'workspace-1', + group: baseGroup, + outputColumns: baseOutputColumns, + }) + expect(res.status).toBe(400) + expect(mockAddWorkflowGroup).not.toHaveBeenCalled() + }) + + it('rejects a nonexistent workflowId', async () => { + workflowAuthzMockFns.mockGetActiveWorkflowContext.mockResolvedValue(null) + const res = await callPost({ + workspaceId: 'workspace-1', + group: baseGroup, + outputColumns: baseOutputColumns, + }) + expect(res.status).toBe(400) + expect(mockAddWorkflowGroup).not.toHaveBeenCalled() + }) + + it('succeeds when the workflow belongs to the same workspace', async () => { + const res = await callPost({ + workspaceId: 'workspace-1', + group: baseGroup, + outputColumns: baseOutputColumns, + }) + expect(res.status).toBe(200) + expect(mockAddWorkflowGroup).toHaveBeenCalled() + }) + + it('skips the workflow check for enrichment groups without a workflowId', async () => { + const res = await callPost({ + workspaceId: 'workspace-1', + group: { ...baseGroup, workflowId: '', enrichmentId: 'enrich_1' }, + outputColumns: baseOutputColumns, + }) + expect(res.status).toBe(200) + expect(workflowAuthzMockFns.mockGetActiveWorkflowContext).not.toHaveBeenCalled() + expect(mockAddWorkflowGroup).toHaveBeenCalled() + }) +}) + +describe('PATCH /api/table/[tableId]/groups', () => { + beforeEach(() => { + vi.clearAllMocks() + hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({ + success: true, + userId: 'user-1', + authType: 'session', + }) + mockCheckAccess.mockResolvedValue({ ok: true, table: buildTable() }) + workflowAuthzMockFns.mockGetActiveWorkflowContext.mockResolvedValue({ + workflow: { id: 'wf_1' }, + workspaceId: 'workspace-1', + workspaceOrganizationId: null, + }) + mockUpdateWorkflowGroup.mockResolvedValue({ + schema: { columns: baseOutputColumns, workflowGroups: [baseGroup] }, + }) + }) + + it('rejects changing workflowId to one in a different workspace', async () => { + workflowAuthzMockFns.mockGetActiveWorkflowContext.mockResolvedValue({ + workflow: { id: 'wf_2' }, + workspaceId: 'other-workspace', + workspaceOrganizationId: null, + }) + const res = await callPatch({ + workspaceId: 'workspace-1', + groupId: 'grp_1', + workflowId: 'wf_2', + }) + expect(res.status).toBe(400) + expect(mockUpdateWorkflowGroup).not.toHaveBeenCalled() + }) + + it('rejects a nonexistent workflowId', async () => { + workflowAuthzMockFns.mockGetActiveWorkflowContext.mockResolvedValue(null) + const res = await callPatch({ + workspaceId: 'workspace-1', + groupId: 'grp_1', + workflowId: 'wf_missing', + }) + expect(res.status).toBe(400) + expect(mockUpdateWorkflowGroup).not.toHaveBeenCalled() + }) + + it('succeeds when changing workflowId to one in the same workspace', async () => { + const res = await callPatch({ + workspaceId: 'workspace-1', + groupId: 'grp_1', + workflowId: 'wf_1', + }) + expect(res.status).toBe(200) + expect(mockUpdateWorkflowGroup).toHaveBeenCalled() + }) + + it('skips the workflow check when workflowId is not being changed', async () => { + const res = await callPatch({ + workspaceId: 'workspace-1', + groupId: 'grp_1', + name: 'Renamed group', + }) + expect(res.status).toBe(200) + expect(workflowAuthzMockFns.mockGetActiveWorkflowContext).not.toHaveBeenCalled() + expect(mockUpdateWorkflowGroup).toHaveBeenCalled() + }) +}) diff --git a/apps/sim/app/api/table/[tableId]/groups/route.ts b/apps/sim/app/api/table/[tableId]/groups/route.ts index e5c6400d3df..d3cb772287b 100644 --- a/apps/sim/app/api/table/[tableId]/groups/route.ts +++ b/apps/sim/app/api/table/[tableId]/groups/route.ts @@ -1,4 +1,5 @@ import { createLogger } from '@sim/logger' +import { getActiveWorkflowContext } from '@sim/platform-authz/workflow' import { type NextRequest, NextResponse } from 'next/server' import { addWorkflowGroupContract, @@ -29,6 +30,22 @@ interface RouteParams { * `addWorkflowGroup` / `updateWorkflowGroup` / `deleteWorkflowGroup`, so they * share this mapper instead of repeating the if-chain three times. */ +/** + * Confirms `workflowId` resolves to an active workflow in `workspaceId` before it is + * persisted onto a table's workflow group. Returns a 400 response when the workflow + * doesn't exist or belongs to a different workspace, otherwise `null`. + */ +async function validateWorkflowInWorkspace( + workflowId: string, + workspaceId: string +): Promise { + const context = await getActiveWorkflowContext(workflowId) + if (!context || context.workspaceId !== workspaceId) { + return NextResponse.json({ error: 'Invalid workflow ID' }, { status: 400 }) + } + return null +} + function mapWorkflowGroupError(error: unknown, fallbackMessage: string): NextResponse { if (error instanceof Error) { const msg = error.message @@ -65,6 +82,13 @@ export const POST = withRouteHandler(async (request: NextRequest, { params }: Ro if (result.table.workspaceId !== validated.workspaceId) { return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) } + if (validated.group.workflowId) { + const workflowError = await validateWorkflowInWorkspace( + validated.group.workflowId, + result.table.workspaceId + ) + if (workflowError) return workflowError + } const updatedTable = await addWorkflowGroup( { tableId, @@ -104,6 +128,13 @@ export const PATCH = withRouteHandler(async (request: NextRequest, { params }: R if (result.table.workspaceId !== validated.workspaceId) { return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) } + if (validated.workflowId !== undefined) { + const workflowError = await validateWorkflowInWorkspace( + validated.workflowId, + result.table.workspaceId + ) + if (workflowError) return workflowError + } const updatedTable = await updateWorkflowGroup( { tableId, From b75971fc81329465ccdb4af6f8cf900eb7c5ceb1 Mon Sep 17 00:00:00 2001 From: waleed Date: Fri, 3 Jul 2026 14:22:04 -0700 Subject: [PATCH 2/2] fix(tables): reorder JSDoc for mapWorkflowGroupError Greptile flagged the JSDoc for mapWorkflowGroupError as orphaned after validateWorkflowInWorkspace was inserted between the comment and the function it documented. Move the comment back above its function. --- apps/sim/app/api/table/[tableId]/groups/route.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/apps/sim/app/api/table/[tableId]/groups/route.ts b/apps/sim/app/api/table/[tableId]/groups/route.ts index d3cb772287b..84aecb13c0c 100644 --- a/apps/sim/app/api/table/[tableId]/groups/route.ts +++ b/apps/sim/app/api/table/[tableId]/groups/route.ts @@ -23,13 +23,6 @@ interface RouteParams { params: Promise<{ tableId: string }> } -/** - * Maps known service-layer error messages onto HTTP responses; falls through - * to a 500 with a generic message for anything unrecognized. The three - * group-route handlers all surface the same error shapes from - * `addWorkflowGroup` / `updateWorkflowGroup` / `deleteWorkflowGroup`, so they - * share this mapper instead of repeating the if-chain three times. - */ /** * Confirms `workflowId` resolves to an active workflow in `workspaceId` before it is * persisted onto a table's workflow group. Returns a 400 response when the workflow @@ -46,6 +39,13 @@ async function validateWorkflowInWorkspace( return null } +/** + * Maps known service-layer error messages onto HTTP responses; falls through + * to a 500 with a generic message for anything unrecognized. The three + * group-route handlers all surface the same error shapes from + * `addWorkflowGroup` / `updateWorkflowGroup` / `deleteWorkflowGroup`, so they + * share this mapper instead of repeating the if-chain three times. + */ function mapWorkflowGroupError(error: unknown, fallbackMessage: string): NextResponse { if (error instanceof Error) { const msg = error.message