The problem is that URI.escape by default doesn't escape '+' (and some other characters). But some web framework (at least webrick) unescape the query string behind Puppet's back changing all '+' to spaces corrupting facts containing '+' characters (like base64 encoded values).
The current fix makes sure we use CGI.escape for all query string parameters. Indirection keys/path are still using URI escaping because this part of the URI format shouldn't be handled like query string parameters (otherwise '/' url separators are encoded which changes the uri path). Signed-off-by: Brice Figureau <[email protected]> --- lib/puppet/configurer/fact_handler.rb | 2 +- lib/puppet/indirector/request.rb | 9 +++++---- lib/puppet/network/http/handler.rb | 2 +- spec/unit/configurer/fact_handler.rb | 16 +++++++++++++--- spec/unit/indirector/request.rb | 12 ++++++------ spec/unit/network/http/mongrel/rest.rb | 6 +++--- spec/unit/network/http/rack/rest.rb | 8 ++++---- spec/unit/network/http/webrick/rest.rb | 8 ++++---- 8 files changed, 37 insertions(+), 26 deletions(-) diff --git a/lib/puppet/configurer/fact_handler.rb b/lib/puppet/configurer/fact_handler.rb index 8717649..43e9f35 100644 --- a/lib/puppet/configurer/fact_handler.rb +++ b/lib/puppet/configurer/fact_handler.rb @@ -33,7 +33,7 @@ module Puppet::Configurer::FactHandler text = facts.render(format) - return {:facts_format => format, :facts => URI.escape(text)} + return {:facts_format => format, :facts => CGI.escape(text)} end # Retrieve facts from the central server. diff --git a/lib/puppet/indirector/request.rb b/lib/puppet/indirector/request.rb index 2ffed60..d9e66cb 100644 --- a/lib/puppet/indirector/request.rb +++ b/lib/puppet/indirector/request.rb @@ -1,3 +1,4 @@ +require 'cgi' require 'uri' require 'puppet/indirector' @@ -115,9 +116,9 @@ class Puppet::Indirector::Request when nil; next when true, false; value = value.to_s when Fixnum, Bignum, Float; value = value # nothing - when String; value = URI.escape(value) - when Symbol; value = URI.escape(value.to_s) - when Array; value = URI.escape(YAML.dump(value)) + when String; value = CGI.escape(value) + when Symbol; value = CGI.escape(value.to_s) + when Array; value = CGI.escape(YAML.dump(value)) else raise ArgumentError, "HTTP REST queries cannot handle values of type '%s'" % value.class end @@ -159,7 +160,7 @@ class Puppet::Indirector::Request begin uri = URI.parse(URI.escape(key)) rescue => detail - raise ArgumentError, "Could not understand URL %s: %s" % [source, detail.to_s] + raise ArgumentError, "Could not understand URL %s: %s" % [key, detail.to_s] end # Just short-circuit these to full paths diff --git a/lib/puppet/network/http/handler.rb b/lib/puppet/network/http/handler.rb index 4df2c41..27e8dbd 100644 --- a/lib/puppet/network/http/handler.rb +++ b/lib/puppet/network/http/handler.rb @@ -208,7 +208,7 @@ module Puppet::Network::HTTP::Handler # in the query string, for security reasons. next result if param == :node next result if param == :ip - value = URI.unescape(value) + value = CGI.unescape(value) if value =~ /^---/ value = YAML.load(value) else diff --git a/spec/unit/configurer/fact_handler.rb b/spec/unit/configurer/fact_handler.rb index f615c08..0c4af95 100755 --- a/spec/unit/configurer/fact_handler.rb +++ b/spec/unit/configurer/fact_handler.rb @@ -95,10 +95,20 @@ describe Puppet::Configurer::FactHandler do end # I couldn't get marshal to work for this, only yaml, so we hard-code yaml. - it "should serialize and URI escape the fact values for uploading" do + it "should serialize and CGI escape the fact values for uploading" do facts = stub 'facts' facts.expects(:render).returns "my text" - text = URI.escape("my text") + text = CGI.escape("my text") + + @facthandler.expects(:find_facts).returns facts + + @facthandler.facts_for_uploading.should == {:facts_format => :yaml, :facts => text} + end + + it "should properly accept facts containing a '+'" do + facts = stub 'facts' + facts.expects(:render).returns "my+text" + text = "my%2Btext" @facthandler.expects(:find_facts).returns facts @@ -108,7 +118,7 @@ describe Puppet::Configurer::FactHandler do it "should hard-code yaml as the serialization" do facts = stub 'facts' facts.expects(:render).with(:yaml).returns "my text" - text = URI.escape("my text") + text = CGI.escape("my text") @facthandler.expects(:find_facts).returns facts diff --git a/spec/unit/indirector/request.rb b/spec/unit/indirector/request.rb index 47ffc31..848a608 100755 --- a/spec/unit/indirector/request.rb +++ b/spec/unit/indirector/request.rb @@ -282,20 +282,20 @@ describe Puppet::Indirector::Request do @request.query_string.should == "?one=1.2" end - it "should URI-escape all option values that are strings" do - escaping = URI.escape("one two") + it "should CGI-escape all option values that are strings" do + escaping = CGI.escape("one two") @request.stubs(:options).returns(:one => "one two") @request.query_string.should == "?one=#{escaping}" end - it "should YAML-dump and URI-escape arrays" do - escaping = URI.escape(YAML.dump(%w{one two})) + it "should YAML-dump and CGI-escape arrays" do + escaping = CGI.escape(YAML.dump(%w{one two})) @request.stubs(:options).returns(:one => %w{one two}) @request.query_string.should == "?one=#{escaping}" end - it "should convert to a string and URI-escape all option values that are symbols" do - escaping = URI.escape("sym bol") + it "should convert to a string and CGI-escape all option values that are symbols" do + escaping = CGI.escape("sym bol") @request.stubs(:options).returns(:one => :"sym bol") @request.query_string.should == "?one=#{escaping}" end diff --git a/spec/unit/network/http/mongrel/rest.rb b/spec/unit/network/http/mongrel/rest.rb index 01d561a..fb7e7e5 100755 --- a/spec/unit/network/http/mongrel/rest.rb +++ b/spec/unit/network/http/mongrel/rest.rb @@ -109,8 +109,8 @@ describe "Puppet::Network::HTTP::MongrelREST" do result[:bar].should == "xyzzy" end - it "should URI-decode the HTTP parameters" do - encoding = URI.escape("foo bar") + it "should CGI-decode the HTTP parameters" do + encoding = CGI.escape("foo bar") @request.expects(:params).returns('QUERY_STRING' => "foo=#{encoding}") result = @handler.params(@request) result[:foo].should == "foo bar" @@ -141,7 +141,7 @@ describe "Puppet::Network::HTTP::MongrelREST" do end it "should YAML-load and URI-decode values that are YAML-encoded" do - escaping = URI.escape(YAML.dump(%w{one two})) + escaping = CGI.escape(YAML.dump(%w{one two})) @request.expects(:params).returns('QUERY_STRING' => "foo=#{escaping}") result = @handler.params(@request) result[:foo].should == %w{one two} diff --git a/spec/unit/network/http/rack/rest.rb b/spec/unit/network/http/rack/rest.rb index 3a36a0b..126b301 100755 --- a/spec/unit/network/http/rack/rest.rb +++ b/spec/unit/network/http/rack/rest.rb @@ -84,8 +84,8 @@ describe "Puppet::Network::HTTP::RackREST" do result[:bar].should == "xyzzy" end - it "should URI-decode the HTTP parameters" do - encoding = URI.escape("foo bar") + it "should CGI-decode the HTTP parameters" do + encoding = CGI.escape("foo bar") req = mk_req("/?foo=#{encoding}") result = @handler.params(req) result[:foo].should == "foo bar" @@ -115,8 +115,8 @@ describe "Puppet::Network::HTTP::RackREST" do result[:foo].should == 1.5 end - it "should YAML-load and URI-decode values that are YAML-encoded" do - escaping = URI.escape(YAML.dump(%w{one two})) + it "should YAML-load and CGI-decode values that are YAML-encoded" do + escaping = CGI.escape(YAML.dump(%w{one two})) req = mk_req("/?foo=#{escaping}") result = @handler.params(req) result[:foo].should == %w{one two} diff --git a/spec/unit/network/http/webrick/rest.rb b/spec/unit/network/http/webrick/rest.rb index 5509905..f5c563e 100755 --- a/spec/unit/network/http/webrick/rest.rb +++ b/spec/unit/network/http/webrick/rest.rb @@ -85,8 +85,8 @@ describe Puppet::Network::HTTP::WEBrickREST do result[:bar].should == "xyzzy" end - it "should URI-decode the HTTP parameters" do - encoding = URI.escape("foo bar") + it "should CGI-decode the HTTP parameters" do + encoding = CGI.escape("foo bar") @request.expects(:query).returns('foo' => encoding) result = @handler.params(@request) result[:foo].should == "foo bar" @@ -104,8 +104,8 @@ describe Puppet::Network::HTTP::WEBrickREST do result[:foo].should be_false end - it "should YAML-load and URI-decode values that are YAML-encoded" do - escaping = URI.escape(YAML.dump(%w{one two})) + it "should YAML-load and CGI-decode values that are YAML-encoded" do + escaping = CGI.escape(YAML.dump(%w{one two})) @request.expects(:query).returns('foo' => escaping) result = @handler.params(@request) result[:foo].should == %w{one two} -- 1.6.0.2 --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en -~----------~----~----~----~------~----~------~--~---
