Add support for multiple Etags in If-None-Match header (#6219)

crystal-lang · Jul 9, 2018 · 4771dcb · 4771dcb
1 parent ee6ec50
commit 4771dcb
Show file tree

Hide file tree

Showing 4 changed files with 163 additions and 2 deletions.
diff --git a/spec/std/http/request_spec.cr b/spec/std/http/request_spec.cr
@@ -380,5 +380,45 @@ module HTTP
         HTTP::Request.from_io(io)
       end
     end
+
+    describe "#if_none_match" do
+      it "reads single value" do
+        HTTP::Request.new("GET", "/", HTTP::Headers{"If-None-Match" => %(W/"1234567")}).if_none_match.should eq [%(W/"1234567")]
+        HTTP::Request.new("GET", "/", HTTP::Headers{"If-None-Match" => %("1234567")}).if_none_match.should eq [%("1234567")]
+      end
+
+      it "reads *" do
+        HTTP::Request.new("GET", "/", HTTP::Headers{"If-None-Match" => "*"}).if_none_match.should eq ["*"]
+      end
+
+      it "reads multiple values" do
+        HTTP::Request.new("GET", "/", HTTP::Headers{"If-None-Match" => %(,W/"1234567",)}).if_none_match.should eq [%(W/"1234567")]
+        HTTP::Request.new("GET", "/", HTTP::Headers{"If-None-Match" => %(, , W/"1234567" , ,)}).if_none_match.should eq [%(W/"1234567")]
+        HTTP::Request.new("GET", "/", HTTP::Headers{"If-None-Match" => %(W/"1234567",W/"12345678")}).if_none_match.should eq [%(W/"1234567"), %(W/"12345678")]
+        HTTP::Request.new("GET", "/", HTTP::Headers{"If-None-Match" => %(W/"1234567" , W/"12345678")}).if_none_match.should eq [%(W/"1234567"), %(W/"12345678")]
+        HTTP::Request.new("GET", "/", HTTP::Headers{"If-None-Match" => %(W/"1234567","12345678")}).if_none_match.should eq [%(W/"1234567"), %("12345678")]
+        HTTP::Request.new("GET", "/", HTTP::Headers{"If-None-Match" => %(W/"1234567" , "12345678")}).if_none_match.should eq [%(W/"1234567"), %("12345678")]
+      end
+    end
+
+    describe "#if_match" do
+      it "reads single value" do
+        HTTP::Request.new("GET", "/", HTTP::Headers{"If-Match" => %(W/"1234567")}).if_match.should eq [%(W/"1234567")]
+        HTTP::Request.new("GET", "/", HTTP::Headers{"If-Match" => %("1234567")}).if_match.should eq [%("1234567")]
+      end
+
+      it "reads *" do
+        HTTP::Request.new("GET", "/", HTTP::Headers{"If-Match" => "*"}).if_match.should eq ["*"]
+      end
+
+      it "reads multiple values" do
+        HTTP::Request.new("GET", "/", HTTP::Headers{"If-Match" => %(,W/"1234567",)}).if_match.should eq [%(W/"1234567")]
+        HTTP::Request.new("GET", "/", HTTP::Headers{"If-Match" => %(, , W/"1234567" , ,)}).if_match.should eq [%(W/"1234567")]
+        HTTP::Request.new("GET", "/", HTTP::Headers{"If-Match" => %(W/"1234567",W/"12345678")}).if_match.should eq [%(W/"1234567"), %(W/"12345678")]
+        HTTP::Request.new("GET", "/", HTTP::Headers{"If-Match" => %(W/"1234567" , W/"12345678")}).if_match.should eq [%(W/"1234567"), %(W/"12345678")]
+        HTTP::Request.new("GET", "/", HTTP::Headers{"If-Match" => %(W/"1234567","12345678")}).if_match.should eq [%(W/"1234567"), %("12345678")]
+        HTTP::Request.new("GET", "/", HTTP::Headers{"If-Match" => %(W/"1234567" , "12345678")}).if_match.should eq [%(W/"1234567"), %("12345678")]
+      end
+    end
   end
 end
diff --git a/spec/std/http/server/handlers/static_file_handler_spec.cr b/spec/std/http/server/handlers/static_file_handler_spec.cr
@@ -81,6 +81,51 @@ describe HTTP::StaticFileHandler do
       response.status_code.should eq(200)
       response.body.should eq(File.read(datapath("static_file_handler", "test.txt")))
     end
+
+    it "returns 304 Not Modified if header is *" do
+      headers = HTTP::Headers.new
+      headers["If-None-Match"] = "*"
+      response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: true
+      response.status_code.should eq(304)
+    end
+
+    it "serves file if header is empty" do
+      headers = HTTP::Headers.new
+      headers["If-None-Match"] = ""
+
+      response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: false
+      response.status_code.should eq(200)
+      response.body.should eq(File.read(datapath("static_file_handler", "test.txt")))
+    end
+
+    it "serves file if header does not contain valid etag" do
+      headers = HTTP::Headers.new
+      headers["If-None-Match"] = ", foo"
+
+      response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: false
+      response.status_code.should eq(200)
+      response.body.should eq(File.read(datapath("static_file_handler", "test.txt")))
+    end
+  end
+
+  context "with multiple If-None-Match header" do
+    it "returns 304 Not Modified if at least one header matches etag" do
+      initial_response = handle HTTP::Request.new("GET", "/test.txt")
+
+      headers = HTTP::Headers.new
+      headers["If-None-Match"] = %(,, ,W/"1234567"   , , #{initial_response.headers["Etag"]},"12345678",%)
+      response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: true
+      response.status_code.should eq(304)
+    end
+
+    it "serves file if no header matches etag" do
+      headers = HTTP::Headers.new
+      headers["If-None-Match"] = "some random etag, 1234567"
+
+      response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: false
+      response.status_code.should eq(200)
+      response.body.should eq(File.read(datapath("static_file_handler", "test.txt")))
+    end
   end
 
   context "with both If-None-Match and If-Modified-Since headers" do

diff --git a/src/http/request.cr b/src/http/request.cr
@@ -159,4 +159,79 @@ class HTTP::Request
     return unless @query_params
     uri.query = query_params.to_s
   end
+
+  def if_match : Array(String)?
+    parse_etags("If-Match")
+  end
+
+  def if_none_match : Array(String)?
+    parse_etags("If-None-Match")
+  end
+
+  private def parse_etags(header_name)
+    header = headers[header_name]?
+
+    return unless header
+    return ["*"] if header == "*"
+
+    etags = [] of String
+    reader = Char::Reader.new(header)
+
+    require_comma = false
+    while reader.has_next?
+      case char = reader.current_char
+      when ' ', '\t'
+        reader.next_char
+      when ','
+        reader.next_char
+        require_comma = false
+      when '"', 'W'
+        if require_comma
+          # return what we've got on error
+          return etags
+        end
+
+        reader, etag = consume_etag(reader)
+        if etag
+          etags << etag
+          require_comma = true
+        else
+          # return what we've got on error
+          return etags
+        end
+      else
+        # return what we've got on error
+        return etags
+      end
+    end
+
+    etags
+  end
+
+  private def consume_etag(reader)
+    start = reader.pos
+
+    if reader.current_char == 'W'
+      reader.next_char
+      return reader, nil if reader.current_char != '/' || !reader.has_next?
+      reader.next_char
+    end
+
+    return reader, nil if reader.current_char != '"'
+    reader.next_char
+
+    while reader.has_next?
+      case char = reader.current_char
+      when '!', '\u{23}'..'\u{7E}', '\u{80}'..'\u{FF}'
+        reader.next_char
+      when '"'
+        reader.next_char
+        return reader, reader.string.byte_slice(start, reader.pos - start)
+      else
+        return reader, nil
+      end
+    end
+
+    return reader, nil
+  end
 end
diff --git a/src/http/server/handlers/static_file_handler.cr b/src/http/server/handlers/static_file_handler.cr
@@ -103,8 +103,9 @@ class HTTP::StaticFileHandler
   private def cache_request?(context : HTTP::Server::Context, last_modified : Time) : Bool
     # According to RFC 7232:
     # A recipient must ignore If-Modified-Since if the request contains an If-None-Match header field
-    if if_none_match = context.request.headers["If-None-Match"]?
-      if_none_match == context.response.headers["Etag"]
+    if if_none_match = context.request.if_none_match
+      match = {"*", context.response.headers["Etag"]}
+      if_none_match.any? { |etag| match.includes?(etag) }
     elsif if_modified_since = context.request.headers["If-Modified-Since"]?
       header_time = HTTP.parse_time(if_modified_since)
       # File mtime probably has a higher resolution than the header value.