fix: properly encode url with unicode characters (#1291)

LinusU · web-flow · commit ace7536c9555 · 2021-09-20T16:09:10.000+02:00
* fix: properly encode url with unicode characters
* release: 2.6.3
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,10 @@ Changelog
 
 # 2.x release
 
+## v2.6.3
+
+- Fix: properly encode url with unicode characters
+
 ## v2.6.2
 
 - Fix: used full filename for main in package.json
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
     "name": "node-fetch",
-    "version": "2.6.2",
+    "version": "2.6.3",
     "description": "A light-weight module that brings window.fetch to node.js",
     "main": "lib/index.js",
     "browser": "./browser.js",
@@ -36,6 +36,9 @@
         "url": "https://github.com/bitinn/node-fetch/issues"
     },
     "homepage": "https://github.com/bitinn/node-fetch",
+    "dependencies": {
+        "whatwg-url": "^5.0.0"
+    },
     "devDependencies": {
         "@ungap/url-search-params": "^0.1.2",
         "abort-controller": "^1.1.0",
@@ -60,7 +63,6 @@
         "rollup": "^0.63.4",
         "rollup-plugin-babel": "^3.0.7",
         "string-to-arraybuffer": "^1.0.2",
-        "teeny-request": "3.7.0",
-        "whatwg-url": "^5.0.0"
+        "teeny-request": "3.7.0"
     }
 }
diff --git a/rollup.config.js b/rollup.config.js
@@ -1,9 +1,12 @@
 import isBuiltin from 'is-builtin-module';
 import babel from 'rollup-plugin-babel';
+import packageJson from './package.json';
 import tweakDefault from './build/rollup-plugin';
 
 process.env.BABEL_ENV = 'rollup';
 
+const dependencies = Object.keys(packageJson.dependencies);
+
 export default {
   input: 'src/index.js',
   output: [
@@ -18,6 +21,6 @@ export default {
     tweakDefault()
   ],
   external: function (id) {
-    return isBuiltin(id);
+    return dependencies.includes(id) || isBuiltin(id);
   }
 };
diff --git a/src/request.js b/src/request.js
@@ -9,6 +9,7 @@
 
 import Url from 'url';
 import Stream from 'stream';
+import {URL} from 'whatwg-url';
 import Headers, { exportNodeCompatibleHeaders } from './headers.js';
 import Body, { clone, extractContentType, getTotalBytes } from './body';
 
@@ -18,6 +19,39 @@ const INTERNALS = Symbol('Request internals');
 const parse_url = Url.parse;
 const format_url = Url.format;
 
+/**
+ * Wrapper around `new URL` to handle arbitrary URLs
+ *
+ * @param  {string} urlStr
+ * @return {void}
+ */
+function parseURL(urlStr) {
+	/*
+		Check whether the URL is absolute or not
+
+		Scheme: https://tools.ietf.org/html/rfc3986#section-3.1
+		Absolute URL: https://tools.ietf.org/html/rfc3986#section-4.3
+	*/
+	if (/^[a-zA-Z][a-zA-Z\d+\-.]*:/.exec(urlStr)) {
+		const url = new URL(urlStr);
+
+		return {
+			path: url.pathname,
+			pathname: url.pathname,
+			hostname: url.hostname,
+			protocol: url.protocol,
+			port: url.port,
+			hash: url.hash,
+			search: url.search,
+			query: url.query,
+			href: url.href,
+		}
+	}
+
+	// Fallback to old implementation for arbitrary URLs
+	return parse_url(urlStr);
+}
+
 const streamDestructionSupported = 'destroy' in Stream.Readable.prototype;
 
 /**
@@ -59,14 +93,14 @@ export default class Request {
 				// in order to support Node.js' Url objects; though WHATWG's URL objects
 				// will fall into this branch also (since their `toString()` will return
 				// `href` property anyway)
-				parsedURL = parse_url(input.href);
+				parsedURL = parseURL(input.href);
 			} else {
 				// coerce input to a string before attempting to parse
-				parsedURL = parse_url(`${input}`);
+				parsedURL = parseURL(`${input}`);
 			}
 			input = {};
 		} else {
-			parsedURL = parse_url(input.url);
+			parsedURL = parseURL(input.url);
 		}
 
 		let method = init.method || input.method || 'GET';
diff --git a/test/server.js b/test/server.js
@@ -32,7 +32,7 @@ export default class TestServer {
 	}
 
 	router(req, res) {
-		let p = parse(req.url).pathname;
+		let p = decodeURIComponent(parse(req.url).pathname);
 
 		if (p === '/hello') {
 			res.statusCode = 200;
@@ -384,6 +384,12 @@ export default class TestServer {
 			});
 			req.pipe(parser);
 		}
+
+		if (p === '/issues/1290/ひらがな') {
+			res.statusCode = 200;
+			res.setHeader('Content-Type', 'text/plain');
+			res.end('Success');
+		}
 	}
 }
 
diff --git a/test/test.js b/test/test.js
@@ -2845,3 +2845,25 @@ describe('external encoding', () => {
 		});
 	});
 });
+
+describe('issue #1290', function() {
+	it('should handle escaped unicode in URLs', () => {
+		const url = `${base}issues/1290/%E3%81%B2%E3%82%89%E3%81%8C%E3%81%AA`;
+		return fetch(url).then((res) => {
+			expect(res.status).to.equal(200);
+			return res.text().then(result => {
+				expect(result).to.equal('Success');
+			});
+		});
+	});
+
+	it('should handle unicode in URLs', () => {
+		const url = `${base}issues/1290/ひらがな`;
+		return fetch(url).then((res) => {
+			expect(res.status).to.equal(200);
+			return res.text().then(result => {
+				expect(result).to.equal('Success');
+			});
+		});
+	});
+});

Original file line number	Diff line number	Diff line change
`@@ -32,7 +32,7 @@ export default class TestServer {`
`32`	`32`	`}`
`33`	`33`
`34`	`34`	`router(req, res) {`
`35`		`- let p = parse(req.url).pathname;`
	`35`	`+ let p = decodeURIComponent(parse(req.url).pathname);`
`36`	`36`
`37`	`37`	`if (p === '/hello') {`
`38`	`38`	`res.statusCode = 200;`
`@@ -384,6 +384,12 @@ export default class TestServer {`
`384`	`384`	`});`
`385`	`385`	`req.pipe(parser);`
`386`	`386`	`}`
	`387`	`+`
	`388`	`+ if (p === '/issues/1290/ひらがな') {`
	`389`	`+ res.statusCode = 200;`
	`390`	`+ res.setHeader('Content-Type', 'text/plain');`
	`391`	`+ res.end('Success');`
	`392`	`+ }`
`387`	`393`	`}`
`388`	`394`	`}`
`389`	`395`