Auto merge of #10176 - jonhoo:config-cli-simple, r=ehuss

Check --config for dotted keys only

This addresses the remaining unresolved issue for `config-cli` (#7722).

Author: bors

src/cargo/util/config/mod.rs

@@ -79,7 +79,7 @@ use cargo_util::paths;
 use curl::easy::Easy;
 use lazycell::LazyCell;
 use serde::Deserialize;
-use toml_edit::easy as toml;
+use toml_edit::{easy as toml, Item};
 use url::Url;
 
 mod de;
@@ -1176,28 +1176,90 @@ impl Config {
                 map.insert("include".to_string(), value);
                 CV::Table(map, Definition::Cli)
             } else {
-                // TODO: This should probably use a more narrow parser, reject
-                // comments, blank lines, [headers], etc.
-                let toml_v: toml::Value = toml::de::from_str(arg)
-                    .with_context(|| format!("failed to parse --config argument `{}`", arg))?;
-                let toml_table = toml_v.as_table().unwrap();
-                if toml_table.len() != 1 {
+                // We only want to allow "dotted key" (see https://toml.io/en/v1.0.0#keys)
+                // expressions followed by a value that's not an "inline table"
+                // (https://toml.io/en/v1.0.0#inline-table). Easiest way to check for that is to
+                // parse the value as a toml_edit::Document, and check that the (single)
+                // inner-most table is set via dotted keys.
+                let doc: toml_edit::Document = arg.parse().with_context(|| {
+                    format!("failed to parse value from --config argument `{arg}` as a dotted key expression")
+                })?;
+                fn non_empty_decor(d: &toml_edit::Decor) -> bool {
+                    d.prefix().map_or(false, |p| !p.trim().is_empty())
+                        || d.suffix().map_or(false, |s| !s.trim().is_empty())
+                }
+                let ok = {
+                    let mut got_to_value = false;
+                    let mut table = doc.as_table();
+                    let mut is_root = true;
+                    while table.is_dotted() || is_root {
+                        is_root = false;
+                        if table.len() != 1 {
+                            break;
+                        }
+                        let (k, n) = table.iter().next().expect("len() == 1 above");
+                        match n {
+                            Item::Table(nt) => {
+                                if table.key_decor(k).map_or(false, non_empty_decor)
+                                    || non_empty_decor(nt.decor())
+                                {
+                                    bail!(
+                                        "--config argument `{arg}` \
+                                            includes non-whitespace decoration"
+                                    )
+                                }
+                                table = nt;
+                            }
+                            Item::Value(v) if v.is_inline_table() => {
+                                bail!(
+                                    "--config argument `{arg}` \
+                                    sets a value to an inline table, which is not accepted"
+                                );
+                            }
+                            Item::Value(v) => {
+                                if non_empty_decor(v.decor()) {
+                                    bail!(
+                                        "--config argument `{arg}` \
+                                            includes non-whitespace decoration"
+                                    )
+                                }
+                                got_to_value = true;
+                                break;
+                            }
+                            Item::ArrayOfTables(_) => {
+                                bail!(
+                                    "--config argument `{arg}` \
+                                    sets a value to an array of tables, which is not accepted"
+                                );
+                            }
+
+                            Item::None => {
+                                bail!("--config argument `{arg}` doesn't provide a value")
+                            }
+                        }
+                    }
+                    got_to_value
+                };
+                if !ok {
                     bail!(
-                        "--config argument `{}` expected exactly one key=value pair, got {} keys",
-                        arg,
-                        toml_table.len()
+                        "--config argument `{arg}` was not a TOML dotted key expression (such as `build.jobs = 2`)"
                     );
                 }
+
+                let toml_v = toml::from_document(doc).with_context(|| {
+                    format!("failed to parse value from --config argument `{arg}`")
+                })?;
+
                 CV::from_toml(Definition::Cli, toml_v)
-                    .with_context(|| format!("failed to convert --config argument `{}`", arg))?
+                    .with_context(|| format!("failed to convert --config argument `{arg}`"))?
             };
             let mut seen = HashSet::new();
             let tmp_table = self
                 .load_includes(tmp_table, &mut seen)
                 .with_context(|| "failed to load --config include".to_string())?;
             loaded_args
                 .merge(tmp_table, true)
-                .with_context(|| format!("failed to merge --config argument `{}`", arg))?;
+                .with_context(|| format!("failed to merge --config argument `{arg}`"))?;
         }
         Ok(loaded_args)
     }

tests/testsuite/config_cli.rs

@@ -3,7 +3,7 @@
 use super::config::{assert_error, assert_match, read_output, write_config, ConfigBuilder};
 use cargo::util::config::Definition;
 use cargo_test_support::{paths, project};
-use std::fs;
+use std::{collections::HashMap, fs};
 
 #[cargo_test]
 fn config_gated() {
@@ -234,11 +234,64 @@ fn merge_array_mixed_def_paths() {
 }
 
 #[cargo_test]
-fn unused_key() {
-    // Unused key passed on command line.
+fn enforces_format() {
+    // These dotted key expressions should all be fine.
     let config = ConfigBuilder::new()
-        .config_arg("build={jobs=1, unused=2}")
+        .config_arg("a=true")
+        .config_arg(" b.a = true ")
+        .config_arg("c.\"b\".'a'=true")
+        .config_arg("d.\"=\".'='=true")
+        .config_arg("e.\"'\".'\"'=true")
         .build();
+    assert_eq!(config.get::<bool>("a").unwrap(), true);
+    assert_eq!(
+        config.get::<HashMap<String, bool>>("b").unwrap(),
+        HashMap::from([("a".to_string(), true)])
+    );
+    assert_eq!(
+        config
+            .get::<HashMap<String, HashMap<String, bool>>>("c")
+            .unwrap(),
+        HashMap::from([("b".to_string(), HashMap::from([("a".to_string(), true)]))])
+    );
+    assert_eq!(
+        config
+            .get::<HashMap<String, HashMap<String, bool>>>("d")
+            .unwrap(),
+        HashMap::from([("=".to_string(), HashMap::from([("=".to_string(), true)]))])
+    );
+    assert_eq!(
+        config
+            .get::<HashMap<String, HashMap<String, bool>>>("e")
+            .unwrap(),
+        HashMap::from([("'".to_string(), HashMap::from([("\"".to_string(), true)]))])
+    );
+
+    // But anything that's not a dotted key expression should be disallowed.
+    let _ = ConfigBuilder::new()
+        .config_arg("[a] foo=true")
+        .build_err()
+        .unwrap_err();
+    let _ = ConfigBuilder::new()
+        .config_arg("a = true\nb = true")
+        .build_err()
+        .unwrap_err();
+
+    // We also disallow overwriting with tables since it makes merging unclear.
+    let _ = ConfigBuilder::new()
+        .config_arg("a = { first = true, second = false }")
+        .build_err()
+        .unwrap_err();
+    let _ = ConfigBuilder::new()
+        .config_arg("a = { first = true }")
+        .build_err()
+        .unwrap_err();
+}
+
+#[cargo_test]
+fn unused_key() {
+    // Unused key passed on command line.
+    let config = ConfigBuilder::new().config_arg("build.unused = 2").build();
 
     config.build_config().unwrap();
     let output = read_output(config);
@@ -284,7 +337,7 @@ fn bad_parse() {
     assert_error(
         config.unwrap_err(),
         "\
-failed to parse --config argument `abc`
+failed to parse value from --config argument `abc` as a dotted key expression
 
 Caused by:
   TOML parse error at line 1, column 4
@@ -295,6 +348,12 @@ Unexpected end of input
 Expected `.` or `=`
 ",
     );
+
+    let config = ConfigBuilder::new().config_arg("").build_err();
+    assert_error(
+        config.unwrap_err(),
+        "--config argument `` was not a TOML dotted key expression (such as `build.jobs = 2`)",
+    );
 }
 
 #[cargo_test]
@@ -305,14 +364,55 @@ fn too_many_values() {
         config.unwrap_err(),
         "\
 --config argument `a=1
-b=2` expected exactly one key=value pair, got 2 keys",
+b=2` was not a TOML dotted key expression (such as `build.jobs = 2`)",
     );
+}
 
-    let config = ConfigBuilder::new().config_arg("").build_err();
+#[cargo_test]
+fn no_inline_table_value() {
+    // Disallow inline tables
+    let config = ConfigBuilder::new()
+        .config_arg("a.b={c = \"d\"}")
+        .build_err();
+    assert_error(
+        config.unwrap_err(),
+        "--config argument `a.b={c = \"d\"}` sets a value to an inline table, which is not accepted"
+    );
+}
+
+#[cargo_test]
+fn no_array_of_tables_values() {
+    // Disallow array-of-tables when not in dotted form
+    let config = ConfigBuilder::new()
+        .config_arg("[[a.b]]\nc = \"d\"")
+        .build_err();
+    assert_error(
+        config.unwrap_err(),
+        "\
+--config argument `[[a.b]]
+c = \"d\"` was not a TOML dotted key expression (such as `build.jobs = 2`)",
+    );
+}
+
+#[cargo_test]
+fn no_comments() {
+    // Disallow comments in dotted form.
+    let config = ConfigBuilder::new()
+        .config_arg("a.b = \"c\" # exactly")
+        .build_err();
+    assert_error(
+        config.unwrap_err(),
+        "\
+--config argument `a.b = \"c\" # exactly` includes non-whitespace decoration",
+    );
+
+    let config = ConfigBuilder::new()
+        .config_arg("# exactly\na.b = \"c\"")
+        .build_err();
     assert_error(
         config.unwrap_err(),
         "\
-         --config argument `` expected exactly one key=value pair, got 0 keys",
+--config argument `# exactly\na.b = \"c\"` includes non-whitespace decoration",
     );
 }
 

tests/testsuite/config_include.rs

@@ -274,7 +274,7 @@ fn cli_path() {
     assert_error(
         config.unwrap_err(),
         "\
-failed to parse --config argument `missing.toml`
+failed to parse value from --config argument `missing.toml` as a dotted key expression
 
 Caused by:
   TOML parse error at line 1, column 13