Fix DeepSource review issues in self-updater

- Handle os.UserHomeDir() error to prevent state file in relative paths
- Restrict config dir permissions from 0o755 to 0o750
- Limit download size with io.LimitReader (50MB cap)
- Replace unused parameter `r` with `_` in test handler
- Refactor replaceBinary into testable replaceBinaryAt function
- Rewrite tests to call actual functions instead of reimplementing logic

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: sourya-deepsource

internal/update/updater.go

@@ -23,6 +23,9 @@ import (
 	"github.com/deepsourcelabs/cli/internal/debug"
 )
 
+// executablePath returns the current executable path. Overridable in tests.
+var executablePath = os.Executable
+
 // UpdateState is the on-disk state written by CheckForUpdate and consumed by ApplyUpdate.
 type UpdateState struct {
 	Version    string    `json:"version"`
@@ -32,14 +35,21 @@ type UpdateState struct {
 }
 
 // updateStatePath returns the path to the update state file (~/.deepsource/update.json).
-func updateStatePath() string {
-	home, _ := os.UserHomeDir()
-	return filepath.Join(home, buildinfo.ConfigDirName, "update.json")
+func updateStatePath() (string, error) {
+	home, err := os.UserHomeDir()
+	if err != nil {
+		return "", fmt.Errorf("getting home directory: %w", err)
+	}
+	return filepath.Join(home, buildinfo.ConfigDirName, "update.json"), nil
 }
 
 // ReadUpdateState reads the update state file. Returns nil if the file does not exist.
 func ReadUpdateState() (*UpdateState, error) {
-	data, err := os.ReadFile(updateStatePath())
+	p, err := updateStatePath()
+	if err != nil {
+		return nil, err
+	}
+	data, err := os.ReadFile(p)
 	if err != nil {
 		if errors.Is(err, os.ErrNotExist) {
 			return nil, nil
@@ -58,8 +68,11 @@ func writeUpdateState(s *UpdateState) error {
 	if err != nil {
 		return fmt.Errorf("marshaling update state: %w", err)
 	}
-	p := updateStatePath()
-	if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil {
+	p, err := updateStatePath()
+	if err != nil {
+		return err
+	}
+	if err := os.MkdirAll(filepath.Dir(p), 0o750); err != nil {
 		return fmt.Errorf("creating config dir: %w", err)
 	}
 	if err := os.WriteFile(p, data, 0o644); err != nil {
@@ -69,7 +82,11 @@ func writeUpdateState(s *UpdateState) error {
 }
 
 func clearUpdateState() {
-	_ = os.Remove(updateStatePath())
+	p, err := updateStatePath()
+	if err != nil {
+		return
+	}
+	_ = os.Remove(p)
 }
 
 // CheckForUpdate fetches the manifest, compares versions, and writes a state
@@ -208,7 +225,8 @@ func downloadFile(client *http.Client, url string) ([]byte, error) {
 		return nil, fmt.Errorf("download returned HTTP %d", resp.StatusCode)
 	}
 
-	data, err := io.ReadAll(resp.Body)
+	const maxUpdateSize = 50 * 1024 * 1024 // 50MB
+	data, err := io.ReadAll(io.LimitReader(resp.Body, maxUpdateSize))
 	if err != nil {
 		return nil, fmt.Errorf("reading download body: %w", err)
 	}
@@ -276,17 +294,21 @@ func extractFromZip(data []byte, binaryName string) ([]byte, error) {
 
 // replaceBinary atomically replaces the current executable with newBinary.
 func replaceBinary(newBinary []byte) error {
-	exe, err := os.Executable()
+	exe, err := executablePath()
 	if err != nil {
 		return fmt.Errorf("finding current executable: %w", err)
 	}
 	exe, err = filepath.EvalSymlinks(exe)
 	if err != nil {
 		return fmt.Errorf("resolving symlinks: %w", err)
 	}
+	return replaceBinaryAt(newBinary, exe)
+}
 
-	dir := filepath.Dir(exe)
-	base := filepath.Base(exe)
+// replaceBinaryAt atomically replaces the binary at destPath with newBinary.
+func replaceBinaryAt(newBinary []byte, destPath string) error {
+	dir := filepath.Dir(destPath)
+	base := filepath.Base(destPath)
 
 	// Write new binary to a temp file in the same directory (same filesystem for rename)
 	tmp, err := os.CreateTemp(dir, base+".new.*")
@@ -311,17 +333,17 @@ func replaceBinary(newBinary []byte) error {
 	}
 
 	// Rename current binary to .bak, then new to current
-	bakPath := exe + ".bak"
+	bakPath := destPath + ".bak"
 	_ = os.Remove(bakPath) // clean up any leftover .bak
 
-	if err := os.Rename(exe, bakPath); err != nil {
+	if err := os.Rename(destPath, bakPath); err != nil {
 		os.Remove(tmpPath)
 		return fmt.Errorf("backing up current binary: %w", err)
 	}
 
-	if err := os.Rename(tmpPath, exe); err != nil {
+	if err := os.Rename(tmpPath, destPath); err != nil {
 		// Try to restore the backup
-		_ = os.Rename(bakPath, exe)
+		_ = os.Rename(bakPath, destPath)
 		os.Remove(tmpPath)
 		return fmt.Errorf("replacing binary: %w", err)
 	}

internal/update/updater_test.go

@@ -86,32 +86,10 @@ func TestReplaceBinary(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// Point os.Executable to our fake binary by using a symlink
-	// Since we can't override os.Executable, test replaceBinary directly
-	// by calling the internal logic with a known path.
 	newContent := []byte("new binary content")
-
-	// Write new binary to temp, rename
-	tmp, err := os.CreateTemp(dir, "deepsource.new.*")
-	if err != nil {
-		t.Fatal(err)
-	}
-	if _, err := tmp.Write(newContent); err != nil {
-		t.Fatal(err)
-	}
-	if err := tmp.Chmod(0o755); err != nil {
-		t.Fatal(err)
-	}
-	tmp.Close()
-
-	bakPath := fakeBin + ".bak"
-	if err := os.Rename(fakeBin, bakPath); err != nil {
-		t.Fatal(err)
-	}
-	if err := os.Rename(tmp.Name(), fakeBin); err != nil {
-		t.Fatal(err)
+	if err := replaceBinaryAt(newContent, fakeBin); err != nil {
+		t.Fatalf("replaceBinaryAt: %v", err)
 	}
-	os.Remove(bakPath)
 
 	got, err := os.ReadFile(fakeBin)
 	if err != nil {
@@ -230,40 +208,41 @@ func TestCheckForUpdate_NewerVersion(t *testing.T) {
 	buildinfo.SetBuildInfo("2.0.30", "", "prod")
 
 	key := runtime.GOOS + "_" + runtime.GOARCH
-	manifest := Manifest{
-		Version: "2.0.40",
-		Platforms: map[string]PlatformInfo{
-			key: {
-				Archive: "deepsource_2.0.40_" + key + ".tar.gz",
-				SHA256:  "abc123",
-			},
-		},
-	}
-	// Simulate what CheckForUpdate does: fetch manifest, compare, write state
-	newer, _ := IsNewer("2.0.30", manifest.Version)
-	if !newer {
-		t.Fatal("expected newer=true")
-	}
-
-	platform := manifest.Platforms[key]
-	state := &UpdateState{
-		Version:    manifest.Version,
-		ArchiveURL: buildinfo.BaseURL + "/build/" + platform.Archive,
-		SHA256:     platform.SHA256,
-		CheckedAt:  time.Now().UTC(),
-	}
-	if err := writeUpdateState(state); err != nil {
-		t.Fatalf("writeUpdateState: %v", err)
+	manifestJSON := fmt.Sprintf(`{
+		"version": "2.0.40",
+		"platforms": {
+			%q: {
+				"archive": "deepsource_2.0.40_%s.tar.gz",
+				"sha256": "abc123"
+			}
+		}
+	}`, key, key)
+
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.Write([]byte(manifestJSON))
+	}))
+	defer srv.Close()
+
+	// Point manifest fetch at our test server
+	origBaseURL := buildinfo.BaseURL
+	buildinfo.BaseURL = srv.URL
+	defer func() { buildinfo.BaseURL = origBaseURL }()
+
+	if err := CheckForUpdate(srv.Client()); err != nil {
+		t.Fatalf("CheckForUpdate: %v", err)
 	}
 
 	got, err := ReadUpdateState()
 	if err != nil {
 		t.Fatalf("ReadUpdateState: %v", err)
 	}
+	if got == nil {
+		t.Fatal("expected non-nil state")
+	}
 	if got.Version != "2.0.40" {
 		t.Errorf("expected version 2.0.40, got %s", got.Version)
 	}
-	expectedURL := fmt.Sprintf("%s/build/deepsource_2.0.40_%s.tar.gz", buildinfo.BaseURL, key)
+	expectedURL := fmt.Sprintf("%s/build/deepsource_2.0.40_%s.tar.gz", srv.URL, key)
 	if got.ArchiveURL != expectedURL {
 		t.Errorf("expected archive URL %s, got %s", expectedURL, got.ArchiveURL)
 	}
@@ -299,7 +278,7 @@ func TestApplyUpdate_WithStateFile(t *testing.T) {
 	checksum := sha256.Sum256(archive)
 	checksumHex := hex.EncodeToString(checksum[:])
 
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
 		w.Write(archive)
 	}))
 	defer srv.Close()
@@ -314,42 +293,38 @@ func TestApplyUpdate_WithStateFile(t *testing.T) {
 		t.Fatalf("writeUpdateState: %v", err)
 	}
 
-	client := srv.Client()
-
-	// ApplyUpdate reads the state file internally, so we test it reads correctly.
-	// However, replaceBinary will use os.Executable() which we can't easily mock.
-	// So we test the pieces: state file reading, download, checksum verification.
-
-	readState, err := ReadUpdateState()
-	if err != nil {
-		t.Fatalf("ReadUpdateState: %v", err)
-	}
-	if readState.Version != "2.0.40" {
-		t.Fatalf("expected version 2.0.40, got %s", readState.Version)
+	// Create a fake binary so replaceBinary can replace it
+	fakeBin := filepath.Join(t.TempDir(), buildinfo.AppName)
+	if err := os.WriteFile(fakeBin, []byte("old binary"), 0o755); err != nil {
+		t.Fatal(err)
 	}
 
-	data, err := downloadFile(client, readState.ArchiveURL)
+	// Override executablePath to point to our fake binary
+	origExecPath := executablePath
+	executablePath = func() (string, error) { return fakeBin, nil }
+	defer func() { executablePath = origExecPath }()
+
+	ver, err := ApplyUpdate(srv.Client())
 	if err != nil {
-		t.Fatalf("downloadFile: %v", err)
+		t.Fatalf("ApplyUpdate: %v", err)
 	}
-
-	if err := verifyChecksum(data, readState.SHA256); err != nil {
-		t.Fatalf("verifyChecksum: %v", err)
+	if ver != "2.0.40" {
+		t.Errorf("expected version 2.0.40, got %s", ver)
 	}
 
-	extracted, err := extractFromTarGz(data, buildinfo.AppName)
+	// Verify the binary was actually replaced
+	got, err := os.ReadFile(fakeBin)
 	if err != nil {
-		t.Fatalf("extractFromTarGz: %v", err)
+		t.Fatal(err)
 	}
-	if !bytes.Equal(extracted, binaryContent) {
-		t.Errorf("extracted binary mismatch: got %q, want %q", extracted, binaryContent)
+	if !bytes.Equal(got, binaryContent) {
+		t.Errorf("binary content mismatch: got %q, want %q", got, binaryContent)
 	}
 
-	// Verify clearUpdateState removes the file
-	clearUpdateState()
-	afterClear, _ := ReadUpdateState()
-	if afterClear != nil {
-		t.Error("state file should be removed after clear")
+	// Verify state file was cleared
+	afterApply, _ := ReadUpdateState()
+	if afterApply != nil {
+		t.Error("state file should be removed after apply")
 	}
 }