mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-07-31 02:15:59 +00:00
* feat(skills): bind request-scoped secrets for in-context (autonomously invoked) skills Extends the #3861 binding point A (slash-activation only) to A+: the injection set is recomputed on every model call from two unioned sources — the run's most recent slash activation (persisted on the run context so the tool loop keeps the binding) and skills the model actually loaded in this thread (ThreadState.skill_context), re-validated against the live registry each call. Authorization stays three-gated regardless of activation style: skill enabled by the operator, values supplied per-request by the caller in context.secrets (never persisted server-side, never from the host env), names declared in the skill's required-secrets frontmatter. Because the set is replaced per call, eviction from skill_context or a caller that stops supplying a value revokes injection on the next call. New frontmatter field secrets-autonomous (default true) lets a skill restrict binding to explicit slash activation; malformed values fail closed to false. Binding changes are recorded as a middleware:skill_secrets journal event carrying names only. Design informed by a survey of peer systems (Claude Code, Codex CLI, opencode, pi, deepagents, hermes-agent, QwenPaw) and specs (agentskills.io, MCP 2025-11-25): the industry trust boundary is enable-time consent plus caller-scoped credentials, not per-invocation ceremony; no surveyed system scopes secrets to an activation turn. Part of #3914 * refactor(skills): centralize secret context keys, document intentional per-call reload Review follow-ups (no behavior change): move the two private binding keys (__slash_skill_secret_source, __skill_secrets_binding_audit) into secret_context.py and add them to REDACTED_CONTEXT_KEYS so the redaction allowlist stays a complete guard even though both keys hold names only. Document why _in_context_secret_sources reloads skills every call rather than caching: load_skills re-reads enabled state so an operator disabling a skill revokes its binding on the next model call — an mtime cache would miss enable/disable toggles and keep injecting after a disable. * fix(skills): match in-context secret bindings by path only, never by name Review finding (confused deputy): _in_context_secret_sources fell back to name matching when a skill_context path did not resolve. DeerFlow lets a custom skill shadow a same-named public/legacy one (load_skills de-dupes by name, custom wins), so a thread that read public/foo could bind the custom foo's declared secrets although the custom skill was never loaded in the thread. The recent user-isolation path changes make by-path misses (and thus the dangerous fallback) more likely. Drop the by-name fallback: match strictly by the exact container file path the model read; an unresolved path simply does not bind (the safe direction). Regression tests cover the shadowing case and a stale path. Part of #3914 * fix(skills): resolve secret-binding sources via registry; strip caller __-keys Security review (willem-bd, #3938): 1. Forged `__slash_skill_secret_source` bypassed the enabled/allowlist/ secrets-autonomous gates. runtime.context is caller-mergeable, and the slash source was trusted as authoritative (its stored requirements were injected directly). Now the slash source records only the activated skill's canonical container path, and BOTH the slash and in-context sources resolve the live registry skill by normalized path each call (_resolve_registry_skill) — binding only that real, enabled, allowlisted skill's own declared secrets. A forged path resolves to nothing. As defense in depth, build_run_config strips caller-supplied __-prefixed context keys at the gateway boundary. 2. Malformed caller requirements crashed the run (unguarded tuple unpack / DoS). The middleware no longer unpacks caller-provided requirement data at all — declarations come from the registry — so a malformed source fails closed instead of raising. 3. Path-normalization asymmetry silently disabled in-context binding on a trailing-slash container_path config. Both the registry keys and the lookup path are now posixpath.normpath'd. Regression tests: forged source rejected, forged-but-real path ignores caller requirements + allowlist, malformed source fails closed, trailing- slash config binds, gateway strips __-keys. Part of #3914 * docs(skills): correct _SLASH_SECRET_SOURCE_KEY comment and note fail-closed trade-off Post-review cleanup: the key now stores only the canonical container path (the comment still described the pre-fix skill-name+requirements shape), and document that a transient registry-load failure fails closed (drops the binding for that call) rather than trusting stale data. --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
208 lines
8.0 KiB
Python
208 lines
8.0 KiB
Python
import logging
|
|
import re
|
|
from pathlib import Path
|
|
|
|
import yaml
|
|
|
|
from .types import SKILL_MD_FILE, SecretRequirement, Skill, SkillCategory
|
|
|
|
logger = logging.getLogger(__name__)
|
|
|
|
# Valid POSIX environment-variable name.
|
|
_ENV_VAR_NAME_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$")
|
|
|
|
|
|
def _format_yaml_error(skill_file: Path, exc: yaml.YAMLError, source: str) -> str:
|
|
"""Render a developer-friendly explanation of a YAML front-matter error."""
|
|
|
|
lines = [f"Invalid YAML front-matter in {skill_file}: {exc}"]
|
|
|
|
mark = getattr(exc, "problem_mark", None)
|
|
source_lines = source.splitlines()
|
|
if mark is not None and 0 <= mark.line < len(source_lines):
|
|
offending = source_lines[mark.line]
|
|
|
|
# mark.line is 0-based within the front-matter body; +1 makes it
|
|
# 1-based, +1 more accounts for the leading `---` fence that the
|
|
# front-matter regex strips before yaml.safe_load sees it. The
|
|
# result matches the line number an author sees in their editor.
|
|
file_line_number = mark.line + 2
|
|
lines.append(f" line {file_line_number}: {offending}")
|
|
|
|
# Targeted hint for the most common authoring mistake: an unquoted
|
|
# scalar value whose body contains ``: ``. We only surface the hint
|
|
# when we are confident it applies, to avoid misleading authors who
|
|
# hit unrelated YAML errors.
|
|
if getattr(exc, "problem", "") == "mapping values are not allowed here" and ":" in offending:
|
|
key, _, value = offending.partition(":")
|
|
value = value.strip()
|
|
if value and value[0] not in {'"', "'", "|", ">", "[", "{"}:
|
|
escaped = value.replace("\\", "\\\\").replace('"', '\\"')
|
|
lines.append(f' hint: values containing ":" must be quoted, e.g. {key}: "{escaped}"')
|
|
|
|
return "\n".join(lines)
|
|
|
|
|
|
def parse_allowed_tools(raw: object, skill_file: Path) -> tuple[str, ...] | None:
|
|
"""Parse the optional allowed-tools frontmatter field.
|
|
|
|
Returns None when the field is omitted. Returns a tuple when the field is a
|
|
YAML sequence of strings, including an empty tuple for explicit no-tool
|
|
skills. Raises ValueError for malformed values.
|
|
"""
|
|
if raw is None:
|
|
return None
|
|
if not isinstance(raw, list):
|
|
raise ValueError(f"allowed-tools in {skill_file} must be a list of strings")
|
|
|
|
allowed_tools: list[str] = []
|
|
for item in raw:
|
|
if not isinstance(item, str):
|
|
raise ValueError(f"allowed-tools in {skill_file} must contain only strings")
|
|
tool_name = item.strip()
|
|
if not tool_name:
|
|
raise ValueError(f"allowed-tools in {skill_file} cannot contain empty tool names")
|
|
allowed_tools.append(tool_name)
|
|
return tuple(allowed_tools)
|
|
|
|
|
|
def parse_required_secrets(raw: object, skill_file: Path) -> tuple[SecretRequirement, ...]:
|
|
"""Parse the optional required-secrets frontmatter field (issue #3861).
|
|
|
|
Accepts a YAML sequence whose items are either a string (the secret / env
|
|
variable name) or a mapping (``{name, optional}``). Returns an empty tuple
|
|
when the field is omitted. Entries whose name is missing or is not a valid
|
|
environment-variable name are dropped with a warning, so one malformed
|
|
declaration does not invalidate the whole skill. Raises ValueError only when
|
|
the field is present but is not a list.
|
|
"""
|
|
if raw is None:
|
|
return ()
|
|
if not isinstance(raw, list):
|
|
raise ValueError(f"required-secrets in {skill_file} must be a list")
|
|
|
|
secrets: list[SecretRequirement] = []
|
|
seen: set[str] = set()
|
|
for item in raw:
|
|
if isinstance(item, str):
|
|
name, optional = item.strip(), False
|
|
elif isinstance(item, dict):
|
|
name = str(item.get("name") or "").strip()
|
|
optional = bool(item.get("optional", False))
|
|
else:
|
|
logger.warning("Ignoring malformed required-secrets entry in %s: %r", skill_file, item)
|
|
continue
|
|
|
|
if not _ENV_VAR_NAME_RE.match(name):
|
|
logger.warning("Ignoring required-secrets entry with invalid env var name in %s: %r", skill_file, name)
|
|
continue
|
|
if name in seen:
|
|
continue
|
|
seen.add(name)
|
|
secrets.append(SecretRequirement(name=name, optional=optional))
|
|
return tuple(secrets)
|
|
|
|
|
|
def parse_secrets_autonomous(raw: object, skill_file: Path) -> bool:
|
|
"""Parse the optional ``secrets-autonomous`` frontmatter field (issue #3914).
|
|
|
|
``True`` (the default) lets declared secrets bind while the skill is
|
|
in-context via an autonomous model load; ``False`` restricts binding to
|
|
explicit ``/slash`` activation. A malformed (non-boolean) value fails
|
|
closed to ``False`` — the safer, less-injection direction.
|
|
"""
|
|
if raw is None:
|
|
return True
|
|
if isinstance(raw, bool):
|
|
return raw
|
|
logger.warning("Ignoring malformed secrets-autonomous value in %s: %r (autonomous binding disabled)", skill_file, raw)
|
|
return False
|
|
|
|
|
|
def parse_skill_file(skill_file: Path, category: SkillCategory, relative_path: Path | None = None) -> Skill | None:
|
|
"""Parse a SKILL.md file and extract metadata.
|
|
|
|
Args:
|
|
skill_file: Path to the SKILL.md file.
|
|
category: Category of the skill.
|
|
relative_path: Relative path from the category root to the skill
|
|
directory. Defaults to the skill directory name when omitted.
|
|
|
|
Returns:
|
|
Skill object if parsing succeeds, None otherwise.
|
|
"""
|
|
if not skill_file.exists() or skill_file.name != SKILL_MD_FILE:
|
|
return None
|
|
|
|
try:
|
|
content = skill_file.read_text(encoding="utf-8")
|
|
|
|
# Extract YAML front-matter block between leading ``---`` fences.
|
|
front_matter_match = re.match(r"^---\s*\n(.*?)\n---\s*\n", content, re.DOTALL)
|
|
if not front_matter_match:
|
|
return None
|
|
|
|
front_matter_text = front_matter_match.group(1)
|
|
|
|
try:
|
|
metadata = yaml.safe_load(front_matter_text)
|
|
except yaml.YAMLError as exc:
|
|
logger.error("%s", _format_yaml_error(skill_file, exc, front_matter_text))
|
|
return None
|
|
|
|
if not isinstance(metadata, dict):
|
|
logger.error("Front-matter in %s is not a YAML mapping", skill_file)
|
|
return None
|
|
|
|
# Extract required fields. Both must be non-empty strings.
|
|
name = metadata.get("name")
|
|
description = metadata.get("description")
|
|
|
|
if not name or not isinstance(name, str):
|
|
return None
|
|
if not description or not isinstance(description, str):
|
|
return None
|
|
|
|
# Normalise: strip surrounding whitespace that YAML may preserve.
|
|
name = name.strip()
|
|
description = description.strip()
|
|
|
|
if not name or not description:
|
|
return None
|
|
|
|
license_text = metadata.get("license")
|
|
if license_text is not None:
|
|
license_text = str(license_text).strip() or None
|
|
|
|
try:
|
|
allowed_tools = parse_allowed_tools(metadata.get("allowed-tools"), skill_file)
|
|
except ValueError as exc:
|
|
logger.error("Invalid allowed-tools in %s: %s", skill_file, exc)
|
|
return None
|
|
|
|
try:
|
|
required_secrets = parse_required_secrets(metadata.get("required-secrets"), skill_file)
|
|
except ValueError as exc:
|
|
logger.error("Invalid required-secrets in %s: %s", skill_file, exc)
|
|
return None
|
|
|
|
secrets_autonomous = parse_secrets_autonomous(metadata.get("secrets-autonomous"), skill_file)
|
|
|
|
return Skill(
|
|
name=name,
|
|
description=description,
|
|
license=license_text,
|
|
skill_dir=skill_file.parent,
|
|
skill_file=skill_file,
|
|
relative_path=relative_path or Path(skill_file.parent.name),
|
|
category=category,
|
|
allowed_tools=allowed_tools,
|
|
enabled=True, # Actual state comes from the extensions config file.
|
|
required_secrets=required_secrets,
|
|
secrets_autonomous=secrets_autonomous,
|
|
)
|
|
|
|
except Exception:
|
|
logger.exception("Unexpected error parsing skill file %s", skill_file)
|
|
return None
|