Tracking: Limit the range of valid toolchain names, take caution in `rust-toolchain.toml`

[rami3l]

As discussed in #4053 (comment), there are basically two issues to address:

1. Improve current error messages and treat toolchain names coming from rust-toolchain.toml as untrusted. Done.
2. Come up with more restrictive rules in respect to legal toolchain names.
   • If the rules allow the prevention of phishing, lift the restriction on displaying toolchain names from rust-toolchain.toml if they are legal.

Tasks

• Toolchain Channel Manipulation within Rustup #4053
• fix(toolchain): forbid toolchain names starting with + #4650
• fix(toolchain)!: restrict named toolchain characters #4932
• Proposal: special toolchain names referring to particular override levels #5025

[cachebag]

@rami3l
I think we should be pretty conservative for custom toolchain names only (i.e. [A-Za-z0-9._-]+) while still rejecting

• none
• +prefix
• official/resolvable toolchain names
• names containing / or \

This IMO keeps common names a user might use and just removes whitepsaces, quotes, shell metacharacters, etc. that seemingly makes error messages or terminal output easier to weaponize.

Now I guess this is technically a compatibility break for existing linked custom toolchains with names such as spaces or #/parentheses, since ToolchainName::try_from is also used when listing installed toolchains...

But given the goal of this issue is to make legal toolchain names clearly defined and safer to display, I think the allowlist is preferable to continuing with “anything except a few banned characters.”

What do you think?

[rami3l]

@cachebag I think we are pretty much discussing 2 things with the naming restrictions here:

1. What should a "generally acceptable toolchain name" should look like.
   • I agree that indeed [A-Za-z0-9._-]+ might already be enough for 99% of the cases. This seems to be a very good decision considering [a-zA-Z_][a-zA-Z_0-9]* from the common identifier rule and 1.23.0-beta.2 from feat: support for parsing beta versions with tags in the toolchain #3858).
   • However there might be some existing normalization rules about slashes that we might want to keep.
   • This is later categorized into official and custom toolchain names so in principle, as you have noticed above, the two cannot overlap. If you have found any counter-examples existing in our current logic, please don't hesitate to file another issue.
2. What names we want to reserve that may have special meaning in the future. For now we have none, but we may want more than that just in case.
   • IIRC there is an issue where the user wants to use some form of the active toolchain explicitly in rustup run and proxies (Environment variable to disable rust-toolchain.toml #2793), so in this case it might also be interesting to choose a prefix to ban. +file: has been proposed as a placeholder in rust-toolchain.toml: support multiple channels, or rather support nightly AND stable version. #4391 (comment) because it starts with +, however I'm not entirely sure if that's the best idea XD

[djc]

• This is later categorized into official and custom toolchain names so in principle, as you have noticed above, the two cannot overlap. If you have found any counter-examples existing in our current logic, please don't hesitate to file another issue.

It would be nice to structure the code such that the type system can enforce this.

[ChrisDenton]

I think we can be more forgiving then [A-Za-z0-9._-]+ and allow any non-ascii character. I cannot see us using anything but ascii for special characters (and control characters are ascii too). I do worry that being too restrictive will break some real-world usage (e.g. people using toolchain names in their own language). Though for rust-toolchain.toml I can definitely see the wisdom in being more restrictive (maybe use something more like rust's identifiers which use XID_CONTINUE).

[rami3l]

I think we can be more forgiving then [A-Za-z0-9._-]+ and allow any non-ascii character.
I do worry that being too restrictive will break some real-world usage (e.g. people using toolchain names in their own language).

@ChrisDenton My 2 cents as a Chinese: this smells like homograph attack to me :o

For example, according to https://github.com/jagracey/Awesome-Unicode#user-content-variable-identifiers-can-effectively-include-whitespace, this is valid Rust:

fn main() {
    let ᅟ = 'f';
    dbg!(ᅟ);
}

This is how it looks in my Ghostty:

... and we can make #4053 resurface if we are not careful enough about what to include. Also, many possible confusions around normalization and other Unicode-specific stuff as I can imagine...

[ChrisDenton]

That does trigger a rustc warning (playground):

warning: identifier contains an uncommon character: 'ᅟ'
...
  = note: this character is included in the Unicode general security profile
  = note: `#[warn(uncommon_codepoints)]` on by default

We could be more restrictive than the rust default (e.g. effectively act as if #![forbid(uncommon_codepoints)] is enabled). I do worry that the more restrictive we are the more likely we are to break people, especially as attacks are only an issue for rust-toolchain.toml.

Though maybe it is worth testing the most restrictive version. Unfortunately I don't think many people use our betas...

[ChrisDenton]

Basically I think there's two issues here:

1. The characters and names we want to restrict because rustup (or maybe other tools) wants to reserve them
2. For the security of rust-toolchain.toml we want to further restrict valid characters

[rami3l]

If you consider something more like the Rust identifier, the pattern might should look more or less like this (not available on the official playground, but you can try with this link):

/*
[dependencies]
unicode-security = "0.1"
*/

fn main() {
    assert!(is_legal_toolchain_name("_wow"));
    assert!(is_legal_toolchain_name("0.03"));
    assert!(is_legal_toolchain_name("合法的"));
    assert!(!is_legal_toolchain_name("µ"));
}

use unicode_security::GeneralSecurityProfile;

fn is_legal_toolchain_name(s: &str) -> bool {
    !s.is_empty() && s.chars().all(|c| c.identifier_allowed())
}

@ChrisDenton Is that what you'd expect in your previous reply? I'm still worried about é vs é, o vs ο and other things this doesn't resolve. Of course those will be existing issues so we can retain the status quo in that respect.