fix(security): sanitize REST path parameters to prevent injection attacks#1115
fix(security): sanitize REST path parameters to prevent injection attacks#1115Linux2010 wants to merge 3 commits into
Conversation
…acks (a2aproject#805) Add sanitize_path_id() validation for task IDs and push notification config IDs used in REST URL paths. Rejects null bytes, control characters, path traversal sequences, and other unsafe inputs. Server-side: validate all path_params in RestDispatcher before passing to request handlers. Client-side: validate request.id and request.task_id before constructing URL paths in RestTransport. Closes a2aproject#805
There was a problem hiding this comment.
Code Review
This pull request introduces path parameter sanitization via a new sanitize_path_id utility to prevent injection of control characters and path traversal sequences across REST client and server dispatcher layers. The review feedback correctly identifies a security vulnerability where single dot (.) and double dot (..) are permitted by the sanitization regex, which could lead to path traversal or routing bypass when interpolated into URL paths. It is recommended to explicitly reject these values and add corresponding unit tests.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if not value: | ||
| raise InvalidRequestError( | ||
| message=f'{param_name} must not be empty', | ||
| ) |
There was a problem hiding this comment.
The current sanitization logic allows single dot (.) and double dot (..) as valid path IDs because they match the _PATH_ID_PATTERN regex (^[A-Za-z0-9._-]+$). While forward slashes are rejected, allowing . and .. can still lead to path traversal or routing bypass/normalization issues when interpolated into URL paths (e.g., /tasks/.. or /tasks/../pushNotificationConfigs).
We should explicitly reject . and .. to prevent these security risks.
| if not value: | |
| raise InvalidRequestError( | |
| message=f'{param_name} must not be empty', | |
| ) | |
| if not value: | |
| raise InvalidRequestError( | |
| message=f'{param_name} must not be empty', | |
| ) | |
| if value in ('.', '..'): | |
| raise InvalidRequestError( | |
| message=f'{param_name} cannot be "." or ".."', | |
| ) |
References
- For unsupported input values, fail explicitly by raising an error instead of silently falling back to a default value.
| def test_path_traversal_rejected(self) -> None: | ||
| """Path traversal sequence raises InvalidRequestError.""" | ||
| with pytest.raises(InvalidRequestError, match='invalid characters'): | ||
| sanitize_path_id('../../etc/passwd') |
There was a problem hiding this comment.
Add unit tests to verify that single dot (.) and double dot (..) are correctly rejected by sanitize_path_id to prevent path traversal risks.
| def test_path_traversal_rejected(self) -> None: | |
| """Path traversal sequence raises InvalidRequestError.""" | |
| with pytest.raises(InvalidRequestError, match='invalid characters'): | |
| sanitize_path_id('../../etc/passwd') | |
| def test_path_traversal_rejected(self) -> None: | |
| """Path traversal sequence raises InvalidRequestError.""" | |
| with pytest.raises(InvalidRequestError, match='invalid characters'): | |
| sanitize_path_id('../../etc/passwd') | |
| def test_dot_and_double_dot_rejected(self) -> None: | |
| """Single dot and double dot are rejected as path traversal risks.""" | |
| with pytest.raises(InvalidRequestError, match='cannot be "\\." or "\\.\\."'): | |
| sanitize_path_id('.') | |
| with pytest.raises(InvalidRequestError, match='cannot be "\\." or "\\.\\."'): | |
| sanitize_path_id('..') |
🧪 Code Coverage (vs
|
| Base | PR | Delta | |
|---|---|---|---|
| src/a2a/server/events/event_queue_v2.py | 91.19% | 91.71% | 🟢 +0.52% |
| src/a2a/utils/telemetry.py | 90.70% | 91.47% | 🟢 +0.78% |
| src/a2a/utils/sanitizers.py (new) | — | 100.00% | — |
| Total | 92.99% | 93.03% | 🟢 +0.04% |
Generated by coverage-comment.yml
Addresses gemini-code-assist review on PR a2aproject#1115: - Add explicit check for bare '.' and '..' in sanitize_path_id() - Add unit tests for dot and double-dot rejection
Summary
Fixes #805 — REST task/request ID sanitization.
REST transport path parameters (
task_id,push_id) were directly interpolated into URL paths without validation. Starlette accepts null bytes (\x00), newlines (\n), and other control characters in path parameters, which could lead to injection attacks.Changes
New:
src/a2a/utils/sanitizers.pysanitize_path_id(value, param_name)— validates path parameter values against a safe character set[A-Za-z0-9._-], rejecting:../../)Modified:
src/a2a/server/routes/rest_dispatcher.pyrequest.path_params["id"]andrequest.path_params["push_id"]usages now go throughsanitize_path_id()before being passed to request handlersModified:
src/a2a/client/transports/rest.pyrequest.idandrequest.task_idURL interpolations now go throughsanitize_path_id()before constructing URL pathsNew:
tests/utils/test_sanitizers.pyModified:
tests/server/routes/test_rest_dispatcher.pyTest Results
Security Impact
GET /tasks/abc%00def→ accepted, null byte passed to task storeGET /tasks/abc%00def→ HTTP 400,InvalidRequestError: id contains control characters