Skip to content

Commit 54b31a7

Browse files
cachebagcachebag
committed
fix(toolchain)!: restrict named toolchain characters
1 parent 8d75c1b commit 54b31a7

4 files changed

Lines changed: 91 additions & 28 deletions

File tree

‎src/toolchain/names.rs‎

Lines changed: 70 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ impl ResolvableToolchainName {
131131
// If candidate could be resolved, return a ready to resolve version of it.
132132
// Otherwise error.
133133
fn validate(candidate: &str) -> Result<Self, InvalidName> {
134-
let candidate = validate(candidate)?;
134+
let candidate = validate_named_toolchain(candidate)?;
135135
if let Ok(desc) = PartialToolchainDesc::from_str(candidate) {
136136
return Ok(Self::Official(desc));
137137
}
@@ -201,7 +201,7 @@ pub(crate) enum MaybeOfficialToolchainName {
201201

202202
impl MaybeOfficialToolchainName {
203203
fn validate(candidate: &str) -> Result<Self, InvalidName> {
204-
Ok(match validate(candidate)? {
204+
Ok(match validate_named_toolchain(candidate)? {
205205
"none" => Self::None,
206206
candidate => Self::Some(
207207
PartialToolchainDesc::from_str(candidate)
@@ -234,7 +234,7 @@ pub enum ToolchainName {
234234
impl ToolchainName {
235235
/// If the string is already resolved, allow direct conversion
236236
fn validate(candidate: &str) -> Result<Self, InvalidName> {
237-
let candidate = validate(candidate)?;
237+
let candidate = validate_named_toolchain(candidate)?;
238238
if let Ok(desc) = ToolchainDesc::from_str(candidate) {
239239
return Ok(Self::Official(desc));
240240
}
@@ -285,9 +285,13 @@ impl ResolvableLocalToolchainName {
285285
return Ok(Self::Named(name));
286286
}
287287

288-
Ok(Self::Path(PathBasedToolchainName::try_from(
289-
&PathBuf::from(candidate) as &Path,
290-
)?))
288+
if candidate.contains('/') || candidate.contains('\\') {
289+
let path = PathBuf::from(candidate);
290+
let path = PathBasedToolchainName::try_from(&path as &Path)?;
291+
return Ok(Self::Path(path));
292+
}
293+
294+
Err(InvalidName::ToolchainName(candidate.into()))
291295
}
292296
}
293297

@@ -363,12 +367,9 @@ pub struct CustomToolchainName(String);
363367

364368
impl CustomToolchainName {
365369
fn validate(candidate: &str) -> Result<Self, InvalidName> {
366-
let candidate = validate(candidate)?;
367-
if candidate.parse::<PartialToolchainDesc>().is_ok()
368-
|| candidate == "none"
369-
|| candidate.contains('/')
370-
|| candidate.contains('\\')
371-
{
370+
let candidate = validate_named_toolchain(candidate)
371+
.map_err(|_| InvalidName::CustomName(candidate.into()))?;
372+
if candidate.parse::<PartialToolchainDesc>().is_ok() || candidate == "none" {
372373
Err(InvalidName::CustomName(candidate.into()))
373374
} else {
374375
Ok(Self(candidate.into()))
@@ -447,6 +448,19 @@ impl Deref for PathBasedToolchainName {
447448
}
448449
}
449450

451+
fn validate_named_toolchain(candidate: &str) -> Result<&str, InvalidName> {
452+
let candidate = validate(candidate)?;
453+
if !matches!(candidate, "." | "..")
454+
&& candidate
455+
.chars()
456+
.all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | '-'))
457+
{
458+
Ok(candidate)
459+
} else {
460+
Err(InvalidName::ToolchainName(candidate.to_owned()))
461+
}
462+
}
463+
450464
/// Common validate rules for all sorts of toolchain names
451465
fn validate(candidate: &str) -> Result<&str, InvalidName> {
452466
if let Some(without_plus) = candidate.strip_prefix('+') {
@@ -477,13 +491,21 @@ mod tests {
477491
fn partial_toolchain_desc_regex() -> String {
478492
let tuple_regex = format!(
479493
r"(-({}))?(?:-({}))?(?:-({}))?",
480-
LIST_ARCHS.join("|"),
481-
LIST_OSES.join("|"),
482-
LIST_ENVS.join("|")
494+
regex_alternates(LIST_ARCHS),
495+
regex_alternates(LIST_OSES),
496+
regex_alternates(LIST_ENVS)
483497
);
484498
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
485499
}
486500

501+
fn regex_alternates(values: &[&str]) -> String {
502+
values
503+
.iter()
504+
.map(|value| regex::escape(value))
505+
.collect::<Vec<_>>()
506+
.join("|")
507+
}
508+
487509
prop_compose! {
488510
fn arb_partial_toolchain_desc()
489511
(s in string_regex(&partial_toolchain_desc_regex()).unwrap()) -> String {
@@ -493,9 +515,8 @@ mod tests {
493515

494516
prop_compose! {
495517
fn arb_custom_name()
496-
(s in r"[^\\/+][^\\/]*") -> String {
518+
(s in r"[A-Za-z0-9._-]+") -> String {
497519
// perhaps need to filter 'none' and partial toolchains - but they won't typically be generated anyway.
498-
// Also filter '+' prefix as that's reserved for +toolchain syntax.
499520
s
500521
}
501522
}
@@ -528,11 +549,18 @@ mod tests {
528549

529550
#[test]
530551
fn test_parse_custom(name in arb_custom_name()) {
552+
prop_assume!(name != "none");
553+
prop_assume!(name != ".");
554+
prop_assume!(name != "..");
555+
prop_assume!(PartialToolchainDesc::from_str(&name).is_err());
531556
CustomToolchainName::try_from(name).unwrap();
532557
}
533558

534559
#[test]
535560
fn test_parse_resolvable_name(name in arb_resolvable_name()) {
561+
prop_assume!(name != "none");
562+
prop_assume!(name != ".");
563+
prop_assume!(name != "..");
536564
ResolvableToolchainName::try_from(name).unwrap();
537565
}
538566

@@ -564,10 +592,10 @@ mod tests {
564592
"1.8.0-x86_64-apple-darwin",
565593
"1.8.0-x86_64-unknown-linux-gnu",
566594
"1.10.0-x86_64-unknown-linux-gnu",
567-
"bar(baz)",
568-
"foo#bar",
569-
"the cake is a lie",
570-
"this.is.not-a+semver",
595+
"bar.baz",
596+
"foo_bar",
597+
"stage1-local",
598+
"this.is.not-a_semver",
571599
]
572600
.into_iter()
573601
.map(|s| ToolchainName::try_from(s).unwrap())
@@ -588,11 +616,11 @@ mod tests {
588616
"1.8.0-beta-x86_64-apple-darwin",
589617
"1.8.0-beta.2-x86_64-apple-darwin",
590618
// https://github.com/rust-lang/rustup/issues/3517
591-
"foo#bar",
592-
"bar(baz)",
593-
"this.is.not-a+semver",
619+
"foo_bar",
620+
"bar.baz",
621+
"this.is.not-a_semver",
594622
// https://github.com/rust-lang/rustup/issues/3168
595-
"the cake is a lie",
623+
"stage1-local",
596624
]
597625
.into_iter()
598626
.map(|s| ToolchainName::try_from(s).unwrap())
@@ -602,4 +630,21 @@ mod tests {
602630

603631
assert_eq!(expected, v);
604632
}
633+
634+
#[test]
635+
fn custom_names_reject_special_characters() {
636+
for name in [
637+
"bar(baz)",
638+
"foo#bar",
639+
"the cake is a lie",
640+
"this.is.not-a+semver",
641+
"quote'toolchain",
642+
".",
643+
"..",
644+
] {
645+
CustomToolchainName::try_from(name).unwrap_err();
646+
ResolvableToolchainName::try_from(name).unwrap_err();
647+
ToolchainName::try_from(name).unwrap_err();
648+
}
649+
}
605650
}

‎tests/suite/cli_misc.rs‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,24 @@ error:[..] invalid custom toolchain name 'beta'
9898
...
9999
error:[..] invalid custom toolchain name 'stable'
100100
...
101+
"#]])
102+
.is_err();
103+
cx.config
104+
.expect(["rustup", "toolchain", "link", "bad name", "foo"])
105+
.await
106+
.with_stderr(snapbox::str![[r#"
107+
...
108+
error:[..] invalid custom toolchain name 'bad name'
109+
...
110+
"#]])
111+
.is_err();
112+
cx.config
113+
.expect(["rustup", "toolchain", "link", "foo#bar", "foo"])
114+
.await
115+
.with_stderr(snapbox::str![[r#"
116+
...
117+
error:[..] invalid custom toolchain name 'foo#bar'
118+
...
101119
"#]])
102120
.is_err();
103121
}

‎tests/suite/cli_rustup.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3747,7 +3747,7 @@ async fn non_utf8_toolchain() {
37473747
.await
37483748
.with_stderr(snapbox::str![[r#"
37493749
...
3750-
error: toolchain '�(' is not installed
3750+
error: invalid toolchain name '�('
37513751
...
37523752
"#]]);
37533753
}
@@ -3774,7 +3774,7 @@ async fn non_utf8_toolchain() {
37743774
.await
37753775
.with_stderr(snapbox::str![[r#"
37763776
...
3777-
error: toolchain '��' is not installed
3777+
error: invalid toolchain name '��'
37783778
...
37793779
"#]]);
37803780
}

‎tests/suite/cli_v2.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1617,7 +1617,7 @@ async fn cannot_add_empty_named_custom_toolchain() {
16171617
.await
16181618
.with_stderr(snapbox::str![[r#"
16191619
...
1620-
error: invalid value '' for '<TOOLCHAIN>': invalid toolchain name ''
1620+
error: invalid value '' for '<TOOLCHAIN>': invalid custom toolchain name ''
16211621
...
16221622
"#]])
16231623
.is_err();

0 commit comments

Comments
 (0)