Adding clarity to query string handling in REST calls

We previously only handled simple strings as values,
but we know handle true and false as booleans, we URI-escape
all strings, and we can yaml-encode and then escape arrays of
strings.

This could get abused a bit, in that we're just yaml-dumping anything
that's an array, but it should be pretty safe.  Mmmm, should.

Signed-off-by: Luke Kanies <luke@madstop.com>

7 files changed, 152 insertions, 19 deletions

diff --git a/lib/puppet/indirector/rest.rb b/lib/puppet/indirector/rest.rb
index 5ac25f02d..2d0799286 100644
--- a/lib/puppet/indirector/rest.rb
+++ b/lib/puppet/indirector/rest.rb
@@ -87,11 +87,21 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus
         deserialize network(request).put("/#{indirection.name}/", request.instance.render, headers)
     end
 
-    private
-
     # Create the query string, if options are present.
     def query_string(request)
         return "" unless request.options and ! request.options.empty?
-        "?" + request.options.collect { |key, value| "%s=%s" % [key, value] }.join("&")
+        "?" + request.options.collect do |key, value|
+            case value
+            when nil; next
+            when true, false; value = value.to_s
+            when String; value = URI.escape(value)
+            when Symbol; value = URI.escape(value.to_s)
+            when Array; value = URI.escape(YAML.dump(value))
+            else
+                raise ArgumentError, "HTTP REST queries cannot handle values of type '%s'" % value.class
+            end
+
+            "%s=%s" % [key, value]
+        end.join("&")
     end
 end
diff --git a/lib/puppet/network/http/handler.rb b/lib/puppet/network/http/handler.rb
index 7af0528eb..9bc94a037 100644
--- a/lib/puppet/network/http/handler.rb
+++ b/lib/puppet/network/http/handler.rb
@@ -180,4 +180,19 @@ module Puppet::Network::HTTP::Handler
     def params(request)
         raise NotImplementedError
     end
+
+    def decode_params(params)
+        params.inject({}) do |result, ary|
+            param, value = ary
+            value = URI.unescape(value)
+            if value =~ /^---/
+                value = YAML.load(value)
+            else
+                value = true if value == "true"
+                value = false if value == "false"
+            end
+            result[param.to_sym] = value
+            result
+        end
+    end
 end
diff --git a/lib/puppet/network/http/mongrel/rest.rb b/lib/puppet/network/http/mongrel/rest.rb
index 45d21ea62..04f413937 100644
--- a/lib/puppet/network/http/mongrel/rest.rb
+++ b/lib/puppet/network/http/mongrel/rest.rb
@@ -23,7 +23,9 @@ class Puppet::Network::HTTP::MongrelREST < Mongrel::HttpHandler
     # Return the query params for this request.  We had to expose this method for
     # testing purposes.
     def params(request)
-        Mongrel::HttpRequest.query_parse(request.params["QUERY_STRING"]).merge(client_info(request))
+        params = Mongrel::HttpRequest.query_parse(request.params["QUERY_STRING"])
+        params = decode_params(params)
+        params.merge(client_info(request))
     end
 
     # what path was requested?
diff --git a/lib/puppet/network/http/webrick/rest.rb b/lib/puppet/network/http/webrick/rest.rb
index f06914365..e34f0d2e8 100644
--- a/lib/puppet/network/http/webrick/rest.rb
+++ b/lib/puppet/network/http/webrick/rest.rb
@@ -13,6 +13,7 @@ class Puppet::Network::HTTP::WEBrickREST < WEBrick::HTTPServlet::AbstractServlet
     # Retrieve the request parameters, including authentication information.
     def params(request)
         result = request.query
+        result = decode_params(result)
         result.merge(client_information(request))
     end
 
diff --git a/spec/unit/indirector/rest.rb b/spec/unit/indirector/rest.rb
index e7afbb5b2..aa6d51b6a 100755
--- a/spec/unit/indirector/rest.rb
+++ b/spec/unit/indirector/rest.rb
@@ -146,6 +146,66 @@ describe Puppet::Indirector::REST do
         end
     end
 
+    describe "when building a query string from request options" do
+        it "should return an empty query string if there are no options" do
+            @request.stubs(:options).returns nil
+            @searcher.query_string(@request).should == ""
+        end
+
+        it "should return an empty query string if the options are empty" do
+            @request.stubs(:options).returns({})
+            @searcher.query_string(@request).should == ""
+        end
+
+        it "should prefix the query string with '?'" do
+            @request.stubs(:options).returns(:one => "two")
+            @searcher.query_string(@request).should =~ /^\?/
+        end
+
+        it "should include all options in the query string, separated by '&'" do
+            @request.stubs(:options).returns(:one => "two", :three => "four")
+            @searcher.query_string(@request).sub(/^\?/, '').split("&").sort.should == %w{one=two three=four}.sort
+        end
+
+        it "should ignore nil options" do
+            @request.stubs(:options).returns(:one => "two", :three => nil)
+            @searcher.query_string(@request).should_not be_include("three")
+        end
+
+        it "should convert 'true' option values into strings" do
+            @request.stubs(:options).returns(:one => true)
+            @searcher.query_string(@request).should == "?one=true"
+        end
+
+        it "should convert 'false' option values into strings" do
+            @request.stubs(:options).returns(:one => false)
+            @searcher.query_string(@request).should == "?one=false"
+        end
+
+        it "should URI-escape all option values that are strings" do
+            escaping = URI.escape("one two")
+            @request.stubs(:options).returns(:one => "one two")
+            @searcher.query_string(@request).should == "?one=#{escaping}"
+        end
+
+        it "should YAML-dump and URI-escape arrays" do
+            escaping = URI.escape(YAML.dump(%w{one two}))
+            @request.stubs(:options).returns(:one => %w{one two})
+            @searcher.query_string(@request).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")
+            @request.stubs(:options).returns(:one => :"sym bol")
+            @searcher.query_string(@request).should == "?one=#{escaping}"
+        end
+
+        it "should fail if options other than booleans or strings are provided" do
+            @request.stubs(:options).returns(:one => {:one => :two})
+            lambda { @searcher.query_string(@request) }.should raise_error(ArgumentError)
+        end
+    end
+
     describe "when doing a find" do
         before :each do
             @connection = stub('mock http connection', :get => @response)
@@ -173,13 +233,6 @@ describe Puppet::Indirector::REST do
             @searcher.find(@request)
         end
 
-        it "should include all options in the query string" do
-            @request.stubs(:options).returns(:one => "two", :three => "four")
-            should_path = "/%s/%s" % [@indirection.name.to_s, "foo"]
-            @connection.expects(:get).with { |path, args| path =~ /\?one=two&three=four$/ or path =~ /\?three=four&one=two$/ }.returns(@response)
-            @searcher.find(@request)
-        end
-
         it "should provide an Accept header containing the list of supported formats joined with commas" do
             @connection.expects(:get).with { |path, args| args["Accept"] == "supported, formats" }.returns(@response)
 
diff --git a/spec/unit/network/http/mongrel/rest.rb b/spec/unit/network/http/mongrel/rest.rb
index 9dbc1c9ab..e72d4f540 100755
--- a/spec/unit/network/http/mongrel/rest.rb
+++ b/spec/unit/network/http/mongrel/rest.rb
@@ -97,16 +97,42 @@ describe "Puppet::Network::HTTP::MongrelREST" do
             end
         end
 
-        describe "and determining the request parameters", :shared => true do
+        describe "and determining the request parameters" do
             before do
                 @request.stubs(:params).returns({})
             end
 
-            it "should include the HTTP request parameters" do
+            it "should include the HTTP request parameters, with the keys as symbols" do
                 @request.expects(:params).returns('QUERY_STRING' => 'foo=baz&bar=xyzzy')
                 result = @handler.params(@request)
-                result["foo"].should == "baz"
-                result["bar"].should == "xyzzy"
+                result[:foo].should == "baz"
+                result[:bar].should == "xyzzy"
+            end
+
+            it "should URI-decode the HTTP parameters" do
+                encoding = URI.escape("foo bar")
+                @request.expects(:params).returns('QUERY_STRING' => "foo=#{encoding}")
+                result = @handler.params(@request)
+                result[:foo].should == "foo bar"
+            end
+
+            it "should convert the string 'true' to the boolean" do
+                @request.expects(:params).returns('QUERY_STRING' => 'foo=true')
+                result = @handler.params(@request)
+                result[:foo].should be_true
+            end
+
+            it "should convert the string 'false' to the boolean" do
+                @request.expects(:params).returns('QUERY_STRING' => 'foo=false')
+                result = @handler.params(@request)
+                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}))
+                @request.expects(:params).returns('QUERY_STRING' => "foo=#{escaping}")
+                result = @handler.params(@request)
+                result[:foo].should == %w{one two}
             end
 
             it "should pass the client's ip address to model find" do
diff --git a/spec/unit/network/http/webrick/rest.rb b/spec/unit/network/http/webrick/rest.rb
index 1f4ccbe29..b32cf49c5 100755
--- a/spec/unit/network/http/webrick/rest.rb
+++ b/spec/unit/network/http/webrick/rest.rb
@@ -83,11 +83,37 @@ describe Puppet::Network::HTTP::WEBrickREST do
         end
 
         describe "and determining the request parameters" do
-            it "should include the HTTP request parameters" do
-                @request.stubs(:query).returns(:foo => :baz, :bar => :xyzzy)
+            it "should include the HTTP request parameters, with the keys as symbols" do
+                @request.stubs(:query).returns("foo" => "baz", "bar" => "xyzzy")
                 result = @handler.params(@request)
-                result[:foo].should == :baz
-                result[:bar].should == :xyzzy
+                result[:foo].should == "baz"
+                result[:bar].should == "xyzzy"
+            end
+
+            it "should URI-decode the HTTP parameters" do
+                encoding = URI.escape("foo bar")
+                @request.expects(:query).returns('foo' => encoding)
+                result = @handler.params(@request)
+                result[:foo].should == "foo bar"
+            end
+
+            it "should convert the string 'true' to the boolean" do
+                @request.expects(:query).returns('foo' => "true")
+                result = @handler.params(@request)
+                result[:foo].should be_true
+            end
+
+            it "should convert the string 'false' to the boolean" do
+                @request.expects(:query).returns('foo' => "false")
+                result = @handler.params(@request)
+                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}))
+                @request.expects(:query).returns('foo' => escaping)
+                result = @handler.params(@request)
+                result[:foo].should == %w{one two}
             end
 
             it "should pass the client's ip address to model find" do