fix: Handle bad redirects more gracefully (#7342)

cli/http_util.rs

@@ -2,7 +2,6 @@
 use crate::version;
 use bytes::Bytes;
 use deno_core::ErrBox;
-use futures::future::FutureExt;
 use reqwest::header::HeaderMap;
 use reqwest::header::HeaderValue;
 use reqwest::header::IF_NONE_MATCH;
@@ -92,77 +91,71 @@ pub enum FetchOnceResult {
 /// yields Code(ResultPayload).
 /// If redirect occurs, does not follow and
 /// yields Redirect(url).
-pub fn fetch_once(
+pub async fn fetch_once(
   client: Client,
   url: &Url,
   cached_etag: Option<String>,
-) -> impl Future<Output = Result<FetchOnceResult, ErrBox>> {
+) -> Result<FetchOnceResult, ErrBox> {
   let url = url.clone();
 
-  let fut = async move {
+  let mut request = client.get(url.clone());
-    let mut request = client.get(url.clone());
 
-    if let Some(etag) = cached_etag {
+  if let Some(etag) = cached_etag {
-      let if_none_match_val = HeaderValue::from_str(&etag).unwrap();
+    let if_none_match_val = HeaderValue::from_str(&etag).unwrap();
-      request = request.header(IF_NONE_MATCH, if_none_match_val);
+    request = request.header(IF_NONE_MATCH, if_none_match_val);
-    }
+  }
-    let response = request.send().await?;
+  let response = request.send().await?;
 
-    if response.status() == StatusCode::NOT_MODIFIED {
+  if response.status() == StatusCode::NOT_MODIFIED {
-      return Ok(FetchOnceResult::NotModified);
+    return Ok(FetchOnceResult::NotModified);
-    }
+  }
 
-    let mut headers_: HashMap<String, String> = HashMap::new();
+  let mut headers_: HashMap<String, String> = HashMap::new();
-    let headers = response.headers();
+  let headers = response.headers();
 
-    if let Some(warning) = headers.get("X-Deno-Warning") {
+  if let Some(warning) = headers.get("X-Deno-Warning") {
-      eprintln!(
+    eprintln!(
-        "{} {}",
+      "{} {}",
-        crate::colors::yellow("Warning"),
+      crate::colors::yellow("Warning"),
-        warning.to_str().unwrap()
+      warning.to_str().unwrap()
-      );
+    );
-    }
+  }
 
-    for key in headers.keys() {
+  for key in headers.keys() {
-      let key_str = key.to_string();
+    let key_str = key.to_string();
-      let values = headers.get_all(key);
+    let values = headers.get_all(key);
-      let values_str = values
+    let values_str = values
-        .iter()
+      .iter()
-        .map(|e| e.to_str().unwrap().to_string())
+      .map(|e| e.to_str().unwrap().to_string())
-        .collect::<Vec<String>>()
+      .collect::<Vec<String>>()
-        .join(",");
+      .join(",");
-      headers_.insert(key_str, values_str);
+    headers_.insert(key_str, values_str);
-    }
+  }
-
-    if response.status().is_redirection() {
-      let location_string = response
-        .headers()
-        .get(LOCATION)
-        .expect("url redirection should provide 'location' header")
-        .to_str()
-        .unwrap();
 
+  if response.status().is_redirection() {
+    if let Some(location) = response.headers().get(LOCATION) {
+      let location_string = location.to_str().unwrap();
       debug!("Redirecting to {:?}...", &location_string);
       let new_url = resolve_url_from_location(&url, location_string);
       return Ok(FetchOnceResult::Redirect(new_url, headers_));
+    } else {
+      return Err(ErrBox::error(format!(
+        "Redirection from '{}' did not provide location header",
+        url
+      )));
     }
+  }
 
-    if response.status().is_client_error()
+  if response.status().is_client_error() || response.status().is_server_error()
-      || response.status().is_server_error()
+  {
-    {
+    let err =
-      let err = io::Error::new(
+      ErrBox::error(format!("Import '{}' failed: {}", &url, response.status()));
-        io::ErrorKind::Other,
+    return Err(err);
-        format!("Import '{}' failed: {}", &url, response.status()),
+  }
-      );
-      return Err(err.into());
-    }
 
-    let body = response.bytes().await?.to_vec();
+  let body = response.bytes().await?.to_vec();
 
-    return Ok(FetchOnceResult::Code(body, headers_));
+  Ok(FetchOnceResult::Code(body, headers_))
-  };
-
-  fut.boxed()
 }
 
 /// Wraps reqwest `Response` so that it can be exposed as an `AsyncRead` and integrated
@@ -500,4 +493,17 @@ mod tests {
       panic!();
     }
   }
+
+  #[tokio::test]
+  async fn bad_redirect() {
+    let _g = test_util::http_server();
+    let url_str = "http://127.0.0.1:4545/bad_redirect";
+    let url = Url::parse(url_str).unwrap();
+    let client = create_http_client(None).unwrap();
+    let result = fetch_once(client, &url, None).await;
+    assert!(result.is_err());
+    let err = result.unwrap_err();
+    // Check that the error message contains the original URL
+    assert!(err.to_string().contains(url_str));
+  }
 }

test_util/src/lib.rs

@@ -264,6 +264,11 @@ pub async fn run_all_servers() {
       );
       Box::new(res)
     });
+  let bad_redirect = warp::path("bad_redirect").map(|| -> Box<dyn Reply> {
+    let mut res = Response::new(Body::from(""));
+    *res.status_mut() = StatusCode::FOUND;
+    Box::new(res)
+  });
 
   let etag_script = warp::path!("etag_script.ts")
     .and(warp::header::optional::<String>("if-none-match"))
@@ -404,7 +409,8 @@ pub async fn run_all_servers() {
     .or(xtypescripttypes)
     .or(echo_server)
     .or(echo_multipart_file)
-    .or(multipart_form_data);
+    .or(multipart_form_data)
+    .or(bad_redirect);
 
   let http_fut =
     warp::serve(content_type_handler.clone()).bind(([127, 0, 0, 1], PORT));