Skip to content

Commit 80ea4ab

Browse files
committed
fix(uploads): tolerate transient storage probes and reject unowned keys with 403
Follow-ups from a backward-compatibility audit of the attachment-key hardening. The existence probe is hygiene, not authorization — the key-format and no-prior-record guards already carry that, and a binding to a nonexistent object grants nothing readable. But `headObject` rethrows non-404 provider errors, so a transient 5xx or throttle would drop a legitimate attachment. Only a definitive not-found now rejects; a thrown error logs and proceeds on the ownership guards. This path is reached solely by >50MB multipart uploads, the one flow that persists no metadata row at upload time. The stage route mapped an ownership rejection to a 500. It is a client error; return 403 instead.
1 parent dc8c8e5 commit 80ea4ab

4 files changed

Lines changed: 53 additions & 2 deletions

File tree

apps/sim/app/api/mothership/local-files/stage/route.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,10 @@ import {
1313
} from '@/lib/copilot/request/http'
1414
import { encodeVfsSegment } from '@/lib/copilot/vfs/path-utils'
1515
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
16-
import { trackChatUpload } from '@/lib/uploads/contexts/workspace/workspace-file-manager'
16+
import {
17+
trackChatUpload,
18+
WorkspaceFileKeyOwnershipError,
19+
} from '@/lib/uploads/contexts/workspace/workspace-file-manager'
1720
import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
1821

1922
const logger = createLogger('StageLocalFileUploadAPI')
@@ -95,6 +98,13 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
9598
uploadPath: `uploads/${encodeVfsSegment(displayName)}`,
9699
})
97100
} catch (error) {
101+
if (error instanceof WorkspaceFileKeyOwnershipError) {
102+
// The caller supplied a key they may not bind — a client error, not ours.
103+
logger.warn('Rejected chat upload staging for an unowned storage key', {
104+
error: error.message,
105+
})
106+
return NextResponse.json({ error: 'Storage key is not available' }, { status: 403 })
107+
}
98108
logger.error('Failed to stage local file upload', error)
99109
return createInternalServerErrorResponse('Failed to stage local file upload')
100110
}

apps/sim/lib/copilot/chat/payload.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -338,6 +338,8 @@ export async function buildCopilotRequestPayload(
338338
// upload routes that issue these keys already require — reaching the chat
339339
// endpoint with `read` must not confer a file-write capability.
340340
const uploadContexts: Array<{ type: string; content: string; tag?: string; path?: string }> = []
341+
// `PermissionType` is exactly read | write | admin, so this covers the whole
342+
// write-or-better half of the ordering.
341343
const canWriteWorkspaceFiles =
342344
params.userPermission === 'write' || params.userPermission === 'admin'
343345
if (chatId && params.workspaceId && fileAttachments && fileAttachments.length > 0) {

apps/sim/lib/uploads/contexts/workspace/track-chat-upload.test.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -412,6 +412,31 @@ describe('trackChatUpload', () => {
412412
expect(dbChainMockFns.values).not.toHaveBeenCalled()
413413
})
414414

415+
/**
416+
* The probe is hygiene, not authorization — a provider 5xx must not drop a
417+
* legitimate >50MB multipart upload, which is the only path that reaches it.
418+
* Only a definitive not-found (`null`) rejects.
419+
*/
420+
it('proceeds when the storage existence probe throws a transient error', async () => {
421+
queueOwnershipLookup([])
422+
mockHeadObject.mockRejectedValueOnce(new Error('503 SlowDown'))
423+
424+
const result = await trackChatUpload(
425+
WORKSPACE_ID,
426+
USER_ID,
427+
CHAT_ID,
428+
S3_KEY,
429+
'image.png',
430+
'image/png',
431+
1024
432+
)
433+
434+
expect(result).toEqual({ displayName: 'image.png' })
435+
expect(dbChainMockFns.values).toHaveBeenCalledWith(
436+
expect.objectContaining({ key: S3_KEY, context: 'mothership' })
437+
)
438+
})
439+
415440
/** Local-storage deployments have no headObject to consult; ownership still gates. */
416441
it('skips the storage existence probe when cloud storage is not configured', async () => {
417442
mockHasCloudStorage.mockReturnValue(false)

apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -689,7 +689,21 @@ export async function trackChatUpload(
689689
const claimable = await resolveClaimableChatUploadRow(workspaceId, userId, s3Key)
690690

691691
if (claimable.kind === 'insert' && hasCloudStorage()) {
692-
const head = await headObject(s3Key, 'workspace')
692+
// Hygiene only — the format and no-prior-record guards above already carry
693+
// authorization, and a binding to a nonexistent object grants nothing
694+
// readable. So reject only on a definitive not-found (`null`); a provider
695+
// 5xx/throttle throws, and failing the attachment on that would drop a
696+
// legitimate >50MB multipart upload (the sole path reaching this branch).
697+
let head: Awaited<ReturnType<typeof headObject>> = null
698+
try {
699+
head = await headObject(s3Key, 'workspace')
700+
} catch (error) {
701+
logger.warn('Chat upload existence probe failed; proceeding on the ownership guards', {
702+
key: s3Key,
703+
error: getErrorMessage(error),
704+
})
705+
head = { size }
706+
}
693707
if (!head) {
694708
throw new WorkspaceFileKeyOwnershipError(s3Key)
695709
}

0 commit comments

Comments
 (0)