diff --git a/pkg/github/issues.go b/pkg/github/issues.go index ef97b0846..f800693ff 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -701,7 +701,7 @@ Options are: result, err := GetSubIssues(ctx, client, deps, owner, repo, issueNumber, pagination) return attachIFC(result), nil, err case "get_parent": - result, err := GetIssueParent(ctx, gqlClient, owner, repo, issueNumber) + result, err := GetIssueParent(ctx, gqlClient, deps, owner, repo, issueNumber) return attachIFC(result), nil, err case "get_labels": result, err := GetIssueLabels(ctx, gqlClient, owner, repo, issueNumber) @@ -900,18 +900,31 @@ func GetSubIssues(ctx context.Context, client *github.Client, deps ToolDependenc return utils.NewToolResultText(string(r)), nil } -// GetIssueParent returns the parent issue of the given issue, or a null parent -// when the issue is not a sub-issue of any other issue. It reads the GraphQL -// Issue.parent field, the upward counterpart to the downward get_sub_issues read. -func GetIssueParent(ctx context.Context, client *githubv4.Client, owner string, repo string, issueNumber int) (*mcp.CallToolResult, error) { +// GetIssueParent returns the parent issue of the given issue, or a null +// parent when the issue is not a sub-issue. It reads the GraphQL +// Issue.parent field, the upward counterpart to get_sub_issues. +// +// The parent title is always sanitized (it may be cross-repo). Under +// lockdown mode the parent is only returned when its author has push +// access to the parent repo (mirroring GetIssue); otherwise it is omitted. +func GetIssueParent(ctx context.Context, client *githubv4.Client, deps ToolDependencies, owner string, repo string, issueNumber int) (*mcp.CallToolResult, error) { + cache, err := deps.GetRepoAccessCache(ctx) + if err != nil { + return nil, fmt.Errorf("failed to get repo access cache: %w", err) + } + flags := deps.GetFlags(ctx) + var query struct { Repository struct { Issue struct { Parent *struct { - Number githubv4.Int - Title githubv4.String - State githubv4.String - URL githubv4.String + Number githubv4.Int + Title githubv4.String + State githubv4.String + URL githubv4.String + Author struct { + Login githubv4.String + } Repository struct { NameWithOwner githubv4.String } @@ -935,10 +948,27 @@ func GetIssueParent(ctx context.Context, client *githubv4.Client, owner string, return MarshalledTextResult(map[string]any{"parent": nil}), nil } + if flags.LockdownMode { + if cache == nil { + return nil, fmt.Errorf("lockdown cache is not configured") + } + // Fail closed: omit the parent if anything needed for the safe-content + // check is missing or unverifiable. + parentAuthorLogin := string(parent.Author.Login) + parentOwner, parentRepo, ok := strings.Cut(string(parent.Repository.NameWithOwner), "/") + if parentAuthorLogin == "" || !ok || parentOwner == "" || parentRepo == "" { + return MarshalledTextResult(map[string]any{"parent": nil}), nil + } + isSafeContent, err := cache.IsSafeContent(ctx, parentAuthorLogin, parentOwner, parentRepo) + if err != nil || !isSafeContent { + return MarshalledTextResult(map[string]any{"parent": nil}), nil + } + } + return MarshalledTextResult(map[string]any{ "parent": map[string]any{ "number": int(parent.Number), - "title": string(parent.Title), + "title": sanitize.Sanitize(string(parent.Title)), "state": string(parent.State), "url": string(parent.URL), "repository": string(parent.Repository.NameWithOwner), diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index e3dfe0fe1..545e407ad 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -3638,10 +3638,13 @@ func Test_GetIssueParent(t *testing.T) { Repository struct { Issue struct { Parent *struct { - Number githubv4.Int - Title githubv4.String - State githubv4.String - URL githubv4.String + Number githubv4.Int + Title githubv4.String + State githubv4.String + URL githubv4.String + Author struct { + Login githubv4.String + } Repository struct { NameWithOwner githubv4.String } @@ -3656,9 +3659,32 @@ func Test_GetIssueParent(t *testing.T) { "issueNumber": githubv4.Int(123), } + parentResponse := func(authorLogin string) githubv4mock.GQLResponse { + return githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "parent": map[string]any{ + "number": githubv4.Int(42), + "title": githubv4.String("Parent\u202e issue"), + "state": githubv4.String("OPEN"), + "url": githubv4.String("https://github.com/owner/repo/issues/42"), + "author": map[string]any{ + "login": githubv4.String(authorLogin), + }, + "repository": map[string]any{ + "nameWithOwner": githubv4.String("owner/repo"), + }, + }, + }, + }, + }) + } + tests := []struct { name string mockedClient *http.Client + lockdownEnabled bool + restOverrides map[string]string expectToolError bool expectedText string }{ @@ -3668,24 +3694,10 @@ func Test_GetIssueParent(t *testing.T) { githubv4mock.NewQueryMatcher( parentMatcherStruct, vars, - githubv4mock.DataResponse(map[string]any{ - "repository": map[string]any{ - "issue": map[string]any{ - "parent": map[string]any{ - "number": githubv4.Int(42), - "title": githubv4.String("Parent issue"), - "state": githubv4.String("OPEN"), - "url": githubv4.String("https://github.com/owner/repo/issues/42"), - "repository": map[string]any{ - "nameWithOwner": githubv4.String("owner/repo"), - }, - }, - }, - }, - }), + parentResponse("author"), ), ), - expectedText: `"number":42`, + expectedText: `"title":"Parent issue"`, }, { name: "issue has no parent", @@ -3715,17 +3727,48 @@ func Test_GetIssueParent(t *testing.T) { ), expectToolError: true, }, + { + name: "lockdown enabled - parent author has push access", + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + parentMatcherStruct, + vars, + parentResponse("maintainer"), + ), + ), + lockdownEnabled: true, + restOverrides: map[string]string{"maintainer": "write"}, + expectedText: `"title":"Parent issue"`, + }, + { + name: "lockdown enabled - parent author lacks push access", + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + parentMatcherStruct, + vars, + parentResponse("externaluser"), + ), + ), + lockdownEnabled: true, + restOverrides: map[string]string{"externaluser": "read"}, + expectedText: `"parent":null`, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { gqlClient := githubv4.NewClient(tc.mockedClient) client := mustNewGHClient(t, nil) + + var restClient *github.Client + if tc.lockdownEnabled { + restClient = mockRESTPermissionServer(t, "read", tc.restOverrides) + } deps := BaseDeps{ Client: client, GQLClient: gqlClient, - RepoAccessCache: stubRepoAccessCache(nil, 15*time.Minute), - Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}), + RepoAccessCache: stubRepoAccessCache(restClient, 15*time.Minute), + Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": tc.lockdownEnabled}), } handler := serverTool.Handler(deps)