From 16afe42549b5b72e41f95f22d8e92375d352a7bf Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Tue, 22 Sep 2026 18:46:25 +0300 Subject: [PATCH 1/6] fix(tray): install the tray binary, not the core, on self-update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tray's self-update hands update.Apply a TargetPath of os.Executable(). cmd/mcpproxy-tray is the only importer of internal/tray, so that path is always mcpproxy-tray — but the extraction picked the wrong member out of the archive: - applyTarGzUpdate selected on strings.HasSuffix(name, "mcpproxy"), and a release .tar.gz ships mcpproxy AND mcpproxy-tray side by side (.github/workflows/release.yml stages both, core first). A "successful" update therefore wrote the headless core over the tray executable, leaving the user with an mcpproxy-tray that is really a core. - applyZipUpdate applied the FIRST non-directory entry regardless of name. Windows archives list mcpproxy.exe first, so this was the same bug, and any future layout change would silently install some other file. Both now extract the member matched on its exact base name via a new trayBinaryName(), and fail closed when the archive does not carry it. The matching logic is the one cmd/mcpproxy already had; it moves to internal/updatecheck (ExtractBinary, MaxArchiveMemberBytes) so both self-update paths share one implementation, the way ParseChecksums and VerifyFileSHA256 were shared. cmd/mcpproxy keeps thin delegating wrappers so its own tests and call sites are unchanged. The tray also gains the 512 MB per-member cap for free. --- cmd/mcpproxy/update_apply.go | 100 +--------- internal/tray/tray.go | 117 +++++------ internal/tray/update_extract_test.go | 281 +++++++++++++++++++++++++++ internal/tray/update_verify_test.go | 14 +- internal/updatecheck/extract.go | 114 +++++++++++ 5 files changed, 473 insertions(+), 153 deletions(-) create mode 100644 internal/tray/update_extract_test.go create mode 100644 internal/updatecheck/extract.go diff --git a/cmd/mcpproxy/update_apply.go b/cmd/mcpproxy/update_apply.go index 5ff01e5f9..0455c4e01 100644 --- a/cmd/mcpproxy/update_apply.go +++ b/cmd/mcpproxy/update_apply.go @@ -1,9 +1,6 @@ package main import ( - "archive/tar" - "archive/zip" - "compress/gzip" "context" "errors" "fmt" @@ -23,10 +20,10 @@ import ( // every rule below is unit-testable against real files in a t.TempDir(). const ( - // maxArchiveMemberBytes bounds a single extracted archive member. The core - // binary is ~60-90 MB; the cap exists so a malicious archive cannot fill - // the disk before the checksum comparison would have rejected it. - maxArchiveMemberBytes = 512 << 20 + // maxArchiveMemberBytes bounds a single extracted archive member. The + // implementation lives in internal/updatecheck so the tray's self-update + // path enforces exactly the same cap. + maxArchiveMemberBytes = updatecheck.MaxArchiveMemberBytes // verifyExecTimeout bounds the post-swap ` --version` probe // (FR-021: success means the new binary actually runs). @@ -57,91 +54,12 @@ func verifyFileSHA256(path, wantHex string) error { } // extractBinary pulls the single archive member named memberName (matched on -// base name, so a future archive that nests files still works) into destPath, -// which is created with mode 0o700 — the caller re-applies the real mode when -// swapping it into place. +// base name) into destPath. The implementation lives in internal/updatecheck +// so the tray's self-update path selects its archive member by the same rule — +// a release archive ships both the core and the tray binary, and picking the +// wrong one installs a working binary over the wrong file. func extractBinary(archivePath, memberName, destPath string) error { - switch { - case strings.HasSuffix(archivePath, ".zip"): - return extractFromZip(archivePath, memberName, destPath) - case strings.HasSuffix(archivePath, ".tar.gz"), strings.HasSuffix(archivePath, ".tgz"): - return extractFromTarGz(archivePath, memberName, destPath) - default: - return fmt.Errorf("unsupported archive format: %s", filepath.Base(archivePath)) - } -} - -func extractFromTarGz(archivePath, memberName, destPath string) error { - f, err := os.Open(archivePath) // #nosec G304 -- self-downloaded temp file - if err != nil { - return fmt.Errorf("open archive: %w", err) - } - defer f.Close() - - gz, err := gzip.NewReader(f) - if err != nil { - return fmt.Errorf("open gzip stream: %w", err) - } - defer gz.Close() - - tr := tar.NewReader(gz) - for { - hdr, err := tr.Next() - if errors.Is(err, io.EOF) { - break - } - if err != nil { - return fmt.Errorf("read archive: %w", err) - } - if hdr.Typeflag != tar.TypeReg || filepath.Base(hdr.Name) != memberName { - continue - } - return writeMember(tr, destPath) - } - return fmt.Errorf("archive does not contain %q", memberName) -} - -func extractFromZip(archivePath, memberName, destPath string) error { - zr, err := zip.OpenReader(archivePath) - if err != nil { - return fmt.Errorf("open archive: %w", err) - } - defer zr.Close() - - for _, entry := range zr.File { - if entry.FileInfo().IsDir() || filepath.Base(entry.Name) != memberName { - continue - } - rc, err := entry.Open() - if err != nil { - return fmt.Errorf("open archive member: %w", err) - } - defer rc.Close() - return writeMember(rc, destPath) - } - return fmt.Errorf("archive does not contain %q", memberName) -} - -// writeMember copies at most maxArchiveMemberBytes from r into destPath. -func writeMember(r io.Reader, destPath string) error { - out, err := os.OpenFile(destPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC|os.O_EXCL, 0o700) // #nosec G304 -- destPath is our own temp path - if err != nil { - return fmt.Errorf("create staged binary: %w", err) - } - written, err := io.Copy(out, io.LimitReader(r, maxArchiveMemberBytes+1)) - if err != nil { - out.Close() - return fmt.Errorf("write staged binary: %w", err) - } - if written > maxArchiveMemberBytes { - out.Close() - return fmt.Errorf("archive member exceeds the %d-byte limit", int64(maxArchiveMemberBytes)) - } - if err := out.Sync(); err != nil { - out.Close() - return fmt.Errorf("flush staged binary: %w", err) - } - return out.Close() + return updatecheck.ExtractBinary(archivePath, memberName, destPath) } // ensureTargetWritable reports why the binary cannot be replaced, naming the diff --git a/internal/tray/tray.go b/internal/tray/tray.go index 9d509423c..4885926b5 100644 --- a/internal/tray/tray.go +++ b/internal/tray/tray.go @@ -3,9 +3,6 @@ package tray import ( - "archive/tar" - "archive/zip" - "compress/gzip" "context" _ "embed" "encoding/json" @@ -1404,7 +1401,10 @@ func (a *App) downloadAndApplyUpdate(release *GitHubRelease, assetName, url stri return err } - archivePath := filepath.Join(workDir, "artifact") + // The archive keeps the published extension (and only that — never the + // asset name itself, which is server-supplied): ExtractBinary dispatches + // tar.gz vs zip off the path suffix. + archivePath := filepath.Join(workDir, "artifact"+archiveExt(assetName)) if err := downloadToFile(url, archivePath); err != nil { return fmt.Errorf("download %s: %w", assetName, err) } @@ -1419,10 +1419,8 @@ func (a *App) downloadAndApplyUpdate(release *GitHubRelease, assetName, url stri zap.String("sha256", wantDigest)) switch { - case strings.HasSuffix(assetName, assetZipExt): - return a.applyZipUpdate(archivePath) - case strings.HasSuffix(assetName, assetTarGzExt): - return a.applyTarGzUpdate(archivePath) + case archiveExt(assetName) != "": + return a.applyArchiveUpdate(archivePath) default: f, err := os.Open(archivePath) // #nosec G304 -- archivePath is inside a temp dir this process just created if err != nil { @@ -1433,72 +1431,75 @@ func (a *App) downloadAndApplyUpdate(release *GitHubRelease, assetName, url stri } } -// applyZipUpdate extracts and applies an update from a downloaded zip archive -func (a *App) applyZipUpdate(archivePath string) error { - r, err := zip.OpenReader(archivePath) - if err != nil { - return err +// archiveExt returns the archive extension assetName carries, or "" when it is +// not an archive this path knows how to open. +func archiveExt(assetName string) string { + switch { + case strings.HasSuffix(assetName, assetZipExt): + return assetZipExt + case strings.HasSuffix(assetName, assetTarGzExt): + return assetTarGzExt + default: + return "" } - defer r.Close() +} - executablePath, err := os.Executable() - if err != nil { - return err +// trayBinaryName is the archive member this process installs over itself. +// +// The self-update runs inside mcpproxy-tray (cmd/mcpproxy-tray is the only +// importer of this package) and hands update.Apply a TargetPath of +// os.Executable(), so the member extracted has to be the TRAY binary. Release +// archives ship the core binary and the tray binary side by side — +// .github/workflows/release.yml stages mcpproxy plus mcpproxy-tray[.exe] — and +// list the core first, so any "first entry" or suffix-based rule pulls out the +// core and installs it over the tray, leaving the user with a tray executable +// that is really a headless core. The core keeps updating itself through +// `mcpproxy update`, which selects its own member the same way +// (coreBinaryName in cmd/mcpproxy/update_cmd.go). +func trayBinaryName() string { + name := "mcpproxy-tray" + if runtime.GOOS == osWindows { + name += ".exe" } + return name +} - for _, f := range r.File { - if f.FileInfo().IsDir() { - continue - } - rc, err := f.Open() - if err != nil { - return err - } +// applyArchiveUpdate extracts the tray binary out of the already-downloaded, +// already-checksum-verified archive and swaps it over the running executable. +// +// Extraction matches the member on its exact base name and fails closed when it +// is absent: an archive without the tray binary means the layout changed, and +// installing some other member instead is worse than not updating. +func (a *App) applyArchiveUpdate(archivePath string) error { + member := trayBinaryName() - err = a.applyUpdate(rc, update.Options{TargetPath: executablePath}) - rc.Close() - return err + stageDir, err := os.MkdirTemp(filepath.Dir(archivePath), "extract-*") + if err != nil { + return fmt.Errorf("create extraction directory: %w", err) } + defer os.RemoveAll(stageDir) - return fmt.Errorf("no file found in zip archive to apply") -} + stagedPath := filepath.Join(stageDir, member) + if err := updatecheck.ExtractBinary(archivePath, member, stagedPath); err != nil { + return fmt.Errorf("extract %s: %w", member, err) + } -// applyTarGzUpdate extracts and applies an update from a downloaded tar.gz archive -func (a *App) applyTarGzUpdate(archivePath string) error { - f, err := os.Open(archivePath) // #nosec G304 -- archivePath is inside a temp dir this process just created + staged, err := os.Open(stagedPath) // #nosec G304 -- stagedPath is inside a temp dir this process just created if err != nil { - return err + return fmt.Errorf("open extracted %s: %w", member, err) } - defer f.Close() + defer staged.Close() - gzr, err := gzip.NewReader(f) + executablePath, err := os.Executable() if err != nil { return err } - defer gzr.Close() - tr := tar.NewReader(gzr) - for { - header, err := tr.Next() - if err == io.EOF { - break - } - if err != nil { - return err - } - - // Look for the mcpproxy binary (could be mcpproxy or mcpproxy.exe) - if strings.HasSuffix(header.Name, "mcpproxy") || strings.HasSuffix(header.Name, "mcpproxy.exe") { - executablePath, err := os.Executable() - if err != nil { - return err - } - - return a.applyUpdate(tr, update.Options{TargetPath: executablePath}) - } - } + a.logger.Info("Installing tray binary from update archive", + zap.String("member", member), + zap.String("target", executablePath)) - return fmt.Errorf("no mcpproxy binary found in tar.gz archive") + return a.applyUpdate(staged, update.Options{TargetPath: executablePath}) } // openConfigDir opens the directory containing the configuration file diff --git a/internal/tray/update_extract_test.go b/internal/tray/update_extract_test.go new file mode 100644 index 000000000..322ac5255 --- /dev/null +++ b/internal/tray/update_extract_test.go @@ -0,0 +1,281 @@ +//go:build !nogui && !headless && !linux + +package tray + +import ( + "archive/tar" + "archive/zip" + "bytes" + "compress/gzip" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "runtime" + "strings" + "sync/atomic" + "testing" + + "github.com/inconshreveable/go-update" + "go.uber.org/zap/zaptest" +) + +// archiveMember is one file inside a test archive. +type archiveMember struct { + name string + payload []byte +} + +// buildTarGzMembers returns a .tar.gz containing the given members in order. +func buildTarGzMembers(t *testing.T, members ...archiveMember) []byte { + t.Helper() + var buf bytes.Buffer + gz := gzip.NewWriter(&buf) + tw := tar.NewWriter(gz) + for _, m := range members { + if err := tw.WriteHeader(&tar.Header{ + Typeflag: tar.TypeReg, + Name: m.name, + Mode: 0o755, + Size: int64(len(m.payload)), + }); err != nil { + t.Fatalf("tar header %s: %v", m.name, err) + } + if _, err := tw.Write(m.payload); err != nil { + t.Fatalf("tar write %s: %v", m.name, err) + } + } + if err := tw.Close(); err != nil { + t.Fatalf("tar close: %v", err) + } + if err := gz.Close(); err != nil { + t.Fatalf("gzip close: %v", err) + } + return buf.Bytes() +} + +// buildZipMembers returns a .zip containing the given members in order. +func buildZipMembers(t *testing.T, members ...archiveMember) []byte { + t.Helper() + var buf bytes.Buffer + zw := zip.NewWriter(&buf) + for _, m := range members { + w, err := zw.Create(m.name) + if err != nil { + t.Fatalf("zip create %s: %v", m.name, err) + } + if _, err := w.Write(m.payload); err != nil { + t.Fatalf("zip write %s: %v", m.name, err) + } + } + if err := zw.Close(); err != nil { + t.Fatalf("zip close: %v", err) + } + return buf.Bytes() +} + +// serveVerifiedAsset stands up a release whose checksums.txt already vouches +// for the archive, so these tests exercise extraction rather than the +// (separately tested) checksum gate. +func serveVerifiedAsset(t *testing.T, assetName string, archive []byte) (*GitHubRelease, string) { + t.Helper() + mux := http.NewServeMux() + mux.HandleFunc("/asset", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(archive) + }) + mux.HandleFunc("/checksums.txt", func(w http.ResponseWriter, _ *http.Request) { + _, _ = io.WriteString(w, sha256Hex(archive)+" "+assetName+"\n") + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + return &GitHubRelease{ + TagName: "v9.9.9", + Assets: []ghAsset{ + {Name: assetName, BrowserDownloadURL: srv.URL + "/asset"}, + {Name: "checksums.txt", BrowserDownloadURL: srv.URL + "/checksums.txt"}, + }, + }, srv.URL + "/asset" +} + +// TestApp_SelfUpdate_InstallsTheTrayBinaryNotTheCore is the regression test for +// the bug this change fixes: a release archive holds BOTH mcpproxy and +// mcpproxy-tray, the self-update runs inside the tray process (only +// cmd/mcpproxy-tray imports this package), and update.Options.TargetPath is +// os.Executable() — the tray. Selecting anything but the tray member therefore +// installs the CORE binary over the TRAY binary and bricks the tray. +// +// The archive deliberately lists mcpproxy first, which is the order +// .github/workflows/release.yml produces, so a "first entry wins" or a +// HasSuffix("mcpproxy") rule picks the wrong file. +func TestApp_SelfUpdate_InstallsTheTrayBinaryNotTheCore(t *testing.T) { + const ( + corePayload = "PRETEND CORE BINARY" + trayPayload = "PRETEND TRAY BINARY" + ) + + tests := []struct { + name string + assetName string + archive func(t *testing.T, members ...archiveMember) []byte + }{ + {"tar.gz", "mcpproxy-latest-darwin-arm64" + assetTarGzExt, buildTarGzMembers}, + {"zip", "mcpproxy-latest-windows-amd64" + assetZipExt, buildZipMembers}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + coreName, trayName := "mcpproxy", "mcpproxy-tray" + if strings.HasSuffix(tt.assetName, assetZipExt) { + coreName, trayName = "mcpproxy.exe", "mcpproxy-tray.exe" + } + // The archive always ships BOTH members under their canonical + // names, whatever this test happens to run on; trayBinaryName() + // is what decides which one the running tray installs. + archive := tt.archive(t, + archiveMember{coreName, []byte(corePayload)}, + archiveMember{trayName, []byte(trayPayload)}, + archiveMember{"mcpproxy-tray", []byte(trayPayload)}, + archiveMember{"mcpproxy-tray.exe", []byte(trayPayload)}, + ) + release, url := serveVerifiedAsset(t, tt.assetName, archive) + + var applied atomic.Int32 + var gotPayload string + var gotTarget string + app := New(NewMockServer(), zaptest.NewLogger(t).Sugar(), "1.0.0", func() {}) + app.applyUpdateFn = func(r io.Reader, opts update.Options) error { + b, err := io.ReadAll(r) + if err != nil { + return err + } + gotPayload = string(b) + gotTarget = opts.TargetPath + applied.Add(1) + return nil + } + + if err := app.downloadAndApplyUpdate(release, tt.assetName, url); err != nil { + t.Fatalf("downloadAndApplyUpdate: %v", err) + } + if got := applied.Load(); got != 1 { + t.Fatalf("applyUpdate called %d times, want 1", got) + } + if gotPayload != trayPayload { + t.Errorf("installed %q, want the tray binary %q — installing the core binary over %s bricks the tray", + gotPayload, trayPayload, trayBinaryName()) + } + exe, err := os.Executable() + if err != nil { + t.Fatalf("os.Executable: %v", err) + } + if gotTarget != exe { + t.Errorf("TargetPath = %q, want the running executable %q", gotTarget, exe) + } + }) + } +} + +// TestApp_SelfUpdate_RefusesArchiveWithoutTrayBinary: an archive that does not +// carry the tray binary must fail closed. Before this change the tar.gz path +// happily installed "mcpproxy" and the zip path installed whatever came first, +// so a layout change silently swapped the wrong file into place. +func TestApp_SelfUpdate_RefusesArchiveWithoutTrayBinary(t *testing.T) { + tests := []struct { + name string + assetName string + archive []byte + }{ + { + name: "tar.gz with only the core binary", + assetName: "mcpproxy-latest-darwin-arm64" + assetTarGzExt, + archive: buildTarGzMembers(t, + archiveMember{"mcpproxy", []byte("core")}, + archiveMember{"README.md", []byte("docs")}, + ), + }, + { + name: "zip with only the core binary", + assetName: "mcpproxy-latest-windows-amd64" + assetZipExt, + archive: buildZipMembers(t, + archiveMember{"mcpproxy.exe", []byte("core")}, + archiveMember{"README.md", []byte("docs")}, + ), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + release, url := serveVerifiedAsset(t, tt.assetName, tt.archive) + + var applied atomic.Int32 + app := New(NewMockServer(), zaptest.NewLogger(t).Sugar(), "1.0.0", func() {}) + app.applyUpdateFn = func(r io.Reader, _ update.Options) error { + _, _ = io.Copy(io.Discard, r) + applied.Add(1) + return nil + } + + err := app.downloadAndApplyUpdate(release, tt.assetName, url) + if err == nil { + t.Fatalf("expected a refusal: the archive has no %s member", trayBinaryName()) + } + if !strings.Contains(err.Error(), trayBinaryName()) { + t.Errorf("error = %q, want it to name the missing member %q", err.Error(), trayBinaryName()) + } + if got := applied.Load(); got != 0 { + t.Errorf("applyUpdate called %d times, want 0 — nothing must be installed when the tray binary is absent", got) + } + }) + } +} + +// TestApp_SelfUpdate_MatchesNestedMemberOnBaseName: archives currently store +// members at the root, but matching on base name means a future layout that +// nests them under a directory keeps working — and, crucially, that a member +// named "not-mcpproxy-tray" does NOT satisfy the match the way HasSuffix did. +func TestApp_SelfUpdate_MatchesNestedMemberOnBaseName(t *testing.T) { + const trayPayload = "NESTED TRAY BINARY" + assetName := "mcpproxy-latest-darwin-arm64" + assetTarGzExt + + archive := buildTarGzMembers(t, + archiveMember{"mcpproxy-" + trayBinaryName(), []byte("DECOY: suffix-matches but is not the tray binary")}, + archiveMember{"mcpproxy-0.68.0-darwin-arm64/" + trayBinaryName(), []byte(trayPayload)}, + ) + release, url := serveVerifiedAsset(t, assetName, archive) + + var gotPayload string + app := New(NewMockServer(), zaptest.NewLogger(t).Sugar(), "1.0.0", func() {}) + app.applyUpdateFn = func(r io.Reader, _ update.Options) error { + b, err := io.ReadAll(r) + if err != nil { + return err + } + gotPayload = string(b) + return nil + } + + if err := app.downloadAndApplyUpdate(release, assetName, url); err != nil { + t.Fatalf("downloadAndApplyUpdate: %v", err) + } + if gotPayload != trayPayload { + t.Errorf("installed %q, want %q (base-name match, not suffix match)", gotPayload, trayPayload) + } +} + +// TestTrayBinaryName pins the member name to the binary the release workflow +// actually ships (release.yml copies mcpproxy-tray / mcpproxy-tray.exe into the +// archive stage), and to the name of the executable this package runs as. +func TestTrayBinaryName(t *testing.T) { + want := "mcpproxy-tray" + if runtime.GOOS == osWindows { + want = "mcpproxy-tray.exe" + } + if got := trayBinaryName(); got != want { + t.Errorf("trayBinaryName() = %q, want %q", got, want) + } + if base := filepath.Base(want); base != want { + t.Errorf("trayBinaryName() must be a bare base name, got %q", want) + } +} diff --git a/internal/tray/update_verify_test.go b/internal/tray/update_verify_test.go index 59523c123..6dfafbee3 100644 --- a/internal/tray/update_verify_test.go +++ b/internal/tray/update_verify_test.go @@ -89,9 +89,12 @@ func TestApp_SelfUpdate_VerifiesTheAssetItActuallySelected(t *testing.T) { aliasName := fmt.Sprintf("mcpproxy-latest-%s-%s%s", runtime.GOOS, runtime.GOARCH, ext) versionedName := fmt.Sprintf("mcpproxy-9.9.9-%s-%s%s", runtime.GOOS, runtime.GOARCH, ext) - archive := buildTarGz(t, "mcpproxy", []byte("pretend core binary")) + // The member is the TRAY binary: that is what this process installs over + // itself (see trayBinaryName). The name is the running platform's, not the + // asset's, because trayBinaryName() answers for the running process. + archive := buildTarGz(t, trayBinaryName(), []byte("pretend tray binary")) if ext == assetZipExt { - archive = buildZip(t, "mcpproxy", []byte("pretend core binary")) + archive = buildZip(t, trayBinaryName(), []byte("pretend tray binary")) } mux := http.NewServeMux() @@ -147,8 +150,11 @@ func TestApp_SelfUpdate_VerifiesTheAssetItActuallySelected(t *testing.T) { // exact asset name with a digest matching the downloaded bytes. Everything else // must fail closed. func TestApp_DownloadAndApplyUpdate_ChecksumGate(t *testing.T) { - tarGz := buildTarGz(t, "mcpproxy", []byte("pretend core binary")) - zipped := buildZip(t, "mcpproxy", []byte("pretend core binary")) + // Both fixtures carry the tray binary, the member the self-update path + // installs (trayBinaryName); this test is about the checksum gate, and an + // archive missing that member would fail for the wrong reason. + tarGz := buildTarGz(t, trayBinaryName(), []byte("pretend tray binary")) + zipped := buildZip(t, trayBinaryName(), []byte("pretend tray binary")) tests := []struct { name string diff --git a/internal/updatecheck/extract.go b/internal/updatecheck/extract.go new file mode 100644 index 000000000..3ccfd1d16 --- /dev/null +++ b/internal/updatecheck/extract.go @@ -0,0 +1,114 @@ +package updatecheck + +import ( + "archive/tar" + "archive/zip" + "compress/gzip" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "strings" +) + +// extract.go holds release-archive extraction, shared by `mcpproxy update` +// (cmd/mcpproxy) and the tray's self-update path (internal/tray). Both install +// a binary out of the same published archive, and both must name the member +// they want exactly: a release .tar.gz/.zip ships the core binary AND the tray +// binary side by side, so "the first entry" or "something ending in mcpproxy" +// picks the wrong one and swaps the wrong file into place. + +// MaxArchiveMemberBytes bounds a single extracted archive member. The core +// binary is ~60-90 MB; the cap exists so a malicious archive cannot fill the +// disk before the checksum comparison would have rejected it. +const MaxArchiveMemberBytes = 512 << 20 + +// ExtractBinary pulls the single archive member named memberName (matched on +// base name, so a future archive that nests files still works) into destPath, +// which is created with mode 0o700 — the caller re-applies the real mode when +// swapping it into place. An archive that does not contain memberName is an +// error: callers install a specific binary, never "whatever was in there". +func ExtractBinary(archivePath, memberName, destPath string) error { + switch { + case strings.HasSuffix(archivePath, ".zip"): + return extractFromZip(archivePath, memberName, destPath) + case strings.HasSuffix(archivePath, ".tar.gz"), strings.HasSuffix(archivePath, ".tgz"): + return extractFromTarGz(archivePath, memberName, destPath) + default: + return fmt.Errorf("unsupported archive format: %s", filepath.Base(archivePath)) + } +} + +func extractFromTarGz(archivePath, memberName, destPath string) error { + f, err := os.Open(archivePath) // #nosec G304 -- self-downloaded temp file + if err != nil { + return fmt.Errorf("open archive: %w", err) + } + defer f.Close() + + gz, err := gzip.NewReader(f) + if err != nil { + return fmt.Errorf("open gzip stream: %w", err) + } + defer gz.Close() + + tr := tar.NewReader(gz) + for { + hdr, err := tr.Next() + if errors.Is(err, io.EOF) { + break + } + if err != nil { + return fmt.Errorf("read archive: %w", err) + } + if hdr.Typeflag != tar.TypeReg || filepath.Base(hdr.Name) != memberName { + continue + } + return writeMember(tr, destPath) + } + return fmt.Errorf("archive does not contain %q", memberName) +} + +func extractFromZip(archivePath, memberName, destPath string) error { + zr, err := zip.OpenReader(archivePath) + if err != nil { + return fmt.Errorf("open archive: %w", err) + } + defer zr.Close() + + for _, entry := range zr.File { + if entry.FileInfo().IsDir() || filepath.Base(entry.Name) != memberName { + continue + } + rc, err := entry.Open() + if err != nil { + return fmt.Errorf("open archive member: %w", err) + } + defer rc.Close() + return writeMember(rc, destPath) + } + return fmt.Errorf("archive does not contain %q", memberName) +} + +// writeMember copies at most MaxArchiveMemberBytes from r into destPath. +func writeMember(r io.Reader, destPath string) error { + out, err := os.OpenFile(destPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC|os.O_EXCL, 0o700) // #nosec G304 -- destPath is our own temp path + if err != nil { + return fmt.Errorf("create staged binary: %w", err) + } + written, err := io.Copy(out, io.LimitReader(r, MaxArchiveMemberBytes+1)) + if err != nil { + out.Close() + return fmt.Errorf("write staged binary: %w", err) + } + if written > MaxArchiveMemberBytes { + out.Close() + return fmt.Errorf("archive member exceeds the %d-byte limit", int64(MaxArchiveMemberBytes)) + } + if err := out.Sync(); err != nil { + out.Close() + return fmt.Errorf("flush staged binary: %w", err) + } + return out.Close() +} From 35b0fc595443203ee55fdc0186e981cdd7112941 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Tue, 22 Sep 2026 18:49:08 +0300 Subject: [PATCH 2/6] ci(prerelease): ship the tray binary in RC archives Round 1 of cross-model review: the tray self-update now extracts the mcpproxy-tray member by name and fails closed when it is absent, but prerelease.yml archived only ${CLEAN_BINARY}. RC builds track the rc channel by construction (App.includePrereleases), so every prerelease tray would have hit 'archive does not contain mcpproxy-tray'. Mirrors what release.yml already does for stable archives. --- .github/workflows/prerelease.yml | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/.github/workflows/prerelease.yml b/.github/workflows/prerelease.yml index 88259dd80..1e39d2f1b 100644 --- a/.github/workflows/prerelease.yml +++ b/.github/workflows/prerelease.yml @@ -424,17 +424,33 @@ jobs: # Create archive with version info - DO NOT create "latest" archives for prereleases ARCHIVE_BASE="mcpproxy-${VERSION#v}-${{ matrix.goos }}-${{ matrix.goarch }}" + # The tray binary ships alongside the core, exactly as release.yml + # does it. The tray's self-update extracts the mcpproxy-tray member + # from this archive by name and fails closed when it is absent, so an + # RC archive without it leaves every prerelease tray unable to update + # (RC builds track the rc channel by construction — see + # App.includePrereleases in internal/tray/tray.go). + FILES_TO_ARCHIVE="${CLEAN_BINARY}" + if [ "${{ matrix.goos }}" = "windows" ] && [ -f "mcpproxy-tray.exe" ]; then + FILES_TO_ARCHIVE="${FILES_TO_ARCHIVE} mcpproxy-tray.exe" + echo "Including mcpproxy-tray.exe in archive" + elif [ "${{ matrix.goos }}" = "darwin" ] && [ -f "mcpproxy-tray" ]; then + FILES_TO_ARCHIVE="${FILES_TO_ARCHIVE} mcpproxy-tray" + echo "Including mcpproxy-tray in archive" + fi + if [ "${{ matrix.archive_format }}" = "zip" ]; then # Create only versioned archive (no latest for prereleases) if [ "${{ matrix.goos }}" = "windows" ]; then # Use PowerShell Compress-Archive on Windows since zip command isn't available - powershell -Command "Compress-Archive -Path '${CLEAN_BINARY}' -DestinationPath '${ARCHIVE_BASE}.zip'" + PS_FILES=$(echo ${FILES_TO_ARCHIVE} | sed 's/ /,/g') + powershell -Command "Compress-Archive -Path ${PS_FILES} -DestinationPath '${ARCHIVE_BASE}.zip'" else - zip "${ARCHIVE_BASE}.zip" ${CLEAN_BINARY} + zip "${ARCHIVE_BASE}.zip" ${FILES_TO_ARCHIVE} fi else # Create only versioned archive (no latest for prereleases) - tar -czf "${ARCHIVE_BASE}.tar.gz" ${CLEAN_BINARY} + tar -czf "${ARCHIVE_BASE}.tar.gz" ${FILES_TO_ARCHIVE} fi - name: Build Linux .deb and .rpm packages From 743488bd550b7b3514cf70dc2b35e989029a8a00 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Tue, 22 Sep 2026 18:55:57 +0300 Subject: [PATCH 3/6] ci(prerelease): build the Windows tray before archiving MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 of cross-model review. prerelease.yml built mcpproxy-tray for darwin only, so the windows branch of the archive step could never fire. The Windows RC tray does reach users — build-windows-installer.ps1 builds it, but that runs AFTER the archives — and it self-updates out of this job's .zip, so the zip must carry the mcpproxy-tray.exe member. Matches release.yml, including the archived tray being unsigned (SignPath signs installers, not archive members). --- .github/workflows/prerelease.yml | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/.github/workflows/prerelease.yml b/.github/workflows/prerelease.yml index 1e39d2f1b..056d23dcd 100644 --- a/.github/workflows/prerelease.yml +++ b/.github/workflows/prerelease.yml @@ -235,10 +235,20 @@ jobs: # it is not made here. go build -ldflags "${LDFLAGS}" -o ${CLEAN_BINARY} ./cmd/mcpproxy - # Build tray binary for macOS + # Build tray binary for the platforms with GUI support, matching + # release.yml. Windows matters here even though the RC tray reaches + # users through the Inno installer (built further below, AFTER the + # archives): that installed tray self-updates by downloading this + # job's .zip and extracting the mcpproxy-tray.exe member by name, so + # a zip built without it leaves every Windows RC tray unable to + # update. As in release.yml, the archived tray is unsigned — SignPath + # signs installers, not archive members. if [ "${{ matrix.goos }}" = "darwin" ]; then echo "Building mcpproxy-tray for macOS..." go build -ldflags "${LDFLAGS}" -o mcpproxy-tray ./cmd/mcpproxy-tray + elif [ "${{ matrix.goos }}" = "windows" ]; then + echo "Building mcpproxy-tray.exe for Windows..." + go build -ldflags "${LDFLAGS}" -o mcpproxy-tray.exe ./cmd/mcpproxy-tray fi # Build Swift tray app (macOS only — replaces Go tray in .app bundle for DMG/PKG) From 73b1385f31374cec1a9b957eaefb51eb7fbca79c Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Tue, 22 Sep 2026 18:59:05 +0300 Subject: [PATCH 4/6] ci: link the archived Windows tray with -H windowsgui Round 3 of cross-model review. scripts/build-windows-installer.ps1 has always passed -H windowsgui, but the tray copy placed in the release/prerelease archives did not, so it links as a console executable and pops a console window on launch. This was latent until now: the tray self-update used to install the CORE binary over the tray, so the archived tray was never actually installed. Now that it selects the mcpproxy-tray member correctly, a Windows self-update would have swapped the installer's GUI binary for a console one. --- .github/workflows/prerelease.yml | 6 +++++- .github/workflows/release.yml | 11 +++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/.github/workflows/prerelease.yml b/.github/workflows/prerelease.yml index 056d23dcd..4203e465a 100644 --- a/.github/workflows/prerelease.yml +++ b/.github/workflows/prerelease.yml @@ -248,7 +248,11 @@ jobs: go build -ldflags "${LDFLAGS}" -o mcpproxy-tray ./cmd/mcpproxy-tray elif [ "${{ matrix.goos }}" = "windows" ]; then echo "Building mcpproxy-tray.exe for Windows..." - go build -ldflags "${LDFLAGS}" -o mcpproxy-tray.exe ./cmd/mcpproxy-tray + # -H windowsgui matches scripts/build-windows-installer.ps1: the + # tray must link as a GUI executable or it opens a console window + # on launch, and a self-update installs this copy over the + # installer's. + go build -ldflags "${LDFLAGS} -H windowsgui" -o mcpproxy-tray.exe ./cmd/mcpproxy-tray fi # Build Swift tray app (macOS only — replaces Go tray in .app bundle for DMG/PKG) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 30240ab7b..85a0e9160 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -449,14 +449,21 @@ jobs: if [ "$EDITION" != "server" ] && { [ "${{ matrix.goos }}" = "darwin" ] || [ "${{ matrix.goos }}" = "windows" ]; }; then echo "Building mcpproxy-tray for ${{ matrix.goos }}..." - # Determine tray binary name + # Determine tray binary name, and on Windows link it as a GUI + # (not console) executable. Without -H windowsgui the tray pops a + # console window on every launch; scripts/build-windows-installer.ps1 + # has always passed it, and the archived copy must match, because a + # tray self-update extracts THIS binary and installs it over the + # installer's copy (internal/tray applyArchiveUpdate). + TRAY_LDFLAGS="${LDFLAGS}" if [ "${{ matrix.goos }}" = "windows" ]; then TRAY_BINARY="mcpproxy-tray.exe" + TRAY_LDFLAGS="${LDFLAGS} -H windowsgui" else TRAY_BINARY="mcpproxy-tray" fi - go build -ldflags "${LDFLAGS}" -o ${TRAY_BINARY} ./cmd/mcpproxy-tray + go build -ldflags "${TRAY_LDFLAGS}" -o ${TRAY_BINARY} ./cmd/mcpproxy-tray fi # Build Swift tray app (macOS only — replaces Go tray in .app bundle for DMG/PKG) From ad693dd34d948719dfa69171affa351ee89860ab Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Tue, 22 Sep 2026 19:08:48 +0300 Subject: [PATCH 5/6] test(tray): pin the exact archive member; guard against tray-less archives MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 of cross-model review (zcode), test-rigour findings: - TestApp_SelfUpdate_InstallsTheTrayBinaryNotTheCore gave both tray members the same payload, so a selection that ignored runtime.GOOS still passed. Members now carry payloads naming them, and the expectation is an independent literal rather than payloadFor(trayBinaryName()) — deriving it from the function under test made the assertion move with any bug in it. - The nested base-name case covered tar.gz only; zip is a separate code path and now has its own subtest. - The workflows' [ -f mcpproxy-tray ] gate degraded silently to shipping a tray-less archive. Since the self-update fails closed without that member, a missing binary now fails the build in both release.yml and prerelease.yml. --- .github/workflows/prerelease.yml | 12 ++++ .github/workflows/release.yml | 9 +++ internal/tray/update_extract_test.go | 98 +++++++++++++++++----------- 3 files changed, 81 insertions(+), 38 deletions(-) diff --git a/.github/workflows/prerelease.yml b/.github/workflows/prerelease.yml index 4203e465a..404bd267c 100644 --- a/.github/workflows/prerelease.yml +++ b/.github/workflows/prerelease.yml @@ -255,6 +255,18 @@ jobs: go build -ldflags "${LDFLAGS} -H windowsgui" -o mcpproxy-tray.exe ./cmd/mcpproxy-tray fi + # Same guard as release.yml: the archive adds the tray by name and + # the tray self-update fails closed without it, so a missing binary + # must fail the build rather than ship a tray-less archive. + if [ "${{ matrix.goos }}" = "darwin" ] && [ ! -f "mcpproxy-tray" ]; then + echo "::error::mcpproxy-tray build produced no binary; the archive would strand every tray self-update" + exit 1 + fi + if [ "${{ matrix.goos }}" = "windows" ] && [ ! -f "mcpproxy-tray.exe" ]; then + echo "::error::mcpproxy-tray.exe build produced no binary; the archive would strand every tray self-update" + exit 1 + fi + # Build Swift tray app (macOS only — replaces Go tray in .app bundle for DMG/PKG) if [ "${{ matrix.goos }}" = "darwin" ]; then chmod +x scripts/build-swift-app.sh diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 85a0e9160..21c9b3671 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -464,6 +464,15 @@ jobs: fi go build -ldflags "${TRAY_LDFLAGS}" -o ${TRAY_BINARY} ./cmd/mcpproxy-tray + + # The archive step below adds this binary by name, and the tray's + # self-update extracts that exact member and fails closed when it + # is missing. A silently tray-less archive would therefore strand + # every tray on this platform, so fail the build instead. + if [ ! -f "${TRAY_BINARY}" ]; then + echo "::error::mcpproxy-tray build produced no ${TRAY_BINARY}; the archive would strand every tray self-update" + exit 1 + fi fi # Build Swift tray app (macOS only — replaces Go tray in .app bundle for DMG/PKG) diff --git a/internal/tray/update_extract_test.go b/internal/tray/update_extract_test.go index 322ac5255..01282fd0f 100644 --- a/internal/tray/update_extract_test.go +++ b/internal/tray/update_extract_test.go @@ -110,10 +110,11 @@ func serveVerifiedAsset(t *testing.T, assetName string, archive []byte) (*GitHub // .github/workflows/release.yml produces, so a "first entry wins" or a // HasSuffix("mcpproxy") rule picks the wrong file. func TestApp_SelfUpdate_InstallsTheTrayBinaryNotTheCore(t *testing.T) { - const ( - corePayload = "PRETEND CORE BINARY" - trayPayload = "PRETEND TRAY BINARY" - ) + // Every member carries a payload that NAMES it, so the assertion pins the + // exact member extracted rather than merely "something tray-shaped": if + // the selection ever stopped honouring runtime.GOOS, an archive whose two + // tray members shared one payload would still pass. + payloadFor := func(member string) string { return "BINARY:" + member } tests := []struct { name string @@ -126,18 +127,18 @@ func TestApp_SelfUpdate_InstallsTheTrayBinaryNotTheCore(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - coreName, trayName := "mcpproxy", "mcpproxy-tray" + coreName := "mcpproxy" if strings.HasSuffix(tt.assetName, assetZipExt) { - coreName, trayName = "mcpproxy.exe", "mcpproxy-tray.exe" + coreName = "mcpproxy.exe" } - // The archive always ships BOTH members under their canonical - // names, whatever this test happens to run on; trayBinaryName() - // is what decides which one the running tray installs. + // The core is listed FIRST, as .github/workflows/release.yml + // produces it, so a "first entry wins" or HasSuffix("mcpproxy") + // rule picks it. Both tray spellings are present with distinct + // payloads; trayBinaryName() decides which one this build takes. archive := tt.archive(t, - archiveMember{coreName, []byte(corePayload)}, - archiveMember{trayName, []byte(trayPayload)}, - archiveMember{"mcpproxy-tray", []byte(trayPayload)}, - archiveMember{"mcpproxy-tray.exe", []byte(trayPayload)}, + archiveMember{coreName, []byte(payloadFor(coreName))}, + archiveMember{"mcpproxy-tray", []byte(payloadFor("mcpproxy-tray"))}, + archiveMember{"mcpproxy-tray.exe", []byte(payloadFor("mcpproxy-tray.exe"))}, ) release, url := serveVerifiedAsset(t, tt.assetName, archive) @@ -162,9 +163,16 @@ func TestApp_SelfUpdate_InstallsTheTrayBinaryNotTheCore(t *testing.T) { if got := applied.Load(); got != 1 { t.Fatalf("applyUpdate called %d times, want 1", got) } - if gotPayload != trayPayload { - t.Errorf("installed %q, want the tray binary %q — installing the core binary over %s bricks the tray", - gotPayload, trayPayload, trayBinaryName()) + // The expectation is an independent literal, NOT + // payloadFor(trayBinaryName()): deriving it from the function + // under test would make this assertion move with any bug in it. + wantMember := "mcpproxy-tray" + if runtime.GOOS == osWindows { + wantMember = "mcpproxy-tray.exe" + } + if want := payloadFor(wantMember); gotPayload != want { + t.Errorf("installed %q, want %q — this process replaces its own executable, so anything but %s bricks the tray", + gotPayload, want, wantMember) } exe, err := os.Executable() if err != nil { @@ -234,33 +242,47 @@ func TestApp_SelfUpdate_RefusesArchiveWithoutTrayBinary(t *testing.T) { // TestApp_SelfUpdate_MatchesNestedMemberOnBaseName: archives currently store // members at the root, but matching on base name means a future layout that // nests them under a directory keeps working — and, crucially, that a member -// named "not-mcpproxy-tray" does NOT satisfy the match the way HasSuffix did. +// named "mcpproxy-mcpproxy-tray" does NOT satisfy the match the way the old +// HasSuffix rule did. Both formats are covered: they are separate code paths. func TestApp_SelfUpdate_MatchesNestedMemberOnBaseName(t *testing.T) { const trayPayload = "NESTED TRAY BINARY" - assetName := "mcpproxy-latest-darwin-arm64" + assetTarGzExt - - archive := buildTarGzMembers(t, - archiveMember{"mcpproxy-" + trayBinaryName(), []byte("DECOY: suffix-matches but is not the tray binary")}, - archiveMember{"mcpproxy-0.68.0-darwin-arm64/" + trayBinaryName(), []byte(trayPayload)}, - ) - release, url := serveVerifiedAsset(t, assetName, archive) - var gotPayload string - app := New(NewMockServer(), zaptest.NewLogger(t).Sugar(), "1.0.0", func() {}) - app.applyUpdateFn = func(r io.Reader, _ update.Options) error { - b, err := io.ReadAll(r) - if err != nil { - return err - } - gotPayload = string(b) - return nil + tests := []struct { + name string + assetName string + build func(t *testing.T, members ...archiveMember) []byte + }{ + {"tar.gz", "mcpproxy-latest-darwin-arm64" + assetTarGzExt, buildTarGzMembers}, + {"zip", "mcpproxy-latest-windows-amd64" + assetZipExt, buildZipMembers}, } - if err := app.downloadAndApplyUpdate(release, assetName, url); err != nil { - t.Fatalf("downloadAndApplyUpdate: %v", err) - } - if gotPayload != trayPayload { - t.Errorf("installed %q, want %q (base-name match, not suffix match)", gotPayload, trayPayload) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + member := trayBinaryName() + archive := tt.build(t, + archiveMember{"mcpproxy-" + member, []byte("DECOY: suffix-matches but is not the tray binary")}, + archiveMember{"mcpproxy-0.68.0-darwin-arm64/" + member, []byte(trayPayload)}, + ) + release, url := serveVerifiedAsset(t, tt.assetName, archive) + + var gotPayload string + app := New(NewMockServer(), zaptest.NewLogger(t).Sugar(), "1.0.0", func() {}) + app.applyUpdateFn = func(r io.Reader, _ update.Options) error { + b, err := io.ReadAll(r) + if err != nil { + return err + } + gotPayload = string(b) + return nil + } + + if err := app.downloadAndApplyUpdate(release, tt.assetName, url); err != nil { + t.Fatalf("downloadAndApplyUpdate: %v", err) + } + if gotPayload != trayPayload { + t.Errorf("installed %q, want %q (base-name match, not suffix match)", gotPayload, trayPayload) + } + }) } } From 25094ed699db29b9f2e4b694e102bab00619c39c Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 23 Sep 2026 06:26:46 +0300 Subject: [PATCH 6/6] ci(tray): gate the tray package on PRs; fail on a core-only archive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4 of cross-model review (zcode), three findings: - internal/tray is behind '!nogui && !headless && !linux', and NOTHING in the PR gate compiles it: pr-build.yml tests with -tags nogui, and unit-tests.yml narrows its pull_request matrix to ubuntu-latest. The self-update regression tests therefore only ran on the push-to-main macOS/Windows legs — after merge. native-tests.yml gains a path-gated macOS go-tray-test job, following the workflow's existing required-safe pattern (a skipped job reports green, so non-tray PRs spend no macOS runner). - TestTrayBinaryName's base-name assertion tested the test's own literal, so filepath.Base(want) == want always held and the branch could never fire. It now checks the return value; verified by making trayBinaryName() return 'bin/mcpproxy-tray', which the fixed assertion catches. - The build-step guard added in the previous commit sat ~250 lines from the conditional that actually degrades silently, and only caught 'go build exited 0 but wrote no file'. It moves to the archive-assembly gate itself, as an else branch, so a future refactor splitting build from archive cannot reopen the original bug. Linux and the server edition fall through untouched - neither ships a tray. --- .github/workflows/native-tests.yml | 28 ++++++++++++++++++++++++++++ .github/workflows/prerelease.yml | 18 ++++++------------ .github/workflows/release.yml | 18 +++++++++--------- internal/tray/update_extract_test.go | 7 +++++-- 4 files changed, 48 insertions(+), 23 deletions(-) diff --git a/.github/workflows/native-tests.yml b/.github/workflows/native-tests.yml index 6146defdb..50193d14d 100644 --- a/.github/workflows/native-tests.yml +++ b/.github/workflows/native-tests.yml @@ -50,6 +50,7 @@ jobs: timeout-minutes: 5 outputs: native: ${{ steps.filter.outputs.native }} + gotray: ${{ steps.filter.outputs.gotray }} steps: - uses: actions/checkout@v7.0.1 - uses: dorny/paths-filter@v4 @@ -62,6 +63,11 @@ jobs: - 'frontend/src/components/settings/**' - 'scripts/check-settings-parity.py' - '.github/workflows/native-tests.yml' + gotray: + - 'internal/tray/**' + - 'cmd/mcpproxy-tray/**' + - 'internal/updatecheck/**' + - '.github/workflows/native-tests.yml' swift-test: name: swift-test @@ -82,6 +88,28 @@ jobs: # code path that looks green. run: swift test + go-tray-test: + name: go-tray-test + needs: changes + if: needs.changes.outputs.gotray == 'true' + runs-on: macos-latest + timeout-minutes: 15 + steps: + - uses: actions/checkout@v7.0.1 + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version: "1.26" + cache: true + # internal/tray is behind `!nogui && !headless && !linux`, so NOTHING in + # the PR gate compiles it: pr-build.yml tests with `-tags nogui`, and + # unit-tests.yml narrows its pull_request matrix to ubuntu-latest. Its + # tests — including the self-update archive-member regression tests — + # used to run only on the push-to-main macOS/Windows legs, i.e. after + # merge. This job is the PR-time gate for that package. No `-tags nogui`, + # and macOS so the `!linux` half of the tag is satisfied too. + - name: Test the tray package + run: go test -race ./internal/tray/... ./internal/updatecheck/... + settings-parity: name: settings-parity needs: changes diff --git a/.github/workflows/prerelease.yml b/.github/workflows/prerelease.yml index 404bd267c..7c9fd7e51 100644 --- a/.github/workflows/prerelease.yml +++ b/.github/workflows/prerelease.yml @@ -255,18 +255,6 @@ jobs: go build -ldflags "${LDFLAGS} -H windowsgui" -o mcpproxy-tray.exe ./cmd/mcpproxy-tray fi - # Same guard as release.yml: the archive adds the tray by name and - # the tray self-update fails closed without it, so a missing binary - # must fail the build rather than ship a tray-less archive. - if [ "${{ matrix.goos }}" = "darwin" ] && [ ! -f "mcpproxy-tray" ]; then - echo "::error::mcpproxy-tray build produced no binary; the archive would strand every tray self-update" - exit 1 - fi - if [ "${{ matrix.goos }}" = "windows" ] && [ ! -f "mcpproxy-tray.exe" ]; then - echo "::error::mcpproxy-tray.exe build produced no binary; the archive would strand every tray self-update" - exit 1 - fi - # Build Swift tray app (macOS only — replaces Go tray in .app bundle for DMG/PKG) if [ "${{ matrix.goos }}" = "darwin" ]; then chmod +x scripts/build-swift-app.sh @@ -463,6 +451,12 @@ jobs: elif [ "${{ matrix.goos }}" = "darwin" ] && [ -f "mcpproxy-tray" ]; then FILES_TO_ARCHIVE="${FILES_TO_ARCHIVE} mcpproxy-tray" echo "Including mcpproxy-tray in archive" + elif [ "${{ matrix.goos }}" = "darwin" ] || [ "${{ matrix.goos }}" = "windows" ]; then + # Same rule as release.yml: fail rather than ship a core-only + # archive that would strand every tray self-update. Linux has no + # tray and falls through untouched. + echo "::error::no tray binary to archive for ${{ matrix.goos }}; a core-only archive would strand every tray self-update" + exit 1 fi if [ "${{ matrix.archive_format }}" = "zip" ]; then diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 21c9b3671..3123a03f5 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -464,15 +464,6 @@ jobs: fi go build -ldflags "${TRAY_LDFLAGS}" -o ${TRAY_BINARY} ./cmd/mcpproxy-tray - - # The archive step below adds this binary by name, and the tray's - # self-update extracts that exact member and fails closed when it - # is missing. A silently tray-less archive would therefore strand - # every tray on this platform, so fail the build instead. - if [ ! -f "${TRAY_BINARY}" ]; then - echo "::error::mcpproxy-tray build produced no ${TRAY_BINARY}; the archive would strand every tray self-update" - exit 1 - fi fi # Build Swift tray app (macOS only — replaces Go tray in .app bundle for DMG/PKG) @@ -724,6 +715,15 @@ jobs: cp mcpproxy-tray "${TARBALL_STAGE}/" FILES_TO_ARCHIVE="${FILES_TO_ARCHIVE} mcpproxy-tray" echo "Including mcpproxy-tray in archive" + elif [ "$EDITION" != "server" ] && { [ "${{ matrix.goos }}" = "darwin" ] || [ "${{ matrix.goos }}" = "windows" ]; }; then + # The tray self-update extracts the mcpproxy-tray member by exact + # name and fails closed without it, so a tray-less archive strands + # every tray on this platform. This branch is the one that used to + # degrade silently: the conditions above simply did not fire and + # the archive shipped core-only. Linux and the server edition fall + # through untouched — neither ships a tray. + echo "::error::no tray binary to archive for ${{ matrix.goos }}; a core-only archive would strand every tray self-update" + exit 1 fi ARCHIVE_OUT="$(pwd)" diff --git a/internal/tray/update_extract_test.go b/internal/tray/update_extract_test.go index 01282fd0f..0760ae3a0 100644 --- a/internal/tray/update_extract_test.go +++ b/internal/tray/update_extract_test.go @@ -297,7 +297,10 @@ func TestTrayBinaryName(t *testing.T) { if got := trayBinaryName(); got != want { t.Errorf("trayBinaryName() = %q, want %q", got, want) } - if base := filepath.Base(want); base != want { - t.Errorf("trayBinaryName() must be a bare base name, got %q", want) + // Against the RETURN VALUE, not the literal above: ExtractBinary compares + // filepath.Base(member), so a name carrying a directory could never match + // anything. Checking `want` here (as this did originally) was a tautology. + if got := trayBinaryName(); filepath.Base(got) != got { + t.Errorf("trayBinaryName() = %q, want a bare base name with no path separator", got) } }