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

Add proxyCookies option #358

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from
Draft

Conversation

SebastiaanYN
Copy link

@SebastiaanYN SebastiaanYN commented May 3, 2020

This PR adds a proxyCookies option which resolves #333.

An issue with this PR is that it includes the cookie package 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.

@codecov
Copy link

codecov bot commented May 3, 2020

Codecov Report

Merging #358 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               dev      #358   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           31        31           
  Branches        13        13           
=========================================
  Hits            31        31           
Impacted Files Coverage Δ
lib/module.js 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f20c51...891c34a. Read the comment docs.

@SebastiaanYN SebastiaanYN changed the title feat: add proxyCookies option Add proxyCookies option May 3, 2020

// If the res already has a Set-Cookie header it should be merged
if (ctx.res.getHeader('Set-Cookie')) {
const newCookies = mergeSetCookies(
Copy link
Member

@pi0 pi0 May 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Author

@SebastiaanYN SebastiaanYN May 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
Copy link
Member

@pi0 pi0 May 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest turning off by default because of security implications it has (private SSR API calls may leak headers)

@pi0 pi0 mentioned this pull request May 3, 2020
@k0mar12
Copy link

k0mar12 commented Nov 8, 2020

@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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be cookies.set(name, setCookie).
Otherwise, cookie might be duplicated by HttpOnly/Domain/SameSite/Max-Age/etc parameters.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example:
1st row - cookie from backend
2nd row - cookie from ssr-cookie-proxy

image


// Combine the cookies set on axios with the new cookies and serialize them
const cookie = serializeCookies({
...parseCookies(axios.defaults.headers.common.cookie),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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']) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

@arkhamvm
Copy link

@pi0 can you clarify please, Nuxt 3 have same behavior, or this cookie-proxy is obsolete?

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.

proxy cookies
4 participants