Skip to content

Commit 6da9081

Browse files
committed
fix(bedrock): address review on saved AWS credential signing
- Translate resolves the saved credential only for a Bedrock model, so a leftover credential choice no longer blocks a call to another provider. - A transient STS failure resolving the credential reports 503 instead of 400, using the classification the AWS tool operations already share. - llm_chat forwards the run's execution ID to the resolver, naming the role session and the credential-access audit after the run. - Native conversation binding keys a saved-credential request on the credential alone, ignoring inline keys left on the block. - Handler tests share one central mock for the Bedrock credential module; the block declaration test is replaced by Translate behavior tests. - The integration suite writes its credential-access audit inside a rollup scope and asserts it before teardown.
1 parent cc48a4f commit 6da9081

16 files changed

Lines changed: 212 additions & 89 deletions

‎apps/sim/blocks/bedrock-saved-credential.test.ts‎

Lines changed: 0 additions & 45 deletions
This file was deleted.
Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
import { describe, expect, it } from 'vitest'
2+
import { TranslateBlock } from '@/blocks/blocks/translate'
3+
4+
const toolParams = (params: Record<string, unknown>) => {
5+
const build = TranslateBlock.tools.config?.params
6+
if (!build) throw new Error('TranslateBlock.tools.config.params is missing')
7+
return build({ context: 'Hello', targetLanguage: 'French', ...params })
8+
}
9+
10+
describe('Translate Bedrock credential', () => {
11+
it('sends the saved AWS credential with a Bedrock model call', () => {
12+
expect(
13+
toolParams({
14+
model: 'bedrock/my-inference-profile',
15+
bedrockAuthMode: 'credential',
16+
bedrockCredential: 'aws-cred',
17+
})
18+
).toMatchObject({ model: 'bedrock/my-inference-profile', bedrockCredential: 'aws-cred' })
19+
})
20+
21+
it('refuses a Bedrock model call set to a saved AWS credential with none selected', () => {
22+
expect(() =>
23+
toolParams({
24+
model: 'bedrock/my-inference-profile',
25+
bedrockAuthMode: 'credential',
26+
bedrockCredential: '',
27+
})
28+
).toThrow('Select an AWS credential for Bedrock')
29+
})
30+
31+
it('ignores a leftover Bedrock credential choice once the model is not on Bedrock', () => {
32+
const params = toolParams({ model: 'gpt-4o', bedrockAuthMode: 'credential' })
33+
34+
expect(params).toMatchObject({ model: 'gpt-4o' })
35+
expect(params.bedrockCredential).toBeUndefined()
36+
})
37+
})

‎apps/sim/blocks/blocks/translate.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
getSelectedBedrockCredential,
88
PROVIDER_CREDENTIAL_INPUTS,
99
} from '@/blocks/utils'
10+
import { findProviderFromModel } from '@/providers/models'
1011

1112
const getTranslationPrompt = (targetLanguage: string) =>
1213
`Translate the following text into ${targetLanguage || 'English'}. Output ONLY the translated text with no additional commentary, explanations, or notes.`
@@ -82,7 +83,11 @@ export const TranslateBlock: BlockConfig = {
8283
bedrockAccessKeyId: params.bedrockAccessKeyId,
8384
bedrockSecretKey: params.bedrockSecretKey,
8485
bedrockRegion: params.bedrockRegion,
85-
bedrockCredential: getSelectedBedrockCredential(params),
86+
bedrockCredential:
87+
typeof params.model === 'string' &&
88+
findProviderFromModel(params.model.trim()) === 'bedrock'
89+
? getSelectedBedrockCredential(params)
90+
: undefined,
8691
}),
8792
},
8893
},

‎apps/sim/executor/handlers/agent/handler.test.ts‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@ import {
66
schemaMock,
77
setEnvFlags,
88
} from '@sim/testing'
9+
import {
10+
llmBedrockCredentialMock,
11+
llmBedrockCredentialMockFns,
12+
} from '@sim/testing/mocks/llm-bedrock-credential.mock'
913
import { getMockLogger } from '@sim/testing/mocks/logger.mock'
1014
import {
1115
permissionCheckMock,
@@ -61,6 +65,8 @@ mockGetBlock.mockReturnValue(undefined)
6165
const mockImportWorkspaceFileSecretProvenanceForModelView =
6266
workspaceFileSecretProvenanceMockFns.mockImportWorkspaceFileSecretProvenanceForModelView
6367
const mockValidateModelProvider = permissionCheckMockFns.mockValidateModelProvider
68+
const mockResolveBedrockCredentialSession =
69+
llmBedrockCredentialMockFns.mockResolveBedrockCredentialSession
6470
mockImportWorkspaceFileSecretProvenanceForModelView.mockResolvedValue(true)
6571
providersUtilsMockFns.mockGetProviderFromModel.mockReturnValue('mock-provider')
6672
providersUtilsMockFns.mockIsDeepResearchModel.mockImplementation((model: string) =>
@@ -75,13 +81,7 @@ const { mockDiscoverMcpServerToolsAsExecutor, mockOpenAgentTurnSession } = vi.ho
7581
mockOpenAgentTurnSession: vi.fn().mockResolvedValue(undefined),
7682
}))
7783

78-
const { mockResolveBedrockCredentialSession } = vi.hoisted(() => ({
79-
mockResolveBedrockCredentialSession: vi.fn(),
80-
}))
81-
82-
vi.mock('@/lib/internal/llm/bedrock-credential', () => ({
83-
resolveBedrockCredentialSession: mockResolveBedrockCredentialSession,
84-
}))
84+
vi.mock('@/lib/internal/llm/bedrock-credential', () => llmBedrockCredentialMock)
8585

8686
vi.mock('@/lib/memory/agent-turn-session', () => ({
8787
openAgentTurnSession: mockOpenAgentTurnSession,

‎apps/sim/executor/handlers/evaluator/handler.test.ts‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,10 @@ import {
22
credentialsAccessMock,
33
credentialsAccessMockFns,
44
} from '@sim/testing/mocks/credentials-access.mock'
5+
import {
6+
llmBedrockCredentialMock,
7+
llmBedrockCredentialMockFns,
8+
} from '@sim/testing/mocks/llm-bedrock-credential.mock'
59
import { permissionsMock, permissionsMockFns } from '@sim/testing/mocks/permissions.mock'
610
import '@sim/testing/mocks/executor'
711

@@ -12,13 +16,7 @@ const { mockResolveAutoModel } = vi.hoisted(() => ({
1216
mockResolveAutoModel: vi.fn(),
1317
}))
1418

15-
const { mockResolveBedrockCredentialSession } = vi.hoisted(() => ({
16-
mockResolveBedrockCredentialSession: vi.fn(),
17-
}))
18-
19-
vi.mock('@/lib/internal/llm/bedrock-credential', () => ({
20-
resolveBedrockCredentialSession: mockResolveBedrockCredentialSession,
21-
}))
19+
vi.mock('@/lib/internal/llm/bedrock-credential', () => llmBedrockCredentialMock)
2220

2321
vi.mock('@/lib/workspaces/permissions/utils', () => permissionsMock)
2422

@@ -60,6 +58,8 @@ credentialsAccessMockFns.mockGetCredentialActorContext.mockResolvedValue({
6058
})
6159

6260
const mockCheckWorkspaceAccess = permissionsMockFns.mockCheckWorkspaceAccess
61+
const mockResolveBedrockCredentialSession =
62+
llmBedrockCredentialMockFns.mockResolveBedrockCredentialSession
6363

6464
const mockGetProviderFromModel = getProviderFromModel as Mock
6565
const mockExecuteProviderRequest = executeProviderRequest as Mock

‎apps/sim/executor/handlers/router/handler.test.ts‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,10 @@ import {
22
credentialsAccessMock,
33
credentialsAccessMockFns,
44
} from '@sim/testing/mocks/credentials-access.mock'
5+
import {
6+
llmBedrockCredentialMock,
7+
llmBedrockCredentialMockFns,
8+
} from '@sim/testing/mocks/llm-bedrock-credential.mock'
59
import { permissionsMock, permissionsMockFns } from '@sim/testing/mocks/permissions.mock'
610
import '@sim/testing/mocks/executor'
711

@@ -18,13 +22,7 @@ const { mockResolveAutoModel } = vi.hoisted(() => ({
1822
mockResolveAutoModel: vi.fn(),
1923
}))
2024

21-
const { mockResolveBedrockCredentialSession } = vi.hoisted(() => ({
22-
mockResolveBedrockCredentialSession: vi.fn(),
23-
}))
24-
25-
vi.mock('@/lib/internal/llm/bedrock-credential', () => ({
26-
resolveBedrockCredentialSession: mockResolveBedrockCredentialSession,
27-
}))
25+
vi.mock('@/lib/internal/llm/bedrock-credential', () => llmBedrockCredentialMock)
2826

2927
vi.mock('@/lib/workspaces/permissions/utils', () => permissionsMock)
3028

@@ -67,6 +65,8 @@ credentialsAccessMockFns.mockGetCredentialActorContext.mockResolvedValue({
6765
})
6866

6967
const mockCheckWorkspaceAccess = permissionsMockFns.mockCheckWorkspaceAccess
68+
const mockResolveBedrockCredentialSession =
69+
llmBedrockCredentialMockFns.mockResolveBedrockCredentialSession
7070

7171
const mockGenerateRouterPrompt = generateRouterPrompt as Mock
7272
const mockGenerateRouterV2Prompt = generateRouterV2Prompt as Mock

‎apps/sim/lib/internal/aws/credentials.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ const THROTTLING_ERROR_NAMES = new Set(['Throttling', 'ThrottlingException'])
120120
* (IDPCommunicationError, when STS could not reach Sim's OIDC issuer), throttling,
121121
* an STS 5xx, or a network failure. The SDK has already retried these.
122122
*/
123-
function isTransientAwsFailure(error: unknown): boolean {
123+
export function isTransientAwsFailure(error: unknown): boolean {
124124
if (!isRecordLike(error)) return false
125125
const status = isRecordLike(error.$metadata) ? error.$metadata.httpStatusCode : undefined
126126
return (

‎apps/sim/lib/internal/llm/bedrock-credential.integration.ts‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
/** A saved AWS credential resolved for a Bedrock model call against real credential rows. */
22

3+
import { withAuditRollupScope } from '@sim/audit'
34
import { db } from '@sim/db'
45
import {
56
auditLog,
@@ -11,7 +12,7 @@ import {
1112
} from '@sim/db/schema'
1213
import { deleteWorkspaceFixture, insertWorkspaceFixture } from '@sim/db/testing/workspace-fixtures'
1314
import { generateId } from '@sim/utils/id'
14-
import { inArray } from 'drizzle-orm'
15+
import { and, eq, inArray } from 'drizzle-orm'
1516
import { afterAll, beforeAll, describe, expect, it } from 'vitest'
1617
import { encryptSecret } from '@/lib/core/security/encryption'
1718
import {
@@ -155,11 +156,19 @@ describe('resolveBedrockCredentialSession', () => {
155156
})
156157

157158
it("resolves a credential member's saved access keys for the credential's workspace", async () => {
158-
await expect(resolveAs(memberId, awsCredentialId)).resolves.toEqual({
159-
credentialId: awsCredentialId,
160-
accessKeyId: ACCESS_KEY_ID,
161-
secretAccessKey: SECRET_ACCESS_KEY,
162-
})
159+
await expect(withAuditRollupScope(() => resolveAs(memberId, awsCredentialId))).resolves.toEqual(
160+
{
161+
credentialId: awsCredentialId,
162+
accessKeyId: ACCESS_KEY_ID,
163+
secretAccessKey: SECRET_ACCESS_KEY,
164+
}
165+
)
166+
167+
const audits = await db
168+
.select({ actorId: auditLog.actorId })
169+
.from(auditLog)
170+
.where(and(eq(auditLog.workspaceId, workspaceId), eq(auditLog.resourceId, awsCredentialId)))
171+
expect(audits).toEqual([{ actorId: memberId }])
163172
})
164173

165174
it('refuses a run with no acting user', async () => {

‎apps/sim/lib/internal/llm/bedrock-credential.ts‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,26 @@
11
import { getErrorMessage } from '@sim/utils/errors'
22
import { authorizeCredentialUseForAuth } from '@/lib/auth/credential-access'
33
import { AuthType } from '@/lib/auth/hybrid'
4-
import { resolveAuthorizedAwsCredential } from '@/lib/internal/aws/credentials'
4+
import {
5+
isTransientAwsFailure,
6+
resolveAuthorizedAwsCredential,
7+
} from '@/lib/internal/aws/credentials'
58
import { recordCredentialAccess } from '@/lib/oauth/token-resolution'
69
import { AWS_SERVICE_ACCOUNT_PROVIDER_ID } from '@/lib/oauth/types'
710
import type { BedrockCredentialSession } from '@/providers/types'
811

12+
type BedrockCredentialFailure = 'unauthorized' | 'unusable' | 'unavailable'
13+
914
/**
1015
* A saved AWS credential could not be used for a Bedrock request: the caller is
11-
* not allowed to use it (`unauthorized`), or it is not a live AWS credential or
12-
* AWS refused it (`unusable`).
16+
* not allowed to use it (`unauthorized`), it is not a live AWS credential or AWS
17+
* refused it (`unusable`), or AWS was briefly unreachable while resolving it
18+
* (`unavailable`, worth retrying).
1319
*/
1420
export class BedrockCredentialError extends Error {
15-
readonly reason: 'unauthorized' | 'unusable'
21+
readonly reason: BedrockCredentialFailure
1622

17-
constructor(
18-
message: string,
19-
options?: { reason?: 'unauthorized' | 'unusable'; cause?: unknown }
20-
) {
23+
constructor(message: string, options?: { reason?: BedrockCredentialFailure; cause?: unknown }) {
2124
super(message, { cause: options?.cause })
2225
this.name = 'BedrockCredentialError'
2326
this.reason = options?.reason ?? 'unusable'
@@ -94,7 +97,7 @@ export async function resolveBedrockCredentialSession({
9497
signal?.throwIfAborted()
9598
throw new BedrockCredentialError(
9699
`Could not use the AWS credential: ${getErrorMessage(error, 'Unknown error')}`,
97-
{ cause: error }
100+
{ reason: isTransientAwsFailure(error) ? 'unavailable' : 'unusable', cause: error }
98101
)
99102
}
100103

‎apps/sim/lib/internal/llm/execute-tool.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ export const executeLlmTool: InternalToolOperationHandler = async (request) => {
5454
actorUserId: request.context.userId,
5555
headers: request.headers,
5656
requestId: request.requestId,
57+
executionId: request.context.executionId,
5758
signal: request.signal,
5859
})
5960
request.signal?.throwIfAborted()

0 commit comments

Comments
 (0)