Repository: node-fetch/node-fetch
Pull request #1369 (2021-11-09 15:40:18)

fix(Request): Fix logical operator priority to disallow GET/HEAD with non-empty body
## Purpose

There was a regression introduced in a conditional which checks for a `body` override in `Request`, where brackets were at the wrong place. Because of this, in certain cases, one could create a GET/HEAD `Request` instance with a non-empty body, which should normally result in an `TypeError` thrown.

## Changes

This PR fixes the issue, and adds a test for this particular case.

## Additional information


___

- [x] I added unit test(s)

___



Patch:
diff --git a/README.md b/README.md
index 297a373..ad7d121 100644
--- a/README.md
+++ b/README.md
@@ -41,7 +41,7 @@
 	- [Streams](#streams)
 	- [Accessing Headers and other Metadata](#accessing-headers-and-other-metadata)
 	- [Extract Set-Cookie Header](#extract-set-cookie-header)
-	- [Post data using a file stream](#post-data-using-a-file-stream)
+	- [Post data using a file](#post-data-using-a-file)
 	- [Request cancellation with AbortSignal](#request-cancellation-with-abortsignal)
 - [API](#api)
 	- [fetch(url[, options])](#fetchurl-options)
@@ -403,7 +403,7 @@ node-fetch also supports any spec-compliant FormData implementations such as [fo
 
 ```js
 import fetch from 'node-fetch';
-import {FormData} from 'formdata-polyfill/esm-min.js';
+import {FormData} from 'formdata-polyfill/esm.min.js';
 
 // Alternative hack to get the same FormData instance as node-fetch
 // const FormData = (await new Response(new URLSearchParams()).formData()).constructor
@@ -750,7 +750,7 @@ An Error thrown when the request is aborted in response to an `AbortSignal`'s `a
 For older versions please use the type definitions from [DefinitelyTyped](https://github.com/DefinitelyTyped/DefinitelyTyped):
 
 ```sh
-npm install --save-dev @types/node-fetch
+npm install --save-dev @types/node-fetch@2.x
 ```
 
 ## Acknowledgement
diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md
index b3c9876..5245cfe 100644
--- a/docs/CHANGELOG.md
+++ b/docs/CHANGELOG.md
@@ -4,6 +4,10 @@ All notable changes will be recorded here.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## Unreleased
+* fix(request): fix crash when an invalid redirection URL is encountered https://github.com/node-fetch/node-fetch/pull/1387
+* fix: handle errors from the request body stream by @mdmitry01 in https://github.com/node-fetch/node-fetch/pull/1392
+
 ## 3.1.0
 
 ## What's Changed
diff --git a/package.json b/package.json
index f79978e..982fe65 100644
--- a/package.json
+++ b/package.json
@@ -64,7 +64,7 @@
   "dependencies": {
     "data-uri-to-buffer": "^4.0.0",
     "formdata-polyfill": "^4.0.10",
-    "fetch-blob": "^3.1.2"
+    "fetch-blob": "^3.1.3"
   },
   "tsd": {
     "cwd": "@types",
diff --git a/src/body.js b/src/body.js
index 85a8ea5..64b880d 100644
--- a/src/body.js
+++ b/src/body.js
@@ -6,7 +6,7 @@
  */
 
 import Stream, {PassThrough} from 'node:stream';
-import {types, deprecate} from 'node:util';
+import {types, deprecate, promisify} from 'node:util';
 
 import Blob from 'fetch-blob';
 import {FormData, formDataToBlob} from 'formdata-polyfill/esm.min.js';
@@ -15,6 +15,7 @@ import {FetchError} from './errors/fetch-error.js';
 import {FetchBaseError} from './errors/base.js';
 import {isBlob, isURLSearchParameters} from './utils/is.js';
 
+const pipeline = promisify(Stream.pipeline);
 const INTERNALS = Symbol('Body internals');
 
 /**
@@ -130,7 +131,7 @@ export default class Body {
 	 */
 	async blob() {
 		const ct = (this.headers && this.headers.get('content-type')) || (this[INTERNALS].body && this[INTERNALS].body.type) || '';
-		const buf = await this.buffer();
+		const buf = await this.arrayBuffer();
 
 		return new Blob([buf], {
 			type: ct
@@ -379,14 +380,14 @@ export const getTotalBytes = request => {
  *
  * @param {Stream.Writable} dest The stream to write to.
  * @param obj.body Body object from the Body instance.
- * @returns {void}
+ * @returns {Promise<void>}
  */
-export const writeToStream = (dest, {body}) => {
+export const writeToStream = async (dest, {body}) => {
 	if (body === null) {
 		// Body is null
 		dest.end();
 	} else {
 		// Body is stream
-		body.pipe(dest);
+		await pipeline(body, dest);
 	}
 };
diff --git a/src/index.js b/src/index.js
index f8686be..dc4bafd 100644
--- a/src/index.js
+++ b/src/index.js
@@ -78,7 +78,7 @@ export default async function fetch(url, options_) {
 		};
 
 		// Send request
-		const request_ = send(parsedURL, options);
+		const request_ = send(parsedURL.toString(), options);
 
 		if (signal) {
 			signal.addEventListener('abort', abortAndFinalize);
@@ -130,7 +130,19 @@ export default async function fetch(url, options_) {
 				const location = headers.get('Location');
 
 				// HTTP fetch step 5.3
-				const locationURL = location === null ? null : new URL(location, request.url);
+				let locationURL = null;
+				try {
+					locationURL = location === null ? null : new URL(location, request.url);
+				} catch {
+					// error here can only be invalid URL in Location: header
+					// do not throw when options.redirect == manual
+					// let the user extract the errorneous redirect URL
+					if (request.redirect !== 'manual') {
+						reject(new FetchError(`uri requested responds with an invalid redirect URL: ${location}`, 'invalid-redirect'));
+						finalize();
+						return;
+					}
+				}
 
 				// HTTP fetch step 5.5
 				switch (request.redirect) {
@@ -291,7 +303,8 @@ export default async function fetch(url, options_) {
 			resolve(response);
 		});
 
-		writeToStream(request_, request);
+		// eslint-disable-next-line promise/prefer-await-to-then
+		writeToStream(request_, request).catch(reject);
 	});
 }
 
diff --git a/src/request.js b/src/request.js
index 6d6272c..d13873b 100644
--- a/src/request.js
+++ b/src/request.js
@@ -1,4 +1,3 @@
-
 /**
  * Request.js
  *
@@ -21,7 +20,7 @@ const INTERNALS = Symbol('Request internals');
 /**
  * Check if `obj` is an instance of Request.
  *
- * @param  {*} obj
+ * @param  {*} object
  * @return {boolean}
  */
 const isRequest = object => {
@@ -60,7 +59,7 @@ export default class Request extends Body {
 		method = method.toUpperCase();
 
 		// eslint-disable-next-line no-eq-null, eqeqeq
-		if (((init.body != null || isRequest(input)) && input.body !== null) &&
+		if ((init.body != null || (isRequest(input) && input.body !== null)) &&
 			(method === 'GET' || method === 'HEAD')) {
 			throw new TypeError('Request with GET/HEAD method cannot have body');
 		}
@@ -133,14 +132,17 @@ export default class Request extends Body {
 		this.referrerPolicy = init.referrerPolicy || input.referrerPolicy || '';
 	}
 
+	/** @returns {string} */
 	get method() {
 		return this[INTERNALS].method;
 	}
 
+	/** @returns {string} */
 	get url() {
 		return formatUrl(this[INTERNALS].parsedURL);
 	}
 
+	/** @returns {Headers} */
 	get headers() {
 		return this[INTERNALS].headers;
 	}
@@ -149,6 +151,7 @@ export default class Request extends Body {
 		return this[INTERNALS].redirect;
 	}
 
+	/** @returns {AbortSignal} */
 	get signal() {
 		return this[INTERNALS].signal;
 	}
@@ -206,8 +209,8 @@ Object.defineProperties(Request.prototype, {
 /**
  * Convert a Request to Node.js http request options.
  *
- * @param   Request  A Request instance
- * @return  Object   The options object to be passed to http.request
+ * @param {Request} request - A Request instance
+ * @return The options object to be passed to http.request
  */
 export const getNodeRequestOptions = request => {
 	const {parsedURL} = request[INTERNALS];
@@ -296,6 +299,7 @@ export const getNodeRequestOptions = request => {
 	};
 
 	return {
+		/** @type {URL} */
 		parsedURL,
 		options
 	};


Test patch:
diff --git a/test/main.js b/test/main.js
index dc4198d..5932f75 100644
--- a/test/main.js
+++ b/test/main.js
@@ -527,6 +527,28 @@ describe('node-fetch', () => {
 		});
 	});
 
+	it('should process an invalid redirect (manual)', () => {
+		const url = `${base}redirect/301/invalid`;
+		const options = {
+			redirect: 'manual'
+		};
+		return fetch(url, options).then(res => {
+			expect(res.url).to.equal(url);
+			expect(res.status).to.equal(301);
+			expect(res.headers.get('location')).to.equal('//super:invalid:url%/');
+		});
+	});
+
+	it('should throw an error on invalid redirect url', () => {
+		const url = `${base}redirect/301/invalid`;
+		return fetch(url).then(() => {
+			expect.fail();
+		}, error => {
+			expect(error).to.be.an.instanceof(FetchError);
+			expect(error.message).to.equal('uri requested responds with an invalid redirect URL: //super:invalid:url%/');
+		});
+	});
+
 	it('should throw a TypeError on an invalid redirect option', () => {
 		const url = `${base}redirect/301`;
 		const options = {
@@ -1456,6 +1478,21 @@ describe('node-fetch', () => {
 		});
 	});
 
+	it('should reject if the request body stream emits an error', () => {
+		const url = `${base}inspect`;
+		const requestBody = new stream.PassThrough();
+		const options = {
+			method: 'POST',
+			body: requestBody
+		};
+		const errorMessage = 'request body stream error';
+		setImmediate(() => {
+			requestBody.emit('error', new Error(errorMessage));
+		});
+		return expect(fetch(url, options))
+			.to.be.rejectedWith(Error, errorMessage);
+	});
+
 	it('should allow POST request with form-data as body', () => {
 		const form = new FormData();
 		form.append('a', '1');
diff --git a/test/request.js b/test/request.js
index de4fed1..527fab9 100644
--- a/test/request.js
+++ b/test/request.js
@@ -123,6 +123,8 @@ describe('Request', () => {
 			.to.throw(TypeError);
 		expect(() => new Request(base, {body: 'a', method: 'head'}))
 			.to.throw(TypeError);
+		expect(() => new Request(new Request(base), {body: 'a'}))
+			.to.throw(TypeError);
 	});
 
 	it('should throw error when including credentials', () => {
diff --git a/test/utils/server.js b/test/utils/server.js
index 2a1e8e9..351a3cd 100644
--- a/test/utils/server.js
+++ b/test/utils/server.js
@@ -239,6 +239,12 @@ export default class TestServer {
 			res.end();
 		}
 
+		if (p === '/redirect/301/invalid') {
+			res.statusCode = 301;
+			res.setHeader('Location', '//super:invalid:url%/');
+			res.end();
+		}
+
 		if (p === '/redirect/302') {
 			res.statusCode = 302;
 			res.setHeader('Location', '/inspect');