Skip to content

Commit 6295a80

Browse files
committed
fix(search): validate Atlassian keys beyond discovery limits
1 parent a52d406 commit 6295a80

6 files changed

Lines changed: 301 additions & 34 deletions

File tree

‎apps/sim/lib/knowledge/application/personal-source-setup.test.ts‎

Lines changed: 185 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/** @vitest-environment node */
2-
import { beforeEach, describe, expect, it, vi } from 'vitest'
2+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
33

44
const mocks = vi.hoisted(() => ({
55
authorizeOperation: vi.fn(),
@@ -69,6 +69,12 @@ import {
6969
listPersonalSourceSetupAccounts,
7070
personalSourceSetup,
7171
} from '@/lib/knowledge/application/personal-source-setup'
72+
import { MAX_SELECTOR_PAGES } from '@/lib/selectors/limits'
73+
import type { SelectorRequest } from '@/lib/selectors/types'
74+
75+
interface ValidationSelectorCall {
76+
input: { request: SelectorRequest; signal: AbortSignal }
77+
}
7278

7379
const principal = { kind: 'session', userId: 'member-1', sessionId: 'session-1' } as const
7480
const owner = { organizationId: 'organization-1', connectorType: 'jira' } as const
@@ -84,6 +90,10 @@ const runConnect = (changes = {}) =>
8490
personalSourceSetup.execute({ principal, input: { ...connect, keys: ['PROJECT'], ...changes } })
8591

8692
describe('personal source setup', () => {
93+
afterEach(() => {
94+
vi.restoreAllMocks()
95+
})
96+
8797
beforeEach(() => {
8898
vi.resetAllMocks()
8999
mocks.authorize.mockResolvedValue(principal.userId)
@@ -245,15 +255,154 @@ describe('personal source setup', () => {
245255
{ kind: 'list', items: [], truncated: true, nextCursor: '50' },
246256
{ kind: 'list', items: [] },
247257
])('rejects unavailable manually entered keys before source creation %#', async (page) => {
248-
mocks.selector.mockResolvedValue(page)
258+
mocks.selector.mockResolvedValueOnce(page).mockResolvedValue({ kind: 'detail', item: null })
249259
await expect(runConnect({ keys: ['MISSING'] })).rejects.toThrow('could not be found')
250260
expect(mocks.configure).not.toHaveBeenCalled()
251261
})
252262

253-
it('rejects a repeated pagination cursor without looping or creating a source', async () => {
254-
mocks.selector.mockResolvedValue({ kind: 'list', items: [], nextCursor: '50' })
263+
it.each(['jira', 'confluence'] as const)(
264+
'verifies a manually entered %s key outside the available listing through the same authorized selector',
265+
async (connectorType) => {
266+
mocks.selector
267+
.mockResolvedValueOnce({ kind: 'list', items: [] })
268+
.mockResolvedValueOnce({ kind: 'detail', item: { id: 'PROJECT', label: 'Project' } })
269+
await expect(runConnect({ connectorType })).resolves.toMatchObject({ kind: 'connected' })
270+
expect(mocks.selector).toHaveBeenLastCalledWith({
271+
principal,
272+
request: undefined,
273+
input: {
274+
selectorKey: connectorType === 'jira' ? 'jira.projectKeys' : 'confluence.spaces',
275+
scope: { kind: 'organization', organizationId: owner.organizationId },
276+
context: { oauthCredential: credential.credentialId, domain: credential.domain },
277+
personalSearchSetup: connectorType,
278+
signal: expect.any(AbortSignal),
279+
request: { kind: 'detail', id: 'PROJECT' },
280+
},
281+
})
282+
expect(mocks.ownAccount).toHaveBeenCalledTimes(2)
283+
expect(mocks.oauth).not.toHaveBeenCalled()
284+
}
285+
)
286+
287+
it.each([
288+
{ name: 'truncated results', pages: 1, truncated: true, nextCursor: '50' },
289+
{ name: 'a repeated cursor', pages: 2, nextCursor: '50' },
290+
{ name: 'the page limit', pages: MAX_SELECTOR_PAGES },
291+
])('resolves remaining keys directly after $name', async ({ pages, truncated, nextCursor }) => {
292+
let listed = 0
293+
mocks.selector.mockImplementation(({ input }: ValidationSelectorCall) => {
294+
if (input.request.kind === 'detail') {
295+
return { kind: 'detail', item: { id: input.request.id, label: 'Project' } }
296+
}
297+
listed++
298+
return { kind: 'list', items: [], nextCursor: nextCursor ?? String(listed), truncated }
299+
})
300+
await expect(runConnect()).resolves.toMatchObject({ kind: 'connected' })
301+
expect(listed).toBe(pages)
302+
expect(mocks.selector).toHaveBeenCalledTimes(pages + 1)
303+
expect(mocks.oauth).not.toHaveBeenCalled()
304+
})
305+
306+
it('gives direct validation a fresh deadline when listing times out', async () => {
307+
const listing = new AbortController()
308+
const details = new AbortController()
309+
vi.spyOn(AbortSignal, 'timeout')
310+
.mockReturnValueOnce(listing.signal)
311+
.mockReturnValueOnce(details.signal)
312+
mocks.selector
313+
.mockImplementationOnce(({ input }: ValidationSelectorCall) => {
314+
listing.abort(new DOMException('Listing timed out', 'TimeoutError'))
315+
input.signal.throwIfAborted()
316+
})
317+
.mockImplementationOnce(({ input }: ValidationSelectorCall) => {
318+
expect(input.signal.aborted).toBe(false)
319+
expect(input.request).toEqual({ kind: 'detail', id: 'PROJECT' })
320+
return { kind: 'detail', item: { id: 'PROJECT', label: 'Project' } }
321+
})
322+
await expect(runConnect()).resolves.toMatchObject({ kind: 'connected' })
323+
expect(AbortSignal.timeout).toHaveBeenCalledTimes(2)
324+
expect(mocks.oauth).not.toHaveBeenCalled()
325+
})
326+
327+
it('fails before mutation when direct validation also exceeds its deadline', async () => {
328+
const listing = new AbortController()
329+
const details = new AbortController()
330+
vi.spyOn(AbortSignal, 'timeout')
331+
.mockReturnValueOnce(listing.signal)
332+
.mockReturnValueOnce(details.signal)
333+
mocks.selector
334+
.mockResolvedValueOnce({ kind: 'list', items: [] })
335+
.mockImplementationOnce(({ input }: ValidationSelectorCall) => {
336+
details.abort(new DOMException('Validation timed out', 'TimeoutError'))
337+
input.signal.throwIfAborted()
338+
})
339+
await expect(runConnect()).rejects.toThrow('took too long')
340+
expect(mocks.configure).not.toHaveBeenCalled()
341+
})
342+
343+
it('does not retry direct validation after the caller cancels listing', async () => {
344+
const caller = new AbortController()
345+
mocks.selector.mockImplementationOnce(({ input }: ValidationSelectorCall) => {
346+
caller.abort(new Error('Setup cancelled'))
347+
input.signal.throwIfAborted()
348+
})
349+
await expect(
350+
personalSourceSetup.execute({
351+
principal,
352+
input: { ...connect, keys: ['PROJECT'] },
353+
request: { headers: new Headers(), signal: caller.signal },
354+
})
355+
).rejects.toThrow('Setup cancelled')
356+
expect(mocks.selector).toHaveBeenCalledTimes(1)
357+
expect(mocks.configure).not.toHaveBeenCalled()
358+
})
359+
360+
it('rejects a detail response for a different key', async () => {
361+
mocks.selector
362+
.mockResolvedValueOnce({ kind: 'list', items: [] })
363+
.mockResolvedValueOnce({ kind: 'detail', item: { id: 'OTHER', label: 'Other project' } })
255364
await expect(runConnect()).rejects.toThrow('could not be found')
256-
expect(mocks.selector).toHaveBeenCalledTimes(2)
365+
expect(mocks.configure).not.toHaveBeenCalled()
366+
})
367+
368+
it('bounds direct validation concurrency and validates each unresolved key only once', async () => {
369+
let release = () => {}
370+
let allStarted = () => {}
371+
const gate = new Promise<void>((resolve) => {
372+
release = resolve
373+
})
374+
const started = new Promise<void>((resolve) => {
375+
allStarted = resolve
376+
})
377+
let active = 0
378+
let maximumActive = 0
379+
mocks.selector.mockImplementation(async ({ input }: ValidationSelectorCall) => {
380+
if (input.request.kind === 'list') return { kind: 'list', items: [] }
381+
active++
382+
maximumActive = Math.max(maximumActive, active)
383+
if (active === 5) allStarted()
384+
await gate
385+
active--
386+
return { kind: 'detail', item: { id: input.request.id, label: input.request.id } }
387+
})
388+
const keys = Array.from({ length: 12 }, (_, index) => `PROJECT${index}`)
389+
const connection = runConnect({ keys: [...keys, ...keys] })
390+
await started
391+
try {
392+
expect(mocks.selector).toHaveBeenCalledTimes(6)
393+
expect(mocks.configure).not.toHaveBeenCalled()
394+
} finally {
395+
release()
396+
}
397+
await expect(connection).resolves.toMatchObject({ kind: 'connected' })
398+
expect(maximumActive).toBe(5)
399+
expect(mocks.selector).toHaveBeenCalledTimes(keys.length + 1)
400+
})
401+
402+
it('propagates listing failures without starting direct validation', async () => {
403+
mocks.selector.mockRejectedValueOnce(new Error('Account revoked'))
404+
await expect(runConnect()).rejects.toThrow('Account revoked')
405+
expect(mocks.selector).toHaveBeenCalledTimes(1)
257406
expect(mocks.configure).not.toHaveBeenCalled()
258407
})
259408

@@ -274,6 +423,37 @@ describe('personal source setup', () => {
274423
expect(mocks.configure).not.toHaveBeenCalled()
275424
})
276425

426+
it.each(['binding', 'ownership'] as const)(
427+
'stops before source creation if the caller cancels during the final %s check',
428+
async (phase) => {
429+
const caller = new AbortController()
430+
if (phase === 'binding') {
431+
mocks.binding.mockImplementationOnce(() => {
432+
caller.abort(new Error('Setup cancelled'))
433+
return {
434+
organizationId: owner.organizationId,
435+
credentialGroupId: 'group-1',
436+
credentialGroupOptionId: 'option-1',
437+
}
438+
})
439+
} else {
440+
mocks.ownAccount.mockResolvedValueOnce(account).mockImplementationOnce(() => {
441+
caller.abort(new Error('Setup cancelled'))
442+
return account
443+
})
444+
}
445+
await expect(
446+
personalSourceSetup.execute({
447+
principal,
448+
input: { ...connect, keys: ['PROJECT'] },
449+
request: { headers: new Headers(), signal: caller.signal },
450+
})
451+
).rejects.toThrow('Setup cancelled')
452+
expect(mocks.configure).not.toHaveBeenCalled()
453+
expect(mocks.dispatch).not.toHaveBeenCalled()
454+
}
455+
)
456+
277457
it('rejects a current operation authorization denial before any setup effects', async () => {
278458
mocks.authorizeOperation.mockRejectedValue(new Error('Membership ended'))
279459
await expect(runConnect()).rejects.toThrow('Membership ended')

‎apps/sim/lib/knowledge/application/personal-source-setup.ts‎

Lines changed: 46 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { createLogger } from '@sim/logger'
22
import { getErrorMessage } from '@sim/utils/errors'
33
import { normalizeAtlassianSiteUrl } from '@/lib/atlassian/discovery'
44
import { OrchestrationError } from '@/lib/core/orchestration/types'
5+
import { mapWithConcurrency } from '@/lib/core/utils/concurrency'
56
import {
67
loadManagedCredentialGroupBinding,
78
loadScopedAccountsCredentialListContext,
@@ -29,6 +30,8 @@ import { MAX_PERSONAL_SOURCE_SETUP_KEYS } from '@/lib/sim-search/personal-source
2930
import { CONNECTOR_META_REGISTRY } from '@/connectors/registry'
3031

3132
const logger = createLogger('PersonalSourceSetup')
33+
const VALIDATION_PHASE_TIMEOUT_MS = 30_000
34+
const DETAIL_VALIDATION_CONCURRENCY = 5
3235

3336
interface PersonalSourceSetupOwner {
3437
organizationId: string
@@ -176,12 +179,13 @@ export const personalSourceSetup = defineAuthorizedKnowledgeUseCase({
176179
const keys = [...new Set(input.keys.map((key) => key.trim()))]
177180
const remaining = new Set(keys)
178181
const cursors = new Set<string>()
179-
const timeout = AbortSignal.timeout(30_000)
182+
const timeout = AbortSignal.timeout(VALIDATION_PHASE_TIMEOUT_MS)
180183
const signal = request?.signal ? AbortSignal.any([request.signal, timeout]) : timeout
181184
let cursor: string | undefined
182185
for (let page = 0; page < MAX_SELECTOR_PAGES; page++) {
183186
let result: SelectorExecutionResult
184187
try {
188+
signal.throwIfAborted()
185189
result = await executeSelector.execute({
186190
principal,
187191
request,
@@ -191,13 +195,9 @@ export const personalSourceSetup = defineAuthorizedKnowledgeUseCase({
191195
request: { kind: 'list', ...(cursor ? { cursor } : {}) },
192196
},
193197
})
198+
signal.throwIfAborted()
194199
} catch (error) {
195-
if (timeout.aborted && !request?.signal?.aborted) {
196-
throw new OrchestrationError(
197-
'validation',
198-
'Checking the selected projects or spaces took too long. Try fewer selections.'
199-
)
200-
}
200+
if (timeout.aborted && !request?.signal?.aborted) break
201201
throw error
202202
}
203203
if (result.kind !== 'list') throw new Error('Source discovery returned an unexpected result')
@@ -207,11 +207,45 @@ export const personalSourceSetup = defineAuthorizedKnowledgeUseCase({
207207
cursor = result.nextCursor
208208
cursors.add(cursor)
209209
}
210+
request?.signal?.throwIfAborted()
210211
if (remaining.size > 0) {
211-
throw new OrchestrationError(
212-
'validation',
213-
'Some selected projects or spaces could not be found with this account. Refresh the choices and try again.'
214-
)
212+
const detailTimeout = AbortSignal.timeout(VALIDATION_PHASE_TIMEOUT_MS)
213+
const details = new AbortController()
214+
const detailSignal = AbortSignal.any([
215+
detailTimeout,
216+
details.signal,
217+
...(request?.signal ? [request.signal] : []),
218+
])
219+
try {
220+
await mapWithConcurrency([...remaining], DETAIL_VALIDATION_CONCURRENCY, async (key) => {
221+
detailSignal.throwIfAborted()
222+
const result = await executeSelector.execute({
223+
principal,
224+
request,
225+
input: {
226+
...selectorInput,
227+
signal: detailSignal,
228+
request: { kind: 'detail', id: key },
229+
},
230+
})
231+
detailSignal.throwIfAborted()
232+
if (result.kind !== 'detail' || result.item?.id !== key) {
233+
throw new OrchestrationError(
234+
'validation',
235+
'Some selected projects or spaces could not be found with this account. Refresh the choices and try again.'
236+
)
237+
}
238+
})
239+
} catch (error) {
240+
details.abort(error)
241+
if (detailTimeout.aborted && !request?.signal?.aborted) {
242+
throw new OrchestrationError(
243+
'validation',
244+
'Checking the selected projects or spaces took too long. Try fewer selections.'
245+
)
246+
}
247+
throw error
248+
}
215249
}
216250
const [binding, group] = await Promise.all([
217251
loadManagedCredentialGroupBinding(input.credentialId),
@@ -232,6 +266,7 @@ export const personalSourceSetup = defineAuthorizedKnowledgeUseCase({
232266
)
233267
}
234268
await authorizePersonalSearchSetupCredential(principal, input)
269+
request?.signal?.throwIfAborted()
235270
const result = await configureSimSearchConnector.execute({
236271
principal,
237272
request,

0 commit comments

Comments
 (0)