Skip to content

fix(client): reject userinfo, dot segments and fragments in CIMD client_id URLs - #3774

Closed
kcncb wants to merge 1 commit into
modelcontextprotocol:mainfrom
kcncb:fix/cimd-client-id-url-validation
Closed

kcncb wants to merge 1 commit into
modelcontextprotocol:mainfrom
kcncb:fix/cimd-client-id-url-validation

Conversation

@kcncb

@kcncb kcncb commented Oct 27, 2026 •

Copy link
Copy Markdown

Fixes #3768

Scope note: this PR covers item 2 of the issue (CIMD client_id URL validation). Item 1, issuer string normalisation, is a behaviour change on the validation path and I'd rather land it separately if maintainers want it.

🔗 Related issues

📋 What's the problem?

is_valid_client_metadata_url() enforced only two of the rules that draft-ietf-oauth-client-id-metadata-document section 3 puts on a client metadata document URL: https and a non-root path. On main today:

>>> from mcp.client.auth.utils import is_valid_client_metadata_url as v
>>> v("https://user:secret@example.com/client")
True
>>> v("https://example.com/path/../client")
True
>>> v("https://example.com/client#section")
True

The authorization server is still the binding check, so this is not a security hole by itself — but the client's pre-flight check accepts URLs it already knows are unusable, and the failure only shows up later in the auth flow.

🛠️ The fix

is_valid_client_metadata_url() now also rejects:

  • URLs with parsed.username or parsed.password
  • paths containing a . or .. segment (percent-decoded first — RFC 3986 normalises %2E to . for dot-segment removal, so /%2E%2E/client is a .. segment)
  • URLs with a non-empty fragment

Query strings, ports and dots inside a segment (/path.with.dots/client) stay valid; the draft does not forbid them.

🧪 Testing

$ uv run --frozen pytest tests/client/test_auth.py -q -k "is_valid_client_metadata_url"
19 passed

Seven rows of the parametrised test are new and fail on main:

$ git checkout main -- src/mcp/client/auth/utils.py
$ uv run --frozen pytest tests/client/test_auth.py -q -k "is_valid_client_metadata_url"
7 failed, 12 passed

ruff check and ruff format --check pass on both touched files.

@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 27, 2026
@github-actions

github-actions Bot commented Oct 27, 2026 •

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 #3768.

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

Issuers are compared after URL normalisation; partial CIMD client_id URL validation

1 participant