Repository: node-fetch/node-fetch
Pull request #1599 (2022-07-07 18:09:37)

fix(Headers): don't forward secure headers on protocol change
<!-- Thanks for contributing! -->

## Purpose
Resolves https://www.huntr.dev/bounties/db31e05b-ff10-4057-81a3-37445bf161cd/ by validating that the URL protocol remains the same when determining whether to send secure headers on a redirect.
This prevents MITM attacks from sniffing secure headers when a redirect downgrades a `https://` to a `http://`

## Changes
Adds an additional check to the redirect `follow` step to determine whether to send secure headers or not.

## Additional information


___

<!-- Mark the ones you have done and remove unnecessary ones. Add new tasks that fit (like TODOs). -->
- [ ] I updated readme
- [x] I added unit test(s)

___

<!-- Add `- fix #_NUMBER_` line for every PR/Issue this PR solves. Do not comma separate them. -->




Patch:
diff --git a/@types/index.d.ts b/@types/index.d.ts
index 2a482f6..2374801 100644
--- a/@types/index.d.ts
+++ b/@types/index.d.ts
@@ -1,5 +1,4 @@
 /// <reference types="node" />
-/// <reference lib="dom" />
 
 import {RequestOptions} from 'http';
 import {FormData} from 'formdata-polyfill/esm.min.js';
diff --git a/README.md b/README.md
index fce3d8f..dc6661e 100644
--- a/README.md
+++ b/README.md
@@ -442,7 +442,7 @@ console.log(data)
 If you for some reason need to post a stream coming from any arbitrary place,
 then you can append a [Blob] or a [File] look-a-like item.
 
-The minium requirement is that it has:
+The minimum requirement is that it has:
 1. A `Symbol.toStringTag` getter or property that is either `Blob` or `File`
 2. A known size.
 3. And either a `stream()` method or a `arrayBuffer()` method that returns a ArrayBuffer.
diff --git a/src/index.js b/src/index.js
index 82efc98..7c4aee8 100644
--- a/src/index.js
+++ b/src/index.js
@@ -22,7 +22,7 @@ import {FetchError} from './errors/fetch-error.js';
 import {AbortError} from './errors/abort-error.js';
 import {isRedirect} from './utils/is-redirect.js';
 import {FormData} from 'formdata-polyfill/esm.min.js';
-import {isDomainOrSubdomain} from './utils/is.js';
+import {isDomainOrSubdomain, isSameProtocol} from './utils/is.js';
 import {parseReferrerPolicyFromHeader} from './utils/referrer.js';
 import {
 	Blob,
@@ -203,7 +203,10 @@ export default async function fetch(url, options_) {
 						// that is not a subdomain match or exact match of the initial domain.
 						// For example, a redirect from "foo.com" to either "foo.com" or "sub.foo.com"
 						// will forward the sensitive headers, but a redirect to "bar.com" will not.
-						if (!isDomainOrSubdomain(request.url, locationURL)) {
+						// headers will also be ignored when following a redirect to a domain using
+						// a different protocol. For example, a redirect from "https://foo.com" to "http://foo.com"
+						// will not forward the sensitive headers
+						if (!isDomainOrSubdomain(request.url, locationURL) || !isSameProtocol(request.url, locationURL)) {
 							for (const name of ['authorization', 'www-authenticate', 'cookie', 'cookie2']) {
 								requestOptions.headers.delete(name);
 							}
diff --git a/src/request.js b/src/request.js
index 38f33df..7a1a954 100644
--- a/src/request.js
+++ b/src/request.js
@@ -65,7 +65,7 @@ export default class Request extends Body {
 			method = method.toUpperCase();
 		}
 
-		if ('data' in init) {
+		if (!isRequest(init) && 'data' in init) {
 			doBadDataWarn();
 		}
 
diff --git a/src/utils/is.js b/src/utils/is.js
index 00eb89b..f9e467e 100644
--- a/src/utils/is.js
+++ b/src/utils/is.js
@@ -71,3 +71,17 @@ export const isDomainOrSubdomain = (destination, original) => {
 
 	return orig === dest || orig.endsWith(`.${dest}`);
 };
+
+/**
+ * isSameProtocol reports whether the two provided URLs use the same protocol.
+ *
+ * Both domains must already be in canonical form.
+ * @param {string|URL} original
+ * @param {string|URL} destination
+ */
+export const isSameProtocol = (destination, original) => {
+	const orig = new URL(original).protocol;
+	const dest = new URL(destination).protocol;
+
+	return orig === dest;
+};


Test patch:
diff --git a/test/main.js b/test/main.js
index 5df12c4..ec58f82 100644
--- a/test/main.js
+++ b/test/main.js
@@ -33,7 +33,7 @@ import ResponseOrig from '../src/response.js';
 import Body, {getTotalBytes, extractContentType} from '../src/body.js';
 import TestServer from './utils/server.js';
 import chaiTimeout from './utils/chai-timeout.js';
-import {isDomainOrSubdomain} from '../src/utils/is.js';
+import {isDomainOrSubdomain, isSameProtocol} from '../src/utils/is.js';
 
 const AbortControllerPolyfill = abortControllerPolyfill.AbortController;
 const encoder = new TextEncoder();
@@ -522,7 +522,7 @@ describe('node-fetch', () => {
 		expect(res.url).to.equal(`${base}inspect`);
 		expect(headers.get('other-safe-headers')).to.equal('stays');
 		expect(headers.get('x-foo')).to.equal('bar');
-		// Unsafe headers should not have been sent to httpbin
+		// Unsafe headers are not removed
 		expect(headers.get('cookie')).to.equal('is=cookie');
 		expect(headers.get('cookie2')).to.equal('is=cookie2');
 		expect(headers.get('www-authenticate')).to.equal('is=www-authenticate');
@@ -542,6 +542,39 @@ describe('node-fetch', () => {
 		expect(isDomainOrSubdomain('http://bob.uk.com', 'http://xyz.uk.com')).to.be.false;
 	});
 
+	it('should not forward secure headers to changed protocol', async () => {
+		const res = await fetch('https://httpbin.org/redirect-to?url=http%3A%2F%2Fhttpbin.org%2Fget&status_code=302', {
+			headers: new Headers({
+				cookie: 'gets=removed',
+				cookie2: 'gets=removed',
+				authorization: 'gets=removed',
+				'www-authenticate': 'gets=removed',
+				'other-safe-headers': 'stays',
+				'x-foo': 'bar'
+			})
+		});
+
+		const headers = new Headers((await res.json()).headers);
+		// Safe headers are not removed
+		expect(headers.get('other-safe-headers')).to.equal('stays');
+		expect(headers.get('x-foo')).to.equal('bar');
+		// Unsafe headers should not have been sent to downgraded http
+		expect(headers.get('cookie')).to.equal(null);
+		expect(headers.get('cookie2')).to.equal(null);
+		expect(headers.get('www-authenticate')).to.equal(null);
+		expect(headers.get('authorization')).to.equal(null);
+	});
+
+	it('isSameProtocol', () => {
+		// Forwarding headers to same protocol is OK
+		expect(isSameProtocol('http://a.com', 'http://a.com')).to.be.true;
+		expect(isSameProtocol('https://a.com', 'https://www.a.com')).to.be.true;
+
+		// Forwarding headers to diff protocol is not OK
+		expect(isSameProtocol('http://b.com', 'https://b.com')).to.be.false;
+		expect(isSameProtocol('http://www.a.com', 'https://a.com')).to.be.false;
+	});
+
 	it('should treat broken redirect as ordinary response (follow)', async () => {
 		const url = `${base}redirect/no-location`;
 		const res = await fetch(url);
@@ -1003,9 +1036,7 @@ describe('node-fetch', () => {
 					}
 				});
 
-				setTimeout(() => {
-					controller.abort();
-				}, 100);
+				controller.abort();
 
 				return expect(promise)
 					.to.eventually.be.rejected
@@ -1029,9 +1060,7 @@ describe('node-fetch', () => {
 					})
 				];
 
-				setTimeout(() => {
-					controller.abort();
-				}, 100);
+				controller.abort();
 
 				return Promise.all(fetches.map(fetched => expect(fetched)
 					.to.eventually.be.rejected