Skip to content

fix: ensure internal x-middleware-set-cookie header is not passed on to lambda #2891

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: ensure internal x-middleware-set-cookie header is not passed on to lambda #2891

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8e4d7b6
fix: ensure internal x-middleware-set-cookie header is not set when s…
mrstork May 1, 2025
674ed88
test: skipping middleware draft mode tests
mrstork May 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions edge-runtime/lib/middleware.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ export function mergeMiddlewareCookies(middlewareResponse: Response, lambdaReque
const middlewareCookies = middlewareResponse.headers.get('x-middleware-set-cookie')

if (middlewareCookies) {
// Next expects internal headers to be omitted when cookies are set by the middleware
// See: https://github.com/vercel/next.js/blob/005db43079c7b59fd8c2594e8362761dc4cb3211/test/e2e/app-dir/app-middleware/app-middleware.test.ts#L197-L207
middlewareResponse.headers.delete('x-middleware-set-cookie')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does final response reaching client (browser/curl etc) would have actual set-cookie with this removal? from what I can see mergeMiddlewareCookies only sets cookie header:

opennextjs-netlify/edge-runtime/lib/response.ts

Lines 245 to 248 in 7795157

const newRequestCookies = mergeMiddlewareCookies(edgeResponse, newRequest)
if (newRequestCookies) {
newRequest.headers.set('Cookie', newRequestCookies)
}

so I wonder if this change as-is wouldn't result in clients no longer actually receiving set-cookie headers?


// Targets commas that are not followed by whitespace
// See: https://github.com/vercel/next.js/blob/e6145d3a37bb4c7b481fd58e05cdff9046ace8ad/packages/next/src/server/web/spec-extension/response.ts#L58-L66
const regex = new RegExp(/,(?!\s)/)
Expand Down
9 changes: 9 additions & 0 deletions tests/test-config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,15 @@
"tests": [
"router autoscrolling on navigation bugs Should apply scroll when loading.js is used"
]
},
{
"file": "test/e2e/app-dir/app-middleware/app-middleware.test.ts",
"reason": "Relies on access to environment variables set on the edge",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See: https://github.com/vercel/next.js/blob/canary/packages/next/src/server/web/get-edge-preview-props.ts

Copy link
Contributor

@pieh pieh May 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is something we can do to actually make it work and possibly this would also handle some other cases that rely on env vars like that:

This sets some "PreviewProps" at build time with a bit of random values
https://github.com/vercel/next.js/blob/f3f0a03e820eecaa15a8067791e930f58d4a03c4/packages/next/src/build/index.ts#L1108-L1113

And this looks like it tries to set them in built middleware
https://github.com/vercel/next.js/blob/f3f0a03e820eecaa15a8067791e930f58d4a03c4/packages/next/src/build/webpack-config.ts#L2009-L2022

Then those env vars seems to be defined in middleware-manifest.json#middleware[].env - for example like this:

{
  "version": 3,
  "middleware": {
    "/": {
      "files": [
        "server/edge-runtime-webpack.js",
        "server/middleware-f3956634.js",
        "server/middleware-2971c211.js",
        "server/middleware-7fce854e.js",
        "server/middleware-89adb3be.js",
        "server/middleware-7874eb72.js",
        "server/middleware-27161c75.js",
        "server/middleware-d1744647.js",
        "server/middleware-16b16fa0.js",
        "server/middleware-8bfe7224.js",
        "server/middleware-b9f70e77.js",
        "server/middleware-377fed06.js"
      ],
      "name": "middleware",
      "page": "/",
      "matchers": [
        {
          "regexp": "^(?:\\/(_next\\/data\\/[^/]{1,}))?\\/test(?:\\/((?:[^\\/#\\?]+?)(?:\\/(?:[^\\/#\\?]+?))*))?(\\.json)?[\\/#\\?]?$",
          "originalSource": "/test/:path*"
        }
      ],
      "wasm": [],
      "assets": [],
      "env": {
        // we have env vars to set, but we don't set them
        "__NEXT_BUILD_ID": "jz-SjGjUVmhXJqhfMaRsg",
        "NEXT_SERVER_ACTIONS_ENCRYPTION_KEY": "TNX5YdWxrvsdPSUR7O84uW1PWttdF3O8O2eWT4G1VpU=",
        "__NEXT_PREVIEW_MODE_ID": "94e82426a39aa9b691f468ba3e0a61a4",
        "__NEXT_PREVIEW_MODE_ENCRYPTION_KEY": "d9975864b9708a047ca1cc8d1baf3c86f98ec4cecad4c98959c8e8ade0ed9574",
        "__NEXT_PREVIEW_MODE_SIGNING_KEY": "152fc2b4ce081946d22431113018c8be384692e2f90af6b31fa61769bf1e4834"
      }
    }
  },
  "functions": {},
  "sortedMiddleware": [
    "/"
  ]
}

And the part that we would need to do is to ensure that we set those process.env.X values from this manifest somewhere in https://github.com/opennextjs/opennextjs-netlify/blob/main/src/build/functions/edge.ts as we don't seem to handle env portion of manifest currently. We do handle matchers, name, files and wasm, but not env (and also not assets which does have #2697 for it)

"tests": [
"app-dir with middleware Mutate request headers for Serverless Functions Supports draft mode",
"app-dir with middleware Mutate request headers for Edge Functions Supports draft mode",
"app-dir with middleware Mutate request headers for next/headers Supports draft mode"
]
}
],
"failures": [
Expand Down
Loading