fix: expand per-conversation secrets in plugin source/ref before fetch#3758
Conversation
Expand ${VAR} placeholders in a plugin's source URL and ref against the
per-conversation secret registry before fetching, so private plugin repos
can be cloned by referencing a user-profile secret, an API-passed secret,
or an OpenHands-managed provider token (e.g. ${BITBUCKET_DATA_CENTER_TOKEN}).
Secrets only (check_env=False); braced-only to avoid mangling literal '$'
in tokens; persistence built from the original spec so the placeholder
(not the secret) is stored and inline credentials stay redacted.
Closes #3755
Co-authored-by: openhands <openhands@all-hands.dev>
jpshackelford
added
customer-support
OpenHands AI
jpshackelford
added
plugins
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||
jpshackelford
left a comment
jpshackelford
left a comment
There was a problem hiding this comment.
Code Review (Roasted) — /codereview
Taste Rating: 🟢 Good taste
This is the kind of change I like to see: small (+155/-4, 2 files), it reuses an existing, well-understood helper instead of inventing a new expansion path, and every non-obvious decision is justified. It plugs a genuine hole — the plugin source/ref was the one secret-consuming site that never read the registry, so ${TOKEN} reached git clone literally and 500'd at conversation start. The fix is the obvious sibling of the MCP-config (#2873) and MCP-tool-param (#3278) work, and it sits right next to the MCP expansion it mirrors. No special cases, no new abstractions, no behavior change for the existing hard-coded-token path.
I tried to find something to roast. The codebase made it hard. Walking the blast radius:
- Persistence stays clean (verified).
ResolvedPluginSource.from_plugin_source(spec, …)is built from the originalspec, not the expanded value, and then runs throughredact_url_credentials(). I confirmed both forms redact tohttps://****@host/.... The test asserting"s3cr3t-value" not in resolved_plugins[0].sourceis the right guard. - Logging stays clean (verified). The
logger.debug(... from {spec.source} ...)uses the original placeholder, and the lower-levelrun_git_commandalready redacts credentials via_redact_args_for_loggingbefore anylogger.error/raise. So even a clone failure with a freshly-expanded token doesn't spill the secret into logs. The PR rides on that existing safety net correctly. - Security defaults are correct (verified).
check_env=False(no host-env exfiltration into a remote URL),support_unbraced=False(a literal$in a token isn't mangled),expand_defaults=False(an unknown${VAR}is preserved verbatim, not silently defaulted). The negative testtest_host_env_not_expanded_in_sourceactually proves thecheck_env=Falseinvariant rather than just asserting the happy path. Good. - No bypass paths. The only conversation-start fetch site is
_ensure_plugins_loaded;Plugin.fetch()is a separate public API with an explicit caller-supplied source._plugin_specsis seeded from the constructor (original placeholders), so resume re-expands freshly against the registry rather than from redacted persisted state. - Tests pass (15/15) and exercise the real
expand_variable_references+SecretRegistry— only the networkgit cloneis faked. That's a legitimate fake (ephemeral boundary), not the "mocks asserting mocks" anti-pattern.
[IMPROVEMENT OPPORTUNITIES] (minor, non-blocking)
-
local_conversation.py~L657–L678 — light DRY. Theexpand_variable_references(..., check_env=False, support_unbraced=False, expand_defaults=False)call is duplicated verbatim forsourceandref, differing only in the input. A tiny local closure would remove the repetition and guarantee the two calls can't drift apart:def expand(value): return expand_variable_references( value, get_secret=get_secret, check_env=False, support_unbraced=False, expand_defaults=False, ) fetch_source = expand(spec.source) fetch_ref = expand(spec.ref) if spec.ref else spec.ref
Purely cosmetic — current form is correct.
-
local_conversation.py~L750 (pre-existing comment) — slightly inaccurate. The MCP block comments that plugin loading "preserves placeholders withexpand_defaults=Falseto avoid double-expansion."source/refnever entermerged_mcp, so there's no actual double-expansion to avoid here; the real reasons are the ones in your new comment (no surprising defaults in a URL). Not introduced by this PR, not worth churning.
[STYLE NOTES]
None worth raising. And before anyone flags the ~14-line comment block on the expansion call as "noise": it documents the check_env=False / support_unbraced=False security rationale — exactly the kind of non-inferable invariant that should be commented. Keep it.
[TESTING GAPS]
None blocking. Coverage is well-chosen (positive source, negative host-env, ref). If you wanted gold-plating you could add a ${VAR:-default} case to pin down that expand_defaults=False leaves ${MISSING:-foo} untouched in a URL, but that's the helper's contract and already tested elsewhere — optional.
One soft note on the PR description: per the strict reading of our review rubric, test runs alone aren't "evidence." For internal SDK plumbing like this the deterministic tests are reasonable proof, but if you want to fully satisfy the bar, drop in the literal command + output (or the originating conversation URL) under an Evidence: heading.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Behavior is opt-in (only fires when a user authors a ${VAR} in source/ref), defaults are conservative (secrets-only, braced-only, no defaults), persisted state and git logging are both already redacted, and there's no change to the existing hard-coded-token path. The one residual is the documented and intentional #4 footgun — a user can embed ${GITHUB_TOKEN} into a URL for an unrelated host and send a managed token off-host. The PR correctly calls this out as user-authored and out of scope; a docs warning would be the right follow-up, not a blocker here.
VERDICT: ✅ Worth merging
Tight, correct, well-tested, and it leans on existing redaction infrastructure instead of reinventing it. Address the DRY nit if you feel like it; otherwise ship it.
KEY INSIGHT
The actual engineering win is restraint: the only real change is "expand source/ref through the same registry the sibling paths already use, with the safest possible flags," and the security correctness falls out of infrastructure (redact_url_credentials, _redact_args_for_logging, original-spec persistence) that already existed — the PR's job was to wire into it without poking a new hole, and it does exactly that.
Reviewed against PR head 0d2e7146: read the changed code plus surrounding _ensure_plugins_loaded, the expand_variable_references helper, ResolvedPluginSource.from_plugin_source, and the git command/redaction layer, and ran the test file (15/15 passing).
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, add a
.agents/skills/custom-codereview-guide.mdfile to your branch with the/codereviewtrigger and the missing context, then re-request a review. See the customization docs.Resolve with AI? Install the iterate skill and run
/iterateto drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎.
This review was generated by an AI agent (OpenHands) on behalf of @jpshackelford.
Address non-blocking code-review feedback on the plugin source/ref secret
expansion: hoist the expand_variable_references(...) call into a single
local helper so the source/ref calls cannot drift, and add a regression
test pinning expand_defaults=False (an unresolved ${MISSING:-default} is
left verbatim, not substituted).
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
The PR achieves its stated goal: plugin source and ref placeholders are expanded from conversation secrets before fetch, while persisted resolved source remains redacted/placeholder-based.
Does this PR achieve its stated goal?
Yes. I exercised the SDK as a user by creating Conversation(...) instances with PluginSource(...), adding secrets through conversation.update_secrets(...), and calling conversation.send_message(...) to trigger real plugin fetching/loading. On origin/main, git attempted to clone/fetch the literal ${...} placeholders and raised PluginFetchError; on the PR head, the same operations loaded the plugin skills successfully, and the resolved source still preserved ${PLUGIN_SEGMENT} rather than the fake secret segment.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed and created the project .venv |
| CI Status | ✅ All visible PR checks are successful; cleanup-on-approval is skipped |
| Functional Verification | ✅ Source URL secret expansion and ref secret expansion both verified before/after |
Functional Verification
Test 1: Secret placeholder in plugin source is expanded before fetch
Step 1 — Reproduce / establish baseline (without the fix):
Ran git checkout --detach origin/main && uv run python /tmp/qa_plugin_secret_expansion.py.
Relevant output:
Cloning repository from file:///tmp/oh-plugin-secret-qa-ho7_7zh6/repos/${PLUGIN_SEGMENT}/private-plugin
fatal: '/tmp/oh-plugin-secret-qa-ho7_7zh6/repos/${PLUGIN_SEGMENT}/private-plugin' does not appear to be a git repository
RESULT=error
SOURCE_TEMPLATE=file:///tmp/oh-plugin-secret-qa-ho7_7zh6/repos/${PLUGIN_SEGMENT}/private-plugin
ACTUAL_PLUGIN_PATH=/tmp/oh-plugin-secret-qa-ho7_7zh6/repos/credential-segment/private-plugin
ERROR_TYPE=PluginFetchError
ERROR=Failed to fetch plugin from file:///tmp/oh-plugin-secret-qa-ho7_7zh6/repos/${PLUGIN_SEGMENT}/private-plugin
This confirms the bug exists on main: the registered conversation secret was ignored and the literal ${PLUGIN_SEGMENT} reached git, so the private-plugin path could not be fetched.
Step 2 — Apply the PR's changes:
Checked out the PR branch at 88716f9f545410886f5a273d308f2106f181cd88 with git checkout expand-secrets-in-plugin-source && git reset --hard origin/expand-secrets-in-plugin-source.
Step 3 — Re-run with the fix in place:
Ran uv run python /tmp/qa_plugin_secret_expansion.py.
Relevant output:
Cloning repository from file:///tmp/oh-plugin-secret-qa-n1o6h3xr/repos/credential-segment/private-plugin
Loaded 1 plugin(s) via Conversation
RESULT=success
SOURCE_TEMPLATE=file:///tmp/oh-plugin-secret-qa-n1o6h3xr/repos/${PLUGIN_SEGMENT}/private-plugin
ACTUAL_PLUGIN_PATH=/tmp/oh-plugin-secret-qa-n1o6h3xr/repos/credential-segment/private-plugin
SKILLS=['qa-private-skill']
RESOLVED_SOURCES=['file:///tmp/oh-plugin-secret-qa-n1o6h3xr/repos/${PLUGIN_SEGMENT}/private-plugin']
SECRET_IN_RESOLVED_SOURCE=False
This confirms the fix works for source: git fetched the expanded URL, the plugin skill became available, and resolved_plugins did not persist the secret-expanded segment.
Test 2: Secret placeholder in plugin ref is expanded before fetch
Step 1 — Reproduce / establish baseline (without the fix):
Ran git checkout --detach origin/main && uv run python /tmp/qa_plugin_ref_secret_expansion.py.
Relevant output:
Git command failed: git clone --depth 1 --branch '${PLUGIN_REF}' file:///tmp/oh-plugin-ref-qa-yj1xb5lo/repos/ref-plugin ...
fatal: Remote branch ${PLUGIN_REF} not found in upstream origin
RESULT=error
SOURCE=file:///tmp/oh-plugin-ref-qa-yj1xb5lo/repos/ref-plugin
REF_TEMPLATE=${PLUGIN_REF}
EXPECTED_REF=qa-secret-ref
ERROR_TYPE=PluginFetchError
ERROR=Failed to fetch plugin from file:///tmp/oh-plugin-ref-qa-yj1xb5lo/repos/ref-plugin
This confirms the baseline behavior for ref: the placeholder was passed literally as the branch name, so git could not find the branch even though the conversation secret specified qa-secret-ref.
Step 2 — Apply the PR's changes:
Checked out the PR branch again at 88716f9f545410886f5a273d308f2106f181cd88.
Step 3 — Re-run with the fix in place:
Ran uv run python /tmp/qa_plugin_ref_secret_expansion.py.
Relevant output:
Cloning repository from file:///tmp/oh-plugin-ref-qa-3hfdvkkb/repos/ref-plugin
Loaded 1 plugin(s) via Conversation
RESULT=success
SOURCE=file:///tmp/oh-plugin-ref-qa-3hfdvkkb/repos/ref-plugin
REF_TEMPLATE=${PLUGIN_REF}
EXPECTED_REF=qa-secret-ref
SKILLS=['qa-ref-skill']
RESOLVED_REFS=['663a01bac0abdd931eed6bb14a831170cbad9128']
This confirms the fix works for ref: the placeholder was expanded to the secret branch name before git fetch/clone, and the loaded plugin exposed the expected skill.
Issues Found
None.
This review was generated by an AI agent (OpenHands) on behalf of the user.
4 participants
What & why
Expand
${VAR}placeholders in a plugin'ssourceURL (andref) against the per-conversation secret registry before the plugin is fetched, so a private plugin repository can be cloned by referencing a secret rather than only by hard-coding the raw token.Closes #3755.
This is the missing sibling of secret expansion for MCP config (#2872 / #2873) and MCP tool parameters (#3277 / #3278). The plugin source path was the only secret-consuming site that never read from the registry, so today a placeholder like
https://x-token-auth:${BITBUCKET_DATA_CENTER_TOKEN}@host/repo.gitreachesgit cloneliterally and fails (surfacing as a500at conversation start).The four ways a customer can supply a git credential for a private plugin
sourceURL${MY_TOKEN}secretsfield)${GITHUB_TOKEN},${GITLAB_TOKEN},${BITBUCKET_TOKEN},${BITBUCKET_DATA_CENTER_TOKEN}Only scenario 1 worked before — and it is the least desirable option (raw token in the request payload and in not-yet-redacted spec serialization). Scenarios 2–4 are the patterns we want customers to use.
Why this one change unlocks scenarios 2–4: all three already land in the SDK
SecretRegistrybefore plugins are fetched — they just were never consumed by the plugin path:AgentContext.secrets, seeded into the registry inLocalConversation.__init__(fill-if-absent).StartConversationRequest.secrets, applied to the registry (higher priority).f"{PROVIDER}_TOKEN"(SaaS: lazyLookupSecretvia webhook; OSS:StaticSecret).In every case
secret_registry.get_secret_value(name)returns the value at the moment_ensure_plugins_loaded()runs.Implementation
In
LocalConversation._ensure_plugins_loaded(), beforefetch_plugin_with_resolution(...), expandspec.sourceandspec.refvia the genericexpand_variable_references()helper extracted in #3278:check_env=False— registered conversation secrets only, never hostos.environ. Folding arbitrary host env vars into a URL sent to a remote git host would be a credential-exfiltration vector.support_unbraced=False— braced${VAR}only, so a literal$in a token/password is never mangled.expand_defaults=False— a missing secret leaves the placeholder untouched (no surprising defaults in a URL).ResolvedPluginSourceis still built from the original spec, so the persistedsourcekeeps the${VAR}placeholder, andfrom_plugin_source()continues to redact any inline credentials. Resume re-fetches via the resolved commit SHA, so the raw secret never lands in persisted state or logs.Tests
tests/sdk/conversation/test_local_conversation_plugins.py::TestPluginSourceSecretExpansion:test_source_secret_expanded_before_fetch—${MY_TOKEN}in the source is expanded to the secret value handed to the fetcher; persistedresolved_plugins[0].sourcedoes not contain the raw secret.test_host_env_not_expanded_in_source— a host env var (no matching registered secret) is not expanded; placeholder preserved verbatim.test_ref_secret_expanded_before_fetch—${VAR}inrefis expanded too.test_unknown_var_with_default_left_untouched—${MISSING:-fallback}(no matching secret) is left verbatim, provingexpand_defaults=False(no silent default substitution inside a URL).ruff check/ruff formatclean on the changed files.Evidence:
Scope / non-goals & security notes
ssh://git@...sources. This change only helps when the credential travels inside the URL. With HTTPS, the token is part of the URL itself (https://user:${TOKEN}@host/repo.git), so expanding${TOKEN}is allgitneeds to authenticate. SSH authenticates out-of-band with a private key (via the SSH agent /~/.ssh), and a URL likessh://git@host/repo.githas nowhere to put a secret — so there is no placeholder to expand. Supporting private SSH sources is a separate concern: it requires provisioning an SSH key into the runtime, not expanding a variable. (Related parsing fix for thessh://spelling: is_git_url() does not recognize the ssh:// scheme (plugin source ssh://… fails with 'Unable to parse') #3759 / fix: recognize ssh:// scheme in is_git_url() #3760.)${GITHUB_TOKEN}) into a URL for a different host, sending that token off-host. This is user-authored and intentional (unlike host-env expansion); worth a docs warning but not blocked here. A custom secret named identically to a provider token can shadow the managed one.sourceonly ever carries the${PLACEHOLDER}, so even the Redact plugin/extension source credentials at a single serialization choke point (follow-up to #2196 review, sibling of OpenHands#12959) #3752 spec-serialization leak exposes the placeholder, not the secret value.This PR was created by an AI agent (OpenHands) on behalf of @jpshackelford.
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:88716f9-pythonRun
All tags pushed for this build
About Multi-Architecture Support
88716f9-python) is a multi-arch manifest supporting both amd64 and arm6488716f9-python-amd64) are also available if needed