diff options
-rw-r--r-- | lib/chef/http.rb | 4 | ||||
-rw-r--r-- | lib/chef/http/decompressor.rb | 3 | ||||
-rw-r--r-- | lib/chef/http/simple.rb | 5 | ||||
-rw-r--r-- | lib/chef/http/validate_content_length.rb | 21 | ||||
-rw-r--r-- | lib/chef/rest.rb | 4 | ||||
-rw-r--r-- | spec/unit/http/simple_spec.rb | 32 | ||||
-rw-r--r-- | spec/unit/http/validate_content_length_spec.rb | 187 | ||||
-rw-r--r-- | spec/unit/rest_spec.rb | 10 |
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 |