-
Notifications
You must be signed in to change notification settings - Fork 204
feat: namespace parsing validation #218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: remote-verification
Are you sure you want to change the base?
Changes from 39 commits
72a1967
ffbed48
f375640
f7a85fa
36579a1
382925e
a9afe32
bc4d621
60ac929
1202ae0
a19b6b5
d8dba84
2e4dc96
2be9c9c
bf1c847
b36f6af
21ef581
1d0316d
f2e1fde
80b36bf
147fc46
36262ff
0ebca38
efc077a
ea7857d
d378b27
adacb2c
3322f15
3b1c54a
455ec12
7343dfb
ec1c6d2
758f0f4
80b74a6
dd203a1
c506ed0
d675d28
3e7ae7e
87978d6
8502e4e
a96a31a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -11,6 +11,7 @@ import ( | |||||||||||||||||||||||||||
"github.com/modelcontextprotocol/registry/internal/auth" | ||||||||||||||||||||||||||||
"github.com/modelcontextprotocol/registry/internal/database" | ||||||||||||||||||||||||||||
"github.com/modelcontextprotocol/registry/internal/model" | ||||||||||||||||||||||||||||
"github.com/modelcontextprotocol/registry/internal/namespace" | ||||||||||||||||||||||||||||
"github.com/modelcontextprotocol/registry/internal/service" | ||||||||||||||||||||||||||||
"golang.org/x/net/html" | ||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
|
@@ -54,6 +55,17 @@ func PublishHandler(registry service.RegistryService, authService auth.Service) | |||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// Validate namespace format if it follows domain-scoped convention | ||||||||||||||||||||||||||||
if err := namespace.ValidateNamespace(serverDetail.Name); err != nil { | ||||||||||||||||||||||||||||
// If the namespace doesn't follow domain-scoped format, check if it's a legacy format | ||||||||||||||||||||||||||||
if !errors.Is(err, namespace.ErrInvalidNamespace) { | ||||||||||||||||||||||||||||
http.Error(w, "Invalid namespace: "+err.Error(), http.StatusBadRequest) | ||||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
// For legacy formats, we'll allow them to pass through for now | ||||||||||||||||||||||||||||
// This provides backward compatibility while encouraging new domain-scoped formats | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic for distinguishing between invalid namespaces and legacy formats is confusing. If any error other than ErrInvalidNamespace causes rejection, but ErrInvalidNamespace allows pass-through, the condition should be more explicit about what constitutes a 'legacy format'.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// Version is required | ||||||||||||||||||||||||||||
if serverDetail.VersionDetail.Version == "" { | ||||||||||||||||||||||||||||
http.Error(w, "Version is required", http.StatusBadRequest) | ||||||||||||||||||||||||||||
|
@@ -73,15 +85,31 @@ func PublishHandler(registry service.RegistryService, authService auth.Service) | |||||||||||||||||||||||||||
token = authHeader[7:] | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// Determine authentication method based on server name prefix | ||||||||||||||||||||||||||||
// Determine authentication method based on server name format | ||||||||||||||||||||||||||||
var authMethod model.AuthMethod | ||||||||||||||||||||||||||||
switch { | ||||||||||||||||||||||||||||
case strings.HasPrefix(serverDetail.Name, "io.github"): | ||||||||||||||||||||||||||||
authMethod = model.AuthMethodGitHub | ||||||||||||||||||||||||||||
// Additional cases can be added here for other prefixes | ||||||||||||||||||||||||||||
default: | ||||||||||||||||||||||||||||
// Keep the default auth method as AuthMethodNone | ||||||||||||||||||||||||||||
authMethod = model.AuthMethodNone | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// Check if the namespace is domain-scoped and extract domain for auth | ||||||||||||||||||||||||||||
if parsed, err := namespace.ParseNamespace(serverDetail.Name); err == nil { | ||||||||||||||||||||||||||||
// For domain-scoped namespaces, determine auth method based on domain | ||||||||||||||||||||||||||||
switch parsed.Domain { | ||||||||||||||||||||||||||||
case "github.com": | ||||||||||||||||||||||||||||
authMethod = model.AuthMethodGitHub | ||||||||||||||||||||||||||||
// Additional domain-specific auth methods can be added here | ||||||||||||||||||||||||||||
default: | ||||||||||||||||||||||||||||
// For other domains, require GitHub auth for now | ||||||||||||||||||||||||||||
// NOTE: Domain verification system needs to be implemented | ||||||||||||||||||||||||||||
authMethod = model.AuthMethodGitHub | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||
// Legacy namespace format - use existing logic | ||||||||||||||||||||||||||||
switch { | ||||||||||||||||||||||||||||
case strings.HasPrefix(serverDetail.Name, "io.github"): | ||||||||||||||||||||||||||||
authMethod = model.AuthMethodGitHub | ||||||||||||||||||||||||||||
// Additional cases can be added here for other prefixes | ||||||||||||||||||||||||||||
default: | ||||||||||||||||||||||||||||
// Keep the default auth method as AuthMethodNone | ||||||||||||||||||||||||||||
authMethod = model.AuthMethodNone | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
serverName := html.EscapeString(serverDetail.Name) | ||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,126 @@ | ||
//nolint:testpackage // Internal package testing allows access to private functions | ||
package v0 | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/modelcontextprotocol/registry/internal/namespace" | ||
) | ||
|
||
// TestNamespaceValidationIntegration tests that the namespace validation | ||
// functionality is working correctly for various namespace formats | ||
func TestNamespaceValidationIntegration(t *testing.T) { | ||
tests := []struct { | ||
name string | ||
serverName string | ||
shouldPass bool | ||
description string | ||
}{ | ||
{ | ||
name: "valid domain-scoped namespace", | ||
serverName: "com.github/my-server", | ||
shouldPass: true, | ||
description: "Standard domain-scoped namespace should be valid", | ||
}, | ||
{ | ||
name: "valid subdomain namespace", | ||
serverName: "com.github.api/tool", | ||
shouldPass: true, | ||
description: "Subdomain-scoped namespace should be valid", | ||
}, | ||
{ | ||
name: "valid apache commons namespace", | ||
serverName: "org.apache.commons/utility", | ||
shouldPass: true, | ||
description: "Apache commons style namespace should be valid", | ||
}, | ||
{ | ||
name: "valid kubernetes namespace", | ||
serverName: "io.kubernetes/plugin", | ||
shouldPass: true, | ||
description: "Kubernetes.io style namespace should be valid", | ||
}, | ||
{ | ||
name: "reserved namespace - localhost", | ||
serverName: "com.localhost/server", | ||
shouldPass: false, | ||
description: "Reserved localhost namespace should be rejected", | ||
}, | ||
{ | ||
name: "reserved namespace - example", | ||
serverName: "com.example/server", | ||
shouldPass: false, | ||
description: "Reserved example namespace should be rejected", | ||
}, | ||
{ | ||
name: "legacy format - io.github prefix", | ||
serverName: "io.github.username/my-server", | ||
shouldPass: true, // This is valid reverse domain notation | ||
description: "Legacy io.github format should be valid as reverse domain notation", | ||
}, | ||
{ | ||
name: "invalid format - forward domain notation", | ||
serverName: "github.com/my-server", | ||
shouldPass: false, | ||
description: "Forward domain notation should be rejected", | ||
}, | ||
{ | ||
name: "invalid format - simple name", | ||
serverName: "my-server", | ||
shouldPass: false, | ||
description: "Simple server name should not match domain-scoped pattern", | ||
}, | ||
} | ||
|
||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
err := namespace.ValidateNamespace(tt.serverName) | ||
|
||
if tt.shouldPass { | ||
if err != nil { | ||
t.Errorf("Expected namespace '%s' to be valid, but got error: %v", tt.serverName, err) | ||
} | ||
} else { | ||
if err == nil { | ||
t.Errorf("Expected namespace '%s' to be invalid, but validation passed", tt.serverName) | ||
} | ||
} | ||
}) | ||
} | ||
} | ||
|
||
// TestDomainExtractionIntegration tests the domain extraction functionality | ||
func TestDomainExtractionIntegration(t *testing.T) { | ||
// Test valid extractions | ||
validTests := []struct { | ||
namespace string | ||
domain string | ||
}{ | ||
{"com.github/my-server", "github.com"}, | ||
{"com.github.api/tool", "api.github.com"}, | ||
{"org.apache.commons/utility", "commons.apache.org"}, | ||
{"io.kubernetes/plugin", "kubernetes.io"}, | ||
} | ||
|
||
for _, test := range validTests { | ||
t.Run("extract_"+test.domain, func(t *testing.T) { | ||
domain, err := namespace.ParseDomainFromNamespace(test.namespace) | ||
if err != nil { | ||
t.Errorf("Unexpected error: %v", err) | ||
} else if domain != test.domain { | ||
t.Errorf("Expected '%s', got '%s'", test.domain, domain) | ||
} | ||
}) | ||
} | ||
|
||
// Test invalid extractions | ||
invalidTests := []string{"invalid-format", "github.com/server", "simple"} | ||
for _, test := range invalidTests { | ||
t.Run("invalid_"+test, func(t *testing.T) { | ||
_, err := namespace.ParseDomainFromNamespace(test) | ||
if err == nil { | ||
t.Errorf("Expected error for '%s'", test) | ||
} | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit confused by the "legacy format" here - what is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a skim to me here, it seems like it's referring to "legacy" as
Legacy io.github format
, but I agree that's odd; it's not legacy, we intend to support it indefinitely (and in fact have a mechanism to potentially extend it to also support e.g.com.gitlab.{user}
etc. So I'd call it something more likePsuedo-domain io.github format
orBorrowed domain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense that this isn't a legacy format, @claude Please fix the comment here to reflect that this is a pseudo-domain io.github format/borrowed domain.