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.

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

Implement flake8-bandit rule S108 #1644

Merged
merged 10 commits into from Jan 5, 2023
39 changes: 39 additions & 0 deletions README.md
Expand Up @@ -770,6 +770,7 @@ For more, see [flake8-bandit](https://pypi.org/project/flake8-bandit/4.1.1/) on
| S105 | HardcodedPasswordString | Possible hardcoded password: `"..."` | |
| S106 | HardcodedPasswordFuncArg | Possible hardcoded password: `"..."` | |
| S107 | HardcodedPasswordDefault | Possible hardcoded password: `"..."` | |
| S108 | HardcodedTempFile | Probable insecure usage of temp file/directory: `"..."` | |

### flake8-blind-except (BLE)

Expand Down Expand Up @@ -2361,6 +2362,44 @@ suppress-none-returning = true

---

### `flake8-bandit`

#### [`hardcoded-tmp-directory`](#hardcoded-tmp-directory)

List of directories that are considered temporary.

**Default value**: `["/tmp", "/var/tmp", "/dev/shm"]`

**Type**: `Vec<String>`

**Example usage**:

```toml
[tool.ruff.flake8-bandit]
hardcoded_tmp_directory = ["/foo/bar"]
```

---

#### [`hardcoded-tmp-directory-extend`](#hardcoded-tmp-directory-extend)

List of directories that are considered temporary.
These directories are added to the list in
`hardcoded_tmp_directory`.

**Default value**: `[]`

**Type**: `Vec<String>`

**Example usage**:

```toml
[tool.ruff.flake8-bandit]
extend_hardcoded_tmp_directory = ["/foo/bar"]
```

---

### `flake8-bugbear`

#### [`extend-immutable-calls`](#extend-immutable-calls)
Expand Down
7 changes: 7 additions & 0 deletions flake8_to_ruff/src/converter.rs
Expand Up @@ -393,6 +393,7 @@ mod tests {
task_tags: None,
update_check: None,
flake8_annotations: None,
flake8_bandit: None,
flake8_bugbear: None,
flake8_errmsg: None,
flake8_pytest_style: None,
Expand Down Expand Up @@ -455,6 +456,7 @@ mod tests {
task_tags: None,
update_check: None,
flake8_annotations: None,
flake8_bandit: None,
flake8_bugbear: None,
flake8_errmsg: None,
flake8_pytest_style: None,
Expand Down Expand Up @@ -517,6 +519,7 @@ mod tests {
task_tags: None,
update_check: None,
flake8_annotations: None,
flake8_bandit: None,
flake8_bugbear: None,
flake8_errmsg: None,
flake8_pytest_style: None,
Expand Down Expand Up @@ -579,6 +582,7 @@ mod tests {
task_tags: None,
update_check: None,
flake8_annotations: None,
flake8_bandit: None,
flake8_bugbear: None,
flake8_errmsg: None,
flake8_pytest_style: None,
Expand Down Expand Up @@ -641,6 +645,7 @@ mod tests {
task_tags: None,
update_check: None,
flake8_annotations: None,
flake8_bandit: None,
flake8_bugbear: None,
flake8_errmsg: None,
flake8_pytest_style: None,
Expand Down Expand Up @@ -712,6 +717,7 @@ mod tests {
task_tags: None,
update_check: None,
flake8_annotations: None,
flake8_bandit: None,
flake8_bugbear: None,
flake8_errmsg: None,
flake8_pytest_style: None,
Expand Down Expand Up @@ -777,6 +783,7 @@ mod tests {
task_tags: None,
update_check: None,
flake8_annotations: None,
flake8_bandit: None,
flake8_bugbear: None,
flake8_errmsg: None,
flake8_pytest_style: None,
Expand Down
16 changes: 16 additions & 0 deletions resources/test/fixtures/flake8_bandit/S108.py
@@ -0,0 +1,16 @@
# ok
with open("/abc/tmp", "w") as f:
f.write("def")

with open("/tmp/abc", "w") as f:
f.write("def")

with open("/var/tmp/123", "w") as f:
f.write("def")

with open("/dev/shm/unit/test", "w") as f:
f.write("def")

# not ok by config
with open("/foo/bar", "w") as f:
f.write("def")
38 changes: 38 additions & 0 deletions ruff.schema.json
Expand Up @@ -121,6 +121,17 @@
}
]
},
"flake8-bandit": {
"description": "Options for the `flake8-bandit` plugin.",
"anyOf": [
{
"$ref": "#/definitions/Flake8BanditOptions"
},
{
"type": "null"
}
]
},
"flake8-bugbear": {
"description": "Options for the `flake8-bugbear` plugin.",
"anyOf": [
Expand Down Expand Up @@ -915,6 +926,7 @@
"S105",
"S106",
"S107",
"S108",
"SIM",
"SIM1",
"SIM10",
Expand Down Expand Up @@ -1082,6 +1094,32 @@
},
"additionalProperties": false
},
"Flake8BanditOptions": {
"type": "object",
"properties": {
"hardcoded-tmp-directory": {
"description": "List of directories that are considered temporary.",
"type": [
"array",
"null"
],
"items": {
"type": "string"
}
},
"hardcoded-tmp-directory-extend": {
"description": "List of directories that are considered temporary. These directories are added to the list in `hardcoded_tmp_directory`.",
"type": [
"array",
"null"
],
"items": {
"type": "string"
}
}
},
"additionalProperties": false
},
"Flake8BugbearOptions": {
"type": "object",
"properties": {
Expand Down
9 changes: 9 additions & 0 deletions src/checkers/ast.rs
Expand Up @@ -2533,6 +2533,15 @@ where
self.add_check(check);
}
}
if self.settings.enabled.contains(&CheckCode::S108) {
if let Some(check) = flake8_bandit::checks::hardcoded_tmp_dir(
expr,
value,
&mut self.settings.flake8_bandit.all_hardcoded_tmp_directories(),
) {
self.add_check(check);
}
}
if self.settings.enabled.contains(&CheckCode::UP025) {
pyupgrade::plugins::rewrite_unicode_literal(self, expr, kind);
}
Expand Down
20 changes: 20 additions & 0 deletions src/flake8_bandit/checks/hardcoded_tmp_dir.rs
@@ -0,0 +1,20 @@
use rustpython_ast::Expr;

use crate::ast::types::Range;
use crate::registry::{Check, CheckKind};

/// S108
pub fn hardcoded_tmp_dir<'a>(
expr: &Expr,
value: &str,
prefixes: &mut impl Iterator<Item = &'a String>,
) -> Option<Check> {
if prefixes.any(|prefix| value.starts_with(prefix)) {
Some(Check::new(
CheckKind::HardcodedTempFile(value.to_string()),
Range::from_located(expr),
))
} else {
None
}
}
2 changes: 2 additions & 0 deletions src/flake8_bandit/checks/mod.rs
Expand Up @@ -7,6 +7,7 @@ pub use hardcoded_password_func_arg::hardcoded_password_func_arg;
pub use hardcoded_password_string::{
assign_hardcoded_password_string, compare_to_hardcoded_password_string,
};
pub use hardcoded_tmp_dir::hardcoded_tmp_dir;

mod assert_used;
mod bad_file_permissions;
Expand All @@ -15,3 +16,4 @@ mod hardcoded_bind_all_interfaces;
mod hardcoded_password_default;
mod hardcoded_password_func_arg;
mod hardcoded_password_string;
mod hardcoded_tmp_dir;
51 changes: 40 additions & 11 deletions src/flake8_bandit/mod.rs
@@ -1,5 +1,6 @@
pub mod checks;
mod helpers;
pub mod settings;

#[cfg(test)]
mod tests {
Expand All @@ -8,26 +9,54 @@ mod tests {
use anyhow::Result;
use test_case::test_case;

use crate::flake8_bandit::settings::Settings;
use crate::linter::test_path;
use crate::registry::CheckCode;
use crate::settings;

#[test_case(CheckCode::S101, Path::new("S101.py"); "S101")]
#[test_case(CheckCode::S102, Path::new("S102.py"); "S102")]
#[test_case(CheckCode::S103, Path::new("S103.py"); "S103")]
#[test_case(CheckCode::S104, Path::new("S104.py"); "S104")]
#[test_case(CheckCode::S105, Path::new("S105.py"); "S105")]
#[test_case(CheckCode::S106, Path::new("S106.py"); "S106")]
#[test_case(CheckCode::S107, Path::new("S107.py"); "S107")]
fn checks(check_code: CheckCode, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", check_code.as_ref(), path.to_string_lossy());
#[test_case(CheckCode::S101, Path::new("S101.py"), Settings::default(), "S101"; "S101")]
#[test_case(CheckCode::S102, Path::new("S102.py"), Settings::default(), "S102"; "S102")]
#[test_case(CheckCode::S103, Path::new("S103.py"), Settings::default(), "S103"; "S103")]
#[test_case(CheckCode::S104, Path::new("S104.py"), Settings::default(), "S104"; "S104")]
#[test_case(CheckCode::S105, Path::new("S105.py"), Settings::default(), "S105"; "S105")]
#[test_case(CheckCode::S106, Path::new("S106.py"), Settings::default(), "S106"; "S106")]
#[test_case(CheckCode::S107, Path::new("S107.py"), Settings::default(), "S107"; "S107")]
#[test_case(CheckCode::S108, Path::new("S108.py"), Settings::default(), "S108_default"; "S108_0")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me. For other plugins, we tend to use the default, and create dedicated test-cases for overridden settings. But this is cool too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback. These were a bit unreadable so I refactored them.

#[test_case(
CheckCode::S108, Path::new("S108.py"),
Settings {
hardcoded_tmp_directory: vec!["/foo".to_string()],
..Settings::default()
},
"S108_override";
"S108_1"
)]
#[test_case(
CheckCode::S108,
Path::new("S108.py"),
Settings {
hardcoded_tmp_directory_extend: vec!["/foo".to_string()],
..Settings::default()
},
"S108_extend";
"S108_2"
)]
fn checks(
check_code: CheckCode,
path: &Path,
plugin_settings: Settings,
label: &str,
) -> Result<()> {
let checks = test_path(
Path::new("./resources/test/fixtures/flake8_bandit")
.join(path)
.as_path(),
&settings::Settings::for_rule(check_code),
&settings::Settings {
flake8_bandit: plugin_settings,
..settings::Settings::for_rule(check_code)
},
)?;
insta::assert_yaml_snapshot!(snapshot, checks);
insta::assert_yaml_snapshot!(label, checks);
Ok(())
}
}
83 changes: 83 additions & 0 deletions src/flake8_bandit/settings.rs
@@ -0,0 +1,83 @@
//! Settings for the `flake8-bandit` plugin.

use ruff_macros::ConfigurationOptions;
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};

fn default_tmp_dirs() -> Vec<String> {
["/tmp", "/var/tmp", "/dev/shm"]
.map(std::string::ToString::to_string)
.to_vec()
}

#[derive(
Debug, PartialEq, Eq, Serialize, Deserialize, Default, ConfigurationOptions, JsonSchema,
)]
#[serde(
deny_unknown_fields,
rename_all = "kebab-case",
rename = "Flake8BanditOptions"
)]
pub struct Options {
#[option(
default = "[\"/tmp\", \"/var/tmp\", \"/dev/shm\"]",
value_type = "Vec<String>",
example = "hardcoded_tmp_directory = [\"/foo/bar\"]"
)]
/// List of directories that are considered temporary.
pub hardcoded_tmp_directory: Option<Vec<String>>,
#[option(
default = "[]",
value_type = "Vec<String>",
example = "extend_hardcoded_tmp_directory = [\"/foo/bar\"]"
)]
/// List of directories that are considered temporary.
/// These directories are added to the list in
/// `hardcoded_tmp_directory`.
pub hardcoded_tmp_directory_extend: Option<Vec<String>>,
}

#[derive(Debug, Hash)]
pub struct Settings {
pub hardcoded_tmp_directory: Vec<String>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could consider just having hardcoded_tmp_directory (removing hardcoded_tmp_directory_extend from Settings), and then concatenating the two in From<Options>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that makes sense.

pub hardcoded_tmp_directory_extend: Vec<String>,
}

impl From<Options> for Settings {
fn from(options: Options) -> Self {
Self {
hardcoded_tmp_directory: options
.hardcoded_tmp_directory
.unwrap_or_else(default_tmp_dirs),
hardcoded_tmp_directory_extend: options
.hardcoded_tmp_directory_extend
.unwrap_or_default(),
}
}
}
impl From<Settings> for Options {
fn from(settings: Settings) -> Self {
Self {
hardcoded_tmp_directory: Some(settings.hardcoded_tmp_directory),
hardcoded_tmp_directory_extend: Some(settings.hardcoded_tmp_directory_extend),
}
}
}

impl Default for Settings {
fn default() -> Self {
Self {
hardcoded_tmp_directory: default_tmp_dirs(),
hardcoded_tmp_directory_extend: Vec::new(),
}
}
}

impl Settings {
/// Returns an iterator over all directories that are considered temporary.
pub fn all_hardcoded_tmp_directories(&'_ self) -> impl Iterator<Item = &'_ String> {
self.hardcoded_tmp_directory
.iter()
.chain(self.hardcoded_tmp_directory_extend.iter())
}
}