From 4de3e07102e4cd02865901ef8ae7713485f46f83 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 30 May 2026 11:53:22 -0700 Subject: [PATCH 01/17] fix(security): harden KB file access, SSO domain registration, webhook path isolation, env secrets, and CSV export --- .../app/api/auth/sso/register/route.test.ts | 173 ++++++++++++++++++ apps/sim/app/api/auth/sso/register/route.ts | 42 ++++- apps/sim/app/api/files/authorization.test.ts | 158 ++++++++++++++++ apps/sim/app/api/files/authorization.ts | 90 +++++++-- .../app/api/table/[tableId]/export/route.ts | 20 +- .../workspaces/[id]/environment/route.test.ts | 96 ++++++++++ .../api/workspaces/[id]/environment/route.ts | 16 +- apps/sim/lib/auth/sso/domain.test.ts | 38 ++++ apps/sim/lib/auth/sso/domain.ts | 31 ++++ apps/sim/lib/webhooks/deploy.ts | 40 ++++ apps/sim/lib/webhooks/processor.test.ts | 110 +++++++++-- apps/sim/lib/webhooks/processor.ts | 37 +++- 12 files changed, 813 insertions(+), 38 deletions(-) create mode 100644 apps/sim/app/api/auth/sso/register/route.test.ts create mode 100644 apps/sim/app/api/files/authorization.test.ts create mode 100644 apps/sim/app/api/workspaces/[id]/environment/route.test.ts create mode 100644 apps/sim/lib/auth/sso/domain.test.ts create mode 100644 apps/sim/lib/auth/sso/domain.ts diff --git a/apps/sim/app/api/auth/sso/register/route.test.ts b/apps/sim/app/api/auth/sso/register/route.test.ts new file mode 100644 index 00000000000..a31852e6805 --- /dev/null +++ b/apps/sim/app/api/auth/sso/register/route.test.ts @@ -0,0 +1,173 @@ +/** + * @vitest-environment node + */ +import { createEnvMock, createMockRequest } from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { + mockGetSession, + mockRegisterSSOProvider, + mockHasSSOAccess, + mockValidateUrlWithDNS, + dbState, + memberTable, + ssoProviderTable, +} = vi.hoisted(() => ({ + mockGetSession: vi.fn(), + mockRegisterSSOProvider: vi.fn(), + mockHasSSOAccess: vi.fn(), + mockValidateUrlWithDNS: vi.fn(), + dbState: { members: [] as any[], providers: [] as any[] }, + memberTable: { + userId: 'member.userId', + organizationId: 'member.organizationId', + role: 'member.role', + }, + ssoProviderTable: { + id: 'sso.id', + providerId: 'sso.providerId', + domain: 'sso.domain', + issuer: 'sso.issuer', + userId: 'sso.userId', + organizationId: 'sso.organizationId', + oidcConfig: 'sso.oidcConfig', + samlConfig: 'sso.samlConfig', + }, +})) + +function makeBuilder(rows: any[]): any { + const thenable: any = Promise.resolve(rows) + thenable.where = () => makeBuilder(rows) + thenable.limit = () => Promise.resolve(rows) + thenable.orderBy = () => Promise.resolve(rows) + return thenable +} + +vi.mock('@sim/db', () => ({ + db: { + select: () => ({ + from: (table: unknown) => + makeBuilder(table === memberTable ? dbState.members : dbState.providers), + }), + }, + member: memberTable, + ssoProvider: ssoProviderTable, +})) + +vi.mock('@/lib/auth', () => ({ + getSession: mockGetSession, + auth: { api: { registerSSOProvider: mockRegisterSSOProvider } }, +})) + +vi.mock('@/lib/billing', () => ({ + hasSSOAccess: mockHasSSOAccess, +})) + +vi.mock('@/lib/auth/sso/domain', () => ({ + normalizeSSODomain: (input: unknown): string | null => { + if (typeof input !== 'string') return null + const value = input.trim().toLowerCase() + return /^[a-z0-9-]+(\.[a-z0-9-]+)+$/.test(value) ? value : null + }, +})) + +vi.mock('@/lib/core/security/input-validation.server', () => ({ + validateUrlWithDNS: mockValidateUrlWithDNS, + secureFetchWithPinnedIP: vi.fn(), +})) + +vi.mock('@/lib/core/config/env', () => createEnvMock({ SSO_ENABLED: 'true' })) + +import { POST } from '@/app/api/auth/sso/register/route' + +const OIDC_BODY = { + providerType: 'oidc' as const, + providerId: 'acme-oidc', + issuer: 'https://idp.acme.com', + domain: 'acme.com', + clientId: 'client-id', + clientSecret: 'client-secret', + authorizationEndpoint: 'https://idp.acme.com/authorize', + tokenEndpoint: 'https://idp.acme.com/token', + userInfoEndpoint: 'https://idp.acme.com/userinfo', + jwksEndpoint: 'https://idp.acme.com/jwks', +} + +function request(body: Record) { + return createMockRequest('POST', body) +} + +describe('POST /api/auth/sso/register', () => { + beforeEach(() => { + vi.clearAllMocks() + dbState.members = [] + dbState.providers = [] + mockGetSession.mockResolvedValue({ user: { id: 'u1' } }) + mockHasSSOAccess.mockResolvedValue(true) + mockValidateUrlWithDNS.mockResolvedValue({ isValid: true, resolvedIP: '1.2.3.4' }) + mockRegisterSSOProvider.mockResolvedValue({ providerId: 'acme-oidc' }) + }) + + it('rejects callers without an Enterprise plan', async () => { + mockHasSSOAccess.mockResolvedValue(false) + const res = await POST(request({ ...OIDC_BODY, orgId: 'org1' })) + expect(res.status).toBe(403) + expect(mockRegisterSSOProvider).not.toHaveBeenCalled() + }) + + it('rejects callers who are not an admin/owner of the target org', async () => { + dbState.members = [{ organizationId: 'org1', role: 'member' }] + const res = await POST(request({ ...OIDC_BODY, orgId: 'org1' })) + expect(res.status).toBe(403) + expect(mockRegisterSSOProvider).not.toHaveBeenCalled() + }) + + it('rejects an invalid domain', async () => { + dbState.members = [{ organizationId: 'org1', role: 'owner' }] + const res = await POST(request({ ...OIDC_BODY, domain: 'not-a-domain', orgId: 'org1' })) + expect(res.status).toBe(400) + expect(mockRegisterSSOProvider).not.toHaveBeenCalled() + }) + + it('rejects a domain already registered by another organization', async () => { + dbState.members = [{ organizationId: 'org-attacker', role: 'owner' }] + dbState.providers = [{ domain: 'acme.com', userId: 'u-victim', organizationId: 'org-victim' }] + const res = await POST(request({ ...OIDC_BODY, orgId: 'org-attacker' })) + const json = await res.json() + expect(res.status).toBe(409) + expect(json.code).toBe('SSO_DOMAIN_ALREADY_REGISTERED') + expect(mockRegisterSSOProvider).not.toHaveBeenCalled() + }) + + it('matches conflicts across casing variants', async () => { + dbState.members = [{ organizationId: 'org-attacker', role: 'owner' }] + dbState.providers = [{ domain: 'ACME.com', userId: 'u-victim', organizationId: 'org-victim' }] + const res = await POST(request({ ...OIDC_BODY, orgId: 'org-attacker' })) + expect(res.status).toBe(409) + expect(mockRegisterSSOProvider).not.toHaveBeenCalled() + }) + + it('registers when the domain is unclaimed', async () => { + dbState.members = [{ organizationId: 'org1', role: 'owner' }] + const res = await POST(request({ ...OIDC_BODY, orgId: 'org1' })) + expect(res.status).toBe(200) + expect(mockRegisterSSOProvider).toHaveBeenCalledTimes(1) + }) + + it('allows the owning tenant to update its own provider for the same domain', async () => { + dbState.members = [{ organizationId: 'org1', role: 'owner' }] + dbState.providers = [{ domain: 'acme.com', userId: 'u1', organizationId: 'org1' }] + const res = await POST(request({ ...OIDC_BODY, orgId: 'org1' })) + expect(res.status).toBe(200) + expect(mockRegisterSSOProvider).toHaveBeenCalledTimes(1) + }) + + it('normalizes the domain before persisting it', async () => { + dbState.members = [{ organizationId: 'org1', role: 'owner' }] + const res = await POST(request({ ...OIDC_BODY, domain: 'ACME.com', orgId: 'org1' })) + expect(res.status).toBe(200) + expect(mockRegisterSSOProvider).toHaveBeenCalledTimes(1) + const config = mockRegisterSSOProvider.mock.calls[0][0].body + expect(config.domain).toBe('acme.com') + }) +}) diff --git a/apps/sim/app/api/auth/sso/register/route.ts b/apps/sim/app/api/auth/sso/register/route.ts index 5b285a5bd28..b262038bf73 100644 --- a/apps/sim/app/api/auth/sso/register/route.ts +++ b/apps/sim/app/api/auth/sso/register/route.ts @@ -6,6 +6,7 @@ import { type NextRequest, NextResponse } from 'next/server' import { ssoRegistrationContract } from '@/lib/api/contracts/auth' import { getValidationErrorMessage, parseRequest } from '@/lib/api/server' import { auth, getSession } from '@/lib/auth' +import { normalizeSSODomain } from '@/lib/auth/sso/domain' import { hasSSOAccess } from '@/lib/billing' import { env } from '@/lib/core/config/env' import { @@ -51,7 +52,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => { if (!parsed.success) return parsed.response const body = parsed.data.body - const { providerId, issuer, domain, providerType, mapping, orgId } = body + const { providerId, issuer, providerType, mapping, orgId } = body if (orgId) { const [membership] = await db @@ -67,6 +68,45 @@ export const POST = withRouteHandler(async (request: NextRequest) => { } } + const domain = normalizeSSODomain(body.domain) + if (!domain) { + return NextResponse.json({ error: 'Enter a valid domain like company.com' }, { status: 400 }) + } + + const isOwnedByCaller = (provider: { + userId: string | null + organizationId: string | null + }): boolean => { + if (orgId) return provider.organizationId === orgId + return provider.userId === session.user.id && !provider.organizationId + } + + const existingProviders = await db + .select({ + domain: ssoProvider.domain, + userId: ssoProvider.userId, + organizationId: ssoProvider.organizationId, + }) + .from(ssoProvider) + const conflictingProvider = existingProviders.find( + (provider) => normalizeSSODomain(provider.domain) === domain && !isOwnedByCaller(provider) + ) + + if (conflictingProvider) { + logger.warn('Rejected SSO registration for domain owned by another tenant', { + domain, + orgId, + userId: session.user.id, + }) + return NextResponse.json( + { + error: 'This domain is already registered for SSO by another organization.', + code: 'SSO_DOMAIN_ALREADY_REGISTERED', + }, + { status: 409 } + ) + } + const headers: Record = {} request.headers.forEach((value, key) => { headers[key] = value diff --git a/apps/sim/app/api/files/authorization.test.ts b/apps/sim/app/api/files/authorization.test.ts new file mode 100644 index 00000000000..b8b512ce598 --- /dev/null +++ b/apps/sim/app/api/files/authorization.test.ts @@ -0,0 +1,158 @@ +/** + * @vitest-environment node + * + * Security regression tests for knowledge-base file authorization. + * + * The historical bug: `verifyKBFileAccess` authorized access to a storage object + * whenever ANY active document's `fileUrl` contained the requested key as a + * substring, in a workspace the caller could reach. That let a tenant plant a + * document referencing another tenant's storage key (even via an unrelated + * external URL) and read/delete the victim's file. + * + * These tests pin the fixed behavior: authorization requires a document whose + * fileUrl canonically resolves to EXACTLY the requested key, and access is + * decided against the OWNING (earliest) document's workspace only. + */ +import { dbChainMock, dbChainMockFns, resetDbChainMock } from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +vi.mock('@sim/db', () => dbChainMock) + +const { mockGetUserEntityPermissions } = vi.hoisted(() => ({ + mockGetUserEntityPermissions: vi.fn(), +})) + +vi.mock('@/lib/workspaces/permissions/utils', () => ({ + getUserEntityPermissions: mockGetUserEntityPermissions, +})) + +const { mockGetFileMetadataByKey } = vi.hoisted(() => ({ + mockGetFileMetadataByKey: vi.fn(), +})) + +vi.mock('@/lib/uploads/server/metadata', () => ({ + getFileMetadataByKey: mockGetFileMetadataByKey, +})) + +vi.mock('@/lib/uploads', () => ({ + getFileMetadata: vi.fn().mockResolvedValue({}), +})) + +import { verifyFileAccess } from '@/app/api/files/authorization' + +const VICTIM_KEY = 'kb/1780162789495-victim-secret.txt' +const ATTACKER_USER = 'attacker-user' + +/** Internal serve URL that legitimately resolves to VICTIM_KEY. */ +const internalUrlFor = (key: string) => + `/api/files/serve/s3/${encodeURIComponent(key)}?context=knowledge-base` + +describe('verifyKBFileAccess', () => { + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + mockGetFileMetadataByKey.mockResolvedValue(null) + }) + + it('grants access to the workspace that owns the storage key', async () => { + dbChainMockFns.limit.mockResolvedValueOnce([ + { workspaceId: 'ws-owner', fileUrl: internalUrlFor(VICTIM_KEY) }, + ]) + mockGetUserEntityPermissions.mockResolvedValue('read') + + const granted = await verifyFileAccess(VICTIM_KEY, 'owner-user', undefined, 'knowledge-base') + + expect(granted).toBe(true) + expect(mockGetUserEntityPermissions).toHaveBeenCalledWith('owner-user', 'workspace', 'ws-owner') + }) + + it('denies access via an external URL that merely contains the key as a substring', async () => { + // The demonstrated PoC: a planted document whose fileUrl is an unrelated + // external URL containing the victim key. It must never authorize storage. + dbChainMockFns.limit.mockResolvedValueOnce([ + { + workspaceId: 'ws-attacker', + fileUrl: `https://attacker.example/anything/${VICTIM_KEY}/marker`, + }, + ]) + mockGetUserEntityPermissions.mockResolvedValue('admin') + + const granted = await verifyFileAccess(VICTIM_KEY, ATTACKER_USER, undefined, 'knowledge-base') + + expect(granted).toBe(false) + }) + + it('denies a later document planted in another workspace (ownership pins to earliest doc)', async () => { + // db query orders by uploadedAt asc, so the owning (victim) document comes + // first; the attacker's later internal-URL document must not grant access. + dbChainMockFns.limit.mockResolvedValueOnce([ + { workspaceId: 'ws-victim', fileUrl: internalUrlFor(VICTIM_KEY) }, + { workspaceId: 'ws-attacker', fileUrl: internalUrlFor(VICTIM_KEY) }, + ]) + mockGetUserEntityPermissions.mockImplementation( + async (_userId: string, _type: string, workspaceId: string) => + workspaceId === 'ws-attacker' ? 'admin' : null + ) + + const granted = await verifyFileAccess(VICTIM_KEY, ATTACKER_USER, undefined, 'knowledge-base') + + expect(granted).toBe(false) + // Authorization is decided against the owning workspace only. + expect(mockGetUserEntityPermissions).toHaveBeenCalledWith( + ATTACKER_USER, + 'workspace', + 'ws-victim' + ) + expect(mockGetUserEntityPermissions).not.toHaveBeenCalledWith( + ATTACKER_USER, + 'workspace', + 'ws-attacker' + ) + }) + + it('denies access when a substring document points at a different key', async () => { + dbChainMockFns.limit.mockResolvedValueOnce([ + { workspaceId: 'ws-attacker', fileUrl: internalUrlFor(`${VICTIM_KEY}-decoy`) }, + ]) + mockGetUserEntityPermissions.mockResolvedValue('admin') + + const granted = await verifyFileAccess(VICTIM_KEY, ATTACKER_USER, undefined, 'knowledge-base') + + expect(granted).toBe(false) + }) + + it('denies access when no document references the key', async () => { + dbChainMockFns.limit.mockResolvedValueOnce([]) + + const granted = await verifyFileAccess(VICTIM_KEY, ATTACKER_USER, undefined, 'knowledge-base') + + expect(granted).toBe(false) + expect(mockGetUserEntityPermissions).not.toHaveBeenCalled() + }) + + it('denies when the owning document has no workspace (fail closed)', async () => { + dbChainMockFns.limit.mockResolvedValueOnce([ + { workspaceId: null, fileUrl: internalUrlFor(VICTIM_KEY) }, + ]) + + const granted = await verifyFileAccess(VICTIM_KEY, ATTACKER_USER, undefined, 'knowledge-base') + + expect(granted).toBe(false) + expect(mockGetUserEntityPermissions).not.toHaveBeenCalled() + }) + + it('does not let a later workspace document override a null-workspace owner', async () => { + // The earliest (owning) document belongs to a workspace-less KB; a later + // document planted in the attacker's workspace must not become the owner. + dbChainMockFns.limit.mockResolvedValueOnce([ + { workspaceId: null, fileUrl: internalUrlFor(VICTIM_KEY) }, + { workspaceId: 'ws-attacker', fileUrl: internalUrlFor(VICTIM_KEY) }, + ]) + mockGetUserEntityPermissions.mockResolvedValue('admin') + + const granted = await verifyFileAccess(VICTIM_KEY, ATTACKER_USER, undefined, 'knowledge-base') + + expect(granted).toBe(false) + expect(mockGetUserEntityPermissions).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/app/api/files/authorization.ts b/apps/sim/app/api/files/authorization.ts index 07ec36261b4..f004db07b0b 100644 --- a/apps/sim/app/api/files/authorization.ts +++ b/apps/sim/app/api/files/authorization.ts @@ -1,14 +1,18 @@ import { db } from '@sim/db' import { document, knowledgeBase, workspaceFile } from '@sim/db/schema' import { createLogger } from '@sim/logger' -import { and, eq, isNull, like, or } from 'drizzle-orm' +import { and, asc, eq, isNull, like, or } from 'drizzle-orm' import { NextResponse } from 'next/server' import { getFileMetadata } from '@/lib/uploads' import type { StorageContext } from '@/lib/uploads/config' import { BLOB_CHAT_CONFIG, S3_CHAT_CONFIG } from '@/lib/uploads/config' import type { StorageConfig } from '@/lib/uploads/core/storage-client' import { getFileMetadataByKey } from '@/lib/uploads/server/metadata' -import { inferContextFromKey } from '@/lib/uploads/utils/file-utils' +import { + extractStorageKey, + inferContextFromKey, + isInternalFileUrl, +} from '@/lib/uploads/utils/file-utils' import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils' import { isUuid } from '@/executor/constants' @@ -370,9 +374,37 @@ async function verifyCopilotFileAccess( } } +/** + * Resolve a knowledge-base document's stored `fileUrl` to the canonical storage + * key it points at, but only when the URL is an internal Sim file-serve URL. + * + * External `http(s)://` URLs and `data:` URIs are accepted for ingestion but + * intentionally return `null` here so they can never authorize access to an + * internal storage object. Returns the storage key (e.g. `kb/-name.txt`) + * only for internal knowledge-base URLs, `null` otherwise. + */ +function resolveInternalKbKey(fileUrl: string | null): string | null { + if (!fileUrl || !isInternalFileUrl(fileUrl)) { + return null + } + try { + const key = extractStorageKey(fileUrl) + return key.startsWith('kb/') ? key : null + } catch { + return null + } +} + /** * Verify access to KB files * KB files: kb/filename + * + * Access is authorized against the workspace that *owns* the storage object, + * never against an arbitrary document that merely references it. Ownership is + * resolved by requiring a document's `fileUrl` to canonically resolve to the + * exact requested storage key (not a substring/`LIKE` match), and by pinning to + * the earliest such document — so a later document planted in another workspace + * cannot authorize the planting user against another tenant's file. */ async function verifyKBFileAccess( cloudKey: string, @@ -380,9 +412,13 @@ async function verifyKBFileAccess( customConfig?: StorageConfig ): Promise { try { - const activeKbFileDocuments = await db + // The `LIKE` predicate only narrows candidates cheaply; it never decides + // authorization. Ordering by `uploadedAt` lets us pin ownership to the + // earliest document, which is the one the file was originally uploaded for. + const candidateDocuments = await db .select({ workspaceId: knowledgeBase.workspaceId, + fileUrl: document.fileUrl, }) .from(document) .innerJoin(knowledgeBase, eq(document.knowledgeBaseId, knowledgeBase.id)) @@ -398,26 +434,52 @@ async function verifyKBFileAccess( ) ) ) - .limit(10) - - for (const doc of activeKbFileDocuments) { - if (!doc.workspaceId) { - continue + .orderBy(asc(document.uploadedAt)) + .limit(50) + + // The owner is the earliest document whose fileUrl resolves to EXACTLY this + // storage key. Substring-only matches (e.g. an external URL that happens to + // contain the key) and documents in other workspaces that merely reference + // the key do not establish ownership. Pinning to the earliest match means a + // later document planted in the caller's own workspace can never authorize + // access to a file another workspace originally uploaded. + const owningDocument = candidateDocuments.find( + (doc) => resolveInternalKbKey(doc.fileUrl) === cloudKey + ) + + if (owningDocument) { + if (!owningDocument.workspaceId) { + logger.warn('KB file access denied: owning document has no workspace', { + userId, + cloudKey, + }) + return false } - const permission = await getUserEntityPermissions(userId, 'workspace', doc.workspaceId) + const permission = await getUserEntityPermissions( + userId, + 'workspace', + owningDocument.workspaceId + ) if (permission !== null) { - logger.debug('KB file access granted (active document lookup)', { + logger.debug('KB file access granted (owning document lookup)', { userId, - workspaceId: doc.workspaceId, + workspaceId: owningDocument.workspaceId, cloudKey, }) return true } + logger.warn('KB file access denied: user lacks permission on owning workspace', { + userId, + workspaceId: owningDocument.workspaceId, + cloudKey, + }) + return false } - // KB file access must resolve through an active KB document. Metadata alone is not enough - // because parent archives intentionally keep the underlying file bytes around for history. + // KB file access must resolve through an active KB document that canonically + // owns the key. Metadata alone is not enough because parent archives + // intentionally keep the underlying file bytes around for history. const fileRecord = await getFileMetadataByKey(cloudKey, 'knowledge-base', { includeDeleted: true, }) @@ -427,7 +489,7 @@ async function verifyKBFileAccess( return false } - logger.warn('KB file access denied because no active KB document matched the file', { + logger.warn('KB file access denied because no owning KB document matched the file', { cloudKey, userId, }) diff --git a/apps/sim/app/api/table/[tableId]/export/route.ts b/apps/sim/app/api/table/[tableId]/export/route.ts index 8f9fa34b807..f5702c0b136 100644 --- a/apps/sim/app/api/table/[tableId]/export/route.ts +++ b/apps/sim/app/api/table/[tableId]/export/route.ts @@ -53,7 +53,9 @@ export const GET = withRouteHandler(async (request: NextRequest, { params }: Rou const encoder = new TextEncoder() try { if (format === 'csv') { - controller.enqueue(encoder.encode(`${toCsvRow(columns.map((c) => c.name))}\n`)) + controller.enqueue( + encoder.encode(`${toCsvRow(columns.map((c) => neutralizeCsvFormula(c.name)))}\n`) + ) } else { controller.enqueue(encoder.encode('[')) } @@ -111,10 +113,26 @@ function sanitizeFilename(name: string): string { return cleaned || 'table' } +/** + * Prefixes a single quote to values that spreadsheet applications would otherwise + * interpret as formulas, preventing CSV formula injection when an exported file is + * opened in Excel, LibreOffice, or Google Sheets. JSON exports preserve raw values. + */ +function neutralizeCsvFormula(value: string): string { + return /^[=+\-@\t\r]/.test(value) ? `'${value}` : value +} + +/** + * Serializes a cell for CSV output. Only string cells are run through + * {@link neutralizeCsvFormula}: numbers, booleans, dates, and JSON-serialized + * objects can never stringify into a spreadsheet formula trigger, so they are + * emitted verbatim to preserve fidelity (e.g. negative numbers stay numeric). + */ function formatCsvValue(value: unknown): string { if (value === null || value === undefined) return '' if (value instanceof Date) return value.toISOString() if (typeof value === 'object') return JSON.stringify(value) + if (typeof value === 'string') return neutralizeCsvFormula(value) return String(value) } diff --git a/apps/sim/app/api/workspaces/[id]/environment/route.test.ts b/apps/sim/app/api/workspaces/[id]/environment/route.test.ts new file mode 100644 index 00000000000..c1192f38408 --- /dev/null +++ b/apps/sim/app/api/workspaces/[id]/environment/route.test.ts @@ -0,0 +1,96 @@ +/** + * Tests for the workspace environment API route. + * + * @vitest-environment node + */ +import { authMock, authMockFns, permissionsMock, permissionsMockFns } from '@sim/testing' +import { NextRequest } from 'next/server' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockGetPersonalAndWorkspaceEnv } = vi.hoisted(() => ({ + mockGetPersonalAndWorkspaceEnv: vi.fn(), +})) + +vi.mock('@/lib/auth', () => authMock) +vi.mock('@/lib/workspaces/permissions/utils', () => permissionsMock) +vi.mock('@/lib/environment/utils', () => ({ + getPersonalAndWorkspaceEnv: mockGetPersonalAndWorkspaceEnv, + invalidateEffectiveDecryptedEnvCache: vi.fn(), +})) + +import { GET } from '@/app/api/workspaces/[id]/environment/route' + +const WORKSPACE_ID = 'ws-1' + +function createRequest() { + return new NextRequest(`http://localhost/api/workspaces/${WORKSPACE_ID}/environment`) +} + +function createContext() { + return { params: Promise.resolve({ id: WORKSPACE_ID }) } +} + +const ENV_RESULT = { + personalEncrypted: {}, + workspaceEncrypted: { OPENAI_API_KEY: 'enc-1', DATABASE_URL: 'enc-2' }, + personalDecrypted: { MY_PERSONAL: 'personal-value' }, + workspaceDecrypted: { + OPENAI_API_KEY: 'sk-live-secret-value', + DATABASE_URL: 'postgres://user:password@db/prod', + }, + conflicts: [], + decryptionFailures: [], +} + +describe('GET /api/workspaces/[id]/environment', () => { + beforeEach(() => { + vi.clearAllMocks() + authMockFns.mockGetSession.mockResolvedValue({ user: { id: 'user-1' } }) + permissionsMockFns.mockGetWorkspaceById.mockResolvedValue({ id: WORKSPACE_ID }) + mockGetPersonalAndWorkspaceEnv.mockResolvedValue(ENV_RESULT) + }) + + it('returns decrypted workspace values to workspace admins', async () => { + permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue('admin') + + const res = await GET(createRequest(), createContext()) + expect(res.status).toBe(200) + + const body = await res.json() + expect(body.data.workspace).toEqual(ENV_RESULT.workspaceDecrypted) + expect(body.data.personal).toEqual(ENV_RESULT.personalDecrypted) + }) + + it.each(['write', 'read'] as const)( + 'returns only variable names (empty values) to %s members', + async (permission) => { + permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue(permission) + + const res = await GET(createRequest(), createContext()) + expect(res.status).toBe(200) + + const body = await res.json() + expect(body.data.workspace).toEqual({ OPENAI_API_KEY: '', DATABASE_URL: '' }) + // Plaintext workspace secrets must never reach non-admins. + expect(JSON.stringify(body.data.workspace)).not.toContain('sk-live-secret-value') + expect(JSON.stringify(body.data.workspace)).not.toContain('postgres://') + // The caller's own personal values are still returned. + expect(body.data.personal).toEqual(ENV_RESULT.personalDecrypted) + } + ) + + it('rejects users without any workspace permission', async () => { + permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue(null) + + const res = await GET(createRequest(), createContext()) + expect(res.status).toBe(401) + expect(mockGetPersonalAndWorkspaceEnv).not.toHaveBeenCalled() + }) + + it('rejects unauthenticated requests', async () => { + authMockFns.mockGetSession.mockResolvedValue(null) + + const res = await GET(createRequest(), createContext()) + expect(res.status).toBe(401) + }) +}) diff --git a/apps/sim/app/api/workspaces/[id]/environment/route.ts b/apps/sim/app/api/workspaces/[id]/environment/route.ts index 47c6f4a1e59..c4707e6de46 100644 --- a/apps/sim/app/api/workspaces/[id]/environment/route.ts +++ b/apps/sim/app/api/workspaces/[id]/environment/route.ts @@ -53,15 +53,21 @@ export const GET = withRouteHandler( return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }) } - const { workspaceDecrypted, personalDecrypted, conflicts } = await getPersonalAndWorkspaceEnv( - userId, - workspaceId - ) + const { workspaceEncrypted, workspaceDecrypted, personalDecrypted, conflicts } = + await getPersonalAndWorkspaceEnv(userId, workspaceId) + + // Plaintext workspace secrets are restricted to workspace admins. Non-admins receive only + // the variable names (with empty values) so env references can still be validated and + // highlighted in the editor without exposing the decrypted secret values. + const workspace = + permission === 'admin' + ? workspaceDecrypted + : Object.fromEntries(Object.keys(workspaceEncrypted).map((key) => [key, ''])) return NextResponse.json( { data: { - workspace: workspaceDecrypted, + workspace, personal: personalDecrypted, conflicts, }, diff --git a/apps/sim/lib/auth/sso/domain.test.ts b/apps/sim/lib/auth/sso/domain.test.ts new file mode 100644 index 00000000000..5ca62331a81 --- /dev/null +++ b/apps/sim/lib/auth/sso/domain.test.ts @@ -0,0 +1,38 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { normalizeSSODomain } from '@/lib/auth/sso/domain' + +describe('normalizeSSODomain', () => { + it('lowercases and trims', () => { + expect(normalizeSSODomain(' Company.COM ')).toBe('company.com') + }) + + it('strips protocol, path, query, and port', () => { + expect(normalizeSSODomain('https://company.com/sso?x=1')).toBe('company.com') + expect(normalizeSSODomain('company.com:8443')).toBe('company.com') + }) + + it('strips wildcard, leading @, and email local part', () => { + expect(normalizeSSODomain('*.company.com')).toBe('company.com') + expect(normalizeSSODomain('@company.com')).toBe('company.com') + expect(normalizeSSODomain('user@company.com')).toBe('company.com') + }) + + it('drops a trailing dot', () => { + expect(normalizeSSODomain('company.com.')).toBe('company.com') + }) + + it('treats casing and formatting variants as the same domain', () => { + expect(normalizeSSODomain('Company.COM')).toBe(normalizeSSODomain('company.com')) + expect(normalizeSSODomain('user@Company.com')).toBe(normalizeSSODomain('company.com')) + }) + + it('rejects values that are not registrable domains', () => { + expect(normalizeSSODomain('')).toBeNull() + expect(normalizeSSODomain('localhost')).toBeNull() + expect(normalizeSSODomain('not a domain')).toBeNull() + expect(normalizeSSODomain('company')).toBeNull() + }) +}) diff --git a/apps/sim/lib/auth/sso/domain.ts b/apps/sim/lib/auth/sso/domain.ts new file mode 100644 index 00000000000..f1d77b6ba30 --- /dev/null +++ b/apps/sim/lib/auth/sso/domain.ts @@ -0,0 +1,31 @@ +/** + * Normalizes a user-supplied SSO email domain to a canonical, comparable form. + * + * Strips protocol, path, query, port, leading wildcard/`@`, an email local + * part, surrounding whitespace, and a trailing dot, then lowercases the result. + * Returns `null` when the input does not look like a registrable domain + * (`example.com`), which callers should treat as a validation error. + * + * Used to compare a requested SSO domain against already-registered providers + * so a tenant cannot claim a domain another tenant already owns via casing or + * formatting variants. + */ +export function normalizeSSODomain(input: string): string | null { + if (typeof input !== 'string') return null + + let value = input.trim().toLowerCase() + if (!value) return null + + value = value.replace(/^[a-z][a-z0-9+.-]*:\/\//, '') + value = value.replace(/^\*\./, '').replace(/^@/, '') + value = value.split('/')[0] + value = value.split('?')[0] + value = value.split('@').pop() ?? value + value = value.split(':')[0] + value = value.replace(/\.$/, '') + + if (!/^[a-z0-9-]+(\.[a-z0-9-]+)+$/.test(value)) return null + if (value.split('.').some((label) => label.length === 0 || label.length > 63)) return null + + return value +} diff --git a/apps/sim/lib/webhooks/deploy.ts b/apps/sim/lib/webhooks/deploy.ts index 6f9b97b84f3..61ca1325e8f 100644 --- a/apps/sim/lib/webhooks/deploy.ts +++ b/apps/sim/lib/webhooks/deploy.ts @@ -23,6 +23,29 @@ import { SYSTEM_SUBBLOCK_IDS } from '@/triggers/constants' const logger = createLogger('DeployWebhookSync') const CREDENTIAL_SET_PREFIX = 'credentialSet:' +/** + * Returns the id of a workflow that already owns an active (non-archived) + * webhook on the given path, when that workflow is different from the one being + * deployed. Webhook paths are user-controlled and the database only enforces + * uniqueness per deployment version, so without this guard two tenants could + * register the same public path and a delivery to one would fan out to the + * other. Returns `null` when the path is free or only used by this workflow. + */ +async function findConflictingWebhookPathOwner(params: { + path: string + workflowId: string +}): Promise { + const { path, workflowId } = params + + const existing = await db + .select({ workflowId: webhook.workflowId }) + .from(webhook) + .where(and(eq(webhook.path, path), isNull(webhook.archivedAt))) + + const conflict = existing.find((row) => row.workflowId !== workflowId) + return conflict ? conflict.workflowId : null +} + interface TriggerSaveError { message: string status: number @@ -538,6 +561,23 @@ export async function saveTriggerWebhooksForDeploy({ } } + const pathConflict = await findConflictingWebhookPathOwner({ + path: triggerPath, + workflowId, + }) + if (pathConflict) { + logger.warn( + `[${requestId}] Webhook path conflict for "${triggerPath}": already owned by workflow ${pathConflict}` + ) + return { + success: false, + error: { + message: `Webhook path "${triggerPath}" is already in use. Choose a different path.`, + status: 409, + }, + } + } + webhookConfigs.set(block.id, { provider, providerConfig, triggerPath, triggerDef }) if (providerConfig.credentialSetId) { diff --git a/apps/sim/lib/webhooks/processor.test.ts b/apps/sim/lib/webhooks/processor.test.ts index c5f3894707f..381cc6108a0 100644 --- a/apps/sim/lib/webhooks/processor.test.ts +++ b/apps/sim/lib/webhooks/processor.test.ts @@ -10,23 +10,36 @@ import { } from '@sim/testing' import { beforeEach, describe, expect, it, vi } from 'vitest' -const { mockGenerateId, mockEnqueue, mockGetJobQueue, mockShouldExecuteInline } = vi.hoisted( - () => ({ - mockGenerateId: vi.fn(), - mockEnqueue: vi.fn(), - mockGetJobQueue: vi.fn(), - mockShouldExecuteInline: vi.fn(), - }) -) +const { + mockGenerateId, + mockEnqueue, + mockGetJobQueue, + mockShouldExecuteInline, + mockWebhookLookupResult, +} = vi.hoisted(() => ({ + mockGenerateId: vi.fn(), + mockEnqueue: vi.fn(), + mockGetJobQueue: vi.fn(), + mockShouldExecuteInline: vi.fn(), + mockWebhookLookupResult: { rows: [] as Array<{ webhook: any; workflow: any }> }, +})) const mockPreprocessExecution = executionPreprocessingMockFns.mockPreprocessExecution -vi.mock('@sim/db', () => ({ - db: {}, - webhook: {}, - workflow: {}, - workflowDeploymentVersion: {}, -})) +vi.mock('@sim/db', () => { + const selectChain = { + from: () => selectChain, + innerJoin: () => selectChain, + leftJoin: () => selectChain, + where: () => Promise.resolve(mockWebhookLookupResult.rows), + } + return { + db: { select: () => selectChain }, + webhook: {}, + workflow: {}, + workflowDeploymentVersion: {}, + } +}) vi.mock('drizzle-orm', () => ({ and: vi.fn(), @@ -109,7 +122,74 @@ vi.mock('@/triggers/jira/utils', () => ({ isJiraEventMatch: vi.fn().mockReturnValue(true), })) -import { checkWebhookPreprocessing, queueWebhookExecution } from '@/lib/webhooks/processor' +import { + checkWebhookPreprocessing, + findAllWebhooksForPath, + queueWebhookExecution, +} from '@/lib/webhooks/processor' + +describe('findAllWebhooksForPath cross-tenant collision', () => { + beforeEach(() => { + vi.clearAllMocks() + mockWebhookLookupResult.rows = [] + }) + + const makeRow = (workflowId: string, webhookId: string, createdAt: Date) => ({ + webhook: { id: webhookId, workflowId, path: 'shared-path', createdAt }, + workflow: { id: workflowId }, + }) + + it('returns all rows when they belong to a single workflow (credential set fan-out)', async () => { + mockWebhookLookupResult.rows = [ + makeRow('workflow-1', 'wh-a', new Date('2026-01-01')), + makeRow('workflow-1', 'wh-b', new Date('2026-01-02')), + ] + + const results = await findAllWebhooksForPath({ requestId: 'req-1', path: 'shared-path' }) + + expect(results).toHaveLength(2) + expect(results.map((r) => r.webhook.id)).toEqual(['wh-a', 'wh-b']) + }) + + it('drops foreign rows when a path collides across workflows, keeping the earliest owner', async () => { + const victim = makeRow('victim-workflow', 'victim-wh', new Date('2026-01-01')) + const attacker = makeRow('attacker-workflow', 'attacker-wh', new Date('2026-05-01')) + mockWebhookLookupResult.rows = [attacker, victim] + + const results = await findAllWebhooksForPath({ requestId: 'req-2', path: 'shared-path' }) + + expect(results).toHaveLength(1) + expect(results[0].webhook.id).toBe('victim-wh') + expect(results[0].webhook.workflowId).toBe('victim-workflow') + }) + + it("preserves the owner's full credential-set fan-out while dropping a foreign row", async () => { + const victimA = makeRow('victim-workflow', 'victim-wh-a', new Date('2026-01-01')) + const victimB = makeRow('victim-workflow', 'victim-wh-b', new Date('2026-01-03')) + const attacker = makeRow('attacker-workflow', 'attacker-wh', new Date('2026-05-01')) + mockWebhookLookupResult.rows = [victimB, attacker, victimA] + + const results = await findAllWebhooksForPath({ requestId: 'req-5', path: 'shared-path' }) + + expect(results).toHaveLength(2) + expect(results.every((r) => r.webhook.workflowId === 'victim-workflow')).toBe(true) + expect(results.map((r) => r.webhook.id).sort()).toEqual(['victim-wh-a', 'victim-wh-b']) + }) + + it('returns an empty array when no webhooks match', async () => { + mockWebhookLookupResult.rows = [] + + const results = await findAllWebhooksForPath({ requestId: 'req-3', path: 'missing' }) + + expect(results).toEqual([]) + }) + + it('returns an empty array when path is not provided', async () => { + const results = await findAllWebhooksForPath({ requestId: 'req-4' }) + + expect(results).toEqual([]) + }) +}) describe('webhook processor execution identity', () => { beforeEach(() => { diff --git a/apps/sim/lib/webhooks/processor.ts b/apps/sim/lib/webhooks/processor.ts index 15f5fcacf6b..aa9ad644d81 100644 --- a/apps/sim/lib/webhooks/processor.ts +++ b/apps/sim/lib/webhooks/processor.ts @@ -306,7 +306,16 @@ async function findWebhookAndWorkflow( /** * Find ALL webhooks matching a path. - * Used for credential sets where multiple webhooks share the same path. + * + * Used for credential sets where multiple webhooks legitimately share the same + * path. Legitimate fan-out is always scoped to a single workflow (credential + * set sync only ever creates rows for one workflow), but webhook paths are + * user-controlled and only unique per deployment version, so two different + * workflows (tenants) can register the same public path. Dispatching a delivery + * to webhooks across multiple workflows would let one tenant receive and process + * another tenant's signed webhook payloads. To prevent that cross-tenant + * collision we constrain the returned rows to the workflow that registered the + * path first and drop any foreign rows. */ export async function findAllWebhooksForPath( options: WebhookProcessorOptions @@ -344,7 +353,31 @@ export async function findAllWebhooksForPath( if (results.length === 0) { logger.warn(`[${options.requestId}] No active webhooks found for path: ${options.path}`) - } else if (results.length > 1) { + return results + } + + const distinctWorkflowIds = new Set(results.map((result) => result.webhook.workflowId)) + + if (distinctWorkflowIds.size > 1) { + const owner = results.reduce((earliest, candidate) => { + const candidateTime = new Date(candidate.webhook.createdAt).getTime() + const earliestTime = new Date(earliest.webhook.createdAt).getTime() + if (candidateTime !== earliestTime) { + return candidateTime < earliestTime ? candidate : earliest + } + return candidate.webhook.id < earliest.webhook.id ? candidate : earliest + }) + const ownerWorkflowId = owner.webhook.workflowId + const ownerResults = results.filter((result) => result.webhook.workflowId === ownerWorkflowId) + + logger.error( + `[${options.requestId}] Cross-tenant webhook path collision for path: ${options.path}. Found ${results.length} active webhooks across ${distinctWorkflowIds.size} workflows. Dispatching only to owner workflow ${ownerWorkflowId} and dropping ${results.length - ownerResults.length} foreign webhook(s).` + ) + + return ownerResults + } + + if (results.length > 1) { logger.info( `[${options.requestId}] Found ${results.length} webhooks for path: ${options.path} (credential set fan-out)` ) From 18e88aaf79de33b5f47b849d338a7bfc3d49d40a Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 30 May 2026 12:04:41 -0700 Subject: [PATCH 02/17] fix(sso): scope domain conflict query with indexed lower(domain) filter Address PR review: avoid a full-table scan on every SSO provider registration by filtering candidate rows in SQL with lower(domain) = , keeping the in-memory ownership check. Also tighten the normalizeSSODomain TSDoc. Co-Authored-By: Claude Opus 4.8 --- apps/sim/app/api/auth/sso/register/route.ts | 3 ++- apps/sim/lib/auth/sso/domain.ts | 14 ++++---------- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/apps/sim/app/api/auth/sso/register/route.ts b/apps/sim/app/api/auth/sso/register/route.ts index b262038bf73..b92cac7c8cf 100644 --- a/apps/sim/app/api/auth/sso/register/route.ts +++ b/apps/sim/app/api/auth/sso/register/route.ts @@ -1,7 +1,7 @@ import { db, member, ssoProvider } from '@sim/db' import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' -import { and, eq } from 'drizzle-orm' +import { and, eq, sql } from 'drizzle-orm' import { type NextRequest, NextResponse } from 'next/server' import { ssoRegistrationContract } from '@/lib/api/contracts/auth' import { getValidationErrorMessage, parseRequest } from '@/lib/api/server' @@ -88,6 +88,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => { organizationId: ssoProvider.organizationId, }) .from(ssoProvider) + .where(eq(sql`lower(${ssoProvider.domain})`, domain)) const conflictingProvider = existingProviders.find( (provider) => normalizeSSODomain(provider.domain) === domain && !isOwnedByCaller(provider) ) diff --git a/apps/sim/lib/auth/sso/domain.ts b/apps/sim/lib/auth/sso/domain.ts index f1d77b6ba30..bdd0cc1714f 100644 --- a/apps/sim/lib/auth/sso/domain.ts +++ b/apps/sim/lib/auth/sso/domain.ts @@ -1,14 +1,8 @@ /** - * Normalizes a user-supplied SSO email domain to a canonical, comparable form. - * - * Strips protocol, path, query, port, leading wildcard/`@`, an email local - * part, surrounding whitespace, and a trailing dot, then lowercases the result. - * Returns `null` when the input does not look like a registrable domain - * (`example.com`), which callers should treat as a validation error. - * - * Used to compare a requested SSO domain against already-registered providers - * so a tenant cannot claim a domain another tenant already owns via casing or - * formatting variants. + * Normalizes a user-supplied SSO email domain to a canonical, comparable form: + * strips protocol, path, query, port, a leading wildcard/`@`, an email local + * part, and a trailing dot, then lowercases. Returns `null` for inputs that are + * not a registrable domain (e.g. `example.com`), which callers treat as invalid. */ export function normalizeSSODomain(input: string): string | null { if (typeof input !== 'string') return null From a8ec4a7cd14cc78ed082189f41b98fcac3313c1f Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 30 May 2026 12:04:59 -0700 Subject: [PATCH 03/17] chore: condense env route security comments Co-Authored-By: Claude Opus 4.8 --- apps/sim/app/api/workspaces/[id]/environment/route.test.ts | 2 -- apps/sim/app/api/workspaces/[id]/environment/route.ts | 4 +--- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/apps/sim/app/api/workspaces/[id]/environment/route.test.ts b/apps/sim/app/api/workspaces/[id]/environment/route.test.ts index c1192f38408..d90893f3858 100644 --- a/apps/sim/app/api/workspaces/[id]/environment/route.test.ts +++ b/apps/sim/app/api/workspaces/[id]/environment/route.test.ts @@ -71,10 +71,8 @@ describe('GET /api/workspaces/[id]/environment', () => { const body = await res.json() expect(body.data.workspace).toEqual({ OPENAI_API_KEY: '', DATABASE_URL: '' }) - // Plaintext workspace secrets must never reach non-admins. expect(JSON.stringify(body.data.workspace)).not.toContain('sk-live-secret-value') expect(JSON.stringify(body.data.workspace)).not.toContain('postgres://') - // The caller's own personal values are still returned. expect(body.data.personal).toEqual(ENV_RESULT.personalDecrypted) } ) diff --git a/apps/sim/app/api/workspaces/[id]/environment/route.ts b/apps/sim/app/api/workspaces/[id]/environment/route.ts index c4707e6de46..9698216cf39 100644 --- a/apps/sim/app/api/workspaces/[id]/environment/route.ts +++ b/apps/sim/app/api/workspaces/[id]/environment/route.ts @@ -56,9 +56,7 @@ export const GET = withRouteHandler( const { workspaceEncrypted, workspaceDecrypted, personalDecrypted, conflicts } = await getPersonalAndWorkspaceEnv(userId, workspaceId) - // Plaintext workspace secrets are restricted to workspace admins. Non-admins receive only - // the variable names (with empty values) so env references can still be validated and - // highlighted in the editor without exposing the decrypted secret values. + // Only workspace admins may read plaintext secrets; others get variable names with empty values. const workspace = permission === 'admin' ? workspaceDecrypted From 8b7dcabbdff62e1b12cc69fbd8fd19decfd50c33 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 30 May 2026 12:06:57 -0700 Subject: [PATCH 04/17] icons update --- apps/docs/components/icons.tsx | 245 +++++++++++++++++++++++++++------ apps/sim/components/icons.tsx | 245 +++++++++++++++++++++++++++------ 2 files changed, 400 insertions(+), 90 deletions(-) diff --git a/apps/docs/components/icons.tsx b/apps/docs/components/icons.tsx index f21df070063..34a8139b2be 100644 --- a/apps/docs/components/icons.tsx +++ b/apps/docs/components/icons.tsx @@ -780,7 +780,7 @@ export function GitLabIcon(props: SVGProps) { export function SerperIcon(props: SVGProps) { return ( - + ) { export const S3Icon = (props: SVGProps) => ( ) { export function BrainIcon(props: SVGProps) { return ( - - - - - - - - - - + + ) } @@ -2342,8 +2323,8 @@ export function ExtendIcon(props: SVGProps) { export function EvernoteIcon(props: SVGProps) { return ( - - + + ) } @@ -2538,7 +2519,7 @@ export function TelegramIcon(props: SVGProps) { ) { } export function MicrosoftOneDriveIcon(props: SVGProps) { + const id = useId() return ( - - - - - - - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + ) } diff --git a/apps/sim/components/icons.tsx b/apps/sim/components/icons.tsx index f21df070063..34a8139b2be 100644 --- a/apps/sim/components/icons.tsx +++ b/apps/sim/components/icons.tsx @@ -780,7 +780,7 @@ export function GitLabIcon(props: SVGProps) { export function SerperIcon(props: SVGProps) { return ( - + ) { export const S3Icon = (props: SVGProps) => ( ) { export function BrainIcon(props: SVGProps) { return ( - - - - - - - - - - + + ) } @@ -2342,8 +2323,8 @@ export function ExtendIcon(props: SVGProps) { export function EvernoteIcon(props: SVGProps) { return ( - - + + ) } @@ -2538,7 +2519,7 @@ export function TelegramIcon(props: SVGProps) { ) { } export function MicrosoftOneDriveIcon(props: SVGProps) { + const id = useId() return ( - - - - - - - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + ) } From a5968ffaa66263e861e0add5e21af884b76757e9 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 30 May 2026 12:07:22 -0700 Subject: [PATCH 05/17] chore(security): tighten inline comments in CSV export and KB file authorization Condense verbose comment blocks to concise TSDoc/single-line form; no behavior change. Co-Authored-By: Claude Opus 4.8 --- apps/sim/app/api/files/authorization.test.ts | 9 +++------ apps/sim/app/api/files/authorization.ts | 16 ++++------------ apps/sim/app/api/table/[tableId]/export/route.ts | 11 ++++------- 3 files changed, 11 insertions(+), 25 deletions(-) diff --git a/apps/sim/app/api/files/authorization.test.ts b/apps/sim/app/api/files/authorization.test.ts index b8b512ce598..6183c685950 100644 --- a/apps/sim/app/api/files/authorization.test.ts +++ b/apps/sim/app/api/files/authorization.test.ts @@ -67,8 +67,7 @@ describe('verifyKBFileAccess', () => { }) it('denies access via an external URL that merely contains the key as a substring', async () => { - // The demonstrated PoC: a planted document whose fileUrl is an unrelated - // external URL containing the victim key. It must never authorize storage. + // PoC: a planted external URL containing the victim key must never authorize storage. dbChainMockFns.limit.mockResolvedValueOnce([ { workspaceId: 'ws-attacker', @@ -83,8 +82,7 @@ describe('verifyKBFileAccess', () => { }) it('denies a later document planted in another workspace (ownership pins to earliest doc)', async () => { - // db query orders by uploadedAt asc, so the owning (victim) document comes - // first; the attacker's later internal-URL document must not grant access. + // Ordered by uploadedAt asc: the victim owns the key, so the attacker's later doc is ignored. dbChainMockFns.limit.mockResolvedValueOnce([ { workspaceId: 'ws-victim', fileUrl: internalUrlFor(VICTIM_KEY) }, { workspaceId: 'ws-attacker', fileUrl: internalUrlFor(VICTIM_KEY) }, @@ -142,8 +140,7 @@ describe('verifyKBFileAccess', () => { }) it('does not let a later workspace document override a null-workspace owner', async () => { - // The earliest (owning) document belongs to a workspace-less KB; a later - // document planted in the attacker's workspace must not become the owner. + // Earliest owner has no workspace; a later attacker-workspace doc must not become owner. dbChainMockFns.limit.mockResolvedValueOnce([ { workspaceId: null, fileUrl: internalUrlFor(VICTIM_KEY) }, { workspaceId: 'ws-attacker', fileUrl: internalUrlFor(VICTIM_KEY) }, diff --git a/apps/sim/app/api/files/authorization.ts b/apps/sim/app/api/files/authorization.ts index f004db07b0b..42120738c91 100644 --- a/apps/sim/app/api/files/authorization.ts +++ b/apps/sim/app/api/files/authorization.ts @@ -412,9 +412,7 @@ async function verifyKBFileAccess( customConfig?: StorageConfig ): Promise { try { - // The `LIKE` predicate only narrows candidates cheaply; it never decides - // authorization. Ordering by `uploadedAt` lets us pin ownership to the - // earliest document, which is the one the file was originally uploaded for. + // LIKE only narrows candidates; ownership is decided below, pinned to the earliest upload. const candidateDocuments = await db .select({ workspaceId: knowledgeBase.workspaceId, @@ -437,12 +435,8 @@ async function verifyKBFileAccess( .orderBy(asc(document.uploadedAt)) .limit(50) - // The owner is the earliest document whose fileUrl resolves to EXACTLY this - // storage key. Substring-only matches (e.g. an external URL that happens to - // contain the key) and documents in other workspaces that merely reference - // the key do not establish ownership. Pinning to the earliest match means a - // later document planted in the caller's own workspace can never authorize - // access to a file another workspace originally uploaded. + // Owner is the earliest document whose fileUrl resolves to EXACTLY this key; substring + // matches and cross-workspace references never establish ownership. const owningDocument = candidateDocuments.find( (doc) => resolveInternalKbKey(doc.fileUrl) === cloudKey ) @@ -477,9 +471,7 @@ async function verifyKBFileAccess( return false } - // KB file access must resolve through an active KB document that canonically - // owns the key. Metadata alone is not enough because parent archives - // intentionally keep the underlying file bytes around for history. + // No owning document: metadata only lets us flag the deleted-file case; it never grants access. const fileRecord = await getFileMetadataByKey(cloudKey, 'knowledge-base', { includeDeleted: true, }) diff --git a/apps/sim/app/api/table/[tableId]/export/route.ts b/apps/sim/app/api/table/[tableId]/export/route.ts index f5702c0b136..a9f7b002070 100644 --- a/apps/sim/app/api/table/[tableId]/export/route.ts +++ b/apps/sim/app/api/table/[tableId]/export/route.ts @@ -114,19 +114,16 @@ function sanitizeFilename(name: string): string { } /** - * Prefixes a single quote to values that spreadsheet applications would otherwise - * interpret as formulas, preventing CSV formula injection when an exported file is - * opened in Excel, LibreOffice, or Google Sheets. JSON exports preserve raw values. + * Prefixes a single quote to values starting with a spreadsheet formula trigger + * (`=`, `+`, `-`, `@`, tab, CR), neutralizing CSV injection in Excel/Sheets. */ function neutralizeCsvFormula(value: string): string { return /^[=+\-@\t\r]/.test(value) ? `'${value}` : value } /** - * Serializes a cell for CSV output. Only string cells are run through - * {@link neutralizeCsvFormula}: numbers, booleans, dates, and JSON-serialized - * objects can never stringify into a spreadsheet formula trigger, so they are - * emitted verbatim to preserve fidelity (e.g. negative numbers stay numeric). + * Serializes a cell for CSV. Only string cells are formula-neutralized; numbers, + * booleans, dates, and JSON objects can never form a trigger and pass through verbatim. */ function formatCsvValue(value: unknown): string { if (value === null || value === undefined) return '' From 4e2559085f849b2cf7b4f0eff3edeb2fd033d6ed Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 30 May 2026 12:10:21 -0700 Subject: [PATCH 06/17] fix(security): validate internal serve origin in KB file authorization Replace the bypassable isInternalFileUrl substring check in resolveInternalKbKey with an origin allow-list (base URL, internal API base URL, TRUSTED_ORIGINS). A crafted external host whose path is /api/files/serve/ no longer resolves to the victim key. Relative same-origin URLs are unaffected. --- apps/sim/app/api/files/authorization.test.ts | 43 ++++++++++++- apps/sim/app/api/files/authorization.ts | 67 +++++++++++++++----- 2 files changed, 92 insertions(+), 18 deletions(-) diff --git a/apps/sim/app/api/files/authorization.test.ts b/apps/sim/app/api/files/authorization.test.ts index 6183c685950..652fed4f02e 100644 --- a/apps/sim/app/api/files/authorization.test.ts +++ b/apps/sim/app/api/files/authorization.test.ts @@ -38,15 +38,27 @@ vi.mock('@/lib/uploads', () => ({ getFileMetadata: vi.fn().mockResolvedValue({}), })) +const { APP_ORIGIN } = vi.hoisted(() => ({ APP_ORIGIN: 'https://app.test' })) + +vi.mock('@/lib/core/utils/urls', () => ({ + getBaseUrl: () => APP_ORIGIN, + getInternalApiBaseUrl: () => APP_ORIGIN, + parseOriginList: () => [], +})) + import { verifyFileAccess } from '@/app/api/files/authorization' const VICTIM_KEY = 'kb/1780162789495-victim-secret.txt' const ATTACKER_USER = 'attacker-user' -/** Internal serve URL that legitimately resolves to VICTIM_KEY. */ +/** Relative internal serve URL that resolves to a storage key (same-origin). */ const internalUrlFor = (key: string) => `/api/files/serve/s3/${encodeURIComponent(key)}?context=knowledge-base` +/** Absolute serve URL on an arbitrary origin. */ +const absoluteServeUrl = (origin: string, key: string) => + `${origin}/api/files/serve/s3/${encodeURIComponent(key)}?context=knowledge-base` + describe('verifyKBFileAccess', () => { beforeEach(() => { vi.clearAllMocks() @@ -66,6 +78,35 @@ describe('verifyKBFileAccess', () => { expect(mockGetUserEntityPermissions).toHaveBeenCalledWith('owner-user', 'workspace', 'ws-owner') }) + it('grants access via an absolute serve URL on the application origin', async () => { + dbChainMockFns.limit.mockResolvedValueOnce([ + { workspaceId: 'ws-owner', fileUrl: absoluteServeUrl(APP_ORIGIN, VICTIM_KEY) }, + ]) + mockGetUserEntityPermissions.mockResolvedValue('read') + + const granted = await verifyFileAccess(VICTIM_KEY, 'owner-user', undefined, 'knowledge-base') + + expect(granted).toBe(true) + expect(mockGetUserEntityPermissions).toHaveBeenCalledWith('owner-user', 'workspace', 'ws-owner') + }) + + it('denies a crafted external host whose path is /api/files/serve/', async () => { + // isInternalFileUrl is a substring check; an attacker-controlled host with the + // serve path resolves to the victim key. The origin allow-list must reject it. + dbChainMockFns.limit.mockResolvedValueOnce([ + { + workspaceId: 'ws-attacker', + fileUrl: absoluteServeUrl('https://attacker.example', VICTIM_KEY), + }, + ]) + mockGetUserEntityPermissions.mockResolvedValue('admin') + + const granted = await verifyFileAccess(VICTIM_KEY, ATTACKER_USER, undefined, 'knowledge-base') + + expect(granted).toBe(false) + expect(mockGetUserEntityPermissions).not.toHaveBeenCalled() + }) + it('denies access via an external URL that merely contains the key as a substring', async () => { // PoC: a planted external URL containing the victim key must never authorize storage. dbChainMockFns.limit.mockResolvedValueOnce([ diff --git a/apps/sim/app/api/files/authorization.ts b/apps/sim/app/api/files/authorization.ts index 42120738c91..06cb4da5e65 100644 --- a/apps/sim/app/api/files/authorization.ts +++ b/apps/sim/app/api/files/authorization.ts @@ -3,16 +3,14 @@ import { document, knowledgeBase, workspaceFile } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { and, asc, eq, isNull, like, or } from 'drizzle-orm' import { NextResponse } from 'next/server' +import { getEnv } from '@/lib/core/config/env' +import { getBaseUrl, getInternalApiBaseUrl, parseOriginList } from '@/lib/core/utils/urls' import { getFileMetadata } from '@/lib/uploads' import type { StorageContext } from '@/lib/uploads/config' import { BLOB_CHAT_CONFIG, S3_CHAT_CONFIG } from '@/lib/uploads/config' import type { StorageConfig } from '@/lib/uploads/core/storage-client' import { getFileMetadataByKey } from '@/lib/uploads/server/metadata' -import { - extractStorageKey, - inferContextFromKey, - isInternalFileUrl, -} from '@/lib/uploads/utils/file-utils' +import { extractStorageKey, inferContextFromKey } from '@/lib/uploads/utils/file-utils' import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils' import { isUuid } from '@/executor/constants' @@ -374,25 +372,59 @@ async function verifyCopilotFileAccess( } } +/** + * Origins this app legitimately serves files from: the public base URL, the + * internal API base URL, and any configured `TRUSTED_ORIGINS`. A document + * `fileUrl` only authorizes storage access when it is relative (same-origin) or + * absolute with one of these origins. + */ +function getInternalServeOrigins(): Set { + const origins = new Set() + for (const resolve of [getBaseUrl, getInternalApiBaseUrl]) { + try { + origins.add(new URL(resolve()).origin) + } catch { + // NEXT_PUBLIC_APP_URL unset/invalid — skip; relative fileUrls still resolve. + } + } + for (const origin of parseOriginList(getEnv('TRUSTED_ORIGINS'))) { + origins.add(origin) + } + return origins +} + /** * Resolve a knowledge-base document's stored `fileUrl` to the canonical storage - * key it points at, but only when the URL is an internal Sim file-serve URL. + * key it points at, but only for internal Sim file-serve URLs. * - * External `http(s)://` URLs and `data:` URIs are accepted for ingestion but - * intentionally return `null` here so they can never authorize access to an - * internal storage object. Returns the storage key (e.g. `kb/-name.txt`) - * only for internal knowledge-base URLs, `null` otherwise. + * Relative paths are same-origin by definition; absolute URLs must match an + * internal serve origin. A foreign host that embeds `/api/files/serve/` in its + * path, and `data:` URIs, return `null` so an attacker-planted document can + * never authorize access to another tenant's storage object. */ -function resolveInternalKbKey(fileUrl: string | null): string | null { - if (!fileUrl || !isInternalFileUrl(fileUrl)) { +function resolveInternalKbKey(fileUrl: string | null, allowedOrigins: Set): string | null { + if (!fileUrl) { return null } - try { - const key = extractStorageKey(fileUrl) - return key.startsWith('kb/') ? key : null - } catch { + let pathname: string + if (fileUrl.startsWith('/')) { + pathname = fileUrl + } else { + try { + const parsed = new URL(fileUrl) + if (!allowedOrigins.has(parsed.origin)) { + return null + } + pathname = parsed.pathname + } catch { + return null + } + } + if (!pathname.startsWith('/api/files/serve/')) { return null } + const key = extractStorageKey(pathname) + return key.startsWith('kb/') ? key : null } /** @@ -437,8 +469,9 @@ async function verifyKBFileAccess( // Owner is the earliest document whose fileUrl resolves to EXACTLY this key; substring // matches and cross-workspace references never establish ownership. + const allowedOrigins = getInternalServeOrigins() const owningDocument = candidateDocuments.find( - (doc) => resolveInternalKbKey(doc.fileUrl) === cloudKey + (doc) => resolveInternalKbKey(doc.fileUrl, allowedOrigins) === cloudKey ) if (owningDocument) { From 4fa9b4e3cda3cfb8303b41205fb9686c3fa92c3a Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 30 May 2026 12:15:51 -0700 Subject: [PATCH 07/17] style(sso): use idiomatic sql lower() comparison for domain conflict query Match the repo's prevailing `sql`lower(col) = value`` idiom for the case-insensitive SSO domain conflict lookup. Co-Authored-By: Claude Opus 4.8 --- apps/sim/app/api/auth/sso/register/route.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/sim/app/api/auth/sso/register/route.ts b/apps/sim/app/api/auth/sso/register/route.ts index b92cac7c8cf..d08fa8375fd 100644 --- a/apps/sim/app/api/auth/sso/register/route.ts +++ b/apps/sim/app/api/auth/sso/register/route.ts @@ -88,7 +88,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => { organizationId: ssoProvider.organizationId, }) .from(ssoProvider) - .where(eq(sql`lower(${ssoProvider.domain})`, domain)) + .where(sql`lower(${ssoProvider.domain}) = ${domain}`) const conflictingProvider = existingProviders.find( (provider) => normalizeSSODomain(provider.domain) === domain && !isOwnedByCaller(provider) ) From 6b860ec8be90be8c8418b2f21ddd55babc4bbd88 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 30 May 2026 12:17:52 -0700 Subject: [PATCH 08/17] fix(security): align workspace env admin gate with hasWorkspaceAdminAccess Use the same admin check the secrets UI uses (owner, admin permission, or org-admin) so owners and org-admins are not wrongly denied their own decrypted workspace secrets, while read-only members remain restricted to names only. Co-Authored-By: Claude Opus 4.8 --- .../workspaces/[id]/environment/route.test.ts | 16 ++++++++++++++- .../api/workspaces/[id]/environment/route.ts | 20 ++++++++++++------- 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/apps/sim/app/api/workspaces/[id]/environment/route.test.ts b/apps/sim/app/api/workspaces/[id]/environment/route.test.ts index d90893f3858..eef1d2f9cd2 100644 --- a/apps/sim/app/api/workspaces/[id]/environment/route.test.ts +++ b/apps/sim/app/api/workspaces/[id]/environment/route.test.ts @@ -47,11 +47,13 @@ describe('GET /api/workspaces/[id]/environment', () => { vi.clearAllMocks() authMockFns.mockGetSession.mockResolvedValue({ user: { id: 'user-1' } }) permissionsMockFns.mockGetWorkspaceById.mockResolvedValue({ id: WORKSPACE_ID }) + permissionsMockFns.mockHasWorkspaceAdminAccess.mockResolvedValue(false) mockGetPersonalAndWorkspaceEnv.mockResolvedValue(ENV_RESULT) }) it('returns decrypted workspace values to workspace admins', async () => { permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue('admin') + permissionsMockFns.mockHasWorkspaceAdminAccess.mockResolvedValue(true) const res = await GET(createRequest(), createContext()) expect(res.status).toBe(200) @@ -62,9 +64,10 @@ describe('GET /api/workspaces/[id]/environment', () => { }) it.each(['write', 'read'] as const)( - 'returns only variable names (empty values) to %s members', + 'returns only variable names (empty values) to non-admin %s members', async (permission) => { permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue(permission) + permissionsMockFns.mockHasWorkspaceAdminAccess.mockResolvedValue(false) const res = await GET(createRequest(), createContext()) expect(res.status).toBe(200) @@ -77,6 +80,17 @@ describe('GET /api/workspaces/[id]/environment', () => { } ) + it('returns decrypted values to an admin-access user whose permission row is not "admin" (owner/org-admin)', async () => { + permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue('write') + permissionsMockFns.mockHasWorkspaceAdminAccess.mockResolvedValue(true) + + const res = await GET(createRequest(), createContext()) + expect(res.status).toBe(200) + + const body = await res.json() + expect(body.data.workspace).toEqual(ENV_RESULT.workspaceDecrypted) + }) + it('rejects users without any workspace permission', async () => { permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue(null) diff --git a/apps/sim/app/api/workspaces/[id]/environment/route.ts b/apps/sim/app/api/workspaces/[id]/environment/route.ts index 9698216cf39..29643543617 100644 --- a/apps/sim/app/api/workspaces/[id]/environment/route.ts +++ b/apps/sim/app/api/workspaces/[id]/environment/route.ts @@ -23,7 +23,11 @@ import { getPersonalAndWorkspaceEnv, invalidateEffectiveDecryptedEnvCache, } from '@/lib/environment/utils' -import { getUserEntityPermissions, getWorkspaceById } from '@/lib/workspaces/permissions/utils' +import { + getUserEntityPermissions, + getWorkspaceById, + hasWorkspaceAdminAccess, +} from '@/lib/workspaces/permissions/utils' const logger = createLogger('WorkspaceEnvironmentAPI') @@ -53,14 +57,16 @@ export const GET = withRouteHandler( return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }) } - const { workspaceEncrypted, workspaceDecrypted, personalDecrypted, conflicts } = - await getPersonalAndWorkspaceEnv(userId, workspaceId) + const [isAdmin, { workspaceEncrypted, workspaceDecrypted, personalDecrypted, conflicts }] = + await Promise.all([ + hasWorkspaceAdminAccess(userId, workspaceId), + getPersonalAndWorkspaceEnv(userId, workspaceId), + ]) // Only workspace admins may read plaintext secrets; others get variable names with empty values. - const workspace = - permission === 'admin' - ? workspaceDecrypted - : Object.fromEntries(Object.keys(workspaceEncrypted).map((key) => [key, ''])) + const workspace = isAdmin + ? workspaceDecrypted + : Object.fromEntries(Object.keys(workspaceEncrypted).map((key) => [key, ''])) return NextResponse.json( { From 474d73f4052922eb0d605d3fbc560af5bb7dbfcf Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 30 May 2026 14:27:29 -0700 Subject: [PATCH 09/17] fix(sso): rely on lower(domain) match for conflict detection, drop dead in-memory recheck Address PR review: the SQL `lower(domain) = ` predicate already excludes rows that the in-memory `normalizeSSODomain(...) === domain` recheck claimed to catch, making that recheck dead/misleading code. Match on the canonical lower-cased domain and filter purely by ownership. Malformed legacy values (wildcards, schemes, ports) never match an email domain at sign-in, so excluding them is not a gap. Test DB mock now applies the lower() predicate so the casing-variant case is genuinely exercised. Co-Authored-By: Claude Opus 4.8 --- apps/sim/app/api/auth/sso/register/route.test.ts | 9 ++++++++- apps/sim/app/api/auth/sso/register/route.ts | 5 +---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/apps/sim/app/api/auth/sso/register/route.test.ts b/apps/sim/app/api/auth/sso/register/route.test.ts index a31852e6805..c2337bc719a 100644 --- a/apps/sim/app/api/auth/sso/register/route.test.ts +++ b/apps/sim/app/api/auth/sso/register/route.test.ts @@ -37,7 +37,14 @@ const { function makeBuilder(rows: any[]): any { const thenable: any = Promise.resolve(rows) - thenable.where = () => makeBuilder(rows) + thenable.where = (condition: any) => { + const values = condition?.values + if (Array.isArray(values) && values.length > 0) { + const target = String(values[values.length - 1]).toLowerCase() + return makeBuilder(rows.filter((r) => String(r.domain ?? '').toLowerCase() === target)) + } + return makeBuilder(rows) + } thenable.limit = () => Promise.resolve(rows) thenable.orderBy = () => Promise.resolve(rows) return thenable diff --git a/apps/sim/app/api/auth/sso/register/route.ts b/apps/sim/app/api/auth/sso/register/route.ts index d08fa8375fd..1636837bf22 100644 --- a/apps/sim/app/api/auth/sso/register/route.ts +++ b/apps/sim/app/api/auth/sso/register/route.ts @@ -83,15 +83,12 @@ export const POST = withRouteHandler(async (request: NextRequest) => { const existingProviders = await db .select({ - domain: ssoProvider.domain, userId: ssoProvider.userId, organizationId: ssoProvider.organizationId, }) .from(ssoProvider) .where(sql`lower(${ssoProvider.domain}) = ${domain}`) - const conflictingProvider = existingProviders.find( - (provider) => normalizeSSODomain(provider.domain) === domain && !isOwnedByCaller(provider) - ) + const conflictingProvider = existingProviders.find((provider) => !isOwnedByCaller(provider)) if (conflictingProvider) { logger.warn('Rejected SSO registration for domain owned by another tenant', { From 592e0484c8ee6e1af47b4dd63d111d9e8360084f Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 30 May 2026 14:48:12 -0700 Subject: [PATCH 10/17] fix(security): scope webhook deploy path conflict to active webhooks findConflictingWebhookPathOwner omitted the isActive filter that the runtime dispatcher (findAllWebhooksForPath) applies, so an inactive but non-archived webhook from another workflow (e.g. after undeploy or failure auto-disable) would permanently block any new deployment on that path even though it never receives deliveries. Align the guard with the runtime isActive + archivedAt filter; the earliest-owner runtime check remains the authoritative cross-tenant protection. Also trims verbose TSDoc on the webhook path-isolation helpers. Co-Authored-By: Claude Opus 4.8 --- apps/sim/lib/webhooks/deploy.ts | 14 +++++++------- apps/sim/lib/webhooks/processor.ts | 15 +++++---------- 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/apps/sim/lib/webhooks/deploy.ts b/apps/sim/lib/webhooks/deploy.ts index 61ca1325e8f..0d3e985ea75 100644 --- a/apps/sim/lib/webhooks/deploy.ts +++ b/apps/sim/lib/webhooks/deploy.ts @@ -24,12 +24,12 @@ const logger = createLogger('DeployWebhookSync') const CREDENTIAL_SET_PREFIX = 'credentialSet:' /** - * Returns the id of a workflow that already owns an active (non-archived) - * webhook on the given path, when that workflow is different from the one being - * deployed. Webhook paths are user-controlled and the database only enforces - * uniqueness per deployment version, so without this guard two tenants could - * register the same public path and a delivery to one would fan out to the - * other. Returns `null` when the path is free or only used by this workflow. + * Returns the id of a different workflow that already owns an active webhook on + * the given path, or `null` if the path is free or owned by this workflow. + * Guards against cross-tenant path collisions, since paths are user-controlled + * and only unique per deployment version. Mirrors the runtime dispatcher's + * `isActive`/`archivedAt` filter so inactive (e.g. undeployed) webhooks — which + * never receive deliveries — don't permanently reserve a path. */ async function findConflictingWebhookPathOwner(params: { path: string @@ -40,7 +40,7 @@ async function findConflictingWebhookPathOwner(params: { const existing = await db .select({ workflowId: webhook.workflowId }) .from(webhook) - .where(and(eq(webhook.path, path), isNull(webhook.archivedAt))) + .where(and(eq(webhook.path, path), eq(webhook.isActive, true), isNull(webhook.archivedAt))) const conflict = existing.find((row) => row.workflowId !== workflowId) return conflict ? conflict.workflowId : null diff --git a/apps/sim/lib/webhooks/processor.ts b/apps/sim/lib/webhooks/processor.ts index aa9ad644d81..96ce09813b4 100644 --- a/apps/sim/lib/webhooks/processor.ts +++ b/apps/sim/lib/webhooks/processor.ts @@ -305,17 +305,12 @@ async function findWebhookAndWorkflow( } /** - * Find ALL webhooks matching a path. + * Finds all webhooks matching a path, scoped to a single workflow. * - * Used for credential sets where multiple webhooks legitimately share the same - * path. Legitimate fan-out is always scoped to a single workflow (credential - * set sync only ever creates rows for one workflow), but webhook paths are - * user-controlled and only unique per deployment version, so two different - * workflows (tenants) can register the same public path. Dispatching a delivery - * to webhooks across multiple workflows would let one tenant receive and process - * another tenant's signed webhook payloads. To prevent that cross-tenant - * collision we constrain the returned rows to the workflow that registered the - * path first and drop any foreign rows. + * Legitimate fan-out (credential sets) is always within one workflow, but paths + * are user-controlled and only unique per deployment version, so two tenants can + * register the same path. On collision we keep only the workflow that registered + * the path first, so one tenant can never receive another's webhook deliveries. */ export async function findAllWebhooksForPath( options: WebhookProcessorOptions From 237be12ca2ecc3e7b3ee32e106da53fe99c6299e Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 30 May 2026 15:34:06 -0700 Subject: [PATCH 11/17] fix(security): exclude archived workflows from webhook deploy path conflict findConflictingWebhookPathOwner now joins workflow and filters isNull(workflow.archivedAt), matching the runtime dispatcher (findAllWebhooksForPath). A webhook on an archived workflow can never receive deliveries at runtime, so it must not block legitimate path reuse with a 409. Co-Authored-By: Claude Opus 4.8 --- apps/sim/lib/webhooks/deploy.ts | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/apps/sim/lib/webhooks/deploy.ts b/apps/sim/lib/webhooks/deploy.ts index 0d3e985ea75..373ab7f9885 100644 --- a/apps/sim/lib/webhooks/deploy.ts +++ b/apps/sim/lib/webhooks/deploy.ts @@ -1,5 +1,11 @@ import { db } from '@sim/db' -import { account, credentialSetMember, webhook, workflowDeploymentVersion } from '@sim/db/schema' +import { + account, + credentialSetMember, + webhook, + workflow, + workflowDeploymentVersion, +} from '@sim/db/schema' import { createLogger } from '@sim/logger' import { generateShortId } from '@sim/utils/id' import { and, eq, inArray, isNotNull, isNull, or } from 'drizzle-orm' @@ -28,8 +34,8 @@ const CREDENTIAL_SET_PREFIX = 'credentialSet:' * the given path, or `null` if the path is free or owned by this workflow. * Guards against cross-tenant path collisions, since paths are user-controlled * and only unique per deployment version. Mirrors the runtime dispatcher's - * `isActive`/`archivedAt` filter so inactive (e.g. undeployed) webhooks — which - * never receive deliveries — don't permanently reserve a path. + * filter (active, non-archived webhook on a non-archived workflow) so webhooks + * that can never receive deliveries don't permanently reserve a path. */ async function findConflictingWebhookPathOwner(params: { path: string @@ -40,7 +46,15 @@ async function findConflictingWebhookPathOwner(params: { const existing = await db .select({ workflowId: webhook.workflowId }) .from(webhook) - .where(and(eq(webhook.path, path), eq(webhook.isActive, true), isNull(webhook.archivedAt))) + .innerJoin(workflow, eq(webhook.workflowId, workflow.id)) + .where( + and( + eq(webhook.path, path), + eq(webhook.isActive, true), + isNull(webhook.archivedAt), + isNull(workflow.archivedAt) + ) + ) const conflict = existing.find((row) => row.workflowId !== workflowId) return conflict ? conflict.workflowId : null From 93121572c577b18895fdba3b03cbee28ec4b70e6 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 30 May 2026 16:00:48 -0700 Subject: [PATCH 12/17] fix(security): anchor KB file ownership to earliest document in any state A KB file's owner is now the earliest document referencing its key regardless of state (active/archived/deleted/excluded); access is granted only when that owning document is still active. Closes the residual where an attacker could plant an active document to claim a file whose original document was archived or deleted. --- apps/sim/app/api/files/authorization.test.ts | 27 +++++++++++ apps/sim/app/api/files/authorization.ts | 47 +++++++++++++------- 2 files changed, 57 insertions(+), 17 deletions(-) diff --git a/apps/sim/app/api/files/authorization.test.ts b/apps/sim/app/api/files/authorization.test.ts index 652fed4f02e..03eb86bc4ff 100644 --- a/apps/sim/app/api/files/authorization.test.ts +++ b/apps/sim/app/api/files/authorization.test.ts @@ -149,6 +149,33 @@ describe('verifyKBFileAccess', () => { ) }) + it('denies a planted active document when the original owner document is archived', async () => { + // The earliest (victim) document is archived, so the file is retired; the attacker's + // later active document must not become the owner and grant cross-tenant access. + dbChainMockFns.limit.mockResolvedValueOnce([ + { workspaceId: 'ws-victim', fileUrl: internalUrlFor(VICTIM_KEY), archivedAt: new Date() }, + { workspaceId: 'ws-attacker', fileUrl: internalUrlFor(VICTIM_KEY) }, + ]) + mockGetUserEntityPermissions.mockResolvedValue('admin') + + const granted = await verifyFileAccess(VICTIM_KEY, ATTACKER_USER, undefined, 'knowledge-base') + + expect(granted).toBe(false) + expect(mockGetUserEntityPermissions).not.toHaveBeenCalled() + }) + + it('denies access when the owning document is soft-deleted (retired file not served)', async () => { + dbChainMockFns.limit.mockResolvedValueOnce([ + { workspaceId: 'ws-owner', fileUrl: internalUrlFor(VICTIM_KEY), deletedAt: new Date() }, + ]) + mockGetUserEntityPermissions.mockResolvedValue('admin') + + const granted = await verifyFileAccess(VICTIM_KEY, 'owner-user', undefined, 'knowledge-base') + + expect(granted).toBe(false) + expect(mockGetUserEntityPermissions).not.toHaveBeenCalled() + }) + it('denies access when a substring document points at a different key', async () => { dbChainMockFns.limit.mockResolvedValueOnce([ { workspaceId: 'ws-attacker', fileUrl: internalUrlFor(`${VICTIM_KEY}-decoy`) }, diff --git a/apps/sim/app/api/files/authorization.ts b/apps/sim/app/api/files/authorization.ts index 06cb4da5e65..30c562697f1 100644 --- a/apps/sim/app/api/files/authorization.ts +++ b/apps/sim/app/api/files/authorization.ts @@ -432,11 +432,12 @@ function resolveInternalKbKey(fileUrl: string | null, allowedOrigins: Set { try { - // LIKE only narrows candidates; ownership is decided below, pinned to the earliest upload. + // Ownership spans every document state: the earliest document to reference the key owns + // it, so a document planted later in another workspace can never claim a file whose + // original document was archived or deleted. LIKE only narrows candidates; the exact + // owner is resolved below. const candidateDocuments = await db .select({ workspaceId: knowledgeBase.workspaceId, fileUrl: document.fileUrl, + archivedAt: document.archivedAt, + deletedAt: document.deletedAt, + userExcluded: document.userExcluded, + kbDeletedAt: knowledgeBase.deletedAt, }) .from(document) .innerJoin(knowledgeBase, eq(document.knowledgeBaseId, knowledgeBase.id)) .where( - and( - eq(document.userExcluded, false), - isNull(document.archivedAt), - isNull(document.deletedAt), - isNull(knowledgeBase.deletedAt), - or( - like(document.fileUrl, `%${cloudKey}%`), - like(document.fileUrl, `%${encodeURIComponent(cloudKey)}%`) - ) + or( + like(document.fileUrl, `%${cloudKey}%`), + like(document.fileUrl, `%${encodeURIComponent(cloudKey)}%`) ) ) .orderBy(asc(document.uploadedAt)) .limit(50) - // Owner is the earliest document whose fileUrl resolves to EXACTLY this key; substring - // matches and cross-workspace references never establish ownership. const allowedOrigins = getInternalServeOrigins() const owningDocument = candidateDocuments.find( (doc) => resolveInternalKbKey(doc.fileUrl, allowedOrigins) === cloudKey ) if (owningDocument) { + const isActive = + !owningDocument.archivedAt && + !owningDocument.deletedAt && + !owningDocument.userExcluded && + !owningDocument.kbDeletedAt + + // The owning document is archived/deleted/excluded: the file is retired and not served, + // and the attacker's later active document is not the owner, so it cannot claim it. + if (!isActive) { + logger.warn('KB file access denied: owning document is not active', { userId, cloudKey }) + return false + } + if (!owningDocument.workspaceId) { logger.warn('KB file access denied: owning document has no workspace', { userId, From b93b5bd4c256d727166130bc4649628be18a3b36 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 30 May 2026 16:10:05 -0700 Subject: [PATCH 13/17] updated greptile icon --- apps/docs/components/icons.tsx | 20 +++++++++++++++----- apps/sim/components/icons.tsx | 20 +++++++++++++++----- 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/apps/docs/components/icons.tsx b/apps/docs/components/icons.tsx index 34a8139b2be..2417e6acb58 100644 --- a/apps/docs/components/icons.tsx +++ b/apps/docs/components/icons.tsx @@ -6037,12 +6037,22 @@ export function GreenhouseIcon(props: SVGProps) { export function GreptileIcon(props: SVGProps) { return ( - + + + ) diff --git a/apps/sim/components/icons.tsx b/apps/sim/components/icons.tsx index 34a8139b2be..2417e6acb58 100644 --- a/apps/sim/components/icons.tsx +++ b/apps/sim/components/icons.tsx @@ -6037,12 +6037,22 @@ export function GreenhouseIcon(props: SVGProps) { export function GreptileIcon(props: SVGProps) { return ( - + + + ) From eb5f56af61475297027614a6f71d6da181dbe9fc Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 30 May 2026 16:28:45 -0700 Subject: [PATCH 14/17] revert(security): drop KB file authorization changes Reverts the knowledge-base file-access work (origin-pinning / owner-pinning / origin allow-list in verifyKBFileAccess) and its test. The other hardening fixes (SSO domain registration, webhook path isolation, workspace env secrets, CSV export) are unchanged. apps/sim/app/api/files/authorization.ts is restored to its origin/staging baseline. --- apps/sim/app/api/files/authorization.test.ts | 223 ------------------- apps/sim/app/api/files/authorization.ts | 144 ++---------- 2 files changed, 22 insertions(+), 345 deletions(-) delete mode 100644 apps/sim/app/api/files/authorization.test.ts diff --git a/apps/sim/app/api/files/authorization.test.ts b/apps/sim/app/api/files/authorization.test.ts deleted file mode 100644 index 03eb86bc4ff..00000000000 --- a/apps/sim/app/api/files/authorization.test.ts +++ /dev/null @@ -1,223 +0,0 @@ -/** - * @vitest-environment node - * - * Security regression tests for knowledge-base file authorization. - * - * The historical bug: `verifyKBFileAccess` authorized access to a storage object - * whenever ANY active document's `fileUrl` contained the requested key as a - * substring, in a workspace the caller could reach. That let a tenant plant a - * document referencing another tenant's storage key (even via an unrelated - * external URL) and read/delete the victim's file. - * - * These tests pin the fixed behavior: authorization requires a document whose - * fileUrl canonically resolves to EXACTLY the requested key, and access is - * decided against the OWNING (earliest) document's workspace only. - */ -import { dbChainMock, dbChainMockFns, resetDbChainMock } from '@sim/testing' -import { beforeEach, describe, expect, it, vi } from 'vitest' - -vi.mock('@sim/db', () => dbChainMock) - -const { mockGetUserEntityPermissions } = vi.hoisted(() => ({ - mockGetUserEntityPermissions: vi.fn(), -})) - -vi.mock('@/lib/workspaces/permissions/utils', () => ({ - getUserEntityPermissions: mockGetUserEntityPermissions, -})) - -const { mockGetFileMetadataByKey } = vi.hoisted(() => ({ - mockGetFileMetadataByKey: vi.fn(), -})) - -vi.mock('@/lib/uploads/server/metadata', () => ({ - getFileMetadataByKey: mockGetFileMetadataByKey, -})) - -vi.mock('@/lib/uploads', () => ({ - getFileMetadata: vi.fn().mockResolvedValue({}), -})) - -const { APP_ORIGIN } = vi.hoisted(() => ({ APP_ORIGIN: 'https://app.test' })) - -vi.mock('@/lib/core/utils/urls', () => ({ - getBaseUrl: () => APP_ORIGIN, - getInternalApiBaseUrl: () => APP_ORIGIN, - parseOriginList: () => [], -})) - -import { verifyFileAccess } from '@/app/api/files/authorization' - -const VICTIM_KEY = 'kb/1780162789495-victim-secret.txt' -const ATTACKER_USER = 'attacker-user' - -/** Relative internal serve URL that resolves to a storage key (same-origin). */ -const internalUrlFor = (key: string) => - `/api/files/serve/s3/${encodeURIComponent(key)}?context=knowledge-base` - -/** Absolute serve URL on an arbitrary origin. */ -const absoluteServeUrl = (origin: string, key: string) => - `${origin}/api/files/serve/s3/${encodeURIComponent(key)}?context=knowledge-base` - -describe('verifyKBFileAccess', () => { - beforeEach(() => { - vi.clearAllMocks() - resetDbChainMock() - mockGetFileMetadataByKey.mockResolvedValue(null) - }) - - it('grants access to the workspace that owns the storage key', async () => { - dbChainMockFns.limit.mockResolvedValueOnce([ - { workspaceId: 'ws-owner', fileUrl: internalUrlFor(VICTIM_KEY) }, - ]) - mockGetUserEntityPermissions.mockResolvedValue('read') - - const granted = await verifyFileAccess(VICTIM_KEY, 'owner-user', undefined, 'knowledge-base') - - expect(granted).toBe(true) - expect(mockGetUserEntityPermissions).toHaveBeenCalledWith('owner-user', 'workspace', 'ws-owner') - }) - - it('grants access via an absolute serve URL on the application origin', async () => { - dbChainMockFns.limit.mockResolvedValueOnce([ - { workspaceId: 'ws-owner', fileUrl: absoluteServeUrl(APP_ORIGIN, VICTIM_KEY) }, - ]) - mockGetUserEntityPermissions.mockResolvedValue('read') - - const granted = await verifyFileAccess(VICTIM_KEY, 'owner-user', undefined, 'knowledge-base') - - expect(granted).toBe(true) - expect(mockGetUserEntityPermissions).toHaveBeenCalledWith('owner-user', 'workspace', 'ws-owner') - }) - - it('denies a crafted external host whose path is /api/files/serve/', async () => { - // isInternalFileUrl is a substring check; an attacker-controlled host with the - // serve path resolves to the victim key. The origin allow-list must reject it. - dbChainMockFns.limit.mockResolvedValueOnce([ - { - workspaceId: 'ws-attacker', - fileUrl: absoluteServeUrl('https://attacker.example', VICTIM_KEY), - }, - ]) - mockGetUserEntityPermissions.mockResolvedValue('admin') - - const granted = await verifyFileAccess(VICTIM_KEY, ATTACKER_USER, undefined, 'knowledge-base') - - expect(granted).toBe(false) - expect(mockGetUserEntityPermissions).not.toHaveBeenCalled() - }) - - it('denies access via an external URL that merely contains the key as a substring', async () => { - // PoC: a planted external URL containing the victim key must never authorize storage. - dbChainMockFns.limit.mockResolvedValueOnce([ - { - workspaceId: 'ws-attacker', - fileUrl: `https://attacker.example/anything/${VICTIM_KEY}/marker`, - }, - ]) - mockGetUserEntityPermissions.mockResolvedValue('admin') - - const granted = await verifyFileAccess(VICTIM_KEY, ATTACKER_USER, undefined, 'knowledge-base') - - expect(granted).toBe(false) - }) - - it('denies a later document planted in another workspace (ownership pins to earliest doc)', async () => { - // Ordered by uploadedAt asc: the victim owns the key, so the attacker's later doc is ignored. - dbChainMockFns.limit.mockResolvedValueOnce([ - { workspaceId: 'ws-victim', fileUrl: internalUrlFor(VICTIM_KEY) }, - { workspaceId: 'ws-attacker', fileUrl: internalUrlFor(VICTIM_KEY) }, - ]) - mockGetUserEntityPermissions.mockImplementation( - async (_userId: string, _type: string, workspaceId: string) => - workspaceId === 'ws-attacker' ? 'admin' : null - ) - - const granted = await verifyFileAccess(VICTIM_KEY, ATTACKER_USER, undefined, 'knowledge-base') - - expect(granted).toBe(false) - // Authorization is decided against the owning workspace only. - expect(mockGetUserEntityPermissions).toHaveBeenCalledWith( - ATTACKER_USER, - 'workspace', - 'ws-victim' - ) - expect(mockGetUserEntityPermissions).not.toHaveBeenCalledWith( - ATTACKER_USER, - 'workspace', - 'ws-attacker' - ) - }) - - it('denies a planted active document when the original owner document is archived', async () => { - // The earliest (victim) document is archived, so the file is retired; the attacker's - // later active document must not become the owner and grant cross-tenant access. - dbChainMockFns.limit.mockResolvedValueOnce([ - { workspaceId: 'ws-victim', fileUrl: internalUrlFor(VICTIM_KEY), archivedAt: new Date() }, - { workspaceId: 'ws-attacker', fileUrl: internalUrlFor(VICTIM_KEY) }, - ]) - mockGetUserEntityPermissions.mockResolvedValue('admin') - - const granted = await verifyFileAccess(VICTIM_KEY, ATTACKER_USER, undefined, 'knowledge-base') - - expect(granted).toBe(false) - expect(mockGetUserEntityPermissions).not.toHaveBeenCalled() - }) - - it('denies access when the owning document is soft-deleted (retired file not served)', async () => { - dbChainMockFns.limit.mockResolvedValueOnce([ - { workspaceId: 'ws-owner', fileUrl: internalUrlFor(VICTIM_KEY), deletedAt: new Date() }, - ]) - mockGetUserEntityPermissions.mockResolvedValue('admin') - - const granted = await verifyFileAccess(VICTIM_KEY, 'owner-user', undefined, 'knowledge-base') - - expect(granted).toBe(false) - expect(mockGetUserEntityPermissions).not.toHaveBeenCalled() - }) - - it('denies access when a substring document points at a different key', async () => { - dbChainMockFns.limit.mockResolvedValueOnce([ - { workspaceId: 'ws-attacker', fileUrl: internalUrlFor(`${VICTIM_KEY}-decoy`) }, - ]) - mockGetUserEntityPermissions.mockResolvedValue('admin') - - const granted = await verifyFileAccess(VICTIM_KEY, ATTACKER_USER, undefined, 'knowledge-base') - - expect(granted).toBe(false) - }) - - it('denies access when no document references the key', async () => { - dbChainMockFns.limit.mockResolvedValueOnce([]) - - const granted = await verifyFileAccess(VICTIM_KEY, ATTACKER_USER, undefined, 'knowledge-base') - - expect(granted).toBe(false) - expect(mockGetUserEntityPermissions).not.toHaveBeenCalled() - }) - - it('denies when the owning document has no workspace (fail closed)', async () => { - dbChainMockFns.limit.mockResolvedValueOnce([ - { workspaceId: null, fileUrl: internalUrlFor(VICTIM_KEY) }, - ]) - - const granted = await verifyFileAccess(VICTIM_KEY, ATTACKER_USER, undefined, 'knowledge-base') - - expect(granted).toBe(false) - expect(mockGetUserEntityPermissions).not.toHaveBeenCalled() - }) - - it('does not let a later workspace document override a null-workspace owner', async () => { - // Earliest owner has no workspace; a later attacker-workspace doc must not become owner. - dbChainMockFns.limit.mockResolvedValueOnce([ - { workspaceId: null, fileUrl: internalUrlFor(VICTIM_KEY) }, - { workspaceId: 'ws-attacker', fileUrl: internalUrlFor(VICTIM_KEY) }, - ]) - mockGetUserEntityPermissions.mockResolvedValue('admin') - - const granted = await verifyFileAccess(VICTIM_KEY, ATTACKER_USER, undefined, 'knowledge-base') - - expect(granted).toBe(false) - expect(mockGetUserEntityPermissions).not.toHaveBeenCalled() - }) -}) diff --git a/apps/sim/app/api/files/authorization.ts b/apps/sim/app/api/files/authorization.ts index 30c562697f1..07ec36261b4 100644 --- a/apps/sim/app/api/files/authorization.ts +++ b/apps/sim/app/api/files/authorization.ts @@ -1,16 +1,14 @@ import { db } from '@sim/db' import { document, knowledgeBase, workspaceFile } from '@sim/db/schema' import { createLogger } from '@sim/logger' -import { and, asc, eq, isNull, like, or } from 'drizzle-orm' +import { and, eq, isNull, like, or } from 'drizzle-orm' import { NextResponse } from 'next/server' -import { getEnv } from '@/lib/core/config/env' -import { getBaseUrl, getInternalApiBaseUrl, parseOriginList } from '@/lib/core/utils/urls' import { getFileMetadata } from '@/lib/uploads' import type { StorageContext } from '@/lib/uploads/config' import { BLOB_CHAT_CONFIG, S3_CHAT_CONFIG } from '@/lib/uploads/config' import type { StorageConfig } from '@/lib/uploads/core/storage-client' import { getFileMetadataByKey } from '@/lib/uploads/server/metadata' -import { extractStorageKey, inferContextFromKey } from '@/lib/uploads/utils/file-utils' +import { inferContextFromKey } from '@/lib/uploads/utils/file-utils' import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils' import { isUuid } from '@/executor/constants' @@ -372,72 +370,9 @@ async function verifyCopilotFileAccess( } } -/** - * Origins this app legitimately serves files from: the public base URL, the - * internal API base URL, and any configured `TRUSTED_ORIGINS`. A document - * `fileUrl` only authorizes storage access when it is relative (same-origin) or - * absolute with one of these origins. - */ -function getInternalServeOrigins(): Set { - const origins = new Set() - for (const resolve of [getBaseUrl, getInternalApiBaseUrl]) { - try { - origins.add(new URL(resolve()).origin) - } catch { - // NEXT_PUBLIC_APP_URL unset/invalid — skip; relative fileUrls still resolve. - } - } - for (const origin of parseOriginList(getEnv('TRUSTED_ORIGINS'))) { - origins.add(origin) - } - return origins -} - -/** - * Resolve a knowledge-base document's stored `fileUrl` to the canonical storage - * key it points at, but only for internal Sim file-serve URLs. - * - * Relative paths are same-origin by definition; absolute URLs must match an - * internal serve origin. A foreign host that embeds `/api/files/serve/` in its - * path, and `data:` URIs, return `null` so an attacker-planted document can - * never authorize access to another tenant's storage object. - */ -function resolveInternalKbKey(fileUrl: string | null, allowedOrigins: Set): string | null { - if (!fileUrl) { - return null - } - let pathname: string - if (fileUrl.startsWith('/')) { - pathname = fileUrl - } else { - try { - const parsed = new URL(fileUrl) - if (!allowedOrigins.has(parsed.origin)) { - return null - } - pathname = parsed.pathname - } catch { - return null - } - } - if (!pathname.startsWith('/api/files/serve/')) { - return null - } - const key = extractStorageKey(pathname) - return key.startsWith('kb/') ? key : null -} - /** * Verify access to KB files * KB files: kb/filename - * - * Access is authorized against the workspace that *owns* the storage object, - * never against an arbitrary document that merely references it. The owner is the - * earliest document (in any state) whose `fileUrl` canonically resolves to the - * exact requested key — not a substring/`LIKE` match. Access is granted only when - * that owning document is still active; an archived or deleted owner keeps the file - * retired, and a document planted later in another workspace can never become the - * owner. */ async function verifyKBFileAccess( cloudKey: string, @@ -445,79 +380,44 @@ async function verifyKBFileAccess( customConfig?: StorageConfig ): Promise { try { - // Ownership spans every document state: the earliest document to reference the key owns - // it, so a document planted later in another workspace can never claim a file whose - // original document was archived or deleted. LIKE only narrows candidates; the exact - // owner is resolved below. - const candidateDocuments = await db + const activeKbFileDocuments = await db .select({ workspaceId: knowledgeBase.workspaceId, - fileUrl: document.fileUrl, - archivedAt: document.archivedAt, - deletedAt: document.deletedAt, - userExcluded: document.userExcluded, - kbDeletedAt: knowledgeBase.deletedAt, }) .from(document) .innerJoin(knowledgeBase, eq(document.knowledgeBaseId, knowledgeBase.id)) .where( - or( - like(document.fileUrl, `%${cloudKey}%`), - like(document.fileUrl, `%${encodeURIComponent(cloudKey)}%`) + and( + eq(document.userExcluded, false), + isNull(document.archivedAt), + isNull(document.deletedAt), + isNull(knowledgeBase.deletedAt), + or( + like(document.fileUrl, `%${cloudKey}%`), + like(document.fileUrl, `%${encodeURIComponent(cloudKey)}%`) + ) ) ) - .orderBy(asc(document.uploadedAt)) - .limit(50) - - const allowedOrigins = getInternalServeOrigins() - const owningDocument = candidateDocuments.find( - (doc) => resolveInternalKbKey(doc.fileUrl, allowedOrigins) === cloudKey - ) - - if (owningDocument) { - const isActive = - !owningDocument.archivedAt && - !owningDocument.deletedAt && - !owningDocument.userExcluded && - !owningDocument.kbDeletedAt - - // The owning document is archived/deleted/excluded: the file is retired and not served, - // and the attacker's later active document is not the owner, so it cannot claim it. - if (!isActive) { - logger.warn('KB file access denied: owning document is not active', { userId, cloudKey }) - return false - } + .limit(10) - if (!owningDocument.workspaceId) { - logger.warn('KB file access denied: owning document has no workspace', { - userId, - cloudKey, - }) - return false + for (const doc of activeKbFileDocuments) { + if (!doc.workspaceId) { + continue } - const permission = await getUserEntityPermissions( - userId, - 'workspace', - owningDocument.workspaceId - ) + const permission = await getUserEntityPermissions(userId, 'workspace', doc.workspaceId) if (permission !== null) { - logger.debug('KB file access granted (owning document lookup)', { + logger.debug('KB file access granted (active document lookup)', { userId, - workspaceId: owningDocument.workspaceId, + workspaceId: doc.workspaceId, cloudKey, }) return true } - logger.warn('KB file access denied: user lacks permission on owning workspace', { - userId, - workspaceId: owningDocument.workspaceId, - cloudKey, - }) - return false } - // No owning document: metadata only lets us flag the deleted-file case; it never grants access. + // KB file access must resolve through an active KB document. Metadata alone is not enough + // because parent archives intentionally keep the underlying file bytes around for history. const fileRecord = await getFileMetadataByKey(cloudKey, 'knowledge-base', { includeDeleted: true, }) @@ -527,7 +427,7 @@ async function verifyKBFileAccess( return false } - logger.warn('KB file access denied because no owning KB document matched the file', { + logger.warn('KB file access denied because no active KB document matched the file', { cloudKey, userId, }) From 3f61c4a2f68d0ab369f57f2a0ca57e51a7712e20 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 30 May 2026 16:35:54 -0700 Subject: [PATCH 15/17] fix(sso): treat caller's own user-scoped provider as owned during conflict check Self-hosters often register SSO user-scoped via the CLI script (no SSO_ORGANIZATION_ID). If they later enable organizations and reconfigure the same domain org-scoped through the UI, the conflict check previously treated their own user-scoped row as another tenant's and returned a misleading 409. Recognize the caller's own user-scoped provider as owned so that migration is allowed, while still blocking another user's or another org's domain. Co-Authored-By: Claude Opus 4.8 --- apps/sim/app/api/auth/sso/register/route.test.ts | 16 ++++++++++++++++ apps/sim/app/api/auth/sso/register/route.ts | 4 ++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/apps/sim/app/api/auth/sso/register/route.test.ts b/apps/sim/app/api/auth/sso/register/route.test.ts index c2337bc719a..87f4cdd6744 100644 --- a/apps/sim/app/api/auth/sso/register/route.test.ts +++ b/apps/sim/app/api/auth/sso/register/route.test.ts @@ -169,6 +169,22 @@ describe('POST /api/auth/sso/register', () => { expect(mockRegisterSSOProvider).toHaveBeenCalledTimes(1) }) + it('lets an org admin adopt their own user-scoped provider for the same domain', async () => { + dbState.members = [{ organizationId: 'org1', role: 'owner' }] + dbState.providers = [{ domain: 'acme.com', userId: 'u1', organizationId: null }] + const res = await POST(request({ ...OIDC_BODY, orgId: 'org1' })) + expect(res.status).toBe(200) + expect(mockRegisterSSOProvider).toHaveBeenCalledTimes(1) + }) + + it("still blocks an org admin from claiming another user's user-scoped domain", async () => { + dbState.members = [{ organizationId: 'org1', role: 'owner' }] + dbState.providers = [{ domain: 'acme.com', userId: 'someone-else', organizationId: null }] + const res = await POST(request({ ...OIDC_BODY, orgId: 'org1' })) + expect(res.status).toBe(409) + expect(mockRegisterSSOProvider).not.toHaveBeenCalled() + }) + it('normalizes the domain before persisting it', async () => { dbState.members = [{ organizationId: 'org1', role: 'owner' }] const res = await POST(request({ ...OIDC_BODY, domain: 'ACME.com', orgId: 'org1' })) diff --git a/apps/sim/app/api/auth/sso/register/route.ts b/apps/sim/app/api/auth/sso/register/route.ts index 1636837bf22..235116fc9e4 100644 --- a/apps/sim/app/api/auth/sso/register/route.ts +++ b/apps/sim/app/api/auth/sso/register/route.ts @@ -77,8 +77,8 @@ export const POST = withRouteHandler(async (request: NextRequest) => { userId: string | null organizationId: string | null }): boolean => { - if (orgId) return provider.organizationId === orgId - return provider.userId === session.user.id && !provider.organizationId + if (provider.userId === session.user.id && !provider.organizationId) return true + return orgId ? provider.organizationId === orgId : false } const existingProviders = await db From 7a187239f2b067d2ddd00ae65a78e1ba2befa355 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 30 May 2026 17:08:20 -0700 Subject: [PATCH 16/17] revert(security): remove workspace-env admin gate Defer to a credential-based access model (separate change). Restores GET /api/workspaces/[id]/environment to main behavior and removes the test. Co-Authored-By: Claude Opus 4.8 --- .../workspaces/[id]/environment/route.test.ts | 108 ------------------ .../api/workspaces/[id]/environment/route.ts | 22 +--- 2 files changed, 6 insertions(+), 124 deletions(-) delete mode 100644 apps/sim/app/api/workspaces/[id]/environment/route.test.ts diff --git a/apps/sim/app/api/workspaces/[id]/environment/route.test.ts b/apps/sim/app/api/workspaces/[id]/environment/route.test.ts deleted file mode 100644 index eef1d2f9cd2..00000000000 --- a/apps/sim/app/api/workspaces/[id]/environment/route.test.ts +++ /dev/null @@ -1,108 +0,0 @@ -/** - * Tests for the workspace environment API route. - * - * @vitest-environment node - */ -import { authMock, authMockFns, permissionsMock, permissionsMockFns } from '@sim/testing' -import { NextRequest } from 'next/server' -import { beforeEach, describe, expect, it, vi } from 'vitest' - -const { mockGetPersonalAndWorkspaceEnv } = vi.hoisted(() => ({ - mockGetPersonalAndWorkspaceEnv: vi.fn(), -})) - -vi.mock('@/lib/auth', () => authMock) -vi.mock('@/lib/workspaces/permissions/utils', () => permissionsMock) -vi.mock('@/lib/environment/utils', () => ({ - getPersonalAndWorkspaceEnv: mockGetPersonalAndWorkspaceEnv, - invalidateEffectiveDecryptedEnvCache: vi.fn(), -})) - -import { GET } from '@/app/api/workspaces/[id]/environment/route' - -const WORKSPACE_ID = 'ws-1' - -function createRequest() { - return new NextRequest(`http://localhost/api/workspaces/${WORKSPACE_ID}/environment`) -} - -function createContext() { - return { params: Promise.resolve({ id: WORKSPACE_ID }) } -} - -const ENV_RESULT = { - personalEncrypted: {}, - workspaceEncrypted: { OPENAI_API_KEY: 'enc-1', DATABASE_URL: 'enc-2' }, - personalDecrypted: { MY_PERSONAL: 'personal-value' }, - workspaceDecrypted: { - OPENAI_API_KEY: 'sk-live-secret-value', - DATABASE_URL: 'postgres://user:password@db/prod', - }, - conflicts: [], - decryptionFailures: [], -} - -describe('GET /api/workspaces/[id]/environment', () => { - beforeEach(() => { - vi.clearAllMocks() - authMockFns.mockGetSession.mockResolvedValue({ user: { id: 'user-1' } }) - permissionsMockFns.mockGetWorkspaceById.mockResolvedValue({ id: WORKSPACE_ID }) - permissionsMockFns.mockHasWorkspaceAdminAccess.mockResolvedValue(false) - mockGetPersonalAndWorkspaceEnv.mockResolvedValue(ENV_RESULT) - }) - - it('returns decrypted workspace values to workspace admins', async () => { - permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue('admin') - permissionsMockFns.mockHasWorkspaceAdminAccess.mockResolvedValue(true) - - const res = await GET(createRequest(), createContext()) - expect(res.status).toBe(200) - - const body = await res.json() - expect(body.data.workspace).toEqual(ENV_RESULT.workspaceDecrypted) - expect(body.data.personal).toEqual(ENV_RESULT.personalDecrypted) - }) - - it.each(['write', 'read'] as const)( - 'returns only variable names (empty values) to non-admin %s members', - async (permission) => { - permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue(permission) - permissionsMockFns.mockHasWorkspaceAdminAccess.mockResolvedValue(false) - - const res = await GET(createRequest(), createContext()) - expect(res.status).toBe(200) - - const body = await res.json() - expect(body.data.workspace).toEqual({ OPENAI_API_KEY: '', DATABASE_URL: '' }) - expect(JSON.stringify(body.data.workspace)).not.toContain('sk-live-secret-value') - expect(JSON.stringify(body.data.workspace)).not.toContain('postgres://') - expect(body.data.personal).toEqual(ENV_RESULT.personalDecrypted) - } - ) - - it('returns decrypted values to an admin-access user whose permission row is not "admin" (owner/org-admin)', async () => { - permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue('write') - permissionsMockFns.mockHasWorkspaceAdminAccess.mockResolvedValue(true) - - const res = await GET(createRequest(), createContext()) - expect(res.status).toBe(200) - - const body = await res.json() - expect(body.data.workspace).toEqual(ENV_RESULT.workspaceDecrypted) - }) - - it('rejects users without any workspace permission', async () => { - permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue(null) - - const res = await GET(createRequest(), createContext()) - expect(res.status).toBe(401) - expect(mockGetPersonalAndWorkspaceEnv).not.toHaveBeenCalled() - }) - - it('rejects unauthenticated requests', async () => { - authMockFns.mockGetSession.mockResolvedValue(null) - - const res = await GET(createRequest(), createContext()) - expect(res.status).toBe(401) - }) -}) diff --git a/apps/sim/app/api/workspaces/[id]/environment/route.ts b/apps/sim/app/api/workspaces/[id]/environment/route.ts index 29643543617..47c6f4a1e59 100644 --- a/apps/sim/app/api/workspaces/[id]/environment/route.ts +++ b/apps/sim/app/api/workspaces/[id]/environment/route.ts @@ -23,11 +23,7 @@ import { getPersonalAndWorkspaceEnv, invalidateEffectiveDecryptedEnvCache, } from '@/lib/environment/utils' -import { - getUserEntityPermissions, - getWorkspaceById, - hasWorkspaceAdminAccess, -} from '@/lib/workspaces/permissions/utils' +import { getUserEntityPermissions, getWorkspaceById } from '@/lib/workspaces/permissions/utils' const logger = createLogger('WorkspaceEnvironmentAPI') @@ -57,21 +53,15 @@ export const GET = withRouteHandler( return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }) } - const [isAdmin, { workspaceEncrypted, workspaceDecrypted, personalDecrypted, conflicts }] = - await Promise.all([ - hasWorkspaceAdminAccess(userId, workspaceId), - getPersonalAndWorkspaceEnv(userId, workspaceId), - ]) - - // Only workspace admins may read plaintext secrets; others get variable names with empty values. - const workspace = isAdmin - ? workspaceDecrypted - : Object.fromEntries(Object.keys(workspaceEncrypted).map((key) => [key, ''])) + const { workspaceDecrypted, personalDecrypted, conflicts } = await getPersonalAndWorkspaceEnv( + userId, + workspaceId + ) return NextResponse.json( { data: { - workspace, + workspace: workspaceDecrypted, personal: personalDecrypted, conflicts, }, From 9119b6a3b545577c61410f96fb2bcffa4c1fdebd Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 30 May 2026 17:18:35 -0700 Subject: [PATCH 17/17] refactor(security): consolidate webhook path-collision check into one helper Extract findConflictingWebhookPathOwner to lib/webhooks/utils.server.ts as the single source of truth for cross-tenant path-collision detection, used by both webhook creation paths (deploy sync and the manual /api/webhooks route). This also repairs two latent issues in the manual route's previous inline check, which queried with limit(1) and only webhook.archivedAt: - limit(1) inspected one arbitrary row, so a same-workflow row could mask a foreign collision (false negative). The shared helper scans all matching rows. - It omitted isActive/workflow.archivedAt, so inactive or archived-workflow webhooks (which never receive deliveries) permanently blocked path reuse. The helper mirrors the runtime dispatcher's filter. Same-workflow webhook reuse for upsert is now a separate, explicit lookup. Co-Authored-By: Claude Opus 4.8 --- apps/sim/app/api/webhooks/route.ts | 41 ++++++++++++++++--------- apps/sim/lib/webhooks/deploy.ts | 44 +++------------------------ apps/sim/lib/webhooks/utils.server.ts | 37 ++++++++++++++++++++++ 3 files changed, 69 insertions(+), 53 deletions(-) diff --git a/apps/sim/app/api/webhooks/route.ts b/apps/sim/app/api/webhooks/route.ts index 4740909c2f5..80f34950ed2 100644 --- a/apps/sim/app/api/webhooks/route.ts +++ b/apps/sim/app/api/webhooks/route.ts @@ -27,7 +27,10 @@ import { } from '@/lib/webhooks/provider-subscriptions' import { getProviderHandler } from '@/lib/webhooks/providers' import { mergeNonUserFields } from '@/lib/webhooks/utils' -import { syncWebhooksForCredentialSet } from '@/lib/webhooks/utils.server' +import { + findConflictingWebhookPathOwner, + syncWebhooksForCredentialSet, +} from '@/lib/webhooks/utils.server' import { extractCredentialSetId, isCredentialSetValue } from '@/executor/constants' const logger = createLogger('WebhooksAPI') @@ -330,21 +333,31 @@ export const POST = withRouteHandler(async (request: NextRequest) => { } } if (!targetWebhookId) { - const existingByPath = await db - .select({ id: webhook.id, workflowId: webhook.workflowId }) + const conflictingOwner = await findConflictingWebhookPathOwner({ + path: finalPath, + workflowId, + }) + if (conflictingOwner) { + logger.warn(`[${requestId}] Webhook path conflict: ${finalPath}`) + return NextResponse.json( + { error: 'Webhook path already exists.', code: 'PATH_EXISTS' }, + { status: 409 } + ) + } + + const ownExisting = await db + .select({ id: webhook.id }) .from(webhook) - .where(and(eq(webhook.path, finalPath), isNull(webhook.archivedAt))) - .limit(1) - if (existingByPath.length > 0) { - // If a webhook with the same path exists but belongs to a different workflow, return an error - if (existingByPath[0].workflowId !== workflowId) { - logger.warn(`[${requestId}] Webhook path conflict: ${finalPath}`) - return NextResponse.json( - { error: 'Webhook path already exists.', code: 'PATH_EXISTS' }, - { status: 409 } + .where( + and( + eq(webhook.path, finalPath), + eq(webhook.workflowId, workflowId), + isNull(webhook.archivedAt) ) - } - targetWebhookId = existingByPath[0].id + ) + .limit(1) + if (ownExisting.length > 0) { + targetWebhookId = ownExisting[0].id } } diff --git a/apps/sim/lib/webhooks/deploy.ts b/apps/sim/lib/webhooks/deploy.ts index 373ab7f9885..f3fa642685b 100644 --- a/apps/sim/lib/webhooks/deploy.ts +++ b/apps/sim/lib/webhooks/deploy.ts @@ -1,11 +1,5 @@ import { db } from '@sim/db' -import { - account, - credentialSetMember, - webhook, - workflow, - workflowDeploymentVersion, -} from '@sim/db/schema' +import { account, credentialSetMember, webhook, workflowDeploymentVersion } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { generateShortId } from '@sim/utils/id' import { and, eq, inArray, isNotNull, isNull, or } from 'drizzle-orm' @@ -18,7 +12,10 @@ import { shouldRecreateExternalWebhookSubscription, } from '@/lib/webhooks/provider-subscriptions' import { getProviderHandler } from '@/lib/webhooks/providers' -import { syncWebhooksForCredentialSet } from '@/lib/webhooks/utils.server' +import { + findConflictingWebhookPathOwner, + syncWebhooksForCredentialSet, +} from '@/lib/webhooks/utils.server' import { buildCanonicalIndex } from '@/lib/workflows/subblocks/visibility' import { getBlock } from '@/blocks' import type { SubBlockConfig } from '@/blocks/types' @@ -29,37 +26,6 @@ import { SYSTEM_SUBBLOCK_IDS } from '@/triggers/constants' const logger = createLogger('DeployWebhookSync') const CREDENTIAL_SET_PREFIX = 'credentialSet:' -/** - * Returns the id of a different workflow that already owns an active webhook on - * the given path, or `null` if the path is free or owned by this workflow. - * Guards against cross-tenant path collisions, since paths are user-controlled - * and only unique per deployment version. Mirrors the runtime dispatcher's - * filter (active, non-archived webhook on a non-archived workflow) so webhooks - * that can never receive deliveries don't permanently reserve a path. - */ -async function findConflictingWebhookPathOwner(params: { - path: string - workflowId: string -}): Promise { - const { path, workflowId } = params - - const existing = await db - .select({ workflowId: webhook.workflowId }) - .from(webhook) - .innerJoin(workflow, eq(webhook.workflowId, workflow.id)) - .where( - and( - eq(webhook.path, path), - eq(webhook.isActive, true), - isNull(webhook.archivedAt), - isNull(workflow.archivedAt) - ) - ) - - const conflict = existing.find((row) => row.workflowId !== workflowId) - return conflict ? conflict.workflowId : null -} - interface TriggerSaveError { message: string status: number diff --git a/apps/sim/lib/webhooks/utils.server.ts b/apps/sim/lib/webhooks/utils.server.ts index a920553f16b..eab8f9cccfe 100644 --- a/apps/sim/lib/webhooks/utils.server.ts +++ b/apps/sim/lib/webhooks/utils.server.ts @@ -10,6 +10,43 @@ import { cleanupExternalWebhook } from '@/lib/webhooks/provider-subscriptions' import { getCredentialsForCredentialSet } from '@/app/api/auth/oauth/utils' import { isPollingWebhookProvider } from '@/triggers/constants' +/** + * Returns the id of a different workflow that already owns an active webhook on + * the given path, or `null` if the path is free or owned by `workflowId`. + * + * Webhook paths are user-controlled and the database only enforces uniqueness + * per deployment version, so this is the single guard against cross-tenant path + * collisions for every webhook creation path. The filter mirrors the runtime + * dispatcher (`findAllWebhooksForPath`): an active, non-archived webhook on a + * non-archived workflow — inactive or archived webhooks never receive + * deliveries, so they must not reserve a path. All matching rows are scanned so + * a same-workflow row can never mask a foreign collision. + */ +export async function findConflictingWebhookPathOwner(params: { + path: string + workflowId: string + tx?: DbOrTx +}): Promise { + const { path, workflowId, tx } = params + const dbCtx = tx ?? db + + const existing = await dbCtx + .select({ workflowId: webhook.workflowId }) + .from(webhook) + .innerJoin(workflow, eq(webhook.workflowId, workflow.id)) + .where( + and( + eq(webhook.path, path), + eq(webhook.isActive, true), + isNull(webhook.archivedAt), + isNull(workflow.archivedAt) + ) + ) + + const conflict = existing.find((row) => row.workflowId !== workflowId) + return conflict ? conflict.workflowId : null +} + /** * Result of syncing webhooks for a credential set */