From ddbb4899c252efabce5bbf4e519df83c07b891bf Mon Sep 17 00:00:00 2001 From: Alessio Attilio Date: Wed, 1 Jul 2026 18:09:02 +0200 Subject: [PATCH] feat(pacquet): implement bugs command (#12687) Port the `pnpm bugs` command to pacquet, following the structure of the TypeScript handler at `pnpm11/deps/inspection/commands/src/bugs/index.ts` and matching its error codes (`ERR_PNPM_NO_BUGS_URL`, `ERR_PNPM_NO_IMPORTER_MANIFEST_FOUND`). The command supports local manifest lookup and registry lookup by package name, with repository URL normalization for GitHub/GitLab/Bitbucket shorthand, hosted git URLs, and self-hosted git servers. Unit tests cover all URL derivation branches (36 tests). Integration tests cover the CLI entry points with local manifests and a mocked registry (9 tests). Related to pnpm/pnpm#11633. --- .github/workflows/release.yml | 2 +- .github/workflows/test.yml | 2 +- .github/workflows/update-latest.yml | 2 +- pacquet/crates/auth-commands/src/logout.rs | 26 +- pacquet/crates/cli/src/cli_args.rs | 2 + .../cli/src/cli_args/audit/signatures.rs | 34 +- .../src/cli_args/audit/signatures/tests.rs | 6 +- pacquet/crates/cli/src/cli_args/bugs.rs | 400 ++++++++++++++++++ pacquet/crates/cli/src/cli_args/bugs/tests.rs | 297 +++++++++++++ .../crates/cli/src/cli_args/cli_command.rs | 4 + pacquet/crates/cli/src/cli_args/dispatch.rs | 1 + .../crates/cli/src/cli_args/dispatch_query.rs | 7 + pacquet/crates/cli/src/cli_args/dist_tag.rs | 18 +- .../src/cli_args/self_update/verify_engine.rs | 44 +- pacquet/crates/cli/tests/bugs.rs | 298 +++++++++++++ pacquet/crates/network/src/lib.rs | 3 + pacquet/crates/network/src/url_encoding.rs | 28 ++ .../crates/package-manager/src/add/tests.rs | 4 +- pacquet/crates/registry/src/package.rs | 13 +- pacquet/crates/registry/src/package_tag.rs | 40 +- .../crates/registry/src/package_tag/tests.rs | 13 + .../crates/registry/src/package_version.rs | 11 +- .../resolving-git-resolver/src/hosted_git.rs | 20 +- pnpr/crates/pnpr/src/server.rs | 8 +- 24 files changed, 1141 insertions(+), 142 deletions(-) create mode 100644 pacquet/crates/cli/src/cli_args/bugs.rs create mode 100644 pacquet/crates/cli/src/cli_args/bugs/tests.rs create mode 100644 pacquet/crates/cli/tests/bugs.rs create mode 100644 pacquet/crates/network/src/url_encoding.rs create mode 100644 pacquet/crates/registry/src/package_tag/tests.rs diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 8b41f5616a..30a7f7dfa2 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -43,7 +43,7 @@ jobs: uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: persist-credentials: false - - uses: garnet-org/action@2b7fc9d79b54f551b43358c27424a36064b3e078 # v2 + - uses: garnet-org/action@2b7fc9d79b54f551b43358c27424a36064b3e078 # v2.0.2 with: api_token: ${{ secrets.GARNET_API_TOKEN }} - name: Install pnpm and Node diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index e770cded32..e96388dada 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -60,7 +60,7 @@ jobs: with: persist-credentials: false - if: ${{ inputs.garnet }} - uses: garnet-org/action@2b7fc9d79b54f551b43358c27424a36064b3e078 # v2 + uses: garnet-org/action@2b7fc9d79b54f551b43358c27424a36064b3e078 # v2.0.2 with: api_token: ${{ secrets.GARNET_API_TOKEN }} - name: Install pnpm and Node diff --git a/.github/workflows/update-latest.yml b/.github/workflows/update-latest.yml index 1b860a38e8..5827d404fd 100644 --- a/.github/workflows/update-latest.yml +++ b/.github/workflows/update-latest.yml @@ -18,7 +18,7 @@ jobs: environment: release runs-on: ubuntu-latest steps: - - uses: garnet-org/action@2b7fc9d79b54f551b43358c27424a36064b3e078 # v2 + - uses: garnet-org/action@2b7fc9d79b54f551b43358c27424a36064b3e078 # v2.0.2 with: api_token: ${{ secrets.GARNET_API_TOKEN }} - name: Setup Node diff --git a/pacquet/crates/auth-commands/src/logout.rs b/pacquet/crates/auth-commands/src/logout.rs index 61cbf3796b..7576780c4d 100644 --- a/pacquet/crates/auth-commands/src/logout.rs +++ b/pacquet/crates/auth-commands/src/logout.rs @@ -14,7 +14,8 @@ use std::{collections::HashMap, future::Future, io, path::PathBuf}; use derive_more::{Display, Error}; use miette::Diagnostic; use pacquet_network::{ - RetryOpts, ThrottledClient, nerf_dart, redact_and_sanitize, send_with_retry, + RetryOpts, ThrottledClient, encode_uri_component, nerf_dart, redact_and_sanitize, + send_with_retry, }; use pacquet_reporter::{LogEvent, LogLevel, PnpmLog, Reporter}; @@ -222,29 +223,6 @@ fn revoke_log_url(revoke_url: &str) -> &str { revoke_url.rsplit_once('/').map_or(revoke_url, |(prefix, _token)| prefix) } -/// Percent-encode `value` the way JavaScript's `encodeURIComponent` -/// does: every byte outside the unreserved set `A-Za-z0-9-_.!~*'()` is -/// escaped as `%XX` with uppercase hex digits. -fn encode_uri_component(value: &str) -> String { - let mut out = String::with_capacity(value.len()); - for &byte in value.as_bytes() { - if byte.is_ascii_alphanumeric() - || matches!(byte, b'-' | b'_' | b'.' | b'!' | b'~' | b'*' | b'\'' | b'(' | b')') - { - out.push(byte as char); - } else { - out.push('%'); - out.push(hex_upper(byte >> 4)); - out.push(hex_upper(byte & 0x0f)); - } - } - out -} - -fn hex_upper(nibble: u8) -> char { - char::from_digit(u32::from(nibble), 16).expect("nibble is < 16").to_ascii_uppercase() -} - /// Errors surfaced by [`logout`]. The two user-facing variants carry /// pnpm's stable error codes (`ERR_PNPM_NOT_LOGGED_IN`, /// `ERR_PNPM_LOGOUT_FAILED`) and messages verbatim. diff --git a/pacquet/crates/cli/src/cli_args.rs b/pacquet/crates/cli/src/cli_args.rs index e65ec48b0e..7a42abb86a 100644 --- a/pacquet/crates/cli/src/cli_args.rs +++ b/pacquet/crates/cli/src/cli_args.rs @@ -2,6 +2,7 @@ pub mod add; pub mod approve_builds; pub mod audit; pub mod bin; +pub mod bugs; pub mod cache; pub mod cat_file; pub mod cat_index; @@ -26,6 +27,7 @@ pub mod logout; pub mod outdated; pub mod pack; pub mod pack_app; + pub mod patch; pub mod patch_commit; pub mod patch_remove; diff --git a/pacquet/crates/cli/src/cli_args/audit/signatures.rs b/pacquet/crates/cli/src/cli_args/audit/signatures.rs index 2901cdd356..9f9319c949 100644 --- a/pacquet/crates/cli/src/cli_args/audit/signatures.rs +++ b/pacquet/crates/cli/src/cli_args/audit/signatures.rs @@ -11,10 +11,7 @@ //! signature is present but does not validate is **invalid** — a tamper //! signal. -use std::{ - collections::{BTreeMap, BTreeSet, HashMap}, - fmt::Write as _, -}; +use std::collections::{BTreeMap, BTreeSet, HashMap}; use base64::Engine as _; use owo_colors::{OwoColorize, Stream}; @@ -23,7 +20,9 @@ use p256::{ pkcs8::DecodePublicKey, }; use pacquet_config::Config; -use pacquet_network::{ThrottledClient, redact_url_credentials, send_with_retry}; +use pacquet_network::{ + ThrottledClient, encode_package_name, redact_url_credentials, send_with_retry, +}; use serde::{Deserialize, Serialize}; use super::{bold, red, retry_opts_from_config, sanitize_response_body}; @@ -514,31 +513,6 @@ fn with_trailing_slash(registry: &str) -> String { if registry.ends_with('/') { registry.to_string() } else { format!("{registry}/") } } -/// Percent-encode a package name for a packument URL, matching pnpm's -/// `toUri`: a scoped name keeps its leading `@` and encodes the rest (so the -/// `/` becomes `%2F`), an unscoped name is encoded whole. -fn encode_package_name(name: &str) -> String { - match name.strip_prefix('@') { - Some(rest) => format!("@{}", encode_uri_component(rest)), - None => encode_uri_component(name), - } -} - -/// Port of JavaScript `encodeURIComponent`: every UTF-8 byte outside the -/// unreserved set is percent-encoded. -fn encode_uri_component(input: &str) -> String { - const UNRESERVED: &[u8] = b"-_.!~*'()"; - let mut output = String::with_capacity(input.len()); - for &byte in input.as_bytes() { - if byte.is_ascii_alphanumeric() || UNRESERVED.contains(&byte) { - output.push(byte as char); - } else { - write!(output, "%{byte:02X}").expect("writing to a String never fails"); - } - } - output -} - pub(super) fn render_signature_verification_result(result: &SignatureVerificationResult) -> String { let mut lines: Vec = Vec::new(); lines.push(format!("audited {} {}", result.audited, plural(result.audited, "package"))); diff --git a/pacquet/crates/cli/src/cli_args/audit/signatures/tests.rs b/pacquet/crates/cli/src/cli_args/audit/signatures/tests.rs index 7bc11ff10e..c611d63fb5 100644 --- a/pacquet/crates/cli/src/cli_args/audit/signatures/tests.rs +++ b/pacquet/crates/cli/src/cli_args/audit/signatures/tests.rs @@ -1,10 +1,10 @@ use super::{ - PackageSignature, RegistryKey, SignaturePackage, SignatureVerificationResult, - encode_package_name, parse_timestamp, render_signature_verification_result, verify_one, - verify_package_signatures, + PackageSignature, RegistryKey, SignaturePackage, SignatureVerificationResult, parse_timestamp, + render_signature_verification_result, verify_one, verify_package_signatures, }; use base64::Engine as _; use p256::ecdsa::SigningKey; +use pacquet_network::encode_package_name; fn signing_key() -> SigningKey { SigningKey::from_slice(&[0x42; 32]).expect("valid P-256 scalar") diff --git a/pacquet/crates/cli/src/cli_args/bugs.rs b/pacquet/crates/cli/src/cli_args/bugs.rs new file mode 100644 index 0000000000..60382f0bc2 --- /dev/null +++ b/pacquet/crates/cli/src/cli_args/bugs.rs @@ -0,0 +1,400 @@ +use std::path::Path; + +use derive_more::{Display, Error}; +use miette::{Context, Diagnostic}; +use pacquet_config::Config; +use pacquet_package_manifest::safe_read_package_json_from_dir; +use pacquet_registry::{PackageTag, PackageVersion}; +use serde_json::Value; +use url::Url; + +use crate::cli_args::registry_client::build_registry_client; + +#[derive(Debug, Display, Error, Diagnostic)] +#[non_exhaustive] +pub enum BugsError { + #[display( + "The current project does not have a bug tracker URL. \ + Add a \"bugs\" or \"repository\" field to its manifest." + )] + #[diagnostic(code(ERR_PNPM_NO_BUGS_URL))] + NoBugsUrl, + + #[display("The package \"{package}\" does not have a bug tracker URL.")] + #[diagnostic(code(ERR_PNPM_NO_BUGS_URL))] + NoBugsUrlForPackage { package: String }, + + #[display("Registry request to {url} failed: {reason}")] + #[diagnostic(code(ERR_PNPM_REGISTRY_ERROR))] + RegistryError { url: String, reason: String }, +} + +#[derive(Debug, clap::Args)] +pub struct BugsArgs { + #[clap(long)] + pub registry: Option, + + pub packages: Vec, +} + +impl BugsArgs { + pub async fn run(&self, config: &Config, dir: &Path) -> miette::Result<()> { + if self.packages.is_empty() { + let url = get_bugs_url_from_current_project(dir)?; + open_url(&url); + } else { + let http_client = build_registry_client(config) + .wrap_err("build the network client for registry requests")?; + + let registries: std::collections::HashMap = + config.resolved_registries().into_iter().collect(); + + let futures = self.packages.iter().map(|spec| { + let (package_name, _tag) = parse_package_spec(spec); + let target_registry = if let Some(ref override_registry) = self.registry { + normalize_registry_url(override_registry) + } else { + let picked = pacquet_resolving_npm_resolver::pick_registry_for_package( + ®istries, + package_name, + Some(spec), + ); + normalize_registry_url(&picked) + }; + + let http_client = &http_client; + let auth_headers = &config.auth_headers; + async move { + get_bugs_url_from_registry(spec, &target_registry, http_client, auth_headers) + .await + .wrap_err_with(|| format!(r#"look up bugs URL for "{spec}""#)) + } + }); + + let results: Vec> = + futures_util::future::join_all(futures).await; + for res in results { + let url = res?; + open_url(&url); + } + } + Ok(()) + } +} + +fn get_bugs_url_from_current_project(dir: &Path) -> miette::Result { + let manifest = + safe_read_package_json_from_dir(dir).wrap_err("read package.json")?.ok_or_else(|| { + let display_path = dir.display(); + miette::miette!( + code = "ERR_PNPM_NO_IMPORTER_MANIFEST_FOUND", + "No package.json was found in {display_path}", + ) + })?; + + pick_bugs_url(&manifest).ok_or_else(|| BugsError::NoBugsUrl.into()) +} + +async fn get_bugs_url_from_registry( + spec: &str, + registry_url: &str, + http_client: &pacquet_network::ThrottledClient, + auth_headers: &pacquet_network::AuthHeaders, +) -> miette::Result { + let (package_name, tag) = parse_package_spec(spec); + let package_tag = match tag { + None => PackageTag::Latest, + Some(tag_str) => tag_str.parse::().unwrap_or(PackageTag::Latest), + }; + let package_version = PackageVersion::fetch_from_registry( + package_name, + package_tag, + http_client, + registry_url, + auth_headers, + ) + .await + .map_err(|err| { + let (url, reason) = match err { + pacquet_registry::RegistryError::Network(net_err) => ( + pacquet_network::redact_url_credentials(&net_err.url), + pacquet_network::redact_url_credentials(&net_err.error.to_string()), + ), + other => ( + pacquet_network::redact_url_credentials(registry_url), + pacquet_network::redact_url_credentials(&other.to_string()), + ), + }; + BugsError::RegistryError { url, reason } + }) + .wrap_err_with(|| format!(r#"fetch package info for "{package_name}" from the registry"#))?; + + let manifest = package_manifest_from_version(&package_version); + pick_bugs_url(&manifest) + .ok_or_else(|| BugsError::NoBugsUrlForPackage { package: package_name.to_string() }.into()) +} + +fn package_manifest_from_version(version: &PackageVersion) -> Value { + let mut map = serde_json::Map::new(); + map.insert("name".to_string(), Value::String(version.name.clone())); + if let Some(bugs) = version.other.get("bugs") { + map.insert("bugs".to_string(), bugs.clone()); + } + if let Some(repo) = version.other.get("repository") { + map.insert("repository".to_string(), repo.clone()); + } + Value::Object(map) +} + +fn pick_bugs_url(manifest: &Value) -> Option { + if let Some(bugs) = manifest.get("bugs") { + let url = match bugs { + Value::String(url_str) => Some(url_str.clone()), + Value::Object(bugs_obj) => { + bugs_obj.get("url").and_then(|val| val.as_str()).map(String::from) + } + _ => None, + }; + if url.as_ref().is_some_and(|url_str| is_http_url(url_str)) { + return url; + } + } + + if let Some(repo) = manifest.get("repository") { + let url = match repo { + Value::String(url_str) => Some(url_str.clone()), + Value::Object(repo_obj) => { + repo_obj.get("url").and_then(|val| val.as_str()).map(String::from) + } + _ => None, + }; + if let Some(ref url) = url { + return repository_to_issues_url(url); + } + } + + None +} + +fn repository_to_issues_url(raw_url: &str) -> Option { + let mut trimmed = raw_url.trim(); + + // Strip fragment and query first to prevent them from leaking into shorthand or SCP paths + if let Some(pos) = trimmed.find('#') { + trimmed = &trimmed[..pos]; + } + if let Some(pos) = trimmed.find('?') { + trimmed = &trimmed[..pos]; + } + + if let Some(url) = try_hosted_git_shorthand(trimmed) { + return Some(url); + } + + let cleaned = trimmed.strip_prefix("git+").unwrap_or(trimmed); + + // Handle SCP-style SSH URLs: `git@github.com:owner/repo.git` + if let Some(rest) = cleaned.strip_prefix("git@") + && let Some(colon_pos) = rest.find(':') + { + let host = &rest[..colon_pos]; + let path = rest[colon_pos + 1..].trim_end_matches('/').trim_end_matches(".git"); + if !host.is_empty() && !path.is_empty() { + return Some(format!("https://{host}/{path}/issues")); + } + } + + let parsed_url = if let Ok(parsed) = Url::parse(cleaned) { + Some(parsed) + } else if cleaned.contains('/') && !cleaned.contains(':') { + let slash_pos = cleaned.find('/'); + let dot_pos = cleaned.find('.'); + if let Some(slash_pos) = slash_pos + && let Some(dot_pos) = dot_pos + && dot_pos < slash_pos + { + Url::parse(&format!("https://{cleaned}")).ok() + } else { + None + } + } else { + None + }; + + if let Some(parsed) = parsed_url { + match parsed.scheme() { + "http" | "https" => { + let mut url = parsed; + url.set_query(None); + url.set_fragment(None); + let path = url.path().trim_end_matches('/').trim_end_matches(".git"); + if path.is_empty() { + return None; + } + let new_path = format!("{path}/issues"); + url.set_path(&new_path); + Some(url.to_string()) + } + "ssh" | "git" | "git+ssh" => { + let host = parsed.host_str()?; + let path = parsed.path().trim_end_matches('/').trim_end_matches(".git"); + if path.is_empty() { + return None; + } + if let Some(port) = parsed.port() { + Some(format!("https://{host}:{port}{path}/issues")) + } else { + Some(format!("https://{host}{path}/issues")) + } + } + _ => None, + } + } else { + None + } +} + +fn try_hosted_git_shorthand(input: &str) -> Option { + if let Some(rest) = input.strip_prefix("github:") { + let (user, project) = split_user_project(rest)?; + return Some(format!("https://github.com/{user}/{project}/issues")); + } + if let Some(rest) = input.strip_prefix("gitlab:") { + let (user, project) = split_user_project(rest)?; + return Some(format!("https://gitlab.com/{user}/{project}/issues")); + } + if let Some(rest) = input.strip_prefix("bitbucket:") { + let (user, project) = split_user_project(rest)?; + return Some(format!("https://bitbucket.org/{user}/{project}/issues")); + } + + // `owner/repo` shorthand — only when no `:`, `//`, or `@` is present, + // to avoid matching `git+, SCP-style SSH, etc. + if !input.contains(':') && !input.contains("//") && !input.contains('@') { + let (user, project) = split_user_project(input)?; + if !project.contains('/') { + return Some(format!("https://github.com/{user}/{project}/issues")); + } + } + + None +} + +fn split_user_project(spec: &str) -> Option<(&str, &str)> { + let spec = spec.trim_end_matches(".git"); + let slash_pos = spec.find('/')?; + let user = &spec[..slash_pos]; + let project = &spec[slash_pos + 1..]; + if user.is_empty() || project.is_empty() { + return None; + } + Some((user, project)) +} + +fn is_http_url(value: &str) -> bool { + Url::parse(value).is_ok_and(|parsed| parsed.scheme() == "http" || parsed.scheme() == "https") +} + +fn normalize_registry_url(url: &str) -> String { + if url.ends_with('/') { url.to_owned() } else { format!("{url}/") } +} + +fn open_url(url: &str) { + let sanitized = crate::cli_args::sanitize::sanitize(url); + let redacted = pacquet_network::redact_url_credentials(&sanitized); + println!("{redacted}"); + + // Clear username/password before passing to open_url_in_browser: + let clean_url_for_browser = if let Ok(mut parsed) = Url::parse(url) { + if !parsed.username().is_empty() || parsed.password().is_some() { + let _ = parsed.set_username(""); + let _ = parsed.set_password(None); + parsed.to_string() + } else { + url.to_string() + } + } else { + url.to_string() + }; + + let clean_url_for_browser = crate::cli_args::sanitize::sanitize(&clean_url_for_browser); + + let result = open_url_in_browser(&clean_url_for_browser); + if let Err(err) = result { + tracing::debug!(target: "pacquet_cli", %err, "could not open browser"); + } +} + +#[cfg(target_os = "linux")] +fn open_url_in_browser(url: &str) -> std::io::Result<()> { + std::process::Command::new("xdg-open") + .arg(url) + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .spawn()?; + Ok(()) +} + +#[cfg(target_os = "macos")] +fn open_url_in_browser(url: &str) -> std::io::Result<()> { + std::process::Command::new("open") + .arg(url) + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .spawn()?; + Ok(()) +} + +#[cfg(target_os = "windows")] +fn open_url_in_browser(url: &str) -> std::io::Result<()> { + use std::ffi::OsStr; + use std::os::windows::ffi::OsStrExt; + + let url_wide: Vec = OsStr::new(url).encode_wide().chain(std::iter::once(0)).collect(); + + // ShellExecuteW invokes the default handler for the URL protocol + // without shell metacharacter injection. The function takes a + // fully-qualified path (or registered protocol) so it does not + // depend on the executable search path or the SystemRoot env var, + // avoiding the hijack vector that rundll32.exe is subject to. + let result = unsafe { + windows_sys::Win32::UI::Shell::ShellExecuteW( + std::ptr::null_mut(), // hwnd + std::ptr::null(), // lpOperation (null => "open") + url_wide.as_ptr(), // lpFile + std::ptr::null(), // lpParameters + std::ptr::null(), // lpDirectory + windows_sys::Win32::UI::WindowsAndMessaging::SW_SHOWNORMAL, + ) + }; + if (result as isize) > 32 { Ok(()) } else { Err(std::io::Error::last_os_error()) } +} + +#[cfg(not(any(target_os = "linux", target_os = "macos", target_os = "windows")))] +fn open_url_in_browser(_url: &str) -> std::io::Result<()> { + Ok(()) +} + +fn parse_package_spec(spec: &str) -> (&str, Option<&str>) { + let spec = spec.trim(); + if let Some(stripped) = spec.strip_prefix('@') { + if let Some(at_pos) = stripped.rfind('@') + && at_pos > 0 + { + let split_pos = at_pos + 1; + return (&spec[..split_pos], Some(&spec[split_pos + 1..])); + } + (spec, None) + } else if let Some(at_pos) = spec.rfind('@') + && at_pos > 0 + { + (&spec[..at_pos], Some(&spec[at_pos + 1..])) + } else { + (spec, None) + } +} + +#[cfg(test)] +mod tests; diff --git a/pacquet/crates/cli/src/cli_args/bugs/tests.rs b/pacquet/crates/cli/src/cli_args/bugs/tests.rs new file mode 100644 index 0000000000..f5d787b1d8 --- /dev/null +++ b/pacquet/crates/cli/src/cli_args/bugs/tests.rs @@ -0,0 +1,297 @@ +use serde_json::json; + +use super::{ + is_http_url, parse_package_spec, pick_bugs_url, repository_to_issues_url, + try_hosted_git_shorthand, +}; + +#[test] +fn pick_bugs_url_returns_bugs_url_from_object() { + let manifest = json!({ + "bugs": { "url": "https://github.com/test/pkg/issues" } + }); + assert_eq!(pick_bugs_url(&manifest).as_deref(), Some("https://github.com/test/pkg/issues")); +} + +#[test] +fn pick_bugs_url_returns_bugs_url_from_string() { + let manifest = json!({ + "bugs": "https://github.com/test/pkg/issues" + }); + assert_eq!(pick_bugs_url(&manifest).as_deref(), Some("https://github.com/test/pkg/issues")); +} + +#[test] +fn pick_bugs_url_falls_back_to_repository_issues_url() { + let manifest = json!({ + "repository": "https://github.com/test/pkg" + }); + assert_eq!(pick_bugs_url(&manifest).as_deref(), Some("https://github.com/test/pkg/issues")); +} + +#[test] +fn pick_bugs_url_prefers_bugs_over_repository() { + let manifest = json!({ + "bugs": { "url": "https://github.com/other/issues" }, + "repository": "https://github.com/test/pkg" + }); + assert_eq!(pick_bugs_url(&manifest).as_deref(), Some("https://github.com/other/issues")); +} + +#[test] +fn pick_bugs_url_returns_none_when_no_bugs_url() { + let manifest = json!({ "name": "test-pkg" }); + assert_eq!(pick_bugs_url(&manifest), None); +} + +#[test] +fn pick_bugs_url_returns_none_for_non_http_bugs() { + let manifest = json!({ + "bugs": "ftp://example.com/bugs" + }); + assert_eq!(pick_bugs_url(&manifest), None); +} + +#[test] +fn pick_bugs_url_returns_none_for_empty_bugs() { + let manifest = json!({ + "bugs": {} + }); + assert_eq!(pick_bugs_url(&manifest), None); +} + +#[test] +fn repo_url_normalizes_git_https_with_dot_git() { + assert_eq!( + repository_to_issues_url("git+https://github.com/test/pkg.git"), + Some("https://github.com/test/pkg/issues".to_string()), + ); +} + +#[test] +fn repo_url_strips_trailing_slash() { + assert_eq!( + repository_to_issues_url("https://github.com/test/pkg/"), + Some("https://github.com/test/pkg/issues".to_string()), + ); +} + +#[test] +fn repo_url_resolves_shorthand_owner_repo() { + assert_eq!( + repository_to_issues_url("test/pkg"), + Some("https://github.com/test/pkg/issues".to_string()), + ); +} + +#[test] +fn repo_url_resolves_github_shorthand() { + assert_eq!( + repository_to_issues_url("github:test/pkg"), + Some("https://github.com/test/pkg/issues".to_string()), + ); +} + +#[test] +fn repo_url_resolves_git_ssh_url() { + assert_eq!( + repository_to_issues_url("git+ssh://git@github.com/test/pkg.git"), + Some("https://github.com/test/pkg/issues".to_string()), + ); +} + +#[test] +fn repo_url_resolves_gitlab_shorthand() { + assert_eq!( + repository_to_issues_url("gitlab:test/pkg"), + Some("https://gitlab.com/test/pkg/issues".to_string()), + ); +} + +#[test] +fn repo_url_falls_back_for_self_hosted_git_server() { + assert_eq!( + repository_to_issues_url("git+https://git.example.com/test/pkg.git"), + Some("https://git.example.com/test/pkg/issues".to_string()), + ); +} + +#[test] +fn repo_url_handles_dot_git_with_trailing_slash() { + assert_eq!( + repository_to_issues_url("git+https://github.com/test/pkg.git/"), + Some("https://github.com/test/pkg/issues".to_string()), + ); +} + +#[test] +fn repo_url_strips_fragment_and_query() { + assert_eq!( + repository_to_issues_url("git+https://github.com/test/pkg.git#main"), + Some("https://github.com/test/pkg/issues".to_string()), + ); +} + +#[test] +fn repo_url_resolves_scp_style_ssh() { + assert_eq!( + repository_to_issues_url("git@github.com:test/pkg.git"), + Some("https://github.com/test/pkg/issues".to_string()), + ); +} + +#[test] +fn repo_url_resolves_scp_style_ssh_with_dot_git_and_trailing_slash() { + assert_eq!( + repository_to_issues_url("git@github.com:test/pkg.git/"), + Some("https://github.com/test/pkg/issues".to_string()), + ); +} + +#[test] +fn repo_url_resolves_bitbucket_shorthand() { + assert_eq!( + repository_to_issues_url("bitbucket:test/pkg"), + Some("https://bitbucket.org/test/pkg/issues".to_string()), + ); +} + +#[test] +fn repo_url_returns_none_for_invalid_url() { + assert_eq!(repository_to_issues_url("not-a-url"), None); +} + +#[test] +fn is_http_url_accepts_https() { + assert!(is_http_url("https://example.com")); +} + +#[test] +fn is_http_url_accepts_http() { + assert!(is_http_url("http://example.com")); +} + +#[test] +fn is_http_url_rejects_ftp() { + assert!(!is_http_url("ftp://example.com")); +} + +#[test] +fn is_http_url_rejects_invalid() { + assert!(!is_http_url("not-a-url")); +} + +#[test] +fn hosted_shorthand_recognises_github_prefix() { + assert_eq!( + try_hosted_git_shorthand("github:owner/repo"), + Some("https://github.com/owner/repo/issues".to_string()), + ); +} + +#[test] +fn hosted_shorthand_recognises_gitlab_prefix() { + assert_eq!( + try_hosted_git_shorthand("gitlab:owner/repo"), + Some("https://gitlab.com/owner/repo/issues".to_string()), + ); +} + +#[test] +fn hosted_shorthand_recognises_bitbucket_prefix() { + assert_eq!( + try_hosted_git_shorthand("bitbucket:owner/repo"), + Some("https://bitbucket.org/owner/repo/issues".to_string()), + ); +} + +#[test] +fn hosted_shorthand_recognises_bare_owner_repo() { + assert_eq!( + try_hosted_git_shorthand("owner/repo"), + Some("https://github.com/owner/repo/issues".to_string()), + ); +} + +#[test] +fn hosted_shorthand_returns_none_for_urls() { + assert_eq!(try_hosted_git_shorthand("https://github.com/owner/repo"), None); +} + +#[test] +fn hosted_shorthand_returns_none_for_git_urls() { + assert_eq!(try_hosted_git_shorthand("git@github.com:owner/repo.git"), None); +} + +#[test] +fn parse_spec_bare_name() { + assert_eq!(parse_package_spec("foo"), ("foo", None)); +} + +#[test] +fn parse_spec_name_with_version() { + assert_eq!(parse_package_spec("foo@1.0.0"), ("foo", Some("1.0.0"))); +} + +#[test] +fn parse_spec_scoped_package() { + assert_eq!(parse_package_spec("@scope/foo"), ("@scope/foo", None)); +} + +#[test] +fn parse_spec_scoped_with_version() { + assert_eq!(parse_package_spec("@scope/foo@1.0.0"), ("@scope/foo", Some("1.0.0"))); +} + +#[test] +fn parse_spec_scoped_with_tag() { + assert_eq!(parse_package_spec("@scope/foo@latest"), ("@scope/foo", Some("latest"))); +} + +#[test] +fn parse_spec_trims_whitespace() { + assert_eq!(parse_package_spec(" foo "), ("foo", None)); +} + +#[test] +fn parse_spec_strips_version_from_name_with_version() { + let (name, tag) = parse_package_spec("react@18.2.0"); + assert_eq!(name, "react"); + assert_eq!(tag, Some("18.2.0")); +} + +#[test] +fn repo_url_resolves_github_dot_com_shorthand_without_scheme() { + assert_eq!( + repository_to_issues_url("github.com/owner/repo"), + Some("https://github.com/owner/repo/issues".to_string()), + ); +} + +#[test] +fn repo_url_retains_ssh_port() { + assert_eq!( + repository_to_issues_url("ssh://git@git.example.com:2222/owner/repo.git"), + Some("https://git.example.com:2222/owner/repo/issues".to_string()), + ); +} + +#[test] +fn repo_url_strips_shorthand_fragment_and_query() { + assert_eq!( + repository_to_issues_url("github:owner/repo#main"), + Some("https://github.com/owner/repo/issues".to_string()), + ); + assert_eq!( + repository_to_issues_url("owner/repo.git#main"), + Some("https://github.com/owner/repo/issues".to_string()), + ); +} + +#[test] +fn repo_url_strips_scp_fragment_and_query() { + assert_eq!( + repository_to_issues_url("git@github.com:owner/repo.git#main"), + Some("https://github.com/owner/repo/issues".to_string()), + ); +} diff --git a/pacquet/crates/cli/src/cli_args/cli_command.rs b/pacquet/crates/cli/src/cli_args/cli_command.rs index 5b801b2fea..e78d70be29 100644 --- a/pacquet/crates/cli/src/cli_args/cli_command.rs +++ b/pacquet/crates/cli/src/cli_args/cli_command.rs @@ -3,6 +3,7 @@ use super::{ approve_builds::ApproveBuildsArgs, audit::AuditArgs, bin::BinArgs, + bugs::BugsArgs, cache::CacheCommand, cat_file::CatFileArgs, cat_index::CatIndexArgs, @@ -139,6 +140,9 @@ pub enum CliCommand { Outdated(OutdatedArgs), /// Checks for known security issues with the installed packages. Audit(AuditArgs), + /// Opens the bug tracker URL of a package in the default browser. + #[clap(visible_alias = "issues")] + Bugs(BugsArgs), /// List installed packages. #[clap(visible_alias = "ls")] List(ListArgs), diff --git a/pacquet/crates/cli/src/cli_args/dispatch.rs b/pacquet/crates/cli/src/cli_args/dispatch.rs index 840d6b08b9..9786f4c774 100644 --- a/pacquet/crates/cli/src/cli_args/dispatch.rs +++ b/pacquet/crates/cli/src/cli_args/dispatch.rs @@ -228,6 +228,7 @@ fn route<'a>(command: CliCommand, ctx: &RunCtx<'a>) -> miette::Result dispatch_install::update(ctx, args), CliCommand::Outdated(args) => dispatch_query::outdated(ctx, args), CliCommand::Audit(args) => dispatch_query::audit(ctx, args), + CliCommand::Bugs(args) => dispatch_query::bugs(ctx, args), CliCommand::List(args) => dispatch_query::list(ctx, args), CliCommand::Ll(args) => dispatch_query::ll(ctx, args), CliCommand::Why(args) => dispatch_query::why(ctx, args), diff --git a/pacquet/crates/cli/src/cli_args/dispatch_query.rs b/pacquet/crates/cli/src/cli_args/dispatch_query.rs index 97fd1449ed..b5275f1c24 100644 --- a/pacquet/crates/cli/src/cli_args/dispatch_query.rs +++ b/pacquet/crates/cli/src/cli_args/dispatch_query.rs @@ -1,6 +1,7 @@ use super::{ audit::{AuditArgs, AuditOutcome}, bin::BinArgs, + bugs::BugsArgs, cache::CacheCommand, cat_file::CatFileArgs, cat_index::CatIndexArgs, @@ -333,6 +334,12 @@ pub(super) fn ignored_builds<'a>( Ok(Box::pin(std::future::ready(Ok(())))) } +pub(super) fn bugs<'a>(ctx: &RunCtx<'a>, args: BugsArgs) -> miette::Result> { + let cfg: &Config = (ctx.config)()?; + let dir = ctx.dir; + Ok(Box::pin(async move { args.run(cfg, dir).await })) +} + pub(super) fn find_hash<'a>( ctx: &RunCtx<'a>, args: FindHashArgs, diff --git a/pacquet/crates/cli/src/cli_args/dist_tag.rs b/pacquet/crates/cli/src/cli_args/dist_tag.rs index 9d27d8db63..a3bb2b131b 100644 --- a/pacquet/crates/cli/src/cli_args/dist_tag.rs +++ b/pacquet/crates/cli/src/cli_args/dist_tag.rs @@ -6,8 +6,8 @@ use miette::{Context, Diagnostic, IntoDiagnostic}; use node_semver::Version; use pacquet_config::Config; use pacquet_network::{ - NetworkSettings, RetryOpts, ThrottledClient, redact_url_credentials, retry_async, - send_with_retry, + NetworkSettings, RetryOpts, ThrottledClient, encode_uri_component, redact_url_credentials, + retry_async, send_with_retry, }; use pacquet_resolving_npm_resolver::pick_registry_for_package; use pacquet_resolving_parse_wanted_dependency::parse_wanted_dependency; @@ -15,7 +15,6 @@ use reqwest::{RequestBuilder, Response, StatusCode}; use serde::Deserialize; use std::{ collections::{BTreeMap, HashMap}, - fmt::Write as _, time::Duration, }; @@ -661,19 +660,6 @@ fn escaped_package_name(package_name: &str) -> String { } } -fn encode_uri_component(input: &str) -> String { - const UNRESERVED: &[u8] = b"-_.!~*'()"; - let mut output = String::with_capacity(input.len()); - for &byte in input.as_bytes() { - if byte.is_ascii_alphanumeric() || UNRESERVED.contains(&byte) { - output.push(byte as char); - } else { - write!(output, "%{byte:02X}").expect("writing to a String never fails"); - } - } - output -} - impl AuthType { fn header_value(self) -> &'static str { match self { diff --git a/pacquet/crates/cli/src/cli_args/self_update/verify_engine.rs b/pacquet/crates/cli/src/cli_args/self_update/verify_engine.rs index 684a1f5ab9..0a044baede 100644 --- a/pacquet/crates/cli/src/cli_args/self_update/verify_engine.rs +++ b/pacquet/crates/cli/src/cli_args/self_update/verify_engine.rs @@ -23,7 +23,8 @@ use pacquet_config::Config; use pacquet_graph_hasher::{host_arch, host_libc, host_platform}; use pacquet_lockfile::{EnvLockfile, PackageKey, SnapshotDepRef}; use pacquet_network::{ - NetworkSettings, RetryOpts, ThrottledClient, redact_url_credentials, send_with_retry, + NetworkSettings, RetryOpts, ThrottledClient, encode_package_name, redact_url_credentials, + send_with_retry, }; use serde::Deserialize; use std::time::Duration; @@ -466,13 +467,19 @@ async fn fetch_packument( { return Err(format!("{display_url} returned an oversized packument ({length} bytes)")); } - let body = response.text().await.map_err(|source| { - format!("{display_url}: {}", redact_url_credentials(&source.to_string())) - })?; - if body.len() as u64 > MAX_PACKUMENT_BYTES { - return Err(format!("{display_url} returned an oversized packument")); + use futures_util::StreamExt as _; + let mut stream = response.bytes_stream(); + let mut body_bytes = Vec::new(); + while let Some(chunk) = stream.next().await { + let chunk = chunk.map_err(|source| { + format!("{display_url}: {}", redact_url_credentials(&source.to_string())) + })?; + if (body_bytes.len() + chunk.len()) as u64 > MAX_PACKUMENT_BYTES { + return Err(format!("{display_url} returned an oversized packument")); + } + body_bytes.extend_from_slice(&chunk); } - serde_json::from_str::(&body) + serde_json::from_slice::(&body_bytes) .map(Some) .map_err(|err| format!("{display_url} returned invalid JSON: {err}")) } @@ -524,28 +531,5 @@ fn with_trailing_slash(registry: &str) -> String { if registry.ends_with('/') { registry.to_string() } else { format!("{registry}/") } } -/// Percent-encode a package name for a packument URL (scoped names keep -/// the leading `@`, the `/` becomes `%2F`). -fn encode_package_name(name: &str) -> String { - match name.strip_prefix('@') { - Some(rest) => format!("@{}", encode_uri_component(rest)), - None => encode_uri_component(name), - } -} - -fn encode_uri_component(input: &str) -> String { - use std::fmt::Write as _; - const UNRESERVED: &[u8] = b"-_.!~*'()"; - let mut output = String::with_capacity(input.len()); - for &byte in input.as_bytes() { - if byte.is_ascii_alphanumeric() || UNRESERVED.contains(&byte) { - output.push(byte as char); - } else { - write!(output, "%{byte:02X}").expect("writing to a String never fails"); - } - } - output -} - #[cfg(test)] mod tests; diff --git a/pacquet/crates/cli/tests/bugs.rs b/pacquet/crates/cli/tests/bugs.rs new file mode 100644 index 0000000000..787ed95487 --- /dev/null +++ b/pacquet/crates/cli/tests/bugs.rs @@ -0,0 +1,298 @@ +use assert_cmd::prelude::*; +use command_extra::CommandExtra; +use pacquet_testing_utils::bin::CommandTempCwd; +use std::{ + fs, + path::{Path, PathBuf}, + process::Command, +}; + +fn pacquet_at(workspace: &Path) -> Command { + Command::cargo_bin("pacquet").expect("find the pacquet binary").with_current_dir(workspace) +} + +fn empty_auth_file(root: &Path) -> PathBuf { + let auth_file = root.join("auth-npmrc"); + fs::write(&auth_file, "").expect("write empty auth .npmrc"); + auth_file +} + +fn run_bugs(workspace: &Path, auth_file: &Path, args: &[&str]) -> std::process::Output { + let mut command = + pacquet_at(workspace).with_arg("--npmrc-auth-file").with_arg(auth_file).with_arg("bugs"); + for arg in args { + command = command.with_arg(arg); + } + command.output().expect("spawn pacquet bugs") +} + +fn version_response(name: &str, extra_fields: &serde_json::Value) -> String { + let mut version = serde_json::json!({ + "name": name, + "version": "1.0.0", + "dist": { + "tarball": "https://example.com/pkg.tgz", + }, + }); + if let Some(obj) = version.as_object_mut() + && let Some(extra) = extra_fields.as_object() + { + for (k, v) in extra { + obj.insert(k.clone(), v.clone()); + } + } + version.to_string() +} + +#[test] +fn prints_bugs_url_from_local_manifest_bugs_object() { + let CommandTempCwd { root, workspace, .. } = CommandTempCwd::init(); + let auth_file = empty_auth_file(root.path()); + fs::write( + workspace.join("package.json"), + r#"{"name":"test-pkg","bugs":{"url":"https://github.com/test/pkg/issues"}}"#, + ) + .expect("write package.json"); + + let output = run_bugs(&workspace, &auth_file, &[]); + + assert!( + output.status.success(), + "bugs must succeed (stderr: {})", + String::from_utf8_lossy(&output.stderr), + ); + let stdout = String::from_utf8_lossy(&output.stdout); + assert_eq!(stdout.trim(), "https://github.com/test/pkg/issues"); + drop(root); +} + +#[test] +fn prints_bugs_url_from_local_manifest_bugs_string() { + let CommandTempCwd { root, workspace, .. } = CommandTempCwd::init(); + let auth_file = empty_auth_file(root.path()); + fs::write( + workspace.join("package.json"), + r#"{"name":"test-pkg","bugs":"https://github.com/test/pkg/issues"}"#, + ) + .expect("write package.json"); + + let output = run_bugs(&workspace, &auth_file, &[]); + + assert!( + output.status.success(), + "bugs must succeed (stderr: {})", + String::from_utf8_lossy(&output.stderr), + ); + let stdout = String::from_utf8_lossy(&output.stdout); + assert_eq!(stdout.trim(), "https://github.com/test/pkg/issues"); + drop(root); +} + +#[test] +fn prints_repository_issues_url_when_bugs_is_missing() { + let CommandTempCwd { root, workspace, .. } = CommandTempCwd::init(); + let auth_file = empty_auth_file(root.path()); + fs::write( + workspace.join("package.json"), + r#"{"name":"test-pkg","repository":"https://github.com/test/pkg"}"#, + ) + .expect("write package.json"); + + let output = run_bugs(&workspace, &auth_file, &[]); + + assert!( + output.status.success(), + "bugs must succeed (stderr: {})", + String::from_utf8_lossy(&output.stderr), + ); + let stdout = String::from_utf8_lossy(&output.stdout); + assert_eq!(stdout.trim(), "https://github.com/test/pkg/issues"); + drop(root); +} + +#[test] +fn normalizes_git_plus_https_repository_url_with_dot_git() { + let CommandTempCwd { root, workspace, .. } = CommandTempCwd::init(); + let auth_file = empty_auth_file(root.path()); + fs::write( + workspace.join("package.json"), + r#"{"name":"test-pkg","repository":{"url":"git+https://github.com/test/pkg.git"}}"#, + ) + .expect("write package.json"); + + let output = run_bugs(&workspace, &auth_file, &[]); + + assert!( + output.status.success(), + "bugs must succeed (stderr: {})", + String::from_utf8_lossy(&output.stderr), + ); + let stdout = String::from_utf8_lossy(&output.stdout); + assert_eq!(stdout.trim(), "https://github.com/test/pkg/issues"); + drop(root); +} + +#[test] +fn fails_when_no_bugs_url_can_be_derived() { + let CommandTempCwd { root, workspace, .. } = CommandTempCwd::init(); + let auth_file = empty_auth_file(root.path()); + fs::write(workspace.join("package.json"), r#"{"name":"test-pkg"}"#) + .expect("write package.json"); + + let output = run_bugs(&workspace, &auth_file, &[]); + + assert!( + !output.status.success(), + "bugs must fail when no URL can be derived (stderr: {})", + String::from_utf8_lossy(&output.stderr), + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("ERR_PNPM_NO_BUGS_URL"), + "stderr must contain ERR_PNPM_NO_BUGS_URL; got:\n{stderr}", + ); + drop(root); +} + +#[test] +fn fails_when_no_package_json_exists() { + let CommandTempCwd { root, workspace, .. } = CommandTempCwd::init(); + let auth_file = empty_auth_file(root.path()); + + let output = run_bugs(&workspace, &auth_file, &[]); + + assert!( + !output.status.success(), + "bugs must fail when no package.json exists (stderr: {})", + String::from_utf8_lossy(&output.stderr), + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("ERR_PNPM_NO_IMPORTER_MANIFEST_FOUND"), + "stderr must contain ERR_PNPM_NO_IMPORTER_MANIFEST_FOUND; got:\n{stderr}", + ); + drop(root); +} + +#[test] +fn looks_up_package_on_registry_by_name() { + let CommandTempCwd { root, workspace, .. } = CommandTempCwd::init(); + let mut server = mockito::Server::new(); + let registry = format!("{}/", server.url()); + + let body = version_response( + "is-negative", + &serde_json::json!({ + "bugs": { "url": "https://github.com/kevva/is-negative/issues" }, + }), + ); + let mock = server.mock("GET", "/is-negative/latest").with_status(200).with_body(&body).create(); + + fs::write(workspace.join(".npmrc"), format!("registry={registry}\n")) + .expect("write project .npmrc"); + let auth_file = empty_auth_file(root.path()); + + let output = run_bugs(&workspace, &auth_file, &["is-negative"]); + + mock.assert(); + assert!( + output.status.success(), + "bugs must succeed (stderr: {})", + String::from_utf8_lossy(&output.stderr), + ); + let stdout = String::from_utf8_lossy(&output.stdout); + assert_eq!(stdout.trim(), "https://github.com/kevva/is-negative/issues"); + drop((root, server)); +} + +#[test] +fn prints_repository_issues_url_from_registry_package() { + let CommandTempCwd { root, workspace, .. } = CommandTempCwd::init(); + let mut server = mockito::Server::new(); + let registry = format!("{}/", server.url()); + + let body = version_response( + "test-pkg", + &serde_json::json!({ + "repository": { "url": "git+https://github.com/test/pkg.git" }, + }), + ); + let mock = server.mock("GET", "/test-pkg/latest").with_status(200).with_body(&body).create(); + + fs::write(workspace.join(".npmrc"), format!("registry={registry}\n")) + .expect("write project .npmrc"); + let auth_file = empty_auth_file(root.path()); + + let output = run_bugs(&workspace, &auth_file, &["test-pkg"]); + + mock.assert(); + assert!( + output.status.success(), + "bugs must succeed (stderr: {})", + String::from_utf8_lossy(&output.stderr), + ); + let stdout = String::from_utf8_lossy(&output.stdout); + assert_eq!(stdout.trim(), "https://github.com/test/pkg/issues"); + drop((root, server)); +} + +#[test] +fn fails_when_registry_package_has_no_bugs_url() { + let CommandTempCwd { root, workspace, .. } = CommandTempCwd::init(); + let mut server = mockito::Server::new(); + let registry = format!("{}/", server.url()); + + let body = version_response("no-bugs-pkg", &serde_json::json!({})); + let mock = server.mock("GET", "/no-bugs-pkg/latest").with_status(200).with_body(&body).create(); + + fs::write(workspace.join(".npmrc"), format!("registry={registry}\n")) + .expect("write project .npmrc"); + let auth_file = empty_auth_file(root.path()); + + let output = run_bugs(&workspace, &auth_file, &["no-bugs-pkg"]); + + mock.assert(); + assert!( + !output.status.success(), + "bugs must fail when package has no bugs URL (stderr: {})", + String::from_utf8_lossy(&output.stderr), + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("ERR_PNPM_NO_BUGS_URL"), + "stderr must contain ERR_PNPM_NO_BUGS_URL; got:\n{stderr}", + ); + drop((root, server)); +} + +#[test] +fn encodes_scoped_package_name_in_registry_request() { + let CommandTempCwd { root, workspace, .. } = CommandTempCwd::init(); + let mut server = mockito::Server::new(); + let registry = format!("{}/", server.url()); + + let body = version_response( + "@scope/pkg", + &serde_json::json!({ + "bugs": { "url": "https://github.com/scope/pkg/issues" }, + }), + ); + let mock = + server.mock("GET", "/@scope%2Fpkg/latest").with_status(200).with_body(&body).create(); + + fs::write(workspace.join(".npmrc"), format!("registry={registry}\n")) + .expect("write project .npmrc"); + let auth_file = empty_auth_file(root.path()); + + let output = run_bugs(&workspace, &auth_file, &["@scope/pkg"]); + + mock.assert(); + assert!( + output.status.success(), + "bugs must succeed for a scoped package (stderr: {})", + String::from_utf8_lossy(&output.stderr), + ); + let stdout = String::from_utf8_lossy(&output.stdout); + assert_eq!(stdout.trim(), "https://github.com/scope/pkg/issues"); + drop((root, server)); +} diff --git a/pacquet/crates/network/src/lib.rs b/pacquet/crates/network/src/lib.rs index ff28d57538..1063da9e48 100644 --- a/pacquet/crates/network/src/lib.rs +++ b/pacquet/crates/network/src/lib.rs @@ -10,6 +10,9 @@ pub use auth::{ AuthHeaders, AuthHeadersByScope, DEFAULT_REGISTRY_SCOPE, MetadataCacheScope, UpstreamRouteHook, base64_encode, nerf_dart, redact_and_sanitize, redact_url_credentials, }; +pub use url_encoding::{encode_package_name, encode_uri_component}; + +mod url_encoding; pub use proxy::{NoProxySetting, ProxyConfig, ProxyError}; pub use retry::{RetryOpts, retry_async, send_with_retry, should_retry_status}; pub use tls::{PerRegistryTls, RegistryTls, TlsConfig, TlsError}; diff --git a/pacquet/crates/network/src/url_encoding.rs b/pacquet/crates/network/src/url_encoding.rs new file mode 100644 index 0000000000..f0d2214742 --- /dev/null +++ b/pacquet/crates/network/src/url_encoding.rs @@ -0,0 +1,28 @@ +use std::fmt::Write as _; + +/// Percent-encode a package name for a packument URL, matching pnpm's +/// `toUri`: a scoped name keeps its leading `@` and encodes the rest (so the +/// `/` becomes `%2F`), an unscoped name is encoded whole. +#[must_use] +pub fn encode_package_name(name: &str) -> String { + match name.strip_prefix('@') { + Some(rest) => format!("@{}", encode_uri_component(rest)), + None => encode_uri_component(name), + } +} + +/// Port of JavaScript `encodeURIComponent`: every UTF-8 byte outside the +/// unreserved set is percent-encoded. +#[must_use] +pub fn encode_uri_component(input: &str) -> String { + const UNRESERVED: &[u8] = b"-_.!~*'()"; + let mut output = String::with_capacity(input.len()); + for &byte in input.as_bytes() { + if byte.is_ascii_alphanumeric() || UNRESERVED.contains(&byte) { + output.push(byte as char); + } else { + write!(output, "%{byte:02X}").expect("writing to a String never fails"); + } + } + output +} diff --git a/pacquet/crates/package-manager/src/add/tests.rs b/pacquet/crates/package-manager/src/add/tests.rs index dae234928e..d8b383a4c1 100644 --- a/pacquet/crates/package-manager/src/add/tests.rs +++ b/pacquet/crates/package-manager/src/add/tests.rs @@ -23,7 +23,7 @@ async fn add_routes_scoped_packages_to_configured_scoped_registry() { let mut default_registry = mockito::Server::new_async().await; let default_latest = default_registry - .mock("GET", "/@private/foo/latest") + .mock("GET", "/@private%2Ffoo/latest") .with_status(500) .expect(0) .create_async() @@ -38,7 +38,7 @@ async fn add_routes_scoped_packages_to_configured_scoped_registry() { let mut scoped_registry = mockito::Server::new_async().await; let scoped_registry_url = format!("{}/", scoped_registry.url()); let scoped_latest = scoped_registry - .mock("GET", "/@private/foo/latest") + .mock("GET", "/@private%2Ffoo/latest") .with_status(200) .with_header("content-type", "application/json") .with_body(scoped_version_body(&scoped_registry_url)) diff --git a/pacquet/crates/registry/src/package.rs b/pacquet/crates/registry/src/package.rs index a6d0bcf507..c2a7f80c10 100644 --- a/pacquet/crates/registry/src/package.rs +++ b/pacquet/crates/registry/src/package.rs @@ -101,13 +101,14 @@ impl Package { registry: &str, auth_headers: &AuthHeaders, ) -> Result { - // Format once. The same string is consumed by the GET, the - // per-URL `Authorization` lookup, and the error mapper — using - // distinct closures risked the auth lookup and request URL - // drifting if the format expression ever changed. - let url = format!("{registry}{name}"); // TODO: use reqwest URL directly + let encoded_name = pacquet_network::encode_package_name(name); + let url = format!("{registry}{encoded_name}"); // TODO: use reqwest URL directly let network_error = |error| NetworkError { error, url: url.clone() }; - let mut request = http_client.acquire_for_url(&url).await.get(&url).header( + // Hold the semaphore permit across send + body consumption so the + // socket-bound stays effective under concurrent fan-out. See the + // doc comment on `ThrottledClientGuard`. + let guard = http_client.acquire_for_url(&url).await; + let mut request = guard.get(&url).header( "accept", "application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*", ); diff --git a/pacquet/crates/registry/src/package_tag.rs b/pacquet/crates/registry/src/package_tag.rs index b129106260..67f08b0333 100644 --- a/pacquet/crates/registry/src/package_tag.rs +++ b/pacquet/crates/registry/src/package_tag.rs @@ -1,15 +1,35 @@ -use derive_more::{Display, From, TryInto}; -use node_semver::{SemverError, Version}; use std::str::FromStr; +use derive_more::{Display, From, TryInto}; +use node_semver::{Range, SemverError, Version}; + /// Version or tag that is attachable to a registry URL. -#[derive(Debug, Display, From, TryInto)] +#[derive(Debug, Display, Clone, From, TryInto)] pub enum PackageTag { /// Literally `latest`. #[display("latest")] Latest, /// Pinned version. + #[display("{}", _0)] Version(Version), + /// A custom tag (e.g. `beta`, `next`). + #[display("{}", _0)] + Tag(String), +} + +impl PackageTag { + /// URL-encoded path segment for use in a registry request URL. + /// The version or custom tag is percent-encoded so the path + /// component stays syntactically valid regardless of the characters + /// it contains (e.g. `+` build metadata in a version). + #[must_use] + pub fn registry_path_segment(&self) -> String { + match self { + PackageTag::Latest => "latest".to_string(), + PackageTag::Version(v) => pacquet_network::encode_uri_component(&v.to_string()), + PackageTag::Tag(tag) => pacquet_network::encode_uri_component(tag), + } + } } impl FromStr for PackageTag { @@ -17,8 +37,20 @@ impl FromStr for PackageTag { fn from_str(value: &str) -> Result { if value == "latest" { Ok(PackageTag::Latest) - } else { + } else if let Ok(version) = value.parse::() { + Ok(PackageTag::Version(version)) + } else if Range::parse(value).is_ok() { + // A semver range (e.g. `^18`) is neither an exact version nor a + // dist-tag, so it is not a valid path segment for the abbreviated + // registry endpoint. Report it as an error (the range never + // parses as a `Version`) rather than treating it as a literal tag + // that would issue a doomed `/pkg/^18` request. value.parse::().map(PackageTag::Version) + } else { + Ok(PackageTag::Tag(value.to_owned())) } } } + +#[cfg(test)] +mod tests; diff --git a/pacquet/crates/registry/src/package_tag/tests.rs b/pacquet/crates/registry/src/package_tag/tests.rs new file mode 100644 index 0000000000..30c3e42449 --- /dev/null +++ b/pacquet/crates/registry/src/package_tag/tests.rs @@ -0,0 +1,13 @@ +use super::PackageTag; + +#[test] +fn registry_path_segment_encodes_each_variant() { + assert_eq!(PackageTag::Latest.registry_path_segment(), "latest"); + assert_eq!("1.2.3".parse::().unwrap().registry_path_segment(), "1.2.3"); + // `+` build metadata is not path-safe and must be percent-encoded. + assert_eq!( + "1.2.3+build.1".parse::().unwrap().registry_path_segment(), + "1.2.3%2Bbuild.1", + ); + assert_eq!(PackageTag::Tag("beta/next".to_owned()).registry_path_segment(), "beta%2Fnext"); +} diff --git a/pacquet/crates/registry/src/package_version.rs b/pacquet/crates/registry/src/package_version.rs index 788e948c6e..8137bebb0d 100644 --- a/pacquet/crates/registry/src/package_version.rs +++ b/pacquet/crates/registry/src/package_version.rs @@ -250,15 +250,18 @@ impl PackageVersion { // Format once and reuse for the request, the auth-header // lookup, and the error mapper. Keeps the auth lookup and // request URL byte-identical and saves two formats. - let url = format!("{registry}{name}/{tag}"); + let encoded_name = pacquet_network::encode_package_name(name); + let url = format!("{registry}{encoded_name}/{}", tag.registry_path_segment()); let network_error = |error| NetworkError { error, url: url.clone() }; - let mut request = http_client.acquire_for_url(&url).await.get(&url).header( + // Hold the semaphore permit across send + body consumption so the + // socket-bound stays effective under concurrent fan-out. See the + // doc comment on `ThrottledClientGuard`. + let guard = http_client.acquire_for_url(&url).await; + let mut request = guard.get(&url).header( "accept", "application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*", ); - // Same auth flow as `Package::fetch_from_registry`. See the - // doc comment there. if let Some(value) = auth_headers.for_url_with_package(&url, Some(name)) { request = request.header("authorization", value); } diff --git a/pacquet/crates/resolving-git-resolver/src/hosted_git.rs b/pacquet/crates/resolving-git-resolver/src/hosted_git.rs index 09aac56f84..fbaf634cc6 100644 --- a/pacquet/crates/resolving-git-resolver/src/hosted_git.rs +++ b/pacquet/crates/resolving-git-resolver/src/hosted_git.rs @@ -16,7 +16,8 @@ //! implemented — only `https` / `ssh` / `sshurl` / `tarball` / //! `shortcut` are used by the resolver. -use std::fmt::{self, Write}; +use pacquet_network::encode_uri_component; +use std::fmt; /// Three host families pacquet recognises. Mirrors upstream's /// `gitHosts` keys at @@ -650,22 +651,5 @@ fn percent_decode(input: &str) -> String { String::from_utf8(buf).unwrap_or_else(|_| input.to_string()) } -/// Match Node's `encodeURIComponent`. Percent-encode every byte -/// outside the safe ASCII set Node keeps unencoded: -/// `A-Z a-z 0-9 - _ . ! ~ * ' ( )`. -fn encode_uri_component(input: &str) -> String { - let mut out = String::with_capacity(input.len()); - for byte in input.bytes() { - let safe = byte.is_ascii_alphanumeric() - || matches!(byte, b'-' | b'_' | b'.' | b'!' | b'~' | b'*' | b'\'' | b'(' | b')'); - if safe { - out.push(byte as char); - } else { - write!(&mut out, "%{byte:02X}").expect("write to String never fails"); - } - } - out -} - #[cfg(test)] mod tests; diff --git a/pnpr/crates/pnpr/src/server.rs b/pnpr/crates/pnpr/src/server.rs index 59e1f1c7f9..507399b6c6 100644 --- a/pnpr/crates/pnpr/src/server.rs +++ b/pnpr/crates/pnpr/src/server.rs @@ -543,8 +543,12 @@ async fn get_two_segments( ); } if first.starts_with('@') { - let full = format!("{first}/{second}"); - serve_packument(&state, &identity, &headers, &full).await + if first.contains('/') { + serve_version_manifest(&state, &identity, &first, &second).await + } else { + let full = format!("{first}/{second}"); + serve_packument(&state, &identity, &headers, &full).await + } } else { serve_version_manifest(&state, &identity, &first, &second).await }