From 984132d2bec75abd278718eb9fc584e53430ff00 Mon Sep 17 00:00:00 2001 From: Priyank Singh Date: Fri, 29 May 2020 15:42:31 +0530 Subject: [PATCH 1/7] http2: add maxHeaderSize option to http2 add maxHeaderSize to http2 as an alias for maxHeaderListSize. Fixes: https://github.com/nodejs/node/issues/33517 --- lib/internal/http2/core.js | 5 ++++- lib/internal/http2/util.js | 20 +++++++++++++++++--- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 8a00cb65e99bcf..40f54e96771a05 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -906,6 +906,9 @@ const validateSettings = hideStackFrames((settings) => { assertWithinRange('maxHeaderListSize', settings.maxHeaderListSize, 0, kMaxInt); + assertWithinRange('maxHeaderSize', + settings.maxHeaderSize, + 0, kMaxInt); if (settings.enablePush !== undefined && typeof settings.enablePush !== 'boolean') { throw new ERR_HTTP2_INVALID_SETTING_VALUE('enablePush', @@ -3112,7 +3115,7 @@ function getUnpackedSettings(buf, options = {}) { settings.maxFrameSize = value; break; case NGHTTP2_SETTINGS_MAX_HEADER_LIST_SIZE: - settings.maxHeaderListSize = value; + settings.maxHeaderListSize = settings.maxHeaderSize = value; break; case NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL: settings.enableConnectProtocol = value !== 0; diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js index dcc1355a3230fd..e3c4cf74d41f8a 100644 --- a/lib/internal/http2/util.js +++ b/lib/internal/http2/util.js @@ -294,9 +294,13 @@ function getDefaultSettings() { if ((flags & (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) === (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) { - holder.maxHeaderListSize = + holder.maxHeaderListSize = holder.maxHeaderSize = settingsBuffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE]; } + if ((flags & (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) === + (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) { + holder.maxHeaderSize = settingsBuffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE]; + } if ((flags & (1 << IDX_SETTINGS_ENABLE_CONNECT_PROTOCOL)) === (1 << IDX_SETTINGS_ENABLE_CONNECT_PROTOCOL)) { @@ -322,6 +326,7 @@ function getSettings(session, remote) { maxFrameSize: settingsBuffer[IDX_SETTINGS_MAX_FRAME_SIZE], maxConcurrentStreams: settingsBuffer[IDX_SETTINGS_MAX_CONCURRENT_STREAMS], maxHeaderListSize: settingsBuffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE], + maxHeaderSize: settingsBuffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE], enableConnectProtocol: !!settingsBuffer[IDX_SETTINGS_ENABLE_CONNECT_PROTOCOL] }; @@ -349,10 +354,19 @@ function updateSettingsBuffer(settings) { settingsBuffer[IDX_SETTINGS_MAX_FRAME_SIZE] = settings.maxFrameSize; } - if (typeof settings.maxHeaderListSize === 'number') { + if (typeof settings.maxHeaderListSize === 'number' || + typeof settings.maxHeaderSize === 'number') { flags |= (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE); + if (settings.maxHeaderSize && settings.maxHeaderListSize) { + if (settings.maxHeaderSize !== settings.maxHeaderListSize) { + throw new ERR_HTTP2_INVALID_SETTING_VALUE.RangeError( + 'maxHeaderListSize', settings.maxHeaderListSize + ); + } + } settingsBuffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE] = - settings.maxHeaderListSize; + settings.maxHeaderListSize !== undefined ? + settings.maxHeaderListSize : settings.maxHeaderSize; } if (typeof settings.enablePush === 'boolean') { flags |= (1 << IDX_SETTINGS_ENABLE_PUSH); From 3ba02dab885da4010db0d2269251df4905e63ee7 Mon Sep 17 00:00:00 2001 From: Priyank Singh Date: Fri, 29 May 2020 21:58:08 +0530 Subject: [PATCH 2/7] test: add tests for maxHeaderSize setting in http2 Refs: https://github.com/nodejs/node/pull/33636 --- test/parallel/test-http2-getpackedsettings.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-http2-getpackedsettings.js b/test/parallel/test-http2-getpackedsettings.js index a54ab4499e1f89..f33c0e916a5d13 100644 --- a/test/parallel/test-http2-getpackedsettings.js +++ b/test/parallel/test-http2-getpackedsettings.js @@ -26,7 +26,9 @@ assert.deepStrictEqual(val, check); ['maxConcurrentStreams', 0], ['maxConcurrentStreams', 2 ** 31 - 1], ['maxHeaderListSize', 0], - ['maxHeaderListSize', 2 ** 32 - 1] + ['maxHeaderListSize', 2 ** 32 - 1], + ['maxHeaderSize', 0], + ['maxHeaderSize', 2 ** 32 - 1] ].forEach((i) => { // Valid options should not throw. http2.getPackedSettings({ [i[0]]: i[1] }); @@ -45,7 +47,9 @@ http2.getPackedSettings({ enablePush: false }); ['maxConcurrentStreams', -1], ['maxConcurrentStreams', 2 ** 32], ['maxHeaderListSize', -1], - ['maxHeaderListSize', 2 ** 32] + ['maxHeaderListSize', 2 ** 32], + ['maxHeaderSize', -1], + ['maxHeaderSize', 2 ** 32] ].forEach((i) => { assert.throws(() => { http2.getPackedSettings({ [i[0]]: i[1] }); @@ -97,6 +101,7 @@ http2.getPackedSettings({ enablePush: false }); maxFrameSize: 20000, maxConcurrentStreams: 200, maxHeaderListSize: 100, + maxHeaderSize: 100, enablePush: true, enableConnectProtocol: false, foo: 'ignored' @@ -149,6 +154,7 @@ http2.getPackedSettings({ enablePush: false }); assert.strictEqual(settings.maxFrameSize, 20000); assert.strictEqual(settings.maxConcurrentStreams, 200); assert.strictEqual(settings.maxHeaderListSize, 100); + assert.strictEqual(settings.maxHeaderSize, 100); assert.strictEqual(settings.enablePush, true); assert.strictEqual(settings.enableConnectProtocol, false); } From 205f38ab750f9e76afe9fe636aad90ecbe3ef0d5 Mon Sep 17 00:00:00 2001 From: Priyank Singh Date: Sat, 30 May 2020 03:00:39 +0530 Subject: [PATCH 3/7] http2: added more tests for maxHeaderSize Refs: https://github.com/nodejs/node/pull/33636 --- lib/internal/http2/util.js | 15 +++++---------- .../test-http2-client-settings-before-connect.js | 2 ++ test/parallel/test-http2-session-settings.js | 5 ++++- test/parallel/test-http2-too-large-headers.js | 5 ++++- 4 files changed, 15 insertions(+), 12 deletions(-) diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js index e3c4cf74d41f8a..c2a8f93920e688 100644 --- a/lib/internal/http2/util.js +++ b/lib/internal/http2/util.js @@ -297,10 +297,6 @@ function getDefaultSettings() { holder.maxHeaderListSize = holder.maxHeaderSize = settingsBuffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE]; } - if ((flags & (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) === - (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) { - holder.maxHeaderSize = settingsBuffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE]; - } if ((flags & (1 << IDX_SETTINGS_ENABLE_CONNECT_PROTOCOL)) === (1 << IDX_SETTINGS_ENABLE_CONNECT_PROTOCOL)) { @@ -357,12 +353,11 @@ function updateSettingsBuffer(settings) { if (typeof settings.maxHeaderListSize === 'number' || typeof settings.maxHeaderSize === 'number') { flags |= (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE); - if (settings.maxHeaderSize && settings.maxHeaderListSize) { - if (settings.maxHeaderSize !== settings.maxHeaderListSize) { - throw new ERR_HTTP2_INVALID_SETTING_VALUE.RangeError( - 'maxHeaderListSize', settings.maxHeaderListSize - ); - } + if(settings.maxHeaderListSize !== undefined && + (settings.maxHeaderListSize !== maxHeaderSize)) { + throw new ERR_HTTP2_INVALID_SETTING_VALUE.RangeError( + 'maxHeaderListSize', settings.maxHeaderListSize + ); } settingsBuffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE] = settings.maxHeaderListSize !== undefined ? diff --git a/test/parallel/test-http2-client-settings-before-connect.js b/test/parallel/test-http2-client-settings-before-connect.js index ed2b859ae202d1..a02a26446995ac 100644 --- a/test/parallel/test-http2-client-settings-before-connect.js +++ b/test/parallel/test-http2-client-settings-before-connect.js @@ -32,6 +32,8 @@ server.listen(0, common.mustCall(() => { ['maxConcurrentStreams', 2 ** 32, RangeError], ['maxHeaderListSize', -1, RangeError], ['maxHeaderListSize', 2 ** 32, RangeError], + ['maxHeaderSize', -1, RangeError], + ['maxHeaderSize', 2 ** 32, RangeError], ['enablePush', 'a', TypeError], ['enablePush', 1, TypeError], ['enablePush', 0, TypeError], diff --git a/test/parallel/test-http2-session-settings.js b/test/parallel/test-http2-session-settings.js index 6ac6a1d4aac1b7..a31682f291479f 100644 --- a/test/parallel/test-http2-session-settings.js +++ b/test/parallel/test-http2-session-settings.js @@ -19,6 +19,7 @@ server.on( assert.strictEqual(typeof settings.maxFrameSize, 'number'); assert.strictEqual(typeof settings.maxConcurrentStreams, 'number'); assert.strictEqual(typeof settings.maxHeaderListSize, 'number'); + assert.strictEqual(typeof settings.maxHeaderSize, 'number'); }; const localSettings = stream.session.localSettings; @@ -100,7 +101,9 @@ server.listen( ['maxFrameSize', 16383], ['maxFrameSize', 2 ** 24], ['maxHeaderListSize', -1], - ['maxHeaderListSize', 2 ** 32] + ['maxHeaderListSize', 2 ** 32], + ['maxHeaderSize', -1], + ['maxHeaderSize', 2 ** 32] ].forEach((i) => { const settings = {}; settings[i[0]] = i[1]; diff --git a/test/parallel/test-http2-too-large-headers.js b/test/parallel/test-http2-too-large-headers.js index aad113deab440f..d646d9f3f74f65 100644 --- a/test/parallel/test-http2-too-large-headers.js +++ b/test/parallel/test-http2-too-large-headers.js @@ -9,7 +9,10 @@ const { NGHTTP2_ENHANCE_YOUR_CALM } = http2.constants; -const server = http2.createServer({ settings: { maxHeaderListSize: 100 } }); +let server = http2.createServer({ settings: { maxHeaderListSize: 100 } }); +server.on('stream', common.mustNotCall()); + +server = http2.createServer({ settings: {maxHeaderSize: 100 } }); server.on('stream', common.mustNotCall()); server.listen(0, common.mustCall(() => { From 2f3782ca7530893166893b764573b8821dc09bd8 Mon Sep 17 00:00:00 2001 From: Priyank Singh Date: Sat, 30 May 2020 03:38:16 +0530 Subject: [PATCH 4/7] http2: fixed linting issues Refs: https://github.com/nodejs/node/pull/33636 --- lib/internal/http2/util.js | 4 ++-- test/parallel/test-http2-too-large-headers.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js index c2a8f93920e688..8b9bb85d4309b3 100644 --- a/lib/internal/http2/util.js +++ b/lib/internal/http2/util.js @@ -353,8 +353,8 @@ function updateSettingsBuffer(settings) { if (typeof settings.maxHeaderListSize === 'number' || typeof settings.maxHeaderSize === 'number') { flags |= (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE); - if(settings.maxHeaderListSize !== undefined && - (settings.maxHeaderListSize !== maxHeaderSize)) { + if (settings.maxHeaderListSize !== undefined && + (settings.maxHeaderListSize !== settings.maxHeaderSize)) { throw new ERR_HTTP2_INVALID_SETTING_VALUE.RangeError( 'maxHeaderListSize', settings.maxHeaderListSize ); diff --git a/test/parallel/test-http2-too-large-headers.js b/test/parallel/test-http2-too-large-headers.js index d646d9f3f74f65..2f20e822a8cb67 100644 --- a/test/parallel/test-http2-too-large-headers.js +++ b/test/parallel/test-http2-too-large-headers.js @@ -12,7 +12,7 @@ const { let server = http2.createServer({ settings: { maxHeaderListSize: 100 } }); server.on('stream', common.mustNotCall()); -server = http2.createServer({ settings: {maxHeaderSize: 100 } }); +server = http2.createServer({ settings: { maxHeaderSize: 100 } }); server.on('stream', common.mustNotCall()); server.listen(0, common.mustCall(() => { From 5857ca5fa339bf6f0be59f4693abe9e0d5daf7c2 Mon Sep 17 00:00:00 2001 From: Priyank Singh Date: Sat, 30 May 2020 20:33:52 +0530 Subject: [PATCH 5/7] http2: fixed failing tests Refs: https://github.com/nodejs/node/pull/33636 --- lib/internal/http2/util.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js index 8b9bb85d4309b3..e008fe27eeee09 100644 --- a/lib/internal/http2/util.js +++ b/lib/internal/http2/util.js @@ -354,6 +354,7 @@ function updateSettingsBuffer(settings) { typeof settings.maxHeaderSize === 'number') { flags |= (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE); if (settings.maxHeaderListSize !== undefined && + settings.maxHeaderSize !== undefined && (settings.maxHeaderListSize !== settings.maxHeaderSize)) { throw new ERR_HTTP2_INVALID_SETTING_VALUE.RangeError( 'maxHeaderListSize', settings.maxHeaderListSize From 8f6c4aaf909486329a1763b907b902fd7b2ce77b Mon Sep 17 00:00:00 2001 From: Priyank Singh Date: Sun, 31 May 2020 23:03:31 +0530 Subject: [PATCH 6/7] http2: added doc for maxHeaderSize Refs: https://github.com/nodejs/node/pull/33636 --- doc/api/http2.md | 1 + test/parallel/test-http2-too-large-headers.js | 41 +++++++++---------- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/doc/api/http2.md b/doc/api/http2.md index 689a1be97df258..4d1397f580d356 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -2495,6 +2495,7 @@ properties. * `maxHeaderListSize` {number} Specifies the maximum size (uncompressed octets) of header list that will be accepted. The minimum allowed value is 0. The maximum allowed value is 232-1. **Default:** `65535`. +* `maxHeaderSize` {number} Alias for `maxHeaderListSize`. * `enableConnectProtocol`{boolean} Specifies `true` if the "Extended Connect Protocol" defined by [RFC 8441][] is to be enabled. This setting is only meaningful if sent by the server. Once the `enableConnectProtocol` setting diff --git a/test/parallel/test-http2-too-large-headers.js b/test/parallel/test-http2-too-large-headers.js index 2f20e822a8cb67..b123d84e72e8fe 100644 --- a/test/parallel/test-http2-too-large-headers.js +++ b/test/parallel/test-http2-too-large-headers.js @@ -9,27 +9,26 @@ const { NGHTTP2_ENHANCE_YOUR_CALM } = http2.constants; -let server = http2.createServer({ settings: { maxHeaderListSize: 100 } }); -server.on('stream', common.mustNotCall()); +for (const prototype of ['maxHeaderListSize', 'maxHeaderSize']) { + const server = http2.createServer({ settings: { [prototype]: 100 } }); + server.on('stream', common.mustNotCall()); -server = http2.createServer({ settings: { maxHeaderSize: 100 } }); -server.on('stream', common.mustNotCall()); + server.listen(0, common.mustCall(() => { + const client = http2.connect(`http://localhost:${server.address().port}`); -server.listen(0, common.mustCall(() => { - const client = http2.connect(`http://localhost:${server.address().port}`); + client.on('remoteSettings', () => { + const req = client.request({ 'foo': 'a'.repeat(1000) }); + req.on('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_ENHANCE_YOUR_CALM' + })); + req.on('close', common.mustCall(() => { + assert.strictEqual(req.rstCode, NGHTTP2_ENHANCE_YOUR_CALM); + server.close(); + client.close(); + })); + }); - client.on('remoteSettings', () => { - const req = client.request({ 'foo': 'a'.repeat(1000) }); - req.on('error', common.expectsError({ - code: 'ERR_HTTP2_STREAM_ERROR', - name: 'Error', - message: 'Stream closed with error code NGHTTP2_ENHANCE_YOUR_CALM' - })); - req.on('close', common.mustCall(() => { - assert.strictEqual(req.rstCode, NGHTTP2_ENHANCE_YOUR_CALM); - server.close(); - client.close(); - })); - }); - -})); + })); +} From 2a506c8a3247b2c71baf34cf8002bf00debc4c77 Mon Sep 17 00:00:00 2001 From: Priyank Singh Date: Tue, 2 Jun 2020 16:53:17 +0530 Subject: [PATCH 7/7] http2: maxHeaderSize overwrites maxHeaderListSize Refs: https://github.com/nodejs/node/pull/33636 --- lib/internal/http2/util.js | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js index e008fe27eeee09..79e8fd4ba530c1 100644 --- a/lib/internal/http2/util.js +++ b/lib/internal/http2/util.js @@ -353,16 +353,17 @@ function updateSettingsBuffer(settings) { if (typeof settings.maxHeaderListSize === 'number' || typeof settings.maxHeaderSize === 'number') { flags |= (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE); - if (settings.maxHeaderListSize !== undefined && - settings.maxHeaderSize !== undefined && - (settings.maxHeaderListSize !== settings.maxHeaderSize)) { - throw new ERR_HTTP2_INVALID_SETTING_VALUE.RangeError( - 'maxHeaderListSize', settings.maxHeaderListSize + if (settings.maxHeaderSize !== undefined && + (settings.maxHeaderSize !== settings.maxHeaderListSize)) { + process.emitWarning( + 'settings.maxHeaderSize overwrite settings.maxHeaderListSize' ); + settingsBuffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE] = + settings.maxHeaderSize; + } else { + settingsBuffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE] = + settings.maxHeaderListSize; } - settingsBuffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE] = - settings.maxHeaderListSize !== undefined ? - settings.maxHeaderListSize : settings.maxHeaderSize; } if (typeof settings.enablePush === 'boolean') { flags |= (1 << IDX_SETTINGS_ENABLE_PUSH);