Merge pull request #123 from shubham-stepsecurity/sm/fix

fix: harden macOS scans against IDE pop-ups and stuck processes

Author: ashishkurmi

.gitignore

@@ -20,8 +20,15 @@
 !docs/**/*.html
 !images/**/*.html
 
-# Go build artifacts — never commit compiled binaries
-**/stepsecurity-dev-machine-guard
+# Go build artifacts — never commit compiled binaries. Anchored to repo
+# root + the cmd/ subdirs so the pattern doesn't accidentally match the
+# source directory cmd/stepsecurity-dev-machine-guard/ and hide every
+# file inside it (the previous unanchored "**/stepsecurity-dev-machine-guard"
+# matched the directory itself, with the gitignore side-effect of
+# excluding all its contents from new tracking).
+/stepsecurity-dev-machine-guard
+/cmd/stepsecurity-dev-machine-guard/stepsecurity-dev-machine-guard
+/cmd/stepsecurity-dev-machine-guard-task/stepsecurity-dev-machine-guard-task
 *.exe
 dist/
 stepsecurity-dev-machine-guard-linux

.tool-versions

@@ -0,0 +1 @@
+golang 1.24.13

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

@@ -232,6 +232,7 @@ func main() {
 			log.Error("Enterprise configuration not found. Run '%s configure' or download the script from your StepSecurity dashboard.", os.Args[0])
 			os.Exit(1)
 		}
+		armExecutionWatchdog(telemetry.ExecutionDeadline(config.MaxExecutionDuration), log)
 		if err := telemetry.Run(exec, log, cfg); err != nil {
 			log.Error("%v", err)
 			os.Exit(1)
@@ -264,8 +265,19 @@ func main() {
 			log.Error("Scheduled installation is not supported on %s", runtime.GOOS)
 			os.Exit(1)
 		}
+
+		// Persist the loader-exported max-execution duration into config.json so
+		// scheduler-fired runs (launchd/systemd/schtasks) — which invoke the
+		// binary directly and never inherit the loader's exported env var — arm
+		// the watchdog with the same value. Best-effort: a write failure just
+		// means scheduled runs fall back to the binary's built-in default.
+		if err := config.PersistMaxExecutionDuration(os.Getenv(telemetry.EnvMaxExecutionDuration)); err != nil {
+			log.Warn("failed to persist max execution duration to config (%v) — scheduled runs will use the built-in default", err)
+		}
+
 		log.Progress("Sending initial telemetry...")
 		fmt.Println()
+		armExecutionWatchdog(telemetry.ExecutionDeadline(config.MaxExecutionDuration), log)
 		telemetryErr := telemetry.Run(exec, log, cfg)
 
 		// On Linux, systemd.Install enabled the timer but did not start it.
@@ -369,6 +381,7 @@ func main() {
 			}
 		case config.IsEnterpriseMode():
 			log.Debug("dispatch: enterprise telemetry (auto-detected)")
+			armExecutionWatchdog(telemetry.ExecutionDeadline(config.MaxExecutionDuration), log)
 			if err := telemetry.Run(exec, log, cfg); err != nil {
 				log.Error("%v", err)
 				os.Exit(1)

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

@@ -0,0 +1,46 @@
+//go:build !windows
+
+package main
+
+import (
+	"os"
+	"syscall"
+	"time"
+
+	"github.com/step-security/dev-machine-guard/internal/progress"
+)
+
+// armExecutionWatchdog spawns a single-shot goroutine that, after d
+// elapses, signals the process to exit. Graceful first (SIGTERM so the
+// signal handler at telemetry.go:259 posts the failure row, releases the
+// lock, and flushes the log tail), then a 5s hard-out via os.Exit(2) if
+// SIGTERM didn't take.
+//
+// The watchdog defends against goroutine wedges that the scan-deadline
+// context cancel can't reach: a hung HTTP upload bound to a stuck TCP
+// socket; an unreaped grandchild that slipped past the PR-122
+// process-group fix; a future leak we haven't anticipated. The scan
+// deadline (STEPSEC_MAX_SCAN_DURATION, 60min default) bounds the scan
+// body; this bounds the whole process.
+//
+// Goroutine lives in main rather than telemetry.Run so it survives any
+// panic/recover and any code path that doesn't return. No cancellation
+// path: the watchdog must fire even when the rest of the process is
+// wedged.
+//
+// d <= 0 disables the watchdog (caller passes the result of
+// telemetry.ExecutionDeadlineFromEnv which already returns 0 for the
+// "off"/"0" env settings).
+func armExecutionWatchdog(d time.Duration, log *progress.Logger) {
+	if d <= 0 {
+		return
+	}
+	go func() {
+		time.Sleep(d)
+		log.Warn("execution-watchdog: max duration %s exceeded — initiating shutdown", d)
+		_ = syscall.Kill(os.Getpid(), syscall.SIGTERM)
+		time.Sleep(5 * time.Second)
+		log.Error("execution-watchdog: SIGTERM did not exit in 5s — hard kill")
+		os.Exit(2)
+	}()
+}

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

@@ -0,0 +1,30 @@
+//go:build windows
+
+package main
+
+import (
+	"os"
+	"time"
+
+	"github.com/step-security/dev-machine-guard/internal/progress"
+)
+
+// armExecutionWatchdog (Windows): same intent as the Unix version but
+// skips the SIGTERM step. Windows console-signal mechanics (CTRL_C_EVENT
+// via os.Process.Signal(os.Interrupt)) only reach console-attached
+// processes, and the scheduled-task path under
+// stepsecurity-dev-machine-guard-task.exe runs without a console. Calling
+// os.Exit(2) directly skips the deferred lock release; the next launch's
+// loader-side stale-process killer (scripts/windows-device-agent/
+// stepsecurity-loader.ps1 Stop-StaleDmgProcesses) plus lock.Acquire's
+// own dead-PID handling clean up any stale state.
+func armExecutionWatchdog(d time.Duration, log *progress.Logger) {
+	if d <= 0 {
+		return
+	}
+	go func() {
+		time.Sleep(d)
+		log.Warn("execution-watchdog: max duration %s exceeded — exiting", d)
+		os.Exit(2)
+	}()
+}

internal/config/config.go

@@ -27,22 +27,33 @@ var (
 	InstallDir          string // "" means default (~/.stepsecurity); non-empty makes the agent put all its files (logs, hook errors, future state) under this directory. Bootstrap config.json itself stays at the legacy location. Per-run opt-out is the CLI flag --install-dir=. Resolution: --install-dir flag > STEPSECURITY_HOME env > this field > default — see internal/paths.
 )
 
+// MaxExecutionDuration is the whole-process execution-watchdog limit
+// (STEPSEC_MAX_EXECUTION_DURATION). Persisted into config.json at install time
+// so scheduler-fired runs (launchd/systemd/schtasks) — which invoke the binary
+// directly and never inherit the loader-exported env var — resolve the same
+// value the loader configured. "" means fall back to the binary's built-in
+// default (4h). Declared in its own var block (not the placeholder group
+// above) because it carries no build-time {{...}} placeholder. See
+// telemetry.ExecutionDeadline.
+var MaxExecutionDuration string
+
 // ConfigFile is the JSON structure persisted to ~/.stepsecurity/config.json.
 type ConfigFile struct {
-	CustomerID          string   `json:"customer_id,omitempty"`
-	APIEndpoint         string   `json:"api_endpoint,omitempty"`
-	APIKey              string   `json:"api_key,omitempty"`
-	ScanFrequencyHours  string   `json:"scan_frequency_hours,omitempty"`
-	SearchDirs          []string `json:"search_dirs,omitempty"`
-	EnableNPMScan       *bool    `json:"enable_npm_scan,omitempty"`
-	EnableBrewScan      *bool    `json:"enable_brew_scan,omitempty"`
-	EnablePythonScan    *bool    `json:"enable_python_scan,omitempty"`
-	IncludeTCCProtected *bool    `json:"include_tcc_protected,omitempty"`
-	ColorMode           string   `json:"color_mode,omitempty"`
-	OutputFormat        string   `json:"output_format,omitempty"`
-	HTMLOutputFile      string   `json:"html_output_file,omitempty"`
-	LogLevel            string   `json:"log_level,omitempty"`
-	InstallDir          string   `json:"install_dir,omitempty"`
+	CustomerID           string   `json:"customer_id,omitempty"`
+	APIEndpoint          string   `json:"api_endpoint,omitempty"`
+	APIKey               string   `json:"api_key,omitempty"`
+	ScanFrequencyHours   string   `json:"scan_frequency_hours,omitempty"`
+	SearchDirs           []string `json:"search_dirs,omitempty"`
+	EnableNPMScan        *bool    `json:"enable_npm_scan,omitempty"`
+	EnableBrewScan       *bool    `json:"enable_brew_scan,omitempty"`
+	EnablePythonScan     *bool    `json:"enable_python_scan,omitempty"`
+	IncludeTCCProtected  *bool    `json:"include_tcc_protected,omitempty"`
+	ColorMode            string   `json:"color_mode,omitempty"`
+	OutputFormat         string   `json:"output_format,omitempty"`
+	HTMLOutputFile       string   `json:"html_output_file,omitempty"`
+	LogLevel             string   `json:"log_level,omitempty"`
+	InstallDir           string   `json:"install_dir,omitempty"`
+	MaxExecutionDuration string   `json:"max_execution_duration,omitempty"`
 }
 
 // userConfigDir returns ~/.stepsecurity — the per-user config location.
@@ -172,6 +183,9 @@ func Load() {
 	if cfg.InstallDir != "" && InstallDir == "" {
 		InstallDir = cfg.InstallDir
 	}
+	if cfg.MaxExecutionDuration != "" && MaxExecutionDuration == "" {
+		MaxExecutionDuration = cfg.MaxExecutionDuration
+	}
 }
 
 // IsEnterpriseMode returns true if valid enterprise credentials are configured.
@@ -642,3 +656,24 @@ func RunConfigureNonInteractive(opts NonInteractiveOptions) error {
 	fmt.Printf("Configuration saved to %s\n", WriteConfigFilePath())
 	return nil
 }
+
+// PersistMaxExecutionDuration records the STEPSEC_MAX_EXECUTION_DURATION value
+// the loader exported into config.json at install time. Scheduler-fired runs
+// (launchd/systemd/schtasks) invoke the binary directly and never inherit the
+// loader's exported env var, so without this they fall back to the built-in 4h
+// default regardless of the loader's MAX_EXECUTION_DURATION_HOURS. Persisting
+// it lets telemetry.ExecutionDeadline pick it up on every scheduled run.
+// Read-modify-write so the loader-written customer_id/api_key/etc. survive.
+// No-op when value is empty (a direct binary install with no loader-configured
+// value keeps the built-in default).
+func PersistMaxExecutionDuration(value string) error {
+	if value == "" {
+		return nil
+	}
+	existing := loadExisting()
+	if existing.MaxExecutionDuration == value {
+		return nil
+	}
+	existing.MaxExecutionDuration = value
+	return save(existing)
+}

internal/config/max_execution_duration_test.go

@@ -0,0 +1,72 @@
+package config
+
+import "testing"
+
+// PersistMaxExecutionDuration is a read-modify-write: it records the loader's
+// max-execution value into config.json without disturbing the customer_id /
+// api_key / etc. the loader already wrote.
+func TestPersistMaxExecutionDuration_RoundTrip(t *testing.T) {
+	withHome(t)
+
+	// Seed config.json the way the loader's write_config would (no max-exec
+	// field), so we prove Persist preserves the existing fields.
+	seed := &ConfigFile{CustomerID: "acme", APIKey: "k", APIEndpoint: "https://api"}
+	if err := save(seed); err != nil {
+		t.Fatalf("seed save: %v", err)
+	}
+
+	if err := PersistMaxExecutionDuration("2h"); err != nil {
+		t.Fatalf("PersistMaxExecutionDuration: %v", err)
+	}
+
+	got := loadExisting()
+	if got.MaxExecutionDuration != "2h" {
+		t.Errorf("MaxExecutionDuration = %q, want %q", got.MaxExecutionDuration, "2h")
+	}
+	if got.CustomerID != "acme" || got.APIKey != "k" || got.APIEndpoint != "https://api" {
+		t.Errorf("read-modify-write clobbered existing fields: %+v", got)
+	}
+}
+
+// An empty value is a no-op (a direct binary install with no loader-exported
+// value must not write an empty field that would later parse to the default).
+func TestPersistMaxExecutionDuration_EmptyIsNoOp(t *testing.T) {
+	withHome(t)
+	if err := save(&ConfigFile{CustomerID: "acme"}); err != nil {
+		t.Fatalf("seed save: %v", err)
+	}
+
+	if err := PersistMaxExecutionDuration(""); err != nil {
+		t.Fatalf("empty should be a no-op, got: %v", err)
+	}
+
+	if got := loadExisting(); got.MaxExecutionDuration != "" {
+		t.Errorf("empty value should not be persisted, got %q", got.MaxExecutionDuration)
+	}
+}
+
+// Load() must surface a persisted max-execution value into the package var the
+// resolver reads on scheduler-fired runs.
+func TestLoad_PopulatesMaxExecutionDuration(t *testing.T) {
+	withHome(t)
+	seed := &ConfigFile{
+		CustomerID:           "acme",
+		APIKey:               "k",
+		APIEndpoint:          "https://api",
+		MaxExecutionDuration: "90m",
+	}
+	if err := save(seed); err != nil {
+		t.Fatalf("seed save: %v", err)
+	}
+
+	// Load only fills package vars still at their zero value; reset so this
+	// test is independent of execution order within the package.
+	MaxExecutionDuration = ""
+	t.Cleanup(func() { MaxExecutionDuration = "" })
+
+	Load()
+
+	if MaxExecutionDuration != "90m" {
+		t.Errorf("Load did not populate MaxExecutionDuration: got %q, want %q", MaxExecutionDuration, "90m")
+	}
+}