From 379b6a7c2902d42d9417beff653a409c12643331 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 18 Oct 2023 10:16:24 -0400 Subject: [PATCH] Avoid flagging HTTP and HTTPS literals in urllib-open --- .../test/fixtures/flake8_bandit/S310.py | 19 ++++ .../src/rules/flake8_bandit/mod.rs | 1 + .../rules/suspicious_function_call.rs | 25 +++- ...s__flake8_bandit__tests__S310_S310.py.snap | 107 ++++++++++++++++++ 4 files changed, 147 insertions(+), 5 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_bandit/S310.py create mode 100644 crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S310_S310.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S310.py b/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S310.py new file mode 100644 index 00000000000000..e4579254248385 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S310.py @@ -0,0 +1,19 @@ +import urllib + +urllib.urlopen(url='http://www.google.com') +urllib.urlopen(url='http://www.google.com', **kwargs) +urllib.urlopen('http://www.google.com') +urllib.urlopen('file:///foo/bar/baz') +urllib.urlopen(url) + +urllib.Request(url='http://www.google.com', **kwargs) +urllib.Request(url='http://www.google.com') +urllib.Request('http://www.google.com') +urllib.Request('file:///foo/bar/baz') +urllib.Request(url) + +urllib.URLopener().open(fullurl='http://www.google.com', **kwargs) +urllib.URLopener().open(fullurl='http://www.google.com') +urllib.URLopener().open('http://www.google.com') +urllib.URLopener().open('file:///foo/bar/baz') +urllib.URLopener().open(url) diff --git a/crates/ruff_linter/src/rules/flake8_bandit/mod.rs b/crates/ruff_linter/src/rules/flake8_bandit/mod.rs index 1c721660ac7792..f915fd88190ad5 100644 --- a/crates/ruff_linter/src/rules/flake8_bandit/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_bandit/mod.rs @@ -42,6 +42,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::SuspiciousURLOpenUsage, Path::new("S310.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_linter/src/rules/flake8_bandit/rules/suspicious_function_call.rs b/crates/ruff_linter/src/rules/flake8_bandit/rules/suspicious_function_call.rs index 516aad0afc304f..1c2fc85b051452 100644 --- a/crates/ruff_linter/src/rules/flake8_bandit/rules/suspicious_function_call.rs +++ b/crates/ruff_linter/src/rules/flake8_bandit/rules/suspicious_function_call.rs @@ -1,10 +1,9 @@ //! Check for calls to suspicious functions, or calls into suspicious modules. //! //! See: -use ruff_python_ast::ExprCall; - use ruff_diagnostics::{Diagnostic, DiagnosticKind, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{self as ast, Expr, ExprCall}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -851,9 +850,25 @@ pub(crate) fn suspicious_function_call(checker: &mut Checker, call: &ExprCall) { // MarkSafe ["django", "utils", "safestring", "mark_safe"] => Some(SuspiciousMarkSafeUsage.into()), // URLOpen - ["urllib", "urlopen" | "urlretrieve" | "URLopener" | "FancyURLopener" | "Request"] | - ["urllib", "request", "urlopen" | "urlretrieve" | "URLopener" | "FancyURLopener"] | - ["six", "moves", "urllib", "request", "urlopen" | "urlretrieve" | "URLopener" | "FancyURLopener"] => Some(SuspiciousURLOpenUsage.into()), + ["urllib", "urlopen" | "urlretrieve" | "Request"] | + ["urllib", "request", "urlopen" | "urlretrieve"] | + ["six", "moves", "urllib", "request", "urlopen" | "urlretrieve"] => { + // If the `url` argument is a string literal, allow `http` and `https` schemes. + if call.arguments.args.iter().all(|arg| !arg.is_starred_expr()) && call.arguments.keywords.iter().all(|keyword| !keyword.arg.is_none()) { + if let Some(url) = &call.arguments.find_argument("url", 0) { + if let Expr::Constant(ast::ExprConstant { value: ast::Constant::Str(url), .. }) = url { + let url = url.trim_start(); + if url.starts_with("http://") || url.starts_with("https://") { + return None; + } + } + } + } + Some(SuspiciousURLOpenUsage.into()) + }, + ["urllib", "URLopener" | "FancyURLopener"] | + ["urllib", "request", "URLopener" | "FancyURLopener"] | + ["six", "moves", "urllib", "request", "URLopener" | "FancyURLopener"] => Some(SuspiciousURLOpenUsage.into()), // NonCryptographicRandom ["random", "random" | "randrange" | "randint" | "choice" | "choices" | "uniform" | "triangular"] => Some(SuspiciousNonCryptographicRandomUsage.into()), // UnverifiedContext diff --git a/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S310_S310.py.snap b/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S310_S310.py.snap new file mode 100644 index 00000000000000..5af774e47677ab --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S310_S310.py.snap @@ -0,0 +1,107 @@ +--- +source: crates/ruff_linter/src/rules/flake8_bandit/mod.rs +--- +S310.py:4:1: S310 Audit URL open for permitted schemes. Allowing use of `file:` or custom schemes is often unexpected. + | +3 | urllib.urlopen(url='http://www.google.com') +4 | urllib.urlopen(url='http://www.google.com', **kwargs) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S310 +5 | urllib.urlopen('http://www.google.com') +6 | urllib.urlopen('file:///foo/bar/baz') + | + +S310.py:6:1: S310 Audit URL open for permitted schemes. Allowing use of `file:` or custom schemes is often unexpected. + | +4 | urllib.urlopen(url='http://www.google.com', **kwargs) +5 | urllib.urlopen('http://www.google.com') +6 | urllib.urlopen('file:///foo/bar/baz') + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S310 +7 | urllib.urlopen(url) + | + +S310.py:7:1: S310 Audit URL open for permitted schemes. Allowing use of `file:` or custom schemes is often unexpected. + | +5 | urllib.urlopen('http://www.google.com') +6 | urllib.urlopen('file:///foo/bar/baz') +7 | urllib.urlopen(url) + | ^^^^^^^^^^^^^^^^^^^ S310 +8 | +9 | urllib.Request(url='http://www.google.com', **kwargs) + | + +S310.py:9:1: S310 Audit URL open for permitted schemes. Allowing use of `file:` or custom schemes is often unexpected. + | + 7 | urllib.urlopen(url) + 8 | + 9 | urllib.Request(url='http://www.google.com', **kwargs) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S310 +10 | urllib.Request(url='http://www.google.com') +11 | urllib.Request('http://www.google.com') + | + +S310.py:12:1: S310 Audit URL open for permitted schemes. Allowing use of `file:` or custom schemes is often unexpected. + | +10 | urllib.Request(url='http://www.google.com') +11 | urllib.Request('http://www.google.com') +12 | urllib.Request('file:///foo/bar/baz') + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S310 +13 | urllib.Request(url) + | + +S310.py:13:1: S310 Audit URL open for permitted schemes. Allowing use of `file:` or custom schemes is often unexpected. + | +11 | urllib.Request('http://www.google.com') +12 | urllib.Request('file:///foo/bar/baz') +13 | urllib.Request(url) + | ^^^^^^^^^^^^^^^^^^^ S310 +14 | +15 | urllib.URLopener().open(fullurl='http://www.google.com', **kwargs) + | + +S310.py:15:1: S310 Audit URL open for permitted schemes. Allowing use of `file:` or custom schemes is often unexpected. + | +13 | urllib.Request(url) +14 | +15 | urllib.URLopener().open(fullurl='http://www.google.com', **kwargs) + | ^^^^^^^^^^^^^^^^^^ S310 +16 | urllib.URLopener().open(fullurl='http://www.google.com') +17 | urllib.URLopener().open('http://www.google.com') + | + +S310.py:16:1: S310 Audit URL open for permitted schemes. Allowing use of `file:` or custom schemes is often unexpected. + | +15 | urllib.URLopener().open(fullurl='http://www.google.com', **kwargs) +16 | urllib.URLopener().open(fullurl='http://www.google.com') + | ^^^^^^^^^^^^^^^^^^ S310 +17 | urllib.URLopener().open('http://www.google.com') +18 | urllib.URLopener().open('file:///foo/bar/baz') + | + +S310.py:17:1: S310 Audit URL open for permitted schemes. Allowing use of `file:` or custom schemes is often unexpected. + | +15 | urllib.URLopener().open(fullurl='http://www.google.com', **kwargs) +16 | urllib.URLopener().open(fullurl='http://www.google.com') +17 | urllib.URLopener().open('http://www.google.com') + | ^^^^^^^^^^^^^^^^^^ S310 +18 | urllib.URLopener().open('file:///foo/bar/baz') +19 | urllib.URLopener().open(url) + | + +S310.py:18:1: S310 Audit URL open for permitted schemes. Allowing use of `file:` or custom schemes is often unexpected. + | +16 | urllib.URLopener().open(fullurl='http://www.google.com') +17 | urllib.URLopener().open('http://www.google.com') +18 | urllib.URLopener().open('file:///foo/bar/baz') + | ^^^^^^^^^^^^^^^^^^ S310 +19 | urllib.URLopener().open(url) + | + +S310.py:19:1: S310 Audit URL open for permitted schemes. Allowing use of `file:` or custom schemes is often unexpected. + | +17 | urllib.URLopener().open('http://www.google.com') +18 | urllib.URLopener().open('file:///foo/bar/baz') +19 | urllib.URLopener().open(url) + | ^^^^^^^^^^^^^^^^^^ S310 + | + +