Skip to content

Commit

Permalink
Modify diagnostic ranges for shell-related bandit rules (#10667)
Browse files Browse the repository at this point in the history
Closes #9994.
  • Loading branch information
charliermarsh authored and MichaReiser committed Jun 27, 2024
1 parent 1126d45 commit c1c7642
Show file tree
Hide file tree
Showing 6 changed files with 101 additions and 120 deletions.
33 changes: 11 additions & 22 deletions crates/ruff_linter/src/rules/flake8_bandit/rules/shell_injection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::Truthiness;
use ruff_python_ast::{self as ast, Arguments, Expr, Keyword};
use ruff_python_ast::{self as ast, Arguments, Expr};
use ruff_python_semantic::SemanticModel;
use ruff_text_size::Ranged;

Expand Down Expand Up @@ -296,27 +296,25 @@ pub(crate) fn shell_injection(checker: &mut Checker, call: &ast::ExprCall) {
// S602
Some(ShellKeyword {
truthiness: truthiness @ (Truthiness::True | Truthiness::Truthy),
keyword,
}) => {
if checker.enabled(Rule::SubprocessPopenWithShellEqualsTrue) {
checker.diagnostics.push(Diagnostic::new(
SubprocessPopenWithShellEqualsTrue {
safety: Safety::from(arg),
is_exact: matches!(truthiness, Truthiness::True),
},
keyword.range(),
call.func.range(),
));
}
}
// S603
Some(ShellKeyword {
truthiness: Truthiness::False | Truthiness::Falsey | Truthiness::Unknown,
keyword,
}) => {
if checker.enabled(Rule::SubprocessWithoutShellEqualsTrue) {
checker.diagnostics.push(Diagnostic::new(
SubprocessWithoutShellEqualsTrue,
keyword.range(),
call.func.range(),
));
}
}
Expand All @@ -325,15 +323,14 @@ pub(crate) fn shell_injection(checker: &mut Checker, call: &ast::ExprCall) {
if checker.enabled(Rule::SubprocessWithoutShellEqualsTrue) {
checker.diagnostics.push(Diagnostic::new(
SubprocessWithoutShellEqualsTrue,
arg.range(),
call.func.range(),
));
}
}
}
}
} else if let Some(ShellKeyword {
truthiness: truthiness @ (Truthiness::True | Truthiness::Truthy),
keyword,
}) = shell_keyword
{
// S604
Expand All @@ -342,7 +339,7 @@ pub(crate) fn shell_injection(checker: &mut Checker, call: &ast::ExprCall) {
CallWithShellEqualsTrue {
is_exact: matches!(truthiness, Truthiness::True),
},
keyword.range(),
call.func.range(),
));
}
}
Expand All @@ -355,7 +352,7 @@ pub(crate) fn shell_injection(checker: &mut Checker, call: &ast::ExprCall) {
StartProcessWithAShell {
safety: Safety::from(arg),
},
arg.range(),
call.func.range(),
));
}
}
Expand Down Expand Up @@ -392,17 +389,15 @@ pub(crate) fn shell_injection(checker: &mut Checker, call: &ast::ExprCall) {
Some(CallKind::Subprocess),
Some(ShellKeyword {
truthiness: Truthiness::True | Truthiness::Truthy,
keyword: _,
})
)
)
{
if let Some(arg) = call.arguments.args.first() {
if is_wildcard_command(arg) {
checker.diagnostics.push(Diagnostic::new(
UnixCommandWildcardInjection,
call.func.range(),
));
checker
.diagnostics
.push(Diagnostic::new(UnixCommandWildcardInjection, arg.range()));
}
}
}
Expand Down Expand Up @@ -451,21 +446,15 @@ fn get_call_kind(func: &Expr, semantic: &SemanticModel) -> Option<CallKind> {
}

#[derive(Copy, Clone, Debug)]
struct ShellKeyword<'a> {
struct ShellKeyword {
/// Whether the `shell` keyword argument is set and evaluates to `True`.
truthiness: Truthiness,
/// The `shell` keyword argument.
keyword: &'a Keyword,
}

/// Return the `shell` keyword argument to the given function call, if any.
fn find_shell_keyword<'a>(
arguments: &'a Arguments,
semantic: &SemanticModel,
) -> Option<ShellKeyword<'a>> {
fn find_shell_keyword(arguments: &Arguments, semantic: &SemanticModel) -> Option<ShellKeyword> {
arguments.find_keyword("shell").map(|keyword| ShellKeyword {
truthiness: Truthiness::from_expr(&keyword.value, |id| semantic.has_builtin_binding(id)),
keyword,
})
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,117 +1,115 @@
---
source: crates/ruff_linter/src/rules/flake8_bandit/mod.rs
---
S602.py:4:15: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell`
S602.py:4:1: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell`
|
3 | # Check different Popen wrappers are checked.
4 | Popen("true", shell=True)
| ^^^^^^^^^^ S602
| ^^^^^ S602
5 | call("true", shell=True)
6 | check_call("true", shell=True)
|

S602.py:5:14: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell`
S602.py:5:1: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell`
|
3 | # Check different Popen wrappers are checked.
4 | Popen("true", shell=True)
5 | call("true", shell=True)
| ^^^^^^^^^^ S602
| ^^^^ S602
6 | check_call("true", shell=True)
7 | check_output("true", shell=True)
|

S602.py:6:20: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell`
S602.py:6:1: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell`
|
4 | Popen("true", shell=True)
5 | call("true", shell=True)
6 | check_call("true", shell=True)
| ^^^^^^^^^^ S602
| ^^^^^^^^^^ S602
7 | check_output("true", shell=True)
8 | run("true", shell=True)
|

S602.py:7:22: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell`
S602.py:7:1: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell`
|
5 | call("true", shell=True)
6 | check_call("true", shell=True)
7 | check_output("true", shell=True)
| ^^^^^^^^^^ S602
| ^^^^^^^^^^^^ S602
8 | run("true", shell=True)
|

S602.py:8:13: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell`
S602.py:8:1: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell`
|
6 | check_call("true", shell=True)
7 | check_output("true", shell=True)
8 | run("true", shell=True)
| ^^^^^^^^^^ S602
| ^^^ S602
9 |
10 | # Check values that truthy values are treated as true.
|

S602.py:11:15: S602 `subprocess` call with truthy `shell` seems safe, but may be changed in the future; consider rewriting without `shell`
S602.py:11:1: 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)
| ^^^^^^^ S602
| ^^^^^ S602
12 | Popen("true", shell=[1])
13 | Popen("true", shell={1: 1})
|

S602.py:12:15: S602 `subprocess` call with truthy `shell` seems safe, but may be changed in the future; consider rewriting without `shell`
S602.py:12:1: 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)
12 | Popen("true", shell=[1])
| ^^^^^^^^^ S602
| ^^^^^ S602
13 | Popen("true", shell={1: 1})
14 | Popen("true", shell=(1,))
|

S602.py:13:15: S602 `subprocess` call with truthy `shell` seems safe, but may be changed in the future; consider rewriting without `shell`
S602.py:13:1: 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])
13 | Popen("true", shell={1: 1})
| ^^^^^^^^^^^^ S602
| ^^^^^ S602
14 | Popen("true", shell=(1,))
|

S602.py:14:15: S602 `subprocess` call with truthy `shell` seems safe, but may be changed in the future; consider rewriting without `shell`
S602.py:14:1: 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})
14 | Popen("true", shell=(1,))
| ^^^^^^^^^^ S602
| ^^^^^ S602
15 |
16 | # Check command argument looks unsafe.
|

S602.py:18:19: S602 `subprocess` call with `shell=True` identified, security issue
S602.py:18:1: S602 `subprocess` call with `shell=True` identified, security issue
|
16 | # Check command argument looks unsafe.
17 | var_string = "true"
18 | Popen(var_string, shell=True)
| ^^^^^^^^^^ S602
| ^^^^^ S602
19 | Popen([var_string], shell=True)
20 | Popen([var_string, ""], shell=True)
|

S602.py:19:21: S602 `subprocess` call with `shell=True` identified, security issue
S602.py:19:1: S602 `subprocess` call with `shell=True` identified, security issue
|
17 | var_string = "true"
18 | Popen(var_string, shell=True)
19 | Popen([var_string], shell=True)
| ^^^^^^^^^^ S602
| ^^^^^ S602
20 | Popen([var_string, ""], shell=True)
|

S602.py:20:25: S602 `subprocess` call with `shell=True` identified, security issue
S602.py:20:1: S602 `subprocess` call with `shell=True` identified, security issue
|
18 | Popen(var_string, shell=True)
19 | Popen([var_string], shell=True)
20 | Popen([var_string, ""], shell=True)
| ^^^^^^^^^^ S602
| ^^^^^ S602
|


Original file line number Diff line number Diff line change
@@ -1,106 +1,104 @@
---
source: crates/ruff_linter/src/rules/flake8_bandit/mod.rs
---
S603.py:4:15: S603 `subprocess` call: check for execution of untrusted input
S603.py:4:1: S603 `subprocess` call: check for execution of untrusted input
|
3 | # Different Popen wrappers are checked.
4 | Popen("true", shell=False)
| ^^^^^^^^^^^ S603
| ^^^^^ S603
5 | call("true", shell=False)
6 | check_call("true", shell=False)
|

S603.py:5:14: S603 `subprocess` call: check for execution of untrusted input
S603.py:5:1: S603 `subprocess` call: check for execution of untrusted input
|
3 | # Different Popen wrappers are checked.
4 | Popen("true", shell=False)
5 | call("true", shell=False)
| ^^^^^^^^^^^ S603
| ^^^^ S603
6 | check_call("true", shell=False)
7 | check_output("true", shell=False)
|

S603.py:6:20: S603 `subprocess` call: check for execution of untrusted input
S603.py:6:1: S603 `subprocess` call: check for execution of untrusted input
|
4 | Popen("true", shell=False)
5 | call("true", shell=False)
6 | check_call("true", shell=False)
| ^^^^^^^^^^^ S603
| ^^^^^^^^^^ S603
7 | check_output("true", shell=False)
8 | run("true", shell=False)
|

S603.py:7:22: S603 `subprocess` call: check for execution of untrusted input
S603.py:7:1: S603 `subprocess` call: check for execution of untrusted input
|
5 | call("true", shell=False)
6 | check_call("true", shell=False)
7 | check_output("true", shell=False)
| ^^^^^^^^^^^ S603
| ^^^^^^^^^^^^ S603
8 | run("true", shell=False)
|

S603.py:8:13: S603 `subprocess` call: check for execution of untrusted input
S603.py:8:1: S603 `subprocess` call: check for execution of untrusted input
|
6 | check_call("true", shell=False)
7 | check_output("true", shell=False)
8 | run("true", shell=False)
| ^^^^^^^^^^^ S603
| ^^^ S603
9 |
10 | # Values that falsey values are treated as false.
|

S603.py:11:15: S603 `subprocess` call: check for execution of untrusted input
S603.py:11:1: S603 `subprocess` call: check for execution of untrusted input
|
10 | # Values that falsey values are treated as false.
11 | Popen("true", shell=0)
| ^^^^^^^ S603
| ^^^^^ S603
12 | Popen("true", shell=[])
13 | Popen("true", shell={})
|

S603.py:12:15: S603 `subprocess` call: check for execution of untrusted input
S603.py:12:1: S603 `subprocess` call: check for execution of untrusted input
|
10 | # Values that falsey values are treated as false.
11 | Popen("true", shell=0)
12 | Popen("true", shell=[])
| ^^^^^^^^ S603
| ^^^^^ S603
13 | Popen("true", shell={})
14 | Popen("true", shell=None)
|

S603.py:13:15: S603 `subprocess` call: check for execution of untrusted input
S603.py:13:1: S603 `subprocess` call: check for execution of untrusted input
|
11 | Popen("true", shell=0)
12 | Popen("true", shell=[])
13 | Popen("true", shell={})
| ^^^^^^^^ S603
| ^^^^^ S603
14 | Popen("true", shell=None)
|

S603.py:14:15: S603 `subprocess` call: check for execution of untrusted input
S603.py:14:1: S603 `subprocess` call: check for execution of untrusted input
|
12 | Popen("true", shell=[])
13 | Popen("true", shell={})
14 | Popen("true", shell=None)
| ^^^^^^^^^^ S603
| ^^^^^ S603
15 |
16 | # Unknown values are treated as falsey.
|

S603.py:17:15: S603 `subprocess` call: check for execution of untrusted input
S603.py:17:1: S603 `subprocess` call: check for execution of untrusted input
|
16 | # Unknown values are treated as falsey.
17 | Popen("true", shell=True if True else False)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S603
| ^^^^^ S603
18 |
19 | # No value is also caught.
|

S603.py:20:7: S603 `subprocess` call: check for execution of untrusted input
S603.py:20:1: S603 `subprocess` call: check for execution of untrusted input
|
19 | # No value is also caught.
20 | Popen("true")
| ^^^^^^ S603
| ^^^^^ S603
|


Loading

0 comments on commit c1c7642

Please sign in to comment.