Implement proper cookie jar for server side requests

[toddw]

When a web browser receives set-cookie headers in a response from a server it will store those in a "cookie-jar" and send those cookies, as long as they haven't expired in future requests to that domain. Our httpClient in getHttpClient.js makes a good pass at emulating the basics of a cookie jar but it is lacking some functionality. The reason why this is important is for situations where you make multiple API requests on the server before returning a response to the client. If the second request relies on cookies set by the first request, our httpClient will need to send those cookies in it's request. And so on with a third, fourth, etc… requests.

Things that can be improved:

1. Set-Cookie headers should set cookies per the host settings that a browser would respect. The cookies should only be sent on future requests from a matching domain. Cookies can be set for specific subdomains or they can use a wildcard subdomain. We should follow the same rules.
2. We should respect expiration of cookies.
3. Currently the set-cookie header from a request appears to wipe out any of the previous cookies and go with the newly set cookies. This is undesired behavior, it should "merge" them giving a precedence to the new cookies received by the latest Set-Cookie header.

While doing all of that we also forward "all" of the cookies back to the browser. There is a very realistic warning in getHttpClient that talks about mixing multiple api endpoints with server side requests. Currently, I'm not aware of a great solution for that problem other than suggesting all server side API requests should be to the same location. If you must hit a secondary API server that you do not control, it is best to do that after componentDidMount and let the browser handle that. This last part is only partially related to the cookie jar.

[zamotany]

Can you provide some example?? Because I don't really see what you mean.

[toddw]

Example: https://github.com/toddw/gluestick-cookie-bug-example

[toddw]

Trouble code: https://github.com/TrueCar/gluestick-shared/blob/master/src/lib/getHttpClient.js#L69-L92

[toddw]

As you will see, all cookies are merged together but it shouldn't be. We should only be merging cookies from the same domain. In this case the node server acts like a web browser and has to handle cookies on it's own since this is all happening before we get to the browser. Cookies on the server side should behave more like cookies do in the browser. If a browser makes a request to exampleA.com and exampleA.com sets cookies, then it makes a request to exampleB.com it shouldn't send cookies it got back from exampleA.com. However if a subsequent request to exampleA.com occurs then whatever cookies were sent back from exampleA.com should be sent to exampleA on the next request.

Furthermore we then send cookies back to the browser https://github.com/TrueCar/gluestick-shared/blob/master/src/lib/getHttpClient.js#L80

However, we shouldn't be sending cookies to the browser unless the domain where the cookie was set matches the request.host we are sending it back to.

[toddw]

The example gluestick project linked above expands on the issue and the instructions.

[zamotany]

From what I know there is no easy way to solve this. We can't achieve such behaviour by just sending cookies in headers, we would need to execute some JS that will set appropriate cookies per domain, but those cookies won't be secure.

One of the solutions would be to use some custom header field as a transport and store this data in localstorage. Still we would need some client-side parser to read it and store it. All request on client should be proxied in that case.

[zamotany]

Also I'm not sure if this cookie jar should be provided by default. Some people doesn't use cookies at all (including me). They use JWT instead or sth like JSON Web Cookie.

[toddw]

we would need to execute some JS that will set appropriate cookies per domain, but those cookies won't be secure.

I assumed we would need to write JS to organize the cookies. Can you help me understand what the security issue would be?

[zamotany]

I'm not expert on cookies, but I tried to set cookie secured cookie with JS and I couldn't, not sure if it's a limitation of console. I'm not sure if inability to set secured cookies will be a problem. What is the use case for cookies in TrueCar apps?

[toddw]

We can move this out of the 1.0 scope. As long as we don't lose the current functionality then I'm fine moving forward. I have an idea of how I'd live to solve this ticket and I can program a solution once v2 gets further along. Thanks for looking into this!