Visitar URL original
fix(validators): reject invalid argument types and empty positional a… · modelcontextprotocol/registry@053877e · GitHub
Skip to content

Commit 053877e

Browse files
JosephDoUrdenJosephDoUrden
committed
fix(validators): reject invalid argument types and empty positional arguments
1 parent 6036804 commit 053877e

5 files changed

Lines changed: 188 additions & 3 deletions

File tree

‎internal/api/handlers/v0/publish_test.go‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -384,6 +384,41 @@ func TestPublishEndpoint(t *testing.T) {
384384
expectedStatus: http.StatusUnprocessableEntity,
385385
expectedError: "Failed to publish server, invalid schema: call /validate for details",
386386
},
387+
{
388+
name: "invalid argument - empty type",
389+
requestBody: apiv0.ServerJSON{
390+
Schema: model.CurrentSchemaURL,
391+
Name: "com.example/test-server",
392+
Description: "A test server",
393+
Version: "1.0.0",
394+
Packages: []model.Package{
395+
{
396+
RegistryType: model.RegistryTypeNPM,
397+
Identifier: "test-package",
398+
Version: "1.0.0",
399+
Transport: model.Transport{
400+
Type: model.TransportTypeStdio,
401+
},
402+
RuntimeArguments: []model.Argument{
403+
{
404+
InputWithVariables: model.InputWithVariables{Input: model.Input{Value: "mcp"}},
405+
Type: "", // type present but empty string, not a missing key
406+
Name: "command",
407+
},
408+
},
409+
},
410+
},
411+
},
412+
tokenClaims: &auth.JWTClaims{
413+
AuthMethod: auth.MethodNone,
414+
Permissions: []auth.Permission{
415+
{Action: auth.PermissionActionPublish, ResourcePattern: "*"},
416+
},
417+
},
418+
setupRegistryService: func(_ service.RegistryService) {},
419+
expectedStatus: http.StatusUnprocessableEntity,
420+
expectedError: "Failed to publish server, invalid schema: call /validate for details",
421+
},
387422
}
388423

389424
for _, tc := range testCases {

‎internal/validators/constants.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,10 @@ var (
2727
ErrArgumentValueStartsWithName = errors.New("argument value cannot start with the argument name")
2828
ErrArgumentDefaultStartsWithName = errors.New("argument default cannot start with the argument name")
2929

30+
// Argument type and shape validation errors
31+
ErrInvalidArgumentType = errors.New("argument type must be 'positional' or 'named'")
32+
ErrPositionalArgumentValueRequired = errors.New("positional argument must provide a value or a valueHint")
33+
3034
// Server name validation errors
3135
ErrMultipleSlashesInServerName = errors.New("server name cannot contain multiple slashes")
3236
ErrInvalidServerNameFormat = errors.New("server name format is invalid")

‎internal/validators/validation_detailed_test.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,12 @@ func TestValidateServerJSON_ContextPaths(t *testing.T) {
163163
Type: model.ArgumentTypeNamed,
164164
Name: "invalid name", // Error in second package's argument
165165
},
166+
{
167+
Type: "", // Error: argument type outside the enum
168+
},
169+
{
170+
Type: model.ArgumentTypePositional, // Error: positional without value or valueHint
171+
},
166172
},
167173
},
168174
},
@@ -180,6 +186,16 @@ func TestValidateServerJSON_ContextPaths(t *testing.T) {
180186
// Should have issues at specific nested paths
181187
assert.True(t, issuePaths["packages[0].version"], "Should have issue at packages[0].version")
182188
assert.True(t, issuePaths["packages[1].runtimeArguments[0].name"], "Should have issue at packages[1].runtimeArguments[0].name")
189+
assert.True(t, issuePaths["packages[1].runtimeArguments[1].type"], "Should have issue at packages[1].runtimeArguments[1].type")
190+
assert.True(t, issuePaths["packages[1].runtimeArguments[2]"], "Should have issue at packages[1].runtimeArguments[2]")
191+
192+
// Pin the API-visible references for the argument checks
193+
issueRefs := make(map[string]string)
194+
for _, issue := range result.Issues {
195+
issueRefs[issue.Path] = issue.Reference
196+
}
197+
assert.Equal(t, "invalid-argument-type", issueRefs["packages[1].runtimeArguments[1].type"])
198+
assert.Equal(t, "positional-argument-value-or-hint-required", issueRefs["packages[1].runtimeArguments[2]"])
183199
}
184200

185201
func TestValidateServerJSON_RefResolution(t *testing.T) {

‎internal/validators/validators.go‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -396,14 +396,35 @@ func looksLikeVersionRange(version string) bool {
396396
func validateArgument(ctx *ValidationContext, obj *model.Argument) *ValidationResult {
397397
result := &ValidationResult{Valid: true, Issues: []ValidationIssue{}}
398398

399-
if obj.Type == model.ArgumentTypeNamed {
399+
switch obj.Type {
400+
case model.ArgumentTypeNamed:
400401
// Validate named argument name format
401402
nameResult := validateNamedArgumentName(ctx.Field("name"), obj.Name)
402403
result.Merge(nameResult)
403404

404405
// Validate value and default don't start with the name
405406
valueResult := validateArgumentValueFields(ctx, obj.Name, obj.Value, obj.Default)
406407
result.Merge(valueResult)
408+
case model.ArgumentTypePositional:
409+
// Positional arguments need a value or a valueHint to be usable
410+
if obj.Value == "" && obj.ValueHint == "" {
411+
issue := NewValidationIssueFromError(
412+
ValidationIssueTypeSemantic,
413+
ctx.String(),
414+
ErrPositionalArgumentValueRequired,
415+
"positional-argument-value-or-hint-required",
416+
)
417+
result.AddIssue(issue)
418+
}
419+
default:
420+
// Reject unknown argument types, including the empty string
421+
issue := NewValidationIssueFromError(
422+
ValidationIssueTypeSemantic,
423+
ctx.Field("type").String(),
424+
fmt.Errorf("%w: %q", ErrInvalidArgumentType, obj.Type),
425+
"invalid-argument-type",
426+
)
427+
result.AddIssue(issue)
407428
}
408429
return result
409430
}

‎internal/validators/validators_test.go‎

Lines changed: 111 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"testing"
77

88
"github.com/stretchr/testify/assert"
9+
"github.com/stretchr/testify/require"
910

1011
"github.com/modelcontextprotocol/registry/internal/config"
1112
"github.com/modelcontextprotocol/registry/internal/validators"
@@ -1241,8 +1242,8 @@ func TestValidateArgument_ValidNamedArguments(t *testing.T) {
12411242

12421243
func TestValidateArgument_ValidPositionalArguments(t *testing.T) {
12431244
positionalCases := []model.Argument{
1244-
{Type: model.ArgumentTypePositional, Name: "anything with spaces"},
1245-
{Type: model.ArgumentTypePositional, Name: "anything<with>brackets"},
1245+
{Type: model.ArgumentTypePositional, Name: "anything with spaces", ValueHint: "target_dir"},
1246+
{Type: model.ArgumentTypePositional, Name: "anything<with>brackets", ValueHint: "target_dir"},
12461247
{
12471248
InputWithVariables: model.InputWithVariables{Input: model.Input{Value: "--port 8080"}},
12481249
Type: model.ArgumentTypePositional,
@@ -1396,6 +1397,105 @@ func TestValidateArgument_ValidValueFields(t *testing.T) {
13961397
}
13971398
}
13981399

1400+
func TestValidateArgument_InvalidArgumentType(t *testing.T) {
1401+
invalidTypeCases := []struct {
1402+
name string
1403+
arg model.Argument
1404+
}{
1405+
{
1406+
// Same shape as live entry io.github.Jordan-Horner/symbols@1.0.1
1407+
// (there under packageArguments): name and value are set, so only
1408+
// the type check can fire
1409+
"empty_type",
1410+
model.Argument{
1411+
InputWithVariables: model.InputWithVariables{Input: model.Input{Value: "mcp"}},
1412+
Type: "",
1413+
Name: "command",
1414+
},
1415+
},
1416+
{"literal", model.Argument{Type: "literal"}},
1417+
{"flag", model.Argument{Type: "flag", Name: "--verbose"}},
1418+
{"environment", model.Argument{Type: "environment", Name: "API_KEY"}},
1419+
// Type values are case-sensitive: "Positional" is not "positional"
1420+
{"uppercase_positional", model.Argument{Type: "Positional"}},
1421+
}
1422+
1423+
for _, tc := range invalidTypeCases {
1424+
t.Run("Invalid_"+tc.name, func(t *testing.T) {
1425+
server := createValidServerWithArgument(tc.arg)
1426+
// ValidateServerJSON returns all validation results; using FirstError() to preserve existing test behavior
1427+
// In future, consider using result.Issues for comprehensive error reporting
1428+
result := validators.ValidateServerJSON(&server, validators.ValidationSchemaVersionAndSemantic)
1429+
err := result.FirstError()
1430+
require.Error(t, err, "Expected error for invalid argument type: %+v", tc.arg)
1431+
assert.Contains(t, err.Error(), "argument type must be 'positional' or 'named'")
1432+
})
1433+
}
1434+
}
1435+
1436+
func TestValidateArgument_PositionalValueOrHint(t *testing.T) {
1437+
positionalCases := []struct {
1438+
name string
1439+
arg model.Argument
1440+
expectError bool
1441+
}{
1442+
{
1443+
"missing_value_and_value_hint",
1444+
model.Argument{Type: model.ArgumentTypePositional},
1445+
true,
1446+
},
1447+
{
1448+
"value_only",
1449+
model.Argument{
1450+
InputWithVariables: model.InputWithVariables{Input: model.Input{Value: "input.txt"}},
1451+
Type: model.ArgumentTypePositional,
1452+
},
1453+
false,
1454+
},
1455+
{
1456+
"value_hint_only",
1457+
model.Argument{Type: model.ArgumentTypePositional, ValueHint: "file_path"},
1458+
false,
1459+
},
1460+
{
1461+
"value_and_value_hint",
1462+
model.Argument{
1463+
InputWithVariables: model.InputWithVariables{Input: model.Input{Value: "input.txt"}},
1464+
Type: model.ArgumentTypePositional,
1465+
ValueHint: "file_path",
1466+
},
1467+
false,
1468+
},
1469+
}
1470+
1471+
for _, tc := range positionalCases {
1472+
t.Run(tc.name, func(t *testing.T) {
1473+
server := createValidServerWithArgument(tc.arg)
1474+
// ValidateServerJSON returns all validation results; using FirstError() to preserve existing test behavior
1475+
// In future, consider using result.Issues for comprehensive error reporting
1476+
result := validators.ValidateServerJSON(&server, validators.ValidationSchemaVersionAndSemantic)
1477+
err := result.FirstError()
1478+
if tc.expectError {
1479+
require.Error(t, err, "Expected error for positional argument without value or valueHint: %+v", tc.arg)
1480+
assert.Contains(t, err.Error(), "positional argument must provide a value or a valueHint")
1481+
} else {
1482+
assert.NoError(t, err, "Expected valid positional argument %+v", tc.arg)
1483+
}
1484+
})
1485+
}
1486+
1487+
// The same check must apply to arguments under packageArguments
1488+
t.Run("missing_value_and_value_hint_in_package_arguments", func(t *testing.T) {
1489+
server := createValidServerWithPackageArgument(model.Argument{Type: model.ArgumentTypePositional})
1490+
// ValidateServerJSON returns all validation results; using FirstError() to preserve existing test behavior
1491+
// In future, consider using result.Issues for comprehensive error reporting
1492+
result := validators.ValidateServerJSON(&server, validators.ValidationSchemaVersionAndSemantic)
1493+
err := result.FirstError()
1494+
require.Error(t, err, "Expected error for positional package argument without value or valueHint")
1495+
assert.Contains(t, err.Error(), "positional argument must provide a value or a valueHint")
1496+
})
1497+
}
1498+
13991499
// Helper function to create a valid server with a specific argument for testing
14001500
func TestValidate_TransportValidation(t *testing.T) {
14011501
tests := []struct {
@@ -1980,6 +2080,15 @@ func createValidServerWithArgument(arg model.Argument) apiv0.ServerJSON {
19802080
}
19812081
}
19822082

2083+
// createValidServerWithPackageArgument is createValidServerWithArgument with the
2084+
// argument under packageArguments instead of runtimeArguments
2085+
func createValidServerWithPackageArgument(arg model.Argument) apiv0.ServerJSON {
2086+
server := createValidServerWithArgument(arg)
2087+
server.Packages[0].RuntimeArguments = nil
2088+
server.Packages[0].PackageArguments = []model.Argument{arg}
2089+
return server
2090+
}
2091+
19832092
func TestValidateTitle(t *testing.T) {
19842093
tests := []struct {
19852094
name string

0 commit comments

Comments
 (0)