src: add error codes to errors thrown in C++

yanivfriedensohn · Trott · commit a0e2c6d2843e · 2019-08-09T15:58:58.000-07:00
PR-URL: #27700 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
diff --git a/lib/internal/crypto/cipher.js b/lib/internal/crypto/cipher.js
@@ -12,7 +12,7 @@ const {
   ERR_INVALID_ARG_TYPE,
   ERR_INVALID_OPT_VALUE
 } = require('internal/errors').codes;
-const { validateString } = require('internal/validators');
+const { validateEncoding, validateString } = require('internal/validators');
 
 const {
   preparePrivateKey,
@@ -161,6 +161,8 @@ Cipher.prototype.update = function update(data, inputEncoding, outputEncoding) {
     throw invalidArrayBufferView('data', data);
   }
 
+  validateEncoding(data, inputEncoding);
+
   const ret = this[kHandle].update(data, inputEncoding);
 
   if (outputEncoding && outputEncoding !== 'buffer') {
diff --git a/lib/internal/crypto/hash.js b/lib/internal/crypto/hash.js
@@ -25,7 +25,8 @@ const {
   ERR_CRYPTO_HASH_UPDATE_FAILED,
   ERR_INVALID_ARG_TYPE
 } = require('internal/errors').codes;
-const { validateString, validateUint32 } = require('internal/validators');
+const { validateEncoding, validateString, validateUint32 } =
+  require('internal/validators');
 const { normalizeEncoding } = require('internal/util');
 const { isArrayBufferView } = require('internal/util/types');
 const LazyTransform = require('internal/streams/lazy_transform');
@@ -61,6 +62,8 @@ Hash.prototype._flush = function _flush(callback) {
 };
 
 Hash.prototype.update = function update(data, encoding) {
+  encoding = encoding || getDefaultEncoding();
+
   const state = this[kState];
   if (state[kFinalized])
     throw new ERR_CRYPTO_HASH_FINALIZED();
@@ -74,7 +77,9 @@ Hash.prototype.update = function update(data, encoding) {
                                    data);
   }
 
-  if (!this[kHandle].update(data, encoding || getDefaultEncoding()))
+  validateEncoding(data, encoding);
+
+  if (!this[kHandle].update(data, encoding))
     throw new ERR_CRYPTO_HASH_UPDATE_FAILED();
   return this;
 };
diff --git a/lib/internal/validators.js b/lib/internal/validators.js
@@ -9,6 +9,7 @@ const {
     ERR_UNKNOWN_SIGNAL
   }
 } = require('internal/errors');
+const { normalizeEncoding } = require('internal/util');
 const {
   isArrayBufferView
 } = require('internal/util/types');
@@ -142,11 +143,24 @@ const validateBuffer = hideStackFrames((buffer, name = 'buffer') => {
   }
 });
 
+function validateEncoding(data, encoding) {
+  const normalizedEncoding = normalizeEncoding(encoding);
+  const length = data.length;
+
+  if (normalizedEncoding === 'hex' && length % 2 !== 0) {
+    throw new ERR_INVALID_ARG_VALUE('encoding', encoding,
+                                    `is invalid for data of length ${length}`);
+  }
+
+  // TODO(bnoordhuis) Add BASE64 check?
+}
+
 module.exports = {
   isInt32,
   isUint32,
   parseMode,
   validateBuffer,
+  validateEncoding,
   validateInteger,
   validateInt32,
   validateUint32,
diff --git a/src/node_buffer.cc b/src/node_buffer.cc
@@ -843,7 +843,7 @@ void IndexOfString(const FunctionCallbackInfo<Value>& args) {
 
     if (IsBigEndian()) {
       StringBytes::InlineDecoder decoder;
-      if (decoder.Decode(env, needle, args[3], UCS2).IsNothing()) return;
+      if (decoder.Decode(env, needle, enc).IsNothing()) return;
       const uint16_t* decoded_string =
           reinterpret_cast<const uint16_t*>(decoder.out());
 
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -4317,8 +4317,9 @@ void CipherBase::Update(const FunctionCallbackInfo<Value>& args) {
   // Only copy the data if we have to, because it's a string
   if (args[0]->IsString()) {
     StringBytes::InlineDecoder decoder;
-    if (!decoder.Decode(env, args[0].As<String>(), args[1], UTF8)
-             .FromMaybe(false))
+    enum encoding enc = ParseEncoding(env->isolate(), args[1], UTF8);
+
+    if (decoder.Decode(env, args[0].As<String>(), enc).IsNothing())
       return;
     r = cipher->Update(decoder.out(), decoder.size(), &out);
   } else {
@@ -4501,8 +4502,9 @@ void Hmac::HmacUpdate(const FunctionCallbackInfo<Value>& args) {
   bool r = false;
   if (args[0]->IsString()) {
     StringBytes::InlineDecoder decoder;
-    if (decoder.Decode(env, args[0].As<String>(), args[1], UTF8)
-            .FromMaybe(false)) {
+    enum encoding enc = ParseEncoding(env->isolate(), args[1], UTF8);
+
+    if (!decoder.Decode(env, args[0].As<String>(), enc).IsNothing()) {
       r = hmac->HmacUpdate(decoder.out(), decoder.size());
     }
   } else {
@@ -4626,8 +4628,9 @@ void Hash::HashUpdate(const FunctionCallbackInfo<Value>& args) {
   bool r = true;
   if (args[0]->IsString()) {
     StringBytes::InlineDecoder decoder;
-    if (!decoder.Decode(env, args[0].As<String>(), args[1], UTF8)
-             .FromMaybe(false)) {
+    enum encoding enc = ParseEncoding(env->isolate(), args[1], UTF8);
+
+    if (decoder.Decode(env, args[0].As<String>(), enc).IsNothing()) {
       args.GetReturnValue().Set(false);
       return;
     }
diff --git a/src/string_bytes.cc b/src/string_bytes.cc
@@ -392,15 +392,6 @@ size_t StringBytes::Write(Isolate* isolate,
 }
 
 
-bool StringBytes::IsValidString(Local<String> string,
-                                enum encoding enc) {
-  if (enc == HEX && string->Length() % 2 != 0)
-    return false;
-  // TODO(bnoordhuis) Add BASE64 check?
-  return true;
-}
-
-
 // Quick and dirty size calculation
 // Will always be at least big enough, but may have some extra
 // UTF8 can be as much as 3x the size, Base64 can have 1-2 extra bytes
diff --git a/src/string_bytes.h b/src/string_bytes.h
@@ -37,14 +37,7 @@ class StringBytes {
    public:
     inline v8::Maybe<bool> Decode(Environment* env,
                                   v8::Local<v8::String> string,
-                                  v8::Local<v8::Value> encoding,
-                                  enum encoding _default) {
-      enum encoding enc = ParseEncoding(env->isolate(), encoding, _default);
-      if (!StringBytes::IsValidString(string, enc)) {
-        env->ThrowTypeError("Bad input string");
-        return v8::Nothing<bool>();
-      }
-
+                                  enum encoding enc) {
       size_t storage;
       if (!StringBytes::StorageSize(env->isolate(), string, enc).To(&storage))
         return v8::Nothing<bool>();
@@ -60,12 +53,6 @@ class StringBytes {
     inline size_t size() const { return length(); }
   };
 
-  // Does the string match the encoding? Quick but non-exhaustive.
-  // Example: a HEX string must have a length that's a multiple of two.
-  // FIXME(bnoordhuis) IsMaybeValidString()? Naming things is hard...
-  static bool IsValidString(v8::Local<v8::String> string,
-                            enum encoding enc);
-
   // Fast, but can be 2 bytes oversized for Base64, and
   // as much as triple UTF-8 strings <= 65536 chars in length
   static v8::Maybe<size_t> StorageSize(v8::Isolate* isolate,
diff --git a/test/parallel/test-crypto.js b/test/parallel/test-crypto.js
@@ -167,42 +167,66 @@ testImmutability(crypto.getCurves);
 
 // Regression tests for https://github.com/nodejs/node-v0.x-archive/pull/5725:
 // hex input that's not a power of two should throw, not assert in C++ land.
-assert.throws(function() {
-  crypto.createCipher('aes192', 'test').update('0', 'hex');
-}, (err) => {
-  const errorMessage =
-    common.hasFipsCrypto ? /not supported in FIPS mode/ : /Bad input string/;
-  // Throws general Error, so there is no opensslErrorStack property.
-  if ((err instanceof Error) &&
-      errorMessage.test(err) &&
-      err.opensslErrorStack === undefined) {
-    return true;
-  }
-});
 
-assert.throws(function() {
-  crypto.createDecipher('aes192', 'test').update('0', 'hex');
-}, (err) => {
-  const errorMessage =
-    common.hasFipsCrypto ? /not supported in FIPS mode/ : /Bad input string/;
-  // Throws general Error, so there is no opensslErrorStack property.
-  if ((err instanceof Error) &&
-      errorMessage.test(err) &&
-      err.opensslErrorStack === undefined) {
-    return true;
+common.expectsError(
+  () => crypto.createCipher('aes192', 'test').update('0', 'hex'),
+  Object.assign(
+    common.hasFipsCrypto ?
+      {
+        code: undefined,
+        type: Error,
+        message: /not supported in FIPS mode/,
+      } :
+      {
+        code: 'ERR_INVALID_ARG_VALUE',
+        type: TypeError,
+        message: "The argument 'encoding' is invalid for data of length 1." +
+            " Received 'hex'",
+      },
+    { opensslErrorStack: undefined }
+  )
+);
+
+common.expectsError(
+  () => crypto.createDecipher('aes192', 'test').update('0', 'hex'),
+  Object.assign(
+    common.hasFipsCrypto ?
+      {
+        code: undefined,
+        type: Error,
+        message: /not supported in FIPS mode/,
+      } :
+      {
+        code: 'ERR_INVALID_ARG_VALUE',
+        type: TypeError,
+        message: "The argument 'encoding' is invalid for data of length 1." +
+            " Received 'hex'",
+      },
+    { opensslErrorStack: undefined }
+  )
+);
+
+common.expectsError(
+  () => crypto.createHash('sha1').update('0', 'hex'),
+  {
+    code: 'ERR_INVALID_ARG_VALUE',
+    type: TypeError,
+    message: "The argument 'encoding' is invalid for data of length 1." +
+        " Received 'hex'",
+    opensslErrorStack: undefined
   }
-});
+);
 
-assert.throws(function() {
-  crypto.createHash('sha1').update('0', 'hex');
-}, (err) => {
-  // Throws TypeError, so there is no opensslErrorStack property.
-  if ((err instanceof Error) &&
-      /^TypeError: Bad input string$/.test(err) &&
-      err.opensslErrorStack === undefined) {
-    return true;
+common.expectsError(
+  () => crypto.createHmac('sha256', 'a secret').update('0', 'hex'),
+  {
+    code: 'ERR_INVALID_ARG_VALUE',
+    type: TypeError,
+    message: "The argument 'encoding' is invalid for data of length 1." +
+        " Received 'hex'",
+    opensslErrorStack: undefined
   }
-});
+);
 
 assert.throws(function() {
   const priv = [
