+1 On Jul 16, 2009, at 1:59 PM, Brice Figureau wrote:
> > 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 > > > > -- Finn's Law: Uncertainty is the final test of innovation. --------------------------------------------------------------------- Luke Kanies | http://reductivelabs.com | http://madstop.com --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
