fix(llm): stop BYOR/custom key leaking into managed-proxy profiles on the read path by juanmichelini · Pull Request #427 · OpenHands/enterprise

juanmichelini · 2026-09-16T19:26:25Z

Problem

SaaS "stale default model" bug (enterprise #421), reported from saas-deploy prod:

Start a conversation with the default managed model — works.
Add a custom ("dummy"/broken) model and set it as the default.
Set the managed default back as active.
Start a new conversation with the default model — it still fails. The dummy model's key is being used for the default (managed) model.

Error code: 401 - {'error': {'message': "LiteLLM Virtual Key expected. Received=dumm****odel, expected to start with 'sk-'.", 'type': 'auth_error', 'param': 'None', 'code': '401'}}

Root cause (read path)

SaasSettingsStore._get_effective_llm_api_key returns the member's effective key, which may be a custom (BYOR) credential (when has_custom_llm_api_key is true). In load(), that key was lifted into agent_settings['llm']['api_key'] before the effective model was finalized. The materialize-Default step (and agent-profile resolution) can then swap the model to a managed openhands/* one while the BYOR key is already attached — so the managed Default model launches carrying dummymodel, and the managed LiteLLM proxy rejects it with the 401 above.

Why the previous version of this PR didn't work

The first attempt gated the effective-key lift on the composed model's managed-ness. But materialize-Default + agent-profile resolution swap the model after the lift, so a BYOR key lifted onto the composed (BYOR) model stayed attached when the model was swapped to managed. The gate ran too early.

Fix (repurposed, narrower read-path guard)

Two cooperating pieces, plus a relaxation to enable the mint:

saas_settings_store.load() — launch-key guard (defense-in-depth). Keep the composed-model lift (it populates the persisted/seed view so settings writes and the legacy-Default seed don't drop a user's key), gated on composed managed-ness == key managed-ness. Add a launch-key guard that runs only in the launch (resolved) view, after the effective model is finalized: if the final model's managed-ness != the key's managed-ness, strip api_key to None. A managed model never launches with a BYOR key; a BYOR model keeps its own key. Persisted/round-trip loads are untouched, so the settings UI and seed keep the user's key.
live_status_app_conversation_service._maybe_refresh_managed_llm_key — launch-time mint. When a managed model has no usable key (None/masked) — exactly what the guard produces after switching back from a broken custom model — mint a fresh managed key via rotate_managed_llm_key(force=True), attach it, and invalidate the user-info cache. Also fixes None api_key being coerced to the truthy string 'None', which previously skipped the mint branch entirely.
saas_settings_store.rotate_managed_llm_key — force param. Relax the has_custom_llm_api_key bail so the mint proceeds even when the member is still in a BYOR state, and clears that state. A truly BYOK org (org._llm_api_key set) is still rejected — those orgs don't use managed keys.

The earlier resolve_profile_llm / is_managed_llm_config layer from this PR's first revision is retained as additional defense-in-depth at the profile-resolution layer; the load() guard above is the primary fix because it operates on the finalized model.

Complementary to #425, which fixes the write path. Together: #425 stops the dummy from being written into the shared slot, and this PR ensures that even if a BYOR key does reach the slot, it can never be handed to a managed profile at launch, and a managed model left keyless by the guard is self-healed by the launch-time mint.

Testing

tests/unit/test_saas_settings_store.py — repro: the dummy BYOR key is no longer attached to the managed Default model (api_key=None); DB-backed rotate_managed_llm_key(force=True) mints a fresh key and clears has_custom_llm_api_key, while no-force still returns BYOK. Suite: 51 passed.
tests/unit/app_server/test_live_status_app_conversation_service.py — 3 new mint tests for _maybe_refresh_managed_llm_key: keyless managed model mints, masked key mints, mint-not-applied returns keyless (best-effort, never raises). Refresh/configure subset: 34 passed.
tests/unit/app_server/test_resolve_profile_llm.py (10p), tests/unit/utils/test_llm_utils.py (29p), test_managed_key_rotation + test_api_keys (126p) — green.
1 pre-existing, env-dependent failure (test_configure_llm_and_mcp_openhands_model_no_base_urls, LITE_LLM_API_URL resolves to the eval proxy in this env) fails identically on main — unrelated to this change.
ruff check on changed files: 0 violations.

This PR was created by an AI agent (OpenHands) on behalf of the repo maintainer.

Enterprise server image for this PR:

ghcr.io/openhands/enterprise-server:sha-7d73c9d

… the read path Enterprise #421: after a user activates a broken custom ("dummy") model and switches back to the managed default, the default model stops working with a LiteLLM 401 ("LiteLLM Virtual Key expected"). Root cause on the read path: the member's effective key -- which may be a custom (BYOR) credential -- was attached to *any* profile, including managed ones routed at the LiteLLM proxy, which rejects third-party keys. Harden the read path so a non-managed key can never reach a managed profile: - resolve_profile_llm: add fallback_is_managed_key (default True to preserve behaviour). The managed-proxy branch (override + keyless fill) now runs only when the fallback is a managed key. A BYOR fallback no longer poisons a managed profile; the managed profile keeps its own (possibly stale) key and runtime rotation refresh heals it. - utils/llm.is_managed_llm_config: app-server-side classifier mirroring storage.managed_llm_key_config_from_model so the three resolve_profile_llm call sites (users_v1, app_conversation_router, live_status service) classify managed configs identically without importing storage. - saas_settings_store.load: compute effective_key_is_managed from has_custom_llm_api_key and gate the composed-path effective-key lift on it (also via _resolve_active_agent_profile). A managed active model is no longer handed a BYOR key sitting in the shared _llm_api_key slot. Complementary to #425, which fixes the write path (force-rotating a fresh managed key on profile switch-back so the dummy does not persist in the slot). This change contains the blast radius if a BYOR key ever reaches the shared slot (rotation failure, legacy store() paths, or a BYOR active default poisoning the exposed profile set). Tests: - test_resolve_profile_llm: 3 regression tests (BYOR fallback no longer overrides/fills a managed profile; managed fallback still overrides). - test_llm_utils: TestIsManagedLlmConfig covers is_managed_llm_config. - test_saas_settings_store: load() regressions -- a member's dummy BYOR key is not attached to a managed active model; a BYOR active model still receives its own key. resolve_profile_llm 10p, llm_utils 29p, saas_settings_store 49p, agent_profiles 48p, org_profiles + resolve_provider 51p -- all green. No new ruff violations (7 pre-existing I001 on main unchanged). Co-authored-by: openhands <openhands@all-hands.dev>

github-actions · 2026-09-16T19:36:27Z

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
openhands/app_server/app_conversation
app_conversation_router.py
live_status_app_conversation_service.py					1105-1106, 1415, 1459, 1477
openhands/app_server/settings
llm_profiles.py
openhands/app_server/utils
llm.py
server/routes
users_v1.py
storage
saas_settings_store.py					1148-1149, 1214
Project Total

_{This report was generated by python-coverage-comment-action}

Enterprise #421: the previous read-path guard gated the effective-key lift on the COMPOSED model, but materialize-Default + agent-profile resolution swap the model AFTER the lift. A BYOR key lifted onto the composed (BYOR) model stayed attached when the model was swapped to managed, so the managed Default still launched with the dummy BYOR key -> LiteLLM 401 ("LiteLLM Virtual Key expected"). Replace the composed-model gate with two cooperating pieces: 1. storage/saas_settings_store.load: keep the composed-model lift (it populates the persisted/seed view so settings writes/seed don't drop a user's key), gated on composed managed-ness == key managed-ness. Add a launch-key guard that runs ONLY in the launch (resolved) view, AFTER the effective model is finalized: if the FINAL model's managed-ness != the key's managed-ness, strip api_key to None. A managed model never launc key's managed-ness, strip api_key to None. A man Persisted/round-trip loads are untouched, so the settings UI and seed keep the user's key. 2. live_status_app_conversation_service._maybe_refresh_managed_llm_key: when a managed model has no usable key (None/masked) -- exactly what the guard produces after switching back from a broken custom model -- mint a fresh managed key via rotate_managed_llm_key(force=True), attach it, and invalidate the user-info cache. Also fix None api_key being coerced to the truthy string 'None' (which skipped the mint branch entirely). 3. saas_settings_store.rotate_managed_llm_key: add force param to relax the has_custom_llm_api_key bail so the mint proceeds even when the member is still in a BYOR state, and clears it. A truly BYOK org (org._llm_api_key set) is still rejected. Tests: - test_saas_settings_store: repro (dummy BYOR key no longer on managed Default -> api_key=None); DB-backed rotate(force=True) mints + clears has_custom, no-force still returns BYOK. - test_live_status_app_conversation_service: 3 mint tests (keyless, masked, mint-not-applied) for _maybe_refresh_managed_llm_key. saas_settings_store 51p, resolve_profile_llm 10p, llm_utils 29p, managed_key_rotation + api_keys 126p, maybe_refresh/configure 34p -- green. 1 pre-existing env-dependent failure (test_configure_llm_and_mcp_openhands _model_no_base_urls, LITE_LLM_API_URL=eval proxy) fails identically on main. Co-authored-by: openhands <openhands@all-hands.dev>

The previous guard only blocked the leak on the resolve-requested launch view (conversation start at live_status_app_conversation_service.py:484). The actual key supplied to the sandbox comes from _seed_llm_profiles_to_sandbox, which fetches settings via a PLAIN get_user_info() (no resolve_agent_profile) — so the launch-key guard in load() is skipped on that path: - the composed-key lift attaches the member's BYOR/dummy key to agent_settings.llm; - materialize_default_llm_payload swaps the model to the managed "Default" (model+base_url only), leaving the BYOR key attached; - _maybe_refresh_managed_llm_key saw has_usable_key=True (dummy key is non-empty) and skipped the mint branch; - get_current_managed_llm_key() returns None in that same state (it bails when has_custom_llm_api_key=True), and the None/mismatch early-returns handed the BYOR key straight through; - the dummy-keyed managed model hit the LiteLLM proxy -> 401 "LiteLLM Virtual Key expected. Received=dumm****odel". Fix: make _maybe_refresh_managed_llm_key the single chokepoint. For a managed model, mint a fresh managed key (rotate_managed_llm_key(force=True), which also clears the BYOR state) whenever the carried key is NOT the org's current managed virtual key — no usable key, no current managed key, or a mismatch (wrong-type/stale). Only when the carried key equals the current managed key does it verify-and-rotate-when-stale as before. This catches the leak regardless of which load view (plain or resolved) produced the LLM. Tests: - test_maybe_refresh_managed_llm_key_mints_on_key_mismatch (renamed from skips_key_mismatch): dummy BYOR key on a managed model -> mint. - test_maybe_refresh_managed_llm_key_mints_when_no_current_managed_key: the exact leak state (managed model + BYOR key + get_current_managed_llm_key returns None) -> mint. - mints_when_keyless/masked/mint_not_applied: updated for the new get_current_managed_llm_key call ordering. Co-authored-by: openhands <openhands@all-hands.dev>

The deployed 551f79b still leaked: preview logs showed the mint firing (mint_managed_key, reason=no_current_managed_key) but rotate_managed_llm_key returning status=byok, has_new_key=false -> mint_not_applied, so a masked key flowed to the proxy (401 "Received=****"). Root cause is one layer above the mint. _get_effective_llm_api_key returned the member's custom (dummy) key whenever has_custom_llm_api_key=True, regardless of the active model. has_custom is sticky: activating a broken BYOR model sets it, and switching the active model back to managed does NOT clear it. So a managed active model (openhands/deepseek-v4-flash) still got the member's stale dummy key as the "effective" key, which the composed lift attached to the managed model -> leak. And this org has an org-level managed key, so the per-member mint correctly bails BYOK (orgs with org-level keys don't use member managed keys) -> no fresh key -> masked -> 401. Fix: make key resolution model-aware. load() computes active_is_managed from the composed (active settings) model and passes it to _get_effective_llm_api_key. A managed active model now takes the org's managed key (or the member's own managed key when has_custom is False), NEVER the stale custom key. A BYOR/custom active model still uses the member's custom key (unchanged). effective_key_is_managed tracks active_is_managed so the composed lift only stamps a managed key onto a managed model. This resolves org-key orgs (the org sk- key is used, mint stays a no-op BYOK) and leaves the member-managed path (no org key) to the launch-time mint. Tests: - test_managed_active_model_uses_org_key_not_stale_custom_key: the leak repro (managed model + sticky has_custom + dummy key) -> org key, not dummy. - test_managed_active_model_returns_none_when_no_managed_key: no org key + BYOR state -> None (mint handles member-managed orgs). - test_byor_active_model_uses_member_custom_key: BYOR model keeps custom key. - existing _get_effective_llm_api_key tests updated for the new kwarg; test_load_does_not_attach_custom_byor_key_to_managed_active_model still passes (no org key in that fixture -> None). Co-authored-by: openhands <openhands@all-hands.dev>

aivong-openhands · 2026-09-21T20:51:26Z

Replicated VM verification — PASS for the intended (switch-back) scenario ✅

Tested on a Replicated embedded-cluster VM (SaaS mode). Control = the baseline
release (enterprise-server:1.61.0); treatment = this PR (sha-7d73c9d),
forward-migrated from a clean baseline (no schema issues; DB head unchanged).

Scenario tested (this PR's target): a member left with a custom/BYOR key in
the slot (has_custom=True) while the active default is a managed model —
the state you reach after setting a broken custom model as default and switching
back to the managed default.

Deterministic (store) check — the read-path guard works:

Launch view resolves the managed model with api_key = None — the guard
strips the BYOR key so a managed model never launches carrying it.

Live UI end-to-end — the launch-time mint reaches the runtime:
A real conversation on the managed default (broken state pre-set) succeeded on
the first try. App-server logs, in order:

managed_llm_key_refresh:mint_managed_key   reason=no_usable_key  has_key=false
saas_settings_store:rotate_managed_llm_key:rotated
managed_llm_key_refresh:minted
app_conversation_start:building_start_request   (mint happened BEFORE this)

LiteLLM saw no dummy**** 401 — the stale BYOR key never reached the proxy.
The member slot was rotated to a fresh managed key and has_custom cleared.
One caveat (minor): a freshly minted managed key produced a single transient
POST /chat/completions 401 that immediately recovered to 200 OK on retry —
looks like the new key isn't registered on the proxy for a brief window (also
showed once as a key … not found in db earlier). Not user-visible here, but
may be worth a short verify/backoff after mint.

Scope note (vs #437): this PR fixes the member-level BYOR-on-managed case
end-to-end, including the in-flight path to the sandbox runtime (the first
conversation works). It does not address the other #421 sub-state — an
org-level stale key (org._llm_api_key set, has_custom=False, managed
default): there the guard treats the model as managed and re-attaches the org
key (no strip, mint doesn't fire), so that cohort stays broken on this PR. That
org-level cohort is what #437 targets (and #437 heals the DB, though its first
conversation still 401s once). So #427 and #437 look complementary rather than
redundant — worth deciding whether both sub-states need coverage before merge.

This comment was created by an AI agent (OpenHands) on behalf of the maintainer.

github-actions Bot added the type: fix A bug fix label Sep 16, 2026

juanmichelini force-pushed the fix/read-path-managed-key-isolation branch from f1a8cce to 9792d42 Compare September 17, 2026 03:37

juanmichelini force-pushed the fix/read-path-managed-key-isolation branch from 9792d42 to 92881a8 Compare September 17, 2026 13:05

openhands-agent added 2 commits September 17, 2026 10:13

lilagrc mentioned this pull request Sep 17, 2026

fix: grant MANAGE_AUTOMATIONS permission to org members #436

Closed

7 tasks

aivong-openhands mentioned this pull request Sep 18, 2026

Managed-key rotation race on conversation start → intermittent token_not_found_in_db 401 (enterprise-server 1.60.0 regression) #439

Closed

Navigation Menu

fix(llm): stop BYOR/custom key leaking into managed-proxy profiles on the read path - #427

fix(llm): stop BYOR/custom key leaking into managed-proxy profiles on the read path#427
juanmichelini wants to merge 4 commits into
mainfrom
fix/read-path-managed-key-isolation

juanmichelini commented Sep 16, 2026 •

edited by github-actions Bot

Loading

Uh oh!

github-actions Bot commented Sep 16, 2026 •

edited

Loading

Uh oh!

aivong-openhands commented Sep 21, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

juanmichelini commented Sep 16, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Root cause (read path)

Why the previous version of this PR didn't work

Fix (repurposed, narrower read-path guard)

Testing

Uh oh!

github-actions Bot commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage report

Uh oh!

aivong-openhands commented Sep 21, 2026

Replicated VM verification — PASS for the intended (switch-back) scenario ✅

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

juanmichelini commented Sep 16, 2026 •

edited by github-actions Bot

Loading

github-actions Bot commented Sep 16, 2026 •

edited

Loading