fix: guard OAuth account removal

2026-06-15 09:21:36 +00:00 · 2026-06-13 21:32:05 -07:00 · 2026-06-13 21:32:05 -07:00 · 1b16c48170
commit 1b16c48170
parent e986e3fc68
11 changed files with 175 additions and 30 deletions
--- a/apps/desktop/src/app/settings/providers-settings.test.tsx
+++ b/apps/desktop/src/app/settings/providers-settings.test.tsx
@ -21,16 +21,18 @@ vi.mock('@/store/onboarding', () => ({
  startManualProviderOAuth: (providerId: string) => startManualProviderOAuth(providerId)
 }))

-function provider(id: string, loggedIn: boolean): OAuthProvider {
+function provider(id: string, loggedIn: boolean, patch: Partial<OAuthProvider> = {}): OAuthProvider {
  return {
    cli_command: `hermes auth add ${id}`,
+    disconnectable: true,
    docs_url: '',
    flow: 'device_code',
    id,
    name: id === 'nous' ? 'Nous Portal' : 'MiniMax',
    status: {
      logged_in: loggedIn
-    }
+    },
+    ...patch
  }
 }

@ -75,4 +77,24 @@ describe('ProvidersSettings', () => {
    expect(startManualProviderOAuth).toHaveBeenCalledWith('nous')
    expect(disconnectOAuthProvider).not.toHaveBeenCalled()
  })
+
+  it('does not offer removal for externally managed providers', async () => {
+    listOAuthProviders.mockResolvedValue({
+      providers: [
+        provider('qwen-oauth', true, {
+          cli_command: 'hermes auth add qwen-oauth',
+          disconnect_hint: 'Use `hermes auth add qwen-oauth` or that provider\'s CLI to remove it.',
+          disconnectable: false,
+          flow: 'external',
+          name: 'Qwen (via Qwen CLI)'
+        })
+      ]
+    })
+
+    await renderProvidersSettings()
+
+    expect(await screen.findByText('Qwen Code')).toBeTruthy()
+    expect(screen.queryByRole('button', { name: 'Remove Qwen Code' })).toBeNull()
+    expect(screen.getByText(/managed outside Hermes/)).toBeTruthy()
+  })
 })
--- a/apps/desktop/src/app/settings/providers-settings.tsx
+++ b/apps/desktop/src/app/settings/providers-settings.tsx
@ -6,6 +6,7 @@ import {
  FeaturedProviderRow,
  KeyProviderRow,
  ProviderRow,
+  providerTitle,
  sortProviders
 } from '@/components/desktop-onboarding-overlay'
 import { Button } from '@/components/ui/button'
@ -187,8 +188,13 @@ function ConnectedProviderRow({
  provider: OAuthProvider
 }) {
  const { t } = useI18n()
-  const title = provider.name
+  const title = providerTitle(provider)
  const Trail = provider.flow === 'external' ? Terminal : ChevronRight
+  const canDisconnect = provider.disconnectable ?? provider.flow !== 'external'
+
+  const disconnectHint = provider.flow === 'external'
+    ? t.settings.providers.removeExternal(title, provider.cli_command)
+    : t.settings.providers.removeKeyManaged(title)

  return (
    <div className="group grid grid-cols-[minmax(0,1fr)_auto] items-center gap-1 rounded-[6px] transition-colors hover:bg-(--ui-control-hover-background)">
@ -201,20 +207,27 @@ function ConnectedProviderRow({
          </span>
        </div>
        <p className="mt-1 text-xs leading-5 text-muted-foreground">{t.onboarding.flowSubtitles[provider.flow]}</p>
+        {!canDisconnect && (
+          <p className="mt-0.5 truncate text-[0.68rem] leading-5 text-muted-foreground/70">
+            {disconnectHint}
+          </p>
+        )}
      </button>
      <div className="flex items-center gap-1 pr-2">
        <Trail className="size-4 text-muted-foreground transition group-hover:text-foreground" />
-        <Button
-          aria-label={`${t.common.remove} ${title}`}
-          disabled={disconnecting}
-          onClick={() => onDisconnect(provider)}
-          size="icon-xs"
-          title={`${t.common.remove} ${title}`}
-          type="button"
-          variant="ghost"
-        >
-          {disconnecting ? <Loader2 className="size-3 animate-spin" /> : <Trash2 className="size-3" />}
-        </Button>
+        {canDisconnect && (
+          <Button
+            aria-label={`${t.common.remove} ${title}`}
+            disabled={disconnecting}
+            onClick={() => onDisconnect(provider)}
+            size="icon-xs"
+            title={`${t.common.remove} ${title}`}
+            type="button"
+            variant="ghost"
+          >
+            {disconnecting ? <Loader2 className="size-3 animate-spin" /> : <Trash2 className="size-3" />}
+          </Button>
+        )}
      </div>
    </div>
  )
@ -270,9 +283,9 @@ export function ProvidersSettings({ onViewChange, view }: ProvidersSettingsProps
  }, [onboardingActive])

  async function handleDisconnect(provider: OAuthProvider) {
-    const name = provider.name
+    const name = providerTitle(provider)

-    if (!window.confirm(`Remove ${name}?`)) {
+    if (!window.confirm(t.settings.providers.removeConfirm(name))) {
      return
    }

@ -280,10 +293,10 @@ export function ProvidersSettings({ onViewChange, view }: ProvidersSettingsProps

    try {
      await disconnectOAuthProvider(provider.id)
-      notify({ durationMs: 3_000, kind: 'success', message: `${name} removed.` })
+      notify({ durationMs: 3_000, kind: 'success', title: t.settings.providers.removedTitle, message: t.settings.providers.removedMessage(name) })
      await refreshOAuthProviders().catch(() => undefined)
    } catch (err) {
-      notifyError(err, `Could not remove ${name}`)
+      notifyError(err, t.settings.providers.failedRemove(name))
    } finally {
      setDisconnecting(null)
    }
--- a/apps/desktop/src/components/desktop-onboarding-overlay.tsx
+++ b/apps/desktop/src/components/desktop-onboarding-overlay.tsx
@ -180,7 +180,7 @@ const PROVIDER_DISPLAY: Record<string, { order: number; title: string }> = {

 const assetPath = (path: string) => `${import.meta.env.BASE_URL}${path.replace(/^\/+/, '')}`

-const providerTitle = (p: OAuthProvider) => PROVIDER_DISPLAY[p.id]?.title ?? p.name
+export const providerTitle = (p: OAuthProvider) => PROVIDER_DISPLAY[p.id]?.title ?? p.name
 const orderOf = (p: OAuthProvider) => PROVIDER_DISPLAY[p.id]?.order ?? 99

 export const sortProviders = (providers: OAuthProvider[]) =>
--- a/apps/desktop/src/i18n/en.ts
+++ b/apps/desktop/src/i18n/en.ts
@ -513,6 +513,12 @@ export const en: Translations = {
      collapse: 'Collapse',
      connectAnother: 'Connect another provider',
      otherProviders: 'Other providers',
+      removeConfirm: provider => `Remove ${provider}?`,
+      removeExternal: (provider, command) => `${provider} is managed outside Hermes. Remove it with ${command}.`,
+      removeKeyManaged: provider => `${provider} is configured from an API key. Remove it from API Keys.`,
+      removedTitle: 'Account removed',
+      removedMessage: provider => `${provider} was removed.`,
+      failedRemove: provider => `Could not remove ${provider}`,
      noProviderKeys: 'No provider API keys available.',
      loading: 'Loading providers...'
    },
--- a/apps/desktop/src/i18n/ja.ts
+++ b/apps/desktop/src/i18n/ja.ts
@ -642,6 +642,12 @@ export const ja = defineLocale({
      collapse: '折りたたむ',
      connectAnother: '別のプロバイダーを接続',
      otherProviders: 'その他のプロバイダー',
+      removeConfirm: provider => `${provider} を削除しますか？`,
+      removeExternal: (provider, command) => `${provider} は Hermes の外部で管理されています。${command} で削除してください。`,
+      removeKeyManaged: provider => `${provider} は API キーで設定されています。API Keys から削除してください。`,
+      removedTitle: 'アカウントを削除しました',
+      removedMessage: provider => `${provider} を削除しました。`,
+      failedRemove: provider => `${provider} を削除できませんでした`,
      noProviderKeys: '利用可能なプロバイダー API キーがありません。',
      loading: 'プロバイダーを読み込み中...'
    },
--- a/apps/desktop/src/i18n/types.ts
+++ b/apps/desktop/src/i18n/types.ts
@ -413,6 +413,12 @@ export interface Translations {
      collapse: string
      connectAnother: string
      otherProviders: string
+      removeConfirm: (provider: string) => string
+      removeExternal: (provider: string, command: string) => string
+      removeKeyManaged: (provider: string) => string
+      removedTitle: string
+      removedMessage: (provider: string) => string
+      failedRemove: (provider: string) => string
      noProviderKeys: string
      loading: string
    }
--- a/apps/desktop/src/i18n/zh-hant.ts
+++ b/apps/desktop/src/i18n/zh-hant.ts
@ -621,6 +621,12 @@ export const zhHant = defineLocale({
      collapse: '收合',
      connectAnother: '連結其他提供方',
      otherProviders: '其他提供方',
+      removeConfirm: provider => `移除 ${provider}？`,
+      removeExternal: (provider, command) => `${provider} 由 Hermes 外部管理。請使用 ${command} 移除。`,
+      removeKeyManaged: provider => `${provider} 由 API 金鑰設定。請從 API Keys 中移除。`,
+      removedTitle: '帳號已移除',
+      removedMessage: provider => `${provider} 已移除。`,
+      failedRemove: provider => `無法移除 ${provider}`,
      noProviderKeys: '沒有可用的提供方 API 金鑰。',
      loading: '正在載入提供方...'
    },
--- a/apps/desktop/src/i18n/zh.ts
+++ b/apps/desktop/src/i18n/zh.ts
@ -708,6 +708,12 @@ export const zh: Translations = {
      collapse: '收起',
      connectAnother: '连接其他提供方',
      otherProviders: '其他提供方',
+      removeConfirm: provider => `移除 ${provider}？`,
+      removeExternal: (provider, command) => `${provider} 由 Hermes 外部管理。请使用 ${command} 移除。`,
+      removeKeyManaged: provider => `${provider} 由 API 密钥配置。请从 API Keys 中移除。`,
+      removedTitle: '账号已移除',
+      removedMessage: provider => `${provider} 已移除。`,
+      failedRemove: provider => `无法移除 ${provider}`,
      noProviderKeys: '没有可用的提供方 API 密钥。',
      loading: '正在加载提供方...'
    },
--- a/apps/desktop/src/types/hermes.ts
+++ b/apps/desktop/src/types/hermes.ts
@ -47,6 +47,8 @@ export interface OAuthProviderStatus {

 export interface OAuthProvider {
  cli_command: string
+  disconnect_hint?: null | string
+  disconnectable?: boolean
  docs_url: string
  flow: 'device_code' | 'external' | 'loopback' | 'pkce'
  id: string
--- a/hermes_cli/web_server.py
+++ b/hermes_cli/web_server.py
@ -5134,6 +5134,15 @@ def _resolve_provider_status(provider_id: str, status_fn) -> Dict[str, Any]:
    return {"logged_in": False}


+def _oauth_provider_disconnect_hint(provider: Dict[str, Any], status: Dict[str, Any]) -> Optional[str]:
+    """Return the manual disconnect path when the API cannot clear this provider."""
+    if provider.get("flow") == "external":
+        return f"Use `{provider['cli_command']}` or that provider's CLI to remove it."
+    if status.get("source") == "env_var":
+        return "Remove the API key from Settings → Keys instead."
+    return None
+
+
@app.get("/api/providers/oauth")
 async def list_oauth_providers():
    """Enumerate every OAuth-capable LLM provider with current status.
@ -5155,12 +5164,15 @@ async def list_oauth_providers():
    providers = []
    for p in _OAUTH_PROVIDER_CATALOG:
        status = _resolve_provider_status(p["id"], p.get("status_fn"))
+        disconnect_hint = _oauth_provider_disconnect_hint(p, status)
        providers.append({
            "id": p["id"],
            "name": p["name"],
            "flow": p["flow"],
            "cli_command": p["cli_command"],
            "docs_url": p["docs_url"],
+            "disconnect_hint": disconnect_hint,
+            "disconnectable": disconnect_hint is None,
            "status": status,
        })
    return {"providers": providers}
@ -5171,37 +5183,56 @@ async def disconnect_oauth_provider(provider_id: str, request: Request):
    """Disconnect an OAuth provider. Token-protected (matches /env/reveal)."""
    _require_token(request)

-    valid_ids = {p["id"] for p in _OAUTH_PROVIDER_CATALOG}
-    if provider_id not in valid_ids:
+    catalog_by_id = {p["id"]: p for p in _OAUTH_PROVIDER_CATALOG}
+    provider = catalog_by_id.get(provider_id)
+    if provider is None:
        raise HTTPException(
            status_code=400,
            detail=f"Unknown provider: {provider_id}. "
-                   f"Available: {', '.join(sorted(valid_ids))}",
+                   f"Available: {', '.join(sorted(catalog_by_id))}",
        )

-    # Anthropic and claude-code clear the same Hermes-managed PKCE file
-    # AND forget the Claude Code import. We don't touch ~/.claude/* directly
-    # — that's owned by the Claude Code CLI; users can re-auth there if they
-    # want to undo a disconnect.
-    if provider_id in {"anthropic", "claude-code"}:
+    disconnect_hint = _oauth_provider_disconnect_hint(provider, {})
+    if disconnect_hint:
+        raise HTTPException(
+            status_code=400,
+            detail=f"{provider['name']} cannot be disconnected automatically. {disconnect_hint}",
+        )
+
+    status = _resolve_provider_status(provider_id, provider.get("status_fn"))
+    disconnect_hint = _oauth_provider_disconnect_hint(provider, status)
+    if disconnect_hint:
+        raise HTTPException(
+            status_code=400,
+            detail=f"{provider['name']} cannot be disconnected automatically. {disconnect_hint}",
+        )
+
+    # Anthropic clears only the Hermes-managed PKCE file and auth-store entry.
+    # The separate claude-code catalog row is external/read-only and rejected
+    # above so we never pretend to remove ~/.claude/* credentials owned by the CLI.
+    if provider_id == "anthropic":
+        cleared = False
        try:
            from agent.anthropic_adapter import _HERMES_OAUTH_FILE
            if _HERMES_OAUTH_FILE.exists():
                _HERMES_OAUTH_FILE.unlink()
+                cleared = True
        except Exception:
            pass
        # Also clear the credential pool entry if present.
        try:
            from hermes_cli.auth import clear_provider_auth
-            clear_provider_auth("anthropic")
+            cleared = clear_provider_auth("anthropic") or cleared
        except Exception:
            pass
        _log.info("oauth/disconnect: %s", provider_id)
-        return {"ok": True, "provider": provider_id}
+        return {"ok": bool(cleared), "provider": provider_id}

    try:
-        from hermes_cli.auth import clear_provider_auth
+        from hermes_cli.auth import clear_provider_auth, invalidate_nous_auth_status_cache
        cleared = clear_provider_auth(provider_id)
+        if provider_id == "nous":
+            invalidate_nous_auth_status_cache()
        _log.info("oauth/disconnect: %s (cleared=%s)", provider_id, cleared)
        return {"ok": bool(cleared), "provider": provider_id}
    except Exception as e:
--- a/tests/hermes_cli/test_web_oauth_dispatch.py
+++ b/tests/hermes_cli/test_web_oauth_dispatch.py
@ -337,6 +337,53 @@ def test_xai_oauth_listed_as_loopback_flow():
    assert "grok" in providers["xai-oauth"]["name"].lower()


+def test_oauth_catalog_marks_external_providers_not_disconnectable():
+    """External CLI credentials are visible in Accounts but cannot be removed by Hermes."""
+    resp = client.get("/api/providers/oauth", headers=HEADERS)
+    assert resp.status_code == 200, resp.text
+    providers = {p["id"]: p for p in resp.json()["providers"]}
+
+    assert providers["qwen-oauth"]["flow"] == "external"
+    assert providers["qwen-oauth"]["disconnectable"] is False
+    assert "provider's CLI" in providers["qwen-oauth"]["disconnect_hint"]
+
+    assert providers["claude-code"]["flow"] == "external"
+    assert providers["claude-code"]["disconnectable"] is False
+    assert "provider's CLI" in providers["claude-code"]["disconnect_hint"]
+
+
+def test_external_oauth_disconnect_rejected_before_auth_mutation(monkeypatch):
+    """DELETE must not pretend to remove credentials owned by another CLI."""
+    from hermes_cli import auth as auth_mod
+
+    def fail_clear_provider_auth(provider_id=None):
+        raise AssertionError("external providers must not reach clear_provider_auth")
+
+    monkeypatch.setattr(auth_mod, "clear_provider_auth", fail_clear_provider_auth)
+
+    resp = client.delete("/api/providers/oauth/qwen-oauth", headers=HEADERS)
+    assert resp.status_code == 400, resp.text
+    assert "cannot be disconnected automatically" in resp.text
+    assert "provider's CLI" in resp.text
+
+
+def test_env_sourced_oauth_status_is_not_disconnectable(monkeypatch):
+    """An env/.env-backed Anthropic API key is removed from Keys, not OAuth Accounts."""
+    monkeypatch.setenv("ANTHROPIC_API_KEY", "test-anthropic-key")
+
+    resp = client.get("/api/providers/oauth", headers=HEADERS)
+    assert resp.status_code == 200, resp.text
+    providers = {p["id"]: p for p in resp.json()["providers"]}
+
+    assert providers["anthropic"]["status"]["source"] == "env_var"
+    assert providers["anthropic"]["disconnectable"] is False
+    assert providers["anthropic"]["disconnect_hint"] == "Remove the API key from Settings → Keys instead."
+
+    delete_resp = client.delete("/api/providers/oauth/anthropic", headers=HEADERS)
+    assert delete_resp.status_code == 400, delete_resp.text
+    assert "Settings" in delete_resp.text
+
+
 def test_xai_loopback_start_returns_authorize_url(monkeypatch):
    """Start MUST bind the loopback listener and hand back an xAI authorize URL."""
    from hermes_cli import auth as auth_mod