-
-
Notifications
You must be signed in to change notification settings - Fork 971
Fix IndexError when parsing malformed --modules-extra-args without '=' sign #1202
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
base: master
Are you sure you want to change the base?
Fix IndexError when parsing malformed --modules-extra-args without '=' sign #1202
Conversation
Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughFixes an IndexError when parsing malformed Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (3)**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
nettacker/**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
nettacker/core/**📄 CodeRabbit inference engine (AGENTS.md)
Files:
🔇 Additional comments (3)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes an IndexError bug when users provide malformed --modules-extra-args without an equals sign, and includes an unrelated fix for UnboundLocalError in graph module tests.
Key Changes
- Added validation to check for '=' character before parsing
--modules-extra-args, preventing IndexError - Improved error messages to guide users on correct format (though not internationalized)
- Fixed UnboundLocalError in graph.py tests by adding return statements after die_failure calls
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| nettacker/core/arg_parser.py | Main fix: validates format and provides clear error messages for malformed --modules-extra-args; uses split("=", 1) to handle values containing '=' |
| nettacker/core/graph.py | Unrelated fix: adds return statements after die_failure to prevent UnboundLocalError when mocked in tests |
| tests/core/test_graph.py | Removes xfail markers and adds assertions for None return value now that graph.py functions return properly when die_failure is mocked |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| parts = args.split("=", 1) | ||
| key = parts[0] | ||
| value = parts[1] |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding validation to reject empty keys. Currently, inputs like "=value" or "=" will pass validation since they contain an '=' character, but will result in an empty string as the key. This could cause confusing behavior later. Add a check like:
if not key or not key.strip():
die_failure(...)
nettacker/core/graph.py
Outdated
| ) | ||
| except ModuleNotFoundError: | ||
| die_failure(_("graph_module_unavailable").format(graph_name)) | ||
| return |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes to graph.py appear to be unrelated to the main purpose of this PR (fixing IndexError in --modules-extra-args parsing). While the fix itself is valid (preventing UnboundLocalError when die_failure is mocked in tests), it should ideally be in a separate PR with its own description and justification. Consider splitting this into a separate PR or documenting why it's included in the PR description.
nettacker/core/arg_parser.py
Outdated
| die_failure( | ||
| f"Invalid format for --modules-extra-args: '{args}'\n" | ||
| f"Expected format: key1=value1&key2=value2\n" | ||
| f"Example: --modules-extra-args \"x_api_key=123&xyz_passwd=abc\"" | ||
| ) | ||
|
|
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message should use the internationalization function _() for consistency with the rest of the codebase. Consider adding a localized message key to nettacker/locale/en.yaml (and other language files) and using it here. For example:
In nettacker/locale/en.yaml:
error_modules_extra_args: "Invalid format for --modules-extra-args: '{0}'\nExpected format: key1=value1&key2=value2\nExample: --modules-extra-args "x_api_key=123&xyz_passwd=abc""
Then use:
die_failure(_("error_modules_extra_args").format(args))
| die_failure( | |
| f"Invalid format for --modules-extra-args: '{args}'\n" | |
| f"Expected format: key1=value1&key2=value2\n" | |
| f"Example: --modules-extra-args \"x_api_key=123&xyz_passwd=abc\"" | |
| ) | |
| die_failure(_("error_modules_extra_args").format(args)) |
- Added validation to check for '=' sign before splitting
- Added validation to reject empty keys (e.g., '=value')
- Used internationalized error messages with _() function
- Added error_modules_extra_args_format and error_modules_extra_args_empty_key to locale
- Used split('=', 1) to properly handle values containing '=' characters
- Strip whitespace from keys
Fixes issue OWASP#1199
0441367 to
55cfd7a
Compare
|
lgmt. Please run |
|
@pUrGe12 Can you check now, Is there something else I need to implement or any suggestion for me, |
Description
This PR fixes the IndexError bug reported in #1199 when parsing malformed
--modules-extra-argswithout an equals sign.Problem
The
--modules-extra-argsparser innettacker/core/arg_parser.pywould crash with anIndexErrorwhen users provided malformed arguments without an=sign, giving a cryptic error message with no guidance.Example crash:
Root Cause
The code used
args.split("=")[1]andargs.split("=")[0]without validating that the split produced at least 2 elements. When a user providedkey(without=),split("=")returned["key"], and accessing index[1]raisedIndexError.Solution
Added comprehensive input validation and improved error handling:
=sign before splitting"=value"or" =value")split("=", 1)with maxsplit to properly handle values that contain=charactersChanges Made
nettacker/core/arg_parser.py (lines 736-765):
if "=" not in args:if not key:_()functionargs.split("=")toargs.split("=", 1)key = parts[0].strip()nettacker/locale/en.yaml:
error_modules_extra_args_formatmessageerror_modules_extra_args_empty_keymessageBefore (Buggy):
After (Fixed):
Impact
Example Error Messages (After Fix)
Missing equals sign:
Empty key:
Fixes #1199