From ddb809bc43d9f3f5d3ec6b762896cfc33f24dc74 Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Mon, 23 Feb 2026 09:28:49 +0000 Subject: [PATCH] Add webhook endpoint authentication bypass and admin context unfiltered data access (#12956) Co-authored-by: openhands --- enterprise/server/middleware.py | 5 +- .../saas_app_conversation_info_injector.py | 7 + ..._saas_sql_app_conversation_info_service.py | 177 +++++++++ enterprise/tests/unit/test_auth_middleware.py | 82 +++++ .../event_callback/webhook_router.py | 3 +- .../app_server/test_webhook_router_auth.py | 346 ++++++++++++++++++ 6 files changed, 617 insertions(+), 3 deletions(-) create mode 100644 tests/unit/app_server/test_webhook_router_auth.py diff --git a/enterprise/server/middleware.py b/enterprise/server/middleware.py index c3d12c7897..3124e1ce67 100644 --- a/enterprise/server/middleware.py +++ b/enterprise/server/middleware.py @@ -164,7 +164,6 @@ class SetAuthCookieMiddleware: '/oauth/device/authorize', '/oauth/device/token', '/api/v1/web-client/config', - '/api/v1/webhooks/secrets', ) if path in ignore_paths: return False @@ -175,6 +174,10 @@ class SetAuthCookieMiddleware: ): return False + # Webhooks access is controlled using separate API keys + if path.startswith('/api/v1/webhooks/'): + return False + is_mcp = path.startswith('/mcp') is_api_route = path.startswith('/api') return is_api_route or is_mcp diff --git a/enterprise/server/utils/saas_app_conversation_info_injector.py b/enterprise/server/utils/saas_app_conversation_info_injector.py index 51d857aba1..64e33797fd 100644 --- a/enterprise/server/utils/saas_app_conversation_info_injector.py +++ b/enterprise/server/utils/saas_app_conversation_info_injector.py @@ -24,6 +24,7 @@ from openhands.app_server.app_conversation.sql_app_conversation_info_service imp ) from openhands.app_server.errors import AuthError from openhands.app_server.services.injector import InjectorState +from openhands.app_server.user.specifiy_user_context import ADMIN class SaasSQLAppConversationInfoService(SQLAppConversationInfoService): @@ -63,6 +64,12 @@ class SaasSQLAppConversationInfoService(SQLAppConversationInfoService): Raises: AuthError: If no user_id is available (secure default: deny access) """ + # For internal operations such as getting a conversation by session_api_key + # we need a mode that does not have filtering. The dependency `as_admin()` + # is used to enable it + if self.user_context == ADMIN: + return query + user_id_str = await self.user_context.get_user_id() if not user_id_str: # Secure default: no user means no access, not "show everything" diff --git a/enterprise/tests/unit/storage/test_saas_sql_app_conversation_info_service.py b/enterprise/tests/unit/storage/test_saas_sql_app_conversation_info_service.py index 6135f870be..ac7f7b3db6 100644 --- a/enterprise/tests/unit/storage/test_saas_sql_app_conversation_info_service.py +++ b/enterprise/tests/unit/storage/test_saas_sql_app_conversation_info_service.py @@ -486,3 +486,180 @@ class TestSaasSQLAppConversationInfoService: # Count should be 0 in org2 count_org2 = await user1_service_org2.count_app_conversation_info() assert count_org2 == 0 + + +class TestSaasSQLAppConversationInfoServiceAdminContext: + """Test suite for SaasSQLAppConversationInfoService with ADMIN context.""" + + @pytest.mark.asyncio + async def test_admin_context_returns_unfiltered_data( + self, + async_session_with_users: AsyncSession, + ): + """Test that ADMIN context returns unfiltered data (no user/org filtering).""" + # Create conversations for different users + user1_service = SaasSQLAppConversationInfoService( + db_session=async_session_with_users, + user_context=SpecifyUserContext(user_id=str(USER1_ID)), + ) + + # Create conversations for user1 in org1 + for i in range(3): + conv = AppConversationInfo( + id=uuid4(), + created_by_user_id=str(USER1_ID), + sandbox_id=f'sandbox_user1_{i}', + title=f'User1 Conversation {i}', + ) + await user1_service.save_app_conversation_info(conv) + + # Now create an ADMIN service + from openhands.app_server.user.specifiy_user_context import ADMIN + + admin_service = SaasSQLAppConversationInfoService( + db_session=async_session_with_users, + user_context=ADMIN, + ) + + # ADMIN should see ALL conversations (unfiltered) + admin_page = await admin_service.search_app_conversation_info() + assert ( + len(admin_page.items) == 3 + ), 'ADMIN context should see all conversations without filtering' + + # ADMIN count should return total count (3) + admin_count = await admin_service.count_app_conversation_info() + assert ( + admin_count == 3 + ), 'ADMIN context should count all conversations without filtering' + + @pytest.mark.asyncio + async def test_admin_context_can_access_any_conversation( + self, + async_session_with_users: AsyncSession, + ): + """Test that ADMIN context can access any conversation regardless of owner.""" + from openhands.app_server.user.specifiy_user_context import ADMIN + + # Create a conversation as user1 + user1_service = SaasSQLAppConversationInfoService( + db_session=async_session_with_users, + user_context=SpecifyUserContext(user_id=str(USER1_ID)), + ) + + conv = AppConversationInfo( + id=uuid4(), + created_by_user_id=str(USER1_ID), + sandbox_id='sandbox_user1', + title='User1 Private Conversation', + ) + await user1_service.save_app_conversation_info(conv) + + # Create a service as user2 in org2 - should not see user1's conversation + user2_service = SaasSQLAppConversationInfoService( + db_session=async_session_with_users, + user_context=SpecifyUserContext(user_id=str(USER2_ID)), + ) + + user2_page = await user2_service.search_app_conversation_info() + assert len(user2_page.items) == 0, 'User2 should not see User1 conversation' + + # But ADMIN should see ALL conversations including user1's + admin_service = SaasSQLAppConversationInfoService( + db_session=async_session_with_users, + user_context=ADMIN, + ) + + admin_page = await admin_service.search_app_conversation_info() + assert len(admin_page.items) == 1 + assert admin_page.items[0].id == conv.id + + # ADMIN should also be able to get specific conversation by ID + admin_get_conv = await admin_service.get_app_conversation_info(conv.id) + assert admin_get_conv is not None + assert admin_get_conv.id == conv.id + + @pytest.mark.asyncio + async def test_secure_select_admin_bypasses_filtering( + self, + async_session_with_users: AsyncSession, + ): + """Test that _secure_select returns unfiltered query for ADMIN context.""" + from openhands.app_server.user.specifiy_user_context import ADMIN + + # Create an ADMIN service + admin_service = SaasSQLAppConversationInfoService( + db_session=async_session_with_users, + user_context=ADMIN, + ) + + # Get the secure select query + query = await admin_service._secure_select() + + # Convert query to string to verify NO filters are present + query_str = str(query.compile(compile_kwargs={'literal_binds': True})) + + # For ADMIN, there should be no user_id or org_id filtering + # The query should not contain filters for user_id or org_id + assert str(USER1_ID) not in query_str.replace( + '-', '' + ), 'ADMIN context should not filter by user_id' + assert str(USER2_ID) not in query_str.replace( + '-', '' + ), 'ADMIN context should not filter by user_id' + + @pytest.mark.asyncio + async def test_regular_user_context_filters_correctly( + self, + async_session_with_users: AsyncSession, + ): + """Test that regular user context properly filters data (control test).""" + from openhands.app_server.user.specifiy_user_context import ADMIN + + # Create conversations for different users + user1_service = SaasSQLAppConversationInfoService( + db_session=async_session_with_users, + user_context=SpecifyUserContext(user_id=str(USER1_ID)), + ) + + # Create 3 conversations for user1 + for i in range(3): + conv = AppConversationInfo( + id=uuid4(), + created_by_user_id=str(USER1_ID), + sandbox_id=f'sandbox_user1_{i}', + title=f'User1 Conversation {i}', + ) + await user1_service.save_app_conversation_info(conv) + + # Create 2 conversations for user2 + user2_service = SaasSQLAppConversationInfoService( + db_session=async_session_with_users, + user_context=SpecifyUserContext(user_id=str(USER2_ID)), + ) + + for i in range(2): + conv = AppConversationInfo( + id=uuid4(), + created_by_user_id=str(USER2_ID), + sandbox_id=f'sandbox_user2_{i}', + title=f'User2 Conversation {i}', + ) + await user2_service.save_app_conversation_info(conv) + + # User1 should only see their 3 conversations + user1_page = await user1_service.search_app_conversation_info() + assert len(user1_page.items) == 3 + + # User2 should only see their 2 conversations + user2_page = await user2_service.search_app_conversation_info() + assert len(user2_page.items) == 2 + + # But ADMIN should see all 5 conversations + admin_service = SaasSQLAppConversationInfoService( + db_session=async_session_with_users, + user_context=ADMIN, + ) + + admin_page = await admin_service.search_app_conversation_info() + assert len(admin_page.items) == 5 diff --git a/enterprise/tests/unit/test_auth_middleware.py b/enterprise/tests/unit/test_auth_middleware.py index 86da5b2753..cfc45ee2e8 100644 --- a/enterprise/tests/unit/test_auth_middleware.py +++ b/enterprise/tests/unit/test_auth_middleware.py @@ -284,3 +284,85 @@ async def test_middleware_ignores_email_resend_path_no_tos_check( assert result == mock_response mock_call_next.assert_called_once_with(mock_request) # Should not raise TosNotAcceptedError for this path + + +@pytest.mark.asyncio +async def test_middleware_skips_webhook_endpoints( + middleware, mock_request, mock_response +): + """Test middleware skips webhook endpoints (/api/v1/webhooks/*) and doesn't require auth.""" + # Test various webhook paths + webhook_paths = [ + '/api/v1/webhooks/events', + '/api/v1/webhooks/events/123', + '/api/v1/webhooks/stats', + '/api/v1/webhooks/parent-conversation', + ] + + for path in webhook_paths: + mock_request.cookies = {} + mock_request.url = MagicMock() + mock_request.url.hostname = 'localhost' + mock_request.url.path = path + mock_call_next = AsyncMock(return_value=mock_response) + + # Act + result = await middleware(mock_request, mock_call_next) + + # Assert - middleware should skip auth check and call next + assert result == mock_response + mock_call_next.assert_called_once_with(mock_request) + + +@pytest.mark.asyncio +async def test_middleware_skips_webhook_secrets_endpoint( + middleware, mock_request, mock_response +): + """Test middleware skips the old /api/v1/webhooks/secrets endpoint.""" + # This was explicitly in ignore_paths but is now handled by the prefix check + mock_request.cookies = {} + mock_request.url = MagicMock() + mock_request.url.hostname = 'localhost' + mock_request.url.path = '/api/v1/webhooks/secrets' + mock_call_next = AsyncMock(return_value=mock_response) + + # Act + result = await middleware(mock_request, mock_call_next) + + # Assert - middleware should skip auth check and call next + assert result == mock_response + mock_call_next.assert_called_once_with(mock_request) + + +@pytest.mark.asyncio +async def test_middleware_does_not_skip_similar_non_webhook_paths( + middleware, mock_response +): + """Test middleware does NOT skip paths that start with /api/v1/webhook (without 's').""" + # These paths should still be processed by the middleware (not skipped) + # They start with /api so _should_attach returns True, and since there's no auth, + # middleware should return 401 response (it catches NoCredentialsError internally) + non_webhook_paths = [ + '/api/v1/webhook/events', + '/api/v1/webhook/something', + ] + + for path in non_webhook_paths: + # Create a fresh mock request for each test + mock_request = MagicMock(spec=Request) + mock_request.cookies = {} + mock_request.url = MagicMock() + mock_request.url.hostname = 'localhost' + mock_request.url.path = path + mock_request.headers = MagicMock() + mock_request.headers.get = MagicMock(side_effect=lambda k: None) + + # Since these paths start with /api, _should_attach returns True + # Since there's no auth, middleware catches NoCredentialsError and returns 401 + mock_call_next = AsyncMock() + result = await middleware(mock_request, mock_call_next) + + # Should return a 401 response, not raise an exception + assert result.status_code == status.HTTP_401_UNAUTHORIZED + # Should NOT call next for non-webhook paths when auth is missing + mock_call_next.assert_not_called() diff --git a/openhands/app_server/event_callback/webhook_router.py b/openhands/app_server/event_callback/webhook_router.py index 0b0091247a..64863bae40 100644 --- a/openhands/app_server/event_callback/webhook_router.py +++ b/openhands/app_server/event_callback/webhook_router.py @@ -62,7 +62,7 @@ async def valid_sandbox( ), sandbox_service: SandboxService = sandbox_service_dependency, ) -> SandboxInfo: - if session_api_key is None: + if not session_api_key: raise HTTPException( status.HTTP_401_UNAUTHORIZED, detail='X-Session-API-Key header is required' ) @@ -144,7 +144,6 @@ async def on_event( event_service: EventService = event_service_dependency, ) -> Success: """Webhook callback for when event stream events occur.""" - app_conversation_info = await valid_conversation( conversation_id, sandbox_info, app_conversation_info_service ) diff --git a/tests/unit/app_server/test_webhook_router_auth.py b/tests/unit/app_server/test_webhook_router_auth.py new file mode 100644 index 0000000000..6f2837a4b3 --- /dev/null +++ b/tests/unit/app_server/test_webhook_router_auth.py @@ -0,0 +1,346 @@ +"""Tests for webhook_router valid_sandbox and valid_conversation functions. + +This module tests the webhook authentication and authorization logic. +""" + +from unittest.mock import AsyncMock, MagicMock +from uuid import uuid4 + +import pytest +from fastapi import HTTPException, status + +from openhands.app_server.event_callback.webhook_router import ( + valid_conversation, + valid_sandbox, +) +from openhands.app_server.sandbox.sandbox_models import SandboxInfo, SandboxStatus +from openhands.app_server.user.specifiy_user_context import ADMIN + + +class TestValidSandbox: + """Test suite for valid_sandbox function.""" + + @pytest.mark.asyncio + async def test_valid_sandbox_with_valid_api_key(self): + """Test that valid API key returns sandbox info.""" + # Arrange + session_api_key = 'valid-api-key-123' + expected_sandbox = SandboxInfo( + id='sandbox-123', + status=SandboxStatus.RUNNING, + session_api_key=session_api_key, + created_by_user_id='user-123', + sandbox_spec_id='spec-123', + ) + + mock_sandbox_service = AsyncMock() + mock_sandbox_service.get_sandbox_by_session_api_key = AsyncMock( + return_value=expected_sandbox + ) + + # Act + result = await valid_sandbox( + user_context=ADMIN, + session_api_key=session_api_key, + sandbox_service=mock_sandbox_service, + ) + + # Assert + assert result == expected_sandbox + mock_sandbox_service.get_sandbox_by_session_api_key.assert_called_once_with( + session_api_key + ) + + @pytest.mark.asyncio + async def test_valid_sandbox_without_api_key_raises_401(self): + """Test that missing API key raises 401 error.""" + # Arrange + mock_sandbox_service = AsyncMock() + + # Act & Assert + with pytest.raises(HTTPException) as exc_info: + await valid_sandbox( + user_context=ADMIN, + session_api_key=None, + sandbox_service=mock_sandbox_service, + ) + + assert exc_info.value.status_code == status.HTTP_401_UNAUTHORIZED + assert 'X-Session-API-Key header is required' in exc_info.value.detail + + @pytest.mark.asyncio + async def test_valid_sandbox_with_invalid_api_key_raises_401(self): + """Test that invalid API key raises 401 error.""" + # Arrange + session_api_key = 'invalid-api-key' + mock_sandbox_service = AsyncMock() + mock_sandbox_service.get_sandbox_by_session_api_key = AsyncMock( + return_value=None + ) + + # Act & Assert + with pytest.raises(HTTPException) as exc_info: + await valid_sandbox( + user_context=ADMIN, + session_api_key=session_api_key, + sandbox_service=mock_sandbox_service, + ) + + assert exc_info.value.status_code == status.HTTP_401_UNAUTHORIZED + assert 'Invalid session API key' in exc_info.value.detail + + @pytest.mark.asyncio + async def test_valid_sandbox_with_empty_api_key_raises_401(self): + """Test that empty API key raises 401 error (same as missing key).""" + # Arrange - empty string is falsy, so it gets rejected at the check + session_api_key = '' + mock_sandbox_service = AsyncMock() + + # Act & Assert - should raise 401 because empty string fails the truth check + with pytest.raises(HTTPException) as exc_info: + await valid_sandbox( + user_context=ADMIN, + session_api_key=session_api_key, + sandbox_service=mock_sandbox_service, + ) + + assert exc_info.value.status_code == status.HTTP_401_UNAUTHORIZED + assert 'X-Session-API-Key header is required' in exc_info.value.detail + # Verify the sandbox service was NOT called (rejected before lookup) + mock_sandbox_service.get_sandbox_by_session_api_key.assert_not_called() + + +class TestValidConversation: + """Test suite for valid_conversation function.""" + + @pytest.mark.asyncio + async def test_valid_conversation_existing_returns_info(self): + """Test that existing conversation returns info.""" + # Arrange + conversation_id = uuid4() + sandbox_info = SandboxInfo( + id='sandbox-123', + status=SandboxStatus.RUNNING, + session_api_key='api-key', + created_by_user_id='user-123', + sandbox_spec_id='spec-123', + ) + + expected_info = MagicMock() + expected_info.created_by_user_id = 'user-123' + + mock_service = AsyncMock() + mock_service.get_app_conversation_info = AsyncMock(return_value=expected_info) + + # Act + result = await valid_conversation( + conversation_id=conversation_id, + sandbox_info=sandbox_info, + app_conversation_info_service=mock_service, + ) + + # Assert + assert result == expected_info + + @pytest.mark.asyncio + async def test_valid_conversation_new_creates_stub(self): + """Test that non-existing conversation creates a stub.""" + # Arrange + conversation_id = uuid4() + sandbox_info = SandboxInfo( + id='sandbox-123', + status=SandboxStatus.RUNNING, + session_api_key='api-key', + created_by_user_id='user-123', + sandbox_spec_id='spec-123', + ) + + mock_service = AsyncMock() + mock_service.get_app_conversation_info = AsyncMock(return_value=None) + + # Act + result = await valid_conversation( + conversation_id=conversation_id, + sandbox_info=sandbox_info, + app_conversation_info_service=mock_service, + ) + + # Assert + assert result.id == conversation_id + assert result.sandbox_id == sandbox_info.id + assert result.created_by_user_id == sandbox_info.created_by_user_id + + @pytest.mark.asyncio + async def test_valid_conversation_different_user_raises_auth_error(self): + """Test that conversation from different user raises AuthError.""" + # Arrange + conversation_id = uuid4() + sandbox_info = SandboxInfo( + id='sandbox-123', + status=SandboxStatus.RUNNING, + session_api_key='api-key', + created_by_user_id='user-123', + sandbox_spec_id='spec-123', + ) + + # Conversation created by different user + different_user_info = MagicMock() + different_user_info.created_by_user_id = 'different-user-id' + + mock_service = AsyncMock() + mock_service.get_app_conversation_info = AsyncMock( + return_value=different_user_info + ) + + # Act & Assert + from openhands.app_server.errors import AuthError + + with pytest.raises(AuthError): + await valid_conversation( + conversation_id=conversation_id, + sandbox_info=sandbox_info, + app_conversation_info_service=mock_service, + ) + + @pytest.mark.asyncio + async def test_valid_conversation_same_user_succeeds(self): + """Test that conversation from same user succeeds.""" + # Arrange + conversation_id = uuid4() + user_id = 'user-123' + sandbox_info = SandboxInfo( + id='sandbox-123', + status=SandboxStatus.RUNNING, + session_api_key='api-key', + created_by_user_id=user_id, + sandbox_spec_id='spec-123', + ) + + # Conversation created by same user + same_user_info = MagicMock() + same_user_info.created_by_user_id = user_id + + mock_service = AsyncMock() + mock_service.get_app_conversation_info = AsyncMock(return_value=same_user_info) + + # Act + result = await valid_conversation( + conversation_id=conversation_id, + sandbox_info=sandbox_info, + app_conversation_info_service=mock_service, + ) + + # Assert + assert result == same_user_info + + +class TestWebhookAuthenticationIntegration: + """Integration tests for webhook authentication flow.""" + + @pytest.mark.asyncio + async def test_full_auth_flow_valid_key(self): + """Test complete auth flow with valid API key.""" + # Arrange + session_api_key = 'valid-api-key' + sandbox_info = SandboxInfo( + id='sandbox-123', + status=SandboxStatus.RUNNING, + session_api_key=session_api_key, + created_by_user_id='user-123', + sandbox_spec_id='spec-123', + ) + + mock_sandbox_service = AsyncMock() + mock_sandbox_service.get_sandbox_by_session_api_key = AsyncMock( + return_value=sandbox_info + ) + + conversation_info = MagicMock() + conversation_info.created_by_user_id = 'user-123' + + mock_conversation_service = AsyncMock() + mock_conversation_service.get_app_conversation_info = AsyncMock( + return_value=conversation_info + ) + + # Act - Call valid_sandbox first + sandbox_result = await valid_sandbox( + user_context=ADMIN, + session_api_key=session_api_key, + sandbox_service=mock_sandbox_service, + ) + + # Then call valid_conversation + conversation_result = await valid_conversation( + conversation_id=uuid4(), + sandbox_info=sandbox_result, + app_conversation_info_service=mock_conversation_service, + ) + + # Assert + assert sandbox_result.id == 'sandbox-123' + assert conversation_result.created_by_user_id == 'user-123' + + @pytest.mark.asyncio + async def test_full_auth_flow_invalid_key_fails(self): + """Test complete auth flow with invalid API key fails at sandbox validation.""" + # Arrange + session_api_key = 'invalid-api-key' + mock_sandbox_service = AsyncMock() + mock_sandbox_service.get_sandbox_by_session_api_key = AsyncMock( + return_value=None + ) + + # Act & Assert - Should fail at valid_sandbox + with pytest.raises(HTTPException) as exc_info: + await valid_sandbox( + user_context=ADMIN, + session_api_key=session_api_key, + sandbox_service=mock_sandbox_service, + ) + + assert exc_info.value.status_code == status.HTTP_401_UNAUTHORIZED + + @pytest.mark.asyncio + async def test_full_auth_flow_wrong_user_fails(self): + """Test complete auth flow with valid key but wrong user fails.""" + # Arrange + session_api_key = 'valid-api-key' + sandbox_info = SandboxInfo( + id='sandbox-123', + status=SandboxStatus.RUNNING, + session_api_key=session_api_key, + created_by_user_id='user-123', + sandbox_spec_id='spec-123', + ) + + mock_sandbox_service = AsyncMock() + mock_sandbox_service.get_sandbox_by_session_api_key = AsyncMock( + return_value=sandbox_info + ) + + # Conversation created by different user + different_user_info = MagicMock() + different_user_info.created_by_user_id = 'different-user' + + mock_conversation_service = AsyncMock() + mock_conversation_service.get_app_conversation_info = AsyncMock( + return_value=different_user_info + ) + + # Act - valid_sandbox succeeds + sandbox_result = await valid_sandbox( + user_context=ADMIN, + session_api_key=session_api_key, + sandbox_service=mock_sandbox_service, + ) + + # But valid_conversation fails + from openhands.app_server.errors import AuthError + + with pytest.raises(AuthError): + await valid_conversation( + conversation_id=uuid4(), + sandbox_info=sandbox_result, + app_conversation_info_service=mock_conversation_service, + )