Add proxyCookies option #358
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #358 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 31 31
Branches 13 13
=========================================
Hits 31 31
Continue to review full report at Codecov.
|
|
|
||
| // If the res already has a Set-Cookie header it should be merged | ||
| if (ctx.res.getHeader('Set-Cookie')) { | ||
| const newCookies = mergeSetCookies( |
There was a problem hiding this comment.
It is fine by spec to send multiple Set-Cookie headers and it would be a much smaller implementation. ++ we don't need an external dependency/process to parse and merge cookies
By your comment: If you make multiple requests that all overwrite a certain cookie, that could increase the header size quite a bit
Yes it would but backend shouldn't send multiple Set-Cookies if it does, we should transparently pass them to the client like how it works on client (if each response does a Set-Cookie browser applies separately and in order)
If you are getting this behavior by your implementation, probably reason is during single SSR lifecycle, after we get first Set-Cookie, should pass it to next SSR API-calls :)
There was a problem hiding this comment.
Yes it would but backend shouldn't send multiple Set-Cookies if it does, we should transparently pass them to the client like how it works on client (if each response does a Set-Cookie browser applies separately and in order)
The issue isn't with a single request to the backend sending duplicate Set-Cookie headers, but with multiple separate requests which all receive a cookie.
Consider you have the route /cookie which returns a header Set-Cookie: count=i where i starts at 0 and for each subsequent request count is incremented in the cookie response. The first request you get count=0, second count=1, third count=2. And then the following code.
async asyncData({ $axios }) {
await $axios.$get('/cookie'); // Set-Cookie: count=0
await $axios.$get('/cookie'); // Set-Cookie: count=1
await $axios.$get('/cookie'); // Set-Cookie: count=2
}When you're in a browser, you'll end up with count=2, because each request is done one at a time.
When you're on the server and the Set-Cookie header isn't merged, the client will receive all the headers at once. It's then left up to the browser to set the correct cookie, which should be the last one, but that can't be guaranteed. I'm also not certain if Node guarantees the headers to be sent in the same order.
Set-Cookie: count=0
Set-Cookie: count=1
Set-Cookie: count=2
If the Set-Cookie headers are merged, you end up with the following response, and the browser should end up with the correct cookie.
Set-Cookie: count=2
There was a problem hiding this comment.
I know what you mean :) but it comes with the cost of much more parse/seralize logic and a library import and Something like a Set-Cookie: count should not honestly happen by the backend. Also, request ordering may affect which Set-Cookie finally takes over and it is hidden by the client then.
There was a problem hiding this comment.
If you're fine with edge cases not being covered that's fine by me. I think in most cases there won't be any issues if Set-Cookie headers aren't merged.
The cookie library is also used to parse the cookies and store them on the Axios instance. This is necessary when dealing, for example, with access tokens that get refreshed after a request, and the next request requires the new token.
Ideally, all this logic is only included in the server-side bundle, but I don't know whether that is possible here.
| @@ -59,6 +59,7 @@ function axiosModule (_moduleOptions) { | |||
| progress: true, | |||
| proxyHeaders: true, | |||
| proxyHeadersIgnore: ['accept', 'host', 'cf-ray', 'cf-connecting-ip', 'content-length', 'content-md5', 'content-type'], | |||
| proxyCookies: true, | |||
There was a problem hiding this comment.
I would suggest turning off by default because of security implications it has (private SSR API calls may leak headers)
|
@pi0 Still get problems with ssr cookie. Any idea about merge this pull request? |
| const cookie = setCookie.split(';')[0] | ||
| const name = Object.keys(parseCookies(cookie))[0] | ||
|
|
||
| cookies.set(name, cookie) |
There was a problem hiding this comment.
It should be cookies.set(name, setCookie).
Otherwise, cookie might be duplicated by HttpOnly/Domain/SameSite/Max-Age/etc parameters.
There was a problem hiding this comment.
For example:
1st row - cookie from backend
2nd row - cookie from ssr-cookie-proxy
|
|
||
| // Combine the cookies set on axios with the new cookies and serialize them | ||
| const cookie = serializeCookies({ | ||
| ...parseCookies(axios.defaults.headers.common.cookie), |
There was a problem hiding this comment.
This line can fail with ERROR argument str must be a string, cause axios.defaults.headers.common.cookie can be undefined
|
|
||
| const arrayify = obj => Array.isArray(obj) ? obj : [obj] | ||
|
|
||
| if (response.headers['set-cookie']) { |
There was a problem hiding this comment.
| if (response.headers['set-cookie']) { | |
| if (response.headers['set-cookie'] && !response.headersSent) { |
We must check headersSent property, or we can get error:
Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
|
@pi0 can you clarify please, Nuxt 3 have same behavior, or this cookie-proxy is obsolete? |
This PR adds a
proxyCookiesoption which resolves #333.An issue with this PR is that it includes the
cookiepackage and other code both on the server and client, while it only has to run on the server. It would be better to only include it on the server, but I'm unsure how that can be done.I also haven't added any tests, because I'm not sure how this can be properly tested.