Skip to content

Commit ccb9b53

Browse files
almaleksiaalmaleksia
authored
Fix SHA validation in create_or_update_file (#2134)
* Fix SHA validation in create_or_update_file * Doc update * Handle non-404 errors * Handle directory paths * Update instructions
1 parent b50a343 commit ccb9b53

File tree

4 files changed

+105
-128
lines changed

4 files changed

+105
-128
lines changed

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1171,7 +1171,7 @@ The following sets of tools are available:
11711171
- `owner`: Repository owner (username or organization) (string, required)
11721172
- `path`: Path where to create/update the file (string, required)
11731173
- `repo`: Repository name (string, required)
1174-
- `sha`: The blob SHA of the file being replaced. (string, optional)
1174+
- `sha`: The blob SHA of the file being replaced. Required if the file already exists. (string, optional)
11751175

11761176
- **create_repository** - Create repository
11771177
- **Required OAuth Scopes**: `repo`

‎pkg/github/__toolsnaps__/create_or_update_file.snap‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
"annotations": {
33
"title": "Create or update file"
44
},
5-
"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",
5+
"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",
66
"inputSchema": {
77
"properties": {
88
"branch": {
@@ -30,7 +30,7 @@
3030
"type": "string"
3131
},
3232
"sha": {
33-
"description": "The blob SHA of the file being replaced.",
33+
"description": "The blob SHA of the file being replaced. Required if the file already exists.",
3434
"type": "string"
3535
}
3636
},

‎pkg/github/repositories.go‎

Lines changed: 57 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"fmt"
88
"io"
99
"net/http"
10-
"net/url"
1110
"strings"
1211

1312
ghErrors "github.com/github/github-mcp-server/pkg/errors"
@@ -323,9 +322,9 @@ func CreateOrUpdateFile(t translations.TranslationHelperFunc) inventory.ServerTo
323322
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.
324323
325324
In order to obtain the SHA of original file version before updating, use the following git command:
326-
git ls-tree HEAD <path to file>
325+
git rev-parse <branch>:<path to file>
327326
328-
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.
327+
SHA MUST be provided for existing file updates.
329328
`),
330329
Annotations: &mcp.ToolAnnotations{
331330
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
360359
},
361360
"sha": {
362361
Type: "string",
363-
Description: "The blob SHA of the file being replaced.",
362+
Description: "The blob SHA of the file being replaced. Required if the file already exists.",
364363
},
365364
},
366365
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
420419

421420
path = strings.TrimPrefix(path, "/")
422421

423-
// SHA validation using conditional HEAD request (efficient - no body transfer)
424-
var previousSHA string
425-
contentURL := fmt.Sprintf("repos/%s/%s/contents/%s", owner, repo, url.PathEscape(path))
426-
if branch != "" {
427-
contentURL += "?ref=" + url.QueryEscape(branch)
428-
}
422+
// SHA validation using Contents API to fetch current file metadata (blob SHA)
423+
getOpts := &github.RepositoryContentGetOptions{Ref: branch}
429424

430425
if sha != "" {
431426
// User provided SHA - validate it's still current
432-
req, err := client.NewRequest("HEAD", contentURL, nil)
433-
if err == nil {
434-
req.Header.Set("If-None-Match", fmt.Sprintf(`"%s"`, sha))
435-
resp, _ := client.Do(ctx, req, nil)
436-
if resp != nil {
437-
defer resp.Body.Close()
438-
439-
switch resp.StatusCode {
440-
case http.StatusNotModified:
441-
// SHA matches current - proceed
442-
opts.SHA = github.Ptr(sha)
443-
case http.StatusOK:
444-
// SHA is stale - reject with current SHA so user can check diff
445-
currentSHA := strings.Trim(resp.Header.Get("ETag"), `"`)
446-
return utils.NewToolResultError(fmt.Sprintf(
447-
"SHA mismatch: provided SHA %s is stale. Current file SHA is %s. "+
448-
"Use get_file_contents or compare commits to review changes before updating.",
449-
sha, currentSHA)), nil, nil
450-
case http.StatusNotFound:
451-
// File doesn't exist - this is a create, ignore provided SHA
452-
}
427+
existingFile, dirContent, respCheck, getErr := client.Repositories.GetContents(ctx, owner, repo, path, getOpts)
428+
if respCheck != nil {
429+
_ = respCheck.Body.Close()
430+
}
431+
switch {
432+
case getErr != nil:
433+
// 404 means file doesn't exist - proceed (new file creation)
434+
// Any other error (403, 500, network) should be surfaced
435+
if respCheck == nil || respCheck.StatusCode != http.StatusNotFound {
436+
return ghErrors.NewGitHubAPIErrorResponse(ctx,
437+
"failed to verify file SHA",
438+
respCheck,
439+
getErr,
440+
), nil, nil
441+
}
442+
case dirContent != nil:
443+
return utils.NewToolResultError(fmt.Sprintf(
444+
"Path %s is a directory, not a file. This tool only works with files.",
445+
path)), nil, nil
446+
case existingFile != nil:
447+
currentSHA := existingFile.GetSHA()
448+
if currentSHA != sha {
449+
return utils.NewToolResultError(fmt.Sprintf(
450+
"SHA mismatch: provided SHA %s is stale. Current file SHA is %s. "+
451+
"Pull the latest changes and use git rev-parse %s:%s to get the current SHA.",
452+
sha, currentSHA, branch, path)), nil, nil
453453
}
454454
}
455455
} else {
456-
// No SHA provided - check if file exists to warn about blind update
457-
req, err := client.NewRequest("HEAD", contentURL, nil)
458-
if err == nil {
459-
resp, _ := client.Do(ctx, req, nil)
460-
if resp != nil {
461-
defer resp.Body.Close()
462-
if resp.StatusCode == http.StatusOK {
463-
previousSHA = strings.Trim(resp.Header.Get("ETag"), `"`)
464-
}
465-
// 404 = new file, no previous SHA needed
456+
// No SHA provided - check if file already exists
457+
existingFile, dirContent, respCheck, getErr := client.Repositories.GetContents(ctx, owner, repo, path, getOpts)
458+
if respCheck != nil {
459+
_ = respCheck.Body.Close()
460+
}
461+
switch {
462+
case getErr != nil:
463+
// 404 means file doesn't exist - proceed with creation
464+
// Any other error (403, 500, network) should be surfaced
465+
if respCheck == nil || respCheck.StatusCode != http.StatusNotFound {
466+
return ghErrors.NewGitHubAPIErrorResponse(ctx,
467+
"failed to check if file exists",
468+
respCheck,
469+
getErr,
470+
), nil, nil
466471
}
472+
case dirContent != nil:
473+
return utils.NewToolResultError(fmt.Sprintf(
474+
"Path %s is a directory, not a file. This tool only works with files.",
475+
path)), nil, nil
476+
case existingFile != nil:
477+
// File exists but no SHA was provided - reject to prevent blind overwrites
478+
return utils.NewToolResultError(fmt.Sprintf(
479+
"File already exists at %s. You must provide the current file's SHA when updating. "+
480+
"Use git rev-parse %s:%s to get the blob SHA, then retry with the sha parameter.",
481+
path, branch, path)), nil, nil
467482
}
468-
}
469-
470-
if previousSHA != "" {
471-
opts.SHA = github.Ptr(previousSHA)
483+
// If file not found, no previous SHA needed (new file creation)
472484
}
473485

474486
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
491503

492504
minimalResponse := convertToMinimalFileContentResponse(fileContent)
493505

494-
// Warn if file was updated without SHA validation (blind update)
495-
if sha == "" && previousSHA != "" {
496-
warning, err := json.Marshal(minimalResponse)
497-
if err != nil {
498-
return nil, nil, fmt.Errorf("failed to marshal response: %w", err)
499-
}
500-
return utils.NewToolResultText(fmt.Sprintf(
501-
"Warning: File updated without SHA validation. Previous file SHA was %s. "+
502-
`Verify no unintended changes were overwritten:
503-
1. Extract the SHA of the local version using git ls-tree HEAD %s.
504-
2. Compare with the previous SHA above.
505-
3. Revert changes if shas do not match.
506-
507-
%s`,
508-
previousSHA, path, string(warning))), nil, nil
509-
}
510-
511506
return MarshalledTextResult(minimalResponse), nil, nil
512507
},
513508
)

‎pkg/github/repositories_test.go‎

Lines changed: 45 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -1157,6 +1157,14 @@ func Test_CreateOrUpdateFile(t *testing.T) {
11571157
{
11581158
name: "successful file update with SHA",
11591159
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
1160+
"GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{
1161+
SHA: github.Ptr("abc123def456"),
1162+
Type: github.Ptr("file"),
1163+
}),
1164+
"GET /repos/{owner}/{repo}/contents/{path:.*}": mockResponse(t, http.StatusOK, &github.RepositoryContent{
1165+
SHA: github.Ptr("abc123def456"),
1166+
Type: github.Ptr("file"),
1167+
}),
11601168
PutReposContentsByOwnerByRepoByPath: expectRequestBody(t, map[string]any{
11611169
"message": "Update example file",
11621170
"content": "IyBVcGRhdGVkIEV4YW1wbGUKClRoaXMgZmlsZSBoYXMgYmVlbiB1cGRhdGVkLg==", // Base64 encoded content
@@ -1210,26 +1218,16 @@ func Test_CreateOrUpdateFile(t *testing.T) {
12101218
expectedErrMsg: "failed to create/update file",
12111219
},
12121220
{
1213-
name: "sha validation - current sha matches (304 Not Modified)",
1221+
name: "sha validation - current sha matches",
12141222
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
1215-
"HEAD /repos/owner/repo/contents/docs/example.md": func(w http.ResponseWriter, req *http.Request) {
1216-
ifNoneMatch := req.Header.Get("If-None-Match")
1217-
if ifNoneMatch == `"abc123def456"` {
1218-
w.WriteHeader(http.StatusNotModified)
1219-
} else {
1220-
w.WriteHeader(http.StatusOK)
1221-
w.Header().Set("ETag", `"abc123def456"`)
1222-
}
1223-
},
1224-
"HEAD /repos/{owner}/{repo}/contents/{path:.*}": func(w http.ResponseWriter, req *http.Request) {
1225-
ifNoneMatch := req.Header.Get("If-None-Match")
1226-
if ifNoneMatch == `"abc123def456"` {
1227-
w.WriteHeader(http.StatusNotModified)
1228-
} else {
1229-
w.WriteHeader(http.StatusOK)
1230-
w.Header().Set("ETag", `"abc123def456"`)
1231-
}
1232-
},
1223+
"GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{
1224+
SHA: github.Ptr("abc123def456"),
1225+
Type: github.Ptr("file"),
1226+
}),
1227+
"GET /repos/{owner}/{repo}/contents/{path:.*}": mockResponse(t, http.StatusOK, &github.RepositoryContent{
1228+
SHA: github.Ptr("abc123def456"),
1229+
Type: github.Ptr("file"),
1230+
}),
12331231
PutReposContentsByOwnerByRepoByPath: expectRequestBody(t, map[string]any{
12341232
"message": "Update example file",
12351233
"content": "IyBVcGRhdGVkIEV4YW1wbGUKClRoaXMgZmlsZSBoYXMgYmVlbiB1cGRhdGVkLg==",
@@ -1260,16 +1258,16 @@ func Test_CreateOrUpdateFile(t *testing.T) {
12601258
expectedContent: mockFileResponse,
12611259
},
12621260
{
1263-
name: "sha validation - stale sha detected (200 OK with different ETag)",
1261+
name: "sha validation - stale sha detected",
12641262
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
1265-
"HEAD /repos/owner/repo/contents/docs/example.md": func(w http.ResponseWriter, _ *http.Request) {
1266-
w.Header().Set("ETag", `"newsha999888"`)
1267-
w.WriteHeader(http.StatusOK)
1268-
},
1269-
"HEAD /repos/{owner}/{repo}/contents/{path:.*}": func(w http.ResponseWriter, _ *http.Request) {
1270-
w.Header().Set("ETag", `"newsha999888"`)
1271-
w.WriteHeader(http.StatusOK)
1272-
},
1263+
"GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{
1264+
SHA: github.Ptr("newsha999888"),
1265+
Type: github.Ptr("file"),
1266+
}),
1267+
"GET /repos/{owner}/{repo}/contents/{path:.*}": mockResponse(t, http.StatusOK, &github.RepositoryContent{
1268+
SHA: github.Ptr("newsha999888"),
1269+
Type: github.Ptr("file"),
1270+
}),
12731271
}),
12741272
requestArgs: map[string]any{
12751273
"owner": "owner",
@@ -1286,7 +1284,10 @@ func Test_CreateOrUpdateFile(t *testing.T) {
12861284
{
12871285
name: "sha validation - file doesn't exist (404), proceed with create",
12881286
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
1289-
"HEAD /repos/owner/repo/contents/docs/example.md": func(w http.ResponseWriter, _ *http.Request) {
1287+
"GET /repos/owner/repo/contents/docs/example.md": func(w http.ResponseWriter, _ *http.Request) {
1288+
w.WriteHeader(http.StatusNotFound)
1289+
},
1290+
"GET /repos/{owner}/{repo}/contents/{path:.*}": func(w http.ResponseWriter, _ *http.Request) {
12901291
w.WriteHeader(http.StatusNotFound)
12911292
},
12921293
PutReposContentsByOwnerByRepoByPath: expectRequestBody(t, map[string]any{
@@ -1297,9 +1298,6 @@ func Test_CreateOrUpdateFile(t *testing.T) {
12971298
}).andThen(
12981299
mockResponse(t, http.StatusCreated, mockFileResponse),
12991300
),
1300-
"HEAD /repos/{owner}/{repo}/contents/{path:.*}": func(w http.ResponseWriter, _ *http.Request) {
1301-
w.WriteHeader(http.StatusNotFound)
1302-
},
13031301
"PUT /repos/{owner}/{repo}/contents/{path:.*}": expectRequestBody(t, map[string]any{
13041302
"message": "Create new file",
13051303
"content": "IyBOZXcgRmlsZQoKVGhpcyBpcyBhIG5ldyBmaWxlLg==",
@@ -1322,32 +1320,16 @@ func Test_CreateOrUpdateFile(t *testing.T) {
13221320
expectedContent: mockFileResponse,
13231321
},
13241322
{
1325-
name: "no sha provided - file exists, returns warning",
1323+
name: "no sha provided - file exists, rejects update",
13261324
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
1327-
"HEAD /repos/owner/repo/contents/docs/example.md": func(w http.ResponseWriter, _ *http.Request) {
1328-
w.Header().Set("ETag", `"existing123"`)
1329-
w.WriteHeader(http.StatusOK)
1330-
},
1331-
PutReposContentsByOwnerByRepoByPath: expectRequestBody(t, map[string]any{
1332-
"message": "Update without SHA",
1333-
"content": "IyBVcGRhdGVkCgpVcGRhdGVkIHdpdGhvdXQgU0hBLg==",
1334-
"branch": "main",
1335-
"sha": "existing123", // SHA is automatically added from ETag
1336-
}).andThen(
1337-
mockResponse(t, http.StatusOK, mockFileResponse),
1338-
),
1339-
"HEAD /repos/{owner}/{repo}/contents/{path:.*}": func(w http.ResponseWriter, _ *http.Request) {
1340-
w.Header().Set("ETag", `"existing123"`)
1341-
w.WriteHeader(http.StatusOK)
1342-
},
1343-
"PUT /repos/{owner}/{repo}/contents/{path:.*}": expectRequestBody(t, map[string]any{
1344-
"message": "Update without SHA",
1345-
"content": "IyBVcGRhdGVkCgpVcGRhdGVkIHdpdGhvdXQgU0hBLg==",
1346-
"branch": "main",
1347-
"sha": "existing123", // SHA is automatically added from ETag
1348-
}).andThen(
1349-
mockResponse(t, http.StatusOK, mockFileResponse),
1350-
),
1325+
"GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{
1326+
SHA: github.Ptr("existing123"),
1327+
Type: github.Ptr("file"),
1328+
}),
1329+
"GET /repos/{owner}/{repo}/contents/{path:.*}": mockResponse(t, http.StatusOK, &github.RepositoryContent{
1330+
SHA: github.Ptr("existing123"),
1331+
Type: github.Ptr("file"),
1332+
}),
13511333
}),
13521334
requestArgs: map[string]any{
13531335
"owner": "owner",
@@ -1357,13 +1339,16 @@ func Test_CreateOrUpdateFile(t *testing.T) {
13571339
"message": "Update without SHA",
13581340
"branch": "main",
13591341
},
1360-
expectError: false,
1361-
expectedErrMsg: "Warning: File updated without SHA validation. Previous file SHA was existing123",
1342+
expectError: true,
1343+
expectedErrMsg: "File already exists at docs/example.md",
13621344
},
13631345
{
13641346
name: "no sha provided - file doesn't exist, no warning",
13651347
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
1366-
"HEAD /repos/owner/repo/contents/docs/example.md": func(w http.ResponseWriter, _ *http.Request) {
1348+
"GET /repos/owner/repo/contents/docs/example.md": func(w http.ResponseWriter, _ *http.Request) {
1349+
w.WriteHeader(http.StatusNotFound)
1350+
},
1351+
"GET /repos/{owner}/{repo}/contents/{path:.*}": func(w http.ResponseWriter, _ *http.Request) {
13671352
w.WriteHeader(http.StatusNotFound)
13681353
},
13691354
PutReposContentsByOwnerByRepoByPath: expectRequestBody(t, map[string]any{
@@ -1373,9 +1358,6 @@ func Test_CreateOrUpdateFile(t *testing.T) {
13731358
}).andThen(
13741359
mockResponse(t, http.StatusCreated, mockFileResponse),
13751360
),
1376-
"HEAD /repos/{owner}/{repo}/contents/{path:.*}": func(w http.ResponseWriter, _ *http.Request) {
1377-
w.WriteHeader(http.StatusNotFound)
1378-
},
13791361
"PUT /repos/{owner}/{repo}/contents/{path:.*}": expectRequestBody(t, map[string]any{
13801362
"message": "Create new file",
13811363
"content": "IyBOZXcgRmlsZQoKQ3JlYXRlZCB3aXRob3V0IFNIQQ==",

0 commit comments

Comments
 (0)