From 22277c195073970dd061d6ec545bc31658df7ebf Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 23 Sep 2026 15:03:54 -0700 Subject: [PATCH 1/7] fix(execution): resolve stored file references only for workspace members Workflow file inputs can reference stored files by id, key, or internal URL, and every resolved file is added to the run's explicit file grants. Any caller could use this, so an anonymous public-API, public MCP, chat, or webhook caller could pull another workflow's run files or workspace files into a run and receive a presigned URL for them. Execution now derives a stored-file reference scope from the run's principal: authorized member principals (session, personal/workspace API key, OAuth token, delegated) resolve workspace-wide; system principals may only reference files already stored under the current execution, which keeps chat and webhook uploads working. The default is the restricted scope, so a new caller fails closed. Input-format defaults are workflow-authored and still resolve workspace-wide. Outside workspace scope, an upload whose URL is an internal file URL is resolved as the stored reference it names rather than downloaded with the run user's access, and a key from another execution is refused before it is looked up, so the refusal does not reveal whether the file exists. --- apps/sim/lib/execution/files.test.ts | 236 ++++++++++++++++-- apps/sim/lib/execution/files.ts | 157 +++++++++--- .../payloads/materialization.server.ts | 8 +- .../workflows/executor/execution-core.test.ts | 91 +++++++ .../lib/workflows/executor/execution-core.ts | 5 +- 5 files changed, 448 insertions(+), 49 deletions(-) diff --git a/apps/sim/lib/execution/files.test.ts b/apps/sim/lib/execution/files.test.ts index 3f81127a73b..78a6fc1da07 100644 --- a/apps/sim/lib/execution/files.test.ts +++ b/apps/sim/lib/execution/files.test.ts @@ -1,4 +1,5 @@ /** @vitest-environment node */ +import type { WorkflowExecutionPrincipal } from '@sim/auth/principal' import { beforeEach, describe, expect, it, vi } from 'vitest' import type { SerializedBlock } from '@/serializer/types' @@ -8,6 +9,7 @@ const mocks = vi.hoisted(() => ({ byKey: vi.fn(), presign: vi.fn(), readWorkspace: vi.fn(), + download: vi.fn(), })) vi.mock('@/lib/uploads/contexts/execution', () => ({ uploadExecutionFile: mocks.upload })) vi.mock('@/lib/uploads/server/metadata', () => ({ @@ -17,6 +19,7 @@ vi.mock('@/lib/uploads/server/metadata', () => ({ vi.mock('@/lib/uploads/core/storage-service', () => ({ generatePresignedDownloadUrl: mocks.presign, })) +vi.mock('@/lib/uploads/utils/file-utils.server', () => ({ downloadFileFromUrl: mocks.download })) vi.mock('@/lib/core/network/resource-scope.server', () => ({ withResourceOutboundScope: (_: unknown, fn: () => unknown) => fn(), })) @@ -31,7 +34,11 @@ vi.mock( }) ) -import { processExecutionFiles, processInputFileFields } from '@/lib/execution/files' +import { + getStoredFileReferenceScope, + processExecutionFiles, + processInputFileFields, +} from '@/lib/execution/files' import { assertUserFileContentAccess } from '@/lib/execution/payloads/materialization.server' import { StartBlockPath } from '@/lib/workflows/triggers/triggers' import { buildStartBlockOutput } from '@/executor/utils/start-block' @@ -102,7 +109,9 @@ describe('workflow input files', () => { scope, 'request', 'actor', - 'selected' + 'selected', + undefined, + 'workspace' ) expect(result).toEqual({ ...input, @@ -127,7 +136,8 @@ describe('workflow input files', () => { [existing, { type: 'file', name: 'new.txt', data: 'data:text/plain;base64,YWJj' }, existing], scope, 'request', - 'actor' + 'actor', + 'workspace' ) expect(files.map((file) => file.id)).toEqual([stored.id, 'new', stored.id]) expect(mocks.upload).toHaveBeenCalledTimes(1) @@ -140,7 +150,13 @@ describe('workflow input files', () => { ) }) it('resolves an ID-only reference against active workspace metadata', async () => { - const result = await processExecutionFiles({ id: 'metadata-id' }, scope, 'request', 'actor') + const result = await processExecutionFiles( + { id: 'metadata-id' }, + scope, + 'request', + 'actor', + 'workspace' + ) expect(result[0]).toMatchObject({ id: 'metadata-id', name: 'report.pdf', size: 24 }) expect(mocks.byId).toHaveBeenCalledWith('metadata-id') }) @@ -150,7 +166,8 @@ describe('workflow input files', () => { { ...file, url: `/api/files/serve/s3/${encodeURIComponent(key)}?context=workspace` }, scope, 'request', - 'actor' + 'actor', + 'workspace' ) expect(mocks.byKey).toHaveBeenCalledWith(key) expect(mocks.byId).not.toHaveBeenCalled() @@ -184,7 +201,9 @@ describe('workflow input files', () => { scope, 'request', 'actor', - 'start' + 'start', + undefined, + 'workspace' ) const output = buildStartBlockOutput({ resolution: { blockId: block.id, block, path: StartBlockPath.UNIFIED }, @@ -211,7 +230,9 @@ describe('workflow input files', () => { scope, 'request', 'actor', - 'start' + 'start', + undefined, + 'workspace' ) expect( buildStartBlockOutput({ @@ -228,9 +249,9 @@ describe('workflow input files', () => { { ...stored, key: 'workspace/foreign/file.pdf' }, ])('rejects missing, deleted, and foreign file bindings before signing', async (record) => { mocks.byKey.mockResolvedValue(record) - await expect(processExecutionFiles([existing], scope, 'request', 'actor')).rejects.toThrow( - 'File not found in this workspace' - ) + await expect( + processExecutionFiles([existing], scope, 'request', 'actor', 'workspace') + ).rejects.toThrow('File not found in this workspace') expect(mocks.presign).not.toHaveBeenCalled() }) it('grants only successfully resolved declared and reserved file inputs, not arbitrary JSON', async () => { @@ -246,7 +267,8 @@ describe('workflow input files', () => { 'request', 'actor', 'start', - granted + granted, + 'workspace' ) expect(granted).toHaveBeenCalledTimes(2) expect(granted.mock.calls.map(([file]) => file.key)).toEqual([stored.key, stored.key]) @@ -264,7 +286,8 @@ describe('workflow input files', () => { 'request', 'actor', 'start', - granted + granted, + 'workspace' ) ).rejects.toThrow('File not found') expect(granted).not.toHaveBeenCalled() @@ -272,7 +295,7 @@ describe('workflow input files', () => { it('normalizes Mothership attachment storage context for downstream materialization', async () => { mocks.byKey.mockResolvedValue({ ...stored, context: 'mothership' }) - const [file] = await processExecutionFiles([existing], scope, 'request', 'actor') + const [file] = await processExecutionFiles([existing], scope, 'request', 'actor', 'workspace') expect(file).toMatchObject({ id: stored.id, context: 'workspace', key: stored.key }) expect(mocks.presign).toHaveBeenCalledWith(stored.key, 'workspace', 300) await assertUserFileContentAccess(file, { @@ -291,7 +314,8 @@ describe('workflow input files', () => { [{ name: 'new.txt', data: 'YWJj', mimeType: 'text/plain' }], scope, 'request', - 'actor' + 'actor', + 'workspace' ) expect(mocks.upload).toHaveBeenCalledWith( scope, @@ -309,7 +333,9 @@ describe('workflow input files', () => { scope, 'request', 'actor', - 'start' + 'start', + undefined, + 'workspace' ) expect(nested).toMatchObject({ input: { @@ -354,8 +380,186 @@ describe('workflow input files', () => { scope, 'request', 'actor', - 'start' + 'start', + undefined, + 'workspace' ) expect(mocks.byKey).toHaveBeenCalledWith(existing.key) }) + + describe('stored references from callers outside the workspace', () => { + const priorRun = { + ...stored, + key: 'execution/11111111-1111-4111-8111-111111111111/other-workflow/old-run/secret.txt', + context: 'execution', + } + const references = [ + { key: priorRun.key }, + { id: 'metadata-id' }, + { + id: 'client-id', + url: `/api/files/serve/s3/${encodeURIComponent(priorRun.key)}?context=execution`, + }, + ] + + it.each([ + ['a public API principal', 'execution' as const], + ['an omitted scope', undefined], + ])( + 'rejects key, ID, and internal URL references from %s and grants nothing', + async (_, scopeOption) => { + for (const record of [priorRun, stored]) { + mocks.byKey.mockResolvedValue(record) + mocks.byId.mockResolvedValue(record) + for (const reference of references) { + const granted = vi.fn() + await expect( + processInputFileFields( + { documents: [reference] }, + [trigger('start', 'documents')], + scope, + 'request', + 'actor', + 'start', + granted, + scopeOption + ) + ).rejects.toThrow('Stored file references require workspace member access') + expect(granted).not.toHaveBeenCalled() + } + } + expect(mocks.presign).not.toHaveBeenCalled() + } + ) + + it('does not reveal whether a rejected reference exists', async () => { + mocks.byKey.mockResolvedValue(null) + await expect( + processExecutionFiles([{ key: priorRun.key }], scope, 'request') + ).rejects.toThrow('Stored file references require workspace member access') + }) + + it('refuses a key from another execution before looking it up', async () => { + await expect( + processExecutionFiles([{ key: priorRun.key }], scope, 'request') + ).rejects.toThrow('Stored file references require workspace member access') + expect(mocks.byKey).not.toHaveBeenCalled() + }) + + it('treats an internal file URL upload as a stored reference, not a download', async () => { + mocks.download.mockResolvedValue(Buffer.from('secret')) + await expect( + processExecutionFiles( + [ + { + type: 'url', + name: 'secret.txt', + data: `/api/files/serve/s3/${encodeURIComponent(priorRun.key)}?context=execution`, + }, + ], + scope, + 'request', + 'actor' + ) + ).rejects.toThrow('Stored file references require workspace member access') + expect(mocks.download).not.toHaveBeenCalled() + expect(mocks.upload).not.toHaveBeenCalled() + }) + + it('keeps downloading internal file URL uploads for a workspace member', async () => { + mocks.download.mockResolvedValue(Buffer.from('bytes')) + await processExecutionFiles( + [ + { + type: 'url', + name: 'secret.txt', + data: `/api/files/serve/s3/${encodeURIComponent(priorRun.key)}?context=execution`, + }, + ], + scope, + 'request', + 'actor', + 'workspace' + ) + expect(mocks.download).toHaveBeenCalledTimes(1) + expect(mocks.upload).toHaveBeenCalledTimes(1) + }) + + it('still resolves files this execution stored before handing its input on', async () => { + const ownKey = `execution/${scope.workspaceId}/${scope.workflowId}/${scope.executionId}/upload.txt` + mocks.byKey.mockResolvedValue({ ...priorRun, key: ownKey }) + const granted = vi.fn() + await processInputFileFields( + { files: [{ id: 'upload', key: ownKey }] }, + [trigger('start', 'other')], + scope, + 'request', + 'actor', + 'start', + granted + ) + expect(granted.mock.calls.map(([file]) => file.key)).toEqual([ownKey]) + }) + + it('resolves the same prior-run reference for a workspace member and grants it', async () => { + mocks.byKey.mockResolvedValue(priorRun) + const granted = vi.fn() + await processInputFileFields( + { documents: [{ key: priorRun.key }] }, + [trigger('start', 'documents')], + scope, + 'request', + 'actor', + 'start', + granted, + 'workspace' + ) + expect(granted.mock.calls.map(([file]) => file.key)).toEqual([priorRun.key]) + }) + }) + + describe('getStoredFileReferenceScope', () => { + const workspaceId = scope.workspaceId + const workflowId = scope.workflowId + const delegation = { + workspaceId, + delegationId: 'delegation', + audience: 'workflow-execution', + issuedAt: new Date(), + expiresAt: new Date(), + } + it.each([ + { kind: 'session', userId: 'user', sessionId: 'session' }, + { kind: 'personal_api_key', userId: 'user', keyId: 'key' }, + { + kind: 'oauth_access_token', + userId: 'user', + clientId: 'client', + tokenId: 'token', + scopes: [], + expiresAt: new Date(), + }, + { kind: 'workspace_api_key', workspaceId, keyId: 'key' }, + { kind: 'delegated', serviceId: 'copilot', subjectUserId: 'user', ...delegation }, + ])('lets authorized member principal $kind reference workspace files', (principal) => { + expect(getStoredFileReferenceScope(principal)).toBe('workspace') + }) + it.each([ + { kind: 'system', serviceId: 'public_api', workspaceId, workflowId }, + { kind: 'system', serviceId: 'internal', workspaceId, workflowId }, + { kind: 'system', serviceId: 'schedule', workspaceId, workflowId }, + { kind: 'system', serviceId: 'table', workspaceId, workflowId }, + { kind: 'system', serviceId: 'chat', workspaceId, workflowId }, + { + kind: 'system', + serviceId: 'webhook', + workspaceId, + workflowId, + webhookId: 'webhook', + provider: 'generic', + }, + ])('limits system principal $serviceId to its own execution files', (principal) => { + expect(getStoredFileReferenceScope(principal)).toBe('execution') + }) + }) }) diff --git a/apps/sim/lib/execution/files.ts b/apps/sim/lib/execution/files.ts index 4257f72b9bb..38a7604fb2b 100644 --- a/apps/sim/lib/execution/files.ts +++ b/apps/sim/lib/execution/files.ts @@ -1,7 +1,9 @@ +import type { WorkflowExecutionPrincipal } from '@sim/auth/principal' import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' import { isPlainRecord } from '@sim/utils/object' import { withResourceOutboundScope } from '@/lib/core/network/resource-scope.server' +import { getExecutionKeyParts } from '@/lib/execution/payloads/access-keys' import { uploadExecutionFile } from '@/lib/uploads/contexts/execution' import { generatePresignedDownloadUrl } from '@/lib/uploads/core/storage-service' import { getFileMetadataById, getFileMetadataByKey } from '@/lib/uploads/server/metadata' @@ -23,6 +25,104 @@ const logger = createLogger('ExecutionFiles') const MAX_FILE_SIZE = 20 * 1024 * 1024 // 20MB +/** + * Which stored files an input reference (`id`, `key`, or internal file URL) may resolve to. + * + * - `workspace`: any live file in the executing workspace. Only for callers acting as an + * authorized workspace member, since a resolved file becomes readable by the run. + * - `execution`: only files already stored under this execution, such as the uploads a chat + * or webhook entry point makes before handing its input on. Every other reference is + * rejected, so a caller outside the workspace cannot pull in files it names by key or ID. + */ +export type StoredFileReferenceScope = 'workspace' | 'execution' + +const STORED_FILE_REFERENCE_DENIED = + 'Stored file references require workspace member access; send the file content instead' + +/** + * The stored-file reference scope an execution principal may use. Only principals that the + * entry point authorized as the workspace's own members get `workspace`; system principals + * (public API, public MCP, chat, webhook, schedule, table, internal) never do. + */ +export function getStoredFileReferenceScope( + principal: WorkflowExecutionPrincipal +): StoredFileReferenceScope { + switch (principal.kind) { + case 'session': + case 'personal_api_key': + case 'oauth_access_token': + case 'workspace_api_key': + case 'delegated': + return 'workspace' + default: + return 'execution' + } +} + +function isCurrentExecutionFileKey( + key: string, + executionContext: { workspaceId: string; workflowId: string; executionId: string } +): boolean { + const parts = getExecutionKeyParts(key) + return ( + parts?.workspaceId === executionContext.workspaceId && + parts.workflowId === executionContext.workflowId && + parts.executionId === executionContext.executionId + ) +} + +/** + * Resolves a reference to an already-stored file under `storedFileScope`. Outside `workspace` + * scope only this execution's own files resolve, and a key naming any other file is refused + * before it is looked up, so the refusal reveals nothing about whether that file exists. + */ +async function resolveStoredFileReference( + reference: { key?: string; id?: string }, + executionContext: { workspaceId: string; workflowId: string; executionId: string }, + storedFileScope: StoredFileReferenceScope +): Promise { + if ( + storedFileScope !== 'workspace' && + reference.key && + !isCurrentExecutionFileKey(reference.key, executionContext) + ) { + throw new Error(STORED_FILE_REFERENCE_DENIED) + } + const record = reference.key + ? await getFileMetadataByKey(reference.key) + : reference.id + ? await getFileMetadataById(reference.id) + : null + if ( + !record || + record.deletedAt || + record.workspaceId !== executionContext.workspaceId || + extractWorkspaceIdFromStorageKey(record.key) !== executionContext.workspaceId || + (record.context !== 'workspace' && + record.context !== 'mothership' && + record.context !== 'execution') + ) { + throw new Error( + storedFileScope === 'workspace' + ? 'File not found in this workspace' + : STORED_FILE_REFERENCE_DENIED + ) + } + if (storedFileScope !== 'workspace' && !isCurrentExecutionFileKey(record.key, executionContext)) { + throw new Error(STORED_FILE_REFERENCE_DENIED) + } + const storageContext = inferContextFromKey(record.key) + return { + id: record.id, + name: record.originalName, + type: record.contentType, + size: getWorkspaceFileSize(record), + key: record.key, + context: storageContext, + url: await generatePresignedDownloadUrl(record.key, storageContext, 5 * 60), + } +} + /** * Process a single file for workflow execution - handles base64 ('file' type) and URL downloads ('url' type) */ @@ -30,7 +130,8 @@ export async function processExecutionFile( fileInput: unknown, executionContext: { workspaceId: string; workflowId: string; executionId: string }, requestId: string, - userId?: string + userId?: string, + storedFileScope: StoredFileReferenceScope = 'execution' ): Promise { const parsed = workflowFileInputSchema.safeParse(fileInput) if (!parsed.success) throw new Error('Invalid workflow file input') @@ -46,28 +147,7 @@ export async function processExecutionFile( key = parseInternalFileUrl(candidate.url).key } const id = 'id' in candidate && typeof candidate.id === 'string' ? candidate.id : undefined - const record = key ? await getFileMetadataByKey(key) : id ? await getFileMetadataById(id) : null - if ( - !record || - record.deletedAt || - record.workspaceId !== executionContext.workspaceId || - extractWorkspaceIdFromStorageKey(record.key) !== executionContext.workspaceId || - (record.context !== 'workspace' && - record.context !== 'mothership' && - record.context !== 'execution') - ) { - throw new Error('File not found in this workspace') - } - const storageContext = inferContextFromKey(record.key) - return { - id: record.id, - name: record.originalName, - type: record.contentType, - size: getWorkspaceFileSize(record), - key: record.key, - context: storageContext, - url: await generatePresignedDownloadUrl(record.key, storageContext, 5 * 60), - } + return resolveStoredFileReference({ key, id }, executionContext, storedFileScope) } const upload = 'mimeType' in candidate && typeof candidate.mimeType === 'string' @@ -130,6 +210,13 @@ export async function processExecutionFile( } if (file.type === 'url' && file.data) { + if (storedFileScope !== 'workspace' && isInternalFileUrl(file.data)) { + return resolveStoredFileReference( + { key: parseInternalFileUrl(file.data).key }, + executionContext, + storedFileScope + ) + } const { downloadFileFromUrl } = await import('@/lib/uploads/utils/file-utils.server') const buffer = await withResourceOutboundScope(executionContext, () => downloadFileFromUrl(file.data, { userId }) @@ -163,7 +250,8 @@ export async function processExecutionFiles( fieldValue: unknown, executionContext: { workspaceId: string; workflowId: string; executionId: string }, requestId: string, - userId?: string + userId?: string, + storedFileScope: StoredFileReferenceScope = 'execution' ): Promise { if (fieldValue === undefined || fieldValue === null) return [] if (typeof fieldValue !== 'object') throw new Error('Workflow files must be file objects') @@ -174,7 +262,13 @@ export async function processExecutionFiles( for (const file of files) { try { - const userFile = await processExecutionFile(file, fullContext, requestId, userId) + const userFile = await processExecutionFile( + file, + fullContext, + requestId, + userId, + storedFileScope + ) if (userFile) { uploadedFiles.push(userFile) @@ -218,6 +312,9 @@ function extractInputFormatFromBlock(block: SerializedBlock): ValidatedInputForm /** * Process file fields in workflow input based on the start block's inputFormat * This handles base64 and URL file inputs from API calls + * + * `storedFileScope` bounds stored references supplied by the caller. Defaults declared on the + * trigger's input format are authored by the workflow's own editors and resolve workspace-wide. */ export async function processInputFileFields( input: unknown, @@ -226,7 +323,8 @@ export async function processInputFileFields( requestId: string, userId?: string, triggerBlockId?: string, - onFileResolved?: (file: UserFile) => void + onFileResolved?: (file: UserFile) => void, + storedFileScope: StoredFileReferenceScope = 'execution' ): Promise { if (!input || typeof input !== 'object' || blocks.length === 0) { return input @@ -269,8 +367,8 @@ export async function processInputFileFields( for (const fileField of fileFields) { const nestedInput = isPlainRecord(processedInput.input) ? processedInput.input : undefined const isNested = nestedInput !== undefined && Object.hasOwn(nestedInput, fileField.name) - let fieldValue = - (isNested ? nestedInput[fileField.name] : processedInput[fileField.name]) ?? fileField.value + const callerValue = isNested ? nestedInput[fileField.name] : processedInput[fileField.name] + let fieldValue = callerValue ?? fileField.value if (typeof fieldValue === 'string' && fieldValue.trim()) { try { fieldValue = JSON.parse(fieldValue) @@ -284,7 +382,8 @@ export async function processInputFileFields( fieldValue, executionContext, requestId, - userId + userId, + callerValue === undefined || callerValue === null ? 'workspace' : storedFileScope ) for (const file of uploadedFiles) onFileResolved?.(file) diff --git a/apps/sim/lib/execution/payloads/materialization.server.ts b/apps/sim/lib/execution/payloads/materialization.server.ts index 3bed537a821..93be16c2efb 100644 --- a/apps/sim/lib/execution/payloads/materialization.server.ts +++ b/apps/sim/lib/execution/payloads/materialization.server.ts @@ -232,8 +232,12 @@ function assertExecutionFileScope(key: string, options: ExecutionMaterialization throw new ExecutionFileAccessError() } - // Explicit file grants are minted only after workspace authorization. They can carry - // an input from another workflow/run without granting any neighboring execution files. + /** + * An explicit grant names one exact key the run may read, even from another workflow or run, + * without opening that run's neighboring files. Grants must only be minted for keys the run's + * principal may already read: a stored input reference is granted only when a workspace member + * supplied it (see `getStoredFileReferenceScope`), otherwise only for this execution's own files. + */ if (allowedFileKeys.has(key)) { return } diff --git a/apps/sim/lib/workflows/executor/execution-core.test.ts b/apps/sim/lib/workflows/executor/execution-core.test.ts index 218f9a2450f..df743a3c12f 100644 --- a/apps/sim/lib/workflows/executor/execution-core.test.ts +++ b/apps/sim/lib/workflows/executor/execution-core.test.ts @@ -150,6 +150,18 @@ vi.mock('@/lib/uploads/contexts/execution', () => ({ uploadExecutionFile: uploadWorkflowInputMock, })) +const { storedFileByKeyMock, presignStoredFileMock } = vi.hoisted(() => ({ + storedFileByKeyMock: vi.fn(), + presignStoredFileMock: vi.fn(), +})) +vi.mock('@/lib/uploads/server/metadata', () => ({ + getFileMetadataById: vi.fn(), + getFileMetadataByKey: storedFileByKeyMock, +})) +vi.mock('@/lib/uploads/core/storage-service', () => ({ + generatePresignedDownloadUrl: presignStoredFileMock, +})) + vi.mock('@/serializer', () => ({ Serializer: class { serializeWorkflow = serializeWorkflowMock @@ -446,6 +458,85 @@ describe('executeWorkflowCore terminal finalization sequencing', () => { } ) + it.each([ + { + name: 'rejects for an anonymous public API caller', + principal: { + kind: 'system', + serviceId: 'public_api', + workspaceId: '22222222-2222-4222-8222-222222222222', + workflowId: 'workflow-1', + }, + granted: false, + }, + { + name: 'resolves and grants for a workspace member', + principal: { kind: 'session', userId: 'user-1', sessionId: 'session-1' }, + granted: true, + }, + ] satisfies Array<{ name: string; principal: WorkflowExecutionPrincipal; granted: boolean }>)( + "a prior run's stored file reference $name", + async ({ principal, granted }) => { + const workspaceId = '22222222-2222-4222-8222-222222222222' + const key = `execution/${workspaceId}/other-workflow/old-run/secret.txt` + storedFileByKeyMock.mockResolvedValue({ + id: 'secret', + key, + workspaceId, + context: 'execution', + originalName: 'secret.txt', + contentType: 'text/plain', + sizeBytes: 6, + deletedAt: null, + }) + presignStoredFileMock.mockResolvedValue('https://signed.example.com/secret.txt') + serializeWorkflowMock.mockReturnValue({ + blocks: [ + { + id: 'start-block', + metadata: { id: 'start_trigger' }, + config: { params: { inputFormat: [{ name: 'documents', type: 'file[]' }] } }, + }, + ], + loops: {}, + parallels: {}, + }) + executorExecuteMock.mockResolvedValue({ + success: true, + status: 'completed', + output: {}, + logs: [], + }) + const snapshot = createSnapshot() + const execution = executeWorkflowCore({ + snapshot: { + ...snapshot, + metadata: { + ...snapshot.metadata, + workspaceId, + principal, + triggerBlockId: 'start-block', + }, + input: { documents: [{ key }] }, + } as unknown as ExecutionSnapshot, + callbacks: {}, + loggingSession: loggingSession as unknown as LoggingSession, + }) + if (granted) { + await execution + expect(executorConstructorMock.mock.calls[0]?.[0]?.contextExtensions?.fileKeys).toEqual([ + key, + ]) + } else { + await expect(execution).rejects.toThrow( + 'Stored file references require workspace member access' + ) + expect(executorConstructorMock).not.toHaveBeenCalled() + expect(presignStoredFileMock).not.toHaveBeenCalled() + } + } + ) + it('begins connecting the signal subscriber synchronously, before the first await', async () => { const executionPromise = executeWorkflowCore({ snapshot: createSnapshot() as unknown as ExecutionSnapshot, diff --git a/apps/sim/lib/workflows/executor/execution-core.ts b/apps/sim/lib/workflows/executor/execution-core.ts index b19dda41956..420be743ab9 100644 --- a/apps/sim/lib/workflows/executor/execution-core.ts +++ b/apps/sim/lib/workflows/executor/execution-core.ts @@ -25,7 +25,7 @@ import { withDatabaseReadRetry } from '@/lib/db/read-retry' import { getExecutionEnvironment } from '@/lib/environment/utils' import { clearExecutionCancellation } from '@/lib/execution/cancellation' import { connectExecutionSignalHub } from '@/lib/execution/execution-signal' -import { processInputFileFields } from '@/lib/execution/files' +import { getStoredFileReferenceScope, processInputFileFields } from '@/lib/execution/files' import { warmLargeValueRefs } from '@/lib/execution/payloads/hydration' import { parseLargeExecutionValue } from '@/lib/execution/payloads/large-execution-value' import type { LoggingSession } from '@/lib/logs/execution/logging-session' @@ -759,7 +759,8 @@ async function executeWorkflowCoreImpl( resolvedTriggerBlockId, (file) => { if (file.key) inputFileKeys.add(file.key) - } + }, + getStoredFileReferenceScope(metadata.principal) ) // Resolve stopAfterBlockId for loop/parallel containers to their sentinel-end IDs From 338559483cca20dbb15e3bf9a1ea2a63c4eb875e Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 23 Sep 2026 15:14:27 -0700 Subject: [PATCH 2/7] fix(mothership): withhold unregistered table secrets from sim_cli results Table rows read through sim_cli reached the model without their persisted secret provenance, so stored secrets in cells were never redacted. Row use cases now report the provenance of the rows they return to an observing transport (mirroring the workspace-file delivery observer), and the agent CLI table transport imports it into the tool call's registry, answers 503 without a registry, and marks the registry incomplete when a row-bearing table route returns without reporting provenance. Provenance reported by detached work after the call settles is ignored. Export download links are refused outright: a signed link to the whole table as plaintext CSV cannot carry provenance once fetched. Run-state and enrichment error text (runState.error, blockErrors, enrichment provider errors) is captured from executor output without its secret provenance, so a read that returns any of it is withheld as well; reads whose run state carries no error text are unaffected. --- .../resolved-secret-trace-registry.test.ts | 1 + .../utils/resolved-secret-trace-registry.ts | 5 + .../api/server/routes/in-process-transport.ts | 13 +- apps/sim/lib/mothership/agent-cli/index.ts | 11 +- .../agent-cli/table-read-transport.test.ts | 280 ++++++++++++++++++ .../agent-cli/table-read-transport.ts | 130 ++++++++ .../application/row-delivery-observer.ts | 40 +++ apps/sim/lib/table/application/rows.test.ts | 272 ++++++++++++++++- apps/sim/lib/table/application/rows.ts | 126 +++++++- 9 files changed, 856 insertions(+), 22 deletions(-) create mode 100644 apps/sim/lib/mothership/agent-cli/table-read-transport.test.ts create mode 100644 apps/sim/lib/mothership/agent-cli/table-read-transport.ts create mode 100644 apps/sim/lib/table/application/row-delivery-observer.ts diff --git a/apps/sim/executor/utils/resolved-secret-trace-registry.test.ts b/apps/sim/executor/utils/resolved-secret-trace-registry.test.ts index 333b3100eac..9c3b81a8756 100644 --- a/apps/sim/executor/utils/resolved-secret-trace-registry.test.ts +++ b/apps/sim/executor/utils/resolved-secret-trace-registry.test.ts @@ -1798,6 +1798,7 @@ describe('incompleteness diagnostics', () => { 'client-tool-content-unavailable', 'knowledge-result-provenance-unavailable', 'table-result-provenance-unavailable', + 'table-run-state-provenance-unavailable', 'mounted-file-provenance-unavailable', 'workspace-file-provenance-unknown', 'file-source-unidentified', diff --git a/apps/sim/executor/utils/resolved-secret-trace-registry.ts b/apps/sim/executor/utils/resolved-secret-trace-registry.ts index cc0bb3bf1fc..e9158202c07 100644 --- a/apps/sim/executor/utils/resolved-secret-trace-registry.ts +++ b/apps/sim/executor/utils/resolved-secret-trace-registry.ts @@ -70,6 +70,11 @@ export type ResolvedSecretIncompletenessReason = | 'knowledge-row-missing' | 'knowledge-row-content-mismatch' | 'table-result-provenance-unavailable' + /** + * A table result carried run-state or enrichment error text, captured from executor output that + * can hold resolved secret plaintext, with no provenance persisted beside it. + */ + | 'table-run-state-provenance-unavailable' | 'mounted-file-provenance-unavailable' | 'workspace-file-provenance-unknown' | 'file-source-unidentified' diff --git a/apps/sim/lib/api/server/routes/in-process-transport.ts b/apps/sim/lib/api/server/routes/in-process-transport.ts index 1383962a359..052837c58e7 100644 --- a/apps/sim/lib/api/server/routes/in-process-transport.ts +++ b/apps/sim/lib/api/server/routes/in-process-transport.ts @@ -24,6 +24,8 @@ type RouteHandler = ( ) => Promise interface CompiledRoute { + /** The generated route pattern, e.g. `/api/v2/tables/{tableId}/rows`. */ + pattern: string regex: RegExp params: string[] /** Literal segments — a more specific pattern wins over a parameterized one. */ @@ -32,6 +34,7 @@ interface CompiledRoute { } interface MatchedRoute { + pattern: string params: Record literals: number load: () => Promise @@ -52,7 +55,13 @@ const COMPILED: CompiledRoute[] = V2_ROUTES.map((route) => { return '([^/]+)' }) .join('/') - return { regex: new RegExp(`^${source}$`), params, literals, load: route.load } + return { + pattern: route.pattern, + regex: new RegExp(`^${source}$`), + params, + literals, + load: route.load, + } }) export function matchV2Route(pathname: string): MatchedRoute | null { @@ -65,7 +74,7 @@ export function matchV2Route(pathname: string): MatchedRoute | null { route.params.forEach((name, index) => { params[name] = decodeURIComponent(match[index + 1] ?? '') }) - best = { params, literals: route.literals, load: route.load } + best = { pattern: route.pattern, params, literals: route.literals, load: route.load } } return best } diff --git a/apps/sim/lib/mothership/agent-cli/index.ts b/apps/sim/lib/mothership/agent-cli/index.ts index f4c87c94011..7f8b904b0b3 100644 --- a/apps/sim/lib/mothership/agent-cli/index.ts +++ b/apps/sim/lib/mothership/agent-cli/index.ts @@ -11,6 +11,7 @@ import { runCli } from '@/lib/mothership/agent-cli/run-cli' import { createScopedCliTransport } from '@/lib/mothership/agent-cli/scoped-transport' import { executeAgentCliService } from '@/lib/mothership/agent-cli/services' import { applySink } from '@/lib/mothership/agent-cli/sink' +import { createTableReadTransport } from '@/lib/mothership/agent-cli/table-read-transport' import { createTracedCliTransport } from '@/lib/mothership/agent-cli/traced-transport' import { createWorkbenchFileProvenance } from '@/lib/mothership/agent-cli/workbench-file-provenance' import { resolveInvocationWorkspace } from '@/lib/mothership/application/workspace-target' @@ -80,10 +81,14 @@ async function executeBoundAgentCliRequest( const files = sessionKey ? createWorkbenchFileProvenance({ ...context, sessionKey }) : undefined const reads = createFileReadTransport({ endpoint, - transport: createTracedCliTransport( + transport: createTableReadTransport({ endpoint, - createScopedCliTransport(endpoint, invocationIdentity) - ), + transport: createTracedCliTransport( + endpoint, + createScopedCliTransport(endpoint, invocationIdentity) + ), + registry: context.resolvedSecretTraceRegistry, + }), userId: context.userId, invocation: invocationIdentity, registry: context.resolvedSecretTraceRegistry, diff --git a/apps/sim/lib/mothership/agent-cli/table-read-transport.test.ts b/apps/sim/lib/mothership/agent-cli/table-read-transport.test.ts new file mode 100644 index 00000000000..80dc4ebf56c --- /dev/null +++ b/apps/sim/lib/mothership/agent-cli/table-read-transport.test.ts @@ -0,0 +1,280 @@ +/** @vitest-environment node */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const mocks = vi.hoisted(() => ({ scoped: vi.fn(), decrypt: vi.fn() })) +vi.mock('@/lib/core/security/encryption', () => ({ decryptSecret: mocks.decrypt })) +vi.mock('@/lib/mothership/agent-cli/scoped-transport', () => ({ + createScopedCliTransport: () => mocks.scoped, +})) +vi.mock('@/lib/mothership/application/workspace-target', () => ({ + resolveInvocationWorkspace: async (owner: { userId: string }, workspaceId?: string) => ({ + workspaceId: workspaceId ?? 'workspace', + userId: owner.userId, + }), +})) +vi.mock('@/lib/execution/remote-sandbox/session-files', () => ({ + SESSION_SANDBOX_HOME: '/home/user', + readSessionSandboxFile: vi.fn(), + writeSessionSandboxFile: vi.fn(), +})) +vi.mock('@/lib/execution/remote-sandbox/session-file-snapshot', () => ({ + openSessionFileSnapshot: vi.fn(), +})) + +import { V2_ROUTES } from '@/lib/api/server/routes/v2-route-table.generated' +import { + createTableReadTransport, + TABLE_ROUTES_WITHOUT_ROW_DATA, +} from '@/lib/mothership/agent-cli/table-read-transport' +import { inspectToolResultForCopilot } from '@/lib/mothership/request/tools/resolved-secret-result' +import { executeSimCli } from '@/lib/mothership/tools/handlers/sim-cli' +import { reportTableRowDelivery } from '@/lib/table/application/row-delivery-observer' +import { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry' + +const SECRET = 'PRIVATE_TABLE_CELL_CANARY_FOR_LOCAL_TEST' +const scope = { userId: 'reader', workspaceId: 'workspace' } +const endpoint = 'https://sim.test' +const rowUrl = `${endpoint}/api/v2/tables/table/rows/row?workspaceId=workspace` +const rowBody = { data: { id: 'row', data: { token: SECRET } } } + +function registry() { + return new ResolvedSecretTraceRegistry([], scope) +} + +function secretProvenance() { + const source = new ResolvedSecretTraceRegistry( + [{ name: 'TABLE_SECRET', plaintext: SECRET, encryptedValue: 'fixture-ciphertext' }], + scope + ) + source.recordResolved('TABLE_SECRET', SECRET, { propagated: true }) + return source.exportProvenance() +} + +/** Stands in for a v2 table route whose use case reports the rows it returns. */ +async function deliveringRoute() { + await reportTableRowDelivery(secretProvenance(), [{ col_token: SECRET }]) + return Response.json(rowBody) +} + +function projected(output: string, trace: ResolvedSecretTraceRegistry) { + return JSON.stringify(inspectToolResultForCopilot({ success: true, output }, trace, 'sim_cli')) +} + +describe('table provenance at the CLI and model-result boundary', () => { + beforeEach(() => { + vi.clearAllMocks() + mocks.decrypt.mockResolvedValue({ decrypted: SECRET }) + }) + + it('activates reported row provenance so the model projection redacts the cell', async () => { + const trace = registry() + const inner = vi.fn(deliveringRoute) + const response = await createTableReadTransport({ + endpoint, + transport: inner, + registry: trace, + })(rowUrl) + + expect(response.status).toBe(200) + const output = await response.text() + expect(output).toBe(JSON.stringify(rowBody)) + expect(trace.isPermanentlyIncomplete()).toBe(false) + expect(projected(output, trace)).not.toContain(SECRET) + }) + + it('withholds a row-bearing result that reported no provenance', async () => { + const trace = registry() + const response = await createTableReadTransport({ + endpoint, + transport: async () => Response.json(rowBody), + registry: trace, + })(rowUrl) + + expect(response.status).toBe(200) + expect(trace.isPermanentlyIncomplete()).toBe(true) + expect(projected(await response.text(), trace)).not.toContain(SECRET) + }) + + it('withholds a result whose run state carries error text without provenance', async () => { + const trace = registry() + const response = await createTableReadTransport({ + endpoint, + transport: async () => { + await reportTableRowDelivery(secretProvenance(), [{ col_token: SECRET }], { + unprovenancedErrorText: true, + }) + return Response.json(rowBody) + }, + registry: trace, + })(rowUrl) + + expect(response.status).toBe(200) + expect(trace.isPermanentlyIncomplete()).toBe(true) + expect(trace.getIncompletenessDiagnostics()?.reasons).toEqual([ + 'table-run-state-provenance-unavailable', + ]) + }) + + it('keeps a delivered result without run-state error text complete', async () => { + const trace = registry() + await createTableReadTransport({ + endpoint, + transport: async () => { + await reportTableRowDelivery(secretProvenance(), [{ col_token: SECRET }], { + unprovenancedErrorText: false, + }) + return Response.json(rowBody) + }, + registry: trace, + })(rowUrl) + + expect(trace.isPermanentlyIncomplete()).toBe(false) + }) + + it.each(['GET', 'HEAD'])( + 'refuses to return a table export download link (%s)', + async (method) => { + const trace = registry() + const inner = vi.fn(async () => + Response.json({ data: { url: 'https://signed.test/export.csv' } }) + ) + const response = await createTableReadTransport({ + endpoint, + transport: inner, + registry: trace, + })(`${endpoint}/api/v2/tables/table/exports/export/download?workspaceId=workspace`, { + method, + }) + + expect(response.status).toBe(403) + expect(await response.text()).not.toContain('signed.test') + expect(inner).not.toHaveBeenCalled() + expect(trace.isPermanentlyIncomplete()).toBe(false) + } + ) + + it('ignores provenance that detached work reports after the call settles', async () => { + const trace = registry() + let release!: () => void + const released = new Promise((resolve) => { + release = resolve + }) + let detached: Promise | undefined + await createTableReadTransport({ + endpoint, + transport: async () => { + detached = released.then(() => + reportTableRowDelivery(secretProvenance(), [{ col_token: SECRET }]) + ) + return Response.json({ error: { message: 'Row not found' } }, { status: 404 }) + }, + registry: trace, + })(rowUrl) + + release() + await detached + expect(projected(JSON.stringify(rowBody), trace)).toContain(SECRET) + }) + + it('refuses row reads without a registry instead of returning plaintext', async () => { + const inner = vi.fn(deliveringRoute) + const response = await createTableReadTransport({ endpoint, transport: inner })(rowUrl) + + expect(response.status).toBe(503) + expect(await response.text()).not.toContain(SECRET) + expect(inner).not.toHaveBeenCalled() + }) + + it('keeps a failed row read from poisoning the turn', async () => { + const trace = registry() + const response = await createTableReadTransport({ + endpoint, + transport: async () => + Response.json({ error: { message: 'Row not found' } }, { status: 404 }), + registry: trace, + })(rowUrl) + + expect(response.status).toBe(404) + expect(trace.isPermanentlyIncomplete()).toBe(false) + }) + + it.each([ + ['GET', `${endpoint}/api/v2/files/file?workspaceId=workspace`], + ['GET', `${endpoint}/api/v2/tables/table?workspaceId=workspace`], + ['POST', `${endpoint}/api/v2/tables/table/rows/search`], + ['DELETE', `${endpoint}/api/v2/tables/table/rows/row?workspaceId=workspace`], + ['GET', 'https://elsewhere.test/api/v2/tables/table/rows/row'], + ])('passes %s %s through untouched without a registry', async (method, url) => { + const upstream = Response.json({ data: { ok: true } }) + const inner = vi.fn(async () => upstream) + const response = await createTableReadTransport({ endpoint, transport: inner })(url, { + method, + }) + + expect(response).toBe(upstream) + expect(inner).toHaveBeenCalledWith(url, { method }) + }) + + it('composes into the sim_cli stack so a table read through the real CLI is redacted', async () => { + mocks.scoped.mockImplementation(deliveringRoute) + const trace = registry() + const result = await executeSimCli( + { + request: { + invocation: { + kind: 'cli', + argv: ['tables', 'rows', 'get', 'table', 'row'], + }, + }, + }, + { + userId: 'reader', + workspaceId: 'workspace', + workflowId: '', + chatId: 'chat', + resolvedSecretTraceRegistry: trace, + } + ) + + expect(mocks.scoped).toHaveBeenCalled() + expect(new URL(String(mocks.scoped.mock.calls[0]?.[0])).pathname).toBe( + '/api/v2/tables/table/rows/row' + ) + expect(JSON.stringify(result.output)).toContain(SECRET) + expect(trace.isPermanentlyIncomplete()).toBe(false) + expect(JSON.stringify(inspectToolResultForCopilot(result, trace, 'sim_cli'))).not.toContain( + SECRET + ) + }) + + /** + * Every v2 table route is either declared row-free or must deliver provenance. + * A new route lands in the row-bearing set by default — its results are withheld + * until its use case reports delivery — and this list forces that to be a decision. + */ + it('classifies every v2 table route', async () => { + const methods = ['GET', 'POST', 'PUT', 'PATCH', 'DELETE'] as const + const declared = new Set() + for (const route of V2_ROUTES) { + if (route.pattern !== '/api/v2/tables' && !route.pattern.startsWith('/api/v2/tables/')) + continue + const module = await route.load() + for (const method of methods) { + if (typeof Reflect.get(module, method) === 'function') + declared.add(`${method} ${route.pattern}`) + } + } + + expect([...TABLE_ROUTES_WITHOUT_ROW_DATA].filter((key) => !declared.has(key))).toEqual([]) + expect([...declared].filter((key) => !TABLE_ROUTES_WITHOUT_ROW_DATA.has(key)).sort()).toEqual([ + 'GET /api/v2/tables/{tableId}/exports/{exportId}/download', + 'GET /api/v2/tables/{tableId}/rows', + 'GET /api/v2/tables/{tableId}/rows/{rowId}', + 'GET /api/v2/tables/{tableId}/rows/{rowId}/enrichment/{groupId}', + 'PATCH /api/v2/tables/{tableId}/rows/{rowId}', + 'POST /api/v2/tables/{tableId}/query', + 'POST /api/v2/tables/{tableId}/rows', + 'POST /api/v2/tables/{tableId}/rows/upsert', + ]) + }, 60_000) +}) diff --git a/apps/sim/lib/mothership/agent-cli/table-read-transport.ts b/apps/sim/lib/mothership/agent-cli/table-read-transport.ts new file mode 100644 index 00000000000..2b4090f4fe7 --- /dev/null +++ b/apps/sim/lib/mothership/agent-cli/table-read-transport.ts @@ -0,0 +1,130 @@ +import { matchV2Route } from '@/lib/api/server/routes/in-process-transport' +import { observeTableRowDelivery } from '@/lib/table/application/row-delivery-observer' +import type { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry' + +/** + * The v2 table routes whose responses carry no cell values, keyed `METHOD pattern`. + * Every other table route — including one added later — must report the provenance + * of the rows it returns, or its result is withheld from the model. + */ +export const TABLE_ROUTES_WITHOUT_ROW_DATA: ReadonlySet = new Set([ + 'GET /api/v2/tables', + 'POST /api/v2/tables', + 'POST /api/v2/tables/bulk-delete', + 'GET /api/v2/tables/folders', + 'POST /api/v2/tables/folders', + 'PATCH /api/v2/tables/folders', + 'DELETE /api/v2/tables/folders', + 'POST /api/v2/tables/folders/restore', + 'POST /api/v2/tables/imports', + 'GET /api/v2/tables/imports/{importId}', + 'DELETE /api/v2/tables/imports/{importId}', + 'POST /api/v2/tables/imports/{importId}/complete', + 'POST /api/v2/tables/imports/{importId}/parts', + 'POST /api/v2/tables/move', + 'GET /api/v2/tables/{tableId}', + 'PATCH /api/v2/tables/{tableId}', + 'DELETE /api/v2/tables/{tableId}', + 'POST /api/v2/tables/{tableId}/cancel-runs', + 'POST /api/v2/tables/{tableId}/columns', + 'PATCH /api/v2/tables/{tableId}/columns', + 'DELETE /api/v2/tables/{tableId}/columns', + 'GET /api/v2/tables/{tableId}/dispatches', + 'POST /api/v2/tables/{tableId}/dispatches', + 'GET /api/v2/tables/{tableId}/dispatches/{dispatchId}', + 'DELETE /api/v2/tables/{tableId}/dispatches/{dispatchId}', + 'POST /api/v2/tables/{tableId}/exports', + 'GET /api/v2/tables/{tableId}/exports/{exportId}', + 'DELETE /api/v2/tables/{tableId}/exports/{exportId}', + 'GET /api/v2/tables/{tableId}/groups', + 'POST /api/v2/tables/{tableId}/groups', + 'PATCH /api/v2/tables/{tableId}/groups', + 'DELETE /api/v2/tables/{tableId}/groups', + 'POST /api/v2/tables/{tableId}/query/count', + 'POST /api/v2/tables/{tableId}/restore', + 'PATCH /api/v2/tables/{tableId}/rows', + 'DELETE /api/v2/tables/{tableId}/rows', + 'DELETE /api/v2/tables/{tableId}/rows/{rowId}', + 'POST /api/v2/tables/{tableId}/rows/{rowId}/enrichment/{groupId}', + 'POST /api/v2/tables/{tableId}/rows/bulk-update', + 'POST /api/v2/tables/{tableId}/rows/search', + 'GET /api/v2/tables/{tableId}/views', + 'POST /api/v2/tables/{tableId}/views', + 'GET /api/v2/tables/{tableId}/views/{viewId}', + 'PATCH /api/v2/tables/{tableId}/views/{viewId}', + 'DELETE /api/v2/tables/{tableId}/views/{viewId}', +]) + +/** + * An export download returns a signed link to the whole table as plaintext CSV, which no + * provenance can follow once fetched, so the link is never returned to the model. + */ +const TABLE_EXPORT_DOWNLOAD_PATTERN = '/api/v2/tables/{tableId}/exports/{exportId}/download' + +/** + * Row data read through the CLI crosses into the model only with its persisted secret + * provenance activated in the turn's registry; a row-bearing response that reported no + * provenance marks the registry incomplete so the result is withheld. So does one whose + * run state or enrichment detail carries error text: that text is captured from executor + * output, which can hold resolved secret plaintext, and no provenance is persisted with it. + */ +export function createTableReadTransport(context: { + endpoint: string + transport: typeof fetch + registry?: ResolvedSecretTraceRegistry +}): typeof fetch { + const base = new URL(context.endpoint) + const basePath = base.pathname.replace(/\/$/, '') + + return async (input, init) => { + const url = new URL(input instanceof Request ? input.url : input) + const method = (init?.method ?? (input instanceof Request ? input.method : 'GET')).toUpperCase() + const route = + url.origin === base.origin && url.pathname.startsWith(`${basePath}/api/v2/tables`) + ? matchV2Route(url.pathname.slice(basePath.length)) + : null + if (!route || TABLE_ROUTES_WITHOUT_ROW_DATA.has(`${method} ${route.pattern}`)) { + return context.transport(input, init) + } + if (route.pattern === TABLE_EXPORT_DOWNLOAD_PATTERN) { + return Response.json( + { + error: { + message: + 'Table export download links are not returned to Sim. Download the export from the Tables page.', + }, + }, + { status: 403 } + ) + } + const registry = context.registry + if (!registry) { + return Response.json( + { error: { message: 'Table data cannot be returned to Sim in this context.' } }, + { status: 503 } + ) + } + let delivered = false + let unprovenancedErrorText = false + let settled = false + let response: Response + try { + response = await observeTableRowDelivery( + async (provenance, values, extras) => { + if (settled) return + delivered = true + if (extras.unprovenancedErrorText) unprovenancedErrorText = true + await registry.importCrossingProvenance(provenance, values, { trusted: true }) + }, + () => context.transport(input, init) + ) + } finally { + settled = true + } + if (response.ok && !delivered) registry.markIncomplete('table-result-provenance-unavailable') + if (response.ok && unprovenancedErrorText) { + registry.markIncomplete('table-run-state-provenance-unavailable') + } + return response + } +} diff --git a/apps/sim/lib/table/application/row-delivery-observer.ts b/apps/sim/lib/table/application/row-delivery-observer.ts new file mode 100644 index 00000000000..37168b52b70 --- /dev/null +++ b/apps/sim/lib/table/application/row-delivery-observer.ts @@ -0,0 +1,40 @@ +import { AsyncLocalStorage } from 'node:async_hooks' +import type { RowData } from '@/lib/table/types' +import type { ResolvedSecretTraceProvenanceV1 } from '@/executor/utils/resolved-secret-trace-registry' + +/** What a use case returns beside its rows that the rows' provenance does not cover. */ +export interface TableRowDeliveryExtras { + /** + * The result carries run-state or enrichment error text (`error`, `blockErrors`, + * cascade provider errors). That text is captured from executor output, which can + * contain resolved secret plaintext, and no provenance is persisted with it. + */ + unprovenancedErrorText: boolean +} + +type TableRowDeliveryObserver = ( + provenance: ResolvedSecretTraceProvenanceV1, + values: RowData[], + extras: TableRowDeliveryExtras +) => Promise + +const observer = new AsyncLocalStorage() + +/** Internal transport evidence observes the persisted provenance of returned rows without changing public API admission or output. */ +export function observeTableRowDelivery(observe: TableRowDeliveryObserver, execute: () => T): T { + return observer.run(observe, execute) +} + +/** Whether the current call has a transport observing returned rows, so use cases read provenance for it. */ +export function hasTableRowDeliveryObserver(): boolean { + return observer.getStore() !== undefined +} + +/** Runs before the canonical use case returns row data to its transport. */ +export async function reportTableRowDelivery( + provenance: ResolvedSecretTraceProvenanceV1, + values: RowData[], + extras: TableRowDeliveryExtras = { unprovenancedErrorText: false } +): Promise { + await observer.getStore()?.(provenance, values, extras) +} diff --git a/apps/sim/lib/table/application/rows.test.ts b/apps/sim/lib/table/application/rows.test.ts index cde68df26ab..3e661a5e224 100644 --- a/apps/sim/lib/table/application/rows.test.ts +++ b/apps/sim/lib/table/application/rows.test.ts @@ -182,6 +182,7 @@ vi.mock('@/lib/table/events', () => ({ })) import { TABLE_LIMITS } from '@/lib/table' +import { observeTableRowDelivery } from '@/lib/table/application/row-delivery-observer' import { batchUpdateTableRows, createTableRows, @@ -770,7 +771,8 @@ describe('row query and upsert application semantics', () => { withExecutions: false, runStateBudgetBytes: TABLE_LIMITS.MAX_ROW_RUN_STATE_BYTES, }, - expect.any(String) + expect.any(String), + undefined ) expect(result.nextCursor).toBe('native-next-cursor') }) @@ -1470,7 +1472,8 @@ describe('opt-in per-cell run state', () => { expect(mockQueryRows).toHaveBeenCalledWith( TABLE, expect.objectContaining({ withExecutions: false }), - expect.any(String) + expect.any(String), + undefined ) }) @@ -1483,7 +1486,8 @@ describe('opt-in per-cell run state', () => { expect(mockQueryRows).toHaveBeenCalledWith( TABLE, expect.objectContaining({ withExecutions: true }), - expect.any(String) + expect.any(String), + undefined ) }) @@ -1811,3 +1815,265 @@ describe('enrichment detail id validation', () => { ) }) }) + +/** + * An internal transport that observes delivery (the Copilot CLI) must see the + * persisted provenance of every row a row-returning use case hands back, even + * though the public surface it dispatched never asks for it on the wire. + */ +describe('row delivery to an observing transport', () => { + const ENRICHED_TABLE: TableDefinition = { + ...TABLE, + schema: { + columns: [{ id: 'column-name', name: 'name', type: 'string' }], + workflowGroups: [{ id: 'group-1', name: 'Enrich', type: 'enrichment', columnIds: [] }], + }, + } + const ROW = { + id: 'row-1', + data: { 'column-name': 'secret-cell' }, + createdAt: new Date('2026-01-01'), + updatedAt: new Date('2026-01-01'), + } + const PAGE = { rows: [ROW], rowCount: 1, totalCount: null, nextCursor: null } + + beforeEach(() => { + vi.clearAllMocks() + mockResolvePermission.mockResolvedValue('write') + mockResolveContext.mockResolvedValue(contextFor(ENRICHED_TABLE)) + mockQueryRows.mockResolvedValue(PAGE) + mockGetRowSummaryById.mockResolvedValue(ROW) + mockLoadExecutionsForRow.mockResolvedValue({}) + mockLoadEnrichmentDetail.mockResolvedValue(null) + mockValidateRowData.mockResolvedValue({ valid: true }) + mockValidateBatchRows.mockResolvedValue({ valid: true }) + mockInsertRow.mockResolvedValue(ROW) + mockBatchInsertRows.mockResolvedValue([ROW]) + mockUpdateRow.mockResolvedValue(ROW) + mockUpsertRow.mockResolvedValue({ operation: 'update', row: ROW }) + }) + + const reads = { + list: () => + listTableRows.execute({ principal: PRINCIPAL, input: { tableId: TABLE.id, limit: 25 } }), + query: () => + queryTableRows.execute({ principal: PRINCIPAL, input: { tableId: TABLE.id, limit: 25 } }), + read: () => + readTableRow.execute({ principal: PRINCIPAL, input: { tableId: TABLE.id, rowId: ROW.id } }), + enrichment: () => + readTableRowEnrichmentDetail.execute({ + principal: PRINCIPAL, + input: { tableId: TABLE.id, rowId: ROW.id, groupId: 'group-1' }, + }), + create: () => + createTableRows.execute({ + principal: PRINCIPAL, + input: { + kind: 'single', + tableId: TABLE.id, + data: { name: 'secret-cell' }, + strictWrite: true, + dataKeying: 'names', + }, + }), + createBatch: () => + createTableRows.execute({ + principal: PRINCIPAL, + input: { + kind: 'batch', + tableId: TABLE.id, + rows: [{ name: 'secret-cell' }], + strictWrite: true, + dataKeying: 'names', + }, + }), + update: () => + updateTableRow.execute({ + principal: PRINCIPAL, + input: { + tableId: TABLE.id, + rowId: ROW.id, + data: { name: 'secret-cell' }, + strictWrite: true, + dataKeying: 'names', + }, + }), + upsert: () => + upsertTableRow.execute({ + principal: PRINCIPAL, + input: { + tableId: TABLE.id, + data: { name: 'secret-cell' }, + strictWrite: true, + dataKeying: 'names', + }, + }), + } + + it.each(Object.keys(reads) as Array)( + '%s reports the returned rows and their provenance', + async (name) => { + const observe = vi.fn(async () => {}) + const result = await observeTableRowDelivery(observe, reads[name]) + + expect(mockLoadSecretProvenance).toHaveBeenCalledWith({ + userId: PRINCIPAL.userId, + workspaceId: TABLE.workspaceId, + }) + expect(observe).toHaveBeenCalledTimes(1) + expect(observe).toHaveBeenCalledWith( + { version: 1, complete: true, entries: [] }, + [ROW.data], + { + unprovenancedErrorText: false, + } + ) + expect((result as { secretProvenance?: unknown }).secretProvenance).toBeUndefined() + } + ) + + it.each(Object.keys(reads) as Array)( + '%s reads no provenance without an observer or an explicit request', + async (name) => { + await reads[name]() + + expect(mockLoadSecretProvenance).not.toHaveBeenCalled() + } + ) + describe('run-state and enrichment error text', () => { + const CLEAN_RUN = { + status: 'completed', + executionId: 'exec-1', + jobId: null, + workflowId: 'workflow-1', + error: null, + } + const RUN_WITH_ERROR = { ...CLEAN_RUN, status: 'error', error: 'failed with sk-live-secret' } + const RUN_WITH_BLOCK_ERROR = { + ...CLEAN_RUN, + status: 'error', + blockErrors: { 'block-1': 'Authorization: Bearer sk-live-secret' }, + } + + async function reportedErrorText(read: () => Promise): Promise { + const observe = vi.fn(async () => {}) + await observeTableRowDelivery(observe, read) + expect(observe).toHaveBeenCalledTimes(1) + const extras = observe.mock.calls[0]?.[2] as { unprovenancedErrorText: boolean } + return extras.unprovenancedErrorText + } + + const withRunState = { + list: () => + listTableRows.execute({ + principal: PRINCIPAL, + input: { tableId: TABLE.id, limit: 25, includeRunState: true }, + }), + query: () => + queryTableRows.execute({ + principal: PRINCIPAL, + input: { tableId: TABLE.id, limit: 25, includeRunState: true }, + }), + } + + it.each(Object.keys(withRunState) as Array)( + '%s signals returned run-state error text', + async (name) => { + mockQueryRows.mockResolvedValue({ + ...PAGE, + rows: [ + { ...ROW, executions: { 'group-1': CLEAN_RUN } }, + { ...ROW, id: 'row-2', executions: { 'group-1': RUN_WITH_ERROR } }, + ], + }) + expect(await reportedErrorText(withRunState[name])).toBe(true) + + mockQueryRows.mockResolvedValue({ + ...PAGE, + rows: [{ ...ROW, executions: { 'group-1': RUN_WITH_BLOCK_ERROR } }], + }) + expect(await reportedErrorText(withRunState[name])).toBe(true) + } + ) + + it.each(Object.keys(withRunState) as Array)( + '%s does not signal run state without error text', + async (name) => { + mockQueryRows.mockResolvedValue({ + ...PAGE, + rows: [ + { ...ROW, executions: { 'group-1': { ...CLEAN_RUN, error: '', blockErrors: {} } } }, + ], + }) + expect(await reportedErrorText(withRunState[name])).toBe(false) + } + ) + + it.each(['list', 'query'] as const)( + '%s does not signal error text the response omits without includeRunState', + async (name) => { + mockQueryRows.mockResolvedValue({ + ...PAGE, + rows: [{ ...ROW, executions: { 'group-1': RUN_WITH_ERROR } }], + }) + expect(await reportedErrorText(reads[name])).toBe(false) + } + ) + + it('read signals included run-state error text, and only when included', async () => { + mockLoadExecutionsForRow.mockResolvedValue({ 'group-1': RUN_WITH_BLOCK_ERROR }) + const readWithRunState = () => + readTableRow.execute({ + principal: PRINCIPAL, + input: { tableId: TABLE.id, rowId: ROW.id, includeRunState: true }, + }) + + expect(await reportedErrorText(readWithRunState)).toBe(true) + expect(await reportedErrorText(reads.read)).toBe(false) + + mockLoadExecutionsForRow.mockResolvedValue({ 'group-1': CLEAN_RUN }) + expect(await reportedErrorText(readWithRunState)).toBe(false) + }) + + it('enrichment detail signals its own group run-state error text', async () => { + mockLoadExecutionsForRow.mockResolvedValue({ 'group-1': RUN_WITH_ERROR }) + expect(await reportedErrorText(reads.enrichment)).toBe(true) + + mockLoadExecutionsForRow.mockResolvedValue({ + 'group-1': CLEAN_RUN, + 'group-2': RUN_WITH_ERROR, + }) + expect(await reportedErrorText(reads.enrichment)).toBe(false) + }) + + it('enrichment detail signals cascade provider error text', async () => { + const provider = { + id: 'hunter', + label: 'Hunter', + toolId: 'hunter_find_email', + status: 'error', + cost: 0, + durationMs: 5, + error: 'upstream rejected key sk-live-secret', + } + const detail = { + startedAt: '2026-01-01T00:00:00.000Z', + completedAt: '2026-01-01T00:00:01.000Z', + durationMs: 1000, + totalCost: 0, + matchedProvider: null, + aborted: false, + providers: [{ ...provider, status: 'no_match', error: null }, provider], + } + mockLoadExecutionsForRow.mockResolvedValue({ 'group-1': CLEAN_RUN }) + mockLoadEnrichmentDetail.mockResolvedValue(detail) + expect(await reportedErrorText(reads.enrichment)).toBe(true) + + mockLoadEnrichmentDetail.mockResolvedValue({ + ...detail, + providers: [{ ...provider, status: 'matched', error: null }], + }) + expect(await reportedErrorText(reads.enrichment)).toBe(false) + }) + }) +}) diff --git a/apps/sim/lib/table/application/rows.ts b/apps/sim/lib/table/application/rows.ts index 45085b36fc6..222fa9417a7 100644 --- a/apps/sim/lib/table/application/rows.ts +++ b/apps/sim/lib/table/application/rows.ts @@ -8,13 +8,14 @@ import { import { db } from '@sim/db' import { getRequestContext } from '@sim/logger' import { generateId } from '@sim/utils/id' -import { isPlainRecord } from '@sim/utils/object' +import { isPlainRecord, toRecord } from '@sim/utils/object' import { capabilityGovernedPrincipalUserId } from '@/lib/core/application' import { OrchestrationError } from '@/lib/core/orchestration/types' import { isPrivateSecretProvenanceScopeCompatible } from '@/lib/execution/durable-secret-provenance' import type { BulkDeleteByIdsResult, BulkOperationResult, + EnrichmentRunDetail, Filter, ReplaceRowsResult, RowData, @@ -54,6 +55,10 @@ import { import { defineAuthorizedTableUseCase } from '@/lib/table/application/authorized-table-use-case' import { resolveActiveTableContext } from '@/lib/table/application/context' import { tableOperations } from '@/lib/table/application/operations' +import { + hasTableRowDeliveryObserver, + reportTableRowDelivery, +} from '@/lib/table/application/row-delivery-observer' import { resolveRowWriteProvenance, type TableRowProvenanceEnvelope, @@ -158,16 +163,68 @@ interface TableResult { type TableRowsProvenance = ReturnType +/** Provenance is read when the caller asked for it or an internal transport observes delivery. */ function createAuthorizedRowsProvenanceReader( workspaceId: string, attributedUserId: string, - include: boolean | undefined + include?: boolean ): TableRowProvenanceReader | undefined { - return include + return include || hasTableRowDeliveryObserver() ? new TableRowProvenanceReader({ userId: attributedUserId, workspaceId }) : undefined } +/** + * Reports the provenance of the rows a use case is about to return to an observing + * transport, and hands it back only when the caller itself asked for it. + */ +async function deliverRowsProvenance( + reader: TableRowProvenanceReader | undefined, + rows: readonly { data: RowData }[], + options: { + /** The caller itself asked for the provenance back. */ + include?: boolean + /** The result also carries run-state or enrichment error text, which has no provenance. */ + unprovenancedErrorText?: boolean + } = {} +): Promise { + if (!reader) return undefined + const provenance = reader.exportProvenance() + await reportTableRowDelivery( + provenance, + rows.map((row) => row.data), + { unprovenancedErrorText: options.unprovenancedErrorText ?? false } + ) + return options.include ? provenance : undefined +} + +function isNonEmptyText(value: unknown): boolean { + return typeof value === 'string' && value.length > 0 +} + +/** Whether one group's run state carries error text: its run `error` or any `blockErrors` entry. */ +function executionHasErrorText(execution: RowExecutionMetadata): boolean { + return ( + isNonEmptyText(execution.error) || + Object.values(execution.blockErrors ?? {}).some(isNonEmptyText) + ) +} + +/** Whether any group in a row's run state carries error text. */ +function runStateHasErrorText(executions: RowExecutions): boolean { + return Object.values(executions).some(executionHasErrorText) +} + +/** + * Whether an enrichment cascade carries provider error text. The blob is schemaless JSONB, + * so it is read as defensively as the v2 presenter projects it. + */ +function enrichmentDetailHasErrorText(detail: EnrichmentRunDetail | null): boolean { + const providers: unknown = detail?.providers + if (!Array.isArray(providers)) return false + return providers.some((provider) => isNonEmptyText(toRecord(provider).error)) +} + function requestId(input: TableScopedInput): string { return input.requestId ?? getRequestContext()?.requestId ?? generateId().slice(0, 8) } @@ -427,8 +484,12 @@ export interface ListTableRowsResult extends TableResult { export const listTableRows = defineAuthorizedTableUseCase({ operation: tableOperations.listRows, resolveContext: ({ input }: { input: ListTableRowsInput }) => resolveActiveTableContext(input), - async execute({ input, context }): Promise { + async execute({ principal, input, context }): Promise { requireIntegerInRange(input.limit, 1, TABLE_LIMITS.MAX_QUERY_LIMIT, 'Limit') + const readProvenance = createAuthorizedRowsProvenanceReader( + context.workspaceId, + actorUserId(principal, context.billedAccountUserId) + ) try { const cursor = input.cursor ? decodeCursor(input.cursor) : undefined if (cursor) assertCursorQueryBinding(cursor, {}) @@ -442,8 +503,14 @@ export const listTableRows = defineAuthorizedTableUseCase({ withExecutions: input.includeRunState ?? false, runStateBudgetBytes: TABLE_LIMITS.MAX_ROW_RUN_STATE_BYTES, }, - requestId(input) + requestId(input), + readProvenance ) + await deliverRowsProvenance(readProvenance, result.rows, { + unprovenancedErrorText: + input.includeRunState === true && + result.rows.some((row) => runStateHasErrorText(row.executions)), + }) return { table: context.table, rows: result.rows, @@ -587,7 +654,12 @@ export const queryTableRows = defineAuthorizedTableUseCase({ return { table: context.table, ...result, - secretProvenance: readProvenance?.exportProvenance(), + secretProvenance: await deliverRowsProvenance(readProvenance, result.rows, { + include: input.includePersistedSecretProvenance, + unprovenancedErrorText: + input.includeRunState === true && + result.rows.some((row) => runStateHasErrorText(row.executions)), + }), } } catch (error) { rethrowQueryValidation(error) @@ -673,7 +745,10 @@ export const readTableRow = defineAuthorizedTableUseCase({ table: context.table, row, ...(runState ? { runState } : {}), - secretProvenance: readProvenance?.exportProvenance(), + secretProvenance: await deliverRowsProvenance(readProvenance, [row], { + include: input.includePersistedSecretProvenance, + unprovenancedErrorText: runState !== undefined && runStateHasErrorText(runState), + }), } }, }) @@ -709,8 +784,17 @@ export const readTableRowEnrichmentDetail = defineAuthorizedTableUseCase({ operation: tableOperations.readRow, resolveContext: ({ input }: { input: ReadTableRowEnrichmentInput }) => resolveActiveTableContext(input), - async execute({ input, context }): Promise { - const row = await getRowSummaryById(context.tableId, input.rowId, context.workspaceId) + async execute({ principal, input, context }): Promise { + const readProvenance = createAuthorizedRowsProvenanceReader( + context.workspaceId, + actorUserId(principal, context.billedAccountUserId) + ) + const row = await getRowSummaryById( + context.tableId, + input.rowId, + context.workspaceId, + readProvenance + ) if (!row) throw new OrchestrationError('not_found', 'Row not found') const group = (context.table.schema.workflowGroups ?? []).find( (candidate) => candidate.id === input.groupId @@ -722,11 +806,17 @@ export const readTableRowEnrichmentDetail = defineAuthorizedTableUseCase({ loadExecutionsForRow(db, input.rowId, { budgetBytes: TABLE_LIMITS.MAX_ROW_RUN_STATE_BYTES }), loadEnrichmentDetail(db, context.tableId, input.rowId, input.groupId), ]) + const runState = executions[input.groupId] ?? null + await deliverRowsProvenance(readProvenance, [row], { + unprovenancedErrorText: + (runState !== null && executionHasErrorText(runState)) || + enrichmentDetailHasErrorText(detail), + }) return { table: context.table, row, group, - runState: executions[input.groupId] ?? null, + runState, detail, } }, @@ -825,7 +915,9 @@ export const createTableRows = defineAuthorizedTableUseCase({ kind: 'single', table: context.table, row, - secretProvenance: readProvenance?.exportProvenance(), + secretProvenance: await deliverRowsProvenance(readProvenance, [row], { + include: input.includePersistedSecretProvenance, + }), } } if (input.rows.length < 1 || input.rows.length > TABLE_LIMITS.MAX_BATCH_INSERT_SIZE) { @@ -878,7 +970,9 @@ export const createTableRows = defineAuthorizedTableUseCase({ kind: 'batch', table: context.table, rows: created, - secretProvenance: readProvenance?.exportProvenance(), + secretProvenance: await deliverRowsProvenance(readProvenance, created, { + include: input.includePersistedSecretProvenance, + }), } }, afterSuccess: ({ context, input, result }) => { @@ -1183,7 +1277,9 @@ export const updateTableRow = defineAuthorizedTableUseCase({ table: context.table, row, changed: Object.keys(data).length > 0, - secretProvenance: readProvenance?.exportProvenance(), + secretProvenance: await deliverRowsProvenance(readProvenance, [row], { + include: input.includePersistedSecretProvenance, + }), } }, afterSuccess: ({ context, input, result }) => { @@ -1506,7 +1602,9 @@ export const upsertTableRow = defineAuthorizedTableUseCase({ table: context.table, row: result.row, operation: result.operation, - secretProvenance: readProvenance?.exportProvenance(), + secretProvenance: await deliverRowsProvenance(readProvenance, [result.row], { + include: input.includePersistedSecretProvenance, + }), } }, afterSuccess: ({ context }) => signalTableRowsChanged(context.tableId), From d1e847387e4c4c230ea8700a550a2d4768f306b9 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 23 Sep 2026 14:55:54 -0700 Subject: [PATCH 3/7] fix(search): run live Calendar service search on the crawl scope PR #8208 widened Google Calendar's domain-wide delegation scopes to include calendar.readonly for every service-account token. Google refuses the whole token exchange when any requested scope is not authorized for the client, so indexed admin-mode Calendar syncs configured for calendar.events.readonly failed every run and were eventually disabled. Live service search only needed the wider scope to read an all-day event's calendar time zone. events.list returns the calendar's timeZone under calendar.events.readonly, so live search now reads it there and delegates with the same single scope as the indexed crawl, matching Drive and Gmail. The admin calendar picker keeps its own calendar.readonly request, as documented. --- apps/docs/content/docs/search/google-calendar.mdx | 4 ++-- apps/sim/connectors/google-calendar/company-crawl.test.ts | 5 +---- apps/sim/connectors/google-calendar/meta.ts | 5 +---- apps/sim/lib/knowledge/connectors/access-token.test.ts | 5 +---- apps/sim/lib/sim-search/live/README.md | 2 +- apps/sim/lib/sim-search/live/google-service.test.ts | 7 +++++++ apps/sim/lib/sim-search/live/google-service.ts | 5 ++++- 7 files changed, 17 insertions(+), 16 deletions(-) diff --git a/apps/docs/content/docs/search/google-calendar.mdx b/apps/docs/content/docs/search/google-calendar.mdx index 0ef3fe6e662..adc08458fc4 100644 --- a/apps/docs/content/docs/search/google-calendar.mdx +++ b/apps/docs/content/docs/search/google-calendar.mdx @@ -22,7 +22,7 @@ https://www.googleapis.com/auth/calendar.readonly https://www.googleapis.com/auth/admin.directory.user.readonly ``` -`calendar.readonly` is needed for calendar metadata and the CalendarList picker; event-read scope alone is insufficient. See [CalendarList authorization](https://developers.google.com/workspace/calendar/api/v3/reference/calendarList/list). +Search reads events with `calendar.events.readonly`. The calendar picker also needs `calendar.readonly` to list the delegated administrator's calendars. See [CalendarList authorization](https://developers.google.com/workspace/calendar/api/v3/reference/calendarList/list). 3. In **Settings → Sources → Google Calendar**, choose **Service account**, then **Add connection**. Add or select the Google credential and set **Delegated administrator** to an active Workspace administrator with Directory user-read access. 4. Select calendars, users, a date range, and optional event-text and attendee settings. Save and select this connection as the service source. @@ -44,7 +44,7 @@ Sim does not copy the organizer's richer event details into an attendee's view o | Problem | Next step | | --- | --- | -| Calendar picker or metadata read fails | Include `calendar.readonly` in delegation, along with the other scopes above. | +| Calendar picker fails | Include `calendar.readonly` in delegation, along with the other scopes above. | | A member gets no matches | Check their personal connection, primary Workspace identity, Users selection, calendars, and source date window. | | Shared calendar is missing | Confirm both the member and delegated source identity can access that calendar ID. | | Event is outside the configured range | Adjust the service source's range or narrow the user's query to its allowed window. | diff --git a/apps/sim/connectors/google-calendar/company-crawl.test.ts b/apps/sim/connectors/google-calendar/company-crawl.test.ts index 248fcf20ebb..be9ecda4dea 100644 --- a/apps/sim/connectors/google-calendar/company-crawl.test.ts +++ b/apps/sim/connectors/google-calendar/company-crawl.test.ts @@ -74,10 +74,7 @@ describe('Google Calendar company crawl', () => { requiredScopes: ['https://www.googleapis.com/auth/calendar'], adminCredentialType: 'service_account', adminServiceAccountScopes: ['https://www.googleapis.com/auth/admin.directory.user.readonly'], - serviceAccountDelegationScopes: [ - 'https://www.googleapis.com/auth/calendar.events.readonly', - 'https://www.googleapis.com/auth/calendar.readonly', - ], + serviceAccountDelegationScopes: ['https://www.googleapis.com/auth/calendar.events.readonly'], serviceAccountSubjectFieldId: 'adminEmail', }) expect( diff --git a/apps/sim/connectors/google-calendar/meta.ts b/apps/sim/connectors/google-calendar/meta.ts index 456af213fa2..6b3490eb7fc 100644 --- a/apps/sim/connectors/google-calendar/meta.ts +++ b/apps/sim/connectors/google-calendar/meta.ts @@ -19,10 +19,7 @@ export const googleCalendarConnectorMeta: ConnectorMeta = { adminCredentialType: 'service_account', serviceAccountScopes: ['https://www.googleapis.com/auth/calendar.events.readonly'], adminServiceAccountScopes: ['https://www.googleapis.com/auth/admin.directory.user.readonly'], - serviceAccountDelegationScopes: [ - 'https://www.googleapis.com/auth/calendar.events.readonly', - 'https://www.googleapis.com/auth/calendar.readonly', - ], + serviceAccountDelegationScopes: ['https://www.googleapis.com/auth/calendar.events.readonly'], serviceAccountSubjectFieldId: 'adminEmail', }, diff --git a/apps/sim/lib/knowledge/connectors/access-token.test.ts b/apps/sim/lib/knowledge/connectors/access-token.test.ts index 83e0170b532..dbae585254d 100644 --- a/apps/sim/lib/knowledge/connectors/access-token.test.ts +++ b/apps/sim/lib/knowledge/connectors/access-token.test.ts @@ -461,10 +461,7 @@ describe.each([ name: 'Google Calendar', auth: googleCalendarConnectorMeta.auth, contentScope: 'https://www.googleapis.com/auth/calendar.events.readonly', - delegatedScopes: [ - 'https://www.googleapis.com/auth/calendar.events.readonly', - 'https://www.googleapis.com/auth/calendar.readonly', - ], + delegatedScopes: ['https://www.googleapis.com/auth/calendar.events.readonly'], }, ])('$name declared company authentication', ({ auth, contentScope, delegatedScopes }) => { const directoryScope = 'https://www.googleapis.com/auth/admin.directory.user.readonly' diff --git a/apps/sim/lib/sim-search/live/README.md b/apps/sim/lib/sim-search/live/README.md index 2f167f49677..86bf763a9fb 100644 --- a/apps/sim/lib/sim-search/live/README.md +++ b/apps/sim/lib/sim-search/live/README.md @@ -48,7 +48,7 @@ Selected labels are alternatives. Source date/query settings are authoritative r ### Google Calendar -Member mode searches calendars accessible through the member's account. Service mode verifies the account's primary-calendar identity, Directory customer, and source user selection before delegating to that same Workspace user. Selected calendar IDs constrain retrieval and source verification; `primary` means that member's primary calendar. The admin picker browses the delegated administrator's calendar list and stores `primary` as a per-member alias. Domain-wide delegation must authorize both Calendar events read and Calendar read scopes; CalendarList requires the latter ([CalendarList authorization](https://developers.google.com/workspace/calendar/api/v3/reference/calendarList/list)). +Member mode searches calendars accessible through the member's account. Service mode verifies the account's primary-calendar identity, Directory customer, and source user selection before delegating to that same Workspace user. Selected calendar IDs constrain retrieval and source verification; `primary` means that member's primary calendar. The admin picker browses the delegated administrator's calendar list and stores `primary` as a per-member alias. Service search delegates with only `calendar.events.readonly` (plus `admin.directory.user.readonly` for the Directory check), the same scope as the indexed crawl; all-day events take the calendar's time zone from the events list response. The picker additionally needs `calendar.readonly` ([CalendarList authorization](https://developers.google.com/workspace/calendar/api/v3/reference/calendarList/list)). Each event must exist under the source's delegated token, be in an allowed calendar, not be cancelled, and overlap the source's configured rolling time window. The existing default is 30 days before and after the request. A stable UTC-day envelope around that window is intersected with the user's date bounds in the provider query, ensuring recurring events expand and query bounds stay stable between pages. The exact rolling source window is still checked for each result. Nonoverlapping date ranges return no results; continuations spanning a UTC-day change may require a fresh search. A source search query is checked with Calendar's event search and exact event-ID matching. All-day events use the calendar's timezone. Attendee details follow the source's include-attendees setting. The member's API access still determines which event details they can see. diff --git a/apps/sim/lib/sim-search/live/google-service.test.ts b/apps/sim/lib/sim-search/live/google-service.test.ts index cb19b31a179..49f2c14a9a9 100644 --- a/apps/sim/lib/sim-search/live/google-service.test.ts +++ b/apps/sim/lib/sim-search/live/google-service.test.ts @@ -269,6 +269,13 @@ describe('Google service source filtering', () => { container: 'primary', }) ).toBe(true) + expect(delegated.json).toHaveBeenCalledWith( + '/calendar/v3/calendars/reader%40example.com/events', + { + query: { maxResults: '1', fields: 'timeZone' }, + } + ) + for (const [path] of delegated.json.mock.calls) expect(path).toMatch(/\/events(\/|$)/) }) it('verifies twenty all-day events within the request budget and refreshes timezones next session', async () => { vi.setSystemTime(new Date('2026-09-22T02:00:00Z')) diff --git a/apps/sim/lib/sim-search/live/google-service.ts b/apps/sim/lib/sim-search/live/google-service.ts index 15c896164f7..6c3b09fe90d 100644 --- a/apps/sim/lib/sim-search/live/google-service.ts +++ b/apps/sim/lib/sim-search/live/google-service.ts @@ -120,12 +120,15 @@ export async function createGoogleServiceVerifier(input: { } const calendarTimeZones = new Map>() const calendarWindow = calendarSourceWindow(config, Date.now()) + /** An events.list response carries the calendar's time zone under the crawl's events-only scope. */ const calendarTimeZone = (subject: string, calendarId: string, client: NativeClient) => { const key = JSON.stringify([subject, calendarId]) let pending = calendarTimeZones.get(key) if (!pending) { pending = client - .json(`/calendar/v3/calendars/${segment(calendarId)}`) + .json(`/calendar/v3/calendars/${segment(calendarId)}/events`, { + query: { maxResults: '1', fields: 'timeZone' }, + }) .then((row) => string(object(row).timeZone)) calendarTimeZones.set(key, pending) } From e6cb9d4c4da24a202825ef6cbf8ca787927c4963 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 23 Sep 2026 15:01:58 -0700 Subject: [PATCH 4/7] fix(search): keep results painted when Show more widens the page --- .../search/search-results-view.test.tsx | 4 ++- .../o/[organizationId]/search/search.test.tsx | 4 ++- .../knowledge-search-results.test.tsx | 1 + .../knowledge-search-results.tsx | 3 +- .../search-transitions.test.tsx | 35 ++++++++++++++----- apps/sim/hooks/queries/kb/knowledge.test.ts | 18 ++++++++++ apps/sim/hooks/queries/kb/knowledge.ts | 35 ++++++++++++------- 7 files changed, 76 insertions(+), 24 deletions(-) diff --git a/apps/sim/app/o/[organizationId]/search/search-results-view.test.tsx b/apps/sim/app/o/[organizationId]/search/search-results-view.test.tsx index 64bf14e18ec..c4932393765 100644 --- a/apps/sim/app/o/[organizationId]/search/search-results-view.test.tsx +++ b/apps/sim/app/o/[organizationId]/search/search-results-view.test.tsx @@ -145,7 +145,9 @@ describe('results-only search layout', () => { const filters = mocks.search.mock.calls.at(-1)![2] expect(filters.source).toBe('slack') expect(mocks.searchChange).toHaveBeenLastCalledWith({ scope, query: 'Orion', filters }) - expect(mocks.search).toHaveBeenLastCalledWith(scope, 'Orion', filters, 20) + expect(mocks.search).toHaveBeenLastCalledWith(scope, 'Orion', filters, 20, { + retainAcrossLimits: true, + }) }) it('summarizes the selected document through Home without duplicating search state', async () => { await render('?q=Orion') diff --git a/apps/sim/app/o/[organizationId]/search/search.test.tsx b/apps/sim/app/o/[organizationId]/search/search.test.tsx index ec5cb0bf645..78f6da0998e 100644 --- a/apps/sim/app/o/[organizationId]/search/search.test.tsx +++ b/apps/sim/app/o/[organizationId]/search/search.test.tsx @@ -163,7 +163,9 @@ async function editDraft(value: string) { function expectVisibleQuery(query: string) { expect(searchInput().value).toBe(query) expect(container.querySelector('a[data-source-link]')?.textContent).toBe(`${query} launch plan`) - expect(mocks.search).toHaveBeenLastCalledWith(scope, query, {}, 20) + expect(mocks.search).toHaveBeenLastCalledWith(scope, query, {}, 20, { + retainAcrossLimits: true, + }) expect(document.activeElement).toBe(searchInput()) } diff --git a/apps/sim/app/workspace/[workspaceId]/home/components/knowledge-search-results/knowledge-search-results.test.tsx b/apps/sim/app/workspace/[workspaceId]/home/components/knowledge-search-results/knowledge-search-results.test.tsx index c07a27256c9..d87125e5c6d 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/components/knowledge-search-results/knowledge-search-results.test.tsx +++ b/apps/sim/app/workspace/[workspaceId]/home/components/knowledge-search-results/knowledge-search-results.test.tsx @@ -195,6 +195,7 @@ describe('result paging and the custom window', () => { await act(async () => more()!.click()) /** The wider search is its own request; the first paint was never widened. */ expect(mocks.search.mock.calls.at(-1)![3]).toBe(50) + expect(mocks.search.mock.calls.at(-1)![4]).toEqual({ retainAcrossLimits: true }) expect(more()).toBeUndefined() mocks.search.mockReturnValue(page(7)) await render() diff --git a/apps/sim/app/workspace/[workspaceId]/home/components/knowledge-search-results/knowledge-search-results.tsx b/apps/sim/app/workspace/[workspaceId]/home/components/knowledge-search-results/knowledge-search-results.tsx index e7c73601170..48766c9ec4e 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/components/knowledge-search-results/knowledge-search-results.tsx +++ b/apps/sim/app/workspace/[workspaceId]/home/components/knowledge-search-results/knowledge-search-results.tsx @@ -201,7 +201,8 @@ function SearchResults({ topK ?? (expanded ? WORKSPACE_KNOWLEDGE_SEARCH_LIMITS.expanded - : WORKSPACE_KNOWLEDGE_SEARCH_LIMITS.initial) + : WORKSPACE_KNOWLEDGE_SEARCH_LIMITS.initial), + { retainAcrossLimits: topK === undefined } ) /** A full first page may collapse to few cards, yet more documents may still match. */ const mayHaveMore = diff --git a/apps/sim/app/workspace/[workspaceId]/home/components/knowledge-search-results/search-transitions.test.tsx b/apps/sim/app/workspace/[workspaceId]/home/components/knowledge-search-results/search-transitions.test.tsx index 2ae1215f024..ddba2c4c0ab 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/components/knowledge-search-results/search-transitions.test.tsx +++ b/apps/sim/app/workspace/[workspaceId]/home/components/knowledge-search-results/search-transitions.test.tsx @@ -174,7 +174,7 @@ async function click(label: string) { async function complete( index: number, - { title = 'Release plan', partial = false, empty = false } = {} + { title = 'Release plan', partial = false, empty = false, count = 1 } = {} ) { await act(async () => { requests[index].resolve({ @@ -182,21 +182,22 @@ async function complete( query: requests[index].body.query, results: empty ? [] - : [ - { - documentId: title, + : Array.from({ length: count }, (_, n) => { + const name = n === 0 ? title : `${title} ${n + 1}` + return { + documentId: name, knowledgeBaseId: 'index', knowledgeBaseName: 'Search index', - documentName: title, - sourceUrl: 'https://example.com/release', + documentName: name, + sourceUrl: `https://example.com/release/${n}`, connectorType: requests[index].body.filters?.source ?? 'slack', sourceModifiedAt: null, author: null, content: 'launch details', chunkIndex: 0, similarity: 0.9, - }, - ], + } + }), retrieval: { status: partial ? 'partial' : 'complete', timedOutLegs: partial ? ['vector'] : [], @@ -285,6 +286,24 @@ describe('search refinement with the real query cache and URL state', () => { expect(container.textContent).not.toContain('Release plan') }) + it('keeps the first page painted while Show more widens it, then while a filter narrows it', async () => { + await render() + await complete(0, { count: 20 }) + await click('Show more') + expect(requests[1].body.topK).toBe(50) + expect(container.textContent).toContain('Updating results…') + expect(container.textContent).not.toContain('Searching…') + expect(container.querySelectorAll('a[data-source-link]')).toHaveLength(20) + await complete(1, { title: 'Wider plan', count: 50 }) + expect(container.querySelectorAll('a[data-source-link]')).toHaveLength(50) + await click('Gmail') + expect(requests[2].body).toMatchObject({ filters: { source: 'gmail' }, topK: 20 }) + expect(container.textContent).toContain('Updating results…') + expect(container.textContent).not.toContain('Searching…') + expect(container.querySelectorAll('a[data-source-link]')).toHaveLength(50) + expect(container.querySelector('a[data-source-link]')?.textContent).toBe('Wider plan') + }) + it('replaces filter URL state while preserving unrelated parameters', async () => { await render({ params: '?q=launch&panel=details' }) await complete(0) diff --git a/apps/sim/hooks/queries/kb/knowledge.test.ts b/apps/sim/hooks/queries/kb/knowledge.test.ts index 8775da00ff3..df0ae1b3d8a 100644 --- a/apps/sim/hooks/queries/kb/knowledge.test.ts +++ b/apps/sim/hooks/queries/kb/knowledge.test.ts @@ -243,6 +243,24 @@ describe('knowledge query placeholder scope', () => { expect(placeholder('workspace-1', 'different', 5, 'reader')).toBeUndefined() }) + it('retains a page-owned search across result limits only for the same reader', () => { + const query = captureQuery(() => + useWorkspaceKnowledgeSearch('workspace-1', 'release', { source: 'slack' }, 50, { + retainAcrossLimits: true, + }) + ) + const previous = { results: [{ documentId: 'private-document' }] } + mocks.getQueryData.mockReturnValue(previous) + const placeholder = (topK: number, userId: string) => + query.placeholderData?.(previous, { + queryKey: knowledgeKeys.search('workspace-1', 'release', {}, topK, userId), + state: { status: 'success', isInvalidated: false }, + }) + expect(placeholder(20, 'reader')).toBe(previous) + expect(placeholder(50, 'reader')).toBe(previous) + expect(placeholder(20, 'other')).toBeUndefined() + }) + it('partitions search cache entries by filter and reader', () => { const query = captureQuery(() => useWorkspaceKnowledgeSearch('workspace-1', 'new query', { source: 'slack' }) diff --git a/apps/sim/hooks/queries/kb/knowledge.ts b/apps/sim/hooks/queries/kb/knowledge.ts index 49e582d1636..7f7258b6dfc 100644 --- a/apps/sim/hooks/queries/kb/knowledge.ts +++ b/apps/sim/hooks/queries/kb/knowledge.ts @@ -1205,13 +1205,24 @@ async function searchWorkspaceKnowledge( return data.data } +interface WorkspaceKnowledgeSearchOptions { + nativeQueries?: NativeSearchQuery[] + reuseFreshResult?: boolean + /** + * Keeps the previous result painted when only the result limit changes. Set it when the + * surface owns the limit (Show more widens the same search); leave it off when the limit is + * part of what was asked for, so a new limit is a new search that never shows the old one. + */ + retainAcrossLimits?: boolean +} + /** Searches the canonical index under the signed-in person's ACLs. */ export function useWorkspaceKnowledgeSearch( owner: string | ResourceScope | undefined, query: string, filters?: WorkspaceSearchFilters, topK = 20, - options?: { nativeQueries?: NativeSearchQuery[]; reuseFreshResult?: boolean } + options?: WorkspaceKnowledgeSearchOptions ) { const { features } = useDeploymentShape() const live = features.liveEnterpriseSearch === true @@ -1258,17 +1269,15 @@ export function useWorkspaceKnowledgeSearch( : 0 : WORKSPACE_KNOWLEDGE_SEARCH_STALE_TIME, retry: false, - placeholderData: (previous, previousQuery) => - !live && - userId && - previousQuery?.state.status === 'success' && - previousQuery.queryKey[6] === topK && - !previousQuery.state.isInvalidated && - knowledgeKeys - .searchQuery(scopeKey, trimmed, userId) - .every((part, index) => previousQuery.queryKey[index] === part) && - queryClient.getQueryData(previousQuery.queryKey) === previous - ? previous - : undefined, + placeholderData: (previous, previousQuery) => { + if (live || !userId || previousQuery?.state.status !== 'success') return undefined + if (previousQuery.state.isInvalidated) return undefined + const prefix = knowledgeKeys.searchQuery(scopeKey, trimmed, userId) + if (!prefix.every((part, index) => previousQuery.queryKey[index] === part)) return undefined + /** `search()` appends filters, then the limit, after the reader/query prefix. */ + const previousTopK = previousQuery.queryKey[prefix.length + 1] + if (!options?.retainAcrossLimits && previousTopK !== topK) return undefined + return queryClient.getQueryData(previousQuery.queryKey) === previous ? previous : undefined + }, }) } From 2527a79d09a5e24663ba5f7810be3408dced7153 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 23 Sep 2026 15:03:15 -0700 Subject: [PATCH 5/7] fix(chat): use the defined brand token for the resource activity dot --- .../[workspaceId]/home/components/chat-panel-layout.test.tsx | 3 +++ .../[workspaceId]/home/components/chat-panel-layout.tsx | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/apps/sim/app/workspace/[workspaceId]/home/components/chat-panel-layout.test.tsx b/apps/sim/app/workspace/[workspaceId]/home/components/chat-panel-layout.test.tsx index 492bcb6ec89..76c1cb34678 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/components/chat-panel-layout.test.tsx +++ b/apps/sim/app/workspace/[workspaceId]/home/components/chat-panel-layout.test.tsx @@ -78,6 +78,9 @@ it.each([1, 2])( `Expand resource view, ${count} resource${count === 1 ? '' : 's'} updated` ) expect(container.querySelector('[role="separator"]')).toBeNull() + expect(button.querySelector('span[aria-hidden="true"]')?.className).toContain( + 'bg-[var(--brand-blue)]' + ) await act(async () => button.click()) expect(toggle).toHaveBeenCalledOnce() expect(container.textContent).toContain('Conversation') diff --git a/apps/sim/app/workspace/[workspaceId]/home/components/chat-panel-layout.tsx b/apps/sim/app/workspace/[workspaceId]/home/components/chat-panel-layout.tsx index e96f98a01fd..5a30d9bb490 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/components/chat-panel-layout.tsx +++ b/apps/sim/app/workspace/[workspaceId]/home/components/chat-panel-layout.tsx @@ -63,7 +63,7 @@ export function ChatPanelLayout({ {collapsed && activityCount > 0 && ( From 558c77ce359ae1c55b37f1bdc2fe1aa28575e730 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 23 Sep 2026 15:03:09 -0700 Subject: [PATCH 6/7] fix(knowledge): document connector auth in the v2 contract and reject $NAME secrets --- apps/docs/content/docs/cli/knowledge.mdx | 4 +-- apps/docs/content/docs/cli/reference.mdx | 4 +-- apps/docs/openapi-v2-knowledge.json | 4 +-- apps/docs/openapi-v2-resources.json | 8 ++--- apps/sim/lib/api/contracts/v2/catalog.ts | 20 ++++++++--- apps/sim/lib/api/contracts/v2/knowledge.ts | 8 +++-- .../knowledge/application/connectors.test.ts | 31 +++++++++++++++++ .../lib/knowledge/application/connectors.ts | 33 +++++++++++++++++-- .../lib/knowledge/orchestration/connectors.ts | 2 +- packages/sim-cli/src/generated/v2-api.ts | 6 ++-- 10 files changed, 99 insertions(+), 21 deletions(-) diff --git a/apps/docs/content/docs/cli/knowledge.mdx b/apps/docs/content/docs/cli/knowledge.mdx index 08f919e125f..01a792b744b 100644 --- a/apps/docs/content/docs/cli/knowledge.mdx +++ b/apps/docs/content/docs/cli/knowledge.mdx @@ -657,8 +657,8 @@ Create Knowledge Connector (OAuth login or personal API key required) | Option | Required | Description | | --- | --- | --- | | `--connector-type ` | Yes | Registered connector type. | -| `--credential-id ` | No | OAuth credential identifier for connectors that require OAuth. | -| `--api-key ` | No | Write-only API key for connectors that use API-key authentication. | +| `--credential-id ` | No | OAuth credential identifier for connector types whose `auth.mode` is `oauth` (see connector types); omit it for `apiKey` connectors. | +| `--api-key ` | No | Write-only API key for connector types whose `auth.mode` is `apiKey` (see connector types), or a personal access token for an OAuth connector that also accepts one, such as GitHub. Send it instead of `credentialId`. Pass a raw key, or a secret reference written as the whole value `{{SECRET_NAME}}`, which the server resolves; `$SECRET_NAME` is not a reference. | | `--source-config ` | Yes | Connector-specific source selection and filtering configuration. (JSON, or @path / @- to read a file or stdin). | | `--sync-interval-minutes ` | No | Scheduled synchronization interval in minutes; zero disables scheduling. | diff --git a/apps/docs/content/docs/cli/reference.mdx b/apps/docs/content/docs/cli/reference.mdx index cec31d1d71b..e98808fa707 100644 --- a/apps/docs/content/docs/cli/reference.mdx +++ b/apps/docs/content/docs/cli/reference.mdx @@ -2162,8 +2162,8 @@ sim knowledge connectors create [options] | Option | Required | Description | | --- | --- | --- | | `--connector-type ` | Yes | Registered connector type. | -| `--credential-id ` | No | OAuth credential identifier for connectors that require OAuth. | -| `--api-key ` | No | Write-only API key for connectors that use API-key authentication. | +| `--credential-id ` | No | OAuth credential identifier for connector types whose `auth.mode` is `oauth` (see connector types); omit it for `apiKey` connectors. | +| `--api-key ` | No | Write-only API key for connector types whose `auth.mode` is `apiKey` (see connector types), or a personal access token for an OAuth connector that also accepts one, such as GitHub. Send it instead of `credentialId`. Pass a raw key, or a secret reference written as the whole value `{{SECRET_NAME}}`, which the server resolves; `$SECRET_NAME` is not a reference. | | `--source-config ` | Yes | Connector-specific source selection and filtering configuration. (JSON, or @path / @- to read a file or stdin). | | `--sync-interval-minutes ` | No | Scheduled synchronization interval in minutes; zero disables scheduling. | diff --git a/apps/docs/openapi-v2-knowledge.json b/apps/docs/openapi-v2-knowledge.json index 57e462048b7..7a693d01088 100644 --- a/apps/docs/openapi-v2-knowledge.json +++ b/apps/docs/openapi-v2-knowledge.json @@ -5449,13 +5449,13 @@ "description": "Registered connector type." }, "credentialId": { - "description": "OAuth credential identifier for connectors that require OAuth.", + "description": "OAuth credential identifier for connector types whose `auth.mode` is `oauth` (see connector types); omit it for `apiKey` connectors.", "type": "string", "minLength": 1, "maxLength": 255 }, "apiKey": { - "description": "Write-only API key for connectors that use API-key authentication.", + "description": "Write-only API key for connector types whose `auth.mode` is `apiKey` (see connector types), or a personal access token for an OAuth connector that also accepts one, such as GitHub. Send it instead of `credentialId`. Pass a raw key, or a secret reference written as the whole value `{{SECRET_NAME}}`, which the server resolves; `$SECRET_NAME` is not a reference.", "type": "string", "minLength": 1, "maxLength": 10000 diff --git a/apps/docs/openapi-v2-resources.json b/apps/docs/openapi-v2-resources.json index 8dc9eee4318..e9ec3dde864 100644 --- a/apps/docs/openapi-v2-resources.json +++ b/apps/docs/openapi-v2-resources.json @@ -15414,7 +15414,7 @@ "mode": { "type": "string", "const": "oauth", - "description": "Authenticates with an OAuth credential." + "description": "Authenticates with an OAuth credential; pass `credentialId` when creating the connector." }, "provider": { "type": "string", @@ -15437,7 +15437,7 @@ "mode": { "type": "string", "const": "apiKey", - "description": "Authenticates with a stored API key." + "description": "Authenticates with a stored API key; pass `apiKey`, and no `credentialId`, when creating the connector." }, "label": { "description": "Label shown above the key field.", @@ -15456,7 +15456,7 @@ "additionalProperties": false } ], - "description": "How the connector authenticates against its source." + "description": "How the connector authenticates against its source: `oauth` connectors take `credentialId` (GitHub also accepts a personal access token as `apiKey`), `apiKey` connectors take `apiKey`." }, "configFields": { "type": "array", @@ -15634,7 +15634,7 @@ "mode": { "type": "string", "enum": ["oauth", "apiKey"], - "description": "How the connector authenticates against its source." + "description": "How the connector authenticates against its source: `oauth` connectors take `credentialId` (GitHub also accepts a personal access token as `apiKey`), `apiKey` connectors take `apiKey`." } }, "required": ["mode"], diff --git a/apps/sim/lib/api/contracts/v2/catalog.ts b/apps/sim/lib/api/contracts/v2/catalog.ts index 5bdb4d30895..720b0ec5959 100644 --- a/apps/sim/lib/api/contracts/v2/catalog.ts +++ b/apps/sim/lib/api/contracts/v2/catalog.ts @@ -671,7 +671,11 @@ export const v2ConnectorTypeSchema = z auth: z .discriminatedUnion('mode', [ z.object({ - mode: z.literal('oauth').describe('Authenticates with an OAuth credential.'), + mode: z + .literal('oauth') + .describe( + 'Authenticates with an OAuth credential; pass `credentialId` when creating the connector.' + ), provider: z.string().describe('OAuth service the credential must authenticate.'), requiredScopes: z .array(z.string()) @@ -679,7 +683,11 @@ export const v2ConnectorTypeSchema = z .describe('Scopes the credential must carry.'), }), z.object({ - mode: z.literal('apiKey').describe('Authenticates with a stored API key.'), + mode: z + .literal('apiKey') + .describe( + 'Authenticates with a stored API key; pass `apiKey`, and no `credentialId`, when creating the connector.' + ), label: z.string().optional().describe('Label shown above the key field.'), placeholder: z.string().optional().describe('Placeholder shown in the key field.'), optional: z @@ -689,7 +697,9 @@ export const v2ConnectorTypeSchema = z ), }), ]) - .describe('How the connector authenticates against its source.'), + .describe( + 'How the connector authenticates against its source: `oauth` connectors take `credentialId` (GitHub also accepts a personal access token as `apiKey`), `apiKey` connectors take `apiKey`.' + ), configFields: z .array(v2ConnectorConfigFieldSchema) .describe('Fields that make up the connector’s `sourceConfig`.'), @@ -731,7 +741,9 @@ export const v2ConnectorTypeSummarySchema = z .object({ mode: z .enum(['oauth', 'apiKey']) - .describe('How the connector authenticates against its source.'), + .describe( + 'How the connector authenticates against its source: `oauth` connectors take `credentialId` (GitHub also accepts a personal access token as `apiKey`), `apiKey` connectors take `apiKey`.' + ), }) .describe( 'Authentication mode only. `detail=full` adds the OAuth provider and scopes, or the API-key field labels.' diff --git a/apps/sim/lib/api/contracts/v2/knowledge.ts b/apps/sim/lib/api/contracts/v2/knowledge.ts index 1d192b23e5b..3aed6d903f0 100644 --- a/apps/sim/lib/api/contracts/v2/knowledge.ts +++ b/apps/sim/lib/api/contracts/v2/knowledge.ts @@ -1730,13 +1730,17 @@ export const v2CreateKnowledgeConnectorBodySchema = z .min(1) .max(255) .optional() - .describe('OAuth credential identifier for connectors that require OAuth.'), + .describe( + 'OAuth credential identifier for connector types whose `auth.mode` is `oauth` (see connector types); omit it for `apiKey` connectors.' + ), apiKey: z .string() .min(1) .max(10_000) .optional() - .describe('Write-only API key for connectors that use API-key authentication.'), + .describe( + 'Write-only API key for connector types whose `auth.mode` is `apiKey` (see connector types), or a personal access token for an OAuth connector that also accepts one, such as GitHub. Send it instead of `credentialId`. Pass a raw key, or a secret reference written as the whole value `{{SECRET_NAME}}`, which the server resolves; `$SECRET_NAME` is not a reference.' + ), sourceConfig: z .record(z.string(), z.unknown().describe('Connector-specific source configuration value.')) .describe('Connector-specific source selection and filtering configuration.'), diff --git a/apps/sim/lib/knowledge/application/connectors.test.ts b/apps/sim/lib/knowledge/application/connectors.test.ts index 18783a33c64..490e8bcf037 100644 --- a/apps/sim/lib/knowledge/application/connectors.test.ts +++ b/apps/sim/lib/knowledge/application/connectors.test.ts @@ -294,6 +294,37 @@ describe('knowledge connector application use cases', () => { ) }) + it('rejects a $NAME spelling of an existing secret instead of storing it as the key', async () => { + mocks.resolveEnvironment.mockResolvedValue({ GITLAB_PAT: { value: 'resolved-pat' } }) + await expect( + createKnowledgeConnector.execute({ + principal: patPrincipal, + input: { ...patInput, apiKey: ' $GITLAB_PAT ' }, + }) + ).rejects.toMatchObject({ + code: 'validation', + message: + 'Secret references use {{GITLAB_PAT}}, not $GITLAB_PAT. Pass apiKey as "{{GITLAB_PAT}}" to use the secret.', + }) + expect(mocks.resolveEnvironment).toHaveBeenCalledWith('writer', 'workspace-b', ['GITLAB_PAT']) + expect(mocks.createConnector).not.toHaveBeenCalled() + }) + + it('passes a $-prefixed literal through when no secret has that name', async () => { + mocks.resolveEnvironment.mockResolvedValue({}) + mocks.createConnector.mockResolvedValueOnce({ + success: true, + connector: { id: 'new-connector', connectorType: 'sftp', accessMode: 'workspace' }, + }) + await createKnowledgeConnector.execute({ + principal: patPrincipal, + input: { ...patInput, apiKey: '$Summer2024' }, + }) + expect(mocks.createConnector).toHaveBeenCalledWith( + expect.objectContaining({ apiKey: '$Summer2024' }) + ) + }) + it('refuses workspace-wide or unreviewed source ingestion into the canonical search index', async () => { mocks.resolveKnowledgeBase.mockResolvedValue({ ...crossWorkspaceContext, diff --git a/apps/sim/lib/knowledge/application/connectors.ts b/apps/sim/lib/knowledge/application/connectors.ts index b56231b6cf5..eed5bb642c0 100644 --- a/apps/sim/lib/knowledge/application/connectors.ts +++ b/apps/sim/lib/knowledge/application/connectors.ts @@ -785,14 +785,43 @@ async function summarizeConnectorMembers( return { active: row?.active ?? 0, suspended: row?.suspended ?? 0, stale: row?.stale ?? 0 } } +/** Whole-value `$NAME`, the shell-style spelling of a secret reference that is never resolved. */ +const SHELL_STYLE_SECRET_PATTERN = /^\$([A-Za-z_][A-Za-z0-9_]*)$/ + +/** + * Rejects an API key spelled `$NAME` when the caller has a secret named `NAME`, so the literal + * reference is not sent to the provider and stored as the key. A `$`-prefixed value that names no + * secret passes through, since password-style keys (SFTP, ServiceNow) can legitimately look alike. + */ +async function rejectShellStyleSecretReference( + apiKey: string, + principal: Principal, + workspaceId: string | undefined +): Promise { + const name = apiKey.trim().match(SHELL_STYLE_SECRET_PATTERN)?.[1] + if (!name) return + const userId = resolvePrincipalSubjectUserId(principal) + if (!userId) return + const variables = await resolveEffectiveEnvironmentVariables(userId, workspaceId, [name]) + if (!Object.hasOwn(variables, name)) return + throw new OrchestrationError( + 'validation', + `Secret references use {{${name}}}, not $${name}. Pass apiKey as "{{${name}}}" to use the secret.` + ) +} + /** Resolves a secret reference at setup time; the connector stores an encrypted token snapshot. */ async function resolveConnectorApiKey( apiKey: string | undefined, principal: Principal, workspaceId: string | undefined ): Promise { - const name = apiKey?.trim().match(/^\{\{\s*([A-Za-z_][A-Za-z0-9_]*)\s*\}\}$/)?.[1] - if (!name) return apiKey + if (apiKey === undefined) return undefined + const name = apiKey.trim().match(/^\{\{\s*([A-Za-z_][A-Za-z0-9_]*)\s*\}\}$/)?.[1] + if (!name) { + await rejectShellStyleSecretReference(apiKey, principal, workspaceId) + return apiKey + } const userId = resolvePrincipalSubjectUserId(principal) if (!userId) { throw new OrchestrationError('forbidden', 'Secret references require a user identity') diff --git a/apps/sim/lib/knowledge/orchestration/connectors.ts b/apps/sim/lib/knowledge/orchestration/connectors.ts index d08071fe1fa..fe2d6bff84e 100644 --- a/apps/sim/lib/knowledge/orchestration/connectors.ts +++ b/apps/sim/lib/knowledge/orchestration/connectors.ts @@ -357,7 +357,7 @@ export async function performCreateKnowledgeConnector( return fail( (configValidation.error && redactKnownSensitiveValues(configValidation.error, [accessToken])) || - `The ${connectorType} connector rejected sourceConfig without a reason — re-check its required fields in knowledgebases/connectors/${connectorType}.json before retrying; the same config will fail again.`, + `The ${connectorType} connector rejected sourceConfig without a reason — re-check its required fields with \`sim connector-types list --detail full\` (GET /api/v2/connector-types?detail=full) before retrying; the same config will fail again.`, 'validation' ) } diff --git a/packages/sim-cli/src/generated/v2-api.ts b/packages/sim-cli/src/generated/v2-api.ts index 4e6380ca540..ee0be100ac4 100644 --- a/packages/sim-cli/src/generated/v2-api.ts +++ b/packages/sim-cli/src/generated/v2-api.ts @@ -14802,11 +14802,13 @@ export const V2_OPERATIONS = { connectorType: { kind: 'string', required: true, describe: 'Registered connector type.' }, credentialId: { kind: 'string', - describe: 'OAuth credential identifier for connectors that require OAuth.', + describe: + 'OAuth credential identifier for connector types whose `auth.mode` is `oauth` (see connector types); omit it for `apiKey` connectors.', }, apiKey: { kind: 'string', - describe: 'Write-only API key for connectors that use API-key authentication.', + describe: + 'Write-only API key for connector types whose `auth.mode` is `apiKey` (see connector types), or a personal access token for an OAuth connector that also accepts one, such as GitHub. Send it instead of `credentialId`. Pass a raw key, or a secret reference written as the whole value `{{SECRET_NAME}}`, which the server resolves; `$SECRET_NAME` is not a reference.', }, sourceConfig: { kind: 'object', From 55e657f2fbc12ae6aa1283c7e688d958bda5194f Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 23 Sep 2026 15:14:56 -0700 Subject: [PATCH 7/7] fix(mothership): resolve Copilot env-reference passwords for chat deploy and file share Copilot now deploys chats and shares files through the v2 API, which stored a whole-value {{NAME}} password literally (or 400'd a short one on length). The application use cases now resolve the reference from the effective environment when, and only when, the caller is an admitted Copilot workspace invocation and the password will actually be stored (password mode; for a file share, only while enabling it), refuse an unset variable by name, and hold the resolved value to the password rules. Every other principal keeps literal semantics. The v2 password fields admit a whole-value reference below the password minimum as one refined string (not a union, which would make the CLI flag JSON-only), still capped at the password maximum; the use cases enforce the 15-character rule on the value actually stored. Connector API-key references share the same principal environment lookup, and the exact reference regex now lives in one module. Unlike the removed Copilot tool path, the resolved password is not recorded in a resolved-secret trace registry: the v2 use case has none, and the password is write-only and never echoed back. --- apps/docs/content/docs/cli/files.mdx | 2 +- apps/docs/content/docs/cli/reference.mdx | 4 +- apps/docs/content/docs/cli/workflows.mdx | 2 +- apps/docs/openapi-v2-files-audit.json | 3 +- apps/docs/openapi-v2-workflows.json | 3 +- .../deployments/chat/route.test.ts | 89 ++++++++++ .../lib/api/contracts/chats.password.test.ts | 24 +++ apps/sim/lib/api/contracts/primitives.ts | 21 +++ .../contracts/public-shares.password.test.ts | 28 +++ .../lib/api/contracts/v2/chat-deployments.ts | 8 +- apps/sim/lib/api/contracts/v2/files.ts | 5 +- .../application/workflow-chat-deployment.ts | 19 +- .../application/environment-reference.test.ts | 108 +++++++++++ .../core/application/environment-reference.ts | 61 +++++++ apps/sim/lib/environment/reference.ts | 10 ++ apps/sim/lib/imap/connection.server.ts | 3 +- .../knowledge/application/connectors.test.ts | 31 ++-- .../lib/knowledge/application/connectors.ts | 11 +- apps/sim/lib/selectors/server/references.ts | 3 +- .../application/share-workspace-file.test.ts | 168 +++++++++++++++++- .../application/share-workspace-file.ts | 43 ++++- packages/sim-cli/src/generated/v2-api.ts | 4 +- 22 files changed, 599 insertions(+), 51 deletions(-) create mode 100644 apps/sim/lib/core/application/environment-reference.test.ts create mode 100644 apps/sim/lib/core/application/environment-reference.ts create mode 100644 apps/sim/lib/environment/reference.ts diff --git a/apps/docs/content/docs/cli/files.mdx b/apps/docs/content/docs/cli/files.mdx index e8934f98bb6..aad0f887ff0 100644 --- a/apps/docs/content/docs/cli/files.mdx +++ b/apps/docs/content/docs/cli/files.mdx @@ -427,7 +427,7 @@ Enable or disable sharing for a file (OAuth login or personal API key required) | --- | --- | --- | | `--is-active ` | Yes | Whether the share should resolve. Disabling preserves the token and the whole access configuration, so re-enabling restores the share as it was; enabling rewrites the credentials the resulting mode does not use. Accepted values: `true`, `false`. | | `--auth-type ` | No | How access to the share is gated. The stored mode is kept when omitted. Enabling `public` clears the stored password and empties `allowedEmails`; `password` empties `allowedEmails`; `email` and `sso` clear the stored password. Accepted values: `public`, `password`, `email`, `sso`. | -| `--password ` | No | Password for a password-gated share. Kept when omitted; enabling `password` with neither a supplied nor a stored password is a 400. | +| `--password ` | No | Password of 15 to 1024 characters for a password-gated share. Kept when omitted; enabling `password` with neither a supplied nor a stored password is a 400. Taken literally, except that a request from the Sim agent resolves a whole-value `{{ENV_VAR}}` reference to that variable before the rules apply. | | `--allowed-emails ` | No | Allowed addresses or `@domain` patterns for email and SSO shares. Kept when omitted; enabling `email` or `sso` with an empty resulting list is a 400. (space-separated, or @path / @- with one value per line; @@value for a literal leading @). | diff --git a/apps/docs/content/docs/cli/reference.mdx b/apps/docs/content/docs/cli/reference.mdx index e98808fa707..459a37af741 100644 --- a/apps/docs/content/docs/cli/reference.mdx +++ b/apps/docs/content/docs/cli/reference.mdx @@ -1185,7 +1185,7 @@ sim files share set [options] | --- | --- | --- | | `--is-active ` | Yes | Whether the share should resolve. Disabling preserves the token and the whole access configuration, so re-enabling restores the share as it was; enabling rewrites the credentials the resulting mode does not use. Accepted values: `true`, `false`. | | `--auth-type ` | No | How access to the share is gated. The stored mode is kept when omitted. Enabling `public` clears the stored password and empties `allowedEmails`; `password` empties `allowedEmails`; `email` and `sso` clear the stored password. Accepted values: `public`, `password`, `email`, `sso`. | -| `--password ` | No | Password for a password-gated share. Kept when omitted; enabling `password` with neither a supplied nor a stored password is a 400. | +| `--password ` | No | Password of 15 to 1024 characters for a password-gated share. Kept when omitted; enabling `password` with neither a supplied nor a stored password is a 400. Taken literally, except that a request from the Sim agent resolves a whole-value `{{ENV_VAR}}` reference to that variable before the rules apply. | | `--allowed-emails ` | No | Allowed addresses or `@domain` patterns for email and SSO shares. Kept when omitted; enabling `email` or `sso` with an empty resulting list is a 400. (space-separated, or @path / @- with one value per line; @@value for a literal leading @). | @@ -6469,7 +6469,7 @@ sim workflows chat publish [options] | `--description ` | No | Description shown to visitors. Omitted clears it. | | `--customizations ` | No | Presentation overrides. Omitted fields take platform defaults. (JSON, or @path / @- to read a file or stdin). | | `--auth-type ` | No | How visitors are gated. `public` leaves the chat open to anyone holding the URL. Accepted values: `public`, `password`, `email`, `sso`. | -| `--password ` | No | Write-only password. Required whenever `authType` is `password`, and rejected otherwise. Never readable back. | +| `--password ` | No | Write-only password of 15 to 1024 characters, not only whitespace. Required whenever `authType` is `password`, and rejected otherwise. Never readable back. Taken literally, except that a request from the Sim agent resolves a whole-value `{{ENV_VAR}}` reference to that variable before the rules apply. | | `--allowed-emails ` | No | Email addresses or domains admitted under `email` and `sso` gating. At least one is required for those modes. (JSON, or @path / @- to read a file or stdin). | | `--output-configs ` | No | Block outputs to surface to visitors. Omitted surfaces none. (JSON, or @path / @- to read a file or stdin). | | `--include-thinking` | No | Allow visitors to receive provider thinking events. | diff --git a/apps/docs/content/docs/cli/workflows.mdx b/apps/docs/content/docs/cli/workflows.mdx index 786e93d54db..f2c37240f5e 100644 --- a/apps/docs/content/docs/cli/workflows.mdx +++ b/apps/docs/content/docs/cli/workflows.mdx @@ -441,7 +441,7 @@ Publish or replace a workflow’s chat deployment (OAuth login or personal API k | `--description ` | No | Description shown to visitors. Omitted clears it. | | `--customizations ` | No | Presentation overrides. Omitted fields take platform defaults. (JSON, or @path / @- to read a file or stdin). | | `--auth-type ` | No | How visitors are gated. `public` leaves the chat open to anyone holding the URL. Accepted values: `public`, `password`, `email`, `sso`. | -| `--password ` | No | Write-only password. Required whenever `authType` is `password`, and rejected otherwise. Never readable back. | +| `--password ` | No | Write-only password of 15 to 1024 characters, not only whitespace. Required whenever `authType` is `password`, and rejected otherwise. Never readable back. Taken literally, except that a request from the Sim agent resolves a whole-value `{{ENV_VAR}}` reference to that variable before the rules apply. | | `--allowed-emails ` | No | Email addresses or domains admitted under `email` and `sso` gating. At least one is required for those modes. (JSON, or @path / @- to read a file or stdin). | | `--output-configs ` | No | Block outputs to surface to visitors. Omitted surfaces none. (JSON, or @path / @- to read a file or stdin). | | `--include-thinking` | No | Allow visitors to receive provider thinking events. | diff --git a/apps/docs/openapi-v2-files-audit.json b/apps/docs/openapi-v2-files-audit.json index c4da561b051..9dfb8b96b8e 100644 --- a/apps/docs/openapi-v2-files-audit.json +++ b/apps/docs/openapi-v2-files-audit.json @@ -5785,9 +5785,8 @@ "enum": ["public", "password", "email", "sso"] }, "password": { - "description": "Password for a password-gated share. Kept when omitted; enabling `password` with neither a supplied nor a stored password is a 400.", + "description": "Password of 15 to 1024 characters for a password-gated share. Kept when omitted; enabling `password` with neither a supplied nor a stored password is a 400. Taken literally, except that a request from the Sim agent resolves a whole-value `{{ENV_VAR}}` reference to that variable before the rules apply.", "type": "string", - "minLength": 15, "maxLength": 1024 }, "allowedEmails": { diff --git a/apps/docs/openapi-v2-workflows.json b/apps/docs/openapi-v2-workflows.json index 61b1749fedb..9e1495379ee 100644 --- a/apps/docs/openapi-v2-workflows.json +++ b/apps/docs/openapi-v2-workflows.json @@ -11558,9 +11558,8 @@ "enum": ["public", "password", "email", "sso"] }, "password": { - "description": "Write-only password. Required whenever `authType` is `password`, and rejected otherwise. Never readable back.", + "description": "Write-only password of 15 to 1024 characters, not only whitespace. Required whenever `authType` is `password`, and rejected otherwise. Never readable back. Taken literally, except that a request from the Sim agent resolves a whole-value `{{ENV_VAR}}` reference to that variable before the rules apply.", "type": "string", - "minLength": 1, "maxLength": 1024 }, "allowedEmails": { diff --git a/apps/sim/app/api/v2/workflows/[workflowId]/deployments/chat/route.test.ts b/apps/sim/app/api/v2/workflows/[workflowId]/deployments/chat/route.test.ts index aa313d22c9c..baf22187627 100644 --- a/apps/sim/app/api/v2/workflows/[workflowId]/deployments/chat/route.test.ts +++ b/apps/sim/app/api/v2/workflows/[workflowId]/deployments/chat/route.test.ts @@ -2,8 +2,10 @@ * @vitest-environment node */ import { + environmentUtilsMockFns, MockV2ApiKeyUnauthenticatedError, resetDbChainMock, + resetEnvironmentUtilsMock, resetEnvMock, setEnv, V2_OPERATION_RATE_LIMIT_ALLOWED, @@ -72,6 +74,8 @@ vi.mock('@/ee/access-control/utils/permission-check', () => ({ vi.mock('@/lib/api/server/routes/v2-api-key-auth', () => v2ApiKeyAuthModuleMock) vi.mock('@/lib/core/rate-limiter', () => v2RateLimiterModuleMock) +import { markCopilotRequest } from '@/lib/api/server/routes/copilot-request' +import { performChatDeploy as realPerformChatDeploy } from '@/lib/workflows/orchestration/chat-deploy' import { DELETE, GET, PUT } from '@/app/api/v2/workflows/[workflowId]/deployments/chat/route' const WORKSPACE_ID = 'workspace-1' @@ -481,6 +485,91 @@ describe('/api/v2/workflows/[workflowId]/deployments/chat', () => { expect(mocks.validateChatDeployAuth).not.toHaveBeenCalled() }) + + describe('password references', () => { + const passwordBody = (password: string) => ({ ...validBody, authType: 'password', password }) + + /** Admitted exactly as the Sim agent's in-process CLI transport admits its calls. */ + const agentPut = (body: unknown) => { + const request = new NextRequest(PATH, { + method: 'PUT', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify(body), + }) + markCopilotRequest(request, { userId: 'user-1', workspaceId: WORKSPACE_ID, chatId: 'c-1' }) + return PUT(request, routeContext) + } + + const environment = (variables: Record) => + environmentUtilsMockFns.mockResolveEffectiveEnvironmentVariables.mockResolvedValueOnce( + Object.fromEntries( + Object.entries(variables).map(([name, value]) => [ + name, + { value, scope: 'workspace', visible: false }, + ]) + ) + ) + + afterEach(resetEnvironmentUtilsMock) + + it("deploys with the value of the agent's referenced variable", async () => { + environment({ CHAT_PW: 'resolved-chat-password' }) + + const response = await agentPut(passwordBody('{{CHAT_PW}}')) + + expect(response.status).toBe(200) + expect( + environmentUtilsMockFns.mockResolveEffectiveEnvironmentVariables + ).toHaveBeenCalledWith('user-1', WORKSPACE_ID, ['CHAT_PW']) + expect(mocks.performChatDeploy.mock.calls[0][0].password).toBe('resolved-chat-password') + }) + + it('refuses an unset variable by name instead of deploying the placeholder', async () => { + const response = await agentPut(passwordBody('{{CHAT_PW}}')) + + expect(response.status).toBe(400) + expect((await response.json()).error.message).toBe( + 'Environment variable "CHAT_PW" referenced by password is not set for this workspace or user. Set it first, or pass the raw value.' + ) + expect(mocks.performChatDeploy).not.toHaveBeenCalled() + }) + + it('holds the resolved value to the chat password rules', async () => { + mocks.performChatDeploy.mockImplementation(realPerformChatDeploy) + environment({ CHAT_PW: 'short' }) + + const response = await agentPut(passwordBody('{{CHAT_PW}}')) + + expect(response.status).toBe(400) + expect((await response.json()).error.message).toBe( + 'Password must be at least 15 characters' + ) + }) + + it('keeps a reference literal for an API key caller, under the same rules', async () => { + mocks.performChatDeploy.mockImplementation(realPerformChatDeploy) + + const response = await put(passwordBody('{{SHORT}}')) + + expect(response.status).toBe(400) + expect((await response.json()).error.message).toBe( + 'Password must be at least 15 characters' + ) + expect( + environmentUtilsMockFns.mockResolveEffectiveEnvironmentVariables + ).not.toHaveBeenCalled() + }) + + it('stores a long literal reference verbatim for an API key caller', async () => { + const response = await put(passwordBody('{{A_LONG_LITERAL_NAME}}')) + + expect(response.status).toBe(200) + expect(mocks.performChatDeploy.mock.calls[0][0].password).toBe('{{A_LONG_LITERAL_NAME}}') + expect( + environmentUtilsMockFns.mockResolveEffectiveEnvironmentVariables + ).not.toHaveBeenCalled() + }) + }) }) describe('DELETE', () => { diff --git a/apps/sim/lib/api/contracts/chats.password.test.ts b/apps/sim/lib/api/contracts/chats.password.test.ts index b594a4d1a03..d40c17cd742 100644 --- a/apps/sim/lib/api/contracts/chats.password.test.ts +++ b/apps/sim/lib/api/contracts/chats.password.test.ts @@ -9,6 +9,7 @@ import { deployedChatPostBodySchema, updateChatBodySchema, } from '@/lib/api/contracts/chats' +import { v2ReplaceChatDeploymentBodySchema } from '@/lib/api/contracts/v2/chat-deployments' const createBody = { workflowId: 'wf-1', @@ -72,4 +73,27 @@ describe('chat deployment password contract', () => { expect(updateChatBodySchema.safeParse({ password: tooLong }).success).toBe(false) expect(updateChatBodySchema.safeParse({ password: '' }).success).toBe(true) }) + + /** + * Only the use case knows whether the caller is Sim's agent, whose reference + * resolves before the password rules apply, so v2 admits a whole-value + * reference below the password minimum. Every other short value, and the internal surface, + * stay refused with the password message rather than a generic union failure. + */ + it('admits a short whole-value reference on v2 only', () => { + const body = { identifier: 'support', title: 'Support', authType: 'password' } + + expect( + v2ReplaceChatDeploymentBodySchema.safeParse({ ...body, password: '{{PW}}' }).success + ).toBe(true) + expect( + v2ReplaceChatDeploymentBodySchema.safeParse({ ...body, password: 'x-{{PW}}' }).error + ?.issues[0].message + ).toBe('Password must be at least 15 characters') + expect( + v2ReplaceChatDeploymentBodySchema.safeParse({ ...body, password: ' ' }).error?.issues[0] + .message + ).toBe('Password cannot contain only whitespace') + expect(chatDeploymentPasswordSchema.safeParse('{{PW}}').success).toBe(false) + }) }) diff --git a/apps/sim/lib/api/contracts/primitives.ts b/apps/sim/lib/api/contracts/primitives.ts index 984be6ccf19..039699e799b 100644 --- a/apps/sim/lib/api/contracts/primitives.ts +++ b/apps/sim/lib/api/contracts/primitives.ts @@ -1,6 +1,7 @@ import { isPlainRecord } from '@sim/utils/object' import { z } from 'zod' import { setRecordValue } from '@/lib/core/utils/records' +import { EXACT_ENVIRONMENT_REFERENCE } from '@/lib/environment/reference' import { PII_LANGUAGE_CODES, stripNerEntities } from '@/lib/guardrails/pii-entities' import { validateRegexPattern } from '@/lib/guardrails/validate_regex' @@ -142,6 +143,26 @@ export function flattenFieldErrors( export const noInputSchema = z.object({}).strict() export type NoInput = z.output +/** + * `literal`, or a whole-value `{{NAME}}` environment-variable reference that + * `literal` would refuse. A refused non-reference reports `literal`'s own + * messages. Built as one refined string rather than a union so the field keeps a + * plain `string` shape in the generated OpenAPI and CLI, where a union becomes a + * JSON-only flag. `literal`'s length cap bounds references too. + */ +export function orExactEnvironmentReference(literal: z.ZodString) { + const capped = + literal.maxLength === null + ? z.string() + : z.string().max(literal.maxLength, { error: 'Password is too long', abort: true }) + return capped.superRefine((value, ctx) => { + if (EXACT_ENVIRONMENT_REFERENCE.test(value)) return + for (const issue of literal.safeParse(value).error?.issues ?? []) { + ctx.addIssue({ code: 'custom', message: issue.message }) + } + }) +} + /** * Accepts canonical RFC 4648 base64, including the empty encoding used for a * zero-byte file. Padding is required when the final quantum is incomplete, diff --git a/apps/sim/lib/api/contracts/public-shares.password.test.ts b/apps/sim/lib/api/contracts/public-shares.password.test.ts index 7cf090c35ca..bd6aea5c6b2 100644 --- a/apps/sim/lib/api/contracts/public-shares.password.test.ts +++ b/apps/sim/lib/api/contracts/public-shares.password.test.ts @@ -33,4 +33,32 @@ describe('public file share password contracts', () => { it('continues accepting legacy short passwords at the public login gate', () => { expect(authenticatePublicFileBodySchema.safeParse({ password: 'legacy' }).success).toBe(true) }) + + /** + * Only the use case knows whether the caller is Sim's agent, whose reference + * resolves before the password rules apply, so v2 admits a whole-value + * reference below the password minimum. Every other short value, and the internal surface, + * stay refused with the password message rather than a generic union failure. + */ + it('admits a short whole-value reference on v2 only', () => { + const body = { workspaceId: 'workspace-1', isActive: true, authType: 'password' } + + expect(v2UpsertFileShareBodySchema.safeParse({ ...body, password: '{{PW}}' }).success).toBe( + true + ) + expect( + v2UpsertFileShareBodySchema.safeParse({ ...body, password: 'x-{{PW}}' }).error?.issues[0] + .message + ).toBe('Password must be at least 15 characters') + expect(sharePasswordSchema.safeParse('{{PW}}').success).toBe(false) + }) + + it('caps a reference at the password length limit with one issue', () => { + const body = { workspaceId: 'workspace-1', isActive: true, authType: 'password' } + const issues = v2UpsertFileShareBodySchema.safeParse({ + ...body, + password: `{{${'A'.repeat(1024)}}}`, + }).error?.issues + expect(issues?.map((issue) => issue.message)).toEqual(['Password is too long']) + }) }) diff --git a/apps/sim/lib/api/contracts/v2/chat-deployments.ts b/apps/sim/lib/api/contracts/v2/chat-deployments.ts index 8551b02ef9b..77420d073fb 100644 --- a/apps/sim/lib/api/contracts/v2/chat-deployments.ts +++ b/apps/sim/lib/api/contracts/v2/chat-deployments.ts @@ -4,6 +4,7 @@ import { chatAuthTypeSchema, chatDeploymentPasswordSchema } from '@/lib/api/cont import { booleanQueryFlagSchema, noInputSchema, + orExactEnvironmentReference, workflowIdSchema, workspaceIdSchema, } from '@/lib/api/contracts/primitives' @@ -346,11 +347,12 @@ export const v2ReplaceChatDeploymentBodySchema = z * make the verb non-idempotent from the caller's point of view — so a * password-gated result must state its password every time. */ - password: chatDeploymentPasswordSchema - .min(1, 'password cannot be empty') + password: orExactEnvironmentReference( + chatDeploymentPasswordSchema.min(1, 'password cannot be empty') + ) .optional() .describe( - 'Write-only password. Required whenever `authType` is `password`, and rejected otherwise. Never readable back.' + 'Write-only password of 15 to 1024 characters, not only whitespace. Required whenever `authType` is `password`, and rejected otherwise. Never readable back. Taken literally, except that a request from the Sim agent resolves a whole-value `{{ENV_VAR}}` reference to that variable before the rules apply.' ), allowedEmails: chatAllowedEmailsSchema .optional() diff --git a/apps/sim/lib/api/contracts/v2/files.ts b/apps/sim/lib/api/contracts/v2/files.ts index 116bfe1ef2c..d5dbbef8c6f 100644 --- a/apps/sim/lib/api/contracts/v2/files.ts +++ b/apps/sim/lib/api/contracts/v2/files.ts @@ -2,6 +2,7 @@ import { z } from 'zod' import { isCanonicalBase64, noInputSchema, + orExactEnvironmentReference, requiredFieldSchema, versionNumberSchema, workspaceFileIdSchema, @@ -698,10 +699,10 @@ export const v2UpsertFileShareBodySchema = z .describe( 'How access to the share is gated. The stored mode is kept when omitted. Enabling `public` clears the stored password and empties `allowedEmails`; `password` empties `allowedEmails`; `email` and `sso` clear the stored password.' ), - password: sharePasswordSchema + password: orExactEnvironmentReference(sharePasswordSchema) .optional() .describe( - 'Password for a password-gated share. Kept when omitted; enabling `password` with neither a supplied nor a stored password is a 400.' + 'Password of 15 to 1024 characters for a password-gated share. Kept when omitted; enabling `password` with neither a supplied nor a stored password is a 400. Taken literally, except that a request from the Sim agent resolves a whole-value `{{ENV_VAR}}` reference to that variable before the rules apply.' ), allowedEmails: z .array(z.string().min(1, 'allowedEmails entries cannot be empty').max(320)) diff --git a/apps/sim/lib/chat-deployments/application/workflow-chat-deployment.ts b/apps/sim/lib/chat-deployments/application/workflow-chat-deployment.ts index 19ca00b9803..9ac5ae5b10e 100644 --- a/apps/sim/lib/chat-deployments/application/workflow-chat-deployment.ts +++ b/apps/sim/lib/chat-deployments/application/workflow-chat-deployment.ts @@ -27,6 +27,7 @@ import { } from '@/lib/chat-deployments/queries' import { buildChatDeploymentUrl } from '@/lib/chat-deployments/urls' import { defineAuthorizedWorkspaceUseCase } from '@/lib/core/application' +import { resolveCopilotSecretReference } from '@/lib/core/application/environment-reference' import { OrchestrationError } from '@/lib/core/orchestration/types' import { performChatDeploy, performChatUndeploy } from '@/lib/workflows/orchestration' import { validateChatDeployAuth } from '@/ee/access-control/utils/permission-check' @@ -202,6 +203,21 @@ export const replaceWorkflowChatDeployment = defineAuthorizedWorkspaceUseCase({ await assertAuthModePermitted(context, principal, authType) + /** + * Replace semantics: a mode that owns no password stores none. Resolved + * before {@link performChatDeploy}, so its password rules apply to the value + * actually stored rather than to an agent's `{{NAME}}` placeholder. + */ + const password = + authType === 'password' + ? await resolveCopilotSecretReference( + principal, + context.workspaceId, + input.password, + 'password' + ) + : null + const allowedEmails = input.allowedEmails ?? [] const outputConfigs = input.outputConfigs ?? [] const customizations = { @@ -223,8 +239,7 @@ export const replaceWorkflowChatDeployment = defineAuthorizedWorkspaceUseCase({ description: input.description ?? '', customizations, authType, - /** Replace semantics: a mode that owns no password stores none. */ - password: authType === 'password' ? input.password : null, + password, allowedEmails, outputConfigs, includeThinking: input.includeThinking ?? false, diff --git a/apps/sim/lib/core/application/environment-reference.test.ts b/apps/sim/lib/core/application/environment-reference.test.ts new file mode 100644 index 00000000000..7c84506ccac --- /dev/null +++ b/apps/sim/lib/core/application/environment-reference.test.ts @@ -0,0 +1,108 @@ +/** + * @vitest-environment node + */ +import { environmentUtilsMockFns, resetEnvironmentUtilsMock } from '@sim/testing' +import { afterEach, describe, expect, it } from 'vitest' +import { markCopilotWorkspaceInvocation } from '@/lib/core/application/copilot-workspace-invocation' +import { + resolveCopilotSecretReference, + resolvePrincipalEnvironmentVariable, +} from '@/lib/core/application/environment-reference' +import { createCopilotChatPrincipal } from '@/lib/mothership/auth/application-delegation' + +const WORKSPACE_ID = 'workspace-1' + +function copilotPrincipal({ admitted = true } = {}) { + const principal = createCopilotChatPrincipal( + { userId: 'user-1', workspaceId: WORKSPACE_ID, chatId: 'chat-1' }, + 'sim:chat-deployments' + ) + if (admitted) markCopilotWorkspaceInvocation(principal) + return principal +} + +const sessionPrincipal = { kind: 'session' as const, userId: 'user-1', sessionId: 'session-1' } + +function environment(variables: Record) { + environmentUtilsMockFns.mockResolveEffectiveEnvironmentVariables.mockResolvedValueOnce( + Object.fromEntries( + Object.entries(variables).map(([name, value]) => [ + name, + { value, scope: 'workspace', visible: false }, + ]) + ) + ) +} + +describe('resolveCopilotSecretReference', () => { + afterEach(resetEnvironmentUtilsMock) + + it("resolves an agent's whole-value reference from the subject user's environment", async () => { + environment({ CHAT_PASSWORD: 'newly-created-password' }) + + await expect( + resolveCopilotSecretReference( + copilotPrincipal(), + WORKSPACE_ID, + '{{ CHAT_PASSWORD }}', + 'password' + ) + ).resolves.toBe('newly-created-password') + expect(environmentUtilsMockFns.mockResolveEffectiveEnvironmentVariables).toHaveBeenCalledWith( + 'user-1', + WORKSPACE_ID, + ['CHAT_PASSWORD'] + ) + }) + + it('refuses an unset variable by name instead of storing the placeholder', async () => { + await expect( + resolveCopilotSecretReference(copilotPrincipal(), WORKSPACE_ID, '{{CHAT_PW}}', 'password') + ).rejects.toMatchObject({ + code: 'validation', + message: + 'Environment variable "CHAT_PW" referenced by password is not set for this workspace or user. Set it first, or pass the raw value.', + }) + }) + + it('refuses an empty variable the same way as an unset one', async () => { + environment({ CHAT_PW: '' }) + + await expect( + resolveCopilotSecretReference(copilotPrincipal(), WORKSPACE_ID, '{{CHAT_PW}}', 'password') + ).rejects.toMatchObject({ code: 'validation', message: expect.stringContaining('"CHAT_PW"') }) + }) + + it('leaves literal and embedded-reference passwords alone without loading the environment', async () => { + for (const value of ['$literal_password', 'prefix-{{CHAT_PW}}', undefined]) { + await expect( + resolveCopilotSecretReference(copilotPrincipal(), WORKSPACE_ID, value, 'password') + ).resolves.toBe(value) + } + expect(environmentUtilsMockFns.mockResolveEffectiveEnvironmentVariables).not.toHaveBeenCalled() + }) + + it('keeps literal semantics for every caller that is not an admitted agent invocation', async () => { + for (const principal of [sessionPrincipal, copilotPrincipal({ admitted: false })]) { + await expect( + resolveCopilotSecretReference(principal, WORKSPACE_ID, '{{CHAT_PW}}', 'password') + ).resolves.toBe('{{CHAT_PW}}') + } + expect(environmentUtilsMockFns.mockResolveEffectiveEnvironmentVariables).not.toHaveBeenCalled() + }) +}) + +describe('resolvePrincipalEnvironmentVariable', () => { + afterEach(resetEnvironmentUtilsMock) + + it('refuses a principal with no subject user', async () => { + await expect( + resolvePrincipalEnvironmentVariable( + { kind: 'workspace_api_key', workspaceId: WORKSPACE_ID, keyId: 'key-1' }, + WORKSPACE_ID, + 'CHAT_PW' + ) + ).rejects.toMatchObject({ code: 'forbidden' }) + expect(environmentUtilsMockFns.mockResolveEffectiveEnvironmentVariables).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/lib/core/application/environment-reference.ts b/apps/sim/lib/core/application/environment-reference.ts new file mode 100644 index 00000000000..fb5a83b8c69 --- /dev/null +++ b/apps/sim/lib/core/application/environment-reference.ts @@ -0,0 +1,61 @@ +import { type Principal, resolvePrincipalSubjectUserId } from '@sim/auth/principal' +import { isCopilotWorkspaceInvocation } from '@/lib/core/application/copilot-workspace-invocation' +import { OrchestrationError } from '@/lib/core/orchestration/types' +import { parseExactEnvironmentReference } from '@/lib/environment/reference' +import { resolveEffectiveEnvironmentVariables } from '@/lib/environment/utils' + +/** + * Resolves one environment variable for the principal's subject user through a + * fresh ACL-aware lookup (workspace values override personal ones). + * + * Returns `undefined` when the variable is unset, inaccessible, undecryptable, or + * empty, so callers phrase their own refusal. Throws `forbidden` for a principal + * with no subject user, which has no personal or member-scoped environment. + */ +export async function resolvePrincipalEnvironmentVariable( + principal: Principal, + workspaceId: string | undefined, + name: string +): Promise { + const userId = resolvePrincipalSubjectUserId(principal) + if (!userId) { + throw new OrchestrationError('forbidden', 'Secret references require a user identity') + } + const variables = await resolveEffectiveEnvironmentVariables(userId, workspaceId, [name]) + const value = Object.hasOwn(variables, name) ? variables[name].value : undefined + return value || undefined +} + +/** + * Resolves a whole-value `{{NAME}}` secret argument sent by Sim's agent. + * + * The agent never sees secret values — the workspace exposes variable names + * only — so "use the password in CHAT_PW" arrives as the literal `{{CHAT_PW}}`, + * and storing it verbatim would make the placeholder the real secret. Only the + * explicit braced form resolves: passwords are free-form strings, so a looser + * heuristic would corrupt real ones. Every other principal keeps literal + * semantics, since a public API caller already holds the value it means. + * + * Returns the resolved value, or `value` unchanged when it is not a reference or + * the caller is not a Copilot workspace invocation. An unset or empty variable is + * a `validation` error naming it, so the agent learns the actual fix instead of + * silently storing the placeholder. + */ +export async function resolveCopilotSecretReference( + principal: Principal, + workspaceId: string, + value: string | undefined, + argName: string +): Promise { + if (principal.kind !== 'delegated' || !isCopilotWorkspaceInvocation(principal)) return value + const name = parseExactEnvironmentReference(value) + if (!name) return value + const resolved = await resolvePrincipalEnvironmentVariable(principal, workspaceId, name) + if (resolved === undefined) { + throw new OrchestrationError( + 'validation', + `Environment variable "${name}" referenced by ${argName} is not set for this workspace or user. Set it first, or pass the raw value.` + ) + } + return resolved +} diff --git a/apps/sim/lib/environment/reference.ts b/apps/sim/lib/environment/reference.ts new file mode 100644 index 00000000000..71ff496ca93 --- /dev/null +++ b/apps/sim/lib/environment/reference.ts @@ -0,0 +1,10 @@ +/** + * A value that is exactly one `{{NAME}}` environment-variable reference, with the + * variable name captured. Embedded references (`prefix-{{NAME}}`) do not match. + */ +export const EXACT_ENVIRONMENT_REFERENCE = /^\{\{\s*([A-Za-z_][A-Za-z0-9_]*)\s*\}\}$/ + +/** Returns the referenced variable name when `value` is a whole-value `{{NAME}}` reference. */ +export function parseExactEnvironmentReference(value: string | undefined): string | undefined { + return value?.match(EXACT_ENVIRONMENT_REFERENCE)?.[1] +} diff --git a/apps/sim/lib/imap/connection.server.ts b/apps/sim/lib/imap/connection.server.ts index 98a922b76dd..16aa9706c19 100644 --- a/apps/sim/lib/imap/connection.server.ts +++ b/apps/sim/lib/imap/connection.server.ts @@ -1,10 +1,9 @@ import { ImapFlow } from 'imapflow' import { validateDatabaseHost } from '@/lib/core/security/input-validation.server' +import { EXACT_ENVIRONMENT_REFERENCE } from '@/lib/environment/reference' import { resolveEffectiveEnvironmentVariables } from '@/lib/environment/utils' import { containsReference } from '@/lib/workflows/sanitization/references' -const EXACT_ENVIRONMENT_REFERENCE = /^\{\{\s*([A-Za-z_][A-Za-z0-9_]*)\s*\}\}$/ - export class ImapConnectionPolicyError extends Error { constructor(readonly code: 'context' | 'hidden_auth' | 'destination' | 'transport') { super('IMAP connection is unavailable') diff --git a/apps/sim/lib/knowledge/application/connectors.test.ts b/apps/sim/lib/knowledge/application/connectors.test.ts index 490e8bcf037..3b267bad572 100644 --- a/apps/sim/lib/knowledge/application/connectors.test.ts +++ b/apps/sim/lib/knowledge/application/connectors.test.ts @@ -294,21 +294,24 @@ describe('knowledge connector application use cases', () => { ) }) - it('rejects a $NAME spelling of an existing secret instead of storing it as the key', async () => { - mocks.resolveEnvironment.mockResolvedValue({ GITLAB_PAT: { value: 'resolved-pat' } }) - await expect( - createKnowledgeConnector.execute({ - principal: patPrincipal, - input: { ...patInput, apiKey: ' $GITLAB_PAT ' }, + it.each([{ GITLAB_PAT: { value: 'resolved-pat' } }, { GITLAB_PAT: { value: '' } }])( + 'rejects a shell-style reference to an existing secret instead of storing it as the key', + async (variables) => { + mocks.resolveEnvironment.mockResolvedValue(variables) + await expect( + createKnowledgeConnector.execute({ + principal: patPrincipal, + input: { ...patInput, apiKey: ' $GITLAB_PAT ' }, + }) + ).rejects.toMatchObject({ + code: 'validation', + message: + 'Secret references use {{GITLAB_PAT}}, not $GITLAB_PAT. Pass apiKey as "{{GITLAB_PAT}}" to use the secret.', }) - ).rejects.toMatchObject({ - code: 'validation', - message: - 'Secret references use {{GITLAB_PAT}}, not $GITLAB_PAT. Pass apiKey as "{{GITLAB_PAT}}" to use the secret.', - }) - expect(mocks.resolveEnvironment).toHaveBeenCalledWith('writer', 'workspace-b', ['GITLAB_PAT']) - expect(mocks.createConnector).not.toHaveBeenCalled() - }) + expect(mocks.resolveEnvironment).toHaveBeenCalledWith('writer', 'workspace-b', ['GITLAB_PAT']) + expect(mocks.createConnector).not.toHaveBeenCalled() + } + ) it('passes a $-prefixed literal through when no secret has that name', async () => { mocks.resolveEnvironment.mockResolvedValue({}) diff --git a/apps/sim/lib/knowledge/application/connectors.ts b/apps/sim/lib/knowledge/application/connectors.ts index eed5bb642c0..f479c8e2378 100644 --- a/apps/sim/lib/knowledge/application/connectors.ts +++ b/apps/sim/lib/knowledge/application/connectors.ts @@ -17,6 +17,7 @@ import { and, asc, desc, eq, inArray, isNull, lt, or, sql } from 'drizzle-orm' import type { ConnectorDocumentFilter } from '@/lib/api/contracts/knowledge/connectors' import type { BillingAttributionSnapshot } from '@/lib/billing/core/billing-attribution' import { requireCurrentHumanRole } from '@/lib/core/application' +import { resolvePrincipalEnvironmentVariable } from '@/lib/core/application/environment-reference' import { requireOrganizationMembership } from '@/lib/core/application/organization-authorization' import { isLiveEnterpriseSearchEnabled } from '@/lib/core/config/env-flags' import { @@ -31,6 +32,7 @@ import { import { redactKnownSensitiveValues } from '@/lib/core/security/redaction' import { generateRequestId } from '@/lib/core/utils/request' import { resolveCredentialTokenIdentity } from '@/lib/credentials/access' +import { parseExactEnvironmentReference } from '@/lib/environment/reference' import { resolveEffectiveEnvironmentVariables } from '@/lib/environment/utils' import { requireKnowledgeMemberAccessAvailable } from '@/lib/knowledge/access/availability' import { knowledgeAccessCondition } from '@/lib/knowledge/access/predicate' @@ -817,17 +819,12 @@ async function resolveConnectorApiKey( workspaceId: string | undefined ): Promise { if (apiKey === undefined) return undefined - const name = apiKey.trim().match(/^\{\{\s*([A-Za-z_][A-Za-z0-9_]*)\s*\}\}$/)?.[1] + const name = parseExactEnvironmentReference(apiKey.trim()) if (!name) { await rejectShellStyleSecretReference(apiKey, principal, workspaceId) return apiKey } - const userId = resolvePrincipalSubjectUserId(principal) - if (!userId) { - throw new OrchestrationError('forbidden', 'Secret references require a user identity') - } - const variables = await resolveEffectiveEnvironmentVariables(userId, workspaceId, [name]) - const value = Object.hasOwn(variables, name) ? variables[name].value : undefined + const value = await resolvePrincipalEnvironmentVariable(principal, workspaceId, name) if (!value) { throw new OrchestrationError('validation', `Secret "${name}" is unavailable or empty`) } diff --git a/apps/sim/lib/selectors/server/references.ts b/apps/sim/lib/selectors/server/references.ts index 42ca40ef360..d72e9043b73 100644 --- a/apps/sim/lib/selectors/server/references.ts +++ b/apps/sim/lib/selectors/server/references.ts @@ -1,3 +1,4 @@ +import { EXACT_ENVIRONMENT_REFERENCE } from '@/lib/environment/reference' import { resolveEffectiveEnvironmentVariables } from '@/lib/environment/utils' import { getSelectorManifestEntry, type ServerSelectorKey } from '@/lib/selectors/manifest' import { SelectorContextUnavailableError } from '@/lib/selectors/server/errors' @@ -7,8 +8,6 @@ import type { } from '@/lib/selectors/server/types' import type { SelectorContext, SelectorRequest } from '@/lib/selectors/types' -const EXACT_ENVIRONMENT_REFERENCE = /^\{\{\s*([A-Za-z_][A-Za-z0-9_]*)\s*\}\}$/ - export interface ResolvedSelectorInputs { context: SelectorContext request: SelectorRequest diff --git a/apps/sim/lib/workspace-files/application/share-workspace-file.test.ts b/apps/sim/lib/workspace-files/application/share-workspace-file.test.ts index 68f2ee2f066..792fca45956 100644 --- a/apps/sim/lib/workspace-files/application/share-workspace-file.test.ts +++ b/apps/sim/lib/workspace-files/application/share-workspace-file.test.ts @@ -1,12 +1,17 @@ /** * @vitest-environment node */ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { environmentUtilsMockFns, resetEnvironmentUtilsMock } from '@sim/testing' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const mocks = vi.hoisted(() => ({ + getShare: vi.fn(), getWorkspaceShares: vi.fn(), + getWorkspaceFile: vi.fn(), + loadFileContext: vi.fn(), loadWorkspace: vi.fn(), resolvePermission: vi.fn(), + upsertFileShare: vi.fn(), })) vi.mock('@sim/platform-authz/workspace', () => ({ @@ -15,22 +20,30 @@ vi.mock('@sim/platform-authz/workspace', () => ({ })) vi.mock('@/lib/public-shares/share-manager', () => ({ - getShareForResource: vi.fn(), + getShareForResource: mocks.getShare, getWorkspaceSharesForResources: mocks.getWorkspaceShares, ShareValidationError: class ShareValidationError extends Error {}, - upsertFileShare: vi.fn(), + upsertFileShare: mocks.upsertFileShare, })) vi.mock('@/lib/uploads/contexts/workspace/workspace-file-manager', () => ({ - getWorkspaceFile: vi.fn(), + getWorkspaceFile: mocks.getWorkspaceFile, loadActiveWorkspaceContext: mocks.loadWorkspace, + loadActiveWorkspaceFileContext: mocks.loadFileContext, })) vi.mock('@/ee/access-control/utils/permission-check', () => ({ validatePublicFileSharing: vi.fn(), })) -import { getWorkspaceFileShares } from '@/lib/workspace-files/application/share-workspace-file' +import type { ShareAuthType } from '@/lib/api/contracts/public-shares' +import { markCopilotWorkspaceInvocation } from '@/lib/core/application/copilot-workspace-invocation' +import { createCopilotChatPrincipal } from '@/lib/mothership/auth/application-delegation' +import { WORKSPACE_FILES_DELEGATION_AUDIENCE } from '@/lib/workspace-files/application/authorization' +import { + getWorkspaceFileShares, + updateWorkspaceFileShare, +} from '@/lib/workspace-files/application/share-workspace-file' import { MAX_WORKSPACE_FILE_BULK_AFFECTED_ITEMS } from '@/lib/workspace-files/limits' const principal = { @@ -80,3 +93,148 @@ describe('getWorkspaceFileShares', () => { expect(mocks.getWorkspaceShares).not.toHaveBeenCalled() }) }) + +describe('updateWorkspaceFileShare password references', () => { + const workspaceContext = { + workspaceId: 'workspace-1', + workspaceOrganizationId: null, + allowPersonalApiKeys: true, + billedAccountUserId: 'billing-owner', + } + + function copilotPrincipal() { + const copilot = createCopilotChatPrincipal( + { userId: 'user-1', workspaceId: 'workspace-1', chatId: 'chat-1' }, + WORKSPACE_FILES_DELEGATION_AUDIENCE + ) + markCopilotWorkspaceInvocation(copilot) + return copilot + } + + function share( + caller: Parameters[0]['principal'], + password: string, + { authType }: { authType?: ShareAuthType } = { authType: 'password' } + ) { + return updateWorkspaceFileShare.execute({ + principal: caller, + input: { + fileId: 'file-1', + assertedWorkspaceId: 'workspace-1', + isActive: true, + authType, + password, + }, + }) + } + + function environment(variables: Record) { + environmentUtilsMockFns.mockResolveEffectiveEnvironmentVariables.mockResolvedValueOnce( + Object.fromEntries( + Object.entries(variables).map(([name, value]) => [ + name, + { value, scope: 'workspace', visible: false }, + ]) + ) + ) + } + + beforeEach(() => { + vi.clearAllMocks() + mocks.resolvePermission.mockResolvedValue('write') + mocks.loadFileContext.mockResolvedValue({ ...workspaceContext, fileId: 'file-1' }) + mocks.getWorkspaceFile.mockResolvedValue({ id: 'file-1', name: 'report.pdf' }) + mocks.getShare.mockResolvedValue(null) + mocks.upsertFileShare.mockResolvedValue({ id: 'share-1', isActive: true }) + }) + + afterEach(resetEnvironmentUtilsMock) + + it("stores the value of the agent's referenced variable, not the placeholder", async () => { + environment({ SHARE_PW: 'resolved-share-password' }) + + await share(copilotPrincipal(), '{{SHARE_PW}}') + + expect(environmentUtilsMockFns.mockResolveEffectiveEnvironmentVariables).toHaveBeenCalledWith( + 'user-1', + 'workspace-1', + ['SHARE_PW'] + ) + expect(mocks.upsertFileShare).toHaveBeenCalledWith( + expect.objectContaining({ password: 'resolved-share-password' }) + ) + }) + + it('refuses an unset variable by name', async () => { + await expect(share(copilotPrincipal(), '{{SHARE_PW}}')).rejects.toMatchObject({ + code: 'validation', + message: expect.stringContaining('Environment variable "SHARE_PW"'), + }) + expect(mocks.upsertFileShare).not.toHaveBeenCalled() + }) + + it('holds the resolved value to the share password rules', async () => { + environment({ SHARE_PW: 'short' }) + + await expect(share(copilotPrincipal(), '{{SHARE_PW}}')).rejects.toMatchObject({ + code: 'validation', + message: 'Password must be at least 15 characters', + }) + expect(mocks.upsertFileShare).not.toHaveBeenCalled() + }) + + it('keeps a reference literal for any other caller, under the same rules', async () => { + await expect(share(principal, '{{SHORT}}')).rejects.toMatchObject({ + code: 'validation', + message: 'Password must be at least 15 characters', + }) + expect(mocks.upsertFileShare).not.toHaveBeenCalled() + + await share(principal, '{{A_LONG_LITERAL_NAME}}') + + expect(mocks.upsertFileShare).toHaveBeenCalledWith( + expect.objectContaining({ password: '{{A_LONG_LITERAL_NAME}}' }) + ) + expect(environmentUtilsMockFns.mockResolveEffectiveEnvironmentVariables).not.toHaveBeenCalled() + }) + + it('leaves a reference unresolved when sharing is turned off', async () => { + await updateWorkspaceFileShare.execute({ + principal: copilotPrincipal(), + input: { + fileId: 'file-1', + assertedWorkspaceId: 'workspace-1', + isActive: false, + authType: 'password', + password: '{{SHARE_PW}}', + }, + }) + expect(environmentUtilsMockFns.mockResolveEffectiveEnvironmentVariables).not.toHaveBeenCalled() + expect(mocks.upsertFileShare).toHaveBeenCalledWith(expect.objectContaining({ isActive: false })) + }) + + it('resolves only when the effective share mode is password', async () => { + mocks.getShare.mockResolvedValue({ authType: 'email' }) + await share(copilotPrincipal(), '{{SHARE_PW}}', {}) + expect(environmentUtilsMockFns.mockResolveEffectiveEnvironmentVariables).not.toHaveBeenCalled() + expect(mocks.upsertFileShare).toHaveBeenCalledWith( + expect.objectContaining({ password: '{{SHARE_PW}}' }) + ) + + mocks.getShare.mockResolvedValue({ authType: 'password' }) + environment({ SHARE_PW: 'resolved-share-password' }) + await share(copilotPrincipal(), '{{SHARE_PW}}', {}) + expect(mocks.upsertFileShare).toHaveBeenLastCalledWith( + expect.objectContaining({ password: 'resolved-share-password' }) + ) + }) + + it('passes a literal password through untouched', async () => { + await share(copilotPrincipal(), 'literal-share-password') + + expect(mocks.upsertFileShare).toHaveBeenCalledWith( + expect.objectContaining({ password: 'literal-share-password' }) + ) + expect(environmentUtilsMockFns.mockResolveEffectiveEnvironmentVariables).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/lib/workspace-files/application/share-workspace-file.ts b/apps/sim/lib/workspace-files/application/share-workspace-file.ts index a0d6b2eabbf..bfb2b0bf4c2 100644 --- a/apps/sim/lib/workspace-files/application/share-workspace-file.ts +++ b/apps/sim/lib/workspace-files/application/share-workspace-file.ts @@ -1,8 +1,14 @@ import { AuditAction, AuditResourceType } from '@sim/audit' -import { resolvePrincipalExecutionActorUserId } from '@sim/auth/principal' +import { type Principal, resolvePrincipalExecutionActorUserId } from '@sim/auth/principal' import { createLogger } from '@sim/logger' -import type { ShareAuthType, ShareRecord } from '@/lib/api/contracts/public-shares' +import { + type ShareAuthType, + type ShareRecord, + sharePasswordSchema, +} from '@/lib/api/contracts/public-shares' +import { resolveCopilotSecretReference } from '@/lib/core/application/environment-reference' import { OrchestrationError } from '@/lib/core/orchestration/types' +import { parseExactEnvironmentReference } from '@/lib/environment/reference' import { getShareForResource, getWorkspaceSharesForResources, @@ -92,6 +98,30 @@ export const getWorkspaceFileShares = defineAuthorizedWorkspaceFileUseCase({ }, }) +/** + * The password to store for a password-gated share. + * + * The v2 contract admits a whole-value `{{NAME}}` reference below the share + * password minimum, because only here is it known whether the caller is Sim's + * agent: the agent's reference resolves to the variable's value, anyone else's + * stays literal, and either way the result is held to the share password rules. + * Other passwords pass through unchanged, with the length rules of the surface + * that admitted them. + */ +async function resolveSharePassword( + principal: Principal, + workspaceId: string, + password: string | undefined +): Promise { + if (!parseExactEnvironmentReference(password)) return password + const resolved = await resolveCopilotSecretReference(principal, workspaceId, password, 'password') + const validated = sharePasswordSchema.safeParse(resolved) + if (!validated.success) { + throw new OrchestrationError('validation', validated.error.issues[0].message) + } + return validated.data +} + export const updateWorkspaceFileShare = defineAuthorizedWorkspaceFileUseCase({ operation: fileOperations.updateShare, async resolveContext({ input }: { input: UpdateWorkspaceFileShareInput }) { @@ -116,11 +146,16 @@ export const updateWorkspaceFileShare = defineAuthorizedWorkspaceFileUseCase({ throw new WorkspaceFileShareNoopError() } + const effectiveAuthType = input.authType ?? existingShare?.authType ?? 'public' if (input.isActive) { - const effectiveAuthType = input.authType ?? existingShare?.authType ?? 'public' await validatePublicFileSharing(userId, context.workspaceId, effectiveAuthType) } + const password = + input.isActive && effectiveAuthType === 'password' + ? await resolveSharePassword(principal, context.workspaceId, input.password) + : input.password + let share: ShareRecord try { share = await upsertFileShare({ @@ -129,7 +164,7 @@ export const updateWorkspaceFileShare = defineAuthorizedWorkspaceFileUseCase({ userId, isActive: input.isActive, authType: input.authType, - password: input.password, + password, allowedEmails: input.allowedEmails, token: input.token, }) diff --git a/packages/sim-cli/src/generated/v2-api.ts b/packages/sim-cli/src/generated/v2-api.ts index ee0be100ac4..d2c0920c604 100644 --- a/packages/sim-cli/src/generated/v2-api.ts +++ b/packages/sim-cli/src/generated/v2-api.ts @@ -20257,7 +20257,7 @@ export const V2_OPERATIONS = { password: { kind: 'string', describe: - 'Write-only password. Required whenever `authType` is `password`, and rejected otherwise. Never readable back.', + 'Write-only password of 15 to 1024 characters, not only whitespace. Required whenever `authType` is `password`, and rejected otherwise. Never readable back. Taken literally, except that a request from the Sim agent resolves a whole-value `{{ENV_VAR}}` reference to that variable before the rules apply.', }, allowedEmails: { kind: 'array', @@ -21635,7 +21635,7 @@ export const V2_OPERATIONS = { password: { kind: 'string', describe: - 'Password for a password-gated share. Kept when omitted; enabling `password` with neither a supplied nor a stored password is a 400.', + 'Password of 15 to 1024 characters for a password-gated share. Kept when omitted; enabling `password` with neither a supplied nor a stored password is a 400. Taken literally, except that a request from the Sim agent resolves a whole-value `{{ENV_VAR}}` reference to that variable before the rules apply.', }, allowedEmails: { kind: 'array',