From 9ffebeab48e2e0b61dc0817430f089b6a1482ea7 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 11 Jan 2018 01:42:08 +0800 Subject: [PATCH] tls: migrate argument type-checking errors * Throw ERR_INVALID_ARG_TYPE from public APIs * Assert argument types in bindings instead of throwing errors PR-URL: https://github.com/nodejs/node/pull/18125 Reviewed-By: Anna Henningsen Reviewed-By: Fedor Indutny Reviewed-By: James M Snell --- lib/_tls_wrap.js | 17 +++++++++--- src/tls_wrap.cc | 23 +++++++--------- test/parallel/test-tls-basic-validations.js | 9 +++++-- test/parallel/test-tls-error-servername.js | 30 +++++++++++++++++++++ 4 files changed, 60 insertions(+), 19 deletions(-) create mode 100644 test/parallel/test-tls-error-servername.js diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index bb25eb5f155ea8..67dd559c87d3f1 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -36,6 +36,9 @@ const { Timer } = process.binding('timer_wrap'); const tls_wrap = process.binding('tls_wrap'); const { TCP, constants: TCPConstants } = process.binding('tcp_wrap'); const { Pipe, constants: PipeConstants } = process.binding('pipe_wrap'); +const { + SecureContext: NativeSecureContext +} = process.binding('crypto'); const errors = require('internal/errors'); const kConnectOptions = Symbol('connect-options'); const kDisableRenegotiation = Symbol('disable-renegotiation'); @@ -407,7 +410,12 @@ TLSSocket.prototype._wrapHandle = function(wrap) { const context = options.secureContext || options.credentials || tls.createSecureContext(options); - const res = tls_wrap.wrap(handle._externalStream, + const externalStream = handle._externalStream; + assert(typeof externalStream === 'object', + 'handle must be a LibuvStreamWrap'); + assert(context.context instanceof NativeSecureContext, + 'context.context must be a NativeSecureContext'); + const res = tls_wrap.wrap(externalStream, context.context, !!options.isServer); res._parent = handle; @@ -548,8 +556,8 @@ TLSSocket.prototype.renegotiate = function(options, callback) { if (this.destroyed) return; - let requestCert = this._requestCert; - let rejectUnauthorized = this._rejectUnauthorized; + let requestCert = !!this._requestCert; + let rejectUnauthorized = !!this._rejectUnauthorized; if (options.requestCert !== undefined) requestCert = !!options.requestCert; @@ -649,6 +657,9 @@ TLSSocket.prototype._start = function() { }; TLSSocket.prototype.setServername = function(name) { + if (typeof name !== 'string') { + throw new errors.Error('ERR_INVALID_ARG_TYPE', 'name', 'string'); + } this._handle.setServername(name); }; diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 78cc20810532c7..e505032345ed47 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -181,16 +181,10 @@ void TLSWrap::InitSSL() { void TLSWrap::Wrap(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - if (args.Length() < 1 || !args[0]->IsObject()) { - return env->ThrowTypeError( - "First argument should be a LibuvStreamWrap instance"); - } - if (args.Length() < 2 || !args[1]->IsObject()) { - return env->ThrowTypeError( - "Second argument should be a SecureContext instance"); - } - if (args.Length() < 3 || !args[2]->IsBoolean()) - return env->ThrowTypeError("Third argument should be boolean"); + CHECK_EQ(args.Length(), 3); + CHECK(args[0]->IsObject()); + CHECK(args[1]->IsObject()); + CHECK(args[2]->IsBoolean()); Local stream_obj = args[0].As(); Local sc = args[1].As(); @@ -758,8 +752,9 @@ void TLSWrap::SetVerifyMode(const FunctionCallbackInfo& args) { TLSWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); - if (args.Length() < 2 || !args[0]->IsBoolean() || !args[1]->IsBoolean()) - return env->ThrowTypeError("Bad arguments, expected two booleans"); + CHECK_EQ(args.Length(), 2); + CHECK(args[0]->IsBoolean()); + CHECK(args[1]->IsBoolean()); if (wrap->ssl_ == nullptr) return env->ThrowTypeError("SetVerifyMode after destroySSL"); @@ -855,8 +850,8 @@ void TLSWrap::SetServername(const FunctionCallbackInfo& args) { TLSWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); - if (args.Length() < 1 || !args[0]->IsString()) - return env->ThrowTypeError("First argument should be a string"); + CHECK_EQ(args.Length(), 1); + CHECK(args[0]->IsString()); if (wrap->started_) return env->ThrowError("Already started."); diff --git a/test/parallel/test-tls-basic-validations.js b/test/parallel/test-tls-basic-validations.js index fc2743ce042064..501cfd91d0ed87 100644 --- a/test/parallel/test-tls-basic-validations.js +++ b/test/parallel/test-tls-basic-validations.js @@ -39,8 +39,13 @@ assert.throws(() => tls.createServer({ ticketKeys: 'abcd' }), assert.throws(() => tls.createServer({ ticketKeys: Buffer.alloc(0) }), /TypeError: Ticket keys length must be 48 bytes/); -assert.throws(() => tls.createSecurePair({}), - /TypeError: Second argument should be a SecureContext instance/); +common.expectsError( + () => tls.createSecurePair({}), + { + code: 'ERR_ASSERTION', + message: 'context.context must be a NativeSecureContext' + } +); { const buffer = Buffer.from('abcd'); diff --git a/test/parallel/test-tls-error-servername.js b/test/parallel/test-tls-error-servername.js new file mode 100644 index 00000000000000..5c1960fce3cd31 --- /dev/null +++ b/test/parallel/test-tls-error-servername.js @@ -0,0 +1,30 @@ +'use strict'; + +// This tests the errors thrown from TLSSocket.prototype.setServername + +const common = require('../common'); +const fixtures = require('../common/fixtures'); + +if (!common.hasCrypto) + common.skip('missing crypto'); + +const { connect } = require('tls'); +const makeDuplexPair = require('../common/duplexpair'); +const { clientSide } = makeDuplexPair(); + +const ca = fixtures.readKey('ca1-cert.pem'); + +const client = connect({ + socket: clientSide, + ca, + host: 'agent1' // Hostname from certificate +}); + +[undefined, null, 1, true, {}].forEach((value) => { + common.expectsError(() => { + client.setServername(value); + }, { + code: 'ERR_INVALID_ARG_TYPE', + message: 'The "name" argument must be of type string' + }); +});