Harden the starter for production-minded use - #1
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the starter template to Next.js 16 and React 19, introducing comprehensive security headers, environment variable validation with Zod, Sentry error handling, and a secure direct-to-R2 upload flow. It also adds health/readiness endpoints, database migrations, and repository tests. However, several critical issues were identified during the review: renaming middleware.ts to proxy.ts breaks Next.js middleware execution, the auth callback redirect validation is vulnerable to an Open Redirect attack, malformed JSON in the upload API returns a 500 error instead of a 400, and the upload component lacks cleanup on unmount. Additionally, HF_TOKEN should be integrated into the Zod environment schema for consistency.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| export async function proxy(request: NextRequest) { | ||
| return updateSession(request) | ||
| } |
There was a problem hiding this comment.
Next.js strictly requires the middleware file to be named middleware.ts (or middleware.js) and located in the root or src directory, with a default or named export of middleware.\n\nRenaming this file to proxy.ts and exporting proxy will cause Next.js to completely ignore it. As a result, the Supabase session update/refresh logic in updateSession(request) will never execute, causing user sessions to expire and fail to refresh.\n\nPlease rename this file back to middleware.ts and export middleware instead of proxy.
| export async function proxy(request: NextRequest) { | |
| return updateSession(request) | |
| } | |
| export async function middleware(request: NextRequest) { | |
| return updateSession(request) | |
| } |
| const url = new URL(request.url) | ||
| const code = url.searchParams.get('code') | ||
| const next = url.searchParams.get('next') || '/dashboard' | ||
| const safeNext = next.startsWith('/') && !next.startsWith('//') ? next : '/dashboard' |
There was a problem hiding this comment.
The current check next.startsWith('/') && !next.startsWith('//') is vulnerable to an Open Redirect attack.\n\nAn attacker can bypass this check by using a backslash after the initial slash, such as /\\example.com. In modern browsers and Node.js, new URL('/\\\\example.com', url.origin) resolves to http://example.com/ because the backslash is normalized to a forward slash.\n\nTo prevent this, ensure that next does not start with /\\ as well.
| const safeNext = next.startsWith('/') && !next.startsWith('//') ? next : '/dashboard' | |
| const safeNext = next.startsWith('/') && !next.startsWith('//') && !next.startsWith('/\\') ? next : '/dashboard' |
| try { | ||
| body = await req.json() | ||
| } catch { | ||
| return NextResponse.json({ error: 'invalid json' }, { status: 400 }) | ||
| } | ||
| const parsed = uploadRequestSchema.safeParse(await req.json()) | ||
| if (!parsed.success) return apiError('Invalid upload request', 400, parsed.error.flatten()) |
There was a problem hiding this comment.
In the previous implementation, malformed JSON in the request body was caught and handled gracefully with a 400 Bad Request response.\n\nIn this updated version, any JSON parsing error from await req.json() will bubble up to the main catch block and trigger internalError(error). This returns a 500 Internal Server Error to the client and logs an exception to Sentry, which can lead to unnecessary noise and false alarms in your error monitoring for simple client-side errors.\n\nWe should separate the JSON parsing try-catch from the main try-catch to return a proper 400 status code for malformed client input.
let json: unknown
try {
json = await req.json()
} catch {
return apiError('Invalid JSON', 400)
}
try {
const parsed = uploadRequestSchema.safeParse(json)
if (!parsed.success) return apiError('Invalid upload request', 400, parsed.error.flatten())| export function UploadDemo() { | ||
| const [status, setStatus] = useState<string>('') | ||
| const [status, setStatus] = useState('') | ||
| const [url, setUrl] = useState<string | null>(null) | ||
| const abortRef = useRef<AbortController | null>(null) |
There was a problem hiding this comment.
If the UploadDemo component unmounts while an upload is still in progress, the active fetch requests are not aborted, and attempting to update the state after unmounting can cause memory leaks or unexpected behavior.\n\nWe should add a useEffect hook to clean up and abort any active requests when the component unmounts.\n\nNote: Remember to import useEffect from 'react' at the top of the file.
export function UploadDemo() {
const [status, setStatus] = useState('')
const [url, setUrl] = useState<string | null>(null)
const abortRef = useRef<AbortController | null>(null)
useEffect(() => {
return () => {
abortRef.current?.abort()
}
}, [])
| ['Cloudflare R2', hasR2Env(), 'Private object storage and signed uploads'], | ||
| ['Resend', Boolean(env.RESEND_API_KEY && env.EMAIL_FROM), 'Transactional email'], | ||
| ['Sentry', Boolean(env.NEXT_PUBLIC_SENTRY_DSN), 'Errors and tracing'], | ||
| ['Hugging Face', Boolean(process.env.HF_TOKEN), 'Optional AI access'], |
There was a problem hiding this comment.
For consistency and to fully leverage the typed environment validation, HF_TOKEN should be added to the Zod schema in lib/env.ts and accessed via env.HF_TOKEN instead of directly reading from process.env.
| ['Hugging Face', Boolean(process.env.HF_TOKEN), 'Optional AI access'], | |
| ['Hugging Face', Boolean(env.HF_TOKEN), 'Optional AI access'], |
| UPLOAD_MAX_BYTES: z.coerce.number().int().positive().default(10 * 1024 * 1024), | ||
| }) |
There was a problem hiding this comment.
Add HF_TOKEN to the validated environment schema so it can be accessed safely and consistently via env.HF_TOKEN across the application.
| UPLOAD_MAX_BYTES: z.coerce.number().int().positive().default(10 * 1024 * 1024), | |
| }) | |
| UPLOAD_MAX_BYTES: z.coerce.number().int().positive().default(10 * 1024 * 1024), | |
| HF_TOKEN: z.string().optional(), | |
| }) |
| test('Next 16 proxy is present and legacy middleware is not referenced by docs', async () => { | ||
| assert.match(await text('proxy.ts'), /export async function proxy/) | ||
| assert.doesNotMatch(await text('README.md'), /middleware\.ts/) | ||
| }) |
There was a problem hiding this comment.
Since the middleware file must be named middleware.ts to be recognized by Next.js, this test should be updated to assert the presence of middleware.ts and the middleware function instead of proxy.ts.
test('Next middleware is present', async () => {
assert.match(await text('middleware.ts'), /export async function middleware/)
})
Review: not merging yet — two blocking issues1. CI fails on its own configuration (root cause of the red This PR deletes Fix: commit the regenerated 2. The branch is based on a pre-rewrite This branch forked from Suggested rescope after rebasing onto current
Leaving as draft per the PR's own description ("draft until those checks complete"). Happy to re-review once it's rebased and CI is green. |
2 participants
What changed
middleware.tsto the Next.js 16proxy.tsconventionnpm run checkSECURITY.mdWhy
The original repository was a strong reference implementation, but its
$0 at scaleframing and minimal demo flows could be interpreted as production-ready. This change keeps the core intentionally small while making its security boundaries, upgrade paths, operational checks, and provider limitations explicit.Developer impact
npm installonce to generate a fresh lockfile after the dependency-major upgrade.supabase/migrations/202607130001_harden_profiles_and_uploads.sql.Validation
GitHub Actions runs lint, type checking, repository tests, a production build, dependency audit, and CodeQL. This PR is intentionally opened as a draft until those checks complete and any dependency migration issues are resolved.