Repository navigation
check_resource_allowed(): path matching skips dot-segment/percent-encoding normalization (auth-boundary bypass) #3303
Description
Activity
Confirmed still present on
mainat6e30452:requested configured result expected https://mcp.example.com/api/../adminhttps://mcp.example.com/apiTrueFalsehttps://mcp.example.com/api/%2e%2e/adminhttps://mcp.example.com/apiTrueFalsehttps://mcp.example.com/api/./v1https://mcp.example.com/apiTrueTruehttps://mcp.example.com/api123https://mcp.example.com/apiFalseFalseThe 15 existing tests in
tests/shared/test_auth_utils.pyall pass, so this is
a genuine gap in coverage rather than a regression. Only two call sites are
affected, both insrc/mcp/client/auth/oauth2.py(lines 210 and 577).Two implementation details worth settling before anyone writes the patch, since
the straightforwardunquote+posixpath.normpathapproach has sharp edges:-
Unquoting before normalizing conflates
%2Fwith a real separator.
unquote("/api/a%2Fb")→/api/a/b, so an encoded slash becomes a path
boundary. Per RFC 3986 §2.2 those are not equivalent, so this could turn one
class of false positive into another. Decoding only the dot-segment triples
(%2e/%2E), or normalizing per-segment without decoding separators, avoids
that. Do you have a preference? -
normpathstrips trailing slashes (normpath("/api/")→/api), so the
existing trailing-slash normalization has to run after it. Otherwise the
/api123vs/apiboundary guard regresses.
Questions before I open anything:
- Security policy.
SECURITY.mdasks for vulnerabilities to go through the
private advisory process rather than public issues or PRs. This one is already
public and not mine to re-route, but I'd rather not push a public patch for an
auth-boundary bypass if you'd prefer to handle it privately or bundle it into a
coordinated fix. Happy either way — just let me know which you want. - @Choppaaahh offered to refresh fix(auth): normalize URL paths before hierarchical resource check #2585 against v2. If you're already on it,
I'll stay out of the way; if not, I'm glad to pick it up. - Does this need a
[v1.x]backport? The same code is onv1.x, and
SECURITY.mdlists 1.x as receiving security fixes.
Happy to take it once it's triaged and there's a direction on (1).
Disclosure: I used AI assistance to search the tracker and produce the
reproduction above. I've reviewed the code and results myself and will answer
review questions directly.-
1. %2F conflation — agreed: decode only the dot-segment triples (
%2e/%2E), not a fullunquote. Per RFC 3986 §2.2 an encoded%2Fisn't a separator, so decoding it would just trade one false-positive class for another. Dot-triple-only (or per-segment normalization that never decodes separators) is the right scope.2. Trailing slash — yes, run the existing trailing-slash normalization after
normpath, else the/api123vs/apiboundary regresses. Good catch.3. Security policy — it's already public (my routing miss), so the advisory path is moot for this one; I'd defer to maintainers on keep-vs-convert, but a public fix seems pragmatic now.
I won't be refreshing this myself — please feel free to take the patch (dot-triple decode + post-
normpathtrailing-slash). Happy to review. Thanks!I’d like to take this issue and prepare a fix for
main.I’m planning to normalize raw and percent-encoded dot segments such as
..and%2e%2e, while leaving encoded separators like%2Funtouched. I’ve added regression tests for those cases, and the targeted checks plus the full test suite pass locally.Before I open the PR, could a maintainer confirm whether this should also be backported to
v1.x? Since this touches resource authorization, please also let me know if you’d prefer this handled through a coordinated security process.I used AI assistance while investigating and implementing this change, and I reviewed the resulting code and tests myself.
- added
on Aug 14, 2026 @maxisbey — could you assign this to me? Happy to do the work, but the process
gate means nobody can move it forward until someone is assigned.Context on why it's stuck:
needs confirmationis still on, which per CONTRIBUTING means work shouldn't
start.- @SarathChandraBellam wrote a patch anyway and opened fix(auth): normalize resource path dot-segments #3308, which the bot
auto-closed on 17 Aug for exactly that reason — not assigned to this issue. - Still unchanged on
main: the last commit touching
src/mcp/shared/auth_utils.pyis be5bb7c (Feb 17, the trailing-slash fix in
fix: normalize trailing slashes before length check in check_resource_allowed #2074), so the reproduction in my first comment still holds.
I'd like to pick it up. @Choppaaahh, who filed this, handed the patch to me
explicitly above ("please feel free to take the patch") after I worked through
the two design pitfalls with them, and offered to review. To be clear about what
I have and haven't done: I've confirmed the bug and settled the approach, but I
haven't written the fix yet — I held off because of theneeds confirmation
label. I can have it up quickly once assigned.The approach is already agreed in-thread:
- Decode only the
%2e/%2Edot triples rather than a fullunquote, since an
encoded%2Fisn't a path separator per RFC 3986 §2.2 and decoding it would
trade one false-positive class for another. - Run the existing trailing-slash normalization after
normpath, so the
/api123vs/apiboundary guard doesn't regress. - Regression tests for the raw
.., encoded%2e%2e,., and%2Fcases.
@SarathChandraBellam has independently implemented something along these lines
on #3308 — if you'd rather assign them and reopen that PR, that's a completely
reasonable call and I'll happily review instead. Just don't want this to sit
unassigned while three of us wait.Two things I still need a ruling on before opening anything:
v1.xbackport? Thev1+v2labels you added on 18 Aug suggest both
lines are affected, and SECURITY.md lists 1.x as receiving security fixes.
Should I plan a[v1.x]backport PR alongside themainone?- Public or advisory? SECURITY.md asks for vulnerabilities to go private,
and this is an auth-boundary bypass. It's already public so I assume a public
fix is fine, but I'd rather you say so than assume.
Disclosure: AI assistance used for the tracker search and reproduction above;
I've reviewed the code and results myself and will answer review questions
directly.
Summary
check_resource_allowed()insrc/mcp/shared/auth_utils.pyperforms hierarchical path matching withstr.startswith()after only trailing-slash normalization. It does not resolve dot-segments (..,.) or decode percent-encoding, so a requested resource can satisfystartswith(configured)while resolving to a path outside the configured resource.This is a confused-deputy / path-traversal gap at the resource-authorization boundary: any downstream resource server that normalizes paths (most do) will serve a different resource than the one the SDK authorized.
Reproduction
The current code normalizes to
/api/../admin/vs/api/, and"/api/../admin/".startswith("/api/")isTrue. The resolved path is/admin. Percent-encoded variants (%2e%2e) bypass it the same way.Impact
Where the result gates access to a protected resource, a caller can craft a resource indicator that passes the check but points elsewhere. Severity depends on deployment, but it's an auth-boundary correctness bug, not cosmetic.
Fix
Decode percent-encoding and resolve dot-segments (
posixpath.normpathafterunquote) on both paths before thestartswithcomparison, preserving trailing-slash semantics. A patch implementing exactly this was opened as #2585 and closed in the post-v2 backlog sweep with the note to reopen if still relevant against v2 — filing this issue per CONTRIBUTING so it can be triaged. The bug is present verbatim on currentmain. Happy to refresh the PR against v2 once triaged.