Repository navigation
Support file downloads from files table with drive-level access control - #330
Conversation
… messages The file view/download endpoints only supported FILE-type pages from the pages table, but channel attachments are stored in the files table with contentHash as the ID. This caused attachments to remain stuck in skeleton loading state since the API returned 404 for file lookups. Updated both endpoints to: 1. First check if ID matches a FILE-type page (existing behavior) 2. Fall back to files table lookup for channel attachments 3. Verify drive membership for authorization 4. Use createDriveServiceToken for processor authentication https://claude.ai/code/session_0168Mk63SNuLe88fpVhsueaZ
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThese changes refactor the file download and view API routes to consolidate processor interactions into dedicated helper functions, implement dual authentication pathways (FILE-type pages and files table entries), and integrate service token-based authentication for secure processor access. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as File Route
participant DB as Database
participant Auth as Auth Library
participant Proc as Processor Service
Client->>API: GET /files/[id]/download
alt FILE-Type Page Path
API->>DB: Look up page by ID
DB-->>API: Page found (FILE type)
API->>DB: Verify filePath exists
API->>Auth: createPageServiceToken(pageId)
Auth-->>API: Service token
API->>Proc: GET file (token, contentHash)
Proc-->>API: File bytes + metadata
else Files Table Path
API->>DB: Look up page by ID
DB-->>API: Not found or non-FILE type
API->>DB: Query files table
DB-->>API: File entry
API->>DB: Verify drive membership
DB-->>API: Access granted
API->>Auth: createDriveServiceToken(driveId)
Auth-->>API: Service token
API->>Proc: GET file (token, contentHash)
Proc-->>API: File bytes + metadata
else Not Found/Unauthorized
API-->>Client: 404 or 403 error
end
API->>API: Assemble response headers
API-->>Client: Download response with file content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/app/api/files/`[id]/view/route.ts:
- Around line 25-29: The fetch call that builds fileResponse when requesting
`${PROCESSOR_URL}/cache/${contentHash}/original` lacks a timeout and can hang;
update the request to use an AbortSignal with a timeout (e.g.,
AbortSignal.timeout(30000)) and pass the signal in the fetch options (alongside
the existing headers and Authorization `serviceToken`), and handle the
abort/timeout case in the surrounding `fetchAndServeFile` logic (or where
fileResponse is processed) to return an appropriate error/timeout response
instead of hanging.
🧹 Nitpick comments (3)
apps/web/src/app/api/files/[id]/download/route.ts (2)
105-150: Consider extracting shared logic between view and download routes.Both routes have nearly identical code for:
- Looking up FILE-type pages and verifying permissions
- Looking up files and checking drive membership
- Creating service tokens
This duplication could be consolidated into shared helper functions to reduce maintenance burden and ensure consistency.
💡 Suggested approach
Consider creating shared helpers in a common location:
// e.g., in `@/lib/file-access.ts` export async function resolveFileAccess( id: string, userId: string ): Promise< | { type: 'page'; page: Page; contentHash: string } | { type: 'file'; file: File; contentHash: string } | { error: string; status: number } > { // Shared lookup and authorization logic }This would reduce the ~50 lines of duplicated authorization logic in each route to a single function call.
136-142: Same UX issue: usingcontentHashas filename for files table entries.As noted in the view route, using the content hash as the download filename provides poor UX. Users will download files named like
abc123def456rather than meaningful names.apps/web/src/app/api/files/[id]/view/route.ts (1)
145-151: Consider storing original filename in files table to improve downloaded file UX.When serving files from the files table, users download files with hash-based names like
abc123def456. The files table currently lacks an original filename field. Consider addingoriginalNamecolumn to the schema to preserve and serve meaningful filenames, or derive names from related message attachments.
- Add 30s timeout to fetch call in view/route.ts to prevent hanging - Add proper timeout error handling (504 status) to both view and download routes - Accept optional `filename` query parameter for meaningful download filenames - Update ChannelView to pass original filename when viewing/downloading attachments This improves UX by ensuring users download files with their original names (e.g., "document.pdf") instead of content hashes. https://claude.ai/code/session_0168Mk63SNuLe88fpVhsueaZ
2 participants
Summary
Extended the file download and view endpoints to support files stored in the
filestable (channel attachments) in addition to the existingpagestable support. Added drive-level access control for files table entries using drive membership verification.Key Changes
pagestable first (FILE-type pages), then fall back to thefilestable (channel attachments)filestable require the user to be a member of the associated drivecreateDriveServiceTokenfor files table entries instead ofcreatePageServiceTokenfetchAndDownloadFileandfetchAndServeFile)Implementation Details
storagePathoridas the content hash (fallback toidifstoragePathis not set)driveMemberstable with bothdriveIdanduserIdconditionshttps://claude.ai/code/session_0168Mk63SNuLe88fpVhsueaZ
Summary by CodeRabbit
Bug Fixes
Refactor