From c2ea3aec65a162f816d5ebc9d67b903ec6a34309 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Wed, 6 Jan 2016 11:48:15 -0800 Subject: [PATCH 1/3] Parital implementation of ruby header checking --- grpc.gemspec | 2 +- src/ruby/ext/grpc/rb_call.c | 26 ++++++++++++++++---------- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/grpc.gemspec b/grpc.gemspec index ee5c22c24c9..006428f80a3 100755 --- a/grpc.gemspec +++ b/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' diff --git a/src/ruby/ext/grpc/rb_call.c b/src/ruby/ext/grpc/rb_call.c index 1647d9b484f..cbea76c3bfd 100644 --- a/src/ruby/ext/grpc/rb_call.c +++ b/src/ruby/ext/grpc/rb_call.c @@ -311,6 +311,20 @@ static int grpc_rb_md_ary_fill_hash_cb(VALUE key, VALUE val, VALUE md_ary_obj) { long array_length; long i; + char *key_str; + if (TYPE(key) == T_SYMBOL) { + key_str = (char *)rb_id2name(SYM2ID(key)); + } else { /* StringValueCStr does all other type exclusions for us */ + key_str = StringValueCStr(key); + } + + if (!grpc_header_key_is_legal(key_str)) { + 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); @@ -319,22 +333,14 @@ static int grpc_rb_md_ary_fill_hash_cb(VALUE key, VALUE val, VALUE md_ary_obj) { /* If the value is an array, add capacity for each value in the array */ array_length = RARRAY_LEN(val); 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); - } + md_ary->metadata[md_ary->count].key = key_str; 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->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); - } + md_ary->metadata[md_ary->count].key = key_str; md_ary->metadata[md_ary->count].value = RSTRING_PTR(val); md_ary->metadata[md_ary->count].value_length = RSTRING_LEN(val); md_ary->count += 1; From 56fada5dfede3a7b1bc63a0da36dc7e509fd36dd Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Wed, 6 Jan 2016 14:40:38 -0800 Subject: [PATCH 2/3] Make the Ruby extension throw an error when passed invalid metadata --- src/ruby/ext/grpc/rb_call.c | 40 +++++++++++++++++++++++++-------- templates/grpc.gemspec.template | 2 +- 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/src/ruby/ext/grpc/rb_call.c b/src/ruby/ext/grpc/rb_call.c index cbea76c3bfd..43adafb73f9 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,15 +310,20 @@ 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)) { + 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); @@ -330,19 +335,36 @@ static int grpc_rb_md_ary_fill_hash_cb(VALUE key, VALUE val, VALUE md_ary_obj) { &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++) { + 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].key = key_str; - 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].value = value_str; + md_ary->metadata[md_ary->count].value_length = value_len; md_ary->count += 1; } } else { + 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].key = key_str; - 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].value = value_str; + md_ary->metadata[md_ary->count].value_length = value_len; md_ary->count += 1; } diff --git a/templates/grpc.gemspec.template b/templates/grpc.gemspec.template index 5c220052e77..8839b9be554 100644 --- a/templates/grpc.gemspec.template +++ b/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' From 24e826ec5642a4bec8bcdc0fa554ceff9df6a942 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Wed, 6 Jan 2016 16:27:58 -0800 Subject: [PATCH 3/3] Updated dependencies, fixed a couple of tests --- grpc.gemspec | 2 +- src/ruby/pb/test/client.rb | 7 ------- src/ruby/spec/pb/health/checker_spec.rb | 9 ++++----- templates/grpc.gemspec.template | 2 +- 4 files changed, 6 insertions(+), 14 deletions(-) diff --git a/grpc.gemspec b/grpc.gemspec index f1561d61ec2..bf4435a9d71 100755 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -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) diff --git a/src/ruby/pb/test/client.rb b/src/ruby/pb/test/client.rb index 94bfff12608..8dbfed4b296 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 794c5922fa0..10d3a0705ac 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 } diff --git a/templates/grpc.gemspec.template b/templates/grpc.gemspec.template index 8839b9be554..fdf87ee13fd 100644 --- a/templates/grpc.gemspec.template +++ b/templates/grpc.gemspec.template @@ -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)