fix: keep cookie-domain in set-cookie header after proxy

AdoKevin commented 2 years ago

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

[x] 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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

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 commented 1 year ago

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 commented 8 months ago

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 commented 6 months ago

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

chimurai / http-proxy-middleware