From 3d2e107b5a04d07494bdd27515fea5199a0e4057 Mon Sep 17 00:00:00 2001 From: akida31 Date: Thu, 16 Feb 2023 16:42:30 +0100 Subject: [PATCH 1/3] Error on invalid token for registry auth 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. --- crates/crates-io/lib.rs | 21 +++++++++++++++++++++ src/cargo/ops/registry.rs | 1 + tests/testsuite/login.rs | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+) diff --git a/crates/crates-io/lib.rs b/crates/crates-io/lib.rs index 0e48b1ca5ad..391a06d346f 100644 --- a/crates/crates-io/lib.rs +++ b/crates/crates-io/lib.rs @@ -394,6 +394,7 @@ impl Registry { Some(s) => s, None => bail!("no upload token found, please run `cargo login`"), }; + check_token(token)?; headers.append(&format!("Authorization: {}", token))?; } self.handle.http_headers(headers)?; @@ -510,3 +511,23 @@ pub fn is_url_crates_io(url: &str) -> bool { .map(|u| u.host_str() == Some("crates.io")) .unwrap_or(false) } + +/// Checks if a token is valid or malformed. +/// +/// This check is necessary to prevent sending tokens which create an invalid HTTP request. +/// It would be easier to check just for alphanumeric tokens, but we can't be sure that all +/// registries only create tokens in that format so that is as less restricted as possible. +pub fn check_token(token: &str) -> Result<()> { + let is_valid = token.bytes().all(|b| { + 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) + }); + + if is_valid { + Ok(()) + } else { + Err(anyhow::anyhow!("invalid token.")) + } +} diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 0584a771457..548d2c34eb2 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -901,6 +901,7 @@ pub fn registry_login( if tok.is_empty() { bail!("please provide a non-empty token"); } + crates_io::check_token(tok.as_ref().expose())?; } } if ®_cfg == &new_token { diff --git a/tests/testsuite/login.rs b/tests/testsuite/login.rs index 2757e2ad4f4..e8943b5c04d 100644 --- a/tests/testsuite/login.rs +++ b/tests/testsuite/login.rs @@ -126,6 +126,38 @@ fn empty_login_token() { .run(); } +#[cargo_test] +fn invalid_login_token() { + let registry = RegistryBuilder::new() + .no_configure_registry() + .no_configure_token() + .build(); + setup_new_credentials(); + + let check_ = |stdin: &str, stderr: &str| { + cargo_process("login") + .replace_crates_io(registry.index_url()) + .with_stdout("please paste the token found on [..]/me below") + .with_stdin(stdin) + .with_stderr(stderr) + .with_status(101) + .run(); + }; + let check = |stdin: &str| { + check_(stdin, "[ERROR] invalid token."); + }; + // first check updates index so it must be handled differently + check_( + "😄", + "\ +[UPDATING] crates.io index +[ERROR] invalid token.", + ); + check("\u{0016}"); + check("\u{0000}"); + check("你好"); +} + #[cargo_test] fn bad_asymmetric_token_args() { // These cases are kept brief as the implementation is covered by clap, so this is only smoke testing that we have clap configured correctly. From 823ab52f19ca3106f7f08e67755ea67c64aa2411 Mon Sep 17 00:00:00 2001 From: akida31 Date: Thu, 16 Feb 2023 19:12:46 +0100 Subject: [PATCH 2/3] Address review comments * 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 --- crates/crates-io/lib.rs | 14 +++++++++----- tests/testsuite/login.rs | 33 +++++++++++++++++++++++---------- 2 files changed, 32 insertions(+), 15 deletions(-) diff --git a/crates/crates-io/lib.rs b/crates/crates-io/lib.rs index 391a06d346f..ad3ea76763d 100644 --- a/crates/crates-io/lib.rs +++ b/crates/crates-io/lib.rs @@ -518,16 +518,20 @@ pub fn is_url_crates_io(url: &str) -> bool { /// It would be easier to check just for alphanumeric tokens, but we can't be sure that all /// registries only create tokens in that format so that is as less restricted as possible. pub fn check_token(token: &str) -> Result<()> { - let is_valid = token.bytes().all(|b| { + if token.is_empty() { + bail!("please provide a non-empty token"); + } + if token.bytes().all(|b| { 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) - }); - - if is_valid { + }) { Ok(()) } else { - Err(anyhow::anyhow!("invalid token.")) + Err(anyhow::anyhow!( + "token contains invalid characters.\nOnly printable ISO-8859-1 characters \ + are allowed as it is sent in a HTTPS header." + )) } } diff --git a/tests/testsuite/login.rs b/tests/testsuite/login.rs index e8943b5c04d..19387aed57c 100644 --- a/tests/testsuite/login.rs +++ b/tests/testsuite/login.rs @@ -134,7 +134,7 @@ fn invalid_login_token() { .build(); setup_new_credentials(); - let check_ = |stdin: &str, stderr: &str| { + let check = |stdin: &str, stderr: &str| { cargo_process("login") .replace_crates_io(registry.index_url()) .with_stdout("please paste the token found on [..]/me below") @@ -143,19 +143,32 @@ fn invalid_login_token() { .with_status(101) .run(); }; - let check = |stdin: &str| { - check_(stdin, "[ERROR] invalid token."); - }; - // first check updates index so it must be handled differently - check_( + + check( "😄", "\ [UPDATING] crates.io index -[ERROR] invalid token.", +[ERROR] token contains invalid characters. +Only printable ISO-8859-1 characters are allowed as it is sent in a HTTPS header.", + ); + check( + "\u{0016}", + "\ +[ERROR] token contains invalid characters. +Only printable ISO-8859-1 characters are allowed as it is sent in a HTTPS header.", + ); + check( + "\u{0000}", + "\ +[ERROR] token contains invalid characters. +Only printable ISO-8859-1 characters are allowed as it is sent in a HTTPS header.", + ); + check( + "你好", + "\ +[ERROR] token contains invalid characters. +Only printable ISO-8859-1 characters are allowed as it is sent in a HTTPS header.", ); - check("\u{0016}"); - check("\u{0000}"); - check("你好"); } #[cargo_test] From 8502fa85689015234c9d5c84e3957ff32f03e072 Mon Sep 17 00:00:00 2001 From: akida31 Date: Thu, 16 Feb 2023 20:28:02 +0100 Subject: [PATCH 3/3] remove double check --- src/cargo/ops/registry.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 548d2c34eb2..a94617511fa 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -898,9 +898,6 @@ pub fn registry_login( }); if let Some(tok) = new_token.as_token() { - if tok.is_empty() { - bail!("please provide a non-empty token"); - } crates_io::check_token(tok.as_ref().expose())?; } }