feat: OAuth provider framework & per-chat model switching#22
Conversation
Covers OAuth2 authentication for Google/OpenAI (Anthropic hidden), connected provider pool, per-chat model switching, and dynamic model lists. Made-with: Cursor
- Rename flow: "device" to flow: "manual" (avoid RFC 8628 confusion) - Add OAuth scopes per provider - Add chat-model filtering for dynamic model lists - Specify PKCE/state temporary storage (in-memory dict, 10 min TTL) Made-with: Cursor
12 tasks covering OAuth strategies, provider pool, token storage, API endpoints, per-chat model switching, and UI components. Made-with: Cursor
- Add token revocation to disconnect() (spec requirement) - Add manual code-paste fallback UI in oauth.js - Complete initialize.py wiring with concrete call sites - Invalidate model cache on connect Made-with: Cursor
Introduce the foundational OAuth provider framework with: - OAuthProvider abstract base class with authorization, token exchange, refresh, revoke, and model listing contracts - OAuthTokens and ModelInfo dataclasses - GoogleOAuth concrete implementation for Google Gemini API access - Tests covering provider_id, authorization URL generation, code exchange, token refresh, and model list filtering with mocked HTTP Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
…ching Made-with: Cursor
…roviders() Made-with: Cursor
…to settings Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
…ntegration Made-with: Cursor
Add OAuth connections section to agent settings with provider listing, connect/disconnect flows, popup-based authorization with manual code fallback, and client credential inputs. Made-with: Cursor
Add model picker to chat top bar allowing users to select from connected OAuth provider models per-chat, with override persistence via the chat_model_override endpoint. Made-with: Cursor
- Add httpx>=0.27.0 to requirements.txt - Fix sniffio error in OAuth tests by mocking httpx.AsyncClient at module level instead of patching class methods Made-with: Cursor
Nafania
left a comment
There was a problem hiding this comment.
Code Review: OAuth Provider Framework & Per-Chat Model Switching
Strengths
- Clean Strategy pattern —
OAuthProviderABC with concrete implementations (Google, OpenAI, Anthropic) is well-structured with clear contracts. Each strategy encapsulates provider-specific auth URLs, token endpoints, model list parsing, and revocation. - Atomic token persistence —
OAuthTokenStoreusestempfile.mkstemp+os.replacefor crash-safe writes. Corruption recovery (malformed JSON → treat as empty) is a good defensive choice. - PKCE support for Anthropic is correctly implemented with S256 challenge.
- ProviderPool credential resolution transparently falls back from OAuth → API key, which integrates cleanly with the existing
get_api_key()flow inmodels.py. - Thorough test coverage — 73+ tests across all layers (strategies, store, pool, YAML config, API endpoints, model override, initialize integration). Tests use proper mocking without hitting real services.
- OAuth state TTL with 10-minute expiry and cleanup prevents stale state accumulation.
- Manual code-paste fallback in the UI is a practical UX decision for environments where popup redirects fail.
Issues
Important (Should Fix)
See inline comments for details. Summary:
asyncio.run()insidedisconnect()will crash when called from async API endpoint — theOAuthDisconnect.process()is async, soasyncio.run()raisesRuntimeError. Same issue in_try_sync_refreshwithasyncio.get_event_loop().- Path traversal in
chat_model_override.py—chat_idis used directly in file path without sanitization. OAuthCallback.process()doesn't check if provider exists before callingexchange_code()— will raiseAttributeErroronNone.- OAuth tokens stored in plaintext JSON —
usr/oauth_tokens.jsoncontains access and refresh tokens in plain text with no file permission restrictions. _pending_statesshared via private import across 3 modules — creates tight coupling and is not multi-worker safe.- Per-chat override ignores
api_basefrom provider config — breaks providers like Venice, LM Studio that require custom API base URLs.
Minor (Nice to Have)
- Vision detection heuristics are fragile — OpenAI uses string matching (
"vision" in mid), Google always returnsTrue. - Model picker dropdown uses heavy inline styles — should use CSS classes.
- No "reset to default" option in the model picker — once a user sets an override, there's no way back without deleting the file.
asyncio.get_event_loop()is deprecated in Python 3.10+ — useasyncio.get_running_loop()instead.
Recommendations
- Consider making
ProviderPool.disconnect()anasync defmethod (or provide both sync/async variants) since all callers are async API endpoints. - The
_pending_statesdict should be extracted into a small class (e.g.,OAuthStateStore) that can be properly shared and tested independently. - Add file permission restriction (
os.chmod 0o600) after writingoauth_tokens.jsonto limit exposure. - For the model picker, add a "Use default" option that deletes the
model_override.jsonfile.
Assessment
Ready to merge: With fixes
The architecture is sound — Strategy pattern for OAuth, transparent credential resolution, and clean per-chat override mechanism. However, the asyncio.run() crash path in disconnect() is a runtime failure that will hit any user trying to disconnect a provider, and the path traversal in chat_model_override is a security concern. Both are straightforward fixes. The remaining issues are quality improvements that can be addressed incrementally.
| strategy = get_oauth_provider(provider_id) | ||
| if strategy: | ||
| try: | ||
| asyncio.run(strategy.revoke(tokens.access_token)) |
There was a problem hiding this comment.
Important: asyncio.run() will crash when called from an async context.
OAuthDisconnect.process() is an async def method, which means it runs inside an event loop. Calling asyncio.run() from within a running loop raises:
RuntimeError: asyncio.run() cannot be called from a running event loop
This means every user who clicks "Disconnect" in the UI will get a 500 error.
Fix: Make disconnect() an async method, or use the same ThreadPoolExecutor pattern you already have in _try_sync_refresh:
async def disconnect(self, provider_id: str):
tokens = self.store.load(provider_id)
if tokens:
strategy = get_oauth_provider(provider_id)
if strategy:
try:
await strategy.revoke(tokens.access_token)
except Exception as e:
logger.warning("Failed to revoke token for %s: %s", provider_id, e)
self.store.delete(provider_id)
self._model_cache.pop(provider_id, None)Then update OAuthDisconnect.process() to await pool.disconnect(provider_id).
| return current | ||
|
|
||
| try: | ||
| loop = asyncio.get_event_loop() |
There was a problem hiding this comment.
Important: asyncio.get_event_loop() is deprecated in Python 3.10+ and will emit DeprecationWarning. In Python 3.12+ it raises a warning when no running loop exists.
Additionally, the ThreadPoolExecutor + asyncio.run() pattern here is fragile — it creates a new event loop in a thread, which can cause issues with httpx.AsyncClient if connection pooling is involved.
Suggestion: Use asyncio.get_running_loop() in a try/except, and consider making get_credential() an async method instead, since its callers (API endpoints, _get_litellm_chat) typically operate in async contexts anyway.
|
|
||
|
|
||
| def _override_path(chat_id: str) -> str: | ||
| return files.get_abs_path(f"usr/chats/{chat_id}/model_override.json") |
There was a problem hiding this comment.
Important: Path traversal vulnerability.
chat_id is used directly in the file path without any sanitization:
f"usr/chats/{chat_id}/model_override.json"A malicious chat_id like ../../etc/passwd/.. could write files outside the intended directory. While files.get_abs_path() might resolve to an absolute path, it doesn't prevent traversal.
Fix: Validate that chat_id contains only safe characters:
import re
def _override_path(chat_id: str) -> str:
if not re.match(r'^[a-zA-Z0-9_-]+$', chat_id):
raise ValueError(f"Invalid chat_id: {chat_id}")
return files.get_abs_path(f"usr/chats/{chat_id}/model_override.json")| cid = dotenv.get_dotenv_value(f"OAUTH_CLIENT_ID_{provider_id.upper()}") or "" | ||
| cs = dotenv.get_dotenv_value(f"OAUTH_CLIENT_SECRET_{provider_id.upper()}") or "" | ||
|
|
||
| tokens = await provider.exchange_code( |
There was a problem hiding this comment.
Important: Missing null check — provider can be None.
get_oauth_provider(provider_id) returns None for unknown providers, but the code calls provider.exchange_code() without checking. This will raise AttributeError: 'NoneType' object has no attribute 'exchange_code'.
This can happen if the YAML config changes between the time the user starts the OAuth flow (authorize) and completes it (callback).
Fix:
provider = get_oauth_provider(provider_id)
if not provider:
return FlaskResponse(
"<html><body><h2>Unknown provider</h2></body></html>",
content_type="text/html", status=400,
)| try: | ||
| with os.fdopen(fd, "w", encoding="utf-8") as f: | ||
| json.dump(data, f, indent=2, default=str) | ||
| os.replace(tmp, self.file_path) |
There was a problem hiding this comment.
Important: OAuth tokens stored in plaintext.
This file writes access tokens and refresh tokens as plain JSON to usr/oauth_tokens.json. Anyone with read access to the filesystem can extract valid OAuth credentials.
The codebase already stores API keys via dotenv (which isn't encrypted either, but at least follows a different pattern). At minimum, consider:
- Restricting file permissions after write:
os.chmod(self.file_path, 0o600) - Documenting the security implications in the spec
- Long-term: encrypting tokens at rest using the same pattern as the secrets manager
| from python.helpers.oauth import get_oauth_provider | ||
| from python.helpers import dotenv | ||
|
|
||
| _pending_states: dict[str, dict] = {} |
There was a problem hiding this comment.
Important: Module-level mutable shared state imported by other modules.
_pending_states is imported directly by oauth_callback.py and oauth_exchange.py via from python.api.oauth_authorize import _pending_states. This creates:
- Tight coupling — renaming or restructuring this module silently breaks the others
- No access control — any module can mutate this dict without going through a proper interface
- Not multi-process safe — if the app runs with multiple workers (e.g., uvicorn workers > 1), each worker has its own
_pending_statesdict and auth flows will fail
Suggestion: Extract into a small OAuthStateStore class (similar to OAuthTokenStore) or at minimum provide getter/setter functions.
| result.append(ModelInfo( | ||
| id=mid, name=mid, | ||
| context_length=m.get("context_window", 0), | ||
| supports_vision="vision" in mid or mid.startswith("gpt-4"), |
There was a problem hiding this comment.
Minor: Fragile vision detection heuristic.
supports_vision="vision" in mid or mid.startswith("gpt-4")This has false positives (e.g., gpt-4-0314 doesn't support vision) and false negatives (e.g., o1, o3-mini, o4-mini may support vision). The OpenAI /models endpoint doesn't reliably expose vision capability.
Suggestion: Either default to True for all chat models (safer — the framework will gracefully handle unsupported calls) or maintain a small allowlist of known vision-capable model prefixes.
| name=m.get("displayName", model_id), | ||
| context_length=m.get("inputTokenLimit", 0), | ||
| supports_vision="generateContent" in methods, | ||
| )) |
There was a problem hiding this comment.
Minor: supports_vision is always True here.
Since the loop already filters to models where "generateContent" in methods, the condition supports_vision="generateContent" in methods is always True. This means all listed Gemini models are marked as vision-capable, which isn't accurate (some Gemini models are text-only).
The Google API exposes an inputTokenLimit field per modality — you could check if the model has image/video input support. Or just default to True for now with a TODO.
| from python.helpers.connected_providers import ProviderPool | ||
| pool = ProviderPool.get_instance() | ||
| if pool.is_connected(chat_override["provider"]): | ||
| chat_llm = models.ModelConfig( |
There was a problem hiding this comment.
Important: Per-chat override doesn't set api_base.
When the chat model is overridden, the new ModelConfig has no api_base set (defaults to ""). For providers configured with a custom api_base in model_providers.yaml (e.g., Venice.ai, LM Studio, Z.AI), this means the model call will go to the wrong endpoint.
Fix: Look up the provider's litellm_provider and kwargs.api_base from the YAML config:
from python.helpers.providers import get_provider_config
cfg = get_provider_config("chat", chat_override["provider"])
api_base = (cfg or {}).get("kwargs", {}).get("api_base", "")And pass it to the ModelConfig.
| } | ||
| }, | ||
|
|
||
| get currentLabel() { |
There was a problem hiding this comment.
Minor: No way to reset to default model.
Once a user selects a per-chat model override, there's no option to go back to the default (settings-configured) model. The currentLabel getter shows "Default model" only when currentOverride is null, but there's no selectModel path that sets it back to null.
Suggestion: Add a resetToDefault() method that sends a DELETE or empty POST to /chat_model_override, and add a "Use default model" option at the top of the dropdown.
- Fix asyncio.run() crash in disconnect() when called from async context by extracting _run_async() helper that detects running event loops - Remove deprecated asyncio.get_event_loop() usage - Sanitize chat_id with regex allowlist to prevent path traversal - Add null check for provider in OAuthCallback before exchange_code() - Set file permissions 0o600 on oauth_tokens.json via os.fchmod/chmod - Include api_base from provider config in per-chat model override - Add "Reset to default" option in model picker dropdown - Add _delete_override() and reset action to chat_model_override endpoint - Add tests for all new behaviors Made-with: Cursor
Nafania
left a comment
There was a problem hiding this comment.
Follow-up review on 2b38104
Good progress — 7 out of 10 issues addressed with tests. Quick summary:
| # | Issue | Status |
|---|---|---|
| 1 | asyncio.run() crash in disconnect() |
Fixed — _run_async() helper with get_running_loop() detection |
| 2 | Path traversal in chat_model_override |
Fixed — regex allowlist + validation in all paths |
| 3 | Null check in OAuthCallback |
Fixed — 400 response with message |
| 4 | Plaintext tokens | Partially fixed — 0o600 permissions applied |
| 5 | _pending_states shared import |
Not addressed (acceptable to defer) |
| 6 | Per-chat override missing api_base |
Has a bug — see inline comment |
| 7-10 | Minor issues | Reset-to-default fixed; vision heuristics and inline styles deferred |
One remaining bug in the api_base fix — see inline comment on initialize.py:54.
| if pool.is_connected(chat_override["provider"]): | ||
| from python.helpers.providers import get_provider_config | ||
| provider_cfg = get_provider_config("chat", chat_override["provider"]) or {} | ||
| api_base = provider_cfg.get("api_base", "") or "" |
There was a problem hiding this comment.
Bug: api_base is read from the wrong level.
In model_providers.yaml, api_base is nested under kwargs, not at the top level:
venice:
name: Venice.ai
litellm_provider: openai
kwargs:
api_base: https://api.venice.ai/api/v1So provider_cfg.get("api_base", "") will always return "" because the key doesn't exist at the top level.
Fix:
api_base = (provider_cfg.get("kwargs") or {}).get("api_base", "")This doesn't affect Google/OpenAI OAuth right now (they don't use api_base), but it would silently fail for any provider that does.
provider_cfg.get("api_base") always returned "" because api_base
is nested under kwargs in model_providers.yaml. Fixed to read from
(provider_cfg.get("kwargs") or {}).get("api_base", "").
Added test verifying api_base is correctly applied for providers
like Venice.ai that require custom API base URLs.
Made-with: Cursor
Nafania
left a comment
There was a problem hiding this comment.
Final review on f4c294d
The api_base fix is correct — now reads from (provider_cfg.get("kwargs") or {}).get("api_base", ""). Good test added verifying Venice.ai-style providers get the right api_base.
Updated status — all Important issues resolved
| # | Issue | Status |
|---|---|---|
| 1 | asyncio.run() crash in disconnect() |
Fixed (2b38104) |
| 2 | Path traversal in chat_model_override |
Fixed (2b38104) |
| 3 | Null check in OAuthCallback |
Fixed (2b38104) |
| 4 | Plaintext tokens — file permissions | Fixed (2b38104) |
| 5 | _pending_states shared import |
Deferred (acceptable) |
| 6 | Per-chat override api_base |
Fixed (f4c294d) |
| 7 | Vision heuristics | Deferred (minor) |
| 8 | Inline styles | Deferred (minor) |
| 9 | Reset to default | Fixed (2b38104) |
| 10 | Deprecated asyncio.get_event_loop() |
Fixed (2b38104) |
All 6 Important issues are resolved. The 4 remaining items are Minor/deferred and don't block merging.
Assessment
Ready to merge: Yes
The OAuth provider framework, ProviderPool credential resolution, per-chat model switching, and UI components are all solid. Security concerns (path traversal, file permissions, null checks) have been addressed. The async runtime crash is fixed. Good test coverage throughout — 140+ tests passing including the new fix-related tests.
- #3: duplicate response loop breaker (breaks after 3 identical responses) - #4: dynamic output truncation threshold based on context window size - #2: resolve §§secret() / $$secret() placeholders in MCP server env/args/url/headers - #19: scheduler update_task tool method + prompt documentation Already applied (verified, skipping): #22 parallel MCP init, agent0ai#62 context window optimization Upstream: PR agent0ai#1265, PR agent0ai#857, PR agent0ai#1150, PR agent0ai#1105 Made-with: Cursor
- #24 Anthropic OAuth session tokens: add anthropic_oauth provider and ANTHROPIC_SESSION_TOKEN fallback for Claude Code integration - #26 Concurrent requests limit: add limit_concurrent to ModelConfig with semaphore-based concurrency control in RateLimiter, settings, UI fields - #40 Prompt include plugin: new _promptinclude plugin that scans workdir for *.promptinclude.md files and injects them into the system prompt with token budgeting and gitignore-style exclusion Also verified #22 and agent0ai#62 were already applied in earlier phases. Made-with: Cursor
Summary
OAuthProviderABC with Google, OpenAI, and Anthropic concrete implementations)ProviderPoolsingleton) abstracting credential resolution — transparently uses OAuth tokens or API keys with automatic token refreshArchitecture
New files (14 source + 8 test)
python/helpers/oauth.pypython/helpers/oauth_store.pypython/helpers/connected_providers.pypython/api/oauth_*.py(6 files)python/api/provider_models.pypython/api/chat_model_override.pywebui/js/oauth.jswebui/js/model-picker.jsModified files
conf/model_providers.yamloauthblocks to google, openai, anthropicpython/helpers/providers.pyget_oauth_providers()python/helpers/settings.pymodels.pyget_api_key()delegates toProviderPoolinitialize.pyrequirements.txthttpx>=0.27.0Spec
docs/superpowers/specs/2026-03-26-oauth-provider-framework-design.mdTest plan
Made with Cursor