Merge pull request #109 from shubham-stepsecurity/sm/update

fix(install-dir): make config field authoritative

Author: ashishkurmi

cmd/stepsecurity-dev-machine-guard/main.go

@@ -100,24 +100,32 @@ func main() {
 	exec := executor.NewReal()
 
 	// Install dir resolution (see internal/paths.Home for the canonical
-	// chain): --install-dir CLI flag > $STEPSECURITY_HOME env var >
-	// install_dir config field > ~/.stepsecurity default. The CLI flag
-	// wins because it is the most explicit per-invocation override the
-	// operator can supply. We feed it to paths via SetOverride; an
-	// explicit `--install-dir=` (empty) is preserved and short-circuits
-	// the path computation below to disable file logging for this run.
+	// chain): --install-dir CLI flag > install_dir config field >
+	// $STEPSECURITY_HOME env var > ~/.stepsecurity default. Config beats
+	// env because config.json is the source of truth that the loader
+	// scripts write to and operators hand-edit; the env var baked into
+	// service unit files at install time can otherwise become stale.
+	// An explicit `--install-dir=` (empty) routes through SetDisabled,
+	// after which paths.Home() returns "" so EVERY on-disk consumer
+	// (filelog, ai-agent hook errors, any future file) uniformly skips
+	// — not just file logging. cli.Parse rejects the empty form when
+	// paired with `install` / `uninstall`, where the platform installers
+	// need a real on-disk path for unit files and the log directory.
 	//
 	// The capture is installed before the logger so every subsequent
 	// stderr write — including the pipe-tee in
 	// internal/telemetry/logcapture.go, which nests inside this one —
 	// flows through to disk.
 	if cfg.InstallDirSet {
-		paths.SetOverride(cfg.InstallDir) // may be "" = disabled
+		if cfg.InstallDir == "" {
+			paths.SetDisabled()
+		} else {
+			paths.SetOverride(cfg.InstallDir)
+		}
 	}
-	installDir := paths.Home()
-	disabled := cfg.InstallDirSet && cfg.InstallDir == ""
+	installDir := paths.Home() // "" when SetDisabled or home unresolved
 	logFilePath := ""
-	if !disabled && installDir != "" {
+	if installDir != "" {
 		logFilePath = filepath.Join(installDir, filelog.Filename)
 		// Pre-rotate BOTH files unconditionally. In interactive mode the
 		// stderr rotation is redundant with filelog.Start's own rotation
@@ -164,7 +172,7 @@ func main() {
 	// auto-move — too risky for v1 (silent overwrites, races with other
 	// processes, perms changes). Just point at the leftovers.
 	legacy := paths.LegacyHome()
-	if !disabled && legacy != "" && installDir != "" && installDir != legacy {
+	if legacy != "" && installDir != "" && installDir != legacy {
 		if leftovers := findLegacyLeftovers(legacy); len(leftovers) > 0 {
 			log.Warn("install dir is %s but the legacy default %s still has files: %s — copy them over manually if you want their history.",
 				installDir, legacy, strings.Join(leftovers, ", "))

internal/cli/cli.go

@@ -256,6 +256,17 @@ func Parse(args []string) (*Config, error) {
 		return nil, fmt.Errorf("--npmrc and --pipconfig are mutually exclusive; pick one")
 	}
 
+	// --install-dir= (explicit empty) disables file logging by routing
+	// paths.Home() to "" globally. That conflicts with `install` /
+	// `uninstall`, whose platform installers (systemd / launchd) call
+	// os.MkdirAll(paths.Home()) and bake STEPSECURITY_HOME into the unit
+	// file — both break or write nonsense values when Home() is empty.
+	// Reject the combination here with a clear message rather than
+	// letting the installer fail opaquely on an empty path.
+	if cfg.InstallDirSet && cfg.InstallDir == "" && (cfg.Command == "install" || cfg.Command == "uninstall") {
+		return nil, fmt.Errorf("--install-dir= (empty) cannot be combined with %s — pass a directory or omit the flag", cfg.Command)
+	}
+
 	return cfg, nil
 }
 

internal/cli/cli_test.go

@@ -1,6 +1,7 @@
 package cli
 
 import (
+	"strings"
 	"testing"
 )
 
@@ -363,6 +364,40 @@ func TestParse_InstallDir_AbsentLeavesUnset(t *testing.T) {
 	}
 }
 
+// TestParse_InstallDir_EmptyRejectedForInstall guards against the
+// combination that would make systemd/launchd Install mkdir an empty
+// path: --install-dir= disables paths.Home() globally, but the install
+// command unconditionally calls os.MkdirAll(paths.Home()). Rejecting at
+// parse time gives the operator a clear error instead of an opaque
+// installer failure.
+func TestParse_InstallDir_EmptyRejectedForInstall(t *testing.T) {
+	for _, cmd := range []string{"install", "uninstall"} {
+		_, err := Parse([]string{cmd, "--install-dir="})
+		if err == nil {
+			t.Errorf("Parse(%q --install-dir=) returned nil error, want rejection", cmd)
+			continue
+		}
+		if !strings.Contains(err.Error(), "--install-dir=") || !strings.Contains(err.Error(), cmd) {
+			t.Errorf("Parse(%q --install-dir=) error %q should reference the flag and the command", cmd, err)
+		}
+	}
+}
+
+// TestParse_InstallDir_EmptyAllowedWithoutInstallCommand confirms the
+// existing "disable file logging" use case for ad-hoc scans still
+// works — the rejection only fires when paired with install/uninstall.
+func TestParse_InstallDir_EmptyAllowedWithoutInstallCommand(t *testing.T) {
+	for _, args := range [][]string{
+		{"--install-dir="},
+		{"send-telemetry", "--install-dir="},
+		{"configure", "show", "--install-dir="},
+	} {
+		if _, err := Parse(args); err != nil {
+			t.Errorf("Parse(%v) returned error %v; --install-dir= should still be valid outside install/uninstall", args, err)
+		}
+	}
+}
+
 func TestParseHooks_AcceptsInstallDir(t *testing.T) {
 	cfg, err := Parse([]string{"hooks", "install", "--install-dir=/opt/sec"})
 	if err != nil {

internal/launchd/launchd.go

@@ -2,6 +2,7 @@ package launchd
 
 import (
 	"context"
+	"encoding/xml"
 	"fmt"
 	"os"
 	"strconv"
@@ -14,6 +15,18 @@ import (
 	"github.com/step-security/dev-machine-guard/internal/progress"
 )
 
+// plistEscape XML-escapes a string for use inside a plist <string>
+// element. Without this, a path containing &, <, >, ', or " would
+// produce malformed XML that launchctl rejects with an opaque parse
+// error at load time. text/template's raw substitution does no
+// escaping by default — html/template would over-escape unrelated
+// content — so we route every templated value through this helper.
+func plistEscape(s string) string {
+	var sb strings.Builder
+	_ = xml.EscapeText(&sb, []byte(s))
+	return sb.String()
+}
+
 const (
 	label           = "com.stepsecurity.agent"
 	daemonPlistPath = "/Library/LaunchDaemons/com.stepsecurity.agent.plist"
@@ -114,14 +127,17 @@ func Install(exec executor.Executor, log *progress.Logger) error {
 		stepHome = paths.Home()
 	}
 
-	// Generate plist
+	// Generate plist. Every <string>-bound value is XML-escaped so a
+	// path with &, <, >, ', or " produces a well-formed plist that
+	// launchctl can actually load. IntervalSeconds is an integer, no
+	// escape needed; Label is a fixed constant.
 	plistData := plistTemplateData{
 		Label:            label,
-		BinaryPath:       binaryPath,
+		BinaryPath:       plistEscape(binaryPath),
 		IntervalSeconds:  intervalSeconds,
-		LogDir:           logDir,
-		UserHome:         userHome,
-		StepSecurityHome: stepHome,
+		LogDir:           plistEscape(logDir),
+		UserHome:         plistEscape(userHome),
+		StepSecurityHome: plistEscape(stepHome),
 	}
 
 	f, err := os.Create(plistPath)

internal/paths/paths.go

@@ -3,11 +3,21 @@
 // from a layered set of sources so callers can stop deriving
 // ~/.stepsecurity independently:
 //
-//  1. --install-dir CLI flag (set by main via SetOverride)
-//  2. $STEPSECURITY_HOME environment variable (set by service unit / loader)
-//  3. install_dir config field (loaded by internal/config)
+//  1. --install-dir CLI flag (set by main via SetOverride / SetDisabled)
+//  2. install_dir config field (loaded by internal/config)
+//  3. $STEPSECURITY_HOME environment variable (fallback only)
 //  4. ~/.stepsecurity (legacy default)
 //
+// Why config beats env: the install_dir field in config.json is the
+// canonical source of truth — the loader scripts (agent-api) write to
+// it on every run, and operators can hand-edit it. Service installers
+// (launchd / systemd / schtasks) bake STEPSECURITY_HOME into their unit
+// files at install time, but that snapshot becomes stale the moment the
+// operator edits config.json. Letting config win means scheduler-
+// invoked runs immediately reflect a config change without requiring
+// the operator to re-run `install`. The env var stays as a defensive
+// fallback for the rare case where config.json is unreadable.
+//
 // config.json itself stays at the legacy location regardless — see
 // internal/config.LegacyDir — so the agent can always bootstrap. All
 // other files (logs, hook errors, the binary placed by the loader) live
@@ -22,46 +32,73 @@ import (
 	"github.com/step-security/dev-machine-guard/internal/config"
 )
 
-// HomeEnvVar is the environment variable consulted in resolution
-// step 2. Service installers bake this into their unit files so
-// scheduler-invoked runs see the same install dir as interactive ones.
+// HomeEnvVar is the environment variable consulted as a defensive
+// fallback when both the CLI override and config.InstallDir are unset.
+// Service installers bake this into their unit files but its precedence
+// is intentionally below config so that an edited install_dir wins.
 const HomeEnvVar = "STEPSECURITY_HOME"
 
 // cliOverride captures the --install-dir CLI flag value (step 1).
 // Set once at startup by main; never mutated thereafter.
 var cliOverride string
 
+// cliDisabled records that --install-dir= (explicit empty) was passed.
+// It is distinct from "cliOverride is empty" because the empty string
+// is also the "unset" sentinel for cliOverride itself. When set, Home()
+// returns "" so every on-disk consumer — filelog, ai-agent hook errors,
+// any future file derived from Home() — uniformly skips file output.
+var cliDisabled bool
+
 // SetOverride installs the CLI-flag value. Called by main after
 // cli.Parse and before any code that calls Home() — see
 // cmd/stepsecurity-dev-machine-guard/main.go.
 func SetOverride(s string) {
 	cliOverride = s
+	cliDisabled = false
+}
+
+// SetDisabled marks the install dir as explicitly disabled for this
+// run. After this call Home() returns "" regardless of env/config/
+// legacy values, so no on-disk artifact is written under the resolved
+// install dir. Used by main when the operator passes --install-dir=
+// (empty) to silence per-run file logging.
+func SetDisabled() {
+	cliDisabled = true
+	cliOverride = ""
 }
 
 // Home returns the resolved install dir. Falls back to LegacyHome when
-// nothing else is set. Empty string is possible only when the home
-// directory itself cannot be resolved. A leading $HOME or ~ token in
-// any source is expanded via expandHome so the returned path is
-// canonical for the current OS, keeping the migration warning in main
-// from misfiring on hand-edited values like "$HOME/.stepsecurity" that
-// resolve to the legacy default.
-//
-// Note: this is a superset of resolveSearchDirs in internal/scan/scanner.go,
-// which only expands the exact literal "$HOME" — the install dir comes
-// from operator-edited config so it has to tolerate the "$HOME/foo" /
-// "~/foo" forms our docs use; search_dirs come from --search-dirs flag
-// values that operators don't combine with subpaths.
+// nothing else is set. Returns "" when SetDisabled() was called or
+// when the home directory itself cannot be resolved. A leading $HOME
+// or ~ token in any source is expanded via expandHome so the returned
+// path is canonical for the current OS; the result is run through
+// filepath.Clean so trailing slashes and "." / ".." components don't
+// cause spurious mismatches in the migration-warning equality check.
 func Home() string {
-	if cliOverride != "" {
-		return expandHome(cliOverride)
+	if cliDisabled {
+		return ""
 	}
-	if v := os.Getenv(HomeEnvVar); v != "" {
-		return expandHome(v)
+	// Read the env var once so a concurrent mutation (test helpers,
+	// future goroutine setup) can't flip the value between the switch
+	// guard and the assignment below — and so we don't pay for a second
+	// syscall on every lookup.
+	envHome := os.Getenv(HomeEnvVar)
+	var raw string
+	switch {
+	case cliOverride != "":
+		raw = cliOverride
+	case config.InstallDir != "":
+		raw = config.InstallDir
+	case envHome != "":
+		raw = envHome
+	default:
+		return LegacyHome()
 	}
-	if config.InstallDir != "" {
-		return expandHome(config.InstallDir)
+	resolved := expandHome(raw)
+	if resolved == "" {
+		return ""
 	}
-	return LegacyHome()
+	return filepath.Clean(resolved)
 }
 
 // expandHome replaces a leading $HOME or ~ token with the resolved