From 03e1daad7d57601ef0bcdb428e890fbe369bcf7a Mon Sep 17 00:00:00 2001 From: Musa Misto Date: Tue, 5 May 2026 16:35:51 +0300 Subject: [PATCH 1/5] feat(gong): verify SHA256 checksum of downloaded binary before installation - Add checksum.go with parseChecksumManifest, verifySHA256, isValidSHA256 - Wire verification into downloadGong between write and chmod/rename - Add baseURL/installDir test-hook fields on Checker (zero value = no behavior change) - Add 20 unit tests and 4 httptest integration tests (32 total) - Add checksum block to .goreleaser.yaml (name_template: checksums.txt, algorithm: sha256) - Document Binary Integrity in SECURITY.md --- .goreleaser.yaml | 4 + SECURITY.md | 4 + pkg/gong/checksum.go | 75 ++++++++++++ pkg/gong/checksum_test.go | 212 ++++++++++++++++++++++++++++++++++ pkg/gong/gong.go | 119 +++++++++++++------ pkg/gong/gong_install_test.go | 168 +++++++++++++++++++++++++++ pkg/gong/gong_test.go | 43 ++++++- 7 files changed, 590 insertions(+), 35 deletions(-) create mode 100644 pkg/gong/checksum.go create mode 100644 pkg/gong/checksum_test.go create mode 100644 pkg/gong/gong_install_test.go diff --git a/.goreleaser.yaml b/.goreleaser.yaml index fbc683b6a5..e5f506fcd3 100644 --- a/.goreleaser.yaml +++ b/.goreleaser.yaml @@ -103,6 +103,10 @@ archives: - goos: windows format: zip +checksum: + name_template: "checksums.txt" + algorithm: sha256 + changelog: sort: asc filters: diff --git a/SECURITY.md b/SECURITY.md index f816b695c7..7f1f65e462 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -4,6 +4,10 @@ Bruin is currently running a single major version, v0, which will continue receiving security updates as we go. +## Binary Integrity + +Bruin verifies downloaded gong helper binaries with SHA256 checksums before installation. If the checksum is missing or does not match the downloaded file, installation stops and the downloaded file is discarded. + ## Reporting a Vulnerability Please report security vulnerabilities via the email address `security@getbruin.com` diff --git a/pkg/gong/checksum.go b/pkg/gong/checksum.go new file mode 100644 index 0000000000..e9df993bac --- /dev/null +++ b/pkg/gong/checksum.go @@ -0,0 +1,75 @@ +package gong + +import ( + "crypto/sha256" + "encoding/hex" + "fmt" + "io" + "os" + "strings" +) + +// parseChecksumManifest parses a sha256sum-format checksum file and returns +// the expected SHA256 hex string for the given artifactName. +// +// The expected file format is one entry per line: +// +// +// +// Empty lines and lines starting with '#' are ignored. +func parseChecksumManifest(contents []byte, artifactName string) (string, error) { + lines := strings.Split(string(contents), "\n") + for _, line := range lines { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + parts := strings.Fields(line) + if len(parts) < 2 { + continue + } + checksum := parts[0] + filename := parts[1] + if filename != artifactName { + continue + } + if !isValidSHA256(checksum) { + return "", fmt.Errorf("invalid checksum for %s", artifactName) + } + return checksum, nil + } + return "", fmt.Errorf("checksum entry not found for %s", artifactName) +} + +// verifySHA256 computes the SHA256 digest of the file at path and compares it +// against the expected hex string. The comparison is case-insensitive so both +// uppercase and lowercase checksums are accepted. +func verifySHA256(path string, expected string) error { + if !isValidSHA256(expected) { + return fmt.Errorf("invalid expected checksum") + } + f, err := os.Open(path) + if err != nil { + return err + } + defer f.Close() + h := sha256.New() + if _, err := io.Copy(h, f); err != nil { + return err + } + actual := hex.EncodeToString(h.Sum(nil)) + if !strings.EqualFold(actual, expected) { + return fmt.Errorf("checksum mismatch: expected %s, got %s", expected, actual) + } + return nil +} + +// isValidSHA256 reports whether value is a valid SHA256 hex digest: +// exactly 64 hexadecimal characters (case-insensitive). +func isValidSHA256(value string) bool { + if len(value) != 64 { + return false + } + _, err := hex.DecodeString(value) + return err == nil +} diff --git a/pkg/gong/checksum_test.go b/pkg/gong/checksum_test.go new file mode 100644 index 0000000000..3d53842e7a --- /dev/null +++ b/pkg/gong/checksum_test.go @@ -0,0 +1,212 @@ +package gong + +import ( + "crypto/sha256" + "encoding/hex" + "fmt" + "os" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// ─── shared test fixtures ──────────────────────────────────────────────────── + +// knownHash is the real SHA256 of the string "test". +// We hardcode it so test expectations are trivially auditable without running +// any hash code in the test setup itself. +const knownHash = "9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08" + +// hashOf returns the lowercase hex SHA256 of b. +// Used to build expected values for verifySHA256 tests without +// duplicating the hash algorithm. +func hashOf(b []byte) string { + sum := sha256.Sum256(b) + return hex.EncodeToString(sum[:]) +} + +// tempFileWithContent writes content to a new temp file inside t.TempDir() +// and returns the file path. Cleanup is handled automatically by the test runner. +func tempFileWithContent(t *testing.T, content []byte) string { + t.Helper() + f, err := os.CreateTemp(t.TempDir(), "verify-sha256-*") + require.NoError(t, err) + _, err = f.Write(content) + require.NoError(t, err) + require.NoError(t, f.Close()) + return f.Name() +} + +// ─── parseChecksumManifest ─────────────────────────────────────────────────── + +func TestParseChecksumManifestReturnsMatchingChecksum(t *testing.T) { + t.Parallel() + artifactName := "gong_linux_amd64" + manifest := []byte(knownHash + " " + artifactName + "\n") + got, err := parseChecksumManifest(manifest, artifactName) + require.NoError(t, err) + assert.Equal(t, knownHash, got) +} + +func TestParseChecksumManifestIgnoresEmptyAndCommentLines(t *testing.T) { + t.Parallel() + artifactName := "gong_linux_amd64" + // Manifest deliberately padded with every kind of noise the spec calls out: + // blank lines, lines of only whitespace, and comment lines. + manifest := []byte(fmt.Sprintf( + "# generated by goreleaser\n"+ + "\n"+ + " \n"+ + "# sha256\n"+ + "%s %s\n"+ + "\n"+ + "# end\n", + knownHash, artifactName, + )) + got, err := parseChecksumManifest(manifest, artifactName) + require.NoError(t, err) + assert.Equal(t, knownHash, got) +} + +// TestParseChecksumManifestPicksCorrectEntryAmongMultiple verifies that when a +// manifest contains several valid entries the parser returns the checksum for +// the requested artifact only — not for any other artifact. +func TestParseChecksumManifestPicksCorrectEntryAmongMultiple(t *testing.T) { + t.Parallel() + arm64Hash := strings.Repeat("a", 64) + amd64Hash := knownHash + manifest := []byte(fmt.Sprintf( + "%s gong_linux_arm64\n"+ + "%s gong_linux_amd64\n", + arm64Hash, amd64Hash, + )) + got, err := parseChecksumManifest(manifest, "gong_linux_amd64") + require.NoError(t, err) + assert.Equal(t, amd64Hash, got) +} + +func TestParseChecksumManifestReturnsErrorWhenArtifactMissing(t *testing.T) { + t.Parallel() + // Manifest contains only arm64; requesting amd64 must fail. + manifest := []byte(knownHash + " gong_linux_arm64\n") + _, err := parseChecksumManifest(manifest, "gong_linux_amd64") + require.Error(t, err) + assert.Contains(t, err.Error(), "checksum entry not found for gong_linux_amd64") +} + +// TestParseChecksumManifestReturnsErrorForMalformedLine covers the case where +// the manifest holds a line with only one whitespace-separated token (no +// checksum column). The parser skips such lines — because a corrupt entry for +// an unrelated artifact must not block a valid one — so the artifact is never +// matched and "checksum entry not found" is returned. The caller therefore +// always receives an error rather than a silent empty string. +func TestParseChecksumManifestReturnsErrorForMalformedLine(t *testing.T) { + t.Parallel() + // Single-token line: the artifact name appears but with no checksum column. + manifest := []byte("gong_linux_amd64\n") + _, err := parseChecksumManifest(manifest, "gong_linux_amd64") + require.Error(t, err) + assert.Contains(t, err.Error(), "checksum entry not found for gong_linux_amd64") +} + +// TestParseChecksumManifestReturnsErrorForInvalidChecksum covers a line that is +// structurally valid (two fields) but whose checksum column contains non-hex +// characters. This is distinct from a malformed line: the artifact IS found, +// but isValidSHA256 rejects the value before we return it. +func TestParseChecksumManifestReturnsErrorForInvalidChecksum(t *testing.T) { + t.Parallel() + // 64 characters but all 'Z' — valid length, invalid hex. + invalidChecksum := strings.Repeat("Z", 64) + artifactName := "gong_linux_amd64" + manifest := []byte(invalidChecksum + " " + artifactName + "\n") + _, err := parseChecksumManifest(manifest, artifactName) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid checksum for gong_linux_amd64") +} + +// ─── verifySHA256 ──────────────────────────────────────────────────────────── + +func TestVerifySHA256ReturnsNilForMatchingChecksum(t *testing.T) { + t.Parallel() + content := []byte("fake gong binary content") + path := tempFileWithContent(t, content) + expected := hashOf(content) + err := verifySHA256(path, expected) + assert.NoError(t, err) +} + +func TestVerifySHA256ReturnsErrorForMismatch(t *testing.T) { + t.Parallel() + content := []byte("fake gong binary content") + path := tempFileWithContent(t, content) + // Checksum of different content — guaranteed not to match. + wrongHash := hashOf([]byte("completely different content")) + err := verifySHA256(path, wrongHash) + require.Error(t, err) + // Error must name both hashes so the operator can diagnose without + // running external tools. + assert.Contains(t, err.Error(), "checksum mismatch") + assert.Contains(t, err.Error(), hashOf(content)) // actual + assert.Contains(t, err.Error(), wrongHash) // expected +} + +// TestVerifySHA256AcceptsUppercaseChecksum ensures the comparison is +// case-insensitive, which is important because GoReleaser emits lowercase +// hashes but users may paste uppercase values from other tools. +func TestVerifySHA256AcceptsUppercaseChecksum(t *testing.T) { + t.Parallel() + content := []byte("fake gong binary content") + path := tempFileWithContent(t, content) + upper := strings.ToUpper(hashOf(content)) + err := verifySHA256(path, upper) + assert.NoError(t, err) +} + +// TestVerifySHA256ReturnsErrorForNonExistentFile ensures the function surfaces +// the OS error when the target file is absent, so callers get a clear signal +// instead of a hash-mismatch error. +func TestVerifySHA256ReturnsErrorForNonExistentFile(t *testing.T) { + t.Parallel() + err := verifySHA256(t.TempDir()+"/does-not-exist", knownHash) + require.Error(t, err) + assert.True(t, os.IsNotExist(err), "expected a not-exist error, got: %v", err) +} + +// TestVerifySHA256ReturnsErrorForInvalidExpectedChecksum verifies that the +// guard inside verifySHA256 fires before any file I/O when the caller passes +// a structurally invalid expected value. +func TestVerifySHA256ReturnsErrorForInvalidExpectedChecksum(t *testing.T) { + t.Parallel() + path := tempFileWithContent(t, []byte("anything")) + err := verifySHA256(path, "tooshort") + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid expected checksum") +} + +// ─── isValidSHA256 ─────────────────────────────────────────────────────────── + +func TestIsValidSHA256(t *testing.T) { + t.Parallel() + cases := []struct { + name string + input string + want bool + }{ + {"lowercase valid", knownHash, true}, + {"uppercase valid", strings.ToUpper(knownHash), true}, + {"mixed case valid", strings.ToUpper(knownHash[:32]) + knownHash[32:], true}, + {"too short", knownHash[:63], false}, + {"too long", knownHash + "0", false}, + {"empty string", "", false}, + {"non-hex chars", strings.Repeat("Z", 64), false}, + {"spaces inside", strings.Repeat("a", 32) + " " + strings.Repeat("b", 31), false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.want, isValidSHA256(tc.input)) + }) + } +} diff --git a/pkg/gong/gong.go b/pkg/gong/gong.go index 02cd49420f..d598a44489 100644 --- a/pkg/gong/gong.go +++ b/pkg/gong/gong.go @@ -24,8 +24,7 @@ import ( var Version string const ( - BaseURL = "https://storage.googleapis.com/gong-release" - + BaseURL = "https://storage.googleapis.com/gong-release" binDir = "bin" httpTimeout = 5 * time.Minute filePermissions = 0o755 @@ -35,8 +34,10 @@ const ( // It supports installing multiple versions side-by-side; concurrent installs of // the same version are deduplicated, while different versions install in parallel. type Checker struct { - mu sync.Mutex - slots map[string]*installSlot + mu sync.Mutex + slots map[string]*installSlot + baseURL string // overridden in tests to redirect downloads to a local HTTP server + installDir string // overridden in tests to install to a temporary directory } type installSlot struct { @@ -52,7 +53,6 @@ func (g *Checker) EnsureGongInstalled(ctx context.Context, version string) (stri if resolvedVersion == "" { resolvedVersion = strings.TrimSpace(Version) } - slot := g.slotFor(resolvedVersion) slot.once.Do(func() { slot.path, slot.err = g.ensureInstalled(ctx, resolvedVersion) @@ -61,6 +61,7 @@ func (g *Checker) EnsureGongInstalled(ctx context.Context, version string) (stri g.dropSlot(resolvedVersion, slot) } }) + return slot.path, slot.err } @@ -86,18 +87,32 @@ func (g *Checker) dropSlot(version string, slot *installSlot) { } } -func (g *Checker) binaryPath(version string) (string, error) { - m := user.NewConfigManager(afero.NewOsFs()) - bruinHomeDirAbsPath, err := m.EnsureAndGetBruinHomeDir() - if err != nil { - return "", errors.Wrap(err, "failed to get bruin home directory") +// effectiveBaseURL returns the base URL to use for downloads. +// Tests set g.baseURL to a local httptest.Server address; production code +// leaves it empty and falls back to the package-level BaseURL constant. +func (g *Checker) effectiveBaseURL() string { + if g.baseURL != "" { + return g.baseURL } + return BaseURL +} - binDirPath := filepath.Join(bruinHomeDirAbsPath, binDir) +func (g *Checker) binaryPath(version string) (string, error) { + var binDirPath string + if g.installDir != "" { + // Used by tests to redirect installs to a temporary directory. + binDirPath = g.installDir + } else { + m := user.NewConfigManager(afero.NewOsFs()) + bruinHomeDirAbsPath, err := m.EnsureAndGetBruinHomeDir() + if err != nil { + return "", errors.Wrap(err, "failed to get bruin home directory") + } + binDirPath = filepath.Join(bruinHomeDirAbsPath, binDir) + } if err := os.MkdirAll(binDirPath, filePermissions); err != nil { return "", errors.Wrap(err, "failed to create bin directory") } - binaryName := "gong-" + version if runtime.GOOS == "windows" { binaryName += ".exe" @@ -110,35 +125,64 @@ func (g *Checker) ensureInstalled(ctx context.Context, version string) (string, if err != nil { return "", err } - if _, err := os.Stat(gongBinaryPath); errors.Is(err, os.ErrNotExist) { if err := g.downloadGong(ctx, version, gongBinaryPath); err != nil { return "", err } return gongBinaryPath, nil } - installedVersion := "" cmd := exec.CommandContext(ctx, gongBinaryPath, "--version") output, err := cmd.CombinedOutput() if err == nil { installedVersion = parseVersionOutput(strings.TrimSpace(string(output))) } - if installedVersion != version { if err := g.downloadGong(ctx, version, gongBinaryPath); err != nil { return "", err } } - return gongBinaryPath, nil } -// buildDownloadURL constructs the download URL for the gong binary based on OS and architecture. +// buildDownloadURL constructs the download URL for the gong binary using the +// package-level BaseURL constant. Existing callers and unit tests use this form. func buildDownloadURL(version string) string { - osName := getOSName() - archName := getArchName() - return fmt.Sprintf("%s/releases/%s/%s/gong_%s", BaseURL, version, osName, archName) + return buildDownloadURLWithBase(BaseURL, version) +} + +// buildDownloadURLWithBase is like buildDownloadURL but accepts an explicit base +// URL so that tests can redirect downloads to a local HTTP server. +func buildDownloadURLWithBase(base, version string) string { + return fmt.Sprintf("%s/releases/%s/%s/gong_%s", base, version, getOSName(), getArchName()) +} + +// buildChecksumURL returns the URL of the SHA256 checksum manifest published +// alongside a gong release. The manifest uses the standard sha256sum format. +func buildChecksumURL(base, version string) string { + return fmt.Sprintf("%s/releases/%s/checksums.txt", base, version) +} + +// downloadChecksumManifest fetches the raw bytes of the checksum manifest at +// the given URL. The caller is responsible for parsing the returned bytes. +func downloadChecksumManifest(ctx context.Context, url string, client *http.Client) ([]byte, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return nil, fmt.Errorf("failed to create checksum request: %w", err) + } + resp, err := client.Do(req) + if err != nil { + return nil, fmt.Errorf("failed to download checksum file: %w", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("failed to download checksum file: server returned status %d", resp.StatusCode) + } + data, err := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) // 1 MiB cap + if err != nil { + return nil, fmt.Errorf("failed to read checksum file: %w", err) + } + return data, nil } func getOSName() string { @@ -173,7 +217,6 @@ func parseVersionOutput(output string) string { if len(parts) < 3 { return output } - ver := parts[len(parts)-1] if !strings.HasPrefix(ver, "v") { ver = "v" + ver @@ -183,37 +226,40 @@ func parseVersionOutput(output string) string { func (g *Checker) downloadGong(ctx context.Context, version, destPath string) error { var output io.Writer = os.Stdout + if printer, ok := ctx.Value(executor.KeyPrinter).(io.Writer); ok { output = printer } _, _ = fmt.Fprintf(output, "===============================\n") + _, _ = fmt.Fprintf(output, "Installing gong %s...\n", version) + _, _ = fmt.Fprintf(output, "This is a one-time operation.\n") + _, _ = fmt.Fprintf(output, "\n") - downloadURL := buildDownloadURL(version) + base := g.effectiveBaseURL() + artifactName := fmt.Sprintf("gong_%s_%s", getOSName(), getArchName()) + downloadURL := buildDownloadURLWithBase(base, version) + _, _ = fmt.Fprintf(output, "Downloading from %s\n", downloadURL) client := &http.Client{ Timeout: httpTimeout, } - req, err := http.NewRequestWithContext(ctx, http.MethodGet, downloadURL, nil) if err != nil { return fmt.Errorf("failed to create download request: %w", err) } - resp, err := client.Do(req) if err != nil { return fmt.Errorf("failed to download gong: %w", err) } defer resp.Body.Close() - if resp.StatusCode != http.StatusOK { return fmt.Errorf("failed to download gong %s: server returned status %d", version, resp.StatusCode) } - // Create a temporary file in the same directory to ensure atomic write destDir := filepath.Dir(destPath) tmpFile, err := os.CreateTemp(destDir, "gong-download-*") @@ -221,39 +267,46 @@ func (g *Checker) downloadGong(ctx context.Context, version, destPath string) er return fmt.Errorf("failed to create temporary file: %w", err) } tmpPath := tmpFile.Name() - // Clean up temp file on error defer func() { if tmpPath != "" { os.Remove(tmpPath) } }() - _, err = io.Copy(tmpFile, resp.Body) if err != nil { tmpFile.Close() return fmt.Errorf("failed to write gong binary: %w", err) } - if err := tmpFile.Close(); err != nil { return fmt.Errorf("failed to close temporary file: %w", err) } - + // Verify the downloaded binary against the published checksum manifest before + // granting executable permissions or moving it to the final destination. + // On any failure the deferred os.Remove cleans up tmpPath automatically. + manifest, err := downloadChecksumManifest(ctx, buildChecksumURL(base, version), client) + if err != nil { + return err + } + expectedChecksum, err := parseChecksumManifest(manifest, artifactName) + if err != nil { + return err + } + if err := verifySHA256(tmpPath, expectedChecksum); err != nil { + return err + } // Set executable permissions (on Windows this is a no-op effectively) if err := os.Chmod(tmpPath, filePermissions); err != nil { return fmt.Errorf("failed to set executable permissions: %w", err) } - // Atomic rename if err := os.Rename(tmpPath, destPath); err != nil { return fmt.Errorf("failed to move gong binary to destination: %w", err) } tmpPath = "" // Prevent cleanup of the renamed file - _, _ = fmt.Fprintf(output, "\n") _, _ = fmt.Fprintf(output, "Installed gong %s, continuing...\n", version) _, _ = fmt.Fprintf(output, "===============================\n") _, _ = fmt.Fprintf(output, "\n") - return nil } diff --git a/pkg/gong/gong_install_test.go b/pkg/gong/gong_install_test.go new file mode 100644 index 0000000000..d018a5766e --- /dev/null +++ b/pkg/gong/gong_install_test.go @@ -0,0 +1,168 @@ +package gong + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "fmt" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// ─── shared fixtures ───────────────────────────────────────────────────────── + +// fakeBinaryContent is a small, deterministic blob used as the stand-in gong +// executable. Using a package-level var (read-only in tests) avoids allocating +// it repeatedly while keeping all tests hermetic. +var fakeBinaryContent = []byte("fake gong binary content for integration tests") + +// checksumManifestFor returns a sha256sum-format manifest whose single entry +// covers binary under the current platform's artifact name (e.g. gong_linux_amd64). +// This mirrors the format that GoReleaser publishes as checksums.txt. +func checksumManifestFor(binary []byte) string { + sum := sha256.Sum256(binary) + artifactName := fmt.Sprintf("gong_%s_%s", getOSName(), getArchName()) + return hex.EncodeToString(sum[:]) + " " + artifactName + "\n" +} + +// installBinaryPath reconstructs the on-disk path where Checker places the +// binary when installDir is set. It mirrors (*Checker).binaryPath exactly so +// there is a single authoritative location per test. +func installBinaryPath(installDir, version string) string { + name := "gong-" + version + if runtime.GOOS == "windows" { + name += ".exe" + } + return filepath.Join(installDir, name) +} + +// newInstallTestServer starts an httptest.Server that routes the two URLs +// Checker derives from its baseURL: +// +// - /releases/{version}/{os}/gong_{arch} → serves binary with HTTP 200 +// - /releases/{version}/checksums.txt → serves checksumBody with checksumStatus +// +// Requests for any other path are treated as test failures. +// checksumStatus == 0 is normalised to http.StatusOK. +// The server is automatically closed when the test finishes. +func newInstallTestServer( + t *testing.T, + version string, + binary []byte, + checksumBody string, + checksumStatus int, +) *httptest.Server { + t.Helper() + if checksumStatus == 0 { + checksumStatus = http.StatusOK + } + binaryURLPath := fmt.Sprintf("/releases/%s/%s/gong_%s", version, getOSName(), getArchName()) + checksumURLPath := fmt.Sprintf("/releases/%s/checksums.txt", version) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case binaryURLPath: + w.WriteHeader(http.StatusOK) + _, _ = w.Write(binary) + case checksumURLPath: + w.WriteHeader(checksumStatus) + _, _ = io.WriteString(w, checksumBody) + default: + // Any unexpected request is a test bug, not a product bug. + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + http.NotFound(w, r) + } + })) + t.Cleanup(srv.Close) + return srv +} + +// ─── integration tests ─────────────────────────────────────────────────────── + +const installTestVersion = "v0.0.0-test" + +// TestEnsureGongInstalled_ValidChecksumInstallsBinary is the happy-path test: +// the checksum manifest matches the downloaded binary, so EnsureGongInstalled +// must return no error and write the binary to the install directory with the +// exact same bytes that the server sent. +func TestEnsureGongInstalled_ValidChecksumInstallsBinary(t *testing.T) { + t.Parallel() + installDir := t.TempDir() + manifest := checksumManifestFor(fakeBinaryContent) + srv := newInstallTestServer(t, installTestVersion, fakeBinaryContent, manifest, 0) + checker := &Checker{baseURL: srv.URL, installDir: installDir} + gotPath, err := checker.EnsureGongInstalled(context.Background(), installTestVersion) + require.NoError(t, err, "valid checksum must not produce an error") + expectedPath := installBinaryPath(installDir, installTestVersion) + assert.Equal(t, expectedPath, gotPath, "returned path must equal the final install destination") + // Content integrity: bytes on disk must exactly match what the server served. + gotBytes, readErr := os.ReadFile(expectedPath) + require.NoError(t, readErr, "installed binary must be readable at the returned path") + assert.Equal(t, fakeBinaryContent, gotBytes, "installed binary content must equal the downloaded binary") +} + +// TestEnsureGongInstalled_InvalidChecksumBlocksInstall verifies that when the +// checksum manifest contains a hash for different content, EnsureGongInstalled +// returns an error that names the mismatch, and leaves no file at the final +// install path (the temp file is cleaned up automatically by the deferred +// os.Remove inside downloadGong). +func TestEnsureGongInstalled_InvalidChecksumBlocksInstall(t *testing.T) { + t.Parallel() + installDir := t.TempDir() + // Build a manifest whose hash is for content that differs from fakeBinaryContent. + wrongManifest := checksumManifestFor([]byte("this is not the binary that was downloaded")) + srv := newInstallTestServer(t, installTestVersion, fakeBinaryContent, wrongManifest, 0) + checker := &Checker{baseURL: srv.URL, installDir: installDir} + _, err := checker.EnsureGongInstalled(context.Background(), installTestVersion) + require.Error(t, err, "a checksum mismatch must produce an error") + assert.Contains(t, err.Error(), "checksum mismatch", + "error message must identify the cause as a checksum mismatch") + _, statErr := os.Stat(installBinaryPath(installDir, installTestVersion)) + assert.True(t, os.IsNotExist(statErr), + "binary must not exist at the final install path after a mismatch") +} + +// TestEnsureGongInstalled_MissingChecksumEntryBlocksInstall verifies that when +// the checksum manifest does not contain an entry for the current platform's +// artifact name, EnsureGongInstalled fails before installing anything. +func TestEnsureGongInstalled_MissingChecksumEntryBlocksInstall(t *testing.T) { + t.Parallel() + installDir := t.TempDir() + // Manifest has a valid entry but for a completely different artifact name, + // so the lookup for the current platform's artifact returns "not found". + unrelatedHash := hashOf(fakeBinaryContent) + manifestWithWrongArtifact := unrelatedHash + " gong_completely_different_arch\n" + srv := newInstallTestServer(t, installTestVersion, fakeBinaryContent, manifestWithWrongArtifact, 0) + checker := &Checker{baseURL: srv.URL, installDir: installDir} + _, err := checker.EnsureGongInstalled(context.Background(), installTestVersion) + require.Error(t, err, "a missing checksum entry must produce an error") + assert.Contains(t, err.Error(), "checksum entry not found", + "error message must report that no matching checksum was found in the manifest") + _, statErr := os.Stat(installBinaryPath(installDir, installTestVersion)) + assert.True(t, os.IsNotExist(statErr), + "binary must not exist at the final install path when its checksum entry is absent") +} + +// TestEnsureGongInstalled_ChecksumServerFailureBlocksInstall verifies that an +// HTTP 500 from the checksum endpoint prevents the binary from reaching its +// final install path. The binary download itself succeeds; only the checksum +// fetch fails — this is the strongest proof that verification is mandatory. +func TestEnsureGongInstalled_ChecksumServerFailureBlocksInstall(t *testing.T) { + t.Parallel() + installDir := t.TempDir() + srv := newInstallTestServer(t, installTestVersion, fakeBinaryContent, "", http.StatusInternalServerError) + checker := &Checker{baseURL: srv.URL, installDir: installDir} + _, err := checker.EnsureGongInstalled(context.Background(), installTestVersion) + require.Error(t, err, + "an HTTP error on the checksum endpoint must propagate as an error") + _, statErr := os.Stat(installBinaryPath(installDir, installTestVersion)) + assert.True(t, os.IsNotExist(statErr), + "binary must not exist at the final install path after a checksum server failure") +} diff --git a/pkg/gong/gong_test.go b/pkg/gong/gong_test.go index 49a54f0948..f3aa3b60ec 100644 --- a/pkg/gong/gong_test.go +++ b/pkg/gong/gong_test.go @@ -16,15 +16,21 @@ func TestBuildDownloadURL(t *testing.T) { url := buildDownloadURL(strings.TrimSpace(Version)) // URL should contain the base URL + assert.Contains(t, url, BaseURL) // URL should contain the version + assert.Contains(t, url, strings.TrimSpace(Version)) // URL should follow the expected format + expectedOS := getOSName() + expectedArch := getArchName() + expectedURL := BaseURL + "/releases/" + strings.TrimSpace(Version) + "/" + expectedOS + "/gong_" + expectedArch + assert.Equal(t, expectedURL, url) } @@ -34,11 +40,14 @@ func TestGetOSName(t *testing.T) { osName := getOSName() // Should return a valid OS name + validOSNames := []string{"darwin", "linux", "windows"} + if runtime.GOOS == "darwin" || runtime.GOOS == "linux" || runtime.GOOS == "windows" { assert.Contains(t, validOSNames, osName) } else { // For other OSes, it should return runtime.GOOS + assert.Equal(t, runtime.GOOS, osName) } } @@ -49,11 +58,14 @@ func TestGetArchName(t *testing.T) { archName := getArchName() // Should return a valid architecture name + validArchNames := []string{"amd64", "arm64"} + if runtime.GOARCH == "amd64" || runtime.GOARCH == "arm64" { assert.Contains(t, validArchNames, archName) } else { // For other architectures, it should return runtime.GOARCH + assert.Equal(t, runtime.GOARCH, archName) } } @@ -62,19 +74,25 @@ func TestParseVersionOutput(t *testing.T) { t.Parallel() tests := []struct { - name string - input string + name string + + input string + expect string }{ {"standard output", "gong version 0.1.2", "v0.1.2"}, + {"already has v prefix", "gong version v0.1.2", "v0.1.2"}, + {"just version number", "0.1.2", "0.1.2"}, + {"empty string", "", ""}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() + assert.Equal(t, tt.expect, parseVersionOutput(tt.input)) }) } @@ -86,7 +104,9 @@ func TestBuildDownloadURL_Format(t *testing.T) { url := buildDownloadURL(strings.TrimSpace(Version)) // Verify the URL matches the expected pattern + // Format: {BaseURL}/releases/{Version}/{os}/gong_{arch} + assert.Regexp(t, `^https://storage\.googleapis\.com/gong-release/releases/v\d+\.\d+\.\d+/(darwin|linux|windows)/gong_(amd64|arm64)$`, url) } @@ -94,6 +114,7 @@ func TestBuildDownloadURL_RespectsRequestedVersion(t *testing.T) { t.Parallel() url := buildDownloadURL("v9.9.9") + assert.Contains(t, url, "/releases/v9.9.9/") } @@ -101,21 +122,31 @@ func TestChecker_SlotPerVersion(t *testing.T) { t.Parallel() c := &Checker{} + a := c.slotFor("v1.0.0") + b := c.slotFor("v1.0.0") + d := c.slotFor("v1.0.5") assert.Same(t, a, b, "same version returns the same slot") + assert.NotSame(t, a, d, "different versions get different slots") // dropSlot must remove only the matching slot. + c.dropSlot("v1.0.0", a) + a2 := c.slotFor("v1.0.0") + assert.NotSame(t, a, a2, "slot should be regenerated after drop") // dropSlot with a stale pointer is a no-op. + c.dropSlot("v1.0.0", a) + a3 := c.slotFor("v1.0.0") + assert.Same(t, a2, a3, "stale drop must not evict a fresh slot") } @@ -125,19 +156,27 @@ func TestChecker_SlotForIsConcurrencySafe(t *testing.T) { c := &Checker{} const goroutines = 32 + results := make([]*installSlot, goroutines) var wg sync.WaitGroup + for i := range goroutines { + wg.Add(1) + go func(idx int) { defer wg.Done() + results[idx] = c.slotFor("v1.0.0") }(i) + } + wg.Wait() require.NotNil(t, results[0]) + for _, slot := range results { assert.Same(t, results[0], slot, "all goroutines must observe the same slot for the same version") } From 55e781ac91fc2fc0afc8c9c1ca19526fe369d606 Mon Sep 17 00:00:00 2001 From: Musa Misto Date: Tue, 5 May 2026 18:35:34 +0300 Subject: [PATCH 2/5] refactor(tests): clean up whitespace and improve readability in gong_test.go Co-authored-by: Copilot --- pkg/gong/gong_test.go | 64 ++----------------------------------------- 1 file changed, 2 insertions(+), 62 deletions(-) diff --git a/pkg/gong/gong_test.go b/pkg/gong/gong_test.go index f3aa3b60ec..602fd193c1 100644 --- a/pkg/gong/gong_test.go +++ b/pkg/gong/gong_test.go @@ -12,87 +12,59 @@ import ( func TestBuildDownloadURL(t *testing.T) { t.Parallel() - url := buildDownloadURL(strings.TrimSpace(Version)) - // URL should contain the base URL - assert.Contains(t, url, BaseURL) - // URL should contain the version - assert.Contains(t, url, strings.TrimSpace(Version)) - // URL should follow the expected format - expectedOS := getOSName() - expectedArch := getArchName() - expectedURL := BaseURL + "/releases/" + strings.TrimSpace(Version) + "/" + expectedOS + "/gong_" + expectedArch - assert.Equal(t, expectedURL, url) } func TestGetOSName(t *testing.T) { t.Parallel() - osName := getOSName() - // Should return a valid OS name - validOSNames := []string{"darwin", "linux", "windows"} - if runtime.GOOS == "darwin" || runtime.GOOS == "linux" || runtime.GOOS == "windows" { assert.Contains(t, validOSNames, osName) } else { // For other OSes, it should return runtime.GOOS - assert.Equal(t, runtime.GOOS, osName) } } func TestGetArchName(t *testing.T) { t.Parallel() - archName := getArchName() - // Should return a valid architecture name - validArchNames := []string{"amd64", "arm64"} - if runtime.GOARCH == "amd64" || runtime.GOARCH == "arm64" { assert.Contains(t, validArchNames, archName) } else { // For other architectures, it should return runtime.GOARCH - assert.Equal(t, runtime.GOARCH, archName) } } func TestParseVersionOutput(t *testing.T) { t.Parallel() - tests := []struct { - name string - - input string - + name string + input string expect string }{ {"standard output", "gong version 0.1.2", "v0.1.2"}, - {"already has v prefix", "gong version v0.1.2", "v0.1.2"}, - {"just version number", "0.1.2", "0.1.2"}, - {"empty string", "", ""}, } - for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - assert.Equal(t, tt.expect, parseVersionOutput(tt.input)) }) } @@ -100,83 +72,51 @@ func TestParseVersionOutput(t *testing.T) { func TestBuildDownloadURL_Format(t *testing.T) { t.Parallel() - url := buildDownloadURL(strings.TrimSpace(Version)) - // Verify the URL matches the expected pattern - // Format: {BaseURL}/releases/{Version}/{os}/gong_{arch} - assert.Regexp(t, `^https://storage\.googleapis\.com/gong-release/releases/v\d+\.\d+\.\d+/(darwin|linux|windows)/gong_(amd64|arm64)$`, url) } func TestBuildDownloadURL_RespectsRequestedVersion(t *testing.T) { t.Parallel() - url := buildDownloadURL("v9.9.9") - assert.Contains(t, url, "/releases/v9.9.9/") } func TestChecker_SlotPerVersion(t *testing.T) { t.Parallel() - c := &Checker{} - a := c.slotFor("v1.0.0") - b := c.slotFor("v1.0.0") - d := c.slotFor("v1.0.5") - assert.Same(t, a, b, "same version returns the same slot") - assert.NotSame(t, a, d, "different versions get different slots") - // dropSlot must remove only the matching slot. - c.dropSlot("v1.0.0", a) - a2 := c.slotFor("v1.0.0") - assert.NotSame(t, a, a2, "slot should be regenerated after drop") - // dropSlot with a stale pointer is a no-op. - c.dropSlot("v1.0.0", a) - a3 := c.slotFor("v1.0.0") - assert.Same(t, a2, a3, "stale drop must not evict a fresh slot") } func TestChecker_SlotForIsConcurrencySafe(t *testing.T) { t.Parallel() - c := &Checker{} - const goroutines = 32 - results := make([]*installSlot, goroutines) - var wg sync.WaitGroup - for i := range goroutines { - wg.Add(1) - go func(idx int) { defer wg.Done() - results[idx] = c.slotFor("v1.0.0") }(i) - } - wg.Wait() - require.NotNil(t, results[0]) - for _, slot := range results { assert.Same(t, results[0], slot, "all goroutines must observe the same slot for the same version") } From faa0cd973ce619e1b49021a5ae853e2f73556ea7 Mon Sep 17 00:00:00 2001 From: Musa Misto Date: Tue, 5 May 2026 18:41:14 +0300 Subject: [PATCH 3/5] refactor(tests): improve readability by adding whitespace in gong_test.go, gong_install_test.go, and checksum_test.go Co-authored-by: Copilot --- pkg/gong/checksum_test.go | 35 +++++++++++++++++++++++++++++++++++ pkg/gong/gong_install_test.go | 14 ++++++++++++++ pkg/gong/gong_test.go | 23 ++++++++++++++++++++--- 3 files changed, 69 insertions(+), 3 deletions(-) diff --git a/pkg/gong/checksum_test.go b/pkg/gong/checksum_test.go index 3d53842e7a..08031b3f32 100644 --- a/pkg/gong/checksum_test.go +++ b/pkg/gong/checksum_test.go @@ -31,6 +31,7 @@ func hashOf(b []byte) string { // and returns the file path. Cleanup is handled automatically by the test runner. func tempFileWithContent(t *testing.T, content []byte) string { t.Helper() + f, err := os.CreateTemp(t.TempDir(), "verify-sha256-*") require.NoError(t, err) _, err = f.Write(content) @@ -43,15 +44,19 @@ func tempFileWithContent(t *testing.T, content []byte) string { func TestParseChecksumManifestReturnsMatchingChecksum(t *testing.T) { t.Parallel() + artifactName := "gong_linux_amd64" manifest := []byte(knownHash + " " + artifactName + "\n") + got, err := parseChecksumManifest(manifest, artifactName) + require.NoError(t, err) assert.Equal(t, knownHash, got) } func TestParseChecksumManifestIgnoresEmptyAndCommentLines(t *testing.T) { t.Parallel() + artifactName := "gong_linux_amd64" // Manifest deliberately padded with every kind of noise the spec calls out: // blank lines, lines of only whitespace, and comment lines. @@ -65,7 +70,9 @@ func TestParseChecksumManifestIgnoresEmptyAndCommentLines(t *testing.T) { "# end\n", knownHash, artifactName, )) + got, err := parseChecksumManifest(manifest, artifactName) + require.NoError(t, err) assert.Equal(t, knownHash, got) } @@ -75,6 +82,7 @@ func TestParseChecksumManifestIgnoresEmptyAndCommentLines(t *testing.T) { // the requested artifact only — not for any other artifact. func TestParseChecksumManifestPicksCorrectEntryAmongMultiple(t *testing.T) { t.Parallel() + arm64Hash := strings.Repeat("a", 64) amd64Hash := knownHash manifest := []byte(fmt.Sprintf( @@ -82,16 +90,21 @@ func TestParseChecksumManifestPicksCorrectEntryAmongMultiple(t *testing.T) { "%s gong_linux_amd64\n", arm64Hash, amd64Hash, )) + got, err := parseChecksumManifest(manifest, "gong_linux_amd64") + require.NoError(t, err) assert.Equal(t, amd64Hash, got) } func TestParseChecksumManifestReturnsErrorWhenArtifactMissing(t *testing.T) { t.Parallel() + // Manifest contains only arm64; requesting amd64 must fail. manifest := []byte(knownHash + " gong_linux_arm64\n") + _, err := parseChecksumManifest(manifest, "gong_linux_amd64") + require.Error(t, err) assert.Contains(t, err.Error(), "checksum entry not found for gong_linux_amd64") } @@ -104,9 +117,12 @@ func TestParseChecksumManifestReturnsErrorWhenArtifactMissing(t *testing.T) { // always receives an error rather than a silent empty string. func TestParseChecksumManifestReturnsErrorForMalformedLine(t *testing.T) { t.Parallel() + // Single-token line: the artifact name appears but with no checksum column. manifest := []byte("gong_linux_amd64\n") + _, err := parseChecksumManifest(manifest, "gong_linux_amd64") + require.Error(t, err) assert.Contains(t, err.Error(), "checksum entry not found for gong_linux_amd64") } @@ -117,11 +133,14 @@ func TestParseChecksumManifestReturnsErrorForMalformedLine(t *testing.T) { // but isValidSHA256 rejects the value before we return it. func TestParseChecksumManifestReturnsErrorForInvalidChecksum(t *testing.T) { t.Parallel() + // 64 characters but all 'Z' — valid length, invalid hex. invalidChecksum := strings.Repeat("Z", 64) artifactName := "gong_linux_amd64" manifest := []byte(invalidChecksum + " " + artifactName + "\n") + _, err := parseChecksumManifest(manifest, artifactName) + require.Error(t, err) assert.Contains(t, err.Error(), "invalid checksum for gong_linux_amd64") } @@ -130,20 +149,26 @@ func TestParseChecksumManifestReturnsErrorForInvalidChecksum(t *testing.T) { func TestVerifySHA256ReturnsNilForMatchingChecksum(t *testing.T) { t.Parallel() + content := []byte("fake gong binary content") path := tempFileWithContent(t, content) expected := hashOf(content) + err := verifySHA256(path, expected) + assert.NoError(t, err) } func TestVerifySHA256ReturnsErrorForMismatch(t *testing.T) { t.Parallel() + content := []byte("fake gong binary content") path := tempFileWithContent(t, content) // Checksum of different content — guaranteed not to match. wrongHash := hashOf([]byte("completely different content")) + err := verifySHA256(path, wrongHash) + require.Error(t, err) // Error must name both hashes so the operator can diagnose without // running external tools. @@ -157,10 +182,13 @@ func TestVerifySHA256ReturnsErrorForMismatch(t *testing.T) { // hashes but users may paste uppercase values from other tools. func TestVerifySHA256AcceptsUppercaseChecksum(t *testing.T) { t.Parallel() + content := []byte("fake gong binary content") path := tempFileWithContent(t, content) upper := strings.ToUpper(hashOf(content)) + err := verifySHA256(path, upper) + assert.NoError(t, err) } @@ -169,7 +197,9 @@ func TestVerifySHA256AcceptsUppercaseChecksum(t *testing.T) { // instead of a hash-mismatch error. func TestVerifySHA256ReturnsErrorForNonExistentFile(t *testing.T) { t.Parallel() + err := verifySHA256(t.TempDir()+"/does-not-exist", knownHash) + require.Error(t, err) assert.True(t, os.IsNotExist(err), "expected a not-exist error, got: %v", err) } @@ -179,8 +209,11 @@ func TestVerifySHA256ReturnsErrorForNonExistentFile(t *testing.T) { // a structurally invalid expected value. func TestVerifySHA256ReturnsErrorForInvalidExpectedChecksum(t *testing.T) { t.Parallel() + path := tempFileWithContent(t, []byte("anything")) + err := verifySHA256(path, "tooshort") + require.Error(t, err) assert.Contains(t, err.Error(), "invalid expected checksum") } @@ -189,6 +222,7 @@ func TestVerifySHA256ReturnsErrorForInvalidExpectedChecksum(t *testing.T) { func TestIsValidSHA256(t *testing.T) { t.Parallel() + cases := []struct { name string input string @@ -206,6 +240,7 @@ func TestIsValidSHA256(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { t.Parallel() + assert.Equal(t, tc.want, isValidSHA256(tc.input)) }) } diff --git a/pkg/gong/gong_install_test.go b/pkg/gong/gong_install_test.go index d018a5766e..54168137cc 100644 --- a/pkg/gong/gong_install_test.go +++ b/pkg/gong/gong_install_test.go @@ -61,6 +61,7 @@ func newInstallTestServer( checksumStatus int, ) *httptest.Server { t.Helper() + if checksumStatus == 0 { checksumStatus = http.StatusOK } @@ -94,14 +95,18 @@ const installTestVersion = "v0.0.0-test" // exact same bytes that the server sent. func TestEnsureGongInstalled_ValidChecksumInstallsBinary(t *testing.T) { t.Parallel() + installDir := t.TempDir() manifest := checksumManifestFor(fakeBinaryContent) srv := newInstallTestServer(t, installTestVersion, fakeBinaryContent, manifest, 0) checker := &Checker{baseURL: srv.URL, installDir: installDir} + gotPath, err := checker.EnsureGongInstalled(context.Background(), installTestVersion) + require.NoError(t, err, "valid checksum must not produce an error") expectedPath := installBinaryPath(installDir, installTestVersion) assert.Equal(t, expectedPath, gotPath, "returned path must equal the final install destination") + // Content integrity: bytes on disk must exactly match what the server served. gotBytes, readErr := os.ReadFile(expectedPath) require.NoError(t, readErr, "installed binary must be readable at the returned path") @@ -115,12 +120,15 @@ func TestEnsureGongInstalled_ValidChecksumInstallsBinary(t *testing.T) { // os.Remove inside downloadGong). func TestEnsureGongInstalled_InvalidChecksumBlocksInstall(t *testing.T) { t.Parallel() + installDir := t.TempDir() // Build a manifest whose hash is for content that differs from fakeBinaryContent. wrongManifest := checksumManifestFor([]byte("this is not the binary that was downloaded")) srv := newInstallTestServer(t, installTestVersion, fakeBinaryContent, wrongManifest, 0) checker := &Checker{baseURL: srv.URL, installDir: installDir} + _, err := checker.EnsureGongInstalled(context.Background(), installTestVersion) + require.Error(t, err, "a checksum mismatch must produce an error") assert.Contains(t, err.Error(), "checksum mismatch", "error message must identify the cause as a checksum mismatch") @@ -134,6 +142,7 @@ func TestEnsureGongInstalled_InvalidChecksumBlocksInstall(t *testing.T) { // artifact name, EnsureGongInstalled fails before installing anything. func TestEnsureGongInstalled_MissingChecksumEntryBlocksInstall(t *testing.T) { t.Parallel() + installDir := t.TempDir() // Manifest has a valid entry but for a completely different artifact name, // so the lookup for the current platform's artifact returns "not found". @@ -141,7 +150,9 @@ func TestEnsureGongInstalled_MissingChecksumEntryBlocksInstall(t *testing.T) { manifestWithWrongArtifact := unrelatedHash + " gong_completely_different_arch\n" srv := newInstallTestServer(t, installTestVersion, fakeBinaryContent, manifestWithWrongArtifact, 0) checker := &Checker{baseURL: srv.URL, installDir: installDir} + _, err := checker.EnsureGongInstalled(context.Background(), installTestVersion) + require.Error(t, err, "a missing checksum entry must produce an error") assert.Contains(t, err.Error(), "checksum entry not found", "error message must report that no matching checksum was found in the manifest") @@ -156,10 +167,13 @@ func TestEnsureGongInstalled_MissingChecksumEntryBlocksInstall(t *testing.T) { // fetch fails — this is the strongest proof that verification is mandatory. func TestEnsureGongInstalled_ChecksumServerFailureBlocksInstall(t *testing.T) { t.Parallel() + installDir := t.TempDir() srv := newInstallTestServer(t, installTestVersion, fakeBinaryContent, "", http.StatusInternalServerError) checker := &Checker{baseURL: srv.URL, installDir: installDir} + _, err := checker.EnsureGongInstalled(context.Background(), installTestVersion) + require.Error(t, err, "an HTTP error on the checksum endpoint must propagate as an error") _, statErr := os.Stat(installBinaryPath(installDir, installTestVersion)) diff --git a/pkg/gong/gong_test.go b/pkg/gong/gong_test.go index 602fd193c1..719c2f8484 100644 --- a/pkg/gong/gong_test.go +++ b/pkg/gong/gong_test.go @@ -12,11 +12,14 @@ import ( func TestBuildDownloadURL(t *testing.T) { t.Parallel() + url := buildDownloadURL(strings.TrimSpace(Version)) + // URL should contain the base URL assert.Contains(t, url, BaseURL) // URL should contain the version assert.Contains(t, url, strings.TrimSpace(Version)) + // URL should follow the expected format expectedOS := getOSName() expectedArch := getArchName() @@ -26,8 +29,9 @@ func TestBuildDownloadURL(t *testing.T) { func TestGetOSName(t *testing.T) { t.Parallel() + osName := getOSName() - // Should return a valid OS name + validOSNames := []string{"darwin", "linux", "windows"} if runtime.GOOS == "darwin" || runtime.GOOS == "linux" || runtime.GOOS == "windows" { assert.Contains(t, validOSNames, osName) @@ -39,8 +43,9 @@ func TestGetOSName(t *testing.T) { func TestGetArchName(t *testing.T) { t.Parallel() + archName := getArchName() - // Should return a valid architecture name + validArchNames := []string{"amd64", "arm64"} if runtime.GOARCH == "amd64" || runtime.GOARCH == "arm64" { assert.Contains(t, validArchNames, archName) @@ -52,6 +57,7 @@ func TestGetArchName(t *testing.T) { func TestParseVersionOutput(t *testing.T) { t.Parallel() + tests := []struct { name string input string @@ -65,6 +71,7 @@ func TestParseVersionOutput(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() + assert.Equal(t, tt.expect, parseVersionOutput(tt.input)) }) } @@ -72,30 +79,37 @@ func TestParseVersionOutput(t *testing.T) { func TestBuildDownloadURL_Format(t *testing.T) { t.Parallel() + url := buildDownloadURL(strings.TrimSpace(Version)) - // Verify the URL matches the expected pattern + // Format: {BaseURL}/releases/{Version}/{os}/gong_{arch} assert.Regexp(t, `^https://storage\.googleapis\.com/gong-release/releases/v\d+\.\d+\.\d+/(darwin|linux|windows)/gong_(amd64|arm64)$`, url) } func TestBuildDownloadURL_RespectsRequestedVersion(t *testing.T) { t.Parallel() + url := buildDownloadURL("v9.9.9") + assert.Contains(t, url, "/releases/v9.9.9/") } func TestChecker_SlotPerVersion(t *testing.T) { t.Parallel() + c := &Checker{} a := c.slotFor("v1.0.0") b := c.slotFor("v1.0.0") d := c.slotFor("v1.0.5") + assert.Same(t, a, b, "same version returns the same slot") assert.NotSame(t, a, d, "different versions get different slots") + // dropSlot must remove only the matching slot. c.dropSlot("v1.0.0", a) a2 := c.slotFor("v1.0.0") assert.NotSame(t, a, a2, "slot should be regenerated after drop") + // dropSlot with a stale pointer is a no-op. c.dropSlot("v1.0.0", a) a3 := c.slotFor("v1.0.0") @@ -104,9 +118,11 @@ func TestChecker_SlotPerVersion(t *testing.T) { func TestChecker_SlotForIsConcurrencySafe(t *testing.T) { t.Parallel() + c := &Checker{} const goroutines = 32 results := make([]*installSlot, goroutines) + var wg sync.WaitGroup for i := range goroutines { wg.Add(1) @@ -116,6 +132,7 @@ func TestChecker_SlotForIsConcurrencySafe(t *testing.T) { }(i) } wg.Wait() + require.NotNil(t, results[0]) for _, slot := range results { assert.Same(t, results[0], slot, "all goroutines must observe the same slot for the same version") From c30e6bfde26c7c7d75539d6d57eb1f86598b712a Mon Sep 17 00:00:00 2001 From: Musa Misto Date: Tue, 5 May 2026 18:45:36 +0300 Subject: [PATCH 4/5] refactor(tests): enhance readability by adding whitespace in gong_test.go and improve test assertions --- pkg/gong/gong.go | 22 ++++++++++++++++---- pkg/gong/gong_test.go | 47 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 63 insertions(+), 6 deletions(-) diff --git a/pkg/gong/gong.go b/pkg/gong/gong.go index d598a44489..f5ee9df685 100644 --- a/pkg/gong/gong.go +++ b/pkg/gong/gong.go @@ -53,6 +53,7 @@ func (g *Checker) EnsureGongInstalled(ctx context.Context, version string) (stri if resolvedVersion == "" { resolvedVersion = strings.TrimSpace(Version) } + slot := g.slotFor(resolvedVersion) slot.once.Do(func() { slot.path, slot.err = g.ensureInstalled(ctx, resolvedVersion) @@ -113,6 +114,7 @@ func (g *Checker) binaryPath(version string) (string, error) { if err := os.MkdirAll(binDirPath, filePermissions); err != nil { return "", errors.Wrap(err, "failed to create bin directory") } + binaryName := "gong-" + version if runtime.GOOS == "windows" { binaryName += ".exe" @@ -125,23 +127,27 @@ func (g *Checker) ensureInstalled(ctx context.Context, version string) (string, if err != nil { return "", err } + if _, err := os.Stat(gongBinaryPath); errors.Is(err, os.ErrNotExist) { if err := g.downloadGong(ctx, version, gongBinaryPath); err != nil { return "", err } return gongBinaryPath, nil } + installedVersion := "" cmd := exec.CommandContext(ctx, gongBinaryPath, "--version") output, err := cmd.CombinedOutput() if err == nil { installedVersion = parseVersionOutput(strings.TrimSpace(string(output))) } + if installedVersion != version { if err := g.downloadGong(ctx, version, gongBinaryPath); err != nil { return "", err } } + return gongBinaryPath, nil } @@ -217,6 +223,7 @@ func parseVersionOutput(output string) string { if len(parts) < 3 { return output } + ver := parts[len(parts)-1] if !strings.HasPrefix(ver, "v") { ver = "v" + ver @@ -226,17 +233,13 @@ func parseVersionOutput(output string) string { func (g *Checker) downloadGong(ctx context.Context, version, destPath string) error { var output io.Writer = os.Stdout - if printer, ok := ctx.Value(executor.KeyPrinter).(io.Writer); ok { output = printer } _, _ = fmt.Fprintf(output, "===============================\n") - _, _ = fmt.Fprintf(output, "Installing gong %s...\n", version) - _, _ = fmt.Fprintf(output, "This is a one-time operation.\n") - _, _ = fmt.Fprintf(output, "\n") base := g.effectiveBaseURL() @@ -248,18 +251,22 @@ func (g *Checker) downloadGong(ctx context.Context, version, destPath string) er client := &http.Client{ Timeout: httpTimeout, } + req, err := http.NewRequestWithContext(ctx, http.MethodGet, downloadURL, nil) if err != nil { return fmt.Errorf("failed to create download request: %w", err) } + resp, err := client.Do(req) if err != nil { return fmt.Errorf("failed to download gong: %w", err) } defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { return fmt.Errorf("failed to download gong %s: server returned status %d", version, resp.StatusCode) } + // Create a temporary file in the same directory to ensure atomic write destDir := filepath.Dir(destPath) tmpFile, err := os.CreateTemp(destDir, "gong-download-*") @@ -267,20 +274,24 @@ func (g *Checker) downloadGong(ctx context.Context, version, destPath string) er return fmt.Errorf("failed to create temporary file: %w", err) } tmpPath := tmpFile.Name() + // Clean up temp file on error defer func() { if tmpPath != "" { os.Remove(tmpPath) } }() + _, err = io.Copy(tmpFile, resp.Body) if err != nil { tmpFile.Close() return fmt.Errorf("failed to write gong binary: %w", err) } + if err := tmpFile.Close(); err != nil { return fmt.Errorf("failed to close temporary file: %w", err) } + // Verify the downloaded binary against the published checksum manifest before // granting executable permissions or moving it to the final destination. // On any failure the deferred os.Remove cleans up tmpPath automatically. @@ -299,14 +310,17 @@ func (g *Checker) downloadGong(ctx context.Context, version, destPath string) er if err := os.Chmod(tmpPath, filePermissions); err != nil { return fmt.Errorf("failed to set executable permissions: %w", err) } + // Atomic rename if err := os.Rename(tmpPath, destPath); err != nil { return fmt.Errorf("failed to move gong binary to destination: %w", err) } tmpPath = "" // Prevent cleanup of the renamed file + _, _ = fmt.Fprintf(output, "\n") _, _ = fmt.Fprintf(output, "Installed gong %s, continuing...\n", version) _, _ = fmt.Fprintf(output, "===============================\n") _, _ = fmt.Fprintf(output, "\n") + return nil } diff --git a/pkg/gong/gong_test.go b/pkg/gong/gong_test.go index 719c2f8484..f3aa3b60ec 100644 --- a/pkg/gong/gong_test.go +++ b/pkg/gong/gong_test.go @@ -16,14 +16,21 @@ func TestBuildDownloadURL(t *testing.T) { url := buildDownloadURL(strings.TrimSpace(Version)) // URL should contain the base URL + assert.Contains(t, url, BaseURL) + // URL should contain the version + assert.Contains(t, url, strings.TrimSpace(Version)) // URL should follow the expected format + expectedOS := getOSName() + expectedArch := getArchName() + expectedURL := BaseURL + "/releases/" + strings.TrimSpace(Version) + "/" + expectedOS + "/gong_" + expectedArch + assert.Equal(t, expectedURL, url) } @@ -32,11 +39,15 @@ func TestGetOSName(t *testing.T) { osName := getOSName() + // Should return a valid OS name + validOSNames := []string{"darwin", "linux", "windows"} + if runtime.GOOS == "darwin" || runtime.GOOS == "linux" || runtime.GOOS == "windows" { assert.Contains(t, validOSNames, osName) } else { // For other OSes, it should return runtime.GOOS + assert.Equal(t, runtime.GOOS, osName) } } @@ -46,11 +57,15 @@ func TestGetArchName(t *testing.T) { archName := getArchName() + // Should return a valid architecture name + validArchNames := []string{"amd64", "arm64"} + if runtime.GOARCH == "amd64" || runtime.GOARCH == "arm64" { assert.Contains(t, validArchNames, archName) } else { // For other architectures, it should return runtime.GOARCH + assert.Equal(t, runtime.GOARCH, archName) } } @@ -59,15 +74,21 @@ func TestParseVersionOutput(t *testing.T) { t.Parallel() tests := []struct { - name string - input string + name string + + input string + expect string }{ {"standard output", "gong version 0.1.2", "v0.1.2"}, + {"already has v prefix", "gong version v0.1.2", "v0.1.2"}, + {"just version number", "0.1.2", "0.1.2"}, + {"empty string", "", ""}, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() @@ -82,7 +103,10 @@ func TestBuildDownloadURL_Format(t *testing.T) { url := buildDownloadURL(strings.TrimSpace(Version)) + // Verify the URL matches the expected pattern + // Format: {BaseURL}/releases/{Version}/{os}/gong_{arch} + assert.Regexp(t, `^https://storage\.googleapis\.com/gong-release/releases/v\d+\.\d+\.\d+/(darwin|linux|windows)/gong_(amd64|arm64)$`, url) } @@ -98,21 +122,31 @@ func TestChecker_SlotPerVersion(t *testing.T) { t.Parallel() c := &Checker{} + a := c.slotFor("v1.0.0") + b := c.slotFor("v1.0.0") + d := c.slotFor("v1.0.5") assert.Same(t, a, b, "same version returns the same slot") + assert.NotSame(t, a, d, "different versions get different slots") // dropSlot must remove only the matching slot. + c.dropSlot("v1.0.0", a) + a2 := c.slotFor("v1.0.0") + assert.NotSame(t, a, a2, "slot should be regenerated after drop") // dropSlot with a stale pointer is a no-op. + c.dropSlot("v1.0.0", a) + a3 := c.slotFor("v1.0.0") + assert.Same(t, a2, a3, "stale drop must not evict a fresh slot") } @@ -120,20 +154,29 @@ func TestChecker_SlotForIsConcurrencySafe(t *testing.T) { t.Parallel() c := &Checker{} + const goroutines = 32 + results := make([]*installSlot, goroutines) var wg sync.WaitGroup + for i := range goroutines { + wg.Add(1) + go func(idx int) { defer wg.Done() + results[idx] = c.slotFor("v1.0.0") }(i) + } + wg.Wait() require.NotNil(t, results[0]) + for _, slot := range results { assert.Same(t, results[0], slot, "all goroutines must observe the same slot for the same version") } From cd7ddb5007b41e49c3c3bc67e6ea12884118400b Mon Sep 17 00:00:00 2001 From: Musa Misto Date: Tue, 5 May 2026 18:50:16 +0300 Subject: [PATCH 5/5] refactor(tests): clean up whitespace and improve readability in gong_test.go --- pkg/gong/gong_test.go | 43 ++----------------------------------------- 1 file changed, 2 insertions(+), 41 deletions(-) diff --git a/pkg/gong/gong_test.go b/pkg/gong/gong_test.go index f3aa3b60ec..49a54f0948 100644 --- a/pkg/gong/gong_test.go +++ b/pkg/gong/gong_test.go @@ -16,21 +16,15 @@ func TestBuildDownloadURL(t *testing.T) { url := buildDownloadURL(strings.TrimSpace(Version)) // URL should contain the base URL - assert.Contains(t, url, BaseURL) // URL should contain the version - assert.Contains(t, url, strings.TrimSpace(Version)) // URL should follow the expected format - expectedOS := getOSName() - expectedArch := getArchName() - expectedURL := BaseURL + "/releases/" + strings.TrimSpace(Version) + "/" + expectedOS + "/gong_" + expectedArch - assert.Equal(t, expectedURL, url) } @@ -40,14 +34,11 @@ func TestGetOSName(t *testing.T) { osName := getOSName() // Should return a valid OS name - validOSNames := []string{"darwin", "linux", "windows"} - if runtime.GOOS == "darwin" || runtime.GOOS == "linux" || runtime.GOOS == "windows" { assert.Contains(t, validOSNames, osName) } else { // For other OSes, it should return runtime.GOOS - assert.Equal(t, runtime.GOOS, osName) } } @@ -58,14 +49,11 @@ func TestGetArchName(t *testing.T) { archName := getArchName() // Should return a valid architecture name - validArchNames := []string{"amd64", "arm64"} - if runtime.GOARCH == "amd64" || runtime.GOARCH == "arm64" { assert.Contains(t, validArchNames, archName) } else { // For other architectures, it should return runtime.GOARCH - assert.Equal(t, runtime.GOARCH, archName) } } @@ -74,25 +62,19 @@ func TestParseVersionOutput(t *testing.T) { t.Parallel() tests := []struct { - name string - - input string - + name string + input string expect string }{ {"standard output", "gong version 0.1.2", "v0.1.2"}, - {"already has v prefix", "gong version v0.1.2", "v0.1.2"}, - {"just version number", "0.1.2", "0.1.2"}, - {"empty string", "", ""}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - assert.Equal(t, tt.expect, parseVersionOutput(tt.input)) }) } @@ -104,9 +86,7 @@ func TestBuildDownloadURL_Format(t *testing.T) { url := buildDownloadURL(strings.TrimSpace(Version)) // Verify the URL matches the expected pattern - // Format: {BaseURL}/releases/{Version}/{os}/gong_{arch} - assert.Regexp(t, `^https://storage\.googleapis\.com/gong-release/releases/v\d+\.\d+\.\d+/(darwin|linux|windows)/gong_(amd64|arm64)$`, url) } @@ -114,7 +94,6 @@ func TestBuildDownloadURL_RespectsRequestedVersion(t *testing.T) { t.Parallel() url := buildDownloadURL("v9.9.9") - assert.Contains(t, url, "/releases/v9.9.9/") } @@ -122,31 +101,21 @@ func TestChecker_SlotPerVersion(t *testing.T) { t.Parallel() c := &Checker{} - a := c.slotFor("v1.0.0") - b := c.slotFor("v1.0.0") - d := c.slotFor("v1.0.5") assert.Same(t, a, b, "same version returns the same slot") - assert.NotSame(t, a, d, "different versions get different slots") // dropSlot must remove only the matching slot. - c.dropSlot("v1.0.0", a) - a2 := c.slotFor("v1.0.0") - assert.NotSame(t, a, a2, "slot should be regenerated after drop") // dropSlot with a stale pointer is a no-op. - c.dropSlot("v1.0.0", a) - a3 := c.slotFor("v1.0.0") - assert.Same(t, a2, a3, "stale drop must not evict a fresh slot") } @@ -156,27 +125,19 @@ func TestChecker_SlotForIsConcurrencySafe(t *testing.T) { c := &Checker{} const goroutines = 32 - results := make([]*installSlot, goroutines) var wg sync.WaitGroup - for i := range goroutines { - wg.Add(1) - go func(idx int) { defer wg.Done() - results[idx] = c.slotFor("v1.0.0") }(i) - } - wg.Wait() require.NotNil(t, results[0]) - for _, slot := range results { assert.Same(t, results[0], slot, "all goroutines must observe the same slot for the same version") }