fix(toolchain)!: restrict named toolchain characters

[cachebag]

Partially addresses #4059 by restricting what characters a named toolchain may contain.

Named toolchains are now validated against the UTS #39 general security profile.

In essence; unicode_security::GeneralSecurityProfile::identifier_allowed() vs this hand-rolled pattern we had before. Letters and digits in any script stay legal, so 合法的 still works, as do ., _ and -. Whitespace, most punctuation, emoji, and invisible or direction-altering characters are rejected. I also exclude : (NTFS alternate data streams) and ' (shell quoting), plus . and .., which the profile allows.

This is breaking for unusual names like foo#bar or names with spaces. However it doesn't touch the rust-toolchain.toml diagnostics. Confusables are also still unresolved (i.e.μ vs µ, precomposed vs decomposed é).

[cachebag]

Note to myself that this will probably need some corrections once #4930 is merged

[rami3l]

@cachebag Thanks for this PR!

I think given your comment in #4932 (comment) I'll probably review #4930 first and come back to this one later, many thanks for your understanding 🙏

[rami3l]

Nit: Given that the previous change #4930 has focused on rewriting the existing validation code from functional to imperative, suggest keeping the imperative style here to minimize the diff. Same with the other functions that you may or may not have changed in this patch.

View changes since the review

[rami3l]

Question: what is the rationale for having two distinct functions validate() and validate_named_toolchain() in this module?

View changes since the review

[cachebag]

As touched on in #4932 (comment) - validate() is a common baseline used by both named and path-capable inputs. The latter is stricter for the purpose of the introduced allowlist

[rami3l]

Sorry for not having expressed my intentions super clearly. My question is rather: after your changes, do the validate() function still have any reasons to exist? Are there any cases where we want the normalization without the legality check/validation?

If not or if it becomes single-used, then maybe it's better just to keep the actual "validation" function to avoid confusions.

[cachebag]

Right- so validate() is still needed because ResolvableLocalToolchainName::validate uses it to handle both named toolchains and paths, so it needs the basic normalization without the character allowlist.

rustup/src/toolchain/names.rs

Lines 293 to 305 in 3432d80

	/// Validates if the string is a resolvable toolchain, or a path based toolchain.
	fn validate(candidate: &str) -> Result<Self, InvalidName> {
	let candidate = validate(candidate)?;
	if let Ok(name) = ResolvableToolchainName::try_from(candidate) {
	return Ok(Self::Named(name));
	}
	Ok(Self::Path(PathBasedToolchainName::try_from(
	&PathBuf::from(candidate) as &Path,
	)?))
	}
	}

More concretely, it's used in three places: ResolvableLocalToolchainName::validate, MaybeResolvableToolchainName::validate and validate_named_toolchain itself.

validate_named_toolchain() builds on top of it for the stricter named-only callers.

I've applied your suggestion for CustomToolchainName. It now uses validate_named_toolchain() and is_legal_named_toolchain has been inlined.

Is there something I am misunderstanding? Are you asking if we should inline validate()'s logic? If so, that repeats code more than we would be by just keeping these two functions separate.

Though maybe I am just misunderstanding your question completely, and if so then pardon me.

[rami3l]

Question: How is it/why should it be different from validate()/validate_named_toolchain()?

View changes since the review

[cachebag]

validate() only does shared normalization/basic checks: reject +, trim trailing /, reject empty. It still allows paths and old broad custom names.

validate_named_toolchain() builds on that and rejects illegal named-toolchain characters, returning a ToolchainName error.

It feels to me like it is the reusable predicate for just the allowlist. I wrote it because CustomToolchainName::validate() needs the same allowlist but must return CustomName, not ToolchainName.

That being said the names are not great. We could probably have them be normalize_toolchain_name(), is_valid_toolchain_name and validate_toolchain_name()?

[rami3l]

Looking at your proposed name it looks like is_valid_toolchain_name(name) can be inlined as validate_toolchain_name(name).is_ok(). Of course this is not the case currently but I can't help thinking about moving towards that direction.

Looking at the docstring of validate():

/// Common validate rules for all sorts of toolchain names

It looks like we should just do all possible verification passes in one place? Would that be possible? Something like the following, for example:

impl CustomToolchainName {
    fn validate(candidate: &str) -> Result<Self, InvalidName> {
-       let candidate = validate(candidate)?;
+       // At this point there is already no need to check for `/` and `\`.
+       let candidate = validate_toolchain_name(candidate)?;
        if candidate.parse::<PartialToolchainDesc>().is_ok()
            || candidate == "none"
-           || candidate.contains('/')
-           || candidate.contains('\\')
        {
            return Err(InvalidName::CustomName(candidate.into()))
        }
        Ok(Self(candidate.into()))
    }
}

What I am trying to say is that in that case is_valid_toolchain_name(name) is not needed anymore.

[cachebag]

Makes sense. Done.

[rami3l]

Nit: If we decide to keep it, it looks like it should be called is_legal_toolchain_name().

View changes since the review

[cachebag]

See above responses.

I do like is_legal_toolchain_name() more than is_valid_...()

[djc]

Why? We have a bunch of validate_* functions to I like that is_valid_ references that more clearly than introducing a seemingly separate concept of "legality".

[cachebag]

"legal" seemed to reflect the new allowlist/policy. i'm not hard pressed on the naming convention, is_valid_ is better for your reasons, that makes sense to me from a code standpoint

[rami3l]

@cachebag Sorry for the delay in reviewing! I have to see if the current code can be further streamlined and what we should do about #4059 (comment). Please stay tuned...