Skip to content

fix(security): sanitize REST path parameters to prevent injection attacks - #1115

Open
Linux2010 wants to merge 4 commits into
a2aproject:mainfrom
Linux2010:fix/805-rest-path-id-sanitization
Open

fix(security): sanitize REST path parameters to prevent injection attacks#1115
Linux2010 wants to merge 4 commits into
a2aproject:mainfrom
Linux2010:fix/805-rest-path-id-sanitization

Conversation

@Linux2010

@Linux2010 Linux2010 commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

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.py

  • sanitize_path_id(value, param_name) — validates path parameter values against a safe character set [A-Za-z0-9._-], rejecting:
    • Empty strings
    • Null bytes and control characters (0x00-0x1F, 0x7F)
    • Bare . and .. (path traversal risk)
    • Path traversal sequences (../../)
    • Spaces, slashes, and other unsafe characters

Modified: src/a2a/server/routes/rest_dispatcher.py

  • All request.path_params["id"] and request.path_params["push_id"] usages now go through sanitize_path_id() before being passed to request handlers

Modified: src/a2a/client/transports/rest.py

  • All request.id and request.task_id URL interpolations now go through sanitize_path_id() before constructing URL paths

New: tests/utils/test_sanitizers.py

  • 22 unit tests covering valid IDs, control characters, dot/double-dot rejection, path traversal, unicode, etc.

Modified: tests/server/routes/test_rest_dispatcher.py

  • 8 integration tests verifying malicious path parameters are rejected with HTTP 400

Test Results

  • All tests pass across Python 3.10–3.14
  • Coverage: 93% (above 88% threshold), new module at 100%
  • Lint: clean (ruff + ty)

Security Impact

  • Before: GET /tasks/abc%00def → accepted, null byte passed to task store
  • After: GET /tasks/abc%00def → HTTP 400, InvalidRequestError: id contains control characters
  • Before: GET /tasks/.. → accepted, potential path traversal
  • After: GET /tasks/.. → HTTP 400, InvalidRequestError: id cannot be "." or ".."

Review Notes

  • Gemini Code Assist flagged that . and .. were allowed by the regex — this was addressed in commit 8afe7f3 (reject bare dot/double-dot) and 2b86f44 (add corresponding tests)

@Linux2010
Linux2010 requested a review from a team as a code owner June 28, 2026 04:02

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +41 to +44
if not value:
raise InvalidRequestError(
message=f'{param_name} must not be empty',
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

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.

Suggested change
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
  1. For unsupported input values, fail explicitly by raising an error instead of silently falling back to a default value.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! This has been addressed in commit 8afe7f3 ("fix(security): reject '.' and '..' as path IDs to prevent path traversal"), which adds an explicit check for bare . and .. before the regex match. These values now raise InvalidRequestError: id cannot be "." or "..".

Comment on lines +78 to +81
def test_path_traversal_rejected(self) -> None:
"""Path traversal sequence raises InvalidRequestError."""
with pytest.raises(InvalidRequestError, match='invalid characters'):
sanitize_path_id('../../etc/passwd')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

low

Add unit tests to verify that single dot (.) and double dot (..) are correctly rejected by sanitize_path_id to prevent path traversal risks.

Suggested change
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('..')

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Added in commit 2b86f44 ("test: add dot/double-dot rejection tests for sanitize_path_id"). The test suite now includes test_dot_rejected, test_double_dot_rejected, and test_dot_and_double_dot_rejected covering this case.

@github-actions

github-actions Bot commented Jun 28, 2026

Copy link
Copy Markdown

🧪 Code Coverage (vs main)

⬇️ Download Full Report

Base PR Delta
src/a2a/utils/sanitizers.py (new) 100.00%
Total 92.97% 92.99% 🟢 +0.02%

Generated by coverage-comment.yml

Linux2010 added a commit to Linux2010/a2a-python that referenced this pull request Jun 28, 2026
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
Linux2010 added a commit to Linux2010/a2a-python that referenced this pull request Jul 4, 2026
Addresses CR feedback on a2aproject#1115: explicitly test that '.' and '..'
are rejected by sanitize_path_id to prevent path traversal risks.
…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
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
Addresses CR feedback on a2aproject#1115: explicitly test that '.' and '..'
are rejected by sanitize_path_id to prevent path traversal risks.
@Linux2010
Linux2010 force-pushed the fix/805-rest-path-id-sanitization branch from c2f3b4a to 2b86f44 Compare July 26, 2026 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: REST task/request id sanitization

1 participant