From 3d74c4f011e9831dfcb177113c8d5f30b2afea0e Mon Sep 17 00:00:00 2001 From: Bryan Zwicker Date: Fri, 26 Jun 2026 10:52:38 -0400 Subject: [PATCH 1/2] Sanitize and lockdown-gate issue_read get_parent title GetIssueParent returned the parent issue title raw and ungated by lockdown mode, so an agent could read an unverified, possibly cross-repo parent title even with lockdown enabled. Always sanitize the parent title and, under lockdown mode, only return the parent when its author has push access to the parent repository, failing closed otherwise. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/github/issues.go | 46 +++++++++++++++++---- pkg/github/issues_test.go | 85 +++++++++++++++++++++++++++++---------- 2 files changed, 103 insertions(+), 28 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index ef97b0846..9bf2afdc6 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) @@ -903,15 +903,29 @@ func GetSubIssues(ctx context.Context, client *github.Client, deps ToolDependenc // 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) { +// +// The parent may live in a different repository, so its title is always +// sanitized before being returned. When lockdown mode is enabled the parent is +// only returned when its author has push access to the parent repository +// (mirroring GetIssue); otherwise it is omitted (fail closed). +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 +949,28 @@ 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") + } + // The lockdown safe-content check keys off the parent author's push + // access to the parent repository. Fail closed by omitting the parent + // if anything required for that 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..ba3878b82 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 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,21 +3694,7 @@ 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`, @@ -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) From f24b8699d1619e7398e437983bde35b3c0386f9d Mon Sep 17 00:00:00 2001 From: Bryan Zwicker Date: Fri, 26 Jun 2026 11:17:16 -0400 Subject: [PATCH 2/2] Prove sanitization in get_parent test; trim comments Embed a U+202E BiDi control char in the mocked parent title so the happy-path assertion fails if Sanitize is removed, and tighten the GetIssueParent comments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/github/issues.go | 18 ++++++++---------- pkg/github/issues_test.go | 4 ++-- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 9bf2afdc6..f800693ff 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -900,14 +900,13 @@ 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. +// 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 may live in a different repository, so its title is always -// sanitized before being returned. When lockdown mode is enabled the parent is -// only returned when its author has push access to the parent repository -// (mirroring GetIssue); otherwise it is omitted (fail closed). +// 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 { @@ -953,9 +952,8 @@ func GetIssueParent(ctx context.Context, client *githubv4.Client, deps ToolDepen if cache == nil { return nil, fmt.Errorf("lockdown cache is not configured") } - // The lockdown safe-content check keys off the parent author's push - // access to the parent repository. Fail closed by omitting the parent - // if anything required for that check is missing or unverifiable. + // 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 == "" { diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index ba3878b82..545e407ad 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -3665,7 +3665,7 @@ func Test_GetIssueParent(t *testing.T) { "issue": map[string]any{ "parent": map[string]any{ "number": githubv4.Int(42), - "title": githubv4.String("Parent issue"), + "title": githubv4.String("Parent\u202e issue"), "state": githubv4.String("OPEN"), "url": githubv4.String("https://github.com/owner/repo/issues/42"), "author": map[string]any{ @@ -3697,7 +3697,7 @@ func Test_GetIssueParent(t *testing.T) { parentResponse("author"), ), ), - expectedText: `"number":42`, + expectedText: `"title":"Parent issue"`, }, { name: "issue has no parent",