Skip to content

Commit

Permalink
Fix eval detection for suspicious-eval-usage (#5506)
Browse files Browse the repository at this point in the history
Closes #5505.
  • Loading branch information
charliermarsh authored Jul 4, 2023
1 parent 0b963dd commit 521e6de
Show file tree
Hide file tree
Showing 7 changed files with 52 additions and 15 deletions.
12 changes: 12 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_bandit/S307.py
Original file line number Diff line number Diff line change
@@ -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
4 changes: 1 addition & 3 deletions crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2584,9 +2584,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);
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/rules/flake8_bandit/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"))]
Expand Down
22 changes: 14 additions & 8 deletions crates/ruff/src/rules/flake8_bandit/rules/exec_used.rs
Original file line number Diff line number Diff line change
@@ -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;

Expand All @@ -14,12 +16,16 @@ impl Violation for ExecBuiltin {
}

/// S102
pub(crate) fn exec_used(expr: &Expr, func: &Expr) -> Option<Diagnostic> {
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()))
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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')
|
Expand All @@ -16,7 +16,7 @@ S102.py:5:1: S102 Use of `exec` detected
3 | exec('x = 2')
4 |
5 | exec('y = 3')
| ^^^^^^^^^^^^^ S102
| ^^^^ S102
|


Original file line number Diff line number Diff line change
@@ -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
|


0 comments on commit 521e6de

Please sign in to comment.