Avoid including literal shell=True for truthy, non-True diagnosti…

…cs (#8359)

## Summary

If the value of `shell` wasn't literally `True`, we now show a message
describing it as truthy, rather than the (misleading) `shell=True`
literal in the diagnostic.

Closes #8310.

crates/ruff_linter/src/rules/flake8_bandit/rules/shell_injection.rs

@@ -38,18 +38,22 @@ use crate::{
 /// - [Common Weakness Enumeration: CWE-78](https://cwe.mitre.org/data/definitions/78.html)
 #[violation]
 pub struct SubprocessPopenWithShellEqualsTrue {
-    seems_safe: bool,
+    safety: Safety,
+    is_exact: bool,
 }
 
 impl Violation for SubprocessPopenWithShellEqualsTrue {
     #[derive_message_formats]
     fn message(&self) -> String {
-        if self.seems_safe {
-            format!(
+        match (self.safety, self.is_exact) {
+            (Safety::SeemsSafe, true) => format!(
                 "`subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell`"
-            )
-        } else {
-            format!("`subprocess` call with `shell=True` identified, security issue")
+            ),
+            (Safety::Unknown, true) => format!("`subprocess` call with `shell=True` identified, security issue"),
+            (Safety::SeemsSafe, false) => format!(
+                "`subprocess` call with truthy `shell` seems safe, but may be changed in the future; consider rewriting without `shell`"
+            ),
+            (Safety::Unknown, false) => format!("`subprocess` call with truthy `shell` identified, security issue"),
         }
     }
 }
@@ -89,16 +93,18 @@ impl Violation for SubprocessWithoutShellEqualsTrue {
 }
 
 /// ## What it does
-/// Checks for method calls that set the `shell` parameter to `true` when
-/// invoking a subprocess.
+/// Checks for method calls that set the `shell` parameter to `true` or another
+/// truthy value when invoking a subprocess.
 ///
 /// ## Why is this bad?
-/// Setting the `shell` parameter to `true` when invoking a subprocess can
-/// introduce security vulnerabilities, as it allows shell metacharacters and
-/// whitespace to be passed to child processes, potentially leading to shell
-/// injection attacks. It is recommended to avoid using `shell=True` unless
-/// absolutely necessary, and when used, to ensure that all inputs are properly
-/// sanitized and quoted to prevent such vulnerabilities.
+/// Setting the `shell` parameter to `true` or another truthy value when
+/// invoking a subprocess can introduce security vulnerabilities, as it allows
+/// shell metacharacters and whitespace to be passed to child processes,
+/// potentially leading to shell injection attacks.
+///
+/// It is recommended to avoid using `shell=True` unless absolutely necessary
+/// and, when used, to ensure that all inputs are properly sanitized and quoted
+/// to prevent such vulnerabilities.
 ///
 /// ## Known problems
 /// Prone to false positives as it is triggered on any function call with a
@@ -115,12 +121,18 @@ impl Violation for SubprocessWithoutShellEqualsTrue {
 /// ## References
 /// - [Python documentation: Security Considerations](https://docs.python.org/3/library/subprocess.html#security-considerations)
 #[violation]
-pub struct CallWithShellEqualsTrue;
+pub struct CallWithShellEqualsTrue {
+    is_exact: bool,
+}
 
 impl Violation for CallWithShellEqualsTrue {
     #[derive_message_formats]
     fn message(&self) -> String {
-        format!("Function call with `shell=True` parameter identified, security issue")
+        if self.is_exact {
+            format!("Function call with `shell=True` parameter identified, security issue")
+        } else {
+            format!("Function call with truthy `shell` parameter identified, security issue")
+        }
     }
 }
 
@@ -162,16 +174,15 @@ impl Violation for CallWithShellEqualsTrue {
 /// - [Python documentation: `subprocess`](https://docs.python.org/3/library/subprocess.html)
 #[violation]
 pub struct StartProcessWithAShell {
-    seems_safe: bool,
+    safety: Safety,
 }
 
 impl Violation for StartProcessWithAShell {
     #[derive_message_formats]
     fn message(&self) -> String {
-        if self.seems_safe {
-            format!("Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell`")
-        } else {
-            format!("Starting a process with a shell, possible injection detected")
+        match self.safety {
+            Safety::SeemsSafe => format!("Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell`"),
+            Safety::Unknown => format!("Starting a process with a shell, possible injection detected"),
         }
     }
 }
@@ -284,21 +295,22 @@ pub(crate) fn shell_injection(checker: &mut Checker, call: &ast::ExprCall) {
             match shell_keyword {
                 // S602
                 Some(ShellKeyword {
-                    truthiness: Truthiness::Truthy,
+                    truthiness: truthiness @ (Truthiness::True | Truthiness::Truthy),
                     keyword,
                 }) => {
                     if checker.enabled(Rule::SubprocessPopenWithShellEqualsTrue) {
                         checker.diagnostics.push(Diagnostic::new(
                             SubprocessPopenWithShellEqualsTrue {
-                                seems_safe: shell_call_seems_safe(arg),
+                                safety: Safety::from(arg),
+                                is_exact: matches!(truthiness, Truthiness::True),
                             },
                             keyword.range(),
                         ));
                     }
                 }
                 // S603
                 Some(ShellKeyword {
-                    truthiness: Truthiness::Falsey | Truthiness::Unknown,
+                    truthiness: Truthiness::False | Truthiness::Falsey | Truthiness::Unknown,
                     keyword,
                 }) => {
                     if checker.enabled(Rule::SubprocessWithoutShellEqualsTrue) {
@@ -320,15 +332,18 @@ pub(crate) fn shell_injection(checker: &mut Checker, call: &ast::ExprCall) {
             }
         }
     } else if let Some(ShellKeyword {
-        truthiness: Truthiness::Truthy,
+        truthiness: truthiness @ (Truthiness::True | Truthiness::Truthy),
         keyword,
     }) = shell_keyword
     {
         // S604
         if checker.enabled(Rule::CallWithShellEqualsTrue) {
-            checker
-                .diagnostics
-                .push(Diagnostic::new(CallWithShellEqualsTrue, keyword.range()));
+            checker.diagnostics.push(Diagnostic::new(
+                CallWithShellEqualsTrue {
+                    is_exact: matches!(truthiness, Truthiness::True),
+                },
+                keyword.range(),
+            ));
         }
     }
 
@@ -338,7 +353,7 @@ pub(crate) fn shell_injection(checker: &mut Checker, call: &ast::ExprCall) {
             if let Some(arg) = call.arguments.args.first() {
                 checker.diagnostics.push(Diagnostic::new(
                     StartProcessWithAShell {
-                        seems_safe: shell_call_seems_safe(arg),
+                        safety: Safety::from(arg),
                     },
                     arg.range(),
                 ));
@@ -376,7 +391,7 @@ pub(crate) fn shell_injection(checker: &mut Checker, call: &ast::ExprCall) {
                 (
                     Some(CallKind::Subprocess),
                     Some(ShellKeyword {
-                        truthiness: Truthiness::Truthy,
+                        truthiness: Truthiness::True | Truthiness::Truthy,
                         keyword: _,
                     })
                 )
@@ -453,10 +468,22 @@ fn find_shell_keyword<'a>(
     })
 }
 
-/// Return `true` if the value provided to the `shell` call seems safe. This is based on Bandit's
-/// definition: string literals are considered okay, but dynamically-computed values are not.
-fn shell_call_seems_safe(arg: &Expr) -> bool {
-    arg.is_string_literal_expr()
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]
+enum Safety {
+    SeemsSafe,
+    Unknown,
+}
+
+impl From<&Expr> for Safety {
+    /// Return the [`Safety`] level for the [`Expr`]. This is based on Bandit's definition: string
+    /// literals are considered okay, but dynamically-computed values are not.
+    fn from(expr: &Expr) -> Self {
+        if expr.is_string_literal_expr() {
+            Self::SeemsSafe
+        } else {
+            Self::Unknown
+        }
+    }
 }
 
 /// Return `true` if the string appears to be a full file path.

crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S602_S602.py.snap

@@ -49,7 +49,7 @@ S602.py:8:13: S602 `subprocess` call with `shell=True` seems safe, but may be ch
 10 | # Check values that truthy values are treated as true.
    |
 
-S602.py:11:15: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell`
+S602.py:11:15: S602 `subprocess` call with truthy `shell` seems safe, but may be changed in the future; consider rewriting without `shell`
    |
 10 | # Check values that truthy values are treated as true.
 11 | Popen("true", shell=1)
@@ -58,7 +58,7 @@ S602.py:11:15: S602 `subprocess` call with `shell=True` seems safe, but may be c
 13 | Popen("true", shell={1: 1})
    |
 
-S602.py:12:15: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell`
+S602.py:12:15: S602 `subprocess` call with truthy `shell` seems safe, but may be changed in the future; consider rewriting without `shell`
    |
 10 | # Check values that truthy values are treated as true.
 11 | Popen("true", shell=1)
@@ -68,7 +68,7 @@ S602.py:12:15: S602 `subprocess` call with `shell=True` seems safe, but may be c
 14 | Popen("true", shell=(1,))
    |
 
-S602.py:13:15: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell`
+S602.py:13:15: S602 `subprocess` call with truthy `shell` seems safe, but may be changed in the future; consider rewriting without `shell`
    |
 11 | Popen("true", shell=1)
 12 | Popen("true", shell=[1])
@@ -77,7 +77,7 @@ S602.py:13:15: S602 `subprocess` call with `shell=True` seems safe, but may be c
 14 | Popen("true", shell=(1,))
    |
 
-S602.py:14:15: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell`
+S602.py:14:15: S602 `subprocess` call with truthy `shell` seems safe, but may be changed in the future; consider rewriting without `shell`
    |
 12 | Popen("true", shell=[1])
 13 | Popen("true", shell={1: 1})

crates/ruff_linter/src/rules/flake8_logging/rules/exception_without_exc_info.rs

@@ -83,6 +83,7 @@ fn exc_info_arg_is_falsey(call: &ExprCall, checker: &mut Checker) -> bool {
         .find_keyword("exc_info")
         .map(|keyword| &keyword.value)
         .is_some_and(|value| {
-            Truthiness::from_expr(value, |id| checker.semantic().is_builtin(id)).is_falsey()
+            let truthiness = Truthiness::from_expr(value, |id| checker.semantic().is_builtin(id));
+            matches!(truthiness, Truthiness::False | Truthiness::Falsey)
         })
 }

crates/ruff_linter/src/rules/flake8_pytest_style/rules/assertion.rs

@@ -491,7 +491,8 @@ fn to_pytest_raises_args<'a>(
 
 /// PT015
 pub(crate) fn assert_falsy(checker: &mut Checker, stmt: &Stmt, test: &Expr) {
-    if Truthiness::from_expr(test, |id| checker.semantic().is_builtin(id)).is_falsey() {
+    let truthiness = Truthiness::from_expr(test, |id| checker.semantic().is_builtin(id));
+    if matches!(truthiness, Truthiness::False | Truthiness::Falsey) {
         checker
             .diagnostics
             .push(Diagnostic::new(PytestAssertAlwaysFalse, stmt.range()));

crates/ruff_linter/src/rules/flake8_simplify/rules/ast_bool_op.rs

@@ -703,17 +703,15 @@ pub(crate) fn expr_or_not_expr(checker: &mut Checker, expr: &Expr) {
 fn get_short_circuit_edit(
     expr: &Expr,
     range: TextRange,
-    truthiness: Truthiness,
+    truthiness: bool,
     in_boolean_test: bool,
     generator: Generator,
 ) -> Edit {
     let content = if in_boolean_test {
-        match truthiness {
-            Truthiness::Truthy => "True".to_string(),
-            Truthiness::Falsey => "False".to_string(),
-            Truthiness::Unknown => {
-                unreachable!("short_circuit_truthiness should be Truthy or Falsey")
-            }
+        if truthiness {
+            "True".to_string()
+        } else {
+            "False".to_string()
         }
     } else {
         generator.expr(expr)
@@ -746,8 +744,8 @@ fn is_short_circuit(
         return None;
     }
     let short_circuit_truthiness = match op {
-        BoolOp::And => Truthiness::Falsey,
-        BoolOp::Or => Truthiness::Truthy,
+        BoolOp::And => false,
+        BoolOp::Or => true,
     };
 
     let mut furthest = expr;
@@ -773,7 +771,7 @@ fn is_short_circuit(
         // we can return the location of the expression. This should only trigger if the
         // short-circuit expression is the first expression in the list; otherwise, we'll see it
         // as `next_value` before we see it as `value`.
-        if value_truthiness == short_circuit_truthiness {
+        if value_truthiness.into_bool() == Some(short_circuit_truthiness) {
             remove = Some(ContentAround::After);
 
             edit = Some(get_short_circuit_edit(
@@ -798,7 +796,7 @@ fn is_short_circuit(
 
         // If the next expression is a constant, and it matches the short-circuit value, then
         // we can return the location of the expression.
-        if next_value_truthiness == short_circuit_truthiness {
+        if next_value_truthiness.into_bool() == Some(short_circuit_truthiness) {
             remove = Some(if index + 1 == values.len() - 1 {
                 ContentAround::Before
             } else {