From e67997c1772845819d07ec432e671c870a702dc3 Mon Sep 17 00:00:00 2001 From: adam jones Date: Wed, 3 Sep 2025 16:01:56 +0100 Subject: [PATCH] Validate server registry urls are allowlisted for that registry type (#348) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Closes a validation gap that allowed servers to be published with unsupported registry types and base URLs. ## Changes - **Registry Type Validation**: Only allow `npm`, `pypi`, `oci`, `nuget`, `mcpb` - **Registry Base URL Validation**: Enforce strict allowlist of official registry URLs - **Required Fields**: Registry type and base URL required when package identifier provided - **Comprehensive Tests**: Clear test structure showing valid vs invalid cases side-by-side ## Validation Rules - `npm` → `https://registry.npmjs.org` - `pypi` → `https://pypi.org` - `oci` → `https://docker.io` - `nuget` → `https://api.nuget.org` - `mcpb` → `https://github.com` or `https://gitlab.com` All other combinations are rejected with clear error messages. --- internal/validators/constants.go | 5 + internal/validators/validators.go | 181 +++++++++++++++++- internal/validators/validators_test.go | 249 +++++++++++++++++++++++++ 3 files changed, 430 insertions(+), 5 deletions(-) diff --git a/internal/validators/constants.go b/internal/validators/constants.go index 37ce46e..a1d1d98 100644 --- a/internal/validators/constants.go +++ b/internal/validators/constants.go @@ -13,6 +13,11 @@ var ( // Remote validation errors ErrInvalidRemoteURL = errors.New("invalid remote URL") + // Registry validation errors + ErrUnsupportedRegistryType = errors.New("unsupported registry type") + ErrUnsupportedRegistryBaseURL = errors.New("unsupported registry base URL") + ErrMismatchedRegistryTypeAndURL = errors.New("registry type and base URL do not match") + // Argument validation errors ErrNamedArgumentNameRequired = errors.New("named argument name is required") ErrInvalidNamedArgumentName = errors.New("invalid named argument name format") diff --git a/internal/validators/validators.go b/internal/validators/validators.go index c278cce..9d9c72a 100644 --- a/internal/validators/validators.go +++ b/internal/validators/validators.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "net/url" + "regexp" "slices" "strings" @@ -141,7 +142,13 @@ func validateRemote(obj *model.Remote) error { return nil } -func validateMCPBPackage(host string) error { +func validateMCPBPackage(fullURL string) error { + parsedURL, err := url.Parse(fullURL) + if err != nil { + return fmt.Errorf("invalid MCPB package URL: %w", err) + } + + host := strings.ToLower(parsedURL.Host) allowedHosts := []string{ "github.com", "www.github.com", @@ -161,12 +168,178 @@ func validateMCPBPackage(host string) error { return fmt.Errorf("MCPB packages must be hosted on allowlisted providers (GitHub or GitLab). Host '%s' is not allowed", host) } + // Validate URL path is a proper release URL with strict structure validation + path := parsedURL.Path + switch host { + case "github.com", "www.github.com": + // GitHub release URLs must match: /owner/repo/releases/download/tag/filename + if !isValidGitHubReleaseURL(path) { + return fmt.Errorf("GitHub MCPB packages must be release assets following the pattern '/owner/repo/releases/download/tag/filename'") + } + case "gitlab.com", "www.gitlab.com": + // GitLab release URLs must match specific patterns + if !isValidGitLabReleaseURL(path) { + return fmt.Errorf("GitLab MCPB packages must be release assets following patterns '/owner/repo/-/releases/tag/downloads/filename' or '/owner/repo/-/package_files/id/download'") + } + } + return nil } +// getDefaultRegistryBaseURL returns the default registry base URL for a given registry type +func getDefaultRegistryBaseURL(registryType string) string { + defaultURLs := map[string]string{ + model.RegistryTypeNPM: model.RegistryURLNPM, + model.RegistryTypePyPI: model.RegistryURLPyPI, + model.RegistryTypeOCI: model.RegistryURLDocker, + model.RegistryTypeNuGet: model.RegistryURLNuGet, + } + + return defaultURLs[registryType] +} + +// isValidGitHubReleaseURL validates that a path follows the GitHub release asset pattern +// Pattern: /owner/repo/releases/download/tag/filename +func isValidGitHubReleaseURL(path string) bool { + // GitHub release URL pattern: /owner/repo/releases/download/tag/filename + // - owner: username or organization (1-39 chars, alphanumeric + hyphens, no consecutive hyphens) + // - repo: repository name (similar rules to owner) + // - tag: release tag (can contain various characters but not empty) + // - filename: asset filename (not empty) + pattern := `^/([a-zA-Z0-9]([a-zA-Z0-9\-]{0,37}[a-zA-Z0-9])?)/([a-zA-Z0-9._\-]+)/releases/download/([^/]+)/([^/]+)$` + matched, _ := regexp.MatchString(pattern, path) + return matched +} + +// isValidGitLabReleaseURL validates that a path follows GitLab release asset patterns +func isValidGitLabReleaseURL(path string) bool { + // GitLab release URL patterns: + // 1. /owner/repo/-/releases/tag/downloads/filename + // 2. /owner/repo/-/package_files/id/download + // 3. /group/subgroup/repo/-/releases/tag/downloads/filename (nested groups) + + // The key insight is that GitLab URLs have "/-/" as a delimiter that separates the + // project path from the GitLab-specific routes. Everything before "/-/" is the project path. + + // Pattern 1: Release downloads with /-/releases/tag/downloads/filename + releasePattern := `^/([a-zA-Z0-9._\-]+(?:/[a-zA-Z0-9._\-]+)*)/-/releases/([^/]+)/downloads/([^/]+)$` + if matched, _ := regexp.MatchString(releasePattern, path); matched { + return true + } + + // Pattern 2: Package files with /-/package_files/id/download + packagePattern := `^/([a-zA-Z0-9._\-]+(?:/[a-zA-Z0-9._\-]+)*)/-/package_files/([0-9]+)/download$` + if matched, _ := regexp.MatchString(packagePattern, path); matched { + return true + } + + return false +} + +// inferMCPBRegistryBaseURL infers the registry base URL from an MCPB identifier +func inferMCPBRegistryBaseURL(identifier string) string { + parsedURL, err := url.Parse(identifier) + if err != nil { + return "" + } + + host := strings.ToLower(parsedURL.Host) + switch host { + case "github.com", "www.github.com": + return model.RegistryURLGitHub + case "gitlab.com", "www.gitlab.com": + return model.RegistryURLGitLab + default: + return "" + } +} + +// validateRegistryType checks if the registry type is supported +func validateRegistryType(registryType string) error { + // Registry type is required + if registryType == "" { + return fmt.Errorf("%w: registry type is required", ErrUnsupportedRegistryType) + } + + supportedTypes := []string{ + model.RegistryTypeNPM, + model.RegistryTypePyPI, + model.RegistryTypeOCI, + model.RegistryTypeNuGet, + model.RegistryTypeMCPB, + } + + for _, supported := range supportedTypes { + if registryType == supported { + return nil + } + } + + return fmt.Errorf("%w: '%s'. Supported types: %v", ErrUnsupportedRegistryType, registryType, supportedTypes) +} + +// validateRegistryBaseURL checks if the registry base URL is valid for the given registry type +func validateRegistryBaseURL(registryType, baseURL string) error { + // Base URL is required for all registry types except MCPB (which uses direct URLs) + if baseURL == "" { + if registryType == model.RegistryTypeMCPB { + return nil // MCPB packages use direct URLs in the identifier + } + return fmt.Errorf("%w: registry base URL is required for registry type '%s'", ErrUnsupportedRegistryBaseURL, registryType) + } + + // Define expected base URLs for each registry type + expectedURLs := map[string][]string{ + model.RegistryTypeNPM: {model.RegistryURLNPM}, + model.RegistryTypePyPI: {model.RegistryURLPyPI}, + model.RegistryTypeOCI: {model.RegistryURLDocker}, + model.RegistryTypeNuGet: {model.RegistryURLNuGet}, + model.RegistryTypeMCPB: {model.RegistryURLGitHub, model.RegistryURLGitLab}, + } + + // Check if the base URL is valid for the registry type + if expectedURLsForType, exists := expectedURLs[registryType]; exists { + for _, expected := range expectedURLsForType { + if baseURL == expected { + return nil + } + } + return fmt.Errorf("%w: '%s' is not valid for registry type '%s'. Expected: %v", + ErrMismatchedRegistryTypeAndURL, baseURL, registryType, expectedURLsForType) + } + + // If registry type is not in our expected URLs map but base URL is provided, + // it's likely an unsupported base URL + return fmt.Errorf("%w: '%s'", ErrUnsupportedRegistryBaseURL, baseURL) +} + func validatePackage(pkg *model.Package) error { registryType := strings.ToLower(pkg.RegistryType) + // Only validate if package has an identifier (i.e., it's a real package reference) + if pkg.Identifier != "" { + // Validate registry type is supported + if err := validateRegistryType(registryType); err != nil { + return err + } + + // Apply defaults for registry base URL if empty + if pkg.RegistryBaseURL == "" { + if registryType == model.RegistryTypeMCPB { + // For MCPB, infer from identifier + pkg.RegistryBaseURL = inferMCPBRegistryBaseURL(pkg.Identifier) + } else { + // For other types, use default + pkg.RegistryBaseURL = getDefaultRegistryBaseURL(registryType) + } + } + + // Validate registry base URL matches the registry type + if err := validateRegistryBaseURL(registryType, pkg.RegistryBaseURL); err != nil { + return err + } + } + // For direct download packages (mcpb or direct URLs) if registryType == model.RegistryTypeMCPB || strings.HasPrefix(pkg.Identifier, "http://") || strings.HasPrefix(pkg.Identifier, "https://") { @@ -175,11 +348,9 @@ func validatePackage(pkg *model.Package) error { return fmt.Errorf("invalid package URL: %w", err) } - host := strings.ToLower(parsedURL.Host) - - // For MCPB packages, validate they're from allowed hosts + // For MCPB packages, validate they're from allowed hosts and are release URLs if registryType == model.RegistryTypeMCPB { - return validateMCPBPackage(host) + return validateMCPBPackage(pkg.Identifier) } // For other URL-based packages, just ensure it's valid diff --git a/internal/validators/validators_test.go b/internal/validators/validators_test.go index b3ae4a7..7ac2178 100644 --- a/internal/validators/validators_test.go +++ b/internal/validators/validators_test.go @@ -2,6 +2,7 @@ package validators_test import ( "fmt" + "strings" "testing" "github.com/modelcontextprotocol/registry/internal/validators" @@ -648,6 +649,185 @@ func TestValidateArgument_ValidValueFields(t *testing.T) { } // Helper function to create a valid server with a specific argument for testing +func TestValidate_RegistryTypes(t *testing.T) { + testCases := []struct { + name string + registryType string + baseURL string + identifier string + expectError bool + }{ + // Valid registry types (should pass) + {"valid_npm", model.RegistryTypeNPM, model.RegistryURLNPM, "test-package", false}, + {"valid_pypi", model.RegistryTypePyPI, model.RegistryURLPyPI, "test-package", false}, + {"valid_oci", model.RegistryTypeOCI, model.RegistryURLDocker, "test-package", false}, + {"valid_nuget", model.RegistryTypeNuGet, model.RegistryURLNuGet, "test-package", false}, + {"valid_mcpb_github", model.RegistryTypeMCPB, model.RegistryURLGitHub, "https://github.com/owner/repo/releases/download/v1.0.0/package.mcpb", false}, + {"valid_mcpb_gitlab", model.RegistryTypeMCPB, model.RegistryURLGitLab, "https://gitlab.com/owner/repo/-/releases/v1.0.0/downloads/package.mcpb", false}, + + // Invalid registry types (should fail) + {"invalid_maven", "maven", "https://example.com/registry", "test-package", true}, + {"invalid_cargo", "cargo", "https://example.com/registry", "test-package", true}, + {"invalid_gem", "gem", "https://example.com/registry", "test-package", true}, + {"invalid_invalid", "invalid", "https://example.com/registry", "test-package", true}, + {"invalid_unknown", "UNKNOWN", "https://example.com/registry", "test-package", true}, + {"invalid_custom", "custom-registry", "https://example.com/registry", "test-package", true}, + {"invalid_github", "github", "https://example.com/registry", "test-package", true}, // This is a source, not a registry type + {"invalid_docker", "docker", "https://example.com/registry", "test-package", true}, // Should be "oci" + {"invalid_empty", "", "https://example.com/registry", "test-package", true}, // Empty registry type + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + serverDetail := apiv0.ServerJSON{ + Name: "com.example/test-server", + Description: "A test server", + Repository: model.Repository{ + URL: "https://github.com/owner/repo", + Source: "github", + ID: "owner/repo", + }, + VersionDetail: model.VersionDetail{ + Version: "1.0.0", + }, + Packages: []model.Package{ + { + Identifier: tc.identifier, + RegistryType: tc.registryType, + RegistryBaseURL: tc.baseURL, + }, + }, + Remotes: []model.Remote{ + { + URL: "https://example.com/remote", + }, + }, + } + + err := validators.ValidateServerJSON(&serverDetail) + if tc.expectError { + assert.Error(t, err) + assert.Contains(t, err.Error(), validators.ErrUnsupportedRegistryType.Error()) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestValidate_RegistryBaseURLs(t *testing.T) { + testCases := []struct { + name string + registryType string + baseURL string + identifier string + expectError bool + }{ + // Invalid base URLs for specific registry types + {"npm_wrong_url", model.RegistryTypeNPM, "https://pypi.org", "test-package", true}, + {"pypi_wrong_url", model.RegistryTypePyPI, "https://registry.npmjs.org", "test-package", true}, + {"oci_wrong_url", model.RegistryTypeOCI, "https://registry.npmjs.org", "test-package", true}, + {"nuget_wrong_url", model.RegistryTypeNuGet, "https://docker.io", "test-package", true}, + {"mcpb_wrong_url", model.RegistryTypeMCPB, "https://evil.com", "https://github.com/owner/repo", true}, + {"mismatched_base_url_1", model.RegistryTypeNPM, model.RegistryURLDocker, "test-package", true}, + {"mismatched_base_url_2", model.RegistryTypeOCI, model.RegistryTypeNuGet, "test-package", true}, + + // Localhost URLs should be rejected - no development exceptions + {"localhost_npm", model.RegistryTypeNPM, "http://localhost:3000", "test-package", true}, + {"localhost_ip", model.RegistryTypePyPI, "http://127.0.0.1:8080", "test-package", true}, + + // Valid combinations (should pass) + {"valid_npm", model.RegistryTypeNPM, model.RegistryURLNPM, "test-package", false}, + {"valid_pypi", model.RegistryTypePyPI, model.RegistryURLPyPI, "test-package", false}, + {"valid_oci", model.RegistryTypeOCI, model.RegistryURLDocker, "test-package", false}, + {"valid_nuget", model.RegistryTypeNuGet, model.RegistryURLNuGet, "test-package", false}, + {"valid_mcpb_github", model.RegistryTypeMCPB, model.RegistryURLGitHub, "https://github.com/owner/repo/releases/download/v1.0.0/package.mcpb", false}, + {"valid_mcpb_gitlab", model.RegistryTypeMCPB, model.RegistryURLGitLab, "https://gitlab.com/owner/repo/-/releases/v1.0.0/downloads/package.mcpb", false}, + {"empty_base_url_npm", model.RegistryTypeNPM, "", "test-package", false}, // should be inferred + {"empty_base_url_nuget", model.RegistryTypeNuGet, "", "test-package", false}, // should be inferred + {"empty_base_url_mcpb", model.RegistryTypeMCPB, "", "https://github.com/owner/repo/releases/download/v1.0.0/package.mcpb", false}, + + // Trailing slash URLs should be rejected - strict exact match only + {"npm_trailing_slash", model.RegistryTypeNPM, "https://registry.npmjs.org/", "test-package", true}, + {"pypi_trailing_slash", model.RegistryTypePyPI, "https://pypi.org/", "test-package", true}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + serverDetail := apiv0.ServerJSON{ + Name: "com.example/test-server", + Description: "A test server", + Repository: model.Repository{ + URL: "https://github.com/owner/repo", + Source: "github", + ID: "owner/repo", + }, + VersionDetail: model.VersionDetail{ + Version: "1.0.0", + }, + Packages: []model.Package{ + { + Identifier: tc.identifier, + RegistryType: tc.registryType, + RegistryBaseURL: tc.baseURL, + }, + }, + Remotes: []model.Remote{ + { + URL: "https://example.com/remote", + }, + }, + } + + err := validators.ValidateServerJSON(&serverDetail) + if tc.expectError { + assert.Error(t, err) + // Check that the error is related to registry validation + errStr := err.Error() + assert.True(t, + strings.Contains(errStr, validators.ErrUnsupportedRegistryBaseURL.Error()) || + strings.Contains(errStr, validators.ErrMismatchedRegistryTypeAndURL.Error()), + "Expected registry validation error, got: %s", errStr) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestValidate_EmptyRegistryType(t *testing.T) { + // Test that empty registry type is rejected + serverDetail := apiv0.ServerJSON{ + Name: "com.example/test-server", + Description: "A test server", + Repository: model.Repository{ + URL: "https://github.com/owner/repo", + Source: "github", + ID: "owner/repo", + }, + VersionDetail: model.VersionDetail{ + Version: "1.0.0", + }, + Packages: []model.Package{ + { + Identifier: "test-package", + RegistryType: "", // Empty registry type + RegistryBaseURL: "", + }, + }, + Remotes: []model.Remote{ + { + URL: "https://example.com/remote", + }, + }, + } + + err := validators.ValidateServerJSON(&serverDetail) + assert.Error(t, err) + assert.Contains(t, err.Error(), validators.ErrUnsupportedRegistryType.Error()) + assert.Contains(t, err.Error(), "registry type is required") +} + func createValidServerWithArgument(arg model.Argument) apiv0.ServerJSON { return apiv0.ServerJSON{ Name: "com.example/test-server", @@ -675,3 +855,72 @@ func createValidServerWithArgument(arg model.Argument) apiv0.ServerJSON { }, } } + +func TestValidate_MCPBReleaseURLs(t *testing.T) { + testCases := []struct { + name string + identifier string + expectError bool + errorMsg string + }{ + // Valid GitHub release URLs + {"valid_github_release", "https://github.com/owner/repo/releases/download/v1.0.0/package.mcpb", false, ""}, + {"valid_github_release_with_path", "https://github.com/org/project/releases/download/v2.1.0/my-server.mcpb", false, ""}, + {"valid_github_complex_tag", "https://github.com/owner/repo/releases/download/v1.0.0-alpha.1+build.123/package.mcpb", false, ""}, + {"valid_github_single_char_owner", "https://github.com/a/b/releases/download/v1.0.0/package.mcpb", false, ""}, + + // Valid GitLab release URLs + {"valid_gitlab_releases", "https://gitlab.com/owner/repo/-/releases/v1.0.0/downloads/package.mcpb", false, ""}, + {"valid_gitlab_package_files", "https://gitlab.com/owner/repo/-/package_files/123/download", false, ""}, + {"valid_gitlab_nested_group", "https://gitlab.com/group/subgroup/repo/-/releases/v1.0.0/downloads/package.mcpb", false, ""}, + {"valid_gitlab_deep_nested", "https://gitlab.com/org/team/project/repo/-/releases/v2.0.0/downloads/server.mcpb", false, ""}, + + // Invalid GitHub URLs (not release URLs) + {"invalid_github_root", "https://github.com/owner/repo", true, "GitHub MCPB packages must be release assets"}, + {"invalid_github_tree", "https://github.com/owner/repo/tree/main", true, "GitHub MCPB packages must be release assets"}, + {"invalid_github_blob", "https://github.com/owner/repo/blob/main/file.mcpb", true, "GitHub MCPB packages must be release assets"}, + {"invalid_github_fake_release_path", "https://github.com/owner/repo/fake/releases/download/v1.0.0/file.mcpb", true, "GitHub MCPB packages must be release assets"}, + {"invalid_github_missing_tag", "https://github.com/owner/repo/releases/download//file.mcpb", true, "GitHub MCPB packages must be release assets"}, + {"invalid_github_missing_filename", "https://github.com/owner/repo/releases/download/v1.0.0/", true, "GitHub MCPB packages must be release assets"}, + + // Invalid GitLab URLs (not release URLs) + {"invalid_gitlab_root", "https://gitlab.com/owner/repo", true, "GitLab MCPB packages must be release assets"}, + {"invalid_gitlab_tree", "https://gitlab.com/owner/repo/-/tree/main", true, "GitLab MCPB packages must be release assets"}, + {"invalid_gitlab_blob", "https://gitlab.com/owner/repo/-/blob/main/file.mcpb", true, "GitLab MCPB packages must be release assets"}, + {"invalid_gitlab_missing_dash_prefix", "https://gitlab.com/owner/repo/releases/v1.0.0/downloads/file.mcpb", true, "GitLab MCPB packages must be release assets"}, + {"invalid_gitlab_missing_downloads", "https://gitlab.com/owner/repo/-/releases/v1.0.0/file.mcpb", true, "GitLab MCPB packages must be release assets"}, + {"invalid_gitlab_invalid_package_files", "https://gitlab.com/owner/repo/-/package_files/abc/download", true, "GitLab MCPB packages must be release assets"}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + server := apiv0.ServerJSON{ + Name: "com.example/test-server", + Description: "Test server", + VersionDetail: model.VersionDetail{ + Version: "1.0.0", + }, + Packages: []model.Package{ + { + RegistryType: model.RegistryTypeMCPB, + RegistryBaseURL: model.RegistryURLGitHub, + Identifier: tc.identifier, + }, + }, + Remotes: []model.Remote{ + { + URL: "https://example.com/remote", + }, + }, + } + + err := validators.ValidateServerJSON(&server) + if tc.expectError { + assert.Error(t, err) + assert.Contains(t, err.Error(), tc.errorMsg) + } else { + assert.NoError(t, err) + } + }) + } +}