CHEF-5100:

  - Enable Content Validation Check for Chef::HTTP::Simple
  - Tidy up debug logs / comments
  - More specs.

8 files changed, 247 insertions, 19 deletions

diff --git a/lib/chef/http.rb b/lib/chef/http.rb
index 06de37fc69..42b5decd6b 100644
--- a/lib/chef/http.rb
+++ b/lib/chef/http.rb
@@ -210,21 +210,18 @@ class Chef
 
     def apply_request_middleware(method, url, headers, data)
       middlewares.inject([method, url, headers, data]) do |req_data, middleware|
-        Chef::Log.debug "calling handle_request for the #{middleware.class} middleware"
         middleware.handle_request(*req_data)
       end
     end
 
     def apply_response_middleware(response, rest_request, return_value)
       middlewares.reverse.inject([response, rest_request, return_value]) do |res_data, middleware|
-        Chef::Log.debug "calling handle_response for the #{middleware.class} middleware"
         middleware.handle_response(*res_data)
       end
     end
 
     def apply_stream_complete_middleware(response, rest_request, return_value)
       middlewares.reverse.inject([response, rest_request, return_value]) do |res_data, middleware|
-        Chef::Log.debug "calling handle_stream_complete for the #{middleware.class} middleware"
         middleware.handle_stream_complete(*res_data)
       end
     end
@@ -396,4 +393,3 @@ class Chef
 
   end
 end
-
diff --git a/lib/chef/http/decompressor.rb b/lib/chef/http/decompressor.rb
index c760dad11c..7d4a5f61dd 100644
--- a/lib/chef/http/decompressor.rb
+++ b/lib/chef/http/decompressor.rb
@@ -26,7 +26,6 @@ class Chef
     class Decompressor
       class NoopInflater
         def inflate(chunk)
-          Chef::Log.debug "decompressing compressed chunk"
           chunk
         end
         alias :handle_chunk :inflate
@@ -138,5 +137,3 @@ class Chef
     end
   end
 end
-
-
diff --git a/lib/chef/http/simple.rb b/lib/chef/http/simple.rb
index 0ecb28846c..d675a17ee8 100644
--- a/lib/chef/http/simple.rb
+++ b/lib/chef/http/simple.rb
@@ -11,6 +11,11 @@ class Chef
       use Decompressor
       use CookieManager
 
+      # ValidateContentLength should come after Decompressor
+      # because the order of middlewares is reversed when handling
+      # responses.
+      use ValidateContentLength
+
     end
   end
 end
diff --git a/lib/chef/http/validate_content_length.rb b/lib/chef/http/validate_content_length.rb
index 88b3bc44d3..076194e31a 100644
--- a/lib/chef/http/validate_content_length.rb
+++ b/lib/chef/http/validate_content_length.rb
@@ -32,12 +32,10 @@ class Chef
         attr_accessor :content_length
 
         def initialize
-          Chef::Log.debug "initializing stream counter for response body size"
           @content_length = 0
         end
 
         def handle_chunk(chunk)
-          Chef::Log.debug "adding chunk to response body size: #{@content_length}"
           @content_length += chunk.bytesize
           chunk
         end
@@ -51,7 +49,7 @@ class Chef
       end
 
       def handle_response(http_response, rest_request, return_value)
-        validate(http_response, http_response.body.bytesize)
+        validate(http_response, http_response.body.bytesize) if http_response && http_response.body
         return [http_response, rest_request, return_value]
       end
 
@@ -61,7 +59,10 @@ class Chef
         else
           validate(http_response, @content_length_counter.content_length)
         end
-        @content_length_counter = nil  # XXX: quickfix to CHEF-5100
+
+        # Make sure the counter is reset since this object might get used
+        # again. See CHEF-5100
+        @content_length_counter = nil
         return [http_response, rest_request, return_value]
       end
 
@@ -72,6 +73,7 @@ class Chef
       private
 
       def response_content_length(response)
+        return nil if response['content-length'].nil?
         if response['content-length'].is_a?(Array)
           response['content-length'].first.to_i
         else
@@ -81,14 +83,8 @@ class Chef
 
       def validate(http_response, response_length)
         content_length    = response_content_length(http_response)
-        transfer_encoding = http_response['transfer_encoding']
-        content_encoding  = http_response['content_encoding']
-
-        Chef::Log.debug "Attempting to validate the Content-Length header of the response"
-        Chef::Log.debug "Content-Length header = #{content_length}"
-        Chef::Log.debug "Transfer-Encoding header = #{transfer_encoding}"
-        Chef::Log.debug "Content-Encoding header = #{content_encoding}"
-        Chef::Log.debug "Actual response body length = #{response_length}"
+        transfer_encoding = http_response['transfer-encoding']
+        content_encoding  = http_response['content-encoding']
 
         if content_length.nil?
           Chef::Log.debug "HTTP server did not include a Content-Length header in response, cannot identify truncated downloads."
@@ -106,6 +102,7 @@ class Chef
           raise Chef::Exceptions::ContentLengthMismatch.new(response_length, content_length)
         end
 
+        Chef::Log.debug "Content-Length validated correctly."
         true
       end
     end
diff --git a/lib/chef/rest.rb b/lib/chef/rest.rb
index 1c5ee6aa84..f0de443058 100644
--- a/lib/chef/rest.rb
+++ b/lib/chef/rest.rb
@@ -72,6 +72,10 @@ class Chef
       @middlewares << @decompressor
       @middlewares << @authenticator
       @middlewares << @request_id
+
+      # ValidateContentLength should come after Decompressor
+      # because the order of middlewares is reversed when handling
+      # responses.
       @middlewares << ValidateContentLength.new(options)
 
     end
diff --git a/spec/unit/http/simple_spec.rb b/spec/unit/http/simple_spec.rb
new file mode 100644
index 0000000000..b33ef1d553
--- /dev/null
+++ b/spec/unit/http/simple_spec.rb
@@ -0,0 +1,32 @@
+#
+# Author:: Serdar Sutay (<serdar@opscode.com>)
+# Copyright:: Copyright (c) 2014 Chef Software, Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+require 'spec_helper'
+
+describe Chef::HTTP::Simple do
+  it "should have content length validation middleware after compressor middleware" do
+    client = Chef::HTTP::Simple.new("dummy.com")
+    middlewares = client.instance_variable_get(:@middlewares)
+    content_length = middlewares.find_index { |e| e.is_a? Chef::HTTP::ValidateContentLength }
+    decompressor = middlewares.find_index { |e| e.is_a? Chef::HTTP::Decompressor }
+
+    content_length.should_not be_nil
+    decompressor.should_not be_nil
+    (decompressor < content_length).should be_true
+  end
+end
diff --git a/spec/unit/http/validate_content_length_spec.rb b/spec/unit/http/validate_content_length_spec.rb
new file mode 100644
index 0000000000..091f2b0757
--- /dev/null
+++ b/spec/unit/http/validate_content_length_spec.rb
@@ -0,0 +1,187 @@
+#
+# Author:: Serdar Sutay (<serdar@opscode.com>)
+# Copyright:: Copyright (c) 2014 Chef Software, Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+require 'spec_helper'
+require 'stringio'
+
+describe Chef::HTTP::ValidateContentLength do
+  class TestClient < Chef::HTTP
+    use Chef::HTTP::ValidateContentLength
+  end
+
+  let(:method) { "GET" }
+  let(:url) { "http://dummy.com" }
+  let(:headers) { {} }
+  let(:data) { false }
+
+  let(:request) { }
+  let(:return_value) { "200" }
+
+  # Test Variables
+  let(:request_type) { :streaming }
+  let(:content_length_value) { 23 }
+  let(:streaming_length) { 23 }
+  let(:response_body) { "Thanks for checking in." }
+  let(:response_headers) {
+    {
+      "content-length" => content_length_value
+    }
+  }
+
+  let(:response) {
+    m = double('HttpResponse', :body => response_body)
+    m.stub(:[]) do |key|
+      response_headers[key]
+    end
+
+    m
+  }
+
+  let(:middleware) {
+    client = TestClient.new(url)
+    client.middlewares[0]
+  }
+
+  def run_content_length_validation
+    stream_handler = middleware.stream_response_handler(response)
+    middleware.handle_request(method, url, headers, data)
+
+    case request_type
+    when :streaming
+      # First stream the data
+      data_length = streaming_length
+      while data_length > 0
+        chunk_size = data_length > 10 ? 10 : data_length
+        stream_handler.handle_chunk(double("Chunk", :bytesize => chunk_size))
+        data_length -= chunk_size
+      end
+
+      # Finally call stream complete
+      middleware.handle_stream_complete(response, request, return_value)
+    when :direct
+      middleware.handle_response(response, request, return_value)
+    else
+      raise "Unknown request_type: #{request_type}"
+    end
+  end
+
+  let(:debug_stream) { StringIO.new }
+  let(:debug_output) { debug_stream.string }
+
+  before(:each) {
+    Chef::Log.level = :debug
+    Chef::Log.stub(:debug) do |message|
+      debug_stream.puts message
+    end
+  }
+
+  describe "without response body" do
+    let(:request_type) { :direct }
+    let(:response_body) { "Thanks for checking in." }
+
+    it "shouldn't raise error" do
+      lambda { run_content_length_validation }.should_not raise_error
+    end
+  end
+
+  describe "without Content-Length header" do
+    let(:response_headers) { { } }
+
+    [ "direct", "streaming" ].each do |req_type|
+      describe "when running #{req_type} request" do
+        let(:request_type) { req_type.to_sym }
+
+        it "should skip validation and log for debug" do
+          run_content_length_validation
+          debug_output.should include("HTTP server did not include a Content-Length header in response")
+        end
+      end
+    end
+  end
+
+  describe "with correct Content-Length header" do
+    [ "direct", "streaming" ].each do |req_type|
+      describe "when running #{req_type} request" do
+        let(:request_type) { req_type.to_sym }
+
+        it "should validate correctly" do
+          run_content_length_validation
+          debug_output.should include("Content-Length validated correctly.")
+        end
+      end
+    end
+  end
+
+  describe "with wrong Content-Length header" do
+    let(:content_length_value) { 25 }
+    [ "direct", "streaming" ].each do |req_type|
+      describe "when running #{req_type} request" do
+        let(:request_type) { req_type.to_sym }
+
+        it "should raise ContentLengthMismatch error" do
+          lambda { run_content_length_validation }.should raise_error(Chef::Exceptions::ContentLengthMismatch)
+        end
+      end
+    end
+  end
+
+  describe "when download is interrupted" do
+    let(:streaming_length) { 12 }
+
+    it "should raise ContentLengthMismatch error" do
+      lambda { run_content_length_validation }.should raise_error(Chef::Exceptions::ContentLengthMismatch)
+    end
+  end
+
+  describe "when Transfer-Encoding & Content-Length is set" do
+    let(:response_headers) {
+      {
+        "content-length" => content_length_value,
+        "transfer-encoding" => "chunked"
+      }
+    }
+
+    [ "direct", "streaming" ].each do |req_type|
+      describe "when running #{req_type} request" do
+        let(:request_type) { req_type.to_sym }
+
+        it "should skip validation and log for debug" do
+          run_content_length_validation
+          debug_output.should include("Transfer-Encoding header is set, skipping Content-Length check.")
+        end
+      end
+    end
+  end
+
+  describe "when client is being reused" do
+    before do
+      run_content_length_validation
+      debug_output.should include("Content-Length validated correctly.")
+    end
+
+    it "should reset internal counter" do
+        middleware.instance_variable_get(:@content_length_counter).should be_nil
+    end
+
+    it "should validate correctly second time" do
+      run_content_length_validation
+      debug_output.should include("Content-Length validated correctly.")
+    end
+  end
+
+end
diff --git a/spec/unit/rest_spec.rb b/spec/unit/rest_spec.rb
index 8c83b34c9f..a8eb1ac7db 100644
--- a/spec/unit/rest_spec.rb
+++ b/spec/unit/rest_spec.rb
@@ -76,6 +76,16 @@ describe Chef::REST do
     Chef::Log.init(log_stringio)
   end
 
+  it "should have content length validation middleware after compressor middleware" do
+    middlewares = rest.instance_variable_get(:@middlewares)
+    content_length = middlewares.find_index { |e| e.is_a? Chef::HTTP::ValidateContentLength }
+    decompressor = middlewares.find_index { |e| e.is_a? Chef::HTTP::Decompressor }
+
+    content_length.should_not be_nil
+    decompressor.should_not be_nil
+    (decompressor < content_length).should be_true
+  end
+
   it "should allow the options hash to be frozen" do
     options = {}.freeze
     # should not raise any exception