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.

Sign up for GitHub

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

Jump to bottom

[flake8-bandit] Support explicit string concatenations in S310 HTTP detection #12315

Merged
merged 1 commit into from
Jul 14, 2024
+197 −172
Merged

[flake8-bandit] Support explicit string concatenations in S310 HTTP detection #12315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_bandit/S310.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

urllib.request.urlopen(url='http://www.google.com')
urllib.request.urlopen(url=f'http://www.google.com')
urllib.request.urlopen(url='http://' + 'www' + '.google.com')
urllib.request.urlopen(url='http://www.google.com', **kwargs)
urllib.request.urlopen(url=f'http://www.google.com', **kwargs)
urllib.request.urlopen('http://www.google.com')
Expand All @@ -11,6 +12,7 @@

urllib.request.Request(url='http://www.google.com')
urllib.request.Request(url=f'http://www.google.com')
urllib.request.Request(url='http://' + 'www' + '.google.com')
urllib.request.Request(url='http://www.google.com', **kwargs)
urllib.request.Request(url=f'http://www.google.com', **kwargs)
urllib.request.Request('http://www.google.com')
Expand All @@ -20,15 +22,18 @@

urllib.request.URLopener().open(fullurl='http://www.google.com')
urllib.request.URLopener().open(fullurl=f'http://www.google.com')
urllib.request.URLopener().open(fullurl='http://' + 'www' + '.google.com')
urllib.request.URLopener().open(fullurl='http://www.google.com', **kwargs)
urllib.request.URLopener().open(fullurl=f'http://www.google.com', **kwargs)
urllib.request.URLopener().open('http://www.google.com')
urllib.request.URLopener().open(f'http://www.google.com')
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(f'http://www.google.com'))
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(url=urllib.request.Request(f'http://www.google.com'), **kwargs)
urllib.request.urlopen(urllib.request.Request('http://www.google.com'))
Expand Down
102 changes: 51 additions & 51 deletions crates/ruff_linter/src/rules/flake8_bandit/rules/suspicious_function_call.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
//! Check for calls to suspicious functions, or calls into suspicious modules.
//!
//! See: <https://bandit.readthedocs.io/en/latest/blacklists/blacklist_calls.html>
use itertools::Either;
use ruff_diagnostics::{Diagnostic, DiagnosticKind, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Decorator, Expr, ExprCall};
use ruff_python_ast::{self as ast, Decorator, Expr, ExprCall, Operator};
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
Expand Down Expand Up @@ -838,6 +839,43 @@ pub(crate) fn suspicious_function_call(checker: &mut Checker, call: &ExprCall) {
true
}

/// Returns `true` if the iterator starts with an HTTP or HTTPS prefix.
fn has_http_prefix(chars: impl Iterator<Item = char> + Clone) -> bool {
has_prefix(chars.clone().skip_while(|c| c.is_whitespace()), "http://")
|| has_prefix(chars.skip_while(|c| c.is_whitespace()), "https://")
}

/// Return the leading characters for an expression, if it's a string literal, f-string, or
/// string concatenation.
fn leading_chars(expr: &Expr) -> Option<impl Iterator<Item = char> + Clone + '_> {
match expr {
// Ex) `"foo"`
Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => {
Some(Either::Left(value.chars()))
}
// Ex) f"foo"
Expr::FString(ast::ExprFString { value, .. }) => {
value.elements().next().and_then(|element| {
if let ast::FStringElement::Literal(ast::FStringLiteralElement {
value, ..
}) = element
{
Some(Either::Right(value.chars()))
} else {
None
}
})
}
// Ex) "foo" + "bar"
Expr::BinOp(ast::ExprBinOp {
op: Operator::Add,
left,
..
}) => leading_chars(left),
_ => None,
}
}

let Some(diagnostic_kind) = checker.semantic().resolve_qualified_name(call.func.as_ref()).and_then(|qualified_name| {
match qualified_name.segments() {
// Pickle
Expand All @@ -864,25 +902,11 @@ pub(crate) fn suspicious_function_call(checker: &mut Checker, call: &ExprCall) {
["django", "utils", "safestring" | "html", "mark_safe"] => Some(SuspiciousMarkSafeUsage.into()),
// URLOpen (`Request`)
["urllib", "request", "Request"] |
["six", "moves", "urllib", "request","Request"] => {
// If the `url` argument is a string literal or an f string, allow `http` and `https` schemes.
["six", "moves", "urllib", "request", "Request"] => {
// If the `url` argument is a string literal or an f-string, 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()) {
match call.arguments.find_argument("url", 0) {
// If the `url` argument is a string literal, allow `http` and `https` schemes.
Some(Expr::StringLiteral(ast::ExprStringLiteral { value, .. })) => {
if has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "http://") || has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "https://") {
return None;
}
},
// If the `url` argument is an f-string literal, allow `http` and `https` schemes.
Some(Expr::FString(ast::ExprFString { value, .. })) => {
if let Some(ast::FStringElement::Literal(ast::FStringLiteralElement { value, .. })) = value.elements().next() {
if has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "http://") || has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "https://") {
return None;
}
}
},
_ => {}
if call.arguments.find_argument("url", 0).and_then(leading_chars).is_some_and(has_http_prefix) {
return None;
}
}
Some(SuspiciousURLOpenUsage.into())
Expand All @@ -892,43 +916,19 @@ pub(crate) fn suspicious_function_call(checker: &mut Checker, call: &ExprCall) {
["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()) {
match call.arguments.find_argument("url", 0) {
// If the `url` argument is a string literal, allow `http` and `https` schemes.
Some(Expr::StringLiteral(ast::ExprStringLiteral { value, .. })) => {
if has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "http://") || has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "https://") {
return None;
}
},

// If the `url` argument is an f-string literal, allow `http` and `https` schemes.
Some(Expr::FString(ast::ExprFString { value, .. })) => {
if let Some(ast::FStringElement::Literal(ast::FStringLiteralElement { value, .. })) = value.elements().next() {
if has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "http://") || has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "https://") {
// If the `url` argument is a `urllib.request.Request` object, allow `http` and `https` schemes.
Some(Expr::Call(ExprCall { func, arguments, .. })) => {
if checker.semantic().resolve_qualified_name(func.as_ref()).is_some_and(|name| name.segments() == ["urllib", "request", "Request"]) {
if arguments.find_argument("url", 0).and_then(leading_chars).is_some_and(has_http_prefix) {
return None;
}
}
},

// If the `url` argument is a `urllib.request.Request` object, allow `http` and `https` schemes.
Some(Expr::Call(ExprCall { func, arguments, .. })) => {
if checker.semantic().resolve_qualified_name(func.as_ref()).is_some_and(|name| name.segments() == ["urllib", "request", "Request"]) {
match arguments.find_argument("url", 0) {
// If the `url` argument is a string literal, allow `http` and `https` schemes.
Some(Expr::StringLiteral(ast::ExprStringLiteral { value, .. })) => {
if has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "http://") || has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "https://") {
return None;
}
},

// If the `url` argument is an f-string literal, allow `http` and `https` schemes.
Some(Expr::FString(ast::ExprFString { value, .. })) => {
if let Some(ast::FStringElement::Literal(ast::FStringLiteralElement { value, .. })) = value.elements().next() {
if has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "http://") || has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "https://") {
return None;
}
}
},
_ => {}
}
// If the `url` argument is a string literal, allow `http` and `https` schemes.
Some(expr) => {
if leading_chars(expr).is_some_and(has_http_prefix) {
return None;
}
},

Expand Down
Loading
Loading