From 6ca1aa7307311173fde7aab84b3ea873000e8cde Mon Sep 17 00:00:00 2001 From: Rain Date: Fri, 12 Aug 2022 13:28:25 -0700 Subject: [PATCH] [nextest-runner] enforce that --config only accepts key-value pairs Port over /~https://github.com/rust-lang/cargo/pull/10176 to nextest. --- cargo-nextest/src/cargo_cli.rs | 2 +- nextest-runner/src/cargo_config.rs | 174 +++++++++++++++++++++++++++-- nextest-runner/src/errors.rs | 53 ++++++++- 3 files changed, 219 insertions(+), 10 deletions(-) diff --git a/cargo-nextest/src/cargo_cli.rs b/cargo-nextest/src/cargo_cli.rs index 05cfe1076f1..f88d653b764 100644 --- a/cargo-nextest/src/cargo_cli.rs +++ b/cargo-nextest/src/cargo_cli.rs @@ -127,7 +127,7 @@ pub(crate) struct CargoOptions { // NOTE: this does not conflict with reuse build opts since we let target.runner be specified // this way - /// Override a configuration value (unstable) + /// Override a configuration value #[clap(long, value_name = "KEY=VALUE")] pub(crate) config: Vec, diff --git a/nextest-runner/src/cargo_config.rs b/nextest-runner/src/cargo_config.rs index e863c0d8dab..1fc3f6d6b13 100644 --- a/nextest-runner/src/cargo_config.rs +++ b/nextest-runner/src/cargo_config.rs @@ -6,11 +6,15 @@ //! Since `cargo config get` is not stable as of Rust 1.61, nextest must do its own config file //! search. -use crate::errors::{CargoConfigSearchError, CargoConfigsConstructError, TargetTripleError}; +use crate::errors::{ + CargoConfigSearchError, CargoConfigsConstructError, InvalidCargoCliConfigReason, + TargetTripleError, +}; use camino::{Utf8Path, Utf8PathBuf}; use once_cell::sync::OnceCell; use serde::Deserialize; use std::{collections::BTreeMap, fmt}; +use toml_edit::Item; /// Represents a target triple that's being cross-compiled against. #[derive(Clone, Debug, PartialEq, Eq)] @@ -221,17 +225,103 @@ fn parse_cli_configs( .map(|config_str| { // Each cargo config is expected to be a valid TOML file. let config_str = config_str.as_ref(); - let config = toml_edit::easy::from_str(config_str).map_err(|error| { - CargoConfigsConstructError::CliConfigParseError { - config_str: config_str.to_owned(), - error, - } - })?; + let config = parse_cli_config(config_str)?; Ok((CargoConfigSource::CliOption, config)) }) .collect() } +fn parse_cli_config(config_str: &str) -> Result { + // This implementation is copied over from /~https://github.com/rust-lang/cargo/pull/10176. + + // 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 = + config_str + .parse() + .map_err(|error| CargoConfigsConstructError::CliConfigParseError { + config_str: config_str.to_owned(), + error, + })?; + + 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()) + { + return Err(CargoConfigsConstructError::InvalidCliConfig { + config_str: config_str.to_owned(), + reason: InvalidCargoCliConfigReason::IncludesNonWhitespaceDecoration, + })?; + } + table = nt; + } + Item::Value(v) if v.is_inline_table() => { + return Err(CargoConfigsConstructError::InvalidCliConfig { + config_str: config_str.to_owned(), + reason: InvalidCargoCliConfigReason::SetsValueToInlineTable, + })?; + } + Item::Value(v) => { + if non_empty_decor(v.decor()) { + return Err(CargoConfigsConstructError::InvalidCliConfig { + config_str: config_str.to_owned(), + reason: InvalidCargoCliConfigReason::IncludesNonWhitespaceDecoration, + })?; + } + got_to_value = true; + break; + } + Item::ArrayOfTables(_) => { + return Err(CargoConfigsConstructError::InvalidCliConfig { + config_str: config_str.to_owned(), + reason: InvalidCargoCliConfigReason::SetsValueToArrayOfTables, + })?; + } + Item::None => { + return Err(CargoConfigsConstructError::InvalidCliConfig { + config_str: config_str.to_owned(), + reason: InvalidCargoCliConfigReason::DoesntProvideValue, + })?; + } + } + } + got_to_value + }; + if !ok { + return Err(CargoConfigsConstructError::InvalidCliConfig { + config_str: config_str.to_owned(), + reason: InvalidCargoCliConfigReason::NotDottedKv, + })?; + } + + let cargo_config: CargoConfig = toml_edit::easy::from_document(doc).map_err(|error| { + CargoConfigsConstructError::CliConfigDeError { + config_str: config_str.to_owned(), + error, + } + })?; + Ok(cargo_config) +} + fn discover_impl( start_search_at: &Utf8Path, terminate_search_at: Option<&Utf8Path>, @@ -342,7 +432,7 @@ pub(crate) struct CargoConfigRunner { pub(crate) runner: Option, } -#[derive(Clone, Deserialize, Debug)] +#[derive(Clone, Deserialize, Debug, Eq, PartialEq)] #[serde(untagged)] pub(crate) enum Runner { Simple(String), @@ -355,6 +445,74 @@ mod tests { use camino::Utf8Path; use color_eyre::eyre::{Context, Result}; use tempfile::TempDir; + use test_case::test_case; + + #[test] + fn test_cli_kv_accepted() { + // These dotted key expressions should all be fine. + let config = parse_cli_config("build.target=\"aarch64-unknown-linux-gnu\"") + .expect("dotted config should parse correctly"); + assert_eq!( + config.build.target.as_deref(), + Some("aarch64-unknown-linux-gnu") + ); + + let config = parse_cli_config(" target.\"aarch64-unknown-linux-gnu\".runner = 'test' ") + .expect("dotted config should parse correctly"); + assert_eq!( + config.target.as_ref().unwrap()["aarch64-unknown-linux-gnu"].runner, + Some(Runner::Simple("test".to_owned())) + ); + + // But anything that's not a dotted key expression should be disallowed. + let _ = parse_cli_config("[a] foo=true").unwrap_err(); + let _ = parse_cli_config("a = true\nb = true").unwrap_err(); + + // We also disallow overwriting with tables since it makes merging unclear. + let _ = parse_cli_config("a = { first = true, second = false }").unwrap_err(); + let _ = parse_cli_config("a = { first = true }").unwrap_err(); + } + + #[test_case( + "", + InvalidCargoCliConfigReason::NotDottedKv + + ; "empty input")] + #[test_case( + "a.b={c = \"d\"}", + InvalidCargoCliConfigReason::SetsValueToInlineTable + + ; "no inline table value")] + #[test_case( + "[[a.b]]\nc = \"d\"", + InvalidCargoCliConfigReason::NotDottedKv + + ; "no array of tables")] + #[test_case( + "a.b = \"c\" # exactly", + InvalidCargoCliConfigReason::IncludesNonWhitespaceDecoration + + ; "no comments after")] + #[test_case( + "# exactly\na.b = \"c\"", + InvalidCargoCliConfigReason::IncludesNonWhitespaceDecoration + + ; "no comments before")] + fn test_invalid_cli_config_reason(arg: &str, expected_reason: InvalidCargoCliConfigReason) { + // Disallow inline tables + let err = parse_cli_config(arg).unwrap_err(); + let actual_reason = match err { + CargoConfigsConstructError::InvalidCliConfig { reason, .. } => reason, + other => panic!( + "expected input {arg} to fail with InvalidCliConfig, actual failure: {other}" + ), + }; + + assert_eq!( + expected_reason, actual_reason, + "expected reason for failure doesn't match actual reason" + ); + } #[test] fn test_find_target_triple() { diff --git a/nextest-runner/src/errors.rs b/nextest-runner/src/errors.rs index 7e77ad8ceeb..6c1c18cb00f 100644 --- a/nextest-runner/src/errors.rs +++ b/nextest-runner/src/errors.rs @@ -797,15 +797,66 @@ pub enum CargoConfigsConstructError { CurrentDirInvalidUtf8(#[source] FromPathBufError), /// Parsing a CLI config option failed. - #[error("failed to parse --config option `{config_str}` as TOML")] + #[error("failed to parse --config argument `{config_str}` as TOML")] CliConfigParseError { /// The CLI config option. config_str: String, + /// The error that occurred trying to parse the config. + #[source] + error: toml_edit::TomlError, + }, + + /// Deserializing a CLI config option into domain types failed. + #[error("failed to deserialize --config argument `{config_str}` as TOML")] + CliConfigDeError { + /// The CLI config option. + config_str: String, + /// The error that occurred trying to deserialize the config. #[source] error: toml_edit::easy::de::Error, }, + + /// A CLI config option is not in the dotted key format. + #[error( + "invalid format for --config argument `{config_str}` (should be a dotted key expression)" + )] + InvalidCliConfig { + /// The CLI config option. + config_str: String, + + /// The reason why this Cargo CLI config is invalid. + #[source] + reason: InvalidCargoCliConfigReason, + }, +} + +/// The reason an invalid CLI config failed. +/// +/// Part of [`CargoConfigsConstructError::InvalidCliConfig`]. +#[derive(Copy, Clone, Debug, Error, Eq, PartialEq)] +#[non_exhaustive] +pub enum InvalidCargoCliConfigReason { + /// The argument is not a TOML dotted key expression. + #[error("was not a TOML dotted key expression (such as `build.jobs = 2`)")] + NotDottedKv, + + /// The argument includes non-whitespace decoration. + #[error("includes non-whitespace decoration")] + IncludesNonWhitespaceDecoration, + + /// The argument sets a value to an inline table. + #[error("sets a value to an inline table, which is not accepted")] + SetsValueToInlineTable, + + /// The argument sets a value to an array of tables. + #[error("sets a value to an array of tables, which is not accepted")] + SetsValueToArrayOfTables, + + /// The argument doesn't provide a value. + #[error("doesn't provide a value")] + DoesntProvideValue, } /// An error occurred while looking for Cargo configuration files.