From c9b31b01abb61413d2127c74164397611e78573a Mon Sep 17 00:00:00 2001 From: Xiaoping Liao <106010272+kyletser@users.noreply.github.com> Date: Sun, 20 Sep 2026 19:35:11 +0800 Subject: [PATCH] fix: support binary file content via base64 encoding in create_or_update_file and push_files JSON tool arguments must be valid UTF-8, so binary file content (e.g. images) could not be written through create_or_update_file or push_files: the content string was passed through verbatim, corrupting non-UTF-8 bytes. Add an optional encoding parameter (utf-8, the default, or base64) to both tools. For base64 content, create_or_update_file decodes it once before handing the raw bytes to the Contents API. push_files uploads decoded content as a git blob and references it by SHA in the tree, because the Trees API rejects TreeEntry content that is not valid UTF-8. Invalid base64 and unsupported encoding values return clear tool errors. Closes #3312 --- .../__toolsnaps__/create_or_update_file.snap | 11 +- pkg/github/__toolsnaps__/push_files.snap | 13 +- pkg/github/helper_test.go | 1 + pkg/github/repositories.go | 90 ++++++-- pkg/github/repositories_test.go | 204 ++++++++++++++++++ 5 files changed, 301 insertions(+), 18 deletions(-) diff --git a/pkg/github/__toolsnaps__/create_or_update_file.snap b/pkg/github/__toolsnaps__/create_or_update_file.snap index faf468567d..7c25f8c01f 100644 --- a/pkg/github/__toolsnaps__/create_or_update_file.snap +++ b/pkg/github/__toolsnaps__/create_or_update_file.snap @@ -17,7 +17,16 @@ "type": "string" }, "content": { - "description": "Content of the file, exactly as it should appear once written. Do not base64-encode it; this server does that before calling the REST API.", + "description": "Content of the file, exactly as it should appear once written. Do not base64-encode it; this server does that before calling the REST API. If the content is binary (for example an image), pass it base64-encoded together with encoding \"base64\".", + "type": "string" + }, + "encoding": { + "default": "utf-8", + "description": "Encoding of the content parameter. \"utf-8\" (default) writes the content as-is. \"base64\" decodes the content once and writes the resulting raw bytes; use it for binary files such as images, whose bytes cannot be represented in a JSON string.", + "enum": [ + "utf-8", + "base64" + ], "type": "string" }, "message": { diff --git a/pkg/github/__toolsnaps__/push_files.snap b/pkg/github/__toolsnaps__/push_files.snap index 798ad18451..1116aab235 100644 --- a/pkg/github/__toolsnaps__/push_files.snap +++ b/pkg/github/__toolsnaps__/push_files.snap @@ -12,12 +12,21 @@ "type": "string" }, "files": { - "description": "Array of file objects to push, each object with path (string) and content (string)", + "description": "Array of file objects to push, each with path (string), content (string), and optional encoding (\"utf-8\" or \"base64\"; defaults to \"utf-8\")", "items": { "additionalProperties": false, "properties": { "content": { - "description": "file content", + "description": "file content; when encoding is \"base64\", this is the base64-encoded content of the file", + "type": "string" + }, + "encoding": { + "default": "utf-8", + "description": "Encoding of the file content. \"utf-8\" (default) writes the content as-is. \"base64\" decodes the content once and writes the resulting raw bytes; use it for binary files such as images, whose bytes cannot be represented in a JSON string.", + "enum": [ + "utf-8", + "base64" + ], "type": "string" }, "path": { diff --git a/pkg/github/helper_test.go b/pkg/github/helper_test.go index 0d089c8430..e059b3ef76 100644 --- a/pkg/github/helper_test.go +++ b/pkg/github/helper_test.go @@ -46,6 +46,7 @@ const ( // Git endpoints GetReposGitBlobsByOwnerByRepoByFileSHA = "GET /repos/{owner}/{repo}/git/blobs/{file_sha}" + PostReposGitBlobsByOwnerByRepo = "POST /repos/{owner}/{repo}/git/blobs" GetReposGitTreesByOwnerByRepoByTree = "GET /repos/{owner}/{repo}/git/trees/{tree}" GetReposGitRefByOwnerByRepoByRef = "GET /repos/{owner}/{repo}/git/ref/{ref:.*}" PostReposGitRefsByOwnerByRepo = "POST /repos/{owner}/{repo}/git/refs" diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 8e2dd25172..dedabb101e 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -437,7 +437,13 @@ SHA MUST be provided for existing file updates. }, "content": { Type: "string", - Description: "Content of the file, exactly as it should appear once written. Do not base64-encode it; this server does that before calling the REST API.", + Description: "Content of the file, exactly as it should appear once written. Do not base64-encode it; this server does that before calling the REST API. If the content is binary (for example an image), pass it base64-encoded together with encoding \"base64\".", + }, + "encoding": { + Type: "string", + Description: "Encoding of the content parameter. \"utf-8\" (default) writes the content as-is. \"base64\" decodes the content once and writes the resulting raw bytes; use it for binary files such as images, whose bytes cannot be represented in a JSON string.", + Enum: []any{"utf-8", "base64"}, + Default: json.RawMessage(`"utf-8"`), }, "message": { Type: "string", @@ -492,7 +498,22 @@ SHA MUST be provided for existing file updates. } // json.Marshal encodes byte arrays with base64, which is required for the API. - contentBytes := []byte(content) + encoding, err := OptionalParam[string](args, "encoding") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + var contentBytes []byte + switch encoding { + case "", "utf-8": + contentBytes = []byte(content) + case "base64": + contentBytes, err = base64.StdEncoding.DecodeString(content) + if err != nil { + return utils.NewToolResultError(fmt.Sprintf("content parameter is not valid base64: %v", err)), nil, nil + } + default: + return utils.NewToolResultError(fmt.Sprintf("invalid encoding %q: must be \"utf-8\" or \"base64\"", encoding)), nil, nil + } // Create the file options opts := &github.RepositoryContentFileOptions{ @@ -1636,7 +1657,7 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { }, "files": { Type: "array", - Description: "Array of file objects to push, each object with path (string) and content (string)", + Description: "Array of file objects to push, each with path (string), content (string), and optional encoding (\"utf-8\" or \"base64\"; defaults to \"utf-8\")", Items: &jsonschema.Schema{ Type: "object", AdditionalProperties: &jsonschema.Schema{Not: &jsonschema.Schema{}}, @@ -1647,7 +1668,13 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { }, "content": { Type: "string", - Description: "file content", + Description: "file content; when encoding is \"base64\", this is the base64-encoded content of the file", + }, + "encoding": { + Type: "string", + Description: "Encoding of the file content. \"utf-8\" (default) writes the content as-is. \"base64\" decodes the content once and writes the resulting raw bytes; use it for binary files such as images, whose bytes cannot be represented in a JSON string.", + Enum: []any{"utf-8", "base64"}, + Default: json.RawMessage(`"utf-8"`), }, }, Required: []string{"path", "content"}, @@ -1680,6 +1707,11 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { return utils.NewToolResultError(err.Error()), nil, nil } + client, err := deps.GetClient(ctx) + if err != nil { + return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) + } + // Parse files parameter - this should be an array of objects with path and content filesObj, ok := args["files"].([]any) if !ok { @@ -1707,17 +1739,45 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { 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) + encoding, _ := fileMap["encoding"].(string) + switch encoding { + case "", "utf-8": + entries = append(entries, &github.TreeEntry{ + Path: github.Ptr(filePath), + Mode: github.Ptr("100644"), + Type: github.Ptr("blob"), + Content: github.Ptr(content), + }) + case "base64": + // The Trees API rejects TreeEntry content that is not valid UTF-8, + // so binary files are uploaded as blobs and referenced by SHA. + decoded, err := base64.StdEncoding.DecodeString(content) + if err != nil { + return utils.NewToolResultError(fmt.Sprintf("file %s content is not valid base64: %v", filePath, err)), nil, nil + } + blob, resp, err := client.Git.CreateBlob(ctx, owner, repo, github.Blob{ + Content: github.Ptr(base64.StdEncoding.EncodeToString(decoded)), + Encoding: github.Ptr("base64"), + }) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, + fmt.Sprintf("failed to create blob for file %s", filePath), + resp, + err, + ), nil, nil + } + if resp != nil && resp.Body != nil { + _ = resp.Body.Close() + } + entries = append(entries, &github.TreeEntry{ + Path: github.Ptr(filePath), + Mode: github.Ptr("100644"), + Type: github.Ptr("blob"), + SHA: blob.SHA, + }) + default: + return utils.NewToolResultError(fmt.Sprintf("file %s has invalid encoding %q: must be \"utf-8\" or \"base64\"", filePath, encoding)), nil, nil + } } // Get the reference for the branch diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 8194895afa..830ee48fd3 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -2049,6 +2049,10 @@ func Test_ListCommits(t *testing.T) { } } +// binaryPNGBase64 is a standard base64 encoding of a 10x10 transparent PNG, +// used to exercise binary file content paths. +const binaryPNGBase64 = "iVBORw0KGgoAAAANSUhEUgAAAAoAAAAKCAIAAAACUFjqAAAAFElEQVR4nGP8n2XJgBsw4ZEbwdIABR4Btgm0KTcAAAAASUVORK5CYII=" + func Test_CreateOrUpdateFile(t *testing.T) { // Verify tool definition once serverTool := CreateOrUpdateFile(translations.NullTranslationHelper) @@ -2071,6 +2075,9 @@ func Test_CreateOrUpdateFile(t *testing.T) { assert.Contains(t, schema.Properties, "branch") assert.Contains(t, schema.Properties, "sha") assert.Contains(t, schema.Properties, "allow_symlink_write") + assert.Contains(t, schema.Properties, "encoding") + require.NotNil(t, schema.Properties["encoding"]) + assert.Equal(t, []any{"utf-8", "base64"}, schema.Properties["encoding"].Enum) assert.Contains(t, schema.Properties["sha"].Description, "with ref set to this tool's branch value") assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "path", "content", "message", "branch"}) @@ -2646,6 +2653,75 @@ func Test_CreateOrUpdateFile(t *testing.T) { expectedContent: mockFileResponse, expectedRequestCount: 2, }, + { + name: "successful binary file creation with base64 encoding", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + "GET /repos/owner/repo/contents/assets/logo.png": 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{ + "message": "Add logo", + // The decoded raw bytes are re-encoded by json.Marshal, which + // yields the same standard base64 string the caller supplied. + "content": binaryPNGBase64, + "branch": "main", + }).andThen( + mockResponse(t, http.StatusCreated, mockFileResponse), + ), + "PUT /repos/{owner}/{repo}/contents/{path:.*}": expectRequestBody(t, map[string]any{ + "message": "Add logo", + "content": binaryPNGBase64, + "branch": "main", + }).andThen( + mockResponse(t, http.StatusCreated, mockFileResponse), + ), + }), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "path": "assets/logo.png", + "content": binaryPNGBase64, + "encoding": "base64", + "message": "Add logo", + "branch": "main", + }, + expectError: false, + expectedContent: mockFileResponse, + expectedRequestCount: 2, + }, + { + name: "base64 encoding with invalid base64 content", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{}), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "path": "assets/logo.png", + "content": "not valid base64!!!", + "encoding": "base64", + "message": "Add logo", + "branch": "main", + }, + expectError: true, + expectedErrMsg: "content parameter is not valid base64", + }, + { + name: "unsupported encoding value", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{}), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "path": "assets/logo.png", + "content": "binary data", + "encoding": "hex", + "message": "Add logo", + "branch": "main", + }, + expectError: true, + expectedErrMsg: `invalid encoding "hex"`, + }, } for _, tc := range tests { @@ -3614,6 +3690,134 @@ func Test_PushFiles(t *testing.T) { expectError: false, expectedErrMsg: "failed to initialize repository", }, + { + name: "successful push of text and base64 binary files", + mockedClient: NewMockedHTTPClient( + // Get branch reference + WithRequestMatch( + GetReposGitRefByOwnerByRepoByRef, + mockRef, + ), + // Get commit + WithRequestMatch( + GetReposGitCommitsByOwnerByRepoByCommitSHA, + mockCommit, + ), + // Create blob for the binary file + WithRequestMatchHandler( + PostReposGitBlobsByOwnerByRepo, + expectRequestBody(t, map[string]any{ + "content": binaryPNGBase64, + "encoding": "base64", + }).andThen( + mockResponse(t, http.StatusCreated, &github.Blob{ + SHA: github.Ptr("blobSHA123"), + }), + ), + ), + // Create tree; the binary file is referenced by blob SHA instead of content + WithRequestMatchHandler( + PostReposGitTreesByOwnerByRepo, + expectRequestBody(t, map[string]any{ + "base_tree": "def456", + "tree": []any{ + map[string]any{ + "path": "README.md", + "mode": "100644", + "type": "blob", + "content": "# Updated README", + }, + map[string]any{ + "path": "assets/logo.png", + "mode": "100644", + "type": "blob", + "sha": "blobSHA123", + }, + }, + }).andThen( + mockResponse(t, http.StatusCreated, mockTree), + ), + ), + // Create commit + WithRequestMatchHandler( + PostReposGitCommitsByOwnerByRepo, + expectRequestBody(t, map[string]any{ + "message": "Update multiple files", + "tree": "ghi789", + "parents": []any{"abc123"}, + }).andThen( + mockResponse(t, http.StatusCreated, mockNewCommit), + ), + ), + // Update reference + WithRequestMatchHandler( + PatchReposGitRefsByOwnerByRepoByRef, + expectRequestBody(t, map[string]any{ + "sha": "jkl012", + "force": false, + }).andThen( + mockResponse(t, http.StatusOK, mockUpdatedRef), + ), + ), + ), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "branch": "main", + "files": []any{ + map[string]any{ + "path": "README.md", + "content": "# Updated README", + }, + map[string]any{ + "path": "assets/logo.png", + "content": binaryPNGBase64, + "encoding": "base64", + }, + }, + "message": "Update multiple files", + }, + expectError: false, + expectedRef: mockUpdatedRef, + }, + { + name: "base64 file with invalid base64 content", + mockedClient: NewMockedHTTPClient(), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "branch": "main", + "files": []any{ + map[string]any{ + "path": "assets/logo.png", + "content": "not valid base64!!!", + "encoding": "base64", + }, + }, + "message": "Update file", + }, + expectError: false, // This returns a tool error, not a Go error + expectedErrMsg: "file assets/logo.png content is not valid base64", + }, + { + name: "file with unsupported encoding value", + mockedClient: NewMockedHTTPClient(), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "branch": "main", + "files": []any{ + map[string]any{ + "path": "assets/logo.png", + "content": "binary data", + "encoding": "hex", + }, + }, + "message": "Update file", + }, + expectError: false, // This returns a tool error, not a Go error + expectedErrMsg: `file assets/logo.png has invalid encoding "hex"`, + }, } for _, tc := range tests {