diff --git a/actix-http/CHANGES.md b/actix-http/CHANGES.md index 5775c17d344..6f41fb77d59 100644 --- a/actix-http/CHANGES.md +++ b/actix-http/CHANGES.md @@ -2,6 +2,11 @@ ## Unreleased +- Parse all HTTP/1 `Connection` header options consistently. [#2692] +- Only treat `Expect: 100-continue` as a continue expectation in HTTP/1.1 requests. + +[#2692]: https://github.com/actix/actix-web/issues/2692 + ## 3.13.5 - Reject invalid WebSocket close-frame status codes, malformed payloads, and invalid UTF-8 close reasons. diff --git a/actix-http/src/builder.rs b/actix-http/src/builder.rs index 001810a8cd5..1535948c76f 100644 --- a/actix-http/src/builder.rs +++ b/actix-http/src/builder.rs @@ -206,7 +206,7 @@ where /// Provide service for `EXPECT: 100-Continue` support. /// - /// Service get called with request that contains `EXPECT` header. + /// Service get called with request that contains `EXPECT: 100-Continue` header. /// Service must return request in case of success, in that case /// request will be forwarded to main service. pub fn expect(self, expect: F) -> HttpServiceBuilder diff --git a/actix-http/src/encoding/encoder.rs b/actix-http/src/encoding/encoder.rs index aa730cfaf8a..9854f01828f 100644 --- a/actix-http/src/encoding/encoder.rs +++ b/actix-http/src/encoding/encoder.rs @@ -366,7 +366,7 @@ impl ContentEncoder { ContentEncoder::Brotli(ref mut encoder) => match encoder.write_all(data) { Ok(_) => Ok(()), Err(err) => { - trace!("Error decoding br encoding: {}", err); + trace!("Error encoding br data: {}", err); Err(err) } }, @@ -375,7 +375,7 @@ impl ContentEncoder { ContentEncoder::Gzip(ref mut encoder) => match encoder.write_all(data) { Ok(_) => Ok(()), Err(err) => { - trace!("Error decoding gzip encoding: {}", err); + trace!("Error encoding gzip data: {}", err); Err(err) } }, @@ -384,7 +384,7 @@ impl ContentEncoder { ContentEncoder::Deflate(ref mut encoder) => match encoder.write_all(data) { Ok(_) => Ok(()), Err(err) => { - trace!("Error decoding deflate encoding: {}", err); + trace!("Error encoding deflate data: {}", err); Err(err) } }, @@ -393,7 +393,7 @@ impl ContentEncoder { ContentEncoder::Zstd(ref mut encoder) => match encoder.write_all(data) { Ok(_) => Ok(()), Err(err) => { - trace!("Error decoding ztsd encoding: {}", err); + trace!("Error encoding zstd data: {}", err); Err(err) } }, diff --git a/actix-http/src/h1/decoder.rs b/actix-http/src/h1/decoder.rs index 5170ea4a1f9..70426341922 100644 --- a/actix-http/src/h1/decoder.rs +++ b/actix-http/src/h1/decoder.rs @@ -151,19 +151,21 @@ pub(crate) trait MessageType: Sized { // connection keep-alive state header::CONNECTION => { - ka = if let Ok(conn) = value.to_str().map(str::trim) { - if conn.eq_ignore_ascii_case("keep-alive") { - Some(ConnectionType::KeepAlive) - } else if conn.eq_ignore_ascii_case("close") { - Some(ConnectionType::Close) - } else if conn.eq_ignore_ascii_case("upgrade") { - Some(ConnectionType::Upgrade) - } else { - None + if let Ok(conn) = value.to_str() { + for option in conn.split(',').map(str::trim) { + if option.eq_ignore_ascii_case("close") { + ka = Some(ConnectionType::Close); + break; + } else if option.eq_ignore_ascii_case("upgrade") + && ka != Some(ConnectionType::Close) + { + ka = Some(ConnectionType::Upgrade); + } else if option.eq_ignore_ascii_case("keep-alive") && ka.is_none() + { + ka = Some(ConnectionType::KeepAlive); + } } - } else { - None - }; + } } header::UPGRADE => { @@ -174,11 +176,27 @@ pub(crate) trait MessageType: Sized { } } - header::EXPECT => { - let bytes = value.as_bytes(); - if bytes.len() >= 4 && &bytes[0..4] == b"100-" { - expect = true; - } + header::EXPECT if version == Version::HTTP_11 => { + let mut quoted = false; + let mut escaped = false; + expect = expect + || value + .as_bytes() + .split(|&byte| { + if escaped { + escaped = false; + } else if quoted && byte == b'\\' { + escaped = true; + } else if byte == b'"' { + quoted = !quoted; + } else { + return byte == b',' && !quoted; + } + false + }) + .any(|item| { + item.trim_ascii().eq_ignore_ascii_case(b"100-continue") + }); } _ => {} @@ -848,6 +866,30 @@ mod tests { assert_eq!(val[1], "c2=cookie2"); } + #[test] + fn test_expect_100_continue() { + for (value, expected) in [ + ("100-custom, 100-Continue", true), + ("100-custom", false), + (r#"custom="foo, 100-continue, bar""#, false), + (r#"custom="foo\", 100-continue, bar""#, false), + (r#"custom="foo\\", 100-Continue"#, true), + (r#"custom="é, bar", 100-Continue"#, true), + ] { + let raw = + format!("POST /test HTTP/1.1\r\ncontent-length: 1\r\nexpect: {value}\r\n\r\n"); + let req = parse_ready!(&mut BytesMut::from(raw.as_str())); + assert_eq!(req.head().expect(), expected, "{value:?}"); + } + + let req = parse_ready!(&mut BytesMut::from( + "POST /test HTTP/1.0\r\n\ + content-length: 1\r\n\ + expect: 100-continue\r\n\r\n", + )); + assert!(!req.head().expect()); + } + #[test] fn test_conn_default_1_0() { let req = parse_ready!(&mut BytesMut::from("GET /test HTTP/1.0\r\n\r\n")); @@ -947,6 +989,29 @@ mod tests { assert_eq!(req.head().connection_type(), ConnectionType::Upgrade); } + #[test] + fn test_conn_multi_value() { + for (connection, expected) in [ + ("keep-alive, Upgrade", ConnectionType::Upgrade), + ("keep-alive\r\nconnection: Upgrade", ConnectionType::Upgrade), + ("close, upgrade", ConnectionType::Close), + ("upgrade, close", ConnectionType::Close), + ("close\r\nconnection: upgrade", ConnectionType::Close), + ("upgrade\r\nconnection: close", ConnectionType::Close), + ("not-upgrade", ConnectionType::KeepAlive), + ] { + let raw = format!("GET /test HTTP/1.1\r\nconnection: {connection}\r\n\r\n"); + let req = parse_ready!(&mut BytesMut::from(raw.as_str())); + assert_eq!(req.head().connection_type(), expected, "{connection:?}"); + assert_eq!( + req.upgrade(), + expected == ConnectionType::Upgrade, + "{connection:?}" + ); + assert_eq!(req.head().upgrade(), req.upgrade(), "{connection:?}"); + } + } + #[test] fn test_conn_upgrade_connect_method() { let req = parse_ready!(&mut BytesMut::from( @@ -1032,12 +1097,7 @@ mod tests { ); let mut reader = MessageDecoder::::default(); let (req, pl) = reader.decode(&mut buf).unwrap().unwrap(); - // `connection: upgrade, http2-settings` doesn't work properly.. - // see MessageType::set_headers(). - // - // The line below should be: - // assert_eq!(req.head().connection_type(), ConnectionType::Upgrade); - assert_eq!(req.head().connection_type(), ConnectionType::KeepAlive); + assert_eq!(req.head().connection_type(), ConnectionType::Upgrade); assert!(req.upgrade()); assert!(!pl.is_unhandled()); } diff --git a/actix-http/src/requests/head.rs b/actix-http/src/requests/head.rs index ddc9dd98f3c..a87de554e7d 100644 --- a/actix-http/src/requests/head.rs +++ b/actix-http/src/requests/head.rs @@ -107,16 +107,21 @@ impl RequestHead { /// Connection upgrade status pub fn upgrade(&self) -> bool { - self.headers() - .get(header::CONNECTION) - .map(|hdr| { - if let Ok(s) = hdr.to_str() { - s.to_ascii_lowercase().contains("upgrade") - } else { - false + let mut upgrade = false; + + for conn in self.headers().get_all(header::CONNECTION) { + if let Ok(conn) = conn.to_str() { + for option in conn.split(',').map(str::trim) { + if option.eq_ignore_ascii_case("close") { + return false; + } else if option.eq_ignore_ascii_case("upgrade") { + upgrade = true; + } } - }) - .unwrap_or(false) + } + } + + upgrade } #[inline] diff --git a/actix-http/src/requests/request.rs b/actix-http/src/requests/request.rs index 6a267a7a6cc..5e736cbc0f3 100644 --- a/actix-http/src/requests/request.rs +++ b/actix-http/src/requests/request.rs @@ -160,9 +160,9 @@ impl

Request

{ /// Check if request requires connection upgrade #[inline] pub fn upgrade(&self) -> bool { - if let Some(conn) = self.head().headers.get(header::CONNECTION) { - if let Ok(s) = conn.to_str() { - return s.to_lowercase().contains("upgrade"); + for conn in self.head().headers.get_all(header::CONNECTION) { + if conn.to_str().is_ok() { + return self.head().upgrade(); } } self.head().method == Method::CONNECT diff --git a/actix-http/src/service.rs b/actix-http/src/service.rs index 2edf8b7d0d8..4c4881c07eb 100644 --- a/actix-http/src/service.rs +++ b/actix-http/src/service.rs @@ -139,8 +139,8 @@ where { /// Sets service for `Expect: 100-Continue` handling. /// - /// An expect service is called with requests that contain an `Expect` header. A successful - /// response type is also a request which will be forwarded to the main service. + /// An expect service is called with requests that contain an `Expect: 100-Continue` header. A + /// successful response type is also a request which will be forwarded to the main service. pub fn expect(self, expect: X1) -> HttpService where X1: ServiceFactory, diff --git a/actix-http/src/ws/mod.rs b/actix-http/src/ws/mod.rs index 5dd543c4091..4e6ade4b3c4 100644 --- a/actix-http/src/ws/mod.rs +++ b/actix-http/src/ws/mod.rs @@ -331,8 +331,9 @@ mod tests { )) .insert_header(( header::CONNECTION, - header::HeaderValue::from_static("upgrade"), + header::HeaderValue::from_static("keep-alive"), )) + .append_header((header::CONNECTION, "Upgrade")) .insert_header(( header::SEC_WEBSOCKET_VERSION, header::HeaderValue::from_static("13"), @@ -344,7 +345,7 @@ mod tests { .finish(); assert_eq!( StatusCode::SWITCHING_PROTOCOLS, - handshake_response(req.head()).finish().status() + handshake(req.head()).unwrap().finish().status() ); } diff --git a/actix-web/src/http/header/last_modified.rs b/actix-web/src/http/header/last_modified.rs index 724a38bbc0b..fe6c70a338a 100644 --- a/actix-web/src/http/header/last_modified.rs +++ b/actix-web/src/http/header/last_modified.rs @@ -11,7 +11,7 @@ crate::http::header::common_header! { /// /// # ABNF /// ```plain - /// Expires = HTTP-date + /// Last-Modified = HTTP-date /// ``` /// /// # Example Values diff --git a/awc/CHANGES.md b/awc/CHANGES.md index 9888c66c365..6c42c7c2037 100644 --- a/awc/CHANGES.md +++ b/awc/CHANGES.md @@ -2,6 +2,7 @@ ## Unreleased +- Parse all `Connection` header options when validating WebSocket handshakes. - Add camel-case header controls to `WebsocketsRequest` via `camel_case_headers()` and `set_camel_case_headers()`. [#3953] - Update `hickory-resolver` dependency to `0.26.1`. - Update `rand` dependency to `0.10`. diff --git a/awc/src/ws.rs b/awc/src/ws.rs index 65ef8b04040..0cdbf6918e2 100644 --- a/awc/src/ws.rs +++ b/awc/src/ws.rs @@ -385,19 +385,27 @@ impl WebsocketsRequest { } // Check for "CONNECTION" header - if let Some(conn) = head.headers.get(&header::CONNECTION) { - if let Ok(s) = conn.to_str() { - if !s.to_ascii_lowercase().contains("upgrade") { - log::trace!("Invalid connection header: {}", s); - return Err(WsClientError::InvalidConnectionHeader(conn.clone())); + let mut upgrade = false; + for conn in head.headers.get_all(header::CONNECTION) { + if let Ok(value) = conn.to_str() { + for option in value.split(',').map(str::trim) { + if option.eq_ignore_ascii_case("close") { + log::trace!("Invalid connection header: {:?}", conn); + return Err(WsClientError::InvalidConnectionHeader(conn.clone())); + } else if option.eq_ignore_ascii_case("upgrade") { + upgrade = true; + } } - } else { + } + } + if !upgrade { + if let Some(conn) = head.headers.get(header::CONNECTION) { log::trace!("Invalid connection header: {:?}", conn); return Err(WsClientError::InvalidConnectionHeader(conn.clone())); + } else { + log::trace!("Missing connection header"); + return Err(WsClientError::MissingConnectionHeader); } - } else { - log::trace!("Missing connection header"); - return Err(WsClientError::MissingConnectionHeader); } if let Some(hdr_key) = head.headers.get(&header::SEC_WEBSOCKET_ACCEPT) {