bugfix: fix file handle leaks in arg_parser.py and messages.py#1528
bugfix: fix file handle leaks in arg_parser.py and messages.py#1528mitre88 wants to merge 2 commits intoOWASP:masterfrom
Conversation
…ation - Replace yaml.load() with yaml.safe_load() to prevent arbitrary code execution - Add validation for database names to prevent SQL injection in CREATE DATABASE statements - Use parameterized identifiers with backticks (MySQL) and quotes (PostgreSQL)
- Use context managers for all file open() calls to prevent resource leaks - Fix load_yaml() to properly close file handles - Fix file reading in targets_list, user_agents, usernames_list, passwords_list, and read_from_file
Summary by CodeRabbit
WalkthroughRefactors file-loading operations to use context managers in argument parsing and YAML message loading, while introducing database name validation with alphanumeric constraints in MySQL and PostgreSQL database creation functions to prevent direct SQL interpolation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65e27fbe99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if Config.db.name not in existing_databases: | ||
| conn.execute(text("CREATE DATABASE {0} ".format(Config.db.name))) | ||
| db_name = Config.db.name | ||
| if db_name.isalnum() and db_name[0].isalpha(): |
There was a problem hiding this comment.
The new validation in mysql_create_database() rejects common valid names like test_db because db_name.isalnum() returns false when underscores are present, so the create path raises ValueError instead of creating the DB. This is a regression in first-run MySQL setups where the DB does not already exist, and it conflicts with existing project usage patterns (for example tests/database/test_mysql.py uses test_db).
Useful? React with 👍 / 👎.
| conn = conn.execution_options(isolation_level="AUTOCOMMIT") | ||
| conn.execute(text(f"CREATE DATABASE {Config.db.name}")) | ||
| db_name = Config.db.name | ||
| if db_name.isalnum() and db_name[0].isalpha(): |
There was a problem hiding this comment.
In the OperationalError fallback path, this check now rejects valid PostgreSQL database names containing underscores (e.g., nettacker_db), because isalnum() excludes _. That makes bootstrap fail with ValueError whenever the target DB is missing and uses a conventional underscored name, which is a behavior change from the previous implementation and from existing test configuration (tests/database/test_postgresql.py).
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nettacker/core/arg_parser.py (1)
738-743: 🛠️ Refactor suggestion | 🟠 MajorMissed conversion —
report_path_filenamestill uses bareopen()+close().This block is inconsistent with the PR’s goal of eliminating file-handle leaks: if
close()is reached but an intervening exception occurs (e.g., after a future edit) the handle leaks, and this is exactly the pattern the rest of the PR replaces. Convert it to a context manager for consistency.🔧 Proposed fix
# Check output file try: - temp_file = open(options.report_path_filename, "w") - temp_file.close() + with open(options.report_path_filename, "w"): + pass except Exception: die_failure(_("file_write_error").format(options.report_path_filename))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nettacker/core/arg_parser.py` around lines 738 - 743, The file-check block using open()/close() should use a context manager to avoid potential file-handle leaks: replace the try block that opens options.report_path_filename, calls temp_file.close(), and excepts to die_failure(_("file_write_error").format(...)) with a with open(options.report_path_filename, "w") as temp_file: pattern so the file is always closed even if an intermediate exception occurs; retain the same exception handling that calls die_failure with _("file_write_error").format(options.report_path_filename).
🧹 Nitpick comments (1)
nettacker/database/postgresql.py (1)
29-33: Scope note: these changes don't match the PR title.The PR title and summary describe fixing file handle leaks in
arg_parser.pyandmessages.py, but this file (andmysql.py) introduces a separate SQL-injection hardening change forCREATE DATABASE. Mixing unrelated changes in one PR makes review and revert harder. Consider splitting the SQL-injection-hardening commit into its own PR, or update the PR title/description to reflect both concerns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nettacker/database/postgresql.py` around lines 29 - 33, The change in nettacker/database/postgresql.py tightens CREATE DATABASE by validating db_name before executing f'CREATE DATABASE "{db_name}"' but this is unrelated to the PR title about file handle leaks in arg_parser.py and messages.py; either move this SQL-injection-hardening change into its own commit/PR (extract the db_name validation and the similar change in mysql.py into a separate branch) or update the PR title and description to explicitly include the database hardening work, referencing the db_name validation and the CREATE DATABASE execution in postgresql.py (and corresponding change in mysql.py) so reviewers can see both concerns together.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nettacker/core/arg_parser.py`:
- Around line 732-737: The file-open call in the options.read_from_file block
reads and splits the file but discards the result; either perform only a
readability check (drop the pointless .split and just read or open/close the
file) or actually store the lines into an options field. Update the block around
options.read_from_file so it either (A) simply opens and closes the file to
validate readability (e.g., use f.read() or context manager without split) or
(B) assigns the split result to a named option like options.wordlist_lines
(e.g., options.wordlist_lines = f.read().split("\n")) so callers can use it;
keep the existing exception handling that calls
die_failure(_("error_wordlist").format(options.read_from_file)).
In `@nettacker/database/mysql.py`:
- Around line 23-27: The db-name validation in mysql_create_database is too
strict (rejects underscores) and can raise IndexError on empty names; replace
the isalnum/first-char check with a regex that enforces non-empty names starting
with a letter and then letters/digits/underscores (similar to
postgres_create_database's suggested fix). Also stop swallowing configuration
errors: do not let the outer broad except (the block that currently prints the
exception) silently absorb a ValueError — either narrow that except to
SQLAlchemy/DB errors only or re-raise ValueError so invalid DB names fail fast
and are not deferred to mysql_create_tables.
In `@nettacker/database/postgresql.py`:
- Around line 29-33: The current db name validation using db_name.isalnum() and
db_name[0].isalpha() is too strict (rejects underscores) and crashes on empty
strings; replace it by first checking length (e.g., if not db_name: raise
ValueError) and then validate Config.db.name against a regex that enforces a
leading letter followed by letters, digits, or underscores (e.g.,
r'^[A-Za-z][A-Za-z0-9_]*$') before calling conn.execute, and when raising
ValueError inside the except OperationalError block use "raise ValueError(...)
from None" to suppress the original exception per the review note; reference the
db_name variable and Config.db.name in your changes.
---
Outside diff comments:
In `@nettacker/core/arg_parser.py`:
- Around line 738-743: The file-check block using open()/close() should use a
context manager to avoid potential file-handle leaks: replace the try block that
opens options.report_path_filename, calls temp_file.close(), and excepts to
die_failure(_("file_write_error").format(...)) with a with
open(options.report_path_filename, "w") as temp_file: pattern so the file is
always closed even if an intermediate exception occurs; retain the same
exception handling that calls die_failure with
_("file_write_error").format(options.report_path_filename).
---
Nitpick comments:
In `@nettacker/database/postgresql.py`:
- Around line 29-33: The change in nettacker/database/postgresql.py tightens
CREATE DATABASE by validating db_name before executing f'CREATE DATABASE
"{db_name}"' but this is unrelated to the PR title about file handle leaks in
arg_parser.py and messages.py; either move this SQL-injection-hardening change
into its own commit/PR (extract the db_name validation and the similar change in
mysql.py into a separate branch) or update the PR title and description to
explicitly include the database hardening work, referencing the db_name
validation and the CREATE DATABASE execution in postgresql.py (and corresponding
change in mysql.py) so reviewers can see both concerns together.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 51307c16-487b-499c-a793-70b380ae6704
📒 Files selected for processing (4)
nettacker/core/arg_parser.pynettacker/core/messages.pynettacker/database/mysql.pynettacker/database/postgresql.py
| if options.read_from_file: | ||
| try: | ||
| open(options.read_from_file).read().split("\n") | ||
| with open(options.read_from_file) as f: | ||
| f.read().split("\n") | ||
| except Exception: | ||
| die_failure(_("error_wordlist").format(options.read_from_file)) |
There was a problem hiding this comment.
Read result is discarded — either store it or drop the split.
f.read().split("\n") on Line 735 is evaluated and thrown away. If the intent is only to validate that the wordlist is readable, the .split("\n") is dead work; if the intent is to actually load the wordlist into options, the result should be assigned. Quick clarification will prevent confusion for future maintainers.
🔧 Option A — validation only
if options.read_from_file:
try:
with open(options.read_from_file) as f:
- f.read().split("\n")
+ f.read()
except Exception:
die_failure(_("error_wordlist").format(options.read_from_file))🔧 Option B — actually load the wordlist
if options.read_from_file:
try:
with open(options.read_from_file) as f:
- f.read().split("\n")
+ options.read_from_file = f.read().split("\n")
except Exception:
die_failure(_("error_wordlist").format(options.read_from_file))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if options.read_from_file: | |
| try: | |
| open(options.read_from_file).read().split("\n") | |
| with open(options.read_from_file) as f: | |
| f.read().split("\n") | |
| except Exception: | |
| die_failure(_("error_wordlist").format(options.read_from_file)) | |
| if options.read_from_file: | |
| try: | |
| with open(options.read_from_file) as f: | |
| f.read() | |
| except Exception: | |
| die_failure(_("error_wordlist").format(options.read_from_file)) |
| if options.read_from_file: | |
| try: | |
| open(options.read_from_file).read().split("\n") | |
| with open(options.read_from_file) as f: | |
| f.read().split("\n") | |
| except Exception: | |
| die_failure(_("error_wordlist").format(options.read_from_file)) | |
| if options.read_from_file: | |
| try: | |
| with open(options.read_from_file) as f: | |
| options.read_from_file = f.read().split("\n") | |
| except Exception: | |
| die_failure(_("error_wordlist").format(options.read_from_file)) |
🧰 Tools
🪛 Ruff (0.15.11)
[warning] 736-736: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nettacker/core/arg_parser.py` around lines 732 - 737, The file-open call in
the options.read_from_file block reads and splits the file but discards the
result; either perform only a readability check (drop the pointless .split and
just read or open/close the file) or actually store the lines into an options
field. Update the block around options.read_from_file so it either (A) simply
opens and closes the file to validate readability (e.g., use f.read() or context
manager without split) or (B) assigns the split result to a named option like
options.wordlist_lines (e.g., options.wordlist_lines = f.read().split("\n")) so
callers can use it; keep the existing exception handling that calls
die_failure(_("error_wordlist").format(options.read_from_file)).
| db_name = Config.db.name | ||
| if db_name.isalnum() and db_name[0].isalpha(): | ||
| conn.execute(text(f"CREATE DATABASE `{db_name}` ")) | ||
| else: | ||
| raise ValueError(f"Invalid database name: {db_name}") |
There was a problem hiding this comment.
Same overly strict validation as postgresql.py — and the ValueError is swallowed.
Two issues here:
db_name.isalnum()rejects underscores, which breaks common names likenettacker_db. Same empty-stringIndexErrorrisk as inpostgresql.py::postgres_create_database. See that review comment for a regex-based fix.- This
raise ValueError(...)is immediately caught by the outerexcept Exception as e: print(e)on line 28–29, so an invalid configured DB name is just printed to stdout and execution continues. The caller will later fail inmysql_create_tableswith a less obvious error. Consider either narrowing the outerexcept(e.g., catch only SQLAlchemy errors) or re-raisingValueErrorso configuration problems surface clearly.
🛡️ Suggested direction
if Config.db.name not in existing_databases:
db_name = Config.db.name
- if db_name.isalnum() and db_name[0].isalpha():
+ if re.fullmatch(r"[A-Za-z][A-Za-z0-9_]{0,62}", db_name or ""):
conn.execute(text(f"CREATE DATABASE `{db_name}`"))
else:
raise ValueError(f"Invalid database name: {db_name!r}")
- except Exception as e:
- print(e)
+ except ValueError:
+ raise
+ except Exception as e:
+ print(e)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nettacker/database/mysql.py` around lines 23 - 27, The db-name validation in
mysql_create_database is too strict (rejects underscores) and can raise
IndexError on empty names; replace the isalnum/first-char check with a regex
that enforces non-empty names starting with a letter and then
letters/digits/underscores (similar to postgres_create_database's suggested
fix). Also stop swallowing configuration errors: do not let the outer broad
except (the block that currently prints the exception) silently absorb a
ValueError — either narrow that except to SQLAlchemy/DB errors only or re-raise
ValueError so invalid DB names fail fast and are not deferred to
mysql_create_tables.
| db_name = Config.db.name | ||
| if db_name.isalnum() and db_name[0].isalpha(): | ||
| conn.execute(text(f'CREATE DATABASE "{db_name}"')) | ||
| else: | ||
| raise ValueError(f"Invalid database name: {db_name}") |
There was a problem hiding this comment.
Validation is overly strict and can crash on empty input.
db_name.isalnum() rejects any database name containing an underscore (e.g., nettacker_db, my_app_prod), which is an extremely common convention and likely a regression for existing deployments. Additionally, if Config.db.name is an empty string, db_name[0] raises IndexError before the validation branch is taken — and that IndexError escapes the surrounding except OperationalError block.
Consider using a regex that matches the identifier grammar you actually want to allow (letter followed by letters/digits/underscores), and length-check before indexing:
🛡️ Proposed fix
+import re
...
db_name = Config.db.name
- if db_name.isalnum() and db_name[0].isalpha():
+ if re.fullmatch(r"[A-Za-z][A-Za-z0-9_]{0,62}", db_name or ""):
conn.execute(text(f'CREATE DATABASE "{db_name}"'))
else:
- raise ValueError(f"Invalid database name: {db_name}")
+ raise ValueError(f"Invalid database name: {db_name!r}") from NoneNote the from None also addresses Ruff's B904 hint, since the raise sits inside the except OperationalError: block.
🧰 Tools
🪛 Ruff (0.15.11)
[warning] 33-33: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nettacker/database/postgresql.py` around lines 29 - 33, The current db name
validation using db_name.isalnum() and db_name[0].isalpha() is too strict
(rejects underscores) and crashes on empty strings; replace it by first checking
length (e.g., if not db_name: raise ValueError) and then validate Config.db.name
against a regex that enforces a leading letter followed by letters, digits, or
underscores (e.g., r'^[A-Za-z][A-Za-z0-9_]*$') before calling conn.execute, and
when raising ValueError inside the except OperationalError block use "raise
ValueError(...) from None" to suppress the original exception per the review
note; reference the db_name variable and Config.db.name in your changes.
|
Duplicate of #1527? |
2 participants
Summary
Fix resource leaks caused by file handles not being properly closed.
Changes
nettacker/core/messages.py: Use context manager forload_yaml()file handlenettacker/core/arg_parser.py: Use context managers for all file open() calls:options.targets_listConfig.path.user_agents_fileoptions.usernames_listoptions.passwords_listoptions.read_from_fileWhy
File handles opened with
open()without a correspondingclose()or context manager can cause resource leaks, especially in long-running processes. Usingwith open() as f:ensures files are properly closed even when exceptions occur.