From 6b2cef65c99cb40fb9ca0789670b9ea9f5fcc2dd Mon Sep 17 00:00:00 2001 From: Brian White Date: Thu, 9 Feb 2017 06:49:39 -0500 Subject: [PATCH 1/2] http: append Cookie header values with semicolon Previously, separate incoming Cookie headers would be concatenated with a comma, which can cause confusion in userland code when it comes to parsing the final Cookie header value. This commit concatenates using a semicolon instead. Fixes: https://github.com/nodejs/node/issues/11256 PR-URL: https://github.com/nodejs/node/pull/11259 Reviewed-By: James M Snell Reviewed-By: Roman Reiss Reviewed-By: Matteo Collina Reviewed-By: Ben Noordhuis --- lib/_http_incoming.js | 20 ++++++++++--------- .../test-http-incoming-matchKnownFields.js | 11 ++++++---- .../test-http-server-multiheaders2.js | 4 +++- 3 files changed, 21 insertions(+), 14 deletions(-) diff --git a/lib/_http_incoming.js b/lib/_http_incoming.js index 53419323f46b0d..d8388917902dd0 100644 --- a/lib/_http_incoming.js +++ b/lib/_http_incoming.js @@ -194,6 +194,9 @@ function matchKnownFields(field) { case 'Set-Cookie': case 'set-cookie': return '\u0001'; + case 'Cookie': + case 'cookie': + return '\u0002cookie'; // The fields below are not used in _addHeaderLine(), but they are common // headers where we can avoid toLowerCase() if the mixed or lower case // versions match the first time through. @@ -215,9 +218,6 @@ function matchKnownFields(field) { case 'Content-Encoding': case 'content-encoding': return '\u0000content-encoding'; - case 'Cookie': - case 'cookie': - return '\u0000cookie'; case 'Origin': case 'origin': return '\u0000origin'; @@ -263,18 +263,20 @@ function matchKnownFields(field) { // // Per RFC2616, section 4.2 it is acceptable to join multiple instances of the // same header with a ', ' if the header in question supports specification of -// multiple values this way. If not, we declare the first instance the winner -// and drop the second. Extended header fields (those beginning with 'x-') are -// always joined. +// multiple values this way. The one exception to this is the Cookie header, +// which has multiple values joined with a '; ' instead. If a header's values +// cannot be joined in either of these ways, we declare the first instance the +// winner and drop the second. Extended header fields (those beginning with +// 'x-') are always joined. IncomingMessage.prototype._addHeaderLine = _addHeaderLine; function _addHeaderLine(field, value, dest) { field = matchKnownFields(field); var flag = field.charCodeAt(0); - if (flag === 0) { + if (flag === 0 || flag === 2) { field = field.slice(1); - // Make comma-separated list + // Make a delimited list if (typeof dest[field] === 'string') { - dest[field] += ', ' + value; + dest[field] += (flag === 0 ? ', ' : '; ') + value; } else { dest[field] = value; } diff --git a/test/parallel/test-http-incoming-matchKnownFields.js b/test/parallel/test-http-incoming-matchKnownFields.js index 7411be4e847d73..f301b9169fcd2b 100644 --- a/test/parallel/test-http-incoming-matchKnownFields.js +++ b/test/parallel/test-http-incoming-matchKnownFields.js @@ -6,9 +6,10 @@ const IncomingMessage = require('http').IncomingMessage; function checkDest(field, result, value) { const dest = {}; - if (value) dest[field] = 'test'; const incomingMessage = new IncomingMessage(field); // dest is changed by IncomingMessage._addHeaderLine + if (value) + incomingMessage._addHeaderLine(field, 'test', dest); incomingMessage._addHeaderLine(field, value, dest); assert.deepStrictEqual(dest, result); } @@ -49,7 +50,7 @@ checkDest('age', {age: 'test'}, 'value'); checkDest('Expires', {expires: undefined}); checkDest('expires', {expires: 'test'}, 'value'); checkDest('Set-Cookie', {'set-cookie': [undefined]}); -checkDest('set-cookie', {'set-cookie': [undefined]}); +checkDest('set-cookie', {'set-cookie': ['test', 'value']}, 'value'); checkDest('Transfer-Encoding', {'transfer-encoding': undefined}); checkDest('transfer-encoding', {'transfer-encoding': 'test, value'}, 'value'); checkDest('Date', {date: undefined}); @@ -64,8 +65,8 @@ checkDest('Vary', {vary: undefined}); checkDest('vary', {vary: 'test, value'}, 'value'); checkDest('Content-Encoding', {'content-encoding': undefined}, undefined); checkDest('content-encoding', {'content-encoding': 'test, value'}, 'value'); -checkDest('Cookies', {cookies: undefined}); -checkDest('cookies', {cookies: 'test, value'}, 'value'); +checkDest('Cookie', {cookie: undefined}); +checkDest('cookie', {cookie: 'test; value'}, 'value'); checkDest('Origin', {origin: undefined}); checkDest('origin', {origin: 'test, value'}, 'value'); checkDest('Upgrade', {upgrade: undefined}); @@ -88,3 +89,5 @@ checkDest('X-Forwarded-Host', {'x-forwarded-host': undefined}); checkDest('x-forwarded-host', {'x-forwarded-host': 'test, value'}, 'value'); checkDest('X-Forwarded-Proto', {'x-forwarded-proto': undefined}); checkDest('x-forwarded-proto', {'x-forwarded-proto': 'test, value'}, 'value'); +checkDest('X-Foo', {'x-foo': undefined}); +checkDest('x-foo', {'x-foo': 'test, value'}, 'value'); diff --git a/test/parallel/test-http-server-multiheaders2.js b/test/parallel/test-http-server-multiheaders2.js index 53e68e6f0d3e74..febffe05235b7f 100644 --- a/test/parallel/test-http-server-multiheaders2.js +++ b/test/parallel/test-http-server-multiheaders2.js @@ -54,8 +54,10 @@ const srv = http.createServer(function(req, res) { 'foo', 'header parsed incorrectly: ' + header); }); multipleAllowed.forEach(function(header) { + const sep = (header.toLowerCase() === 'cookie' ? '; ' : ', '); assert.strictEqual(req.headers[header.toLowerCase()], - 'foo, bar', 'header parsed incorrectly: ' + header); + 'foo' + sep + 'bar', + 'header parsed incorrectly: ' + header); }); res.writeHead(200, {'Content-Type': 'text/plain'}); From d3480776c70bb1622afee5a2a5f39abf9e42a507 Mon Sep 17 00:00:00 2001 From: Brian White Date: Fri, 10 Feb 2017 20:32:32 -0500 Subject: [PATCH 2/2] http: concatenate outgoing Cookie headers This commit enables automatic concatenation of multiple Cookie header values with a semicolon, except when 2D header arrays are used. Fixes: https://github.com/nodejs/node/issues/11256 PR-URL: https://github.com/nodejs/node/pull/11259 Reviewed-By: James M Snell Reviewed-By: Roman Reiss Reviewed-By: Matteo Collina Reviewed-By: Ben Noordhuis --- lib/_http_outgoing.js | 41 +++++++++--- test/parallel/test-http.js | 129 +++++++++++++++++++++++-------------- 2 files changed, 112 insertions(+), 58 deletions(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 373476a3c43d52..186905ce5fc6a8 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -20,6 +20,27 @@ var RE_FIELDS = new RegExp('^(?:Connection|Transfer-Encoding|Content-Length|' + var RE_CONN_VALUES = /(?:^|\W)close|upgrade(?:$|\W)/ig; var RE_TE_CHUNKED = common.chunkExpression; +// isCookieField performs a case-insensitive comparison of a provided string +// against the word "cookie." This method (at least as of V8 5.4) is faster than +// the equivalent case-insensitive regexp, even if isCookieField does not get +// inlined. +function isCookieField(s) { + if (s.length !== 6) return false; + var ch = s.charCodeAt(0); + if (ch !== 99 && ch !== 67) return false; + ch = s.charCodeAt(1); + if (ch !== 111 && ch !== 79) return false; + ch = s.charCodeAt(2); + if (ch !== 111 && ch !== 79) return false; + ch = s.charCodeAt(3); + if (ch !== 107 && ch !== 75) return false; + ch = s.charCodeAt(4); + if (ch !== 105 && ch !== 73) return false; + ch = s.charCodeAt(5); + if (ch !== 101 && ch !== 69) return false; + return true; +} + var dateCache; function utcDate() { if (!dateCache) { @@ -275,12 +296,14 @@ function _storeHeader(firstLine, headers) { value = entry[1]; if (value instanceof Array) { - for (j = 0; j < value.length; j++) { - storeHeader(this, state, field, value[j], false); + if (value.length < 2 || !isCookieField(field)) { + for (j = 0; j < value.length; j++) + storeHeader(this, state, field, value[j], false); + continue; } - } else { - storeHeader(this, state, field, value, false); + value = value.join('; '); } + storeHeader(this, state, field, value, false); } } else if (headers instanceof Array) { for (i = 0; i < headers.length; i++) { @@ -302,12 +325,14 @@ function _storeHeader(firstLine, headers) { value = headers[field]; if (value instanceof Array) { - for (j = 0; j < value.length; j++) { - storeHeader(this, state, field, value[j], true); + if (value.length < 2 || !isCookieField(field)) { + for (j = 0; j < value.length; j++) + storeHeader(this, state, field, value[j], true); + continue; } - } else { - storeHeader(this, state, field, value, true); + value = value.join('; '); } + storeHeader(this, state, field, value, true); } } diff --git a/test/parallel/test-http.js b/test/parallel/test-http.js index 1178c745e1e12a..dc77be4fdde189 100644 --- a/test/parallel/test-http.js +++ b/test/parallel/test-http.js @@ -1,84 +1,113 @@ 'use strict'; -require('../common'); +const common = require('../common'); const assert = require('assert'); const http = require('http'); const url = require('url'); -let responses_sent = 0; -let responses_recvd = 0; -let body0 = ''; -let body1 = ''; +const expectedRequests = ['/hello', '/there', '/world']; -const server = http.Server(function(req, res) { - if (responses_sent === 0) { - assert.strictEqual('GET', req.method); - assert.strictEqual('/hello', url.parse(req.url).pathname); +const server = http.Server(common.mustCall(function(req, res) { + assert.strictEqual(expectedRequests.shift(), req.url); - console.dir(req.headers); - assert.strictEqual(true, 'accept' in req.headers); - assert.strictEqual('*/*', req.headers['accept']); - - assert.strictEqual(true, 'foo' in req.headers); - assert.strictEqual('bar', req.headers['foo']); + switch (req.url) { + case '/hello': + assert.strictEqual(req.method, 'GET'); + assert.strictEqual(req.headers['accept'], '*/*'); + assert.strictEqual(req.headers['foo'], 'bar'); + assert.strictEqual(req.headers.cookie, 'foo=bar; bar=baz; baz=quux'); + break; + case '/there': + assert.strictEqual(req.method, 'PUT'); + assert.strictEqual(req.headers.cookie, 'node=awesome; ta=da'); + break; + case '/world': + assert.strictEqual(req.method, 'POST'); + assert.deepStrictEqual(req.headers.cookie, 'abc=123; def=456; ghi=789'); + break; + default: + assert(false, `Unexpected request for ${req.url}`); } - if (responses_sent === 1) { - assert.strictEqual('POST', req.method); - assert.strictEqual('/world', url.parse(req.url).pathname); + if (expectedRequests.length === 0) this.close(); - } req.on('end', function() { res.writeHead(200, {'Content-Type': 'text/plain'}); res.write('The path was ' + url.parse(req.url).pathname); res.end(); - responses_sent += 1; }); req.resume(); - - //assert.strictEqual('127.0.0.1', res.connection.remoteAddress); -}); +}, 3)); server.listen(0); server.on('listening', function() { const agent = new http.Agent({ port: this.address().port, maxSockets: 1 }); - http.get({ + const req = http.get({ port: this.address().port, path: '/hello', - headers: {'Accept': '*/*', 'Foo': 'bar'}, + headers: { + Accept: '*/*', + Foo: 'bar', + Cookie: [ 'foo=bar', 'bar=baz', 'baz=quux' ] + }, agent: agent - }, function(res) { - assert.strictEqual(200, res.statusCode); - responses_recvd += 1; + }, common.mustCall(function(res) { + const cookieHeaders = req._header.match(/^Cookie: .+$/img); + assert.deepStrictEqual(cookieHeaders, + ['Cookie: foo=bar; bar=baz; baz=quux']); + assert.strictEqual(res.statusCode, 200); + let body = ''; res.setEncoding('utf8'); - res.on('data', function(chunk) { body0 += chunk; }); - console.error('Got /hello response'); - }); + res.on('data', function(chunk) { body += chunk; }); + res.on('end', common.mustCall(function() { + assert.strictEqual(body, 'The path was /hello'); + })); + })); - setTimeout(function() { + setTimeout(common.mustCall(function() { + const req = http.request({ + port: server.address().port, + method: 'PUT', + path: '/there', + agent: agent + }, common.mustCall(function(res) { + const cookieHeaders = req._header.match(/^Cookie: .+$/img); + assert.deepStrictEqual(cookieHeaders, ['Cookie: node=awesome; ta=da']); + assert.strictEqual(res.statusCode, 200); + let body = ''; + res.setEncoding('utf8'); + res.on('data', function(chunk) { body += chunk; }); + res.on('end', common.mustCall(function() { + assert.strictEqual(body, 'The path was /there'); + })); + })); + req.setHeader('Cookie', ['node=awesome', 'ta=da']); + req.end(); + }), 1); + + setTimeout(common.mustCall(function() { const req = http.request({ port: server.address().port, method: 'POST', path: '/world', + headers: [ ['Cookie', 'abc=123'], + ['Cookie', 'def=456'], + ['Cookie', 'ghi=789'] ], agent: agent - }, function(res) { - assert.strictEqual(200, res.statusCode); - responses_recvd += 1; + }, common.mustCall(function(res) { + const cookieHeaders = req._header.match(/^Cookie: .+$/img); + assert.deepStrictEqual(cookieHeaders, + ['Cookie: abc=123', + 'Cookie: def=456', + 'Cookie: ghi=789']); + assert.strictEqual(res.statusCode, 200); + let body = ''; res.setEncoding('utf8'); - res.on('data', function(chunk) { body1 += chunk; }); - console.error('Got /world response'); - }); + res.on('data', function(chunk) { body += chunk; }); + res.on('end', common.mustCall(function() { + assert.strictEqual(body, 'The path was /world'); + })); + })); req.end(); - }, 1); -}); - -process.on('exit', function() { - console.error('responses_recvd: ' + responses_recvd); - assert.strictEqual(2, responses_recvd); - - console.error('responses_sent: ' + responses_sent); - assert.strictEqual(2, responses_sent); - - assert.strictEqual('The path was /hello', body0); - assert.strictEqual('The path was /world', body1); + }), 2); });