fix(fetch): properly redirect non-ascii location header url #2971

Xvezda · 2024-03-18T16:51:32Z

This relates to...

Rationale

The fetch function behaves differently from major web browsers(chrome, safari, firefox etc), and even CURL (with -L option).

Here is a minimal reproducible URL I made: https://non-ascii-location-header.sys.workers.dev/redirect

Worker code

export default {
	async fetch(request: Request, env: Env, ctx: ExecutionContext): Promise<Response> {
		try {
			const url = new URL(request.url);
			if (url.pathname === '/redirect') {
				return new Response('', {
					status: 302,
					headers: {
						Location: '/안녕',
					},
				});
			}
			return new Response(`pathname ${url.pathname}`);
		} catch (e) {
			return new Response(`error: ${(e as Error).message}`, { status: 500 });
		}
	},
};

By accessing the address at https://non-ascii-location-header.sys.workers.dev/redirect directly
or https://non-ascii-location-header.sys.workers.dev/ first and then making a fetch request to the /redirect path using the developer tools leads to reaching https://non-ascii-location-header.sys.workers.dev/%EC%95%88%EB%85%95
which shows pathname /%EC%95%88%EB%85%95 as a result.

However, using undici fetch function does not behave in the same way.

> r = await fetch('https://non-ascii-location-header.sys.workers.dev/redirect')
... await r.text();
pathname /%C3%AC%C2%95%C2%88%C3%AB%C2%85%C2%95

This issue can also be observed at chrome web store: https://chromewebstore.google.com/detail/gbgmenmdglilmbmemagekpeaodajbeei

Welcome to Node.js v21.6.2.
Type ".help" for more information.
> await fetch('https://chromewebstore.google.com/detail/gbgmenmdglilmbmemagekpeaodajbeei')
Uncaught TypeError: fetch failed
    at node:internal/deps/undici/undici:12443:11
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async REPL1:1:33 {
  cause: Error: redirect count exceeded
      at makeNetworkError (node:internal/deps/undici/undici:5675:35)
      at httpRedirectFetch (node:internal/deps/undici/undici:10696:32)
      at httpFetch (node:internal/deps/undici/undici:10668:28)
      at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
      at async node:internal/deps/undici/undici:10440:20
      at async mainFetch (node:internal/deps/undici/undici:10430:20)
      at async httpFetch (node:internal/deps/undici/undici:10668:22)
      at async node:internal/deps/undici/undici:10440:20
      at async mainFetch (node:internal/deps/undici/undici:10430:20)
      at async httpFetch (node:internal/deps/undici/undici:10668:22)
}

I do not have an in-depth understanding of the fetch standard, so if this PR request violates the standard in any way, please feel free to close it.

Thank you.

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

KhafraDev

this is correct spec-wise

KhafraDev · 2024-03-18T21:39:07Z

@anonrig do you know why this is needed?

anonrig · 2024-03-18T21:48:05Z

cc @lemire

anonrig · 2024-03-18T21:48:35Z

Please don't merge it until I have some time to review this tomorrow.

KhafraDev · 2024-03-18T22:03:14Z

> base = `http://127.0.0.1:51619/redirect`
'http://127.0.0.1:51619/redirect'
> location = `/ì\x95\x88ë\x85\x95`
'/ì\x95\x88ë\x85\x95'
> new URL(location, base).toString()
'http://127.0.0.1:51619/%C3%AC%C2%95%C2%88%C3%AB%C2%85%C2%95'

I am confused

anonrig

@KhafraDev This is intended, and the output is correct. Look at the example below:

> let u = new URL('http://127.0.0.1:51619/redirect')
undefined
> u.pathname = '/ì\x95\x88ë\x85\x95'
'/ì\x95\x88ë\x85\x95'
> u
URL {
  href: 'http://127.0.0.1:51619/%C3%AC%C2%95%C2%88%C3%AB%C2%85%C2%95',
  origin: 'http://127.0.0.1:51619',
  protocol: 'http:',
  username: '',
  password: '',
  host: '127.0.0.1:51619',
  hostname: '127.0.0.1',
  port: '51619',
  pathname: '/%C3%AC%C2%95%C2%88%C3%AB%C2%85%C2%95',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}

URL specification path state (https://url.spec.whatwg.org/#path-state) says that:

UTF-8 percent-encode c using the path percent-encode set and append the result to buffer.

anonrig · 2024-03-18T22:32:53Z

You can replicate the same behavior on Safari as well.

lemire · 2024-03-18T22:56:54Z

lib/web/fetch/util.js

@@ -44,7 +44,8 @@ function responseLocationURL (response, requestFragment) {
  // 3. If location is a header value, then set location to the result of
  //    parsing location with response’s URL.
  if (location !== null && isValidHeaderValue(location)) {
-    location = new URL(location, responseURL(response))
+    // Make sure location is properly encoded.
+    location = new URL(Buffer.from(location, 'ascii').toString('utf8'), responseURL(response))


This seems strange to me: Buffer.from(location, 'ascii').toString('utf8'). I suggest at least some comments to explain what the intention is.

What does "parsing location with response URL" mean precisely? I interpret it is a pathname setter...

The spec links "parsing" to https://url.spec.whatwg.org/#concept-url-parser

@lemire
This issue arises when the location header's address is not encoded as with encodeURI, but is responded in binary form.
For example, while the result of encodeURIComponent('안녕') is %EC%95%88%EB%85%95, in binary form, this appears as \xEC\x95\x88\xEB\x85\x95, same as the outcome of Buffer.from('안녕', 'utf8').toString('binary').
What I am attempting in this code is to normalize characters that exist outside the ASCII range into UTF-8 form and URL encode them, to ensure behavior consistent with major web browsers.

@KhafraDev Yes. Parsing with the base will work the same in ada, test here : ada-url/ada#611

KhafraDev · 2024-03-19T01:36:03Z

This is intended, and the output is correct. Look at the example below:

This seems like quite a bad issue if it's causing pages not to load. How does Safari handle loading the google webstore in this case (example)?

Xvezda · 2024-03-19T09:17:51Z

I've added comments to make the code clearer and revised it to be more concise. Please take a look.

lib/web/fetch/util.js

lemire · 2024-03-19T13:46:17Z

lib/web/fetch/util.js

+ */
+function normalizeBinaryStringToUtf8 (value) {
+  return Buffer.from(value, 'binary').toString('utf8')
+}


let b = Buffer.from(value, 'binary'); if(Buffer.isAscii(b)) { return value; } // skip new string alloc. return b.toString('utf8'); // must alloc a new string

No need to call Buffer.from(). Buffer.isAscii(b) is a static function.

@anonrig We get at the core of the issue here. The value is a string not a buffer. At some point it was wrongly interpreted as latin1 (maybe in http) and so we must now convert it to a buffer and then back to a string.

I think that this function should receive a Buffer and then the Buffer could be interpreted as UTF-8 and we'd be done.

But this is far beyond the scope of this PR.

The irony is that we seem to try to purposefully (in http) prevent people from sending UTF-8 content as UTF-8... and then we go back and say "no, it is really UTF-8".

Much technical debt.

mcollina

lgtm

lib/web/fetch/util.js

See: nodejs#2971 (comment)

* adding test for nodejs/undici#2971 * lint

mcollina · 2024-03-21T09:40:17Z

@anonrig PTAL

) * fix(fetch): properly redirect non-ascii location header url * chore: fix typo * test: use simpler code * chore: clarify what the code does * chore: add comment * chore: normalize location url only if it contains invalid character * chore: apply suggestion See: nodejs#2971 (comment) * chore: remove redundant condition check

Xvezda added 2 commits March 19, 2024 00:52

fix(fetch): properly redirect non-ascii location header url

e07ade3

chore: fix typo

48fd3c1

KhafraDev approved these changes Mar 18, 2024

View reviewed changes

anonrig self-requested a review March 18, 2024 21:48

anonrig requested changes Mar 18, 2024

View reviewed changes

lemire reviewed Mar 18, 2024

View reviewed changes

Xvezda added 2 commits March 19, 2024 12:53

test: use simpler code

01ceae8

chore: clarify what the code does

6d5f770

chore: add comment

ea34bc2

lemire added a commit to ada-url/ada that referenced this pull request Mar 19, 2024

adding test for nodejs/undici#2971

fc23bb0

lemire mentioned this pull request Mar 19, 2024

adding test for undici issue 971 ada-url/ada#611

Merged

lemire reviewed Mar 19, 2024

View reviewed changes

lib/web/fetch/util.js Outdated Show resolved Hide resolved

lemire reviewed Mar 19, 2024

View reviewed changes

mcollina approved these changes Mar 19, 2024

View reviewed changes

chore: normalize location url only if it contains invalid character

efccdde

lemire reviewed Mar 19, 2024

View reviewed changes

lib/web/fetch/util.js Outdated Show resolved Hide resolved

chore: apply suggestion

45f2e48

See: nodejs#2971 (comment)

KhafraDev approved these changes Mar 19, 2024

View reviewed changes

chore: remove redundant condition check

3519ca1

lemire added a commit to ada-url/ada that referenced this pull request Mar 19, 2024

adding test for undici issue 971 (#611)

0170547

* adding test for nodejs/undici#2971 * lint

KhafraDev approved these changes Mar 20, 2024

View reviewed changes

anonrig approved these changes Mar 21, 2024

View reviewed changes

mcollina merged commit 1b62571 into nodejs:main Mar 21, 2024
21 of 22 checks passed

ojoched mentioned this pull request Apr 25, 2024

ERR_FR_TOO_MANY_REDIRECTS using Axios but not in browser, using curl or python-requests axios/axios#6367

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(fetch): properly redirect non-ascii location header url #2971

fix(fetch): properly redirect non-ascii location header url #2971

Xvezda commented Mar 18, 2024 •

edited

Loading

KhafraDev left a comment

KhafraDev commented Mar 18, 2024

anonrig commented Mar 18, 2024

anonrig commented Mar 18, 2024

KhafraDev commented Mar 18, 2024 •

edited

Loading

anonrig left a comment

anonrig commented Mar 18, 2024

lemire Mar 18, 2024 •

edited

Loading

lemire Mar 18, 2024 •

edited

Loading

KhafraDev Mar 19, 2024

Xvezda Mar 19, 2024 •

edited

Loading

lemire Mar 19, 2024

KhafraDev commented Mar 19, 2024

Xvezda commented Mar 19, 2024

lemire Mar 19, 2024 •

edited

Loading

anonrig Mar 19, 2024

lemire Mar 19, 2024

lemire Mar 19, 2024

lemire Mar 19, 2024

mcollina left a comment

mcollina commented Mar 21, 2024

fix(fetch): properly redirect non-ascii location header url #2971

fix(fetch): properly redirect non-ascii location header url #2971

Conversation

Xvezda commented Mar 18, 2024 • edited Loading

This relates to...

Rationale

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

KhafraDev left a comment

Choose a reason for hiding this comment

KhafraDev commented Mar 18, 2024

anonrig commented Mar 18, 2024

anonrig commented Mar 18, 2024

KhafraDev commented Mar 18, 2024 • edited Loading

anonrig left a comment

Choose a reason for hiding this comment

anonrig commented Mar 18, 2024

lemire Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

lemire Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

KhafraDev Mar 19, 2024

Choose a reason for hiding this comment

Xvezda Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

lemire Mar 19, 2024

Choose a reason for hiding this comment

KhafraDev commented Mar 19, 2024

Xvezda commented Mar 19, 2024

lemire Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

anonrig Mar 19, 2024

Choose a reason for hiding this comment

lemire Mar 19, 2024

Choose a reason for hiding this comment

lemire Mar 19, 2024

Choose a reason for hiding this comment

lemire Mar 19, 2024

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Mar 21, 2024

Xvezda commented Mar 18, 2024 •

edited

Loading

KhafraDev commented Mar 18, 2024 •

edited

Loading

lemire Mar 18, 2024 •

edited

Loading

lemire Mar 18, 2024 •

edited

Loading

Xvezda Mar 19, 2024 •

edited

Loading

lemire Mar 19, 2024 •

edited

Loading