Repository: node-fetch/node-fetch
Pull request #843 (2020-05-28 02:59:42)

Polyfill `http.validateHeaderName` and `http.validateHeaderValue`
This PR makes use of native [header/value validation functions](https://nodejs.org/api/http.html#http_http_validateheadername_name) added in Node 14.3 and adjusts error message/code for our functions polyfilling the same behavior.

Richienb: I'm putting this on hold until at least April 2022 at which point Node.js 14 would be the last LTS version.

/remind me to re-review on 1 April 2022


tinovyatkin: Did you check the code? It uses feature detection and native functions when available and rolls-back to our implementation if they don’t. No experimental warnings, etc.

This approach was used several times in this library in the past (remember gated Stream.destroy for example) and I can’t understand why you decided to put it on hold in this case?
Richienb: Is there a difference between `http.validateHeaderName` and our implementation?
tinovyatkin: RegExes are different: https://github.com/nodejs/node/blob/26f150022f9b4d4709c1d4ad450c6f9e9fce8d0e/lib/_http_common.js#L204
Ours are negating. 
It's a big chance, like with may other features in the past, that this will be backported to Node 12.
Richienb: Since adding this support doesn't really change anything, either we copy over the regex or wait for the v12 backport and which we'll most likely support in node-fetch v4.
LinusU: Personally, I feel that it's better to either wait until we can drop support for Node.js versions that doesn't have this, or copy their regex here and only use our copy. That way there won't be subtle differences between different versions, since potentially we aren't behaving in the exact same way...
tinovyatkin: Colleagues, I'm really lost here...
This PR is basically polyfilling functions added into Node 14 as they directly related to our functionality (adjusting the shape of our error messages and adding wrong header name into value error)
It's the practice that moved forward the whole JavaScript development community. It's something this project did before with `Readable.destroy`, Brotli and a bunch of other features.
The `fetch` itself is still a polyfill in many cases browser-side.

But now @Richienb put it on hold until Node 14 becomes LTS, and @LinusU says that just copying source code is better than attempt to use native feature first.

I'm better will close this PR and do nothing until I'll be able to understand this dramatic policy shift.
LinusU: > [...] dramatic policy shift.

So, I'm quite new to this project so I'm not entirely up to speed on what has been the policy currently. Maybe I could have made that clearer, I was just speaking of my personal opinion which is coming from maintaining other packages...

With that said, I will try to explain myself more throughly, but I will stress that this just my _personal opinion_ on what technically best solution is:

Since our function and the built-in Node.js function doesn't use the same regex, it's likely that there will be subtle differences in the behaviour of those two functions. This means that the behaviour of our library will change, when running on Node.js 14.3 or newer.

Since we have to maintain a code path that works before Node.js 14.3 anyways, I think it's better that we only have one code path, since I don't see any downside with this.

The idea of not using the built-in version even when available is sometimes called a [ponyfill](https://ponyfill.com/), and that link has some more information on why it's preferred.

As I said, this is just my personal opinion...
jimmywarting: @tinovyatkin tone down a bit!
tinovyatkin: @LinusU we all speak personal opinions here and nobody is the ultimate source of true :-)
If there is no written policy then I assume previously accepted contributions are the policy and we did the use of gated features here in the past (see above).

We certainly can copy the source from code, but:
1. Our code works and somehow battle-tested, but the Node functions are just released and subject to internal changes
2. However, where they are available our headers will be verified by them anyway as soon as they get to native`http.request`. So, it will be a share to have a header that passed our check but will be thrown by native Node check
3. If we decide to just copy without attempting to use native function first then we can easily forget to switch to the native functions on a future refactoring when these functions will be available on our minimum required platform
4. If we just copy, we also should track changes to these functions in Node to stay update with bug fixes. We should not discard the case when Node may decide to move these functions into C++.

So, I see current approach as compliance with JavaScript industry best practice and future-proof.
Richienb: > 3\. we can easily forget to switch to the native functions on a future refactoring

Just add a `TODO` comment for reminding.
jimmywarting: hmm, on the fence, 14.3 just got out so many won't benefit from it just yet. I like native things too. I would probably say use native first if possible, if not copy over nodes regex to our code also instead of using two different. but i also would want to see at least some differences between the two, which is faster/slower, is one or the other doing things wrong, dose something pass throught the cracks that should or shouldn't be a valid header, who wrote the current regex that we use today, where dose it come from?
tinovyatkin: @jimmywarting 
Our header validation was initially borrowed from Node code (it was non-Regex version): https://github.com/node-fetch/node-fetch/commit/2cafdcb5e449f02980ea54b74342ab41d77a1f76

Then copying RegExp version from Node core again: https://github.com/node-fetch/node-fetch/commit/a8f6d79c39dabd2fa8a4938b2eaae5ae8d3e6f1d

Both changes were made by @TimothyGu 

But in some moment we stop tracking, so, without putting native code call we probably will forget again 😄 

Our current code at `master`:
```js
const invalidTokenRegex = /[^`\-\w!#$%&'*+.|~]/;
const invalidHeaderCharRegex = /[^\t\u0020-\u007E\u0080-\u00FF]/;

function validateName(name) {
	name = String(name);
	if (invalidTokenRegex.test(name) || name === '') {
		throw new TypeError(`'${name}' is not a legal HTTP header name`);
	}
}

function validateValue(value) {
	value = String(value);
	if (invalidHeaderCharRegex.test(value)) {
		throw new TypeError(`'${value}' is not a legal HTTP header value`);
	}
}
```

Node [current code](https://github.com/nodejs/node/blob/26f150022f9b4d4709c1d4ad450c6f9e9fce8d0e/lib/_http_common.js#L204) is bigger as it public API now, but RegExp part is as below:
```js
const tokenRegExp = /^[\^_`a-zA-Z\-0-9!#$%&'*+.|~]+$/;
/**
 * Verifies that the given val is a valid HTTP token
 * per the rules defined in RFC 7230
 * See https://tools.ietf.org/html/rfc7230#section-3.2.6
 */
function checkIsHttpToken(val) {
  return tokenRegExp.test(val);
}

const headerCharRegex = /[^\t\x20-\x7e\x80-\xff]/;
/**
 * True if val contains an invalid field-vchar
 *  field-value    = *( field-content / obs-fold )
 *  field-content  = field-vchar [ 1*( SP / HTAB ) field-vchar ]
 *  field-vchar    = VCHAR / obs-text
 */
function checkInvalidHeaderChar(val) {
  return headerCharRegex.test(val);
}
```
tinovyatkin: @jimmywarting Node.JS [announces High severity security release](https://nodejs.org/en/blog/vulnerability/june-2020-security-releases/) upcoming this Sat, so, probably everybody on Node 14.x will upgrade and have this feature.
jimmywarting: can you switch to using node's regex as a fallback if `http.validateHeaderName` and `http.validateHeaderValue` isn't available?

tinovyatkin: @jimmywarting done. We need #846 to be merged to actually test on Node 14.3, as for now all tests are running on LTS.

Patch:
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index c1192f5..b9ddc41 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -2,12 +2,12 @@ name: CI
 
 on:
   push:
-    branches: [ master ]
+    branches: [master]
   pull_request:
     paths:
-      - '**.js'
-      - 'package.json'
-      - '.github/workflows/ci.yml'
+      - "**.js"
+      - "package.json"
+      - ".github/workflows/ci.yml"
 
 jobs:
   test:
@@ -41,7 +41,6 @@ jobs:
       # upload coverage only once
       - name: Coveralls
         uses: coverallsapp/github-action@master
-        if: matrix.node == '14.x' && matrix.os == 'ubuntu-latest'
+        if: matrix.node == '12.x' && matrix.os == 'ubuntu-latest'
         with:
           github-token: ${{ secrets.GITHUB_TOKEN }}
-
diff --git a/README.md b/README.md
index 9026b75..0460306 100644
--- a/README.md
+++ b/README.md
@@ -277,10 +277,10 @@ const streamPipeline = util.promisify(require('stream').pipeline);
 	const response = await fetch('https://assets-cdn.github.com/images/modules/logos_page/Octocat.png');
 	
 	if (response.ok) {
-		return streamPipeline(res.body, fs.createWriteStream('./octocat.png'));
+		return streamPipeline(response.body, fs.createWriteStream('./octocat.png'));
 	}
 
-	throw new Error(`unexpected response ${res.statusText}`);
+	throw new Error(`unexpected response ${response.statusText}`);
 })();
 ```
 
@@ -309,11 +309,11 @@ const fetch = require('node-fetch');
 (async () => {
 	const response = await fetch('https://github.com/');
 	
-	console.log(res.ok);
-	console.log(res.status);
-	console.log(res.statusText);
-	console.log(res.headers.raw());
-	console.log(res.headers.get('content-type'));
+	console.log(response.ok);
+	console.log(response.status);
+	console.log(response.statusText);
+	console.log(response.headers.raw());
+	console.log(response.headers.get('content-type'));
 })();
 ```
 
@@ -328,7 +328,7 @@ const fetch = require('node-fetch');
 	const response = await fetch('https://example.com');
 	
 	// Returns an array of values, instead of a string of comma-separated values
-	console.log(res.headers.raw()['set-cookie']);
+	console.log(response.headers.raw()['set-cookie']);
 })();
 ```
 
@@ -446,7 +446,8 @@ The default values are shown after each option key.
     compress: true,         // support gzip/deflate content encoding. false to disable
     size: 0,                // maximum response body size in bytes. 0 to disable
     agent: null,            // http(s).Agent instance or function that returns an instance (see below)
-    highWaterMark: 16384    // the maximum number of bytes to store in the internal buffer before ceasing to read from the underlying resource.
+    highWaterMark: 16384,   // the maximum number of bytes to store in the internal buffer before ceasing to read from the underlying resource.
+    insecureHTTPParser: false	// Use an insecure HTTP parser that accepts invalid HTTP headers when `true`.
 }
 ```
 
@@ -537,6 +538,11 @@ const fetch = require('node-fetch');
 })();
 ```
 
+#### Insecure HTTP Parser
+
+Passed through to the `insecureHTTPParser` option on http(s).request. See [`http.request`](https://nodejs.org/api/http.html#http_http_request_url_options_callback) for more information.
+
+
 <a id="class-request"></a>
 
 ### Class: Request
@@ -628,7 +634,7 @@ Construct a new `Headers` object. `init` can be either `null`, a `Headers` objec
 
 ```js
 // Example adapted from https://fetch.spec.whatwg.org/#example-headers-class
-const Headers = require('node-fetch');
+const { Headers } = require('node-fetch');
 
 const meta = {
 	'Content-Type': 'text/xml',
diff --git a/docs/v3-UPGRADE-GUIDE.md b/docs/v3-UPGRADE-GUIDE.md
index 0cc541a..ad9848f 100644
--- a/docs/v3-UPGRADE-GUIDE.md
+++ b/docs/v3-UPGRADE-GUIDE.md
@@ -126,6 +126,6 @@ Since v3.x you no longer need to install `@types/node-fetch` package in order to
 [cross-fetch]: https://github.com/lquixada/cross-fetch
 [fetch-charset-detection]: https://github.com/Richienb/fetch-charset-detection
 [fetch-charset-detection-docs]: https://richienb.github.io/fetch-charset-detection/globals.html#convertbody
-[fetch-blob]: https://github.com/bitinn/fetch-blob#readme
+[fetch-blob]: https://github.com/node-fetch/fetch-blob#readme
 [whatwg-nodejs-url]: https://nodejs.org/api/url.html#url_the_whatwg_url_api
 [changelog]: CHANGELOG.md
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/headers.js b/src/headers.js
index 20424a8..0a3e250 100644
--- a/src/headers.js
+++ b/src/headers.js
@@ -5,23 +5,27 @@
  */
 
 import {types} from 'util';
+import http from 'http';
 
-const invalidTokenRegex = /[^`\-\w!#$%&'*+.|~]/;
-const invalidHeaderCharRegex = /[^\t\u0020-\u007E\u0080-\u00FF]/;
-
-function validateName(name) {
-	name = String(name);
-	if (invalidTokenRegex.test(name) || name === '') {
-		throw new TypeError(`'${name}' is not a legal HTTP header name`);
-	}
-}
+const validateHeaderName = typeof http.validateHeaderName === 'function' ?
+	http.validateHeaderName :
+	name => {
+		if (!/^[\^`\-\w!#$%&'*+.|~]+$/.test(name)) {
+			const err = new TypeError(`Header name must be a valid HTTP token [${name}]`);
+			Object.defineProperty(err, 'code', {value: 'ERR_INVALID_HTTP_TOKEN'});
+			throw err;
+		}
+	};
 
-function validateValue(value) {
-	value = String(value);
-	if (invalidHeaderCharRegex.test(value)) {
-		throw new TypeError(`'${value}' is not a legal HTTP header value`);
-	}
-}
+const validateHeaderValue = typeof http.validateHeaderValue === 'function' ?
+	http.validateHeaderValue :
+	(name, value) => {
+		if (/[^\t\u0020-\u007E\u0080-\u00FF]/.test(value)) {
+			const err = new TypeError(`Invalid character in header content ["${name}"]`);
+			Object.defineProperty(err, 'code', {value: 'ERR_INVALID_CHAR'});
+			throw err;
+		}
+	};
 
 /**
  * @typedef {Headers | Record<string, string> | Iterable<readonly [string, string]> | Iterable<Iterable<string>>} HeadersInit
@@ -91,9 +95,9 @@ export default class Headers extends URLSearchParams {
 		result =
 			result.length > 0 ?
 				result.map(([name, value]) => {
-					validateName(name);
-					validateValue(value);
-					return [String(name).toLowerCase(), value];
+					validateHeaderName(name);
+					validateHeaderValue(name, String(value));
+					return [String(name).toLowerCase(), String(value)];
 				}) :
 				undefined;
 
@@ -107,12 +111,12 @@ export default class Headers extends URLSearchParams {
 					case 'append':
 					case 'set':
 						return (name, value) => {
-							validateName(name);
-							validateValue(value);
+							validateHeaderName(name);
+							validateHeaderValue(name, String(value));
 							return URLSearchParams.prototype[p].call(
 								receiver,
 								String(name).toLowerCase(),
-								value
+								String(value)
 							);
 						};
 
@@ -120,7 +124,7 @@ export default class Headers extends URLSearchParams {
 					case 'has':
 					case 'getAll':
 						return name => {
-							validateName(name);
+							validateHeaderName(name);
 							return URLSearchParams.prototype[p].call(
 								receiver,
 								String(name).toLowerCase()
@@ -142,7 +146,7 @@ export default class Headers extends URLSearchParams {
 	}
 
 	get [Symbol.toStringTag]() {
-		return 'Headers';
+		return this.constructor.name;
 	}
 
 	toString() {
@@ -247,7 +251,15 @@ export function fromRawHeaders(headers = []) {
 
 				return result;
 			}, [])
-			.filter(([name, value]) => !(invalidTokenRegex.test(name) || invalidHeaderCharRegex.test(value)))
+			.filter(([name, value]) => {
+				try {
+					validateHeaderName(name);
+					validateHeaderValue(name, String(value));
+					return true;
+				} catch {
+					return false;
+				}
+			})
 
 	);
 }
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..bb0df34 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
  *
@@ -59,20 +39,11 @@ export default class Request extends Body {
 	constructor(input, init = {}) {
 		let parsedURL;
 
-		// Normalize input and force URL to be encoded as UTF-8 (https://github.com/bitinn/node-fetch/issues/245)
+		// Normalize input and force URL to be encoded as UTF-8 (https://github.com/node-fetch/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 = {};
 		}
 
@@ -129,6 +100,7 @@ export default class Request extends Body {
 		this.counter = init.counter || input.counter || 0;
 		this.agent = init.agent || input.agent;
 		this.highWaterMark = init.highWaterMark || input.highWaterMark || 16384;
+		this.insecureHTTPParser = init.insecureHTTPParser || input.insecureHTTPParser || false;
 	}
 
 	get method() {
@@ -189,10 +161,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)) {
@@ -247,6 +215,7 @@ export const getNodeRequestOptions = request => {
 		href: parsedURL.href,
 		method: request.method,
 		headers: headers[Symbol.for('nodejs.util.inspect.custom')](),
+		insecureHTTPParser: request.insecureHTTPParser,
 		agent
 	};
 


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);