Feature: validate targets before executing modules (-C) - #1163
Feature: validate targets before executing modules (-C)#1163Aarush289 wants to merge 39 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds optional pre-scan target validation through configuration and CLI settings. Targets are resolved concurrently, canonicalized, ordered, deduplicated, and invalid entries skipped before scanning. Application compare-report creation and exit-code return handling are also updated. ChangesPre-scan validation
Repository housekeeping
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 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 |
|
LGTM! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)nettacker/config.py(1 hunks)nettacker/core/app.py(3 hunks)nettacker/core/arg_parser.py(1 hunks)nettacker/core/hostcheck.py(1 hunks)nettacker/locale/en.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use 4-space indents in Python code
Limit lines to 99 characters (ruff/ruff-format/isort profile=black)
Module and file names should use lower_snake_case
Function and variable names should use lower_snake_case
Class names should use PascalCase
Constants should use UPPER_SNAKE_CASE
Keep functions small and add type hints where practical
Files:
nettacker/config.pynettacker/core/hostcheck.pynettacker/core/arg_parser.pynettacker/core/app.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/config.pynettacker/core/hostcheck.pynettacker/core/arg_parser.pynettacker/core/app.py
nettacker/config.py
📄 CodeRabbit inference engine (AGENTS.md)
Manage defaults (API key, DB path, paths) in nettacker/config.py and review sensitive headers list before logging
Files:
nettacker/config.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/hostcheck.pynettacker/core/arg_parser.pynettacker/core/app.py
🧠 Learnings (4)
📚 Learning: 2025-09-07T19:20:58.332Z
Learnt from: CR
Repo: OWASP/Nettacker PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-07T19:20:58.332Z
Learning: Applies to nettacker/config.py : Manage defaults (API key, DB path, paths) in nettacker/config.py and review sensitive headers list before logging
Applied to files:
nettacker/config.py
📚 Learning: 2025-09-07T19:20:58.332Z
Learnt from: CR
Repo: OWASP/Nettacker PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-07T19:20:58.332Z
Learning: Applies to nettacker/**/*.py : Add docstrings for public APIs in the nettacker package
Applied to files:
nettacker/config.py
📚 Learning: 2025-10-25T11:31:26.498Z
Learnt from: Aarush289
Repo: OWASP/Nettacker PR: 1155
File: nettacker/core/hostcheck.py:121-122
Timestamp: 2025-10-25T11:31:26.498Z
Learning: In nettacker/core/hostcheck.py, the resolve_quick() function intentionally applies timeout_sec per resolution pass (not cumulatively), allowing up to 2× timeout_sec total execution time across both AI_ADDRCONFIG passes by design.
Applied to files:
nettacker/core/hostcheck.pynettacker/core/app.py
📚 Learning: 2025-09-07T19:20:58.332Z
Learnt from: CR
Repo: OWASP/Nettacker PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-07T19:20:58.332Z
Learning: Applies to nettacker.py : Repository provides nettacker.py as an entry script (use as Python entry point)
Applied to files:
nettacker/core/app.py
🧬 Code graph analysis (3)
nettacker/core/hostcheck.py (1)
nettacker/core/ip.py (2)
is_single_ipv4(51-61)is_single_ipv6(88-98)
nettacker/core/arg_parser.py (1)
nettacker/config.py (1)
Config(184-188)
nettacker/core/app.py (1)
nettacker/core/hostcheck.py (1)
resolve_quick(54-92)
🪛 Ruff (0.14.2)
nettacker/core/app.py
302-302: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
302-302: Avoid specifying long messages outside the exception class
(TRY003)
323-323: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (1)
.gitignore (1)
34-36: Clarify the relationship between these artifacts and the target validation feature.The three new entries (
Public_sign,Public_sign.pub, andcks_proxy) are unclear in their connection to the stated PR objective (pre-scan target validation with DNS resolution). Additionally, best practices suggest that public key files (.pub) should not typically be gitignored—only private keys should be.Please clarify:
- What role do these artifacts play in the target validation workflow?
- Is
Public_signthe private key andPublic_sign.pubthe public counterpart? If so, onlyPublic_signshould be gitignored.- What is
cks_proxyused for?
|
Hi @securestep9 , gentle nudge for a review whenever you get a chance. Happy to address any feedback. Thanks! |
b004b4c to
c8c4b5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (5)
nettacker/core/parser.py (5)
58-87: Consider fixing PEP 8 spacing issues.Multiple spacing inconsistencies throughout the function:
- Line 59:
line=line.strip()should beline = line.strip()- Lines 62, 71:
if(should beif (- Lines 74, 81:
else :should beelse:
100-103: Use singular variable name for clarity.The loop variable
portson line 102 represents a single port number, so it should be namedportfor clarity.- for ports in range (int(start),int(end)+1): - final_list.append(ports) + for port in range(int(start), int(end) + 1): + final_list.append(port)
236-243: Use consistent snake_case naming.Variable
Ignore_caseon line 237 uses inconsistent capitalization. Per coding guidelines, variables should usesnake_case.Based on coding guidelines: "Function and variable names should use lower_snake_case."
Apply this diff:
new_line_specifier=False - Ignore_case = False + ignore_case = False for c in options: if c == 's': new_line_specifier=True if c == 'i': - Ignore_case = True + ignore_case = Truesig={ "type":sig_type, "service":service, "regex":regex, - "Ignore_case":Ignore_case, - "New_line_specifier":new_line_specifier, + "ignore_case":ignore_case, + "new_line_specifier":new_line_specifier,Also applies to: 312-313
128-335: Consider breaking down the complex function.This 207-line function handles multiple parsing responsibilities. Consider extracting helper functions like
_parse_probe_header(),_parse_signature(), and_parse_version_fields()to improve testability and maintainability.
347-351: Consider adding command-line argument support.The
__main__block uses hardcoded filenames and has no error handling. For better usability and robustness, consider usingargparseto accept file paths as arguments and wrap the calls in try-except blocks.Example:
if __name__ == "__main__": import argparse parser = argparse.ArgumentParser(description="Parse nmap-service-probes file") parser.add_argument("input", help="Input probe file path") parser.add_argument("output", help="Output YAML file path") args = parser.parse_args() try: probes, excluded_ports = parse_probe_file(args.input) write_yaml(probes, excluded_ports, args.output) print(f"Successfully wrote {len(probes)} probes to {args.output}") except Exception as e: print(f"Error: {e}", file=sys.stderr) sys.exit(1)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nettacker/core/parser.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use 4-space indents in Python code
Limit lines to 99 characters (ruff/ruff-format/isort profile=black)
Module and file names should use lower_snake_case
Function and variable names should use lower_snake_case
Class names should use PascalCase
Constants should use UPPER_SNAKE_CASE
Keep functions small and add type hints where practical
Files:
nettacker/core/parser.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/parser.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/parser.py
🧬 Code graph analysis (1)
nettacker/core/parser.py (1)
tests/core/test_exclude_ports.py (1)
options(18-19)
🪛 Ruff (0.14.8)
nettacker/core/parser.py
161-161: Found useless expression. Either assign it to a variable or remove it.
(B018)
🔇 Additional comments (1)
nettacker/core/parser.py (1)
145-146: Verify handling of multiple Exclude directives in probe files.Line 146 assigns the result of
parse_excluded(line)toexcluded_ports, which will overwrite any previously parsed exclusions. If the probe file format allows multipleExcludedirectives, they should be merged rather than replaced. Confirm the intended behavior by checking: (1) whether probe files can contain multipleExcludelines, (2) the data structure returned byparse_excluded(), and (3) howexcluded_portsis used after being assigned.
18c2a71 to
e847178
Compare
… not and changes are done accordingly in app.py to drop buggy target names . e.g. 465.54.543.35 , google , ajkbfdsv etc.
Signed-off-by: Aarush289 <cs24b064@smail.iitm.ac.in>
Signed-off-by: Aarush289 <cs24b064@smail.iitm.ac.in>
…ased name Signed-off-by: Aarush289 <cs24b064@smail.iitm.ac.in>
Signed-off-by: Aarush289 <cs24b064@smail.iitm.ac.in>
Signed-off-by: Aarush289 <cs24b064@smail.iitm.ac.in>
Signed-off-by: Aarush289 <cs24b064@smail.iitm.ac.in>
Signed-off-by: Aarush289 <cs24b064@smail.iitm.ac.in>
Signed-off-by: Aarush289 <cs24b064@smail.iitm.ac.in>
Signed-off-by: Aarush289 <cs24b064@smail.iitm.ac.in>
Signed-off-by: Aarush289 <cs24b064@smail.iitm.ac.in>
Signed-off-by: Aarush289 <cs24b064@smail.iitm.ac.in>
Signed-off-by: Aarush289 <cs24b064@smail.iitm.ac.in>
Signed-off-by: Aarush289 <cs24b064@smail.iitm.ac.in>
Signed-off-by: Aarush289 <cs24b064@smail.iitm.ac.in>
Signed-off-by: Aarush289 <cs24b064@smail.iitm.ac.in>
Signed-off-by: Aarush289 <cs24b064@smail.iitm.ac.in>
Signed-off-by: Aarush289 <cs24b064@smail.iitm.ac.in>
Signed-off-by: Aarush289 <cs24b064@smail.iitm.ac.in>
Signed-off-by: Aarush289 <cs24b064@smail.iitm.ac.in>
Signed-off-by: Aarush <cs24b064@smail.iitm.ac.in>
Signed-off-by: Aarush <cs24b064@smail.iitm.ac.in>
Signed-off-by: Aarush <cs24b064@smail.iitm.ac.in>
e76f633 to
bd38ad4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
nettacker/core/app.py (2)
342-349: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReusing
parallel_module_scanto size the DNS-validation thread pool conflates two unrelated concurrency knobs.
parallel_module_scangoverns concurrent module-scan threads; passing it asmax_threadshere means bumping module concurrency also scales concurrent DNS lookups, which may not be the intended coupling (and skips themin(len(targets), 10)cap applied only in theNonedefault path). Consider a dedicated setting/cap for validation concurrency instead.🤖 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 `@nettacker/core/app.py` around lines 342 - 349, Update scan_target_group so filter_valid_targets no longer uses parallel_module_scan to set DNS-validation max_threads. Introduce or reuse a dedicated validation-concurrency setting, or preserve the capped default behavior (min(len(targets), 10)) when none is configured, while leaving module-scan concurrency independent.
292-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type hints to the new public method.
filter_valid_targetshas a docstring but no type hints on its parameters/return, which the coding guidelines recommend fornettacker/**/*.py.- def filter_valid_targets(self, targets, timeout_per_target=2.0, max_threads=None, dedupe=True): + def filter_valid_targets( + self, + targets: Iterable[str], + timeout_per_target: float = 2.0, + max_threads: int | None = None, + dedupe: bool = True, + ) -> list[str]:As per coding guidelines, "Keep functions small, use type hints where practical, and add docstrings for public APIs."
🤖 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 `@nettacker/core/app.py` at line 292, Add type annotations to the public method filter_valid_targets for all parameters, including the targets collection and optional timeout_per_target, max_threads, and dedupe values, and annotate its return type. Use existing project typing conventions and preserve the current behavior and docstring.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 `@nettacker/core/app.py`:
- Around line 298-301: Update the exception handler around the targets
conversion to bind the caught TypeError and explicitly chain the replacement
TypeError with “from err” (or intentionally suppress context with “from None”),
preserving the existing message and validation behavior.
- Around line 297-341: Update the per-target execution in _task or its
fut.result() handling so unexpected exceptions from resolve_quick are caught for
that worker, logged as invalid targets, and represented as a dropped target.
Preserve processing of other futures and ensure filter_valid_targets completes
without aborting scan_target_group because of one target failure.
---
Nitpick comments:
In `@nettacker/core/app.py`:
- Around line 342-349: Update scan_target_group so filter_valid_targets no
longer uses parallel_module_scan to set DNS-validation max_threads. Introduce or
reuse a dedicated validation-concurrency setting, or preserve the capped default
behavior (min(len(targets), 10)) when none is configured, while leaving
module-scan concurrency independent.
- Line 292: Add type annotations to the public method filter_valid_targets for
all parameters, including the targets collection and optional
timeout_per_target, max_threads, and dedupe values, and annotate its return
type. Use existing project typing conventions and preserve the current behavior
and docstring.
🪄 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: 59d96cd2-b557-4923-9ed8-60bd4b6a8b9f
📒 Files selected for processing (6)
.gitignorenettacker/config.pynettacker/core/app.pynettacker/core/arg_parser.pynettacker/core/hostcheck.pynettacker/locale/en.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- nettacker/config.py
- nettacker/core/arg_parser.py
- nettacker/locale/en.yaml
- nettacker/core/hostcheck.py
| try: | ||
| targets = list(targets) | ||
| except TypeError: | ||
| raise TypeError(f"`targets` must be iterable, got {type(targets).__name__}") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Chain the re-raised exception.
Static analysis flags this: re-raising TypeError inside an except TypeError block without from err/from None obscures the original traceback context.
🐛 Proposed fix
try:
targets = list(targets)
- except TypeError:
- raise TypeError(f"`targets` must be iterable, got {type(targets).__name__}")
+ except TypeError as err:
+ raise TypeError(
+ f"`targets` must be iterable, got {type(targets).__name__}"
+ ) from err📝 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.
| try: | |
| targets = list(targets) | |
| except TypeError: | |
| raise TypeError(f"`targets` must be iterable, got {type(targets).__name__}") | |
| try: | |
| targets = list(targets) | |
| except TypeError as err: | |
| raise TypeError( | |
| f"`targets` must be iterable, got {type(targets).__name__}" | |
| ) from err |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 301-301: 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nettacker/core/app.py` around lines 298 - 301, Update the exception handler
around the targets conversion to bind the caught TypeError and explicitly chain
the replacement TypeError with “from err” (or intentionally suppress context
with “from None”), preserving the existing message and validation behavior.
Source: Linters/SAST tools
|
@codex review this PR using skill: https://github.com/nettacker-codex-ai/skills/blob/main/pr-review-recommender/SKILL.md |
![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: e04d1d01c0
ℹ️ 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".
| with concurrent.futures.ThreadPoolExecutor(max_workers=1) as ex: | ||
| fut = ex.submit(_call, use_ai) | ||
| fut.result(timeout=timeout_sec) # raises on timeout or error |
There was a problem hiding this comment.
When getaddrinfo blocks longer than timeout_sec (for example, with a stalled DNS resolver), fut.result() raises TimeoutError, but exiting this executor context invokes shutdown(wait=True) and blocks until the lookup actually returns before trying the second lookup. A mocked 250 ms lookup with a 30 ms timeout therefore takes about 500 ms, and a permanently stuck resolver can stall every -C scan indefinitely; use an isolation mechanism that does not synchronously join the timed-out operation.
Useful? React with 👍 / 👎.
| def filter_valid_targets(self, targets, timeout_per_target=2.0, max_threads=None, dedupe=True): | ||
| """ | ||
| Parallel validation of targets via resolve_quick(target, timeout_sec). | ||
| Returns a list of canonical targets (order preserved, invalids removed). |
There was a problem hiding this comment.
This introduces hostname parsing, two-stage DNS resolution, timeout handling, deduplication, and scan-pipeline filtering without any corresponding tests, despite the repository explicitly requiring tests for new features. Add focused tests for successful, invalid, and timed-out resolutions plus scan_target_group filtering so regressions such as the ineffective timeout above are caught.
AGENTS.md reference: AGENTS.md:L27-L31
Useful? React with 👍 / 👎.
| if canon: | ||
| validated_target[idx] = canon |
There was a problem hiding this comment.
When -C is used with a hostname whose spelling changes during canonicalization, such as Example.COM or a trailing-dot name, this child process scans and stores discovery events under the lowercased/cleaned target while the parent retains the original target. The subsequent find_events(target, "port_scan", scan_id) lookup in expand_targets is an exact, case-sensitive target match, so default service discovery finds no event and removes an otherwise live host before the requested modules run; either propagate canonical targets to the parent before grouping or retain the original spelling for event keys.
Useful? React with 👍 / 👎.
| return filtered | ||
|
|
||
| def scan_target_group(self, targets, scan_id, process_number): | ||
| if not self.arguments.socks_proxy and self.arguments.validate_before_scan: |
There was a problem hiding this comment.
For /new/scan requests that explicitly submit validate_before_scan=false, Flask supplies the nonempty string "false"; api/engine.py only normalizes skip_service_discovery before constructing the SimpleNamespace, so this condition treats that value as true and unexpectedly enables target filtering. Normalize this new form field using the endpoint's existing boolean convention before evaluating it.
Useful? React with 👍 / 👎.
| timeout_per_target=2.0, | ||
| max_threads=self.arguments.parallel_module_scan or None, | ||
| dedupe=True, |
There was a problem hiding this comment.
When users set a large -M/--parallel-module-scan value for module throughput, this passes that unbounded value directly to the validation executor, bypassing the ten-worker cap used only when max_threads is None. Each validation worker then creates another one-thread executor in resolve_quick, and this occurs in every target-group process, so a large target list and -M value can create thousands of threads and exhaust memory or thread limits before scanning starts; apply a dedicated bounded validation-worker limit.
Useful? React with 👍 / 👎.
| if is_single_ipv4(host) or is_single_ipv6(host): # IP literal, no resolution needed | ||
| return True, host |
There was a problem hiding this comment.
When -C is used with an offline or unroutable IP literal, this early return marks it valid without performing any network operation, so the target is still passed to every requested module. DNS success for hostnames likewise establishes only name resolution, not connectivity, which conflicts with the new CLI help promising that unreachable targets are skipped; either add an actual bounded reachability probe or describe this option strictly as hostname-syntax/DNS validation.
Useful? React with 👍 / 👎.
3 participants
Proposed change
This PR adds an optional target validation step before running any scans.
When
-C(or--validate-before-scan) is used, Nettacker resolves each target first and filters out invalid or unreachable ones.This improves performance and reduces unnecessary resource usage by skipping modules for targets that cannot be resolved.
If
-Cis not used, behavior remains unchanged and all targets are scanned as before.No breaking changes.
Type of change
Checklist
make pre-commit, it didn't generate any changesmake test, all tests passed locally