From 4b70d2d6f4619ea3ca12aab65e77aa6d9226dabe Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Wed, 20 Mar 2019 12:18:16 +0100
Subject: [PATCH 1/2] http2: remove side effects from validateSettings

The function did not only validate the input so far but it also made
a copy of the input object and returned that copy to the callee
function. That copy was not necessary for all call sites and it was
not obvious that the function did not only validate the input but
that it also returned a copy of it. This makes sure the function does
nothing more than validation and copying is happening in the callee
function when required.
---
 lib/internal/http2/core.js | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index 4893cdc96b0b9b..0e7d8c544cc0db 100644
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -789,7 +789,7 @@ function pingCallback(cb) {
 // 6. enablePush must be a boolean
 // All settings are optional and may be left undefined
 function validateSettings(settings) {
-  settings = { ...settings };
+  if (settings === undefined) return;
   assertWithinRange('headerTableSize',
                     settings.headerTableSize,
                     0, kMaxInt);
@@ -813,7 +813,6 @@ function validateSettings(settings) {
     Error.captureStackTrace(err, 'validateSettings');
     throw err;
   }
-  return settings;
 }
 
 // Creates the internal binding.Http2Session handle for an Http2Session
@@ -1152,7 +1151,7 @@ class Http2Session extends EventEmitter {
     if (this.destroyed)
       throw new ERR_HTTP2_INVALID_SESSION();
     assertIsObject(settings, 'settings');
-    settings = validateSettings(settings);
+    validateSettings(settings);
 
     if (callback && typeof callback !== 'function')
       throw new ERR_INVALID_CALLBACK();
@@ -1160,7 +1159,7 @@ class Http2Session extends EventEmitter {
 
     this[kState].pendingAck++;
 
-    const settingsFn = submitSettings.bind(this, settings, callback);
+    const settingsFn = submitSettings.bind(this, { ...settings }, callback);
     if (this.connecting) {
       this.once('connect', settingsFn);
       return;
@@ -2830,7 +2829,8 @@ function createServer(options, handler) {
 // HTTP2-Settings header frame.
 function getPackedSettings(settings) {
   assertIsObject(settings, 'settings');
-  updateSettingsBuffer(validateSettings(settings));
+  validateSettings(settings);
+  updateSettingsBuffer({ ...settings });
   return binding.packSettings();
 }
 

From 5c1b27fb574baa0d6e57e563e24045a4d7cea77e Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Wed, 20 Mar 2019 13:45:53 +0100
Subject: [PATCH 2/2] http2: validate settings input

The settings input was not marked as optional in our documentation
but we did not throw an error in those cases. The API does not seem
to be useful without the correct input, so I went for throwing an
error in such cases instead of updating the documentation.
---
 lib/internal/http2/core.js                    | 6 +++++-
 test/parallel/test-http2-getpackedsettings.js | 6 ++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index 0e7d8c544cc0db..117db86598ad0d 100644
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -789,7 +789,11 @@ function pingCallback(cb) {
 // 6. enablePush must be a boolean
 // All settings are optional and may be left undefined
 function validateSettings(settings) {
-  if (settings === undefined) return;
+  if (settings === undefined) {
+    const err = new ERR_INVALID_ARG_TYPE('settings', 'object', settings);
+    Error.captureStackTrace(err, 'validateSettings');
+    throw err;
+  }
   assertWithinRange('headerTableSize',
                     settings.headerTableSize,
                     0, kMaxInt);
diff --git a/test/parallel/test-http2-getpackedsettings.js b/test/parallel/test-http2-getpackedsettings.js
index 77c8cf442f5f27..0df2457f76c67c 100644
--- a/test/parallel/test-http2-getpackedsettings.js
+++ b/test/parallel/test-http2-getpackedsettings.js
@@ -88,8 +88,10 @@ http2.getPackedSettings({ enablePush: false });
 
 // Check for not passing settings.
 {
-  const packed = http2.getPackedSettings();
-  assert.strictEqual(packed.length, 0);
+  assert.throws(
+    () => http2.getPackedSettings(),
+    { code: 'ERR_INVALID_ARG_TYPE' }
+  );
 }
 
 {