fix(skills): mask Skill.mcp_tools credentials at rest#3774
Conversation
Skill.mcp_tools is an unmodeled dict with no masking serializer, so any MCP server credential a skill carries in env/headers dumped in plaintext wherever a Skill is serialized — including two at-rest disk paths today: conversation state (base_state.json / agent-server meta.json) and PersistedSettings, both via agent_context.skills[]. OpenHandsAgentSettings.mcp_config already masks via serialize_mcp_config, but skills bypassed it. Add a @field_serializer on Skill.mcp_tools that routes through the same serialize_mcp_config (single masking source of truth): redacted by default, plaintext under expose_secrets, encrypted under a cipher. serialize_mcp_config is imported function-locally to avoid the settings.model -> agent_context -> skills import cycle. Fixing at the Skill level covers every serialization site (AgentContext, conversation state, settings persistence, profiles, future containers) rather than one boundary at a time. Supersedes the OpenHandsAgentProfile-boundary mask on branch agent-profile-model. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: PR #3774 — mask Skill.mcp_tools credentials at rest
🟢 Good taste — Elegant, minimal security fix with no issues found.
Summary
This PR addresses a security gap where Skill.mcp_tools would serialize MCP server credentials (API keys, bearer tokens) in plaintext during serialization. The fix routes mcp_tools through the same serialize_mcp_config used by settings.mcp_config, ensuring consistent secret masking behavior across the codebase.
Taste Assessment
The implementation demonstrates good engineering judgment:
-
Pattern reuse: Leverages the existing
serialize_mcp_configfunction rather than duplicating masking logic — this ensures parity withmcp_configbehavior and reduces maintenance burden. -
Lazy import: The
from openhands.sdk.settings.model import serialize_mcp_configinside the serializer avoids thesettings.model -> agent_context -> skillsimport cycle. This is the right solution for this constraint. -
Comprehensive test coverage: Five new tests cover all scenarios:
- Default redaction (secret absent from serialized output)
- Opt-in plaintext exposure (
expose_secrets: plaintext) - Encryption with cipher (
expose_secrets: encrypted) - In-memory values remain accessible (runtime usage unaffected)
Noneroundtrips unchanged
-
Minimal surface area: Only 27 lines of production code added, with focused purpose.
No Issues Found
No critical issues, improvement opportunities, or testing gaps identified.
Risk and Safety Evaluation
- Overall PR
⚠️ Risk Assessment: 🟢 LOW
This change adds security protection rather than removing it. The in-memory mcp_tools attribute retains real values, ensuring runtime MCP functionality is unaffected. The change only affects serialization output, and the tests confirm that deserialization from serialized data still works correctly.
Verdict
✅ Worth merging: Clean, well-tested security fix that follows existing patterns.
Key Insight
The lazy import pattern here is worth documenting as a precedent — it's the idiomatic solution when a serializer needs access to another module that would create a circular dependency.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
Skill.mcp_tools credential serialization was exercised before/after; the PR masks default at-rest dumps while preserving opt-in plaintext/encrypted serialization and live in-memory access.
Does this PR achieve its stated goal?
Yes. The stated goal is to mask Skill.mcp_tools credentials at rest. On origin/main, a real SDK script created a Skill with MCP env/headers credentials and Skill.model_dump, Skill.model_dump_json, and AgentContext.model_dump all reported contains_secret=True; on the PR commit, the same script reported contains_secret=False with <redacted> placeholders for default Skill/AgentContext dumps. Opt-in plaintext still exposed the value, encrypted serialization produced a non-redacted encrypted value, and skill.mcp_tools retained the live value for runtime use.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully and installed the uv-managed workspace. |
| CI Status | ⏳ At QA time: 20 checks successful, 2 skipped, 10 in progress; no CI was rerun. |
| Functional Verification | ✅ Before/after SDK serialization behavior matched the PR goal. |
Functional Verification
Test 1: Skill and AgentContext MCP credentials are masked at rest
Step 1 — Reproduce / establish baseline without the fix:
Ran:
git checkout --quiet origin/main
uv run python /tmp/qa_skill_mcp_serialization.pyObserved excerpt:
in_memory_secret_preserved=True
skill.default: contains_secret=True; length=558
skill.default.env.API_KEY= <fake QA token printed in plaintext>
skill.default.header.Authorization= Bearer <fake QA token printed in plaintext>
skill.model_dump_json.contains_secret=True
agent_context.default: contains_secret=True; length=835
skill.expose_plaintext: contains_secret=True; length=558
skill.expose_encrypted: contains_secret=True; length=558
clean.mcp_tools_is_none= True
This confirms the baseline bug: default Skill serialization, JSON serialization, AgentContext serialization, and encrypted-mode serialization all still carried the MCP credential in plaintext.
Step 2 — Apply the PR's changes:
Checked out the PR commit:
git checkout --quiet 5adcad31a23ad54cf7e9df9d4af838ea95ba59deStep 3 — Re-run with the fix in place:
Ran the same SDK script:
uv run python /tmp/qa_skill_mcp_serialization.pyObserved excerpt:
in_memory_secret_preserved=True
skill.default: contains_secret=False; length=481
skill.default: redacted_placeholders=2
skill.default.env.API_KEY= <redacted>
skill.default.header.Authorization= <redacted>
skill.model_dump_json.contains_secret=False
agent_context.default: contains_secret=False; length=758
agent_context.default: redacted_placeholders=2
skill.expose_plaintext: contains_secret=True; length=558
skill.expose_encrypted: contains_secret=False; length=765
skill.expose_encrypted.env.API_KEY= gAAAAABq...
encrypted_secret_differs_from_redacted= True
clean.mcp_tools_is_none= True
This shows the PR fixes the leak for default at-rest serialization paths while preserving the documented escape hatches: plaintext exposure still works only when requested, encrypted exposure no longer stores plaintext, in-memory MCP values remain available to runtime code, and a Skill without mcp_tools still round-trips as None.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
2 participants
HUMAN:
mask Skill.mcp_tools credentials at rest
AGENT:
Why
Skill.mcp_toolsis an unmodeleddictwith no masking serializer, so any MCP server credential a skill carries inenv/headersis dumped in plaintext wherever aSkillis serialized — even underexpose_secrets=False. This breaks the secret-free-at-rest invariant and is a real, pre-existing latent leak on two at-rest disk paths today (independent of AgentProfiles):base_state.json(SDK) /meta.json(agent-server), viaagent.agent_context.skills[].PersistedSettings→ settings file, via<variant>.agent_context.skills[].OpenHandsAgentSettings.mcp_configalready masks viaserialize_mcp_config, butagent_context.skills[].mcp_toolsbypassed it.This is the root-cause fix for the gap surfaced in the #3757 review (which masked it only at the
OpenHandsAgentProfileboundary). Masking onSkillitself covers every serialization site —AgentContext, conversation state, settings persistence, profiles, and any future container — and lets the per-boundary serializer on branchagent-profile-modelbe retired.Summary
@field_serializer("mcp_tools")onSkillthat routes the value through the sameserialize_mcp_configused for settingsmcp_config(single masking source of truth): redacted by default, plaintext underexpose_secrets, encrypted under a cipher.serialize_mcp_configfunction-locally to avoid thesettings.model → agent_context → skillsmodule-load cycle (the same lazy-import pattern already used elsewhere inskill.py).self.mcp_toolsattribute keeps real values, so runtime MCP usage (which readsagent.mcp_config, a separate field) is unaffected.Issue Number
Follow-up to #3757 review.
How to Test
Ran in the worktree (
uv run):Cross-cutting propagation check (proves the root fix reaches the
AgentContextpath the boundary fix did not):Output: default dump → secret absent;
expose_secrets→ secret present.ruff check,ruff format --check, andpyrighton the changed files: clean (0 errors).New regression tests in
tests/sdk/skills/test_skill_serialization.py: redacted-by-default, exposed-under-expose_secrets, encrypted-under-cipher, real-values-accessible-in-memory, andNoneround-trip.Type
Notes
mcp_toolsre-serializes throughMCPConfig.model_validate(...).model_dump(exclude_defaults=True)(same normalization asmcp_config); structure round-trips with<redacted>placeholders.OpenHandsAgentProfile._serialize_secret_freeboundary mask on branchagent-profile-model([AgentProfile][sdk] AgentProfile kind-discriminated union #3757) becomes redundant and can be removed.Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimgolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:5adcad3-pythonRun
All tags pushed for this build
About Multi-Architecture Support
5adcad3-python) is a multi-arch manifest supporting both amd64 and arm645adcad3-python-amd64) are also available if needed