errors: alter and test ERR_TLS_REQUIRED_SERVER_NAME #19988

davidmarkclements · 2018-04-12T20:59:12Z

changes the base instance for ERR_TLS_REQUIRED_SERVER_NAME
from Error to TypeError as a more accurate representation
of the error and adds a unit test for missing servername
input that triggers this error in Server.prototype.addContext

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

BridgeAR · 2018-04-13T19:15:58Z

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

lpinca · 2018-04-14T05:52:36Z

test/parallel/test-tls-sni-server-client.js

+// check for throwing case where servername is not supplied
+common.expectsError(tls.createServer(serverOptions, () => {}).addContext, {
+  type: TypeError,
+  code: 'ERR_TLS_REQUIRED_SERVER_NAME',


Why not ERR_MISSING_ARGS?

lpinca · 2018-04-14T05:53:34Z

test/parallel/test-tls-sni-server-client.js

+common.expectsError(tls.createServer(serverOptions, () => {}).addContext, {
+  type: TypeError,
+  code: 'ERR_TLS_REQUIRED_SERVER_NAME',
+  message: '"servername" is a required parameter'


The argument is called hostname in the docs, not sure what to use.

Normally the variable name in the source code. I see the point here about using the documentation name though...

If docs and source code disagree, one or the other should be changed so they are in agreement.

BridgeAR

It would be great to actually use ERR_MISSING_ARGS instead.

BridgeAR · 2018-05-15T15:24:26Z

Ping @davidmarkclements

Trott · 2018-11-14T05:39:54Z

Anyone interested in fixing this up and getting it landed? Or should this be closed?

changes the base instance for ERR_TLS_REQUIRED_SERVER_NAME from Error to TypeError as a more accurate representation of the error and adds a unit test for missing servername input that triggers this error in Server.prototype.addContext

Replace ERR_TLS_REQUIRED_SERVER_NAME with ERR_MISSING_ARGS. Update tls.md documentation so that argument name in documentation reflects the argument name in code.

Trott · 2018-11-14T06:02:39Z

I think I've made the necessary changes such that this can land. @BridgeAR PTAL

Trott · 2018-11-14T06:03:26Z

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

BridgeAR · 2018-12-13T15:45:14Z

lib/internal/errors.js

 E('ERR_TLS_REQUIRED_SERVER_NAME',
-  '"servername" is required parameter for Server.addContext', Error);
+  '"servername" is a required parameter', TypeError);


This is now unused and should be removed.

BridgeAR · 2018-12-13T15:45:53Z

@Trott with the code removed it's LGTM.

sam-github · 2018-12-13T16:27:53Z

doc/api/tls.md

 <!-- YAML
 added: v0.5.3
 -->

-* `hostname` {string} A SNI hostname or wildcard (e.g. `'*'`)
+* `servername` {string} A SNI hostname or wildcard (e.g. `'*'`)


"hostname" --> "server name", consistent with https://github.com/nodejs/node/blob/master/doc/api/tls.md#event-secureconnection

a string containing the server name requested via SNI.

sam-github · 2018-12-13T16:28:52Z

doc/api/tls.md

 * `context` {Object} An object containing any of the possible properties
  from the [`tls.createSecureContext()`][] `options` arguments (e.g. `key`,
  `cert`, `ca`, etc).

 The `server.addContext()` method adds a secure context that will be used if
-the client request's SNI name matches the supplied `hostname` (or wildcard).
+the client request's SNI name matches the supplied `severname` (or wildcard).


"severname" -> "servername" (typo)

sam-github · 2018-12-13T16:33:16Z

test/parallel/test-tls-sni-server-client.js

@@ -84,6 +84,13 @@ const clientsOptions = [{
 const serverResults = [];
 const clientResults = [];

+// check for throwing case where servername is not supplied


Puncutate the comment, please (leading capital, trailing period).

sam-github · 2018-12-13T16:35:03Z

doc/api/errors.md

@@ -2030,6 +2024,15 @@ removed: v10.0.0

 Used when a TLS renegotiation request has failed in a non-specific way.

+<a id="ERR_TLS_REQUIRED_SERVER_NAME"></a>


Can be removed now, as its no longer used.

sam-github · 2018-12-13T16:39:45Z

lib/_tls_wrap.js

@@ -1051,7 +1051,7 @@ Server.prototype.setOptions = util.deprecate(function(options) {
 // SNI Contexts High-Level API
 Server.prototype.addContext = function(servername, context) {
  if (!servername) {
-    throw new ERR_TLS_REQUIRED_SERVER_NAME();
+    throw new ERR_MISSING_ARGS('servername');


I think the convention here would be:

if (typeof servername !== 'string') throw new ERR_INVALID_ARG_TYPE('servername', 'string', arg);

to test both the presence of the arg, but also its type. If it's not a string it will throw anyway now just below on the call to .replace(), but throwing a more descriptive error makes it look less like a node.js bug.

BridgeAR · 2019-03-10T00:16:38Z

Closing due to long inactivity. @davidmarkclements please open a new PR in case you would like to continue working on this.

nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Apr 12, 2018

jasnell approved these changes Apr 12, 2018

View reviewed changes

BridgeAR approved these changes Apr 13, 2018

View reviewed changes

BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 13, 2018

BridgeAR requested a review from a team April 13, 2018 19:15

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 13, 2018

addaleax approved these changes Apr 13, 2018

View reviewed changes

lpinca reviewed Apr 14, 2018

View reviewed changes

BridgeAR requested changes Apr 15, 2018

View reviewed changes

BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 15, 2018

jasnell added the stalled Issues and PRs that are stalled. label Sep 10, 2018

davidmarkclements and others added 2 commits November 13, 2018 21:42

test: remove ERR_TLS_REQUIRED_SERVER_NAME

4f4d2fa

Replace ERR_TLS_REQUIRED_SERVER_NAME with ERR_MISSING_ARGS. Update tls.md documentation so that argument name in documentation reflects the argument name in code.

BridgeAR reviewed Dec 13, 2018

View reviewed changes

sam-github suggested changes Dec 13, 2018

View reviewed changes

BridgeAR closed this Mar 10, 2019

sam-github mentioned this pull request Jun 3, 2020

A possible TODO list for new (or current) contributors #33715

Closed

mkrawczuk mentioned this pull request Jun 18, 2020

tls: stronger validation for 'servername' in server.addContext #33943

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

errors: alter and test ERR_TLS_REQUIRED_SERVER_NAME #19988

errors: alter and test ERR_TLS_REQUIRED_SERVER_NAME #19988

davidmarkclements commented Apr 12, 2018

BridgeAR commented Apr 13, 2018

lpinca Apr 14, 2018

lpinca Apr 14, 2018

BridgeAR Apr 15, 2018 •

edited

Loading

Trott Nov 14, 2018

BridgeAR left a comment

BridgeAR commented May 15, 2018

Trott commented Nov 14, 2018

Trott commented Nov 14, 2018

Trott commented Nov 14, 2018

BridgeAR Dec 13, 2018

BridgeAR commented Dec 13, 2018

sam-github Dec 13, 2018

sam-github Dec 13, 2018

sam-github Dec 13, 2018

sam-github Dec 13, 2018

sam-github Dec 13, 2018

BridgeAR commented Mar 10, 2019

		@@ -2030,6 +2024,15 @@ removed: v10.0.0

		Used when a TLS renegotiation request has failed in a non-specific way.

		<a id="ERR_TLS_REQUIRED_SERVER_NAME"></a>

errors: alter and test ERR_TLS_REQUIRED_SERVER_NAME #19988

errors: alter and test ERR_TLS_REQUIRED_SERVER_NAME #19988

Conversation

davidmarkclements commented Apr 12, 2018

Checklist

BridgeAR commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR Apr 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR commented May 15, 2018

Trott commented Nov 14, 2018

Trott commented Nov 14, 2018

Trott commented Nov 14, 2018

Choose a reason for hiding this comment

BridgeAR commented Dec 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Mar 10, 2019

BridgeAR Apr 15, 2018 •

edited

Loading