From e1e65d94e9e5298a6262dd1496f195ce91efc2a2 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 15 Apr 2024 22:05:19 -0400 Subject: [PATCH] Allow urllib.request.urlopen calls with static Request argument --- .../test/fixtures/flake8_bandit/S310.py | 6 +++ .../rules/suspicious_function_call.rs | 42 +++++++++++++++--- ...s__flake8_bandit__tests__S310_S310.py.snap | 43 +++++++++++++++++++ 3 files changed, 85 insertions(+), 6 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S310.py b/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S310.py index c69c5a15d6a89..14c9ee2690877 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S310.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S310.py @@ -17,3 +17,9 @@ urllib.request.URLopener().open('http://www.google.com') urllib.request.URLopener().open('file:///foo/bar/baz') urllib.request.URLopener().open(url) + +urllib.request.urlopen(url=urllib.request.Request('http://www.google.com')) +urllib.request.urlopen(url=urllib.request.Request('http://www.google.com'), **kwargs) +urllib.request.urlopen(urllib.request.Request('http://www.google.com')) +urllib.request.urlopen(urllib.request.Request('file:///foo/bar/baz')) +urllib.request.urlopen(urllib.request.Request(url)) 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 a29ec54434127..00aa3d1cdf439 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 @@ -849,15 +849,45 @@ pub(crate) fn suspicious_function_call(checker: &mut Checker, call: &ExprCall) { ["" | "builtins", "eval"] => Some(SuspiciousEvalUsage.into()), // MarkSafe ["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"] => { + // URLOpen (`Request`) + ["urllib", "request","Request"] | + ["six", "moves", "urllib", "request","Request"] => { // 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_some()) { if let Some(Expr::StringLiteral(ast::ExprStringLiteral { value, .. })) = &call.arguments.find_argument("url", 0) { - let url = value.to_str().trim_start(); - if url.starts_with("http://") || url.starts_with("https://") { - return None; + let url = value.to_str().trim_start(); + if url.starts_with("http://") || url.starts_with("https://") { + return None; + } + + } + } + Some(SuspiciousURLOpenUsage.into()) + } + // URLOpen (`urlopen`, `urlretrieve`) + ["urllib", "request", "urlopen" | "urlretrieve" ] | + ["six", "moves", "urllib", "request", "urlopen" | "urlretrieve" ] => { + if call.arguments.args.iter().all(|arg| !arg.is_starred_expr()) && call.arguments.keywords.iter().all(|keyword| keyword.arg.is_some()) { + if let Some(arg) = &call.arguments.find_argument("url", 0) { + // If the `url` argument is a string literal, allow `http` and `https` schemes. + if let Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = arg { + let url = value.to_str().trim_start(); + if url.starts_with("http://") || url.starts_with("https://") { + return None; + } + } + + // If the `url` argument is a `urllib.request.Request` object, allow `http` and `https` schemes. + if let Expr::Call(ExprCall { func, arguments, .. }) = arg { + if checker.semantic().resolve_qualified_name(func.as_ref()).is_some_and(|name| name.segments() == ["urllib", "request", "Request"]) { + if let Some( Expr::StringLiteral(ast::ExprStringLiteral { value, .. })) = arguments.find_argument("url", 0) { + let url = value.to_str().trim_start(); + if url.starts_with("http://") || url.starts_with("https://") { + return None; + } + + } + } } } } 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 index ddf363cb4b608..a58da3774b520 100644 --- 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 @@ -102,6 +102,49 @@ S310.py:19:1: S310 Audit URL open for permitted schemes. Allowing use of `file:` 18 | urllib.request.URLopener().open('file:///foo/bar/baz') 19 | urllib.request.URLopener().open(url) | ^^^^^^^^^^^^^^^^^^^^^^^^^^ S310 +20 | +21 | urllib.request.urlopen(url=urllib.request.Request('http://www.google.com')) | +S310.py:22:1: S310 Audit URL open for permitted schemes. Allowing use of `file:` or custom schemes is often unexpected. + | +21 | urllib.request.urlopen(url=urllib.request.Request('http://www.google.com')) +22 | urllib.request.urlopen(url=urllib.request.Request('http://www.google.com'), **kwargs) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S310 +23 | urllib.request.urlopen(urllib.request.Request('http://www.google.com')) +24 | urllib.request.urlopen(urllib.request.Request('file:///foo/bar/baz')) + | +S310.py:24:1: S310 Audit URL open for permitted schemes. Allowing use of `file:` or custom schemes is often unexpected. + | +22 | urllib.request.urlopen(url=urllib.request.Request('http://www.google.com'), **kwargs) +23 | urllib.request.urlopen(urllib.request.Request('http://www.google.com')) +24 | urllib.request.urlopen(urllib.request.Request('file:///foo/bar/baz')) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S310 +25 | urllib.request.urlopen(urllib.request.Request(url)) + | + +S310.py:24:24: S310 Audit URL open for permitted schemes. Allowing use of `file:` or custom schemes is often unexpected. + | +22 | urllib.request.urlopen(url=urllib.request.Request('http://www.google.com'), **kwargs) +23 | urllib.request.urlopen(urllib.request.Request('http://www.google.com')) +24 | urllib.request.urlopen(urllib.request.Request('file:///foo/bar/baz')) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S310 +25 | urllib.request.urlopen(urllib.request.Request(url)) + | + +S310.py:25:1: S310 Audit URL open for permitted schemes. Allowing use of `file:` or custom schemes is often unexpected. + | +23 | urllib.request.urlopen(urllib.request.Request('http://www.google.com')) +24 | urllib.request.urlopen(urllib.request.Request('file:///foo/bar/baz')) +25 | urllib.request.urlopen(urllib.request.Request(url)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S310 + | + +S310.py:25:24: S310 Audit URL open for permitted schemes. Allowing use of `file:` or custom schemes is often unexpected. + | +23 | urllib.request.urlopen(urllib.request.Request('http://www.google.com')) +24 | urllib.request.urlopen(urllib.request.Request('file:///foo/bar/baz')) +25 | urllib.request.urlopen(urllib.request.Request(url)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ S310 + |