Merge pull request #4634 from murgatroid99/ruby_metadata_downcasing

Make the Ruby extension throw an error when passed invalid metadata

grpc.gemspec

@@ -33,7 +33,7 @@ Gem::Specification.new do |s|
   s.platform      = Gem::Platform::RUBY
 
   s.add_dependency 'google-protobuf', '~> 3.0.0alpha.1.1'
-  s.add_dependency 'googleauth', '~> 0.4'
+  s.add_dependency 'googleauth', '~> 0.5.1'
 
   s.add_development_dependency 'bundler', '~> 1.9'
   s.add_development_dependency 'logging', '~> 2.0'
@@ -42,7 +42,7 @@ Gem::Specification.new do |s|
   s.add_development_dependency 'rake-compiler', '~> 0.9'
   s.add_development_dependency 'rspec', '~> 3.2'
   s.add_development_dependency 'rubocop', '~> 0.30.0'
-  s.add_development_dependency 'signet', '~>0.6.0'
+  s.add_development_dependency 'signet', '~>0.7.0'
 
   s.extensions = %w(src/ruby/ext/grpc/extconf.rb)
 

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;
   }
 

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)

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 }
 

templates/grpc.gemspec.template

@@ -35,7 +35,7 @@
     s.platform      = Gem::Platform::RUBY
 
     s.add_dependency 'google-protobuf', '~> 3.0.0alpha.1.1'
-    s.add_dependency 'googleauth', '~> 0.4'
+    s.add_dependency 'googleauth', '~> 0.5.1'
 
     s.add_development_dependency 'bundler', '~> 1.9'
     s.add_development_dependency 'logging', '~> 2.0'
@@ -44,7 +44,7 @@
     s.add_development_dependency 'rake-compiler', '~> 0.9'
     s.add_development_dependency 'rspec', '~> 3.2'
     s.add_development_dependency 'rubocop', '~> 0.30.0'
-    s.add_development_dependency 'signet', '~>0.6.0'
+    s.add_development_dependency 'signet', '~>0.7.0'
 
     s.extensions = %w(src/ruby/ext/grpc/extconf.rb)