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

Conversation

jimmywarting
Copy link
Collaborator

@jimmywarting jimmywarting commented Jan 13, 2022

Purpose

Avoid forwarding secure headers such as authorization, www-authenticate, cookie & cookie2 when redirecting to a untrusted site. (so it follows browses origin policy)
(matches Go behavior)

Changes

When the location headers redirects to an other hostname, strip away secure request headers meant for the original destination

Additional

Should patch v2 also (not many can update to esm)


  • I updated ./docs/CHANGELOG.md with a link to this PR or Issue
  • I updated ./docs/v3-UPGRADE-GUIDE
  • I updated readme
  • I added unit test(s)

src/utils/is.js Outdated
if (!a.endsWith(b)) {
return false;
}

return a[a.length - b.length - 1] === '.';
Copy link
Member

@LinusU LinusU Jan 13, 2022

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

@jimmywarting jimmywarting Jan 13, 2022

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

@jimmywarting jimmywarting Jan 13, 2022

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

@LinusU LinusU Jan 15, 2022

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

@jimmywarting jimmywarting Jan 15, 2022

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

@jimmywarting jimmywarting Jan 15, 2022

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

@LinusU LinusU Jan 16, 2022

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

gr2m
gr2m approved these changes Jan 13, 2022
Copy link
Collaborator

@gr2m gr2m left a comment

Looks good to me. Feel free to refactor the isDomainOrSubdomain method to make it easier to read, but it's good as is from my perspective.

src/index.js Outdated Show resolved Hide resolved
@jimmywarting jimmywarting merged commit f5d3cf5 into node-fetch:main Jan 14, 2022
8 checks passed
@jimmywarting jimmywarting deleted the fix/secure-headers branch Jan 14, 2022
@gr2m
Copy link
Collaborator

@gr2m gr2m commented Jan 15, 2022

Should patch v2 also (not many can update to esm)

Will you do that yourself or shall we do a follow up issue so we don't forget?

@bitinn
Copy link
Collaborator

@bitinn bitinn commented Jan 15, 2022

Thx for everyone to get to this, this issue can finally be closed :)

But do note the drawback:

#274

@jimmywarting
Copy link
Collaborator Author

@jimmywarting jimmywarting commented Jan 15, 2022

I can fix V2 also

@@ -56,3 +56,20 @@ export const isAbortSignal = object => {
)
);
};

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

@glasser glasser Jan 18, 2022

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

@jimmywarting jimmywarting Jan 18, 2022

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.

* @param {string|URL} original
* @param {string|URL} destination
*/
export const isDomainOrSubdomain = (destination, original) => {
Copy link
Contributor

@glasser glasser Jan 19, 2022

Choose a reason for hiding this comment

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

The names of the variables here don't see to match the values passed in?

if (!isDomainOrSubdomain(request.url, locationURL)) {

Surely locationURL is the destination and request.url is the original?

Copy link
Collaborator Author

@jimmywarting jimmywarting Jan 19, 2022

Choose a reason for hiding this comment

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

I think you are right. it works as intended and how it is suppose to protect you, But the variable names are mixed up

Copy link
Collaborator Author

@jimmywarting jimmywarting Jan 19, 2022

Choose a reason for hiding this comment

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

No rush to get fixed, can create a issue about it if needed

@kentonv
Copy link

@kentonv kentonv commented Jan 21, 2022

This change may match Go behavior but it differs from browser behavior and deviates from the fetch API spec.

In Chrome, if I do:

fetch(url, {headers: {"Authorization": "foo"}})

and this results in a redirect to a third party, Chrome will forward the Authorization header to the third party, assuming that the third party's CORS policy allows it. Note that the CORS policy of the original URL is irrelevant -- the third party controls the policy here. So, in an attack scenario, the attacker controls the policy, so CORS is not a defense.

AFAICT, Chrome's behavior conforms to the Fetch spec. The Fetch spec only says these headers should be removed on a redirect if they were implicitly added by the user-agent in the first place -- i.e. not when they were explicitly passed to fetch() in the request.headers attribute. Since node-fetch presumably never adds these headers implicitly, it seems like this doesn't apply.

I am concerned that this change could actually break legitimate use cases. Imagine api.example.com is a REST API that requires authorization via an Authorization header. Now imagine example.com decides to rebrand as example.org, using 3xx redirects to redirect all requests to the new domain. API clients coded against the old domain will break.

@jimmywarting
Copy link
Collaborator Author

@jimmywarting jimmywarting commented Jan 21, 2022

That may be legit cases...

I have also wonder about the usage of Request.credentials if we somehow could use this in any way. The option same-origin could technically match what we have made in this change and use that as the default. and if credentials is include, then we could allow insecure headers to be forwarded?

Xhr have withCredentials
that has similar options

@kentonv
Copy link

@kentonv kentonv commented Jan 21, 2022

Request.credentials seems to instruct the browser on whether or not it should automatically add cookies and HTTP basic auth credentials to the request. If the application explicitly specifies an Authorization header, though, I don't think Requset.credentials is meant to have any effect on that. In Chrome, at least, an explicitly-specified Authorization header seems to be redirected even when Request.credentials is same-origin (the default).

@glasser
Copy link
Contributor

@glasser glasser commented Jan 21, 2022

@kentonv (Hi!) Just to check, your concern is specifically about Authorization, right? It does seem reasonable to me that cookies should not be sent to a different origin.

@kentonv
Copy link

@kentonv kentonv commented Jan 21, 2022

Hi @glasser! Long time no see.

TBH, I'm not really sure what this should mean for Cookie. Technically, the fetch() spec does not allow the application to specify this header at all, so for a "conformant" implementation the question is moot. But, of course, that part of the fetch spec really only makes sense for browsers.

What makes sense on the server side? I don't really know because I don't know under what circumstances a server would ever send a Cookie header. I'd expect Node apps are mainly using fetch() to call APIs, and APIs rarely if ever use Cookie.

For Cloudflare Workers (of which I'm the lead engineer), our fetch() implementation does have to allow the Cookie header for the proxying use case: many workers receive a request from a client, rewrite it in some way, and then pass it on to an origin server using fetch(). The Cookie header must come along for that ride -- and it's important that it do so even if the hostname changed in the process. But, in CF Workers, proxied requests always have redirect = "manual", because a redirect response should always be returned back to the original client rather than handled within the worker. So, what happens to Cookie on a redirect is moot for the common case.

A Worker can also make a stand-alone fetch() that is not proxying any existing request, and in that case the default redirect = "follow" applies. But this is again typically used for calling APIs, and I don't know why Cookie would be used at all, so it's hard to say what the right behavior is when redirecting.

FWIW, at present Cloudflare Workers has no special logic whatsoever for Cookie or Authorization -- these are just plain old headers to us.

Of course, for the API use case, it's very common for other headers to contain sensitive information, too. Some APIs use non-standard headers like X-Auth-Key for their secrets. But also if the API uses any special headers at all, who knows what is in them? So it feels weird to me to even try to protect this use case from leaking secrets on a redirect... it seems like it's really the application's responsibility to use manual redirect logic if they don't want the whole request to be forwarded.

So in the absence of more information about real-world use cases, my inclination is to say that there's no reason to treat Cookie specially here. But I can definitely see where it is debatable.

@lpinca
Copy link

@lpinca lpinca commented Feb 1, 2022

curl behavior is to not forward the Authorization and Cookie header if the host is different (tls client cert is forwarded) but I agree with @kentonv. This might break legitimate use cases and I don't see why a special logic should be applied to them. There might be many other headers containing sensitive data like X-API-KEY, etc.

@kentonv
Copy link

@kentonv kentonv commented Mar 11, 2022

Interestingly, here's a Stack Overflow question from a real user who legitimately tried to redirect an API, and was frustrated that the Authorization header was dropped:

https://stackoverflow.com/questions/71441897/cloudflare-worker-redirect-stripping-auth-headers/

@jimmywarting
Copy link
Collaborator Author

@jimmywarting jimmywarting commented Mar 11, 2022

yea, indeed interesting... a real problem.
if this api request would have happen in the browser then the Authorization would never be forwarded anyway. we only follow what what the browsers spec say and do...

i really do think it should be up to the developer to say to the http client that it should explicitly forward insecure headers in case of a redirect. I'm not sure if I would trust the api to keep my credentials safe and sound if some malicious user finds a way to redirect some endpoint to a 3th party domain and steal my credentials.

i think every http client, http, https, http2, curl, wget go, ruby and everything else should have something some kind of option to choose from when making the request in case of a redirect to a other 3th party domain

fetch(url, {
  forward_headers_and_body_when_redirect_to_untrusted_domain: true  // false by default
})

if it don't work in the browser or Go HTTP anyway then you are trying to solve the problem in a wrong way anyway that don't work for all http client, there could be other api users that use some other http client that also don't support forwarding secure headers

there is also other ways to tackle the problem like
redirectTo(url?credentials=${headers.get('authentication')} <- but this is pretty bad/smelly


I almost do think authentication header should be forwared but not cookies cuz they are set to a specific domain/path
so why should authentication be treaded any differently then x-api-key?

@jimmywarting
Copy link
Collaborator Author

@jimmywarting jimmywarting commented Mar 11, 2022

maybe Alt-Svc could be a solution?

@kentonv
Copy link

@kentonv kentonv commented Mar 11, 2022

if this api request would have happen in the browser then the Authorization would never be forwarded anyway. we only follow what what the browsers spec say and do...

No no, to reiterate: Browsers do forward the Authorization header (assuming it was explicitly passed to fetch()). The spec does not say to remove this header. So this PR actually goes against spec. See my earlier comments in this thread...

However, given that so many other HTTP client libraries have implemented this behavior, I guess realistically it just isn't possible to redirect an API across domains today, so maybe that use case is moot.

@jimmywarting
Copy link
Collaborator Author

@jimmywarting jimmywarting commented Mar 11, 2022

Do you know what alt-sve will do with cookies and other headers if it decided to try another url the next time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants