From 3db7a4eb365bd08a169ba510172cfd12be830d1f Mon Sep 17 00:00:00 2001 From: Jamie Tanna Date: Thu, 16 Apr 2020 17:59:11 +0100 Subject: [PATCH] Fix: URL Encode keys and values in URL encoded form data As noted in #687, the existing implementation of our querystring construction was not valid according to the HTTP/1.1 specification, which was noticed with an integration issue with Apache Tomcat. This affects both the generation of Query Strings, and URL encoded form parameters. The solution is to ensure that we always URL-encode the keys, as well as the values. It appears that the existing test we had for this, "returns a Rails style query string", was not actually quite testing what we thought it was, as we had not tested the rendering of the raw values, only the URL decoded ones. Closes #697. --- lib/httparty/hash_conversions.rb | 2 +- spec/httparty/hash_conversions_spec.rb | 10 +++++----- spec/httparty/request/body_spec.rb | 2 +- spec/httparty/request_spec.rb | 5 +++-- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/lib/httparty/hash_conversions.rb b/lib/httparty/hash_conversions.rb index 6e72664..2955868 100644 --- a/lib/httparty/hash_conversions.rb +++ b/lib/httparty/hash_conversions.rb @@ -27,7 +27,7 @@ module HTTParty normalized_keys = normalize_keys(key, value) normalized_keys.flatten.each_slice(2).inject('') do |string, (k, v)| - string + "#{k}=#{ERB::Util.url_encode(v.to_s)}&" + string + "#{ERB::Util.url_encode(k)}=#{ERB::Util.url_encode(v.to_s)}&" end end diff --git a/spec/httparty/hash_conversions_spec.rb b/spec/httparty/hash_conversions_spec.rb index 23606df..f4df4f5 100644 --- a/spec/httparty/hash_conversions_spec.rb +++ b/spec/httparty/hash_conversions_spec.rb @@ -11,13 +11,13 @@ RSpec.describe HTTParty::HashConversions do phones: ['111-111-1111', '222-222-2222'] } } - expect(HTTParty::HashConversions.to_params(hash)).to eq("name=bob&address[street]=111%20ruby%20ave.&address[city]=ruby%20central&address[phones][]=111-111-1111&address[phones][]=222-222-2222") + expect(HTTParty::HashConversions.to_params(hash)).to eq("name=bob&address%5Bstreet%5D=111%20ruby%20ave.&address%5Bcity%5D=ruby%20central&address%5Bphones%5D%5B%5D=111-111-1111&address%5Bphones%5D%5B%5D=222-222-2222") end context "nested params" do it 'creates a params string from a hash' do hash = { marketing_event: { marketed_resources: [ {type:"product", id: 57474842640 } ] } } - expect(HTTParty::HashConversions.to_params(hash)).to eq("marketing_event[marketed_resources][][type]=product&marketing_event[marketed_resources][][id]=57474842640") + expect(HTTParty::HashConversions.to_params(hash)).to eq("marketing_event%5Bmarketed_resources%5D%5B%5D%5Btype%5D=product&marketing_event%5Bmarketed_resources%5D%5B%5D%5Bid%5D=57474842640") end end end @@ -27,7 +27,7 @@ RSpec.describe HTTParty::HashConversions do it "creates a params string" do expect( HTTParty::HashConversions.normalize_param(:people, ["Bob Jones", "Mike Smith"]) - ).to eq("people[]=Bob%20Jones&people[]=Mike%20Smith&") + ).to eq("people%5B%5D=Bob%20Jones&people%5B%5D=Mike%20Smith&") end end @@ -35,7 +35,7 @@ RSpec.describe HTTParty::HashConversions do it "creates a params string" do expect( HTTParty::HashConversions.normalize_param(:people, []) - ).to eq("people[]=&") + ).to eq("people%5B%5D=&") end end @@ -43,7 +43,7 @@ RSpec.describe HTTParty::HashConversions do it "creates a params string" do expect( HTTParty::HashConversions.normalize_param(:person, { name: "Bob Jones" }) - ).to eq("person[name]=Bob%20Jones&") + ).to eq("person%5Bname%5D=Bob%20Jones&") end end diff --git a/spec/httparty/request/body_spec.rb b/spec/httparty/request/body_spec.rb index b62b2fd..4eb1e1a 100644 --- a/spec/httparty/request/body_spec.rb +++ b/spec/httparty/request/body_spec.rb @@ -15,7 +15,7 @@ RSpec.describe HTTParty::Request::Body do context 'when params is hash' do let(:params) { { people: ["Bob Jones", "Mike Smith"] } } - let(:converted_params) { "people[]=Bob%20Jones&people[]=Mike%20Smith"} + let(:converted_params) { "people%5B%5D=Bob%20Jones&people%5B%5D=Mike%20Smith"} it { is_expected.to eq converted_params } diff --git a/spec/httparty/request_spec.rb b/spec/httparty/request_spec.rb index 804d8ee..4b4c237 100644 --- a/spec/httparty/request_spec.rb +++ b/spec/httparty/request_spec.rb @@ -89,7 +89,7 @@ RSpec.describe HTTParty::Request do it "sets correct query string" do request = HTTParty::Request.new(Net::HTTP::Get, 'http://google.com', query: { fake_array: [] }) - expect(request.uri).to eq(URI.parse("http://google.com/?fake_array[]=")) + expect(request.uri).to eq(URI.parse("http://google.com/?fake_array%5B%5D=")) end end @@ -97,7 +97,7 @@ RSpec.describe HTTParty::Request do it "sets correct query" do request = HTTParty::Request.new(Net::HTTP::Get, 'http://google.com', query: { fake_array: [1] }) - expect(request.uri).to eq(URI.parse("http://google.com/?fake_array[]=1")) + expect(request.uri).to eq(URI.parse("http://google.com/?fake_array%5B%5D=1")) end end end @@ -327,6 +327,7 @@ RSpec.describe HTTParty::Request do it "returns a Rails style query string" do @request.options[:query] = {foo: %w(bar baz)} expect(CGI.unescape(@request.uri.query)).to eq("foo[]=bar&foo[]=baz") + expect(@request.uri.query).to eq("foo%5B%5D=bar&foo%5B%5D=baz") end end end