From fd6e4a76f83d7ecb83da1820d47a7675f4507f5d Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Thu, 18 Nov 2021 16:35:24 +0000 Subject: [PATCH] skip body in 304 responses --- actix-files/CHANGES.md | 3 ++ actix-files/src/named.rs | 25 ++++++---- actix-http/src/h1/encoder.rs | 8 +++ actix-http/tests/test_server.rs | 87 +++++++++++++++++++++++++++++++++ 4 files changed, 113 insertions(+), 10 deletions(-) diff --git a/actix-files/CHANGES.md b/actix-files/CHANGES.md index e1a2c90c5..23accca40 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. [#????] + +[#????]: https://github.com/actix/actix-web/pull/???? ## 0.6.0-beta.8 - 2021-10-20 diff --git a/actix-files/src/named.rs b/actix-files/src/named.rs index 241e78cf0..dac548708 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/src/h1/encoder.rs b/actix-http/src/h1/encoder.rs index cda70a151..e8e71e35a 100644 --- a/actix-http/src/h1/encoder.rs +++ b/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; + } + _ => {} } } diff --git a/actix-http/tests/test_server.rs b/actix-http/tests/test_server.rs index 2dca09e21..eb8348080 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 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; +}