From deece8d519512a83a98c7b8b1cdcd664fc0ee03a Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Thu, 2 Dec 2021 17:04:40 +0000 Subject: [PATCH] re-instate accept-encoding typed header (#2482) --- CHANGES.md | 3 + actix-http/src/header/shared/quality_item.rs | 2 +- actix-http/src/header/utils.rs | 7 + scripts/ci-test | 2 + src/http/header/accept.rs | 5 +- src/http/header/accept_encoding.rs | 23 +- src/http/header/accept_language.rs | 1 + src/http/header/cache_control.rs | 290 +++++++------------ src/http/header/encoding.rs | 13 +- src/http/header/macros.rs | 51 +++- src/http/header/mod.rs | 8 +- 11 files changed, 190 insertions(+), 215 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 36a56b828..c754d4dd6 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -3,6 +3,7 @@ ## Unreleased - 2021-xx-xx ### Added * Methods on `AcceptLanguage`: `ranked` and `preference`. [#2480] +* `AcceptEncoding` typed header. [#2482] ### Changed * Rename `Accept::{mime_precedence => ranked}`. [#2480] @@ -10,8 +11,10 @@ ### Fixed * Accept wildcard `*` items in `AcceptLanguage`. [#2480] +* Typed headers containing lists that require one or more items now enforce this minimum. [#2482] [#2480]: https://github.com/actix/actix-web/pull/2480 +[#2482]: https://github.com/actix/actix-web/pull/2482 ## 4.0.0-beta.13 - 2021-11-30 diff --git a/actix-http/src/header/shared/quality_item.rs b/actix-http/src/header/shared/quality_item.rs index 9b170f01a..a109b44ea 100644 --- a/actix-http/src/header/shared/quality_item.rs +++ b/actix-http/src/header/shared/quality_item.rs @@ -27,7 +27,7 @@ const MAX_FLOAT_QUALITY: f32 = 1.0; /// /// [RFC 7231 §5.3.1](https://datatracker.ietf.org/doc/html/rfc7231#section-5.3.1) gives more /// information on quality values in HTTP header fields. -#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] pub struct Quality(u16); impl Quality { diff --git a/actix-http/src/header/utils.rs b/actix-http/src/header/utils.rs index 2168202b9..a23f5b751 100644 --- a/actix-http/src/header/utils.rs +++ b/actix-http/src/header/utils.rs @@ -83,6 +83,13 @@ mod tests { let res: Vec = from_comma_delimited(headers.iter()).unwrap(); assert_eq!(res, vec![0; 0]); + let headers = vec![ + HeaderValue::from_static("1, 2"), + HeaderValue::from_static("3,4"), + ]; + let res: Vec = from_comma_delimited(headers.iter()).unwrap(); + assert_eq!(res, vec![1, 2, 3, 4]); + let headers = vec![ HeaderValue::from_static(""), HeaderValue::from_static(","), diff --git a/scripts/ci-test b/scripts/ci-test index 096eb7600..98e13927d 100755 --- a/scripts/ci-test +++ b/scripts/ci-test @@ -14,3 +14,5 @@ cargo test --lib --tests -p=actix-test --all-features cargo test --lib --tests -p=actix-files cargo test --lib --tests -p=actix-multipart --all-features cargo test --lib --tests -p=actix-web-actors --all-features + +cargo test --workspace --doc diff --git a/src/http/header/accept.rs b/src/http/header/accept.rs index a0c98547d..fe291c011 100644 --- a/src/http/header/accept.rs +++ b/src/http/header/accept.rs @@ -118,8 +118,9 @@ crate::http::header::common_header! { #[test] fn test_fuzzing1() { - use actix_http::test::TestRequest; - let req = TestRequest::default().insert_header((crate::http::header::ACCEPT, "chunk#;e")).finish(); + let req = test::TestRequest::default() + .insert_header((header::ACCEPT, "chunk#;e")) + .finish(); let header = Accept::parse(&req); assert!(header.is_ok()); } diff --git a/src/http/header/accept_encoding.rs b/src/http/header/accept_encoding.rs index 0440153ae..85cd0a4f7 100644 --- a/src/http/header/accept_encoding.rs +++ b/src/http/header/accept_encoding.rs @@ -1,8 +1,9 @@ -// TODO: reinstate module +use actix_http::header::QualityItem; -use header::{Encoding, QualityItem}; +use super::{common_header, Encoding}; +use crate::http::header; -header! { +common_header! { /// `Accept-Encoding` header, defined /// in [RFC 7231](https://datatracker.ietf.org/doc/html/rfc7231#section-5.3.4) /// @@ -30,7 +31,7 @@ header! { /// use actix_web::HttpResponse; /// use actix_web::http::header::{AcceptEncoding, Encoding, qitem}; /// - /// let mut builder = HttpResponse::new(); + /// let mut builder = HttpResponse::Ok(); /// builder.insert_header( /// AcceptEncoding(vec![qitem(Encoding::Chunked)]) /// ); @@ -39,7 +40,7 @@ header! { /// use actix_web::HttpResponse; /// use actix_web::http::header::{AcceptEncoding, Encoding, qitem}; /// - /// let mut builder = HttpResponse::new(); + /// let mut builder = HttpResponse::Ok(); /// builder.insert_header( /// AcceptEncoding(vec![ /// qitem(Encoding::Chunked), @@ -52,7 +53,7 @@ header! { /// use actix_web::HttpResponse; /// use actix_web::http::header::{AcceptEncoding, Encoding, QualityItem, q, qitem}; /// - /// let mut builder = HttpResponse::new(); + /// let mut builder = HttpResponse::Ok(); /// builder.insert_header( /// AcceptEncoding(vec![ /// qitem(Encoding::Chunked), @@ -65,14 +66,14 @@ header! { test_parse_and_format { // From the RFC - crate::http::header::common_header_test!(test1, vec![b"compress, gzip"]); - crate::http::header::common_header_test!(test2, vec![b""], Some(AcceptEncoding(vec![]))); - crate::http::header::common_header_test!(test3, vec![b"*"]); + common_header_test!(test1, vec![b"compress, gzip"]); + common_header_test!(test2, vec![b""], Some(AcceptEncoding(vec![]))); + common_header_test!(test3, vec![b"*"]); // Note: Removed quality 1 from gzip - crate::http::header::common_header_test!(test4, vec![b"compress;q=0.5, gzip"]); + common_header_test!(test4, vec![b"compress;q=0.5, gzip"]); // Note: Removed quality 1 from gzip - crate::http::header::common_header_test!(test5, vec![b"gzip, identity; q=0.5, *;q=0"]); + common_header_test!(test5, vec![b"gzip, identity; q=0.5, *;q=0"]); } } diff --git a/src/http/header/accept_language.rs b/src/http/header/accept_language.rs index fb1637eb1..229f95ef1 100644 --- a/src/http/header/accept_language.rs +++ b/src/http/header/accept_language.rs @@ -67,6 +67,7 @@ common_header! { vec![b"da, en-gb;q=0.8, en;q=0.7"] ); + common_header_test!( not_ordered_by_weight, vec![b"en-US, en; q=0.5, fr"], diff --git a/src/http/header/cache_control.rs b/src/http/header/cache_control.rs index 27cf30ce4..490d36558 100644 --- a/src/http/header/cache_control.rs +++ b/src/http/header/cache_control.rs @@ -1,92 +1,97 @@ -use std::fmt::{self, Write}; -use std::str::FromStr; - -use derive_more::{Deref, DerefMut}; - -use super::{fmt_comma_delimited, from_comma_delimited, Header, IntoHeaderValue, Writer}; +use std::{fmt, str}; +use super::common_header; use crate::http::header; -/// `Cache-Control` header, defined -/// in [RFC 7234 §5.2](https://datatracker.ietf.org/doc/html/rfc7234#section-5.2). -/// -/// The `Cache-Control` header field is used to specify directives for -/// caches along the request/response chain. Such cache directives are -/// unidirectional in that the presence of a directive in a request does -/// not imply that the same directive is to be given in the response. -/// -/// # ABNF -/// ```plain -/// Cache-Control = 1#cache-directive -/// cache-directive = token [ "=" ( token / quoted-string ) ] -/// ``` -/// -/// # Example Values -/// -/// * `no-cache` -/// * `private, community="UCI"` -/// * `max-age=30` -/// -/// # Examples -/// ``` -/// use actix_web::HttpResponse; -/// use actix_web::http::header::{CacheControl, CacheDirective}; -/// -/// let mut builder = HttpResponse::Ok(); -/// builder.insert_header(CacheControl(vec![CacheDirective::MaxAge(86400u32)])); -/// ``` -/// -/// ``` -/// use actix_web::HttpResponse; -/// use actix_web::http::header::{CacheControl, CacheDirective}; -/// -/// let mut builder = HttpResponse::Ok(); -/// builder.insert_header(CacheControl(vec![ -/// CacheDirective::NoCache, -/// CacheDirective::Private, -/// CacheDirective::MaxAge(360u32), -/// CacheDirective::Extension("foo".to_owned(), Some("bar".to_owned())), -/// ])); -/// ``` -#[derive(Debug, Clone, PartialEq, Eq, Deref, DerefMut)] -pub struct CacheControl(pub Vec); +common_header! { + /// `Cache-Control` header, defined + /// in [RFC 7234 §5.2](https://datatracker.ietf.org/doc/html/rfc7234#section-5.2). + /// + /// The `Cache-Control` header field is used to specify directives for + /// caches along the request/response chain. Such cache directives are + /// unidirectional in that the presence of a directive in a request does + /// not imply that the same directive is to be given in the response. + /// + /// # ABNF + /// ```text + /// Cache-Control = 1#cache-directive + /// cache-directive = token [ "=" ( token / quoted-string ) ] + /// ``` + /// + /// # Example Values + /// * `no-cache` + /// * `private, community="UCI"` + /// * `max-age=30` + /// + /// # Examples + /// ``` + /// use actix_web::HttpResponse; + /// use actix_web::http::header::{CacheControl, CacheDirective}; + /// + /// let mut builder = HttpResponse::Ok(); + /// builder.insert_header(CacheControl(vec![CacheDirective::MaxAge(86400u32)])); + /// ``` + /// + /// ``` + /// use actix_web::HttpResponse; + /// use actix_web::http::header::{CacheControl, CacheDirective}; + /// + /// let mut builder = HttpResponse::Ok(); + /// builder.insert_header(CacheControl(vec![ + /// CacheDirective::NoCache, + /// CacheDirective::Private, + /// CacheDirective::MaxAge(360u32), + /// CacheDirective::Extension("foo".to_owned(), Some("bar".to_owned())), + /// ])); + /// ``` + (CacheControl, header::CACHE_CONTROL) => (CacheDirective)+ -// TODO: this could just be the crate::http::header::common_header! macro -impl Header for CacheControl { - fn name() -> header::HeaderName { - header::CACHE_CONTROL - } + test_parse_and_format { + common_header_test!(no_headers, vec![b""; 0], None); + common_header_test!(empty_header, vec![b""; 1], None); + common_header_test!(bad_syntax, vec![b"foo="], None); - #[inline] - fn parse(msg: &T) -> Result - where - T: crate::HttpMessage, - { - let directives = from_comma_delimited(msg.headers().get_all(&Self::name()))?; - if !directives.is_empty() { - Ok(CacheControl(directives)) - } else { - Err(crate::error::ParseError::Header) + common_header_test!( + multiple_headers, + vec![&b"no-cache"[..], &b"private"[..]], + Some(CacheControl(vec![ + CacheDirective::NoCache, + CacheDirective::Private, + ])) + ); + + common_header_test!( + argument, + vec![b"max-age=100, private"], + Some(CacheControl(vec![ + CacheDirective::MaxAge(100), + CacheDirective::Private, + ])) + ); + + common_header_test!( + extension, + vec![b"foo, bar=baz"], + Some(CacheControl(vec![ + CacheDirective::Extension("foo".to_owned(), None), + CacheDirective::Extension("bar".to_owned(), Some("baz".to_owned())), + ])) + ); + + #[test] + fn parse_quote_form() { + let req = test::TestRequest::default() + .insert_header((header::CACHE_CONTROL, "max-age=\"200\"")) + .finish(); + + assert_eq!( + Header::parse(&req).ok(), + Some(CacheControl(vec![CacheDirective::MaxAge(200)])) + ) } } } -impl fmt::Display for CacheControl { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - fmt_comma_delimited(f, &self.0[..]) - } -} - -impl IntoHeaderValue for CacheControl { - type Error = header::InvalidHeaderValue; - - fn try_into_value(self) -> Result { - let mut writer = Writer::new(); - let _ = write!(&mut writer, "{}", self); - header::HeaderValue::from_maybe_shared(writer.take()) - } -} - /// `CacheControl` contains a list of these directives. #[derive(Debug, Clone, PartialEq, Eq)] pub enum CacheDirective { @@ -126,38 +131,40 @@ pub enum CacheDirective { impl fmt::Display for CacheDirective { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { use self::CacheDirective::*; - fmt::Display::fmt( - match *self { - NoCache => "no-cache", - NoStore => "no-store", - NoTransform => "no-transform", - OnlyIfCached => "only-if-cached", - MaxAge(secs) => return write!(f, "max-age={}", secs), - MaxStale(secs) => return write!(f, "max-stale={}", secs), - MinFresh(secs) => return write!(f, "min-fresh={}", secs), + let dir_str = match self { + NoCache => "no-cache", + NoStore => "no-store", + NoTransform => "no-transform", + OnlyIfCached => "only-if-cached", - MustRevalidate => "must-revalidate", - Public => "public", - Private => "private", - ProxyRevalidate => "proxy-revalidate", - SMaxAge(secs) => return write!(f, "s-maxage={}", secs), + MaxAge(secs) => return write!(f, "max-age={}", secs), + MaxStale(secs) => return write!(f, "max-stale={}", secs), + MinFresh(secs) => return write!(f, "min-fresh={}", secs), - Extension(ref name, None) => &name[..], - Extension(ref name, Some(ref arg)) => { - return write!(f, "{}={}", name, arg); - } - }, - f, - ) + MustRevalidate => "must-revalidate", + Public => "public", + Private => "private", + ProxyRevalidate => "proxy-revalidate", + SMaxAge(secs) => return write!(f, "s-maxage={}", secs), + + Extension(name, None) => name.as_str(), + Extension(name, Some(arg)) => return write!(f, "{}={}", name, arg), + }; + + f.write_str(dir_str) } } -impl FromStr for CacheDirective { - type Err = Option<::Err>; - fn from_str(s: &str) -> Result::Err>> { +impl str::FromStr for CacheDirective { + type Err = Option<::Err>; + + fn from_str(s: &str) -> Result { use self::CacheDirective::*; + match s { + "" => Err(None), + "no-cache" => Ok(NoCache), "no-store" => Ok(NoStore), "no-transform" => Ok(NoTransform), @@ -166,7 +173,7 @@ impl FromStr for CacheDirective { "public" => Ok(Public), "private" => Ok(Private), "proxy-revalidate" => Ok(ProxyRevalidate), - "" => Err(None), + _ => match s.find('=') { Some(idx) if idx + 1 < s.len() => { match (&s[..idx], (&s[idx + 1..]).trim_matches('"')) { @@ -183,76 +190,3 @@ impl FromStr for CacheDirective { } } } - -#[cfg(test)] -mod tests { - use super::*; - use crate::http::header::Header; - use actix_http::test::TestRequest; - - #[test] - fn test_parse_multiple_headers() { - let req = TestRequest::default() - .insert_header((header::CACHE_CONTROL, "no-cache, private")) - .finish(); - let cache = Header::parse(&req); - assert_eq!( - cache.ok(), - Some(CacheControl(vec![ - CacheDirective::NoCache, - CacheDirective::Private, - ])) - ) - } - - #[test] - fn test_parse_argument() { - let req = TestRequest::default() - .insert_header((header::CACHE_CONTROL, "max-age=100, private")) - .finish(); - let cache = Header::parse(&req); - assert_eq!( - cache.ok(), - Some(CacheControl(vec![ - CacheDirective::MaxAge(100), - CacheDirective::Private, - ])) - ) - } - - #[test] - fn test_parse_quote_form() { - let req = TestRequest::default() - .insert_header((header::CACHE_CONTROL, "max-age=\"200\"")) - .finish(); - let cache = Header::parse(&req); - assert_eq!( - cache.ok(), - Some(CacheControl(vec![CacheDirective::MaxAge(200)])) - ) - } - - #[test] - fn test_parse_extension() { - let req = TestRequest::default() - .insert_header((header::CACHE_CONTROL, "foo, bar=baz")) - .finish(); - let cache = Header::parse(&req); - assert_eq!( - cache.ok(), - Some(CacheControl(vec![ - CacheDirective::Extension("foo".to_owned(), None), - CacheDirective::Extension("bar".to_owned(), Some("baz".to_owned())), - ])) - ) - } - - #[test] - fn test_parse_bad_syntax() { - let req = TestRequest::default() - .insert_header((header::CACHE_CONTROL, "foo=")) - .finish(); - let cache: Result = Header::parse(&req); - assert_eq!(cache.ok(), None) - } -} diff --git a/src/http/header/encoding.rs b/src/http/header/encoding.rs index ce31c100f..a61edda67 100644 --- a/src/http/header/encoding.rs +++ b/src/http/header/encoding.rs @@ -4,26 +4,33 @@ pub use self::Encoding::{ Brotli, Chunked, Compress, Deflate, EncodingExt, Gzip, Identity, Trailers, Zstd, }; -/// A value to represent an encoding used in `Transfer-Encoding` -/// or `Accept-Encoding` header. -#[derive(Clone, PartialEq, Debug)] +/// A value to represent an encoding used in `Transfer-Encoding` or `Accept-Encoding` header. +#[derive(Debug, Clone, PartialEq, Eq)] pub enum Encoding { /// The `chunked` encoding. Chunked, + /// The `br` encoding. Brotli, + /// The `gzip` encoding. Gzip, + /// The `deflate` encoding. Deflate, + /// The `compress` encoding. Compress, + /// The `identity` encoding. Identity, + /// The `trailers` encoding. Trailers, + /// The `zstd` encoding. Zstd, + /// Some other encoding that is less common, can be any String. EncodingExt(String), } diff --git a/src/http/header/macros.rs b/src/http/header/macros.rs index d91d1d282..3f530658c 100644 --- a/src/http/header/macros.rs +++ b/src/http/header/macros.rs @@ -4,11 +4,14 @@ macro_rules! common_header_test_module { mod $tm { #![allow(unused_imports)] - use std::str; - use actix_http::http::Method; - use mime::*; - use $crate::http::header::*; + use ::core::str; + + use ::actix_http::{http::Method, test}; + use ::mime::*; + + use $crate::http::header::{self, *}; use super::{$id as HeaderField, *}; + $($tf)* } } @@ -19,22 +22,22 @@ macro_rules! common_header_test { ($id:ident, $raw:expr) => { #[test] fn $id() { - use actix_http::test; + use ::actix_http::test; let raw = $raw; - let a: Vec> = raw.iter().map(|x| x.to_vec()).collect(); + let headers = raw.iter().map(|x| x.to_vec()).collect::>(); let mut req = test::TestRequest::default(); - for item in a { - req = req.insert_header((HeaderField::name(), item)).take(); + for item in headers { + req = req.append_header((HeaderField::name(), item)).take(); } let req = req.finish(); let value = HeaderField::parse(&req); let result = format!("{}", value.unwrap()); - let expected = String::from_utf8(raw[0].to_vec()).unwrap(); + let expected = ::std::string::String::from_utf8(raw[0].to_vec()).unwrap(); let result_cmp: Vec = result .to_ascii_lowercase() @@ -56,14 +59,17 @@ macro_rules! common_header_test { fn $id() { use actix_http::test; - let a: Vec> = $raw.iter().map(|x| x.to_vec()).collect(); + let headers = $raw.iter().map(|x| x.to_vec()).collect::>(); let mut req = test::TestRequest::default(); - for item in a { - req.insert_header((HeaderField::name(), item)); + + for item in headers { + req.append_header((HeaderField::name(), item)); } + let req = req.finish(); let val = HeaderField::parse(&req); - let exp: Option = $exp; + + let exp: ::core::option::Option = $exp; // test parsing assert_eq!(val.ok(), exp); @@ -122,6 +128,7 @@ macro_rules! common_header { impl $crate::http::header::IntoHeaderValue for $id { type Error = $crate::http::header::InvalidHeaderValue; + #[inline] fn try_into_value(self) -> Result<$crate::http::header::HeaderValue, Self::Error> { use ::core::fmt::Write; let mut writer = $crate::http::header::Writer::new(); @@ -142,10 +149,19 @@ macro_rules! common_header { fn name() -> $crate::http::header::HeaderName { $name } + #[inline] - fn parse(msg: &M) -> Result { - $crate::http::header::from_comma_delimited( - msg.headers().get_all(Self::name())).map($id) + fn parse(msg: &M) -> Result{ + let headers = msg.headers().get_all(Self::name()); + + $crate::http::header::from_comma_delimited(headers) + .and_then(|items| { + if items.is_empty() { + Err($crate::error::ParseError::Header) + } else { + Ok($id(items)) + } + }) } } @@ -159,6 +175,7 @@ macro_rules! common_header { impl $crate::http::header::IntoHeaderValue for $id { type Error = $crate::http::header::InvalidHeaderValue; + #[inline] fn try_into_value(self) -> Result<$crate::http::header::HeaderValue, Self::Error> { use ::core::fmt::Write; let mut writer = $crate::http::header::Writer::new(); @@ -197,6 +214,7 @@ macro_rules! common_header { impl $crate::http::header::IntoHeaderValue for $id { type Error = $crate::http::header::InvalidHeaderValue; + #[inline] fn try_into_value(self) -> Result<$crate::http::header::HeaderValue, Self::Error> { self.0.try_into_value() } @@ -251,6 +269,7 @@ macro_rules! common_header { impl $crate::http::header::IntoHeaderValue for $id { type Error = $crate::http::header::InvalidHeaderValue; + #[inline] fn try_into_value(self) -> Result<$crate::http::header::HeaderValue, Self::Error> { use ::core::fmt::Write; let mut writer = $crate::http::header::Writer::new(); diff --git a/src/http/header/mod.rs b/src/http/header/mod.rs index 750f0e5b9..98548dadd 100644 --- a/src/http/header/mod.rs +++ b/src/http/header/mod.rs @@ -17,9 +17,9 @@ use bytes::{Bytes, BytesMut}; // - header parsing utils pub use actix_http::http::header::*; -mod accept_charset; -// mod accept_encoding; mod accept; +mod accept_charset; +mod accept_encoding; mod accept_language; mod allow; mod cache_control; @@ -46,9 +46,9 @@ mod preference; pub(crate) use macros::common_header_test; pub(crate) use macros::{common_header, common_header_test_module}; -pub use self::accept_charset::AcceptCharset; -//pub use self::accept_encoding::AcceptEncoding; pub use self::accept::Accept; +pub use self::accept_charset::AcceptCharset; +pub use self::accept_encoding::AcceptEncoding; pub use self::accept_language::AcceptLanguage; pub use self::allow::Allow; pub use self::cache_control::{CacheControl, CacheDirective};