Build a 304 from the response it revalidates

CHANGELOG.md

@@ -22,6 +22,15 @@
   connection with 1002 or 1007. A well-formed close is answered with the
   client's own status code echoed back, as RFC 6455 §5.5.1 asks for, and a
   close carrying no payload is still answered with an empty close.
+- **A `304 Not Modified` keeps the headers the `200` would have sent.** The
+  revalidation response was built from an empty header map holding nothing but
+  `ETag`, so `Cache-Control`, `Vary`, `Last-Modified`, `Expires` and the
+  `Access-Control-*` headers a hook had added were thrown away: a browser
+  revalidating a cross-origin resource got a 304 with no
+  `Access-Control-Allow-Origin` and failed the CORS check, and a shared cache
+  lost the `Vary: Origin` telling it to key the entry by origin. The 304 now
+  carries everything the `200` did apart from the body and the headers that
+  describe it.
 
 ## [0.9.3]
 

test/web.carp

@@ -71,6 +71,38 @@
   @(Array.unsafe-first
     &(Map.get-with-default (Response.headers resp) name &[@""])))
 
+; builds the 304 for a conditional GET, running `ah` as the after-hooks
+(defn cached-304 [ah]
+  (let [app (App.GET (App.create)
+                     @"/"
+                     (fn [-r -p]
+                       (-> (Response.text @"hi")
+                           (Response.with-header @"ETag" @"\"abc\"")
+                           (Response.with-header @"Cache-Control"
+                                                 @"max-age=60"))))
+        bh (the (Array (Fn [&Request &(Map String String)] (Maybe Response))) [])
+        raw @"GET / HTTP/1.1\r\nHost: x\r\nOrigin: http://a.test\r\nIf-None-Match: \"abc\"\r\n\r\n"
+        pair (web-build-response &app &bh ah &(String.to-bytes &raw))]
+    @(Pair.a &pair)))
+
+; like `cached-304`, with the CORS after-hook configured for `origin`
+(defn cors-304 [origin]
+  (let-do [ah (the
+            (Array (Fn [&Request &(Map String String) Response] Response))
+            [CORS.after-hook])]
+    (CORS.configure origin)
+    (cached-304 &ah)))
+
+(defn no-hooks []
+  (the (Array (Fn [&Request &(Map String String) Response] Response)) []))
+
+; the 304 built from a 200 that also carried `name: value`
+(defn not-modified-with [name value]
+  (web-not-modified
+    (-> (Response.text @"hi")
+        (Response.with-header @"ETag" @"\"abc\"")
+        (Response.with-header name value))))
+
 ; matches `raw` against a single Server-Sent Events route at `pattern`
 (defn sse-match [raw pattern]
   (web-try-sse &(String.to-bytes raw)
@@ -1298,6 +1330,82 @@
       (Map.contains? (Response.headers &resp) "ETag"))
     "304 response preserves ETag header")
 
+  ; -- 304 keeps the caching and negotiation headers the 200 carried --
+  (assert-equal test
+    "max-age=60"
+    &(header-val &(cached-304 &(no-hooks)) "Cache-Control")
+    "304 keeps Cache-Control")
+  (assert-equal test
+    "http://a.test"
+    &(header-val &(cors-304 @"http://a.test") "Access-Control-Allow-Origin")
+    "304 keeps the CORS after-hook's Access-Control-Allow-Origin")
+  (assert-equal test
+    "Origin"
+    &(header-val &(cors-304 @"http://a.test") "Vary")
+    "304 keeps the CORS after-hook's Vary")
+
+  ; -- 304 drops the representation and framing headers --
+  (assert-false test
+    (Map.contains? (Response.headers &(cached-304 &(no-hooks))) "Content-Type")
+    "304 drops Content-Type")
+  (assert-true test
+    (String.empty? (Response.body &(cached-304 &(no-hooks))))
+    "304 has no body")
+  (assert-equal test
+    "0"
+    &(header-val
+      &(web-finalize-response (cached-304 &(no-hooks)) true)
+      "Content-Length")
+    "a finalized 304 sends Content-Length: 0")
+
+  ; -- the drop list matches header names case-insensitively --
+  (assert-false test
+    (Map.contains?
+      (Response.headers &(not-modified-with @"content-length" @"5"))
+      "content-length")
+    "304 drops a lowercase content-length")
+  (assert-false test
+    (Map.contains?
+      (Response.headers &(not-modified-with @"transfer-encoding" @"chunked"))
+      "transfer-encoding")
+    "304 drops a lowercase transfer-encoding")
+  (assert-false test
+    (Map.contains?
+      (Response.headers &(not-modified-with @"Content-type" @"text/plain"))
+      "Content-type")
+    "304 drops a mixed-case Content-type")
+  (assert-equal test
+    1
+    (Array.length
+      &(header-values-ci
+        (Response.headers
+          &(web-finalize-response (not-modified-with @"content-length" @"5")
+                                  true))
+        "Content-Length"))
+    "a finalized 304 sends one Content-Length even when the handler set another")
+
+  ; -- a sendfile 304 leaves the transfer machinery disarmed --
+  (assert-equal test
+    304
+    @(Response.code
+      &(static-resp
+        "GET /static/index.html HTTP/1.1\r\nRange: bytes=0-1\r\nIf-Modified-Since: Fri, 31 Dec 2100 23:59:59 GMT\r\n\r\n"))
+    "a ranged conditional GET for a static file returns 304")
+  (assert-false test
+    (Map.contains?
+      (Response.headers
+        &(static-resp
+          "GET /static/index.html HTTP/1.1\r\nRange: bytes=0-1\r\nIf-Modified-Since: Fri, 31 Dec 2100 23:59:59 GMT\r\n\r\n"))
+      "X-Sendfile")
+    "a 304 for a static file drops X-Sendfile")
+  (assert-false test
+    (Map.contains?
+      (Response.headers
+        &(static-resp
+          "GET /static/index.html HTTP/1.1\r\nRange: bytes=0-1\r\nIf-Modified-Since: Fri, 31 Dec 2100 23:59:59 GMT\r\n\r\n"))
+      "X-Sendfile-Range")
+    "a 304 for a static file drops the X-Sendfile-Range marker")
+
   ; -- No 304 when ETags differ --
   (assert-equal test
     200

web.carp

@@ -1881,15 +1881,22 @@ stream and any proxy in front of it from timing out.")
             false
             (= &inm (Array.unsafe-first &vals))))))
 
-; Build a 304 Not Modified response, preserving the ETag header.
+; Turn a response into a 304 keeping the headers RFC 9110 §15.4.5 requires.
 (hidden web-not-modified)
 (defn web-not-modified [resp]
-  (let [etag-vals (Map.get-with-default (Response.headers &resp) "ETag" &[])]
-    (if (= (Array.length &etag-vals) 0)
-      (Response.init 304 @"Not Modified" @"HTTP/1.1" [] {} @"")
-      (let [k @"ETag"
-            v [@(Array.unsafe-first &etag-vals)]]
-        (Response.init 304 @"Not Modified" @"HTTP/1.1" [] (Map.put {} &k &v) @"")))))
+  (let [drop-keys [@"content-length" @"content-type" @"transfer-encoding"
+                   @"x-sendfile" @"x-sendfile-range"]
+        hdrs (Map.kv-reduce
+          &(fn [m k v]
+            (if (Array.contains? &drop-keys &(String.ascii-to-lower k))
+              m
+              (Map.put m k v)))
+          (the (Map String (Array String)) {})
+          (Response.headers &resp))]
+    (-> resp
+        (Response.with-status 304 @"Not Modified")
+        (Response.set-body @"")
+        (Response.set-headers hdrs))))
 
 ; True when the response's Last-Modified is not newer than If-Modified-Since.
 (hidden web-if-modified-since?)