fix: resolve provider connection credentials in GET /api/profiles/{name} - #4584
Open
all-hands-bot wants to merge 1 commit into
Open
fix: resolve provider connection credentials in GET /api/profiles/{name}#4584all-hands-bot wants to merge 1 commit into
all-hands-bot wants to merge 1 commit into
Conversation
PR #4492 introduced provider-connection-backed LLM profiles, but the GET /api/profiles/{name} endpoint hardcoded resolve_provider=False. This meant RemoteWorkspace.get_llm() — which calls that endpoint with X-Expose-Secrets: plaintext — received api_key=None and base_url=None for any profile linked to a provider connection, making the resulting LLM unable to authenticate. Fix: add a resolve_provider query parameter (default False, preserving display-mode behavior) and have _fetch_llm_profile_config() pass resolve_provider=true so credentials are resolved at read-at-use time. Co-authored-by: openhands <openhands@all-hands.dev>
Contributor
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
Contributor
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Contributor
Coverage Report •
|
||||||||||||||||||||||||||||||
Collaborator
Author
|
👋 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
HUMAN:
AGENT:
Why
Source PR reviewed: #4492 ("Add read-at-use LLM provider connections")
Risk lenses applied: #1 (Boundary contracts and alternate valid inputs), #4 (Multiple construction and configuration paths), #8 (Credentials, redaction, and sensitive persistence boundaries)
Deterministic reproduction and impact:
PR #4492 introduced provider-connection-backed LLM profiles where credentials are stored in a
ProviderConnectionStoreand referenced byprovider_connection_idfrom the LLM profile. However,GET /api/profiles/{name}hardcodedresolve_provider=False:This means the endpoint never resolves provider connection credentials, even when the caller sends
X-Expose-Secrets: plaintext. The returnedapi_keyandbase_urlare alwaysNonefor provider-connection-backed profiles.RemoteWorkspace.get_llm()calls this endpoint via_fetch_llm_profile_config()to build a working LLM. Because the endpoint returnsapi_key=None, the resultingLLMobject has no API key and cannot authenticate — every LLM call fails withLLMAuthenticationError.This affects any user who:
POST /api/llm/provider-connectionsprovider_connection_idworkspace.get_llm()(or any agent-server flow that resolves the active profile)Root cause: The endpoint was designed for display/listing (where
resolve_provider=Falseavoids injecting credentials and avoids failing on dangling references). But it is also the same endpoint used byget_llm()for credential resolution. The display-mode default was applied universally, with no opt-in for credential resolution.Proof that the regression test fails on unmodified main:
Output on
main(before fix):The profile is linked to a provider connection with
api_key="sk-provider-resolve-test", butGET /api/profiles/conn-profilereturnsapi_key=Noneeven withX-Expose-Secrets: plaintext.Why the original tests did not catch this:
The existing
RemoteWorkspace.get_llm()tests (e.g.,test_get_llm_with_profile_name) mock the HTTP response and fabricate theapi_keyfield in the mock — they never exercise the real server endpoint that hardcodesresolve_provider=False. The router-level tests (test_settings_active_profile_resolves_provider_connection) test theGET /api/settingspath (which goes throughsettings_router, notprofiles_router) and that path does resolve the provider. No test verified thatGET /api/profiles/{name}itself returns resolved credentials when asked.Summary
resolve_provider: bool = Falsequery parameter toGET /api/profiles/{name}inprofiles_router.py; whentrue, the linked provider connection'sapi_keyandbase_urlare resolved into the returned config_fetch_llm_profile_config()inopenhands/sdk/workspace/remote/base.pyto passparams={"resolve_provider": "true"}soget_llm()receives resolved credentialstest_get_profile_resolve_provider_returns_connection_credentialsverifying both display-mode (api_key=None) and resolved-mode (api_keyfrom provider connection)test_get_llm_with_profile_nameto assertresolve_provider=trueis passed in the request paramsREST API contract changes
Compared with base OpenAPI
9421149592dafor public/api/**paths.Issue Number
N/A — discovered during the weekly regression hunt.
How to Test
Before/after test commands and results:
$ uv run pytest tests/sdk/workspace/remote/test_remote_workspace.py -k "get_llm" -x 8 passedEnd-to-end reproduction:
The regression test creates a provider connection with
api_key="sk-provider-resolve-test"andbase_url="https://api.openai.com/v1", links a profile to it, then verifies:GET /api/profiles/conn-profile(default) returnsapi_key=None(display mode — correct)GET /api/profiles/conn-profile?resolve_provider=truewithX-Expose-Secrets: plaintextreturnsapi_key="sk-provider-resolve-test"andbase_url="https://api.openai.com/v1"(resolved mode — was broken, now fixed)Video/Screenshots
N/A — text-based reproduction with captured test output above.
Design Doc
N/A — small, focused fix.
Type
Notes
@juanmichelini(author of PR Add read-at-use LLM provider connections #4492) and@hieptl(approver of PR Add read-at-use LLM provider connections #4492)resolve_providerquery parameter defaults tofalse, preserving the existing display-mode behavior for all current callers (UI,GET /api/settingsviasettings_router, etc.)X-Expose-Secrets: plaintextheader still controls whether secrets are exposed in the response;resolve_provider=trueonly controls whether the provider connection's credentials are resolved into the config (they are still subject to the secret exposure mode)provider_connection_idwithresolve_provider=truewill raiseProviderConnectionNotFound→ HTTP 422, which is the expected behavior forget_llm()(it should fail loudly rather than silently returnapi_key=None)🐳 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-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:b7da2cd-pythonRun
All tags pushed for this build
About Multi-Architecture Support
b7da2cd-python) is a multi-arch manifest supporting both amd64 and arm64b7da2cd-python-amd64) are also available if needed