From 87009455c9df05f227a3cbae5658b58828e17bcb Mon Sep 17 00:00:00 2001 From: Patrick Sinclair Date: Thu, 7 Aug 2025 16:42:27 +0100 Subject: [PATCH] WEBrick::HTTPServlet::FileHandler: Fix if-range header returning 304 instead of 206/200 Range requests with if-range header were incorrectly returning 304 (Not Modified) status instead of the proper 206 (Partial Content) or 200 (OK) responses. According to HTTP specification, if-range header should: - Return 206 (Partial Content) if the resource hasn't been modified - Return 200 (OK) with full content if the resource has been modified - Never return 304 (Not Modified) The 304 behavior should only apply to if-modified-since and if-none-match headers, even for range requests. Changes: - Move if-range handling out of not_modified? method into dedicated logic - Fix date precision issues in conditional header comparisons by normalizing to HTTP date format before comparison - Preserve existing 304 behavior for if-modified-since/if-none-match headers - Add comprehensive tests covering all if-range scenarios Fixes range request behavior to comply with RFC 7233 specification. --- lib/webrick/httpservlet/filehandler.rb | 71 ++++++++++++++------- test/webrick/test_filehandler.rb | 88 ++++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 23 deletions(-) diff --git a/lib/webrick/httpservlet/filehandler.rb b/lib/webrick/httpservlet/filehandler.rb index 4e759d8..d4ff7b7 100644 --- a/lib/webrick/httpservlet/filehandler.rb +++ b/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 diff --git a/test/webrick/test_filehandler.rb b/test/webrick/test_filehandler.rb index db7618d..a55e11f 100644 --- a/test/webrick/test_filehandler.rb +++ b/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|