diff --git a/README.md b/README.md index f83989a1e..1b926b132 100644 --- a/README.md +++ b/README.md @@ -1171,7 +1171,7 @@ The following sets of tools are available: - `owner`: Repository owner (username or organization) (string, required) - `path`: Path where to create/update the file (string, required) - `repo`: Repository name (string, required) - - `sha`: The blob SHA of the file being replaced. (string, optional) + - `sha`: The blob SHA of the file being replaced. Required if the file already exists. (string, optional) - **create_repository** - Create repository - **Required OAuth Scopes**: `repo` diff --git a/pkg/github/__toolsnaps__/create_or_update_file.snap b/pkg/github/__toolsnaps__/create_or_update_file.snap index 9d28c8085..e6900c905 100644 --- a/pkg/github/__toolsnaps__/create_or_update_file.snap +++ b/pkg/github/__toolsnaps__/create_or_update_file.snap @@ -2,7 +2,7 @@ "annotations": { "title": "Create or update file" }, - "description": "Create or update a single file in a GitHub repository. \nIf updating, you should provide the SHA of the file you want to update. Use this tool to create or update a file in a GitHub repository remotely; do not use it for local file operations.\n\nIn order to obtain the SHA of original file version before updating, use the following git command:\ngit ls-tree HEAD \u003cpath to file\u003e\n\nIf the SHA is not provided, the tool will attempt to acquire it by fetching the current file contents from the repository, which may lead to rewriting latest committed changes if the file has changed since last retrieval.\n", + "description": "Create or update a single file in a GitHub repository. \nIf updating, you should provide the SHA of the file you want to update. Use this tool to create or update a file in a GitHub repository remotely; do not use it for local file operations.\n\nIn order to obtain the SHA of original file version before updating, use the following git command:\ngit rev-parse \u003cbranch\u003e:\u003cpath to file\u003e\n\nSHA MUST be provided for existing file updates.\n", "inputSchema": { "properties": { "branch": { @@ -30,7 +30,7 @@ "type": "string" }, "sha": { - "description": "The blob SHA of the file being replaced.", + "description": "The blob SHA of the file being replaced. Required if the file already exists.", "type": "string" } }, diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index a236609bc..6eab707f9 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -7,7 +7,6 @@ import ( "fmt" "io" "net/http" - "net/url" "strings" ghErrors "github.com/github/github-mcp-server/pkg/errors" @@ -323,9 +322,9 @@ func CreateOrUpdateFile(t translations.TranslationHelperFunc) inventory.ServerTo If updating, you should provide the SHA of the file you want to update. Use this tool to create or update a file in a GitHub repository remotely; do not use it for local file operations. In order to obtain the SHA of original file version before updating, use the following git command: -git ls-tree HEAD +git rev-parse : -If the SHA is not provided, the tool will attempt to acquire it by fetching the current file contents from the repository, which may lead to rewriting latest committed changes if the file has changed since last retrieval. +SHA MUST be provided for existing file updates. `), Annotations: &mcp.ToolAnnotations{ Title: t("TOOL_CREATE_OR_UPDATE_FILE_USER_TITLE", "Create or update file"), @@ -360,7 +359,7 @@ If the SHA is not provided, the tool will attempt to acquire it by fetching the }, "sha": { Type: "string", - Description: "The blob SHA of the file being replaced.", + Description: "The blob SHA of the file being replaced. Required if the file already exists.", }, }, Required: []string{"owner", "repo", "path", "content", "message", "branch"}, @@ -420,55 +419,68 @@ If the SHA is not provided, the tool will attempt to acquire it by fetching the path = strings.TrimPrefix(path, "/") - // SHA validation using conditional HEAD request (efficient - no body transfer) - var previousSHA string - contentURL := fmt.Sprintf("repos/%s/%s/contents/%s", owner, repo, url.PathEscape(path)) - if branch != "" { - contentURL += "?ref=" + url.QueryEscape(branch) - } + // SHA validation using Contents API to fetch current file metadata (blob SHA) + getOpts := &github.RepositoryContentGetOptions{Ref: branch} if sha != "" { // User provided SHA - validate it's still current - req, err := client.NewRequest("HEAD", contentURL, nil) - if err == nil { - req.Header.Set("If-None-Match", fmt.Sprintf(`"%s"`, sha)) - resp, _ := client.Do(ctx, req, nil) - if resp != nil { - defer resp.Body.Close() - - switch resp.StatusCode { - case http.StatusNotModified: - // SHA matches current - proceed - opts.SHA = github.Ptr(sha) - case http.StatusOK: - // SHA is stale - reject with current SHA so user can check diff - currentSHA := strings.Trim(resp.Header.Get("ETag"), `"`) - return utils.NewToolResultError(fmt.Sprintf( - "SHA mismatch: provided SHA %s is stale. Current file SHA is %s. "+ - "Use get_file_contents or compare commits to review changes before updating.", - sha, currentSHA)), nil, nil - case http.StatusNotFound: - // File doesn't exist - this is a create, ignore provided SHA - } + existingFile, dirContent, respCheck, getErr := client.Repositories.GetContents(ctx, owner, repo, path, getOpts) + if respCheck != nil { + _ = respCheck.Body.Close() + } + switch { + case getErr != nil: + // 404 means file doesn't exist - proceed (new file creation) + // Any other error (403, 500, network) should be surfaced + if respCheck == nil || respCheck.StatusCode != http.StatusNotFound { + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to verify file SHA", + respCheck, + getErr, + ), nil, nil + } + case dirContent != nil: + return utils.NewToolResultError(fmt.Sprintf( + "Path %s is a directory, not a file. This tool only works with files.", + path)), nil, nil + case existingFile != nil: + currentSHA := existingFile.GetSHA() + if currentSHA != sha { + return utils.NewToolResultError(fmt.Sprintf( + "SHA mismatch: provided SHA %s is stale. Current file SHA is %s. "+ + "Pull the latest changes and use git rev-parse %s:%s to get the current SHA.", + sha, currentSHA, branch, path)), nil, nil } } } else { - // No SHA provided - check if file exists to warn about blind update - req, err := client.NewRequest("HEAD", contentURL, nil) - if err == nil { - resp, _ := client.Do(ctx, req, nil) - if resp != nil { - defer resp.Body.Close() - if resp.StatusCode == http.StatusOK { - previousSHA = strings.Trim(resp.Header.Get("ETag"), `"`) - } - // 404 = new file, no previous SHA needed + // No SHA provided - check if file already exists + existingFile, dirContent, respCheck, getErr := client.Repositories.GetContents(ctx, owner, repo, path, getOpts) + if respCheck != nil { + _ = respCheck.Body.Close() + } + switch { + case getErr != nil: + // 404 means file doesn't exist - proceed with creation + // Any other error (403, 500, network) should be surfaced + if respCheck == nil || respCheck.StatusCode != http.StatusNotFound { + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to check if file exists", + respCheck, + getErr, + ), nil, nil } + case dirContent != nil: + return utils.NewToolResultError(fmt.Sprintf( + "Path %s is a directory, not a file. This tool only works with files.", + path)), nil, nil + case existingFile != nil: + // File exists but no SHA was provided - reject to prevent blind overwrites + return utils.NewToolResultError(fmt.Sprintf( + "File already exists at %s. You must provide the current file's SHA when updating. "+ + "Use git rev-parse %s:%s to get the blob SHA, then retry with the sha parameter.", + path, branch, path)), nil, nil } - } - - if previousSHA != "" { - opts.SHA = github.Ptr(previousSHA) + // If file not found, no previous SHA needed (new file creation) } fileContent, resp, err := client.Repositories.CreateFile(ctx, owner, repo, path, opts) @@ -491,23 +503,6 @@ If the SHA is not provided, the tool will attempt to acquire it by fetching the minimalResponse := convertToMinimalFileContentResponse(fileContent) - // Warn if file was updated without SHA validation (blind update) - if sha == "" && previousSHA != "" { - warning, err := json.Marshal(minimalResponse) - if err != nil { - return nil, nil, fmt.Errorf("failed to marshal response: %w", err) - } - return utils.NewToolResultText(fmt.Sprintf( - "Warning: File updated without SHA validation. Previous file SHA was %s. "+ - `Verify no unintended changes were overwritten: -1. Extract the SHA of the local version using git ls-tree HEAD %s. -2. Compare with the previous SHA above. -3. Revert changes if shas do not match. - -%s`, - previousSHA, path, string(warning))), nil, nil - } - return MarshalledTextResult(minimalResponse), nil, nil }, ) diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index fdb5780c2..a27eef5e1 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -1157,6 +1157,14 @@ func Test_CreateOrUpdateFile(t *testing.T) { { name: "successful file update with SHA", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + "GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("abc123def456"), + Type: github.Ptr("file"), + }), + "GET /repos/{owner}/{repo}/contents/{path:.*}": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("abc123def456"), + Type: github.Ptr("file"), + }), PutReposContentsByOwnerByRepoByPath: expectRequestBody(t, map[string]any{ "message": "Update example file", "content": "IyBVcGRhdGVkIEV4YW1wbGUKClRoaXMgZmlsZSBoYXMgYmVlbiB1cGRhdGVkLg==", // Base64 encoded content @@ -1210,26 +1218,16 @@ func Test_CreateOrUpdateFile(t *testing.T) { expectedErrMsg: "failed to create/update file", }, { - name: "sha validation - current sha matches (304 Not Modified)", + name: "sha validation - current sha matches", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - "HEAD /repos/owner/repo/contents/docs/example.md": func(w http.ResponseWriter, req *http.Request) { - ifNoneMatch := req.Header.Get("If-None-Match") - if ifNoneMatch == `"abc123def456"` { - w.WriteHeader(http.StatusNotModified) - } else { - w.WriteHeader(http.StatusOK) - w.Header().Set("ETag", `"abc123def456"`) - } - }, - "HEAD /repos/{owner}/{repo}/contents/{path:.*}": func(w http.ResponseWriter, req *http.Request) { - ifNoneMatch := req.Header.Get("If-None-Match") - if ifNoneMatch == `"abc123def456"` { - w.WriteHeader(http.StatusNotModified) - } else { - w.WriteHeader(http.StatusOK) - w.Header().Set("ETag", `"abc123def456"`) - } - }, + "GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("abc123def456"), + Type: github.Ptr("file"), + }), + "GET /repos/{owner}/{repo}/contents/{path:.*}": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("abc123def456"), + Type: github.Ptr("file"), + }), PutReposContentsByOwnerByRepoByPath: expectRequestBody(t, map[string]any{ "message": "Update example file", "content": "IyBVcGRhdGVkIEV4YW1wbGUKClRoaXMgZmlsZSBoYXMgYmVlbiB1cGRhdGVkLg==", @@ -1260,16 +1258,16 @@ func Test_CreateOrUpdateFile(t *testing.T) { expectedContent: mockFileResponse, }, { - name: "sha validation - stale sha detected (200 OK with different ETag)", + name: "sha validation - stale sha detected", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - "HEAD /repos/owner/repo/contents/docs/example.md": func(w http.ResponseWriter, _ *http.Request) { - w.Header().Set("ETag", `"newsha999888"`) - w.WriteHeader(http.StatusOK) - }, - "HEAD /repos/{owner}/{repo}/contents/{path:.*}": func(w http.ResponseWriter, _ *http.Request) { - w.Header().Set("ETag", `"newsha999888"`) - w.WriteHeader(http.StatusOK) - }, + "GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("newsha999888"), + Type: github.Ptr("file"), + }), + "GET /repos/{owner}/{repo}/contents/{path:.*}": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("newsha999888"), + Type: github.Ptr("file"), + }), }), requestArgs: map[string]any{ "owner": "owner", @@ -1286,7 +1284,10 @@ func Test_CreateOrUpdateFile(t *testing.T) { { name: "sha validation - file doesn't exist (404), proceed with create", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - "HEAD /repos/owner/repo/contents/docs/example.md": func(w http.ResponseWriter, _ *http.Request) { + "GET /repos/owner/repo/contents/docs/example.md": func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + }, + "GET /repos/{owner}/{repo}/contents/{path:.*}": func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusNotFound) }, PutReposContentsByOwnerByRepoByPath: expectRequestBody(t, map[string]any{ @@ -1297,9 +1298,6 @@ func Test_CreateOrUpdateFile(t *testing.T) { }).andThen( mockResponse(t, http.StatusCreated, mockFileResponse), ), - "HEAD /repos/{owner}/{repo}/contents/{path:.*}": func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusNotFound) - }, "PUT /repos/{owner}/{repo}/contents/{path:.*}": expectRequestBody(t, map[string]any{ "message": "Create new file", "content": "IyBOZXcgRmlsZQoKVGhpcyBpcyBhIG5ldyBmaWxlLg==", @@ -1322,32 +1320,16 @@ func Test_CreateOrUpdateFile(t *testing.T) { expectedContent: mockFileResponse, }, { - name: "no sha provided - file exists, returns warning", + name: "no sha provided - file exists, rejects update", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - "HEAD /repos/owner/repo/contents/docs/example.md": func(w http.ResponseWriter, _ *http.Request) { - w.Header().Set("ETag", `"existing123"`) - w.WriteHeader(http.StatusOK) - }, - PutReposContentsByOwnerByRepoByPath: expectRequestBody(t, map[string]any{ - "message": "Update without SHA", - "content": "IyBVcGRhdGVkCgpVcGRhdGVkIHdpdGhvdXQgU0hBLg==", - "branch": "main", - "sha": "existing123", // SHA is automatically added from ETag - }).andThen( - mockResponse(t, http.StatusOK, mockFileResponse), - ), - "HEAD /repos/{owner}/{repo}/contents/{path:.*}": func(w http.ResponseWriter, _ *http.Request) { - w.Header().Set("ETag", `"existing123"`) - w.WriteHeader(http.StatusOK) - }, - "PUT /repos/{owner}/{repo}/contents/{path:.*}": expectRequestBody(t, map[string]any{ - "message": "Update without SHA", - "content": "IyBVcGRhdGVkCgpVcGRhdGVkIHdpdGhvdXQgU0hBLg==", - "branch": "main", - "sha": "existing123", // SHA is automatically added from ETag - }).andThen( - mockResponse(t, http.StatusOK, mockFileResponse), - ), + "GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("existing123"), + Type: github.Ptr("file"), + }), + "GET /repos/{owner}/{repo}/contents/{path:.*}": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("existing123"), + Type: github.Ptr("file"), + }), }), requestArgs: map[string]any{ "owner": "owner", @@ -1357,13 +1339,16 @@ func Test_CreateOrUpdateFile(t *testing.T) { "message": "Update without SHA", "branch": "main", }, - expectError: false, - expectedErrMsg: "Warning: File updated without SHA validation. Previous file SHA was existing123", + expectError: true, + expectedErrMsg: "File already exists at docs/example.md", }, { name: "no sha provided - file doesn't exist, no warning", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - "HEAD /repos/owner/repo/contents/docs/example.md": func(w http.ResponseWriter, _ *http.Request) { + "GET /repos/owner/repo/contents/docs/example.md": func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + }, + "GET /repos/{owner}/{repo}/contents/{path:.*}": func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusNotFound) }, PutReposContentsByOwnerByRepoByPath: expectRequestBody(t, map[string]any{ @@ -1373,9 +1358,6 @@ func Test_CreateOrUpdateFile(t *testing.T) { }).andThen( mockResponse(t, http.StatusCreated, mockFileResponse), ), - "HEAD /repos/{owner}/{repo}/contents/{path:.*}": func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusNotFound) - }, "PUT /repos/{owner}/{repo}/contents/{path:.*}": expectRequestBody(t, map[string]any{ "message": "Create new file", "content": "IyBOZXcgRmlsZQoKQ3JlYXRlZCB3aXRob3V0IFNIQQ==",