From 8f081867d238cae80cda418d0f69a26f9433a51c Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Sun, 26 Apr 2020 09:39:18 +0530 Subject: [PATCH 1/9] http2: add type checks for Http2ServerResponse.end Refs: https://github.com/nodejs/node/issues/29829 fixup fixup: add Uint8Array fixup: add Uint8Array to checks fixup: add Uint8Array to checks --- lib/internal/http2/compat.js | 19 +++++++++++++++++-- test/parallel/test-http2-res-end-types.js | 20 ++++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-http2-res-end-types.js diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index 851fec4b3a6ae5..477dd3774e1386 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -13,6 +13,7 @@ const { const assert = require('internal/assert'); const Stream = require('stream'); +const { Buffer } = require('buffer'); const { Readable } = Stream; const { constants: { @@ -39,6 +40,7 @@ const { ERR_HTTP2_PSEUDOHEADER_NOT_ALLOWED, ERR_HTTP2_STATUS_INVALID, ERR_INVALID_ARG_VALUE, + ERR_INVALID_ARG_TYPE, ERR_INVALID_CALLBACK, ERR_INVALID_HTTP_TOKEN, ERR_STREAM_WRITE_AFTER_END @@ -698,8 +700,21 @@ class Http2ServerResponse extends Stream { encoding = 'utf8'; } - if (chunk !== null && chunk !== undefined) - this.write(chunk, encoding); + if (chunk !== null && chunk !== undefined) { + if (typeof chunk === 'string' || + (chunk instanceof Buffer) || + (chunk instanceof Uint8Array)) { + this.write(chunk, encoding); + } else { + // Will not be forwarded to Http2ServerResponse object + // since this behavior is not allowed in compat mode + const err = new ERR_INVALID_ARG_TYPE('chunk', + ['string', 'Buffer'], + chunk); + // Throws an ERR_HTTP2_STREAM_ERROR error + this.destroy(err); + } + } state.headRequest = stream.headRequest; state.ending = true; diff --git a/test/parallel/test-http2-res-end-types.js b/test/parallel/test-http2-res-end-types.js new file mode 100644 index 00000000000000..ce0387eb2f9e1f --- /dev/null +++ b/test/parallel/test-http2-res-end-types.js @@ -0,0 +1,20 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) { common.skip('missing crypto'); } +const assert = require('assert'); +const http2 = require('http2'); + +const server = http2.createServer(common.mustCall(function(req, res) { + res.end([`This will throw and destroy on session.request, + but will not forward error on res.`]); +})); + +server.listen(0, common.mustCall(function() { + const session = http2.connect(`http://localhost:${server.address().port}`); + const req = session.request(); + req.on('error', common.mustCall((e) => { + assert.strictEqual(e.code, 'ERR_HTTP2_STREAM_ERROR'); + session.close(); + server.close(); + })); +})); From dd62977d79caf4214b0bb209fa80eabd73a192b9 Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Thu, 30 Apr 2020 08:57:58 +0530 Subject: [PATCH 2/9] fixup --- lib/internal/http2/compat.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index 477dd3774e1386..b20670e7c610fd 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -49,6 +49,7 @@ const { } = require('internal/errors'); const { validateString } = require('internal/validators'); const { kSocket, kRequest, kProxySocket } = require('internal/http2/util'); +const { isUint8Array } = require('internal/util/types'); const kBeginSend = Symbol('begin-send'); const kState = Symbol('state'); @@ -703,7 +704,7 @@ class Http2ServerResponse extends Stream { if (chunk !== null && chunk !== undefined) { if (typeof chunk === 'string' || (chunk instanceof Buffer) || - (chunk instanceof Uint8Array)) { + (isUint8Array(chunk))) { this.write(chunk, encoding); } else { // Will not be forwarded to Http2ServerResponse object From 4ef609aa2eff5d297338a17a715ce1a33aa37328 Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Thu, 30 Apr 2020 08:59:57 +0530 Subject: [PATCH 3/9] fixup: drop Buffer checks --- lib/internal/http2/compat.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index b20670e7c610fd..ed814e32b2623e 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -703,7 +703,6 @@ class Http2ServerResponse extends Stream { if (chunk !== null && chunk !== undefined) { if (typeof chunk === 'string' || - (chunk instanceof Buffer) || (isUint8Array(chunk))) { this.write(chunk, encoding); } else { From 850384aa8dcc64f6558012cf366c9837f565883b Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Thu, 30 Apr 2020 09:05:19 +0530 Subject: [PATCH 4/9] fixup: lint error --- lib/internal/http2/compat.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index ed814e32b2623e..d7cfcc04b577c4 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -13,7 +13,6 @@ const { const assert = require('internal/assert'); const Stream = require('stream'); -const { Buffer } = require('buffer'); const { Readable } = Stream; const { constants: { From f7a974e3a7e78cef26bf07452df92b1379da3e75 Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Thu, 30 Apr 2020 09:44:58 +0530 Subject: [PATCH 5/9] fixup: add Uint8Array to valid types --- lib/internal/http2/compat.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index d7cfcc04b577c4..b682c878cf14af 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -708,7 +708,7 @@ class Http2ServerResponse extends Stream { // Will not be forwarded to Http2ServerResponse object // since this behavior is not allowed in compat mode const err = new ERR_INVALID_ARG_TYPE('chunk', - ['string', 'Buffer'], + ['string', 'Buffer', 'Uint8Array'], chunk); // Throws an ERR_HTTP2_STREAM_ERROR error this.destroy(err); From 26efdc5a5073cfcb2ce831599a174f1c85dcb59f Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Sun, 3 May 2020 13:25:14 +0530 Subject: [PATCH 6/9] fixup: dont modify HTTP2 code and remove unnecessary test --- lib/internal/http2/compat.js | 18 ++---------------- test/parallel/test-http2-res-end-types.js | 20 -------------------- 2 files changed, 2 insertions(+), 36 deletions(-) delete mode 100644 test/parallel/test-http2-res-end-types.js diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index b682c878cf14af..851fec4b3a6ae5 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -39,7 +39,6 @@ const { ERR_HTTP2_PSEUDOHEADER_NOT_ALLOWED, ERR_HTTP2_STATUS_INVALID, ERR_INVALID_ARG_VALUE, - ERR_INVALID_ARG_TYPE, ERR_INVALID_CALLBACK, ERR_INVALID_HTTP_TOKEN, ERR_STREAM_WRITE_AFTER_END @@ -48,7 +47,6 @@ const { } = require('internal/errors'); const { validateString } = require('internal/validators'); const { kSocket, kRequest, kProxySocket } = require('internal/http2/util'); -const { isUint8Array } = require('internal/util/types'); const kBeginSend = Symbol('begin-send'); const kState = Symbol('state'); @@ -700,20 +698,8 @@ class Http2ServerResponse extends Stream { encoding = 'utf8'; } - if (chunk !== null && chunk !== undefined) { - if (typeof chunk === 'string' || - (isUint8Array(chunk))) { - this.write(chunk, encoding); - } else { - // Will not be forwarded to Http2ServerResponse object - // since this behavior is not allowed in compat mode - const err = new ERR_INVALID_ARG_TYPE('chunk', - ['string', 'Buffer', 'Uint8Array'], - chunk); - // Throws an ERR_HTTP2_STREAM_ERROR error - this.destroy(err); - } - } + if (chunk !== null && chunk !== undefined) + this.write(chunk, encoding); state.headRequest = stream.headRequest; state.ending = true; diff --git a/test/parallel/test-http2-res-end-types.js b/test/parallel/test-http2-res-end-types.js deleted file mode 100644 index ce0387eb2f9e1f..00000000000000 --- a/test/parallel/test-http2-res-end-types.js +++ /dev/null @@ -1,20 +0,0 @@ -'use strict'; -const common = require('../common'); -if (!common.hasCrypto) { common.skip('missing crypto'); } -const assert = require('assert'); -const http2 = require('http2'); - -const server = http2.createServer(common.mustCall(function(req, res) { - res.end([`This will throw and destroy on session.request, - but will not forward error on res.`]); -})); - -server.listen(0, common.mustCall(function() { - const session = http2.connect(`http://localhost:${server.address().port}`); - const req = session.request(); - req.on('error', common.mustCall((e) => { - assert.strictEqual(e.code, 'ERR_HTTP2_STREAM_ERROR'); - session.close(); - server.close(); - })); -})); From eb1dffc4f6d1aa9ca955cd3d81abacbf24fb005a Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Sun, 10 May 2020 14:42:17 +0530 Subject: [PATCH 7/9] add test --- test/parallel/test-http-outgoing-end-types.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 test/parallel/test-http-outgoing-end-types.js diff --git a/test/parallel/test-http-outgoing-end-types.js b/test/parallel/test-http-outgoing-end-types.js new file mode 100644 index 00000000000000..e9b1b0eea21f9d --- /dev/null +++ b/test/parallel/test-http-outgoing-end-types.js @@ -0,0 +1,18 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); + +const httpServer = http.createServer(common.mustCall(function(req, res) { + httpServer.close(); + assert.throws(() => { + res.end(['Throws.']); + }, { + code: 'ERR_INVALID_ARG_TYPE' + }); + res.end(); +})); + +httpServer.listen(0, common.mustCall(function() { + http.get({ port: this.address().port }); +})); \ No newline at end of file From b19e3515c47283b4d846280c1b94c2e17f5a84bd Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Mon, 11 May 2020 00:06:48 +0530 Subject: [PATCH 8/9] rebase --- lib/_http_outgoing.js | 1 - test/parallel/test-http-outgoing-end-types.js | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index bfe329425372c7..f0f73ae999dc80 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -63,7 +63,6 @@ const { hideStackFrames } = require('internal/errors'); const { validateString } = require('internal/validators'); -const { isUint8Array } = require('internal/util/types'); const HIGH_WATER_MARK = getDefaultHighWaterMark(); const { CRLF, debug } = common; diff --git a/test/parallel/test-http-outgoing-end-types.js b/test/parallel/test-http-outgoing-end-types.js index e9b1b0eea21f9d..20b443bff2c1f5 100644 --- a/test/parallel/test-http-outgoing-end-types.js +++ b/test/parallel/test-http-outgoing-end-types.js @@ -15,4 +15,4 @@ const httpServer = http.createServer(common.mustCall(function(req, res) { httpServer.listen(0, common.mustCall(function() { http.get({ port: this.address().port }); -})); \ No newline at end of file +})); From 4373ed1baf6af2a4fb7cb7aaae6c9e400a891467 Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Sun, 24 May 2020 05:22:31 +0530 Subject: [PATCH 9/9] fixup: remove changes from _http_outgoing.js --- lib/_http_outgoing.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index f0f73ae999dc80..bfe329425372c7 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -63,6 +63,7 @@ const { hideStackFrames } = require('internal/errors'); const { validateString } = require('internal/validators'); +const { isUint8Array } = require('internal/util/types'); const HIGH_WATER_MARK = getDefaultHighWaterMark(); const { CRLF, debug } = common;