Skip to content

Commit 855ce1f

Browse files
Bill LeoutsakosBill Leoutsakos
Bill Leoutsakos
authored and
Bill Leoutsakos
committed
fix(selectors): address review findings
1 parent db0ffc0 commit 855ce1f

21 files changed

Lines changed: 629 additions & 81 deletions

‎apps/sim/app/api/selectors/execute/route.test.ts‎

Lines changed: 110 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -4,25 +4,85 @@
44
import { createMockRequest } from '@sim/testing'
55
import { describe, expect, it, vi } from 'vitest'
66

7-
const mocks = vi.hoisted(() => ({ status: 200 }))
8-
9-
vi.mock('@/lib/api/server/routes', () => ({
10-
defineInternalJsonRoute: vi.fn(
11-
(options: { staticResponseHeaders?: HeadersInit }) => async () =>
12-
new Response(JSON.stringify({ ok: mocks.status < 400 }), {
13-
status: mocks.status,
14-
headers: options.staticResponseHeaders,
15-
})
16-
),
17-
extendInternalErrorPolicy: vi.fn(() => ({})),
18-
internalErrorResponse: vi.fn(),
19-
internalOrchestrationErrorPolicy: {},
20-
internalRateLimits: { none: vi.fn(() => ({ kind: 'none' })) },
21-
internalSessionAuth: {},
7+
const mocks = vi.hoisted(() => ({
8+
status: 200,
9+
errorPolicy: undefined as
10+
| {
11+
project(error: unknown): { body: unknown; status: number; headers?: HeadersInit } | null
12+
unhandled?(): { body: unknown; status: number; headers?: HeadersInit }
13+
}
14+
| undefined,
2215
}))
2316

17+
vi.mock('@/lib/api/server/routes', () => {
18+
const internalErrorResponse = vi.fn((status: number, body: unknown, headers?: HeadersInit) => ({
19+
body,
20+
status,
21+
headers,
22+
}))
23+
const internalOrchestrationErrorPolicy = {
24+
project(error: unknown) {
25+
if (!(error instanceof Error) || !('code' in error)) return null
26+
const code = (error as Error & { code: string }).code
27+
const status =
28+
code === 'validation'
29+
? 400
30+
: code === 'unauthorized'
31+
? 401
32+
: code === 'forbidden'
33+
? 403
34+
: code === 'not_found'
35+
? 404
36+
: code === 'conflict'
37+
? 409
38+
: 500
39+
return internalErrorResponse(status, { error: error.message })
40+
},
41+
unhandled: () => internalErrorResponse(500, { error: 'Internal server error' }),
42+
}
43+
44+
return {
45+
defineInternalJsonRoute: vi.fn(
46+
(options: { errorPolicy: typeof mocks.errorPolicy; staticResponseHeaders?: HeadersInit }) => {
47+
mocks.errorPolicy = options.errorPolicy
48+
return async () =>
49+
new Response(JSON.stringify({ ok: mocks.status < 400 }), {
50+
status: mocks.status,
51+
headers: options.staticResponseHeaders,
52+
})
53+
}
54+
),
55+
extendInternalErrorPolicy: vi.fn(
56+
(
57+
base: typeof internalOrchestrationErrorPolicy,
58+
project: (error: unknown) => ReturnType<typeof internalErrorResponse> | null
59+
) => ({
60+
project: (error: unknown) => project(error) ?? base.project(error),
61+
unhandled: base.unhandled,
62+
})
63+
),
64+
internalErrorResponse,
65+
internalOrchestrationErrorPolicy,
66+
internalRateLimits: { none: vi.fn(() => ({ kind: 'none' })) },
67+
internalSessionAuth: {},
68+
}
69+
})
70+
71+
import { NoWorkspaceAccessError } from '@/lib/core/application/workspace-authorization'
72+
import { OrchestrationError } from '@/lib/core/orchestration/types'
73+
import {
74+
SelectorConnectionUnavailableError,
75+
SelectorContextUnavailableError,
76+
SelectorOptionsUnavailableError,
77+
} from '@/lib/selectors/server/errors'
2478
import { POST } from '@/app/api/selectors/execute/route'
2579

80+
function project(error: unknown) {
81+
const result = mocks.errorPolicy?.project(error)
82+
if (!result) throw new Error('Expected route error policy to project the error')
83+
return result
84+
}
85+
2686
describe('POST /api/selectors/execute', () => {
2787
it('marks success, authentication, parse, and unhandled responses private and non-cacheable', async () => {
2888
for (const status of [200, 400, 401, 500]) {
@@ -33,4 +93,39 @@ describe('POST /api/selectors/execute', () => {
3393
expect(response.headers.get('Cache-Control')).toBe('private, no-store')
3494
}
3595
})
96+
97+
it.each([
98+
['missing workflow', new OrchestrationError('not_found', 'Workflow not found')],
99+
['missing workspace', new OrchestrationError('not_found', 'Workspace not found')],
100+
['asserted workspace mismatch', new OrchestrationError('not_found', 'Workflow not found')],
101+
['cross-tenant workspace', new NoWorkspaceAccessError()],
102+
])('conceals %s as the same selector-scope absence', (_case, error) => {
103+
expect(project(error)).toEqual({
104+
status: 404,
105+
body: { error: 'Selector scope not found' },
106+
headers: { 'Cache-Control': 'private, no-store' },
107+
})
108+
})
109+
110+
it.each([
111+
[new SelectorContextUnavailableError(), 400, 'Context unavailable'],
112+
[new SelectorConnectionUnavailableError(), 403, 'Connection unavailable'],
113+
[new SelectorOptionsUnavailableError(), 502, 'Options unavailable'],
114+
])('preserves selector error projection for %s', (error, status, message) => {
115+
expect(project(error)).toEqual({
116+
status,
117+
body: { error: message },
118+
headers: { 'Cache-Control': 'private, no-store' },
119+
})
120+
})
121+
122+
it('preserves same-workspace forbidden errors', () => {
123+
expect(
124+
project(new OrchestrationError('forbidden', 'Insufficient workspace permissions'))
125+
).toEqual({
126+
status: 403,
127+
body: { error: 'Insufficient workspace permissions' },
128+
headers: undefined,
129+
})
130+
})
36131
})

‎apps/sim/app/api/selectors/execute/route.ts‎

Lines changed: 38 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,14 @@ import { executeSelectorContract } from '@/lib/api/contracts/selectors/execute'
22
import {
33
defineInternalJsonRoute,
44
extendInternalErrorPolicy,
5+
type InternalErrorPolicy,
56
internalErrorResponse,
67
internalOrchestrationErrorPolicy,
78
internalRateLimits,
89
internalSessionAuth,
910
} from '@/lib/api/server/routes'
11+
import { createInternalResourceConcealmentPolicy } from '@/lib/api/server/routes/resource-concealment'
12+
import { asOrchestrationError } from '@/lib/core/orchestration/types'
1013
import { executeSelector } from '@/lib/selectors/application/execute-selector'
1114
import { selectorOperations } from '@/lib/selectors/application/operations'
1215
import {
@@ -16,18 +19,43 @@ import {
1619
} from '@/lib/selectors/server/errors'
1720

1821
const PRIVATE_NO_STORE = { 'Cache-Control': 'private, no-store' } as const
22+
const SELECTOR_SCOPE_NOT_FOUND = 'Selector scope not found'
1923

20-
const selectorErrorPolicy = extendInternalErrorPolicy(internalOrchestrationErrorPolicy, (error) => {
21-
if (error instanceof SelectorContextUnavailableError) {
22-
return internalErrorResponse(400, { error: 'Context unavailable' }, PRIVATE_NO_STORE)
23-
}
24-
if (error instanceof SelectorConnectionUnavailableError) {
25-
return internalErrorResponse(403, { error: 'Connection unavailable' }, PRIVATE_NO_STORE)
26-
}
27-
if (error instanceof SelectorOptionsUnavailableError) {
28-
return internalErrorResponse(502, { error: 'Options unavailable' }, PRIVATE_NO_STORE)
24+
const selectorOperationErrorPolicy = extendInternalErrorPolicy(
25+
internalOrchestrationErrorPolicy,
26+
(error) => {
27+
if (error instanceof SelectorContextUnavailableError) {
28+
return internalErrorResponse(400, { error: 'Context unavailable' }, PRIVATE_NO_STORE)
29+
}
30+
if (error instanceof SelectorConnectionUnavailableError) {
31+
return internalErrorResponse(403, { error: 'Connection unavailable' }, PRIVATE_NO_STORE)
32+
}
33+
if (error instanceof SelectorOptionsUnavailableError) {
34+
return internalErrorResponse(502, { error: 'Options unavailable' }, PRIVATE_NO_STORE)
35+
}
36+
return null
2937
}
30-
return null
38+
)
39+
40+
/**
41+
* This route accepts both workflow and workspace scopes, whose canonical loaders
42+
* use different not-found messages. Normalize those ordinary misses together
43+
* with concealed cross-tenant denials so neither status nor body reveals whether
44+
* a caller-supplied scope exists.
45+
*/
46+
const selectorScopeNotFoundPolicy: InternalErrorPolicy = {
47+
project(error) {
48+
if (asOrchestrationError(error)?.code === 'not_found') {
49+
return internalErrorResponse(404, { error: SELECTOR_SCOPE_NOT_FOUND }, PRIVATE_NO_STORE)
50+
}
51+
return selectorOperationErrorPolicy.project(error)
52+
},
53+
unhandled: selectorOperationErrorPolicy.unhandled,
54+
}
55+
56+
const selectorErrorPolicy = createInternalResourceConcealmentPolicy({
57+
base: selectorScopeNotFoundPolicy,
58+
notFoundMessage: SELECTOR_SCOPE_NOT_FOUND,
3159
})
3260

3361
export const POST = defineInternalJsonRoute({

‎apps/sim/hooks/queries/credential-groups.ts‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ import {
2424
credentialGroupKeys,
2525
fetchCredentialGroupList,
2626
} from '@/hooks/queries/utils/credential-group-queries'
27-
import { selectorKeys } from '@/hooks/queries/utils/selector-keys'
27+
import { invalidateSelectorQueries } from '@/hooks/queries/utils/selector-keys'
2828

2929
export function useCredentialGroups(workspaceId?: string) {
3030
return useQuery({
@@ -125,7 +125,7 @@ export function useUpdateCredentialGroupAccess() {
125125
queryKey: credentialGroupKeys.access(variables.workspaceId, variables.groupId),
126126
exact: true,
127127
}),
128-
queryClient.invalidateQueries({ queryKey: selectorKeys.all }),
128+
invalidateSelectorQueries(queryClient),
129129
]),
130130
})
131131
}
@@ -142,7 +142,7 @@ export function useCreateCredentialGroup() {
142142
}) => requestJson(createCredentialGroupContract, { params: { id: workspaceId }, body }),
143143
onSettled: (_data, _error, variables) => {
144144
queryClient.invalidateQueries({ queryKey: credentialGroupKeys.list(variables.workspaceId) })
145-
queryClient.invalidateQueries({ queryKey: selectorKeys.all })
145+
invalidateSelectorQueries(queryClient)
146146
},
147147
})
148148
}
@@ -159,7 +159,7 @@ export function useDeleteCredentialGroup() {
159159
queryClient.removeQueries({
160160
queryKey: credentialGroupKeys.detail(variables.workspaceId, variables.groupId),
161161
})
162-
queryClient.invalidateQueries({ queryKey: selectorKeys.all })
162+
invalidateSelectorQueries(queryClient)
163163
},
164164
})
165165
}
@@ -191,7 +191,7 @@ export function useUpdateCredentialGroup() {
191191
queryClient.invalidateQueries({
192192
queryKey: credentialGroupKeys.detail(variables.workspaceId, variables.groupId),
193193
}),
194-
queryClient.invalidateQueries({ queryKey: selectorKeys.all }),
194+
invalidateSelectorQueries(queryClient),
195195
]),
196196
})
197197
}
@@ -234,7 +234,7 @@ export function useInviteCredentialGroupEnrollments() {
234234
queryClient.invalidateQueries({
235235
queryKey: credentialGroupKeys.detail(variables.workspaceId, variables.groupId),
236236
})
237-
queryClient.invalidateQueries({ queryKey: selectorKeys.all })
237+
invalidateSelectorQueries(queryClient)
238238
},
239239
})
240240
}
@@ -258,7 +258,7 @@ export function useResendCredentialGroupEnrollment() {
258258
queryClient.invalidateQueries({
259259
queryKey: credentialGroupKeys.detail(variables.workspaceId, variables.groupId),
260260
})
261-
queryClient.invalidateQueries({ queryKey: selectorKeys.all })
261+
invalidateSelectorQueries(queryClient)
262262
},
263263
})
264264
}
@@ -282,7 +282,7 @@ export function useDeleteCredentialGroupEnrollment() {
282282
queryClient.invalidateQueries({
283283
queryKey: credentialGroupKeys.detail(variables.workspaceId, variables.groupId),
284284
})
285-
queryClient.invalidateQueries({ queryKey: selectorKeys.all })
285+
invalidateSelectorQueries(queryClient)
286286
},
287287
})
288288
}

‎apps/sim/hooks/queries/credentials.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ import {
3030
requireWorkspaceCredentialListResponse,
3131
WORKSPACE_CREDENTIAL_LIST_STALE_TIME,
3232
} from '@/hooks/queries/utils/fetch-workspace-credentials'
33-
import { selectorKeys } from '@/hooks/queries/utils/selector-keys'
33+
import { invalidateSelectorQueries } from '@/hooks/queries/utils/selector-keys'
3434

3535
/**
3636
* Key prefix for OAuth credential queries.
@@ -133,7 +133,7 @@ export function useCreateWorkspaceCredential() {
133133
queryClient.invalidateQueries({
134134
queryKey: OAUTH_CREDENTIALS_KEY,
135135
})
136-
queryClient.invalidateQueries({ queryKey: selectorKeys.all })
136+
invalidateSelectorQueries(queryClient)
137137
},
138138
})
139139
}
@@ -224,7 +224,7 @@ export function useUpdateWorkspaceCredential() {
224224
queryClient.invalidateQueries({
225225
queryKey: OAUTH_CREDENTIALS_KEY,
226226
})
227-
queryClient.invalidateQueries({ queryKey: selectorKeys.all })
227+
invalidateSelectorQueries(queryClient)
228228
},
229229
})
230230
}
@@ -242,7 +242,7 @@ export function useDeleteWorkspaceCredential() {
242242
queryClient.invalidateQueries({ queryKey: OAUTH_CREDENTIALS_KEY })
243243
queryClient.invalidateQueries({ queryKey: environmentKeys.all })
244244
queryClient.invalidateQueries({ queryKey: oauthConnectionsKeys.connections() })
245-
queryClient.invalidateQueries({ queryKey: selectorKeys.all })
245+
invalidateSelectorQueries(queryClient)
246246
},
247247
})
248248
}
@@ -287,7 +287,7 @@ export function useUpsertWorkspaceCredentialMember() {
287287
queryClient.invalidateQueries({
288288
queryKey: workspaceCredentialKeys.detail(variables.credentialId),
289289
})
290-
queryClient.invalidateQueries({ queryKey: selectorKeys.all })
290+
invalidateSelectorQueries(queryClient)
291291
},
292292
})
293293
}
@@ -313,7 +313,7 @@ export function useRemoveWorkspaceCredentialMember() {
313313
queryClient.invalidateQueries({
314314
queryKey: workspaceCredentialKeys.detail(variables.credentialId),
315315
})
316-
queryClient.invalidateQueries({ queryKey: selectorKeys.all })
316+
invalidateSelectorQueries(queryClient)
317317
},
318318
})
319319
}

‎apps/sim/hooks/queries/environment.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import {
99
} from '@/lib/api/contracts'
1010
import type { WorkspaceEnvironmentData } from '@/lib/environment/api'
1111
import { fetchPersonalEnvironment, fetchWorkspaceEnvironment } from '@/lib/environment/api'
12-
import { selectorKeys } from '@/hooks/queries/utils/selector-keys'
12+
import { invalidateSelectorQueries } from '@/hooks/queries/utils/selector-keys'
1313

1414
const logger = createLogger('EnvironmentQueries')
1515

@@ -80,7 +80,7 @@ export function useSavePersonalEnvironment() {
8080
await Promise.all([
8181
queryClient.invalidateQueries({ queryKey: environmentKeys.personal() }),
8282
queryClient.invalidateQueries({ queryKey: environmentKeys.workspaces() }),
83-
queryClient.invalidateQueries({ queryKey: selectorKeys.all }),
83+
invalidateSelectorQueries(queryClient),
8484
])
8585
},
8686
})
@@ -110,7 +110,7 @@ export function useUpsertWorkspaceEnvironment() {
110110
queryClient.invalidateQueries({
111111
queryKey: environmentKeys.workspace(variables.workspaceId),
112112
}),
113-
queryClient.invalidateQueries({ queryKey: selectorKeys.all }),
113+
invalidateSelectorQueries(queryClient),
114114
]),
115115
})
116116
}
@@ -139,7 +139,7 @@ export function useRemoveWorkspaceEnvironment() {
139139
queryClient.invalidateQueries({
140140
queryKey: environmentKeys.workspace(variables.workspaceId),
141141
}),
142-
queryClient.invalidateQueries({ queryKey: selectorKeys.all }),
142+
invalidateSelectorQueries(queryClient),
143143
]),
144144
})
145145
}

0 commit comments

Comments
 (0)