Skip to content

Commit

Permalink
Detect mark_safe usages in decorators (#9887)
Browse files Browse the repository at this point in the history
## Summary

Django's `mark_safe` can also be used as a decorator, so we should
detect usages of `@mark_safe` for the purpose of the relevant Bandit
rule.

Closes #9780.
  • Loading branch information
charliermarsh committed Feb 8, 2024
1 parent ed07fa0 commit f76a3e8
Show file tree
Hide file tree
Showing 5 changed files with 88 additions and 2 deletions.
22 changes: 22 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_bandit/S308.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
from django.utils.safestring import mark_safe


def some_func():
return mark_safe('<script>alert("evil!")</script>')


@mark_safe
def some_func():
return '<script>alert("evil!")</script>'


from django.utils.html import mark_safe


def some_func():
return mark_safe('<script>alert("evil!")</script>')


@mark_safe
def some_func():
return '<script>alert("evil!")</script>'
5 changes: 5 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,11 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::HardcodedPasswordDefault) {
flake8_bandit::rules::hardcoded_password_default(checker, parameters);
}
if checker.enabled(Rule::SuspiciousMarkSafeUsage) {
for decorator in decorator_list {
flake8_bandit::rules::suspicious_function_decorator(checker, decorator);
}
}
if checker.enabled(Rule::PropertyWithParameters) {
pylint::rules::property_with_parameters(checker, stmt, decorator_list, parameters);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/flake8_bandit/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ mod tests {
#[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::SuspiciousMarkSafeUsage, Path::new("S308.py"))]
#[test_case(Rule::SuspiciousURLOpenUsage, Path::new("S310.py"))]
#[test_case(Rule::SuspiciousTelnetUsage, Path::new("S312.py"))]
#[test_case(Rule::SuspiciousTelnetlibImport, Path::new("S401.py"))]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
//! See: <https://bandit.readthedocs.io/en/latest/blacklists/blacklist_calls.html>
use ruff_diagnostics::{Diagnostic, DiagnosticKind, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr, ExprCall};
use ruff_python_ast::{self as ast, Decorator, Expr, ExprCall};
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
Expand Down Expand Up @@ -848,7 +848,7 @@ pub(crate) fn suspicious_function_call(checker: &mut Checker, call: &ExprCall) {
// Eval
["" | "builtins", "eval"] => Some(SuspiciousEvalUsage.into()),
// MarkSafe
["django", "utils", "safestring", "mark_safe"] => Some(SuspiciousMarkSafeUsage.into()),
["django", "utils", "safestring" | "html", "mark_safe"] => Some(SuspiciousMarkSafeUsage.into()),
// URLOpen (`urlopen`, `urlretrieve`, `Request`)
["urllib", "request", "urlopen" | "urlretrieve" | "Request"] |
["six", "moves", "urllib", "request", "urlopen" | "urlretrieve" | "Request"] => {
Expand Down Expand Up @@ -901,3 +901,27 @@ pub(crate) fn suspicious_function_call(checker: &mut Checker, call: &ExprCall) {
checker.diagnostics.push(diagnostic);
}
}

/// S308
pub(crate) fn suspicious_function_decorator(checker: &mut Checker, decorator: &Decorator) {
let Some(diagnostic_kind) = checker
.semantic()
.resolve_call_path(&decorator.expression)
.and_then(|call_path| {
match call_path.as_slice() {
// MarkSafe
["django", "utils", "safestring" | "html", "mark_safe"] => {
Some(SuspiciousMarkSafeUsage.into())
}
_ => None,
}
})
else {
return;
};

let diagnostic = Diagnostic::new::<DiagnosticKind>(diagnostic_kind, decorator.range());
if checker.enabled(diagnostic.kind.rule()) {
checker.diagnostics.push(diagnostic);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
---
source: crates/ruff_linter/src/rules/flake8_bandit/mod.rs
---
S308.py:5:12: S308 Use of `mark_safe` may expose cross-site scripting vulnerabilities
|
4 | def some_func():
5 | return mark_safe('<script>alert("evil!")</script>')
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S308
|

S308.py:8:1: S308 Use of `mark_safe` may expose cross-site scripting vulnerabilities
|
8 | @mark_safe
| ^^^^^^^^^^ S308
9 | def some_func():
10 | return '<script>alert("evil!")</script>'
|

S308.py:17:12: S308 Use of `mark_safe` may expose cross-site scripting vulnerabilities
|
16 | def some_func():
17 | return mark_safe('<script>alert("evil!")</script>')
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S308
|

S308.py:20:1: S308 Use of `mark_safe` may expose cross-site scripting vulnerabilities
|
20 | @mark_safe
| ^^^^^^^^^^ S308
21 | def some_func():
22 | return '<script>alert("evil!")</script>'
|


0 comments on commit f76a3e8

Please sign in to comment.