Repository navigation
fix(agent-server): enforce profile secret scope on runtime-launched conversations - #5193
all-hands-bot wants to merge 1 commit into
Conversation
…onversations OH_RUNTIME_LAUNCHED_PROFILE binds an agent profile to per-conversation container runtimes, but the create-time secret filter introduced in #4931 only ran when the start request named agent_profile_id explicitly. A create request reaching a profile-bound runtime container could inject secrets beyond the profile's secret_refs allow-list; the scope was only enforced later on resume. Filter request secrets by the env-launched profile's secret_refs at creation too. Co-authored-by: openhands <openhands@all-hands.dev>
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
neubig
left a comment
neubig
left a comment
There was a problem hiding this comment.
The secret-scope fix is focused and correctly preserves the None, empty, and populated allow-list semantics. The regression test exercises the real conversation-service path and covers the relevant boundary.
AI disclosure: This review was submitted by OpenHands on behalf of @neubig.
|
@all-hands-bot Please fix the currently failing CI checks, including the PR-description validation and the failing AI disclosure: This comment was generated by OpenHands on behalf of @neubig. |
3 participants
HUMAN:
AGENT:
Why
Found during the weekly high-impact regression hunt (source PR reviewed: #4931 "feat(profiles): scope which secrets an agent profile receives"; applicable risk lenses: #8 Credentials/redaction/sensitive persistence boundaries and #4 Multiple construction and configuration paths).
#4931 added server-side secret scoping: when a conversation is started with
agent_profile_id,request.secretsis filtered by the profile'ssecret_refsallow-list — "Enforced here, not client-side: a caller that sends more secrets than the profile allows must not widen the agent's scope." However, the filter only runs insideif request.agent_profile_id is not None:. In the per-conversation container runtime (docker_runtime, #3403), the profile is bound to the container via theOH_RUNTIME_LAUNCHED_PROFILEenv var and the start request carries an explicitagentwithagent_profile_id=None. On that path no create-time filtering runs at all, even though the env-launched profile (including itssecret_refs) is recorded on the stored conversation and is enforced on resume. The security boundary is therefore inconsistent: create allows scope widening, resume denies it.Deterministic reproduction (against unmodified
main, SDK v1.49.2): setOH_RUNTIME_LAUNCHED_PROFILEto aLaunchedAgentProfilewithsecret_refs=["GITHUB_TOKEN"], thenConversationService.start_conversation()with an explicit agent andsecrets={"GITHUB_TOKEN": ..., "DATADOG_API_KEY": ...}:(log also shows
Added 2 secrets to conversation).Impact: a client starting a conversation inside a profile-bound runtime container can inject arbitrary secrets beyond the profile's allow-list — exactly the scope-widening #4931 set out to prevent server-side.
Root cause:
ConversationService._start_conversationapplies thesecret_refsfilter only on the explicit-agent_profile_idbranch; theOH_RUNTIME_LAUNCHED_PROFILEbranch (read earlier in the same method) never filtersrequest.secrets.Why the original tests did not catch this: #4931's create-time tests (
test_start_conversation_drops_secrets_the_profile_disallows) only exercise theagent_profile_idrequest path, and its resume tests only exercise stored-profile filtering. No test combined the env-var launch binding with a create-time secrets payload.Summary
_start_conversation, when the launched profile comes fromOH_RUNTIME_LAUNCHED_PROFILE(i.e.request.agent_profile_id is None) and itssecret_refsis set, filterrequest.secretsby that allow-list — same enforcement as the explicit-profile path.test_runtime_launched_profile_drops_disallowed_secrets(parametrized overNone/[]/["GITHUB_TOKEN"]) asserting stored secrets and launched-profile provenance through a realConversationService.Issue Number
N/A — found by weekly regression hunt, no tracking issue.
How to Test
Regression test:
main(production fix stashed):2 failed, 1 passed— the scoped cases (secret_refs=[]andsecret_refs=["GITHUB_TOKEN"]) fail becauseDATADOG_API_KEYis stored:11 passed.End-to-end validation (real
ConversationService, real event service, no mocks): a script that setsOH_RUNTIME_LAUNCHED_PROFILEwithsecret_refs=["GITHUB_TOKEN"]and starts a conversation with{"GITHUB_TOKEN", "DATADOG_API_KEY"}secrets prints, after the fix:Neighboring suites all pass with the fix:
Video/Screenshots
N/A — backend service change; console output of the reproduction and test runs is included above.
Design Doc
N/A — narrow security-boundary fix (17 lines of production code).
Type
Notes
agent_profile_idpath is unchanged; when both are present the request-resolved profile still wins (it overwriteslaunched_agent_profilebefore this branch, which is anelif).secret_refs=Noneon the env-launched profile preserves unrestricted behavior, matching the resume-time semantics (null preserves unrestricted behavior for older conversations).🐳 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-jdkpython-node-runtimepython-node-runtimepython-node-runtimegolang: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:719ed42-pythonRun
All tags pushed for this build
About Multi-Architecture Support
719ed42-python) is a multi-arch manifest supporting both amd64 and arm64719ed42-python-amd64) are also available if neededJev-Fast-Audit
⚡ Jev fast audit · estimates · 0.33s · commit 719ed42
Strongest signal: No primary concern selected.
Evidence: No primary concern to locate.
Coverage: complete supplied coverage; 3/3 hunks, 2/2 files.
All estimates and evidence