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 83b76ac5..ea43f1b1 100644 --- a/charts/skillhub/values.schema.json +++ b/charts/skillhub/values.schema.json @@ -496,10 +496,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": 110100480 }, "image": { "$ref": "#/definitions/image" }, "service": { "type": "object", diff --git a/charts/skillhub/values.yaml b/charts/skillhub/values.yaml index 5e38e21a..3ec62e94 100644 --- a/charts/skillhub/values.yaml +++ b/charts/skillhub/values.yaml @@ -450,6 +450,7 @@ web: scanner: enabled: true replicaCount: 1 + maxUploadSizeBytes: 110100480 image: registry: "" tag: "" diff --git a/compose.release.yml b/compose.release.yml index bb55a8f1..30214c51 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 diff --git a/docs/security-scanning.md b/docs/security-scanning.md index e5e09d5d..536ce465 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 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 or request bodies. + ## 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 4db5d8bb..054e6616 100644 --- a/docs/skillhub/en/faq.md +++ b/docs/skillhub/en/faq.md @@ -205,7 +205,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 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 e8a094d9..7aeb7080 100644 --- a/docs/skillhub/faq.md +++ b/docs/skillhub/faq.md @@ -208,7 +208,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 和 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/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..c008d17e 100644 --- a/scanner/skillhub_scanner_app.py +++ b/scanner/skillhub_scanner_app.py @@ -1,36 +1,153 @@ """Runtime safeguards around the upstream Cisco Skill Scanner ASGI application.""" import asyncio +import inspect import logging import os import shutil import tempfile +from functools import wraps +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") _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"} +_REDACTION_MARKER = "_skillhub_redaction_installed" _log = logging.getLogger(__name__) -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()) - for candidate in root.glob("skill_scanner_*"): - if not candidate.is_dir(): +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 _redact_finding_text(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 _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 - try: - shutil.rmtree(candidate) - except OSError as error: - _log.warning("Could not remove stale scanner directory %s: %s", candidate, error) + 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): + 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 + + +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: @@ -54,7 +171,7 @@ async def _await_scan_until(scan_task: asyncio.Task, deadline: float, request_pa _restart_after_hard_timeout(request_path) -app.router.add_event_handler("startup", _cleanup_stale_scan_directories) +_install_scan_response_redaction() @app.middleware("http") diff --git a/scanner/tests/test_skillhub_scanner_app.py b/scanner/tests/test_skillhub_scanner_app.py index fd58469a..2c341706 100644 --- a/scanner/tests/test_skillhub_scanner_app.py +++ b/scanner/tests/test_skillhub_scanner_app.py @@ -1,5 +1,8 @@ import asyncio import importlib.util +import os +import re +import shutil import sys import tempfile import types @@ -9,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)) @@ -19,6 +23,39 @@ class _FakeRouter: class _FakeApp: def __init__(self): self.router = _FakeRouter() + 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", + _FakeScanResponse( + [ + { + "description": f"YARA match: {github_canary}", + "metadata": { + "openai": openai_canary, + "aws": aws_canary, + "authorization": f"Bearer {jwt_canary}", + "labeled": f"api_key={labeled_canary}", + "private_key": private_key_canary, + }, + } + ] + ), + ), + _FakeRoute( + "/scan-upload", + _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 @@ -31,30 +68,110 @@ 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"} + self._response = response + + async def endpoint(): + return response + + 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" 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.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, "skill_scanner": types.ModuleType("skill_scanner"), "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(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 @@ -62,6 +179,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 @@ -107,24 +227,102 @@ 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() + async def test_default_upload_limit_matches_skillhub_package_limit(self): + self.assertEqual(110100480, self.module._router_stub.MAX_UPLOAD_SIZE_BYTES) - self.module._cleanup_stale_scan_directories(root) + 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.assertFalse(stale.exists()) - self.assertTrue(unrelated.exists()) + self.assertEqual(123456, module._router_stub.MAX_UPLOAD_SIZE_BYTES) - 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_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_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) + + 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}}] + + response = await route.dependant.call() + + self.assertEqual(expected, response.findings) + + 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 new file mode 100755 index 00000000..58d5a319 --- /dev/null +++ b/scripts/tests/scanner-2-1-contract-test.sh @@ -0,0 +1,384 @@ +#!/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" + +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 = { + "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") + + +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 \ + '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" +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" 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..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 @@ -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,39 @@ public record ScanOptions( boolean useTrigger ) { + 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"); + } + 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..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 @@ -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,16 +71,27 @@ 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()) { - body.put("aidefense_api_key", options.aidefenseApiKey()); - } body.put("use_virustotal", options.useVirusTotal()); body.put("use_trigger", options.useTrigger()); 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 +107,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 +128,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..4ab70800 --- /dev/null +++ b/server/skillhub-infra/src/test/java/com/iflytek/skillhub/infra/scanner/ScanOptionsTest.java @@ -0,0 +1,49 @@ +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"); + } + + @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/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..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 @@ -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_sendsAidefenseApiKeyOnlyViaHeader() { + 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).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); + 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) { diff --git a/web/e2e/security-audit-redaction.spec.ts b/web/e2e/security-audit-redaction.spec.ts new file mode 100644 index 00000000..9cb1dfe9 --- /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({ 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() + + 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({ timeout: 15_000 }) + 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 4c25114a..c506f0be 100644 --- a/web/src/i18n/locales/en.json +++ b/web/src/i18n/locales/en.json @@ -1835,7 +1835,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 fc0688d7..b63ec012 100644 --- a/web/src/i18n/locales/ru.json +++ b/web/src/i18n/locales/ru.json @@ -1862,7 +1862,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 f900f983..92b6fb79 100644 --- a/web/src/i18n/locales/zh.json +++ b/web/src/i18n/locales/zh.json @@ -1834,7 +1834,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(), + ) + }) })