Skip to content

Commit 1a50053

Browse files
almaleksiaalmaleksia
committed
Fix SHA validation in create_or_update_file
1 parent b50a343 commit 1a50053

File tree

3 files changed

+76
-128
lines changed

3 files changed

+76
-128
lines changed

‎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 HEAD:\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: 29 additions & 63 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 HEAD:<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,39 @@ 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, _, respCheck, getErr := client.Repositories.GetContents(ctx, owner, repo, path, getOpts)
428+
if respCheck != nil {
429+
_ = respCheck.Body.Close()
430+
}
431+
if getErr == nil && existingFile != nil {
432+
currentSHA := existingFile.GetSHA()
433+
if currentSHA != sha {
434+
return utils.NewToolResultError(fmt.Sprintf(
435+
"SHA mismatch: provided SHA %s is stale. Current file SHA is %s. "+
436+
"Pull the latest changes and use git rev-parse HEAD:%s to get the current SHA.",
437+
sha, currentSHA, path)), nil, nil
453438
}
454439
}
440+
// If file not found or error, proceed (could be a new file creation)
455441
} 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
466-
}
442+
// No SHA provided - check if file already exists
443+
existingFile, _, respCheck, getErr := client.Repositories.GetContents(ctx, owner, repo, path, getOpts)
444+
if respCheck != nil {
445+
_ = respCheck.Body.Close()
467446
}
468-
}
469-
470-
if previousSHA != "" {
471-
opts.SHA = github.Ptr(previousSHA)
447+
if getErr == nil && existingFile != nil {
448+
// File exists but no SHA was provided - reject to prevent blind overwrites
449+
return utils.NewToolResultError(fmt.Sprintf(
450+
"File already exists at %s. You must provide the current file's SHA when updating. "+
451+
"Use git rev-parse HEAD:%s to get the blob SHA, then retry with the sha parameter.",
452+
path, path)), nil, nil
453+
}
454+
// If file not found or error, no previous SHA needed (new file creation)
472455
}
473456

474457
fileContent, resp, err := client.Repositories.CreateFile(ctx, owner, repo, path, opts)
@@ -491,23 +474,6 @@ If the SHA is not provided, the tool will attempt to acquire it by fetching the
491474

492475
minimalResponse := convertToMinimalFileContentResponse(fileContent)
493476

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-
511477
return MarshalledTextResult(minimalResponse), nil, nil
512478
},
513479
)

‎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)