From 233f40762bed5d2c4f9b7b50459e1b1fdc5eee9e Mon Sep 17 00:00:00 2001 From: Tim Smith Date: Mon, 24 Aug 2026 11:42:30 -0700 Subject: [PATCH] Replace URI.encode, removed in Ruby 3.0, with Fog::OpenStack.escape URI.encode (an alias for URI.escape) was removed in Ruby 3.0, so every one of these call sites raises NoMethodError on Ruby 3 and later. The library still loads; it fails when the request is actually made. This gem already has the right helper for the job. Fog::OpenStack.escape is documented as 'CGI.escape, but without special treatment on spaces' and is what the storage requests have always used. The thirteen call sites here are simply the ones that never got migrated to it. CGI.escape is not a suitable replacement: it encodes a space as '+', which is only correct in a query string. In a URL path a '+' is a literal plus, so any resource name containing a space would be corrupted. Two call sites need care: - delete_multiple_objects escapes the joined 'container/object' string, so the separator has to survive. It passes '/' as an extra excluded character, matching how Storage::Files#get_url already does this. - The compute service requests escaped their optional params by assigning back into the hash while iterating it, which mutated the caller's hash. transform_values escapes the values without that side effect. Also declares a development dependency on base64. It became a bundled gem in Ruby 3.4, and webmock requires it without depending on it, so without this neither the spec nor the unit suite can even load on Ruby 3.4 or later. Adds spec/escaping_spec.rb covering the escaping behaviour of each group of call sites, the space and separator cases that distinguish Fog::OpenStack.escape from CGI.escape, and a guard that URI.encode does not reappear anywhere in lib. Signed-off-by: Tim Smith --- fog-openstack.gemspec | 3 + .../compute/requests/delete_service.rb | 2 +- .../compute/requests/disable_service.rb | 2 +- .../requests/disable_service_log_reason.rb | 2 +- .../compute/requests/enable_service.rb | 2 +- .../requests/delete_multiple_objects.rb | 2 +- .../workflow/v2/requests/delete_action.rb | 2 +- .../v2/requests/delete_cron_trigger.rb | 2 +- .../v2/requests/delete_environment.rb | 2 +- .../workflow/v2/requests/delete_workbook.rb | 2 +- .../workflow/v2/requests/get_action.rb | 2 +- .../workflow/v2/requests/get_cron_trigger.rb | 2 +- .../workflow/v2/requests/get_environment.rb | 2 +- .../workflow/v2/requests/get_workbook.rb | 2 +- spec/escaping_spec.rb | 98 +++++++++++++++++++ 15 files changed, 114 insertions(+), 13 deletions(-) create mode 100644 spec/escaping_spec.rb 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