Repository navigation
Conversation
There was a problem hiding this comment.
Hey @haya-asif
thanks for the PR.
However, there are a couple of things to fix.
Perhaps, take a moment to explore a little the repo and understand how secrets are handled.
Agent-server clones private plugins with https://****@… (first run and resume)
StoredConversation extends StartConversationRequest (plugins: list[PluginSource], request.py:171). The agent-server builds it with request.model_dump(mode="json", context={"expose_secrets": True}) (conversation_service.py:761), persists it via stored.model_dump_json() to meta.json (event_service.py:126), and later clones from stored.plugins (event_service.py:790 → _ensure_plugins_loaded).
The new serializer fires on every one of those dumps and ignores expose_secrets — the exact flag the server passes to keep values intact. Worse, it redacts ${MY_TOKEN} placeholders, which are not secrets (they're expanded from the secret registry at fetch time) and never leaked. So _expand_secret_refs has nothing to expand and git clones https://****@… → 403.
from openhands.sdk.plugin.types import PluginSource
# ${MY_TOKEN} is the recommended way to clone a private plugin: it is NOT a
# secret and is expanded from the secret registry at fetch time.
src = "https://x-token-auth:${MY_TOKEN}@host/repo.git"
# The agent-server builds StoredConversation via this exact call
# (conversation_service.py:761) and later clones from stored.plugins.
print(PluginSource(source=src).model_dump(mode="json", context={"expose_secrets": True})["source"])
# PR #3786 → 'https://****@host/repo.git' (placeholder gone → git clone 403s)
# main → 'https://x-token-auth:${MY_TOKEN}@host/repo.git' (works)ExtensionManager.update() re-clones private extensions with https://****@…
update() reloads the source from .installed.json and re-installs with it (manager.py:312-325): current_info = self.get(name) (which reads disk via InstallationMetadata.load_from_dir) then self.install(source=current_info.source, …).
After the PR, the persisted source is redacted, and extensions don't support ${VAR} expansion, so the re-fetch clones https://****@… and fails.
from pathlib import Path
from openhands.sdk.extensions.installation.info import InstallationInfo
src = "https://oauth2:SUPER_SECRET@github.com/org/ext.git"
info = InstallationInfo(name="ext", source=src, install_path=Path("/tmp/ext"))
# This value is what lands in .installed.json, is reloaded by manager.get(),
# and is passed straight to install() inside update() (manager.py:312-325).
print(info.model_dump()["source"])
# PR #3786 → 'https://****@github.com/org/ext.git' (update() re-clones this → 403)
# main → 'https://oauth2:SUPER_SECRET@github.com/org/ext.git' (works)ResolvedPluginSource.source is now raw in memory → new attribute-access leak
the PR moved redaction from construction-time to the serializer, changing from_plugin_source from source=redact_url_credentials(plugin_source.source) to source=plugin_source.source.
The model now holds the raw URL and only redacts on model_dump. Any direct attribute read therefore leaks, exactly the f-string/attribute bypass the issue warned about.
The PR's audit missed examples/05_skills_and_plugins/02_loading_plugins/main.py:114, which prints resolved.source directly. On main this printed ****.
from openhands.sdk.plugin.types import PluginSource, ResolvedPluginSource
resolved = ResolvedPluginSource.from_plugin_source(
PluginSource(source="https://oauth2:SUPER_SECRET@github.com/org/repo.git"),
resolved_ref="deadbeef",
)
# Any direct attribute read leaks. Real sink:
# examples/05_skills_and_plugins/02_loading_plugins/main.py:114
# print(f" - {resolved.source} @ {resolved.resolved_ref}")
print(resolved.source)
# PR #3786 → 'https://oauth2:SUPER_SECRET@github.com/org/repo.git' (raw → leaked)
# main → 'https://****@github.com/org/repo.git' (redacted, safe)c7aafe9 to
91ddc3d
Compare
|
Would appreciate another look @VascoSch92 — thanks for the detailed review. |
91ddc3d to
6a39cd5
Compare
6a39cd5 to
f6964f0
Compare
VascoSch92
left a comment
There was a problem hiding this comment.
For me is (almost) good.
There are just two things that we should tests to be sure we don't have regression in the future:
ExtensionManager.update()re-clones a credentialed source. This is the exact regression-#2 path and the sole justification for keeping.installed.jsonplaintext. Right now nothing proves the credential actually reaches the clone; a silent break here reintroduces the private-extension failure.- Plugin-load log regression guard, issue #3752 explicitly required it, and a serializer can't catch f-string/attribute leaks. Without it, anyone re-adding
spec.sourceto that log silently reopens the credential leak the PR exists to close.
Otherwise, @jpshackelford and @enyst WDYT?
Inline credentials in a plugin source URL (e.g.
https://oauth2:TOKEN@host/repo.git) leaked into persisted state
(conversation meta.json), the auto `plugins` tag, the remote create
payload, and ordinary model dumps, because PluginSource.source
serialized verbatim.
Add one @field_serializer("source") on PluginSource that masks the
userinfo (https://****@host) on every dump while keeping the raw value
on the attribute for fetch/clone. ${VAR} references are preserved (new
preserve_placeholders flag on redact_url_credentials): they are not
secrets — they are expanded from the secret registry at fetch time,
including server-side — so the recommended private-plugin path keeps
working without raw credentials crossing the wire or landing on disk.
The auto `plugins` tag is masked the same way. ResolvedPluginSource
keeps redacting at construction, so attribute reads stay safe.
Extensions (.installed.json) are intentionally left untouched: they have
no ${VAR} mechanism and the layer has no cipher, so the source must stay
intact at rest for ExtensionManager.update() to re-clone. A regression
guard locks that in; encrypting extension metadata at rest is a separate
follow-up.
Closes #3752. Supersedes #3786 (original work by @haya-asif).
Co-Authored-By: Haya Asif <132150688+haya-asif@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Since the PR was stale I took the liberty of resolving the issue here #3786 Thanks @haya-asif for initiating the effort! |
HUMAN:
This PR fixes a credential leak where plugin/extension source URLs containing embedded tokens (e.g.https://oauth2:TOKEN@host/repo.git) were written verbatim to persisted state (.installed.json, conversation state) and logs. The fix redacts them at the model serialization boundary while keeping the raw value in memory for fetching.
AGENT:
Why
Closes #3752. Credential-bearing plugin/extension
sourceURLs (e.g.https://oauth2:TOKEN@host/repo.git) leaked into persisted state(
.installed.json,meta.json, conversation state, the autopluginstag) andlogs. The first attempt hand-rolled redaction and broke private-plugin clones;
this version routes the
sourcefield through the SDK's existing secretconvention instead.
Summary
serialize_credential_url/validate_credential_urlinutils/pydantic_secrets.py(besideserialize_secret/validate_secret),branching on
resolve_expose_mode: redact (default → mask only theuserinfo,
https://****@host), plaintext (trusted dumps → real URL),encrypted (a
cipher→ Fernet token, decrypted on load).PluginSource.sourceandInstallationInfo.sourcewire these via thestandard
field_validator(mode="before")+field_serializer(when_used="always")pair.ResolvedPluginSourcekeeps redacting at construction (unchanged frommain).RemoteConversationsends plugins withexpose_secrets=True; the autopluginstag is redacted;.installed.jsonis persisted with an exposecontext so
ExtensionManager.update()can re-clone.Maps to the review:
source (and
${VAR}), andmeta.jsonis encrypted at rest, not cleartext.ExtensionManager.update()re-clones from.installed.json(round-trip preserved).ResolvedPluginSource.sourceredacted in memory (attribute read is safe).Issue Number
Closes #3752.
How to Test
New regression tests cover each path: default redaction, expose, the cipher
encrypt-at-rest round-trip on
PluginSourceandInstallationInfo(thecredential becomes a
gAAAAA…token and decrypts back), the remote-sendpayload, the
pluginstag, and the.installed.json→update()re-clone.Full
tests/sdk+tests/agent_servergreen; ruff + pyright clean.Type
Notes
sourceuses a bespokeserialize_credential_urlrather thanSecretStrbecause it is a polymorphic URL (
github:owner/repo, local path,${VAR})needing structure-preserving (userinfo-only) redaction, not a full
**********mask.
.installed.jsonstores the source in plaintext at rest (the extensionslayer has no cipher) — same posture as before this PR, and required for
update().