Conversation
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-<os>-<arch>` 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 <noreply@anthropic.com>
Deploying mcpproxy-docs with
|
| Latest commit: |
d210af1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4c05b70e.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-verify-update-artifact-c.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 35747409072 --repo smart-mcp-proxy/mcpproxy-go
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The tray's GitHub self-update downloaded a release asset over
http.Getand passed it straight toupdate.Applywith emptyupdate.Options— noChecksum, noSignature, noPublicKey. Whatever came back on that connection was written over the running binary.internal/tray/tray.gois behind!nogui && !headless && !linux, so the exposed surface is macOS non-bundle, non-Homebrew installs and all Windows installs. (macOS.appbundles are refused earlier and update through Sparkle, which does verify EdDSA; Homebrew installs are refused infindAssetURL.)The tray path was effectively the un-hardened duplicate of an already-hardened flow:
mcpproxy update --self(cmd/mcpproxy/update_cmd.go) has verified artifacts againstchecksums.txtall along.How
Every release publishes
checksums.txt, asha256summanifest over all assets (.github/workflows/release.yml, andprerelease.ymlfor RCs). The tray now:checksums.txtfrom the same release,It fails closed — no manifest asset, an unreachable or unparseable manifest, an unlisted asset, or any mismatch aborts and installs nothing.
Two details that decide correctness:
update.Options.Checksumwould have hashed the extracted bytes on both archive paths — the wrong thing.findAssetprefers themcpproxy-latest-<os>-<arch>alias, and in a real release its digest differs from the versioned archive's (notarization). So the asset name is now carried from selection into the lookup instead of being rederived.ParseChecksums/VerifyFileSHA256move frompackage maintointernal/updatecheckso both self-update paths share one parser instead of a second copy of security-critical parsing;cmd/mcpproxykeeps thin wrappers. The parser follows the coreutils grammar strictly (digest, space, mode byte, name verbatim) and refuses ambiguity: conflicting duplicate names, an over-cap manifest that may have been cut mid-line, and escape sequences coreutils does not emit.Verified
go test ./internal/tray/... ./internal/updatecheck/... ./cmd/mcpproxy/... -racegreen;golangci-lintclean in both the bare and--build-tags serverpasses.mcpproxy-latest-darwin-arm64.tar.gzverifies and applies, and serving a different real asset under that name is refused with a digest mismatch. All 24 manifest entries parse.Follow-ups (not here)
checksums.txtitself. The release publishes the bundle andupdate --selfverifies it by shelling out tocosign; doing that in the tray would abort on the many machines without cosign, so it needs its own call. No new dependency either way.applyTarGzUpdatematches the archive member by suffixmcpproxy/mcpproxy.exeand writes it overos.Executable()— for the tray that ismcpproxy-tray, so a successful tray self-update installs the core binary over the tray binary.applyZipUpdateapplies the first non-dir zip entry regardless of name. Pre-existing, untouched here;cmd/mcpproxygets this right viacoreBinaryName().🤖 Generated with Claude Code