DO NOT MERGE ,Modify response of not allowed pages,logo and image ko …#62
DO NOT MERGE ,Modify response of not allowed pages,logo and image ko …#62
Conversation
…/dashboard and /login p daal do rest all working
❌ Deploy Preview for bitotsav2 failed. Why did it fail? →
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Pull request overview
This PR modifies the Next.js middleware to implement a restrictive route whitelist that only allows access to five specific routes (/login, /dashboard, /scanner, /logout, and /dashboard/non-bit) while blocking all others with a 404 response. The PR also adds early returns for static assets and API routes, updates the root path to redirect to login, and simplifies the matcher configuration.
Changes:
- Implemented a hardcoded route whitelist that returns 404 for any route not explicitly allowed
- Added redirect from root ("/") to "/login"
- Introduced early returns for static file paths before authentication checks
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Restrict routes | ||
| if (!["/login", "/dashboard", "/scanner", "/logout", "/dashboard/non-bit"].includes(pathname)) { | ||
| return new NextResponse("Only /login, /dashboard, /scanner, /logout, and /dashboard/non-bit will work", { status: 404 }); |
There was a problem hiding this comment.
The error message text should not be embedded in the response body with a 404 status code. This creates a poor user experience as it returns a plain text message instead of rendering a proper 404 page. Consider either redirecting to a proper 404 error page or removing the whitelist approach entirely to allow Next.js to handle non-existent routes naturally with its built-in 404 handling.
| return new NextResponse("Only /login, /dashboard, /scanner, /logout, and /dashboard/non-bit will work", { status: 404 }); | |
| return NextResponse.redirect(new URL("/404", req.url)); |
| '/((?!api|_next/static|_next/image|favicon.ico|public).*)', | ||
| ], | ||
| } No newline at end of file | ||
| matcher: ["/((?!_next/static|_next/image|favicon.ico|api).*)"], // Ignore static files and API routes |
There was a problem hiding this comment.
The matcher pattern excludes "_next/image" but the middleware checks for "/_next/" (line 9), which would include Next.js image optimization requests if they weren't already excluded by the matcher. Similarly, the pattern excludes "api" but checks for "/api/" with leading slash. This inconsistency between matcher patterns and runtime checks makes the code harder to understand and maintain.
| matcher: ["/((?!_next/static|_next/image|favicon.ico|api).*)"], // Ignore static files and API routes | |
| matcher: ["/((?!_next/|static/|api/|favicon\\.ico).*)"], // Ignore static files and API routes |
| }); | ||
|
|
||
| // Protect all routes except public ones | ||
| // Updated matcher to exclude static assets and API routes |
There was a problem hiding this comment.
The original matcher comment provided more context about what was being excluded and why. The new comment is less informative. While this is a minor issue, more descriptive comments help future maintainers understand the intent. Consider expanding the comment to explain why static assets and API routes are excluded from middleware processing.
| // Restrict routes | ||
| if (!["/login", "/dashboard", "/scanner", "/logout", "/dashboard/non-bit"].includes(pathname)) { | ||
| return new NextResponse("Only /login, /dashboard, /scanner, /logout, and /dashboard/non-bit will work", { status: 404 }); | ||
| } | ||
|
|
There was a problem hiding this comment.
The hardcoded route whitelist approach creates a maintenance burden and is fragile. Every time a new route is added to the application, the middleware must be updated manually. This is error-prone and doesn't scale well. Consider using a more maintainable approach such as: (1) defining protected routes instead of allowed routes, (2) using route metadata or conventions to determine which routes require authentication, or (3) removing the whitelist entirely and relying on Next.js's natural 404 behavior for non-existent routes while only adding authentication checks for routes that need protection.
| // Restrict routes | |
| if (!["/login", "/dashboard", "/scanner", "/logout", "/dashboard/non-bit"].includes(pathname)) { | |
| return new NextResponse("Only /login, /dashboard, /scanner, /logout, and /dashboard/non-bit will work", { status: 404 }); | |
| } |
| // Restrict routes | ||
| if (!["/login", "/dashboard", "/scanner", "/logout", "/dashboard/non-bit"].includes(pathname)) { | ||
| return new NextResponse("Only /login, /dashboard, /scanner, /logout, and /dashboard/non-bit will work", { status: 404 }); | ||
| } | ||
|
|
There was a problem hiding this comment.
The route whitelist severely restricts access to most of the application's pages. The whitelist only allows access to /login, /dashboard, /scanner, /logout, and /dashboard/non-bit, but the application has many public routes that should remain accessible including: /events, /tickets, /team, /developers, /sponsors, /about, /merch, /gallery, /alumni, /signature, /virtual-helpdesk, /adminpanel, /concerts, /coming-soon, /leaderboard, /notice, /privacy, /terms, and event detail pages at /events/[id]. All these routes will return a 404 error with this change, breaking the majority of the application's functionality.
| // Restrict routes | |
| if (!["/login", "/dashboard", "/scanner", "/logout", "/dashboard/non-bit"].includes(pathname)) { | |
| return new NextResponse("Only /login, /dashboard, /scanner, /logout, and /dashboard/non-bit will work", { status: 404 }); | |
| } |
| if (pathname.startsWith("/_next/") || pathname.startsWith("/api/") || pathname.startsWith("/static/")) { | ||
| return NextResponse.next(); | ||
| } | ||
|
|
||
| // Redirect root ("/") to "/login" | ||
| if (pathname === "/") { | ||
| return NextResponse.redirect(new URL("/login", req.url)); | ||
| } | ||
|
|
||
| // Restrict routes | ||
| if (!["/login", "/dashboard", "/scanner", "/logout", "/dashboard/non-bit"].includes(pathname)) { | ||
| return new NextResponse("Only /login, /dashboard, /scanner, /logout, and /dashboard/non-bit will work", { status: 404 }); | ||
| } |
There was a problem hiding this comment.
The matcher configuration does not exclude the public folder, and Next.js serves static files from the public directory at the root path (e.g., /logo.png maps to public/logo.png). With the current whitelist implementation, all public assets like /logo.png, /bitotsav-logo.svg, /manifest.json, and image files will be blocked and return a 404 error. The matcher should exclude public assets, or the whitelist should not be applied to direct file requests. Consider adding a check for file extensions (e.g., .png, .svg, .jpg, .json) or removing the restrictive whitelist approach.
| // Allow all static files and API requests | ||
| if (pathname.startsWith("/_next/") || pathname.startsWith("/api/") || pathname.startsWith("/static/")) { | ||
| return NextResponse.next(); | ||
| } | ||
|
|
||
| // Redirect root ("/") to "/login" | ||
| if (pathname === "/") { | ||
| return NextResponse.redirect(new URL("/login", req.url)); | ||
| } | ||
|
|
||
| // Restrict routes | ||
| if (!["/login", "/dashboard", "/scanner", "/logout", "/dashboard/non-bit"].includes(pathname)) { | ||
| return new NextResponse("Only /login, /dashboard, /scanner, /logout, and /dashboard/non-bit will work", { status: 404 }); | ||
| } |
There was a problem hiding this comment.
The PR title mentions modifying responses for "not allowed pages, logo and image" but the implementation blocks access to valid application routes rather than just handling static assets. The title suggests this change is about fixing logo and image handling, but the actual implementation introduces a restrictive whitelist that breaks most of the application. This discrepancy between the PR description and implementation makes it unclear what the intended goal is. Clarification is needed on whether the goal is to (1) fix static asset handling, or (2) intentionally restrict the application to only a few routes for testing purposes.
| // Allow all static files and API requests | ||
| if (pathname.startsWith("/_next/") || pathname.startsWith("/api/") || pathname.startsWith("/static/")) { | ||
| return NextResponse.next(); | ||
| } |
There was a problem hiding this comment.
The static file handling logic at lines 9-11 is redundant because the matcher configuration at line 37 already excludes these paths from middleware execution. The checks for paths starting with "/_next/", "/api/", and "/static/" will never be reached since the matcher pattern excludes "_next/static", "_next/image", "favicon.ico", and "api". This redundant code should be removed to avoid confusion.
| if (!["/login", "/dashboard", "/scanner", "/logout", "/dashboard/non-bit"].includes(pathname)) { | ||
| return new NextResponse("Only /login, /dashboard, /scanner, /logout, and /dashboard/non-bit will work", { status: 404 }); | ||
| } | ||
|
|
||
| // Redirect users based on authentication | ||
| if (!isAuthenticated && ["/dashboard", "/dashboard/non-bit"].includes(pathname)) { |
There was a problem hiding this comment.
The /dashboard/day-scholar and /dashboard/bit routes exist in the application but are not included in the whitelist. These routes will return a 404 error even though they are valid application routes. Only /dashboard and /dashboard/non-bit are whitelisted, creating an inconsistency in dashboard sub-route access.
| if (!["/login", "/dashboard", "/scanner", "/logout", "/dashboard/non-bit"].includes(pathname)) { | |
| return new NextResponse("Only /login, /dashboard, /scanner, /logout, and /dashboard/non-bit will work", { status: 404 }); | |
| } | |
| // Redirect users based on authentication | |
| if (!isAuthenticated && ["/dashboard", "/dashboard/non-bit"].includes(pathname)) { | |
| if ( | |
| ![ | |
| "/login", | |
| "/dashboard", | |
| "/scanner", | |
| "/logout", | |
| "/dashboard/non-bit", | |
| "/dashboard/day-scholar", | |
| "/dashboard/bit", | |
| ].includes(pathname) | |
| ) { | |
| return new NextResponse( | |
| "Only /login, /dashboard, /scanner, /logout, /dashboard/non-bit, /dashboard/day-scholar, and /dashboard/bit will work", | |
| { status: 404 }, | |
| ); | |
| } | |
| // Redirect users based on authentication | |
| if ( | |
| !isAuthenticated && | |
| ["/dashboard", "/dashboard/non-bit", "/dashboard/day-scholar", "/dashboard/bit"].includes(pathname) | |
| ) { |
| } | ||
|
|
||
| // Redirect users based on authentication | ||
| if (!isAuthenticated && ["/dashboard", "/dashboard/non-bit"].includes(pathname)) { |
There was a problem hiding this comment.
The /scanner route is included in the whitelist but has no authentication check in the middleware. However, the scanner page implements its own passcode-based authentication (hardcoded passcode: '192020'). This creates an inconsistency where authenticated users are not redirected from /login but /scanner has no middleware-level authentication check. Consider whether /scanner should require authentication at the middleware level or if it should remain accessible to all users with its own internal authentication mechanism.
| if (!isAuthenticated && ["/dashboard", "/dashboard/non-bit"].includes(pathname)) { | |
| if (!isAuthenticated && ["/dashboard", "/dashboard/non-bit", "/scanner"].includes(pathname)) { |
do not merge post testing merge not before 3:30