fix: preflight should check binary exists and directory executable (#170)

Author: iAziz786

app/services/updatepreflight/preflight.go

@@ -3,6 +3,9 @@ package updatepreflight
 import (
 	"fmt"
 	"os"
+	"path/filepath"
+
+	"golang.org/x/sys/unix"
 )
 
 const (
@@ -46,7 +49,7 @@ func New(cfg PreflightConfig) *PreflightChecker {
 func (p *PreflightChecker) Check(requiredSpace int64) *PreflightResult {
 	var errs []string
 
-	if err := p.checkBinaryWritable(); err != nil {
+	if err := p.checkInstallPath(); err != nil {
 		errs = append(errs, err.Error())
 	}
 
@@ -64,12 +67,21 @@ func (p *PreflightChecker) Check(requiredSpace int64) *PreflightResult {
 	}
 }
 
-func (p *PreflightChecker) checkBinaryWritable() error {
-	f, err := os.OpenFile(p.agentBinaryPath, os.O_WRONLY, 0)
-	if err != nil {
-		return fmt.Errorf("agent binary %s is not writable: %w", p.agentBinaryPath, err)
+func (p *PreflightChecker) checkInstallPath() error {
+	// Check that the binary exists
+	if _, err := os.Stat(p.agentBinaryPath); err != nil {
+		if os.IsNotExist(err) {
+			return fmt.Errorf("agent binary does not exist at %s", p.agentBinaryPath)
+		}
+		return fmt.Errorf("cannot stat agent binary %s: %w", p.agentBinaryPath, err)
 	}
-	f.Close()
+
+	// Check that the directory is writable (needed for atomic rename during upgrade)
+	dir := filepath.Dir(p.agentBinaryPath)
+	if err := unix.Access(dir, unix.W_OK); err != nil {
+		return fmt.Errorf("cannot write to install directory %s: %w", dir, err)
+	}
+
 	return nil
 }
 

app/services/updatepreflight/preflight_test.go

@@ -7,10 +7,16 @@ import (
 	"testing"
 )
 
-func TestCheck_BinaryWritable(t *testing.T) {
+func TestCheck_InstallDirNotWritable(t *testing.T) {
 	dir := t.TempDir()
-	binaryPath := filepath.Join(dir, "hostlink")
-	os.WriteFile(binaryPath, []byte("binary"), 0444)
+	// Create a read-only directory for the binary
+	readOnlyBinDir := filepath.Join(dir, "bin")
+	os.MkdirAll(readOnlyBinDir, 0755)
+	binaryPath := filepath.Join(readOnlyBinDir, "hostlink")
+	os.WriteFile(binaryPath, []byte("binary"), 0755)
+	// Make the directory read-only AFTER creating the file
+	os.Chmod(readOnlyBinDir, 0555)
+	t.Cleanup(func() { os.Chmod(readOnlyBinDir, 0755) }) // restore for cleanup
 
 	checker := New(PreflightConfig{
 		AgentBinaryPath: binaryPath,
@@ -20,12 +26,12 @@ func TestCheck_BinaryWritable(t *testing.T) {
 
 	result := checker.Check(10 * 1024 * 1024) // 10MB required
 	if result.Passed {
-		t.Error("expected Passed to be false when binary is not writable")
+		t.Error("expected Passed to be false when install directory is not writable")
 	}
-	assertContainsError(t, result.Errors, "not writable")
+	assertContainsError(t, result.Errors, "cannot write to install directory")
 }
 
-func TestCheck_BinaryWritable_Passes(t *testing.T) {
+func TestCheck_InstallPath_Passes(t *testing.T) {
 	dir := t.TempDir()
 	binaryPath := filepath.Join(dir, "hostlink")
 	os.WriteFile(binaryPath, []byte("binary"), 0755)
@@ -101,23 +107,31 @@ func TestCheck_DiskSpaceSufficient(t *testing.T) {
 
 func TestCheck_AllErrorsReported(t *testing.T) {
 	dir := t.TempDir()
-	binaryPath := filepath.Join(dir, "hostlink")
-	os.WriteFile(binaryPath, []byte("binary"), 0444) // not writable
 
-	readOnlyDir := filepath.Join(dir, "updates")
-	os.MkdirAll(readOnlyDir, 0555) // not writable
+	// Create binary in a read-only directory
+	readOnlyBinDir := filepath.Join(dir, "bin")
+	os.MkdirAll(readOnlyBinDir, 0755)
+	binaryPath := filepath.Join(readOnlyBinDir, "hostlink")
+	os.WriteFile(binaryPath, []byte("binary"), 0755)
+	os.Chmod(readOnlyBinDir, 0555) // make read-only after creating file
+	t.Cleanup(func() { os.Chmod(readOnlyBinDir, 0755) })
+
+	// Create read-only updates directory
+	readOnlyUpdatesDir := filepath.Join(dir, "updates")
+	os.MkdirAll(readOnlyUpdatesDir, 0555)
+	t.Cleanup(func() { os.Chmod(readOnlyUpdatesDir, 0755) })
 
 	checker := New(PreflightConfig{
 		AgentBinaryPath: binaryPath,
-		UpdatesDir:      readOnlyDir,
+		UpdatesDir:      readOnlyUpdatesDir,
 		StatFunc:        func(path string) (uint64, error) { return 1024, nil }, // tiny
 	})
 
 	result := checker.Check(10 * 1024 * 1024)
 	if result.Passed {
 		t.Error("expected Passed to be false")
 	}
-	// Should have 3 errors: binary not writable, dir not writable, disk space
+	// Should have 3 errors: install dir not writable, updates dir not writable, disk space
 	if len(result.Errors) < 3 {
 		t.Errorf("expected at least 3 errors, got %d: %v", len(result.Errors), result.Errors)
 	}
@@ -155,7 +169,30 @@ func TestCheck_BinaryNotExists(t *testing.T) {
 	if result.Passed {
 		t.Error("expected Passed to be false when binary does not exist")
 	}
-	assertContainsError(t, result.Errors, "not writable")
+	assertContainsError(t, result.Errors, "does not exist")
+}
+
+func TestCheck_BinaryExistsButDirNotWritable(t *testing.T) {
+	dir := t.TempDir()
+	// Create binary in a directory, then make the directory read-only
+	binDir := filepath.Join(dir, "usr", "bin")
+	os.MkdirAll(binDir, 0755)
+	binaryPath := filepath.Join(binDir, "hostlink")
+	os.WriteFile(binaryPath, []byte("binary"), 0755)
+	os.Chmod(binDir, 0555) // read-only after file creation
+	t.Cleanup(func() { os.Chmod(binDir, 0755) })
+
+	checker := New(PreflightConfig{
+		AgentBinaryPath: binaryPath,
+		UpdatesDir:      dir,
+		StatFunc:        func(path string) (uint64, error) { return 1 << 30, nil },
+	})
+
+	result := checker.Check(10 * 1024 * 1024)
+	if result.Passed {
+		t.Error("expected Passed to be false when binary directory is not writable")
+	}
+	assertContainsError(t, result.Errors, "cannot write to install directory")
 }
 
 // assertContainsError checks that at least one error string contains the substring.