fix(toolchain): restrict named toolchain characters

Author: cachebag

src/toolchain/names.rs

@@ -117,19 +117,6 @@ macro_rules! try_from_str {
     };
 }
 
-/// 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 {
@@ -152,7 +139,7 @@ impl ResolvableToolchainName {
     // If candidate could be resolved, return a ready to resolve version of it.
     // Otherwise error.
     fn validate(candidate: &str) -> Result<ResolvableToolchainName, InvalidName> {
-        let candidate = validate(candidate)?;
+        let candidate = validate_named_toolchain(candidate)?;
         candidate
             .parse::<PartialToolchainDesc>()
             .map(ResolvableToolchainName::Official)
@@ -226,7 +213,7 @@ pub(crate) enum MaybeOfficialToolchainName {
 
 impl MaybeOfficialToolchainName {
     fn validate(candidate: &str) -> Result<MaybeOfficialToolchainName, InvalidName> {
-        let candidate = validate(candidate)?;
+        let candidate = validate_named_toolchain(candidate)?;
         if candidate == "none" {
             Ok(MaybeOfficialToolchainName::None)
         } else {
@@ -262,7 +249,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)?;
         candidate
             .parse::<ToolchainDesc>()
             .map(ToolchainName::Official)
@@ -311,8 +298,13 @@ impl ResolvableLocalToolchainName {
         ResolvableToolchainName::try_from(candidate)
             .map(ResolvableLocalToolchainName::Named)
             .or_else(|_| {
-                PathBasedToolchainName::try_from(&PathBuf::from(candidate) as &Path)
-                    .map(ResolvableLocalToolchainName::Path)
+                let path = PathBuf::from(candidate);
+                if candidate.contains('/') || candidate.contains('\\') {
+                    PathBasedToolchainName::try_from(&path as &Path)
+                        .map(ResolvableLocalToolchainName::Path)
+                } else {
+                    Err(InvalidName::ToolchainName(candidate.into()))
+                }
             })
     }
 }
@@ -394,6 +386,7 @@ impl CustomToolchainName {
             || candidate == "none"
             || candidate.contains('/')
             || candidate.contains('\\')
+            || !is_legal_named_toolchain(candidate)
         {
             Err(InvalidName::CustomName(candidate.into()))
         } else {
@@ -473,6 +466,36 @@ impl Deref for PathBasedToolchainName {
     }
 }
 
+fn validate_named_toolchain(candidate: &str) -> Result<&str, InvalidName> {
+    let candidate = validate(candidate)?;
+    if is_legal_named_toolchain(candidate) {
+        Ok(candidate)
+    } else {
+        Err(InvalidName::ToolchainName(candidate.into()))
+    }
+}
+
+/// 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)
+    }
+}
+
+fn is_legal_named_toolchain(candidate: &str) -> bool {
+    !matches!(candidate, "." | "..") && candidate.chars().all(is_legal_named_toolchain_char)
+}
+
+fn is_legal_named_toolchain_char(c: char) -> bool {
+    c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | '-')
+}
+
 #[cfg(test)]
 mod tests {
     use std::str::FromStr;
@@ -490,13 +513,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 {
@@ -506,9 +537,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
         }
     }
@@ -541,11 +571,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::try_from(name).unwrap();
         }
 
         #[test]
         fn test_parse_resolvable_name(name in arb_resolvable_name()) {
+            prop_assume!(name != "none");
+            prop_assume!(name != ".");
+            prop_assume!(name != "..");
             ResolvableToolchainName::try_from(name).unwrap();
         }
 
@@ -577,10 +614,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::try_from(s).unwrap())
@@ -601,11 +638,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::try_from(s).unwrap())
@@ -615,4 +652,21 @@ 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",
+            "quote'toolchain",
+            ".",
+            "..",
+        ] {
+            CustomToolchainName::try_from(name).unwrap_err();
+            ResolvableToolchainName::try_from(name).unwrap_err();
+            ToolchainName::try_from(name).unwrap_err();
+        }
+    }
 }

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();
 }

tests/suite/cli_rustup.rs

@@ -3747,7 +3747,7 @@ async fn non_utf8_toolchain() {
         .await
         .with_stderr(snapbox::str![[r#"
 ...
-error: toolchain '�(' is not installed
+error: invalid toolchain name '�('
 ...
 "#]]);
 }