From c66a882b7bf424653eddc8b81278b961d4737528 Mon Sep 17 00:00:00 2001 From: waleed Date: Sat, 6 Jun 2026 12:07:36 -0700 Subject: [PATCH 01/10] fix(clickhouse): pin outbound HTTP connection to validated IP (DNS rebinding) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit clickhouseRequest() validated config.host via validateDatabaseHost() but discarded the resolved IP and called fetch() with the original hostname, triggering a second DNS lookup. A workflow author controlling the host parameter could use DNS rebinding to pass validation against a public IP and then connect to an internal/private address (SSRF). Replace fetch() with secureFetchWithPinnedIP(), connecting to the validated resolvedIP while preserving the hostname for Host/TLS SNI — the same DNS-pinning pattern used by the other DB tools. Set Content-Length explicitly so request framing is identical to the previous fetch. Add tests locking the contract: connection targets the validated IP not the hostname, no request is issued on validation failure, http/https and allowHttp are selected from secure, and body/headers propagate. --- .../app/api/tools/clickhouse/utils.test.ts | 126 ++++++++++++++++++ apps/sim/app/api/tools/clickhouse/utils.ts | 34 +++-- 2 files changed, 142 insertions(+), 18 deletions(-) create mode 100644 apps/sim/app/api/tools/clickhouse/utils.test.ts diff --git a/apps/sim/app/api/tools/clickhouse/utils.test.ts b/apps/sim/app/api/tools/clickhouse/utils.test.ts new file mode 100644 index 0000000000..ceac1af924 --- /dev/null +++ b/apps/sim/app/api/tools/clickhouse/utils.test.ts @@ -0,0 +1,126 @@ +/** + * @vitest-environment node + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { ClickHouseConnectionConfig } from '@/tools/clickhouse/types' + +const { mockValidateDatabaseHost, mockSecureFetchWithPinnedIP, mockValidateSqlWhereClause } = + vi.hoisted(() => ({ + mockValidateDatabaseHost: vi.fn(), + mockSecureFetchWithPinnedIP: vi.fn(), + mockValidateSqlWhereClause: vi.fn(), + })) + +vi.mock('@/lib/core/security/input-validation.server', () => ({ + validateDatabaseHost: mockValidateDatabaseHost, + secureFetchWithPinnedIP: mockSecureFetchWithPinnedIP, + validateSqlWhereClause: mockValidateSqlWhereClause, +})) + +import { executeClickHouseInsert, executeClickHouseQuery } from '@/app/api/tools/clickhouse/utils' + +function makeConfig( + overrides: Partial = {} +): ClickHouseConnectionConfig { + return { + host: 'clickhouse.example.com', + port: 8123, + database: 'default', + username: 'default', + password: 'secret', + secure: false, + ...overrides, + } +} + +function okResponse(body: string, summary?: string) { + return { + ok: true, + status: 200, + statusText: 'OK', + text: async () => body, + headers: { + get: (name: string) => + name.toLowerCase() === 'x-clickhouse-summary' ? (summary ?? null) : null, + }, + } +} + +describe('clickhouseRequest DNS pinning', () => { + beforeEach(() => { + vi.clearAllMocks() + mockValidateDatabaseHost.mockResolvedValue({ + isValid: true, + resolvedIP: '93.184.216.34', + originalHostname: 'clickhouse.example.com', + }) + mockValidateSqlWhereClause.mockReturnValue({ isValid: true }) + mockSecureFetchWithPinnedIP.mockResolvedValue(okResponse('{"data":[{"x":1}],"rows":1}')) + }) + + it('pins the connection to the validated IP, not the attacker-controlled hostname', async () => { + await executeClickHouseQuery(makeConfig({ host: 'rebind.attacker.example' }), 'SELECT 1') + + expect(mockValidateDatabaseHost).toHaveBeenCalledWith('rebind.attacker.example', 'host') + expect(mockSecureFetchWithPinnedIP).toHaveBeenCalledTimes(1) + + const [url, pinnedIP, options] = mockSecureFetchWithPinnedIP.mock.calls[0] + // The actual TCP target is the validated IP — re-resolution of the hostname can never happen. + expect(pinnedIP).toBe('93.184.216.34') + // The hostname is preserved only in the URL (for Host header / TLS SNI), never used to connect. + expect(url).toContain('rebind.attacker.example') + expect(options.method).toBe('POST') + }) + + it('never issues the request when host validation fails (no SSRF window)', async () => { + mockValidateDatabaseHost.mockResolvedValue({ + isValid: false, + error: 'host resolves to a blocked IP address', + }) + + await expect(executeClickHouseQuery(makeConfig(), 'SELECT 1')).rejects.toThrow( + 'host resolves to a blocked IP address' + ) + expect(mockSecureFetchWithPinnedIP).not.toHaveBeenCalled() + }) + + it('uses https and disallows http redirects when secure is true', async () => { + await executeClickHouseQuery(makeConfig({ secure: true, port: 8443 }), 'SELECT 1') + + const [url, , options] = mockSecureFetchWithPinnedIP.mock.calls[0] + expect(url).toMatch(/^https:\/\//) + expect(options.allowHttp).toBe(false) + }) + + it('allows http for the initial request when secure is false', async () => { + await executeClickHouseQuery(makeConfig({ secure: false }), 'SELECT 1') + + const [url, , options] = mockSecureFetchWithPinnedIP.mock.calls[0] + expect(url).toMatch(/^http:\/\//) + expect(options.allowHttp).toBe(true) + }) + + it('sends the statement as the body with a matching Content-Length and auth headers', async () => { + await executeClickHouseInsert(makeConfig(), 'events', { id: 1 }) + + const [, , options] = mockSecureFetchWithPinnedIP.mock.calls[0] + expect(options.body).toContain('INSERT INTO `events` FORMAT JSONEachRow') + expect(options.headers['Content-Length']).toBe(String(Buffer.byteLength(options.body, 'utf-8'))) + expect(options.headers['X-ClickHouse-User']).toBe('default') + expect(options.headers['X-ClickHouse-Key']).toBe('secret') + }) + + it('propagates non-ok responses as errors with the body text', async () => { + mockSecureFetchWithPinnedIP.mockResolvedValue({ + ok: false, + status: 400, + statusText: 'Bad Request', + text: async () => 'Code: 62. DB::Exception: Syntax error', + headers: { get: () => null }, + }) + + await expect(executeClickHouseQuery(makeConfig(), 'SELECT 1')).rejects.toThrow( + 'Code: 62. DB::Exception: Syntax error' + ) + }) +}) diff --git a/apps/sim/app/api/tools/clickhouse/utils.ts b/apps/sim/app/api/tools/clickhouse/utils.ts index ce4ad3afcb..e0687791a7 100644 --- a/apps/sim/app/api/tools/clickhouse/utils.ts +++ b/apps/sim/app/api/tools/clickhouse/utils.ts @@ -1,4 +1,5 @@ import { + secureFetchWithPinnedIP, validateDatabaseHost, validateSqlWhereClause, } from '@/lib/core/security/input-validation.server' @@ -81,24 +82,21 @@ async function clickhouseRequest( url.searchParams.set('readonly', '1') } - const controller = new AbortController() - const timeout = setTimeout(() => controller.abort(), REQUEST_TIMEOUT_MS) - - let response: Response - try { - response = await fetch(url.toString(), { - method: 'POST', - headers: { - 'X-ClickHouse-User': config.username, - 'X-ClickHouse-Key': config.password, - 'Content-Type': 'text/plain; charset=utf-8', - }, - body: statement, - signal: controller.signal, - }) - } finally { - clearTimeout(timeout) - } + // Pin the connection to the IP that passed validation. Without this, fetch() + // would re-resolve `config.host` and a DNS-rebinding hostname could point the + // actual request at an internal/private address after validation succeeded. + const response = await secureFetchWithPinnedIP(url.toString(), hostValidation.resolvedIP!, { + method: 'POST', + headers: { + 'X-ClickHouse-User': config.username, + 'X-ClickHouse-Key': config.password, + 'Content-Type': 'text/plain; charset=utf-8', + 'Content-Length': String(Buffer.byteLength(statement, 'utf-8')), + }, + body: statement, + timeout: REQUEST_TIMEOUT_MS, + allowHttp: !config.secure, + }) const text = await response.text() From 37373b8e397d42106140909592fe90624f08dbcf Mon Sep 17 00:00:00 2001 From: waleed Date: Sat, 6 Jun 2026 12:07:41 -0700 Subject: [PATCH 02/10] fix(mcp): pin auth-type probe to validated IP to close SSRF/DNS-rebinding window The MCP auth-type probe (detectMcpAuthType) issued raw, unpinned fetch() calls against the user-supplied server URL, re-resolving DNS independently of validateMcpServerSsrf. This re-opened the exact DNS-rebinding (TOCTOU) window the pinned McpClient path was built to close: a hostname that resolves to a public IP during validation could resolve to an internal IP during the probe. The probe now pins to the IP already validated by the caller via createMcpPinnedFetch(resolvedIP); when no pre-validated IP is available it falls back to createSsrfGuardedMcpFetch(), which validates and pins each request. The best-effort session-close DELETE reuses the same pinned fetch. Both call sites (test-connection route and performCreateMcpServer) thread the resolved IP into the probe. --- .../api/mcp/servers/test-connection/route.ts | 2 +- apps/sim/lib/mcp/oauth/probe.test.ts | 112 ++++++++++++++++++ apps/sim/lib/mcp/oauth/probe.ts | 35 +++++- .../lib/mcp/orchestration/server-lifecycle.ts | 24 ++-- 4 files changed, 158 insertions(+), 15 deletions(-) create mode 100644 apps/sim/lib/mcp/oauth/probe.test.ts diff --git a/apps/sim/app/api/mcp/servers/test-connection/route.ts b/apps/sim/app/api/mcp/servers/test-connection/route.ts index b570f8f8ff..a72c0c506b 100644 --- a/apps/sim/app/api/mcp/servers/test-connection/route.ts +++ b/apps/sim/app/api/mcp/servers/test-connection/route.ts @@ -173,7 +173,7 @@ export const POST = withRouteHandler( // Skip unauth connect when the server returns an RFC 9728 OAuth challenge. if (testConfig.url) { - const detectedAuthType = await detectMcpAuthType(testConfig.url) + const detectedAuthType = await detectMcpAuthType(testConfig.url, resolvedIP) if (detectedAuthType === 'oauth') { result.authRequired = true result.authType = 'oauth' diff --git a/apps/sim/lib/mcp/oauth/probe.test.ts b/apps/sim/lib/mcp/oauth/probe.test.ts new file mode 100644 index 0000000000..34e7d6199e --- /dev/null +++ b/apps/sim/lib/mcp/oauth/probe.test.ts @@ -0,0 +1,112 @@ +/** + * @vitest-environment node + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { + mockCreateMcpPinnedFetch, + mockCreateSsrfGuardedMcpFetch, + mockPinnedFetch, + mockGuardedFetch, +} = vi.hoisted(() => { + const mockPinnedFetch = vi.fn() + const mockGuardedFetch = vi.fn() + return { + mockPinnedFetch, + mockGuardedFetch, + mockCreateMcpPinnedFetch: vi.fn(() => mockPinnedFetch), + mockCreateSsrfGuardedMcpFetch: vi.fn(() => mockGuardedFetch), + } +}) + +vi.mock('@/lib/mcp/pinned-fetch', () => ({ + createMcpPinnedFetch: mockCreateMcpPinnedFetch, + createSsrfGuardedMcpFetch: mockCreateSsrfGuardedMcpFetch, +})) + +import { detectMcpAuthType } from '@/lib/mcp/oauth/probe' + +function makeResponse(init: { status?: number; headers?: Record }): Response { + const status = init.status ?? 200 + return { + status, + ok: status >= 200 && status < 300, + headers: new Headers(init.headers ?? {}), + } as unknown as Response +} + +describe('detectMcpAuthType — connection pinning (SSRF / DNS-rebinding)', () => { + let globalFetchSpy: ReturnType + + beforeEach(() => { + vi.clearAllMocks() + globalFetchSpy = vi.fn() + vi.stubGlobal('fetch', globalFetchSpy) + }) + + it('pins the probe to the pre-validated IP when resolvedIP is supplied', async () => { + mockPinnedFetch.mockResolvedValue(makeResponse({ status: 200 })) + + const authType = await detectMcpAuthType('https://rebind.example.com/mcp', '203.0.113.10') + + expect(authType).toBe('none') + expect(mockCreateMcpPinnedFetch).toHaveBeenCalledWith('203.0.113.10') + expect(mockCreateSsrfGuardedMcpFetch).not.toHaveBeenCalled() + expect(mockPinnedFetch).toHaveBeenCalledTimes(1) + // The unpinned global fetch must never be used — that was the SSRF sink. + expect(globalFetchSpy).not.toHaveBeenCalled() + }) + + it('falls back to the SSRF-guarded fetch when no resolvedIP is supplied', async () => { + mockGuardedFetch.mockResolvedValue(makeResponse({ status: 200 })) + + const authType = await detectMcpAuthType('https://example.com/mcp') + + expect(authType).toBe('none') + expect(mockCreateSsrfGuardedMcpFetch).toHaveBeenCalledTimes(1) + expect(mockCreateMcpPinnedFetch).not.toHaveBeenCalled() + expect(mockGuardedFetch).toHaveBeenCalledTimes(1) + expect(globalFetchSpy).not.toHaveBeenCalled() + }) + + it('classifies an RFC 9728 OAuth challenge as oauth via the pinned fetch', async () => { + mockPinnedFetch.mockResolvedValue( + makeResponse({ + status: 401, + headers: { + 'www-authenticate': + 'Bearer resource_metadata="https://example.com/.well-known/oauth-protected-resource"', + }, + }) + ) + + const authType = await detectMcpAuthType('https://example.com/mcp', '203.0.113.10') + + expect(authType).toBe('oauth') + expect(globalFetchSpy).not.toHaveBeenCalled() + }) + + it('does not probe (no network call) for non-https, non-loopback URLs', async () => { + const authType = await detectMcpAuthType('http://example.com/mcp', '203.0.113.10') + + expect(authType).toBe('headers') + expect(mockCreateMcpPinnedFetch).not.toHaveBeenCalled() + expect(mockCreateSsrfGuardedMcpFetch).not.toHaveBeenCalled() + expect(globalFetchSpy).not.toHaveBeenCalled() + }) + + it('reuses the pinned fetch for best-effort session cleanup (DELETE)', async () => { + mockPinnedFetch + .mockResolvedValueOnce(makeResponse({ status: 200, headers: { 'mcp-session-id': 'sess-1' } })) + .mockResolvedValueOnce(makeResponse({ status: 200 })) + + const authType = await detectMcpAuthType('https://example.com/mcp', '203.0.113.10') + + expect(authType).toBe('none') + // POST probe + DELETE cleanup, both through the pinned fetch. + await vi.waitFor(() => expect(mockPinnedFetch).toHaveBeenCalledTimes(2)) + const deleteCall = mockPinnedFetch.mock.calls[1] + expect(deleteCall[1]).toMatchObject({ method: 'DELETE' }) + expect(globalFetchSpy).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/lib/mcp/oauth/probe.ts b/apps/sim/lib/mcp/oauth/probe.ts index a9892d4e6e..887ba8ce97 100644 --- a/apps/sim/lib/mcp/oauth/probe.ts +++ b/apps/sim/lib/mcp/oauth/probe.ts @@ -1,13 +1,26 @@ import { extractWWWAuthenticateParams } from '@modelcontextprotocol/sdk/client/auth.js' +import type { FetchLike } from '@modelcontextprotocol/sdk/shared/transport.js' import { createLogger } from '@sim/logger' import { isLoopbackHostname } from '@/lib/core/utils/urls' +import { createMcpPinnedFetch, createSsrfGuardedMcpFetch } from '@/lib/mcp/pinned-fetch' import type { McpAuthType } from '@/lib/mcp/types' const logger = createLogger('McpOauthProbe') const PROBE_TIMEOUT_MS = 5000 -export async function detectMcpAuthType(url: string): Promise { +/** + * Probes an MCP server URL to classify its auth requirement. + * + * The probe must never re-resolve DNS independently of the caller's SSRF + * validation, or it re-opens the DNS-rebinding window. When the caller passes a + * pre-validated `resolvedIP` the connection is pinned to it; otherwise an + * SSRF-guarded fetch validates and pins each request itself. + */ +export async function detectMcpAuthType( + url: string, + resolvedIP?: string | null +): Promise { let parsed: URL try { parsed = new URL(url) @@ -18,11 +31,16 @@ export async function detectMcpAuthType(url: string): Promise { if (parsed.protocol !== 'https:' && !isLoopbackHttp) { return 'headers' } + + const probeFetch: FetchLike = resolvedIP + ? createMcpPinnedFetch(resolvedIP) + : createSsrfGuardedMcpFetch() + const controller = new AbortController() const timer = setTimeout(() => controller.abort(), PROBE_TIMEOUT_MS) try { - const res = await fetch(url, { + const res = await probeFetch(url, { method: 'POST', redirect: 'manual', headers: { @@ -44,7 +62,7 @@ export async function detectMcpAuthType(url: string): Promise { const sessionId = res.headers.get('mcp-session-id') if (sessionId) { - void closeMcpSession(url, sessionId) + void closeMcpSession(url, sessionId, probeFetch) } if (res.status === 401) { @@ -71,14 +89,19 @@ export async function detectMcpAuthType(url: string): Promise { /** * Best-effort DELETE to release the streamable-HTTP session the probe just - * allocated. Failures are ignored — the session will expire on the server side. + * allocated. Reuses the probe's pinned fetch so this cleanup hop stays pinned. + * Failures are ignored — the session will expire on the server side. */ -async function closeMcpSession(url: string, sessionId: string): Promise { +async function closeMcpSession( + url: string, + sessionId: string, + probeFetch: FetchLike +): Promise { try { const controller = new AbortController() const timer = setTimeout(() => controller.abort(), PROBE_TIMEOUT_MS) try { - await fetch(url, { + await probeFetch(url, { method: 'DELETE', headers: { 'Mcp-Session-Id': sessionId }, signal: controller.signal, diff --git a/apps/sim/lib/mcp/orchestration/server-lifecycle.ts b/apps/sim/lib/mcp/orchestration/server-lifecycle.ts index 1a11e97484..d8d2d73115 100644 --- a/apps/sim/lib/mcp/orchestration/server-lifecycle.ts +++ b/apps/sim/lib/mcp/orchestration/server-lifecycle.ts @@ -86,17 +86,24 @@ export interface PerformMcpServerResult { authType?: McpAuthType } -async function validateMcpServerUrl(url: string): Promise { +type ValidateMcpServerUrlResult = + | { ok: true; resolvedIP: string | null } + | { ok: false; result: PerformMcpServerResult } + +async function validateMcpServerUrl(url: string): Promise { try { validateMcpDomain(url) - await validateMcpServerSsrf(url) - return null + const resolvedIP = await validateMcpServerSsrf(url) + return { ok: true, resolvedIP } } catch (error) { if (error instanceof McpDomainNotAllowedError || error instanceof McpSsrfError) { - return { success: false, error: error.message, errorCode: 'forbidden' } + return { ok: false, result: { success: false, error: error.message, errorCode: 'forbidden' } } } if (error instanceof McpDnsResolutionError) { - return { success: false, error: error.message, errorCode: 'bad_gateway' } + return { + ok: false, + result: { success: false, error: error.message, errorCode: 'bad_gateway' }, + } } throw error } @@ -106,7 +113,8 @@ export async function performCreateMcpServer( params: PerformCreateMcpServerParams ): Promise { const validation = await validateMcpServerUrl(params.url) - if (validation) return validation + if (!validation.ok) return validation.result + const validatedIP = validation.resolvedIP const transport = params.transport || 'streamable-http' const timeout = params.timeout || 30000 @@ -142,7 +150,7 @@ export async function performCreateMcpServer( resolvedAuthType = (existingServer.authType ?? 'headers') as McpAuthType } else if (params.url && !hasHeaders) { try { - resolvedAuthType = await detectMcpAuthType(params.url) + resolvedAuthType = await detectMcpAuthType(params.url, validatedIP) } catch (e) { logger.warn('Probe failed, defaulting to headers', { url: params.url, error: e }) resolvedAuthType = 'headers' @@ -281,7 +289,7 @@ export async function performUpdateMcpServer( ): Promise { if (params.url) { const validation = await validateMcpServerUrl(params.url) - if (validation) return validation + if (!validation.ok) return validation.result } const oauthClientSecretEncrypted = From e64b70304b8a9b07da2e6f4a9ea475c0defe4c4b Mon Sep 17 00:00:00 2001 From: waleed Date: Sat, 6 Jun 2026 12:07:42 -0700 Subject: [PATCH 03/10] fix(security): stop returning plaintext OAuth access tokens from copilot credentials GET /api/copilot/credentials returned each connected account's live, post-refresh OAuth access token in plaintext to any session for that user. The endpoint is only used for credential display/masking and no client reads the token, so drop accessToken from the get_credentials tool output and the copilot credentials response contract. Also removes the incidental refreshTokenIfNeeded side-effect on this read path. Adds regression tests: - get-credentials: asserts the response exposes only masked metadata and never leaks the access/refresh token. - revoke: locks in that revokeMcpOauthTokens routes OAuth discovery and RFC 7009 revocation through the SSRF-guarded fetch (no raw fetch to an attacker-controlled revocation_endpoint). --- apps/sim/lib/api/contracts/copilot.ts | 1 - .../tools/server/user/get-credentials.test.ts | 134 +++++++++++++++ .../tools/server/user/get-credentials.ts | 18 -- apps/sim/lib/mcp/oauth/revoke.test.ts | 161 ++++++++++++++++++ 4 files changed, 295 insertions(+), 19 deletions(-) create mode 100644 apps/sim/lib/copilot/tools/server/user/get-credentials.test.ts create mode 100644 apps/sim/lib/mcp/oauth/revoke.test.ts diff --git a/apps/sim/lib/api/contracts/copilot.ts b/apps/sim/lib/api/contracts/copilot.ts index 41bffb5f3b..3970246cff 100644 --- a/apps/sim/lib/api/contracts/copilot.ts +++ b/apps/sim/lib/api/contracts/copilot.ts @@ -430,7 +430,6 @@ const copilotConnectedCredentialSchema = z.object({ serviceName: z.string(), lastUsed: z.string(), isDefault: z.boolean(), - accessToken: z.string().nullable(), }) const copilotNotConnectedServiceSchema = z.object({ diff --git a/apps/sim/lib/copilot/tools/server/user/get-credentials.test.ts b/apps/sim/lib/copilot/tools/server/user/get-credentials.test.ts new file mode 100644 index 0000000000..6b8cdd130e --- /dev/null +++ b/apps/sim/lib/copilot/tools/server/user/get-credentials.test.ts @@ -0,0 +1,134 @@ +/** + * @vitest-environment node + * + * Regression test: the credentials response must expose only display metadata, + * never the connected account's OAuth access/refresh token. + */ + +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const SECRET_ACCESS_TOKEN = 'ya29.a0SECRET_GOOGLE_BEARER_TOKEN_DO_NOT_LEAK' + +const { selectMock, getAllOAuthServicesMock, getPersonalAndWorkspaceEnvMock, jwtDecodeMock } = + vi.hoisted(() => ({ + selectMock: vi.fn(), + getAllOAuthServicesMock: vi.fn(), + getPersonalAndWorkspaceEnvMock: vi.fn(), + jwtDecodeMock: vi.fn(), + })) + +vi.mock('@sim/db', () => ({ + db: { select: selectMock }, +})) + +vi.mock('@/lib/oauth', () => ({ + getAllOAuthServices: getAllOAuthServicesMock, +})) + +vi.mock('@/lib/environment/utils', () => ({ + getPersonalAndWorkspaceEnv: getPersonalAndWorkspaceEnvMock, +})) + +vi.mock('jwt-decode', () => ({ + jwtDecode: jwtDecodeMock, +})) + +import { getCredentialsServerTool } from './get-credentials' + +/** + * Wires the two sequential `db.select()` reads the tool performs: + * 1. `select().from(account).where()` → account rows (awaited directly) + * 2. `select({...}).from(user).where().limit(1)` → user row + */ +function wireDb(accountRows: unknown[], userRows: Array<{ email: string }>) { + const whereThenable = { + then: (resolve: (rows: unknown[]) => unknown) => resolve(accountRows), + limit: () => Promise.resolve(userRows), + } + const builder = { from: () => builder, where: () => whereThenable } + selectMock.mockReturnValue(builder) +} + +describe('getCredentialsServerTool', () => { + beforeEach(() => { + vi.clearAllMocks() + + wireDb( + [ + { + id: 'acct-google-1', + providerId: 'google-default', + accountId: '1234567890', + idToken: 'jwt-token', + accessToken: SECRET_ACCESS_TOKEN, + refreshToken: 'refresh-secret', + updatedAt: new Date('2026-04-17T02:26:05.546Z'), + }, + ], + [{ email: 'brent@cellular.so' }] + ) + + getAllOAuthServicesMock.mockReturnValue([ + { + providerId: 'google-default', + name: 'Google', + description: 'Google account', + baseProvider: 'google', + }, + { + providerId: 'slack', + name: 'Slack', + description: 'Slack workspace', + baseProvider: 'slack', + }, + ]) + + getPersonalAndWorkspaceEnvMock.mockResolvedValue({ + personalEncrypted: {}, + workspaceEncrypted: {}, + conflicts: [], + }) + + jwtDecodeMock.mockReturnValue({ email: 'brent@cellular.so' }) + }) + + it('never returns access tokens for connected OAuth credentials', async () => { + const result = await getCredentialsServerTool.execute({}, { userId: 'user-1' }) + + const credentials = result.oauth.connected.credentials + expect(credentials).toHaveLength(1) + + for (const credential of credentials) { + expect(credential).not.toHaveProperty('accessToken') + expect(credential).not.toHaveProperty('refreshToken') + expect(credential).not.toHaveProperty('idToken') + } + }) + + it('returns only masked display metadata for each credential', async () => { + const result = await getCredentialsServerTool.execute({}, { userId: 'user-1' }) + + expect(result.oauth.connected.credentials[0]).toEqual({ + id: 'acct-google-1', + name: 'brent@cellular.so', + provider: 'google-default', + serviceName: 'Google', + lastUsed: '2026-04-17T02:26:05.546Z', + isDefault: true, + }) + }) + + it('does not leak the token value anywhere in the serialized response', async () => { + const result = await getCredentialsServerTool.execute({}, { userId: 'user-1' }) + + expect(JSON.stringify(result)).not.toContain(SECRET_ACCESS_TOKEN) + expect(JSON.stringify(result)).not.toContain('refresh-secret') + }) + + it('rejects unauthenticated callers without touching the database', async () => { + await expect(getCredentialsServerTool.execute({}, undefined)).rejects.toThrow( + 'Authentication required' + ) + expect(selectMock).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/lib/copilot/tools/server/user/get-credentials.ts b/apps/sim/lib/copilot/tools/server/user/get-credentials.ts index f856bb62c2..c8088ee71a 100644 --- a/apps/sim/lib/copilot/tools/server/user/get-credentials.ts +++ b/apps/sim/lib/copilot/tools/server/user/get-credentials.ts @@ -6,10 +6,8 @@ import { eq } from 'drizzle-orm' import { jwtDecode } from 'jwt-decode' import { createPermissionError, verifyWorkflowAccess } from '@/lib/copilot/auth/permissions' import type { BaseServerTool } from '@/lib/copilot/tools/server/base-tool' -import { generateRequestId } from '@/lib/core/utils/request' import { getPersonalAndWorkspaceEnv } from '@/lib/environment/utils' import { getAllOAuthServices } from '@/lib/oauth' -import { refreshTokenIfNeeded } from '@/app/api/auth/oauth/utils' interface GetCredentialsParams { workflowId?: string @@ -76,9 +74,7 @@ export const getCredentialsServerTool: BaseServerTool serviceName: string lastUsed: string isDefault: boolean - accessToken: string | null }> = [] - const requestId = generateRequestId() for (const acc of accounts) { const providerId = acc.providerId @@ -104,19 +100,6 @@ export const getCredentialsServerTool: BaseServerTool const service = allOAuthServices.find((s) => s.providerId === providerId) const serviceName = service?.name ?? providerId - let accessToken: string | null = acc.accessToken ?? null - try { - const { accessToken: refreshedToken } = await refreshTokenIfNeeded( - requestId, - acc as any, - acc.id - ) - accessToken = refreshedToken || accessToken - } catch (error) { - logger.warn('Failed to refresh OAuth access token', { - error: toError(error).message, - }) - } connectedCredentials.push({ id: acc.id, name: displayName, @@ -124,7 +107,6 @@ export const getCredentialsServerTool: BaseServerTool serviceName, lastUsed: acc.updatedAt.toISOString(), isDefault: featureType === 'default', - accessToken, }) } diff --git a/apps/sim/lib/mcp/oauth/revoke.test.ts b/apps/sim/lib/mcp/oauth/revoke.test.ts new file mode 100644 index 0000000000..d8f6342568 --- /dev/null +++ b/apps/sim/lib/mcp/oauth/revoke.test.ts @@ -0,0 +1,161 @@ +/** + * @vitest-environment node + * + * Regression test: `revokeMcpOauthTokens` must route both metadata discovery + * and the RFC 7009 revocation POST through the SSRF-guarded fetch, since + * `revocation_endpoint` comes from attacker-controlled server metadata. Uses + * the real `createSsrfGuardedMcpFetch` so it fails if revoke.ts regresses to a + * raw `fetch`. + */ + +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const BLOCKED_ENDPOINT = 'http://169.254.170.2/v2/credentials/' +const PUBLIC_SERVER_URL = 'https://mcp.attacker.com' +const PUBLIC_SERVER_IP = '203.0.113.10' + +const { + MockAgent, + mockUndiciFetch, + mockValidateMcpServerSsrf, + mockDiscoverOAuthServerInfo, + mockLoadOauthRow, + mockDecryptSecret, + mockDbSelect, +} = vi.hoisted(() => { + class MockAgent { + close() { + return Promise.resolve() + } + } + return { + MockAgent, + mockUndiciFetch: vi.fn(), + mockValidateMcpServerSsrf: vi.fn(), + mockDiscoverOAuthServerInfo: vi.fn(), + mockLoadOauthRow: vi.fn(), + mockDecryptSecret: vi.fn(), + mockDbSelect: vi.fn(), + } +}) + +vi.mock('undici', () => ({ Agent: MockAgent, fetch: mockUndiciFetch })) +vi.mock('@/lib/core/security/input-validation.server', () => ({ + createPinnedLookup: vi.fn(() => 'pinned-lookup-fn'), +})) +vi.mock('@/lib/mcp/domain-check', () => ({ + validateMcpServerSsrf: mockValidateMcpServerSsrf, +})) +vi.mock('@modelcontextprotocol/sdk/client/auth.js', () => ({ + discoverOAuthServerInfo: mockDiscoverOAuthServerInfo, +})) +vi.mock('@/lib/mcp/oauth/storage', () => ({ + loadOauthRow: mockLoadOauthRow, +})) +vi.mock('@/lib/core/security/encryption', () => ({ + decryptSecret: mockDecryptSecret, +})) +vi.mock('@sim/db', () => ({ + db: { select: mockDbSelect }, +})) + +import { __resetPinnedAgentsForTests } from '@/lib/mcp/pinned-fetch' +import { revokeMcpOauthTokens } from './revoke' + +function wireServerRow(row: Record) { + const builder = { + from: () => builder, + where: () => builder, + limit: () => Promise.resolve([row]), + } + mockDbSelect.mockReturnValue(builder) +} + +describe('revokeMcpOauthTokens — SSRF guard', () => { + beforeEach(() => { + vi.clearAllMocks() + __resetPinnedAgentsForTests() + + mockLoadOauthRow.mockResolvedValue({ + tokens: { access_token: 'access-secret', refresh_token: 'refresh-secret' }, + clientInformation: { client_id: 'client-123' }, + }) + + wireServerRow({ + url: PUBLIC_SERVER_URL, + oauthClientId: 'client-123', + oauthClientSecret: null, + }) + + mockDiscoverOAuthServerInfo.mockResolvedValue({ + authorizationServerMetadata: { + issuer: PUBLIC_SERVER_URL, + revocation_endpoint: BLOCKED_ENDPOINT, + }, + }) + + mockUndiciFetch.mockResolvedValue(new Response('ok')) + + // Catches a regression to raw globalThis.fetch without hitting the network. + vi.spyOn(globalThis, 'fetch').mockResolvedValue(new Response('ok')) + + // Public server host resolves; the revocation endpoint is blocked. + mockValidateMcpServerSsrf.mockImplementation(async (target: string) => { + if (target.startsWith(BLOCKED_ENDPOINT) || target.includes('169.254.')) { + throw new Error('MCP server URL resolves to a blocked IP address') + } + return PUBLIC_SERVER_IP + }) + }) + + it('routes metadata discovery through the SSRF-guarded fetch', async () => { + await revokeMcpOauthTokens('server-1') + + expect(mockDiscoverOAuthServerInfo).toHaveBeenCalledTimes(1) + const [, options] = mockDiscoverOAuthServerInfo.mock.calls[0] + expect(typeof options?.fetchFn).toBe('function') + }) + + it('validates the attacker-controlled revocation_endpoint before issuing the request', async () => { + await revokeMcpOauthTokens('server-1') + + expect(mockValidateMcpServerSsrf).toHaveBeenCalledWith(BLOCKED_ENDPOINT) + }) + + it('never issues an outbound request to the blocked revocation endpoint', async () => { + await revokeMcpOauthTokens('server-1') + + const allCalls = [ + ...mockUndiciFetch.mock.calls, + ...(globalThis.fetch as ReturnType).mock.calls, + ] + for (const call of allCalls) { + const target = typeof call[0] === 'string' ? call[0] : String(call[0]) + expect(target).not.toContain('169.254.170.2') + } + }) + + it('swallows the SSRF rejection — revocation is best-effort and never throws', async () => { + await expect(revokeMcpOauthTokens('server-1')).resolves.toBeUndefined() + }) + + it('still issues the revocation POST when the endpoint resolves to a public IP', async () => { + const publicEndpoint = 'https://mcp.attacker.com/oauth/revoke' + mockDiscoverOAuthServerInfo.mockResolvedValue({ + authorizationServerMetadata: { + issuer: PUBLIC_SERVER_URL, + revocation_endpoint: publicEndpoint, + }, + }) + + await revokeMcpOauthTokens('server-1') + + expect(mockValidateMcpServerSsrf).toHaveBeenCalledWith(publicEndpoint) + const revokeCalls = mockUndiciFetch.mock.calls.filter((call) => { + const target = typeof call[0] === 'string' ? call[0] : String(call[0]) + return target === publicEndpoint + }) + expect(revokeCalls.length).toBeGreaterThan(0) + expect(revokeCalls[0][1]).toMatchObject({ method: 'POST' }) + }) +}) From 5e067b29ba3be7b32621301a679a0ee08614c798 Mon Sep 17 00:00:00 2001 From: waleed Date: Sat, 6 Jun 2026 12:07:47 -0700 Subject: [PATCH 04/10] fix(webhooks): verify X-Twilio-Signature on Twilio SMS webhooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The twilio (SMS) provider handler implemented no verifyAuth, so the webhook dispatcher queued workflow executions for any request to a known SMS trigger path without validating the Twilio signature — allowing forged inbound SMS events. Only the twilio-voice handler performed signature verification. Extract the shared HMAC-SHA1 signature validation into twilio-signature.ts and wire it into both the SMS and Voice handlers. Verification is enforced when an auth token is configured (parity with Voice); requests without a configured token pass through per the provider-wide optional-secret convention. Add regression tests for both handlers. --- .../webhooks/providers/twilio-signature.ts | 118 +++++++++++++++ .../webhooks/providers/twilio-voice.test.ts | 85 +++++++++++ .../lib/webhooks/providers/twilio-voice.ts | 117 +-------------- .../sim/lib/webhooks/providers/twilio.test.ts | 136 ++++++++++++++++++ apps/sim/lib/webhooks/providers/twilio.ts | 7 +- 5 files changed, 348 insertions(+), 115 deletions(-) create mode 100644 apps/sim/lib/webhooks/providers/twilio-signature.ts create mode 100644 apps/sim/lib/webhooks/providers/twilio-voice.test.ts create mode 100644 apps/sim/lib/webhooks/providers/twilio.test.ts diff --git a/apps/sim/lib/webhooks/providers/twilio-signature.ts b/apps/sim/lib/webhooks/providers/twilio-signature.ts new file mode 100644 index 0000000000..acd7d6b8b5 --- /dev/null +++ b/apps/sim/lib/webhooks/providers/twilio-signature.ts @@ -0,0 +1,118 @@ +import crypto from 'crypto' +import { createLogger } from '@sim/logger' +import { safeCompare } from '@sim/security/compare' +import { NextResponse } from 'next/server' +import type { AuthContext } from '@/lib/webhooks/providers/types' + +const logger = createLogger('WebhookProvider:TwilioSignature') + +/** + * Validate `X-Twilio-Signature`: HMAC-SHA1 over the callback URL plus each POST + * param key/value sorted alphabetically. + * @see https://www.twilio.com/docs/usage/security#validating-requests + */ +async function validateTwilioSignature( + authToken: string, + signature: string, + url: string, + params: Record +): Promise { + try { + if (!authToken || !signature || !url) { + logger.warn('Twilio signature validation missing required fields', { + hasAuthToken: !!authToken, + hasSignature: !!signature, + hasUrl: !!url, + }) + return false + } + const sortedKeys = Object.keys(params).sort() + let data = url + for (const key of sortedKeys) { + data += key + params[key] + } + const encoder = new TextEncoder() + const key = await crypto.subtle.importKey( + 'raw', + encoder.encode(authToken), + { name: 'HMAC', hash: 'SHA-1' }, + false, + ['sign'] + ) + const signatureBytes = await crypto.subtle.sign('HMAC', key, encoder.encode(data)) + const signatureArray = Array.from(new Uint8Array(signatureBytes)) + const signatureBase64 = btoa(String.fromCharCode(...signatureArray)) + return safeCompare(signatureBase64, signature) + } catch (error) { + logger.error('Error validating Twilio signature:', error) + return false + } +} + +/** + * Reconstruct the public callback URL Twilio signed, recovering the original + * host/proto from forwarding headers when Sim runs behind a proxy. Forged headers + * don't help an attacker: without the auth token they can't match the signature. + */ +function getExternalUrl(request: Request): string { + const proto = request.headers.get('x-forwarded-proto') || 'https' + const host = request.headers.get('x-forwarded-host') || request.headers.get('host') + + if (host) { + const url = new URL(request.url) + return `${proto}://${host}${url.pathname}${url.search}` + } + + return request.url +} + +/** + * Shared `verifyAuth` for Twilio webhook providers (SMS and Voice). Enforces a + * valid `X-Twilio-Signature` when an auth token is configured; skips verification + * when none is set (the provider-wide "optional secret" convention). + */ +export async function verifyTwilioAuth( + { request, rawBody, requestId, providerConfig }: AuthContext, + providerLabel: string +): Promise { + const authToken = providerConfig.authToken as string | undefined + + if (!authToken) { + return null + } + + const signature = request.headers.get('x-twilio-signature') + if (!signature) { + logger.warn(`[${requestId}] ${providerLabel} webhook missing signature header`) + return new NextResponse('Unauthorized - Missing Twilio signature', { status: 401 }) + } + + let params: Record = {} + try { + if (typeof rawBody === 'string') { + const urlParams = new URLSearchParams(rawBody) + params = Object.fromEntries(urlParams.entries()) + } + } catch (error) { + logger.error( + `[${requestId}] Error parsing ${providerLabel} webhook body for signature validation:`, + error + ) + return new NextResponse('Bad Request - Invalid body format', { status: 400 }) + } + + const fullUrl = getExternalUrl(request) + const isValidSignature = await validateTwilioSignature(authToken, signature, fullUrl, params) + + if (!isValidSignature) { + logger.warn(`[${requestId}] ${providerLabel} signature verification failed`, { + url: fullUrl, + signatureLength: signature.length, + paramsCount: Object.keys(params).length, + authTokenLength: authToken.length, + }) + return new NextResponse('Unauthorized - Invalid Twilio signature', { status: 401 }) + } + + return null +} diff --git a/apps/sim/lib/webhooks/providers/twilio-voice.test.ts b/apps/sim/lib/webhooks/providers/twilio-voice.test.ts new file mode 100644 index 0000000000..f01d133398 --- /dev/null +++ b/apps/sim/lib/webhooks/providers/twilio-voice.test.ts @@ -0,0 +1,85 @@ +/** + * @vitest-environment node + */ +import crypto from 'crypto' +import { createMockRequest } from '@sim/testing' +import { describe, expect, it } from 'vitest' +import { twilioVoiceHandler } from '@/lib/webhooks/providers/twilio-voice' + +/** Twilio canonical signature: HMAC-SHA1(authToken, url + sorted(key+value)) base64. */ +function signTwilio(authToken: string, url: string, params: Record): string { + const data = Object.keys(params) + .sort() + .reduce((acc, key) => acc + key + params[key], url) + return crypto.createHmac('sha1', authToken).update(Buffer.from(data, 'utf8')).digest('base64') +} + +describe('twilioVoiceHandler', () => { + describe('verifyAuth', () => { + const authToken = 'voice-auth-token' + const url = 'http://localhost:3000/api/test' + const params = { CallSid: 'CA123', From: '+15551234567', To: '+15557654321' } + const rawBody = new URLSearchParams(params).toString() + const signature = signTwilio(authToken, url, params) + + it('skips verification when no auth token is configured', async () => { + const request = createMockRequest('POST', undefined, {}) + const res = await twilioVoiceHandler.verifyAuth!({ + request: request as any, + rawBody, + requestId: 'r1', + providerConfig: {}, + webhook: {}, + workflow: {}, + }) + expect(res).toBeNull() + }) + + it('returns 401 when the signature header is missing', async () => { + const request = createMockRequest('POST', undefined, {}) + const res = await twilioVoiceHandler.verifyAuth!({ + request: request as any, + rawBody, + requestId: 'r1', + providerConfig: { authToken }, + webhook: {}, + workflow: {}, + }) + expect(res?.status).toBe(401) + }) + + it('returns 401 when the signature is invalid', async () => { + const request = createMockRequest('POST', undefined, { 'x-twilio-signature': 'bad' }) + const res = await twilioVoiceHandler.verifyAuth!({ + request: request as any, + rawBody, + requestId: 'r1', + providerConfig: { authToken }, + webhook: {}, + workflow: {}, + }) + expect(res?.status).toBe(401) + }) + + it('returns null when the signature is valid', async () => { + const request = createMockRequest('POST', undefined, { 'x-twilio-signature': signature }) + const res = await twilioVoiceHandler.verifyAuth!({ + request: request as any, + rawBody, + requestId: 'r1', + providerConfig: { authToken }, + webhook: {}, + workflow: {}, + }) + expect(res).toBeNull() + }) + }) + + describe('extractIdempotencyId', () => { + it('prefers MessageSid, falls back to CallSid', () => { + expect(twilioVoiceHandler.extractIdempotencyId!({ MessageSid: 'SM1' })).toBe('SM1') + expect(twilioVoiceHandler.extractIdempotencyId!({ CallSid: 'CA1' })).toBe('CA1') + expect(twilioVoiceHandler.extractIdempotencyId!({})).toBeNull() + }) + }) +}) diff --git a/apps/sim/lib/webhooks/providers/twilio-voice.ts b/apps/sim/lib/webhooks/providers/twilio-voice.ts index 543264cd73..c00208307b 100644 --- a/apps/sim/lib/webhooks/providers/twilio-voice.ts +++ b/apps/sim/lib/webhooks/providers/twilio-voice.ts @@ -1,7 +1,5 @@ -import crypto from 'crypto' -import { createLogger } from '@sim/logger' -import { safeCompare } from '@sim/security/compare' import { NextResponse } from 'next/server' +import { verifyTwilioAuth } from '@/lib/webhooks/providers/twilio-signature' import type { AuthContext, FormatInputContext, @@ -10,118 +8,9 @@ import type { } from '@/lib/webhooks/providers/types' import { convertSquareBracketsToTwiML } from '@/lib/webhooks/utils' -const logger = createLogger('WebhookProvider:TwilioVoice') - -async function validateTwilioSignature( - authToken: string, - signature: string, - url: string, - params: Record -): Promise { - try { - if (!authToken || !signature || !url) { - logger.warn('Twilio signature validation missing required fields', { - hasAuthToken: !!authToken, - hasSignature: !!signature, - hasUrl: !!url, - }) - return false - } - const sortedKeys = Object.keys(params).sort() - let data = url - for (const key of sortedKeys) { - data += key + params[key] - } - logger.debug('Twilio signature validation string built', { - url, - sortedKeys, - dataLength: data.length, - }) - const encoder = new TextEncoder() - const key = await crypto.subtle.importKey( - 'raw', - encoder.encode(authToken), - { name: 'HMAC', hash: 'SHA-1' }, - false, - ['sign'] - ) - const signatureBytes = await crypto.subtle.sign('HMAC', key, encoder.encode(data)) - const signatureArray = Array.from(new Uint8Array(signatureBytes)) - const signatureBase64 = btoa(String.fromCharCode(...signatureArray)) - logger.debug('Twilio signature comparison', { - computedSignature: `${signatureBase64.substring(0, 10)}...`, - providedSignature: `${signature.substring(0, 10)}...`, - computedLength: signatureBase64.length, - providedLength: signature.length, - match: signatureBase64 === signature, - }) - return safeCompare(signatureBase64, signature) - } catch (error) { - logger.error('Error validating Twilio signature:', error) - return false - } -} - -function getExternalUrl(request: Request): string { - const proto = request.headers.get('x-forwarded-proto') || 'https' - const host = request.headers.get('x-forwarded-host') || request.headers.get('host') - - if (host) { - const url = new URL(request.url) - const reconstructed = `${proto}://${host}${url.pathname}${url.search}` - return reconstructed - } - - return request.url -} - export const twilioVoiceHandler: WebhookProviderHandler = { - async verifyAuth({ request, rawBody, requestId, providerConfig }: AuthContext) { - const authToken = providerConfig.authToken as string | undefined - - if (authToken) { - const signature = request.headers.get('x-twilio-signature') - - if (!signature) { - logger.warn(`[${requestId}] Twilio Voice webhook missing signature header`) - return new NextResponse('Unauthorized - Missing Twilio signature', { - status: 401, - }) - } - - let params: Record = {} - try { - if (typeof rawBody === 'string') { - const urlParams = new URLSearchParams(rawBody) - params = Object.fromEntries(urlParams.entries()) - } - } catch (error) { - logger.error( - `[${requestId}] Error parsing Twilio webhook body for signature validation:`, - error - ) - return new NextResponse('Bad Request - Invalid body format', { - status: 400, - }) - } - - const fullUrl = getExternalUrl(request) - const isValidSignature = await validateTwilioSignature(authToken, signature, fullUrl, params) - - if (!isValidSignature) { - logger.warn(`[${requestId}] Twilio Voice signature verification failed`, { - url: fullUrl, - signatureLength: signature.length, - paramsCount: Object.keys(params).length, - authTokenLength: authToken.length, - }) - return new NextResponse('Unauthorized - Invalid Twilio signature', { - status: 401, - }) - } - } - - return null + verifyAuth(ctx: AuthContext) { + return verifyTwilioAuth(ctx, 'Twilio Voice') }, extractIdempotencyId(body: unknown) { diff --git a/apps/sim/lib/webhooks/providers/twilio.test.ts b/apps/sim/lib/webhooks/providers/twilio.test.ts new file mode 100644 index 0000000000..c72fad5d8a --- /dev/null +++ b/apps/sim/lib/webhooks/providers/twilio.test.ts @@ -0,0 +1,136 @@ +/** + * @vitest-environment node + */ +import crypto from 'crypto' +import { createMockRequest } from '@sim/testing' +import { describe, expect, it } from 'vitest' +import { twilioHandler } from '@/lib/webhooks/providers/twilio' + +/** Twilio canonical signature: HMAC-SHA1(authToken, url + sorted(key+value)) base64. */ +function signTwilio(authToken: string, url: string, params: Record): string { + const data = Object.keys(params) + .sort() + .reduce((acc, key) => acc + key + params[key], url) + return crypto.createHmac('sha1', authToken).update(Buffer.from(data, 'utf8')).digest('base64') +} + +describe('twilioHandler', () => { + describe('verifyAuth', () => { + const authToken = 'test-auth-token' + const url = 'http://localhost:3000/api/test' + const params = { From: '+15551234567', To: '+15557654321', Body: 'hello', MessageSid: 'SM123' } + const rawBody = new URLSearchParams(params).toString() + const signature = signTwilio(authToken, url, params) + + it('rejects a forged request with no signature header', async () => { + const request = createMockRequest('POST', undefined, { + 'content-type': 'application/x-www-form-urlencoded', + }) + const res = await twilioHandler.verifyAuth!({ + request: request as any, + rawBody, + requestId: 'r1', + providerConfig: { authToken }, + webhook: {}, + workflow: {}, + }) + expect(res?.status).toBe(401) + }) + + it('rejects a request with an invalid signature', async () => { + const request = createMockRequest('POST', undefined, { + 'x-twilio-signature': 'not-the-real-signature', + }) + const res = await twilioHandler.verifyAuth!({ + request: request as any, + rawBody, + requestId: 'r1', + providerConfig: { authToken }, + webhook: {}, + workflow: {}, + }) + expect(res?.status).toBe(401) + }) + + it('accepts a request with a valid signature', async () => { + const request = createMockRequest('POST', undefined, { + 'x-twilio-signature': signature, + }) + const res = await twilioHandler.verifyAuth!({ + request: request as any, + rawBody, + requestId: 'r1', + providerConfig: { authToken }, + webhook: {}, + workflow: {}, + }) + expect(res).toBeNull() + }) + + it('skips verification when no auth token is configured (optional-secret convention)', async () => { + const request = createMockRequest('POST', undefined, {}) + const res = await twilioHandler.verifyAuth!({ + request: request as any, + rawBody, + requestId: 'r1', + providerConfig: {}, + webhook: {}, + workflow: {}, + }) + expect(res).toBeNull() + }) + + it('reconstructs the public URL from forwarding headers when validating', async () => { + const publicUrl = 'https://sim.ai/api/webhooks/trigger/twilio-sms-abc123' + const fwdSignature = signTwilio(authToken, publicUrl, params) + const request = createMockRequest( + 'POST', + undefined, + { + 'x-twilio-signature': fwdSignature, + 'x-forwarded-proto': 'https', + 'x-forwarded-host': 'sim.ai', + }, + 'http://internal-host:3000/api/webhooks/trigger/twilio-sms-abc123' + ) + const res = await twilioHandler.verifyAuth!({ + request: request as any, + rawBody, + requestId: 'r1', + providerConfig: { authToken }, + webhook: {}, + workflow: {}, + }) + expect(res).toBeNull() + }) + + it('rejects a forged body even with a forwarded host (no valid token)', async () => { + const request = createMockRequest( + 'POST', + undefined, + { + 'x-twilio-signature': signTwilio('attacker-guess', url, params), + 'x-forwarded-host': 'sim.ai', + }, + url + ) + const res = await twilioHandler.verifyAuth!({ + request: request as any, + rawBody, + requestId: 'r1', + providerConfig: { authToken }, + webhook: {}, + workflow: {}, + }) + expect(res?.status).toBe(401) + }) + }) + + describe('extractIdempotencyId', () => { + it('prefers MessageSid, falls back to CallSid', () => { + expect(twilioHandler.extractIdempotencyId!({ MessageSid: 'SM1' })).toBe('SM1') + expect(twilioHandler.extractIdempotencyId!({ CallSid: 'CA1' })).toBe('CA1') + expect(twilioHandler.extractIdempotencyId!({})).toBeNull() + }) + }) +}) diff --git a/apps/sim/lib/webhooks/providers/twilio.ts b/apps/sim/lib/webhooks/providers/twilio.ts index 3ba33decc1..15736b6374 100644 --- a/apps/sim/lib/webhooks/providers/twilio.ts +++ b/apps/sim/lib/webhooks/providers/twilio.ts @@ -1,6 +1,11 @@ -import type { WebhookProviderHandler } from '@/lib/webhooks/providers/types' +import { verifyTwilioAuth } from '@/lib/webhooks/providers/twilio-signature' +import type { AuthContext, WebhookProviderHandler } from '@/lib/webhooks/providers/types' export const twilioHandler: WebhookProviderHandler = { + verifyAuth(ctx: AuthContext) { + return verifyTwilioAuth(ctx, 'Twilio SMS') + }, + extractIdempotencyId(body: unknown) { const obj = body as Record return (obj.MessageSid as string) || (obj.CallSid as string) || null From 715e67204509e4261928aa1fbfaa6f9c69bd5491 Mon Sep 17 00:00:00 2001 From: waleed Date: Sat, 6 Jun 2026 12:08:04 -0700 Subject: [PATCH 05/10] fix(connectors): route user-controlled connector hosts through DNS-validated, IP-pinned fetch Knowledge connectors that accept a custom service host/endpoint (S3-compatible endpoints, self-managed GitLab/Sentry hosts, Obsidian vault URLs) performed server-side fetches without the repository's SSRF guard, letting an authenticated user with KB write access probe internal/loopback hosts from the backend. Add secureFetchWithRetry (validateUrlWithDNS + secureFetchWithPinnedIP + the same retry/backoff as fetchWithRetry) and route every request in the s3, gitlab, sentry, and obsidian connectors through it - including pagination and hydration. Gate the S3 plain-http loopback exception to self-hosted deployments. --- apps/sim/connectors/gitlab/gitlab.ts | 21 +-- apps/sim/connectors/obsidian/obsidian.ts | 33 +++-- apps/sim/connectors/s3/s3.ts | 23 ++-- apps/sim/connectors/sentry/sentry.ts | 12 +- apps/sim/connectors/utils.ts | 4 +- .../sim/lib/knowledge/documents/utils.test.ts | 123 +++++++++++++++++- apps/sim/lib/knowledge/documents/utils.ts | 61 +++++++++ 7 files changed, 230 insertions(+), 47 deletions(-) diff --git a/apps/sim/connectors/gitlab/gitlab.ts b/apps/sim/connectors/gitlab/gitlab.ts index b41303bdf2..78acaebc6f 100644 --- a/apps/sim/connectors/gitlab/gitlab.ts +++ b/apps/sim/connectors/gitlab/gitlab.ts @@ -1,8 +1,9 @@ import { createLogger } from '@sim/logger' import { getErrorMessage, toError } from '@sim/utils/errors' import { GitLabIcon } from '@/components/icons' +import type { SecureFetchResponse } from '@/lib/core/security/input-validation.server' import { isSameOrigin } from '@/lib/core/utils/validation' -import { fetchWithRetry, VALIDATE_RETRY_OPTIONS } from '@/lib/knowledge/documents/utils' +import { secureFetchWithRetry, VALIDATE_RETRY_OPTIONS } from '@/lib/knowledge/documents/utils' import type { ConnectorConfig, ExternalDocument, ExternalDocumentList } from '@/connectors/types' import { computeContentHash, joinTagArray, parseTagDate } from '@/connectors/utils' @@ -461,8 +462,8 @@ async function fetchProject( encodedProject: string, accessToken: string, retryOptions?: typeof VALIDATE_RETRY_OPTIONS -): Promise { - return fetchWithRetry( +): Promise { + return secureFetchWithRetry( `${apiBase}/projects/${encodedProject}`, { method: 'GET', headers: authHeaders(accessToken) }, retryOptions @@ -760,7 +761,7 @@ export const gitlabConnector: ConnectorConfig = { continued: Boolean(state.fileNextUrl), }) - const response = await fetchWithRetry(url, { + const response = await secureFetchWithRetry(url, { method: 'GET', headers: authHeaders(accessToken), }) @@ -816,7 +817,7 @@ export const gitlabConnector: ConnectorConfig = { const url = `${apiBase}/projects/${encodedProject}/wikis?with_content=1` logger.info('Listing GitLab wiki pages', { host, project: encodedProject }) - const response = await fetchWithRetry(url, { + const response = await secureFetchWithRetry(url, { method: 'GET', headers: authHeaders(accessToken), }) @@ -891,7 +892,7 @@ export const gitlabConnector: ConnectorConfig = { incremental: Boolean(lastSyncAt), }) - const response = await fetchWithRetry(url, { + const response = await secureFetchWithRetry(url, { method: 'GET', headers: authHeaders(accessToken), }) @@ -954,7 +955,7 @@ export const gitlabConnector: ConnectorConfig = { if (!slug) return null const url = `${apiBase}/projects/${encodedProject}/wikis/${encodeURIComponent(slug)}?render_html=false` - const response = await fetchWithRetry(url, { + const response = await secureFetchWithRetry(url, { method: 'GET', headers: authHeaders(accessToken), }) @@ -975,7 +976,7 @@ export const gitlabConnector: ConnectorConfig = { if (!iidStr || Number.isNaN(iid)) return null const url = `${apiBase}/projects/${encodedProject}/issues/${iid}` - const response = await fetchWithRetry(url, { + const response = await secureFetchWithRetry(url, { method: 'GET', headers: authHeaders(accessToken), }) @@ -1002,7 +1003,7 @@ export const gitlabConnector: ConnectorConfig = { accessToken ) const url = `${apiBase}/projects/${encodedProject}/repository/files/${encodeURIComponent(path)}?ref=${encodeURIComponent(ref)}` - const response = await fetchWithRetry(url, { + const response = await secureFetchWithRetry(url, { method: 'GET', headers: authHeaders(accessToken), }) @@ -1078,7 +1079,7 @@ export const gitlabConnector: ConnectorConfig = { const userRef = typeof sourceConfig.ref === 'string' ? sourceConfig.ref.trim() : '' if (userRef && activePhases(choice).includes('repo')) { - const refResponse = await fetchWithRetry( + const refResponse = await secureFetchWithRetry( `${apiBase}/projects/${encodedProject}/repository/commits/${encodeURIComponent(userRef)}`, { method: 'GET', headers: authHeaders(accessToken) }, VALIDATE_RETRY_OPTIONS diff --git a/apps/sim/connectors/obsidian/obsidian.ts b/apps/sim/connectors/obsidian/obsidian.ts index 964aa33b57..1169a54206 100644 --- a/apps/sim/connectors/obsidian/obsidian.ts +++ b/apps/sim/connectors/obsidian/obsidian.ts @@ -2,7 +2,7 @@ import { createLogger } from '@sim/logger' import { toError } from '@sim/utils/errors' import { ObsidianIcon } from '@/components/icons' import { validateExternalUrl } from '@/lib/core/security/input-validation' -import { fetchWithRetry, VALIDATE_RETRY_OPTIONS } from '@/lib/knowledge/documents/utils' +import { secureFetchWithRetry, VALIDATE_RETRY_OPTIONS } from '@/lib/knowledge/documents/utils' import type { ConnectorConfig, ExternalDocument, ExternalDocumentList } from '@/connectors/types' import { joinTagArray, parseTagDate } from '@/connectors/utils' @@ -24,18 +24,15 @@ interface NoteJson { } /** - * Normalizes the vault URL and validates it against SSRF protections. + * Normalizes the vault URL and runs an early structural SSRF check via the + * shared `validateExternalUrl` policy (hosted Sim blocks localhost/private/HTTP; + * self-hosted allows http://localhost only). * - * The Obsidian Local REST API plugin runs on the user's own machine, so there - * is no SaaS domain to allowlist — the vault URL is fully user-controlled. We - * defer to the shared `validateExternalUrl` policy: - * - hosted Sim: blocks localhost, private IPs, HTTP (forces HTTPS) - * - self-hosted Sim: allows http://localhost (built-in carve-out), still - * blocks non-loopback private IPs and dangerous ports (22, 25, 3306, - * 5432, 6379, 27017, 9200) - * - * This does not defend against DNS rebinding; for hosted deployments the user - * must expose the plugin through a public URL (tunnel, port-forward). + * The authoritative SSRF boundary is enforced at request time: every vault + * request goes through {@link secureFetchWithRetry}, which resolves DNS, + * re-checks the resolved IP, and pins the connection to it — closing the + * DNS-rebinding gap a synchronous string check cannot. On hosted Sim the plugin + * must be exposed through a public URL. */ function resolveVaultEndpoint(rawUrl: string | undefined): string { let url = (rawUrl || DEFAULT_VAULT_URL).trim().replace(/\/+$/, '') @@ -57,12 +54,12 @@ async function listDirectory( baseUrl: string, accessToken: string, dirPath: string, - retryOptions?: Parameters[2] + retryOptions?: Parameters[2] ): Promise { const encodedDir = dirPath ? dirPath.split('/').map(encodeURIComponent).join('/') : '' const endpoint = encodedDir ? `${baseUrl}/vault/${encodedDir}/` : `${baseUrl}/vault/` - const response = await fetchWithRetry( + const response = await secureFetchWithRetry( endpoint, { method: 'GET', @@ -88,7 +85,7 @@ async function listVaultFiles( baseUrl: string, accessToken: string, folderPath?: string, - retryOptions?: Parameters[2], + retryOptions?: Parameters[2], depth = 0 ): Promise { if (depth > MAX_RECURSION_DEPTH) { @@ -134,9 +131,9 @@ async function fetchNote( baseUrl: string, accessToken: string, filePath: string, - retryOptions?: Parameters[2] + retryOptions?: Parameters[2] ): Promise { - const response = await fetchWithRetry( + const response = await secureFetchWithRetry( `${baseUrl}/vault/${filePath.split('/').map(encodeURIComponent).join('/')}`, { method: 'GET', @@ -304,7 +301,7 @@ export const obsidianConnector: ConnectorConfig = { } try { - const response = await fetchWithRetry( + const response = await secureFetchWithRetry( `${baseUrl}/`, { method: 'GET', diff --git a/apps/sim/connectors/s3/s3.ts b/apps/sim/connectors/s3/s3.ts index 2b2ac1843a..db4d0ab85d 100644 --- a/apps/sim/connectors/s3/s3.ts +++ b/apps/sim/connectors/s3/s3.ts @@ -2,7 +2,8 @@ import crypto from 'crypto' import { createLogger } from '@sim/logger' import { getErrorMessage, toError } from '@sim/utils/errors' import { S3Icon } from '@/components/icons' -import { fetchWithRetry, VALIDATE_RETRY_OPTIONS } from '@/lib/knowledge/documents/utils' +import { isHosted } from '@/lib/core/config/feature-flags' +import { secureFetchWithRetry, VALIDATE_RETRY_OPTIONS } from '@/lib/knowledge/documents/utils' import type { ConnectorConfig, ExternalDocument, ExternalDocumentList } from '@/connectors/types' import { parseTagDate, readBodyWithLimit } from '@/connectors/utils' import { encodeS3PathComponent, getSignatureKey } from '@/tools/s3/utils' @@ -112,9 +113,11 @@ function isSupportedKey(key: string, allowedExtensions: Set): boolean { } /** - * Returns true when the host is a loopback address for which plain `http://` - * is tolerated (local MinIO development). Any other host must use `https://` so - * that credentials are never transmitted over cleartext. + * Returns true when the host is a loopback address for which plain `http://` is + * tolerated (local MinIO development on a self-hosted deployment). Any other + * host must use `https://`. This is only an early check — the SSRF boundary is + * enforced at request time by {@link secureFetchWithRetry}, which blocks + * loopback/private targets on hosted Sim regardless of what this parser accepts. */ function isLoopbackHost(host: string): boolean { const bare = host.replace(/^\[|\]$/g, '') @@ -160,9 +163,9 @@ function parseEndpoint(raw: string): S3Endpoint { const host = url.hostname if (!host) throw new Error('Endpoint is missing a host') - if (scheme === 'http' && !isLoopbackHost(host)) { + if (scheme === 'http' && !(isLoopbackHost(host) && !isHosted)) { throw new Error( - 'Plain http:// endpoints are only allowed for localhost — use https:// otherwise' + 'Plain http:// endpoints are only allowed for localhost on self-hosted deployments — use https:// otherwise' ) } @@ -261,7 +264,7 @@ function buildUrl(ctx: S3Context, encodedPath: string, canonicalQueryString: str * Reuses {@link getSignatureKey} from the s3 tool utilities. * * The signed headers embed `x-amz-date` and are reused verbatim across - * `fetchWithRetry` attempts. S3 allows a 15-minute clock-skew window; the + * `secureFetchWithRetry` attempts. S3 allows a 15-minute clock-skew window; the * retry helper's worst-case total backoff (~31s default, ~10s in validate) is * far inside that window, so a stale timestamp never triggers * RequestTimeTooSkewed. @@ -449,7 +452,7 @@ async function listObjectsPage( ctx: S3Context, prefix: string, continuationToken: string | undefined, - retryOptions?: Parameters[2], + retryOptions?: Parameters[2], maxKeys: number = LIST_MAX_KEYS ): Promise<{ objects: S3ObjectEntry[]; isTruncated: boolean; nextContinuationToken?: string }> { const queryParams: Record = { @@ -466,7 +469,7 @@ async function listObjectsPage( const url = buildUrl(ctx, bucketPath, canonicalQueryString) - const response = await fetchWithRetry(url, { method: 'GET', headers }, retryOptions) + const response = await secureFetchWithRetry(url, { method: 'GET', headers }, retryOptions) if (!response.ok) { const errorText = await response.text() @@ -616,7 +619,7 @@ export const s3Connector: ConnectorConfig = { const headers = buildSignedHeaders(ctx, 'GET', encodedPath, '') const url = buildUrl(ctx, encodedPath, '') - const response = await fetchWithRetry(url, { method: 'GET', headers }) + const response = await secureFetchWithRetry(url, { method: 'GET', headers }) if (response.status === 404) return null if (!response.ok) { diff --git a/apps/sim/connectors/sentry/sentry.ts b/apps/sim/connectors/sentry/sentry.ts index 55838ecda5..3972bc6803 100644 --- a/apps/sim/connectors/sentry/sentry.ts +++ b/apps/sim/connectors/sentry/sentry.ts @@ -1,7 +1,7 @@ import { createLogger } from '@sim/logger' import { getErrorMessage, toError } from '@sim/utils/errors' import { SentryIcon } from '@/components/icons' -import { fetchWithRetry, VALIDATE_RETRY_OPTIONS } from '@/lib/knowledge/documents/utils' +import { secureFetchWithRetry, VALIDATE_RETRY_OPTIONS } from '@/lib/knowledge/documents/utils' import type { ConnectorConfig, ExternalDocument, ExternalDocumentList } from '@/connectors/types' import { parseTagDate } from '@/connectors/utils' @@ -373,7 +373,7 @@ async function fetchLatestEvent( ): Promise { const url = `${apiBase}/organizations/${encodeURIComponent(organization)}/issues/${encodeURIComponent(issueId)}/events/latest/` - const response = await fetchWithRetry(url, { + const response = await secureFetchWithRetry(url, { method: 'GET', headers: authHeaders(accessToken), }) @@ -507,7 +507,7 @@ export const sentryConnector: ConnectorConfig = { maxIssues, }) - const response = await fetchWithRetry(url.toString(), { + const response = await secureFetchWithRetry(url.toString(), { method: 'GET', headers: authHeaders(accessToken), }) @@ -564,7 +564,7 @@ export const sentryConnector: ConnectorConfig = { const url = `${apiBase}/organizations/${encodeURIComponent(organization)}/issues/${encodeURIComponent(externalId)}/` - const response = await fetchWithRetry(url, { + const response = await secureFetchWithRetry(url, { method: 'GET', headers: authHeaders(accessToken), }) @@ -629,7 +629,7 @@ export const sentryConnector: ConnectorConfig = { * scope and the project-scoped path style, and gives a precise "not found" * message when the org or project slug is wrong. */ - const projectResponse = await fetchWithRetry( + const projectResponse = await secureFetchWithRetry( `${apiBase}/projects/${encodeURIComponent(organization)}/${encodeURIComponent(project)}/`, { method: 'GET', @@ -670,7 +670,7 @@ export const sentryConnector: ConnectorConfig = { issuesProbeUrl.searchParams.set('query', DEFAULT_QUERY) issuesProbeUrl.searchParams.set('limit', '1') - const issuesResponse = await fetchWithRetry( + const issuesResponse = await secureFetchWithRetry( issuesProbeUrl.toString(), { method: 'GET', diff --git a/apps/sim/connectors/utils.ts b/apps/sim/connectors/utils.ts index cc78c3e680..d30bc67017 100644 --- a/apps/sim/connectors/utils.ts +++ b/apps/sim/connectors/utils.ts @@ -1,3 +1,5 @@ +import type { SecureFetchResponse } from '@/lib/core/security/input-validation.server' + /** * Strips HTML tags from content and decodes common HTML entities. */ @@ -88,7 +90,7 @@ export function parseMultiValue(value: unknown): string[] { * Returns null when the cap is exceeded. */ export async function readBodyWithLimit( - response: Response, + response: Response | SecureFetchResponse, maxBytes: number ): Promise { if (!response.body) { diff --git a/apps/sim/lib/knowledge/documents/utils.test.ts b/apps/sim/lib/knowledge/documents/utils.test.ts index 0474f0a80b..a16ba0e643 100644 --- a/apps/sim/lib/knowledge/documents/utils.test.ts +++ b/apps/sim/lib/knowledge/documents/utils.test.ts @@ -1,8 +1,37 @@ /** * @vitest-environment node */ -import { describe, expect, it } from 'vitest' -import { isRetryableError } from './utils' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockSecureFetchWithValidation } = vi.hoisted(() => ({ + mockSecureFetchWithValidation: vi.fn(), +})) + +vi.mock('@/lib/core/security/input-validation.server', () => ({ + secureFetchWithValidation: mockSecureFetchWithValidation, +})) + +import { isRetryableError, secureFetchWithRetry } from './utils' + +/** Builds a minimal SecureFetchResponse-shaped object for tests. */ +function fakeResponse( + status: number, + options: { headers?: Record; body?: string } = {} +) { + const headers = options.headers ?? {} + return { + ok: status >= 200 && status < 300, + status, + statusText: `status-${status}`, + headers: { get: (name: string) => headers[name.toLowerCase()] ?? null }, + body: null, + text: async () => options.body ?? '', + json: async () => JSON.parse(options.body ?? '{}'), + arrayBuffer: async () => new ArrayBuffer(0), + } +} + +const FAST_RETRY = { initialDelayMs: 1, maxDelayMs: 2, maxRetries: 3 } describe('isRetryableError', () => { describe('retryable status codes', () => { @@ -150,3 +179,93 @@ describe('isRetryableError', () => { }) }) }) + +describe('secureFetchWithRetry', () => { + beforeEach(() => { + mockSecureFetchWithValidation.mockReset() + }) + + it('routes the request through secureFetchWithValidation and returns the response', async () => { + mockSecureFetchWithValidation.mockResolvedValue(fakeResponse(200, { body: 'ok' })) + + const response = await secureFetchWithRetry('https://example.com/api', { + method: 'GET', + headers: { Accept: 'application/json' }, + }) + + expect(response.status).toBe(200) + expect(mockSecureFetchWithValidation).toHaveBeenCalledTimes(1) + const [url, options, paramName] = mockSecureFetchWithValidation.mock.calls[0] + expect(url).toBe('https://example.com/api') + expect(options).toMatchObject({ method: 'GET', headers: { Accept: 'application/json' } }) + expect(paramName).toBe('url') + }) + + it('propagates SSRF validation failures without retrying', async () => { + mockSecureFetchWithValidation.mockRejectedValue( + new Error('url resolves to a blocked IP address') + ) + + await expect( + secureFetchWithRetry('https://attacker.test', { method: 'GET' }, FAST_RETRY) + ).rejects.toThrow('blocked IP address') + + expect(mockSecureFetchWithValidation).toHaveBeenCalledTimes(1) + }) + + it('retries on a retryable status (503) and succeeds', async () => { + mockSecureFetchWithValidation + .mockResolvedValueOnce(fakeResponse(503, { body: 'try later' })) + .mockResolvedValueOnce(fakeResponse(200, { body: 'ok' })) + + const response = await secureFetchWithRetry( + 'https://example.com/api', + { method: 'GET' }, + FAST_RETRY + ) + + expect(response.status).toBe(200) + expect(mockSecureFetchWithValidation).toHaveBeenCalledTimes(2) + }) + + it('does not retry a non-retryable status (404) and returns it to the caller', async () => { + mockSecureFetchWithValidation.mockResolvedValue(fakeResponse(404, { body: 'missing' })) + + const response = await secureFetchWithRetry( + 'https://example.com/api', + { method: 'GET' }, + FAST_RETRY + ) + + expect(response.status).toBe(404) + expect(mockSecureFetchWithValidation).toHaveBeenCalledTimes(1) + }) + + it('forwards allowHttp / timeout / maxResponseBytes to the pinned fetch', async () => { + mockSecureFetchWithValidation.mockResolvedValue(fakeResponse(200)) + + await secureFetchWithRetry( + 'http://localhost:9000', + { method: 'GET' }, + { allowHttp: true, timeout: 5000, maxResponseBytes: 1024, ...FAST_RETRY } + ) + + const [, options] = mockSecureFetchWithValidation.mock.calls[0] + expect(options).toMatchObject({ allowHttp: true, timeout: 5000, maxResponseBytes: 1024 }) + }) + + it('honors Retry-After (seconds) on a 429 before retrying', async () => { + mockSecureFetchWithValidation + .mockResolvedValueOnce(fakeResponse(429, { headers: { 'retry-after': '0' } })) + .mockResolvedValueOnce(fakeResponse(200)) + + const response = await secureFetchWithRetry( + 'https://example.com/api', + { method: 'GET' }, + FAST_RETRY + ) + + expect(response.status).toBe(200) + expect(mockSecureFetchWithValidation).toHaveBeenCalledTimes(2) + }) +}) diff --git a/apps/sim/lib/knowledge/documents/utils.ts b/apps/sim/lib/knowledge/documents/utils.ts index 0a2221cdc3..ceabd5ac13 100644 --- a/apps/sim/lib/knowledge/documents/utils.ts +++ b/apps/sim/lib/knowledge/documents/utils.ts @@ -2,6 +2,11 @@ import { createLogger } from '@sim/logger' import { toError } from '@sim/utils/errors' import { sleep } from '@sim/utils/helpers' import { randomFloat } from '@sim/utils/random' +import { + type SecureFetchOptions, + type SecureFetchResponse, + secureFetchWithValidation, +} from '@/lib/core/security/input-validation.server' const logger = createLogger('RetryUtils') @@ -209,3 +214,59 @@ export async function fetchWithRetry( return response }, retryOptions) } + +export interface SecureFetchRetryOptions extends RetryOptions { + allowHttp?: boolean + timeout?: number + maxResponseBytes?: number +} + +/** + * SSRF-safe counterpart to {@link fetchWithRetry} for connector requests to + * user-controlled hosts. Every attempt re-runs {@link secureFetchWithValidation} + * (DNS resolution, private/loopback/reserved-IP rejection, IP-pinned connection, + * redirect re-validation); retry/backoff semantics mirror {@link fetchWithRetry}. + */ +export async function secureFetchWithRetry( + url: string, + options: SecureFetchOptions = {}, + retryOptions: SecureFetchRetryOptions = {} +): Promise { + const { allowHttp, timeout, maxResponseBytes, ...retry } = retryOptions + + return retryWithExponentialBackoff(async () => { + const response = await secureFetchWithValidation( + url, + { + ...options, + ...(allowHttp !== undefined ? { allowHttp } : {}), + ...(timeout !== undefined ? { timeout } : {}), + ...(maxResponseBytes !== undefined ? { maxResponseBytes } : {}), + }, + 'url' + ) + + if (!response.ok && isRetryableError({ status: response.status })) { + const errorText = await response.text() + const error: HTTPError = new Error( + `HTTP ${response.status}: ${response.statusText} - ${errorText}` + ) + error.status = response.status + error.statusText = response.statusText + + const retryAfter = response.headers.get('retry-after') + if (retryAfter) { + const waitMs = Number.isNaN(Number(retryAfter)) + ? Math.max(0, new Date(retryAfter).getTime() - Date.now()) + : Number(retryAfter) * 1000 + if (waitMs > 0) { + error.retryAfterMs = waitMs + } + } + + throw error + } + + return response + }, retry) +} From 0f7bf96d554ef303db749b33ceda13ae7d2b203d Mon Sep 17 00:00:00 2001 From: waleed Date: Sat, 6 Jun 2026 12:22:30 -0700 Subject: [PATCH 06/10] fix(audit-logs): scope enterprise audit log access to organization boundary Actor membership was used as a standalone tenant predicate, letting org admins read members' audit activity from personal workspaces and other tenants. Scope queries to org-attached workspaces plus org-level events, with actor membership only narrowing the scope; validate workspaceId filters against the caller's organization. --- apps/sim/app/api/audit-logs/route.ts | 11 +- .../app/api/v1/audit-logs/[id]/route.test.ts | 129 ++++++++++++++ apps/sim/app/api/v1/audit-logs/[id]/route.ts | 33 ++-- apps/sim/app/api/v1/audit-logs/query.test.ts | 166 ++++++++++++++++++ apps/sim/app/api/v1/audit-logs/query.ts | 69 +++++--- apps/sim/app/api/v1/audit-logs/route.test.ts | 130 ++++++++++++++ apps/sim/app/api/v1/audit-logs/route.ts | 19 +- 7 files changed, 514 insertions(+), 43 deletions(-) create mode 100644 apps/sim/app/api/v1/audit-logs/[id]/route.test.ts create mode 100644 apps/sim/app/api/v1/audit-logs/query.test.ts create mode 100644 apps/sim/app/api/v1/audit-logs/route.test.ts diff --git a/apps/sim/app/api/audit-logs/route.ts b/apps/sim/app/api/audit-logs/route.ts index 9dcf5977a5..5f2a352279 100644 --- a/apps/sim/app/api/audit-logs/route.ts +++ b/apps/sim/app/api/audit-logs/route.ts @@ -10,6 +10,7 @@ import { formatAuditLogEntry } from '@/app/api/v1/audit-logs/format' import { buildFilterConditions, buildOrgScopeCondition, + getOrgWorkspaceIds, queryAuditLogs, } from '@/app/api/v1/audit-logs/query' @@ -29,7 +30,7 @@ export const GET = withRouteHandler(async (request: NextRequest) => { return authResult.response } - const { orgMemberIds } = authResult.context + const { organizationId, orgMemberIds } = authResult.context const parsed = await parseRequest( listAuditLogsContract, @@ -57,7 +58,13 @@ export const GET = withRouteHandler(async (request: NextRequest) => { cursor, } = parsed.data.query - const scopeCondition = await buildOrgScopeCondition(orgMemberIds, includeDeparted) + const orgWorkspaceIds = await getOrgWorkspaceIds(organizationId) + const scopeCondition = buildOrgScopeCondition({ + organizationId, + orgWorkspaceIds, + orgMemberIds, + includeDeparted, + }) const filterConditions = buildFilterConditions({ action, resourceType, diff --git a/apps/sim/app/api/v1/audit-logs/[id]/route.test.ts b/apps/sim/app/api/v1/audit-logs/[id]/route.test.ts new file mode 100644 index 0000000000..c44f99f0d2 --- /dev/null +++ b/apps/sim/app/api/v1/audit-logs/[id]/route.test.ts @@ -0,0 +1,129 @@ +/** + * @vitest-environment node + * + * Tests for GET /api/v1/audit-logs/[id] — verifies the lookup is constrained + * by the organization scope and 404s for rows outside it. + */ +import { createMockRequest, dbChainMock, dbChainMockFns } from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { + mockCheckRateLimit, + mockValidateEnterpriseAuditAccess, + mockBuildOrgScopeCondition, + mockGetOrgWorkspaceIds, +} = vi.hoisted(() => ({ + mockCheckRateLimit: vi.fn(), + mockValidateEnterpriseAuditAccess: vi.fn(), + mockBuildOrgScopeCondition: vi.fn(), + mockGetOrgWorkspaceIds: vi.fn(), +})) + +vi.mock('@sim/db', () => dbChainMock) + +vi.mock('@/app/api/v1/middleware', () => ({ + checkRateLimit: mockCheckRateLimit, + createRateLimitResponse: vi.fn(), +})) + +vi.mock('@/app/api/v1/audit-logs/auth', () => ({ + validateEnterpriseAuditAccess: mockValidateEnterpriseAuditAccess, +})) + +vi.mock('@/app/api/v1/audit-logs/query', () => ({ + buildOrgScopeCondition: mockBuildOrgScopeCondition, + getOrgWorkspaceIds: mockGetOrgWorkspaceIds, +})) + +vi.mock('@/app/api/v1/logs/meta', () => ({ + getUserLimits: vi.fn().mockResolvedValue({}), + createApiResponse: vi.fn((body: unknown) => ({ body, headers: {} })), +})) + +import { GET } from '@/app/api/v1/audit-logs/[id]/route' + +const ORG_ID = 'org-1' +const MEMBER_IDS = ['admin-1', 'member-1'] +const ORG_WORKSPACE_IDS = ['ws-org-1'] +const SCOPE_SENTINEL = { type: 'org-scope-sentinel' } + +const AUDIT_ROW = { + id: 'log-1', + workspaceId: 'ws-org-1', + actorId: 'member-1', + actorName: 'Member', + actorEmail: 'member@example.com', + action: 'workflow.created', + resourceType: 'workflow', + resourceId: 'wf-1', + resourceName: 'My Workflow', + description: 'Created workflow', + metadata: {}, + ipAddress: '127.0.0.1', + userAgent: 'test', + createdAt: new Date('2026-01-01T00:00:00Z'), +} + +function callRoute(id: string) { + const request = createMockRequest( + 'GET', + undefined, + {}, + `http://localhost:3000/api/v1/audit-logs/${id}` + ) + return GET(request, { params: Promise.resolve({ id }) }) +} + +describe('GET /api/v1/audit-logs/[id]', () => { + beforeEach(() => { + vi.clearAllMocks() + mockCheckRateLimit.mockResolvedValue({ allowed: true, userId: 'admin-1' }) + mockValidateEnterpriseAuditAccess.mockResolvedValue({ + success: true, + context: { organizationId: ORG_ID, orgMemberIds: MEMBER_IDS }, + }) + mockGetOrgWorkspaceIds.mockResolvedValue(ORG_WORKSPACE_IDS) + mockBuildOrgScopeCondition.mockReturnValue(SCOPE_SENTINEL) + }) + + it('constrains the lookup with the org scope condition (includeDeparted)', async () => { + dbChainMockFns.limit.mockResolvedValueOnce([AUDIT_ROW]) + + const response = await callRoute('log-1') + + expect(response.status).toBe(200) + expect(mockBuildOrgScopeCondition).toHaveBeenCalledWith({ + organizationId: ORG_ID, + orgWorkspaceIds: ORG_WORKSPACE_IDS, + orgMemberIds: MEMBER_IDS, + includeDeparted: true, + }) + expect(dbChainMockFns.where).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'and', + conditions: expect.arrayContaining([SCOPE_SENTINEL]), + }) + ) + }) + + it('returns 404 when the row is outside the organization scope', async () => { + dbChainMockFns.limit.mockResolvedValueOnce([]) + + const response = await callRoute('log-outside-org') + + expect(response.status).toBe(404) + const body = await response.json() + expect(body.error).toBe('Audit log not found') + }) + + it('excludes ipAddress and userAgent from the response', async () => { + dbChainMockFns.limit.mockResolvedValueOnce([AUDIT_ROW]) + + const response = await callRoute('log-1') + const body = await response.json() + + expect(body.data.id).toBe('log-1') + expect(body.data.ipAddress).toBeUndefined() + expect(body.data.userAgent).toBeUndefined() + }) +}) diff --git a/apps/sim/app/api/v1/audit-logs/[id]/route.ts b/apps/sim/app/api/v1/audit-logs/[id]/route.ts index 3c7208cc84..bac0b83f16 100644 --- a/apps/sim/app/api/v1/audit-logs/[id]/route.ts +++ b/apps/sim/app/api/v1/audit-logs/[id]/route.ts @@ -4,24 +4,26 @@ * Get a single audit log entry by ID, scoped to the authenticated user's organization. * Requires enterprise subscription and org admin/owner role. * - * Scope includes logs from current org members AND logs within org workspaces - * (including those from departed members or system actions with null actorId). + * Scope is the organization boundary: logs within org-attached workspaces and + * org-level events (including those from departed members or system actions + * with null actorId). * * Response: { data: AuditLogEntry, limits: UserLimits } */ import { db } from '@sim/db' -import { auditLog, workspace } from '@sim/db/schema' +import { auditLog } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' import { generateId } from '@sim/utils/id' -import { and, eq, inArray, or } from 'drizzle-orm' +import { and, eq } from 'drizzle-orm' import { type NextRequest, NextResponse } from 'next/server' import { v1GetAuditLogContract } from '@/lib/api/contracts/v1/audit-logs' import { parseRequest } from '@/lib/api/server' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { validateEnterpriseAuditAccess } from '@/app/api/v1/audit-logs/auth' import { formatAuditLogEntry } from '@/app/api/v1/audit-logs/format' +import { buildOrgScopeCondition, getOrgWorkspaceIds } from '@/app/api/v1/audit-logs/query' import { createApiResponse, getUserLimits } from '@/app/api/v1/logs/meta' import { checkRateLimit, createRateLimitResponse } from '@/app/api/v1/middleware' @@ -53,25 +55,20 @@ export const GET = withRouteHandler( return authResult.response } - const { orgMemberIds } = authResult.context + const { organizationId, orgMemberIds } = authResult.context - const orgWorkspaceIds = db - .select({ id: workspace.id }) - .from(workspace) - .where(inArray(workspace.ownerId, orgMemberIds)) + const orgWorkspaceIds = await getOrgWorkspaceIds(organizationId) + const scopeCondition = buildOrgScopeCondition({ + organizationId, + orgWorkspaceIds, + orgMemberIds, + includeDeparted: true, + }) const [log] = await db .select() .from(auditLog) - .where( - and( - eq(auditLog.id, id), - or( - inArray(auditLog.actorId, orgMemberIds), - inArray(auditLog.workspaceId, orgWorkspaceIds) - ) - ) - ) + .where(and(eq(auditLog.id, id), scopeCondition)) .limit(1) if (!log) { diff --git a/apps/sim/app/api/v1/audit-logs/query.test.ts b/apps/sim/app/api/v1/audit-logs/query.test.ts new file mode 100644 index 0000000000..245b1ebeb7 --- /dev/null +++ b/apps/sim/app/api/v1/audit-logs/query.test.ts @@ -0,0 +1,166 @@ +/** + * @vitest-environment node + * + * Tests for the enterprise audit-log tenant boundary. The global drizzle-orm + * mock returns structured operator objects, so these tests assert directly on + * the predicate tree. + */ +import { dbChainMock, dbChainMockFns } from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +vi.mock('@sim/db', () => dbChainMock) + +import { buildOrgScopeCondition, getOrgWorkspaceIds } from '@/app/api/v1/audit-logs/query' + +const ORG_ID = 'org-1' +const MEMBER_IDS = ['user-1', 'user-2'] +const WORKSPACE_IDS = ['ws-1', 'ws-2'] + +interface MockCondition { + type?: string + conditions?: MockCondition[] + column?: string + values?: string[] + left?: string + right?: string + strings?: string[] +} + +function asCondition(value: unknown): MockCondition { + return value as MockCondition +} + +/** + * Asserts the condition matches null-workspace rows tied to the organization + * via metadata or the organization resource itself. + */ +function expectOrgLevelCondition(condition: MockCondition, organizationId: string): void { + expect(condition.type).toBe('and') + const [nullCheck, orgLink] = condition.conditions! + expect(nullCheck).toMatchObject({ type: 'isNull', column: 'workspaceId' }) + + expect(orgLink.type).toBe('or') + const [metadataMatch, orgResourceMatch] = orgLink.conditions! + expect(metadataMatch.strings?.join('?')).toContain("->>'organizationId' =") + expect(metadataMatch.values).toContain(organizationId) + + expect(orgResourceMatch.type).toBe('and') + expect(orgResourceMatch.conditions).toEqual([ + expect.objectContaining({ type: 'eq', left: 'resourceType', right: 'organization' }), + expect.objectContaining({ type: 'eq', left: 'resourceId', right: organizationId }), + ]) +} + +describe('buildOrgScopeCondition', () => { + it('never uses actor membership as a standalone boundary (default scope)', () => { + const condition = asCondition( + buildOrgScopeCondition({ + organizationId: ORG_ID, + orgWorkspaceIds: WORKSPACE_IDS, + orgMemberIds: MEMBER_IDS, + includeDeparted: false, + }) + ) + + expect(condition.type).toBe('and') + const [orgScope, actorFilter] = condition.conditions! + + expect(orgScope.type).toBe('or') + const [workspaceScope, orgLevel] = orgScope.conditions! + expect(workspaceScope).toMatchObject({ + type: 'inArray', + column: 'workspaceId', + values: WORKSPACE_IDS, + }) + expectOrgLevelCondition(orgLevel, ORG_ID) + + expect(actorFilter).toMatchObject({ + type: 'inArray', + column: 'actorId', + values: MEMBER_IDS, + }) + }) + + it('omits the actor filter entirely when includeDeparted is true', () => { + const condition = asCondition( + buildOrgScopeCondition({ + organizationId: ORG_ID, + orgWorkspaceIds: WORKSPACE_IDS, + orgMemberIds: MEMBER_IDS, + includeDeparted: true, + }) + ) + + expect(condition.type).toBe('or') + const [workspaceScope, orgLevel] = condition.conditions! + expect(workspaceScope).toMatchObject({ + type: 'inArray', + column: 'workspaceId', + values: WORKSPACE_IDS, + }) + expectOrgLevelCondition(orgLevel, ORG_ID) + + expect(JSON.stringify(condition)).not.toContain('actorId') + }) + + it('falls back to the org-level branch alone when the org has no workspaces', () => { + const condition = asCondition( + buildOrgScopeCondition({ + organizationId: ORG_ID, + orgWorkspaceIds: [], + orgMemberIds: MEMBER_IDS, + includeDeparted: true, + }) + ) + + expectOrgLevelCondition(condition, ORG_ID) + }) + + it('still applies the actor filter on top of the org scope with no workspaces', () => { + const condition = asCondition( + buildOrgScopeCondition({ + organizationId: ORG_ID, + orgWorkspaceIds: [], + orgMemberIds: MEMBER_IDS, + includeDeparted: false, + }) + ) + + expect(condition.type).toBe('and') + const [orgLevel, actorFilter] = condition.conditions! + expectOrgLevelCondition(orgLevel, ORG_ID) + expect(actorFilter).toMatchObject({ + type: 'inArray', + column: 'actorId', + values: MEMBER_IDS, + }) + }) + + it('matches nothing when the org has no current members and includeDeparted is false', () => { + const condition = asCondition( + buildOrgScopeCondition({ + organizationId: ORG_ID, + orgWorkspaceIds: WORKSPACE_IDS, + orgMemberIds: [], + includeDeparted: false, + }) + ) + + expect(condition.strings?.join('')).toBe('1 = 0') + }) +}) + +describe('getOrgWorkspaceIds', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('selects workspaces by organization ownership, not member ownership', async () => { + const ids = await getOrgWorkspaceIds(ORG_ID) + + expect(ids).toEqual([]) + expect(dbChainMockFns.where).toHaveBeenCalledWith( + expect.objectContaining({ type: 'eq', left: 'organizationId', right: ORG_ID }) + ) + }) +}) diff --git a/apps/sim/app/api/v1/audit-logs/query.ts b/apps/sim/app/api/v1/audit-logs/query.ts index 307457346c..f166b10cea 100644 --- a/apps/sim/app/api/v1/audit-logs/query.ts +++ b/apps/sim/app/api/v1/audit-logs/query.ts @@ -1,7 +1,8 @@ +import { AuditResourceType } from '@sim/audit' import { db } from '@sim/db' import { auditLog, workspace } from '@sim/db/schema' import type { InferSelectModel } from 'drizzle-orm' -import { and, desc, eq, gte, ilike, inArray, lt, lte, or, type SQL, sql } from 'drizzle-orm' +import { and, desc, eq, gte, ilike, inArray, isNull, lt, lte, or, type SQL, sql } from 'drizzle-orm' type DbAuditLog = InferSelectModel @@ -68,33 +69,59 @@ export function buildFilterConditions(params: AuditLogFilterParams): SQL { + const rows = await db + .select({ id: workspace.id }) + .from(workspace) + .where(eq(workspace.organizationId, organizationId)) + return rows.map((row) => row.id) +} + +export interface OrgScopeParams { + organizationId: string + orgWorkspaceIds: string[] + orgMemberIds: string[] includeDeparted: boolean -): Promise> { - if (orgMemberIds.length === 0) { - return sql`1 = 0` - } +} - if (!includeDeparted) { - return inArray(auditLog.actorId, orgMemberIds) - } +/** + * Builds the tenant-boundary predicate for organization audit log access: + * rows in org-attached workspaces, plus org-level rows (`workspace_id IS + * NULL`) tied to the org via `metadata.organizationId` or the organization + * resource itself. Actor membership is never a standalone boundary — when + * `includeDeparted` is false it only narrows the org scope. + */ +export function buildOrgScopeCondition(params: OrgScopeParams): SQL { + const { organizationId, orgWorkspaceIds, orgMemberIds, includeDeparted } = params + + const orgLevelCondition = and( + isNull(auditLog.workspaceId), + or( + sql`${auditLog.metadata}->>'organizationId' = ${organizationId}`, + and( + eq(auditLog.resourceType, AuditResourceType.ORGANIZATION), + eq(auditLog.resourceId, organizationId) + ) + ) + )! - const orgWorkspaces = await db - .select({ id: workspace.id }) - .from(workspace) - .where(inArray(workspace.ownerId, orgMemberIds)) + const orgScope = + orgWorkspaceIds.length > 0 + ? or(inArray(auditLog.workspaceId, orgWorkspaceIds), orgLevelCondition)! + : orgLevelCondition - const orgWorkspaceIds = orgWorkspaces.map((w) => w.id) + if (includeDeparted) { + return orgScope + } - if (orgWorkspaceIds.length > 0) { - return or( - inArray(auditLog.actorId, orgMemberIds), - inArray(auditLog.workspaceId, orgWorkspaceIds) - )! + if (orgMemberIds.length === 0) { + return sql`1 = 0` } - return inArray(auditLog.actorId, orgMemberIds) + return and(orgScope, inArray(auditLog.actorId, orgMemberIds))! } function buildCursorCondition(cursor: string): SQL | null { diff --git a/apps/sim/app/api/v1/audit-logs/route.test.ts b/apps/sim/app/api/v1/audit-logs/route.test.ts new file mode 100644 index 0000000000..336f50371c --- /dev/null +++ b/apps/sim/app/api/v1/audit-logs/route.test.ts @@ -0,0 +1,130 @@ +/** + * @vitest-environment node + * + * Tests for GET /api/v1/audit-logs — verifies filters are validated against + * the caller's organization and the scope is built from the org context. + */ +import { createMockRequest } from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { + mockCheckRateLimit, + mockValidateEnterpriseAuditAccess, + mockBuildOrgScopeCondition, + mockGetOrgWorkspaceIds, + mockQueryAuditLogs, + mockBuildFilterConditions, +} = vi.hoisted(() => ({ + mockCheckRateLimit: vi.fn(), + mockValidateEnterpriseAuditAccess: vi.fn(), + mockBuildOrgScopeCondition: vi.fn(), + mockGetOrgWorkspaceIds: vi.fn(), + mockQueryAuditLogs: vi.fn(), + mockBuildFilterConditions: vi.fn(), +})) + +vi.mock('@/app/api/v1/middleware', () => ({ + checkRateLimit: mockCheckRateLimit, + createRateLimitResponse: vi.fn(), +})) + +vi.mock('@/app/api/v1/audit-logs/auth', () => ({ + validateEnterpriseAuditAccess: mockValidateEnterpriseAuditAccess, +})) + +vi.mock('@/app/api/v1/audit-logs/query', () => ({ + buildFilterConditions: mockBuildFilterConditions, + buildOrgScopeCondition: mockBuildOrgScopeCondition, + getOrgWorkspaceIds: mockGetOrgWorkspaceIds, + queryAuditLogs: mockQueryAuditLogs, +})) + +vi.mock('@/app/api/v1/logs/meta', () => ({ + getUserLimits: vi.fn().mockResolvedValue({}), + createApiResponse: vi.fn((body: unknown) => ({ body, headers: {} })), +})) + +import { GET } from '@/app/api/v1/audit-logs/route' + +const ORG_ID = 'org-1' +const MEMBER_IDS = ['admin-1', 'member-1'] +const ORG_WORKSPACE_IDS = ['ws-org-1', 'ws-org-2'] +const SCOPE_SENTINEL = { type: 'org-scope-sentinel' } + +function makeRequest(query: string) { + return createMockRequest('GET', undefined, {}, `http://localhost:3000/api/v1/audit-logs${query}`) +} + +describe('GET /api/v1/audit-logs', () => { + beforeEach(() => { + vi.clearAllMocks() + mockCheckRateLimit.mockResolvedValue({ allowed: true, userId: 'admin-1' }) + mockValidateEnterpriseAuditAccess.mockResolvedValue({ + success: true, + context: { organizationId: ORG_ID, orgMemberIds: MEMBER_IDS }, + }) + mockGetOrgWorkspaceIds.mockResolvedValue(ORG_WORKSPACE_IDS) + mockBuildOrgScopeCondition.mockReturnValue(SCOPE_SENTINEL) + mockBuildFilterConditions.mockReturnValue([]) + mockQueryAuditLogs.mockResolvedValue({ data: [], nextCursor: undefined }) + }) + + it('rejects an actorId that is not a current org member', async () => { + const response = await GET(makeRequest('?actorId=outsider-1')) + + expect(response.status).toBe(400) + const body = await response.json() + expect(body.error).toBe('actorId is not a member of your organization') + expect(mockQueryAuditLogs).not.toHaveBeenCalled() + }) + + it('rejects a workspaceId that does not belong to the organization', async () => { + const response = await GET(makeRequest('?workspaceId=ws-other-org')) + + expect(response.status).toBe(400) + const body = await response.json() + expect(body.error).toBe('workspaceId does not belong to your organization') + expect(mockQueryAuditLogs).not.toHaveBeenCalled() + }) + + it('accepts a workspaceId that belongs to the organization', async () => { + const response = await GET(makeRequest('?workspaceId=ws-org-1')) + + expect(response.status).toBe(200) + expect(mockQueryAuditLogs).toHaveBeenCalled() + }) + + it('builds the scope from the organization context, never from actors alone', async () => { + const response = await GET(makeRequest('?actorId=member-1')) + + expect(response.status).toBe(200) + expect(mockBuildOrgScopeCondition).toHaveBeenCalledWith({ + organizationId: ORG_ID, + orgWorkspaceIds: ORG_WORKSPACE_IDS, + orgMemberIds: MEMBER_IDS, + includeDeparted: false, + }) + + const [conditions] = mockQueryAuditLogs.mock.calls[0] + expect(conditions[0]).toBe(SCOPE_SENTINEL) + }) + + it('passes includeDeparted through to the scope builder', async () => { + const response = await GET(makeRequest('?includeDeparted=true')) + + expect(response.status).toBe(200) + expect(mockBuildOrgScopeCondition).toHaveBeenCalledWith( + expect.objectContaining({ includeDeparted: true }) + ) + }) + + it('returns the auth failure response when enterprise access is denied', async () => { + const denied = new Response(JSON.stringify({ error: 'nope' }), { status: 403 }) + mockValidateEnterpriseAuditAccess.mockResolvedValue({ success: false, response: denied }) + + const response = await GET(makeRequest('')) + + expect(response.status).toBe(403) + expect(mockQueryAuditLogs).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/app/api/v1/audit-logs/route.ts b/apps/sim/app/api/v1/audit-logs/route.ts index 365381b075..227cb7f8e6 100644 --- a/apps/sim/app/api/v1/audit-logs/route.ts +++ b/apps/sim/app/api/v1/audit-logs/route.ts @@ -31,6 +31,7 @@ import { formatAuditLogEntry } from '@/app/api/v1/audit-logs/format' import { buildFilterConditions, buildOrgScopeCondition, + getOrgWorkspaceIds, queryAuditLogs, } from '@/app/api/v1/audit-logs/query' import { createApiResponse, getUserLimits } from '@/app/api/v1/logs/meta' @@ -57,7 +58,7 @@ export const GET = withRouteHandler(async (request: NextRequest) => { return authResult.response } - const { orgMemberIds } = authResult.context + const { organizationId, orgMemberIds } = authResult.context const parsed = await parseRequest( v1ListAuditLogsContract, @@ -85,7 +86,21 @@ export const GET = withRouteHandler(async (request: NextRequest) => { ) } - const scopeCondition = await buildOrgScopeCondition(orgMemberIds, params.includeDeparted) + const orgWorkspaceIds = await getOrgWorkspaceIds(organizationId) + + if (params.workspaceId && !orgWorkspaceIds.includes(params.workspaceId)) { + return NextResponse.json( + { error: 'workspaceId does not belong to your organization' }, + { status: 400 } + ) + } + + const scopeCondition = buildOrgScopeCondition({ + organizationId, + orgWorkspaceIds, + orgMemberIds, + includeDeparted: params.includeDeparted, + }) const filterConditions = buildFilterConditions({ action: params.action, resourceType: params.resourceType, From 720b2036ffb656f036f28ca6078745cc54daf469 Mon Sep 17 00:00:00 2001 From: waleed Date: Sat, 6 Jun 2026 12:54:24 -0700 Subject: [PATCH 07/10] fix(webhooks): warn when Twilio webhook has no auth token configured Addresses PR review: when no auth token is set, verifyTwilioAuth skips signature verification (optional-secret convention). Log a warning so operators can detect a webhook running unauthenticated. --- apps/sim/lib/webhooks/providers/twilio-signature.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/apps/sim/lib/webhooks/providers/twilio-signature.ts b/apps/sim/lib/webhooks/providers/twilio-signature.ts index acd7d6b8b5..a8f74d25e4 100644 --- a/apps/sim/lib/webhooks/providers/twilio-signature.ts +++ b/apps/sim/lib/webhooks/providers/twilio-signature.ts @@ -78,6 +78,9 @@ export async function verifyTwilioAuth( const authToken = providerConfig.authToken as string | undefined if (!authToken) { + logger.warn( + `[${requestId}] ${providerLabel} webhook has no auth token configured — accepting request without signature verification. Configure an auth token to require signed requests.` + ) return null } From 4427bb59fca729062e6b30d45db84a5a66d57a2f Mon Sep 17 00:00:00 2001 From: waleed Date: Sat, 6 Jun 2026 13:17:57 -0700 Subject: [PATCH 08/10] fix(audit-logs): include system events (null actor) in default org audit scope SQL IN never matches NULL, so system/automated events inside org workspaces were hidden unless includeDeparted=true. The default scope now matches current members OR null-actor rows, still inside the org boundary. --- apps/sim/app/api/v1/audit-logs/query.test.ts | 22 +++++++++++++------- apps/sim/app/api/v1/audit-logs/query.ts | 12 ++++++----- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/apps/sim/app/api/v1/audit-logs/query.test.ts b/apps/sim/app/api/v1/audit-logs/query.test.ts index 245b1ebeb7..c4a4aacecf 100644 --- a/apps/sim/app/api/v1/audit-logs/query.test.ts +++ b/apps/sim/app/api/v1/audit-logs/query.test.ts @@ -75,9 +75,11 @@ describe('buildOrgScopeCondition', () => { expectOrgLevelCondition(orgLevel, ORG_ID) expect(actorFilter).toMatchObject({ - type: 'inArray', - column: 'actorId', - values: MEMBER_IDS, + type: 'or', + conditions: [ + expect.objectContaining({ type: 'inArray', column: 'actorId', values: MEMBER_IDS }), + expect.objectContaining({ type: 'isNull', column: 'actorId' }), + ], }) }) @@ -130,13 +132,15 @@ describe('buildOrgScopeCondition', () => { const [orgLevel, actorFilter] = condition.conditions! expectOrgLevelCondition(orgLevel, ORG_ID) expect(actorFilter).toMatchObject({ - type: 'inArray', - column: 'actorId', - values: MEMBER_IDS, + type: 'or', + conditions: [ + expect.objectContaining({ type: 'inArray', column: 'actorId', values: MEMBER_IDS }), + expect.objectContaining({ type: 'isNull', column: 'actorId' }), + ], }) }) - it('matches nothing when the org has no current members and includeDeparted is false', () => { + it('only matches system events when the org has no current members', () => { const condition = asCondition( buildOrgScopeCondition({ organizationId: ORG_ID, @@ -146,7 +150,9 @@ describe('buildOrgScopeCondition', () => { }) ) - expect(condition.strings?.join('')).toBe('1 = 0') + expect(condition.type).toBe('and') + const [, actorFilter] = condition.conditions! + expect(actorFilter).toMatchObject({ type: 'isNull', column: 'actorId' }) }) }) diff --git a/apps/sim/app/api/v1/audit-logs/query.ts b/apps/sim/app/api/v1/audit-logs/query.ts index f166b10cea..8a4173be86 100644 --- a/apps/sim/app/api/v1/audit-logs/query.ts +++ b/apps/sim/app/api/v1/audit-logs/query.ts @@ -92,7 +92,8 @@ export interface OrgScopeParams { * rows in org-attached workspaces, plus org-level rows (`workspace_id IS * NULL`) tied to the org via `metadata.organizationId` or the organization * resource itself. Actor membership is never a standalone boundary — when - * `includeDeparted` is false it only narrows the org scope. + * `includeDeparted` is false it only narrows the org scope to current members + * and system events (null actor). */ export function buildOrgScopeCondition(params: OrgScopeParams): SQL { const { organizationId, orgWorkspaceIds, orgMemberIds, includeDeparted } = params @@ -117,11 +118,12 @@ export function buildOrgScopeCondition(params: OrgScopeParams): SQL { return orgScope } - if (orgMemberIds.length === 0) { - return sql`1 = 0` - } + const currentActorCondition = + orgMemberIds.length > 0 + ? or(inArray(auditLog.actorId, orgMemberIds), isNull(auditLog.actorId))! + : isNull(auditLog.actorId) - return and(orgScope, inArray(auditLog.actorId, orgMemberIds))! + return and(orgScope, currentActorCondition)! } function buildCursorCondition(cursor: string): SQL | null { From 4a7c354bec9cf1623459971a17c65fd060a2ff23 Mon Sep 17 00:00:00 2001 From: waleed Date: Sat, 6 Jun 2026 13:17:57 -0700 Subject: [PATCH 09/10] fix(auth): type-safe access to OAuth2Tokens raw payload Installed better-auth's OAuth2Tokens no longer declares the raw property; access it through an intersection cast (no behavior change) so type-check passes. --- apps/sim/lib/auth/auth.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apps/sim/lib/auth/auth.ts b/apps/sim/lib/auth/auth.ts index cd27bbd5e2..e7bde9ee3f 100644 --- a/apps/sim/lib/auth/auth.ts +++ b/apps/sim/lib/auth/auth.ts @@ -2543,7 +2543,8 @@ export const auth = betterAuth({ * marker disambiguates it from a legacy bot id (same `U.../B...` shape); * absent it, we keep the legacy format and today's behavior. */ - const authedUser = tokens.raw?.authed_user as { id?: string } | undefined + const rawTokens = (tokens as typeof tokens & { raw?: Record }).raw + const authedUser = rawTokens?.authed_user as { id?: string } | undefined const installerUserId = authedUser?.id const userSegment = installerUserId ? `usr_${installerUserId}` From 69b38572bf7ea5af4a78b3f693c60e847207bd44 Mon Sep 17 00:00:00 2001 From: waleed Date: Sat, 6 Jun 2026 13:34:15 -0700 Subject: [PATCH 10/10] fix(build): keep connector SSRF fetch out of the client bundle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The connectors SSRF fix routed s3/gitlab/sentry/obsidian through secureFetchWithRetry, which transitively imports input-validation.server (and its Node-only `dns/promises`). connectors/registry.ts is imported by client components for connector metadata, so the connector sync code — which only ever runs in server API routes — gets pulled into the client bundle, and Turbopack fails to resolve `dns/promises` (no browser shim). - Move secureFetchWithRetry into a dedicated `secure-fetch.server` module so the shared documents/utils stays client-safe; connectors import from there. - Add a browser-only `turbopack.resolveAlias` stub for `dns`/`dns/promises` (the documented Next 16 remedy). Server bundles keep the real module, so SSRF validation is unaffected — only the never-executed client copy is stubbed. Verified with a full `next build` (compiles successfully, no module errors). --- apps/sim/connectors/gitlab/gitlab.ts | 3 +- apps/sim/connectors/obsidian/obsidian.ts | 3 +- apps/sim/connectors/s3/s3.ts | 3 +- apps/sim/connectors/sentry/sentry.ts | 3 +- .../security/empty-node-fallback.browser.ts | 13 ++++ .../documents/secure-fetch.server.ts | 71 +++++++++++++++++++ .../sim/lib/knowledge/documents/utils.test.ts | 3 +- apps/sim/lib/knowledge/documents/utils.ts | 63 +--------------- apps/sim/next.config.ts | 11 +++ 9 files changed, 106 insertions(+), 67 deletions(-) create mode 100644 apps/sim/lib/core/security/empty-node-fallback.browser.ts create mode 100644 apps/sim/lib/knowledge/documents/secure-fetch.server.ts diff --git a/apps/sim/connectors/gitlab/gitlab.ts b/apps/sim/connectors/gitlab/gitlab.ts index 78acaebc6f..19f209d4d4 100644 --- a/apps/sim/connectors/gitlab/gitlab.ts +++ b/apps/sim/connectors/gitlab/gitlab.ts @@ -3,7 +3,8 @@ import { getErrorMessage, toError } from '@sim/utils/errors' import { GitLabIcon } from '@/components/icons' import type { SecureFetchResponse } from '@/lib/core/security/input-validation.server' import { isSameOrigin } from '@/lib/core/utils/validation' -import { secureFetchWithRetry, VALIDATE_RETRY_OPTIONS } from '@/lib/knowledge/documents/utils' +import { secureFetchWithRetry } from '@/lib/knowledge/documents/secure-fetch.server' +import { VALIDATE_RETRY_OPTIONS } from '@/lib/knowledge/documents/utils' import type { ConnectorConfig, ExternalDocument, ExternalDocumentList } from '@/connectors/types' import { computeContentHash, joinTagArray, parseTagDate } from '@/connectors/utils' diff --git a/apps/sim/connectors/obsidian/obsidian.ts b/apps/sim/connectors/obsidian/obsidian.ts index 1169a54206..e6f8d6cda3 100644 --- a/apps/sim/connectors/obsidian/obsidian.ts +++ b/apps/sim/connectors/obsidian/obsidian.ts @@ -2,7 +2,8 @@ import { createLogger } from '@sim/logger' import { toError } from '@sim/utils/errors' import { ObsidianIcon } from '@/components/icons' import { validateExternalUrl } from '@/lib/core/security/input-validation' -import { secureFetchWithRetry, VALIDATE_RETRY_OPTIONS } from '@/lib/knowledge/documents/utils' +import { secureFetchWithRetry } from '@/lib/knowledge/documents/secure-fetch.server' +import { VALIDATE_RETRY_OPTIONS } from '@/lib/knowledge/documents/utils' import type { ConnectorConfig, ExternalDocument, ExternalDocumentList } from '@/connectors/types' import { joinTagArray, parseTagDate } from '@/connectors/utils' diff --git a/apps/sim/connectors/s3/s3.ts b/apps/sim/connectors/s3/s3.ts index db4d0ab85d..677427e25d 100644 --- a/apps/sim/connectors/s3/s3.ts +++ b/apps/sim/connectors/s3/s3.ts @@ -3,7 +3,8 @@ import { createLogger } from '@sim/logger' import { getErrorMessage, toError } from '@sim/utils/errors' import { S3Icon } from '@/components/icons' import { isHosted } from '@/lib/core/config/feature-flags' -import { secureFetchWithRetry, VALIDATE_RETRY_OPTIONS } from '@/lib/knowledge/documents/utils' +import { secureFetchWithRetry } from '@/lib/knowledge/documents/secure-fetch.server' +import { VALIDATE_RETRY_OPTIONS } from '@/lib/knowledge/documents/utils' import type { ConnectorConfig, ExternalDocument, ExternalDocumentList } from '@/connectors/types' import { parseTagDate, readBodyWithLimit } from '@/connectors/utils' import { encodeS3PathComponent, getSignatureKey } from '@/tools/s3/utils' diff --git a/apps/sim/connectors/sentry/sentry.ts b/apps/sim/connectors/sentry/sentry.ts index 3972bc6803..80a108c015 100644 --- a/apps/sim/connectors/sentry/sentry.ts +++ b/apps/sim/connectors/sentry/sentry.ts @@ -1,7 +1,8 @@ import { createLogger } from '@sim/logger' import { getErrorMessage, toError } from '@sim/utils/errors' import { SentryIcon } from '@/components/icons' -import { secureFetchWithRetry, VALIDATE_RETRY_OPTIONS } from '@/lib/knowledge/documents/utils' +import { secureFetchWithRetry } from '@/lib/knowledge/documents/secure-fetch.server' +import { VALIDATE_RETRY_OPTIONS } from '@/lib/knowledge/documents/utils' import type { ConnectorConfig, ExternalDocument, ExternalDocumentList } from '@/connectors/types' import { parseTagDate } from '@/connectors/utils' diff --git a/apps/sim/lib/core/security/empty-node-fallback.browser.ts b/apps/sim/lib/core/security/empty-node-fallback.browser.ts new file mode 100644 index 0000000000..f42bee2675 --- /dev/null +++ b/apps/sim/lib/core/security/empty-node-fallback.browser.ts @@ -0,0 +1,13 @@ +/** + * Browser fallback for Node-only builtins (e.g. `dns/promises`) that get pulled + * into the client bundle by server-only code which never executes in the + * browser — notably the connector registry, whose `ConnectorConfig` objects are + * imported by client UI for metadata while their `listDocuments`/`getDocument` + * fetch logic (which transitively imports `input-validation.server`) only ever + * runs in server API routes. + * + * Wired in via `turbopack.resolveAlias` with the `browser` condition only, so + * the real Node module is still resolved on the server and SSRF validation + * remains fully intact. See `next.config.ts`. + */ +export default {} diff --git a/apps/sim/lib/knowledge/documents/secure-fetch.server.ts b/apps/sim/lib/knowledge/documents/secure-fetch.server.ts new file mode 100644 index 0000000000..98fc82ddbf --- /dev/null +++ b/apps/sim/lib/knowledge/documents/secure-fetch.server.ts @@ -0,0 +1,71 @@ +import { + type SecureFetchOptions, + type SecureFetchResponse, + secureFetchWithValidation, +} from '@/lib/core/security/input-validation.server' +import { + type HTTPError, + isRetryableError, + type RetryOptions, + retryWithExponentialBackoff, +} from '@/lib/knowledge/documents/utils' + +export interface SecureFetchRetryOptions extends RetryOptions { + allowHttp?: boolean + timeout?: number + maxResponseBytes?: number +} + +/** + * SSRF-safe counterpart to {@link fetchWithRetry} for connector requests to + * user-controlled hosts. Every attempt re-runs {@link secureFetchWithValidation} + * (DNS resolution, private/loopback/reserved-IP rejection, IP-pinned connection, + * redirect re-validation); retry/backoff semantics mirror {@link fetchWithRetry}. + * + * Lives in a `.server.ts` module because it pulls in Node-only `dns/promises` + * via {@link secureFetchWithValidation}; importing it from the shared + * `documents/utils` barrel would drag that into client bundles. + */ +export async function secureFetchWithRetry( + url: string, + options: SecureFetchOptions = {}, + retryOptions: SecureFetchRetryOptions = {} +): Promise { + const { allowHttp, timeout, maxResponseBytes, ...retry } = retryOptions + + return retryWithExponentialBackoff(async () => { + const response = await secureFetchWithValidation( + url, + { + ...options, + ...(allowHttp !== undefined ? { allowHttp } : {}), + ...(timeout !== undefined ? { timeout } : {}), + ...(maxResponseBytes !== undefined ? { maxResponseBytes } : {}), + }, + 'url' + ) + + if (!response.ok && isRetryableError({ status: response.status })) { + const errorText = await response.text() + const error: HTTPError = new Error( + `HTTP ${response.status}: ${response.statusText} - ${errorText}` + ) + error.status = response.status + error.statusText = response.statusText + + const retryAfter = response.headers.get('retry-after') + if (retryAfter) { + const waitMs = Number.isNaN(Number(retryAfter)) + ? Math.max(0, new Date(retryAfter).getTime() - Date.now()) + : Number(retryAfter) * 1000 + if (waitMs > 0) { + error.retryAfterMs = waitMs + } + } + + throw error + } + + return response + }, retry) +} diff --git a/apps/sim/lib/knowledge/documents/utils.test.ts b/apps/sim/lib/knowledge/documents/utils.test.ts index a16ba0e643..88e5c87ed2 100644 --- a/apps/sim/lib/knowledge/documents/utils.test.ts +++ b/apps/sim/lib/knowledge/documents/utils.test.ts @@ -11,7 +11,8 @@ vi.mock('@/lib/core/security/input-validation.server', () => ({ secureFetchWithValidation: mockSecureFetchWithValidation, })) -import { isRetryableError, secureFetchWithRetry } from './utils' +import { secureFetchWithRetry } from './secure-fetch.server' +import { isRetryableError } from './utils' /** Builds a minimal SecureFetchResponse-shaped object for tests. */ function fakeResponse( diff --git a/apps/sim/lib/knowledge/documents/utils.ts b/apps/sim/lib/knowledge/documents/utils.ts index ceabd5ac13..ee717bf7a8 100644 --- a/apps/sim/lib/knowledge/documents/utils.ts +++ b/apps/sim/lib/knowledge/documents/utils.ts @@ -2,15 +2,10 @@ import { createLogger } from '@sim/logger' import { toError } from '@sim/utils/errors' import { sleep } from '@sim/utils/helpers' import { randomFloat } from '@sim/utils/random' -import { - type SecureFetchOptions, - type SecureFetchResponse, - secureFetchWithValidation, -} from '@/lib/core/security/input-validation.server' const logger = createLogger('RetryUtils') -interface HTTPError extends Error { +export interface HTTPError extends Error { status?: number statusText?: string retryAfterMs?: number @@ -214,59 +209,3 @@ export async function fetchWithRetry( return response }, retryOptions) } - -export interface SecureFetchRetryOptions extends RetryOptions { - allowHttp?: boolean - timeout?: number - maxResponseBytes?: number -} - -/** - * SSRF-safe counterpart to {@link fetchWithRetry} for connector requests to - * user-controlled hosts. Every attempt re-runs {@link secureFetchWithValidation} - * (DNS resolution, private/loopback/reserved-IP rejection, IP-pinned connection, - * redirect re-validation); retry/backoff semantics mirror {@link fetchWithRetry}. - */ -export async function secureFetchWithRetry( - url: string, - options: SecureFetchOptions = {}, - retryOptions: SecureFetchRetryOptions = {} -): Promise { - const { allowHttp, timeout, maxResponseBytes, ...retry } = retryOptions - - return retryWithExponentialBackoff(async () => { - const response = await secureFetchWithValidation( - url, - { - ...options, - ...(allowHttp !== undefined ? { allowHttp } : {}), - ...(timeout !== undefined ? { timeout } : {}), - ...(maxResponseBytes !== undefined ? { maxResponseBytes } : {}), - }, - 'url' - ) - - if (!response.ok && isRetryableError({ status: response.status })) { - const errorText = await response.text() - const error: HTTPError = new Error( - `HTTP ${response.status}: ${response.statusText} - ${errorText}` - ) - error.status = response.status - error.statusText = response.statusText - - const retryAfter = response.headers.get('retry-after') - if (retryAfter) { - const waitMs = Number.isNaN(Number(retryAfter)) - ? Math.max(0, new Date(retryAfter).getTime() - Date.now()) - : Number(retryAfter) * 1000 - if (waitMs > 0) { - error.retryAfterMs = waitMs - } - } - - throw error - } - - return response - }, retry) -} diff --git a/apps/sim/next.config.ts b/apps/sim/next.config.ts index 30108178e1..d5b81c9da5 100644 --- a/apps/sim/next.config.ts +++ b/apps/sim/next.config.ts @@ -91,6 +91,17 @@ const nextConfig: NextConfig = { './lib/execution/sandbox/bundles/*.cjs', ], }, + turbopack: { + resolveAlias: { + // `dns/promises` has no browser shim. Server-only connector fetch logic + // (which imports `input-validation.server`) is statically reachable from + // the client bundle via the connector registry, but never runs there. + // Stub it for the browser only; the server keeps the real module so SSRF + // validation is unaffected. + 'dns/promises': { browser: './lib/core/security/empty-node-fallback.browser.ts' }, + dns: { browser: './lib/core/security/empty-node-fallback.browser.ts' }, + }, + }, experimental: { optimizeCss: true, preloadEntriesOnStart: false,