Skip to content

Commit 566adb7

Browse files
authored
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.
1 parent 0db73f6 commit 566adb7

File tree

35 files changed

+194
-61
lines changed

35 files changed

+194
-61
lines changed

Cargo.lock

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

cli/Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ deno_emit = "=0.42.0"
7272
deno_graph = { version = "=0.78.0", features = ["tokio_executor"] }
7373
deno_lint = { version = "=0.60.0", features = ["docs"] }
7474
deno_lockfile.workspace = true
75-
deno_npm = "=0.21.1"
75+
deno_npm = "=0.21.2"
7676
deno_runtime = { workspace = true, features = ["include_js_files_for_snapshotting"] }
7777
deno_semver = "=0.5.4"
7878
deno_task_shell = "=0.16.1"

cli/args/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -619,6 +619,7 @@ pub fn create_default_npmrc() -> Arc<ResolvedNpmRc> {
619619
config: Default::default(),
620620
},
621621
scopes: Default::default(),
622+
registry_configs: Default::default(),
622623
})
623624
}
624625

cli/http_util.rs

Lines changed: 38 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ use crate::version::get_user_agent;
77
use cache_control::Cachability;
88
use cache_control::CacheControl;
99
use chrono::DateTime;
10-
use deno_core::anyhow::bail;
1110
use deno_core::error::custom_error;
1211
use deno_core::error::generic_error;
1312
use deno_core::error::AnyError;
@@ -30,6 +29,7 @@ use std::sync::Arc;
3029
use std::thread::ThreadId;
3130
use std::time::Duration;
3231
use std::time::SystemTime;
32+
use thiserror::Error;
3333

3434
// TODO(ry) HTTP headers are not unique key, value pairs. There may be more than
3535
// one header line with the same key. This should be changed to something like
@@ -260,6 +260,27 @@ impl HttpClientProvider {
260260
}
261261
}
262262

263+
#[derive(Debug, Error)]
264+
#[error("Bad response: {:?}{}", .status_code, .response_text.as_ref().map(|s| format!("\n\n{}", s)).unwrap_or_else(String::new))]
265+
pub struct BadResponseError {
266+
pub status_code: StatusCode,
267+
pub response_text: Option<String>,
268+
}
269+
270+
#[derive(Debug, Error)]
271+
pub enum DownloadError {
272+
#[error(transparent)]
273+
Reqwest(#[from] reqwest::Error),
274+
#[error(transparent)]
275+
ToStr(#[from] reqwest::header::ToStrError),
276+
#[error("Redirection from '{}' did not provide location header", .request_url)]
277+
NoRedirectHeader { request_url: Url },
278+
#[error("Too many redirects.")]
279+
TooManyRedirects,
280+
#[error(transparent)]
281+
BadResponse(#[from] BadResponseError),
282+
}
283+
263284
#[derive(Debug)]
264285
pub struct HttpClient {
265286
#[allow(clippy::disallowed_types)] // reqwest::Client allowed here
@@ -409,7 +430,7 @@ impl HttpClient {
409430
url: impl reqwest::IntoUrl,
410431
maybe_header: Option<(HeaderName, HeaderValue)>,
411432
progress_guard: &UpdateGuard,
412-
) -> Result<Option<Vec<u8>>, AnyError> {
433+
) -> Result<Option<Vec<u8>>, DownloadError> {
413434
self
414435
.download_inner(url, maybe_header, Some(progress_guard))
415436
.await
@@ -429,34 +450,33 @@ impl HttpClient {
429450
url: impl reqwest::IntoUrl,
430451
maybe_header: Option<(HeaderName, HeaderValue)>,
431452
progress_guard: Option<&UpdateGuard>,
432-
) -> Result<Option<Vec<u8>>, AnyError> {
453+
) -> Result<Option<Vec<u8>>, DownloadError> {
433454
let response = self.get_redirected_response(url, maybe_header).await?;
434455

435456
if response.status() == 404 {
436457
return Ok(None);
437458
} else if !response.status().is_success() {
438459
let status = response.status();
439460
let maybe_response_text = response.text().await.ok();
440-
bail!(
441-
"Bad response: {:?}{}",
442-
status,
443-
match maybe_response_text {
444-
Some(text) => format!("\n\n{text}"),
445-
None => String::new(),
446-
}
447-
);
461+
return Err(DownloadError::BadResponse(BadResponseError {
462+
status_code: status,
463+
response_text: maybe_response_text
464+
.map(|s| s.trim().to_string())
465+
.filter(|s| !s.is_empty()),
466+
}));
448467
}
449468

450469
get_response_body_with_progress(response, progress_guard)
451470
.await
452471
.map(Some)
472+
.map_err(Into::into)
453473
}
454474

455475
async fn get_redirected_response(
456476
&self,
457477
url: impl reqwest::IntoUrl,
458478
mut maybe_header: Option<(HeaderName, HeaderValue)>,
459-
) -> Result<reqwest::Response, AnyError> {
479+
) -> Result<reqwest::Response, DownloadError> {
460480
let mut url = url.into_url()?;
461481
let mut builder = self.get(url.clone());
462482
if let Some((header_name, header_value)) = maybe_header.as_ref() {
@@ -486,7 +506,7 @@ impl HttpClient {
486506
return Ok(new_response);
487507
}
488508
}
489-
Err(custom_error("Http", "Too many redirects."))
509+
Err(DownloadError::TooManyRedirects)
490510
} else {
491511
Ok(response)
492512
}
@@ -496,7 +516,7 @@ impl HttpClient {
496516
async fn get_response_body_with_progress(
497517
response: reqwest::Response,
498518
progress_guard: Option<&UpdateGuard>,
499-
) -> Result<Vec<u8>, AnyError> {
519+
) -> Result<Vec<u8>, reqwest::Error> {
500520
if let Some(progress_guard) = progress_guard {
501521
if let Some(total_size) = response.content_length() {
502522
progress_guard.set_total_size(total_size);
@@ -546,17 +566,17 @@ fn resolve_url_from_location(base_url: &Url, location: &str) -> Url {
546566
fn resolve_redirect_from_response(
547567
request_url: &Url,
548568
response: &reqwest::Response,
549-
) -> Result<Url, AnyError> {
569+
) -> Result<Url, DownloadError> {
550570
debug_assert!(response.status().is_redirection());
551571
if let Some(location) = response.headers().get(LOCATION) {
552572
let location_string = location.to_str()?;
553573
log::debug!("Redirecting to {:?}...", &location_string);
554574
let new_url = resolve_url_from_location(request_url, location_string);
555575
Ok(new_url)
556576
} else {
557-
Err(generic_error(format!(
558-
"Redirection from '{request_url}' did not provide location header"
559-
)))
577+
Err(DownloadError::NoRedirectHeader {
578+
request_url: request_url.clone(),
579+
})
560580
}
561581
}
562582

cli/npm/managed/cache/tarball.rs

Lines changed: 34 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,11 @@ use deno_npm::npm_rc::ResolvedNpmRc;
1515
use deno_npm::registry::NpmPackageVersionDistInfo;
1616
use deno_runtime::deno_fs::FileSystem;
1717
use deno_semver::package::PackageNv;
18+
use reqwest::StatusCode;
19+
use reqwest::Url;
1820

1921
use crate::args::CacheSetting;
22+
use crate::http_util::DownloadError;
2023
use crate::http_util::HttpClientProvider;
2124
use crate::npm::common::maybe_auth_header_for_npm_registry;
2225
use crate::util::progress_bar::ProgressBar;
@@ -138,8 +141,6 @@ impl TarballCache {
138141
let tarball_cache = self.clone();
139142
async move {
140143
let registry_url = tarball_cache.npmrc.get_registry_url(&package_nv.name);
141-
let registry_config =
142-
tarball_cache.npmrc.get_registry_config(&package_nv.name).clone();
143144
let package_folder =
144145
tarball_cache.cache.package_folder_for_nv_and_url(&package_nv, registry_url);
145146
let should_use_cache = tarball_cache.cache.should_use_cache_for_package(&package_nv);
@@ -161,14 +162,40 @@ impl TarballCache {
161162
bail!("Tarball URL was empty.");
162163
}
163164

164-
let maybe_auth_header =
165-
maybe_auth_header_for_npm_registry(&registry_config);
165+
// IMPORTANT: npm registries may specify tarball URLs at different URLS than the
166+
// registry, so we MUST get the auth for the tarball URL and not the registry URL.
167+
let tarball_uri = Url::parse(&dist.tarball)?;
168+
let maybe_registry_config =
169+
tarball_cache.npmrc.tarball_config(&tarball_uri);
170+
let maybe_auth_header = maybe_registry_config.and_then(|c| maybe_auth_header_for_npm_registry(c));
166171

167172
let guard = tarball_cache.progress_bar.update(&dist.tarball);
168-
let maybe_bytes = tarball_cache.http_client_provider
173+
let result = tarball_cache.http_client_provider
169174
.get_or_create()?
170-
.download_with_progress(&dist.tarball, maybe_auth_header, &guard)
171-
.await?;
175+
.download_with_progress(tarball_uri, maybe_auth_header, &guard)
176+
.await;
177+
let maybe_bytes = match result {
178+
Ok(maybe_bytes) => maybe_bytes,
179+
Err(DownloadError::BadResponse(err)) => {
180+
if err.status_code == StatusCode::UNAUTHORIZED
181+
&& maybe_registry_config.is_none()
182+
&& tarball_cache.npmrc.get_registry_config(&package_nv.name).auth_token.is_some()
183+
{
184+
bail!(
185+
concat!(
186+
"No auth for tarball URI, but present for scoped registry.\n\n",
187+
"Tarball URI: {}\n",
188+
"Scope URI: {}\n\n",
189+
"More info here: https://github.com/npm/cli/wiki/%22No-auth-for-URI,-but-auth-present-for-scoped-registry%22"
190+
),
191+
dist.tarball,
192+
registry_url,
193+
)
194+
}
195+
return Err(err.into())
196+
},
197+
Err(err) => return Err(err.into()),
198+
};
172199
match maybe_bytes {
173200
Some(bytes) => {
174201
let extraction_mode = if should_use_cache || !package_folder_exists {

tests/integration/lsp_tests.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8896,8 +8896,8 @@ fn lsp_npmrc() {
88968896
temp_dir.write(
88978897
temp_dir.path().join(".npmrc"),
88988898
"\
8899-
@denotest:registry=http://127.0.0.1:4261/
8900-
//127.0.0.1:4261/:_authToken=private-reg-token
8899+
@denotest:registry=http://localhost:4261/
8900+
//localhost:4261/:_authToken=private-reg-token
89018901
",
89028902
);
89038903
let file = source_file(
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
module.exports = () => 'hi_private1';
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
{
2+
"name": "@denotest/tarballs-privateserver2",
3+
"version": "1.0.0"
4+
}
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
module.exports = () => 'hi_private2';
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
{
2+
"name": "@denotest/tarballs-privateserver2",
3+
"version": "1.0.0"
4+
}

0 commit comments

Comments
 (0)