Skip to content

feat: Add mergeCookies option #1318

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

Closed
wants to merge 1 commit into from
Closed

feat: Add mergeCookies option #1318

wants to merge 1 commit into from

Conversation

RidgeA
Copy link

@RidgeA RidgeA commented Jan 14, 2019

mergeCookie options allow to merge set-cookie header form passed request object and from a target response

Test added.
Documentation updated.

Use Case:
In the project, I'm working on currently, we are using Express-Gateway (https://github.com/ExpressGateway/express-gateway) as a single entry point for a set of microservices.
Express-Gateway uses http-proxy when proxeing requests to a destination service.
In the gateway we have to setup a cookie with CSRF token.
But some endpoints of our microservices setups it's own cookies and in this case all cookies, that has been setuped in gateway wipes out by cookies from a microservice response.

Raw example:

"use strict";

const http = require("http");
const httpProxy = require("http-proxy");
const proxy = httpProxy.createProxyServer({});

const gateway = http.createServer((req, res) => {
  res.setHeader('set-cookie', ["gateway=true; Path=/"]);
  proxy.web(req, res, {target: "http://localhost:3002"});
});

gateway.listen(3003);

const backend = http.createServer((req, res) => {
  res.setHeader("set-cookie", ["backend=true; Path=/", "another_backend_cookie=true; Path=/"]);
  res.end("OK");
});

backend.listen(3002);

Curl:

$ curl -v http://localhost:3003
* Rebuilt URL to: http://localhost:3003/
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 3003 (#0)
> GET / HTTP/1.1
> Host: localhost:3003
> User-Agent: curl/7.61.1
> Accept: */*
> 
< HTTP/1.1 200 OK
< set-cookie: backend=true; Path=/
< set-cookie: another_backend_cookie=true; Path=/
< date: Mon, 14 Jan 2019 12:24:27 GMT
< connection: close
< content-length: 2
< 
* Closing connection 0
OK

By this pull-request I want to add one more option that will instruct proxy-server to merge set-cookie header instead of overwriting it.
Existing behaviour remains the same.

`mergeCookie` options allows to merge `set-cookie` header form passed request object and from target
@RidgeA
Copy link
Author

RidgeA commented Feb 12, 2019

Hi!
Is there any chance that it will be merged?
Do I need to change something ?

@jsmylnycky
Copy link
Contributor

Hi @RidgeA

I'm helping to get things moving on this project again, and this looks like a good enough feature to consider. We'll be mapping out the next release and I'll make sure to get this on the radar.

Thanks!

@RidgeA
Copy link
Author

RidgeA commented Aug 27, 2019

@jsmylnycky wow, glad to hear :-)
I forget about this PR :-)
As I can see there are some issues from CI, is there something that I should do with it?

@jsmylnycky
Copy link
Contributor

@RidgeA If you rebase from master, that should get it through CI. Some things got screwed up due to the org change.

@RidgeA
Copy link
Author

RidgeA commented Aug 27, 2019

@jsmylnycky I will, np.
but I've deleted the repository from my account due to lack of activity in the project.
I could create a new fork and a new PR, but maybe there are better options?

@jsmylnycky
Copy link
Contributor

@RidgeA Whichever you're most comfortable with...either fork and PR into it, or just clone this repository directly, checkout a new branch, and submit a PR against master.

If you don't get around to it, I can still mark this for a future release and put it in myself with your changed files as a reference.

@RidgeA
Copy link
Author

RidgeA commented Aug 28, 2019

@jaggernoth I created a new PR - #1375
Thank you !

@RidgeA RidgeA closed this Aug 28, 2019
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.

2 participants