feat: add scapy integration (basic ARP scan) #813
Conversation
Co-authored-by: ocervell <9629314+ocervell@users.noreply.github.com>
Co-authored-by: ocervell <9629314+ocervell@users.noreply.github.com>
Co-authored-by: ocervell <9629314+ocervell@users.noreply.github.com>
ocervell
changed the title
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 6
🧹 Nitpick comments (4)
tests/unit/test_python_runner.py (1)
1-162: LGTM! Comprehensive test suite for PythonRunner.The test suite thoroughly covers PythonRunner functionality:
- Basic task execution and output types
- Custom options passing
- Context access (name, inputs)
- Input validation (empty/multiple inputs)
- NotImplementedError when yielder not overridden
All test logic is sound and assertions are appropriate.
Note on static analysis hints: The Ruff warnings about
ClassVarannotations are false positives in this test context. Test class attributes (likeinput_types,output_types,opts) are intentionally defined as mutable class attributes for each test case and don't needClassVarannotations since they're not shared across instances in a problematic way.secator/tasks/scapy.py (1)
9-10: Consider addingClassVarannotations for type safety.The mutable class attributes
input_typesandoutput_typesshould be annotated withtyping.ClassVarto indicate they are class-level attributes rather than instance attributes. This improves type safety and clarity.Apply this diff:
+from typing import ClassVar + @task() class scapy(PythonRunner): """Use Scapy to send an ARP request to a CIDR range and return the alive IPs.""" - input_types = [CIDR_RANGE, IP] - output_types = [Ip] + input_types: ClassVar = [CIDR_RANGE, IP] + output_types: ClassVar = [Ip]secator/runners/python.py (2)
37-38: Consider addingClassVarannotations for type safety.The mutable class attributes
tagsandoptsshould be annotated withtyping.ClassVarfor better type safety and to clearly indicate they are class-level attributes.Apply this diff:
"""Python runner for executing custom Python code.""" import logging +from typing import ClassVar from secator.config import CONFIG from secator.runners import Runner from secator.template import TemplateLoader ... class PythonRunner(Runner): ... default_exporters = CONFIG.tasks.exporters - tags = [] - opts = {} + tags: ClassVar = [] + opts: ClassVar = {} profile = 'io'
41-42: Unused parameter inneeds_chunkingmethod.The
syncparameter is not used in the method body. If this parameter is required for interface compatibility with the base class or future use, consider prefixing it with an underscore (_sync) to indicate it's intentionally unused.Apply this diff:
- def needs_chunking(self, sync): + def needs_chunking(self, _sync): return False
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
secator/cli.py(1 hunks)secator/cli_helper.py(1 hunks)secator/loader.py(1 hunks)secator/output_types/ip.py(3 hunks)secator/runners/__init__.py(1 hunks)secator/runners/python.py(1 hunks)secator/tasks/scapy.py(1 hunks)secator/template.py(1 hunks)secator/utils.py(1 hunks)tests/unit/test_python_runner.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
secator/runners/__init__.py (1)
secator/runners/python.py (1)
PythonRunner(12-123)
secator/tasks/scapy.py (3)
secator/runners/python.py (1)
PythonRunner(12-123)secator/output_types/ip.py (1)
Ip(16-45)secator/output_types/error.py (1)
Error(8-42)
secator/output_types/ip.py (1)
secator/utils.py (2)
rich_to_ansi(295-312)format_object(793-800)
tests/unit/test_python_runner.py (6)
secator/runners/python.py (2)
PythonRunner(12-123)yielder(104-113)secator/output_types/info.py (1)
Info(8-24)secator/output_types/url.py (1)
Url(15-110)secator/output_types/vulnerability.py (1)
Vulnerability(12-101)secator/output_types/tag.py (1)
Tag(9-69)secator/runners/_base.py (1)
errors(284-287)
secator/runners/python.py (3)
secator/runners/_base.py (1)
Runner(56-1231)secator/template.py (1)
TemplateLoader(11-55)secator/config.py (1)
get(226-249)
🪛 Flake8 (7.3.0)
secator/tasks/scapy.py
[error] 28-28: local variable 'e' is assigned to but never used
(F841)
🪛 Ruff (0.14.7)
secator/tasks/scapy.py
9-9: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
10-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
28-28: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
tests/unit/test_python_runner.py
16-16: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
35-35: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
65-65: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
66-68: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
87-87: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
100-100: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
120-120: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
137-137: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
secator/runners/python.py
37-37: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
38-38: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
41-41: Unused method argument: sync
(ARG002)
44-44: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
91-91: Unused static method argument: self
(ARG004)
98-98: Unused static method argument: self
(ARG004)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration (3.11, ubuntu-latest)
🔇 Additional comments (11)
secator/cli_helper.py (1)
130-130: LGTM! Defensive initialization prevents undefined variable.The explicit initialization of
input_types = []ensures it's defined before conditional branches at lines 149, 161, and 174 may assign it, preventing potentialUnboundLocalErrorat line 178.secator/runners/__init__.py (1)
7-7: LGTM! New PythonRunner properly exposed.The addition of
PythonRunnerto__all__and its import fromsecator.runners.pythoncorrectly expose the new public surface for Python-based tasks.Also applies to: 14-14
secator/loader.py (1)
49-50: LGTM! Defensive attribute access prevents AttributeError.Using
getattr(cls, 'cmd', None)andgetattr(cls, 'install_cmd', None)safely handles tasks (e.g., PythonRunner-based) that may not define these attributes, returningNoneinstead of raisingAttributeError.secator/output_types/ip.py (2)
7-7: LGTM! New field enables richer IP metadata.The addition of
extra_data: dictallows carrying protocol-specific metadata (e.g., ARP protocol info, RTT) alongside the IP entry, enhancing the Ip output type's expressiveness.Also applies to: 21-21
43-44: LGTM! Extra data properly rendered in representation.The
__repr__enhancement correctly displaysextra_datawhen present usingformat_object, maintaining consistency with the codebase's display patterns.secator/template.py (1)
184-184: LGTM! Defensive attribute access for meta_opts.Using
getattr(cls, 'meta_opts', {}).copy()safely handles tasks that don't definemeta_opts, returning an empty dict instead of raisingAttributeError. This aligns with the defensive access pattern used throughout this PR.secator/cli.py (1)
1627-1630: LGTM! Defensive handling of tasks without cmd attribute.The code now safely skips tasks that don't define a
cmdattribute (e.g., PythonRunner-based tasks) by usinggetattr(cls, 'cmd', None)and checking forNone. This preventsAttributeErrorduring the update process.secator/utils.py (1)
93-94: Path-handling logic change is correct and intentional.The expanded condition
if not input_types or 'path' in input_types:aligns with the Runner base class default ofinput_types = []. When a runner (task/scan/workflow) declares no specific input types, it should accept any input form—including paths passed directly. The change prevents unintended file reading for runners without declared input requirements. All examined task implementations explicitly declareinput_types, so existing tasks are unaffected. The behavior is safe and correct.secator/tasks/scapy.py (1)
1-5: LGTM!The initial imports are correctly organized and include all necessary components for defining a PythonRunner-based task.
secator/runners/python.py (2)
1-10: LGTM!Module setup follows standard conventions with appropriate docstring, imports, and logger initialization.
104-123: LGTM!The abstract
yieldermethod anddelayclassmethod for Celery integration are well-implemented. The dynamic profile handling indelayis a nice touch for flexible queue assignment.
| from scapy.all import ARP, Ether, srp | ||
| import ipaddress |
There was a problem hiding this comment.
Move imports to the top of the file.
Per PEP 8, all imports should be placed at the top of the file, before any code definitions. This improves code readability and makes dependencies explicit upfront.
Apply this diff:
from secator.runners import PythonRunner
from secator.definitions import CIDR_RANGE, IP
from secator.output_types import Ip, Error
from secator.decorators import task
+from scapy.all import ARP, Ether, srp
+import ipaddress
@task()
class scapy(PythonRunner):
"""Use Scapy to send an ARP request to a CIDR range and return the alive IPs."""
input_types = [CIDR_RANGE, IP]
output_types = [Ip]
def yielder(self):
for cidr_range in self.inputs:
for finding in arp_scan(cidr_range):
yield finding
-from scapy.all import ARP, Ether, srp
-import ipaddress
-
def arp_scan(network_range):📝 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.
| from scapy.all import ARP, Ether, srp | |
| import ipaddress | |
| from secator.runners import PythonRunner | |
| from secator.definitions import CIDR_RANGE, IP | |
| from secator.output_types import Ip, Error | |
| from secator.decorators import task | |
| from scapy.all import ARP, Ether, srp | |
| import ipaddress | |
| @task() | |
| class scapy(PythonRunner): | |
| """Use Scapy to send an ARP request to a CIDR range and return the alive IPs.""" | |
| input_types = [CIDR_RANGE, IP] | |
| output_types = [Ip] | |
| def yielder(self): | |
| for cidr_range in self.inputs: | |
| for finding in arp_scan(cidr_range): | |
| yield finding | |
| def arp_scan(network_range): |
🤖 Prompt for AI Agents
In secator/tasks/scapy.py around lines 17 to 18, the imports "from scapy.all
import ARP, Ether, srp" and "import ipaddress" must be moved to the top of the
file per PEP8; relocate these import statements to the module-level import
section (above any function/class definitions), group standard library and
third-party imports appropriately, and ensure there are no duplicate imports or
circular dependencies after the move.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.