Skip to content

Commit f78fc4e

Browse files
Bill LeoutsakosBill Leoutsakos
authored andcommitted
fix(selectors): address Cubic rerun findings
1 parent 96df0b7 commit f78fc4e

15 files changed

Lines changed: 333 additions & 39 deletions

File tree

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1080,8 +1080,10 @@ execute through `selectors.execute`; never add a client provider module or selec
10801080
`canonicalParamId: 'oauthCredential'` on the credential sub-block is the line people forget. The
10811081
shared context builder projects only active `dependsOn` values and keys canonical pairs by their
10821082
canonical id. Exact environment references such as `{{GMAIL_CREDENTIAL_ID}}` stay unresolved in the
1083-
browser and are resolved only by the authorized server executor. A credential field is also
1084-
recognized by its `oauth-input` type as a compatibility fallback.
1083+
browser and are resolved only by the authorized server executor. The builder does not infer a
1084+
nonstandard credential id from `type: 'oauth-input'`; give it
1085+
`canonicalParamId: 'oauthCredential'`, or declare an explicit manifest `sourceFields` alias when a
1086+
legacy source id must be retained.
10851087

10861088
**`options` — everything else.** A static array, or a pure function of the block's own values for a list that narrows to a sibling's selection. No I/O.
10871089

apps/sim/app/api/webhooks/route.test.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ describe('POST /api/webhooks polling configuration', () => {
8181
blockId: 'block-1',
8282
path: 'imap-hook',
8383
provider: 'imap',
84+
deploymentVersionId: 'deployment-1',
8485
providerConfig: {
8586
host: '{{IMAP_HOST}}',
8687
username: '{{IMAP_USERNAME}}',
@@ -115,6 +116,7 @@ describe('POST /api/webhooks polling configuration', () => {
115116
requestId: 'mock-request-id',
116117
userId: 'actor-1',
117118
workspaceId: 'canonical-workspace',
119+
deploymentVersionId: 'deployment-1',
118120
})
119121
})
120122
})

apps/sim/app/api/webhooks/route.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -509,6 +509,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
509509
userId,
510510
workspaceId:
511511
typeof workflowRecord.workspaceId === 'string' ? workflowRecord.workspaceId : null,
512+
deploymentVersionId: savedWebhook.deploymentVersionId ?? null,
512513
})
513514

514515
if (!success) {

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/dropdown/dropdown.test.tsx

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ const { fetched } = vi.hoisted(() => ({
1414
hasLoadedOptions: true,
1515
fetchError: null as string | null,
1616
hydratedOptions: [] as { id: string; label: string }[],
17+
selectedValues: ['col_a', 'col_gone'] as string[],
1718
},
1819
}))
1920

@@ -61,7 +62,7 @@ vi.mock(
6162
)
6263
vi.mock(
6364
'@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/hooks/use-sub-block-value',
64-
() => ({ useSubBlockValue: () => [['col_a', 'col_gone'], () => {}] })
65+
() => ({ useSubBlockValue: () => [fetched.selectedValues, () => {}] })
6566
)
6667
vi.mock(
6768
'@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/providers/active-search-target-provider',
@@ -154,4 +155,25 @@ describe('Dropdown multi-select stale selections', () => {
154155
fetched.hydratedOptions = []
155156
}
156157
})
158+
159+
it('preserves selected value order when hydrating multiple missing options', () => {
160+
const previousOptions = fetched.options
161+
const previousSelectedValues = fetched.selectedValues
162+
fetched.options = []
163+
fetched.selectedValues = ['col_first', 'col_second']
164+
fetched.hydratedOptions = [
165+
{ id: 'col_first', label: 'First column' },
166+
{ id: 'col_second', label: 'Second column' },
167+
]
168+
try {
169+
const html = render()
170+
expect(html.indexOf('data-value="col_first"')).toBeLessThan(
171+
html.indexOf('data-value="col_second"')
172+
)
173+
} finally {
174+
fetched.options = previousOptions
175+
fetched.selectedValues = previousSelectedValues
176+
fetched.hydratedOptions = []
177+
}
178+
})
157179
})

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/dropdown/dropdown.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,7 @@ export const Dropdown = memo(function Dropdown({
207207
}
208208
}
209209

210-
for (const option of hydratedOptions) {
210+
for (const option of [...hydratedOptions].reverse()) {
211211
const alreadyPresent = opts.some((existing) =>
212212
typeof existing === 'string' ? existing === option.id : existing.id === option.id
213213
)

apps/sim/lib/imap/connection.server.test.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,24 @@ describe('IMAP connection policy', () => {
9090
)
9191
})
9292

93+
it('preserves the legacy TLS defaults for nullable connection values', () => {
94+
expect(
95+
normalizeLiteralImapConnection({
96+
host: 'imap.example.com',
97+
port: null,
98+
secure: null,
99+
username: 'mailbox-user',
100+
password: 'literal-password',
101+
})
102+
).toEqual({
103+
host: 'imap.example.com',
104+
port: 993,
105+
secure: true,
106+
username: 'mailbox-user',
107+
password: 'literal-password',
108+
})
109+
})
110+
93111
it('resolves exact personal and visible shared references for the deployment actor', async () => {
94112
environmentUtilsMockFns.mockResolveEffectiveEnvironmentVariables.mockResolvedValue({
95113
PERSONAL_PASSWORD: {
@@ -182,6 +200,21 @@ describe('IMAP connection policy', () => {
182200
)
183201
})
184202

203+
it('rejects unresolved workflow references in literal IMAP connection fields', () => {
204+
expect(() =>
205+
normalizeLiteralImapConnection({
206+
host: 'imap.example.com',
207+
username: '<previous.output>',
208+
password: 'literal-password',
209+
})
210+
).toThrowError(
211+
expect.objectContaining<Partial<ImapConnectionPolicyError>>({
212+
name: 'ImapConnectionPolicyError',
213+
code: 'context',
214+
})
215+
)
216+
})
217+
185218
it('reauthorizes requested references on every resolution and fails closed after revocation', async () => {
186219
environmentUtilsMockFns.mockResolveEffectiveEnvironmentVariables
187220
.mockResolvedValueOnce({

apps/sim/lib/imap/connection.server.ts

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { ImapFlow } from 'imapflow'
22
import { validateDatabaseHost } from '@/lib/core/security/input-validation.server'
33
import { resolveEffectiveEnvironmentVariables } from '@/lib/environment/utils'
4+
import { containsReference } from '@/lib/workflows/sanitization/references'
45

56
const EXACT_ENVIRONMENT_REFERENCE = /^\{\{\s*([A-Za-z_][A-Za-z0-9_]*)\s*\}\}$/
67

@@ -27,8 +28,8 @@ export interface ResolvedImapConnection {
2728
password: string
2829
}
2930

30-
function containsTemplateDelimiter(value: string): boolean {
31-
return value.includes('{{') || value.includes('}}')
31+
function containsUnresolvedReference(value: string): boolean {
32+
return value.includes('{{') || value.includes('}}') || containsReference(value)
3233
}
3334

3435
export function hasImapEnvironmentReferences(input: ImapConnectionInput): boolean {
@@ -41,9 +42,10 @@ function normalizeConnection(input: ImapConnectionInput): ResolvedImapConnection
4142
const host = typeof input.host === 'string' ? input.host.trim() : ''
4243
const username = typeof input.username === 'string' ? input.username : ''
4344
const password = typeof input.password === 'string' ? input.password : ''
44-
const port = input.port === undefined || input.port === '' ? 993 : Number(input.port)
45+
const port =
46+
input.port === null || input.port === undefined || input.port === '' ? 993 : Number(input.port)
4547
const secure =
46-
input.secure === undefined || input.secure === ''
48+
input.secure === null || input.secure === undefined || input.secure === ''
4749
? true
4850
: typeof input.secure === 'string'
4951
? input.secure.toLowerCase() === 'true'
@@ -77,7 +79,7 @@ export async function resolveImapConnectionForActor(input: {
7779
if (typeof value !== 'string') return []
7880
const match = EXACT_ENVIRONMENT_REFERENCE.exec(value)
7981
if (match) return [match[1]]
80-
if (containsTemplateDelimiter(value)) throw new ImapConnectionPolicyError('context')
82+
if (containsUnresolvedReference(value)) throw new ImapConnectionPolicyError('context')
8183
return []
8284
})
8385
),
@@ -119,7 +121,7 @@ export function normalizeResolvedImapConnection(
119121

120122
export function normalizeLiteralImapConnection(input: ImapConnectionInput): ResolvedImapConnection {
121123
for (const value of [input.host, input.port, input.secure, input.username, input.password]) {
122-
if (typeof value === 'string' && containsTemplateDelimiter(value)) {
124+
if (typeof value === 'string' && containsUnresolvedReference(value)) {
123125
throw new ImapConnectionPolicyError('context')
124126
}
125127
}
Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
5+
6+
const { mockFetch, mockResolveCredentialBundle, mockResolveCloudId } = vi.hoisted(() => ({
7+
mockFetch: vi.fn(),
8+
mockResolveCredentialBundle: vi.fn(),
9+
mockResolveCloudId: vi.fn(),
10+
}))
11+
12+
vi.mock('@/lib/selectors/server/providers/credential-bundle', () => ({
13+
resolveSelectorCredentialBundle: mockResolveCredentialBundle,
14+
}))
15+
16+
vi.mock('@/lib/selectors/server/providers/atlassian', () => ({
17+
resolveSelectorAtlassianCloudId: mockResolveCloudId,
18+
}))
19+
20+
import { SelectorOptionsUnavailableError } from '@/lib/selectors/server/errors'
21+
import { createSelectorProtectedValues } from '@/lib/selectors/server/protected-values'
22+
import { confluenceSelectorAttachments } from '@/lib/selectors/server/providers/confluence'
23+
import type { ExecuteServerSelectorArgs } from '@/lib/selectors/server/types'
24+
25+
function pageDetailArgs(): ExecuteServerSelectorArgs {
26+
return {
27+
selectorKey: 'confluence.pages',
28+
context: { oauthCredential: 'credential-1', domain: 'acme.atlassian.net' },
29+
request: { kind: 'detail', id: 'page-1' },
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+
credential: { suppliedId: 'credential-1' },
35+
references: new Map(),
36+
protectedValues: createSelectorProtectedValues(),
37+
}
38+
}
39+
40+
describe('Confluence server selector adapters', () => {
41+
beforeEach(() => {
42+
vi.clearAllMocks()
43+
vi.stubGlobal('fetch', mockFetch)
44+
mockResolveCredentialBundle.mockResolvedValue({ accessToken: 'server-only-token' })
45+
mockResolveCloudId.mockResolvedValue('cloud-1')
46+
})
47+
48+
afterAll(() => vi.unstubAllGlobals())
49+
50+
it('hydrates page details through the bounded provider reader without requesting page bodies', async () => {
51+
mockFetch.mockResolvedValueOnce(
52+
new Response(JSON.stringify({ id: 'page-1', title: 'Architecture' }), { status: 200 })
53+
)
54+
55+
await expect(
56+
confluenceSelectorAttachments['confluence.pages'].execute(pageDetailArgs())
57+
).resolves.toEqual({
58+
kind: 'detail',
59+
item: { id: 'page-1', label: 'Architecture' },
60+
})
61+
62+
const requestedUrl = String(mockFetch.mock.calls[0]?.[0])
63+
expect(requestedUrl).toBe(
64+
'https://api.atlassian.com/ex/confluence/cloud-1/wiki/api/v2/pages/page-1'
65+
)
66+
expect(requestedUrl).not.toContain('body-format')
67+
})
68+
69+
it('rejects an oversized page detail response before parsing it', async () => {
70+
mockFetch.mockResolvedValueOnce(
71+
new Response('{}', {
72+
status: 200,
73+
headers: { 'content-length': String(16 * 1024 * 1024 + 1) },
74+
})
75+
)
76+
77+
await expect(
78+
confluenceSelectorAttachments['confluence.pages'].execute(pageDetailArgs())
79+
).rejects.toBeInstanceOf(SelectorOptionsUnavailableError)
80+
})
81+
})

apps/sim/lib/selectors/server/providers/confluence.ts

Lines changed: 9 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import {
1313
listSelectorResult,
1414
type ServerSelectorAttachmentMap,
1515
} from '@/lib/selectors/server/types'
16-
import { fetchConfluencePage } from '@/tools/confluence/client'
1716

1817
type ConfluenceSelectorKey = Extract<ServerSelectorKey, 'confluence.spaces' | 'confluence.pages'>
1918

@@ -160,23 +159,16 @@ async function executePages(args: ExecuteServerSelectorArgs) {
160159
if (!/^[A-Za-z0-9_-]{1,255}$/.test(pageId)) {
161160
throw new SelectorContextUnavailableError()
162161
}
163-
let response: Response
164-
try {
165-
response = await fetchConfluencePage({
166-
...auth,
167-
pageId,
162+
const page = await fetchProviderJson<ConfluencePage>(
163+
`https://api.atlassian.com/ex/confluence/${auth.cloudId}/wiki/api/v2/pages/${pageId}`,
164+
{
165+
headers: {
166+
Accept: 'application/json',
167+
Authorization: `Bearer ${auth.accessToken}`,
168+
},
168169
signal: args.signal,
169-
})
170-
} catch {
171-
throw new SelectorOptionsUnavailableError()
172-
}
173-
if (!response.ok) throw new SelectorOptionsUnavailableError()
174-
let page: ConfluencePage
175-
try {
176-
page = (await response.json()) as ConfluencePage
177-
} catch {
178-
throw new SelectorOptionsUnavailableError()
179-
}
170+
}
171+
)
180172
if (!page.id || !page.title) throw new SelectorOptionsUnavailableError()
181173
return detailSelectorResult({ id: page.id, label: page.title })
182174
}

apps/sim/lib/selectors/server/sanitize.test.ts

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,45 @@ describe('sanitizeSelectorResult', () => {
141141
).toThrow(SelectorOptionsUnavailableError)
142142
})
143143

144+
it('rejects protected plaintext in metadata keys without applying the detail exemption', () => {
145+
const protectedValues = createSelectorProtectedValues()
146+
protectedValues.add('resolved-id')
147+
148+
expect(() =>
149+
sanitizeSelectorResult(
150+
{
151+
kind: 'detail',
152+
item: {
153+
id: 'resolved-id',
154+
label: 'resolved-id',
155+
meta: { 'prefix-resolved-id-suffix': null },
156+
},
157+
},
158+
protectedValues,
159+
{ allowedDetailExactProtectedValue: 'resolved-id' }
160+
)
161+
).toThrow(SelectorOptionsUnavailableError)
162+
})
163+
164+
it('preserves allowed metadata keys that shadow object prototype properties', () => {
165+
const meta = Object.create(null) as Record<string, null>
166+
meta.__proto__ = null
167+
168+
const result = sanitizeSelectorResult(
169+
{
170+
kind: 'list',
171+
items: [{ id: 'resource-1', label: 'Resource one', meta }],
172+
},
173+
createSelectorProtectedValues()
174+
)
175+
176+
expect(result.kind).toBe('list')
177+
if (result.kind !== 'list') throw new Error('Expected list selector result')
178+
expect(Object.hasOwn(result.items[0].meta ?? {}, '__proto__')).toBe(true)
179+
expect(result.items[0].meta?.__proto__).toBeNull()
180+
expect(JSON.stringify(result.items[0].meta)).toBe('{"__proto__":null}')
181+
})
182+
144183
it('rejects metadata strings larger than the response contract permits', () => {
145184
expect(() =>
146185
sanitizeSelectorResult(

0 commit comments

Comments
 (0)