fix(toolchain)!: restrict named toolchain characters - #4932
Conversation
|
Note to myself that this will probably need some corrections once #4930 is merged |
1318f92 to
9118fcc
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@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 🙏 |
86f5e64 to
b8c8bb9
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
913f977 to
a8ed506
Compare
| } | ||
| } | ||
|
|
||
| fn is_legal_named_toolchain(candidate: &str) -> bool { |
There was a problem hiding this comment.
Nit: If we decide to keep it, it looks like it should be called is_legal_toolchain_name().
There was a problem hiding this comment.
See above responses.
I do like is_legal_toolchain_name() more than is_valid_...()
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
"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
changed the title
b1e3812 to
39fbbcc
Compare
|
@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... |
8e4516c to
39f608c
Compare
d14a77d to
4259f6e
Compare
This comment has been minimized.
This comment has been minimized.
0bc07d2 to
2eecf4a
Compare
Validate named toolchains with the UTS rust-lang#39 general security profile via `unicode_security::GeneralSecurityProfile::identifier_allowed()` instead of an ASCII allowlist, following the approach sketched in rust-lang#4059. Letters and digits in any script are now legal, so `合法的` works as a custom toolchain name. ASCII letters, digits, `.`, `_` and `-` remain allowed, so every official toolchain name still parses. Whitespace, most punctuation, emoji, and invisible or direction-altering characters such as U+202E RIGHT-TO-LEFT OVERRIDE are still rejected. Two characters the profile permits are excluded anyway: `:`, because a named toolchain becomes a directory under `.rustup/toolchains` and `name:stream` denotes an NTFS alternate data stream on Windows, and `'`, which needs quoting in too many shells to be worth allowing. `.` and `..` are rejected explicitly, since the profile permits both. Confusables remain unresolved: `μ` is accepted while `µ` is not, and precomposed and decomposed `é` are distinct names. That matches the existing status quo.
`try_from_str!()` fed `TryFrom<String>`, `TryFrom<&str>` and `FromStr` off a single inherent `validate()`, which is why each name type had one. With the macro gone every such method had exactly one caller, its own `from_str()`, so move the bodies there and drop the methods. Rename the free `validate()` to `normalize_name()`, which is precisely what it does: strips a `+` prefix, trims trailing slashes, and rejects empty names.
| /// Strips the trailing slashes a shell may have completed onto a toolchain | ||
| /// directory, and rejects the `+toolchain` argument form along with names that | ||
| /// are empty once normalized. | ||
| fn normalize_name(candidate: &str) -> Result<&str, InvalidName> { |
There was a problem hiding this comment.
This is a good idea! In this case can we use validate_name() instead of validate_named_toolchain() for the previous function?
|
|
||
| /// Common validate rules for all sorts of toolchain names | ||
| fn validate(candidate: &str) -> Result<&str, InvalidName> { | ||
| /// Normalization shared by all sorts of toolchain names. |
There was a problem hiding this comment.
Nit: Suggest putting the renaming commit(s) in the beginning of this PR.
|
|
||
| Custom toolchain names may contain ASCII letters, ASCII digits, `.`, `_`, and | ||
| `-`. | ||
| Custom toolchain names may contain any character that [UTS #39] allows in an |
There was a problem hiding this comment.
Nit: I think this commit can be squashed into previous ones to keep the overall diff minimized, as we are not really going to adopt the ASCII-only string pattern for toolchain names.
| /// 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] = &[':', '\'']; |
There was a problem hiding this comment.
Very carefully designed, nice work :)
4 participants
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#baror names with spaces. However it doesn't touch therust-toolchain.tomldiagnostics. Confusables are also still unresolved (i.e.μvsµ, precomposed vs decomposedé).