From d4b8b2470159456a5961e9b19e1e38112c2a63f7 Mon Sep 17 00:00:00 2001 From: mesh-tools Date: Thu, 15 Oct 2026 20:39:13 +0000 Subject: [PATCH] Let gitea_commit take a large patch from a hash-checked spool file (issue 852) A feature's diff is megabytes, too large to type into a call. gitea_commit now also takes patch_file, the random hex name of a file in the module's spool directory /var/lib/mesh/forge-patches (mode 0733, owned by the machine's account), with patch_sha256. The tool reads only that directory, never through a symlink or a hard link, refuses a file whose SHA-256 differs, applies it as it applies patch, and removes it. Co-Authored-By: Claude --- modules/gitea/README.md | 14 +- modules/gitea/cmd/forge/commit.go | 36 ++++- modules/gitea/cmd/forge/main.go | 10 +- modules/gitea/cmd/forge/patchfile.go | 109 +++++++++++++ modules/gitea/cmd/forge/patchfile_test.go | 183 ++++++++++++++++++++++ modules/gitea/module.json | 10 +- 6 files changed, 350 insertions(+), 12 deletions(-) create mode 100644 modules/gitea/cmd/forge/patchfile.go create mode 100644 modules/gitea/cmd/forge/patchfile_test.go diff --git a/modules/gitea/README.md b/modules/gitea/README.md index c769a732..dca4ba53 100644 --- a/modules/gitea/README.md +++ b/modules/gitea/README.md @@ -44,7 +44,7 @@ base64-encoded. The Go bundle `cmd/forge` serves the commit, and the reads an ag | tool | | what | |---|---|---| -| `gitea_commit` | a | one commit on a branch of its own, created from `base` (default the default branch) when absent, carrying either `files` (each `{path, content \| content_base64 \| delete: true}`) or `patch` (a unified diff, as `git diff` writes it); answers the commit and its parent | +| `gitea_commit` | a | one commit on a branch of its own, created from `base` (default the default branch) when absent, carrying either `files` (each `{path, content \| content_base64 \| delete: true}`) `patch` (a unified diff, as `git diff` writes it), or `patch_file` with `patch_sha256` (a patch too large to type, see below); answers the commit and its parent | | `gitea_commits` | r | a ref's history, newest first, optionally only commits touching a path: sha, author, date, subject | | `gitea_compare` | r | what head holds that base does not: its commits and the files they touch | | `gitea_pull_reviews` | r | a pull request's reviews — state, the commit reviewed and whether it is the head now, stale, dismissed — with their line comments | @@ -62,6 +62,18 @@ the forge cannot compile covers its own text. `cmd/forge/rules.go` and `guard.ts commit. Nothing about merging changes: a merge still needs an independent review at the head and both required checks green, through `gitea_merge_pull_request`. +**A patch too large to type** (novox/hq issue 852: a feature's diff is megabytes) goes as `patch_file`, with +`patch_sha256`. The caller writes the patch into the module's spool directory `forge-patches` +(`/var/lib/mesh/forge-patches`, mode 0733 and owned by the machine's account, which the tools run as: anyone writes, +nobody lists; the bundle is given its path as `MESH_GITEA_PATCH_DIR`) and names it: `patch_file` is the file's NAME, +32 to 64 characters of `0-9a-f` that the caller chooses at random, never a path. The tool then reads that one file, +applies it exactly as it applies `patch` (same size limit, same answer) and removes it once it has found it, whether +the commit was made or not. A file refused before that (a bad name, a missing hash or spool directory, a name with no +file) stays in the spool. It refuses, sending nothing, a name of another shape, a file that is missing, empty, over +4 MiB, with more than one hard link or not a regular file (a symlink is never followed), and a file whose SHA-256 is not `patch_sha256`: the directory has no sticky bit +(the manifest allows none), so another writer could replace a file, and the hash is how the caller knows the tool read +the bytes it wrote. It reads nothing outside the directory. + **How the commit is made.** `files` become one call to the forge's multi-file contents endpoint: a path the ref holds is updated, one it does not is created, and an update or delete carries the blob read just before, so a file someone changed meanwhile is refused by the forge rather than overwritten. The blobs are read at diff --git a/modules/gitea/cmd/forge/commit.go b/modules/gitea/cmd/forge/commit.go index d3c2af52..f3b5e0df 100644 --- a/modules/gitea/cmd/forge/commit.go +++ b/modules/gitea/cmd/forge/commit.go @@ -35,6 +35,11 @@ type CommitAsk struct { Message string Files []FileChange Patch string // a unified diff, as git diff writes it, instead of Files + // PatchFile is the name of a file in the spool directory holding the patch (novox/hq issue 852), with the + // SHA-256 its caller says it has; commitAsk reads it into Patch. + PatchFile string + PatchSHA256 string + patchFileGiven bool } // Committed is what gitea_commit made. @@ -69,6 +74,11 @@ func readCommitAsk(args map[string]any) (CommitAsk, error) { a := CommitAsk{Owner: text(args, "owner"), Repo: text(args, "repo"), Branch: text(args, "branch"), Base: text(args, "base"), Message: strings.TrimSpace(raw(args, "message"))} a.Patch = raw(args, "patch") + if v, ok := args["patch_file"]; ok && v != nil { + a.patchFileGiven = true + a.PatchFile = raw(args, "patch_file") + } + a.PatchSHA256 = strings.ToLower(strings.TrimSpace(raw(args, "patch_sha256"))) if a.Branch == "" { return a, refuse("branch is required: the branch the commit goes on, a branch of its own (created from base when absent). Nothing was sent") } @@ -111,11 +121,23 @@ func readCommitAsk(args map[string]any) (CommitAsk, error) { a.Files = append(a.Files, fc) } } + sources := 0 + for _, g := range []bool{len(a.Files) > 0, strings.TrimSpace(a.Patch) != "", a.patchFileGiven} { + if g { + sources++ + } + } switch { - case len(a.Files) == 0 && strings.TrimSpace(a.Patch) == "": - return a, refuse("give files (each {path, content | content_base64 | delete: true}) or patch (a unified diff, as git diff writes it). Nothing was sent") - case len(a.Files) > 0 && strings.TrimSpace(a.Patch) != "": - return a, refuse("give files or patch, not both: one commit carries one of them. Nothing was sent") + case sources == 0: + return a, refuse("give files (each {path, content | content_base64 | delete: true}), patch (a unified diff, as git diff writes it) or patch_file (the name of a patch in the spool directory). Nothing was sent") + case sources > 1: + return a, refuse("give files, patch or patch_file, not more than one: one commit carries one of them. Nothing was sent") + case a.PatchSHA256 != "" && !a.patchFileGiven: + return a, refuse("patch_sha256 goes with patch_file only. Nothing was sent") + case a.patchFileGiven: + if err := checkPatchFileAsk(a); err != nil { + return a, err + } case len(a.Files) > maxFiles: return a, refuse("%d files is more than the %d one commit carries. Nothing was sent", len(a.Files), maxFiles) case len(a.Patch) > maxPatchBytes: @@ -134,13 +156,13 @@ func readCommitAsk(args map[string]any) (CommitAsk, error) { return a, refuse("%s is named twice in files. Nothing was sent", fc.Path) } seen[fc.Path] = true - given := 0 + have := 0 for _, g := range []bool{fc.Content != nil, fc.ContentBase64 != nil, fc.Delete} { if g { - given++ + have++ } } - if given != 1 { + if have != 1 { return a, refuse("files[%d] (%s) needs exactly one of content, content_base64 or delete: true. Nothing was sent", i, fc.Path) } if fc.Content != nil && !utf8.ValidString(*fc.Content) { diff --git a/modules/gitea/cmd/forge/main.go b/modules/gitea/cmd/forge/main.go index ee5e1e0a..6388f096 100644 --- a/modules/gitea/cmd/forge/main.go +++ b/modules/gitea/cmd/forge/main.go @@ -55,7 +55,9 @@ func Tools(forge func() (*Forge, error)) []stdio.Tool { "the branch holds them — or patch, a unified diff as git diff writes it, which the forge applies. " + "At most 200 files, 4 MiB each and 16 MiB together, or a 4 MiB patch. Answers the commit it made and " + "the parent the forge made it on, read back from the branch, and says when the branch moved under " + - "it. Replaces git commit and git " + + "it. A patch too large to type goes as patch_file with patch_sha256: the name of a file in the " + + "spool directory (read once, applied as patch is, removed afterwards, never read from anywhere " + + "else). Replaces git commit and git " + "push of a feature branch. (a)", Input: map[string]any{ "type": "object", @@ -79,7 +81,9 @@ func Tools(forge func() (*Forge, error)) []stdio.Tool { "required": []string{"path"}, }, }, - "patch": str("a unified diff of the change, as git diff or git format-patch writes it; give this or files"), + "patch": str("a unified diff of the change, as git diff or git format-patch writes it; give this, patch_file or files"), + "patch_file": str("for a patch too large to type: the NAME (32 to 64 characters of 0-9a-f, chosen at random by the caller, no path) of a file the caller wrote into the forge patch spool directory (the module's forge-patches directory); needs patch_sha256; give this, patch or files"), + "patch_sha256": str("with patch_file: the file's SHA-256, in hex; the commit is refused, and nothing is sent, when the file's differs"), }, "required": []string{"owner", "repo", "branch", "message"}, }, @@ -88,7 +92,7 @@ func Tools(forge func() (*Forge, error)) []stdio.Tool { if err != nil { return nil, err } - return commit(ctx, f, a) + return commitAsk(ctx, f, os.Getenv(patchDirEnv), a) }), }, { diff --git a/modules/gitea/cmd/forge/patchfile.go b/modules/gitea/cmd/forge/patchfile.go new file mode 100644 index 00000000..ba040f91 --- /dev/null +++ b/modules/gitea/cmd/forge/patchfile.go @@ -0,0 +1,109 @@ +package main + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "errors" + "io" + "os" + "path/filepath" + "regexp" + "strings" + "syscall" +) + +// A patch too large to type into a call is handed over as a file (novox/hq issue 852). The caller writes it into +// one spool directory the module declares (mode 0733: anyone writes, nobody lists) and names it in patch_file, +// with the file's SHA-256. The tool runs as the machine's account, so it reads nothing but that directory: the +// name is 32 to 64 characters of [0-9a-f], so it holds no separator and no dot; the file must be a regular file +// (never a symlink); and its hash must be the caller's, because without the sticky bit another writer could have +// replaced it. The file is removed once taken, whether the commit was made or not. + +const patchDirEnv = "MESH_GITEA_PATCH_DIR" + +var ( + patchFileName = regexp.MustCompile(`^[0-9a-f]{32,64}$`) + patchSHA = regexp.MustCompile(`^[0-9a-f]{64}$`) +) + +// checkPatchFileAsk judges the arguments of a patch_file ask, before any file is looked at. +func checkPatchFileAsk(a CommitAsk) error { + if !patchFileName.MatchString(a.PatchFile) { + return refuse("patch_file %q is not a name: 32 to 64 characters of 0-9a-f, no path (the file's name in the spool directory, chosen at random). Nothing was sent", a.PatchFile) + } + if !patchSHA.MatchString(a.PatchSHA256) { + return refuse("patch_file needs patch_sha256: the file's SHA-256, 64 hex characters. Nothing was sent") + } + return nil +} + +// commitAsk makes the commit the ask names; a patch_file is first read from the spool directory into the patch +// and removed afterwards. +func commitAsk(ctx context.Context, f *Forge, spool string, a CommitAsk) (*Committed, error) { + if !a.patchFileGiven { + return commit(ctx, f, a) + } + if err := checkPatchFileAsk(a); err != nil { + return nil, err + } + if spool == "" { + return nil, refuse("patch_file is not available: %s is not set, so there is no spool directory. Nothing was sent", patchDirEnv) + } + path := filepath.Join(spool, a.PatchFile) + defer func() { + // Whatever happened, the file is not left: removing a symlink removes the link, never its target. + if st, err := os.Lstat(path); err == nil && !st.IsDir() { + _ = os.Remove(path) + } + }() + body, err := readSpooled(path, a.PatchFile) + if err != nil { + return nil, err + } + got := sha256.Sum256(body) + if hex.EncodeToString(got[:]) != a.PatchSHA256 { + return nil, refuse("the file %s is not the patch the caller hashed: its SHA-256 is %s, patch_sha256 says %s. Nothing was sent", + a.PatchFile, hex.EncodeToString(got[:]), a.PatchSHA256) + } + a.Patch = string(body) + return commit(ctx, f, a) +} + +// readSpooled reads a regular file of at most maxPatchBytes, never through a symlink. +func readSpooled(path, name string) ([]byte, error) { + st, err := os.Lstat(path) + switch { + case errors.Is(err, os.ErrNotExist): + return nil, refuse("patch_file %s: no such file in the spool directory. Nothing was sent", name) + case err != nil: + return nil, refuse("patch_file %s cannot be read: %v. Nothing was sent", name, err) + case !st.Mode().IsRegular(): + return nil, refuse("patch_file %s is not a regular file (a symlink or a directory is never followed). Nothing was sent", name) + case st.Size() > maxPatchBytes: + return nil, refuse("patch_file %s is %s, more than the %s one commit carries: split it into several commits. Nothing was sent", name, mib(int(st.Size())), mib(maxPatchBytes)) + } + fh, err := os.OpenFile(path, os.O_RDONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0) + if err != nil { + return nil, refuse("patch_file %s cannot be opened: %v. Nothing was sent", name, err) + } + defer fh.Close() + fst, err := fh.Stat() + if err != nil || !fst.Mode().IsRegular() { + return nil, refuse("patch_file %s is not a regular file. Nothing was sent", name) + } + // A hard link to a file elsewhere is no symlink, but it is not the caller's own file either. + if sys, ok := fst.Sys().(*syscall.Stat_t); !ok || sys.Nlink > 1 { + return nil, refuse("patch_file %s has more than one hard link: it must be a file of its own. Nothing was sent", name) + } + body, err := io.ReadAll(io.LimitReader(fh, maxPatchBytes+1)) + switch { + case err != nil: + return nil, refuse("patch_file %s cannot be read: %v. Nothing was sent", name, err) + case len(body) > maxPatchBytes: + return nil, refuse("patch_file %s is more than the %s one commit carries: split it into several commits. Nothing was sent", name, mib(maxPatchBytes)) + case strings.TrimSpace(string(body)) == "": + return nil, refuse("patch_file %s is empty. Nothing was sent", name) + } + return body, nil +} diff --git a/modules/gitea/cmd/forge/patchfile_test.go b/modules/gitea/cmd/forge/patchfile_test.go new file mode 100644 index 00000000..cd0f34e1 --- /dev/null +++ b/modules/gitea/cmd/forge/patchfile_test.go @@ -0,0 +1,183 @@ +package main + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "os" + "path/filepath" + "strings" + "testing" +) + +// novox/hq issue 852: a patch too large to type into a call is handed over as a file in one spool directory. + +const spoolName = "0123456789abcdef0123456789abcdef" +const spoolPatch = "diff --git a/README.md b/README.md\n--- a/README.md\n+++ b/README.md\n@@ -1 +1 @@\n-hello\n+hi\n" + +func sum(s string) string { h := sha256.Sum256([]byte(s)); return hex.EncodeToString(h[:]) } + +func spoolWith(t *testing.T, name, content string) string { + t.Helper() + dir := t.TempDir() + if name != "" { + if err := os.WriteFile(filepath.Join(dir, name), []byte(content), 0o600); err != nil { + t.Fatal(err) + } + } + return dir +} + +func fileAsk(t *testing.T, name, sha string) CommitAsk { + t.Helper() + return ask(t, map[string]any{"branch": "fix/9", "patch_file": name, "patch_sha256": sha}) +} + +func TestAPatchFileIsAppliedAsThePatchWouldBeAndTheFileIsGone(t *testing.T) { + f := newFake(t) + dir := spoolWith(t, spoolName, spoolPatch) + got, err := commitAsk(context.Background(), f.serve(), dir, fileAsk(t, spoolName, sum(spoolPatch))) + if err != nil { + t.Fatal(err) + } + if len(f.patches) != 1 || f.patches[0]["content"] != spoolPatch || f.patches[0]["new_branch"] != "fix/9" || got.Commit != "p1" { + t.Fatalf("%v %+v", f.patches, got) + } + if _, err := os.Lstat(filepath.Join(dir, spoolName)); !os.IsNotExist(err) { + t.Fatalf("the file is still there: %v", err) + } +} + +func TestAPatchFileIsRemovedWhenTheForgeRefusesIt(t *testing.T) { + f := newFake(t) + dir := spoolWith(t, spoolName, spoolPatch) + // the default branch is refused after the file was read + a := ask(t, map[string]any{"branch": "main", "patch_file": spoolName, "patch_sha256": sum(spoolPatch)}) + if _, err := commitAsk(context.Background(), f.serve(), dir, a); err == nil { + t.Fatal("taken") + } + if _, err := os.Lstat(filepath.Join(dir, spoolName)); !os.IsNotExist(err) { + t.Fatalf("the file is still there: %v", err) + } +} + +func TestAPatchFileThatCannotBeTakenIsRefusedBeforeAnythingIsSent(t *testing.T) { + outside := filepath.Join(t.TempDir(), "secret") + if err := os.WriteFile(outside, []byte(spoolPatch), 0o600); err != nil { + t.Fatal(err) + } + type tc struct { + name string + setup func(dir string) + file string + sha string + want string + } + for _, c := range []tc{ + {"wrong sha", func(d string) { _ = os.WriteFile(filepath.Join(d, spoolName), []byte(spoolPatch), 0o600) }, spoolName, sum("other"), "sha256"}, + {"not found", func(string) {}, spoolName, sum(spoolPatch), "no such file"}, + {"symlink", func(d string) { _ = os.Symlink(outside, filepath.Join(d, spoolName)) }, spoolName, sum(spoolPatch), "regular file"}, + {"hard link", func(d string) { _ = os.Link(outside, filepath.Join(d, spoolName)) }, spoolName, sum(spoolPatch), "hard link"}, + {"directory", func(d string) { _ = os.Mkdir(filepath.Join(d, spoolName), 0o700) }, spoolName, sum(spoolPatch), "regular file"}, + {"too large", func(d string) { + _ = os.WriteFile(filepath.Join(d, spoolName), []byte(strings.Repeat("x", maxPatchBytes+1)), 0o600) + }, spoolName, sum(spoolPatch), "MiB"}, + {"empty", func(d string) { _ = os.WriteFile(filepath.Join(d, spoolName), nil, 0o600) }, spoolName, sum(""), "empty"}, + } { + f := newFake(t) + dir := t.TempDir() + c.setup(dir) + _, err := commitAsk(context.Background(), f.serve(), dir, fileAsk(t, c.file, c.sha)) + if err == nil || !strings.Contains(err.Error(), c.want) || !strings.Contains(err.Error(), "Nothing was sent") { + t.Errorf("%s: %v", c.name, err) + } + if len(f.patches)+len(f.writes) != 0 { + t.Errorf("%s: something was sent", c.name) + } + } + if b, err := os.ReadFile(outside); err != nil || string(b) != spoolPatch { + t.Errorf("the file outside the spool was touched: %v", err) + } +} + +func TestAPatchFileAskThatCannotBeReadIsRefused(t *testing.T) { + ok := sum(spoolPatch) + for name, args := range map[string]map[string]any{ + "no sha": {"patch_file": spoolName}, + "sha not hex": {"patch_file": spoolName, "patch_sha256": strings.Repeat("z", 64)}, + "sha too short": {"patch_file": spoolName, "patch_sha256": ok[:63]}, + "empty name": {"patch_file": "", "patch_sha256": ok, "files": filesArg()}, + "dotdot": {"patch_file": "../" + spoolName, "patch_sha256": ok}, + "slash": {"patch_file": "a/" + spoolName, "patch_sha256": ok}, + "absolute": {"patch_file": "/etc/passwd", "patch_sha256": ok}, + "too short": {"patch_file": strings.Repeat("a", 31), "patch_sha256": ok}, + "too long": {"patch_file": strings.Repeat("a", 65), "patch_sha256": ok}, + "upper case": {"patch_file": strings.ToUpper(spoolName), "patch_sha256": ok}, + "not hex": {"patch_file": strings.Repeat("g", 32), "patch_sha256": ok}, + "with patch": {"patch_file": spoolName, "patch_sha256": ok, "patch": "diff"}, + "with files": {"patch_file": spoolName, "patch_sha256": ok, "files": filesArg(map[string]any{"path": "x", "content": "x"})}, + "sha without it": {"patch": "diff", "patch_sha256": ok}, + } { + full := map[string]any{"owner": "acme", "repo": "app", "message": "m", "branch": "b"} + for k, v := range args { + full[k] = v + } + _, err := readCommitAsk(full) + if err == nil || !strings.Contains(err.Error(), "Nothing was sent") { + t.Errorf("%s: %v", name, err) + } + } + // 64 characters is taken; so is an upper-case sha. + for _, n := range []string{spoolName, strings.Repeat("ab", 32)} { + if _, err := readCommitAsk(map[string]any{"owner": "acme", "repo": "app", "message": "m", "branch": "b", + "patch_file": n, "patch_sha256": strings.ToUpper(sum(spoolPatch))}); err != nil { + t.Errorf("%s: %v", n, err) + } + } +} + +func TestAPatchFileWithoutASpoolDirectoryIsRefused(t *testing.T) { + f := newFake(t) + _, err := commitAsk(context.Background(), f.serve(), "", fileAsk(t, spoolName, sum(spoolPatch))) + if err == nil || !strings.Contains(err.Error(), "MESH_GITEA_PATCH_DIR") || !strings.Contains(err.Error(), "Nothing was sent") { + t.Fatalf("%v", err) + } +} + +func TestTheManifestDeclaresTheSpoolDirectoryAndGivesItToTheBundle(t *testing.T) { + raw, err := os.ReadFile("../../module.json") + if err != nil { + t.Fatal(err) + } + var m struct { + Resources []struct { + ID, Type, Path, Mode, Owner string + } `json:"resources"` + Build struct { + Artifacts []struct { + From string `json:"from"` + Env map[string]string `json:"env"` + } `json:"artifacts"` + } `json:"build"` + } + if err := jsonInto(raw, &m); err != nil { + t.Fatal(err) + } + found := false + for _, r := range m.Resources { + if r.ID == "forge-patches" { + found = true + if r.Type != "directory" || r.Mode != "0733" || r.Owner != "${machine:account}" || r.Path == "" { + t.Errorf("%+v", r) + } + } + } + if !found { + t.Error("no forge-patches directory") + } + for _, a := range m.Build.Artifacts { + if a.From == "cmd/forge" && a.Env["MESH_GITEA_PATCH_DIR"] != "${dir:forge-patches}" { + t.Errorf("env %v", a.Env) + } + } +} diff --git a/modules/gitea/module.json b/modules/gitea/module.json index 987cd1d6..1908795c 100644 --- a/modules/gitea/module.json +++ b/modules/gitea/module.json @@ -184,6 +184,13 @@ "trusted": true, "content": "# The accounts whose \"ready at \" comment marks a pull request ready (novox/hq issue 722), from the setting\n# ready-authors, written by the mesh. Replaced on every push; change the setting, never this file.\nauthors=${setting:ready-authors}\n" }, + { + "id": "forge-patches", + "type": "directory", + "path": "/var/lib/mesh/forge-patches", + "mode": "0733", + "owner": "${machine:account}" + }, { "id": "state", "type": "directory", @@ -347,7 +354,8 @@ "env": { "MESH_GITEA_URL": "http://127.0.0.1:${port:3000}", "MESH_GITEA_TOOLS_USER": "mesh-tools", - "MESH_GITEA_TOKEN_FILE": "${dir:runtime-state}/token" + "MESH_GITEA_TOKEN_FILE": "${dir:runtime-state}/token", + "MESH_GITEA_PATCH_DIR": "${dir:forge-patches}" } } ]