Auto merge of #11318 - RyanAD:3736-improve-invalid-char-error, r=weihanglo

Add warning when PATH env separator is in project path

Closes #3736

Adds a check during `cargo init` and `cargo new` to check if the path contains an invalid PATH character (`:`, `;`, or `"`). These characters can break things in various ways (including `cargo test`). These characters are not valid in certain environment variables and cannot be escaped.

For more information see:
https://github.com/rust-lang/rust/blob/7feb003882ecf7699e6705b537673e20985accff/library/std/src/sys/unix/os.rs#L233
https://man7.org/linux/man-pages/man8/ld.so.8.html
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html
https://doc.rust-lang.org/std/env/fn.join_paths.html#errors

To test cargo new and cargo init:
`cargo new --name testing test:ing`
`mkdir test:ing2 && cd 'test:ing2' && cargo init --name testing`

To test the updated error message when in a directory with invalid characters:
`cargo new testing3 && mv testing3 test:ing3 && cd 'test:ing3' && cargo test`

cc `@weihanglo`

Author: bors

crates/cargo-util/src/paths.rs

@@ -18,18 +18,19 @@ use tempfile::Builder as TempFileBuilder;
 /// environment variable this is will be used for, which is included in the
 /// error message.
 pub fn join_paths<T: AsRef<OsStr>>(paths: &[T], env: &str) -> Result<OsString> {
-    env::join_paths(paths.iter())
-        .with_context(|| {
-            let paths = paths.iter().map(Path::new).collect::<Vec<_>>();
-            format!("failed to join path array: {:?}", paths)
-        })
-        .with_context(|| {
-            format!(
-                "failed to join search paths together\n\
-                     Does ${} have an unterminated quote character?",
-                env
-            )
-        })
+    env::join_paths(paths.iter()).with_context(|| {
+        let mut message = format!(
+            "failed to join paths from `${env}` together\n\n\
+             Check if any of path segments listed below contain an \
+             unterminated quote character or path separator:"
+        );
+        for path in paths {
+            use std::fmt::Write;
+            write!(&mut message, "\n    {:?}", Path::new(path)).unwrap();
+        }
+
+        message
+    })
 }
 
 /// Returns the name of the environment variable used for searching for
@@ -743,3 +744,44 @@ fn exclude_from_time_machine(path: &Path) {
     // Errors are ignored, since it's an optional feature and failure
     // doesn't prevent Cargo from working
 }
+
+#[cfg(test)]
+mod tests {
+    use super::join_paths;
+
+    #[test]
+    fn join_paths_lists_paths_on_error() {
+        let valid_paths = vec!["/testing/one", "/testing/two"];
+        // does not fail on valid input
+        let _joined = join_paths(&valid_paths, "TESTING1").unwrap();
+
+        #[cfg(unix)]
+        {
+            let invalid_paths = vec!["/testing/one", "/testing/t:wo/three"];
+            let err = join_paths(&invalid_paths, "TESTING2").unwrap_err();
+            assert_eq!(
+                err.to_string(),
+                "failed to join paths from `$TESTING2` together\n\n\
+             Check if any of path segments listed below contain an \
+             unterminated quote character or path separator:\
+             \n    \"/testing/one\"\
+             \n    \"/testing/t:wo/three\"\
+             "
+            );
+        }
+        #[cfg(windows)]
+        {
+            let invalid_paths = vec!["/testing/one", "/testing/t\"wo/three"];
+            let err = join_paths(&invalid_paths, "TESTING2").unwrap_err();
+            assert_eq!(
+                err.to_string(),
+                "failed to join paths from `$TESTING2` together\n\n\
+             Check if any of path segments listed below contain an \
+             unterminated quote character or path separator:\
+             \n    \"/testing/one\"\
+             \n    \"/testing/t\\\"wo/three\"\
+             "
+            );
+        }
+    }
+}

src/cargo/ops/cargo_new.rs

@@ -7,10 +7,11 @@ use cargo_util::paths;
 use serde::de;
 use serde::Deserialize;
 use std::collections::BTreeMap;
-use std::fmt;
+use std::ffi::OsStr;
 use std::io::{BufRead, BufReader, ErrorKind};
 use std::path::{Path, PathBuf};
 use std::str::FromStr;
+use std::{fmt, slice};
 use toml_edit::easy as toml;
 
 #[derive(Clone, Copy, Debug, PartialEq)]
@@ -261,6 +262,19 @@ fn check_name(
     Ok(())
 }
 
+/// Checks if the path contains any invalid PATH env characters.
+fn check_path(path: &Path, shell: &mut Shell) -> CargoResult<()> {
+    // warn if the path contains characters that will break `env::join_paths`
+    if let Err(_) = paths::join_paths(slice::from_ref(&OsStr::new(path)), "") {
+        let path = path.to_string_lossy();
+        shell.warn(format!(
+            "the path `{path}` contains invalid PATH characters (usually `:`, `;`, or `\"`)\n\
+            It is recommended to use a different name to avoid problems."
+        ))?;
+    }
+    Ok(())
+}
+
 fn detect_source_paths_and_types(
     package_path: &Path,
     package_name: &str,
@@ -421,6 +435,8 @@ pub fn new(opts: &NewOptions, config: &Config) -> CargoResult<()> {
         )
     }
 
+    check_path(path, &mut config.shell())?;
+
     let is_bin = opts.kind.is_bin();
 
     let name = get_name(path, opts)?;
@@ -458,6 +474,8 @@ pub fn init(opts: &NewOptions, config: &Config) -> CargoResult<NewProjectKind> {
         anyhow::bail!("`cargo init` cannot be run on existing Cargo packages")
     }
 
+    check_path(path, &mut config.shell())?;
+
     let name = get_name(path, opts)?;
 
     let mut src_paths_types = vec![];

tests/testsuite/init/mod.rs

@@ -28,6 +28,8 @@ mod mercurial_autodetect;
 mod multibin_project_name_clash;
 #[cfg(not(windows))]
 mod no_filename;
+#[cfg(unix)]
+mod path_contains_separator;
 mod pijul_autodetect;
 mod reserved_name;
 mod simple_bin;

tests/testsuite/init/path_contains_separator/in/.keep

@@ -0,0 +1,26 @@
+use cargo_test_support::compare::assert_ui;
+use cargo_test_support::prelude::*;
+use cargo_test_support::{t, Project};
+
+use cargo_test_support::curr_dir;
+
+#[cargo_test]
+fn path_contains_separator() {
+    let project = Project::from_template(curr_dir!().join("in"));
+    let project_root = &project.root().join("test:ing");
+
+    if !project_root.exists() {
+        t!(std::fs::create_dir(&project_root));
+    }
+
+    snapbox::cmd::Command::cargo_ui()
+        .arg_line("init --bin --vcs none --edition 2015 --name testing")
+        .current_dir(project_root)
+        .assert()
+        .success()
+        .stdout_matches_path(curr_dir!().join("stdout.log"))
+        .stderr_matches_path(curr_dir!().join("stderr.log"));
+
+    assert_ui().subset_matches(curr_dir!().join("out"), project_root);
+    assert!(!project_root.join(".gitignore").is_file());
+}

tests/testsuite/init/path_contains_separator/mod.rs

@@ -0,0 +1,8 @@
+[package]
+name = "testing"
+version = "0.1.0"
+edition = "2015"
+
+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+
+[dependencies]

tests/testsuite/init/path_contains_separator/out/Cargo.toml

@@ -0,0 +1,3 @@
+fn main() {
+    println!("Hello, world!");
+}

tests/testsuite/init/path_contains_separator/out/src/main.rs

@@ -0,0 +1,3 @@
+warning: the path `[ROOT]/case/test:ing/.` contains invalid PATH characters (usually `:`, `;`, or `"`)
+It is recommended to use a different name to avoid problems.
+     Created binary (application) package

tests/testsuite/init/path_contains_separator/stderr.log

@@ -529,3 +529,17 @@ Caused by:
         )
         .run();
 }
+
+#[cfg(unix)]
+#[cargo_test]
+fn path_with_invalid_character() {
+    cargo_process("new --name testing test:ing")
+        .with_stderr(
+            "\
+[WARNING] the path `[CWD]/test:ing` contains invalid PATH characters (usually `:`, `;`, or `\"`)
+It is recommended to use a different name to avoid problems.
+[CREATED] binary (application) `testing` package
+",
+        )
+        .run();
+}