From 272ee9322d7f630a23d9d53723a77db0dd09f595 Mon Sep 17 00:00:00 2001 From: Tadas Antanavicius Date: Tue, 30 Sep 2025 10:07:42 -0700 Subject: [PATCH] fix: add server name validation and clean invalid data before migration 009 (#591) ## Summary This PR adds two important improvements to ensure data integrity: 1. **Adds server name format validation at the API layer** to match the database constraint from migration 008 2. **Includes migration 008 to clean up invalid data** before applying strict constraints in migration 009 ## Changes ### Server Name Validation - Enforces validation at the API layer matching the database constraint - **Namespace pattern**: `[a-zA-Z0-9][a-zA-Z0-9.-]*[a-zA-Z0-9]` - Must start and end with alphanumeric characters - Can contain dots and hyphens in the middle - **Name pattern**: `[a-zA-Z0-9][a-zA-Z0-9._-]*[a-zA-Z0-9]` - Must start and end with alphanumeric characters - Can contain dots, underscores, and hyphens in the middle - Provides clear, user-friendly error messages indicating which part is invalid - Uses DRY approach with composed regex patterns ### Migration 008: Data Cleanup - Removes servers with invalid names or empty versions (5 servers affected in production) - Fixes invalid status values by setting them to 'active' (1 server affected) - Removes duplicate name+version combinations - Includes comprehensive safety checks to prevent accidental data loss - Must run before migration 009 which adds strict constraints ## Testing - All validator tests pass - Migration includes safety checks that match production data - CI verification included --------- Co-authored-by: Claude --- go.mod | 1 + go.sum | 2 + .../migrations/008_clean_invalid_data.sql | 197 ++++++++++++++++++ ...sql => 009_separate_official_metadata.sql} | 0 internal/validators/constants.go | 1 + internal/validators/validators.go | 29 +++ 6 files changed, 230 insertions(+) create mode 100644 internal/database/migrations/008_clean_invalid_data.sql rename internal/database/migrations/{008_separate_official_metadata.sql => 009_separate_official_metadata.sql} (100%) diff --git a/go.mod b/go.mod index 955c9be..5efe39b 100644 --- a/go.mod +++ b/go.mod @@ -32,6 +32,7 @@ require ( github.com/jackc/pgpassfile v1.0.0 // indirect github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 // indirect github.com/jackc/puddle/v2 v2.2.2 // indirect + github.com/lib/pq v1.10.9 // indirect 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 diff --git a/go.sum b/go.sum index c204224..61138e5 100644 --- a/go.sum +++ b/go.sum @@ -42,6 +42,8 @@ github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0SNc= github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw= +github.com/lib/pq v1.10.9 h1:YXG7RB+JIjhP29X+OtkiDnYaXQwpS4JEWq7dtCCRUEw= +github.com/lib/pq v1.10.9/go.mod h1:AlVN5x4E4T544tWzH6hKfbfQvm3HdbOxrmggDNAPY9o= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA= 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= diff --git a/internal/database/migrations/008_clean_invalid_data.sql b/internal/database/migrations/008_clean_invalid_data.sql new file mode 100644 index 0000000..82762ec --- /dev/null +++ b/internal/database/migrations/008_clean_invalid_data.sql @@ -0,0 +1,197 @@ +-- Migration 008: Clean up invalid data before applying stricter constraints in migration 009 +-- This migration removes or fixes data that would violate constraints introduced in the next migration + +BEGIN; + +-- Safety check: Count exactly what we'll be modifying +DO $$ +DECLARE + invalid_name_count INTEGER; + empty_version_count INTEGER; + invalid_status_count INTEGER; + duplicate_count INTEGER; + total_to_delete INTEGER; + total_servers INTEGER; + has_known_bad_server BOOLEAN; + r RECORD; +BEGIN + -- Get total server count + SELECT COUNT(*) INTO total_servers FROM servers; + + -- Check if we have the specific known problematic server from production + -- This server only exists in production data, not in test fixtures + SELECT EXISTS( + SELECT 1 FROM servers + WHERE value->>'name' = 'io.github.joelverhagen/Knapcode.SampleMcpServer/aot' + ) INTO has_known_bad_server; + + -- Skip migration if we don't have the known bad data (indicates test environment) + IF NOT has_known_bad_server THEN + RAISE NOTICE 'Migration 008: Skipping cleanup - no known problematic data found (likely test/dev environment)'; + RETURN; -- Exit early, don't run any cleanup + END IF; + + RAISE NOTICE 'Migration 008: Found known problematic data, proceeding with cleanup'; + + -- Count servers with invalid name format + SELECT COUNT(*) INTO invalid_name_count + FROM servers + WHERE value->>'name' NOT SIMILAR TO '[a-zA-Z0-9][a-zA-Z0-9.-]*[a-zA-Z0-9]/[a-zA-Z0-9][a-zA-Z0-9._-]*[a-zA-Z0-9]'; + + -- Count servers with empty or NULL versions + SELECT COUNT(*) INTO empty_version_count + FROM servers + WHERE value->>'version' IS NULL OR value->>'version' = ''; + + -- Count servers with invalid status + SELECT COUNT(*) INTO invalid_status_count + FROM servers + WHERE value->>'status' IS NOT NULL + AND value->>'status' != '' + AND value->>'status' NOT IN ('active', 'deprecated', 'deleted'); + + -- Count duplicate name+version combinations + SELECT COUNT(*) INTO duplicate_count + FROM ( + SELECT value->>'name', value->>'version', COUNT(*) as cnt + FROM servers + GROUP BY value->>'name', value->>'version' + HAVING COUNT(*) > 1 + ) dups; + + -- Calculate total deletions (some servers might have both invalid name AND empty version) + SELECT COUNT(*) INTO total_to_delete + FROM servers + WHERE value->>'name' NOT SIMILAR TO '[a-zA-Z0-9][a-zA-Z0-9.-]*[a-zA-Z0-9]/[a-zA-Z0-9][a-zA-Z0-9._-]*[a-zA-Z0-9]' + OR value->>'version' IS NULL + OR value->>'version' = ''; + + -- Log the cleanup operations with safety check + RAISE NOTICE 'Migration 008 Data Cleanup Plan:'; + RAISE NOTICE ' Total servers in database: %', total_servers; + + IF total_servers > 0 THEN + RAISE NOTICE ' Servers to DELETE: % (% percent of total)', total_to_delete, ROUND((total_to_delete::numeric / total_servers * 100), 2); + ELSE + RAISE NOTICE ' Servers to DELETE: %', total_to_delete; + END IF; + + RAISE NOTICE ' - Invalid names: %', invalid_name_count; + RAISE NOTICE ' - Empty versions: %', empty_version_count; + RAISE NOTICE ' Servers to UPDATE (fix status): %', invalid_status_count; + RAISE NOTICE ' Duplicate name+version pairs: %', duplicate_count; + + -- Always log the specific servers that will be deleted for transparency + IF total_to_delete > 0 THEN + RAISE NOTICE ''; + RAISE NOTICE 'Servers that will be deleted:'; + FOR r IN + SELECT value->>'name' as name, value->>'version' as version, + CASE + WHEN value->>'name' NOT SIMILAR TO '[a-zA-Z0-9][a-zA-Z0-9.-]*[a-zA-Z0-9]/[a-zA-Z0-9][a-zA-Z0-9._-]*[a-zA-Z0-9]' THEN 'Invalid name format' + WHEN value->>'version' IS NULL THEN 'NULL version' + WHEN value->>'version' = '' THEN 'Empty version' + END as reason + FROM servers + WHERE value->>'name' NOT SIMILAR TO '[a-zA-Z0-9][a-zA-Z0-9.-]*[a-zA-Z0-9]/[a-zA-Z0-9][a-zA-Z0-9._-]*[a-zA-Z0-9]' + OR value->>'version' IS NULL + OR value->>'version' = '' + ORDER BY value->>'name' + LOOP + RAISE NOTICE ' - % @ % (reason: %)', r.name, COALESCE(r.version, ''), r.reason; + END LOOP; + END IF; + + -- Always log the specific servers that will have status updated for transparency + IF invalid_status_count > 0 THEN + RAISE NOTICE ''; + RAISE NOTICE 'Servers that will have status updated:'; + FOR r IN + SELECT value->>'name' as name, value->>'version' as version, value->>'status' as status + FROM servers + WHERE value->>'status' IS NOT NULL + AND value->>'status' != '' + AND value->>'status' NOT IN ('active', 'deprecated', 'deleted') + ORDER BY value->>'name' + LOOP + RAISE NOTICE ' - % @ % (current status: %)', r.name, r.version, r.status; + END LOOP; + END IF; + + -- SAFETY CHECK: Ensure we're deleting exactly the expected data + -- We only reach this point if we found the known bad server + -- In production (2025-09-30), we expect exactly 5 deletions and 1 status update + IF total_to_delete != 5 THEN + RAISE EXCEPTION 'Safety check failed: Expected to delete exactly 5 servers but would delete %. Check the log above for details. Aborting to prevent data loss.', total_to_delete; + END IF; + + IF invalid_status_count != 1 THEN + RAISE EXCEPTION 'Safety check failed: Expected to update exactly 1 server status but would update %. Check the log above for details. Aborting to prevent data corruption.', invalid_status_count; + END IF; +END $$; + +-- Delete servers with invalid names or empty versions +-- These cannot be reasonably fixed and would violate primary key constraints +DELETE FROM servers +WHERE value->>'name' NOT SIMILAR TO '[a-zA-Z0-9][a-zA-Z0-9.-]*[a-zA-Z0-9]/[a-zA-Z0-9][a-zA-Z0-9._-]*[a-zA-Z0-9]' + OR value->>'version' IS NULL + OR value->>'version' = ''; + +-- Fix invalid status values by setting them to 'active' +-- These can be reasonably defaulted to a valid value +UPDATE servers +SET value = jsonb_set(value, '{status}', '"active"') +WHERE value->>'status' IS NOT NULL + AND value->>'status' != '' + AND value->>'status' NOT IN ('active', 'deprecated', 'deleted'); + +-- Remove duplicate name+version combinations +-- Keep the one with the most recent publishedAt date +DELETE FROM servers s1 +WHERE EXISTS ( + SELECT 1 FROM servers s2 + WHERE s2.value->>'name' = s1.value->>'name' + AND s2.value->>'version' = s1.value->>'version' + AND (s2.value->>'publishedAt')::timestamp > (s1.value->>'publishedAt')::timestamp +); + +-- Verify the operations completed as expected +DO $$ +DECLARE + remaining_count INTEGER; + actual_deleted INTEGER; + actual_updated INTEGER; + still_invalid_names INTEGER; + still_empty_versions INTEGER; + still_invalid_status INTEGER; +BEGIN + SELECT COUNT(*) INTO remaining_count FROM servers; + + -- Check if any invalid data remains + SELECT COUNT(*) INTO still_invalid_names + FROM servers + WHERE value->>'name' NOT SIMILAR TO '[a-zA-Z0-9][a-zA-Z0-9.-]*[a-zA-Z0-9]/[a-zA-Z0-9][a-zA-Z0-9._-]*[a-zA-Z0-9]'; + + SELECT COUNT(*) INTO still_empty_versions + FROM servers + WHERE value->>'version' IS NULL OR value->>'version' = ''; + + SELECT COUNT(*) INTO still_invalid_status + FROM servers + WHERE value->>'status' IS NOT NULL + AND value->>'status' != '' + AND value->>'status' NOT IN ('active', 'deprecated', 'deleted'); + + RAISE NOTICE 'Data cleanup complete:'; + RAISE NOTICE ' Servers remaining: %', remaining_count; + RAISE NOTICE ' Invalid names remaining: %', still_invalid_names; + RAISE NOTICE ' Empty versions remaining: %', still_empty_versions; + RAISE NOTICE ' Invalid status remaining: %', still_invalid_status; + + -- Final safety check: Ensure we cleaned everything we intended to + IF still_invalid_names > 0 OR still_empty_versions > 0 OR still_invalid_status > 0 THEN + RAISE EXCEPTION 'Cleanup incomplete! Invalid data still remains. Aborting.'; + END IF; +END $$; + +COMMIT; \ No newline at end of file diff --git a/internal/database/migrations/008_separate_official_metadata.sql b/internal/database/migrations/009_separate_official_metadata.sql similarity index 100% rename from internal/database/migrations/008_separate_official_metadata.sql rename to internal/database/migrations/009_separate_official_metadata.sql diff --git a/internal/validators/constants.go b/internal/validators/constants.go index 661ac28..cf1f462 100644 --- a/internal/validators/constants.go +++ b/internal/validators/constants.go @@ -28,6 +28,7 @@ var ( // Server name validation errors ErrMultipleSlashesInServerName = errors.New("server name cannot contain multiple slashes") + ErrInvalidServerNameFormat = errors.New("server name format is invalid") ) // RepositorySource represents valid repository sources diff --git a/internal/validators/validators.go b/internal/validators/validators.go index d21a421..25cdf32 100644 --- a/internal/validators/validators.go +++ b/internal/validators/validators.go @@ -14,6 +14,18 @@ import ( "github.com/modelcontextprotocol/registry/pkg/model" ) +// Server name validation patterns +var ( + // Component patterns for namespace and name parts + namespacePattern = `[a-zA-Z0-9][a-zA-Z0-9.-]*[a-zA-Z0-9]` + namePartPattern = `[a-zA-Z0-9][a-zA-Z0-9._-]*[a-zA-Z0-9]` + + // Compiled regexes + namespaceRegex = regexp.MustCompile(`^` + namespacePattern + `$`) + namePartRegex = regexp.MustCompile(`^` + namePartPattern + `$`) + serverNameRegex = regexp.MustCompile(`^` + namespacePattern + `/` + namePartPattern + `$`) +) + // Regexes to detect semver range syntaxes var ( // Case 1: comparator ranges @@ -403,11 +415,28 @@ func parseServerName(serverJSON apiv0.ServerJSON) (string, error) { return "", ErrMultipleSlashesInServerName } + // Split and check for empty parts 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") } + // Validate name format using regex + if !serverNameRegex.MatchString(name) { + namespace := parts[0] + serverName := parts[1] + + // Check which part is invalid for a better error message + if !namespaceRegex.MatchString(namespace) { + return "", fmt.Errorf("%w: namespace '%s' is invalid. Namespace must start and end with alphanumeric characters, and may contain dots and hyphens in the middle", ErrInvalidServerNameFormat, namespace) + } + if !namePartRegex.MatchString(serverName) { + return "", fmt.Errorf("%w: name '%s' is invalid. Name must start and end with alphanumeric characters, and may contain dots, underscores, and hyphens in the middle", ErrInvalidServerNameFormat, serverName) + } + // Fallback in case both somehow pass individually but not together + return "", fmt.Errorf("%w: invalid format for '%s'", ErrInvalidServerNameFormat, name) + } + return name, nil }