From 5c5ea26744bef3d377580b2ecb4a643df757e1e3 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 60a45df41deaa246970b2dbbe6058ae1ccde18ac 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 --- .../toolbox-core/src/toolbox_core/tool.py | 3 +- packages/toolbox-core/tests/test_client.py | 29 ------------------- 2 files changed, 2 insertions(+), 30 deletions(-) diff --git a/packages/toolbox-core/src/toolbox_core/tool.py b/packages/toolbox-core/src/toolbox_core/tool.py index 2bbb1a3f..23f15033 100644 --- a/packages/toolbox-core/src/toolbox_core/tool.py +++ b/packages/toolbox-core/src/toolbox_core/tool.py @@ -327,7 +327,8 @@ def add_auth_token_getters( ) return self.__copy( - # create a read-only map for updated getters, params and tokens that are still required + # create read-only values for updated getters, params and tokens + # that are still required auth_service_token_getters=MappingProxyType(new_getters), required_authn_params=MappingProxyType(new_req_authn_params), required_authz_tokens=tuple(new_req_authz_tokens), 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 0f45b691a4052191893233d40ef05fb329a99e61 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 | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/packages/toolbox-core/src/toolbox_core/tool.py b/packages/toolbox-core/src/toolbox_core/tool.py index 23f15033..c78127fc 100644 --- a/packages/toolbox-core/src/toolbox_core/tool.py +++ b/packages/toolbox-core/src/toolbox_core/tool.py @@ -16,6 +16,7 @@ from inspect import Signature from types import MappingProxyType from typing import Any, Callable, Coroutine, Mapping, Optional, Sequence, Union +from warnings import warn from aiohttp import ClientSession @@ -245,18 +246,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 5ff4b7b22896680a03d4f6efb8eb4c57ae60697f 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 --- .../toolbox-core/src/toolbox_core/tool.py | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/packages/toolbox-core/src/toolbox_core/tool.py b/packages/toolbox-core/src/toolbox_core/tool.py index c78127fc..98d04e17 100644 --- a/packages/toolbox-core/src/toolbox_core/tool.py +++ b/packages/toolbox-core/src/toolbox_core/tool.py @@ -119,6 +119,17 @@ def __init__( # map of client headers to their value/callable/coroutine self.__client_headers = client_headers + # 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 ( + required_authn_params or required_authz_tokens or client_headers + ) and not self.__url.startswith("https://"): + warn( + "Sending ID token over HTTP. User data may be exposed. Use HTTPS for secure communication." + ) + @property def _name(self) -> str: return self.__name__ @@ -246,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 3bb4df71f8a607ccee67e04c930dae3087f0dc75 Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Tue, 13 May 2025 15:57:56 +0530 Subject: [PATCH 5/6] chore: Add unit test coverage --- packages/toolbox-core/tests/test_tool.py | 166 ++++++++++++++++++++--- 1 file changed, 148 insertions(+), 18 deletions(-) diff --git a/packages/toolbox-core/tests/test_tool.py b/packages/toolbox-core/tests/test_tool.py index 4b723883..17ebb871 100644 --- a/packages/toolbox-core/tests/test_tool.py +++ b/packages/toolbox-core/tests/test_tool.py @@ -22,11 +22,13 @@ from aiohttp import ClientSession from aioresponses import aioresponses from pydantic import ValidationError +from warnings import catch_warnings, simplefilter from toolbox_core.protocol import ParameterSchema from toolbox_core.tool import ToolboxTool, create_func_docstring, resolve_value TEST_BASE_URL = "http://toolbox.example.com" +HTTPS_BASE_URL = "https://toolbox.example.com" TEST_TOOL_NAME = "sample_tool" @@ -195,7 +197,7 @@ async def test_tool_creation_callable_and_run( Tests creating a ToolboxTool, checks callability, and simulates a run. """ tool_name = TEST_TOOL_NAME - base_url = TEST_BASE_URL + base_url = HTTPS_BASE_URL invoke_url = f"{base_url}/api/tool/{tool_name}/invoke" input_args = {"message": "hello world", "count": 5} @@ -246,7 +248,7 @@ async def test_tool_run_with_pydantic_validation_error( due to Pydantic validation *before* making an HTTP request. """ tool_name = TEST_TOOL_NAME - base_url = TEST_BASE_URL + base_url = HTTPS_BASE_URL invoke_url = f"{base_url}/api/tool/{tool_name}/invoke" with aioresponses() as m: @@ -340,18 +342,23 @@ async def test_resolve_value_async_callable(): def test_tool_init_basic(http_session, sample_tool_params, sample_tool_description): """Tests basic tool initialization without headers or auth.""" - tool_instance = ToolboxTool( - session=http_session, - base_url=TEST_BASE_URL, - name=TEST_TOOL_NAME, - description=sample_tool_description, - params=sample_tool_params, - required_authn_params={}, - required_authz_tokens=[], - auth_service_token_getters={}, - bound_params={}, - client_headers={}, - ) + with catch_warnings(record=True) as record: + simplefilter("always") + + tool_instance = ToolboxTool( + session=http_session, + base_url=HTTPS_BASE_URL, + name=TEST_TOOL_NAME, + description=sample_tool_description, + params=sample_tool_params, + required_authn_params={}, + required_authz_tokens=[], + auth_service_token_getters={}, + bound_params={}, + client_headers={}, + ) + assert len(record) == 0, f"ToolboxTool instantiation unexpectedly warned: {[f'{w.category.__name__}: {w.message}' for w in record]}" + assert tool_instance.__name__ == TEST_TOOL_NAME assert inspect.iscoroutinefunction(tool_instance.__call__) assert "message" in tool_instance.__signature__.parameters @@ -367,7 +374,7 @@ def test_tool_init_with_client_headers( """Tests tool initialization *with* client headers.""" tool_instance = ToolboxTool( session=http_session, - base_url=TEST_BASE_URL, + base_url=HTTPS_BASE_URL, name=TEST_TOOL_NAME, description=sample_tool_description, params=sample_tool_params, @@ -395,7 +402,7 @@ def test_tool_init_header_auth_conflict( ): ToolboxTool( session=http_session, - base_url=TEST_BASE_URL, + base_url=HTTPS_BASE_URL, name="auth_conflict_tool", description=sample_tool_description, params=sample_tool_auth_params, @@ -418,7 +425,7 @@ def test_tool_add_auth_token_getters_conflict_with_existing_client_header( """ tool_instance = ToolboxTool( session=http_session, - base_url=TEST_BASE_URL, + base_url=HTTPS_BASE_URL, name="tool_with_client_header", description=sample_tool_description, params=sample_tool_params, @@ -454,7 +461,7 @@ def test_add_auth_token_getters_unused_token( """ tool_instance = ToolboxTool( session=http_session, - base_url=TEST_BASE_URL, + base_url=HTTPS_BASE_URL, name=TEST_TOOL_NAME, description=sample_tool_description, params=sample_tool_params, @@ -469,3 +476,126 @@ def test_add_auth_token_getters_unused_token( with pytest.raises(ValueError, match=expected_error_message): tool_instance.add_auth_token_getters(unused_auth_getters) + + +# --- Test for the HTTP Warning --- +@pytest.mark.parametrize( + "trigger_condition_params", + [ + {"client_headers": {"X-Some-Header": "value"}}, + {"required_authn_params": {"param1": ["auth-service1"]}}, + {"required_authz_tokens": ["auth-service2"]}, + { + "client_headers": {"X-Some-Header": "value"}, + "required_authn_params": {"param1": ["auth-service1"]}, + }, + { + "client_headers": {"X-Some-Header": "value"}, + "required_authz_tokens": ["auth-service2"], + }, + { + "required_authn_params": {"param1": ["auth-service1"]}, + "required_authz_tokens": ["auth-service2"], + }, + { + "client_headers": {"X-Some-Header": "value"}, + "required_authn_params": {"param1": ["auth-service1"]}, + "required_authz_tokens": ["auth-service2"], + }, + ], + ids=[ + "client_headers_only", + "authn_params_only", + "authz_tokens_only", + "headers_and_authn", + "headers_and_authz", + "authn_and_authz", + "all_three_conditions", + ], +) +def test_tool_init_http_warning_when_sensitive_info_over_http( + http_session: ClientSession, + sample_tool_params: list[ParameterSchema], + sample_tool_description: str, + trigger_condition_params: dict, +): + """ + Tests that a UserWarning is issued if client headers, auth params, or + auth tokens are present and the base_url is HTTP. + """ + expected_warning_message = ( + "Sending ID token over HTTP. User data may be exposed. " + "Use HTTPS for secure communication." + ) + + init_kwargs = { + "session": http_session, + "base_url": TEST_BASE_URL, + "name": "http_warning_tool", + "description": sample_tool_description, + "params": sample_tool_params, + "required_authn_params": {}, + "required_authz_tokens": [], + "auth_service_token_getters": {}, + "bound_params": {}, + "client_headers": {}, + } + # Apply the specific conditions for this parametrized test + init_kwargs.update(trigger_condition_params) + + with pytest.warns(UserWarning, match=expected_warning_message): + ToolboxTool(**init_kwargs) + + +def test_tool_init_no_http_warning_if_https( + http_session: ClientSession, + sample_tool_params: list[ParameterSchema], + sample_tool_description: str, + static_client_header: dict, +): + """ + Tests that NO UserWarning is issued if client headers are present but + the base_url is HTTPS. + """ + with catch_warnings(record=True) as record: + simplefilter("always") + + ToolboxTool( + session=http_session, + base_url=HTTPS_BASE_URL, + name="https_tool", + description=sample_tool_description, + params=sample_tool_params, + required_authn_params={}, + required_authz_tokens=[], + auth_service_token_getters={}, + bound_params={}, + client_headers=static_client_header, + ) + assert len(record) == 0, f"Expected no warnings, but got: {[f'{w.category.__name__}: {w.message}' for w in record]}" + +def test_tool_init_no_http_warning_if_no_sensitive_info_on_http( + http_session: ClientSession, + sample_tool_params: list[ParameterSchema], + sample_tool_description: str, +): + """ + Tests that NO UserWarning is issued if the URL is HTTP but there are + no client headers, auth params, or auth tokens. + """ + with catch_warnings(record=True) as record: + simplefilter("always") + + ToolboxTool( + session=http_session, + base_url=TEST_BASE_URL, + name="http_tool_no_sensitive", + description=sample_tool_description, + params=sample_tool_params, + required_authn_params={}, + required_authz_tokens=[], + auth_service_token_getters={}, + bound_params={}, + client_headers={}, + ) + assert len(record) == 0, f"Expected no warnings, but got: {[f'{w.category.__name__}: {w.message}' for w in record]}" From 1d8995e47c947218204778aca8199ad9df856ebc Mon Sep 17 00:00:00 2001 From: Anubhav Dhawan Date: Tue, 13 May 2025 15:59:06 +0530 Subject: [PATCH 6/6] chore: Delint --- packages/toolbox-core/tests/test_tool.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/packages/toolbox-core/tests/test_tool.py b/packages/toolbox-core/tests/test_tool.py index 17ebb871..2324b0c5 100644 --- a/packages/toolbox-core/tests/test_tool.py +++ b/packages/toolbox-core/tests/test_tool.py @@ -16,13 +16,13 @@ import inspect from typing import AsyncGenerator, Callable, Mapping from unittest.mock import AsyncMock, Mock +from warnings import catch_warnings, simplefilter import pytest import pytest_asyncio from aiohttp import ClientSession from aioresponses import aioresponses from pydantic import ValidationError -from warnings import catch_warnings, simplefilter from toolbox_core.protocol import ParameterSchema from toolbox_core.tool import ToolboxTool, create_func_docstring, resolve_value @@ -357,7 +357,9 @@ def test_tool_init_basic(http_session, sample_tool_params, sample_tool_descripti bound_params={}, client_headers={}, ) - assert len(record) == 0, f"ToolboxTool instantiation unexpectedly warned: {[f'{w.category.__name__}: {w.message}' for w in record]}" + assert ( + len(record) == 0 + ), f"ToolboxTool instantiation unexpectedly warned: {[f'{w.category.__name__}: {w.message}' for w in record]}" assert tool_instance.__name__ == TEST_TOOL_NAME assert inspect.iscoroutinefunction(tool_instance.__call__) @@ -572,7 +574,10 @@ def test_tool_init_no_http_warning_if_https( bound_params={}, client_headers=static_client_header, ) - assert len(record) == 0, f"Expected no warnings, but got: {[f'{w.category.__name__}: {w.message}' for w in record]}" + assert ( + len(record) == 0 + ), f"Expected no warnings, but got: {[f'{w.category.__name__}: {w.message}' for w in record]}" + def test_tool_init_no_http_warning_if_no_sensitive_info_on_http( http_session: ClientSession, @@ -598,4 +603,6 @@ def test_tool_init_no_http_warning_if_no_sensitive_info_on_http( bound_params={}, client_headers={}, ) - assert len(record) == 0, f"Expected no warnings, but got: {[f'{w.category.__name__}: {w.message}' for w in record]}" + assert ( + len(record) == 0 + ), f"Expected no warnings, but got: {[f'{w.category.__name__}: {w.message}' for w in record]}"