Skip to content

Commit 4c700cf

Browse files
committed
improvement(executor): make the internal API URL builder explicit
buildAPIUrl picked its base from the path string, the same shape-derived trust the tool transport just dropped. It is now buildInternalApiUrl: always the internal base, and a non-/api/ path throws instead of quietly resolving elsewhere. Ids interpolated into those paths are encoded.
1 parent 9a7ef93 commit 4c700cf

14 files changed

Lines changed: 155 additions & 42 deletions

File tree

apps/sim/executor/handlers/agent/agent-handler.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ vi.mock('@/providers', () => ({
9191

9292
vi.mock('@/executor/utils/http', () => ({
9393
buildAuthHeaders: vi.fn().mockResolvedValue({ 'Content-Type': 'application/json' }),
94-
buildAPIUrl: vi.fn((path: string, params?: Record<string, string>) => {
94+
buildInternalApiUrl: vi.fn((path: string, params?: Record<string, string>) => {
9595
const url = new URL(path, 'http://localhost:3000')
9696
if (params) {
9797
for (const [key, value] of Object.entries(params)) {

apps/sim/executor/handlers/agent/agent-handler.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ import type {
5959
import { parseResponseFormat } from '@/executor/handlers/shared/response-format'
6060
import type { BlockHandler, ExecutionContext, StreamingExecution } from '@/executor/types'
6161
import { collectBlockData } from '@/executor/utils/block-data'
62-
import { buildAPIUrl, buildAuthHeaders } from '@/executor/utils/http'
62+
import { buildAuthHeaders, buildInternalApiUrl } from '@/executor/utils/http'
6363
import { stringifyJSON } from '@/executor/utils/json'
6464
import { projectResolvedSecretDiagnosticContent } from '@/executor/utils/resolved-secret-content-projection'
6565
import { prepareResolvedSecretProjectedInputs } from '@/executor/utils/resolved-secret-input-projection'
@@ -1212,7 +1212,7 @@ export class AgentBlockHandler implements BlockHandler {
12121212
}
12131213

12141214
const headers = await buildAuthHeaders(ctx.userId)
1215-
const url = buildAPIUrl('/api/mcp/tools/discover', {
1215+
const url = buildInternalApiUrl('/api/mcp/tools/discover', {
12161216
serverId,
12171217
workspaceId: ctx.workspaceId,
12181218
workflowId: ctx.workflowId,

apps/sim/executor/handlers/evaluator/evaluator-handler.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,11 @@ import type { BlockOutput } from '@/blocks/types'
1515
import { validateModelProvider } from '@/ee/access-control/utils/permission-check'
1616
import { BlockType, DEFAULTS, EVALUATOR } from '@/executor/constants'
1717
import type { BlockHandler, ExecutionContext } from '@/executor/types'
18-
import { buildAPIUrl, buildAuthHeaders, extractAPIErrorMessage } from '@/executor/utils/http'
18+
import {
19+
buildAuthHeaders,
20+
buildInternalApiUrl,
21+
extractAPIErrorMessage,
22+
} from '@/executor/utils/http'
1923
import { isJSONString, parseJSON, stringifyJSON } from '@/executor/utils/json'
2024
import { projectResolvedSecretDiagnosticError } from '@/executor/utils/resolved-secret-content-projection'
2125
import type {
@@ -186,7 +190,7 @@ export class EvaluatorBlockHandler implements BlockHandler {
186190
}
187191

188192
try {
189-
const url = buildAPIUrl('/api/providers', ctx.userId ? { userId: ctx.userId } : {})
193+
const url = buildInternalApiUrl('/api/providers', ctx.userId ? { userId: ctx.userId } : {})
190194

191195
const providerRequest: ProviderRequest = {
192196
model,

apps/sim/executor/handlers/mothership/mothership-handler.test.ts

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ const PRIVATE_PROVENANCE = {
3232
const {
3333
mockAreModelSafeWorkspaceFileKeys,
3434
mockBuildAuthHeaders,
35-
mockBuildAPIUrl,
35+
mockBuildInternalApiUrl,
3636
mockExtractAPIErrorMessage,
3737
mockGenerateId,
3838
mockIsExecutionCancelled,
@@ -41,7 +41,7 @@ const {
4141
} = vi.hoisted(() => ({
4242
mockAreModelSafeWorkspaceFileKeys: vi.fn(),
4343
mockBuildAuthHeaders: vi.fn(),
44-
mockBuildAPIUrl: vi.fn(),
44+
mockBuildInternalApiUrl: vi.fn(),
4545
mockExtractAPIErrorMessage: vi.fn(),
4646
mockGenerateId: vi.fn(),
4747
mockIsExecutionCancelled: vi.fn(),
@@ -57,7 +57,7 @@ vi.mock('@/lib/uploads/contexts/workspace/workspace-file-secret-provenance', ()
5757

5858
vi.mock('@/executor/utils/http', () => ({
5959
buildAuthHeaders: mockBuildAuthHeaders,
60-
buildAPIUrl: mockBuildAPIUrl,
60+
buildInternalApiUrl: mockBuildInternalApiUrl,
6161
extractAPIErrorMessage: mockExtractAPIErrorMessage,
6262
}))
6363

@@ -155,7 +155,9 @@ describe('MothershipBlockHandler', () => {
155155
vi.stubGlobal('fetch', fetchMock)
156156

157157
mockBuildAuthHeaders.mockResolvedValue({ Authorization: 'Bearer internal' })
158-
mockBuildAPIUrl.mockReturnValue(new URL('/api/mothership/execute', 'http://localhost:3000'))
158+
mockBuildInternalApiUrl.mockReturnValue(
159+
new URL('/api/mothership/execute', 'http://localhost:3000')
160+
)
159161
mockExtractAPIErrorMessage.mockResolvedValue('boom')
160162
mockGenerateId.mockReset()
161163
mockIsExecutionCancelled.mockReset()

apps/sim/executor/handlers/mothership/mothership-handler.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,11 @@ import type {
4242
NormalizedBlockOutput,
4343
StreamingExecution,
4444
} from '@/executor/types'
45-
import { buildAPIUrl, buildAuthHeaders, extractAPIErrorMessage } from '@/executor/utils/http'
45+
import {
46+
buildAuthHeaders,
47+
buildInternalApiUrl,
48+
extractAPIErrorMessage,
49+
} from '@/executor/utils/http'
4650
import type {
4751
ResolvedSecretInputPath,
4852
ResolvedSecretTraceRegistry,
@@ -796,7 +800,7 @@ export class MothershipBlockHandler implements BlockHandler {
796800
requestId
797801
)
798802

799-
const url = buildAPIUrl('/api/mothership/execute')
803+
const url = buildInternalApiUrl('/api/mothership/execute')
800804
const headers = await buildAuthHeaders(ctx.userId)
801805
headers.Accept = 'application/x-ndjson'
802806
headers[MOTHERSHIP_EXECUTE_STREAM_HEADER] = MOTHERSHIP_EXECUTE_STREAM_VALUE

apps/sim/executor/handlers/workflow/workflow-handler.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ vi.mock('@/lib/auth/internal', () => ({
183183

184184
vi.mock('@/executor/utils/http', () => ({
185185
buildAuthHeaders: vi.fn().mockResolvedValue({ 'Content-Type': 'application/json' }),
186-
buildAPIUrl: vi.fn((path: string) => new URL(path, 'http://localhost:3000')),
186+
buildInternalApiUrl: vi.fn((path: string) => new URL(path, 'http://localhost:3000')),
187187
extractAPIErrorMessage: vi.fn(async (response: Response) => {
188188
const defaultMessage = `API request failed with status ${response.status}`
189189
try {

apps/sim/executor/handlers/workflow/workflow-handler.ts

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ import {
4242
type StreamingExecution,
4343
} from '@/executor/types'
4444
import { hasExecutionResult } from '@/executor/utils/errors'
45-
import { buildAPIUrl, buildAuthHeaders } from '@/executor/utils/http'
45+
import { buildAuthHeaders, buildInternalApiUrl } from '@/executor/utils/http'
4646
import { getIterationContext } from '@/executor/utils/iteration-context'
4747
import { parseJSON } from '@/executor/utils/json'
4848
import { lazyCleanupInputMapping } from '@/executor/utils/lazy-cleanup'
@@ -952,7 +952,7 @@ export class WorkflowBlockHandler implements BlockHandler {
952952

953953
private async loadChildWorkflow(workflowId: string, userId?: string) {
954954
const headers = await buildAuthHeaders(userId)
955-
const url = buildAPIUrl(`/api/workflows/${workflowId}`)
955+
const url = buildInternalApiUrl(`/api/workflows/${encodeURIComponent(workflowId)}`)
956956

957957
const response = await fetch(url.toString(), { headers })
958958

@@ -1015,7 +1015,7 @@ export class WorkflowBlockHandler implements BlockHandler {
10151015
private async checkChildDeployment(workflowId: string, userId?: string): Promise<boolean> {
10161016
try {
10171017
const headers = await buildAuthHeaders(userId)
1018-
const url = buildAPIUrl(`/api/workflows/${workflowId}/deployed`)
1018+
const url = buildInternalApiUrl(`/api/workflows/${encodeURIComponent(workflowId)}/deployed`)
10191019

10201020
const response = await fetch(url.toString(), {
10211021
headers,
@@ -1037,7 +1037,9 @@ export class WorkflowBlockHandler implements BlockHandler {
10371037

10381038
private async loadChildWorkflowDeployed(workflowId: string, userId?: string) {
10391039
const headers = await buildAuthHeaders(userId)
1040-
const deployedUrl = buildAPIUrl(`/api/workflows/${workflowId}/deployed`)
1040+
const deployedUrl = buildInternalApiUrl(
1041+
`/api/workflows/${encodeURIComponent(workflowId)}/deployed`
1042+
)
10411043

10421044
const deployedRes = await fetch(deployedUrl.toString(), {
10431045
headers,
@@ -1058,7 +1060,7 @@ export class WorkflowBlockHandler implements BlockHandler {
10581060
throw new Error(`Deployed state missing or invalid for child workflow ${workflowId}`)
10591061
}
10601062

1061-
const metaUrl = buildAPIUrl(`/api/workflows/${workflowId}`)
1063+
const metaUrl = buildInternalApiUrl(`/api/workflows/${encodeURIComponent(workflowId)}`)
10621064
const metaRes = await fetch(metaUrl.toString(), {
10631065
headers,
10641066
cache: 'no-store',
Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { beforeEach, describe, expect, it, vi } from 'vitest'
5+
6+
const { mockGetInternalApiBaseUrl } = vi.hoisted(() => ({
7+
mockGetInternalApiBaseUrl: vi.fn(),
8+
}))
9+
10+
vi.mock('@/lib/core/utils/urls', () => ({
11+
getInternalApiBaseUrl: mockGetInternalApiBaseUrl,
12+
}))
13+
14+
vi.mock('@/lib/auth/internal', () => ({
15+
generateInternalToken: vi.fn().mockResolvedValue('token'),
16+
}))
17+
18+
import { buildInternalApiUrl } from '@/executor/utils/http'
19+
20+
describe('buildInternalApiUrl', () => {
21+
beforeEach(() => {
22+
vi.clearAllMocks()
23+
mockGetInternalApiBaseUrl.mockReturnValue('http://internal.sim.local')
24+
})
25+
26+
it('resolves an internal path against the internal base URL', () => {
27+
const url = buildInternalApiUrl('/api/workflows/wf-1')
28+
29+
expect(url.toString()).toBe('http://internal.sim.local/api/workflows/wf-1')
30+
})
31+
32+
it('appends query params', () => {
33+
const url = buildInternalApiUrl('/api/table/t-1', { workspaceId: 'ws-1' })
34+
35+
expect(url.searchParams.get('workspaceId')).toBe('ws-1')
36+
})
37+
38+
it('rejects a path that is not an internal API route', () => {
39+
expect(() => buildInternalApiUrl('/health')).toThrow(/must start with \/api\//)
40+
})
41+
42+
it('rejects an absolute URL, which would escape the internal base', () => {
43+
expect(() => buildInternalApiUrl('https://attacker.example/api/x')).toThrow(
44+
/must start with \/api\//
45+
)
46+
})
47+
48+
it('keeps a traversal-shaped id inside its own path segment when encoded by the caller', () => {
49+
const id = '../../admin/secrets'
50+
const url = buildInternalApiUrl(`/api/table/${encodeURIComponent(id)}`)
51+
52+
expect(url.pathname).toBe('/api/table/..%2F..%2Fadmin%2Fsecrets')
53+
})
54+
})

apps/sim/executor/utils/http.ts

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { generateInternalToken } from '@/lib/auth/internal'
2-
import { getBaseUrl, getInternalApiBaseUrl } from '@/lib/core/utils/urls'
2+
import { getInternalApiBaseUrl } from '@/lib/core/utils/urls'
33
import { HTTP } from '@/executor/constants'
44

55
export async function buildAuthHeaders(userId?: string): Promise<Record<string, string>> {
@@ -15,9 +15,20 @@ export async function buildAuthHeaders(userId?: string): Promise<Record<string,
1515
return headers
1616
}
1717

18-
export function buildAPIUrl(path: string, params?: Record<string, string>): URL {
19-
const baseUrl = path.startsWith('/api/') ? getInternalApiBaseUrl() : getBaseUrl()
20-
const url = new URL(path, baseUrl)
18+
/**
19+
* Resolves a Sim-internal API path against the internal base URL. Callers pair this with
20+
* {@link buildAuthHeaders}, so the path must be a fixed internal route the caller chose — never a
21+
* caller- or model-supplied URL, and never a value interpolated into the path unencoded.
22+
*
23+
* @throws when `path` is not a relative `/api/` path, which would otherwise send an
24+
* internally-signed request somewhere the caller did not intend.
25+
*/
26+
export function buildInternalApiUrl(path: string, params?: Record<string, string>): URL {
27+
if (!path.startsWith('/api/')) {
28+
throw new Error(`Internal API path must start with /api/: ${path}`)
29+
}
30+
31+
const url = new URL(path, getInternalApiBaseUrl())
2132

2233
if (params) {
2334
for (const [key, value] of Object.entries(params)) {

apps/sim/providers/utils.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,10 +84,10 @@ async function fetchWorkflowMetadata(
8484
workflowId: string
8585
): Promise<{ name: string; description: string | null } | null> {
8686
try {
87-
const { buildAuthHeaders, buildAPIUrl } = await import('@/executor/utils/http')
87+
const { buildAuthHeaders, buildInternalApiUrl } = await import('@/executor/utils/http')
8888

8989
const headers = await buildAuthHeaders()
90-
const url = buildAPIUrl(`/api/workflows/${workflowId}`)
90+
const url = buildInternalApiUrl(`/api/workflows/${encodeURIComponent(workflowId)}`)
9191

9292
const response = await fetch(url.toString(), { headers })
9393
if (!response.ok) {

0 commit comments

Comments
 (0)