Improve robustness: canonicalize & validate targets before scan #1155
Conversation
… 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 <[email protected]>
Summary by CodeRabbit
WalkthroughThis pull request introduces target validation and hostname resolution utilities to the nettacker application. It adds a new module for DNS resolution with timeout handling, integrates parallel target validation into the scan workflow, and updates build configuration files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 5
🧹 Nitpick comments (6)
nettacker/core/hostcheck.py (4)
35-51: Tighten exception handling and clean up one-liners.Catching bare
Exceptionand using semicolons harms readability and linting. Catch OS errors only and expand statements.def _system_search_suffixes() -> list[str]: @@ - try: - with open("/etc/resolv.conf") as f: - for line in f: - line = line.strip() - if not line or line.startswith("#"): continue - if line.startswith("search") or line.startswith("domain"): - sufs += [x for x in line.split()[1:] if x] - except Exception: - pass - seen = set(); out: list[str] = [] + try: + with open("/etc/resolv.conf") as f: + for line in f: + line = line.strip() + if not line or line.startswith("#"): + continue + if line.startswith("search") or line.startswith("domain"): + sufs += [x for x in line.split()[1:] if x] + except OSError: + pass + seen: set[str] = set() + out: list[str] = [] for s in sufs: - if s not in seen: - seen.add(s); out.append(s) + if s not in seen: + seen.add(s) + out.append(s) return out
13-21: Style and hints: remove semicolons; add basic docstrings.
- Avoid multiple statements per line.
- Add succinct docstrings for public APIs.
def is_ip_literal(name: str) -> bool: - try: - socket.inet_pton(socket.AF_INET, name); return True + """Return True if name is a valid IPv4 or IPv6 address literal.""" + try: + socket.inet_pton(socket.AF_INET, name) + return True except OSError: pass try: - socket.inet_pton(socket.AF_INET6, name); return True + socket.inet_pton(socket.AF_INET6, name) + return True except OSError: return False @@ -def _gai_once(name: str, use_ai_addrconfig: bool, port): +def _gai_once(name: str, use_ai_addrconfig: bool, port): + """Single getaddrinfo attempt with optional AI_ADDRCONFIG.""" flags = getattr(socket, "AI_ADDRCONFIG", 0) if use_ai_addrconfig else 0 return socket.getaddrinfo( name, port, socket.AF_UNSPEC, socket.SOCK_STREAM, 0, flags )Also applies to: 55-60
61-65: Add a docstring describing timeout/pass behavior.Clarify that there are two passes (AI_ADDRCONFIG on/off) and total runtime can be up to ~2× timeout.
def resolve_quick(host: str, timeout_sec: float = 2.0, try_search_suffixes: bool = True, allow_single_label: bool = True) -> tuple[bool, str | None]: + """ + Attempt a fast DNS-style resolution and return (ok, canonical_host). + Two passes are tried (AI_ADDRCONFIG on, then off). Each pass has its own + deadline of `timeout_sec`, so worst-case runtime ≈ 2 × timeout. + """
82-89: Minor cleanups: dedupe and one-liners.Use
dict.fromkeysfor stable dedupe and avoid one-liners.- seen, uniq = set(), [] - for c in candidates: - if c not in seen: - seen.add(c); uniq.append(c) - candidates = uniq + candidates = list(dict.fromkeys(candidates)) @@ - # cancel stragglers - for p in pending: p.cancel() + for p in pending: + p.cancel() @@ - for f in fut2cand: f.cancel() + for f in fut2cand: + f.cancel()Also applies to: 112-113, 119-119
nettacker/core/app.py (2)
346-352: PEP8: simplify condition; align call.Use
if not ...:and fix indentation.- if(not self.arguments.socks_proxy): - targets = self.filter_valid_targets( - targets, - timeout_per_target=2.0, - max_workers=self.arguments.parallel_module_scan or None, - dedupe=True, - ) + if not self.arguments.socks_proxy: + targets = self.filter_valid_targets( + targets, + timeout_per_target=2.0, + max_workers=self.arguments.parallel_module_scan or None, + dedupe=True, + )
26-26: Import only what you use.If
is_ip_literal/valid_hostnameare unused here, drop them to reduce surface and lint noise.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nettacker/core/app.py(4 hunks)nettacker/core/hostcheck.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/app.pynettacker/core/hostcheck.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/app.pynettacker/core/hostcheck.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/app.pynettacker/core/hostcheck.py
🧬 Code graph analysis (1)
nettacker/core/app.py (1)
nettacker/core/hostcheck.py (3)
resolve_quick(61-120)is_ip_literal(13-21)valid_hostname(23-33)
🪛 Ruff (0.14.1)
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)
322-322: Do not catch blind exception: Exception
(BLE001)
nettacker/core/hostcheck.py
15-15: Multiple statements on one line (semicolon)
(E702)
15-15: Consider moving this statement to an else block
(TRY300)
19-19: Multiple statements on one line (semicolon)
(E702)
19-19: Consider moving this statement to an else block
(TRY300)
42-42: Multiple statements on one line (colon)
(E701)
45-46: try-except-pass detected, consider logging the exception
(S110)
45-45: Do not catch blind exception: Exception
(BLE001)
47-47: Multiple statements on one line (semicolon)
(E702)
50-50: Multiple statements on one line (semicolon)
(E702)
64-64: Unused function argument: allow_single_label
(ARG001)
85-85: Multiple statements on one line (semicolon)
(E702)
90-90: Loop control variable pass_ix not used within loop body
Rename unused pass_ix to _pass_ix
(B007)
109-109: Abstract raise to an inner function
(TRY301)
109-109: Avoid specifying long messages outside the exception class
(TRY003)
112-112: Multiple statements on one line (colon)
(E701)
114-117: try-except-continue detected, consider logging the exception
(S112)
114-114: Do not catch blind exception: Exception
(BLE001)
114-114: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
119-119: Multiple statements on one line (colon)
(E701)
🔇 Additional comments (1)
nettacker/core/app.py (1)
310-343: Order-preserving dedupe is good; consider lowercase canonicalization at source.
filter_valid_targetskeeps order and dedupes. Ensureresolve_quicklowercases hostnames so dedupe is case-insensitive (see hostcheck comment). No code change needed here if upstream is fixed.
Signed-off-by: Aarush289 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
nettacker/core/hostcheck.py (3)
62-62: Honor theallow_single_labelparameter.The
allow_single_labelparameter is declared but never checked (flagged by ruff ARG001). Ifallow_single_label=False, the function should reject single-label hostnames early.Apply this diff to honor the parameter:
else: # single label (e.g., "intranet") + if not allow_single_label: + return False, None if try_search_suffixes:Also applies to: 72-78
111-111: Canonicalize hostname before returning.Hostnames should be normalized to lowercase for consistent matching and deduplication downstream. The current return statement omits this canonicalization step.
Apply this diff:
chosen = fut2cand[fut] # cancel stragglers for p in pending: p.cancel() - return True, (chosen[:-1] if chosen.endswith(".") else chosen) + canon = chosen[:-1] if chosen.endswith(".") else chosen + return True, canon.lower()
88-118: Prevent ThreadPoolExecutor from blocking on early return.Returning inside the
with ThreadPoolExecutor(...)block triggersshutdown(wait=True), which blocks until all in-flightgetaddrinfocalls finish. This negates the "first success wins" optimization and can add full timeout delays per candidate. Additionally,max_workers=len(candidates)can spawn excessive threads whenresolve_quickis called concurrently fromfilter_valid_targets.Refactor to manage the executor explicitly and call
shutdown(wait=False, cancel_futures=True)before returning:- for pass_ix, (use_ai_addrconfig, port) in enumerate(((True, None), (False, None))): + for _pass_ix, (use_ai_addrconfig, port) in enumerate(((True, None), (False, None))): deadline = time.monotonic() + timeout_sec - with concurrent.futures.ThreadPoolExecutor(max_workers=len(candidates)) as ex: - fut2cand = {ex.submit(_gai_once, c, use_ai_addrconfig, port): c for c in candidates} + max_workers = min(len(candidates), 8) + ex = concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) + try: + fut2cand = {ex.submit(_gai_once, c, use_ai_addrconfig, port): c for c in candidates} pending = set(fut2cand) while pending: remaining = deadline - time.monotonic() if remaining <= 0: break done, pending = concurrent.futures.wait( pending, timeout=remaining, return_when=concurrent.futures.FIRST_COMPLETED ) for fut in done: try: res = fut.result() # success if no exception - # Optional: assert non-empty result if not res: - # treat as failure - raise RuntimeError("empty getaddrinfo result") + continue chosen = fut2cand[fut] - # cancel stragglers - for p in pending: p.cancel() - return True, (chosen[:-1] if chosen.endswith(".") else chosen) - except Exception as e: - # TEMPORARY DEBUG LOGS — remove later - # print(f"[pass {pass_ix}] {fut2cand[fut]} -> {type(e).__name__}: {e!s}") + for p in pending: + p.cancel() + # non-blocking shutdown before returning + try: + ex.shutdown(wait=False, cancel_futures=True) + except TypeError: # Python <3.9 lacks cancel_futures + ex.shutdown(wait=False) + canon = chosen[:-1] if chosen.endswith(".") else chosen + return True, canon.lower() + except Exception: continue - # cancel any survivors in this pass - for f in fut2cand: f.cancel() + for f in fut2cand: + f.cancel() + finally: + try: + ex.shutdown(wait=False, cancel_futures=True) + except TypeError: + ex.shutdown(wait=False)This also renames
pass_ix→_pass_ix(unused) and removes the unused exception variablee.
🧹 Nitpick comments (2)
nettacker/core/hostcheck.py (2)
33-49: Consider adding a docstring and splitting multi-statement lines.While private, a brief docstring would clarify the function's purpose (parsing /etc/resolv.conf for DNS search suffixes). Additionally, splitting the semicolon-joined statements on lines 40, 45, and 48 would align with the project's style guidelines.
Example diff for docstring:
def _system_search_suffixes() -> list[str]: + """Parse /etc/resolv.conf and return DNS search suffixes.""" # Only used when host has no dots; mirrors OS resolver search behavior (UNIX).
53-57: Consider adding a docstring.A brief docstring would clarify the wrapper's purpose and the role of the
use_ai_addrconfigparameter.Example:
def _gai_once(name: str, use_ai_addrconfig: bool, port): + """Wrap socket.getaddrinfo with optional AI_ADDRCONFIG flag.""" flags = getattr(socket, "AI_ADDRCONFIG", 0) if use_ai_addrconfig else 0
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nettacker/core/hostcheck.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/hostcheck.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/hostcheck.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/hostcheck.py
🪛 Ruff (0.14.1)
nettacker/core/hostcheck.py
15-15: Multiple statements on one line (semicolon)
(E702)
15-15: Consider moving this statement to an else block
(TRY300)
19-19: Multiple statements on one line (semicolon)
(E702)
19-19: Consider moving this statement to an else block
(TRY300)
40-40: Multiple statements on one line (colon)
(E701)
43-44: try-except-pass detected, consider logging the exception
(S110)
43-43: Do not catch blind exception: Exception
(BLE001)
45-45: Multiple statements on one line (semicolon)
(E702)
48-48: Multiple statements on one line (semicolon)
(E702)
62-62: Unused function argument: allow_single_label
(ARG001)
83-83: Multiple statements on one line (semicolon)
(E702)
88-88: Loop control variable pass_ix not used within loop body
Rename unused pass_ix to _pass_ix
(B007)
107-107: Abstract raise to an inner function
(TRY301)
107-107: Avoid specifying long messages outside the exception class
(TRY003)
110-110: Multiple statements on one line (colon)
(E701)
112-115: try-except-continue detected, consider logging the exception
(S112)
112-112: Do not catch blind exception: Exception
(BLE001)
112-112: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
117-117: Multiple statements on one line (colon)
(E701)
…ased name Signed-off-by: Aarush289 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
nettacker/core/hostcheck.py (4)
13-21: Add docstring and refactor semicolon statements.The missing docstring and semicolon-joined statements flagged in the previous review remain unaddressed.
23-33: Add docstring for public API.The missing docstring flagged in the previous review remains unaddressed.
61-66: Add docstring for public API.The missing docstring flagged in the previous review remains unaddressed.
94-122: Early-return still blocks on executor threads; restructure required.The ThreadPoolExecutor context manager issue flagged in the previous review remains unaddressed. Returning inside
with ThreadPoolExecutor(...)triggersshutdown(wait=True), negating the "first-success wins" optimization and causing the function to wait for all in-flightgetaddrinfocalls. Additionally, usingmax_workers=len(candidates)can spawn excessive threads.
🧹 Nitpick comments (1)
nettacker/core/hostcheck.py (1)
35-51: Consider refactoring semicolon statements for readability.Lines 42, 47, and 50 use semicolons to join statements. While functional, splitting them improves readability.
- if not line or line.startswith("#"): continue + if not line or line.startswith("#"): + continue if line.startswith("search") or line.startswith("domain"): sufs += [x for x in line.split()[1:] if x] except Exception: pass - seen = set(); out: list[str] = [] + seen = set() + out: list[str] = [] for s in sufs: if s not in seen: - seen.add(s); out.append(s) + seen.add(s) + out.append(s)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nettacker/core/hostcheck.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/hostcheck.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/hostcheck.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/hostcheck.py
🪛 Ruff (0.14.1)
nettacker/core/hostcheck.py
15-15: Multiple statements on one line (semicolon)
(E702)
15-15: Consider moving this statement to an else block
(TRY300)
19-19: Multiple statements on one line (semicolon)
(E702)
19-19: Consider moving this statement to an else block
(TRY300)
42-42: Multiple statements on one line (colon)
(E701)
45-46: try-except-pass detected, consider logging the exception
(S110)
45-45: Do not catch blind exception: Exception
(BLE001)
47-47: Multiple statements on one line (semicolon)
(E702)
50-50: Multiple statements on one line (semicolon)
(E702)
89-89: Multiple statements on one line (semicolon)
(E702)
94-94: Loop control variable pass_ix not used within loop body
Rename unused pass_ix to _pass_ix
(B007)
112-112: Abstract raise to an inner function
(TRY301)
112-112: Avoid specifying long messages outside the exception class
(TRY003)
115-115: Multiple statements on one line (colon)
(E701)
118-119: try-except-continue detected, consider logging the exception
(S112)
118-118: Do not catch blind exception: Exception
(BLE001)
118-118: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
121-121: Multiple statements on one line (colon)
(E701)
🔇 Additional comments (1)
nettacker/core/hostcheck.py (1)
55-59: LGTM!Clean wrapper for
getaddrinfowith configurableAI_ADDRCONFIGflag.
Signed-off-by: Aarush289 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
nettacker/core/hostcheck.py (4)
13-21: Add docstring for public API.Per coding guidelines, public APIs in the nettacker package require docstrings. This was flagged in a previous review but remains unaddressed.
Apply this diff to add a docstring and improve readability:
def is_ip_literal(name: str) -> bool: + """Return True if name is a valid IPv4 or IPv6 address literal.""" try: - socket.inet_pton(socket.AF_INET, name); return True + socket.inet_pton(socket.AF_INET, name) + return True except OSError: pass try: - socket.inet_pton(socket.AF_INET6, name); return True + socket.inet_pton(socket.AF_INET6, name) + return True except OSError: return FalseAs per coding guidelines
30-32: Remove debug print statement.Line 31 contains
print("itegb")which pollutes production output. This critical issue was flagged in a previous review but remains unaddressed.Remove the debug code:
if len(parts) < 2 and not allow_single_label: - # log.warn("Its a name like google") - print("itegb") return False
23-33: Add docstring for public API.Per coding guidelines, public APIs in the nettacker package require docstrings. This was flagged in a previous review but remains unaddressed.
Apply this diff:
def valid_hostname(host: str, allow_single_label: bool = False) -> bool: + """ + Validate hostname syntax per RFC 1123. + + Args: + host: Hostname to validate. + allow_single_label: If True, accept single-label names (e.g., "localhost"). + + Returns: + True if the hostname is syntactically valid. + """ if len(host) > 253:As per coding guidelines
105-133: Early-return still blocks on executor threads; restructure and cap inner concurrency.Returning inside a
with ThreadPoolExecutor(...)triggersshutdown(wait=True)and waits for all in-flightgetaddrinfocalls. This negates "first-success wins" and can add full timeouts per candidate. Also, spinninglen(candidates)threads per resolve risks thread explosion when called in parallel fromfilter_valid_targets. This critical architectural issue was flagged in a previous review but remains unaddressed.Refactor to:
- Instantiate the executor explicitly and call
shutdown(wait=False, cancel_futures=True)before returning.- Cap inner
max_workersto a small value (e.g., 4–8) to bound threads.- Rename unused
pass_ixto_pass_ix.- Remove unused exception variable
e.Apply:
- for pass_ix, (use_ai_addrconfig, port) in enumerate(((True, None), (False, None))): + for _pass_ix, (use_ai_addrconfig, port) in enumerate(((True, None), (False, None))): deadline = time.monotonic() + timeout_sec - with concurrent.futures.ThreadPoolExecutor(max_workers=len(candidates)) as ex: - fut2cand = {ex.submit(_gai_once, c, use_ai_addrconfig, port): c for c in candidates} + maxw = min(len(candidates), 8) + ex = concurrent.futures.ThreadPoolExecutor(max_workers=maxw) + try: + fut2cand = {ex.submit(_gai_once, c, use_ai_addrconfig, port): c for c in candidates} pending = set(fut2cand) while pending: remaining = deadline - time.monotonic() if remaining <= 0: break done, pending = concurrent.futures.wait( pending, timeout=remaining, return_when=concurrent.futures.FIRST_COMPLETED ) for fut in done: try: - res = fut.result() # success if no exception - # Optional: assert non-empty result + res = fut.result() if not res: - # treat as failure - raise RuntimeError("empty getaddrinfo result") + continue chosen = fut2cand[fut] - # cancel stragglers - for p in pending: p.cancel() + for p in pending: + p.cancel() + try: + ex.shutdown(wait=False, cancel_futures=True) + except TypeError: # Python <3.9 + ex.shutdown(wait=False) canon = chosen[:-1] if chosen.endswith(".") else chosen return True, canon.lower() - except Exception as e: + except Exception: continue - # cancel any survivors in this pass - for f in fut2cand: f.cancel() + for f in fut2cand: + f.cancel() + finally: + try: + ex.shutdown(wait=False, cancel_futures=True) + except TypeError: # Python <3.9 + ex.shutdown(wait=False) return False, None
🧹 Nitpick comments (1)
nettacker/core/hostcheck.py (1)
35-51: Consider splitting semicolon-joined statements for consistency.The logic correctly extracts and deduplicates search suffixes from
/etc/resolv.conf. The blind exception handling on line 45 is appropriate since the file may not exist on all platforms.Optionally, split semicolon-joined statements for better consistency with project style:
if not line or line.startswith("#"): continue + if not line or line.startswith("#"): + continueseen = set(); out: list[str] = [] + seen = set() + out: list[str] = [] for s in sufs: if s not in seen: - seen.add(s); out.append(s) + seen.add(s) + out.append(s)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nettacker/core/hostcheck.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/hostcheck.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/hostcheck.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/hostcheck.py
🪛 Ruff (0.14.1)
nettacker/core/hostcheck.py
15-15: Multiple statements on one line (semicolon)
(E702)
15-15: Consider moving this statement to an else block
(TRY300)
19-19: Multiple statements on one line (semicolon)
(E702)
19-19: Consider moving this statement to an else block
(TRY300)
42-42: Multiple statements on one line (colon)
(E701)
45-46: try-except-pass detected, consider logging the exception
(S110)
45-45: Do not catch blind exception: Exception
(BLE001)
47-47: Multiple statements on one line (semicolon)
(E702)
50-50: Multiple statements on one line (semicolon)
(E702)
100-100: Multiple statements on one line (semicolon)
(E702)
105-105: Loop control variable pass_ix not used within loop body
Rename unused pass_ix to _pass_ix
(B007)
123-123: Abstract raise to an inner function
(TRY301)
123-123: Avoid specifying long messages outside the exception class
(TRY003)
126-126: Multiple statements on one line (colon)
(E701)
129-130: try-except-continue detected, consider logging the exception
(S112)
129-129: Do not catch blind exception: Exception
(BLE001)
129-129: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
132-132: Multiple statements on one line (colon)
(E701)
🔇 Additional comments (3)
nettacker/core/hostcheck.py (3)
1-12: LGTM!Imports and module constants are appropriate. The
_LABELregex correctly validates DNS labels per RFC 1123.
55-59: LGTM!The wrapper correctly applies
AI_ADDRCONFIGconditionally, with appropriate fallback handling viagetattr.
79-103: Candidate-building logic is sound.The hostname candidate generation correctly handles both dotted and single-label names, with appropriate suffix expansion and deduplication.
Signed-off-by: Aarush289 <[email protected]>
Signed-off-by: Aarush289 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
nettacker/core/app.py (1)
296-309: Wrap long signature and fix exception raising style.This is a duplicate of a previous review comment. The signature exceeds the 99-char limit and the
TypeErrorshould be raised withfrom Noneto satisfy B904.Apply this diff:
- def filter_valid_targets(self, targets, timeout_per_target=2.0, max_workers=None, dedupe=True): + def filter_valid_targets( + self, + targets, + timeout_per_target: float = 2.0, + max_workers: int | None = None, + dedupe: bool = True, + ) -> list[str]: """ Parallel validation of targets via resolve_quick(target, timeout_sec). Returns a list of canonical targets (order preserved, invalids removed). """ # Ensure it's a concrete list (len, indexing OK) try: targets = list(targets) except TypeError: - raise TypeError(f"`targets` must be iterable, got {type(targets).__name__}") + raise TypeError( + f"`targets` must be iterable, got {type(targets).__name__}" + ) from Nonenettacker/core/hostcheck.py (1)
45-46: Remove debug print statement.This is a duplicate of a previous critical issue. The
print("itegb")statement on line 46 is leftover debug code that pollutes production output and must be removed.Apply this diff:
if len(parts) < 2 and not allow_single_label: - # log.warn("Its a name like google") - print("itegb") return False
🧹 Nitpick comments (8)
nettacker/core/app.py (3)
26-26: Remove unused imports.
is_ip_literalandvalid_hostnameare imported but never used in this file. Onlyresolve_quickis actually called.Apply this diff:
-from nettacker.core.hostcheck import resolve_quick, is_ip_literal, valid_hostname +from nettacker.core.hostcheck import resolve_quick
210-210: Use professional, i18n-compatible log messages.The log message "Process started!!" is informal. Consider using the existing internationalization pattern with a message key (e.g.,
_("process_started")), consistent with other logs in this file like_("scan_started")on line 47.Apply this diff:
- log.info("Process started!!") + log.info(_("process_started"))(Similarly for "Process done!!" on lines 219 and 226.)
332-332: Consider using debug level for invalid target logs.Logging each invalid target at
infolevel could be noisy when processing many invalid targets. Consider usinglog.debug()instead, or add a summary count at info level after processing.- log.info(f"Invalid target -> dropping: {orig_target}") + log.debug(f"Invalid target -> dropping: {orig_target}")nettacker/core/hostcheck.py (5)
57-57: Split multiple statements on one line.Line 57 combines conditional and continue with a colon separator. Per Ruff E701, place
continueon a separate line for readability.- if not line or line.startswith("#"): continue + if not line or line.startswith("#"): + continue
62-65: Split semicolon-joined statements.Lines 62 and 65 use semicolons to join statements on one line. Per coding guidelines (E702), split these onto separate lines for better readability.
- seen = set(); out: list[str] = [] + seen = set() + out: list[str] = [] for s in sufs: if s not in seen: - seen.add(s); out.append(s) + seen.add(s) + out.append(s)
115-115: Split semicolon-joined statements.Line 115 uses a semicolon to join two statements. Per coding guidelines (E702), split onto separate lines.
- seen.add(c); uniq.append(c) + seen.add(c) + uniq.append(c)
120-120: Rename unused loop variable.The loop variable
pass_ixis not used within the loop body. Per Ruff B007, prefix with underscore to indicate it's intentionally unused.- for pass_ix, (use_ai_addrconfig, port) in enumerate(((True, None), (False, None))): + for _pass_ix, (use_ai_addrconfig, port) in enumerate(((True, None), (False, None))):
150-151: Consider logging failed resolution attempts.The bare
Exceptioncatch on line 150 silently continues, which might hide unexpected errors during development. Consider usinglog.debug()to capture failure details while maintaining resilience.except Exception: + log.debug(f"Resolution failed for candidate {fut2cand.get(fut)}: {exc!s}") continueNote: This requires importing the logger and binding the exception (e.g.,
except Exception as exc:).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nettacker/core/app.py(6 hunks)nettacker/core/hostcheck.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/app.pynettacker/core/hostcheck.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/app.pynettacker/core/hostcheck.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/app.pynettacker/core/hostcheck.py
🧬 Code graph analysis (1)
nettacker/core/app.py (1)
nettacker/core/hostcheck.py (3)
resolve_quick(76-161)is_ip_literal(13-24)valid_hostname(26-48)
🪛 Ruff (0.14.1)
nettacker/core/app.py
305-305: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
305-305: Avoid specifying long messages outside the exception class
(TRY003)
nettacker/core/hostcheck.py
17-17: Consider moving this statement to an else block
(TRY300)
22-22: Consider moving this statement to an else block
(TRY300)
57-57: Multiple statements on one line (colon)
(E701)
60-61: try-except-pass detected, consider logging the exception
(S110)
60-60: Do not catch blind exception: Exception
(BLE001)
62-62: Multiple statements on one line (semicolon)
(E702)
65-65: Multiple statements on one line (semicolon)
(E702)
115-115: Multiple statements on one line (semicolon)
(E702)
120-120: Loop control variable pass_ix not used within loop body
Rename unused pass_ix to _pass_ix
(B007)
150-151: try-except-continue detected, consider logging the exception
(S112)
150-150: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (2)
nettacker/core/hostcheck.py (2)
70-74: LGTM: Portable DNS lookup helper.The
getattrfallback forAI_ADDRCONFIGensures compatibility with platforms that don't support this flag. Clean implementation.
144-160: Well done: Non-blocking executor shutdown.The explicit non-blocking shutdown with
wait=False, cancel_futures=True(with Python 3.8 fallback) correctly addresses the previous review concern about blocking on in-flight DNS queries. This ensures the "first success wins" behavior works as intended.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
nettacker/core/hostcheck.py (1)
140-140: Fix indentation to 4 spaces.Line 140 appears to have inconsistent indentation (11 spaces instead of 12). Ensure consistent 4-space indentation.
Apply this diff:
chosen = fut2cand[fut] for p in pending: - p.cancel() + p.cancel()Based on coding guidelines (use 4-space indents).
🧹 Nitpick comments (3)
nettacker/core/hostcheck.py (3)
48-64: Refactor semicolons for readability and document platform limitation.The function logic is correct, but several style issues reduce readability:
- Lines 55, 60, 63: Multiple statements on one line with semicolons make the code harder to read.
- Unix-only implementation: The function only works on Unix-like systems with
/etc/resolv.conf. On Windows, it silently returns an empty list, which may cause single-label hostnames to fail resolution unexpectedly.Apply this diff to improve readability:
for line in f: line = line.strip() - if not line or line.startswith("#"): continue + if not line or line.startswith("#"): + continue if line.startswith("search") or line.startswith("domain"): sufs += [x for x in line.split()[1:] if x] except Exception: pass - seen = set(); out: list[str] = [] + seen = set() + out: list[str] = [] for s in sufs: if s not in seen: - seen.add(s); out.append(s) + seen.add(s) + out.append(s) return outConsider documenting the Unix-only behavior in a docstring or comment, or implement Windows support using platform-specific DNS configuration sources.
118-118: Rename unused loop variable.The loop control variable
pass_ixis not used within the loop body. Rename it to_pass_ixto indicate it's intentionally unused.Apply this diff:
- for pass_ix, (use_ai_addrconfig, port) in enumerate(((True, None), (False, None))): + for _pass_ix, (use_ai_addrconfig, port) in enumerate(((True, None), (False, None))):Based on coding guidelines (follow Python conventions and static analysis hints).
148-149: Narrow exception handling to avoid masking errors.The bare
except Exception:on lines 148-149 catches all exceptions fromfut.result(), which could inadvertently suppress unexpected errors. Consider catching specific exceptions likesocket.gaierrororOSErrorthatgetaddrinforaises, or at minimum add a comment explaining why broad catching is necessary.Apply this diff to narrow the exception handling:
try: res = fut.result() if not res: continue chosen = fut2cand[fut] for p in pending: p.cancel() # ensure we don't wait on the executor shutdown try: ex.shutdown(wait=False, cancel_futures=True) except TypeError: # Py<3.9 ex.shutdown(wait=False) canon = chosen[:-1] if chosen.endswith(".") else chosen return True, canon.lower() - except Exception: + except (OSError, socket.gaierror): + # DNS resolution failed for this candidate, try next continue
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nettacker/core/hostcheck.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/hostcheck.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/hostcheck.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/hostcheck.py
🪛 Ruff (0.14.1)
nettacker/core/hostcheck.py
17-17: Consider moving this statement to an else block
(TRY300)
22-22: Consider moving this statement to an else block
(TRY300)
55-55: Multiple statements on one line (colon)
(E701)
58-59: try-except-pass detected, consider logging the exception
(S110)
58-58: Do not catch blind exception: Exception
(BLE001)
60-60: Multiple statements on one line (semicolon)
(E702)
63-63: Multiple statements on one line (semicolon)
(E702)
113-113: Multiple statements on one line (semicolon)
(E702)
118-118: Loop control variable pass_ix not used within loop body
Rename unused pass_ix to _pass_ix
(B007)
148-149: try-except-continue detected, consider logging the exception
(S112)
148-148: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (5)
nettacker/core/hostcheck.py (5)
1-12: LGTM: Imports and regex pattern are appropriate.The imports and hostname label regex pattern are well-structured. The
_LABELpattern correctly implements RFC 1123 label validation.
13-24: LGTM: IP literal validation is correct.The function correctly validates both IPv4 and IPv6 address literals using
socket.inet_pton. Docstring is clear and complete.
26-46: LGTM: Hostname validation correctly implements RFC 1123.The validation logic is sound with proper length checks, trailing dot handling, and label validation. Docstring is complete and clear.
68-72: LGTM: Helper function correctly wraps getaddrinfo.The function safely handles the optional
AI_ADDRCONFIGflag and correctly passes parameters tosocket.getaddrinfo.
74-159: Well-implemented concurrent DNS resolution with proper cleanup.The function correctly implements timeout-aware concurrent DNS resolution with several good practices:
- Bounded concurrency (max 4 workers) prevents thread explosion.
- Deadline-based timeout enforcement across all candidates.
- Non-blocking executor shutdown on success and in cleanup.
- Proper future cancellation to avoid waiting for stragglers.
- Canonical hostname normalization (lowercase, no trailing dot).
- Python 3.7-3.8 compatibility with TypeError handling.
The two-pass approach (with/without
AI_ADDRCONFIG) provides good fallback behavior.
Signed-off-by: Aarush289 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
nettacker/core/hostcheck.py (6)
113-117: Remove semicolon; follow ruff E702 and project style.- for c in candidates: - if c not in seen: - seen.add(c); uniq.append(c) + for c in candidates: + if c not in seen: + seen.add(c) + uniq.append(c)
77-95: Clarify/enforce timeout semantics (per-pass vs total).Current code applies timeout per pass, but the docstring reads like a total budget. Choose one:
- Option A (update docstring to “per pass”).
- Option B (enforce a single total deadline across both passes — recommended).
I’m including a minimal diff for Option B.
def resolve_quick( @@ - Args: + Args: host: Hostname or IP literal to resolve. - timeout_sec: Maximum time to wait for resolution. + timeout_sec: Maximum total time budget across both passes. @@ - for _pass_ix, (use_ai_addrconfig, port) in enumerate(((True, None), (False, None))): - deadline = time.monotonic() + timeout_sec + deadline_total = time.monotonic() + timeout_sec + for _pass_ix, (use_ai_addrconfig, port) in enumerate(((True, None), (False, None))): @@ - while pending: - remaining = deadline - time.monotonic() + while pending: + remaining = deadline_total - time.monotonic() if remaining <= 0: break
95-112: Fast-path IP literals to avoid unnecessary DNS queries.candidates: list[str] = [] + # Fast-path: numeric IP literals don't need DNS + if is_ip_literal(host): + return True, host
48-60: Narrow exception handling for resolv.conf parsing; optional debug.Catching bare Exception and silently passing hides real errors. Limit to expected I/O/decoding issues; optionally add a debug log.
- try: - with open("/etc/resolv.conf") as f: + try: + with open("/etc/resolv.conf") as f: @@ - except Exception: - pass + except (OSError, UnicodeDecodeError): + # Optional: add debug log if module logger is available + # logger.debug("Could not read /etc/resolv.conf for search suffixes", exc_info=True) + pass
69-69: Remove leftover meta comment.Looks like an internal note from a refactor. Safe to delete.
-# --- safer, more robust pieces to replace in hostcheck.py ---
121-121: Drop enumerate since index is unused.Slight cleanup; avoids unnecessary tuple unpacking.
- for _pass_ix, (use_ai_addrconfig, port) in enumerate(((True, None), (False, None))): + for (use_ai_addrconfig, port) in ((True, None), (False, None)):
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nettacker/core/hostcheck.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/hostcheck.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/hostcheck.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/hostcheck.py
🪛 Ruff (0.14.1)
nettacker/core/hostcheck.py
17-17: Consider moving this statement to an else block
(TRY300)
22-22: Consider moving this statement to an else block
(TRY300)
59-60: try-except-pass detected, consider logging the exception
(S110)
59-59: Do not catch blind exception: Exception
(BLE001)
116-116: Multiple statements on one line (semicolon)
(E702)
Signed-off-by: Aarush289 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
nettacker/core/hostcheck.py (3)
13-24: Optional: Flatten exception handling with else blocks.The current pattern is clear and correct. However, ruff suggests using
elseblocks afterexceptto make the control flow more explicit (lines 17, 22). This is purely stylistic.Apply this diff if you prefer the explicit else pattern:
def is_ip_literal(name: str) -> bool: """Return True if name is a valid IPv4 or IPv6 address literal.""" try: socket.inet_pton(socket.AF_INET, name) - return True except OSError: - pass - try: - socket.inet_pton(socket.AF_INET6, name) - return True - except OSError: - return False + try: + socket.inet_pton(socket.AF_INET6, name) + except OSError: + return False + else: + return True + else: + return True
48-67: Note Unix-specific behavior; consider Windows compatibility.This function only works on Unix-like systems that have
/etc/resolv.conf. On Windows, it will silently return an empty list (caught by the exception handler). If Nettacker needs to support suffix search on Windows, you would need to query the registry or usesocket.getfqdn()behavior.If Windows support is needed, consider using platform-specific logic:
def _system_search_suffixes() -> list[str]: sufs: list[str] = [] if sys.platform == "win32": # Windows: could query registry or use alternative approach pass else: # Unix: read /etc/resolv.conf try: with open("/etc/resolv.conf") as f: for line in f: line = line.strip() if not line or line.startswith("#"): continue if line.startswith("search") or line.startswith("domain"): sufs += [x for x in line.split()[1:] if x] except Exception: pass # deduplicate...
77-163: Consider short-circuiting for IP literals.If
hostis already an IP address, callinggetaddrinfois unnecessary overhead. Since the module exportsis_ip_literal, consider checking it first and returning immediately. This avoids spawning threads and DNS timeouts for IP literals.Apply this diff:
def resolve_quick( host: str, timeout_sec: float = 2.0, try_search_suffixes: bool = True, allow_single_label: bool = True ) -> tuple[bool, str | None]: """ Perform fast DNS resolution with timeout and suffix search. Args: host: Hostname or IP literal to resolve. timeout_sec: Maximum time to wait for resolution. try_search_suffixes: If True, append /etc/resolv.conf search suffixes for single-label hosts. allow_single_label: If True, allow single-label hostnames (e.g., "intranet"). Returns: (True, canonical_hostname) on success, (False, None) on failure/timeout. """ + # Short-circuit if already an IP literal + if is_ip_literal(host): + return True, host.lower() + candidates: list[str] = []
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nettacker/core/hostcheck.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/hostcheck.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/hostcheck.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/hostcheck.py
🪛 Ruff (0.14.1)
nettacker/core/hostcheck.py
17-17: Consider moving this statement to an else block
(TRY300)
22-22: Consider moving this statement to an else block
(TRY300)
59-60: try-except-pass detected, consider logging the exception
(S110)
59-59: Do not catch blind exception: Exception
(BLE001)
116-116: Multiple statements on one line (semicolon)
(E702)
🔇 Additional comments (2)
nettacker/core/hostcheck.py (2)
26-46: LGTM!The hostname validation logic is correct, follows RFC 1123, and properly handles trailing dots before length validation. The docstring clearly documents the parameters and behavior.
71-75: LGTM!Clean wrapper around
getaddrinfowith proper handling of the optionalAI_ADDRCONFIGflag.
Signed-off-by: Aarush289 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
nettacker/core/hostcheck.py (1)
39-42: Length check before trimming breaks RFC compliance (edge case).The length validation (line 39) occurs before trimming the trailing dot (lines 41-42), incorrectly rejecting valid FQDNs. For example, a hostname of exactly 253 characters with a trailing dot (254 total) is valid but will be rejected before the dot is trimmed.
Apply this diff to check length after trimming:
- if len(host) > 253: - return False if host.endswith("."): host = host[:-1] + if len(host) > 253: + return False
🧹 Nitpick comments (1)
nettacker/core/hostcheck.py (1)
48-67: Consider logging exceptions for troubleshooting.The broad exception handler (lines 59-60) silently ignores errors when reading
/etc/resolv.conf. While this is acceptable for an optional configuration file, logging exceptions would help diagnose issues like permission problems or file corruption during troubleshooting.Optionally, add logging:
+ import logging + logger = logging.getLogger(__name__) + try: with open("/etc/resolv.conf") as f: ... - except Exception: + except Exception as e: + logger.debug(f"Could not read /etc/resolv.conf: {e}") pass
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nettacker/core/hostcheck.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/hostcheck.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/hostcheck.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/hostcheck.py
🧠 Learnings (1)
📚 Learning: 2025-10-25T11:31:26.476Z
Learnt from: Aarush289
PR: OWASP/Nettacker#1155
File: nettacker/core/hostcheck.py:121-122
Timestamp: 2025-10-25T11:31:26.476Z
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.py
🪛 Ruff (0.14.1)
nettacker/core/hostcheck.py
17-17: Consider moving this statement to an else block
(TRY300)
22-22: Consider moving this statement to an else block
(TRY300)
59-60: try-except-pass detected, consider logging the exception
(S110)
59-59: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (3)
nettacker/core/hostcheck.py (3)
1-12: LGTM!The imports and regex pattern for hostname label validation are appropriate and correctly implement RFC 1123 syntax requirements.
13-24: LGTM!The function correctly validates IPv4/IPv6 address literals using
socket.inet_pton. Docstring is clear, and previous style issues have been resolved.
71-164: LGTM! Significant improvements from previous review.The resolution logic is well-structured:
- Candidate building correctly handles dotted/single-label hostnames and search suffixes
- Explicit executor management with non-blocking shutdown prevents blocking on in-flight DNS calls
- Bounded concurrency (
maxw = min(len(candidates), 4)) prevents thread explosion- Per-pass timeout is intentional and allows fallback without AI_ADDRCONFIG
- Specific exception handling for DNS errors is appropriate
All major issues flagged in previous reviews have been addressed.
Signed-off-by: Aarush289 <[email protected]>
nettacker/core/hostcheck.py
Outdated
| # Only used when host has no dots; mirrors OS resolver search behavior (UNIX). | ||
| sufs: list[str] = [] | ||
| try: | ||
| with open("/etc/resolv.conf") as f: |
There was a problem hiding this comment.
@Aarush289 using /etc/resolv.conf here is not acceptable. This will not work on Windows and other non-Linux platforms and we have just spent lots of effort making Nettacker platform-independent
|
Can you also provide justification/reasoning behind this proposed PR. Nettacker mainly operates on Networks (as the name suggests) using IP ranges and subdomain discovery. While theoretically it can be fed whole bunch of unresolvable hostnames as targets our standard recommended way of validating them is to use ping_before_scan option which will filter out non-pingable hosts. I suggest that you re-work your proposal as an option e.g. validate_before_scan rather than modification of the core functionality. This way the users will have a choice to turn the validation on or off |
|
Hi @securestep9 , Thank you for the thoughtful review and for pointing out the OS-specific concerns and the risk of changing default behavior. You’re absolutely right—this should be an opt-in feature rather than a core change. A few notes on my original motivation:
As Per your guidance, I’ll rework the PR as an optional feature rather than modifying core flow. Concretely, I plan to:
Thank you again for the clear direction and for flagging the Windows issue—totally agree it shouldn’t be OS-specific. I’ll incorporate your feedback and resubmit as an opt-in feature with proper documentation. Best, |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
nettacker/core/app.py (2)
293-306: Fix type error raising style and wrap the long signature.Two issues remain from previous review:
- Raise from None: Line 302 should raise with
from Noneto satisfy B904 and keep tracebacks clean.- Line length: The signature at line 293 exceeds the 99-character limit and should be wrapped.
Apply this diff:
-def filter_valid_targets(self, targets, timeout_per_target=2.0, max_workers=None, dedupe=True): +def filter_valid_targets( + self, + targets, + timeout_per_target: float = 2.0, + max_workers: int | None = None, + dedupe: bool = True, +) -> list[str]: """ Parallel validation of targets via resolve_quick(target, timeout_sec). Returns a list of canonical targets (order preserved, invalids removed). """ # Ensure it's a concrete list (len, indexing OK) try: targets = list(targets) except TypeError: - raise TypeError(f"`targets` must be iterable, got {type(targets).__name__}") + raise TypeError( + f"`targets` must be iterable, got {type(targets).__name__}" + ) from None
345-351: Fix formatting and indentation.The formatting issues flagged in the previous review remain unaddressed:
- Line 345: Missing space after
ifkeyword — should beif notinstead ofif(not.- Lines 346-351: Inconsistent indentation in the function call.
Apply this diff:
- if(not self.arguments.socks_proxy and self.arguments.validate_before_scan): + if not self.arguments.socks_proxy and self.arguments.validate_before_scan: targets = self.filter_valid_targets( - targets, - timeout_per_target=2.0, - max_workers=self.arguments.parallel_module_scan or None, - dedupe=True, - ) + targets, + timeout_per_target=2.0, + max_workers=self.arguments.parallel_module_scan or None, + dedupe=True, + )
🧹 Nitpick comments (1)
nettacker/locale/ru.yaml (1)
35-35: Consider moving the entry next to related*_before_scanoptions for consistency.The locale entry is functionally correct and the Russian translation is appropriate. However, it's inserted at line 35 (between error-related entries) rather than grouped with the similar
ping_before_scanentry at line 134. Relocating it to maintain logical grouping of related feature flags would improve file maintainability and readability.Consider moving this line to be adjacent to
ping_before_scanat line 134 to keep related validation options grouped together.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.gitignore(1 hunks)nettacker/config.py(1 hunks)nettacker/core/app.py(4 hunks)nettacker/core/arg_parser.py(1 hunks)nettacker/core/hostcheck.py(1 hunks)nettacker/locale/ar.yaml(1 hunks)nettacker/locale/bn.yaml(1 hunks)nettacker/locale/de.yaml(1 hunks)nettacker/locale/el.yaml(1 hunks)nettacker/locale/en.yaml(1 hunks)nettacker/locale/es.yaml(1 hunks)nettacker/locale/fa.yaml(1 hunks)nettacker/locale/fr.yaml(1 hunks)nettacker/locale/hi.yaml(1 hunks)nettacker/locale/hy.yaml(1 hunks)nettacker/locale/id.yaml(1 hunks)nettacker/locale/it.yaml(1 hunks)nettacker/locale/iw.yaml(1 hunks)nettacker/locale/ja.yaml(1 hunks)nettacker/locale/ko.yaml(1 hunks)nettacker/locale/nl.yaml(1 hunks)nettacker/locale/ps.yaml(1 hunks)nettacker/locale/pt-br.yaml(1 hunks)nettacker/locale/ru.yaml(1 hunks)nettacker/locale/tr.yaml(1 hunks)nettacker/locale/ur.yaml(1 hunks)nettacker/locale/vi.yaml(1 hunks)nettacker/locale/zh-cn.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- nettacker/locale/nl.yaml
🧰 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/core/app.pynettacker/config.pynettacker/core/hostcheck.pynettacker/core/arg_parser.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/app.pynettacker/config.pynettacker/core/hostcheck.pynettacker/core/arg_parser.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/app.pynettacker/core/hostcheck.pynettacker/core/arg_parser.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
🧠 Learnings (1)
📚 Learning: 2025-10-25T11:31:26.498Z
Learnt from: Aarush289
PR: OWASP/Nettacker#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.py
🧬 Code graph analysis (3)
nettacker/core/app.py (1)
nettacker/core/hostcheck.py (3)
resolve_quick(76-128)is_ip_literal(24-35)valid_hostname(37-57)
nettacker/core/hostcheck.py (1)
nettacker/core/ip.py (8)
get_ip_range(29-48)generate_ip_range(7-26)is_single_ipv4(51-61)is_ipv4_range(64-73)is_ipv4_cidr(76-85)is_single_ipv6(88-98)is_ipv6_range(101-110)is_ipv6_cidr(113-122)
nettacker/core/arg_parser.py (1)
nettacker/config.py (1)
Config(184-188)
🪛 Ruff (0.14.1)
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)
nettacker/core/hostcheck.py
28-28: Consider moving this statement to an else block
(TRY300)
33-33: Consider moving this statement to an else block
(TRY300)
79-79: Unused function argument: try_search_suffixes
(ARG001)
🪛 YAMLlint (1.37.1)
nettacker/locale/el.yaml
[error] 27-27: duplication of key "validate_before_scan" in mapping
(key-duplicates)
🔇 Additional comments (28)
nettacker/locale/es.yaml (1)
22-22: Locale key is appropriately translated and correctly placed.The Spanish translation clearly describes the pre-scan validation behavior. No issues identified.
.gitignore (1)
34-36: Clarify the purpose of the newly added .gitignore entries.The three entries (
Public_sign,Public_sign.pub,cks_proxy) appear to be cryptographic keys and proxy-related artifacts. While they follow existing patterns, their purpose is not immediately clear from context. Confirm these are intentional and not temporary/debug artifacts.nettacker/locale/pt-br.yaml (1)
31-31: Portuguese translation is clear and consistent.The translation appropriately describes the pre-scan validation feature. No issues identified.
nettacker/locale/ur.yaml (1)
31-31: Urdu translation is properly formatted.The translation correctly describes the feature in Urdu. No issues identified.
nettacker/locale/bn.yaml (1)
24-24: Bengali translation is properly formatted.The translation appropriately describes the pre-scan validation feature. No issues identified.
nettacker/locale/hi.yaml (1)
25-25: Hindi translation is clear and properly formatted.The translation appropriately describes the feature. No issues identified.
nettacker/locale/iw.yaml (1)
30-30: Hebrew translation is properly formatted.The translation accurately conveys the feature intent. No issues identified.
nettacker/locale/fa.yaml (1)
23-23: Persian translation is properly formatted.The translation clearly describes the feature in Persian. No issues identified.
nettacker/locale/zh-cn.yaml (1)
26-26: Locale entry for new feature is good.The Chinese translation for
validate_before_scanis grammatically correct and clearly describes the pre-scan validation feature.nettacker/locale/ps.yaml (1)
25-25: Locale entry for new feature is good.The Pashto translation for
validate_before_scanappropriately describes the pre-scan validation behavior.nettacker/locale/hy.yaml (1)
26-26: Locale entry for new feature is good.The Armenian translation for
validate_before_scanclearly describes the pre-scan validation feature and its behavior.nettacker/locale/vi.yaml (1)
25-25: Locale entry for new feature is good.The Vietnamese translation for
validate_before_scanappropriately describes the pre-scan validation behavior and target skipping.nettacker/locale/ko.yaml (1)
29-29: Locale entry for new feature is good.The Korean translation for
validate_before_scanclearly describes the pre-scan validation feature and target skipping behavior.nettacker/locale/fr.yaml (1)
25-25: Locale entry for new feature is good.The French translation for
validate_before_scanis grammatically correct and accurately conveys the pre-scan validation behavior.nettacker/locale/id.yaml (1)
27-27: Locale entry for new feature is good.The Indonesian translation for
validate_before_scanaccurately describes the pre-scan validation feature and target skipping behavior.nettacker/locale/ar.yaml (1)
25-25: Locale entry for new feature is good.The Arabic translation for
validate_before_scanaccurately describes the pre-scan validation feature and unreachable target skipping.nettacker/locale/tr.yaml (1)
27-27: LGTM! Turkish locale entry added correctly.The Turkish translation for
validate_before_scanis properly formatted and aligns with the new pre-scan validation feature being introduced across all locales.nettacker/locale/de.yaml (1)
18-18: LGTM! German locale entry added correctly.The German translation for
validate_before_scanis well-formatted and consistent with the feature's intent to pre-validate targets before scanning.nettacker/locale/en.yaml (1)
14-14: LGTM! English locale entry is clear and well-positioned.The description clearly communicates the feature's purpose: pre-validating targets before scanning and skipping unreachable ones.
nettacker/config.py (1)
165-165: LGTM! Configuration attribute added with correct opt-in default.The
validate_before_scanattribute is properly added with aFalsedefault, making the pre-scan validation feature opt-in as agreed upon in the PR discussion. This preserves existing behavior by default while allowing users to enable validation via the CLI flag.nettacker/locale/it.yaml (1)
26-26: LGTM! Italian locale entry added correctly.The Italian translation for
validate_before_scanis well-formatted and accurately conveys the feature's behavior.nettacker/locale/ja.yaml (1)
28-28: LGTM! Japanese locale entry added correctly.The Japanese translation for
validate_before_scanis properly formatted and clearly describes the pre-scan validation behavior.nettacker/core/app.py (2)
10-10: LGTM!The concurrent.futures imports are correctly added for the parallel target validation feature.
26-26: LGTM!The hostcheck module imports are correctly added for target validation.
nettacker/core/hostcheck.py (4)
1-22: LGTM!Imports and setup are appropriate. The
_LABELregex correctly validates hostname labels per RFC 1123.
24-35: LGTM!The function correctly validates IPv4/IPv6 literals using
socket.inet_pton. Docstring is present and clear.
37-57: LGTM!The hostname validation logic correctly implements RFC 1123 requirements. Trailing dot handling and label validation are properly ordered.
60-74: LGTM!The helper functions are well-designed.
_gai_oncecorrectly wrapsgetaddrinfowith optional AI_ADDRCONFIG, and_clean_hostrobustly normalizes input.
Signed-off-by: Aarush289 <[email protected]>
Signed-off-by: Aarush289 <[email protected]>
Signed-off-by: Aarush289 <[email protected]>
Signed-off-by: Aarush289 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nettacker/core/hostcheck.py (1)
94-94: Fix formatting: missing space afterifkeyword.Line 94 uses
if(...)with parentheses directly after the keyword; Python style convention requires a space and prefers implicit condition boolean evaluation without parens.Apply this diff:
- if(is_ip_literal(host)): + if is_ip_literal(host):
🧹 Nitpick comments (1)
nettacker/core/hostcheck.py (1)
81-83: Update docstring: remove stale mention of "suffix search".The docstring (line 82) states "Perform fast DNS resolution with timeout and suffix search," but the
try_search_suffixesparameter was removed in prior commits. Update the docstring to reflect the current API.Apply this diff:
""" - Perform fast DNS resolution with timeout and suffix search. + Perform fast DNS resolution with timeout.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
nettacker/core/arg_parser.py(1 hunks)nettacker/core/hostcheck.py(1 hunks)nettacker/locale/el.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- nettacker/locale/el.yaml
- nettacker/core/arg_parser.py
🧰 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/hostcheck.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/hostcheck.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/hostcheck.py
🧠 Learnings (1)
📚 Learning: 2025-10-25T11:31:26.498Z
Learnt from: Aarush289
PR: OWASP/Nettacker#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.py
🧬 Code graph analysis (1)
nettacker/core/hostcheck.py (1)
nettacker/core/ip.py (2)
is_single_ipv4(51-61)is_single_ipv6(88-98)
🪛 Ruff (0.14.1)
nettacker/core/hostcheck.py
28-28: Consider moving this statement to an else block
(TRY300)
33-33: Consider moving this statement to an else block
(TRY300)
Signed-off-by: Aarush289 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
nettacker/core/hostcheck.py (5)
81-90: Clarify docstring: timeout is per pass and result is canonicalized.- Perform fast DNS resolution with timeout. + Perform fast DNS resolution with timeout. + The timeout applies per resolution pass (AI_ADDRCONFIG on, then off). @@ - timeout_sec: Maximum time to wait for resolution. + timeout_sec: Maximum time to wait per resolution pass. @@ - (True, host_name) on success, (False, None) on failure/timeout. + (True, canonical_host) on success (lowercased, trailing dot stripped), + (False, None) on failure/timeout across both passes.Based on learnings
100-105: Use consistent single‑label validation; remove redundant check.- if not valid_hostname(host): + if not valid_hostname(host, allow_single_label=allow_single_label): return False, None - - if "." not in host and not allow_single_label: - return False, None
92-96: Return canonicalized IP literal (lowercase) for consistency.- if is_single_ipv4(host) or is_single_ipv6(host): - if is_ip_literal(host): - return True, host - return False, None + if is_single_ipv4(host) or is_single_ipv6(host): + if is_ip_literal(host): + return True, host.lower() + return False, None
66-74: Align comments with behavior in _clean_host (no internal collapsing).-def _clean_host(s: str) -> str: - # remove surrounding quotes and whitespace, lone commas, repeated dots +def _clean_host(s: str) -> str: + # Remove surrounding quotes and whitespace s = s.strip().strip('"').strip("'") s = s.strip() # again, after quote strip - # drop trailing commas that often sneak in from CSV-like inputs + # Drop a trailing comma (CSV leftovers) if s.endswith(","): s = s[:-1].rstrip() - # collapse accidental spaces inside + # Note: does not collapse internal spaces or repeated dots return s
24-35: Minor style: prefer try/except/else for early returns.def is_ip_literal(name: str) -> bool: """Return True if name is a valid IPv4 or IPv6 address literal.""" - try: - socket.inet_pton(socket.AF_INET, name) - return True - except OSError: - pass - try: - socket.inet_pton(socket.AF_INET6, name) - return True - except OSError: - return False + try: + socket.inet_pton(socket.AF_INET, name) + except OSError: + pass + else: + return True + try: + socket.inet_pton(socket.AF_INET6, name) + except OSError: + return False + else: + return True
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nettacker/core/hostcheck.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/hostcheck.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/hostcheck.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/hostcheck.py
🧠 Learnings (1)
📚 Learning: 2025-10-25T11:31:26.498Z
Learnt from: Aarush289
PR: OWASP/Nettacker#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.py
🧬 Code graph analysis (1)
nettacker/core/hostcheck.py (1)
nettacker/core/ip.py (8)
get_ip_range(29-48)generate_ip_range(7-26)is_single_ipv4(51-61)is_ipv4_range(64-73)is_ipv4_cidr(76-85)is_single_ipv6(88-98)is_ipv6_range(101-110)is_ipv6_cidr(113-122)
🪛 Ruff (0.14.1)
nettacker/core/hostcheck.py
28-28: Consider moving this statement to an else block
(TRY300)
33-33: Consider moving this statement to an else block
(TRY300)
110-110: Local variable deadline is assigned to but never used
Remove assignment to unused variable deadline
(F841)
🔇 Additional comments (1)
nettacker/core/hostcheck.py (1)
50-57: Hostname validation looks good.Trailing-dot trim before length check and RFC 1123 label checks are correct.
Signed-off-by: Aarush289 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
nettacker/core/hostcheck.py (1)
24-35: LGTM! Optional: consider else block for style.The validation logic is correct and the docstring is clear. The static analysis hint (TRY300) suggests using
elseblocks instead of a secondtry, but the current pattern is explicit and readable.If you prefer a slightly more idiomatic style:
def is_ip_literal(name: str) -> bool: """Return True if name is a valid IPv4 or IPv6 address literal.""" try: socket.inet_pton(socket.AF_INET, name) return True except OSError: - pass - try: + try: - socket.inet_pton(socket.AF_INET6, name) - return True + socket.inet_pton(socket.AF_INET6, name) + return True - except OSError: - return False + except OSError: + return False
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nettacker/core/hostcheck.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/hostcheck.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/hostcheck.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/hostcheck.py
🧠 Learnings (1)
📚 Learning: 2025-10-25T11:31:26.498Z
Learnt from: Aarush289
PR: OWASP/Nettacker#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.py
🧬 Code graph analysis (1)
nettacker/core/hostcheck.py (1)
nettacker/core/ip.py (8)
get_ip_range(29-48)generate_ip_range(7-26)is_single_ipv4(51-61)is_ipv4_range(64-73)is_ipv4_cidr(76-85)is_single_ipv6(88-98)is_ipv6_range(101-110)is_ipv6_cidr(113-122)
🪛 Ruff (0.14.1)
nettacker/core/hostcheck.py
28-28: Consider moving this statement to an else block
(TRY300)
33-33: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (3)
nettacker/core/hostcheck.py (3)
37-57: LGTM!The hostname validation logic correctly implements RFC 1123 requirements, handles trailing dots before length validation (addressing previous feedback), and provides flexible single-label support.
60-74: LGTM!The helper functions are well-structured:
_gai_onceprovides a clean wrapper for getaddrinfo with AI_ADDRCONFIG support, and_clean_hosthandles common input normalization cases.
91-121: LGTM! Per-pass timeout correctly implemented.The DNS resolution logic is sound:
- Input normalization and validation prevent common issues
- Defensive double-check (netaddr validation + inet_pton) for IP literals is reasonable
- Per-pass timeout design (allowing up to 2× timeout_sec total) is intentional per learnings
continueonTimeoutErrorensures both passes are attempted- Returns canonical lowercase hostname on success
Signed-off-by: Aarush289 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nettacker/core/hostcheck.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/hostcheck.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/hostcheck.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/hostcheck.py
🧠 Learnings (1)
📚 Learning: 2025-10-25T11:31:26.498Z
Learnt from: Aarush289
PR: OWASP/Nettacker#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.py
🧬 Code graph analysis (1)
nettacker/core/hostcheck.py (1)
nettacker/core/ip.py (8)
get_ip_range(29-48)generate_ip_range(7-26)is_single_ipv4(51-61)is_ipv4_range(64-73)is_ipv4_cidr(76-85)is_single_ipv6(88-98)is_ipv6_range(101-110)is_ipv6_cidr(113-122)
🪛 Ruff (0.14.1)
nettacker/core/hostcheck.py
26-26: Consider moving this statement to an else block
(TRY300)
31-31: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (3)
nettacker/core/hostcheck.py (3)
22-33: LGTM! Past issues addressed.The function now includes a proper docstring and uses separate statements instead of semicolons. The logic correctly validates IPv4/IPv6 literals using
socket.inet_pton.
35-55: LGTM! Edge-case fix applied correctly.The function properly strips the trailing dot before checking length (line 48-49 before 50), which addresses the past edge-case issue. The docstring is clear and the RFC 1123 validation logic is correct.
107-119: LGTM! Timeout logic correctly implements per-pass behavior.The resolution logic correctly implements the intended per-pass timeout design from the retrieved learning:
- Each pass (lines 107) gets a fresh
timeout_secseconds (line 112)TimeoutErrornow triggerscontinue(line 115) instead of early return, allowing the second pass to execute- Clean implementation without manual deadline management
This addresses all past timeout-related concerns.
nettacker/core/app.py
Outdated
|
|
||
| if dedupe: | ||
| seen, unique = set(), [] | ||
| for c in filtered: |
There was a problem hiding this comment.
Consider better names, if iterator is not being used elsewhere then just use an "_"
nettacker/core/app.py
Outdated
| max_workers = min(len(targets), 10) # cap threads | ||
|
|
||
| # Preserve order | ||
| canon_by_index = [None] * len(targets) |
nettacker/core/app.py
Outdated
| try: | ||
| idx, orig_target, canon = fut.result() | ||
| except (OSError, socket.gaierror) as exc: | ||
| log.debug(f"Invalid target (resolver error): {exc!s}") |
There was a problem hiding this comment.
The logger doesn't have a .debug() method. Change this to .error().
nettacker/core/hostcheck.py
Outdated
| try: | ||
| socket.inet_pton(socket.AF_INET, name) | ||
| return True | ||
| except OSError: |
There was a problem hiding this comment.
exceptions are slower than if-else statements. You're better off with precompiling an IPv4 regex (IPv6 would be too complex) at least and not relying on a try-except block as part of the main execution flow.
| """ | ||
| if host.endswith("."): | ||
| host = host[:-1] | ||
| if len(host) > 253: |
There was a problem hiding this comment.
Do add a comment here to specify this comes from RFC1123 which specifies the number of characters to be 250 at max (without dots) and 253 with dots.
There was a problem hiding this comment.
You don't have to translate for all. Just en.yaml is enough, we'll leave the translations to the native speakers.
nettacker/core/hostcheck.py
Outdated
| ) | ||
|
|
||
| def _clean_host(s: str) -> str: | ||
| # remove surrounding quotes and whitespace, lone commas, repeated dots |
There was a problem hiding this comment.
Are you handling repeated dots?
There was a problem hiding this comment.
Actionable comments posted: 80
♻️ Duplicate comments (14)
.venv/bin/py.test (1)
1-8: Critical: Virtual environment file should not be committed.Same issue as other
.venv/files - this auto-generated wrapper contains hardcoded local paths and should not be in version control..venv/bin/pip3.10 (1)
1-8: Critical: Virtual environment file should not be committed.Same issue - this
.venv/file contains hardcoded local paths and should not be in version control..venv/bin/pytest (1)
1-8: Critical: Virtual environment file should not be committed.Same issue - this
.venv/file should not be in version control..venv/bin/futurize (1)
1-8: Critical: Virtual environment file should not be committed.Same issue - this
.venv/file should not be in version control..venv/bin/coverage-3.10 (1)
1-8: Critical: Virtual environment file should not be committed.Same issue - this
.venv/file should not be in version control..venv/bin/coverage (1)
1-8: Critical: Virtual environment file should not be committed.Same issue - this
.venv/file should not be in version control..venv/bin/ipython3 (1)
1-8: Critical: Virtual environment file should not be committed.Same issue - this
.venv/file should not be in version control..venv/bin/pip (1)
1-8: Critical: Virtual environment files must not be committed.Same critical issue as
.venv/bin/ipython: this is an auto-generated virtual environment wrapper with a machine-specific absolute path (Line 1) that will not work on other systems. All.venv/files must be removed from this PR.See the review comment on
.venv/bin/ipythonfor detailed remediation steps..venv/bin/coverage3 (1)
1-8: Critical: Virtual environment files must not be committed.Same critical issue as the other
.venv/files: this is an auto-generated wrapper with a machine-specific absolute path (Line 1). All.venv/files must be removed from this PR.See the review comment on
.venv/bin/ipythonfor detailed remediation steps..venv/bin/ping6.py (5)
1-86: CRITICAL: Virtual environment files should not be committed to version control.This file has the same critical issue as
ping.py- it's located in.venv/bin/, which is a virtual environment directory that should never be committed to version control. Please refer to the detailed explanation in the review comment forping.py.Remove all
.venv/files from this PR:git rm -r .venv/
1-1: CRITICAL: Hardcoded absolute path in shebang won't work on other systems.Same issue as in
ping.py- the shebang contains a hardcoded absolute path:/home/aarush_/Pictures/updated_owasp_by_me/Nettacker/.venv/bin/pythonUse a portable shebang:
-#!/home/aarush_/Pictures/updated_owasp_by_me/Nettacker/.venv/bin/python +#!/usr/bin/env python3
61-86: No graceful exit mechanism for infinite loop.The script uses
while 1:with no break condition or signal handler, identical to the issue inping.py.Add signal handling for graceful shutdown:
+import signal + +def signal_handler(sig, frame): + print('\nPing interrupted.') + s.close() + sys.exit(0) + +signal.signal(signal.SIGINT, signal_handler) + while 1:
73-80: Missing error handling for socket operations.The
sendtoandrecvfromoperations lack error handling, which can cause the script to crash on network errors.Add try-except blocks:
- s.sendto(icmp.get_packet(), (dst, 0)) + try: + s.sendto(icmp.get_packet(), (dst, 0)) + except socket.error as e: + print(f"Send failed: {e}") + continue if s in select.select([s], [], [], 1)[0]: - reply = s.recvfrom(2000)[0] + try: + reply = s.recvfrom(2000)[0] + except socket.error as e: + print(f"Receive failed: {e}") + continue
83-86: Sleep only occurs after receiving a reply, not between all ping attempts.The
time.sleep(1)is inside theifblock (line 76), causing the same timing issue as inping.py.Move the sleep outside the if block:
if ICMP6.ICMP6.ECHO_REPLY == rip.get_type(): print("%d bytes from %s: icmp_seq=%d " % (rip.child().get_size()-4, dst, rip.get_echo_sequence_number())) - time.sleep(1) + time.sleep(1)
🧹 Nitpick comments (13)
.venv/bin/mqtt_check.py (1)
67-71: Address exception handling issues (only if file legitimately belongs in the codebase).Given that this file should not be committed (see previous comments), these issues are secondary. However, if similar error handling patterns exist elsewhere in your codebase:
Lines 69-71: Catching bare
Exceptionis too broad. Consider catching specific exception types (e.g.,argparse.ArgumentError).Lines 70, 89: Use
logging.exception(str(e))instead oflogging.error(str(e))to automatically include traceback information, eliminating the need for the manual traceback logic on lines 86-88.Apply this pattern only to legitimate application code, not third-party tool scripts:
try: options = parser.parse_args() except Exception as e: - logging.error(str(e)) + logging.exception("Failed to parse arguments") sys.exit(1)try: check_mqtt.run() except Exception as e: - if logging.getLogger().level == logging.DEBUG: - import traceback - traceback.print_exc() - logging.error(e) + logging.exception("MQTT connection failed")Note: This suggestion assumes the code would be part of the application, which it should not be based on the current file location.
Also applies to: 83-89
.venv/bin/getST.py (1)
91-91: Static analysis issues are present but secondary to the main problem.The static analysis tool (Ruff) has flagged several legitimate code quality issues in this file:
- Unused method parameters
oldSessionKey(lines 91, 298)- Broad exception catching without specific handling (lines 656, 738)
- Using
logging.errorinstead oflogging.exceptionin exception handlers (lines 658, 660, 662)However, since this is third-party library code from Impacket that should not be committed to this repository, these issues should not be fixed here. When you properly add Impacket as a dependency via pip/requirements.txt, you'll get the maintained version from the upstream project.
The correct approach is to use Impacket as a dependency rather than vendoring or committing its installed files.
Also applies to: 298-298, 656-662, 738-738
.venv/bin/esentutl.py (4)
44-49: Simplify logging withlogging.exception.Line 48 passes
exc_info=Truemanually;logging.exceptiondoes this automatically.- except Exception: - logging.debug('Exception:', exc_info=True) - logging.error('Error while calling getNextRow(), trying the next one') + except Exception: + logging.exception('Error while calling getNextRow(), trying the next one')Note: The bare
Exceptioncatch is acceptable for this utility script context where continuing iteration after row errors is desired behavior.
38-38: Fix quote inconsistency.Line 38 uses double quotes inconsistently with the rest of the file.
- logging.error('Can"t get a cursor for table: %s' % tableName) + logging.error("Can't get a cursor for table: %s" % tableName)
55-56: Remove extra whitespace.Line 55 has unnecessary leading whitespace before the
ifstatement.- if record[j] is not None: - print("%-30s: %r" % (j, record[j])) + if record[j] is not None: + print("%-30s: %r" % (j, record[j]))
115-117: Fix exit code for success case.
sys.exit(1)indicates failure, butmain()completed successfully. Either remove the exit call (implicit 0) or usesys.exit(0).if __name__ == '__main__': main() - sys.exit(1) + sys.exit(0).venv/bin/Get-GPPPassword.py (2)
91-91: Refactor lambda to def for better readability.Per PEP8 E731, lambda expressions should not be assigned to named variables. Using
defprovides better tracebacks and clearer intent.As per coding guidelines from static analysis.
- read_or_empty = lambda element, attribute: (element.getAttribute(attribute) if element.getAttribute(attribute) is not None else "") + def read_or_empty(element, attribute): + value = element.getAttribute(attribute) + return value if value is not None else ""
318-321: Use logging.exception for better error reporting.The main exception handler should use
logging.exception()to automatically include the full traceback, making debugging easier.Based on learnings from static analysis.
except Exception as e: if logging.getLogger().level == logging.DEBUG: traceback.print_exc() - logging.error(str(e)) + logging.exception("Failed to execute GPP password extraction").venv/bin/get_gprof (3)
55-68: Use subprocess module instead of deprecated os.system().Line 63 uses
os.system()to execute external commands, which is deprecated and has several drawbacks:
- More prone to shell injection vulnerabilities
- Limited error handling and process control
- Harder to capture output
Use
subprocess.run()instead:- try: - res = os.system(msg) - except Exception: - print ("Please verify install of 'gprof2dot' to view profile graphs") - if res: - print ("Please verify install of 'gprof2dot' to view profile graphs") + import subprocess + try: + result = subprocess.run(msg, shell=True, check=True, capture_output=True, text=True) + except subprocess.CalledProcessError: + print ("Please verify install of 'gprof2dot' to view profile graphs") + except FileNotFoundError: + print ("Please verify install of 'gprof2dot' to view profile graphs")
45-49: Avoid bare exception handlers; catch specific exceptions or provide error details.Lines 47 and 64 catch bare
Exceptionand provide generic error messages without any details about what actually failed. This makes debugging difficult and hides potentially important error information from users.Catch specific exceptions or at least log the actual error:
try: obj = eval(obj[-1]) - except Exception: - print ("Error processing object instance") + except (SyntaxError, NameError, TypeError, ValueError) as e: + print(f"Error processing object instance: {e}") sys.exit()try: res = os.system(msg) - except Exception: - print ("Please verify install of 'gprof2dot' to view profile graphs") + except OSError as e: + print(f"Error running gprof2dot: {e}") + print("Please verify install of 'gprof2dot' to view profile graphs")Also applies to: 62-67
59-59: Remove commented-out code.Line 59 contains commented-out code that should be removed. Version control preserves the history, so there's no need to keep dead code in comments.
- #name = os.path.splitext(os.path.basename(__file__))[0] cProfile.run("dill.dumps(obj)", filename="%s.prof" % name).venv/bin/atexec.py (2)
115-116: Consider usingsecretsmodule for temp name generation.While the collision risk is low with 8 random ASCII letters, using
secrets.choiceinstead ofrandom.choiceprovides cryptographically strong randomness and better aligns with security best practices for generating unpredictable identifiers.Apply this diff:
+ import secrets - tmpName = ''.join([random.choice(string.ascii_letters) for _ in range(8)]) + tmpName = ''.join([secrets.choice(string.ascii_letters) for _ in range(8)]) tmpFileName = tmpName + '.tmp'
217-233: Consider adding a retry limit to prevent indefinite hangs.The file retrieval loop (lines 217-233) retries indefinitely on 'SHARING' errors with 3-second delays. If the file never becomes available (e.g., task failed silently, permissions issue), the script will hang forever.
Consider adding a maximum retry count:
smbConnection = rpctransport.get_smb_connection() waitOnce = True max_retries = 20 # 60 seconds total for SHARING errors retry_count = 0 while True: try: logging.info('Attempting to read ADMIN$\\Temp\\%s' % tmpFileName) smbConnection.getFile('ADMIN$', 'Temp\\%s' % tmpFileName, output_callback) break except Exception as e: if str(e).find('SHARING') > 0: retry_count += 1 if retry_count > max_retries: logging.error('Max retries exceeded waiting for output file') raise time.sleep(3) elif str(e).find('STATUS_OBJECT_NAME_NOT_FOUND') >= 0: if waitOnce is True: time.sleep(3) waitOnce = False else: raise else: raise
Aarush289
force-pushed
the
feature/my-change
branch
from
4491c2d to
8565db6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nettacker/core/app.py (1)
293-302: Raise TypeError withfrom None.We still re-raise the
TypeErrorwithout suppressing the originalListconversion traceback, which violates B904 and makes the error noisy. Please addfrom None(as previously requested) when re-raising.- raise TypeError(f"`targets` must be iterable, got {type(targets).__name__}") + raise TypeError( + f"`targets` must be iterable, got {type(targets).__name__}" + ) from None
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nettacker/core/app.py(4 hunks)nettacker/core/hostcheck.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/hostcheck.pynettacker/core/app.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/hostcheck.pynettacker/core/app.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/hostcheck.pynettacker/core/app.py
🧠 Learnings (4)
📚 Learning: 2025-10-25T11:31:26.498Z
Learnt from: Aarush289
PR: OWASP/Nettacker#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.py
📚 Learning: 2025-09-07T19:20:58.332Z
Learnt from: CR
PR: OWASP/Nettacker#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/core/hostcheck.py
📚 Learning: 2025-09-07T19:20:58.332Z
Learnt from: CR
PR: OWASP/Nettacker#0
File: AGENTS.md:0-0
Timestamp: 2025-09-07T19:20:58.332Z
Learning: Applies to nettacker/main.py : CLI entry point code resides in nettacker/main.py
Applied to files:
nettacker/core/app.py
📚 Learning: 2025-09-07T19:20:58.332Z
Learnt from: CR
PR: OWASP/Nettacker#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 (2)
nettacker/core/hostcheck.py (1)
nettacker/core/ip.py (2)
is_single_ipv4(51-61)is_single_ipv6(88-98)
nettacker/core/app.py (1)
nettacker/core/hostcheck.py (3)
resolve_quick(85-127)is_ip_literal(32-44)valid_hostname(46-66)
🪛 Ruff (0.14.2)
nettacker/core/hostcheck.py
42-42: Consider moving this statement to an else block
(TRY300)
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)
Proposed change
Introduce a fast, parallel pre-check stage that validates targets before scanning and drops invalid/bogus entries early.
Current behavior attempts socket connects on every port; for hostnames this triggers DNS lookups that can block for a long time on nonsense names. With multiple ports, bogus inputs can consume minutes and significant resources.
This PR:
Runs a lightweight, parallel target validation pass via
resolve_quick(...):Skips the scan stage for rejected targets, preventing expensive connect/DNS fallbacks.
Canonicalizes valid targets (e.g., lowercases hostnames, strips trailing dot) and preserves input order.
Adds only a small up-front delay (seconds for large batches) while saving minutes on bad inputs.
Privacy safeguard (proxy mode):
If the user requests a proxy (e.g.,
-Ror--socks-proxy), the pre-check is disabled to avoid any local DNS lookups that could expose the user’s IP to their ISP/resolver. In proxy mode, targets proceed directly to the scanning path (letting the proxy handle DNS), preserving user anonymity.Examples dropped early now:
290.52.245.54(invalid IPv4),google(no resolvable FQDN), random strings likedsjfiejb.Type of change
Checklist
make pre-commit, it didn't generate any changesmake test, all tests passed locallyAdditional context
resolve_quick(...)fromapp.pyprior to scheduling scans.min(len(targets), 64)) to avoid spikes.-R/--socks-proxyis set to prevent local DNS leaks.