Skip to content

Commit 04623ed

Browse files
rdimitrovclaude
andauthored
fix(validators): harden mcp-name matching (PyPI/NuGet anchoring, comment-form safe) + cargo follow-ups (#1331)
Registry-wide hardening of the `mcp-name:` ownership-token match, plus the cargo follow-up fixes from the #1330 review — consolidated here per maintainer request. Rebased onto current `main` (the merged #1330 cargo commit drops out), so the diff is just the net-new work. ### Changes - **PyPI/NuGet: boundary-anchored token match.** Replace `strings.Contains(readme, "mcp-name: "+name)` with the shared `containsMCPNameToken`, so a README declaring a longer name (`…/widget-pro`) no longer satisfies an ownership claim for a shorter prefix (`…/widget`). (Cargo already got this in #1330; NPM is unaffected — it compares an exact metadata field.) - **Matcher: treat the HTML comment close as a boundary.** `<!-- mcp-name: NAME-->` / `<!--mcp-name: NAME-->` (any spacing) validate again — the documented hidden-comment form for PyPI/NuGet — while a genuine longer name (`…/widget--pro`) still does not. - **Cargo hardening (review of #1330):** pin scheme+port (not just host) on the README fetch and redirects; a rate-limited/failed crate-version existence probe now reports *transient* instead of "not found"; a 403 with the crate present no longer flatly asserts "no README". - **Docs:** note the token must be followed by a boundary. ### ⚠️ Behavior change for PyPI/NuGet — read before merging The match is **strictly stricter** (verified by fuzz, 2.3M execs: it can only flip pass→fail, never fail→pass). After the comment-close fix, the **only** forms that newly fail are unusual *inline* ones: the token ending a sentence (`…/my-mcp.`) or glued to a trailing `/`. The documented forms (own line, in `<!-- … -->`) are unaffected. **Correcting an earlier claim:** this re-validates on **edits too**, not just new versions — `edit.go → UpdateServer → ValidateUpdateRequest → validateRegistryOwnership` runs the token check on any edit of a live server (only delete-transitions skip it). So an existing PyPI/NuGet server whose README uses one of the breaking inline forms would fail on its next publish *or* edit. Given the v0.1 API-freeze posture, this should land as a deliberate, noted change. ### Testing `go build`, `vet`, `gofmt`, `golangci-lint` (prod files), `check-schema`, `validate-examples` all clean. Hermetic + **live** PyPI/NuGet/cargo suite passes. New: `TestCargoURLAllowed`, a combined-fixture probe-429→transient case, comment-form matcher cases, and `FuzzContainsMCPNameToken` (the strictly-stricter property, 2.3M execs). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 7a6d1bc commit 04623ed

8 files changed

Lines changed: 312 additions & 55 deletions

File tree

docs/modelcontextprotocol-io/package-types.mdx

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,8 @@ This MCP server executes SQL queries and manages database connections.
8787
<!-- mcp-name: io.github.username/database-query-mcp -->
8888
```
8989

90+
The `mcp-name:` token must be followed by a boundary — a newline, whitespace, an HTML tag, or the comment close `-->`. Keep it on its own line or inside `<!-- … -->`; do not glue it directly to trailing characters such as a sentence-ending period (`…/database-query-mcp.`), which prevents the match.
91+
9092
## NuGet Packages
9193

9294
For NuGet packages, the MCP Registry currently supports the official NuGet registry (`https://api.nuget.org/v3/index.json`) only.
@@ -125,6 +127,8 @@ This MCP server manages Azure DevOps work items and pipelines.
125127
<!-- mcp-name: io.github.username/azure-devops-mcp -->
126128
```
127129

130+
The `mcp-name:` token must be followed by a boundary — a newline, whitespace, an HTML tag, or the comment close `-->`. Keep it on its own line or inside `<!-- … -->`; do not glue it directly to trailing characters such as a sentence-ending period (`…/azure-devops-mcp.`), which prevents the match.
131+
128132
## Cargo (Rust) Packages
129133

130134
For Cargo packages, the MCP Registry currently supports the official crates.io registry (`https://crates.io`) only.

internal/validators/registries/cargo.go

Lines changed: 85 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -106,16 +106,37 @@ func cargoAllowedHosts(baseURL string) map[string]struct{} {
106106
return hosts
107107
}
108108

109+
// cargoURLAllowed reports whether u is a URL the validator may fetch for the
110+
// given base. The host must be in allowedHosts; additionally, for the real
111+
// crates.io base, the scheme must be https and the port must be the default —
112+
// so a metadata response or redirect cannot downgrade to http or steer the
113+
// validator at a non-standard port on an otherwise-trusted host. For test bases
114+
// (httptest servers) only the host is checked, so mocks keep working.
115+
func cargoURLAllowed(u *url.URL, baseURL string, allowedHosts map[string]struct{}) bool {
116+
if _, ok := allowedHosts[u.Hostname()]; !ok {
117+
return false
118+
}
119+
if baseURL == model.RegistryURLCrates {
120+
if u.Scheme != "https" {
121+
return false
122+
}
123+
if p := u.Port(); p != "" && p != "443" {
124+
return false
125+
}
126+
}
127+
return true
128+
}
129+
109130
// newCargoHTTPClient builds the client used for all crates.io calls. The
110-
// CheckRedirect policy pins every redirect hop to allowedHosts, so even though
111-
// the initial URL is host-pinned, an upstream 3xx cannot redirect the validator
112-
// to an unexpected host.
113-
func newCargoHTTPClient(allowedHosts map[string]struct{}) *http.Client {
131+
// CheckRedirect policy pins every redirect hop via cargoURLAllowed, so even
132+
// though the initial URL is checked, an upstream 3xx cannot redirect the
133+
// validator to an unexpected host, scheme, or port.
134+
func newCargoHTTPClient(baseURL string, allowedHosts map[string]struct{}) *http.Client {
114135
return &http.Client{
115136
Timeout: 10 * time.Second,
116137
CheckRedirect: func(req *http.Request, via []*http.Request) error {
117-
if _, ok := allowedHosts[req.URL.Hostname()]; !ok {
118-
return fmt.Errorf("refusing redirect to unexpected host %q", req.URL.Hostname())
138+
if !cargoURLAllowed(req.URL, baseURL, allowedHosts) {
139+
return fmt.Errorf("refusing redirect to unexpected URL %q", req.URL.Redacted())
119140
}
120141
if len(via) >= 10 {
121142
return errors.New("stopped after 10 redirects")
@@ -125,37 +146,53 @@ func newCargoHTTPClient(allowedHosts map[string]struct{}) *http.Client {
125146
}
126147
}
127148

128-
// cargoVersionExists checks whether a specific crate version exists on crates.io,
129-
// used to disambiguate a 403 from the README CDN. static.crates.io (S3) returns
130-
// 403 both for a genuinely-missing crate/version AND for a crate that exists but
131-
// has no rendered README, so a 403 alone cannot tell a publisher which it is.
132-
//
133-
// Returns (exists, determined): determined is false if the existence endpoint
134-
// itself was unreachable or returned an unexpected status, in which case the
135-
// caller should fall back to a generic message rather than assert existence.
136-
func cargoVersionExists(ctx context.Context, client *http.Client, baseURL, identifier, version string) (exists, determined bool) {
149+
// cargoVersionState is the outcome of probing the crate-version metadata
150+
// endpoint, used to disambiguate a 403 from the README CDN.
151+
type cargoVersionState int
152+
153+
const (
154+
// cargoVersionUnknown: the probe returned an unexpected status we can't classify.
155+
cargoVersionUnknown cargoVersionState = iota
156+
// cargoVersionExists: the crate version exists (200).
157+
cargoVersionExists
158+
// cargoVersionMissing: the crate version does not exist (404).
159+
cargoVersionMissing
160+
// cargoVersionTransient: the probe failed for a retryable reason (network
161+
// error, 429, or 5xx) — existence is undetermined and the caller should not
162+
// report "not found".
163+
cargoVersionTransient
164+
)
165+
166+
// probeCargoVersion checks whether a specific crate version exists on crates.io.
167+
// static.crates.io (S3) returns 403 both for a genuinely-missing crate/version
168+
// AND for a crate that exists but has no rendered README, so a 403 from the CDN
169+
// alone cannot tell a publisher which it is; this probe disambiguates, while
170+
// distinguishing a transient failure from a definitive missing/exists answer.
171+
func probeCargoVersion(ctx context.Context, client *http.Client, baseURL, identifier, version string) cargoVersionState {
137172
versionURL := fmt.Sprintf("%s/api/v1/crates/%s/%s",
138173
baseURL, url.PathEscape(identifier), url.PathEscape(version))
139174
req, err := http.NewRequestWithContext(ctx, http.MethodGet, versionURL, nil)
140175
if err != nil {
141-
return false, false
176+
return cargoVersionUnknown
142177
}
143178
req.Header.Set("User-Agent", cargoUserAgent)
144179
req.Header.Set("Accept", "application/json")
145180

146181
resp, err := client.Do(req)
147182
if err != nil {
148-
return false, false
183+
return cargoVersionTransient
149184
}
150185
defer resp.Body.Close()
151186

152-
switch resp.StatusCode {
153-
case http.StatusOK:
154-
return true, true
155-
case http.StatusNotFound:
156-
return false, true
187+
switch {
188+
case resp.StatusCode == http.StatusOK:
189+
return cargoVersionExists
190+
case resp.StatusCode == http.StatusNotFound:
191+
return cargoVersionMissing
192+
case resp.StatusCode == http.StatusTooManyRequests, resp.StatusCode >= 500 && resp.StatusCode < 600:
193+
return cargoVersionTransient
157194
default:
158-
return false, false
195+
return cargoVersionUnknown
159196
}
160197
}
161198

@@ -196,15 +233,21 @@ func cargoReadmeStatusError(ctx context.Context, client *http.Client, pkg model.
196233
// endpoint so the publisher gets an actionable message rather than a blanket
197234
// "not found".
198235
func cargoReadme403Error(ctx context.Context, client *http.Client, pkg model.Package, serverName string) error {
199-
exists, determined := cargoVersionExists(ctx, client, pkg.RegistryBaseURL, pkg.Identifier, pkg.Version)
200-
switch {
201-
case determined && exists:
202-
return fmt.Errorf("cargo package '%s' version '%s' exists on crates.io but has no rendered README. Add a README containing 'mcp-name: %s' and publish a new version", pkg.Identifier, pkg.Version, serverName)
203-
case determined && !exists:
236+
switch probeCargoVersion(ctx, client, pkg.RegistryBaseURL, pkg.Identifier, pkg.Version) {
237+
case cargoVersionExists:
238+
// The crate/version exists but the README CDN returned 403. The likely
239+
// cause is a missing README, but a 403 is not definitive proof (e.g. a
240+
// transient CDN/WAF block), so don't flatly assert "no README".
241+
return fmt.Errorf("cargo package '%s' version '%s' exists on crates.io, but its rendered README could not be retrieved (status: 403). If it has no README, add one containing 'mcp-name: %s' and publish a new version", pkg.Identifier, pkg.Version, serverName)
242+
case cargoVersionMissing:
204243
return fmt.Errorf("cargo package '%s' version '%s' not found on crates.io", pkg.Identifier, pkg.Version)
205-
default:
206-
return fmt.Errorf("cargo package '%s' version '%s' not found on crates.io (status: 403)", pkg.Identifier, pkg.Version)
244+
case cargoVersionTransient:
245+
return fmt.Errorf("crates.io could not confirm cargo package '%s' version '%s' (README status: 403, version check inconclusive) — likely transient, retry later", pkg.Identifier, pkg.Version)
246+
case cargoVersionUnknown:
247+
// Probe returned an unclassifiable status — fall through to the
248+
// best-effort message below.
207249
}
250+
return fmt.Errorf("cargo package '%s' version '%s' not found on crates.io (status: 403)", pkg.Identifier, pkg.Version)
208251
}
209252

210253
// validateCargoREADME performs the two-call README fetch and the mcp-name token
@@ -213,7 +256,7 @@ func cargoReadme403Error(ctx context.Context, client *http.Client, pkg model.Pac
213256
// bypassing the exact-baseURL guard that ValidateCargo enforces for callers.
214257
func validateCargoREADME(ctx context.Context, pkg model.Package, serverName string) error {
215258
allowedHosts := cargoAllowedHosts(pkg.RegistryBaseURL)
216-
client := newCargoHTTPClient(allowedHosts)
259+
client := newCargoHTTPClient(pkg.RegistryBaseURL, allowedHosts)
217260

218261
// Step 1: fetch the README pointer from the documented API endpoint.
219262
metaURL := fmt.Sprintf("%s/api/v1/crates/%s/%s/readme",
@@ -246,14 +289,15 @@ func validateCargoREADME(ctx context.Context, pkg model.Package, serverName stri
246289
return fmt.Errorf("cargo package '%s' metadata response missing 'url' field", pkg.Identifier)
247290
}
248291

249-
// Pin the README pointer to an allowed host before fetching it, so a metadata
250-
// response cannot steer the validator at an internal or attacker-chosen host.
292+
// Pin the README pointer to an allowed host/scheme/port before fetching it, so
293+
// a metadata response cannot steer the validator at an internal or
294+
// attacker-chosen URL.
251295
readmeParsed, err := url.Parse(meta.URL)
252296
if err != nil || readmeParsed.Hostname() == "" {
253297
return fmt.Errorf("cargo package '%s': crates.io returned an unparseable README URL", pkg.Identifier)
254298
}
255-
if _, ok := allowedHosts[readmeParsed.Hostname()]; !ok {
256-
return fmt.Errorf("cargo package '%s': crates.io returned a README URL on unexpected host %q — refusing to fetch", pkg.Identifier, readmeParsed.Hostname())
299+
if !cargoURLAllowed(readmeParsed, pkg.RegistryBaseURL, allowedHosts) {
300+
return fmt.Errorf("cargo package '%s': crates.io returned a README URL on an unexpected host/scheme %q — refusing to fetch", pkg.Identifier, readmeParsed.Redacted())
257301
}
258302

259303
// Step 2: fetch the rendered README from the (now host-validated) URL.
@@ -287,5 +331,11 @@ func validateCargoREADME(ctx context.Context, pkg model.Package, serverName stri
287331
return nil
288332
}
289333

334+
// If the token IS present but glued to a trailing character, explain that
335+
// rather than telling the publisher to add a token they can already see.
336+
if trailing, glued := mcpNameTokenGluedTrailing(string(body), serverName); glued {
337+
return fmt.Errorf("cargo package '%s' ownership validation failed: found 'mcp-name: %s' in the README, but it is immediately followed by %q rather than a boundary. The token must be followed by a space, newline, or an HTML tag — put it on its own line and publish a new version", pkg.Identifier, serverName, trailing)
338+
}
339+
290340
return fmt.Errorf("cargo package '%s' ownership validation failed. The server name '%s' must appear as 'mcp-name: %s' in the package README", pkg.Identifier, serverName, serverName)
291341
}
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
package registries
2+
3+
import (
4+
"net/url"
5+
"testing"
6+
7+
"github.com/modelcontextprotocol/registry/pkg/model"
8+
)
9+
10+
// TestCargoURLAllowed covers the SSRF allow-check: for the real crates.io base,
11+
// the host must be allow-listed AND the scheme/port must be https/default; for a
12+
// test (httptest) base only the host is checked so mocks keep working.
13+
func TestCargoURLAllowed(t *testing.T) {
14+
prodHosts := cargoAllowedHosts(model.RegistryURLCrates) // {crates.io, static.crates.io}
15+
mockBase := "http://127.0.0.1:54321"
16+
mockHosts := cargoAllowedHosts(mockBase) // {127.0.0.1}
17+
18+
cases := []struct {
19+
desc string
20+
raw string
21+
baseURL string
22+
hosts map[string]struct{}
23+
want bool
24+
}{
25+
{"prod: https static.crates.io", "https://static.crates.io/readmes/x/x.html", model.RegistryURLCrates, prodHosts, true},
26+
{"prod: https crates.io", "https://crates.io/api/v1/crates/x/1.0.0", model.RegistryURLCrates, prodHosts, true},
27+
{"prod: http downgrade rejected", "http://static.crates.io/x", model.RegistryURLCrates, prodHosts, false},
28+
{"prod: non-default port rejected", "https://static.crates.io:8443/x", model.RegistryURLCrates, prodHosts, false},
29+
{"prod: explicit 443 ok", "https://static.crates.io:443/x", model.RegistryURLCrates, prodHosts, true},
30+
{"prod: foreign host rejected", "https://evil.example/x", model.RegistryURLCrates, prodHosts, false},
31+
{"prod: userinfo host is evil rejected", "https://static.crates.io@evil.example/x", model.RegistryURLCrates, prodHosts, false},
32+
{"test base: mock host any scheme/port ok", "http://127.0.0.1:54321/readme-static/x", mockBase, mockHosts, true},
33+
{"test base: foreign host rejected", "http://127.0.0.2:54321/x", mockBase, mockHosts, false},
34+
}
35+
36+
for _, tc := range cases {
37+
t.Run(tc.desc, func(t *testing.T) {
38+
u, err := url.Parse(tc.raw)
39+
if err != nil {
40+
t.Fatalf("parse %q: %v", tc.raw, err)
41+
}
42+
if got := cargoURLAllowed(u, tc.baseURL, tc.hosts); got != tc.want {
43+
t.Fatalf("cargoURLAllowed(%q, base=%q) = %v, want %v", tc.raw, tc.baseURL, got, tc.want)
44+
}
45+
})
46+
}
47+
}

internal/validators/registries/cargo_test.go

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -340,6 +340,7 @@ func TestValidateCargoCombinedFixture(t *testing.T) {
340340
readmeStatus int
341341
readmeBody string
342342
versionExists bool // response for the /api/v1/crates/{n}/{v} existence probe (403 disambiguation)
343+
versionProbe int // if non-zero, the existence probe returns this status (overrides versionExists)
343344
wantErr bool
344345
wantContains []string
345346
wantNotContains []string
@@ -377,16 +378,30 @@ func TestValidateCargoCombinedFixture(t *testing.T) {
377378
wantNotContains: []string{"has no rendered README"},
378379
},
379380
{
380-
// Crate/version exists but has no rendered README: CDN 403 + existence
381-
// probe 200. Must NOT be reported as "not found".
381+
// Crate/version exists but the README CDN 403s: existence probe 200.
382+
// Must NOT be reported as "not found", and must not flatly assert the
383+
// README is absent (a 403 isn't definitive proof of that).
382384
name: "readme_403_no_readme",
383385
crateName: "combined-readme403-noreadme",
384386
version: "0.1.0",
385387
metaStatus: http.StatusOK,
386388
readmeStatus: http.StatusForbidden,
387389
versionExists: true,
388390
wantErr: true,
389-
wantContains: []string{"has no rendered README"},
391+
wantContains: []string{"exists on crates.io", "could not be retrieved"},
392+
wantNotContains: []string{"not found"},
393+
},
394+
{
395+
// CDN 403 + the existence probe itself is rate-limited (429): existence
396+
// is undetermined, so report transient/retryable, NOT "not found".
397+
name: "readme_403_probe_transient",
398+
crateName: "combined-readme403-probe429",
399+
version: "0.1.0",
400+
metaStatus: http.StatusOK,
401+
readmeStatus: http.StatusForbidden,
402+
versionProbe: http.StatusTooManyRequests,
403+
wantErr: true,
404+
wantContains: []string{"transient"},
390405
wantNotContains: []string{"not found"},
391406
},
392407
{
@@ -421,6 +436,18 @@ func TestValidateCargoCombinedFixture(t *testing.T) {
421436
wantErr: true,
422437
wantContains: []string{"ownership validation failed"},
423438
},
439+
{
440+
// Token present but glued to a trailing period — the error must explain
441+
// the boundary cause, not tell the publisher to add a token they can see.
442+
name: "glued_trailing_period_explained",
443+
crateName: "combined-glued",
444+
version: "0.1.0",
445+
metaStatus: http.StatusOK,
446+
readmeStatus: http.StatusOK,
447+
readmeBody: fmt.Sprintf("<html><body><p>mcp-name: %s.</p></body></html>", serverName),
448+
wantErr: true,
449+
wantContains: []string{"immediately followed by", `"."`},
450+
},
424451
}
425452

426453
// lastMetaPath captures the metadata request path seen by the handler so
@@ -450,10 +477,13 @@ func TestValidateCargoCombinedFixture(t *testing.T) {
450477
}
451478
// Existence probe used to disambiguate a README 403.
452479
if r.URL.Path == versionPath {
453-
if tt.versionExists {
480+
switch {
481+
case tt.versionProbe != 0:
482+
http.Error(w, "simulated probe status", tt.versionProbe)
483+
case tt.versionExists:
454484
w.Header().Set("Content-Type", "application/json")
455485
_ = json.NewEncoder(w).Encode(map[string]any{"version": map[string]string{"num": tt.version}})
456-
} else {
486+
default:
457487
http.Error(w, "not found", http.StatusNotFound)
458488
}
459489
return

0 commit comments

Comments
 (0)