diff --git a/seaweed-volume/src/server/handlers.rs b/seaweed-volume/src/server/handlers.rs index fdba467c7c..1310793564 100644 --- a/seaweed-volume/src/server/handlers.rs +++ b/seaweed-volume/src/server/handlers.rs @@ -1519,41 +1519,78 @@ fn not_modified_response( etag: &str, last_modified_str: &Option, ) -> Option { - // 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::>() + .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)] diff --git a/weed/server/volume_server_handlers_read.go b/weed/server/volume_server_handlers_read.go index 0c46428920..16ab5bf96a 100644 --- a/weed/server/volume_server_handlers_read.go +++ b/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) diff --git a/weed/server/volume_server_handlers_read_test.go b/weed/server/volume_server_handlers_read_test.go index 758ff52ea8..1402a9954d 100644 --- a/weed/server/volume_server_handlers_read_test.go +++ b/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) + } + }) + } +}