Merge pull request #319 from QutBioacoustics/bug-range-requests

Fixed various bugs with range request implementation, Closes #318

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

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)

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)

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

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