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
WalkthroughReplaces direct file reads with context-managed opens for argument parsing and YAML message loading (using yaml.safe_load), and adds validation to database name usage in MySQL and PostgreSQL creation functions to avoid raw 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? |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nettacker/core/messages.py (1)
2-2:⚠️ Potential issue | 🟠 MajorRemove unused
StringIOimport to unblock CI.Line 2 is now unused after the
load_yamlrefactor, and the pipeline already reportsrufffailure for this.Proposed fix
-from io import StringIO🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nettacker/core/messages.py` at line 2, Remove the unused StringIO import from the top of messages.py (the import of StringIO is no longer used after the load_yaml refactor); open messages.py, delete the "from io import StringIO" import line so that ruff no longer flags an unused import and CI passes, leaving all references to load_yaml and other symbols unchanged.
♻️ Duplicate comments (4)
nettacker/database/mysql.py (2)
27-29:⚠️ Potential issue | 🟠 MajorInvalid configuration is being swallowed by the broad
except.
ValueErrorfrom Line 27 is caught byexcept Exceptionand only printed, which hides a hard configuration failure and defers breakage downstream. Re-raiseValueError(or narrow the exception type).Proposed fix
- except Exception as 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 27 - 29, The broad except block is swallowing the ValueError raised for an invalid db_name; update the handler so ValueError is propagated rather than printed — either narrow the except to the specific exceptions you expect or re-raise ValueError explicitly (e.g., detect isinstance(e, ValueError) and raise) and only log/handle non-ValueError exceptions; ensure the code referencing db_name continues to raise on invalid configuration instead of silently printing the error.
23-27:⚠️ Potential issue | 🟠 MajorValidation rejects common DB names and can crash on empty input.
This check blocks identifiers with
_and can raiseIndexErrorwhenConfig.db.nameis empty. Prefer a regex-based non-empty identifier validation.Proposed fix
+import re ... - db_name = Config.db.name - if db_name.isalnum() and db_name[0].isalpha(): + db_name = Config.db.name or "" + if re.fullmatch(r"[A-Za-z][A-Za-z0-9_]{0,62}", db_name): 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}")🤖 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 current validation for db_name in nettacker/database/mysql.py rejects valid identifiers (e.g., containing underscores) and can IndexError on empty strings; replace the alnum/first-char checks around Config.db.name with a regex-based non-empty validation (e.g., require the name match ^[A-Za-z][A-Za-z0-9_]*$) before calling conn.execute(text(...)), and ensure you explicitly handle empty names by raising a clear ValueError if the regex does not match.nettacker/database/postgresql.py (1)
29-33:⚠️ Potential issue | 🟠 MajorHarden DB-name validation and exception chaining in this error path.
db_name[0]can fail on empty input, andisalnum()rejects common names with_. Also, theValueErrorraised insideexcept OperationalError:should be chained explicitly (from None/from err) to satisfy Ruff B904 and make traceback intent clear.Proposed fix
+import re ... - db_name = Config.db.name - if db_name.isalnum() and db_name[0].isalpha(): + db_name = Config.db.name or "" + if re.fullmatch(r"[A-Za-z][A-Za-z0-9_]{0,62}", db_name): 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 None🤖 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, Validate db_name safely before indexing by first checking it's non-empty and matching a stricter regex (e.g. r'^[A-Za-z][A-Za-z0-9_]*$') instead of using db_name.isalnum() and db_name[0].isalpha(); use re.match to allow underscores and prevent IndexError, then call conn.execute(text(f'CREATE DATABASE "{db_name}"')). If you catch an OperationalError and convert it to a ValueError, chain the exception explicitly (e.g. raise ValueError(f"Invalid database name: {db_name}") from err) so tracebacks are intentional; update the code paths around the db_name variable, conn.execute and text(...) accordingly.nettacker/core/arg_parser.py (1)
734-735:⚠️ Potential issue | 🟡 MinorDiscarded split result in wordlist validation still does unnecessary work.
f.read().split("\n")on Line 735 is computed and discarded. If this block is only validating readability, drop.split("\n"); if you intend to use the data, assign it.Proposed minimal fix (validation-only path)
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))🤖 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 734 - 735, The current with open(options.read_from_file) as f: f.read().split("\n") computes and discards the split result; if this block only checks file readability, replace the expression with a simple f.read() (or better, remove reading entirely and just open/close the file) to avoid the unnecessary split call — otherwise, if the file contents are meant to be used later, assign the result to a variable (e.g., lines = f.read().split("\n")) so the data isn't discarded; locate the with open(...) that references options.read_from_file in nettacker/core/arg_parser.py and apply one of these two fixes.
🧹 Nitpick comments (1)
nettacker/core/arg_parser.py (1)
717-721: Narrow broad exception handling in these file-read branches.Catching
Exceptionhere masks unexpected runtime errors; prefer specific I/O and Unicode exceptions.Proposed targeted exception handling
elif options.usernames_list: try: with open(options.usernames_list) as f: options.usernames = list(set(f.read().split("\n"))) - except Exception: + except (OSError, UnicodeError): die_failure(_("error_username").format(options.usernames_list)) ... elif options.passwords_list: try: with open(options.passwords_list) as f: options.passwords = list(set(f.read().split("\n"))) - except Exception: + except (OSError, UnicodeError): die_failure(_("error_passwords").format(options.passwords_list))Also applies to: lines 726-730
🤖 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 717 - 721, Replace the broad "except Exception" in the file-read block that opens options.usernames_list with targeted exception handlers (e.g., FileNotFoundError, PermissionError, UnicodeDecodeError and/or OSError) so only I/O/decoding errors are caught; when handling, pass the caught exception's message into die_failure(_("error_username").format(options.usernames_list)) (or adjust the error format to include the exception text) and apply the same change to the similar block that reads the other list (the branch at lines 726-730) to avoid masking unexpected runtime errors.
🤖 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/database/postgresql.py`:
- Around line 29-34: The connection in the database creation path can leak if
validation raises because conn.close() is skipped; update the code that obtains
conn (the block using conn, text(...) and Config.db.name) to ensure the
connection is always closed by using a context manager (e.g., with
engine.connect() as conn:) or wrap the current logic in try/finally where
conn.close() is called in finally; apply this change around the CREATE DATABASE
logic that references db_name, conn, and text(...) so the connection is
guaranteed closed on exceptions.
---
Outside diff comments:
In `@nettacker/core/messages.py`:
- Line 2: Remove the unused StringIO import from the top of messages.py (the
import of StringIO is no longer used after the load_yaml refactor); open
messages.py, delete the "from io import StringIO" import line so that ruff no
longer flags an unused import and CI passes, leaving all references to load_yaml
and other symbols unchanged.
---
Duplicate comments:
In `@nettacker/core/arg_parser.py`:
- Around line 734-735: The current with open(options.read_from_file) as f:
f.read().split("\n") computes and discards the split result; if this block only
checks file readability, replace the expression with a simple f.read() (or
better, remove reading entirely and just open/close the file) to avoid the
unnecessary split call — otherwise, if the file contents are meant to be used
later, assign the result to a variable (e.g., lines = f.read().split("\n")) so
the data isn't discarded; locate the with open(...) that references
options.read_from_file in nettacker/core/arg_parser.py and apply one of these
two fixes.
In `@nettacker/database/mysql.py`:
- Around line 27-29: The broad except block is swallowing the ValueError raised
for an invalid db_name; update the handler so ValueError is propagated rather
than printed — either narrow the except to the specific exceptions you expect or
re-raise ValueError explicitly (e.g., detect isinstance(e, ValueError) and
raise) and only log/handle non-ValueError exceptions; ensure the code
referencing db_name continues to raise on invalid configuration instead of
silently printing the error.
- Around line 23-27: The current validation for db_name in
nettacker/database/mysql.py rejects valid identifiers (e.g., containing
underscores) and can IndexError on empty strings; replace the alnum/first-char
checks around Config.db.name with a regex-based non-empty validation (e.g.,
require the name match ^[A-Za-z][A-Za-z0-9_]*$) before calling
conn.execute(text(...)), and ensure you explicitly handle empty names by raising
a clear ValueError if the regex does not match.
In `@nettacker/database/postgresql.py`:
- Around line 29-33: Validate db_name safely before indexing by first checking
it's non-empty and matching a stricter regex (e.g. r'^[A-Za-z][A-Za-z0-9_]*$')
instead of using db_name.isalnum() and db_name[0].isalpha(); use re.match to
allow underscores and prevent IndexError, then call conn.execute(text(f'CREATE
DATABASE "{db_name}"')). If you catch an OperationalError and convert it to a
ValueError, chain the exception explicitly (e.g. raise ValueError(f"Invalid
database name: {db_name}") from err) so tracebacks are intentional; update the
code paths around the db_name variable, conn.execute and text(...) accordingly.
---
Nitpick comments:
In `@nettacker/core/arg_parser.py`:
- Around line 717-721: Replace the broad "except Exception" in the file-read
block that opens options.usernames_list with targeted exception handlers (e.g.,
FileNotFoundError, PermissionError, UnicodeDecodeError and/or OSError) so only
I/O/decoding errors are caught; when handling, pass the caught exception's
message into die_failure(_("error_username").format(options.usernames_list)) (or
adjust the error format to include the exception text) and apply the same change
to the similar block that reads the other list (the branch at lines 726-730) to
avoid masking unexpected runtime errors.
🪄 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: 33f3bcff-4fbc-4ecf-b405-01218ae6153f
📒 Files selected for processing (4)
nettacker/core/arg_parser.pynettacker/core/messages.pynettacker/database/mysql.pynettacker/database/postgresql.py
| 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}") | ||
| conn.close() |
There was a problem hiding this comment.
Connection can remain open when validation fails.
If Line 33 raises, Line 34 is skipped and this connection path leaks. Use a context manager for the connection to guarantee closure on exceptions.
Proposed fix
- conn = engine.connect()
- conn = conn.execution_options(isolation_level="AUTOCOMMIT")
- 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}")
- conn.close()
+ with engine.connect() as raw_conn:
+ conn = raw_conn.execution_options(isolation_level="AUTOCOMMIT")
+ 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}")🧰 Tools
🪛 Ruff (0.15.12)
[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 - 34, The connection in the
database creation path can leak if validation raises because conn.close() is
skipped; update the code that obtains conn (the block using conn, text(...) and
Config.db.name) to ensure the connection is always closed by using a context
manager (e.g., with engine.connect() as conn:) or wrap the current logic in
try/finally where conn.close() is called in finally; apply this change around
the CREATE DATABASE logic that references db_name, conn, and text(...) so the
connection is guaranteed closed on exceptions.
3 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.