fix(security): harden KB file access, SSO domain registration, webhook path isolation, env secrets, and CSV export

Author: waleedlatif1

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<string, unknown>) {
+  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')
+  })
+})

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<string, string> = {}
     request.headers.forEach((value, key) => {
       headers[key] = value

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()
+  })
+})