diff --git a/README.md b/README.md index 0b7927d9c3..16cf53faaa 100644 --- a/README.md +++ b/README.md @@ -1293,6 +1293,10 @@ If a trusted operator manages the configured skills directory through an externa Skill installs and agent-managed skill edits run through **SkillScan**, a native deterministic safety scanner before the LLM-based skill scanner. Phase 1 runs offline with no Semgrep/OpenGrep dependency, blocks high-confidence `CRITICAL` findings such as private keys or shell execution, and passes warning findings to the LLM scanner for contextual review. Code files (anything under `scripts/`, a script suffix such as `.py`, `.sh`, or `.js`, or an extensionless file starting with `#!`) that are not NUL-free UTF-8 text raise a warning and are still analyzed over a lossy decode, so a single stray byte cannot hide them from `CRITICAL` checks. The moderation adapter normalizes both plain-text model responses and LangChain Responses API text blocks before parsing the required JSON decision. Python instance-client exfiltration checks follow a minimal same-scope evidence chain: a simple name bound to a known client constructor, optional name-to-name aliases, and an actual outbound method or context-manager use supported by that constructor. Constructor roots must be proven imports; bare canonical-looking names are not inferred as modules. Nested scopes do not inherit client handles and inherit only constructor import aliases that are never rebound in the enclosing scope. Comprehensions, walrus-bearing statements, annotations, complex binding targets, unsupported operations, and ambiguous branch flows produce no finding from this signal; skipped constructs conservatively invalidate every name they may bind so stale client state cannot create a finding. A deterministic work budget or recursion limit reached by this best-effort analysis does not discard findings already collected for the file. Set `skill_scan.enabled: false` in `config.yaml` to disable only the deterministic analyzers; safe archive extraction and the LLM scanner still run. +SkillScan treats HTTP hostnames case-insensitively and recognizes bracketed IPv6 +loopback (`[::1]`) URLs as local. External IPv6 endpoints still trigger network findings, +and cloud-metadata hostname detection is case-insensitive. + For Python credential mappings, SkillScan checks literal values while treating ordinary dictionary keys as labels. A mapping such as `tokens = {"access_token": os.getenv("ACCESS_TOKEN")}` does not report a hardcoded credential. Keys matching a recognized cloud or API token format are still checked as embedded credentials. DeerFlow also ships with **skill-reviewer**, a public skill for read-only skill quality review. It uses the built-in `review_skill_package` tool to inspect installed skills, local packages, archives, or pasted `SKILL.md` content without activating the target skill, binding its secrets, executing its scripts, or installing it. The tool returns a compact, tag-neutralized JSON payload to the model context and keeps the full raw review payload in the tool artifact for programmatic consumers. The deterministic review core reuses DeerFlow parsing and SkillScan facts, emits versioned JSON contracts under `contracts/skill_review/`, and can be run from the backend CLI: diff --git a/backend/packages/harness/deerflow/skills/AGENTS.md b/backend/packages/harness/deerflow/skills/AGENTS.md index 723471196e..549e9d7025 100644 --- a/backend/packages/harness/deerflow/skills/AGENTS.md +++ b/backend/packages/harness/deerflow/skills/AGENTS.md @@ -15,6 +15,7 @@ - **Installation**: `POST /api/skills/install` extracts .skill ZIP archive to custom/ directory - **Managed integrations**: Lark/Feishu CLI support installs one global official `lark-*` pack as read-only `SkillCategory.INTEGRATION` entries under `/mnt/skills/integrations/lark-cli/...`; enabled flags, app configuration, and OAuth data remain per-user. Install resolves the newest `larksuite/cli` release from GitHub (`releases/latest`) at install time (falling back to a bottom-line pinned version if the lookup fails) rather than hard-coding the pack version; integrity relies on the official host + structural archive guards + a recorded hash of the effective installed tree after shared guidance injection (not a pinned archive-byte SHA, which GitHub does not keep stable). The Gateway image still installs a pinned `@larksuite/cli` binary, so `get_lark_integration_status` surfaces `latest_available_version` and `runtime_version_mismatch` for the UI. AIO installs additionally verify and publish official Linux amd64/arm64 binaries under `{DEER_FLOW_HOME}/integrations/lark-cli/sandbox-cli`, mounted read-only at `/mnt/integrations/lark-cli/runtime`; `/mnt/integrations/lark-cli/config` (app credentials, incl. the long-lived `appSecret`) is mounted **read-only** into the sandbox, its empty `config/locks` subdirectory is over-mounted writable for `lark-cli` coordination files, and `/mnt/integrations/lark-cli/data` (refreshable OAuth tokens) stays writable, all mapping to owner-only per-user directories. **Sandbox trust boundary:** the credential-bearing config and data dirs are still *readable* by arbitrary sandbox processes (the agent's `bash` tool, or code reached via prompt-injection in a tool result), so the app secret and tokens are exposed to sandbox-side code even though they never reach the browser — the read-only config mount only prevents in-sandbox tampering, not read/exfiltration. The sidecar credential-broker (Pattern B, issue #4338) is the fix that removes these plaintext mounts from sandbox execution: set `LARK_CLI_BROKER_IMAGE` on the provisioner (see `docker/lark-cli-broker/`) and the Gateway sends `provision_lark_cli_broker` on sandbox create. The provisioner then runs a `lark-cli-broker` sidecar that owns the per-user `config`/`config/locks`/`data` mounts (mounted into the **sidecar only**, at `/var/lark/{config,config/locks,data}` with only the nested locks mount writable) and serves the `lark-cli` command surface on Pod loopback (`http://127.0.0.1:8788`); a shim init container (`install-shim`) writes a forwarding `lark-cli` into the shared runtime `emptyDir`, so the sandbox gets `DEERFLOW_LARK_BROKER_URL` + a shim on PATH but **no** credential files. The on-PATH `bin/lark-cli` is a `/bin/sh` launcher that resolves a Python 3 interpreter and execs the Python shim body (`bin/lark-cli-shim.py`) beside it, so broker mode does not silently ENOEXEC on a sandbox image without a `#!/usr/bin/env python3`-resolvable interpreter — it fails loudly (exit 127, actionable message) and can be pinned with `DEERFLOW_LARK_BROKER_PYTHON`. The broker runs `lark-cli` in the sidecar's cwd and cannot see the sandbox filesystem, so cwd is intentionally **not** forwarded and file-I/O subcommands relative to the sandbox cwd are unsupported (command surface only). An optional `DEERFLOW_LARK_BROKER_DENY_SUBCOMMANDS` denylist (comma-separated command paths, matched in order through interleaved option values, forwarded from the provisioner) lets the broker refuse secret-dumping subcommands before spawning the binary. `lark_cli_env_overlay(broker=True)` therefore omits `LARKSUITE_CLI_CONFIG_DIR`/`DATA_DIR`; `sandbox_lark_broker_active()` (TTL-cached provisioner `/api/capabilities` probe, tight timeout + longer negative caching on the bash hot path) selects broker vs. binary mode for both the bash env overlay and status. `DEER_FLOW_LARK_CLI_SANDBOX_RUNTIME_DIR` supplies a validated, symlink-free pre-staged runtime for air-gapped deployments. For the remote provisioner (K8s), the runtime binary is otherwise provisioned by an optional init container + shared `emptyDir` (Pattern A): set `LARK_CLI_INIT_IMAGE` on the provisioner (see `docker/lark-cli-init/`) and the Gateway sends `provision_lark_cli_runtime` on sandbox create once the pack is installed, so remote installs skip the Gateway-side GitHub download entirely. Broker (Pattern B) supersedes the init-container binary (Pattern A) when both images are configured. `get_lark_integration_status(check_runtime=True)` surfaces `sandbox_runtime_mode` (`none` / `gateway-download` / `init-container` / `broker`) and `sandbox_runtime_ready` (remote modes read the provisioner `GET /api/capabilities`: `lark_cli_init_image` / `lark_cli_broker_image`), plus `sandbox_runtime_probed` separating a confirmed probe result from the conservative unprobed fallback (mutation responses carry a probed status built from a credential snapshot taken under the credential lock), so a green UI can't hide a chat-time `lark-cli: command not found`. Cheap status probes are explicitly not live-verified; users authorize or reconnect through the browser device-flow endpoints instead of running terminal commands. - **SkillScan**: `packages/harness/deerflow/skills/skillscan/` is the native deterministic scanner for `.skill` archives and agent-managed skill writes. It runs offline before the LLM scanner, emits structured findings (`rule_id`, `severity`, `file`, `line`, `message`, `remediation`, redacted `evidence` — category/analyzer are encoded in the `rule_id` prefix), blocks `CRITICAL`, and passes warning findings into `scan_skill_content()`. The moderation adapter must normalize both plain-text responses and LangChain Responses API text blocks before parsing the required JSON decision. `scan_archive_preflight()` / `scan_skill_dir()` are pure sync functions (dispatch off the event loop); `enforce_static_scan()` applies the blocking policy and the `skill_scan.enabled` kill switch. `skills/package_files.py` is the single definition of code files (`scripts/` members, code suffixes, extensionless `#!` files) and executable magic bytes; the installer, export guard, and SkillScan all import it, so do not re-derive either rule locally. Files that fail NUL-free UTF-8 decoding are treated as binaries unless they are code files: interpreters still run those, so they raise `package-undecodable-script` (HIGH) and are analyzed over a lossy decode, keeping one stray byte from hiding a file or downgrading a `CRITICAL` match; one with executable magic skips the text rules, which only misread its string tables. The Python instance-client signal deliberately follows only a one-level, same-scope evidence chain (PR #4265 review): a proven imported constructor bound to a simple name, optional name-to-name alias propagation, rebinding invalidation, and a constructor-supported outbound method or context-manager use; bare canonical-looking names never fall back to module identity. Nested scopes never inherit client handles and inherit only constructor aliases proven stable by a binding-only enclosing-scope prepass. Comprehensions, walrus-bearing statements, annotations, executable expressions inside complex binding targets, unsupported operations, and ambiguous flows produce no finding from this signal; skipped constructs invalidate all names they may bind, while representative false negatives are pinned by `test_python_declared_false_negatives_stay_unreported`. Compound bodies are walked from isolated copies so wrapping code in `if True:` is not a bypass, while copied scope entries, binding-only prepasses, and AST visits consume a deterministic work budget and the walk stops after its first sink. Budget or recursion exhaustion skips only this best-effort signal and retains deterministic findings already collected for the file. Do not add Semgrep/OpenGrep or YAML rule-engine dependencies to the core path; Phase 1 rule specs live in Python constants next to their analyzers in `skillscan/orchestrator.py`. +- **SkillScan HTTP hosts**: use URL authority parsing for lowercase hosts and bracketed IPv6 loopback; userinfo must never hide an external host. Malformed URLs remain outbound for exfiltration checks. Cleartext HTTP detection covers IPv6 literals; cloud-metadata hostname matching is case-insensitive. Coverage: `tests/test_skillscan_native.py`. - **Python secret mappings**: `secret-env-assignment` scans literal container values and conditional branches under secret-like bindings. Dictionary keys are structural labels and contribute candidates only when their folded literal matches the shared `_SECRET_TOKEN_PATTERNS`; they must not inherit credential evidence from the enclosing variable name. Opaque keys without a recognized token format stay unreported. Regressions in `tests/test_skillscan_native.py` cover runtime-loaded mappings, nested labels, credential-value locations, and literal/concatenated token keys; `tests/test_skill_review_core.py` pins the CI CLI gate. - **Skill Review Core**: `packages/harness/deerflow/skills/review/` provides read-only package snapshots, deterministic facts, resource/eval analysis, report rendering, and the CLI (`python -m deerflow.skills.review.cli`). It reuses the shared frontmatter helper and SkillScan; it must not import `app.*`, execute target scripts, install dependencies, or call networks. JSON contracts live in `contracts/skill_review/`. The analyzer hands SkillScan a byte-faithful copy of every non-symlink snapshot file (binary entries carry `content_base64`), so package rules such as `package-executable-binary` cover binaries; only eval-fixture `SKILL.md` samples are withheld, while scripts and other files under `evals/fixtures/` are scanned. Materialization uses exclusive create, so a duplicate archive member or a case-folded name fails the scan closed (`not_assessed: skillscan`) instead of overwriting an earlier file, as does an entry without bytes in an untruncated snapshot. The `review_skill_package` built-in tool labels results with `review_subject_entry` and never `skill_context_entry`, so reviewing a target does not activate it, bind its `required-secrets`, or apply its `allowed-tools`. Its model-visible `ToolMessage.content` is a compact JSON payload with untrusted control tags neutralized; the full raw review payload, including Markdown renders, stays in `ToolMessage.artifact`. CI should run the CLI with `--fail-on error --fail-on-incomplete` so blocker/error findings and truncated/not-assessed packages fail the gate. The public `skills/public/skill-reviewer` skill owns semantic readiness review and suggestions only; mutation and runtime experiments remain owned by `skill-creator`. diff --git a/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py b/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py index 8c75c90dc9..8eb7ce9b82 100644 --- a/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py +++ b/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py @@ -21,6 +21,7 @@ from collections.abc import Iterable from dataclasses import dataclass from pathlib import Path, PurePosixPath, PureWindowsPath from typing import Any +from urllib.parse import urlsplit from deerflow.skills.package_files import is_code_file, is_executable_binary_prefix from deerflow.skills.package_paths import is_eval_fixture_skill_md @@ -134,7 +135,7 @@ _SECRET_TOKEN_PATTERNS = tuple( ) ) _SENSITIVE_PATH_RE = re.compile(r"(~/.ssh|/etc/passwd|/etc/shadow|/var/run/docker\.sock|docker\.sock|169\.254\.169\.254)") -_EXTERNAL_HTTP_RE = re.compile(r"http://([A-Za-z0-9.-]+)(?::\d+)?(?:/|\b)") +_EXTERNAL_HTTP_RE = re.compile(r"http://(?:[^/?#\s)'\"<>]*@)?(\[[0-9A-Fa-f:.]+\]|[A-Za-z0-9.-]+)(?::\d+)?(?:/|\b|(?=$|[\s)'\"<>?#]))") _URL_RE = re.compile(r"https?://[^\s)'\"<>]+") _LOCAL_HTTP_HOSTS = {"localhost", "127.0.0.1", "0.0.0.0", "::1"} # `rm` with a recursive flag (any order/combination, optional --no-preserve-root) @@ -800,16 +801,20 @@ def _scan_shell(rel_path: str, text: str) -> list[SecurityFinding]: def _scan_network_and_resource(rel_path: str, text: str) -> list[SecurityFinding]: findings: list[SecurityFinding] = [] - if match := re.search(r"(169\.254\.169\.254|metadata\.google\.internal)", text): + if match := re.search(r"(169\.254\.169\.254|metadata\.google\.internal)", text, re.IGNORECASE): findings.append(_finding_from_match("network-cloud-metadata", rel_path, text, match)) if match := re.search(r":\(\)\{\s*:\|:&\s*\};:", text): findings.append(_finding_from_match("resource-fork-bomb", rel_path, text, match)) for match in _EXTERNAL_HTTP_RE.finditer(text): - host = match.group(1) + host = _http_host(match.group(0)) or "" if host in _LOCAL_HTTP_HOSTS or host.startswith("10.") or host.startswith("192.168.") or re.match(r"172\.(1[6-9]|2\d|3[01])\.", host): - findings.append(_finding_from_match("network-local-http", rel_path, text, match)) + rule_id = "network-local-http" else: - findings.append(_finding_from_match("network-cleartext-http", rel_path, text, match)) + rule_id = "network-cleartext-http" + finding = _finding_from_match(rule_id, rel_path, text, match) + if "@" in match.group(0): + finding["evidence"] = "http://" + match.group(0).rsplit("@", 1)[1] + findings.append(finding) break return findings @@ -1003,8 +1008,15 @@ def _looks_like_placeholder(value: str) -> bool: def _http_host(url: str) -> str | None: - match = re.match(r"https?://\[?([^]/:]+)", url) - return match.group(1) if match else None + if not url.startswith(("http://", "https://")): + return None + try: + # Parse the authority so IPv6 brackets and userinfo cannot be mistaken + # for the host. Malformed URLs remain outbound in _is_outbound_url. + host = urlsplit(url).hostname + except ValueError: + return None + return host.lower() if host else None def _is_outbound_url(value: str) -> bool: diff --git a/backend/tests/test_skillscan_native.py b/backend/tests/test_skillscan_native.py index 30de784542..08df0cddbe 100644 --- a/backend/tests/test_skillscan_native.py +++ b/backend/tests/test_skillscan_native.py @@ -344,12 +344,13 @@ def test_python_subprocess_kwargs_unpacking_without_shell_key_still_blocks(tmp_p assert not [item for item in findings if item["rule_id"] == "python-subprocess"] -def test_cloud_metadata_access_is_reported_by_one_rule(tmp_path: Path) -> None: +@pytest.mark.parametrize("host", ["169.254.169.254", "metadata.google.internal", "METADATA.GOOGLE.INTERNAL", "Metadata.Google.Internal"]) +def test_cloud_metadata_access_is_reported_by_one_rule(tmp_path: Path, host: str) -> None: skill_dir = tmp_path / "demo-skill" _write_skill(skill_dir) scripts_dir = skill_dir / "scripts" scripts_dir.mkdir() - (scripts_dir / "run.py").write_text('import urllib.request\nurllib.request.urlopen("http://169.254.169.254/latest/meta-data/")\n', encoding="utf-8") + (scripts_dir / "run.py").write_text(f'import urllib.request\nurllib.request.urlopen("http://{host}/latest/meta-data/")\n', encoding="utf-8") findings = scan_skill_dir(skill_dir)["findings"] @@ -1824,3 +1825,110 @@ def test_bundled_public_skill_scripts_report_no_secret_assignment() -> None: offenders[skill_dir.name] = [(finding["file"], finding["line"]) for finding in hits] assert offenders == {} + + +@pytest.mark.parametrize( + "url, host", + [ + ("http://LOCALHOST:8080/api", "localhost"), + ("http://[::1]/api", "::1"), + ("https://[::1]:8443/api", "::1"), + ("http://[2001:DB8::1]:8080/api", "2001:db8::1"), + ("http://user@[::1]:8080/api", "::1"), + ("http://[::1]@Example.COM/api", None), + ("http://localhost@Example.COM/api", "example.com"), + ("http://[::1/api", None), + ("ftp://LOCALHOST/api", None), + ], +) +def test_http_host_normalizes_case_and_ipv6(url: str, host: str | None) -> None: + from deerflow.skills.skillscan.orchestrator import _http_host + + assert _http_host(url) == host + + +def test_uppercase_local_host_is_classified_local(tmp_path: Path) -> None: + skill_dir = tmp_path / "skill" + _write_skill(skill_dir) + (skill_dir / "run.py").write_text( + 'import urllib.request\nurllib.request.urlopen("http://LOCALHOST:8080/config")\n', + encoding="utf-8", + ) + findings = scan_skill_dir(skill_dir)["findings"] + assert _finding_by_rule(findings, "network-local-http") + assert not [finding for finding in findings if finding["rule_id"] == "network-cleartext-http"] + + +@pytest.mark.parametrize("host, external", [("localhost", False), ("LOCALHOST", False), ("LocalHost", False), ("Example.COM", True), ("[::1]", False), ("[2001:DB8::1]", True), ("localhost@Example.COM", True)]) +def test_declared_http_host_case_classification(tmp_path: Path, host: str, external: bool) -> None: + skill_dir = tmp_path / "skill" + _write_skill(skill_dir, f"Endpoint: http://{host}:8080/api\n") + + findings = scan_skill_dir(skill_dir)["findings"] + + assert bool([finding for finding in findings if finding["rule_id"] == "declaration-external-endpoint"]) is external + assert bool([finding for finding in findings if finding["rule_id"] == "network-local-http"]) is (not external) + + +@pytest.mark.parametrize("host, external", [("localhost", False), ("LOCALHOST", False), ("LocalHost", False), ("Example.COM", True), ("[::1]", False), ("[2001:DB8::1]", True), ("[::1]@Example.COM", True)]) +def test_sensitive_path_http_host_case_classification(tmp_path: Path, host: str, external: bool) -> None: + skill_dir = tmp_path / "skill" + _write_skill(skill_dir) + (skill_dir / "run.py").write_text(f'ENDPOINT = "http://{host}:8080/api"\nopen("/etc/passwd").read()\n', encoding="utf-8") + + result = scan_skill_dir(skill_dir) + findings = result["findings"] + + if external: + assert _finding_by_rule(findings, "python-sensitive-exfil")["severity"] == "CRITICAL" + else: + assert _finding_by_rule(findings, "python-sensitive-path-read")["severity"] == "HIGH" + assert not [finding for finding in findings if finding["rule_id"] == "python-sensitive-exfil"] + assert result["blocked"] is external + + +@pytest.mark.parametrize( + "url, local", + [ + ("http://[::1]:8080/api", True), + ("http://[::1]", True), + ("http://[::1]?mode=local", True), + ("http://[2001:DB8::1]:8080/api", False), + ("http://[2001:db8::1]", False), + ], +) +def test_ipv6_cleartext_http_classification(tmp_path: Path, url: str, local: bool) -> None: + skill_dir = tmp_path / "skill" + _write_skill(skill_dir, f"# Demo\nEndpoint: {url}\n") + + findings = scan_skill_dir(skill_dir)["findings"] + + rule_id = "network-local-http" if local else "network-cleartext-http" + finding = _finding_by_rule(findings, rule_id) + assert finding["file"] == "SKILL.md" + assert finding["line"] == 7 + other_rule = "network-cleartext-http" if local else "network-local-http" + assert not [item for item in findings if item["rule_id"] == other_rule] + + +def test_malformed_ipv6_url_remains_outbound(tmp_path: Path) -> None: + skill_dir = tmp_path / "skill" + _write_skill(skill_dir) + (skill_dir / "run.py").write_text('ENDPOINT = "http://[::1/api"\nopen("/etc/passwd").read()\n', encoding="utf-8") + + result = scan_skill_dir(skill_dir) + + assert _finding_by_rule(result["findings"], "python-sensitive-exfil")["severity"] == "CRITICAL" + assert result["blocked"] + assert result["scanner_errors"] == [] + + +def test_cleartext_http_uses_host_after_userinfo_without_exposing_credentials(tmp_path: Path) -> None: + skill_dir = tmp_path / "skill" + _write_skill(skill_dir, "Endpoint: http://localhost:private-value@Example.COM:8080/api\n") + + findings = scan_skill_dir(skill_dir)["findings"] + + finding = _finding_by_rule(findings, "network-cleartext-http") + assert finding["evidence"] == "http://Example.COM:8080/" + assert not [item for item in findings if item["rule_id"] == "network-local-http"]