Skip to content

Commit

Permalink
Support explicit string concatenations in S310 HTTP detection
Browse files Browse the repository at this point in the history
  • Loading branch information
@charliermarsh
charliermarsh committed Jul 13, 2024
1 parent 3bfbbbc commit 72d848d
Show file tree
Hide file tree
Showing 4 changed files with 197 additions and 172 deletions.
5 changes: 5 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_bandit/S310.py
View file
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
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

0 comments on commit 72d848d

Please sign in to comment.