Enforce profile-selected secret delivery in local and Docker runtimes - #5015
Conversation
An agent profile could not restrict its conversations' secrets: a "Code Exploration" agent that should only read a repo still received the deploy tokens and the production database URL, and the same profile driving an automation handed that set to anyone who could reach the bot. secret_refs holds names only — values live in the secrets store and reach a conversation as LookupSecrets — so the profile stays secret-free at rest, the same way mcp_server_refs references MCP servers. Tri-state matches it too: null = all, [] = none, a list = those names. Enforced in start_conversation rather than client-side, so a caller sending more secrets than the profile allows cannot widen the agent's scope. An ACP profile always additionally receives its own provider credentials, derived from ACP_PROVIDERS: they travel the same channel as the user's saved secrets, and filtering them out would leave the subprocess unable to authenticate. No equivalent carve-out exists for the OpenHands variant, and a test pins that: every other credential it needs rides a channel secret_refs never sees. Fixes #17236 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The ACP provider-credential union made the stored list mean something other than what it said. The credential is an ordinary saved secret — the ACP credential form writes it through the same secrets endpoint — so it appears in the profile editor's picker like any other. A user could clear it, save, and the server would put it back, with the editor showing a state that was never true. It also injected the provider's base URL, which is optional proxy routing rather than authentication. secret_refs is now exactly the allow-list: no derived additions, for either agent kind. An ACP profile that omits its own credential fails to authenticate, loudly and recoverably, which is the honest outcome of the configuration the user saved. The editor selects those credentials by default when scoping starts, so the common path still works and the guardrail stays visible. Drops allowed_secret_names entirely — with nothing derived, the allow-list is the stored field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage Report •
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Taste Rating: 🟢 Good taste
Reviewed the full diff across all 16 files. The PR enforces profile-selected secret delivery in both local and Docker runtimes, adds bash output masking for scoped shell commands, and keeps profiles secret-free at rest.
Key findings:
select_profile_secretsis well-designed: stored saved names take precedence over caller-supplied values, preventing alias attacks. An unscoped profile (secret_refs=None) preserves existing behavior. Backward compatible — old profiles without the field load as unrestricted.- The bash masking approach is thorough:
mask_secrets_in_output(" ")pre-resolves all registered sources (not just command-referenced ones) socompile_stream_mask()covers values that could appear in output without being named in the command (e.g. tokens in git remote URLs). Per-streamStreamOutputMaskinstances correctly handle secrets split across chunk boundaries, andflush()releases held-back text at process exit. _resolve_agent_from_profilereturn type change from 2-tuple to 3-tuple is fully propagated across all call sites and tests.- Docker mediation path correctly applies the same
select_profile_secretsfilter before serialization, and the test verifies canary values do not appear in the wire format. - The
dependencies.pywiring (secret_registry=event_service.get_conversation().state.secret_registry) correctly passes the live registry by reference;update_secretsmutates the dict in place, so the bash service sees updates.
No material bugs, security issues, or design flaws found. The changes are well-contained: secret_refs defaults to None, the BashEventService masking only activates when a registry is wired, and the agent's TerminalTool path is unaffected.
[RISK ASSESSMENT]
- Overall PR: 🟢 LOW
secret_refsdefaults toNone→ no behavior change for existing profiles or eval runs.- BashEventService is the UI terminal-tab API, not the agent's TerminalTool, so benchmark performance is not affected.
- The
select_profile_secretsfilter only runs on theagent_profile_idlaunch path, which is not the typical eval path.
VERDICT: ✅ Worth merging
KEY INSIGHT: The side-effect pattern of calling mask_secrets_in_output(" ") to pre-resolve uncached sources before compiling the stream mask is correct but indirect — a dedicated resolve_all() method would be more self-documenting, though not worth blocking on.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Review Summary
This PR enforces profile-selected secret delivery across local and Docker runtimes, adds output masking for scoped bash commands, and introduces the secret_refs allow-list field on agent profiles. The implementation is clean, well-tested, and the security design is sound.
Key observations
Security design — solid:
select_profile_secretscorrectly makes stored values authoritative over caller-supplied values, preventing aliasing attacks where a caller widens the scope by supplying a differentLookupSecretunder an allowed name.- The
secret_refsfield is a strict allow-list with no implicit additions — an ACP profile that omits its provider credential simply doesn'''t receive it, which is the right failure mode (loud, recoverable). - Output masking via
StreamOutputMaskcorrectly handles secrets split across read boundaries, and themask_secrets_in_output(" ")pre-resolution ensures all secret values are cached before the stream masks are compiled. - Proper
serialize_secret()usage onStaticSecret— no hand-rolled redaction. - Secret values never appear in profile definitions or system prompts; only names are advertised.
Backward compatibility — fine:
secret_refsis additive withdefault=None; old profiles load unchanged (verified bytest_a_profile_without_the_key_loads_unrestricted).- No schema version bump needed for an optional additive field.
- Non-profile conversations (
agent_profile_id is None) are completely unaffected —select_profile_secretsis only called inside the profile branch. - TypeScript client marks the field optional (
?).
Tests — thorough and real:
test_scoped_bash_injects_registry_and_masks_split_outputruns a real subprocess, verifies env injection, split-output masking, stderr masking, and persistence safety (no secret in event files).test_profile_secret_scope_precedes_docker_lookupexercises the full Docker mediation path with real profile resolution, serialization, and prompt rendering.test_start_conversation_drops_secrets_the_profile_disallowsverifies the server-side filter on the local path.
Eval-risk flag
This PR modifies bash service output handling (secret masking) and environment variable injection from the secret registry. While the no-secrets path is functionally unchanged (StreamOutputMask(None, 0) is a no-op), this touches terminal/stdout handling which could plausibly affect benchmark/evaluation performance. No eval monitor link is provided in the PR description.
Per the repo'''s review policy, I'''m leaving a COMMENT rather than APPROVE. A human maintainer should decide whether lightweight evals are needed to confirm no regression in benchmark scenarios where secrets are configured.
Risk Assessment
LOW — No material bugs or security issues found. The design is principled and the implementation is well-tested. The only flag is the eval-risk consideration for bash output handling changes.
[RISK ASSESSMENT]
- [Overall PR] Risk Assessment: LOW
The code is correct and the security model is sound. The eval-risk flag is procedural (bash/terminal output handling is in the eval-sensitive category) rather than indicating a concrete problem.
VERDICT: No material findings. Code is ready for merge pending human maintainer eval decision.
KEY INSIGHT: The two-function split (filter_profile_secrets in the SDK as a pure utility, select_profile_secrets in the agent-server as a store-resolving enforcer) cleanly separates the concerns of client-side filtering from server-side enforcement.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run/iterateto automatically drive this PR through CI, review, and QA until it'''s merge-ready.
Was this review helpful? React with thumbs up or thumbs down to give feedback.
…actory/restack-secrets
e747da2 to
379d41a
Compare
|
Consolidated during the factory stack audit (#5016). GitHub automatically marked this PR merged into a feature branch when its commits became ancestors; this was not a merge to main. Current native stack OpenHands/OpenHands#5018 is OpenHands/OpenHands#4966 (shared API/lifecycle contracts) -> OpenHands/OpenHands#5017 (shared profile-secret enforcement, also using OpenHands/OpenHands#4931) -> OpenHands/OpenHands#3403 (final Docker implementation, including Docker lifecycle/release/credential handoff). Please review that stack; this PR is no longer a separate merge prerequisite. |
4 participants
HUMAN:
AGENT:
Why
Profile-only automation launches did not receive selected saved secrets. Docker materialization also bypassed the profile allow-list in OpenHands/OpenHands#4931. As a result, the factory embedded its gateway grants in uploaded bundles instead of receiving them through its profile.
Summary
codeact_agentOpenHands#4931's secret_refs contract and apply one selector before local construction or Docker materialization. Stored selected names take precedence over caller-supplied aliases; excluded lookups never run.REST API contract changes
Compared with base OpenAPI
1f494dfac753for public/api/**paths.Issue Number
Fixes OpenHands/OpenHands#5014. Integrates OpenHands/OpenHands#4931 (#5030).
How to Test
uv run --frozen --group dev pytest tests/agent_server/docker_runtime/test_mediation.py tests/agent_server/test_agent_profile_conv_start.py tests/sdk/profiles/test_secret_refs.py tests/agent_server/test_bash_service.py tests/agent_server/test_runtime_router.py -qThe local/Docker live probe used the public AsyncAgentServerClient to create a profile-only conversation and execute the same environment assertion in each runtime. Both exited zero, exposed only SELECTED_TOKEN, excluded UNRELATED_TOKEN, masked printed values, retained profile provenance, and persisted encrypted selected values. No real credentials were used. Commands, resource limits, and evidence are in
.pr/profile-secret-boundary.mdand.pr/profile-secret-live.json.Video/Screenshots
Machine-readable live evidence:
.pr/profile-secret-live.json; both runtimes report PROFILE_SECRET_BOUNDARY_PASS.Type
Notes
Stacked on OpenHands/OpenHands#5008. Review OpenHands/OpenHands#4931's profile selection first, then OpenHands/OpenHands#4966 -> OpenHands/OpenHands#3403 -> OpenHands/OpenHands#5008 -> this integration. This branch merges the existing OpenHands/OpenHands#4931 rather than reimplementing its schema. The integration's own changes are separate commits. Companion documentation: OpenHands/docs#793.
Factory consumption is in OpenHands/extensions#562. Upstream GitHub credentials remain in the trusted gateway; profiles carry only their role-specific gateway grant. Token permissions must be restricted at issuance independently of secret selection. Local workspaces share their host and are not security sandboxes.
Current validation
Final focused validation: 88 tests passed. Both real local/Docker environment probes passed, and the disposable probe servers/container were stopped. Native stack OpenHands/OpenHands#4969 includes this PR after OpenHands/OpenHands#5008.
🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimnikolaik/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:e747da2-pythonRun
All tags pushed for this build
About Multi-Architecture Support
e747da2-python) is a multi-arch manifest supporting both amd64 and arm64e747da2-python-amd64) are also available if needed