diff --git a/fog-openstack.gemspec b/fog-openstack.gemspec index 4d5d6e65e..19ac8c521 100644 --- a/fog-openstack.gemspec +++ b/fog-openstack.gemspec @@ -24,6 +24,9 @@ Gem::Specification.new do |spec| spec.add_dependency 'fog-core', '~> 2.1' spec.add_dependency 'fog-json', '>= 1.0' + # base64 became a bundled gem in Ruby 3.4, so it must be declared explicitly. + # webmock requires it but does not depend on it. + spec.add_development_dependency 'base64' spec.add_development_dependency 'bundler' spec.add_development_dependency 'coveralls' spec.add_development_dependency "mime-types" diff --git a/lib/fog/openstack/compute/requests/delete_service.rb b/lib/fog/openstack/compute/requests/delete_service.rb index 6193a3da9..5e62b7271 100644 --- a/lib/fog/openstack/compute/requests/delete_service.rb +++ b/lib/fog/openstack/compute/requests/delete_service.rb @@ -4,7 +4,7 @@ class Compute class Real def delete_service(uuid, optional_params = nil) # Encode all params - optional_params = optional_params.each { |k, v| optional_params[k] = URI.encode(v) } if optional_params + optional_params = optional_params.transform_values { |v| Fog::OpenStack.escape(v) } if optional_params request( :expects => [202, 204], diff --git a/lib/fog/openstack/compute/requests/disable_service.rb b/lib/fog/openstack/compute/requests/disable_service.rb index cc20f0e4f..5ef3fe8f0 100644 --- a/lib/fog/openstack/compute/requests/disable_service.rb +++ b/lib/fog/openstack/compute/requests/disable_service.rb @@ -6,7 +6,7 @@ def disable_service(host, binary, optional_params = nil) data = {"host" => host, "binary" => binary} # Encode all params - optional_params = optional_params.each { |k, v| optional_params[k] = URI.encode(v) } if optional_params + optional_params = optional_params.transform_values { |v| Fog::OpenStack.escape(v) } if optional_params request( :body => Fog::JSON.encode(data), diff --git a/lib/fog/openstack/compute/requests/disable_service_log_reason.rb b/lib/fog/openstack/compute/requests/disable_service_log_reason.rb index c18c834f6..ca9e8b843 100644 --- a/lib/fog/openstack/compute/requests/disable_service_log_reason.rb +++ b/lib/fog/openstack/compute/requests/disable_service_log_reason.rb @@ -6,7 +6,7 @@ def disable_service_log_reason(host, binary, disabled_reason, optional_params = data = {"host" => host, "binary" => binary, "disabled_reason" => disabled_reason} # Encode all params - optional_params = optional_params.each { |k, v| optional_params[k] = URI.encode(v) } if optional_params + optional_params = optional_params.transform_values { |v| Fog::OpenStack.escape(v) } if optional_params request( :body => Fog::JSON.encode(data), diff --git a/lib/fog/openstack/compute/requests/enable_service.rb b/lib/fog/openstack/compute/requests/enable_service.rb index cc146268c..346716a65 100644 --- a/lib/fog/openstack/compute/requests/enable_service.rb +++ b/lib/fog/openstack/compute/requests/enable_service.rb @@ -6,7 +6,7 @@ def enable_service(host, binary, optional_params = nil) data = {"host" => host, "binary" => binary} # Encode all params - optional_params = optional_params.each { |k, v| optional_params[k] = URI.encode(v) } if optional_params + optional_params = optional_params.transform_values { |v| Fog::OpenStack.escape(v) } if optional_params request( :body => Fog::JSON.encode(data), diff --git a/lib/fog/openstack/storage/requests/delete_multiple_objects.rb b/lib/fog/openstack/storage/requests/delete_multiple_objects.rb index ca69fecfa..f176a51d6 100644 --- a/lib/fog/openstack/storage/requests/delete_multiple_objects.rb +++ b/lib/fog/openstack/storage/requests/delete_multiple_objects.rb @@ -45,7 +45,7 @@ class Real def delete_multiple_objects(container, object_names, options = {}) body = object_names.map do |name| object_name = container ? "#{container}/#{name}" : name - URI.encode(object_name) + Fog::OpenStack.escape(object_name, '/') end.join("\n") response = request({ diff --git a/lib/fog/openstack/workflow/v2/requests/delete_action.rb b/lib/fog/openstack/workflow/v2/requests/delete_action.rb index 4c4918980..85258c824 100644 --- a/lib/fog/openstack/workflow/v2/requests/delete_action.rb +++ b/lib/fog/openstack/workflow/v2/requests/delete_action.rb @@ -7,7 +7,7 @@ def delete_action(name) request( :expects => 204, :method => "DELETE", - :path => "actions/#{URI.encode(name)}" + :path => "actions/#{Fog::OpenStack.escape(name)}" ) end end diff --git a/lib/fog/openstack/workflow/v2/requests/delete_cron_trigger.rb b/lib/fog/openstack/workflow/v2/requests/delete_cron_trigger.rb index 9cf48bc51..2636f7ed4 100644 --- a/lib/fog/openstack/workflow/v2/requests/delete_cron_trigger.rb +++ b/lib/fog/openstack/workflow/v2/requests/delete_cron_trigger.rb @@ -7,7 +7,7 @@ def delete_cron_trigger(name) request( :expects => 204, :method => "DELETE", - :path => "cron_triggers/#{URI.encode(name)}" + :path => "cron_triggers/#{Fog::OpenStack.escape(name)}" ) end end diff --git a/lib/fog/openstack/workflow/v2/requests/delete_environment.rb b/lib/fog/openstack/workflow/v2/requests/delete_environment.rb index 21308810f..49f1d9fac 100644 --- a/lib/fog/openstack/workflow/v2/requests/delete_environment.rb +++ b/lib/fog/openstack/workflow/v2/requests/delete_environment.rb @@ -7,7 +7,7 @@ def delete_environment(name) request( :expects => 204, :method => "DELETE", - :path => "environments/#{URI.encode(name)}" + :path => "environments/#{Fog::OpenStack.escape(name)}" ) end end diff --git a/lib/fog/openstack/workflow/v2/requests/delete_workbook.rb b/lib/fog/openstack/workflow/v2/requests/delete_workbook.rb index 649caea29..53dee1797 100644 --- a/lib/fog/openstack/workflow/v2/requests/delete_workbook.rb +++ b/lib/fog/openstack/workflow/v2/requests/delete_workbook.rb @@ -7,7 +7,7 @@ def delete_workbook(name) request( :expects => 204, :method => "DELETE", - :path => "workbooks/#{URI.encode(name)}" + :path => "workbooks/#{Fog::OpenStack.escape(name)}" ) end end diff --git a/lib/fog/openstack/workflow/v2/requests/get_action.rb b/lib/fog/openstack/workflow/v2/requests/get_action.rb index c937c48f7..778a143e8 100644 --- a/lib/fog/openstack/workflow/v2/requests/get_action.rb +++ b/lib/fog/openstack/workflow/v2/requests/get_action.rb @@ -7,7 +7,7 @@ def get_action(name) request( :expects => 200, :method => "GET", - :path => "actions/#{URI.encode(name)}" + :path => "actions/#{Fog::OpenStack.escape(name)}" ) end end diff --git a/lib/fog/openstack/workflow/v2/requests/get_cron_trigger.rb b/lib/fog/openstack/workflow/v2/requests/get_cron_trigger.rb index 924362d6d..347b0257f 100644 --- a/lib/fog/openstack/workflow/v2/requests/get_cron_trigger.rb +++ b/lib/fog/openstack/workflow/v2/requests/get_cron_trigger.rb @@ -7,7 +7,7 @@ def get_cron_trigger(name) request( :expects => 200, :method => "GET", - :path => "cron_triggers/#{URI.encode(name)}" + :path => "cron_triggers/#{Fog::OpenStack.escape(name)}" ) end end diff --git a/lib/fog/openstack/workflow/v2/requests/get_environment.rb b/lib/fog/openstack/workflow/v2/requests/get_environment.rb index 5bdad97db..0e6f97de8 100644 --- a/lib/fog/openstack/workflow/v2/requests/get_environment.rb +++ b/lib/fog/openstack/workflow/v2/requests/get_environment.rb @@ -7,7 +7,7 @@ def get_environment(name) request( :expects => 200, :method => "GET", - :path => "environments/#{URI.encode(name)}" + :path => "environments/#{Fog::OpenStack.escape(name)}" ) end end diff --git a/lib/fog/openstack/workflow/v2/requests/get_workbook.rb b/lib/fog/openstack/workflow/v2/requests/get_workbook.rb index 33597a310..00a8037e2 100644 --- a/lib/fog/openstack/workflow/v2/requests/get_workbook.rb +++ b/lib/fog/openstack/workflow/v2/requests/get_workbook.rb @@ -7,7 +7,7 @@ def get_workbook(name) request( :expects => 200, :method => "GET", - :path => "workbooks/#{URI.encode(name)}" + :path => "workbooks/#{Fog::OpenStack.escape(name)}" ) end end diff --git a/spec/escaping_spec.rb b/spec/escaping_spec.rb new file mode 100644 index 000000000..2a0148163 --- /dev/null +++ b/spec/escaping_spec.rb @@ -0,0 +1,98 @@ +require 'spec_helper' + +require 'fog/openstack/compute/requests/enable_service' +require 'fog/openstack/storage/requests/delete_multiple_objects' +require 'fog/openstack/workflow/v2/requests/get_action' + +# Builds a Real instance without running its initializer (which would need +# credentials and a live endpoint) and captures what it hands to #request. +def capture_request(klass, response_body = '{}') + instance = klass.allocate + captured = nil + instance.define_singleton_method(:request) do |params, *_args| + captured = params + Excon::Response.new(:body => response_body) + end + yield instance + captured +end + +describe 'URL escaping' do + describe 'Fog::OpenStack.escape' do + # This is why these call sites cannot use CGI.escape: in a URL *path* a + # '+' is a literal plus, not a space, so CGI.escape corrupts any name + # containing a space. + it 'encodes a space as %20 rather than +' do + assert_equal 'sp%20ace', Fog::OpenStack.escape('sp ace') + assert_equal 'sp+ace', CGI.escape('sp ace') + end + + it 'leaves unreserved characters alone' do + assert_equal 'name-with.dots_and-dashes', Fog::OpenStack.escape('name-with.dots_and-dashes') + end + + it 'escapes a slash by default but can be told to keep it' do + assert_equal 'a%2Fb', Fog::OpenStack.escape('a/b') + assert_equal 'a/b', Fog::OpenStack.escape('a/b', '/') + end + end + + describe 'Workflow::V2 resource names in the path' do + it 'escapes the name into a single path segment' do + captured = capture_request(Fog::OpenStack::Workflow::V2::Real) do |real| + real.get_action('my action') + end + assert_equal 'actions/my%20action', captured[:path] + end + + it 'escapes a slash inside the name so it cannot forge a path segment' do + captured = capture_request(Fog::OpenStack::Workflow::V2::Real) do |real| + real.get_action('a/b') + end + assert_equal 'actions/a%2Fb', captured[:path] + end + end + + describe 'Storage#delete_multiple_objects' do + it 'escapes each name but keeps the container/object separator intact' do + captured = capture_request(Fog::OpenStack::Storage::Real) do |real| + real.delete_multiple_objects('my container', ['some object', 'plain']) + end + assert_equal "my%20container/some%20object\nmy%20container/plain", captured[:body] + end + + it 'escapes bare names when no container is given' do + captured = capture_request(Fog::OpenStack::Storage::Real) do |real| + real.delete_multiple_objects(nil, ['some object']) + end + assert_equal 'some%20object', captured[:body] + end + end + + describe 'Compute service optional params' do + it 'escapes the values' do + captured = capture_request(Fog::OpenStack::Compute::Real) do |real| + real.enable_service('host1', 'nova-compute', 'reason' => 'out of service') + end + assert_equal({'reason' => 'out%20of%20service'}, captured[:query]) + end + + it 'does not mutate the hash the caller passed in' do + params = {'reason' => 'out of service'} + capture_request(Fog::OpenStack::Compute::Real) do |real| + real.enable_service('host1', 'nova-compute', params) + end + assert_equal({'reason' => 'out of service'}, params) + end + end + + describe 'the whole library' do + it 'no longer calls URI.encode or URI.escape, removed in Ruby 3.0' do + lib = File.expand_path('../lib', __dir__) + offenders = Dir.glob("#{lib}/**/*.rb").select do |file| + File.read(file).match?(/\bURI\.(encode|escape)\b/) + end + assert_empty(offenders.map { |f| f.sub("#{lib}/", '') }) + end + end +end