launchd: replace plist template with XML encoder

Replace the template-based creation of a 'plist' file (used to configure a
launchd daemon) with XML marshalling. There are a couple of benefits to
configuring the 'plist' with Go's XML infrastructure, mainly built-in
character escaping and avoiding the fragility of defining the structured XML
format as a plaintext string.

As part of this change, add more tests of 'plist' content, and make the
tests more thoroughly verify file contents ensuring:

- that the generated plist has all the expected options
- that the plist doesn't include more options than it should
- that characters are properly escaped

Signed-off-by: Victoria Dye <[email protected]>

Author: vdye

internal/daemon/launchd.go

@@ -2,24 +2,44 @@ package daemon
 
 import (
 	"bytes"
+	"encoding/xml"
 	"fmt"
 	"path/filepath"
-	"text/template"
 
 	"github.com/github/git-bundle-server/internal/common"
 )
 
-const launchTemplate string = `<?xml version="1.0" encoding="UTF-8"?>
-<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
-<plist version="1.0">
-  <dict>
-    <key>Label</key><string>{{.Label}}</string>
-    <key>Program</key><string>{{.Program}}</string>
-    <key>StandardOutPath</key><string>{{.StdOut}}</string>
-    <key>StandardErrorPath</key><string>{{.StdErr}}</string>
-  </dict>
-</plist>
-`
+type xmlItem struct {
+	XMLName xml.Name
+	Value   string `xml:",chardata"`
+}
+
+type xmlArray struct {
+	XMLName  xml.Name
+	Elements []interface{} `xml:",any"`
+}
+
+type plist struct {
+	XMLName xml.Name `xml:"plist"`
+	Version string   `xml:"version,attr"`
+	Config  xmlArray `xml:"dict"`
+}
+
+const plistHeader string = `<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">`
+
+func xmlName(name string) xml.Name {
+	return xml.Name{Local: name}
+}
+
+func (p *plist) addKeyValue(key string, value any) {
+	p.Config.Elements = append(p.Config.Elements, xmlItem{XMLName: xmlName("key"), Value: key})
+	switch value := value.(type) {
+	case string:
+		p.Config.Elements = append(p.Config.Elements, xmlItem{XMLName: xmlName("string"), Value: value})
+	default:
+		panic("Invalid value type in 'addKeyValue'")
+	}
+}
 
 const domainFormat string = "gui/%s"
 
@@ -31,6 +51,19 @@ type launchdConfig struct {
 	StdErr string
 }
 
+func (c *launchdConfig) toPlist() *plist {
+	p := &plist{
+		Version: "1.0",
+		Config:  xmlArray{Elements: []interface{}{}},
+	}
+	p.addKeyValue("Label", c.Label)
+	p.addKeyValue("Program", c.Program)
+	p.addKeyValue("StandardOutPath", c.StdOut)
+	p.addKeyValue("StandardErrorPath", c.StdErr)
+
+	return p
+}
+
 type launchd struct {
 	user       common.UserProvider
 	cmdExec    common.CommandExecutor
@@ -104,11 +137,14 @@ func (l *launchd) Create(config *DaemonConfig, force bool) error {
 
 	// Generate the configuration
 	var newPlist bytes.Buffer
-	t, err := template.New(config.Label).Parse(launchTemplate)
+	newPlist.WriteString(xml.Header)
+	newPlist.WriteString(plistHeader)
+	encoder := xml.NewEncoder(&newPlist)
+	encoder.Indent("", "  ")
+	err := encoder.Encode(lConfig.toPlist())
 	if err != nil {
-		return fmt.Errorf("unable to generate launchd configuration: %w", err)
+		return fmt.Errorf("could not encode plist: %w", err)
 	}
-	t.Execute(&newPlist, lConfig)
 
 	// Check the existing file - if it's the same as the new content, do not overwrite
 	user, err := l.user.CurrentUser()

internal/daemon/launchd_test.go

@@ -5,14 +5,16 @@ import (
 	"fmt"
 	"os/user"
 	"path/filepath"
+	"regexp"
+	"strings"
 	"testing"
 
 	"github.com/github/git-bundle-server/internal/daemon"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 )
 
-var launchdCreateTests = []struct {
+var launchdCreateBehaviorTests = []struct {
 	title string
 
 	// Inputs
@@ -97,6 +99,74 @@ var launchdCreateTests = []struct {
 	},
 }
 
+var launchdCreatePlistTests = []struct {
+	title string
+
+	// Inputs
+	config *daemon.DaemonConfig
+
+	// Expected values
+	expectedPlistLines []string
+}{
+	{
+		title:  "Created plist is correct",
+		config: &basicDaemonConfig,
+		expectedPlistLines: []string{
+			`<?xml version="1.0" encoding="UTF-8"?>`,
+			`<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">`,
+			`<plist version="1.0">`,
+			"<dict>",
+
+			"<key>Label</key>",
+			fmt.Sprintf("<string>%s</string>", basicDaemonConfig.Label),
+
+			"<key>Program</key>",
+			fmt.Sprintf("<string>%s</string>", basicDaemonConfig.Program),
+
+			"<key>StandardOutPath</key>",
+			"<string>/dev/null</string>",
+
+			"<key>StandardErrorPath</key>",
+			"<string>/dev/null</string>",
+
+			"</dict>",
+			"</plist>",
+		},
+	},
+	{
+		title: "Plist contents are escaped",
+		config: &daemon.DaemonConfig{
+			// All of <'&"\t> should be replaced by the associated escape code
+			// 🤔 is in-range for XML (no replacement), but ￿ (\uFFFF) is
+			// out-of-range and replaced with � (\uFFFD)
+			// See https://www.w3.org/TR/xml11/Overview.html#charsets for details
+			Label:   "test-escape<'&\"	🤔￿>",
+			Program: "/path/to/the/program with a space",
+		},
+		expectedPlistLines: []string{
+			`<?xml version="1.0" encoding="UTF-8"?>`,
+			`<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">`,
+			`<plist version="1.0">`,
+			"<dict>",
+
+			"<key>Label</key>",
+			"<string>test-escape&lt;&#39;&amp;&#34;&#x9;🤔�&gt;</string>",
+
+			"<key>Program</key>",
+			"<string>/path/to/the/program with a space</string>",
+
+			"<key>StandardOutPath</key>",
+			"<string>/dev/null</string>",
+
+			"<key>StandardErrorPath</key>",
+			"<string>/dev/null</string>",
+
+			"</dict>",
+			"</plist>",
+		},
+	},
+}
+
 func TestLaunchd_Create(t *testing.T) {
 	// Set up mocks
 	testUser := &user.User{
@@ -114,7 +184,7 @@ func TestLaunchd_Create(t *testing.T) {
 	launchd := daemon.NewLaunchdProvider(testUserProvider, testCommandExecutor, testFileSystem)
 
 	// Verify launchd commands called
-	for _, tt := range launchdCreateTests {
+	for _, tt := range launchdCreateBehaviorTests {
 		forceArg := tt.force.toBoolList()
 		for _, force := range forceArg {
 			t.Run(fmt.Sprintf("%s (force='%t')", tt.title, force), func(t *testing.T) {
@@ -168,53 +238,61 @@ func TestLaunchd_Create(t *testing.T) {
 	}
 
 	// Verify content of created file
-	t.Run("Created file content and path are correct", func(t *testing.T) {
-		var actualFilename string
-		var actualFileBytes []byte
-
-		// Mock responses for successful fresh write
-		testCommandExecutor.On("Run",
-			"launchctl",
-			mock.MatchedBy(func(args []string) bool { return args[0] == "print" }),
-		).Return(daemon.LaunchdServiceNotFoundErrorCode, nil).Once()
-		testCommandExecutor.On("Run",
-			"launchctl",
-			mock.MatchedBy(func(args []string) bool { return args[0] == "bootstrap" }),
-		).Return(0, nil).Once()
-		testFileSystem.On("FileExists",
-			mock.AnythingOfType("string"),
-		).Return(false, nil).Once()
-
-		// Use mock to save off input args
-		testFileSystem.On("WriteFile",
-			mock.MatchedBy(func(filename string) bool {
-				actualFilename = filename
-				return true
-			}),
-			mock.MatchedBy(func(fileBytes any) bool {
-				// Save off value and always match
-				actualFileBytes = fileBytes.([]byte)
-				return true
-			}),
-		).Return(nil).Once()
-
-		err := launchd.Create(&basicDaemonConfig, false)
-		assert.Nil(t, err)
-		mock.AssertExpectationsForObjects(t, testCommandExecutor, testFileSystem)
-
-		// Check filename
-		expectedFilename := filepath.Clean(fmt.Sprintf("/my/test/dir/Library/LaunchAgents/%s.plist", basicDaemonConfig.Label))
-		assert.Equal(t, expectedFilename, actualFilename)
-
-		// Check file contents
-		err = xml.Unmarshal(actualFileBytes, new(interface{}))
-		if err != nil {
-			assert.Fail(t, "plist XML is malformed")
-		}
-		fileContents := string(actualFileBytes)
-		assert.Contains(t, fileContents, fmt.Sprintf("<key>Label</key><string>%s</string>", basicDaemonConfig.Label))
-		assert.Contains(t, fileContents, fmt.Sprintf("<key>Program</key><string>%s</string>", basicDaemonConfig.Program))
-	})
+	for _, tt := range launchdCreatePlistTests {
+		t.Run(tt.title, func(t *testing.T) {
+			var actualFilename string
+			var actualFileBytes []byte
+
+			// Mock responses for successful fresh write
+			testCommandExecutor.On("Run",
+				"launchctl",
+				mock.MatchedBy(func(args []string) bool { return args[0] == "print" }),
+			).Return(daemon.LaunchdServiceNotFoundErrorCode, nil).Once()
+			testCommandExecutor.On("Run",
+				"launchctl",
+				mock.MatchedBy(func(args []string) bool { return args[0] == "bootstrap" }),
+			).Return(0, nil).Once()
+			testFileSystem.On("FileExists",
+				mock.AnythingOfType("string"),
+			).Return(false, nil).Once()
+
+			// Use mock to save off input args
+			testFileSystem.On("WriteFile",
+				mock.MatchedBy(func(filename string) bool {
+					actualFilename = filename
+					return true
+				}),
+				mock.MatchedBy(func(fileBytes any) bool {
+					// Save off value and always match
+					actualFileBytes = fileBytes.([]byte)
+					return true
+				}),
+			).Return(nil).Once()
+
+			err := launchd.Create(tt.config, false)
+			assert.Nil(t, err)
+			mock.AssertExpectationsForObjects(t, testCommandExecutor, testFileSystem)
+
+			// Check filename
+			expectedFilename := filepath.Clean(fmt.Sprintf("/my/test/dir/Library/LaunchAgents/%s.plist", tt.config.Label))
+			assert.Equal(t, expectedFilename, actualFilename)
+
+			// Check XML
+			err = xml.Unmarshal(actualFileBytes, new(interface{}))
+			if err != nil {
+				assert.Fail(t, "plist XML is malformed")
+			}
+			fileContents := strings.TrimSpace(string(actualFileBytes))
+			plistLines := strings.Split(
+				regexp.MustCompile(`>\s*<`).ReplaceAllString(fileContents, ">\n<"), "\n")
+
+			assert.ElementsMatch(t, tt.expectedPlistLines, plistLines)
+
+			// Reset mocks
+			testCommandExecutor.Mock = mock.Mock{}
+			testFileSystem.Mock = mock.Mock{}
+		})
+	}
 }
 
 func TestLaunchd_Start(t *testing.T) {