Fix #2261 - Make sure query string parameters are properly escaped

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 <brice-puppet@daysofwonder.com>

8 files changed, 37 insertions, 26 deletions

diff --git a/lib/puppet/configurer/fact_handler.rb b/lib/puppet/configurer/fact_handler.rb
index 87176496c..43e9f35f4 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 2ffed60e2..d9e66cb5b 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 4df2c4141..27e8dbd1d 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 f615c0897..0c4af9554 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 47ffc319c..848a608a4 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 01d561a48..fb7e7e57d 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 3a36a0b67..126b30152 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 55099055c..f5c563e0d 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}