Skip to content

fix: resolve dot-segments and percent-encoding in check_resource_allowed (#3303) - #3532

Closed
karawanshy wants to merge 1 commit into
modelcontextprotocol:mainfrom
karawanshy:fix/3303-resource-path-traversal
Closed

karawanshy wants to merge 1 commit into
modelcontextprotocol:mainfrom
karawanshy:fix/3303-resource-path-traversal

Conversation

@karawanshy

Copy link
Copy Markdown

Summary

Fixes #3303.

check_resource_allowed() in src/mcp/shared/auth_utils.py performed hierarchical
resource matching with str.startswith() on raw URL paths, after only
trailing-slash normalization. It did not resolve dot-segments (., ..) or decode
percent-encoding, so a requested resource could satisfy startswith(configured)
while resolving to a path outside the configured resource.

check_resource_allowed(
    "https://mcp.example.com/api/../admin",  # resolves to /admin
    "https://mcp.example.com/api",           # token scoped to /api
)  # -> True on current main (should be False)

Because any downstream resource server normalizes the path before serving, the SDK
authorizes /api but the server delivers /admin — a confused-deputy /
path-traversal gap at the resource-authorization boundary. Percent-encoded variants
(%2e%2e) bypass it the same way.

Change

Add a small _normalize_path helper that percent-decodes the path and resolves
dot-segments (posixpath.normpath, anchored at / so .. cannot escape above
root), applied to both the requested and configured paths before the existing
origin check and trailing-slash / prefix comparison.

Legitimate hierarchical children (/api/v1/users) and dot-segments that resolve
back inside the resource (/api/v1/../v1) still match; only paths that escape the
configured resource are now rejected.

Deliberate choices

  • Single-pass decoding (per RFC 3986): %252e%252e decodes only to the literal
    %2e%2e, matching a conformant server that decodes once. A fixed-point loop was
    intentionally avoided — it would make the check stricter than a conformant server
    and over-reject legitimate requests. Pinned by a regression test.
  • %2f/: an encoded slash is treated as a separator. This is the
    conservative direction for an authorization check, since collapsing it can only
    resolve the path higher/shorter, never grant access to a deeper path.

Testing

  • uv run --frozen pytest tests/shared/test_auth_utils.py — 20 passed (15 existing
    • 5 new: dot-segment traversal, percent-encoded traversal, harmless in-bounds
      dot-segments still allowed, single-pass decode, symmetric configured-path
      normalization).
  • uv run --frozen ruff check / ruff format --check / pyright — all clean.

Notes

This refreshes the previously-closed #2585 against current main, as suggested in
the issue.

check_resource_allowed compared raw URL paths with str.startswith, so a
requested resource such as https://host/api/../admin (which resolves to
/admin) satisfied the check for a token scoped to /api, bypassing the
resource-authorization boundary. Percent-encoded variants (%2e%2e) slipped
through the same way.

Normalize both the requested and configured paths (single-pass percent-decode
per RFC 3986, then posixpath.normpath anchored at /) before the existing
origin and hierarchical-prefix comparison. Legitimate child resources still
match; traversal that escapes the configured path no longer does.

Fixes modelcontextprotocol#3303
@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 Sep 18, 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 #3303.

If a maintainer assigns you to #3303, 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.

@github-actions github-actions Bot closed this Sep 18, 2026
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.

check_resource_allowed(): path matching skips dot-segment/percent-encoding normalization (auth-boundary bypass)

1 participant