diff --git a/actix-web/CHANGES.md b/actix-web/CHANGES.md index ba53e912a..59901dfd4 100644 --- a/actix-web/CHANGES.md +++ b/actix-web/CHANGES.md @@ -4,6 +4,7 @@ - Panic when calling `Route::to()` or `Route::service()` after `Route::wrap()` to prevent silently dropping route middleware. [#3944] - Fix `HttpRequest::{match_pattern,match_name}` reporting path-only matches when route guards disambiguate overlapping resources. [#3346] +- Fix `Readlines` handling of lines split across payload chunks so combined line limits are enforced and complete lines are yielded. [#3944]: https://github.com/actix/actix-web/pull/3944 [#3346]: https://github.com/actix/actix-web/issues/3346 diff --git a/actix-web/src/types/readlines.rs b/actix-web/src/types/readlines.rs index e75239968..17998a1a3 100644 --- a/actix-web/src/types/readlines.rs +++ b/actix-web/src/types/readlines.rs @@ -65,6 +65,23 @@ where err: Some(err), } } + + /// Decodes one complete logical line using the request's configured encoding. + /// + /// Callers are expected to pass only the bytes that belong to the line being yielded, + /// whether they came from the internal buffer, the current payload chunk, or both. + fn decode(encoding: &'static Encoding, bytes: &[u8]) -> Result { + if encoding == UTF_8 { + str::from_utf8(bytes) + .map_err(|_| ReadlinesError::EncodingError) + .map(str::to_owned) + } else { + encoding + .decode_without_bom_handling_and_without_replacement(bytes) + .map(Cow::into_owned) + .ok_or(ReadlinesError::EncodingError) + } + } } impl Stream for Readlines @@ -95,18 +112,7 @@ where if ind + 1 > this.limit { return Poll::Ready(Some(Err(ReadlinesError::LimitOverflow))); } - let line = if this.encoding == UTF_8 { - str::from_utf8(&this.buf.split_to(ind + 1)) - .map_err(|_| ReadlinesError::EncodingError)? - .to_owned() - } else { - this.encoding - .decode_without_bom_handling_and_without_replacement( - &this.buf.split_to(ind + 1), - ) - .map(Cow::into_owned) - .ok_or(ReadlinesError::EncodingError)? - }; + let line = Self::decode(this.encoding, &this.buf.split_to(ind + 1))?; return Poll::Ready(Some(Ok(line))); } this.checked_buff = true; @@ -125,24 +131,17 @@ where } if let Some(ind) = found { // check if line is longer than limit - if ind + 1 > this.limit { + if this.buf.len() + ind + 1 > this.limit { return Poll::Ready(Some(Err(ReadlinesError::LimitOverflow))); } - let line = if this.encoding == UTF_8 { - str::from_utf8(&bytes.split_to(ind + 1)) - .map_err(|_| ReadlinesError::EncodingError)? - .to_owned() - } else { - this.encoding - .decode_without_bom_handling_and_without_replacement( - &bytes.split_to(ind + 1), - ) - .map(Cow::into_owned) - .ok_or(ReadlinesError::EncodingError)? - }; - // extend buffer with rest of the bytes; + + this.buf.extend_from_slice(&bytes.split_to(ind + 1)); + let line = Self::decode(this.encoding, &this.buf)?; + this.buf.clear(); + + // buffer bytes following the returned line this.buf.extend_from_slice(&bytes); - this.checked_buff = false; + this.checked_buff = this.buf.is_empty(); return Poll::Ready(Some(Ok(line))); } this.buf.extend_from_slice(&bytes); @@ -156,16 +155,7 @@ where if this.buf.len() > this.limit { return Poll::Ready(Some(Err(ReadlinesError::LimitOverflow))); } - let line = if this.encoding == UTF_8 { - str::from_utf8(&this.buf) - .map_err(|_| ReadlinesError::EncodingError)? - .to_owned() - } else { - this.encoding - .decode_without_bom_handling_and_without_replacement(&this.buf) - .map(Cow::into_owned) - .ok_or(ReadlinesError::EncodingError)? - }; + let line = Self::decode(this.encoding, &this.buf)?; this.buf.clear(); Poll::Ready(Some(Ok(line))) } @@ -177,10 +167,16 @@ where #[cfg(test)] mod tests { - use futures_util::StreamExt as _; + use std::{ + pin::Pin, + task::{Context, Poll}, + }; + + use actix_http::{h1, Request}; + use futures_util::{task::noop_waker_ref, StreamExt as _}; use super::*; - use crate::test::TestRequest; + use crate::{error::ReadlinesError, test::TestRequest}; #[actix_rt::test] async fn test_readlines() { @@ -208,4 +204,46 @@ mod tests { "Contrary to popular belief, Lorem Ipsum is not simply random text." ); } + + #[test] + fn test_readlines_limit_across_chunks() { + let (mut sender, payload) = h1::Payload::create(false); + let payload: actix_http::Payload = payload.into(); + let mut req = Request::with_payload(payload); + let mut stream = Readlines::new(&mut req).limit(10); + let mut cx = Context::from_waker(noop_waker_ref()); + + sender.feed_data(Bytes::from_static(b"AAAAAAAAAA")); + assert!(matches!( + Pin::new(&mut stream).poll_next(&mut cx), + Poll::Pending + )); + + sender.feed_data(Bytes::from_static(b"A\n")); + assert!(matches!( + Pin::new(&mut stream).poll_next(&mut cx), + Poll::Ready(Some(Err(ReadlinesError::LimitOverflow))) + )); + } + + #[test] + fn test_readlines_returns_full_line_across_chunks() { + let (mut sender, payload) = h1::Payload::create(false); + let payload: actix_http::Payload = payload.into(); + let mut req = Request::with_payload(payload); + let mut stream = Readlines::new(&mut req); + let mut cx = Context::from_waker(noop_waker_ref()); + + sender.feed_data(Bytes::from_static(b"hello ")); + assert!(matches!( + Pin::new(&mut stream).poll_next(&mut cx), + Poll::Pending + )); + + sender.feed_data(Bytes::from_static(b"world\nnext")); + assert!(matches!( + Pin::new(&mut stream).poll_next(&mut cx), + Poll::Ready(Some(Ok(ref line))) if line == "hello world\n" + )); + } }