Auto merge of #6469 - ehuss:registry-name, r=dwijnand

Restrict registry names to same style as package names.

See #4688 (comment)

src/cargo/core/package_id_spec.rs

@@ -7,7 +7,7 @@ use url::Url;
 
 use crate::core::PackageId;
 use crate::util::errors::{CargoResult, CargoResultExt};
-use crate::util::{ToSemver, ToUrl};
+use crate::util::{validate_package_name, ToSemver, ToUrl};
 
 /// Some or all of the data required to identify a package:
 ///
@@ -64,11 +64,7 @@ impl PackageIdSpec {
             Some(version) => Some(Version::parse(version)?),
             None => None,
         };
-        for ch in name.chars() {
-            if !ch.is_alphanumeric() && ch != '_' && ch != '-' {
-                failure::bail!("invalid character in pkgid `{}`: `{}`", spec, ch)
-            }
-        }
+        validate_package_name(name, "pkgid", "")?;
         Ok(PackageIdSpec {
             name: name.to_string(),
             version,

src/cargo/ops/cargo_new.rs

@@ -10,7 +10,7 @@ use git2::Repository as GitRepository;
 use crate::core::{compiler, Workspace};
 use crate::util::errors::{CargoResult, CargoResultExt};
 use crate::util::{existing_vcs_repo, internal, FossilRepo, GitRepo, HgRepo, PijulRepo};
-use crate::util::{paths, Config};
+use crate::util::{paths, validate_package_name, Config};
 
 use toml;
 
@@ -161,20 +161,7 @@ fn check_name(name: &str, opts: &NewOptions) -> CargoResult<()> {
         }
     }
 
-    for c in name.chars() {
-        if c.is_alphanumeric() {
-            continue;
-        }
-        if c == '_' || c == '-' {
-            continue;
-        }
-        failure::bail!(
-            "Invalid character `{}` in crate name: `{}`{}",
-            c,
-            name,
-            name_help
-        )
-    }
+    validate_package_name(name, "crate name", name_help)?;
     Ok(())
 }
 

src/cargo/ops/registry.rs

@@ -19,8 +19,8 @@ use crate::sources::{RegistrySource, SourceConfigMap};
 use crate::util::config::{self, Config};
 use crate::util::errors::{CargoResult, CargoResultExt};
 use crate::util::important_paths::find_root_manifest_for_wd;
-use crate::util::paths;
 use crate::util::ToUrl;
+use crate::util::{paths, validate_package_name};
 use crate::version;
 
 pub struct RegistryConfig {
@@ -294,12 +294,15 @@ pub fn registry_configuration(
     registry: Option<String>,
 ) -> CargoResult<RegistryConfig> {
     let (index, token) = match registry {
-        Some(registry) => (
-            Some(config.get_registry_index(&registry)?.to_string()),
-            config
-                .get_string(&format!("registries.{}.token", registry))?
-                .map(|p| p.val),
-        ),
+        Some(registry) => {
+            validate_package_name(&registry, "registry name", "")?;
+            (
+                Some(config.get_registry_index(&registry)?.to_string()),
+                config
+                    .get_string(&format!("registries.{}.token", registry))?
+                    .map(|p| p.val),
+            )
+        }
         None => {
             // Checking out for default index and token
             (

src/cargo/util/command_prelude.rs

@@ -6,7 +6,7 @@ use crate::core::Workspace;
 use crate::ops::{CompileFilter, CompileOptions, NewOptions, Packages, VersionControl};
 use crate::sources::CRATES_IO_REGISTRY;
 use crate::util::important_paths::find_root_manifest_for_wd;
-use crate::util::paths;
+use crate::util::{paths, validate_package_name};
 use crate::CargoResult;
 use clap::{self, SubCommand};
 
@@ -370,11 +370,12 @@ pub trait ArgMatchesExt {
                          requires -Zunstable-options to use."
                     ));
                 }
+                validate_package_name(registry, "registry name", "")?;
 
                 if registry == CRATES_IO_REGISTRY {
                     // If "crates.io" is specified then we just need to return None
                     // as that will cause cargo to use crates.io. This is required
-                    // for the case where a default alterative registry is used
+                    // for the case where a default alternative registry is used
                     // but the user wants to switch back to crates.io for a single
                     // command.
                     Ok(None)

src/cargo/util/config.rs

@@ -24,11 +24,11 @@ use crate::core::shell::Verbosity;
 use crate::core::{CliUnstable, Shell, SourceId, Workspace};
 use crate::ops;
 use crate::util::errors::{internal, CargoResult, CargoResultExt};
-use crate::util::paths;
 use crate::util::toml as cargo_toml;
 use crate::util::Filesystem;
 use crate::util::Rustc;
 use crate::util::ToUrl;
+use crate::util::{paths, validate_package_name};
 use url::Url;
 
 use self::ConfigValue as CV;
@@ -656,6 +656,7 @@ impl Config {
 
     /// Gets the index for a registry.
     pub fn get_registry_index(&self, registry: &str) -> CargoResult<Url> {
+        validate_package_name(registry, "registry name", "")?;
         Ok(
             match self.get_string(&format!("registries.{}.index", registry))? {
                 Some(index) => {

src/cargo/util/mod.rs

@@ -59,3 +59,19 @@ pub fn elapsed(duration: Duration) -> String {
         format!("{}.{:02}s", secs, duration.subsec_nanos() / 10_000_000)
     }
 }
+
+/// Check the base requirements for a package name.
+///
+/// This can be used for other things than package names, to enforce some
+/// level of sanity. Note that package names have other restrictions
+/// elsewhere. `cargo new` has a few restrictions, such as checking for
+/// reserved names. crates.io has even more restrictions.
+pub fn validate_package_name(name: &str, what: &str, help: &str) -> CargoResult<()> {
+    if let Some(ch) = name
+        .chars()
+        .find(|ch| !ch.is_alphanumeric() && *ch != '_' && *ch != '-')
+    {
+        failure::bail!("Invalid character `{}` in {}: `{}`{}", ch, what, name, help);
+    }
+    Ok(())
+}

src/cargo/util/toml/mod.rs

@@ -21,7 +21,7 @@ use crate::core::{GitReference, PackageIdSpec, SourceId, WorkspaceConfig, Worksp
 use crate::sources::{CRATES_IO_INDEX, CRATES_IO_REGISTRY};
 use crate::util::errors::{CargoResult, CargoResultExt, ManifestError};
 use crate::util::paths;
-use crate::util::{self, Config, ToUrl};
+use crate::util::{self, validate_package_name, Config, ToUrl};
 
 mod targets;
 use self::targets::targets;
@@ -809,19 +809,7 @@ impl TomlManifest {
             failure::bail!("package name cannot be an empty string")
         }
 
-        for c in package_name.chars() {
-            if c.is_alphanumeric() {
-                continue;
-            }
-            if c == '_' || c == '-' {
-                continue;
-            }
-            failure::bail!(
-                "Invalid character `{}` in package name: `{}`",
-                c,
-                package_name
-            )
-        }
+        validate_package_name(package_name, "package name", "")?;
 
         let pkgid = project.to_package_id(source_id)?;
 

tests/testsuite/alt_registry.rs

@@ -558,3 +558,50 @@ fn patch_alt_reg() {
         )
         .run();
 }
+
+#[test]
+fn bad_registry_name() {
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+            cargo-features = ["alternative-registries"]
+
+            [project]
+            name = "foo"
+            version = "0.0.1"
+            authors = []
+
+            [dependencies.bar]
+            version = "0.0.1"
+            registry = "bad name"
+        "#,
+        )
+        .file("src/main.rs", "fn main() {}")
+        .build();
+
+    p.cargo("build")
+        .masquerade_as_nightly_cargo()
+        .with_status(101)
+        .with_stderr(
+            "\
+[ERROR] failed to parse manifest at `[CWD]/Cargo.toml`
+
+Caused by:
+  Invalid character ` ` in registry name: `bad name`",
+        )
+        .run();
+
+    for cmd in &[
+        "init", "install", "login", "owner", "publish", "search", "yank",
+    ] {
+        p.cargo(cmd)
+            .arg("-Zunstable-options")
+            .arg("--registry")
+            .arg("bad name")
+            .masquerade_as_nightly_cargo()
+            .with_status(101)
+            .with_stderr("[ERROR] Invalid character ` ` in registry name: `bad name`")
+            .run();
+    }
+}