Skip to content

errors: alter and test ERR_TLS_REQUIRED_SERVER_NAME #19988

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.

Sign up for GitHub

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

Closed
davidmarkclements wants to merge 2 commits into from
Closed

errors: alter and test ERR_TLS_REQUIRED_SERVER_NAME #19988

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4ba80ce
errors: alter and test ERR_TLS_REQUIRED_SERVER_NAME
Apr 12, 2018
4f4d2fa
test: remove ERR_TLS_REQUIRED_SERVER_NAME
Trott Nov 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
test: remove ERR_TLS_REQUIRED_SERVER_NAME 
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.
  • Loading branch information
@Trott
Trott committed Nov 14, 2018
commit 4f4d2faea3f336aa4b93bf048359118557e893e3
15 changes: 9 additions & 6 deletions doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1665,12 +1665,6 @@ An attempt to renegotiate the TLS session failed.

An attempt was made to renegotiate TLS on a socket instance with TLS disabled.

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

While using TLS, the `server.addContext()` method was called without providing
a hostname in the first parameter.

<a id="ERR_TLS_SESSION_ATTACK"></a>
### ERR_TLS_SESSION_ATTACK

Expand Down Expand Up @@ -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>
Copy link
Copy Markdown
Contributor

@sam-github sam-github Dec 13, 2018

Choose a reason for hiding this comment

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

Can be removed now, as its no longer used.

### ERR_TLS_REQUIRED_SERVER_NAME
<!-- YAML
removed: REPLACEME
-->

While using TLS, the `server.addContext()` method was called without providing
a hostname in the first parameter.

<a id="ERR_UNKNOWN_BUILTIN_MODULE"></a>
### ERR_UNKNOWN_BUILTIN_MODULE
<!-- YAML
Expand Down
6 changes: 3 additions & 3 deletions doc/api/tls.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,18 +347,18 @@ called:
* `tlsSocket` {tls.TLSSocket} The `tls.TLSSocket` instance from which the
error originated.

### server.addContext(hostname, context)
### server.addContext(servername, context)
<!-- YAML
added: v0.5.3
-->

* `hostname` {string} A SNI hostname or wildcard (e.g. `'*'`)
* `servername` {string} A SNI hostname or wildcard (e.g. `'*'`)
Copy link
Copy Markdown
Contributor

@sam-github sam-github Dec 13, 2018

Choose a reason for hiding this comment

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

"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.

* `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).
Copy link
Copy Markdown
Contributor

@sam-github sam-github Dec 13, 2018

Choose a reason for hiding this comment

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

"severname" -> "servername" (typo)


### server.address()
<!-- YAML
Expand Down
4 changes: 2 additions & 2 deletions lib/_tls_wrap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,13 @@ const { owner_symbol } = require('internal/async_hooks').symbols;
const { SecureContext: NativeSecureContext } = internalBinding('crypto');
const {
ERR_INVALID_ARG_TYPE,
ERR_MISSING_ARGS,
ERR_MULTIPLE_CALLBACK,
ERR_SOCKET_CLOSED,
ERR_TLS_DH_PARAM_SIZE,
ERR_TLS_HANDSHAKE_TIMEOUT,
ERR_TLS_RENEGOTIATE,
ERR_TLS_RENEGOTIATION_DISABLED,
ERR_TLS_REQUIRED_SERVER_NAME,
ERR_TLS_SESSION_ATTACK,
ERR_TLS_SNI_FROM_SERVER
} = require('internal/errors').codes;
Expand Down Expand Up @@ -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');
Copy link
Copy Markdown
Contributor

@sam-github sam-github Dec 13, 2018

Choose a reason for hiding this comment

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

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.

}

var re = new RegExp('^' +
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-tls-sni-server-client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,8 @@ const clientResults = [];
// check for throwing case where servername is not supplied
Copy link
Copy Markdown
Contributor

@sam-github sam-github Dec 13, 2018

Choose a reason for hiding this comment

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

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

common.expectsError(tls.createServer(serverOptions, () => {}).addContext, {
type: TypeError,
code: 'ERR_TLS_REQUIRED_SERVER_NAME',
message: '"servername" is a required parameter'
code: 'ERR_MISSING_ARGS',
message: 'The "servername" argument must be specified'
});

const server = tls.createServer(serverOptions, function(c) {
Expand Down