Skip to content

Commit a8f86c0

Browse files
waleedlatif1claude
andauthored
fix(security): harden SSO domain registration, webhook path isolation, and CSV export (#4813)
* fix(security): harden KB file access, SSO domain registration, webhook path isolation, env secrets, and CSV export * 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) = <normalized>, keeping the in-memory ownership check. Also tighten the normalizeSSODomain TSDoc. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: condense env route security comments Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * icons update * 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 <noreply@anthropic.com> * 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/<victim-key> no longer resolves to the victim key. Relative same-origin URLs are unaffected. * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * fix(sso): rely on lower(domain) match for conflict detection, drop dead in-memory recheck Address PR review: the SQL `lower(domain) = <normalized>` 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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. * updated greptile icon * 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. * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 1d4a277 commit a8f86c0

12 files changed

Lines changed: 956 additions & 136 deletions

File tree

apps/docs/components/icons.tsx

Lines changed: 215 additions & 50 deletions
Large diffs are not rendered by default.
Lines changed: 196 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,196 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { createEnvMock, createMockRequest } from '@sim/testing'
5+
import { beforeEach, describe, expect, it, vi } from 'vitest'
6+
7+
const {
8+
mockGetSession,
9+
mockRegisterSSOProvider,
10+
mockHasSSOAccess,
11+
mockValidateUrlWithDNS,
12+
dbState,
13+
memberTable,
14+
ssoProviderTable,
15+
} = vi.hoisted(() => ({
16+
mockGetSession: vi.fn(),
17+
mockRegisterSSOProvider: vi.fn(),
18+
mockHasSSOAccess: vi.fn(),
19+
mockValidateUrlWithDNS: vi.fn(),
20+
dbState: { members: [] as any[], providers: [] as any[] },
21+
memberTable: {
22+
userId: 'member.userId',
23+
organizationId: 'member.organizationId',
24+
role: 'member.role',
25+
},
26+
ssoProviderTable: {
27+
id: 'sso.id',
28+
providerId: 'sso.providerId',
29+
domain: 'sso.domain',
30+
issuer: 'sso.issuer',
31+
userId: 'sso.userId',
32+
organizationId: 'sso.organizationId',
33+
oidcConfig: 'sso.oidcConfig',
34+
samlConfig: 'sso.samlConfig',
35+
},
36+
}))
37+
38+
function makeBuilder(rows: any[]): any {
39+
const thenable: any = Promise.resolve(rows)
40+
thenable.where = (condition: any) => {
41+
const values = condition?.values
42+
if (Array.isArray(values) && values.length > 0) {
43+
const target = String(values[values.length - 1]).toLowerCase()
44+
return makeBuilder(rows.filter((r) => String(r.domain ?? '').toLowerCase() === target))
45+
}
46+
return makeBuilder(rows)
47+
}
48+
thenable.limit = () => Promise.resolve(rows)
49+
thenable.orderBy = () => Promise.resolve(rows)
50+
return thenable
51+
}
52+
53+
vi.mock('@sim/db', () => ({
54+
db: {
55+
select: () => ({
56+
from: (table: unknown) =>
57+
makeBuilder(table === memberTable ? dbState.members : dbState.providers),
58+
}),
59+
},
60+
member: memberTable,
61+
ssoProvider: ssoProviderTable,
62+
}))
63+
64+
vi.mock('@/lib/auth', () => ({
65+
getSession: mockGetSession,
66+
auth: { api: { registerSSOProvider: mockRegisterSSOProvider } },
67+
}))
68+
69+
vi.mock('@/lib/billing', () => ({
70+
hasSSOAccess: mockHasSSOAccess,
71+
}))
72+
73+
vi.mock('@/lib/auth/sso/domain', () => ({
74+
normalizeSSODomain: (input: unknown): string | null => {
75+
if (typeof input !== 'string') return null
76+
const value = input.trim().toLowerCase()
77+
return /^[a-z0-9-]+(\.[a-z0-9-]+)+$/.test(value) ? value : null
78+
},
79+
}))
80+
81+
vi.mock('@/lib/core/security/input-validation.server', () => ({
82+
validateUrlWithDNS: mockValidateUrlWithDNS,
83+
secureFetchWithPinnedIP: vi.fn(),
84+
}))
85+
86+
vi.mock('@/lib/core/config/env', () => createEnvMock({ SSO_ENABLED: 'true' }))
87+
88+
import { POST } from '@/app/api/auth/sso/register/route'
89+
90+
const OIDC_BODY = {
91+
providerType: 'oidc' as const,
92+
providerId: 'acme-oidc',
93+
issuer: 'https://idp.acme.com',
94+
domain: 'acme.com',
95+
clientId: 'client-id',
96+
clientSecret: 'client-secret',
97+
authorizationEndpoint: 'https://idp.acme.com/authorize',
98+
tokenEndpoint: 'https://idp.acme.com/token',
99+
userInfoEndpoint: 'https://idp.acme.com/userinfo',
100+
jwksEndpoint: 'https://idp.acme.com/jwks',
101+
}
102+
103+
function request(body: Record<string, unknown>) {
104+
return createMockRequest('POST', body)
105+
}
106+
107+
describe('POST /api/auth/sso/register', () => {
108+
beforeEach(() => {
109+
vi.clearAllMocks()
110+
dbState.members = []
111+
dbState.providers = []
112+
mockGetSession.mockResolvedValue({ user: { id: 'u1' } })
113+
mockHasSSOAccess.mockResolvedValue(true)
114+
mockValidateUrlWithDNS.mockResolvedValue({ isValid: true, resolvedIP: '1.2.3.4' })
115+
mockRegisterSSOProvider.mockResolvedValue({ providerId: 'acme-oidc' })
116+
})
117+
118+
it('rejects callers without an Enterprise plan', async () => {
119+
mockHasSSOAccess.mockResolvedValue(false)
120+
const res = await POST(request({ ...OIDC_BODY, orgId: 'org1' }))
121+
expect(res.status).toBe(403)
122+
expect(mockRegisterSSOProvider).not.toHaveBeenCalled()
123+
})
124+
125+
it('rejects callers who are not an admin/owner of the target org', async () => {
126+
dbState.members = [{ organizationId: 'org1', role: 'member' }]
127+
const res = await POST(request({ ...OIDC_BODY, orgId: 'org1' }))
128+
expect(res.status).toBe(403)
129+
expect(mockRegisterSSOProvider).not.toHaveBeenCalled()
130+
})
131+
132+
it('rejects an invalid domain', async () => {
133+
dbState.members = [{ organizationId: 'org1', role: 'owner' }]
134+
const res = await POST(request({ ...OIDC_BODY, domain: 'not-a-domain', orgId: 'org1' }))
135+
expect(res.status).toBe(400)
136+
expect(mockRegisterSSOProvider).not.toHaveBeenCalled()
137+
})
138+
139+
it('rejects a domain already registered by another organization', async () => {
140+
dbState.members = [{ organizationId: 'org-attacker', role: 'owner' }]
141+
dbState.providers = [{ domain: 'acme.com', userId: 'u-victim', organizationId: 'org-victim' }]
142+
const res = await POST(request({ ...OIDC_BODY, orgId: 'org-attacker' }))
143+
const json = await res.json()
144+
expect(res.status).toBe(409)
145+
expect(json.code).toBe('SSO_DOMAIN_ALREADY_REGISTERED')
146+
expect(mockRegisterSSOProvider).not.toHaveBeenCalled()
147+
})
148+
149+
it('matches conflicts across casing variants', async () => {
150+
dbState.members = [{ organizationId: 'org-attacker', role: 'owner' }]
151+
dbState.providers = [{ domain: 'ACME.com', userId: 'u-victim', organizationId: 'org-victim' }]
152+
const res = await POST(request({ ...OIDC_BODY, orgId: 'org-attacker' }))
153+
expect(res.status).toBe(409)
154+
expect(mockRegisterSSOProvider).not.toHaveBeenCalled()
155+
})
156+
157+
it('registers when the domain is unclaimed', async () => {
158+
dbState.members = [{ organizationId: 'org1', role: 'owner' }]
159+
const res = await POST(request({ ...OIDC_BODY, orgId: 'org1' }))
160+
expect(res.status).toBe(200)
161+
expect(mockRegisterSSOProvider).toHaveBeenCalledTimes(1)
162+
})
163+
164+
it('allows the owning tenant to update its own provider for the same domain', async () => {
165+
dbState.members = [{ organizationId: 'org1', role: 'owner' }]
166+
dbState.providers = [{ domain: 'acme.com', userId: 'u1', organizationId: 'org1' }]
167+
const res = await POST(request({ ...OIDC_BODY, orgId: 'org1' }))
168+
expect(res.status).toBe(200)
169+
expect(mockRegisterSSOProvider).toHaveBeenCalledTimes(1)
170+
})
171+
172+
it('lets an org admin adopt their own user-scoped provider for the same domain', async () => {
173+
dbState.members = [{ organizationId: 'org1', role: 'owner' }]
174+
dbState.providers = [{ domain: 'acme.com', userId: 'u1', organizationId: null }]
175+
const res = await POST(request({ ...OIDC_BODY, orgId: 'org1' }))
176+
expect(res.status).toBe(200)
177+
expect(mockRegisterSSOProvider).toHaveBeenCalledTimes(1)
178+
})
179+
180+
it("still blocks an org admin from claiming another user's user-scoped domain", async () => {
181+
dbState.members = [{ organizationId: 'org1', role: 'owner' }]
182+
dbState.providers = [{ domain: 'acme.com', userId: 'someone-else', organizationId: null }]
183+
const res = await POST(request({ ...OIDC_BODY, orgId: 'org1' }))
184+
expect(res.status).toBe(409)
185+
expect(mockRegisterSSOProvider).not.toHaveBeenCalled()
186+
})
187+
188+
it('normalizes the domain before persisting it', async () => {
189+
dbState.members = [{ organizationId: 'org1', role: 'owner' }]
190+
const res = await POST(request({ ...OIDC_BODY, domain: 'ACME.com', orgId: 'org1' }))
191+
expect(res.status).toBe(200)
192+
expect(mockRegisterSSOProvider).toHaveBeenCalledTimes(1)
193+
const config = mockRegisterSSOProvider.mock.calls[0][0].body
194+
expect(config.domain).toBe('acme.com')
195+
})
196+
})

apps/sim/app/api/auth/sso/register/route.ts

Lines changed: 40 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
11
import { db, member, ssoProvider } from '@sim/db'
22
import { createLogger } from '@sim/logger'
33
import { getErrorMessage } from '@sim/utils/errors'
4-
import { and, eq } from 'drizzle-orm'
4+
import { and, eq, sql } from 'drizzle-orm'
55
import { type NextRequest, NextResponse } from 'next/server'
66
import { ssoRegistrationContract } from '@/lib/api/contracts/auth'
77
import { getValidationErrorMessage, parseRequest } from '@/lib/api/server'
88
import { auth, getSession } from '@/lib/auth'
9+
import { normalizeSSODomain } from '@/lib/auth/sso/domain'
910
import { hasSSOAccess } from '@/lib/billing'
1011
import { env } from '@/lib/core/config/env'
1112
import {
@@ -51,7 +52,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
5152
if (!parsed.success) return parsed.response
5253

5354
const body = parsed.data.body
54-
const { providerId, issuer, domain, providerType, mapping, orgId } = body
55+
const { providerId, issuer, providerType, mapping, orgId } = body
5556

5657
if (orgId) {
5758
const [membership] = await db
@@ -67,6 +68,43 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
6768
}
6869
}
6970

71+
const domain = normalizeSSODomain(body.domain)
72+
if (!domain) {
73+
return NextResponse.json({ error: 'Enter a valid domain like company.com' }, { status: 400 })
74+
}
75+
76+
const isOwnedByCaller = (provider: {
77+
userId: string | null
78+
organizationId: string | null
79+
}): boolean => {
80+
if (provider.userId === session.user.id && !provider.organizationId) return true
81+
return orgId ? provider.organizationId === orgId : false
82+
}
83+
84+
const existingProviders = await db
85+
.select({
86+
userId: ssoProvider.userId,
87+
organizationId: ssoProvider.organizationId,
88+
})
89+
.from(ssoProvider)
90+
.where(sql`lower(${ssoProvider.domain}) = ${domain}`)
91+
const conflictingProvider = existingProviders.find((provider) => !isOwnedByCaller(provider))
92+
93+
if (conflictingProvider) {
94+
logger.warn('Rejected SSO registration for domain owned by another tenant', {
95+
domain,
96+
orgId,
97+
userId: session.user.id,
98+
})
99+
return NextResponse.json(
100+
{
101+
error: 'This domain is already registered for SSO by another organization.',
102+
code: 'SSO_DOMAIN_ALREADY_REGISTERED',
103+
},
104+
{ status: 409 }
105+
)
106+
}
107+
70108
const headers: Record<string, string> = {}
71109
request.headers.forEach((value, key) => {
72110
headers[key] = value

apps/sim/app/api/table/[tableId]/export/route.ts

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,9 @@ export const GET = withRouteHandler(async (request: NextRequest, { params }: Rou
5353
const encoder = new TextEncoder()
5454
try {
5555
if (format === 'csv') {
56-
controller.enqueue(encoder.encode(`${toCsvRow(columns.map((c) => c.name))}\n`))
56+
controller.enqueue(
57+
encoder.encode(`${toCsvRow(columns.map((c) => neutralizeCsvFormula(c.name)))}\n`)
58+
)
5759
} else {
5860
controller.enqueue(encoder.encode('['))
5961
}
@@ -111,10 +113,23 @@ function sanitizeFilename(name: string): string {
111113
return cleaned || 'table'
112114
}
113115

116+
/**
117+
* Prefixes a single quote to values starting with a spreadsheet formula trigger
118+
* (`=`, `+`, `-`, `@`, tab, CR), neutralizing CSV injection in Excel/Sheets.
119+
*/
120+
function neutralizeCsvFormula(value: string): string {
121+
return /^[=+\-@\t\r]/.test(value) ? `'${value}` : value
122+
}
123+
124+
/**
125+
* Serializes a cell for CSV. Only string cells are formula-neutralized; numbers,
126+
* booleans, dates, and JSON objects can never form a trigger and pass through verbatim.
127+
*/
114128
function formatCsvValue(value: unknown): string {
115129
if (value === null || value === undefined) return ''
116130
if (value instanceof Date) return value.toISOString()
117131
if (typeof value === 'object') return JSON.stringify(value)
132+
if (typeof value === 'string') return neutralizeCsvFormula(value)
118133
return String(value)
119134
}
120135

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

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,10 @@ import {
2727
} from '@/lib/webhooks/provider-subscriptions'
2828
import { getProviderHandler } from '@/lib/webhooks/providers'
2929
import { mergeNonUserFields } from '@/lib/webhooks/utils'
30-
import { syncWebhooksForCredentialSet } from '@/lib/webhooks/utils.server'
30+
import {
31+
findConflictingWebhookPathOwner,
32+
syncWebhooksForCredentialSet,
33+
} from '@/lib/webhooks/utils.server'
3134
import { extractCredentialSetId, isCredentialSetValue } from '@/executor/constants'
3235

3336
const logger = createLogger('WebhooksAPI')
@@ -330,21 +333,31 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
330333
}
331334
}
332335
if (!targetWebhookId) {
333-
const existingByPath = await db
334-
.select({ id: webhook.id, workflowId: webhook.workflowId })
336+
const conflictingOwner = await findConflictingWebhookPathOwner({
337+
path: finalPath,
338+
workflowId,
339+
})
340+
if (conflictingOwner) {
341+
logger.warn(`[${requestId}] Webhook path conflict: ${finalPath}`)
342+
return NextResponse.json(
343+
{ error: 'Webhook path already exists.', code: 'PATH_EXISTS' },
344+
{ status: 409 }
345+
)
346+
}
347+
348+
const ownExisting = await db
349+
.select({ id: webhook.id })
335350
.from(webhook)
336-
.where(and(eq(webhook.path, finalPath), isNull(webhook.archivedAt)))
337-
.limit(1)
338-
if (existingByPath.length > 0) {
339-
// If a webhook with the same path exists but belongs to a different workflow, return an error
340-
if (existingByPath[0].workflowId !== workflowId) {
341-
logger.warn(`[${requestId}] Webhook path conflict: ${finalPath}`)
342-
return NextResponse.json(
343-
{ error: 'Webhook path already exists.', code: 'PATH_EXISTS' },
344-
{ status: 409 }
351+
.where(
352+
and(
353+
eq(webhook.path, finalPath),
354+
eq(webhook.workflowId, workflowId),
355+
isNull(webhook.archivedAt)
345356
)
346-
}
347-
targetWebhookId = existingByPath[0].id
357+
)
358+
.limit(1)
359+
if (ownExisting.length > 0) {
360+
targetWebhookId = ownExisting[0].id
348361
}
349362
}
350363

0 commit comments

Comments
 (0)