From 9f92debf000b3b162ff81ff06ea6d3e7a22dce25 Mon Sep 17 00:00:00 2001 From: dongmucat <1127093059@qq.com> Date: Thu, 24 Sep 2026 15:01:28 +0800 Subject: [PATCH] fix(scanner): redact mounted route findings Signed-off-by: dongmucat <1127093059@qq.com> --- scanner/skillhub_scanner_app.py | 81 ++++++++++++++++++---- scanner/tests/test_skillhub_scanner_app.py | 56 ++++++++++++++- scripts/tests/scanner-2-1-contract-test.sh | 54 +++++++++++++++ 3 files changed, 178 insertions(+), 13 deletions(-) diff --git a/scanner/skillhub_scanner_app.py b/scanner/skillhub_scanner_app.py index 3c38e22b..c008d17e 100644 --- a/scanner/skillhub_scanner_app.py +++ b/scanner/skillhub_scanner_app.py @@ -1,6 +1,7 @@ """Runtime safeguards around the upstream Cisco Skill Scanner ASGI application.""" import asyncio +import inspect import logging import os import shutil @@ -46,6 +47,7 @@ _upstream_router.MAX_UPLOAD_SIZE_BYTES = max( _active_scans = 0 _active_scans_guard = asyncio.Lock() _SCAN_PATHS = {"/scan", "/scan-upload"} +_REDACTION_MARKER = "_skillhub_redaction_installed" _log = logging.getLogger(__name__) @@ -74,23 +76,78 @@ def _redact_supported_tokens(value): return value -def _install_scan_response_redaction() -> None: - """Redact supported token forms before FastAPI serializes scan findings.""" - for route in _upstream_router.router.routes: - if getattr(route, "path", None) not in _SCAN_PATHS or "POST" not in getattr(route, "methods", set()): +def _iter_route_objects(container, seen=None): + """Walk FastAPI/Starlette route containers, including mounted child routers.""" + seen = set() if seen is None else seen + routes = getattr(container, "routes", None) + if routes is None: + nested_router = getattr(container, "router", None) + if nested_router is not None and nested_router is not container: + yield from _iter_route_objects(nested_router, seen) + return + for route in routes: + route_id = id(route) + if route_id in seen: continue - endpoint = route.endpoint + seen.add(route_id) + yield route + for nested in ( + getattr(route, "original_router", None), + getattr(route, "app", None), + getattr(route, "router", None), + ): + yield from _iter_route_objects(nested, seen) + +def _redact_scan_response(response): + findings = getattr(response, "findings", None) + if isinstance(findings, list): + response.findings = _redact_supported_tokens(findings) + return response + + +def _make_redacting_endpoint(endpoint): + if inspect.iscoroutinefunction(endpoint): @wraps(endpoint) async def redacting_endpoint(*args, __endpoint=endpoint, **kwargs): - response = await __endpoint(*args, **kwargs) - findings = getattr(response, "findings", None) - if isinstance(findings, list): - response.findings = _redact_supported_tokens(findings) - return response + return _redact_scan_response(await __endpoint(*args, **kwargs)) + else: + @wraps(endpoint) + def redacting_endpoint(*args, __endpoint=endpoint, **kwargs): + return _redact_scan_response(__endpoint(*args, **kwargs)) + return redacting_endpoint - route.endpoint = redacting_endpoint - route.dependant.call = redacting_endpoint + +def _install_scan_response_redaction() -> None: + """Redact supported token forms before FastAPI serializes scan findings.""" + upstream_routes = list(_iter_route_objects(_upstream_router.router)) + target_routes = [ + route + for route in upstream_routes + if getattr(route, "path", None) in _SCAN_PATHS + and "POST" in getattr(route, "methods", set()) + ] + if not target_routes: + raise RuntimeError("Scanner routes /scan and /scan-upload were not found") + target_endpoints = {route.endpoint for route in target_routes} + target_paths = {route.path for route in target_routes} + all_routes = [ + *list(_iter_route_objects(getattr(app, "router", app))), + *upstream_routes, + ] + for route in all_routes: + endpoint = getattr(route, "endpoint", None) + path = getattr(route, "path", None) + is_target_endpoint = endpoint in target_endpoints + is_target_path = path in target_paths and "POST" in getattr(route, "methods", set()) + if not (is_target_endpoint or is_target_path) or getattr(route, _REDACTION_MARKER, False): + continue + wrapped_endpoint = _make_redacting_endpoint(endpoint) + route.endpoint = wrapped_endpoint + dependant = getattr(route, "dependant", None) + if dependant is not None: + dependant.call = wrapped_endpoint + setattr(route, _REDACTION_MARKER, True) def _restart_after_hard_timeout(request_path: str) -> NoReturn: diff --git a/scanner/tests/test_skillhub_scanner_app.py b/scanner/tests/test_skillhub_scanner_app.py index de9a0672..2c341706 100644 --- a/scanner/tests/test_skillhub_scanner_app.py +++ b/scanner/tests/test_skillhub_scanner_app.py @@ -12,8 +12,9 @@ from unittest.mock import patch class _FakeRouter: - def __init__(self): + def __init__(self, routes=None): self.handlers = [] + self.routes = routes or [] def add_event_handler(self, _event, _handler): self.handlers.append((_event, _handler)) @@ -52,6 +53,9 @@ class _FakeApp: _FakeScanResponse([{"description": "No credentials", "metadata": {"safe": True}}]), ), ] + mounted_routes = [route.clone("/mounted") for route in self.upstream_routes] + nested_routes = [route.clone("/nested") for route in self.upstream_routes] + self.router.routes = [_FakeIncludedRouter(mounted_routes), _FakeMount(nested_routes)] def middleware(self, _kind): return lambda function: function @@ -75,6 +79,7 @@ class _FakeRoute: def __init__(self, path, response): self.path = path self.methods = {"POST"} + self._response = response async def endpoint(): return response @@ -82,6 +87,22 @@ class _FakeRoute: self.endpoint = endpoint self.dependant = types.SimpleNamespace(call=endpoint) + def clone(self, prefix=""): + clone = _FakeRoute(f"{prefix}{self.path}", self._response) + clone.endpoint = self.endpoint + clone.dependant = types.SimpleNamespace(call=self.endpoint) + return clone + + +class _FakeIncludedRouter: + def __init__(self, routes): + self.original_router = types.SimpleNamespace(routes=routes) + + +class _FakeMount: + def __init__(self, routes): + self.app = types.SimpleNamespace(router=types.SimpleNamespace(routes=routes)) + class _Request: method = "POST" @@ -235,6 +256,39 @@ class SkillHubScannerAppTest(unittest.IsolatedAsyncioTestCase): self.assertEqual(200, response.status_code) self.assertEqual({"X-Contract": "preserved"}, response.headers) + async def test_mounted_app_route_also_redacts_findings(self): + included = next(route for route in self.module.app.router.routes if isinstance(route, _FakeIncludedRouter)) + route = next(route for route in included.original_router.routes if route.path == "/mounted/scan") + + response = await route.endpoint() + + self.assertNotIn("ghp_", str(response.findings)) + self.assertNotIn("sk-proj-", str(response.findings)) + self.assertEqual(200, response.status_code) + self.assertEqual({"X-Contract": "preserved"}, response.headers) + + async def test_nested_fastapi_app_route_also_redacts_findings(self): + mount = next(route for route in self.module.app.router.routes if isinstance(route, _FakeMount)) + route = next(route for route in mount.app.router.routes if route.path == "/nested/scan") + + response = await route.endpoint() + + self.assertNotIn("ghp_", str(response.findings)) + self.assertNotIn("sk-proj-", str(response.findings)) + self.assertEqual(200, response.status_code) + self.assertEqual({"X-Contract": "preserved"}, response.headers) + + async def test_sync_endpoint_wrapper_redacts_findings(self): + response = _FakeScanResponse([{"description": "api_key=custom-secret-1234567890"}]) + + def endpoint(): + return response + + wrapped = self.module._make_redacting_endpoint(endpoint) + + self.assertIs(response, wrapped()) + self.assertNotIn("custom-secret-1234567890", str(response.findings)) + async def test_safe_scan_findings_are_unchanged(self): route = next(route for route in self.module._router_stub.router.routes if route.path == "/scan-upload") expected = [{"description": "No credentials", "metadata": {"safe": True}}] diff --git a/scripts/tests/scanner-2-1-contract-test.sh b/scripts/tests/scanner-2-1-contract-test.sh index b2bb54ea..58d5a319 100755 --- a/scripts/tests/scanner-2-1-contract-test.sh +++ b/scripts/tests/scanner-2-1-contract-test.sh @@ -187,7 +187,13 @@ MAIN_URL="http://127.0.0.1:$MAIN_PORT" wait_for_health "$MAIN_URL" docker exec -i "$MAIN_CONTAINER" python - <<'PY' +from types import SimpleNamespace + +from fastapi import APIRouter, FastAPI, Response +from fastapi.testclient import TestClient +from pydantic import BaseModel from skill_scanner.core.analyzers.llm_analyzer import LLMProvider +import skillhub_scanner_app as scanner_app from skillhub_scanner_app import _redact_supported_tokens canaries = { @@ -217,6 +223,54 @@ if _redact_supported_tokens(mixed) != "before\napi_key=\tafter": raise SystemExit("redaction changed non-secret characters around a credential") if not LLMProvider.is_valid_provider("azure-openai") or LLMProvider.is_valid_provider("azure"): raise SystemExit("unexpected Scanner 2.1 Azure provider contract") + + +class ScanResponse(BaseModel): + findings: list[dict] + safe_text: str + + +upstream_router = APIRouter() + + +def install_http_canary(path): + @upstream_router.post(path, response_model=ScanResponse) + async def scan(response: Response): + response.headers["X-Contract"] = "preserved" + return ScanResponse( + findings=[{"description": f"api_key={canaries['labeled']}"}], + safe_text="line one\n" + ("x" * 5000), + ) + +install_http_canary("/scan") +install_http_canary("/scan-upload") +http_app = FastAPI() +http_app.include_router(upstream_router) +nested_app = FastAPI() +nested_app.include_router(upstream_router) +http_app.mount("/nested", nested_app) + +previous_app = scanner_app.app +previous_router = scanner_app._upstream_router +scanner_app.app = http_app +scanner_app._upstream_router = SimpleNamespace(router=upstream_router) +try: + scanner_app._install_scan_response_redaction() + with TestClient(http_app) as client: + for path in ("/scan", "/scan-upload", "/nested/scan", "/nested/scan-upload"): + response = client.post(path) + if response.status_code != 200: + raise SystemExit(f"HTTP canary failed for {path}: {response.status_code}") + if response.headers.get("X-Contract") != "preserved": + raise SystemExit(f"HTTP canary changed headers for {path}") + body = response.text + if canaries["labeled"] in body: + raise SystemExit(f"HTTP canary leaked secret for {path}") + if "line one\\n" not in body or ("x" * 5000) not in body: + raise SystemExit(f"HTTP canary changed safe text for {path}") +finally: + scanner_app.app = previous_app + scanner_app._upstream_router = previous_router PY docker exec "$MAIN_CONTAINER" python -c \