Bug: Added proxy route to handle serverside download #1695
Conversation
|
@AshishViradiya153 is attempting to deploy a commit to the mftsio Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughA new API route was introduced to act as a proxy for obtaining presigned S3 GET URLs, handling authentication and forwarding requests to an internal endpoint. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProxyAPI (/api/file/s3/get-presigned-get-url-proxy)
participant InternalAPI (/api/file/s3/get-presigned-get-url)
Client->>ProxyAPI: POST /api/file/s3/get-presigned-get-url-proxy (with key)
ProxyAPI->>ProxyAPI: Authenticate session
ProxyAPI->>InternalAPI: POST /api/file/s3/get-presigned-get-url (with key, internal API key)
InternalAPI-->>ProxyAPI: Presigned URL or error
ProxyAPI-->>Client: Presigned URL or error
sequenceDiagram
participant Server
participant InternalAPI (/api/file/s3/get-presigned-get-url)
Server->>InternalAPI: POST /api/file/s3/get-presigned-get-url (with key, internal API key)
InternalAPI-->>Server: Presigned URL or error
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pages/api/file/s3/get-presigned-get-url-proxy.ts (1)
10-12: ReturnAllowheader with 405 responseRFC-9110 recommends including an
Allowresponse header that enumerates supported methods when responding with405 Method Not Allowed.- if (req.method !== "POST") { - return res.status(405).end("Method Not Allowed"); - } + if (req.method !== "POST") { + res.setHeader("Allow", "POST"); + return res.status(405).end("Method Not Allowed"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/files/get-file.ts(1 hunks)pages/api/file/s3/get-presigned-get-url-proxy.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pages/api/file/s3/get-presigned-get-url-proxy.ts (1)
pages/api/auth/[...nextauth].ts (1)
authOptions(25-189)
🔇 Additional comments (1)
lib/files/get-file.ts (1)
35-41: Same base-URL inconsistency as proxy routeHere we use
NEXT_PUBLIC_BASE_URL; the proxy usesNEXTAUTH_URL. Unify them or resolve via a shared util to prevent subtle deployment bugs.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pages/api/file/s3/get-presigned-get-url-proxy.ts (1)
25-37: 🛠️ Refactor suggestionInconsistent base-URL env-var — risk of 404s in dev
NEXTAUTH_URLis used here while the server-only call inlib/files/get-file.tsrelies onNEXT_PUBLIC_BASE_URL.
The divergence means one of the two environments (local dev / prod) will break unless both vars are set.- const response = await fetch( - `${process.env.NEXTAUTH_URL}/api/file/s3/get-presigned-get-url`, + const base = + process.env.NEXT_PUBLIC_BASE_URL ?? + process.env.NEXTAUTH_URL ?? + ""; + const response = await fetch( + `${base}/api/file/s3/get-presigned-get-url`,Even better: extract this logic into a shared util so both call-sites stay in sync.
🧹 Nitpick comments (3)
pages/api/file/s3/get-presigned-get-url-proxy.ts (1)
38-53: Minor: simplify content-type guard with optional chaining
response.headers.get("content-type")?.includes("application/json")is terser and avoids the explicit null check.
Pure readability—no functional change.🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
lib/files/get-file.ts (2)
31-40: Header type overly narrow
headers: Record<string, string>forbidsHeadersandstring[][]whichfetchaccepts.
Widen toHeadersInitfor flexibility:- headers: Record<string, string>, + headers: HeadersInit,
42-59: Nit: optional-chain for content-type checkStatic analysis flagged this—can be reduced to one line without changing behaviour:
- if (contentType && contentType.includes("application/json")) { + if (contentType?.includes("application/json")) {🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/documents/add-document-modal.tsx(3 hunks)lib/files/get-file.ts(1 hunks)pages/api/file/s3/get-presigned-get-url-proxy.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pages/api/file/s3/get-presigned-get-url-proxy.ts (1)
pages/api/auth/[...nextauth].ts (1)
authOptions(25-189)
🪛 Biome (1.9.4)
lib/files/get-file.ts
[error] 46-46: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
pages/api/file/s3/get-presigned-get-url-proxy.ts
[error] 42-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
components/documents/add-document-modal.tsx (1)
267-269: Good call-out: trigger SWR revalidation after create documentThe extra
mutate(/api/teams/${teamId}/documents)ensures the list cache is refreshed for other views—nice!
If you want to avoid an additional network round-trip when the user is immediately redirected (router.pushtwo lines below), you could optimistically update by passing the new document tomutateand skipping revalidate, but that’s optional.pages/api/file/s3/get-presigned-get-url-proxy.ts (1)
26-36: Leak of internal API key not possible, but strip header on failureIf
process.env.INTERNAL_API_KEYis empty in a mis-configured env,Authorization: "Bearer "will still be sent.
Guard it to avoid sending an empty secret:- Authorization: `Bearer ${process.env.INTERNAL_API_KEY}`, + ...(process.env.INTERNAL_API_KEY + ? { Authorization: `Bearer ${process.env.INTERNAL_API_KEY}` } + : {}),lib/files/get-file.ts (1)
66-69: 👍 Boolean coercion fixed
isServeris now a real boolean via!!process.env.INTERNAL_API_KEY; this prevents accidental string leakage.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores