fix: Normalize the Location header in redirects

[sommeeeer]

Currently in draft mode. It will pass all our E2E tests, but requires more manual testing. Especially the middleware part.

For #940 and opennextjs/opennextjs-cloudflare#809

For redirects coming from next config we want that the Location header value is encoded properly according to RFC. In next start and deployed to Vercel the header can also be relative or absolute. We should aim for the same behavior.

I am a bit unsure what to do about NextResponse.redirect() coming from the middleware. Should we encode it aswell? I'll keep investigating this but are open for suggestions.

Update: I've done a lot of manual testing now, and made some improvements along the way. We seem to be matching the behavior of next start now.

They do encode redirects from the middleware aswell. They also normalize it. It only accepts absolute URLs aswell. Doesn't matter if you use Response.redirect(), NextResponse.redirect() or create your own new Response(null, .... The behavior seems to be the same.

Interesting enough if you have this Response in the middleware it will crash with TypeError: Invalid URL:

if (url.pathname.startsWith('/self-redirect')) {
  return new Response(null, {
    status: 302,
    headers: {
      // This will TypeError: Invalid URL
      // It seems to only accept absolute URL like with NextResponse.redirect() and Response.redirect()
      // However you will not get the error thrown in validateURL shown below
      location: '/hoho',
    },
  });
}

With NextResponse.redirect and Response.redirect it will try to validate the URL with this function in Next.js: validateURL. You will get sent to this docs page explaining you need to use absolute URLs: https://nextjs.org/docs/messages/middleware-relative-urls

[changeset-bot]

🦋 Changeset detected

Latest commit: 6092aaf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@opennextjs/aws	Patch
app-pages-router	Patch
app-router	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[pkg-pr-new]

Open in StackBlitz

pnpm add https://pkg.pr.new/@opennextjs/aws@941

commit: 6092aaf

[leog]

Confirmed it works as expected using your changes @sommeeeer:
leog/OpenNext-AWS-Normalize-the-Location-header-in-redirects@540caa0

https://d13iq8evz1d8fc.cloudfront.net/about?param=test%2F%2F

Very promising, thank you! 💪

[conico974]

A few nits. Also, could you add unit test for the normalizeLocationHeader function

[sommeeeer]

I turned this into draft mode again. I need to do some more thouroghly testing as it seems I overlooked the fact that Next actually does not encode the search paramaters in the Location header coming from the middleware.

Update: The PR is ready now we should be matching Next behavior now. They encode query params in redirects from Next Config, but not with URLSearchParams. They use stringify from node:querystring. Query parameters in Redirects from the middleware is not touched.

[conico974]

LGTM Thanks!!

[sommeeeer]

Thanks for the review Nico!