Use append for multi-value headers in config header application

[southpolesteve]

Summary

• Config headers from next.config.js were applied using set(), which replaces any existing header with the same key. For multi-value headers like Vary and Set-Cookie, this destroys previously set values. For example, a config header setting Vary: Accept-Language would replace the existing Vary: RSC, Accept header, breaking CDN caching behavior.
• Changed to use append() for Vary and Set-Cookie headers so values accumulate correctly. All other headers continue to use set() (override is the correct behavior for single-value headers).

Changes

• packages/vinext/src/server/app-dev-server.ts: Config header application in handler() wrapper uses append() for Vary and Set-Cookie, set() for all others.
• packages/vinext/src/server/prod-server.ts: Config header merging into middlewareHeaders object concatenates Vary and Set-Cookie values instead of overwriting.
• tests/fixtures/pages-basic/next.config.mjs: Added a Vary config header rule on /ssr.
• tests/pages-router.test.ts: New test verifying Vary config header appears in the response.

[southpolesteve]

/bigbonk review this

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vinext@291

commit: 30d5229

[github-actions]

Example	Preview	Production	Original
app-router-cloudflare	preview	production
pages-router-cloudflare	preview	production
app-router-playground	preview	production	original
realworld-api-rest	preview	production
nextra-docs-template	preview	production
benchmarks	preview	production
hackernews	preview	production	original

[ask-bonk]

Review

The core fix is correct — using append() / concatenation for Vary and Set-Cookie instead of set() / overwrite is the right approach. These are the two standard multi-value headers where replacement destroys meaningful data.

However, this PR fixes 2 of the 4 call sites that apply config headers. Per the AGENTS.md guidelines: "When fixing a bug in any of these files, check whether the same bug exists in the others. Do not leave known bugs as 'follow-ups' — fix them in the same PR."

Missing parity fixes

1. packages/vinext/src/index.ts:3857-3867 (Pages Router dev)

The applyHeaders() helper unconditionally uses res.setHeader():

function applyHeaders(pathname, res, headers, ctx) {
  const matched = matchHeaders(pathname, headers, ctx);
  for (const header of matched) {
    res.setHeader(header.key, header.value); // overwrites for all headers
  }
}

This needs the same treatment — use res.appendHeader() (available since Node 18.3) for Vary and Set-Cookie, res.setHeader() for everything else. Or use res.getHeader() + manual concatenation if you want to support older Node versions.

2. packages/vinext/src/deploy.ts:614-620 (Pages Router Cloudflare Worker entry)

The generated worker entry always overwrites:

for (const h of matched) {
  middlewareHeaders[h.key.toLowerCase()] = h.value; // overwrites for all headers
}

This needs the same concatenation logic that prod-server.ts now has.

3. mergeHeaders() in deploy.ts:704-719 also has a related problem — it uses a flat Record<string, string> and response.headers.forEach((v, k) => { merged[k] = v }). For Set-Cookie, Headers.forEach() may combine multiple Set-Cookie values into one entry or lose them depending on the runtime. This isn't directly caused by this PR but is worth noting since you're already in this area.

Subtle issue in prod-server.ts merge step

The fix correctly concatenates Vary into middlewareHeaders, but then at lines 951-952:

const responseHeaders: Record<string, string> = { ...middlewareHeaders };
response.headers.forEach((v, k) => { responseHeaders[k] = v; });

If the response itself has a Vary header (e.g., Vary: RSC, Accept), this overlay completely replaces the concatenated config Vary value. So the config Vary is effectively lost for pages that set their own Vary. This is pre-existing behavior and not a regression from this PR, but it means the fix is incomplete for the scenario described in the PR description ("a config header setting Vary: Accept-Language would replace the existing Vary: RSC, Accept header"). The current test passes because it only checks that Accept-Language is present, but doesn't verify that the response's original Vary values are also preserved.

The same pattern exists in app-dev-server.ts — the append() approach handles this correctly there since Headers.append() genuinely accumulates values. Consider doing the same in prod-server.ts: instead of spreading into a flat object, build a Headers object and use append() for multi-value keys.

Test coverage

The new test is good but could be stronger:

const vary = res.headers.get("vary") ?? "";
expect(vary).toContain("Accept-Language");

This only verifies the config value is present. It doesn't verify that existing Vary values (like RSC or Accept if present) are preserved alongside it. A stronger assertion would check both the config value AND the pre-existing values are present.

Also, there's no test for the Pages Router dev path (index.ts applyHeaders()), which currently has the bug. The existing dev-mode header tests at line 234 don't test multi-value header behavior.

Summary

The approach is sound. The two files touched are correctly fixed. But this needs:

1. Apply the same fix to index.ts:applyHeaders() (Pages Router dev)
2. Apply the same fix to deploy.ts worker entry generation (Pages Router Cloudflare Workers)
3. Ideally strengthen the test to verify both old and new Vary values coexist

[ask-bonk]

Review posted on PR #291. Here's the summary:

The core fix is correct — append() for Vary and Set-Cookie is the right approach. But the PR only fixes 2 of 4 call sites, and the AGENTS.md guidelines explicitly say to fix all parity issues in the same PR.

Three issues raised:

1. Missing fix in index.ts:3857-3867 — The Pages Router dev applyHeaders() helper still uses res.setHeader() unconditionally, which overwrites multi-value headers.

2. Missing fix in deploy.ts:614-620 — The generated Cloudflare Worker entry still does plain assignment (middlewareHeaders[key] = value), which overwrites multi-value headers.

3. Subtle merge issue in prod-server.ts:951-952 — The config Vary is correctly concatenated into middlewareHeaders, but then response.headers.forEach((v, k) => { responseHeaders[k] = v }) overlays response headers on top, which can replace the concatenated Vary value entirely. Pre-existing issue, but it undercuts the fix for the exact scenario described in the PR description.

The test could also be stronger — it only checks that Accept-Language is present but doesn't verify pre-existing Vary values are preserved alongside it.

github run

[southpolesteve]

Pushed parity fixes for the two missing call sites:

• index.ts applyHeaders() (Pages Router dev): now uses getHeader() + concatenation for Vary and Set-Cookie before calling setHeader()
• deploy.ts (Cloudflare Worker entry): same ", " concatenation pattern as prod-server.ts

All 4 config header application sites now use append semantics for multi-value headers.

/bigbonk review this

[ask-bonk]

Follow-up Review

The two parity fixes look correct. The index.ts and deploy.ts changes now match the append semantics used in the other two call sites. A couple of observations:

index.ts applyHeaders() — minor edge case with Set-Cookie

res.getHeader("set-cookie") in Node.js returns a string[] when multiple Set-Cookie headers have been set (Node's HTTP module special-cases this header). So line 3903:

res.setHeader(header.key, existing + ", " + header.value);

would produce "cookie1=a,cookie2=b, newcookie=c" (array stringified with commas) which is technically valid for a single Set-Cookie header value but loses the array-of-values semantics. In practice this is unlikely to cause problems because:

1. Config headers rarely set Set-Cookie (it's almost always done programmatically)
2. The intermediate state is short-lived — the response is finalized shortly after

But if you want correctness for the Set-Cookie case specifically, you could do:

if (lk === "set-cookie") {
  const existing = res.getHeader(lk);
  const arr = existing ? (Array.isArray(existing) ? existing : [existing]) : [];
  arr.push(header.value);
  res.setHeader(header.key, arr);
}

This is a nit — Set-Cookie via config headers is an edge case of an edge case. The Vary path is the one that matters and it's correct.

prod-server.ts — still has the original bug (not a regression)

The previous review flagged this and it's still present at line 855:

middlewareHeaders[h.key.toLowerCase()] = h.value;

This is the only call site that wasn't fixed. The deploy.ts fix was applied (it's the Worker-entry equivalent), but prod-server.ts was supposedly fixed in the first commit — yet looking at the current code, it still does plain assignment. Was the prod-server.ts change from the first commit lost during rebase?

app-dev-server.ts — also still uses set()

At line 1262:

response.headers.set(h.key, h.value);

This was noted as fixed in the original PR description but the code still shows set(). Same question — was this lost?

Summary

The two files in this push (index.ts and deploy.ts) are correctly fixed. But prod-server.ts:855 and app-dev-server.ts:1262 appear to still use overwrite semantics. Can you double-check whether the first commit's changes are actually in the branch?

[ask-bonk]

Review posted. Here's the summary:

The two files in this push (index.ts and deploy.ts) are correctly fixed. The append semantics for Vary and Set-Cookie match the pattern described in the PR.

However, two issues:

1. prod-server.ts:855 still does plain assignment (middlewareHeaders[h.key.toLowerCase()] = h.value) — no append logic. This was supposedly fixed in the first commit but the current code doesn't have it.

2. app-dev-server.ts:1262 still uses response.headers.set() — also supposedly fixed in the first commit but not present in the current code.

It looks like the first commit's changes to these two files may have been lost during rebase or were in a different commit that isn't in this branch. The diff for this push only shows changes to index.ts and deploy.ts, and the PR's changed files list only shows those two files — but the original PR description claims app-dev-server.ts and prod-server.ts were also fixed.

Minor nit on index.ts: res.getHeader("set-cookie") returns a string[] in Node.js when multiple values exist. The current concatenation (existing + ", " + header.value) would stringify the array with commas. Unlikely to matter in practice but worth knowing.

github run

[southpolesteve]

Good catch. The original prod-server.ts and app-dev-server.ts fixes were lost during a rebase. Re-applied them in the latest push. All four config header application sites (app-dev-server.ts, prod-server.ts, index.ts, deploy.ts) now use append semantics for Vary and Set-Cookie. Added test and fixture for the Vary append behavior.

/bigbonk review this

[ask-bonk]

@southpolesteve Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

[southpolesteve]

/bigbonk review this