From 44e80dc9615759cddb728c945682b93a266cfb08 Mon Sep 17 00:00:00 2001 From: Rod Boev Date: Sat, 20 Jun 2026 22:36:34 -0400 Subject: [PATCH 1/2] fix(discussions): validate required params in read handlers (#2740) --- pkg/github/discussions.go | 48 ++++++----- pkg/github/discussions_test.go | 146 ++++++++++++--------------------- 2 files changed, 79 insertions(+), 115 deletions(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 68ed014b2b..0b539ab21d 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -11,7 +11,6 @@ import ( "github.com/github/github-mcp-server/pkg/scopes" "github.com/github/github-mcp-server/pkg/translations" "github.com/github/github-mcp-server/pkg/utils" - "github.com/go-viper/mapstructure/v2" "github.com/google/go-github/v87/github" "github.com/google/jsonschema-go/jsonschema" "github.com/modelcontextprotocol/go-sdk/mcp" @@ -313,15 +312,19 @@ func GetDiscussion(t translations.TranslationHelperFunc) inventory.ServerTool { }, []scopes.Scope{scopes.Repo}, func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { - // Decode params - var params struct { - Owner string - Repo string - DiscussionNumber int32 + owner, err := RequiredParam[string](args, "owner") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil } - if err := mapstructure.WeakDecode(args, ¶ms); err != nil { + repo, err := RequiredParam[string](args, "repo") + if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + discussionNumber, err := RequiredInt(args, "discussionNumber") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + client, err := deps.GetGQLClient(ctx) if err != nil { return utils.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil, nil @@ -345,9 +348,9 @@ func GetDiscussion(t translations.TranslationHelperFunc) inventory.ServerTool { } `graphql:"repository(owner: $owner, name: $repo)"` } vars := map[string]any{ - "owner": githubv4.String(params.Owner), - "repo": githubv4.String(params.Repo), - "discussionNumber": githubv4.Int(params.DiscussionNumber), + "owner": githubv4.String(owner), + "repo": githubv4.String(repo), + "discussionNumber": githubv4.Int(discussionNumber), // #nosec G115 - discussion numbers are always small positive integers } if err := client.Query(ctx, &q, vars); err != nil { return utils.NewToolResultError(err.Error()), nil, nil @@ -384,7 +387,7 @@ func GetDiscussion(t translations.TranslationHelperFunc) inventory.ServerTool { result := utils.NewToolResultText(string(out)) // Discussion content is user-authored (untrusted); confidentiality // follows repo visibility. - result = attachRepoVisibilityIFCLabelLazy(ctx, deps, params.Owner, params.Repo, result, ifc.LabelRepoUserContent) + result = attachRepoVisibilityIFCLabelLazy(ctx, deps, owner, repo, result, ifc.LabelRepoUserContent) return result, nil, nil }, ) @@ -425,13 +428,16 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve }, []scopes.Scope{scopes.Repo}, func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { - // Decode params - var params struct { - Owner string - Repo string - DiscussionNumber int32 + owner, err := RequiredParam[string](args, "owner") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + repo, err := RequiredParam[string](args, "repo") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil } - if err := mapstructure.WeakDecode(args, ¶ms); err != nil { + discussionNumber, err := RequiredInt(args, "discussionNumber") + if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } @@ -467,9 +473,9 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve } vars := map[string]any{ - "owner": githubv4.String(params.Owner), - "repo": githubv4.String(params.Repo), - "discussionNumber": githubv4.Int(params.DiscussionNumber), + "owner": githubv4.String(owner), + "repo": githubv4.String(repo), + "discussionNumber": githubv4.Int(discussionNumber), // #nosec G115 - discussion numbers are always small positive integers "first": githubv4.Int(*paginationParams.First), } if paginationParams.After != nil { @@ -592,7 +598,7 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve result := utils.NewToolResultText(string(out)) // Discussion comments are user-authored (untrusted); confidentiality // follows repo visibility. - result = attachRepoVisibilityIFCLabelLazy(ctx, deps, params.Owner, params.Repo, result, ifc.LabelRepoUserContent) + result = attachRepoVisibilityIFCLabelLazy(ctx, deps, owner, repo, result, ifc.LabelRepoUserContent) return result, nil, nil }, ) diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index 36fdb6c43a..3fc090016a 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -588,50 +588,38 @@ func Test_GetDiscussion(t *testing.T) { assert.Equal(t, "General", category["name"]) }) } -} - -func Test_GetDiscussionWithStringNumber(t *testing.T) { - // Test that WeakDecode handles string discussionNumber from MCP clients - toolDef := GetDiscussion(translations.NullTranslationHelper) - qGetDiscussion := "query($discussionNumber:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussion(number: $discussionNumber){number,title,body,createdAt,closed,isAnswered,answerChosenAt,url,category{name}}}}" - - vars := map[string]any{ - "owner": "owner", - "repo": "repo", - "discussionNumber": float64(1), + // Test missing required parameters + missingParamTests := []struct { + name string + params map[string]any + errParam string + }{ + { + name: "missing owner", + params: map[string]any{"repo": "repo", "discussionNumber": int32(1)}, + errParam: "owner", + }, + { + name: "missing discussionNumber", + params: map[string]any{"owner": "owner", "repo": "repo"}, + errParam: "discussionNumber", + }, } - matcher := githubv4mock.NewQueryMatcher(qGetDiscussion, vars, githubv4mock.DataResponse(map[string]any{ - "repository": map[string]any{"discussion": map[string]any{ - "number": 1, - "title": "Test Discussion Title", - "body": "This is a test discussion", - "url": "https://github.com/owner/repo/discussions/1", - "createdAt": "2025-04-25T12:00:00Z", - "closed": false, - "isAnswered": false, - "category": map[string]any{"name": "General"}, - }}, - })) - httpClient := githubv4mock.NewMockedHTTPClient(matcher) - gqlClient := githubv4.NewClient(httpClient) - deps := BaseDeps{GQLClient: gqlClient} - handler := toolDef.Handler(deps) - - // Send discussionNumber as a string instead of a number - reqParams := map[string]any{"owner": "owner", "repo": "repo", "discussionNumber": "1"} - req := createMCPRequest(reqParams) - res, err := handler(ContextWithDeps(context.Background(), deps), &req) - require.NoError(t, err) + for _, tc := range missingParamTests { + t.Run(tc.name, func(t *testing.T) { + deps := BaseDeps{GQLClient: githubv4.NewClient(&http.Client{})} + handler := toolDef.Handler(deps) - text := getTextResult(t, res).Text - require.False(t, res.IsError, "expected no error, got: %s", text) + req := createMCPRequest(tc.params) + res, _ := handler(ContextWithDeps(context.Background(), deps), &req) + text := getTextResult(t, res).Text - var out map[string]any - require.NoError(t, json.Unmarshal([]byte(text), &out)) - assert.Equal(t, float64(1), out["number"]) - assert.Equal(t, "Test Discussion Title", out["title"]) + require.True(t, res.IsError) + assert.Contains(t, text, tc.errParam) + }) + } } func Test_GetDiscussionComments(t *testing.T) { @@ -721,68 +709,38 @@ func Test_GetDiscussionComments(t *testing.T) { assert.Equal(t, "This is the first comment", response.Comments[0].Body) assert.Equal(t, "DC_id2", response.Comments[1].ID) assert.Equal(t, "This is the second comment", response.Comments[1].Body) -} - -func Test_GetDiscussionCommentsWithStringNumber(t *testing.T) { - // Test that WeakDecode handles string discussionNumber from MCP clients - toolDef := GetDiscussionComments(translations.NullTranslationHelper) - qGetComments := "query($after:String$discussionNumber:Int!$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussion(number: $discussionNumber){comments(first: $first, after: $after){nodes{id,body,isAnswer},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount}}}}" - - vars := map[string]any{ - "owner": "owner", - "repo": "repo", - "discussionNumber": float64(1), - "first": float64(30), - "after": (*string)(nil), - } - - mockResponse := githubv4mock.DataResponse(map[string]any{ - "repository": map[string]any{ - "discussion": map[string]any{ - "comments": map[string]any{ - "nodes": []map[string]any{ - {"id": "DC_id3", "body": "First comment"}, - }, - "pageInfo": map[string]any{ - "hasNextPage": false, - "hasPreviousPage": false, - "startCursor": "", - "endCursor": "", - }, - "totalCount": 1, - }, - }, + // Test missing required parameters + missingParamTests := []struct { + name string + params map[string]any + errParam string + }{ + { + name: "missing owner", + params: map[string]any{"repo": "repo", "discussionNumber": int32(1)}, + errParam: "owner", + }, + { + name: "missing discussionNumber", + params: map[string]any{"owner": "owner", "repo": "repo"}, + errParam: "discussionNumber", }, - }) - matcher := githubv4mock.NewQueryMatcher(qGetComments, vars, mockResponse) - httpClient := githubv4mock.NewMockedHTTPClient(matcher) - gqlClient := githubv4.NewClient(httpClient) - deps := BaseDeps{GQLClient: gqlClient} - handler := toolDef.Handler(deps) - - // Send discussionNumber as a string instead of a number - reqParams := map[string]any{ - "owner": "owner", - "repo": "repo", - "discussionNumber": "1", } - request := createMCPRequest(reqParams) - result, err := handler(ContextWithDeps(context.Background(), deps), &request) - require.NoError(t, err) + for _, tc := range missingParamTests { + t.Run(tc.name, func(t *testing.T) { + deps := BaseDeps{GQLClient: githubv4.NewClient(&http.Client{})} + handler := toolDef.Handler(deps) - textContent := getTextResult(t, result) - require.False(t, result.IsError, "expected no error, got: %s", textContent.Text) + req := createMCPRequest(tc.params) + res, _ := handler(ContextWithDeps(context.Background(), deps), &req) + text := getTextResult(t, res).Text - var out struct { - Comments []map[string]any `json:"comments"` - TotalCount int `json:"totalCount"` + require.True(t, res.IsError) + assert.Contains(t, text, tc.errParam) + }) } - require.NoError(t, json.Unmarshal([]byte(textContent.Text), &out)) - assert.Len(t, out.Comments, 1) - assert.Equal(t, "DC_id3", out.Comments[0]["id"]) - assert.Equal(t, "First comment", out.Comments[0]["body"]) } func Test_ListDiscussionCategories(t *testing.T) { From 19e60a0a3163725041e1c16df6d004c28171b72c Mon Sep 17 00:00:00 2001 From: Rod Boev Date: Sat, 20 Jun 2026 22:53:58 -0400 Subject: [PATCH 2/2] Add missing repo param test for discussion read handlers --- pkg/github/discussions_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index 3fc090016a..45b0899f25 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -600,6 +600,11 @@ func Test_GetDiscussion(t *testing.T) { params: map[string]any{"repo": "repo", "discussionNumber": int32(1)}, errParam: "owner", }, + { + name: "missing repo", + params: map[string]any{"owner": "owner", "discussionNumber": int32(1)}, + errParam: "repo", + }, { name: "missing discussionNumber", params: map[string]any{"owner": "owner", "repo": "repo"}, @@ -721,6 +726,11 @@ func Test_GetDiscussionComments(t *testing.T) { params: map[string]any{"repo": "repo", "discussionNumber": int32(1)}, errParam: "owner", }, + { + name: "missing repo", + params: map[string]any{"owner": "owner", "discussionNumber": int32(1)}, + errParam: "repo", + }, { name: "missing discussionNumber", params: map[string]any{"owner": "owner", "repo": "repo"},