-
Notifications
You must be signed in to change notification settings - Fork 324
fix(config): respect has/missing conditions in next.config headers #48
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
6ea08f7
f89d8df
280237e
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 |
|---|---|---|
|
|
@@ -15,14 +15,24 @@ import { | |
| type ResolvedNextConfig, | ||
| type NextRedirect, | ||
| type NextRewrite, | ||
| type NextHeader, | ||
| } from "./config/next-config.js"; | ||
|
|
||
| import { findMiddlewareFile, runMiddleware } from "./server/middleware.js"; | ||
| import { generateSafeRegExpCode, generateMiddlewareMatcherCode, generateNormalizePathCode } from "./server/middleware-codegen.js"; | ||
| import { normalizePath } from "./server/normalize-path.js"; | ||
| import { findInstrumentationFile, runInstrumentation } from "./server/instrumentation.js"; | ||
| import { validateDevRequest } from "./server/dev-origin-check.js"; | ||
| import { safeRegExp, escapeHeaderSource, isExternalUrl, proxyExternalRequest } from "./config/config-matchers.js"; | ||
| import { | ||
| safeRegExp, | ||
| isExternalUrl, | ||
| proxyExternalRequest, | ||
| parseCookies, | ||
| matchHeaders, | ||
| matchRedirect, | ||
| matchRewrite, | ||
| type RequestContext, | ||
| } from "./config/config-matchers.js"; | ||
| import { scanMetadataFiles } from "./server/metadata-routes.js"; | ||
| import { staticExportPages } from "./build/static-export.js"; | ||
| import tsconfigPaths from "vite-tsconfig-paths"; | ||
|
|
@@ -2472,9 +2482,26 @@ hydrate(); | |
| } | ||
| } | ||
|
|
||
| // Build request context once for has/missing condition checks | ||
| // across headers, redirects, and rewrites. | ||
| const reqUrl = new URL(url, `http://${req.headers.host || "localhost"}`); | ||
|
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. Nit: You're building the That said, this pattern of converting Node.js headers to |
||
| const reqCtxHeaders = new Headers( | ||
| Object.fromEntries( | ||
| Object.entries(req.headers) | ||
| .filter(([, v]) => v !== undefined) | ||
| .map(([k, v]) => [k, Array.isArray(v) ? v.join(", ") : String(v)]) | ||
| ), | ||
| ); | ||
| const reqCtx: RequestContext = { | ||
| headers: reqCtxHeaders, | ||
| cookies: parseCookies(reqCtxHeaders.get("cookie")), | ||
| query: reqUrl.searchParams, | ||
| host: reqCtxHeaders.get("host") ?? reqUrl.host, | ||
| }; | ||
|
|
||
| // Apply custom headers from next.config.js | ||
| if (nextConfig?.headers.length) { | ||
| applyHeaders(pathname, res, nextConfig.headers); | ||
| applyHeaders(pathname, res, nextConfig.headers, reqCtx); | ||
| } | ||
|
|
||
| // Apply redirects from next.config.js | ||
|
|
@@ -2483,6 +2510,7 @@ hydrate(); | |
| pathname, | ||
| res, | ||
| nextConfig.redirects, | ||
| reqCtx, | ||
| ); | ||
| if (redirected) return; | ||
| } | ||
|
|
@@ -2491,7 +2519,7 @@ hydrate(); | |
| let resolvedUrl = url; | ||
| if (nextConfig?.rewrites.beforeFiles.length) { | ||
| resolvedUrl = | ||
| applyRewrites(pathname, nextConfig.rewrites.beforeFiles) ?? | ||
| applyRewrites(pathname, nextConfig.rewrites.beforeFiles, reqCtx) ?? | ||
| url; | ||
| } | ||
|
|
||
|
|
@@ -2532,6 +2560,7 @@ hydrate(); | |
| const afterRewrite = applyRewrites( | ||
| resolvedUrl.split("?")[0], | ||
| nextConfig.rewrites.afterFiles, | ||
| reqCtx, | ||
| ); | ||
| if (afterRewrite) resolvedUrl = afterRewrite; | ||
| } | ||
|
|
@@ -2557,6 +2586,7 @@ hydrate(); | |
| const fallbackRewrite = applyRewrites( | ||
| resolvedUrl.split("?")[0], | ||
| nextConfig.rewrites.fallback, | ||
| reqCtx, | ||
| ); | ||
| if (fallbackRewrite) { | ||
| // External fallback rewrite — proxy to external URL | ||
|
|
@@ -3402,22 +3432,15 @@ function applyRedirects( | |
| pathname: string, | ||
| res: any, | ||
| redirects: NextRedirect[], | ||
| ctx?: RequestContext, | ||
| ): boolean { | ||
| for (const redirect of redirects) { | ||
| const params = matchConfigPattern(pathname, redirect.source); | ||
| if (params) { | ||
| let dest = redirect.destination; | ||
| for (const [key, value] of Object.entries(params)) { | ||
| dest = dest.replace(`:${key}*`, value); | ||
| dest = dest.replace(`:${key}+`, value); | ||
| dest = dest.replace(`:${key}`, value); | ||
| } | ||
| // Sanitize to prevent open redirect via protocol-relative URLs | ||
| dest = sanitizeDestinationLocal(dest); | ||
| res.writeHead(redirect.permanent ? 308 : 307, { Location: dest }); | ||
| res.end(); | ||
| return true; | ||
| } | ||
| const result = matchRedirect(pathname, redirects, ctx); | ||
| if (result) { | ||
| // Sanitize to prevent open redirect via protocol-relative URLs | ||
| const dest = sanitizeDestinationLocal(result.destination); | ||
| res.writeHead(result.permanent ? 308 : 307, { Location: dest }); | ||
| res.end(); | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
@@ -3494,20 +3517,12 @@ async function proxyExternalRewriteNode( | |
| function applyRewrites( | ||
| pathname: string, | ||
| rewrites: NextRewrite[], | ||
| ctx?: RequestContext, | ||
| ): string | null { | ||
| for (const rewrite of rewrites) { | ||
| const params = matchConfigPattern(pathname, rewrite.source); | ||
| if (params) { | ||
| let dest = rewrite.destination; | ||
| for (const [key, value] of Object.entries(params)) { | ||
| dest = dest.replace(`:${key}*`, value); | ||
| dest = dest.replace(`:${key}+`, value); | ||
| dest = dest.replace(`:${key}`, value); | ||
| } | ||
| // Sanitize to prevent open redirect via protocol-relative URLs | ||
| dest = sanitizeDestinationLocal(dest); | ||
| return dest; | ||
| } | ||
| const dest = matchRewrite(pathname, rewrites, ctx); | ||
| if (dest) { | ||
| // Sanitize to prevent open redirect via protocol-relative URLs | ||
| return sanitizeDestinationLocal(dest); | ||
| } | ||
| return null; | ||
| } | ||
|
|
@@ -3518,16 +3533,12 @@ function applyRewrites( | |
| function applyHeaders( | ||
|
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. This function applyHeaders(
pathname: string,
res: any,
headers: NextHeader[],
ctx?: RequestContext,
): void {
const matched = matchHeaders(pathname, headers, ctx);
for (const header of matched) {
res.setHeader(header.key, header.value);
}
}This eliminates ~30 lines of duplicated pattern matching logic and ensures the two code paths can't drift out of sync — which is exactly the kind of parity issue flagged in AGENTS.md. |
||
| pathname: string, | ||
| res: any, | ||
| headers: Array<{ source: string; headers: Array<{ key: string; value: string }> }>, | ||
| headers: NextHeader[], | ||
| ctx?: RequestContext, | ||
| ): void { | ||
| for (const rule of headers) { | ||
| const escaped = escapeHeaderSource(rule.source); | ||
| const sourceRegex = safeRegExp("^" + escaped + "$"); | ||
| if (sourceRegex && sourceRegex.test(pathname)) { | ||
| for (const header of rule.headers) { | ||
| res.setHeader(header.key, header.value); | ||
| } | ||
| } | ||
| const matched = matchHeaders(pathname, headers, ctx); | ||
| for (const header of matched) { | ||
| res.setHeader(header.key, header.value); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1152,7 +1152,7 @@ async function __proxyExternalRequest(request, externalUrl) { | |
| return new Response(upstream.body, { status: upstream.status, statusText: upstream.statusText, headers: respHeaders }); | ||
| } | ||
|
|
||
| function __applyConfigHeaders(pathname) { | ||
| function __applyConfigHeaders(pathname, ctx) { | ||
| const result = []; | ||
| for (const rule of __configHeaders) { | ||
| const groups = []; | ||
|
|
@@ -1168,7 +1168,12 @@ function __applyConfigHeaders(pathname) { | |
| .replace(/:[\\w-]+/g, "[^/]+") | ||
| .replace(/___GROUP_(\\d+)___/g, (_, idx) => "(" + groups[Number(idx)] + ")"); | ||
| const sourceRegex = __safeRegExp("^" + escaped + "$"); | ||
| if (sourceRegex && sourceRegex.test(pathname)) result.push(...rule.headers); | ||
| if (sourceRegex && sourceRegex.test(pathname)) { | ||
| if (ctx && (rule.has || rule.missing)) { | ||
| if (!__checkHasConditions(rule.has, rule.missing, ctx)) continue; | ||
| } | ||
| result.push(...rule.headers); | ||
| } | ||
| } | ||
| return result; | ||
|
Comment on lines
1155
to
1178
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. This Also worth noting: the inline escaping here uses the simpler |
||
| } | ||
|
|
@@ -1185,7 +1190,8 @@ export default async function handler(request) { | |
| _runWithCacheState(() => | ||
| _runWithPrivateCache(() => | ||
| runWithFetchCache(async () => { | ||
| const response = await _handleRequest(request); | ||
| const __reqCtx = __buildRequestContext(request); | ||
| const response = await _handleRequest(request, __reqCtx); | ||
| // Apply custom headers from next.config.js to non-redirect responses. | ||
| // Skip redirects (3xx) because Response.redirect() creates immutable headers, | ||
| // and Next.js doesn't apply custom headers to redirects anyway. | ||
|
|
@@ -1194,7 +1200,7 @@ export default async function handler(request) { | |
| let pathname; | ||
| try { pathname = __normalizePath(decodeURIComponent(url.pathname)); } catch { pathname = url.pathname; } | ||
| ${bp ? `if (pathname.startsWith(${JSON.stringify(bp)})) pathname = pathname.slice(${JSON.stringify(bp)}.length) || "/";` : ""} | ||
| const extraHeaders = __applyConfigHeaders(pathname); | ||
| const extraHeaders = __applyConfigHeaders(pathname, __reqCtx); | ||
| for (const h of extraHeaders) { | ||
| response.headers.set(h.key, h.value); | ||
| } | ||
|
|
@@ -1207,7 +1213,7 @@ export default async function handler(request) { | |
| ); | ||
| } | ||
|
|
||
| async function _handleRequest(request) { | ||
| async function _handleRequest(request, __reqCtx) { | ||
| const url = new URL(request.url); | ||
|
|
||
| // ── Cross-origin request protection ───────────────────────────────── | ||
|
|
@@ -1253,7 +1259,6 @@ async function _handleRequest(request) { | |
| } | ||
|
|
||
| // ── Apply redirects from next.config.js ─────────────────────────────── | ||
| const __reqCtx = __buildRequestContext(request); | ||
| if (__configRedirects.length) { | ||
| const __redir = __applyConfigRedirects(pathname, __reqCtx); | ||
| if (__redir) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -150,14 +150,37 @@ test.describe("Config Custom Headers (OpenNext compat)", () => { | |
| ); | ||
|
|
||
| // Ref: opennextjs-cloudflare headers.test.ts — has/missing conditions | ||
| // vinext matchHeaders() in config-matchers.ts only checks source pattern. | ||
| // Tests: ON-15 #6 in TRACKING.md | ||
| test.fixme( | ||
| "config headers with has/missing conditions", | ||
| async () => { | ||
| // Would test: header rule with has: [{ type: "cookie", key: "logged-in" }] | ||
| // only applies when the cookie is present in the request. | ||
| // Needs: has/missing support in matchHeaders(), matchRedirect(), matchRewrite() | ||
| }, | ||
| ); | ||
| test("config headers with has/missing conditions", async ({ request }) => { | ||
| const baseline = await request.get(`${BASE}/about`); | ||
| expect(baseline.status()).toBe(200); | ||
| expect(baseline.headers()["x-conditional-header"]).toBeUndefined(); | ||
| expect(baseline.headers()["x-preview-header"]).toBeUndefined(); | ||
|
|
||
| // has(header) + missing(cookie) => header should be applied | ||
| const withRequiredHeader = await request.get(`${BASE}/about`, { | ||
| headers: { "x-user-tier": "pro" }, | ||
| }); | ||
| expect(withRequiredHeader.status()).toBe(200); | ||
| expect(withRequiredHeader.headers()["x-conditional-header"]).toBe("enabled"); | ||
|
|
||
| // same request + cookie that must be missing => header should not be applied | ||
| const blockedByMissingCondition = await request.get(`${BASE}/about`, { | ||
| headers: { | ||
| "x-user-tier": "pro", | ||
| "cookie": "no-config-header=1", | ||
| }, | ||
| }); | ||
| expect(blockedByMissingCondition.status()).toBe(200); | ||
| expect(blockedByMissingCondition.headers()["x-conditional-header"]).toBeUndefined(); | ||
|
|
||
| // has(query) => header should be applied | ||
| const withPreviewQuery = await request.get(`${BASE}/about?preview=1`); | ||
| expect(withPreviewQuery.status()).toBe(200); | ||
| expect(withPreviewQuery.headers()["x-preview-header"]).toBe("true"); | ||
|
|
||
| // unmatched query => header should not be applied | ||
| const withoutPreviewQuery = await request.get(`${BASE}/about?preview=0`); | ||
|
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. Nice test case — verifying that |
||
| expect(withoutPreviewQuery.status()).toBe(200); | ||
| expect(withoutPreviewQuery.headers()["x-preview-header"]).toBeUndefined(); | ||
| }); | ||
| }); | ||
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.
Worth a brief comment explaining the design: when
ctxis not provided,has/missingconditions are silently ignored and all path-matched rules apply unconditionally. This is intentional for backward compatibility but could be surprising to future readers.