WEBrick::HTTPServlet::FileHandler: Fix if-range header returning 304 instead of 206/200

lib/webrick/httpservlet/filehandler.rb

@@ -46,35 +46,60 @@ def do_GET(req, res)
         mtime = st.mtime
         res['etag'] = sprintf("%x-%x-%x", st.ino, st.size, st.mtime.to_i)
 
-        if not_modified?(req, res, mtime, res['etag'])
-          res.body = ''
-          raise HTTPStatus::NotModified
-        elsif req['range']
-          make_partial_content(req, res, @local_path, st.size)
-          raise HTTPStatus::PartialContent
+        if req['range']
+          # Handle if-range header specially for range requests
+          if req['if-range']
+            if if_range_matches?(req, res, mtime)
+              # Resource unchanged - return partial content (206)
+              make_partial_content(req, res, @local_path, st.size)
+              raise HTTPStatus::PartialContent
+            else
+              # Resource changed - return full content (200)
+              mtype = HTTPUtils::mime_type(@local_path, @config[:MimeTypes])
+              res['content-type'] = mtype
+              res['content-length'] = st.size.to_s
+              res['last-modified'] = mtime.httpdate
+              res.body = File.open(@local_path, "rb")
+            end
+          else
+            # Range request without if-range - check other conditional headers
+            if not_modified?(req, res, mtime, res['etag'])
+              res.body = ''
+              raise HTTPStatus::NotModified
+            else
+              make_partial_content(req, res, @local_path, st.size)
+              raise HTTPStatus::PartialContent
+            end
+          end
         else
-          mtype = HTTPUtils::mime_type(@local_path, @config[:MimeTypes])
-          res['content-type'] = mtype
-          res['content-length'] = st.size.to_s
-          res['last-modified'] = mtime.httpdate
-          res.body = File.open(@local_path, "rb")
+          # Non-range request
+          if not_modified?(req, res, mtime, res['etag'])
+            res.body = ''
+            raise HTTPStatus::NotModified
+          else
+            mtype = HTTPUtils::mime_type(@local_path, @config[:MimeTypes])
+            res['content-type'] = mtype
+            res['content-length'] = st.size.to_s
+            res['last-modified'] = mtime.httpdate
+            res.body = File.open(@local_path, "rb")
+          end
         end
       end
 
-      def not_modified?(req, res, mtime, etag)
-        if ir = req['if-range']
-          begin
-            if Time.httpdate(ir) >= mtime
-              return true
-            end
-          rescue
-            if HTTPUtils::split_header_value(ir).member?(res['etag'])
-              return true
-            end
-          end
+      def if_range_matches?(req, res, mtime)
+        return unless ir = req['if-range']
+
+        begin
+          Time.httpdate(ir) >= mtime.floor
+        rescue
+          HTTPUtils::split_header_value(ir).member?(res['etag'])
         end
+      end
+
+      def not_modified?(req, res, mtime, etag)
+        ims = req['if-modified-since']
+        if (ims = req['if-modified-since']) && Time.parse(ims) >= mtime.floor
 
-        if (ims = req['if-modified-since']) && Time.parse(ims) >= mtime
           return true
         end
 

test/webrick/test_filehandler.rb

@@ -185,6 +185,94 @@ def test_filehandler
     end
   end
 
+  def test_if_range_header
+    config = { :DocumentRoot => File.dirname(__FILE__), }
+    this_file = File.basename(__FILE__)
+    filesize = File.size(__FILE__)
+    this_data = File.binread(__FILE__)
+
+    TestWEBrick.start_httpserver(config) do |server, addr, port, log|
+      http = Net::HTTP.new(addr, port)
+
+      # First, get the file to obtain its ETag and Last-Modified
+      req = Net::HTTP::Get.new("/#{this_file}")
+      etag = nil
+      last_modified = nil
+      http.request(req) do |res|
+        assert_equal("200", res.code, log.call)
+        etag = res["etag"]
+        last_modified = res["last-modified"]
+        assert_not_nil(etag, "ETag should be present")
+        assert_not_nil(last_modified, "Last-Modified should be present")
+      end
+
+      # Test if-range with valid etag - should return 206 (partial content)
+      req = Net::HTTP::Get.new("/#{this_file}", {
+        "range" => "bytes=0-99",
+        "if-range" => etag
+      })
+      http.request(req) do |res|
+        assert_equal("206", res.code, log.call)
+        assert_equal("text/plain", res.content_type, log.call)
+        assert_equal(this_data[0..99], res.body, log.call)
+      end
+
+      # Test if-range with valid last-modified date - should return 206 (partial content)
+      req = Net::HTTP::Get.new("/#{this_file}", {
+        "range" => "bytes=100-199",
+        "if-range" => last_modified
+      })
+      http.request(req) do |res|
+        assert_equal("206", res.code, log.call)
+        assert_equal("text/plain", res.content_type, log.call)
+        assert_equal(this_data[100..199], res.body, log.call)
+      end
+
+      # Test if-range with invalid etag - should return 200 (full content)
+      req = Net::HTTP::Get.new("/#{this_file}", {
+        "range" => "bytes=0-99",
+        "if-range" => '"invalid-etag"'
+      })
+      http.request(req) do |res|
+        assert_equal("200", res.code, log.call)
+        assert_equal("text/plain", res.content_type, log.call)
+        assert_equal(this_data, res.body, log.call)
+      end
+
+      # Test if-range with old date - should return 200 (full content)
+      old_date = (Time.parse(last_modified) - 3600).httpdate  # 1 hour ago
+      req = Net::HTTP::Get.new("/#{this_file}", {
+        "range" => "bytes=0-99",
+        "if-range" => old_date
+      })
+      http.request(req) do |res|
+        assert_equal("200", res.code, log.call)
+        assert_equal("text/plain", res.content_type, log.call)
+        assert_equal(this_data, res.body, log.call)
+      end
+
+      # Test that if-modified-since still works for range requests (should return 304)
+      req = Net::HTTP::Get.new("/#{this_file}", {
+        "range" => "bytes=0-99",
+        "if-modified-since" => last_modified
+      })
+      http.request(req) do |res|
+        assert_equal("304", res.code, log.call)
+        assert_equal(nil, res.body, log.call)
+      end
+
+      # Test that if-none-match still works for range requests (should return 304)
+      req = Net::HTTP::Get.new("/#{this_file}", {
+        "range" => "bytes=0-99",
+        "if-none-match" => etag
+      })
+      http.request(req) do |res|
+        assert_equal("304", res.code, log.call)
+        assert_equal(nil, res.body, log.call)
+      end
+    end
+  end
+
   def test_non_disclosure_name
     config = { :DocumentRoot => File.dirname(__FILE__), }
     log_tester = lambda {|log, access_log|