Skip to content

Commit d331e3c

Browse files
authored
feat: Warn on insecure tool invocation with authentication (#223)
* chore: Add unit test cases * chore: Delint * 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. * fix!: Warn about https only during tool initialization * chore: Add unit test coverage * chore: Delint
1 parent 7cde398 commit d331e3c

File tree

2 files changed

+169
-19
lines changed

2 files changed

+169
-19
lines changed

packages/toolbox-core/src/toolbox_core/tool.py

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
from inspect import Signature
1717
from types import MappingProxyType
1818
from typing import Any, Callable, Coroutine, Mapping, Optional, Sequence, Union
19+
from warnings import warn
1920

2021
from aiohttp import ClientSession
2122

@@ -118,6 +119,17 @@ def __init__(
118119
# map of client headers to their value/callable/coroutine
119120
self.__client_headers = client_headers
120121

122+
# ID tokens contain sensitive user information (claims). Transmitting
123+
# these over HTTP exposes the data to interception and unauthorized
124+
# access. Always use HTTPS to ensure secure communication and protect
125+
# user privacy.
126+
if (
127+
required_authn_params or required_authz_tokens or client_headers
128+
) and not self.__url.startswith("https://"):
129+
warn(
130+
"Sending ID token over HTTP. User data may be exposed. Use HTTPS for secure communication."
131+
)
132+
121133
@property
122134
def _name(self) -> str:
123135
return self.__name__
@@ -327,7 +339,8 @@ def add_auth_token_getters(
327339
)
328340

329341
return self.__copy(
330-
# create a read-only map for updated getters, params and tokens that are still required
342+
# create read-only values for updated getters, params and tokens
343+
# that are still required
331344
auth_service_token_getters=MappingProxyType(new_getters),
332345
required_authn_params=MappingProxyType(new_req_authn_params),
333346
required_authz_tokens=tuple(new_req_authz_tokens),

packages/toolbox-core/tests/test_tool.py

Lines changed: 155 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import inspect
1717
from typing import AsyncGenerator, Callable, Mapping
1818
from unittest.mock import AsyncMock, Mock
19+
from warnings import catch_warnings, simplefilter
1920

2021
import pytest
2122
import pytest_asyncio
@@ -27,6 +28,7 @@
2728
from toolbox_core.tool import ToolboxTool, create_func_docstring, resolve_value
2829

2930
TEST_BASE_URL = "http://toolbox.example.com"
31+
HTTPS_BASE_URL = "https://toolbox.example.com"
3032
TEST_TOOL_NAME = "sample_tool"
3133

3234

@@ -195,7 +197,7 @@ async def test_tool_creation_callable_and_run(
195197
Tests creating a ToolboxTool, checks callability, and simulates a run.
196198
"""
197199
tool_name = TEST_TOOL_NAME
198-
base_url = TEST_BASE_URL
200+
base_url = HTTPS_BASE_URL
199201
invoke_url = f"{base_url}/api/tool/{tool_name}/invoke"
200202

201203
input_args = {"message": "hello world", "count": 5}
@@ -246,7 +248,7 @@ async def test_tool_run_with_pydantic_validation_error(
246248
due to Pydantic validation *before* making an HTTP request.
247249
"""
248250
tool_name = TEST_TOOL_NAME
249-
base_url = TEST_BASE_URL
251+
base_url = HTTPS_BASE_URL
250252
invoke_url = f"{base_url}/api/tool/{tool_name}/invoke"
251253

252254
with aioresponses() as m:
@@ -340,18 +342,25 @@ async def test_resolve_value_async_callable():
340342

341343
def test_tool_init_basic(http_session, sample_tool_params, sample_tool_description):
342344
"""Tests basic tool initialization without headers or auth."""
343-
tool_instance = ToolboxTool(
344-
session=http_session,
345-
base_url=TEST_BASE_URL,
346-
name=TEST_TOOL_NAME,
347-
description=sample_tool_description,
348-
params=sample_tool_params,
349-
required_authn_params={},
350-
required_authz_tokens=[],
351-
auth_service_token_getters={},
352-
bound_params={},
353-
client_headers={},
354-
)
345+
with catch_warnings(record=True) as record:
346+
simplefilter("always")
347+
348+
tool_instance = ToolboxTool(
349+
session=http_session,
350+
base_url=HTTPS_BASE_URL,
351+
name=TEST_TOOL_NAME,
352+
description=sample_tool_description,
353+
params=sample_tool_params,
354+
required_authn_params={},
355+
required_authz_tokens=[],
356+
auth_service_token_getters={},
357+
bound_params={},
358+
client_headers={},
359+
)
360+
assert (
361+
len(record) == 0
362+
), f"ToolboxTool instantiation unexpectedly warned: {[f'{w.category.__name__}: {w.message}' for w in record]}"
363+
355364
assert tool_instance.__name__ == TEST_TOOL_NAME
356365
assert inspect.iscoroutinefunction(tool_instance.__call__)
357366
assert "message" in tool_instance.__signature__.parameters
@@ -367,7 +376,7 @@ def test_tool_init_with_client_headers(
367376
"""Tests tool initialization *with* client headers."""
368377
tool_instance = ToolboxTool(
369378
session=http_session,
370-
base_url=TEST_BASE_URL,
379+
base_url=HTTPS_BASE_URL,
371380
name=TEST_TOOL_NAME,
372381
description=sample_tool_description,
373382
params=sample_tool_params,
@@ -395,7 +404,7 @@ def test_tool_init_header_auth_conflict(
395404
):
396405
ToolboxTool(
397406
session=http_session,
398-
base_url=TEST_BASE_URL,
407+
base_url=HTTPS_BASE_URL,
399408
name="auth_conflict_tool",
400409
description=sample_tool_description,
401410
params=sample_tool_auth_params,
@@ -418,7 +427,7 @@ def test_tool_add_auth_token_getters_conflict_with_existing_client_header(
418427
"""
419428
tool_instance = ToolboxTool(
420429
session=http_session,
421-
base_url=TEST_BASE_URL,
430+
base_url=HTTPS_BASE_URL,
422431
name="tool_with_client_header",
423432
description=sample_tool_description,
424433
params=sample_tool_params,
@@ -454,7 +463,7 @@ def test_add_auth_token_getters_unused_token(
454463
"""
455464
tool_instance = ToolboxTool(
456465
session=http_session,
457-
base_url=TEST_BASE_URL,
466+
base_url=HTTPS_BASE_URL,
458467
name=TEST_TOOL_NAME,
459468
description=sample_tool_description,
460469
params=sample_tool_params,
@@ -469,3 +478,131 @@ def test_add_auth_token_getters_unused_token(
469478

470479
with pytest.raises(ValueError, match=expected_error_message):
471480
tool_instance.add_auth_token_getters(unused_auth_getters)
481+
482+
483+
# --- Test for the HTTP Warning ---
484+
@pytest.mark.parametrize(
485+
"trigger_condition_params",
486+
[
487+
{"client_headers": {"X-Some-Header": "value"}},
488+
{"required_authn_params": {"param1": ["auth-service1"]}},
489+
{"required_authz_tokens": ["auth-service2"]},
490+
{
491+
"client_headers": {"X-Some-Header": "value"},
492+
"required_authn_params": {"param1": ["auth-service1"]},
493+
},
494+
{
495+
"client_headers": {"X-Some-Header": "value"},
496+
"required_authz_tokens": ["auth-service2"],
497+
},
498+
{
499+
"required_authn_params": {"param1": ["auth-service1"]},
500+
"required_authz_tokens": ["auth-service2"],
501+
},
502+
{
503+
"client_headers": {"X-Some-Header": "value"},
504+
"required_authn_params": {"param1": ["auth-service1"]},
505+
"required_authz_tokens": ["auth-service2"],
506+
},
507+
],
508+
ids=[
509+
"client_headers_only",
510+
"authn_params_only",
511+
"authz_tokens_only",
512+
"headers_and_authn",
513+
"headers_and_authz",
514+
"authn_and_authz",
515+
"all_three_conditions",
516+
],
517+
)
518+
def test_tool_init_http_warning_when_sensitive_info_over_http(
519+
http_session: ClientSession,
520+
sample_tool_params: list[ParameterSchema],
521+
sample_tool_description: str,
522+
trigger_condition_params: dict,
523+
):
524+
"""
525+
Tests that a UserWarning is issued if client headers, auth params, or
526+
auth tokens are present and the base_url is HTTP.
527+
"""
528+
expected_warning_message = (
529+
"Sending ID token over HTTP. User data may be exposed. "
530+
"Use HTTPS for secure communication."
531+
)
532+
533+
init_kwargs = {
534+
"session": http_session,
535+
"base_url": TEST_BASE_URL,
536+
"name": "http_warning_tool",
537+
"description": sample_tool_description,
538+
"params": sample_tool_params,
539+
"required_authn_params": {},
540+
"required_authz_tokens": [],
541+
"auth_service_token_getters": {},
542+
"bound_params": {},
543+
"client_headers": {},
544+
}
545+
# Apply the specific conditions for this parametrized test
546+
init_kwargs.update(trigger_condition_params)
547+
548+
with pytest.warns(UserWarning, match=expected_warning_message):
549+
ToolboxTool(**init_kwargs)
550+
551+
552+
def test_tool_init_no_http_warning_if_https(
553+
http_session: ClientSession,
554+
sample_tool_params: list[ParameterSchema],
555+
sample_tool_description: str,
556+
static_client_header: dict,
557+
):
558+
"""
559+
Tests that NO UserWarning is issued if client headers are present but
560+
the base_url is HTTPS.
561+
"""
562+
with catch_warnings(record=True) as record:
563+
simplefilter("always")
564+
565+
ToolboxTool(
566+
session=http_session,
567+
base_url=HTTPS_BASE_URL,
568+
name="https_tool",
569+
description=sample_tool_description,
570+
params=sample_tool_params,
571+
required_authn_params={},
572+
required_authz_tokens=[],
573+
auth_service_token_getters={},
574+
bound_params={},
575+
client_headers=static_client_header,
576+
)
577+
assert (
578+
len(record) == 0
579+
), f"Expected no warnings, but got: {[f'{w.category.__name__}: {w.message}' for w in record]}"
580+
581+
582+
def test_tool_init_no_http_warning_if_no_sensitive_info_on_http(
583+
http_session: ClientSession,
584+
sample_tool_params: list[ParameterSchema],
585+
sample_tool_description: str,
586+
):
587+
"""
588+
Tests that NO UserWarning is issued if the URL is HTTP but there are
589+
no client headers, auth params, or auth tokens.
590+
"""
591+
with catch_warnings(record=True) as record:
592+
simplefilter("always")
593+
594+
ToolboxTool(
595+
session=http_session,
596+
base_url=TEST_BASE_URL,
597+
name="http_tool_no_sensitive",
598+
description=sample_tool_description,
599+
params=sample_tool_params,
600+
required_authn_params={},
601+
required_authz_tokens=[],
602+
auth_service_token_getters={},
603+
bound_params={},
604+
client_headers={},
605+
)
606+
assert (
607+
len(record) == 0
608+
), f"Expected no warnings, but got: {[f'{w.category.__name__}: {w.message}' for w in record]}"

0 commit comments

Comments
 (0)