From ba0398bb1a2c16b1004bb23c24368e6a5988cac0 Mon Sep 17 00:00:00 2001 From: dongmucat <1127093059@qq.com> Date: Thu, 17 Sep 2026 17:07:01 +0800 Subject: [PATCH 1/9] feat(scanner): upgrade runtime to 2.1.0 Signed-off-by: dongmucat <1127093059@qq.com> --- scanner/Dockerfile | 23 +- .../apply_1_0_2_llm_base_url_backport.py | 62 ---- scanner/skillhub_scanner_app.py | 43 ++- scanner/tests/test_skillhub_scanner_app.py | 95 +++++- scripts/tests/scanner-2-1-contract-test.sh | 288 ++++++++++++++++++ scripts/tests/scanner-llm-base-url-test.sh | 232 -------------- 6 files changed, 431 insertions(+), 312 deletions(-) delete mode 100644 scanner/backports/apply_1_0_2_llm_base_url_backport.py create mode 100755 scripts/tests/scanner-2-1-contract-test.sh delete mode 100755 scripts/tests/scanner-llm-base-url-test.sh diff --git a/scanner/Dockerfile b/scanner/Dockerfile index 7d937a94..fb0eea8d 100644 --- a/scanner/Dockerfile +++ b/scanner/Dockerfile @@ -1,27 +1,20 @@ -FROM python:3.11-alpine - -ARG SKILL_SCANNER_VERSION=1.0.2 +FROM python:3.11-slim-bookworm WORKDIR /app -COPY backports/apply_1_0_2_llm_base_url_backport.py /tmp/apply_1_0_2_llm_base_url_backport.py -COPY skillhub_scanner_app.py /app/skillhub_scanner_app.py +RUN pip install --no-cache-dir --only-binary=:all: \ + "cisco-ai-skill-scanner==2.1.0" && \ + groupadd --system app && \ + useradd --system --gid app --home-dir /nonexistent --no-create-home app && \ + install -d -o app -g app /tmp/skillhub-scans -RUN pip install --no-cache-dir \ - "cisco-ai-skill-scanner==${SKILL_SCANNER_VERSION}" \ - "litellm==1.90.2" && \ - python /tmp/apply_1_0_2_llm_base_url_backport.py /usr/local/lib/python3.11/site-packages && \ - rm /tmp/apply_1_0_2_llm_base_url_backport.py && \ - addgroup -S app && \ - adduser -S app -G app && \ - mkdir -p /tmp/skillhub-scans && \ - chown app:app /tmp/skillhub-scans +COPY skillhub_scanner_app.py /app/skillhub_scanner_app.py USER app EXPOSE 8000 HEALTHCHECK --interval=10s --timeout=3s \ - CMD wget -qO- http://127.0.0.1:8000/health || exit 1 + CMD ["python", "-c", "import urllib.request; urllib.request.urlopen('http://127.0.0.1:8000/health', timeout=2).read()"] CMD ["uvicorn", "skillhub_scanner_app:app", "--host", "0.0.0.0", "--port", "8000"] diff --git a/scanner/backports/apply_1_0_2_llm_base_url_backport.py b/scanner/backports/apply_1_0_2_llm_base_url_backport.py deleted file mode 100644 index 7ea6bbd6..00000000 --- a/scanner/backports/apply_1_0_2_llm_base_url_backport.py +++ /dev/null @@ -1,62 +0,0 @@ -#!/usr/bin/env python3 -"""Backport SKILL_SCANNER_LLM_BASE_URL support into cisco-ai-skill-scanner 1.0.2.""" - -from __future__ import annotations - -import re -import sys -from pathlib import Path - -EXPECTED_DIST_INFO = "cisco_ai_skill_scanner-1.0.2.dist-info" -ROUTER_RELATIVE_PATH = Path("skill_scanner/api/router.py") - -def replace_exact(content: str, old: str, new: str, expected_count: int, label: str) -> str: - actual_count = content.count(old) - if actual_count != expected_count: - raise SystemExit(f"Expected {expected_count} occurrences of {label}, found {actual_count}.") - return content.replace(old, new, expected_count) - - -def replace_regex(content: str, pattern: str, replacement: str, expected_count: int, label: str) -> str: - updated, actual_count = re.subn(pattern, replacement, content, count=expected_count, flags=re.MULTILINE) - if actual_count != expected_count: - raise SystemExit(f"Expected {expected_count} regex replacements for {label}, found {actual_count}.") - return updated - - -def main() -> int: - site_packages = Path(sys.argv[1]) if len(sys.argv) > 1 else Path("/usr/local/lib/python3.11/site-packages") - dist_info = site_packages / EXPECTED_DIST_INFO - if not dist_info.exists(): - raise SystemExit(f"Expected {EXPECTED_DIST_INFO} under {site_packages}, but it was not found.") - - router_path = site_packages / ROUTER_RELATIVE_PATH - content = router_path.read_text(encoding="utf-8") - content = replace_regex( - content, - r'^(?P\s*)llm_model = os.getenv\("SKILL_SCANNER_LLM_MODEL"\)$', - r'\g<0>\n\gllm_base_url = os.getenv("SKILL_SCANNER_LLM_BASE_URL")', - 2, - "llm_model environment lookup", - ) - content = replace_exact( - content, - "LLMAnalyzer(model=llm_model)", - "LLMAnalyzer(model=llm_model, base_url=llm_base_url)", - 2, - "LLMAnalyzer model constructor", - ) - content = replace_exact( - content, - "LLMAnalyzer(provider=provider_str)", - "LLMAnalyzer(provider=provider_str, base_url=llm_base_url)", - 2, - "LLMAnalyzer provider constructor", - ) - - router_path.write_text(content, encoding="utf-8") - return 0 - - -if __name__ == "__main__": - raise SystemExit(main()) diff --git a/scanner/skillhub_scanner_app.py b/scanner/skillhub_scanner_app.py index 17ab130c..2ddacab4 100644 --- a/scanner/skillhub_scanner_app.py +++ b/scanner/skillhub_scanner_app.py @@ -3,8 +3,11 @@ import asyncio import logging import os +import re import shutil import tempfile +from functools import wraps +from importlib import import_module from pathlib import Path from typing import NoReturn @@ -13,19 +16,56 @@ from fastapi.responses import JSONResponse from skill_scanner.api.api import app +_upstream_router = import_module("skill_scanner.api.router") _MAX_CONCURRENT_SCANS = max(1, int(os.getenv("SKILLHUB_SCANNER_MAX_CONCURRENT_SCANS", "1"))) _HARD_TIMEOUT_SECONDS = max(1, int(os.getenv("SKILLHUB_SCANNER_HARD_TIMEOUT_SECONDS", "930"))) +_upstream_router.MAX_UPLOAD_SIZE_BYTES = max( + 1, int(os.getenv("SKILLHUB_SCANNER_MAX_UPLOAD_SIZE_BYTES", "110100480")) +) _active_scans = 0 _active_scans_guard = asyncio.Lock() _SCAN_PATHS = {"/scan", "/scan-upload"} +_SUPPORTED_TOKEN_PATTERN = re.compile( + r"\b(?:gh[pousr]_[A-Za-z0-9]{20,255}|sk-(?:proj-)?[A-Za-z0-9_-]{20,255})\b" +) _log = logging.getLogger(__name__) +def _redact_supported_tokens(value): + if isinstance(value, str): + return _SUPPORTED_TOKEN_PATTERN.sub("", value) + if isinstance(value, list): + return [_redact_supported_tokens(item) for item in value] + if isinstance(value, dict): + return {key: _redact_supported_tokens(item) for key, item in value.items()} + 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()): + continue + endpoint = route.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 + + route.endpoint = redacting_endpoint + route.dependant.call = redacting_endpoint + + def _cleanup_stale_scan_directories(temp_root: Path | None = None) -> None: """Remove incomplete upstream extraction directories left by a process restart.""" root = temp_root or Path(tempfile.gettempdir()) + current_upload_root = Path(_upstream_router._API_UPLOAD_ROOT).resolve() for candidate in root.glob("skill_scanner_*"): - if not candidate.is_dir(): + if not candidate.is_dir() or candidate.resolve() == current_upload_root: continue try: shutil.rmtree(candidate) @@ -54,6 +94,7 @@ async def _await_scan_until(scan_task: asyncio.Task, deadline: float, request_pa _restart_after_hard_timeout(request_path) +_install_scan_response_redaction() app.router.add_event_handler("startup", _cleanup_stale_scan_directories) diff --git a/scanner/tests/test_skillhub_scanner_app.py b/scanner/tests/test_skillhub_scanner_app.py index fd58469a..7972ac2e 100644 --- a/scanner/tests/test_skillhub_scanner_app.py +++ b/scanner/tests/test_skillhub_scanner_app.py @@ -1,5 +1,6 @@ import asyncio import importlib.util +import os import sys import tempfile import types @@ -19,6 +20,26 @@ class _FakeRouter: class _FakeApp: def __init__(self): self.router = _FakeRouter() + self.routes = [object()] + github_canary = "ghp_" + "A1b2C3d4E5f6G7h8I9j0K1l2M3n4O5p6Q7r8" + openai_canary = "sk-proj-" + "Z9y8X7w6V5u4T3s2R1q0" * 3 + self.upstream_routes = [ + _FakeRoute( + "/scan", + _FakeScanResponse( + [ + { + "description": f"YARA match: {github_canary}", + "metadata": {"evidence": openai_canary}, + } + ] + ), + ), + _FakeRoute( + "/scan-upload", + _FakeScanResponse([{"description": "No credentials", "metadata": {"safe": True}}]), + ), + ] def middleware(self, _kind): return lambda function: function @@ -31,30 +52,55 @@ class _FakeResponse: self.headers = headers +class _FakeScanResponse: + def __init__(self, findings): + self.findings = findings + self.status_code = 200 + self.headers = {"X-Contract": "preserved"} + + +class _FakeRoute: + def __init__(self, path, response): + self.path = path + self.methods = {"POST"} + + async def endpoint(): + return response + + self.endpoint = endpoint + self.dependant = types.SimpleNamespace(call=endpoint) + + class _Request: method = "POST" url = types.SimpleNamespace(path="/scan-upload") -def _load_module(): +def _load_module(environment=None): fastapi = types.ModuleType("fastapi") fastapi.Request = object responses = types.ModuleType("fastapi.responses") responses.JSONResponse = _FakeResponse api = types.ModuleType("skill_scanner.api.api") api.app = _FakeApp() + router = types.ModuleType("skill_scanner.api.router") + router.MAX_UPLOAD_SIZE_BYTES = -1 + router._API_UPLOAD_ROOT = Path(tempfile.gettempdir()) / "skill_scanner_current" + router.router = types.SimpleNamespace(routes=api.app.upstream_routes) stubs = { "fastapi": fastapi, "fastapi.responses": responses, "skill_scanner": types.ModuleType("skill_scanner"), "skill_scanner.api": types.ModuleType("skill_scanner.api"), "skill_scanner.api.api": api, + "skill_scanner.api.router": router, } - with patch.dict(sys.modules, stubs): + with patch.dict(os.environ, environment or {}, clear=True), patch.dict(sys.modules, stubs): module_path = Path(__file__).parents[1] / "skillhub_scanner_app.py" spec = importlib.util.spec_from_file_location("skillhub_scanner_app_under_test", module_path) module = importlib.util.module_from_spec(spec) spec.loader.exec_module(module) + module._router_stub = router return module @@ -120,6 +166,51 @@ class SkillHubScannerAppTest(unittest.IsolatedAsyncioTestCase): self.assertFalse(stale.exists()) self.assertTrue(unrelated.exists()) + async def test_startup_cleanup_keeps_current_upstream_upload_directory(self): + with tempfile.TemporaryDirectory() as temp_root: + root = Path(temp_root) + current = root / "skill_scanner_current" + stale = root / "skill_scanner_stale" + current.mkdir() + stale.mkdir() + self.module._router_stub._API_UPLOAD_ROOT = current + + self.module._cleanup_stale_scan_directories(root) + + self.assertTrue(current.exists()) + self.assertFalse(stale.exists()) + + async def test_default_upload_limit_matches_skillhub_package_limit(self): + self.assertEqual(110100480, self.module._router_stub.MAX_UPLOAD_SIZE_BYTES) + + async def test_upload_limit_can_be_overridden_by_environment(self): + module = _load_module({"SKILLHUB_SCANNER_MAX_UPLOAD_SIZE_BYTES": "123456"}) + + self.assertEqual(123456, module._router_stub.MAX_UPLOAD_SIZE_BYTES) + + async def test_upload_limit_is_at_least_one_byte(self): + module = _load_module({"SKILLHUB_SCANNER_MAX_UPLOAD_SIZE_BYTES": "0"}) + + self.assertEqual(1, module._router_stub.MAX_UPLOAD_SIZE_BYTES) + + async def test_scan_findings_redact_supported_tokens_without_changing_response_contract(self): + route = next(route for route in self.module._router_stub.router.routes if route.path == "/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_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}}] + + response = await route.dependant.call() + + self.assertEqual(expected, response.findings) + async def test_startup_cleanup_is_registered_on_the_upstream_router(self): self.assertEqual( [("startup", self.module._cleanup_stale_scan_directories)], diff --git a/scripts/tests/scanner-2-1-contract-test.sh b/scripts/tests/scanner-2-1-contract-test.sh new file mode 100755 index 00000000..3cf02eab --- /dev/null +++ b/scripts/tests/scanner-2-1-contract-test.sh @@ -0,0 +1,288 @@ +#!/usr/bin/env bash +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +TMP_DIR="$(mktemp -d)" +CONTAINERS=() + +cleanup() { + local status=$? + trap - EXIT + if ((${#CONTAINERS[@]})); then + docker rm -f "${CONTAINERS[@]}" >/dev/null 2>&1 || true + fi + rm -rf "$TMP_DIR" + exit "$status" +} +trap cleanup EXIT + +if [[ -n "${SCANNER_IMAGE:-}" ]]; then + IMAGE="$SCANNER_IMAGE" +else + IMAGE="skillhub-scanner-contract-test:$(date +%s)" + docker build -t "$IMAGE" "$REPO_ROOT/scanner" +fi + +python3 - "$TMP_DIR" <<'PY' +import json +import sys +import zipfile +from pathlib import Path + +root = Path(sys.argv[1]) +github_canary = "ghp_" + "A1b2C3d4E5f6G7h8I9j0K1l2M3n4O5p6Q7r8" +openai_canary = "sk-proj-" + "Z9y8X7w6V5u4T3s2R1q0" * 3 + + +def write_zip(name, files): + with zipfile.ZipFile(root / name, "w", compression=zipfile.ZIP_STORED) as archive: + for path, content in files.items(): + archive.writestr(path, content) + + +manifest = """--- +name: contract-skill +description: Scanner 2.1 runtime contract fixture. +--- + +Harmless contract fixture. +""" +write_zip("safe.zip", {"contract-skill/SKILL.md": manifest}) +write_zip( + "javascript-secret.zip", + { + "contract-skill/SKILL.md": manifest, + "contract-skill/index.js": f'const githubToken = "{github_canary}";\n', + }, +) +write_zip( + "dotenv-secret.zip", + { + "contract-skill/SKILL.md": manifest, + "contract-skill/.env": f"OPENAI_API_KEY={openai_canary}\n", + }, +) + +large_files = {"large-skill/SKILL.md": manifest.replace("contract-skill", "large-skill")} +for index in range(5): + large_files[f"large-skill/payload-{index}.txt"] = b"x" * (10 * 1024 * 1024) +large_files["large-skill/payload-5.txt"] = b"x" * (1024 * 1024) +write_zip("large-51mib.zip", large_files) + +(root / "canaries.json").write_text( + json.dumps({"github": github_canary, "openai": openai_canary}), encoding="utf-8" +) + +local_skill = root / "local-scan" / "contract-skill" +local_skill.mkdir(parents=True) +(local_skill / "SKILL.md").write_text(manifest, encoding="utf-8") +PY + +start_scanner() { + local name=$1 + shift + docker run -d --name "$name" -p 127.0.0.1::8000 "$@" "$IMAGE" >/dev/null + CONTAINERS+=("$name") + SCANNER_PORT="$(docker port "$name" 8000/tcp | awk -F: 'END {print $NF}')" +} + +wait_for_health() { + local url=$1 + python3 - "$url" <<'PY' +import json +import sys +import time +import urllib.error +import urllib.request + +url = sys.argv[1] +last_error = None +for _ in range(120): + try: + with urllib.request.urlopen(url + "/health", timeout=2) as response: + payload = json.load(response) + if response.status == 200: + break + except (OSError, urllib.error.URLError, json.JSONDecodeError) as error: + last_error = error + time.sleep(1) +else: + raise SystemExit(f"scanner did not become healthy: {last_error}") + +if payload.get("version") != "2.1.0": + raise SystemExit(f"expected scanner 2.1.0, got {payload!r}") +expected = {"static_analyzer", "bytecode_analyzer", "pipeline_analyzer"} +available = set(payload.get("analyzers_available", [])) +if not expected.issubset(available): + raise SystemExit(f"missing deterministic analyzers: {sorted(expected - available)}") +PY +} + +post_zip() { + local url=$1 + local archive=$2 + local output=$3 + python3 - "$url" "$archive" "$output" <<'PY' +import json +import sys +import urllib.error +import urllib.request +import uuid +from pathlib import Path + +url, archive_path, output_path = sys.argv[1:] +boundary = "----skillhub-" + uuid.uuid4().hex +archive = Path(archive_path).read_bytes() +prefix = ( + f"--{boundary}\r\n" + 'Content-Disposition: form-data; name="policy"\r\n\r\n' + "balanced\r\n" + f"--{boundary}\r\n" + f'Content-Disposition: form-data; name="file"; filename="{Path(archive_path).name}"\r\n' + "Content-Type: application/zip\r\n\r\n" +).encode() +body = prefix + archive + f"\r\n--{boundary}--\r\n".encode() +request = urllib.request.Request( + url + "/scan-upload", + data=body, + headers={"Content-Type": f"multipart/form-data; boundary={boundary}"}, + method="POST", +) +try: + with urllib.request.urlopen(request, timeout=900) as response: + raw = response.read() + status = response.status +except urllib.error.HTTPError as error: + raw = error.read() + status = error.code +Path(output_path).write_bytes(raw) +if status != 200: + raise SystemExit(f"scan upload returned HTTP {status}: {raw[:1000]!r}") +try: + payload = json.loads(raw) +except json.JSONDecodeError as error: + raise SystemExit(f"scan upload did not return JSON: {error}") from error +required = {"scan_id", "skill_name", "findings", "scan_metadata"} +if not required.issubset(payload) or not isinstance(payload["findings"], list): + raise SystemExit(f"invalid scan response contract: {payload!r}") +PY +} + +assert_hardcoded_secret() { + local response=$1 + python3 - "$response" <<'PY' +import json +import sys + +payload = json.load(open(sys.argv[1], encoding="utf-8")) +if not any(finding.get("category") == "hardcoded_secrets" for finding in payload["findings"]): + raise SystemExit("expected a hardcoded_secrets finding") +PY +} + +MAIN_CONTAINER="skillhub-scanner-contract-main-$$" +start_scanner "$MAIN_CONTAINER" +MAIN_PORT="$SCANNER_PORT" +MAIN_URL="http://127.0.0.1:$MAIN_PORT" +wait_for_health "$MAIN_URL" + +post_zip "$MAIN_URL" "$TMP_DIR/safe.zip" "$TMP_DIR/safe.json" +post_zip "$MAIN_URL" "$TMP_DIR/javascript-secret.zip" "$TMP_DIR/javascript-secret.json" +post_zip "$MAIN_URL" "$TMP_DIR/dotenv-secret.zip" "$TMP_DIR/dotenv-secret.json" +post_zip "$MAIN_URL" "$TMP_DIR/large-51mib.zip" "$TMP_DIR/large-51mib.json" + +assert_hardcoded_secret "$TMP_DIR/javascript-secret.json" +assert_hardcoded_secret "$TMP_DIR/dotenv-secret.json" + +python3 - "$TMP_DIR/safe.json" <<'PY' +import json +import sys + +payload = json.load(open(sys.argv[1], encoding="utf-8")) +cel = payload.get("scan_metadata", {}).get("cel", {}) +if cel.get("runtime") != "cel-go": + raise SystemExit(f"expected CEL runtime cel-go, got {cel!r}") +if not cel.get("runtime_version"): + raise SystemExit(f"expected a non-empty CEL runtime version, got {cel!r}") +if cel.get("fallbacks") != 0 or cel.get("errors") != []: + raise SystemExit(f"unexpected CEL fallback/error telemetry: {cel!r}") +PY + +docker logs "$MAIN_CONTAINER" >"$TMP_DIR/main-container.log" 2>&1 +python3 - "$TMP_DIR/canaries.json" "$TMP_DIR" <<'PY' +import json +import sys +from pathlib import Path + +canaries = json.load(open(sys.argv[1], encoding="utf-8")) +root = Path(sys.argv[2]) +for path in [*root.glob("*.json"), root / "main-container.log"]: + if path.name == "canaries.json": + continue + content = path.read_text(encoding="utf-8", errors="replace") + for label, canary in canaries.items(): + if canary in content: + def find_canary(value, location="$"): + if isinstance(value, dict): + for key, child in value.items(): + found = find_canary(child, f"{location}.{key}") + if found: + return found + elif isinstance(value, list): + for index, child in enumerate(value): + found = find_canary(child, f"{location}[{index}]") + if found: + return found + elif isinstance(value, str) and canary in value: + return location + return None + + location = None + if path.suffix == ".json": + location = find_canary(json.loads(content)) + raise SystemExit( + f"full {label} canary leaked through {path.name}" + + (f" at {location}" if location else "") + ) +PY + +LOCAL_CONTAINER="skillhub-scanner-contract-local-$$" +start_scanner "$LOCAL_CONTAINER" \ + -e SKILL_SCANNER_ALLOWED_ROOTS=/tmp/skillhub-scans \ + -v "$TMP_DIR/local-scan:/tmp/skillhub-scans:ro" +LOCAL_PORT="$SCANNER_PORT" +LOCAL_URL="http://127.0.0.1:$LOCAL_PORT" +wait_for_health "$LOCAL_URL" + +python3 - "$LOCAL_URL" <<'PY' +import json +import sys +import urllib.error +import urllib.request + +url = sys.argv[1] + + +def scan(path): + request = urllib.request.Request( + url + "/scan", + data=json.dumps({"skill_directory": path, "policy": "balanced"}).encode(), + headers={"Content-Type": "application/json"}, + method="POST", + ) + try: + with urllib.request.urlopen(request, timeout=900) as response: + return response.status, json.load(response) + except urllib.error.HTTPError as error: + return error.code, json.loads(error.read()) + + +inside_status, inside = scan("/tmp/skillhub-scans/contract-skill") +if inside_status != 200 or not isinstance(inside, dict): + raise SystemExit(f"allowed local scan failed: HTTP {inside_status}: {inside!r}") +outside_status, outside = scan("/etc") +if outside_status not in (403, 404) or not isinstance(outside, dict): + raise SystemExit(f"outside-root scan should be denied, got HTTP {outside_status}: {outside!r}") +PY + +echo "scanner-2-1-contract-test passed" diff --git a/scripts/tests/scanner-llm-base-url-test.sh b/scripts/tests/scanner-llm-base-url-test.sh deleted file mode 100755 index 79378b61..00000000 --- a/scripts/tests/scanner-llm-base-url-test.sh +++ /dev/null @@ -1,232 +0,0 @@ -#!/usr/bin/env bash -set -euo pipefail - -REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" -SCANNER_DIR="$REPO_ROOT/scanner" -TMP_DIRS=() - -cleanup() { - local status=$? - local d - for d in "${TMP_DIRS[@]+"${TMP_DIRS[@]}"}"; do - rm -rf "$d" - done - exit "$status" -} -trap cleanup EXIT - -new_tmp() { - local d - d="$(mktemp -d)" - TMP_DIRS+=("$d") - echo "$d" -} - -fail() { - echo "FAIL: $*" >&2 - exit 1 -} - -tmp="$(new_tmp)" -skill_dir="$tmp/skill" -mkdir -p "$skill_dir/demo-skill" - -cat >"$skill_dir/demo-skill/SKILL.md" <<'EOF' ---- -name: demo-skill -description: Minimal valid skill used for scanner integration coverage. -license: Apache-2.0 ---- - -This is a harmless demo skill used for scanner integration testing. -EOF - -cat >"$skill_dir/demo-skill/run.sh" <<'EOF' -#!/usr/bin/env sh -echo "demo" -EOF -chmod +x "$skill_dir/demo-skill/run.sh" - -IMAGE_TAG="skillhub-scanner-llm-base-url-test:$(date +%s)" -docker build --no-cache -t "$IMAGE_TAG" "$SCANNER_DIR" >/dev/null - -docker run --rm -i \ - -v "$skill_dir:/work/skill:ro" \ - --entrypoint python \ - "$IMAGE_TAG" - <<'PY' -import asyncio -from datetime import datetime, timezone -import http.server -import io -import inspect -import json -import os -from pathlib import Path -import threading -import urllib.request -import zipfile - -from fastapi.params import Query -from skill_scanner.core.models import ScanResult -import skill_scanner.api.router as router - -signature = inspect.signature(router.scan_uploaded_skill) -if not isinstance(signature.parameters["use_llm"].default, Query): - raise SystemExit("scan-upload use_llm should remain a Query parameter") -if not isinstance(signature.parameters["llm_provider"].default, Query): - raise SystemExit("scan-upload llm_provider should remain a Query parameter") - -state = {"base_urls": [], "paths": []} - - -class Handler(http.server.BaseHTTPRequestHandler): - def log_message(self, format, *args): # noqa: A003 - return - - def do_POST(self): # noqa: N802 - length = int(self.headers.get("content-length", "0")) - self.rfile.read(length) - state["paths"].append(self.path) - - payload = json.dumps( - { - "id": "chatcmpl-test", - "object": "chat.completion", - "created": int(datetime.now(timezone.utc).timestamp()), - "model": "local-model", - "choices": [ - { - "index": 0, - "message": {"role": "assistant", "content": "No findings."}, - "finish_reason": "stop", - } - ], - "usage": {"prompt_tokens": 1, "completion_tokens": 1, "total_tokens": 2}, - } - ).encode("utf-8") - self.send_response(200) - self.send_header("Content-Type", "application/json") - self.send_header("Content-Length", str(len(payload))) - self.end_headers() - self.wfile.write(payload) - - -server = http.server.HTTPServer(("127.0.0.1", 0), Handler) -thread = threading.Thread(target=server.serve_forever, daemon=True) -thread.start() - -target_base_url = f"http://127.0.0.1:{server.server_port}/v1" -os.environ["SKILL_SCANNER_LLM_BASE_URL"] = target_base_url -os.environ["SKILL_SCANNER_LLM_MODEL"] = "test-model" - - -class FakeStaticAnalyzer: - pass - - -class FakeLLMAnalyzer: - def __init__(self, model=None, provider=None, base_url=None): - self.model = model - self.provider = provider - self.base_url = base_url - state["base_urls"].append(base_url) - - def analyze(self, skill_path): - request = urllib.request.Request( - self.base_url + "/chat/completions", - data=b"{}", - headers={"Content-Type": "application/json"}, - method="POST", - ) - with urllib.request.urlopen(request, timeout=5) as response: - response.read() - - -class FakeSkillScanner: - def __init__(self, analyzers): - self.analyzers = analyzers - - def scan_skill(self, skill_path): - for analyzer in self.analyzers: - analyze = getattr(analyzer, "analyze", None) - if callable(analyze): - analyze(skill_path) - - return ScanResult( - skill_name="demo-skill", - skill_directory=str(skill_path), - findings=[], - scan_duration_seconds=0.05, - analyzers_used=["fake-llm"], - timestamp=datetime.now(timezone.utc), - ) - - -router.StaticAnalyzer = FakeStaticAnalyzer -router.LLMAnalyzer = FakeLLMAnalyzer -router.SkillScanner = FakeSkillScanner -router.LLM_AVAILABLE = True - -request = router.ScanRequest( - skill_directory="/work/skill/demo-skill", - use_llm=True, - llm_provider="openai", - use_behavioral=False, - use_aidefense=False, - aidefense_api_key=None, -) - -def build_skill_archive_bytes(skill_root: str) -> bytes: - skill_path = Path(skill_root) - buffer = io.BytesIO() - with zipfile.ZipFile(buffer, "w", compression=zipfile.ZIP_DEFLATED) as archive: - for path in skill_path.rglob("*"): - if path.is_file(): - archive.writestr(str(path.relative_to(skill_path.parent)), path.read_bytes()) - return buffer.getvalue() - -class FakeUploadFile: - def __init__(self, filename: str, payload: bytes): - self.filename = filename - self._payload = payload - - async def read(self) -> bytes: - return self._payload - -try: - direct_response = asyncio.run(router.scan_skill(request)) - - upload_response = asyncio.run( - router.scan_uploaded_skill( - file=FakeUploadFile("demo-skill.zip", build_skill_archive_bytes("/work/skill/demo-skill")), - use_llm=True, - llm_provider="openai", - use_behavioral=False, - use_aidefense=False, - aidefense_api_key=None, - ) - ) -finally: - server.shutdown() - thread.join(timeout=5) - -if not getattr(direct_response, "scan_id", None): - raise SystemExit("scan_skill should still return a scan response") -if not getattr(upload_response, "scan_id", None): - raise SystemExit("scan_uploaded_skill should still return a scan response") -if len(state["base_urls"]) != 2: - raise SystemExit(f"expected two LLM analyzer constructions, got {len(state['base_urls'])}") -if any(base_url != target_base_url for base_url in state["base_urls"]): - raise SystemExit(f"expected every base_url to be {target_base_url}, got {state['base_urls']}") -if len(state["paths"]) != 2: - raise SystemExit(f"expected two LLM requests, got {state['paths']}") -if not all(path.startswith("/v1/") for path in state["paths"]): - raise SystemExit(f"expected every request path to start with /v1/, got {state['paths']}") -PY - -grep -Fq "name: SKILL_SCANNER_LLM_BASE_URL" "$REPO_ROOT/deploy/k8s/base/scanner-deployment.yaml" \ - || fail "Kubernetes scanner deployment must expose SKILL_SCANNER_LLM_BASE_URL" -grep -Fq "skill-scanner-llm-base-url" "$REPO_ROOT/deploy/k8s/base/secret.yaml.example" \ - || fail "Kubernetes secret example must document skill-scanner-llm-base-url" - -echo "scanner-llm-base-url-test passed" From 9bbadca31b2bf0bc2c663745cc9b5e0300e659bd Mon Sep 17 00:00:00 2001 From: dongmucat <1127093059@qq.com> Date: Thu, 17 Sep 2026 18:08:25 +0800 Subject: [PATCH 2/9] fix(security): clarify safe audit verdict Signed-off-by: dongmucat <1127093059@qq.com> --- web/e2e/security-audit-redaction.spec.ts | 173 ++++++++++++++++++ .../security-audit/finding-item.test.tsx | 14 ++ web/src/i18n/locales/en.json | 2 +- web/src/i18n/locales/ru.json | 2 +- web/src/i18n/locales/zh.json | 2 +- web/src/i18n/security-audit-locale.test.ts | 16 ++ 6 files changed, 206 insertions(+), 3 deletions(-) create mode 100644 web/e2e/security-audit-redaction.spec.ts diff --git a/web/e2e/security-audit-redaction.spec.ts b/web/e2e/security-audit-redaction.spec.ts new file mode 100644 index 00000000..5ac14092 --- /dev/null +++ b/web/e2e/security-audit-redaction.spec.ts @@ -0,0 +1,173 @@ +import { expect, test, type Page, type Route } from '@playwright/test' +import { setEnglishLocale } from './helpers/auth-fixtures' + +const CANARY = 'SYNTHETIC-CANARY-868' +const MASKED_SNIPPET = 'const token = "[REDACTED]"' + +function envelope(data: unknown) { + return { + code: 0, + msg: 'success', + data, + timestamp: '2026-09-17T00:00:00Z', + requestId: 'security-audit-redaction-fixture', + } +} + +async function fulfill(route: Route, data: unknown) { + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify(envelope(data)), + }) +} + +async function mockSkillDetail(page: Page, audits: unknown[]) { + await page.route(/\/api\//, async (route) => { + const url = new URL(route.request().url()) + const { pathname } = url + + if (!pathname.startsWith('/api/')) { + await route.continue() + return + } + + if (pathname === '/api/v1/auth/me') { + await fulfill(route, { + userId: 'audit-admin', + displayName: 'Audit Admin', + platformRoles: ['SUPER_ADMIN'], + oauthProvider: 'local', + canChangePassword: true, + }) + return + } + + if (pathname === '/api/web/skills/global/redaction-skill') { + await fulfill(route, { + id: 868, + slug: 'redaction-skill', + displayName: 'Redaction Skill', + ownerId: 'skill-owner', + ownerDisplayName: 'Skill Owner', + summary: 'Security audit redaction fixture', + visibility: 'PUBLIC', + status: 'ACTIVE', + downloadCount: 0, + starCount: 0, + ratingCount: 0, + hidden: false, + namespace: 'global', + canManageLifecycle: false, + canSubmitPromotion: false, + canInteract: false, + canReport: false, + headlineVersion: { id: 8681, version: '1.0.0', status: 'PUBLISHED' }, + publishedVersion: { id: 8681, version: '1.0.0', status: 'PUBLISHED' }, + resolutionMode: 'PUBLISHED', + labels: [], + }) + return + } + + if (pathname === '/api/web/skills/global/redaction-skill/versions') { + await fulfill(route, { + items: [{ + id: 8681, + version: '1.0.0', + status: 'PUBLISHED', + fileCount: 0, + totalSize: 0, + publishedAt: '2026-09-17T00:00:00Z', + downloadAvailable: true, + }], + total: 1, + page: 0, + size: 20, + }) + return + } + + if (pathname === '/api/web/skills/global/redaction-skill/versions/1.0.0/files') { + await fulfill(route, []) + return + } + + if (pathname === '/api/v1/skills/868/versions/8681/security-audit') { + await fulfill(route, audits) + return + } + + if (pathname === '/api/web/skills/868/reviews') { + await fulfill(route, { items: [], total: 0, page: 0, size: 20 }) + return + } + + if (pathname === '/api/web/skills/868/reviews/me') { + await fulfill(route, null) + return + } + + if (pathname === '/api/web/notifications/unread-count') { + await fulfill(route, { count: 0 }) + return + } + + await fulfill(route, []) + }) +} + +test.describe('Security audit redaction', () => { + test.beforeEach(async ({ page }) => { + await setEnglishLocale(page) + }) + + test('shows the precise SAFE verdict and only the masked finding snippet', async ({ page }) => { + await mockSkillDetail(page, [{ + id: 1, + scanId: 'scan-868', + scannerType: 'builtin', + verdict: 'SAFE', + isSafe: true, + maxSeverity: 'MEDIUM', + findingsCount: 1, + findings: [{ + ruleId: 'SECRET-001', + severity: 'MEDIUM', + category: 'secret', + title: 'Embedded credential', + message: 'A credential-like value was detected.', + filePath: 'scripts/deploy.ts', + lineNumber: 7, + codeSnippet: MASKED_SNIPPET, + remediation: 'Read credentials from the environment.', + analyzer: 'synthetic', + metadata: { originalSnippet: CANARY }, + }], + scanDurationSeconds: 1.2, + failureReason: null, + scannedAt: '2026-09-17T00:00:00Z', + createdAt: '2026-09-17T00:00:00Z', + }]) + + await page.goto('/space/global/redaction-skill') + + await expect(page.getByRole('heading', { name: 'Redaction Skill', exact: true }).first()).toBeVisible() + await expect(page.getByText('No high-risk findings', { exact: true })).toBeVisible() + await page.getByRole('button', { name: 'View Details' }).click() + await page.getByRole('button', { name: 'Findings' }).click() + + await expect(page.getByText(MASKED_SNIPPET, { exact: true })).toBeVisible() + await expect(page.locator('body')).not.toContainText(CANARY) + }) + + test('keeps the skill detail usable when the audit list is empty', async ({ page }) => { + await mockSkillDetail(page, []) + + await page.goto('/space/global/redaction-skill') + + await expect(page.getByRole('heading', { name: 'Redaction Skill', exact: true }).first()).toBeVisible() + await expect(page.getByText('Security Audit', { exact: true })).toHaveCount(0) + await expect(page.locator('body')).not.toContainText(CANARY) + }) +}) diff --git a/web/src/features/security-audit/finding-item.test.tsx b/web/src/features/security-audit/finding-item.test.tsx index a0d0c488..72177864 100644 --- a/web/src/features/security-audit/finding-item.test.tsx +++ b/web/src/features/security-audit/finding-item.test.tsx @@ -98,6 +98,20 @@ describe('FindingItem', () => { expect(html).toContain('SELECT * FROM users WHERE id = ${input}') }) + it('renders a masked snippet without exposing the original canary', () => { + const html = renderToStaticMarkup( + , + ) + + expect(html).toContain('const token = "[REDACTED]"') + expect(html).not.toContain('SYNTHETIC-CANARY-868') + }) + it('omits the code snippet when codeSnippet is null', () => { const finding = createFinding({ codeSnippet: null }) const html = renderToStaticMarkup() diff --git a/web/src/i18n/locales/en.json b/web/src/i18n/locales/en.json index dd1facca..d798fac3 100644 --- a/web/src/i18n/locales/en.json +++ b/web/src/i18n/locales/en.json @@ -1776,7 +1776,7 @@ "remediation": "Remediation", "viewDetails": "View Details", "verdict": { - "SAFE": "Safe", + "SAFE": "No high-risk findings", "SUSPICIOUS": "Suspicious", "DANGEROUS": "Dangerous", "BLOCKED": "High Risk" diff --git a/web/src/i18n/locales/ru.json b/web/src/i18n/locales/ru.json index c7a1c8c9..2785424d 100644 --- a/web/src/i18n/locales/ru.json +++ b/web/src/i18n/locales/ru.json @@ -1803,7 +1803,7 @@ "remediation": "Рекомендации", "viewDetails": "Подробности", "verdict": { - "SAFE": "Безопасно", + "SAFE": "Угроз высокого риска не обнаружено", "SUSPICIOUS": "Подозрительно", "DANGEROUS": "Опасно", "BLOCKED": "Высокий риск" diff --git a/web/src/i18n/locales/zh.json b/web/src/i18n/locales/zh.json index 681e3545..53df4ad9 100644 --- a/web/src/i18n/locales/zh.json +++ b/web/src/i18n/locales/zh.json @@ -1775,7 +1775,7 @@ "remediation": "修复建议", "viewDetails": "查看详情", "verdict": { - "SAFE": "安全", + "SAFE": "未发现高风险问题", "SUSPICIOUS": "可疑", "DANGEROUS": "危险", "BLOCKED": "高风险" diff --git a/web/src/i18n/security-audit-locale.test.ts b/web/src/i18n/security-audit-locale.test.ts index 3cb5e5f6..0ce69811 100644 --- a/web/src/i18n/security-audit-locale.test.ts +++ b/web/src/i18n/security-audit-locale.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from 'vitest' import en from './locales/en.json' import zh from './locales/zh.json' +import ru from './locales/ru.json' describe('security audit locales', () => { it('defines the scanning label in both locales', () => { @@ -12,4 +13,19 @@ describe('security audit locales', () => { expect(zh.securityAudit.verdict.BLOCKED).toBe('高风险') expect(en.securityAudit.verdict.BLOCKED).toBe('High Risk') }) + + it('uses the precise safe verdict wording in all locales', () => { + expect(en.securityAudit.verdict.SAFE).toBe('No high-risk findings') + expect(zh.securityAudit.verdict.SAFE).toBe('未发现高风险问题') + expect(ru.securityAudit.verdict.SAFE).toBe('Угроз высокого риска не обнаружено') + }) + + it('keeps verdict keys in parity across all locales', () => { + expect(Object.keys(en.securityAudit.verdict).sort()).toEqual( + Object.keys(zh.securityAudit.verdict).sort(), + ) + expect(Object.keys(en.securityAudit.verdict).sort()).toEqual( + Object.keys(ru.securityAudit.verdict).sort(), + ) + }) }) From 475c49702cad27a68a1109b415357d848be92b0d Mon Sep 17 00:00:00 2001 From: dongmucat <1127093059@qq.com> Date: Fri, 18 Sep 2026 10:39:55 +0800 Subject: [PATCH 3/9] fix(scanner): align deployment health checks and upload limits Signed-off-by: dongmucat <1127093059@qq.com> --- .github/workflows/pr-scanner-image.yml | 37 +++++++++++++++++++ .github/workflows/pr-scripts.yml | 11 ++++++ .../templates/scanner-deployment.yaml | 2 + charts/skillhub/values.schema.json | 3 +- charts/skillhub/values.yaml | 1 + compose.release.yml | 3 +- deploy/k8s/base/configmap.yaml | 1 + deploy/k8s/base/scanner-deployment.yaml | 5 +++ docker-compose.yml | 3 +- 9 files changed, 63 insertions(+), 3 deletions(-) create mode 100644 .github/workflows/pr-scanner-image.yml diff --git a/.github/workflows/pr-scanner-image.yml b/.github/workflows/pr-scanner-image.yml new file mode 100644 index 00000000..35cf5b65 --- /dev/null +++ b/.github/workflows/pr-scanner-image.yml @@ -0,0 +1,37 @@ +name: PR Scanner Image + +on: + pull_request: + paths: + - 'scanner/**' + - 'scripts/tests/scanner-2-1-contract-test.sh' + - '.github/workflows/pr-scanner-image.yml' + +permissions: + contents: read + +jobs: + scanner-contract: + name: Scanner contract (${{ matrix.arch }}) + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + arch: [amd64, arm64] + steps: + - uses: actions/checkout@v4 + with: + persist-credentials: false + - uses: docker/setup-qemu-action@v3 + - uses: docker/setup-buildx-action@v3 + - name: Build scanner image + uses: docker/build-push-action@v6 + with: + context: scanner + platforms: linux/${{ matrix.arch }} + tags: skillhub-scanner-contract:${{ matrix.arch }}-${{ github.sha }} + load: true + - name: Run scanner contract + env: + SCANNER_IMAGE: skillhub-scanner-contract:${{ matrix.arch }}-${{ github.sha }} + run: bash scripts/tests/scanner-2-1-contract-test.sh diff --git a/.github/workflows/pr-scripts.yml b/.github/workflows/pr-scripts.yml index 1ab4a2ee..e8180512 100644 --- a/.github/workflows/pr-scripts.yml +++ b/.github/workflows/pr-scripts.yml @@ -4,9 +4,16 @@ on: pull_request: paths: - 'scripts/**' + - 'scanner/**' + - 'docker-compose.yml' - '.env.release.example' - '.env.release.draft' - 'compose.release.yml' + - 'deploy/k8s/base/configmap.yaml' + - 'deploy/k8s/base/scanner-deployment.yaml' + - 'charts/skillhub/values.yaml' + - 'charts/skillhub/values.schema.json' + - 'charts/skillhub/templates/scanner-deployment.yaml' - 'server/Dockerfile' - 'Makefile' - 'web/Dockerfile' @@ -36,6 +43,10 @@ jobs: - uses: actions/setup-node@v4 with: node-version: '21' + - uses: actions/setup-python@v5 + with: + python-version: '3.11' + - run: python -m unittest discover -s scanner/tests -p 'test_*.py' - run: bash scripts/tests/publish-cli-test.sh - run: bash scripts/tests/runtime-secret-test.sh - run: bash scripts/tests/validate-release-config-test.sh diff --git a/charts/skillhub/templates/scanner-deployment.yaml b/charts/skillhub/templates/scanner-deployment.yaml index b511465e..dc15d73d 100644 --- a/charts/skillhub/templates/scanner-deployment.yaml +++ b/charts/skillhub/templates/scanner-deployment.yaml @@ -56,6 +56,8 @@ spec: {{- with .Values.scanner.extraEnv }} {{- toYaml . | nindent 12 }} {{- end }} + - name: SKILLHUB_SCANNER_MAX_UPLOAD_SIZE_BYTES + value: {{ .Values.scanner.maxUploadSizeBytes | quote }} resources: {{- toYaml .Values.scanner.resources | nindent 12 }} readinessProbe: diff --git a/charts/skillhub/values.schema.json b/charts/skillhub/values.schema.json index 1bede63b..942ee7c0 100644 --- a/charts/skillhub/values.schema.json +++ b/charts/skillhub/values.schema.json @@ -462,10 +462,11 @@ { "type": "object", "additionalProperties": false, - "required": ["enabled", "replicaCount", "image", "service", "resources", "extraEnv", "podAnnotations", "imagePullSecrets", "nodeSelector", "tolerations", "affinity", "probes", "autoscaling", "podDisruptionBudget"], + "required": ["enabled", "replicaCount", "maxUploadSizeBytes", "image", "service", "resources", "extraEnv", "podAnnotations", "imagePullSecrets", "nodeSelector", "tolerations", "affinity", "probes", "autoscaling", "podDisruptionBudget"], "properties": { "enabled": { "type": "boolean" }, "replicaCount": { "type": "integer", "minimum": 1 }, + "maxUploadSizeBytes": { "type": "integer", "minimum": 1 }, "image": { "$ref": "#/definitions/image" }, "service": { "type": "object", diff --git a/charts/skillhub/values.yaml b/charts/skillhub/values.yaml index 092e73e4..b69f7c86 100644 --- a/charts/skillhub/values.yaml +++ b/charts/skillhub/values.yaml @@ -433,6 +433,7 @@ web: scanner: enabled: true replicaCount: 1 + maxUploadSizeBytes: 110100480 image: registry: "" tag: "" diff --git a/compose.release.yml b/compose.release.yml index 317226b5..34361aca 100644 --- a/compose.release.yml +++ b/compose.release.yml @@ -8,8 +8,9 @@ services: SKILL_SCANNER_LLM_MODEL: ${SKILL_SCANNER_LLM_MODEL:-} SKILLHUB_SCANNER_MAX_CONCURRENT_SCANS: ${SKILLHUB_SCANNER_MAX_CONCURRENT_SCANS:-1} SKILLHUB_SCANNER_HARD_TIMEOUT_SECONDS: ${SKILLHUB_SCANNER_HARD_TIMEOUT_SECONDS:-930} + SKILLHUB_SCANNER_MAX_UPLOAD_SIZE_BYTES: ${SKILLHUB_SCANNER_MAX_UPLOAD_SIZE_BYTES:-110100480} healthcheck: - test: ["CMD", "wget", "-qO-", "http://127.0.0.1:8000/health"] + test: ["CMD", "python", "-c", "import urllib.request; urllib.request.urlopen('http://127.0.0.1:8000/health', timeout=2).read()"] interval: 10s timeout: 5s retries: 10 diff --git a/deploy/k8s/base/configmap.yaml b/deploy/k8s/base/configmap.yaml index 295da98a..d7daeb32 100644 --- a/deploy/k8s/base/configmap.yaml +++ b/deploy/k8s/base/configmap.yaml @@ -39,6 +39,7 @@ data: skill-scan-reclaim-min-idle: PT16M skill-scanner-max-concurrent-scans: "1" skill-scanner-hard-timeout-seconds: "930" + skill-scanner-max-upload-size-bytes: "110100480" # Bootstrap 管理员配置(非敏感) bootstrap-admin-enabled: "true" diff --git a/deploy/k8s/base/scanner-deployment.yaml b/deploy/k8s/base/scanner-deployment.yaml index 26fb3667..be27bd11 100644 --- a/deploy/k8s/base/scanner-deployment.yaml +++ b/deploy/k8s/base/scanner-deployment.yaml @@ -50,6 +50,11 @@ spec: configMapKeyRef: name: skillhub-config key: skill-scanner-hard-timeout-seconds + - name: SKILLHUB_SCANNER_MAX_UPLOAD_SIZE_BYTES + valueFrom: + configMapKeyRef: + name: skillhub-config + key: skill-scanner-max-upload-size-bytes readinessProbe: httpGet: path: /health diff --git a/docker-compose.yml b/docker-compose.yml index a43078e7..2f0e45df 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -7,8 +7,9 @@ services: SKILL_SCANNER_LLM_API_KEY: ${SKILL_SCANNER_LLM_API_KEY:-} SKILL_SCANNER_LLM_BASE_URL: ${SKILL_SCANNER_LLM_BASE_URL:-} SKILL_SCANNER_LLM_MODEL: ${SKILL_SCANNER_LLM_MODEL:-} + SKILLHUB_SCANNER_MAX_UPLOAD_SIZE_BYTES: ${SKILLHUB_SCANNER_MAX_UPLOAD_SIZE_BYTES:-110100480} healthcheck: - test: ["CMD", "wget", "-qO-", "http://127.0.0.1:8000/health"] + test: ["CMD", "python", "-c", "import urllib.request; urllib.request.urlopen('http://127.0.0.1:8000/health', timeout=2).read()"] interval: 10s timeout: 5s retries: 5 From 29f5c4cf86ec960168e96bc27fb59e988445086f Mon Sep 17 00:00:00 2001 From: dongmucat <1127093059@qq.com> Date: Fri, 18 Sep 2026 10:58:10 +0800 Subject: [PATCH 4/9] fix(scanner): harden scanner HTTP contract Signed-off-by: dongmucat <1127093059@qq.com> --- .../skillhub/config/SkillScannerConfig.java | 2 + .../portal/SecurityAuditControllerTest.java | 26 ++++++ .../security/SecurityScanServiceTest.java | 24 ++++++ .../skillhub/infra/http/HttpClient.java | 6 +- .../infra/http/HttpClientException.java | 10 ++- .../infra/http/WebClientHttpClient.java | 6 ++ .../skillhub/infra/scanner/ScanOptions.java | 28 ++++++- .../infra/scanner/SkillScannerService.java | 43 ++++++---- .../infra/http/WebClientHttpClientTest.java | 80 +++++++++++++++++++ .../infra/scanner/ScanOptionsTest.java | 33 ++++++++ .../scanner/SkillScannerAdapterTest.java | 5 ++ .../scanner/SkillScannerLoggingTest.java | 10 +++ .../scanner/SkillScannerServiceTest.java | 37 ++++++++- 13 files changed, 289 insertions(+), 21 deletions(-) create mode 100644 server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/http/WebClientHttpClientTest.java create mode 100644 server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/ScanOptionsTest.java diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/SkillScannerConfig.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/SkillScannerConfig.java index 9753e982..cbcbe7d0 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/SkillScannerConfig.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/SkillScannerConfig.java @@ -91,6 +91,8 @@ public class SkillScannerConfig { analyzers.isBehavioral(), analyzers.isLlm(), analyzers.getLlmProvider(), + analyzers.getLlmConsensusRuns(), + properties.getPolicy().getPreset(), analyzers.isMeta(), analyzers.isAiDefense(), analyzers.getAiDefenseApiKey(), diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/portal/SecurityAuditControllerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/portal/SecurityAuditControllerTest.java index e19be679..eb4de6b4 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/portal/SecurityAuditControllerTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/portal/SecurityAuditControllerTest.java @@ -40,7 +40,10 @@ import static org.springframework.security.test.web.servlet.request.SecurityMock import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; @SpringBootTest @AutoConfigureMockMvc @@ -128,6 +131,29 @@ class SecurityAuditControllerTest { .andExpect(jsonPath("$.data[0].findings[0].ruleId").value("STATIC-001")); } + @Test + void getSecurityAudit_doesNotExposeUnmaskedCanaryFromStoredFinding() throws Exception { + SecurityAudit audit = new SecurityAudit(42L, ScannerType.SKILL_SCANNER); + setField(audit, "id", 8L); + audit.setScanId("scan-masked"); + audit.setVerdict(SecurityVerdict.DANGEROUS); + audit.setIsSafe(false); + audit.setMaxSeverity("HIGH"); + audit.setFindings(""" + [{"ruleId":"TOKEN-001","severity":"HIGH","category":"secrets","title":"Token detected","message":"","filePath":"SKILL.md","lineNumber":4,"codeSnippet":"token="}] + """.trim()); + given(skillVersionRepository.findById(42L)).willReturn(java.util.Optional.of(skillVersion(42L, 8L))); + given(skillRepository.findById(8L)).willReturn(java.util.Optional.of(skill(8L, "reviewer-1"))); + given(securityAuditRepository.findLatestActiveByVersionId(42L)).willReturn(List.of(audit)); + + mockMvc.perform(get("/api/v1/skills/8/versions/42/security-audit") + .with(auth("reviewer-1")) + .requestAttr("userNsRoles", Map.of(5L, NamespaceRole.ADMIN))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.data[0].findings[0].message").value("")) + .andExpect(content().string(not(containsString("ghp_012345678901234567890123456789012345")))); + } + @Test void getSecurityAudit_returnsEmptyListWhenAuditMissing() throws Exception { given(skillVersionRepository.findById(42L)).willReturn(java.util.Optional.of(skillVersion(42L, 8L))); diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/security/SecurityScanServiceTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/security/SecurityScanServiceTest.java index 3292e499..eca72962 100644 --- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/security/SecurityScanServiceTest.java +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/security/SecurityScanServiceTest.java @@ -18,6 +18,7 @@ import org.springframework.transaction.support.TransactionSynchronizationManager import java.lang.reflect.Field; import java.nio.file.Path; import java.util.List; +import java.util.Map; import java.util.Optional; import static org.assertj.core.api.Assertions.assertThat; @@ -404,6 +405,29 @@ class SecurityScanServiceTest { verify(skillVersionRepository).save(version); } + @Test + void processScanResult_persistsScannerMaskedFindingWithoutOriginalCanary() { + SecurityAudit audit = new SecurityAudit(42L, ScannerType.SKILL_SCANNER, "task-masked"); + SkillVersion version = new SkillVersion(8L, "1.0.0", "publisher-1"); + version.setStatus(SkillVersionStatus.SCANNING); + + given(auditRepository.findByTaskId("task-masked")).willReturn(Optional.of(audit)); + given(auditRepository.findLatestActiveByVersionIdAndScannerType(42L, ScannerType.SKILL_SCANNER)) + .willReturn(Optional.of(audit)); + given(skillVersionRepository.findById(42L)).willReturn(Optional.of(version)); + + String canary = "ghp_012345678901234567890123456789012345"; + SecurityFinding maskedFinding = new SecurityFinding( + "TOKEN-001", "HIGH", "secrets", "Token detected", "", + "SKILL.md", 4, "token=", "Rotate token", "static", Map.of()); + service.processScanResult( + "task-masked", 42L, ScannerType.SKILL_SCANNER, + new SecurityScanResponse("scan-masked", SecurityVerdict.DANGEROUS, 1, "HIGH", + List.of(maskedFinding), 0.2)); + + assertThat(audit.getFindings()).contains("").doesNotContain(canary); + } + @Test void processScanResult_forStaleAttemptDoesNotCompleteCurrentAttempt() throws Exception { SecurityAudit stale = new SecurityAudit(42L, ScannerType.SKILL_SCANNER, "task-stale"); diff --git a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/http/HttpClient.java b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/http/HttpClient.java index f99ed121..fd4d74cb 100644 --- a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/http/HttpClient.java +++ b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/http/HttpClient.java @@ -7,7 +7,11 @@ public interface HttpClient { T get(String uri, Class responseType); - T post(String uri, Object body, Class responseType); + default T post(String uri, Object body, Class responseType) { + return post(uri, body, new HttpHeaders(), responseType); + } + + T post(String uri, Object body, HttpHeaders headers, Class responseType); T postMultipart(String uri, MultiValueMap parts, Class responseType); diff --git a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/http/HttpClientException.java b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/http/HttpClientException.java index 921bfe7d..1b26d078 100644 --- a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/http/HttpClientException.java +++ b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/http/HttpClientException.java @@ -6,9 +6,11 @@ public class HttpClientException extends RuntimeException { private final String responseBody; public HttpClientException(int statusCode, String responseBody) { - super("HTTP " + statusCode + ": " + responseBody); + super(responseBody == null || responseBody.isBlank() + ? "HTTP " + statusCode + : "HTTP " + statusCode + ": " + bounded(responseBody)); this.statusCode = statusCode; - this.responseBody = responseBody; + this.responseBody = responseBody == null ? null : bounded(responseBody); } public HttpClientException(String message, Throwable cause) { @@ -33,4 +35,8 @@ public class HttpClientException extends RuntimeException { String message = root.getMessage(); return root.getClass().getSimpleName() + (message == null || message.isBlank() ? "" : ": " + message); } + + private static String bounded(String body) { + return body.length() <= 2048 ? body : body.substring(0, 2048) + "...[truncated]"; + } } diff --git a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/http/WebClientHttpClient.java b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/http/WebClientHttpClient.java index 0181d168..e16b0ea9 100644 --- a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/http/WebClientHttpClient.java +++ b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/http/WebClientHttpClient.java @@ -39,10 +39,16 @@ public class WebClientHttpClient implements HttpClient { @Override public T post(String uri, Object body, Class responseType) { + return post(uri, body, new HttpHeaders(), responseType); + } + + @Override + public T post(String uri, Object body, HttpHeaders headers, Class responseType) { log.debug("POST {}", uri); try { return webClient.post() .uri(uri) + .headers(httpHeaders -> httpHeaders.addAll(headers)) .contentType(MediaType.APPLICATION_JSON) .bodyValue(body) .retrieve() diff --git a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/scanner/ScanOptions.java b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/scanner/ScanOptions.java index 4ae5a82d..1b96ff83 100644 --- a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/scanner/ScanOptions.java +++ b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/scanner/ScanOptions.java @@ -4,6 +4,8 @@ public record ScanOptions( boolean useBehavioral, boolean useLlm, String llmProvider, + int llmConsensusRuns, + String policyPreset, boolean enableMeta, boolean useAidefense, String aidefenseApiKey, @@ -11,7 +13,31 @@ public record ScanOptions( boolean useTrigger ) { + public ScanOptions { + if (llmConsensusRuns < 1) { + throw new IllegalArgumentException("llmConsensusRuns must be at least 1"); + } + if (!"strict".equals(policyPreset) + && !"balanced".equals(policyPreset) + && !"permissive".equals(policyPreset)) { + throw new IllegalArgumentException("policyPreset must be strict, balanced, or permissive"); + } + } + + /** Backward-compatible constructor for callers that do not configure the new scanner options. */ + public ScanOptions(boolean useBehavioral, + boolean useLlm, + String llmProvider, + boolean enableMeta, + boolean useAidefense, + String aidefenseApiKey, + boolean useVirusTotal, + boolean useTrigger) { + this(useBehavioral, useLlm, llmProvider, 1, "balanced", enableMeta, + useAidefense, aidefenseApiKey, useVirusTotal, useTrigger); + } + public static ScanOptions disabled() { - return new ScanOptions(false, false, "anthropic", false, false, "", false, false); + return new ScanOptions(false, false, "anthropic", 1, "balanced", false, false, "", false, false); } } diff --git a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/scanner/SkillScannerService.java b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/scanner/SkillScannerService.java index 6aab4eb3..d2e40c70 100644 --- a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/scanner/SkillScannerService.java +++ b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/scanner/SkillScannerService.java @@ -38,25 +38,26 @@ public class SkillScannerService { Map body = buildScanRequestBody(skillDirectory, options); try { - return httpClient.post(uri, body, SkillScannerApiResponse.class); + return httpClient.post(uri, body, buildScannerHeaders(options), SkillScannerApiResponse.class); } catch (HttpClientException e) { - log.error("Scanner API error: status={}, body={}", e.getStatusCode(), summarizeResponseBody(e.getResponseBody())); - throw e; + log.error("Scanner API error: status={}, operation=scanDirectory", e.getStatusCode()); + throw sanitizedException(e); } } public SkillScannerApiResponse scanUpload(Path skillPackagePath, ScanOptions options) { String uri = buildUploadUri(options); - log.info("Uploading skill package to scanner: {}", sanitizeUri(uri)); + log.info("Uploading skill package to scanner: {}", uri); MultiValueMap parts = new LinkedMultiValueMap<>(); parts.add("file", new FileSystemResource(skillPackagePath)); + addScannerOptionsParts(parts, options); HttpHeaders headers = buildScannerHeaders(options); try { return httpClient.postMultipart(uri, parts, headers, SkillScannerApiResponse.class); } catch (HttpClientException e) { - log.error("Scanner API error: status={}, body={}", e.getStatusCode(), summarizeResponseBody(e.getResponseBody())); - throw e; + log.error("Scanner API error: status={}, operation=scanUpload", e.getStatusCode()); + throw sanitizedException(e); } } @@ -70,6 +71,8 @@ public class SkillScannerService { body.put("use_behavioral", options.useBehavioral()); body.put("use_llm", options.useLlm()); body.put("llm_provider", options.llmProvider()); + body.put("llm_consensus_runs", options.llmConsensusRuns()); + body.put("policy", options.policyPreset()); body.put("enable_meta", options.enableMeta()); body.put("use_aidefense", options.useAidefense()); if (options.useAidefense() && !options.aidefenseApiKey().isEmpty()) { @@ -80,6 +83,18 @@ public class SkillScannerService { return body; } + private void addScannerOptionsParts(MultiValueMap parts, ScanOptions options) { + parts.add("use_behavioral", Boolean.toString(options.useBehavioral())); + parts.add("use_llm", Boolean.toString(options.useLlm())); + parts.add("llm_provider", options.llmProvider()); + parts.add("llm_consensus_runs", Integer.toString(options.llmConsensusRuns())); + parts.add("policy", options.policyPreset()); + parts.add("enable_meta", Boolean.toString(options.enableMeta())); + parts.add("use_aidefense", Boolean.toString(options.useAidefense())); + parts.add("use_virustotal", Boolean.toString(options.useVirusTotal())); + parts.add("use_trigger", Boolean.toString(options.useTrigger())); + } + private String buildUploadUri(ScanOptions options) { StringBuilder uri = new StringBuilder(baseUrl + scanPath); uri.append("?use_behavioral=").append(options.useBehavioral()); @@ -95,7 +110,7 @@ public class SkillScannerService { private HttpHeaders buildScannerHeaders(ScanOptions options) { HttpHeaders headers = new HttpHeaders(); if (options.useAidefense() && !options.aidefenseApiKey().isEmpty()) { - headers.add("X-AIDefense-Api-Key", options.aidefenseApiKey()); + headers.add("X-AIDefense-Key", options.aidefenseApiKey()); } return headers; } @@ -116,15 +131,11 @@ public class SkillScannerService { return normalized.endsWith("/") ? normalized.substring(0, normalized.length() - 1) : normalized; } - private String summarizeResponseBody(String body) { - if (body == null || body.isBlank()) { - return ""; + private HttpClientException sanitizedException(HttpClientException exception) { + if (exception.getStatusCode() > 0) { + return new HttpClientException(exception.getStatusCode(), null); } - String singleLine = body.replaceAll("\\s+", " ").trim(); - return singleLine.length() > 200 ? singleLine.substring(0, 200) + "...[truncated]" : singleLine; - } - - private String sanitizeUri(String uri) { - return uri.replaceAll("([?&]aidefense_api_key=)[^&]+", "$1***"); + Throwable cause = exception.getCause() == null ? exception : exception.getCause(); + return new HttpClientException("Scanner API request failed", cause); } } diff --git a/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/http/WebClientHttpClientTest.java b/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/http/WebClientHttpClientTest.java new file mode 100644 index 00000000..f85cf649 --- /dev/null +++ b/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/http/WebClientHttpClientTest.java @@ -0,0 +1,80 @@ +package com.iflytek.skillhub.infra.http; + +import com.sun.net.httpserver.HttpExchange; +import com.sun.net.httpserver.HttpServer; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.web.reactive.function.client.WebClient; + +import java.io.IOException; +import java.net.InetSocketAddress; +import java.nio.charset.StandardCharsets; +import java.util.Map; +import java.util.concurrent.atomic.AtomicReference; + +import static org.assertj.core.api.Assertions.assertThat; + +class WebClientHttpClientTest { + + private HttpServer server; + private AtomicReference requestBody; + private AtomicReference requestHeader; + + @BeforeEach + void setUp() throws IOException { + requestBody = new AtomicReference<>(); + requestHeader = new AtomicReference<>(); + server = HttpServer.create(new InetSocketAddress("localhost", 0), 0); + server.createContext("/json", this::handleJson); + server.start(); + } + + @AfterEach + void tearDown() { + server.stop(0); + } + + @Test + void postJson_sendsHeadersAndDecodesResponse() { + WebClientHttpClient client = new WebClientHttpClient(WebClient.builder().build()); + + @SuppressWarnings("unchecked") + Map response = (Map) client.post( + endpoint(), + Map.of("message", "hello"), + new org.springframework.http.HttpHeaders() {{ set("X-Test", "header-value"); }}, + Map.class + ); + + assertThat(response).containsEntry("ok", true); + assertThat(requestHeader.get()).isEqualTo("header-value"); + assertThat(requestBody.get()).contains("\"message\":\"hello\""); + } + + @Test + void postJson_withoutHeaders_remainsSupported() { + WebClientHttpClient client = new WebClientHttpClient(WebClient.builder().build()); + + @SuppressWarnings("unchecked") + Map response = (Map) client.post(endpoint(), Map.of("message", "legacy"), Map.class); + + assertThat(response).containsEntry("ok", true); + assertThat(requestHeader.get()).isNull(); + } + + private String endpoint() { + return "http://localhost:" + server.getAddress().getPort() + "/json"; + } + + private void handleJson(HttpExchange exchange) throws IOException { + requestBody.set(new String(exchange.getRequestBody().readAllBytes(), StandardCharsets.UTF_8)); + requestHeader.set(exchange.getRequestHeaders().getFirst("X-Test")); + byte[] response = "{\"ok\":true}".getBytes(StandardCharsets.UTF_8); + exchange.getResponseHeaders().set("Content-Type", "application/json"); + exchange.sendResponseHeaders(200, response.length); + try (var output = exchange.getResponseBody()) { + output.write(response); + } + } +} diff --git a/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/ScanOptionsTest.java b/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/ScanOptionsTest.java new file mode 100644 index 00000000..3669ffed --- /dev/null +++ b/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/ScanOptionsTest.java @@ -0,0 +1,33 @@ +package com.iflytek.skillhub.infra.scanner; + +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +class ScanOptionsTest { + + @Test + void disabled_usesSafeConsensusAndBalancedPolicyDefaults() { + ScanOptions options = ScanOptions.disabled(); + + assertThat(options.llmConsensusRuns()).isEqualTo(1); + assertThat(options.policyPreset()).isEqualTo("balanced"); + } + + @Test + void rejectsInvalidConsensusRuns() { + assertThatThrownBy(() -> new ScanOptions( + false, false, "anthropic", 0, "balanced", false, false, "", false, false)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("llmConsensusRuns"); + } + + @Test + void rejectsUnknownPolicyPreset() { + assertThatThrownBy(() -> new ScanOptions( + false, false, "anthropic", 1, "custom", false, false, "", false, false)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("policyPreset"); + } +} diff --git a/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/SkillScannerAdapterTest.java b/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/SkillScannerAdapterTest.java index 29c62316..2de33918 100644 --- a/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/SkillScannerAdapterTest.java +++ b/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/SkillScannerAdapterTest.java @@ -168,6 +168,11 @@ class SkillScannerAdapterTest { throw new UnsupportedOperationException(); } + @Override + public T post(String uri, Object body, HttpHeaders headers, Class responseType) { + throw new UnsupportedOperationException(); + } + @Override public T postMultipart(String uri, org.springframework.util.MultiValueMap parts, Class responseType) { diff --git a/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/SkillScannerLoggingTest.java b/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/SkillScannerLoggingTest.java index 008ece0d..8fc29a41 100644 --- a/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/SkillScannerLoggingTest.java +++ b/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/SkillScannerLoggingTest.java @@ -122,6 +122,11 @@ class SkillScannerLoggingTest { throw new UnsupportedOperationException(); } + @Override + public T post(String uri, Object body, HttpHeaders headers, Class responseType) { + throw new UnsupportedOperationException(); + } + @Override public T postMultipart(String uri, MultiValueMap parts, Class responseType) { throw new UnsupportedOperationException(); @@ -165,6 +170,11 @@ class SkillScannerLoggingTest { throw new UnsupportedOperationException(); } + @Override + public T post(String uri, Object body, HttpHeaders headers, Class responseType) { + throw new UnsupportedOperationException(); + } + @Override public T postMultipart(String uri, MultiValueMap parts, Class responseType) { throw new UnsupportedOperationException(); diff --git a/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/SkillScannerServiceTest.java b/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/SkillScannerServiceTest.java index 276a9193..0d3fb68c 100644 --- a/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/SkillScannerServiceTest.java +++ b/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/SkillScannerServiceTest.java @@ -63,6 +63,8 @@ class SkillScannerServiceTest { assertThat(body.get("skill_directory")).isEqualTo("/tmp/demo"); assertThat(body.get("use_behavioral")).isEqualTo(true); assertThat(body.get("use_llm")).isEqualTo(false); + assertThat(body.get("llm_consensus_runs")).isEqualTo(1); + assertThat(body.get("policy")).isEqualTo("balanced"); } @Test @@ -94,6 +96,8 @@ class SkillScannerServiceTest { assertThat(httpClient.lastMultipartUri).contains("use_llm=true"); assertThat(httpClient.lastMultipartUri).contains("llm_provider=openai"); assertThat(httpClient.lastMultipartParts.getFirst("file")).isNotNull(); + assertThat(httpClient.lastMultipartParts.getFirst("llm_consensus_runs")).isEqualTo("1"); + assertThat(httpClient.lastMultipartParts.getFirst("policy")).isEqualTo("balanced"); } @Test @@ -120,7 +124,28 @@ class SkillScannerServiceTest { service.scanUpload(Path.of("/tmp/demo.zip"), options); assertThat(httpClient.lastMultipartUri).doesNotContain("aidefense_api_key"); - assertThat(httpClient.lastMultipartHeaders.getFirst("X-AIDefense-Api-Key")).isEqualTo("secret-key"); + assertThat(httpClient.lastMultipartHeaders.getFirst("X-AIDefense-Key")).isEqualTo("secret-key"); + } + + @Test + void scanDirectory_keepsLegacyAidefenseBodyFieldAndSendsHeader() { + FakeHttpClient httpClient = new FakeHttpClient(); + httpClient.postResponse = new SkillScannerApiResponse( + "scan-4", "test-skill", true, "LOW", 0, null, 0.5, "2026-03-22T07:00:00"); + SkillScannerService service = new SkillScannerService( + httpClient, "http://scanner.test", "/scan-upload", "/health"); + ScanOptions options = new ScanOptions(false, false, "anthropic", 3, "strict", + false, true, "secret-key", false, false); + + service.scanDirectory("/tmp/demo", options); + + @SuppressWarnings("unchecked") + Map body = (Map) httpClient.lastPostBody; + assertThat(body.get("aidefense_api_key")).isEqualTo("secret-key"); + assertThat(httpClient.lastPostHeaders.getFirst("X-AIDefense-Key")).isEqualTo("secret-key"); + assertThat(httpClient.lastPostUri).doesNotContain("secret-key"); + assertThat(body.get("llm_consensus_runs")).isEqualTo(3); + assertThat(body.get("policy")).isEqualTo("strict"); } @Test @@ -145,6 +170,7 @@ class SkillScannerServiceTest { private Object multipartResponse; private String lastPostUri; private Object lastPostBody; + private HttpHeaders lastPostHeaders; private String lastMultipartUri; private MultiValueMap lastMultipartParts; private HttpHeaders lastMultipartHeaders; @@ -164,6 +190,15 @@ class SkillScannerServiceTest { return (T) postResponse; } + @Override + @SuppressWarnings("unchecked") + public T post(String uri, Object body, HttpHeaders headers, Class responseType) { + this.lastPostUri = uri; + this.lastPostBody = body; + this.lastPostHeaders = headers; + return (T) postResponse; + } + @Override @SuppressWarnings("unchecked") public T postMultipart(String uri, MultiValueMap parts, Class responseType) { From 8964838f438ba6308a077846061b7866b0008533 Mon Sep 17 00:00:00 2001 From: dongmucat <1127093059@qq.com> Date: Fri, 18 Sep 2026 10:59:50 +0800 Subject: [PATCH 5/9] fix(scanner): enforce upload size floor Signed-off-by: dongmucat <1127093059@qq.com> --- charts/skillhub/values.schema.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/charts/skillhub/values.schema.json b/charts/skillhub/values.schema.json index 942ee7c0..c30381b7 100644 --- a/charts/skillhub/values.schema.json +++ b/charts/skillhub/values.schema.json @@ -466,7 +466,7 @@ "properties": { "enabled": { "type": "boolean" }, "replicaCount": { "type": "integer", "minimum": 1 }, - "maxUploadSizeBytes": { "type": "integer", "minimum": 1 }, + "maxUploadSizeBytes": { "type": "integer", "minimum": 110100480 }, "image": { "$ref": "#/definitions/image" }, "service": { "type": "object", From 22839d06c7fd2bad3fd51948afb5ac6e8a067b40 Mon Sep 17 00:00:00 2001 From: dongmucat <1127093059@qq.com> Date: Fri, 18 Sep 2026 11:06:09 +0800 Subject: [PATCH 6/9] test(security): tolerate cold route startup Signed-off-by: dongmucat <1127093059@qq.com> --- web/e2e/security-audit-redaction.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/web/e2e/security-audit-redaction.spec.ts b/web/e2e/security-audit-redaction.spec.ts index 5ac14092..9cb1dfe9 100644 --- a/web/e2e/security-audit-redaction.spec.ts +++ b/web/e2e/security-audit-redaction.spec.ts @@ -152,7 +152,7 @@ test.describe('Security audit redaction', () => { await page.goto('/space/global/redaction-skill') - await expect(page.getByRole('heading', { name: 'Redaction Skill', exact: true }).first()).toBeVisible() + await expect(page.getByRole('heading', { name: 'Redaction Skill', exact: true }).first()).toBeVisible({ timeout: 15_000 }) await expect(page.getByText('No high-risk findings', { exact: true })).toBeVisible() await page.getByRole('button', { name: 'View Details' }).click() await page.getByRole('button', { name: 'Findings' }).click() @@ -166,7 +166,7 @@ test.describe('Security audit redaction', () => { await page.goto('/space/global/redaction-skill') - await expect(page.getByRole('heading', { name: 'Redaction Skill', exact: true }).first()).toBeVisible() + await expect(page.getByRole('heading', { name: 'Redaction Skill', exact: true }).first()).toBeVisible({ timeout: 15_000 }) await expect(page.getByText('Security Audit', { exact: true })).toHaveCount(0) await expect(page.locator('body')).not.toContainText(CANARY) }) From 78bfe10c91b1c25391f5ec4a064ffe2a638332d4 Mon Sep 17 00:00:00 2001 From: dongmucat <1127093059@qq.com> Date: Fri, 18 Sep 2026 11:12:59 +0800 Subject: [PATCH 7/9] docs(security): update scanner 2.1 operations guidance Signed-off-by: dongmucat <1127093059@qq.com> --- docs/security-scanning.md | 35 ++++++++++++++++++++++++++++++++--- docs/skillhub/en/faq.md | 18 +++++++++++++++++- docs/skillhub/faq.md | 18 +++++++++++++++++- 3 files changed, 66 insertions(+), 5 deletions(-) diff --git a/docs/security-scanning.md b/docs/security-scanning.md index e5e09d5d..b659f81a 100644 --- a/docs/security-scanning.md +++ b/docs/security-scanning.md @@ -1,8 +1,14 @@ # Skill Scanner Backend Runtime Guide +> **Document status:** This is a historical backend implementation note, updated with the +> current Scanner 2.1.0 runtime and rollout constraints. It does not expand the supported +> analyzer or deployment contract. + ## Overview SkillHub now supports a backend-only security scanning chain around `skill-scanner`. +The Scanner image pins `cisco-ai-skill-scanner==2.1.0` and uses a glibc-based Linux runtime. +Published images support `linux/amd64` and `linux/arm64`. The publish flow changes are: 1. publish request enters `SkillPublishService` @@ -20,14 +26,17 @@ Frontend is intentionally out of scope here. The frontend should fetch audit det Two runtime modes are supported: - `local` - Use `POST /scan` and pass a filesystem path. This only works when SkillHub and `skill-scanner` can see the same files. + Use `POST /scan` and pass a filesystem path. SkillHub and `skill-scanner` must mount the same + directory at the same path. The Scanner must also allow that root; for the standard path, set + `SKILL_SCANNER_ALLOWED_ROOTS=/tmp/skillhub-scans`. - `upload` - Use `POST /scan-upload` and upload the package archive. This is the safer default for split deployments. + Use `POST /scan-upload` and upload the package archive. This is the mode used by the official + Compose and Kubernetes deployments. Recommended usage: - local development with shared filesystem: `local` -- Kubernetes or any split-service deployment: `upload` +- official Compose, Kubernetes, or any split-service deployment: `upload` ## Backend Configuration @@ -70,6 +79,7 @@ Scanner-side optional environment variables: - `SKILL_SCANNER_LLM_MODEL` - `SKILLHUB_SCANNER_MAX_CONCURRENT_SCANS` (default `1`) - `SKILLHUB_SCANNER_HARD_TIMEOUT_SECONDS` (default `930`) +- `SKILLHUB_SCANNER_MAX_UPLOAD_SIZE_BYTES` (default `110100480`, or 105 MiB) If the LLM variables are absent, the scanner should still run with non-LLM analyzers. The default timeout ordering is server read timeout (900 seconds), scanner hard timeout @@ -95,6 +105,21 @@ Relevant manifests: The scanner service is internal-only by default and is consumed by the backend through cluster DNS. +## Rolling Upgrade to Scanner 2.1.0 + +The Server and Scanner HTTP contracts must be upgraded in this order: + +1. deploy the compatibility Server release while the old Scanner is still running +2. drain and remove every old Server instance, including in-flight scan requests +3. upgrade the Scanner to 2.1.0 +4. verify `/health` and an upload-mode scan before restoring normal traffic + +Do not run an old Server against Scanner 2.1.0. During a mixed-version rollout in `upload` mode, +keep AI Defense disabled (`SKILLHUB_SCANNER_USE_AI_DEFENSE=false`, the default). If AI Defense must +remain enabled before the old Scanner is retired, configure its credential directly in the old +Scanner environment using the variable supported by that Scanner version. Never place an AI Defense +key in URL query parameters. + ## Verification Verify the scanner service itself: @@ -133,6 +158,10 @@ Response fields include: - `scannedAt` - `createdAt` +`isSafe: true` means the scan found no high-risk issue. It does not mean that the scan produced no +findings: lower-severity findings may still be present and `findingsCount` may be non-zero. The UI +therefore renders this state as **No high-risk findings**, not as an unconditional safety guarantee. + ## Failure Semantics - scan task retries are handled by `AbstractStreamConsumer` diff --git a/docs/skillhub/en/faq.md b/docs/skillhub/en/faq.md index acfe6d89..ecae1d0d 100644 --- a/docs/skillhub/en/faq.md +++ b/docs/skillhub/en/faq.md @@ -200,7 +200,23 @@ A: SkillHub has built-in security scanning. The scanner integration, task orches ## Q: Which version of cisco-ai-skill-scanner does SkillHub use? -A: `scanner/Dockerfile` runs `pip install cisco-ai-skill-scanner` directly without pinning a version, so the latest version on PyPI is pulled when the image is built. To pin a version, do so yourself when customizing the build. +A: `scanner/Dockerfile` pins `cisco-ai-skill-scanner==2.1.0`. The Scanner image uses glibc Linux and supports `linux/amd64` and `linux/arm64`. + +## Q: Should the Scanner use upload mode or local mode? + +A: The official Compose and Kubernetes deployments use `upload` mode and send skill packages through `POST /scan-upload`. `SKILLHUB_SCANNER_MAX_UPLOAD_SIZE_BYTES` controls the upload limit; its default is `110100480` bytes (105 MiB). + +Use `local` mode only when the Server and Scanner can see the same directory at the **same path**. Both services must share the mount, and the Scanner must allow that root; for the standard path, set `SKILL_SCANNER_ALLOWED_ROOTS=/tmp/skillhub-scans`. + +## Q: Does “No high-risk findings” mean that a security scan returned no findings? + +A: No. A Scanner response with `is_safe=true`, rendered in the UI as “No high-risk findings,” only means that no high-risk issue was found. Lower-severity findings may still exist and `findingsCount` may be greater than zero. Review the finding details instead of treating this state as an unconditional safety guarantee. + +## Q: How should I roll out the Scanner 2.1.0 upgrade? + +A: First deploy the Server release that is compatible with the 2.1.0 protocol. While the old Scanner is still running, drain and remove every old Server instance and its in-flight scans. Then upgrade the Scanner and verify `/health` plus one upload-mode scan. Do not connect an old Server to Scanner 2.1.0. + +During the mixed-version window, keep AI Defense disabled in upload mode (the default is `SKILLHUB_SCANNER_USE_AI_DEFENSE=false`). If AI Defense must remain enabled before the upgrade, configure its credential directly in the old Scanner environment using the variable supported by that Scanner version; never put an AI Defense key in URL query parameters. ## Q: How do I troubleshoot a `registry returned 400` error from `skillhub publish` (CLI)? diff --git a/docs/skillhub/faq.md b/docs/skillhub/faq.md index 8906150c..386cec1d 100644 --- a/docs/skillhub/faq.md +++ b/docs/skillhub/faq.md @@ -200,7 +200,23 @@ A: SkillHub 内置安全扫描能力。其中扫描接入、任务编排、审 ## Q: SkillHub 使用的 cisco-ai-skill-scanner 是哪个版本? -A: `scanner/Dockerfile` 中直接执行 `pip install cisco-ai-skill-scanner`,未锁定版本,因此构建镜像时会拉取 PyPI 上的最新版本。如需固定版本,可在二次开发时自行锁定。 +A: `scanner/Dockerfile` 已固定使用 `cisco-ai-skill-scanner==2.1.0`。Scanner 镜像基于 glibc Linux,支持 `linux/amd64` 和 `linux/arm64`。 + +## Q: Scanner 应该使用 upload mode 还是 local mode? + +A: 官方 Compose 和 Kubernetes 部署使用 `upload` mode,通过 `POST /scan-upload` 上传技能包。上传大小上限由 `SKILLHUB_SCANNER_MAX_UPLOAD_SIZE_BYTES` 控制,默认是 `110100480` 字节(105 MiB)。 + +`local` mode 仅适用于 Server 与 Scanner 能在**相同路径**看到同一目录的部署。双方需要共享挂载路径,并在 Scanner 中设置允许的根目录;使用标准路径时配置 `SKILL_SCANNER_ALLOWED_ROOTS=/tmp/skillhub-scans`。 + +## Q: 安全审计显示“未发现高风险问题”,是否代表没有任何 findings? + +A: 不是。Scanner 返回 `is_safe=true`、UI 显示“未发现高风险问题”,仅表示没有发现高风险问题;低等级 findings 仍可能存在,`findingsCount` 也可能大于 0。请继续查看 findings 明细,而不要把该状态理解为无条件安全保证。 + +## Q: 如何滚动升级到 Scanner 2.1.0? + +A: 必须先部署兼容 2.1.0 协议的 Server,在旧 Scanner 仍运行时排空并下线所有旧 Server 实例及其进行中的扫描,然后再升级 Scanner,最后验证 `/health` 和一次 upload mode 扫描。不要让旧 Server 连接 Scanner 2.1.0。 + +混合版本期间,upload mode 应保持 AI Defense 关闭(默认 `SKILLHUB_SCANNER_USE_AI_DEFENSE=false`)。如果升级前必须继续使用 AI Defense,应按旧 Scanner 版本支持的环境变量把凭据直接配置到旧 Scanner 环境中;不要把 AI Defense key 放入 URL query 参数。 ## Q: 使用 CLI `skillhub publish` 报错 `registry returned 400` 怎么排查? From aacf57487db55197949896d684dde83973d849a5 Mon Sep 17 00:00:00 2001 From: dongmucat <1127093059@qq.com> Date: Fri, 18 Sep 2026 16:13:39 +0800 Subject: [PATCH 8/9] fix(scanner): harden Scanner 2.1 integration Signed-off-by: dongmucat <1127093059@qq.com> --- docs/security-scanning.md | 4 +- docs/skillhub/en/faq.md | 2 +- docs/skillhub/faq.md | 2 +- scanner/skillhub_scanner_app.py | 57 +++++--- scanner/tests/test_skillhub_scanner_app.py | 127 +++++++++++++----- scripts/tests/scanner-2-1-contract-test.sh | 42 ++++++ .../skillhub/infra/scanner/ScanOptions.java | 8 ++ .../infra/scanner/SkillScannerService.java | 3 - .../infra/scanner/ScanOptionsTest.java | 16 +++ .../scanner/SkillScannerServiceTest.java | 4 +- 10 files changed, 200 insertions(+), 65 deletions(-) diff --git a/docs/security-scanning.md b/docs/security-scanning.md index b659f81a..536ce465 100644 --- a/docs/security-scanning.md +++ b/docs/security-scanning.md @@ -114,11 +114,11 @@ The Server and Scanner HTTP contracts must be upgraded in this order: 3. upgrade the Scanner to 2.1.0 4. verify `/health` and an upload-mode scan before restoring normal traffic -Do not run an old Server against Scanner 2.1.0. During a mixed-version rollout in `upload` mode, +Do not run an old Server against Scanner 2.1.0. During a mixed-version rollout in either scan mode, keep AI Defense disabled (`SKILLHUB_SCANNER_USE_AI_DEFENSE=false`, the default). If AI Defense must remain enabled before the old Scanner is retired, configure its credential directly in the old Scanner environment using the variable supported by that Scanner version. Never place an AI Defense -key in URL query parameters. +key in URL query parameters or request bodies. ## Verification diff --git a/docs/skillhub/en/faq.md b/docs/skillhub/en/faq.md index ecae1d0d..4a86d35a 100644 --- a/docs/skillhub/en/faq.md +++ b/docs/skillhub/en/faq.md @@ -216,7 +216,7 @@ A: No. A Scanner response with `is_safe=true`, rendered in the UI as “No high- A: First deploy the Server release that is compatible with the 2.1.0 protocol. While the old Scanner is still running, drain and remove every old Server instance and its in-flight scans. Then upgrade the Scanner and verify `/health` plus one upload-mode scan. Do not connect an old Server to Scanner 2.1.0. -During the mixed-version window, keep AI Defense disabled in upload mode (the default is `SKILLHUB_SCANNER_USE_AI_DEFENSE=false`). If AI Defense must remain enabled before the upgrade, configure its credential directly in the old Scanner environment using the variable supported by that Scanner version; never put an AI Defense key in URL query parameters. +During the mixed-version window, keep AI Defense disabled in both upload and local modes (the default is `SKILLHUB_SCANNER_USE_AI_DEFENSE=false`). If AI Defense must remain enabled before the upgrade, configure its credential directly in the old Scanner environment using the variable supported by that Scanner version; never put an AI Defense key in URL query parameters or request bodies. ## Q: How do I troubleshoot a `registry returned 400` error from `skillhub publish` (CLI)? diff --git a/docs/skillhub/faq.md b/docs/skillhub/faq.md index 386cec1d..af331756 100644 --- a/docs/skillhub/faq.md +++ b/docs/skillhub/faq.md @@ -216,7 +216,7 @@ A: 不是。Scanner 返回 `is_safe=true`、UI 显示“未发现高风险问题 A: 必须先部署兼容 2.1.0 协议的 Server,在旧 Scanner 仍运行时排空并下线所有旧 Server 实例及其进行中的扫描,然后再升级 Scanner,最后验证 `/health` 和一次 upload mode 扫描。不要让旧 Server 连接 Scanner 2.1.0。 -混合版本期间,upload mode 应保持 AI Defense 关闭(默认 `SKILLHUB_SCANNER_USE_AI_DEFENSE=false`)。如果升级前必须继续使用 AI Defense,应按旧 Scanner 版本支持的环境变量把凭据直接配置到旧 Scanner 环境中;不要把 AI Defense key 放入 URL query 参数。 +混合版本期间,upload 和 local mode 都应保持 AI Defense 关闭(默认 `SKILLHUB_SCANNER_USE_AI_DEFENSE=false`)。如果升级前必须继续使用 AI Defense,应按旧 Scanner 版本支持的环境变量把凭据直接配置到旧 Scanner 环境中;不要把 AI Defense key 放入 URL query 参数或请求体。 ## Q: 使用 CLI `skillhub publish` 报错 `registry returned 400` 怎么排查? diff --git a/scanner/skillhub_scanner_app.py b/scanner/skillhub_scanner_app.py index 2ddacab4..3c38e22b 100644 --- a/scanner/skillhub_scanner_app.py +++ b/scanner/skillhub_scanner_app.py @@ -3,7 +3,6 @@ import asyncio import logging import os -import re import shutil import tempfile from functools import wraps @@ -11,9 +10,31 @@ from importlib import import_module from pathlib import Path from typing import NoReturn + +_RUNTIME_TEMP_ROOT = Path( + os.getenv("SKILLHUB_SCANNER_RUNTIME_TEMP_ROOT", "/tmp/skillhub-scanner-runtime") +) + + +def _prepare_runtime_temp_root() -> None: + """Recreate the scanner-owned temp root before upstream allocates request directories.""" + if _RUNTIME_TEMP_ROOT.name != "skillhub-scanner-runtime" or _RUNTIME_TEMP_ROOT.is_symlink(): + raise RuntimeError("Scanner runtime temp root must be a non-symlink skillhub-scanner-runtime directory") + if _RUNTIME_TEMP_ROOT.exists(): + if not _RUNTIME_TEMP_ROOT.is_dir(): + raise RuntimeError("Scanner runtime temp root must be a directory") + shutil.rmtree(_RUNTIME_TEMP_ROOT) + _RUNTIME_TEMP_ROOT.mkdir(parents=True, mode=0o700) + _RUNTIME_TEMP_ROOT.chmod(0o700) + tempfile.tempdir = str(_RUNTIME_TEMP_ROOT) + + +_prepare_runtime_temp_root() + from fastapi import Request from fastapi.responses import JSONResponse from skill_scanner.api.api import app +from skill_scanner.cli import cli as _upstream_cli _upstream_router = import_module("skill_scanner.api.router") @@ -25,15 +46,27 @@ _upstream_router.MAX_UPLOAD_SIZE_BYTES = max( _active_scans = 0 _active_scans_guard = asyncio.Lock() _SCAN_PATHS = {"/scan", "/scan-upload"} -_SUPPORTED_TOKEN_PATTERN = re.compile( - r"\b(?:gh[pousr]_[A-Za-z0-9]{20,255}|sk-(?:proj-)?[A-Za-z0-9_-]{20,255})\b" -) _log = logging.getLogger(__name__) +def _redact_finding_text(message: str) -> str: + """Redact credentials without applying CLI-only truncation or control escaping.""" + redacted = _upstream_cli._STATUS_PRIVATE_KEY_RE.sub("", message) + for pattern in ( + _upstream_cli._STATUS_URL_USERINFO_RE, + _upstream_cli._STATUS_URL_TOKEN_USERINFO_RE, + _upstream_cli._STATUS_QUERY_SECRET_RE, + _upstream_cli._STATUS_BEARER_SECRET_RE, + _upstream_cli._STATUS_LABELED_SECRET_RE, + ): + redacted = pattern.sub(_upstream_cli._replace_status_secret, redacted) + redacted = _upstream_cli._STATUS_PROVIDER_SECRET_RE.sub("", redacted) + return _upstream_cli._STATUS_JWT_RE.sub("", redacted) + + def _redact_supported_tokens(value): if isinstance(value, str): - return _SUPPORTED_TOKEN_PATTERN.sub("", value) + return _redact_finding_text(value) if isinstance(value, list): return [_redact_supported_tokens(item) for item in value] if isinstance(value, dict): @@ -60,19 +93,6 @@ def _install_scan_response_redaction() -> None: route.dependant.call = redacting_endpoint -def _cleanup_stale_scan_directories(temp_root: Path | None = None) -> None: - """Remove incomplete upstream extraction directories left by a process restart.""" - root = temp_root or Path(tempfile.gettempdir()) - current_upload_root = Path(_upstream_router._API_UPLOAD_ROOT).resolve() - for candidate in root.glob("skill_scanner_*"): - if not candidate.is_dir() or candidate.resolve() == current_upload_root: - continue - try: - shutil.rmtree(candidate) - except OSError as error: - _log.warning("Could not remove stale scanner directory %s: %s", candidate, error) - - def _restart_after_hard_timeout(request_path: str) -> NoReturn: """Terminate the single-scan worker so the container runtime can recover it.""" _log.critical( @@ -95,7 +115,6 @@ async def _await_scan_until(scan_task: asyncio.Task, deadline: float, request_pa _install_scan_response_redaction() -app.router.add_event_handler("startup", _cleanup_stale_scan_directories) @app.middleware("http") diff --git a/scanner/tests/test_skillhub_scanner_app.py b/scanner/tests/test_skillhub_scanner_app.py index 7972ac2e..de9a0672 100644 --- a/scanner/tests/test_skillhub_scanner_app.py +++ b/scanner/tests/test_skillhub_scanner_app.py @@ -1,6 +1,8 @@ import asyncio import importlib.util import os +import re +import shutil import sys import tempfile import types @@ -23,6 +25,10 @@ class _FakeApp: self.routes = [object()] github_canary = "ghp_" + "A1b2C3d4E5f6G7h8I9j0K1l2M3n4O5p6Q7r8" openai_canary = "sk-proj-" + "Z9y8X7w6V5u4T3s2R1q0" * 3 + aws_canary = "AKIA1234567890ABCDEF" + jwt_canary = "eyJabcde.abcdefgh.ijklmnop" + labeled_canary = "custom-secret-1234567890" + private_key_canary = "-----BEGIN PRIVATE KEY-----\nabc123\n-----END PRIVATE KEY-----" self.upstream_routes = [ _FakeRoute( "/scan", @@ -30,7 +36,13 @@ class _FakeApp: [ { "description": f"YARA match: {github_canary}", - "metadata": {"evidence": openai_canary}, + "metadata": { + "openai": openai_canary, + "aws": aws_canary, + "authorization": f"Bearer {jwt_canary}", + "labeled": f"api_key={labeled_canary}", + "private_key": private_key_canary, + }, } ] ), @@ -85,8 +97,30 @@ def _load_module(environment=None): api.app = _FakeApp() router = types.ModuleType("skill_scanner.api.router") router.MAX_UPLOAD_SIZE_BYTES = -1 - router._API_UPLOAD_ROOT = Path(tempfile.gettempdir()) / "skill_scanner_current" router.router = types.SimpleNamespace(routes=api.app.upstream_routes) + cli = types.ModuleType("skill_scanner.cli.cli") + cli._STATUS_PRIVATE_KEY_RE = re.compile( + r"-----BEGIN PRIVATE KEY-----[\s\S]*?-----END PRIVATE KEY-----" + ) + cli._STATUS_URL_USERINFO_RE = re.compile(r"(?!)") + cli._STATUS_URL_TOKEN_USERINFO_RE = re.compile(r"(?!)") + cli._STATUS_QUERY_SECRET_RE = re.compile(r"(?!)") + cli._STATUS_BEARER_SECRET_RE = re.compile( + r"(?i)(?P\bBearer\s+)(?P[A-Za-z0-9._-]+)" + ) + cli._STATUS_LABELED_SECRET_RE = re.compile( + r"(?i)(?P\bapi_key=)(?P[^\s]+)" + ) + cli._STATUS_PROVIDER_SECRET_RE = re.compile( + r"\b(?:AKIA[0-9A-Z]{16}|ghp_[A-Za-z0-9]{20,255}|" + r"github_pat_[A-Za-z0-9_]{20,255}|sk-(?:proj-)?[A-Za-z0-9_-]{20,255})\b" + ) + cli._STATUS_JWT_RE = re.compile(r"\beyJ[A-Za-z0-9_-]{5,}\.[A-Za-z0-9_-]{5,}\.[A-Za-z0-9_-]{5,}\b") + + def replace_status_secret(match): + return f"{match.group('prefix')}{match.groupdict().get('suffix', '')}" + + cli._replace_status_secret = replace_status_secret stubs = { "fastapi": fastapi, "fastapi.responses": responses, @@ -94,13 +128,29 @@ def _load_module(environment=None): "skill_scanner.api": types.ModuleType("skill_scanner.api"), "skill_scanner.api.api": api, "skill_scanner.api.router": router, + "skill_scanner.cli": types.ModuleType("skill_scanner.cli"), + "skill_scanner.cli.cli": cli, } - with patch.dict(os.environ, environment or {}, clear=True), patch.dict(sys.modules, stubs): + test_temp_parent = Path(tempfile.mkdtemp(prefix="skillhub-scanner-wrapper-test-")) + runtime_temp_root = test_temp_parent / "skillhub-scanner-runtime" + runtime_temp_root.mkdir() + (runtime_temp_root / "stale-upload.zip").write_text("stale", encoding="utf-8") + (test_temp_parent / "outside.txt").write_text("keep", encoding="utf-8") + module_environment = { + "SKILLHUB_SCANNER_RUNTIME_TEMP_ROOT": str(runtime_temp_root), + **(environment or {}), + } + previous_tempdir = tempfile.tempdir + with patch.dict(os.environ, module_environment, clear=True), patch.dict(sys.modules, stubs): module_path = Path(__file__).parents[1] / "skillhub_scanner_app.py" spec = importlib.util.spec_from_file_location("skillhub_scanner_app_under_test", module_path) module = importlib.util.module_from_spec(spec) - spec.loader.exec_module(module) + try: + spec.loader.exec_module(module) + finally: + tempfile.tempdir = previous_tempdir module._router_stub = router + module._test_temp_parent = test_temp_parent return module @@ -108,6 +158,9 @@ class SkillHubScannerAppTest(unittest.IsolatedAsyncioTestCase): async def asyncSetUp(self): self.module = _load_module() + def tearDown(self): + shutil.rmtree(self.module._test_temp_parent) + async def test_excess_scan_is_rejected(self): self.module._active_scans = 1 @@ -153,53 +206,32 @@ class SkillHubScannerAppTest(unittest.IsolatedAsyncioTestCase): restart.assert_called_once_with("/scan-upload") self.assertEqual(0, self.module._active_scans) - async def test_startup_cleanup_removes_only_scanner_directories(self): - with tempfile.TemporaryDirectory() as temp_root: - root = Path(temp_root) - stale = root / "skill_scanner_abcd" - unrelated = root / "skillhub-data" - stale.mkdir() - unrelated.mkdir() - - self.module._cleanup_stale_scan_directories(root) - - self.assertFalse(stale.exists()) - self.assertTrue(unrelated.exists()) - - async def test_startup_cleanup_keeps_current_upstream_upload_directory(self): - with tempfile.TemporaryDirectory() as temp_root: - root = Path(temp_root) - current = root / "skill_scanner_current" - stale = root / "skill_scanner_stale" - current.mkdir() - stale.mkdir() - self.module._router_stub._API_UPLOAD_ROOT = current - - self.module._cleanup_stale_scan_directories(root) - - self.assertTrue(current.exists()) - self.assertFalse(stale.exists()) - async def test_default_upload_limit_matches_skillhub_package_limit(self): self.assertEqual(110100480, self.module._router_stub.MAX_UPLOAD_SIZE_BYTES) async def test_upload_limit_can_be_overridden_by_environment(self): module = _load_module({"SKILLHUB_SCANNER_MAX_UPLOAD_SIZE_BYTES": "123456"}) + self.addCleanup(shutil.rmtree, module._test_temp_parent) self.assertEqual(123456, module._router_stub.MAX_UPLOAD_SIZE_BYTES) async def test_upload_limit_is_at_least_one_byte(self): module = _load_module({"SKILLHUB_SCANNER_MAX_UPLOAD_SIZE_BYTES": "0"}) + self.addCleanup(shutil.rmtree, module._test_temp_parent) self.assertEqual(1, module._router_stub.MAX_UPLOAD_SIZE_BYTES) - async def test_scan_findings_redact_supported_tokens_without_changing_response_contract(self): + async def test_scan_findings_redact_credentials_without_changing_response_contract(self): route = next(route for route in self.module._router_stub.router.routes if route.path == "/scan") response = await route.endpoint() self.assertNotIn("ghp_", str(response.findings)) self.assertNotIn("sk-proj-", str(response.findings)) + self.assertNotIn("AKIA1234567890ABCDEF", str(response.findings)) + self.assertNotIn("eyJabcde.abcdefgh.ijklmnop", str(response.findings)) + self.assertNotIn("custom-secret-1234567890", str(response.findings)) + self.assertNotIn("BEGIN PRIVATE KEY", str(response.findings)) self.assertEqual(200, response.status_code) self.assertEqual({"X-Contract": "preserved"}, response.headers) @@ -211,11 +243,32 @@ class SkillHubScannerAppTest(unittest.IsolatedAsyncioTestCase): self.assertEqual(expected, response.findings) - async def test_startup_cleanup_is_registered_on_the_upstream_router(self): - self.assertEqual( - [("startup", self.module._cleanup_stale_scan_directories)], - self.module.app.router.handlers, - ) + async def test_redaction_preserves_safe_multiline_and_long_finding_text(self): + safe_multiline = "line one\n\tline two" + safe_long = "x" * 5000 + + redacted = self.module._redact_supported_tokens([safe_multiline, safe_long]) + + self.assertEqual([safe_multiline, safe_long], redacted) + + async def test_redaction_preserves_non_secret_control_characters_around_secret(self): + value = "before\napi_key=custom-secret-1234567890\tafter" + + redacted = self.module._redact_supported_tokens(value) + + self.assertEqual("before\napi_key=\tafter", redacted) + + async def test_startup_does_not_register_global_temp_directory_cleanup(self): + self.assertEqual([], self.module.app.router.handlers) + + async def test_import_recreates_private_runtime_temp_root_without_touching_parent(self): + runtime_root = self.module._RUNTIME_TEMP_ROOT + outside = self.module._test_temp_parent / "outside.txt" + + self.assertTrue(runtime_root.is_dir()) + self.assertEqual(0o700, runtime_root.stat().st_mode & 0o777) + self.assertFalse((runtime_root / "stale-upload.zip").exists()) + self.assertEqual("keep", outside.read_text(encoding="utf-8")) if __name__ == "__main__": diff --git a/scripts/tests/scanner-2-1-contract-test.sh b/scripts/tests/scanner-2-1-contract-test.sh index 3cf02eab..b2bb54ea 100755 --- a/scripts/tests/scanner-2-1-contract-test.sh +++ b/scripts/tests/scanner-2-1-contract-test.sh @@ -186,6 +186,48 @@ MAIN_PORT="$SCANNER_PORT" MAIN_URL="http://127.0.0.1:$MAIN_PORT" wait_for_health "$MAIN_URL" +docker exec -i "$MAIN_CONTAINER" python - <<'PY' +from skill_scanner.core.analyzers.llm_analyzer import LLMProvider +from skillhub_scanner_app import _redact_supported_tokens + +canaries = { + "aws": "AKIA1234567890ABCDEF", + "github": "github_pat_abcdefghijklmnopqrstuvwxyz", + "jwt": "eyJabcde.abcdefgh.ijklmnop", + "labeled": "custom-secret-1234567890", + "private_key": "-----BEGIN PRIVATE KEY-----\nabc123\n-----END PRIVATE KEY-----", +} +findings = { + "aws": canaries["aws"], + "github": canaries["github"], + "authorization": f"Bearer {canaries['jwt']}", + "labeled": f"api_key={canaries['labeled']}", + "private_key": canaries["private_key"], +} +redacted = str(_redact_supported_tokens(findings)) +for label, canary in canaries.items(): + if canary in redacted: + raise SystemExit(f"{label} canary was not redacted") + +safe_values = ["line one\n\tline two", "x" * 5000] +if _redact_supported_tokens(safe_values) != safe_values: + raise SystemExit("redaction changed safe multiline or long finding text") +mixed = "before\napi_key=custom-secret-1234567890\tafter" +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") +PY + +docker exec "$MAIN_CONTAINER" python -c \ + 'from pathlib import Path; Path("/tmp/skillhub-scanner-runtime/stale-after-timeout").write_text("stale")' +docker restart "$MAIN_CONTAINER" >/dev/null +MAIN_PORT="$(docker port "$MAIN_CONTAINER" 8000/tcp | awk -F: 'END {print $NF}')" +MAIN_URL="http://127.0.0.1:$MAIN_PORT" +wait_for_health "$MAIN_URL" +docker exec "$MAIN_CONTAINER" python -c \ + 'from pathlib import Path; assert not Path("/tmp/skillhub-scanner-runtime/stale-after-timeout").exists()' + post_zip "$MAIN_URL" "$TMP_DIR/safe.zip" "$TMP_DIR/safe.json" post_zip "$MAIN_URL" "$TMP_DIR/javascript-secret.zip" "$TMP_DIR/javascript-secret.json" post_zip "$MAIN_URL" "$TMP_DIR/dotenv-secret.zip" "$TMP_DIR/dotenv-secret.json" diff --git a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/scanner/ScanOptions.java b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/scanner/ScanOptions.java index 1b96ff83..a703e806 100644 --- a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/scanner/ScanOptions.java +++ b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/scanner/ScanOptions.java @@ -14,6 +14,14 @@ public record ScanOptions( ) { public ScanOptions { + if ("azure".equals(llmProvider)) { + llmProvider = "azure-openai"; + } + if (!"anthropic".equals(llmProvider) + && !"openai".equals(llmProvider) + && !"azure-openai".equals(llmProvider)) { + throw new IllegalArgumentException("llmProvider must be anthropic, openai, or azure"); + } if (llmConsensusRuns < 1) { throw new IllegalArgumentException("llmConsensusRuns must be at least 1"); } diff --git a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/scanner/SkillScannerService.java b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/scanner/SkillScannerService.java index d2e40c70..aa75a018 100644 --- a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/scanner/SkillScannerService.java +++ b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/scanner/SkillScannerService.java @@ -75,9 +75,6 @@ public class SkillScannerService { body.put("policy", options.policyPreset()); body.put("enable_meta", options.enableMeta()); body.put("use_aidefense", options.useAidefense()); - if (options.useAidefense() && !options.aidefenseApiKey().isEmpty()) { - body.put("aidefense_api_key", options.aidefenseApiKey()); - } body.put("use_virustotal", options.useVirusTotal()); body.put("use_trigger", options.useTrigger()); return body; diff --git a/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/ScanOptionsTest.java b/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/ScanOptionsTest.java index 3669ffed..4ab70800 100644 --- a/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/ScanOptionsTest.java +++ b/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/ScanOptionsTest.java @@ -30,4 +30,20 @@ class ScanOptionsTest { .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("policyPreset"); } + + @Test + void rejectsUnknownLlmProvider() { + assertThatThrownBy(() -> new ScanOptions( + false, false, "openai&unexpected=value", 1, "balanced", false, false, "", false, false)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("llmProvider"); + } + + @Test + void acceptsDocumentedAzureLlmProvider() { + ScanOptions options = new ScanOptions( + false, true, "azure", 1, "balanced", false, false, "", false, false); + + assertThat(options.llmProvider()).isEqualTo("azure-openai"); + } } diff --git a/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/SkillScannerServiceTest.java b/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/SkillScannerServiceTest.java index 0d3fb68c..9219abad 100644 --- a/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/SkillScannerServiceTest.java +++ b/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/SkillScannerServiceTest.java @@ -128,7 +128,7 @@ class SkillScannerServiceTest { } @Test - void scanDirectory_keepsLegacyAidefenseBodyFieldAndSendsHeader() { + void scanDirectory_sendsAidefenseApiKeyOnlyViaHeader() { FakeHttpClient httpClient = new FakeHttpClient(); httpClient.postResponse = new SkillScannerApiResponse( "scan-4", "test-skill", true, "LOW", 0, null, 0.5, "2026-03-22T07:00:00"); @@ -141,7 +141,7 @@ class SkillScannerServiceTest { @SuppressWarnings("unchecked") Map body = (Map) httpClient.lastPostBody; - assertThat(body.get("aidefense_api_key")).isEqualTo("secret-key"); + assertThat(body).doesNotContainKey("aidefense_api_key"); assertThat(httpClient.lastPostHeaders.getFirst("X-AIDefense-Key")).isEqualTo("secret-key"); assertThat(httpClient.lastPostUri).doesNotContain("secret-key"); assertThat(body.get("llm_consensus_runs")).isEqualTo(3); 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 9/9] 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 \