Skip to content

Commit 0473a5b

Browse files
committed
refactor(mcp): remove dead code from _internal.py and update tests
Remove unused _validate_header_value, _is_printable_ascii, _is_control_char, _is_whitespace, _get_forbidden_char_desc, and _HEADER_WHITESPACE constant from _internal.py. Update tests to cover the actual provider code path instead of the removed internal function.
1 parent 5aa163f commit 0473a5b

2 files changed

Lines changed: 18 additions & 138 deletions

File tree

src/google/adk/tools/mcp_tool/_internal.py

Lines changed: 0 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -21,23 +21,18 @@
2121
2222
- Header validation implements RFC 7230 §3.2 for proper HTTP header format
2323
- Only truly dangerous control characters are removed from header values
24-
- Legitimate multi-line headers with proper folding are preserved
25-
- Binary data handling is separate from text data for security
2624
- All functions log security-relevant warnings when appropriate
2725
2826
**RFC 7230 Compliance:**
2927
3028
- Header names: only letters, digits, and hyphens allowed
3129
- Header values: control characters (0x00-0x1F, 0x7F) are dangerous
32-
- Header folding: CRLF sequences are preserved for legitimate use cases
33-
- Binary data: handled separately with explicit allow_binary flag
3430
3531
**Attack Prevention:**
3632
3733
- HTTP header injection attacks via control character filtering
3834
- Response splitting attacks through CRLF handling
3935
- Log injection attacks via character sanitization
40-
- Type confusion attacks through strict validation
4136
"""
4237

4338
from __future__ import annotations
@@ -48,9 +43,6 @@
4843

4944
logger = logging.getLogger("google_adk." + __name__)
5045

51-
# Header whitespace characters (RFC 7230 §3.2.4)
52-
_HEADER_WHITESPACE = "\r\n"
53-
5446
# RFC 7230 compliant header name pattern (allows letters, digits, hyphens)
5547
_HEADER_NAME_PATTERN = re.compile(r"^[a-zA-Z0-9-]+\Z")
5648

@@ -90,38 +82,6 @@
9082
}
9183

9284

93-
def _is_printable_ascii(char: str) -> bool:
94-
"""Check if character is printable ASCII."""
95-
try:
96-
return 0x20 <= ord(char) <= 0x7E
97-
except ValueError:
98-
return False
99-
100-
101-
def _is_control_char(char: str) -> bool:
102-
"""Check if character is a control character."""
103-
return char in _DANGEROUS_CHARS
104-
105-
106-
def _is_whitespace(char: str) -> bool:
107-
"""Check if character is whitespace."""
108-
return char in _HEADER_WHITESPACE
109-
110-
111-
def _get_forbidden_char_desc(char: str) -> str:
112-
"""Get description of forbidden character."""
113-
if char == "\r":
114-
return "carriage return"
115-
elif char == "\n":
116-
return "line feed"
117-
elif char == "\t":
118-
return "horizontal tab"
119-
elif _is_printable_ascii(char):
120-
return f"non-printable ASCII: {repr(char)}"
121-
else:
122-
return f"control character: {repr(char)}"
123-
124-
12585
def validate_header_name(header_name: str) -> None:
12686
"""Validates that a header name conforms to RFC 7230.
12787
Only allows printable ASCII, no control chars, spaces, or separators.
@@ -158,53 +118,6 @@ def validate_header_format(header_format: str) -> None:
158118
)
159119

160120

161-
def _validate_header_value(value: Any, allow_binary: bool = False) -> None:
162-
"""Validates header values with RFC 7230 compliance and proper binary handling.
163-
164-
Args:
165-
value: The header value to validate.
166-
allow_binary: Whether to allow binary data (bytes) in header values.
167-
168-
Raises:
169-
ValueError: If value contains dangerous characters.
170-
"""
171-
if value is None:
172-
return
173-
174-
if isinstance(value, bytes):
175-
if not allow_binary:
176-
raise ValueError("Binary data not allowed in HTTP header values")
177-
# For binary data, check for dangerous bytes
178-
for byte_val in value:
179-
if byte_val < 128: # ASCII range
180-
char = chr(byte_val)
181-
if char in _DANGEROUS_CHARS:
182-
raise ValueError(
183-
f"Binary data contains dangerous byte: {repr(char)} "
184-
f"({_get_forbidden_char_desc(char)})"
185-
)
186-
return
187-
188-
# For strings, check for dangerous characters that could enable injection
189-
if isinstance(value, str):
190-
for char in value:
191-
if char in _DANGEROUS_CHARS:
192-
raise ValueError(
193-
f"Header value contains dangerous character: {repr(char)} "
194-
f"({_get_forbidden_char_desc(char)})"
195-
)
196-
return
197-
198-
# For other types, convert to string and validate
199-
str_value = str(value)
200-
for char in str_value:
201-
if char in _DANGEROUS_CHARS:
202-
raise ValueError(
203-
"Header value (converted to string) contains dangerous character: "
204-
f"{repr(char)} ({_get_forbidden_char_desc(char)})"
205-
)
206-
207-
208121
def sanitize_header_value(value: Any) -> str:
209122
"""Sanitizes a header value to prevent injection attacks.
210123

tests/unittests/tools/mcp_tool/test_jwt_token_propagation.py

Lines changed: 18 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -579,40 +579,27 @@ def test_header_value_crlf_stripped_in_provider(self):
579579
headers = provider(mock_context)
580580
assert headers == {"Authorization": "Bearer clean-token-123"}
581581

582-
def test_header_value_validation_rfc_compliant(self):
583-
"""Test that header value validation is RFC 7230 compliant."""
584-
from google.adk.tools.mcp_tool._internal import _validate_header_value
582+
def test_header_value_sanitization_in_provider(self):
583+
"""Test that header value sanitization works through the provider path."""
584+
from google.adk.tools.mcp_tool.mcp_toolset import create_session_state_header_provider
585585

586-
# Valid header values should pass validation
587-
valid_values = [
588-
"Bearer token123",
589-
"application/json",
590-
"text/html; charset=utf-8",
591-
"Basic dXNlcjpwYXNz",
592-
"multipart/form-data; boundary=something",
593-
123, # Numbers should be converted to string
594-
True, # Boolean should be converted to string
595-
45.67, # Float should be converted to string
596-
]
586+
mock_context = Mock(spec=ReadonlyContext)
597587

598-
for value in valid_values:
599-
try:
600-
_validate_header_value(value) # Should not raise
601-
except ValueError:
602-
pytest.fail(f"Valid value {value!r} should not raise ValueError")
603-
604-
# Invalid header values should raise ValueError
605-
invalid_values = [
606-
"token\x00with\x01null", # Contains control characters
607-
"data\x02with\x03control", # Contains control characters
608-
b"binary\x00data", # Binary data with null bytes
609-
# {"complex": "object"}, # Complex object (when converted to string) - REMOVED as it is valid
610-
# ["list", "data"], # List (when converted to string) - REMOVED as it is valid
611-
]
588+
# Test that dangerous characters in values get sanitized
589+
mock_context.state = {"token": "clean-token"}
590+
provider = create_session_state_header_provider(
591+
state_key="token", header_name="Authorization", header_format="{value}"
592+
)
593+
headers = provider(mock_context)
594+
assert headers == {"Authorization": "clean-token"}
612595

613-
for value in invalid_values:
614-
with pytest.raises(ValueError):
615-
_validate_header_value(value)
596+
# Test that numbers and booleans are handled correctly
597+
mock_context.state = {"count": 123}
598+
provider = create_session_state_header_provider(
599+
state_key="count", header_name="X-Count", header_format="{value}"
600+
)
601+
headers = provider(mock_context)
602+
assert headers == {"X-Count": "123"}
616603

617604
def test_session_state_header_provider_rfc_compliant(self):
618605
"""Test that session state header provider handles edge cases correctly."""
@@ -648,26 +635,6 @@ def test_session_state_header_provider_rfc_compliant(self):
648635
headers = provider(mock_context)
649636
assert headers == {"Authorization": "Bearer default-token"}
650637

651-
def test_binary_data_handling(self):
652-
"""Test proper handling of binary data in header values."""
653-
from google.adk.tools.mcp_tool._internal import _validate_header_value
654-
655-
# Binary data should be rejected by default
656-
with pytest.raises(ValueError, match="Binary data not allowed"):
657-
_validate_header_value(b"binary data")
658-
659-
# Binary data should be accepted with allow_binary=True if no dangerous bytes
660-
safe_binary = b"safe binary data"
661-
try:
662-
_validate_header_value(safe_binary, allow_binary=True)
663-
except ValueError:
664-
pytest.fail("Safe binary data should be allowed with allow_binary=True")
665-
666-
# Binary data with dangerous bytes should be rejected even with allow_binary=True
667-
dangerous_binary = b"dangerous\x00data\x01with\x02control"
668-
with pytest.raises(ValueError, match="dangerous"):
669-
_validate_header_value(dangerous_binary, allow_binary=True)
670-
671638
def test_header_format_crlf_injection_protection(self):
672639
"""Test that header format strings with CRLF sequences are rejected."""
673640
from google.adk.tools.mcp_tool._internal import validate_header_format

0 commit comments

Comments
 (0)