From c69fa75cd5b2031f99e6d358343b18e22ceee5d9 Mon Sep 17 00:00:00 2001 From: Dan Parizher <105245560+danparizher@users.noreply.github.com> Date: Tue, 14 Oct 2025 04:06:17 -0400 Subject: [PATCH] Fix false negatives in `Truthiness::from_expr` for lambdas, generators, and f-strings (#20704) --- .../test/fixtures/flake8_bandit/S602.py | 7 ++ .../test/fixtures/flake8_bandit/S604.py | 16 ++++ .../test/fixtures/flake8_bandit/S609.py | 7 ++ .../test/fixtures/flake8_simplify/SIM222.py | 7 ++ ...s__flake8_bandit__tests__S602_S602.py.snap | 41 ++++++++++ ...s__flake8_bandit__tests__S604_S604.py.snap | 82 +++++++++++++++++++ ...s__flake8_bandit__tests__S609_S609.py.snap | 41 ++++++++++ ...ke8_simplify__tests__SIM222_SIM222.py.snap | 74 +++++++++++++++++ crates/ruff_python_ast/src/helpers.rs | 14 +++- 9 files changed, 285 insertions(+), 4 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S602.py b/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S602.py index 6c40b547e0..a9bab4f122 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S602.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S602.py @@ -33,3 +33,10 @@ class ShellConfig: def run(self, username): Popen("true", shell={**self.shell_defaults, **self.fetch_shell_config(username)}) + +# Additional truthiness cases for generator, lambda, and f-strings +Popen("true", shell=(i for i in ())) +Popen("true", shell=lambda: 0) +Popen("true", shell=f"{b''}") +x = 1 +Popen("true", shell=f"{x=}") diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S604.py b/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S604.py index 46131a04b0..49d906508b 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S604.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S604.py @@ -6,3 +6,19 @@ foo(shell=True) foo(shell={**{}}) foo(shell={**{**{}}}) + +# Truthy non-bool values for `shell` +foo(shell=(i for i in ())) +foo(shell=lambda: 0) + +# f-strings guaranteed non-empty +foo(shell=f"{b''}") +x = 1 +foo(shell=f"{x=}") + +# Additional truthiness cases for generator, lambda, and f-strings +foo(shell=(i for i in ())) +foo(shell=lambda: 0) +foo(shell=f"{b''}") +x = 1 +foo(shell=f"{x=}") diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S609.py b/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S609.py index 6f75dd7451..85de797042 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S609.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S609.py @@ -9,3 +9,10 @@ os.system("tar cf foo.tar bar/*") subprocess.Popen(["chmod", "+w", "*.py"], shell={**{}}) subprocess.Popen(["chmod", "+w", "*.py"], shell={**{**{}}}) + +# Additional truthiness cases for generator, lambda, and f-strings +subprocess.Popen("chmod +w foo*", shell=(i for i in ())) +subprocess.Popen("chmod +w foo*", shell=lambda: 0) +subprocess.Popen("chmod +w foo*", shell=f"{b''}") +x = 1 +subprocess.Popen("chmod +w foo*", shell=f"{x=}") diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM222.py b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM222.py index e2f909acf0..e1e299c98e 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM222.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM222.py @@ -197,3 +197,10 @@ for x in {**a, **b} or [None]: # https://github.com/astral-sh/ruff/issues/7127 def f(a: "'b' or 'c'"): ... + +# https://github.com/astral-sh/ruff/issues/20703 +print(f"{b''}" or "bar") # SIM222 +x = 1 +print(f"{x=}" or "bar") # SIM222 +(lambda: 1) or True # SIM222 +(i for i in range(1)) or "bar" # SIM222 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 2056011585..4115627c2d 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 @@ -127,3 +127,44 @@ S602 `subprocess` call with `shell=True` identified, security issue 21 | 22 | # Check dict display with only double-starred expressions can be falsey. | + +S602 `subprocess` call with truthy `shell` seems safe, but may be changed in the future; consider rewriting without `shell` + --> S602.py:38:1 + | +37 | # Additional truthiness cases for generator, lambda, and f-strings +38 | Popen("true", shell=(i for i in ())) + | ^^^^^ +39 | Popen("true", shell=lambda: 0) +40 | Popen("true", shell=f"{b''}") + | + +S602 `subprocess` call with truthy `shell` seems safe, but may be changed in the future; consider rewriting without `shell` + --> S602.py:39:1 + | +37 | # Additional truthiness cases for generator, lambda, and f-strings +38 | Popen("true", shell=(i for i in ())) +39 | Popen("true", shell=lambda: 0) + | ^^^^^ +40 | Popen("true", shell=f"{b''}") +41 | x = 1 + | + +S602 `subprocess` call with truthy `shell` seems safe, but may be changed in the future; consider rewriting without `shell` + --> S602.py:40:1 + | +38 | Popen("true", shell=(i for i in ())) +39 | Popen("true", shell=lambda: 0) +40 | Popen("true", shell=f"{b''}") + | ^^^^^ +41 | x = 1 +42 | Popen("true", shell=f"{x=}") + | + +S602 `subprocess` call with truthy `shell` seems safe, but may be changed in the future; consider rewriting without `shell` + --> S602.py:42:1 + | +40 | Popen("true", shell=f"{b''}") +41 | x = 1 +42 | Popen("true", shell=f"{x=}") + | ^^^^^ + | diff --git a/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S604_S604.py.snap b/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S604_S604.py.snap index 80764d3418..798e916ba6 100644 --- a/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S604_S604.py.snap +++ b/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S604_S604.py.snap @@ -9,3 +9,85 @@ S604 Function call with `shell=True` parameter identified, security issue 6 | 7 | foo(shell={**{}}) | + +S604 Function call with truthy `shell` parameter identified, security issue + --> S604.py:11:1 + | +10 | # Truthy non-bool values for `shell` +11 | foo(shell=(i for i in ())) + | ^^^ +12 | foo(shell=lambda: 0) + | + +S604 Function call with truthy `shell` parameter identified, security issue + --> S604.py:12:1 + | +10 | # Truthy non-bool values for `shell` +11 | foo(shell=(i for i in ())) +12 | foo(shell=lambda: 0) + | ^^^ +13 | +14 | # f-strings guaranteed non-empty + | + +S604 Function call with truthy `shell` parameter identified, security issue + --> S604.py:15:1 + | +14 | # f-strings guaranteed non-empty +15 | foo(shell=f"{b''}") + | ^^^ +16 | x = 1 +17 | foo(shell=f"{x=}") + | + +S604 Function call with truthy `shell` parameter identified, security issue + --> S604.py:17:1 + | +15 | foo(shell=f"{b''}") +16 | x = 1 +17 | foo(shell=f"{x=}") + | ^^^ +18 | +19 | # Additional truthiness cases for generator, lambda, and f-strings + | + +S604 Function call with truthy `shell` parameter identified, security issue + --> S604.py:20:1 + | +19 | # Additional truthiness cases for generator, lambda, and f-strings +20 | foo(shell=(i for i in ())) + | ^^^ +21 | foo(shell=lambda: 0) +22 | foo(shell=f"{b''}") + | + +S604 Function call with truthy `shell` parameter identified, security issue + --> S604.py:21:1 + | +19 | # Additional truthiness cases for generator, lambda, and f-strings +20 | foo(shell=(i for i in ())) +21 | foo(shell=lambda: 0) + | ^^^ +22 | foo(shell=f"{b''}") +23 | x = 1 + | + +S604 Function call with truthy `shell` parameter identified, security issue + --> S604.py:22:1 + | +20 | foo(shell=(i for i in ())) +21 | foo(shell=lambda: 0) +22 | foo(shell=f"{b''}") + | ^^^ +23 | x = 1 +24 | foo(shell=f"{x=}") + | + +S604 Function call with truthy `shell` parameter identified, security issue + --> S604.py:24:1 + | +22 | foo(shell=f"{b''}") +23 | x = 1 +24 | foo(shell=f"{x=}") + | ^^^ + | diff --git a/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S609_S609.py.snap b/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S609_S609.py.snap index 96282702f2..07c3f3b970 100644 --- a/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S609_S609.py.snap +++ b/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S609_S609.py.snap @@ -43,3 +43,44 @@ S609 Possible wildcard injection in call due to `*` usage 9 | 10 | subprocess.Popen(["chmod", "+w", "*.py"], shell={**{}}) | + +S609 Possible wildcard injection in call due to `*` usage + --> S609.py:14:18 + | +13 | # Additional truthiness cases for generator, lambda, and f-strings +14 | subprocess.Popen("chmod +w foo*", shell=(i for i in ())) + | ^^^^^^^^^^^^^^^ +15 | subprocess.Popen("chmod +w foo*", shell=lambda: 0) +16 | subprocess.Popen("chmod +w foo*", shell=f"{b''}") + | + +S609 Possible wildcard injection in call due to `*` usage + --> S609.py:15:18 + | +13 | # Additional truthiness cases for generator, lambda, and f-strings +14 | subprocess.Popen("chmod +w foo*", shell=(i for i in ())) +15 | subprocess.Popen("chmod +w foo*", shell=lambda: 0) + | ^^^^^^^^^^^^^^^ +16 | subprocess.Popen("chmod +w foo*", shell=f"{b''}") +17 | x = 1 + | + +S609 Possible wildcard injection in call due to `*` usage + --> S609.py:16:18 + | +14 | subprocess.Popen("chmod +w foo*", shell=(i for i in ())) +15 | subprocess.Popen("chmod +w foo*", shell=lambda: 0) +16 | subprocess.Popen("chmod +w foo*", shell=f"{b''}") + | ^^^^^^^^^^^^^^^ +17 | x = 1 +18 | subprocess.Popen("chmod +w foo*", shell=f"{x=}") + | + +S609 Possible wildcard injection in call due to `*` usage + --> S609.py:18:18 + | +16 | subprocess.Popen("chmod +w foo*", shell=f"{b''}") +17 | x = 1 +18 | subprocess.Popen("chmod +w foo*", shell=f"{x=}") + | ^^^^^^^^^^^^^^^ + | diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM222_SIM222.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM222_SIM222.py.snap index 691d91f976..f6c8bba110 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM222_SIM222.py.snap +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM222_SIM222.py.snap @@ -1062,3 +1062,77 @@ help: Replace with `"bar"` 170 | 171 | note: This is an unsafe fix and may change runtime behavior + +SIM222 [*] Use `f"{b''}"` instead of `f"{b''}" or ...` + --> SIM222.py:202:7 + | +201 | # https://github.com/astral-sh/ruff/issues/20703 +202 | print(f"{b''}" or "bar") # SIM222 + | ^^^^^^^^^^^^^^^^^ +203 | x = 1 +204 | print(f"{x=}" or "bar") # SIM222 + | +help: Replace with `f"{b''}"` +199 | def f(a: "'b' or 'c'"): ... +200 | +201 | # https://github.com/astral-sh/ruff/issues/20703 + - print(f"{b''}" or "bar") # SIM222 +202 + print(f"{b''}") # SIM222 +203 | x = 1 +204 | print(f"{x=}" or "bar") # SIM222 +205 | (lambda: 1) or True # SIM222 +note: This is an unsafe fix and may change runtime behavior + +SIM222 [*] Use `f"{x=}"` instead of `f"{x=}" or ...` + --> SIM222.py:204:7 + | +202 | print(f"{b''}" or "bar") # SIM222 +203 | x = 1 +204 | print(f"{x=}" or "bar") # SIM222 + | ^^^^^^^^^^^^^^^^ +205 | (lambda: 1) or True # SIM222 +206 | (i for i in range(1)) or "bar" # SIM222 + | +help: Replace with `f"{x=}"` +201 | # https://github.com/astral-sh/ruff/issues/20703 +202 | print(f"{b''}" or "bar") # SIM222 +203 | x = 1 + - print(f"{x=}" or "bar") # SIM222 +204 + print(f"{x=}") # SIM222 +205 | (lambda: 1) or True # SIM222 +206 | (i for i in range(1)) or "bar" # SIM222 +note: This is an unsafe fix and may change runtime behavior + +SIM222 [*] Use `lambda: 1` instead of `lambda: 1 or ...` + --> SIM222.py:205:1 + | +203 | x = 1 +204 | print(f"{x=}" or "bar") # SIM222 +205 | (lambda: 1) or True # SIM222 + | ^^^^^^^^^^^^^^^^^^^ +206 | (i for i in range(1)) or "bar" # SIM222 + | +help: Replace with `lambda: 1` +202 | print(f"{b''}" or "bar") # SIM222 +203 | x = 1 +204 | print(f"{x=}" or "bar") # SIM222 + - (lambda: 1) or True # SIM222 +205 + lambda: 1 # SIM222 +206 | (i for i in range(1)) or "bar" # SIM222 +note: This is an unsafe fix and may change runtime behavior + +SIM222 [*] Use `(i for i in range(1))` instead of `(i for i in range(1)) or ...` + --> SIM222.py:206:1 + | +204 | print(f"{x=}" or "bar") # SIM222 +205 | (lambda: 1) or True # SIM222 +206 | (i for i in range(1)) or "bar" # SIM222 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: Replace with `(i for i in range(1))` +203 | x = 1 +204 | print(f"{x=}" or "bar") # SIM222 +205 | (lambda: 1) or True # SIM222 + - (i for i in range(1)) or "bar" # SIM222 +206 + (i for i in range(1)) # SIM222 +note: This is an unsafe fix and may change runtime behavior diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index 978a1ae984..9d5159a829 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -12,7 +12,7 @@ use crate::parenthesize::parenthesized_range; use crate::statement_visitor::StatementVisitor; use crate::visitor::Visitor; use crate::{ - self as ast, Arguments, AtomicNodeIndex, CmpOp, DictItem, ExceptHandler, Expr, + self as ast, Arguments, AtomicNodeIndex, CmpOp, DictItem, ExceptHandler, Expr, ExprNoneLiteral, InterpolatedStringElement, MatchCase, Operator, Pattern, Stmt, TypeParam, }; use crate::{AnyNodeRef, ExprContext}; @@ -1219,6 +1219,8 @@ impl Truthiness { F: Fn(&str) -> bool, { match expr { + Expr::Lambda(_) => Self::Truthy, + Expr::Generator(_) => Self::Truthy, Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => { if value.is_empty() { Self::Falsey @@ -1388,7 +1390,9 @@ fn is_non_empty_f_string(expr: &ast::ExprFString) -> bool { Expr::FString(f_string) => is_non_empty_f_string(f_string), // These literals may or may not be empty. Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => !value.is_empty(), - Expr::BytesLiteral(ast::ExprBytesLiteral { value, .. }) => !value.is_empty(), + // Confusingly, f"{b""}" renders as the string 'b""', which is non-empty. + // Therefore, any bytes interpolation is guaranteed non-empty when stringified. + Expr::BytesLiteral(_) => true, } } @@ -1397,7 +1401,9 @@ fn is_non_empty_f_string(expr: &ast::ExprFString) -> bool { ast::FStringPart::FString(f_string) => { f_string.elements.iter().all(|element| match element { InterpolatedStringElement::Literal(string_literal) => !string_literal.is_empty(), - InterpolatedStringElement::Interpolation(f_string) => inner(&f_string.expression), + InterpolatedStringElement::Interpolation(f_string) => { + f_string.debug_text.is_some() || inner(&f_string.expression) + } }) } }) @@ -1493,7 +1499,7 @@ pub fn pep_604_optional(expr: &Expr) -> Expr { ast::ExprBinOp { left: Box::new(expr.clone()), op: Operator::BitOr, - right: Box::new(Expr::NoneLiteral(ast::ExprNoneLiteral::default())), + right: Box::new(Expr::NoneLiteral(ExprNoneLiteral::default())), range: TextRange::default(), node_index: AtomicNodeIndex::NONE, }