diff --git a/environment/bundled_tools_path_test.go b/environment/bundled_tools_path_test.go new file mode 100644 index 0000000..61acd39 --- /dev/null +++ b/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)) + }) + } +} diff --git a/environment/environment.go b/environment/environment.go index 1e50270..a47ea45 100644 --- a/environment/environment.go +++ b/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 diff --git a/environment/environment_test.go b/environment/environment_test.go index 2307f83..02c99fc 100644 --- a/environment/environment_test.go +++ b/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")