diff --git a/crates/ruff_linter/src/rules/flake8_bandit/rules/shell_injection.rs b/crates/ruff_linter/src/rules/flake8_bandit/rules/shell_injection.rs index a3cec2451c1b2..4b28ffb13bf38 100644 --- a/crates/ruff_linter/src/rules/flake8_bandit/rules/shell_injection.rs +++ b/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,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. diff --git a/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S602_S602.py.snap b/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S602_S602.py.snap index 7e93ab2dd710f..bd5c25865458f 100644 --- a/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S602_S602.py.snap +++ b/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}) diff --git a/crates/ruff_linter/src/rules/flake8_logging/rules/exception_without_exc_info.rs b/crates/ruff_linter/src/rules/flake8_logging/rules/exception_without_exc_info.rs index d3eac436387f5..ed4151befb416 100644 --- a/crates/ruff_linter/src/rules/flake8_logging/rules/exception_without_exc_info.rs +++ b/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) }) } diff --git a/crates/ruff_linter/src/rules/flake8_pytest_style/rules/assertion.rs b/crates/ruff_linter/src/rules/flake8_pytest_style/rules/assertion.rs index 1ccc208298a52..80a6181832a0a 100644 --- a/crates/ruff_linter/src/rules/flake8_pytest_style/rules/assertion.rs +++ b/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())); diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_bool_op.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_bool_op.rs index 7694dd021cb33..cd3b4a879fded 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_bool_op.rs +++ b/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 { diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index 49b3c24b787c0..e182403605ce8 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/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> for Truthiness { - fn from(value: Option) -> Self { - match value { - Some(true) => Truthiness::Truthy, - Some(false) => Truthiness::Falsey, - None => Truthiness::Unknown, - } - } -} - -impl From for Option { - 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(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 { + match self { + Self::True | Self::Truthy => Some(true), + Self::False | Self::Falsey => Some(false), + Self::Unknown => None, } - .into() } }