From c738c5ef8bed1bb054c0eba9d862510450276143 Mon Sep 17 00:00:00 2001 From: Hyacinth-of-Security <144213008+lxcxjxhx@users.noreply.github.com> Date: Mon, 9 Nov 2026 16:49:37 +0800 Subject: [PATCH] fix(skills): give a bracketed literal in the userinfo slot no host on every Python (#8017) * fix(skills): give a bracketed literal in the userinfo slot no host on every Python `_http_host` answered from `urlsplit(url).hostname`, which decides the ambiguous authority `http://[::1]@Example.COM/api` differently across interpreters: 3.12 reads the bracketed part as userinfo and returns the host `example.com`, 3.13 rejects the authority and `urlsplit` raises, so the helper returns None. The same URL got two different answers depending on the runtime. The answer is not cosmetic: `_is_outbound_url` compares it against `_LOCAL_HTTP_HOSTS`, so whether a URL counts as local or outbound moved with the interpreter. The test table added with the surrounding refactor (#7950) already states the intended answer, None, which is why `tests/test_skillscan_native.py` fails on 3.12 and passes on 3.13. The bracketed-literal case is now rejected before `hostname` is read, so the answer no longer depends on that URL-parsing change, and the shape keeps the treatment the other malformed authorities get: no host, therefore outbound. The test table also gains a bracketed literal with an IPv6 host and a port (`http://[2001:DB8::1]@example.com:8443/api`), so a fix that only special-cased the exact URL from the failure cannot pass. On 3.12: `tests/test_skillscan_native.py` goes from 1 failed / 387 passed to 388 passed, and the skillscan suite (834 tests) passes. Reverting only the orchestrator change puts both bracketed cases back to failing. * fix(skills): reject raw brackets in URL userinfo --------- Co-authored-by: lxcxjxhx Co-authored-by: Willem Jiang --- README.md | 3 ++- backend/packages/harness/deerflow/skills/AGENTS.md | 2 +- .../harness/deerflow/skills/skillscan/orchestrator.py | 10 +++++++--- backend/tests/test_skillscan_native.py | 5 +++++ 4 files changed, 15 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 8f9a362f20..63e88c26b9 100644 --- a/README.md +++ b/README.md @@ -2165,7 +2165,8 @@ Windows scripts (`.bat`, `.cmd`, `.ps1`, `.psm1`, `.js`, `.jse`, `.vbs`, `.vbe`, SkillScan warns about remote downloads piped into common shells, including `env` launchers, sudo, interpreter paths, and shell line continuations. It distinguishes environment dumps from `env` commands, including assignments and GNU split-string escapes. Shell parsing and split expansion are bounded; reaching a limit preserves critical findings already detected. Pipes to non-shell tools such as `jq` and `tee` do not trigger this warning. SkillScan treats HTTP/HTTPS scheme spellings and HTTP hostnames case-insensitively and recognizes bracketed IPv6 -loopback (`[::1]`) URLs as local. External IPv6 endpoints still trigger network findings, +loopback (`[::1]`) hosts as local. Raw brackets in the URL's userinfo make it count as outbound for +exfiltration checks. External IPv6 endpoints still trigger network findings, and cloud-metadata hostname detection is case-insensitive. SkillScan recognizes unquoted integer model-usage counters such as `total_tokens` and `input_tokens` in skill examples as measurements. Benchmark token statistics and signed deltas are also recognized; credential fields and other string values remain subject to secret detection. diff --git a/backend/packages/harness/deerflow/skills/AGENTS.md b/backend/packages/harness/deerflow/skills/AGENTS.md index 3c86af34e3..a0d793dfd9 100644 --- a/backend/packages/harness/deerflow/skills/AGENTS.md +++ b/backend/packages/harness/deerflow/skills/AGENTS.md @@ -31,7 +31,7 @@ - **Name portability**: a skill name is a `custom/` directory component. Creation (`skill_manage` `create`, archive install) rejects Windows reserved device names via `windows_incompatible_segment()` on every platform, and extraction rejects member segments that cannot be created on every host; reads and deletes stay permissive for skills an older release created on Linux. Tests: `tests/test_skill_name_portability.py`. - **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()` caches confirmed provisioner capabilities for admission and fails closed on unknowns; bash overlays use the admitted sandbox's actual mode. Configured broker provisioning rejects direct credential mounts. `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 flows whose order cannot be decided produce no finding from this signal; 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: a part that cannot follow a sibling's rebinding (an `if` branch, the one `match` case, a `try` or loop body) applies its own bindings in source order, while a part that can (a loop or `try` `else`, a handler, a `finally`) starts with those names dropped; dispatch-time bindings (a loop target, `except ... as`, a `match` capture) are always dropped first, and nothing bound inside a body reaches the code after the statement. 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**: recognize HTTP/HTTPS schemes case-insensitively in both URL regexes and prefix checks, without changing source evidence. 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`. +- **SkillScan HTTP hosts**: recognize HTTP/HTTPS schemes case-insensitively in both URL regexes and prefix checks, without changing source evidence. Use URL authority parsing for lowercase hosts and bracketed IPv6 loopback. Userinfo must never hide an external host. Treat raw brackets in userinfo as malformed on all Python versions. Malformed URLs remain outbound for exfiltration checks. Cleartext HTTP detection covers IPv6 literals. Cloud-metadata hostname matching is case-insensitive. Coverage in `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 or bind its `required-secrets`. 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`. Local path targets are host paths, so the tool confines them to the configured skills root and the caller's own `{base_dir}/users/{user_id}/`; never re-add the Gateway cwd or `/tmp`, because `DEER_FLOW_HOME` sits under the cwd and those roots expose other users' skills. A skills root that contains `{base_dir}/users/` is dropped with a warning. 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 e20ac016fe..f19531114a 100644 --- a/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py +++ b/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py @@ -1446,11 +1446,15 @@ def _http_host(url: str) -> str | None: if not url[:8].lower().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 + parts = urlsplit(url) except ValueError: return None + # Python versions differ on which brackets in userinfo they accept. + # Treat raw brackets there as malformed so the URL stays outbound. + userinfo, at_sign, _hostport = parts.netloc.rpartition("@") + if at_sign and ("[" in userinfo or "]" in userinfo): + return None + host = parts.hostname return host.lower() if host else None diff --git a/backend/tests/test_skillscan_native.py b/backend/tests/test_skillscan_native.py index 80e0353d85..02cab052e3 100644 --- a/backend/tests/test_skillscan_native.py +++ b/backend/tests/test_skillscan_native.py @@ -2214,6 +2214,11 @@ def test_shell_curl_without_pipe_finishes_on_repeated_backslash_text(tmp_path: P ("http://[2001:DB8::1]:8080/api", "2001:db8::1"), ("http://user@[::1]:8080/api", "::1"), ("http://[::1]@Example.COM/api", None), + ("http://[2001:DB8::1]@example.com:8443/api", None), + ("http://[::1]@[::1]:8443/api", None), + ("http://[@[::1]", None), + ("http://x[@[::1]", None), + ("http://]@[::1]", None), ("http://localhost@Example.COM/api", "example.com"), ("http://[::1/api", None), ("ftp://LOCALHOST/api", None),