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: keep cookie-domain in set-cookie header after proxy #806

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

AdoKevin
Copy link

Description

Before this commit, Domain property of set-cookie header was trying to remove. But the regex is incorrect with lazy mode, cause if we had a JSESSIONID=monster;Domain=httpbin.com;, after proxy we got JSESSIONID=monster;ttpbin.com;, only Domain= and first character of domain value was removed.

But in my opinion, we should not remove the cookie domain. If the domain is matched with client side, the domain should set as server expected, otherwise the cookie should drop by browser or client by default. Or if some cases need to change the domain of cookie, use response interceptor is better.

Motivation and Context

How has this been tested?

I've created e2e test for this case, and all unit tests passed.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@chimurai
Copy link
Owner

Opinions aside;

What are the issues you are facing with the current implementation?

Curious wether the http-spec describes how to deal with mismatching domain cookies (drop them or throw error)

@AdoKevin
Copy link
Author

AdoKevin commented Jul 27, 2022

For example, a site at sub.example.com and server has a proxy targets to backend.example.com. backend.example.com response a set-cookie header 'JSESSIONID=monster;Domain=example.com'. What I expected is this cookie would be set with domain example.com for sharing cookie across all sub-domain site of example.com. Current implementation mess up the domain attribute, browser defaults to the host of current sub.example.com.

Form the http-spec rfc6265, user agent should drop cookie with mismatching domain cookies.

The user agent will reject cookies unless the Domain attribute
specifies a scope for the cookie that would include the origin
server. For example, the user agent will accept a cookie with a
Domain attribute of "example.com" or of "foo.example.com" from
foo.example.com, but the user agent will not accept a cookie with a
Domain attribute of "bar.example.com" or of "baz.foo.example.com".

And 5.3-6 said:

If the domain-attribute is non-empty:
If the canonicalized request-host does not domain-match the
domain-attribute:
Ignore the cookie entirely and abort these steps.

@chimurai
Copy link
Owner

Thanks for looking up the client behaviour in the http-spec.

With this change it would mean a potential breaking change, since cookies would be dropped by the client.

I can imagine you would like to have the control to keep the cookies, remove it or even modify it.

Do you known what the default behaviour in Nginx regarding these domain cookies?

@AdoKevin
Copy link
Author

AdoKevin commented Aug 1, 2022

I set up a Nginx proxy server with configuration like:

server {
    listen       80;
    listen  [::]:80;
    server_name  localhost;

    location /auth-static {
	proxy_pass http://172.16.148.220:30010/auth-static;
    }
}

By default, Nginx didn't change anything in set-cookie header, and finally this cookie dropped by Chrome.
image

After I looked up the Nginx document, they launched a directive at version 1.1.15 named proxy_cookie_domain, which can be used to control the cookie domain while proxy handling. Or do you guys consider adding an option like proxy_cookie_domain?

@AdoKevin
Copy link
Author

AdoKevin commented Mar 9, 2023

Hello, any progress on this issue? It bothers me a little bit. Or need any further information?

BTW, the https://github.com/http-party/node-http-proxy#options behaviors' what I expected, keeps the cookie domain value from server, but also provide option cookieDomainRewrite to rewrite cookie domain.

@dantman
Copy link

dantman commented Jan 10, 2024

I was having a related issue because I was trying to use http-proxy-middleware as part of a dev proxy for making changes to an Ory based OAuth setup. Where I needed to leave Domain=example.com in place so that both localtunnel.example.com and sso.example.com could access the cookie. Here's what I've uncovered.

The http-proxy-middleware documentation states the following:

The following options are provided by the underlying http-proxy library.

  • option.cookieDomainRewrite: rewrites domain of set-cookie headers. Possible values:
    • false (default): disable cookie rewriting
    • String: new domain, for example cookieDomainRewrite: "new.domain". To remove the domain, use cookieDomainRewrite: "".

According to the README the default is to leave cookies in place. But that's not actually the case. But it does bring up that cookieDomainRewrite: "" should do the same thing as what this code is supposed to do.


In my situation trying out cookieDomainRewrite it appeared http-proxy's cookieDomainRewrite option did not work in http-proxy-middleware. But on closer look at why and found that it's because I needed to also rewrite the body using the response-interceptor recipe and when selfHandleResponse: true http-proxy does not run any of it's web-outgoing passes including the cookie rewrites.

See this part of http-proxy that is supposed to run the web-outgoing passes, including writeHeaders which includes the cookie rewriting code, but does not run when selfHandleResponse: true.
https://github.com/http-party/node-http-proxy/blob/9b96cd725127a024dabebec6c7ea8c807272223d/lib/http-proxy/passes/web-incoming.js#L175-L179


My recommendation would be to try and make the response interceptor work without selfHandleResponse or otherwise use web-incoming passes as part of response interceptor. Then drop this domain stripping code that actually malforms the cookie instead with a default of cookieDomainRewrite: "" to strip cookie domains by default.

@KyorCode
Copy link

KyorCode commented Mar 5, 2024

An update would be much appreciated since we are also running into this issue.

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.

4 participants