From 4908b10180ce65dbd1a0312b1dbc2d278f13869c 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 1d7aedbd..608f8530 100644 --- a/packages/toolbox-core/tests/test_client.py +++ b/packages/toolbox-core/tests/test_client.py @@ -400,6 +400,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 ad7c0e4873cff7e8dec0130c6601ad3aa2f72f17 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 608f8530..1d7aedbd 100644 --- a/packages/toolbox-core/tests/test_client.py +++ b/packages/toolbox-core/tests/test_client.py @@ -400,35 +400,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 51b9463fdc31bc0671167348e7e01164a9f517b9 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 e8d61c55b8781a07e5532ad34fe8942cead2d772 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 23a514ba50f913544445f95734a2bdee3b53da5b Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Wed, 7 May 2025 16:36:29 +0530 Subject: [PATCH 5/6] fix: Fix issue causing masking of emtpy loop or thread check while loading tool/toolset --- .../toolbox-core/src/toolbox_core/sync_client.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/packages/toolbox-core/src/toolbox_core/sync_client.py b/packages/toolbox-core/src/toolbox_core/sync_client.py index c954a920..22e708b7 100644 --- a/packages/toolbox-core/src/toolbox_core/sync_client.py +++ b/packages/toolbox-core/src/toolbox_core/sync_client.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. + import asyncio from threading import Thread from typing import Any, Callable, Coroutine, Mapping, Optional, TypeVar, Union @@ -59,7 +60,7 @@ async def create_client(): # Ignoring type since we're already checking the existence of a loop above. self.__async_client = asyncio.run_coroutine_threadsafe( - create_client(), self.__class__.__loop # type: ignore + create_client(), self.__class__.__loop ).result() def close(self): @@ -101,11 +102,10 @@ def load_tool( """ coro = self.__async_client.load_tool(name, auth_token_getters, bound_params) - # We have already created a new loop in the init method in case it does not already exist - async_tool = asyncio.run_coroutine_threadsafe(coro, self.__loop).result() # type: ignore - if not self.__loop or not self.__thread: raise ValueError("Background loop or thread cannot be None.") + + async_tool = asyncio.run_coroutine_threadsafe(coro, self.__loop).result() return ToolboxSyncTool(async_tool, self.__loop, self.__thread) def load_toolset( @@ -130,11 +130,10 @@ def load_toolset( """ coro = self.__async_client.load_toolset(name, auth_token_getters, bound_params) - # We have already created a new loop in the init method in case it does not already exist - async_tools = asyncio.run_coroutine_threadsafe(coro, self.__loop).result() # type: ignore - 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 return [ ToolboxSyncTool(async_tool, self.__loop, self.__thread) for async_tool in async_tools From f3a18658c0c31e7c993450683b3a6073fcfbffb2 Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Wed, 7 May 2025 16:40:45 +0530 Subject: [PATCH 6/6] chore: Remove unused condition in client test mocks --- packages/toolbox-core/tests/test_client.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/toolbox-core/tests/test_client.py b/packages/toolbox-core/tests/test_client.py index 1d7aedbd..b57624b7 100644 --- a/packages/toolbox-core/tests/test_client.py +++ b/packages/toolbox-core/tests/test_client.py @@ -301,10 +301,8 @@ async def client(self, aioresponses, test_tool_auth, tool_name, expected_header) # mock tool INVOKE call def require_headers(url, **kwargs): - if kwargs["headers"].get("my-auth-service_token") == expected_header: - return CallbackResult(status=200, body="{}") - else: - return CallbackResult(status=400, body="{}") + assert kwargs["headers"].get("my-auth-service_token") == expected_header + return CallbackResult(status=200, body="{}") aioresponses.post( f"{TEST_BASE_URL}/api/tool/{tool_name}/invoke",