From 5dbf76a7aa58675a990194b86161c5ae15f84e24 Mon Sep 17 00:00:00 2001
From: Sudarsan Reddy <sreddy@cloudflare.com>
Date: Thu, 6 Apr 2023 15:32:41 +0100
Subject: [PATCH] TUN-7335: Fix cloudflared update not working in windows

This PR fixes some long standing bugs in the windows update
paths. We previously did not surface the errors at all leading to
this function failing silently.

This PR:

1. Now returns the ExitError if the bat run for update fails.
2. Fixes the errors surfaced by that return:
    a. The batch file doesnt play well with spaces. This is fixed by
    using PROGRA~1/2 which are aliases windows uses.
    b. The existing script also seemed to be irregular about where batch
    files were put and looked for. This is also fixed in this script.
---
 cmd/cloudflared/updater/update.go         | 14 ++++++++++++
 cmd/cloudflared/updater/workers_update.go | 26 ++++++++++++++++-------
 2 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/cmd/cloudflared/updater/update.go b/cmd/cloudflared/updater/update.go
index 5ad40dcb..31be284f 100644
--- a/cmd/cloudflared/updater/update.go
+++ b/cmd/cloudflared/updater/update.go
@@ -6,6 +6,7 @@ import (
 	"os"
 	"path/filepath"
 	"runtime"
+	"strings"
 	"time"
 
 	"github.com/facebookgo/grace/gracenet"
@@ -95,11 +96,24 @@ func CheckForUpdate(options updateOptions) (CheckResult, error) {
 		url = StagingUpdateURL
 	}
 
+	if runtime.GOOS == "windows" {
+		cfdPath = encodeWindowsPath(cfdPath)
+	}
+
 	s := NewWorkersService(version, url, cfdPath, Options{IsBeta: options.isBeta,
 		IsForced: options.isForced, RequestedVersion: options.intendedVersion})
 
 	return s.Check()
 }
+func encodeWindowsPath(path string) string {
+	// We do this because Windows allows spaces in directories such as
+	// Program Files but does not allow these directories to be spaced in batch files.
+	targetPath := strings.Replace(path, "Program Files (x86)", "PROGRA~2", -1)
+	// This is to do the same in 32 bit systems. We do this second so that the first
+	// replace is for x86 dirs.
+	targetPath = strings.Replace(targetPath, "Program Files", "PROGRA~1", -1)
+	return targetPath
+}
 
 func applyUpdate(options updateOptions, update CheckResult) UpdateOutcome {
 	if update.Version() == "" || options.updateDisabled {
diff --git a/cmd/cloudflared/updater/workers_update.go b/cmd/cloudflared/updater/workers_update.go
index 443f896d..b2d451a9 100644
--- a/cmd/cloudflared/updater/workers_update.go
+++ b/cmd/cloudflared/updater/workers_update.go
@@ -25,14 +25,13 @@ const (
 	// rename cloudflared.exe.new to cloudflared.exe
 	// delete cloudflared.exe.old
 	// start the service
-	// delete the batch file
-	windowsUpdateCommandTemplate = `@echo off
-sc stop cloudflared >nul 2>&1
+	// exit with code 0 if we've reached this point indicating success.
+	windowsUpdateCommandTemplate = `sc stop cloudflared >nul 2>&1
 rename "{{.TargetPath}}" {{.OldName}}
 rename "{{.NewPath}}" {{.BinaryName}}
 del "{{.OldPath}}"
 sc start cloudflared >nul 2>&1
-del {{.BatchName}}`
+exit /b 0`
 	batchFileName = "cfd_update.bat"
 )
 
@@ -214,8 +213,9 @@ func isValidChecksum(checksum, filePath string) error {
 // writeBatchFile writes a batch file out to disk
 // see the dicussion on why it has to be done this way
 func writeBatchFile(targetPath string, newPath string, oldPath string) error {
-	os.Remove(batchFileName) //remove any failed updates before download
-	f, err := os.Create(batchFileName)
+	batchFilePath := filepath.Join(filepath.Dir(targetPath), batchFileName)
+	os.Remove(batchFilePath) //remove any failed updates before download
+	f, err := os.Create(batchFilePath)
 	if err != nil {
 		return err
 	}
@@ -241,6 +241,16 @@ func writeBatchFile(targetPath string, newPath string, oldPath string) error {
 
 // run each OS command for windows
 func runWindowsBatch(batchFile string) error {
-	cmd := exec.Command("cmd", "/c", batchFile)
-	return cmd.Start()
+	defer os.Remove(batchFile)
+	cmd := exec.Command("cmd", "/C", batchFile)
+	_, err := cmd.Output()
+	// Remove the batch file we created. Don't let this interfere with the error
+	// we report.
+	if err != nil {
+		if exitError, ok := err.(*exec.ExitError); ok {
+			return fmt.Errorf("Error during update : %s;", string(exitError.Stderr))
+		}
+
+	}
+	return err
 }