Skip to content

Commit 496496e

Browse files
committed
Consider workflow scope or user-escalation pattern for merging PRs that modify workflow files
1 parent 31b541e commit 496496e

File tree

3 files changed

+95
-4
lines changed

3 files changed

+95
-4
lines changed

pkg/github/pullrequests.go

Lines changed: 48 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"fmt"
77
"io"
88
"net/http"
9+
"strings"
910

1011
"github.com/go-viper/mapstructure/v2"
1112
"github.com/google/go-github/v79/github"
@@ -1045,6 +1046,27 @@ func ListPullRequests(t translations.TranslationHelperFunc) inventory.ServerTool
10451046
})
10461047
}
10471048

1049+
// workflowProtectedPaths contains the directory prefixes that require the workflow scope
1050+
var workflowProtectedPaths = []string{
1051+
".github/workflows/",
1052+
".github/workflows-lab/",
1053+
}
1054+
1055+
// containsWorkflowFiles checks if any of the given commit files are in workflow-protected directories
1056+
func containsWorkflowFiles(files []*github.CommitFile) bool {
1057+
for _, file := range files {
1058+
if file == nil || file.Filename == nil {
1059+
continue
1060+
}
1061+
for _, protectedPath := range workflowProtectedPaths {
1062+
if strings.HasPrefix(*file.Filename, protectedPath) {
1063+
return true
1064+
}
1065+
}
1066+
}
1067+
return false
1068+
}
1069+
10481070
// MergePullRequest creates a tool to merge a pull request.
10491071
func MergePullRequest(t translations.TranslationHelperFunc) inventory.ServerTool {
10501072
schema := &jsonschema.Schema{
@@ -1118,15 +1140,37 @@ func MergePullRequest(t translations.TranslationHelperFunc) inventory.ServerTool
11181140
return utils.NewToolResultError(err.Error()), nil, nil
11191141
}
11201142

1143+
client, err := deps.GetClient(ctx)
1144+
if err != nil {
1145+
return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil
1146+
}
1147+
1148+
// Pre-flight check: detect workflow files that require the 'workflow' scope
1149+
files, resp, err := client.PullRequests.ListFiles(ctx, owner, repo, pullNumber, &github.ListOptions{PerPage: 100})
1150+
if err != nil {
1151+
return ghErrors.NewGitHubAPIErrorResponse(ctx,
1152+
"failed to list pull request files for workflow scope check",
1153+
resp,
1154+
err,
1155+
), nil, nil
1156+
}
1157+
if resp != nil && resp.Body != nil {
1158+
_ = resp.Body.Close()
1159+
}
1160+
1161+
if containsWorkflowFiles(files) {
1162+
return utils.NewToolResultError(
1163+
"This pull request modifies GitHub Actions workflow files (.github/workflows/). " +
1164+
"Merging requires the 'workflow' OAuth scope, which is not included by default. " +
1165+
"Please use a Personal Access Token with the 'workflow' scope, or merge manually via the GitHub UI.",
1166+
), nil, nil
1167+
}
1168+
11211169
options := &github.PullRequestOptions{
11221170
CommitTitle: commitTitle,
11231171
MergeMethod: mergeMethod,
11241172
}
11251173

1126-
client, err := deps.GetClient(ctx)
1127-
if err != nil {
1128-
return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil
1129-
}
11301174
result, resp, err := client.PullRequests.Merge(ctx, owner, repo, pullNumber, commitMessage, options)
11311175
if err != nil {
11321176
return ghErrors.NewGitHubAPIErrorResponse(ctx,

pkg/github/pullrequests_test.go

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -709,6 +709,18 @@ func Test_MergePullRequest(t *testing.T) {
709709
SHA: github.Ptr("abcd1234efgh5678"),
710710
}
711711

712+
// Setup mock files - no workflow files
713+
mockNonWorkflowFiles := []*github.CommitFile{
714+
{Filename: github.Ptr("src/main.go")},
715+
{Filename: github.Ptr("README.md")},
716+
}
717+
718+
// Setup mock files - with workflow files
719+
mockWorkflowFiles := []*github.CommitFile{
720+
{Filename: github.Ptr("src/main.go")},
721+
{Filename: github.Ptr(".github/workflows/ci.yml")},
722+
}
723+
712724
tests := []struct {
713725
name string
714726
mockedClient *http.Client
@@ -720,6 +732,7 @@ func Test_MergePullRequest(t *testing.T) {
720732
{
721733
name: "successful merge",
722734
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
735+
GetReposPullsFilesByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockNonWorkflowFiles),
723736
PutReposPullsMergeByOwnerByRepoByPullNumber: expectRequestBody(t, map[string]interface{}{
724737
"commit_title": "Merge PR #42",
725738
"commit_message": "Merging awesome feature",
@@ -742,6 +755,7 @@ func Test_MergePullRequest(t *testing.T) {
742755
{
743756
name: "merge fails",
744757
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
758+
GetReposPullsFilesByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockNonWorkflowFiles),
745759
PutReposPullsMergeByOwnerByRepoByPullNumber: func(w http.ResponseWriter, _ *http.Request) {
746760
w.WriteHeader(http.StatusMethodNotAllowed)
747761
_, _ = w.Write([]byte(`{"message": "Pull request cannot be merged"}`))
@@ -755,6 +769,36 @@ func Test_MergePullRequest(t *testing.T) {
755769
expectError: true,
756770
expectedErrMsg: "failed to merge pull request",
757771
},
772+
{
773+
name: "merge blocked by workflow files",
774+
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
775+
GetReposPullsFilesByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockWorkflowFiles),
776+
}),
777+
requestArgs: map[string]interface{}{
778+
"owner": "owner",
779+
"repo": "repo",
780+
"pullNumber": float64(42),
781+
},
782+
expectError: true,
783+
expectedErrMsg: "workflow",
784+
},
785+
{
786+
name: "merge with .github but not workflows directory proceeds",
787+
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
788+
GetReposPullsFilesByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, []*github.CommitFile{
789+
{Filename: github.Ptr(".github/CODEOWNERS")},
790+
{Filename: github.Ptr(".github/pull_request_template.md")},
791+
}),
792+
PutReposPullsMergeByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockMergeResult),
793+
}),
794+
requestArgs: map[string]interface{}{
795+
"owner": "owner",
796+
"repo": "repo",
797+
"pullNumber": float64(42),
798+
},
799+
expectError: false,
800+
expectedMergeResult: mockMergeResult,
801+
},
758802
}
759803

760804
for _, tc := range tests {

pkg/scopes/scopes.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,9 @@ const (
5555

5656
// WritePackages grants write access to packages
5757
WritePackages Scope = "write:packages"
58+
59+
// Workflow grants read and write access to GitHub Actions workflow files
60+
Workflow Scope = "workflow"
5861
)
5962

6063
// ScopeHierarchy defines parent-child relationships between scopes.

0 commit comments

Comments
 (0)