-
Notifications
You must be signed in to change notification settings - Fork 221
fix: Pages prod config headers and redirects after middleware rewrites #448
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
772c166
07aba4f
be11d4e
52387fe
239ba26
25d4b5b
763c6e0
2be7a82
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| export default { | ||
| async headers() { | ||
| return [ | ||
| { | ||
| source: "/headers-before-middleware-rewrite", | ||
| headers: [{ key: "x-rewrite-source-header", value: "1" }], | ||
| }, | ||
| ]; | ||
| }, | ||
|
|
||
| async redirects() { | ||
| return [ | ||
| { | ||
| source: "/redirect-before-middleware-rewrite", | ||
| destination: "/about", | ||
| permanent: false, | ||
| }, | ||
| { | ||
| source: "/redirect-before-middleware-response", | ||
| destination: "/about", | ||
| permanent: false, | ||
| }, | ||
| ]; | ||
| }, | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ import { | |
| requestContextFromRequest, | ||
| isExternalUrl, | ||
| proxyExternalRequest, | ||
| sanitizeDestination, | ||
| } from "vinext/config/config-matchers"; | ||
| import { mergeHeaders } from "vinext/server/worker-utils"; | ||
|
|
||
|
|
@@ -73,9 +74,25 @@ export default { | |
| request = new Request(strippedUrl, request); | ||
| } | ||
|
|
||
| // Build request context for has/missing condition matching | ||
| // Build request context for config matching that runs before middleware. | ||
| const reqCtx = requestContextFromRequest(request); | ||
|
|
||
| // Apply redirects from next.config.js before middleware. | ||
| if (configRedirects.length) { | ||
| const redirect = matchRedirect(pathname, configRedirects, reqCtx); | ||
| if (redirect) { | ||
| const dest = sanitizeDestination( | ||
| basePath && !isExternalUrl(redirect.destination) && !redirect.destination.startsWith(basePath) | ||
| ? basePath + redirect.destination | ||
| : redirect.destination, | ||
| ); | ||
| return new Response(null, { | ||
| status: redirect.permanent ? 308 : 307, | ||
| headers: { Location: dest }, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // Run middleware | ||
| let resolvedUrl = urlWithQuery; | ||
| const middlewareHeaders: Record<string, string | string[]> = {}; | ||
|
|
@@ -142,11 +159,12 @@ export default { | |
| }); | ||
| } | ||
|
|
||
| const postMwReqCtx = requestContextFromRequest(request); | ||
| let resolvedPathname = resolvedUrl.split("?")[0]; | ||
|
|
||
| // Apply custom headers from next.config.js | ||
| if (configHeaders.length) { | ||
| const matched = matchHeaders(resolvedPathname, configHeaders); | ||
| const matched = matchHeaders(pathname, configHeaders, reqCtx); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| for (const h of matched) { | ||
| const lk = h.key.toLowerCase(); | ||
| if (lk === "set-cookie") { | ||
|
|
@@ -160,29 +178,15 @@ export default { | |
| } | ||
| } else if (lk === "vary" && middlewareHeaders[lk]) { | ||
| middlewareHeaders[lk] += ", " + h.value; | ||
| } else { | ||
| } else if (!(lk in middlewareHeaders)) { | ||
| middlewareHeaders[lk] = h.value; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Apply redirects from next.config.js | ||
| if (configRedirects.length) { | ||
| const redirect = matchRedirect(resolvedPathname, configRedirects, reqCtx); | ||
| if (redirect) { | ||
| const dest = basePath && !redirect.destination.startsWith(basePath) | ||
| ? basePath + redirect.destination | ||
| : redirect.destination; | ||
| return new Response(null, { | ||
| status: redirect.permanent ? 308 : 307, | ||
| headers: { Location: dest }, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // Apply beforeFiles rewrites from next.config.js | ||
| if (configRewrites.beforeFiles?.length) { | ||
| const rewritten = matchRewrite(resolvedPathname, configRewrites.beforeFiles, reqCtx); | ||
| const rewritten = matchRewrite(resolvedPathname, configRewrites.beforeFiles, postMwReqCtx); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good: |
||
| if (rewritten) { | ||
| if (isExternalUrl(rewritten)) { | ||
| return proxyExternalRequest(request, rewritten); | ||
|
|
@@ -202,7 +206,7 @@ export default { | |
|
|
||
| // Apply afterFiles rewrites | ||
| if (configRewrites.afterFiles?.length) { | ||
| const rewritten = matchRewrite(resolvedPathname, configRewrites.afterFiles, reqCtx); | ||
| const rewritten = matchRewrite(resolvedPathname, configRewrites.afterFiles, postMwReqCtx); | ||
| if (rewritten) { | ||
| if (isExternalUrl(rewritten)) { | ||
| return proxyExternalRequest(request, rewritten); | ||
|
|
@@ -219,7 +223,7 @@ export default { | |
|
|
||
| // Fallback rewrites (if SSR returned 404) | ||
| if (response && response.status === 404 && configRewrites.fallback?.length) { | ||
| const fallbackRewrite = matchRewrite(resolvedPathname, configRewrites.fallback, reqCtx); | ||
| const fallbackRewrite = matchRewrite(resolvedPathname, configRewrites.fallback, postMwReqCtx); | ||
| if (fallbackRewrite) { | ||
| if (isExternalUrl(fallbackRewrite)) { | ||
| return proxyExternalRequest(request, fallbackRewrite); | ||
|
|
@@ -240,4 +244,3 @@ export default { | |
| } | ||
| }, | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1943,16 +1943,39 @@ async function _handleRequest(request, __reqCtx, _mwCtx) { | |||||||||
| } | ||||||||||
| const hasDefault = typeof handler["default"] === "function"; | ||||||||||
|
|
||||||||||
| // Route handlers need the same middleware header/status merge behavior as | ||||||||||
| // page responses. This keeps middleware response headers visible on API | ||||||||||
| // routes in Workers/dev, and preserves custom rewrite status overrides. | ||||||||||
| function attachRouteHandlerMiddlewareContext(response) { | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When
Suggested change
|
||||||||||
| // _mwCtx.headers is only set (non-null) when middleware actually ran and | ||||||||||
| // produced a continue/rewrite response. An empty Headers object (middleware | ||||||||||
| // ran but produced no response headers) is a harmless edge case: the early | ||||||||||
| // return is skipped, but the copy loop below is a no-op, so no incorrect | ||||||||||
| // headers are added. The allocation cost in that case is acceptable. | ||||||||||
| if (!_mwCtx.headers && _mwCtx.status == null) return response; | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The early-return guard ( |
||||||||||
| const responseHeaders = new Headers(response.headers); | ||||||||||
| if (_mwCtx.headers) { | ||||||||||
| for (const [key, value] of _mwCtx.headers) { | ||||||||||
| responseHeaders.append(key, value); | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: |
||||||||||
| } | ||||||||||
| } | ||||||||||
| return new Response(response.body, { | ||||||||||
| status: _mwCtx.status ?? response.status, | ||||||||||
| statusText: response.statusText, | ||||||||||
| headers: responseHeaders, | ||||||||||
| }); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // OPTIONS auto-implementation: respond with Allow header and 204 | ||||||||||
| if (method === "OPTIONS" && typeof handler["OPTIONS"] !== "function") { | ||||||||||
| const allowMethods = hasDefault ? HTTP_METHODS : exportedMethods; | ||||||||||
| if (!allowMethods.includes("OPTIONS")) allowMethods.push("OPTIONS"); | ||||||||||
| setHeadersContext(null); | ||||||||||
| setNavigationContext(null); | ||||||||||
| return new Response(null, { | ||||||||||
| return attachRouteHandlerMiddlewareContext(new Response(null, { | ||||||||||
| status: 204, | ||||||||||
| headers: { "Allow": allowMethods.join(", ") }, | ||||||||||
| }); | ||||||||||
| })); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // HEAD auto-implementation: run GET handler and strip body | ||||||||||
|
|
@@ -1996,28 +2019,28 @@ async function _handleRequest(request, __reqCtx, _mwCtx) { | |||||||||
| if (draftCookie) newHeaders.append("Set-Cookie", draftCookie); | ||||||||||
|
|
||||||||||
| if (isAutoHead) { | ||||||||||
| return new Response(null, { | ||||||||||
| return attachRouteHandlerMiddlewareContext(new Response(null, { | ||||||||||
| status: response.status, | ||||||||||
| statusText: response.statusText, | ||||||||||
| headers: newHeaders, | ||||||||||
| }); | ||||||||||
| })); | ||||||||||
| } | ||||||||||
| return new Response(response.body, { | ||||||||||
| return attachRouteHandlerMiddlewareContext(new Response(response.body, { | ||||||||||
| status: response.status, | ||||||||||
| statusText: response.statusText, | ||||||||||
| headers: newHeaders, | ||||||||||
| }); | ||||||||||
| })); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if (isAutoHead) { | ||||||||||
| // Strip body for auto-HEAD, preserve headers and status | ||||||||||
| return new Response(null, { | ||||||||||
| return attachRouteHandlerMiddlewareContext(new Response(null, { | ||||||||||
| status: response.status, | ||||||||||
| statusText: response.statusText, | ||||||||||
| headers: response.headers, | ||||||||||
| }); | ||||||||||
| })); | ||||||||||
| } | ||||||||||
| return response; | ||||||||||
| return attachRouteHandlerMiddlewareContext(response); | ||||||||||
| } catch (err) { | ||||||||||
| getAndClearPendingCookies(); // Clear any pending cookies on error | ||||||||||
| // Catch redirect() / notFound() thrown from route handlers | ||||||||||
|
|
@@ -2029,16 +2052,16 @@ async function _handleRequest(request, __reqCtx, _mwCtx) { | |||||||||
| const statusCode = parts[3] ? parseInt(parts[3], 10) : 307; | ||||||||||
| setHeadersContext(null); | ||||||||||
| setNavigationContext(null); | ||||||||||
| return new Response(null, { | ||||||||||
| return attachRouteHandlerMiddlewareContext(new Response(null, { | ||||||||||
| status: statusCode, | ||||||||||
| headers: { Location: new URL(redirectUrl, request.url).toString() }, | ||||||||||
| }); | ||||||||||
| })); | ||||||||||
| } | ||||||||||
| if (digest === "NEXT_NOT_FOUND" || digest.startsWith("NEXT_HTTP_ERROR_FALLBACK;")) { | ||||||||||
| const statusCode = digest === "NEXT_NOT_FOUND" ? 404 : parseInt(digest.split(";")[1], 10); | ||||||||||
| setHeadersContext(null); | ||||||||||
| setNavigationContext(null); | ||||||||||
| return new Response(null, { status: statusCode }); | ||||||||||
| return attachRouteHandlerMiddlewareContext(new Response(null, { status: statusCode })); | ||||||||||
| } | ||||||||||
| } | ||||||||||
| setHeadersContext(null); | ||||||||||
|
|
@@ -2051,17 +2074,17 @@ async function _handleRequest(request, __reqCtx, _mwCtx) { | |||||||||
| ).catch((reportErr) => { | ||||||||||
| console.error("[vinext] Failed to report route handler error:", reportErr); | ||||||||||
| }); | ||||||||||
| return new Response(null, { status: 500 }); | ||||||||||
| return attachRouteHandlerMiddlewareContext(new Response(null, { status: 500 })); | ||||||||||
| } finally { | ||||||||||
| setHeadersAccessPhase(previousHeadersPhase); | ||||||||||
| } | ||||||||||
| } | ||||||||||
| setHeadersContext(null); | ||||||||||
| setNavigationContext(null); | ||||||||||
| return new Response(null, { | ||||||||||
| return attachRouteHandlerMiddlewareContext(new Response(null, { | ||||||||||
| status: 405, | ||||||||||
| headers: { Allow: exportedMethods.join(", ") }, | ||||||||||
| }); | ||||||||||
| })); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Build the component tree: layouts wrapping the page | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: The deploy template and prod-server both use
!hasBasePath(redirect.destination, basePath)here instead of!redirect.destination.startsWith(basePath). The difference matters whenbasePathis/appand the destination is/application—startsWithwould incorrectly skip the prefix, whilehasBasePathcorrectly requires a segment boundary.Since this is an example file and
hasBasePathisn't exported fromvinext/config/config-matchers, this is low-priority, but worth noting for correctness parity.Or alternatively, inline the
hasBasePathlogic or export it from the config-matchers barrel.