Exposure of Sensitive Information to an Unauthorized Actor in node-fetch/node-fetch

@@ -4,9 +4,30 @@ 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
+## What's Changed
+* core: update fetch-blob by @jimmywarting in https://github.com/node-fetch/node-fetch/pull/1371
+* docs: Fix typo around sending a file by @jimmywarting in https://github.com/node-fetch/node-fetch/pull/1381
+* core: (http.request): Cast URL to string before sending it to NodeJS core by @jimmywarting in https://github.com/node-fetch/node-fetch/pull/1378
+* core: handle errors from the request body stream by @mdmitry01 in https://github.com/node-fetch/node-fetch/pull/1392
+* core: Better handle wrong redirect header in a response by @tasinet in https://github.com/node-fetch/node-fetch/pull/1387
+* core: Don't use buffer to make a blob by @jimmywarting in https://github.com/node-fetch/node-fetch/pull/1402
+* docs: update readme for TS @types/node-fetch by @adamellsworth in https://github.com/node-fetch/node-fetch/pull/1405
+* core: Fix logical operator priority to disallow GET/HEAD with non-empty body by @maxshirshin in https://github.com/node-fetch/node-fetch/pull/1369
+* core: Don't use global buffer by @jimmywarting in https://github.com/node-fetch/node-fetch/pull/1422
+* ci: fix main branch by @dnalborczyk in https://github.com/node-fetch/node-fetch/pull/1429
+* core: use more node: protocol imports by @dnalborczyk in https://github.com/node-fetch/node-fetch/pull/1428
+* core: Warn when using data by @jimmywarting in https://github.com/node-fetch/node-fetch/pull/1421
+* docs: Create SECURITY.md by @JamieSlome in https://github.com/node-fetch/node-fetch/pull/1445
+* core: don't forward secure headers to 3th party by @jimmywarting in https://github.com/node-fetch/node-fetch/pull/1449
+
+## New Contributors
+* @mdmitry01 made their first contribution in https://github.com/node-fetch/node-fetch/pull/1392
+* @tasinet made their first contribution in https://github.com/node-fetch/node-fetch/pull/1387
+* @adamellsworth made their first contribution in https://github.com/node-fetch/node-fetch/pull/1405
+* @maxshirshin made their first contribution in https://github.com/node-fetch/node-fetch/pull/1369
+* @JamieSlome made their first contribution in https://github.com/node-fetch/node-fetch/pull/1445
+
+**Full Changelog**: https://github.com/node-fetch/node-fetch/compare/v3.1.0...v3.1.2
 
 ## 3.1.0
 

@@ -1,6 +1,6 @@
 {
   "name": "node-fetch",
-  "version": "3.1.0",
+  "version": "3.1.1",
   "description": "A light-weight module that brings Fetch API to node.js",
   "main": "./src/index.js",
   "sideEffects": false,

@@ -1,6 +1,6 @@
 {
     "name": "node-fetch",
-    "version": "2.6.6",
+    "version": "2.6.7",
     "description": "A light-weight module that brings window.fetch to node.js",
     "main": "lib/index.js",
     "browser": "./browser.js",

@@ -13,16 +13,29 @@ import https from 'https';
 import zlib from 'zlib';
 import Stream from 'stream';
 
-import Body, { writeToStream, getTotalBytes } from './body';
-import Response from './response';
-import Headers, { createHeadersLenient } from './headers';
-import Request, { getNodeRequestOptions } from './request';
-import FetchError from './fetch-error';
-import AbortError from './abort-error';
+import Body, { writeToStream, getTotalBytes } from './body.js';
+import Response from './response.js';
+import Headers, { createHeadersLenient } from './headers.js';
+import Request, { getNodeRequestOptions } from './request.js';
+import FetchError from './fetch-error.js';
+import AbortError from './abort-error.js';
+
+import whatwgUrl from 'whatwg-url';
+
+const URL = Url.URL || whatwgUrl.URL;
 
 // fix an issue where "PassThrough", "resolve" aren't a named export for node <10
 const PassThrough = Stream.PassThrough;
-const resolve_url = Url.resolve;
+
+const isDomainOrSubdomain = (destination, original) => {
+	const orig = new URL(original).hostname;
+	const dest = new URL(destination).hostname;
+
+	return orig === dest || (
+		orig[orig.length - dest.length - 1] === '.' && orig.endsWith(dest)
+	);
+};
+
 
 /**
  * Fetch function
@@ -109,7 +122,19 @@ export default function fetch(url, opts) {
 				const location = headers.get('Location');
 
 				// HTTP fetch step 5.3
-				const locationURL = location === null ? null : resolve_url(request.url, location);
+				let locationURL = null;
+				try {
+					locationURL = location === null ? null : new URL(location, request.url).toString();
+				} catch (err) {
+					// 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) {
@@ -154,9 +179,15 @@ export default function fetch(url, opts) {
 							body: request.body,
 							signal: request.signal,
 							timeout: request.timeout,
-                            size: request.size
+              size: request.size
 						};
 
+						if (!isDomainOrSubdomain(request.url, locationURL)) {
+							for (const name of ['authorization', 'www-authenticate', 'cookie', 'cookie2']) {
+								requestOpts.headers.delete(name);
+							}
+						}
+
 						// HTTP-redirect fetch step 9
 						if (res.statusCode !== 303 && request.body && getTotalBytes(request) === null) {
 							reject(new FetchError('Cannot follow redirect with body being a readable stream', 'unsupported-redirect'));

@@ -1,7 +1,6 @@
 import * as http from 'http';
 import { parse } from 'url';
 import * as zlib from 'zlib';
-import * as stream from 'stream';
 import { multipart as Multipart } from 'parted';
 
 let convert;
@@ -66,6 +65,12 @@ export default class TestServer {
 			}));
 		}
 
+		if (p.startsWith('/redirect-to/3')) {
+			res.statusCode = p.slice(13, 16);
+			res.setHeader('Location', p.slice(17));
+			res.end();
+		}
+
 		if (p === '/gzip') {
 			res.statusCode = 200;
 			res.setHeader('Content-Type', 'text/plain');

@@ -1569,6 +1569,53 @@ describe('node-fetch', () => {
 		});
 	});
 
+	it('should not forward secure headers to 3th party', () => {
+		return fetch(`${base}redirect-to/302/https://httpbin.org/get`, {
+			headers: new Headers({
+				cookie: 'gets=removed',
+				cookie2: 'gets=removed',
+				authorization: 'gets=removed',
+				'www-authenticate': 'gets=removed',
+				'other-safe-headers': 'stays',
+				'x-foo': 'bar'
+			})
+		}).then(res => res.json()).then(json => {
+			const headers = new Headers(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 httpbin
+			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('should forward secure headers to same host', () => {
+		return fetch(`${base}redirect-to/302/${base}inspect`, {
+			headers: new Headers({
+				cookie: 'is=cookie',
+				cookie2: 'is=cookie2',
+				authorization: 'is=authorization',
+				'other-safe-headers': 'stays',
+				'www-authenticate': 'is=www-authenticate',
+				'x-foo': 'bar'
+			})
+		}).then(res => res.json().then(json => {
+			const headers = new Headers(json.headers);
+			// Safe headers are not removed
+			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
+			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');
+			expect(headers.get('authorization')).to.equal('is=authorization');
+		}));
+	});
+
 	it('should allow PATCH request', function() {
 		const url = `${base}inspect`;
 		const opts = {

@@ -21,6 +21,7 @@ import Request, {getNodeRequestOptions} from './request.js';
 import {FetchError} from './errors/fetch-error.js';
 import {AbortError} from './errors/abort-error.js';
 import {isRedirect} from './utils/is-redirect.js';
+import {isDomainOrSubdomain} from './utils/is.js';
 import {parseReferrerPolicyFromHeader} from './utils/referrer.js';
 
 export {Headers, Request, Response, FetchError, AbortError, isRedirect};
@@ -188,6 +189,18 @@ export default async function fetch(url, options_) {
 							referrerPolicy: request.referrerPolicy
 						};
 
+						// when forwarding sensitive headers like "Authorization",
+						// "WWW-Authenticate", and "Cookie" to untrusted targets.
+						// These headers will be ignored when following a redirect to a domain
+						// 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)) {
+							for (const name of ['authorization', 'www-authenticate', 'cookie', 'cookie2']) {
+								requestOptions.headers.delete(name);
+							}
+						}
+
 						// HTTP-redirect fetch step 9
 						if (response_.statusCode !== 303 && request.body && options_.body instanceof Stream.Readable) {
 							reject(new FetchError('Cannot follow redirect with body being a readable stream', 'unsupported-redirect'));

@@ -56,3 +56,29 @@ export const isAbortSignal = object => {
 		)
 	);
 };
+
+/**
+ * isDomainOrSubdomain reports whether sub is a subdomain (or exact match) of
+ * the parent domain.
+ *
+ * Both domains must already be in canonical form.
+ * @param {string|URL} sub
+ * @param {string|URL} parent
+ */
+export const isDomainOrSubdomain = (sub, parent) => {
+	const a = new URL(sub).hostname;
+	const b = new URL(parent).hostname;
+
+	if (a === b) {
+		return true;
+	}
+
+	// If sub is "foo.example.com" and parent is "example.com",
+	// that means sub must end in "."+parent.
+	// Do it without allocating.
+	if (!a.endsWith(b)) {
+		return false;
+	}
+
+	return a[a.length - b.length - 1] === '.';
+};

@@ -496,6 +496,53 @@ describe('node-fetch', () => {
 		});
 	});
 
+	it('should not forward secure headers to 3th party', async () => {
+		const res = await fetch(`${base}redirect-to/302/https://httpbin.org/get`, {
+			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 httpbin
+		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('should forward secure headers to same host', async () => {
+		const res = await fetch(`${base}redirect-to/302/${base}inspect`, {
+			headers: new Headers({
+				cookie: 'is=cookie',
+				cookie2: 'is=cookie2',
+				authorization: 'is=authorization',
+				'other-safe-headers': 'stays',
+				'www-authenticate': 'is=www-authenticate',
+				'x-foo': 'bar'
+			})
+		});
+
+		const headers = new Headers((await res.json()).headers);
+		// Safe headers are not removed
+		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
+		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');
+		expect(headers.get('authorization')).to.equal('is=authorization');
+	});
+
 	it('should treat broken redirect as ordinary response (follow)', () => {
 		const url = `${base}redirect/no-location`;
 		return fetch(url).then(res => {

@@ -245,6 +245,12 @@ export default class TestServer {
 			res.end();
 		}
 
+		if (p.startsWith('/redirect-to/3')) {
+			res.statusCode = p.slice(13, 16);
+			res.setHeader('Location', p.slice(17));
+			res.end();
+		}
+
 		if (p === '/redirect/302') {
 			res.statusCode = 302;
 			res.setHeader('Location', '/inspect');