From 30e2a2b62bbca0c56ea22bbc7100d8466b6bd0cc Mon Sep 17 00:00:00 2001 From: Matthias Endler Date: Fri, 10 Mar 2023 15:15:37 +0100 Subject: [PATCH] Fix `--max-redirects` (#987) Having more than the max number of redirects caused lychee to abort the requests, but did not lead to an error. Related: https://github.com/lycheeverse/lychee-action/issues/164 --- lychee-bin/src/options.rs | 9 ++--- lychee-bin/tests/cli.rs | 15 ++++++++ lychee-lib/src/client.rs | 65 ++++++++++++++++++++++++++++++++++ lychee-lib/src/types/error.rs | 4 +++ lychee-lib/src/types/status.rs | 2 ++ 5 files changed, 88 insertions(+), 7 deletions(-) diff --git a/lychee-bin/src/options.rs b/lychee-bin/src/options.rs index c06ed19..64f12d3 100644 --- a/lychee-bin/src/options.rs +++ b/lychee-bin/src/options.rs @@ -42,8 +42,9 @@ const HELP_MSG_CONFIG_FILE: &str = formatcp!( const TIMEOUT_STR: &str = concatcp!(DEFAULT_TIMEOUT_SECS); const RETRY_WAIT_TIME_STR: &str = concatcp!(DEFAULT_RETRY_WAIT_TIME_SECS); -#[derive(Debug, Deserialize, Clone)] +#[derive(Debug, Deserialize, Default, Clone)] pub(crate) enum Format { + #[default] Compact, Detailed, Json, @@ -65,12 +66,6 @@ impl FromStr for Format { } } -impl Default for Format { - fn default() -> Self { - Format::Compact - } -} - // Macro for generating default functions to be used by serde macro_rules! default_function { ( $( $name:ident : $T:ty = $e:expr; )* ) => { diff --git a/lychee-bin/tests/cli.rs b/lychee-bin/tests/cli.rs index 572e626..f5ca5d3 100644 --- a/lychee-bin/tests/cli.rs +++ b/lychee-bin/tests/cli.rs @@ -1159,4 +1159,19 @@ mod cli { Ok(()) } + + #[test] + fn test_prevent_too_many_redirects() -> Result<()> { + let mut cmd = main_command(); + let url = "https://httpstat.us/308"; + + cmd.write_stdin(url) + .arg("--max-redirects") + .arg("0") + .arg("-") + .assert() + .failure(); + + Ok(()) + } } diff --git a/lychee-lib/src/client.rs b/lychee-lib/src/client.rs index 0e205fa..99b9eb9 100644 --- a/lychee-lib/src/client.rs +++ b/lychee-lib/src/client.rs @@ -643,6 +643,7 @@ mod tests { use http::{header::HeaderMap, StatusCode}; use reqwest::header; use tempfile::tempdir; + use wiremock::matchers::path; use super::ClientBuilder; use crate::{mock_server, test_utils::get_mock_client_response, Uri}; @@ -827,4 +828,68 @@ mod tests { let res = client.check("http://\"").await.unwrap(); assert!(res.status().is_failure()); } + + #[tokio::test] + async fn test_max_redirects() { + let mock_server = wiremock::MockServer::start().await; + + let ok_uri = format!("{}/ok", &mock_server.uri()); + let redirect_uri = format!("{}/redirect", &mock_server.uri()); + + // Set up permanent redirect loop + let redirect = wiremock::ResponseTemplate::new(StatusCode::PERMANENT_REDIRECT) + .insert_header("Location", ok_uri.as_str()); + wiremock::Mock::given(wiremock::matchers::method("GET")) + .and(path("/redirect")) + .respond_with(redirect) + .mount(&mock_server) + .await; + + let ok = wiremock::ResponseTemplate::new(StatusCode::OK); + wiremock::Mock::given(wiremock::matchers::method("GET")) + .and(path("/ok")) + .respond_with(ok) + .mount(&mock_server) + .await; + + let client = ClientBuilder::builder() + .max_redirects(1_usize) + .build() + .client() + .unwrap(); + + let res = client.check(redirect_uri.clone()).await.unwrap(); + assert!(res.status().is_failure()); + + let client = ClientBuilder::builder() + .max_redirects(2_usize) + .build() + .client() + .unwrap(); + + let res = client.check(redirect_uri).await.unwrap(); + assert!(res.status().is_success()); + } + + #[tokio::test] + async fn test_limit_max_redirects() { + let mock_server = wiremock::MockServer::start().await; + + // Set up permanent redirect loop + let template = wiremock::ResponseTemplate::new(StatusCode::PERMANENT_REDIRECT) + .insert_header("Location", mock_server.uri().as_str()); + wiremock::Mock::given(wiremock::matchers::method("GET")) + .respond_with(template) + .mount(&mock_server) + .await; + + let client = ClientBuilder::builder() + .max_redirects(0_usize) + .build() + .client() + .unwrap(); + + let res = client.check(mock_server.uri()).await.unwrap(); + assert!(res.status().is_failure()); + } } diff --git a/lychee-lib/src/types/error.rs b/lychee-lib/src/types/error.rs index 6d54ad1..d0aa196 100644 --- a/lychee-lib/src/types/error.rs +++ b/lychee-lib/src/types/error.rs @@ -99,6 +99,9 @@ pub enum ErrorKind { /// Regex error #[error("Error when using regex engine: {0}")] Regex(#[from] regex::Error), + /// Too many redirects (HTTP 3xx) were encountered (configurable) + #[error("Too many redirects")] + TooManyRedirects(#[source] reqwest::Error), } impl ErrorKind { @@ -210,6 +213,7 @@ impl Hash for ErrorKind { std::mem::discriminant(self).hash(state); } Self::Regex(e) => e.to_string().hash(state), + Self::TooManyRedirects(e) => e.to_string().hash(state), } } } diff --git a/lychee-lib/src/types/status.rs b/lychee-lib/src/types/status.rs index 2abaf16..1c56032 100644 --- a/lychee-lib/src/types/status.rs +++ b/lychee-lib/src/types/status.rs @@ -242,6 +242,8 @@ impl From for Status { fn from(e: reqwest::Error) -> Self { if e.is_timeout() { Self::Timeout(e.status()) + } else if e.is_redirect() { + Self::Error(ErrorKind::TooManyRedirects(e)) } else if e.is_builder() { Self::Unsupported(ErrorKind::BuildRequestClient(e)) } else if e.is_body() || e.is_decode() {