From 0343eceda4d6a84e728e54f30ee14f1f96e1b90d Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Thu, 7 Sep 2017 07:29:26 -0400 Subject: [PATCH] http2: fix refs to status 205, add tests Fix references within http2 core to HTTP_STATUS_CONTENT_RESET to point to the correct HTTP_STATUS_RESET_CONTENT. Add tests for status 204, 205 & 304 in respond, respondWithFD & respondWithFile. Add general error tests for respondWithFD & respondWithFile. PR-URL: https://github.com/nodejs/node/pull/15153 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Claudio Rodriguez Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Ruben Bridgewater --- lib/internal/http2/core.js | 8 +- .../test-http2-respond-file-errors.js | 103 +++++++++++++++ .../test-http2-respond-file-fd-errors.js | 123 ++++++++++++++++++ test/parallel/test-http2-respond-no-data.js | 40 ++++++ 4 files changed, 270 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-http2-respond-file-errors.js create mode 100644 test/parallel/test-http2-respond-file-fd-errors.js create mode 100644 test/parallel/test-http2-respond-no-data.js diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 8646f843a5bc88..12842e1f170a6e 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -121,7 +121,7 @@ const { HTTP2_METHOD_CONNECT, HTTP_STATUS_CONTINUE, - HTTP_STATUS_CONTENT_RESET, + HTTP_STATUS_RESET_CONTENT, HTTP_STATUS_OK, HTTP_STATUS_NO_CONTENT, HTTP_STATUS_NOT_MODIFIED, @@ -1879,7 +1879,7 @@ class ServerHttp2Stream extends Http2Stream { // the options.endStream option to true so that the underlying // bits do not attempt to send any. if (statusCode === HTTP_STATUS_NO_CONTENT || - statusCode === HTTP_STATUS_CONTENT_RESET || + statusCode === HTTP_STATUS_RESET_CONTENT || statusCode === HTTP_STATUS_NOT_MODIFIED || state.headRequest === true) { options.endStream = true; @@ -1973,7 +1973,7 @@ class ServerHttp2Stream extends Http2Stream { const statusCode = headers[HTTP2_HEADER_STATUS] |= 0; // Payload/DATA frames are not permitted in these cases if (statusCode === HTTP_STATUS_NO_CONTENT || - statusCode === HTTP_STATUS_CONTENT_RESET || + statusCode === HTTP_STATUS_RESET_CONTENT || statusCode === HTTP_STATUS_NOT_MODIFIED) { throw new errors.Error('ERR_HTTP2_PAYLOAD_FORBIDDEN', statusCode); } @@ -2050,7 +2050,7 @@ class ServerHttp2Stream extends Http2Stream { const statusCode = headers[HTTP2_HEADER_STATUS] |= 0; // Payload/DATA frames are not permitted in these cases if (statusCode === HTTP_STATUS_NO_CONTENT || - statusCode === HTTP_STATUS_CONTENT_RESET || + statusCode === HTTP_STATUS_RESET_CONTENT || statusCode === HTTP_STATUS_NOT_MODIFIED) { throw new errors.Error('ERR_HTTP2_PAYLOAD_FORBIDDEN', statusCode); } diff --git a/test/parallel/test-http2-respond-file-errors.js b/test/parallel/test-http2-respond-file-errors.js new file mode 100644 index 00000000000000..158c8cb4f074ed --- /dev/null +++ b/test/parallel/test-http2-respond-file-errors.js @@ -0,0 +1,103 @@ +// Flags: --expose-http2 +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const http2 = require('http2'); +const path = require('path'); + +const optionsWithTypeError = { + offset: 'number', + length: 'number', + statCheck: 'function', + getTrailers: 'function' +}; + +const types = { + boolean: true, + function: () => {}, + number: 1, + object: {}, + array: [], + null: null, + symbol: Symbol('test') +}; + +const fname = path.resolve(common.fixturesDir, 'elipses.txt'); + +const server = http2.createServer(); + +server.on('stream', common.mustCall((stream) => { + // Check for all possible TypeError triggers on options + Object.keys(optionsWithTypeError).forEach((option) => { + Object.keys(types).forEach((type) => { + if (type === optionsWithTypeError[option]) { + return; + } + + common.expectsError( + () => stream.respondWithFile(fname, { + [http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain' + }, { + [option]: types[type] + }), + { + type: TypeError, + code: 'ERR_INVALID_OPT_VALUE', + message: `The value "${String(types[type])}" is invalid ` + + `for option "${option}"` + } + ); + }); + }); + + // Should throw if :status 204, 205 or 304 + [204, 205, 304].forEach((status) => common.expectsError( + () => stream.respondWithFile(fname, { + [http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain', + ':status': status, + }), + { + code: 'ERR_HTTP2_PAYLOAD_FORBIDDEN', + message: `Responses with ${status} status must not have a payload` + } + )); + + // Should throw if headers already sent + stream.respond({ + ':status': 200, + }); + common.expectsError( + () => stream.respondWithFile(fname, { + [http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain' + }), + { + code: 'ERR_HTTP2_HEADERS_SENT', + message: 'Response has already been initiated.' + } + ); + + // Should throw if stream already destroyed + stream.destroy(); + common.expectsError( + () => stream.respondWithFile(fname, { + [http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain' + }), + { + code: 'ERR_HTTP2_INVALID_STREAM', + message: 'The stream has been destroyed' + } + ); +})); + +server.listen(0, common.mustCall(() => { + const client = http2.connect(`http://localhost:${server.address().port}`); + const req = client.request(); + + req.on('streamClosed', common.mustCall(() => { + client.destroy(); + server.close(); + })); + req.end(); +})); diff --git a/test/parallel/test-http2-respond-file-fd-errors.js b/test/parallel/test-http2-respond-file-fd-errors.js new file mode 100644 index 00000000000000..035699da50d199 --- /dev/null +++ b/test/parallel/test-http2-respond-file-fd-errors.js @@ -0,0 +1,123 @@ +// Flags: --expose-http2 +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const http2 = require('http2'); +const path = require('path'); +const fs = require('fs'); + +const optionsWithTypeError = { + offset: 'number', + length: 'number', + statCheck: 'function', + getTrailers: 'function' +}; + +const types = { + boolean: true, + function: () => {}, + number: 1, + object: {}, + array: [], + null: null, + symbol: Symbol('test') +}; + +const fname = path.resolve(common.fixturesDir, 'elipses.txt'); +const fd = fs.openSync(fname, 'r'); + +const server = http2.createServer(); + +server.on('stream', common.mustCall((stream) => { + // should throw if fd isn't a number + Object.keys(types).forEach((type) => { + if (type === 'number') { + return; + } + + common.expectsError( + () => stream.respondWithFD(types[type], { + [http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain' + }), + { + type: TypeError, + code: 'ERR_INVALID_ARG_TYPE', + message: 'The "fd" argument must be of type number' + } + ); + }); + + // Check for all possible TypeError triggers on options + Object.keys(optionsWithTypeError).forEach((option) => { + Object.keys(types).forEach((type) => { + if (type === optionsWithTypeError[option]) { + return; + } + + common.expectsError( + () => stream.respondWithFD(fd, { + [http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain' + }, { + [option]: types[type] + }), + { + type: TypeError, + code: 'ERR_INVALID_OPT_VALUE', + message: `The value "${String(types[type])}" is invalid ` + + `for option "${option}"` + } + ); + }); + }); + + // Should throw if :status 204, 205 or 304 + [204, 205, 304].forEach((status) => common.expectsError( + () => stream.respondWithFD(fd, { + [http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain', + ':status': status, + }), + { + code: 'ERR_HTTP2_PAYLOAD_FORBIDDEN', + message: `Responses with ${status} status must not have a payload` + } + )); + + // Should throw if headers already sent + stream.respond({ + ':status': 200, + }); + common.expectsError( + () => stream.respondWithFD(fd, { + [http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain' + }), + { + code: 'ERR_HTTP2_HEADERS_SENT', + message: 'Response has already been initiated.' + } + ); + + // Should throw if stream already destroyed + stream.destroy(); + common.expectsError( + () => stream.respondWithFD(fd, { + [http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain' + }), + { + code: 'ERR_HTTP2_INVALID_STREAM', + message: 'The stream has been destroyed' + } + ); +})); + +server.listen(0, common.mustCall(() => { + const client = http2.connect(`http://localhost:${server.address().port}`); + const req = client.request(); + + req.on('streamClosed', common.mustCall(() => { + client.destroy(); + server.close(); + })); + req.end(); +})); diff --git a/test/parallel/test-http2-respond-no-data.js b/test/parallel/test-http2-respond-no-data.js new file mode 100644 index 00000000000000..b2fbf44af193df --- /dev/null +++ b/test/parallel/test-http2-respond-no-data.js @@ -0,0 +1,40 @@ +// Flags: --expose-http2 +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const http2 = require('http2'); +const assert = require('assert'); + +const server = http2.createServer(); + +// Check that stream ends immediately after respond on :status 204, 205 & 304 + +const status = [204, 205, 304]; + +server.on('stream', common.mustCall((stream) => { + stream.on('streamClosed', common.mustCall(() => { + assert.strictEqual(stream.destroyed, true); + })); + stream.respond({ ':status': status.shift() }); +}, 3)); + +server.listen(0, common.mustCall(makeRequest)); + +function makeRequest() { + const client = http2.connect(`http://localhost:${server.address().port}`); + const req = client.request(); + req.resume(); + + req.on('end', common.mustCall(() => { + client.destroy(); + + if (!status.length) { + server.close(); + } else { + makeRequest(); + } + })); + req.end(); +}