Skip to content

feat: RFC 6570 URI templates with operator-aware security#2356

Open
maxisbey wants to merge 35 commits intomainfrom
feat/uri-template-rfc6570
Open

feat: RFC 6570 URI templates with operator-aware security#2356
maxisbey wants to merge 35 commits intomainfrom
feat/uri-template-rfc6570

Conversation

@maxisbey
Copy link
Contributor

@maxisbey maxisbey commented Mar 26, 2026

Replaces the regex-based resource template matcher with a proper RFC 6570 implementation, adds configurable path-safety validation, and ships a standalone UriTemplate utility usable from the low-level server.

Motivation and Context

The existing implementation supports only RFC 6570 Level 1 (simple {var} expansion). Users writing file://{+path} to match multi-segment paths hit a confusing "mismatch" error because the decorator's regex-based parameter extraction doesn't understand the + operator. The TypeScript, C#, and Go SDKs all support Level 3 or higher.

The matcher also has a regex-escaping gap: template literals like . become wildcards, so data://v1.0/{id} incorrectly matches data://v1X0/42.

Separately, path traversal via encoded slashes (..%2Fetc%2Fpasswd) is a known concern for resource handlers that touch the filesystem. The Kotlin SDK documents a decode-once-validate-in-handler model; we adopt the same model with configurable pre-validation.

Closes #436, closes #220, closes #159, closes #378
Supersedes #197, #427, #1439, #2353

Architecture

Three layers:

mcp.shared.uri_template.UriTemplate — standalone RFC 6570 engine supporting Levels 1-3 plus path-style explode ({/var*}, {.var*}, {;var*}). Provides parse(), expand(), and match(). The matcher is a faithful inverse of expansion: values are percent-decoded and returned as-is with no path-safety validation. Query expressions ({?q,lang}) are matched leniently via parse_qs — order-agnostic, partial params accepted, extras ignored — so Python function defaults apply naturally. Usable directly from the low-level server.

mcp.shared.path_securitycontains_path_traversal(), is_absolute_path(), safe_join() primitives. safe_join resolves the joined path and verifies it stays within a base directory, catching symlink escapes and null bytes.

ResourceSecurity policy — configurable dataclass wired into MCPServer(resource_security=...) and @mcp.resource(..., security=...). Defaults reject .. sequences that escape the starting directory and absolute-path values. Per-parameter exemptions via exempt_params={"name"}.

How Has This Been Tested?

  • 211 tests in tests/shared/test_uri_template.py covering parsing, expansion, matching, round-trip, lenient query matching, and parse-time validation
  • 50 tests in tests/shared/test_path_security.py covering traversal detection, absolute-path detection, and safe_join including symlink escapes
  • E2E tests through Client verifying the @mcp.resource decorator with all operators, ResourceSecurity policy enforcement, and function-default fallthrough for optional query params
  • Adversarial coverage: double-encoding (%252F), encoded backslash (%5C), encoded dots (%2E%2E), null bytes, multi-param with one poisoned value
  • 1524 tests total on the feature branch (1241 on main); all pass

ReDoS protection: templates with {+var} or {#var} in positions that would cause quadratic-time regex backtracking ({+a}{b}, {+a}/x/{+b}) are rejected at parse time. The safe common cases — file://docs/{+path}, {+path}.txt, {+path}{?query} — remain fully supported and verified linear at large input sizes.

Breaking Changes

  • Extracted parameters that would escape their starting directory via .. or look like absolute paths are rejected by default. Configure via ResourceSecurity to opt out.
  • Template literals are now regex-escaped. A . in the template matches only a literal dot.
  • {?q,lang} matching a URI with no query params now returns a match with absent params (so function defaults apply) instead of no match.
  • Malformed templates (unclosed braces, duplicate variable names, unsupported syntax, ReDoS-prone patterns) raise InvalidUriTemplate at decoration time instead of silently misbehaving.
  • A non-template URI paired with a handler taking only Context now errors at decoration time. This pattern was previously silently broken (resource registered but unreachable).

See docs/migration.md for migration guidance.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

UriTemplate.match() decodes once and returns values as-is without post-decode validation. This aligns with the Kotlin and C# SDKs and with the MCP spec's own canonical file:///{path} example, which requires multi-segment values. Path-safety validation lives in ResourceSecurity (configurable pre-checks) and safe_join (the gold-standard resolve-and-verify check for filesystem handlers).

The ; path-param operator supports full expand/match round-trip including the RFC §3.2.7 ifemp edge case — the only SDK implementation that does.

New documentation at docs/server/resources.md covers template syntax, security configuration, and low-level server usage.

AI Disclaimer

Adds `mcp.shared.uri_template.UriTemplate`, a standalone utility for
parsing, expanding, and matching RFC 6570 URI templates. Supports
Levels 1-3 fully plus path-style explode (`{/var*}`, `{.var*}`,
`{;var*}`).

Matching enforces structural integrity: decoded values are validated
against their operator's permitted character set. A simple `{var}`
whose decoded value contains `/` is rejected, preventing `%2F`
smuggling while still allowing `/` in `{+var}` where it is
intentional. This is the operator-aware generalization of the
post-decode check for encoded path separators.

Also fixes the existing regex-escaping gap where template literals
like `.` were treated as regex wildcards.

The utility lives in `shared/` so it is usable from both client code
(expand) and server code (match), including lowlevel server
implementations that do not use MCPServer.
Adds `mcp.shared.path_security` with three standalone utilities for
defending against path-traversal attacks when URI template parameters
flow into filesystem operations:

- `contains_path_traversal()` — base-free component-level check for
  `..` escapes, handles both `/` and `\` separators
- `is_absolute_path()` — detects POSIX, Windows drive, and UNC
  absolute paths (which silently discard the base in `Path` joins)
- `safe_join()` — resolve-and-verify within a sandbox root; catches
  `..`, absolute injection, and symlink escapes

These are pure functions usable from both MCPServer and lowlevel
server implementations. `PathEscapeError(ValueError)` is raised by
`safe_join` on violation.
Refactors the internal `ResourceTemplate` to use the RFC 6570
`UriTemplate` engine for matching, and adds a configurable
`ResourceSecurity` policy for path-safety checks on extracted
parameters.

`ResourceTemplate.matches()` now:
- Delegates to `UriTemplate.match()` for full RFC 6570 Level 1-3
  support (plus path-style explode). `{+path}` can match
  multi-segment paths.
- Enforces structural integrity: `%2F` smuggled into a simple
  `{var}` is rejected.
- Applies `ResourceSecurity` policy: path traversal (`..` components)
  and absolute paths rejected by default, with per-parameter
  exemption available.

The `@mcp.resource()` decorator now parses the template once at
decoration time via `UriTemplate.parse()`, replacing the regex-based
param extraction that couldn't handle operators like `{+path}`.
Malformed templates surface immediately with a clear
`InvalidUriTemplate` including position info.

Also fixes the pre-existing bug where template literals were not
regex-escaped (a `.` in the template acted as a wildcard).
Adds `resource_security` to `MCPServer.__init__` and a per-resource
`security` override to the `@resource()` decorator. Templates inherit
the server-wide policy unless overridden.

Exports `ResourceSecurity` and `DEFAULT_RESOURCE_SECURITY` from
`mcp.server.mcpserver` for user configuration.

Usage:

    # Server-wide relaxation
    mcp = MCPServer(resource_security=ResourceSecurity(reject_path_traversal=False))

    # Per-resource exemption for non-path parameters
    @mcp.resource(
        "git://diff/{+range}",
        security=ResourceSecurity(exempt_params=frozenset({"range"})),
    )
    def git_diff(range: str) -> str: ...
Documents the RFC 6570 support, security hardening defaults, and
opt-out configuration for the resource template rewrite. Grouped with
the existing resource URI section.
Changes the type from frozenset[str] to collections.abc.Set[str] so
users can write exempt_params={"range"} instead of
exempt_params=frozenset({"range"}). The default factory stays
frozenset for immutability.
… usage

Adds docs/server/resources.md as the first page under the planned
docs/server/ directory. Covers static resources, RFC 6570 template
patterns, the built-in security checks and how to relax them, the
safe_join pattern for filesystem handlers, and equivalent patterns for
low-level Server implementations.

Creates the docs/server/ nav section in mkdocs.yml.
RFC 6570 requires repeated variables to expand to the same value.
Enforcing this at match time would require backreferences with
potentially exponential cost. We reject at parse time instead,
following the recommendation in #697.

Previously a template like {x}/{x} would parse and silently return
only the last captured value on match.
@maxisbey maxisbey requested a review from pja-ant March 26, 2026 17:56
maxisbey added 17 commits March 26, 2026 18:24
Adds coverage for encoding-based attack vectors across both security
layers:

Layer 1 (structural integrity in UriTemplate.match):
- Double-encoding %252F decoded once, accepted as literal %2F
- Multi-param template with one poisoned value rejects whole match
- Value decoding to only the forbidden delimiter rejected

Layer 2 (ResourceSecurity traversal check):
- %5C backslash passes structural, caught by traversal normalization
- %2E%2E encoded dots pass structural, caught by traversal check
- Mixed encoded+literal slash fails at regex before decoding
Cheap heuristic for distinguishing concrete URIs from templates
without full parsing. Returns True if the string contains at least
one {...} pair. Does not validate; a True result does not guarantee
parse() will succeed.

Matches the TypeScript SDK's UriTemplate.isTemplate() utility.
Adds a max_uri_length keyword argument (default 64 KiB) that returns
None for oversized inputs before regex evaluation. Guards against
resource exhaustion from pathologically long URIs, particularly on
stdio transport where there is no inherent message size limit.

Consistent with the existing max_length/max_expressions limits on
parse(); the default is exported as DEFAULT_MAX_URI_LENGTH.
Ports three test scenarios from the TypeScript SDK:

- Repeated-slash literals (///{a}////{b}////) preserved exactly and
  rejected when slash count differs
- Trailing extra path component rejected (/users/{id} vs
  /users/123/extra); guards against a refactor from fullmatch to
  match or search
- Adjacent variables with prefix-overlapping names ({var}{vara});
  documents the greedy capture split and confirms positional groups
  map to the correct dict keys
Null bytes pass through Path construction but fail at the syscall
boundary with a cryptic 'embedded null byte' error. Rejecting in
safe_join gives callers a clear PathEscapeError instead, and guards
against null-byte injection when the path is used for anything other
than immediate file I/O (logging, subprocess args, config).
The @resource() decorator now classifies resources based solely on
whether the URI contains template variables, not on whether the
handler has parameters.

Previously, a handler taking only a Context parameter on a
non-template URI would register as a zero-variable template. The
template matched with an empty dict, which the walrus check in
resource_manager treated as falsy, making the resource permanently
unreachable. This has never worked.

Now such a handler errors at decoration time with a clear message
noting that Context injection for static resources is planned but not
yet supported. Handlers with non-Context parameters on non-template
URIs also get a clearer error than the old 'Mismatch' message.

Also changes the resource_manager walrus check to compare against
None explicitly, as defense-in-depth against any future case where
matches() legitimately returns an empty dict.
Three fixes to the path-style parameter operator:

Non-explode {;id}: the regex used =? (optional equals), which let
{;id} match ;identity=john by consuming 'id' as a prefix. Added a
lookahead asserting the name ends at = or a delimiter.

Explode {;keys*} matching: captured segments included the name=
prefix (returning ['keys=a', 'keys=b'] instead of ['a', 'b']) and
did not validate the parameter name (so ;admin=true matched). Now
strips the prefix and rejects wrong names in post-processing.

Explode {;keys*} expansion: emitted name= for empty items. RFC
3.2.7's ifemp rule says ; omits the = for empty values, so
['a', '', 'b'] now expands to ;keys=a;keys;keys=b.

All three are covered by new round-trip tests including the
empty-item edge case.
UriTemplate.match() no longer rejects decoded values containing
characters like /, ?, #, &. It now faithfully returns whatever
expand() would have encoded, so match(expand(x)) == x holds for all
inputs.

The previous check broke round-trip for legitimate values (a&b
expanded to a%26b but match rejected it) and was inconsistent with
every other MCP SDK. The spec's own canonical example file:///{path}
requires multi-segment values; Kotlin and C# already decode without
rejection and document handler-side validation as the security
contract.

Path-safety validation remains in ResourceSecurity (configurable) and
safe_join (the gold-standard check). The %2F path-traversal attack
vector is still blocked: ..%2Fetc%2Fpasswd decodes to ../etc/passwd,
which contains_path_traversal rejects. Tests confirm this end-to-end.

This aligns us with Kotlin's documented model: decode once, pass to
handler, handler validates.
UriTemplate.match() now handles trailing {?...}/{&...} expressions via
urllib.parse.parse_qs instead of positional regex. Query parameters
are matched order-agnostic, partial params are accepted, and
unrecognized params are ignored. Parameters absent from the URI stay
absent from the result so downstream function defaults apply.

This restores the round-trip invariant for query expansion: RFC 6570
skips undefined vars during expand(), so {?q,lang} with only q set
produces ?q=foo. Previously match() rejected that output; now it
returns {'q': 'foo'}.

Templates with a literal ? in the path portion (?fixed=1{&page})
fall back to strict regex matching since the URI split won't align
with the template's expression boundary.

The docs example at docs/server/resources.md (logs://{service}{?since,level}
with Python defaults) now works as documented.
Four small corrections:

Varname grammar: the RFC grammar requires dots only between varchar
groups, so {foo..bar} and {foo.} are now rejected. Previously the
regex allowed any dot placement after the first char.

Adjacent explodes: previously only same-operator adjacent explodes
({/a*}{/b*}) were rejected. Different operators ({/a*}{.b*}) are
equally ambiguous because the first operator's character class
typically includes the second's separator, so the first explode
greedily consumes both. All adjacent explodes are now rejected; a
literal or non-explode variable between them still disambiguates.

Documented the inherent ambiguity of multi-var reserved expressions
({+x,y} with commas in values) and the intentional tradeoff that
{+var} match stops at ? and # so {+path}{?q} can separate correctly.
…aptures

Two RFC-conformance fixes:

Reserved expansion ({+var}, {#var}) now passes through existing %XX
pct-triplets unchanged per RFC 6570 section 3.2.3, while still
encoding bare %. Previously quote() double-encoded path%2Fto into
path%252Fto. Simple expansion is unchanged (still encodes %
unconditionally).

Match patterns now use * instead of + quantifiers so defined-but-empty
values round-trip. RFC says empty variables still emit the operator
prefix: {#section} with section='' expands to '#', but the previous
.+ pattern could not match the empty capture after it. All eight
operators now consistently accept empty values.

The quantifier change affects adjacent-unrestricted-var resolution:
{a}{b} matching 'xy' now gives {a: 'xy', b: ''} (greedy first-wins)
instead of the previous {a: 'x', b: 'y'} (artifact of + backtracking).
Adjacent vars without a separating literal are inherently ambiguous
either way; a literal between them ({a}-{b}) still disambiguates.
Replaces tuple[X, ...] with list[X] throughout UriTemplate internals
and public API. The tuples were defensive immutability nobody needed:
the dataclass fields are compare=False so they do not participate in
hash/eq, and the public properties now return fresh copies so callers
cannot mutate internal state.

Helper function parameters take Sequence[X] where they only iterate;
returns are concrete list[X]. The only remaining tuples are the
fixed-arity (pair) return types on _parse and _split_query_tail, which
is the correct use of tuple.
The resource template migration section was documenting new features
alongside behavior changes. Trimmed to the four actual breakages:
path-safety checks now applied by default, template literals regex-
escaped, lenient query matching, and parse-time validation. New
capabilities and best-practice guidance moved to the Resources doc
via a link at the end.
Adds a sentence on lenient query matching (order-agnostic, extras
ignored, defaults apply) after the logs example.

Adds the component-based clarification for the .. check so users know
values like HEAD~3..HEAD and v1.0..v2.0 are unaffected.

Fixes the exempt_params motivating example in both resources.md and
migration.md. The previous git://diff/{+range} example used
HEAD~3..HEAD, which the component-based check already passes without
exemption. Replaced with inspect://file/{+target} receiving absolute
paths, which genuinely requires the opt-out.
The [^?#]* match pattern for {+var} and {#var} overlaps with every
other operator's character class. When a trailing literal fails to
match, the regex engine backtracks through O(n) split points with
O(n) rescanning each, yielding quadratic time. A 64KB payload (the
default max_uri_length) against {+prefix}{/path*}/END consumed ~25s
CPU per request.

Two conditions trigger the quadratic case, now both rejected at parse
time:

1. {+var} immediately adjacent to any expression ({+a}{b}, {+a}{/b*})
2. Two {+var}/{#var} anywhere in the template, even with a literal
   between them ({+a}/x/{+b}) since [^?#]* matches the literal too

What remains permitted:

- {+path} at end of template (the flagship use case)
- {+path}.txt or {+path}/edit (literal suffix, linear backtracking)
- {+path}{?v,page} (query expressions stripped before pattern build)
- {+a}/sep/{b} (literal + bounded expression, disambiguated)

The _check_adjacent_explodes function is generalized to
_check_ambiguous_adjacency covering both explode adjacency and the
new reserved-expansion constraints.
migration.md: added note that static URIs with Context-only handlers
now error at decoration time. The pattern was previously silently
unreachable (the resource registered but could never be read); now
it surfaces early. Duplicate-variable-names rejection was already
covered in the malformed-templates paragraph.

resources.md: clarified that the .. check is depth-based (rejects
values that would escape the starting directory, so a/../b passes).
Changed template reference table intro from 'what the SDK supports'
to 'the most common patterns' since the table intentionally omits
the rarely-used fragment and path-param operators.

test_uri_template.py: corrected the stray-} test comment. RFC 6570
section 2.1 strictly excludes } from literals; we accept it for
TypeScript SDK parity, not because the RFC is lenient.
Adds two no-match cases for the lenient-query code path in
UriTemplate.match(): path regex failing when query vars are present,
and ; explode name mismatch in the path portion before a {?...}
expression.

Adds a passing case to test_resource_security_default_rejects_traversal
so the handler body executes (the test previously only sent rejected
URIs, leaving the handler uncovered).

Replaces the _make helper's unreachable return with
raise NotImplementedError since those tests only exercise matches().
…{&var}

Three fixes to the two-phase query matching path:

Replaced parse_qs with a manual &/= split using unquote(). parse_qs
follows application/x-www-form-urlencoded semantics where + decodes
to space, but RFC 6570 and RFC 3986 treat + as a literal sub-delim.
A client sending ?q=C++ previously got 'C  '; the path-portion
decoder (unquote) already handled this correctly, so the two code
paths disagreed.

Fragment is now stripped before splitting on ?. A URI like
logs://api?level=error#section1 previously returned
level='error#section1' via the lenient path while the strict-regex
path correctly stopped at #.

_split_query_tail now requires the trailing tail to start with a
{?...} expression. A standalone {&page} expands with an & prefix
(no ?), so partition('?') found no split and the path regex failed.
Such templates now fall through to strict regex which handles them
correctly. Also extends the path-portion check to bail on {?...}
expressions left in the path, not just literal ?.
match() docstring: qualified the round-trip claim with the RFC
section 1.4 caveat that values containing their operator's separator
unencoded do not round-trip (e.g. {.ext} with 'tar.gz').

resource() decorator docstring: removed the 'or the function has
parameters' clause which commit 674783f made stale; template/static
is now decided purely by URI variables.

Added DEFAULT_MAX_TEMPLATE_LENGTH, DEFAULT_MAX_EXPRESSIONS, and
DEFAULT_MAX_URI_LENGTH to __all__ to match the stated intent that
these are part of the public API.

Moved DEFAULT_MAX_URI_LENGTH import in test file from function body
to top-level per repo convention.
The eight resource-template tests added in this PR were placed inside
the legacy TestServer and TestContextInjection classes to match
surrounding code, but the repo convention is standalone module-level
functions. Moved to the bottom of the file alongside the existing
standalone tests.
…acks

Adds cases for the fallback paths introduced in the lenient-query
fixes: literal ? in path portion, {?...} expression in path portion,
empty & segments in query string, and duplicate query keys.
@maxisbey maxisbey marked this pull request as ready for review March 26, 2026 23:13
_extract_path was dropping all empty segments when splitting an
explode capture, but only the first empty item comes from the
leading operator prefix. Subsequent empties are legitimate values:
{/path*} with ['a', '', 'c'] expands to /a//c and must match back
to the same list.

Split by separator, strip only items[0] if empty, then iterate.
The ; operator is unaffected since empty values use the bare-name
form which is a non-empty segment.
…ator

The explode capture pattern ((?:SEP body)*?) means non-empty captures
always start with the separator, so split()[0] is always empty. The
defensive if-check was a dead branch; slice unconditionally instead.
_split_query_tail enabled lenient matching for page{#section}{?q},
but lenient matching's partition('#') stripped the fragment before
the path regex (which expects #section) could see it, causing
fullmatch to always fail.

Extended the path-portion fallback check to also bail on {#...}
expressions and literal # characters, mirroring the existing ? check.
Such templates are semantically unusual (query-after-fragment is not
valid URI structure) but now round-trip correctly via strict regex.
Copy link
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

No new bugs found and the two issues from my earlier review are fixed, but this is a large feature PR (~1000 lines of new security-sensitive code across uri_template.py and path_security.py) with breaking changes that warrants human review.

Extended reasoning...

Overview

This PR replaces the regex-based resource template matcher with a full RFC 6570 URI template engine (uri_template.py, ~876 lines), adds filesystem path safety primitives (path_security.py, ~156 lines), and wires both into the MCPServer decorator via a configurable ResourceSecurity policy. It also adds 376 lines of new documentation and extensive test coverage (~800 lines across two new test files plus additions to existing test files). The PR claims to close 4 issues and supersede 4 other PRs.

Security risks

The path_security.py module implements path-traversal detection, absolute-path detection, and safe_join — all security-critical primitives. The contains_path_traversal function uses a depth-tracking algorithm rather than a simple substring check, which is the right approach but needs careful human verification. The ResourceSecurity policy is secure-by-default (rejecting traversal and absolute paths), which is good. However, the correctness of these security boundaries is too important for automated approval alone.

The uri_template.py module includes ReDoS protection that rejects quadratic-backtracking patterns at parse time. The design choices here (which patterns to allow vs reject) are architectural decisions that benefit from human review.

Level of scrutiny

This PR requires high scrutiny. It introduces ~1000 lines of new production code in two new modules, contains multiple breaking changes, and touches security-sensitive code paths (path traversal protection, URI parsing for resource routing). The RFC 6570 implementation involves complex regex construction and matching logic that could have subtle edge cases beyond what automated testing catches.

Other factors

The test coverage is thorough (211 tests for URI templates, 50 for path security, plus E2E tests), and the two bugs I found in my previous review were promptly fixed with regression tests. The documentation is comprehensive. However, the breaking changes listed in the migration guide affect existing users and represent design decisions (e.g., static URI + Context-only handler now errors, query params match leniently by default) that a maintainer should explicitly sign off on.

Bare dict return types are now parameterized (dict[str, str] or
dict[str, Any] as appropriate). Low-level handler examples now
include ServerRequestContext[Any] and PaginatedRequestParams types
for the ctx and params parameters, with the corresponding imports
added to each code block.
Added a link to the MCP resources specification after the intro.

Rewrote the multi-segment paths section to lead with the problem:
show a URI that fails with {name} before introducing {+name} as the
fix. Code comments align inputs with outputs for at-a-glance parsing.

Rewrote the query parameters section to lead with the two concrete
URIs a user would want to support (base and with-query), then show
how one template covers both.
The adjacency check rejected {+a}{b} but not the symmetric {a}{+b}.
Both produce overlapping greedy quantifiers; a 64KB crafted input
against prefix{a}{+b}.json takes ~23s to reject.

Added prev_path_expr tracking so {+var} immediately after any path
expression is rejected. {expr}{#var} remains allowed since the #
operator prepends a literal '#' that the preceding group's character
class excludes, giving a natural boundary.

Also adds the missing 'from typing import Any' to the three low-level
server examples in docs/server/resources.md.
Comment on lines +840 to +869
prev_explode = False
prev_reserved = False
prev_path_expr = False
seen_reserved = False
for part in parts:
if isinstance(part, str):
# A literal breaks immediate adjacency but does not reset
# the seen-reserved count: [^?#]* matches most literals.
prev_explode = False
prev_reserved = False
prev_path_expr = False
continue
for var in part.variables:
# ?/& are stripped before pattern building and never reach
# the path regex.
if var.operator in ("?", "&"):
prev_explode = False
prev_reserved = False
prev_path_expr = False
continue

if prev_reserved or (var.operator == "+" and prev_path_expr):
raise InvalidUriTemplate(
"{+var} or {#var} immediately adjacent to another expression "
"causes quadratic-time matching; separate them with a literal",
template=template,
)
if var.operator in ("+", "#") and seen_reserved:
raise InvalidUriTemplate(
"Multiple {+var} or {#var} expressions in one template cause "
Copy link
Contributor

Choose a reason for hiding this comment

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

🔴 ReDoS gap: _check_ambiguous_adjacency rejects reserved-adjacent and explode-adjacent patterns but misses two adjacent non-reserved expressions like {a}{b}suffix. The resulting regex ([^/?#&,]*)([^/?#&,]*)suffix has identical character classes in both groups, causing O(n²) backtracking (~16s at max_uri_length=65536) when the suffix fails to match. Fix: reject any two path expressions immediately adjacent without a disambiguating literal separator.

Extended reasoning...

What the bug is

_check_ambiguous_adjacency (lines 840-883) is designed to reject URI template patterns that produce regexes with quadratic-time backtracking. It correctly handles three cases: (1) reserved expressions adjacent to other expressions ({+a}{b}), (2) multiple reserved expressions anywhere in the template ({+a}/x/{+b}), and (3) adjacent explode expressions ({/a*}{/b*}). However, it does not check for two adjacent non-reserved, non-explode expressions with identical character classes.

How it manifests

UriTemplate.parse("{a}{b}suffix") succeeds without error. The compiled regex is ([^/?#&,]*)([^/?#&,]*)suffix. Both capture groups use the exact same character class [^/?#&,]*. When matching a string like "a" * N + "x" (where the trailing suffix literal never appears), the regex engine must try all O(N) ways to split the a characters between the two groups, and for each split point the second group tries the remaining O(N) characters — totaling O(N²) backtrack steps.

Step-by-step proof

  1. UriTemplate.parse("{a}{b}suffix") succeeds — _check_ambiguous_adjacency does not reject it because prev_reserved is False for both simple-operator variables, and neither is an explode.
  2. The regex pattern is ([^/?#&,]*)([^/?#&,]*)suffix.
  3. Call match("a" * 10000 + "x") — the literal suffix is absent, so the regex must fail.
  4. The engine tries splitting 10000 a characters between groups 1 and 2 in all possible ways: ~10000²/2 = 50 million backtrack steps.
  5. Benchmarking confirms quadratic growth: N=1000 → 4ms, N=2000 → 18ms, N=5000 → 100ms, N=10000 → 400ms, N=20000 → 1.5s, extrapolating to ~16s at N=65536 (max_uri_length).
  6. Cross-operator variants like {.a}{b}suffix and {/a}{b}suffix also exhibit quadratic growth at the same rate, since their character classes overlap with simple expression's [^/?#&,]*.

Why existing code does not prevent it

The prev_path_expr flag (line 842) is only checked in the condition var.operator == "+" and prev_path_expr (line 861), which fires only when a + operator follows a path expression. Two adjacent simple-operator expressions (operator == "") bypass all three checks: prev_reserved is False, neither has operator == "+", and neither is an explode. The function was specifically written to prevent quadratic backtracking, but this case slipped through.

Impact

A server author using a template like {scheme}{host}.example.com or {a}{b}suffix would inadvertently create a ReDoS vector. Any client can craft a URI of up to 65536 characters that freezes the matching thread for ~16 seconds, enabling CPU denial of service. The _check_ambiguous_adjacency function was added by this PR specifically to prevent this class of vulnerability, making this a gap in its intended protection.

How to fix

Add a check for two adjacent non-reserved path expressions. After the existing checks on line 861-877, add:

if prev_path_expr and not prev_reserved and not var.explode and not prev_explode:
    # Two adjacent non-reserved expressions share the same character
    # class, causing O(n²) backtracking when a trailing literal fails.
    raise InvalidUriTemplate(
        "Adjacent expressions without a literal separator cause "
        "quadratic-time matching; separate them with a literal",
        template=template,
    )

This would reject {a}{b}suffix, {.a}{b}suffix, {a}{/b}suffix, etc., while still allowing {a}-{b} (literal separator) and {a}{?q} (query expressions are stripped before regex).

### Resource templates: matching behavior changes

Resource template matching has been rewritten with RFC 6570 support.
Four behaviors have changed:
Copy link
Contributor

Choose a reason for hiding this comment

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

🟡 Line 551 says "Four behaviors have changed:" but the section lists five bolded items (Path-safety checks, Template literals regex-escaped, Query parameters match leniently, Malformed templates fail at decoration time, Static URIs with Context-only handlers now error). Should say "Five" instead of "Four".

Extended reasoning...

What the bug is

The migration guide section "Resource templates: matching behavior changes" (line 551) introduces the behavioral changes with "Four behaviors have changed:" but then proceeds to list five distinct bolded items.

The five items

Counting the bold-headed paragraphs in the section:

  1. Path-safety checks applied by default. (line 553)
  2. Template literals are regex-escaped. (line 573)
  3. Query parameters match leniently. (line 577)
  4. Malformed templates fail at decoration time. (line 583)
  5. Static URIs with Context-only handlers now error. (line 588)

Step-by-step verification

Each of these five items is a distinct behavioral change with its own bolded header paragraph. They correspond to the five bullet points in the PR description under "Breaking Changes" as well. The count "Four" was likely correct at an earlier draft before the fifth item (static URIs with Context-only handlers) was added, and the introductory count was not updated.

Impact

This is a minor documentation inaccuracy. Users reading the migration guide may be confused by the mismatch between the stated count and the actual number of items, or might stop reading after the fourth item, missing the fifth behavioral change.

How to fix

Change line 551 from Four behaviors have changed: to Five behaviors have changed:.

Comment on lines +380 to +382
from typing import Any

from mcp.types import ListResourceTemplatesResult, PaginatedRequestParams, ResourceTemplate
Copy link
Contributor

Choose a reason for hiding this comment

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

🟡 The third low-level server code example ("Listing templates", lines 379-401) uses ServerRequestContext[Any] on line 386 and Server(...) on line 396, but the import block (lines 380-382) only imports Any and types from mcp.types. Missing from mcp.server.context import ServerRequestContext and from mcp.server.lowlevel import Server. Users copying this example verbatim will get NameError. The first two low-level examples correctly import both.

Extended reasoning...

What the bug is

The third low-level server code example in docs/server/resources.md — the "Listing templates" section (lines 379-401) — uses ServerRequestContext[Any] in the function signature on line 386 and Server(...) on line 396, but neither is imported. The import block (lines 380-382) only contains:

from typing import Any

from mcp.types import ListResourceTemplatesResult, PaginatedRequestParams, ResourceTemplate

How it manifests

A user who copies this code example as a standalone snippet and runs it will immediately get:

NameError: name 'ServerRequestContext' is not defined

If they fix that, they'll next hit:

NameError: name 'Server' is not defined

The first two examples get it right

The first example ("Static resources", lines 253-295) correctly imports both at lines 256 and 265:

  • from mcp.server.lowlevel import Server
  • from mcp.server.context import ServerRequestContext

The second example ("Matching requests", lines 313-340) also correctly imports both at lines 316-317.

The third example was likely written as a continuation (it references TEMPLATES and on_read_resource from the second example), but it includes its own import block, implying it is intended to be self-contained. The presence of an import block that imports Any and several mcp.types — but omits the two other needed imports — is clearly an oversight.

Distinction from previously fixed bug

This is distinct from the earlier review comment (comment index #4 in the timeline) which flagged missing from typing import Any across all three examples. That bug was fixed — Any is now correctly imported on line 380. However, the two other imports (ServerRequestContext and Server) were not added to this third example.

Impact

This is a documentation-only issue — no running code is affected. However, the examples are explicitly designed to be copy-paste-ready for users building on the low-level server API. The fix is straightforward: add from mcp.server.context import ServerRequestContext and from mcp.server.lowlevel import Server to the import block at lines 380-382.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant