From 67e1685189aef5b6f135cb5e0edd94431c93f665 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 1 May 2025 18:36:07 -0400 Subject: [PATCH 1/4] fix: respect etag for npm packument caching --- Cargo.lock | 25 ++------ Cargo.toml | 8 ++- cli/http_util.rs | 87 ++++++++++++++++++---------- cli/npm/mod.rs | 60 ++++++++++++++++--- cli/standalone/binary.rs | 13 ++--- cli/tools/installer.rs | 5 +- cli/tools/upgrade.rs | 6 +- resolvers/deno/clippy.toml | 86 +++++++++++++-------------- resolvers/node/clippy.toml | 86 +++++++++++++-------------- resolvers/npm_cache/Cargo.toml | 2 +- resolvers/npm_cache/lib.rs | 38 ++++++++---- resolvers/npm_cache/registry_info.rs | 72 ++++++++++++++++------- resolvers/npm_cache/remote.rs | 23 ++------ resolvers/npm_cache/tarball.rs | 20 +++++-- resolvers/npm_cache/todo.md | 4 +- 15 files changed, 316 insertions(+), 219 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 055b7c539a588d..b52b7581efdd1c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -734,8 +734,6 @@ checksum = "1bf2a5fb3207c12b5d208ebc145f967fea5cac41a021c37417ccc31ba40f39ee" [[package]] name = "capacity_builder" version = "0.5.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8f2d24a6dcf0cd402a21b65d35340f3a49ff3475dc5fdac91d22d2733e6641c6" dependencies = [ "capacity_builder_macros", "ecow", @@ -746,8 +744,6 @@ dependencies = [ [[package]] name = "capacity_builder_macros" version = "0.3.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3b4a6cae9efc04cc6cbb8faf338d2c497c165c83e74509cf4dbedea948bbf6e5" dependencies = [ "quote", "syn 2.0.87", @@ -2390,12 +2386,12 @@ dependencies = [ "faster-hex", "flate2", "futures", - "http 1.1.0", "log", "parking_lot", "percent-encoding", "rand", "ring", + "serde", "serde_json", "sys_traits", "tar", @@ -2631,8 +2627,6 @@ dependencies = [ [[package]] name = "deno_semver" version = "0.7.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4775271f9b5602482698f76d24ea9ed8ba27af7f587a7e9a876916300c542435" dependencies = [ "capacity_builder", "deno_error", @@ -4396,13 +4390,12 @@ dependencies = [ [[package]] name = "hipstr" -version = "0.6.0" +version = "0.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "97971ffc85d4c98de12e2608e992a43f5294ebb625fdb045b27c731b64c4c6d6" +checksum = "07a5072958d04f9147e517881d929d3f4706612712f8f4cfcd247f2b716d5262" dependencies = [ + "loom", "serde", - "serde_bytes", - "sptr", ] [[package]] @@ -7403,9 +7396,9 @@ dependencies = [ [[package]] name = "serde_json" -version = "1.0.122" +version = "1.0.140" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "784b6203951c57ff748476b126ccb5e8e2959a5c19e5c617ab1956be3dbc68da" +checksum = "20068b6e96dc6c9bd23e01df8827e6c7e1f2fddd43c21810382803c136b99373" dependencies = [ "indexmap 2.8.0", "itoa", @@ -7719,12 +7712,6 @@ dependencies = [ "der", ] -[[package]] -name = "sptr" -version = "0.3.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3b9b39299b249ad65f3b7e96443bad61c02ca5cd3589f46cb6d610a0fd6c0d6a" - [[package]] name = "sqlformat" version = "0.3.5" diff --git a/Cargo.toml b/Cargo.toml index f2021c53e0b5fc..aef482215eb599 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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,7 @@ opt-level = 3 opt-level = 3 [profile.release.package.deno_npm_cache] opt-level = 3 + +[patch.crates-io] +deno_semver = { path = "../deno_semver" } +capacity_builder = { path = "../capacity_builder" } diff --git a/cli/http_util.rs b/cli/http_util.rs index a12fde937ec711..e108caeafc38f0 100644 --- a/cli/http_util.rs +++ b/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, + body: Vec, + }, + NotFound, + NotModified, +} + +impl HttpClientResponse { + pub fn into_bytes(self) -> Result, 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>, 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, 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>, DownloadError> { + ) -> Result { 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, ) -> Result { - 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, progress_guard: Option<&UpdateGuard>, - ) -> Result>, DownloadError> { - let (response, _) = self.get_redirected_response(url, maybe_header).await?; + ) -> Result { + 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,19 +320,17 @@ 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, ) -> Result<(http::Response, 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() @@ -317,13 +343,12 @@ impl HttpClient { 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 diff --git a/cli/npm/mod.rs b/cli/npm/mod.rs index 273309c151f3f6..d364e769e42f93 100644 --- a/cli/npm/mod.rs +++ b/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,8 +295,9 @@ 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>, deno_npm_cache::DownloadError> { + maybe_auth: Option, + maybe_etag: Option, + ) -> Result { 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 { @@ -302,9 +305,39 @@ impl deno_npm_cache::NpmCacheHttpClient for CliNpmCacheHttpClient { error: err, } })?; + let mut headers = http::HeaderMap::new(); + if let Some(auth) = maybe_auth { + headers.append( + http::header::AUTHORIZATION, + // todo(THIS PR): no unwrap + 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 +348,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 +420,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) diff --git a/cli/standalone/binary.rs b/cli/standalone/binary.rs index e3bd416c8506b3..e1314bc25bed59 100644 --- a/cli/standalone/binary.rs +++ b/cli/standalone/binary.rs @@ -321,7 +321,7 @@ impl<'a> DenoCompileBinaryWriter<'a> { binary_path_suffix: &str, ) -> Result, 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) diff --git a/cli/tools/installer.rs b/cli/tools/installer.rs index 337ad6a7a2fc7b..99d82859f93bcf 100644 --- a/cli/tools/installer.rs +++ b/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; } diff --git a/cli/tools/upgrade.rs b/cli/tools/upgrade.rs index 7149c152d9bd29..266df663f6bb63 100644 --- a/cli/tools/upgrade.rs +++ b/cli/tools/upgrade.rs @@ -918,11 +918,11 @@ async fn download_package( // provide an empty string here in order to prefer the downloading // text above which will stay alive after the progress bars are complete let progress = progress_bar.update(""); - let maybe_bytes = client - .download_with_progress_and_retries(download_url.clone(), None, &progress) + let response = client + .download_with_progress_and_retries(download_url.clone(), &Default::default(), &progress) .await .with_context(|| format!("Failed downloading {download_url}. The version you requested may not have been built for the current architecture."))?; - Ok(maybe_bytes) + Ok(response.into_maybe_bytes()?) } fn replace_exe(from: &Path, to: &Path) -> Result<(), std::io::Error> { diff --git a/resolvers/deno/clippy.toml b/resolvers/deno/clippy.toml index 886ba3fd1ac034..00d80c85e43211 100644 --- a/resolvers/deno/clippy.toml +++ b/resolvers/deno/clippy.toml @@ -1,47 +1,47 @@ disallowed-methods = [ - { path = "std::env::current_dir", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::path::Path::canonicalize", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::path::Path::is_dir", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::path::Path::is_file", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::path::Path::is_symlink", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::path::Path::metadata", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::path::Path::read_dir", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::path::Path::read_link", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::path::Path::symlink_metadata", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::path::Path::try_exists", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::path::PathBuf::exists", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::path::PathBuf::canonicalize", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::path::PathBuf::is_dir", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::path::PathBuf::is_file", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::path::PathBuf::is_symlink", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::path::PathBuf::metadata", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::path::PathBuf::read_dir", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::path::PathBuf::read_link", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::path::PathBuf::symlink_metadata", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::path::PathBuf::try_exists", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::env::set_current_dir", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::env::temp_dir", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::fs::canonicalize", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::fs::copy", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::fs::create_dir_all", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::fs::create_dir", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::fs::DirBuilder::new", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::fs::hard_link", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::fs::metadata", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::fs::OpenOptions::new", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::fs::read_dir", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::fs::read_link", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::fs::read_to_string", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::fs::read", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::fs::remove_dir_all", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::fs::remove_dir", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::fs::remove_file", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::fs::rename", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::fs::set_permissions", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::fs::symlink_metadata", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::fs::write", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::path::Path::canonicalize", reason = "File system operations should be done using DenoResolverFs trait" }, - { path = "std::path::Path::exists", reason = "File system operations should be done using DenoResolverFs trait" }, + { path = "std::env::current_dir", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::Path::canonicalize", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::Path::is_dir", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::Path::is_file", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::Path::is_symlink", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::Path::metadata", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::Path::read_dir", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::Path::read_link", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::Path::symlink_metadata", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::Path::try_exists", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::PathBuf::exists", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::PathBuf::canonicalize", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::PathBuf::is_dir", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::PathBuf::is_file", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::PathBuf::is_symlink", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::PathBuf::metadata", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::PathBuf::read_dir", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::PathBuf::read_link", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::PathBuf::symlink_metadata", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::PathBuf::try_exists", reason = "File system operations should be done using sys_traits" }, + { path = "std::env::set_current_dir", reason = "File system operations should be done using sys_traits" }, + { path = "std::env::temp_dir", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::canonicalize", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::copy", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::create_dir_all", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::create_dir", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::DirBuilder::new", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::hard_link", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::metadata", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::OpenOptions::new", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::read_dir", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::read_link", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::read_to_string", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::read", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::remove_dir_all", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::remove_dir", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::remove_file", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::rename", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::set_permissions", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::symlink_metadata", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::write", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::Path::canonicalize", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::Path::exists", reason = "File system operations should be done using sys_traits" }, { path = "url::Url::to_file_path", reason = "Use deno_path_util instead so it works in Wasm" }, { path = "url::Url::from_file_path", reason = "Use deno_path_util instead so it works in Wasm" }, { path = "url::Url::from_directory_path", reason = "Use deno_path_util instead so it works in Wasm" }, diff --git a/resolvers/node/clippy.toml b/resolvers/node/clippy.toml index 90eaba3fae2a8f..00d80c85e43211 100644 --- a/resolvers/node/clippy.toml +++ b/resolvers/node/clippy.toml @@ -1,47 +1,47 @@ disallowed-methods = [ - { path = "std::env::current_dir", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::path::Path::canonicalize", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::path::Path::is_dir", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::path::Path::is_file", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::path::Path::is_symlink", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::path::Path::metadata", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::path::Path::read_dir", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::path::Path::read_link", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::path::Path::symlink_metadata", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::path::Path::try_exists", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::path::PathBuf::exists", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::path::PathBuf::canonicalize", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::path::PathBuf::is_dir", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::path::PathBuf::is_file", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::path::PathBuf::is_symlink", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::path::PathBuf::metadata", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::path::PathBuf::read_dir", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::path::PathBuf::read_link", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::path::PathBuf::symlink_metadata", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::path::PathBuf::try_exists", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::env::set_current_dir", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::env::temp_dir", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::fs::canonicalize", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::fs::copy", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::fs::create_dir_all", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::fs::create_dir", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::fs::DirBuilder::new", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::fs::hard_link", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::fs::metadata", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::fs::OpenOptions::new", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::fs::read_dir", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::fs::read_link", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::fs::read_to_string", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::fs::read", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::fs::remove_dir_all", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::fs::remove_dir", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::fs::remove_file", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::fs::rename", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::fs::set_permissions", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::fs::symlink_metadata", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::fs::write", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::path::Path::canonicalize", reason = "File system operations should be done using NodeResolverFs trait" }, - { path = "std::path::Path::exists", reason = "File system operations should be done using NodeResolverFs trait" }, + { path = "std::env::current_dir", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::Path::canonicalize", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::Path::is_dir", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::Path::is_file", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::Path::is_symlink", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::Path::metadata", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::Path::read_dir", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::Path::read_link", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::Path::symlink_metadata", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::Path::try_exists", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::PathBuf::exists", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::PathBuf::canonicalize", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::PathBuf::is_dir", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::PathBuf::is_file", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::PathBuf::is_symlink", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::PathBuf::metadata", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::PathBuf::read_dir", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::PathBuf::read_link", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::PathBuf::symlink_metadata", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::PathBuf::try_exists", reason = "File system operations should be done using sys_traits" }, + { path = "std::env::set_current_dir", reason = "File system operations should be done using sys_traits" }, + { path = "std::env::temp_dir", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::canonicalize", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::copy", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::create_dir_all", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::create_dir", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::DirBuilder::new", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::hard_link", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::metadata", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::OpenOptions::new", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::read_dir", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::read_link", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::read_to_string", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::read", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::remove_dir_all", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::remove_dir", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::remove_file", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::rename", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::set_permissions", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::symlink_metadata", reason = "File system operations should be done using sys_traits" }, + { path = "std::fs::write", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::Path::canonicalize", reason = "File system operations should be done using sys_traits" }, + { path = "std::path::Path::exists", reason = "File system operations should be done using sys_traits" }, { path = "url::Url::to_file_path", reason = "Use deno_path_util instead so it works in Wasm" }, { path = "url::Url::from_file_path", reason = "Use deno_path_util instead so it works in Wasm" }, { path = "url::Url::from_directory_path", reason = "Use deno_path_util instead so it works in Wasm" }, diff --git a/resolvers/npm_cache/Cargo.toml b/resolvers/npm_cache/Cargo.toml index 9daef05c865eff..7b6ab753d73cf3 100644 --- a/resolvers/npm_cache/Cargo.toml +++ b/resolvers/npm_cache/Cargo.toml @@ -26,12 +26,12 @@ deno_unsync = { workspace = true, features = ["tokio"] } faster-hex.workspace = true flate2 = { workspace = true, features = ["zlib-ng-compat"] } futures.workspace = true -http.workspace = true log.workspace = true parking_lot.workspace = true percent-encoding.workspace = true rand.workspace = true ring.workspace = true +serde.workspace = true serde_json.workspace = true sys_traits.workspace = true tar.workspace = true diff --git a/resolvers/npm_cache/lib.rs b/resolvers/npm_cache/lib.rs index 17eb01b2102a4f..9d5037e8351ea4 100644 --- a/resolvers/npm_cache/lib.rs +++ b/resolvers/npm_cache/lib.rs @@ -10,20 +10,17 @@ use deno_cache_dir::file_fetcher::CacheSetting; use deno_cache_dir::npm::NpmCacheDir; use deno_error::JsErrorBox; use deno_npm::npm_rc::ResolvedNpmRc; -use deno_npm::registry::NpmPackageInfo; use deno_npm::NpmPackageCacheFolderId; use deno_path_util::fs::atomic_write_file_with_retries; use deno_semver::package::PackageNv; use deno_semver::StackString; use deno_semver::Version; -use http::HeaderName; -use http::HeaderValue; -use http::StatusCode; use parking_lot::Mutex; use sys_traits::FsCreateDirAll; use sys_traits::FsHardLink; use sys_traits::FsMetadata; use sys_traits::FsOpen; +use sys_traits::FsRead; use sys_traits::FsReadDir; use sys_traits::FsRemoveFile; use sys_traits::FsRename; @@ -45,14 +42,15 @@ pub use fs_util::HardLinkFileError; // using RegistryInfoProvider. pub use registry_info::get_package_url; pub use registry_info::RegistryInfoProvider; -pub use remote::maybe_auth_header_for_npm_registry; +pub use registry_info::SerializedCachedPackageInfo; +pub use remote::maybe_auth_header_value_for_npm_registry; pub use tarball::EnsurePackageError; pub use tarball::TarballCache; #[derive(Debug, deno_error::JsError)] #[class(generic)] pub struct DownloadError { - pub status_code: Option, + pub status_code: Option, pub error: JsErrorBox, } @@ -68,13 +66,25 @@ impl std::fmt::Display for DownloadError { } } +pub enum NpmCacheHttpClientResponse { + NotFound, + NotModified, + Bytes(NpmCacheHttpClientBytesResponse), +} + +pub struct NpmCacheHttpClientBytesResponse { + pub bytes: Vec, + pub etag: Option, +} + #[async_trait::async_trait(?Send)] pub trait NpmCacheHttpClient: Send + Sync + 'static { async fn download_with_retries_on_any_tokio_runtime( &self, url: Url, - maybe_auth_header: Option<(HeaderName, HeaderValue)>, - ) -> Result>, DownloadError>; + maybe_auth: Option, + maybe_etag: Option, + ) -> Result; } /// Indicates how cached source files should be handled. @@ -134,6 +144,7 @@ pub struct NpmCache< + FsHardLink + FsMetadata + FsOpen + + FsRead + FsReadDir + FsRemoveFile + FsRename @@ -152,6 +163,7 @@ impl< + FsHardLink + FsMetadata + FsOpen + + FsRead + FsReadDir + FsRemoveFile + FsRename @@ -299,21 +311,23 @@ impl< pub fn load_package_info( &self, name: &str, - ) -> Result, serde_json::Error> { + ) -> Result, serde_json::Error> { let file_cache_path = self.get_registry_package_info_file_cache_path(name); - let file_text = match std::fs::read_to_string(file_cache_path) { + let file_bytes = match self.sys.fs_read(&file_cache_path) { Ok(file_text) => file_text, Err(err) if err.kind() == ErrorKind::NotFound => return Ok(None), Err(err) => return Err(serde_json::Error::io(err)), }; - serde_json::from_str(&file_text) + + // todo(dsherret): deserialize in a blocking task + serde_json::from_slice(&file_bytes) } pub fn save_package_info( &self, name: &str, - package_info: &NpmPackageInfo, + package_info: &SerializedCachedPackageInfo, ) -> Result<(), JsErrorBox> { let file_cache_path = self.get_registry_package_info_file_cache_path(name); let file_text = diff --git a/resolvers/npm_cache/registry_info.rs b/resolvers/npm_cache/registry_info.rs index b5f7df5968b857..770196349b2e17 100644 --- a/resolvers/npm_cache/registry_info.rs +++ b/resolvers/npm_cache/registry_info.rs @@ -15,10 +15,13 @@ use deno_unsync::sync::MultiRuntimeAsyncValueCreator; use futures::future::LocalBoxFuture; use futures::FutureExt; use parking_lot::Mutex; +use serde::Deserialize; +use serde::Serialize; use sys_traits::FsCreateDirAll; use sys_traits::FsHardLink; use sys_traits::FsMetadata; use sys_traits::FsOpen; +use sys_traits::FsRead; use sys_traits::FsReadDir; use sys_traits::FsRemoveFile; use sys_traits::FsRename; @@ -26,14 +29,28 @@ use sys_traits::SystemRandom; use sys_traits::ThreadSleep; use url::Url; -use crate::remote::maybe_auth_header_for_npm_registry; +use crate::remote::maybe_auth_header_value_for_npm_registry; use crate::NpmCache; use crate::NpmCacheHttpClient; +use crate::NpmCacheHttpClientResponse; use crate::NpmCacheSetting; type LoadResult = Result>; type LoadFuture = LocalBoxFuture<'static, LoadResult>; +#[derive(Debug, Serialize, Deserialize, Clone)] +pub struct SerializedCachedPackageInfo { + #[serde(flatten)] + pub info: NpmPackageInfo, + /// Custom property that includes the etag. + #[serde( + default, + skip_serializing_if = "Option::is_none", + rename = "_denoetag" + )] + pub etag: Option, +} + #[derive(Debug, Clone)] enum FutureResult { PackageNotExists, @@ -122,6 +139,7 @@ pub struct LoadPackageInfoError { #[class(inherit)] #[error("{0}")] pub struct LoadPackageInfoInnerError(pub Arc); + // todo(#27198): refactor to store this only in the http cache /// Downloads packuments from the npm registry. @@ -134,6 +152,7 @@ pub struct RegistryInfoProvider< + FsHardLink + FsMetadata + FsOpen + + FsRead + FsReadDir + FsRemoveFile + FsRename @@ -158,6 +177,7 @@ impl< + FsHardLink + FsMetadata + FsOpen + + FsRead + FsReadDir + FsRemoveFile + FsRename @@ -312,9 +332,9 @@ impl< let downloader = self.clone(); let package_url = get_package_url(&self.npmrc, name); let registry_config = self.npmrc.get_registry_config(name); - let maybe_auth_header = - match maybe_auth_header_for_npm_registry(registry_config) { - Ok(maybe_auth_header) => maybe_auth_header, + let maybe_auth_header_value = + match maybe_auth_header_value_for_npm_registry(registry_config) { + Ok(maybe_auth_header_value) => maybe_auth_header_value, Err(err) => { return std::future::ready(Err(Arc::new(JsErrorBox::from_err(err)))) .boxed_local() @@ -322,17 +342,19 @@ impl< }; let name = name.to_string(); async move { - if (downloader.cache.cache_setting().should_use_for_npm_package(&name) && !downloader.force_reload_flag.is_raised()) - // if this has been previously reloaded, then try loading from the - // file system cache + let maybe_file_cached = if (downloader.cache.cache_setting().should_use_for_npm_package(&name) && !downloader.force_reload_flag.is_raised()) + // if this has been previously reloaded, then try loading from the file system cache || downloader.previously_loaded_packages.lock().contains(&name) { // attempt to load from the file cache - if let Some(info) = downloader.cache.load_package_info(&name).map_err(JsErrorBox::from_err)? { - let result = Arc::new(info); - return Ok(FutureResult::SavedFsCache(result)); + if let Some(cached_info) = downloader.cache.load_package_info(&name).map_err(JsErrorBox::from_err)? { + return Ok(FutureResult::SavedFsCache(Arc::new(cached_info.info))); + } else { + None } - } + } else { + downloader.cache.load_package_info(&name).ok().flatten() + }; if *downloader.cache.cache_setting() == NpmCacheSetting::Only { return Err(JsErrorBox::new( @@ -345,21 +367,30 @@ impl< downloader.previously_loaded_packages.lock().insert(name.to_string()); - let maybe_bytes = downloader + let (maybe_etag, maybe_cached_info) = match maybe_file_cached { + Some(cached_info) => (cached_info.etag, Some(cached_info.info)), + None => (None, None) + }; + + let response = downloader .http_client .download_with_retries_on_any_tokio_runtime( package_url, - maybe_auth_header, + maybe_auth_header_value, + maybe_etag, ) .await.map_err(JsErrorBox::from_err)?; - match maybe_bytes { - Some(bytes) => { + match response { + NpmCacheHttpClientResponse::NotModified => Ok(FutureResult::SavedFsCache(Arc::new(maybe_cached_info.unwrap()))), + NpmCacheHttpClientResponse::NotFound => Ok(FutureResult::PackageNotExists), + NpmCacheHttpClientResponse::Bytes(response) => { let future_result = deno_unsync::spawn_blocking( move || -> Result { - let package_info = serde_json::from_slice(&bytes).map_err(JsErrorBox::from_err)?; + let mut package_info: SerializedCachedPackageInfo = serde_json::from_slice(&response.bytes).map_err(JsErrorBox::from_err)?; + package_info.etag = response.etag; match downloader.cache.save_package_info(&name, &package_info) { Ok(()) => { - Ok(FutureResult::SavedFsCache(Arc::new(package_info))) + Ok(FutureResult::SavedFsCache(Arc::new(package_info.info))) } Err(err) => { log::debug!( @@ -367,7 +398,7 @@ impl< name, err ); - Ok(FutureResult::ErroredFsCache(Arc::new(package_info))) + Ok(FutureResult::ErroredFsCache(Arc::new(package_info.info))) } } }, @@ -375,8 +406,7 @@ impl< .await .map_err(JsErrorBox::from_err)??; Ok(future_result) - } - None => Ok(FutureResult::PackageNotExists), + }, } } .map(|r| r.map_err(Arc::new)) @@ -390,6 +420,7 @@ pub struct NpmRegistryApiAdapter< + FsHardLink + FsMetadata + FsOpen + + FsRead + FsReadDir + FsRemoveFile + FsRename @@ -407,6 +438,7 @@ impl< + FsHardLink + FsMetadata + FsOpen + + FsRead + FsReadDir + FsRemoveFile + FsRename diff --git a/resolvers/npm_cache/remote.rs b/resolvers/npm_cache/remote.rs index 0e04d0550272b0..ed74e5637a80f8 100644 --- a/resolvers/npm_cache/remote.rs +++ b/resolvers/npm_cache/remote.rs @@ -3,7 +3,6 @@ use base64::prelude::BASE64_STANDARD; use base64::Engine; use deno_npm::npm_rc::RegistryConfig; -use http::header; #[derive(Debug, thiserror::Error, deno_error::JsError)] pub enum AuthHeaderForNpmRegistryError { @@ -16,24 +15,15 @@ pub enum AuthHeaderForNpmRegistryError { } // TODO(bartlomieju): support more auth methods besides token and basic auth -pub fn maybe_auth_header_for_npm_registry( +pub fn maybe_auth_header_value_for_npm_registry( registry_config: &RegistryConfig, -) -> Result< - Option<(header::HeaderName, header::HeaderValue)>, - AuthHeaderForNpmRegistryError, -> { +) -> Result, AuthHeaderForNpmRegistryError> { if let Some(token) = registry_config.auth_token.as_ref() { - return Ok(Some(( - header::AUTHORIZATION, - header::HeaderValue::from_str(&format!("Bearer {}", token)).unwrap(), - ))); + return Ok(Some(format!("Bearer {}", token))); } if let Some(auth) = registry_config.auth.as_ref() { - return Ok(Some(( - header::AUTHORIZATION, - header::HeaderValue::from_str(&format!("Basic {}", auth)).unwrap(), - ))); + return Ok(Some(format!("Basic {}", auth))); } let (username, password) = ( @@ -59,10 +49,7 @@ pub fn maybe_auth_header_for_npm_registry( String::from_utf8_lossy(&pw_base64) )); - return Ok(Some(( - header::AUTHORIZATION, - header::HeaderValue::from_str(&format!("Basic {}", bearer)).unwrap(), - ))); + return Ok(Some(format!("Basic {}", bearer))); } Ok(None) diff --git a/resolvers/npm_cache/tarball.rs b/resolvers/npm_cache/tarball.rs index 7a9453e6556e56..8c28ea03fc01b0 100644 --- a/resolvers/npm_cache/tarball.rs +++ b/resolvers/npm_cache/tarball.rs @@ -10,12 +10,12 @@ use deno_semver::package::PackageNv; use deno_unsync::sync::MultiRuntimeAsyncValueCreator; use futures::future::LocalBoxFuture; use futures::FutureExt; -use http::StatusCode; use parking_lot::Mutex; use sys_traits::FsCreateDirAll; use sys_traits::FsHardLink; use sys_traits::FsMetadata; use sys_traits::FsOpen; +use sys_traits::FsRead; use sys_traits::FsReadDir; use sys_traits::FsRemoveFile; use sys_traits::FsRename; @@ -23,11 +23,12 @@ use sys_traits::SystemRandom; use sys_traits::ThreadSleep; use url::Url; -use crate::remote::maybe_auth_header_for_npm_registry; +use crate::remote::maybe_auth_header_value_for_npm_registry; use crate::tarball_extract::verify_and_extract_tarball; use crate::tarball_extract::TarballExtractionMode; use crate::NpmCache; use crate::NpmCacheHttpClient; +use crate::NpmCacheHttpClientResponse; use crate::NpmCacheSetting; type LoadResult = Result<(), Arc>; @@ -55,6 +56,7 @@ pub struct TarballCache< + FsMetadata + FsOpen + FsRemoveFile + + FsRead + FsReadDir + FsRename + ThreadSleep @@ -78,6 +80,7 @@ pub struct EnsurePackageError { #[source] source: Arc, } + impl< THttpClient: NpmCacheHttpClient, TSys: FsCreateDirAll @@ -85,6 +88,7 @@ impl< + FsMetadata + FsOpen + FsRemoveFile + + FsRead + FsReadDir + FsRename + ThreadSleep @@ -202,15 +206,19 @@ impl< let tarball_uri = Url::parse(&dist.tarball).map_err(JsErrorBox::from_err)?; let maybe_registry_config = tarball_cache.npmrc.tarball_config(&tarball_uri); - let maybe_auth_header = maybe_registry_config.and_then(|c| maybe_auth_header_for_npm_registry(c).ok()?); + let maybe_auth_header = maybe_registry_config.and_then(|c| maybe_auth_header_value_for_npm_registry(c).ok()?); let result = tarball_cache.http_client - .download_with_retries_on_any_tokio_runtime(tarball_uri, maybe_auth_header) + .download_with_retries_on_any_tokio_runtime(tarball_uri, maybe_auth_header, None) .await; let maybe_bytes = match result { - Ok(maybe_bytes) => maybe_bytes, + Ok(response) => match response { + NpmCacheHttpClientResponse::NotModified => unreachable!(), // no e-tag + NpmCacheHttpClientResponse::NotFound => None, + NpmCacheHttpClientResponse::Bytes(r) => Some(r.bytes), + }, Err(err) => { - if err.status_code == Some(StatusCode::UNAUTHORIZED) + if err.status_code == Some(401) && maybe_registry_config.is_none() && tarball_cache.npmrc.get_registry_config(&package_nv.name).auth_token.is_some() { diff --git a/resolvers/npm_cache/todo.md b/resolvers/npm_cache/todo.md index e460fcae8698dc..e0340b8d873380 100644 --- a/resolvers/npm_cache/todo.md +++ b/resolvers/npm_cache/todo.md @@ -1,7 +1,5 @@ This crate is a work in progress: 1. Add a clippy.toml file that bans accessing the file system directory and - instead does it through a trait. + instead does it through sys_traits. 1. Make this crate work in Wasm. -1. Refactor to store npm packument in a single place: - https://github.com/denoland/deno/issues/27198 From 49db8ba80684476994b9be08a95ec8934acd43a4 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 1 May 2025 19:40:15 -0400 Subject: [PATCH 2/4] Add test --- Cargo.lock | 23 +++++++--- Cargo.toml | 5 +-- cli/http_util.rs | 2 +- cli/npm/mod.rs | 1 - resolvers/npm_cache/registry_info.rs | 7 ++- tests/specs/npm/packument_etag/__test__.jsonc | 10 +++++ .../npm/packument_etag/install_reload.out | 5 +++ tests/specs/npm/packument_etag/package.json | 5 +++ tests/util/server/src/servers/npm_registry.rs | 44 +++++++++++++++++-- 9 files changed, 86 insertions(+), 16 deletions(-) create mode 100644 tests/specs/npm/packument_etag/__test__.jsonc create mode 100644 tests/specs/npm/packument_etag/install_reload.out create mode 100644 tests/specs/npm/packument_etag/package.json diff --git a/Cargo.lock b/Cargo.lock index b52b7581efdd1c..2a7a5215af4c0c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -734,6 +734,8 @@ checksum = "1bf2a5fb3207c12b5d208ebc145f967fea5cac41a021c37417ccc31ba40f39ee" [[package]] name = "capacity_builder" version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8f2d24a6dcf0cd402a21b65d35340f3a49ff3475dc5fdac91d22d2733e6641c6" dependencies = [ "capacity_builder_macros", "ecow", @@ -744,6 +746,8 @@ dependencies = [ [[package]] name = "capacity_builder_macros" version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3b4a6cae9efc04cc6cbb8faf338d2c497c165c83e74509cf4dbedea948bbf6e5" dependencies = [ "quote", "syn 2.0.87", @@ -2351,9 +2355,7 @@ dependencies = [ [[package]] name = "deno_npm" -version = "0.33.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8bbe8ae992daf5506f430f2df2c5cf1ce95afe0a08d749251a5761488c71426d" +version = "0.33.1" dependencies = [ "async-trait", "capacity_builder", @@ -2627,6 +2629,8 @@ dependencies = [ [[package]] name = "deno_semver" version = "0.7.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4775271f9b5602482698f76d24ea9ed8ba27af7f587a7e9a876916300c542435" dependencies = [ "capacity_builder", "deno_error", @@ -4390,12 +4394,13 @@ dependencies = [ [[package]] name = "hipstr" -version = "0.8.0" +version = "0.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "07a5072958d04f9147e517881d929d3f4706612712f8f4cfcd247f2b716d5262" +checksum = "97971ffc85d4c98de12e2608e992a43f5294ebb625fdb045b27c731b64c4c6d6" dependencies = [ - "loom", "serde", + "serde_bytes", + "sptr", ] [[package]] @@ -7712,6 +7717,12 @@ dependencies = [ "der", ] +[[package]] +name = "sptr" +version = "0.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3b9b39299b249ad65f3b7e96443bad61c02ca5cd3589f46cb6d610a0fd6c0d6a" + [[package]] name = "sqlformat" version = "0.3.5" diff --git a/Cargo.toml b/Cargo.toml index aef482215eb599..033cd6c1bf2988 100644 --- a/Cargo.toml +++ b/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" @@ -518,5 +518,4 @@ opt-level = 3 opt-level = 3 [patch.crates-io] -deno_semver = { path = "../deno_semver" } -capacity_builder = { path = "../capacity_builder" } +deno_npm = { path = "../deno_npm" } diff --git a/cli/http_util.rs b/cli/http_util.rs index e108caeafc38f0..0a7d9e47c705e6 100644 --- a/cli/http_util.rs +++ b/cli/http_util.rs @@ -338,7 +338,7 @@ impl HttpClient { .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(); diff --git a/cli/npm/mod.rs b/cli/npm/mod.rs index d364e769e42f93..fc6cbee1c88ed4 100644 --- a/cli/npm/mod.rs +++ b/cli/npm/mod.rs @@ -309,7 +309,6 @@ impl deno_npm_cache::NpmCacheHttpClient for CliNpmCacheHttpClient { if let Some(auth) = maybe_auth { headers.append( http::header::AUTHORIZATION, - // todo(THIS PR): no unwrap http::header::HeaderValue::try_from(auth).unwrap(), ); } diff --git a/resolvers/npm_cache/registry_info.rs b/resolvers/npm_cache/registry_info.rs index 770196349b2e17..343bddb910317b 100644 --- a/resolvers/npm_cache/registry_info.rs +++ b/resolvers/npm_cache/registry_info.rs @@ -46,7 +46,7 @@ pub struct SerializedCachedPackageInfo { #[serde( default, skip_serializing_if = "Option::is_none", - rename = "_denoetag" + rename = "_deno.etag" )] pub etag: Option, } @@ -381,7 +381,10 @@ impl< ) .await.map_err(JsErrorBox::from_err)?; match response { - NpmCacheHttpClientResponse::NotModified => Ok(FutureResult::SavedFsCache(Arc::new(maybe_cached_info.unwrap()))), + NpmCacheHttpClientResponse::NotModified => { + log::debug!("Respected etag for packument '{0}'", name); // used in the tests + Ok(FutureResult::SavedFsCache(Arc::new(maybe_cached_info.unwrap()))) + }, NpmCacheHttpClientResponse::NotFound => Ok(FutureResult::PackageNotExists), NpmCacheHttpClientResponse::Bytes(response) => { let future_result = deno_unsync::spawn_blocking( diff --git a/tests/specs/npm/packument_etag/__test__.jsonc b/tests/specs/npm/packument_etag/__test__.jsonc new file mode 100644 index 00000000000000..4f258aa227d16d --- /dev/null +++ b/tests/specs/npm/packument_etag/__test__.jsonc @@ -0,0 +1,10 @@ +{ + "tempDir": true, + "steps": [{ + "args": "install --no-lock", + "output": "[WILDCARD]" + }, { + "args": "install --no-lock --reload --log-level=debug", + "output": "install_reload.out" + }] +} diff --git a/tests/specs/npm/packument_etag/install_reload.out b/tests/specs/npm/packument_etag/install_reload.out new file mode 100644 index 00000000000000..209a289da99cb2 --- /dev/null +++ b/tests/specs/npm/packument_etag/install_reload.out @@ -0,0 +1,5 @@ +[WILDCARD] +Download http://localhost:4260/@denotest%2fadd +[WILDCARD] Respected etag for packument '@denotest/add'[WILDCARD] +Download http://localhost:4260/@denotest/add/1.0.0.tgz +[WILDCARD] diff --git a/tests/specs/npm/packument_etag/package.json b/tests/specs/npm/packument_etag/package.json new file mode 100644 index 00000000000000..15a786ad71fa22 --- /dev/null +++ b/tests/specs/npm/packument_etag/package.json @@ -0,0 +1,5 @@ +{ + "dependencies": { + "@denotest/add": "*" + } +} diff --git a/tests/util/server/src/servers/npm_registry.rs b/tests/util/server/src/servers/npm_registry.rs index 1afca1329fefb0..884e52d15b3ffb 100644 --- a/tests/util/server/src/servers/npm_registry.rs +++ b/tests/util/server/src/servers/npm_registry.rs @@ -7,14 +7,19 @@ use std::net::SocketAddr; use std::net::SocketAddrV6; use std::path::PathBuf; +use base64::prelude::BASE64_STANDARD; +use base64::Engine; use bytes::Bytes; use futures::future::LocalBoxFuture; use futures::FutureExt; +use http::HeaderMap; +use http::HeaderValue; use http_body_util::combinators::UnsyncBoxBody; use hyper::body::Incoming; use hyper::Request; use hyper::Response; use hyper::StatusCode; +use sha2::Digest; use super::custom_headers; use super::empty_body; @@ -175,8 +180,13 @@ async fn handle_req_for_registry( } // otherwise try to serve from the registry - if let Some(resp) = - try_serve_npm_registry(uri_path, file_path.clone(), test_npm_registry).await + if let Some(resp) = try_serve_npm_registry( + uri_path, + file_path.clone(), + req.headers(), + test_npm_registry, + ) + .await { return resp; } @@ -190,6 +200,7 @@ async fn handle_req_for_registry( fn handle_custom_npm_registry_path( scope_name: &str, path: &str, + headers: &HeaderMap, test_npm_registry: &npm::TestNpmRegistry, ) -> Result>>, anyhow::Error> { let mut parts = path @@ -211,7 +222,32 @@ fn handle_custom_npm_registry_path( if let Some(registry_file) = test_npm_registry.registry_file(&package_name)? { - let file_resp = custom_headers("registry.json", registry_file); + let actual_etag = format!( + "\"{}\"", + BASE64_STANDARD.encode(sha2::Sha256::digest(®istry_file)) + ); + if headers.get("If-None-Match").and_then(|v| v.to_str().ok()) + == Some(actual_etag.as_str()) + { + let mut response = Response::new(UnsyncBoxBody::new( + http_body_util::Full::new(Bytes::from(vec![])), + )); + response + .headers_mut() + .insert("Content-Type", HeaderValue::from_static("text/plain")); + response + .headers_mut() + .insert("Content-Length", HeaderValue::from_static("0")); + *response.status_mut() = StatusCode::NOT_MODIFIED; + return Ok(Some(response)); + } + + let mut file_resp = custom_headers("registry.json", registry_file); + file_resp.headers_mut().append( + http::header::ETAG, + http::header::HeaderValue::from_str(&actual_etag).unwrap(), + ); + return Ok(Some(file_resp)); } } @@ -228,6 +264,7 @@ fn should_download_npm_packages() -> bool { async fn try_serve_npm_registry( uri_path: &str, mut testdata_file_path: PathBuf, + headers: &HeaderMap, test_npm_registry: &npm::TestNpmRegistry, ) -> Option>, anyhow::Error>> { if let Some((scope_name, package_name_with_path)) = test_npm_registry @@ -238,6 +275,7 @@ async fn try_serve_npm_registry( match handle_custom_npm_registry_path( scope_name, package_name_with_path, + headers, test_npm_registry, ) { Ok(Some(response)) => return Some(Ok(response)), From 76f7400c5b52d55edcf617c02e471d8acf164fbe Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 1 May 2025 19:41:03 -0400 Subject: [PATCH 3/4] Update to released version and fix test --- Cargo.lock | 4 +++- Cargo.toml | 5 +---- tests/specs/lockfile/out_of_date_npm_info/rm-version.ts | 1 + 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2a7a5215af4c0c..c4b7e47f817a55 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2355,7 +2355,9 @@ dependencies = [ [[package]] name = "deno_npm" -version = "0.33.1" +version = "0.33.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5217eb9aaebd02e92baeea7b474ca3f663b611204ccbfba11f8d296be08abe69" dependencies = [ "async-trait", "capacity_builder", diff --git a/Cargo.toml b/Cargo.toml index 033cd6c1bf2988..be7345930963a6 100644 --- a/Cargo.toml +++ b/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.1" +deno_npm = "=0.33.2" deno_package_json = { version = "=0.6.0", default-features = false } deno_path_util = "=0.3.2" deno_semver = "=0.7.1" @@ -516,6 +516,3 @@ opt-level = 3 opt-level = 3 [profile.release.package.deno_npm_cache] opt-level = 3 - -[patch.crates-io] -deno_npm = { path = "../deno_npm" } diff --git a/tests/specs/lockfile/out_of_date_npm_info/rm-version.ts b/tests/specs/lockfile/out_of_date_npm_info/rm-version.ts index 9b81f99df7bf0e..f18fe77ba394a3 100644 --- a/tests/specs/lockfile/out_of_date_npm_info/rm-version.ts +++ b/tests/specs/lockfile/out_of_date_npm_info/rm-version.ts @@ -9,6 +9,7 @@ if (registryJson["dist-tags"]["latest"] === version) { const latestVersion = Object.keys(registryJson.versions).sort()[0]; registryJson["dist-tags"]["latest"] = latestVersion; } +delete registryJson["_deno.etag"]; const registryJsonString = JSON.stringify(registryJson, null, 2); Deno.writeTextFileSync(registryPath, registryJsonString); console.log(registryJsonString); From d458dca8584159f1a7616ef01165d9d8019b27d9 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 1 May 2025 19:44:03 -0400 Subject: [PATCH 4/4] remove not needed --- tests/util/server/src/servers/npm_registry.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/tests/util/server/src/servers/npm_registry.rs b/tests/util/server/src/servers/npm_registry.rs index 884e52d15b3ffb..9cf6ce5db801c8 100644 --- a/tests/util/server/src/servers/npm_registry.rs +++ b/tests/util/server/src/servers/npm_registry.rs @@ -232,12 +232,6 @@ fn handle_custom_npm_registry_path( let mut response = Response::new(UnsyncBoxBody::new( http_body_util::Full::new(Bytes::from(vec![])), )); - response - .headers_mut() - .insert("Content-Type", HeaderValue::from_static("text/plain")); - response - .headers_mut() - .insert("Content-Length", HeaderValue::from_static("0")); *response.status_mut() = StatusCode::NOT_MODIFIED; return Ok(Some(response)); }