Challenge workflow file writes for OAuth scope

pkg/github/header_params_test.go

@@ -29,6 +29,10 @@ func TestAllToolsRoutingParamsGetHeaders(t *testing.T) {
 		if !ok || schema == nil {
 			continue
 		}
+		if pathSchema := schema.Properties["path"]; pathSchema != nil {
+			require.NotContainsf(t, pathSchema.Extra, "x-mcp-header",
+				"tool %q path must remain in MCP arguments", tool.Name)
+		}
 		for prop, header := range inventory.HeaderParams {
 			ps, ok := schema.Properties[prop]
 			if !ok || ps == nil {

pkg/github/repositories.go

@@ -404,7 +404,7 @@ func ListBranches(t translations.TranslationHelperFunc) inventory.ServerTool {
 
 // CreateOrUpdateFile creates a tool to create or update a file in a GitHub repository.
 func CreateOrUpdateFile(t translations.TranslationHelperFunc) inventory.ServerTool {
-	return NewTool(
+	tool := NewTool(
 		ToolsetMetadataRepos,
 		mcp.Tool{
 			Name: "create_or_update_file",
@@ -469,6 +469,10 @@ SHA MUST be provided for existing file updates.
 			if err != nil {
 				return utils.NewToolResultError(err.Error()), nil, nil
 			}
+			path, err = validateRelativePath(path)
+			if err != nil {
+				return utils.NewToolResultError(fmt.Sprintf("invalid path: %s", err)), nil, nil
+			}
 			content, err := RequiredParam[string](args, "content")
 			if err != nil {
 				return utils.NewToolResultError(err.Error()), nil, nil
@@ -507,8 +511,6 @@ SHA MUST be provided for existing file updates.
 				return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err)
 			}
 
-			path = strings.TrimPrefix(path, "/")
-
 			// SHA validation using Contents API to fetch current file metadata (blob SHA)
 			getOpts := &github.RepositoryContentGetOptions{Ref: branch}
 
@@ -596,6 +598,8 @@ SHA MUST be provided for existing file updates.
 			return MarshalledTextResult(minimalResponse), nil, nil
 		},
 	)
+	tool.ScopeResolver = workflowScopeForPath
+	return tool
 }
 
 // CreateRepository creates a tool to create a new GitHub repository.
@@ -1244,7 +1248,7 @@ func ForkRepository(t translations.TranslationHelperFunc) inventory.ServerTool {
 // The approach implemented here gets automatic commit signing when used with either the github-actions user or as an app,
 // both of which suit an LLM well.
 func DeleteFile(t translations.TranslationHelperFunc) inventory.ServerTool {
-	return NewTool(
+	tool := NewTool(
 		ToolsetMetadataRepos,
 		mcp.Tool{
 			Name:        "delete_file",
@@ -1295,6 +1299,10 @@ func DeleteFile(t translations.TranslationHelperFunc) inventory.ServerTool {
 			if err != nil {
 				return utils.NewToolResultError(err.Error()), nil, nil
 			}
+			path, err = validateRelativePath(path)
+			if err != nil {
+				return utils.NewToolResultError(fmt.Sprintf("invalid path: %s", err)), nil, nil
+			}
 			message, err := RequiredParam[string](args, "message")
 			if err != nil {
 				return utils.NewToolResultError(err.Error()), nil, nil
@@ -1425,6 +1433,8 @@ func DeleteFile(t translations.TranslationHelperFunc) inventory.ServerTool {
 			return utils.NewToolResultText(string(r)), nil, nil
 		},
 	)
+	tool.ScopeResolver = workflowScopeForPath
+	return tool
 }
 
 // CreateBranch creates a tool to create a new branch.
@@ -1542,7 +1552,7 @@ func CreateBranch(t translations.TranslationHelperFunc) inventory.ServerTool {
 
 // PushFiles creates a tool to push multiple files in a single commit to a GitHub repository.
 func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool {
-	return NewTool(
+	tool := NewTool(
 		ToolsetMetadataRepos,
 		mcp.Tool{
 			Name:        "push_files",
@@ -1618,6 +1628,35 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool {
 				return utils.NewToolResultError("files parameter must be an array of objects with path and content"), nil, nil
 			}
 
+			entries := make([]*github.TreeEntry, 0, len(filesObj))
+			for _, file := range filesObj {
+				fileMap, ok := file.(map[string]any)
+				if !ok {
+					return utils.NewToolResultError("each file must be an object with path and content"), nil, nil
+				}
+
+				filePath, ok := fileMap["path"].(string)
+				if !ok || filePath == "" {
+					return utils.NewToolResultError("each file must have a path"), nil, nil
+				}
+				filePath, err = validateRelativePath(filePath)
+				if err != nil {
+					return utils.NewToolResultError(fmt.Sprintf("invalid file path: %s", err)), nil, nil
+				}
+
+				content, ok := fileMap["content"].(string)
+				if !ok {
+					return utils.NewToolResultError("each file must have content"), nil, nil
+				}
+
+				entries = append(entries, &github.TreeEntry{
+					Path:    github.Ptr(filePath),
+					Mode:    github.Ptr("100644"),
+					Type:    github.Ptr("blob"),
+					Content: github.Ptr(content),
+				})
+			}
+
 			client, err := deps.GetClient(ctx)
 			if err != nil {
 				return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err)
@@ -1691,34 +1730,6 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool {
 				baseCommit = base
 			}
 
-			// Create tree entries for all files (or remaining files if empty repo)
-			var entries []*github.TreeEntry
-
-			for _, file := range filesObj {
-				fileMap, ok := file.(map[string]any)
-				if !ok {
-					return utils.NewToolResultError("each file must be an object with path and content"), nil, nil
-				}
-
-				path, ok := fileMap["path"].(string)
-				if !ok || path == "" {
-					return utils.NewToolResultError("each file must have a path"), nil, nil
-				}
-
-				content, ok := fileMap["content"].(string)
-				if !ok {
-					return utils.NewToolResultError("each file must have content"), nil, nil
-				}
-
-				// Create a tree entry for the file
-				entries = append(entries, &github.TreeEntry{
-					Path:    github.Ptr(path),
-					Mode:    github.Ptr("100644"), // Regular file mode
-					Type:    github.Ptr("blob"),
-					Content: github.Ptr(content),
-				})
-			}
-
 			// Create a new tree with the file entries (baseCommit is now guaranteed to exist)
 			newTree, resp, err := client.Git.CreateTree(ctx, owner, repo, *baseCommit.Tree.SHA, entries)
 			if err != nil {
@@ -1773,6 +1784,8 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool {
 			return utils.NewToolResultText(string(r)), nil, nil
 		},
 	)
+	tool.ScopeResolver = workflowScopeForFiles
+	return tool
 }
 
 // ListTags creates a tool to list tags in a GitHub repository.

pkg/github/repository_path.go

@@ -0,0 +1,66 @@
+package github
+
+import (
+	"fmt"
+	"path"
+	"slices"
+	"strings"
+
+	"github.com/github/github-mcp-server/pkg/scopes"
+)
+
+const workflowPathPrefix = ".github/workflows/"
+
+func validateRelativePath(value string) (string, error) {
+	if value == "" {
+		return "", fmt.Errorf("path must not be empty")
+	}
+	if path.IsAbs(value) {
+		return "", fmt.Errorf("path must be relative")
+	}
+	if strings.Contains(value, `\`) {
+		return "", fmt.Errorf("path must use forward slashes")
+	}
+	if slices.Contains(strings.Split(value, "/"), "..") {
+		return "", fmt.Errorf("path must not contain parent directory traversal")
+	}
+
+	cleaned := path.Clean(value)
+	if cleaned == "." {
+		return "", fmt.Errorf("path must identify a file")
+	}
+	return cleaned, nil
+}
+
+func isWorkflowPath(value string) bool {
+	return strings.HasPrefix(value, workflowPathPrefix) && len(value) > len(workflowPathPrefix)
+}
+
+func workflowScopeForPath(arguments map[string]any) []string {
+	value, ok := arguments["path"].(string)
+	if !ok {
+		return nil
+	}
+	cleaned, err := validateRelativePath(value)
+	if err != nil || !isWorkflowPath(cleaned) {
+		return nil
+	}
+	return []string{string(scopes.Workflow)}
+}
+
+func workflowScopeForFiles(arguments map[string]any) []string {
+	files, ok := arguments["files"].([]any)
+	if !ok {
+		return nil
+	}
+	for _, file := range files {
+		fileMap, ok := file.(map[string]any)
+		if !ok {
+			continue
+		}
+		if len(workflowScopeForPath(fileMap)) > 0 {
+			return []string{string(scopes.Workflow)}
+		}
+	}
+	return nil
+}

pkg/github/repository_path_test.go

@@ -0,0 +1,140 @@
+package github
+
+import (
+	"context"
+	"testing"
+
+	"github.com/github/github-mcp-server/pkg/inventory"
+	"github.com/github/github-mcp-server/pkg/translations"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestValidateRelativePath(t *testing.T) {
+	tests := []struct {
+		name    string
+		value   string
+		want    string
+		wantErr string
+	}{
+		{name: "file", value: "docs/readme.md", want: "docs/readme.md"},
+		{name: "normalizes dot segment", value: "./.github/workflows/ci.yml", want: ".github/workflows/ci.yml"},
+		{name: "normalizes duplicate separator", value: ".github//workflows/ci.yml", want: ".github/workflows/ci.yml"},
+		{name: "empty", value: "", wantErr: "must not be empty"},
+		{name: "current directory", value: ".", wantErr: "must identify a file"},
+		{name: "absolute", value: "/.github/workflows/ci.yml", wantErr: "must be relative"},
+		{name: "parent traversal", value: "docs/../.github/workflows/ci.yml", wantErr: "parent directory traversal"},
+		{name: "leading traversal", value: "../.github/workflows/ci.yml", wantErr: "parent directory traversal"},
+		{name: "backslash traversal", value: `docs\..\.github\workflows\ci.yml`, wantErr: "forward slashes"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := validateRelativePath(tt.value)
+			if tt.wantErr != "" {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), tt.wantErr)
+				return
+			}
+			require.NoError(t, err)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}
+
+func TestFileWriteWorkflowScopeResolvers(t *testing.T) {
+	tests := []struct {
+		name string
+		tool inventory.ServerTool
+		args map[string]any
+		want []string
+	}{
+		{
+			name: "create regular file",
+			tool: CreateOrUpdateFile(translations.NullTranslationHelper),
+			args: map[string]any{"path": "docs/readme.md"},
+		},
+		{
+			name: "create workflow",
+			tool: CreateOrUpdateFile(translations.NullTranslationHelper),
+			args: map[string]any{"path": ".github/workflows/ci.yml"},
+			want: []string{"workflow"},
+		},
+		{
+			name: "delete normalized workflow",
+			tool: DeleteFile(translations.NullTranslationHelper),
+			args: map[string]any{"path": "./.github/workflows/ci.yml"},
+			want: []string{"workflow"},
+		},
+		{
+			name: "reject traversal instead of resolving it",
+			tool: DeleteFile(translations.NullTranslationHelper),
+			args: map[string]any{"path": "docs/../.github/workflows/ci.yml"},
+		},
+		{
+			name: "push regular files",
+			tool: PushFiles(translations.NullTranslationHelper),
+			args: map[string]any{"files": []any{map[string]any{"path": "README.md"}}},
+		},
+		{
+			name: "push includes workflow",
+			tool: PushFiles(translations.NullTranslationHelper),
+			args: map[string]any{"files": []any{
+				map[string]any{"path": "README.md"},
+				map[string]any{"path": ".github/workflows/ci.yml"},
+			}},
+			want: []string{"workflow"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			require.NotNil(t, tt.tool.ScopeResolver)
+			assert.Equal(t, tt.want, tt.tool.ScopeResolver(tt.args))
+		})
+	}
+}
+
+func TestFileWriteToolsRejectUnsafePathsBeforeAPICalls(t *testing.T) {
+	tests := []struct {
+		name string
+		tool inventory.ServerTool
+		args map[string]any
+	}{
+		{
+			name: "create or update",
+			tool: CreateOrUpdateFile(translations.NullTranslationHelper),
+			args: map[string]any{
+				"owner": "owner", "repo": "repo", "path": "../workflow.yml",
+				"content": "content", "message": "message", "branch": "main",
+			},
+		},
+		{
+			name: "delete",
+			tool: DeleteFile(translations.NullTranslationHelper),
+			args: map[string]any{
+				"owner": "owner", "repo": "repo", "path": "/.github/workflows/ci.yml",
+				"message": "message", "branch": "main",
+			},
+		},
+		{
+			name: "push",
+			tool: PushFiles(translations.NullTranslationHelper),
+			args: map[string]any{
+				"owner": "owner", "repo": "repo", "branch": "main", "message": "message",
+				"files": []any{map[string]any{"path": `..\workflow.yml`, "content": "content"}},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			deps := BaseDeps{}
+			request := createMCPRequest(tt.args)
+			result, err := tt.tool.Handler(deps)(ContextWithDeps(context.Background(), deps), &request)
+			require.NoError(t, err)
+			require.True(t, result.IsError)
+			assert.Contains(t, getErrorResult(t, result).Text, "path")
+		})
+	}
+}