Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(rules): implement flake8-bandit S201 (flask_debug_true) #7503

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 22 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_bandit/S201.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
from flask import Flask

app = Flask(__name__)

@app.route('/')
def main():
raise

#bad
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For whatever reason, the comment we typically use here is # Errors and then # OK (instead of #okay).

app.run(debug=True)

#okay
app.run()
app.run(debug=False)

#unrelated
run()
run(debug=True)
run(debug)
foo.run(debug=True)
app = 1
app.run(debug=True)
3 changes: 3 additions & 0 deletions crates/ruff/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::LoggingConfigInsecureListen) {
flake8_bandit::rules::logging_config_insecure_listen(checker, call);
}
if checker.enabled(Rule::FlaskDebugTrue) {
flake8_bandit::rules::flask_debug_true(checker, call);
}
if checker.any_enabled(&[
Rule::SubprocessWithoutShellEqualsTrue,
Rule::SubprocessPopenWithShellEqualsTrue,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8Bandit, "110") => (RuleGroup::Unspecified, rules::flake8_bandit::rules::TryExceptPass),
(Flake8Bandit, "112") => (RuleGroup::Unspecified, rules::flake8_bandit::rules::TryExceptContinue),
(Flake8Bandit, "113") => (RuleGroup::Unspecified, rules::flake8_bandit::rules::RequestWithoutTimeout),
(Flake8Bandit, "201") => (RuleGroup::Unspecified, rules::flake8_bandit::rules::FlaskDebugTrue),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're now in the habit of adding new rules under RuleGroup::Preview -- mind changing it here?

(Flake8Bandit, "301") => (RuleGroup::Unspecified, rules::flake8_bandit::rules::SuspiciousPickleUsage),
(Flake8Bandit, "302") => (RuleGroup::Unspecified, rules::flake8_bandit::rules::SuspiciousMarshalUsage),
(Flake8Bandit, "303") => (RuleGroup::Unspecified, rules::flake8_bandit::rules::SuspiciousInsecureHashUsage),
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 @@ -19,6 +19,7 @@ mod tests {
#[test_case(Rule::BadFilePermissions, Path::new("S103.py"))]
#[test_case(Rule::CallWithShellEqualsTrue, Path::new("S604.py"))]
#[test_case(Rule::ExecBuiltin, Path::new("S102.py"))]
#[test_case(Rule::FlaskDebugTrue, Path::new("S201.py"))]
#[test_case(Rule::HardcodedBindAllInterfaces, Path::new("S104.py"))]
#[test_case(Rule::HardcodedPasswordDefault, Path::new("S107.py"))]
#[test_case(Rule::HardcodedPasswordFuncArg, Path::new("S106.py"))]
Expand Down
89 changes: 89 additions & 0 deletions crates/ruff/src/rules/flake8_bandit/rules/flask_debug_true.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::is_const_true;
use ruff_python_ast::{Expr, ExprAttribute, ExprCall, Stmt, StmtAssign};
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for uses of `debug=True` in Flask.
///
/// ## Why is this bad?
/// Enabling debug mode shows an interactive debugger in the browser if an error occurs, and allows
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's annoying, but we manually format these to fit 80 characters, since they sometimes get printed to the terminal.

/// running arbitrary Python code from the browser. This could leak sensitive information, or allow
/// an attacker to run arbitrary code.
///
/// ## Example
/// ```python
/// import flask
///
/// app = Flask()
///
/// app.run(debug=True)
/// ```
///
/// Use instead:
/// ```python
/// import flask
///
/// app = Flask()
///
/// app.run(debug=os.environ["ENV"] == "dev")
/// ```
///
/// ## References
/// - [Flask documentation: Debug Mode](https://flask.palletsprojects.com/en/latest/quickstart/#debug-mode)
#[violation]
pub struct FlaskDebugTrue;

impl Violation for FlaskDebugTrue {
#[derive_message_formats]
fn message(&self) -> String {
format!("Use of `debug=True` in Flask app detected")
}
}

/// S201
pub(crate) fn flask_debug_true(checker: &mut Checker, call: &ExprCall) {
let Expr::Attribute(ExprAttribute { attr, value, .. }) = call.func.as_ref() else {
return;
};

if attr.as_str() != "run" {
return;
}

if let Some(debug_argument) = call.arguments.find_keyword("debug") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can reduce one level of indentation by doing:

let Some(debug_argument) = call.arguments.find_keyword("debug") else {
    return;
};

...

if !is_const_true(&debug_argument.value) {
return;
}

if let Expr::Name(name) = value.as_ref() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here: can use let-else to reduce indentation.

checker
.semantic()
.resolve_name(name)
.map_or((), |binding_id| {
if let Some(Stmt::Assign(StmtAssign { value, .. })) = checker
.semantic()
.binding(binding_id)
.statement(checker.semantic())
{
if let Expr::Call(ExprCall { func, .. }) = value.as_ref() {
if checker
.semantic()
.resolve_call_path(func)
.is_some_and(|call_path| {
matches!(call_path.as_slice(), ["flask", "Flask"])
})
{
checker
.diagnostics
.push(Diagnostic::new(FlaskDebugTrue, debug_argument.range()));
}
}
}
});
}
}
}
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/flake8_bandit/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
pub(crate) use assert_used::*;
pub(crate) use bad_file_permissions::*;
pub(crate) use exec_used::*;
pub(crate) use flask_debug_true::*;
pub(crate) use hardcoded_bind_all_interfaces::*;
pub(crate) use hardcoded_password_default::*;
pub(crate) use hardcoded_password_func_arg::*;
Expand All @@ -24,6 +25,7 @@ pub(crate) use unsafe_yaml_load::*;
mod assert_used;
mod bad_file_permissions;
mod exec_used;
mod flask_debug_true;
mod hardcoded_bind_all_interfaces;
mod hardcoded_password_default;
mod hardcoded_password_func_arg;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---
source: crates/ruff/src/rules/flake8_bandit/mod.rs
---
S201.py:10:9: S201 Use of `debug=True` in Flask app detected
|
9 | #bad
10 | app.run(debug=True)
| ^^^^^^^^^^ S201
11 |
12 | #okay
|


3 changes: 3 additions & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.