From 1c0984eeb8335818716c1f6dbfd5604046752918 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Tue, 8 Sep 2026 16:26:05 -0400 Subject: [PATCH] Encode the id as a URL path segment, not a form value `Base.element_path` and `CustomMethods#custom_method_element_url` escaped `id` with `URI.encode_www_form_component`, an `application/x-www-form-urlencoded` encoder. A space became `+`, so `Person.find("ann mary")` requested `/people/ann+mary.json` and a backend reading the path as a path saw the id as `ann+mary`. The custom-method path had the same corruption. Escape with `ERB::Util.url_encode` at both sites, matching the prefix half. These were the only places in `lib/` where a runtime value was form-encoded into a path. Breaking: an id containing a space now goes on the wire as `%20` rather than `+`, and a literal `+` as `%2B`. Nothing raises; the request is simply made to a different URL. Backends that form-decode the path are unaffected. Applications that compensated for the old encoding must remove the workaround. --- lib/active_resource.rb | 1 + lib/active_resource/base.rb | 2 +- lib/active_resource/custom_methods.rb | 2 +- test/cases/base/custom_methods_test.rb | 15 +++++++++++++++ test/cases/base_test.rb | 7 ++++++- test/cases/finder_test.rb | 8 ++++++++ 6 files changed, 32 insertions(+), 3 deletions(-) diff --git a/lib/active_resource.rb b/lib/active_resource.rb index 42b074aa53..9b1ee04372 100644 --- a/lib/active_resource.rb +++ b/lib/active_resource.rb @@ -23,6 +23,7 @@ # WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. #++ +require "erb" require "uri" require "active_support" diff --git a/lib/active_resource/base.rb b/lib/active_resource/base.rb index 8ecd62718f..5e241eee92 100644 --- a/lib/active_resource/base.rb +++ b/lib/active_resource/base.rb @@ -883,7 +883,7 @@ def element_path(id, prefix_options = {}, query_options = nil) check_prefix_options(prefix_options) prefix_options, query_options = split_options(prefix_options) if query_options.nil? - "#{prefix(prefix_options)}#{collection_name}/#{URI.encode_www_form_component(id.to_s)}#{format_extension}#{query_string(query_options)}" + "#{prefix(prefix_options)}#{collection_name}/#{ERB::Util.url_encode(id.to_s)}#{format_extension}#{query_string(query_options)}" end # Gets the element url for the given ID in +id+. If the +query_options+ parameter is omitted, Rails diff --git a/lib/active_resource/custom_methods.rb b/lib/active_resource/custom_methods.rb index a35edacf3e..b5c77c0d21 100644 --- a/lib/active_resource/custom_methods.rb +++ b/lib/active_resource/custom_methods.rb @@ -140,7 +140,7 @@ def delete(method_name, options = {}) private def custom_method_element_url(method_name, options = {}) - "#{self.class.prefix(prefix_options)}#{self.class.collection_name}/#{URI.encode_www_form_component(id.to_s)}/#{method_name}#{self.class.format_extension}#{self.class.__send__(:query_string, options)}" + "#{self.class.prefix(prefix_options)}#{self.class.collection_name}/#{ERB::Util.url_encode(id.to_s)}/#{method_name}#{self.class.format_extension}#{self.class.__send__(:query_string, options)}" end def custom_method_new_element_url(method_name, options = {}) diff --git a/test/cases/base/custom_methods_test.rb b/test/cases/base/custom_methods_test.rb index 954de0204b..e65e93c0d4 100644 --- a/test/cases/base/custom_methods_test.rb +++ b/test/cases/base/custom_methods_test.rb @@ -212,6 +212,21 @@ def test_custom_element_method_identifier_encoding assert_equal ActiveResource::Response.new(luis, 204), Person.find("luís").put(:deactivate) end + + def test_custom_element_method_identifier_is_encoded_for_a_path_not_a_form + ann = { person: { id: "ann mary", name: "Ann Mary" } }.to_json + ann_plus = { person: { id: "ann+mary", name: "Ann Plus Mary" } }.to_json + + ActiveResource::HttpMock.respond_to do |mock| + mock.get "/people/ann%20mary.json", {}, ann + mock.put "/people/ann%20mary/deactivate.json", {}, ann, 204 + mock.get "/people/ann%2Bmary.json", {}, ann_plus + mock.put "/people/ann%2Bmary/deactivate.json", {}, ann_plus, 204 + end + + assert_equal ActiveResource::Response.new(ann, 204), Person.find("ann mary").put(:deactivate) + assert_equal ActiveResource::Response.new(ann_plus, 204), Person.find("ann+mary").put(:deactivate) + end end class SingletonCustomMethodsTest < ActiveSupport::TestCase diff --git a/test/cases/base_test.rb b/test/cases/base_test.rb index b19655fe5a..357e893435 100644 --- a/test/cases/base_test.rb +++ b/test/cases/base_test.rb @@ -854,7 +854,12 @@ def test_custom_element_path assert_equal "/people/1/addresses/1.json", StreetAddress.element_path(1, person_id: 1) assert_equal "/people/1/addresses/1.json", StreetAddress.element_path(1, "person_id" => 1) assert_equal "/people/Greg/addresses/1.json", StreetAddress.element_path(1, "person_id" => "Greg") - assert_equal "/people/ann%20mary/addresses/ann+mary.json", StreetAddress.element_path(:'ann mary', "person_id" => "ann mary") + assert_equal "/people/ann%20mary/addresses/ann%20mary.json", StreetAddress.element_path(:'ann mary', "person_id" => "ann mary") + end + + def test_id_is_encoded_for_a_path_not_a_form + assert_equal "/people/ann%20mary.json", Person.element_path("ann mary") + assert_equal "/people/ann%2Bmary.json", Person.element_path("ann+mary") end def test_custom_element_path_without_required_prefix_param diff --git a/test/cases/finder_test.rb b/test/cases/finder_test.rb index 10326a1c65..4367648977 100644 --- a/test/cases/finder_test.rb +++ b/test/cases/finder_test.rb @@ -352,6 +352,14 @@ def test_find_identifier_encoding assert_equal "David", david.name end + def test_find_identifier_encoding_for_space + ActiveResource::HttpMock.respond_to { |m| m.get "/people/ann%20mary.json", {}, @david } + + david = Person.find("ann mary") + + assert_equal "David", david.name + end + def test_find_identifier_encoding_for_path_traversal ActiveResource::HttpMock.respond_to { |m| m.get "/people/..%2F.json", {}, @david }