Merge pull request #4634 from murgatroid99/ruby_metadata_downcasing

Make the Ruby extension throw an error when passed invalid metadata

3 files changed, 47 insertions, 27 deletions

diff --git a/src/ruby/ext/grpc/rb_call.c b/src/ruby/ext/grpc/rb_call.c
index 1647d9b484..43adafb73f 100644
--- a/src/ruby/ext/grpc/rb_call.c
+++ b/src/ruby/ext/grpc/rb_call.c
@@ -1,6 +1,6 @@
 /*
  *
- * Copyright 2015, Google Inc.
+ * Copyright 2015-2016, Google Inc.
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -310,33 +310,61 @@ static int grpc_rb_md_ary_fill_hash_cb(VALUE key, VALUE val, VALUE md_ary_obj) {
   grpc_metadata_array *md_ary = NULL;
   long array_length;
   long i;
+  char *key_str;
+  size_t key_len;
+  char *value_str;
+  size_t value_len;
+
+  if (TYPE(key) == T_SYMBOL) {
+    key_str = (char *)rb_id2name(SYM2ID(key));
+    key_len = strlen(key_str);
+  } else { /* StringValueCStr does all other type exclusions for us */
+    key_str = StringValueCStr(key);
+    key_len = RSTRING_LEN(key);
+  }
+
+  if (!grpc_header_key_is_legal(key_str, key_len)) {
+    rb_raise(rb_eArgError,
+             "'%s' is an invalid header key, must match [a-z0-9-_.]+",
+             key_str);
+    return ST_STOP;
+  }
 
   /* Construct a metadata object from key and value and add it */
   TypedData_Get_Struct(md_ary_obj, grpc_metadata_array,
                        &grpc_rb_md_ary_data_type, md_ary);
 
   if (TYPE(val) == T_ARRAY) {
-    /* If the value is an array, add capacity for each value in the array */
     array_length = RARRAY_LEN(val);
+    /* If the value is an array, add capacity for each value in the array */
     for (i = 0; i < array_length; i++) {
-      if (TYPE(key) == T_SYMBOL) {
-        md_ary->metadata[md_ary->count].key = (char *)rb_id2name(SYM2ID(key));
-      } else { /* StringValueCStr does all other type exclusions for us */
-        md_ary->metadata[md_ary->count].key = StringValueCStr(key);
+      value_str = RSTRING_PTR(rb_ary_entry(val, i));
+      value_len = RSTRING_LEN(rb_ary_entry(val, i));
+      if (!grpc_is_binary_header(key_str, key_len) &&
+          !grpc_header_nonbin_value_is_legal(value_str, value_len)) {
+        // The value has invalid characters
+        rb_raise(rb_eArgError,
+                 "Header value '%s' has invalid characters", value_str);
+        return ST_STOP;
       }
-      md_ary->metadata[md_ary->count].value = RSTRING_PTR(rb_ary_entry(val, i));
-      md_ary->metadata[md_ary->count].value_length =
-          RSTRING_LEN(rb_ary_entry(val, i));
+      md_ary->metadata[md_ary->count].key = key_str;
+      md_ary->metadata[md_ary->count].value = value_str;
+      md_ary->metadata[md_ary->count].value_length = value_len;
       md_ary->count += 1;
     }
   } else {
-    if (TYPE(key) == T_SYMBOL) {
-      md_ary->metadata[md_ary->count].key = (char *)rb_id2name(SYM2ID(key));
-    } else { /* StringValueCStr does all other type exclusions for us */
-      md_ary->metadata[md_ary->count].key = StringValueCStr(key);
+    value_str = RSTRING_PTR(val);
+    value_len = RSTRING_LEN(val);
+    if (!grpc_is_binary_header(key_str, key_len) &&
+        !grpc_header_nonbin_value_is_legal(value_str, value_len)) {
+      // The value has invalid characters
+      rb_raise(rb_eArgError,
+               "Header value '%s' has invalid characters", value_str);
+      return ST_STOP;
     }
-    md_ary->metadata[md_ary->count].value = RSTRING_PTR(val);
-    md_ary->metadata[md_ary->count].value_length = RSTRING_LEN(val);
+    md_ary->metadata[md_ary->count].key = key_str;
+    md_ary->metadata[md_ary->count].value = value_str;
+    md_ary->metadata[md_ary->count].value_length = value_len;
     md_ary->count += 1;
   }
 
diff --git a/src/ruby/pb/test/client.rb b/src/ruby/pb/test/client.rb
index 94bfff1260..8dbfed4b29 100755
--- a/src/ruby/pb/test/client.rb
+++ b/src/ruby/pb/test/client.rb
@@ -56,8 +56,6 @@ require 'test/proto/empty'
 require 'test/proto/messages'
 require 'test/proto/test_services'
 
-require 'signet/ssl_config'
-
 AUTH_ENV = Google::Auth::CredentialsLoader::ENV_VAR
 
 # RubyLogger defines a logger for gRPC based on the standard ruby logger.
@@ -268,11 +266,6 @@ class NamedTests
     auth_creds = Google::Auth.get_application_default(@args.oauth_scope)
     kw = auth_creds.updater_proc.call({})
 
-    # TODO(jtattermusch): downcase the metadata keys here to make sure
-    # they are not rejected by C core. This is a hotfix that should
-    # be addressed by introducing auto-downcasing logic.
-    kw = Hash[ kw.each_pair.map { |k, v|  [k.downcase, v] }]
-
     resp = perform_large_unary(fill_username: true,
                                fill_oauth_scope: true,
                                **kw)
diff --git a/src/ruby/spec/pb/health/checker_spec.rb b/src/ruby/spec/pb/health/checker_spec.rb
index 794c5922fa..10d3a0705a 100644
--- a/src/ruby/spec/pb/health/checker_spec.rb
+++ b/src/ruby/spec/pb/health/checker_spec.rb
@@ -47,13 +47,12 @@ describe 'Health protobuf code generation' do
       end
 
       it 'should have the same content as created by code generation' do
-        root_dir = File.dirname(
-          File.dirname(File.dirname(File.dirname(__FILE__))))
-        pb_dir = File.join(root_dir, 'pb')
+        root_dir = File.join(File.dirname(__FILE__), '..', '..', '..', '..')
+        pb_dir = File.join(root_dir, 'proto')
 
         # Get the current content
-        service_path = File.join(pb_dir, 'grpc', 'health', 'v1alpha',
-                                 'health_services.rb')
+        service_path = File.join(root_dir, 'ruby', 'pb', 'grpc',
+                                 'health', 'v1alpha', 'health_services.rb')
         want = nil
         File.open(service_path) { |f| want = f.read }