From ec5f73a2acf9972b9b03a63bbab7b2d1988b1b87 Mon Sep 17 00:00:00 2001 From: Ossama Lafhel Date: Sat, 13 Sep 2025 22:22:40 +0200 Subject: [PATCH 1/3] fix: validate server names to allow only single slash (#471) Added validation to reject server names with multiple slashes, ensuring consistency between JSON schema pattern and API validation. - Count slashes in parseServerName() and reject if > 1 - Add ErrMultipleSlashesInServerName error constant - Add unit tests in validators_test.go - Add integration test in publish_test.go --- internal/api/handlers/v0/publish_test.go | 231 +++++++++++++++++++++++ internal/validators/constants.go | 6 +- internal/validators/validators.go | 8 +- internal/validators/validators_test.go | 104 +++++++++- 4 files changed, 344 insertions(+), 5 deletions(-) diff --git a/internal/api/handlers/v0/publish_test.go b/internal/api/handlers/v0/publish_test.go index 61932769..9e246c3d 100644 --- a/internal/api/handlers/v0/publish_test.go +++ b/internal/api/handlers/v0/publish_test.go @@ -224,6 +224,135 @@ func TestPublishEndpoint(t *testing.T) { setupRegistryService: func(_ service.RegistryService) {}, expectedStatus: http.StatusOK, }, + // IB-2-registry: Integration test for multi-slash server name rejection + { + name: "invalid server name - multiple slashes (two slashes)", + requestBody: apiv0.ServerJSON{ + Name: "com.example/server/path", + Description: "Server with multiple slashes in name", + Version: "1.0.0", + Repository: model.Repository{ + URL: "https://github.com/example/test-server", + Source: "github", + ID: "example/test-server", + }, + }, + tokenClaims: &auth.JWTClaims{ + AuthMethod: auth.MethodNone, + Permissions: []auth.Permission{ + {Action: auth.PermissionActionPublish, ResourcePattern: "*"}, + }, + }, + setupRegistryService: func(_ service.RegistryService) {}, + expectedStatus: http.StatusBadRequest, + expectedError: "server name format is invalid: must contain exactly one slash", + }, + { + name: "invalid server name - multiple slashes (three slashes)", + requestBody: apiv0.ServerJSON{ + Name: "org.company/dept/team/project", + Description: "Server with three slashes in name", + Version: "1.0.0", + }, + tokenClaims: &auth.JWTClaims{ + AuthMethod: auth.MethodNone, + Permissions: []auth.Permission{ + {Action: auth.PermissionActionPublish, ResourcePattern: "*"}, + }, + }, + setupRegistryService: func(_ service.RegistryService) {}, + expectedStatus: http.StatusBadRequest, + expectedError: "server name format is invalid: must contain exactly one slash", + }, + { + name: "invalid server name - consecutive slashes", + requestBody: apiv0.ServerJSON{ + Name: "com.example//double-slash", + Description: "Server with consecutive slashes", + Version: "1.0.0", + }, + tokenClaims: &auth.JWTClaims{ + AuthMethod: auth.MethodNone, + Permissions: []auth.Permission{ + {Action: auth.PermissionActionPublish, ResourcePattern: "*"}, + }, + }, + setupRegistryService: func(_ service.RegistryService) {}, + expectedStatus: http.StatusBadRequest, + expectedError: "server name format is invalid: must contain exactly one slash", + }, + { + name: "invalid server name - URL-like path", + requestBody: apiv0.ServerJSON{ + Name: "com.example/servers/v1/api", + Description: "Server with URL-like path structure", + Version: "1.0.0", + }, + tokenClaims: &auth.JWTClaims{ + AuthMethod: auth.MethodNone, + Permissions: []auth.Permission{ + {Action: auth.PermissionActionPublish, ResourcePattern: "*"}, + }, + }, + setupRegistryService: func(_ service.RegistryService) {}, + expectedStatus: http.StatusBadRequest, + expectedError: "server name format is invalid: must contain exactly one slash", + }, + { + name: "invalid server name - many slashes", + requestBody: apiv0.ServerJSON{ + Name: "a/b/c/d/e/f", + Description: "Server with many slashes", + Version: "1.0.0", + }, + tokenClaims: &auth.JWTClaims{ + AuthMethod: auth.MethodNone, + Permissions: []auth.Permission{ + {Action: auth.PermissionActionPublish, ResourcePattern: "*"}, + }, + }, + setupRegistryService: func(_ service.RegistryService) {}, + expectedStatus: http.StatusBadRequest, + expectedError: "server name format is invalid: must contain exactly one slash", + }, + { + name: "invalid server name - with packages and remotes", + requestBody: apiv0.ServerJSON{ + Name: "com.example/test/server/v2", + Description: "Complex server with invalid name", + Version: "2.0.0", + Repository: model.Repository{ + URL: "https://github.com/example/test-server", + Source: "github", + ID: "example/test-server", + }, + Packages: []model.Package{ + { + RegistryType: model.RegistryTypeNPM, + Identifier: "test-package", + Version: "2.0.0", + Transport: model.Transport{ + Type: model.TransportTypeStdio, + }, + }, + }, + Remotes: []model.Transport{ + { + Type: model.TransportTypeStreamableHTTP, + URL: "https://example.com/api", + }, + }, + }, + tokenClaims: &auth.JWTClaims{ + AuthMethod: auth.MethodNone, + Permissions: []auth.Permission{ + {Action: auth.PermissionActionPublish, ResourcePattern: "*"}, + }, + }, + setupRegistryService: func(_ service.RegistryService) {}, + expectedStatus: http.StatusBadRequest, + expectedError: "server name format is invalid: must contain exactly one slash", + }, } for _, tc := range testCases { @@ -279,3 +408,105 @@ func TestPublishEndpoint(t *testing.T) { }) } } + +// TestPublishEndpoint_MultipleSlashesEdgeCases tests additional edge cases for multi-slash validation +func TestPublishEndpoint_MultipleSlashesEdgeCases(t *testing.T) { + testSeed := make([]byte, ed25519.SeedSize) + _, err := rand.Read(testSeed) + require.NoError(t, err) + testConfig := &config.Config{ + JWTPrivateKey: hex.EncodeToString(testSeed), + EnableRegistryValidation: false, + } + + testCases := []struct { + name string + serverName string + expectedStatus int + description string + }{ + { + name: "valid - single slash", + serverName: "com.example/server", + expectedStatus: http.StatusOK, + description: "Valid server name with single slash should succeed", + }, + { + name: "invalid - trailing slash after valid name", + serverName: "com.example/server/", + expectedStatus: http.StatusBadRequest, + description: "Trailing slash creates multiple slashes", + }, + { + name: "invalid - leading and middle slash", + serverName: "/com.example/server", + expectedStatus: http.StatusBadRequest, + description: "Leading slash with middle slash", + }, + { + name: "invalid - file system style path", + serverName: "usr/local/bin/server", + expectedStatus: http.StatusBadRequest, + description: "File system style paths should be rejected", + }, + { + name: "invalid - version-like suffix", + serverName: "com.example/server/v1.0.0", + expectedStatus: http.StatusBadRequest, + description: "Version suffixes with slash should be rejected", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Create registry service + registryService := service.NewRegistryService(database.NewMemoryDB(), testConfig) + + // Create a new ServeMux and Huma API + mux := http.NewServeMux() + api := humago.New(mux, huma.DefaultConfig("Test API", "1.0.0")) + + // Register the endpoint + v0.RegisterPublishEndpoint(api, registryService, testConfig) + + // Create request body + requestBody := apiv0.ServerJSON{ + Name: tc.serverName, + Description: "Test server", + Version: "1.0.0", + } + + bodyBytes, err := json.Marshal(requestBody) + require.NoError(t, err) + + // Create request + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, "/v0/publish", bytes.NewBuffer(bodyBytes)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/json") + + // Set auth header with permissions + tokenClaims := auth.JWTClaims{ + AuthMethod: auth.MethodNone, + Permissions: []auth.Permission{ + {Action: auth.PermissionActionPublish, ResourcePattern: "*"}, + }, + } + token, err := generateTestJWTToken(testConfig, tokenClaims) + require.NoError(t, err) + req.Header.Set("Authorization", "Bearer "+token) + + // Perform request + rr := httptest.NewRecorder() + mux.ServeHTTP(rr, req) + + // Assertions + assert.Equal(t, tc.expectedStatus, rr.Code, + "%s: expected status %d, got %d", tc.description, tc.expectedStatus, rr.Code) + + if tc.expectedStatus == http.StatusBadRequest { + assert.Contains(t, rr.Body.String(), "server name format is invalid: must contain exactly one slash", + "%s: should contain specific error message", tc.description) + } + }) + } +} \ No newline at end of file diff --git a/internal/validators/constants.go b/internal/validators/constants.go index e7a36ea1..21e54b79 100644 --- a/internal/validators/constants.go +++ b/internal/validators/constants.go @@ -25,6 +25,10 @@ var ( ErrInvalidNamedArgumentName = errors.New("invalid named argument name format") ErrArgumentValueStartsWithName = errors.New("argument value cannot start with the argument name") ErrArgumentDefaultStartsWithName = errors.New("argument default cannot start with the argument name") + + // Server name validation errors + ErrInvalidServerNameFormat = errors.New("server name format is invalid: must contain exactly one slash") + ErrMultipleSlashesInServerName = errors.New("server name cannot contain multiple slashes") ) // RepositorySource represents valid repository sources @@ -33,4 +37,4 @@ type RepositorySource string const ( SourceGitHub RepositorySource = "github" SourceGitLab RepositorySource = "gitlab" -) +) \ No newline at end of file diff --git a/internal/validators/validators.go b/internal/validators/validators.go index 95f73ba2..c8892c55 100644 --- a/internal/validators/validators.go +++ b/internal/validators/validators.go @@ -402,6 +402,12 @@ func parseServerName(serverJSON apiv0.ServerJSON) (string, error) { return "", fmt.Errorf("server name must be in format 'dns-namespace/name' (e.g., 'com.example.api/server')") } + // Check for multiple slashes - reject if found + slashCount := strings.Count(name, "/") + if slashCount > 1 { + return "", fmt.Errorf("%w: %s", ErrMultipleSlashesInServerName, name) + } + parts := strings.SplitN(name, "/", 2) if len(parts) != 2 || parts[0] == "" || parts[1] == "" { return "", fmt.Errorf("server name must be in format 'dns-namespace/name' with non-empty namespace and name parts") @@ -504,4 +510,4 @@ func isValidHostForDomain(hostname, publisherDomain string) bool { } return false -} +} \ No newline at end of file diff --git a/internal/validators/validators_test.go b/internal/validators/validators_test.go index ea2b65af..04133524 100644 --- a/internal/validators/validators_test.go +++ b/internal/validators/validators_test.go @@ -171,6 +171,25 @@ func TestValidate(t *testing.T) { }, expectedError: validators.ErrVersionLooksLikeRange.Error(), }, + // Server name validation - multiple slashes + { + name: "server name with two slashes", + serverDetail: apiv0.ServerJSON{ + Name: "com.example/server/path", + Description: "A test server", + Version: "1.0.0", + }, + expectedError: validators.ErrMultipleSlashesInServerName.Error(), + }, + { + name: "server name with three slashes", + serverDetail: apiv0.ServerJSON{ + Name: "com.example/server/path/deep", + Description: "A test server", + Version: "1.0.0", + }, + expectedError: validators.ErrMultipleSlashesInServerName.Error(), + }, { name: "valid server detail with all fields", serverDetail: apiv0.ServerJSON{ @@ -847,11 +866,12 @@ func TestValidate_ServerNameFormat(t *testing.T) { errorMsg: "non-empty namespace and name parts", }, { - name: "multiple slashes - uses first as separator", + name: "multiple slashes - should be rejected", serverDetail: apiv0.ServerJSON{ Name: "com.example/server/path", }, - expectError: false, + expectError: true, + errorMsg: "server name cannot contain multiple slashes", }, } @@ -869,6 +889,84 @@ func TestValidate_ServerNameFormat(t *testing.T) { } } +func TestValidate_MultipleSlashesInServerName(t *testing.T) { + tests := []struct { + name string + serverName string + expectError bool + errorMsg string + }{ + { + name: "single slash - valid", + serverName: "com.example/my-server", + expectError: false, + }, + { + name: "two slashes - invalid", + serverName: "com.example/my-server/extra", + expectError: true, + errorMsg: "server name cannot contain multiple slashes", + }, + { + name: "three slashes - invalid", + serverName: "com.example/my/server/name", + expectError: true, + errorMsg: "server name cannot contain multiple slashes", + }, + { + name: "many slashes - invalid", + serverName: "com.example/a/b/c/d/e", + expectError: true, + errorMsg: "server name cannot contain multiple slashes", + }, + { + name: "double slash - invalid", + serverName: "com.example//server", + expectError: true, + errorMsg: "server name cannot contain multiple slashes", + }, + { + name: "trailing slash counts as two - invalid", + serverName: "com.example/server/", + expectError: true, + errorMsg: "server name cannot contain multiple slashes", + }, + { + name: "no slash - still invalid for different reason", + serverName: "com.example.server", + expectError: true, + errorMsg: "server name must be in format 'dns-namespace/name'", + }, + { + name: "complex valid namespace with single slash", + serverName: "com.microsoft.azure.service/webapp-server", + expectError: false, + }, + { + name: "complex namespace with multiple slashes - invalid", + serverName: "com.microsoft.azure/service/webapp-server", + expectError: true, + errorMsg: "server name cannot contain multiple slashes", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + serverDetail := apiv0.ServerJSON{ + Name: tt.serverName, + } + err := validators.ValidateServerJSON(&serverDetail) + + if tt.expectError { + assert.Error(t, err) + assert.Contains(t, err.Error(), tt.errorMsg) + } else { + assert.NoError(t, err) + } + }) + } +} + func TestValidateArgument_ValidNamedArguments(t *testing.T) { validCases := []model.Argument{ { @@ -1492,4 +1590,4 @@ func createValidServerWithArgument(arg model.Argument) apiv0.ServerJSON { }, }, } -} +} \ No newline at end of file From 74c0cc220de82357e5a3c4f88724d7c48fcc5a82 Mon Sep 17 00:00:00 2001 From: Ossama Lafhel Date: Mon, 15 Sep 2025 17:01:48 +0200 Subject: [PATCH 2/3] docs: add server name format documentation to API types Add descriptive documentation to the Name field in ServerJSON struct explaining the expected format 'reverse-dns-namespace/name' and allowed characters for namespace and name parts. --- pkg/api/v0/types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/api/v0/types.go b/pkg/api/v0/types.go index 700221d3..bee392eb 100644 --- a/pkg/api/v0/types.go +++ b/pkg/api/v0/types.go @@ -29,7 +29,7 @@ type ServerMeta struct { // ServerJSON represents complete server information as defined in the MCP spec, with extension support type ServerJSON struct { Schema string `json:"$schema,omitempty"` - Name string `json:"name" minLength:"1" maxLength:"200"` + Name string `json:"name" minLength:"1" maxLength:"200" doc:"Server name in format 'reverse-dns-namespace/name' (e.g., 'com.example/server'). Namespace allows alphanumeric, dots, hyphens. Name allows alphanumeric, dots, underscores, hyphens."` Description string `json:"description" minLength:"1" maxLength:"100"` Status model.Status `json:"status,omitempty" minLength:"1"` Repository model.Repository `json:"repository,omitempty"` From bb64ce1ba0b80d240ab56b9329d7c0c48c0e099b Mon Sep 17 00:00:00 2001 From: Ossama Lafhel Date: Mon, 15 Sep 2025 17:05:40 +0200 Subject: [PATCH 3/3] fix: move server name pattern validation to code for better error messages - Add regex validation in parseServerName with specific error messages - Validate namespace allows only alphanumeric, dots and hyphens - Validate name allows only alphanumeric, dots, underscores and hyphens - Update tests to expect descriptive error messages - Add tests for invalid character validation - Fix test server names to use valid format - Move regex patterns to utils.go for centralization - Add error constants to constants.go for consistency - Remove redundant validation checks Based on feedback from PR reviewers, this implementation: - Maintains HTTP 400 status instead of 422 for better developer experience - Provides clear, actionable error messages - Follows existing codebase patterns for validation - Centralizes validation logic for maintainability Co-authored-by: Joel Verhagen Co-authored-by: Avish Co-authored-by: Adam Jones --- .github/workflows/release.yml | 2 +- go.mod | 13 ++-- go.sum | 26 ++++---- internal/api/handlers/v0/edit.go | 5 ++ internal/api/handlers/v0/edit_test.go | 2 +- .../handlers/v0/publish_integration_test.go | 2 +- internal/api/handlers/v0/publish_test.go | 61 ++++++++++++++----- internal/service/registry_service.go | 4 +- internal/validators/constants.go | 3 +- internal/validators/utils.go | 5 ++ internal/validators/validators.go | 15 ++++- internal/validators/validators_test.go | 3 +- 12 files changed, 99 insertions(+), 42 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index e3a0239f..a9958803 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -24,7 +24,7 @@ jobs: go-version-file: 'go.mod' - name: Install cosign - uses: sigstore/cosign-installer@d58896d6a1865668819e1d91763c7751a165e159 + uses: sigstore/cosign-installer@d7543c93d881b35a8faa02e8e3605f69b7a1ce62 - name: Install Syft uses: anchore/sbom-action/download-syft@v0.20.5 diff --git a/go.mod b/go.mod index 1ff57ada..8a237fea 100644 --- a/go.mod +++ b/go.mod @@ -9,17 +9,17 @@ require ( github.com/golang-jwt/jwt/v5 v5.3.0 github.com/google/uuid v1.6.0 github.com/jackc/pgx/v5 v5.7.6 - github.com/prometheus/client_golang v1.23.0 + github.com/prometheus/client_golang v1.23.2 github.com/santhosh-tekuri/jsonschema/v5 v5.3.1 github.com/stretchr/testify v1.11.1 go.opentelemetry.io/contrib/instrumentation/runtime v0.63.0 go.opentelemetry.io/otel v1.38.0 - go.opentelemetry.io/otel/exporters/prometheus v0.59.1 + go.opentelemetry.io/otel/exporters/prometheus v0.60.0 go.opentelemetry.io/otel/metric v1.38.0 go.opentelemetry.io/otel/sdk v1.38.0 go.opentelemetry.io/otel/sdk/metric v1.38.0 golang.org/x/mod v0.28.0 - golang.org/x/oauth2 v0.30.0 + golang.org/x/oauth2 v0.31.0 ) require ( @@ -36,15 +36,16 @@ require ( github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/prometheus/client_model v0.6.2 // indirect - github.com/prometheus/common v0.65.0 // indirect - github.com/prometheus/otlptranslator v0.0.0-20250717125610-8549f4ab4f8f // indirect + github.com/prometheus/common v0.66.1 // indirect + github.com/prometheus/otlptranslator v0.0.2 // indirect github.com/prometheus/procfs v0.17.0 // indirect go.opentelemetry.io/auto/sdk v1.1.0 // indirect go.opentelemetry.io/otel/trace v1.38.0 // indirect + go.yaml.in/yaml/v2 v2.4.2 // indirect golang.org/x/crypto v0.41.0 // indirect golang.org/x/sync v0.16.0 // indirect golang.org/x/sys v0.35.0 // indirect golang.org/x/text v0.28.0 // indirect - google.golang.org/protobuf v1.36.6 // indirect + google.golang.org/protobuf v1.36.8 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/go.sum b/go.sum index fa631651..c204224f 100644 --- a/go.sum +++ b/go.sum @@ -46,14 +46,14 @@ github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= -github.com/prometheus/client_golang v1.23.0 h1:ust4zpdl9r4trLY/gSjlm07PuiBq2ynaXXlptpfy8Uc= -github.com/prometheus/client_golang v1.23.0/go.mod h1:i/o0R9ByOnHX0McrTMTyhYvKE4haaf2mW08I+jGAjEE= +github.com/prometheus/client_golang v1.23.2 h1:Je96obch5RDVy3FDMndoUsjAhG5Edi49h0RJWRi/o0o= +github.com/prometheus/client_golang v1.23.2/go.mod h1:Tb1a6LWHB3/SPIzCoaDXI4I8UHKeFTEQ1YCr+0Gyqmg= github.com/prometheus/client_model v0.6.2 h1:oBsgwpGs7iVziMvrGhE53c/GrLUsZdHnqNwqPLxwZyk= github.com/prometheus/client_model v0.6.2/go.mod h1:y3m2F6Gdpfy6Ut/GBsUqTWZqCUvMVzSfMLjcu6wAwpE= -github.com/prometheus/common v0.65.0 h1:QDwzd+G1twt//Kwj/Ww6E9FQq1iVMmODnILtW1t2VzE= -github.com/prometheus/common v0.65.0/go.mod h1:0gZns+BLRQ3V6NdaerOhMbwwRbNh9hkGINtQAsP5GS8= -github.com/prometheus/otlptranslator v0.0.0-20250717125610-8549f4ab4f8f h1:QQB6SuvGZjK8kdc2YaLJpYhV8fxauOsjE6jgcL6YJ8Q= -github.com/prometheus/otlptranslator v0.0.0-20250717125610-8549f4ab4f8f/go.mod h1:P8AwMgdD7XEr6QRUJ2QWLpiAZTgTE2UYgjlu3svompI= +github.com/prometheus/common v0.66.1 h1:h5E0h5/Y8niHc5DlaLlWLArTQI7tMrsfQjHV+d9ZoGs= +github.com/prometheus/common v0.66.1/go.mod h1:gcaUsgf3KfRSwHY4dIMXLPV0K/Wg1oZ8+SbZk/HH/dA= +github.com/prometheus/otlptranslator v0.0.2 h1:+1CdeLVrRQ6Psmhnobldo0kTp96Rj80DRXRd5OSnMEQ= +github.com/prometheus/otlptranslator v0.0.2/go.mod h1:P8AwMgdD7XEr6QRUJ2QWLpiAZTgTE2UYgjlu3svompI= github.com/prometheus/procfs v0.17.0 h1:FuLQ+05u4ZI+SS/w9+BWEM2TXiHKsUQ9TADiRH7DuK0= github.com/prometheus/procfs v0.17.0/go.mod h1:oPQLaDAMRbA+u8H5Pbfq+dl3VDAvHxMUOVhe0wYB2zw= github.com/rogpeppe/go-internal v1.13.1 h1:KvO1DLK/DRN07sQ1LQKScxyZJuNnedQ5/wKSR38lUII= @@ -71,8 +71,8 @@ go.opentelemetry.io/contrib/instrumentation/runtime v0.63.0 h1:PeBoRj6af6xMI7qCu go.opentelemetry.io/contrib/instrumentation/runtime v0.63.0/go.mod h1:ingqBCtMCe8I4vpz/UVzCW6sxoqgZB37nao91mLQ3Bw= go.opentelemetry.io/otel v1.38.0 h1:RkfdswUDRimDg0m2Az18RKOsnI8UDzppJAtj01/Ymk8= go.opentelemetry.io/otel v1.38.0/go.mod h1:zcmtmQ1+YmQM9wrNsTGV/q/uyusom3P8RxwExxkZhjM= -go.opentelemetry.io/otel/exporters/prometheus v0.59.1 h1:HcpSkTkJbggT8bjYP+BjyqPWlD17BH9C5CYNKeDzmcA= -go.opentelemetry.io/otel/exporters/prometheus v0.59.1/go.mod h1:0FJL+gjuUoM07xzik3KPBaN+nz/CoB15kV6WLMiXZag= +go.opentelemetry.io/otel/exporters/prometheus v0.60.0 h1:cGtQxGvZbnrWdC2GyjZi0PDKVSLWP/Jocix3QWfXtbo= +go.opentelemetry.io/otel/exporters/prometheus v0.60.0/go.mod h1:hkd1EekxNo69PTV4OWFGZcKQiIqg0RfuWExcPKFvepk= go.opentelemetry.io/otel/metric v1.38.0 h1:Kl6lzIYGAh5M159u9NgiRkmoMKjvbsKtYRwgfrA6WpA= go.opentelemetry.io/otel/metric v1.38.0/go.mod h1:kB5n/QoRM8YwmUahxvI3bO34eVtQf2i4utNVLr9gEmI= go.opentelemetry.io/otel/sdk v1.38.0 h1:l48sr5YbNf2hpCUj/FoGhW9yDkl+Ma+LrVl8qaM5b+E= @@ -83,20 +83,22 @@ go.opentelemetry.io/otel/trace v1.38.0 h1:Fxk5bKrDZJUH+AMyyIXGcFAPah0oRcT+LuNtJr go.opentelemetry.io/otel/trace v1.38.0/go.mod h1:j1P9ivuFsTceSWe1oY+EeW3sc+Pp42sO++GHkg4wwhs= go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= +go.yaml.in/yaml/v2 v2.4.2 h1:DzmwEr2rDGHl7lsFgAHxmNz/1NlQ7xLIrlN2h5d1eGI= +go.yaml.in/yaml/v2 v2.4.2/go.mod h1:081UH+NErpNdqlCXm3TtEran0rJZGxAYx9hb/ELlsPU= golang.org/x/crypto v0.41.0 h1:WKYxWedPGCTVVl5+WHSSrOBT0O8lx32+zxmHxijgXp4= golang.org/x/crypto v0.41.0/go.mod h1:pO5AFd7FA68rFak7rOAGVuygIISepHftHnr8dr6+sUc= golang.org/x/mod v0.28.0 h1:gQBtGhjxykdjY9YhZpSlZIsbnaE2+PgjfLWUQTnoZ1U= golang.org/x/mod v0.28.0/go.mod h1:yfB/L0NOf/kmEbXjzCPOx1iK1fRutOydrCMsqRhEBxI= -golang.org/x/oauth2 v0.30.0 h1:dnDm7JmhM45NNpd8FDDeLhK6FwqbOf4MLCM9zb1BOHI= -golang.org/x/oauth2 v0.30.0/go.mod h1:B++QgG3ZKulg6sRPGD/mqlHQs5rB3Ml9erfeDY7xKlU= +golang.org/x/oauth2 v0.31.0 h1:8Fq0yVZLh4j4YA47vHKFTa9Ew5XIrCP8LC6UeNZnLxo= +golang.org/x/oauth2 v0.31.0/go.mod h1:lzm5WQJQwKZ3nwavOZ3IS5Aulzxi68dUSgRHujetwEA= golang.org/x/sync v0.16.0 h1:ycBJEhp9p4vXvUZNszeOq0kGTPghopOL8q0fq3vstxw= golang.org/x/sync v0.16.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA= golang.org/x/sys v0.35.0 h1:vz1N37gP5bs89s7He8XuIYXpyY0+QlsKmzipCbUtyxI= golang.org/x/sys v0.35.0/go.mod h1:BJP2sWEmIv4KK5OTEluFJCKSidICx8ciO85XgH3Ak8k= golang.org/x/text v0.28.0 h1:rhazDwis8INMIwQ4tpjLDzUhx6RlXqZNPEM0huQojng= golang.org/x/text v0.28.0/go.mod h1:U8nCwOR8jO/marOQ0QbDiOngZVEBB7MAiitBuMjXiNU= -google.golang.org/protobuf v1.36.6 h1:z1NpPI8ku2WgiWnf+t9wTPsn6eP1L7ksHUlkfLvd9xY= -google.golang.org/protobuf v1.36.6/go.mod h1:jduwjTPXsFjZGTmRluh+L6NjiWu7pchiJ2/5YcXBHnY= +google.golang.org/protobuf v1.36.8 h1:xHScyCOEuuwZEc6UtSOvPbAT4zRh0xcNRYekJwfqyMc= +google.golang.org/protobuf v1.36.8/go.mod h1:fuxRtAxBytpl4zzqUh6/eyUujkJdNiuEkXntxiD/uRU= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q= diff --git a/internal/api/handlers/v0/edit.go b/internal/api/handlers/v0/edit.go index 10256b41..b573ece7 100644 --- a/internal/api/handlers/v0/edit.go +++ b/internal/api/handlers/v0/edit.go @@ -11,6 +11,7 @@ import ( "github.com/modelcontextprotocol/registry/internal/config" "github.com/modelcontextprotocol/registry/internal/database" "github.com/modelcontextprotocol/registry/internal/service" + "github.com/modelcontextprotocol/registry/internal/validators" apiv0 "github.com/modelcontextprotocol/registry/pkg/api/v0" "github.com/modelcontextprotocol/registry/pkg/model" ) @@ -68,6 +69,10 @@ func RegisterEditEndpoints(api huma.API, registry service.RegistryService, cfg * // Prevent renaming servers if currentServer.Name != input.Body.Name { + // Validate the new name format before rejecting the rename + if err := validators.ValidatePublishRequest(ctx, input.Body, cfg); err != nil { + return nil, huma.Error400BadRequest(err.Error()) + } return nil, huma.Error400BadRequest("Cannot rename server") } diff --git a/internal/api/handlers/v0/edit_test.go b/internal/api/handlers/v0/edit_test.go index ef8562f2..71ec430e 100644 --- a/internal/api/handlers/v0/edit_test.go +++ b/internal/api/handlers/v0/edit_test.go @@ -237,7 +237,7 @@ func TestEditServerEndpoint(t *testing.T) { }, serverID: testServerID, expectedStatus: http.StatusBadRequest, - expectedError: "Bad Request", + expectedError: "server name must be in format 'dns-namespace/name'", }, { name: "cannot undelete server", diff --git a/internal/api/handlers/v0/publish_integration_test.go b/internal/api/handlers/v0/publish_integration_test.go index 1ad07ecc..d7a26181 100644 --- a/internal/api/handlers/v0/publish_integration_test.go +++ b/internal/api/handlers/v0/publish_integration_test.go @@ -141,7 +141,7 @@ func TestPublishIntegration(t *testing.T) { t.Run("publish fails with missing authorization header", func(t *testing.T) { publishReq := apiv0.ServerJSON{ - Name: "test-server", + Name: "com.example/test-server", } body, err := json.Marshal(publishReq) diff --git a/internal/api/handlers/v0/publish_test.go b/internal/api/handlers/v0/publish_test.go index 9e246c3d..6e15168c 100644 --- a/internal/api/handlers/v0/publish_test.go +++ b/internal/api/handlers/v0/publish_test.go @@ -80,7 +80,7 @@ func TestPublishEndpoint(t *testing.T) { { name: "successful publish with no auth (AuthMethodNone)", requestBody: apiv0.ServerJSON{ - Name: "example/test-server", + Name: "com.example/test-server", Description: "A test server without auth", Repository: model.Repository{ URL: "https://github.com/example/test-server", @@ -92,7 +92,7 @@ func TestPublishEndpoint(t *testing.T) { tokenClaims: &auth.JWTClaims{ AuthMethod: auth.MethodNone, Permissions: []auth.Permission{ - {Action: auth.PermissionActionPublish, ResourcePattern: "example/*"}, + {Action: auth.PermissionActionPublish, ResourcePattern: "com.example/*"}, }, }, setupRegistryService: func(_ service.RegistryService) { @@ -127,7 +127,7 @@ func TestPublishEndpoint(t *testing.T) { { name: "invalid token", requestBody: apiv0.ServerJSON{ - Name: "test-server", + Name: "com.example/test-server", Description: "A test server", Version: "1.0.0", }, @@ -165,7 +165,7 @@ func TestPublishEndpoint(t *testing.T) { { name: "registry service error", requestBody: apiv0.ServerJSON{ - Name: "example/test-server", + Name: "com.example/test-server", Description: "A test server", Version: "1.0.0", Repository: model.Repository{ @@ -183,7 +183,7 @@ func TestPublishEndpoint(t *testing.T) { setupRegistryService: func(registry service.RegistryService) { // Pre-publish the same server to cause duplicate version error existingServer := apiv0.ServerJSON{ - Name: "example/test-server", + Name: "com.example/test-server", Description: "Existing test server", Version: "1.0.0", Repository: model.Repository{ @@ -224,7 +224,6 @@ func TestPublishEndpoint(t *testing.T) { setupRegistryService: func(_ service.RegistryService) {}, expectedStatus: http.StatusOK, }, - // IB-2-registry: Integration test for multi-slash server name rejection { name: "invalid server name - multiple slashes (two slashes)", requestBody: apiv0.ServerJSON{ @@ -245,7 +244,7 @@ func TestPublishEndpoint(t *testing.T) { }, setupRegistryService: func(_ service.RegistryService) {}, expectedStatus: http.StatusBadRequest, - expectedError: "server name format is invalid: must contain exactly one slash", + expectedError: "server name cannot contain multiple slashes", }, { name: "invalid server name - multiple slashes (three slashes)", @@ -262,7 +261,7 @@ func TestPublishEndpoint(t *testing.T) { }, setupRegistryService: func(_ service.RegistryService) {}, expectedStatus: http.StatusBadRequest, - expectedError: "server name format is invalid: must contain exactly one slash", + expectedError: "server name cannot contain multiple slashes", }, { name: "invalid server name - consecutive slashes", @@ -279,7 +278,7 @@ func TestPublishEndpoint(t *testing.T) { }, setupRegistryService: func(_ service.RegistryService) {}, expectedStatus: http.StatusBadRequest, - expectedError: "server name format is invalid: must contain exactly one slash", + expectedError: "server name cannot contain multiple slashes", }, { name: "invalid server name - URL-like path", @@ -296,7 +295,7 @@ func TestPublishEndpoint(t *testing.T) { }, setupRegistryService: func(_ service.RegistryService) {}, expectedStatus: http.StatusBadRequest, - expectedError: "server name format is invalid: must contain exactly one slash", + expectedError: "server name cannot contain multiple slashes", }, { name: "invalid server name - many slashes", @@ -313,7 +312,7 @@ func TestPublishEndpoint(t *testing.T) { }, setupRegistryService: func(_ service.RegistryService) {}, expectedStatus: http.StatusBadRequest, - expectedError: "server name format is invalid: must contain exactly one slash", + expectedError: "server name cannot contain multiple slashes", }, { name: "invalid server name - with packages and remotes", @@ -351,7 +350,41 @@ func TestPublishEndpoint(t *testing.T) { }, setupRegistryService: func(_ service.RegistryService) {}, expectedStatus: http.StatusBadRequest, - expectedError: "server name format is invalid: must contain exactly one slash", + expectedError: "server name cannot contain multiple slashes", + }, + { + name: "invalid server name - invalid namespace characters", + requestBody: apiv0.ServerJSON{ + Name: "com.example@/test-server", + Description: "Server with invalid namespace characters", + Version: "1.0.0", + }, + tokenClaims: &auth.JWTClaims{ + AuthMethod: auth.MethodNone, + Permissions: []auth.Permission{ + {Action: auth.PermissionActionPublish, ResourcePattern: "*"}, + }, + }, + setupRegistryService: func(_ service.RegistryService) {}, + expectedStatus: http.StatusBadRequest, + expectedError: "namespace contains invalid characters", + }, + { + name: "invalid server name - invalid name characters", + requestBody: apiv0.ServerJSON{ + Name: "com.example/test@server", + Description: "Server with invalid name characters", + Version: "1.0.0", + }, + tokenClaims: &auth.JWTClaims{ + AuthMethod: auth.MethodNone, + Permissions: []auth.Permission{ + {Action: auth.PermissionActionPublish, ResourcePattern: "*"}, + }, + }, + setupRegistryService: func(_ service.RegistryService) {}, + expectedStatus: http.StatusBadRequest, + expectedError: "name contains invalid characters", }, } @@ -504,8 +537,8 @@ func TestPublishEndpoint_MultipleSlashesEdgeCases(t *testing.T) { "%s: expected status %d, got %d", tc.description, tc.expectedStatus, rr.Code) if tc.expectedStatus == http.StatusBadRequest { - assert.Contains(t, rr.Body.String(), "server name format is invalid: must contain exactly one slash", - "%s: should contain specific error message", tc.description) + assert.Contains(t, rr.Body.String(), "server name", + "%s: should contain server name validation error", tc.description) } }) } diff --git a/internal/service/registry_service.go b/internal/service/registry_service.go index 86dc0108..a5b221a6 100644 --- a/internal/service/registry_service.go +++ b/internal/service/registry_service.go @@ -77,7 +77,7 @@ func (s *registryServiceImpl) Publish(req apiv0.ServerJSON) (*apiv0.ServerJSON, defer cancel() // Validate the request - if err := validators.ValidatePublishRequest(req, s.cfg); err != nil { + if err := validators.ValidatePublishRequest(ctx, req, s.cfg); err != nil { return nil, err } @@ -206,7 +206,7 @@ func (s *registryServiceImpl) EditServer(id string, req apiv0.ServerJSON) (*apiv defer cancel() // Validate the request - if err := validators.ValidatePublishRequest(req, s.cfg); err != nil { + if err := validators.ValidatePublishRequest(ctx, req, s.cfg); err != nil { return nil, err } diff --git a/internal/validators/constants.go b/internal/validators/constants.go index 21e54b79..622064b6 100644 --- a/internal/validators/constants.go +++ b/internal/validators/constants.go @@ -27,8 +27,9 @@ var ( ErrArgumentDefaultStartsWithName = errors.New("argument default cannot start with the argument name") // Server name validation errors - ErrInvalidServerNameFormat = errors.New("server name format is invalid: must contain exactly one slash") ErrMultipleSlashesInServerName = errors.New("server name cannot contain multiple slashes") + ErrInvalidNamespaceCharacters = errors.New("namespace contains invalid characters: only alphanumeric characters, dots (.) and hyphens (-) are allowed") + ErrInvalidNameCharacters = errors.New("name contains invalid characters: only alphanumeric characters, dots (.), underscores (_) and hyphens (-) are allowed") ) // RepositorySource represents valid repository sources diff --git a/internal/validators/utils.go b/internal/validators/utils.go index 52c4b00f..5a521345 100644 --- a/internal/validators/utils.go +++ b/internal/validators/utils.go @@ -12,6 +12,11 @@ var ( // For example: // - GitHub: https://github.com/user/repo githubURLRegex = regexp.MustCompile(`^https?://(www\.)?github\.com/[\w.-]+/[\w.-]+/?$`) gitlabURLRegex = regexp.MustCompile(`^https?://(www\.)?gitlab\.com/[\w.-]+/[\w.-]+/?$`) + + // Regular expressions for validating server name components + // Pattern: ^[a-zA-Z0-9.-]+/[a-zA-Z0-9._-]+$ + serverNamespacePattern = regexp.MustCompile(`^[a-zA-Z0-9.-]+$`) + serverNamePattern = regexp.MustCompile(`^[a-zA-Z0-9._-]+$`) ) // IsValidRepositoryURL checks if the given URL is valid for the specified repository source diff --git a/internal/validators/validators.go b/internal/validators/validators.go index c8892c55..1c9fbd1c 100644 --- a/internal/validators/validators.go +++ b/internal/validators/validators.go @@ -343,7 +343,7 @@ func validateRemoteTransport(obj *model.Transport) error { } // ValidatePublishRequest validates a complete publish request including extensions -func ValidatePublishRequest(req apiv0.ServerJSON, cfg *config.Config) error { +func ValidatePublishRequest(ctx context.Context, req apiv0.ServerJSON, cfg *config.Config) error { // Validate publisher extensions in _meta if err := validatePublisherExtensions(req); err != nil { return err @@ -356,7 +356,6 @@ func ValidatePublishRequest(req apiv0.ServerJSON, cfg *config.Config) error { // Validate registry ownership for all packages if validation is enabled and server is not deleted if cfg.EnableRegistryValidation && req.Status != model.StatusDeleted { - ctx := context.Background() for i, pkg := range req.Packages { if err := ValidatePackage(ctx, pkg, req.Name); err != nil { return fmt.Errorf("registry validation failed for package %d (%s): %w", i, pkg.Identifier, err) @@ -405,7 +404,7 @@ func parseServerName(serverJSON apiv0.ServerJSON) (string, error) { // Check for multiple slashes - reject if found slashCount := strings.Count(name, "/") if slashCount > 1 { - return "", fmt.Errorf("%w: %s", ErrMultipleSlashesInServerName, name) + return "", ErrMultipleSlashesInServerName } parts := strings.SplitN(name, "/", 2) @@ -413,6 +412,16 @@ func parseServerName(serverJSON apiv0.ServerJSON) (string, error) { return "", fmt.Errorf("server name must be in format 'dns-namespace/name' with non-empty namespace and name parts") } + // Validate namespace and name format according to schema + // Pattern: ^[a-zA-Z0-9.-]+/[a-zA-Z0-9._-]+$ + if !serverNamespacePattern.MatchString(parts[0]) { + return "", fmt.Errorf("server namespace '%s' %w", parts[0], ErrInvalidNamespaceCharacters) + } + + if !serverNamePattern.MatchString(parts[1]) { + return "", fmt.Errorf("server name '%s' %w", parts[1], ErrInvalidNameCharacters) + } + return name, nil } diff --git a/internal/validators/validators_test.go b/internal/validators/validators_test.go index 04133524..d698a8a2 100644 --- a/internal/validators/validators_test.go +++ b/internal/validators/validators_test.go @@ -1,6 +1,7 @@ package validators_test import ( + "context" "fmt" "testing" @@ -1550,7 +1551,7 @@ func TestValidate_RegistryTypesAndUrls(t *testing.T) { }, } - err := validators.ValidatePublishRequest(serverJSON, &config.Config{ + err := validators.ValidatePublishRequest(context.Background(), serverJSON, &config.Config{ EnableRegistryValidation: true, }) if tc.expectError {