Avoid including literal shell=True for truthy, non-True diagnostics (#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 https://github.com/astral-sh/ruff/issues/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 {
+        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,13 +295,14 @@ 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(),
                         ));
@@ -298,7 +310,7 @@ pub(crate) fn shell_injection(checker: &mut Checker, call: &ast::ExprCall) {
                 }
                 // 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 {

crates/ruff_python_ast/src/helpers.rs

@@ -1037,53 +1037,74 @@ pub fn is_unpacking_assignment(parent: &Stmt, child: &Expr) -> bool {
 
 #[derive(Copy, Clone, Debug, PartialEq, is_macro::Is)]
 pub enum Truthiness {
-    // An expression evaluates to `False`.
+    /// The expression is `True`.
+    True,
+    /// The expression is `False`.
+    False,
+    /// The expression evaluates to a `False`-like value (e.g., `None`, `0`, `[]`, `""`).
     Falsey,
-    // An expression evaluates to `True`.
+    /// The expression evaluates to a `True`-like value (e.g., `1`, `"foo"`).
     Truthy,
-    // An expression evaluates to an unknown value (e.g., a variable `x` of unknown type).
+    /// The expression evaluates to an unknown value (e.g., a variable `x` of unknown type).
     Unknown,
 }
 
-impl From<Option<bool>> for Truthiness {
-    fn from(value: Option<bool>) -> Self {
-        match value {
-            Some(true) => Truthiness::Truthy,
-            Some(false) => Truthiness::Falsey,
-            None => Truthiness::Unknown,
-        }
-    }
-}
-
-impl From<Truthiness> for Option<bool> {
-    fn from(truthiness: Truthiness) -> Self {
-        match truthiness {
-            Truthiness::Truthy => Some(true),
-            Truthiness::Falsey => Some(false),
-            Truthiness::Unknown => None,
-        }
-    }
-}
-
 impl Truthiness {
+    /// Return the truthiness of an expression.
     pub fn from_expr<F>(expr: &Expr, is_builtin: F) -> Self
     where
         F: Fn(&str) -> bool,
     {
         match expr {
-            Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => Some(!value.is_empty()),
-            Expr::BytesLiteral(ast::ExprBytesLiteral { value, .. }) => Some(!value.is_empty()),
+            Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => {
+                if value.is_empty() {
+                    Self::Falsey
+                } else {
+                    Self::Truthy
+                }
+            }
+            Expr::BytesLiteral(ast::ExprBytesLiteral { value, .. }) => {
+                if value.is_empty() {
+                    Self::Falsey
+                } else {
+                    Self::Truthy
+                }
+            }
             Expr::NumberLiteral(ast::ExprNumberLiteral { value, .. }) => match value {
-                ast::Number::Int(int) => Some(*int != 0),
-                ast::Number::Float(float) => Some(*float != 0.0),
-                ast::Number::Complex { real, imag, .. } => Some(*real != 0.0 || *imag != 0.0),
+                ast::Number::Int(int) => {
+                    if *int == 0 {
+                        Self::Falsey
+                    } else {
+                        Self::Truthy
+                    }
+                }
+                ast::Number::Float(float) => {
+                    if *float == 0.0 {
+                        Self::Falsey
+                    } else {
+                        Self::Truthy
+                    }
+                }
+                ast::Number::Complex { real, imag, .. } => {
+                    if *real == 0.0 && *imag == 0.0 {
+                        Self::Falsey
+                    } else {
+                        Self::Truthy
+                    }
+                }
             },
-            Expr::BooleanLiteral(ast::ExprBooleanLiteral { value, .. }) => Some(*value),
-            Expr::NoneLiteral(_) => Some(false),
-            Expr::EllipsisLiteral(_) => Some(true),
+            Expr::BooleanLiteral(ast::ExprBooleanLiteral { value, .. }) => {
+                if *value {
+                    Self::True
+                } else {
+                    Self::False
+                }
+            }
+            Expr::NoneLiteral(_) => Self::Falsey,
+            Expr::EllipsisLiteral(_) => Self::Truthy,
             Expr::FString(ast::ExprFString { values, .. }) => {
                 if values.is_empty() {
-                    Some(false)
+                    Self::Falsey
                 } else if values.iter().any(|value| {
                     if let Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = &value {
                         !value.is_empty()
@@ -1091,15 +1112,27 @@ impl Truthiness {
                         false
                     }
                 }) {
-                    Some(true)
+                    Self::Truthy
                 } else {
-                    None
+                    Self::Unknown
                 }
             }
             Expr::List(ast::ExprList { elts, .. })
             | Expr::Set(ast::ExprSet { elts, .. })
-            | Expr::Tuple(ast::ExprTuple { elts, .. }) => Some(!elts.is_empty()),
-            Expr::Dict(ast::ExprDict { keys, .. }) => Some(!keys.is_empty()),
+            | Expr::Tuple(ast::ExprTuple { elts, .. }) => {
+                if elts.is_empty() {
+                    Self::Falsey
+                } else {
+                    Self::Truthy
+                }
+            }
+            Expr::Dict(ast::ExprDict { keys, .. }) => {
+                if keys.is_empty() {
+                    Self::Falsey
+                } else {
+                    Self::Truthy
+                }
+            }
             Expr::Call(ast::ExprCall {
                 func,
                 arguments: Arguments { args, keywords, .. },
@@ -1109,23 +1142,30 @@ impl Truthiness {
                     if is_iterable_initializer(id.as_str(), |id| is_builtin(id)) {
                         if args.is_empty() && keywords.is_empty() {
                             // Ex) `list()`
-                            Some(false)
+                            Self::Falsey
                         } else if args.len() == 1 && keywords.is_empty() {
                             // Ex) `list([1, 2, 3])`
-                            Self::from_expr(&args[0], is_builtin).into()
+                            Self::from_expr(&args[0], is_builtin)
                         } else {
-                            None
+                            Self::Unknown
                         }
                     } else {
-                        None
+                        Self::Unknown
                     }
                 } else {
-                    None
+                    Self::Unknown
                 }
             }
-            _ => None,
+            _ => Self::Unknown,
+        }
+    }
+
+    pub fn into_bool(self) -> Option<bool> {
+        match self {
+            Self::True | Self::Truthy => Some(true),
+            Self::False | Self::Falsey => Some(false),
+            Self::Unknown => None,
         }
-        .into()
     }
 }