refactor(runner): route flashduty auth through bash extraEnv (drops flashduty_exec op)

environment/bundled_tools_path_test.go

@@ -0,0 +1,61 @@
+package environment
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// withBundledToolsPath must put the bundled-tools dir first on PATH so our
+// flashduty CLI shadows any same-named binary later on a BYOC host's PATH —
+// while staying a no-op when there's nothing to do.
+func TestWithBundledToolsPath(t *testing.T) {
+	cases := []struct {
+		name string
+		env  []string
+		dir  string
+		want []string
+	}{
+		{
+			name: "prepends to existing PATH",
+			env:  []string{"FOO=bar", "PATH=/usr/bin:/bin"},
+			dir:  "/opt/fd/bin",
+			want: []string{"FOO=bar", "PATH=/opt/fd/bin:/usr/bin:/bin"},
+		},
+		{
+			name: "already first is left untouched",
+			env:  []string{"PATH=/opt/fd/bin:/usr/bin"},
+			dir:  "/opt/fd/bin",
+			want: []string{"PATH=/opt/fd/bin:/usr/bin"},
+		},
+		{
+			name: "synthesizes PATH when absent",
+			env:  []string{"FOO=bar"},
+			dir:  "/opt/fd/bin",
+			want: []string{"FOO=bar", "PATH=/opt/fd/bin"},
+		},
+		{
+			name: "empty PATH value gets the dir",
+			env:  []string{"PATH="},
+			dir:  "/opt/fd/bin",
+			want: []string{"PATH=/opt/fd/bin"},
+		},
+		{
+			name: "empty dir is a no-op",
+			env:  []string{"PATH=/usr/bin"},
+			dir:  "",
+			want: []string{"PATH=/usr/bin"},
+		},
+		{
+			name: "dir present but not first is still prepended",
+			env:  []string{"PATH=/usr/bin:/opt/fd/bin"},
+			dir:  "/opt/fd/bin",
+			want: []string{"PATH=/opt/fd/bin:/usr/bin:/opt/fd/bin"},
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			assert.Equal(t, tc.want, withBundledToolsPath(tc.env, tc.dir))
+		})
+	}
+}

environment/environment.go

@@ -632,26 +632,112 @@ func (e *Environment) resolveTimeout(timeoutSec int) time.Duration {
 
 // executeBashCommand executes a bash command with the given parameters.
 //
-// extraEnv is merged on top of the runner process's own environment. Values
-// may be secrets — they are NEVER logged here or in callers. The bash command
-// should reference them as `$VAR_NAME` so expansion happens in-shell.
+// extraEnv is merged on top of a Flashduty-scrubbed view of the runner's
+// own environment: every FLASHDUTY_* key inherited from the parent process
+// is dropped before the merge, so the runner's own shell pollution never
+// reaches the subprocess. Per-call Flashduty credentials reach this path
+// only when safari's bash-side guard explicitly injects them into extraEnv
+// after recognizing a whitelisted CLI in the command — they win the merge
+// since extraEnv is layered last.
 func (e *Environment) executeBashCommand(ctx context.Context, command, workdir string, timeout time.Duration, extraEnv map[string]string) (*protocol.BashResult, error) {
 	ctx, cancel := context.WithTimeout(ctx, timeout)
 	defer cancel()
 
 	cmd := exec.CommandContext(ctx, "bash", "-c", command) //nolint:gosec // G204: command is user-initiated via workspace tool
 	cmd.Dir = workdir
-	if len(extraEnv) > 0 {
-		// Inherit parent env, then layer caller-provided entries on top so
-		// duplicates take the caller value (matches `exec.Command` semantics
-		// when cmd.Env is set — last KEY=VALUE wins).
-		merged := os.Environ()
-		for k, v := range extraEnv {
-			merged = append(merged, k+"="+v)
+	merged := scrubFlashdutySecrets(os.Environ())
+	for k, v := range extraEnv {
+		merged = append(merged, k+"="+v)
+	}
+	cmd.Env = withBundledToolsPath(merged, bundledToolsDir())
+	return runCapturedCommand(ctx, cmd)
+}
+
+// bundledToolsDir returns the directory holding the CLIs the runner ships with
+// it — the flashduty CLI in particular. Bash subprocesses must resolve OUR
+// bundled flashduty rather than whatever a BYOC host happens to have earlier on
+// PATH (a different version, or an unrelated binary of the same name).
+// FLASHDUTY_RUNNER_BIN_DIR overrides the location; the default is the directory
+// the runner executable lives in, since both the container image and install.sh
+// place the CLI next to the runner binary. Returns "" when the path can't be
+// determined, in which case PATH is left untouched.
+func bundledToolsDir() string {
+	if d := os.Getenv("FLASHDUTY_RUNNER_BIN_DIR"); d != "" {
+		return d
+	}
+	exe, err := os.Executable()
+	if err != nil {
+		return ""
+	}
+	return filepath.Dir(exe)
+}
+
+// withBundledToolsPath returns env with dir prepended to its PATH entry so the
+// bundled CLI shadows any same-named binary later on the host PATH. It is a
+// no-op when dir is empty or already the first PATH element, and synthesizes a
+// PATH entry if env had none.
+func withBundledToolsPath(env []string, dir string) []string {
+	if dir == "" {
+		return env
+	}
+	out := make([]string, 0, len(env)+1)
+	found := false
+	for _, e := range env {
+		if cur, ok := strings.CutPrefix(e, "PATH="); ok {
+			found = true
+			switch {
+			case cur == dir || strings.HasPrefix(cur, dir+":"):
+				out = append(out, e) // already first — leave as-is
+			case cur == "":
+				out = append(out, "PATH="+dir)
+			default:
+				out = append(out, "PATH="+dir+":"+cur)
+			}
+			continue
 		}
-		cmd.Env = merged
+		out = append(out, e)
+	}
+	if !found {
+		out = append(out, "PATH="+dir)
 	}
+	return out
+}
+
+// scrubFlashdutySecrets returns a copy of env with every entry whose key
+// starts with "FLASHDUTY_" removed. The intent is to keep ambient Flashduty
+// credentials (FLASHDUTY_APP_KEY, FLASHDUTY_API_BASE, …) out of bash
+// invocations even when the runner process itself was launched from a
+// shell that exports them (typical on BYOC dev workstations). Safari's
+// bash guard re-supplies them per call via wire-level extraEnv on calls
+// that genuinely invoke a Flashduty CLI.
+//
+// Implementation note: env entries are "KEY=VALUE"; we split on the first
+// "=" rather than checking the full prefix to avoid false negatives on
+// pathological inputs like "FLASHDUTY_=...".
+func scrubFlashdutySecrets(env []string) []string {
+	scrubbed := make([]string, 0, len(env))
+	for _, entry := range env {
+		eq := strings.IndexByte(entry, '=')
+		if eq < 0 {
+			scrubbed = append(scrubbed, entry)
+			continue
+		}
+		if strings.HasPrefix(entry[:eq], "FLASHDUTY_") {
+			continue
+		}
+		scrubbed = append(scrubbed, entry)
+	}
+	return scrubbed
+}
 
+// runCapturedCommand runs a prepared exec.Cmd and captures stdout/stderr with
+// a 10MB per-stream cap, then assembles a BashResult with truncation and exit
+// code populated.
+//
+// ctx MUST be the same WithTimeout-wrapped context exec.CommandContext was
+// built from — runCapturedCommand inspects ctx.Err() to distinguish a
+// deadline-induced kill from a genuine non-zero exit.
+func runCapturedCommand(ctx context.Context, cmd *exec.Cmd) (*protocol.BashResult, error) {
 	// 10MB per-stream cap prevents OOM from runaway commands while leaving
 	// plenty of headroom for normal LLM-context output.
 	const maxOutputSize = 10 * 1024 * 1024

environment/environment_test.go

@@ -328,6 +328,67 @@ func TestEnvironment_Bash_Env(t *testing.T) {
 	assert.Equal(t, "still-here", result.Stdout)
 }
 
+// TestEnvironment_Bash_ScrubsFlashdutySecrets pins the Phase 2 invariant
+// caught by Boss in E2E: even when the runner inherits FLASHDUTY_APP_KEY
+// from its launching shell, generic bash MUST NOT see it. Only
+// flashduty_exec receives those credentials, per-call via wire extraEnv.
+func TestEnvironment_Bash_ScrubsFlashdutySecrets(t *testing.T) {
+	ws := newTestEnvironment(t)
+	ctx := context.Background()
+
+	t.Setenv("FLASHDUTY_APP_KEY", "leaked-secret")
+	t.Setenv("FLASHDUTY_API_BASE", "http://leaked-host")
+	t.Setenv("KEEP_ME_AROUND", "non-secret-value")
+
+	result, err := ws.Bash(ctx, &protocol.BashArgs{
+		Command: `printf "APP=%s|BASE=%s|KEEP=%s" "${FLASHDUTY_APP_KEY:-EMPTY}" "${FLASHDUTY_API_BASE:-EMPTY}" "${KEEP_ME_AROUND:-EMPTY}"`,
+	})
+	require.NoError(t, err)
+	assert.Equal(t, "APP=EMPTY|BASE=EMPTY|KEEP=non-secret-value", result.Stdout,
+		"generic bash must not inherit FLASHDUTY_* from the runner process — Phase 2 contract violation")
+
+	// Same scrub applies even when extraEnv is non-empty (the scrub happens
+	// before the merge, so caller env is layered on top of a clean view).
+	result, err = ws.Bash(ctx, &protocol.BashArgs{
+		Command: `printf "APP=%s|EXTRA=%s" "${FLASHDUTY_APP_KEY:-EMPTY}" "${SOME_EXTRA:-EMPTY}"`,
+		Env:     map[string]string{"SOME_EXTRA": "from-caller"},
+	})
+	require.NoError(t, err)
+	assert.Equal(t, "APP=EMPTY|EXTRA=from-caller", result.Stdout)
+
+	// Caller is still free to set FLASHDUTY_* explicitly through extraEnv if
+	// they really want to (the scrub only removes ambient inheritance).
+	result, err = ws.Bash(ctx, &protocol.BashArgs{
+		Command: `printf %s "$FLASHDUTY_APP_KEY"`,
+		Env:     map[string]string{"FLASHDUTY_APP_KEY": "caller-supplied"},
+	})
+	require.NoError(t, err)
+	assert.Equal(t, "caller-supplied", result.Stdout)
+}
+
+// TestScrubFlashdutySecrets pins the helper's behavior independently of the
+// bash exec path: every FLASHDUTY_* key dropped, malformed entries kept
+// verbatim, non-matching keys untouched.
+func TestScrubFlashdutySecrets(t *testing.T) {
+	in := []string{
+		"FLASHDUTY_APP_KEY=secret",
+		"FLASHDUTY_API_BASE=http://x",
+		"FLASHDUTY_=edge",
+		"PATH=/usr/bin",
+		"KEEPME=ok",
+		"MALFORMED_ENTRY_NO_EQUALS",
+		"NEAR_FLASHDUTY_OK=keep",
+	}
+	out := scrubFlashdutySecrets(in)
+	want := []string{
+		"PATH=/usr/bin",
+		"KEEPME=ok",
+		"MALFORMED_ENTRY_NO_EQUALS",
+		"NEAR_FLASHDUTY_OK=keep",
+	}
+	assert.Equal(t, want, out)
+}
+
 func TestEnvironment_Bash_PermissionDenied(t *testing.T) {
 	tmpDir := t.TempDir()
 	// Note: rules are sorted alphabetically, so "echo *" comes after "*"
@@ -404,6 +465,28 @@ func TestSyncSkill_ProbeHit(t *testing.T) {
 	assert.Equal(t, resolveDir(t, dir), res.Path)
 }
 
+// TestEnvironment_Bash_ExtraEnvFlashdutyInjection confirms that the bash
+// path accepts injected FLASHDUTY_* values via extraEnv (the new safari
+// bash-guard path) and that they reach the subprocess even though the
+// scrub strips ambient inheritance. This pins the merge-order invariant
+// in executeBashCommand: scrubbed os.Environ first, extraEnv overlay last.
+func TestEnvironment_Bash_ExtraEnvFlashdutyInjection(t *testing.T) {
+	ws := newTestEnvironment(t)
+	ctx := context.Background()
+
+	// Parent process exports FLASHDUTY_APP_KEY — must be scrubbed.
+	t.Setenv("FLASHDUTY_APP_KEY", "ambient-leaked")
+
+	// Safari guard supplies a per-call key via extraEnv — must land in subprocess.
+	result, err := ws.Bash(ctx, &protocol.BashArgs{
+		Command: `printf %s "$FLASHDUTY_APP_KEY"`,
+		Env:     map[string]string{"FLASHDUTY_APP_KEY": "per-call-injected"},
+	})
+	require.NoError(t, err)
+	assert.Equal(t, "per-call-injected", result.Stdout,
+		"extraEnv overlay must win over the scrub — that's the inject-on-whitelist path")
+}
+
 func TestSyncSkill_InstallOverwrites(t *testing.T) {
 	ws := newTestEnvironment(t)
 	dir := filepath.Join(ws.Root(), "skills", "demo")