Skip to content

Commit ec63c20

Browse files
committed
hardening: tighten limits, immutability, and drive-letter detection
Bundled low-severity hardening: - Lower DEFAULT_MAX_TEMPLATE_LENGTH from 1MB to 8KB. Real templates are under 200 bytes; the old limit allowed 0.75s parse times. - Replace max_expressions with max_variables (default 256). A single {v0,v1,...,vN} expression packed arbitrarily many variables under one expression count, bypassing the limit. - Store UriTemplate internals as tuples. The dataclass is frozen but list fields were mutable via t._parts.append(), violating the immutability contract. - Coerce ResourceSecurity.exempt_params to frozenset in __post_init__ so hash() works even when callers pass a regular set. - Check drive letters against ASCII only. str.isalpha() is Unicode-aware, so is_absolute_path("Ω:foo") falsely returned True.
1 parent 6e55991 commit ec63c20

File tree

6 files changed

+64
-35
lines changed

6 files changed

+64
-35
lines changed

src/mcp/server/mcpserver/resources/templates.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,11 @@ def git_diff(range: str) -> str: ...
5454
exempt_params: Set[str] = field(default_factory=frozenset[str])
5555
"""Parameter names to skip all checks for."""
5656

57+
def __post_init__(self) -> None:
58+
# Coerce to frozenset so the dataclass stays hashable even if
59+
# callers pass a regular set.
60+
object.__setattr__(self, "exempt_params", frozenset(self.exempt_params))
61+
5762
def validate(self, params: Mapping[str, str | list[str]]) -> str | None:
5863
"""Check all parameter values against the configured policy.
5964

src/mcp/shared/path_security.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ def read_doc(path: str) -> str:
1515
return safe_join("/data/docs", path).read_text()
1616
"""
1717

18+
import string
1819
from pathlib import Path
1920

2021
__all__ = ["PathEscapeError", "contains_path_traversal", "is_absolute_path", "safe_join"]
@@ -99,8 +100,9 @@ def is_absolute_path(value: str) -> bool:
99100
return False
100101
if value[0] in ("/", "\\"):
101102
return True
102-
# Windows drive letter: C:, C:\, C:/
103-
if len(value) >= 2 and value[1] == ":" and value[0].isalpha():
103+
# Windows drive letter: C:, C:\, C:/. ASCII-only so that values
104+
# like "Ω:namespace" are not falsely rejected.
105+
if len(value) >= 2 and value[1] == ":" and value[0] in string.ascii_letters:
104106
return True
105107
return False
106108

src/mcp/shared/uri_template.py

Lines changed: 31 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,8 @@
4545
from urllib.parse import quote, unquote
4646

4747
__all__ = [
48-
"DEFAULT_MAX_EXPRESSIONS",
4948
"DEFAULT_MAX_TEMPLATE_LENGTH",
49+
"DEFAULT_MAX_VARIABLES",
5050
"DEFAULT_MAX_URI_LENGTH",
5151
"InvalidUriTemplate",
5252
"Operator",
@@ -63,8 +63,8 @@
6363
# (Percent-encoded varchars are technically allowed but unseen in practice.)
6464
_VARNAME_RE = re.compile(r"^[A-Za-z0-9_]+(?:\.[A-Za-z0-9_]+)*$")
6565

66-
DEFAULT_MAX_TEMPLATE_LENGTH = 1_000_000
67-
DEFAULT_MAX_EXPRESSIONS = 10_000
66+
DEFAULT_MAX_TEMPLATE_LENGTH = 8_192
67+
DEFAULT_MAX_VARIABLES = 256
6868
DEFAULT_MAX_URI_LENGTH = 65_536
6969

7070
# RFC 3986 reserved characters, kept unencoded by {+var} and {#var}.
@@ -284,12 +284,12 @@ class UriTemplate:
284284
"""
285285

286286
template: str
287-
_parts: list[_Part] = field(repr=False, compare=False)
288-
_variables: list[Variable] = field(repr=False, compare=False)
289-
_prefix: list[_Atom] = field(repr=False, compare=False)
287+
_parts: tuple[_Part, ...] = field(repr=False, compare=False)
288+
_variables: tuple[Variable, ...] = field(repr=False, compare=False)
289+
_prefix: tuple[_Atom, ...] = field(repr=False, compare=False)
290290
_greedy: Variable | None = field(repr=False, compare=False)
291-
_suffix: list[_Atom] = field(repr=False, compare=False)
292-
_query_variables: list[Variable] = field(repr=False, compare=False)
291+
_suffix: tuple[_Atom, ...] = field(repr=False, compare=False)
292+
_query_variables: tuple[Variable, ...] = field(repr=False, compare=False)
293293

294294
@staticmethod
295295
def is_template(value: str) -> bool:
@@ -319,17 +319,19 @@ def parse(
319319
template: str,
320320
*,
321321
max_length: int = DEFAULT_MAX_TEMPLATE_LENGTH,
322-
max_expressions: int = DEFAULT_MAX_EXPRESSIONS,
322+
max_variables: int = DEFAULT_MAX_VARIABLES,
323323
) -> UriTemplate:
324324
"""Parse a URI template string.
325325
326326
Args:
327327
template: An RFC 6570 URI template.
328328
max_length: Maximum permitted length of the template string.
329329
Guards against resource exhaustion.
330-
max_expressions: Maximum number of ``{...}`` expressions
331-
permitted. Guards against pathological inputs that could
332-
produce expensive regexes.
330+
max_variables: Maximum number of variables permitted across
331+
all expressions. Counting variables rather than
332+
``{...}`` expressions closes the gap where a single
333+
``{v0,v1,...,vN}`` expression packs arbitrarily many
334+
variables under one expression count.
333335
334336
Raises:
335337
InvalidUriTemplate: If the template is malformed, exceeds the
@@ -341,7 +343,7 @@ def parse(
341343
template=template,
342344
)
343345

344-
parts, variables = _parse(template, max_expressions=max_expressions)
346+
parts, variables = _parse(template, max_variables=max_variables)
345347

346348
# Trailing {?...}/{&...} expressions are matched leniently via
347349
# parse_qs rather than the scan: order-agnostic, partial, ignores
@@ -352,12 +354,12 @@ def parse(
352354

353355
return cls(
354356
template=template,
355-
_parts=parts,
356-
_variables=variables,
357-
_prefix=prefix,
357+
_parts=tuple(parts),
358+
_variables=tuple(variables),
359+
_prefix=tuple(prefix),
358360
_greedy=greedy,
359-
_suffix=suffix,
360-
_query_variables=query_vars,
361+
_suffix=tuple(suffix),
362+
_query_variables=tuple(query_vars),
361363
)
362364

363365
@property
@@ -680,7 +682,7 @@ def _split_query_tail(parts: list[_Part]) -> tuple[list[_Part], list[Variable]]:
680682
return parts[:split], query_vars
681683

682684

683-
def _parse(template: str, *, max_expressions: int) -> tuple[list[_Part], list[Variable]]:
685+
def _parse(template: str, *, max_variables: int) -> tuple[list[_Part], list[Variable]]:
684686
"""Split a template into an ordered sequence of literals and expressions.
685687
686688
Walks the string, alternating between collecting literal runs and
@@ -694,7 +696,6 @@ def _parse(template: str, *, max_expressions: int) -> tuple[list[_Part], list[Va
694696
"""
695697
parts: list[_Part] = []
696698
variables: list[Variable] = []
697-
expression_count = 0
698699
i = 0
699700
n = len(template)
700701

@@ -719,18 +720,17 @@ def _parse(template: str, *, max_expressions: int) -> tuple[list[_Part], list[Va
719720
position=brace,
720721
)
721722

722-
expression_count += 1
723-
if expression_count > max_expressions:
724-
raise InvalidUriTemplate(
725-
f"Template exceeds maximum of {max_expressions} expressions",
726-
template=template,
727-
)
728-
729723
# Delegate body (between braces, exclusive) to the expression parser.
730724
expr = _parse_expression(template, template[brace + 1 : end], brace)
731725
parts.append(expr)
732726
variables.extend(expr.variables)
733727

728+
if len(variables) > max_variables:
729+
raise InvalidUriTemplate(
730+
f"Template exceeds maximum of {max_variables} variables",
731+
template=template,
732+
)
733+
734734
# Advance past the closing brace.
735735
i = end + 1
736736

@@ -903,7 +903,7 @@ def _partition_greedy(atoms: list[_Atom], template: str) -> tuple[list[_Atom], V
903903
return atoms[:greedy_idx], greedy.var, atoms[greedy_idx + 1 :]
904904

905905

906-
def _scan_suffix(atoms: list[_Atom], uri: str, end: int) -> tuple[dict[str, str | list[str]], int] | None:
906+
def _scan_suffix(atoms: Sequence[_Atom], uri: str, end: int) -> tuple[dict[str, str | list[str]], int] | None:
907907
"""Scan atoms right-to-left from ``end``, returning captures and start position.
908908
909909
Each bounded variable takes the minimum span that lets its
@@ -973,7 +973,9 @@ def _scan_suffix(atoms: list[_Atom], uri: str, end: int) -> tuple[dict[str, str
973973
return result, pos
974974

975975

976-
def _scan_prefix(atoms: list[_Atom], uri: str, start: int, limit: int) -> tuple[dict[str, str | list[str]], int] | None:
976+
def _scan_prefix(
977+
atoms: Sequence[_Atom], uri: str, start: int, limit: int
978+
) -> tuple[dict[str, str | list[str]], int] | None:
977979
"""Scan atoms left-to-right from ``start``, not exceeding ``limit``.
978980
979981
Each bounded variable takes the minimum span that lets its

tests/server/mcpserver/resources/test_resource_template.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,14 @@ def test_matches_null_byte_check_can_be_disabled():
8989
assert t.matches("file://docs/key%00.txt") == {"name": "key\x00.txt"}
9090

9191

92+
def test_resource_security_hashable_with_regular_set():
93+
# Frozen dataclass auto-generates __hash__ from all fields, so a
94+
# mutable set would make the instance unhashable. __post_init__
95+
# coerces to frozenset.
96+
policy = ResourceSecurity(exempt_params={"a", "b"})
97+
assert hash(policy) == hash(ResourceSecurity(exempt_params=frozenset({"a", "b"})))
98+
99+
92100
def test_security_rejection_does_not_fall_through_to_next_template():
93101
# A strict template's security rejection must halt iteration, not
94102
# fall through to a later permissive template. Previously matches()

tests/shared/test_path_security.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,9 @@ def test_contains_path_traversal(value: str, expected: bool):
7373
("1:foo", False),
7474
# Colon not in position 1
7575
("ab:c", False),
76+
# Non-ASCII letter is not a drive letter
77+
("Ω:namespace", False),
78+
("é:foo", False),
7679
],
7780
)
7881
def test_is_absolute_path(value: str, expected: bool):

tests/shared/test_uri_template.py

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -281,14 +281,23 @@ def test_parse_rejects_oversized_template():
281281
UriTemplate.parse("x" * 101, max_length=100)
282282

283283

284-
def test_parse_rejects_too_many_expressions():
285-
with pytest.raises(InvalidUriTemplate, match="maximum of"):
286-
UriTemplate.parse("{a}" * 11, max_expressions=10)
284+
def test_parse_rejects_too_many_variables():
285+
template = "".join(f"{{v{i}}}" for i in range(11))
286+
with pytest.raises(InvalidUriTemplate, match="maximum of 10 variables"):
287+
UriTemplate.parse(template, max_variables=10)
288+
289+
290+
def test_parse_counts_variables_not_expressions():
291+
# A single {v0,v1,...} expression packs many variables under one
292+
# brace pair. Counting expressions would miss this.
293+
template = "{" + ",".join(f"v{i}" for i in range(11)) + "}"
294+
with pytest.raises(InvalidUriTemplate, match="maximum of 10 variables"):
295+
UriTemplate.parse(template, max_variables=10)
287296

288297

289298
def test_parse_custom_limits_allow_larger():
290299
template = "".join(f"{{v{i}}}" for i in range(20))
291-
tmpl = UriTemplate.parse(template, max_expressions=20)
300+
tmpl = UriTemplate.parse(template, max_variables=20)
292301
assert len(tmpl.variables) == 20
293302

294303

0 commit comments

Comments
 (0)