-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Enable proxy response to have multiple Set-Cookie raw headers #1101 #1102
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments are for the future as I will take care of this and push it out since it is broken for this case currently. Thanks for this!
@@ -99,18 +106,21 @@ module.exports = { // <-- | |||
|
|||
// message.rawHeaders is added in: v0.11.6 | |||
// https://nodejs.org/api/http.html#http_message_rawheaders | |||
if (proxyRes.rawHeaders != undefined) { | |||
if (proxyRes.rawHeaders !== undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current code here supports if it is undefined
or null
, we should be sure that it is strictly undefined.
@@ -84,12 +84,19 @@ module.exports = { // <-- | |||
*/ | |||
writeHeaders: function writeHeaders(req, res, proxyRes, options) { | |||
var rewriteCookieDomainConfig = options.cookieDomainRewrite, | |||
// In proxyRes.rawHeaders Set-Cookie headers are sparse. | |||
// so, we'll collect Set-Cookie headers, and set them in the response as an array. | |||
set_cookies = [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
camelCase
setHeader = function(key, header) { | ||
if (header != undefined) { | ||
if (rewriteCookieDomainConfig && key.toLowerCase() === 'set-cookie') { | ||
header = common.rewriteCookieDomain(header, rewriteCookieDomainConfig); | ||
if (key === 'Set-Cookie' || key === 'set-cookie') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can still do key.toLowerCase() === 'set-cookie'
cherry-picked, thanks! |
@kriswill Thanks to fix this issue. and sorry for any inconvenience. |
Implementation of proxy HTTP raw headers response processing in 1.16.0 fails to account for multiple
Set-Cookie
headers. This patch accounts for multiple Set-Cookie header, and sets the source response headers to an array.