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

* 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

Author: Xvezda

lib/web/fetch/util.js

@@ -44,6 +44,12 @@ 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)) {
+    if (!isValidEncodedURL(location)) {
+      // Some websites respond location header in UTF-8 form without encoding them as ASCII
+      // and major browsers redirect them to correctly UTF-8 encoded addresses.
+      // Here, we handle that behavior in the same way.
+      location = normalizeBinaryStringToUtf8(location)
+    }
     location = new URL(location, responseURL(response))
   }
 
@@ -57,6 +63,36 @@ function responseLocationURL (response, requestFragment) {
   return location
 }
 
+/**
+ * @see https://www.rfc-editor.org/rfc/rfc1738#section-2.2
+ * @param {string} url
+ * @returns {boolean}
+ */
+function isValidEncodedURL (url) {
+  for (const c of url) {
+    const code = c.charCodeAt(0)
+    // Not used in US-ASCII
+    if (code >= 0x80) {
+      return false
+    }
+    // Control characters
+    if ((code >= 0x00 && code <= 0x1F) || code === 0x7F) {
+      return false
+    }
+  }
+  return true
+}
+
+/**
+ * If string contains non-ASCII characters, assumes it's UTF-8 encoded and decodes it.
+ * Since UTF-8 is a superset of ASCII, this will work for ASCII strings as well.
+ * @param {string} value
+ * @returns {string}
+ */
+function normalizeBinaryStringToUtf8 (value) {
+  return Buffer.from(value, 'binary').toString('utf8')
+}
+
 /** @returns {URL} */
 function requestCurrentURL (request) {
   return request.urlList[request.urlList.length - 1]

test/fetch/fetch-url-after-redirect.js

@@ -38,3 +38,24 @@ test('after redirecting the url of the response is set to the target url', async
 
   assert.strictEqual(response.url, `http://127.0.0.1:${port}/target`)
 })
+
+test('location header with non-ASCII character redirects to a properly encoded url', async (t) => {
+  // redirect -> %EC%95%88%EB%85%95 (안녕), not %C3%AC%C2%95%C2%88%C3%AB%C2%85%C2%95
+  const server = createServer((req, res) => {
+    if (res.req.url.endsWith('/redirect')) {
+      res.writeHead(302, undefined, { Location: `/${Buffer.from('안녕').toString('binary')}` })
+      res.end()
+    } else {
+      res.writeHead(200, 'dummy', { 'Content-Type': 'text/plain' })
+      res.end()
+    }
+  })
+  t.after(closeServerAsPromise(server))
+
+  const listenAsync = promisify(server.listen.bind(server))
+  await listenAsync(0)
+  const { port } = server.address()
+  const response = await fetch(`http://127.0.0.1:${port}/redirect`)
+
+  assert.strictEqual(response.url, `http://127.0.0.1:${port}/${encodeURIComponent('안녕')}`)
+})