Skip to content

Commit 477028d

Browse files
authored
fix(chat): release-audit follow-ups for Project file tabs and sandbox copy parsing (#9169)
* fix(chat): release-audit follow-ups for Project file tabs and sandbox copy parsing * fix(chat): exempt only Project-owned file tabs from the stream workspace guard * fix(chat): read a sandbox copy body once under the cap before routing it Parsing a clone of the live stream hung on an oversized body, because cancelling one teed branch waits for the unread other. The copy body is now buffered once at the entry, and both the protocol check and either copy path read that in-memory request.
1 parent 5f49cbf commit 477028d

6 files changed

Lines changed: 163 additions & 34 deletions

File tree

‎apps/sim/app/workspace/[workspaceId]/home/hooks/stream/handle-resource-event.test.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,22 @@ describe('organization resource stream', () => {
469469
expect(workspace.removeResource).not.toHaveBeenCalled()
470470
expect(mocks.invalidateResourceQueries).not.toHaveBeenCalled()
471471
})
472+
473+
it('opens a Project file tab, which names no workspace, in an organization chat', () => {
474+
const org = makeStreamLoopDeps({ workspaceId: undefined })
475+
const projectFile = {
476+
type: 'file' as const,
477+
id: 'file-1',
478+
title: 'context.md',
479+
owner: { entityType: 'project' as const, entityId: 'project-1' },
480+
}
481+
handleResourceEvent({ deps: org } as StreamLoopContext, {
482+
...removeEvent('file', 'file-1'),
483+
payload: { op: 'upsert', resource: projectFile },
484+
})
485+
expect(org.addResource).toHaveBeenCalledWith(projectFile)
486+
expect(mocks.invalidateResourceQueries).not.toHaveBeenCalled()
487+
})
472488
})
473489

474490
describe('owner-addressed effects on a legacy tab', () => {

‎apps/sim/app/workspace/[workspaceId]/home/hooks/stream/handle-resource-event.ts‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { searchResourceMatchesOwner } from '@/lib/mothership/resources/search'
1010
import {
1111
chatResourceKeyFor,
1212
getChatResourceSelectionId,
13+
isFileResourceType,
1314
mergeChatResource,
1415
} from '@/lib/mothership/resources/types'
1516
import { notifyWorkflowExternalUpdate } from '@/lib/workflows/external-update'
@@ -194,12 +195,19 @@ export function handleResourceEvent(ctx: StreamLoopContext, parsed: ResourceEven
194195
if (owner?.entityType === 'project' && payload.resource.type === 'file' && payload.resource.id)
195196
invalidateProjectFileQueries(queryClient, owner.entityId, payload.resource.id)
196197
const workspaceId = payload.resource.workspaceId ?? chatWorkspaceId
197-
if (!workspaceId || (chatWorkspaceId && workspaceId !== chatWorkspaceId)) return
198+
/** A Project file names no workspace, so an organization chat has none to check it against. */
199+
if (
200+
workspaceId
201+
? chatWorkspaceId && workspaceId !== chatWorkspaceId
202+
: !(owner?.entityType === 'project' && isFileResourceType(payload.resource.type))
203+
)
204+
return
198205
// Browser and terminal tabs are projected from the desktop app's live
199206
// lists, never from the stream; older servers announced them as resources.
200207
if (payload.resource.type === 'browser' || payload.resource.type === 'terminal') return
201208
const resourceType = payload.resource.type
202-
invalidateResourceQueries(queryClient, workspaceId, resourceType, payload.resource.id)
209+
if (workspaceId)
210+
invalidateResourceQueries(queryClient, workspaceId, resourceType, payload.resource.id)
203211
if (resourceType === 'workflow' && payload.op !== 'remove' && payload.resource.id) {
204212
notifyWorkflowExternalUpdate(payload.resource.id)
205213
}
@@ -246,7 +254,7 @@ export function handleResourceEvent(ctx: StreamLoopContext, parsed: ResourceEven
246254
: undefined
247255
const resource: MothershipResource = {
248256
...payload.resource,
249-
...(chatWorkspaceId ? {} : { workspaceId }),
257+
...(!chatWorkspaceId && workspaceId ? { workspaceId } : {}),
250258
type: payload.resource.type as MothershipResourceType,
251259
title:
252260
typeof payload.resource.title === 'string' ? payload.resource.title : payload.resource.id,
@@ -264,7 +272,7 @@ export function handleResourceEvent(ctx: StreamLoopContext, parsed: ResourceEven
264272
current === getChatResourceSelectionId(resource) ? null : current
265273
)
266274
} else removeResource(resource)
267-
if (resourceType === 'workflow') {
275+
if (resourceType === 'workflow' && workspaceId) {
268276
removeWorkflowFromActiveCache(queryClient, workspaceId, resource.id)
269277
}
270278
return
@@ -352,7 +360,7 @@ export function handleResourceEvent(ctx: StreamLoopContext, parsed: ResourceEven
352360
} else onResourceEvent?.(getChatResourceSelectionId(resource))
353361
}
354362

355-
if (resource.type === 'workflow') {
363+
if (resource.type === 'workflow' && workspaceId) {
356364
ensureWorkflowInRegistry(resource.id, resource.title, workspaceId)
357365
}
358366
}
Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
import { queueTableRows, resetDbChainMock } from '@sim/testing/mocks/database.mock'
2+
import { schemaMock } from '@sim/testing/mocks/schema.mock'
3+
import { beforeEach, describe, expect, it } from 'vitest'
4+
import { changeStoredChatResources } from '@/lib/mothership/resources/store'
5+
import type { MothershipResource } from '@/lib/mothership/resources/types'
6+
7+
const unscopedFile: MothershipResource = { type: 'file', id: 'file-1', title: 'notes.md' }
8+
const scopedFile: MothershipResource = { ...unscopedFile, workspaceId: 'ws' }
9+
const table: MothershipResource = { type: 'table', id: 'table-1', title: 'Accounts' }
10+
11+
function storeWorkspaceChat(resources: MothershipResource[]) {
12+
queueTableRows(schemaMock.copilotChats, [{ resources, workspaceId: 'ws' }])
13+
}
14+
15+
describe('changeStoredChatResources reorder', () => {
16+
beforeEach(() => {
17+
resetDbChainMock()
18+
})
19+
20+
it('keeps the reorder when a workspace chat holds a tab both unscoped and scoped', async () => {
21+
storeWorkspaceChat([unscopedFile, scopedFile, table])
22+
23+
const reordered = await changeStoredChatResources('chat-1', {
24+
kind: 'reorder',
25+
resources: [table, unscopedFile, scopedFile],
26+
})
27+
28+
expect(reordered.map((resource) => resource.type)).toEqual(['table', 'file'])
29+
})
30+
31+
it('still rejects an order that drops or invents a resource', async () => {
32+
storeWorkspaceChat([scopedFile, table])
33+
await expect(
34+
changeStoredChatResources('chat-1', { kind: 'reorder', resources: [table, table] })
35+
).rejects.toThrow('Reordered resources must match existing resources')
36+
37+
storeWorkspaceChat([scopedFile, table])
38+
await expect(
39+
changeStoredChatResources('chat-1', {
40+
kind: 'reorder',
41+
resources: [table, { ...unscopedFile, id: 'file-2' }],
42+
})
43+
).rejects.toThrow('Reordered resources must match existing resources')
44+
})
45+
})

‎apps/sim/lib/mothership/resources/store.ts‎

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import {
88
type MothershipResource,
99
type MothershipResourceUpdate,
1010
mergeChatResource,
11+
reorderStoredChatResources,
1112
sanitizeChatResources,
1213
} from '@/lib/mothership/resources/types'
1314

@@ -112,25 +113,20 @@ export async function changeStoredChatResources(
112113
const incoming = sanitizeChatResources(
113114
change.resources.filter((resource) => resource.id !== 'streaming-file')
114115
)
115-
const byKey = new Map(existing.map((resource) => [keyOf(resource), resource]))
116116
if (change.kind === 'reorder') {
117-
const keys = incoming.map(keyOf)
118-
if (
119-
keys.length !== byKey.size ||
120-
new Set(keys).size !== keys.length ||
121-
keys.some((key) => !byKey.has(key))
122-
) {
117+
const reordered = reorderStoredChatResources(
118+
existing,
119+
incoming,
120+
chat.workspaceId ?? undefined
121+
)
122+
if (!reordered)
123123
throw new OrchestrationError(
124124
'validation',
125125
'Reordered resources must match existing resources'
126126
)
127-
}
128-
resources = keys.map((key) => {
129-
const resource = byKey.get(key)
130-
if (!resource) throw new OrchestrationError('validation', 'Resource order changed')
131-
return resource
132-
})
127+
resources = reordered
133128
} else {
129+
const byKey = new Map(existing.map((resource) => [keyOf(resource), resource]))
134130
for (const resource of incoming) {
135131
const key = keyOf(resource)
136132
const previous = byKey.get(key)

‎apps/sim/lib/mothership/tools/sandbox-resource-transport.test.ts‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
import { urlsMockFns } from '@sim/testing/mocks/urls.mock'
77
import { beforeEach, describe, expect, it, vi } from 'vitest'
88
import { copilotRequestPrincipal, isCopilotRequest } from '@/lib/api/server/routes/copilot-request'
9+
import { DEFAULT_MAX_JSON_BODY_BYTES } from '@/lib/api/server/validation'
910
import {
1011
type ContentSource,
1112
noteContentDeliveryOperation,
@@ -465,3 +466,33 @@ it('validates a copy body itself once the lease negotiated the file-owner protoc
465466
expect(response.status).toBe(400)
466467
expect(fetcher).not.toHaveBeenCalled()
467468
})
469+
470+
it.each([undefined, 1 as const])(
471+
'refuses an oversized streamed copy body at the cap without dispatch (protocol %s)',
472+
async (fileOwnerProtocolVersion) => {
473+
readScope.mockResolvedValue({ ...scope, fileOwnerProtocolVersion })
474+
routeMatcher.mockReturnValue({ params: {}, load: async () => ({ POST: fetcher }) })
475+
const projectCopy = new TextEncoder().encode(
476+
JSON.stringify({
477+
source: { owner: workspaceOwner, fileIds: ['file'] },
478+
destination: { owner: { entityType: 'project', entityId: 'project' } },
479+
})
480+
)
481+
const padding = new Uint8Array(1024 * 1024).fill(0x20)
482+
const paddingChunks = Math.ceil((2 * DEFAULT_MAX_JSON_BODY_BYTES) / padding.byteLength)
483+
let pulledChunks = 0
484+
const body = new ReadableStream<Uint8Array>({
485+
pull(controller) {
486+
if (pulledChunks === 0) controller.enqueue(projectCopy)
487+
else if (pulledChunks <= paddingChunks) controller.enqueue(padding)
488+
else controller.close()
489+
pulledChunks++
490+
},
491+
})
492+
const init: RequestInit & { duplex: 'half' } = { method: 'POST', body, duplex: 'half' }
493+
const response = await proxySandboxResourceRequest(request('/api/v2/files/copy', init), token)
494+
expect(response.status).toBe(413)
495+
expect(fetcher).not.toHaveBeenCalled()
496+
expect(pulledChunks).toBeLessThan(paddingChunks)
497+
}
498+
)

‎apps/sim/lib/mothership/tools/sandbox-resource-transport.ts‎

Lines changed: 49 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,18 @@
11
import { createLogger } from '@sim/logger'
22
import { generateId } from '@sim/utils/id'
33
import { NextRequest } from 'next/server'
4-
import { fileCopyInputSchema } from '@/lib/api/contracts/file-copy-input'
54
import { v2CopyFileItemsContract } from '@/lib/api/contracts/v2/file-copy'
65
import { markCopilotRequest } from '@/lib/api/server/routes/copilot-request'
76
import { matchV2Route } from '@/lib/api/server/routes/in-process-transport'
7+
import { V2_PARSE_DEFAULTS } from '@/lib/api/server/routes/v2-json-route'
8+
import { DEFAULT_MAX_JSON_BODY_BYTES, parseRequest } from '@/lib/api/server/validation'
89
import { withWorkspaceInvocationScope } from '@/lib/core/application/workspace-invocation-scope'
910
import { asOrchestrationError, statusForOrchestrationError } from '@/lib/core/orchestration/types'
11+
import {
12+
assertContentLengthWithinLimit,
13+
isPayloadSizeLimitError,
14+
readStreamToBufferWithLimit,
15+
} from '@/lib/core/utils/stream-limits'
1016
import { getInternalApiBaseUrl } from '@/lib/core/utils/urls'
1117
import { createResourceEffectTransport } from '@/lib/mothership/agent-cli/resource-effects'
1218
import { resolveInvocationWorkspace } from '@/lib/mothership/application/workspace-target'
@@ -27,9 +33,10 @@ const logger = createLogger('MothershipSandboxResourceTransport')
2733

2834
/** Private callback observes the real authenticated v2 request without changing its body or API contract. */
2935
export async function proxySandboxResourceRequest(
30-
request: Request,
36+
callback: Request,
3137
token: string
3238
): Promise<Response> {
39+
let request = callback
3340
const url = new URL(request.url)
3441
const prefix = `/api/mothership/sandbox/${token}`
3542
const path = url.pathname.slice(prefix.length)
@@ -51,11 +58,13 @@ export async function proxySandboxResourceRequest(
5158
* owner is refused there; workspace copies and invalid bodies keep the workspace route and its
5259
* v2 validation response.
5360
*/
54-
if (
55-
path === v2CopyFileItemsContract.path &&
56-
(scope.fileOwnerProtocolVersion === 1 || (await copiesBeyondWorkspaces(request)))
57-
)
58-
return proxySandboxFileCopyRequest(request, token, scope)
61+
if (path === v2CopyFileItemsContract.path) {
62+
const buffered = await bufferCopyRequest(request)
63+
if (buffered instanceof Response) return buffered
64+
request = buffered
65+
if (scope.fileOwnerProtocolVersion === 1 || (await copiesBeyondWorkspaces(request)))
66+
return proxySandboxFileCopyRequest(request, token, scope)
67+
}
5968
const ownerHeader = request.headers.get('x-mothership-file-owner')
6069
let owner: FileOperationOwner | undefined
6170
if (ownerHeader !== null) {
@@ -203,18 +212,42 @@ async function proxyAuthorizedSandboxRequest(
203212
})
204213
}
205214

206-
/** A valid copy naming a non-workspace source or destination needs the file-owner protocol. */
207-
async function copiesBeyondWorkspaces(request: Request): Promise<boolean> {
208-
let body: unknown
215+
/**
216+
* Reads a copy body once, under the v2 body cap, so the protocol check and whichever copy path
217+
* handles it each read an in-memory copy. Parsing a clone of the live stream instead would hang on
218+
* an oversized body: cancelling one branch of a teed stream waits for the unread other branch.
219+
*/
220+
async function bufferCopyRequest(request: Request): Promise<Request | Response> {
221+
if (!request.body) return request
209222
try {
210-
body = await request.clone().json()
211-
} catch {
212-
return false
223+
assertContentLengthWithinLimit(request.headers, DEFAULT_MAX_JSON_BODY_BYTES, 'Request body')
224+
const body = await readStreamToBufferWithLimit(request.body, {
225+
maxBytes: DEFAULT_MAX_JSON_BODY_BYTES,
226+
label: 'Request body',
227+
signal: request.signal,
228+
})
229+
return new Request(request, { body: new Uint8Array(body) })
230+
} catch (error) {
231+
if (isPayloadSizeLimitError(error)) return V2_PARSE_DEFAULTS.payloadTooLargeResponse()
232+
throw error
213233
}
214-
const parsed = fileCopyInputSchema.safeParse(body)
234+
}
235+
236+
/**
237+
* A valid copy naming a non-workspace source or destination needs the file-owner protocol. Parsed
238+
* as the copy route parses it: an invalid body stays on the workspace route, which returns its own
239+
* 400.
240+
*/
241+
async function copiesBeyondWorkspaces(request: Request): Promise<boolean> {
242+
const parsed = await parseRequest(
243+
v2CopyFileItemsContract,
244+
new NextRequest(request.clone()),
245+
{},
246+
V2_PARSE_DEFAULTS
247+
)
215248
return (
216249
parsed.success &&
217-
(parsed.data.source.owner.entityType !== 'workspace' ||
218-
parsed.data.destination.owner.entityType !== 'workspace')
250+
(parsed.data.body.source.owner.entityType !== 'workspace' ||
251+
parsed.data.body.destination.owner.entityType !== 'workspace')
219252
)
220253
}

0 commit comments

Comments
 (0)