From 194a69153751e5d28df78ec69444daa80e93f84c Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Fri, 19 Nov 2021 14:04:12 +0000 Subject: [PATCH] files: 304 Not Modified responses omit Content-Length header (#2453) --- actix-files/CHANGES.md | 3 ++ actix-files/src/named.rs | 25 ++++++---- actix-http-test/src/lib.rs | 6 +-- actix-http/src/body/body.rs | 2 + actix-http/src/config.rs | 2 + actix-http/src/h1/client.rs | 2 +- actix-http/src/h1/codec.rs | 24 ++++----- actix-http/src/h1/encoder.rs | 21 ++++++-- actix-http/src/message.rs | 2 +- actix-http/tests/test_server.rs | 87 +++++++++++++++++++++++++++++++++ actix-test/src/lib.rs | 6 +-- 11 files changed, 145 insertions(+), 35 deletions(-) diff --git a/actix-files/CHANGES.md b/actix-files/CHANGES.md index e1a2c90c..41336c21 100644 --- a/actix-files/CHANGES.md +++ b/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 diff --git a/actix-files/src/named.rs b/actix-files/src/named.rs index 241e78cf..dac54870 100644 --- a/actix-files/src/named.rs +++ b/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); diff --git a/actix-http-test/src/lib.rs b/actix-http-test/src/lib.rs index cda98cea..699bb266 100644 --- a/actix-http-test/src/lib.rs +++ b/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 diff --git a/actix-http/src/body/body.rs b/actix-http/src/body/body.rs index 04fc957c..c6439b55 100644 --- a/actix-http/src/body/body.rs +++ b/actix-http/src/body/body.rs @@ -32,6 +32,8 @@ pub enum AnyBody { } impl AnyBody { + // TODO: a None body constructor + /// Constructs a new, empty body. pub fn empty() -> Self { Self::Bytes(Bytes::new()) diff --git a/actix-http/src/config.rs b/actix-http/src/config.rs index 069099b8..5d020edf 100644 --- a/actix-http/src/config.rs +++ b/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, } diff --git a/actix-http/src/h1/client.rs b/actix-http/src/h1/client.rs index 4a610468..bec16797 100644 --- a/actix-http/src/h1/client.rs +++ b/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 diff --git a/actix-http/src/h1/codec.rs b/actix-http/src/h1/codec.rs index 634ca25e..29f6f417 100644 --- a/actix-http/src/h1/codec.rs +++ b/actix-http/src/h1/codec.rs @@ -29,7 +29,7 @@ pub struct Codec { decoder: decoder::MessageDecoder, payload: Option, 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(); - if self.ctype == ConnectionType::KeepAlive + self.conn_type = head.connection_type(); + 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, 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, 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, BodySize)>> for Codec { self.encoder.encode_eof(dst)?; } } + Ok(()) } } diff --git a/actix-http/src/h1/encoder.rs b/actix-http/src/h1/encoder.rs index e07c3295..cc26a200 100644 --- a/actix-http/src/h1/encoder.rs +++ b/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 MessageEncoder { stream: bool, version: Version, length: BodySize, - ctype: ConnectionType, + conn_type: ConnectionType, config: &ServiceConfig, ) -> io::Result<()> { // transfer encoding @@ -349,7 +358,7 @@ impl MessageEncoder { } 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. diff --git a/actix-http/src/message.rs b/actix-http/src/message.rs index 84125fb3..e0bed063 100644 --- a/actix-http/src/message.rs +++ b/actix-http/src/message.rs @@ -317,7 +317,7 @@ impl ResponseHead { } #[inline] - pub(crate) fn ctype(&self) -> Option { + pub(crate) fn conn_type(&self) -> Option { if self.flags.contains(Flags::CLOSE) { Some(ConnectionType::Close) } else if self.flags.contains(Flags::KEEP_ALIVE) { diff --git a/actix-http/tests/test_server.rs b/actix-http/tests/test_server.rs index 2dca09e2..11bc8e93 100644 --- a/actix-http/tests/test_server.rs +++ b/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 = 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; +} diff --git a/actix-test/src/lib.rs b/actix-test/src/lib.rs index cf5738aa..6c776a87 100644 --- a/actix-test/src/lib.rs +++ b/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