fix(npm): use configured auth for tarball urls instead of scope auth (#24111)

Deno was using the scope auth for the tarball urls, which is not always
correct. We are going to do a release immediately for this issue.

Cargo.lock

@@ -1734,9 +1734,9 @@ dependencies = [
 
 [[package]]
 name = "deno_npm"
-version = "0.21.1"
+version = "0.21.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "0971210ea837a8153c2e1bec7d65aabf372f436845e576db95c596c0f4b1209d"
+checksum = "5c7ae566a3cba78bf05751c20708f28385fe339b1d07bd8daff16316317d4228"
 dependencies = [
  "anyhow",
  "async-trait",

cli/Cargo.toml

@@ -72,7 +72,7 @@ deno_emit = "=0.42.0"
 deno_graph = { version = "=0.78.0", features = ["tokio_executor"] }
 deno_lint = { version = "=0.60.0", features = ["docs"] }
 deno_lockfile.workspace = true
-deno_npm = "=0.21.1"
+deno_npm = "=0.21.2"
 deno_runtime = { workspace = true, features = ["include_js_files_for_snapshotting"] }
 deno_semver = "=0.5.4"
 deno_task_shell = "=0.16.1"

cli/args/mod.rs

@@ -619,6 +619,7 @@ pub fn create_default_npmrc() -> Arc<ResolvedNpmRc> {
       config: Default::default(),
     },
     scopes: Default::default(),
+    registry_configs: Default::default(),
   })
 }
 

cli/http_util.rs

@@ -7,7 +7,6 @@ use crate::version::get_user_agent;
 use cache_control::Cachability;
 use cache_control::CacheControl;
 use chrono::DateTime;
-use deno_core::anyhow::bail;
 use deno_core::error::custom_error;
 use deno_core::error::generic_error;
 use deno_core::error::AnyError;
@@ -30,6 +29,7 @@ use std::sync::Arc;
 use std::thread::ThreadId;
 use std::time::Duration;
 use std::time::SystemTime;
+use thiserror::Error;
 
 // TODO(ry) HTTP headers are not unique key, value pairs. There may be more than
 // one header line with the same key. This should be changed to something like
@@ -260,6 +260,27 @@ impl HttpClientProvider {
   }
 }
 
+#[derive(Debug, Error)]
+#[error("Bad response: {:?}{}", .status_code, .response_text.as_ref().map(|s| format!("\n\n{}", s)).unwrap_or_else(String::new))]
+pub struct BadResponseError {
+  pub status_code: StatusCode,
+  pub response_text: Option<String>,
+}
+
+#[derive(Debug, Error)]
+pub enum DownloadError {
+  #[error(transparent)]
+  Reqwest(#[from] reqwest::Error),
+  #[error(transparent)]
+  ToStr(#[from] reqwest::header::ToStrError),
+  #[error("Redirection from '{}' did not provide location header", .request_url)]
+  NoRedirectHeader { request_url: Url },
+  #[error("Too many redirects.")]
+  TooManyRedirects,
+  #[error(transparent)]
+  BadResponse(#[from] BadResponseError),
+}
+
 #[derive(Debug)]
 pub struct HttpClient {
   #[allow(clippy::disallowed_types)] // reqwest::Client allowed here
@@ -409,7 +430,7 @@ impl HttpClient {
     url: impl reqwest::IntoUrl,
     maybe_header: Option<(HeaderName, HeaderValue)>,
     progress_guard: &UpdateGuard,
-  ) -> Result<Option<Vec<u8>>, AnyError> {
+  ) -> Result<Option<Vec<u8>>, DownloadError> {
     self
       .download_inner(url, maybe_header, Some(progress_guard))
       .await
@@ -429,7 +450,7 @@ impl HttpClient {
     url: impl reqwest::IntoUrl,
     maybe_header: Option<(HeaderName, HeaderValue)>,
     progress_guard: Option<&UpdateGuard>,
-  ) -> Result<Option<Vec<u8>>, AnyError> {
+  ) -> Result<Option<Vec<u8>>, DownloadError> {
     let response = self.get_redirected_response(url, maybe_header).await?;
 
     if response.status() == 404 {
@@ -437,26 +458,25 @@ impl HttpClient {
     } else if !response.status().is_success() {
       let status = response.status();
       let maybe_response_text = response.text().await.ok();
-      bail!(
+      return Err(DownloadError::BadResponse(BadResponseError {
-        "Bad response: {:?}{}",
+        status_code: status,
-        status,
+        response_text: maybe_response_text
-        match maybe_response_text {
+          .map(|s| s.trim().to_string())
-          Some(text) => format!("\n\n{text}"),
+          .filter(|s| !s.is_empty()),
-          None => String::new(),
+      }));
-        }
-      );
     }
 
     get_response_body_with_progress(response, progress_guard)
       .await
       .map(Some)
+      .map_err(Into::into)
   }
 
   async fn get_redirected_response(
     &self,
     url: impl reqwest::IntoUrl,
     mut maybe_header: Option<(HeaderName, HeaderValue)>,
-  ) -> Result<reqwest::Response, AnyError> {
+  ) -> Result<reqwest::Response, DownloadError> {
     let mut url = url.into_url()?;
     let mut builder = self.get(url.clone());
     if let Some((header_name, header_value)) = maybe_header.as_ref() {
@@ -486,7 +506,7 @@ impl HttpClient {
           return Ok(new_response);
         }
       }
-      Err(custom_error("Http", "Too many redirects."))
+      Err(DownloadError::TooManyRedirects)
     } else {
       Ok(response)
     }
@@ -496,7 +516,7 @@ impl HttpClient {
 async fn get_response_body_with_progress(
   response: reqwest::Response,
   progress_guard: Option<&UpdateGuard>,
-) -> Result<Vec<u8>, AnyError> {
+) -> Result<Vec<u8>, reqwest::Error> {
   if let Some(progress_guard) = progress_guard {
     if let Some(total_size) = response.content_length() {
       progress_guard.set_total_size(total_size);
@@ -546,7 +566,7 @@ fn resolve_url_from_location(base_url: &Url, location: &str) -> Url {
 fn resolve_redirect_from_response(
   request_url: &Url,
   response: &reqwest::Response,
-) -> Result<Url, AnyError> {
+) -> Result<Url, DownloadError> {
   debug_assert!(response.status().is_redirection());
   if let Some(location) = response.headers().get(LOCATION) {
     let location_string = location.to_str()?;
@@ -554,9 +574,9 @@ fn resolve_redirect_from_response(
     let new_url = resolve_url_from_location(request_url, location_string);
     Ok(new_url)
   } else {
-    Err(generic_error(format!(
+    Err(DownloadError::NoRedirectHeader {
-      "Redirection from '{request_url}' did not provide location header"
+      request_url: request_url.clone(),
-    )))
+    })
   }
 }
 

cli/npm/managed/cache/tarball.rs

@@ -15,8 +15,11 @@ use deno_npm::npm_rc::ResolvedNpmRc;
 use deno_npm::registry::NpmPackageVersionDistInfo;
 use deno_runtime::deno_fs::FileSystem;
 use deno_semver::package::PackageNv;
+use reqwest::StatusCode;
+use reqwest::Url;
 
 use crate::args::CacheSetting;
+use crate::http_util::DownloadError;
 use crate::http_util::HttpClientProvider;
 use crate::npm::common::maybe_auth_header_for_npm_registry;
 use crate::util::progress_bar::ProgressBar;
@@ -138,8 +141,6 @@ impl TarballCache {
     let tarball_cache = self.clone();
     async move {
       let registry_url = tarball_cache.npmrc.get_registry_url(&package_nv.name);
-      let registry_config =
-        tarball_cache.npmrc.get_registry_config(&package_nv.name).clone();
       let package_folder =
         tarball_cache.cache.package_folder_for_nv_and_url(&package_nv, registry_url);
       let should_use_cache = tarball_cache.cache.should_use_cache_for_package(&package_nv);
@@ -161,14 +162,40 @@ impl TarballCache {
         bail!("Tarball URL was empty.");
       }
 
-      let maybe_auth_header =
+      // IMPORTANT: npm registries may specify tarball URLs at different URLS than the
-        maybe_auth_header_for_npm_registry(&registry_config);
+      // registry, so we MUST get the auth for the tarball URL and not the registry URL.
+      let tarball_uri = Url::parse(&dist.tarball)?;
+      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));
 
       let guard = tarball_cache.progress_bar.update(&dist.tarball);
-      let maybe_bytes = tarball_cache.http_client_provider
+      let result = tarball_cache.http_client_provider
         .get_or_create()?
-        .download_with_progress(&dist.tarball, maybe_auth_header, &guard)
+        .download_with_progress(tarball_uri, maybe_auth_header, &guard)
-        .await?;
+        .await;
+      let maybe_bytes = match result {
+        Ok(maybe_bytes) => maybe_bytes,
+        Err(DownloadError::BadResponse(err)) => {
+          if err.status_code == StatusCode::UNAUTHORIZED
+            && maybe_registry_config.is_none()
+            && tarball_cache.npmrc.get_registry_config(&package_nv.name).auth_token.is_some()
+          {
+            bail!(
+              concat!(
+                "No auth for tarball URI, but present for scoped registry.\n\n",
+                "Tarball URI: {}\n",
+                "Scope URI: {}\n\n",
+                "More info here: https://github.com/npm/cli/wiki/%22No-auth-for-URI,-but-auth-present-for-scoped-registry%22"
+              ),
+              dist.tarball,
+              registry_url,
+            )
+          }
+          return Err(err.into())
+        },
+        Err(err) => return Err(err.into()),
+      };
       match maybe_bytes {
         Some(bytes) => {
           let extraction_mode = if should_use_cache || !package_folder_exists {

tests/integration/lsp_tests.rs

@@ -8896,8 +8896,8 @@ fn lsp_npmrc() {
   temp_dir.write(
     temp_dir.path().join(".npmrc"),
     "\
-@denotest:registry=http://127.0.0.1:4261/
+@denotest:registry=http://localhost:4261/
-//127.0.0.1:4261/:_authToken=private-reg-token
+//localhost:4261/:_authToken=private-reg-token
 ",
   );
   let file = source_file(

tests/registry/npm-private/@denotest/tarballs-privateserver2/1.0.0/index.js

@@ -0,0 +1 @@
+module.exports = () => 'hi_private1';

tests/registry/npm-private/@denotest/tarballs-privateserver2/1.0.0/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "@denotest/tarballs-privateserver2",
+  "version": "1.0.0"
+}

tests/registry/npm-private2/@denotest/tarballs-privateserver2/1.0.0/index.js

@@ -0,0 +1 @@
+module.exports = () => 'hi_private2';

tests/registry/npm-private2/@denotest/tarballs-privateserver2/1.0.0/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "@denotest/tarballs-privateserver2",
+  "version": "1.0.0"
+}

tests/registry/npm/@denotest/tarballs-privateserver2/1.0.0/index.js

@@ -0,0 +1,2 @@
+// this is a special package that the test server serves tarballs from the second private registry server
+module.exports = () => 'hi';

tests/registry/npm/@denotest/tarballs-privateserver2/1.0.0/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "@denotest/tarballs-privateserver2",
+  "version": "1.0.0"
+}

tests/specs/compile/npmrc/.npmrc

@@ -1,4 +1,4 @@
-@denotest:registry=http://127.0.0.1:4261/
+@denotest:registry=http://localhost:4261/
-//127.0.0.1:4261/:_authToken=private-reg-token
+//localhost:4261/:_authToken=private-reg-token
-@denotest2:registry=http://127.0.0.1:4262/
+@denotest2:registry=http://localhost:4262/
-//127.0.0.1:4262/:_authToken=private-reg-token2
+//localhost:4262/:_authToken=private-reg-token2

tests/specs/compile/npmrc/install.out

@@ -1,7 +1,7 @@
 ⚠️ `deno install` behavior will change in Deno 2. To preserve the current behavior use the `-g` or `--global` flag.
 [UNORDERED_START]
-Download http://127.0.0.1:4261/@denotest/basic
+Download http://localhost:4261/@denotest/basic
-Download http://127.0.0.1:4262/@denotest2/basic
+Download http://localhost:4262/@denotest2/basic
 Download http://localhost:4261/@denotest/basic/1.0.0.tgz
 Download http://localhost:4262/@denotest2/basic/1.0.0.tgz
 Initialize @denotest2/basic@1.0.0

tests/specs/npm/npmrc/.npmrc

@@ -1,4 +1,4 @@
-@denotest:registry=http://127.0.0.1:4261/
+@denotest:registry=http://localhost:4261/
-//127.0.0.1:4261/:_authToken=private-reg-token
+//localhost:4261/:_authToken=private-reg-token
-@denotest2:registry=http://127.0.0.1:4262/
+@denotest2:registry=http://localhost:4262/
-//127.0.0.1:4262/:_authToken=private-reg-token2
+//localhost:4262/:_authToken=private-reg-token2

tests/specs/npm/npmrc/install.out

@@ -1,7 +1,7 @@
 ⚠️ `deno install` behavior will change in Deno 2. To preserve the current behavior use the `-g` or `--global` flag.
 [UNORDERED_START]
-Download http://127.0.0.1:4261/@denotest/basic
+Download http://localhost:4261/@denotest/basic
-Download http://127.0.0.1:4262/@denotest2/basic
+Download http://localhost:4262/@denotest2/basic
 Download http://localhost:4261/@denotest/basic/1.0.0.tgz
 Download http://localhost:4262/@denotest2/basic/1.0.0.tgz
 Initialize @denotest2/basic@1.0.0

tests/specs/npm/npmrc_bad_registry_config/.npmrc

@@ -1,5 +1,5 @@
-@denotest:registry=http://127.0.0.1:4261/
+@denotest:registry=http://localhost:4261/
 ; This configuration is wrong - the registry URL must
 ; be exactly the same as registry configured for the scope,
 ; not root url + scope name.
-//127.0.0.1:4261/denotest/:_authToken=invalid-token
+//localhost:4261/denotest/:_authToken=invalid-token

tests/specs/npm/npmrc_bad_registry_config/main.out

@@ -1,4 +1,4 @@
 ⚠️ `deno install` behavior will change in Deno 2. To preserve the current behavior use the `-g` or `--global` flag.
-Download http://127.0.0.1:4261/@denotest/basic
+Download http://localhost:4261/@denotest/basic
-error: Error getting response at http://127.0.0.1:4261/@denotest/basic for package "@denotest/basic": Bad response: 401
+error: Error getting response at http://localhost:4261/@denotest/basic for package "@denotest/basic": Bad response: 401
 [WILDCARD]

tests/specs/npm/npmrc_bad_token/.npmrc

@@ -1,2 +1,2 @@
-@denotest:registry=http://127.0.0.1:4261/
+@denotest:registry=http://localhost:4261/
-//127.0.0.1:4261/:_authToken=invalid-token
+//localhost:4261/:_authToken=invalid-token

tests/specs/npm/npmrc_bad_token/main.out

@@ -1,4 +1,4 @@
 ⚠️ `deno install` behavior will change in Deno 2. To preserve the current behavior use the `-g` or `--global` flag.
-Download http://127.0.0.1:4261/@denotest/basic
+Download http://localhost:4261/@denotest/basic
-error: Error getting response at http://127.0.0.1:4261/@denotest/basic for package "@denotest/basic": Bad response: 401
+error: Error getting response at http://localhost:4261/@denotest/basic for package "@denotest/basic": Bad response: 401
 [WILDCARD]

tests/specs/npm/npmrc_basic_auth/.npmrc

@@ -1,4 +1,4 @@
-@denotest:registry=http://127.0.0.1:4261/
+@denotest:registry=http://localhost:4261/
-//127.0.0.1:4261/:_auth=ZGVubzpsYW5k
+//localhost:4261/:_auth=ZGVubzpsYW5k
-@denotest2:registry=http://127.0.0.1:4262/
+@denotest2:registry=http://localhost:4262/
-//127.0.0.1:4262/:_auth=ZGVubzpsYW5kMg==
+//localhost:4262/:_auth=ZGVubzpsYW5kMg==

tests/specs/npm/npmrc_basic_auth/install.out

@@ -1,7 +1,7 @@
 ⚠️ `deno install` behavior will change in Deno 2. To preserve the current behavior use the `-g` or `--global` flag.
 [UNORDERED_START]
-Download http://127.0.0.1:4261/@denotest/basic
+Download http://localhost:4261/@denotest/basic
-Download http://127.0.0.1:4262/@denotest2/basic
+Download http://localhost:4262/@denotest2/basic
 Download http://localhost:4261/@denotest/basic/1.0.0.tgz
 Download http://localhost:4262/@denotest2/basic/1.0.0.tgz
 Initialize @denotest2/basic@1.0.0

tests/specs/npm/npmrc_deno_json/.npmrc

@@ -1,2 +1,2 @@
-@denotest:registry=http://127.0.0.1:4261/
+@denotest:registry=http://localhost:4261/
-//127.0.0.1:4261/:_authToken=private-reg-token
+//localhost:4261/:_authToken=private-reg-token

tests/specs/npm/npmrc_deno_json/main.out

@@ -1,4 +1,4 @@
-Download http://127.0.0.1:4261/@denotest/basic
+Download http://localhost:4261/@denotest/basic
 Download http://localhost:4261/@denotest/basic/1.0.0.tgz
 0
 42

tests/specs/npm/npmrc_not_next_to_package_json/.npmrc

@@ -1,2 +1,2 @@
-@denotest:registry=http://127.0.0.1:4261/
+@denotest:registry=http://localhost:4261/
-//127.0.0.1:4261/:_authToken=private-reg-token
+//localhost:4261/:_authToken=private-reg-token

tests/specs/npm/npmrc_tarball_other_server/__test__.jsonc

@@ -0,0 +1,17 @@
+{
+  "tempDir": true,
+  "tests": {
+    "auth_success": {
+      "cwd": "success",
+      "args": "run --node-modules-dir -A main.js",
+      "output": "success/main.out",
+      "exitCode": 1
+    },
+    "auth_fail": {
+      "cwd": "fail",
+      "args": "run --node-modules-dir -A main.js",
+      "output": "fail/main.out",
+      "exitCode": 1
+    }
+  }
+}

tests/specs/npm/npmrc_tarball_other_server/fail/.npmrc

@@ -0,0 +1,2 @@
+@denotest:registry=http://localhost:4261/
+//localhost:4261/:_authToken=private-reg-token

tests/specs/npm/npmrc_tarball_other_server/fail/main.js

@@ -0,0 +1,3 @@
+import getValue from "@denotest/tarballs-privateserver2";
+
+console.log(getValue());

tests/specs/npm/npmrc_tarball_other_server/fail/main.out

@@ -0,0 +1,11 @@
+Download http://localhost:4261/@denotest/tarballs-privateserver2
+Download http://localhost:4262/@denotest/tarballs-privateserver2/1.0.0.tgz
+error: Failed caching npm package '@denotest/tarballs-privateserver2@1.0.0'.
+
+Caused by:
+    No auth for tarball URI, but present for scoped registry.
+    
+    Tarball URI: http://localhost:4262/@denotest/tarballs-privateserver2/1.0.0.tgz
+    Scope URI: http://localhost:4261/
+    
+    More info here: https://github.com/npm/cli/wiki/%22No-auth-for-URI,-but-auth-present-for-scoped-registry%22

tests/specs/npm/npmrc_tarball_other_server/fail/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "npmrc_test",
+  "version": "0.0.1",
+  "dependencies": {
+    "@denotest/tarballs-privateserver2": "1.0.0"
+  }
+}

tests/specs/npm/npmrc_tarball_other_server/success/.npmrc

@@ -0,0 +1,3 @@
+@denotest:registry=http://localhost:4261/
+//localhost:4261/:_authToken=private-reg-token
+//localhost:4262/:_authToken=private-reg-token2

tests/specs/npm/npmrc_tarball_other_server/success/main.js

@@ -0,0 +1,3 @@
+import getValue from "@denotest/tarballs-privateserver2";
+
+console.log(getValue());

tests/specs/npm/npmrc_tarball_other_server/success/main.out

@@ -0,0 +1,10 @@
+Download http://localhost:4261/@denotest/tarballs-privateserver2
+Download http://localhost:4262/@denotest/tarballs-privateserver2/1.0.0.tgz
+[# This fails on a checksum issue, because the test server isn't smart enough]
+[# to serve proper checksums for a package at another registry. That's fine]
+[# though because this shows us that we're making it to this step instead of]
+[# failing sooner on an auth issue.]
+error: Failed caching npm package '@denotest/tarballs-privateserver2@1.0.0'.
+
+Caused by:
+    Tarball checksum did not match [WILDCARD]

tests/specs/npm/npmrc_tarball_other_server/success/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "npmrc_test",
+  "version": "0.0.1",
+  "dependencies": {
+    "@denotest/tarballs-privateserver2": "1.0.0"
+  }
+}

tests/util/server/src/npm.rs

@@ -165,6 +165,12 @@ fn get_npm_package(
   local_path: &str,
   package_name: &str,
 ) -> Result<Option<CustomNpmPackage>> {
+  let registry_hostname = if package_name == "@denotest/tarballs-privateserver2"
+  {
+    "http://localhost:4262"
+  } else {
+    registry_hostname
+  };
   let package_folder = tests_path()
     .join("registry")
     .join(local_path)