fix(toolchain)!: restrict named toolchain characters

Cargo.toml

@@ -101,6 +101,7 @@ tracing = "0.1"
 tracing-log = "0.2"
 tracing-opentelemetry = { version = "0.33", optional = true }
 tracing-subscriber = { version = "0.3.23", features = ["env-filter"] }
+unicode-security = "0.1"
 url = "2.4"
 wait-timeout = "0.2"
 xz2 = "0.1.3"

doc/user-guide/src/concepts/toolchains.md

@@ -57,6 +57,14 @@ local builds of the Rust toolchain. To teach `rustup` about your build, run:
 $ rustup toolchain link my-toolchain path/to/my/toolchain/sysroot
 ```
 
+Custom toolchain names may contain any character that [UTS #39] allows in an
+identifier, except for `:` and `'`. In practice that means letters and digits
+in any script, along with `.`, `_`, and `-`; whitespace, most punctuation,
+emoji, and invisible or direction-altering characters are rejected. The names
+`.` and `..` are not accepted either.
+
+[UTS #39]: https://www.unicode.org/reports/tr39/#Identifier_Status_and_Type
+
 For example, on Ubuntu you might clone `rust-lang/rust` into `~/rust`, build
 it, and then run:
 

src/toolchain/names.rs

@@ -49,6 +49,7 @@ use std::{
 };
 
 use thiserror::Error;
+use unicode_security::GeneralSecurityProfile;
 
 use crate::dist::{PartialToolchainDesc, TargetTuple, ToolchainDesc};
 
@@ -71,19 +72,6 @@ pub enum InvalidName {
     PlusPrefix(String),
 }
 
-/// Common validate rules for all sorts of toolchain names
-fn validate(candidate: &str) -> Result<&str, InvalidName> {
-    if let Some(without_plus) = candidate.strip_prefix('+') {
-        return Err(InvalidName::PlusPrefix(without_plus.to_string()));
-    }
-    let normalized_name = candidate.trim_end_matches('/');
-    if normalized_name.is_empty() {
-        Err(InvalidName::ToolchainName(candidate.into()))
-    } else {
-        Ok(normalized_name)
-    }
-}
-
 /// A toolchain name from user input.
 #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)]
 pub(crate) enum ResolvableToolchainName {
@@ -103,7 +91,7 @@ impl ResolvableToolchainName {
     // If candidate could be resolved, return a ready to resolve version of it.
     // Otherwise error.
     fn validate(candidate: &str) -> Result<Self, InvalidName> {
-        let candidate = validate(candidate)?;
+        let candidate = validate_named_toolchain(candidate)?;
         if let Ok(desc) = PartialToolchainDesc::from_str(candidate) {
             return Ok(Self::Official(desc));
         }
@@ -185,7 +173,7 @@ pub(crate) enum MaybeOfficialToolchainName {
 
 impl MaybeOfficialToolchainName {
     fn validate(candidate: &str) -> Result<Self, InvalidName> {
-        Ok(match validate(candidate)? {
+        Ok(match validate_named_toolchain(candidate)? {
             "none" => Self::None,
             candidate => Self::Some(
                 PartialToolchainDesc::from_str(candidate)
@@ -224,7 +212,7 @@ pub enum ToolchainName {
 impl ToolchainName {
     /// If the string is already resolved, allow direct conversion
     fn validate(candidate: &str) -> Result<Self, InvalidName> {
-        let candidate = validate(candidate)?;
+        let candidate = validate_named_toolchain(candidate)?;
         if let Ok(desc) = ToolchainDesc::from_str(candidate) {
             return Ok(Self::Official(desc));
         }
@@ -290,9 +278,13 @@ impl ResolvableLocalToolchainName {
             return Ok(Self::Named(name));
         }
 
-        Ok(Self::Path(PathBasedToolchainName::try_from(
-            &PathBuf::from(candidate) as &Path,
-        )?))
+        if candidate.contains('/') || candidate.contains('\\') {
+            let path = PathBuf::from(candidate);
+            let path = PathBasedToolchainName::try_from(&path as &Path)?;
+            return Ok(Self::Path(path));
+        }
+
+        Err(InvalidName::ToolchainName(candidate.into()))
     }
 }
 
@@ -373,12 +365,9 @@ pub struct CustomToolchainName(String);
 
 impl CustomToolchainName {
     fn validate(candidate: &str) -> Result<Self, InvalidName> {
-        let candidate = validate(candidate)?;
-        if candidate.parse::<PartialToolchainDesc>().is_ok()
-            || candidate == "none"
-            || candidate.contains('/')
-            || candidate.contains('\\')
-        {
+        let candidate = validate_named_toolchain(candidate)
+            .map_err(|_| InvalidName::CustomName(candidate.into()))?;
+        if candidate.parse::<PartialToolchainDesc>().is_ok() || candidate == "none" {
             Err(InvalidName::CustomName(candidate.into()))
         } else {
             Ok(Self(candidate.into()))
@@ -463,6 +452,48 @@ impl Deref for PathBasedToolchainName {
     }
 }
 
+/// Common validate rules for toolchain names that aren't paths.
+///
+/// Beyond the shared [`validate`] rules, every character must be allowed in an
+/// identifier by the Unicode general security profile (UTS #39). That admits
+/// ASCII letters, digits, `.`, `_` and `-` -- so every official toolchain name
+/// still parses -- along with unicode identifiers such as `合法的`, while
+/// rejecting whitespace, most punctuation, emoji, and invisible or
+/// direction-altering characters.
+fn validate_named_toolchain(candidate: &str) -> Result<&str, InvalidName> {
+    let candidate = validate(candidate)?;
+    // `.` and `..` pass the profile but would escape or alias the toolchains
+    // directory, so they need rejecting on their own.
+    if !matches!(candidate, "." | "..")
+        && candidate
+            .chars()
+            .all(|c| c.identifier_allowed() && !EXCLUDED.contains(&c))
+    {
+        Ok(candidate)
+    } else {
+        Err(InvalidName::ToolchainName(candidate.to_owned()))
+    }
+}
+
+/// Common validate rules for all sorts of toolchain names
+fn validate(candidate: &str) -> Result<&str, InvalidName> {
+    if let Some(without_plus) = candidate.strip_prefix('+') {
+        return Err(InvalidName::PlusPrefix(without_plus.to_string()));
+    }
+    let normalized_name = candidate.trim_end_matches('/');
+    if normalized_name.is_empty() {
+        Err(InvalidName::ToolchainName(candidate.into()))
+    } else {
+        Ok(normalized_name)
+    }
+}
+
+/// Characters that UTS #39 permits in identifiers but that we reject anyway,
+/// because a named toolchain also has to work as a directory name under
+/// `.rustup/toolchains`: `:` would open an NTFS alternate data stream, and `'`
+/// needs quoting in enough shells to be a nuisance.
+const EXCLUDED: &[char] = &[':', '\''];
+
 #[cfg(test)]
 mod tests {
     use std::str::FromStr;
@@ -480,13 +511,21 @@ mod tests {
     fn partial_toolchain_desc_regex() -> String {
         let tuple_regex = format!(
             r"(-({}))?(?:-({}))?(?:-({}))?",
-            LIST_ARCHS.join("|"),
-            LIST_OSES.join("|"),
-            LIST_ENVS.join("|")
+            regex_alternates(LIST_ARCHS),
+            regex_alternates(LIST_OSES),
+            regex_alternates(LIST_ENVS)
         );
         r"(nightly|beta|stable|[0-9]{1}(\.(0|[1-9][0-9]{0,2}))(\.(0|[1-9][0-9]{0,1}))?(-beta(\.(0|[1-9][1-9]{0,1}))?)?)(-([0-9]{4}-[0-9]{2}-[0-9]{2}))?".to_owned() + &tuple_regex
     }
 
+    fn regex_alternates(values: &[&str]) -> String {
+        values
+            .iter()
+            .map(|value| regex::escape(value))
+            .collect::<Vec<_>>()
+            .join("|")
+    }
+
     prop_compose! {
         fn arb_partial_toolchain_desc()
             (s in string_regex(&partial_toolchain_desc_regex()).unwrap()) -> String {
@@ -496,9 +535,8 @@ mod tests {
 
     prop_compose! {
         fn arb_custom_name()
-            (s in r"[^\\/+][^\\/]*") -> String {
+            (s in r"[A-Za-z0-9._-]+") -> String {
                 // perhaps need to filter 'none' and partial toolchains - but they won't typically be generated anyway.
-                // Also filter '+' prefix as that's reserved for +toolchain syntax.
                 s
         }
     }
@@ -531,11 +569,18 @@ mod tests {
 
         #[test]
         fn test_parse_custom(name in arb_custom_name()) {
+            prop_assume!(name != "none");
+            prop_assume!(name != ".");
+            prop_assume!(name != "..");
+            prop_assume!(PartialToolchainDesc::from_str(&name).is_err());
             CustomToolchainName::from_str(&name).unwrap();
         }
 
         #[test]
         fn test_parse_resolvable_name(name in arb_resolvable_name()) {
+            prop_assume!(name != "none");
+            prop_assume!(name != ".");
+            prop_assume!(name != "..");
             ResolvableToolchainName::from_str(&name).unwrap();
         }
 
@@ -567,10 +612,10 @@ mod tests {
             "1.8.0-x86_64-apple-darwin",
             "1.8.0-x86_64-unknown-linux-gnu",
             "1.10.0-x86_64-unknown-linux-gnu",
-            "bar(baz)",
-            "foo#bar",
-            "the cake is a lie",
-            "this.is.not-a+semver",
+            "bar.baz",
+            "foo_bar",
+            "stage1-local",
+            "this.is.not-a_semver",
         ]
         .into_iter()
         .map(|s| ToolchainName::from_str(s).unwrap())
@@ -591,11 +636,11 @@ mod tests {
             "1.8.0-beta-x86_64-apple-darwin",
             "1.8.0-beta.2-x86_64-apple-darwin",
             // https://github.com/rust-lang/rustup/issues/3517
-            "foo#bar",
-            "bar(baz)",
-            "this.is.not-a+semver",
+            "foo_bar",
+            "bar.baz",
+            "this.is.not-a_semver",
             // https://github.com/rust-lang/rustup/issues/3168
-            "the cake is a lie",
+            "stage1-local",
         ]
         .into_iter()
         .map(|s| ToolchainName::from_str(s).unwrap())
@@ -605,4 +650,51 @@ mod tests {
 
         assert_eq!(expected, v);
     }
+
+    #[test]
+    fn custom_names_reject_special_characters() {
+        for name in [
+            "bar(baz)",
+            "foo#bar",
+            "the cake is a lie",
+            "this.is.not-a+semver",
+            ".",
+            "..",
+            // permitted by UTS #39, excluded by us
+            "quote'toolchain",
+            "stream:name",
+            // rejected by UTS #39
+            "µ",                 // MICRO SIGN, restricted in favour of U+03BC
+            "🦀",                // emoji
+            "tilde~home",        // not identifier punctuation
+            "abc\u{202E}def",    // RIGHT-TO-LEFT OVERRIDE
+            "zero\u{200D}width", // ZERO WIDTH JOINER
+            "no\u{00A0}break",   // NO-BREAK SPACE
+        ] {
+            CustomToolchainName::from_str(name).unwrap_err();
+            ResolvableToolchainName::from_str(name).unwrap_err();
+            ToolchainName::from_str(name).unwrap_err();
+        }
+    }
+
+    #[test]
+    fn custom_names_accept_unicode_identifiers() {
+        for name in ["合法的", "μ", "café", "Ελληνικά", "тулчейн"] {
+            CustomToolchainName::from_str(name).unwrap();
+            ResolvableToolchainName::from_str(name).unwrap();
+            ToolchainName::from_str(name).unwrap();
+        }
+    }
+
+    #[test]
+    fn official_names_survive_the_identifier_profile() {
+        for name in [
+            "stable-x86_64-unknown-linux-gnu",
+            "nightly-2015-01-01-aarch64-apple-darwin",
+            "1.86.0-x86_64-pc-windows-msvc",
+            "beta-i686-pc-windows-gnullvm",
+        ] {
+            ToolchainName::from_str(name).unwrap();
+        }
+    }
 }

tests/suite/cli_misc.rs

@@ -98,6 +98,24 @@ error:[..] invalid custom toolchain name 'beta'
 ...
 error:[..] invalid custom toolchain name 'stable'
 ...
+"#]])
+        .is_err();
+    cx.config
+        .expect(["rustup", "toolchain", "link", "bad name", "foo"])
+        .await
+        .with_stderr(snapbox::str![[r#"
+...
+error:[..] invalid custom toolchain name 'bad name'
+...
+"#]])
+        .is_err();
+    cx.config
+        .expect(["rustup", "toolchain", "link", "foo#bar", "foo"])
+        .await
+        .with_stderr(snapbox::str![[r#"
+...
+error:[..] invalid custom toolchain name 'foo#bar'
+...
 "#]])
         .is_err();
 }