Error on invalid alphanumeric token for crates.io

[Akida31]

ref #11571

When using cargo login and calling an api which requires authentification there will be an error if the given token is not a valid alphanumerical string.
This check is only enabled for crates.io because
only for that registry we can be certain, that the generated token should have been alphanumeric, see the code here. So if I'm not mistaken, this should not be a breaking change, since crates.io only generates fitting tokens. (Should I add a comment to the crates.io code that modifying this logic can break cargo?)

I'm not sure if the fix works and is enough to close the issue, please say if you have any corrections or improvements!

I don't know if the check should also be enabled for other registries and it would be really bad if the check is too strict.
In the linked issue it was recommended to encode invalid characters, but I don't know in which encoding. I saw in this http rfc that only the ISO-8859-1 charset is allowed and everything else must be encoded but this seems somewhat complex and hard to implement. There is a crate rust-encoding which should be capable doing this (from a first look), but I don't know if a new dependency only for this is justified. There seems to be percent encoding already in the dependency tree but I have no idea if it would be correct and work.
If you have any idea about this encoding, please say so.

r? @Eh2406 (since you suggested the encoding part)

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon.

Please see the contribution instructions for more information.

[Eh2406]

Thank you for working on this!

I think this is a good change and a big step in the right direction. But I think we can go further. If the HTTP spec requires ISO-8859-1 charset, then I think cargo should error if you give it a token that is not in that range. (I do not think cargo should do any escaping.) This would technically be a breaking change, but only for registries that use very odd tokens. How hard would it be to check just that property?

If it's doable, I can bring up the breaking change with the Team.

[Akida31]

Thanks for your reply and sorry for the late answer, I had not worked with encodings before so I had to read some things (and had many other things to do ;) ).

If I understand ISO 8859-1 correctly, only bytes in the range 0x20-0x7e and 0xa0-0xff are allowed (defined). But our String is utf-8 and for that it looks like the last valid code point for chars with a size of 1 byte is 0x7e.

I've created a test here: commit / files
It looks like flask accepts all header values (even if they are invalid per spec) and axum accepts only the following (source)

axum and crates.io use the http crate for validation of header values:

fn is_valid(b: u8) -> bool {
    b >= 32 && b != 127 || b == b'\t'
}

The python requests library only permits unicode values up to 0xff.
So I would recommend restricting the token to

fn check(b: u8) -> bool {
    b >= 32 // undefined in ISO-8859-1, in ASCII/ UTF-8 not-printable character
    && b < 128 // utf-8: the first bit signals a multi-byte character
    && b != 127 // 127 is a control character in ascii and not in ISO 8859-1
    || b == b't' // tab is also allowed (even when < 32)
}

As this is more restrictive than the is_valid check in the crates.io server more tokens would be denied from cargo.
But crates.io tokens are currently only alphanumeric so this should not be a problem in the future. alexandrie (an alternative registry) uses only alphanumeric tokens too.
So it could make sense to restrict tokens to alphanumeric, since it would make the logic easier.

This is my opinion and I have not that much experience with that topic :)
If you think that the code above is the way to proceed, please say so and I will implement it.

[Eh2406]

Thank you for doing the research! I agree, the correct approach is for cargo to check the token as inputted for all registries and make sure it passes the check function you defined. We could be more restrictive for crates.io, but I don't know if it's worth it.

This is technically a breaking change. But I don't think it will affect users doing anything reasonable so I think it is worth doing. Any registry uses other characters is just asking for HTTP Confucian attacks.

However, that is my opinion. I will nominate for the cargo team to discuss in a meeting.

[Eh2406]

The cargo team discussed this proposal today. We agreed to require that all tokens match your proposed check function! If this breaks someone they can file an issue and suggest a new standard, but I strongly suspect this will not break anyone.

[Akida31]

I've rebased and implemented the check as written.
I'm not quite sure about the error message (in crates/crates_io/lib.rs:531). It is rather unhelpful but I think printing the token wouldn't be that secure.
Also I don't know what the best solution is for src/cargo/ops/registry.rs:904: Exposing the token isn't that nice but Secure is not available in crates/crates_io/lib.rs

[Eh2406]

Thank you so much, this is looking almost there.

[Akida31]

• moved is_empty check into check_token
• improved error message (is quite long now but should explain the error well)
• removed one helper function from new test

If you have any other improvements, please say so!

And I don't know how the normal process is, if this somewhat breaking change should get a short note in release notes or somewhere else.

[Eh2406]

@bors r+

[bors]

📌 Commit 8502fa8 has been approved by Eh2406

It is now in the queue for this repository.

[bors]

⌛ Testing commit 8502fa8 with merge a66f123...

[bors]

☀️ Test successful - checks-actions
Approved by: Eh2406
Pushing a66f123 to master...

[Akida31]

Thanks for your review!

[weihanglo]

I guess we forgot to bump the version of crates-io crate, no?