Skip to content

Commit 18c364d

Browse files
charliermarshcharliermarsh
authored
[flake8-bandit] Support explicit string concatenations in S310 HTTP detection (#12315)
Closes #12314.
1 parent 7a7c601 commit 18c364d

File tree

4 files changed

+197
-172
lines changed

4 files changed

+197
-172
lines changed

‎crates/ruff_linter/resources/test/fixtures/flake8_bandit/S310.py

+5
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
urllib.request.urlopen(url='http://www.google.com')
44
urllib.request.urlopen(url=f'http://www.google.com')
5+
urllib.request.urlopen(url='http://' + 'www' + '.google.com')
56
urllib.request.urlopen(url='http://www.google.com', **kwargs)
67
urllib.request.urlopen(url=f'http://www.google.com', **kwargs)
78
urllib.request.urlopen('http://www.google.com')
@@ -11,6 +12,7 @@
1112

1213
urllib.request.Request(url='http://www.google.com')
1314
urllib.request.Request(url=f'http://www.google.com')
15+
urllib.request.Request(url='http://' + 'www' + '.google.com')
1416
urllib.request.Request(url='http://www.google.com', **kwargs)
1517
urllib.request.Request(url=f'http://www.google.com', **kwargs)
1618
urllib.request.Request('http://www.google.com')
@@ -20,15 +22,18 @@
2022

2123
urllib.request.URLopener().open(fullurl='http://www.google.com')
2224
urllib.request.URLopener().open(fullurl=f'http://www.google.com')
25+
urllib.request.URLopener().open(fullurl='http://' + 'www' + '.google.com')
2326
urllib.request.URLopener().open(fullurl='http://www.google.com', **kwargs)
2427
urllib.request.URLopener().open(fullurl=f'http://www.google.com', **kwargs)
2528
urllib.request.URLopener().open('http://www.google.com')
2629
urllib.request.URLopener().open(f'http://www.google.com')
30+
urllib.request.URLopener().open('http://' + 'www' + '.google.com')
2731
urllib.request.URLopener().open('file:///foo/bar/baz')
2832
urllib.request.URLopener().open(url)
2933

3034
urllib.request.urlopen(url=urllib.request.Request('http://www.google.com'))
3135
urllib.request.urlopen(url=urllib.request.Request(f'http://www.google.com'))
36+
urllib.request.urlopen(url=urllib.request.Request('http://' + 'www' + '.google.com'))
3237
urllib.request.urlopen(url=urllib.request.Request('http://www.google.com'), **kwargs)
3338
urllib.request.urlopen(url=urllib.request.Request(f'http://www.google.com'), **kwargs)
3439
urllib.request.urlopen(urllib.request.Request('http://www.google.com'))

‎crates/ruff_linter/src/rules/flake8_bandit/rules/suspicious_function_call.rs

+51-51
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
//! Check for calls to suspicious functions, or calls into suspicious modules.
22
//!
33
//! See: <https://bandit.readthedocs.io/en/latest/blacklists/blacklist_calls.html>
4+
use itertools::Either;
45
use ruff_diagnostics::{Diagnostic, DiagnosticKind, Violation};
56
use ruff_macros::{derive_message_formats, violation};
6-
use ruff_python_ast::{self as ast, Decorator, Expr, ExprCall};
7+
use ruff_python_ast::{self as ast, Decorator, Expr, ExprCall, Operator};
78
use ruff_text_size::Ranged;
89

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

842+
/// Returns `true` if the iterator starts with an HTTP or HTTPS prefix.
843+
fn has_http_prefix(chars: impl Iterator<Item = char> + Clone) -> bool {
844+
has_prefix(chars.clone().skip_while(|c| c.is_whitespace()), "http://")
845+
|| has_prefix(chars.skip_while(|c| c.is_whitespace()), "https://")
846+
}
847+
848+
/// Return the leading characters for an expression, if it's a string literal, f-string, or
849+
/// string concatenation.
850+
fn leading_chars(expr: &Expr) -> Option<impl Iterator<Item = char> + Clone + '_> {
851+
match expr {
852+
// Ex) `"foo"`
853+
Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => {
854+
Some(Either::Left(value.chars()))
855+
}
856+
// Ex) f"foo"
857+
Expr::FString(ast::ExprFString { value, .. }) => {
858+
value.elements().next().and_then(|element| {
859+
if let ast::FStringElement::Literal(ast::FStringLiteralElement {
860+
value, ..
861+
}) = element
862+
{
863+
Some(Either::Right(value.chars()))
864+
} else {
865+
None
866+
}
867+
})
868+
}
869+
// Ex) "foo" + "bar"
870+
Expr::BinOp(ast::ExprBinOp {
871+
op: Operator::Add,
872+
left,
873+
..
874+
}) => leading_chars(left),
875+
_ => None,
876+
}
877+
}
878+
841879
let Some(diagnostic_kind) = checker.semantic().resolve_qualified_name(call.func.as_ref()).and_then(|qualified_name| {
842880
match qualified_name.segments() {
843881
// Pickle
@@ -864,25 +902,11 @@ pub(crate) fn suspicious_function_call(checker: &mut Checker, call: &ExprCall) {
864902
["django", "utils", "safestring" | "html", "mark_safe"] => Some(SuspiciousMarkSafeUsage.into()),
865903
// URLOpen (`Request`)
866904
["urllib", "request", "Request"] |
867-
["six", "moves", "urllib", "request","Request"] => {
868-
// If the `url` argument is a string literal or an f string, allow `http` and `https` schemes.
905+
["six", "moves", "urllib", "request", "Request"] => {
906+
// If the `url` argument is a string literal or an f-string, allow `http` and `https` schemes.
869907
if call.arguments.args.iter().all(|arg| !arg.is_starred_expr()) && call.arguments.keywords.iter().all(|keyword| keyword.arg.is_some()) {
870-
match call.arguments.find_argument("url", 0) {
871-
// If the `url` argument is a string literal, allow `http` and `https` schemes.
872-
Some(Expr::StringLiteral(ast::ExprStringLiteral { value, .. })) => {
873-
if has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "http://") || has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "https://") {
874-
return None;
875-
}
876-
},
877-
// If the `url` argument is an f-string literal, allow `http` and `https` schemes.
878-
Some(Expr::FString(ast::ExprFString { value, .. })) => {
879-
if let Some(ast::FStringElement::Literal(ast::FStringLiteralElement { value, .. })) = value.elements().next() {
880-
if has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "http://") || has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "https://") {
881-
return None;
882-
}
883-
}
884-
},
885-
_ => {}
908+
if call.arguments.find_argument("url", 0).and_then(leading_chars).is_some_and(has_http_prefix) {
909+
return None;
886910
}
887911
}
888912
Some(SuspiciousURLOpenUsage.into())
@@ -892,43 +916,19 @@ pub(crate) fn suspicious_function_call(checker: &mut Checker, call: &ExprCall) {
892916
["six", "moves", "urllib", "request", "urlopen" | "urlretrieve" ] => {
893917
if call.arguments.args.iter().all(|arg| !arg.is_starred_expr()) && call.arguments.keywords.iter().all(|keyword| keyword.arg.is_some()) {
894918
match call.arguments.find_argument("url", 0) {
895-
// If the `url` argument is a string literal, allow `http` and `https` schemes.
896-
Some(Expr::StringLiteral(ast::ExprStringLiteral { value, .. })) => {
897-
if has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "http://") || has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "https://") {
898-
return None;
899-
}
900-
},
901-
902-
// If the `url` argument is an f-string literal, allow `http` and `https` schemes.
903-
Some(Expr::FString(ast::ExprFString { value, .. })) => {
904-
if let Some(ast::FStringElement::Literal(ast::FStringLiteralElement { value, .. })) = value.elements().next() {
905-
if has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "http://") || has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "https://") {
919+
// If the `url` argument is a `urllib.request.Request` object, allow `http` and `https` schemes.
920+
Some(Expr::Call(ExprCall { func, arguments, .. })) => {
921+
if checker.semantic().resolve_qualified_name(func.as_ref()).is_some_and(|name| name.segments() == ["urllib", "request", "Request"]) {
922+
if arguments.find_argument("url", 0).and_then(leading_chars).is_some_and(has_http_prefix) {
906923
return None;
907924
}
908925
}
909926
},
910927

911-
// If the `url` argument is a `urllib.request.Request` object, allow `http` and `https` schemes.
912-
Some(Expr::Call(ExprCall { func, arguments, .. })) => {
913-
if checker.semantic().resolve_qualified_name(func.as_ref()).is_some_and(|name| name.segments() == ["urllib", "request", "Request"]) {
914-
match arguments.find_argument("url", 0) {
915-
// If the `url` argument is a string literal, allow `http` and `https` schemes.
916-
Some(Expr::StringLiteral(ast::ExprStringLiteral { value, .. })) => {
917-
if has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "http://") || has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "https://") {
918-
return None;
919-
}
920-
},
921-
922-
// If the `url` argument is an f-string literal, allow `http` and `https` schemes.
923-
Some(Expr::FString(ast::ExprFString { value, .. })) => {
924-
if let Some(ast::FStringElement::Literal(ast::FStringLiteralElement { value, .. })) = value.elements().next() {
925-
if has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "http://") || has_prefix(value.chars().skip_while(|c| c.is_whitespace()), "https://") {
926-
return None;
927-
}
928-
}
929-
},
930-
_ => {}
931-
}
928+
// If the `url` argument is a string literal, allow `http` and `https` schemes.
929+
Some(expr) => {
930+
if leading_chars(expr).is_some_and(has_http_prefix) {
931+
return None;
932932
}
933933
},
934934

0 commit comments

Comments
 (0)