From e6bdd32069ec49a382f3a4c67bad66c8ebbb8179 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 4 Jul 2023 13:53:32 -0400 Subject: [PATCH] Fix eval detection for suspicious-eval-usage --- .../test/fixtures/flake8_bandit/S307.py | 12 ++++++++++ crates/ruff/src/checkers/ast/mod.rs | 4 +--- crates/ruff/src/rules/flake8_bandit/mod.rs | 1 + .../rules/flake8_bandit/rules/exec_used.rs | 22 ++++++++++++------- .../rules/suspicious_function_call.rs | 4 ++-- ...s__flake8_bandit__tests__S102_S102.py.snap | 4 ++-- ...s__flake8_bandit__tests__S307_S307.py.snap | 20 +++++++++++++++++ 7 files changed, 52 insertions(+), 15 deletions(-) create mode 100644 crates/ruff/resources/test/fixtures/flake8_bandit/S307.py create mode 100644 crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S307_S307.py.snap diff --git a/crates/ruff/resources/test/fixtures/flake8_bandit/S307.py b/crates/ruff/resources/test/fixtures/flake8_bandit/S307.py new file mode 100644 index 00000000000000..06bccc084a8033 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_bandit/S307.py @@ -0,0 +1,12 @@ +import os + +print(eval("1+1")) # S307 +print(eval("os.getcwd()")) # S307 + + +class Class(object): + def eval(self): + print("hi") + + def foo(self): + self.eval() # OK diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 6eb1f6b25cec16..08fceeb2f53dfb 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -2579,9 +2579,7 @@ where flake8_pie::rules::unnecessary_dict_kwargs(self, expr, keywords); } if self.enabled(Rule::ExecBuiltin) { - if let Some(diagnostic) = flake8_bandit::rules::exec_used(expr, func) { - self.diagnostics.push(diagnostic); - } + flake8_bandit::rules::exec_used(self, func); } if self.enabled(Rule::BadFilePermissions) { flake8_bandit::rules::bad_file_permissions(self, func, args, keywords); diff --git a/crates/ruff/src/rules/flake8_bandit/mod.rs b/crates/ruff/src/rules/flake8_bandit/mod.rs index 4abd69f58af28f..87d0e449eb32f8 100644 --- a/crates/ruff/src/rules/flake8_bandit/mod.rs +++ b/crates/ruff/src/rules/flake8_bandit/mod.rs @@ -39,6 +39,7 @@ mod tests { #[test_case(Rule::SubprocessPopenWithShellEqualsTrue, Path::new("S602.py"))] #[test_case(Rule::SubprocessWithoutShellEqualsTrue, Path::new("S603.py"))] #[test_case(Rule::SuspiciousPickleUsage, Path::new("S301.py"))] + #[test_case(Rule::SuspiciousEvalUsage, Path::new("S307.py"))] #[test_case(Rule::SuspiciousTelnetUsage, Path::new("S312.py"))] #[test_case(Rule::TryExceptContinue, Path::new("S112.py"))] #[test_case(Rule::TryExceptPass, Path::new("S110.py"))] diff --git a/crates/ruff/src/rules/flake8_bandit/rules/exec_used.rs b/crates/ruff/src/rules/flake8_bandit/rules/exec_used.rs index 3ff3db8dedc87a..d2dfb83fb5c1a6 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/exec_used.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/exec_used.rs @@ -1,8 +1,10 @@ -use rustpython_parser::ast::{self, Expr, Ranged}; +use rustpython_parser::ast::{Expr, Ranged}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; +use crate::checkers::ast::Checker; + #[violation] pub struct ExecBuiltin; @@ -14,12 +16,16 @@ impl Violation for ExecBuiltin { } /// S102 -pub(crate) fn exec_used(expr: &Expr, func: &Expr) -> Option { - let Expr::Name(ast::ExprName { id, .. }) = func else { - return None; - }; - if id != "exec" { - return None; +pub(crate) fn exec_used(checker: &mut Checker, func: &Expr) { + if checker + .semantic() + .resolve_call_path(func) + .map_or(false, |call_path| { + matches!(call_path.as_slice(), ["" | "builtin", "exec"]) + }) + { + checker + .diagnostics + .push(Diagnostic::new(ExecBuiltin, func.range())); } - Some(Diagnostic::new(ExecBuiltin, expr.range())) } diff --git a/crates/ruff/src/rules/flake8_bandit/rules/suspicious_function_call.rs b/crates/ruff/src/rules/flake8_bandit/rules/suspicious_function_call.rs index cfbb0df50c2f86..6a2add760d9c22 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/suspicious_function_call.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/suspicious_function_call.rs @@ -219,7 +219,7 @@ impl Violation for SuspiciousFTPLibUsage { } } -/// S001 +/// S301, S302, S303, S304, S305, S306, S307, S308, S310, S311, S312, S313, S314, S315, S316, S317, S318, S319, S320, S321, S323 pub(crate) fn suspicious_function_call(checker: &mut Checker, expr: &Expr) { let Expr::Call(ast::ExprCall { func, .. }) = expr else { return; @@ -246,7 +246,7 @@ pub(crate) fn suspicious_function_call(checker: &mut Checker, expr: &Expr) { // Mktemp ["tempfile", "mktemp"] => Some(SuspiciousMktempUsage.into()), // Eval - ["eval"] => Some(SuspiciousEvalUsage.into()), + ["" | "builtins", "eval"] => Some(SuspiciousEvalUsage.into()), // MarkSafe ["django", "utils", "safestring", "mark_safe"] => Some(SuspiciousMarkSafeUsage.into()), // URLOpen diff --git a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S102_S102.py.snap b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S102_S102.py.snap index ccf9572377b454..075092cedae08b 100644 --- a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S102_S102.py.snap +++ b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S102_S102.py.snap @@ -6,7 +6,7 @@ S102.py:3:5: S102 Use of `exec` detected 1 | def fn(): 2 | # Error 3 | exec('x = 2') - | ^^^^^^^^^^^^^ S102 + | ^^^^ S102 4 | 5 | exec('y = 3') | @@ -16,7 +16,7 @@ S102.py:5:1: S102 Use of `exec` detected 3 | exec('x = 2') 4 | 5 | exec('y = 3') - | ^^^^^^^^^^^^^ S102 + | ^^^^ S102 | diff --git a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S307_S307.py.snap b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S307_S307.py.snap new file mode 100644 index 00000000000000..f5c6ac82d8d121 --- /dev/null +++ b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S307_S307.py.snap @@ -0,0 +1,20 @@ +--- +source: crates/ruff/src/rules/flake8_bandit/mod.rs +--- +S307.py:3:7: S307 Use of possibly insecure function; consider using `ast.literal_eval` + | +1 | import os +2 | +3 | print(eval("1+1")) # S307 + | ^^^^^^^^^^^ S307 +4 | print(eval("os.getcwd()")) # S307 + | + +S307.py:4:7: S307 Use of possibly insecure function; consider using `ast.literal_eval` + | +3 | print(eval("1+1")) # S307 +4 | print(eval("os.getcwd()")) # S307 + | ^^^^^^^^^^^^^^^^^^^ S307 + | + +