From 173ad645418dd90350644fc8f662ef31ef43ce4c Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Tue, 6 May 2025 17:11:47 +0530 Subject: [PATCH 1/6] chore: Add unit test cases --- packages/toolbox-core/tests/test_client.py | 29 ++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/packages/toolbox-core/tests/test_client.py b/packages/toolbox-core/tests/test_client.py index b57624b7..3fc371b5 100644 --- a/packages/toolbox-core/tests/test_client.py +++ b/packages/toolbox-core/tests/test_client.py @@ -398,6 +398,35 @@ async def test_constructor_getters_missing_fail(self, tool_name, client): await client.load_tool(tool_name, auth_token_getters={AUTH_SERVICE: {}}) + @pytest.mark.asyncio + async def test_add_auth_token_getters_missing_fail(self, tool_name, client): + """ + Tests that adding a missing auth token getter raises ValueError. + """ + AUTH_SERVICE = "xmy-auth-service" + + tool = await client.load_tool(tool_name) + + with pytest.raises( + ValueError, + match=f"Authentication source\(s\) \`{AUTH_SERVICE}\` unused by tool \`{tool_name}\`.", + ): + tool.add_auth_token_getters({AUTH_SERVICE: {}}) + + @pytest.mark.asyncio + async def test_constructor_getters_missing_fail(self, tool_name, client): + """ + Tests that adding a missing auth token getter raises ValueError. + """ + AUTH_SERVICE = "xmy-auth-service" + + with pytest.raises( + ValueError, + match=f"Validation failed for tool '{tool_name}': unused auth tokens: {AUTH_SERVICE}.", + ): + await client.load_tool(tool_name, auth_token_getters={AUTH_SERVICE: {}}) + + class TestBoundParameter: @pytest.fixture From f02dd4f3ffbd076b8ae31e6a096112b3abdaeb37 Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Tue, 6 May 2025 17:12:37 +0530 Subject: [PATCH 2/6] chore: Delint --- packages/toolbox-core/tests/test_client.py | 29 ---------------------- 1 file changed, 29 deletions(-) diff --git a/packages/toolbox-core/tests/test_client.py b/packages/toolbox-core/tests/test_client.py index 3fc371b5..b57624b7 100644 --- a/packages/toolbox-core/tests/test_client.py +++ b/packages/toolbox-core/tests/test_client.py @@ -398,35 +398,6 @@ async def test_constructor_getters_missing_fail(self, tool_name, client): await client.load_tool(tool_name, auth_token_getters={AUTH_SERVICE: {}}) - @pytest.mark.asyncio - async def test_add_auth_token_getters_missing_fail(self, tool_name, client): - """ - Tests that adding a missing auth token getter raises ValueError. - """ - AUTH_SERVICE = "xmy-auth-service" - - tool = await client.load_tool(tool_name) - - with pytest.raises( - ValueError, - match=f"Authentication source\(s\) \`{AUTH_SERVICE}\` unused by tool \`{tool_name}\`.", - ): - tool.add_auth_token_getters({AUTH_SERVICE: {}}) - - @pytest.mark.asyncio - async def test_constructor_getters_missing_fail(self, tool_name, client): - """ - Tests that adding a missing auth token getter raises ValueError. - """ - AUTH_SERVICE = "xmy-auth-service" - - with pytest.raises( - ValueError, - match=f"Validation failed for tool '{tool_name}': unused auth tokens: {AUTH_SERVICE}.", - ): - await client.load_tool(tool_name, auth_token_getters={AUTH_SERVICE: {}}) - - class TestBoundParameter: @pytest.fixture From 5b72b21575f1450ea4f50ccd84b61c8b171a6e70 Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Tue, 6 May 2025 17:40:02 +0530 Subject: [PATCH 3/6] feat: Warn on insecure tool invocation with authentication This change introduces a warning that is displayed immediately before a tool invocation if: 1. The invocation includes an authentication header. 2. The connection is being made over non-secure HTTP. > [!IMPORTANT] The purpose of this warning is to alert the user to the security risk of sending credentials over an unencrypted channel and to encourage the use of HTTPS. --- packages/toolbox-core/src/toolbox_core/tool.py | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/packages/toolbox-core/src/toolbox_core/tool.py b/packages/toolbox-core/src/toolbox_core/tool.py index 98d04e17..79da97a8 100644 --- a/packages/toolbox-core/src/toolbox_core/tool.py +++ b/packages/toolbox-core/src/toolbox_core/tool.py @@ -257,18 +257,27 @@ async def __call__(self, *args: Any, **kwargs: Any) -> str: payload[param] = await resolve_value(value) # create headers for auth services - headers = {} + auth_headers = {} for auth_service, token_getter in self.__auth_service_token_getters.items(): - headers[self.__get_auth_header(auth_service)] = await resolve_value( + auth_headers[self.__get_auth_header(auth_service)] = await resolve_value( token_getter ) for client_header_name, client_header_val in self.__client_headers.items(): - headers[client_header_name] = await resolve_value(client_header_val) + auth_headers[client_header_name] = await resolve_value(client_header_val) + + # ID tokens contain sensitive user information (claims). Transmitting + # these over HTTP exposes the data to interception and unauthorized + # access. Always use HTTPS to ensure secure communication and protect + # user privacy. + if auth_headers and not self.__url.startswith("https://"): + warn( + "Sending ID token over HTTP. User data may be exposed. Use HTTPS for secure communication." + ) async with self.__session.post( self.__url, json=payload, - headers=headers, + headers=auth_headers, ) as resp: body = await resp.json() if resp.status < 200 or resp.status >= 300: From b7f3cafb8676f8c4d8a893abdbcf51d81d6f9108 Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Tue, 13 May 2025 15:57:46 +0530 Subject: [PATCH 4/6] fix!: Warn about https only during tool initialization --- packages/toolbox-core/src/toolbox_core/tool.py | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/packages/toolbox-core/src/toolbox_core/tool.py b/packages/toolbox-core/src/toolbox_core/tool.py index 79da97a8..98d04e17 100644 --- a/packages/toolbox-core/src/toolbox_core/tool.py +++ b/packages/toolbox-core/src/toolbox_core/tool.py @@ -257,27 +257,18 @@ async def __call__(self, *args: Any, **kwargs: Any) -> str: payload[param] = await resolve_value(value) # create headers for auth services - auth_headers = {} + headers = {} for auth_service, token_getter in self.__auth_service_token_getters.items(): - auth_headers[self.__get_auth_header(auth_service)] = await resolve_value( + headers[self.__get_auth_header(auth_service)] = await resolve_value( token_getter ) for client_header_name, client_header_val in self.__client_headers.items(): - auth_headers[client_header_name] = await resolve_value(client_header_val) - - # ID tokens contain sensitive user information (claims). Transmitting - # these over HTTP exposes the data to interception and unauthorized - # access. Always use HTTPS to ensure secure communication and protect - # user privacy. - if auth_headers and not self.__url.startswith("https://"): - warn( - "Sending ID token over HTTP. User data may be exposed. Use HTTPS for secure communication." - ) + headers[client_header_name] = await resolve_value(client_header_val) async with self.__session.post( self.__url, json=payload, - headers=auth_headers, + headers=headers, ) as resp: body = await resp.json() if resp.status < 200 or resp.status >= 300: From 545e281bcbd09f258adaa95ad8f2ca151ff0b53f Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Thu, 8 May 2025 16:19:58 +0530 Subject: [PATCH 5/6] chore: Simplify add_headers to make it sync in async client --- packages/toolbox-core/src/toolbox_core/client.py | 4 ++-- packages/toolbox-core/src/toolbox_core/sync_client.py | 9 +++------ 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/packages/toolbox-core/src/toolbox_core/client.py b/packages/toolbox-core/src/toolbox_core/client.py index 8e5ca278..cd3879ff 100644 --- a/packages/toolbox-core/src/toolbox_core/client.py +++ b/packages/toolbox-core/src/toolbox_core/client.py @@ -308,11 +308,11 @@ async def load_toolset( return tools - async def add_headers( + def add_headers( self, headers: Mapping[str, Union[Callable, Coroutine, str]] ) -> None: """ - Asynchronously Add headers to be included in each request sent through this client. + Add headers to be included in each request sent through this client. Args: headers: Headers to include in each request sent through this client. diff --git a/packages/toolbox-core/src/toolbox_core/sync_client.py b/packages/toolbox-core/src/toolbox_core/sync_client.py index 77afe900..77df8d8d 100644 --- a/packages/toolbox-core/src/toolbox_core/sync_client.py +++ b/packages/toolbox-core/src/toolbox_core/sync_client.py @@ -144,7 +144,7 @@ def load_toolset( if not self.__loop or not self.__thread: raise ValueError("Background loop or thread cannot be None.") - async_tools = asyncio.run_coroutine_threadsafe(coro, self.__loop).result() # type: ignore + async_tools = asyncio.run_coroutine_threadsafe(coro, self.__loop).result() return [ ToolboxSyncTool(async_tool, self.__loop, self.__thread) for async_tool in async_tools @@ -154,7 +154,7 @@ def add_headers( self, headers: Mapping[str, Union[Callable, Coroutine, str]] ) -> None: """ - Synchronously Add headers to be included in each request sent through this client. + Add headers to be included in each request sent through this client. Args: headers: Headers to include in each request sent through this client. @@ -162,10 +162,7 @@ def add_headers( Raises: ValueError: If any of the headers are already registered in the client. """ - coro = self.__async_client.add_headers(headers) - - # We have already created a new loop in the init method in case it does not already exist - asyncio.run_coroutine_threadsafe(coro, self.__loop).result() # type: ignore + self.__async_client.add_headers(headers) def __enter__(self): """Enter the runtime context related to this client instance.""" From a6a24b80ffeffa7d3a32acdbe5663af12f523527 Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Thu, 8 May 2025 16:24:10 +0530 Subject: [PATCH 6/6] chore: Fix unit tests --- packages/toolbox-core/tests/test_client.py | 2 +- .../toolbox-core/tests/test_sync_client.py | 44 +++++++++++++------ 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/packages/toolbox-core/tests/test_client.py b/packages/toolbox-core/tests/test_client.py index b57624b7..1b36fe0d 100644 --- a/packages/toolbox-core/tests/test_client.py +++ b/packages/toolbox-core/tests/test_client.py @@ -1443,7 +1443,7 @@ async def test_add_headers_success( ) async with ToolboxClient(TEST_BASE_URL) as client: - await client.add_headers(static_header) + client.add_headers(static_header) assert client._ToolboxClient__client_headers == static_header tool = await client.load_tool(tool_name) diff --git a/packages/toolbox-core/tests/test_sync_client.py b/packages/toolbox-core/tests/test_sync_client.py index 47f3a722..4b43e466 100644 --- a/packages/toolbox-core/tests/test_sync_client.py +++ b/packages/toolbox-core/tests/test_sync_client.py @@ -15,7 +15,7 @@ import inspect from typing import Any, Callable, Mapping, Optional -from unittest.mock import AsyncMock, patch +from unittest.mock import AsyncMock, Mock, patch import pytest from aioresponses import CallbackResult, aioresponses @@ -44,8 +44,12 @@ def sync_client_environment(): # This ensures any client created will start a new loop/thread. # Ensure no loop/thread is running from a previous misbehaving test or setup - assert original_loop is None or not original_loop.is_running() - assert original_thread is None or not original_thread.is_alive() + if original_loop and original_loop.is_running(): + original_loop.call_soon_threadsafe(original_loop.stop) + if original_thread and original_thread.is_alive(): + original_thread.join(timeout=5) + ToolboxSyncClient._ToolboxSyncClient__loop = None + ToolboxSyncClient._ToolboxSyncClient__thread = None ToolboxSyncClient._ToolboxSyncClient__loop = None ToolboxSyncClient._ToolboxSyncClient__thread = None @@ -408,20 +412,32 @@ def post_callback(url, **kwargs): result = tool(param1="test") assert result == expected_payload["result"] - @pytest.mark.usefixtures("sync_client_environment") def test_sync_add_headers_duplicate_fail(self): - """ - Tests that adding a duplicate header via add_headers raises ValueError. - Manually create client to control initial headers. - """ + """Tests that adding a duplicate header via add_headers raises ValueError (from async client).""" initial_headers = {"X-Initial-Header": "initial_value"} + mock_async_client = AsyncMock(spec=ToolboxClient) - with ToolboxSyncClient(TEST_BASE_URL, client_headers=initial_headers) as client: - with pytest.raises( - ValueError, - match="Client header\\(s\\) `X-Initial-Header` already registered", - ): - client.add_headers({"X-Initial-Header": "another_value"}) + # Configure add_headers to simulate the ValueError from ToolboxClient + def mock_add_headers(headers): + # Simulate ToolboxClient's check + if "X-Initial-Header" in headers: + raise ValueError( + "Client header(s) `X-Initial-Header` already registered" + ) + + mock_async_client.add_headers = Mock(side_effect=mock_add_headers) + + with patch( + "toolbox_core.sync_client.ToolboxClient", return_value=mock_async_client + ): + with ToolboxSyncClient( + TEST_BASE_URL, client_headers=initial_headers + ) as client: + with pytest.raises( + ValueError, + match="Client header\\(s\\) `X-Initial-Header` already registered", + ): + client.add_headers({"X-Initial-Header": "another_value"}) class TestSyncAuth: