fix(files): address principal serve review findings

Author: TheodoreSpeaks

apps/sim/app/api/files/serve/[...path]/route.test.ts

@@ -94,7 +94,7 @@ vi.mock('@/lib/uploads/contexts/workspace/workspace-file-manager', () => ({
 }))
 
 vi.mock('@/lib/workspace-files/api', () => ({
-  internalSessionOrExecutorAuth: { authenticate: mockAuthenticateWorkspaceFile },
+  internalWorkspaceFileServeAuth: { authenticate: mockAuthenticateWorkspaceFile },
 }))
 
 vi.mock('@/lib/workspace-files/application/read-workspace-file-content-by-key', () => ({

apps/sim/app/api/files/serve/[...path]/route.ts

@@ -19,7 +19,7 @@ import { parseWorkspaceFileKey } from '@/lib/uploads/contexts/workspace/workspac
 import { downloadFile } from '@/lib/uploads/core/storage-service'
 import { resolveServableImageBytes } from '@/lib/uploads/server/image-derivative'
 import { inferContextFromKey } from '@/lib/uploads/utils/file-utils'
-import { internalSessionOrExecutorAuth } from '@/lib/workspace-files/api'
+import { internalWorkspaceFileServeAuth } from '@/lib/workspace-files/api'
 import { readWorkspaceFileContentByKey } from '@/lib/workspace-files/application/read-workspace-file-content-by-key'
 import { verifyFileAccess } from '@/app/api/files/authorization'
 import {
@@ -168,7 +168,7 @@ export const GET = withRouteHandler(
       const storageContext = inferContextFromKey(cloudKey)
       const workspacePrincipal =
         storageContext === 'workspace'
-          ? await internalSessionOrExecutorAuth.authenticate(request, { path })
+          ? await internalWorkspaceFileServeAuth.authenticate(request, { path })
           : undefined
       const legacyAuthResult = workspacePrincipal
         ? undefined

apps/sim/app/api/v1/auth.test.ts

@@ -0,0 +1,86 @@
+/**
+ * @vitest-environment node
+ */
+
+import { NextRequest } from 'next/server'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+const mocks = vi.hoisted(() => ({
+  authenticateApiKey: vi.fn(),
+  updateLastUsed: vi.fn(),
+}))
+
+vi.mock('@/lib/core/config/env-flags', () => ({ isAuthDisabled: false }))
+vi.mock('@/lib/api-key/service', () => ({
+  authenticateApiKeyFromHeader: mocks.authenticateApiKey,
+  updateApiKeyLastUsed: mocks.updateLastUsed,
+}))
+
+import { authenticateV1Request } from '@/app/api/v1/auth'
+
+describe('v1 API key authentication', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+  })
+
+  it('constructs a personal API-key Principal from canonical key identity', async () => {
+    mocks.authenticateApiKey.mockResolvedValue({
+      success: true,
+      userId: 'user-1',
+      keyId: 'key-1',
+      keyType: 'personal',
+    })
+
+    await expect(
+      authenticateV1Request(
+        new NextRequest('http://localhost/api/v1/files', {
+          headers: { 'x-api-key': 'secret' },
+        })
+      )
+    ).resolves.toMatchObject({
+      authenticated: true,
+      principal: { kind: 'personal_api_key', userId: 'user-1', keyId: 'key-1' },
+    })
+  })
+
+  it('constructs a workspace API-key Principal without borrowing the creator identity', async () => {
+    mocks.authenticateApiKey.mockResolvedValue({
+      success: true,
+      userId: 'creator-1',
+      keyId: 'key-1',
+      keyType: 'workspace',
+      workspaceId: 'workspace-1',
+    })
+
+    const result = await authenticateV1Request(
+      new NextRequest('http://localhost/api/v1/files', {
+        headers: { 'x-api-key': 'secret' },
+      })
+    )
+
+    expect(result.principal).toEqual({
+      kind: 'workspace_api_key',
+      workspaceId: 'workspace-1',
+      keyId: 'key-1',
+    })
+    expect(result.principal).not.toHaveProperty('userId')
+  })
+
+  it('fails closed when authenticated key identity is incomplete', async () => {
+    mocks.authenticateApiKey.mockResolvedValue({
+      success: true,
+      userId: 'creator-1',
+      keyId: 'key-1',
+      keyType: 'workspace',
+    })
+
+    await expect(
+      authenticateV1Request(
+        new NextRequest('http://localhost/api/v1/files', {
+          headers: { 'x-api-key': 'secret' },
+        })
+      )
+    ).resolves.toEqual({ authenticated: false, error: 'Authentication failed' })
+    expect(mocks.updateLastUsed).not.toHaveBeenCalled()
+  })
+})

apps/sim/app/api/v1/auth.ts

@@ -1,3 +1,4 @@
+import type { PersonalApiKeyPrincipal, WorkspaceApiKeyPrincipal } from '@sim/auth/principal'
 import { createLogger } from '@sim/logger'
 import type { NextRequest } from 'next/server'
 import { authenticateApiKeyFromHeader, updateApiKeyLastUsed } from '@/lib/api-key/service'
@@ -11,6 +12,7 @@ export interface AuthResult {
   userId?: string
   workspaceId?: string
   keyType?: 'personal' | 'workspace'
+  principal?: PersonalApiKeyPrincipal | WorkspaceApiKeyPrincipal
   error?: string
 }
 
@@ -20,6 +22,11 @@ export async function authenticateV1Request(request: NextRequest): Promise<AuthR
       authenticated: true,
       userId: ANONYMOUS_USER_ID,
       keyType: 'personal',
+      principal: {
+        kind: 'personal_api_key',
+        userId: ANONYMOUS_USER_ID,
+        keyId: 'auth-disabled',
+      },
     }
   }
 
@@ -43,13 +50,35 @@ export async function authenticateV1Request(request: NextRequest): Promise<AuthR
       }
     }
 
-    await updateApiKeyLastUsed(result.keyId!)
+    if (!result.keyId || !result.userId || !result.keyType) {
+      throw new Error('Authenticated v1 API key is missing its canonical identity')
+    }
+    let principal: PersonalApiKeyPrincipal | WorkspaceApiKeyPrincipal
+    if (result.keyType === 'workspace') {
+      if (!result.workspaceId) {
+        throw new Error('Authenticated workspace API key is missing its workspace scope')
+      }
+      principal = {
+        kind: 'workspace_api_key',
+        workspaceId: result.workspaceId,
+        keyId: result.keyId,
+      }
+    } else {
+      principal = {
+        kind: 'personal_api_key',
+        userId: result.userId,
+        keyId: result.keyId,
+      }
+    }
+
+    await updateApiKeyLastUsed(result.keyId)
 
     return {
       authenticated: true,
-      userId: result.userId!,
+      userId: result.userId,
       workspaceId: result.workspaceId,
       keyType: result.keyType,
+      principal,
     }
   } catch (error) {
     logger.error('API key authentication error', { error })

apps/sim/app/api/v1/files/[fileId]/route.test.ts

@@ -8,24 +8,27 @@ const {
   mockCheckRateLimit,
   mockValidateWorkspaceAccess,
   mockGetWorkspaceFile,
-  mockFetchServableWorkspaceFileBuffer,
+  mockDownloadWorkspaceFileStream,
 } = vi.hoisted(() => ({
   mockCheckRateLimit: vi.fn(),
   mockValidateWorkspaceAccess: vi.fn(),
   mockGetWorkspaceFile: vi.fn(),
-  mockFetchServableWorkspaceFileBuffer: vi.fn(),
+  mockDownloadWorkspaceFileStream: vi.fn(),
 }))
 
 vi.mock('@/app/api/v1/middleware', () => ({
   checkRateLimit: mockCheckRateLimit,
   createRateLimitResponse: () => new Response('rate limited', { status: 429 }),
+  requireRateLimitPrincipal: (rateLimit: { principal: unknown }) => rateLimit.principal,
   validateWorkspaceAccess: mockValidateWorkspaceAccess,
   v1ValidationErrorResponse: (e: { issues: unknown[] }) =>
     NextResponse.json({ error: 'Validation error', details: e.issues }, { status: 400 }),
 }))
 vi.mock('@/lib/uploads/contexts/workspace', () => ({
   getWorkspaceFile: mockGetWorkspaceFile,
-  fetchServableWorkspaceFileBuffer: mockFetchServableWorkspaceFileBuffer,
+}))
+vi.mock('@/lib/workspace-files/application/download-workspace-file', () => ({
+  downloadWorkspaceFileStream: { execute: mockDownloadWorkspaceFileStream },
 }))
 vi.mock('@/lib/workspace-files/orchestration', () => ({
   performDeleteWorkspaceFileItems: vi.fn(),
@@ -37,14 +40,19 @@ vi.mock('@sim/audit', () => ({
 }))
 vi.mock('@/lib/posthog/server', () => ({ captureServerEvent: vi.fn() }))
 
-import { DocCompileUserError } from '@/lib/copilot/tools/server/files/doc-compile-error'
+import { OrchestrationError } from '@/lib/core/orchestration/types'
 import { GET } from '@/app/api/v1/files/[fileId]/route'
 
 const WORKSPACE_ID = 'ws-1'
 const FILE_ID = 'file-1'
 const context = { params: Promise.resolve({ fileId: FILE_ID }) }
 
 const DOCX_MIME = 'application/vnd.openxmlformats-officedocument.wordprocessingml.document'
+const PRINCIPAL = {
+  kind: 'personal_api_key' as const,
+  userId: 'user-1',
+  keyId: 'key-1',
+}
 
 function request() {
   return createMockRequest(
@@ -71,16 +79,31 @@ function generatedDocument(name = 'report.docx') {
   }
 }
 
+function renderedDownload(buffer: Buffer) {
+  return {
+    file: generatedDocument(),
+    stream: new ReadableStream<Uint8Array>({
+      start(controller) {
+        controller.enqueue(buffer)
+        controller.close()
+      },
+    }),
+    contentLength: buffer.length,
+    contentType: DOCX_MIME,
+  }
+}
+
 describe('v1 file download', () => {
   beforeEach(() => {
     vi.clearAllMocks()
-    mockCheckRateLimit.mockResolvedValue({ allowed: true, userId: 'user-1' })
+    mockCheckRateLimit.mockResolvedValue({
+      allowed: true,
+      userId: 'user-1',
+      principal: PRINCIPAL,
+    })
     mockValidateWorkspaceAccess.mockResolvedValue(null)
     mockGetWorkspaceFile.mockResolvedValue(generatedDocument())
-    mockFetchServableWorkspaceFileBuffer.mockResolvedValue({
-      buffer: Buffer.from('PKrendered'),
-      contentType: DOCX_MIME,
-    })
+    mockDownloadWorkspaceFileStream.mockResolvedValue(renderedDownload(Buffer.from('PKrendered')))
   })
 
   it('serves the rendered bytes and the rendered content type', async () => {
@@ -104,19 +127,16 @@ describe('v1 file download', () => {
 
   it('reports Content-Length from the rendered bytes, not the declared source size', async () => {
     const rendered = Buffer.alloc(50_000)
-    mockFetchServableWorkspaceFileBuffer.mockResolvedValue({
-      buffer: rendered,
-      contentType: DOCX_MIME,
-    })
+    mockDownloadWorkspaceFileStream.mockResolvedValue(renderedDownload(rendered))
 
     const response = await GET(request(), context)
 
     expect(response.headers.get('Content-Length')).toBe(String(rendered.length))
   })
 
   it('returns a retryable 409 while the artifact is still compiling', async () => {
-    mockFetchServableWorkspaceFileBuffer.mockRejectedValue(
-      new DocCompileUserError('Document is still being generated')
+    mockDownloadWorkspaceFileStream.mockRejectedValue(
+      new OrchestrationError('conflict', 'Document is still being generated')
     )
 
     const response = await GET(request(), context)
@@ -127,11 +147,17 @@ describe('v1 file download', () => {
   })
 
   it('404s a file that does not exist', async () => {
-    mockGetWorkspaceFile.mockResolvedValue(null)
+    mockDownloadWorkspaceFileStream.mockRejectedValue(
+      new OrchestrationError('not_found', 'File not found')
+    )
 
     const response = await GET(request(), context)
 
     expect(response.status).toBe(404)
-    expect(mockFetchServableWorkspaceFileBuffer).not.toHaveBeenCalled()
+    expect(mockDownloadWorkspaceFileStream).toHaveBeenCalledWith({
+      principal: PRINCIPAL,
+      input: { fileId: FILE_ID, assertedWorkspaceId: WORKSPACE_ID },
+      request: expect.anything(),
+    })
   })
 })