From 90a5e6f039bb47a9744fb40f8e17c6bb0cbfc434 Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Thu, 25 Jun 2026 15:34:28 -0700 Subject: [PATCH] fix(copilot): strip platform-managed apiKey on hosted-tool blocks in edit_workflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit preValidateCredentialInputs only stripped apiKey for hosted LLM models (getHostedModels). Agent-authored apiKey on hosted-tool blocks (Fal video/image, etc.) slipped through, disabling hosted-key injection and misleading the agent into telling users to bring a key. Generalize the strip to key off tool.hosting — the same canonical signal injectHostedKeyIfNeeded uses at execution. Resolve the block's active tool via its tools.config.tool selector, honor the per-provider enabled gate, and strip the exact hosting.apiKeyParam field (no hardcoded 'apiKey' assumption). Surfaces the existing non-fatal note to the agent. Self-hosted and non-hosted providers untouched. --- .../workflow/edit-workflow/validation.test.ts | 261 +++++++++++++++++- .../workflow/edit-workflow/validation.ts | 144 +++++++++- 2 files changed, 385 insertions(+), 20 deletions(-) diff --git a/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/validation.test.ts b/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/validation.test.ts index 9c929988030..ccaedf1f9f1 100644 --- a/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/validation.test.ts +++ b/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/validation.test.ts @@ -2,13 +2,17 @@ * @vitest-environment node */ import { envFlagsMock } from '@sim/testing' -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { normalizeConditionRouterIds } from './builders' -const { mockValidateSelectorIds, mockGetModelOptions } = vi.hoisted(() => ({ - mockValidateSelectorIds: vi.fn(), - mockGetModelOptions: vi.fn(() => []), -})) +const { mockValidateSelectorIds, mockGetModelOptions, mockEnvFlags, mockGetTool } = vi.hoisted( + () => ({ + mockValidateSelectorIds: vi.fn(), + mockGetModelOptions: vi.fn(() => []), + mockEnvFlags: { isHosted: false }, + mockGetTool: vi.fn(), + }) +) const conditionBlockConfig = { type: 'condition', @@ -65,6 +69,53 @@ const canonicalCredBlockConfig = { ], } +// Mirrors video_generator_v3: routes provider -> tool; only video_falai has hosting. +const videoBlockConfig = { + type: 'video_generator_v3', + name: 'Video Generator', + outputs: {}, + subBlocks: [{ id: 'provider', type: 'dropdown' }], + tools: { + access: ['video_runway', 'video_falai'], + config: { + tool: (params: Record) => + params.provider === 'falai' ? 'video_falai' : 'video_runway', + }, + }, +} + +// A hosted block whose tool's managed key param is NOT named 'apiKey'. +const customKeyBlockConfig = { + type: 'custom_key_block', + name: 'Custom Key Block', + outputs: {}, + subBlocks: [{ id: 'serviceKey', type: 'short-input' }], + tools: { access: ['custom_key_tool'], config: { tool: () => 'custom_key_tool' } }, +} + +// Single tool with a per-provider `enabled` gate (mirrors image_generate, falai-only hosting). +const imageBlockConfig = { + type: 'image_generator_v2', + name: 'Image Generator', + outputs: {}, + subBlocks: [{ id: 'provider', type: 'dropdown' }], + tools: { access: ['image_generate'], config: { tool: () => 'image_generate' } }, +} + +// Tool registry stand-in for the hosted-tool tests. +const toolsByIdMock: Record = { + video_falai: { id: 'video_falai', hosting: { apiKeyParam: 'apiKey' } }, + video_runway: { id: 'video_runway' }, + custom_key_tool: { id: 'custom_key_tool', hosting: { apiKeyParam: 'serviceKey' } }, + image_generate: { + id: 'image_generate', + hosting: { + apiKeyParam: 'apiKey', + enabled: (p: Record) => p.provider === 'falai', + }, + }, +} + vi.mock('@/blocks/registry', () => ({ getBlock: (type: string) => type === 'condition' @@ -81,18 +132,33 @@ vi.mock('@/blocks/registry', () => ({ ? knowledgeBlockConfig : type === 'canonicalcred' ? canonicalCredBlockConfig - : undefined, + : type === 'video_generator_v3' + ? videoBlockConfig + : type === 'custom_key_block' + ? customKeyBlockConfig + : type === 'image_generator_v2' + ? imageBlockConfig + : undefined, })) vi.mock('@/blocks/utils', () => ({ getModelOptions: mockGetModelOptions, })) +vi.mock('@/tools/utils', () => ({ + getTool: mockGetTool, +})) + vi.mock('@/lib/copilot/validation/selector-validator', () => ({ validateSelectorIds: mockValidateSelectorIds, })) -vi.mock('@/lib/core/config/env-flags', () => envFlagsMock) +vi.mock('@/lib/core/config/env-flags', () => ({ + ...envFlagsMock, + get isHosted() { + return mockEnvFlags.isHosted + }, +})) vi.mock('@/providers/utils', () => ({ getHostedModels: () => [], @@ -304,6 +370,187 @@ describe('preValidateCredentialInputs', () => { }) }) +describe('preValidateCredentialInputs (hosted-tool blocks)', () => { + beforeEach(() => { + vi.clearAllMocks() + mockValidateSelectorIds.mockResolvedValue({ valid: [], invalid: [] }) + mockGetTool.mockImplementation((id: string) => toolsByIdMock[id]) + mockEnvFlags.isHosted = true + }) + + afterEach(() => { + mockEnvFlags.isHosted = false + }) + + const ctx = { userId: 'user-1', workspaceId: 'workspace-1' } + + it('strips apiKey when the block resolves to a hosted tool on hosted Sim', async () => { + const operations = [ + { + operation_type: 'add' as const, + block_id: 'video-1', + params: { + type: 'video_generator_v3', + inputs: { provider: 'falai', model: 'veo-3.1', apiKey: '{{FAL_API_KEY}}' }, + }, + }, + ] + + const result = await preValidateCredentialInputs(operations, ctx) + + expect(result.filteredOperations[0]?.params?.inputs?.apiKey).toBeUndefined() + expect(result.errors).toHaveLength(1) + expect(result.errors[0]).toMatchObject({ blockId: 'video-1', field: 'apiKey' }) + expect(result.errors[0]?.error).toContain('managed by Sim') + }) + + it('preserves apiKey when the resolved tool has no hosting (non-falai provider)', async () => { + const operations = [ + { + operation_type: 'add' as const, + block_id: 'video-1', + params: { + type: 'video_generator_v3', + inputs: { provider: 'runway', apiKey: 'user-runway-key' }, + }, + }, + ] + + const result = await preValidateCredentialInputs(operations, ctx) + + expect(result.filteredOperations[0]?.params?.inputs?.apiKey).toBe('user-runway-key') + expect(result.errors).toHaveLength(0) + }) + + it('resolves provider from existing block state for edit ops that only set apiKey', async () => { + const operations = [ + { + operation_type: 'edit' as const, + block_id: 'video-1', + params: { + type: 'video_generator_v3', + inputs: { apiKey: '{{FAL_API_KEY}}' }, + }, + }, + ] + const workflowState = { + blocks: { + 'video-1': { + type: 'video_generator_v3', + subBlocks: { provider: { value: 'falai' } }, + }, + }, + } + + const result = await preValidateCredentialInputs(operations, ctx, workflowState) + + expect(result.filteredOperations[0]?.params?.inputs?.apiKey).toBeUndefined() + expect(result.errors).toHaveLength(1) + }) + + it('strips apiKey on a hosted-tool block nested inside a loop', async () => { + const operations = [ + { + operation_type: 'add' as const, + block_id: 'loop-1', + params: { + type: 'loop', + inputs: {}, + nestedNodes: { + 'video-child': { + type: 'video_generator_v3', + inputs: { provider: 'falai', model: 'veo-3.1', apiKey: '{{FAL_API_KEY}}' }, + }, + }, + }, + }, + ] + + const result = await preValidateCredentialInputs(operations, ctx) + + const nested = result.filteredOperations[0]?.params?.nestedNodes as + | Record }> + | undefined + expect(nested?.['video-child']?.inputs?.apiKey).toBeUndefined() + expect(result.errors).toHaveLength(1) + expect(result.errors[0]).toMatchObject({ blockId: 'video-child', field: 'apiKey' }) + }) + + it("strips a hosted tool's key field even when it is not named apiKey", async () => { + const operations = [ + { + operation_type: 'add' as const, + block_id: 'custom-1', + params: { + type: 'custom_key_block', + inputs: { serviceKey: '{{SOME_SERVICE_KEY}}' }, + }, + }, + ] + + const result = await preValidateCredentialInputs(operations, ctx) + + expect(result.filteredOperations[0]?.params?.inputs?.serviceKey).toBeUndefined() + expect(result.errors).toHaveLength(1) + expect(result.errors[0]).toMatchObject({ blockId: 'custom-1', field: 'serviceKey' }) + }) + + it('preserves apiKey on self-hosted deployments (isHosted false)', async () => { + mockEnvFlags.isHosted = false + const operations = [ + { + operation_type: 'add' as const, + block_id: 'video-1', + params: { + type: 'video_generator_v3', + inputs: { provider: 'falai', model: 'veo-3.1', apiKey: '{{FAL_API_KEY}}' }, + }, + }, + ] + + const result = await preValidateCredentialInputs(operations, ctx) + + expect(result.filteredOperations[0]?.params?.inputs?.apiKey).toBe('{{FAL_API_KEY}}') + expect(result.errors).toHaveLength(0) + }) + + it('strips apiKey when the tool hosting enabled gate passes (image, falai)', async () => { + const operations = [ + { + operation_type: 'add' as const, + block_id: 'image-1', + params: { + type: 'image_generator_v2', + inputs: { provider: 'falai', apiKey: '{{FAL_API_KEY}}' }, + }, + }, + ] + + const result = await preValidateCredentialInputs(operations, ctx) + + expect(result.filteredOperations[0]?.params?.inputs?.apiKey).toBeUndefined() + expect(result.errors).toHaveLength(1) + }) + + it('preserves apiKey when the tool hosting enabled gate fails (image, non-falai)', async () => { + const operations = [ + { + operation_type: 'add' as const, + block_id: 'image-1', + params: { + type: 'image_generator_v2', + inputs: { provider: 'openai', apiKey: 'user-openai-key' }, + }, + }, + ] + + const result = await preValidateCredentialInputs(operations, ctx) + + expect(result.filteredOperations[0]?.params?.inputs?.apiKey).toBe('user-openai-key') + expect(result.errors).toHaveLength(0) + }) +}) + const CTX = { userId: 'user-1', workspaceId: 'workspace-1' } describe('validateWorkflowSelectorIds (credential inclusion)', () => { diff --git a/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/validation.ts b/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/validation.ts index 58d04785611..710b2afd657 100644 --- a/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/validation.ts +++ b/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/validation.ts @@ -14,6 +14,7 @@ import type { SubBlockConfig } from '@/blocks/types' import { getModelOptions } from '@/blocks/utils' import { EDGE, normalizeName } from '@/executor/constants' import { isKnownModelId, suggestModelIdsForUnknownModel } from '@/providers/models' +import { getTool } from '@/tools/utils' import { TRIGGER_RUNTIME_SUBBLOCK_IDS } from '@/triggers/constants' import type { EdgeHandleValidationResult, @@ -940,7 +941,10 @@ export async function collectUnresolvedReferences( /** * Pre-validates credential and apiKey inputs in operations before they are applied. * - Validates oauth-input (credential) IDs are accessible to the user in the workflow workspace - * - Filters out apiKey inputs for hosted models when isHosted is true + * - Filters out apiKey inputs when isHosted is true and the key is platform-managed: either a + * hosted LLM model (model in getHostedModels) or a block whose active tool declares + * `hosting` (e.g. Fal-backed video/image generators) - the canonical signal also used by + * injectHostedKeyIfNeeded at execution * - Also validates credentials and apiKeys in nestedNodes (blocks inside loop/parallel) * Returns validation errors for any removed inputs. */ @@ -969,7 +973,9 @@ export async function preValidateCredentialInputs( operationIndex: number blockId: string blockType: string - model: string + fieldName: string + reason: 'hosted_model' | 'hosted_tool' + model?: string nestedBlockId?: string }> = [] @@ -1024,12 +1030,80 @@ export async function preValidateCredentialInputs( operationIndex: opIndex, blockId, blockType, + fieldName: 'apiKey', + reason: 'hosted_model', model: modelValue, nestedBlockId, }) } } + /** + * Collect inputs targeting a hosted tool's key param. `tool.hosting` is the canonical + * "Sim provides this key" signal — the same one injectHostedKeyIfNeeded uses at execution. It + * names the managed field (`apiKeyParam`) and gates per-provider (`enabled`). We resolve the + * tool the block's current inputs select (via the block's `tools.config.tool` selector), so + * multi-provider blocks (video routing falai -> video_falai) and per-provider gates + * (image_generate, falai-only) match execution exactly. The UI hides these fields, but the + * copilot can still author them, so strip them here. + */ + function collectHostedToolApiKeyInput( + blockConfig: ReturnType, + inputs: Record, + toolParams: Record, + opIndex: number, + blockId: string, + blockType: string, + nestedBlockId?: string + ) { + if (!isHosted || !blockConfig?.tools) return + + // Resolve which tool(s) the current inputs select. With a selector there is exactly one active + // tool; without one, every accessible tool is a candidate. + let candidateToolIds: string[] + const toolSelector = blockConfig.tools.config?.tool + if (toolSelector) { + try { + candidateToolIds = [toolSelector(toolParams)] + } catch { + return + } + } else { + candidateToolIds = blockConfig.tools.access ?? [] + } + + const managedFieldIds = new Set() + for (const toolId of candidateToolIds) { + const tool = getTool(toolId) + if (!tool?.hosting) continue + if (tool.hosting.enabled && !tool.hosting.enabled(toolParams)) continue + managedFieldIds.add(tool.hosting.apiKeyParam) + } + + for (const fieldId of managedFieldIds) { + const value = inputs[fieldId] + if (typeof value !== 'string' || value.trim() === '') continue + + const alreadyCollected = hostedApiKeyInputs.some( + (e) => + e.operationIndex === opIndex && + e.blockId === blockId && + e.nestedBlockId === nestedBlockId && + e.fieldName === fieldId + ) + if (alreadyCollected) continue + + hostedApiKeyInputs.push({ + operationIndex: opIndex, + blockId, + blockType, + fieldName: fieldId, + reason: 'hosted_tool', + nestedBlockId, + }) + } + } + operations.forEach((op, opIndex) => { // Process main block inputs if (op.params?.inputs && op.params?.type) { @@ -1071,6 +1145,31 @@ export async function preValidateCredentialInputs( op.block_id, op.params.type ) + + // The active tool depends on inputs (e.g. provider). On edit ops that don't restate + // provider, merge the existing block's subblock values so the tool selector and its + // `enabled` gate resolve correctly. + let toolParams = op.params.inputs as Record + if (op.operation_type === 'edit' && workflowState) { + const existingBlock = (workflowState.blocks as Record)?.[op.block_id] as + | Record + | undefined + const existingSubBlocks = existingBlock?.subBlocks as + | Record + | undefined + if (existingSubBlocks) { + toolParams = { ...buildSubBlockValues(existingSubBlocks), ...toolParams } + } + } + + collectHostedToolApiKeyInput( + blockConfig, + op.params.inputs as Record, + toolParams, + opIndex, + op.block_id, + op.params.type + ) } } @@ -1100,6 +1199,15 @@ export async function preValidateCredentialInputs( // Check for apiKey inputs on hosted models in nested block const modelValue = childInputs.model as string | undefined collectHostedApiKeyInput(childInputs, modelValue, opIndex, op.block_id, childType, childId) + collectHostedToolApiKeyInput( + childBlockConfig, + childInputs, + childInputs, + opIndex, + op.block_id, + childType, + childId + ) }) } }) @@ -1116,10 +1224,16 @@ export async function preValidateCredentialInputs( // Filter out apiKey inputs for hosted models and add validation errors if (hasHostedApiKeysToFilter) { - logger.info('Filtering apiKey inputs for hosted models', { count: hostedApiKeyInputs.length }) + logger.info('Filtering platform-managed apiKey inputs', { count: hostedApiKeyInputs.length }) + + const stripMessage = (input: (typeof hostedApiKeyInputs)[number]): string => + input.reason === 'hosted_tool' + ? `Cannot set "${input.fieldName}" for "${input.blockType}" - it is managed by Sim on the hosted platform. Leave "${input.fieldName}" unset.` + : `Cannot set API key for hosted model "${input.model}" - API keys are managed by the platform when using hosted models` for (const apiKeyInput of hostedApiKeyInputs) { const op = filteredOperations[apiKeyInput.operationIndex] + const field = apiKeyInput.fieldName // Handle nested block apiKey filtering if (apiKeyInput.nestedBlockId) { @@ -1128,36 +1242,40 @@ export async function preValidateCredentialInputs( | undefined const nestedBlock = nestedNodes?.[apiKeyInput.nestedBlockId] const nestedInputs = nestedBlock?.inputs as Record | undefined - if (nestedInputs?.apiKey) { - nestedInputs.apiKey = undefined - logger.debug('Filtered apiKey for hosted model in nested block', { + if (nestedInputs?.[field]) { + nestedInputs[field] = undefined + logger.debug('Filtered platform-managed apiKey in nested block', { parentBlockId: apiKeyInput.blockId, nestedBlockId: apiKeyInput.nestedBlockId, + field, + reason: apiKeyInput.reason, model: apiKeyInput.model, }) errors.push({ blockId: apiKeyInput.nestedBlockId, blockType: apiKeyInput.blockType, - field: 'apiKey', + field, value: '[redacted]', - error: `Cannot set API key for hosted model "${apiKeyInput.model}" - API keys are managed by the platform when using hosted models`, + error: stripMessage(apiKeyInput), }) } - } else if (op.params?.inputs?.apiKey) { + } else if (op.params?.inputs?.[field]) { // Handle main block apiKey filtering - op.params.inputs.apiKey = undefined - logger.debug('Filtered apiKey for hosted model', { + op.params.inputs[field] = undefined + logger.debug('Filtered platform-managed apiKey', { blockId: apiKeyInput.blockId, + field, + reason: apiKeyInput.reason, model: apiKeyInput.model, }) errors.push({ blockId: apiKeyInput.blockId, blockType: apiKeyInput.blockType, - field: 'apiKey', + field, value: '[redacted]', - error: `Cannot set API key for hosted model "${apiKeyInput.model}" - API keys are managed by the platform when using hosted models`, + error: stripMessage(apiKeyInput), }) } }