From 56ed5e1d2ef9567692b713a3295bf7ae161748ca 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> {