Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(Headers): don't forward secure headers to 3th party #1449

Merged
merged 2 commits into from Jan 14, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/index.js
Expand Up @@ -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};
Expand Down Expand Up @@ -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.
jimmywarting marked this conversation as resolved.
Show resolved Hide resolved
// 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'));
Expand Down
26 changes: 26 additions & 0 deletions src/utils/is.js
Expand Up @@ -56,3 +56,29 @@ export const isAbortSignal = object => {
)
);
};

/**
* isDomainOrSubdomain reports whether sub is a subdomain (or exact match) of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this use the public suffix list instead? https://publicsuffix.org/

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that would be necessary.

* 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] === '.';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about adding the . to the endsWith call?

Suggested change
if (!a.endsWith(b)) {
return false;
}
return a[a.length - b.length - 1] === '.';
return a.endsWith(`.${b}`)

I think that this is much easier to read ☺️

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you are making a request to fridas-blog.uk.com and it redirects to uk.com then uk.com should not know about the cookie... cuz the cookie can be tied to fridas-blog.uk.com

a = referer (original request) fridas-blog.uk.com
b = destination (Location) uk.com

a.endsWith('.${b}') === true
b is not the same host or a subdomain of a...

so i guess it can't work...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you are only after something that is sorter and dose the same thing:

export const isDomainOrSubdomain = (a, b) => {
  a = new URL(a).hostname
  b = new URL(b).hostname
  return a === b || (
    a[a.length - b.length - 1] === '.' && a.endsWith(b)
  )
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't understand how the two checks are different? As far as I can tell, unless I'm missing something, the two functions below behave the exact same way?

function one (a, b) {
  return a === b || (
    a[a.length - b.length - 1] === '.' && a.endsWith(b)
  )
}

function two (a, b) {
  return a === b || a.endsWith(`.${b}`)
}

The case you are mentioning seems to be handled wrong in the committed code then?

> isDomainOrSubdomain('http://uk.com', 'http://fridas-blog.uk.com')
true

(extra ping @jimmywarting since this is already merged)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, double checked again and you are right... they are equally the same, But there is no mistake in the merged code...

I was blinded by the order of arguments passed down to isDomainOrSubdomain and mixing up whats gets passed down to the function and in which order. i was just so blinded by how Go and follow-redirects also did it using the dot index

But at least there is no rush to change it now... can change it later

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a & b was a stupid variable name...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍

Submitted PR to make the code easier to understand here: #1455

};
47 changes: 47 additions & 0 deletions test/main.js
Expand Up @@ -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 => {
Expand Down
6 changes: 6 additions & 0 deletions test/utils/server.js
Expand Up @@ -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');
Expand Down