Skip to content

Fix SHA validation in create_or_update_file #2134

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
almaleksia merged 5 commits into from
Mar 4, 2026
+105 −128
Merged

Fix SHA validation in create_or_update_file #2134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1a50053
Fix SHA validation in create_or_update_file
almaleksia Mar 4, 2026
35a514c
Doc update
almaleksia Mar 4, 2026
a556214
Handle non-404 errors
almaleksia Mar 4, 2026
d90eb82
Handle directory paths
almaleksia Mar 4, 2026
3e0da68
Update instructions
almaleksia Mar 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
4 changes: 2 additions & 2 deletions pkg/github/__toolsnaps__/create_or_update_file.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down Expand Up @@ -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"
}
},
Expand Down
119 changes: 57 additions & 62 deletions pkg/github/repositories.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"fmt"
"io"
"net/http"
"net/url"
"strings"

ghErrors "github.com/github/github-mcp-server/pkg/errors"
Expand Down Expand Up @@ -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 <path to file>
git rev-parse <branch>:<path to file>

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"),
Expand Down Expand Up @@ -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"},
Expand Down Expand Up @@ -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)
Expand All @@ -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 != "" {
Copy link
Contributor Author

@almaleksia almaleksia Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing blind update completely

Copy link
Collaborator

@SamMorrowDrums SamMorrowDrums Mar 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fair to accept the errors, thought this is probably the most controversial change.

Copy link
Contributor Author

@almaleksia almaleksia Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided if GitHub API requires sha for updates who are we to make it optional :D

Copy link
Collaborator

@SamMorrowDrums SamMorrowDrums Mar 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I think it's fine to make agent fail, but I don't think it's wrong either way, just different trade-offs.

You still retain a git record of what you changed, so it might be surprising to update a file that has changed since you last checked, but you can at least still work out what happened.

The concern for me was always concurrent updates from different agents and then thinking independently that two things were done, but actually only one was because the other overwrites it. That's not great, and I think it's ok for us to not allow that by accident.

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
},
)
Expand Down
108 changes: 45 additions & 63 deletions pkg/github/repositories_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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==",
Expand Down Expand Up @@ -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",
Expand All @@ -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{
Expand All @@ -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==",
Expand All @@ -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",
Expand All @@ -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{
Expand All @@ -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==",
Expand Down