Add minimal Python MCP recon server with fixed nmap/curl/whois tools - #52

The function signature specifies target: str, which means target cannot be None. Therefore, this check for target is None is unreachable code and can be removed. The MCP framework will ensure that required arguments are provided.

:::writing{variant=“standard” id=“18452”}
Good catch. Since target is declared as str, None is not a valid value, and MCP already enforces required arguments at the tool boundary. That makes the target is None branch unreachable in normal execution, so I’ll remove it.

If we want defensive handling for malformed callers outside MCP, the better approach would be to widen the signature explicitly, for example target: str | None, and then validate inside the function. As written, the current check is redundant.
:::

Logging the fully rendered command (cmd_str) can leak secrets if the target is a URL containing credentials or tokens (e.g., https://user:pass@host or query params). Either redact sensitive URL components before logging, or log only the tool name plus a minimally identifying target (e.g., host) to avoid credential exposure.

subprocess.run(..., capture_output=True) buffers the full stdout/stderr in memory and truncates only after completion, so a noisy command can still cause high memory usage before truncation kicks in. If you want a hard output cap, consider streaming reads with a byte/char limit (or redirecting to a temp file) so the process can’t exceed MAX_OUTPUT_CHARS in memory.

setup.sh assumes python3 -m pip exists and installs packages into whatever environment python3 points to (often the system interpreter), which can fail without permissions and can pollute global site-packages. Consider checking for pip explicitly and guiding users to create/activate a venv (or use pipx) before installing mcp.

Because _TARGET_PATTERN permits "/", nmap_scan currently accepts CIDR/range-style targets (e.g., 10.0.0.0/8), which can generate a large amount of traffic before the timeout triggers. If the intent is “basic recon” on a single host, consider tightening validation for nmap/whois to disallow CIDR/ranges/paths and require a hostname/IP only.

_validate_target() strips the target into candidate, but the sanitized value is never returned/used. This means inputs like "example.com " can pass validation and still be executed with the original (whitespace-containing) argument, which is inconsistent with the safety guarantees. Consider returning the sanitized target (or a tuple of (sanitized, error)) and building the command from that value.

The shared _TARGET_PATTERN allows URL schemes/paths (":" and "/"), which lets curl_head accept file://... and other non-HTTP(S) schemes. That can exfiltrate local files from the host running the server, contradicting the “remote recon” safety intent. Recommend tool-specific validation for curl that only permits http:// and https:// (and rejects file:, scp:, etc.).

⚠️ Potential issue | 🟠 Major

Don't strip for validation unless you also execute the stripped value.

Line 61 normalizes into candidate, but _run_tool() still passes the raw target to the subprocess. " example.com " therefore clears validation even though README_PENTEST.md Line 31 says whitespace is rejected.

 def _validate_target(target: str) -> str | None:
     if target is None:
         return "target is required"
-    candidate = target.strip()
-    if not candidate:
+    if not target.strip():
         return "target cannot be empty"
+    if target != target.strip():
+        return "target cannot contain leading or trailing whitespace"
+    candidate = target
     if len(candidate) > 254:
         return "target is too long"

Verify each finding against the current code and only fix it if needed.

In `@server.py` around lines 58 - 70, Validation currently strips the input inside
_validate_target but the caller (_run_tool) continues to pass the original
unstripped target to the subprocess, letting inputs like " example.com " bypass
the README rule; fix by normalizing once and using the normalized value
downstream: either change _validate_target to return the cleaned/stripped string
(or raise/return an error) so callers can use that returned value, or ensure
_run_tool explicitly does before calling
_validate_target and before launching the subprocess; update references to
_validate_target and any subprocess invocation in _run_tool to use the stripped
value.

⚠️ Potential issue | 🟠 Major

#!/bin/bash
python3 - <<'PY'
import subprocess, sys
cmd = [
    sys.executable,
    "-c",
    "import sys,time; print('out'); print('err', file=sys.stderr); sys.stdout.flush(); sys.stderr.flush(); time.sleep(1)",
]
try:
    subprocess.run(cmd, capture_output=True, text=True, timeout=0.1)
except subprocess.TimeoutExpired as exc:
    print(type(exc.stdout).__name__, repr(exc.stdout))
    print(type(exc.stderr).__name__, repr(exc.stderr))
PY

#!/bin/bash
python3 - <<'PY'
import subprocess, sys
cmd = [
    sys.executable,
    "-c",
    "import sys,time; print('out'); print('err', file=sys.stderr); sys.stdout.flush(); sys.stderr.flush(); time.sleep(1)",
]
try:
    subprocess.run(cmd, capture_output=True, text=True, timeout=0.1)
except subprocess.TimeoutExpired as exc:
    print(type(exc.stdout).__name__, repr(exc.stdout))
    print(type(exc.stderr).__name__, repr(exc.stderr))
PY

# Find and examine server.py
find . -name "server.py" -type f

TimeoutExpired stdout/stderr are bytes, not str—partial output is lost.

Python's subprocess.TimeoutExpired always exposes stdout/stderr as bytes (even with text=True), per official documentation. Lines 116-117's isinstance(exc.stdout, str) check will always fail, returning empty strings instead of partial output.

     except subprocess.TimeoutExpired as exc:
-        partial_stdout = exc.stdout if isinstance(exc.stdout, str) else ""
-        partial_stderr = exc.stderr if isinstance(exc.stderr, str) else ""
+        partial_stdout = (
+            exc.stdout.decode(errors="replace")
+            if isinstance(exc.stdout, bytes)
+            else (exc.stdout or "")
+        )
+        partial_stderr = (
+            exc.stderr.decode(errors="replace")
+            if isinstance(exc.stderr, bytes)
+            else (exc.stderr or "")
+        )
         stdout, stdout_truncated = _truncate(partial_stdout)
         stderr, stderr_truncated = _truncate(partial_stderr)

Verify each finding against the current code and only fix it if needed.

In `@server.py` around lines 115 - 117, The TimeoutExpired handler is discarding
partial output because it checks for str; update the except block that sets
partial_stdout and partial_stderr (the subprocess.TimeoutExpired handler) to
accept bytes by handling bytes or str: if exc.stdout/exc.stderr are bytes,
decode them (e.g., using .decode('utf-8', errors='replace')), if they are
already str use as-is, and default to empty string when None; ensure you apply
the same logic to both partial_stdout and partial_stderr so partial output is
preserved for logging or reporting.

⚠️ Potential issue | 🟠 Major

#!/bin/bash
set -euo pipefail
command -v curl >/dev/null

printf 'Current behavior:\n'
curl -I file:///etc/hosts 2>&1 | sed -n '1,5p' || true

printf '\nRestricted behavior:\n'
curl --proto '=http,https' -I file:///etc/hosts 2>&1 | sed -n '1,5p' || true

curl_head accepts all protocols, not just HTTP(S).

The description claims "HTTP header inspection," but curl -I by default supports file://, ftp://, ftps://, and other protocols beyond HTTP and HTTPS. This allows the tool to access local files or non-HTTP resources from the host, contradicting its stated purpose.

Add --proto '=http,https' to restrict the tool to only HTTP and HTTPS:

 def curl_head(target: str) -> ToolResult:
-    return _run_tool(["curl", "-I", target], target)
+    return _run_tool(["curl", "--proto", "=http,https", "-I", target], target)

Verify each finding against the current code and only fix it if needed.

In `@server.py` around lines 149 - 154, The curl_head tool currently invokes curl
in curl_head and accepts non-HTTP protocols; modify the argument list passed to
_run_tool in curl_head to include the flag --proto '=http,https' so curl is
restricted to only HTTP/HTTPS, and update the curl_head description if needed to
reflect the enforced protocol restriction; target the curl_head function and the
call to _run_tool for this change.

setup.sh installs mcp>=1.0.0, but server.py imports mcp.server.fastmcp.FastMCP (added in mcp v1.2.0). As-is, a fresh setup can succeed but the server will fail at import time. Bump the minimum to a version that includes FastMCP (e.g. mcp>=1.2.0) and keep README/setup consistent.

This script uses python3 -m pip install ... into the system environment. On many distros with PEP 668 “externally managed” Python, this fails unless the user creates a venv or uses pipx/--user. Consider updating setup.sh (and README) to create/activate a virtualenv and install into it, or document the supported installation method explicitly.

TIMEOUT_SECONDS / MAX_OUTPUT_CHARS are parsed with int(os.getenv(...)) at import time. If the env vars are set to non-integer values, the module import will raise ValueError and crash the server (contradicting the “failure containment” goal). Consider defensive parsing with defaults/clamping so the server can still start with a structured/logged error.

cmd_str is redacted for logging, but the returned ToolResult includes the unredacted command. Since targets may include URL userinfo (e.g. http://user:pass@host), this can leak credentials back to the MCP client/logs downstream. Consider returning the redacted command string (or separately returning both raw+redacted, with the raw omitted by default).

The current target validation permits arbitrary schemes/paths (e.g. file:///..., gopher://...) and localhost/private IPs. With curl -I <target>, that enables local file probing and SSRF to local/internal services, which conflicts with the stated goal of “basic remote recon” and “strict safety”. Consider restricting curl_head targets to http:///https:// (or host[:port] only) and optionally blocking loopback/link-local/private ranges.