volume: check If-None-Match before If-Modified-Since and match ETag lists (#12027)

* filer: match If-Match and If-None-Match against "*" and ETag lists

checkPreconditions compared the whole header as one ETag, so "*", a
list of ETags and a weak W/ tag in If-None-Match never matched
(RFC 9110 13.1.1, 13.1.2).

* volume: check If-None-Match before If-Modified-Since and match ETag lists

A volume GET answered 304 from If-Modified-Since even when If-None-Match
named a different ETag, and compared If-None-Match as one exact string,
so "*", lists and weak tags never matched (RFC 9110 13.1.2, 13.2.2).

* filer: split etag lists only outside quoted tags

A comma inside a quoted opaque tag is a legal etagc, not a list
separator; strings.Split cut such tags into fragments that
CanonicalizeETag could turn into a false match.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

* filer: match repeated If-Match and If-None-Match header lines

Header.Get reads only the first field line, so a match on a later
repeated header was missed; join all field values like any other
comma-separated list.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

* volume: match repeated If-None-Match header lines

Header.Get reads only the first field line; join all field values so a
match on a later repeated header is seen. Also covers a comma inside a
quoted opaque tag.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

* volume: send the etag on a 304 like the filer does

RFC 9110 15.4.5 lists ETag among the fields a 304 must carry when a 200
would send it; SetEtag now runs before the notModified check.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

* volume: mirror the conditional-GET fixes in the rust server

not_modified_response now checks If-None-Match before If-Modified-Since,
matches "*", weak tags, quoted commas and repeated field lines through
the same list rules, and sends the etag on a 304.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

* volume: treat an empty If-None-Match like the Go server does

* filer: ignore empty elements in etag precondition lists

* volume: ignore empty elements in etag lists like the filer

* filer: let a present-but-empty precondition line suppress the date check

* volume: gate If-Modified-Since on a present If-None-Match line

* volume: gate If-Modified-Since on header presence like the Go server

---------

Co-authored-by: user.email <chrislusf@users.noreply.github.com>
Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-authored-by: chrislusf <chris.lu@gmail.com>

seaweed-volume/src/server/handlers.rs

@@ -1519,41 +1519,78 @@ fn not_modified_response(
     etag: &str,
     last_modified_str: &Option<String>,
 ) -> Option<Response> {
-    // If-Modified-Since first (Go order), then If-None-Match.
+    let not_modified = || {
+        let mut resp = StatusCode::NOT_MODIFIED.into_response();
+        resp.headers_mut()
+            .insert(header::ETAG, etag.parse().unwrap());
+        if let Some(lm) = last_modified_str {
+            resp.headers_mut()
+                .insert(header::LAST_MODIFIED, lm.parse().unwrap());
+        }
+        resp
+    };
+
+    // If-Modified-Since applies only when there is no If-None-Match (RFC 9110 13.2.2).
+    if headers.contains_key(header::IF_NONE_MATCH) {
+        let inm = headers
+            .get_all(header::IF_NONE_MATCH)
+            .iter()
+            .map(|v| v.to_str().unwrap_or(""))
+            .collect::<Vec<_>>()
+            .join(",");
+        return etag_list_matches(&inm, etag).then(not_modified);
+    }
+
     if n.last_modified > 0
-        && let Some(ims_header) = headers.get(header::IF_MODIFIED_SINCE)
-        && let Ok(ims_str) = ims_header.to_str()
+        && let Some(ims_str) = headers
+            .get(header::IF_MODIFIED_SINCE)
+            .and_then(|v| v.to_str().ok())
     {
         // Parse HTTP date format: "Mon, 02 Jan 2006 15:04:05 GMT"
         if let Ok(ims_time) =
             chrono::NaiveDateTime::parse_from_str(ims_str, "%a, %d %b %Y %H:%M:%S GMT")
             && (n.last_modified as i64) <= ims_time.and_utc().timestamp()
         {
-            let mut resp = StatusCode::NOT_MODIFIED.into_response();
-            if let Some(lm) = last_modified_str {
-                resp.headers_mut()
-                    .insert(header::LAST_MODIFIED, lm.parse().unwrap());
-            }
-            // Go sets ETag after the 304 paths, so a 304 has no ETag.
-            return Some(resp);
+            return Some(not_modified());
         }
     }
-
-    if let Some(if_none_match) = headers.get(header::IF_NONE_MATCH)
-        && let Ok(inm) = if_none_match.to_str()
-        && inm == etag
-    {
-        let mut resp = StatusCode::NOT_MODIFIED.into_response();
-        if let Some(lm) = last_modified_str {
-            resp.headers_mut()
-                .insert(header::LAST_MODIFIED, lm.parse().unwrap());
-        }
-        // Go sets ETag after the 304 paths, so a 304 has no ETag.
-        return Some(resp);
-    }
     None
 }
 
+/// Weak comparison against "*" or a comma-separated entity-tag list (RFC 9110 13.1.2).
+fn etag_list_matches(header: &str, etag: &str) -> bool {
+    if header.trim() == "*" {
+        return true;
+    }
+    let etag = etag.trim_matches('"');
+    split_etag_list(header).into_iter().any(|tag| {
+        let tag = tag.trim();
+        let tag = tag.strip_prefix("W/").unwrap_or(tag);
+        // Empty list elements are ignored, so padding like a trailing
+        // comma cannot match an empty opaque tag.
+        !tag.is_empty() && tag.trim_matches('"') == etag
+    })
+}
+
+/// Splits an entity-tag list on the commas between tags; a comma inside a
+/// quoted opaque tag does not split.
+fn split_etag_list(header: &str) -> Vec<&str> {
+    let mut tags = Vec::new();
+    let (mut start, mut quoted) = (0, false);
+    for (i, b) in header.bytes().enumerate() {
+        match b {
+            b'"' => quoted = !quoted,
+            b',' if !quoted => {
+                tags.push(&header[start..i]);
+                start = i + 1;
+            }
+            _ => {}
+        }
+    }
+    tags.push(&header[start..]);
+    tags
+}
+
 /// The 200 reply headers, and the extension, which falls back to the stored name's.
 fn read_response_headers(
     n: &Needle,
@@ -5644,6 +5681,80 @@ mod tests {
         delete_handler(State(state.clone()), request).await.status()
     }
 
+    #[test]
+    fn test_not_modified_checks_if_none_match_first() {
+        let n = Needle {
+            last_modified: 1700000000,
+            ..Needle::default()
+        };
+        let (etag, last_modified_str) = etag_and_last_modified(&n);
+        let bare = n.etag();
+        let before = "Tue, 14 Nov 2023 22:12:20 GMT".to_string();
+        let after = "Tue, 14 Nov 2023 22:14:20 GMT".to_string();
+
+        let cases: Vec<(Vec<(axum::http::HeaderName, String)>, bool)> = vec![
+            (vec![(header::IF_NONE_MATCH, format!("\"{bare}\""))], true),
+            (vec![(header::IF_NONE_MATCH, format!("W/\"{bare}\""))], true),
+            (
+                vec![(header::IF_NONE_MATCH, format!("\"x\", \"{bare}\""))],
+                true,
+            ),
+            (vec![(header::IF_NONE_MATCH, "*".into())], true),
+            (vec![(header::IF_NONE_MATCH, "\"x\"".into())], false),
+            (
+                vec![
+                    (header::IF_NONE_MATCH, "\"x\"".into()),
+                    (header::IF_NONE_MATCH, format!("\"{bare}\"")),
+                ],
+                true,
+            ),
+            (
+                vec![(header::IF_NONE_MATCH, format!("\"{bare},old\""))],
+                false,
+            ),
+            (
+                vec![(header::IF_NONE_MATCH, format!("\"x\",,\"{bare}\""))],
+                true,
+            ),
+            (
+                vec![
+                    (header::IF_NONE_MATCH, "\"x\"".into()),
+                    (header::IF_MODIFIED_SINCE, after.clone()),
+                ],
+                false,
+            ),
+            (
+                vec![
+                    (header::IF_NONE_MATCH, "".into()),
+                    (header::IF_MODIFIED_SINCE, after.clone()),
+                ],
+                false,
+            ),
+            (vec![(header::IF_MODIFIED_SINCE, after)], true),
+            (vec![(header::IF_MODIFIED_SINCE, before)], false),
+        ];
+        for (pairs, want) in cases {
+            let mut headers = HeaderMap::new();
+            for (name, value) in &pairs {
+                headers.append(name, value.parse().unwrap());
+            }
+            assert_eq!(
+                not_modified_response(&n, &headers, &etag, &last_modified_str).is_some(),
+                want,
+                "{pairs:?}"
+            );
+        }
+
+        let mut headers = HeaderMap::new();
+        headers.insert(
+            header::IF_NONE_MATCH,
+            format!("\"{bare}\"").parse().unwrap(),
+        );
+        let resp = not_modified_response(&n, &headers, &etag, &last_modified_str).unwrap();
+        assert!(resp.headers().contains_key(header::ETAG));
+        assert!(resp.headers().contains_key(header::LAST_MODIFIED));
+    }
+
     /// A GET whose needle read is parked must not hold the store lock: a
     /// writer, here a real append to the same volume, must get through.
     #[tokio::test(flavor = "multi_thread", worker_threads = 2)]

weed/server/volume_server_handlers_read.go

@@ -149,6 +149,20 @@ func (vs *VolumeServer) proxyReqToTargetServer(w http.ResponseWriter, r *http.Re
 	}
 }
 
+// notModified checks If-Modified-Since only when there is no If-None-Match
+// (RFC 9110 13.2.2).
+func notModified(r *http.Request, etag string, lastModified uint64) bool {
+	if inm := r.Header.Values("If-None-Match"); len(inm) != 0 {
+		return etagListMatches(strings.Join(inm, ","), etag, true)
+	}
+	ims := r.Header.Get("If-Modified-Since")
+	if ims == "" || lastModified == 0 {
+		return false
+	}
+	t, err := time.Parse(http.TimeFormat, ims)
+	return err == nil && t.Unix() >= int64(lastModified)
+}
+
 func (vs *VolumeServer) GetOrHeadHandler(w http.ResponseWriter, r *http.Request) {
 	n := new(needle.Needle)
 	vid, fid, filename, ext, _ := parseURLPath(r.URL.Path)
@@ -244,21 +258,9 @@ func (vs *VolumeServer) GetOrHeadHandler(w http.ResponseWriter, r *http.Request)
 	}
 	if n.LastModified != 0 {
 		w.Header().Set("Last-Modified", time.Unix(int64(n.LastModified), 0).UTC().Format(http.TimeFormat))
-		if r.Header.Get("If-Modified-Since") != "" {
-			if t, parseError := time.Parse(http.TimeFormat, r.Header.Get("If-Modified-Since")); parseError == nil {
-				if t.Unix() >= int64(n.LastModified) {
-					if readOption.IsMetaOnly {
-						if v := vs.store.GetVolume(volumeId); v != nil {
-							v.NoteMetaOnlySuccess()
-						}
-					}
-					w.WriteHeader(http.StatusNotModified)
-					return
-				}
-			}
-		}
 	}
-	if inm := r.Header.Get("If-None-Match"); inm == "\""+n.Etag()+"\"" {
+	SetEtag(w, n.Etag())
+	if notModified(r, n.Etag(), n.LastModified) {
 		if readOption.IsMetaOnly {
 			if v := vs.store.GetVolume(volumeId); v != nil {
 				v.NoteMetaOnlySuccess()
@@ -267,7 +269,6 @@ func (vs *VolumeServer) GetOrHeadHandler(w http.ResponseWriter, r *http.Request)
 		w.WriteHeader(http.StatusNotModified)
 		return
 	}
-	SetEtag(w, n.Etag())
 
 	if n.HasPairs() {
 		pairMap := make(map[string]string)

weed/server/volume_server_handlers_read_test.go

@@ -6,6 +6,7 @@ import (
 	"net/http/httptest"
 	"strings"
 	"testing"
+	"time"
 )
 
 func TestInternalErrorWritesTheReadError(t *testing.T) {
@@ -59,3 +60,39 @@ func TestShouldAttemptStreamWrite(t *testing.T) {
 		}
 	}
 }
+
+func TestNotModifiedChecksIfNoneMatchFirst(t *testing.T) {
+	const etag = "353dd8be"
+	const lastModified = 1700000000
+	before := time.Unix(lastModified-60, 0).UTC().Format(http.TimeFormat)
+	after := time.Unix(lastModified+60, 0).UTC().Format(http.TimeFormat)
+
+	tests := []struct {
+		name    string
+		headers [][2]string
+		want    bool
+	}{
+		{"matching etag", [][2]string{{"If-None-Match", `"353dd8be"`}}, true},
+		{"weak etag", [][2]string{{"If-None-Match", `W/"353dd8be"`}}, true},
+		{"etag in a list", [][2]string{{"If-None-Match", `"x", "353dd8be"`}}, true},
+		{"star", [][2]string{{"If-None-Match", `*`}}, true},
+		{"other etag", [][2]string{{"If-None-Match", `"x"`}}, false},
+		{"etag across repeated lines", [][2]string{{"If-None-Match", `"x"`}, {"If-None-Match", `"353dd8be"`}}, true},
+		{"comma inside quotes", [][2]string{{"If-None-Match", `"353dd8be,old"`}}, false},
+		{"other etag ignores If-Modified-Since", [][2]string{{"If-None-Match", `"x"`}, {"If-Modified-Since", after}}, false},
+		{"empty If-None-Match ignores If-Modified-Since", [][2]string{{"If-None-Match", ``}, {"If-Modified-Since", after}}, false},
+		{"not modified since", [][2]string{{"If-Modified-Since", after}}, true},
+		{"modified since", [][2]string{{"If-Modified-Since", before}}, false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			r := httptest.NewRequest(http.MethodGet, "/3,01637037d6", nil)
+			for _, kv := range tt.headers {
+				r.Header.Add(kv[0], kv[1])
+			}
+			if got := notModified(r, etag, lastModified); got != tt.want {
+				t.Errorf("notModified = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}