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>

Author: zwick

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),

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)