From 2546582faa74d896b316e7db6e8b73101212b732 Mon Sep 17 00:00:00 2001 From: Maxwell Gerber Date: Thu, 7 Jul 2022 11:03:57 -0700 Subject: [PATCH 1/2] fix(Headers): don't forward secure headers on protocol change --- src/index.js | 7 +++++-- src/utils/is.js | 14 ++++++++++++++ test/main.js | 38 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 55 insertions(+), 4 deletions(-) diff --git a/src/index.js b/src/index.js index 82efc9888..7c4aee87b 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/utils/is.js b/src/utils/is.js index 00eb89b0e..f9e467e45 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; +}; diff --git a/test/main.js b/test/main.js index 5df12c4ca..a65aed2ac 100644 --- a/test/main.js +++ b/test/main.js @@ -7,6 +7,7 @@ import path from 'node:path'; import stream from 'node:stream'; import vm from 'node:vm'; import zlib from 'node:zlib'; +import https from 'node:https'; import {text} from 'stream-consumers'; import AbortControllerMysticatea from 'abort-controller'; @@ -33,7 +34,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 +523,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 +543,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); From 70ec76a01678b3e8c39b134babd9e47394d0de84 Mon Sep 17 00:00:00 2001 From: Maxwell Gerber Date: Thu, 7 Jul 2022 18:33:34 -0700 Subject: [PATCH 2/2] fix lint --- test/main.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/main.js b/test/main.js index a65aed2ac..7e195f005 100644 --- a/test/main.js +++ b/test/main.js @@ -7,7 +7,6 @@ import path from 'node:path'; import stream from 'node:stream'; import vm from 'node:vm'; import zlib from 'node:zlib'; -import https from 'node:https'; import {text} from 'stream-consumers'; import AbortControllerMysticatea from 'abort-controller'; @@ -544,7 +543,7 @@ describe('node-fetch', () => { }); 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`, { + 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',