From ce62fbebfea893fbf15c8c2fc2b13862a4f84bb9 Mon Sep 17 00:00:00 2001 From: Kobe Date: Sun, 15 Mar 2026 19:26:00 -0700 Subject: [PATCH 1/3] Fix privacy data flow claims --- README.md | 19 +++++++++---------- SECURITY.md | 7 +++++++ src/goop_face/detector.py | 9 +++++---- src/goop_face/mcp/server.py | 27 ++++++++++++++++++--------- tests/test_client.py | 30 ++++++++++++++++++++++++++++++ tests/test_mcp/test_server.py | 11 ++++++++--- 6 files changed, 77 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index 0b054b6..000fcfd 100644 --- a/README.md +++ b/README.md @@ -1,10 +1,10 @@ # goop-face -Privacy-preserving facial protection -- detect faces locally, protect them from recognition systems using neural adversarial perturbations. +Facial protection tooling with local detection, on-device protection, and optional server-backed protection. ## Features -- **Local face detection** using InsightFace (buffalo_sc model, never leaves your machine) +- **Local face detection** using InsightFace (buffalo_sc model) - **Local CPU protection** via ONNX perturbation generator (~30-60ms inference, no GPU required) - **GPU backend protection** via goop-face-engine (diffusion-based, higher quality, multi-model evasion scoring) - **MCP server** for AI agent integration (Claude Code, Claude Desktop, etc.) @@ -30,7 +30,7 @@ For development: pip install "goop-face[dev,mcp]" ``` -**Note:** On first run, InsightFace will download the `buffalo_sc` detection model (~300MB). This is a one-time download. +**Note:** On first run, InsightFace will download the `buffalo_sc` detection model (~300MB). That one-time model download requires network access. ## Quick Start @@ -200,16 +200,15 @@ Use the same configuration in Claude Desktop's MCP settings. ### Privacy Model -- Face detection **always** runs locally -- original images never leave your machine. -- **Local tier**: Everything stays on-device. The ONNX generator runs via CPU, no network calls. -- **Server tier**: Only the detected 112x112 face crop is sent to the GPU backend, not the full image. +- `detect_faces`: Runs locally in this process. The detector itself does not make network requests after its model is installed. +- **Local tier**: The full image is read locally, faces are detected locally, and the bundled ONNX generator runs on aligned face crops. No inference-time network calls are made. +- **Server tier**: The current Python client and MCP server upload the full input image to `goop-face-engine` over HTTP. The backend returns the protected or analyzed result. +- **Auto tier**: Chooses `server` when `GOOP_FACE_ENGINE_URL` is configured; otherwise it falls back to `local`. ### Processing Pipeline -1. **Detect** -- InsightFace (buffalo_sc) finds faces and produces bounding boxes -2. **Align** -- Each face is cropped and resized to 112x112 -3. **Perturb** -- The ONNX generator (local) or diffusion model (server) produces adversarial perturbations -4. **Paste back** -- Protected crops are resized and blended back into the original image with soft Gaussian-blurred edges +1. **Local protect path** -- Detect with InsightFace, align to 112x112, run the ONNX generator, then paste protected crops back into the original image. +2. **Server protect/analyze path** -- Base64-encode the full input image, send it to the configured backend, and write the backend response to disk. ## Configuration diff --git a/SECURITY.md b/SECURITY.md index f143f87..934e31c 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -22,6 +22,13 @@ Email: kobepaw@proton.me - Local ONNX inference pipeline - Face detection integration +## Privacy and Data Flow Notes + +- `detect_faces` and the local protection path run in-process on the local machine. +- The first use of InsightFace downloads its model from upstream. +- Server-backed `protect` and `analyze` requests currently send the full input image to the configured `goop-face-engine` backend. +- `auto` mode prefers the configured backend over local inference. + Out of scope: - goop-face-engine (separate project) - InsightFace upstream vulnerabilities (report to their maintainers) diff --git a/src/goop_face/detector.py b/src/goop_face/detector.py index 78baace..965a759 100644 --- a/src/goop_face/detector.py +++ b/src/goop_face/detector.py @@ -1,7 +1,7 @@ """Client-side face detection and alignment. -Runs locally — no data leaves the machine. Uses InsightFace for detection -and alignment before sending to the backend for protection. +This module runs locally and returns detected/aligned face crops to the +caller. It does not perform network I/O itself. """ from __future__ import annotations @@ -26,8 +26,9 @@ class DetectedFace: class FaceDetector: """Face detection and alignment using InsightFace. - Face detection runs client-side so raw images never leave the user's - machine. Only the detected/aligned face crop is sent to the backend. + Detection and alignment happen client-side. This class returns bounding + boxes and aligned crops to the caller; whether image data is later sent + elsewhere depends on the calling code. """ def __init__(self, min_face_size: int = 30, det_thresh: float = 0.5): diff --git a/src/goop_face/mcp/server.py b/src/goop_face/mcp/server.py index 7ae85d6..7609ced 100644 --- a/src/goop_face/mcp/server.py +++ b/src/goop_face/mcp/server.py @@ -1,7 +1,9 @@ """MCP server exposing goop-face tools to AI agents. -Uses FastMCP over stdio transport. All tools accept file paths -and return JSON. Face detection is always local (privacy-preserving). +Uses FastMCP over stdio transport. All tools accept file paths and return +JSON. `detect_faces` stays local; `protect_face` and `analyze_face` may +process locally or upload the full input image to a configured backend, +depending on tier and tool. """ from __future__ import annotations @@ -26,10 +28,11 @@ mcp_server = FastMCP( "goop-face", instructions=( - "Facial privacy protection tools. Detects faces locally " - "(no data leaves the machine), then protects them from " - "recognition systems using adversarial perturbations. " - "Supports local CPU inference or GPU backend for higher quality." + "Facial privacy protection tools. Face detection stays local. " + "Local protection runs on-device; server-backed protection and " + "analysis upload the full input image to the configured backend " + "for processing. Supports local CPU inference or GPU backend for " + "higher quality." ), ) @@ -164,8 +167,13 @@ async def protect_face( ) -> str: """Protect a facial image from recognition systems. - Face detection runs locally — the original image never leaves your machine. - Only the detected face crop is processed (locally or sent to the GPU backend). + Data flow depends on the selected tier: + + - `local`: reads the full image locally, detects faces locally, and runs + the bundled ONNX model on aligned face crops. + - `server`: uploads the full input image to the configured backend client. + - `auto`: prefers `server` when a backend is configured; otherwise falls + back to `local`. Args: image_path: Absolute path to the input image file. @@ -305,7 +313,8 @@ async def detect_faces(image_path: str) -> str: async def analyze_face(image_path: str) -> str: """Analyze a face image's vulnerability to facial recognition systems. - Requires a running goop-face-engine backend (GOOP_FACE_ENGINE_URL). + Requires a running goop-face-engine backend (GOOP_FACE_ENGINE_URL) and + uploads the full input image to that backend. Args: image_path: Absolute path to the face image file. diff --git a/tests/test_client.py b/tests/test_client.py index 5f98047..b7502ec 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -3,6 +3,7 @@ from __future__ import annotations import base64 +import json from unittest.mock import patch import httpx @@ -61,10 +62,12 @@ class _CountingTransport(httpx.BaseTransport): def __init__(self, responses: list[httpx.Response]): self._responses = list(responses) self.call_count = 0 + self.requests: list[httpx.Request] = [] def handle_request(self, request: httpx.Request) -> httpx.Response: idx = min(self.call_count, len(self._responses) - 1) self.call_count += 1 + self.requests.append(request) return self._responses[idx] @@ -74,10 +77,12 @@ class _AsyncCountingTransport(httpx.AsyncBaseTransport): def __init__(self, responses: list[httpx.Response]): self._responses = list(responses) self.call_count = 0 + self.requests: list[httpx.Request] = [] async def handle_async_request(self, request: httpx.Request) -> httpx.Response: idx = min(self.call_count, len(self._responses) - 1) self.call_count += 1 + self.requests.append(request) return self._responses[idx] @@ -183,6 +188,31 @@ def test_progress_callback_called(self, tmp_image): assert isinstance(result, ProtectionResult) client.close() + def test_protect_uploads_full_input_image(self, tmp_image): + transport = _CountingTransport([_json_response(PROTECT_RESPONSE)]) + client = GoopFaceClient(base_url="http://test") + client._client = httpx.Client(transport=transport, base_url="http://test") + + client.protect(tmp_image) + + request = transport.requests[0] + payload = json.loads(request.read().decode()) + assert payload["image"] == base64.b64encode(tmp_image.read_bytes()).decode() + client.close() + + def test_analyze_uploads_full_input_image(self, tmp_image): + transport = _CountingTransport([_json_response({"ok": True})]) + client = GoopFaceClient(base_url="http://test") + client._client = httpx.Client(transport=transport, base_url="http://test") + + result = client.analyze(tmp_image) + + request = transport.requests[0] + payload = json.loads(request.read().decode()) + assert result == {"ok": True} + assert payload["image"] == base64.b64encode(tmp_image.read_bytes()).decode() + client.close() + # --------------------------------------------------------------------------- # TestAsyncGoopFaceClient diff --git a/tests/test_mcp/test_server.py b/tests/test_mcp/test_server.py index 6ae2e59..39a2e0f 100644 --- a/tests/test_mcp/test_server.py +++ b/tests/test_mcp/test_server.py @@ -288,7 +288,8 @@ async def test_local_tier(self, tmp_path): async def test_server_tier(self, tmp_path): mock_client = MagicMock() mock_client.protect.return_value = _mock_server_result() - _inject_server_client(mock_client) + mock_detector = MagicMock() + _inject_server_client(mock_client, mock_detector=mock_detector) img_path = _make_test_image(tmp_path) result = json.loads(await protect_face(str(img_path), tier="server")) @@ -298,7 +299,8 @@ async def test_server_tier(self, tmp_path): assert result["ssim"] == 0.91 assert result["faces_protected"] == 1 assert "output_path" in result - mock_client.protect.assert_called_once() + mock_client.protect.assert_called_once_with(str(img_path), mode="balanced") + mock_detector.detect.assert_not_called() @pytest.mark.asyncio async def test_auto_tier_prefers_server(self, tmp_path): @@ -306,19 +308,22 @@ async def test_auto_tier_prefers_server(self, tmp_path): mock_client.protect.return_value = _mock_server_result() mock_protector = MagicMock() mock_protector.protect.return_value = _mock_local_result() + mock_detector = MagicMock() from goop_face.mcp.config import MCPConfig server_module._config = MCPConfig(engine_url="http://test:8900") server_module._client = mock_client server_module._local_protector = mock_protector + server_module._detector = mock_detector img_path = _make_test_image(tmp_path) result = json.loads(await protect_face(str(img_path), tier="auto")) assert result["tier"] == "server" - mock_client.protect.assert_called_once() + mock_client.protect.assert_called_once_with(str(img_path), mode="balanced") mock_protector.protect.assert_not_called() + mock_detector.detect.assert_not_called() @pytest.mark.asyncio async def test_auto_tier_falls_back_to_local(self, tmp_path): From 7dc2d286f885e3ca742fd274f45bf54e22828698 Mon Sep 17 00:00:00 2001 From: Kobe Date: Sun, 15 Mar 2026 19:29:03 -0700 Subject: [PATCH 2/3] Harden MCP file boundaries --- src/goop_face/mcp/config.py | 28 +++- src/goop_face/mcp/server.py | 105 +++++++++++---- tests/test_mcp/test_server.py | 244 ++++++++++++++++++++++++++-------- 3 files changed, 290 insertions(+), 87 deletions(-) diff --git a/src/goop_face/mcp/config.py b/src/goop_face/mcp/config.py index 67bf4ca..a264326 100644 --- a/src/goop_face/mcp/config.py +++ b/src/goop_face/mcp/config.py @@ -6,7 +6,11 @@ from pathlib import Path from typing import Literal -from pydantic import BaseModel, ConfigDict, model_validator +from pydantic import BaseModel, ConfigDict, Field, model_validator + + +def _split_path_list(value: str) -> tuple[Path, ...]: + return tuple(Path(part) for part in value.split(os.pathsep) if part) class MCPConfig(BaseModel): @@ -25,6 +29,8 @@ class MCPConfig(BaseModel): api_key: str | None = None default_mode: Literal["stealth", "balanced", "maximum"] = "balanced" output_dir: Path = Path(".") + input_roots: tuple[Path, ...] = Field(default_factory=tuple) + output_roots: tuple[Path, ...] = Field(default_factory=tuple) @model_validator(mode="before") @classmethod @@ -34,10 +40,28 @@ def _load_env_defaults(cls, values: dict) -> dict: "api_key": "GOOP_FACE_API_KEY", "default_mode": "GOOP_FACE_DEFAULT_MODE", "output_dir": "GOOP_FACE_OUTPUT_DIR", + "input_roots": "GOOP_FACE_INPUT_ROOTS", + "output_roots": "GOOP_FACE_OUTPUT_ROOTS", } for field, env_var in env_map.items(): if field not in values: env_val = os.environ.get(env_var) if env_val: - values[field] = env_val + values[field] = ( + _split_path_list(env_val) + if field in {"input_roots", "output_roots"} + else env_val + ) return values + + @model_validator(mode="after") + def _apply_default_roots(self) -> MCPConfig: + cwd = Path.cwd() + output_root = self.output_dir if self.output_dir != Path(".") else cwd + + if not self.input_roots: + object.__setattr__(self, "input_roots", (cwd, output_root)) + if not self.output_roots: + object.__setattr__(self, "output_roots", (output_root,)) + + return self diff --git a/src/goop_face/mcp/server.py b/src/goop_face/mcp/server.py index 7609ced..e75cf1c 100644 --- a/src/goop_face/mcp/server.py +++ b/src/goop_face/mcp/server.py @@ -16,6 +16,7 @@ from pathlib import Path from mcp.server.fastmcp import FastMCP +from PIL import Image, UnidentifiedImageError # All logging to stderr — stdout reserved for MCP JSON-RPC logging.basicConfig( @@ -47,47 +48,91 @@ MAX_BATCH_SIZE = 50 -# Prefixes that must never be accessed via MCP tool inputs -_BLOCKED_PREFIXES = ("/etc", "/proc", "/sys", "/dev", "/var/run") +def _canonical_path(path: Path, *, must_exist: bool) -> Path: + expanded = path.expanduser() + try: + return expanded.resolve(strict=must_exist) + except FileNotFoundError: + raise -def _validate_image_path(path_str: str) -> Path: - """Validate and resolve image path, blocking traversal attempts.""" - # Block explicit traversal sequences before resolving - if ".." in path_str: - raise ValueError(f"Access denied: path traversal detected in {path_str}") - path = Path(path_str).resolve() +def _is_within_root(path: Path, root: Path) -> bool: + return path == root or root in path.parents + + +def _canonical_roots(roots: tuple[Path, ...]) -> tuple[Path, ...]: + return tuple(_canonical_path(root, must_exist=False) for root in roots) + + +def _ensure_allowed_path( + path: Path, + roots: tuple[Path, ...], + *, + label: str, + must_exist: bool, +) -> Path: + canonical_path = _canonical_path(path, must_exist=must_exist) + canonical_roots = _canonical_roots(roots) + + if not any(_is_within_root(canonical_path, root) for root in canonical_roots): + allowed = ", ".join(str(root) for root in canonical_roots) + raise ValueError(f"Access denied: {label} is outside approved roots ({allowed})") - if any(str(path).startswith(p) for p in _BLOCKED_PREFIXES): - raise ValueError(f"Access denied: {path_str}") + return canonical_path - if not path.exists(): - raise FileNotFoundError(f"Image not found: {path_str}") + +def _validate_image_file(path: Path) -> None: + try: + with Image.open(path) as image: + image.verify() + except (UnidentifiedImageError, OSError) as exc: + raise ValueError(f"Not an image file: {path}") from exc + + +def _validate_image_path(path_str: str) -> Path: + """Validate and resolve an image path inside approved input roots.""" + assert _config is not None + + raw_path = Path(path_str) + try: + path = _ensure_allowed_path( + raw_path, + _config.input_roots, + label="image_path", + must_exist=True, + ) + except FileNotFoundError as exc: + raise FileNotFoundError(f"Image not found: {path_str}") from exc if not path.is_file(): raise ValueError(f"Not a file: {path_str}") - # Block symlinks pointing outside their parent directory - if path.is_symlink(): - real = path.resolve() - parent = path.parent.resolve() - if not str(real).startswith(str(parent)): - raise ValueError(f"Access denied: symlink escapes parent directory: {path_str}") - + _validate_image_file(path) return path def _resolve_output_path(output_path: str, input_path: Path) -> Path: - """Resolve the output path, using config.output_dir as default directory.""" + """Resolve an output path inside approved output roots.""" assert _config is not None - if output_path: - return Path(output_path) - # Use config.output_dir as the default output directory - if _config.output_dir != Path("."): - _config.output_dir.mkdir(parents=True, exist_ok=True) - return _config.output_dir / f"{input_path.stem}_protected{input_path.suffix}" - return input_path.with_stem(f"{input_path.stem}_protected") + + raw_output = ( + Path(output_path) + if output_path + else ( + _config.output_dir / f"{input_path.stem}_protected{input_path.suffix}" + if _config.output_dir != Path(".") + else input_path.with_stem(f"{input_path.stem}_protected") + ) + ) + + canonical_parent = _ensure_allowed_path( + raw_output.parent, + _config.output_roots, + label="output_path", + must_exist=False, + ) + return canonical_parent / raw_output.name def _init(): @@ -204,11 +249,15 @@ async def protect_face( except ValueError as e: return json.dumps({"error": str(e)}) - out_path = _resolve_output_path(output_path, path) + try: + out_path = _resolve_output_path(output_path, path) + except (ValueError, FileNotFoundError) as e: + return json.dumps({"error": str(e)}) if selected_tier == "local": assert _local_protector is not None # guaranteed by _select_tier try: + out_path.parent.mkdir(parents=True, exist_ok=True) result = await asyncio.to_thread( _local_protector.protect, str(path), diff --git a/tests/test_mcp/test_server.py b/tests/test_mcp/test_server.py index 39a2e0f..e2ec1ed 100644 --- a/tests/test_mcp/test_server.py +++ b/tests/test_mcp/test_server.py @@ -3,6 +3,7 @@ from __future__ import annotations import json +import os from pathlib import Path from unittest.mock import MagicMock @@ -35,6 +36,12 @@ def _make_test_image(tmp_path: Path, name: str = "face.png") -> Path: return path +def _make_test_text_file(tmp_path: Path, name: str = "note.txt") -> Path: + path = tmp_path / name + path.write_text("not an image", encoding="utf-8") + return path + + def _mock_local_result(): """Create a mock LocalProtectionResult.""" mock = MagicMock() @@ -63,21 +70,34 @@ def _mock_server_result(): return mock -def _inject_local_protector(mock_protector): +def _inject_local_protector(mock_protector, root: Path | None = None): """Inject a mock local protector into the server module.""" from goop_face.mcp.config import MCPConfig - server_module._config = MCPConfig() + server_module._config = MCPConfig( + input_roots=(root or Path.cwd(),), + output_roots=((root or Path.cwd()),), + ) server_module._local_protector = mock_protector server_module._detector = None server_module._client = None -def _inject_server_client(mock_client, mock_detector=None): +def _inject_server_client( + mock_client, + mock_detector=None, + root: Path | None = None, + output_root: Path | None = None, +): """Inject a mock server client into the server module.""" from goop_face.mcp.config import MCPConfig - server_module._config = MCPConfig(engine_url="http://test:8900") + resolved_root = root or Path.cwd() + server_module._config = MCPConfig( + engine_url="http://test:8900", + input_roots=(resolved_root,), + output_roots=(output_root or resolved_root,), + ) server_module._client = mock_client server_module._detector = mock_detector server_module._local_protector = None @@ -97,6 +117,8 @@ def test_config_defaults(self): assert cfg.api_key is None assert cfg.default_mode == "balanced" assert cfg.output_dir == Path(".") + assert Path.cwd() in cfg.input_roots + assert Path.cwd() in cfg.output_roots def test_config_loads_from_env(self, monkeypatch): from goop_face.mcp.config import MCPConfig @@ -105,12 +127,19 @@ def test_config_loads_from_env(self, monkeypatch): monkeypatch.setenv("GOOP_FACE_API_KEY", "secret-key") monkeypatch.setenv("GOOP_FACE_DEFAULT_MODE", "maximum") monkeypatch.setenv("GOOP_FACE_OUTPUT_DIR", "/tmp/faces") + monkeypatch.setenv( + "GOOP_FACE_INPUT_ROOTS", + os.pathsep.join(("/tmp/input-a", "/tmp/input-b")), + ) + monkeypatch.setenv("GOOP_FACE_OUTPUT_ROOTS", "/tmp/output-a") cfg = MCPConfig() assert cfg.engine_url == "http://gpu:8900" assert cfg.api_key == "secret-key" assert cfg.default_mode == "maximum" assert cfg.output_dir == Path("/tmp/faces") + assert cfg.input_roots == (Path("/tmp/input-a"), Path("/tmp/input-b")) + assert cfg.output_roots == (Path("/tmp/output-a"),) def test_config_explicit_overrides_env(self, monkeypatch): from goop_face.mcp.config import MCPConfig @@ -182,11 +211,12 @@ def test_tier_auto_no_backends(self): class TestDetectFaces: @pytest.mark.asyncio - async def test_file_not_found(self): + async def test_file_not_found(self, tmp_path): from goop_face.mcp.config import MCPConfig - server_module._config = MCPConfig() - result = json.loads(await detect_faces("/nonexistent/image.png")) + missing_path = tmp_path / "missing.png" + server_module._config = MCPConfig(input_roots=(tmp_path,), output_roots=(tmp_path,)) + result = json.loads(await detect_faces(str(missing_path))) assert "error" in result assert "Image not found" in result["error"] @@ -194,9 +224,9 @@ async def test_file_not_found(self): async def test_no_detector(self, tmp_path): from goop_face.mcp.config import MCPConfig - server_module._config = MCPConfig() - server_module._detector = None img_path = _make_test_image(tmp_path) + server_module._config = MCPConfig(input_roots=(tmp_path,), output_roots=(tmp_path,)) + server_module._detector = None result = json.loads(await detect_faces(str(img_path))) assert "error" in result @@ -206,12 +236,12 @@ async def test_no_detector(self, tmp_path): async def test_success(self, tmp_path): from goop_face.mcp.config import MCPConfig - server_module._config = MCPConfig() mock_det = MagicMock() face = MagicMock() face.bbox = (10, 20, 110, 120) face.confidence = 0.9876 mock_det.detect.return_value = [face] + server_module._config = MCPConfig(input_roots=(tmp_path,), output_roots=(tmp_path,)) server_module._detector = mock_det img_path = _make_test_image(tmp_path) @@ -226,9 +256,9 @@ async def test_success(self, tmp_path): async def test_detection_error(self, tmp_path): from goop_face.mcp.config import MCPConfig - server_module._config = MCPConfig() mock_det = MagicMock() mock_det.detect.side_effect = RuntimeError("corrupt image") + server_module._config = MCPConfig(input_roots=(tmp_path,), output_roots=(tmp_path,)) server_module._detector = mock_det img_path = _make_test_image(tmp_path) @@ -244,11 +274,12 @@ async def test_detection_error(self, tmp_path): class TestProtectFace: @pytest.mark.asyncio - async def test_file_not_found(self): + async def test_file_not_found(self, tmp_path): from goop_face.mcp.config import MCPConfig - server_module._config = MCPConfig() - result = json.loads(await protect_face("/nonexistent/image.png")) + missing_path = tmp_path / "missing.png" + server_module._config = MCPConfig(input_roots=(tmp_path,), output_roots=(tmp_path,)) + result = json.loads(await protect_face(str(missing_path))) assert "error" in result assert "Image not found" in result["error"] @@ -256,8 +287,8 @@ async def test_file_not_found(self): async def test_invalid_mode(self, tmp_path): from goop_face.mcp.config import MCPConfig - server_module._config = MCPConfig() img_path = _make_test_image(tmp_path) + server_module._config = MCPConfig(input_roots=(tmp_path,), output_roots=(tmp_path,)) result = json.loads(await protect_face(str(img_path), mode="ultra")) assert "error" in result @@ -267,7 +298,7 @@ async def test_invalid_mode(self, tmp_path): async def test_local_tier(self, tmp_path): mock_protector = MagicMock() mock_protector.protect.return_value = _mock_local_result() - _inject_local_protector(mock_protector) + _inject_local_protector(mock_protector, root=tmp_path) img_path = _make_test_image(tmp_path) result = json.loads(await protect_face(str(img_path), tier="local", mode="balanced")) @@ -289,7 +320,7 @@ async def test_server_tier(self, tmp_path): mock_client = MagicMock() mock_client.protect.return_value = _mock_server_result() mock_detector = MagicMock() - _inject_server_client(mock_client, mock_detector=mock_detector) + _inject_server_client(mock_client, mock_detector=mock_detector, root=tmp_path) img_path = _make_test_image(tmp_path) result = json.loads(await protect_face(str(img_path), tier="server")) @@ -312,7 +343,11 @@ async def test_auto_tier_prefers_server(self, tmp_path): from goop_face.mcp.config import MCPConfig - server_module._config = MCPConfig(engine_url="http://test:8900") + server_module._config = MCPConfig( + engine_url="http://test:8900", + input_roots=(tmp_path,), + output_roots=(tmp_path,), + ) server_module._client = mock_client server_module._local_protector = mock_protector server_module._detector = mock_detector @@ -329,7 +364,7 @@ async def test_auto_tier_prefers_server(self, tmp_path): async def test_auto_tier_falls_back_to_local(self, tmp_path): mock_protector = MagicMock() mock_protector.protect.return_value = _mock_local_result() - _inject_local_protector(mock_protector) + _inject_local_protector(mock_protector, root=tmp_path) img_path = _make_test_image(tmp_path) result = json.loads(await protect_face(str(img_path), tier="auto")) @@ -340,7 +375,7 @@ async def test_auto_tier_falls_back_to_local(self, tmp_path): async def test_custom_output_path(self, tmp_path): mock_protector = MagicMock() mock_protector.protect.return_value = _mock_local_result() - _inject_local_protector(mock_protector) + _inject_local_protector(mock_protector, root=tmp_path) img_path = _make_test_image(tmp_path) out = tmp_path / "custom_output.png" @@ -348,11 +383,39 @@ async def test_custom_output_path(self, tmp_path): assert result["output_path"] == str(out) + @pytest.mark.asyncio + async def test_output_path_outside_approved_root_rejected(self, tmp_path): + mock_protector = MagicMock() + mock_protector.protect.return_value = _mock_local_result() + _inject_local_protector(mock_protector, root=tmp_path) + + img_path = _make_test_image(tmp_path) + outside_path = tmp_path.parent / "escape.png" + result = json.loads( + await protect_face(str(img_path), tier="local", output_path=str(outside_path)) + ) + + assert "error" in result + assert "outside approved roots" in result["error"] + mock_protector.protect.assert_not_called() + + @pytest.mark.asyncio + async def test_server_tier_rejects_non_image_before_upload(self, tmp_path): + mock_client = MagicMock() + _inject_server_client(mock_client, root=tmp_path) + + text_path = _make_test_text_file(tmp_path) + result = json.loads(await protect_face(str(text_path), tier="server")) + + assert "error" in result + assert "Not an image file" in result["error"] + mock_client.protect.assert_not_called() + @pytest.mark.asyncio async def test_server_tier_backend_error(self, tmp_path): mock_client = MagicMock() mock_client.protect.side_effect = ConnectionError("refused") - _inject_server_client(mock_client) + _inject_server_client(mock_client, root=tmp_path) img_path = _make_test_image(tmp_path) result = json.loads(await protect_face(str(img_path), tier="server")) @@ -364,7 +427,7 @@ async def test_server_tier_backend_error(self, tmp_path): async def test_local_tier_protection_error(self, tmp_path): mock_protector = MagicMock() mock_protector.protect.side_effect = RuntimeError("ONNX inference failed") - _inject_local_protector(mock_protector) + _inject_local_protector(mock_protector, root=tmp_path) img_path = _make_test_image(tmp_path) result = json.loads(await protect_face(str(img_path), tier="local")) @@ -383,19 +446,20 @@ class TestAnalyzeFace: async def test_no_backend(self, tmp_path): from goop_face.mcp.config import MCPConfig - server_module._config = MCPConfig() img_path = _make_test_image(tmp_path) + server_module._config = MCPConfig(input_roots=(tmp_path,), output_roots=(tmp_path,)) result = json.loads(await analyze_face(str(img_path))) assert "error" in result assert "No backend configured" in result["error"] @pytest.mark.asyncio - async def test_file_not_found(self): + async def test_file_not_found(self, tmp_path): from goop_face.mcp.config import MCPConfig - server_module._config = MCPConfig() - result = json.loads(await analyze_face("/nonexistent/image.png")) + missing_path = tmp_path / "missing.png" + server_module._config = MCPConfig(input_roots=(tmp_path,), output_roots=(tmp_path,)) + result = json.loads(await analyze_face(str(missing_path))) assert "error" in result assert "Image not found" in result["error"] @@ -409,7 +473,7 @@ async def test_success(self, tmp_path): {"name": "FaceNet", "match_score": 0.88}, ], } - _inject_server_client(mock_client) + _inject_server_client(mock_client, root=tmp_path) img_path = _make_test_image(tmp_path) result = json.loads(await analyze_face(str(img_path))) @@ -422,7 +486,7 @@ async def test_success(self, tmp_path): async def test_analysis_error(self, tmp_path): mock_client = MagicMock() mock_client.analyze.side_effect = RuntimeError("model error") - _inject_server_client(mock_client) + _inject_server_client(mock_client, root=tmp_path) img_path = _make_test_image(tmp_path) result = json.loads(await analyze_face(str(img_path))) @@ -440,7 +504,7 @@ class TestBatchProtect: async def test_batch_success(self, tmp_path): mock_protector = MagicMock() mock_protector.protect.return_value = _mock_local_result() - _inject_local_protector(mock_protector) + _inject_local_protector(mock_protector, root=tmp_path) paths = [str(_make_test_image(tmp_path, f"face_{i}.png")) for i in range(3)] result = json.loads(await batch_protect(paths, tier="local", mode="balanced")) @@ -456,10 +520,10 @@ async def test_batch_success(self, tmp_path): async def test_batch_mixed_results(self, tmp_path): mock_protector = MagicMock() mock_protector.protect.return_value = _mock_local_result() - _inject_local_protector(mock_protector) + _inject_local_protector(mock_protector, root=tmp_path) good_path = str(_make_test_image(tmp_path, "good.png")) - bad_path = "/nonexistent/image.png" + bad_path = str(tmp_path / "missing.png") result = json.loads( await batch_protect([good_path, bad_path], tier="local", mode="balanced") ) @@ -472,7 +536,7 @@ async def test_batch_mixed_results(self, tmp_path): async def test_batch_empty_list(self): from goop_face.mcp.config import MCPConfig - server_module._config = MCPConfig() + server_module._config = MCPConfig(input_roots=(Path.cwd(),), output_roots=(Path.cwd(),)) result = json.loads(await batch_protect([])) assert "error" in result assert "must not be empty" in result["error"] @@ -481,7 +545,7 @@ async def test_batch_empty_list(self): async def test_batch_exceeds_max_size(self): from goop_face.mcp.config import MCPConfig - server_module._config = MCPConfig() + server_module._config = MCPConfig(input_roots=(Path.cwd(),), output_roots=(Path.cwd(),)) paths = [f"/fake/img_{i}.png" for i in range(MAX_BATCH_SIZE + 1)] result = json.loads(await batch_protect(paths)) assert "error" in result @@ -494,58 +558,119 @@ async def test_batch_exceeds_max_size(self): class TestPathValidation: - def test_path_traversal_blocked(self): - with pytest.raises(ValueError, match="path traversal"): - _validate_image_path("../../etc/passwd") + def test_rejects_path_outside_allowlisted_roots(self, tmp_path): + from goop_face.mcp.config import MCPConfig - def test_blocked_prefix_etc(self, tmp_path): - with pytest.raises(ValueError, match="Access denied"): - _validate_image_path("/etc/passwd") + img_path = _make_test_image(tmp_path, "outside.png") + server_module._config = MCPConfig( + input_roots=(tmp_path / "allowed",), + output_roots=(tmp_path / "allowed",), + ) + + with pytest.raises(ValueError, match="outside approved roots"): + _validate_image_path(str(img_path)) + + def test_file_not_found(self, tmp_path): + from goop_face.mcp.config import MCPConfig - def test_blocked_prefix_proc(self): - with pytest.raises(ValueError, match="Access denied"): - _validate_image_path("/proc/self/environ") + missing_path = tmp_path / "missing.png" + server_module._config = MCPConfig(input_roots=(tmp_path,), output_roots=(tmp_path,)) - def test_file_not_found(self): with pytest.raises(FileNotFoundError, match="Image not found"): - _validate_image_path("/tmp/nonexistent_test_image_12345.png") + _validate_image_path(str(missing_path)) def test_not_a_file(self, tmp_path): + from goop_face.mcp.config import MCPConfig + + server_module._config = MCPConfig(input_roots=(tmp_path,), output_roots=(tmp_path,)) with pytest.raises(ValueError, match="Not a file"): _validate_image_path(str(tmp_path)) def test_valid_path(self, tmp_path): + from goop_face.mcp.config import MCPConfig + img_path = _make_test_image(tmp_path) + server_module._config = MCPConfig(input_roots=(tmp_path,), output_roots=(tmp_path,)) result = _validate_image_path(str(img_path)) assert result.exists() assert result.is_file() + def test_dotdot_path_inside_allowlisted_root_is_allowed(self, tmp_path): + from goop_face.mcp.config import MCPConfig + + nested_dir = tmp_path / "nested" + nested_dir.mkdir() + img_path = _make_test_image(tmp_path) + path_with_dotdot = nested_dir / ".." / img_path.name + server_module._config = MCPConfig(input_roots=(tmp_path,), output_roots=(tmp_path,)) + + result = _validate_image_path(str(path_with_dotdot)) + assert result == img_path.resolve() + + def test_symlink_escape_is_rejected(self, tmp_path): + from goop_face.mcp.config import MCPConfig + + allowed_root = tmp_path / "allowed" + allowed_root.mkdir() + outside_root = tmp_path / "outside" + outside_root.mkdir() + outside_image = _make_test_image(outside_root, "outside.png") + symlink_path = allowed_root / "linked.png" + symlink_path.symlink_to(outside_image) + server_module._config = MCPConfig(input_roots=(allowed_root,), output_roots=(allowed_root,)) + + with pytest.raises(ValueError, match="outside approved roots"): + _validate_image_path(str(symlink_path)) + + def test_canonicalized_root_alias_is_allowed(self, tmp_path): + from goop_face.mcp.config import MCPConfig + + real_root = tmp_path / "real" + real_root.mkdir() + alias_root = tmp_path / "alias" + alias_root.symlink_to(real_root, target_is_directory=True) + img_path = _make_test_image(real_root, "aliased.png") + server_module._config = MCPConfig(input_roots=(alias_root,), output_roots=(alias_root,)) + + result = _validate_image_path(str(img_path)) + assert result == img_path.resolve() + @pytest.mark.asyncio - async def test_protect_face_blocks_traversal(self, tmp_path): + async def test_protect_face_rejects_path_outside_allowlist(self, tmp_path): from goop_face.mcp.config import MCPConfig - server_module._config = MCPConfig() - result = json.loads(await protect_face("../../etc/shadow")) + img_path = _make_test_text_file(tmp_path, "outside.txt") + server_module._config = MCPConfig( + input_roots=(tmp_path / "allowed",), + output_roots=(tmp_path / "allowed",), + ) + result = json.loads(await protect_face(str(img_path))) assert "error" in result - assert "path traversal" in result["error"] + assert "outside approved roots" in result["error"] @pytest.mark.asyncio - async def test_detect_faces_blocks_traversal(self, tmp_path): + async def test_detect_faces_rejects_non_image(self, tmp_path): from goop_face.mcp.config import MCPConfig - server_module._config = MCPConfig() - result = json.loads(await detect_faces("/proc/self/maps")) + text_path = _make_test_text_file(tmp_path) + server_module._config = MCPConfig(input_roots=(tmp_path,), output_roots=(tmp_path,)) + result = json.loads(await detect_faces(str(text_path))) assert "error" in result - assert "Access denied" in result["error"] + assert "Not an image file" in result["error"] @pytest.mark.asyncio - async def test_analyze_face_blocks_traversal(self, tmp_path): + async def test_analyze_face_rejects_non_image(self, tmp_path): from goop_face.mcp.config import MCPConfig - server_module._config = MCPConfig() - result = json.loads(await analyze_face("/sys/class/dmi/id/product_name")) + text_path = _make_test_text_file(tmp_path) + server_module._config = MCPConfig( + engine_url="http://test:8900", + input_roots=(tmp_path,), + output_roots=(tmp_path,), + ) + result = json.loads(await analyze_face(str(text_path))) assert "error" in result - assert "Access denied" in result["error"] + assert "Not an image file" in result["error"] # --------------------------------------------------------------------------- @@ -575,7 +700,11 @@ async def test_default_mode_used_when_not_specified(self, tmp_path): from goop_face.mcp.config import MCPConfig - server_module._config = MCPConfig(default_mode="maximum") + server_module._config = MCPConfig( + default_mode="maximum", + input_roots=(tmp_path,), + output_roots=(tmp_path,), + ) server_module._local_protector = mock_protector server_module._detector = None server_module._client = None @@ -591,3 +720,4 @@ def test_output_dir_from_env(self, monkeypatch, tmp_path): monkeypatch.setenv("GOOP_FACE_OUTPUT_DIR", str(tmp_path / "output")) cfg = MCPConfig() assert cfg.output_dir == tmp_path / "output" + assert cfg.output_roots == (tmp_path / "output",) From cd5ba0ebcee674ddb42ac6d749fd3b6e1f6c3b48 Mon Sep 17 00:00:00 2001 From: Kobe Date: Sun, 15 Mar 2026 19:29:57 -0700 Subject: [PATCH 3/3] Harden install and release trust paths --- .github/workflows/ci.yml | 14 +++++++++ .github/workflows/release.yml | 17 ++++++++++- .github/workflows/security.yml | 10 ++++--- README.md | 12 ++++++++ scripts/smoke_wheel_install.sh | 51 ++++++++++++++++++++++++++++++++ src/goop_face/mcp/__init__.py | 47 +++++++++++++++++++++++++++-- src/goop_face/mcp/__main__.py | 4 +-- tests/test_mcp_entrypoint.py | 54 ++++++++++++++++++++++++++++++++++ 8 files changed, 200 insertions(+), 9 deletions(-) create mode 100755 scripts/smoke_wheel_install.sh create mode 100644 tests/test_mcp_entrypoint.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bd3037f..ef27ac6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -69,3 +69,17 @@ jobs: - run: pip install -e ".[dev]" - run: pip install mypy - run: mypy src/ + + wheel-smoke: + name: Wheel Smoke + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4 + with: + lfs: true + - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5 + with: + python-version: "3.12" + cache: pip + - run: PYTHON_BIN=python bash scripts/smoke_wheel_install.sh diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 99d9656..a0277f5 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -28,11 +28,26 @@ jobs: - run: pip install -e ".[dev,mcp]" - run: pytest tests/ -v -m "not integration" + wheel-smoke: + name: Wheel install smoke + runs-on: ubuntu-latest + timeout-minutes: 15 + needs: [test] + steps: + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4 + with: + lfs: true + - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5 + with: + python-version: "3.12" + cache: pip + - run: PYTHON_BIN=python bash scripts/smoke_wheel_install.sh + publish: name: Publish to PyPI runs-on: ubuntu-latest timeout-minutes: 20 - needs: [test] + needs: [test, wheel-smoke] environment: pypi steps: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4 diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 1a74607..3d991f0 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -62,7 +62,9 @@ jobs: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4 with: fetch-depth: 0 - - uses: gitleaks/gitleaks-action@v2 - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - continue-on-error: false + - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5 + with: + python-version: "3.12" + cache: pip + - run: pip install "gitleaks==8.24.2" + - run: gitleaks git --source . --redact --no-banner --exit-code 1 diff --git a/README.md b/README.md index 000fcfd..6b4df5b 100644 --- a/README.md +++ b/README.md @@ -24,6 +24,10 @@ For MCP server support (adds the `goop-face-mcp` entry point): pip install "goop-face[mcp]" ``` +If you install the base package only, `goop-face-mcp` exits with a short +install hint instead of a Python traceback. The MCP runtime is optional by +design. + For development: ```bash @@ -135,6 +139,9 @@ for face in faces: goop-face exposes tools via [MCP (Model Context Protocol)](https://modelcontextprotocol.io/) for AI agent integration. The `goop-face-mcp` entry point runs a FastMCP server over stdio transport. +The MCP server only exposes stdio transport. Keep it behind the MCP host +process rather than exposing it directly on the network. + ### Claude Code Add to your project (creates `.mcp.json` in the project root): @@ -150,6 +157,11 @@ Add to your project (creates `.mcp.json` in the project root): } ``` +For production backends, prefer an `https://...` URL or a loopback endpoint +that you tunnel separately. `GOOP_FACE_API_KEY` is only forwarded to the +configured backend as an `Authorization: Bearer ...` header and is not used +during local-only operation. + Or add globally to `~/.claude/settings.local.json` with backend configuration: ```json diff --git a/scripts/smoke_wheel_install.sh b/scripts/smoke_wheel_install.sh new file mode 100755 index 0000000..4dccff3 --- /dev/null +++ b/scripts/smoke_wheel_install.sh @@ -0,0 +1,51 @@ +#!/usr/bin/env bash + +set -euo pipefail + +PYTHON_BIN="${PYTHON_BIN:-python3}" + +"${PYTHON_BIN}" - <<'PY' +import sys + +if sys.version_info < (3, 10): + raise SystemExit("smoke_wheel_install.sh requires Python 3.10+") +PY + +tmpdir="$(mktemp -d)" +trap 'rm -rf "${tmpdir}"' EXIT + +"${PYTHON_BIN}" -m venv "${tmpdir}/build-venv" +build_bin="${tmpdir}/build-venv/bin" + +"${build_bin}/pip" install --upgrade pip build +rm -rf dist build +"${build_bin}/python" -m build --wheel + +wheel_path="$(find dist -maxdepth 1 -name '*.whl' | head -n 1)" +if [[ -z "${wheel_path}" ]]; then + echo "wheel build did not produce a dist/*.whl artifact" >&2 + exit 1 +fi + +"${PYTHON_BIN}" -m venv "${tmpdir}/venv" +venv_bin="${tmpdir}/venv/bin" + +"${venv_bin}/pip" install --upgrade pip +"${venv_bin}/pip" install "${wheel_path}" +"${venv_bin}/goop-face" --help >/dev/null + +set +e +mcp_output="$("${venv_bin}/goop-face-mcp" 2>&1 >/dev/null)" +mcp_status=$? +set -e + +if [[ "${mcp_status}" -eq 0 ]]; then + echo "goop-face-mcp unexpectedly succeeded without optional MCP dependencies" >&2 + exit 1 +fi + +printf '%s' "${mcp_output}" | grep -F 'pip install "goop-face[mcp]"' >/dev/null + +"${venv_bin}/pip" install "mcp>=1.2.0,<2" +"${venv_bin}/python" -c "from goop_face.mcp import mcp_server; print(mcp_server.name)" \ + | grep -Fx "goop-face" >/dev/null diff --git a/src/goop_face/mcp/__init__.py b/src/goop_face/mcp/__init__.py index fd8bbca..9cd6e71 100644 --- a/src/goop_face/mcp/__init__.py +++ b/src/goop_face/mcp/__init__.py @@ -1,5 +1,48 @@ -"""goop-face MCP server — facial privacy protection tools for AI agents.""" +"""goop-face MCP server entrypoints. + +The ``goop-face-mcp`` console script is always installed so the base package has +a stable public interface. The actual MCP runtime remains optional and is +loaded lazily so missing extras produce an actionable error instead of a raw +import traceback. +""" + +from __future__ import annotations + +import importlib +import sys + +_MCP_INSTALL_HINT = ( + 'goop-face-mcp requires the optional "mcp" dependency. ' + 'Install it with: pip install "goop-face[mcp]"' +) + + +def _load_server_module(): + try: + return importlib.import_module("goop_face.mcp.server") + except ModuleNotFoundError as exc: + missing_root = (exc.name or "").split(".", 1)[0] + if missing_root == "mcp": + raise RuntimeError(_MCP_INSTALL_HINT) from exc + raise + + +def main() -> int: + """Run the MCP server or print a dependency hint for base installs.""" + try: + server_module = _load_server_module() + except RuntimeError as exc: + print(str(exc), file=sys.stderr) + return 1 + + server_module.main() + return 0 + + +def __getattr__(name: str): + if name == "mcp_server": + return _load_server_module().mcp_server + raise AttributeError(f"module {__name__!r} has no attribute {name!r}") -from goop_face.mcp.server import main, mcp_server __all__ = ["main", "mcp_server"] diff --git a/src/goop_face/mcp/__main__.py b/src/goop_face/mcp/__main__.py index 5fa96a2..9a3a18a 100644 --- a/src/goop_face/mcp/__main__.py +++ b/src/goop_face/mcp/__main__.py @@ -1,5 +1,5 @@ """Allow ``python -m goop_face.mcp`` to start the MCP server.""" -from goop_face.mcp.server import main +from goop_face.mcp import main -main() +raise SystemExit(main()) diff --git a/tests/test_mcp_entrypoint.py b/tests/test_mcp_entrypoint.py new file mode 100644 index 0000000..e898eba --- /dev/null +++ b/tests/test_mcp_entrypoint.py @@ -0,0 +1,54 @@ +"""Tests for the public MCP entrypoint behavior.""" + +from __future__ import annotations + +import types + +import pytest + +import goop_face.mcp as mcp_module + + +def test_main_prints_install_hint_when_mcp_extra_is_missing(monkeypatch, capsys): + def fake_import_module(name: str): + assert name == "goop_face.mcp.server" + raise ModuleNotFoundError("No module named 'mcp'", name="mcp") + + monkeypatch.setattr(mcp_module.importlib, "import_module", fake_import_module) + + exit_code = mcp_module.main() + + captured = capsys.readouterr() + assert exit_code == 1 + assert 'pip install "goop-face[mcp]"' in captured.err + assert captured.out == "" + + +def test_main_runs_server_when_optional_dependency_is_available(monkeypatch): + calls: list[str] = [] + + def fake_main(): + calls.append("ran") + + fake_server_module = types.SimpleNamespace(main=fake_main, mcp_server=object()) + + monkeypatch.setattr( + mcp_module.importlib, + "import_module", + lambda name: fake_server_module, + ) + + exit_code = mcp_module.main() + + assert exit_code == 0 + assert calls == ["ran"] + + +def test_mcp_server_attribute_raises_helpful_error_when_extra_is_missing(monkeypatch): + def fake_import_module(name: str): + raise ModuleNotFoundError("No module named 'mcp'", name="mcp") + + monkeypatch.setattr(mcp_module.importlib, "import_module", fake_import_module) + + with pytest.raises(RuntimeError, match='pip install "goop-face\\[mcp\\]"'): + _ = mcp_module.mcp_server