-
Notifications
You must be signed in to change notification settings - Fork 461
feat(client): add opt-in card validation hooks #1142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,85 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||
| """Optional validators for AgentCard instances before client creation.""" | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| from __future__ import annotations | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| import ipaddress | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| from collections.abc import Callable | ||||||||||||||||||||||||||||||||||||||||||||||||
| from urllib.parse import urlparse | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| from a2a.types.a2a_pb2 import AgentCard | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| CardValidator = Callable[[AgentCard], None] | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| class InvalidAgentCardError(ValueError): | ||||||||||||||||||||||||||||||||||||||||||||||||
| """Raised when an AgentCard fails a registered validation hook.""" | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| def _iter_card_urls(card: AgentCard) -> list[str]: | ||||||||||||||||||||||||||||||||||||||||||||||||
| urls = [ | ||||||||||||||||||||||||||||||||||||||||||||||||
| interface.url | ||||||||||||||||||||||||||||||||||||||||||||||||
| for interface in card.supported_interfaces | ||||||||||||||||||||||||||||||||||||||||||||||||
| if interface.url | ||||||||||||||||||||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||||||||||||||||||||
| if card.documentation_url: | ||||||||||||||||||||||||||||||||||||||||||||||||
| urls.append(card.documentation_url) | ||||||||||||||||||||||||||||||||||||||||||||||||
| if card.icon_url: | ||||||||||||||||||||||||||||||||||||||||||||||||
| urls.append(card.icon_url) | ||||||||||||||||||||||||||||||||||||||||||||||||
| return urls | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| def reject_non_https_urls(card: AgentCard) -> None: | ||||||||||||||||||||||||||||||||||||||||||||||||
| """Reject cards whose interface or metadata URLs are not https://.""" | ||||||||||||||||||||||||||||||||||||||||||||||||
| for url in _iter_card_urls(card): | ||||||||||||||||||||||||||||||||||||||||||||||||
| if urlparse(url).scheme != 'https': | ||||||||||||||||||||||||||||||||||||||||||||||||
| raise InvalidAgentCardError( | ||||||||||||||||||||||||||||||||||||||||||||||||
| f'Card URL must use https, got: {url!r}' | ||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| def reject_private_urls(card: AgentCard) -> None: | ||||||||||||||||||||||||||||||||||||||||||||||||
| """Reject cards whose URLs use private, loopback, or link-local IP hosts.""" | ||||||||||||||||||||||||||||||||||||||||||||||||
| for url in _iter_card_urls(card): | ||||||||||||||||||||||||||||||||||||||||||||||||
| _check_not_private(url) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| def _check_not_private(url: str) -> None: | ||||||||||||||||||||||||||||||||||||||||||||||||
| parsed = urlparse(url) | ||||||||||||||||||||||||||||||||||||||||||||||||
| host = parsed.hostname | ||||||||||||||||||||||||||||||||||||||||||||||||
| if host is None: | ||||||||||||||||||||||||||||||||||||||||||||||||
| raise InvalidAgentCardError(f'URL has no hostname: {url!r}') | ||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||
| ip = ipaddress.ip_address(host) | ||||||||||||||||||||||||||||||||||||||||||||||||
| except ValueError: | ||||||||||||||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+50
to
+56
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||
| if ip.is_private or ip.is_loopback or ip.is_link_local or ip.is_reserved: | ||||||||||||||||||||||||||||||||||||||||||||||||
| raise InvalidAgentCardError( | ||||||||||||||||||||||||||||||||||||||||||||||||
| f'URL points to a private or loopback address: {url!r}' | ||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| def require_supported_interfaces(card: AgentCard) -> None: | ||||||||||||||||||||||||||||||||||||||||||||||||
| """Reject cards that do not declare at least one supported interface.""" | ||||||||||||||||||||||||||||||||||||||||||||||||
| if not card.supported_interfaces: | ||||||||||||||||||||||||||||||||||||||||||||||||
| raise InvalidAgentCardError( | ||||||||||||||||||||||||||||||||||||||||||||||||
| 'Card must declare at least one supported interface.' | ||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||
| for interface in card.supported_interfaces: | ||||||||||||||||||||||||||||||||||||||||||||||||
| if not interface.url: | ||||||||||||||||||||||||||||||||||||||||||||||||
| raise InvalidAgentCardError( | ||||||||||||||||||||||||||||||||||||||||||||||||
| 'Each supported interface must include a URL.' | ||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||
| if not interface.protocol_binding: | ||||||||||||||||||||||||||||||||||||||||||||||||
| raise InvalidAgentCardError( | ||||||||||||||||||||||||||||||||||||||||||||||||
| 'Each supported interface must include a protocol binding.' | ||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| def validate_card( | ||||||||||||||||||||||||||||||||||||||||||||||||
| card: AgentCard, validators: list[CardValidator] | None | ||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> None: | ||||||||||||||||||||||||||||||||||||||||||||||||
| """Run registered validators against an AgentCard.""" | ||||||||||||||||||||||||||||||||||||||||||||||||
| for validator in validators or []: | ||||||||||||||||||||||||||||||||||||||||||||||||
| validator(card) | ||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| """Tests for optional AgentCard validation hooks.""" | ||
|
|
||
| import httpx | ||
| import pytest | ||
|
|
||
| from a2a.client import ( | ||
| ClientConfig, | ||
| ClientFactory, | ||
| InvalidAgentCardError, | ||
| reject_non_https_urls, | ||
| reject_private_urls, | ||
| require_supported_interfaces, | ||
| ) | ||
| from a2a.types.a2a_pb2 import ( | ||
| AgentCapabilities, | ||
| AgentCard, | ||
| AgentInterface, | ||
| ) | ||
| from a2a.utils.constants import TransportProtocol | ||
|
|
||
|
|
||
| def _valid_card(**overrides) -> AgentCard: | ||
| card = AgentCard( | ||
| name='Test Agent', | ||
| description='An agent for testing.', | ||
| supported_interfaces=[ | ||
| AgentInterface( | ||
| protocol_binding=TransportProtocol.JSONRPC, | ||
| url='https://primary-url.com', | ||
| ) | ||
| ], | ||
| version='1.0.0', | ||
| capabilities=AgentCapabilities(), | ||
| skills=[], | ||
| default_input_modes=[], | ||
| default_output_modes=[], | ||
| ) | ||
| for key, value in overrides.items(): | ||
| if key == 'supported_interfaces': | ||
| card.ClearField('supported_interfaces') | ||
| card.supported_interfaces.extend(value) | ||
| else: | ||
| setattr(card, key, value) | ||
| return card | ||
|
|
||
|
|
||
| def test_require_supported_interfaces_rejects_empty_card(): | ||
| card = _valid_card() | ||
| card.ClearField('supported_interfaces') | ||
|
|
||
| with pytest.raises(InvalidAgentCardError, match='supported interface'): | ||
| require_supported_interfaces(card) | ||
|
|
||
|
|
||
| def test_reject_non_https_urls_rejects_http_interface(): | ||
| card = _valid_card( | ||
| supported_interfaces=[ | ||
| AgentInterface( | ||
| protocol_binding=TransportProtocol.JSONRPC, | ||
| url='http://insecure.example.com', | ||
| ) | ||
| ] | ||
| ) | ||
|
|
||
| with pytest.raises(InvalidAgentCardError, match='https'): | ||
| reject_non_https_urls(card) | ||
|
|
||
|
|
||
| def test_reject_private_urls_rejects_loopback(): | ||
| card = _valid_card( | ||
| supported_interfaces=[ | ||
| AgentInterface( | ||
| protocol_binding=TransportProtocol.JSONRPC, | ||
| url='https://127.0.0.1', | ||
| ) | ||
| ] | ||
| ) | ||
|
|
||
| with pytest.raises(InvalidAgentCardError, match='private or loopback'): | ||
| reject_private_urls(card) | ||
|
Comment on lines
+69
to
+80
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Parameterize the private URL rejection test to cover @pytest.mark.parametrize(
'url',
[
'https://127.0.0.1',
'https://localhost',
'https://test.localhost',
'https://my-service.local',
'https://[::1]',
],
)
def test_reject_private_urls_rejects_loopback(url):
card = _valid_card(
supported_interfaces=[
AgentInterface(
protocol_binding=TransportProtocol.JSONRPC,
url=url,
)
]
)
with pytest.raises(InvalidAgentCardError, match='private or loopback'):
reject_private_urls(card) |
||
|
|
||
|
|
||
| def test_client_factory_runs_validators_on_create(): | ||
| card = _valid_card( | ||
| supported_interfaces=[ | ||
| AgentInterface( | ||
| protocol_binding=TransportProtocol.JSONRPC, | ||
| url='http://insecure.example.com', | ||
| ) | ||
| ] | ||
| ) | ||
| factory = ClientFactory( | ||
| ClientConfig(httpx_client=httpx.AsyncClient()), | ||
| card_validators=[reject_non_https_urls], | ||
| ) | ||
|
|
||
| with pytest.raises(InvalidAgentCardError): | ||
| factory.create(card) | ||
|
|
||
|
|
||
| def test_client_factory_default_has_no_validation(): | ||
| card = _valid_card() | ||
| factory = ClientFactory(ClientConfig(httpx_client=httpx.AsyncClient())) | ||
| client = factory.create(card) | ||
| assert client is not None | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
_iter_card_urlshelper extracts URLs from the card's supported interfaces, documentation, and icon, but it misses theprovider.urlfield. If an untrusted card specifies a malicious or private URL inprovider.url, it will bypass validation. Includingcard.provider.urlensures all metadata URLs are validated. Note that we only check and appendcard.provider.urlif it is present, to handle cases where the URL might not be set.References
ServerCallContext.user) is non-optional, avoid adding redundant checks for its existence and rely on the data model's contract.