Leave unsafe-method responses alone and honor If-Range

CHANGELOG.md

@@ -1,5 +1,24 @@
 # Changelog
 
+## Unreleased
+
+### Fixed
+- **A conditional `PUT`, `POST`, `PATCH` or `DELETE` is no longer told it
+  failed after it succeeded.** web evaluates `If-None-Match` after the handler
+  has run, so for a method that changes state the write had already happened,
+  and the response's `ETag` described the new representation. A conditional
+  create (`PUT` with `If-None-Match: *`) wrote and was then answered with a
+  `412`. web now sends such a response as the handler built it and answers
+  only a `GET` or `HEAD` with a `304`. A handler that needs a precondition
+  evaluates it before it acts, with http's `Request.preconditions` against the
+  resource's current validators.
+- **A resumed download of a changed file is no longer spliced.** `If-Range` was
+  ignored, so a `Range` request for a static file that had changed got a `206`
+  with bytes of the new file, which the client appended to the old one's. A
+  `Range` whose `If-Range` names another `ETag` or `Last-Modified` now gets
+  the whole file with a `200`, as RFC 9110 §13.1.5 requires. A weak `ETag`
+  never matches.
+
 ## [0.12.0]
 
 ### Changed

test/web.carp

@@ -167,30 +167,6 @@
 (defn no-hooks-before []
   (the (Array (Fn [&Request &(Map String String)] (Maybe Response))) []))
 
-; the response a `verb` request carrying `hdrs` gets from a route whose 200
-; sets an ETag, `X-Custom` and a cookie, behind the CORS after-hook
-(defn cors-conditional [verb hdrs]
-  (let-do [app (App.route (App.create)
-                          @verb
-                          @"/"
-                          (fn [-r -p]
-                            (-> (Response.text @"hi")
-                                (Response.with-header @"ETag" @"\"abc\"")
-                                (Response.with-header @"X-Custom" @"yes")
-                                (Response.set-simple-cookie @"sid" @"s1"))))
-           bh (the
-             (Array (Fn [&Request &(Map String String)] (Maybe Response)))
-             [])
-           ah (the
-             (Array (Fn [&Request &(Map String String) Response] Response))
-             [CORS.after-hook])
-           raw (fmt
-             "%s / HTTP/1.1\r\nHost: x\r\nOrigin: http://a.test\r\n%s\r\n"
-             verb
-             hdrs)]
-    (CORS.configure @"http://a.test")
-    @(Pair.a &(web-build-response &app &bh &ah &(String.to-bytes &raw)))))
-
 ; `resp` as it goes on the wire, i.e. once its framing headers are filled in
 (defn framed [resp] (web-finalize-response resp true))
 
@@ -1317,6 +1293,80 @@
           "HEAD /static/index.html HTTP/1.1\r\nRange: bytes=0-4\r\n\r\n")))
     "HEAD range request has no body")
 
+  ; -- a Range whose If-Range names another version is ignored (RFC 9110 §13.1.5) --
+  (assert-equal test
+    200
+    @(Response.code
+      &(static-resp
+        "HEAD /static/index.html HTTP/1.1\r\nRange: bytes=0-4\r\nIf-Range: \"changed\"\r\n\r\n"))
+    "an If-Range naming another ETag serves the whole file")
+  (assert-false test
+    (Map.contains?
+      (Response.headers
+        &(static-resp
+          "GET /static/index.html HTTP/1.1\r\nRange: bytes=0-4\r\nIf-Range: \"changed\"\r\n\r\n"))
+      "X-Sendfile-Range")
+    "an If-Range naming another ETag leaves a GET's range marker off")
+  (assert-equal test
+    206
+    @(Response.code
+      &(static-resp
+        &(fmt
+          "HEAD /static/index.html HTTP/1.1\r\nRange: bytes=0-4\r\nIf-Range: %s\r\n\r\n"
+          &(header-val
+            &(static-resp "HEAD /static/index.html HTTP/1.1\r\n\r\n")
+            "ETag"))))
+    "an If-Range naming the current ETag serves the range")
+  (assert-true test
+    (Map.contains?
+      (Response.headers
+        &(static-resp
+          &(fmt
+            "GET /static/index.html HTTP/1.1\r\nRange: bytes=0-4\r\nIf-Range: %s\r\n\r\n"
+            &(header-val
+              &(static-resp "HEAD /static/index.html HTTP/1.1\r\n\r\n")
+              "ETag"))))
+      "X-Sendfile-Range")
+    "an If-Range naming the current ETag arms a GET's range marker")
+  (assert-equal test
+    200
+    @(Response.code
+      &(static-resp
+        &(fmt
+          "HEAD /static/index.html HTTP/1.1\r\nRange: bytes=0-4\r\nIf-Range: W/%s\r\n\r\n"
+          &(header-val
+            &(static-resp "HEAD /static/index.html HTTP/1.1\r\n\r\n")
+            "ETag"))))
+    "a weak If-Range never matches, so the whole file is served")
+  (assert-equal test
+    206
+    @(Response.code
+      &(static-resp
+        &(fmt
+          "HEAD /static/index.html HTTP/1.1\r\nRange: bytes=0-4\r\nIf-Range: %s\r\n\r\n"
+          &(header-val
+            &(static-resp "HEAD /static/index.html HTTP/1.1\r\n\r\n")
+            "Last-Modified"))))
+    "an If-Range naming the current Last-Modified serves the range")
+  (assert-equal test
+    200
+    @(Response.code
+      &(static-resp
+        "HEAD /static/index.html HTTP/1.1\r\nRange: bytes=0-4\r\nIf-Range: Sat, 01 Jan 2000 00:00:00 GMT\r\n\r\n"))
+    "an If-Range naming another date serves the whole file")
+  (assert-equal test
+    200
+    @(Response.code
+      &(static-resp
+        "HEAD /static/index.html HTTP/1.1\r\nIf-Range: \"changed\"\r\n\r\n"))
+    "an If-Range without a Range changes nothing")
+  (assert-equal test
+    200
+    @(Response.code
+      &(static-resp
+        "HEAD /static/index.html HTTP/1.1\r\nRange: bytes=0-4\r\nif-range: \"changed\"\r\n\r\n"))
+    "a lower-case if-range is read too")
+
   ; -- HEAD with an unsatisfiable range answers 416 --
   (assert-equal test
     416
@@ -1561,59 +1611,37 @@
     (inm-code "W/\"abc\"" "\"abcd\"")
     "the weak comparison still compares the whole opaque tag")
 
-  ; -- 304 is a GET/HEAD outcome; other methods get 412 (RFC 9110 §13.2.2) --
+  ; -- 304 is a GET/HEAD outcome; any other method's handler has already acted,
+  ; so its response goes out as the handler built it --
   (assert-equal test
     304
     (conditional-code "HEAD" "\"abc\"" "If-None-Match: \"abc\"\r\n")
     "a matching If-None-Match on HEAD returns 304")
   (assert-equal test
-    412
+    200
     (conditional-code "PUT" "\"abc\"" "If-None-Match: \"abc\"\r\n")
-    "a matching If-None-Match on PUT returns 412")
+    "a matching If-None-Match on PUT leaves the handler's response alone")
+  (assert-equal test
+    200
+    (conditional-code "PUT" "\"abc\"" "If-None-Match: *\r\n")
+    "a conditional create on PUT gets the handler's response, not a 412")
   (assert-equal test
-    412
+    200
     (conditional-code "DELETE" "\"abc\"" "If-None-Match: *\r\n")
-    "a matching If-None-Match on DELETE returns 412")
+    "a matching If-None-Match on DELETE leaves the handler's response alone")
+  (assert-equal test
+    200
+    (conditional-code "POST" "\"abc\"" "If-None-Match: \"abc\"\r\n")
+    "a matching If-None-Match on POST leaves the handler's response alone")
   (assert-equal test
     200
     (conditional-code "PUT" "\"abc\"" "If-None-Match: \"z\"\r\n")
     "a failing If-None-Match on PUT leaves the 200 alone")
-  (assert-true test
-    (String.empty?
-      (Response.body
-        &(conditional-resp "PUT" "\"abc\"" "" "If-None-Match: \"abc\"\r\n")))
-    "a 412 has no body")
-  (assert-false test
-    (Map.contains?
-      (Response.headers
-        &(conditional-resp "PUT" "\"abc\"" "" "If-None-Match: \"abc\"\r\n"))
-      "Content-Type")
-    "a 412 drops Content-Type")
-
-  ; -- a 412 keeps every header that was not describing the body --
-  (assert-true test
-    (Map.contains?
-      (Response.headers
-        &(conditional-resp "PUT" "\"abc\"" "" "If-None-Match: \"abc\"\r\n"))
-      "ETag")
-    "a 412 carries the ETag its precondition was tested against")
-  (assert-equal test
-    "http://a.test"
-    &(header-val
-      &(cors-conditional "PUT" "If-None-Match: \"abc\"\r\n")
-      "Access-Control-Allow-Origin")
-    "a 412 keeps the CORS after-hook's Access-Control-Allow-Origin")
   (assert-equal test
-    "yes"
-    &(header-val
-      &(cors-conditional "PUT" "If-None-Match: \"abc\"\r\n")
-      "X-Custom")
-    "a 412 keeps a header the handler set")
-  (assert-true test
-    (Map.contains?
-      (Response.headers &(cors-conditional "PUT" "If-None-Match: \"abc\"\r\n"))
-      "Set-Cookie")
-    "a 412 keeps a cookie the handler set")
+    "hi"
+    (Response.body
+      &(conditional-resp "PUT" "\"abc\"" "" "If-None-Match: \"abc\"\r\n"))
+    "a matching If-None-Match on PUT keeps the handler's body")
 
   ; -- If-Modified-Since is GET/HEAD-only and yields to If-None-Match --
   (assert-equal test
@@ -1781,13 +1809,6 @@
         (conditional-resp "HEAD" "\"abc\"" "" "If-None-Match: \"z\"\r\n"))
       "Content-Length")
     "a HEAD 200 keeps the length its GET would have had")
-  (assert-equal test
-    "0"
-    &(header-val
-      &(framed
-        (conditional-resp "PUT" "\"abc\"" "" "If-None-Match: \"abc\"\r\n"))
-      "Content-Length")
-    "a 412 frames the empty body it does carry")
 
   ; -- a sendfile 304 leaves the transfer machinery disarmed --
   (assert-equal test

web.carp

@@ -1928,6 +1928,29 @@ stream and any proxy in front of it from timing out.")
                         (Map.remove @(Response.headers &resp)
                                     &@"X-Sendfile-Range")))
 
+; The first value of response header `name`, if any.
+(hidden web-response-header)
+(defn web-response-header [resp name]
+  (let [vals (header-values-ci (Response.headers resp) name)]
+    (if (Array.empty? &vals)
+      (Maybe.Nothing)
+      (Maybe.Just @(Array.unsafe-first &vals)))))
+
+; Whether a Range may be served: true unless the request's If-Range names a
+; version other than the response's (RFC 9110 §13.1.5), in which case the
+; whole file goes out instead. The validators are read only for an If-Range.
+(hidden web-if-range-current?)
+(defn web-if-range-current? [req resp]
+  (if (Maybe.nothing? &(Request.header req "If-Range"))
+    true
+    (let [etag (match (web-response-header resp "ETag")
+            (Maybe.Just v) (Result.to-maybe (ETag.parse &v))
+            (Maybe.Nothing) (Maybe.Nothing))
+          modified (match (web-response-header resp "Last-Modified")
+            (Maybe.Just v) (Result.to-maybe (HttpDate.parse &v))
+            (Maybe.Nothing) (Maybe.Nothing))]
+      (Request.if-range-matches? req &etag &modified))))
+
 ; Attach the request's Range header to a sendfile response as an internal
 ; marker, resolved against the file size at transfer setup.
 (hidden web-mark-range)
@@ -1938,9 +1961,12 @@ stream and any proxy in front of it from timing out.")
       (match (Request.header req "Range")
         (Maybe.Nothing) resp
         (Maybe.Just rv)
-          (let [k @"X-Sendfile-Range"
-                v [rv]]
-            (Response.set-headers resp (Map.put @(Response.headers &resp) &k &v))))))
+          (if (web-if-range-current? req &resp)
+            (let [k @"X-Sendfile-Range"
+                  v [rv]]
+              (Response.set-headers resp
+                                    (Map.put @(Response.headers &resp) &k &v)))
+            resp))))
 
 ; Resolve the marker attached by web-mark-range against a known file size.
 (hidden web-resolve-marker)
@@ -2132,11 +2158,6 @@ stream and any proxy in front of it from timing out.")
 (hidden web-not-modified)
 (defn web-not-modified [resp] (web-bodyless resp 304 @"Not Modified"))
 
-; The 412 a failed precondition owes a non-GET/HEAD request (RFC 9110 §15.5.13).
-(hidden web-precondition-failed)
-(defn web-precondition-failed [resp]
-  (web-bodyless resp 412 @"Precondition Failed"))
-
 ; True when the response's Last-Modified is not newer than If-Modified-Since.
 (hidden web-if-modified-since?)
 (defn web-if-modified-since? [req resp]
@@ -2153,15 +2174,19 @@ stream and any proxy in front of it from timing out.")
                 (Maybe.Nothing) false
                 (Maybe.Just mtime) (not (Long.< since mtime))))))))
 
-; If-None-Match takes precedence over If-Modified-Since (RFC 9110 §13.1.3),
-; which applies to GET and HEAD only; a non-2xx response ignores both (§13.2.1).
+; If-None-Match takes precedence over If-Modified-Since (RFC 9110 §13.1.3); a
+; non-2xx response ignores both (§13.2.1). Only a GET or HEAD is answered with
+; a 304: any other method's handler has acted by the time its response is seen,
+; so the response goes out as the handler built it, and a handler that needs a
+; precondition evaluates it before acting.
 (hidden web-conditional-not-modified?)
 (defn web-conditional-not-modified? [req resp]
   (let [code @(Response.code resp)]
     (cond
+      (not (web-get-or-head? req)) false
       (or (< code 200) (>= code 300)) false
       (Array.empty? &(header-values-ci (Request.headers req) "If-None-Match"))
-        (and (web-get-or-head? req) (web-if-modified-since? req resp))
+        (web-if-modified-since? req resp)
       (web-etag-match? req resp))))
 
 ; Forward-declare App.max-header-line so top-level validation helpers can
@@ -2255,11 +2280,8 @@ stream and any proxy in front of it from timing out.")
                       (~(Route.handler r) &req &params))))
               after-resp (web-run-after after-hooks &req &params resp)
               ranged (web-mark-range &req after-resp)
-              ; a failed precondition: 304 for GET/HEAD, 412 for anything else
               cond-resp (if (web-conditional-not-modified? &req &ranged)
-                (if (web-get-or-head? &req)
-                  (web-not-modified ranged)
-                  (web-precondition-failed ranged))
+                (web-not-modified ranged)
                 ranged)
               ; HEAD: strip body, preserve Content-Length
               final (if is-head (web-strip-head-body cond-resp) cond-resp)]