Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls: add missing 'new' #27614

Closed
wants to merge 2 commits into from
Closed

tls: add missing 'new' #27614

wants to merge 2 commits into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented May 8, 2019

ERR_INVALID_OPT_VALUE cannot be constructed without new.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label May 8, 2019
@sam-github
Copy link
Contributor

Sorry about that :-(.

I think this should have a regression test.

@sam-github
Copy link
Contributor

I'm writing a test, I'll post it here.

@sam-github
Copy link
Contributor

@cjihrig how did you find this, by code inspection, or did you actually trigger the bug?

I suggest the following (I can PR, or you add to your commit, as you wish).

diff --git a/lib/_tls_common.js b/lib/_tls_common.js
index 2a4ad9b24e..82ab45f389 100644
--- a/lib/_tls_common.js
+++ b/lib/_tls_common.js
@@ -163,13 +163,14 @@ exports.createSecureContext = function createSecureContext(options) {                                                                                  
   // cipher suites all have a standard name format beginning with TLS_, so split
   // the ciphers and pass them to the appropriate API.
   const ciphers = (options.ciphers || tls.DEFAULT_CIPHERS).split(':');
-  const cipherList = ciphers.filter((_) => !_.match(/^TLS_/)).join(':');
+  const cipherList = ciphers.filter((_) => !_.match(/^TLS_/) &&
+                                    _.length > 0).join(':');
   const cipherSuites = ciphers.filter((_) => _.match(/^TLS_/)).join(':');

   if (cipherSuites === '' && cipherList === '') {
     // Specifying empty cipher suites for both TLS1.2 and TLS1.3 is invalid, its
     // not possible to handshake with no suites.
-    throw ERR_INVALID_OPT_VALUE('ciphers', ciphers);
+    throw new ERR_INVALID_OPT_VALUE('ciphers', ciphers);
   }

   c.context.setCipherSuites(cipherSuites);
diff --git a/test/parallel/test-tls-set-ciphers.js b/test/parallel/test-tls-set-ciphers.js                                                                                   
index 96eb0944f4..fbca83b3d5 100644
--- a/test/parallel/test-tls-set-ciphers.js
+++ b/test/parallel/test-tls-set-ciphers.js
@@ -91,3 +91,13 @@ test('TLS_AES_128_CCM_8_SHA256', U,

 test('TLS_AES_128_CCM_8_SHA256', 'TLS_AES_128_CCM_8_SHA256',
      'TLS_AES_128_CCM_8_SHA256');
+
+// Invalid cipher values
+test(9, 'AES256-SHA', U, 'ERR_INVALID_ARG_TYPE', U);
+test('AES256-SHA', 9, U, U, 'ERR_INVALID_ARG_TYPE');
+test(':', 'AES256-SHA', U, 'ERR_INVALID_OPT_VALUE', U);
+test('AES256-SHA', ':', U, U, 'ERR_INVALID_OPT_VALUE');
+
+// Using '' is synonymous for "use default ciphers"
+test('TLS_AES_256_GCM_SHA384', '', 'TLS_AES_256_GCM_SHA384');
+test('', 'TLS_AES_256_GCM_SHA384', 'TLS_AES_256_GCM_SHA384');

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change LGTM, but I have a (non-blocking) preference that we add a test here too.

@cjihrig
Copy link
Contributor Author

cjihrig commented May 9, 2019

how did you find this, by code inspection, or did you actually trigger the bug?

@sam-github I just stumbled on it in the code. I didn't trigger it. I'll pull in your other changes as a separate commit with you as the author.

cjihrig and others added 2 commits May 10, 2019 10:59
ERR_INVALID_OPT_VALUE cannot be constructed without new.
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 10, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/23030/

EDIT(cjihrig): CI was yellow.

@Trott
Copy link
Member

Trott commented May 10, 2019

Landed in 95c1cb4...ae749e7

@Trott Trott closed this May 10, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request May 10, 2019
ERR_INVALID_OPT_VALUE cannot be constructed without new.

PR-URL: nodejs#27614
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Trott pushed a commit to Trott/io.js that referenced this pull request May 10, 2019
PR-URL: nodejs#27614
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@cjihrig cjihrig deleted the missing-new branch May 10, 2019 18:31
targos pushed a commit that referenced this pull request May 11, 2019
ERR_INVALID_OPT_VALUE cannot be constructed without new.

PR-URL: #27614
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit that referenced this pull request May 11, 2019
PR-URL: #27614
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@BridgeAR BridgeAR mentioned this pull request May 21, 2019
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants