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 88259dd80..7c9fd7e51 100644 --- a/.github/workflows/prerelease.yml +++ b/.github/workflows/prerelease.yml @@ -235,10 +235,24 @@ 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..." + # -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) @@ -424,17 +438,39 @@ 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" + 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 # 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 diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 30240ab7b..3123a03f5 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) @@ -708,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/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..0760ae3a0 --- /dev/null +++ b/internal/tray/update_extract_test.go @@ -0,0 +1,306 @@ +//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) { + // 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 + 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 := "mcpproxy" + if strings.HasSuffix(tt.assetName, assetZipExt) { + coreName = "mcpproxy.exe" + } + // 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(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) + + 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) + } + // 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 { + 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 "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" + + 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}, + } + + 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) + } + }) + } +} + +// 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) + } + // 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) + } +} 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() +}