d813d2b805Fixes #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).
316 lines
12 KiB
Go
316 lines
12 KiB
Go
package registries_test
|
|
|
|
import (
|
|
"context"
|
|
"fmt"
|
|
"io"
|
|
"net/http"
|
|
"net/http/httptest"
|
|
"strings"
|
|
"testing"
|
|
"time"
|
|
|
|
"github.com/modelcontextprotocol/registry/internal/validators/registries"
|
|
"github.com/modelcontextprotocol/registry/pkg/model"
|
|
"github.com/stretchr/testify/assert"
|
|
)
|
|
|
|
// newNPMMock stands in for registry.npmjs.org: it routes the version fetch and
|
|
// package probe by path shape and returns the given statuses (versionBody used on 200).
|
|
func newNPMMock(versionStatus int, versionBody string, packageStatus int) *httptest.Server {
|
|
return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
|
// Route on the escaped path: a scoped name like @scope/pkg is sent as the
|
|
// single segment @scope%2Fpkg, and decoding it would wrongly split it in two.
|
|
parts := strings.Split(strings.Trim(r.URL.EscapedPath(), "/"), "/")
|
|
// Pin the method per endpoint (GET fetch, HEAD probe) so a method
|
|
// regression in the validator surfaces as a 405 instead of passing.
|
|
switch len(parts) {
|
|
case 2: // /{name}/{version}
|
|
if r.Method != http.MethodGet {
|
|
w.WriteHeader(http.StatusMethodNotAllowed)
|
|
return
|
|
}
|
|
if versionStatus != http.StatusOK {
|
|
w.WriteHeader(versionStatus)
|
|
return
|
|
}
|
|
w.Header().Set("Content-Type", "application/json")
|
|
_, _ = io.WriteString(w, versionBody)
|
|
case 1: // /{name} (package-existence probe)
|
|
if r.Method != http.MethodHead {
|
|
w.WriteHeader(http.StatusMethodNotAllowed)
|
|
return
|
|
}
|
|
w.WriteHeader(packageStatus)
|
|
default:
|
|
w.WriteHeader(http.StatusInternalServerError)
|
|
}
|
|
}))
|
|
}
|
|
|
|
// TestValidateNPM_VersionNotYetVisible is the #553 regression for npm: version 404s
|
|
// while the package exists, so the error must report a missing version, not a missing package.
|
|
func TestValidateNPM_VersionNotYetVisible(t *testing.T) {
|
|
ctx := context.Background()
|
|
mock := newNPMMock(http.StatusNotFound, "", http.StatusOK)
|
|
defer mock.Close()
|
|
|
|
pkg := model.Package{RegistryType: model.RegistryTypeNPM, RegistryBaseURL: mock.URL, Identifier: "demo-pkg", Version: "9.9.9"}
|
|
err := registries.ValidateNPMPackage(ctx, pkg, "io.github.test/demo")
|
|
assert.Error(t, err)
|
|
assert.Contains(t, err.Error(), "exists, but version '9.9.9'", "package-exists/version-missing must be distinguished from package-missing")
|
|
}
|
|
|
|
// TestValidateNPM_PackageMissing: both endpoints 404, so "not found" is correct.
|
|
func TestValidateNPM_PackageMissing(t *testing.T) {
|
|
ctx := context.Background()
|
|
mock := newNPMMock(http.StatusNotFound, "", http.StatusNotFound)
|
|
defer mock.Close()
|
|
|
|
pkg := model.Package{RegistryType: model.RegistryTypeNPM, RegistryBaseURL: mock.URL, Identifier: "demo-pkg", Version: "1.0.0"}
|
|
err := registries.ValidateNPMPackage(ctx, pkg, "io.github.test/demo")
|
|
assert.Error(t, err)
|
|
assert.Contains(t, err.Error(), "not found")
|
|
assert.NotContains(t, err.Error(), "exists, but version", "a genuinely missing package must not claim the version exists")
|
|
}
|
|
|
|
// TestValidateNPM_TransientUpstream: a 5xx on the version fetch must be retryable,
|
|
// not "not found".
|
|
func TestValidateNPM_TransientUpstream(t *testing.T) {
|
|
ctx := context.Background()
|
|
mock := newNPMMock(http.StatusBadGateway, "", http.StatusOK)
|
|
defer mock.Close()
|
|
|
|
pkg := model.Package{RegistryType: model.RegistryTypeNPM, RegistryBaseURL: mock.URL, Identifier: "demo-pkg", Version: "1.0.0"}
|
|
err := registries.ValidateNPMPackage(ctx, pkg, "io.github.test/demo")
|
|
assert.Error(t, err)
|
|
assert.Contains(t, err.Error(), "transient")
|
|
assert.NotContains(t, err.Error(), "not found", "transient upstream errors must not be reported as 'not found'")
|
|
}
|
|
|
|
// TestValidateNPM_VersionNotFoundProbeInconclusive: version 404 plus a transient
|
|
// probe (503) leaves existence undetermined, so the validator must not say "not found".
|
|
func TestValidateNPM_VersionNotFoundProbeInconclusive(t *testing.T) {
|
|
ctx := context.Background()
|
|
mock := newNPMMock(http.StatusNotFound, "", http.StatusServiceUnavailable)
|
|
defer mock.Close()
|
|
|
|
pkg := model.Package{RegistryType: model.RegistryTypeNPM, RegistryBaseURL: mock.URL, Identifier: "demo-pkg", Version: "1.0.0"}
|
|
err := registries.ValidateNPMPackage(ctx, pkg, "io.github.test/demo")
|
|
assert.Error(t, err)
|
|
assert.Contains(t, err.Error(), "transient")
|
|
assert.NotContains(t, err.Error(), "not found", "an inconclusive probe must not assert the package is missing")
|
|
}
|
|
|
|
// TestValidateNPM_ProbeDeadlineBounded: a hung probe must be cut off by the
|
|
// probe's own short deadline instead of riding out the client's full 10s
|
|
// timeout, and the cutoff must read as inconclusive rather than "not found".
|
|
func TestValidateNPM_ProbeDeadlineBounded(t *testing.T) {
|
|
ctx := context.Background()
|
|
mock := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
|
parts := strings.Split(strings.Trim(r.URL.EscapedPath(), "/"), "/")
|
|
if len(parts) == 2 { // version fetch
|
|
w.WriteHeader(http.StatusNotFound)
|
|
return
|
|
}
|
|
// Package probe: hang until the client gives up.
|
|
<-r.Context().Done()
|
|
}))
|
|
defer mock.Close()
|
|
|
|
pkg := model.Package{RegistryType: model.RegistryTypeNPM, RegistryBaseURL: mock.URL, Identifier: "demo-pkg", Version: "1.0.0"}
|
|
start := time.Now()
|
|
err := registries.ValidateNPMPackage(ctx, pkg, "io.github.test/demo")
|
|
elapsed := time.Since(start)
|
|
assert.Error(t, err)
|
|
assert.Contains(t, err.Error(), "transient", "a probe cut off by its deadline is inconclusive, not 'not found'")
|
|
assert.Less(t, elapsed, 8*time.Second, "a hung probe must be bounded by the probe deadline, not the client timeout")
|
|
}
|
|
|
|
// TestValidateNPM_ScopedVersionNotYetVisible covers scoped names (@scope/name), the
|
|
// escaped-single-segment case, through the version fetch and the HEAD probe.
|
|
func TestValidateNPM_ScopedVersionNotYetVisible(t *testing.T) {
|
|
ctx := context.Background()
|
|
mock := newNPMMock(http.StatusNotFound, "", http.StatusOK)
|
|
defer mock.Close()
|
|
|
|
pkg := model.Package{RegistryType: model.RegistryTypeNPM, RegistryBaseURL: mock.URL, Identifier: "@scope/demo", Version: "9.9.9"}
|
|
err := registries.ValidateNPMPackage(ctx, pkg, "io.github.test/demo")
|
|
assert.Error(t, err)
|
|
assert.Contains(t, err.Error(), "exists, but version '9.9.9'", "scoped names must route correctly through both endpoints")
|
|
}
|
|
|
|
// TestValidateNPM_VersionEndpointRateLimited: a 429 on the version fetch is reported
|
|
// as rate-limited/transient.
|
|
func TestValidateNPM_VersionEndpointRateLimited(t *testing.T) {
|
|
ctx := context.Background()
|
|
mock := newNPMMock(http.StatusTooManyRequests, "", http.StatusOK)
|
|
defer mock.Close()
|
|
|
|
pkg := model.Package{RegistryType: model.RegistryTypeNPM, RegistryBaseURL: mock.URL, Identifier: "demo-pkg", Version: "1.0.0"}
|
|
err := registries.ValidateNPMPackage(ctx, pkg, "io.github.test/demo")
|
|
assert.Error(t, err)
|
|
assert.Contains(t, err.Error(), "rate-limited")
|
|
assert.NotContains(t, err.Error(), "not found")
|
|
}
|
|
|
|
// TestValidateNPM_VersionNotFoundProbeUnclassified: the version 404s and the probe
|
|
// returns an unclassifiable status, so the validator falls back to a plain
|
|
// version-not-found message.
|
|
func TestValidateNPM_VersionNotFoundProbeUnclassified(t *testing.T) {
|
|
ctx := context.Background()
|
|
mock := newNPMMock(http.StatusNotFound, "", http.StatusTeapot)
|
|
defer mock.Close()
|
|
|
|
pkg := model.Package{RegistryType: model.RegistryTypeNPM, RegistryBaseURL: mock.URL, Identifier: "demo-pkg", Version: "1.0.0"}
|
|
err := registries.ValidateNPMPackage(ctx, pkg, "io.github.test/demo")
|
|
assert.Error(t, err)
|
|
assert.Contains(t, err.Error(), "version '1.0.0' not found")
|
|
assert.NotContains(t, err.Error(), "exists, but version")
|
|
}
|
|
|
|
// TestValidateNPM_PositivePathMock: a version response with the matching mcpName validates.
|
|
func TestValidateNPM_PositivePathMock(t *testing.T) {
|
|
ctx := context.Background()
|
|
const serverName = "io.github.test/demo"
|
|
body := fmt.Sprintf(`{"mcpName":%q}`, serverName)
|
|
mock := newNPMMock(http.StatusOK, body, http.StatusOK)
|
|
defer mock.Close()
|
|
|
|
pkg := model.Package{RegistryType: model.RegistryTypeNPM, RegistryBaseURL: mock.URL, Identifier: "demo-pkg", Version: "1.0.0"}
|
|
err := registries.ValidateNPMPackage(ctx, pkg, serverName)
|
|
assert.NoError(t, err, "a version response with the matching mcpName should validate")
|
|
}
|
|
|
|
func TestValidateNPM_RealPackages(t *testing.T) {
|
|
ctx := context.Background()
|
|
|
|
tests := []struct {
|
|
name string
|
|
packageName string
|
|
version string
|
|
serverName string
|
|
expectError bool
|
|
errorMessage string
|
|
}{
|
|
{
|
|
name: "empty package identifier should fail",
|
|
packageName: "",
|
|
version: "1.0.0",
|
|
serverName: "com.example/test",
|
|
expectError: true,
|
|
errorMessage: "package identifier is required for NPM packages",
|
|
},
|
|
{
|
|
name: "empty package version should fail",
|
|
packageName: "test-package",
|
|
version: "",
|
|
serverName: "com.example/test",
|
|
expectError: true,
|
|
errorMessage: "package version is required for NPM packages",
|
|
},
|
|
{
|
|
name: "both empty identifier and version should fail with identifier error first",
|
|
packageName: "",
|
|
version: "",
|
|
serverName: "com.example/test",
|
|
expectError: true,
|
|
errorMessage: "package identifier is required for NPM packages",
|
|
},
|
|
{
|
|
name: "non-existent package should fail",
|
|
packageName: generateRandomPackageName(),
|
|
version: "1.0.0",
|
|
serverName: "com.example/test",
|
|
expectError: true,
|
|
errorMessage: "not found",
|
|
},
|
|
{
|
|
name: "real package without mcpName should fail",
|
|
packageName: "express", // Popular package without mcpName field
|
|
version: "4.18.2",
|
|
serverName: "com.example/test",
|
|
expectError: true,
|
|
errorMessage: "missing required 'mcpName' field",
|
|
},
|
|
{
|
|
name: "real package without mcpName should fail",
|
|
packageName: "lodash", // Another popular package
|
|
version: "4.17.21",
|
|
serverName: "com.example/completely-different-name",
|
|
expectError: true,
|
|
errorMessage: "missing required 'mcpName' field",
|
|
},
|
|
{
|
|
name: "real package without mcpName should fail",
|
|
packageName: "airtable-mcp-server",
|
|
version: "1.5.0",
|
|
serverName: "io.github.domdomegg/airtable-mcp-server",
|
|
expectError: true,
|
|
errorMessage: "missing required 'mcpName' field",
|
|
},
|
|
{
|
|
name: "real package with incorrect mcpName should fail",
|
|
packageName: "airtable-mcp-server",
|
|
version: "1.7.2",
|
|
serverName: "io.github.not-domdomegg/airtable-mcp-server",
|
|
expectError: true,
|
|
errorMessage: "Expected mcpName 'io.github.not-domdomegg/airtable-mcp-server', got 'io.github.domdomegg/airtable-mcp-server'",
|
|
},
|
|
{
|
|
name: "real package with correct mcpName should pass",
|
|
packageName: "airtable-mcp-server",
|
|
version: "1.7.2",
|
|
serverName: "io.github.domdomegg/airtable-mcp-server",
|
|
expectError: false,
|
|
},
|
|
{
|
|
name: "scoped package that doesn't exist should fail",
|
|
packageName: "@nonexistent-scope/nonexistent-package",
|
|
version: "1.0.0",
|
|
serverName: "com.example/test",
|
|
expectError: true,
|
|
errorMessage: "not found",
|
|
},
|
|
{
|
|
name: "scoped package without mcpName should fail",
|
|
packageName: "@types/node",
|
|
version: "20.0.0",
|
|
serverName: "com.example/test",
|
|
expectError: true,
|
|
errorMessage: "missing required 'mcpName' field",
|
|
},
|
|
{
|
|
name: "scoped package with mcpName should pass",
|
|
packageName: "@hellocoop/admin-mcp",
|
|
version: "1.5.7",
|
|
serverName: "io.github.hellocoop/admin-mcp",
|
|
expectError: false,
|
|
},
|
|
}
|
|
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
pkg := model.Package{
|
|
RegistryType: model.RegistryTypeNPM,
|
|
Identifier: tt.packageName,
|
|
Version: tt.version,
|
|
}
|
|
|
|
err := registries.ValidateNPM(ctx, pkg, tt.serverName)
|
|
|
|
// A live 429/5xx from the registry is inconclusive, not a failure
|
|
// of the case under test.
|
|
if err != nil && strings.Contains(err.Error(), "retry later") {
|
|
t.Skipf("transient registry response: %v", err)
|
|
}
|
|
|
|
if tt.expectError {
|
|
assert.Error(t, err)
|
|
assert.Contains(t, err.Error(), tt.errorMessage)
|
|
} else {
|
|
assert.NoError(t, err)
|
|
}
|
|
})
|
|
}
|
|
}
|