files: 304 Not Modified responses omit Content-Length header (#2453)

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. [#2453]
+
+[#2453]: https://github.com/actix/actix-web/pull/2453
 
 
 ## 0.6.0-beta.8 - 2021-10-20

actix-files/src/named.rs

@@ -1,17 +1,22 @@
-use actix_service::{Service, ServiceFactory};
+use std::{
-use actix_utils::future::{ok, ready, Ready};
+    fs::{File, Metadata},
-use actix_web::dev::{AppService, HttpServiceFactory, ResourceDef};
+    io,
-use std::fs::{File, Metadata};
+    ops::{Deref, DerefMut},
-use std::io;
+    path::{Path, PathBuf},
-use std::ops::{Deref, DerefMut};
+    time::{SystemTime, UNIX_EPOCH},
-use std::path::{Path, PathBuf};
+};
-use std::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-test/src/lib.rs

@@ -273,12 +273,12 @@ impl TestServer {
         self.client.headers()
     }
 
-    /// Gracefully stop HTTP server.
+    /// Stop HTTP server.
     ///
-    /// Waits for spawned `Server` and `System` to shutdown gracefully.
+    /// Waits for spawned `Server` and `System` to (force) shutdown.
     pub async fn stop(&mut self) {
         // signal server to stop
-        self.server.stop(true).await;
+        self.server.stop(false).await;
 
         // also signal system to stop
         // though this is handled by `ServerBuilder::exit_system` too

actix-http/src/body/body.rs

@@ -32,6 +32,8 @@ pub enum AnyBody<B = BoxBody> {
 }
 
 impl AnyBody {
+    // TODO: a None body constructor
+
     /// Constructs a new, empty body.
     pub fn empty() -> Self {
         Self::Bytes(Bytes::new())

actix-http/src/config.rs

@@ -20,8 +20,10 @@ pub(crate) const DATE_VALUE_LENGTH: usize = 29;
 pub enum KeepAlive {
     /// Keep alive in seconds
     Timeout(usize),
+
     /// Rely on OS to shutdown tcp connection
     Os,
+
     /// Disabled
     Disabled,
 }

actix-http/src/h1/client.rs

@@ -120,7 +120,7 @@ impl Decoder for ClientCodec {
         debug_assert!(!self.inner.payload.is_some(), "Payload decoder is set");
 
         if let Some((req, payload)) = self.inner.decoder.decode(src)? {
-            if let Some(ctype) = req.ctype() {
+            if let Some(ctype) = req.conn_type() {
                 // do not use peer's keep-alive
                 self.inner.ctype = if ctype == ConnectionType::KeepAlive {
                     self.inner.ctype

actix-http/src/h1/codec.rs

@@ -29,7 +29,7 @@ pub struct Codec {
     decoder: decoder::MessageDecoder<Request>,
     payload: Option<PayloadDecoder>,
     version: Version,
-    ctype: ConnectionType,
+    conn_type: ConnectionType,
 
     // encoder part
     flags: Flags,
@@ -65,7 +65,7 @@ impl Codec {
             decoder: decoder::MessageDecoder::default(),
             payload: None,
             version: Version::HTTP_11,
-            ctype: ConnectionType::Close,
+            conn_type: ConnectionType::Close,
             encoder: encoder::MessageEncoder::default(),
         }
     }
@@ -73,13 +73,13 @@ impl Codec {
     /// Check if request is upgrade.
     #[inline]
     pub fn upgrade(&self) -> bool {
-        self.ctype == ConnectionType::Upgrade
+        self.conn_type == ConnectionType::Upgrade
     }
 
     /// Check if last response is keep-alive.
     #[inline]
     pub fn keepalive(&self) -> bool {
-        self.ctype == ConnectionType::KeepAlive
+        self.conn_type == ConnectionType::KeepAlive
     }
 
     /// Check if keep-alive enabled on server level.
@@ -124,11 +124,11 @@ impl Decoder for Codec {
             let head = req.head();
             self.flags.set(Flags::HEAD, head.method == Method::HEAD);
             self.version = head.version;
-            self.ctype = head.connection_type();
+            self.conn_type = head.connection_type();
-            if self.ctype == ConnectionType::KeepAlive
+            if self.conn_type == ConnectionType::KeepAlive
                 && !self.flags.contains(Flags::KEEPALIVE_ENABLED)
             {
-                self.ctype = ConnectionType::Close
+                self.conn_type = ConnectionType::Close
             }
             match payload {
                 PayloadType::None => self.payload = None,
@@ -159,14 +159,14 @@ impl Encoder<Message<(Response<()>, BodySize)>> for Codec {
                 res.head_mut().version = self.version;
 
                 // connection status
-                self.ctype = if let Some(ct) = res.head().ctype() {
+                self.conn_type = if let Some(ct) = res.head().conn_type() {
                     if ct == ConnectionType::KeepAlive {
-                        self.ctype
+                        self.conn_type
                     } else {
                         ct
                     }
                 } else {
-                    self.ctype
+                    self.conn_type
                 };
 
                 // encode message
@@ -177,10 +177,9 @@ impl Encoder<Message<(Response<()>, BodySize)>> for Codec {
                     self.flags.contains(Flags::STREAM),
                     self.version,
                     length,
-                    self.ctype,
+                    self.conn_type,
                     &self.config,
                 )?;
-                // self.headers_size = (dst.len() - len) as u32;
             }
             Message::Chunk(Some(bytes)) => {
                 self.encoder.encode_chunk(bytes.as_ref(), dst)?;
@@ -189,6 +188,7 @@ impl Encoder<Message<(Response<()>, BodySize)>> for Codec {
                 self.encoder.encode_eof(dst)?;
             }
         }
+
         Ok(())
     }
 }

actix-http/src/h1/encoder.rs

@@ -56,7 +56,7 @@ pub(crate) trait MessageType: Sized {
         dst: &mut BytesMut,
         version: Version,
         mut length: BodySize,
-        ctype: ConnectionType,
+        conn_type: ConnectionType,
         config: &ServiceConfig,
     ) -> io::Result<()> {
         let chunked = self.chunked();
@@ -71,14 +71,23 @@ pub(crate) trait MessageType: Sized {
                 | StatusCode::PROCESSING
                 | StatusCode::NO_CONTENT => {
                     // skip content-length and transfer-encoding headers
-                    // See https://tools.ietf.org/html/rfc7230#section-3.3.1
+                    // see https://tools.ietf.org/html/rfc7230#section-3.3.1
                     // and https://tools.ietf.org/html/rfc7230#section-3.3.2
                     skip_len = true;
                     length = BodySize::None
                 }
+
+                StatusCode::NOT_MODIFIED => {
+                    // 304 responses should never have a body but should retain a manually set
+                    // content-length header see https://tools.ietf.org/html/rfc7232#section-4.1
+                    skip_len = false;
+                    length = BodySize::None;
+                }
+
                 _ => {}
             }
         }
+
         match length {
             BodySize::Stream => {
                 if chunked {
@@ -102,7 +111,7 @@ pub(crate) trait MessageType: Sized {
         }
 
         // Connection
-        match ctype {
+        match conn_type {
             ConnectionType::Upgrade => dst.put_slice(b"connection: upgrade\r\n"),
             ConnectionType::KeepAlive if version < Version::HTTP_11 => {
                 if camel_case {
@@ -327,7 +336,7 @@ impl<T: MessageType> MessageEncoder<T> {
         stream: bool,
         version: Version,
         length: BodySize,
-        ctype: ConnectionType,
+        conn_type: ConnectionType,
         config: &ServiceConfig,
     ) -> io::Result<()> {
         // transfer encoding
@@ -349,7 +358,7 @@ impl<T: MessageType> MessageEncoder<T> {
         }
 
         message.encode_status(dst)?;
-        message.encode_headers(dst, version, length, ctype, config)
+        message.encode_headers(dst, version, length, conn_type, config)
     }
 }
 
@@ -363,10 +372,12 @@ pub(crate) struct TransferEncoding {
 enum TransferEncodingKind {
     /// An Encoder for when Transfer-Encoding includes `chunked`.
     Chunked(bool),
+
     /// An Encoder for when Content-Length is set.
     ///
     /// Enforces that the body is not longer than the Content-Length header.
     Length(u64),
+
     /// An Encoder for when Content-Length is not known.
     ///
     /// Application decides when to stop writing.

actix-http/src/message.rs

@@ -317,7 +317,7 @@ impl ResponseHead {
     }
 
     #[inline]
-    pub(crate) fn ctype(&self) -> Option<ConnectionType> {
+    pub(crate) fn conn_type(&self) -> Option<ConnectionType> {
         if self.flags.contains(Flags::CLOSE) {
             Some(ConnectionType::Close)
         } else if self.flags.contains(Flags::KEEP_ALIVE) {

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 may choose not to read it
+    // TODO: this is probably a bug, especially since CL header can differ in length from the body
+    assert!(!srv.load_body(res).await.unwrap().is_empty());
+
+    // TODO: add stream response tests
+
+    srv.stop().await;
+}

actix-test/src/lib.rs

@@ -520,12 +520,12 @@ impl TestServer {
         self.client.headers()
     }
 
-    /// Gracefully stop HTTP server.
+    /// Stop HTTP server.
     ///
-    /// Waits for spawned `Server` and `System` to shutdown gracefully.
+    /// Waits for spawned `Server` and `System` to shutdown (force) shutdown.
     pub async fn stop(mut self) {
         // signal server to stop
-        self.server.stop(true).await;
+        self.server.stop(false).await;
 
         // also signal system to stop
         // though this is handled by `ServerBuilder::exit_system` too