Repository: node-fetch/node-fetch
Pull request #853 (2020-05-29 19:13:52)

Fix Data URI handling and drop all URL analysis RegExps
This PR adds a breaking test for existing dataUri implementation and reimplements it without using RegExp and also drops them in Request, following browsers logic on this case.

Only changes to tests are breaking case (from Wikipedia) for current implementation and adjusting of error messages as they are coming from native URL parser now (unsupported protocol message is copied from Chrome `fetch`)



tinovyatkin: @jimmywarting well, install size is not smaller. But spec compliance is way better, as seen in Wikipedia example (on the mimeType part). I believe maintenance is more future-proof too...

jimmywarting: I'm not 100% convinced yet, just b/c it has more downloads doesn't really mean it's more popular or faster, often with the case of such packages they get backed up by other bigger modules like jsDom to make them seem more popular, like how babel used left-pad for example. 

we haven't received any complains yet of some incompatible data url, so why should we change something that is already working?

in cases like this I would want to see some benchmarking too to compare both packages.

I'm also keen to help data-uri-to-buffer with PR if it's doing something wrong. (just created an [issue](https://github.com/TooTallNate/node-data-uri-to-buffer/issues/14) wondering if he still needs bufferFrom and offered to help)

Think there are better things you can put your effort into, if you like i can assign some issues to you
tinovyatkin: This PR is to fix another bug (see the breaking test), dependency can stay as it, they are little different in mime types handling, but as I understand it doesn’t matter here.

jimmywarting: #311 is an old issue worth bringing up, @TimothyGu first recommended that we used data-uri-to-buffer but later suggested that we also would use data-urls but the pr got closed by @Richienb who used native node functions (#659).
tinovyatkin: yeah, my first intention was to use native Buffer.from, but data uri can be pretty complex...
jimmywarting: well, #659 should never have been accepted. but that is gone now

Patch:
diff --git a/.github/ISSUE_TEMPLATE/config.yml b/.github/ISSUE_TEMPLATE/config.yml
index 55f1844..17f583a 100644
--- a/.github/ISSUE_TEMPLATE/config.yml
+++ b/.github/ISSUE_TEMPLATE/config.yml
@@ -1,4 +1,4 @@
-blank_issues_enabled: false
+blank_issues_enabled: true
 contact_links:
   - name: Discord Server 
     url: https://discord.gg/Zxbndcm
diff --git a/package.json b/package.json
index 967664b..85f07dd 100644
--- a/package.json
+++ b/package.json
@@ -67,7 +67,7 @@
 		"xo": "^0.30.0"
 	},
 	"dependencies": {
-		"data-uri-to-buffer": "^3.0.0",
+		"data-uri-to-buffer": "^3.0.1",
 		"fetch-blob": "^1.0.6"
 	},
 	"tsd": {
diff --git a/src/index.js b/src/index.js
index 95f4283..77d7e5a 100644
--- a/src/index.js
+++ b/src/index.js
@@ -10,7 +10,7 @@ import http from 'http';
 import https from 'https';
 import zlib from 'zlib';
 import Stream, {PassThrough, pipeline as pump} from 'stream';
-import dataURIToBuffer from 'data-uri-to-buffer';
+import dataUriToBuffer from 'data-uri-to-buffer';
 
 import {writeToStream, getTotalBytes} from './body.js';
 import Response from './response.js';
@@ -22,36 +22,32 @@ import {isRedirect} from './utils/is-redirect.js';
 
 export {Headers, Request, Response, FetchError, AbortError, isRedirect};
 
+const supportedSchemas = new Set(['data:', 'http:', 'https:']);
+
 /**
  * Fetch function
  *
- * @param   Mixed    url   Absolute url or Request instance
- * @param   Object   opts  Fetch options
- * @return  Promise
+ * @param   {string | URL | import('./request').default} url - Absolute url or Request instance
+ * @param   {*} [options_] - Fetch options
+ * @return  {Promise<import('./response').default>}
  */
 export default async function fetch(url, options_) {
-	// Regex for data uri
-	const dataUriRegex = /^\s*data:([a-z]+\/[a-z]+(;[a-z-]+=[a-z-]+)?)?(;base64)?,[\w!$&',()*+;=\-.~:@/?%\s]*\s*$/i;
-
-	// If valid data uri
-	if (dataUriRegex.test(url)) {
-		const data = dataURIToBuffer(url);
-		const response = new Response(data, {headers: {'Content-Type': data.type}});
-		return response;
-	}
-
-	// If invalid data uri
-	if (url.toString().startsWith('data:')) {
-		const request = new Request(url, options_);
-		throw new FetchError(`[${request.method}] ${request.url} invalid URL`, 'system');
-	}
-
-	// Wrap http.request into fetch
 	return new Promise((resolve, reject) => {
 		// Build request object
 		const request = new Request(url, options_);
 		const options = getNodeRequestOptions(request);
+		if (!supportedSchemas.has(options.protocol)) {
+			throw new TypeError(`node-fetch cannot load ${url}. URL scheme "${options.protocol.replace(/:$/, '')}" is not supported.`);
+		}
 
+		if (options.protocol === 'data:') {
+			const data = dataUriToBuffer(request.url);
+			const response = new Response(data, {headers: {'Content-Type': data.typeFull}});
+			resolve(response);
+			return;
+		}
+
+		// Wrap http.request into fetch
 		const send = (options.protocol === 'https:' ? https : http).request;
 		const {signal} = request;
 		let response = null;
@@ -282,4 +278,3 @@ export default async function fetch(url, options_) {
 		writeToStream(request_, request);
 	});
 }
-
diff --git a/src/request.js b/src/request.js
index 67fb393..9819244 100644
--- a/src/request.js
+++ b/src/request.js
@@ -28,26 +28,6 @@ const isRequest = object => {
 	);
 };
 
-/**
- * Wrapper around `new URL` to handle relative URLs (https://github.com/nodejs/node/issues/12682)
- *
- * @param  {string} urlStr
- * @return {void}
- */
-const parseURL = urlString => {
-	/*
-		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(urlString)) {
-		return new URL(urlString);
-	}
-
-	throw new TypeError('Only absolute URLs are supported');
-};
-
 /**
  * Request class
  *
@@ -61,18 +41,9 @@ export default class Request extends Body {
 
 		// Normalize input and force URL to be encoded as UTF-8 (https://github.com/bitinn/node-fetch/issues/245)
 		if (isRequest(input)) {
-			parsedURL = parseURL(input.url);
+			parsedURL = new URL(input.url);
 		} else {
-			if (input && input.href) {
-				// 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 = parseURL(input.href);
-			} else {
-				// Coerce input to a string before attempting to parse
-				parsedURL = parseURL(`${input}`);
-			}
-
+			parsedURL = new URL(input);
 			input = {};
 		}
 
@@ -189,10 +160,6 @@ export const getNodeRequestOptions = request => {
 		headers.set('Accept', '*/*');
 	}
 
-	if (!/^https?:$/.test(parsedURL.protocol)) {
-		throw new TypeError('Only HTTP(S) protocols are supported');
-	}
-
 	// HTTP-network-or-cache fetch steps 2.4-2.7
 	let contentLengthValue = null;
 	if (request.body === null && /^(post|put)$/i.test(request.method)) {


Test patch:
diff --git a/test/external-encoding.js b/test/external-encoding.js
index 3bfd1d8..caf0ffd 100644
--- a/test/external-encoding.js
+++ b/test/external-encoding.js
@@ -19,7 +19,7 @@ describe('external encoding', () => {
 		it('should accept data uri of plain text', () => {
 			return fetch('data:,Hello%20World!').then(r => {
 				expect(r.status).to.equal(200);
-				expect(r.headers.get('Content-Type')).to.equal('text/plain');
+				expect(r.headers.get('Content-Type')).to.equal('text/plain;charset=US-ASCII');
 				return r.text().then(t => expect(t).to.equal('Hello World!'));
 			});
 		});
@@ -27,7 +27,7 @@ describe('external encoding', () => {
 		it('should reject invalid data uri', () => {
 			return fetch('data:@@@@').catch(error => {
 				expect(error).to.exist;
-				expect(error.message).to.include('invalid URL');
+				expect(error.message).to.include('malformed data: URI');
 			});
 		});
 	});
diff --git a/test/main.js b/test/main.js
index b1500b8..a8f31b1 100644
--- a/test/main.js
+++ b/test/main.js
@@ -95,17 +95,17 @@ describe('node-fetch', () => {
 
 	it('should reject with error if url is protocol relative', () => {
 		const url = '//example.com/';
-		return expect(fetch(url)).to.eventually.be.rejectedWith(TypeError, 'Only absolute URLs are supported');
+		return expect(fetch(url)).to.eventually.be.rejectedWith(TypeError, /Invalid URL/);
 	});
 
 	it('should reject with error if url is relative path', () => {
 		const url = '/some/path';
-		return expect(fetch(url)).to.eventually.be.rejectedWith(TypeError, 'Only absolute URLs are supported');
+		return expect(fetch(url)).to.eventually.be.rejectedWith(TypeError, /Invalid URL/);
 	});
 
 	it('should reject with error if protocol is unsupported', () => {
 		const url = 'ftp://example.com/';
-		return expect(fetch(url)).to.eventually.be.rejectedWith(TypeError, 'Only HTTP(S) protocols are supported');
+		return expect(fetch(url)).to.eventually.be.rejectedWith(TypeError, /URL scheme "ftp" is not supported/);
 	});
 
 	itIf(process.platform !== 'win32')('should reject with error on network failure', () => {
@@ -2132,6 +2132,15 @@ describe('node-fetch', () => {
 			});
 		});
 
+		it('should accept data uri 2', async () => {
+			const r = await fetch('data:text/plain;charset=UTF-8;page=21,the%20data:1234,5678');
+			expect(r.status).to.equal(200);
+			expect(r.headers.get('Content-Type')).to.equal('text/plain;charset=UTF-8;page=21');
+
+			const b = await r.text();
+			expect(b).to.equal('the data:1234,5678');
+		});
+
 		it('should reject invalid data uri', () => {
 			return fetch(invalidDataUrl).catch(error => {
 				console.assert(error);