Auto merge of #11600 - Akida31:issue-11571-bad-token, r=Eh2406

bors · bors · commit a66f1233f9ad · 2023-02-16T19:46:40.000Z
Error on invalid alphanumeric token for crates.io 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](https://github.com/rust-lang/crates.io/blob/7ea41e9d345f05566ee776b7cbb62e46ccf6b393/src/util/token.rs#L15). 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](https://www.rfc-editor.org/rfc/rfc7230#section-3.2.4) that only the ISO-8859-1 charset is allowed and everything else must be [encoded](https://www.rfc-editor.org/rfc/rfc7230#section-3.2.4) 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)
diff --git 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,27 @@ 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<()> {
+    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)
+    }) {
+        Ok(())
+    } else {
+        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/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs
@@ -898,9 +898,7 @@ 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())?;
         }
     }
     if &reg_cfg == &new_token {
diff --git a/tests/testsuite/login.rs b/tests/testsuite/login.rs
@@ -126,6 +126,51 @@ 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();
+    };
+
+    check(
+        "😄",
+        "\
+[UPDATING] crates.io index
+[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.",
+    );
+}
+
 #[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.

Original file line number	Diff line number	Diff line change
`@@ -898,9 +898,7 @@ pub fn registry_login(`
`898`	`898`	`});`
`899`	`899`
`900`	`900`	`if let Some(tok) = new_token.as_token() {`
`901`		`- if tok.is_empty() {`
`902`		`- bail!("please provide a non-empty token");`
`903`		`- }`
	`901`	`+ crates_io::check_token(tok.as_ref().expose())?;`
`904`	`902`	`}`
`905`	`903`	`}`
`906`	`904`	`if &reg_cfg == &new_token {`