Skip to content

Commit 9017a26

Browse files
committed
fix(security): add SSRF protection to database tools and webhook delivery
1 parent 635179d commit 9017a26

File tree

37 files changed

+273
-101
lines changed

37 files changed

+273
-101
lines changed

apps/sim/app/api/chat/utils.test.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,17 @@ vi.mock('@/lib/core/config/feature-flags', () => ({
5151
isProd: false,
5252
}))
5353

54+
vi.mock('@/lib/core/config/env', () => ({
55+
getEnv: vi.fn((key: string) => {
56+
if (key === 'NEXT_PUBLIC_APP_URL') return 'http://localhost:3000'
57+
return undefined
58+
}),
59+
}))
60+
61+
vi.mock('@/lib/core/utils/urls', () => ({
62+
getBaseUrl: vi.fn(() => 'http://localhost:3000'),
63+
}))
64+
5465
vi.mock('@/lib/workflows/utils', () => ({
5566
authorizeWorkflowByWorkspacePermission: vi.fn(),
5667
}))

apps/sim/app/api/form/utils.test.ts

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,17 @@ vi.mock('@/lib/core/config/feature-flags', () => ({
2424
isProd: false,
2525
}))
2626

27+
vi.mock('@/lib/core/config/env', () => ({
28+
getEnv: vi.fn((key: string) => {
29+
if (key === 'NEXT_PUBLIC_APP_URL') return 'http://localhost:3000'
30+
return undefined
31+
}),
32+
}))
33+
34+
vi.mock('@/lib/core/utils/urls', () => ({
35+
getBaseUrl: vi.fn(() => 'http://localhost:3000'),
36+
}))
37+
2738
vi.mock('@/lib/workflows/utils', () => ({
2839
authorizeWorkflowByWorkspacePermission: vi.fn(),
2940
}))
@@ -114,7 +125,7 @@ describe('Form API Utils', () => {
114125
})
115126

116127
describe('CORS handling', () => {
117-
it.concurrent('should add CORS headers for any origin', () => {
128+
it('should add CORS headers for allowed origins', () => {
118129
const mockRequest = {
119130
headers: {
120131
get: vi.fn().mockReturnValue('http://localhost:3000'),
@@ -147,7 +158,25 @@ describe('Form API Utils', () => {
147158
)
148159
})
149160

150-
it.concurrent('should not set CORS headers when no origin', () => {
161+
it('should not set CORS headers for disallowed origins', () => {
162+
const mockRequest = {
163+
headers: {
164+
get: vi.fn().mockReturnValue('https://evil.com'),
165+
},
166+
} as any
167+
168+
const mockResponse = {
169+
headers: {
170+
set: vi.fn(),
171+
},
172+
} as unknown as NextResponse
173+
174+
addCorsHeaders(mockResponse, mockRequest)
175+
176+
expect(mockResponse.headers.set).not.toHaveBeenCalled()
177+
})
178+
179+
it('should not set CORS headers when no origin', () => {
151180
const mockRequest = {
152181
headers: {
153182
get: vi.fn().mockReturnValue(''),

apps/sim/app/api/mcp/servers/[id]/refresh/route.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,8 @@ export const POST = withMcpAuth<{ id: string }>('read')(
192192
)
193193
} catch (error) {
194194
connectionStatus = 'error'
195-
lastError = error instanceof Error ? error.message : 'Connection test failed'
195+
lastError =
196+
error instanceof Error ? error.message.split('\n')[0].slice(0, 200) : 'Connection failed'
196197
logger.warn(`[${requestId}] Failed to connect to server ${serverId}:`, error)
197198
}
198199

apps/sim/app/api/mcp/servers/test-connection/route.ts

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,26 @@ interface TestConnectionResult {
4141
warnings?: string[]
4242
}
4343

44+
/**
45+
* Extracts a user-friendly error message from connection errors.
46+
* Keeps diagnostic info (timeout, DNS, HTTP status) but strips
47+
* verbose internals (Zod details, full response bodies, stack traces).
48+
*/
49+
function sanitizeConnectionError(error: unknown): string {
50+
if (!(error instanceof Error)) {
51+
return 'Unknown connection error'
52+
}
53+
54+
const msg = error.message
55+
56+
if (msg.length > 200) {
57+
const firstLine = msg.split('\n')[0]
58+
return firstLine.length > 200 ? `${firstLine.slice(0, 200)}...` : firstLine
59+
}
60+
61+
return msg
62+
}
63+
4464
/**
4565
* POST - Test connection to an MCP server before registering it
4666
*/
@@ -137,8 +157,7 @@ export const POST = withMcpAuth('write')(
137157
} catch (toolError) {
138158
logger.warn(`[${requestId}] Connection established but could not list tools:`, toolError)
139159
result.success = false
140-
const errorMessage = toolError instanceof Error ? toolError.message : 'Unknown error'
141-
result.error = `Connection established but could not list tools: ${errorMessage}`
160+
result.error = 'Connection established but could not list tools'
142161
result.warnings = result.warnings || []
143162
result.warnings.push(
144163
'Server connected but tool listing failed - connection may be incomplete'
@@ -163,11 +182,7 @@ export const POST = withMcpAuth('write')(
163182
logger.warn(`[${requestId}] MCP server test failed:`, error)
164183

165184
result.success = false
166-
if (error instanceof Error) {
167-
result.error = error.message
168-
} else {
169-
result.error = 'Unknown connection error'
170-
}
185+
result.error = sanitizeConnectionError(error)
171186
} finally {
172187
if (client) {
173188
try {

apps/sim/app/api/mcp/tools/execute/route.ts

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -89,11 +89,12 @@ export const POST = withMcpAuth('read')(
8989
tool = tools.find((t) => t.name === toolName) ?? null
9090

9191
if (!tool) {
92+
logger.warn(`[${requestId}] Tool ${toolName} not found on server ${serverId}`, {
93+
availableTools: tools.map((t) => t.name),
94+
})
9295
return createMcpErrorResponse(
93-
new Error(
94-
`Tool ${toolName} not found on server ${serverId}. Available tools: ${tools.map((t) => t.name).join(', ')}`
95-
),
96-
'Tool not found',
96+
new Error('Tool not found'),
97+
'Tool not found on the specified server',
9798
404
9899
)
99100
}

apps/sim/app/api/tools/a2a/cancel-task/route.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ export async function POST(request: NextRequest) {
7676
return NextResponse.json(
7777
{
7878
success: false,
79-
error: error instanceof Error ? error.message : 'Failed to cancel task',
79+
error: 'Failed to cancel task',
8080
},
8181
{ status: 500 }
8282
)

apps/sim/app/api/tools/a2a/delete-push-notification/route.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ export async function POST(request: NextRequest) {
8686
return NextResponse.json(
8787
{
8888
success: false,
89-
error: error instanceof Error ? error.message : 'Failed to delete push notification',
89+
error: 'Failed to delete push notification',
9090
},
9191
{ status: 500 }
9292
)

apps/sim/app/api/tools/a2a/get-agent-card/route.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ export async function POST(request: NextRequest) {
8484
return NextResponse.json(
8585
{
8686
success: false,
87-
error: error instanceof Error ? error.message : 'Failed to fetch Agent Card',
87+
error: 'Failed to fetch Agent Card',
8888
},
8989
{ status: 500 }
9090
)

apps/sim/app/api/tools/a2a/get-push-notification/route.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ export async function POST(request: NextRequest) {
107107
return NextResponse.json(
108108
{
109109
success: false,
110-
error: error instanceof Error ? error.message : 'Failed to get push notification',
110+
error: 'Failed to get push notification',
111111
},
112112
{ status: 500 }
113113
)

apps/sim/app/api/tools/a2a/get-task/route.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ export async function POST(request: NextRequest) {
8787
return NextResponse.json(
8888
{
8989
success: false,
90-
error: error instanceof Error ? error.message : 'Failed to get task',
90+
error: 'Failed to get task',
9191
},
9292
{ status: 500 }
9393
)

0 commit comments

Comments
 (0)