Skip to content

Commit 020fed3

Browse files
JosephDoUrdenrdimitrov
JosephDoUrden
and
rdimitrov
authored
fix(validators): reject invalid argument types and empty positional arguments (#1583)
## Summary Publish accepts arguments that violate the schema. `validateArgument` only checks the named branch, so a `type` that is empty or outside the enum passes through, and a positional argument with neither `value` nor `valueHint` also passes. The scan in #1546 counted 52 entries with bad argument types and 8 bare positional ones, my own paging of /v0/servers found even more of the first kind. One I checked by hand: `io.github.Jordan-Horner/[email protected]` (empty type under packageArguments), another is `com.keyboardcrumbs/[email protected]` (type "literal", still active). Every published schema version requires `type` to be "positional" or "named" and positional args to carry `value` or `valueHint`. Relates to #1546, the empty repository case is #1555's, and version "latest" is already handled since #413. ## Change `validateArgument` becomes a switch. Named branch unchanged. Positional now requires `value` or `valueHint`. Unknown or empty `type` is rejected. Two new errors in constants.go, references `invalid-argument-type` and `positional-argument-value-or-hint-required`. Why not full schema validation at publish: the enhanced validation design doc gates that behind the anyOf error consolidation (Phase 3), raw anyOf failures on arguments come out as a noisy multi-error cluster. These two checks give one clean message each and are one case arm each, easy to delete at Phase 5 when schema-first lands. Until then publish and edit just agree with what /v0/validate already reports. Heads up on two consequences: editing a stored server that still carries one of these legacy arguments will 422 until the argument is fixed, and the importer skips invalid servers with a warning, so a re-seed from a live dump drops those entries instead of failing. Also #1339 adds a check inside the named branch of the same function, whichever lands second has a small conflict to resolve there, happy to do it from my side. ## Test Tests first, they fail on main, the handler regression actually publishes an empty-type argument with 200 on main and gets 422 with the fix. On the branch: go test ./internal/... all green including the handler tests against postgres, golangci-lint v2.13.1 (what CI pins) 0 issues, make validate passes. Two existing fixtures had positional args with neither value nor valueHint, they got a valueHint since no published schema version ever allowed that shape. New error paths and references are pinned in validation_detailed_test.go. ## AI Disclosure AI assistance (Claude) was used for issue research and during development. All changes were reviewed and tested by the author. --------- Co-authored-by: Radoslav Dimitrov <[email protected]>
1 parent 8a2efd0 commit 020fed3

9 files changed

Lines changed: 404 additions & 3 deletions

File tree

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

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
package v0_test
2+
3+
import (
4+
"bytes"
5+
"context"
6+
"crypto/ed25519"
7+
"crypto/rand"
8+
"encoding/hex"
9+
"encoding/json"
10+
"fmt"
11+
"net/http"
12+
"net/http/httptest"
13+
"testing"
14+
15+
"github.com/danielgtaylor/huma/v2"
16+
"github.com/danielgtaylor/huma/v2/adapters/humago"
17+
v0 "github.com/modelcontextprotocol/registry/internal/api/handlers/v0"
18+
"github.com/modelcontextprotocol/registry/internal/auth"
19+
"github.com/modelcontextprotocol/registry/internal/config"
20+
"github.com/modelcontextprotocol/registry/internal/database"
21+
"github.com/modelcontextprotocol/registry/internal/service"
22+
"github.com/modelcontextprotocol/registry/pkg/model"
23+
"github.com/stretchr/testify/require"
24+
)
25+
26+
// Use raw JSON: marshaling a Go string with omitempty would hide the difference
27+
// between a missing value and an explicitly empty CLI argument.
28+
func TestPublishEndpoint_ArgumentPresence(t *testing.T) {
29+
seed := make([]byte, ed25519.SeedSize)
30+
_, err := rand.Read(seed)
31+
require.NoError(t, err)
32+
cfg := &config.Config{JWTPrivateKey: hex.EncodeToString(seed)}
33+
token, err := generateTestJWTToken(cfg, auth.JWTClaims{
34+
AuthMethod: auth.MethodNone,
35+
Permissions: []auth.Permission{
36+
{Action: auth.PermissionActionPublish, ResourcePattern: "example/*"},
37+
},
38+
})
39+
require.NoError(t, err)
40+
41+
for _, prefix := range []string{"/v0", "/v0.1"} {
42+
for _, field := range []string{"runtimeArguments", "packageArguments"} {
43+
t.Run(prefix+"/"+field, func(t *testing.T) {
44+
registry := service.NewRegistryService(database.NewTestDB(t), cfg)
45+
mux := http.NewServeMux()
46+
api := humago.New(mux, huma.DefaultConfig("Test API", "1.0.0"))
47+
v0.RegisterPublishEndpoint(api, prefix, registry, cfg)
48+
49+
for i, tc := range []struct {
50+
name string
51+
arg string
52+
status int
53+
}{
54+
{"empty value", `{"type":"positional","value":""}`, http.StatusOK},
55+
{"empty hint", `{"type":"positional","valueHint":""}`, http.StatusOK},
56+
{"both empty", `{"type":"positional","value":"","valueHint":""}`, http.StatusOK},
57+
{"value", `{"type":"positional","value":"file.txt"}`, http.StatusOK},
58+
{"hint", `{"type":"positional","valueHint":"file"}`, http.StatusOK},
59+
{"named", `{"type":"named","name":"--file","value":""}`, http.StatusOK},
60+
{"missing", `{"type":"positional"}`, http.StatusUnprocessableEntity},
61+
{"null", `{"type":"positional","value":null}`, http.StatusUnprocessableEntity},
62+
{"invalid type", `{"type":"literal","value":"file.txt"}`, http.StatusUnprocessableEntity},
63+
} {
64+
t.Run(tc.name, func(t *testing.T) {
65+
name := fmt.Sprintf("example/argument-%d", i)
66+
body := fmt.Sprintf(`{"$schema":%q,"name":%q,"description":"Argument presence regression test","version":"1.0.0","packages":[{"registryType":"npm","identifier":"test-package","version":"1.0.0","transport":{"type":"stdio"},%q:[%s]}]}`, model.CurrentSchemaURL, name, field, tc.arg)
67+
req := httptest.NewRequest(http.MethodPost, prefix+"/publish", bytes.NewBufferString(body))
68+
req.Header.Set("Content-Type", "application/json")
69+
req.Header.Set("Authorization", "Bearer "+token)
70+
rr := httptest.NewRecorder()
71+
mux.ServeHTTP(rr, req)
72+
require.Equal(t, tc.status, rr.Code, rr.Body.String())
73+
if tc.status != http.StatusOK {
74+
return
75+
}
76+
77+
// Read through the service again to exercise JSONB storage decoding,
78+
// then check that the value survives response serialization too.
79+
stored, err := registry.GetServerByNameAndVersion(context.Background(), name, "1.0.0", false)
80+
require.NoError(t, err)
81+
args := stored.Server.Packages[0].RuntimeArguments
82+
if field == "packageArguments" {
83+
args = stored.Server.Packages[0].PackageArguments
84+
}
85+
require.Len(t, args, 1)
86+
encoded, err := json.Marshal(args[0])
87+
require.NoError(t, err)
88+
require.JSONEq(t, tc.arg, string(encoded))
89+
require.Contains(t, rr.Body.String(), string(encoded))
90+
})
91+
}
92+
})
93+
}
94+
}
95+
}

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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -403,6 +403,41 @@ func TestPublishEndpoint(t *testing.T) {
403403
expectedStatus: http.StatusUnprocessableEntity,
404404
expectedError: "Failed to publish server, invalid schema: call /validate for details",
405405
},
406+
{
407+
name: "invalid argument - empty type",
408+
requestBody: apiv0.ServerJSON{
409+
Schema: model.CurrentSchemaURL,
410+
Name: "com.example/test-server",
411+
Description: "A test server",
412+
Version: "1.0.0",
413+
Packages: []model.Package{
414+
{
415+
RegistryType: model.RegistryTypeNPM,
416+
Identifier: "test-package",
417+
Version: "1.0.0",
418+
Transport: model.Transport{
419+
Type: model.TransportTypeStdio,
420+
},
421+
RuntimeArguments: []model.Argument{
422+
{
423+
InputWithVariables: model.InputWithVariables{Input: model.Input{Value: "mcp"}},
424+
Type: "", // type present but empty string, not a missing key
425+
Name: "command",
426+
},
427+
},
428+
},
429+
},
430+
},
431+
tokenClaims: &auth.JWTClaims{
432+
AuthMethod: auth.MethodNone,
433+
Permissions: []auth.Permission{
434+
{Action: auth.PermissionActionPublish, ResourcePattern: "*"},
435+
},
436+
},
437+
setupRegistryService: func(_ service.RegistryService) {},
438+
expectedStatus: http.StatusUnprocessableEntity,
439+
expectedError: "Failed to publish server, invalid schema: call /validate for details",
440+
},
406441
}
407442

408443
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: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -420,14 +420,36 @@ func looksLikeVersionRange(version string) bool {
420420
func validateArgument(ctx *ValidationContext, obj *model.Argument) *ValidationResult {
421421
result := &ValidationResult{Valid: true, Issues: []ValidationIssue{}}
422422

423-
if obj.Type == model.ArgumentTypeNamed {
423+
switch obj.Type {
424+
case model.ArgumentTypeNamed:
424425
// Validate named argument name format
425426
nameResult := validateNamedArgumentName(ctx.Field("name"), obj.Name)
426427
result.Merge(nameResult)
427428

428429
// Validate value and default don't start with the name
429430
valueResult := validateArgumentValueFields(ctx, obj.Name, obj.Value, obj.Default)
430431
result.Merge(valueResult)
432+
case model.ArgumentTypePositional:
433+
// The schema requires value or valueHint to be present; an explicitly
434+
// empty value is a valid CLI argument.
435+
if !obj.HasValueOrHint() {
436+
issue := NewValidationIssueFromError(
437+
ValidationIssueTypeSemantic,
438+
ctx.String(),
439+
ErrPositionalArgumentValueRequired,
440+
"positional-argument-value-or-hint-required",
441+
)
442+
result.AddIssue(issue)
443+
}
444+
default:
445+
// Reject unknown argument types, including the empty string
446+
issue := NewValidationIssueFromError(
447+
ValidationIssueTypeSemantic,
448+
ctx.Field("type").String(),
449+
fmt.Errorf("%w: %q", ErrInvalidArgumentType, obj.Type),
450+
"invalid-argument-type",
451+
)
452+
result.AddIssue(issue)
431453
}
432454
return result
433455
}

‎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"
@@ -1302,8 +1303,8 @@ func TestValidateArgument_ValidNamedArguments(t *testing.T) {
13021303

13031304
func TestValidateArgument_ValidPositionalArguments(t *testing.T) {
13041305
positionalCases := []model.Argument{
1305-
{Type: model.ArgumentTypePositional, Name: "anything with spaces"},
1306-
{Type: model.ArgumentTypePositional, Name: "anything<with>brackets"},
1306+
{Type: model.ArgumentTypePositional, Name: "anything with spaces", ValueHint: "target_dir"},
1307+
{Type: model.ArgumentTypePositional, Name: "anything<with>brackets", ValueHint: "target_dir"},
13071308
{
13081309
InputWithVariables: model.InputWithVariables{Input: model.Input{Value: "--port 8080"}},
13091310
Type: model.ArgumentTypePositional,
@@ -1457,6 +1458,105 @@ func TestValidateArgument_ValidValueFields(t *testing.T) {
14571458
}
14581459
}
14591460

1461+
func TestValidateArgument_InvalidArgumentType(t *testing.T) {
1462+
invalidTypeCases := []struct {
1463+
name string
1464+
arg model.Argument
1465+
}{
1466+
{
1467+
// Same shape as live entry io.github.Jordan-Horner/[email protected]
1468+
// (there under packageArguments): name and value are set, so only
1469+
// the type check can fire
1470+
"empty_type",
1471+
model.Argument{
1472+
InputWithVariables: model.InputWithVariables{Input: model.Input{Value: "mcp"}},
1473+
Type: "",
1474+
Name: "command",
1475+
},
1476+
},
1477+
{"literal", model.Argument{Type: "literal"}},
1478+
{"flag", model.Argument{Type: "flag", Name: "--verbose"}},
1479+
{"environment", model.Argument{Type: "environment", Name: "API_KEY"}},
1480+
// Type values are case-sensitive: "Positional" is not "positional"
1481+
{"uppercase_positional", model.Argument{Type: "Positional"}},
1482+
}
1483+
1484+
for _, tc := range invalidTypeCases {
1485+
t.Run("Invalid_"+tc.name, func(t *testing.T) {
1486+
server := createValidServerWithArgument(tc.arg)
1487+
// ValidateServerJSON returns all validation results; using FirstError() to preserve existing test behavior
1488+
// In future, consider using result.Issues for comprehensive error reporting
1489+
result := validators.ValidateServerJSON(&server, validators.ValidationSchemaVersionAndSemantic)
1490+
err := result.FirstError()
1491+
require.Error(t, err, "Expected error for invalid argument type: %+v", tc.arg)
1492+
assert.Contains(t, err.Error(), "argument type must be 'positional' or 'named'")
1493+
})
1494+
}
1495+
}
1496+
1497+
func TestValidateArgument_PositionalValueOrHint(t *testing.T) {
1498+
positionalCases := []struct {
1499+
name string
1500+
arg model.Argument
1501+
expectError bool
1502+
}{
1503+
{
1504+
"missing_value_and_value_hint",
1505+
model.Argument{Type: model.ArgumentTypePositional},
1506+
true,
1507+
},
1508+
{
1509+
"value_only",
1510+
model.Argument{
1511+
InputWithVariables: model.InputWithVariables{Input: model.Input{Value: "input.txt"}},
1512+
Type: model.ArgumentTypePositional,
1513+
},
1514+
false,
1515+
},
1516+
{
1517+
"value_hint_only",
1518+
model.Argument{Type: model.ArgumentTypePositional, ValueHint: "file_path"},
1519+
false,
1520+
},
1521+
{
1522+
"value_and_value_hint",
1523+
model.Argument{
1524+
InputWithVariables: model.InputWithVariables{Input: model.Input{Value: "input.txt"}},
1525+
Type: model.ArgumentTypePositional,
1526+
ValueHint: "file_path",
1527+
},
1528+
false,
1529+
},
1530+
}
1531+
1532+
for _, tc := range positionalCases {
1533+
t.Run(tc.name, func(t *testing.T) {
1534+
server := createValidServerWithArgument(tc.arg)
1535+
// ValidateServerJSON returns all validation results; using FirstError() to preserve existing test behavior
1536+
// In future, consider using result.Issues for comprehensive error reporting
1537+
result := validators.ValidateServerJSON(&server, validators.ValidationSchemaVersionAndSemantic)
1538+
err := result.FirstError()
1539+
if tc.expectError {
1540+
require.Error(t, err, "Expected error for positional argument without value or valueHint: %+v", tc.arg)
1541+
assert.Contains(t, err.Error(), "positional argument must provide a value or a valueHint")
1542+
} else {
1543+
assert.NoError(t, err, "Expected valid positional argument %+v", tc.arg)
1544+
}
1545+
})
1546+
}
1547+
1548+
// The same check must apply to arguments under packageArguments
1549+
t.Run("missing_value_and_value_hint_in_package_arguments", func(t *testing.T) {
1550+
server := createValidServerWithPackageArgument(model.Argument{Type: model.ArgumentTypePositional})
1551+
// ValidateServerJSON returns all validation results; using FirstError() to preserve existing test behavior
1552+
// In future, consider using result.Issues for comprehensive error reporting
1553+
result := validators.ValidateServerJSON(&server, validators.ValidationSchemaVersionAndSemantic)
1554+
err := result.FirstError()
1555+
require.Error(t, err, "Expected error for positional package argument without value or valueHint")
1556+
assert.Contains(t, err.Error(), "positional argument must provide a value or a valueHint")
1557+
})
1558+
}
1559+
14601560
// Helper function to create a valid server with a specific argument for testing
14611561
func TestValidate_TransportValidation(t *testing.T) {
14621562
tests := []struct {
@@ -2041,6 +2141,15 @@ func createValidServerWithArgument(arg model.Argument) apiv0.ServerJSON {
20412141
}
20422142
}
20432143

2144+
// createValidServerWithPackageArgument is createValidServerWithArgument with the
2145+
// argument under packageArguments instead of runtimeArguments
2146+
func createValidServerWithPackageArgument(arg model.Argument) apiv0.ServerJSON {
2147+
server := createValidServerWithArgument(arg)
2148+
server.Packages[0].RuntimeArguments = nil
2149+
server.Packages[0].PackageArguments = []model.Argument{arg}
2150+
return server
2151+
}
2152+
20442153
func TestValidateTitle(t *testing.T) {
20452154
tests := []struct {
20462155
name string

0 commit comments

Comments
 (0)