From 654cad4aa7738a43ea2fdc1103fb6253b987a648 Mon Sep 17 00:00:00 2001 From: adam jones Date: Thu, 28 Aug 2025 20:20:43 +0100 Subject: [PATCH] Clean up publish and edit handler and service logic (#319) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thanks Camila for being a great Go consultant and advising on how to tidy this up! (I'm still a lil grumpy about some parts, but it's in much better shape 😄) - Consolidated publisher extension validation into a single function. - Updated the `Publish` and `EditServer` methods to directly use `XPublisher` from the request. - Removed outdated tests for publisher extension extraction and validation. - Introduced new validation functions for server details and remote URL matching. - Enhanced error handling for server name format and remote URL validation. - Updated existing tests to reflect changes in validation logic and structure. ## Motivation and Context Things were starting to become a bit spaghetti-y after lots of small PRs and contributions from many people. Camila and I went through and figured out how we could make this a lot cleaner. ## How Has This Been Tested? Tested locally, plus all the integration and unit tests still pass. ## Breaking Changes None --------- Co-authored-by: Claude --- internal/api/handlers/v0/edit.go | 29 +- internal/api/handlers/v0/edit_test.go | 37 ++- internal/api/handlers/v0/publish.go | 30 +- .../handlers/v0/publish_integration_test.go | 4 +- internal/api/handlers/v0/publish_test.go | 10 +- internal/service/fake_service.go | 24 +- internal/service/registry_service.go | 185 ++++-------- internal/validators/validators.go | 230 +++++++-------- internal/validators/validators_test.go | 269 ++---------------- 9 files changed, 240 insertions(+), 578 deletions(-) diff --git a/internal/api/handlers/v0/edit.go b/internal/api/handlers/v0/edit.go index e11dd8e..04e8204 100644 --- a/internal/api/handlers/v0/edit.go +++ b/internal/api/handlers/v0/edit.go @@ -2,7 +2,6 @@ package v0 import ( "context" - "encoding/json" "errors" "net/http" "strings" @@ -19,9 +18,9 @@ import ( // EditServerInput represents the input for editing a server type EditServerInput struct { - Authorization string `header:"Authorization" doc:"Registry JWT token with edit permissions" required:"true"` - ID string `path:"id" doc:"Server ID (UUID)" format:"uuid"` - RawBody []byte `body:"raw"` + Authorization string `header:"Authorization" doc:"Registry JWT token with edit permissions" required:"true"` + ID string `path:"id" doc:"Server ID (UUID)" format:"uuid"` + Body apiv0.PublishRequest `body:""` } // RegisterEditEndpoints registers the edit endpoint @@ -54,11 +53,6 @@ func RegisterEditEndpoints(api huma.API, registry service.RegistryService, cfg * return nil, huma.Error401Unauthorized("Invalid or expired Registry JWT token", err) } - // Validate that only allowed extension fields are present - if err := validators.ValidatePublishRequestExtensions(input.RawBody); err != nil { - return nil, huma.Error400BadRequest("Invalid request format", err) - } - // Get current server to check permissions against existing name currentServer, err := registry.GetByID(input.ID) if err != nil { @@ -73,30 +67,23 @@ func RegisterEditEndpoints(api huma.API, registry service.RegistryService, cfg * return nil, huma.Error403Forbidden("You do not have edit permissions for this server") } - // Parse the validated request body - var editRequest apiv0.PublishRequest - if err := json.Unmarshal(input.RawBody, &editRequest); err != nil { - return nil, huma.Error400BadRequest("Invalid JSON format", err) - } - - // Validate the server detail - validator := validators.NewObjectValidator() - if err := validator.Validate(&editRequest.Server); err != nil { + // Perform all schema validation + if err := validators.ValidatePublishRequest(input.Body); err != nil { return nil, huma.Error400BadRequest(err.Error()) } // Prevent renaming servers - if currentServer.Server.Name != editRequest.Server.Name { + if currentServer.Server.Name != input.Body.Server.Name { return nil, huma.Error400BadRequest("Cannot rename server") } // Prevent undeleting servers - once deleted, they stay deleted - if currentServer.Server.Status == model.StatusDeleted && editRequest.Server.Status != model.StatusDeleted { + if currentServer.Server.Status == model.StatusDeleted && input.Body.Server.Status != model.StatusDeleted { return nil, huma.Error400BadRequest("Cannot change status of deleted server. Deleted servers cannot be undeleted.") } // Edit the server - updatedServer, err := registry.EditServer(input.ID, editRequest) + updatedServer, err := registry.EditServer(input.ID, input.Body) if err != nil { if errors.Is(err, database.ErrNotFound) { return nil, huma.Error404NotFound("Server not found") diff --git a/internal/api/handlers/v0/edit_test.go b/internal/api/handlers/v0/edit_test.go index a9be004..3c1501e 100644 --- a/internal/api/handlers/v0/edit_test.go +++ b/internal/api/handlers/v0/edit_test.go @@ -95,7 +95,13 @@ func TestEditServerEndpoint(t *testing.T) { name: "invalid authorization header format", serverID: "550e8400-e29b-41d4-a716-446655440001", authHeader: "InvalidFormat token123", - requestBody: apiv0.PublishRequest{}, + requestBody: apiv0.PublishRequest{ + Server: model.ServerJSON{ + Name: "io.github.domdomegg/test-server", + Description: "Test server", + VersionDetail: model.VersionDetail{Version: "1.0.0"}, + }, + }, setupMocks: func(_ *MockRegistryService) {}, expectedStatus: http.StatusUnauthorized, expectedError: "Unauthorized", @@ -104,7 +110,13 @@ func TestEditServerEndpoint(t *testing.T) { name: "invalid token", serverID: "550e8400-e29b-41d4-a716-446655440001", authHeader: "Bearer invalid-token", - requestBody: apiv0.PublishRequest{}, + requestBody: apiv0.PublishRequest{ + Server: model.ServerJSON{ + Name: "io.github.domdomegg/test-server", + Description: "Test server", + VersionDetail: model.VersionDetail{Version: "1.0.0"}, + }, + }, setupMocks: func(_ *MockRegistryService) {}, expectedStatus: http.StatusUnauthorized, expectedError: "Unauthorized", @@ -240,7 +252,7 @@ func TestEditServerEndpoint(t *testing.T) { expectedError: "Bad Request", }, { - name: "invalid request body", + name: "validation error - invalid server name", serverID: "550e8400-e29b-41d4-a716-446655440001", authHeader: func() string { cfg := &config.Config{JWTPrivateKey: "bb2c6b424005acd5df47a9e2c87f446def86dd740c888ea3efb825b23f7ef47c"} @@ -253,8 +265,23 @@ func TestEditServerEndpoint(t *testing.T) { }) return "Bearer " + token }(), - requestBody: "invalid json", - setupMocks: func(_ *MockRegistryService) {}, + requestBody: apiv0.PublishRequest{ + Server: model.ServerJSON{ + Name: "invalid-name-format", // Missing namespace/name format + Description: "Test server", + VersionDetail: model.VersionDetail{Version: "1.0.0"}, + }, + }, + setupMocks: func(registry *MockRegistryService) { + currentServer := &apiv0.ServerRecord{ + Server: model.ServerJSON{ + Name: "io.github.domdomegg/test-server", + Description: "Original server", + Status: model.StatusActive, + }, + } + registry.On("GetByID", "550e8400-e29b-41d4-a716-446655440001").Return(currentServer, nil) + }, expectedStatus: http.StatusBadRequest, expectedError: "Bad Request", }, diff --git a/internal/api/handlers/v0/publish.go b/internal/api/handlers/v0/publish.go index a732254..ea6e3cb 100644 --- a/internal/api/handlers/v0/publish.go +++ b/internal/api/handlers/v0/publish.go @@ -2,7 +2,6 @@ package v0 import ( "context" - "encoding/json" "net/http" "strings" @@ -16,8 +15,8 @@ import ( // PublishServerInput represents the input for publishing a server type PublishServerInput struct { - Authorization string `header:"Authorization" doc:"Registry JWT token (obtained from /v0/auth/token/github)" required:"true"` - RawBody []byte `body:"raw"` + Authorization string `header:"Authorization" doc:"Registry JWT token (obtained from /v0/auth/token/github)" required:"true"` + Body apiv0.PublishRequest `body:""` } // RegisterPublishEndpoint registers the publish endpoint @@ -50,33 +49,18 @@ func RegisterPublishEndpoint(api huma.API, registry service.RegistryService, cfg return nil, huma.Error401Unauthorized("Invalid or expired Registry JWT token", err) } - // Validate that only allowed extension fields are present - if err := validators.ValidatePublishRequestExtensions(input.RawBody); err != nil { - return nil, huma.Error400BadRequest("Invalid request format", err) - } - - // Parse the validated request body - var publishRequest apiv0.PublishRequest - if err := json.Unmarshal(input.RawBody, &publishRequest); err != nil { - return nil, huma.Error400BadRequest("Invalid JSON format", err) - } - - // Get server details from request body - serverDetail := publishRequest.Server - - // Validate the server detail - validator := validators.NewObjectValidator() - if err := validator.Validate(&serverDetail); err != nil { + // Perform all schema validation + if err := validators.ValidatePublishRequest(input.Body); err != nil { return nil, huma.Error400BadRequest(err.Error()) } - // Verify that the token's repository matches the server being published - if !jwtManager.HasPermission(serverDetail.Name, auth.PermissionActionPublish, claims.Permissions) { + // Verify that the token has permission to publish the server + if !jwtManager.HasPermission(input.Body.Server.Name, auth.PermissionActionPublish, claims.Permissions) { return nil, huma.Error403Forbidden("You do not have permission to publish this server") } // Publish the server with extensions - publishedServer, err := registry.Publish(publishRequest) + publishedServer, err := registry.Publish(input.Body) if err != nil { return nil, huma.Error400BadRequest("Failed to publish server", err) } diff --git a/internal/api/handlers/v0/publish_integration_test.go b/internal/api/handlers/v0/publish_integration_test.go index 253b510..4b672f1 100644 --- a/internal/api/handlers/v0/publish_integration_test.go +++ b/internal/api/handlers/v0/publish_integration_test.go @@ -169,7 +169,9 @@ func TestPublishIntegration(t *testing.T) { t.Run("publish fails with invalid token", func(t *testing.T) { publishReq := apiv0.PublishRequest{ Server: model.ServerJSON{ - Name: "test-server", + Name: "io.github.domdomegg/test-server", + Description: "Test server", + VersionDetail: model.VersionDetail{Version: "1.0.0"}, }, } diff --git a/internal/api/handlers/v0/publish_test.go b/internal/api/handlers/v0/publish_test.go index 87947c0..314b9be 100644 --- a/internal/api/handlers/v0/publish_test.go +++ b/internal/api/handlers/v0/publish_test.go @@ -150,8 +150,14 @@ func TestPublishEndpoint(t *testing.T) { expectedError: "required header parameter is missing", }, { - name: "invalid authorization header format", - requestBody: apiv0.PublishRequest{}, + name: "invalid authorization header format", + requestBody: apiv0.PublishRequest{ + Server: model.ServerJSON{ + Name: "io.github.domdomegg/test-server", + Description: "Test server", + VersionDetail: model.VersionDetail{Version: "1.0.0"}, + }, + }, authHeader: "InvalidFormat", setupMocks: func(_ *MockRegistryService) {}, expectedStatus: http.StatusUnauthorized, diff --git a/internal/service/fake_service.go b/internal/service/fake_service.go index 4f1a206..bd34dce 100644 --- a/internal/service/fake_service.go +++ b/internal/service/fake_service.go @@ -114,18 +114,16 @@ func (s *fakeRegistryService) Publish(req apiv0.PublishRequest) (*apiv0.ServerRe defer cancel() // Validate the request - if err := validators.ValidatePublisherExtensions(req); err != nil { + if err := validators.ValidatePublishRequest(req); err != nil { return nil, err } - // Validate server name exists - if _, err := validators.ParseServerName(req.Server); err != nil { - return nil, err + // Use publisher extensions directly from request + publisherExtensions := req.XPublisher + if publisherExtensions == nil { + publisherExtensions = make(map[string]interface{}) } - // Extract publisher extensions from request - publisherExtensions := validators.ExtractPublisherExtensions(req) - // Create registry metadata for fake service (always marks as latest) now := time.Now() registryMetadata := apiv0.RegistryExtensions{ @@ -152,18 +150,16 @@ func (s *fakeRegistryService) EditServer(id string, req apiv0.PublishRequest) (* defer cancel() // Validate the request - if err := validators.ValidatePublisherExtensions(req); err != nil { + if err := validators.ValidatePublishRequest(req); err != nil { return nil, err } - // Validate server name exists and format - if _, err := validators.ParseServerName(req.Server); err != nil { - return nil, err + // Use publisher extensions directly from request + publisherExtensions := req.XPublisher + if publisherExtensions == nil { + publisherExtensions = make(map[string]interface{}) } - // Extract publisher extensions from request - publisherExtensions := validators.ExtractPublisherExtensions(req) - // Update server in database serverRecord, err := s.db.UpdateServer(ctx, id, req.Server, publisherExtensions) if err != nil { diff --git a/internal/service/registry_service.go b/internal/service/registry_service.go index 04897d6..91eff1a 100644 --- a/internal/service/registry_service.go +++ b/internal/service/registry_service.go @@ -4,8 +4,6 @@ import ( "context" "errors" "fmt" - "net/url" - "strings" "time" "github.com/google/uuid" @@ -71,61 +69,6 @@ func (s *registryServiceImpl) GetByID(id string) (*apiv0.ServerRecord, error) { return serverRecord, nil } -// validateMCPBPackage validates MCPB packages -func validateMCPBPackage(host string) error { - allowedHosts := []string{ - "github.com", - "www.github.com", - "gitlab.com", - "www.gitlab.com", - } - - isAllowed := false - for _, allowed := range allowedHosts { - if host == allowed { - isAllowed = true - break - } - } - - if !isAllowed { - return fmt.Errorf("MCPB packages must be hosted on allowlisted providers (GitHub or GitLab). Host '%s' is not allowed", host) - } - - return nil -} - -// validatePackage validates packages to ensure they meet requirements -func validatePackage(pkg *model.Package) error { - registryType := strings.ToLower(pkg.RegistryType) - - // For direct download packages (mcpb or direct URLs) - if registryType == "mcpb" || - strings.HasPrefix(pkg.Identifier, "http://") || strings.HasPrefix(pkg.Identifier, "https://") { - parsedURL, err := url.Parse(pkg.Identifier) - if err != nil { - return fmt.Errorf("invalid package URL: %w", err) - } - - host := strings.ToLower(parsedURL.Host) - - // For MCPB packages, validate they're from allowed hosts - if registryType == "mcpb" || strings.HasSuffix(strings.ToLower(pkg.Identifier), ".mcpb") { - return validateMCPBPackage(host) - } - - // For other URL-based packages, just ensure it's valid - if parsedURL.Scheme == "" || parsedURL.Host == "" { - return fmt.Errorf("package URL must be a valid absolute URL") - } - return nil - } - - // For registry-based packages, no special validation needed - // Registry types like "npm", "pypi", "docker-hub", "nuget" are all valid - return nil -} - // Publish publishes a server with separated extensions func (s *registryServiceImpl) Publish(req apiv0.PublishRequest) (*apiv0.ServerRecord, error) { // Create a timeout context for the database operation @@ -133,91 +76,60 @@ func (s *registryServiceImpl) Publish(req apiv0.PublishRequest) (*apiv0.ServerRe defer cancel() // Validate the request - if err := validators.ValidatePublisherExtensions(req); err != nil { + if err := validators.ValidatePublishRequest(req); err != nil { return nil, err } - // Validate server name exists and format - if _, err := validators.ParseServerName(req.Server); err != nil { - return nil, err - } - - // Validate all packages - for _, pkg := range req.Server.Packages { - if err := validatePackage(&pkg); err != nil { - return nil, fmt.Errorf("validation failed: %w", err) - } - } - - // Validate reverse-DNS namespace matching for remote URLs - if err := validators.ValidateRemoteNamespaceMatch(req.Server); err != nil { - return nil, err - } + publishTime := time.Now() // Check for duplicate remote URLs if err := s.validateNoDuplicateRemoteURLs(ctx, req.Server); err != nil { return nil, err } - // Get the new version's details - newVersion := req.Server.VersionDetail.Version - newName := req.Server.Name - currentTime := time.Now() - - // Check for existing versions of this server - existingServers, _, err := s.db.List(ctx, map[string]any{"name": newName}, "", maxServerVersionsPerServer) + existingServerVersions, _, err := s.db.List(ctx, map[string]any{"name": req.Server.Name}, "", maxServerVersionsPerServer) if err != nil && !errors.Is(err, database.ErrNotFound) { return nil, err } - if len(existingServers) == maxServerVersionsPerServer { + // Check we haven't exceeded the maximum versions allowed for a server + if len(existingServerVersions) >= maxServerVersionsPerServer { return nil, database.ErrMaxServersReached } - // Determine if this version should be marked as latest - isLatest := true - var existingLatestID string - - // Check all existing versions for duplicates and determine if new version should be latest - for _, server := range existingServers { + // Check this isn't a duplicate version + for _, server := range existingServerVersions { existingVersion := server.Server.VersionDetail.Version - - // Early exit: check for duplicate version - if existingVersion == newVersion { + if existingVersion == req.Server.VersionDetail.Version { return nil, database.ErrInvalidVersion } - - if server.XIOModelContextProtocolRegistry.IsLatest { - existingLatestID = server.XIOModelContextProtocolRegistry.ID - existingTime, _ := time.Parse(time.RFC3339, server.XIOModelContextProtocolRegistry.ReleaseDate) - - // Compare versions using the proper versioning strategy - comparison := CompareVersions(newVersion, existingVersion, currentTime, existingTime) - if comparison <= 0 { - // New version is not greater than existing latest - isLatest = false - } - } } - // Extract publisher extensions from request - publisherExtensions := validators.ExtractPublisherExtensions(req) + // Determine if this version should be marked as latest + existingLatest := s.getCurrentLatestVersion(existingServerVersions) + isNewLatest := true + if existingLatest != nil { + isNewLatest = CompareVersions( + req.Server.VersionDetail.Version, + existingLatest.Server.VersionDetail.Version, + publishTime, + existingLatest.XIOModelContextProtocolRegistry.PublishedAt, + ) > 0 + } // Create registry metadata with service-determined values registryMetadata := apiv0.RegistryExtensions{ ID: uuid.New().String(), - PublishedAt: currentTime, - UpdatedAt: currentTime, - IsLatest: isLatest, - ReleaseDate: currentTime.Format(time.RFC3339), + PublishedAt: publishTime, + UpdatedAt: publishTime, + IsLatest: isNewLatest, + ReleaseDate: publishTime.Format(time.RFC3339), } - // If this will be the latest version, we need to update the existing latest - if isLatest && existingLatestID != "" { - // Update the existing latest to no longer be latest - if err := s.db.UpdateLatestFlag(ctx, existingLatestID, false); err != nil { - return nil, err - } + // Use publisher extensions directly from request + publisherExtensions := req.XPublisher + if publisherExtensions == nil { + publisherExtensions = make(map[string]interface{}) } // Publish to database with the registry metadata @@ -226,6 +138,13 @@ func (s *registryServiceImpl) Publish(req apiv0.PublishRequest) (*apiv0.ServerRe return nil, err } + // Mark previous latest as no longer latest + if isNewLatest && existingLatest != nil { + if err := s.db.UpdateLatestFlag(ctx, existingLatest.XIOModelContextProtocolRegistry.ID, false); err != nil { + return nil, err + } + } + // Return ServerRecord directly (it's now the same as ServerResponse) return serverRecord, nil } @@ -255,30 +174,23 @@ func (s *registryServiceImpl) validateNoDuplicateRemoteURLs(ctx context.Context, return nil } +// getCurrentLatestVersion finds the current latest version from existing server versions +func (s *registryServiceImpl) getCurrentLatestVersion(existingServerVersions []*apiv0.ServerRecord) *apiv0.ServerRecord { + for _, server := range existingServerVersions { + if server.XIOModelContextProtocolRegistry.IsLatest { + return server + } + } + return nil +} + // EditServer updates an existing server with new details (admin operation) func (s *registryServiceImpl) EditServer(id string, req apiv0.PublishRequest) (*apiv0.ServerRecord, error) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() // Validate the request - if err := validators.ValidatePublisherExtensions(req); err != nil { - return nil, err - } - - // Validate server name exists and format - if _, err := validators.ParseServerName(req.Server); err != nil { - return nil, err - } - - // Validate all packages - for _, pkg := range req.Server.Packages { - if err := validatePackage(&pkg); err != nil { - return nil, fmt.Errorf("validation failed: %w", err) - } - } - - // Validate reverse-DNS namespace matching for remote URLs - if err := validators.ValidateRemoteNamespaceMatch(req.Server); err != nil { + if err := validators.ValidatePublishRequest(req); err != nil { return nil, err } @@ -287,8 +199,11 @@ func (s *registryServiceImpl) EditServer(id string, req apiv0.PublishRequest) (* return nil, err } - // Extract publisher extensions from request - publisherExtensions := validators.ExtractPublisherExtensions(req) + // Use publisher extensions directly from request + publisherExtensions := req.XPublisher + if publisherExtensions == nil { + publisherExtensions = make(map[string]interface{}) + } // Update server in database serverRecord, err := s.db.UpdateServer(ctx, id, req.Server, publisherExtensions) diff --git a/internal/validators/validators.go b/internal/validators/validators.go index 84f96fb..0fe74cd 100644 --- a/internal/validators/validators.go +++ b/internal/validators/validators.go @@ -11,33 +11,47 @@ import ( "github.com/modelcontextprotocol/registry/pkg/model" ) -// ServerValidator validates server details -type ServerValidator struct { - *RepositoryValidator // Embedded RepositoryValidator for repository validation -} - -// Validate checks if the server details are valid -func (v *ServerValidator) Validate(obj *model.ServerJSON) error { - if err := v.RepositoryValidator.Validate(&obj.Repository); err != nil { +func ValidateServerJSON(serverJSON *model.ServerJSON) error { + // Validate server name exists and format + if _, err := parseServerName(*serverJSON); err != nil { return err } + + // Validate repository + if err := validateRepository(&serverJSON.Repository); err != nil { + return err + } + + // Validate all packages (basic field validation) + for _, pkg := range serverJSON.Packages { + if err := validatePackageField(&pkg); err != nil { + return err + } + } + + // Validate all packages (URL and registry type validation) + for _, pkg := range serverJSON.Packages { + if err := validatePackage(&pkg); err != nil { + return fmt.Errorf("validation failed: %w", err) + } + } + + // Validate all remotes + for _, remote := range serverJSON.Remotes { + if err := validateRemote(&remote); err != nil { + return err + } + } + + // Validate reverse-DNS namespace matching for remote URLs + if err := validateRemoteNamespaceMatch(*serverJSON); err != nil { + return err + } + return nil } -// NewServerValidator creates a new ServerValidator instance -func NewServerValidator() *ServerValidator { - return &ServerValidator{ - RepositoryValidator: NewRepositoryValidator(), - } -} - -// RepositoryValidator validates repository details -type RepositoryValidator struct { - validSources map[RepositorySource]bool -} - -// Validate checks if the repository details are valid -func (rv *RepositoryValidator) Validate(obj *model.Repository) error { +func validateRepository(obj *model.Repository) error { // Skip validation for empty repository (optional field) if obj.URL == "" && obj.Source == "" { return nil @@ -52,18 +66,7 @@ func (rv *RepositoryValidator) Validate(obj *model.Repository) error { return nil } -// NewRepositoryValidator creates a new RepositoryValidator instance -func NewRepositoryValidator() *RepositoryValidator { - return &RepositoryValidator{ - validSources: map[RepositorySource]bool{SourceGitHub: true, SourceGitLab: true}, - } -} - -// PackageValidator validates package details -type PackageValidator struct{} - -// Validate checks if the package details are valid -func (pv *PackageValidator) Validate(obj *model.Package) error { +func validatePackageField(obj *model.Package) error { if !HasNoSpaces(obj.Identifier) { return ErrPackageNameHasSpaces } @@ -71,65 +74,82 @@ func (pv *PackageValidator) Validate(obj *model.Package) error { return nil } -// NewPackageValidator creates a new PackageValidator instance -func NewPackageValidator() *PackageValidator { - return &PackageValidator{} -} - -// RemoteValidator validates remote connection details -type RemoteValidator struct{} - -// Validate checks if the remote connection details are valid -func (rv *RemoteValidator) Validate(obj *model.Remote) error { +func validateRemote(obj *model.Remote) error { if !IsValidURL(obj.URL) { return fmt.Errorf("%w: %s", ErrInvalidRemoteURL, obj.URL) } return nil } -// NewRemoteValidator creates a new RemoteValidator instance -func NewRemoteValidator() *RemoteValidator { - return &RemoteValidator{} -} - -// ObjectValidator aggregates multiple validators for different object types -// This allows for a single entry point to validate complex objects that may contain multiple fields -// that need validation. -type ObjectValidator struct { - ServerValidator *ServerValidator - PackageValidator *PackageValidator - RemoteValidator *RemoteValidator -} - -func NewObjectValidator() *ObjectValidator { - return &ObjectValidator{ - ServerValidator: NewServerValidator(), - PackageValidator: NewPackageValidator(), - RemoteValidator: NewRemoteValidator(), - } -} - -func (ov *ObjectValidator) Validate(obj *model.ServerJSON) error { - if err := ov.ServerValidator.Validate(obj); err != nil { - return err +func validateMCPBPackage(host string) error { + allowedHosts := []string{ + "github.com", + "www.github.com", + "gitlab.com", + "www.gitlab.com", } - for _, pkg := range obj.Packages { - if err := ov.PackageValidator.Validate(&pkg); err != nil { - return err + isAllowed := false + for _, allowed := range allowedHosts { + if host == allowed { + isAllowed = true + break } } - for _, remote := range obj.Remotes { - if err := ov.RemoteValidator.Validate(&remote); err != nil { - return err - } + if !isAllowed { + return fmt.Errorf("MCPB packages must be hosted on allowlisted providers (GitHub or GitLab). Host '%s' is not allowed", host) } + return nil } -// ValidatePublisherExtensions validates that publisher extensions are within size limits -func ValidatePublisherExtensions(req apiv0.PublishRequest) error { +func validatePackage(pkg *model.Package) error { + registryType := strings.ToLower(pkg.RegistryType) + + // For direct download packages (mcpb or direct URLs) + if registryType == "mcpb" || + strings.HasPrefix(pkg.Identifier, "http://") || strings.HasPrefix(pkg.Identifier, "https://") { + parsedURL, err := url.Parse(pkg.Identifier) + if err != nil { + return fmt.Errorf("invalid package URL: %w", err) + } + + host := strings.ToLower(parsedURL.Host) + + // For MCPB packages, validate they're from allowed hosts + if registryType == "mcpb" { + return validateMCPBPackage(host) + } + + // For other URL-based packages, just ensure it's valid + if parsedURL.Scheme == "" || parsedURL.Host == "" { + return fmt.Errorf("package URL must be a valid absolute URL") + } + return nil + } + + // For registry-based packages, no special validation needed + // Registry types like "npm", "pypi", "docker-hub", "nuget" are all valid + return nil +} + +// ValidatePublishRequest validates a complete publish request including extensions +func ValidatePublishRequest(req apiv0.PublishRequest) error { + // Validate publisher extensions + if err := validatePublisherExtensions(req); err != nil { + return err + } + + // Validate the server detail (includes all nested validation) + if err := ValidateServerJSON(&req.Server); err != nil { + return err + } + + return nil +} + +func validatePublisherExtensions(req apiv0.PublishRequest) error { const maxExtensionSize = 4 * 1024 // 4KB limit // Check size limit for x-publisher extension @@ -146,50 +166,8 @@ func ValidatePublisherExtensions(req apiv0.PublishRequest) error { return nil } -// ValidatePublishRequestExtensions validates that only allowed extension fields are present -func ValidatePublishRequestExtensions(requestData []byte) error { - // Parse the raw JSON to check for unknown fields - var rawRequest map[string]interface{} - if err := json.Unmarshal(requestData, &rawRequest); err != nil { - return fmt.Errorf("failed to parse request JSON: %w", err) - } - - // Define allowed top-level fields - allowedFields := map[string]bool{ - "server": true, - "x-publisher": true, - } - - // Check for any disallowed fields - var invalidFields []string - for field := range rawRequest { - if !allowedFields[field] { - invalidFields = append(invalidFields, field) - } - } - - if len(invalidFields) > 0 { - return fmt.Errorf("invalid extension fields: %v. Only 'server' and 'x-publisher' fields are allowed", invalidFields) - } - - return nil -} - -// ExtractPublisherExtensions extracts publisher extensions from a apiv0.PublishRequest -func ExtractPublisherExtensions(req apiv0.PublishRequest) map[string]interface{} { - publisherExtensions := make(map[string]interface{}) - if req.XPublisher != nil { - // Copy fields directly, avoiding double nesting - for k, v := range req.XPublisher { - publisherExtensions[k] = v - } - } - return publisherExtensions -} - -// ParseServerName extracts the server name from a model.ServerJSON for validation purposes -func ParseServerName(serverDetail model.ServerJSON) (string, error) { - name := serverDetail.Name +func parseServerName(serverJSON model.ServerJSON) (string, error) { + name := serverJSON.Name if name == "" { return "", fmt.Errorf("server name is required and must be a string") } @@ -207,11 +185,11 @@ func ParseServerName(serverDetail model.ServerJSON) (string, error) { return name, nil } -// ValidateRemoteNamespaceMatch validates that remote URLs match the reverse-DNS namespace -func ValidateRemoteNamespaceMatch(serverDetail model.ServerJSON) error { - namespace := serverDetail.Name +// validateRemoteNamespaceMatch validates that remote URLs match the reverse-DNS namespace +func validateRemoteNamespaceMatch(serverJSON model.ServerJSON) error { + namespace := serverJSON.Name - for _, remote := range serverDetail.Remotes { + for _, remote := range serverJSON.Remotes { if err := validateRemoteURLMatchesNamespace(remote.URL, namespace); err != nil { return fmt.Errorf("remote URL %s does not match namespace %s: %w", remote.URL, namespace, err) } diff --git a/internal/validators/validators_test.go b/internal/validators/validators_test.go index eb9c9a4..568e8ae 100644 --- a/internal/validators/validators_test.go +++ b/internal/validators/validators_test.go @@ -1,20 +1,14 @@ package validators_test import ( - "encoding/json" "testing" - "time" "github.com/modelcontextprotocol/registry/internal/validators" - apiv0 "github.com/modelcontextprotocol/registry/pkg/api/v0" "github.com/modelcontextprotocol/registry/pkg/model" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) -func TestObjectValidator_Validate(t *testing.T) { - validator := validators.NewObjectValidator() - +func TestValidate(t *testing.T) { tests := []struct { name string serverDetail model.ServerJSON @@ -23,7 +17,7 @@ func TestObjectValidator_Validate(t *testing.T) { { name: "valid server detail with all fields", serverDetail: model.ServerJSON{ - Name: "test-server", + Name: "com.example/test-server", Description: "A test server", Repository: model.Repository{ URL: "https://github.com/owner/repo", @@ -51,7 +45,7 @@ func TestObjectValidator_Validate(t *testing.T) { { name: "server with invalid repository source", serverDetail: model.ServerJSON{ - Name: "test-server", + Name: "com.example/test-server", Description: "A test server", Repository: model.Repository{ URL: "https://bitbucket.org/owner/repo", @@ -66,7 +60,7 @@ func TestObjectValidator_Validate(t *testing.T) { { name: "server with invalid GitHub URL format", serverDetail: model.ServerJSON{ - Name: "test-server", + Name: "com.example/test-server", Description: "A test server", Repository: model.Repository{ URL: "https://github.com/owner", // Missing repo name @@ -81,7 +75,7 @@ func TestObjectValidator_Validate(t *testing.T) { { name: "server with invalid GitLab URL format", serverDetail: model.ServerJSON{ - Name: "test-server", + Name: "com.example/test-server", Description: "A test server", Repository: model.Repository{ URL: "https://gitlab.com", // Missing owner and repo @@ -96,7 +90,7 @@ func TestObjectValidator_Validate(t *testing.T) { { name: "package with spaces in name", serverDetail: model.ServerJSON{ - Name: "test-server", + Name: "com.example/test-server", Description: "A test server", Repository: model.Repository{ URL: "https://github.com/owner/repo", @@ -118,7 +112,7 @@ func TestObjectValidator_Validate(t *testing.T) { { name: "multiple packages with one invalid", serverDetail: model.ServerJSON{ - Name: "test-server", + Name: "com.example/test-server", Description: "A test server", Repository: model.Repository{ URL: "https://github.com/owner/repo", @@ -145,7 +139,7 @@ func TestObjectValidator_Validate(t *testing.T) { { name: "remote with invalid URL", serverDetail: model.ServerJSON{ - Name: "test-server", + Name: "com.example/test-server", Description: "A test server", Repository: model.Repository{ URL: "https://github.com/owner/repo", @@ -165,7 +159,7 @@ func TestObjectValidator_Validate(t *testing.T) { { name: "remote with missing scheme", serverDetail: model.ServerJSON{ - Name: "test-server", + Name: "com.example/test-server", Description: "A test server", Repository: model.Repository{ URL: "https://github.com/owner/repo", @@ -185,7 +179,7 @@ func TestObjectValidator_Validate(t *testing.T) { { name: "multiple remotes with one invalid", serverDetail: model.ServerJSON{ - Name: "test-server", + Name: "com.example/test-server", Description: "A test server", Repository: model.Repository{ URL: "https://github.com/owner/repo", @@ -208,7 +202,7 @@ func TestObjectValidator_Validate(t *testing.T) { { name: "server detail with nil packages and remotes", serverDetail: model.ServerJSON{ - Name: "test-server", + Name: "com.example/test-server", Description: "A test server", Repository: model.Repository{ URL: "https://github.com/owner/repo", @@ -225,7 +219,7 @@ func TestObjectValidator_Validate(t *testing.T) { { name: "server detail with empty packages and remotes slices", serverDetail: model.ServerJSON{ - Name: "test-server", + Name: "com.example/test-server", Description: "A test server", Repository: model.Repository{ URL: "https://github.com/owner/repo", @@ -243,7 +237,7 @@ func TestObjectValidator_Validate(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := validator.Validate(&tt.serverDetail) + err := validators.ValidateServerJSON(&tt.serverDetail) if tt.expectedError == "" { assert.NoError(t, err) @@ -255,234 +249,7 @@ func TestObjectValidator_Validate(t *testing.T) { } } -func TestExtractPublisherExtensions(t *testing.T) { - tests := []struct { - name string - request apiv0.PublishRequest - expected map[string]interface{} - }{ - { - name: "nil publisher extensions", - request: apiv0.PublishRequest{ - Server: model.ServerJSON{Name: "test"}, - }, - expected: map[string]interface{}{}, - }, - { - name: "empty publisher extensions", - request: apiv0.PublishRequest{ - Server: model.ServerJSON{Name: "test"}, - XPublisher: map[string]interface{}{}, - }, - expected: map[string]interface{}{}, - }, - { - name: "simple publisher extensions", - request: apiv0.PublishRequest{ - Server: model.ServerJSON{Name: "test"}, - XPublisher: map[string]interface{}{ - "build_info": map[string]interface{}{ - "version": "1.2.3", - "commit": "abc123", - }, - "publisher_metadata": "test-publisher", - }, - }, - expected: map[string]interface{}{ - "build_info": map[string]interface{}{ - "version": "1.2.3", - "commit": "abc123", - }, - "publisher_metadata": "test-publisher", - }, - }, - { - name: "nested publisher extensions", - request: apiv0.PublishRequest{ - Server: model.ServerJSON{Name: "test"}, - XPublisher: map[string]interface{}{ - "ci": map[string]interface{}{ - "pipeline": map[string]interface{}{ - "id": "12345", - "branch": "main", - }, - "artifacts": []string{"binary", "docs"}, - }, - }, - }, - expected: map[string]interface{}{ - "ci": map[string]interface{}{ - "pipeline": map[string]interface{}{ - "id": "12345", - "branch": "main", - }, - "artifacts": []string{"binary", "docs"}, - }, - }, - }, - { - name: "nil publisher extensions (should be ignored)", - request: apiv0.PublishRequest{ - Server: model.ServerJSON{Name: "test"}, - XPublisher: nil, - }, - expected: map[string]interface{}{}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := validators.ExtractPublisherExtensions(tt.request) - assert.Equal(t, tt.expected, result) - }) - } -} - -func TestExtractPublisherExtensions_DoesNotDoubleNest(t *testing.T) { - // This test specifically verifies the bug we fixed - no double nesting - request := apiv0.PublishRequest{ - Server: model.ServerJSON{Name: "test"}, - XPublisher: map[string]interface{}{ - "tool": "publisher-cli", - "version": "1.0.0", - }, - } - - result := validators.ExtractPublisherExtensions(request) - - // Verify we get the data directly, not wrapped in another "x-publisher" key - assert.Equal(t, "publisher-cli", result["tool"]) - assert.Equal(t, "1.0.0", result["version"]) - - // Verify we don't have double nesting - assert.NotContains(t, result, "x-publisher") -} - -func TestServerResponse_JSONSerialization(t *testing.T) { - // Test that ServerResponse properly serializes to extension wrapper format - publishedTime := time.Date(2023, 12, 1, 10, 30, 0, 0, time.UTC) - - response := apiv0.ServerRecord{ - Server: model.ServerJSON{ - Name: "test-server", - Description: "A test server", - Repository: model.Repository{ - URL: "https://github.com/test/server", - Source: "github", - ID: "test/server", - }, - VersionDetail: model.VersionDetail{ - Version: "1.0.0", - }, - }, - XIOModelContextProtocolRegistry: apiv0.RegistryExtensions{ - ID: "registry-id-123", - PublishedAt: publishedTime, - IsLatest: true, - ReleaseDate: publishedTime.Format(time.RFC3339), - }, - XPublisher: map[string]interface{}{ - "build_tool": "ci-pipeline", - "commit": "abc123def", - }, - } - - // Serialize to JSON - jsonData, err := json.Marshal(response) - require.NoError(t, err) - - // Parse back to verify structure - var parsed map[string]interface{} - err = json.Unmarshal(jsonData, &parsed) - require.NoError(t, err) - - // Verify three-layer structure exists - assert.Contains(t, parsed, "server") - assert.Contains(t, parsed, "x-io.modelcontextprotocol.registry") - assert.Contains(t, parsed, "x-publisher") - - // Verify server data - serverData := parsed["server"].(map[string]interface{}) - assert.Equal(t, "test-server", serverData["name"]) - - // Verify registry metadata - registryData := parsed["x-io.modelcontextprotocol.registry"].(map[string]interface{}) - assert.Equal(t, "registry-id-123", registryData["id"]) - - // Verify publisher extensions - publisherData := parsed["x-publisher"].(map[string]interface{}) - assert.Equal(t, "ci-pipeline", publisherData["build_tool"]) - assert.Equal(t, "abc123def", publisherData["commit"]) -} - -func TestPublishRequest_WithPublisherExtensions(t *testing.T) { - // Test that PublishRequest properly deserializes publisher extensions - jsonData := `{ - "server": { - "name": "test-server", - "description": "Test server", - "version_detail": { - "version": "1.0.0" - } - }, - "x-publisher": { - "tool": "publisher-cli", - "metadata": { - "build_date": "2023-12-01", - "commit": "abc123" - } - } - }` - - var request apiv0.PublishRequest - err := json.Unmarshal([]byte(jsonData), &request) - require.NoError(t, err) - - // Verify server data - assert.Equal(t, "test-server", request.Server.Name) - assert.Equal(t, "Test server", request.Server.Description) - - // Verify publisher extensions - require.NotNil(t, request.XPublisher) - assert.Equal(t, "publisher-cli", request.XPublisher["tool"]) - - metadata := request.XPublisher["metadata"].(map[string]interface{}) - assert.Equal(t, "2023-12-01", metadata["build_date"]) - assert.Equal(t, "abc123", metadata["commit"]) -} - -func TestServerResponse_EmptyExtensions(t *testing.T) { - // Test behavior with empty/nil extensions - response := apiv0.ServerRecord{ - Server: model.ServerJSON{ - Name: "minimal-server", - Description: "Minimal test", - }, - XIOModelContextProtocolRegistry: apiv0.RegistryExtensions{ - ID: "min-id", - }, - XPublisher: nil, - } - - jsonData, err := json.Marshal(response) - require.NoError(t, err) - - var parsed map[string]interface{} - err = json.Unmarshal(jsonData, &parsed) - require.NoError(t, err) - - // Should still have the structure, even with nil publisher extensions - assert.Contains(t, parsed, "server") - assert.Contains(t, parsed, "x-io.modelcontextprotocol.registry") - - // x-publisher should be null/nil in JSON when empty - publisherValue, exists := parsed["x-publisher"] - if exists { - assert.Nil(t, publisherValue) - } -} - -func TestValidateRemoteNamespaceMatch(t *testing.T) { +func TestValidate_RemoteNamespaceMatch(t *testing.T) { tests := []struct { name string serverDetail model.ServerJSON @@ -560,7 +327,7 @@ func TestValidateRemoteNamespaceMatch(t *testing.T) { }, }, expectError: true, - errorMsg: "URL must have a valid hostname", + errorMsg: "invalid remote URL", }, { name: "empty remotes array", @@ -597,7 +364,7 @@ func TestValidateRemoteNamespaceMatch(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := validators.ValidateRemoteNamespaceMatch(tt.serverDetail) + err := validators.ValidateServerJSON(&tt.serverDetail) if tt.expectError { assert.Error(t, err) @@ -609,7 +376,7 @@ func TestValidateRemoteNamespaceMatch(t *testing.T) { } } -func TestParseServerName(t *testing.T) { +func TestValidate_ServerNameFormat(t *testing.T) { tests := []struct { name string serverDetail model.ServerJSON @@ -673,7 +440,7 @@ func TestParseServerName(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - _, err := validators.ParseServerName(tt.serverDetail) + err := validators.ValidateServerJSON(&tt.serverDetail) if tt.expectError { assert.Error(t, err)