From 5f0bb3c6f4328003012e98ba70ce18e4e2e842de Mon Sep 17 00:00:00 2001 From: Marvin Hagemeister Date: Thu, 24 Oct 2024 20:03:56 +0200 Subject: fix: `.npmrc` settings not being passed to install/add command (#26473) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We weren't passing the resolved npmrc settings to the install commands. This lead us to always fall back to the default registry url instead of using the one from npmrc. Fixes https://github.com/denoland/deno/issues/26139 Fixes https://github.com/denoland/deno/issues/26033 Fixes https://github.com/denoland/deno/issues/25924 Fixes https://github.com/denoland/deno/issues/25822 Fixes https://github.com/denoland/deno/issues/26152 --------- Co-authored-by: Bartek Iwańczuk --- cli/args/mod.rs | 1 + cli/cache/mod.rs | 1 + cli/file_fetcher.rs | 30 +++++++++++++++- cli/http_util.rs | 21 +++++++++++ cli/lsp/npm.rs | 6 +++- cli/lsp/registries.rs | 1 + cli/npm/managed/cache/mod.rs | 2 +- cli/npm/managed/cache/registry_info.rs | 64 +++++++++++++++++----------------- cli/npm/managed/mod.rs | 2 +- cli/npm/mod.rs | 25 ++++++++++--- cli/tools/registry/pm.rs | 6 +++- 11 files changed, 118 insertions(+), 41 deletions(-) (limited to 'cli') diff --git a/cli/args/mod.rs b/cli/args/mod.rs index d8f553101..927f43e85 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -578,6 +578,7 @@ fn discover_npmrc( let resolved = npmrc .as_resolved(npm_registry_url()) .context("Failed to resolve .npmrc options")?; + log::debug!(".npmrc found at: '{}'", path.display()); Ok(Arc::new(resolved)) } diff --git a/cli/cache/mod.rs b/cli/cache/mod.rs index ded163b4e..bf8f1b1f0 100644 --- a/cli/cache/mod.rs +++ b/cli/cache/mod.rs @@ -378,6 +378,7 @@ impl Loader for FetchCacher { } else { FetchPermissionsOptionRef::DynamicContainer(&permissions) }, + maybe_auth: None, maybe_accept: None, maybe_cache_setting: maybe_cache_setting.as_ref(), }, diff --git a/cli/file_fetcher.rs b/cli/file_fetcher.rs index e92aca542..95d778f0b 100644 --- a/cli/file_fetcher.rs +++ b/cli/file_fetcher.rs @@ -24,6 +24,7 @@ use deno_graph::source::LoaderChecksum; use deno_path_util::url_to_file_path; use deno_runtime::deno_permissions::PermissionsContainer; use deno_runtime::deno_web::BlobStore; +use http::header; use log::debug; use std::borrow::Cow; use std::collections::HashMap; @@ -181,6 +182,7 @@ pub enum FetchPermissionsOptionRef<'a> { pub struct FetchOptions<'a> { pub specifier: &'a ModuleSpecifier, pub permissions: FetchPermissionsOptionRef<'a>, + pub maybe_auth: Option<(header::HeaderName, header::HeaderValue)>, pub maybe_accept: Option<&'a str>, pub maybe_cache_setting: Option<&'a CacheSetting>, } @@ -350,6 +352,7 @@ impl FileFetcher { maybe_accept: Option<&str>, cache_setting: &CacheSetting, maybe_checksum: Option<&LoaderChecksum>, + maybe_auth: Option<(header::HeaderName, header::HeaderValue)>, ) -> Result { debug!( "FileFetcher::fetch_remote_no_follow - specifier: {}", @@ -442,6 +445,7 @@ impl FileFetcher { .as_ref() .map(|(_, etag)| etag.clone()), maybe_auth_token: maybe_auth_token.clone(), + maybe_auth: maybe_auth.clone(), maybe_progress_guard: maybe_progress_guard.as_ref(), }) .await? @@ -538,7 +542,18 @@ impl FileFetcher { specifier: &ModuleSpecifier, ) -> Result { self - .fetch_inner(specifier, FetchPermissionsOptionRef::AllowAll) + .fetch_inner(specifier, None, FetchPermissionsOptionRef::AllowAll) + .await + } + + #[inline(always)] + pub async fn fetch_bypass_permissions_with_maybe_auth( + &self, + specifier: &ModuleSpecifier, + maybe_auth: Option<(header::HeaderName, header::HeaderValue)>, + ) -> Result { + self + .fetch_inner(specifier, maybe_auth, FetchPermissionsOptionRef::AllowAll) .await } @@ -552,6 +567,7 @@ impl FileFetcher { self .fetch_inner( specifier, + None, FetchPermissionsOptionRef::StaticContainer(permissions), ) .await @@ -560,12 +576,14 @@ impl FileFetcher { async fn fetch_inner( &self, specifier: &ModuleSpecifier, + maybe_auth: Option<(header::HeaderName, header::HeaderValue)>, permissions: FetchPermissionsOptionRef<'_>, ) -> Result { self .fetch_with_options(FetchOptions { specifier, permissions, + maybe_auth, maybe_accept: None, maybe_cache_setting: None, }) @@ -585,12 +603,14 @@ impl FileFetcher { max_redirect: usize, ) -> Result { let mut specifier = Cow::Borrowed(options.specifier); + let mut maybe_auth = options.maybe_auth.clone(); for _ in 0..=max_redirect { match self .fetch_no_follow_with_options(FetchNoFollowOptions { fetch_options: FetchOptions { specifier: &specifier, permissions: options.permissions, + maybe_auth: maybe_auth.clone(), maybe_accept: options.maybe_accept, maybe_cache_setting: options.maybe_cache_setting, }, @@ -602,6 +622,10 @@ impl FileFetcher { return Ok(file); } FileOrRedirect::Redirect(redirect_specifier) => { + // If we were redirected to another origin, don't send the auth header anymore. + if redirect_specifier.origin() != specifier.origin() { + maybe_auth = None; + } specifier = Cow::Owned(redirect_specifier); } } @@ -666,6 +690,7 @@ impl FileFetcher { options.maybe_accept, options.maybe_cache_setting.unwrap_or(&self.cache_setting), maybe_checksum, + options.maybe_auth, ) .await } @@ -756,6 +781,7 @@ mod tests { FetchOptions { specifier, permissions: FetchPermissionsOptionRef::AllowAll, + maybe_auth: None, maybe_accept: None, maybe_cache_setting: Some(&file_fetcher.cache_setting), }, @@ -1255,6 +1281,7 @@ mod tests { FetchOptions { specifier: &specifier, permissions: FetchPermissionsOptionRef::AllowAll, + maybe_auth: None, maybe_accept: None, maybe_cache_setting: Some(&file_fetcher.cache_setting), }, @@ -1268,6 +1295,7 @@ mod tests { FetchOptions { specifier: &specifier, permissions: FetchPermissionsOptionRef::AllowAll, + maybe_auth: None, maybe_accept: None, maybe_cache_setting: Some(&file_fetcher.cache_setting), }, diff --git a/cli/http_util.rs b/cli/http_util.rs index 9c9ae9e41..4b17936d6 100644 --- a/cli/http_util.rs +++ b/cli/http_util.rs @@ -19,6 +19,7 @@ use deno_runtime::deno_fetch; use deno_runtime::deno_fetch::create_http_client; use deno_runtime::deno_fetch::CreateHttpClientOptions; use deno_runtime::deno_tls::RootCertStoreProvider; +use http::header; use http::header::HeaderName; use http::header::HeaderValue; use http::header::ACCEPT; @@ -204,6 +205,7 @@ pub struct FetchOnceArgs<'a> { pub maybe_accept: Option, pub maybe_etag: Option, pub maybe_auth_token: Option, + pub maybe_auth: Option<(header::HeaderName, header::HeaderValue)>, pub maybe_progress_guard: Option<&'a UpdateGuard>, } @@ -382,6 +384,8 @@ impl HttpClient { request .headers_mut() .insert(AUTHORIZATION, authorization_val); + } else if let Some((header, value)) = args.maybe_auth { + request.headers_mut().insert(header, value); } if let Some(accept) = args.maybe_accept { let accepts_val = HeaderValue::from_str(&accept)?; @@ -792,6 +796,7 @@ mod test { maybe_etag: None, maybe_auth_token: None, maybe_progress_guard: None, + maybe_auth: None, }) .await; if let Ok(FetchOnceResult::Code(body, headers)) = result { @@ -818,6 +823,7 @@ mod test { maybe_etag: None, maybe_auth_token: None, maybe_progress_guard: None, + maybe_auth: None, }) .await; if let Ok(FetchOnceResult::Code(body, headers)) = result { @@ -845,6 +851,7 @@ mod test { maybe_etag: None, maybe_auth_token: None, maybe_progress_guard: None, + maybe_auth: None, }) .await; if let Ok(FetchOnceResult::Code(body, headers)) = result { @@ -866,6 +873,7 @@ mod test { maybe_etag: Some("33a64df551425fcc55e".to_string()), maybe_auth_token: None, maybe_progress_guard: None, + maybe_auth: None, }) .await; assert_eq!(res.unwrap(), FetchOnceResult::NotModified); @@ -885,6 +893,7 @@ mod test { maybe_etag: None, maybe_auth_token: None, maybe_progress_guard: None, + maybe_auth: None, }) .await; if let Ok(FetchOnceResult::Code(body, headers)) = result { @@ -914,6 +923,7 @@ mod test { maybe_etag: None, maybe_auth_token: None, maybe_progress_guard: None, + maybe_auth: None, }) .await; if let Ok(FetchOnceResult::Code(body, _)) = result { @@ -939,6 +949,7 @@ mod test { maybe_etag: None, maybe_auth_token: None, maybe_progress_guard: None, + maybe_auth: None, }) .await; if let Ok(FetchOnceResult::Redirect(url, _)) = result { @@ -974,6 +985,7 @@ mod test { maybe_etag: None, maybe_auth_token: None, maybe_progress_guard: None, + maybe_auth: None, }) .await; if let Ok(FetchOnceResult::Code(body, headers)) = result { @@ -1021,6 +1033,7 @@ mod test { maybe_etag: None, maybe_auth_token: None, maybe_progress_guard: None, + maybe_auth: None, }) .await; @@ -1083,6 +1096,7 @@ mod test { maybe_etag: None, maybe_auth_token: None, maybe_progress_guard: None, + maybe_auth: None, }) .await; @@ -1136,6 +1150,7 @@ mod test { maybe_etag: None, maybe_auth_token: None, maybe_progress_guard: None, + maybe_auth: None, }) .await; if let Ok(FetchOnceResult::Code(body, headers)) = result { @@ -1177,6 +1192,7 @@ mod test { maybe_etag: None, maybe_auth_token: None, maybe_progress_guard: None, + maybe_auth: None, }) .await; if let Ok(FetchOnceResult::Code(body, headers)) = result { @@ -1199,6 +1215,7 @@ mod test { maybe_etag: Some("33a64df551425fcc55e".to_string()), maybe_auth_token: None, maybe_progress_guard: None, + maybe_auth: None, }) .await; assert_eq!(res.unwrap(), FetchOnceResult::NotModified); @@ -1233,6 +1250,7 @@ mod test { maybe_etag: None, maybe_auth_token: None, maybe_progress_guard: None, + maybe_auth: None, }) .await; if let Ok(FetchOnceResult::Code(body, headers)) = result { @@ -1262,6 +1280,7 @@ mod test { maybe_etag: None, maybe_auth_token: None, maybe_progress_guard: None, + maybe_auth: None, }) .await; assert!(result.is_err()); @@ -1283,6 +1302,7 @@ mod test { maybe_etag: None, maybe_auth_token: None, maybe_progress_guard: None, + maybe_auth: None, }) .await; @@ -1306,6 +1326,7 @@ mod test { maybe_etag: None, maybe_auth_token: None, maybe_progress_guard: None, + maybe_auth: None, }) .await; diff --git a/cli/lsp/npm.rs b/cli/lsp/npm.rs index 8bdeb7e7d..2decfc342 100644 --- a/cli/lsp/npm.rs +++ b/cli/lsp/npm.rs @@ -4,6 +4,7 @@ use dashmap::DashMap; use deno_core::anyhow::anyhow; use deno_core::error::AnyError; use deno_core::serde_json; +use deno_npm::npm_rc::NpmRc; use deno_semver::package::PackageNv; use deno_semver::Version; use serde::Deserialize; @@ -25,7 +26,10 @@ pub struct CliNpmSearchApi { impl CliNpmSearchApi { pub fn new(file_fetcher: Arc) -> Self { - let resolver = NpmFetchResolver::new(file_fetcher.clone()); + let resolver = NpmFetchResolver::new( + file_fetcher.clone(), + Arc::new(NpmRc::default().as_resolved(npm_registry_url()).unwrap()), + ); Self { file_fetcher, resolver, diff --git a/cli/lsp/registries.rs b/cli/lsp/registries.rs index 5f7ce0082..ade353e68 100644 --- a/cli/lsp/registries.rs +++ b/cli/lsp/registries.rs @@ -482,6 +482,7 @@ impl ModuleRegistry { .fetch_with_options(FetchOptions { specifier: &specifier, permissions: FetchPermissionsOptionRef::AllowAll, + maybe_auth: None, maybe_accept: Some("application/vnd.deno.reg.v2+json, application/vnd.deno.reg.v1+json;q=0.9, application/json;q=0.8"), maybe_cache_setting: None, }) diff --git a/cli/npm/managed/cache/mod.rs b/cli/npm/managed/cache/mod.rs index fa0e8c8a5..aaec48668 100644 --- a/cli/npm/managed/cache/mod.rs +++ b/cli/npm/managed/cache/mod.rs @@ -26,7 +26,7 @@ use crate::cache::CACHE_PERM; use crate::util::fs::atomic_write_file_with_retries; use crate::util::fs::hard_link_dir_recursive; -mod registry_info; +pub mod registry_info; mod tarball; mod tarball_extract; diff --git a/cli/npm/managed/cache/registry_info.rs b/cli/npm/managed/cache/registry_info.rs index 6c4a7503b..6d39d3c13 100644 --- a/cli/npm/managed/cache/registry_info.rs +++ b/cli/npm/managed/cache/registry_info.rs @@ -84,7 +84,7 @@ impl RegistryInfoDownloader { self.load_package_info_inner(name).await.with_context(|| { format!( "Error getting response at {} for package \"{}\"", - self.get_package_url(name), + get_package_url(&self.npmrc, name), name ) }) @@ -190,7 +190,7 @@ impl RegistryInfoDownloader { fn create_load_future(self: &Arc, name: &str) -> LoadFuture { let downloader = self.clone(); - let package_url = self.get_package_url(name); + 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) { @@ -239,36 +239,36 @@ impl RegistryInfoDownloader { .map(|r| r.map_err(Arc::new)) .boxed_local() } +} - fn get_package_url(&self, name: &str) -> Url { - let registry_url = self.npmrc.get_registry_url(name); - // The '/' character in scoped package names "@scope/name" must be - // encoded for older third party registries. Newer registries and - // npm itself support both ways - // - encoded: https://registry.npmjs.org/@rollup%2fplugin-json - // - non-ecoded: https://registry.npmjs.org/@rollup/plugin-json - // To support as many third party registries as possible we'll - // always encode the '/' character. +pub fn get_package_url(npmrc: &ResolvedNpmRc, name: &str) -> Url { + let registry_url = npmrc.get_registry_url(name); + // The '/' character in scoped package names "@scope/name" must be + // encoded for older third party registries. Newer registries and + // npm itself support both ways + // - encoded: https://registry.npmjs.org/@rollup%2fplugin-json + // - non-ecoded: https://registry.npmjs.org/@rollup/plugin-json + // To support as many third party registries as possible we'll + // always encode the '/' character. - // list of all characters used in npm packages: - // !, ', (, ), *, -, ., /, [0-9], @, [A-Za-z], _, ~ - const ASCII_SET: percent_encoding::AsciiSet = - percent_encoding::NON_ALPHANUMERIC - .remove(b'!') - .remove(b'\'') - .remove(b'(') - .remove(b')') - .remove(b'*') - .remove(b'-') - .remove(b'.') - .remove(b'@') - .remove(b'_') - .remove(b'~'); - let name = percent_encoding::utf8_percent_encode(name, &ASCII_SET); - registry_url - // Ensure that scoped package name percent encoding is lower cased - // to match npm. - .join(&name.to_string().replace("%2F", "%2f")) - .unwrap() - } + // list of all characters used in npm packages: + // !, ', (, ), *, -, ., /, [0-9], @, [A-Za-z], _, ~ + const ASCII_SET: percent_encoding::AsciiSet = + percent_encoding::NON_ALPHANUMERIC + .remove(b'!') + .remove(b'\'') + .remove(b'(') + .remove(b')') + .remove(b'*') + .remove(b'-') + .remove(b'.') + .remove(b'@') + .remove(b'_') + .remove(b'~'); + let name = percent_encoding::utf8_percent_encode(name, &ASCII_SET); + registry_url + // Ensure that scoped package name percent encoding is lower cased + // to match npm. + .join(&name.to_string().replace("%2F", "%2f")) + .unwrap() } diff --git a/cli/npm/managed/mod.rs b/cli/npm/managed/mod.rs index ec50a9c65..d0880557f 100644 --- a/cli/npm/managed/mod.rs +++ b/cli/npm/managed/mod.rs @@ -55,7 +55,7 @@ use super::CliNpmResolver; use super::InnerCliNpmResolverRef; use super::ResolvePkgFolderFromDenoReqError; -mod cache; +pub mod cache; mod registry; mod resolution; mod resolvers; diff --git a/cli/npm/mod.rs b/cli/npm/mod.rs index 53baaf77b..f48f7a740 100644 --- a/cli/npm/mod.rs +++ b/cli/npm/mod.rs @@ -8,10 +8,12 @@ use std::path::Path; use std::path::PathBuf; use std::sync::Arc; +use common::maybe_auth_header_for_npm_registry; use dashmap::DashMap; use deno_ast::ModuleSpecifier; use deno_core::error::AnyError; use deno_core::serde_json; +use deno_npm::npm_rc::ResolvedNpmRc; use deno_npm::registry::NpmPackageInfo; use deno_resolver::npm::ByonmNpmResolver; use deno_resolver::npm::ByonmResolvePkgFolderFromDenoReqError; @@ -19,10 +21,10 @@ use deno_runtime::deno_node::NodeRequireResolver; use deno_runtime::ops::process::NpmProcessStateProvider; use deno_semver::package::PackageNv; use deno_semver::package::PackageReq; +use managed::cache::registry_info::get_package_url; use node_resolver::NpmResolver; use thiserror::Error; -use crate::args::npm_registry_url; use crate::file_fetcher::FileFetcher; pub use self::byonm::CliByonmNpmResolver; @@ -115,14 +117,19 @@ pub struct NpmFetchResolver { nv_by_req: DashMap>, info_by_name: DashMap>>, file_fetcher: Arc, + npmrc: Arc, } impl NpmFetchResolver { - pub fn new(file_fetcher: Arc) -> Self { + pub fn new( + file_fetcher: Arc, + npmrc: Arc, + ) -> Self { Self { nv_by_req: Default::default(), info_by_name: Default::default(), file_fetcher, + npmrc, } } @@ -157,11 +164,21 @@ impl NpmFetchResolver { return info.value().clone(); } let fetch_package_info = || async { - let info_url = npm_registry_url().join(name).ok()?; + let info_url = get_package_url(&self.npmrc, name); let file_fetcher = self.file_fetcher.clone(); + let registry_config = self.npmrc.get_registry_config(name); + // TODO(bartlomieju): this should error out, not use `.ok()`. + let maybe_auth_header = + maybe_auth_header_for_npm_registry(registry_config).ok()?; // spawn due to the lsp's `Send` requirement let file = deno_core::unsync::spawn(async move { - file_fetcher.fetch_bypass_permissions(&info_url).await.ok() + file_fetcher + .fetch_bypass_permissions_with_maybe_auth( + &info_url, + maybe_auth_header, + ) + .await + .ok() }) .await .ok()??; diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index 2060b9a13..d1be901d6 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -367,10 +367,14 @@ pub async fn add( Default::default(), None, ); + + let npmrc = cli_factory.cli_options().unwrap().npmrc(); + deps_file_fetcher.set_download_log_level(log::Level::Trace); let deps_file_fetcher = Arc::new(deps_file_fetcher); let jsr_resolver = Arc::new(JsrFetchResolver::new(deps_file_fetcher.clone())); - let npm_resolver = Arc::new(NpmFetchResolver::new(deps_file_fetcher)); + let npm_resolver = + Arc::new(NpmFetchResolver::new(deps_file_fetcher, npmrc.clone())); let mut selected_packages = Vec::with_capacity(add_flags.packages.len()); let mut package_reqs = Vec::with_capacity(add_flags.packages.len()); -- cgit v1.2.3