feat(acp): type acp_env values as SecretStr to mask secrets in memory#3436
feat(acp): type acp_env values as SecretStr to mask secrets in memory#3436simonrosenberg wants to merge 4 commits into
Conversation
Closes #3430. `ACPAgent.acp_env` and `ACPAgentSettings.acp_env` held plaintext `str` values, so masking happened only at serialization time — `repr(agent)`, logs, and tracebacks leaked provider API keys. Type the values as `SecretStr` so they're masked in memory too (the field_serializer already handles redact/encrypt/expose on the wire). To avoid a REST contract break, a shared `SecretEnvValue` alias pins the JSON/OpenAPI schema to a plain `string` via `WithJsonSchema` instead of SecretStr's default `format: password, writeOnly: true` (which would drop the field from response schemas). Verified the generated agent-server OpenAPI for `acp_env` is byte-identical to main (`additionalProperties: {type: string}`, no writeOnly) — no oasdiff breakage. - `ACPAgent.acp_env` / `ACPAgentSettings.acp_env`: `dict[str, SecretEnvValue]` - serializers: drop the now-redundant `SecretStr(v)` wrap - spawn site unwraps via `.get_secret_value()` - `resolve_acp_env()` returns `dict[str, SecretStr]` (wraps provider env) - decryption validators unchanged (the shared helper passes SecretStr through) Note: because `dict` is invariant, constructing with a `dict[str, str]` literal no longer type-checks (runtime still coerces). Callers pass `SecretStr` values or go through `create_agent()`/`model_validate`. The OpenHands app builds the runtime agent via `create_agent()` (covered); any direct `ACPAgent(acp_env=...)` construction there should wrap values. Test plan: 433 passed across acp_agent / settings / settings_router / remote-logging / validation-sanitization / pydantic_secrets / agent_context suites; repr-masking assertions added. 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 •
|
||||||||||||||||||||||||||||||
The acp_env field is now dict[str, SecretStr]; update the test_start_conversation_accepts_acp_agent_settings assertion to compare plaintext via get_secret_value(). Missed in the initial sweep. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
acp_env is now dict[str, SecretStr]; compare plaintext via get_secret_value() in test_start_conversation_accepts_acp_agent_settings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
simonrosenberg
force-pushed
the
feat-acp-env-secretstr
branch
from
3548652 to
c10daeb
Compare
acp_env is now dict[str, SecretStr]; compare plaintext via get_secret_value() in test_start_conversation_accepts_acp_agent_settings, in ruff-canonical formatting. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: feat(acp): type acp_env values as SecretStr to mask secrets in memory
Overall Assessment: ✅ Looks good — clean, well-scoped security improvement with excellent test coverage.
Summary
This PR correctly fixes a real security gap: acp_env dict values were stored as plain str in memory, leaking through repr(), log-string interpolation, and exception tracebacks. Typing them as SecretStr puts acp_env on the same footing as every other secret scalar in the SDK (LLM.api_key, StaticSecret.value, etc.).
The implementation is technically sound:
SecretEnvValue = Annotated[SecretStr, WithJsonSchema({"type": "string"})]sidesteps the OpenAPI schema breakage that bareSecretStrwould cause (writeOnly: truedrops fields from response schemas — a REST-breaking change). Pinning the schema to{"type": "string"}preserves the existing wire format exactly.validate_secret_dictalready handles bothstr → SecretStrcoercion andSecretStr-passthrough, so the existing cipher round-trip / plaintext-passthrough / undecryptable-warning paths work unchanged.- All three use-sites are updated consistently: field type declarations,
field_serializers (redundantSecretStr(v)wrap removed), and the subprocess spawn site (.get_secret_value()unwrapping added). resolve_acp_env()wraps provider-derivedstrvalues inSecretStr, keeping the returned dict uniformly masked.repr-masking assertions added to redaction tests — good regression guard.
The PR description is unusually thorough: problem statement, fix rationale, OpenAPI non-regression evidence, and tradeoff acknowledgement all make this easy to review with confidence.
Minor observations
_secret_envhelper only intest_acp_agent.py:test_settings.pyuses inlineSecretStr(...)instead. Mild inconsistency — not a correctness issue, but worth harmonising if the pattern grows._serialize_acp_envtype annotation usesSecretStrnotSecretEnvValue: SinceSecretEnvValue = Annotated[SecretStr, ...], the runtime type isSecretStr, so this is correct. UsingSecretEnvValuein the serializer signatures would add documentation consistency, but it is a very minor point.resolve_acp_env()merge:{k: SecretStr(v) for k, v in self.resolve_provider_env().items()}—resolve_provider_env()returnsdict[str, str]so there is no double-wrapping risk. Clear and correct.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
| # schemas (writeOnly) and is a breaking REST change; it's also inaccurate | ||
| # here, since the value really is returned as a (masked) string. Validation | ||
| # is unaffected — plain strings are still coerced to ``SecretStr``. | ||
| SecretEnvValue = Annotated[SecretStr, WithJsonSchema({"type": "string"})] |
There was a problem hiding this comment.
Well-designed alias. Using WithJsonSchema({"type": "string"}) to override pydantic's SecretStr defaults (format: password, writeOnly: true) is exactly the right approach — it avoids the REST breaking change while keeping full in-memory masking.
Worth noting for future use: if any other dict[str, str]-style secret maps are added to the SDK (e.g., MCP env headers, other subprocess envs), SecretEnvValue is ready to reuse from here rather than re-inventing the pattern.
| env = default_environment() | ||
| env.update(os.environ) | ||
| env.update(self.acp_env) | ||
| env.update({k: v.get_secret_value() for k, v in self.acp_env.items()}) |
There was a problem hiding this comment.
Correct. This is the single 'exit point' where the SecretStr shell is shed. By keeping values masked as SecretStr from field construction all the way through to here, all earlier paths — serialization, repr, logging, tracebacks — are protected. .get_secret_value() should only appear at the subprocess boundary, which is exactly what this does.
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def _secret_env(values: dict[str, str]) -> dict[str, SecretStr]: |
There was a problem hiding this comment.
Nice helper. One nit: test_settings.py builds acp_env with inline SecretStr(...) rather than using _secret_env. If more test files start constructing acp_env dicts, consider moving _secret_env (or an equivalent) to a shared conftest.py. At the current scope it is fine.
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review
This PR correctly fixes a real security gap: acp_env values were stored as plaintext str in memory, leaking through repr(), log-string interpolation, and exception tracebacks. The implementation is clean and consistent with the SDK's existing secret-handling patterns.
The existing review already covers the technical mechanics well. I want to highlight two additional points:
⚠️ resolve_acp_env() is a behavioral public API change — caller adaptation required
resolve_acp_env() previously returned dict[str, str] (values ready for subprocess use). It now returns dict[str, SecretStr]. This is the correct design decision, but direct callers must adapt.
Failure mode to be aware of: In Python, str(SecretStr("my-api-key")) evaluates to ********** — not the actual value. So if a caller does:
env = os.environ.copy()
env.update(settings.resolve_acp_env()) # SecretStr objects silently inserted
subprocess.run(cmd, env=env) # subprocess receives "**********" instead of real secretsThis fails silently — the subprocess receives masked values and likely crashes or misbehaves with no obvious indication of what went wrong.
The updated docstring documents the new return type, and the correct adaptation is:
env.update({k: v.get_secret_value() for k, v in settings.resolve_acp_env().items()})Recommend adding a note to the CHANGELOG / migration guide flagging this as a behavioral change for any callers using resolve_acp_env() directly outside of create_agent().
✅ Plain-string construction of acp_env remains backward-compatible
The PR updates test literals to use SecretStr(...) explicitly:
acp_env={"OPENAI_API_KEY": SecretStr("sk-real-secret")}Worth noting (and perhaps documenting) that plain strings still work:
acp_env={"OPENAI_API_KEY": "sk-real-secret"} # also valid — Pydantic coerces str → SecretStrPydantic v2 coerces plain str to SecretStr for SecretStr-typed fields, and validate_secret_dict explicitly handles the coercion path too. So the construction API is backward-compatible — only the stored type changes. The explicit SecretStr(...) in tests is a clear stylistic choice, but users should know they don't have to change their existing acp_env dict construction.
Overall
The core fix is correct and well-executed. The SecretEnvValue alias with WithJsonSchema({"type": "string"}) is exactly the right approach to avoid REST schema breakage. The subprocess unwrap site is the correct single exit point. Tests are comprehensive. ✅
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
| return env | ||
|
|
||
| def resolve_acp_env(self) -> dict[str, str]: | ||
| def resolve_acp_env(self) -> dict[str, SecretStr]: |
There was a problem hiding this comment.
Heads-up — public API behavioral change: Prior to this PR, resolve_acp_env() returned dict[str, str] (values ready to pass directly to subprocess.Popen(env=...) or os.environ.update()). It now returns dict[str, SecretStr].
Any caller doing:
env.update(settings.resolve_acp_env())will silently inject SecretStr objects instead of plain strings. When converted to string (e.g., via os.environ.update() or f-string interpolation), str(SecretStr("...")) evaluates to "**********" — a masked value — which would cause the subprocess to receive masked values silently instead of the real secrets.
The updated docstring documents this correctly. Consider adding a CHANGELOG/migration note that direct callers need to adapt:
env.update({k: v.get_secret_value() for k, v in settings.resolve_acp_env().items()})
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
The ACP env secret-masking behavior works as described: PR values are masked in memory while serialization, encrypted round-trip, subprocess env injection, and OpenAPI shape remain functional.
Does this PR achieve its stated goal?
Yes. On main, constructing ACPAgent / ACPAgentSettings with a synthetic acp_env secret leaked the value through repr(...) and repr(agent.acp_env); on f0271ea1231b97ec12aa3d06ef5f53ee621ac61f, the same SDK usage stores values as SecretStr and those repr paths no longer expose the plaintext. I also verified that exposed/encrypted serialization still preserves the value when requested, create_agent() still carries the resolved env, the ACP subprocess startup path receives the plaintext env value, and the real /openapi.json response keeps acp_env as a plain string map.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully with uv sync --dev. |
| CI Status | ✅ Product/CI checks observed green; one qa-changes automation run was still in progress and an earlier one was cancelled. |
| Functional Verification | ✅ Real SDK construction/serialization and agent-server OpenAPI behavior verified before/after. |
Functional Verification
Test 1: SDK ACP env masking and serialization
Step 1 — Reproduce / establish baseline (without the fix):
Ran git switch --detach origin/main && uv run python /tmp/qa_acp_secret_behavior.py with a synthetic OPENAI_API_KEY in acp_env:
{
"agent_env_value_type": "str",
"settings_env_value_type": "str",
"resolved_env_value_type": "str",
"agent_repr_leaks_secret": true,
"agent_env_repr_leaks_secret": true,
"settings_repr_leaks_secret": true,
"agent_default_json_dump": {"OPENAI_API_KEY": "**********", "BASE_URL": "**********"},
"agent_exposed_json_dump": {"OPENAI_API_KEY": "sk-qa-secret-12345", "BASE_URL": "https://llm-proxy.example.test"},
"encrypted_payload_contains_plaintext": false,
"restored_env_revealed": {"OPENAI_API_KEY": "sk-qa-secret-12345"}
}This confirms the reported bug: serialization already redacted/encrypted, but in-memory repr/direct-dict paths exposed the plaintext because values were plain str.
Step 2 — Apply the PR's changes:
Checked out f0271ea1231b97ec12aa3d06ef5f53ee621ac61f.
Step 3 — Re-run with the fix in place:
Ran uv run python /tmp/qa_acp_secret_behavior.py:
{
"agent_env_value_type": "SecretStr",
"settings_env_value_type": "SecretStr",
"resolved_env_value_type": "SecretStr",
"created_agent_env_value_type": "SecretStr",
"agent_repr_leaks_secret": false,
"agent_env_repr_leaks_secret": false,
"settings_repr_leaks_secret": false,
"agent_default_json_dump": {"OPENAI_API_KEY": "**********", "BASE_URL": "**********"},
"agent_exposed_json_dump": {"OPENAI_API_KEY": "sk-qa-secret-12345", "BASE_URL": "https://llm-proxy.example.test"},
"created_agent_env_revealed": {"OPENAI_API_KEY": "sk-qa-secret-12345"},
"encrypted_payload_contains_plaintext": false,
"restored_env_revealed": {"OPENAI_API_KEY": "sk-qa-secret-12345"}
}This shows the fix works for the stated goal: the same user-level construction path now masks in-memory repr output while preserving value access for explicit expose/encryption and agent creation flows.
Test 2: REST/OpenAPI compatibility through a real server
Step 1 — Establish baseline:
On main, started the actual server with uv run agent-server --host 127.0.0.1 --port 18081 and queried curl http://127.0.0.1:18081/openapi.json for ACP schemas:
{
"ACPAgent-Input": {"additionalProperties": {"type": "string"}, "description": "Additional environment variables for the ACP server process", "title": "Acp Env", "type": "object"},
"ACPAgent-Output": {"additionalProperties": {"type": "string"}, "description": "Additional environment variables for the ACP server process", "title": "Acp Env", "type": "object"}
}This establishes the previous REST contract: acp_env is a string-valued object in both input and output schemas.
Step 2 — Apply the PR's changes:
Checked out f0271ea1231b97ec12aa3d06ef5f53ee621ac61f.
Step 3 — Re-run with the fix in place:
Started uv run agent-server --host 127.0.0.1 --port 18080 and queried the same /openapi.json path:
{
"ACPAgent-Input": {"additionalProperties": {"type": "string"}, "description": "Additional environment variables for the ACP server process", "title": "Acp Env", "type": "object"},
"ACPAgent-Output": {"additionalProperties": {"type": "string"}, "description": "Additional environment variables for the ACP server process", "title": "Acp Env", "type": "object"}
}The schema is unchanged for the ACP env property, so the PR does not introduce the SecretStr writeOnly/password schema break it set out to avoid.
Test 3: ACP subprocess env injection
Step 1 — Exercise the changed runtime path:
On the PR commit, ran uv run python /tmp/qa_acp_subprocess_start.py, which constructs an ACPAgent with acp_env={"OPENAI_API_KEY": "sk-qa-subprocess-secret"} and starts a temporary child command that writes the received env to /tmp/qa_acp_subprocess_env.json.
Observed output/side effect:
{"OPENAI_API_KEY": "sk-qa-subprocess-secret"}The temporary child was intentionally not a full ACP server, so the conversation initialization did not complete, but the subprocess started and received the plaintext env value. This specifically exercises the spawn/env handoff that must unwrap SecretStr before launching the ACP process.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
2 participants
Closes #3430.
Problem
ACPAgent.acp_envandACPAgentSettings.acp_envwere typeddict[str, str]. Masking happened only at serialization time (thefield_serializerwraps each value inSecretStron the way out), so the in-memory values were plaintext and leaked through paths that bypass the serializer:repr(agent)/print(agent)— pydantic renders the raw dict valueslogger.info(f"...{agent.acp_env}")or any direct log of the dictEvery other secret scalar in the SDK (
LLM.api_key,StaticSecret.value, …) isSecretStr;acp_envwas the odd one out.Fix
Type the values as
SecretStrso they're masked in memory. A shared alias keeps the wire/OpenAPI schema unchanged:WithJsonSchema({"type": "string"})pins the JSON/OpenAPI schema to a plainstringinstead ofSecretStr's defaultformat: password, writeOnly: true. That default would drop the field from response schemas (writeOnly) — a breaking REST change — and is also inaccurate here, since the value is returned (as a masked/encrypted string).ACPAgent.acp_env/ACPAgentSettings.acp_env→dict[str, SecretEnvValue]SecretStr(v)wrap.get_secret_value()resolve_acp_env()returnsdict[str, SecretStr](wraps provider-derived env)validate_secret_dicthelper passesSecretStrthrough and coerces decryptedstr→SecretStrNo REST breakage (verified locally)
The generated agent-server OpenAPI for both
acp_env-bearing schemas (ACPAgent-Input/-Output) is byte-identical tomain:Python API/REST API (OpenAPI)breakage checks should stay green.Tradeoff to be aware of
Because
dictis invariant in its value type, constructing with adict[str, str]literal no longer type-checks (dict[str,str]⊄dict[str,SecretStr]) — runtime still coerces via the before-validator. Callers either passSecretStrvalues or go throughcreate_agent()/model_validate. The OpenHands app builds the runtime agent viacreate_agent()(covered byresolve_acp_env); any directACPAgent(acp_env=...)construction there should wrap values inSecretStr(runtime works regardless). This is the inherent cost of in-memory masking for a dict field.Test plan
uv run pytestover acp_agent / settings / settings_router / remote-request-logging / validation-sanitization / pydantic_secrets / agent_context — 433 passed. Addedrepr()-masking assertions to the redaction tests; existing cipher round-trip / plaintext-passthrough / undecryptable-warning tests updated to unwrapSecretStr.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:f0271ea-pythonRun
All tags pushed for this build
About Multi-Architecture Support
f0271ea-python) is a multi-arch manifest supporting both amd64 and arm64f0271ea-python-amd64) are also available if needed