Skip to content

Commit 17872f6

Browse files
icecrasher321claude
andcommitted
fix(tools): honor a tool's declared credential selector, and refuse its aliases first
Sixty-eight tools — Snowflake among them — declare the credential selector as a required `user-only` parameter (`oauthCredential` or `credential`) with no `oauth` block, filled by their block from an `oauth-input` field. The required-input check ran against the raw body, before the top-level `credentialId` had been placed anywhere, so a valid credential was rejected as a missing `oauthCredential`. And the alias refusal ran only over *undeclared* keys, so `input.oauthCredential` on such a tool passed the declared-key check and bypassed the top-level field — credential precedence differing per tool. The credential is still named once, at the top level. It now lands under whichever selector the tool declares (or `credential`, which the executor reads for OAuth resolution, when it declares none), and required inputs are validated against what the executor will actually receive. A declared required selector also demands `credentialId` up front, the same as an `oauth` block does. The alias refusal is unconditional and runs first. Verified live: a Snowflake call with a top-level credential passes the validator and fails downstream at resolution; `input.oauthCredential` is refused; omitting the credential names `credentialId` as required. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent f156cff commit 17872f6

2 files changed

Lines changed: 119 additions & 16 deletions

File tree

apps/sim/lib/tool-execution/application/execute-tool.test.ts

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,14 @@ const TOOL_METADATA: Record<string, Record<string, unknown>> = {
114114
},
115115
hosting: { apiKeyParam: 'apiKey' },
116116
},
117+
snowflake_execute_sql: {
118+
id: 'snowflake_execute_sql',
119+
name: 'Snowflake Execute SQL',
120+
params: {
121+
oauthCredential: { type: 'string', required: true, visibility: 'user-only' },
122+
statement: { type: 'string', required: true, visibility: 'user-or-llm' },
123+
},
124+
},
117125
thinking_tool: {
118126
id: 'thinking_tool',
119127
name: 'Thinking',
@@ -176,6 +184,7 @@ const previewBlock = block({
176184
})
177185
const zendeskBlock = block({ type: 'zendesk', tools: { access: ['zendesk_get_ticket'] } })
178186
const thinkingBlock = block({ type: 'thinking', tools: { access: ['thinking_tool'] } })
187+
const snowflakeBlock = block({ type: 'snowflake', tools: { access: ['snowflake_execute_sql'] } })
179188
const confluenceBlock = block({
180189
type: 'confluence_v2',
181190
tools: { access: ['confluence_read_v2'] },
@@ -213,6 +222,7 @@ describe('executeToolForCaller', () => {
213222
confluenceBlock,
214223
zendeskBlock,
215224
thinkingBlock,
225+
snowflakeBlock,
216226
])
217227
mocks.executeRegistryTool.mockResolvedValue({ success: true, output: { markdown: '# Hi' } })
218228
mocks.resolveBillingAttribution.mockResolvedValue({ workspaceId: WORKSPACE_ID })
@@ -391,6 +401,54 @@ describe('executeToolForCaller', () => {
391401
expect(mocks.executeRegistryTool).not.toHaveBeenCalled()
392402
})
393403

404+
/**
405+
* Sixty-eight tools declare the selector as a required `user-only` parameter
406+
* (`oauthCredential` or `credential`) with no `oauth` block — Snowflake among
407+
* them. Validating required inputs against the raw body rejected a valid
408+
* top-level `credentialId` as a missing `oauthCredential`.
409+
*/
410+
it('satisfies a declared credential selector with the top-level credentialId', async () => {
411+
await expect(
412+
run({
413+
toolId: 'snowflake_execute_sql',
414+
credentialId: 'cred-sf',
415+
input: { statement: 'select 1' },
416+
})
417+
).resolves.toMatchObject({ status: 'succeeded' })
418+
419+
const [, params] = mocks.executeRegistryTool.mock.calls[0]
420+
expect(params.oauthCredential).toBe('cred-sf')
421+
expect(params.credential).toBeUndefined()
422+
})
423+
424+
it('demands credentialId for a declared required selector even without an oauth block', async () => {
425+
await expect(
426+
run({ toolId: 'snowflake_execute_sql', input: { statement: 'select 1' } })
427+
).rejects.toMatchObject({
428+
code: 'validation',
429+
message: expect.stringContaining('credentialId is required'),
430+
})
431+
expect(mocks.executeRegistryTool).not.toHaveBeenCalled()
432+
})
433+
434+
/**
435+
* The alias check has to run before the declared-key check, or a tool that
436+
* declares `oauthCredential` lets a caller bypass the top-level field and
437+
* credential precedence starts differing per tool.
438+
*/
439+
it('refuses input.oauthCredential even where the tool declares it', async () => {
440+
await expect(
441+
run({
442+
toolId: 'snowflake_execute_sql',
443+
input: { statement: 'select 1', oauthCredential: 'cred-sf' },
444+
})
445+
).rejects.toMatchObject({
446+
code: 'validation',
447+
message: expect.stringContaining('top-level credentialId'),
448+
})
449+
expect(mocks.executeRegistryTool).not.toHaveBeenCalled()
450+
})
451+
394452
it('passes the named credential through as the tool credential', async () => {
395453
await run({ toolId: 'slack_message', input: { text: 'hi' }, credentialId: 'cred-1' })
396454

apps/sim/lib/tool-execution/application/execute-tool.ts

Lines changed: 61 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,30 @@ function hostedKeyParamFor(
6464
return tool.hosting.apiKeyParam
6565
}
6666

67+
/**
68+
* The three spellings the executor accepts for "which credential".
69+
*
70+
* Inside the executor they are interchangeable: `normalizeCopilotCredentialParams`
71+
* folds `credentialId` into `credential`, and `oauthCredential` is copied onto
72+
* `credential` before resolution. On a public contract the credential is named
73+
* once, at the top level, and mapped onto whichever of these the tool declares.
74+
*/
75+
const CREDENTIAL_SELECTORS = ['credential', 'credentialId', 'oauthCredential'] as const
76+
77+
/**
78+
* The credential-selector parameter a tool declares, if it declares one.
79+
*
80+
* Two shapes exist. A tool with an `oauth` block hides `accessToken` and lets
81+
* resolution fill it, declaring no selector at all. Sixty-eight others —
82+
* Snowflake among them — declare the selector itself as a required `user-only`
83+
* parameter (`oauthCredential` or `credential`) that their block fills from an
84+
* `oauth-input` field. Both are the same contract to a caller: a top-level
85+
* `credentialId`, placed where the tool expects it.
86+
*/
87+
function declaredCredentialSelector(tool: ExecutableToolConfig): string | undefined {
88+
return CREDENTIAL_SELECTORS.find((name) => tool.params?.[name] !== undefined)
89+
}
90+
6791
/**
6892
* Refuses an input key the tool does not declare.
6993
*
@@ -94,6 +118,22 @@ function assertNoUndeclaredInputs(
94118
): void {
95119
const params = tool.params ?? {}
96120

121+
/**
122+
* Unconditional, and first: a tool may *declare* `oauthCredential` as a
123+
* parameter, and it would otherwise pass the declared-key check below and
124+
* bypass the top-level `credentialId` — giving credential precedence that
125+
* differs from one tool to the next.
126+
*/
127+
const credentialAlias = Object.keys(args).find((key) =>
128+
(CREDENTIAL_SELECTORS as readonly string[]).includes(key)
129+
)
130+
if (credentialAlias) {
131+
throw new OrchestrationError(
132+
'validation',
133+
`input.${credentialAlias} is not accepted; pass the credential as the top-level credentialId field`
134+
)
135+
}
136+
97137
/**
98138
* Declared is not the same as accepted. A `hidden` parameter is Sim's to fill
99139
* — a resolved credential's `accessToken`, a hosted key, a block-composed
@@ -113,16 +153,6 @@ function assertNoUndeclaredInputs(
113153
const undeclared = Object.keys(args).filter((key) => !Object.hasOwn(params, key))
114154
if (undeclared.length === 0) return
115155

116-
const credentialAlias = undeclared.find((key) =>
117-
['credential', 'credentialId', 'oauthCredential'].includes(key)
118-
)
119-
if (credentialAlias) {
120-
throw new OrchestrationError(
121-
'validation',
122-
`input.${credentialAlias} is not accepted; pass the credential as the top-level credentialId field`
123-
)
124-
}
125-
126156
throw new OrchestrationError(
127157
'validation',
128158
`${toolId} does not accept ${undeclared.map((key) => `input.${key}`).join(', ')}`
@@ -234,14 +264,30 @@ export const executeToolForCaller = defineAuthorizedWorkspaceUseCase({
234264

235265
const tool = getTool(toolId)
236266
if (!tool) throw new OrchestrationError('not_found', 'Tool not found')
237-
if (tool.oauth?.required && !input.credentialId) {
267+
assertNoUndeclaredInputs(tool, toolId, input.input)
268+
269+
const selector = declaredCredentialSelector(tool)
270+
const requiresCredential =
271+
tool.oauth?.required === true || (selector !== undefined && tool.params[selector]?.required)
272+
if (requiresCredential && !input.credentialId) {
238273
throw new OrchestrationError(
239274
'validation',
240-
`credentialId is required: ${toolId} authenticates with a ${tool.oauth.provider} credential`
275+
`credentialId is required: ${toolId} authenticates with a ${tool.oauth?.provider ?? 'connected'} credential`
241276
)
242277
}
243-
assertNoUndeclaredInputs(tool, toolId, input.input)
244-
assertRequiredCallerInputsPresent(tool, toolId, input.input)
278+
279+
/**
280+
* What the executor will receive, minus `_context`. The credential lands
281+
* under the selector the tool declares, so a declared required
282+
* `oauthCredential` is satisfied by the top-level `credentialId` rather than
283+
* rejected as missing; a tool that declares none gets `credential`, which the
284+
* executor reads for OAuth resolution.
285+
*/
286+
const callerParams: Record<string, unknown> = {
287+
...input.input,
288+
...(input.credentialId ? { [selector ?? 'credential']: input.credentialId } : {}),
289+
}
290+
assertRequiredCallerInputsPresent(tool, toolId, callerParams)
245291

246292
const userId = principalUserId(principal)
247293
if (!userId) {
@@ -254,8 +300,7 @@ export const executeToolForCaller = defineAuthorizedWorkspaceUseCase({
254300
})
255301

256302
const params: Record<string, unknown> = {
257-
...input.input,
258-
...(input.credentialId ? { credential: input.credentialId } : {}),
303+
...callerParams,
259304
_context: {
260305
userId,
261306
workspaceId: context.workspaceId,

0 commit comments

Comments
 (0)