Skip to content

fix(uri_template): use surrogateescape for unquote and quote to preserve non-UTF-8 octets (#3721) - #3724

Closed
nikolas-sapa wants to merge 1 commit into
modelcontextprotocol:mainfrom
nikolas-sapa:fix/uritemplate-surrogateescape-3721
Closed

nikolas-sapa wants to merge 1 commit into
modelcontextprotocol:mainfrom
nikolas-sapa:fix/uritemplate-surrogateescape-3721

Conversation

@nikolas-sapa

Copy link
Copy Markdown

Fixes #3721

Problem

RFC 3986 §2.1 percent-encoding encodes arbitrary octets, not UTF-8 characters exclusively. When matching URIs containing non-UTF-8 percent-encoded bytes (such as file://caf%E9 representing Latin-1 café), UriTemplate.match() replaced undecodable bytes with Unicode replacement character U+FFFD ('caf\ufffd'). This destroyed the original byte data before reaching resource handlers and broke the bidirectional invertibility guarantee between match() and expand() (expand({'name': 'caf\ufffd'}) becomes 'file://caf%EF%BF%BD').

Root Cause

All urllib.parse.unquote() calls across src/mcp/shared/uri_template.py omitted the errors parameter, falling back to standard library default errors="replace". Additionally, _encode() used standard quote() without errors="surrogateescape", causing re-expansion of surrogate-escaped strings to raise UnicodeEncodeError.

Changes

  1. Explicitly specify errors="surrogateescape" at all capture/unquote sites in uri_template.py:
    • _parse_query (lenient query parameters)
    • _extract_greedy (scalar and explode segments)
    • _scan_suffix (ifemp named parameters and bounded suffix variables)
    • _scan_prefix (bounded prefix variables)
  2. Pass errors="surrogateescape" in _encode() for quote() so that surrogate-escaped strings accurately re-encode back to the original percent triplets.
  3. Added unit tests verifying byte preservation and bidirectional invertibility across all URI template operator forms ({var}, {+var}, {#var}, {.var}, {/var}, {;var}, {?var}, {/list*}).

Verification

  • Tested reproduction verifying failure of 9/9 operator forms on unmodified code and success on patched code.
  • All 316 tests in tests/shared/test_uri_template.py passed in 0.32s.

@github-actions github-actions Bot added the missing-issue-link Auto-closed: PR needs a linked issue assigned to its author (see CONTRIBUTING.md) label Oct 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This PR has been closed automatically. This repo only keeps pull requests open when they come from a maintainer, or from a contributor a maintainer has assigned to the linked issue, and you aren't currently assigned to #3721.

If a maintainer assigns you to #3721, this PR reopens on its own and there's nothing more you need to do here. Assignment is a maintainer call based on capacity; comments that only ask to be assigned don't factor in. What does help is engaging on the issue itself by confirming the repro, explaining why it matters for your use case, or describing the approach you'd take.

You're welcome to keep pushing commits here (just avoid force-pushing, since GitHub can't reopen a rewritten branch), but that on its own won't get the PR reviewed or the issue assigned, and realistically most auto-closed PRs stay closed. There's no need to open a new PR either way.

CONTRIBUTING.md has the full reasoning, but in short:

  • We're a small team with very little capacity to review community PRs right now.
  • Many recent PRs are AI-generated with little human review, and reviewing one carefully still costs a maintainer as much time as it ever did. A well-described issue is usually more useful to us than the code.

Maintainers: reopen, remove missing-issue-link, or add bypass-issue-check to override.

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

Labels

missing-issue-link Auto-closed: PR needs a linked issue assigned to its author (see CONTRIBUTING.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UriTemplate.match() replaces non-UTF-8 percent-escapes with U+FFFD, corrupting the value handed to resource handlers

1 participant