Correctly handling URI escaping throughout the REST process

This means, at the least, that we can now serve files
via REST when they have spaces and other weird characters
in their names.

This involves a small change to many files.

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

13 files changed, 57 insertions, 97 deletions

diff --git a/lib/puppet/file_serving/configuration.rb b/lib/puppet/file_serving/configuration.rb
index de5995d7e..608924c8b 100644
--- a/lib/puppet/file_serving/configuration.rb
+++ b/lib/puppet/file_serving/configuration.rb
@@ -9,10 +9,8 @@ require 'puppet/file_serving/mount/file'
 require 'puppet/file_serving/mount/modules'
 require 'puppet/file_serving/mount/plugins'
 require 'puppet/util/cacher'
-require 'puppet/util/uri_helper'
 
 class Puppet::FileServing::Configuration
-    include Puppet::Util::URIHelper
     require 'puppet/file_serving/configuration/parser'
 
     class << self
@@ -70,9 +68,7 @@ class Puppet::FileServing::Configuration
         # Reparse the configuration if necessary.
         readconfig
 
-        uri = key2uri(request.key)
-
-        mount_name, path = uri.path.sub(/^\//, '').split(File::Separator, 2)
+        mount_name, path = request.key.split(File::Separator, 2)
 
         raise(ArgumentError, "Cannot find file: Invalid path '%s'" % mount_name) unless mount_name =~ %r{^[-\w]+$}
 
diff --git a/lib/puppet/indirector/direct_file_server.rb b/lib/puppet/indirector/direct_file_server.rb
index bcda92366..f69f9e14b 100644
--- a/lib/puppet/indirector/direct_file_server.rb
+++ b/lib/puppet/indirector/direct_file_server.rb
@@ -3,12 +3,10 @@
 #  Copyright (c) 2007. All rights reserved.
 
 require 'puppet/file_serving/terminus_helper'
-require 'puppet/util/uri_helper'
 require 'puppet/indirector/terminus'
 
 class Puppet::Indirector::DirectFileServer < Puppet::Indirector::Terminus
 
-    include Puppet::Util::URIHelper
     include Puppet::FileServing::TerminusHelper
 
     def find(request)
diff --git a/lib/puppet/indirector/file_content/rest.rb b/lib/puppet/indirector/file_content/rest.rb
index 31df7626d..7b3cade8e 100644
--- a/lib/puppet/indirector/file_content/rest.rb
+++ b/lib/puppet/indirector/file_content/rest.rb
@@ -3,7 +3,6 @@
 #  Copyright (c) 2007. All rights reserved.
 
 require 'puppet/file_serving/content'
-require 'puppet/util/uri_helper'
 require 'puppet/indirector/file_content'
 require 'puppet/indirector/rest'
 
diff --git a/lib/puppet/indirector/file_metadata/rest.rb b/lib/puppet/indirector/file_metadata/rest.rb
index 0f3d9c6fd..8cbf91049 100644
--- a/lib/puppet/indirector/file_metadata/rest.rb
+++ b/lib/puppet/indirector/file_metadata/rest.rb
@@ -3,7 +3,6 @@
 #  Copyright (c) 2007. All rights reserved.
 
 require 'puppet/file_serving/metadata'
-require 'puppet/util/uri_helper'
 require 'puppet/indirector/file_metadata'
 require 'puppet/indirector/rest'
 
diff --git a/lib/puppet/indirector/request.rb b/lib/puppet/indirector/request.rb
index c6f268ab2..539cd0e62 100644
--- a/lib/puppet/indirector/request.rb
+++ b/lib/puppet/indirector/request.rb
@@ -1,3 +1,4 @@
+require 'uri'
 require 'puppet/indirector'
 
 # This class encapsulates all of the information you need to make an
@@ -14,6 +15,10 @@ class Puppet::Indirector::Request
         ! ! authenticated
     end
 
+    def escaped_key
+        URI.escape(key)
+    end
+
     # LAK:NOTE This is a messy interface to the cache, and it's only
     # used by the Configurer class.  I decided it was better to implement
     # it now and refactor later, when we have a better design, than
@@ -96,7 +101,7 @@ class Puppet::Indirector::Request
 
         # Just short-circuit these to full paths
         if uri.scheme == "file"
-            @key = uri.path
+            @key = URI.unescape(uri.path)
             return
         end
 
@@ -111,6 +116,6 @@ class Puppet::Indirector::Request
         end
 
         @protocol = uri.scheme
-        @key = uri.path.sub(/^\//, '')
+        @key = URI.unescape(uri.path.sub(/^\//, ''))
     end
 end
diff --git a/lib/puppet/indirector/rest.rb b/lib/puppet/indirector/rest.rb
index 2d0799286..ce459b905 100644
--- a/lib/puppet/indirector/rest.rb
+++ b/lib/puppet/indirector/rest.rb
@@ -62,12 +62,12 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus
     end
 
     def find(request)
-        deserialize network(request).get("/#{indirection.name}/#{request.key}#{query_string(request)}", headers)
+        deserialize network(request).get("/#{indirection.name}/#{request.escaped_key}#{query_string(request)}", headers)
     end
     
     def search(request)
         if request.key
-            path = "/#{indirection.name}s/#{request.key}#{query_string(request)}"
+            path = "/#{indirection.name}s/#{request.escaped_key}#{query_string(request)}"
         else
             path = "/#{indirection.name}s#{query_string(request)}"
         end
@@ -79,7 +79,7 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus
     
     def destroy(request)
         raise ArgumentError, "DELETE does not accept options" unless request.options.empty?
-        deserialize network(request).delete("/#{indirection.name}/#{request.key}", headers)
+        deserialize network(request).delete("/#{indirection.name}/#{request.escaped_key}", headers)
     end
     
     def save(request)
diff --git a/lib/puppet/network/http/handler.rb b/lib/puppet/network/http/handler.rb
index 9bc94a037..cce679864 100644
--- a/lib/puppet/network/http/handler.rb
+++ b/lib/puppet/network/http/handler.rb
@@ -75,6 +75,7 @@ module Puppet::Network::HTTP::Handler
     # Execute our find.
     def do_find(request, response)
         key = request_key(request) || raise(ArgumentError, "Could not locate lookup key in request path [#{path(request)}]")
+        key = URI.unescape(key)
         args = params(request)
         unless result = model.find(key, args)
             return do_exception(response, "Could not find %s %s" % [model.name, key], 404)
@@ -93,6 +94,7 @@ module Puppet::Network::HTTP::Handler
     def do_search(request, response)
         args = params(request)
         if key = request_key(request)
+            key = URI.unescape(key)
             result = model.search(key, args)
         else
             result = model.search(args)
@@ -110,6 +112,7 @@ module Puppet::Network::HTTP::Handler
     # Execute our destroy.
     def do_destroy(request, response)
         key = request_key(request) || raise(ArgumentError, "Could not locate lookup key in request path [#{path(request)}]")
+        key = URI.unescape(key)
         args = params(request)
         result = model.destroy(key, args)
 
diff --git a/lib/puppet/util/uri_helper.rb b/lib/puppet/util/uri_helper.rb
deleted file mode 100644
index cb9320387..000000000
--- a/lib/puppet/util/uri_helper.rb
+++ /dev/null
@@ -1,22 +0,0 @@
-#
-#  Created by Luke Kanies on 2007-10-16.
-#  Copyright (c) 2007. All rights reserved.
-
-require 'uri'
-require 'puppet/util'
-
-# Helper methods for dealing with URIs.
-module Puppet::Util::URIHelper
-    def key2uri(key)
-        # Return it directly if it's fully qualified.
-        if key =~ /^#{::File::SEPARATOR}/
-            key = "file://" + key
-        end
-
-        begin
-            uri = URI.parse(URI.escape(key))
-        rescue => detail
-            raise ArgumentError, "Could not understand URI %s: %s" % [key, detail.to_s]
-        end
-    end
-end
diff --git a/spec/unit/file_serving/configuration.rb b/spec/unit/file_serving/configuration.rb
index c2d2e5014..e62423caf 100755
--- a/spec/unit/file_serving/configuration.rb
+++ b/spec/unit/file_serving/configuration.rb
@@ -134,7 +134,7 @@ describe Puppet::FileServing::Configuration do
             @config = Puppet::FileServing::Configuration.create
             @config.stubs(:find_mount)
 
-            @request = stub 'request', :key => "puppet:///foo/bar/baz", :options => {}
+            @request = stub 'request', :key => "foo/bar/baz", :options => {}
         end
 
         it "should reread the configuration" do
@@ -150,13 +150,13 @@ describe Puppet::FileServing::Configuration do
         end
 
         it "should fail if the mount name is not alpha-numeric" do
-            @request.expects(:key).returns "puppet:///foo&bar/asdf"
+            @request.expects(:key).returns "foo&bar/asdf"
 
             lambda { @config.split_path(@request) }.should raise_error(ArgumentError)
         end
 
         it "should support dashes in the mount name" do
-            @request.expects(:key).returns "puppet:///foo-bar/asdf"
+            @request.expects(:key).returns "foo-bar/asdf"
 
             lambda { @config.split_path(@request) }.should_not raise_error(ArgumentError)
         end
@@ -182,7 +182,7 @@ describe Puppet::FileServing::Configuration do
         end
 
         it "should remove any double slashes" do
-            @request.stubs(:key).returns "puppet:///foo/bar//baz"
+            @request.stubs(:key).returns "foo/bar//baz"
             mount = stub 'mount', :name => "foo"
             @config.expects(:find_mount).returns mount
 
@@ -190,7 +190,7 @@ describe Puppet::FileServing::Configuration do
         end
 
         it "should return the relative path as nil if it is an empty string" do
-            @request.expects(:key).returns "puppet:///foo"
+            @request.expects(:key).returns "foo"
             mount = stub 'mount', :name => "foo"
             @config.expects(:find_mount).returns mount
 
@@ -198,7 +198,7 @@ describe Puppet::FileServing::Configuration do
         end
 
         it "should add 'modules/' to the relative path if the modules mount is used but not specified, for backward compatibility" do
-            @request.expects(:key).returns "puppet:///foo/bar"
+            @request.expects(:key).returns "foo/bar"
             mount = stub 'mount', :name => "modules"
             @config.expects(:find_mount).returns mount
 
diff --git a/spec/unit/indirector/request.rb b/spec/unit/indirector/request.rb
index dc1e4e39d..fc3ed44e6 100755
--- a/spec/unit/indirector/request.rb
+++ b/spec/unit/indirector/request.rb
@@ -79,10 +79,12 @@ describe Puppet::Indirector::Request do
         describe "and the request key is a URI" do
             describe "and the URI is a 'file' URI" do
                 before do
-                    @request = Puppet::Indirector::Request.new(:ind, :method, "file:///my/file")
+                    @request = Puppet::Indirector::Request.new(:ind, :method, "file:///my/file with spaces")
                 end
 
-                it "should set the request key to the full file path" do @request.key.should == "/my/file" end
+                it "should set the request key to the unescaped full file path" do
+                    @request.key.should == "/my/file with spaces"
+                end
 
                 it "should not set the protocol" do
                     @request.protocol.should be_nil
@@ -118,8 +120,8 @@ describe Puppet::Indirector::Request do
                 Puppet::Indirector::Request.new(:ind, :method, "http://host/stuff").port.should == 80
             end
 
-            it "should set the request key to the unqualified path from the URI" do
-                Puppet::Indirector::Request.new(:ind, :method, "http:///stuff").key.should == "stuff"
+            it "should set the request key to the unescaped unqualified path from the URI" do
+                Puppet::Indirector::Request.new(:ind, :method, "http:///stuff with spaces").key.should == "stuff with spaces"
             end
 
             it "should set the :uri attribute to the full URI" do
@@ -171,4 +173,8 @@ describe Puppet::Indirector::Request do
     it "should use its indirection name and key, if it has no uri, as its string representation" do
         Puppet::Indirector::Request.new(:myind, :find, "key") == "/myind/key"
     end
+
+    it "should be able to return the URI-escaped key" do
+        Puppet::Indirector::Request.new(:myind, :find, "my key").escaped_key.should == URI.escape("my key")
+    end
 end
diff --git a/spec/unit/indirector/rest.rb b/spec/unit/indirector/rest.rb
index aa6d51b6a..4fa30b20a 100755
--- a/spec/unit/indirector/rest.rb
+++ b/spec/unit/indirector/rest.rb
@@ -211,7 +211,8 @@ describe Puppet::Indirector::REST do
             @connection = stub('mock http connection', :get => @response)
             @searcher.stubs(:network).returns(@connection)    # neuter the network connection
 
-            @request = stub 'request', :key => 'foo', :options => {}
+            # Use a key with spaces, so we can test escaping
+            @request = stub 'request', :escaped_key => 'foo', :options => {}
         end
 
         it "should call the GET http method on a network connection" do
@@ -227,7 +228,7 @@ describe Puppet::Indirector::REST do
             @searcher.find(@request).should == 'myobject'
         end        
 
-        it "should use the indirection name and request key to create the path" do
+        it "should use the indirection name and escaped request key to create the path" do
             should_path = "/%s/%s" % [@indirection.name.to_s, "foo"]
             @connection.expects(:get).with { |path, args| path == should_path }.returns(@response)
             @searcher.find(@request)
@@ -258,7 +259,7 @@ describe Puppet::Indirector::REST do
 
             @model.stubs(:convert_from_multiple)
 
-            @request = stub 'request', :key => 'foo', :options => {}
+            @request = stub 'request', :escaped_key => 'foo', :options => {}, :key => "bar"
         end
 
         it "should call the GET http method on a network connection" do
@@ -281,7 +282,7 @@ describe Puppet::Indirector::REST do
             @searcher.search(@request)
         end
 
-        it "should use the plural indirection name and request key to create the path if the request key is set" do
+        it "should use the plural indirection name and escaped request key to create the path if the request key is set" do
             should_path = "/%ss/%s" % [@indirection.name.to_s, "foo"]
             @connection.expects(:get).with { |path, args| path == should_path }.returns(@response)
             @searcher.search(@request)
@@ -319,7 +320,7 @@ describe Puppet::Indirector::REST do
             @connection = stub('mock http connection', :delete => @response)
             @searcher.stubs(:network).returns(@connection)    # neuter the network connection
 
-            @request = stub 'request', :key => 'foo', :options => {}
+            @request = stub 'request', :escaped_key => 'foo', :options => {}
         end
 
         it "should call the DELETE http method on a network connection" do
@@ -341,7 +342,7 @@ describe Puppet::Indirector::REST do
             @searcher.destroy(@request).should == 'myobject'
         end        
 
-        it "should use the indirection name and request key to create the path" do
+        it "should use the indirection name and escaped request key to create the path" do
             should_path = "/%s/%s" % [@indirection.name.to_s, "foo"]
             @connection.expects(:delete).with { |path, args| path == should_path }.returns(@response)
             @searcher.destroy(@request)
diff --git a/spec/unit/network/http/handler.rb b/spec/unit/network/http/handler.rb
index ed0f25121..6f1a57b8b 100755
--- a/spec/unit/network/http/handler.rb
+++ b/spec/unit/network/http/handler.rb
@@ -229,12 +229,20 @@ describe Puppet::Network::HTTP::Handler do
                 Puppet::Network::FormatHandler.stubs(:format).returns @format
             end
 
-            it "should fail to find model if key is not specified" do
+            it "should fail if the key is not specified" do
                 @handler.stubs(:request_key).returns(nil)
 
                 lambda { @handler.do_find(@request, @response) }.should raise_error(ArgumentError)
             end
 
+            it "should use the escaped request key" do
+                @handler.stubs(:request_key).returns URI.escape("my key")
+                @model_class.expects(:find).with do |key, args|
+                    key == "my key"
+                end.returns @result
+                @handler.do_find(@request, @response)
+            end
+
             it "should use a common method for determining the request parameters" do
                 @handler.stubs(:params).returns(:foo => :baz, :bar => :xyzzy)
                 @model_class.expects(:find).with do |key, args|
@@ -336,9 +344,9 @@ describe Puppet::Network::HTTP::Handler do
                 @handler.do_search(@request, @response)
             end
 
-            it "should use a request key if one is provided" do
-                @handler.expects(:request_key).with(@request).returns "foo"
-                @model_class.expects(:search).with { |key, args| key == "foo" }.returns @result
+            it "should use an escaped request key if one is provided" do
+                @handler.expects(:request_key).with(@request).returns URI.escape("foo bar")
+                @model_class.expects(:search).with { |key, args| key == "foo bar" }.returns @result
                 @handler.do_search(@request, @response)
             end
 
@@ -402,6 +410,14 @@ describe Puppet::Network::HTTP::Handler do
                 lambda { @handler.do_destroy(@request, @response) }.should raise_error(ArgumentError)
             end
 
+            it "should use the escaped request key to destroy the instance in the model" do
+                @handler.expects(:request_key).returns URI.escape("foo bar")
+                @model_class.expects(:destroy).with do |key, args|
+                    key == "foo bar"
+                end
+                @handler.do_destroy(@request, @response)
+            end
+
             it "should use a common method for determining the request parameters" do
                 @handler.stubs(:params).returns(:foo => :baz, :bar => :xyzzy)
                 @model_class.expects(:destroy).with do |key, args|
diff --git a/spec/unit/util/uri_helper.rb b/spec/unit/util/uri_helper.rb
deleted file mode 100755
index f454a2ced..000000000
--- a/spec/unit/util/uri_helper.rb
+++ /dev/null
@@ -1,41 +0,0 @@
-#!/usr/bin/env ruby
-#
-#  Created by Luke Kanies on 2007-10-18.
-#  Copyright (c) 2007. All rights reserved.
-
-require File.dirname(__FILE__) + '/../../spec_helper'
-
-require 'puppet/util/uri_helper'
-
-describe Puppet::Util::URIHelper, " when converting a key to a URI" do
-    before do
-        @helper = Object.new
-        @helper.extend(Puppet::Util::URIHelper)
-    end
-
-    it "should return the URI instance" do
-        URI.expects(:parse).with("file:///myhost/blah").returns(:myuri)
-        @helper.key2uri("/myhost/blah").should == :myuri
-    end
-
-    it "should escape the key before parsing" do
-        URI.expects(:escape).with("mykey").returns("http://myhost/blah")
-        URI.expects(:parse).with("http://myhost/blah").returns(:myuri)
-        @helper.key2uri("mykey").should == :myuri
-    end
-
-    it "should use the URI class to parse the key" do
-        URI.expects(:parse).with("http://myhost/blah").returns(:myuri)
-        @helper.key2uri("http://myhost/blah").should == :myuri
-    end
-
-    it "should set the scheme to 'file' if the key is a fully qualified path" do
-        URI.expects(:parse).with("file:///myhost/blah").returns(:myuri)
-        @helper.key2uri("/myhost/blah").should == :myuri
-    end
-
-    it "should set the host to 'nil' if the key is a fully qualified path" do
-        URI.expects(:parse).with("file:///myhost/blah").returns(:myuri)
-        @helper.key2uri("/myhost/blah").should == :myuri
-    end
-end