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.

Sign up for GitHub

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

Add proxyCookies option #358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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)

proxy: false,
retry: false,
https,
Expand Down
72 changes: 72 additions & 0 deletions lib/plugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import Axios from 'axios'
import defu from 'defu'
<% if (options.retry) { %>import axiosRetry from 'axios-retry'<% } %>
<% if (options.proxyCookies) { %>import { parse as parseCookies } from 'cookie'<% } %>

const $nuxt = typeof window !== 'undefined' && window['$<%= options.globalName %>']

Expand Down Expand Up @@ -179,6 +180,68 @@ const setupProgress = (axios) => {
axios.defaults.onDownloadProgress = onProgress
}<% } %>

<% if (options.proxyCookies) { %>
const proxyCookies = (ctx, axios, response) => {
const parseSetCookies = cookies => {
return cookies
.map(cookie => cookie.split(';')[0])
.reduce((obj, cookie) => ({
...obj,
...parseCookies(cookie)
}), {})
}

const serializeCookies = cookies => {
return Object
.entries(cookies)
.map(([name, value]) => `${name}=${encodeURIComponent(value)}`)
.join('; ')
}

const mergeSetCookies = (oldCookies, newCookies) => {
const cookies = new Map()

const add = setCookie => {
const cookie = setCookie.split(';')[0]
const name = Object.keys(parseCookies(cookie))[0]

cookies.set(name, cookie)
Copy link

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.

Copy link

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

}

oldCookies.forEach(add)
newCookies.forEach(add)

return [...cookies.values()]
}

const arrayify = obj => Array.isArray(obj) ? obj : [obj]

if (response.headers['set-cookie']) {
Copy link

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

const setCookies = arrayify(response.headers['set-cookie'])

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

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

...parseSetCookies(setCookies)
})

axios.defaults.headers.common.cookie = cookie

// 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 •
edited
Loading

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 •
edited
Loading

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.

arrayify(ctx.res.getHeader('Set-Cookie')),
setCookies
)

ctx.res.setHeader('Set-Cookie', newCookies)
} else {
ctx.res.setHeader('Set-Cookie', setCookies)
}
}
}
<% } %>

export default (ctx, inject) => {
// baseURL
const baseURL = process.browser
Expand Down Expand Up @@ -213,6 +276,15 @@ export default (ctx, inject) => {

const axios = createAxiosInstance(axiosOptions)

<% if (options.proxyCookies) { %>
// Proxy cookies
if (process.server) {
axios.onResponse(response => {
proxyCookies(ctx, axios, response)
})
}
<% } %>

// Inject axios to the context as $axios
ctx.$axios = axios
inject('axios', axios)
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
"axios": "^0.19.2",
"axios-retry": "^3.1.8",
"consola": "^2.11.3",
"cookie": "^0.4.1",
"defu": "^2.0.2"
},
"devDependencies": {
Expand Down
5 changes: 5 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3289,6 +3289,11 @@ cookie@^0.3.1:
resolved "https://registry.yarnpkg.com/cookie/-/cookie-0.3.1.tgz#e7e0a1f9ef43b4c8ba925c5c5a96e806d16873bb"
integrity sha1-5+Ch+e9DtMi6klxcWpboBtFoc7s=

cookie@^0.4.1:
version "0.4.1"
resolved "https://registry.yarnpkg.com/cookie/-/cookie-0.4.1.tgz#afd713fe26ebd21ba95ceb61f9a8116e50a537d1"
integrity sha512-ZwrFkGJxUR3EIoXtO+yVE69Eb7KlixbaeAWfBQB9vVsNn/o+Yw69gBWSSDK825hQNdN+wF8zELf3dFNl/kxkUA==

copy-concurrently@^1.0.0:
version "1.0.5"
resolved "https://registry.yarnpkg.com/copy-concurrently/-/copy-concurrently-1.0.5.tgz#92297398cae34937fcafd6ec8139c18051f0b5e0"
Expand Down