update the macos sandbox, and policy mode configuration to make things more strict by default with fallback

Author: leafo

.github/workflows/test.yml

@@ -18,3 +18,5 @@ jobs:
       - if: runner.os == 'Linux'
         run: sudo apt-get install -y bubblewrap
       - run: go test -v -race ./...
+      - if: runner.os == 'macOS'
+        run: go test -v -count=1 ./runner

README.md

@@ -31,6 +31,7 @@ Shared sandbox settings are configured through `RunnerParams.SandboxConfig`:
 - `Type`: explicit backend (`"bubblewrap"`, `"firejail"`, `"flatpak"`) or auto (`""`)
 - `NoNetwork`: disable network access for the selected backend
 - `AllowEnv`: additional environment variable names to pass through from the host
+- `PolicyMode`: backend-specific policy mode (currently used by macOS `sandbox-exec`)
 
 Sandbox backends:
 
@@ -50,7 +51,13 @@ Backend selection:
 
 ### macOS
 
-Uses Apple's `sandbox-exec` with a generated [Seatbelt](https://reverse.put.as/wp-content/uploads/2011/09/Apple-Sandbox-Guide-v1.0.pdf) (SBPL) policy. The policy defaults to deny, then grants access to the game's install folder, system libraries, fonts, audio, and networking. For app bundles, a temporary shim `.app` wrapper is created that invokes `sandbox-exec` inside the bundle structure so that macOS treats it as a proper application.
+Uses Apple's `sandbox-exec` with a generated [Seatbelt](https://reverse.put.as/wp-content/uploads/2011/09/Apple-Sandbox-Guide-v1.0.pdf) (SBPL) policy. The policy defaults to deny, then grants access to the game's install folder plus required runtime resources. `SandboxConfig.NoNetwork` is supported on macOS and removes network rules from the generated profile. Environment forwarding in sandbox mode follows a strict allowlist baseline (including itch launch vars) plus `SandboxConfig.AllowEnv`.
+
+For app bundles, a temporary shim `.app` wrapper is created that invokes `sandbox-exec` inside the bundle structure so that macOS treats it as a proper application.
+
+Policy rollout mode can be controlled with `RunnerParams.SandboxConfig.PolicyMode`:
+- `"balanced"` (default)
+- `"legacy"` (compatibility fallback)
 
 ### Windows
 

runner/app_darwin.go

@@ -7,10 +7,17 @@ import (
 	"os"
 	"os/exec"
 	"os/signal"
+	"sort"
+	"strconv"
+	"strings"
 
 	"github.com/itchio/ox/macox"
 )
 
+var pgrepCommand = exec.Command
+var killCommand = exec.Command
+var openCommand = exec.Command
+
 type appRunner struct {
 	params       RunnerParams
 	target       *MacLaunchTarget
@@ -82,38 +89,68 @@ func RunAppBundle(params RunnerParams, bundlePath string) error {
 
 	consumer.Infof("Actual binary is (%s)", binaryPath)
 
-	cmd := exec.Command("open", args...)
+	cmd := openCommand("open", args...)
 	// I doubt this matters
 	cmd.Dir = params.Dir
 	cmd.Env = params.Env
 	// 'open' does not relay stdout or stderr, so we don't
 	// even bother setting them
 
+	preLaunchPIDs, err := matchingPIDs(binaryPath)
+	havePreLaunchSnapshot := err == nil
+	if err != nil {
+		consumer.Warnf("Could not snapshot existing app PIDs before launch: %s", err.Error())
+	}
+
 	processDone := make(chan struct{})
+	interruptSignals := make(chan os.Signal, 1)
+	signal.Notify(interruptSignals, os.Interrupt)
+	defer signal.Stop(interruptSignals)
 
 	go func() {
-		// catch SIGINT and kill the child if needed
-		c := make(chan os.Signal, 1)
-		signal.Notify(c, os.Interrupt)
-
 		consumer.Infof("Signal handler installed...")
 
 		// Block until a signal is received.
 		select {
 		case <-params.Ctx.Done():
 			consumer.Warnf("Context done!")
-		case s := <-c:
+		case s := <-interruptSignals:
 			consumer.Warnf("Got signal: %v", s)
 		case <-processDone:
 			return
 		}
 
 		consumer.Warnf("Killing app...")
-		// TODO: kill the actual binary, not the app
-		cmd := exec.Command("pkill", "-f", binaryPath)
-		err := cmd.Run()
+		postLaunchPIDs, err := matchingPIDs(binaryPath)
 		if err != nil {
-			consumer.Errorf("While killing: %s", err.Error())
+			consumer.Errorf("While discovering app PIDs: %s", err.Error())
+			if cmd.Process != nil {
+				_ = cmd.Process.Kill()
+			}
+			return
+		}
+
+		var killList []int
+		if havePreLaunchSnapshot {
+			killList = diffPIDs(postLaunchPIDs, preLaunchPIDs)
+		} else {
+			killList = mapKeys(postLaunchPIDs)
+		}
+
+		if len(killList) == 0 {
+			consumer.Warnf("No launch-specific process IDs found to terminate")
+			if cmd.Process != nil {
+				_ = cmd.Process.Kill()
+			}
+			return
+		}
+
+		for _, pid := range killList {
+			killCmd := killCommand("kill", "-TERM", strconv.Itoa(pid))
+			err = killCmd.Run()
+			if err != nil {
+				consumer.Warnf("Could not terminate pid %d: %s", pid, err.Error())
+			}
 		}
 	}()
 
@@ -125,3 +162,49 @@ func RunAppBundle(params RunnerParams, bundlePath string) error {
 
 	return nil
 }
+
+func matchingPIDs(binaryPath string) (map[int]struct{}, error) {
+	cmd := pgrepCommand("pgrep", "-f", binaryPath)
+	output, err := cmd.Output()
+	if err != nil {
+		if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 1 {
+			return map[int]struct{}{}, nil
+		}
+		return nil, fmt.Errorf("pgrep -f %q: %w", binaryPath, err)
+	}
+
+	pids := make(map[int]struct{})
+	for _, line := range strings.Split(strings.TrimSpace(string(output)), "\n") {
+		line = strings.TrimSpace(line)
+		if line == "" {
+			continue
+		}
+		pid, convErr := strconv.Atoi(line)
+		if convErr != nil {
+			return nil, fmt.Errorf("parsing pid %q: %w", line, convErr)
+		}
+		pids[pid] = struct{}{}
+	}
+	return pids, nil
+}
+
+func diffPIDs(after map[int]struct{}, before map[int]struct{}) []int {
+	var out []int
+	for pid := range after {
+		if _, found := before[pid]; found {
+			continue
+		}
+		out = append(out, pid)
+	}
+	sort.Ints(out)
+	return out
+}
+
+func mapKeys(pidSet map[int]struct{}) []int {
+	var out []int
+	for pid := range pidSet {
+		out = append(out, pid)
+	}
+	sort.Ints(out)
+	return out
+}

runner/app_darwin_test.go

@@ -0,0 +1,106 @@
+//go:build darwin
+
+package runner
+
+import (
+	"context"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func writeTestAppBundle(t *testing.T) string {
+	t.Helper()
+
+	bundleDir := filepath.Join(t.TempDir(), "TestHelper.app")
+	macosDir := filepath.Join(bundleDir, "Contents", "MacOS")
+	require.NoError(t, os.MkdirAll(macosDir, 0755))
+
+	execPath := filepath.Join(macosDir, "testhelper")
+	require.NoError(t, os.WriteFile(execPath, []byte("#!/bin/sh\nexit 0\n"), 0755))
+
+	plist := `<?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>CFBundleExecutable</key>
+	<string>testhelper</string>
+</dict>
+</plist>`
+	require.NoError(t, os.WriteFile(filepath.Join(bundleDir, "Contents", "Info.plist"), []byte(plist), 0644))
+
+	return bundleDir
+}
+
+func TestRunAppBundleCancellationKillsOnlyNewPIDs(t *testing.T) {
+	origPgrep := pgrepCommand
+	origKill := killCommand
+	origOpen := openCommand
+	t.Cleanup(func() {
+		pgrepCommand = origPgrep
+		killCommand = origKill
+		openCommand = origOpen
+	})
+
+	var pgrepCalls int
+	var killMu sync.Mutex
+	var killed []string
+
+	pgrepCommand = func(name string, args ...string) *exec.Cmd {
+		pgrepCalls++
+		if pgrepCalls == 1 {
+			return exec.Command("sh", "-c", "printf '10\n20\n'")
+		}
+		return exec.Command("sh", "-c", "printf '10\n20\n30\n40\n'")
+	}
+	killCommand = func(name string, args ...string) *exec.Cmd {
+		killMu.Lock()
+		killed = append(killed, args[len(args)-1])
+		killMu.Unlock()
+		return exec.Command("sh", "-c", "true")
+	}
+	openCommand = func(name string, args ...string) *exec.Cmd {
+		return exec.Command("sh", "-c", "sleep 0.2")
+	}
+
+	bundlePath := writeTestAppBundle(t)
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	go func() {
+		time.Sleep(50 * time.Millisecond)
+		cancel()
+	}()
+
+	err := RunAppBundle(
+		RunnerParams{
+			Consumer: newSandboxExecTestConsumer(t),
+			Ctx:      ctx,
+		},
+		bundlePath,
+	)
+	require.NoError(t, err)
+
+	killMu.Lock()
+	defer killMu.Unlock()
+	assert.Equal(t, []string{"30", "40"}, killed)
+}
+
+func TestDiffPIDsIsSortedAndExcludesExisting(t *testing.T) {
+	after := map[int]struct{}{
+		30: {},
+		40: {},
+		20: {},
+	}
+	before := map[int]struct{}{
+		20: {},
+	}
+
+	assert.Equal(t, []int{30, 40}, diffPIDs(after, before))
+}

runner/env_allowlist_darwin.go

@@ -0,0 +1,51 @@
+//go:build darwin
+
+package runner
+
+import "strings"
+
+var darwinBaseSandboxEnvVars = []string{
+	"HOME",
+	"USER",
+	"LOGNAME",
+	"PATH",
+	"LANG",
+	"LC_ALL",
+	"LC_CTYPE",
+	"TMP",
+	"TEMP",
+	"TMPDIR",
+}
+
+func darwinSandboxEnvAllowlist() []string {
+	allowlist := make([]string, 0, len(darwinBaseSandboxEnvVars)+len(ItchioLaunchEnvVars))
+	allowlist = append(allowlist, darwinBaseSandboxEnvVars...)
+	allowlist = append(allowlist, ItchioLaunchEnvVars...)
+	return allowlist
+}
+
+func collectAllowedEnvDarwin(paramsEnv []string, hostEnv []string, extraKeys []string) []string {
+	allowlist := darwinSandboxEnvAllowlist()
+	allowlist = append(allowlist, extraKeys...)
+	out := make([]string, 0, len(allowlist))
+	for _, key := range allowlist {
+		if val, found := envLookupWithPresenceDarwin(paramsEnv, key); found {
+			out = append(out, key+"="+val)
+			continue
+		}
+		if val, found := envLookupWithPresenceDarwin(hostEnv, key); found {
+			out = append(out, key+"="+val)
+		}
+	}
+	return out
+}
+
+func envLookupWithPresenceDarwin(env []string, key string) (string, bool) {
+	prefix := key + "="
+	for _, e := range env {
+		if strings.HasPrefix(e, prefix) {
+			return e[len(prefix):], true
+		}
+	}
+	return "", false
+}