Xinmin Zeng 4d660b202a
feat(skills): bind request-scoped secrets for autonomously-invoked skills (A+) (#3938)
* 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>
2026-07-04 23:34:32 +08:00

104 lines
4.2 KiB
Python

"""Request-scoped secret carrier in the run context (issue #3861).
Callers pass per-request secrets out-of-band in ``config.context.secrets`` — a
mapping of name -> value. The value never enters the prompt, tool arguments, or
the executed command string; it is injected as an environment variable into a
skill's sandbox subprocess only when an activated skill declares it via the
``required-secrets`` frontmatter field.
This module centralises the reserved key name and safe extraction so the carrier
contract lives in one place, consumed by the skill-activation middleware (to
build the per-turn injection set) and the tracing redactor (to strip it from
trace payloads).
"""
from __future__ import annotations
from typing import Any
# Reserved sub-key of the run context that holds request-scoped secrets supplied
# by the caller. Source of truth for what a skill *may* receive.
SECRETS_CONTEXT_KEY = "secrets"
# Reserved sub-key holding the secrets resolved for the currently activated skill
# (binding point A). Written by the skill-activation middleware, read by the bash
# tool. Both reserved keys are stripped from trace payloads (see tracing redactor).
ACTIVE_SECRETS_CONTEXT_KEY = "__active_skill_secrets"
def _string_pairs(raw: Any) -> dict[str, str]:
if not isinstance(raw, dict):
return {}
return {key: value for key, value in raw.items() if isinstance(key, str) and isinstance(value, str)}
def extract_request_secrets(context: Any) -> dict[str, str]:
"""Return the caller-supplied request-scoped secrets mapping, or ``{}``.
Only string-keyed, string-valued entries are kept; anything else is ignored
so a malformed carrier can never crash secret resolution or injection.
"""
if not isinstance(context, dict):
return {}
return _string_pairs(context.get(SECRETS_CONTEXT_KEY))
def read_active_secrets(context: Any) -> dict[str, str]:
"""Return the secrets resolved for the active skill (the per-run injection
set), or ``{}``. Read by the bash tool to build the subprocess env."""
if not isinstance(context, dict):
return {}
return _string_pairs(context.get(ACTIVE_SECRETS_CONTEXT_KEY))
# Private run-context keys the skill-activation middleware uses to carry secret
# bindings across a run. Only ``secrets`` / ``__active_skill_secrets`` hold
# values; the binding-source and audit keys hold names only. All are listed so
# the redaction allowlist stays a complete guard even if a future edit starts
# storing a value under one of the name-only keys.
_SLASH_SECRET_SOURCE_KEY = "__slash_skill_secret_source"
_SECRETS_BINDING_AUDIT_KEY = "__skill_secrets_binding_audit"
# Run-context keys whose values are request-scoped secrets and must be stripped
# before a context mapping is serialized anywhere observable (traces, logs).
REDACTED_CONTEXT_KEYS = frozenset(
{
SECRETS_CONTEXT_KEY,
ACTIVE_SECRETS_CONTEXT_KEY,
_SLASH_SECRET_SOURCE_KEY,
_SECRETS_BINDING_AUDIT_KEY,
}
)
def redact_secret_context_keys(context: Any) -> Any:
"""Return a shallow copy of ``context`` with secret-bearing keys removed.
Defensive helper for any code path that serializes the run context into an
observable surface. DeerFlow's own trace-metadata builder never copies the
context, so this is belt-and-suspenders for future call sites and custom
tracer configurations.
"""
if not isinstance(context, dict):
return context
return {key: value for key, value in context.items() if key not in REDACTED_CONTEXT_KEYS}
def redact_config_secrets(config: Any) -> Any:
"""Return a copy of a run config safe to persist or echo back to clients.
The request config (``body.config``) is stored verbatim on the run record
(``runs.kwargs_json``) and echoed by the run API. Strip the secret-bearing
keys from its ``context`` so a request-scoped secret is never persisted or
returned, while the live config that drives the run (built separately) keeps
them. Non-dict / context-less configs pass through unchanged.
"""
if not isinstance(config, dict):
return config
context = config.get("context")
if not isinstance(context, dict):
return config
redacted = dict(config)
redacted["context"] = redact_secret_context_keys(context)
return redacted