From 26a8c189d84b1a96e23de9a6bf0cdd614370ef07 Mon Sep 17 00:00:00 2001
From: Pierre Chevalier <pierrechevalier83@gmail.com>
Date: Tue, 29 Nov 2022 22:24:17 +0000
Subject: [PATCH] fix: don't bypass conflicts when using `requires`

The intent for `is_missing_required_ok` is to allow a situation where
two args are required, but they're part of the same group, or one
overrides the other, which is equivalent, so having either of them is
good enough.

The way it was implemented, by reusing `gather_conflicts` introduced a
loophole where `required` could be bypassed when it shouldn't.

Fixes #4520
---
 src/parser/validator.rs | 50 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/src/parser/validator.rs b/src/parser/validator.rs
index 0d97c2f63786..3cfbac905e2d 100644
--- a/src/parser/validator.rs
+++ b/src/parser/validator.rs
@@ -381,8 +381,8 @@ impl<'cmd> Validator<'cmd> {
         conflicts: &mut Conflicts,
     ) -> bool {
         debug!("Validator::is_missing_required_ok: {}", a.get_id());
-        let conflicts = conflicts.gather_conflicts(self.cmd, matcher, a.get_id());
-        !conflicts.is_empty()
+        let overrides = conflicts.gather_overrides(self.cmd, matcher, a.get_id());
+        !overrides.is_empty()
     }
 
     // Failing a required unless means, the arg's "unless" wasn't present, and neither were they
@@ -530,6 +530,52 @@ impl Conflicts {
             conf
         })
     }
+
+    fn gather_overrides(&mut self, cmd: &Command, matcher: &ArgMatcher, arg_id: &Id) -> Vec<Id> {
+        debug!("Conflicts::gather_overrides: arg={:?}", arg_id);
+        let mut overrides = Vec::new();
+        for other_arg_id in matcher
+            .arg_ids()
+            .filter(|arg_id| matcher.check_explicit(arg_id, &ArgPredicate::IsPresent))
+        {
+            if arg_id == other_arg_id {
+                continue;
+            }
+
+            if self
+                .gather_direct_overrides(cmd, arg_id)
+                .contains(other_arg_id)
+            {
+                overrides.push(other_arg_id.clone());
+            }
+            if self
+                .gather_direct_overrides(cmd, other_arg_id)
+                .contains(arg_id)
+            {
+                overrides.push(other_arg_id.clone());
+            }
+        }
+        debug!("Conflicts::gather_overrides: overrides={:?}", conflicts);
+        overrides
+    }
+
+    fn gather_direct_overrides(&mut self, cmd: &Command, arg_id: &Id) -> &[Id] {
+        self.potential.entry(arg_id.clone()).or_insert_with(|| {
+            let conf = if let Some(arg) = cmd.find(arg_id) {
+                arg.overrides.clone()
+            } else if let Some(group) = cmd.find_group(arg_id) {
+                group.conflicts.clone()
+            } else {
+                debug_assert!(false, "id={:?} is unknown", arg_id);
+                Vec::new()
+            };
+            debug!(
+                "Conflicts::gather_direct_overrides id={:?}, overrides={:?}",
+                arg_id, conf
+            );
+            conf
+        })
+    }
 }
 
 pub(crate) fn get_possible_values_cli(a: &Arg) -> Vec<PossibleValue> {