Error on invalid alphanumeric token for crates.io #11600

Akida31 · 2023-01-19T18:00:57Z

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 · 2023-01-19T18:01:04Z

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 · 2023-01-19T20:51:10Z

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 · 2023-01-26T14:44:46Z

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 · 2023-01-26T15:05:09Z

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 · 2023-02-14T19:58:37Z

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.

When using registry operations with authentication there will be now an error if the given token is not valid. This is a technically a breaking change because a registry might give some tokens which will be denied by these new checks. In practice these tokens cause issues with HTTP so no registry should generate them.

Akida31 · 2023-02-16T15:57:57Z

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.

src/cargo/ops/registry.rs

crates/crates-io/lib.rs

tests/testsuite/login.rs

* 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

Akida31 · 2023-02-16T18:16:10Z

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 · 2023-02-16T19:46:34Z

@bors r+

bors · 2023-02-16T19:46:36Z

📌 Commit 8502fa8 has been approved by Eh2406

It is now in the queue for this repository.

bors · 2023-02-16T19:46:43Z

⌛ Testing commit 8502fa8 with merge a66f123...

bors · 2023-02-16T20:34:10Z

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

Akida31 · 2023-02-17T09:45:07Z

Thanks for your review!

10 commits in 39c13e67a5962466cc7253d41bc1099bbcb224c3..17b3d0de0897e1c6b8ca347bd39f850bb0a5b9f6 2023-02-12 02:01:08 +0000 to 2023-02-17 19:45:09 +0000 - fix: unsupported protocol error on old macos version (rust-lang/cargo#11733) - Error on invalid alphanumeric token for crates.io (rust-lang/cargo#11600) - Add clippy lints (rust-lang/cargo#11722) - chore: Make dependencies alphabetical order (rust-lang/cargo#11719) - chore: bump mdbook to 0.4.27 (rust-lang/cargo#11716) - Amend `mdman` tests. (rust-lang/cargo#11715) - Run CI for macOS on nightly (rust-lang/cargo#11712) - doc: doc comments and intra-doc links for `core::compiler` (rust-lang/cargo#11711) - Ensure em dashes are recognizable in markup (rust-lang/cargo#11646) - Set CARGO_BIN_NAME environment variable also for binary examples (rust-lang/cargo#11705)

Update cargo 10 commits in 39c13e67a5962466cc7253d41bc1099bbcb224c3..17b3d0de0897e1c6b8ca347bd39f850bb0a5b9f6 2023-02-12 02:01:08 +0000 to 2023-02-17 19:45:09 +0000 - fix: unsupported protocol error on old macos version (rust-lang/cargo#11733) - Error on invalid alphanumeric token for crates.io (rust-lang/cargo#11600) - Add clippy lints (rust-lang/cargo#11722) - chore: Make dependencies alphabetical order (rust-lang/cargo#11719) - chore: bump mdbook to 0.4.27 (rust-lang/cargo#11716) - Amend `mdman` tests. (rust-lang/cargo#11715) - Run CI for macOS on nightly (rust-lang/cargo#11712) - doc: doc comments and intra-doc links for `core::compiler` (rust-lang/cargo#11711) - Ensure em dashes are recognizable in markup (rust-lang/cargo#11646) - Set CARGO_BIN_NAME environment variable also for binary examples (rust-lang/cargo#11705) r? `@ghost`

weihanglo · 2023-03-09T12:06:12Z

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

This was an overlook of rust-lang#11600

Bump crates-io to 0.36.0 This was an overlook of #11600 Since we already got #11806 to backport, I guess it is not harmful to <https://github.com/rust-lang/cargo/labels/beta-nominated> this as well. Maybe it do need a backport as `src/cargo/ops/registry.rs` use a new public API from that PR. BTW, please help check if it is really a breaking change.

Bump crates-io to 0.36.0 This was an overlook of rust-lang#11600 Since we already got rust-lang#11806 to backport, I guess it is not harmful to <https://github.com/rust-lang/cargo/labels/beta-nominated> this as well. Maybe it do need a backport as `src/cargo/ops/registry.rs` use a new public API from that PR. BTW, please help check if it is really a breaking change.

Validate token on publish. The `publish` path was not validating the token like the other API routes were (like owner, or yank). This does not appear to be intentional from what I can tell. This consolidates the relevant code so that it is shared with all the API calls. cc #11600 Closes #11571

Fix credential token format validation. The existing validation incorrectly excluded tab because of a missing backslash. This updates to add the backslash. This also rewords the comments. I found the current comments to be a little confusing. This also extends the test to verify more valid inputs. cc #11600

Pkgsrc changes: * Adjust patches and cargo checksums to new versions. * Sadly, the patch to reduce the cargo verbosity no longer applies, so I've asked upstream about the proper way to get the old result. (so the build log becomes Quite Bloated for now). Upstream changes: Version 1.69.0 (2023-04-20) ========================== Language -------- - [Deriving built-in traits on packed structs works with `Copy` fields.] (rust-lang/rust#104429) - [Stabilize the `cmpxchg16b` target feature on x86 and x86_64.] (rust-lang/rust#106774) - [Improve analysis of trait bounds for associated types.] (rust-lang/rust#103695) - [Allow associated types to be used as union fields.] (rust-lang/rust#106938) - [Allow `Self: Autotrait` bounds on dyn-safe trait methods.] (rust-lang/rust#107082) - [Treat `str` as containing `[u8]` for auto trait purposes.] (rust-lang/rust#107941) Compiler -------- - [Upgrade `*-pc-windows-gnu` on CI to mingw-w64 v10 and GCC 12.2.] (rust-lang/rust#100178) - [Rework min_choice algorithm of member constraints.] (rust-lang/rust#105300) - [Support `true` and `false` as boolean flags in compiler arguments.] (rust-lang/rust#107043) - [Default `repr(C)` enums to `c_int` size.] (rust-lang/rust#107592) Libraries --------- - [Implement the unstable `DispatchFromDyn` for cell types, allowing downstream experimentation with custom method receivers.] (rust-lang/rust#97373) - [Document that `fmt::Arguments::as_str()` may return `Some(_)` in more cases after optimization, subject to change.] (rust-lang/rust#106823) - [Implement `AsFd` and `AsRawFd` for `Rc`.] (rust-lang/rust#107317) Stabilized APIs --------------- - [`CStr::from_bytes_until_nul`] (https://doc.rust-lang.org/stable/core/ffi/struct.CStr.html#method.from_bytes_until_nul) - [`core::ffi::FromBytesUntilNulError`] (https://doc.rust-lang.org/stable/core/ffi/struct.FromBytesUntilNulError.html) These APIs are now stable in const contexts: - [`SocketAddr::new`] (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.new) - [`SocketAddr::ip`] (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.ip) - [`SocketAddr::port`] (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.port) - [`SocketAddr::is_ipv4`] (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.is_ipv4) - [`SocketAddr::is_ipv6`] (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.is_ipv6) - [`SocketAddrV4::new`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV4.html#method.new) - [`SocketAddrV4::ip`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV4.html#method.ip) - [`SocketAddrV4::port`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV4.html#method.port) - [`SocketAddrV6::new`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.new) - [`SocketAddrV6::ip`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.ip) - [`SocketAddrV6::port`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.port) - [`SocketAddrV6::flowinfo`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.flowinfo) - [`SocketAddrV6::scope_id`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.scope_id) Cargo ----- - [Cargo now suggests `cargo fix` or `cargo clippy --fix` when compilation warnings are auto-fixable.] (rust-lang/cargo#11558) - [Cargo now suggests `cargo add` if you try to install a library crate.] (rust-lang/cargo#11410) - [Cargo now sets the `CARGO_BIN_NAME` environment variable also for binary examples.] (rust-lang/cargo#11705) Rustdoc ----- - [Vertically compact trait bound formatting.] (rust-lang/rust#102842) - [Only include stable lints in `rustdoc::all` group.] (rust-lang/rust#106316) - [Compute maximum Levenshtein distance based on the query.] (rust-lang/rust#107141) - [Remove inconsistently-present sidebar tooltips.] (rust-lang/rust#107490) - [Search by macro when query ends with `!`.] (rust-lang/rust#108143) Compatibility Notes ------------------- - [The `rust-analysis` component from `rustup` now only contains a warning placeholder.] (rust-lang/rust#101841) This was primarily intended for RLS, and the corresponding `-Zsave-analysis` flag has been removed from the compiler as well. - [Unaligned references to packed fields are now a hard error.] (rust-lang/rust#102513) This has been a warning since 1.53, and denied by default with a future-compatibility warning since 1.62. - [Update the minimum external LLVM to 14.] (rust-lang/rust#107573) - [Cargo now emits errors on invalid characters in a registry token.] (rust-lang/cargo#11600) - [When `default-features` is set to false of a workspace dependency, and an inherited dependency of a member has `default-features = true`, Cargo will enable default features of that dependency.] (rust-lang/cargo#11409) - [Cargo denies `CARGO_HOME` in the `[env]` configuration table. Cargo itself doesn't pick up this value, but recursive calls to cargo would, which was not intended.] (rust-lang/cargo#11644) - [Debuginfo for build dependencies is now off if not explicitly set. This is expected to improve the overall build time.] (rust-lang/cargo#11252) Internal Changes ---------------- These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools. - [Move `format_args!()` into AST (and expand it during AST lowering)] (rust-lang/rust#106745)

Pkgsrc changes: * Adjust patches and cargo checksums to new versions. Upstream changes: Version 1.69.0 (2023-04-20) ========================== Language -------- - [Deriving built-in traits on packed structs works with `Copy` fields.] (rust-lang/rust#104429) - [Stabilize the `cmpxchg16b` target feature on x86 and x86_64.] (rust-lang/rust#106774) - [Improve analysis of trait bounds for associated types.] (rust-lang/rust#103695) - [Allow associated types to be used as union fields.] (rust-lang/rust#106938) - [Allow `Self: Autotrait` bounds on dyn-safe trait methods.] (rust-lang/rust#107082) - [Treat `str` as containing `[u8]` for auto trait purposes.] (rust-lang/rust#107941) Compiler -------- - [Upgrade `*-pc-windows-gnu` on CI to mingw-w64 v10 and GCC 12.2.] (rust-lang/rust#100178) - [Rework min_choice algorithm of member constraints.] (rust-lang/rust#105300) - [Support `true` and `false` as boolean flags in compiler arguments.] (rust-lang/rust#107043) - [Default `repr(C)` enums to `c_int` size.] (rust-lang/rust#107592) Libraries --------- - [Implement the unstable `DispatchFromDyn` for cell types, allowing downstream experimentation with custom method receivers.] (rust-lang/rust#97373) - [Document that `fmt::Arguments::as_str()` may return `Some(_)` in more cases after optimization, subject to change.] (rust-lang/rust#106823) - [Implement `AsFd` and `AsRawFd` for `Rc`.] (rust-lang/rust#107317) Stabilized APIs --------------- - [`CStr::from_bytes_until_nul`] (https://doc.rust-lang.org/stable/core/ffi/struct.CStr.html#method.from_bytes_until_nul) - [`core::ffi::FromBytesUntilNulError`] (https://doc.rust-lang.org/stable/core/ffi/struct.FromBytesUntilNulError.html) These APIs are now stable in const contexts: - [`SocketAddr::new`] (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.new) - [`SocketAddr::ip`] (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.ip) - [`SocketAddr::port`] (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.port) - [`SocketAddr::is_ipv4`] (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.is_ipv4) - [`SocketAddr::is_ipv6`] (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.is_ipv6) - [`SocketAddrV4::new`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV4.html#method.new) - [`SocketAddrV4::ip`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV4.html#method.ip) - [`SocketAddrV4::port`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV4.html#method.port) - [`SocketAddrV6::new`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.new) - [`SocketAddrV6::ip`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.ip) - [`SocketAddrV6::port`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.port) - [`SocketAddrV6::flowinfo`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.flowinfo) - [`SocketAddrV6::scope_id`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.scope_id) Cargo ----- - [Cargo now suggests `cargo fix` or `cargo clippy --fix` when compilation warnings are auto-fixable.] (rust-lang/cargo#11558) - [Cargo now suggests `cargo add` if you try to install a library crate.] (rust-lang/cargo#11410) - [Cargo now sets the `CARGO_BIN_NAME` environment variable also for binary examples.] (rust-lang/cargo#11705) Rustdoc ----- - [Vertically compact trait bound formatting.] (rust-lang/rust#102842) - [Only include stable lints in `rustdoc::all` group.] (rust-lang/rust#106316) - [Compute maximum Levenshtein distance based on the query.] (rust-lang/rust#107141) - [Remove inconsistently-present sidebar tooltips.] (rust-lang/rust#107490) - [Search by macro when query ends with `!`.] (rust-lang/rust#108143) Compatibility Notes ------------------- - [The `rust-analysis` component from `rustup` now only contains a warning placeholder.] (rust-lang/rust#101841) This was primarily intended for RLS, and the corresponding `-Zsave-analysis` flag has been removed from the compiler as well. - [Unaligned references to packed fields are now a hard error.] (rust-lang/rust#102513) This has been a warning since 1.53, and denied by default with a future-compatibility warning since 1.62. - [Update the minimum external LLVM to 14.] (rust-lang/rust#107573) - [Cargo now emits errors on invalid characters in a registry token.] (rust-lang/cargo#11600) - [When `default-features` is set to false of a workspace dependency, and an inherited dependency of a member has `default-features = true`, Cargo will enable default features of that dependency.] (rust-lang/cargo#11409) - [Cargo denies `CARGO_HOME` in the `[env]` configuration table. Cargo itself doesn't pick up this value, but recursive calls to cargo would, which was not intended.] (rust-lang/cargo#11644) - [Debuginfo for build dependencies is now off if not explicitly set. This is expected to improve the overall build time.] (rust-lang/cargo#11252) Internal Changes ---------------- These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools. - [Move `format_args!()` into AST (and expand it during AST lowering)] (rust-lang/rust#106745)

rustbot assigned Eh2406 Jan 19, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 19, 2023

Eh2406 added the I-nominated-to-discuss To be discussed during issue triage on the next Cargo team meeting label Jan 26, 2023

Akida31 force-pushed the issue-11571-bad-token branch from be38179 to 3d2e107 Compare February 16, 2023 15:53

rustbot added the A-interacts-with-crates.io Area: interaction with registries label Feb 16, 2023

Eh2406 approved these changes Feb 16, 2023

View reviewed changes

src/cargo/ops/registry.rs Outdated Show resolved Hide resolved

crates/crates-io/lib.rs Outdated Show resolved Hide resolved

tests/testsuite/login.rs Outdated Show resolved Hide resolved

Address review comments

823ab52

* 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

Eh2406 added the relnotes Release-note worthy label Feb 16, 2023

remove double check

8502fa8

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 16, 2023

bors merged commit a66f123 into rust-lang:master Feb 16, 2023

Akida31 deleted the issue-11571-bad-token branch February 17, 2023 09:45

weihanglo mentioned this pull request Feb 17, 2023

Update cargo rust-lang/rust#108197

Merged

ehuss removed the I-nominated-to-discuss To be discussed during issue triage on the next Cargo team meeting label Feb 21, 2023

ehuss added this to the 1.69.0 milestone Mar 9, 2023

weihanglo added a commit to weihanglo/cargo that referenced this pull request Mar 9, 2023

Bump crates-io to 0.36.0

6321ebb

This was an overlook of rust-lang#11600

weihanglo mentioned this pull request Mar 9, 2023

Bump crates-io to 0.36.0 #11820

Merged

weihanglo added a commit to weihanglo/cargo that referenced this pull request Mar 9, 2023

Bump crates-io to 0.36.0

8296cfe

This was an overlook of rust-lang#11600

This was referenced Apr 9, 2023

Fix credential token format validation. #11951

Merged

Validate token on publish. #11952

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error on invalid alphanumeric token for crates.io #11600

Error on invalid alphanumeric token for crates.io #11600

Akida31 commented Jan 19, 2023

rustbot commented Jan 19, 2023

Eh2406 commented Jan 19, 2023

Akida31 commented Jan 26, 2023

Eh2406 commented Jan 26, 2023

Eh2406 commented Feb 14, 2023

Akida31 commented Feb 16, 2023

Eh2406 left a comment

Akida31 commented Feb 16, 2023

Eh2406 commented Feb 16, 2023

bors commented Feb 16, 2023

bors commented Feb 16, 2023

bors commented Feb 16, 2023

Akida31 commented Feb 17, 2023

weihanglo commented Mar 9, 2023

Error on invalid alphanumeric token for crates.io #11600

Error on invalid alphanumeric token for crates.io #11600

Conversation

Akida31 commented Jan 19, 2023

rustbot commented Jan 19, 2023

Eh2406 commented Jan 19, 2023

Akida31 commented Jan 26, 2023

Eh2406 commented Jan 26, 2023

Eh2406 commented Feb 14, 2023

Akida31 commented Feb 16, 2023

Eh2406 left a comment

Choose a reason for hiding this comment

Akida31 commented Feb 16, 2023

Eh2406 commented Feb 16, 2023

bors commented Feb 16, 2023

bors commented Feb 16, 2023

bors commented Feb 16, 2023

Akida31 commented Feb 17, 2023

weihanglo commented Mar 9, 2023