feat: add regex validation to server name in API types (#471) #479

ossamalafhel · 2025-09-15T12:00:29Z

Summary

Following @domdomegg's suggestion in #476, this PR started as adding regex validation to the server name field but evolved based on extensive reviewer feedback into a comprehensive validation improvement.

Changes

Initial Implementation

Added pattern validation to the Name field in pkg/api/v0/types.go
Updated tests to expect HTTP 422 for validation errors

Based on Review Feedback

Removed regex from API annotations - Moved validation to handler code to maintain HTTP 400 status (more familiar than 422) as suggested by @joelverhagen
Updated regex pattern - Changed from ^[^/]+/[^/]+$ to the more specific ^[a-zA-Z0-9.-]+/[a-zA-Z0-9._-]+$ to match the schema definition
Moved regex patterns to utils.go - Centralized all validation regexes in one place as suggested by @Avish34
Added error constants to constants.go - Created ErrInvalidNamespaceCharacters and ErrInvalidNameCharacters for consistent error messages
Improved error messages - Now provides clearer messages like "server name must match pattern..." instead of generic validation errors
Removed redundant validation - Eliminated unnecessary strings.Contains(name, "//") check that was already covered by slash count validation

Key Decisions

Chose handler validation over schema validation to maintain HTTP 400 status and provide better error messages
Kept case-sensitive validation allowing uppercase letters (discussion with @joelverhagen about potentially enforcing lowercase for future consideration)
Centralized validation logic following the existing codebase patterns

Context

This PR demonstrates the value of code review - what started as a simple regex addition evolved into a better implementation that:

Maintains backward compatibility with HTTP 400 status codes
Provides clearer, more actionable error messages
Follows the codebase's existing patterns for validation
Makes the code more maintainable by centralizing regex patterns and error messages

Related to #471
Builds on #476

Testing

All existing tests updated and passing
Added comprehensive test cases for the new validation pattern
Verified error messages are clear and helpful
Tested various edge cases discovered during review

Note: Special thanks to @joelverhagen, @Avish34, and @domdomegg for their thorough reviews that significantly improved this implementation.

…otocol#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

…e-validation-inconsistency fix: validate server names to allow only single slash (modelcontextprotocol#471)

internal/validators/validators.go

internal/api/handlers/v0/publish_test.go

internal/api/handlers/v0/edit_test.go

pkg/api/v0/types.go

joelverhagen · 2025-09-15T15:07:28Z

internal/validators/validators.go

+	// Validate namespace and name format according to schema
+	// Pattern: ^[a-zA-Z0-9.-]+/[a-zA-Z0-9._-]+$
+	namespacePattern := regexp.MustCompile(`^[a-zA-Z0-9.-]+$`)
+	namePattern := regexp.MustCompile(`^[a-zA-Z0-9._-]+$`)


@domdomegg - are server names case sensitive? NuGet and PyPI treat package names as case insensitive at the API level but npm and Docker do not (last I checked). Personally, I recommend enforcing lowercase everywhere and being case sensitive. It is has caused headaches in NuGet and the roughly "domain name + path" pattern of server names is not unlike URLs which have a lowercase domain name (ubiquitous) and kebab case path portion (not ubiquitous but common in say GH repo names).

@joelverhagen Good point! Currently the regex allows uppercase letters in both namespace and name parts.

I agree that enforcing lowercase would be beneficial for consistency, especially since:

The namespace part follows reverse DNS convention where domains are lowercase

It avoids confusion between Com.Example/server vs com.example/server

It aligns with web conventions

Should we update the regex to enforce lowercase in this PR or create a separate issue for it? If we do it here, we'd need to:

Update the regex patterns to ^[a-z0-9.-]+/[a-z0-9._-]+$

Update the error messages and documentation

Add tests for uppercase rejection

What do you think @domdomegg?

nit: Can we move the regex to utils.go? We have kept regex for the URLs there. It kind of make sense to have all regex used in validation at one place.

@Avish34 Thanks for the review! Good suggestions:

Moved regex patterns to utils.go alongside other validation regexes

Added constant error messages in constants.go:

ErrInvalidNamespaceCharacters

ErrInvalidNameCharacters

This keeps all validation patterns centralized and error messages consistent. Changes implemented in the latest commit!

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.

internal/validators/validators.go

…sages - 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 <[email protected]> Co-authored-by: Avish <[email protected]> Co-authored-by: Adam Jones <[email protected]>

domdomegg · 2025-09-19T20:18:17Z

internal/api/handlers/v0/edit.go


 		// Prevent renaming servers
 		if currentServer.Name != input.Body.Name {
+			// Validate the new name format before rejecting the rename


This seems unnecessary, if we're going to reject the request anyway?

domdomegg · 2025-09-19T20:20:53Z

internal/validators/validators.go

 	}

+	// Check for multiple slashes - reject if found
+	slashCount := strings.Count(name, "/")


I think this would already be caught by the parts split and patterns below? Ideally I want to keep this simple as it's hard to follow when there are quite so many rules - eliminating overlapping/duplicate ones is valuable. Possibly can all be merged into one fairly simple regex.

ossamalafhel and others added 2 commits September 13, 2025 22:22

Merge pull request #1 from ossamalafhel/feature/bug-report-server-nam…

800bd4c

…e-validation-inconsistency fix: validate server names to allow only single slash (modelcontextprotocol#471)

ossamalafhel mentioned this pull request Sep 15, 2025

fix: validate server names to allow only single slash (#471) #476

Merged

9 tasks

ossamalafhel force-pushed the fix/add-regex-validation-server-name branch 16 times, most recently from c43df8e to aad4d4b Compare September 15, 2025 13:30

ossamalafhel changed the title ~~feat: add regex validation to server name in API types~~ feat: add regex validation to server name in API types (#398) Sep 15, 2025

ossamalafhel changed the title ~~feat: add regex validation to server name in API types (#398)~~ feat: add regex validation to server name in API types Sep 15, 2025

ossamalafhel changed the title ~~feat: add regex validation to server name in API types~~ feat: add regex validation to server name in API types (#471) Sep 15, 2025

domdomegg reviewed Sep 15, 2025

View reviewed changes

internal/validators/validators.go Outdated Show resolved Hide resolved

ossamalafhel force-pushed the fix/add-regex-validation-server-name branch from aad4d4b to 6a6e710 Compare September 15, 2025 14:25