FIX: API security and client/server implications#67
FIX: API security and client/server implications#67jayleaton wants to merge 2 commits intomichaelshimeles:mainfrom
Conversation
Greptile SummaryThis PR addresses several valid security gaps in the starter kit by introducing Key findings:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Browser as Browser (Client)
participant SA as uploadImageAction<br/>(Server Action)
participant SF as secureFetch<br/>(lib/serverUtils)
participant RH as createRouteHandler<br/>(lib/routeHandler)
participant R2 as Cloudflare R2
participant DB as Database (userImage)
Browser->>SA: FormData (file)
SA->>SF: secureFetch('/api/upload-image', { body: formData })
SF->>SF: auth.api.getSession() — validate session #1
SF-->>SA: throw Error('Unauthorized') if no session
SF->>RH: POST /api/upload-image (with cookie)
RH->>RH: auth.api.getSession() — validate session #2
RH-->>Browser: 401 Unauthorized if no session
RH->>RH: Validate file type & size
RH->>R2: uploadImageAssets(buffer, filename)
R2-->>RH: public URL
RH->>DB: db.insert(userImage) { url, filename, userId }
RH-->>SF: { url }
SF-->>SA: { url }
SA-->>Browser: { url }
|
| return `http://localhost:${process.env.PORT ?? 3000}` | ||
| } |
There was a problem hiding this comment.
localhost fallback will break production
getBaseUrl() falls back to http://localhost:${port} when NEXT_PUBLIC_APP_URL is not set. secureFetch in lib/serverUtils.ts uses this function to construct the URL for all internal API calls (e.g. the upload action). In any production environment where NEXT_PUBLIC_APP_URL is not configured, every server-action-initiated upload will silently attempt an HTTP request to http://localhost:3000/api/upload-image and fail. This would cause all image uploads to break in production with no obvious error surfacing to the developer.
Consider throwing a clear error if the env var is missing in a non-development environment:
| return `http://localhost:${process.env.PORT ?? 3000}` | |
| } | |
| if (process.env.NEXT_PUBLIC_APP_URL) return process.env.NEXT_PUBLIC_APP_URL | |
| if (process.env.NODE_ENV === 'production') { | |
| throw new Error('NEXT_PUBLIC_APP_URL must be set in production') | |
| } | |
| return `http://localhost:${process.env.PORT ?? 3000}` |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/utils.ts
Line: 10-11
Comment:
**`localhost` fallback will break production**
`getBaseUrl()` falls back to `http://localhost:${port}` when `NEXT_PUBLIC_APP_URL` is not set. `secureFetch` in `lib/serverUtils.ts` uses this function to construct the URL for all internal API calls (e.g. the upload action). In any production environment where `NEXT_PUBLIC_APP_URL` is not configured, every server-action-initiated upload will silently attempt an HTTP request to `http://localhost:3000/api/upload-image` and fail. This would cause all image uploads to break in production with no obvious error surfacing to the developer.
Consider throwing a clear error if the env var is missing in a non-development environment:
```suggestion
if (process.env.NEXT_PUBLIC_APP_URL) return process.env.NEXT_PUBLIC_APP_URL
if (process.env.NODE_ENV === 'production') {
throw new Error('NEXT_PUBLIC_APP_URL must be set in production')
}
return `http://localhost:${process.env.PORT ?? 3000}`
```
How can I resolve this? If you propose a fix, please make it concise.| export const config = { | ||
| api: { bodyParser: false }, // Disable default body parsing | ||
| api: { bodyParser: false }, | ||
| }; |
There was a problem hiding this comment.
Pages Router config has no effect in the App Router
export const config = { api: { bodyParser: false } } is a Next.js Pages Router convention for disabling the built-in body parser. In the App Router (i.e. any file under app/), this export is silently ignored — the runtime already handles FormData natively in route handlers without needing this flag. Keeping it risks confusing future developers into thinking it provides some protection or behavior change.
| export const config = { | |
| api: { bodyParser: false }, // Disable default body parsing | |
| api: { bodyParser: false }, | |
| }; | |
| // App Router handles FormData natively; no bodyParser config needed. |
Or simply remove the export entirely.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/api/upload-image/route.ts
Line: 7-9
Comment:
**Pages Router `config` has no effect in the App Router**
`export const config = { api: { bodyParser: false } }` is a Next.js Pages Router convention for disabling the built-in body parser. In the App Router (i.e. any file under `app/`), this export is silently ignored — the runtime already handles `FormData` natively in route handlers without needing this flag. Keeping it risks confusing future developers into thinking it provides some protection or behavior change.
```suggestion
// App Router handles FormData natively; no bodyParser config needed.
```
Or simply remove the export entirely.
How can I resolve this? If you propose a fix, please make it concise.Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
| @@ -32,33 +34,30 @@ export async function POST(req: NextRequest) { | |||
| ); | |||
| } | |||
There was a problem hiding this comment.
SVG files allow stored XSS attacks
image/svg+xml is included in the allowlist. SVG files can contain embedded JavaScript (e.g. <script>alert(1)</script>) that executes when a user opens the file URL directly in a browser. Since uploads are stored on Cloudflare R2 and the URL is returned to any authenticated user, this becomes a stored XSS vector: an authenticated user uploads a malicious SVG, shares the R2 URL, and any victim who opens that link has JavaScript executed in their browser.
Consider removing image/svg+xml from the allowlist:
| const allowedMimeTypes = [ | |
| "image/jpeg", | |
| "image/jpg", | |
| "image/png", | |
| "image/gif", | |
| "image/webp", | |
| ]; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/api/upload-image/route.ts
Line: 21-35
Comment:
**SVG files allow stored XSS attacks**
`image/svg+xml` is included in the allowlist. SVG files can contain embedded JavaScript (e.g. `<script>alert(1)</script>`) that executes when a user opens the file URL directly in a browser. Since uploads are stored on Cloudflare R2 and the URL is returned to any authenticated user, this becomes a stored XSS vector: an authenticated user uploads a malicious SVG, shares the R2 URL, and any victim who opens that link has JavaScript executed in their browser.
Consider removing `image/svg+xml` from the allowlist:
```suggestion
const allowedMimeTypes = [
"image/jpeg",
"image/jpg",
"image/png",
"image/gif",
"image/webp",
];
```
How can I resolve this? If you propose a fix, please make it concise.| const { messages } = await req.json(); | ||
|
|
||
| const result = streamText({ | ||
| model: openai.responses("gpt-4o"), | ||
| messages, | ||
| tools: { | ||
| web_search_preview: openai.tools.webSearchPreview(), | ||
| }, | ||
| }); | ||
| const result = streamText({ | ||
| model: openai.responses("gpt-4o"), | ||
| messages, | ||
| tools: { | ||
| web_search_preview: openai.tools.webSearchPreview(), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
No validation on messages input — cost-escalation risk
messages is extracted from the request body and forwarded directly to streamText without any schema validation. An authenticated user could send:
- An arbitrarily large number of messages
- Extremely long individual message strings
- Malformed message objects that crash the handler
Because each call consumes OpenAI API credits, this opens the door to deliberate cost escalation by any authenticated user. Consider adding a simple guard:
const { messages } = await req.json();
if (!Array.isArray(messages) || messages.length === 0) {
return NextResponse.json({ error: "Invalid messages" }, { status: 400 });
}
if (messages.length > 50) {
return NextResponse.json({ error: "Too many messages" }, { status: 400 });
}Prompt To Fix With AI
This is a comment left during a code review.
Path: app/api/chat/route.ts
Line: 10-18
Comment:
**No validation on `messages` input — cost-escalation risk**
`messages` is extracted from the request body and forwarded directly to `streamText` without any schema validation. An authenticated user could send:
- An arbitrarily large number of messages
- Extremely long individual message strings
- Malformed message objects that crash the handler
Because each call consumes OpenAI API credits, this opens the door to deliberate cost escalation by any authenticated user. Consider adding a simple guard:
```typescript
const { messages } = await req.json();
if (!Array.isArray(messages) || messages.length === 0) {
return NextResponse.json({ error: "Invalid messages" }, { status: 400 });
}
if (messages.length > 50) {
return NextResponse.json({ error: "Too many messages" }, { status: 400 });
}
```
How can I resolve this? If you propose a fix, please make it concise.
PLEASE IMPLEMENT URGENTLY!
This template fails to follow multiple web security best practices. If you have a project using this template, then your API is most likely publicly exposed and compromised.
IMPORTANT:
This repository has not been updated in several months and might no longer be maintained.
IF this PR has not been merged and you are reading this, I can NOT recommend that people use this template.
I have a monorepo template on my GitHub that uses the same stack as this. It has all of these issues resolved and a few other minor issues that this template has missed.
Note
Require authentication for
/api/chatand/api/upload-image, persist uploaded image metadata, and route client uploads through server actions for stronger API securityIntroduce
createRouteHandlerfor authenticated API routes, gate/api/chatand/api/upload-image, adduserImagepersistence, and adduploadImageActionfor server-side uploads; update the settings UI and pages to use these paths and session checks. See lib/routeHandler.ts, app/api/upload-image/route.ts, app/api/chat/route.ts, and app/actions/upload.ts.📍Where to Start
Start with the
createRouteHandlerin lib/routeHandler.ts, then review thePOSThandlers in app/api/upload-image/route.ts and app/api/chat/route.ts, followed byuploadImageActionin app/actions/upload.ts.Macroscope summarized 0d17fb3.