Skip to content

fix(security): 2 improvements across 2 files #1201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
tomaioo wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

fix(security): 2 improvements across 2 files #1201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
71de309
fix(security): 2 improvements across 2 files
tomaioo Jun 19, 2026
0274bd6
fix(security): 2 improvements across 2 files
tomaioo Jun 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion secator/tasks/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
import importlib
import re
from secator.loader import discover_tasks

TASKS = discover_tasks()
__all__ = [
cls.__name__
for cls in TASKS
]
for cls in TASKS:
exec(f'from .{cls.__name__} import {cls.__name__}')
name = cls.__name__
if not re.match(r'^[a-zA-Z_][a-zA-Z0-9_]*$', name):
raise ValueError(f'Invalid task class name: {name}')
importlib.import_module(f'.{name}', __package__)
Comment on lines 10 to +14

@coderabbitai coderabbitai Bot Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify that tests expect task classes to be directly importable from secator.tasks
rg -n -C2 'from secator\.tasks import' --type py

Repository: freelabz/secator

Length of output: 6970

🏁 Script executed:

cat -n secator/tasks/__init__.py

Repository: freelabz/secator

Length of output: 496

🏁 Script executed:

rg -n "def discover_tasks" --type py -A 20

Repository: freelabz/secator

Length of output: 1230

🏁 Script executed:

rg -n "def discover_internal_tasks" --type py -A 30

Repository: freelabz/secator

Length of output: 1746

🏁 Script executed:

rg -n "__getattr__" secator/tasks/__init__.py

Repository: freelabz/secator

Length of output: 42

🏁 Script executed:

git log --oneline -n 20 -- secator/tasks/__init__.py

Repository: freelabz/secator

Length of output: 113

🏁 Script executed:

git show 0274bd6 -- secator/tasks/__init__.py

Repository: freelabz/secator

Length of output: 968

🏁 Script executed:

python3 -c "from secator.tasks import nmap; print(nmap)" 2>&1

Repository: freelabz/secator

Length of output: 515

🏁 Script executed:

git show HEAD~1:secator/tasks/__init__.py 2>/dev/null || echo "No previous version"

Repository: freelabz/secator

Length of output: 80

🏁 Script executed:

git log --all --oneline -- secator/tasks/__init__.py | head -5

Repository: freelabz/secator

Length of output: 397

🏁 Script executed:

git show df94657:secator/tasks/__init__.py

Repository: freelabz/secator

Length of output: 244

🏁 Script executed:

rg -n "import sys" secator/tasks/__init__.py

Repository: freelabz/secator

Length of output: 42

🏁 Script executed:

grep -r "from secator.tasks import" tests/ --include="*.py" | head -3 | cut -d: -f1 | uniq | xargs -I {} sh -c 'echo "File: {}"; head -20 "{}"'

Repository: freelabz/secator

Length of output: 588

🏁 Script executed:

python3 << 'PY'
# Simulate what happens with the current __init__.py logic
import importlib
import re
from pathlib import Path

# Check what __all__ does without actual bindings
namespace = {}
namespace['__all__'] = ['nmap', 'httpx']

# Try to import something that's in __all__ but not in namespace
try:
    nmap = namespace['nmap']
    print("nmap is accessible:", nmap)
except KeyError:
    print("KeyError: 'nmap' in __all__ but not in namespace - import would fail")

# This is what a direct import does
print("\nWhen you do 'from module import nmap', Python looks for 'nmap' in:")
print("1. module.__dict__ (module's globals)")
print("2. __all__ only affects 'from module import *'")
PY

Repository: freelabz/secator

Length of output: 282

Task classes not exported to package namespace — breaks from secator.tasks import <task> imports.

The refactor from exec(f'from .{cls.__name__} import {cls.__name__}') to importlib.import_module() introduced a critical flaw. The old approach both imported the module AND bound the class to the package namespace. The new code only loads the module into sys.modules but discards the return value, leaving the class unbound.

Additionally, __all__ is defined but irrelevant—it only controls from secator.tasks import * behavior. Direct imports like from secator.tasks import nmap (as used throughout tests) require the class to exist in the module's __dict__, which it currently does not.

🐛 Proposed fix to bind the class to the package namespace 
 for cls in TASKS:
 	name = cls.__name__
 	if not re.match(r'^[a-zA-Z_][a-zA-Z0-9_]*$', name):
 		raise ValueError(f'Invalid task class name: {name}')
-	importlib.import_module(f'.{name}', __package__)
+	module = importlib.import_module(f'.{name}', __package__)
+	globals()[name] = getattr(module, name)
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/tasks/__init__.py` around lines 10 - 14, The
importlib.import_module() call in the loop only loads the module into
sys.modules but does not bind the task class to the package namespace. To fix
this, capture the return value of importlib.import_module() and extract the
class from the imported module object using getattr() with the class name, then
bind it to the current module's namespace using globals()[cls.__name__] =
imported_class so that direct imports like `from secator.tasks import
<classname>` will function correctly instead of raising ImportError.

7 changes: 7 additions & 0 deletions secator/tasks/dirsearch.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import os
import shlex
from pathlib import Path
import yaml

# from secator.decorators import task
Expand Down Expand Up @@ -75,6 +76,12 @@ def on_init(self):

@staticmethod
def on_cmd_done(self):
output_path = Path(self.output_path).resolve()
reports_folder = Path(self.reports_folder).resolve()
if not str(output_path).startswith(str(reports_folder)):
yield Error(message=f'Output path {output_path} is not within reports folder {reports_folder}')
return
Comment on lines +79 to +83

@coderabbitai coderabbitai Bot Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check Python version requirements to determine which fix to use
fd -t f -e toml -e cfg -e txt 'pyproject|setup|requirements' --exec cat {}
# Also check for existing usage of is_relative_to in the codebase
rg -n 'is_relative_to|relative_to' --type py

Repository: freelabz/secator

Length of output: 2347

🏁 Script executed:

# Check the actual code in dirsearch.py at lines 79-83
head -85 secator/tasks/dirsearch.py | tail -10

Repository: freelabz/secator

Length of output: 416

🏁 Script executed:

# Search for how paths are validated elsewhere in the codebase
rg -n 'startswith.*/' --type py -A 2 -B 2 | head -50

Repository: freelabz/secator

Length of output: 3135

🏁 Script executed:

# Check if dirsearch.py exists and verify its structure
wc -l secator/tasks/dirsearch.py
head -100 secator/tasks/dirsearch.py | tail -40

Repository: freelabz/secator

Length of output: 1433

String-based startswith path check is bypassable—use path resolution exception handling instead.

The containment check using str(output_path).startswith(str(reports_folder)) can be bypassed. If reports_folder is /home/user/reports and output_path is /home/user/reports_evil/file.json, the check passes because the string starts with /home/user/reports, even though the path is not actually inside the reports folder.

Since the project requires Python 3.8+ support, use the relative_to() exception pattern for robust containment checking. This pattern is already used elsewhere in the codebase.

🔒 Proposed fix for Python 3.8+ compatibility 
 	`@staticmethod`
 	def on_cmd_done(self):
 		output_path = Path(self.output_path).resolve()
 		reports_folder = Path(self.reports_folder).resolve()
-		if not str(output_path).startswith(str(reports_folder)):
+		try:
+			output_path.relative_to(reports_folder)
+		except ValueError:
 			yield Error(message=f'Output path {output_path} is not within reports folder {reports_folder}')
 			return
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
output_path = Path(self.output_path).resolve()
reports_folder = Path(self.reports_folder).resolve()
if not str(output_path).startswith(str(reports_folder)):
yield Error(message=f'Output path {output_path} is not within reports folder {reports_folder}')
return
output_path = Path(self.output_path).resolve()
reports_folder = Path(self.reports_folder).resolve()
try:
output_path.relative_to(reports_folder)
except ValueError:
yield Error(message=f'Output path {output_path} is not within reports folder {reports_folder}')
return
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/tasks/dirsearch.py` around lines 79 - 83, Replace the string-based
path containment check in the validation block (where output_path and
reports_folder are compared using startswith) with a try-except pattern using
the relative_to() method. Wrap the relative_to() call in a try-except block to
catch ValueError when output_path is not actually within reports_folder, then
yield the Error if the exception occurs. This approach properly validates path
containment and prevents directory traversal attacks, regardless of similar path
name prefixes.


if not os.path.exists(self.output_path):
yield Error(message=f'Could not find JSON results in {self.output_path}')
return
Expand Down
Loading