From 66fbf5f3c93506adbfd1b0c72017b7d5dd30e434 Mon Sep 17 00:00:00 2001 From: Bill Leoutsakos Date: Tue, 8 Sep 2026 16:32:22 -0700 Subject: [PATCH] fix(tools): normalize credential selectors before validation --- .../copilot/tool-executor/executor.test.ts | 2 +- .../sim/lib/copilot/tool-executor/executor.ts | 4 - apps/sim/tools/index.test.ts | 172 ++++++++++++++++++ apps/sim/tools/index.ts | 32 +++- apps/sim/tools/params.test.ts | 43 +++++ apps/sim/tools/params.ts | 19 +- 6 files changed, 257 insertions(+), 15 deletions(-) diff --git a/apps/sim/lib/copilot/tool-executor/executor.test.ts b/apps/sim/lib/copilot/tool-executor/executor.test.ts index 9c4be645f05..28d401741df 100644 --- a/apps/sim/lib/copilot/tool-executor/executor.test.ts +++ b/apps/sim/lib/copilot/tool-executor/executor.test.ts @@ -189,7 +189,6 @@ describe('copilot tool executor fallback', () => { expect.objectContaining({ maxResults: 10, credentialId: 'cred-123', - credential: 'cred-123', _context: expect.objectContaining({ userId: 'user-1', workflowId: 'workflow-1', @@ -206,6 +205,7 @@ describe('copilot tool executor fallback', () => { }), }) ) + expect(executeAppTool.mock.calls[0]?.[1]).not.toHaveProperty('credential') expect(result).toEqual({ success: true, output: { emails: [] } }) }) diff --git a/apps/sim/lib/copilot/tool-executor/executor.ts b/apps/sim/lib/copilot/tool-executor/executor.ts index a8d407d9bad..f7c580e0bf6 100644 --- a/apps/sim/lib/copilot/tool-executor/executor.ts +++ b/apps/sim/lib/copilot/tool-executor/executor.ts @@ -218,10 +218,6 @@ function buildAppToolParams( ): Record { const result = { ...params } - if (result.credentialId && !result.credential && !result.oauthCredential) { - result.credential = result.credentialId - } - result._context = { ...(typeof result._context === 'object' && result._context !== null ? (result._context as object) diff --git a/apps/sim/tools/index.test.ts b/apps/sim/tools/index.test.ts index 1cb84748caa..06a8e528da4 100644 --- a/apps/sim/tools/index.test.ts +++ b/apps/sim/tools/index.test.ts @@ -4541,6 +4541,16 @@ describe('Copilot OAuth Credential Enforcement', () => { let cleanupEnvVars: () => void beforeEach(() => { + tools.test_credential_alias = { + id: 'test_credential_alias', + name: 'Credential Alias Test', + description: 'A synthetic stored-credential tool', + version: '1.0.0', + params: { + oauthCredential: { type: 'string', required: true, visibility: 'user-or-llm' }, + }, + operation: { input: (params) => ({ oauthCredential: params.oauthCredential }) }, + } satisfies InternalToolConfig process.env.NEXT_PUBLIC_APP_URL = 'http://localhost:3000' /* * getInternalApiBaseUrl prefers INTERNAL_API_BASE_URL over the app URL, so @@ -4556,10 +4566,172 @@ describe('Copilot OAuth Credential Enforcement', () => { }) afterEach(() => { + Reflect.deleteProperty(tools, 'test_credential_alias') vi.resetAllMocks() cleanupEnvVars() }) + it.each([ + { params: { credentialId: ' alias-id ' }, expected: 'alias-id' }, + { params: { credential: 'generic-id' }, expected: 'generic-id' }, + { params: { credential: 'generic-id', credentialId: 'alias-id' }, expected: 'generic-id' }, + { + params: { + oauthCredential: 'explicit-id', + credential: 'generic-id', + credentialId: 'alias-id', + }, + expected: 'explicit-id', + }, + ])( + 'normalizes credential selectors before validation: $expected', + async ({ params, expected }) => { + mockResolveExecutorCredentialToken.mockResolvedValue({ + accessToken: 'resolved-token', + credentialType: 'service_account', + }) + for (const copilotToolExecution of [false, true]) { + mockExecuteInternalToolOperation.mockClear() + const result = await executeTool('test_credential_alias', params, { + executionContext: createToolExecutionContext({ + workspaceId: 'workspace-456', + userId: 'user-1', + copilotToolExecution, + }), + }) + expect(result.success, result.error).toBe(true) + expect(mockExecuteInternalToolOperation.mock.calls[0]?.[0].input).toEqual({ + oauthCredential: expected, + }) + } + } + ) + + it.each(['', ' ', null, false, 123])( + 'does not replace an invalid explicit selector: %s', + async (oauthCredential) => { + mockExecuteInternalToolOperation.mockClear() + mockResolveExecutorCredentialToken.mockClear() + const result = await executeTool('test_credential_alias', { + oauthCredential, + credentialId: 'another-credential', + }) + + expect(result).toMatchObject({ + success: false, + error: 'Credential selection must be a nonempty string', + }) + expect(mockExecuteInternalToolOperation).not.toHaveBeenCalled() + expect(mockResolveExecutorCredentialToken).not.toHaveBeenCalled() + } + ) + + it.each(['credential', 'credentialId'])( + 'keeps declared %s authoritative over other aliases', + async (selector) => { + tools.test_credential_alias.params = { + [selector]: { type: 'string', required: true, visibility: 'user-or-llm' }, + } + mockResolveExecutorCredentialToken.mockResolvedValue({ accessToken: 'resolved-token' }) + const result = await executeTool( + 'test_credential_alias', + { + credential: 'declared-id', + oauthCredential: 'other-id', + credentialId: 'alias-id', + [selector]: 'declared-id', + }, + { + executionContext: createToolExecutionContext({ + workspaceId: 'workspace-456', + userId: 'user-1', + }), + } + ) + + expect(result.success, result.error).toBe(true) + expect(mockResolveExecutorCredentialToken).toHaveBeenCalledWith( + expect.objectContaining({ + credentialId: 'declared-id', + }) + ) + } + ) + + it('does not bypass authorization when an alias identifies an unavailable credential', async () => { + mockResolveExecutorCredentialToken.mockRejectedValueOnce(new Error('Credential unavailable')) + mockExecuteInternalToolOperation.mockClear() + const result = await executeTool( + 'test_credential_alias', + { credentialId: 'unavailable-id' }, + { + executionContext: createToolExecutionContext({ + workspaceId: 'workspace-456', + userId: 'user-1', + }), + } + ) + + expect(result).toMatchObject({ success: false, error: 'Credential unavailable' }) + expect(mockExecuteInternalToolOperation).not.toHaveBeenCalled() + }) + + it.each(['oauthCredential', 'credential', 'credentialId'])( + 'uses resolved %s references instead of stale compatibility aliases', + async (selector) => { + tools.test_credential_alias.params = { + [selector]: { type: 'string', required: true, visibility: 'user-only' }, + } + mockGetEffectiveDecryptedEnv.mockResolvedValue({ TEST_CREDENTIAL: 'resolved-id' }) + mockResolveExecutorCredentialToken.mockResolvedValue({ accessToken: 'resolved-token' }) + const result = await executeTool( + 'test_credential_alias', + { + oauthCredential: 'other-id', + credential: 'other-id', + credentialId: 'other-id', + [selector]: '{{TEST_CREDENTIAL}}', + }, + { + executionContext: createToolExecutionContext({ + workspaceId: 'workspace-456', + userId: 'user-1', + copilotToolExecution: true, + }), + } + ) + + expect(result.success, result.error).toBe(true) + expect(mockGetEffectiveDecryptedEnv).toHaveBeenCalledWith('user-1', 'workspace-456') + expect(mockResolveExecutorCredentialToken).toHaveBeenCalledWith( + expect.objectContaining({ + credentialId: 'resolved-id', + }) + ) + } + ) + + it('resolves alias-only OAuth calls through the authorized credential path', async () => { + mockResolveExecutorCredentialToken.mockResolvedValueOnce({ accessToken: 'resolved-token' }) + const context = createToolExecutionContext({ + workspaceId: 'workspace-456', + copilotToolExecution: true, + }) + const result = await executeTool( + 'gmail_read', + { credentialId: 'alias-id' }, + { executionContext: context } + ) + + expect(result.success).toBe(true) + expect(mockResolveExecutorCredentialToken).toHaveBeenCalledWith( + expect.objectContaining({ + credentialId: 'alias-id', + toolId: 'gmail_read', + }) + ) + }) + it('fails fast when copilot executes an oauth tool without an explicit credential selector', async () => { const fetchMock = vi.fn() global.fetch = Object.assign(fetchMock, { preconnect: vi.fn() }) as typeof fetch diff --git a/apps/sim/tools/index.ts b/apps/sim/tools/index.ts index 6ae970f13ad..1b4818784fc 100644 --- a/apps/sim/tools/index.ts +++ b/apps/sim/tools/index.ts @@ -511,11 +511,26 @@ function readExplicitCredentialSelector(params: Record): string return undefined } -function normalizeCopilotCredentialParams(params: Record): void { - const credentialId = typeof params.credentialId === 'string' ? params.credentialId.trim() : '' - if (credentialId && !params.credential && !params.oauthCredential) { - params.credential = credentialId +function normalizeCopilotCredentialParams( + tool: ToolDefinition, + params: Record +): string | undefined { + const aliases = ['oauthCredential', 'credential', 'credentialId'] + const selector = aliases.find( + (name) => tool.params[name] !== undefined && tool.params[name].visibility !== 'hidden' + ) + const selectedKey = + selector && params[selector] !== undefined + ? selector + : aliases.find((name) => params[name] !== undefined) + if (!selectedKey) return + + const selected = params[selectedKey] + if (typeof selected !== 'string' || selected.trim().length === 0) { + throw new Error('Credential selection must be a nonempty string') } + params[selector ?? 'credential'] = selectedKey === 'credentialId' ? selected.trim() : selected + return selector ?? 'credential' } function enforceCopilotCredentialSelection( @@ -1856,6 +1871,10 @@ async function executeToolImplementation( } } + const credentialSelector = tool + ? normalizeCopilotCredentialParams(tool, contextParams) + : undefined + // Validate the tool and its parameters validateRequiredParametersAfterMerge(toolId, tool, contextParams) @@ -1865,7 +1884,6 @@ async function executeToolImplementation( } await normalizeFileParams(tool, contextParams, scope, executionContext) - normalizeCopilotCredentialParams(contextParams) enforceCopilotCredentialSelection(toolId, tool, contextParams, scope) await resolveToolEnvReferences(tool, contextParams, scope, resolvedSecretTraceRegistry) @@ -1886,8 +1904,8 @@ async function executeToolImplementation( } // If we have a credential parameter, fetch the access token - if (contextParams.oauthCredential) { - contextParams.credential = contextParams.oauthCredential + if (credentialSelector) { + contextParams.credential = contextParams[credentialSelector] } if (operationContext?.requestMode === 'assistant' && tool.personalToken) { if (typeof window !== 'undefined' || !operationContext.workspaceId) { diff --git a/apps/sim/tools/params.test.ts b/apps/sim/tools/params.test.ts index d271d413139..bee028ed4ac 100644 --- a/apps/sim/tools/params.test.ts +++ b/apps/sim/tools/params.test.ts @@ -436,6 +436,49 @@ describe('Tool Parameters Utils', () => { expect(copilotSchema.required).toContain('credentialId') }) + it.each(['oauthCredential', 'credential', 'credentialId'])( + 'publishes one Copilot credential field for declared %s selectors', + (selector) => { + for (const oauth of [undefined, { required: true, provider: 'test-provider' }]) { + const tool = { + ...mockToolConfig, + ...(oauth ? { oauth } : {}), + params: { + [selector]: { type: 'string', required: true, visibility: 'user-only' as const }, + }, + } + const schema = createUserToolSchema(tool, { surface: 'copilot' }) + expect(Object.keys(schema.properties)).toEqual(['credentialId']) + expect(schema.required).toEqual(['credentialId']) + expect(schema.properties.credentialId.description).not.toContain('{{VAR_NAME}}') + const defaultSchema = createUserToolSchema(tool) + expect(Object.keys(defaultSchema.properties)).toEqual([selector]) + expect(defaultSchema.required).toEqual([selector]) + } + } + ) + + it('keeps optional credential selectors optional and hidden authority unpublished', () => { + const schema = createUserToolSchema( + { + ...mockToolConfig, + params: { oauthCredential: { type: 'string', required: false, visibility: 'user-only' } }, + }, + { surface: 'copilot' } + ) + expect(Object.keys(schema.properties)).toEqual(['credentialId']) + expect(schema.required).toEqual([]) + const hiddenSchema = createUserToolSchema( + { + ...mockToolConfig, + params: { credentialId: { type: 'string', required: true, visibility: 'hidden' } }, + }, + { surface: 'copilot' } + ) + expect(hiddenSchema.properties).toEqual({}) + expect(hiddenSchema.required).toEqual([]) + }) + it.concurrent('emits file params as reference strings by default', () => { const toolWithFileParams = { ...mockToolConfig, diff --git a/apps/sim/tools/params.ts b/apps/sim/tools/params.ts index 7666075e19b..caa6c1eb0a3 100644 --- a/apps/sim/tools/params.ts +++ b/apps/sim/tools/params.ts @@ -272,6 +272,13 @@ export function createUserToolSchema( options: UserToolSchemaOptions = {} ): ToolSchema { const surface = options.surface ?? 'default' + const credentialSelectors = ['oauthCredential', 'credential', 'credentialId'] + const declaredCredentialSelectors = credentialSelectors.filter( + (name) => + toolConfig.params[name] !== undefined && toolConfig.params[name].visibility !== 'hidden' + ) + const useCredentialId = + surface === 'copilot' && (toolConfig.oauth?.required || declaredCredentialSelectors.length > 0) const hostedApiKeyParam = options.hostedKeySupport && toolConfig.hosting && !toolConfig.hosting.enabled ? toolConfig.hosting.apiKeyParam @@ -284,6 +291,7 @@ export function createUserToolSchema( for (const [paramId, param] of Object.entries(toolConfig.params)) { if (!param) continue + if (useCredentialId && credentialSelectors.includes(paramId)) continue const visibility = param.visibility ?? 'user-or-llm' if (visibility === 'hidden') { continue @@ -315,13 +323,18 @@ export function createUserToolSchema( } } - if (toolConfig.oauth?.required && surface === 'copilot') { + if (useCredentialId) { schema.properties.credentialId = { type: 'string', description: - 'Credential ID to use for this OAuth tool call. Required for Copilot/Superagent execution. Get valid IDs from environment/credentials.json.', + 'Credential ID to use for this connected tool call. Get valid IDs from environment/credentials.json.', + } + if ( + toolConfig.oauth?.required || + declaredCredentialSelectors.some((name) => toolConfig.params[name]?.required) + ) { + schema.required.push('credentialId') } - schema.required.push('credentialId') } return schema