Skip to content

Commit 746413a

Browse files
committed
fix(desktop): harden the raw upload routes now that they skip the proxy
- The desktop import and browser file routes refuse the dedicated MCP host themselves, as the proxy did before they left its matcher. - The import's execution token travels in a header, not the query string, so it stays out of load balancer, CDN and trace URLs. - A name that trims to nothing or to a dot segment is refused at the contract and, for the whole import, before anything lands, through one shared rule. Such a name no longer surfaces as a 500 part way through. - The browser download route also requires a declared length and refuses a body that does not match it. - Route tests cover 411, 413, a length mismatch, an entry that is not admitted reading no body, the 401 mappings, the missing token header and an unstorable name.
1 parent b64fa6e commit 746413a

13 files changed

Lines changed: 297 additions & 24 deletions

File tree

‎apps/desktop/e2e/background-executor.spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,7 @@ class FixtureSim {
197197
!call ||
198198
call.deviceId !== query.deviceId ||
199199
call.toolName !== 'import_local_files' ||
200-
call.token !== query.executionToken ||
200+
call.token !== request.headers['x-sim-execution-token'] ||
201201
call.status !== 'running'
202202
) {
203203
this.json(response, 404, { error: 'Desktop import not found' })

‎apps/desktop/src/main/desktop-executor/client.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
* own session cookie, which is the session the device registered under; Sim refuses any other.
44
*/
55

6+
import { DESKTOP_IMPORT_TOKEN_HEADER } from '@sim/desktop-bridge'
67
import { getErrorMessage } from '@sim/utils/errors'
78
import { parseRetryAfter } from '@sim/utils/retry'
89
import { truncateAtCodePoint } from '@sim/utils/string'
@@ -110,7 +111,8 @@ export function createDesktopExecutorClient(
110111
path: string,
111112
body?: Record<string, unknown> | Blob,
112113
signal?: AbortSignal,
113-
timeoutMs = REQUEST_TIMEOUT_MS
114+
timeoutMs = REQUEST_TIMEOUT_MS,
115+
extraHeaders: Record<string, string> = {}
114116
): Promise<Response> {
115117
const timeout = AbortSignal.timeout(timeoutMs)
116118
const raw = body instanceof Blob
@@ -125,6 +127,7 @@ export function createDesktopExecutorClient(
125127
...(body
126128
? { 'Content-Type': raw ? 'application/octet-stream' : 'application/json' }
127129
: {}),
130+
...extraHeaders,
128131
},
129132
...(encoded !== undefined ? { body: encoded } : {}),
130133
signal: signal ? AbortSignal.any([signal, timeout]) : timeout,
@@ -221,7 +224,6 @@ export function createDesktopExecutorClient(
221224
const query = new URLSearchParams({
222225
deviceId,
223226
toolCallId: call.toolCallId,
224-
executionToken: call.executionToken,
225227
kind,
226228
sourceName,
227229
relativePath,
@@ -231,7 +233,8 @@ export function createDesktopExecutorClient(
231233
`/api/desktop/tool/import?${query}`,
232234
content,
233235
signal,
234-
IMPORT_TIMEOUT_MS
236+
IMPORT_TIMEOUT_MS,
237+
{ [DESKTOP_IMPORT_TOKEN_HEADER]: call.executionToken }
235238
)
236239
const entry = parseImportedEntry(await response.json().catch(() => null))
237240
if (!entry) throw malformed('import')

‎apps/sim/app/api/desktop/tool/file/route.test.ts‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,4 +87,26 @@ describe('/api/desktop/tool/file', () => {
8787
expect(request.bodyUsed).toBe(false)
8888
expect(mockSave).not.toHaveBeenCalled()
8989
})
90+
91+
it('saves nothing from a download that does not match its declared length', async () => {
92+
const res = await PUT(
93+
put('toolCallId=call-2&name=report.csv', new Uint8Array([1, 2, 3]), {
94+
'content-length': '10',
95+
})
96+
)
97+
98+
expect(res.status).toBe(400)
99+
expect(mockSave).not.toHaveBeenCalled()
100+
})
101+
102+
it('refuses a download that does not declare its length', async () => {
103+
const request = new NextRequest(`${URL_BASE}?toolCallId=call-3&name=a.csv`, {
104+
method: 'PUT',
105+
body: new ReadableStream({ start: (controller) => controller.close() }),
106+
duplex: 'half',
107+
})
108+
109+
expect((await PUT(request)).status).toBe(411)
110+
expect(mockSave).not.toHaveBeenCalled()
111+
})
90112
})

‎apps/sim/app/api/desktop/tool/file/route.ts‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {
44
readBrowserUploadFileContract,
55
saveBrowserDownloadContract,
66
} from '@/lib/api/contracts/desktop-browser-files'
7+
import { isOffAppHost } from '@/lib/api/mcp/host-routing'
78
import { parseRequest } from '@/lib/api/server'
89
import {
910
defineInternalBinaryRoute,
@@ -31,7 +32,7 @@ export const dynamic = 'force-dynamic'
3132
* The desktop main process fetches one file a claimed `browser_upload_file` call attaches to a
3233
* page. The call's persisted arguments name the file; the request only picks which of them.
3334
*/
34-
export const POST = defineInternalBinaryRoute({
35+
const readUploadFile = defineInternalBinaryRoute({
3536
contract: readBrowserUploadFileContract,
3637
auth: internalSessionAuth,
3738
operation: readBrowserUploadFile.operation,
@@ -49,6 +50,14 @@ export const POST = defineInternalBinaryRoute({
4950
}),
5051
})
5152

53+
/** Off the proxy (see `proxy.ts`), so the dedicated MCP host is refused here, as the proxy would. */
54+
function notFoundOffAppHost(): NextResponse {
55+
return NextResponse.json(withRequestId({ error: 'Not found' }), { status: 404 })
56+
}
57+
58+
export const POST: typeof readUploadFile = (request, context) =>
59+
isOffAppHost(request) ? Promise.resolve(notFoundOffAppHost()) : readUploadFile(request, context)
60+
5261
/**
5362
* PUT /api/desktop/tool/file?toolCallId=…&name=…
5463
*
@@ -58,6 +67,7 @@ export const POST = defineInternalBinaryRoute({
5867
* re-validates and claims that call.
5968
*/
6069
export const PUT = withRouteHandler(async (request: NextRequest) => {
70+
if (isOffAppHost(request)) return notFoundOffAppHost()
6171
let principal
6272
try {
6373
principal = await internalSessionAuth.authenticate()
@@ -67,7 +77,13 @@ export const PUT = withRouteHandler(async (request: NextRequest) => {
6777
}
6878
throw error
6979
}
70-
const declaredLength = Number(request.headers.get('content-length'))
80+
const lengthHeader = request.headers.get('content-length')
81+
if (lengthHeader === null) {
82+
return NextResponse.json(withRequestId({ error: 'A download must declare its length' }), {
83+
status: 411,
84+
})
85+
}
86+
const declaredLength = Number(lengthHeader)
7187
if (!Number.isFinite(declaredLength) || declaredLength > BROWSER_FILE_TRANSFER_MAX_BYTES) {
7288
return NextResponse.json(withRequestId({ error: 'Download is too large to save' }), {
7389
status: 413,
@@ -98,6 +114,13 @@ export const PUT = withRouteHandler(async (request: NextRequest) => {
98114
}
99115
throw error
100116
}
117+
// Anything between the device and here that cut the body short must not become a saved file.
118+
if (content.length !== declaredLength) {
119+
return NextResponse.json(
120+
withRequestId({ error: 'The download did not arrive whole; nothing was saved' }),
121+
{ status: 400 }
122+
)
123+
}
101124

102125
try {
103126
const { file } = await saveBrowserDownload.execute({
Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
import { authMockFns } from '@sim/testing'
2+
import { NextRequest } from 'next/server'
3+
import { beforeEach, describe, expect, it, vi } from 'vitest'
4+
import { DesktopDeviceUnrecognizedError } from '@/lib/desktop/executor/errors'
5+
6+
const { admitted, stored, admitError } = vi.hoisted(() => ({
7+
/** Entries the route admitted, in order. */
8+
admitted: [] as Array<Record<string, unknown>>,
9+
/** Entries the route stored, with how many bytes each file carried. */
10+
stored: [] as Array<{ executionToken: unknown; bytes: number | null }>,
11+
admitError: { next: null as Error | null },
12+
}))
13+
14+
vi.mock('@/lib/desktop/application/import', () => ({
15+
admitDesktopImportEntry: async (_principal: unknown, input: Record<string, unknown>) => {
16+
const error = admitError.next
17+
admitError.next = null
18+
if (error) throw error
19+
admitted.push(input)
20+
},
21+
importDesktopEntry: {
22+
execute: async ({ input }: { input: { executionToken: unknown; content?: Buffer } }) => {
23+
stored.push({
24+
executionToken: input.executionToken,
25+
bytes: input.content ? input.content.length : null,
26+
})
27+
return { id: 'file-1', name: 'notes.txt' }
28+
},
29+
},
30+
}))
31+
32+
import { PUT } from '@/app/api/desktop/tool/import/route'
33+
34+
const DEVICE = '00000000-0000-4000-8000-000000000001'
35+
const QUERY = `deviceId=${DEVICE}&toolCallId=call-1&kind=file&sourceName=Reports&relativePath=notes.txt`
36+
37+
/** A request whose body counts how much of it the route read. */
38+
function put(options: {
39+
body?: Uint8Array
40+
length?: string | null
41+
token?: string | null
42+
host?: string
43+
query?: string
44+
}) {
45+
const body = options.body ?? new TextEncoder().encode('hello')
46+
let pulled = 0
47+
const stream = new ReadableStream<Uint8Array>(
48+
{
49+
pull(controller) {
50+
pulled += 1
51+
controller.enqueue(body)
52+
controller.close()
53+
},
54+
},
55+
// Pulled only when the route reads it, never ahead of time.
56+
{ highWaterMark: 0 }
57+
)
58+
const headers: Record<string, string> = {}
59+
const length = options.length === undefined ? String(body.byteLength) : options.length
60+
if (length !== null) headers['content-length'] = length
61+
if (options.token !== null) headers['x-sim-execution-token'] = options.token ?? 'token-1'
62+
if (options.host) headers.host = options.host
63+
const request = new NextRequest(
64+
`http://${options.host ?? 'localhost'}/api/desktop/tool/import?${options.query ?? QUERY}`,
65+
{ method: 'PUT', headers, body: stream, duplex: 'half' }
66+
)
67+
return { request, pulled: () => pulled }
68+
}
69+
70+
describe('PUT /api/desktop/tool/import', () => {
71+
beforeEach(() => {
72+
authMockFns.mockGetSession.mockResolvedValue({
73+
user: { id: 'u1' },
74+
session: { id: 's1' },
75+
})
76+
admitted.length = 0
77+
stored.length = 0
78+
admitError.next = null
79+
})
80+
81+
it('stores a whole file under the claim the token header names', async () => {
82+
const { request } = put({})
83+
84+
const response = await PUT(request, {})
85+
86+
expect(response.status).toBe(200)
87+
expect(stored).toEqual([{ executionToken: 'token-1', bytes: 5 }])
88+
})
89+
90+
it('refuses a body that does not match its declared length, storing nothing', async () => {
91+
const { request } = put({ length: '10' })
92+
93+
const response = await PUT(request, {})
94+
95+
expect(response.status).toBe(400)
96+
expect(stored).toEqual([])
97+
})
98+
99+
it('refuses a file that does not declare its length', async () => {
100+
const { request } = put({ length: null })
101+
102+
expect((await PUT(request, {})).status).toBe(411)
103+
expect(stored).toEqual([])
104+
})
105+
106+
it('refuses a file over the import limit before admitting it', async () => {
107+
const { request, pulled } = put({ length: String(65 * 1024 * 1024) })
108+
109+
expect((await PUT(request, {})).status).toBe(413)
110+
expect(admitted).toEqual([])
111+
expect(pulled()).toBe(0)
112+
})
113+
114+
it('reads no body for an entry it does not admit', async () => {
115+
admitError.next = new DesktopDeviceUnrecognizedError()
116+
const { request, pulled } = put({})
117+
118+
const response = await PUT(request, {})
119+
120+
expect(response.status).toBe(401)
121+
expect(pulled()).toBe(0)
122+
expect(stored).toEqual([])
123+
})
124+
125+
it('answers an unauthenticated request with 401', async () => {
126+
authMockFns.mockGetSession.mockResolvedValueOnce(null)
127+
const { request } = put({})
128+
129+
expect((await PUT(request, {})).status).toBe(401)
130+
expect(admitted).toEqual([])
131+
})
132+
133+
it('refuses a request without the execution token header', async () => {
134+
const { request } = put({ token: null })
135+
136+
expect((await PUT(request, {})).status).toBe(400)
137+
expect(admitted).toEqual([])
138+
})
139+
140+
it('refuses a name a workspace cannot store before admitting it', async () => {
141+
const { request } = put({
142+
query: `deviceId=${DEVICE}&toolCallId=call-1&kind=file&sourceName=Reports&relativePath=${encodeURIComponent('.. /notes.txt')}`,
143+
})
144+
145+
expect((await PUT(request, {})).status).toBe(400)
146+
expect(admitted).toEqual([])
147+
})
148+
})

‎apps/sim/app/api/desktop/tool/import/route.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
1-
import { MAX_DESKTOP_IMPORT_FILE_BYTES } from '@sim/desktop-bridge'
1+
import { DESKTOP_IMPORT_TOKEN_HEADER, MAX_DESKTOP_IMPORT_FILE_BYTES } from '@sim/desktop-bridge'
22
import { type NextRequest, NextResponse } from 'next/server'
33
import { importDesktopEntryContract } from '@/lib/api/contracts/desktop-executor'
4+
import { isOffAppHost } from '@/lib/api/mcp/host-routing'
45
import { parseRequest } from '@/lib/api/server'
56
import { desktopExecutorRateLimit } from '@/lib/api/server/routes/desktop-executor'
67
import {
@@ -26,6 +27,8 @@ const TOO_LARGE = `Desktop imports support files up to ${MAX_DESKTOP_IMPORT_FILE
2627
* handed to the use case that re-validates the claim and stores it.
2728
*/
2829
export const PUT = withRouteHandler(async (request: NextRequest) => {
30+
if (isOffAppHost(request))
31+
return NextResponse.json(withRequestId({ error: 'Not found' }), { status: 404 })
2932
let principal
3033
try {
3134
principal = await internalSessionAuth.authenticate()
@@ -39,7 +42,10 @@ export const PUT = withRouteHandler(async (request: NextRequest) => {
3942
if (limited) return limited
4043
const parsed = await parseRequest(importDesktopEntryContract, request, {})
4144
if (!parsed.success) return parsed.response
42-
const query = parsed.data.query
45+
const query = {
46+
...parsed.data.query,
47+
executionToken: parsed.data.headers[DESKTOP_IMPORT_TOKEN_HEADER],
48+
}
4349
const lengthHeader = request.headers.get('content-length')
4450
const declaredLength = Number(lengthHeader ?? 0)
4551
if (!Number.isFinite(declaredLength) || declaredLength > MAX_DESKTOP_IMPORT_FILE_BYTES) {

‎apps/sim/lib/api/contracts/desktop-executor.ts‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { DESKTOP_IMPORT_TOKEN_HEADER, isStorableImportName } from '@sim/desktop-bridge'
12
import { z } from 'zod'
23
import { desktopToolCallIdSchema } from '@/lib/api/contracts/desktop-tool-authorization'
34
import { defineRouteContract } from '@/lib/api/contracts/types'
@@ -183,8 +184,9 @@ export const completeDesktopToolContract = defineRouteContract({
183184
error: z.object({ error: z.string() }),
184185
})
185186

186-
/** Workspace file names cannot hold a backslash, which a macOS or Linux file name can. */
187-
const BACKSLASH_NAME = 'Sim cannot store a file or folder whose name contains a backslash'
187+
/** Names a workspace cannot hold, though a macOS or Linux file name can be any of them. */
188+
const UNSTORABLE_NAME =
189+
'Sim cannot store a file or folder whose name is blank, "." or "..", or contains a backslash'
188190

189191
/** One relative path inside an import source, as the device's manifest lists it. */
190192
const desktopImportRelativePathSchema = z
@@ -196,21 +198,19 @@ const desktopImportRelativePathSchema = z
196198
path.split('/').every((segment) => segment !== '' && segment !== '.' && segment !== '..'),
197199
'Relative path must stay inside the import source'
198200
)
199-
.refine((path) => !path.includes('\\'), BACKSLASH_NAME)
201+
.refine((path) => path === '' || path.split('/').every(isStorableImportName), UNSTORABLE_NAME)
200202

201203
const importDesktopEntryQuerySchema = z.object({
202204
deviceId: desktopDeviceIdSchema,
203205
toolCallId: desktopToolCallIdSchema,
204-
executionToken: z.string().min(1).max(128),
205206
kind: z.enum(['file', 'directory']),
206207
/** The import source's own name: the folder a directory import lands in, or the file. */
207208
sourceName: z
208209
.string()
209210
.trim()
210211
.min(1, 'Source name is required')
211212
.max(255)
212-
.refine((name) => !name.includes('/'), 'Source name must be a single name')
213-
.refine((name) => !name.includes('\\'), BACKSLASH_NAME),
213+
.refine(isStorableImportName, UNSTORABLE_NAME),
214214
relativePath: desktopImportRelativePathSchema,
215215
})
216216

@@ -228,6 +228,7 @@ export const importDesktopEntryContract = defineRouteContract({
228228
method: 'PUT',
229229
path: '/api/desktop/tool/import',
230230
query: importDesktopEntryQuerySchema,
231+
headers: z.object({ [DESKTOP_IMPORT_TOKEN_HEADER]: z.string().min(1).max(128) }),
231232
response: { mode: 'json', schema: importDesktopEntryResponseSchema },
232233
error: z.object({ error: z.string() }),
233234
})

0 commit comments

Comments
 (0)