fix encoding error with location and refresh headers

[jamsinclair]

Bug

• Related issues linked using fixes #number
• Integration tests added

Edit: Looks like this may've been a regression of #17907?

• Fixes TypeError [ERR_INVALID_CHAR]: Invalid character in header content ["Location"] #24977
• Fixes TypeError [ERR_INVALID_CHAR]: Invalid character in header content ["Location"] #24047

Minimal reproduction with a next app: https://github.com/jamsinclair/nextjs-refresh-header-bug

Redirected URLs with search param keys containing characters outside of the default ASCII and utf-8 encodings would throw a server 500 error.
e.g. a redirect URL with /?テスト would error.

The errors serverside were either: TypeError [ERR_INVALID_CHAR]: Invalid character in header content ["Location"] or TypeError [ERR_INVALID_CHAR]: Invalid character in header content ["Refresh"]

Solution

The issue arose as we were only re-encoding the search param values and not the keys.

next.js/packages/next/server/next-server.ts

Lines 866 to 870 in b6c590b

	encodeURIComponent(value) {
	if (initialQueryValues.some((val) => val === value)) {
	return encodeURIComponent(value)
	}
	return value

Updating the stringify method to also encode the search param keys seems to have resolved the issue.

For the future, it may be worth enforcing stricter assumptions around search params and standardising everywhere with theURLSearchParams interface. Not confident I can lead that refactor 😅

[ihmpavel]

Hi, do you plan to merge current canary and fix test suites? We are experiencing same problem with content-disposition header. I will be happy to help you to get this merged 🙏

[ihmpavel]

Hi, that looks like some tests are still failing (lint/prettier). Could you please recheck it? Or are they irrelevant to this PR?

[ijjk]

Failing test suites

Commit: 1447ec4

test/e2e/basepath.test.ts

• basePath > should use urls with basepath in router events for hash changes
• basePath > should use urls with basepath in router events for cancelled routes
• basePath > should use urls with basepath in router events for failed route change

Expand output

● basePath › should use urls with basepath in router events for hash changes

expect(received).toEqual(expected) // deep equality

- Expected  -  0
+ Received  + 21

@@ -1,7 +1,28 @@
  Array [
    Array [
+     "routeChangeStart",
+     "/docs/hello",
+     Object {
+       "shallow": true,
+     },
+   ],
+   Array [
+     "beforeHistoryChange",
+     "/docs/hello",
+     Object {
+       "shallow": true,
+     },
+   ],
+   Array [
+     "routeChangeComplete",
+     "/docs/hello",
+     Object {
+       "shallow": true,
+     },
+   ],
+   Array [
      "hashChangeStart",
      "/docs/hello#some-hash",
      Object {
        "shallow": false,
      },

  713 |
  714 |         const eventLog = await browser.eval('window._getEventLog()')
> 715 |         expect(eventLog).toEqual([
      |                          ^
  716 |           [
  717 |             'hashChangeStart',
  718 |             `${basePath}/hello#some-hash`,

  at Object.<anonymous> (e2e/basepath.test.ts:715:26)

● basePath › should use urls with basepath in router events for cancelled routes

expect(received).toEqual(expected) // deep equality

- Expected  -  0
+ Received  + 21

@@ -1,8 +1,29 @@
  Array [
    Array [
      "routeChangeStart",
+     "/docs/hello",
+     Object {
+       "shallow": true,
+     },
+   ],
+   Array [
+     "beforeHistoryChange",
+     "/docs/hello",
+     Object {
+       "shallow": true,
+     },
+   ],
+   Array [
+     "routeChangeComplete",
+     "/docs/hello",
+     Object {
+       "shallow": true,
+     },
+   ],
+   Array [
+     "routeChangeStart",
      "/docs/slow-route",
      Object {
        "shallow": false,
      },
    ],

  742 |
  743 |         const eventLog = await browser.eval('window._getEventLog()')
> 744 |         expect(eventLog).toEqual([
      |                          ^
  745 |           ['routeChangeStart', `${basePath}/slow-route`, { shallow: false }],
  746 |           [
  747 |             'routeChangeError',

  at Object.<anonymous> (e2e/basepath.test.ts:744:26)

● basePath › should use urls with basepath in router events for failed route change

expect(received).toEqual(expected) // deep equality

- Expected  -  0
+ Received  + 21

@@ -1,8 +1,29 @@
  Array [
    Array [
      "routeChangeStart",
+     "/docs/hello",
+     Object {
+       "shallow": true,
+     },
+   ],
+   Array [
+     "beforeHistoryChange",
+     "/docs/hello",
+     Object {
+       "shallow": true,
+     },
+   ],
+   Array [
+     "routeChangeComplete",
+     "/docs/hello",
+     Object {
+       "shallow": true,
+     },
+   ],
+   Array [
+     "routeChangeStart",
      "/docs/error-route",
      Object {
        "shallow": false,
      },
    ],

  769 |
  770 |         const eventLog = await browser.eval('window._getEventLog()')
> 771 |         expect(eventLog).toEqual([
      |                          ^
  772 |           ['routeChangeStart', `${basePath}/error-route`, { shallow: false }],
  773 |           [
  774 |             'routeChangeError',

  at Object.<anonymous> (e2e/basepath.test.ts:771:26)

[ihmpavel]

Hi, thank you for your changes. The test results with fixed prettier are weird, what do you think? Everything should be successful, but is not. Maybe reviewers should look into this?

[jamsinclair]

Thanks @ihmpavel for helping push this PR along.

It looks like the failing tests are unrelated and can be seen in failing in other new PRs.

@styfle I know couple months back you helped with another encoding issue (#27671)? Would you be able to spare some time to give some feedback? 🙏

[ihmpavel]

I was hoping your PR will fix my issue, but I was wrong 😄 Your last comment pointed me into right direction. Thank you. (I created a new issue.)

[styfle]

Looks good to me, thanks!

[jamsinclair]

@styfle Thanks for the review! I fixed the merge conflicts, the other day. Feel free to take this long running PR across the finish line when you get a chance 🙏

[ijjk]

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	jamsinclair/next.js fix-location-header-invalid-char	Change
buildDuration	21.6s	22.2s	⚠️ +603ms
buildDurationCached	4.5s	4.3s	-130ms
nodeModulesSize	336 MB	336 MB	⚠️ +479 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	jamsinclair/next.js fix-location-header-invalid-char	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.498	3.647	⚠️ +0.15
/ avg req/sec	714.63	685.58	⚠️ -29.05
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.736	1.69	-0.05
/error-in-render avg req/sec	1440.01	1478.95	+38.94

Client Bundles (main, webpack, commons)

	vercel/next.js canary	jamsinclair/next.js fix-location-header-invalid-char	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	28.3 kB	28.3 kB	✓
webpack-HASH.js gzip	1.45 kB	1.45 kB	✓
Overall change	72.2 kB	72.2 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	jamsinclair/next.js fix-location-header-invalid-char	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	jamsinclair/next.js fix-location-header-invalid-char	Change
_app-HASH.js gzip	1.37 kB	1.37 kB	✓
_error-HASH.js gzip	194 B	194 B	✓
amp-HASH.js gzip	312 B	312 B	✓
css-HASH.js gzip	327 B	327 B	✓
dynamic-HASH.js gzip	2.38 kB	2.38 kB	✓
head-HASH.js gzip	350 B	350 B	✓
hooks-HASH.js gzip	635 B	635 B	✓
image-HASH.js gzip	4.44 kB	4.44 kB	✓
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	1.87 kB	1.87 kB	✓
routerDirect..HASH.js gzip	321 B	321 B	✓
script-HASH.js gzip	383 B	383 B	✓
withRouter-HASH.js gzip	318 B	318 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	13.3 kB	13.3 kB	✓

Client Build Manifests

	vercel/next.js canary	jamsinclair/next.js fix-location-header-invalid-char	Change
_buildManifest.js gzip	460 B	460 B	✓
Overall change	460 B	460 B	✓

Rendered Page Sizes

	vercel/next.js canary	jamsinclair/next.js fix-location-header-invalid-char	Change
index.html gzip	523 B	523 B	✓
link.html gzip	536 B	536 B	✓
withRouter.html gzip	517 B	517 B	✓
Overall change	1.58 kB	1.58 kB	✓

---

Default Build with SWC (Decrease detected ✓)

General Overall increase ⚠️

	vercel/next.js canary	jamsinclair/next.js fix-location-header-invalid-char	Change
buildDuration	23.4s	23.2s	-213ms
buildDurationCached	4.4s	4.3s	-22ms
nodeModulesSize	336 MB	336 MB	⚠️ +479 B

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	jamsinclair/next.js fix-location-header-invalid-char	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.617	3.61	-0.01
/ avg req/sec	691.13	692.54	+1.41
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.642	1.804	⚠️ +0.16
/error-in-render avg req/sec	1522.62	1385.45	⚠️ -137.17

Client Bundles (main, webpack, commons)

	vercel/next.js canary	jamsinclair/next.js fix-location-header-invalid-char	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.3 kB	42.3 kB	✓
main-HASH.js gzip	28.5 kB	28.5 kB	✓
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	72.5 kB	72.5 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	jamsinclair/next.js fix-location-header-invalid-char	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	jamsinclair/next.js fix-location-header-invalid-char	Change
_app-HASH.js gzip	1.35 kB	1.35 kB	✓
_error-HASH.js gzip	180 B	180 B	✓
amp-HASH.js gzip	305 B	305 B	✓
css-HASH.js gzip	321 B	321 B	✓
dynamic-HASH.js gzip	2.38 kB	2.38 kB	✓
head-HASH.js gzip	342 B	342 B	✓
hooks-HASH.js gzip	622 B	622 B	✓
image-HASH.js gzip	4.46 kB	4.46 kB	✓
index-HASH.js gzip	256 B	256 B	✓
link-HASH.js gzip	1.91 kB	1.91 kB	✓
routerDirect..HASH.js gzip	314 B	314 B	✓
script-HASH.js gzip	375 B	375 B	✓
withRouter-HASH.js gzip	309 B	309 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	13.2 kB	13.2 kB	✓

Client Build Manifests

	vercel/next.js canary	jamsinclair/next.js fix-location-header-invalid-char	Change
_buildManifest.js gzip	460 B	460 B	✓
Overall change	460 B	460 B	✓

Rendered Page Sizes

	vercel/next.js canary	jamsinclair/next.js fix-location-header-invalid-char	Change
index.html gzip	524 B	524 B	✓
link.html gzip	535 B	535 B	✓
withRouter.html gzip	517 B	517 B	✓
Overall change	1.58 kB	1.58 kB	✓

Commit: d4ba24b

[timneutkens]

Just verified this and it looks good, thanks 💯

[timneutkens]

Thanks @jamsinclair! I've opened #33763 where the merge conflict is solved. Added you as co-author ofcourse 👍