fix(config): When merging, replace rather than combining specific configuration keys (#15066)

In general, Cargo merges lists in configuration by concatenating them.
However, sometimes the lists don't make sense for merging, such as a
program and its arguments. We had the `UnmergedStringList` type that
handled this case for merging environment variables, but it did not work
for multiple configs.

* Removes the `UnmergedStringList` type, which only worked for
preventing merging of environment variables with configuration.
* Adds a new function `is_nonmergable_list` which hard-codes which
configuration keys contain lists that should not be merged.

Fixes #14906

Author: ehuss

src/cargo/util/context/de.rs

@@ -154,17 +154,13 @@ impl<'de, 'gctx> de::Deserializer<'de> for Deserializer<'gctx> {
     where
         V: de::Visitor<'de>,
     {
-        let merge = if name == "StringList" {
-            true
-        } else if name == "UnmergedStringList" {
-            false
+        if name == "StringList" {
+            let vals = self.gctx.get_list_or_string(&self.key)?;
+            let vals: Vec<String> = vals.into_iter().map(|vd| vd.0).collect();
+            visitor.visit_newtype_struct(vals.into_deserializer())
         } else {
-            return visitor.visit_newtype_struct(self);
-        };
-
-        let vals = self.gctx.get_list_or_string(&self.key, merge)?;
-        let vals: Vec<String> = vals.into_iter().map(|vd| vd.0).collect();
-        visitor.visit_newtype_struct(vals.into_deserializer())
+            visitor.visit_newtype_struct(self)
+        }
     }
 
     fn deserialize_enum<V>(

src/cargo/util/context/key.rs

@@ -93,6 +93,16 @@ impl ConfigKey {
     pub fn is_root(&self) -> bool {
         self.parts.is_empty()
     }
+
+    /// Returns whether or not the given key string matches this key.
+    /// Use * to match any key part.
+    pub fn matches(&self, pattern: &str) -> bool {
+        let mut parts = self.parts();
+        pattern
+            .split('.')
+            .all(|pat| parts.next() == Some(pat) || pat == "*")
+            && parts.next().is_none()
+    }
 }
 
 impl fmt::Display for ConfigKey {

src/cargo/util/context/mod.rs

@@ -736,7 +736,7 @@ impl GlobalContext {
                     Ok(Some(CV::List(cv_list, cv_def)))
                 }
                 Some(cv) => {
-                    // This can't assume StringList or UnmergedStringList.
+                    // This can't assume StringList.
                     // Return an error, which is the behavior of merging
                     // multiple config.toml files with the same scenario.
                     bail!(
@@ -910,21 +910,9 @@ impl GlobalContext {
     }
 
     /// Helper for `StringList` type to get something that is a string or list.
-    fn get_list_or_string(
-        &self,
-        key: &ConfigKey,
-        merge: bool,
-    ) -> CargoResult<Vec<(String, Definition)>> {
+    fn get_list_or_string(&self, key: &ConfigKey) -> CargoResult<Vec<(String, Definition)>> {
         let mut res = Vec::new();
 
-        if !merge {
-            self.get_env_list(key, &mut res)?;
-
-            if !res.is_empty() {
-                return Ok(res);
-            }
-        }
-
         match self.get_cv(key)? {
             Some(CV::List(val, _def)) => res.extend(val),
             Some(CV::String(val, def)) => {
@@ -943,6 +931,7 @@ impl GlobalContext {
     }
 
     /// Internal method for getting an environment variable as a list.
+    /// If the key is a non-mergable list and a value is found in the environment, existing values are cleared.
     fn get_env_list(
         &self,
         key: &ConfigKey,
@@ -953,6 +942,10 @@ impl GlobalContext {
             return Ok(());
         };
 
+        if is_nonmergable_list(&key) {
+            output.clear();
+        }
+
         let def = Definition::Environment(key.as_env_key().to_string());
         if self.cli_unstable().advanced_env && env_val.starts_with('[') && env_val.ends_with(']') {
             // Parse an environment string as a TOML array.
@@ -2227,13 +2220,31 @@ impl ConfigValue {
     ///
     /// Container and non-container types cannot be mixed.
     fn merge(&mut self, from: ConfigValue, force: bool) -> CargoResult<()> {
+        self.merge_helper(from, force, &mut ConfigKey::new())
+    }
+
+    fn merge_helper(
+        &mut self,
+        from: ConfigValue,
+        force: bool,
+        parts: &mut ConfigKey,
+    ) -> CargoResult<()> {
+        let is_higher_priority = from.definition().is_higher_priority(self.definition());
         match (self, from) {
             (&mut CV::List(ref mut old, _), CV::List(ref mut new, _)) => {
-                if force {
-                    old.append(new);
+                if is_nonmergable_list(&parts) {
+                    // Use whichever list is higher priority.
+                    if force || is_higher_priority {
+                        mem::swap(new, old);
+                    }
                 } else {
-                    new.append(old);
-                    mem::swap(new, old);
+                    // Merge the lists together.
+                    if force {
+                        old.append(new);
+                    } else {
+                        new.append(old);
+                        mem::swap(new, old);
+                    }
                 }
                 old.sort_by(|a, b| a.1.cmp(&b.1));
             }
@@ -2243,7 +2254,8 @@ impl ConfigValue {
                         Occupied(mut entry) => {
                             let new_def = value.definition().clone();
                             let entry = entry.get_mut();
-                            entry.merge(value, force).with_context(|| {
+                            parts.push(&key);
+                            entry.merge_helper(value, force, parts).with_context(|| {
                                 format!(
                                     "failed to merge key `{}` between \
                                      {} and {}",
@@ -2273,7 +2285,7 @@ impl ConfigValue {
                 ));
             }
             (old, mut new) => {
-                if force || new.definition().is_higher_priority(old.definition()) {
+                if force || is_higher_priority {
                     mem::swap(old, &mut new);
                 }
             }
@@ -2348,6 +2360,17 @@ impl ConfigValue {
     }
 }
 
+/// List of which configuration lists cannot be merged.
+/// Instead of merging, these the higher priority list replaces the lower priority list.
+fn is_nonmergable_list(key: &ConfigKey) -> bool {
+    key.matches("registry.credential-provider")
+        || key.matches("registries.*.credential-provider")
+        || key.matches("target.*.runner")
+        || key.matches("host.runner")
+        || key.matches("credential-alias.*")
+        || key.matches("doc.browser")
+}
+
 pub fn homedir(cwd: &Path) -> Option<PathBuf> {
     ::home::cargo_home_with_cwd(cwd).ok()
 }
@@ -2916,14 +2939,6 @@ impl StringList {
     }
 }
 
-/// Alternative to [`StringList`] that follows precedence rules, rather than merging config values with environment values,
-///
-/// e.g. a string list found in the environment will be used instead of one in a config file.
-///
-/// This is currently only used by [`PathAndArgs`]
-#[derive(Debug, Deserialize)]
-pub struct UnmergedStringList(Vec<String>);
-
 #[macro_export]
 macro_rules! __shell_print {
     ($config:expr, $which:ident, $newline:literal, $($arg:tt)*) => ({

src/cargo/util/context/path.rs

@@ -1,4 +1,4 @@
-use super::{GlobalContext, UnmergedStringList, Value};
+use super::{GlobalContext, StringList, Value};
 use serde::{de::Error, Deserialize};
 use std::path::PathBuf;
 
@@ -53,6 +53,10 @@ impl ConfigRelativePath {
 ///
 /// Typically you should use `ConfigRelativePath::resolve_program` on the path
 /// to get the actual program.
+///
+/// **Note**: Any usage of this type in config needs to be listed in
+/// the `util::context::is_nonmergable_list` check to prevent list merging
+/// from multiple config files.
 #[derive(Debug, Clone, PartialEq)]
 pub struct PathAndArgs {
     pub path: ConfigRelativePath,
@@ -64,7 +68,7 @@ impl<'de> serde::Deserialize<'de> for PathAndArgs {
     where
         D: serde::Deserializer<'de>,
     {
-        let vsl = Value::<UnmergedStringList>::deserialize(deserializer)?;
+        let vsl = Value::<StringList>::deserialize(deserializer)?;
         let mut strings = vsl.val.0;
         if strings.is_empty() {
             return Err(D::Error::invalid_length(0, &"at least one element"));

tests/testsuite/config.rs

@@ -9,6 +9,7 @@ use std::path::{Path, PathBuf};
 
 use cargo::core::features::{GitFeatures, GitoxideFeatures};
 use cargo::core::{PackageIdSpec, Shell};
+use cargo::util::auth::RegistryConfig;
 use cargo::util::context::{
     self, Definition, GlobalContext, JobsConfig, SslVersionConfig, StringList,
 };
@@ -2164,6 +2165,37 @@ gitoxide = \"fetch\"
     }
 }
 
+#[cargo_test]
+fn nonmergable_lists() {
+    let root_path = paths::root().join(".cargo/config.toml");
+    write_config_at(
+        &root_path,
+        "\
+[registries.example]
+credential-provider = ['a', 'b']
+",
+    );
+
+    let foo_path = paths::root().join("foo/.cargo/config.toml");
+    write_config_at(
+        &foo_path,
+        "\
+[registries.example]
+credential-provider = ['c', 'd']
+",
+    );
+
+    let gctx = GlobalContextBuilder::new().cwd("foo").build();
+    let provider = gctx
+        .get::<Option<RegistryConfig>>(&format!("registries.example"))
+        .unwrap()
+        .unwrap()
+        .credential_provider
+        .unwrap();
+    assert_eq!(provider.path.raw_value(), "c");
+    assert_eq!(provider.args, ["d"]);
+}
+
 #[cargo_test]
 fn build_std() {
     let gctx = GlobalContextBuilder::new()