From d210af1ae81e4a5d4018cc128be58f6f3932c62c Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Tue, 22 Sep 2026 18:26:11 +0300 Subject: [PATCH] fix(tray): verify self-update artifacts against release checksums The tray's GitHub self-update path downloaded a release asset over http.Get and handed it straight to update.Apply with empty update.Options - no Checksum, no Signature, no PublicKey. Any response that reached that code was installed over the running binary. The file is behind `!nogui && !headless && !linux`, so this shipped on macOS non-bundle installs and on Windows (macOS .app bundles are refused earlier and use Sparkle, which does verify). Every release already publishes checksums.txt, a sha256sum manifest over all assets. The tray now fetches it from the same release, looks up the exact asset name it selected, and refuses to apply unless the SHA-256 of the downloaded archive matches. It fails closed: a release with no manifest, an unreachable or unparseable manifest, an asset the manifest does not list, and any digest mismatch all abort with a clear error and install nothing. The digest is taken over the downloaded archive, not over the extracted member, because that is what the manifest covers - update.Options.Checksum would hash the wrong bytes on both archive paths. The lookup uses the asset name actually downloaded: findAsset prefers the `mcpproxy-latest--` alias, whose digest differs from the versioned archive's in a real release, so rederiving a name would compare against the wrong entry. ParseChecksums/VerifyFileSHA256 move from package main to internal/updatecheck so both self-update paths share one parser rather than keeping a second copy of security-critical parsing; cmd/mcpproxy keeps thin wrappers. The parser follows the coreutils grammar strictly (digest, space, mode byte, then the name verbatim) and rejects ambiguity - conflicting duplicate names, an over-cap manifest that may have been truncated mid-line, escape sequences coreutils does not emit. Follow-up, not in this change: the release also publishes a cosign bundle over checksums.txt, which `mcpproxy update --self` verifies by shelling out to the cosign binary. Doing the same in the tray would abort on the many machines without cosign installed, so it needs its own decision. Co-Authored-By: Claude Opus 5 --- cmd/mcpproxy/update_apply.go | 58 +---- internal/tray/tray.go | 203 +++++++++++++---- internal/tray/update_verify_test.go | 293 +++++++++++++++++++++++++ internal/updatecheck/checksums.go | 168 ++++++++++++++ internal/updatecheck/checksums_test.go | 204 +++++++++++++++++ 5 files changed, 833 insertions(+), 93 deletions(-) create mode 100644 internal/tray/update_verify_test.go create mode 100644 internal/updatecheck/checksums.go create mode 100644 internal/updatecheck/checksums_test.go diff --git a/cmd/mcpproxy/update_apply.go b/cmd/mcpproxy/update_apply.go index 2a83ca49c..5ff01e5f9 100644 --- a/cmd/mcpproxy/update_apply.go +++ b/cmd/mcpproxy/update_apply.go @@ -5,8 +5,6 @@ import ( "archive/zip" "compress/gzip" "context" - "crypto/sha256" - "encoding/hex" "errors" "fmt" "io" @@ -15,6 +13,8 @@ import ( "path/filepath" "strings" "time" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/updatecheck" ) // update_apply.go holds the mechanical half of `mcpproxy update`'s self-update @@ -44,60 +44,16 @@ const ( backupSuffix = ".old" ) -// parseChecksums parses a sha256sum-format manifest (" " or -// " *") into name -> lowercase hex digest. Unparseable lines are -// skipped rather than failing the whole file: the manifest is generated by CI -// and a future extra line must not break verification of the entry we need. +// parseChecksums parses a sha256sum-format manifest into name -> hex digest. +// The implementation lives in internal/updatecheck so the tray's self-update +// path verifies artifacts with exactly the same parser. func parseChecksums(r io.Reader) (map[string]string, error) { - data, err := io.ReadAll(io.LimitReader(r, 4<<20)) - if err != nil { - return nil, fmt.Errorf("read checksums: %w", err) - } - out := map[string]string{} - for _, line := range strings.Split(string(data), "\n") { - line = strings.TrimSpace(line) - if line == "" || strings.HasPrefix(line, "#") { - continue - } - fields := strings.Fields(line) - if len(fields) < 2 { - continue - } - digest := strings.ToLower(fields[0]) - if len(digest) != 64 { - continue - } - if _, err := hex.DecodeString(digest); err != nil { - continue - } - name := strings.TrimPrefix(fields[len(fields)-1], "*") - name = strings.TrimPrefix(name, "./") - out[name] = digest - } - if len(out) == 0 { - return nil, errors.New("checksums manifest contains no usable entries") - } - return out, nil + return updatecheck.ParseChecksums(r) } // verifyFileSHA256 hard-fails unless path hashes to wantHex. func verifyFileSHA256(path, wantHex string) error { - f, err := os.Open(path) // #nosec G304 -- path is a file this process just downloaded into its own temp dir - if err != nil { - return fmt.Errorf("open downloaded artifact: %w", err) - } - defer f.Close() - - h := sha256.New() - if _, err := io.Copy(h, f); err != nil { - return fmt.Errorf("hash downloaded artifact: %w", err) - } - got := hex.EncodeToString(h.Sum(nil)) - if !strings.EqualFold(got, wantHex) { - return fmt.Errorf("checksum mismatch for %s: manifest says %s, download is %s (nothing was installed)", - filepath.Base(path), wantHex, got) - } - return nil + return updatecheck.VerifyFileSHA256(path, wantHex) } // extractBinary pulls the single archive member named memberName (matched on diff --git a/internal/tray/tray.go b/internal/tray/tray.go index 5161f6cbc..9d509423c 100644 --- a/internal/tray/tray.go +++ b/internal/tray/tray.go @@ -28,6 +28,7 @@ import ( internalRuntime "github.com/smart-mcp-proxy/mcpproxy-go/internal/runtime" "github.com/smart-mcp-proxy/mcpproxy-go/internal/server" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/updatecheck" // "github.com/smart-mcp-proxy/mcpproxy-go/internal/upstream/cli" // replaced by in-process OAuth ) @@ -147,6 +148,11 @@ type App struct { // nil so performSelfUpdate runs. selfUpdateFunc func() + // applyUpdateFn, when non-nil, replaces the real binary swap + // (update.Apply). Tests inject it to observe whether the self-update path + // would have installed an artifact; production leaves it nil. + applyUpdateFn func(io.Reader, update.Options) error + // Config path for opening from menu configPath string @@ -1121,13 +1127,13 @@ func (a *App) performSelfUpdate() { return } - downloadURL, err := a.findAssetURL(release) + assetName, downloadURL, err := a.findAsset(release) if err != nil { a.logger.Error("Failed to find asset for your system", zap.Error(err)) return } - if err := a.downloadAndApplyUpdate(downloadURL); err != nil { + if err := a.downloadAndApplyUpdate(release, assetName, downloadURL); err != nil { a.logger.Error("Update failed", zap.Error(err)) } } @@ -1196,11 +1202,20 @@ func (a *App) getLatestReleaseIncludingPrereleases() (*GitHubRelease, error) { return &releases[0], nil } -// findAssetURL finds the correct asset URL for the current system +// findAssetURL finds the correct asset URL for the current system. func (a *App) findAssetURL(release *GitHubRelease) (string, error) { + _, url, err := a.findAsset(release) + return url, err +} + +// findAsset resolves both the asset NAME and its download URL for the current +// system. The name is what checksums.txt keys its digests by, so callers must +// verify against the name actually selected here rather than rederiving one: +// the "latest-*" aliases and the versioned archives have different digests. +func (a *App) findAsset(release *GitHubRelease) (name, url string, err error) { // Check if this is a Homebrew installation to avoid conflicts if a.isHomebrewInstallation() { - return "", fmt.Errorf("auto-update disabled for Homebrew installations - use 'brew upgrade mcpproxy' instead") + return "", "", fmt.Errorf("auto-update disabled for Homebrew installations - use 'brew upgrade mcpproxy' instead") } // Determine file extension based on platform @@ -1216,7 +1231,7 @@ func (a *App) findAssetURL(release *GitHubRelease) (string, error) { latestSuffix := fmt.Sprintf("latest-%s-%s%s", runtime.GOOS, runtime.GOARCH, extension) for _, asset := range release.Assets { if strings.HasSuffix(asset.Name, latestSuffix) { - return asset.BrowserDownloadURL, nil + return asset.Name, asset.BrowserDownloadURL, nil } } @@ -1224,11 +1239,11 @@ func (a *App) findAssetURL(release *GitHubRelease) (string, error) { versionedSuffix := fmt.Sprintf("-%s-%s%s", runtime.GOOS, runtime.GOARCH, extension) for _, asset := range release.Assets { if strings.HasSuffix(asset.Name, versionedSuffix) { - return asset.BrowserDownloadURL, nil + return asset.Name, asset.BrowserDownloadURL, nil } } - return "", fmt.Errorf("no suitable asset found for %s-%s (tried %s and %s)", + return "", "", fmt.Errorf("no suitable asset found for %s-%s (tried %s and %s)", runtime.GOOS, runtime.GOARCH, latestSuffix, versionedSuffix) } @@ -1273,38 +1288,154 @@ func (a *App) isAppBundle() bool { return strings.Contains(execPath, ".app/Contents/MacOS/") } -// downloadAndApplyUpdate downloads and applies the update -func (a *App) downloadAndApplyUpdate(url string) error { - resp, err := http.Get(url) // #nosec G107 -- URL is from GitHub releases API which is trusted +// maxUpdateDownloadBytes bounds a single downloaded release artifact. The +// archives are ~30-90 MB; the cap exists so a hostile or broken response +// cannot fill the disk before verification would have rejected it. +const maxUpdateDownloadBytes = 512 << 20 + +// checksumsAssetName is the sha256sum-format manifest every release publishes +// beside its artifacts (.github/workflows/release.yml and prerelease.yml both +// generate it over every file in release-files/). +const checksumsAssetName = "checksums.txt" + +// applyUpdate performs the binary swap. Tests inject applyUpdateFn so the +// gating logic can be exercised without rewriting the test binary. +func (a *App) applyUpdate(r io.Reader, opts update.Options) error { + if a.applyUpdateFn != nil { + return a.applyUpdateFn(r, opts) + } + return update.Apply(r, opts) +} + +// downloadToFile fetches url into destPath, refusing any non-200 response. +func downloadToFile(url, destPath string) error { + resp, err := http.Get(url) // #nosec G107 -- URL comes from the GitHub releases API for this repo if err != nil { return err } defer resp.Body.Close() - if strings.HasSuffix(url, assetZipExt) { - return a.applyZipUpdate(resp.Body) - } else if strings.HasSuffix(url, assetTarGzExt) { - return a.applyTarGzUpdate(resp.Body) + if resp.StatusCode != http.StatusOK { + return fmt.Errorf("unexpected HTTP status %s", resp.Status) } - return update.Apply(resp.Body, update.Options{}) + f, err := os.Create(destPath) // #nosec G304 -- destPath is inside a temp dir this process just created + if err != nil { + return err + } + _, copyErr := io.Copy(f, io.LimitReader(resp.Body, maxUpdateDownloadBytes)) + closeErr := f.Close() + if copyErr != nil { + return copyErr + } + return closeErr } -// applyZipUpdate extracts and applies an update from a zip archive -func (a *App) applyZipUpdate(body io.Reader) error { - tmpfile, err := os.CreateTemp("", fmt.Sprintf("update-*%s", assetZipExt)) +// resolveAssetDigest fetches checksums.txt from the SAME release and returns +// the digest it publishes for assetName. Every failure mode - no manifest +// asset, an unreachable or unparseable manifest, an asset the manifest does not +// list - is an error, so the caller can never fall through to an unverified +// install. +// +// The lookup keys on the exact asset name that was selected for download: a +// release publishes both "mcpproxy-latest--" aliases and versioned +// archives, and on macOS their digests differ (notarization), so rederiving a +// name here instead of using the downloaded one would compare the wrong entry. +func (a *App) resolveAssetDigest(release *GitHubRelease, assetName, workDir string) (string, error) { + var checksumsURL string + for _, asset := range release.Assets { + if asset.Name == checksumsAssetName { + checksumsURL = asset.BrowserDownloadURL + break + } + } + if checksumsURL == "" { + return "", fmt.Errorf("release %s publishes no %s, so %s cannot be verified; refusing to install it", + release.TagName, checksumsAssetName, assetName) + } + + checksumsPath := filepath.Join(workDir, checksumsAssetName) + if err := downloadToFile(checksumsURL, checksumsPath); err != nil { + return "", fmt.Errorf("download %s for release %s: %w", checksumsAssetName, release.TagName, err) + } + + f, err := os.Open(checksumsPath) // #nosec G304 -- checksumsPath is inside a temp dir this process just created if err != nil { - return err + return "", fmt.Errorf("open %s: %w", checksumsAssetName, err) + } + defer f.Close() + + manifest, err := updatecheck.ParseChecksums(f) + if err != nil { + return "", fmt.Errorf("parse %s for release %s: %w", checksumsAssetName, release.TagName, err) } - defer os.Remove(tmpfile.Name()) - defer tmpfile.Close() - _, err = io.Copy(tmpfile, body) + digest, ok := manifest[assetName] + if !ok { + return "", fmt.Errorf("%s is not listed in %s for release %s; refusing to install an unlisted artifact", + assetName, checksumsAssetName, release.TagName) + } + return digest, nil +} + +// downloadAndApplyUpdate downloads the release asset and applies it, but only +// once the downloaded bytes match the SHA-256 that the release's own +// checksums.txt publishes for that exact asset. It fails closed: a missing +// manifest, a missing entry or any mismatch refuses the update rather than +// installing an unverified binary. +// +// The digest covers the ARCHIVE as published, so the hash is taken over the +// downloaded file before anything is extracted from it - update.Options.Checksum +// would instead hash the extracted member, which the manifest says nothing +// about. Signature verification of checksums.txt itself (the cosign bundle the +// release also publishes, as `mcpproxy update --self` verifies) is a follow-up; +// it needs the cosign binary, which a tray user's machine usually lacks. +func (a *App) downloadAndApplyUpdate(release *GitHubRelease, assetName, url string) error { + workDir, err := os.MkdirTemp("", "mcpproxy-tray-update-*") + if err != nil { + return fmt.Errorf("create work directory: %w", err) + } + defer os.RemoveAll(workDir) + + // Resolved before the ~90 MB download so an unverifiable release costs + // nothing, and so no code path can reach the apply without a digest. + wantDigest, err := a.resolveAssetDigest(release, assetName, workDir) if err != nil { return err } - r, err := zip.OpenReader(tmpfile.Name()) + archivePath := filepath.Join(workDir, "artifact") + if err := downloadToFile(url, archivePath); err != nil { + return fmt.Errorf("download %s: %w", assetName, err) + } + + if err := updatecheck.VerifyFileSHA256(archivePath, wantDigest); err != nil { + return fmt.Errorf("refusing to install %s from release %s: %w", assetName, release.TagName, err) + } + + a.logger.Info("Update artifact verified against release checksums", + zap.String("asset", assetName), + zap.String("release", release.TagName), + zap.String("sha256", wantDigest)) + + switch { + case strings.HasSuffix(assetName, assetZipExt): + return a.applyZipUpdate(archivePath) + case strings.HasSuffix(assetName, assetTarGzExt): + return a.applyTarGzUpdate(archivePath) + default: + f, err := os.Open(archivePath) // #nosec G304 -- archivePath is inside a temp dir this process just created + if err != nil { + return err + } + defer f.Close() + return a.applyUpdate(f, update.Options{}) + } +} + +// 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 } @@ -1324,7 +1455,7 @@ func (a *App) applyZipUpdate(body io.Reader) error { return err } - err = update.Apply(rc, update.Options{TargetPath: executablePath}) + err = a.applyUpdate(rc, update.Options{TargetPath: executablePath}) rc.Close() return err } @@ -1332,27 +1463,15 @@ func (a *App) applyZipUpdate(body io.Reader) error { return fmt.Errorf("no file found in zip archive to apply") } -// applyTarGzUpdate extracts and applies an update from a tar.gz archive -func (a *App) applyTarGzUpdate(body io.Reader) error { - // For tar.gz files, we need to extract and find the binary - tmpfile, err := os.CreateTemp("", fmt.Sprintf("update-*%s", assetTarGzExt)) +// 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 if err != nil { return err } - defer os.Remove(tmpfile.Name()) - defer tmpfile.Close() - - _, err = io.Copy(tmpfile, body) - if err != nil { - return err - } - - // Open the tar.gz file and extract the binary - if _, err := tmpfile.Seek(0, 0); err != nil { - return fmt.Errorf("failed to seek to beginning of file: %w", err) - } + defer f.Close() - gzr, err := gzip.NewReader(tmpfile) + gzr, err := gzip.NewReader(f) if err != nil { return err } @@ -1375,7 +1494,7 @@ func (a *App) applyTarGzUpdate(body io.Reader) error { return err } - return update.Apply(tr, update.Options{TargetPath: executablePath}) + return a.applyUpdate(tr, update.Options{TargetPath: executablePath}) } } diff --git a/internal/tray/update_verify_test.go b/internal/tray/update_verify_test.go new file mode 100644 index 000000000..59523c123 --- /dev/null +++ b/internal/tray/update_verify_test.go @@ -0,0 +1,293 @@ +//go:build !nogui && !headless && !linux + +package tray + +import ( + "archive/tar" + "archive/zip" + "bytes" + "compress/gzip" + "crypto/sha256" + "encoding/hex" + "fmt" + "io" + "net/http" + "net/http/httptest" + "runtime" + "strings" + "sync/atomic" + "testing" + + "github.com/inconshreveable/go-update" + "go.uber.org/zap/zaptest" +) + +// ghAsset mirrors the anonymous asset struct inside GitHubRelease so tests can +// build release fixtures without reaching for the network. +type ghAsset = struct { + Name string `json:"name"` + BrowserDownloadURL string `json:"browser_download_url"` +} + +const fakeDigest = "1111111111111111111111111111111111111111111111111111111111111111" + +func sha256Hex(b []byte) string { + sum := sha256.Sum256(b) + return hex.EncodeToString(sum[:]) +} + +// buildTarGz returns a .tar.gz containing one member with the given name. +func buildTarGz(t *testing.T, member string, payload []byte) []byte { + t.Helper() + var buf bytes.Buffer + gz := gzip.NewWriter(&buf) + tw := tar.NewWriter(gz) + if err := tw.WriteHeader(&tar.Header{Name: member, Mode: 0o755, Size: int64(len(payload))}); err != nil { + t.Fatalf("tar header: %v", err) + } + if _, err := tw.Write(payload); err != nil { + t.Fatalf("tar write: %v", 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() +} + +// buildZip returns a .zip containing one member with the given name. +func buildZip(t *testing.T, member string, payload []byte) []byte { + t.Helper() + var buf bytes.Buffer + zw := zip.NewWriter(&buf) + w, err := zw.Create(member) + if err != nil { + t.Fatalf("zip create: %v", err) + } + if _, err := w.Write(payload); err != nil { + t.Fatalf("zip write: %v", err) + } + if err := zw.Close(); err != nil { + t.Fatalf("zip close: %v", err) + } + return buf.Bytes() +} + +// TestApp_SelfUpdate_VerifiesTheAssetItActuallySelected walks the real +// selection path: findAsset prefers the "mcpproxy-latest-*" alias over the +// versioned archive, and in a real release those two assets have DIFFERENT +// digests (macOS notarization rewrites the versioned one). So a manifest that +// lists only the versioned name must refuse the alias download rather than +// verify it against the neighbouring entry. +func TestApp_SelfUpdate_VerifiesTheAssetItActuallySelected(t *testing.T) { + ext := assetTarGzExt + if runtime.GOOS == osWindows { + ext = assetZipExt + } + 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")) + if ext == assetZipExt { + archive = buildZip(t, "mcpproxy", []byte("pretend core binary")) + } + + 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) { + // Correct digest, but published under the versioned name only. + _, _ = io.WriteString(w, sha256Hex(archive)+" "+versionedName+"\n") + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + release := &GitHubRelease{ + TagName: "v9.9.9", + Assets: []ghAsset{ + {Name: versionedName, BrowserDownloadURL: srv.URL + "/asset"}, + {Name: aliasName, BrowserDownloadURL: srv.URL + "/asset"}, + {Name: "checksums.txt", BrowserDownloadURL: srv.URL + "/checksums.txt"}, + }, + } + + 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 + } + + assetName, url, err := app.findAsset(release) + if err != nil { + t.Fatalf("findAsset: %v", err) + } + if assetName != aliasName { + t.Fatalf("findAsset chose %q, want the %q alias (this test only means something if the alias wins)", assetName, aliasName) + } + + err = app.downloadAndApplyUpdate(release, assetName, url) + if err == nil { + t.Fatalf("expected refusal: %q is not listed in the manifest", aliasName) + } + if !strings.Contains(err.Error(), "not listed") { + t.Errorf("error = %q, want it to say the asset is not listed", err.Error()) + } + if got := applied.Load(); got != 0 { + t.Errorf("applyUpdate called %d times, want 0", got) + } +} + +// TestApp_DownloadAndApplyUpdate_ChecksumGate asserts the tray self-update path +// installs an artifact ONLY when checksums.txt from the same release lists that +// 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")) + + tests := []struct { + name string + // assetName is the release asset being installed. + assetName string + // archive is the bytes the server hands back for that asset. + archive []byte + // manifest is the checksums.txt body; empty means "serve a 500". + manifest string + // publishChecksums controls whether the release lists checksums.txt. + publishChecksums bool + wantApplied bool + wantErrContains string + }{ + { + name: "matching digest installs", + assetName: "mcpproxy-latest-darwin-arm64.tar.gz", + archive: tarGz, + manifest: sha256Hex(tarGz) + " mcpproxy-latest-darwin-arm64.tar.gz\n", + publishChecksums: true, + wantApplied: true, + }, + { + name: "digest mismatch refuses", + assetName: "mcpproxy-latest-darwin-arm64.tar.gz", + archive: tarGz, + manifest: fakeDigest + " mcpproxy-latest-darwin-arm64.tar.gz\n", + publishChecksums: true, + wantApplied: false, + wantErrContains: "checksum mismatch", + }, + { + name: "asset not listed in manifest refuses", + assetName: "mcpproxy-latest-darwin-arm64.tar.gz", + archive: tarGz, + // The manifest is well-formed but covers only other assets: the + // "latest-*" alias and the versioned archive have different digests, + // so a near-miss name must never satisfy the gate. + manifest: sha256Hex(tarGz) + " mcpproxy-0.68.0-darwin-arm64.tar.gz\n", + publishChecksums: true, + wantApplied: false, + wantErrContains: "not listed", + }, + { + name: "release without checksums.txt refuses", + assetName: "mcpproxy-latest-darwin-arm64.tar.gz", + archive: tarGz, + manifest: sha256Hex(tarGz) + " mcpproxy-latest-darwin-arm64.tar.gz\n", + publishChecksums: false, + wantApplied: false, + wantErrContains: "checksums.txt", + }, + { + name: "unreachable manifest refuses", + assetName: "mcpproxy-latest-darwin-arm64.tar.gz", + archive: tarGz, + manifest: "", // server answers 500 + publishChecksums: true, + wantApplied: false, + wantErrContains: "checksums.txt", + }, + { + name: "zip path matching digest installs", + assetName: "mcpproxy-latest-windows-amd64.zip", + archive: zipped, + manifest: sha256Hex(zipped) + " mcpproxy-latest-windows-amd64.zip\n", + publishChecksums: true, + wantApplied: true, + }, + { + name: "zip path digest mismatch refuses", + assetName: "mcpproxy-latest-windows-amd64.zip", + archive: zipped, + manifest: fakeDigest + " mcpproxy-latest-windows-amd64.zip\n", + publishChecksums: true, + wantApplied: false, + wantErrContains: "checksum mismatch", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/asset", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(tt.archive) + }) + mux.HandleFunc("/checksums.txt", func(w http.ResponseWriter, _ *http.Request) { + if tt.manifest == "" { + http.Error(w, "boom", http.StatusInternalServerError) + return + } + _, _ = io.WriteString(w, tt.manifest) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + release := &GitHubRelease{ + TagName: "v9.9.9", + Assets: []ghAsset{ + {Name: tt.assetName, BrowserDownloadURL: srv.URL + "/asset"}, + }, + } + if tt.publishChecksums { + release.Assets = append(release.Assets, ghAsset{ + Name: "checksums.txt", + BrowserDownloadURL: srv.URL + "/checksums.txt", + }) + } + + 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, srv.URL+"/asset") + + if tt.wantErrContains == "" { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + } else { + if err == nil { + t.Fatalf("expected the update to be refused, got nil error (applied=%d)", applied.Load()) + } + if !strings.Contains(err.Error(), tt.wantErrContains) { + t.Errorf("error = %q, want it to mention %q", err.Error(), tt.wantErrContains) + } + } + + wantCount := int32(0) + if tt.wantApplied { + wantCount = 1 + } + if got := applied.Load(); got != wantCount { + t.Errorf("applyUpdate called %d times, want %d (an unverified artifact must never be installed)", got, wantCount) + } + }) + } +} diff --git a/internal/updatecheck/checksums.go b/internal/updatecheck/checksums.go new file mode 100644 index 000000000..308b0b6f1 --- /dev/null +++ b/internal/updatecheck/checksums.go @@ -0,0 +1,168 @@ +package updatecheck + +import ( + "crypto/sha256" + "encoding/hex" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "strings" +) + +// checksums.go holds the release-artifact integrity primitives shared by every +// self-update path: `mcpproxy update --self` (cmd/mcpproxy) and the tray's +// GitHub self-update (internal/tray). They live here — a plain, build-tag-free +// package both can import — rather than being duplicated, because a second copy +// of security-critical parsing is a second thing to get wrong. + +// maxChecksumsManifestBytes bounds the manifest read. A release manifest lists +// a few dozen artifacts; the cap keeps a hostile response from being read into +// memory unbounded. Hitting the cap is an error rather than a silent truncation +// (see ParseChecksums): a manifest cut mid-line could otherwise be parsed as a +// shorter, different filename than the one it really names. +const maxChecksumsManifestBytes = 4 << 20 + +// digestHexLen is the length of a hex-encoded SHA-256 digest. +const digestHexLen = 64 + +// ParseChecksums parses a sha256sum-format manifest into name -> lowercase hex +// digest. The grammar is the one coreutils writes and `sha256sum -c` reads: +// +// <64-hex> +// +// where is ' ' (text) or '*' (binary) and is every remaining +// byte of the line, verbatim. The name is taken as-is rather than tokenised, +// because everything after those two separator bytes is significant: a name may +// contain spaces, may begin with '*', and may begin with "./". Treating any of +// those as syntax would file an entry under a name the manifest never published +// — i.e. hand back a digest for a different file than the one asked about. +// +// A name containing a backslash, newline or carriage return is written by +// coreutils in escaped form, marked by a backslash before the digest. +// +// Unparseable lines are skipped rather than failing the whole file: the +// manifest is generated by CI and a future extra line must not break +// verification of the entry we need. Three things are NOT tolerated, because +// each could make a name resolve to a digest published for some other file: +// +// - a manifest with no usable entries (never read as "nothing to verify"), +// - the same name listed twice with different digests, +// - a manifest large enough to have been truncated by the read cap. +func ParseChecksums(r io.Reader) (map[string]string, error) { + data, err := io.ReadAll(io.LimitReader(r, maxChecksumsManifestBytes+1)) + if err != nil { + return nil, fmt.Errorf("read checksums: %w", err) + } + if len(data) > maxChecksumsManifestBytes { + return nil, fmt.Errorf("checksums manifest exceeds %d bytes; refusing to parse a possibly truncated manifest", + maxChecksumsManifestBytes) + } + + out := map[string]string{} + for _, line := range strings.Split(string(data), "\n") { + // Strip only the CRLF carriage return: trailing spaces are part of the + // name as far as sha256sum is concerned. + line = strings.TrimSuffix(line, "\r") + if line == "" || strings.HasPrefix(line, "#") { + continue + } + + // coreutils marks an entry whose name needed escaping with a leading + // backslash before the digest. + escaped := strings.HasPrefix(line, `\`) + if escaped { + line = line[1:] + } + + // 64 digest bytes + the 2 separator bytes + at least one name byte. + if len(line) < digestHexLen+3 { + continue + } + digest := strings.ToLower(line[:digestHexLen]) + if _, err := hex.DecodeString(digest); err != nil { + continue + } + // The first separator byte is always a literal space; only the second + // one varies (text vs binary mode). Anything else is a line + // `sha256sum -c` would itself reject as improperly formatted. + if line[digestHexLen] != ' ' { + continue + } + if mode := line[digestHexLen+1]; mode != ' ' && mode != '*' { + continue + } + + name := line[digestHexLen+2:] + if escaped { + unescaped, ok := unescapeChecksumName(name) + if !ok { + // An escape sequence coreutils does not produce: the real name + // is unknown, so skip rather than guess at it. + continue + } + name = unescaped + } + + if prev, ok := out[name]; ok && prev != digest { + return nil, fmt.Errorf("checksums manifest lists %q twice with different digests (%s and %s); refusing to guess which one is authoritative", + name, prev, digest) + } + out[name] = digest + } + + if len(out) == 0 { + return nil, errors.New("checksums manifest contains no usable entries") + } + return out, nil +} + +// unescapeChecksumName reverses the coreutils escaping applied to names +// containing a backslash, newline or carriage return. It reports false for a +// sequence coreutils never emits (including a trailing lone backslash), so the +// caller can drop the line instead of inventing a name for it. +func unescapeChecksumName(name string) (string, bool) { + var b strings.Builder + for i := 0; i < len(name); i++ { + if name[i] != '\\' { + b.WriteByte(name[i]) + continue + } + if i+1 >= len(name) { + return "", false + } + i++ + switch name[i] { + case 'n': + b.WriteByte('\n') + case 'r': + b.WriteByte('\r') + case '\\': + b.WriteByte('\\') + default: + return "", false + } + } + return b.String(), true +} + +// VerifyFileSHA256 hard-fails unless path hashes to wantHex. +func VerifyFileSHA256(path, wantHex string) error { + f, err := os.Open(path) // #nosec G304 -- path is a file this process just downloaded into its own temp dir + if err != nil { + return fmt.Errorf("open downloaded artifact: %w", err) + } + defer f.Close() + + h := sha256.New() + if _, err := io.Copy(h, f); err != nil { + return fmt.Errorf("hash downloaded artifact: %w", err) + } + got := hex.EncodeToString(h.Sum(nil)) + if !strings.EqualFold(got, wantHex) { + return fmt.Errorf("checksum mismatch for %s: manifest says %s, download is %s (nothing was installed)", + filepath.Base(path), wantHex, got) + } + return nil +} diff --git a/internal/updatecheck/checksums_test.go b/internal/updatecheck/checksums_test.go new file mode 100644 index 000000000..275f30d57 --- /dev/null +++ b/internal/updatecheck/checksums_test.go @@ -0,0 +1,204 @@ +package updatecheck + +import ( + "strings" + "testing" +) + +const ( + digestA = "1111111111111111111111111111111111111111111111111111111111111111" + digestB = "2222222222222222222222222222222222222222222222222222222222222222" +) + +func TestParseChecksums_RealManifestShape(t *testing.T) { + manifest := strings.Join([]string{ + "# a comment line", + digestA + " mcpproxy-latest-darwin-arm64.tar.gz", + digestB + " *mcpproxy-latest-windows-amd64.zip", // binary-mode marker + "garbage line without a digest", + "deadbeef too-short-digest.txt", + "", + }, "\n") + + got, err := ParseChecksums(strings.NewReader(manifest)) + if err != nil { + t.Fatalf("ParseChecksums: %v", err) + } + if got["mcpproxy-latest-darwin-arm64.tar.gz"] != digestA { + t.Errorf("darwin entry = %q, want %q", got["mcpproxy-latest-darwin-arm64.tar.gz"], digestA) + } + if got["mcpproxy-latest-windows-amd64.zip"] != digestB { + t.Errorf("windows entry = %q (the '*' binary-mode marker must be stripped)", got["mcpproxy-latest-windows-amd64.zip"]) + } + if _, ok := got["too-short-digest.txt"]; ok { + t.Error("a malformed digest must be skipped, not accepted") + } + if len(got) != 2 { + t.Errorf("parsed %d entries, want 2: %v", len(got), got) + } +} + +// A name containing spaces must be keyed by the WHOLE name. Keying on the last +// whitespace-separated field would file "decoy mcpproxy-latest-darwin-arm64.tar.gz" +// under "mcpproxy-latest-darwin-arm64.tar.gz", i.e. report an entry for an asset +// the manifest never named. +func TestParseChecksums_NameWithSpacesIsNotMisattributed(t *testing.T) { + const target = "mcpproxy-latest-darwin-arm64.tar.gz" + manifest := digestA + " decoy " + target + "\n" + + got, err := ParseChecksums(strings.NewReader(manifest)) + if err != nil { + t.Fatalf("ParseChecksums: %v", err) + } + if _, ok := got[target]; ok { + t.Fatalf("%q must NOT be present: the manifest only names %q", target, "decoy "+target) + } + if got["decoy "+target] != digestA { + t.Errorf("entry = %v, want the full name %q to carry the digest", got, "decoy "+target) + } +} + +func TestParseChecksums_ConflictingDuplicateIsRefused(t *testing.T) { + manifest := digestA + " mcpproxy.tar.gz\n" + digestB + " mcpproxy.tar.gz\n" + + if _, err := ParseChecksums(strings.NewReader(manifest)); err == nil { + t.Fatal("two different digests for the same name must be an error, not last-one-wins") + } + + // An identical duplicate is unambiguous and stays acceptable. + same := digestA + " mcpproxy.tar.gz\n" + digestA + " mcpproxy.tar.gz\n" + got, err := ParseChecksums(strings.NewReader(same)) + if err != nil { + t.Fatalf("identical duplicate should parse: %v", err) + } + if got["mcpproxy.tar.gz"] != digestA { + t.Errorf("entry = %q, want %q", got["mcpproxy.tar.gz"], digestA) + } +} + +// Everything after the two separator bytes is the name, verbatim. A text-mode +// line whose name happens to start with '*' or "./" must NOT be filed under the +// stripped spelling: doing so would report a digest for an asset the manifest +// never named, letting those bytes satisfy verification for a different name. +func TestParseChecksums_NameIsVerbatimAfterTheSeparator(t *testing.T) { + const target = "mcpproxy-latest-darwin-arm64.tar.gz" + manifest := strings.Join([]string{ + digestA + " *" + target, // text mode, name really starts with '*' + digestB + " ./" + target, // text mode, name really starts with './' + "", + }, "\n") + + got, err := ParseChecksums(strings.NewReader(manifest)) + if err != nil { + t.Fatalf("ParseChecksums: %v", err) + } + if _, ok := got[target]; ok { + t.Errorf("%q must NOT be present: neither line names it", target) + } + if got["*"+target] != digestA { + t.Errorf("entries = %v, want %q under its verbatim name", got, "*"+target) + } + if got["./"+target] != digestB { + t.Errorf("entries = %v, want %q under its verbatim name", got, "./"+target) + } +} + +// Binary mode is the one place a '*' is syntax: " *" — a single +// separator space, then the mode character. +func TestParseChecksums_BinaryModeMarkerIsSyntax(t *testing.T) { + got, err := ParseChecksums(strings.NewReader(digestA + " *mcpproxy.zip\n")) + if err != nil { + t.Fatalf("ParseChecksums: %v", err) + } + if got["mcpproxy.zip"] != digestA { + t.Errorf("entries = %v, want mcpproxy.zip -> %s", got, digestA) + } +} + +// The separator coreutils emits is a literal space; a tab-separated line is +// one `sha256sum -c` rejects as improperly formatted, so it is not a checksum +// record and must not produce a name -> digest mapping. +func TestParseChecksums_TabSeparatorIsRejected(t *testing.T) { + manifest := digestA + "\t mcpproxy-latest-darwin-arm64.tar.gz\n" + digestB + " other.tar.gz\n" + + got, err := ParseChecksums(strings.NewReader(manifest)) + if err != nil { + t.Fatalf("ParseChecksums: %v", err) + } + if _, ok := got["mcpproxy-latest-darwin-arm64.tar.gz"]; ok { + t.Errorf("entries = %v, want the tab-separated line skipped", got) + } + if len(got) != 1 { + t.Errorf("parsed %d entries, want only the well-formed one: %v", len(got), got) + } +} + +func TestParseChecksums_CRLFManifest(t *testing.T) { + got, err := ParseChecksums(strings.NewReader(digestA + " mcpproxy.tar.gz\r\n")) + if err != nil { + t.Fatalf("ParseChecksums: %v", err) + } + if got["mcpproxy.tar.gz"] != digestA { + t.Errorf("entries = %v, want the CR stripped from the name", got) + } +} + +func TestParseChecksums_OversizedManifestIsRefused(t *testing.T) { + // A manifest at the read cap may have been cut mid-line, which could parse + // as a shorter (different) filename than the real one. + var b strings.Builder + b.WriteString(digestA + " real-asset.tar.gz\n") + for b.Len() <= maxChecksumsManifestBytes { + b.WriteString(digestB + " filler-asset-with-a-reasonably-long-name.tar.gz\n") + } + + if _, err := ParseChecksums(strings.NewReader(b.String())); err == nil { + t.Fatal("an over-cap manifest must be refused, not silently truncated") + } +} + +func TestParseChecksums_EmptyManifestIsAnError(t *testing.T) { + if _, err := ParseChecksums(strings.NewReader("# only comments\n")); err == nil { + t.Fatal("expected an error for a manifest with no usable entries") + } +} + +// coreutils writes a leading backslash before the digest when the name needed +// escaping, and escapes exactly backslash, newline and carriage return. +func TestParseChecksums_EscapedNames(t *testing.T) { + tests := []struct { + name string + line string + wantName string + wantSkip bool + }{ + {name: "backslash", line: `\` + digestA + ` weird\\name.tar.gz`, wantName: `weird\name.tar.gz`}, + {name: "newline", line: `\` + digestA + ` weird\nname.tar.gz`, wantName: "weird\nname.tar.gz"}, + {name: "carriage return", line: `\` + digestA + ` weird\rname.tar.gz`, wantName: "weird\rname.tar.gz"}, + // Sequences coreutils never emits: the real name is unknowable, so the + // line is dropped rather than decoded into a name nobody published. + {name: "unknown escape", line: `\` + digestA + ` weird\qname.tar.gz`, wantSkip: true}, + {name: "trailing backslash", line: `\` + digestA + ` weirdname.tar.gz\`, wantSkip: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // A second, ordinary entry keeps the manifest non-empty so a skipped + // line shows up as a missing key rather than a parse error. + manifest := tt.line + "\n" + digestB + " other.tar.gz\n" + got, err := ParseChecksums(strings.NewReader(manifest)) + if err != nil { + t.Fatalf("ParseChecksums: %v", err) + } + if tt.wantSkip { + if len(got) != 1 { + t.Fatalf("entries = %v, want only the ordinary entry", got) + } + return + } + if got[tt.wantName] != digestA { + t.Errorf("entries = %v, want %q -> %s", got, tt.wantName, digestA) + } + }) + } +}