errors: alter and test ERR_TLS_REQUIRED_SERVER_NAME#19988
errors: alter and test ERR_TLS_REQUIRED_SERVER_NAME#19988davidmarkclements wants to merge 2 commits intonodejs:masterfrom davidmarkclements:errors-patch10
Conversation
There was a problem hiding this comment.
The argument is called hostname in the docs, not sure what to use.
There was a problem hiding this comment.
Normally the variable name in the source code. I see the point here about using the documentation name though...
There was a problem hiding this comment.
If docs and source code disagree, one or the other should be changed so they are in agreement.
BridgeAR
left a comment
There was a problem hiding this comment.
It would be great to actually use ERR_MISSING_ARGS instead.
|
Ping @davidmarkclements |
|
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.
|
I think I've made the necessary changes such that this can land. @BridgeAR PTAL |
| // This should probably be a `TypeError`. | ||
| E('ERR_TLS_REQUIRED_SERVER_NAME', | ||
| '"servername" is required parameter for Server.addContext', Error); | ||
| '"servername" is a required parameter', TypeError); |
There was a problem hiding this comment.
This is now unused and should be removed.
|
@Trott with the code removed it's LGTM. |
| --> | ||
|
|
||
| * `hostname` {string} A SNI hostname or wildcard (e.g. `'*'`) | ||
| * `servername` {string} A SNI hostname or wildcard (e.g. `'*'`) |
There was a problem hiding this comment.
"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.
|
|
||
| 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). |
There was a problem hiding this comment.
"severname" -> "servername" (typo)
| const serverResults = []; | ||
| const clientResults = []; | ||
|
|
||
| // check for throwing case where servername is not supplied |
There was a problem hiding this comment.
Puncutate the comment, please (leading capital, trailing period).
|
|
||
| Used when a TLS renegotiation request has failed in a non-specific way. | ||
|
|
||
| <a id="ERR_TLS_REQUIRED_SERVER_NAME"></a> |
There was a problem hiding this comment.
Can be removed now, as its no longer used.
| Server.prototype.addContext = function(servername, context) { | ||
| if (!servername) { | ||
| throw new ERR_TLS_REQUIRED_SERVER_NAME(); | ||
| throw new ERR_MISSING_ARGS('servername'); |
There was a problem hiding this comment.
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.
|
Closing due to long inactivity. @davidmarkclements please open a new PR in case you would like to continue working on this. |
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), orvcbuild test(Windows) passes