diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 2d73d211..515959c0 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -67,6 +67,9 @@ class ApplicationController < ActionController::Base (session[:last_seen_at].blank? || Time.zone.at(session[:last_seen_at].to_i) < 10.minutes.ago) } + # We've had headers misbehave. Validating them here means we can email someone about the problem! + after_action :validate_headers + # A dummy method to get rid of all the Rubymine errors. # @return [User] def current_user @@ -565,4 +568,13 @@ def should_check_authorization? !is_devise_controller && !is_admin_controller end + def validate_headers + if response.headers.has_key?('Content-Length') && response.headers['Content-Length'].to_i < 0 + raise CustomErrors::BadHeaderError + end + if response.headers.has_key?(:content_length) && response.headers[:content_length].to_i < 0 + raise CustomErrors::BadHeaderError + end + end + end diff --git a/app/models/range_request.rb b/app/models/range_request.rb index b695394b..9f41e131 100644 --- a/app/models/range_request.rb +++ b/app/models/range_request.rb @@ -29,6 +29,7 @@ module Constants MULTIPART_HEADER_LENGTH = 49 MULTIPART_DASH_LINE_BREAK_LENGTH = 8 CONVERT_INDEX_TO_LENGTH = 1 + CONVERT_LENGTH_TO_INDEX = -1 REQUIRED_PARAMETERS = [:start_offset, :end_offset, :recorded_date, :site_id, :site_name, :ext, :file_path, :media_type] @@ -303,7 +304,7 @@ def response_info(options, rails_request) range_end_bytes: [], # the largest possible value for range_end_bytes - range_end_bytes_max: file_size - CONVERT_INDEX_TO_LENGTH, + range_end_bytes_max: file_size + CONVERT_LENGTH_TO_INDEX, range_length_max: @max_range_size, write_buffer_size: @write_buffer_size, @@ -359,6 +360,15 @@ def response_range(info) # # Can also have multiple ranges delimited by commas, as in: # Range: bytes=0-500,600-1000 * Get bytes 0-500 (the first 501 bytes), inclusive plus bytes 600-1000 (401 bytes) inclusive + # + # https://tools.ietf.org/html/rfc7233#page-4 + # Byte offsets (start and end) should be inclusive + # Byte offsets start at 0 + # e.g. + # The first 500 bytes (byte offsets 0-499, inclusive): + # bytes=0-499 + # The second 500 bytes (byte offsets 500-999, inclusive): + # bytes=500-999 return_value[:range_start_bytes] = [] return_value[:range_end_bytes] = [] @@ -368,31 +378,46 @@ def response_range(info) start_range = current_range[start_index] end_range = current_range[end_index] + # e.g. "-", "" + # NB: these technically aren't legal forms (as far as I can tell) if start_range.blank? && end_range.blank? # default to 0 - @max_range_size (or whatever is available) of file start_range = info[:range_start_bytes_min] - end_range = [@max_range_size, info[:range_end_bytes_max]].min + end_range = [@max_range_size + CONVERT_LENGTH_TO_INDEX, info[:range_end_bytes_max]].min + # e.g. "0-1", "0-500", "400-1000" elsif !start_range.blank? && !end_range.blank? # both supplied start_range = start_range.to_i end_range = end_range.to_i + # e.g. "500-", "0-" elsif !start_range.blank? && end_range.blank? # given a start but no end, get the smallest of remaining length and @max_range_size start_range = start_range.to_i - end_range = info[:range_end_bytes_max] - start_range.to_i + end_range = [start_range + @max_range_size + CONVERT_LENGTH_TO_INDEX, info[:range_end_bytes_max]].min + # https://tools.ietf.org/html/rfc7233#page-5 + # assuming a representation of length 10000: + # The final 500 bytes (byte offsets 9500-9999, inclusive): + # bytes=-500 + # e.g. "-200" elsif start_range.blank? && !end_range.blank? # No beginning specified, get last n bytes of file - start_range = info[:range_end_bytes_max] - [end_range.to_i, @max_range_size].min + start_range = info[:range_end_bytes_max] + CONVERT_INDEX_TO_LENGTH - [end_range.to_i, @max_range_size].min end_range = info[:range_end_bytes_max] end start_range = info[:range_start_bytes_min] if start_range < info[:range_start_bytes_min] end_range = info[:range_end_bytes_max] if end_range > info[:range_end_bytes_max] - end_range = start_range + @max_range_size if (end_range - start_range) > @max_range_size + # e.g. bytes=0-499, max_range_size=500 => 499 - 0 + 1 = 500 > 500 + if (end_range - start_range + CONVERT_INDEX_TO_LENGTH) > @max_range_size + fail CustomErrors::BadRequestError, 'The requested range exceeded the maximum allowed.' + end + if start_range > end_range + fail CustomErrors::BadRequestError, 'The requested range specified a first byte that was greater than the last byte.' + end return_value[:range_start_bytes].push(start_range) return_value[:range_end_bytes].push(end_range) diff --git a/lib/modules/custom_errors.rb b/lib/modules/custom_errors.rb index c1e52eda..0bb0e209 100644 --- a/lib/modules/custom_errors.rb +++ b/lib/modules/custom_errors.rb @@ -5,6 +5,7 @@ class ItemNotFoundError < StandardError; end class TooManyItemsFoundError < StandardError; end class AnalysisJobStartError < StandardError; end class OrphanedSiteError < StandardError; end + class BadHeaderError < StandardError; end class RequestedMediaTypeError < StandardError attr_reader :available_formats_info def initialize(message = nil, available_formats_info = nil) diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb new file mode 100644 index 00000000..dcdebf7e --- /dev/null +++ b/spec/controllers/application_controller_spec.rb @@ -0,0 +1,21 @@ +require "rails_helper" + + +RSpec.describe ApplicationController, type: :controller do + controller do + skip_authorization_check + + def index + response.headers['Content-Length'] = -100 + render text: "test response" + end + end + + describe "Application wide tests" do + it "it fails if content-length is negative" do + expect { + get :index + }.to raise_error(CustomErrors::BadHeaderError) + end + end +end \ No newline at end of file diff --git a/spec/media_tools/test-audio-mono-long.ogg b/spec/media_tools/test-audio-mono-long.ogg new file mode 100644 index 00000000..e1cd7b99 Binary files /dev/null and b/spec/media_tools/test-audio-mono-long.ogg differ diff --git a/spec/models/range_request_spec.rb b/spec/models/range_request_spec.rb index f4b5b8dd..28ec1685 100644 --- a/spec/models/range_request_spec.rb +++ b/spec/models/range_request_spec.rb @@ -13,6 +13,14 @@ Digest::SHA256.hexdigest etag_string } + let(:audio_file_mono_long) { File.expand_path(File.join(File.dirname(__FILE__), '..', 'media_tools', 'test-audio-mono-long.ogg')) } + let(:audio_file_mono_media_type_long) { Mime::Type.lookup('audio/ogg') } + let(:audio_file_mono_size_bytes_long) { 1317526 } + let(:audio_file_mono_modified_time_long) { File.mtime(audio_file_mono_long) } + let(:audio_file_mono_etag_long) { + etag_string = audio_file_mono_long.to_s + '|' + audio_file_mono_modified_time_long.getutc.to_s + '|' + audio_file_mono_size_bytes_long.to_s + Digest::SHA256.hexdigest etag_string + } let(:range_options) { { @@ -27,6 +35,19 @@ } } + let(:range_options_long) { + { + start_offset: 11, + end_offset: 151, + recorded_date: Time.zone.now, + site_name: 'site_name', + site_id: 42, + ext: audio_file_mono_media_type_long.to_sym.to_s, + file_path: audio_file_mono_long, + media_type: audio_file_mono_media_type_long.to_s + } + } + let(:mock_request) { container = OpenStruct.new container.headers = {} @@ -116,23 +137,105 @@ expect(info[:is_multipart]).to be_truthy expect(info[:range_start_bytes][0]).to eq(0) - expect(info[:range_end_bytes][0]).to eq(range_request.max_range_size) + expect(info[:range_end_bytes][0]).to eq(range_request.max_range_size - 1) expect(info[:range_start_bytes][1]).to eq(0) - expect(info[:range_end_bytes][1]).to eq(range_request.max_range_size) + expect(info[:range_end_bytes][1]).to eq(range_request.max_range_size - 1) # last 654321 bytes (or last max_range_size, which ever is smaller) - expect(info[:range_start_bytes][2]).to eq(audio_file_mono_size_bytes - 500 - 1) + expect(info[:range_start_bytes][2]).to eq(audio_file_mono_size_bytes - 500) expect(info[:range_end_bytes][2]).to eq(audio_file_mono_size_bytes - 1) expect(info[:range_start_bytes][3]).to eq(0) expect(info[:range_end_bytes][3]).to eq(10) expect(info[:range_start_bytes][4]).to eq(50) - expect(info[:range_end_bytes][4]).to eq(range_request.max_range_size + 50) + expect(info[:range_end_bytes][4]).to eq(range_request.max_range_size + 50 - 1) + end - expect(info[:range_start_bytes][1]).to eq(0) - expect(info[:range_end_bytes][1]).to eq(range_request.max_range_size) + context 'special open end range case' do + # this test case comes from a real-world production bug: https://github.com/QutBioacoustics/baw-server/issues/318 + # the second part of a large range request triggers a negative content length and the last part of the content + # range header to be less than the first part. + + + # before bug fix: + # file_size: 822281 + # request: "Range: bytes 512001-" + # info[:range_start]: 512001 + # info[:range_end]: 310279 <-- problem, end less than start! + # info[:response_headers]['Content-Length']: -201721 <-- problem, negative range! + it 'should succeed with: [single range] special test case, open range greater than max range size' do + mock_request.headers[RangeRequest::HTTP_HEADER_RANGE] = 'bytes=512001-' + info = range_request.build_response(range_options, mock_request) + expect(info[:response_code]).to eq(RangeRequest::HTTP_CODE_PARTIAL_CONTENT) + expect(info[:response_is_range]).to be_truthy + expect(info[:is_multipart]).to be_falsey + + expect(info[:range_start_bytes][0]).to eq(512001) + expect(info[:range_end_bytes][0]).to eq(audio_file_mono_size_bytes - 1) + + expect(info[:response_headers]['Content-Length']).to eq(310280.to_s) + end + + + it 'should succeed with: [single range] special test case, open range greater than max range size, larger file' do + mock_request.headers[RangeRequest::HTTP_HEADER_RANGE] = 'bytes=512001-' + + # ensure xxx- range still honors max_range_size + # using the LONG file here! + info = range_request.build_response(range_options_long, mock_request) + expect(info[:response_code]).to eq(RangeRequest::HTTP_CODE_PARTIAL_CONTENT) + expect(info[:response_is_range]).to be_truthy + expect(info[:is_multipart]).to be_falsey + + expect(info[:range_start_bytes][0]).to eq(512001) + # > "abcdefghij"[3..(3+5-1)] + # => "defgh" + expect(info[:range_end_bytes][0]).to eq(512001 + range_request.max_range_size - 1) + + expect(info[:response_headers]['Content-Length']).to eq(range_request.max_range_size.to_s) + end + + it 'should succeed with: [single range] special test case, last bytes greater than max range offset' do + mock_request.headers[RangeRequest::HTTP_HEADER_RANGE] = 'bytes=-500' + info = range_request.build_response(range_options, mock_request) + expect(info[:response_code]).to eq(RangeRequest::HTTP_CODE_PARTIAL_CONTENT) + expect(info[:response_is_range]).to be_truthy + expect(info[:is_multipart]).to be_falsey + + expect(info[:range_start_bytes][0]).to eq(audio_file_mono_size_bytes - 500) # 821781 + expect(info[:range_end_bytes][0]).to eq(audio_file_mono_size_bytes - 1) + + expect(info[:response_headers]['Content-Length']).to eq(500.to_s) + end + + it 'should succeed with: [single range] special test case, last bytes range greater than max range size' do + mock_request.headers[RangeRequest::HTTP_HEADER_RANGE] = 'bytes=-800000' + info = range_request.build_response(range_options, mock_request) + expect(info[:response_code]).to eq(RangeRequest::HTTP_CODE_PARTIAL_CONTENT) + expect(info[:response_is_range]).to be_truthy + expect(info[:is_multipart]).to be_falsey + + expect(info[:range_start_bytes][0]).to eq(audio_file_mono_size_bytes - range_request.max_range_size) + expect(info[:range_end_bytes][0]).to eq(audio_file_mono_size_bytes - 1) + + expect(info[:response_headers]['Content-Length']).to eq(range_request.max_range_size.to_s) + end + + it 'should succeed with: [single range] special test case, entire file' do + mock_request.headers[RangeRequest::HTTP_HEADER_RANGE] = 'bytes=0-822280' + range_request = RangeRequest.new(1000000) + info = range_request.build_response(range_options, mock_request) + expect(info[:response_code]).to eq(RangeRequest::HTTP_CODE_PARTIAL_CONTENT) + expect(info[:response_is_range]).to be_truthy + expect(info[:is_multipart]).to be_falsey + + expect(info[:range_start_bytes][0]).to eq(0) + expect(info[:range_end_bytes][0]).to eq(audio_file_mono_size_bytes - 1) + + expect(info[:response_headers]['Content-Length']).to eq(audio_file_mono_size_bytes.to_s) + end end it 'should succeed with if-modified-since earlier than file modified time' do