Skip to content

Commit b77a8d4

Browse files
Bill LeoutsakosBill Leoutsakos
authored andcommitted
fix(selectors): revalidate resolved inputs and preserve safe errors
1 parent 26012eb commit b77a8d4

13 files changed

Lines changed: 513 additions & 49 deletions

File tree

.agents/skills/add-selector/SKILL.md

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -77,10 +77,14 @@ non-selector callers, keep the route as a thin caller of that primitive. If it i
7777
move the logic and remove the obsolete route and contract. Never import a route handler or make an
7878
internal HTTP request from an attachment.
7979

80-
The attachment must return normalized selector results only. It must not return provider payloads,
81-
resolved context, credential IDs, tokens, or secrets. Let the shared executor own scope
82-
authorization, exact-reference resolution, credential authorization, error projection, output
83-
sanitization, and abort propagation.
80+
The attachment must return normalized selector results only. It must never deliberately or
81+
wholesale echo selector context, and hidden/server-only resolved material, credential IDs, tokens,
82+
and authentication secrets must never cross the response boundary. Browser-known literals and
83+
viewable personal/shared values are not automatically server-only secrets, but they may appear in
84+
an option only when the adapter intentionally projects them as provider resource metadata. Let the
85+
shared executor own scope authorization, exact-reference resolution, credential authorization,
86+
error projection, and output sanitization; adapters must pass and preserve the executor's abort
87+
signal during provider work.
8488

8589
## Wire the UI declaration
8690

@@ -97,6 +101,10 @@ Do not add:
97101

98102
All server selectors use the shared POST contract and React Query facade. Query identities must stay
99103
opaque and must not include context values, references, credential IDs, secrets, or their hashes.
104+
Selector code must not add context, token, or result caches. The sole existing cache exception is
105+
authorized client-credential resolution after authorization and provider binding: it may reuse the
106+
credential service's TTL-governed, lazily pruned process-local token cache. This exception requires
107+
explicit security-owner acceptance; do not broaden it or describe it as hard-bounded.
100108

101109
## Focused validation
102110

.agents/skills/validate-selector/SKILL.md

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,18 @@ Review its destination classification:
5757
network safety.
5858

5959
Missing and inaccessible references, and missing, unauthorized, or provider-mismatched credentials,
60-
must not become existence oracles. Resolved plaintext, credentials, tokens, and raw upstream errors
61-
must not enter responses, query/cache/rate keys, logs, audit metadata, redirects, or error messages.
62-
Only normalized `{ id, label, meta? }` options and bounded cursors may cross the boundary.
60+
must not become existence oracles. Hidden/server-only resolved plaintext, credentials, authentication
61+
material, and raw upstream errors must not enter responses, query/cache/rate keys, selector result
62+
caches, logs, audit metadata, redirects, or error messages. Browser-known literals and viewable
63+
personal/shared values are not automatically protected plaintext, but the executor must still never
64+
deliberately or wholesale echo selector context. Only intentionally projected, normalized
65+
`{ id, label, meta? }` options and bounded cursors may cross the boundary.
66+
67+
Selector code must not introduce a context, token, or result cache. The sole existing cache
68+
exception is authorized client-credential resolution after authorization and provider binding,
69+
which may reuse the credential service's TTL-governed, lazily pruned process-local token cache. The
70+
cache is not hard-bounded, the exception requires explicit security-owner acceptance, and selector
71+
work must not expand it.
6372

6473
## Validate provider reuse and browser boundaries
6574

apps/sim/lib/api/contracts/selectors/execute.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { z } from 'zod'
22
import { defineRouteContract } from '@/lib/api/contracts'
3-
import { workflowIdSchema, workspaceIdSchema } from '@/lib/api/contracts/primitives'
3+
import { MAX_ID_LENGTH, workflowIdSchema, workspaceIdSchema } from '@/lib/api/contracts/primitives'
44
import { type SelectorKey, selectorManifest } from '@/lib/selectors/manifest'
55
import { selectorContextKeys } from '@/lib/selectors/types'
66

@@ -16,7 +16,7 @@ export const selectorScopeSchema = z.discriminatedUnion('kind', [
1616
z
1717
.object({
1818
kind: z.literal('workflow'),
19-
workflowId: workflowIdSchema,
19+
workflowId: workflowIdSchema.max(MAX_ID_LENGTH, 'Workflow ID is too long'),
2020
workspaceId: workspaceIdSchema.optional(),
2121
})
2222
.strict(),

apps/sim/lib/selectors/application/execute-selector.test.ts

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ vi.mock('@/lib/selectors/server/sanitize', () => ({
6161
sanitizeSelectorResult: mocks.sanitize,
6262
}))
6363

64+
import { selectorScopeSchema } from '@/lib/api/contracts/selectors/execute'
6465
import { executeSelector } from '@/lib/selectors/application/execute-selector'
6566
import { getSelectorManifestEntry } from '@/lib/selectors/manifest'
6667
import {
@@ -256,6 +257,76 @@ describe('executeSelector', () => {
256257
expect(mocks.executeAttachment).not.toHaveBeenCalled()
257258
})
258259

260+
it('rejects oversized resolved context before credentials, destinations, or providers', async () => {
261+
const prepare = vi.fn(async () => ({ baseUrl: 'https://example.com' }))
262+
mocks.getAttachment.mockReturnValueOnce({
263+
destination: { kind: 'credential-bound', prepare },
264+
credential: { kind: 'stored', field: 'oauthCredential', serviceIds: ['gmail'] },
265+
execute: mocks.executeAttachment,
266+
})
267+
mocks.resolveReferences.mockImplementationOnce(async () => {
268+
mocks.events.push('reference-resolution')
269+
return {
270+
context: { oauthCredential: 'x'.repeat(16_385) },
271+
request: { kind: 'list' },
272+
references: new Map(),
273+
}
274+
})
275+
276+
await expect(execute()).rejects.toEqual(new SelectorContextUnavailableError())
277+
278+
expect(mocks.authorizeCredential).not.toHaveBeenCalled()
279+
expect(prepare).not.toHaveBeenCalled()
280+
expect(mocks.executeAttachment).not.toHaveBeenCalled()
281+
expect(mocks.logger.warn).not.toHaveBeenCalled()
282+
})
283+
284+
it.each([
285+
['empty', ''],
286+
['oversized', 'x'.repeat(16_385)],
287+
])(
288+
'rejects %s resolved detail ids before credentials, destinations, or providers',
289+
async (_case, resolvedId) => {
290+
const prepare = vi.fn(async () => ({ baseUrl: 'https://example.com' }))
291+
mocks.resolveScope.mockImplementationOnce(async () => {
292+
mocks.events.push('canonical-scope')
293+
return {
294+
workspaceId: 'workspace-1',
295+
workspaceOrganizationId: null,
296+
allowPersonalApiKeys: true,
297+
selectorKey: 'google.drive',
298+
selectorManifest: getSelectorManifestEntry('google.drive'),
299+
selectorScope: scope,
300+
}
301+
})
302+
mocks.getAttachment.mockReturnValueOnce({
303+
destination: { kind: 'credential-bound', prepare },
304+
credential: { kind: 'stored', field: 'oauthCredential', serviceIds: ['google-drive'] },
305+
execute: mocks.executeAttachment,
306+
})
307+
mocks.resolveReferences.mockImplementationOnce(async () => {
308+
mocks.events.push('reference-resolution')
309+
return {
310+
context: { oauthCredential: 'credential-1' },
311+
request: { kind: 'detail', id: resolvedId },
312+
references: new Map(),
313+
}
314+
})
315+
316+
await expect(
317+
execute({
318+
selectorKey: 'google.drive',
319+
request: { kind: 'detail', id: '{{GOOGLE_FILE_ID}}' },
320+
})
321+
).rejects.toEqual(new SelectorContextUnavailableError())
322+
323+
expect(mocks.authorizeCredential).not.toHaveBeenCalled()
324+
expect(prepare).not.toHaveBeenCalled()
325+
expect(mocks.executeAttachment).not.toHaveBeenCalled()
326+
expect(mocks.logger.warn).not.toHaveBeenCalled()
327+
}
328+
)
329+
259330
it('projects provider failures to a safe error and never logs request context', async () => {
260331
mocks.executeAttachment.mockRejectedValueOnce(
261332
new Error('upstream leaked selector-secret-canary for {{GMAIL_CREDENTIAL_ID}}')
@@ -359,3 +430,14 @@ describe('executeSelector', () => {
359430
})
360431
})
361432
})
433+
434+
describe('selector scope contract', () => {
435+
it('rejects workflow ids longer than 128 characters', () => {
436+
expect(
437+
selectorScopeSchema.safeParse({ kind: 'workflow', workflowId: 'w'.repeat(128) }).success
438+
).toBe(true)
439+
expect(
440+
selectorScopeSchema.safeParse({ kind: 'workflow', workflowId: 'w'.repeat(129) }).success
441+
).toBe(false)
442+
})
443+
})

apps/sim/lib/selectors/application/execute-selector.ts

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
import { createLogger } from '@sim/logger'
2-
import type { ExecuteSelectorRequest } from '@/lib/api/contracts/selectors/execute'
2+
import {
3+
type ExecuteSelectorRequest,
4+
selectorContextSchema,
5+
selectorRequestSchema,
6+
} from '@/lib/api/contracts/selectors/execute'
37
import { defineAuthorizedWorkspaceUseCase } from '@/lib/core/application'
48
import { type CredentialAuditRequest, recordCredentialAccess } from '@/lib/oauth/token-resolution'
59
import { selectorOperations } from '@/lib/selectors/application/operations'
@@ -129,14 +133,22 @@ async function executeAuthorizedSelector(args: {
129133
workspaceId: args.context.workspaceId,
130134
protectedValues,
131135
})
132-
if (!isSelectorReady(args.input.selectorKey, resolved.context)) {
136+
const parsedContext = selectorContextSchema.safeParse(resolved.context)
137+
const parsedRequest = selectorRequestSchema.safeParse(resolved.request)
138+
if (!parsedContext.success || !parsedRequest.success) {
139+
throw new SelectorContextUnavailableError()
140+
}
141+
const resolvedContext = parsedContext.data
142+
const resolvedRequest = parsedRequest.data
143+
144+
if (!isSelectorReady(args.input.selectorKey, resolvedContext)) {
133145
throw new SelectorContextUnavailableError()
134146
}
135147

136148
const credential = attachment.credential
137149
? await authorizeSelectorCredential({
138150
principal: args.principal,
139-
context: resolved.context,
151+
context: resolvedContext,
140152
scope: args.input.scope,
141153
workspaceId: args.context.workspaceId,
142154
policy: attachment.credential,
@@ -166,8 +178,8 @@ async function executeAuthorizedSelector(args: {
166178

167179
const selectorArgs = {
168180
selectorKey: args.input.selectorKey as ServerSelectorKey,
169-
context: resolved.context,
170-
request: resolved.request,
181+
context: resolvedContext,
182+
request: resolvedRequest,
171183
scope: args.input.scope,
172184
workspaceId: args.context.workspaceId,
173185
principal: args.principal,
@@ -196,7 +208,7 @@ async function executeAuthorizedSelector(args: {
196208
}
197209
const referencedDetailResolvedId = getReferencedDetailResolvedId({
198210
originalRequest: args.input.request,
199-
resolvedRequest: resolved.request,
211+
resolvedRequest,
200212
references: resolved.references,
201213
})
202214
const sanitizedProviderResult = sanitizeSelectorResult(
@@ -208,7 +220,7 @@ async function executeAuthorizedSelector(args: {
208220
)
209221
const result = restoreReferencedDetailValues({
210222
originalRequest: args.input.request,
211-
resolvedRequest: resolved.request,
223+
resolvedRequest,
212224
result: sanitizedProviderResult,
213225
references: resolved.references,
214226
})
Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { CloudWatchLogsServiceException } from '@aws-sdk/client-cloudwatch-logs'
5+
import { beforeEach, describe, expect, it, vi } from 'vitest'
6+
7+
const { mockListCloudWatchLogGroups, mockListCloudWatchLogStreams } = vi.hoisted(() => ({
8+
mockListCloudWatchLogGroups: vi.fn(),
9+
mockListCloudWatchLogStreams: vi.fn(),
10+
}))
11+
12+
vi.mock('@/tools/cloudwatch/listing', () => ({
13+
listCloudWatchLogGroups: mockListCloudWatchLogGroups,
14+
listCloudWatchLogStreams: mockListCloudWatchLogStreams,
15+
}))
16+
17+
import { createSelectorProtectedValues } from '@/lib/selectors/server/protected-values'
18+
import { cloudWatchSelectorAttachments } from '@/lib/selectors/server/providers/cloudwatch'
19+
import type { ExecuteServerSelectorArgs } from '@/lib/selectors/server/types'
20+
21+
function logGroupArgs(signal?: AbortSignal): ExecuteServerSelectorArgs {
22+
return {
23+
selectorKey: 'cloudwatch.logGroups',
24+
context: {
25+
awsAccessKeyId: 'access-key',
26+
awsSecretAccessKey: 'secret-key',
27+
awsRegion: 'us-east-1',
28+
},
29+
request: { kind: 'list' },
30+
scope: { kind: 'workspace', workspaceId: 'workspace-1' },
31+
workspaceId: 'workspace-1',
32+
principal: { kind: 'session', userId: 'user-1', sessionId: 'session-1' },
33+
requesterUserId: 'user-1',
34+
references: new Map(),
35+
protectedValues: createSelectorProtectedValues(),
36+
signal,
37+
}
38+
}
39+
40+
function cloudWatchError(status: number): CloudWatchLogsServiceException {
41+
return new CloudWatchLogsServiceException({
42+
name: 'CloudWatchLogsError',
43+
$fault: status >= 500 ? 'server' : 'client',
44+
$metadata: { httpStatusCode: status },
45+
})
46+
}
47+
48+
describe('CloudWatch server selector adapter errors', () => {
49+
beforeEach(() => vi.clearAllMocks())
50+
51+
it.each([
52+
[401, 'SelectorConnectionUnavailableError', 401],
53+
[403, 'SelectorConnectionUnavailableError', 403],
54+
[429, 'SelectorOptionsUnavailableError', 429],
55+
[500, 'SelectorOptionsUnavailableError', 502],
56+
] as const)(
57+
'maps trusted AWS status %i to the safe selector taxonomy',
58+
async (status, name, safeStatus) => {
59+
mockListCloudWatchLogGroups.mockRejectedValueOnce(cloudWatchError(status))
60+
61+
await expect(
62+
cloudWatchSelectorAttachments['cloudwatch.logGroups'].execute(logGroupArgs())
63+
).rejects.toMatchObject({ name, status: safeStatus })
64+
}
65+
)
66+
67+
it('does not trust a status-shaped unknown error', async () => {
68+
mockListCloudWatchLogGroups.mockRejectedValueOnce({ $metadata: { httpStatusCode: 401 } })
69+
70+
await expect(
71+
cloudWatchSelectorAttachments['cloudwatch.logGroups'].execute(logGroupArgs())
72+
).rejects.toMatchObject({ name: 'SelectorOptionsUnavailableError', status: 502 })
73+
})
74+
75+
it('preserves caller cancellation', async () => {
76+
const controller = new AbortController()
77+
const abortError = new DOMException('The operation was aborted', 'AbortError')
78+
controller.abort(abortError)
79+
mockListCloudWatchLogGroups.mockRejectedValueOnce(abortError)
80+
81+
await expect(
82+
cloudWatchSelectorAttachments['cloudwatch.logGroups'].execute(logGroupArgs(controller.signal))
83+
).rejects.toBe(abortError)
84+
})
85+
86+
it('rejects an invalid region before invoking the AWS listing helper', async () => {
87+
const args = logGroupArgs()
88+
args.context.awsRegion = 'not-a-region'
89+
90+
await expect(
91+
cloudWatchSelectorAttachments['cloudwatch.logGroups'].execute(args)
92+
).rejects.toMatchObject({ name: 'SelectorContextUnavailableError' })
93+
expect(mockListCloudWatchLogGroups).not.toHaveBeenCalled()
94+
expect(mockListCloudWatchLogStreams).not.toHaveBeenCalled()
95+
})
96+
})

0 commit comments

Comments
 (0)