Skip to content

Commit babc3f5

Browse files
fix: reject invalid REST boolean query values (#1383)
# Description `GET /tasks?includeArtifacts=garbage` currently returns 200 and treats the value as `false`, silently omitting artifacts. Reject unrecognized boolean query values with the existing `400 INVALID_ARGUMENT` response before calling the request handler. Preserve case-insensitive `true`/`false` parsing, the omitted-parameter default, and last-value-wins behavior. Regression tests cover public REST requests and parameter parsing, including malformed values, artifact responses, and error messages that do not echo the supplied value. - [x] Follow the [CONTRIBUTING guide](https://github.com/a2aproject/a2a-python/blob/main/CONTRIBUTING.md). - [x] Use a Conventional Commits title. - [x] Tests and linter pass. - [x] No documentation change is needed. Validation: - `./scripts/lint.sh` - Full suite: 2358 passed, 199 skipped, 3 xfailed, 1 xpassed (192 warnings). - Coverage: 92.33%, above the 88% requirement. The local full-suite and coverage runs used a temporary helper that continuously drains the cross-version fixture servers' output to avoid blocked pipes. The fixture assertions and timeouts were unchanged. Co-authored-by: mykytanetipa <mykytanetipa@google.com>
1 parent 9c90c48 commit babc3f5

3 files changed

Lines changed: 101 additions & 2 deletions

File tree

‎src/a2a/utils/proto_utils.py‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323

2424
from google.api.field_behavior_pb2 import FieldBehavior, field_behavior
2525
from google.protobuf.descriptor import FieldDescriptor
26-
from google.protobuf.json_format import Parse, ParseDict
26+
from google.protobuf.json_format import Parse, ParseDict, ParseError
2727
from google.protobuf.message import Message as ProtobufMessage
2828
from google.rpc import error_details_pb2
2929

@@ -225,6 +225,10 @@ def parse_params(params: QueryParams, message: ProtobufMessage) -> None:
225225
if raw_val is not None:
226226
parsed_val: Any = raw_val
227227
if field.type == field.TYPE_BOOL and isinstance(raw_val, str):
228+
if raw_val.lower() not in ('true', 'false'):
229+
raise ParseError(
230+
f'Invalid boolean query parameter {k}: expected "true" or "false".'
231+
)
228232
parsed_val = raw_val.lower() == 'true'
229233
processed[k] = parsed_val
230234

‎tests/server/routes/test_rest_dispatcher.py‎

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
from collections.abc import AsyncIterator
44
from unittest.mock import AsyncMock, MagicMock
55

6+
import httpx
67
import pytest
78

89
from a2a.server.context import ServerCallContext
@@ -11,9 +12,11 @@
1112
from a2a.server.routes.rest_dispatcher import (
1213
RestDispatcher,
1314
)
15+
from a2a.server.routes.rest_routes import create_rest_routes
1416
from a2a.types.a2a_pb2 import (
1517
AgentCapabilities,
1618
AgentCard,
19+
Artifact,
1720
ListTaskPushNotificationConfigsResponse,
1821
ListTasksResponse,
1922
Message,
@@ -23,6 +26,7 @@
2326
from a2a.utils.errors import (
2427
UnsupportedOperationError,
2528
)
29+
from starlette.applications import Starlette
2630
from starlette.requests import Request
2731
from starlette.responses import JSONResponse
2832

@@ -225,6 +229,21 @@ async def test_list_tasks(self, rest_dispatcher_instance, mock_handler):
225229
True,
226230
id='included',
227231
),
232+
pytest.param(
233+
QueryParams({'includeArtifacts': 'false'}),
234+
False,
235+
id='excluded',
236+
),
237+
pytest.param(
238+
QueryParams({'includeArtifacts': 'TRUE'}),
239+
True,
240+
id='uppercase-true',
241+
),
242+
pytest.param(
243+
QueryParams({'includeArtifacts': 'False'}),
244+
False,
245+
id='mixed-case-false',
246+
),
228247
],
229248
)
230249
async def test_list_tasks_artifact_presence(
@@ -235,14 +254,46 @@ async def test_list_tasks_artifact_presence(
235254
artifacts_expected: bool,
236255
) -> None:
237256
mock_handler.on_list_tasks.return_value = ListTasksResponse(
238-
tasks=[Task(id='test_task')]
257+
tasks=[
258+
Task(
259+
id='test_task',
260+
artifacts=[Artifact(artifact_id='test-artifact')],
261+
)
262+
]
239263
)
240264
req = make_mock_request(method='GET', query_params=query_params)
241265

242266
response = await rest_dispatcher_instance.list_tasks(req)
243267

244268
task = json.loads(response.body)['tasks'][0]
245269
assert ('artifacts' in task) is artifacts_expected
270+
if artifacts_expected:
271+
assert task['artifacts'][0]['artifactId'] == 'test-artifact'
272+
273+
@pytest.mark.parametrize(
274+
'value', ['garbage', '', '0', '1', ' ', ' true ', 'private-query-value']
275+
)
276+
async def test_list_tasks_rejects_invalid_boolean_query(
277+
self, mock_handler: AsyncMock, value: str
278+
) -> None:
279+
app = Starlette(routes=create_rest_routes(mock_handler))
280+
async with httpx.AsyncClient(
281+
transport=httpx.ASGITransport(app=app),
282+
base_url='http://test',
283+
trust_env=False,
284+
) as client:
285+
response = await client.get(
286+
'/tasks',
287+
params={'includeArtifacts': value},
288+
headers={'a2a-version': '1.0'},
289+
)
290+
291+
assert response.status_code == 400
292+
assert response.json()['error']['status'] == 'INVALID_ARGUMENT'
293+
assert 'includeArtifacts' in response.json()['error']['message']
294+
if value.strip():
295+
assert value not in response.json()['error']['message']
296+
mock_handler.on_list_tasks.assert_not_awaited()
246297

247298
async def test_get_push_notification(
248299
self, rest_dispatcher_instance, mock_handler

‎tests/utils/test_proto_utils.py‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,50 @@ class FakeOldFieldOptional:
222222
class TestRestParams:
223223
"""Unit tests for REST parameter conversion."""
224224

225+
@pytest.mark.parametrize(
226+
('values', 'expected'),
227+
[
228+
([], None),
229+
(['true'], True),
230+
(['false'], False),
231+
(['TRUE'], True),
232+
(['False'], False),
233+
(['true', 'false'], False),
234+
(['false', 'true'], True),
235+
(['garbage', 'true'], True),
236+
],
237+
)
238+
def test_boolean_query_values(
239+
self, values: list[str], expected: bool | None
240+
) -> None:
241+
message = ListTasksRequest()
242+
243+
proto_utils.parse_params(
244+
QueryParams([('includeArtifacts', value) for value in values]),
245+
message,
246+
)
247+
248+
assert message.HasField('include_artifacts') is (expected is not None)
249+
assert message.include_artifacts is bool(expected)
250+
251+
@pytest.mark.parametrize(
252+
'value', ['garbage', '', '0', '1', ' ', ' true ', 'private-query-value']
253+
)
254+
@pytest.mark.parametrize('prefix', [[], ['true']])
255+
def test_invalid_boolean_query_values(
256+
self, value: str, prefix: list[str]
257+
) -> None:
258+
with pytest.raises(ParseError, match='includeArtifacts') as error:
259+
proto_utils.parse_params(
260+
QueryParams(
261+
[('includeArtifacts', item) for item in [*prefix, value]]
262+
),
263+
ListTasksRequest(),
264+
)
265+
266+
if value.strip():
267+
assert value not in str(error.value)
268+
225269
def test_rest_params_roundtrip(self):
226270
"""Test the comprehensive roundtrip conversion for REST parameters."""
227271

0 commit comments

Comments
 (0)