Fixed windows compatibility issue #1142
Conversation
- Replace os.path.join, os.path.normpath, os.path.basename with pathlib equivalents - Update tests/conftest.py to use pathlib.Path instead of os.path functions - Modify nettacker/core/graph.py to use pathlib for path operations - Update nettacker/api/core.py and nettacker/api/engine.py to use Path objects - Fix tests/test_yaml_regexes.py to use pathlib.Path.name instead of os.path.basename - Remove obsolete os.path patches from tests/core/test_graph.py This ensures the application works consistently across Windows and Unix-like systems by eliminating platform-specific path assumptions throughout the codebase. Fixes OWASP#933
- Replace string startswith() with Path.is_relative_to() for secure path validation - Fix runtime TypeError where Path object was treated as string in file extension checks - Use proper pathlib.Path.suffix for file extension detection - Improve Content-Disposition header with proper filename extraction - Remove redundant str() call around json.dumps() Security improvements: - Prevents path traversal attacks using modern pathlib semantics - Uses resolve(strict=True) to ensure target file exists before validation - Replaces brittle string prefix checks with robust path relationship validation Bug fixes: - Fixes TypeError in report generation where len(Path_object) would fail - Handles complex file extensions like .dd.json correctly - Maintains backward compatibility while using modern Python patterns
- Add sanitize_path mock to ensure PermissionError test can trigger properly - Mock returns '../../etc/passwd' to simulate path traversal attempt - Preserves test intent while adapting to new pathlib.Path approach - Addresses reviewer concern about removed os.path mocks
|
Warning Rate limit exceeded@codeshazard has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughReplaced os.path usage with pathlib across API, engine, core, and tests; enforce path containment via Path.resolve() and Path.is_relative_to(); switch file I/O to Path.read_bytes()/Path.open(); tighten MIME and Content-Disposition handling; robust YAML loading with UTF‑8 and English fallback; changed Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nettacker/core/app.py (1)
68-102: Restore thecheck_dependenciesdefinition to avoid an import-time crashBy commenting out the
def check_dependenciesline but leaving the indented body, Python now sees thetry:at an indentation level with no enclosing block, so importing this module raisesIndentationError: unexpected indent (app.py, line 72). Even if it compiled, the logic would execute once at import instead of per-instance. Please reinstate the function definition (and keep the body inside it) while relaxing the platform gate as needed.- #def check_dependencies(self): - #if sys.platform not in {"darwin", "freebsd13", "freebsd14", "freebsd15", "linux"}: - #die_failure(_("error_platform")) + def check_dependencies(self): + # Platform gate temporarily disabled to allow Windows compatibility. + # if sys.platform not in {"darwin", "freebsd13", "freebsd14", "freebsd15", "linux"}: + # die_failure(_("error_platform"))nettacker/api/engine.py (1)
223-237: Guard against missing report filename before callingPath()If the client omits
report_path_filename,raw_report_path_filenameisNone. Passing that intoPath(report_path_filename).nameraises aTypeError, turning what should be a 400 into a 500. Add a simple falsy check before constructing thePath.def sanitize_report_path_filename(report_path_filename): """ sanitize the report_path_filename @@ - filename = secure_filename(Path(report_path_filename).name) + if not report_path_filename: + return False + + filename = secure_filename(Path(report_path_filename).name)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
nettacker/api/core.py(2 hunks)nettacker/api/engine.py(4 hunks)nettacker/core/app.py(2 hunks)nettacker/core/arg_parser.py(4 hunks)nettacker/core/graph.py(1 hunks)nettacker/core/messages.py(2 hunks)nettacker/core/template.py(1 hunks)results(1 hunks)tests/conftest.py(1 hunks)tests/core/test_graph.py(1 hunks)tests/test_yaml_regexes.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.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/template.pytests/conftest.pynettacker/api/engine.pytests/test_yaml_regexes.pynettacker/core/arg_parser.pytests/core/test_graph.pynettacker/core/graph.pynettacker/core/messages.pynettacker/api/core.pynettacker/core/app.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/template.pynettacker/api/engine.pynettacker/core/arg_parser.pynettacker/core/graph.pynettacker/core/messages.pynettacker/api/core.pynettacker/core/app.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/template.pynettacker/core/arg_parser.pynettacker/core/graph.pynettacker/core/messages.pynettacker/core/app.py
tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Mirror package layout in tests (e.g., tests/core/, tests/lib/, etc.)
Files:
tests/conftest.pytests/test_yaml_regexes.pytests/core/test_graph.py
nettacker/api/**
📄 CodeRabbit inference engine (AGENTS.md)
Place API-related code under nettacker/api/
Files:
nettacker/api/engine.pynettacker/api/core.py
tests/**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/test_*.py: Place tests under tests/ and name files as test_*.py
Use pytest (with pytest-asyncio, xdist) for tests
Files:
tests/test_yaml_regexes.pytests/core/test_graph.py
🧠 Learnings (1)
📚 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:
tests/conftest.py
🧬 Code graph analysis (6)
nettacker/core/template.py (1)
nettacker/config.py (1)
Config(183-187)
nettacker/api/engine.py (2)
nettacker/api/core.py (2)
get_file(110-130)mime_types(30-107)nettacker/config.py (1)
Config(183-187)
nettacker/core/arg_parser.py (1)
nettacker/config.py (1)
Config(183-187)
nettacker/core/graph.py (1)
nettacker/core/utils/common.py (1)
sanitize_path(430-445)
nettacker/core/messages.py (2)
nettacker/core/template.py (1)
open(30-36)nettacker/config.py (1)
Config(183-187)
nettacker/api/core.py (1)
nettacker/config.py (1)
Config(183-187)
🪛 Ruff (0.13.1)
tests/core/test_graph.py
247-247: Unused function argument: mock_sanitize
(ARG001)
nettacker/core/messages.py
25-25: Probable use of unsafe loader FullLoader with yaml.load. Allows instantiation of arbitrary objects. Consider yaml.safe_load.
(S506)
55-55: Do not catch blind exception: Exception
(BLE001)
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/app.py(2 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.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/app.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/app.py
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
nettacker/core/messages.py (1)
53-57: Catch specific errors when priming the message cacheCatching
Exceptionhere hides unexpected bugs (e.g., programmer errors) and was flagged by Ruff (BLE001). Narrow this to the OS/YAML/typing errors we expect so real faults still surface.-try: - message_cache = load_message().messages -except Exception as e: - print(f"[!] Failed to load messages: {e}") +try: + message_cache = load_message().messages +except (OSError, yaml.YAMLError, TypeError, ValueError) as exc: + print(f"[!] Failed to load messages: {exc}")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nettacker/core/messages.py(2 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/messages.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/messages.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/messages.py
🧬 Code graph analysis (1)
nettacker/core/messages.py (2)
nettacker/core/template.py (1)
open(30-36)nettacker/config.py (1)
Config(183-187)
🪛 Ruff (0.13.1)
nettacker/core/messages.py
55-55: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
nettacker/core/messages.py (1)
37-50: Stop emitting debug prints during importThis module is imported by both CLI and API paths, so these unconditional prints still leak into user-facing output and can corrupt JSON/HTTP responses. Please drop them or move the diagnostics behind the project logger guarded by the runtime debug level—this was already flagged earlier and remains unresolved.
@@ - print(f"[DEBUG] Selected language: {self.language}") - print(f"[DEBUG] Available languages: {get_languages()}") @@ - print(f"[DEBUG] Selected language: {self.language}")
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/messages.py(2 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/messages.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/messages.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/messages.py
🧬 Code graph analysis (1)
nettacker/core/messages.py (2)
nettacker/core/template.py (1)
open(30-36)nettacker/config.py (1)
Config(183-187)
🪛 Ruff (0.13.1)
nettacker/core/messages.py
37-37: Undefined name log
(F821)
38-38: Undefined name log
(F821)
54-54: Do not catch blind exception: Exception
(BLE001)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nettacker/core/app.py (2)
170-183: Fix Windows crash: os.geteuid() is unavailable on Windows.
Accessing os.geteuid on Windows raises AttributeError, breaking --ping-before-scan flows. Replace with a cross‑platform admin/root check.Apply this diff within the selected range:
- if self.arguments.ping_before_scan: - if os.geteuid() == 0: + if self.arguments.ping_before_scan: + # Cross-platform admin check + is_admin = False + if os.name == "nt": + try: + import ctypes # lazy import; Windows-only code path + is_admin = bool(ctypes.windll.shell32.IsUserAnAdmin()) + except Exception: + is_admin = False + else: + is_admin = hasattr(os, "geteuid") and os.geteuid() == 0 + if is_admin: selected_modules = self.arguments.selected_modules self.arguments.selected_modules = ["icmp_scan"] self.start_scan(scan_id) self.arguments.selected_modules = selected_modules if "icmp_scan" in self.arguments.selected_modules: self.arguments.selected_modules.remove("icmp_scan") self.arguments.targets = self.filter_target_by_event(targets, scan_id, "icmp_scan") else: - log.warn(_("icmp_need_root_access")) + log.warning(_("icmp_need_root_access")) if "icmp_scan" in self.arguments.selected_modules: self.arguments.selected_modules.remove("icmp_scan")
293-293: Avoid os.EX_OK on Windows (may be undefined).
Use a portable constant.Apply this diff and module-level constant:
- return os.EX_OK + return EXIT_OKOutside this range, add near the imports:
# Portable exit code for success EXIT_OK = getattr(os, "EX_OK", 0)
🧹 Nitpick comments (10)
nettacker/core/app.py (6)
55-66: Prefer Path.read_text with UTF‑8 to avoid default-encoding issues on Windows.
This keeps path handling consistent with pathlib usage.Apply this diff:
- log.write_to_api_console( - open(Config.path.logo_file) - .read() - .format( + log.write_to_api_console( + Config.path.logo_file.read_text(encoding="utf-8").format( cyan=TerminalCodes.CYAN.value, red=TerminalCodes.RED.value, rst=TerminalCodes.RESET.value, v1=version_info()[0], v2=version_info()[1], yellow=TerminalCodes.YELLOW.value, ) )
87-87: Use logging.warning instead of deprecated logging.warn.Apply this diff:
- log.warn("Database files migrated from .data to .nettacker ...") + log.warning("Database files migrated from .data to .nettacker ...")- log.warn(_("icmp_need_root_access")) + log.warning(_("icmp_need_root_access"))Also applies to: 180-180
120-133: URL parsing is brittle; switch to urllib.parse.urlsplit.
Manual splits are error‑prone for edge cases (userinfo, IPv6 literals, empty path). Recommend refactor to urlsplit for correctness and readability.Example:
from urllib.parse import urlsplit parts = urlsplit(target) host = parts.hostname or target base_path = (parts.path or "").lstrip("/") base_path = f"{base_path}/" if base_path and not base_path.endswith("/") else base_path targets.append(host)
252-258: Avoid variable shadowing: target_groups reused as loop variable.
Shadowing hurts readability and can confuse future edits.Apply this diff:
- for t_id, target_groups in enumerate(target_groups): + for t_id, group in enumerate(target_groups): process = multiprocess.Process( - target=self.scan_target_group, args=(target_groups, scan_id, t_id) + target=self.scan_target_group, args=(group, scan_id, t_id) )
117-152: Docstring says “returns a generator” but function returns a list.
Align docstring or return type.Update the docstring “Returns: a list of targets” or yield values consistently.
75-91: SQLite bootstrap: slightly clearer error message.
The PermissionError references only tmp_dir, but results_dir may also fail. Consider including both for clarity. Not a blocker.nettacker/core/messages.py (4)
42-49: Use pathlib consistently and guard against empty YAML (safe_load can return None).
Both improve robustness and Windows path handling.Apply this diff:
- # Build path safely - language_file = os.path.join(Config.path.locale_dir, f"{self.language}.yaml") - self.messages = load_yaml(language_file) + # Build path safely with pathlib and handle empty files + language_file = Config.path.locale_dir / f"{self.language}.yaml" + self.messages = load_yaml(language_file) or {}- fallback_file = os.path.join(Config.path.locale_dir, "en.yaml") - self.messages_en = load_yaml(fallback_file) + fallback_file = Config.path.locale_dir / "en.yaml" + self.messages_en = load_yaml(fallback_file) or {}
33-33: Minor: prefer Path.stem over os.path for clarity.
Keeps code idiomatic with pathlib usage elsewhere.Apply this diff:
- languages_list.append(os.path.splitext(os.path.basename(str(language)))[0]) + languages_list.append(language.stem)
61-73: Optional: add type hints to messages().
Improves editor support with minimal risk.Apply this diff:
+from typing import Union @@ -def messages(msg_id): +def messages(msg_id: Union[str, int]) -> str: @@ - return message_cache.get(str(msg_id), str(msg_id)) + return message_cache.get(str(msg_id), str(msg_id))
36-45: Style: class name should be PascalCase per guidelines.
Renaming load_message -> LoadMessage is non‑blocking but recommended for consistency.As per coding guidelines
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nettacker/core/app.py(2 hunks)nettacker/core/messages.py(2 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/messages.pynettacker/core/app.py
nettacker/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add docstrings for public APIs in the nettacker package
Files:
nettacker/core/messages.pynettacker/core/app.py
nettacker/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core libraries under nettacker/core/
Files:
nettacker/core/messages.pynettacker/core/app.py
🧬 Code graph analysis (1)
nettacker/core/messages.py (2)
nettacker/core/template.py (1)
open(30-36)nettacker/config.py (1)
Config(183-187)
🪛 Ruff (0.13.1)
nettacker/core/messages.py
56-56: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (4)
nettacker/core/app.py (2)
44-44: No-op whitespace change — OK to leave as-is.
69-74: Good: platform gate removed; cross‑platform init now proceeds.
This unblocks Windows while retaining clear logs.nettacker/core/messages.py (2)
25-27: Good: switched to yaml.safe_load with UTF‑8.
Eliminates unsafe deserialization and locale encoding issues.
39-41: LGTM: debug logs now go through logger.
No stdout leakage; respects log level.
|
@arkid15r @securestep9 All comments resolved — just waiting on workflow approvals to proceed. Let me know if anything else needs attention. Thanks again! |
| if sys.platform not in {"darwin", "freebsd13", "freebsd14", "freebsd15", "linux"}: | ||
| die_failure(_("error_platform")) | ||
|
|
||
| if sys.platform.startswith("win"): |
There was a problem hiding this comment.
This isn't right. You're basically saying all platforms are supported (we're close but we don't want to make that statement yet). Please just add win32 to the set, should be good enough
There was a problem hiding this comment.
Thanks for the advise! Honestly I don't have much knowledge about it and I'm constantly learning while doing it! I've been using AI at instances to understand the code better in order to contribute efficiently! I'll stick to path for path management and try to refactor the code using only path. I've done the necessary changes u asked for such as adding only windows compatibility and removing every other improvements not related to it.
After refactoring the code to using only path now I need to check whether it is windows compatible or not.
In order to do that, I ran a test
pytest -v
Now solving the errors and debugging!
Any suggestions for what I'm doing!
| # Exclude Module Name | ||
| exclude_modules = sorted(self.modules.keys())[:10] | ||
| exclude_modules.remove("all") | ||
| exclude_modules = [m for m in exclude_modules if m != "all"] |
There was a problem hiding this comment.
Is this change related to windows compatibility?
There was a problem hiding this comment.
It's not!
Previously when trying to run the code it was giving an error so in order to resolve it I changed it.
I'll try to run the code without changing it.
| "-v", | ||
| "--verbose", | ||
| action="store_true", | ||
| type=int, |
There was a problem hiding this comment.
Is this related to windows compatibility?
There was a problem hiding this comment.
It's not!
Previously when trying to run the code it was giving an error so in order to resolve it I changed it.
I'll try to run the code without changing it.
| ) | ||
| ) | ||
| log.debug(f"Selected language: {self.language}") | ||
| log.debug(f"Available languages: {get_languages()}") |
There was a problem hiding this comment.
We don't have a .debug() in our logger
There was a problem hiding this comment.
ThankYou! for letting me know that, I'll use log.info in accordance with Nettacker logger.
| from io import StringIO | ||
|
|
||
| import logging | ||
| log = logging.getLogger(__name__) |
There was a problem hiding this comment.
we use our own logger at nettacker.logger from where you import the get_logger() function
There was a problem hiding this comment.
Understood, made the necessary changes!
|
|
||
| message_cache = load_message().messages | ||
|
|
||
| try: |
There was a problem hiding this comment.
If unrelated to OS compatibility, please don't include them
There was a problem hiding this comment.
Understood, made the necessary changes.
| @patch("nettacker.core.graph.get_logs_by_scan_id") | ||
| @patch("nettacker.core.graph.get_options_by_scan_id") | ||
| @patch("nettacker.core.graph.os.path.normpath", side_effect=lambda x: "/etc/passwd") | ||
| @patch("nettacker.core.graph.os.path.join", side_effect=lambda *args: "/etc/passwd") |
There was a problem hiding this comment.
Please look into my comment for #1137 for this.
There was a problem hiding this comment.
I understand, when I was refactoring the code using only path removing those lines would be good. As per ur comment these two lines are invoking some tests and so considering it's importance I rewrote the lines using path. Will that be alright?
This PR addresses Windows compatibility issues encountered while testing PR #1137. The original implementation failed to run successfully on Windows due to platform-specific path handling, CLI argument parsing, and language detection logic.
To resolve this, I made the following changes:
These changes allow Nettacker to run successfully on Windows 11 using:
=>python nettacker.py -i scanme.nmap.org -m admin_scan -v 2
This PR builds on PR #1137 and fulfills the contributor’s request to test and fix Windows-specific issues.
Type of change
Checklist
At last, ThankYou for the opportunity!
If there are any issues Please guide me!