Node: fix metadata validation bug, improve error reporting

src/node/ext/call_credentials.cc

@@ -187,7 +187,8 @@ NAN_METHOD(PluginCallback) {
   shared_ptr<Resources> resources(new Resources);
   grpc_status_code code = static_cast<grpc_status_code>(
       Nan::To<uint32_t>(info[0]).FromJust());
-  char *details = *Utf8String(info[1]);
+  Utf8String details_utf8_str(info[1]);
+  char *details = *details_utf8_str;
   grpc_metadata_array array;
   if (!CreateMetadataArray(Nan::To<Object>(info[2]).ToLocalChecked(),
                            &array, resources)){

src/node/ext/node_grpc.cc

@@ -237,7 +237,8 @@ NAN_METHOD(MetadataKeyIsLegal) {
         "headerKeyIsLegal's argument must be a string");
   }
   Local<String> key = Nan::To<String>(info[0]).ToLocalChecked();
-  char *key_str = *Nan::Utf8String(key);
+  Nan::Utf8String key_utf8_str(key);
+  char *key_str = *key_utf8_str;
   info.GetReturnValue().Set(static_cast<bool>(
       grpc_header_key_is_legal(key_str, static_cast<size_t>(key->Length()))));
 }
@@ -248,7 +249,8 @@ NAN_METHOD(MetadataNonbinValueIsLegal) {
         "metadataNonbinValueIsLegal's argument must be a string");
   }
   Local<String> value = Nan::To<String>(info[0]).ToLocalChecked();
-  char *value_str = *Nan::Utf8String(value);
+  Nan::Utf8String value_utf8_str(value);
+  char *value_str = *value_utf8_str;
   info.GetReturnValue().Set(static_cast<bool>(
       grpc_header_nonbin_value_is_legal(
           value_str, static_cast<size_t>(value->Length()))));
@@ -260,7 +262,8 @@ NAN_METHOD(MetadataKeyIsBinary) {
         "metadataKeyIsLegal's argument must be a string");
   }
   Local<String> key = Nan::To<String>(info[0]).ToLocalChecked();
-  char *key_str = *Nan::Utf8String(key);
+  Nan::Utf8String key_utf8_str(key);
+  char *key_str = *key_utf8_str;
   info.GetReturnValue().Set(static_cast<bool>(
       grpc_is_binary_header(key_str, static_cast<size_t>(key->Length()))));
 }

src/node/src/credentials.js

@@ -98,6 +98,8 @@ exports.createFromMetadataGenerator = function(metadata_generator) {
         message = error.message;
         if (error.hasOwnProperty('code')) {
           code = error.code;
+        } else {
+          code = grpc.status.UNAUTHENTICATED;
         }
         if (!metadata) {
           metadata = new Metadata();
@@ -116,13 +118,16 @@ exports.createFromMetadataGenerator = function(metadata_generator) {
 exports.createFromGoogleCredential = function(google_credential) {
   return exports.createFromMetadataGenerator(function(auth_context, callback) {
     var service_url = auth_context.service_url;
+    console.log('Service URL:', service_url);
     google_credential.getRequestMetadata(service_url, function(err, header) {
       if (err) {
+        console.log('Auth error:', err);
         callback(err);
         return;
       }
       var metadata = new Metadata();
       metadata.add('authorization', header.Authorization);
+      console.log(header.Authorization);
       callback(null, metadata);
     });
   });

src/node/src/metadata.js

@@ -64,7 +64,7 @@ function normalizeKey(key) {
   if (grpc.metadataKeyIsLegal(key)) {
     return key;
   } else {
-    throw new Error('Metadata key contains illegal characters');
+    throw new Error('Metadata key"' + key + '" contains illegal characters');
   }
 }
 
@@ -79,7 +79,8 @@ function validate(key, value) {
           'keys that don\'t end with \'-bin\' must have String values');
     }
     if (!grpc.metadataNonbinValueIsLegal(value)) {
-      throw new Error('Metadata string value contains illegal characters');
+      throw new Error('Metadata string value "' + value +
+                      '" contains illegal characters');
     }
   }
 }

tools/run_tests/run_interop_tests.py

@@ -422,7 +422,7 @@ def _job_kill_handler(job):
     time.sleep(2)
 
 
-def cloud_to_prod_jobspec(language, test_case, server_host_name, 
+def cloud_to_prod_jobspec(language, test_case, server_host_name,
                           server_host_detail, docker_image=None, auth=False):
   """Creates jobspec for cloud-to-prod interop test"""
   container_name = None
@@ -441,7 +441,7 @@ def cloud_to_prod_jobspec(language, test_case, server_host_name,
   cwd = language.client_cwd
 
   if docker_image:
-    container_name = dockerjob.random_name('interop_client_%s' % 
+    container_name = dockerjob.random_name('interop_client_%s' %
                                            language.safename)
     cmdline = docker_run_cmdline(cmdline,
                                  image=docker_image,
@@ -457,7 +457,7 @@ def cloud_to_prod_jobspec(language, test_case, server_host_name,
           cmdline=cmdline,
           cwd=cwd,
           environ=environ,
-          shortname='%s:%s:%s:%s' % (suite_name, server_host_name, language, 
+          shortname='%s:%s:%s:%s' % (suite_name, server_host_name, language,
                                      test_case),
           timeout_seconds=90,
           flake_retries=5 if args.allow_flakes else 0,
@@ -575,18 +575,18 @@ def aggregate_http2_results(stdout):
     'percent': 1.0 * passed / (passed + failed)
   }
 
-# A dictionary of prod servers to test. 
+# A dictionary of prod servers to test.
 # Format: server_name: (server_host, server_host_override, errors_allowed)
 # TODO(adelez): implement logic for errors_allowed where if the indicated tests
 # fail, they don't impact the overall test result.
 prod_servers = {
-    'default': ('grpc-test.sandbox.googleapis.com', 
+    'default': ('grpc-test.sandbox.googleapis.com',
                 'grpc-test.sandbox.googleapis.com', False),
-    'gateway_v2': ('grpc-test2.sandbox.googleapis.com', 
+    'gateway_v2': ('grpc-test2.sandbox.googleapis.com',
                    'grpc-test2.sandbox.googleapis.com', True),
-    'cloud_gateway': ('216.239.32.255', 'grpc-test.sandbox.googleapis.com', 
+    'cloud_gateway': ('216.239.32.255', 'grpc-test.sandbox.googleapis.com',
                       False),
-    'cloud_gateway_v2': ('216.239.32.255', 'grpc-test2.sandbox.googleapis.com', 
+    'cloud_gateway_v2': ('216.239.32.255', 'grpc-test2.sandbox.googleapis.com',
                          True)
 }
 
@@ -720,7 +720,7 @@ try:
           if not test_case in language.unimplemented_test_cases():
             if not test_case in _SKIP_ADVANCED + _SKIP_COMPRESSION:
               test_job = cloud_to_prod_jobspec(
-                  language, test_case, server_host_name, 
+                  language, test_case, server_host_name,
                   prod_servers[server_host_name],
                   docker_image=docker_images.get(str(language)))
               jobs.append(test_job)
@@ -728,7 +728,7 @@ try:
       if args.http2_interop:
         for test_case in _HTTP2_TEST_CASES:
           test_job = cloud_to_prod_jobspec(
-              http2Interop, test_case, server_host_name, 
+              http2Interop, test_case, server_host_name,
               prod_servers[server_host_name],
               docker_image=docker_images.get(str(http2Interop)))
           jobs.append(test_job)
@@ -739,7 +739,7 @@ try:
         for test_case in _AUTH_TEST_CASES:
           if not test_case in language.unimplemented_test_cases():
             test_job = cloud_to_prod_jobspec(
-                language, test_case, server_host_name, 
+                language, test_case, server_host_name,
                 prod_servers[server_host_name],
                 docker_image=docker_images.get(str(language)), auth=True)
             jobs.append(test_job)
@@ -802,7 +802,7 @@ try:
   report_utils.render_interop_html_report(
       set([str(l) for l in languages]), servers, _TEST_CASES, _AUTH_TEST_CASES,
       _HTTP2_TEST_CASES, resultset, num_failures,
-      args.cloud_to_prod_auth or args.cloud_to_prod, args.prod_servers, 
+      args.cloud_to_prod_auth or args.cloud_to_prod, args.prod_servers,
       args.http2_interop)
 
 finally: