Skip to content

Commit 82771e4

Browse files
IrynaKulakovaIrynaKulakova
authored
Merge branch 'main' into fix/2714-sanitize-issue-comment-body
2 parents 844776f + 6592847 commit 82771e4

2 files changed

Lines changed: 105 additions & 32 deletions

File tree

‎pkg/github/issues.go‎

Lines changed: 40 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -701,7 +701,7 @@ Options are:
701701
result, err := GetSubIssues(ctx, client, deps, owner, repo, issueNumber, pagination)
702702
return attachIFC(result), nil, err
703703
case "get_parent":
704-
result, err := GetIssueParent(ctx, gqlClient, owner, repo, issueNumber)
704+
result, err := GetIssueParent(ctx, gqlClient, deps, owner, repo, issueNumber)
705705
return attachIFC(result), nil, err
706706
case "get_labels":
707707
result, err := GetIssueLabels(ctx, gqlClient, owner, repo, issueNumber)
@@ -900,18 +900,31 @@ func GetSubIssues(ctx context.Context, client *github.Client, deps ToolDependenc
900900
return utils.NewToolResultText(string(r)), nil
901901
}
902902

903-
// GetIssueParent returns the parent issue of the given issue, or a null parent
904-
// when the issue is not a sub-issue of any other issue. It reads the GraphQL
905-
// Issue.parent field, the upward counterpart to the downward get_sub_issues read.
906-
func GetIssueParent(ctx context.Context, client *githubv4.Client, owner string, repo string, issueNumber int) (*mcp.CallToolResult, error) {
903+
// GetIssueParent returns the parent issue of the given issue, or a null
904+
// parent when the issue is not a sub-issue. It reads the GraphQL
905+
// Issue.parent field, the upward counterpart to get_sub_issues.
906+
//
907+
// The parent title is always sanitized (it may be cross-repo). Under
908+
// lockdown mode the parent is only returned when its author has push
909+
// access to the parent repo (mirroring GetIssue); otherwise it is omitted.
910+
func GetIssueParent(ctx context.Context, client *githubv4.Client, deps ToolDependencies, owner string, repo string, issueNumber int) (*mcp.CallToolResult, error) {
911+
cache, err := deps.GetRepoAccessCache(ctx)
912+
if err != nil {
913+
return nil, fmt.Errorf("failed to get repo access cache: %w", err)
914+
}
915+
flags := deps.GetFlags(ctx)
916+
907917
var query struct {
908918
Repository struct {
909919
Issue struct {
910920
Parent *struct {
911-
Number githubv4.Int
912-
Title githubv4.String
913-
State githubv4.String
914-
URL githubv4.String
921+
Number githubv4.Int
922+
Title githubv4.String
923+
State githubv4.String
924+
URL githubv4.String
925+
Author struct {
926+
Login githubv4.String
927+
}
915928
Repository struct {
916929
NameWithOwner githubv4.String
917930
}
@@ -935,10 +948,27 @@ func GetIssueParent(ctx context.Context, client *githubv4.Client, owner string,
935948
return MarshalledTextResult(map[string]any{"parent": nil}), nil
936949
}
937950

951+
if flags.LockdownMode {
952+
if cache == nil {
953+
return nil, fmt.Errorf("lockdown cache is not configured")
954+
}
955+
// Fail closed: omit the parent if anything needed for the safe-content
956+
// check is missing or unverifiable.
957+
parentAuthorLogin := string(parent.Author.Login)
958+
parentOwner, parentRepo, ok := strings.Cut(string(parent.Repository.NameWithOwner), "/")
959+
if parentAuthorLogin == "" || !ok || parentOwner == "" || parentRepo == "" {
960+
return MarshalledTextResult(map[string]any{"parent": nil}), nil
961+
}
962+
isSafeContent, err := cache.IsSafeContent(ctx, parentAuthorLogin, parentOwner, parentRepo)
963+
if err != nil || !isSafeContent {
964+
return MarshalledTextResult(map[string]any{"parent": nil}), nil
965+
}
966+
}
967+
938968
return MarshalledTextResult(map[string]any{
939969
"parent": map[string]any{
940970
"number": int(parent.Number),
941-
"title": string(parent.Title),
971+
"title": sanitize.Sanitize(string(parent.Title)),
942972
"state": string(parent.State),
943973
"url": string(parent.URL),
944974
"repository": string(parent.Repository.NameWithOwner),

‎pkg/github/issues_test.go‎

Lines changed: 65 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -3529,10 +3529,13 @@ func Test_GetIssueParent(t *testing.T) {
35293529
Repository struct {
35303530
Issue struct {
35313531
Parent *struct {
3532-
Number githubv4.Int
3533-
Title githubv4.String
3534-
State githubv4.String
3535-
URL githubv4.String
3532+
Number githubv4.Int
3533+
Title githubv4.String
3534+
State githubv4.String
3535+
URL githubv4.String
3536+
Author struct {
3537+
Login githubv4.String
3538+
}
35363539
Repository struct {
35373540
NameWithOwner githubv4.String
35383541
}
@@ -3547,9 +3550,32 @@ func Test_GetIssueParent(t *testing.T) {
35473550
"issueNumber": githubv4.Int(123),
35483551
}
35493552

3553+
parentResponse := func(authorLogin string) githubv4mock.GQLResponse {
3554+
return githubv4mock.DataResponse(map[string]any{
3555+
"repository": map[string]any{
3556+
"issue": map[string]any{
3557+
"parent": map[string]any{
3558+
"number": githubv4.Int(42),
3559+
"title": githubv4.String("Parent\u202e issue"),
3560+
"state": githubv4.String("OPEN"),
3561+
"url": githubv4.String("https://github.com/owner/repo/issues/42"),
3562+
"author": map[string]any{
3563+
"login": githubv4.String(authorLogin),
3564+
},
3565+
"repository": map[string]any{
3566+
"nameWithOwner": githubv4.String("owner/repo"),
3567+
},
3568+
},
3569+
},
3570+
},
3571+
})
3572+
}
3573+
35503574
tests := []struct {
35513575
name string
35523576
mockedClient *http.Client
3577+
lockdownEnabled bool
3578+
restOverrides map[string]string
35533579
expectToolError bool
35543580
expectedText string
35553581
}{
@@ -3559,24 +3585,10 @@ func Test_GetIssueParent(t *testing.T) {
35593585
githubv4mock.NewQueryMatcher(
35603586
parentMatcherStruct,
35613587
vars,
3562-
githubv4mock.DataResponse(map[string]any{
3563-
"repository": map[string]any{
3564-
"issue": map[string]any{
3565-
"parent": map[string]any{
3566-
"number": githubv4.Int(42),
3567-
"title": githubv4.String("Parent issue"),
3568-
"state": githubv4.String("OPEN"),
3569-
"url": githubv4.String("https://github.com/owner/repo/issues/42"),
3570-
"repository": map[string]any{
3571-
"nameWithOwner": githubv4.String("owner/repo"),
3572-
},
3573-
},
3574-
},
3575-
},
3576-
}),
3588+
parentResponse("author"),
35773589
),
35783590
),
3579-
expectedText: `"number":42`,
3591+
expectedText: `"title":"Parent issue"`,
35803592
},
35813593
{
35823594
name: "issue has no parent",
@@ -3606,17 +3618,48 @@ func Test_GetIssueParent(t *testing.T) {
36063618
),
36073619
expectToolError: true,
36083620
},
3621+
{
3622+
name: "lockdown enabled - parent author has push access",
3623+
mockedClient: githubv4mock.NewMockedHTTPClient(
3624+
githubv4mock.NewQueryMatcher(
3625+
parentMatcherStruct,
3626+
vars,
3627+
parentResponse("maintainer"),
3628+
),
3629+
),
3630+
lockdownEnabled: true,
3631+
restOverrides: map[string]string{"maintainer": "write"},
3632+
expectedText: `"title":"Parent issue"`,
3633+
},
3634+
{
3635+
name: "lockdown enabled - parent author lacks push access",
3636+
mockedClient: githubv4mock.NewMockedHTTPClient(
3637+
githubv4mock.NewQueryMatcher(
3638+
parentMatcherStruct,
3639+
vars,
3640+
parentResponse("externaluser"),
3641+
),
3642+
),
3643+
lockdownEnabled: true,
3644+
restOverrides: map[string]string{"externaluser": "read"},
3645+
expectedText: `"parent":null`,
3646+
},
36093647
}
36103648

36113649
for _, tc := range tests {
36123650
t.Run(tc.name, func(t *testing.T) {
36133651
gqlClient := githubv4.NewClient(tc.mockedClient)
36143652
client := mustNewGHClient(t, nil)
3653+
3654+
var restClient *github.Client
3655+
if tc.lockdownEnabled {
3656+
restClient = mockRESTPermissionServer(t, "read", tc.restOverrides)
3657+
}
36153658
deps := BaseDeps{
36163659
Client: client,
36173660
GQLClient: gqlClient,
3618-
RepoAccessCache: stubRepoAccessCache(nil, 15*time.Minute),
3619-
Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}),
3661+
RepoAccessCache: stubRepoAccessCache(restClient, 15*time.Minute),
3662+
Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": tc.lockdownEnabled}),
36203663
}
36213664
handler := serverTool.Handler(deps)
36223665

0 commit comments

Comments
 (0)