fix: redact credentials from URLs in logs, exceptions, and persisted state#2196
fix: redact credentials from URLs in logs, exceptions, and persisted state#2196jpshackelford wants to merge 2 commits intomainfrom
Conversation
…state This addresses security issue #2152 by preventing credential leakage: - Add redact_url_credentials() utility function to sanitize URLs with embedded credentials (e.g., https://oauth2:token@host/path becomes https://****@host/path) - Update git/utils.py:run_git_command() to redact credentials from: - Error log messages - Debug log messages - GitCommandError exception messages and command fields - Update git/cached_repo.py to redact credentials in clone log messages - Update plugin/fetch.py to redact credentials in: - PluginFetchError exception messages - Warning log messages - Update plugin/types.py:ResolvedPluginSource.from_plugin_source() to redact credentials before persistence (credentials only needed at fetch time) - Export redact_url_credentials from openhands.sdk public API for cross-repo usage (related: OpenHands/OpenHands#12959) - Add comprehensive tests for credential redaction Co-authored-by: openhands <openhands@all-hands.dev>
API breakage checks (Griffe)Result: Passed |
Coverage Report •
|
||||||||||||||||||||||||||||||||||||||||||||||||||
Agent server REST API breakage checks (OpenAPI)Result: Failed Log excerpt (first 1000 characters) |
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - Clean security fix with pragmatic design.
Key Insight: Simple regex-based redaction applied consistently across logs, exceptions, and persisted state. The decision to redact credentials in ResolvedPluginSource is sound design - credentials are only needed at fetch time, and resolved plugins reference local cached paths for subsequent access.
What Works:
- Focused
redact_url_credentials()function does one thing well - Comprehensive test coverage testing real behavior (not mocks) ✓
- Handles edge cases correctly (ports, special chars, SSH URLs unchanged)
- Applied consistently: logs, error messages, exceptions, persistence
Design Decision (Good):
Redacting in ResolvedPluginSource.from_plugin_source() prevents credential leakage in conversation state without breaking functionality - resolved plugins use cached repo_path, so re-authentication not needed.
✅ Worth merging - Solves real security problem (#2152) with minimal complexity. No agent behavior changes, just safer logging and persistence.
Summary
Fixes #2152
This PR addresses a security issue where credentials embedded in plugin source URLs (e.g.,
https://oauth2:SECRET_TOKEN@gitlab.com/org/private-repo) were being exposed in:ResolvedPluginSourceChanges
redact_url_credentials(url)ingit/utils.pythat replaces credentials in URLs with****(e.g.,https://****@gitlab.com/org/repo)run_git_command(): Redacts credentials from error logs, debug logs, andGitCommandErrorexception messages/command fieldscached_repo.py: Redacts credentials in clone log messagesplugin/fetch.py: Redacts credentials inPluginFetchErrorexceptions and warning logsplugin/types.py:ResolvedPluginSource.from_plugin_source()now redacts credentials before persistence (credentials are only needed at fetch time)redact_url_credentialsis exported fromopenhands.sdkfor cross-repo usage (related to Security: Credentials in plugin source URLs exposed via API responses OpenHands#12959)Example
Before:
After:
Checklist
@jpshackelford can click here to continue refining the PR
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-nodejs22golang: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:e05f9de-pythonRun
All tags pushed for this build
About Multi-Architecture Support
e05f9de-python) is a multi-arch manifest supporting both amd64 and arm64e05f9de-python-amd64) are also available if needed