feat(tasks): add masscan integration - #1225
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds Changesmasscan Task
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
secator/tasks/masscan.py (3)
60-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd or confirm the required
install_cmdmetadata.This task defines
install_pre, but not the guideline-requiredinstall_cmd, so installation metadata may not be picked up consistently. As per coding guidelines,secator/tasks/*.py: “UseCommandsubclass insecator/tasks/to integrate new tools, definingcmd,input_type,output_types, andinstall_cmdattributes”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@secator/tasks/masscan.py` around lines 60 - 62, The Masscan task metadata is missing the required install_cmd declaration, so add or confirm it in the Masscan task class alongside the existing install_pre mapping. Update the task definition in masscan.py, using the same tool-identifying class or Command subclass used for cmd, input_type, and output_types, so installation metadata is consistently discoverable by the framework.Source: Coding guidelines
70-70: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse a set for IP de-duplication.
Masscan can produce large result sets; list membership makes each new IP check O(n).
Proposed fix
- self.seen_ips = [] + self.seen_ips = set() ... - if ip not in self.seen_ips: + if ip not in self.seen_ips: yield Ip( ip=ip, host=ip, alive=False, tags=['masscan'] ) - self.seen_ips.append(ip) + self.seen_ips.add(ip)Also applies to: 139-146
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@secator/tasks/masscan.py` at line 70, The IP tracking in Masscan task deduplication currently uses a list, making repeated membership checks in the result handling path inefficient for large scans. Update the seen_ips state in the Masscan task implementation to use a set and adjust the related result-processing logic around the masscan parsing methods so IP existence checks and inserts stay O(1), while preserving the same deduplication behavior.
117-180: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftRoute custom parsing through
item_loadersor document the exception.The JSON output parser is implemented in
on_cmd_done; the task guideline expects custom output parsers to live initem_loaders. As per coding guidelines,secator/tasks/*.py: “Implement custom output parsers viaitem_loadersin task Command subclasses”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@secator/tasks/masscan.py` around lines 117 - 180, The JSON parsing logic in on_cmd_done should not stay inline in the task handler; move the custom parser into an item_loaders implementation for the Masscan task Command subclass, or explicitly document why this task is exempt. Use the existing on_cmd_done and Masscan task symbols to locate the current parsing block and route the line-by-line JSON-to-Ip/Port conversion through item_loaders instead.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@secator/tasks/masscan.py`:
- Line 37: The masscan task is using a nonstandard option name for output paths,
so the stack’s `masscan.output_path` wiring is ignored. Update the task
configuration and any reads/writes in `masscan.py` to use the standard
`OUTPUT_PATH` key instead of `output_json_path`, and ensure the existing
output-path handling in the masscan task methods references that same symbol
consistently.
- Around line 33-39: Several option definitions in the masscan task options
dictionary exceed the configured 120-character line limit and will fail flake8.
Reformat the long entries in the task’s option mapping so each definition is
wrapped across multiple lines while preserving the same keys and values,
especially the longer `output_json_path`, `exclude_ports`, and `resume_conf`
entries.
- Around line 129-132: The masscan JSON parsing loop in the file-reading logic
is stripping commas before removing trailing newlines, so comma-terminated
records can still fail JSON parsing; update the record normalization in the loop
that uses json.loads to strip whitespace/newlines first and then remove any
trailing comma, and remove the unused enumerate index from that iteration.
- Around line 123-126: The exclude_ports parsing in masscan handling should
ignore empty tokens before converting to integers, since trailing commas can
produce an empty string and break result parsing. Update the logic in the
exclude_ports block to filter out blank values before the int(...) conversion,
and keep the Info(message=...) reporting based on the cleaned list so the
Masscan task continues processing valid ports only.
---
Nitpick comments:
In `@secator/tasks/masscan.py`:
- Around line 60-62: The Masscan task metadata is missing the required
install_cmd declaration, so add or confirm it in the Masscan task class
alongside the existing install_pre mapping. Update the task definition in
masscan.py, using the same tool-identifying class or Command subclass used for
cmd, input_type, and output_types, so installation metadata is consistently
discoverable by the framework.
- Line 70: The IP tracking in Masscan task deduplication currently uses a list,
making repeated membership checks in the result handling path inefficient for
large scans. Update the seen_ips state in the Masscan task implementation to use
a set and adjust the related result-processing logic around the masscan parsing
methods so IP existence checks and inserts stay O(1), while preserving the same
deduplication behavior.
- Around line 117-180: The JSON parsing logic in on_cmd_done should not stay
inline in the task handler; move the custom parser into an item_loaders
implementation for the Masscan task Command subclass, or explicitly document why
this task is exempt. Use the existing on_cmd_done and Masscan task symbols to
locate the current parsing block and route the line-by-line JSON-to-Ip/Port
conversion through item_loaders instead.
🪄 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: 64fa10d7-707e-40fe-a8f2-b0db8cfc372e
📒 Files selected for processing (3)
secator/tasks/masscan.pysecator/utils_test.pytests/fixtures/masscan_output.json
| 'connection_timeout': {'type': int, 'short': 'ct', 'default': None, 'help': 'TCP connection timeout in seconds for banner grabbing'}, | ||
| 'source_port': {'type': int, 'short': 'sp', 'default': None, 'help': 'Spoof source port number'}, | ||
| 'source_ip': {'type': str, 'short': 'si', 'default': None, 'help': 'Spoof source IP address'}, | ||
| 'interface': {'type': str, 'short': 'iface', 'default': None, 'help': 'Network interface to use'}, | ||
| 'output_json_path': {'type': str, 'short': 'oJ', 'default': None, 'help': 'Output JSON file path', 'internal': True, 'display': False}, | ||
| 'exclude_ports': {'type': str, 'short': 'ep', 'default': None, 'help': 'Exclude ports from scan', 'internal': True, 'display': True}, | ||
| 'resume_conf': {'type': str, 'default': None, 'help': 'Path to resume file', 'internal': False, 'display': True}, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Wrap long option definitions under 120 chars.
Several option entries exceed the configured flake8 line length and can fail lint once this file is checked. As per coding guidelines, **/*.py: “Use flake8 linting with max-line-length=120, ignoring W191, E101, E128, E265, W605 as configured in .flake8”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@secator/tasks/masscan.py` around lines 33 - 39, Several option definitions in
the masscan task options dictionary exceed the configured 120-character line
limit and will fail flake8. Reformat the long entries in the task’s option
mapping so each definition is wrapped across multiple lines while preserving the
same keys and values, especially the longer `output_json_path`, `exclude_ports`,
and `resume_conf` entries.
Source: Coding guidelines
| 'source_port': {'type': int, 'short': 'sp', 'default': None, 'help': 'Spoof source port number'}, | ||
| 'source_ip': {'type': str, 'short': 'si', 'default': None, 'help': 'Spoof source IP address'}, | ||
| 'interface': {'type': str, 'short': 'iface', 'default': None, 'help': 'Network interface to use'}, | ||
| 'output_json_path': {'type': str, 'short': 'oJ', 'default': None, 'help': 'Output JSON file path', 'internal': True, 'display': False}, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use the standard OUTPUT_PATH option key.
The stack wiring refers to masscan.output_path, but this task defines and reads output_json_path, so configured output paths won’t be honored. This also leaves OUTPUT_PATH unused.
Proposed fix
- 'output_json_path': {'type': str, 'short': 'oJ', 'default': None, 'help': 'Output JSON file path', 'internal': True, 'display': False},
+ OUTPUT_PATH: {
+ 'type': str,
+ 'short': 'oJ',
+ 'default': None,
+ 'help': 'Output JSON file path',
+ 'internal': True,
+ 'display': False,
+ },
...
- self.output_path = self.get_opt_value('output_json_path') or self.output_path
+ self.output_path = self.get_opt_value(OUTPUT_PATH) or self.output_pathAlso applies to: 93-98
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@secator/tasks/masscan.py` at line 37, The masscan task is using a nonstandard
option name for output paths, so the stack’s `masscan.output_path` wiring is
ignored. Update the task configuration and any reads/writes in `masscan.py` to
use the standard `OUTPUT_PATH` key instead of `output_json_path`, and ensure the
existing output-path handling in the masscan task methods references that same
symbol consistently.
Source: Linters/SAST tools
| exclude_ports = self.get_opt_value('exclude_ports') or [] | ||
| if exclude_ports: | ||
| exclude_ports = [int(p) for p in exclude_ports.split(',')] | ||
| yield Info(message=f'Excluded ports: {exclude_ports}') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle empty exclude_ports tokens.
A value like 80,443, reaches int('') and aborts result parsing.
Proposed fix
exclude_ports = self.get_opt_value('exclude_ports') or []
if exclude_ports:
- exclude_ports = [int(p) for p in exclude_ports.split(',')]
+ exclude_ports = {int(p.strip()) for p in exclude_ports.split(',') if p.strip()}
yield Info(message=f'Excluded ports: {exclude_ports}')📝 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.
| exclude_ports = self.get_opt_value('exclude_ports') or [] | |
| if exclude_ports: | |
| exclude_ports = [int(p) for p in exclude_ports.split(',')] | |
| yield Info(message=f'Excluded ports: {exclude_ports}') | |
| exclude_ports = self.get_opt_value('exclude_ports') or [] | |
| if exclude_ports: | |
| exclude_ports = {int(p.strip()) for p in exclude_ports.split(',') if p.strip()} | |
| yield Info(message=f'Excluded ports: {exclude_ports}') |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@secator/tasks/masscan.py` around lines 123 - 126, The exclude_ports parsing
in masscan handling should ignore empty tokens before converting to integers,
since trailing commas can produce an empty string and break result parsing.
Update the logic in the exclude_ports block to filter out blank values before
the int(...) conversion, and keep the Info(message=...) reporting based on the
cleaned list so the Masscan task continues processing valid ports only.
| for (ix, line) in enumerate(f): | ||
| if not line.startswith('{'): | ||
| continue | ||
| item = json.loads(line.rstrip(',')) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Strip newlines before removing JSON record commas.
line.rstrip(',') does not remove a comma before \n, so normal comma-terminated masscan records can raise JSONDecodeError. This also removes the unused loop index.
Proposed fix
- for (ix, line) in enumerate(f):
- if not line.startswith('{'):
+ for line in f:
+ line = line.strip()
+ if not line.startswith('{'):
continue
- item = json.loads(line.rstrip(','))
+ item = json.loads(line.rstrip(','))📝 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.
| for (ix, line) in enumerate(f): | |
| if not line.startswith('{'): | |
| continue | |
| item = json.loads(line.rstrip(',')) | |
| for line in f: | |
| line = line.strip() | |
| if not line.startswith('{'): | |
| continue | |
| item = json.loads(line.rstrip(',')) |
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 129-129: Loop control variable ix not used within loop body
Rename unused ix to _ix
(B007)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@secator/tasks/masscan.py` around lines 129 - 132, The masscan JSON parsing
loop in the file-reading logic is stripping commas before removing trailing
newlines, so comma-terminated records can still fail JSON parsing; update the
record normalization in the loop that uses json.loads to strip
whitespace/newlines first and then remove any trailing comma, and remove the
unused enumerate index from that iteration.
Source: Linters/SAST tools
1 participant
Summary by CodeRabbit
New Features
Bug Fixes