Skip to content

Commit d813d2b

Browse files
authored
fix(validators): distinguish a missing package from a missing version on PyPI/NPM (#1411)
Fixes #553. The PyPI and npm validators fetch the version-specific metadata endpoint (`/pypi/{name}/{version}/json` and the npm equivalent) and report any non-200 as `<pkg> not found`. That endpoint 404s for two different reasons though: the package doesn't exist, or it exists and only the *version* is missing (e.g. a release that hasn't propagated yet). So a valid package reads as gone: ```diff - PyPI package 'requests' not found (status: 404) + PyPI package 'requests' exists, but version '99.99.99' was not found (status: 404). A newly published release can take a moment to appear on PyPI. Wait and retry, or publish version '99.99.99' before registering it ``` On a version 404 it probes the package-level endpoint (`/pypi/{name}/json`, `/{name}`) with a `HEAD` to tell the two apart, and reports 429/5xx as transient rather than "not found". `HEAD` so it reads a status without pulling the whole packument. The probe carries its own 3s deadline: it only refines the error message, so a hung probe must not stretch the validator past the ~10s-per-registry budget the publish path assumes. Same probe-and-classify shape as the existing cargo validator. The fetch is split into `validatePyPIPackage` / `validateNPMPackage` behind `export_test.go` so the branches are testable with `httptest`, the same seam cargo uses. ### Testing - [x] hermetic `httptest` for the status matrix (version-missing, package-missing, 5xx, 429, inconclusive probe, positive path, scoped npm); the mocks pin the expected method per endpoint (`GET` fetch, `HEAD` probe) - [x] hermetic deadline test: a hung probe is cut off at ~3s and reported as inconclusive, not "not found" - [x] existing live package tests still pass, now also hitting the HEAD probe; on a live 429/5xx they now `t.Skip` as inconclusive instead of flaking (deliberate: CI runs these against the real registries with no short-mode gating) - [x] `gofmt` / `go vet` / `golangci-lint` clean Out of scope: auto-retry for the propagation race, and SSRF redirect-pinning parity for the pypi/npm clients (pre-existing, shared with nuget).
1 parent 5d736c7 commit d813d2b

5 files changed

Lines changed: 551 additions & 4 deletions

File tree

internal/validators/registries/export_test.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,3 +7,14 @@ package registries
77
//
88
// Intended for cargo_test.go's positive-path and transient-error tests only.
99
var ValidateCargoREADME = validateCargoREADME
10+
11+
// ValidatePyPIPackage and ValidateNPMPackage expose the package-private
12+
// validatePyPIPackage / validateNPMPackage to the external _test package so
13+
// httptest-driven tests can exercise the metadata fetch and status-disambiguation
14+
// pipeline (missing package vs missing/unpropagated version vs transient upstream)
15+
// against a mock server, bypassing the exact-baseURL guard that the public
16+
// ValidatePyPI / ValidateNPM enforce.
17+
var (
18+
ValidatePyPIPackage = validatePyPIPackage
19+
ValidateNPMPackage = validateNPMPackage
20+
)

internal/validators/registries/npm.go

Lines changed: 97 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,14 @@ func ValidateNPM(ctx context.Context, pkg model.Package, serverName string) erro
5252
pkg.RegistryBaseURL, model.RegistryTypeNPM, model.RegistryURLNPM)
5353
}
5454

55+
return validateNPMPackage(ctx, pkg, serverName)
56+
}
57+
58+
// validateNPMPackage performs the version-metadata fetch and the mcpName check.
59+
// It is split out from ValidateNPM so that httptest-based tests can drive the HTTP
60+
// pipeline against a mock server (exposed via export_test.go), bypassing the
61+
// exact-baseURL guard that ValidateNPM enforces for callers.
62+
func validateNPMPackage(ctx context.Context, pkg model.Package, serverName string) error {
5563
client := &http.Client{Timeout: 10 * time.Second}
5664

5765
requestURL := pkg.RegistryBaseURL + "/" + url.PathEscape(pkg.Identifier) + "/" + url.PathEscape(pkg.Version)
@@ -60,7 +68,7 @@ func ValidateNPM(ctx context.Context, pkg model.Package, serverName string) erro
6068
return fmt.Errorf("failed to create request: %w", err)
6169
}
6270

63-
req.Header.Set("User-Agent", "MCP-Registry-Validator/1.0")
71+
req.Header.Set("User-Agent", userAgent)
6472
req.Header.Set("Accept", "application/json")
6573

6674
resp, err := client.Do(req)
@@ -70,7 +78,7 @@ func ValidateNPM(ctx context.Context, pkg model.Package, serverName string) erro
7078
defer resp.Body.Close()
7179

7280
if resp.StatusCode != http.StatusOK {
73-
return fmt.Errorf("NPM package '%s' not found (status: %d)", pkg.Identifier, resp.StatusCode)
81+
return npmFetchStatusError(ctx, client, pkg, resp.StatusCode)
7482
}
7583

7684
var npmResp NPMPackageResponse
@@ -88,3 +96,90 @@ func ValidateNPM(ctx context.Context, pkg model.Package, serverName string) erro
8896

8997
return nil
9098
}
99+
100+
// npmPackageState is the outcome of probing the package-level NPM metadata
101+
// endpoint, used to disambiguate a 404 from the version-specific endpoint.
102+
type npmPackageState int
103+
104+
const (
105+
// npmPackageUnknown: the probe returned a status we can't classify.
106+
npmPackageUnknown npmPackageState = iota
107+
// npmPackageExists: the package exists (200) but the requested version does not.
108+
npmPackageExists
109+
// npmPackageMissing: the package itself does not exist (404).
110+
npmPackageMissing
111+
// npmPackageTransient: the probe failed for a retryable reason (network error,
112+
// 429, or 5xx). Existence is undetermined and the caller should not report
113+
// "not found".
114+
npmPackageTransient
115+
)
116+
117+
// npmFetchStatusError maps a non-200 status from the version-specific metadata
118+
// endpoint to a caller-actionable error. A 404 is delegated to npmVersion404Error
119+
// for disambiguation. 429/5xx are transient, not "not found".
120+
func npmFetchStatusError(ctx context.Context, client *http.Client, pkg model.Package, status int) error {
121+
switch {
122+
case status == http.StatusNotFound:
123+
return npmVersion404Error(ctx, client, pkg)
124+
case status == http.StatusTooManyRequests:
125+
return fmt.Errorf("NPM rate-limited the metadata request for package '%s' (status: 429). Likely transient, retry later", pkg.Identifier)
126+
case status >= 500 && status < 600:
127+
return fmt.Errorf("NPM upstream error fetching metadata for package '%s' (status: %d). Likely transient, retry later", pkg.Identifier, status)
128+
default:
129+
return fmt.Errorf("NPM package '%s' metadata fetch failed (status: %d)", pkg.Identifier, status)
130+
}
131+
}
132+
133+
// npmVersion404Error disambiguates a 404 from the version-specific endpoint: a
134+
// genuinely-missing package versus a package that exists but whose requested
135+
// version is absent (commonly because a freshly published release has not yet
136+
// propagated). It probes the package-level endpoint so the publisher gets an
137+
// actionable message rather than a blanket "not found".
138+
func npmVersion404Error(ctx context.Context, client *http.Client, pkg model.Package) error {
139+
switch probeNPMPackage(ctx, client, pkg.RegistryBaseURL, pkg.Identifier) {
140+
case npmPackageExists:
141+
return fmt.Errorf("NPM package '%s' exists, but version '%s' was not found (status: 404). A newly published release can take a moment to appear on the registry. Wait and retry, or publish version '%s' before registering it", pkg.Identifier, pkg.Version, pkg.Version)
142+
case npmPackageMissing:
143+
return fmt.Errorf("NPM package '%s' not found (status: 404)", pkg.Identifier)
144+
case npmPackageTransient:
145+
return fmt.Errorf("NPM could not confirm package '%s' version '%s' (version status: 404, package check inconclusive). Likely transient, retry later", pkg.Identifier, pkg.Version)
146+
case npmPackageUnknown:
147+
// Probe returned an unclassifiable status, so fall through to the
148+
// best-effort message below.
149+
}
150+
return fmt.Errorf("NPM package '%s' version '%s' not found (status: 404)", pkg.Identifier, pkg.Version)
151+
}
152+
153+
// probeNPMPackage checks whether a package exists on the NPM registry regardless
154+
// of version, with a HEAD request to the package-level endpoint (/{name}). Only
155+
// the status code is used, so HEAD avoids downloading the (large) packument.
156+
func probeNPMPackage(ctx context.Context, client *http.Client, baseURL, identifier string) npmPackageState {
157+
// The probe only refines the 404 error message, so it must not extend the
158+
// validator's worst case by another full client timeout.
159+
ctx, cancel := context.WithTimeout(ctx, 3*time.Second)
160+
defer cancel()
161+
162+
probeURL := baseURL + "/" + url.PathEscape(identifier)
163+
req, err := http.NewRequestWithContext(ctx, http.MethodHead, probeURL, nil)
164+
if err != nil {
165+
return npmPackageUnknown
166+
}
167+
req.Header.Set("User-Agent", userAgent)
168+
169+
resp, err := client.Do(req)
170+
if err != nil {
171+
return npmPackageTransient
172+
}
173+
defer resp.Body.Close()
174+
175+
switch {
176+
case resp.StatusCode == http.StatusOK:
177+
return npmPackageExists
178+
case resp.StatusCode == http.StatusNotFound:
179+
return npmPackageMissing
180+
case resp.StatusCode == http.StatusTooManyRequests, resp.StatusCode >= 500 && resp.StatusCode < 600:
181+
return npmPackageTransient
182+
default:
183+
return npmPackageUnknown
184+
}
185+
}

internal/validators/registries/npm_test.go

Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,186 @@ package registries_test
22

33
import (
44
"context"
5+
"fmt"
6+
"io"
7+
"net/http"
8+
"net/http/httptest"
9+
"strings"
510
"testing"
11+
"time"
612

713
"github.com/modelcontextprotocol/registry/internal/validators/registries"
814
"github.com/modelcontextprotocol/registry/pkg/model"
915
"github.com/stretchr/testify/assert"
1016
)
1117

18+
// newNPMMock stands in for registry.npmjs.org: it routes the version fetch and
19+
// package probe by path shape and returns the given statuses (versionBody used on 200).
20+
func newNPMMock(versionStatus int, versionBody string, packageStatus int) *httptest.Server {
21+
return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
22+
// Route on the escaped path: a scoped name like @scope/pkg is sent as the
23+
// single segment @scope%2Fpkg, and decoding it would wrongly split it in two.
24+
parts := strings.Split(strings.Trim(r.URL.EscapedPath(), "/"), "/")
25+
// Pin the method per endpoint (GET fetch, HEAD probe) so a method
26+
// regression in the validator surfaces as a 405 instead of passing.
27+
switch len(parts) {
28+
case 2: // /{name}/{version}
29+
if r.Method != http.MethodGet {
30+
w.WriteHeader(http.StatusMethodNotAllowed)
31+
return
32+
}
33+
if versionStatus != http.StatusOK {
34+
w.WriteHeader(versionStatus)
35+
return
36+
}
37+
w.Header().Set("Content-Type", "application/json")
38+
_, _ = io.WriteString(w, versionBody)
39+
case 1: // /{name} (package-existence probe)
40+
if r.Method != http.MethodHead {
41+
w.WriteHeader(http.StatusMethodNotAllowed)
42+
return
43+
}
44+
w.WriteHeader(packageStatus)
45+
default:
46+
w.WriteHeader(http.StatusInternalServerError)
47+
}
48+
}))
49+
}
50+
51+
// TestValidateNPM_VersionNotYetVisible is the #553 regression for npm: version 404s
52+
// while the package exists, so the error must report a missing version, not a missing package.
53+
func TestValidateNPM_VersionNotYetVisible(t *testing.T) {
54+
ctx := context.Background()
55+
mock := newNPMMock(http.StatusNotFound, "", http.StatusOK)
56+
defer mock.Close()
57+
58+
pkg := model.Package{RegistryType: model.RegistryTypeNPM, RegistryBaseURL: mock.URL, Identifier: "demo-pkg", Version: "9.9.9"}
59+
err := registries.ValidateNPMPackage(ctx, pkg, "io.github.test/demo")
60+
assert.Error(t, err)
61+
assert.Contains(t, err.Error(), "exists, but version '9.9.9'", "package-exists/version-missing must be distinguished from package-missing")
62+
}
63+
64+
// TestValidateNPM_PackageMissing: both endpoints 404, so "not found" is correct.
65+
func TestValidateNPM_PackageMissing(t *testing.T) {
66+
ctx := context.Background()
67+
mock := newNPMMock(http.StatusNotFound, "", http.StatusNotFound)
68+
defer mock.Close()
69+
70+
pkg := model.Package{RegistryType: model.RegistryTypeNPM, RegistryBaseURL: mock.URL, Identifier: "demo-pkg", Version: "1.0.0"}
71+
err := registries.ValidateNPMPackage(ctx, pkg, "io.github.test/demo")
72+
assert.Error(t, err)
73+
assert.Contains(t, err.Error(), "not found")
74+
assert.NotContains(t, err.Error(), "exists, but version", "a genuinely missing package must not claim the version exists")
75+
}
76+
77+
// TestValidateNPM_TransientUpstream: a 5xx on the version fetch must be retryable,
78+
// not "not found".
79+
func TestValidateNPM_TransientUpstream(t *testing.T) {
80+
ctx := context.Background()
81+
mock := newNPMMock(http.StatusBadGateway, "", http.StatusOK)
82+
defer mock.Close()
83+
84+
pkg := model.Package{RegistryType: model.RegistryTypeNPM, RegistryBaseURL: mock.URL, Identifier: "demo-pkg", Version: "1.0.0"}
85+
err := registries.ValidateNPMPackage(ctx, pkg, "io.github.test/demo")
86+
assert.Error(t, err)
87+
assert.Contains(t, err.Error(), "transient")
88+
assert.NotContains(t, err.Error(), "not found", "transient upstream errors must not be reported as 'not found'")
89+
}
90+
91+
// TestValidateNPM_VersionNotFoundProbeInconclusive: version 404 plus a transient
92+
// probe (503) leaves existence undetermined, so the validator must not say "not found".
93+
func TestValidateNPM_VersionNotFoundProbeInconclusive(t *testing.T) {
94+
ctx := context.Background()
95+
mock := newNPMMock(http.StatusNotFound, "", http.StatusServiceUnavailable)
96+
defer mock.Close()
97+
98+
pkg := model.Package{RegistryType: model.RegistryTypeNPM, RegistryBaseURL: mock.URL, Identifier: "demo-pkg", Version: "1.0.0"}
99+
err := registries.ValidateNPMPackage(ctx, pkg, "io.github.test/demo")
100+
assert.Error(t, err)
101+
assert.Contains(t, err.Error(), "transient")
102+
assert.NotContains(t, err.Error(), "not found", "an inconclusive probe must not assert the package is missing")
103+
}
104+
105+
// TestValidateNPM_ProbeDeadlineBounded: a hung probe must be cut off by the
106+
// probe's own short deadline instead of riding out the client's full 10s
107+
// timeout, and the cutoff must read as inconclusive rather than "not found".
108+
func TestValidateNPM_ProbeDeadlineBounded(t *testing.T) {
109+
ctx := context.Background()
110+
mock := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
111+
parts := strings.Split(strings.Trim(r.URL.EscapedPath(), "/"), "/")
112+
if len(parts) == 2 { // version fetch
113+
w.WriteHeader(http.StatusNotFound)
114+
return
115+
}
116+
// Package probe: hang until the client gives up.
117+
<-r.Context().Done()
118+
}))
119+
defer mock.Close()
120+
121+
pkg := model.Package{RegistryType: model.RegistryTypeNPM, RegistryBaseURL: mock.URL, Identifier: "demo-pkg", Version: "1.0.0"}
122+
start := time.Now()
123+
err := registries.ValidateNPMPackage(ctx, pkg, "io.github.test/demo")
124+
elapsed := time.Since(start)
125+
assert.Error(t, err)
126+
assert.Contains(t, err.Error(), "transient", "a probe cut off by its deadline is inconclusive, not 'not found'")
127+
assert.Less(t, elapsed, 8*time.Second, "a hung probe must be bounded by the probe deadline, not the client timeout")
128+
}
129+
130+
// TestValidateNPM_ScopedVersionNotYetVisible covers scoped names (@scope/name), the
131+
// escaped-single-segment case, through the version fetch and the HEAD probe.
132+
func TestValidateNPM_ScopedVersionNotYetVisible(t *testing.T) {
133+
ctx := context.Background()
134+
mock := newNPMMock(http.StatusNotFound, "", http.StatusOK)
135+
defer mock.Close()
136+
137+
pkg := model.Package{RegistryType: model.RegistryTypeNPM, RegistryBaseURL: mock.URL, Identifier: "@scope/demo", Version: "9.9.9"}
138+
err := registries.ValidateNPMPackage(ctx, pkg, "io.github.test/demo")
139+
assert.Error(t, err)
140+
assert.Contains(t, err.Error(), "exists, but version '9.9.9'", "scoped names must route correctly through both endpoints")
141+
}
142+
143+
// TestValidateNPM_VersionEndpointRateLimited: a 429 on the version fetch is reported
144+
// as rate-limited/transient.
145+
func TestValidateNPM_VersionEndpointRateLimited(t *testing.T) {
146+
ctx := context.Background()
147+
mock := newNPMMock(http.StatusTooManyRequests, "", http.StatusOK)
148+
defer mock.Close()
149+
150+
pkg := model.Package{RegistryType: model.RegistryTypeNPM, RegistryBaseURL: mock.URL, Identifier: "demo-pkg", Version: "1.0.0"}
151+
err := registries.ValidateNPMPackage(ctx, pkg, "io.github.test/demo")
152+
assert.Error(t, err)
153+
assert.Contains(t, err.Error(), "rate-limited")
154+
assert.NotContains(t, err.Error(), "not found")
155+
}
156+
157+
// TestValidateNPM_VersionNotFoundProbeUnclassified: the version 404s and the probe
158+
// returns an unclassifiable status, so the validator falls back to a plain
159+
// version-not-found message.
160+
func TestValidateNPM_VersionNotFoundProbeUnclassified(t *testing.T) {
161+
ctx := context.Background()
162+
mock := newNPMMock(http.StatusNotFound, "", http.StatusTeapot)
163+
defer mock.Close()
164+
165+
pkg := model.Package{RegistryType: model.RegistryTypeNPM, RegistryBaseURL: mock.URL, Identifier: "demo-pkg", Version: "1.0.0"}
166+
err := registries.ValidateNPMPackage(ctx, pkg, "io.github.test/demo")
167+
assert.Error(t, err)
168+
assert.Contains(t, err.Error(), "version '1.0.0' not found")
169+
assert.NotContains(t, err.Error(), "exists, but version")
170+
}
171+
172+
// TestValidateNPM_PositivePathMock: a version response with the matching mcpName validates.
173+
func TestValidateNPM_PositivePathMock(t *testing.T) {
174+
ctx := context.Background()
175+
const serverName = "io.github.test/demo"
176+
body := fmt.Sprintf(`{"mcpName":%q}`, serverName)
177+
mock := newNPMMock(http.StatusOK, body, http.StatusOK)
178+
defer mock.Close()
179+
180+
pkg := model.Package{RegistryType: model.RegistryTypeNPM, RegistryBaseURL: mock.URL, Identifier: "demo-pkg", Version: "1.0.0"}
181+
err := registries.ValidateNPMPackage(ctx, pkg, serverName)
182+
assert.NoError(t, err, "a version response with the matching mcpName should validate")
183+
}
184+
12185
func TestValidateNPM_RealPackages(t *testing.T) {
13186
ctx := context.Background()
14187

@@ -126,6 +299,12 @@ func TestValidateNPM_RealPackages(t *testing.T) {
126299

127300
err := registries.ValidateNPM(ctx, pkg, tt.serverName)
128301

302+
// A live 429/5xx from the registry is inconclusive, not a failure
303+
// of the case under test.
304+
if err != nil && strings.Contains(err.Error(), "retry later") {
305+
t.Skipf("transient registry response: %v", err)
306+
}
307+
129308
if tt.expectError {
130309
assert.Error(t, err)
131310
assert.Contains(t, err.Error(), tt.errorMessage)

0 commit comments

Comments
 (0)