Fix #2516 - Fix format detection when content-type contains charset

Even though Puppet never transmist a charset information in its
response/request content-type, some proxy (especially Apache with the
infamous AddDefaultCharset configuration) may add this "incorrect"
information.

This patch makes sure that only the mime-type is used when looking
for the format associated with a response or a request.

The patch also provides a better error message when the client or server
code is fed with a request whose mime-type can not be mapped to a known
format.

It also fixes a typo noticed by the original reporter.

Signed-off-by: Brice Figureau <brice-puppet@daysofwonder.com>

6 files changed, 34 insertions, 3 deletions

diff --git a/lib/puppet/indirector/rest.rb b/lib/puppet/indirector/rest.rb
index 4d818c986..35847de88 100644
--- a/lib/puppet/indirector/rest.rb
+++ b/lib/puppet/indirector/rest.rb
@@ -41,11 +41,13 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus
                 raise "No content type in http response; cannot parse"
             end
 
+            content_type = response['content-type'].gsub(/\s*;.*$/,'') # strip any appended charset
+
             # Convert the response to a deserialized object.
             if multiple
-                model.convert_from_multiple(response['content-type'], response.body)
+                model.convert_from_multiple(content_type, response.body)
             else
-                model.convert_from(response['content-type'], response.body)
+                model.convert_from(content_type, response.body)
             end
         else
             # Raise the http error if we didn't get a 'success' of some kind.
diff --git a/lib/puppet/network/format_handler.rb b/lib/puppet/network/format_handler.rb
index 17b863fda..2ffbcef3d 100644
--- a/lib/puppet/network/format_handler.rb
+++ b/lib/puppet/network/format_handler.rb
@@ -81,9 +81,10 @@ module Puppet::Network::FormatHandler
             out = format
         when %r{\w+/\w+}
             out = mime(format)
-        when
+        else
             out = format(format)
         end
+        raise ArgumentError, "No format match the given format name or mime-type (%s)" % format if out.nil?
         out.name
     end
 
diff --git a/lib/puppet/network/http/handler.rb b/lib/puppet/network/http/handler.rb
index 27e8dbd1d..817661db1 100644
--- a/lib/puppet/network/http/handler.rb
+++ b/lib/puppet/network/http/handler.rb
@@ -42,7 +42,9 @@ module Puppet::Network::HTTP::Handler
 
     def request_format(request)
         if header = content_type_header(request)
+            header.gsub!(/\s*;.*$/,'') # strip any charset
             format = Puppet::Network::FormatHandler.mime(header)
+            raise "Client sent a mime-type (%s) that doesn't correspond to a format we support" % header if format.nil?
             return format.name.to_s if format.suitable?
         end
 
diff --git a/spec/unit/indirector/rest.rb b/spec/unit/indirector/rest.rb
index 696af9ae7..a352f2a2a 100755
--- a/spec/unit/indirector/rest.rb
+++ b/spec/unit/indirector/rest.rb
@@ -121,6 +121,17 @@ describe Puppet::Indirector::REST do
 
             @searcher.deserialize(response, true).should == "myobjects"
         end
+
+        it "should strip the content-type header to keep only the mime-type" do
+            @model.expects(:convert_from).with("text/plain", "mydata").returns "myobject"
+
+            response = mock 'response'
+            response.stubs(:[]).with("content-type").returns "text/plain; charset=utf-8"
+            response.stubs(:body).returns "mydata"
+            response.stubs(:code).returns "200"
+
+            @searcher.deserialize(response)
+        end
     end
 
     describe "when creating an HTTP client" do
diff --git a/spec/unit/network/format_handler.rb b/spec/unit/network/format_handler.rb
index 604ae1be3..8a79a58b3 100755
--- a/spec/unit/network/format_handler.rb
+++ b/spec/unit/network/format_handler.rb
@@ -87,6 +87,11 @@ describe Puppet::Network::FormatHandler do
         lambda { Puppet::Network::FormatHandler.protected_format(:myformat).render("foo") }.should raise_error(Puppet::Network::FormatHandler::FormatError)
     end
 
+    it "should raise an error if we couldn't find a format by name or mime-type" do
+        Puppet::Network::FormatHandler.stubs(:format).with(:myformat).returns nil
+        lambda { Puppet::Network::FormatHandler.protected_format(:myformat) }.should raise_error
+    end
+
     describe "when using formats" do
         before do
             @format = mock 'format'
diff --git a/spec/unit/network/http/handler.rb b/spec/unit/network/http/handler.rb
index a0c817361..1addb5890 100755
--- a/spec/unit/network/http/handler.rb
+++ b/spec/unit/network/http/handler.rb
@@ -129,6 +129,16 @@ describe Puppet::Network::HTTP::Handler do
             @handler.do_exception(@response, "A test", 404)
         end
 
+        it "should raise an error if the request is formatted in an unknown format" do
+            @handler.stubs(:content_type_header).returns "unknown format"
+            lambda { @handler.request_format(@request) }.should raise_error
+        end
+
+        it "should still find the correct format if content type contains charset information" do
+            @handler.stubs(:content_type_header).returns "text/plain; charset=UTF-8"
+            @handler.request_format(@request).should == "s"
+        end
+
         describe "when finding a model instance" do
             before do
                 @irequest = stub 'indirection_request', :method => :find, :indirection_name => "my_handler", :to_hash => {}, :key => "my_result", :model => @model_class