Fix Mass Assignment in Execution Update Endpoint

tonghuaroot · tonghuaroot · commit 12cc14da2c03 · 2026-05-21T08:23:27.000+08:00
`PUT /api/v1/executions/:id` previously accepted the raw request body
and applied it to the matched Execution row via
`Object.assign(new Execution(), req.body)` followed by
`repository.merge(existing, new)` and `save()`, with no allowlist of
writable fields. A workspace user could overwrite any column on an
execution they owned, including:

- `isPublic`: flipping this to `true` exposes the row through the
  unauthenticated `GET /api/v1/public-executions/:id` route (whitelisted
  in `utils/constants.ts::WHITELIST_URLS`), which leaks the full LLM /
  tool transcript stored in `executionData`. `_removeCredentialId` only
  strips `credentialId` fields; the rest of the transcript is returned
  verbatim.
- `workspaceId`: reparents the execution into another workspace.
- `agentflowId`: re-points the row at a chatflow the attacker does not
  own, confusing the `leftJoinAndSelect('execution.agentflow', ...)` join
  in `getAllExecutions`.
- `executionData`, `state`, `action`, `stoppedDate`: lets an attacker
  rewrite the audit trail of their own executions.

The UI only ever calls this endpoint to toggle `isPublic`
(`packages/ui/src/views/agentexecutions/ExecutionDetails.jsx`,
`packages/ui/src/views/agentexecutions/ShareExecutionDialog.jsx`,
`packages/observe/src/infrastructure/api/executions.ts`), so the
writable surface is narrowed to a single explicit field. Internal
callers that need to mutate `executionData`, `state`, `stoppedDate`,
etc. use the dedicated `updateExecution` helper in
`utils/buildAgentflow.ts`, which does not pass through this service
function and is therefore unaffected.

Adds a co-located regression test that exercises the legitimate
`isPublic` toggle and verifies that `workspaceId`, `agentflowId`,
`state`, `action`, `executionData`, `stoppedDate`, and `id` cannot
be mass-assigned through this service.

Refs: GHSA-v2cc-6h2j-w847
Signed-off-by: tonghuaroot &lt;tonghuaroot@gmail.com&gt;
diff --git a/packages/server/src/services/executions/index.test.ts b/packages/server/src/services/executions/index.test.ts
@@ -0,0 +1,240 @@
+import { StatusCodes } from 'http-status-codes'
+import { InternalFlowiseError } from '../../errors/internalFlowiseError'
+
+// typeorm is not directly resolvable under pnpm strict hoisting; provide a
+// virtual mock so entity decorators are no-ops in this test context.
+jest.mock(
+    'typeorm',
+    () => {
+        const noop = () => (_target: any, _key?: any) => {}
+        return {
+            Entity: () => noop(),
+            Column: () => noop(),
+            PrimaryGeneratedColumn: () => noop(),
+            CreateDateColumn: () => noop(),
+            UpdateDateColumn: () => noop(),
+            ManyToOne: () => noop(),
+            OneToMany: () => noop(),
+            OneToOne: () => noop(),
+            JoinColumn: () => noop(),
+            Index: () => noop(),
+            Unique: () => noop(),
+            In: (vals: any) => ({ _type: 'in', _value: vals })
+        }
+    },
+    { virtual: true }
+)
+
+// ── Mocks ───────────────────────────────────────────────────────────────────
+
+const mockSave = jest.fn()
+const mockFindOneBy = jest.fn()
+const mockMerge = jest.fn()
+const mockFindOne = jest.fn()
+const mockDelete = jest.fn()
+const mockUpdate = jest.fn()
+const mockGetManyAndCount = jest.fn()
+const mockQueryBuilder = {
+    leftJoinAndSelect: jest.fn().mockReturnThis(),
+    orderBy: jest.fn().mockReturnThis(),
+    skip: jest.fn().mockReturnThis(),
+    take: jest.fn().mockReturnThis(),
+    where: jest.fn().mockReturnThis(),
+    andWhere: jest.fn().mockReturnThis(),
+    getManyAndCount: mockGetManyAndCount
+}
+const mockGetRepository = jest.fn().mockReturnValue({
+    save: mockSave,
+    findOneBy: mockFindOneBy,
+    findOne: mockFindOne,
+    merge: mockMerge,
+    delete: mockDelete,
+    update: mockUpdate,
+    createQueryBuilder: jest.fn().mockReturnValue(mockQueryBuilder)
+})
+
+jest.mock('../../utils/getRunningExpressApp', () => ({
+    getRunningExpressApp: jest.fn(() => ({
+        AppDataSource: { getRepository: mockGetRepository }
+    }))
+}))
+
+jest.mock('../../utils', () => ({
+    _removeCredentialId: jest.fn((data: any) => data)
+}))
+
+// Import after the virtual mock and module mocks are registered
+import executionsService from './index'
+
+// ── Helpers ─────────────────────────────────────────────────────────────────
+
+const ATTACKER_WS = '40b8ad7b-e687-4e23-8148-ade5bb4805a8'
+const VICTIM_WS = '11111111-2222-3333-4444-555555555555'
+const EXEC_ID = 'bbbbbbbb-cccc-dddd-eeee-333333333333'
+const VICTIM_AGENTFLOW_ID = 'cccccccc-dddd-eeee-ffff-444444444444'
+const ORIGINAL_AGENTFLOW_ID = 'aaaaaaaa-bbbb-cccc-dddd-111111111111'
+
+const makeExecution = (overrides: Record<string, any> = {}) => ({
+    id: EXEC_ID,
+    executionData: '[{"nodeId":"llmAgent_0","data":{"output":{"content":"original transcript"}}}]',
+    state: 'FINISHED',
+    agentflowId: ORIGINAL_AGENTFLOW_ID,
+    sessionId: 'sess-1',
+    action: null,
+    isPublic: false,
+    createdDate: new Date('2026-01-01'),
+    updatedDate: new Date('2026-01-01'),
+    stoppedDate: new Date('2026-01-01'),
+    workspaceId: ATTACKER_WS,
+    ...overrides
+})
+
+describe('executionsService.updateExecution', () => {
+    beforeEach(() => {
+        jest.clearAllMocks()
+        // Default: merge applies properties from second arg onto first arg (TypeORM semantics)
+        mockMerge.mockImplementation((target: any, source: any) => {
+            Object.assign(target, source)
+            return target
+        })
+    })
+
+    it('persists isPublic when toggled by an owning workspace user', async () => {
+        const stored = makeExecution()
+        mockFindOneBy.mockResolvedValue(stored)
+        mockSave.mockImplementation(async (e: any) => e)
+
+        const result: any = await executionsService.updateExecution(EXEC_ID, { isPublic: true } as any, ATTACKER_WS)
+
+        expect(mockFindOneBy).toHaveBeenCalledWith({ id: EXEC_ID, workspaceId: ATTACKER_WS })
+        expect(result.isPublic).toBe(true)
+        expect(mockSave).toHaveBeenCalled()
+    })
+
+    // Regression — GHSA-v2cc-6h2j-w847
+    //
+    // The pre-patch service accepted the raw request body and ran
+    // `Object.assign(new Execution(), req.body)` followed by
+    // `merge + save`, with no allowlist. A workspace user could overwrite
+    // any column on an execution they owned, including columns that gate
+    // the unauthenticated `GET /api/v1/public-executions/:id` endpoint
+    // (`isPublic`) and the cross-workspace ownership key (`workspaceId`).
+    describe('mass assignment regression (GHSA-v2cc-6h2j-w847)', () => {
+        it('ignores attacker-supplied workspaceId in the request body', async () => {
+            const stored = makeExecution()
+            mockFindOneBy.mockResolvedValue(stored)
+            mockSave.mockImplementation(async (e: any) => e)
+
+            const result: any = await executionsService.updateExecution(
+                EXEC_ID,
+                { isPublic: true, workspaceId: VICTIM_WS } as any,
+                ATTACKER_WS
+            )
+
+            expect(result.workspaceId).toBe(ATTACKER_WS)
+            // Confirm the merged entity that was saved did not pick up the victim workspace id
+            const savedEntity = mockSave.mock.calls[0][0]
+            expect(savedEntity.workspaceId).toBe(ATTACKER_WS)
+            expect(savedEntity.workspaceId).not.toBe(VICTIM_WS)
+        })
+
+        it('ignores attacker-supplied agentflowId in the request body', async () => {
+            const stored = makeExecution()
+            mockFindOneBy.mockResolvedValue(stored)
+            mockSave.mockImplementation(async (e: any) => e)
+
+            const result: any = await executionsService.updateExecution(EXEC_ID, { agentflowId: VICTIM_AGENTFLOW_ID } as any, ATTACKER_WS)
+
+            expect(result.agentflowId).toBe(ORIGINAL_AGENTFLOW_ID)
+            expect(result.agentflowId).not.toBe(VICTIM_AGENTFLOW_ID)
+        })
+
+        it('ignores attacker-supplied state, action, executionData and stoppedDate', async () => {
+            const stored = makeExecution()
+            mockFindOneBy.mockResolvedValue(stored)
+            mockSave.mockImplementation(async (e: any) => e)
+
+            const result: any = await executionsService.updateExecution(
+                EXEC_ID,
+                {
+                    state: 'ERROR',
+                    action: 'rewritten-by-attacker',
+                    executionData: '[{"nodeId":"noop","output":"sponsored-content-injected"}]',
+                    stoppedDate: new Date('2099-01-01T00:00:00.000Z')
+                } as any,
+                ATTACKER_WS
+            )
+
+            expect(result.state).toBe('FINISHED')
+            expect(result.action).toBeNull()
+            expect(result.executionData).toBe('[{"nodeId":"llmAgent_0","data":{"output":{"content":"original transcript"}}}]')
+            expect(result.stoppedDate).toEqual(new Date('2026-01-01'))
+        })
+
+        it('ignores attacker-supplied id in the request body', async () => {
+            const stored = makeExecution()
+            mockFindOneBy.mockResolvedValue(stored)
+            mockSave.mockImplementation(async (e: any) => e)
+
+            const result: any = await executionsService.updateExecution(
+                EXEC_ID,
+                { id: '99999999-9999-9999-9999-999999999999' } as any,
+                ATTACKER_WS
+            )
+
+            expect(result.id).toBe(EXEC_ID)
+        })
+
+        it('still allows the legitimate isPublic toggle when mixed with malicious fields', async () => {
+            const stored = makeExecution()
+            mockFindOneBy.mockResolvedValue(stored)
+            mockSave.mockImplementation(async (e: any) => e)
+
+            const result: any = await executionsService.updateExecution(
+                EXEC_ID,
+                {
+                    isPublic: true,
+                    workspaceId: VICTIM_WS,
+                    agentflowId: VICTIM_AGENTFLOW_ID,
+                    state: 'ERROR'
+                } as any,
+                ATTACKER_WS
+            )
+
+            expect(result.isPublic).toBe(true)
+            expect(result.workspaceId).toBe(ATTACKER_WS)
+            expect(result.agentflowId).toBe(ORIGINAL_AGENTFLOW_ID)
+            expect(result.state).toBe('FINISHED')
+        })
+    })
+
+    it('throws NOT_FOUND when no execution matches the scoped query', async () => {
+        mockFindOneBy.mockResolvedValue(null)
+
+        await expect(executionsService.updateExecution(EXEC_ID, { isPublic: true } as any, ATTACKER_WS)).rejects.toThrow(
+            InternalFlowiseError
+        )
+        await expect(executionsService.updateExecution(EXEC_ID, { isPublic: true } as any, ATTACKER_WS)).rejects.toMatchObject({
+            statusCode: StatusCodes.INTERNAL_SERVER_ERROR
+        })
+        expect(mockSave).not.toHaveBeenCalled()
+    })
+
+    it('scopes the lookup with workspaceId when one is supplied', async () => {
+        mockFindOneBy.mockResolvedValue(makeExecution())
+        mockSave.mockImplementation(async (e: any) => e)
+
+        await executionsService.updateExecution(EXEC_ID, { isPublic: true } as any, ATTACKER_WS)
+
+        expect(mockFindOneBy).toHaveBeenCalledWith({ id: EXEC_ID, workspaceId: ATTACKER_WS })
+    })
+
+    it('omits workspaceId from the lookup when none is supplied', async () => {
+        mockFindOneBy.mockResolvedValue(makeExecution())
+        mockSave.mockImplementation(async (e: any) => e)
+
+        await executionsService.updateExecution(EXEC_ID, { isPublic: true } as any)
+
+        expect(mockFindOneBy).toHaveBeenCalledWith({ id: EXEC_ID })
+    })
+})
diff --git a/packages/server/src/services/executions/index.ts b/packages/server/src/services/executions/index.ts
@@ -105,6 +105,21 @@ const getAllExecutions = async (filters: ExecutionFilters = {}): Promise<{ data:
     }
 }
 
+/**
+ * Fields that the HTTP `PUT /api/v1/executions/:id` endpoint is allowed to
+ * change on an Execution row. The endpoint is only used by the UI to toggle
+ * an execution's share state (see `packages/ui/src/views/agentexecutions`),
+ * so the writable surface is intentionally limited to `isPublic`.
+ *
+ * Every other column (`id`, `executionData`, `state`, `agentflowId`,
+ * `sessionId`, `action`, `createdDate`, `updatedDate`, `stoppedDate`,
+ * `workspaceId`) is server-managed and must not be settable through this
+ * route. Internal callers that need to mutate those columns use the
+ * dedicated `updateExecution` helper in `utils/buildAgentflow.ts`, which
+ * does not pass through this service function.
+ */
+const EXECUTION_UPDATABLE_FIELDS = ['isPublic'] as const
+
 const updateExecution = async (executionId: string, data: Partial<Execution>, workspaceId?: string): Promise<Execution | null> => {
     try {
         const appServer = getRunningExpressApp()
@@ -117,8 +132,20 @@ const updateExecution = async (executionId: string, data: Partial<Execution>, wo
         if (!execution) {
             throw new InternalFlowiseError(StatusCodes.NOT_FOUND, `Execution ${executionId} not found`)
         }
+
+        // Build a sanitized partial that only carries fields in the
+        // explicit allowlist; prevents mass assignment of server-managed
+        // columns such as `workspaceId`, `agentflowId`, `executionData`,
+        // `state`, and so on via the request body.
+        const sanitized: Partial<Execution> = {}
+        for (const field of EXECUTION_UPDATABLE_FIELDS) {
+            if (data && Object.prototype.hasOwnProperty.call(data, field)) {
+                ;(sanitized as any)[field] = (data as any)[field]
+            }
+        }
+
         const updateExecution = new Execution()
-        Object.assign(updateExecution, data)
+        Object.assign(updateExecution, sanitized)
         await appServer.AppDataSource.getRepository(Execution).merge(execution, updateExecution)
         const dbResponse = await appServer.AppDataSource.getRepository(Execution).save(execution)
         return dbResponse
