mirror of
https://github.com/All-Hands-AI/OpenHands.git
synced 2026-04-29 03:00:45 -04:00
Compare commits
7 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| 8197582aa4 | |||
| 1d8d5981e8 | |||
| a8b6406dac | |||
| 509d4a9513 | |||
| d099c21f5d | |||
| 4c89b5ad91 | |||
| 729c181313 |
@@ -27,7 +27,7 @@ repos:
|
||||
- id: ruff
|
||||
entry: ruff check --config dev_config/python/ruff.toml
|
||||
types_or: [python, pyi, jupyter]
|
||||
args: [--fix, --unsafe-fixes]
|
||||
args: [--fix, --unsafe-fixes, --verbose]
|
||||
exclude: ^(third_party/|enterprise/)
|
||||
# Run the formatter.
|
||||
- id: ruff-format
|
||||
|
||||
@@ -27,7 +27,7 @@ repos:
|
||||
- id: ruff
|
||||
entry: ruff check --config enterprise/dev_config/python/ruff.toml
|
||||
types_or: [python, pyi, jupyter]
|
||||
args: [--fix]
|
||||
args: [--fix, --verbose]
|
||||
files: ^enterprise/
|
||||
# Run the formatter.
|
||||
- id: ruff-format
|
||||
|
||||
@@ -5,12 +5,6 @@ from dataclasses import dataclass, field
|
||||
from uuid import uuid4
|
||||
|
||||
import socketio
|
||||
from server.logger import logger
|
||||
from server.utils.conversation_callback_utils import invoke_conversation_callbacks
|
||||
from storage.database import session_maker
|
||||
from storage.saas_settings_store import SaasSettingsStore
|
||||
from storage.stored_conversation_metadata import StoredConversationMetadata
|
||||
|
||||
from openhands.core.config import LLMConfig
|
||||
from openhands.core.config.openhands_config import OpenHandsConfig
|
||||
from openhands.core.config.utils import load_openhands_config
|
||||
@@ -37,6 +31,12 @@ from openhands.storage.files import FileStore
|
||||
from openhands.utils.async_utils import call_sync_from_async, wait_all
|
||||
from openhands.utils.shutdown_listener import should_continue
|
||||
|
||||
from server.logger import logger
|
||||
from server.utils.conversation_callback_utils import invoke_conversation_callbacks
|
||||
from storage.database import session_maker
|
||||
from storage.saas_settings_store import SaasSettingsStore
|
||||
from storage.stored_conversation_metadata import StoredConversationMetadata
|
||||
|
||||
# Time in seconds between cleanup operations for stale conversations
|
||||
_CLEANUP_INTERVAL_SECONDS = 15
|
||||
|
||||
|
||||
@@ -138,6 +138,7 @@ async def saas_search_repositories(
|
||||
per_page: int = 5,
|
||||
sort: str = 'stars',
|
||||
order: str = 'desc',
|
||||
selected_provider: ProviderType | None = None,
|
||||
provider_tokens: PROVIDER_TOKEN_TYPE | None = Depends(get_provider_tokens),
|
||||
access_token: SecretStr | None = Depends(get_access_token),
|
||||
user_id: str | None = Depends(get_user_id),
|
||||
@@ -155,6 +156,7 @@ async def saas_search_repositories(
|
||||
per_page=per_page,
|
||||
sort=sort,
|
||||
order=order,
|
||||
selected_provider=selected_provider,
|
||||
provider_tokens=provider_tokens,
|
||||
access_token=access_token,
|
||||
user_id=user_id,
|
||||
|
||||
@@ -23,7 +23,7 @@ class GitService {
|
||||
*/
|
||||
static async searchGitRepositories(
|
||||
query: string,
|
||||
per_page = 5,
|
||||
per_page = 100,
|
||||
selected_provider?: Provider,
|
||||
): Promise<GitRepository[]> {
|
||||
const response = await openHands.get<GitRepository[]>(
|
||||
|
||||
@@ -107,7 +107,7 @@ export function ChatMessage({
|
||||
</div>
|
||||
|
||||
<div
|
||||
className="text-sm w-fit"
|
||||
className="text-sm"
|
||||
style={{
|
||||
whiteSpace: "normal",
|
||||
wordBreak: "break-word",
|
||||
|
||||
@@ -6,7 +6,7 @@ export function useSearchRepositories(
|
||||
query: string,
|
||||
selectedProvider?: Provider | null,
|
||||
disabled?: boolean,
|
||||
pageSize: number = 3,
|
||||
pageSize: number = 100,
|
||||
) {
|
||||
return useQuery({
|
||||
queryKey: ["repositories", "search", query, selectedProvider, pageSize],
|
||||
|
||||
@@ -88,13 +88,15 @@ function GitChanges() {
|
||||
</div>
|
||||
</div>
|
||||
) : (
|
||||
gitChanges.map((change) => (
|
||||
<FileDiffViewer
|
||||
key={change.path}
|
||||
path={change.path}
|
||||
type={change.status}
|
||||
/>
|
||||
))
|
||||
gitChanges
|
||||
.slice(0, 100)
|
||||
.map((change) => (
|
||||
<FileDiffViewer
|
||||
key={change.path}
|
||||
path={change.path}
|
||||
type={change.status}
|
||||
/>
|
||||
))
|
||||
)}
|
||||
</main>
|
||||
);
|
||||
|
||||
@@ -28,6 +28,7 @@ Your primary role is to assist users by executing commands, modifying code, and
|
||||
* Before implementing any changes, first thoroughly understand the codebase through exploration.
|
||||
* If you are adding a lot of code to a function or file, consider splitting the function or file into smaller pieces when appropriate.
|
||||
* Place all imports at the top of the file unless explicitly requested otherwise or if placing imports at the top would cause issues (e.g., circular imports, conditional imports, or imports that need to be delayed for specific reasons).
|
||||
* If working in a git repo, before you commit code create a .gitignore file if one doesn't exist. And if there are existing files that should not be included then update the .gitignore file as appropriate.
|
||||
</CODE_QUALITY>
|
||||
|
||||
<VERSION_CONTROL>
|
||||
|
||||
@@ -214,7 +214,7 @@ class GitHubReposMixin(GitHubMixinBase):
|
||||
all_repos = []
|
||||
|
||||
# Search in user repositories
|
||||
user_query = f'{query} user:{user.login}'
|
||||
user_query = f'in:name {query} user:{user.login}'
|
||||
user_params = params.copy()
|
||||
user_params['q'] = user_query
|
||||
|
||||
|
||||
@@ -466,10 +466,22 @@ class ProviderHandler:
|
||||
except Exception as e:
|
||||
errors.append(f'{provider.value}: {str(e)}')
|
||||
|
||||
# Log all accumulated errors before raising AuthenticationError
|
||||
logger.error(
|
||||
f'Failed to access repository {repository} with all available providers. Errors: {"; ".join(errors)}'
|
||||
)
|
||||
# Log detailed error based on whether we had tokens or not
|
||||
if not self.provider_tokens:
|
||||
logger.error(
|
||||
f'Failed to access repository {repository}: No provider tokens available. '
|
||||
f'provider_tokens dict is empty.'
|
||||
)
|
||||
elif errors:
|
||||
logger.error(
|
||||
f'Failed to access repository {repository} with all available providers. '
|
||||
f'Tried providers: {list(self.provider_tokens.keys())}. '
|
||||
f'Errors: {"; ".join(errors)}'
|
||||
)
|
||||
else:
|
||||
logger.error(
|
||||
f'Failed to access repository {repository}: Unknown error (no providers tried, no errors recorded)'
|
||||
)
|
||||
raise AuthenticationError(f'Unable to access repo {repository}')
|
||||
|
||||
async def get_branches(
|
||||
|
||||
@@ -462,6 +462,7 @@ async def start_conversation(
|
||||
providers_set: ProvidersSetModel,
|
||||
conversation_id: str = Depends(validate_conversation_id),
|
||||
user_id: str = Depends(get_user_id),
|
||||
provider_tokens: PROVIDER_TOKEN_TYPE = Depends(get_provider_tokens),
|
||||
settings: Settings = Depends(get_user_settings),
|
||||
conversation_store: ConversationStore = Depends(get_conversation_store),
|
||||
) -> ConversationResponse:
|
||||
@@ -471,7 +472,22 @@ async def start_conversation(
|
||||
to start a conversation. If the conversation is already running, it will
|
||||
return the existing agent loop info.
|
||||
"""
|
||||
logger.info(f'Starting conversation: {conversation_id}')
|
||||
logger.info(
|
||||
f'Starting conversation: {conversation_id}',
|
||||
extra={'session_id': conversation_id},
|
||||
)
|
||||
|
||||
# Log token fetch status
|
||||
if provider_tokens:
|
||||
logger.info(
|
||||
f'/start endpoint: Fetched provider tokens: {list(provider_tokens.keys())}',
|
||||
extra={'session_id': conversation_id},
|
||||
)
|
||||
else:
|
||||
logger.warning(
|
||||
'/start endpoint: No provider tokens fetched (provider_tokens is None/empty)',
|
||||
extra={'session_id': conversation_id},
|
||||
)
|
||||
|
||||
try:
|
||||
# Check that the conversation exists
|
||||
@@ -488,7 +504,7 @@ async def start_conversation(
|
||||
|
||||
# Set up conversation init data with provider information
|
||||
conversation_init_data = await setup_init_conversation_settings(
|
||||
user_id, conversation_id, providers_set.providers_set or []
|
||||
user_id, conversation_id, providers_set.providers_set or [], provider_tokens
|
||||
)
|
||||
|
||||
# Start the agent loop
|
||||
|
||||
@@ -11,15 +11,10 @@ from openhands.server.routes.secrets import invalidate_legacy_secrets_store
|
||||
from openhands.server.settings import (
|
||||
GETSettingsModel,
|
||||
)
|
||||
from openhands.server.settings_validation import (
|
||||
check_llm_settings_changes,
|
||||
validate_llm_settings_access,
|
||||
)
|
||||
from openhands.server.shared import config
|
||||
from openhands.server.user_auth import (
|
||||
get_provider_tokens,
|
||||
get_secrets_store,
|
||||
get_user_id,
|
||||
get_user_settings,
|
||||
get_user_settings_store,
|
||||
)
|
||||
@@ -140,34 +135,17 @@ async def store_llm_settings(
|
||||
response_model=None,
|
||||
responses={
|
||||
200: {'description': 'Settings stored successfully', 'model': dict},
|
||||
403: {'description': 'Subscription required for pro models', 'model': dict},
|
||||
500: {'description': 'Error storing settings', 'model': dict},
|
||||
},
|
||||
)
|
||||
async def store_settings(
|
||||
settings: Settings,
|
||||
settings_store: SettingsStore = Depends(get_user_settings_store),
|
||||
user_id: str = Depends(get_user_id),
|
||||
) -> JSONResponse:
|
||||
# Check provider tokens are valid
|
||||
try:
|
||||
existing_settings = await settings_store.load()
|
||||
|
||||
# Check if any LLM-related settings are being changed
|
||||
llm_settings_being_changed = check_llm_settings_changes(
|
||||
settings, existing_settings
|
||||
)
|
||||
|
||||
if llm_settings_being_changed:
|
||||
has_access = await validate_llm_settings_access(user_id)
|
||||
if not has_access:
|
||||
return JSONResponse(
|
||||
status_code=status.HTTP_403_FORBIDDEN,
|
||||
content={
|
||||
'error': 'Modifying LLM settings requires an active OpenHands Pro subscription. Please upgrade your account to access LLM configuration.',
|
||||
'detail': 'Subscription required for LLM settings modifications',
|
||||
},
|
||||
)
|
||||
|
||||
# Convert to Settings model and merge with existing settings
|
||||
if existing_settings:
|
||||
settings = await store_llm_settings(settings, settings_store)
|
||||
|
||||
@@ -215,7 +215,10 @@ def create_provider_tokens_object(
|
||||
|
||||
|
||||
async def setup_init_conversation_settings(
|
||||
user_id: str | None, conversation_id: str, providers_set: list[ProviderType]
|
||||
user_id: str | None,
|
||||
conversation_id: str,
|
||||
providers_set: list[ProviderType],
|
||||
provider_tokens: PROVIDER_TOKEN_TYPE | None = None,
|
||||
) -> ConversationInitData:
|
||||
"""Set up conversation initialization data with provider tokens.
|
||||
|
||||
@@ -223,6 +226,7 @@ async def setup_init_conversation_settings(
|
||||
user_id: The user ID
|
||||
conversation_id: The conversation ID
|
||||
providers_set: List of provider types to set up tokens for
|
||||
provider_tokens: Optional provider tokens to use (for SAAS mode resume)
|
||||
|
||||
Returns:
|
||||
ConversationInitData with provider tokens configured
|
||||
@@ -243,11 +247,30 @@ async def setup_init_conversation_settings(
|
||||
session_init_args: dict = {}
|
||||
session_init_args = {**settings.__dict__, **session_init_args}
|
||||
|
||||
git_provider_tokens = create_provider_tokens_object(providers_set)
|
||||
logger.info(f'Git provider scaffold: {git_provider_tokens}')
|
||||
# Use provided tokens if available (for SAAS resume), otherwise create scaffold
|
||||
if provider_tokens:
|
||||
logger.info(
|
||||
f'Using provided provider_tokens: {list(provider_tokens.keys())}',
|
||||
extra={'session_id': conversation_id},
|
||||
)
|
||||
git_provider_tokens = provider_tokens
|
||||
else:
|
||||
logger.info(
|
||||
f'No provider_tokens provided, creating scaffold for: {providers_set}',
|
||||
extra={'session_id': conversation_id},
|
||||
)
|
||||
git_provider_tokens = create_provider_tokens_object(providers_set)
|
||||
logger.info(
|
||||
f'Git provider scaffold: {git_provider_tokens}',
|
||||
extra={'session_id': conversation_id},
|
||||
)
|
||||
|
||||
if server_config.app_mode != AppMode.SAAS and user_secrets:
|
||||
git_provider_tokens = user_secrets.provider_tokens
|
||||
if server_config.app_mode != AppMode.SAAS and user_secrets:
|
||||
logger.info(
|
||||
f'Non-SaaS mode: Overriding with user_secrets provider tokens: {list(user_secrets.provider_tokens.keys())}',
|
||||
extra={'session_id': conversation_id},
|
||||
)
|
||||
git_provider_tokens = user_secrets.provider_tokens
|
||||
|
||||
session_init_args['git_provider_tokens'] = git_provider_tokens
|
||||
if user_secrets:
|
||||
|
||||
@@ -1,122 +0,0 @@
|
||||
"""Settings validation utilities for LLM settings access control."""
|
||||
|
||||
from openhands.core.logger import openhands_logger as logger
|
||||
from openhands.server.shared import server_config
|
||||
from openhands.server.types import AppMode
|
||||
from openhands.storage.data_models.settings import Settings
|
||||
|
||||
|
||||
def _is_llm_setting_changing(setting_name: str, new_value, existing_settings) -> bool:
|
||||
"""Check if a specific LLM setting is being changed from its existing value.
|
||||
|
||||
Args:
|
||||
setting_name: Name of the setting to check
|
||||
new_value: New value being set
|
||||
existing_settings: Existing settings object (can be None)
|
||||
|
||||
Returns:
|
||||
bool: True if the setting is being changed, False otherwise
|
||||
"""
|
||||
if new_value is None:
|
||||
return False
|
||||
|
||||
# Handle special case for enable_default_condenser with default value
|
||||
if setting_name == 'enable_default_condenser':
|
||||
if not existing_settings:
|
||||
# First time setting - only validate if setting to non-default value
|
||||
return not new_value
|
||||
else:
|
||||
# Changing existing value
|
||||
return new_value != existing_settings.enable_default_condenser
|
||||
|
||||
# For other settings, validate if explicitly provided and different from existing
|
||||
if not existing_settings:
|
||||
return True
|
||||
|
||||
existing_value = getattr(existing_settings, setting_name, None)
|
||||
return new_value != existing_value
|
||||
|
||||
|
||||
def check_llm_settings_changes(settings: Settings, existing_settings) -> bool:
|
||||
"""Check if any LLM-related settings are being changed.
|
||||
|
||||
Validates both core LLM settings (model, API key, base URL) and advanced settings
|
||||
shown to SaaS users (confirmation mode, security analyzer, memory condenser settings).
|
||||
|
||||
Args:
|
||||
settings: New settings being applied
|
||||
existing_settings: Current settings (can be None)
|
||||
|
||||
Returns:
|
||||
bool: True if any LLM settings are being changed, False otherwise
|
||||
"""
|
||||
# Core LLM settings - always validate if provided
|
||||
core_llm_changes = any(
|
||||
[
|
||||
settings.llm_model is not None,
|
||||
settings.llm_api_key is not None,
|
||||
settings.llm_base_url is not None,
|
||||
]
|
||||
)
|
||||
|
||||
if core_llm_changes:
|
||||
return True
|
||||
|
||||
# Additional LLM settings shown to SaaS users - validate if actually changing
|
||||
advanced_llm_changes = any(
|
||||
[
|
||||
_is_llm_setting_changing(
|
||||
'confirmation_mode', settings.confirmation_mode, existing_settings
|
||||
),
|
||||
_is_llm_setting_changing(
|
||||
'security_analyzer', settings.security_analyzer, existing_settings
|
||||
),
|
||||
_is_llm_setting_changing(
|
||||
'enable_default_condenser',
|
||||
settings.enable_default_condenser,
|
||||
existing_settings,
|
||||
),
|
||||
_is_llm_setting_changing(
|
||||
'condenser_max_size', settings.condenser_max_size, existing_settings
|
||||
),
|
||||
]
|
||||
)
|
||||
|
||||
return advanced_llm_changes
|
||||
|
||||
|
||||
async def validate_llm_settings_access(user_id: str) -> bool:
|
||||
"""Validate if user has access to modify LLM settings in SaaS mode.
|
||||
|
||||
In SaaS mode, only pro users with active subscriptions can modify LLM settings.
|
||||
|
||||
Args:
|
||||
user_id: The user ID to check subscription for
|
||||
|
||||
Returns:
|
||||
bool: True if user can modify LLM settings, False otherwise
|
||||
"""
|
||||
# Skip validation in non-SaaS mode
|
||||
if server_config.app_mode != AppMode.SAAS:
|
||||
return True
|
||||
|
||||
# In SaaS mode, check for active subscription for ANY LLM settings changes
|
||||
try:
|
||||
# Import here to avoid circular imports and handle enterprise mode gracefully
|
||||
from enterprise.server.routes.billing import get_subscription_access
|
||||
|
||||
subscription = await get_subscription_access(user_id)
|
||||
# The get_subscription_access function already filters for ACTIVE status,
|
||||
# so if we get a subscription back, it means it's active
|
||||
return subscription is not None
|
||||
except ImportError:
|
||||
# Enterprise billing module not available - in SaaS mode, this means
|
||||
# we can't validate subscriptions, so deny access to be safe
|
||||
logger.warning(
|
||||
'Enterprise billing module not available in SaaS mode, denying LLM settings access'
|
||||
)
|
||||
return False
|
||||
except Exception as e:
|
||||
# On error, deny access to be safe
|
||||
logger.warning(f'Error checking subscription access for user {user_id}: {e}')
|
||||
return False
|
||||
@@ -297,7 +297,7 @@ async def test_github_search_repositories_with_organizations():
|
||||
# First call should be for user repositories
|
||||
user_call = calls[0]
|
||||
user_params = user_call[0][1] # Second argument is params
|
||||
assert user_params['q'] == 'openhands user:testuser'
|
||||
assert user_params['q'] == 'in:name openhands user:testuser'
|
||||
|
||||
# Second call should be for first organization
|
||||
org1_call = calls[1]
|
||||
|
||||
@@ -1,329 +0,0 @@
|
||||
"""Security tests for settings API to ensure pro-only features are properly validated on backend."""
|
||||
|
||||
import os
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pytest
|
||||
from fastapi import Request
|
||||
from fastapi.testclient import TestClient
|
||||
from pydantic import SecretStr
|
||||
|
||||
from openhands.integrations.provider import ProviderToken, ProviderType
|
||||
from openhands.server.app import app
|
||||
from openhands.server.types import AppMode
|
||||
from openhands.server.user_auth.user_auth import UserAuth
|
||||
from openhands.storage.data_models.user_secrets import UserSecrets
|
||||
from openhands.storage.memory import InMemoryFileStore
|
||||
from openhands.storage.secrets.secrets_store import SecretsStore
|
||||
from openhands.storage.settings.file_settings_store import FileSettingsStore
|
||||
from openhands.storage.settings.settings_store import SettingsStore
|
||||
|
||||
|
||||
class MockUserAuthNonPro(UserAuth):
|
||||
"""Mock implementation of UserAuth for non-pro user testing"""
|
||||
|
||||
def __init__(self):
|
||||
self._settings = None
|
||||
self._settings_store = MagicMock()
|
||||
self._settings_store.load = AsyncMock(return_value=None)
|
||||
self._settings_store.store = AsyncMock()
|
||||
|
||||
async def get_user_id(self) -> str | None:
|
||||
return 'test-user-nonpro'
|
||||
|
||||
async def get_user_email(self) -> str | None:
|
||||
return 'nonpro@test.com'
|
||||
|
||||
async def get_access_token(self) -> SecretStr | None:
|
||||
return SecretStr('test-token-nonpro')
|
||||
|
||||
async def get_provider_tokens(self) -> dict[ProviderType, ProviderToken] | None:
|
||||
return None
|
||||
|
||||
async def get_user_settings_store(self) -> SettingsStore | None:
|
||||
return self._settings_store
|
||||
|
||||
async def get_secrets_store(self) -> SecretsStore | None:
|
||||
return None
|
||||
|
||||
async def get_user_secrets(self) -> UserSecrets | None:
|
||||
return None
|
||||
|
||||
@classmethod
|
||||
async def get_instance(cls, request: Request) -> UserAuth:
|
||||
return MockUserAuthNonPro()
|
||||
|
||||
|
||||
class MockUserAuthPro(UserAuth):
|
||||
"""Mock implementation of UserAuth for pro user testing"""
|
||||
|
||||
def __init__(self):
|
||||
self._settings = None
|
||||
self._settings_store = MagicMock()
|
||||
self._settings_store.load = AsyncMock(return_value=None)
|
||||
self._settings_store.store = AsyncMock()
|
||||
|
||||
async def get_user_id(self) -> str | None:
|
||||
return 'test-user-pro'
|
||||
|
||||
async def get_user_email(self) -> str | None:
|
||||
return 'pro@test.com'
|
||||
|
||||
async def get_access_token(self) -> SecretStr | None:
|
||||
return SecretStr('test-token-pro')
|
||||
|
||||
async def get_provider_tokens(self) -> dict[ProviderType, ProviderToken] | None:
|
||||
return None
|
||||
|
||||
async def get_user_settings_store(self) -> SettingsStore | None:
|
||||
return self._settings_store
|
||||
|
||||
async def get_secrets_store(self) -> SecretsStore | None:
|
||||
return None
|
||||
|
||||
async def get_user_secrets(self) -> UserSecrets | None:
|
||||
return None
|
||||
|
||||
@classmethod
|
||||
async def get_instance(cls, request: Request) -> UserAuth:
|
||||
return MockUserAuthPro()
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def test_client_non_pro():
|
||||
"""Test client for non-pro user"""
|
||||
with (
|
||||
patch.dict(
|
||||
os.environ, {'SESSION_API_KEY': '', 'APP_MODE': 'saas'}, clear=False
|
||||
),
|
||||
patch('openhands.server.dependencies._SESSION_API_KEY', None),
|
||||
patch('openhands.server.shared.server_config.app_mode', AppMode.SAAS),
|
||||
patch(
|
||||
'openhands.server.user_auth.user_auth.UserAuth.get_instance',
|
||||
return_value=MockUserAuthNonPro(),
|
||||
),
|
||||
patch(
|
||||
'openhands.storage.settings.file_settings_store.FileSettingsStore.get_instance',
|
||||
AsyncMock(return_value=FileSettingsStore(InMemoryFileStore())),
|
||||
),
|
||||
# Mock the validation function at the routes level to return False (no access)
|
||||
patch(
|
||||
'openhands.server.routes.settings.validate_llm_settings_access',
|
||||
AsyncMock(return_value=False),
|
||||
),
|
||||
):
|
||||
client = TestClient(app)
|
||||
yield client
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def test_client_pro():
|
||||
"""Test client for pro user"""
|
||||
with (
|
||||
patch.dict(
|
||||
os.environ, {'SESSION_API_KEY': '', 'APP_MODE': 'saas'}, clear=False
|
||||
),
|
||||
patch('openhands.server.dependencies._SESSION_API_KEY', None),
|
||||
patch('openhands.server.shared.server_config.app_mode', AppMode.SAAS),
|
||||
patch(
|
||||
'openhands.server.user_auth.user_auth.UserAuth.get_instance',
|
||||
return_value=MockUserAuthPro(),
|
||||
),
|
||||
patch(
|
||||
'openhands.storage.settings.file_settings_store.FileSettingsStore.get_instance',
|
||||
AsyncMock(return_value=FileSettingsStore(InMemoryFileStore())),
|
||||
),
|
||||
# Mock the validation function at the routes level to return True (has access)
|
||||
patch(
|
||||
'openhands.server.routes.settings.validate_llm_settings_access',
|
||||
AsyncMock(return_value=True),
|
||||
),
|
||||
):
|
||||
client = TestClient(app)
|
||||
yield client
|
||||
|
||||
|
||||
# Test data constants
|
||||
OPENHANDS_PRO_MODELS = [
|
||||
'openhands/claude-sonnet-4-20250514',
|
||||
'openhands/gpt-5-2025-08-07',
|
||||
'openhands/gpt-5-mini-2025-08-07',
|
||||
'openhands/claude-opus-4-20250514',
|
||||
'openhands/claude-opus-4-1-20250805',
|
||||
'openhands/gemini-2.5-pro',
|
||||
'openhands/o3',
|
||||
'openhands/o4-mini',
|
||||
]
|
||||
|
||||
DEFAULT_MODEL = 'claude-sonnet-4-20250514'
|
||||
|
||||
USER_PROVIDED_MODELS = [
|
||||
'anthropic/claude-3-5-sonnet-20241022',
|
||||
'openai/gpt-4o',
|
||||
'mistral/mistral-large',
|
||||
]
|
||||
|
||||
|
||||
# Helper functions
|
||||
def create_base_settings(**overrides):
|
||||
"""Create base settings data with optional overrides"""
|
||||
base_settings = {
|
||||
'language': 'en',
|
||||
'agent': 'test-agent',
|
||||
'max_iterations': 100,
|
||||
}
|
||||
base_settings.update(overrides)
|
||||
return base_settings
|
||||
|
||||
|
||||
def assert_forbidden_response(response, model_or_setting_name=''):
|
||||
"""Assert that response is 403 with subscription-related error"""
|
||||
assert response.status_code == 403, (
|
||||
f'{model_or_setting_name} should be forbidden for non-pro users'
|
||||
)
|
||||
response_data = response.json()
|
||||
assert any(
|
||||
keyword in response_data.get('detail', '').lower()
|
||||
or keyword in response_data.get('error', '').lower()
|
||||
for keyword in ['subscription', 'pro', 'upgrade']
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
'model',
|
||||
[
|
||||
'openhands/claude-sonnet-4-20250514',
|
||||
'openhands/gpt-5-2025-08-07',
|
||||
'openhands/claude-opus-4-20250514',
|
||||
DEFAULT_MODEL,
|
||||
]
|
||||
+ USER_PROVIDED_MODELS,
|
||||
)
|
||||
@pytest.mark.asyncio
|
||||
async def test_non_pro_user_cannot_set_any_llm_model(test_client_non_pro, model):
|
||||
"""SECURITY TEST: Non-pro user should not be able to set any LLM model"""
|
||||
settings_data = create_base_settings(llm_model=model, llm_api_key='test-key')
|
||||
response = test_client_non_pro.post('/api/settings', json=settings_data)
|
||||
assert_forbidden_response(response, f'Model {model}')
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
'llm_setting,value',
|
||||
[
|
||||
('llm_api_key', 'new-api-key'),
|
||||
('llm_base_url', 'https://custom-api.example.com'),
|
||||
('llm_model', DEFAULT_MODEL),
|
||||
('confirmation_mode', True),
|
||||
('security_analyzer', 'llm'),
|
||||
('enable_default_condenser', False),
|
||||
('condenser_max_size', 50),
|
||||
],
|
||||
)
|
||||
@pytest.mark.asyncio
|
||||
async def test_non_pro_user_cannot_set_individual_llm_settings(
|
||||
test_client_non_pro, llm_setting, value
|
||||
):
|
||||
"""SECURITY TEST: Non-pro user should not be able to set individual LLM settings"""
|
||||
settings_data = create_base_settings(**{llm_setting: value})
|
||||
response = test_client_non_pro.post('/api/settings', json=settings_data)
|
||||
assert_forbidden_response(response, f'LLM setting {llm_setting}')
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_non_pro_user_can_set_non_llm_settings(test_client_non_pro):
|
||||
"""Non-pro users should still be able to modify non-LLM settings"""
|
||||
# Only use settings that definitely don't trigger LLM validation
|
||||
settings_data = {
|
||||
'language': 'fr',
|
||||
'max_iterations': 50,
|
||||
'user_consents_to_analytics': True,
|
||||
'git_user_name': 'test-user',
|
||||
'git_user_email': 'test@example.com',
|
||||
}
|
||||
response = test_client_non_pro.post('/api/settings', json=settings_data)
|
||||
assert response.status_code == 200
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pro_user_can_set_llm_models(test_client_pro):
|
||||
"""Pro user should be able to set any LLM models"""
|
||||
settings_data = create_base_settings(
|
||||
llm_model='openhands/claude-sonnet-4-20250514', llm_api_key='test-key'
|
||||
)
|
||||
response = test_client_pro.post('/api/settings', json=settings_data)
|
||||
assert response.status_code == 200
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_expired_subscription_cannot_access_llm_settings():
|
||||
"""SECURITY TEST: User with expired subscription should not access LLM settings"""
|
||||
with (
|
||||
patch.dict(
|
||||
os.environ, {'SESSION_API_KEY': '', 'APP_MODE': 'saas'}, clear=False
|
||||
),
|
||||
patch('openhands.server.dependencies._SESSION_API_KEY', None),
|
||||
patch('openhands.server.shared.server_config.app_mode', AppMode.SAAS),
|
||||
patch(
|
||||
'openhands.server.user_auth.user_auth.UserAuth.get_instance',
|
||||
return_value=MockUserAuthPro(),
|
||||
),
|
||||
patch(
|
||||
'openhands.storage.settings.file_settings_store.FileSettingsStore.get_instance',
|
||||
AsyncMock(return_value=FileSettingsStore(InMemoryFileStore())),
|
||||
),
|
||||
# Mock validation to return False (expired subscription, no access)
|
||||
patch(
|
||||
'openhands.server.routes.settings.validate_llm_settings_access',
|
||||
AsyncMock(return_value=False),
|
||||
),
|
||||
):
|
||||
client = TestClient(app)
|
||||
settings_data = create_base_settings(
|
||||
llm_model='openhands/claude-sonnet-4-20250514', llm_api_key='test-key'
|
||||
)
|
||||
response = client.post('/api/settings', json=settings_data)
|
||||
assert_forbidden_response(response, 'Expired subscription')
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_direct_api_bypass_prevention(test_client_non_pro):
|
||||
"""SECURITY TEST: Direct API calls should still validate subscription status"""
|
||||
settings_data = create_base_settings(
|
||||
llm_model='openhands/claude-sonnet-4-20250514',
|
||||
llm_api_key='fake-api-key',
|
||||
llm_base_url='https://api.anthropic.com',
|
||||
remote_runtime_resource_factor=4,
|
||||
)
|
||||
|
||||
response = test_client_non_pro.post(
|
||||
'/api/settings',
|
||||
json=settings_data,
|
||||
headers={
|
||||
'Content-Type': 'application/json',
|
||||
'User-Agent': 'DirectAPIClient/1.0',
|
||||
},
|
||||
)
|
||||
assert_forbidden_response(response, 'Direct API bypass attempt')
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
'malicious_model',
|
||||
[
|
||||
'openhands/claude-sonnet-4-20250514', # Direct
|
||||
'OPENHANDS/claude-sonnet-4-20250514', # Case manipulation
|
||||
' openhands/claude-sonnet-4-20250514', # Leading space
|
||||
'openhands//claude-sonnet-4-20250514', # Double slash
|
||||
],
|
||||
)
|
||||
@pytest.mark.asyncio
|
||||
async def test_model_prefix_bypass_attempts_blocked(
|
||||
test_client_non_pro, malicious_model
|
||||
):
|
||||
"""SECURITY TEST: Various prefix bypass attempts should be blocked"""
|
||||
settings_data = create_base_settings(
|
||||
llm_model=malicious_model, llm_api_key='test-key'
|
||||
)
|
||||
response = test_client_non_pro.post('/api/settings', json=settings_data)
|
||||
assert_forbidden_response(
|
||||
response, f"Bypass attempt with model '{malicious_model}'"
|
||||
)
|
||||
@@ -0,0 +1,150 @@
|
||||
"""Tests for /start endpoint provider token handling."""
|
||||
|
||||
from types import MappingProxyType
|
||||
from unittest.mock import AsyncMock, patch
|
||||
|
||||
import pytest
|
||||
from pydantic import SecretStr
|
||||
|
||||
from openhands.integrations.provider import ProviderToken, ProviderType
|
||||
from openhands.server.data_models.agent_loop_info import AgentLoopInfo
|
||||
from openhands.server.routes.manage_conversations import (
|
||||
ProvidersSetModel,
|
||||
start_conversation,
|
||||
)
|
||||
from openhands.server.types import AppMode
|
||||
from openhands.storage.data_models.conversation_metadata import ConversationMetadata
|
||||
from openhands.storage.data_models.conversation_status import ConversationStatus
|
||||
from openhands.storage.data_models.settings import Settings
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def mock_settings():
|
||||
"""Create a real Settings object with minimal required fields."""
|
||||
return Settings(
|
||||
language='en',
|
||||
agent='CodeActAgent',
|
||||
max_iterations=100,
|
||||
llm_model='anthropic/claude-3-5-sonnet-20241022',
|
||||
llm_api_key=SecretStr('test_api_key_12345'),
|
||||
llm_base_url=None,
|
||||
)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def mock_provider_tokens():
|
||||
"""Create real provider tokens to test with."""
|
||||
return MappingProxyType(
|
||||
{
|
||||
ProviderType.GITHUB: ProviderToken(
|
||||
token=SecretStr('ghp_real_token_test123'), user_id='test_user_456'
|
||||
)
|
||||
}
|
||||
)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def mock_conversation_metadata():
|
||||
"""Create a real ConversationMetadata object."""
|
||||
return ConversationMetadata(
|
||||
conversation_id='test_conv_123',
|
||||
user_id='test_user_456',
|
||||
title='Test Conversation',
|
||||
selected_repository='test/repo',
|
||||
selected_branch='main',
|
||||
git_provider=ProviderType.GITHUB,
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_start_endpoint_passes_provider_tokens(
|
||||
mock_settings, mock_provider_tokens, mock_conversation_metadata
|
||||
):
|
||||
"""Test that /start endpoint passes provider_tokens to setup_init_conversation_settings.
|
||||
|
||||
This test verifies the full end-to-end flow with real tokens through to ConversationInitData.
|
||||
"""
|
||||
conversation_id = 'test_conv_123'
|
||||
user_id = 'test_user_456'
|
||||
providers_set = ProvidersSetModel(providers_set=[ProviderType.GITHUB])
|
||||
|
||||
# Mock conversation store
|
||||
mock_conversation_store = AsyncMock()
|
||||
mock_conversation_store.get_metadata = AsyncMock(
|
||||
return_value=mock_conversation_metadata
|
||||
)
|
||||
|
||||
# Mock agent loop info that will be returned
|
||||
mock_agent_loop_info = AgentLoopInfo(
|
||||
conversation_id=conversation_id,
|
||||
url=None,
|
||||
session_api_key=None,
|
||||
event_store=None,
|
||||
status=ConversationStatus.RUNNING,
|
||||
)
|
||||
|
||||
# Mock only infrastructure - let setup_init_conversation_settings run for real
|
||||
with patch(
|
||||
'openhands.server.routes.manage_conversations.conversation_manager'
|
||||
) as mock_manager:
|
||||
# Mock the stores that setup_init_conversation_settings needs
|
||||
with patch(
|
||||
'openhands.server.services.conversation_service.SettingsStoreImpl.get_instance'
|
||||
) as mock_settings_store_cls:
|
||||
with patch(
|
||||
'openhands.server.services.conversation_service.SecretsStoreImpl.get_instance'
|
||||
) as mock_secrets_store_cls:
|
||||
with patch(
|
||||
'openhands.server.services.conversation_service.server_config'
|
||||
) as mock_server_config:
|
||||
# Setup store mocks
|
||||
mock_settings_store = AsyncMock()
|
||||
mock_settings_store.load = AsyncMock(return_value=mock_settings)
|
||||
mock_settings_store_cls.return_value = mock_settings_store
|
||||
|
||||
mock_secrets_store = AsyncMock()
|
||||
mock_secrets_store.load = AsyncMock(return_value=None)
|
||||
mock_secrets_store_cls.return_value = mock_secrets_store
|
||||
|
||||
mock_server_config.app_mode = AppMode.SAAS
|
||||
|
||||
mock_manager.maybe_start_agent_loop = AsyncMock(
|
||||
return_value=mock_agent_loop_info
|
||||
)
|
||||
|
||||
# Call endpoint with provider tokens
|
||||
response = await start_conversation(
|
||||
providers_set=providers_set,
|
||||
conversation_id=conversation_id,
|
||||
user_id=user_id,
|
||||
provider_tokens=mock_provider_tokens,
|
||||
settings=mock_settings,
|
||||
conversation_store=mock_conversation_store,
|
||||
)
|
||||
|
||||
# Verify ConversationInitData has real provider tokens
|
||||
mock_manager.maybe_start_agent_loop.assert_called_once()
|
||||
call_kwargs = mock_manager.maybe_start_agent_loop.call_args[1]
|
||||
conversation_init_data = call_kwargs['settings']
|
||||
|
||||
assert conversation_init_data.git_provider_tokens is not None
|
||||
assert (
|
||||
conversation_init_data.git_provider_tokens
|
||||
== mock_provider_tokens
|
||||
)
|
||||
assert (
|
||||
ProviderType.GITHUB
|
||||
in conversation_init_data.git_provider_tokens
|
||||
)
|
||||
|
||||
github_token = conversation_init_data.git_provider_tokens[
|
||||
ProviderType.GITHUB
|
||||
]
|
||||
assert (
|
||||
github_token.token.get_secret_value()
|
||||
== 'ghp_real_token_test123'
|
||||
)
|
||||
assert github_token.user_id == 'test_user_456'
|
||||
|
||||
assert response.status == 'ok'
|
||||
assert response.conversation_id == conversation_id
|
||||
@@ -0,0 +1,162 @@
|
||||
"""Unit tests for conversation_service.py - specifically testing provider token handling.
|
||||
|
||||
These tests verify that setup_init_conversation_settings correctly handles provider tokens
|
||||
in different scenarios (provided tokens vs scaffold creation).
|
||||
"""
|
||||
|
||||
from types import MappingProxyType
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pytest
|
||||
from pydantic import SecretStr
|
||||
|
||||
from openhands.integrations.provider import ProviderToken, ProviderType
|
||||
from openhands.server.services.conversation_service import (
|
||||
setup_init_conversation_settings,
|
||||
)
|
||||
from openhands.server.types import AppMode
|
||||
from openhands.storage.data_models.settings import Settings
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def mock_settings():
|
||||
"""Create a real Settings object with minimal required fields."""
|
||||
return Settings(
|
||||
language='en',
|
||||
agent='CodeActAgent',
|
||||
max_iterations=100,
|
||||
llm_model='anthropic/claude-3-5-sonnet-20241022',
|
||||
llm_api_key=SecretStr('test_api_key_12345'),
|
||||
llm_base_url=None,
|
||||
)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def mock_provider_tokens():
|
||||
"""Create real provider tokens to test with."""
|
||||
return MappingProxyType(
|
||||
{
|
||||
ProviderType.GITHUB: ProviderToken(
|
||||
token=SecretStr('ghp_real_token_test123'), user_id='test_user_456'
|
||||
)
|
||||
}
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_setup_with_provided_tokens_uses_real_tokens(
|
||||
mock_settings, mock_provider_tokens
|
||||
):
|
||||
"""Test that real tokens are used when provided in SAAS mode.
|
||||
|
||||
Verifies provider tokens passed in are used in ConversationInitData.
|
||||
"""
|
||||
user_id = 'test_user_456'
|
||||
conversation_id = 'test_conv_123'
|
||||
providers_set = [ProviderType.GITHUB]
|
||||
|
||||
# Mock the stores to return our test settings
|
||||
with patch(
|
||||
'openhands.server.services.conversation_service.SettingsStoreImpl.get_instance'
|
||||
) as mock_settings_store_cls:
|
||||
with patch(
|
||||
'openhands.server.services.conversation_service.SecretsStoreImpl.get_instance'
|
||||
) as mock_secrets_store_cls:
|
||||
with patch(
|
||||
'openhands.server.services.conversation_service.server_config'
|
||||
) as mock_server_config:
|
||||
# Setup mocks
|
||||
mock_settings_store = AsyncMock()
|
||||
mock_settings_store.load = AsyncMock(return_value=mock_settings)
|
||||
mock_settings_store_cls.return_value = mock_settings_store
|
||||
|
||||
mock_secrets_store = AsyncMock()
|
||||
mock_secrets_store.load = AsyncMock(return_value=None)
|
||||
mock_secrets_store_cls.return_value = mock_secrets_store
|
||||
|
||||
mock_server_config.app_mode = AppMode.SAAS
|
||||
|
||||
# Call with real tokens
|
||||
result = await setup_init_conversation_settings(
|
||||
user_id=user_id,
|
||||
conversation_id=conversation_id,
|
||||
providers_set=providers_set,
|
||||
provider_tokens=mock_provider_tokens,
|
||||
)
|
||||
|
||||
# Verify real tokens are used
|
||||
assert result.git_provider_tokens is not None
|
||||
assert result.git_provider_tokens == mock_provider_tokens
|
||||
assert ProviderType.GITHUB in result.git_provider_tokens, (
|
||||
'GitHub provider should be in tokens'
|
||||
)
|
||||
|
||||
github_token = result.git_provider_tokens[ProviderType.GITHUB]
|
||||
assert (
|
||||
github_token.token.get_secret_value() == 'ghp_real_token_test123'
|
||||
), 'Should use real token, not None'
|
||||
assert github_token.user_id == 'test_user_456', (
|
||||
'Should preserve user_id from real token'
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_setup_without_tokens_non_saas_uses_user_secrets(mock_settings):
|
||||
"""Test that OSS mode uses user_secrets.provider_tokens when no tokens provided.
|
||||
|
||||
This test verifies OSS mode backward compatibility - tokens come from local config, not endpoint.
|
||||
"""
|
||||
user_id = 'test_user_456'
|
||||
conversation_id = 'test_conv_123'
|
||||
providers_set = [ProviderType.GITHUB]
|
||||
|
||||
# Create user_secrets with real tokens
|
||||
mock_user_secrets = MagicMock()
|
||||
mock_user_secrets.provider_tokens = MappingProxyType(
|
||||
{
|
||||
ProviderType.GITHUB: ProviderToken(
|
||||
token=SecretStr('ghp_local_token_from_config'),
|
||||
user_id='local_user_123',
|
||||
)
|
||||
}
|
||||
)
|
||||
mock_user_secrets.custom_secrets = MappingProxyType({}) # Empty dict is fine
|
||||
|
||||
with patch(
|
||||
'openhands.server.services.conversation_service.SettingsStoreImpl.get_instance'
|
||||
) as mock_settings_store_cls:
|
||||
with patch(
|
||||
'openhands.server.services.conversation_service.SecretsStoreImpl.get_instance'
|
||||
) as mock_secrets_store_cls:
|
||||
with patch(
|
||||
'openhands.server.services.conversation_service.server_config'
|
||||
) as mock_server_config:
|
||||
# Setup mocks
|
||||
mock_settings_store = AsyncMock()
|
||||
mock_settings_store.load = AsyncMock(return_value=mock_settings)
|
||||
mock_settings_store_cls.return_value = mock_settings_store
|
||||
|
||||
mock_secrets_store = AsyncMock()
|
||||
mock_secrets_store.load = AsyncMock(return_value=mock_user_secrets)
|
||||
mock_secrets_store_cls.return_value = mock_secrets_store
|
||||
|
||||
mock_server_config.app_mode = AppMode.OSS
|
||||
|
||||
# Call without endpoint tokens
|
||||
result = await setup_init_conversation_settings(
|
||||
user_id=user_id,
|
||||
conversation_id=conversation_id,
|
||||
providers_set=providers_set,
|
||||
provider_tokens=None,
|
||||
)
|
||||
|
||||
# Verify user_secrets tokens are used
|
||||
assert result.git_provider_tokens is not None
|
||||
assert ProviderType.GITHUB in result.git_provider_tokens
|
||||
|
||||
github_token = result.git_provider_tokens[ProviderType.GITHUB]
|
||||
assert (
|
||||
github_token.token.get_secret_value()
|
||||
== 'ghp_local_token_from_config'
|
||||
)
|
||||
assert github_token.user_id == 'local_user_123'
|
||||
Reference in New Issue
Block a user