Skip to content

fix(agent_hub): BUG redact outbound secrets by content, not file path - #969

Open
mushenL wants to merge 4 commits into
modelscope:mainfrom
mushenL:bug-0909-01
Open

fix(agent_hub): BUG redact outbound secrets by content, not file path#969
mushenL wants to merge 4 commits into
modelscope:mainfrom
mushenL:bug-0909-01

Conversation

@mushenL

@mushenL mushenL commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Change Summary

sanitize_outbound_file decided whether to clean a file by its PATH: each
framework whitelisted its own root config (ms-agent settings.json/mcp.json,
qwenpaw agent.json, hermes config.yaml, openhuman config.toml) and returned
everything else verbatim -- openclaw, nanobot and qoder defined no hook at all.
The collect patterns take skills/* recursively plus the persona and memory
documents, so a key an AI assistant wrote into a skill script, a skill-local
mcp.json, SOUL.md or MEMORY.md was uploaded into the remote repo and its git
history.

Add a content-driven layer (_secrets.py) that runs after the per-framework hook
in _sync.sanitize_outbound, the only production caller of that hook, so all
seven frameworks are covered without editing a single spec:

  • Tier A: config-shaped files at any path, by name or by an mcpServers shape
    trigger. A shape-triggered file is cleaned inside that subtree only -- the
    shared vocabulary blanks any key named tokens/keys, which would destroy
    data in a skill's JSON fixture or a memory dump.
  • Tier B: every other text file gets high-confidence secret VALUES replaced
    with a [REDACTED:<kind>] marker (vendor prefixes, JWT, Bearer, a narrowed
    key-name vocabulary, URL credentials, base64 payloads that decode to a key).
    The bag rules never apply here, and the value gate refuses variable
    references, placeholders, identifier shapes and low-entropy strings so
    documentation survives.

The layer never raises (the watch daemon swallows exceptions and would silently
stop syncing), returns the original bytes object when nothing was redacted
(drop_unchanged_defaults and the sha256 push-skip both compare bytes), and is
idempotent (the watcher baselines the sanitized sha). Findings bubble up to a
Secrets redacted table in upload -- printed before the --dry-run return -- and
to a warning in the watch log; they carry the kind and key name, never the
secret.

Also promotes openhuman's private TOML scrubber to a shared
scrub_toml_secrets so .toml at any path uses one vocabulary.

Redaction is outbound only: sanitize_inbound_file also serves local convert
writes, where it would strip the user's own keys from the converted agent.
Already-leaked secrets stay in the remote git history and must be rotated.

Verified: 18 sentinels across 7 frameworks leak 0 (was 31 hits on ms-agent +
qwenpaw); 0 modifications across 690 clean files (default templates, docs, SDK
source, real converted agent packages); tests/agent_hub 369 passed.

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Run pre-commit install and pre-commit run --all-files before git commit, and passed lint check.
  • Documentation reflects the changes where applicable

The comments added with the content-driven redaction layer restated the code,
repeated the same rationale twice in one file, and carried a bug narrative that
belongs in the commit history. What remains is only the non-obvious why: the
case-sensitive Bearer match, the sk- left boundary, why scrub_url_secrets is not
reused for free text, why the base64 pass is not behind the prefilter, and the
never-raise / original-bytes / idempotent contracts. Also fixes the vendor
labels, which were shifted one line off their patterns.

Test methods are consolidated, not thinned: the vendor cases now cover all
eleven prefix branches instead of two, and the homogeneous false-positive guards
became subTest tables. Four redundant cases left the repo suite -- a multiline
rerun of an already per-line corpus, a trivial report-formatting assertion, a
per-framework benign check now folded into the leak test, and a dry-run report
check already covered by two existing tests.

No behavior change: the leak repro still reports 0 and the false-positive
benchmark still reports 0 hits over 690 clean files. Suite goes 369 -> 361
passed with subtests 93 -> 102.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant