skip body in 304 responses

actix-files/CHANGES.md

@@ -1,6 +1,9 @@
 # Changes
 
 ## Unreleased - 2021-xx-xx
+* Fix 304 Not Modified responses to omit the Content-Length header, as per the spec. [#????]
+
+[#????]: https://github.com/actix/actix-web/pull/????
 
 
 ## 0.6.0-beta.8 - 2021-10-20

actix-files/src/named.rs

@@ -1,17 +1,22 @@
-use actix_service::{Service, ServiceFactory};
-use actix_utils::future::{ok, ready, Ready};
-use actix_web::dev::{AppService, HttpServiceFactory, ResourceDef};
-use std::fs::{File, Metadata};
-use std::io;
-use std::ops::{Deref, DerefMut};
-use std::path::{Path, PathBuf};
-use std::time::{SystemTime, UNIX_EPOCH};
+use std::{
+    fs::{File, Metadata},
+    io,
+    ops::{Deref, DerefMut},
+    path::{Path, PathBuf},
+    time::{SystemTime, UNIX_EPOCH},
+};
 
 #[cfg(unix)]
 use std::os::unix::fs::MetadataExt;
 
+use actix_http::body::AnyBody;
+use actix_service::{Service, ServiceFactory};
+use actix_utils::future::{ok, ready, Ready};
 use actix_web::{
-    dev::{BodyEncoding, ServiceRequest, ServiceResponse, SizedStream},
+    dev::{
+        AppService, BodyEncoding, HttpServiceFactory, ResourceDef, ServiceRequest,
+        ServiceResponse, SizedStream,
+    },
     http::{
         header::{
             self, Charset, ContentDisposition, DispositionParam, DispositionType, ExtendedValue,
@@ -443,7 +448,7 @@ impl NamedFile {
         if precondition_failed {
             return resp.status(StatusCode::PRECONDITION_FAILED).finish();
         } else if not_modified {
-            return resp.status(StatusCode::NOT_MODIFIED).finish();
+            return resp.status(StatusCode::NOT_MODIFIED).body(AnyBody::None);
         }
 
         let reader = ChunkedReadFile::new(length, offset, self.file);

actix-http/src/h1/encoder.rs

@@ -76,6 +76,14 @@ pub(crate) trait MessageType: Sized {
                     skip_len = true;
                     length = BodySize::None
                 }
+
+                StatusCode::NOT_MODIFIED => {
+                    // don't skip content-length header for not modified responses
+                    // see https://datatracker.ietf.org/doc/html/rfc7232#section-4.1
+                    skip_len = false;
+                    length = BodySize::None;
+                }
+
                 _ => {}
             }
         }

actix-http/tests/test_server.rs

@@ -759,3 +759,90 @@ async fn test_h1_on_connect() {
 
     srv.stop().await;
 }
+
+/// Tests compliance with 304 Not Modified spec in RFC 7232 §4.1.
+/// https://datatracker.ietf.org/doc/html/rfc7232#section-4.1
+#[actix_rt::test]
+async fn test_not_modified_spec_h1() {
+    // TODO: this test needing a few seconds to complete reveals some weirdness with either the
+    // dispatcher or the client, though similar hangs occur on other tests in this file, only
+    // succeeding, it seems, because of the keepalive timer
+
+    static CL: header::HeaderName = header::CONTENT_LENGTH;
+
+    let mut srv = test_server(|| {
+        HttpService::build()
+            .h1(|req: Request| {
+                let res: Response<AnyBody> = match req.path() {
+                    // with no content-length
+                    "/none" => {
+                        Response::with_body(StatusCode::NOT_MODIFIED, AnyBody::None)
+                    }
+
+                    // with no content-length
+                    "/body" => Response::with_body(
+                        StatusCode::NOT_MODIFIED,
+                        AnyBody::from("1234"),
+                    ),
+
+                    // with manual content-length header and specific None body
+                    "/cl-none" => {
+                        let mut res =
+                            Response::with_body(StatusCode::NOT_MODIFIED, AnyBody::None);
+                        res.headers_mut()
+                            .insert(CL.clone(), header::HeaderValue::from_static("24"));
+                        res
+                    }
+
+                    // with manual content-length header and ignore-able body
+                    "/cl-body" => {
+                        let mut res = Response::with_body(
+                            StatusCode::NOT_MODIFIED,
+                            AnyBody::from("1234"),
+                        );
+                        res.headers_mut()
+                            .insert(CL.clone(), header::HeaderValue::from_static("4"));
+                        res
+                    }
+
+                    _ => panic!("unknown route"),
+                };
+
+                ok::<_, Infallible>(res)
+            })
+            .tcp()
+    })
+    .await;
+
+    let res = srv.get("/none").send().await.unwrap();
+    assert_eq!(res.status(), http::StatusCode::NOT_MODIFIED);
+    assert_eq!(res.headers().get(&CL), None);
+    assert!(srv.load_body(res).await.unwrap().is_empty());
+
+    let res = srv.get("/body").send().await.unwrap();
+    assert_eq!(res.status(), http::StatusCode::NOT_MODIFIED);
+    assert_eq!(res.headers().get(&CL), None);
+    assert!(srv.load_body(res).await.unwrap().is_empty());
+
+    let res = srv.get("/cl-none").send().await.unwrap();
+    assert_eq!(res.status(), http::StatusCode::NOT_MODIFIED);
+    assert_eq!(
+        res.headers().get(&CL),
+        Some(&header::HeaderValue::from_static("24")),
+    );
+    assert!(srv.load_body(res).await.unwrap().is_empty());
+
+    let res = srv.get("/cl-body").send().await.unwrap();
+    assert_eq!(res.status(), http::StatusCode::NOT_MODIFIED);
+    assert_eq!(
+        res.headers().get(&CL),
+        Some(&header::HeaderValue::from_static("4")),
+    );
+    // server does not prevent payload from being sent but clients will likely choose not to read it
+    // TODO: this is probably a bug
+    assert!(!srv.load_body(res).await.unwrap().is_empty());
+
+    // TODO: add stream response tests
+
+    srv.stop().await;
+}