fix(npm): respect etag for npm packument caching

Cargo.toml

@@ -65,7 +65,7 @@ deno_lint = "=0.74.0"
 deno_lockfile = "=0.28.0"
 deno_media_type = { version = "=0.2.8", features = ["module_specifier"] }
 deno_native_certs = "0.3.0"
-deno_npm = "=0.33.0"
+deno_npm = "=0.33.1"
 deno_package_json = { version = "=0.6.0", default-features = false }
 deno_path_util = "=0.3.2"
 deno_semver = "=0.7.1"
@@ -225,10 +225,10 @@ saffron = "=0.1.0"
 same-file = "1.0.6"
 scopeguard = "1.2.0"
 semver = "=1.0.25"
-serde = { version = "1.0.149", features = ["derive"] }
+serde = { version = "1.0.219", features = ["derive"] }
 serde-value = "0.7"
 serde_bytes = "0.11"
-serde_json = "1.0.85"
+serde_json = "1.0.140"
 serde_repr = "=0.1.19"
 simd-json = "0.14.0"
 slab = "0.4"
@@ -516,3 +516,6 @@ opt-level = 3
 opt-level = 3
 [profile.release.package.deno_npm_cache]
 opt-level = 3
+
+[patch.crates-io]
+deno_npm = { path = "../deno_npm" }

cli/http_util.rs

@@ -136,11 +136,46 @@ pub enum DownloadErrorKind {
   #[class("Http")]
   #[error("Not Found.")]
   NotFound,
+  #[class("Http")]
+  #[error("Received unhandled Not Modified response.")]
+  UnhandledNotModified,
   #[class(inherit)]
   #[error(transparent)]
   Other(JsErrorBox),
 }
 
+#[derive(Debug)]
+pub enum HttpClientResponse {
+  Success {
+    headers: HeaderMap<HeaderValue>,
+    body: Vec<u8>,
+  },
+  NotFound,
+  NotModified,
+}
+
+impl HttpClientResponse {
+  pub fn into_bytes(self) -> Result<Vec<u8>, DownloadError> {
+    match self {
+      Self::Success { body, .. } => Ok(body),
+      Self::NotFound => Err(DownloadErrorKind::NotFound.into_box()),
+      Self::NotModified => {
+        Err(DownloadErrorKind::UnhandledNotModified.into_box())
+      }
+    }
+  }
+
+  pub fn into_maybe_bytes(self) -> Result<Option<Vec<u8>>, DownloadError> {
+    match self {
+      Self::Success { body, .. } => Ok(Some(body)),
+      Self::NotFound => Ok(None),
+      Self::NotModified => {
+        Err(DownloadErrorKind::UnhandledNotModified.into_box())
+      }
+    }
+  }
+}
+
 #[derive(Debug)]
 pub struct HttpClient {
   client: deno_fetch::Client,
@@ -226,27 +261,18 @@ impl HttpClient {
   }
 
   pub async fn download(&self, url: Url) -> Result<Vec<u8>, DownloadError> {
-    let maybe_bytes = self.download_inner(url, None, None).await?;
-    match maybe_bytes {
-      Some(bytes) => Ok(bytes),
-      None => Err(DownloadErrorKind::NotFound.into_box()),
-    }
+    let response = self.download_inner(url, &Default::default(), None).await?;
+    response.into_bytes()
   }
 
   pub async fn download_with_progress_and_retries(
     &self,
     url: Url,
-    maybe_header: Option<(HeaderName, HeaderValue)>,
+    headers: &HeaderMap,
     progress_guard: &UpdateGuard,
-  ) -> Result<Option<Vec<u8>>, DownloadError> {
+  ) -> Result<HttpClientResponse, DownloadError> {
     crate::util::retry::retry(
-      || {
-        self.download_inner(
-          url.clone(),
-          maybe_header.clone(),
-          Some(progress_guard),
-        )
-      },
+      || self.download_inner(url.clone(), headers, Some(progress_guard)),
       |e| {
         matches!(
           e.as_kind(),
@@ -260,22 +286,24 @@ impl HttpClient {
   pub async fn get_redirected_url(
     &self,
     url: Url,
-    maybe_header: Option<(HeaderName, HeaderValue)>,
+    headers: &HeaderMap<HeaderValue>,
   ) -> Result<Url, AnyError> {
-    let (_, url) = self.get_redirected_response(url, maybe_header).await?;
+    let (_, url) = self.get_redirected_response(url, headers).await?;
     Ok(url)
   }
 
   async fn download_inner(
     &self,
     url: Url,
-    maybe_header: Option<(HeaderName, HeaderValue)>,
+    headers: &HeaderMap<HeaderValue>,
     progress_guard: Option<&UpdateGuard>,
-  ) -> Result<Option<Vec<u8>>, DownloadError> {
-    let (response, _) = self.get_redirected_response(url, maybe_header).await?;
+  ) -> Result<HttpClientResponse, DownloadError> {
+    let (response, _) = self.get_redirected_response(url, headers).await?;
 
     if response.status() == 404 {
-      return Ok(None);
+      return Ok(HttpClientResponse::NotFound);
+    } else if response.status() == 304 {
+      return Ok(HttpClientResponse::NotModified);
     } else if !response.status().is_success() {
       let status = response.status();
       let maybe_response_text = body_to_string(response).await.ok();
@@ -292,38 +320,35 @@ impl HttpClient {
 
     get_response_body_with_progress(response, progress_guard)
       .await
-      .map(|(_, body)| Some(body))
+      .map(|(headers, body)| HttpClientResponse::Success { headers, body })
       .map_err(|err| DownloadErrorKind::Other(err).into_box())
   }
 
   async fn get_redirected_response(
     &self,
     mut url: Url,
-    mut maybe_header: Option<(HeaderName, HeaderValue)>,
+    headers: &HeaderMap<HeaderValue>,
   ) -> Result<(http::Response<deno_fetch::ResBody>, Url), DownloadError> {
     let mut req = self.get(url.clone())?.build();
-    if let Some((header_name, header_value)) = maybe_header.as_ref() {
-      req.headers_mut().append(header_name, header_value.clone());
-    }
+    *req.headers_mut() = headers.clone();
     let mut response = self
       .client
       .clone()
       .send(req)
       .await
       .map_err(|e| DownloadErrorKind::Fetch(e).into_box())?;
     let status = response.status();
-    if status.is_redirection() {
+    if status.is_redirection() && status != http::StatusCode::NOT_MODIFIED {
       for _ in 0..5 {
         let new_url = resolve_redirect_from_response(&url, &response)?;
         let mut req = self.get(new_url.clone())?.build();
 
-        if new_url.origin() == url.origin() {
-          if let Some((header_name, header_value)) = maybe_header.as_ref() {
-            req.headers_mut().append(header_name, header_value.clone());
-          }
-        } else {
-          maybe_header = None;
+        let mut headers = headers.clone();
+        // SECURITY: Do NOT forward auth headers to a new origin
+        if new_url.origin() != url.origin() {
+          headers.remove(http::header::AUTHORIZATION);
         }
+        *req.headers_mut() = headers;
 
         let new_response = self
           .client

cli/npm/mod.rs

@@ -8,6 +8,7 @@ use std::sync::Arc;
 
 use dashmap::DashMap;
 use deno_config::workspace::Workspace;
+use deno_core::error::AnyError;
 use deno_core::futures::stream::FuturesOrdered;
 use deno_core::futures::TryStreamExt;
 use deno_core::serde_json;
@@ -19,6 +20,8 @@ use deno_npm::registry::NpmPackageInfo;
 use deno_npm::registry::NpmPackageVersionInfo;
 use deno_npm::registry::NpmRegistryApi;
 use deno_npm::resolution::DefaultTarballUrlProvider;
+use deno_npm_cache::NpmCacheHttpClientBytesResponse;
+use deno_npm_cache::NpmCacheHttpClientResponse;
 use deno_resolver::npm::ByonmNpmResolverCreateOptions;
 use deno_runtime::colors;
 use deno_semver::package::PackageName;
@@ -27,8 +30,6 @@ use deno_semver::package::PackageReq;
 use deno_semver::SmallStackString;
 use deno_semver::StackString;
 use deno_semver::Version;
-use http::HeaderName;
-use http::HeaderValue;
 use indexmap::IndexMap;
 use thiserror::Error;
 
@@ -69,6 +70,7 @@ impl NpmPackageInfoApiAdapter {
     }
   }
 }
+
 async fn get_infos(
   info_provider: &(dyn NpmRegistryApi + Send + Sync),
   workspace_patch_packages: &WorkspaceNpmPatchPackages,
@@ -293,18 +295,48 @@ impl deno_npm_cache::NpmCacheHttpClient for CliNpmCacheHttpClient {
   async fn download_with_retries_on_any_tokio_runtime(
     &self,
     url: Url,
-    maybe_auth_header: Option<(HeaderName, HeaderValue)>,
-  ) -> Result<Option<Vec<u8>>, deno_npm_cache::DownloadError> {
+    maybe_auth: Option<String>,
+    maybe_etag: Option<String>,
+  ) -> Result<NpmCacheHttpClientResponse, deno_npm_cache::DownloadError> {
     let guard = self.progress_bar.update(url.as_str());
     let client = self.http_client_provider.get_or_create().map_err(|err| {
       deno_npm_cache::DownloadError {
         status_code: None,
         error: err,
       }
     })?;
+    let mut headers = http::HeaderMap::new();
+    if let Some(auth) = maybe_auth {
+      headers.append(
+        http::header::AUTHORIZATION,
+        http::header::HeaderValue::try_from(auth).unwrap(),
+      );
+    }
+    if let Some(etag) = maybe_etag {
+      headers.append(
+        http::header::IF_NONE_MATCH,
+        http::header::HeaderValue::try_from(etag).unwrap(),
+      );
+    }
     client
-      .download_with_progress_and_retries(url, maybe_auth_header, &guard)
+      .download_with_progress_and_retries(url, &headers, &guard)
       .await
+      .map(|response| match response {
+        crate::http_util::HttpClientResponse::Success { headers, body } => {
+          NpmCacheHttpClientResponse::Bytes(NpmCacheHttpClientBytesResponse {
+            etag: headers
+              .get(http::header::ETAG)
+              .and_then(|e| e.to_str().map(|t| t.to_string()).ok()),
+            bytes: body,
+          })
+        }
+        crate::http_util::HttpClientResponse::NotFound => {
+          NpmCacheHttpClientResponse::NotFound
+        }
+        crate::http_util::HttpClientResponse::NotModified => {
+          NpmCacheHttpClientResponse::NotModified
+        }
+      })
       .map_err(|err| {
         use crate::http_util::DownloadErrorKind::*;
         let status_code = match err.as_kind() {
@@ -315,10 +347,11 @@ impl deno_npm_cache::NpmCacheHttpClient for CliNpmCacheHttpClient {
           | ToStr { .. }
           | RedirectHeaderParse { .. }
           | TooManyRedirects
+          | UnhandledNotModified
           | NotFound
           | Other(_) => None,
           BadResponse(bad_response_error) => {
-            Some(bad_response_error.status_code)
+            Some(bad_response_error.status_code.as_u16())
           }
         };
         deno_npm_cache::DownloadError {
@@ -386,8 +419,18 @@ impl NpmFetchResolver {
       let registry_config = self.npmrc.get_registry_config(name);
       // TODO(bartlomieju): this should error out, not use `.ok()`.
       let maybe_auth_header =
-        deno_npm_cache::maybe_auth_header_for_npm_registry(registry_config)
-          .ok()?;
+        deno_npm_cache::maybe_auth_header_value_for_npm_registry(
+          registry_config,
+        )
+        .map_err(AnyError::from)
+        .and_then(|value| match value {
+          Some(value) => Ok(Some((
+            http::header::AUTHORIZATION,
+            http::HeaderValue::try_from(value.into_bytes())?,
+          ))),
+          None => Ok(None),
+        })
+        .ok()?;
       let file = self
         .file_fetcher
         .fetch_bypass_permissions_with_maybe_auth(&info_url, maybe_auth_header)

cli/standalone/binary.rs

@@ -321,7 +321,7 @@ impl<'a> DenoCompileBinaryWriter<'a> {
     binary_path_suffix: &str,
   ) -> Result<Vec<u8>, AnyError> {
     let download_url = format!("https://dl.deno.land/{binary_path_suffix}");
-    let maybe_bytes = {
+    let response = {
       let progress_bars = ProgressBar::new(ProgressBarStyle::DownloadBars);
       let progress = progress_bars.update(&download_url);
 
@@ -330,17 +330,14 @@ impl<'a> DenoCompileBinaryWriter<'a> {
         .get_or_create()?
         .download_with_progress_and_retries(
           download_url.parse()?,
-          None,
+          &Default::default(),
           &progress,
         )
         .await?
     };
-    let bytes = match maybe_bytes {
-      Some(bytes) => bytes,
-      None => {
-        bail!("Download could not be found, aborting");
-      }
-    };
+    let bytes = response
+      .into_bytes()
+      .with_context(|| format!("Failed downloading '{}'", download_url))?;
 
     let create_dir_all = |dir: &Path| {
       std::fs::create_dir_all(dir)

cli/tools/installer.rs

@@ -170,8 +170,9 @@ pub async fn infer_name_from_url(
 
   if url.path() == "/" {
     if let Ok(client) = http_client_provider.get_or_create() {
-      if let Ok(redirected_url) =
-        client.get_redirected_url(url.clone(), None).await
+      if let Ok(redirected_url) = client
+        .get_redirected_url(url.clone(), &Default::default())
+        .await
       {
         url = redirected_url;
       }