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

[Xvezda]

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

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

[KhafraDev]

this is correct spec-wise

[KhafraDev]

@anonrig do you know why this is needed?

[anonrig]

cc @lemire

[anonrig]

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

[KhafraDev]

> 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]

You can replicate the same behavior on Safari as well.

[lemire]

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

[lemire]

[KhafraDev]

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

[Xvezda]

@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.

[lemire]

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

[KhafraDev]

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]

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

[lemire]

  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

[anonrig]

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

[lemire]

@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.

[lemire]

But this is far beyond the scope of this PR.

[lemire]

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

[mcollina]

@anonrig PTAL