diff --git a/doc/api/http.markdown b/doc/api/http.markdown index 257a676a5b..8dfde77524 100644 --- a/doc/api/http.markdown +++ b/doc/api/http.markdown @@ -649,8 +649,8 @@ response.addTrailers({'Content-MD5': '7895bf4b8828b55ceaf47747b4bca667'}); response.end(); ``` -Attempting to set a trailer field name that contains invalid characters will -result in a [`TypeError`][] being thrown. +Attempting to set a header field name or value that contains invalid characters +will result in a [`TypeError`][] being thrown. ### response.end([data][, encoding][, callback]) @@ -721,8 +721,8 @@ or response.setHeader('Set-Cookie', ['type=ninja', 'language=javascript']); ``` -Attempting to set a header field name that contains invalid characters will -result in a [`TypeError`][] being thrown. +Attempting to set a header field name or value that contains invalid characters +will result in a [`TypeError`][] being thrown. When headers have been set with [`response.setHeader()`][], they will be merged with any headers passed to [`response.writeHead()`][], with the headers passed to @@ -836,8 +836,8 @@ response.writeHead(200, { This method must only be called once on a message and it must be called before [`response.end()`][] is called. -If you call [`response.write()`][] or [`response.end()`][] before calling this, the -implicit/mutable headers will be calculated and call this function for you. +If you call [`response.write()`][] or [`response.end()`][] before calling this, +the implicit/mutable headers will be calculated and call this function for you. When headers have been set with [`response.setHeader()`][], they will be merged with any headers passed to [`response.writeHead()`][], with the headers passed to @@ -860,6 +860,9 @@ should be used to determine the number of bytes in a given encoding. And Node.js does not check whether Content-Length and the length of the body which has been transmitted are equal or not. +Attempting to set a header field name or value that contains invalid characters +will result in a [`TypeError`][] being thrown. + ## Class: http.IncomingMessage An `IncomingMessage` object is created by [`http.Server`][] or diff --git a/lib/_http_common.js b/lib/_http_common.js index 5f5af3325e..328b6eea8a 100644 --- a/lib/_http_common.js +++ b/lib/_http_common.js @@ -231,3 +231,20 @@ function checkIsHttpToken(val) { return typeof val === 'string' && token.test(val); } exports._checkIsHttpToken = checkIsHttpToken; + +/** + * True if val contains an invalid field-vchar + * field-value = *( field-content / obs-fold ) + * field-content = field-vchar [ 1*( SP / HTAB ) field-vchar ] + * field-vchar = VCHAR / obs-text + **/ +function checkInvalidHeaderChar(val) { + val = '' + val; + for (var i = 0; i < val.length; i++) { + const ch = val.charCodeAt(i); + if (ch === 9) continue; + if (ch <= 31 || ch > 255 || ch === 127) return true; + } + return false; +} +exports._checkInvalidHeaderChar = checkInvalidHeaderChar; diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 450975b72a..56228f5139 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -305,8 +305,10 @@ function storeHeader(self, state, field, value) { throw new TypeError( 'Header name must be a valid HTTP Token ["' + field + '"]'); } - value = escapeHeaderValue(value); - state.messageHeader += field + ': ' + value + CRLF; + if (common._checkInvalidHeaderChar(value) === true) { + throw new TypeError('The header content contains invalid characters'); + } + state.messageHeader += field + ': ' + escapeHeaderValue(value) + CRLF; if (connectionExpression.test(field)) { state.sentConnectionHeader = true; @@ -341,8 +343,10 @@ OutgoingMessage.prototype.setHeader = function(name, value) { if (value === undefined) throw new Error('"value" required in setHeader("' + name + '", value)'); if (this._header) - throw new Error('Can\'t set headers after they are sent'); - + throw new Error('Can\'t set headers after they are sent.'); + if (common._checkInvalidHeaderChar(value) === true) { + throw new TypeError('The header content contains invalid characters'); + } if (this._headers === null) this._headers = {}; @@ -515,6 +519,9 @@ OutgoingMessage.prototype.addTrailers = function(headers) { throw new TypeError( 'Trailer name must be a valid HTTP Token ["' + field + '"]'); } + if (common._checkInvalidHeaderChar(value) === true) { + throw new TypeError('The header content contains invalid characters'); + } this._trailer += field + ': ' + escapeHeaderValue(value) + CRLF; } }; diff --git a/test/parallel/test-http-header-response-splitting.js b/test/parallel/test-http-header-response-splitting.js deleted file mode 100644 index bb1da8ca7c..0000000000 --- a/test/parallel/test-http-header-response-splitting.js +++ /dev/null @@ -1,87 +0,0 @@ -'use strict'; -const common = require('../common'); -const assert = require('assert'); -const http = require('http'); - -var testIndex = 0; -const testCount = 2 * 4 * 6; -const responseBody = 'Hi mars!'; - -var server = http.createServer(function(req, res) { - function reply(header) { - switch (testIndex % 4) { - case 0: - res.writeHead(200, { a: header, b: header }); - break; - case 1: - res.setHeader('a', header); - res.setHeader('b', header); - res.writeHead(200); - break; - case 2: - res.setHeader('a', header); - res.writeHead(200, { b: header }); - break; - case 3: - res.setHeader('a', [header]); - res.writeHead(200, { b: header }); - break; - default: - assert.fail(null, null, 'unreachable'); - } - res.write(responseBody); - if (testIndex % 8 < 4) { - res.addTrailers({ ta: header, tb: header }); - } else { - res.addTrailers([['ta', header], ['tb', header]]); - } - res.end(); - } - switch ((testIndex / 8) | 0) { - case 0: - reply('foo \r\ninvalid: bar'); - break; - case 1: - reply('foo \ninvalid: bar'); - break; - case 2: - reply('foo \rinvalid: bar'); - break; - case 3: - reply('foo \n\n\ninvalid: bar'); - break; - case 4: - reply('foo \r\n \r\n \r\ninvalid: bar'); - break; - case 5: - reply('foo \r \n \r \n \r \ninvalid: bar'); - break; - default: - assert(false); - } - if (++testIndex === testCount) { - server.close(); - } -}); - -server.listen(common.PORT, common.mustCall(function() { - for (var i = 0; i < testCount; i++) { - http.get({ port: common.PORT, path: '/' }, common.mustCall(function(res) { - assert.strictEqual(res.headers.a, 'foo invalid: bar'); - assert.strictEqual(res.headers.b, 'foo invalid: bar'); - assert.strictEqual(res.headers.foo, undefined); - assert.strictEqual(res.headers.invalid, undefined); - var data = ''; - res.setEncoding('utf8'); - res.on('data', function(s) { data += s; }); - res.on('end', common.mustCall(function() { - assert.equal(data, responseBody); - assert.strictEqual(res.trailers.ta, 'foo invalid: bar'); - assert.strictEqual(res.trailers.tb, 'foo invalid: bar'); - assert.strictEqual(res.trailers.foo, undefined); - assert.strictEqual(res.trailers.invalid, undefined); - })); - res.resume(); - })); - } -})); diff --git a/test/parallel/test-http-response-splitting.js b/test/parallel/test-http-response-splitting.js new file mode 100644 index 0000000000..4c954bf90a --- /dev/null +++ b/test/parallel/test-http-response-splitting.js @@ -0,0 +1,55 @@ +'use strict'; + +const common = require('../common'); +const http = require('http'); +const net = require('net'); +const url = require('url'); +const assert = require('assert'); + +// Response splitting example, credit: Amit Klein, Safebreach +const str = '/welcome?lang=bar%c4%8d%c4%8aContent­Length:%200%c4%8d%c4%8a%c' + + '4%8d%c4%8aHTTP/1.1%20200%20OK%c4%8d%c4%8aContent­Length:%202' + + '0%c4%8d%c4%8aLast­Modified:%20Mon,%2027%20Oct%202003%2014:50:18' + + '%20GMT%c4%8d%c4%8aContent­Type:%20text/html%c4%8d%c4%8a%c4%8' + + 'd%c4%8a%3chtml%3eGotcha!%3c/html%3e'; + +// Response splitting example, credit: Сковорода Никита Андреевич (@ChALkeR) +const x = 'fooഊSet-Cookie: foo=barഊഊ'; +const y = 'foo⠊Set-Cookie: foo=bar'; + +var count = 0; + +const server = http.createServer((req, res) => { + switch (count++) { + case 0: + const loc = url.parse(req.url, true).query.lang; + assert.throws(common.mustCall(() => { + res.writeHead(302, {Location: `/foo?lang=${loc}`}); + })); + break; + case 1: + assert.throws(common.mustCall(() => { + res.writeHead(200, {'foo' : x}); + })); + break; + case 2: + assert.throws(common.mustCall(() => { + res.writeHead(200, {'foo' : y}); + })); + break; + default: + assert.fail(null, null, 'should not get to here.'); + } + if (count === 3) + server.close(); + res.end('ok'); +}); +server.listen(common.PORT, () => { + const end = 'HTTP/1.1\r\n\r\n'; + const client = net.connect({port: common.PORT}, () => { + client.write(`GET ${str} ${end}`); + client.write(`GET / ${end}`); + client.write(`GET / ${end}`); + client.end(); + }); +});