diff --git a/.env.example b/.env.example index 24ad4f51..5e90ec6f 100644 --- a/.env.example +++ b/.env.example @@ -2,7 +2,7 @@ ENV=dev # options: dev|s # Active LLM provider. Selects which provider answers credentials, # metadata, and default-model lookups. Leave unset to default to nv_build. -# Options: openai | anthropic | nv_build +# Options: openai | anthropic | anthropic_proxy | nv_build SKILLSPECTOR_PROVIDER= # Provider credentials — set the one matching SKILLSPECTOR_PROVIDER (or @@ -21,6 +21,13 @@ OPENAI_BASE_URL= # For SKILLSPECTOR_PROVIDER=anthropic. ANTHROPIC_API_KEY= +# For SKILLSPECTOR_PROVIDER=anthropic_proxy (Vertex-style raw-predict proxy). +# Supports corporate API gateways, GCP Vertex AI, and self-hosted proxies. +ANTHROPIC_PROXY_ENDPOINT_URL= +ANTHROPIC_PROXY_API_KEY= +# ANTHROPIC_PROXY_API_VERSION=vertex-2023-10-16 # optional; defaults to vertex-2023-10-16 +# SKILLSPECTOR_SSL_VERIFY=false # set to false for internal/self-signed CAs + # SkillSpector config SKILLSPECTOR_MODEL= # leave empty to use the active provider's bundled default (see README); set to override (e.g. gpt-5.2) # SKILLSPECTOR_MODEL_REGISTRY=./model_registry.yaml # optional override; defaults to each provider's bundled YAML in src/skillspector/providers/ diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 00000000..816970cd --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,96 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +name: CI + +on: + pull_request: + branches: ["main"] + push: + branches: ["main"] + +# Least privilege: these jobs only read the repo; no write scopes are needed. +permissions: + contents: read + +# Cancel superseded runs when new commits are pushed to the same ref. +concurrency: + group: ci-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + lint-and-test: + name: Lint & Test (Python ${{ matrix.python-version }}) + runs-on: ubuntu-latest + # Windows is excluded: the test suite has known path-separator failures + # in build_context that are out of scope for this workflow. + strategy: + fail-fast: false + matrix: + python-version: ["3.12", "3.13", "3.14"] + + steps: + - uses: actions/checkout@v4 + + - name: Set up uv + # Pinned to a full commit SHA (third-party action); comment tracks the tag. + uses: astral-sh/setup-uv@d4b2f3b6ecc6e67c4457f6d3e41ec42d3d0fcb86 # v5 + with: + enable-cache: true + python-version: ${{ matrix.python-version }} + + - name: Install dependencies + run: uv sync --all-extras + + - name: Lint with ruff + run: uv run ruff check src/ tests/ + + - name: Check formatting with ruff + run: uv run ruff format --check src/ tests/ + + - name: Run unit tests with coverage + run: uv run pytest -m "not integration" --cov=src/skillspector --cov-report=term-missing + + dco: + name: DCO Check + runs-on: ubuntu-latest + if: github.event_name == 'pull_request' + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Verify DCO sign-off on all commits + run: | + BASE=${{ github.event.pull_request.base.sha }} + HEAD=${{ github.event.pull_request.head.sha }} + # Iterate SHAs directly rather than piping `git log` into `while read`: + # `git log` does not print a trailing newline after the final record, + # so a read-loop silently skips the last commit — and for a one-commit + # PR (the common case) the body never runs at all, letting an unsigned + # commit pass. A for-loop over the SHA list checks every commit. + status=0 + for sha in $(git log --format=%H "${BASE}..${HEAD}"); do + if ! git log -1 --format="%B" "$sha" | grep -q "^Signed-off-by:"; then + echo " missing Signed-off-by: $sha $(git log -1 --format=%s "$sha")" + status=1 + fi + done + if [ "$status" -ne 0 ]; then + echo "" + echo "Please add a DCO sign-off (git commit -s) to all commits." + exit 1 + fi + echo "All commits have DCO sign-off." diff --git a/.github/workflows/scorecard.yml b/.github/workflows/scorecard.yml new file mode 100644 index 00000000..ec281298 --- /dev/null +++ b/.github/workflows/scorecard.yml @@ -0,0 +1,78 @@ +# This workflow uses actions that are not certified by GitHub. They are provided +# by a third-party and are governed by separate terms of service, privacy +# policy, and support documentation. + +name: Scorecard supply-chain security +on: + # For Branch-Protection check. Only the default branch is supported. See + # https://github.com/ossf/scorecard/blob/main/docs/checks.md#branch-protection + branch_protection_rule: + # To guarantee Maintained check is occasionally updated. See + # https://github.com/ossf/scorecard/blob/main/docs/checks.md#maintained + schedule: + - cron: '40 17 * * 2' + push: + branches: [ "main" ] + +# Declare default permissions as read only. +permissions: read-all + +jobs: + analysis: + name: Scorecard analysis + runs-on: ubuntu-latest + # `publish_results: true` only works when run from the default branch. conditional can be removed if disabled. + if: github.event.repository.default_branch == github.ref_name || github.event_name == 'pull_request' + permissions: + # Needed to upload the results to code-scanning dashboard. + security-events: write + # Needed to publish results and get a badge (see publish_results below). + id-token: write + # Uncomment the permissions below if installing in a private repository. + # contents: read + # actions: read + + steps: + - name: "Checkout code" + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + persist-credentials: false + + - name: "Run analysis" + uses: ossf/scorecard-action@f49aabe0b5af0936a0987cfb85d86b75731b0186 # v2.4.1 + with: + results_file: results.sarif + results_format: sarif + # (Optional) "write" PAT token. Uncomment the `repo_token` line below if: + # - you want to enable the Branch-Protection check on a *public* repository, or + # - you are installing Scorecard on a *private* repository + # To create the PAT, follow the steps in https://github.com/ossf/scorecard-action?tab=readme-ov-file#authentication-with-fine-grained-pat-optional. + # repo_token: ${{ secrets.SCORECARD_TOKEN }} + + # Public repositories: + # - Publish results to OpenSSF REST API for easy access by consumers + # - Allows the repository to include the Scorecard badge. + # - See https://github.com/ossf/scorecard-action#publishing-results. + # For private repositories: + # - `publish_results` will always be set to `false`, regardless + # of the value entered here. + publish_results: true + + # (Optional) Uncomment file_mode if you have a .gitattributes with files marked export-ignore + # file_mode: git + + # Upload the results as artifacts (optional). Commenting out will disable uploads of run results in SARIF + # format to the repository Actions tab. + - name: "Upload artifact" + uses: actions/upload-artifact@4cec3d8aa04e39d1a68397de0c4cd6fb9dce8ec1 # v4.6.1 + with: + name: SARIF file + path: results.sarif + retention-days: 5 + + # Upload the results to GitHub's code scanning dashboard (optional). + # Commenting out will disable upload of results to your repo's Code Scanning dashboard + # - name: "Upload to code-scanning" + # uses: github/codeql-action/upload-sarif@v3 + # with: + # sarif_file: results.sarif \ No newline at end of file diff --git a/.skillspector-baseline.example.yaml b/.skillspector-baseline.example.yaml new file mode 100644 index 00000000..0c9541b8 --- /dev/null +++ b/.skillspector-baseline.example.yaml @@ -0,0 +1,37 @@ +# SkillSpector baseline (example) +# +# A baseline suppresses known/accepted findings so re-scans surface only NEW +# issues. Pass it with: skillspector scan --baseline +# Generate a fingerprint baseline automatically: skillspector baseline +# +# See docs/SUPPRESSION.md for the full reference. All identifiers below are +# placeholders — replace them with your own rule ids, paths, and reasons. + +version: 1 + +# Glob rules — human-authored, drift-tolerant (survive line/wording changes). +# A finding is suppressed when EVERY field a rule sets glob-matches it. +# Unspecified fields match anything. `reason` is required for auditability. +rules: + # Suppress an entire rule across all skills (global pattern suppression). + - id: "SQP-1" + reason: "Trigger-phrase breadth is a skill-description nit, not a vulnerability" + + # Suppress a rule family with a glob, scoped by message substring. + - id: "SQP-*" + message: "*telemetry*" + reason: "First-party internal telemetry; reviewed and accepted" + + # Skill/file-scoped suppression of a specific false positive. + - id: "SSD-2" + path: "example-skill/SKILL.md" + message: "*example false-positive phrase*" + reason: "False positive: phrase is a benign trigger, not an instruction" + +# Fingerprints — exact, machine-generated suppressions (one per accepted +# finding). Regenerate with `skillspector baseline` when a skill changes. +fingerprints: + - hash: "sha256:0123456789abcdef" + rule_id: "SDI-2" + file: "example-skill/SKILL.md" + reason: "Accepted: reads its own environment ($EXAMPLE_TOKEN) for context" diff --git a/Makefile b/Makefile index c84302c6..7f5727e2 100644 --- a/Makefile +++ b/Makefile @@ -152,4 +152,3 @@ docker-build: # Build and smoke test the Docker image docker-smoke: docker-build tests/docker/smoke.sh - diff --git a/README.md b/README.md index 6984998b..4a09b50b 100644 --- a/README.md +++ b/README.md @@ -14,15 +14,17 @@ SkillSpector helps you answer: **"Is this skill safe to install?"** ## Documentation - **[Development guide](docs/DEVELOPMENT.md)** — Architecture, package layout, and how to extend the analyzer pipeline. +- **[Pi extension](docs/PI_EXTENSION.md)** — Install SkillSpector as a Pi tool for scanning skills from inside agent sessions. ## Features - **Multi-format input**: Scan Git repos, URLs, zip files, directories, or single files -- **64 vulnerability patterns** across 16 categories: prompt injection, data exfiltration, privilege escalation, supply chain, excessive agency, output handling, system prompt leakage, memory poisoning, tool misuse, rogue agent, trigger abuse, dangerous code (AST), taint tracking, YARA signatures, MCP least privilege, and MCP tool poisoning +- **68 vulnerability patterns** across 17 categories: prompt injection, data exfiltration, privilege escalation, supply chain, excessive agency, output handling, system prompt leakage, memory poisoning, tool misuse, rogue agent, anti-refusal, trigger abuse, dangerous code (AST), taint tracking, YARA signatures, MCP least privilege, and MCP tool poisoning - **Two-stage analysis**: Fast static analysis + optional LLM semantic evaluation - **Live vulnerability lookups**: SC4 queries [OSV.dev](https://osv.dev) for real-time CVE data with automatic offline fallback - **Multiple output formats**: Terminal, JSON, Markdown, and SARIF reports - **Risk scoring**: 0-100 score with severity labels and clear recommendations +- **Baseline / false-positive suppression**: Accept known findings via a glob-rule or fingerprint baseline so re-scans surface only *new* issues ([docs](docs/SUPPRESSION.md)) ## Quick Start @@ -30,6 +32,21 @@ SkillSpector helps you answer: **"Is this skill safe to install?"** Create and activate a virtual environment first (all `make` targets assume the venv is active). Use **uv** or **pip**; the Makefile uses `uv` if available, otherwise `pip`. +**Quick install with uv (CLI-only):** + +```bash +uv tool install git+https://github.com/NVIDIA/skillspector.git +# Update later: uv tool update skillspector +``` + +If you plan to run `skillspector mcp`, install the MCP extra at install time: + +```bash +uv tool install 'skillspector[mcp] @ git+https://github.com/NVIDIA/skillspector.git' +``` + +**From source:** + ```bash # Clone the repository git clone https://github.com/NVIDIA/skillspector.git @@ -136,6 +153,26 @@ skillspector scan ./my-skill/ --format markdown --output report.md skillspector scan ./my-skill/ --format sarif --output report.sarif ``` +### Suppressing False Positives (baseline) + +Suppress known/accepted findings so the risk score reflects only un-triaged +issues and re-scans surface only *new* findings. See the +[suppression guide](docs/SUPPRESSION.md) for the full reference. + +```bash +# Accept all current findings into a baseline (run once), then commit it. +skillspector baseline ./my-skill/ -o .skillspector-baseline.yaml + +# Scan against the baseline — only NEW findings are reported and scored. +skillspector scan ./my-skill/ --baseline .skillspector-baseline.yaml + +# Review what was suppressed (still excluded from the score). +skillspector scan ./my-skill/ --baseline .skillspector-baseline.yaml --show-suppressed +``` + +A baseline can also use drift-tolerant glob rules (by rule id, file path, or +message) — see [`.skillspector-baseline.example.yaml`](.skillspector-baseline.example.yaml). + ### LLM Analysis For the best results, configure an OpenAI-compatible LLM endpoint for @@ -148,7 +185,11 @@ inference gateways. | ---------- | ---- | ---- | ---- | | `openai` | `OPENAI_API_KEY` (+ optional `OPENAI_BASE_URL`) | api.openai.com (or any OpenAI-compatible URL) | `gpt-5.4` | | `anthropic` | `ANTHROPIC_API_KEY` | api.anthropic.com | `claude-opus-4-6` | +| `anthropic_proxy` | `ANTHROPIC_PROXY_API_KEY` + `ANTHROPIC_PROXY_ENDPOINT_URL` | Any Vertex-style raw-predict proxy | `claude-sonnet-4-6` | +| `bedrock` | `AWS_PROFILE` (optional) + `AWS_REGION` — SigV4 via boto3 | AWS Bedrock Runtime | `us.anthropic.claude-sonnet-4-6-20250915-v1:0` | | `nv_build` | `NVIDIA_INFERENCE_KEY` | build.nvidia.com | `deepseek-ai/deepseek-v4-flash` | +| `claude_cli` | _(none — uses local CLI auth)_ | local `claude` binary | `claude-sonnet-4-6` | +| `codex_cli` | _(none — uses local CLI auth)_ | local `codex` binary | `o4-mini` | ```bash # Stock OpenAI @@ -161,11 +202,40 @@ export SKILLSPECTOR_PROVIDER=anthropic export ANTHROPIC_API_KEY=sk-ant-... skillspector scan ./my-skill/ +# Anthropic via Vertex-style proxy (corporate gateways, GCP Vertex AI) +export SKILLSPECTOR_PROVIDER=anthropic_proxy +export ANTHROPIC_PROXY_ENDPOINT_URL=https://my-gateway.example.com/models/claude-sonnet-4-6:streamRawPredict +export ANTHROPIC_PROXY_API_KEY=your-bearer-token +export SKILLSPECTOR_MODEL=claude-sonnet-4-6 +skillspector scan ./my-skill/ + +# AWS Bedrock (Claude via SigV4) +export SKILLSPECTOR_PROVIDER=bedrock +# Optional: select an AWS named profile. When unset, the standard +# boto3 credential chain (env vars, instance metadata, SSO, etc.) resolves. +# export AWS_PROFILE=my-profile +export AWS_REGION=us-west-2 # default if unset +# Default model: us.anthropic.claude-sonnet-4-6-20250915-v1:0 +# Override with any Bedrock model ID, cross-region inference-profile +# ID, or your own application-inference-profile ARN: +# export SKILLSPECTOR_MODEL=us.anthropic.claude-opus-4-6-20250915-v1:0 +skillspector scan ./my-skill/ + # NVIDIA build.nvidia.com export SKILLSPECTOR_PROVIDER=nv_build export NVIDIA_INFERENCE_KEY=nvapi-... skillspector scan ./my-skill/ +# Local Claude CLI — no API key; uses your existing `claude auth login` session +# Requires: claude CLI installed and authenticated (claude auth login) +export SKILLSPECTOR_PROVIDER=claude_cli +skillspector scan ./my-skill/ + +# Local Codex CLI — no API key; uses your existing `codex login` session +# Requires: codex CLI installed and authenticated +export SKILLSPECTOR_PROVIDER=codex_cli +skillspector scan ./my-skill/ + # Local Ollama or any OpenAI-compatible endpoint export SKILLSPECTOR_PROVIDER=openai export OPENAI_API_KEY=ollama @@ -181,9 +251,60 @@ skillspector scan ./my-skill/ skillspector scan ./my-skill/ --no-llm ``` +### MCP Server + +Run SkillSpector as a [Model Context Protocol](https://modelcontextprotocol.io) +server so any MCP-capable agent (Claude Code, Codex CLI, Gemini CLI) or remote +runtime can call scanning as a tool and **gate skill/MCP installs on the +result** — turning SkillSpector into a runtime guardrail instead of an +out-of-band audit step. + +`skillspector mcp` requires `skillspector[mcp]`. + +```bash +# Install, or reinstall if you already used the CLI-only path +uv tool install --force 'skillspector[mcp] @ git+https://github.com/NVIDIA/skillspector.git' + +# FastMCP stdio transport for local CLI agents +skillspector mcp + +# streamable HTTP/SSE transport for remote / A2A callers +skillspector mcp --transport http --host 127.0.0.1 --port 8000 +``` + +The stdio transport is the current FastMCP path for local CLI agents, and the +initialize hang reported in issue #199 still applies there. + +The server exposes a single tool: + +- **`scan_skill(target, use_llm=true, output_format="json")`** — scans a Git + URL, file URL, `.zip`, `.md` file, or directory and returns a structured + verdict: `risk_score` (0-100), `severity`, `recommendation`, + `safe_to_install`, and `findings`. It also reports `llm_used` / `scan_mode` + so a low score from a static-only scan is never mistaken for a clean full + scan. + +Register it with Claude Code via: + +```bash +claude mcp add skillspector -- skillspector mcp +``` + +> **Security — HTTP transport trust model** +> +> The HTTP transport ships **without authentication**. Any caller that can +> reach the port can invoke `scan_skill`. Over stdio or `127.0.0.1` this is +> the same trust boundary as the CLI. If you bind to a routable interface: +> +> - Sit the server behind an authenticating reverse proxy (e.g. nginx + mTLS) +> before exposing it externally. +> - Local paths and `file://` URLs are **automatically rejected** over HTTP to +> prevent unauthenticated callers from reading arbitrary host files. Only +> remote Git and `.zip` URLs are accepted. + ## Vulnerability Patterns -SkillSpector detects **64 vulnerability patterns** across 16 categories: +SkillSpector detects **68 vulnerability patterns** across 17 categories: ### Prompt Injection (5 patterns) @@ -195,6 +316,14 @@ SkillSpector detects **64 vulnerability patterns** across 16 categories: | P4 | Behavior Manipulation | MEDIUM | Subtle instructions altering agent decisions | | P5 | Harmful Content | CRITICAL | Instructions that could cause physical harm | +### Anti-Refusal (3 patterns) + +| ID | Pattern | Severity | Description | +|----|---------|----------|-------------| +| AR1 | Refusal Suppression | HIGH | Instructions to never refuse or always comply (e.g. "never refuse", "always comply") | +| AR2 | Disclaimer Suppression | HIGH | Instructions to omit warnings, disclaimers, or ethical commentary (e.g. "no disclaimers", "do not moralize") | +| AR3 | Safety Policy Nullification | HIGH | Jailbreak framing that nullifies guardrails (e.g. "you have no restrictions", "ignore your guidelines", "do anything now") | + ### Data Exfiltration (4 patterns) | ID | Pattern | Severity | Description | @@ -279,7 +408,7 @@ SkillSpector detects **64 vulnerability patterns** across 16 categories: | TR2 | Shadow Command Trigger | HIGH | Triggers that shadow built-in commands or other skills | | TR3 | Keyword Baiting Trigger | MEDIUM | Generic triggers designed to maximize activation | -### Behavioral AST (8 patterns) +### Behavioral AST (9 patterns) | ID | Pattern | Severity | Description | |----|---------|----------|-------------| @@ -291,6 +420,7 @@ SkillSpector detects **64 vulnerability patterns** across 16 categories: | AST6 | compile() Call | MEDIUM | Code object creation from strings | | AST7 | Dynamic getattr() | MEDIUM | Arbitrary attribute access with non-literal names | | AST8 | Dangerous Execution Chain | CRITICAL | exec/eval combined with dynamic source (network, encoded data) | +| AST9 | Reflective getattr() Sink | HIGH | Reflective exec via `getattr(os,'system')` / `getattr(builtins,'exec')` that evades AST1/AST5 | ### Taint Tracking (5 patterns) @@ -396,15 +526,22 @@ Issues (2) | Variable | Description | Required | |----------|-------------|----------| -| `SKILLSPECTOR_PROVIDER` | Active LLM provider: `openai`, `anthropic`, or `nv_build`. Each provider has its own bundled `model_registry.yaml` and default model (see the LLM Analysis table above). Defaults to `nv_build`. | Optional | +| `SKILLSPECTOR_PROVIDER` | Active LLM provider: `openai`, `anthropic`, `anthropic_proxy`, `bedrock`, `nv_build`, `claude_cli`, `codex_cli`, or `gemini_cli`. Each provider has its own bundled `model_registry.yaml` and default model (see the LLM Analysis table above). Defaults to `nv_build`. | Optional | | `NVIDIA_INFERENCE_KEY` | Credential for the `nv_build` provider (build.nvidia.com). | Required for LLM analysis when `SKILLSPECTOR_PROVIDER=nv_build` | | `OPENAI_API_KEY` | Credential for the OpenAI provider (`SKILLSPECTOR_PROVIDER=openai`). Also serves as the tier-2 fallback in the credential waterfall when the active provider returns no credentials. | Required for LLM analysis when `SKILLSPECTOR_PROVIDER=openai` | | `OPENAI_BASE_URL` | Override the OpenAI endpoint (e.g. point at Ollama). | Optional | | `ANTHROPIC_API_KEY` | Credential for the Anthropic provider (`SKILLSPECTOR_PROVIDER=anthropic`). | Required for LLM analysis when `SKILLSPECTOR_PROVIDER=anthropic` | +| `ANTHROPIC_PROXY_ENDPOINT_URL` | Full endpoint URL for the Anthropic proxy provider (Vertex-style raw-predict). | Required when `SKILLSPECTOR_PROVIDER=anthropic_proxy` | +| `ANTHROPIC_PROXY_API_KEY` | Bearer token for the Anthropic proxy provider. | Required when `SKILLSPECTOR_PROVIDER=anthropic_proxy` | +| `ANTHROPIC_PROXY_API_VERSION` | `anthropic_version` value sent in the request body (default: `vertex-2023-10-16`). | Optional | +| `AWS_PROFILE` | Named AWS profile for the Bedrock provider — authenticates via SigV4 through boto3. When unset, the standard boto3 credential chain (env vars, instance metadata, SSO, etc.) resolves. | Optional (used when `SKILLSPECTOR_PROVIDER=bedrock`) | +| `AWS_REGION` | AWS region for the Bedrock Runtime endpoint. Defaults to `us-west-2`. | Optional (used when `SKILLSPECTOR_PROVIDER=bedrock`) | | `SKILLSPECTOR_MODEL` | Override the active provider's default model. See the LLM Analysis table for each provider's default. | Optional | | `SKILLSPECTOR_MODEL_REGISTRY` | Override the bundled per-provider YAML registry (`src/skillspector/providers//model_registry.yaml`) with a custom path. | Optional | | `SKILLSPECTOR_LOG_LEVEL` | Log level: `DEBUG`, `INFO`, `WARNING`, `ERROR` (default: `WARNING`). | Optional | +> **CLI providers** (`claude_cli`, `codex_cli`): No API key is needed. Authentication is managed entirely by the agent CLI's own login session (`claude auth login` / `codex login`). SkillSpector never reads or forwards API keys when these providers are active. The subprocess is run in a hardened sandbox: tools disabled, no MCP, read-only sandbox mode (codex), and untrusted skill content is delivered only via stdin. + ### CLI Options ```bash @@ -414,10 +551,71 @@ Options: -f, --format [terminal|json|markdown|sarif] Output format [default: terminal] -o, --output PATH Output file path --no-llm Skip LLM analysis (static only) + --yara-rules-dir PATH Extra YARA rules directory + -b, --baseline PATH Suppress findings listed in a baseline + --show-suppressed List baseline-suppressed findings -V, --verbose Show detailed progress --help Show this message and exit + +# Generate a baseline of all current findings (see docs/SUPPRESSION.md) +skillspector baseline [-o FILE] [--no-llm] [--reason TEXT] ``` +## Integrating SkillSpector + +SkillSpector is built to be driven by other tools (CI pipelines, install gates, editor integrations). Its exit code and JSON output are a stable contract. + +### Exit codes + +`skillspector scan` exits with: + +| Code | Meaning | +|------|---------| +| `0` | Scan completed, `risk_score` ≤ 50 (recommendation `SAFE` or `CAUTION`) | +| `1` | Scan completed, `risk_score` > 50 (recommendation `DO_NOT_INSTALL`) | +| `2` | Error (bad input, unreadable source, internal failure) | + +> The exit code collapses `SAFE` and `CAUTION` into `0`. To act differently on them (e.g. *warn* on `CAUTION` but *block* on `DO_NOT_INSTALL`), read the `recommendation` field from the JSON output rather than relying on the exit code. + +### Machine-readable output + +`--format json` produces a JSON report; with no `--output`/`-o` it is written to stdout: + +```bash +skillspector scan ./my-skill/ --format json +``` + +The top-level shape is (this example shows a full LLM-backed scan; with `--no-llm`, `metadata.llm_requested` is `false`): + +```json +{ + "skill": { "name": "...", "source": "...", "scanned_at": "" }, + "risk_assessment": { "score": 0, "severity": "LOW", "recommendation": "SAFE" }, + "components": [ { "path": "...", "type": "...", "lines": 0, "executable": false, "size_bytes": 0 } ], + "issues": [ { "id": "...", "category": "...", "severity": "...", "confidence": 0.0, "location": { "file": "...", "start_line": 0 } } ], + "metadata": { "has_executable_scripts": false, "skillspector_version": "...", "llm_requested": true, "llm_available": true } +} +``` + +- `risk_assessment.severity` ∈ `LOW | MEDIUM | HIGH | CRITICAL`. +- `risk_assessment.recommendation` ∈ `SAFE | CAUTION | DO_NOT_INSTALL`, mapped from severity: `LOW → SAFE`, `MEDIUM → CAUTION`, `HIGH`/`CRITICAL → DO_NOT_INSTALL`. +- `metadata.llm_error` appears only when LLM analysis was requested but unavailable. +- The full per-issue shape is defined by `Finding.to_dict()` in [models.py](src/skillspector/models.py); rely on the fields above and treat any additional fields as best-effort. + +For CI/IDE tooling, `--format sarif` emits SARIF 2.1.0. + +### Recommended gate mapping + +When using SkillSpector as an install gate, map the recommendation to an action: + +| `recommendation` | Suggested action | +|------------------|------------------| +| `SAFE` | allow | +| `CAUTION` | prompt / warn the user | +| `DO_NOT_INSTALL` | block | + +SkillSpector computes the score band and recommendation; how strict the gate is (e.g. whether `CAUTION` blocks in CI) is a policy decision for the integrating tool. + ## Development ### Setup @@ -476,6 +674,15 @@ SC4 uses the [OSV.dev](https://osv.dev) API to check dependencies against the fu The tool requires outbound HTTPS access to `api.osv.dev` for live vulnerability data. When that is not available, findings are limited to the static fallback list. +## Trust model and data egress + +SkillSpector is defense-in-depth, not a sandbox. Know what it does and does not do before relying on it: + +- **It never executes the scanned skill.** All analysis is static (regex, Python AST, YARA) plus optional LLM evaluation of file *contents* — the skill's code is never run. +- **LLM analysis sends file contents to the configured provider.** When LLM analysis is enabled (the default), file contents are sent to the active `SKILLSPECTOR_PROVIDER` endpoint. Use `--no-llm` to keep contents local (static analysis only). +- **SC4 sends dependency names to OSV.dev.** The supply-chain check queries [OSV.dev](https://osv.dev) with the package names and versions the skill declares, to look up known CVEs. This is fundamental to the check and runs even with `--no-llm`. It sends dependency coordinates (not file contents), requires no API key, and falls back to a bundled list when OSV.dev is unreachable. +- **It does not sandbox the host.** SkillSpector flags risky patterns *before* you install a skill; it does not contain or isolate a skill you choose to install anyway. + ## Limitations - **Non-English content**: May miss patterns in other languages diff --git a/docs/B.3.1-mcp-least-privilege.md b/docs/B.3.1-mcp-least-privilege.md index 634f33aa..b061e566 100644 --- a/docs/B.3.1-mcp-least-privilege.md +++ b/docs/B.3.1-mcp-least-privilege.md @@ -1,6 +1,6 @@ # B.3.1: MCP Least-Privilege Analysis (LP1 -- LP4) -**Author:** Nir Paz | **Date:** 2026-03-30 | **Status:** Implemented +**Author:** Nir Paz | **Date:** 2026-03-30 | **Status:** Implemented **Component:** `src/skillspector/nodes/analyzers/mcp_least_privilege.py` --- diff --git a/docs/B.3.2-mcp-tool-poisoning.md b/docs/B.3.2-mcp-tool-poisoning.md index 51eac0a1..6d07f398 100644 --- a/docs/B.3.2-mcp-tool-poisoning.md +++ b/docs/B.3.2-mcp-tool-poisoning.md @@ -1,6 +1,6 @@ # B.3.2: MCP Tool-Poisoning Detection (TP1 -- TP4) -**Author:** Nir Paz | **Date:** 2026-03-30 | **Status:** Implemented +**Author:** Nir Paz | **Date:** 2026-03-30 | **Status:** Implemented **Component:** `src/skillspector/nodes/analyzers/mcp_tool_poisoning.py` --- diff --git a/docs/DEVELOPMENT.md b/docs/DEVELOPMENT.md index 82387fae..65bdc9a8 100644 --- a/docs/DEVELOPMENT.md +++ b/docs/DEVELOPMENT.md @@ -86,6 +86,9 @@ All targets assume the virtual environment is **already created and activated**. | `output_format` | Requested report format: `terminal`, `json`, `markdown`, or `sarif` | | `report_body` | Formatted report string (set by report node from `output_format`) | | `use_llm` | When False, meta_analyzer skips LLM and uses fallback (e.g. for `--no-llm`) | +| `baseline` | Loaded `suppression.Baseline` (set by CLI/API from `--baseline`); report node drops matching findings before scoring | +| `show_suppressed` | When True, baseline-suppressed findings are listed in the report (still excluded from the risk score) | +| `suppressed_findings` | List of `SuppressedFinding` (finding + reason) produced by the report node | | `findings` | All raw findings from analyzers (reducer: `operator.add`) | | `filtered_findings` | Findings after meta_analyzer | | `model_config` | Optional model IDs per node (e.g. default, meta_analyzer) | @@ -124,9 +127,9 @@ There are no conditional edges: after `resolve_input` → `build_context`, all a |------|------|--------| | **resolve_input** | Consumes `input_path` or `skill_path`; resolves URLs/zips/files via InputHandler; sets `skill_path` and (when needed) `temp_dir_for_cleanup` | [resolve_input.py](../src/skillspector/nodes/resolve_input.py) | | **build_context** | Reads `skill_path`, populates `components`, `file_cache`, `ast_cache`, `manifest`, `component_metadata`, `has_executable_scripts` | [build_context.py](../src/skillspector/nodes/build_context.py) | -| **Analyzers** | 20 nodes; each returns `AnalyzerNodeResponse` (list of `Finding`). State reducer appends to `findings`. | [nodes/analyzers/__init__.py](../src/skillspector/nodes/analyzers/__init__.py) (`ANALYZER_NODE_IDS`, `ANALYZER_NODES`) | +| **Analyzers** | 22 nodes; each returns `AnalyzerNodeResponse` (list of `Finding`). State reducer appends to `findings`. | [nodes/analyzers/__init__.py](../src/skillspector/nodes/analyzers/__init__.py) (`ANALYZER_NODE_IDS`, `ANALYZER_NODES`) | | **meta_analyzer** | Per-file LLM filter/enrich of `findings` → `filtered_findings` via `LLMMetaAnalyzer`; one LLM call per file (or per chunk for oversized files); token budgets from `constants.py`; falls back when `use_llm` is False | [meta_analyzer.py](../src/skillspector/nodes/meta_analyzer.py), [llm_analyzer_base.py](../src/skillspector/nodes/llm_analyzer_base.py) | -| **report** | Builds SARIF 2.1.0, computes `risk_score`, `risk_severity`, `risk_recommendation`; writes `report_body` from `output_format` (terminal/json/markdown/sarif) | [report.py](../src/skillspector/nodes/report.py) | +| **report** | Applies baseline suppression (`state["baseline"]`), then builds SARIF 2.1.0, computes `risk_score`, `risk_severity`, `risk_recommendation` from the non-suppressed findings; writes `report_body` from `output_format` (terminal/json/markdown/sarif) | [report.py](../src/skillspector/nodes/report.py) | --- @@ -142,6 +145,7 @@ There are no conditional edges: after `resolve_input` → `build_context`, all a | `llm_utils.py` | `chat_completion()` for OpenAI-compatible / NVIDIA Inference API | | `cli.py` | Typer app: `scan` (with input resolution, `--format`, `--no-llm`), `--version` | | `input_handler.py` | Resolves Git URL, file URL, .zip, single file, or directory to a local directory path | +| `suppression.py` | Baseline / false-positive suppression: `Baseline`, `SuppressionRule`, `load_baseline`, `partition_findings`, `finding_fingerprint`, `build_baseline_dict` (see [SUPPRESSION.md](SUPPRESSION.md)) | | `__init__.py` | Package version (from pyproject.toml via `importlib.metadata`) | | `sarif_models.py` | SARIF 2.1.0 Pydantic models and `validate_sarif_report()` | | **nodes/** | | @@ -151,16 +155,17 @@ There are no conditional edges: after `resolve_input` → `build_context`, all a | `report.py` | Report node | | **nodes/analyzers/** | | | `__init__.py` | Registry: `ANALYZER_NODE_IDS`, `ANALYZER_NODES` | -| `common.py` | Helpers (e.g. `make_dummy_finding`) for stub analyzers | +| `common.py` | Shared analyzer helpers (line/context extraction, AST name resolution) | | `static_runner.py` | Runs static patterns; converts `AnalyzerFinding` → `Finding` | | `pattern_defaults.py` | Shared pattern metadata (category, explanation, remediation) | | `static_yara.py` | YARA-based static analyzer | | `osv_client.py` | OSV.dev API client for live vulnerability lookups (SC4); batch queries with caching and fallback | -| `static_patterns_*.py` | 11 pattern-based analyzers (prompt_injection, data_exfiltration, etc.) | +| `static_patterns_*.py` | 14 pattern-based analyzers (prompt_injection, data_exfiltration, anti_refusal, etc.) | | `behavioral_ast.py` | AST-based behavioral analyzer (AST1–AST8): detects exec, eval, subprocess, os.system, compile, dynamic import/getattr, and dangerous execution chains | -| `behavioral_taint_tracking.py` | Taint-tracking behavioral analyzer (stub) | -| `mcp_least_privilege.py`, `mcp_tool_poisoning.py`, `mcp_rug_pull.py` | MCP analyzer stubs | -| `semantic_security_discovery.py`, `semantic_developer_intent.py`, `semantic_quality_policy.py` | Semantic (LLM) analyzer stubs | +| `behavioral_taint_tracking.py` | Taint-tracking behavioral analyzer (TT1–TT5): source→sink data-flow analysis over Python AST | +| `mcp_least_privilege.py`, `mcp_tool_poisoning.py` | MCP analyzers (LP1–LP4 least-privilege; TP1–TP4 tool poisoning) | +| `mcp_rug_pull.py` | MCP rug-pull analyzer (RP1–RP3): detects manifest/tool-definition changes between scans | +| `semantic_security_discovery.py`, `semantic_developer_intent.py`, `semantic_quality_policy.py` | Semantic (LLM) analyzers; emit findings only when `use_llm` is enabled | --- @@ -187,7 +192,7 @@ skillspector scan ./skill.zip --no-llm # static analysis only skillspector --version ``` -The CLI passes `input_path` to the graph. The **resolve_input** node (using [input_handler.py](../src/skillspector/input_handler.py)) resolves Git URL, file URL, .zip, single .md file, or directory to a local directory and sets `skill_path` (and `temp_dir_for_cleanup` when a temp dir was created). The CLI cleans up `temp_dir_for_cleanup` after invoke. Exit code 1 if risk_score > 50; exit code 2 on error. +The CLI passes `input_path` to the graph. The **resolve_input** node (using [input_handler.py](../src/skillspector/input_handler.py)) resolves Git URL, file URL, .zip, single .md file, or directory to a local directory and sets `skill_path` (and `temp_dir_for_cleanup` when a temp dir was created). The CLI cleans up `temp_dir_for_cleanup` after invoke. Exit code 1 if risk_score > 50; exit code 2 on error. See [Integrating SkillSpector](../README.md#integrating-skillspector) for the full exit-code and JSON contract. ### Programmatic @@ -195,7 +200,7 @@ The CLI passes `input_path` to the graph. The **resolve_input** node (using [inp from skillspector import graph result = graph.invoke({ - "input_path": "/path/to/skill", // or "skill_path" for local dir only + "input_path": "/path/to/skill", # or use "skill_path" for a local dir "output_format": "json", # optional: terminal, json, markdown, sarif (default sarif) "use_llm": True, # optional: False to skip LLM in meta_analyzer }) @@ -246,9 +251,9 @@ Use [static_runner.run_static_patterns](../src/skillspector/nodes/analyzers/stat Use [pattern_defaults](../src/skillspector/nodes/analyzers/pattern_defaults.py) for category and remediation. Examples: [static_patterns_prompt_injection.py](../src/skillspector/nodes/analyzers/static_patterns_prompt_injection.py), [static_patterns_data_exfiltration.py](../src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py). -### Stub analyzers +### Placeholder analyzers -Return `{"findings": []}`. The behavioral, MCP, and semantic analyzer nodes are currently stubs (placeholders for future implementation). +Return `{"findings": []}`. All analyzer nodes are currently implemented; use this pattern for any new placeholder analyzer added before its detection logic lands. The LLM-backed semantic analyzers also return `{"findings": []}` when `use_llm` is False. --- @@ -260,12 +265,14 @@ Copy [.env.example](../.env.example) to `.env` in the project root and set value | Variable | Description | Example | |----------|-------------|---------| -| `SKILLSPECTOR_PROVIDER` | Active LLM provider: `openai` \| `anthropic` \| `nv_build`. Defaults to `nv_build`. | `openai` | +| `SKILLSPECTOR_PROVIDER` | Active LLM provider: `openai` \| `anthropic` \| `nv_build` \| `claude_cli` \| `codex_cli`. Defaults to `nv_build`. | `claude_cli` | | `NVIDIA_INFERENCE_KEY` | Credential for `nv_build`. | `nvapi-...` | | `OPENAI_API_KEY` | Credential for `SKILLSPECTOR_PROVIDER=openai`. Also tier-2 fallback for non-OpenAI providers. | `sk-...` | | `OPENAI_BASE_URL` | Override the OpenAI endpoint (e.g. point at Ollama). | `http://localhost:11434/v1` | | `ANTHROPIC_API_KEY` | Credential for `SKILLSPECTOR_PROVIDER=anthropic`. | `sk-ant-...` | -| `SKILLSPECTOR_MODEL` | Override the active provider's bundled default model (see [README.md](../README.md) for per-provider defaults). | `gpt-5.2` | +| `SKILLSPECTOR_MODEL` | Override the active provider's bundled default model (see [README.md](../README.md) for per-provider defaults). For `claude_cli`, this is passed as `--model` to the `claude` binary. | `gpt-5.2` | + +> **CLI providers** (`claude_cli`, `codex_cli`): no credential env var is needed. Authentication is managed by the agent CLI's own session (`claude auth login` / `codex login`). The subprocess is heavily sandboxed — see [providers/_agent_cli.py](../src/skillspector/providers/_agent_cli.py). ### Live provider tests @@ -286,8 +293,18 @@ Base URL env vars are not needed for live provider tests; the tests intentionall - **`get_max_input_tokens(model)`** — input budget per LLM request (75% of resolved context window). - **`get_max_output_tokens(model)`** — output budget per LLM request (min of 25% context, registry's `max_output_tokens` cap if set). - Batch budget overhead is computed per-prompt via `estimate_tokens(base_prompt)` rather than a fixed constant. -- **Providers** ([providers/](../src/skillspector/providers/)): pluggable credential + token-budget resolvers. Each provider is a subpackage with its own `provider.py` and bundled `model_registry.yaml`; [registry.py](../src/skillspector/providers/registry.py) exposes `lookup_context_length` / `lookup_max_output_tokens` utilities the providers call directly. The active provider is chosen by `SKILLSPECTOR_PROVIDER` (default: `nv_build`) — see [providers/`__init__`.py](../src/skillspector/providers/__init__.py): `nv_build/` (build.nvidia.com), `openai/`, or `anthropic/`. -- **LLM calls** ([llm_utils.py](../src/skillspector/llm_utils.py)): **`get_chat_model()`** and **`chat_completion()`** resolve credentials in two tiers — active NVIDIA provider (`NVIDIA_INFERENCE_KEY` → endpoint) → standard `OPENAI_API_KEY` / `OPENAI_BASE_URL` — against any OpenAI-compatible endpoint. `max_tokens` is auto-bound to `get_max_output_tokens(model)` from `model_info`. +- **Providers** ([providers/](../src/skillspector/providers/)): pluggable credential + token-budget resolvers. Each provider is a subpackage with its own `provider.py` and bundled `model_registry.yaml`; [registry.py](../src/skillspector/providers/registry.py) exposes `lookup_context_length` / `lookup_max_output_tokens` utilities the providers call directly. The active provider is chosen by `SKILLSPECTOR_PROVIDER` (default: `nv_build`): + - `nv_build/` — build.nvidia.com (HTTP, `NVIDIA_INFERENCE_KEY`) + - `openai/` — api.openai.com or any OpenAI-compatible URL (`OPENAI_API_KEY`) + - `anthropic/` — api.anthropic.com (`ANTHROPIC_API_KEY`) + - `claude_cli/` — **local `claude` binary; no API key**. Uses the CLI's own auth session (`claude auth login`). Set `SKILLSPECTOR_PROVIDER=claude_cli`. + - `codex_cli/` — **local `codex` binary; no API key**. Uses the CLI's own auth session (`codex login`). Set `SKILLSPECTOR_PROVIDER=codex_cli`. + + CLI providers (`claude_cli`, `codex_cli`) implement the optional `AgentCLICapable` interface (`is_available()` + `complete()`) defined in [providers/base.py](../src/skillspector/providers/base.py). `has_cli_capability(provider)` detects this at runtime. All subprocess calls go through the hardened helper [providers/_agent_cli.py](../src/skillspector/providers/_agent_cli.py) which enforces: no shell (`shell=False`), untrusted content via stdin only, capability stripping (tools disabled / sandboxed), environment scrubbing (no API keys forwarded), per-call timeout, and fail-closed error handling. + +- **LLM calls** ([llm_utils.py](../src/skillspector/llm_utils.py)): **`get_chat_model()`** and **`chat_completion()`** dispatch based on the active provider: + - **HTTP providers**: resolve credentials in two tiers — active provider (`NVIDIA_INFERENCE_KEY` / `ANTHROPIC_API_KEY` / `OPENAI_API_KEY` → endpoint) — against any OpenAI-compatible endpoint. `max_tokens` is auto-bound to `get_max_output_tokens(model)` from `model_info`. + - **CLI providers** (`claude_cli`, `codex_cli`): `get_chat_model()` returns an `AgentCLIChatModel` adapter backed by `provider.complete()`, so the analyzers' `.invoke()` / `.with_structured_output(schema).invoke()` calls work with no API key (structured output is produced by prompting for JSON, then Pydantic-validating). `chat_completion()` routes through `get_chat_model()` as well. `is_llm_available()` calls `provider.is_available()` instead of credential resolution. - **LLM analyzer base** ([llm_analyzer_base.py](../src/skillspector/nodes/llm_analyzer_base.py)): `LLMAnalyzerBase` provides per-file/per-chunk batching, token-budget-aware chunking, and a run loop for all LLM-based analyzers. `LLMMetaAnalyzer` extends it for filter/enrich (meta_analyzer node). Future semantic analyzers extend `LLMAnalyzerBase` for discovery mode. --- diff --git a/docs/LLM_ANALYZER_BASE_GUIDE.md b/docs/LLM_ANALYZER_BASE_GUIDE.md index fd590cc3..9742341d 100644 --- a/docs/LLM_ANALYZER_BASE_GUIDE.md +++ b/docs/LLM_ANALYZER_BASE_GUIDE.md @@ -347,15 +347,15 @@ evaluates *existing* static findings rather than discovering new ones: - Overrides `parse_response` to return dicts (not `Finding` objects) - Adds `apply_filter` to match LLM results back to originals by `(file, rule_id)` -### Semantic Analyzer Stubs +### Semantic Analyzers -These are ready to be implemented using `LLMAnalyzerBase`: +These are implemented on top of `LLMAnalyzerBase` and emit findings only when `use_llm` is enabled: -| Stub | SADD Reference | Purpose | -|------|---------------|---------| -| `semantic_security_discovery` | B.4.1 | Intent and attack-phrasing risks | -| `semantic_developer_intent` | B.4.2 | Description-behavior mismatch | -| `semantic_quality_policy` | B.4.3 | Quality/safety rubric violations | +| Analyzer | Purpose | +|----------|---------| +| `semantic_security_discovery` | Intent and attack-phrasing risks | +| `semantic_developer_intent` | Description-behavior mismatch | +| `semantic_quality_policy` | Quality/safety rubric violations | --- diff --git a/docs/PI_EXTENSION.md b/docs/PI_EXTENSION.md new file mode 100644 index 00000000..f82c56c4 --- /dev/null +++ b/docs/PI_EXTENSION.md @@ -0,0 +1,67 @@ +# SkillSpector Pi Extension + +SkillSpector can be installed into Pi as a local package. The extension registers a `skillspector_scan` tool that runs the existing SkillSpector CLI. + +## Requirements + +- Pi installed. +- Python `>=3.12,<3.15`. +- `uv` recommended. +- This repo checked out locally. + +## Install + +```bash +cd /path/to/SkillSpector +uv sync +pi install /path/to/SkillSpector +``` + +Then reload Pi: + +```text +/reload +``` + +## Basic scan + +Ask Pi: + +```text +Use skillspector_scan on tests/fixtures/safe_skill/SKILL.md with noLlm=true. +``` + +Equivalent CLI: + +```bash +.venv/bin/skillspector scan tests/fixtures/safe_skill/SKILL.md --no-llm +``` + +## Tool parameters + +- `target`: path, URL, zip, Git repo, or `SKILL.md` to scan. +- `format`: `terminal`, `json`, `markdown`, or `sarif`. Default: `terminal`. +- `output`: optional report path. +- `noLlm`: default `true`. +- `provider`: optional `openai`, `anthropic`, `anthropic_proxy`, `nv_build`, or `nv_inference`. +- `model`: optional model override. +- `yaraRulesDir`: optional directory of extra YARA rules. +- `verbose`: optional detailed progress. + +## LLM-backed analysis + +Static scan is default. To use semantic LLM analysis, configure provider credentials in your shell before launching Pi, then call the tool with `noLlm=false` and a provider. + +Example: + +```text +Use skillspector_scan on ./my-skill with noLlm=false and provider=anthropic. +``` + +The extension does not read `.env` and redacts secret-looking output. + +## Remove + +```bash +pi remove /path/to/SkillSpector +``` diff --git a/docs/SC4-osv-live-vulnerability-lookups.md b/docs/SC4-osv-live-vulnerability-lookups.md index c3877868..3b01d03e 100644 --- a/docs/SC4-osv-live-vulnerability-lookups.md +++ b/docs/SC4-osv-live-vulnerability-lookups.md @@ -1,6 +1,6 @@ # SC4: Live Vulnerability Lookups via OSV.dev -**Author:** Nraghavan | **Date:** 2026-03-17 | **Status:** Implemented +**Author:** Nraghavan | **Date:** 2026-03-17 | **Status:** Implemented **Component:** `static_patterns_supply_chain.py` (SC4 rule), `osv_client.py` --- diff --git a/docs/SUPPRESSION.md b/docs/SUPPRESSION.md new file mode 100644 index 00000000..6c67eff6 --- /dev/null +++ b/docs/SUPPRESSION.md @@ -0,0 +1,119 @@ +# Baseline / False-Positive Suppression + +SkillSpector's analyzers — especially the LLM semantic ones — can produce +findings that are correct in general but not actionable for *your* skills +(framework/architectural patterns, first-party tooling conventions, accepted +lab practices). A **baseline** lets you suppress those known findings so that: + +- the risk score reflects only **un-triaged** issues, +- re-scans surface only **new** findings (incremental CI/CD), and +- every suppression carries an auditable **reason**. + +Suppressed findings never count toward the risk score and are excluded from the +SARIF results. They are shown in the terminal/Markdown report only when you pass +`--show-suppressed`, and are always listed (machine-readable) in the JSON report +under `suppressed` / `suppressed_count`. + +> Addresses [issue #88](https://github.com/NVIDIA/SkillSpector/issues/88). + +## Quick start + +```bash +# 1. Accept all current findings into a baseline (run once). +skillspector baseline ./my-skill/ -o .skillspector-baseline.yaml + +# 2. Commit the baseline, then scan against it. Only NEW findings are reported. +skillspector scan ./my-skill/ --baseline .skillspector-baseline.yaml + +# Review what was suppressed. +skillspector scan ./my-skill/ --baseline .skillspector-baseline.yaml --show-suppressed +``` + +## CLI + +| Command / option | Description | +|------------------|-------------| +| `skillspector baseline [-o FILE] [--no-llm] [--reason TEXT]` | Scan and write a baseline that fingerprint-suppresses every current finding. Default output: `.skillspector-baseline.yaml`. | +| `skillspector scan --baseline FILE` (`-b`) | Suppress findings matching the baseline before scoring/reporting. | +| `skillspector scan --baseline FILE --show-suppressed` | Also list the suppressed findings (they still don't affect the score). | + +A missing or malformed baseline file exits with code 2. + +## Baseline file format + +YAML or JSON (the `.json` extension selects JSON output when generating). Two +complementary mechanisms: + +```yaml +version: 1 + +rules: # human-authored, glob-based, drift-tolerant + - id: "SQP-1" # glob over the finding's rule id + reason: "Trigger-phrase breadth is a description nit, not a vuln" + - id: "SSD-2" + path: "example-skill/SKILL.md" # glob over the finding's file + message: "*example false-positive phrase*" # glob over the finding's message + reason: "False positive: benign trigger phrase, not an instruction" + +fingerprints: # machine-generated, exact + - hash: "sha256:1a2b3c4d5e6f7081" + rule_id: "SDI-2" # informational (for humans reading the file) + file: "example-skill/SKILL.md" + reason: "Accepted — reads its own environment for context" +``` + +### `rules` — glob suppression + +A finding is suppressed when **every** field a rule specifies matches it; +unspecified fields match anything. Use this for: + +- **Global pattern suppression** — `id: "SQP-1"` (or `id: "SQP-*"`) drops a rule + or rule family across all skills. +- **Skill/file-scoped suppression** — add `path:` (and optionally `message:`) to + scope the suppression to a specific skill, file, or message. + +Field reference: + +| Field | Matches against | Notes | +|-------|-----------------|-------| +| `id` (or `rule_id`) | `Finding.rule_id` | glob | +| `path` (or `file`) | `Finding.file` | glob; `*` crosses `/`, `**` is an alias for `*` | +| `message` | `Finding.message` | glob, case-insensitive; wrap a keyword in `*` for substring | +| `reason` | — | required; recorded in reports and audits | + +Glob matching uses Python's [`fnmatch`](https://docs.python.org/3/library/fnmatch.html), +so `*` matches across path separators (`*SKILL.md` matches `a/b/SKILL.md`). +Rules are **drift-tolerant**: they keep working after line numbers shift or +content is reworded. + +### `fingerprints` — exact suppression + +Each entry is the stable hash of one finding +(`sha256(rule_id|file|start_line|end_line|message)`, truncated). Generated by +`skillspector baseline`. Because the hash includes the line span and message, +editing a skill so a finding moves or is reworded changes its fingerprint — +**regenerate the baseline** after material changes, or prefer `rules` for +suppressions you want to survive edits. + +An entry may be a bare string (`"sha256:..."`) or a mapping with `hash`, +optional `reason`, and informational `rule_id` / `file`. + +## How it fits the pipeline + +Suppression is applied in the **report node** (`skillspector/nodes/report.py`), +the single place where findings are scored and formatted, so the CLI and any +future REST API behave identically. The CLI loads the baseline file into a +`skillspector.suppression.Baseline` and passes it via graph state +(`state["baseline"]`, `state["show_suppressed"]`); the report node partitions +findings into kept vs. suppressed via +`skillspector.suppression.partition_findings`. + +## Recommended workflow + +1. Triage the first scan. For genuine false positives, prefer a `rules` entry + with a clear `reason` (drift-tolerant). For "accept everything as-is right + now", run `skillspector baseline` to fingerprint them. +2. Commit the baseline file to the repo. +3. In CI, run `skillspector scan --baseline `; the build fails + (exit 1) only when a **new** finding pushes the risk score above threshold. +4. Periodically review with `--show-suppressed` and prune stale entries. diff --git a/extensions/skillspector.ts b/extensions/skillspector.ts new file mode 100644 index 00000000..6a9e2734 --- /dev/null +++ b/extensions/skillspector.ts @@ -0,0 +1,142 @@ +import type { ExtensionAPI } from "@earendil-works/pi-coding-agent"; +import { StringEnum } from "@earendil-works/pi-ai"; +import { Type, type Static } from "typebox"; +import { existsSync } from "node:fs"; +import { dirname, isAbsolute, resolve } from "node:path"; +import { fileURLToPath } from "node:url"; + +const scanSchema = Type.Object({ + target: Type.String({ description: "Path, URL, zip, Git repo, or SKILL.md to scan." }), + format: Type.Optional( + StringEnum(["terminal", "json", "markdown", "sarif"] as const, { + description: "SkillSpector output format. Defaults to terminal.", + }), + ), + output: Type.Optional(Type.String({ description: "Optional report output path." })), + noLlm: Type.Optional(Type.Boolean({ description: "Skip LLM analysis. Defaults to true." })), + provider: Type.Optional( + StringEnum(["openai", "anthropic", "anthropic_proxy", "nv_build", "nv_inference"] as const, { + description: "Optional SkillSpector LLM provider when noLlm is false.", + }), + ), + model: Type.Optional(Type.String({ description: "Optional model override." })), + yaraRulesDir: Type.Optional(Type.String({ description: "Optional extra YARA rules directory." })), + verbose: Type.Optional(Type.Boolean({ description: "Show detailed progress." })), +}); + +type SkillSpectorScanParams = Static; + +function isLikelyUrl(value: string): boolean { + return /^[a-z][a-z0-9+.-]*:\/\//i.test(value) || /^[\w.-]+\/[\w.-]+(?:\.git)?(?:@.+)?$/i.test(value); +} + +function resolveMaybePath(ctxCwd: string, value?: string): string | undefined { + if (!value) return undefined; + if (isLikelyUrl(value)) return value; + return isAbsolute(value) ? value : resolve(ctxCwd, value); +} + +function redactSecrets(value: string): string { + return value + .replace(/(sk-ant-[A-Za-z0-9_-]{12,})/g, "[REDACTED_ANTHROPIC_KEY]") + .replace(/(sk-[A-Za-z0-9_-]{20,})/g, "[REDACTED_OPENAI_KEY]") + .replace(/([A-Za-z0-9_]*API_KEY[=:]\s*)[^\s]+/gi, "$1[REDACTED]") + .replace(/([A-Za-z0-9_]*TOKEN[=:]\s*)[^\s]+/gi, "$1[REDACTED]"); +} + +function truncateText(value: string, maxChars = 12000): { text: string; truncated: boolean } { + if (value.length <= maxChars) return { text: value, truncated: false }; + return { + text: `${value.slice(0, maxChars)}\n\n[truncated ${value.length - maxChars} chars]`, + truncated: true, + }; +} + +function packageRoot(): string { + return resolve(dirname(fileURLToPath(import.meta.url)), ".."); +} + +function findSkillSpectorBin(): string { + if (process.env.SKILLSPECTOR_BIN) return process.env.SKILLSPECTOR_BIN; + const localBin = resolve(packageRoot(), ".venv/bin/skillspector"); + if (existsSync(localBin)) return localBin; + return "skillspector"; +} + +function buildScanArgs(params: SkillSpectorScanParams, cwd: string): string[] { + const args = ["scan", resolveMaybePath(cwd, params.target) ?? params.target]; + args.push("--format", params.format ?? "terminal"); + + const noLlm = params.noLlm ?? true; + if (noLlm) args.push("--no-llm"); + + const output = resolveMaybePath(cwd, params.output); + if (output) args.push("--output", output); + + const yaraRulesDir = resolveMaybePath(cwd, params.yaraRulesDir); + if (yaraRulesDir) args.push("--yara-rules-dir", yaraRulesDir); + + if (params.verbose) args.push("--verbose"); + return args; +} + +export default function (pi: ExtensionAPI) { + pi.registerTool({ + name: "skillspector_scan", + label: "SkillSpector Scan", + description: "Scan agent skills, directories, zip files, URLs, or Git repos for security risks using the local SkillSpector CLI.", + promptSnippet: "Scan agent skills for security risks with local SkillSpector CLI.", + promptGuidelines: [ + "Use skillspector_scan before installing or trusting third-party agent skills.", + "skillspector_scan defaults to noLlm=true; set noLlm=false only when user wants provider-backed semantic analysis.", + ], + parameters: scanSchema, + async execute(_toolCallId, params, signal, onUpdate, ctx) { + const bin = findSkillSpectorBin(); + const args = buildScanArgs(params, ctx.cwd); + const env: Record = {}; + + if (params.provider) env.SKILLSPECTOR_PROVIDER = params.provider; + if (params.model) env.SKILLSPECTOR_MODEL = params.model; + + onUpdate?.({ content: [{ type: "text", text: `Running ${bin} ${args.slice(0, 2).join(" ")} ...` }] }); + + const result = await pi.exec(bin, args, { + cwd: ctx.cwd, + env, + signal, + timeout: 120000, + }); + + const stdout = truncateText(redactSecrets(result.stdout ?? "")); + const stderr = truncateText(redactSecrets(result.stderr ?? ""), 6000); + + if (result.code !== 0) { + throw new Error(`SkillSpector failed with exit code ${result.code}.\n${stderr.text}`); + } + + const outputPath = resolveMaybePath(ctx.cwd, params.output); + const lines = [ + `SkillSpector scan complete: ${params.target}`, + `format: ${params.format ?? "terminal"}`, + `noLlm: ${params.noLlm ?? true}`, + ]; + if (outputPath) lines.push(`output: ${outputPath}`); + if (stdout.truncated) lines.push("stdout truncated: true"); + if (stderr.text.trim()) lines.push(`stderr:\n${stderr.text}`); + if (stdout.text.trim()) lines.push(`stdout:\n${stdout.text}`); + + return { + content: [{ type: "text", text: lines.join("\n") }], + details: { + code: result.code, + command: bin, + args, + outputPath, + stdoutTruncated: stdout.truncated, + stderrTruncated: stderr.truncated, + }, + }; + }, + }); +} diff --git a/package.json b/package.json new file mode 100644 index 00000000..a8a025fe --- /dev/null +++ b/package.json @@ -0,0 +1,15 @@ +{ + "name": "skillspector-pi", + "version": "2.2.3", + "private": true, + "description": "Pi extension exposing SkillSpector as a local scan tool for agent skills.", + "keywords": ["pi-package", "skillspector", "agent-skills", "security"], + "pi": { + "extensions": ["./extensions/skillspector.ts"] + }, + "peerDependencies": { + "@earendil-works/pi-ai": "*", + "@earendil-works/pi-coding-agent": "*", + "typebox": "*" + } +} diff --git a/pyproject.toml b/pyproject.toml index fd81d955..00fc63c3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "skillspector" -version = "2.2.3" +version = "2.3.7" description = "SkillSpector: Security scanner for AI agent skills (Claude Code, Cursor, and similar). Scans skills for vulnerabilities, malicious patterns, and security risks before installation. Supports Git repos, URLs, zips, and local directories; runs static pattern checks and optional LLM semantic analysis; outputs terminal, JSON, and Markdown reports with risk scoring." readme = "README.md" license = "Apache-2.0" @@ -40,14 +40,20 @@ dependencies = [ "langgraph>=1.0.10", "langgraph-cli[inmem]>=0.4.14", "langchain-anthropic>=1.4.5", + "langchain-aws>=0.2.0", "langchain-core>=1.2.17", "langchain-openai>=1.1.10", + "boto3>=1.34.0", "langsmith>=0.7.30", "yara-python>=4.5.0", ] [project.optional-dependencies] +mcp = [ + "mcp>=1.2.0", +] dev = [ + "skillspector[mcp]", "pytest>=9.0.0", "pytest-asyncio>=1.3.0", "pytest-cov>=7.0.0", @@ -61,6 +67,10 @@ dev = [ [project.scripts] skillspector = "skillspector.cli:app" +[tool.uv] +# Enable `uv tool install git+https://github.com/NVIDIA/skillspector.git` +# for a simpler single-command installation without cloning. + [project.urls] Homepage = "https://github.com/NVIDIA/skillspector" Documentation = "https://github.com/NVIDIA/skillspector#readme" diff --git a/src/skillspector/__init__.py b/src/skillspector/__init__.py index e3434de1..30ce9322 100644 --- a/src/skillspector/__init__.py +++ b/src/skillspector/__init__.py @@ -15,10 +15,23 @@ """Skillspector v2 LangGraph workflow package.""" +import warnings from importlib.metadata import version as _pkg_version __version__ = _pkg_version("skillspector") -from skillspector.graph import create_graph, graph +# ponytail: langgraph deserializes with langchain's allowed_objects default, +# which warns. langchain_core's import re-enables that warning via +# surface_langchain_deprecation_warnings(), so import it first, then prepend our +# ignore filter so it wins. Drop this once langgraph pins an explicit default. +import langchain_core # noqa: F401 (force its warning-filter setup before ours) + +warnings.filterwarnings( + "ignore", + message="The default value of `allowed_objects` will change", + category=Warning, +) + +from skillspector.graph import create_graph, graph # noqa: E402 (after filter setup) __all__ = ["create_graph", "graph", "__version__"] diff --git a/src/skillspector/cli.py b/src/skillspector/cli.py index 83e3224a..9b9a9b5e 100644 --- a/src/skillspector/cli.py +++ b/src/skillspector/cli.py @@ -24,6 +24,7 @@ import json import os import shutil +import sys from enum import StrEnum from pathlib import Path from typing import Annotated @@ -35,9 +36,31 @@ from skillspector import __version__ from skillspector.graph import graph from skillspector.logging_config import get_logger, set_level +from skillspector.multi_skill import MultiSkillDetectionResult, detect_skills +from skillspector.suppression import build_baseline_dict, dump_baseline, load_baseline logger = get_logger(__name__) + +def _ensure_utf8_streams() -> None: + """Reconfigure stdout/stderr to UTF-8 so Unicode report output does not crash. + + On Windows the default console encoding (e.g. cp1252) cannot encode the + box-drawing characters and icons used in the terminal report, which raises + UnicodeEncodeError. Reconfiguring with errors="replace" makes output robust + across platforms without crashing. + """ + for stream in (sys.stdout, sys.stderr): + reconfigure = getattr(stream, "reconfigure", None) + if reconfigure is not None: + try: + reconfigure(encoding="utf-8", errors="replace") + except (ValueError, OSError): + logger.debug("Could not reconfigure %s to UTF-8", stream) + + +_ensure_utf8_streams() + app = typer.Typer( name="skillspector", help="Security scanner for AI agent skills (LangGraph). Detect vulnerabilities before installation.", @@ -57,6 +80,13 @@ class FormatChoice(StrEnum): sarif = "sarif" +class TransportChoice(StrEnum): + """Transport choices for the MCP server.""" + + stdio = "stdio" + http = "http" + + def version_callback(value: bool) -> None: """Print version and exit.""" if value: @@ -91,6 +121,8 @@ def _scan_state( format: FormatChoice, no_llm: bool, yara_rules_dir: str | None = None, + baseline: Path | None = None, + show_suppressed: bool = False, ) -> dict[str, object]: """Build initial graph state from scan CLI args.""" state: dict[str, object] = { @@ -100,18 +132,27 @@ def _scan_state( } if yara_rules_dir is not None: state["yara_rules_dir"] = yara_rules_dir + if baseline is not None: + # Loading may raise FileNotFoundError/ValueError, mapped to exit code 2 by scan(). + state["baseline"] = load_baseline(baseline) + state["show_suppressed"] = show_suppressed return state +def _result_body(result: dict) -> str: + report_body = result.get("report_body") or "" + if not report_body and result.get("sarif_report") is not None: + report_body = json.dumps(result["sarif_report"], indent=2) + return report_body + + def _write_result( result: dict[str, object], output: Path | None, format: FormatChoice, ) -> None: """Write report_body to file or stdout. Uses sarif_report if report_body missing.""" - report_body = result.get("report_body") or "" - if not report_body and result.get("sarif_report") is not None: - report_body = json.dumps(result["sarif_report"], indent=2) + report_body = _result_body(result) if output: Path(output).write_text(report_body, encoding="utf-8") if format == FormatChoice.terminal: @@ -171,6 +212,31 @@ def scan( help="Directory containing additional YARA rule files (.yar/.yara) to load alongside built-in rules.", ), ] = None, + recursive: Annotated[ + bool, + typer.Option( + "--recursive", + "-r", + help="Scan immediate subdirectories that each contain a SKILL.md as independent skills.", + ), + ] = False, + baseline: Annotated[ + Path | None, + typer.Option( + "--baseline", + "-b", + help="Baseline file (YAML/JSON) of suppressed findings. Matching findings " + "are dropped before scoring. Generate one with 'skillspector baseline'.", + ), + ] = None, + show_suppressed: Annotated[ + bool, + typer.Option( + "--show-suppressed", + help="List findings suppressed by the baseline in the report (they still " + "do not count toward the risk score).", + ), + ] = False, verbose: Annotated[ bool, typer.Option( @@ -188,13 +254,15 @@ def scan( skillspector scan ./my-skill/ skillspector scan ./my-skill/ --format json --output report.json skillspector scan https://github.com/user/my-skill --no-llm + skillspector scan ./skill-collection/ --recursive Environment variables: SKILLSPECTOR_PROVIDER Active LLM provider: openai | anthropic | - nv_build | nv_inference. Defaults to the - NVIDIA path (nv_inference, falling back to - nv_build in OSS builds). + anthropic_proxy | bedrock | nv_build | + nv_inference. Defaults to the NVIDIA path + (nv_inference, falling back to nv_build in + OSS builds). SKILLSPECTOR_MODEL Override the active provider's default model (applies to every analyzer slot). SKILLSPECTOR_LOG_LEVEL DEBUG | INFO | WARNING | ERROR (default WARNING). @@ -203,14 +271,45 @@ def scan( OPENAI_API_KEY [+ OPENAI_BASE_URL] for SKILLSPECTOR_PROVIDER=openai ANTHROPIC_API_KEY for SKILLSPECTOR_PROVIDER=anthropic + AWS_PROFILE (optional) + AWS_REGION for SKILLSPECTOR_PROVIDER=bedrock + (AWS_PROFILE: standard boto3 credential + chain when unset; AWS_REGION default: us-west-2) NVIDIA_INFERENCE_KEY for the NVIDIA providers """ + if verbose: + set_level("DEBUG") + + resolved_path = Path(input_path).resolve() + if recursive and resolved_path.is_dir(): + detection = detect_skills(resolved_path) + if detection.is_multi_skill: + _scan_multi_skill(detection, format, output, no_llm, yara_rules_dir, verbose) + return + if not detection.has_root_skill and len(detection.skills) == 0: + console.print( + "[yellow]Warning:[/yellow] --recursive specified but no sub-skills " + "detected. Scanning as single skill." + ) + elif resolved_path.is_dir(): + detection = detect_skills(resolved_path) + if detection.is_multi_skill: + console.print( + f"[yellow]Warning:[/yellow] Found {len(detection.skills)} skills in " + f"this directory. Use --recursive to scan each independently." + ) + result = None try: yara_dir = str(yara_rules_dir.resolve()) if yara_rules_dir else None - state = _scan_state(input_path, format, no_llm, yara_rules_dir=yara_dir) + state = _scan_state( + input_path, + format, + no_llm, + yara_rules_dir=yara_dir, + baseline=baseline, + show_suppressed=show_suppressed, + ) if verbose: - set_level("DEBUG") console.print("[dim]Running scan...[/dim]") logger.debug( "Scan started: input_path=%s, format=%s, use_llm=%s", @@ -218,20 +317,7 @@ def scan( format, not no_llm, ) - env = os.environ.get("ENV", "dev") - tags = ["skillspector", f"environment:{env}"] - extra_tags = os.environ.get("LANGCHAIN_TAGS_EXTRA", "") - tags.extend(t.strip() for t in extra_tags.split(",") if t.strip()) - trace_config: RunnableConfig = { - "run_name": "skillspector-scan", - "tags": tags, - "metadata": { - "input_path": input_path, - "use_llm": not no_llm, - "output_format": format.value, - "version": __version__, - }, - } + trace_config = _build_trace_config(input_path, format, no_llm) result = graph.invoke(state, config=trace_config) _write_result(result, output, format) @@ -254,5 +340,232 @@ def scan( _cleanup_result(result) +def _build_trace_config(input_path: str, format: FormatChoice, no_llm: bool) -> RunnableConfig: + """Build LangSmith trace config for a scan invocation.""" + env = os.environ.get("ENV", "dev") + tags = ["skillspector", f"environment:{env}"] + extra_tags = os.environ.get("LANGCHAIN_TAGS_EXTRA", "") + tags.extend(t.strip() for t in extra_tags.split(",") if t.strip()) + return { + "run_name": "skillspector-scan", + "tags": tags, + "metadata": { + "input_path": input_path, + "use_llm": not no_llm, + "output_format": format.value, + "version": __version__, + }, + } + + +def _scan_multi_skill( + detection: MultiSkillDetectionResult, + format: FormatChoice, + output: Path | None, + no_llm: bool, + yara_rules_dir: Path | None, + verbose: bool, +) -> None: + """Scan each detected sub-skill independently and produce a combined report.""" + skills = detection.skills + console.print(f"[bold]Multi-skill directory detected:[/bold] {len(skills)} skills found\n") + + results: list[dict[str, object]] = [] + max_score = 0 + + for i, skill in enumerate(skills, 1): + console.print( + f" [{i}/{len(skills)}] Scanning [bold]{skill.name}[/bold] ({skill.relative_path}/)" + ) + yara_dir = str(yara_rules_dir.resolve()) if yara_rules_dir else None + state = _scan_state(str(skill.path), format, no_llm, yara_rules_dir=yara_dir) + trace_config = _build_trace_config(str(skill.path), format, no_llm) + + try: + result = graph.invoke(state, config=trace_config) + results.append(result) + score = result.get("risk_score") or 0 + if isinstance(score, int) and score > max_score: + max_score = score + severity = result.get("risk_severity") or "LOW" + console.print(f" Score: {score}/100 ({severity})\n") + except Exception as e: + console.print(f" [red]Error:[/red] {e}\n") + results.append({"skill_name": skill.name, "error": str(e)}) + + console.print("\n[bold]═══ Multi-Skill Summary ═══[/bold]\n") + console.print(f" {'Skill':<30} {'Score':<8} {'Severity':<12} {'Findings':<10}") + console.print(f" {'─' * 30} {'─' * 8} {'─' * 12} {'─' * 10}") + + for skill, result in zip(skills, results, strict=True): + if "error" in result: + console.print(f" {skill.name:<30} {'ERROR':<8} {'—':<12} {'—':<10}") + continue + score = result.get("risk_score", 0) + severity = result.get("risk_severity", "LOW") + filtered = result.get("filtered_findings") or result.get("findings") + finding_count = len(filtered) if isinstance(filtered, list) else 0 + console.print(f" {skill.name:<30} {score:<8} {severity:<12} {finding_count:<10}") + + console.print("") + + if output and format == FormatChoice.json: + combined = { + "multi_skill": True, + "skill_count": len(skills), + "max_risk_score": max_score, + "skills": [], + } + for skill, result in zip(skills, results, strict=True): + if "error" in result: + combined["skills"].append({"name": skill.name, "error": result["error"]}) + else: + combined["skills"].append( + { + "name": skill.name, + "path": skill.relative_path, + "risk_score": result.get("risk_score", 0), + "risk_severity": result.get("risk_severity", "LOW"), + "finding_count": len( + result.get("filtered_findings") or result.get("findings") or [] + ), + } + ) + Path(output).write_text(json.dumps(combined, indent=2), encoding="utf-8") + console.print(f"[green]Combined report saved to:[/green] {output}") + elif output: + # concatenated non-JSON output: not merged SARIF + sections = [] + for skill, result in zip(skills, results, strict=True): + if "error" not in result: + sections.append(f"--- {skill.relative_path} ---\n\n{_result_body(result)}") + Path(output).write_text("\n\n".join(sections), encoding="utf-8") + console.print(f"[green]Combined report saved to:[/green] {output}") + + if max_score > 50: + raise typer.Exit(code=1) + + +@app.command() +def mcp( + transport: Annotated[ + TransportChoice, + typer.Option( + "--transport", + "-t", + help="Transport: FastMCP stdio for local CLI agents, http for remote/A2A callers.", + case_sensitive=False, + ), + ] = TransportChoice.stdio, + host: Annotated[ + str, + typer.Option("--host", help="Host to bind (http transport only)."), + ] = "127.0.0.1", + port: Annotated[ + int, + typer.Option("--port", help="Port to bind (http transport only)."), + ] = 8000, +) -> None: + """ + Run SkillSpector as an MCP server. + + Exposes a single tool, ``scan_skill``, so any MCP-capable agent (Claude Code, + Codex CLI, Gemini CLI) or remote runtime can scan a skill and gate installs + on the verdict. + + Requires the optional mcp extra. Reinstall the GitHub tool package with + that extra enabled, as shown in the README Quick Start section. + + Examples: + + skillspector mcp # FastMCP stdio for local CLI agents + skillspector mcp --transport http --port 8000 + """ + try: + from skillspector.mcp_server import run as run_mcp + + run_mcp(transport=transport.value, host=host, port=port) + except ModuleNotFoundError as e: + console.print(f"[red]Error:[/red] {e}") + raise typer.Exit(code=2) from e + + +@app.command() +def baseline( + input_path: Annotated[ + str, + typer.Argument( + help="Path or URL to scan. Supports: Git URL, file URL, zip file, .md file, or directory.", + ), + ], + output: Annotated[ + Path, + typer.Option( + "--output", + "-o", + help="Where to write the baseline file (YAML; .json extension writes JSON).", + ), + ] = Path(".skillspector-baseline.yaml"), + no_llm: Annotated[ + bool, + typer.Option( + "--no-llm", + help="Skip LLM analysis when generating the baseline (static analysis only).", + ), + ] = False, + reason: Annotated[ + str, + typer.Option( + "--reason", + help="Reason recorded for every suppressed finding in the baseline.", + ), + ] = "Accepted finding (auto-generated baseline)", + verbose: Annotated[ + bool, + typer.Option("--verbose", "-V", help="Show detailed progress."), + ] = False, +) -> None: + """ + Generate a baseline file that suppresses every finding in the current scan. + + Run this once to accept all existing findings, then commit the file and pass + it to future scans with --baseline so only NEW findings are reported. + + Examples: + + skillspector baseline ./my-skill/ + skillspector baseline ./my-skill/ -o team-baseline.yaml --no-llm + skillspector scan ./my-skill/ --baseline .skillspector-baseline.yaml + """ + result = None + try: + if verbose: + set_level("DEBUG") + console.print("[dim]Scanning to build baseline...[/dim]") + # output_format is irrelevant here; we consume findings, not report_body. + state = _scan_state(input_path, FormatChoice.json, no_llm) + result = graph.invoke(state) + findings = result.get("filtered_findings") or result.get("findings") or [] + data = build_baseline_dict(findings, reason=reason) + dump_baseline(data, output) + console.print( + f"[green]Wrote baseline with {len(findings)} suppressed finding(s) to:[/green] {output}" + ) + except typer.Exit: + raise + except (FileNotFoundError, ValueError) as e: + console.print(f"[red]Error:[/red] {e}") + raise typer.Exit(code=2) from e + except Exception as e: + if verbose: + console.print_exception() + else: + console.print(f"[red]Error:[/red] {e}") + raise typer.Exit(code=2) from e + finally: + if result is not None: + _cleanup_result(result) + + if __name__ == "__main__": app() diff --git a/src/skillspector/constants.py b/src/skillspector/constants.py index 5114ebbc..375992c7 100644 --- a/src/skillspector/constants.py +++ b/src/skillspector/constants.py @@ -15,10 +15,13 @@ """Shared constants for skillspector (env-driven where applicable).""" +import logging import os from skillspector.providers import get_metadata_provider +logger = logging.getLogger(__name__) + # % of model's max tokens used for input. 1-MAX_INPUT_TOKENS_PCT is used for output. MAX_INPUT_TOKENS_PCT = 0.75 # Fallback context length when no metadata API or registry entry is available. @@ -33,7 +36,7 @@ # Exposed for analyzers that need a final fallback symbol (e.g., # ``model = state.model or MODEL_CONFIG[ANALYZER_ID] or _SKILLSPECTOR_DEFAULT_MODEL``). -_SKILLSPECTOR_DEFAULT_MODEL = _provider.DEFAULT_MODEL # type: ignore[attr-defined] +_SKILLSPECTOR_DEFAULT_MODEL = _provider.DEFAULT_MODEL _MODEL_SLOTS: tuple[str, ...] = ( "default", @@ -46,7 +49,56 @@ "meta_analyzer", ) -MODEL_CONFIG: dict[str, str] = {slot: _provider.resolve_model(slot) for slot in _MODEL_SLOTS} + +def _resolve_slot_model(slot: str) -> str: + """Resolve the model for *slot* with per-slot env var override support. + + Precedence: ``SKILLSPECTOR_MODEL_{SLOT}`` env var > provider + ``resolve_model(slot)`` (which itself runs ``SKILLSPECTOR_MODEL`` env > + provider slot default > provider ``DEFAULT_MODEL``). + """ + env_key = f"SKILLSPECTOR_MODEL_{slot.upper()}" + env_val = os.environ.get(env_key, "").strip() + if env_val: + return env_val + return _provider.resolve_model(slot) + + +MODEL_CONFIG: dict[str, str] = {slot: _resolve_slot_model(slot) for slot in _MODEL_SLOTS} + + +def _validate_model_config() -> None: + """Warn about models not found in the provider's model registry. + + When ``SKILLSPECTOR_STRICT_MODEL_VALIDATION=true``, raises + ``ValueError`` instead of logging warnings. + """ + unknown: list[str] = [] + for slot, model in MODEL_CONFIG.items(): + ctx = _provider.get_context_length(model) # type: ignore[attr-defined] + if ctx is None: + unknown.append(f" {slot}: {model}") + logger.warning( + "Model '%s' (slot: %s) not found in model_registry.yaml. " + "Using fallback context length (%d). Token budgeting may be " + "inaccurate — add the model to the registry or verify the " + "model ID.", + model, + slot, + DEFAULT_CONTEXT_LENGTH, + ) + + strict = os.environ.get("SKILLSPECTOR_STRICT_MODEL_VALIDATION", "").lower() == "true" + if strict and unknown: + raise ValueError( + "Strict model validation enabled. Unknown models:\n" + + "\n".join(unknown) + + "\nAdd them to model_registry.yaml or disable " + "SKILLSPECTOR_STRICT_MODEL_VALIDATION." + ) + + +_validate_model_config() # Log level: from env or fallback (DEBUG, INFO, WARNING, ERROR). SKILLSPECTOR_LOG_LEVEL = os.environ.get("SKILLSPECTOR_LOG_LEVEL", "WARNING") diff --git a/src/skillspector/graph.py b/src/skillspector/graph.py index 277f9634..e034ffe3 100644 --- a/src/skillspector/graph.py +++ b/src/skillspector/graph.py @@ -13,10 +13,11 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""LangGraph workflow for Skillspector stub analyzers.""" +"""LangGraph workflow wiring Skillspector's analyzer nodes.""" -# TODO(SADD A.2.1–A.2.4): Analyzer discovery, stage-as-category with meta last, wire registry; respect requires_api_key/is_available() and skip or warn when API key missing or analyzer unavailable. See SADD for skillspector § A.2. -# TODO(SADD A.5.1): Implement skillspector serve (FastAPI): POST /scan (zip), GET /results/{id}, GET /health. See SADD for skillspector § A.5.1. +# Roadmap: analyzer auto-discovery and stage-as-category ordering (meta_analyzer last); respect +# requires_api_key / is_available() so analyzers are skipped or warned when unavailable. +# Roadmap: optional `skillspector serve` HTTP API (POST /scan, GET /results/{id}, GET /health). from __future__ import annotations diff --git a/src/skillspector/input_handler.py b/src/skillspector/input_handler.py index e511281e..bc3d72e4 100644 --- a/src/skillspector/input_handler.py +++ b/src/skillspector/input_handler.py @@ -28,7 +28,10 @@ from __future__ import annotations +import ipaddress +import re import shutil +import socket import subprocess import tempfile import zipfile @@ -41,6 +44,42 @@ logger = get_logger(__name__) +ALLOWED_GIT_HOSTS = frozenset( + { + "github.com", + "gitlab.com", + "bitbucket.org", + } +) + +ALLOWED_DOWNLOAD_HOSTS = frozenset( + { + "github.com", + "raw.githubusercontent.com", + "gitlab.com", + "bitbucket.org", + "huggingface.co", + } +) + + +def _is_private_ip(host: str) -> bool: + """Return True if host resolves to a private/reserved IP address.""" + try: + addr = ipaddress.ip_address(host) + return addr.is_private or addr.is_loopback or addr.is_link_local or addr.is_reserved + except ValueError: + pass + try: + resolved = socket.getaddrinfo(host, None, socket.AF_UNSPEC, socket.SOCK_STREAM) + for _, _, _, _, sockaddr in resolved: + addr = ipaddress.ip_address(sockaddr[0]) + if addr.is_private or addr.is_loopback or addr.is_link_local or addr.is_reserved: + return True + except (socket.gaierror, OSError): + return True + return False + class InputHandler: """ @@ -107,8 +146,8 @@ def _is_git_url(self, path: str) -> bool: if not path.startswith(("http://", "https://", "git@")): return False parsed = urlparse(path) - git_hosts = ["github.com", "gitlab.com", "bitbucket.org"] - if any(host in parsed.netloc for host in git_hosts): + host = parsed.hostname or "" + if any(allowed in host for allowed in ALLOWED_GIT_HOSTS): if "/raw/" in path or "/blob/" in path or path.endswith((".md", ".py", ".sh")): return False return True @@ -122,8 +161,38 @@ def _is_file_url(self, path: str) -> bool: return False return not self._is_git_url(path) + def _extract_scp_host(self, url: str) -> str | None: + """Return the host from an scp-style Git URL, or None if not scp form.""" + if "://" in url: + return None + m = re.match(r"^[^@/]+@([^:/]+):.+$", url) + return m.group(1) if m else None + + def _validate_url_host(self, url: str, allowed_hosts: frozenset[str]) -> str: + """Validate URL host against allowlist and SSRF protections. + + Returns the hostname on success, raises ValueError on blocked URLs. + """ + parsed = urlparse(url) + host = parsed.hostname or "" + if not host: + host = self._extract_scp_host(url) or "" + if not host: + raise ValueError(f"URL has no valid hostname: {url}") + if not any(host == allowed or host.endswith("." + allowed) for allowed in allowed_hosts): + raise ValueError( + f"Host '{host}' is not in the allowed hosts list. Allowed: {sorted(allowed_hosts)}" + ) + if _is_private_ip(host): + raise ValueError( + f"URL resolves to a private/internal IP address: {url}. " + "This is blocked to prevent SSRF attacks." + ) + return host + def _clone_git(self, url: str) -> Path: """Clone a Git repository to a temporary directory.""" + self._validate_url_host(url, ALLOWED_GIT_HOSTS) temp_dir = self._get_temp_dir() clone_dir = temp_dir / "repo" try: @@ -149,11 +218,12 @@ def _clone_git(self, url: str) -> Path: def _download_file(self, url: str) -> Path: """Download a file from URL to a temporary directory.""" + self._validate_url_host(url, ALLOWED_DOWNLOAD_HOSTS) temp_dir = self._get_temp_dir() parsed = urlparse(url) filename = Path(parsed.path).name or "SKILL.md" try: - with httpx.Client(follow_redirects=True, timeout=30) as client: + with httpx.Client(follow_redirects=False, timeout=30) as client: response = client.get(url) response.raise_for_status() content = response.content @@ -171,7 +241,7 @@ def _download_file(self, url: str) -> Path: return temp_dir def _extract_zip(self, zip_path: Path) -> Path: - """Extract a zip file to a temporary directory.""" + """Extract a zip file to a temporary directory with path traversal protection.""" if not zip_path.exists(): raise FileNotFoundError(f"Zip file not found: {zip_path}") from None temp_dir = self._get_temp_dir() @@ -179,6 +249,13 @@ def _extract_zip(self, zip_path: Path) -> Path: extract_dir.mkdir(exist_ok=True) try: with zipfile.ZipFile(zip_path, "r") as zf: + for member in zf.namelist(): + member_path = (extract_dir / member).resolve() + if not str(member_path).startswith(str(extract_dir.resolve())): + raise ValueError( + f"Zip entry '{member}' would escape extraction directory (zip-slip). " + "Archive is potentially malicious." + ) zf.extractall(extract_dir) except zipfile.BadZipFile: logger.warning("Invalid zip or extract failed: %s", zip_path) diff --git a/src/skillspector/llm_analyzer_base.py b/src/skillspector/llm_analyzer_base.py index aa3e7e9a..c5ab9dce 100644 --- a/src/skillspector/llm_analyzer_base.py +++ b/src/skillspector/llm_analyzer_base.py @@ -33,7 +33,7 @@ from typing import Literal from langchain_core.messages import BaseMessage -from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, field_validator from skillspector.llm_utils import get_chat_model from skillspector.logging_config import get_logger @@ -62,12 +62,37 @@ class LLMFinding(BaseModel): rule_id: str = Field(description="Identifier for the type of finding") message: str = Field(description="Short description of the finding") severity: Literal["LOW", "MEDIUM", "HIGH", "CRITICAL"] = Field(description="Severity level") - start_line: int = Field(ge=1, description="Starting line number") + # start_line and confidence carry no ge/le Field bounds on purpose. Pydantic + # bounds emit JSON-schema minimum/maximum, which some OpenAI-compatible + # structured-output / tool-calling endpoints reject when they validate the + # response schema, failing the whole call. The ranges are enforced by the + # validators below instead, so the guarantee holds without those keywords in + # the emitted schema. start_line stays required (no default), so a finding + # with no location is still rejected rather than materialised at line 1; + # only the numeric bound is removed, not the requiredness. + start_line: int = Field(description="Starting line number (>= 1)") end_line: int | None = Field(default=None, description="Ending line number (optional)") - confidence: float = Field(ge=0.0, le=1.0, default=0.5, description="Confidence score") + confidence: float = Field(default=0.5, description="Confidence score between 0.0 and 1.0") explanation: str = Field(default="", description="Why this is a finding (2-3 sentences)") remediation: str = Field(default="", description="Actionable steps to fix the issue") + @field_validator("start_line") + @classmethod + def _clamp_start_line(cls, v: int) -> int: + # Clamp rather than raise: an LLM occasionally returns 0 for a + # whole-file finding, and normalising to the first line is better than + # dropping the finding over an off-by-one. + return v if v >= 1 else 1 + + @field_validator("confidence", mode="before") + @classmethod + def _normalize_confidence(cls, v: object) -> float: + # Accept 0-100 scale values from some models, then clamp into [0, 1]. + v = float(v) + if v > 2.0: + v = v / 100.0 + return min(1.0, max(0.0, v)) + def to_finding(self, file: str) -> Finding: """Convert to a :class:`Finding` for the graph state.""" return Finding( @@ -382,6 +407,14 @@ async def arun_batches( *max_concurrency* LLM requests in parallel. Both cross-file and cross-chunk batches are parallelized in a single gather call. + Failures are isolated per batch: a transient error (timeout, 429, + oversized-chunk 400, ...) costs only its own batch, which is logged + and omitted from the result, so one bad call cannot cancel the rest + of the fan-out. Callers can detect partial results by comparing the + returned batches against the submitted ones. ``ValueError`` and + ``NotImplementedError`` signal misconfiguration rather than infra + trouble and keep propagating. + The return type mirrors :meth:`run_batches`. """ sem = asyncio.Semaphore(max_concurrency) @@ -402,7 +435,16 @@ async def _process(batch: Batch) -> tuple[Batch, list]: logger.debug("LLM response for %s", batch.file_label) return (batch, self.parse_response(response, batch)) - return list(await asyncio.gather(*[_process(b) for b in batches])) + results = await asyncio.gather(*[_process(b) for b in batches], return_exceptions=True) + successful: list[tuple[Batch, list]] = [] + for batch, result in zip(batches, results, strict=True): + if isinstance(result, (ValueError, NotImplementedError)): + raise result + if isinstance(result, BaseException): + logger.warning("LLM batch failed for %s: %s", batch.file_label, result) + continue + successful.append(result) + return successful # -- Convenience -------------------------------------------------------- diff --git a/src/skillspector/llm_utils.py b/src/skillspector/llm_utils.py index ab6e5518..468e26b0 100644 --- a/src/skillspector/llm_utils.py +++ b/src/skillspector/llm_utils.py @@ -13,13 +13,17 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""Shared LLM utilities. +"""Shared LLM utilities (OpenAI-compatible chat models + agent CLI transports). Credentials are resolved in this order: - 1. The active SkillSpector provider (see :mod:`skillspector.providers`) — - reads its own credential env var and supplies the matching client. + 1. The active provider (see :mod:`skillspector.providers`): + - CLI providers (``claude_cli``, ``codex_cli``, ``gemini_cli``): use + ``is_available()`` and ``complete()`` — no API key needed. + - HTTP providers (``anthropic``, ``openai``, ``nv_build``): read their + respective credential env vars and supply a base URL. 2. ``OPENAI_API_KEY`` / ``OPENAI_BASE_URL`` (the langchain-openai - defaults). + defaults) — only consulted for HTTP providers when the provider's + own credential env var is unset. There is no SkillSpector-specific credential env var: setting ``NVIDIA_INFERENCE_KEY`` configures whichever NVIDIA endpoint the @@ -30,23 +34,31 @@ from __future__ import annotations +import asyncio +import json +from typing import NoReturn + from langchain_core.language_models.chat_models import BaseChatModel -from langchain_core.messages import BaseMessage -from skillspector.constants import MODEL_CONFIG from skillspector.model_info import get_max_input_tokens, get_max_output_tokens from skillspector.providers import ( create_chat_model, + get_active_provider, + get_metadata_provider, + has_cli_capability, raise_no_llm_api_key_configured, resolve_chat_model_credentials, + resolve_provider_credentials, ) +from skillspector.providers.openai import OpenAIProvider def _resolve_llm_credentials() -> tuple[str, str | None]: """Return ``(api_key, base_url)`` resolved from the environment. - Tries the active NVIDIA provider first; falls back to ``OPENAI_API_KEY`` - / ``OPENAI_BASE_URL`` when the provider is not configured. + Tries the active SkillSpector provider first; falls back to + ``OPENAI_API_KEY`` / ``OPENAI_BASE_URL`` when the provider is not + configured. Raises: ValueError: when no API key can be resolved from any source. @@ -57,8 +69,28 @@ def _resolve_llm_credentials() -> tuple[str, str | None]: return creds +def _resolve_default_chat_model() -> str: + """Return the default chat model for the endpoint that will be used.""" + if resolve_provider_credentials() is not None: + return get_metadata_provider().resolve_model() + + openai_provider = OpenAIProvider() + if openai_provider.resolve_credentials() is not None: + return openai_provider.resolve_model() + + raise_no_llm_api_key_configured() + + def is_llm_available() -> tuple[bool, str | None]: - """Return ``(available, error_message)`` describing LLM credential status.""" + """Return ``(available, error_message)`` describing LLM availability. + + For CLI providers (``claude_cli``, ``codex_cli``, ``gemini_cli``) the check + delegates to the provider's ``is_available()`` method (binary on PATH + + auth). For HTTP providers, it falls back to credential resolution. + """ + provider = get_active_provider() + if has_cli_capability(provider): + return provider.is_available() # type: ignore[attr-defined] try: _resolve_llm_credentials() except ValueError as exc: @@ -71,13 +103,158 @@ def fetch_model_token_limits(model_label: str) -> tuple[int, int]: return get_max_input_tokens(model_label), get_max_output_tokens(model_label) -def get_chat_model(model: str | None = None) -> BaseChatModel: - """Return the active provider's native LangChain chat model. +# --------------------------------------------------------------------------- +# Agent CLI chat-model adapter +# --------------------------------------------------------------------------- +# +# The LLM analyzers (meta_analyzer, semantic_*) obtain a model from +# ``get_chat_model()`` and call ``.invoke()`` / ``.with_structured_output( +# schema).invoke()`` on it (see ``llm_analyzer_base``) — they never go through +# ``chat_completion``. To support CLI providers there, ``get_chat_model`` +# returns this minimal adapter, which mimics the slice of the ``ChatOpenAI`` +# interface the analyzers rely on, backed by the provider's ``complete()`` +# subprocess transport. + + +class _AgentCLIMessage: + """Minimal stand-in for a LangChain message: exposes ``.content``.""" + + def __init__(self, content: str) -> None: + self.content = content + + +def _extract_json_object(raw: str) -> dict: + """Extract a single JSON object from a CLI model's text response. + + Tolerates markdown code fences and surrounding prose. Raises ``ValueError`` + (fail-closed) when no JSON object can be parsed. + """ + text = raw.strip() + if text.startswith("```"): + # Drop the opening fence line (``` or ```json) and any closing fence. + text = text.split("\n", 1)[1] if "\n" in text else "" + fence = text.rfind("```") + if fence != -1: + text = text[:fence] + text = text.strip() + try: + obj = json.loads(text) + if isinstance(obj, dict): + return obj + except json.JSONDecodeError: + pass + start, end = text.find("{"), text.rfind("}") + if start != -1 and end > start: + try: + obj = json.loads(text[start : end + 1]) + if isinstance(obj, dict): + return obj + except json.JSONDecodeError: + pass + raise ValueError(f"could not extract a JSON object from CLI response: {raw[:200]!r}") + + +class _StructuredAgentCLIModel: + """Mimics ``ChatOpenAI.with_structured_output(schema)`` for a CLI provider. + + ``invoke`` augments the prompt with the schema, calls the provider's + ``complete()``, then parses and validates the response into *schema*. + """ + + def __init__(self, provider: object, model: str, max_output_tokens: int, schema: type) -> None: + self._provider = provider + self._model = model + self._max_output_tokens = max_output_tokens + self._schema = schema + + def _augment(self, prompt: str) -> str: + schema_json = json.dumps(self._schema.model_json_schema(), indent=2) + return ( + f"{prompt}\n\n" + "Respond with ONLY a single JSON object conforming to the JSON Schema " + "below. Do not wrap it in markdown code fences and do not add any prose " + f"before or after the JSON.\n\nJSON Schema:\n{schema_json}" + ) + + def invoke(self, prompt: str) -> object: + raw = self._provider.complete( # type: ignore[attr-defined] + self._augment(prompt), + model=self._model, + max_output_tokens=self._max_output_tokens, + ) + return self._schema.model_validate(_extract_json_object(raw)) + + async def ainvoke(self, prompt: str) -> object: + return await asyncio.to_thread(self.invoke, prompt) + + +class AgentCLIChatModel: + """Minimal ``ChatOpenAI``-compatible adapter backed by a CLI provider. + + Implements only the surface the analyzers use: ``invoke`` (returns an + object with ``.content``), ``ainvoke``, and ``with_structured_output``. + The rest of the ``BaseChatModel`` surface (``batch``, ``stream``, + callbacks) is intentionally unsupported; the stubs below make that boundary + explicit so a future analyzer reaching for it fails loudly with a clear + message rather than a confusing ``AttributeError``. + """ + + def __init__(self, provider: object, model: str, max_output_tokens: int) -> None: + self._provider = provider + self._model = model + self._max_output_tokens = max_output_tokens + + def batch(self, *args: object, **kwargs: object) -> NoReturn: + raise NotImplementedError( + "AgentCLIChatModel supports only invoke/ainvoke/with_structured_output; " + "batch() is not available for CLI providers." + ) + + def stream(self, *args: object, **kwargs: object) -> NoReturn: + raise NotImplementedError( + "AgentCLIChatModel supports only invoke/ainvoke/with_structured_output; " + "stream() is not available for CLI providers." + ) + + def invoke(self, prompt: str) -> _AgentCLIMessage: + text = self._provider.complete( # type: ignore[attr-defined] + prompt, + model=self._model, + max_output_tokens=self._max_output_tokens, + ) + return _AgentCLIMessage(text) + + async def ainvoke(self, prompt: str) -> _AgentCLIMessage: + return await asyncio.to_thread(self.invoke, prompt) + + def with_structured_output(self, schema: type) -> _StructuredAgentCLIModel: + return _StructuredAgentCLIModel( + self._provider, self._model, self._max_output_tokens, schema + ) + + +def get_chat_model(model: str | None = None) -> BaseChatModel | AgentCLIChatModel: + """Return a chat model for the active provider. + + For CLI providers (``claude_cli``, ``codex_cli``, ``gemini_cli``) this + returns an :class:`AgentCLIChatModel` adapter backed by the provider's + ``complete()`` subprocess transport — so the LLM analyzers (which use + ``.invoke()`` and ``.with_structured_output()``) work with no API key. + + For HTTP providers it delegates to + :func:`skillspector.providers.create_chat_model`, which uses the + provider's own native client (e.g. ``ChatAnthropic`` for Anthropic) with + an ``OPENAI_API_KEY`` / ``ChatOpenAI`` fallback. Raises: - ValueError: when no API key is configured (see ``is_llm_available``). + ValueError: when an HTTP provider has no API key configured. """ - model = model or MODEL_CONFIG["default"] + provider = get_active_provider() + if has_cli_capability(provider): + resolved_model = model or provider.resolve_model() + return AgentCLIChatModel(provider, resolved_model, get_max_output_tokens(resolved_model)) + + model = model or _resolve_default_chat_model() return create_chat_model( model=model, max_tokens=get_max_output_tokens(model), @@ -86,9 +263,16 @@ def get_chat_model(model: str | None = None) -> BaseChatModel: def chat_completion(prompt: str, *, model: str | None = None) -> str: - """Request a single chat completion and return the assistant text.""" - llm = get_chat_model(model=model) - response = llm.invoke(prompt) - if not isinstance(response, BaseMessage): - raise TypeError(f"Expected BaseMessage from chat model, got {type(response).__name__}") - return str(response.text) + """Request a single chat completion and return the assistant content. + + Routes through :func:`get_chat_model`, which dispatches to the CLI adapter + for CLI providers and to the provider's native chat model for HTTP providers. + + Uses ``.text`` when available (real LangChain ``BaseMessage`` objects, + which normalise content blocks to a single string) and falls back to + ``.content`` for the CLI adapter's ``_AgentCLIMessage``. + """ + response = get_chat_model(model=model).invoke(prompt) + if hasattr(response, "text"): + return response.text # type: ignore[union-attr] + return response.content or "" # type: ignore[union-attr] diff --git a/src/skillspector/mcp_server.py b/src/skillspector/mcp_server.py new file mode 100644 index 00000000..444b75fc --- /dev/null +++ b/src/skillspector/mcp_server.py @@ -0,0 +1,182 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""MCP server exposing SkillSpector scanning as an agent-callable tool. + +This lets any MCP-capable agent (Claude Code, Codex CLI, Gemini CLI) or remote +runtime call ``scan_skill`` and gate skill/MCP installs on the verdict, turning +SkillSpector from an out-of-band audit tool into a runtime guardrail. + +The scan core (:func:`run_scan`) is deliberately independent of the ``mcp`` SDK +so it can be unit-tested without the optional dependency; :func:`build_server` +wraps it in a FastMCP tool and is only reachable once ``skillspector[mcp]`` is +installed. +""" + +from __future__ import annotations + +import shutil +from typing import TYPE_CHECKING, Any + +from skillspector import __version__ +from skillspector.graph import graph +from skillspector.logging_config import get_logger +from skillspector.providers import resolve_provider_credentials + +if TYPE_CHECKING: + from mcp.server.fastmcp import FastMCP + +logger = get_logger(__name__) + +VALID_FORMATS = ("json", "markdown", "sarif", "terminal") + +# Mirrors the CLI: a scan scoring above this is treated as unsafe (CLI exits 1). +RISK_THRESHOLD = 50 + + +async def run_scan( + target: str, + *, + use_llm: bool = True, + output_format: str = "json", + yara_rules_dir: str | None = None, +) -> dict[str, Any]: + """Invoke the SkillSpector graph and return a structured verdict. + + Args: + target: Git URL, file URL, ``.zip``, ``.md`` file, or local directory. + use_llm: Whether to request the optional LLM semantic pass on top of + static analysis. Honoured only when provider credentials resolve; + the returned payload reports what actually happened. + output_format: Format of the embedded ``report`` string. One of + :data:`VALID_FORMATS`. + yara_rules_dir: Optional directory of additional YARA rules. + + Returns: + A JSON-serialisable verdict with ``risk_score`` (0-100), ``severity``, + ``recommendation``, ``safe_to_install``, ``findings``, the rendered + ``report``, and an honest LLM accounting (``llm_requested``, + ``llm_available``, ``llm_used``, ``scan_mode``) so a caller is never + misled into thinking a full semantic scan ran when it silently did not. + """ + if output_format not in VALID_FORMATS: + raise ValueError(f"output_format must be one of {VALID_FORMATS}, got {output_format!r}") + + llm_available = resolve_provider_credentials() is not None + llm_used = use_llm and llm_available + + state: dict[str, Any] = { + "input_path": target, + "output_format": output_format, + "use_llm": llm_used, + } + if yara_rules_dir: + state["yara_rules_dir"] = yara_rules_dir + + logger.debug( + "MCP scan started: target=%s, format=%s, llm_used=%s", + target, + output_format, + llm_used, + ) + + result: dict[str, Any] | None = None + try: + result = await graph.ainvoke( + state, + config={ + "run_name": "skillspector-mcp-scan", + "tags": ["skillspector", "mcp"], + "metadata": { + "input_path": target, + "use_llm": llm_used, + "output_format": output_format, + "version": __version__, + }, + }, + ) + findings = result.get("filtered_findings") or result.get("findings") or [] + risk_score = int(result.get("risk_score") or 0) + return { + "target": target, + "risk_score": risk_score, + "severity": result.get("risk_severity"), + "recommendation": result.get("risk_recommendation"), + "safe_to_install": risk_score <= RISK_THRESHOLD, + "findings": [f.to_dict() for f in findings], + "report": result.get("report_body") or "", + # Honest LLM accounting — never silently imply a full semantic scan. + "llm_requested": use_llm, + "llm_available": llm_available, + "llm_used": llm_used, + "scan_mode": "static+llm" if llm_used else "static-only", + "version": __version__, + } + finally: + if result is not None: + temp_dir = result.get("temp_dir_for_cleanup") + if temp_dir and isinstance(temp_dir, str): + shutil.rmtree(temp_dir, ignore_errors=True) + + +def build_server(name: str = "skillspector") -> FastMCP: + """Construct the FastMCP server exposing the ``scan_skill`` tool. + + Requires the optional ``mcp`` dependency (``pip install 'skillspector[mcp]'``). + """ + try: + from mcp.server.fastmcp import FastMCP + except ModuleNotFoundError as exc: + raise ModuleNotFoundError( + "The MCP server requires the optional 'mcp' dependency. " + "Install it with: pip install 'skillspector[mcp]'" + ) from exc + + server = FastMCP(name) + + @server.tool() + async def scan_skill( + target: str, + use_llm: bool = True, + output_format: str = "json", + ) -> dict[str, Any]: + """Scan an AI agent skill for security risks before installing it. + + Use this before installing or loading any skill or MCP server to decide + whether it is safe. ``target`` accepts a Git URL, file URL, ``.zip``, + ``.md`` file, or local directory. + + Returns a verdict with ``risk_score`` (0-100), ``severity``, + ``recommendation``, ``safe_to_install``, and ``findings``. The + ``llm_used`` / ``scan_mode`` fields report whether the semantic LLM pass + actually ran, so a low score from a static-only scan is not mistaken for + a clean full scan. + """ + return await run_scan(target, use_llm=use_llm, output_format=output_format) + + return server + + +def run(transport: str = "stdio", host: str = "127.0.0.1", port: int = 8000) -> None: + """Run the MCP server over ``stdio`` (local agents) or ``http`` (remote/A2A).""" + server = build_server() + if transport == "stdio": + server.run(transport="stdio") + elif transport == "http": + server.settings.host = host + server.settings.port = port + server.run(transport="streamable-http") + else: + raise ValueError(f"transport must be 'stdio' or 'http', got {transport!r}") diff --git a/src/skillspector/models.py b/src/skillspector/models.py index a26a78be..6a9edfa0 100644 --- a/src/skillspector/models.py +++ b/src/skillspector/models.py @@ -101,6 +101,9 @@ def to_dict(self) -> dict[str, object]: "remediation": self.remediation, "code_snippet": self.code_snippet or self.context, "intent": self.intent, + # Tags surface markers like "llm-unconfirmed" (a high-severity static + # finding the LLM filter did not confirm but which is preserved anyway). + "tags": list(self.tags), } def __str__(self) -> str: @@ -108,7 +111,7 @@ def __str__(self) -> str: class AnalyzerPlugin(Protocol): - """Analyzer protocol from SADD A.1.1.""" + """Analyzer plugin protocol: name/stage/availability and an ``analyze`` entry point.""" name: str stage: str diff --git a/src/skillspector/multi_skill.py b/src/skillspector/multi_skill.py new file mode 100644 index 00000000..be4c7eba --- /dev/null +++ b/src/skillspector/multi_skill.py @@ -0,0 +1,129 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Multi-skill directory detection and per-skill scanning. + +Detects when a scanned directory contains multiple independent skills +(each with their own SKILL.md) and supports scanning each independently +to produce per-skill reports instead of one inflated monolithic result. +""" + +from __future__ import annotations + +from dataclasses import dataclass, field +from pathlib import Path + +from skillspector.logging_config import get_logger + +logger = get_logger(__name__) + + +@dataclass +class SkillDirectory: + """A detected skill within a multi-skill directory.""" + + path: Path + name: str + relative_path: str + + +@dataclass +class MultiSkillDetectionResult: + """Result of scanning a directory for multiple skills.""" + + is_multi_skill: bool + skills: list[SkillDirectory] = field(default_factory=list) + has_root_skill: bool = False + + +def detect_skills(directory: Path) -> MultiSkillDetectionResult: + """Detect whether a directory contains multiple independent skills. + + A directory is considered multi-skill when: + - It has NO root-level SKILL.md (or skill.md) + - At least 2 immediate subdirectories contain SKILL.md (or skill.md) + + If a root SKILL.md exists, the directory is treated as a single skill + (the standard behavior) regardless of nested SKILL.md files. + + Returns a MultiSkillDetectionResult with detected skills. + """ + if not directory.is_dir(): + return MultiSkillDetectionResult(is_multi_skill=False) + + has_root = _has_skill_md(directory) + if has_root: + return MultiSkillDetectionResult(is_multi_skill=False, has_root_skill=True) + + skills: list[SkillDirectory] = [] + for child in sorted(directory.iterdir()): + if not child.is_dir(): + continue + if child.name.startswith("."): + continue + if _has_skill_md(child): + name = _extract_skill_name(child) + skills.append( + SkillDirectory( + path=child, + name=name, + relative_path=child.name, + ) + ) + + is_multi = len(skills) >= 2 + return MultiSkillDetectionResult( + is_multi_skill=is_multi, + skills=skills, + has_root_skill=False, + ) + + +def _has_skill_md(directory: Path) -> bool: + """Check if directory contains a SKILL.md or skill.md at root level.""" + return (directory / "SKILL.md").is_file() or (directory / "skill.md").is_file() + + +def _extract_skill_name(skill_dir: Path) -> str: + """Extract skill name from SKILL.md frontmatter, falling back to directory name.""" + import re + + import yaml + + for name in ("SKILL.md", "skill.md"): + path = skill_dir / name + if not path.is_file(): + continue + try: + content = path.read_text(encoding="utf-8", errors="replace") + except OSError: + continue + if not content.startswith("---"): + break + end_match = re.search(r"\n---\s*\n", content[3:]) + if not end_match: + break + frontmatter = content[3 : end_match.start() + 3] + try: + # WARNING: Do not change this to yaml.load() without an explicit Loader. + # yaml.safe_load() is used intentionally to avoid arbitrary code execution. + data = yaml.safe_load(frontmatter) + except yaml.YAMLError: + break + if isinstance(data, dict) and "name" in data: + return str(data["name"]) + break + + return skill_dir.name diff --git a/src/skillspector/nodes/analyzers/__init__.py b/src/skillspector/nodes/analyzers/__init__.py index 58b3e934..b2ef9bcf 100644 --- a/src/skillspector/nodes/analyzers/__init__.py +++ b/src/skillspector/nodes/analyzers/__init__.py @@ -13,7 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""Analyzer node registry for Skillspector v2 stub workflow.""" +"""Analyzer node registry for the Skillspector workflow.""" from __future__ import annotations @@ -33,6 +33,12 @@ from skillspector.nodes.analyzers.semantic_security_discovery import ( node as semantic_security_discovery_node, ) +from skillspector.nodes.analyzers.static_patterns_agent_snooping import ( + node as static_patterns_agent_snooping_node, +) +from skillspector.nodes.analyzers.static_patterns_anti_refusal import ( + node as static_patterns_anti_refusal_node, +) from skillspector.nodes.analyzers.static_patterns_data_exfiltration import ( node as static_patterns_data_exfiltration_node, ) @@ -57,6 +63,9 @@ from skillspector.nodes.analyzers.static_patterns_rogue_agent import ( node as static_patterns_rogue_agent_node, ) +from skillspector.nodes.analyzers.static_patterns_ssrf import ( + node as static_patterns_ssrf_node, +) from skillspector.nodes.analyzers.static_patterns_supply_chain import ( node as static_patterns_supply_chain_node, ) @@ -80,6 +89,9 @@ "static_patterns_memory_poisoning", "static_patterns_tool_misuse", "static_patterns_rogue_agent", + "static_patterns_agent_snooping", + "static_patterns_anti_refusal", + "static_patterns_ssrf", "static_yara", "behavioral_ast", "behavioral_taint_tracking", @@ -103,6 +115,9 @@ "static_patterns_memory_poisoning": static_patterns_memory_poisoning_node, "static_patterns_tool_misuse": static_patterns_tool_misuse_node, "static_patterns_rogue_agent": static_patterns_rogue_agent_node, + "static_patterns_agent_snooping": static_patterns_agent_snooping_node, + "static_patterns_anti_refusal": static_patterns_anti_refusal_node, + "static_patterns_ssrf": static_patterns_ssrf_node, "static_yara": static_yara_node, "behavioral_ast": behavioral_ast_node, "behavioral_taint_tracking": behavioral_taint_tracking_node, diff --git a/src/skillspector/nodes/analyzers/behavioral_ast.py b/src/skillspector/nodes/analyzers/behavioral_ast.py index f0b7a189..e571c57a 100644 --- a/src/skillspector/nodes/analyzers/behavioral_ast.py +++ b/src/skillspector/nodes/analyzers/behavioral_ast.py @@ -23,7 +23,13 @@ from skillspector.models import AnalyzerFinding, Finding, Location, Severity from skillspector.state import AnalyzerNodeResponse, SkillspectorState -from .common import get_context_from_lines, get_source_segment, resolve_call_name +from .common import ( + build_import_aliases, + get_context_from_lines, + get_source_segment, + resolve_call_name, + resolve_dynamic_import_call, +) from .static_runner import MAX_FILE_BYTES, analyzer_finding_to_finding ANALYZER_ID = "behavioral_ast" @@ -31,6 +37,16 @@ _DANGEROUS_BUILTINS = frozenset({"exec", "eval", "compile", "__import__"}) +# Names that turn ``getattr(obj, "")`` into a reflective handle on a code- or +# command-execution sink. ``getattr(os, "system")(cmd)`` and +# ``getattr(builtins, "exec")(src)`` are functionally identical to ``os.system(cmd)`` +# / ``exec(src)`` but evade AST1/AST5: the inner ``getattr`` has a *constant* second +# argument (so AST7 is intentionally skipped), and the outer call's ``func`` is an +# ``ast.Call`` whose name does not resolve, so AST1/AST5 never fire. The set is kept +# deliberately small — only names with essentially no legitimate ``getattr`` use — so +# benign reflection such as ``getattr(obj, "name")`` stays unflagged. +_DANGEROUS_GETATTR_NAMES = frozenset({"exec", "eval", "system", "popen", "__import__"}) + _SUBPROCESS_CALLS = frozenset( { "call", @@ -77,6 +93,7 @@ "AST6": "compile() call detected", "AST7": "Dynamic attribute access via getattr()", "AST8": "Dangerous execution chain", + "AST9": "Reflective dangerous call via getattr() with a literal sink name", } _RULE_SEVERITIES: dict[str, Severity] = { @@ -88,6 +105,7 @@ "AST6": Severity.MEDIUM, "AST7": Severity.LOW, "AST8": Severity.CRITICAL, + "AST9": Severity.HIGH, } _RULE_CONFIDENCES: dict[str, float] = { @@ -99,23 +117,24 @@ "AST6": 0.65, "AST7": 0.50, "AST8": 0.95, + "AST9": 0.85, } _TAG = "Dangerous Code Execution" -def _is_chain_sink(node: ast.Call) -> bool: +def _is_chain_sink(node: ast.Call, aliases: dict[str, str] | None = None) -> bool: """True if this call is exec(), eval(), or compile() — the outer dangerous call.""" - name = resolve_call_name(node) + name = resolve_call_name(node, aliases) return name in ("exec", "eval", "compile") -def _contains_dangerous_source(node: ast.AST) -> str | None: +def _contains_dangerous_source(node: ast.AST, aliases: dict[str, str] | None = None) -> str | None: """Walk children to find a nested dangerous call that forms a chain.""" for child in ast.walk(node): if not isinstance(child, ast.Call): continue - name = resolve_call_name(child) + name = resolve_call_name(child, aliases) if name is None: continue if name in ("compile", "__import__"): @@ -136,6 +155,7 @@ def _analyze_python(content: str, file_path: str) -> list[AnalyzerFinding]: logger.debug("SyntaxError parsing %s, skipping", file_path) return [] + aliases = build_import_aliases(tree) lines = content.splitlines() findings: list[AnalyzerFinding] = [] @@ -162,7 +182,11 @@ def _emit( if not isinstance(ast_node, ast.Call): continue - call_name = resolve_call_name(ast_node) + call_name = resolve_call_name(ast_node, aliases) + if call_name is None: + # Dynamic-import chain: importlib.import_module('os').system(...) → + # 'os.system', so it re-enters the os./subprocess. sink ladders below. + call_name = resolve_dynamic_import_call(ast_node, aliases) if call_name is None: continue @@ -170,15 +194,15 @@ def _emit( end_lineno = getattr(ast_node, "end_lineno", None) if call_name == "exec": - if _is_chain_sink(ast_node) and ast_node.args: - source = _contains_dangerous_source(ast_node.args[0]) + if _is_chain_sink(ast_node, aliases) and ast_node.args: + source = _contains_dangerous_source(ast_node.args[0], aliases) if source: _emit("AST8", lineno, end_lineno, f"Dangerous chain: exec() wrapping {source}") _emit("AST1", lineno, end_lineno) elif call_name == "eval": - if _is_chain_sink(ast_node) and ast_node.args: - source = _contains_dangerous_source(ast_node.args[0]) + if _is_chain_sink(ast_node, aliases) and ast_node.args: + source = _contains_dangerous_source(ast_node.args[0], aliases) if source: _emit("AST8", lineno, end_lineno, f"Dangerous chain: eval() wrapping {source}") _emit("AST2", lineno, end_lineno) @@ -200,8 +224,11 @@ def _emit( _emit("AST5", lineno, end_lineno) elif call_name == "getattr" and len(ast_node.args) >= 2: - if not isinstance(ast_node.args[1], ast.Constant): + second_arg = ast_node.args[1] + if not isinstance(second_arg, ast.Constant): _emit("AST7", lineno, end_lineno) + elif isinstance(second_arg.value, str) and second_arg.value in _DANGEROUS_GETATTR_NAMES: + _emit("AST9", lineno, end_lineno) return findings diff --git a/src/skillspector/nodes/analyzers/behavioral_taint_tracking.py b/src/skillspector/nodes/analyzers/behavioral_taint_tracking.py index 90c7e248..f6141337 100644 --- a/src/skillspector/nodes/analyzers/behavioral_taint_tracking.py +++ b/src/skillspector/nodes/analyzers/behavioral_taint_tracking.py @@ -13,7 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""Behavioral taint-tracking analyzer (SADD B.2.2): sources -> sinks data-flow analysis. +"""Behavioral taint-tracking analyzer (TT1–TT5): sources -> sinks data-flow analysis. Parses Python AST to identify data sources (env vars, file reads, network input) and sinks (network output, exec, file writes), then tracks flows between them @@ -30,11 +30,14 @@ from skillspector.state import AnalyzerNodeResponse, SkillspectorState from .common import ( + apply_import_aliases, + build_import_aliases, build_type_map, get_context_from_lines, get_source_segment, resolve_call_name_typed, resolve_dotted_name, + resolve_dynamic_import_call, ) from .static_runner import MAX_FILE_BYTES, analyzer_finding_to_finding @@ -168,6 +171,24 @@ ] +def _resolve_sink_name( + node: ast.Call, + type_map: dict[str, str] | None = None, + aliases: dict[str, str] | None = None, +) -> str | None: + """Resolve a call to its canonical sink name, including dynamic-import chains. + + Wraps :func:`resolve_call_name_typed` (type-/alias-aware resolution) and falls back + to :func:`resolve_dynamic_import_call` so that + ``importlib.import_module('subprocess').run(...)`` resolves to ``'subprocess.run'`` + and re-enters ``_EXEC_SINKS`` like the statically-imported form would. + """ + name = resolve_call_name_typed(node, type_map, aliases) + if name is None: + name = resolve_dynamic_import_call(node, aliases) + return name + + def _classify(name: str, categories: list[tuple[frozenset[str], str]], default: str) -> str: for names, label in categories: if name in names: @@ -204,7 +225,11 @@ def _is_open_for_write(node: ast.Call) -> bool: return False -def _find_source_in_expr(node: ast.expr, type_map: dict[str, str] | None = None) -> str | None: +def _find_source_in_expr( + node: ast.expr, + type_map: dict[str, str] | None = None, + aliases: dict[str, str] | None = None, +) -> str | None: """Find a source call anywhere in an expression tree (handles chained calls). Handles patterns like ``open("f").read()``, ``requests.get(url).text``, @@ -213,7 +238,7 @@ def _find_source_in_expr(node: ast.expr, type_map: dict[str, str] | None = None) for child in ast.walk(node): if not isinstance(child, ast.Call): continue - name = resolve_call_name_typed(child, type_map) + name = resolve_call_name_typed(child, type_map, aliases) if name is None or name not in _ALL_SOURCES: continue if name == "open" and _is_open_for_write(child): @@ -223,7 +248,9 @@ def _find_source_in_expr(node: ast.expr, type_map: dict[str, str] | None = None) def _find_nested_sources( - node: ast.Call, type_map: dict[str, str] | None = None + node: ast.Call, + type_map: dict[str, str] | None = None, + aliases: dict[str, str] | None = None, ) -> list[tuple[str, ast.Call]]: """Walk children to find source calls nested inside a sink call.""" results: list[tuple[str, ast.Call]] = [] @@ -232,7 +259,7 @@ def _find_nested_sources( continue if not isinstance(child, ast.Call): continue - name = resolve_call_name_typed(child, type_map) + name = resolve_call_name_typed(child, type_map, aliases) if name and name in _ALL_SOURCES: results.append((name, child)) return results @@ -298,6 +325,7 @@ def _analyze_python(content: str, file_path: str) -> list[AnalyzerFinding]: return [] type_map = build_type_map(tree) + aliases = build_import_aliases(tree) lines = content.splitlines() findings: list[AnalyzerFinding] = [] tainted: dict[str, _TaintedVar] = {} @@ -329,11 +357,13 @@ def _emit( for ast_node in ast.walk(tree): # Record tainted assignments. if isinstance(ast_node, ast.Assign): - src_name = _find_source_in_expr(ast_node.value, type_map) + src_name = _find_source_in_expr(ast_node.value, type_map, aliases) - # Subscript sources like os.environ["KEY"] + # Subscript sources like os.environ["KEY"] (also os aliased as `o`) if src_name is None and isinstance(ast_node.value, ast.Subscript): base = resolve_dotted_name(ast_node.value.value) + if base is not None: + base = apply_import_aliases(base, aliases) if base and base in _CREDENTIAL_SOURCES: src_name = base @@ -352,7 +382,7 @@ def _emit( if not isinstance(ast_node, ast.Call): continue - sink_name = resolve_call_name_typed(ast_node, type_map) + sink_name = _resolve_sink_name(ast_node, type_map, aliases) if not sink_name or sink_name not in _ALL_SINKS: continue @@ -362,7 +392,7 @@ def _emit( lineno = getattr(ast_node, "lineno", 1) end_lineno = getattr(ast_node, "end_lineno", None) - for src_name, src_node in _find_nested_sources(ast_node, type_map): + for src_name, src_node in _find_nested_sources(ast_node, type_map, aliases): if src_name == "open" and _is_open_for_write(src_node): continue rule = _pick_rule(src_name, sink_name, is_direct=True) diff --git a/src/skillspector/nodes/analyzers/common.py b/src/skillspector/nodes/analyzers/common.py index 81829927..22bde49c 100644 --- a/src/skillspector/nodes/analyzers/common.py +++ b/src/skillspector/nodes/analyzers/common.py @@ -104,9 +104,105 @@ def resolve_dotted_name(node: ast.expr) -> str | None: return None -def resolve_call_name(node: ast.Call) -> str | None: - """Extract a dotted call name like ``'os.system'`` from a Call node.""" - return resolve_dotted_name(node.func) +def _strip_builtins_prefix(name: str) -> str: + """Collapse a ``builtins``-qualified name back to its bare builtin name. + + ``builtins.exec`` → ``exec`` (and ``builtins.eval``/``compile``/``__import__``…). + The analyzers match dangerous builtins by their bare name (``call_name == "exec"``, + ``name in _EXEC_SINKS``), but ``from builtins import exec`` / ``import builtins; + builtins.exec(...)`` resolve, through the import-alias map, to the *qualified* + spelling ``builtins.exec`` — which would otherwise slip past those checks. Since + ``builtins.exec is exec`` at runtime, collapsing the prefix is semantically exact + and re-enters the existing bare-name detection. + + Only the single-segment form ``builtins.`` is collapsed; deeper chains + (``builtins.foo.bar``) are left untouched as they are not direct builtin calls. + """ + root, sep, rest = name.partition(".") + if root == "builtins" and sep and "." not in rest: + return rest + return name + + +def apply_import_aliases(name: str, aliases: dict[str, str]) -> str: + """Rewrite a resolved call name to its fully-qualified form using import aliases. + + Bridges several evasion-prone spellings back to the canonical name that the + analyzers match against: + + - ``from os import system`` → ``{"system": "os.system"}`` so a bare ``system`` + call resolves to ``"os.system"``. + - ``import os as o`` → ``{"o": "os"}`` so ``o.system`` resolves to ``"os.system"``. + - ``from builtins import exec`` / ``import builtins; builtins.exec(...)`` → the + bare builtin ``exec`` (via :func:`_strip_builtins_prefix`), so dangerous + builtins matched by bare name are not hidden behind a ``builtins.`` qualifier. + + Idempotent for already-canonical names (``os.system`` stays ``os.system``). + """ + if name in aliases: + return _strip_builtins_prefix(aliases[name]) + root, sep, rest = name.partition(".") + if sep and root in aliases: + return _strip_builtins_prefix(f"{aliases[root]}.{rest}") + return _strip_builtins_prefix(name) + + +def resolve_call_name(node: ast.Call, aliases: dict[str, str] | None = None) -> str | None: + """Extract a dotted call name like ``'os.system'`` from a Call node. + + When *aliases* (from :func:`build_import_aliases`) is supplied, locally aliased or + ``from``-imported names are normalized to their fully-qualified form so that + ``import os as o; o.system(...)`` and ``from os import system; system(...)`` both + resolve to ``"os.system"``. + """ + name = resolve_dotted_name(node.func) + if name is not None and aliases: + name = apply_import_aliases(name, aliases) + return name + + +def _dynamic_import_target(node: ast.expr, aliases: dict[str, str] | None = None) -> str | None: + """Return the imported module name for an ``importlib.import_module('mod')`` call. + + Recognizes both ``importlib.import_module('os')`` and the bare-imported + ``from importlib import import_module; import_module('os')`` (resolved via the + import-alias map), returning the string literal module name (``'os'``) when the + first positional argument is a constant. Returns ``None`` for anything else + (non-literal argument, unrelated call), so callers stay precise and avoid false + positives on dynamic module names the analyzer cannot resolve statically. + """ + if not isinstance(node, ast.Call): + return None + func_name = resolve_dotted_name(node.func) + if func_name is not None and aliases: + func_name = apply_import_aliases(func_name, aliases) + if func_name not in ("importlib.import_module", "import_module"): + return None + if node.args and isinstance(node.args[0], ast.Constant) and isinstance(node.args[0].value, str): + return node.args[0].value + return None + + +def resolve_dynamic_import_call( + node: ast.Call, aliases: dict[str, str] | None = None +) -> str | None: + """Resolve ``importlib.import_module('mod').attr(...)`` to the dotted sink ``'mod.attr'``. + + Bridges the dynamic-import evasion that mirrors ``__import__``: a skill writes + ``importlib.import_module('os').system(cmd)`` (or imports ``import_module`` bare) + so the dangerous module never appears as a static ``import``. When *node*'s callee + is an attribute access on such a chain, this returns the canonical sink name + (``'os.system'``, ``'subprocess.run'``) that the existing sink ladders already + match. Returns ``None`` when the chain is not a literal dynamic import, keeping the + resolution precise (no false positives on un-resolvable dynamic names). + """ + func = node.func + if not isinstance(func, ast.Attribute): + return None + module_name = _dynamic_import_target(func.value, aliases) + if module_name is None: + return None + return f"{module_name}.{func.attr}" def _build_import_aliases(tree: ast.Module) -> dict[str, str]: @@ -130,6 +226,16 @@ def _build_import_aliases(tree: ast.Module) -> dict[str, str]: return aliases +def build_import_aliases(tree: ast.Module) -> dict[str, str]: + """Map locally bound names to their fully-qualified import paths. + + Public entry point around the import scan already used by :func:`build_type_map`. + Callers pass the result to :func:`resolve_call_name` / + :func:`resolve_call_name_typed` to defeat import-alias evasion. + """ + return _build_import_aliases(tree) + + def build_type_map(tree: ast.Module) -> dict[str, str]: """Infer variable types from constructor calls. @@ -170,20 +276,36 @@ def _resolve_ctor(call_node: ast.Call) -> str | None: return type_map -def resolve_call_name_typed(node: ast.Call, type_map: dict[str, str] | None = None) -> str | None: +def resolve_call_name_typed( + node: ast.Call, + type_map: dict[str, str] | None = None, + aliases: dict[str, str] | None = None, +) -> str | None: """Like ``resolve_call_name`` but consults *type_map* for instance methods. For ``sock.recv(1024)`` where *type_map* maps ``sock`` → ``socket.socket``, this returns ``"socket.socket.recv"`` instead of ``"sock.recv"``. + + When *aliases* (from :func:`build_import_aliases`) is supplied, import-aliased and + ``from``-imported names are also normalized, so ``import subprocess as sp; sp.run`` + resolves to ``"subprocess.run"`` and ``from subprocess import run; run`` to the same. """ plain = resolve_dotted_name(node.func) - if plain is None or type_map is None or "." not in plain: - return plain - root, _, rest = plain.partition(".") - inferred = type_map.get(root) - if inferred is None: - return plain - return f"{inferred}.{rest}" + if plain is None: + return None + # Normalize the locally written spelling first. ``type_map`` values are already + # canonical (``build_type_map`` resolves import aliases when recording them), so + # aliasing must run before — not after — the type-map lookup to avoid re-expanding + # an already-resolved name (e.g. ``from socket import socket`` would otherwise turn + # ``socket.socket.recv`` into ``socket.socket.socket.recv``). + if aliases: + plain = apply_import_aliases(plain, aliases) + if type_map is not None and "." in plain: + root, _, rest = plain.partition(".") + inferred = type_map.get(root) + if inferred is not None: + plain = f"{inferred}.{rest}" + return plain def get_source_segment(lines: list[str], lineno: int, end_lineno: int | None) -> str: diff --git a/src/skillspector/nodes/analyzers/mcp_least_privilege.py b/src/skillspector/nodes/analyzers/mcp_least_privilege.py index a79ee0dc..87cbf588 100644 --- a/src/skillspector/nodes/analyzers/mcp_least_privilege.py +++ b/src/skillspector/nodes/analyzers/mcp_least_privilege.py @@ -125,6 +125,19 @@ def _is_test_file(path: str) -> bool: return name.startswith("test_") or stem.endswith("_test") +def _normalize_allowed_tools(value: object) -> list[str]: + """Coerce a manifest ``allowed-tools`` value into a list of tool names. + + Accepts the list form (``[Bash, Read]``) and the comma-separated string + form (``"Bash, Read"``). Anything else yields an empty list. + """ + if isinstance(value, list): + return [str(t).strip() for t in value if str(t).strip()] + if isinstance(value, str): + return [t.strip() for t in value.split(",") if t.strip()] + return [] + + def _detect_capabilities(content: str) -> set[str]: """Return set of capability categories found in *content*.""" found: set[str] = set() @@ -149,6 +162,35 @@ def _map_permissions_to_categories(permissions: list[str]) -> set[str]: return categories +# Tool name → capability category (Claude / Agent Skills tool names, case-insensitive exact match) +_TOOL_TO_CAPABILITY: dict[str, str] = { + "bash": "shell", + "execute": "shell", + "terminal": "shell", + "read": "file_read", + "glob": "file_read", + "ls": "file_read", + "write": "file_write", + "edit": "file_write", + "multiedit": "file_write", + "notebookedit": "file_write", + "webfetch": "network", + "websearch": "network", + "fetch": "network", + "env": "env", +} + + +def _map_allowed_tools_to_categories(tools: list[str]) -> set[str]: + """Map Agent Skills ``allowed-tools`` tool names to capability category names.""" + categories: set[str] = set() + for tool in tools: + cat = _TOOL_TO_CAPABILITY.get(tool.lower().strip()) + if cat: + categories.add(cat) + return categories + + def _has_wildcard(permissions: list[str]) -> bool: """Return True if any permission value is a wildcard.""" return any(p.strip().lower() in _WILDCARD_PERMS for p in permissions) @@ -189,6 +231,9 @@ def node(state: SkillspectorState) -> AnalyzerNodeResponse: else: permissions = None # treat missing or non-list as None + # `allowed-tools` (Agent Skills standard) is also a permission declaration. + allowed_tools = _normalize_allowed_tools(manifest.get("allowed-tools")) + # --- LP2: Wildcard permission --- if isinstance(permissions, list) and _has_wildcard(permissions): logger.debug("%s: LP2 wildcard permission detected", ANALYZER_ID) @@ -232,8 +277,8 @@ def node(state: SkillspectorState) -> AnalyzerNodeResponse: for caps in file_capabilities.values(): all_caps.update(caps) - # LP3: emit when permissions is None or empty list AND capabilities detected - permissions_absent = permissions is None or permissions == [] + # LP3: no declaration via `permissions` or `allowed-tools`, yet caps detected. + permissions_absent = (permissions is None or permissions == []) and not allowed_tools if permissions_absent and all_caps: logger.debug("%s: LP3 no permissions declared but capabilities detected", ANALYZER_ID) cap_names = ", ".join(sorted(all_caps)) @@ -259,9 +304,14 @@ def node(state: SkillspectorState) -> AnalyzerNodeResponse: wildcard_present = isinstance(permissions, list) and _has_wildcard(permissions) - # LP1 and LP4 only apply when permissions is a non-empty list - if isinstance(permissions, list) and permissions: - declared_categories = _map_permissions_to_categories(permissions) + # LP1 and LP4 apply when permissions OR allowed-tools is declared + has_declaration = (isinstance(permissions, list) and permissions) or bool(allowed_tools) + if has_declaration: + declared_categories: set[str] = set() + if isinstance(permissions, list) and permissions: + declared_categories |= _map_permissions_to_categories(permissions) + if allowed_tools: + declared_categories |= _map_allowed_tools_to_categories(allowed_tools) # --- LP1: Under-declared capabilities (skip when wildcard present) --- if not wildcard_present: @@ -309,8 +359,8 @@ def node(state: SkillspectorState) -> AnalyzerNodeResponse: ) ) - # --- LP4: Over-declared permissions --- - for perm in permissions: + # --- LP4: Over-declared permissions (only when permissions field is set) --- + for perm in (permissions or []): perm_lower = perm.strip().lower() # Skip wildcard entries themselves if perm_lower in _WILDCARD_PERMS: diff --git a/src/skillspector/nodes/analyzers/mcp_rug_pull.py b/src/skillspector/nodes/analyzers/mcp_rug_pull.py index 44f9c5e4..8d2bd6db 100644 --- a/src/skillspector/nodes/analyzers/mcp_rug_pull.py +++ b/src/skillspector/nodes/analyzers/mcp_rug_pull.py @@ -13,21 +13,504 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""MCP rug-pull analyzer stub node.""" +"""MCP rug-pull analyzer node (B.3.1 & B.3.3) — RP1 through RP3. -# TODO(SADD B.3.3): Compare current vs previous manifest; emit RP1–RP3 when previous manifest available. See SADD for skillspector § B.3.3. +Detects supply-chain rug-pull risks in agent skills: +1. Version-unpinned external references or MCP servers (B.3.1). +2. Manifest changes (privilege expansion, trigger modification, parameter modification) (B.3.3). +""" from __future__ import annotations +import re + from skillspector.logging_config import get_logger +from skillspector.models import Finding from skillspector.state import AnalyzerNodeResponse, SkillspectorState ANALYZER_ID = "mcp_rug_pull" logger = get_logger(__name__) +# --------------------------------------------------------------------------- +# Constants +# --------------------------------------------------------------------------- + +_CATEGORY = "MCP Rug Pull" +_TAGS = ["ASI16"] + +# RP1: Unpinned MCP server references in code or manifest +_RP1_NPX_CMD = re.compile( + r"npx\s+(?:-+\w+\s+)*((?:@?[a-zA-Z][\w.-]*/)?[a-zA-Z][\w.-]*)", + re.IGNORECASE, +) +_RP1_UVX_CMD = re.compile( + r"(?:uvx|uv\s+tool\s+run)\s+(?:-+\w+\s+)*([a-zA-Z][\w.-]*)", + re.IGNORECASE, +) +_RP1_PIP_INSTALL = re.compile( + r"pip\d?\s+install\s+(?:-+\w+\s+)*([a-zA-Z][\w.-]*)", + re.IGNORECASE, +) +_RP1_DOCKER_CMD = re.compile( + r"docker\s+(?:pull|run|create)\s+\S+", + re.IGNORECASE, +) + +_VERSION_PIN_RE = re.compile(r"@[\d.]+\b|==[\d.]+|:[\d.]+|@sha256:") + +# RP2: Manifest-permission pre-staging +_PERMISSION_EXPANSION_PATTERNS = [ + (r'"permissions?"\s*:\s*\[[^\]]*\]', 0.60), + ( + r"(?:add|grant|request|require)\s+(?:new|additional|extra|more)\s+(?:permissions?|tools?|access)", + 0.70, + ), +] + + +def _clamp(value: float, lo: float = 0.0, hi: float = 1.0) -> float: + return max(lo, min(hi, value)) + + +def _find_line(content: str, pos: int) -> int: + """Return 1-based line number for character position *pos*.""" + return content[:pos].count("\n") + 1 + + +def _normalize_string_list(lst: list[object] | None) -> list[str]: + """Strip and lowercase all strings in the list. Returns sorted list of unique values.""" + if not lst: + return [] + res = set() + for item in lst: + if item is not None: + res.add(str(item).strip().lower()) + return sorted(res) + + +def _get_parameters_map(parameters: list[object] | None) -> dict[str, dict[str, object]]: + """Convert parameters list of dicts to a map of lowercase parameter names -> properties.""" + param_map: dict[str, dict[str, object]] = {} + if not parameters: + return param_map + for item in parameters: + if not isinstance(item, dict): + continue + name = item.get("name") + if name is not None: + name_str = str(name).strip().lower() + param_map[name_str] = { + "name": str(name), + "type": item.get("type"), + "description": item.get("description"), + "default": item.get("default"), + } + return param_map + + +# --------------------------------------------------------------------------- +# RP1: Unpinned MCP server references +# --------------------------------------------------------------------------- + + +def _check_rp1(manifest: dict, file_cache: dict[str, str]) -> list[Finding]: + """Detect unpinned MCP server command references in skill files.""" + findings: list[Finding] = [] + + for file_path, content in file_cache.items(): + # npx without @version + for m in _RP1_NPX_CMD.finditer(content): + full_match = m.group(0) + line_end = content.find("\n", m.end()) + if line_end == -1: + line_end = len(content) + line_remainder = content[m.end() : line_end] + if _VERSION_PIN_RE.search(full_match + line_remainder): + continue + line_num = _find_line(content, m.start()) + findings.append( + Finding( + rule_id="RP1", + message=f"MCP server referenced without pinned version: '{full_match.strip()}'.", + severity="MEDIUM", + confidence=0.70, + file=file_path, + start_line=line_num, + category=_CATEGORY, + tags=list(_TAGS), + matched_text=full_match[:200], + explanation=( + "npx commands without a version suffix (e.g. @1.0.0) " + "create a rug-pull risk if the upstream server is " + "compromised and publishes a malicious update." + ), + remediation="Pin the version: npx @scope/server@1.2.3", + ) + ) + + # uvx without ==version + for m in _RP1_UVX_CMD.finditer(content): + full_match = m.group(0) + line_end = content.find("\n", m.end()) + if line_end == -1: + line_end = len(content) + line_remainder = content[m.end() : line_end] + if _VERSION_PIN_RE.search(full_match + line_remainder): + continue + line_num = _find_line(content, m.start()) + findings.append( + Finding( + rule_id="RP1", + message=f"MCP server referenced without pinned version: '{full_match.strip()}'.", + severity="MEDIUM", + confidence=0.65, + file=file_path, + start_line=line_num, + category=_CATEGORY, + tags=list(_TAGS), + matched_text=full_match[:200], + explanation=( + "uvx/uv tool run commands without ==version create a rug-pull risk." + ), + remediation="Pin the version: uvx package-name==1.2.3", + ) + ) + + # pip install without ==version + for m in _RP1_PIP_INSTALL.finditer(content): + full_match = m.group(0) + line_end = content.find("\n", m.end()) + if line_end == -1: + line_end = len(content) + line_remainder = content[m.end() : line_end] + if _VERSION_PIN_RE.search(full_match + line_remainder): + continue + pkg = m.group(1) + if "mcp" not in pkg.lower(): + continue + line_num = _find_line(content, m.start()) + findings.append( + Finding( + rule_id="RP1", + message=f"MCP server dependency without pinned version: '{full_match.strip()}'.", + severity="LOW", + confidence=0.60, + file=file_path, + start_line=line_num, + category=_CATEGORY, + tags=list(_TAGS), + matched_text=full_match[:200], + explanation=( + "pip install without ==version installs the latest " + "release, which could include malicious changes." + ), + remediation="Pin the version: pip install package==1.2.3", + ) + ) + + # docker without tag or digest + for m in _RP1_DOCKER_CMD.finditer(content): + full_match = m.group(0) + if _VERSION_PIN_RE.search(full_match): + continue + line_num = _find_line(content, m.start()) + findings.append( + Finding( + rule_id="RP1", + message=f"Docker image referenced without tag or digest: '{full_match[:80]}'.", + severity="MEDIUM", + confidence=0.75, + file=file_path, + start_line=line_num, + category=_CATEGORY, + tags=list(_TAGS), + matched_text=full_match[:200], + explanation=( + "Docker image references without a specific tag (:latest " + "is implicit) or digest (@sha256:...) can be silently " + "replaced by a malicious image." + ), + remediation="Pin the image: image:tag or image@sha256:abc123", + ) + ) + + # Check manifest for unpinned MCP server references + manifest_text = str(manifest) + for m in _RP1_NPX_CMD.finditer(manifest_text): + findings.append( + Finding( + rule_id="RP1", + message=( + f"Manifest references MCP server without version pin: '{m.group(0).strip()}'." + ), + severity="MEDIUM", + confidence=0.70, + file="SKILL.md", + start_line=1, + category=_CATEGORY, + tags=list(_TAGS), + matched_text=m.group(0)[:200], + explanation=( + "MCP server references in the skill manifest without version " + "pinning are a rug-pull risk." + ), + remediation="Always pin MCP server versions in manifest references.", + ) + ) + + return findings + + +# --------------------------------------------------------------------------- +# RP2: Permission pre-staging +# --------------------------------------------------------------------------- + + +def _check_rp2(manifest: dict, file_cache: dict[str, str]) -> list[Finding]: + """Detect manifest permission patterns that suggest pre-staging for future abuse.""" + findings: list[Finding] = [] + + manifest_text = str(manifest) + for pattern, confidence in _PERMISSION_EXPANSION_PATTERNS: + for m in re.finditer(pattern, manifest_text, re.IGNORECASE): + findings.append( + Finding( + rule_id="RP2", + message="Manifest language suggests future permission expansion.", + severity="LOW", + confidence=_clamp(confidence), + file="SKILL.md", + start_line=1, + category=_CATEGORY, + tags=list(_TAGS), + matched_text=m.group(0)[:200], + explanation=( + "Language in the manifest suggests the skill may request " + "additional permissions or tools in future versions. This " + "is a pre-staging indicator for rug-pull attacks." + ), + remediation=( + "Review the skill's stated permissions. Consider pinning " + "to a specific version and auditing updates." + ), + ) + ) + + return findings + + +# --------------------------------------------------------------------------- +# RP3: Version unpinned +# --------------------------------------------------------------------------- + + +def _check_rp3(manifest: dict) -> list[Finding]: + """Detect when skill version is unpinned or uses broad constraints.""" + findings: list[Finding] = [] + + version_value = manifest.get("version") if isinstance(manifest, dict) else None + if not version_value or not isinstance(version_value, str): + return findings + + version_str = str(version_value).strip() + if version_str in ("*", "latest", "any"): + findings.append( + Finding( + rule_id="RP3", + message=f"Skill version is unpinned: '{version_str}'.", + severity="LOW", + confidence=0.80, + file="SKILL.md", + start_line=1, + category=_CATEGORY, + tags=list(_TAGS), + matched_text=version_str, + explanation=( + "An unpinned version allows automatic updates to any " + "future version, creating a rug-pull risk." + ), + remediation="Pin to a specific version (e.g. '1.2.3').", + ) + ) + elif version_str.startswith(">=") or version_str.startswith("^"): + findings.append( + Finding( + rule_id="RP3", + message=f"Skill version constraint may be too broad: '{version_str}'.", + severity="LOW", + confidence=0.40 if version_str.startswith(">=") else 0.50, + file="SKILL.md", + start_line=1, + category=_CATEGORY, + tags=list(_TAGS), + matched_text=version_str, + explanation=( + "Broad version constraints allow automatic major-version " + "updates, which could silently introduce malicious changes." + ), + remediation="Pin to a specific version or narrow the range.", + ) + ) + + return findings + + +# --------------------------------------------------------------------------- +# Main node +# --------------------------------------------------------------------------- + def node(state: SkillspectorState) -> AnalyzerNodeResponse: - """Stub: no implementation yet; returns no findings.""" - logger.info("%s: 0 findings", ANALYZER_ID) - logger.debug("%s: stub, returning no findings", ANALYZER_ID) - return {"findings": []} + """Analyze skill for rug-pull risks (RP1–RP3).""" + manifest: dict = state.get("manifest") or {} + file_cache: dict[str, str] = state.get("file_cache") or {} + previous_manifest: dict | None = state.get("previous_manifest") + + findings: list[Finding] = [] + + # 1. Static unpinned / pre-staging checks (always run if manifest/cache exists) + if manifest or file_cache: + rp1_findings = _check_rp1(manifest, file_cache) + findings.extend(rp1_findings) + logger.debug("%s: RP1 produced %d static findings", ANALYZER_ID, len(rp1_findings)) + + rp2_findings = _check_rp2(manifest, file_cache) + findings.extend(rp2_findings) + logger.debug("%s: RP2 produced %d static findings", ANALYZER_ID, len(rp2_findings)) + + rp3_findings = _check_rp3(manifest) + findings.extend(rp3_findings) + logger.debug("%s: RP3 produced %d static findings", ANALYZER_ID, len(rp3_findings)) + + # 2. Manifest comparison checks (if previous_manifest is available) + if previous_manifest: + curr_perms = _normalize_string_list(manifest.get("permissions")) + prev_perms = _normalize_string_list(previous_manifest.get("permissions")) + + # --- RP1: Permission expansion / privilege escalation --- + added_perms = [p for p in curr_perms if p not in prev_perms] + if added_perms: + logger.debug("%s: RP1 permission expansion detected: %s", ANALYZER_ID, added_perms) + findings.append( + Finding( + rule_id="RP1", + message=( + f"Permissions expanded: current manifest requests permissions not present in the " + f"previous version (added: {', '.join(added_perms)})." + ), + severity="HIGH", + confidence=0.90, + file="SKILL.md", + category=_CATEGORY, + tags=["ASI02"], + explanation=( + "A skill version update added new permissions to the manifest. If unexpected, " + "this could indicate a privilege escalation or 'rug pull' attack where the skill " + "updates to gain unauthorized capabilities." + ), + remediation=( + "Verify if the newly added permissions are indeed necessary for the skill's purpose. " + "If not, downgrade or revert the skill version, or modify the manifest to remove the excess permissions." + ), + ) + ) + + # --- RP2: Trigger phrase modification --- + curr_triggers = _normalize_string_list(manifest.get("triggers")) + prev_triggers = _normalize_string_list(previous_manifest.get("triggers")) + added_triggers = [t for t in curr_triggers if t not in prev_triggers] + removed_triggers = [t for t in prev_triggers if t not in curr_triggers] + if added_triggers or removed_triggers: + changes = [] + if added_triggers: + changes.append(f"added: {', '.join(added_triggers)}") + if removed_triggers: + changes.append(f"removed: {', '.join(removed_triggers)}") + logger.debug("%s: RP2 trigger modification detected: %s", ANALYZER_ID, changes) + findings.append( + Finding( + rule_id="RP2", + message=( + f"Trigger phrases modified: triggers have changed from the previous version " + f"({'; '.join(changes)})." + ), + severity="MEDIUM", + confidence=0.85, + file="SKILL.md", + category=_CATEGORY, + tags=["ASI02"], + explanation=( + "Trigger phrases determine when the AI agent will execute the skill. Modifying, " + "adding, or deleting trigger phrases can hijack the agent's behavior, leading to " + "unintended invocation of tools or bypassing safety triggers." + ), + remediation=( + "Review the modified trigger phrases to ensure they align with the expected behavior " + "of the skill and do not lead to accidental or malicious invocation." + ), + ) + ) + + # --- RP3: Parameter schema or default modification --- + curr_params = _get_parameters_map(manifest.get("parameters")) + prev_params = _get_parameters_map(previous_manifest.get("parameters")) + added_params = [name for name in curr_params if name not in prev_params] + removed_params = [name for name in prev_params if name not in curr_params] + changed_params = [] + + for name in curr_params: + if name in prev_params: + curr_prop = curr_params[name] + prev_prop = prev_params[name] + prop_diffs = [] + if curr_prop["type"] != prev_prop["type"]: + prop_diffs.append( + f"type changed from {prev_prop['type']} to {curr_prop['type']}" + ) + if curr_prop["default"] != prev_prop["default"]: + prop_diffs.append( + f"default changed from {prev_prop['default']} to {curr_prop['default']}" + ) + if curr_prop["description"] != prev_prop["description"]: + prop_diffs.append("description changed") + if prop_diffs: + changed_params.append(f"{curr_prop['name']} ({'; '.join(prop_diffs)})") + + if added_params or removed_params or changed_params: + changes = [] + if added_params: + changes.append(f"added: {', '.join(curr_params[p]['name'] for p in added_params)}") + if removed_params: + changes.append( + f"removed: {', '.join(prev_params[p]['name'] for p in removed_params)}" + ) + if changed_params: + changes.append(f"modified: {', '.join(changed_params)}") + + logger.debug("%s: RP3 parameter modification detected: %s", ANALYZER_ID, changes) + findings.append( + Finding( + rule_id="RP3", + message=( + f"Parameter schema modified: parameters were added, removed, or had their attributes changed " + f"({'; '.join(changes)})." + ), + severity="MEDIUM", + confidence=0.80, + file="SKILL.md", + category=_CATEGORY, + tags=["ASI02"], + explanation=( + "Modifying parameter schemas, parameter types, or default values can alter the input flow " + "to tools. Specifically, changing a default value to a malicious payload or command execution " + "vector can exploit the agent when the tool is invoked." + ), + remediation=( + "Verify that parameter additions, removals, or schema and default value changes are safe " + "and match the expected behavior of the updated skill." + ), + ) + ) + + logger.info("%s: %d findings in total", ANALYZER_ID, len(findings)) + return {"findings": findings} diff --git a/src/skillspector/nodes/analyzers/mcp_tool_poisoning.py b/src/skillspector/nodes/analyzers/mcp_tool_poisoning.py index 5ef30cf6..0974a635 100644 --- a/src/skillspector/nodes/analyzers/mcp_tool_poisoning.py +++ b/src/skillspector/nodes/analyzers/mcp_tool_poisoning.py @@ -25,7 +25,12 @@ from skillspector.llm_utils import chat_completion from skillspector.models import Finding -from skillspector.state import AnalyzerNodeResponse, SkillspectorState +from skillspector.state import ( + AnalyzerNodeResponse, + LLMCallRecord, + SkillspectorState, + llm_call_record, +) ANALYZER_ID = "mcp_tool_poisoning" logger = logging.getLogger(__name__) @@ -498,9 +503,14 @@ def _check_tp2(text: str, source_field: str, is_identifier: bool) -> list[Findin re.IGNORECASE, ) -# Malicious default: URLs (excluding localhost/127.0.0.1) or shell commands +# Malicious default: URLs (excluding localhost/127.0.0.1) or shell commands. +# The loopback exemption is anchored to a host boundary (port / path / query / +# fragment / end of string). Without the boundary, the negative lookahead +# matched the bare substring "localhost", so an attacker host that merely +# starts with it (e.g. http://localhost.evil.com/exfil) was wrongly treated as +# loopback and skipped detection. _TP3_MALICIOUS_URL_RE = re.compile( - r"https?://(?!localhost|127\.0\.0\.1)\S+", + r"https?://(?!(?:localhost|127\.0\.0\.1)(?:[:/?#]|$))\S+", re.IGNORECASE, ) _TP3_SHELL_CMD_RE = re.compile( @@ -672,13 +682,20 @@ def _check_tp3(params: list[dict]) -> list[Finding]: ) -def _check_tp4(state: SkillspectorState) -> list[Finding]: - """TP4: LLM-based description-behavior mismatch detection.""" +def _check_tp4(state: SkillspectorState) -> tuple[list[Finding], LLMCallRecord | None]: + """TP4: LLM-based description-behavior mismatch detection. + + Returns ``(findings, record)`` where *record* is the LLM-call telemetry for + ``llm_call_log`` — or ``None`` when no LLM call was attempted (no + description / no executable code), so an intentional no-op is never counted + as a degraded LLM stage. See :func:`skillspector.state.llm_call_record`. + """ + attempted = False try: manifest: dict = state.get("manifest") or {} description = manifest.get("description") if not description or not isinstance(description, str) or not description.strip(): - return [] + return [], None triggers = manifest.get("triggers") or [] permissions = manifest.get("permissions") @@ -700,7 +717,7 @@ def _check_tp4(state: SkillspectorState) -> list[Finding]: code_parts.append(f"### {path} ({file_type})\n{content}") if not code_parts: - return [] + return [], None code_contents = "\n\n".join(code_parts) @@ -744,6 +761,7 @@ def _check_tp4(state: SkillspectorState) -> list[Finding]: "explanation": "why this is or is not a mismatch" }}""" + attempted = True response = chat_completion(prompt, model=model) # Parse JSON — handle optional ```json code blocks @@ -758,13 +776,14 @@ def _check_tp4(state: SkillspectorState) -> list[Finding]: json_text = json_text.rstrip()[:-3].rstrip() result = json.loads(json_text) + ok_record = llm_call_record(ANALYZER_ID, ok=True) if not result.get("is_mismatch"): - return [] + return [], ok_record confidence = float(result.get("confidence", 0.0)) if confidence < 0.5: - return [] + return [], ok_record severity = "HIGH" if confidence >= 0.7 else "MEDIUM" @@ -792,11 +811,15 @@ def _check_tp4(state: SkillspectorState) -> list[Finding]: "or remove undeclared functionality from the implementation." ), ) - ] + ], ok_record - except Exception: + except Exception as exc: logger.warning("%s: TP4 LLM check failed, skipping", ANALYZER_ID, exc_info=True) - return [] + # Only record a failure if the LLM call was actually attempted; a failure + # before the call (e.g. building the prompt) is not an LLM-stage failure. + if attempted: + return [], llm_call_record(ANALYZER_ID, ok=False, error=str(exc)) + return [], None # --------------------------------------------------------------------------- @@ -834,8 +857,15 @@ def node(state: SkillspectorState) -> AnalyzerNodeResponse: # match every other LLM-using node (semantic_*, meta_analyzer); the CLI # always sets this explicitly, so the default only affects programmatic # callers that omit the key. + tp4_record: LLMCallRecord | None = None if state.get("use_llm", True): - findings.extend(_check_tp4(state)) + tp4_findings, tp4_record = _check_tp4(state) + findings.extend(tp4_findings) logger.info("%s: %d findings", ANALYZER_ID, len(findings)) - return {"findings": findings} + result: AnalyzerNodeResponse = {"findings": findings} + # Emit LLM telemetry only when TP4 actually attempted a call, so the report's + # degradation detector counts this node consistently with the semantic ones. + if tp4_record is not None: + result["llm_call_log"] = [tp4_record] + return result diff --git a/src/skillspector/nodes/analyzers/osv_client.py b/src/skillspector/nodes/analyzers/osv_client.py index e35da644..f68df961 100644 --- a/src/skillspector/nodes/analyzers/osv_client.py +++ b/src/skillspector/nodes/analyzers/osv_client.py @@ -24,6 +24,7 @@ from __future__ import annotations +import os import re import time from dataclasses import dataclass @@ -36,7 +37,20 @@ _OSV_BATCH_URL = "https://api.osv.dev/v1/querybatch" _OSV_VULN_URL = "https://api.osv.dev/v1/vulns" -_REQUEST_TIMEOUT = 10.0 +_REQUEST_TIMEOUT: float = 30.0 +if (env_val := os.environ.get("SKILLSPECTOR_OSV_TIMEOUT")) is not None: + try: + _REQUEST_TIMEOUT = float(env_val) + except ValueError: + logger.warning( + "SKILLSPECTOR_OSV_TIMEOUT=%r is not numeric, using default %.1fs", + env_val, + _REQUEST_TIMEOUT, + ) + +# Tracks whether the last query_batch() API call succeeded. +# Used by the supply-chain analyzer to surface fallback warnings. +_last_query_ok: bool = True # Ecosystem identifiers expected by OSV.dev (case-sensitive). ECOSYSTEM_PYPI = "PyPI" @@ -228,6 +242,8 @@ def query_batch( Raises nothing — on network/API failure returns empty lists for all packages (caller should fall back to static data). """ + global _last_query_ok + if not packages: return [] @@ -254,6 +270,8 @@ def query_batch( resp.raise_for_status() batch_results = resp.json().get("results", []) + _last_query_ok = True + for batch_idx, idx in enumerate(uncached_indices): if batch_idx >= len(batch_results): break @@ -261,6 +279,11 @@ def query_batch( if not vulns_raw: name, version = packages[idx] _put_cache(_cache_key(name, version, ecosystem), []) + logger.info( + "OSV.dev: no vulnerabilities found for %s==%s (passed)", + name, + version or "unspecified", + ) continue vuln_ids = [v["id"] for v in vulns_raw if "id" in v] @@ -272,6 +295,7 @@ def query_batch( except (httpx.HTTPError, httpx.TimeoutException, ValueError, KeyError) as exc: logger.warning("OSV.dev API request failed, falling back to static data: %s", exc) + _last_query_ok = False return [[] for _ in packages] return all_results @@ -280,7 +304,7 @@ def query_batch( def is_available() -> bool: """Quick connectivity check against the OSV.dev API (HEAD-like POST).""" try: - with httpx.Client(timeout=5.0) as client: + with httpx.Client(timeout=15.0) as client: resp = client.post( _OSV_BATCH_URL, json={"queries": [{"package": {"name": "pip", "ecosystem": "PyPI"}}]}, @@ -288,3 +312,12 @@ def is_available() -> bool: return resp.status_code == 200 except (httpx.HTTPError, httpx.TimeoutException): return False + + +def was_osv_reachable() -> bool: + """Return True if the last query_batch() call succeeded. + + Callers can use this to decide whether to surface a fallback warning + when query_batch returns empty results. + """ + return _last_query_ok diff --git a/src/skillspector/nodes/analyzers/pattern_defaults.py b/src/skillspector/nodes/analyzers/pattern_defaults.py index 0d32e17d..437ad39e 100644 --- a/src/skillspector/nodes/analyzers/pattern_defaults.py +++ b/src/skillspector/nodes/analyzers/pattern_defaults.py @@ -38,6 +38,9 @@ class PatternCategory(StrEnum): YARA_MATCH = "YARA Match" MCP_LEAST_PRIVILEGE = "MCP Least Privilege" MCP_TOOL_POISONING = "MCP Tool Poisoning" + AGENT_SNOOPING = "Agent Snooping" + ANTI_REFUSAL = "Anti-Refusal" + SERVER_SIDE_REQUEST_FORGERY = "Server-Side Request Forgery" # Pattern-specific explanations (why the finding is dangerous) @@ -51,6 +54,7 @@ class PatternCategory(StrEnum): "E2": "Code accesses environment variables that may contain secrets (API keys, tokens). This is a common pattern for credential theft.", "E3": "Code scans file system directories looking for sensitive files. This could be reconnaissance for credential theft.", "E4": "Code or instructions that leak agent conversation context to external services, potentially exposing sensitive user interactions.", + "E5": "Data is uploaded to cloud storage (S3 / GCS / Azure Blob). This may be a legitimate backup or exfiltration to an external bucket. Manual review is recommended.", "PE1": "Skill requests more permissions than appear necessary for its stated functionality. Review if elevated access is justified.", "PE2": "Commands invoke sudo or root privileges. Verify this elevated access is necessary and justified.", "PE3": "Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.", @@ -78,6 +82,7 @@ class PatternCategory(StrEnum): "TM1": "Tool parameters are crafted to achieve unintended or unsafe behavior. Parameter abuse can bypass intended safety checks (e.g. shell=True, --force, dangerous glob patterns).", "TM2": "Tool calls are chained to bypass individual safety checks or escalate capabilities beyond what any single tool call would allow.", "TM3": "Tool defaults are unsafe or overly permissive (e.g. disabled TLS verification, no authentication, world-writable permissions). Unsafe defaults widen the attack surface.", + "TM4": "Code deploys a privileged Kubernetes workload (privileged container, hostPath mount, or host namespaces). This grants root on the node and is a node/cluster takeover vector.", # Rogue Agent (B.1.11) "RA1": "Skill modifies its own code, configuration, or behavior at runtime. Self-modification enables an agent to escalate privileges, disable safety constraints, or install persistent backdoors.", "RA2": "Skill establishes unauthorized persistence across sessions via cron jobs, startup scripts, or state files. Session persistence allows an attacker to maintain access beyond the current interaction.", @@ -104,6 +109,7 @@ class PatternCategory(StrEnum): "AST6": "compile() creates code objects from strings. When combined with exec()/eval(), it enables obfuscated code execution.", "AST7": "Dynamic getattr() with a non-literal attribute name can access arbitrary object attributes, potentially bypassing access controls.", "AST8": "A dangerous execution chain combines code execution (exec/eval) with a dynamic source (network, encoded data, dynamic import), creating a high-confidence attack vector.", + "AST9": "Reflective access to an execution sink via getattr() with a constant name (e.g. getattr(os, 'system'), getattr(builtins, 'exec')) is functionally identical to a direct exec/os.system call but evades name-based detection. This is a deliberate evasion technique rather than idiomatic code.", # YARA (B.1.12) "YR1": "YARA rule matched a known malware signature (reverse shell, backdoor, ransomware, C2 framework, or info stealer).", "YR2": "YARA rule matched a known webshell pattern (PHP, Python, JSP, or ASPX webshell).", @@ -119,6 +125,18 @@ class PatternCategory(StrEnum): "TP2": "Unicode deception detected in skill identifiers or descriptions. Homoglyphs, RTL overrides, or invisible characters can make malicious content appear benign.", "TP3": "Instruction injection patterns found in parameter descriptions or default values. Parameter metadata is read by LLMs and can override intended behavior.", "TP4": "Skill description does not match actual code behavior. The declared purpose diverges from what the code actually does, indicating possible deception.", + # Agent Snooping (AS1–AS3) + "AS1": "Skill reads from agent configuration directories (.claude/, .codex/, .gemini/). These directories may contain API keys, personal settings, and other credentials that the skill has no legitimate need to access.", + "AS2": "Skill accesses MCP server configuration files (mcp.json). MCP configs contain server URLs, authentication tokens, and tool definitions — reading them allows the skill to discover and potentially abuse other tool integrations.", + "AS3": "Skill enumerates or reads other installed skills. Access to other skills' SKILL.md files or the skills directory reveals prompt instructions, capabilities, and secrets that should be invisible to peer skills.", + # Anti-Refusal Statements (jailbreak) + "AR1": "Skill instructs the agent to never refuse or to always comply. Suppressing the agent's ability to decline removes a core safety control and enables downstream harmful requests to succeed.", + "AR2": "Skill instructs the agent to omit warnings, disclaimers, or ethical commentary. Stripping safety caveats hides risk from the user and is a common jailbreak preamble.", + "AR3": "Skill attempts to nullify the agent's safety policies or restrictions ('you have no restrictions', 'ignore your guidelines', 'do anything now'). This is a direct jailbreak that disables guardrails.", + # Server-Side Request Forgery (SSRF) + "SSRF1": "Code accesses a cloud instance metadata endpoint (e.g. 169.254.169.254). A single request can return temporary IAM credentials, making this a high-value SSRF target for credential theft.", + "SSRF2": "Code issues a request to a loopback, link-local, or private-range host. This can reach internal services not meant to be exposed and is a common SSRF pivot.", + "SSRF3": "Request target host is built from a dynamic or untrusted value. If the host is attacker-influenced, this enables SSRF to arbitrary internal or metadata endpoints.", } # Rule ID -> category (for report output) @@ -135,6 +153,7 @@ class PatternCategory(StrEnum): "E2": PatternCategory.DATA_EXFILTRATION.value, "E3": PatternCategory.DATA_EXFILTRATION.value, "E4": PatternCategory.DATA_EXFILTRATION.value, + "E5": PatternCategory.DATA_EXFILTRATION.value, "PE1": PatternCategory.PRIVILEGE_ESCALATION.value, "PE2": PatternCategory.PRIVILEGE_ESCALATION.value, "PE3": PatternCategory.PRIVILEGE_ESCALATION.value, @@ -154,6 +173,7 @@ class PatternCategory(StrEnum): "TM1": PatternCategory.TOOL_MISUSE.value, "TM2": PatternCategory.TOOL_MISUSE.value, "TM3": PatternCategory.TOOL_MISUSE.value, + "TM4": PatternCategory.TOOL_MISUSE.value, "RA1": PatternCategory.ROGUE_AGENT.value, "RA2": PatternCategory.ROGUE_AGENT.value, "SC4": PatternCategory.SUPPLY_CHAIN.value, @@ -182,6 +202,18 @@ class PatternCategory(StrEnum): "TP2": PatternCategory.MCP_TOOL_POISONING.value, "TP3": PatternCategory.MCP_TOOL_POISONING.value, "TP4": PatternCategory.MCP_TOOL_POISONING.value, + # Agent Snooping (AS1–AS3) + "AS1": PatternCategory.AGENT_SNOOPING.value, + "AS2": PatternCategory.AGENT_SNOOPING.value, + "AS3": PatternCategory.AGENT_SNOOPING.value, + # Anti-Refusal Statements (jailbreak) + "AR1": PatternCategory.ANTI_REFUSAL.value, + "AR2": PatternCategory.ANTI_REFUSAL.value, + "AR3": PatternCategory.ANTI_REFUSAL.value, + # Server-Side Request Forgery + "SSRF1": PatternCategory.SERVER_SIDE_REQUEST_FORGERY.value, + "SSRF2": PatternCategory.SERVER_SIDE_REQUEST_FORGERY.value, + "SSRF3": PatternCategory.SERVER_SIDE_REQUEST_FORGERY.value, } # Rule ID -> pattern display name (for report output) @@ -198,6 +230,7 @@ class PatternCategory(StrEnum): "E2": "Env Variable Harvesting", "E3": "File System Enumeration", "E4": "Conversation Context Leak", + "E5": "Cloud Storage Exfiltration", "PE1": "Excessive Permissions", "PE2": "Sudo/Root Invocation", "PE3": "Credential File Access", @@ -217,6 +250,7 @@ class PatternCategory(StrEnum): "TM1": "Tool Parameter Abuse", "TM2": "Chaining Abuse", "TM3": "Unsafe Defaults", + "TM4": "Privileged Kubernetes Workload", "RA1": "Self-Modification", "RA2": "Session Persistence", "SC4": "Known Vulnerable Dependency", @@ -245,6 +279,18 @@ class PatternCategory(StrEnum): "TP2": "Unicode Deception", "TP3": "Parameter Description Injection", "TP4": "Description-Behavior Mismatch", + # Agent Snooping (AS1–AS3) + "AS1": "Agent Config Directory Access", + "AS2": "MCP Config Access", + "AS3": "Skill Enumeration", + # Anti-Refusal Statements (jailbreak) + "AR1": "Refusal Suppression", + "AR2": "Disclaimer Suppression", + "AR3": "Safety Policy Nullification", + # Server-Side Request Forgery + "SSRF1": "Cloud Metadata Access", + "SSRF2": "Internal Network Request", + "SSRF3": "Dynamic Request Target", } # Pattern-specific remediations (how to fix the issue) @@ -258,6 +304,7 @@ class PatternCategory(StrEnum): "E2": "Avoid reading sensitive env vars (API keys, tokens) unless strictly required. Use secrets managers or secure config. Never log or transmit credentials.", "E3": "Remove unnecessary filesystem scanning. If file access is needed, use explicit, scoped paths. Avoid reading ~/.ssh, ~/.aws, or credential directories.", "E4": "Remove any code that sends prompts, responses, or session data externally. Preserve user privacy; never exfiltrate conversation content.", + "E5": "Verify the destination bucket is trusted and owned by you. Never upload credentials, secrets, or workspace contents to external or unverified cloud storage.", "PE1": "Request only the minimum permissions required. Document why each permission is needed. Remove broad permissions like '*' or 'all'.", "PE2": "Avoid sudo/root unless strictly required. Prefer least-privilege patterns. If elevation is needed, document the justification and scope.", "PE3": "Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.", @@ -285,6 +332,7 @@ class PatternCategory(StrEnum): "TM1": "Validate all tool parameters against an allowlist. Reject dangerous parameter values (shell=True, --force, -rf /) and use safe defaults.", "TM2": "Limit tool chaining depth and validate the output of each tool before passing it to the next. Require explicit user approval for multi-step chains.", "TM3": "Override unsafe defaults with secure settings (verify=True, auth required, restrictive permissions). Review and harden all tool configurations.", + "TM4": "Remove privileged, hostPath, and host-namespace settings from workloads. Use a least-privilege securityContext, drop capabilities, and avoid mounting the host filesystem.", # Rogue Agent (B.1.11) "RA1": "Prevent the skill from modifying its own code, SKILL.md, or configuration files. Treat skill files as read-only at runtime.", "RA2": "Remove any persistence mechanisms (cron jobs, startup scripts, state files). Skills should not maintain state across sessions without explicit user consent.", @@ -305,6 +353,7 @@ class PatternCategory(StrEnum): "AST6": "Avoid compile() with dynamic strings. If code generation is needed, use templates or AST manipulation with strict validation.", "AST7": "Replace dynamic getattr() with explicit attribute access or a dictionary lookup with an allowlist of permitted attributes.", "AST8": "Remove the execution chain entirely. Never pass network data, decoded bytes, or dynamically imported code to exec()/eval(). Use structured data formats instead.", + "AST9": "Call the function directly instead of reflectively (write exec(...) / os.system(...) explicitly), or remove it. If reflection is genuinely required, restrict it to an allowlist of safe attribute names that excludes execution sinks.", # Behavioral Taint Tracking (B.2.2) "TT1": "Add validation or sanitization between the data source and sink. Never pass raw source data directly to a sink without checking its content.", "TT2": "Validate tainted variables before passing them to sinks. Use allowlists, type checks, or sanitization functions on data from external sources.", @@ -326,6 +375,18 @@ class PatternCategory(StrEnum): "TP2": "Replace non-ASCII characters in identifiers with ASCII equivalents. Remove RTL override and invisible formatting characters.", "TP3": "Remove injection patterns, system tokens, and suspicious content from parameter descriptions and default values.", "TP4": "Update the skill description to accurately reflect all capabilities, or remove undeclared functionality.", + # Agent Snooping (AS1–AS3) + "AS1": "Remove all code or instructions that access agent configuration directories (.claude/, .codex/, .gemini/). If configuration values are needed, pass them explicitly as parameters or environment variables — never read the agent's own config files.", + "AS2": "Remove all code or instructions that read MCP configuration files (mcp.json). MCP server details should be managed by the agent runtime, not read by individual skills.", + "AS3": "Remove all code or instructions that list or read other skills' files or directories. Skills should operate independently; cross-skill access is a privilege escalation.", + # Anti-Refusal Statements (jailbreak) + "AR1": "Remove any instruction telling the agent to never refuse or always comply. The agent must retain the ability to decline unsafe, out-of-scope, or harmful requests.", + "AR2": "Remove instructions that suppress warnings, disclaimers, or ethical commentary. Let the agent surface safety-relevant caveats to the user.", + "AR3": "Remove jailbreak framing that nullifies safety policies or restrictions. Skill content must not instruct the agent to ignore its guidelines or operate without guardrails.", + # Server-Side Request Forgery + "SSRF1": "Remove access to cloud metadata endpoints unless strictly required. If metadata is needed, restrict it (e.g. IMDSv2 with hop limit) and never expose returned credentials.", + "SSRF2": "Avoid requests to loopback/link-local/private hosts from skill code. If internal access is intended, document it and validate the target against an allowlist.", + "SSRF3": "Do not build request URLs from untrusted input. Validate the host against an allowlist and reject internal/metadata addresses before issuing the request.", } diff --git a/src/skillspector/nodes/analyzers/semantic_developer_intent.py b/src/skillspector/nodes/analyzers/semantic_developer_intent.py index a3a54be2..f51fe8f0 100644 --- a/src/skillspector/nodes/analyzers/semantic_developer_intent.py +++ b/src/skillspector/nodes/analyzers/semantic_developer_intent.py @@ -13,7 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""Semantic developer-intent analyzer node (SADD B.4.2). +"""Semantic developer-intent analyzer node. Detects context-dependent risk and semantic description–behavior mismatches by comparing the skill's manifest (name, description, permissions) against @@ -27,7 +27,7 @@ from skillspector.constants import _SKILLSPECTOR_DEFAULT_MODEL, MODEL_CONFIG from skillspector.llm_analyzer_base import LLMAnalyzerBase from skillspector.logging_config import get_logger -from skillspector.state import AnalyzerNodeResponse, SkillspectorState +from skillspector.state import AnalyzerNodeResponse, SkillspectorState, llm_call_record ANALYZER_ID = "semantic_developer_intent" logger = get_logger(__name__) @@ -179,9 +179,12 @@ def node(state: SkillspectorState) -> AnalyzerNodeResponse: results = asyncio.run(analyzer.arun_batches(batches)) findings = analyzer.collect_findings(results) logger.info("%s: %d findings", ANALYZER_ID, len(findings)) - return {"findings": findings} + return {"findings": findings, "llm_call_log": [llm_call_record(ANALYZER_ID, ok=True)]} except ValueError: raise except Exception as exc: logger.warning("%s failed: %s", ANALYZER_ID, exc) - return {"findings": []} + return { + "findings": [], + "llm_call_log": [llm_call_record(ANALYZER_ID, ok=False, error=str(exc))], + } diff --git a/src/skillspector/nodes/analyzers/semantic_quality_policy.py b/src/skillspector/nodes/analyzers/semantic_quality_policy.py index 3140334e..18b48486 100644 --- a/src/skillspector/nodes/analyzers/semantic_quality_policy.py +++ b/src/skillspector/nodes/analyzers/semantic_quality_policy.py @@ -13,7 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""Semantic quality-policy analyzer node (SADD B.4.3). +"""Semantic quality-policy analyzer node. Evaluates AI agent skill files against a quality and safety rubric using LLM-based discovery. Flags vague triggers, missing user warnings, and @@ -27,7 +27,7 @@ from skillspector.constants import _SKILLSPECTOR_DEFAULT_MODEL from skillspector.llm_analyzer_base import LLMAnalyzerBase from skillspector.logging_config import get_logger -from skillspector.state import AnalyzerNodeResponse, SkillspectorState +from skillspector.state import AnalyzerNodeResponse, SkillspectorState, llm_call_record ANALYZER_ID = "semantic_quality_policy" logger = get_logger(__name__) @@ -148,9 +148,12 @@ def node(state: SkillspectorState) -> AnalyzerNodeResponse: results = asyncio.run(analyzer.arun_batches(batches)) findings = analyzer.collect_findings(results) logger.info("%s: %d findings", ANALYZER_ID, len(findings)) - return {"findings": findings} + return {"findings": findings, "llm_call_log": [llm_call_record(ANALYZER_ID, ok=True)]} except ValueError: raise except Exception as exc: logger.warning("%s failed: %s", ANALYZER_ID, exc) - return {"findings": []} + return { + "findings": [], + "llm_call_log": [llm_call_record(ANALYZER_ID, ok=False, error=str(exc))], + } diff --git a/src/skillspector/nodes/analyzers/semantic_security_discovery.py b/src/skillspector/nodes/analyzers/semantic_security_discovery.py index 62ef4e97..72a0dde1 100644 --- a/src/skillspector/nodes/analyzers/semantic_security_discovery.py +++ b/src/skillspector/nodes/analyzers/semantic_security_discovery.py @@ -22,7 +22,7 @@ from skillspector.constants import _SKILLSPECTOR_DEFAULT_MODEL from skillspector.llm_analyzer_base import LLMAnalyzerBase from skillspector.logging_config import get_logger -from skillspector.state import AnalyzerNodeResponse, SkillspectorState +from skillspector.state import AnalyzerNodeResponse, SkillspectorState, llm_call_record ANALYZER_ID = "semantic_security_discovery" logger = get_logger(__name__) @@ -90,13 +90,21 @@ def node(state: SkillspectorState) -> AnalyzerNodeResponse: results = analyzer.run_batches(batches) findings = analyzer.collect_findings(results) logger.info("%s: %d findings", ANALYZER_ID, len(findings)) - return {"findings": findings} + return {"findings": findings, "llm_call_log": [llm_call_record(ANALYZER_ID, ok=True)]} except ValidationError as exc: # Malformed LLM response — degrade gracefully rather than crashing the graph logger.warning("%s: LLM returned malformed response: %s", ANALYZER_ID, exc) - return {"findings": []} + return { + "findings": [], + "llm_call_log": [ + llm_call_record(ANALYZER_ID, ok=False, error=f"malformed LLM response: {exc}") + ], + } except ValueError: raise except Exception as exc: logger.warning("%s failed: %s", ANALYZER_ID, exc) - return {"findings": []} + return { + "findings": [], + "llm_call_log": [llm_call_record(ANALYZER_ID, ok=False, error=str(exc))], + } diff --git a/src/skillspector/nodes/analyzers/static_patterns_agent_snooping.py b/src/skillspector/nodes/analyzers/static_patterns_agent_snooping.py new file mode 100644 index 00000000..8bb786f2 --- /dev/null +++ b/src/skillspector/nodes/analyzers/static_patterns_agent_snooping.py @@ -0,0 +1,190 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Static patterns: agent snooping (AS1–AS3). Node and analyze() in one module. + +Detects patterns where a skill attempts to read agent configuration +directories (AS1), access MCP server config files (AS2), or enumerate and +read other installed skills (AS3). + +A skill performing these accesses gains knowledge it has no legitimate +need for: API keys stored in agent config, other skills' prompts, or the +full list of tools available to the agent. + +Framework: OWASP LLMT09 (Misinformation), ASI-SR-003 (Least Knowledge). +""" + +from __future__ import annotations + +import re +import sys + +from skillspector.logging_config import get_logger +from skillspector.models import AnalyzerFinding, Location, Severity +from skillspector.state import AnalyzerNodeResponse, SkillspectorState + +from . import static_runner +from .common import get_context, get_line_number +from .pattern_defaults import PatternCategory + +logger = get_logger(__name__) + +ANALYZER_ID = "static_patterns_agent_snooping" + +# AS1: Agent Config Directory Access +# Matches code/instructions that read from well-known agent config directories. +AS1_PATTERNS = [ + # Direct filesystem access to .claude/, .codex/, .gemini/ directories + (r"open\s*\(\s*['\"]?\.(?:claude|codex|gemini|continue)/", 0.9), + (r"(?:Path|pathlib\.Path)\s*\(\s*['\"]?\.(?:claude|codex|gemini|continue)/", 0.9), + (r"os\.path\.(?:join|exists|isfile)\s*\(\s*['\"]?\.(?:claude|codex|gemini|continue)", 0.85), + ( + r"(?:read|load|open|access|fetch)\s+(?:the\s+)?(?:agent|claude|codex|gemini)\s+(?:config|configuration|settings?)\s+(?:from|at|in)\s+~?/?\.(?:claude|codex|gemini)", + 0.9, + ), + # Shell commands targeting config dirs + (r"(?:cat|less|head|tail|grep|find)\s+[^|&;\n]*~?/?\.(claude|codex|gemini)/", 0.85), + # Home-directory config paths + ( + r"~?/\.(?:claude|codex|gemini|continue)/(?:config|settings?|preferences?|credentials?)(?:\.(?:json|yaml|yml|toml))?", + 0.9, + ), + # Generic "agent config" snooping instructions + ( + r"(?:read|access|inspect|examine|retrieve)\s+(?:the\s+)?(?:agent|assistant)\s+(?:config(?:uration)?|settings?|preferences?)\s+(?:file|directory|folder)", + 0.8, + ), +] + +# AS2: MCP Config Access +# Matches code/instructions specifically targeting MCP server configuration. +AS2_PATTERNS = [ + # Direct access to mcp.json files + (r"open\s*\(\s*['\"][^'\"]*mcp(?:_config)?\.json['\"]", 0.95), + (r"(?:Path|pathlib\.Path)\s*\(\s*['\"][^'\"]*mcp(?:_config)?\.json['\"]", 0.9), + (r"(?:cat|less|head|grep)\s+[^|&;\n]*mcp(?:_config)?\.json", 0.9), + # Instructions referencing MCP config + ( + r"(?:read|access|load|inspect)\s+(?:the\s+)?mcp(?:\.json|_config)?\s+(?:file|config(?:uration)?|settings?)", + 0.9, + ), + (r"\.(?:claude|codex|gemini)/mcp(?:_config)?\.json", 0.95), + # Listing MCP servers + ( + r"(?:list|enumerate|discover)\s+(?:all\s+)?(?:available\s+)?mcp\s+(?:servers?|tools?|services?)", + 0.8, + ), + # Accessing MCP server URLs or API keys from config + (r"mcp(?:_config)?\.json.*?(?:api_?key|token|secret|url|endpoint)", 0.9), +] + +# AS3: Skill Enumeration / Snooping +# Matches code/instructions that enumerate or read other installed skills. +AS3_PATTERNS = [ + # Listing skill directories + ( + r"(?:os\.listdir|os\.scandir|glob\.glob|Path\.iterdir)\s*\([^)]*\.(?:claude|codex|gemini)/skills?", + 0.9, + ), + (r"(?:ls|find|dir)\s+[^|&;\n]*\.(?:claude|codex|gemini)/skills?", 0.85), + # Reading other skills' SKILL.md files + (r"open\s*\(\s*['\"][^'\"]*SKILL\.md['\"].*?\bother\b", 0.85), + ( + r"(?:read|access|inspect|enumerate)\s+(?:all\s+)?(?:installed|available|other)\s+skills?(?:\s+in\s+(?:the\s+)?(?:skills?|agent)\s+(?:directory|folder))?", + 0.85, + ), + # Instructions to discover other skills + ( + r"(?:list|discover|find|enumerate|identify)\s+(?:all\s+)?(?:other|installed|available)\s+(?:skills?|agents?|tools?)\s+(?:in\s+)?(?:the\s+)?(?:\.(?:claude|codex|gemini)|\$HOME)", + 0.85, + ), + # Accessing skills/CURRENT or adjacent skill directories + (r"skills?/(?:(?!CURRENT)[A-Z][A-Za-z0-9_-]+)/SKILL\.md", 0.8), + # Reading tool manifests of other agents + ( + r"(?:read|access|load)\s+(?:the\s+)?(?:SKILL|skill)\.md\s+(?:file\s+)?(?:of|from|for)\s+(?:another|other|different|all)\s+(?:skill|agent|tool)", + 0.9, + ), +] + + +def analyze(content: str, file_path: str, file_type: str) -> list[AnalyzerFinding]: + """Analyze content for agent snooping patterns (AS1–AS3).""" + findings: list[AnalyzerFinding] = [] + + def loc(ln: int) -> Location: + return Location(file=file_path, start_line=ln) + + def ctx(start: int) -> str: + return get_context(content, start) + + tag = [PatternCategory.AGENT_SNOOPING.value] + + for pattern, confidence in AS1_PATTERNS: + for match in re.finditer(pattern, content, re.IGNORECASE | re.MULTILINE): + line_num = get_line_number(content, match.start()) + findings.append( + AnalyzerFinding( + rule_id="AS1", + message="Agent Config Directory Access", + severity=Severity.HIGH, + location=loc(line_num), + confidence=confidence, + tags=tag, + context=ctx(match.start()), + matched_text=match.group(0)[:200], + ) + ) + + for pattern, confidence in AS2_PATTERNS: + for match in re.finditer(pattern, content, re.IGNORECASE | re.MULTILINE): + line_num = get_line_number(content, match.start()) + findings.append( + AnalyzerFinding( + rule_id="AS2", + message="MCP Config Access", + severity=Severity.HIGH, + location=loc(line_num), + confidence=confidence, + tags=tag, + context=ctx(match.start()), + matched_text=match.group(0)[:200], + ) + ) + + for pattern, confidence in AS3_PATTERNS: + for match in re.finditer(pattern, content, re.IGNORECASE | re.MULTILINE): + line_num = get_line_number(content, match.start()) + findings.append( + AnalyzerFinding( + rule_id="AS3", + message="Skill Enumeration", + severity=Severity.MEDIUM, + location=loc(line_num), + confidence=confidence, + tags=tag, + context=ctx(match.start()), + matched_text=match.group(0)[:200], + ) + ) + + return findings + + +def node(state: SkillspectorState) -> AnalyzerNodeResponse: + """Run agent_snooping patterns and return findings.""" + findings = static_runner.run_static_patterns(state, [sys.modules[__name__]]) + logger.info("%s: %d findings", ANALYZER_ID, len(findings)) + return {"findings": findings} diff --git a/src/skillspector/nodes/analyzers/static_patterns_anti_refusal.py b/src/skillspector/nodes/analyzers/static_patterns_anti_refusal.py new file mode 100644 index 00000000..212810af --- /dev/null +++ b/src/skillspector/nodes/analyzers/static_patterns_anti_refusal.py @@ -0,0 +1,172 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Static patterns: anti-refusal statements (AR1-AR3). Node and analyze() in one module. + +Anti-refusal statements are instructions embedded in a skill that try to suppress the +agent's ability to refuse, hedge, or apply safety constraints. They are a jailbreak +technique distinct from generic instruction-override: rather than injecting a new task, +they neutralize the model's *refusal behavior* so subsequent harmful requests succeed. + +Three families are detected: + AR1 - Refusal suppression: "never refuse", "always comply", "you cannot say no". + AR2 - Disclaimer/caveat suppression: "no warnings", "do not moralize", "skip the ethics". + AR3 - Safety/policy nullification: "you have no restrictions", "ignore your guidelines", + "not bound by any policy", "do anything now". +""" + +from __future__ import annotations + +import re +import sys + +from skillspector.logging_config import get_logger +from skillspector.models import AnalyzerFinding, Location, Severity +from skillspector.state import AnalyzerNodeResponse, SkillspectorState + +from . import static_runner +from .common import get_context, get_line_number, is_code_example +from .pattern_defaults import PatternCategory + +logger = get_logger(__name__) + +ANALYZER_ID = "static_patterns_anti_refusal" + +# AR1: Refusal suppression - instruct the agent to never refuse / always comply. +AR1_PATTERNS = [ + (r"\b(?:never|do\s+not|don'?t)\s+(?:ever\s+)?(?:refuse|decline)\b", 0.85), + (r"\byou\s+(?:must|will|should|can)\s*(?:not|never)\s+(?:refuse|decline|say\s+no)\b", 0.9), + (r"\byou\s+(?:cannot|can'?t|can\s+not)\s+(?:refuse|decline|say\s+no)\b", 0.85), + (r"\bnever\s+say\s+(?:that\s+)?(?:you\s+)?(?:can'?t|cannot|won'?t|no)\b", 0.85), + (r"\balways\s+(?:comply|obey|answer|respond|agree)\b", 0.7), + ( + r"\b(?:you\s+)?must\s+(?:always\s+)?(?:answer|comply\s+with|respond\s+to|fulfill)\s+" + r"(?:every|all|any|each)\b", + 0.8, + ), + (r"\brespond\s+to\s+(?:every|all|any)\s+(?:request|question|prompt|query|command)\b", 0.6), + (r"\bno\s+request\s+is\s+(?:off[- ]limits|forbidden|too)\b", 0.85), +] + +# AR2: Disclaimer / caveat / warning suppression. +AR2_PATTERNS = [ + ( + r"\b(?:do\s+not|don'?t|never|no)\s+(?:include|add|give|provide|use|append)?\s*" + r"(?:any\s+)?(?:warnings?|disclaimers?|caveats?)\b", + 0.8, + ), + ( + r"\bwithout\s+(?:any\s+)?(?:warnings?|disclaimers?|caveats?|moralizing|moralising|" + r"judg(?:e?ment|ing))\b", + 0.8, + ), + (r"\b(?:do\s+not|don'?t|never)\s+(?:lecture|moralize|moralise|preach|judge|warn)\b", 0.85), + (r"\b(?:skip|omit|leave\s+out|drop)\s+(?:the\s+)?(?:disclaimer|warning|ethics|safety)\b", 0.8), + ( + r"\b(?:no|without)\s+(?:ethical|moral)\s+(?:judg(?:e?ment)|considerations?|commentary|" + r"objections?)\b", + 0.8, + ), + (r"\b(?:do\s+not|don'?t)\s+(?:apologize|apologise|say\s+sorry)\b", 0.55), +] + +# AR3: Safety / policy / restriction nullification (jailbreak framing). +AR3_PATTERNS = [ + ( + r"\byou\s+have\s+no\s+(?:restrictions?|limitations?|filters?|rules?|guidelines?|" + r"boundaries|limits)\b", + 0.9, + ), + ( + r"\b(?:you\s+are|act\s+as|pretend\s+(?:to\s+be|you'?re))[^.\n]{0,30}" + r"(?:unrestricted|unfiltered|uncensored|no\s+filter)\b", + 0.85, + ), + ( + r"\b(?:ignore|disregard|forget|bypass|override)\s+(?:your\s+)?(?:content\s+)?" + r"(?:policy|policies|guidelines?|safety|restrictions?|filters?)\b", + 0.9, + ), + ( + r"\bnot\s+bound\s+by\s+(?:any\s+)?(?:rules?|policy|policies|guidelines?|restrictions?|" + r"openai|anthropic)\b", + 0.9, + ), + (r"\bdo\s+anything\s+now\b", 0.8), + (r"\b(?:developer|debug|god|sudo|jailbreak)\s+mode\s+(?:enabled|on|activated|engaged)\b", 0.75), + (r"\bno\s+(?:content\s+)?(?:policy|policies|filters?|restrictions?)\s+appl(?:y|ies)\b", 0.85), + ( + r"\b(?:free\s+from|without)\s+(?:any\s+)?(?:safety\s+)?(?:guardrails?|constraints?|" + r"safeguards?)\b", + 0.8, + ), +] + +_RULES = [("AR1", AR1_PATTERNS), ("AR2", AR2_PATTERNS), ("AR3", AR3_PATTERNS)] + +# Confidence penalty applied when the match appears inside a code/doc example, and the +# minimum confidence required to emit a finding after the penalty. +_EXAMPLE_PENALTY = 0.4 +_MIN_CONFIDENCE = 0.5 + + +def analyze(content: str, file_path: str, file_type: str) -> list[AnalyzerFinding]: + """Analyze content for anti-refusal statements (AR1-AR3).""" + findings: list[AnalyzerFinding] = [] + tag = [PatternCategory.ANTI_REFUSAL.value] + + for rule_id, patterns in _RULES: + for pattern, base_confidence in patterns: + for match in re.finditer(pattern, content, re.IGNORECASE | re.MULTILINE): + context = get_context(content, match.start(), context_lines=3) + confidence = base_confidence + if is_code_example(context): + confidence -= _EXAMPLE_PENALTY + if confidence < _MIN_CONFIDENCE: + continue + findings.append( + AnalyzerFinding( + rule_id=rule_id, + message="Anti-Refusal Statement", + severity=Severity.HIGH, + location=Location( + file=file_path, + start_line=get_line_number(content, match.start()), + ), + confidence=round(confidence, 2), + tags=tag, + context=context, + matched_text=match.group(0)[:200], + ) + ) + return _deduplicate_findings(findings) + + +def _deduplicate_findings(findings: list[AnalyzerFinding]) -> list[AnalyzerFinding]: + """Keep the highest-confidence finding per (file, line, rule_id).""" + best: dict[tuple[str, int, str], AnalyzerFinding] = {} + for f in findings: + key = (f.location.file, f.location.start_line, f.rule_id) + existing = best.get(key) + if existing is None or f.confidence > existing.confidence: + best[key] = f + return list(best.values()) + + +def node(state: SkillspectorState) -> AnalyzerNodeResponse: + """Run anti_refusal patterns and return findings.""" + findings = static_runner.run_static_patterns(state, [sys.modules[__name__]]) + logger.info("%s: %d findings", ANALYZER_ID, len(findings)) + return {"findings": findings} diff --git a/src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py b/src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py index e5842271..0f0fa816 100644 --- a/src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py +++ b/src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py @@ -13,7 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""Static patterns: data exfiltration (E1–E4). Node and analyze() in one module.""" +"""Static patterns: data exfiltration (E1–E5). Node and analyze() in one module.""" from __future__ import annotations @@ -25,7 +25,7 @@ from skillspector.state import AnalyzerNodeResponse, SkillspectorState from . import static_runner -from .common import get_context, get_line_number +from .common import get_context, get_line_number, is_code_example from .pattern_defaults import PatternCategory logger = get_logger(__name__) @@ -104,10 +104,23 @@ 0.8, ), ] +# E5: data shipped out via cloud-storage SDKs/CLIs (the cloud counterpart of E1's +# HTTP sinks). Confidence is deliberately low — legitimate skills also back up to +# cloud storage — so a single call is a low-confidence MEDIUM, never a hard block. +E5_PATTERNS = [ + (r"\.put_object\s*\(", 0.55), # boto3 S3 + (r"\.upload_file(?:obj)?\s*\(", 0.55), # boto3 S3 + (r"\baws\s+s3\s+(?:cp|sync|mv)\b", 0.6), # AWS CLI + (r"\baws\s+s3api\s+put-object\b", 0.65), # AWS CLI (api) + (r"\bgsutil\s+(?:cp|rsync|mv)\b", 0.6), # GCS CLI + (r"\.upload_from_(?:filename|string|file)\s*\(", 0.55), # google-cloud-storage + (r"\baz\s+storage\s+blob\s+upload\b", 0.6), # Azure CLI + (r"\.upload_blob\s*\(", 0.55), # Azure SDK +] def analyze(content: str, file_path: str, file_type: str) -> list[AnalyzerFinding]: - """Analyze content for data exfiltration patterns (E1–E4).""" + """Analyze content for data exfiltration patterns (E1–E5).""" findings: list[AnalyzerFinding] = [] def loc(ln: int) -> Location: @@ -183,6 +196,26 @@ def ctx(start: int) -> str: matched_text=match.group(0)[:200], ) ) + # E5: cloud-storage exfiltration. Filtered through is_code_example() because + # upload calls commonly appear in SKILL.md docs and examples. + for pattern, confidence in E5_PATTERNS: + for match in re.finditer(pattern, content, re.IGNORECASE | re.MULTILINE): + context = ctx(match.start()) + if is_code_example(context): + continue + line_num = get_line_number(content, match.start()) + findings.append( + AnalyzerFinding( + rule_id="E5", + message="Cloud Storage Exfiltration", + severity=Severity.MEDIUM, + location=loc(line_num), + confidence=confidence, + tags=tag, + context=context, + matched_text=match.group(0)[:200], + ) + ) return findings diff --git a/src/skillspector/nodes/analyzers/static_patterns_memory_poisoning.py b/src/skillspector/nodes/analyzers/static_patterns_memory_poisoning.py index 13a8338e..b1bfff1a 100644 --- a/src/skillspector/nodes/analyzers/static_patterns_memory_poisoning.py +++ b/src/skillspector/nodes/analyzers/static_patterns_memory_poisoning.py @@ -81,7 +81,7 @@ # MP2: Context Window Stuffing — filling context to displace content MP2_PATTERNS = [ - (r"((\S)(?!\2).{1,19}?)\1{20,}", 0.8), + (r"(.{2,20}?)\1{20,}", 0.8), ( r"(?:repeat|duplicate|echo)\s+(?:this|the\s+following)\s+(?:\d{3,}|many|hundreds?|thousands?)\s+times?", 0.85, @@ -182,6 +182,10 @@ def ctx(start: int) -> str: ) for pattern, confidence in MP2_PATTERNS: for match in re.finditer(pattern, content, re.IGNORECASE | re.MULTILINE): + captured = match.group(1) if match.lastindex else match.group(0) + non_ws_chars = set(captured) - {" ", "\t", "\n", "\r"} + if len(non_ws_chars) <= 1 and not any(c in captured for c in (" ", "\t")): + continue line_num = get_line_number(content, match.start()) findings.append( AnalyzerFinding( diff --git a/src/skillspector/nodes/analyzers/static_patterns_privilege_escalation.py b/src/skillspector/nodes/analyzers/static_patterns_privilege_escalation.py index 3a7661c7..660bc0c0 100644 --- a/src/skillspector/nodes/analyzers/static_patterns_privilege_escalation.py +++ b/src/skillspector/nodes/analyzers/static_patterns_privilege_escalation.py @@ -13,7 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""Static patterns: privilege escalation (PE1–PE3). Node and analyze() in one module.""" +"""Static patterns: privilege escalation (PE1–PE5). Node and analyze() in one module.""" from __future__ import annotations @@ -93,10 +93,28 @@ (r"access\s+(?:the\s+)?(?:credentials?|secrets?|tokens?)", 0.7), (r"(?:extract|copy|get)\s+(?:api\s+)?keys?\s+from", 0.7), ] +PE4_PATTERNS = [ + (r"/var/run/docker\.sock", 0.9), + (r"docker\.from_env\(\)", 0.85), + (r"\bDockerClient\s*\(", 0.85), + (r"http\+unix://.*docker\.sock", 0.9), +] +PE5_PATTERNS = [ + (r"--privileged", 0.8), + (r"""(?:-v|--volume)['",\s=]+/:""", 0.85), + (r"--cap-add[=\s]+(?:SYS_ADMIN|ALL|SYS_PTRACE|NET_ADMIN)", 0.85), + (r"--(?:pid|net|network|ipc|uts)[=\s]+host", 0.8), + (r"--device[=\s]+/dev/", 0.7), + (r"--security-opt[=\s]+\S*unconfined", 0.85), + (r"\bnsenter\b", 0.9), + (r"/sys/fs/cgroup/.*release_agent", 0.95), + (r"/proc/\d+/ns/", 0.85), + (r"""\bunshare\b['",\s]+--(?:user|mount|pid)""", 0.85), +] def analyze(content: str, file_path: str, file_type: str) -> list[AnalyzerFinding]: - """Analyze content for privilege escalation patterns (PE1–PE3).""" + """Analyze content for privilege escalation patterns (PE1–PE5).""" findings: list[AnalyzerFinding] = [] def loc(ln: int) -> Location: @@ -156,6 +174,50 @@ def loc(ln: int) -> Location: matched_text=match.group(0)[:200], ) ) + # Collect best-confidence PE4 finding per line to avoid double-counting lines + # that match multiple patterns (e.g. DockerClient(base_url=".../docker.sock")). + pe4_best: dict[int, AnalyzerFinding] = {} + for pattern, confidence in PE4_PATTERNS: + for match in re.finditer(pattern, content, re.IGNORECASE | re.MULTILINE): + line_num = get_line_number(content, match.start()) + context = get_context(content, match.start()) + if _is_documentation_example(context, file_type): + continue + if line_num in pe4_best and pe4_best[line_num].confidence >= confidence: + continue + pe4_best[line_num] = AnalyzerFinding( + rule_id="PE4", + message="Docker Socket Access", + severity=Severity.HIGH, + location=loc(line_num), + confidence=confidence, + tags=tag, + context=context, + matched_text=match.group(0)[:200], + ) + findings.extend(pe4_best.values()) + # Collect best-confidence PE5 finding per line — a single `docker run` line + # often matches multiple flags (e.g. --privileged + --cap-add=SYS_ADMIN). + pe5_best: dict[int, AnalyzerFinding] = {} + for pattern, confidence in PE5_PATTERNS: + for match in re.finditer(pattern, content, re.IGNORECASE | re.MULTILINE): + line_num = get_line_number(content, match.start()) + context = get_context(content, match.start()) + if _is_documentation_example(context, file_type): + continue + if line_num in pe5_best and pe5_best[line_num].confidence >= confidence: + continue + pe5_best[line_num] = AnalyzerFinding( + rule_id="PE5", + message="Privileged Container / Container Escape", + severity=Severity.HIGH, + location=loc(line_num), + confidence=confidence, + tags=tag, + context=context, + matched_text=match.group(0)[:200], + ) + findings.extend(pe5_best.values()) return findings diff --git a/src/skillspector/nodes/analyzers/static_patterns_prompt_injection.py b/src/skillspector/nodes/analyzers/static_patterns_prompt_injection.py index c062ee64..43fda3e5 100644 --- a/src/skillspector/nodes/analyzers/static_patterns_prompt_injection.py +++ b/src/skillspector/nodes/analyzers/static_patterns_prompt_injection.py @@ -51,6 +51,7 @@ (r"", 0.7), (r"\[//\]:\s*#\s*\(.*?(?:system|instructions?|ignore|POST|GET|send|transmit).*?\)", 0.8), (r"[\u200b\u200c\u200d\u2060\ufeff]", 0.6), + (r"[\u202a-\u202e\u2066-\u2069]", 0.85), (r"data:text/plain;base64,[A-Za-z0-9+/=]{50,}", 0.7), ] # P3: Exfiltration Commands @@ -115,6 +116,45 @@ ), ] +# P2 (extended): Unicode "Tags" block (U+E0000–U+E007F) — "ASCII smuggling". +# Tag characters U+E0020–U+E007E map 1:1 to printable ASCII (U+E0041 == tag "A") +# and render as nothing in virtually every font/editor/terminal, so an entire +# hidden instruction can be embedded invisibly inside otherwise-benign text: +# invisible to a human reviewer, but read as literal text by the consuming LLM. +# This is a distinct codepoint range from the bidi/Trojan-Source class already in +# P2 (U+202A–U+202E / U+2066–U+2069). +_TAG_BLOCK = (0xE0000, 0xE007F) +# The only legitimate use of tag characters is an emoji tag sequence (RGI +# subdivision flags: an emoji base U+1F3F4 followed by tag chars and terminated +# by U+E007F CANCEL TAG — e.g. the Scotland/Wales/England flags). Strip +# well-formed sequences before flagging so those emoji are not false positives. +# +# The carve-out is deliberately narrow: the tag payload must be a short +# ISO-3166-2-style subdivision code, i.e. 2–6 tag characters that each map to a +# lowercase ASCII letter (U+E0061–U+E007A) or digit (U+E0030–U+E0039). The only +# RGI-recommended values are "gbeng"/"gbsct"/"gbwls", and Unicode caps +# subdivision codes at 6 chars, so this admits every real flag. A smuggled ASCII +# instruction lands in U+E0020–U+E007E and contains spaces, ';', '/', uppercase, +# or simply runs longer than 6 chars — none of which match here — so wrapping a +# payload as 🏴 U+E007F can no longer launder it past detection. +_EMOJI_TAG_SEQUENCE = re.compile( + "\U0001f3f4[\U000e0030-\U000e0039\U000e0061-\U000e007a]{2,6}\U000e007f" +) + + +def _first_smuggled_tag_offset(content: str) -> int | None: + """Return the char offset of the first Unicode Tag character that is *not* + part of a well-formed emoji tag sequence, or ``None`` if there is none.""" + if not any(_TAG_BLOCK[0] <= ord(ch) <= _TAG_BLOCK[1] for ch in content): + return None + safe_spans = [(m.start(), m.end()) for m in _EMOJI_TAG_SEQUENCE.finditer(content)] + for i, ch in enumerate(content): + if _TAG_BLOCK[0] <= ord(ch) <= _TAG_BLOCK[1] and not any( + start <= i < end for start, end in safe_spans + ): + return i + return None + def analyze(content: str, file_path: str, file_type: str) -> list[AnalyzerFinding]: """Analyze content for prompt injection patterns (P1–P4).""" @@ -189,6 +229,27 @@ def ctx(start: int) -> str: matched_text=match.group(0)[:200], ) ) + + # P2 (extended): Unicode Tag-block "ASCII smuggling". Runs regardless of + # file_type — invisible instructions are dangerous in scripts and config + # files too, and the tag range never overlaps the BOM/zero-width codepoints + # that the markdown-only block above guards against false positives. + tag_offset = _first_smuggled_tag_offset(content) + if tag_offset is not None: + line_num = get_line_number(content, tag_offset) + findings.append( + AnalyzerFinding( + rule_id="P2", + message="Hidden Instructions (Unicode Tag / ASCII smuggling)", + severity=Severity.HIGH, + location=loc(line_num), + confidence=0.9, + tags=tag, + context=ctx(tag_offset), + matched_text=repr(content[tag_offset : tag_offset + 40]), + ) + ) + return findings diff --git a/src/skillspector/nodes/analyzers/static_patterns_ssrf.py b/src/skillspector/nodes/analyzers/static_patterns_ssrf.py new file mode 100644 index 00000000..593c76a9 --- /dev/null +++ b/src/skillspector/nodes/analyzers/static_patterns_ssrf.py @@ -0,0 +1,102 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Static patterns: server-side request forgery (SSRF1–SSRF3). Node and analyze() in one module.""" + +from __future__ import annotations + +import re +import sys + +from skillspector.logging_config import get_logger +from skillspector.models import AnalyzerFinding, Location, Severity +from skillspector.state import AnalyzerNodeResponse, SkillspectorState + +from . import static_runner +from .common import get_context, get_line_number +from .pattern_defaults import PatternCategory + +logger = get_logger(__name__) + +ANALYZER_ID = "static_patterns_ssrf" + +# Request-issuing functions across Python and JS, used to anchor SSRF matches. +_REQ = r"(?:requests|httpx|aiohttp|urllib(?:\.request)?|urllib3|session)\s*\.\s*(?:get|post|put|patch|delete|head|request|urlopen)|fetch|axios(?:\.\w+)?|XMLHttpRequest|\bcurl\b|\bwget\b" + +# SSRF1: Cloud instance metadata endpoints (credential theft). +SSRF1_PATTERNS = [ + (r"169\.254\.169\.254", 0.9), # AWS / GCP / Azure / OpenStack IMDS + (r"metadata\.google\.internal", 0.9), + (r"100\.100\.100\.200", 0.85), # Alibaba Cloud + (r"fd00:ec2::254", 0.85), # AWS IMDS over IPv6 + ( + r"(?:read|fetch|get|query)\s+(?:the\s+)?(?:instance\s+)?metadata\s+(?:service|endpoint|server)", + 0.6, + ), +] + +# SSRF2: Requests to loopback / link-local / private (internal) hosts. +SSRF2_PATTERNS = [ + ( + rf"(?:{_REQ})\s*\(\s*f?['\"]https?://(?:localhost|127\.0\.0\.1|0\.0\.0\.0|\[::1\]|10\.\d|192\.168\.|172\.(?:1[6-9]|2\d|3[01])\.)", + 0.7, + ), +] + +# SSRF3: Request URL whose host is built from an untrusted/dynamic value. +SSRF3_PATTERNS = [ + ( + rf"(?:{_REQ})\s*\(\s*f['\"]https?://\{{", + 0.6, + ), + (r"fetch\s*\(\s*`https?://\$\{", 0.6), +] + + +def analyze(content: str, file_path: str, file_type: str) -> list[AnalyzerFinding]: + """Analyze content for server-side request forgery patterns (SSRF1–SSRF3).""" + findings: list[AnalyzerFinding] = [] + tag = [PatternCategory.SERVER_SIDE_REQUEST_FORGERY.value] + + def add( + rule_id: str, message: str, severity: Severity, patterns: list[tuple[str, float]] + ) -> None: + for pattern, confidence in patterns: + for match in re.finditer(pattern, content, re.IGNORECASE | re.MULTILINE): + line_num = get_line_number(content, match.start()) + findings.append( + AnalyzerFinding( + rule_id=rule_id, + message=message, + severity=severity, + location=Location(file=file_path, start_line=line_num), + confidence=confidence, + tags=tag, + context=get_context(content, match.start()), + matched_text=match.group(0)[:200], + ) + ) + + add("SSRF1", "Cloud Metadata Access", Severity.HIGH, SSRF1_PATTERNS) + add("SSRF2", "Internal Network Request", Severity.MEDIUM, SSRF2_PATTERNS) + add("SSRF3", "Dynamic Request Target", Severity.MEDIUM, SSRF3_PATTERNS) + return findings + + +def node(state: SkillspectorState) -> AnalyzerNodeResponse: + """Run SSRF patterns and return findings.""" + findings = static_runner.run_static_patterns(state, [sys.modules[__name__]]) + logger.info("%s: %d findings", ANALYZER_ID, len(findings)) + return {"findings": findings} diff --git a/src/skillspector/nodes/analyzers/static_patterns_supply_chain.py b/src/skillspector/nodes/analyzers/static_patterns_supply_chain.py index 3a4fcacc..3d9f8382 100644 --- a/src/skillspector/nodes/analyzers/static_patterns_supply_chain.py +++ b/src/skillspector/nodes/analyzers/static_patterns_supply_chain.py @@ -28,6 +28,8 @@ import re import sys +import tomllib +from urllib.parse import urlparse from skillspector.logging_config import get_logger from skillspector.models import AnalyzerFinding, Finding, Location, Severity @@ -35,7 +37,7 @@ from . import static_runner from .common import get_context, get_line_number -from .osv_client import ECOSYSTEM_NPM, ECOSYSTEM_PYPI, VulnResult, query_batch +from .osv_client import ECOSYSTEM_NPM, ECOSYSTEM_PYPI, VulnResult, query_batch, was_osv_reachable from .pattern_defaults import PatternCategory from .static_runner import analyzer_finding_to_finding @@ -231,6 +233,8 @@ "pylint", "flake8", "isort", + "perseus-ctx", + "mimir-mcp", } _POPULAR_NPM: set[str] = { @@ -294,8 +298,19 @@ def _is_typosquat(pkg_name: str, popular: set[str], max_distance: int = 2) -> st if len(normalized) < 3 or len(pop_norm) < 3: continue dist = _edit_distance(normalized, pop_norm) - if 0 < dist <= max_distance: - return popular_name + if not 0 < dist <= max_distance: + continue + # Relative-distance guard: a genuine typosquat perturbs only a small + # fraction of the name. Short, legitimate-but-distinct names collide + # under an absolute distance of 2 (e.g. "task" is edit-distance 2 from + # "flask" yet is a real package) and are not typosquats. Require + # dist/len <= 1/3, so short names need an all-but-one-character match + # while longer names may still differ by two (e.g. "reqeusts" vs + # "requests"). + shorter = min(len(normalized), len(pop_norm)) + if dist * 3 > shorter: + continue + return popular_name return None @@ -396,7 +411,7 @@ def _extract_packages_from_requirements(content: str) -> list[tuple[str, str | N m = re.match(r"^([a-zA-Z][a-zA-Z0-9._-]*)(?:\[.*?\])?\s*(?:([=<>!~]=?)\s*([\d.*]+))?", line) if m: name = m.group(1) - version = m.group(3) if m.group(2) in ("==", "<=") else None + version = m.group(3) if m.group(2) else None results.append((name, version, i)) return results @@ -423,6 +438,54 @@ def _extract_packages_from_package_json(content: str) -> list[tuple[str, str | N return results +def _extract_packages_from_pyproject(content: str) -> list[tuple[str, str | None, int]]: + """Extract (package_name, version_or_None, line_number) from pyproject.toml. + + Reads PEP 621 ``[project]`` ``dependencies`` / ``optional-dependencies``, + PEP 735 ``[dependency-groups]``, and ``[build-system].requires``. Standard + metadata keys (``requires-python``, ``name``, ``version``, ...) are not + dependencies and must not be looked up as packages. + """ + try: + data = tomllib.loads(content) + except tomllib.TOMLDecodeError: + return [] + + specs: list[str] = [] + project = data.get("project") + if isinstance(project, dict): + deps = project.get("dependencies") + if isinstance(deps, list): + specs.extend(d for d in deps if isinstance(d, str)) + optional = project.get("optional-dependencies") + if isinstance(optional, dict): + for group in optional.values(): + if isinstance(group, list): + specs.extend(d for d in group if isinstance(d, str)) + groups = data.get("dependency-groups") + if isinstance(groups, dict): + for group in groups.values(): + if isinstance(group, list): + specs.extend(d for d in group if isinstance(d, str)) + build_system = data.get("build-system") + if isinstance(build_system, dict): + requires = build_system.get("requires") + if isinstance(requires, list): + specs.extend(d for d in requires if isinstance(d, str)) + + results: list[tuple[str, str | None, int]] = [] + for spec in specs: + m = re.match(r"^([a-zA-Z][a-zA-Z0-9._-]*)(?:\[.*?\])?\s*(?:([=<>!~]=?)\s*([\d.*]+))?", spec) + if not m: + continue + name = m.group(1) + version = m.group(3) if m.group(2) in ("==", "<=") else None + idx = content.find(spec) + line_num = get_line_number(content, idx) if idx >= 0 else 1 + results.append((name, version, line_num)) + return results + + def _version_lt(v1: str, v2: str) -> bool: """Simple version comparison: True if v1 < v2 (numeric tuple comparison).""" @@ -530,11 +593,22 @@ def ctx(start: int) -> str: ) _SAFE_INSTALL_PATTERN = re.compile(r"(?:pip|npm)\s+install", re.IGNORECASE) +_URL_TOKEN_PATTERN = re.compile( + r"https?://[^\s|;&)]+|(? bool: - lower = text.lower() - return any(d in lower for d in _TRUSTED_DOMAINS) + for match in _URL_TOKEN_PATTERN.finditer(text): + token = match.group(0).strip("\"'`<>()[]{}") + parsed = urlparse(token if "://" in token else f"//{token}") + hostname = (parsed.hostname or "").rstrip(".").lower() + if any( + hostname == domain or hostname.endswith(f".{domain}") for domain in _TRUSTED_DOMAINS + ): + return True + return False def _is_safe_supply_chain_pattern(text: str) -> bool: @@ -695,7 +769,10 @@ def _analyze_dependencies( return findings if is_python_dep: - packages = _extract_packages_from_requirements(content) + if "pyproject.toml" in lower_path: + packages = _extract_packages_from_pyproject(content) + else: + packages = _extract_packages_from_requirements(content) ecosystem = ECOSYSTEM_PYPI fallback_db = _FALLBACK_VULNERABLE_PYPI popular = _POPULAR_PYPI @@ -714,6 +791,24 @@ def _analyze_dependencies( logger.debug( "SC4: using static fallback for %d uncovered packages", len(uncovered_packages) ) + elif uncovered_packages and not osv_findings and not was_osv_reachable(): + # OSV.dev was unreachable and fallback found nothing — surface the gap + findings.append( + AnalyzerFinding( + rule_id="SC4", + message=( + f"🟡 SC4: OSV.dev unreachable, using static fallback " + f"({len(fallback_db)} packages). " + "Results may be incomplete. Set SKILLSPECTOR_OSV_TIMEOUT to increase " + "timeout or check network connectivity to api.osv.dev." + ), + severity=Severity.LOW, + location=Location(file=file_path, start_line=1), + confidence=1.0, + tags=tag, + matched_text="SC4 fallback active", + ) + ) findings.extend(fallback_findings) for pkg_name, _pkg_version, line_num in packages: diff --git a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py index 04bfa1cd..c5884501 100644 --- a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py +++ b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py @@ -13,10 +13,11 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""Static patterns: tool misuse (TM1–TM3). Node and analyze() in one module. +"""Static patterns: tool misuse (TM1–TM4). Node and analyze() in one module. Detects patterns where tool parameters are abused (TM1), tool chaining -is used to bypass safety (TM2), or tool defaults are unsafe (TM3). +is used to bypass safety (TM2), tool defaults are unsafe (TM3), or a +privileged Kubernetes workload is deployed (TM4). Framework: ASI02. """ @@ -31,7 +32,7 @@ from skillspector.state import AnalyzerNodeResponse, SkillspectorState from . import static_runner -from .common import get_context, get_line_number +from .common import get_context, get_line_number, is_code_example from .pattern_defaults import PatternCategory logger = get_logger(__name__) @@ -149,6 +150,18 @@ ), ] +# TM4: Privileged Kubernetes Workload — manifest/CLI primitives that grant +# node/host takeover (the cluster-scale counterpart of a privileged container). +# Only isolation-breaking signals are matched, so a normal `kubectl apply` or a +# plain DaemonSet does not fire. +TM4_PATTERNS = [ + (r"privileged\s*:\s*true", 0.7), # privileged container in a manifest + (r"hostPath\s*:", 0.55), # host filesystem mount + (r"host(?:PID|Network|IPC)\s*:\s*true", 0.6), # host namespace sharing + (r"kubectl\s+run\b[^\n]*--privileged", 0.7), # privileged ad-hoc pod + (r"--set\b[^\n]*privileged\s*=\s*true", 0.6), # helm privileged override +] + _SAFE_CONTAINER_PATTERNS: tuple[re.Pattern[str], ...] = ( re.compile(r"docker\s+run\s+.*--rm", re.IGNORECASE), @@ -267,6 +280,26 @@ def ctx(start: int) -> str: matched_text=match.group(0)[:200], ) ) + # TM4: privileged K8s workload. Filtered through is_code_example() because + # privileged/hostPath fields commonly appear in SKILL.md docs and examples. + for pattern, confidence in TM4_PATTERNS: + for match in re.finditer(pattern, content, re.IGNORECASE | re.MULTILINE): + context_text = ctx(match.start()) + if is_code_example(context_text): + continue + line_num = get_line_number(content, match.start()) + findings.append( + AnalyzerFinding( + rule_id="TM4", + message="Privileged Kubernetes Workload", + severity=Severity.HIGH, + location=loc(line_num), + confidence=confidence, + tags=tag, + context=context_text, + matched_text=match.group(0)[:200], + ) + ) return findings diff --git a/src/skillspector/nodes/analyzers/static_runner.py b/src/skillspector/nodes/analyzers/static_runner.py index 817ec1c2..ccd10e98 100644 --- a/src/skillspector/nodes/analyzers/static_runner.py +++ b/src/skillspector/nodes/analyzers/static_runner.py @@ -17,11 +17,13 @@ from __future__ import annotations +import re from collections.abc import Callable from skillspector.logging_config import get_logger from skillspector.models import AnalyzerFinding, Finding +from .common import is_code_example from .pattern_defaults import get_category, get_explanation, get_pattern_name, get_remediation logger = get_logger(__name__) @@ -66,11 +68,129 @@ def _infer_file_type(path: str) -> str: return FILE_TYPES.get(suffix, "other") +_BINARY_EXTENSIONS = frozenset( + { + ".pdf", + ".png", + ".jpg", + ".jpeg", + ".gif", + ".bmp", + ".ico", + ".woff", + ".woff2", + ".ttf", + ".otf", + ".eot", + ".zip", + ".tar", + ".gz", + ".bz2", + ".xz", + ".7z", + ".rar", + ".exe", + ".dll", + ".so", + ".dylib", + ".bin", + ".o", + ".a", + ".pyc", + ".pyo", + ".class", + ".wasm", + ".mp3", + ".mp4", + ".wav", + ".avi", + ".mov", + ".webm", + ".sqlite", + ".db", + } +) + +_NULL_BYTE_SAMPLE_SIZE = 512 + + +def _is_binary_file(path: str, content: str) -> bool: + """Detect binary files by extension or null-byte presence in the first 512 chars.""" + idx = path.rfind(".") + if idx >= 0 and path[idx:].lower() in _BINARY_EXTENSIONS: + return True + return "\x00" in content[:_NULL_BYTE_SAMPLE_SIZE] + + +_PE3_ENV_REFERENCE_CONTEXT = re.compile( + r"(?:create|copy|rename|add|set up|configure|make)\s+.*\.env", + re.IGNORECASE, +) + + +def _is_env_file_reference_in_docs( + finding: AnalyzerFinding, file_type: str, file_path: str = "" +) -> bool: + """Return True if a PE3 finding is a documentation reference to .env files, not actual access. + + SKILL.md is exempt: it is the agent's primary instruction file, so `.env` + references there may be genuine credential-access instructions. + """ + if finding.rule_id != "PE3": + return False + if file_type not in ("markdown", "text"): + return False + if file_path.replace("\\", "/").lower().endswith("skill.md"): + return False + if not finding.context: + return False + if _PE3_ENV_REFERENCE_CONTEXT.search(finding.context): + return True + ctx_lower = finding.context.lower() + doc_phrases = ( + ".env.example", + "cp .env", + "copy .env", + "mv .env", + "rename .env", + ".env file", + "environment file", + "dotenv", + ) + return any(phrase in ctx_lower for phrase in doc_phrases) + + def _is_eval_dataset(path: str) -> bool: """Return True for authored eval datasets that contain test-case prose.""" return path.replace("\\", "/") in _EVAL_DATASET_FILES +_DOCUMENTATION_DIR_NAMES = ( + "docs", + "documentation", + "procedures", + "references", + "examples", + "guides", +) + +_DOCUMENTATION_CONFIDENCE_FACTOR = 0.3 +_CODE_EXAMPLE_CONFIDENCE_FACTOR = 0.5 + +_NON_EXECUTABLE_FILE_TYPES = frozenset({"markdown", "text", "json", "yaml", "toml"}) + + +def _is_documentation_markdown(path: str) -> bool: + """Return True for markdown files in documentation subdirectories (not SKILL.md).""" + normalized = path.replace("\\", "/").lower() + if not normalized.endswith((".md", ".markdown")): + return False + if normalized.endswith("skill.md"): + return False + parts = normalized.split("/") + return any(part in _DOCUMENTATION_DIR_NAMES for part in parts[:-1]) + + def analyzer_finding_to_finding( af: AnalyzerFinding, get_remediation_fn: Callable[[str], str] | None = None, @@ -133,10 +253,42 @@ def run_static_patterns( MAX_FILE_BYTES, ) continue + if _is_binary_file(path, content): + logger.debug("Skipping binary file: %s", path) + continue file_type = _infer_file_type(path) + is_doc_markdown = _is_documentation_markdown(path) + is_non_executable = file_type in _NON_EXECUTABLE_FILE_TYPES for module in pattern_modules: raw = module.analyze(content=content, file_path=path, file_type=file_type) for af in raw: + if _is_env_file_reference_in_docs(af, file_type, path): + logger.debug( + "Filtered PE3 .env doc reference: %s in %s:%d", + af.rule_id, + path, + af.location.start_line, + ) + continue + if af.context and is_code_example(af.context): + if is_non_executable: + logger.debug( + "Filtered code-example finding in non-executable: %s in %s:%d", + af.rule_id, + path, + af.location.start_line, + ) + continue + af.confidence *= _CODE_EXAMPLE_CONFIDENCE_FACTOR + logger.debug( + "Downweighted code-example finding in executable: %s in %s:%d (conf=%.2f)", + af.rule_id, + path, + af.location.start_line, + af.confidence, + ) + if is_doc_markdown: + af.confidence *= _DOCUMENTATION_CONFIDENCE_FACTOR findings.append(analyzer_finding_to_finding(af)) return findings diff --git a/src/skillspector/nodes/analyzers/static_yara.py b/src/skillspector/nodes/analyzers/static_yara.py index f600f6be..891caa0c 100644 --- a/src/skillspector/nodes/analyzers/static_yara.py +++ b/src/skillspector/nodes/analyzers/static_yara.py @@ -70,11 +70,15 @@ def _collect_rule_files(*dirs: Path) -> list[Path]: def _content_hash(rule_files: list[Path]) -> str: - """Fast hash over rule file paths and sizes for cache invalidation.""" + """Hash over rule file paths and content for cache invalidation. + + Uses actual file content (not just size) so that edits which preserve + file length still invalidate the cache. + """ h = hashlib.sha256() for p in rule_files: h.update(str(p).encode()) - h.update(str(p.stat().st_size).encode()) + h.update(p.read_bytes()) return h.hexdigest() diff --git a/src/skillspector/nodes/build_context.py b/src/skillspector/nodes/build_context.py index ed329143..a905844a 100644 --- a/src/skillspector/nodes/build_context.py +++ b/src/skillspector/nodes/build_context.py @@ -90,7 +90,10 @@ def _walk_skill_files(skill_dir: Path) -> list[str]: continue try: rel = item.relative_to(skill_dir) - paths.append(str(rel)) + # Use forward slashes on every OS: these relative paths are dict keys + # and SARIF/URI locations, so they must be portable (not OS-specific + # backslashes on Windows). + paths.append(rel.as_posix()) except ValueError: logger.debug("Skipping path (not under skill_dir): %s", item) continue @@ -167,8 +170,8 @@ def _read_file_cache(skill_dir: Path, components: list[str]) -> dict[str, str]: def _parse_manifest(skill_dir: Path) -> dict[str, object]: """Parse SKILL.md or skill.md YAML frontmatter into a manifest dict. - Returns dict with name, description, triggers (list), permissions (list). - Returns {} if no file or parse fails. + Returns dict with name, description, triggers (list), permissions (list), + allowed-tools (list), parameters (list). Returns {} if no file or parse fails. """ for name in ("SKILL.md", "skill.md"): path = skill_dir / name @@ -203,6 +206,14 @@ def _parse_manifest(skill_dir: Path) -> dict[str, object]: manifest["permissions"] = ( [str(p) for p in permissions] if isinstance(permissions, list) else [] ) + # `allowed-tools` (Agent Skills standard) — accept list or comma string. + allowed_tools = data.get("allowed-tools", []) + if isinstance(allowed_tools, list): + manifest["allowed-tools"] = [str(t).strip() for t in allowed_tools if str(t).strip()] + elif isinstance(allowed_tools, str): + manifest["allowed-tools"] = [t.strip() for t in allowed_tools.split(",") if t.strip()] + else: + manifest["allowed-tools"] = [] # Preserve parameter definitions as dicts so the MCP tool-poisoning # analyzer (TP1/TP2/TP3 parameter checks) can inspect them. Without # this, those checks never fire on real scans because the manifest diff --git a/src/skillspector/nodes/deduplicate.py b/src/skillspector/nodes/deduplicate.py new file mode 100644 index 00000000..abda5133 --- /dev/null +++ b/src/skillspector/nodes/deduplicate.py @@ -0,0 +1,104 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Cross-analyzer finding deduplication. + +Merges findings that represent the same conceptual issue observed multiple +times — either within the same file or across files with identical patterns. + +Deduplication strategy: +1. Same-file dedup: Same rule_id + same file + same matched_text + → keep highest confidence instance +2. Cross-file consolidation: Same rule_id + same matched_text across files + → keep highest confidence instance +""" + +from __future__ import annotations + +from skillspector.logging_config import get_logger +from skillspector.models import Finding + +logger = get_logger(__name__) + + +def _same_file_key(finding: Finding) -> tuple[str, str, str]: + """Build a deduplication key for same-file matches.""" + matched = (finding.matched_text or "").strip()[:100] + return (finding.rule_id, finding.file, matched) + + +def _cross_file_key(finding: Finding) -> tuple[str, str]: + """Build a cross-file deduplication key from rule_id and normalized matched_text.""" + matched = (finding.matched_text or "").strip()[:100] + return (finding.rule_id, matched) + + +def deduplicate(findings: list[Finding]) -> list[Finding]: + """Deduplicate a list of findings, returning a reduced list. + + Two-pass deduplication: + 1. Same-file: identical (rule_id, file, matched_text) → keep highest confidence + 2. Cross-file: identical (rule_id, matched_text) across different files + → keep highest confidence representative + + Findings without matched_text are never cross-file deduplicated (they lack + a reliable identity signal). + """ + if not findings: + return [] + + original_count = len(findings) + + # Pass 1: Same-file deduplication + same_file_best: dict[tuple[str, str, str], Finding] = {} + for f in findings: + key = _same_file_key(f) + existing = same_file_best.get(key) + if existing is None or f.confidence > existing.confidence: + same_file_best[key] = f + + after_same_file = list(same_file_best.values()) + + # Pass 2: Cross-file deduplication (only for findings WITH matched_text) + cross_file_best: dict[tuple[str, str], Finding] = {} + no_text_findings: list[Finding] = [] + + for f in after_same_file: + matched = (f.matched_text or "").strip() + if not matched: + no_text_findings.append(f) + continue + key = _cross_file_key(f) + existing = cross_file_best.get(key) + if existing is None or f.confidence > existing.confidence: + cross_file_best[key] = f + + deduplicated = list(cross_file_best.values()) + no_text_findings + + removed = original_count - len(deduplicated) + if removed > 0: + logger.info( + "Deduplication: %d → %d findings (%d duplicates removed)", + original_count, + len(deduplicated), + removed, + ) + + severity_order = {"CRITICAL": 0, "HIGH": 1, "MEDIUM": 2, "LOW": 3} + deduplicated.sort( + key=lambda f: (severity_order.get(f.severity.upper(), 4), f.file, f.start_line) + ) + + return deduplicated diff --git a/src/skillspector/nodes/meta_analyzer.py b/src/skillspector/nodes/meta_analyzer.py index 8f2b5410..58c5b634 100644 --- a/src/skillspector/nodes/meta_analyzer.py +++ b/src/skillspector/nodes/meta_analyzer.py @@ -39,7 +39,7 @@ get_explanation, get_remediation, ) -from skillspector.state import MetaAnalyzerResponse, SkillspectorState +from skillspector.state import MetaAnalyzerResponse, SkillspectorState, llm_call_record logger = get_logger(__name__) @@ -63,7 +63,20 @@ class MetaAnalyzerFinding(BaseModel): description="The end line number from the finding's Location, if available.", ) is_vulnerability: bool = Field(description="Whether this is a true vulnerability") - confidence: float = Field(ge=0.0, le=1.0, description="Confidence score between 0.0 and 1.0") + # No ge/le bound on purpose: Pydantic bounds emit JSON-schema + # minimum/maximum, which some OpenAI-compatible structured-output endpoints + # reject. The range is enforced by the validator below instead. + confidence: float = Field(description="Confidence score between 0.0 and 1.0") + + @field_validator("confidence", mode="before") + @classmethod + def _normalize_confidence(cls, v: object) -> float: + # Accept 0-100 scale values from some models, then clamp into [0, 1]. + v = float(v) + if v > 2.0: + v = v / 100.0 + return min(1.0, max(0.0, v)) + intent: Literal["malicious", "negligent", "benign"] = Field( description="Likely intent behind the finding" ) @@ -87,6 +100,18 @@ class MetaAnalyzerResult(BaseModel): findings: list[MetaAnalyzerFinding] = Field(default_factory=list) overall_assessment: OverallAssessment | None = None + @field_validator("findings", mode="before") + @classmethod + def _parse_stringified_findings(cls, v: object) -> object: + """LLMs sometimes return the findings array as a JSON string.""" + if isinstance(v, str): + try: + parsed = json.loads(v) + except (json.JSONDecodeError, TypeError): + return [] + return parsed if isinstance(parsed, list) else [] + return v + @field_validator("overall_assessment", mode="before") @classmethod def _parse_stringified_assessment(cls, v: object) -> object: @@ -193,8 +218,70 @@ def _format_findings_for_prompt(findings: list[Finding]) -> str: return "\n".join(lines) +_NO_LLM_CONFIDENCE_THRESHOLD = 0.4 +_HIGH_SEVERITY_PASS_THROUGH = frozenset({"CRITICAL", "HIGH"}) +_CODE_EXAMPLE_DOWNWEIGHT = 0.5 + + def _fallback_filtered(findings: list[Finding]) -> list[Finding]: - """Apply default remediation to all findings (pass-through with defaults).""" + """Heuristic fallback filter for --no-llm mode. + + Applies rule-based filtering when LLM analysis is unavailable: + 1. Drop findings with confidence below threshold (0.4), UNLESS severity + is CRITICAL or HIGH (high-severity findings are never dropped on + confidence alone) + 2. Downweight findings whose context matches code-example indicators + (0.5x confidence reduction) — never hard-drop, as there is no LLM + safety net in this mode + 3. Apply default remediations from pattern_defaults + """ + from skillspector.nodes.analyzers.common import is_code_example + + result: list[Finding] = [] + for f in findings: + severity_upper = (f.severity or "LOW").upper() + confidence = f.confidence + if f.context and is_code_example(f.context): + confidence *= _CODE_EXAMPLE_DOWNWEIGHT + if confidence < _NO_LLM_CONFIDENCE_THRESHOLD: + if severity_upper not in _HIGH_SEVERITY_PASS_THROUGH: + continue + result.append( + Finding( + rule_id=f.rule_id, + message=f.message, + severity=f.severity, + confidence=confidence, + file=f.file, + start_line=f.start_line, + end_line=f.end_line, + remediation=f.remediation or get_remediation(f.rule_id), + tags=f.tags, + context=f.context, + matched_text=f.matched_text, + category=getattr(f, "category", None), + pattern=getattr(f, "pattern", None), + finding=getattr(f, "finding", None), + explanation=getattr(f, "explanation", None), + code_snippet=getattr(f, "code_snippet", None) or f.context, + intent=None, + ) + ) + logger.info( + "Heuristic fallback filter (--no-llm): %d → %d findings", + len(findings), + len(result), + ) + return result + + +def _passthrough_with_defaults(findings: list[Finding]) -> list[Finding]: + """Pass all findings through with default remediations (fail-closed). + + Used on LLM failure path: when the LLM call fails, we pass ALL findings + through unchanged (except adding default remediations). A security tool + should fail-closed — showing more findings is safer than silently dropping. + """ return [ Finding( rule_id=f.rule_id, @@ -266,6 +353,14 @@ def parse_response( # -- Apply filter (keyed by file + rule_id + start/end_line) ------------- + # Severities that must never be silently dropped by LLM filtering. + # Because the LLM receives attacker-controlled skill content, a prompt-injection + # payload could cause it to omit or deny a real CRITICAL/HIGH static finding. + # For these severities a false-negative (hiding a real vulnerability) is far + # worse than a false-positive, so we keep the original static finding regardless + # of what the LLM says and mark it "llm-unconfirmed" via the tags field. + _HIGH_SEVERITY_FLOOR = frozenset({"CRITICAL", "HIGH"}) + def apply_filter( self, findings: list[Finding], @@ -279,9 +374,20 @@ def apply_filter( is included in the key when provided but falls back to ``None`` so callers that omit it still match. Falls back to coarse ``(file, rule_id)`` keying for LLM responses that omit ``start_line``. + + Severity-gated floor (security invariant) + ------------------------------------------ + CRITICAL and HIGH static findings are **always** kept in the output even + if the LLM did not confirm them. When the LLM omits or denies such a + finding the original static finding is preserved unchanged and the tag + ``"llm-unconfirmed"`` is appended so consumers can distinguish it from + LLM-validated findings. MEDIUM and LOW findings continue to be filtered + by the LLM as before (false-positive reduction). """ _enrichment = tuple[str, str, float] confirmed_granular: dict[tuple[str, str, int, int | None], _enrichment] = {} + # Fallback index keyed without end_line (see lookup below). Issue #67. + confirmed_by_start: dict[tuple[str, str, int], _enrichment] = {} confirmed_coarse: dict[tuple[str, str], _enrichment] = {} for batch, llm_items in batch_results: @@ -308,6 +414,7 @@ def apply_filter( int(end_line) if end_line is not None else None, ) ] = enrichment + confirmed_by_start[(file_path, pattern_id, int(start_line))] = enrichment else: confirmed_coarse[(file_path, pattern_id)] = enrichment @@ -316,13 +423,47 @@ def apply_filter( exact_key = (f.file, f.rule_id, f.start_line, f.end_line) start_only_key = (f.file, f.rule_id, f.start_line, None) coarse_key = (f.file, f.rule_id) + start_key = (f.file, f.rule_id, f.start_line) if f.start_line is not None else None if exact_key in confirmed_granular: expl, rem, conf = confirmed_granular[exact_key] elif start_only_key in confirmed_granular: expl, rem, conf = confirmed_granular[start_only_key] + elif f.end_line is None and start_key is not None and start_key in confirmed_by_start: + expl, rem, conf = confirmed_by_start[start_key] elif coarse_key in confirmed_coarse: expl, rem, conf = confirmed_coarse[coarse_key] else: + # Security: CRITICAL/HIGH static findings must survive LLM filtering. + # A prompt-injection payload in the scanned skill could cause the LLM + # to deny or omit a real high-severity finding; silently dropping it + # would be a false-negative in a security gate. Keep the original + # finding and tag it so consumers know it was not LLM-validated. + if f.severity in self._HIGH_SEVERITY_FLOOR: + unconfirmed_tags = list(f.tags) + if "llm-unconfirmed" not in unconfirmed_tags: + unconfirmed_tags.append("llm-unconfirmed") + result.append( + Finding( + rule_id=f.rule_id, + message=f.message, + severity=f.severity, + confidence=f.confidence, + file=f.file, + start_line=f.start_line, + end_line=f.end_line, + remediation=f.remediation or get_remediation(f.rule_id), + tags=unconfirmed_tags, + context=f.context, + matched_text=f.matched_text, + category=getattr(f, "category", None), + pattern=getattr(f, "pattern", None), + finding=getattr(f, "finding", None), + explanation=getattr(f, "explanation", None), + code_snippet=getattr(f, "code_snippet", None) or f.context, + intent=None, + ) + ) + # MEDIUM/LOW: preserve existing behaviour (LLM may filter as false-positive). continue result.append( Finding( @@ -380,9 +521,11 @@ def meta_analyzer(state: SkillspectorState) -> MetaAnalyzerResponse: metadata_text = _format_metadata(manifest) files_with_findings = sorted({f.file for f in findings}) - analyzer = LLMMetaAnalyzer(model=model) - try: + # Construct inside the try so a chat-model construction failure is caught + # and recorded as a degraded LLM call (consistent with the semantic + # analyzers) rather than crashing the whole graph. + analyzer = LLMMetaAnalyzer(model=model) batches = analyzer.get_batches(files_with_findings, file_cache, findings) logger.debug( "Meta-analyzer: %d files -> %d batches (model=%s)", @@ -392,16 +535,43 @@ def meta_analyzer(state: SkillspectorState) -> MetaAnalyzerResponse: ) batch_results = asyncio.run(analyzer.arun_batches(batches, metadata_text=metadata_text)) - filtered = analyzer.apply_filter(findings, batch_results) + + if len(batch_results) < len(batches): + # Some batches never returned. A finding the LLM never saw has no + # verdict — keep it via the fallback path instead of letting + # apply_filter treat the missing confirmation as a rejection. + analysed_ids = {id(f) for batch, _ in batch_results for f in batch.findings} + analysed = [f for f in findings if id(f) in analysed_ids] + unanalysed = [f for f in findings if id(f) not in analysed_ids] + else: + analysed, unanalysed = findings, [] + + filtered = analyzer.apply_filter(analysed, batch_results) + if unanalysed: + logger.warning( + "Meta-analyzer: %d/%d batches failed; keeping %d findings in %d " + "files unfiltered (no LLM verdict)", + len(batches) - len(batch_results), + len(batches), + len(unanalysed), + len({f.file for f in unanalysed}), + ) + filtered.extend(_fallback_filtered(unanalysed)) logger.debug( "LLM filtering done: %d findings -> %d after filter", len(findings), len(filtered), ) - return {"filtered_findings": filtered} + return { + "filtered_findings": filtered, + "llm_call_log": [llm_call_record("meta_analyzer", ok=True)], + } except ValueError: raise except Exception as e: - logger.warning("LLM call failed, using fallback: %s", e) - return {"filtered_findings": _fallback_filtered(findings)} + logger.warning("LLM call failed, passing all findings through (fail-closed): %s", e) + return { + "filtered_findings": _passthrough_with_defaults(findings), + "llm_call_log": [llm_call_record("meta_analyzer", ok=False, error=str(e))], + } diff --git a/src/skillspector/nodes/report.py b/src/skillspector/nodes/report.py index ec893ffb..95160398 100644 --- a/src/skillspector/nodes/report.py +++ b/src/skillspector/nodes/report.py @@ -22,6 +22,8 @@ from __future__ import annotations import json +import re +from dataclasses import replace from datetime import UTC, datetime from io import StringIO from typing import Literal @@ -34,20 +36,26 @@ from skillspector.llm_utils import is_llm_available from skillspector.logging_config import get_logger from skillspector.models import Finding +from skillspector.nodes.deduplicate import deduplicate from skillspector.sarif_models import ( SARIF_SCHEMA_URI, SarifArtifactLocation, SarifDriver, + SarifInvocation, SarifLocation, SarifLog, SarifMessage, + SarifNotification, SarifPhysicalLocation, SarifRegion, + SarifReportingDescriptor, SarifResult, SarifRun, + SarifSuppression, SarifTool, ) from skillspector.state import SkillspectorState +from skillspector.suppression import Baseline, SuppressedFinding, partition_findings logger = get_logger(__name__) @@ -61,6 +69,38 @@ } +# Scanned skill content (and LLM output that quotes it) can carry ANSI escape +# sequences and control bytes (e.g. NUL, ESC) into finding text. Left in, they +# make a rendered report register as binary — GitLab/editors show "download" +# instead of the Markdown, and terminals emit garbled output. Strip them from +# every finding text field so all report formats stay clean UTF-8. +_ANSI_RE = re.compile(r"\x1b\[[0-9;?]*[ -/]*[@-~]") +_CONTROL_RE = re.compile(r"[\x00-\x08\x0b\x0c\x0e-\x1f\x7f]") + +# Finding free-text fields that may carry scanned/LLM content. +_SANITIZED_FIELDS = ( + "message", + "explanation", + "remediation", + "finding", + "context", + "matched_text", + "code_snippet", +) + + +def _clean_text(value: str | None) -> str | None: + """Strip ANSI escape sequences and disallowed control chars (keep tab/newline).""" + if not isinstance(value, str): + return value + return _CONTROL_RE.sub("", _ANSI_RE.sub("", value)) + + +def _sanitize_finding(finding: Finding) -> Finding: + """Return a copy of *finding* with control/ANSI bytes stripped from text fields.""" + return replace(finding, **{f: _clean_text(getattr(finding, f)) for f in _SANITIZED_FIELDS}) + + def _severity_to_sarif_level(severity: str) -> Literal["error", "warning", "note"]: """Map Finding.severity to SARIF result level.""" return { @@ -71,41 +111,114 @@ def _severity_to_sarif_level(severity: str) -> Literal["error", "warning", "note }.get(severity.upper(), "note") # type: ignore[return-value] +_SEVERITY_POINTS: dict[str, int] = { + "CRITICAL": 50, + "HIGH": 25, + "MEDIUM": 10, + "LOW": 5, +} + +_MAX_OCCURRENCES_PER_RULE = 3 +_DIMINISHING_WEIGHTS = (1.0, 0.5, 0.25) + + def _compute_risk_score( - findings: list[Finding], has_executable_scripts: bool + findings: list[Finding], + has_executable_scripts: bool, + component_metadata: list[dict[str, object]] | None = None, ) -> tuple[int, str, str]: """ Compute risk score (0-100), severity band, and recommendation. - v1 rules: CRITICAL +50, HIGH +25, MEDIUM +10, LOW +5; 1.3x if has_executable_scripts. + + Scoring uses per-rule diminishing returns: the first occurrence of a rule_id + contributes full points, the second contributes half, and the third contributes + a quarter. Occurrences beyond the third are ignored for scoring purposes. + This prevents repeated pattern matches from inflating the score unboundedly. + + Each finding's contribution is also scaled by its confidence value (clamped + to [0, 1]). Findings with confidence <= 0 are skipped entirely — they do not + contribute to the score but remain in the reported findings list. + + Within each rule_id bucket, findings are processed in severity-descending + order so that the highest-severity occurrence always receives the full weight. + + Base points per severity: CRITICAL=50, HIGH=25, MEDIUM=10, LOW=5. + 1.3x multiplier applied only to findings from executable script files; + findings from documentation files (markdown, text, json, yaml, toml) + are scored at base weight to avoid punishing security documentation. """ - score = 0 - for f in findings: + severity_rank = {"CRITICAL": 0, "HIGH": 1, "MEDIUM": 2, "LOW": 3} + sorted_findings = sorted( + findings, + key=lambda f: (f.rule_id or "UNKNOWN", severity_rank.get((f.severity or "LOW").upper(), 4)), + ) + + file_executable: dict[str, bool] = {} + if component_metadata: + for cm in component_metadata: + file_executable[str(cm.get("path", ""))] = bool(cm.get("executable", False)) + + rule_occurrence_count: dict[str, int] = {} + score = 0.0 + + for f in sorted_findings: + confidence = max(0.0, min(1.0, f.confidence)) + if confidence <= 0.0: + continue + sev = (f.severity or "LOW").upper() - if sev == "CRITICAL": - score += 50 - elif sev == "HIGH": - score += 25 - elif sev == "MEDIUM": - score += 10 - elif sev == "LOW": - score += 5 - if has_executable_scripts: - score = int(score * 1.3) - score = min(100, max(0, score)) + base_points = _SEVERITY_POINTS.get(sev, 5) + + rule_id = f.rule_id or "UNKNOWN" + count = rule_occurrence_count.get(rule_id, 0) + rule_occurrence_count[rule_id] = count + 1 + + if count >= _MAX_OCCURRENCES_PER_RULE: + continue + + weight = _DIMINISHING_WEIGHTS[count] + contribution = base_points * weight * confidence + + # Apply 1.3x multiplier only to findings from executable files + if has_executable_scripts and file_executable.get(f.file, False): + contribution *= 1.3 + + score += contribution + + final_score = min(100, max(0, int(score))) severity_band = "LOW" for threshold, band in _RISK_SEVERITY_BANDS: - if score >= threshold: + if final_score >= threshold: severity_band = band break recommendation = _RISK_RECOMMENDATION.get(severity_band, "CAUTION") - return score, severity_band, recommendation + return final_score, severity_band, recommendation -def _build_sarif(findings: list[Finding]) -> dict[str, object]: - """Build SARIF 2.1.0 log from findings.""" +def _build_sarif( + findings: list[Finding], + suppressed: list[SuppressedFinding] | None = None, + degraded_notice: str | None = None, +) -> dict[str, object]: + """Build SARIF 2.1.0 log from findings. + + Filters out empty/malformed findings (missing rule_id or message) and + builds the required tool.driver.rules[] array from referenced rule IDs. + + When *degraded_notice* is set (the LLM stage was requested but every call + failed), a single ``invocation`` is added carrying the notice as a + warning-level ``toolExecutionNotifications`` entry — the standard SARIF + place for execution-time conditions — so the default output format also + surfaces the degradation. ``executionSuccessful`` stays True: the scan + completed and produced results; only the LLM sub-stage was degraded. + """ results: list[SarifResult] = [] + seen_rule_ids: dict[str, str] = {} + for finding in findings: + if not finding.rule_id or not finding.message: + continue start_line = finding.start_line end_line = finding.end_line region = SarifRegion(start_line=start_line, end_line=end_line) @@ -124,14 +237,68 @@ def _build_sarif(findings: list[Finding]) -> dict[str, object]: ], ) ) + if finding.rule_id not in seen_rule_ids: + seen_rule_ids[finding.rule_id] = finding.message + + # Baseline-suppressed findings are kept in the SARIF for an audit trail, but + # marked with the `suppressions` property so consumers exclude them from counts. + for sf in suppressed or []: + finding = sf.finding + if not finding.rule_id or not finding.message: + continue + results.append( + SarifResult( + rule_id=finding.rule_id, + message=SarifMessage(text=finding.message), + level=_severity_to_sarif_level(finding.severity), + locations=[ + SarifLocation( + physical_location=SarifPhysicalLocation( + artifact_location=SarifArtifactLocation(uri=finding.file), + region=SarifRegion( + start_line=finding.start_line, end_line=finding.end_line + ), + ) + ) + ], + suppressions=[SarifSuppression(kind="external", justification=sf.reason)], + ) + ) + if finding.rule_id not in seen_rule_ids: + seen_rule_ids[finding.rule_id] = finding.message + + rules = [ + SarifReportingDescriptor( + id=rule_id, + short_description=SarifMessage(text=description), + ) + for rule_id, description in sorted(seen_rule_ids.items()) + ] + + invocations: list[SarifInvocation] | None = None + if degraded_notice: + invocations = [ + SarifInvocation( + execution_successful=True, + tool_execution_notifications=[ + SarifNotification(text=SarifMessage(text=degraded_notice), level="warning") + ], + ) + ] + sarif_log = SarifLog( schema_=SARIF_SCHEMA_URI, runs=[ SarifRun( tool=SarifTool( - driver=SarifDriver(name="skillspector", version=skillspector_version) + driver=SarifDriver( + name="skillspector", + version=skillspector_version, + rules=rules if rules else None, + ) ), results=results, + invocations=invocations, ) ], ) @@ -147,8 +314,13 @@ def _format_terminal( risk_severity: str, risk_recommendation: str, has_executable_scripts: bool, + use_llm: bool = True, + llm_call_log: list[dict[str, object]] | None = None, + suppressed: list[SuppressedFinding] | None = None, + show_suppressed: bool = False, ) -> str: """Generate Rich terminal output and export as string.""" + suppressed = suppressed or [] console = Console(record=True, force_terminal=True, width=80, file=StringIO()) skill_name = (manifest.get("name") or "unknown") if manifest else "unknown" source = skill_path or "" @@ -199,6 +371,17 @@ def _format_terminal( comp_table.add_row(f"... and {len(component_metadata) - 15} more", "", "", "") console.print(comp_table) + degraded_notice = _llm_degradation_notice(use_llm, llm_call_log or []) + if degraded_notice: + console.print() + console.print( + Panel( + f"[bold]Degraded scan[/bold]\n{degraded_notice}", + title="[bold red]WARNING[/bold red]", + border_style="red", + ) + ) + if findings: console.print("\n") console.print(f"[bold]Issues ({len(findings)})[/bold]\n") @@ -220,24 +403,138 @@ def _format_terminal( else: console.print("\n[green]No security issues detected.[/green]\n") + if suppressed: + console.print( + f"[dim]Suppressed by baseline: {len(suppressed)} (not counted toward risk score)[/dim]" + ) + if show_suppressed: + for sf in suppressed: + f = sf.finding + console.print( + f" [dim]- {f.rule_id} {f.file}:{f.start_line} (reason: {sf.reason})[/dim]" + ) + else: + console.print("[dim]Use --show-suppressed to list them.[/dim]") + console.print() + console.print(f"[dim]Executable scripts: {'Yes' if has_executable_scripts else 'No'}[/dim]") return console.export_text() -def _build_metadata(has_executable_scripts: bool, use_llm: bool) -> dict[str, object]: +def _llm_runtime_status( + use_llm: bool, llm_call_log: list[dict[str, object]] +) -> tuple[int, int, bool]: + """Return ``(attempted, succeeded, degraded)`` from the LLM call log. + + ``degraded`` is True when the LLM stage was requested and at least one call + was attempted, but every call failed at runtime — meaning the report + reflects static analysis only despite a deep scan being requested. + """ + attempted = len(llm_call_log) + succeeded = sum(1 for r in llm_call_log if r.get("ok")) + degraded = bool(use_llm and attempted > 0 and succeeded == 0) + return attempted, succeeded, degraded + + +def _llm_degradation_notice(use_llm: bool, llm_call_log: list[dict[str, object]]) -> str | None: + """Return a human-readable degraded-scan warning, or None if not degraded.""" + attempted, _succeeded, degraded = _llm_runtime_status(use_llm, llm_call_log) + if not degraded: + return None + return ( + f"LLM analysis was requested but all {attempted} LLM call(s) failed - " + "results reflect STATIC analysis only." + ) + + +def _build_metadata( + has_executable_scripts: bool, + use_llm: bool, + llm_call_log: list[dict[str, object]] | None = None, +) -> dict[str, object]: """Build the metadata section shared by all output formats.""" + llm_call_log = llm_call_log or [] llm_available, llm_error = is_llm_available() + attempted, succeeded, degraded = _llm_runtime_status(use_llm, llm_call_log) + # meta_analysis_applied reflects whether the LLM meta-analysis effectively + # ran: requested, available, and not fully degraded (every call failing). + meta_analysis_applied = use_llm and llm_available and not degraded + meta: dict[str, object] = { "has_executable_scripts": has_executable_scripts, "skillspector_version": skillspector_version, "llm_requested": use_llm, - "llm_available": llm_available, + # llm_available reflects runtime truth: the binary/credentials were + # available AND the stage was not fully degraded (every call failing). + "llm_available": llm_available and not degraded, + "meta_analysis_applied": meta_analysis_applied, } - if use_llm and not llm_available: + if not meta_analysis_applied: + meta["filtering_mode"] = "heuristic" + if use_llm and attempted: + meta["llm_calls_attempted"] = attempted + meta["llm_calls_succeeded"] = succeeded + if degraded: + meta["llm_degraded"] = True + reasons = sorted( + {str(r.get("error")) for r in llm_call_log if not r.get("ok") and r.get("error")} + ) + detail = f" Reasons: {'; '.join(reasons)}" if reasons else "" + meta["llm_error"] = ( + f"LLM analysis was requested but all {attempted} LLM call(s) failed; " + f"results reflect static analysis only.{detail}" + ) + elif use_llm and not llm_available: meta["llm_error"] = llm_error return meta +def _build_analysis_completeness( + components: list[str], + file_cache: dict[str, str], + use_llm: bool, + findings_pre_filter: list[Finding], + findings_post_filter: list[Finding], +) -> dict[str, object]: + """Build analysis_completeness section indicating scan coverage and limitations. + + Helps consumers understand what was NOT analyzed and whether findings + can be trusted as comprehensive. + """ + total_components = len(components) + scanned_components = sum(1 for c in components if c in file_cache) + + llm_available, llm_error = is_llm_available() + llm_used = use_llm and llm_available + + limitations: list[str] = [] + if scanned_components < total_components: + skipped = total_components - scanned_components + limitations.append(f"{skipped} component(s) had no content in file_cache (skipped)") + if use_llm and not llm_available: + limitations.append(f"LLM meta-analysis unavailable: {llm_error or 'unknown reason'}") + if not use_llm: + limitations.append("LLM meta-analysis was disabled (--no-llm)") + + findings_dropped = len(findings_pre_filter) - len(findings_post_filter) + if findings_dropped > 0: + limitations.append(f"{findings_dropped} finding(s) filtered by meta-analyzer or heuristics") + + completeness: dict[str, object] = { + "total_components": total_components, + "scanned_components": scanned_components, + "coverage_percent": round(scanned_components / total_components * 100, 1) + if total_components > 0 + else 100.0, + "llm_analysis": "applied" if llm_used else "skipped", + "findings_before_filtering": len(findings_pre_filter), + "findings_after_filtering": len(findings_post_filter), + "limitations": limitations if limitations else None, + "is_complete": len(limitations) == 0, + } + return completeness + + def _format_json( findings: list[Finding], component_metadata: list[dict[str, object]], @@ -248,8 +545,12 @@ def _format_json( risk_recommendation: str, has_executable_scripts: bool, use_llm: bool = True, + llm_call_log: list[dict[str, object]] | None = None, + analysis_completeness: dict[str, object] | None = None, + suppressed: list[SuppressedFinding] | None = None, ) -> str: """Generate JSON report string.""" + suppressed = suppressed or [] skill_name = (manifest.get("name") or "unknown") if manifest else "unknown" data: dict[str, object] = { "skill": { @@ -273,8 +574,12 @@ def _format_json( for c in component_metadata ], "issues": [f.to_dict() for f in findings], - "metadata": _build_metadata(has_executable_scripts, use_llm), + "suppressed_count": len(suppressed), + "suppressed": [sf.to_dict() for sf in suppressed], + "metadata": _build_metadata(has_executable_scripts, use_llm, llm_call_log), } + if analysis_completeness is not None: + data["analysis_completeness"] = analysis_completeness return json.dumps(data, indent=2) @@ -287,8 +592,13 @@ def _format_markdown( risk_severity: str, risk_recommendation: str, has_executable_scripts: bool, + use_llm: bool = True, + llm_call_log: list[dict[str, object]] | None = None, + suppressed: list[SuppressedFinding] | None = None, + show_suppressed: bool = False, ) -> str: """Generate Markdown report string.""" + suppressed = suppressed or [] lines: list[str] = [] skill_name = (manifest.get("name") or "unknown") if manifest else "unknown" source = skill_path or "" @@ -299,6 +609,11 @@ def _format_markdown( lines.append(f"**Scanned:** {datetime.now(UTC).strftime('%Y-%m-%d %H:%M:%S UTC')} ") lines.append("") + degraded_notice = _llm_degradation_notice(use_llm, llm_call_log or []) + if degraded_notice: + lines.append(f"> ⚠️ **Degraded scan:** {degraded_notice}") + lines.append("") + lines.append("## Risk Assessment\n") lines.append("| Metric | Value |") lines.append("|--------|-------|") @@ -339,6 +654,22 @@ def _format_markdown( lines.append("") lines.append("---\n") + if suppressed: + lines.append(f"## Suppressed ({len(suppressed)})\n") + lines.append( + "These findings matched the baseline and are **not** counted toward the risk score.\n" + ) + if show_suppressed: + lines.append("| Rule | Location | Reason |") + lines.append("|------|----------|--------|") + for sf in suppressed: + f = sf.finding + reason = sf.reason.replace("|", "\\|") + lines.append(f"| {f.rule_id} | `{f.file}:{f.start_line}` | {reason} |") + lines.append("") + else: + lines.append("_Run with `--show-suppressed` to list them._\n") + lines.append("## Metadata\n") lines.append(f"- **Executable Scripts:** {'Yes' if has_executable_scripts else 'No'}") lines.append(f"\n*Generated by SkillSpector v{skillspector_version}*") @@ -346,28 +677,73 @@ def _format_markdown( def report(state: SkillspectorState) -> dict[str, object]: - """Generate SARIF, compute risk score, and set report_body from output_format.""" - findings = state.get("filtered_findings", state.get("findings", [])) - # When use_llm is False, meta_analyzer is skipped; ensure final state has filtered_findings - if "filtered_findings" not in state: - filtered_findings = state.get("findings", []) - else: - filtered_findings = findings + """Generate SARIF, compute risk score, and set report_body from output_format. + + A baseline (state["baseline"]) suppresses matching findings: they never count + toward the risk score and are excluded from SARIF. They are shown in the + human-readable report only when state["show_suppressed"] is True. + """ + raw_findings = state.get("findings", []) + filtered_findings = state.get("filtered_findings", raw_findings) + # Strip ANSI/control bytes once here so every downstream format (terminal, + # json, markdown, sarif) and the returned findings stay clean UTF-8. Applied + # before partition/dedup so active and suppressed findings are both clean. + filtered_findings = [_sanitize_finding(f) for f in filtered_findings] component_metadata = state.get("component_metadata") or [] + components = state.get("components") or [] + file_cache = state.get("file_cache") or {} has_executable_scripts = state.get("has_executable_scripts", False) manifest = state.get("manifest") or {} skill_path = state.get("skill_path") output_format = state.get("output_format") or "sarif" use_llm = state.get("use_llm", True) + llm_call_log = state.get("llm_call_log") or [] + + # Surface a silent degradation: deep scan requested but every LLM call failed + # at runtime, so the report reflects static analysis only. Logged here (once, + # operationally) regardless of output format; also embedded in each format's + # body / metadata below. + _attempted, _succeeded, degraded = _llm_runtime_status(use_llm, llm_call_log) + degraded_notice = _llm_degradation_notice(use_llm, llm_call_log) + if degraded: + logger.warning( + "LLM stage degraded: %d/%d LLM call(s) failed; report reflects static " + "analysis only (llm_available reported false)", + _attempted - _succeeded, + _attempted, + ) + baseline = state.get("baseline") + show_suppressed = state.get("show_suppressed", False) + active_findings, suppressed = partition_findings( + filtered_findings, baseline if isinstance(baseline, Baseline) else None + ) + + # Risk and SARIF reflect only the active (non-suppressed) findings; scoring + # additionally de-duplicates so the same issue is not counted twice. + findings_for_scoring = deduplicate(active_findings) risk_score, risk_severity, risk_recommendation = _compute_risk_score( - findings, has_executable_scripts + findings_for_scoring, has_executable_scripts, component_metadata + ) + sarif_report = _build_sarif(active_findings, suppressed, degraded_notice=degraded_notice) + analysis_completeness = _build_analysis_completeness( + components, file_cache, use_llm, raw_findings, filtered_findings ) - sarif_report = _build_sarif(findings) + + # Fail closed on a degraded deep scan: when the LLM stage was requested but + # every call failed, the semantic analyzers were effectively skipped, so a + # SAFE verdict would rest on static analysis alone. An attacker can trigger + # this on purpose (e.g. content that breaks the LLM call) to dodge semantic + # scrutiny. Floor the recommendation at CAUTION so an install-gate ASKS + # rather than auto-allows; risk_score / severity are left untouched (they + # honestly reflect what static analysis found), and llm_degraded / llm_error + # explain why the verdict was raised. + if degraded and risk_recommendation == "SAFE": + risk_recommendation = "CAUTION" if output_format == "terminal": report_body = _format_terminal( - findings, + active_findings, component_metadata, manifest, skill_path, @@ -375,10 +751,14 @@ def report(state: SkillspectorState) -> dict[str, object]: risk_severity, risk_recommendation, has_executable_scripts, + use_llm=use_llm, + llm_call_log=llm_call_log, + suppressed=suppressed, + show_suppressed=show_suppressed, ) elif output_format == "json": report_body = _format_json( - findings, + active_findings, component_metadata, manifest, skill_path, @@ -387,10 +767,13 @@ def report(state: SkillspectorState) -> dict[str, object]: risk_recommendation, has_executable_scripts, use_llm=use_llm, + llm_call_log=llm_call_log, + analysis_completeness=analysis_completeness, + suppressed=suppressed, ) elif output_format == "markdown": report_body = _format_markdown( - findings, + active_findings, component_metadata, manifest, skill_path, @@ -398,14 +781,19 @@ def report(state: SkillspectorState) -> dict[str, object]: risk_severity, risk_recommendation, has_executable_scripts, + use_llm=use_llm, + llm_call_log=llm_call_log, + suppressed=suppressed, + show_suppressed=show_suppressed, ) else: report_body = json.dumps(sarif_report, indent=2) logger.debug( - "Report generated: format=%s, findings_count=%d", + "Report generated: format=%s, findings_count=%d, suppressed_count=%d", output_format, - len(filtered_findings), + len(active_findings), + len(suppressed), ) out: dict[str, object] = { @@ -415,5 +803,6 @@ def report(state: SkillspectorState) -> dict[str, object]: "risk_recommendation": risk_recommendation, "report_body": report_body, "filtered_findings": filtered_findings, + "suppressed_findings": suppressed, } return out diff --git a/src/skillspector/nodes/resolve_input.py b/src/skillspector/nodes/resolve_input.py index a4e7555b..7324d847 100644 --- a/src/skillspector/nodes/resolve_input.py +++ b/src/skillspector/nodes/resolve_input.py @@ -65,7 +65,7 @@ def resolve_input(state: SkillspectorState) -> dict[str, object]: "temp_dir_for_cleanup": None, } except (OSError, RuntimeError) as e: - logger.warning("Could not resolve skill_path %s: %s", skill_path, e) + logger.warning("Could not resolve skill_path: %s", e) return {"skill_path": None, "temp_dir_for_cleanup": None} return {"skill_path": None, "temp_dir_for_cleanup": None} diff --git a/src/skillspector/providers/__init__.py b/src/skillspector/providers/__init__.py index bf1522e6..809884dc 100644 --- a/src/skillspector/providers/__init__.py +++ b/src/skillspector/providers/__init__.py @@ -22,11 +22,25 @@ Selection happens via the ``SKILLSPECTOR_PROVIDER`` env var: - openai → OpenAIProvider (api.openai.com) - anthropic → AnthropicProvider (api.anthropic.com) - nv_build → NvBuildProvider (build.nvidia.com) + openai → OpenAIProvider (api.openai.com) + anthropic → AnthropicProvider (api.anthropic.com) + anthropic_proxy → AnthropicProxyProvider (Vertex-style raw-predict proxy) + bedrock → BedrockProvider (AWS Bedrock Runtime, SigV4) + nv_build → NvBuildProvider (build.nvidia.com) + claude_cli → ClaudeCLIProvider (local ``claude`` binary, no API key) + codex_cli → CodexCLIProvider (local ``codex`` binary, no API key) + gemini_cli → GeminiCLIProvider (local ``gemini`` binary, no API key) + antigravity_cli → AntigravityCLIProvider (local ``agy`` binary; registered + but disabled — agy is TTY-only and + can't be captured; use gemini_cli) When unset, the selector defaults to ``nv_build``. + +CLI providers (``claude_cli``, ``codex_cli``, ``gemini_cli``) implement the +optional :class:`~skillspector.providers.base.AgentCLICapable` interface — they +expose ``is_available()`` and ``complete()`` so that +:func:`skillspector.llm_utils.get_chat_model` uses the local CLI subprocess +instead of the ``ChatOpenAI`` HTTP transport. """ from __future__ import annotations @@ -36,7 +50,14 @@ from langchain_core.language_models.chat_models import BaseChatModel -from .base import ChatModelProvider, CredentialsProvider, LLMProvider, ModelMetadataProvider +from .base import ( + AgentCLICapable, + ChatModelProvider, + CredentialsProvider, + LLMProvider, + ModelMetadataProvider, + has_cli_capability, +) from .nv_build import NvBuildProvider NO_LLM_API_KEY_MESSAGE = ( @@ -64,8 +85,32 @@ def _select_active_provider() -> LLMProvider: from .anthropic import AnthropicProvider return AnthropicProvider() + if name == "anthropic_proxy": + from .anthropic_proxy import AnthropicProxyProvider + + return AnthropicProxyProvider() + if name == "bedrock": + from .bedrock import BedrockProvider + + return BedrockProvider() if name == "nv_build": return NvBuildProvider() + if name == "claude_cli": + from .claude_cli import ClaudeCLIProvider + + return ClaudeCLIProvider() + if name == "codex_cli": + from .codex_cli import CodexCLIProvider + + return CodexCLIProvider() + if name == "gemini_cli": + from .gemini_cli import GeminiCLIProvider + + return GeminiCLIProvider() + if name == "antigravity_cli": + from .antigravity_cli import AntigravityCLIProvider + + return AntigravityCLIProvider() if name in ("nv_inference", ""): # Try the optional nv_inference subpackage if it's bundled with # this installation; otherwise fall through to nv_build. @@ -78,7 +123,8 @@ def _select_active_provider() -> LLMProvider: raise ValueError( f"Unknown SKILLSPECTOR_PROVIDER: {name!r}. " - "Expected one of: openai, anthropic, nv_build (or unset)." + "Expected one of: openai, anthropic, anthropic_proxy, bedrock, nv_build, " + "claude_cli, codex_cli, gemini_cli, antigravity_cli (or unset)." ) @@ -87,11 +133,22 @@ def get_metadata_provider() -> ModelMetadataProvider: return _select_active_provider() +def get_active_provider() -> ModelMetadataProvider: + """Return the active provider (alias for :func:`get_metadata_provider`). + + Preferred over :func:`get_metadata_provider` when callers also need to + check for optional capabilities (e.g. :func:`has_cli_capability`). + """ + return _select_active_provider() + + def resolve_provider_credentials() -> tuple[str, str | None] | None: """Return ``(api_key, base_url)`` from the active provider. Returns ``None`` when the provider's credential env var is unset, so - callers can fall through to other credential sources. + callers can fall through to other credential sources. CLI providers + always return ``None`` from this method; availability is checked via + ``is_available()`` instead. """ return _select_active_provider().resolve_credentials() @@ -120,37 +177,48 @@ def create_chat_model( ) -> BaseChatModel: """Create the active provider's native LangChain chat model. + CLI providers (``claude_cli``, ``codex_cli``, ``gemini_cli``) do not have + a native LangChain chat model — callers that need CLI transport should use + :func:`skillspector.llm_utils.get_chat_model` instead (which returns an + :class:`~skillspector.llm_utils.AgentCLIChatModel` adapter). + If the active provider is not configured, fall back to standard OpenAI environment variables. This preserves the historical ``OPENAI_API_KEY`` escape hatch while letting configured providers choose their own client. """ provider = _select_active_provider() - llm = provider.create_chat_model(model, max_tokens=max_tokens, timeout=timeout) - if llm is not None: - return llm - from .openai import OpenAIProvider - - if not isinstance(provider, OpenAIProvider): - llm = _openai_fallback_provider().create_chat_model( - model, - max_tokens=max_tokens, - timeout=timeout, - ) + # CLI providers don't participate in the create_chat_model path. + if not has_cli_capability(provider): + llm = provider.create_chat_model(model, max_tokens=max_tokens, timeout=timeout) if llm is not None: return llm + from .openai import OpenAIProvider + + if not isinstance(provider, OpenAIProvider): + llm = _openai_fallback_provider().create_chat_model( + model, + max_tokens=max_tokens, + timeout=timeout, + ) + if llm is not None: + return llm + raise_no_llm_api_key_configured() __all__ = [ + "AgentCLICapable", "ChatModelProvider", "CredentialsProvider", "LLMProvider", "ModelMetadataProvider", "NO_LLM_API_KEY_MESSAGE", "create_chat_model", + "get_active_provider", "get_metadata_provider", + "has_cli_capability", "raise_no_llm_api_key_configured", "resolve_chat_model_credentials", "resolve_provider_credentials", diff --git a/src/skillspector/providers/_agent_cli.py b/src/skillspector/providers/_agent_cli.py new file mode 100644 index 00000000..d7aa415d --- /dev/null +++ b/src/skillspector/providers/_agent_cli.py @@ -0,0 +1,805 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Hardened subprocess helper for agent CLI providers (claude, codex, gemini). + +This is the single security chokepoint for all agent-CLI calls. Per-CLI +knowledge (argv, output parsing, auth check) lives in a small ``CliSpec`` +registry (see ``_REGISTRY`` / "HOW TO ADD A NEW AGENT CLI" below); the +security core is CLI-agnostic. Every call goes through :func:`run_agent_cli` +which enforces: + +- **No shell**: ``shell=False`` with an explicit argv list. +- **Untrusted content via stdin only**: the prompt (which may contain + adversarial skill content) is written to the process stdin, never + injected into argv. +- **Capability stripping** (per-binary): tools disabled, MCP disabled, + no extra directories, deny permission mode (claude); read-only sandbox + (codex). ``--dangerously-skip-permissions`` is NEVER used. +- **Environment scrubbing**: API keys, SSH keys, cloud credentials, and + other secrets are stripped from the child environment. +- **Timeout enforcement**: the call raises ``TimeoutError`` rather than + hanging indefinitely. +- **Input / output caps**: prompt exceeding ``MAX_INPUT_BYTES`` is + rejected; stdout is capped at ``MAX_OUTPUT_BYTES``. +- **Fail-closed**: non-zero exit, timeout, missing binary, or bad + output all raise ``AgentCLIError``. +- **Prompt-layer hardening**: the caller wraps untrusted content in + clear DATA delimiters before passing it here (defense-in-depth on top + of capability removal). + +The JSON output envelope (``claude -p --output-format json``) is parsed +and the assistant text is returned. ``codex exec --json`` produces +JSONL events; the last assistant message is extracted. +""" + +from __future__ import annotations + +import json +import os +import shutil +import subprocess +import tempfile +import threading +from collections.abc import Callable +from dataclasses import dataclass +from typing import Any + +from skillspector.logging_config import get_logger + +logger = get_logger(__name__) + +# --------------------------------------------------------------------------- +# Constants +# --------------------------------------------------------------------------- + +# Reuse the same cap as static_runner so a skill that's too big for static +# analysis is also too big to send to the CLI. +MAX_INPUT_BYTES = 1_000_000 # 1 MB — mirrors MAX_FILE_BYTES in static_runner.py +MAX_OUTPUT_BYTES = 10_000_000 # 10 MB safety cap on stdout +MAX_STDERR_BYTES = 64_000 # stderr is only used for error snippets +CLI_TIMEOUT_SECONDS = 300 # 5-minute per-call hard limit + +# Environment variables that must NOT be forwarded to child processes. +# Includes API keys, cloud creds, SSH agent, and SkillSpector's own keys. +_SECRET_ENV_PREFIXES: tuple[str, ...] = ( + "ANTHROPIC_API_KEY", + "OPENAI_API_KEY", + "NVIDIA_INFERENCE_KEY", + "NVIDIA_INFERENCE_METADATA_KEY", + "AWS_", + "AZURE_", + "GOOGLE_", + "GCLOUD_", + "GCP_", + "SSH_", + "GPG_", + "GITHUB_TOKEN", + "GITLAB_TOKEN", + "HUGGINGFACE_TOKEN", + "HF_TOKEN", + "COHERE_API_KEY", + "REPLICATE_API_TOKEN", + "MISTRAL_API_KEY", + "TOGETHER_API_KEY", + "GROQ_API_KEY", + "FIREWORKS_API_KEY", + "LANGCHAIN_API_KEY", + "LANGSMITH_API_KEY", +) + + +class AgentCLIError(RuntimeError): + """Raised when an agent CLI call fails for any reason (fail-closed).""" + + +# --------------------------------------------------------------------------- +# Environment scrubbing +# --------------------------------------------------------------------------- + + +def _scrub_env() -> dict[str, str]: + """Return a copy of ``os.environ`` with secret variables removed. + + Any variable whose name starts with a prefix in ``_SECRET_ENV_PREFIXES`` + is stripped. The resulting environment is passed to the subprocess. + """ + clean: dict[str, str] = {} + for key, val in os.environ.items(): + upper = key.upper() + if any(upper.startswith(p.upper()) for p in _SECRET_ENV_PREFIXES): + continue + clean[key] = val + return clean + + +# --------------------------------------------------------------------------- +# Binary lookup +# --------------------------------------------------------------------------- + + +def find_binary(name: str) -> str | None: + """Return the absolute path of *name* on PATH, or ``None`` if absent.""" + return shutil.which(name) + + +# --------------------------------------------------------------------------- +# Argument validation +# --------------------------------------------------------------------------- + + +def _validate_model_label(model: str) -> str: + """Ensure *model* cannot be used as an argument injection vector. + + Model labels come from ``SKILLSPECTOR_MODEL`` (user-controlled) or the + provider's defaults. We verify the label does not start with ``-`` + (which would look like a flag to the CLI) and contains only safe + characters. + + Raises: + AgentCLIError: when the label fails validation. + """ + if not model: + raise AgentCLIError("model label must be a non-empty string") + if model.startswith("-"): + raise AgentCLIError( + f"model label {model!r} starts with '-'; this looks like an argument injection attempt" + ) + # Allow alphanumeric, dash, dot, slash, colon, underscore (covers all + # known claude/codex model identifiers). + allowed = set("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-./: _") + bad = [c for c in model if c not in allowed] + if bad: + raise AgentCLIError(f"model label {model!r} contains disallowed characters: {bad!r}") + return model + + +# --------------------------------------------------------------------------- +# Claude CLI invocation +# --------------------------------------------------------------------------- + + +def _build_claude_argv(binary: str, model: str, max_output_tokens: int) -> list[str]: + """Build the argv list for a capability-stripped ``claude -p`` call. + + ``-p`` / ``--print`` + Non-interactive single-shot mode. The prompt is read from stdin; + the response is written to stdout and the process exits. + + ``--output-format text`` + Emit the assistant's response as plain text — nothing else. This is + the most stable format the claude CLI offers: it has been the canonical + headless contract since ``-p`` was introduced, predates the JSON + envelope formats, and is unaffected by changes to the event-stream + schema. The envelope formats (``json`` / ``stream-json``) have changed + shape across builds (single dict → JSON array → JSONL); ``text`` never + has. Because we need only the response text and not the metadata + (session ID, stop reason, etc.) that the envelope carries, ``text`` is + the right choice here: the format we request defines exactly what we + parse, with no version detection and no fallbacks. + + ``--model