From 0d2e71464bb9fe115747eb97ca9f879790ec0b63 Mon Sep 17 00:00:00 2001 From: openhands Date: Tue, 16 Jun 2026 15:20:40 +0000 Subject: [PATCH 1/2] fix: expand per-conversation secrets in plugin source/ref before fetch 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 --- .../conversation/impl/local_conversation.py | 44 ++++++- .../test_local_conversation_plugins.py | 115 ++++++++++++++++++ 2 files changed, 155 insertions(+), 4 deletions(-) diff --git a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py index 52996ccacc..275d4f1806 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py @@ -66,7 +66,10 @@ ConfirmationPolicyBase, ) from openhands.sdk.skills import load_available_skills, merge_skills_by_name -from openhands.sdk.skills.utils import expand_mcp_variables +from openhands.sdk.skills.utils import ( + expand_mcp_variables, + expand_variable_references, +) from openhands.sdk.subagent import ( AgentDefinition, register_file_agents, @@ -643,14 +646,47 @@ def _ensure_plugins_loaded(self) -> None: self._resolved_plugins = [] for spec in self._plugin_specs: + # Expand ${VAR} placeholders in the source/ref using + # per-conversation secrets, so private plugins can be cloned + # with a token supplied via the secrets API, e.g.: + # "https://x-token-auth:${MY_TOKEN}@host/org/repo.git" + # + # SECURITY: secrets only (check_env=False) -- never fold host + # environment variables into a URL sent to a remote git host. + # Braced-only (support_unbraced=False) avoids mangling a literal + # "$" that may legitimately appear in a token/password. + get_secret = self._state.secret_registry.get_secret_value + fetch_source = expand_variable_references( + spec.source, + get_secret=get_secret, + check_env=False, + support_unbraced=False, + expand_defaults=False, + ) + fetch_ref = ( + expand_variable_references( + spec.ref, + get_secret=get_secret, + check_env=False, + support_unbraced=False, + expand_defaults=False, + ) + if spec.ref + else spec.ref + ) + # Fetch plugin and get resolved commit SHA path, resolved_ref = fetch_plugin_with_resolution( - source=spec.source, - ref=spec.ref, + source=fetch_source, + ref=fetch_ref, repo_path=spec.repo_path, ) - # Store resolved ref for persistence + # Store resolved ref for persistence. Build this from the + # ORIGINAL spec (not the expanded values) so the persisted + # record keeps the ${VAR} placeholder rather than the raw + # secret. from_plugin_source() additionally redacts any inline + # credentials. Resume re-fetches via the resolved commit SHA. resolved = ResolvedPluginSource.from_plugin_source(spec, resolved_ref) self._resolved_plugins.append(resolved) diff --git a/tests/sdk/conversation/test_local_conversation_plugins.py b/tests/sdk/conversation/test_local_conversation_plugins.py index cfa7b066de..65ee71c8ca 100644 --- a/tests/sdk/conversation/test_local_conversation_plugins.py +++ b/tests/sdk/conversation/test_local_conversation_plugins.py @@ -601,3 +601,118 @@ def mock_create_mcp_tools(config, timeout): assert header == "default-header-value" conversation.close() + + +class TestPluginSourceSecretExpansion: + """Secrets in plugin ``source``/``ref`` are expanded before fetch. + + This enables cloning private plugin repositories with a token supplied via + the per-conversation secrets API, e.g. a ``source`` of + ``https://x-token-auth:${MY_TOKEN}@host/org/repo.git``. + """ + + def _make_conversation( + self, tmp_path: Path, basic_agent, plugin_source: str, ref: str | None = None + ): + plugin_dir = create_test_plugin( + tmp_path / "plugin", + name="private-plugin", + skills=[{"name": "private-skill", "content": "Private content"}], + ) + workspace = tmp_path / "workspace" + workspace.mkdir() + conversation = LocalConversation( + agent=basic_agent, + workspace=workspace, + plugins=[PluginSource(source=plugin_source, ref=ref)], + visualizer=None, + ) + return conversation, plugin_dir + + def test_source_secret_expanded_before_fetch(self, tmp_path: Path, basic_agent): + """A ${VAR} in the source is replaced with the secret value before clone.""" + source = "https://x-token-auth:${MY_TOKEN}@host.example.com/org/repo.git" + conversation, plugin_dir = self._make_conversation( + tmp_path, basic_agent, source + ) + conversation.update_secrets({"MY_TOKEN": "s3cr3t-value"}) + + captured: dict[str, str | None] = {} + + def fake_fetch(source, ref=None, repo_path=None, **kwargs): + captured["source"] = source + captured["ref"] = ref + return plugin_dir, "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef" + + with patch( + "openhands.sdk.conversation.impl.local_conversation." + "fetch_plugin_with_resolution", + side_effect=fake_fetch, + ): + conversation._ensure_plugins_loaded() + + # The secret was expanded in the URL handed to the fetcher. + assert captured["source"] == ( + "https://x-token-auth:s3cr3t-value@host.example.com/org/repo.git" + ) + + # Persisted state must NOT contain the raw secret value. + assert conversation.resolved_plugins is not None + assert "s3cr3t-value" not in conversation.resolved_plugins[0].source + + conversation.close() + + def test_host_env_not_expanded_in_source( + self, tmp_path: Path, basic_agent, monkeypatch + ): + """Host environment variables must NOT be folded into the source URL.""" + monkeypatch.setenv("HOST_ONLY_VAR", "host-value") + source = "https://x-token-auth:${HOST_ONLY_VAR}@host.example.com/org/repo.git" + conversation, plugin_dir = self._make_conversation( + tmp_path, basic_agent, source + ) + # Deliberately register NO secret named HOST_ONLY_VAR. + + captured: dict[str, str | None] = {} + + def fake_fetch(source, ref=None, repo_path=None, **kwargs): + captured["source"] = source + return plugin_dir, "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef" + + with patch( + "openhands.sdk.conversation.impl.local_conversation." + "fetch_plugin_with_resolution", + side_effect=fake_fetch, + ): + conversation._ensure_plugins_loaded() + + # Placeholder preserved verbatim - host env was not used. + assert captured["source"] == source + assert "host-value" not in (captured["source"] or "") + + conversation.close() + + def test_ref_secret_expanded_before_fetch(self, tmp_path: Path, basic_agent): + """A ${VAR} in the ref is also expanded from secrets.""" + source = str(tmp_path / "plugin") + conversation, plugin_dir = self._make_conversation( + tmp_path, basic_agent, source, ref="${MY_REF}" + ) + conversation.update_secrets({"MY_REF": "v1.2.3"}) + + captured: dict[str, str | None] = {} + + def fake_fetch(source, ref=None, repo_path=None, **kwargs): + captured["ref"] = ref + return plugin_dir, "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef" + + with patch( + "openhands.sdk.conversation.impl.local_conversation." + "fetch_plugin_with_resolution", + side_effect=fake_fetch, + ): + conversation._ensure_plugins_loaded() + + assert captured["ref"] == "v1.2.3" + + conversation.close() From 667f43339634a5a238d3e2fb620ad12195cad439 Mon Sep 17 00:00:00 2001 From: openhands Date: Tue, 16 Jun 2026 20:51:27 +0000 Subject: [PATCH 2/2] refactor: DRY plugin secret expansion + add ${VAR:-default} test 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 --- .../conversation/impl/local_conversation.py | 43 ++++++++----------- .../test_local_conversation_plugins.py | 33 ++++++++++++++ 2 files changed, 52 insertions(+), 24 deletions(-) diff --git a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py index 275d4f1806..71bad5510a 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py @@ -645,35 +645,30 @@ def _ensure_plugins_loaded(self) -> None: logger.info(f"Loading {len(self._plugin_specs)} plugin(s)...") self._resolved_plugins = [] - for spec in self._plugin_specs: - # Expand ${VAR} placeholders in the source/ref using - # per-conversation secrets, so private plugins can be cloned - # with a token supplied via the secrets API, e.g.: - # "https://x-token-auth:${MY_TOKEN}@host/org/repo.git" - # - # SECURITY: secrets only (check_env=False) -- never fold host - # environment variables into a URL sent to a remote git host. - # Braced-only (support_unbraced=False) avoids mangling a literal - # "$" that may legitimately appear in a token/password. - get_secret = self._state.secret_registry.get_secret_value - fetch_source = expand_variable_references( - spec.source, + # Expand ${VAR} placeholders in the source/ref using per-conversation + # secrets, so private plugins can be cloned with a token supplied via + # the secrets API, e.g. "https://x-token-auth:${MY_TOKEN}@host/repo.git". + # + # SECURITY: secrets only (check_env=False) -- never fold host + # environment variables into a URL sent to a remote git host. + # Braced-only (support_unbraced=False) avoids mangling a literal "$" + # that may legitimately appear in a token/password. expand_defaults + # is False so an unknown ${VAR} is left verbatim rather than silently + # defaulted inside a URL. + get_secret = self._state.secret_registry.get_secret_value + + def _expand_secret_refs(value: str) -> str: + return expand_variable_references( + value, get_secret=get_secret, check_env=False, support_unbraced=False, expand_defaults=False, ) - fetch_ref = ( - expand_variable_references( - spec.ref, - get_secret=get_secret, - check_env=False, - support_unbraced=False, - expand_defaults=False, - ) - if spec.ref - else spec.ref - ) + + for spec in self._plugin_specs: + fetch_source = _expand_secret_refs(spec.source) + fetch_ref = _expand_secret_refs(spec.ref) if spec.ref else spec.ref # Fetch plugin and get resolved commit SHA path, resolved_ref = fetch_plugin_with_resolution( diff --git a/tests/sdk/conversation/test_local_conversation_plugins.py b/tests/sdk/conversation/test_local_conversation_plugins.py index 65ee71c8ca..5c2be91d89 100644 --- a/tests/sdk/conversation/test_local_conversation_plugins.py +++ b/tests/sdk/conversation/test_local_conversation_plugins.py @@ -692,6 +692,39 @@ def fake_fetch(source, ref=None, repo_path=None, **kwargs): conversation.close() + def test_unknown_var_with_default_left_untouched(self, tmp_path: Path, basic_agent): + """`${MISSING:-default}` is preserved verbatim (expand_defaults=False). + + An unresolved variable in a URL must not be silently replaced with its + default -- the placeholder is left intact so the failure is visible + rather than producing a wrong-but-plausible URL. + """ + source = "https://x-token-auth:${MISSING:-fallback}@host.example.com/o/r.git" + conversation, plugin_dir = self._make_conversation( + tmp_path, basic_agent, source + ) + # No secret named MISSING registered. + + captured: dict[str, str | None] = {} + + def fake_fetch(source, ref=None, repo_path=None, **kwargs): + captured["source"] = source + return plugin_dir, "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef" + + with patch( + "openhands.sdk.conversation.impl.local_conversation." + "fetch_plugin_with_resolution", + side_effect=fake_fetch, + ): + conversation._ensure_plugins_loaded() + + # Placeholder preserved verbatim: the default is NOT substituted in, + # the whole ${MISSING:-fallback} token is left intact. + assert captured["source"] == source + assert "${MISSING:-fallback}" in (captured["source"] or "") + + conversation.close() + def test_ref_secret_expanded_before_fetch(self, tmp_path: Path, basic_agent): """A ${VAR} in the ref is also expanded from secrets.""" source = str(tmp_path / "plugin")