Confine subprocess diagnostics and retain redacted tails

internal/adapters/subprocess/diagnostics.go

@@ -18,6 +18,7 @@ const (
 	MaxStdoutDiagnosticBytes int64 = 8 * 1024 * 1024
 	// MaxStderrDiagnosticBytes bounds persisted stderr from one external command.
 	MaxStderrDiagnosticBytes int64 = 8 * 1024 * 1024
+	diagnosticTailBytes            = 2048
 )
 
 var inheritedEnvironmentNames = map[string]struct{}{
@@ -87,6 +88,7 @@ type diagnosticWriter struct {
 	received  int64
 	persisted int64
 	redactor  streamRedactor
+	tail      []byte
 }
 
 func openLogWriters(stdoutPath, stderrPath, owner string, sensitiveValues []string) (*logWriters, error) {
@@ -169,6 +171,7 @@ func (w *diagnosticWriter) writeRedacted(data []byte) error {
 	}
 	written, err := w.target.Write(toWrite)
 	w.persisted += int64(written)
+	w.retainTail(toWrite[:written])
 	w.logs.mu.Unlock()
 	if err != nil {
 		return err
@@ -179,6 +182,34 @@ func (w *diagnosticWriter) writeRedacted(data []byte) error {
 	return nil
 }
 
+func (w *diagnosticWriter) Tail() string {
+	w.logs.mu.Lock()
+	defer w.logs.mu.Unlock()
+	return strings.TrimSpace(string(w.tail))
+}
+
+func (w *diagnosticWriter) retainTail(data []byte) {
+	if len(data) >= diagnosticTailBytes {
+		if cap(w.tail) < diagnosticTailBytes {
+			w.tail = make([]byte, diagnosticTailBytes)
+		} else {
+			w.tail = w.tail[:diagnosticTailBytes]
+		}
+		copy(w.tail, data[len(data)-diagnosticTailBytes:])
+		return
+	}
+	if cap(w.tail) < diagnosticTailBytes {
+		retained := make([]byte, len(w.tail), diagnosticTailBytes)
+		copy(retained, w.tail)
+		w.tail = retained
+	}
+	if overflow := len(w.tail) + len(data) - diagnosticTailBytes; overflow > 0 {
+		copy(w.tail, w.tail[overflow:])
+		w.tail = w.tail[:len(w.tail)-overflow]
+	}
+	w.tail = append(w.tail, data...)
+}
+
 func (w *diagnosticWriter) reachLimit() error {
 	limit := &captureLimitError{stream: w.stream, owner: w.owner, limit: w.limit}
 	w.logs.mu.Lock()
@@ -224,7 +255,7 @@ func openDiagnosticFile(path string) (*os.File, error) {
 	if err := fileops.EnsureWorkspaceDirectory(filepath.Dir(path)); err != nil {
 		return nil, fmt.Errorf("create log directory for %q: %w", path, err)
 	}
-	file, err := os.Create(path)
+	file, err := fileops.OpenFileConfined(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, fileops.WorkspaceFileMode)
 	if err != nil {
 		return nil, fmt.Errorf("open log file %q: %w", path, err)
 	}

internal/adapters/subprocess/run.go

@@ -3,7 +3,6 @@ package subprocess
 import (
 	"context"
 	"fmt"
-	"io"
 	"os"
 	"os/exec"
 	"strings"
@@ -124,7 +123,7 @@ func Run(ctx context.Context, req RunRequest) (RunResult, error) {
 		return result, nil
 	}
 
-	stderrTail := readDiagnosticTail(req.StderrLogPath, 2048)
+	stderrTail := logs.stderr.Tail()
 	diagnostics := buildDiagnostics(req, result, stderrTail)
 
 	if captureLimit != nil {
@@ -208,36 +207,3 @@ func fdDiagnosticsHint(exitCode int, stderrTail string) string {
 	}
 	return ""
 }
-
-func readDiagnosticTail(path string, maxBytes int64) string {
-	if strings.TrimSpace(path) == "" || maxBytes <= 0 {
-		return ""
-	}
-	f, err := os.Open(path)
-	if err != nil {
-		return ""
-	}
-	defer f.Close()
-
-	info, err := f.Stat()
-	if err != nil {
-		return ""
-	}
-	size := info.Size()
-	start := int64(0)
-	if size > maxBytes {
-		start = size - maxBytes
-	}
-	if _, err := f.Seek(start, io.SeekStart); err != nil {
-		return ""
-	}
-	data, err := io.ReadAll(f)
-	if err != nil {
-		return ""
-	}
-	tail := strings.TrimSpace(string(data))
-	if tail == "" {
-		return ""
-	}
-	return tail
-}

internal/adapters/subprocess/run_test.go

@@ -3,9 +3,11 @@ package subprocess
 import (
 	"bytes"
 	"context"
+	"errors"
 	"os"
 	"os/exec"
 	"path/filepath"
+	"runtime"
 	"strconv"
 	"strings"
 	"testing"
@@ -134,6 +136,166 @@ func TestRunFailureRedactsSensitiveTail(t *testing.T) {
 	}
 }
 
+func TestRunRejectsSymlinkDiagnosticWithoutTruncatingTarget(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("creating symlinks requires privileges that are not available on every Windows runner")
+	}
+	exe, err := os.Executable()
+	if err != nil {
+		t.Fatalf("os.Executable() error = %v", err)
+	}
+
+	dir := t.TempDir()
+	targetPath := filepath.Join(dir, "outside.log")
+	const original = "must remain unchanged"
+	if err := os.WriteFile(targetPath, []byte(original), 0o600); err != nil {
+		t.Fatalf("WriteFile(target) error = %v", err)
+	}
+	stdoutPath := filepath.Join(dir, "stdout.log")
+	if err := os.Symlink(targetPath, stdoutPath); err != nil {
+		t.Fatalf("Symlink() error = %v", err)
+	}
+
+	_, err = Run(context.Background(), RunRequest{
+		Executable:    exe,
+		Args:          []string{"-test.run=^TestSubprocessHelper$", "--", "success"},
+		EnvOverrides:  map[string]string{"GO_WANT_SUBPROCESS_HELPER": "1"},
+		StdoutLogPath: stdoutPath,
+		StderrLogPath: filepath.Join(dir, "stderr.log"),
+	})
+	if err == nil || !strings.Contains(err.Error(), "symbolic link") {
+		t.Fatalf("Run() error = %v, want symbolic-link rejection", err)
+	}
+	data, readErr := os.ReadFile(targetPath)
+	if readErr != nil {
+		t.Fatalf("ReadFile(target) error = %v", readErr)
+	}
+	if string(data) != original {
+		t.Fatalf("target content = %q, want %q", data, original)
+	}
+}
+
+func TestRunFailureUsesOpenedDiagnosticAfterPathReplacement(t *testing.T) {
+	exe, err := os.Executable()
+	if err != nil {
+		t.Fatalf("os.Executable() error = %v", err)
+	}
+
+	dir := t.TempDir()
+	readyPath := filepath.Join(dir, "ready")
+	releasePath := filepath.Join(dir, "release")
+	stderrPath := filepath.Join(dir, "stderr.log")
+	openedPath := filepath.Join(dir, "opened-stderr.log")
+	const secretValue = "replacement-api-key-value"
+	const commandContent = "trusted command failure"
+	req := RunRequest{
+		Executable: exe,
+		Args:       []string{"-test.run=^TestSubprocessHelper$", "--", "delayed-fail"},
+		EnvOverrides: map[string]string{
+			"GO_WANT_SUBPROCESS_HELPER":      "1",
+			"API_KEY":                        secretValue,
+			"SUBPROCESS_HELPER_READY_PATH":   readyPath,
+			"SUBPROCESS_HELPER_RELEASE_PATH": releasePath,
+			"SUBPROCESS_HELPER_STDERR":       commandContent,
+		},
+		StdoutLogPath: filepath.Join(dir, "stdout.log"),
+		StderrLogPath: stderrPath,
+	}
+
+	resultCh := make(chan error, 1)
+	go func() {
+		_, runErr := Run(context.Background(), req)
+		resultCh <- runErr
+	}()
+	waitForHelperFile(t, readyPath)
+	if err := os.Rename(stderrPath, openedPath); err != nil {
+		t.Fatalf("Rename(stderr log) error = %v", err)
+	}
+	if err := os.WriteFile(stderrPath, []byte(secretValue), 0o600); err != nil {
+		t.Fatalf("WriteFile(replacement) error = %v", err)
+	}
+	if err := os.WriteFile(releasePath, []byte("continue"), 0o600); err != nil {
+		t.Fatalf("WriteFile(release) error = %v", err)
+	}
+
+	select {
+	case runErr := <-resultCh:
+		if runErr == nil {
+			t.Fatal("Run() error = nil, want command failure")
+		}
+		if strings.Contains(runErr.Error(), secretValue) {
+			t.Fatalf("error read replacement-path content: %q", runErr)
+		}
+		if !strings.Contains(runErr.Error(), commandContent) {
+			t.Fatalf("error = %q, want retained command diagnostic", runErr)
+		}
+	case <-time.After(3 * time.Second):
+		t.Fatal("Run() did not return after helper release")
+	}
+
+	openedData, err := os.ReadFile(openedPath)
+	if err != nil {
+		t.Fatalf("ReadFile(opened diagnostic) error = %v", err)
+	}
+	if !strings.Contains(string(openedData), commandContent) {
+		t.Fatalf("opened diagnostic = %q, want command content", openedData)
+	}
+	replacementData, err := os.ReadFile(stderrPath)
+	if err != nil {
+		t.Fatalf("ReadFile(replacement diagnostic) error = %v", err)
+	}
+	if string(replacementData) != secretValue {
+		t.Fatalf("replacement diagnostic = %q, want %q", replacementData, secretValue)
+	}
+}
+
+func TestRunRedactsSplitCredentialInSeparateAndSharedDiagnostics(t *testing.T) {
+	exe, err := os.Executable()
+	if err != nil {
+		t.Fatalf("os.Executable() error = %v", err)
+	}
+	const secretValue = "split-super-secret-value"
+
+	for _, shared := range []bool{false, true} {
+		t.Run(map[bool]string{false: "separate", true: "shared"}[shared], func(t *testing.T) {
+			dir := t.TempDir()
+			stdoutPath := filepath.Join(dir, "stdout.log")
+			stderrPath := filepath.Join(dir, "stderr.log")
+			if shared {
+				stderrPath = stdoutPath
+			}
+			req := RunRequest{
+				Executable: exe,
+				Args:       []string{"-test.run=^TestSubprocessHelper$", "--", "splitsecret"},
+				EnvOverrides: map[string]string{
+					"GO_WANT_SUBPROCESS_HELPER": "1",
+					"API_KEY":                   secretValue,
+				},
+				StdoutLogPath: stdoutPath,
+				StderrLogPath: stderrPath,
+			}
+
+			_, runErr := Run(context.Background(), req)
+			if runErr == nil {
+				t.Fatal("Run() error = nil, want command failure")
+			}
+			if strings.Contains(runErr.Error(), secretValue) || !strings.Contains(runErr.Error(), "<redacted>") {
+				t.Fatalf("error = %q, want redacted credential", runErr)
+			}
+			paths := map[string]struct{}{stdoutPath: {}, stderrPath: {}}
+			for path := range paths {
+				data, readErr := os.ReadFile(path)
+				if readErr != nil {
+					t.Fatalf("ReadFile(%q) error = %v", path, readErr)
+				}
+				if strings.Contains(string(data), secretValue) || !strings.Contains(string(data), "<redacted>") {
+					t.Fatalf("diagnostic %q = %q, want redacted credential", path, data)
+				}
+			}
+		})
+	}
+}
+
 func TestRunRedactsInheritedSensitiveEnvironment(t *testing.T) {
 	exe, err := os.Executable()
 	if err != nil {
@@ -251,6 +413,30 @@ func TestDiagnosticWriterHonorsExactLimitAndCapPlusOne(t *testing.T) {
 	}
 }
 
+func TestDiagnosticWriterRetainsBoundedRedactedTail(t *testing.T) {
+	logs := &logWriters{limits: make(chan *captureLimitError, 1)}
+	var output bytes.Buffer
+	secret := "credential-value"
+	writer := newDiagnosticWriter(logs, "stderr", "test", &output, 16*1024, []string{secret})
+	prefix := strings.Repeat("x", diagnosticTailBytes+512)
+	if _, err := writer.Write([]byte(prefix + secret[:7])); err != nil {
+		t.Fatalf("first Write() error = %v", err)
+	}
+	if _, err := writer.Write([]byte(secret[7:] + "-failure")); err != nil {
+		t.Fatalf("second Write() error = %v", err)
+	}
+	if err := writer.Flush(); err != nil {
+		t.Fatalf("Flush() error = %v", err)
+	}
+	tail := writer.Tail()
+	if len(tail) > diagnosticTailBytes {
+		t.Fatalf("retained tail length = %d, want at most %d", len(tail), diagnosticTailBytes)
+	}
+	if strings.Contains(tail, secret) || !strings.Contains(tail, "<redacted>-failure") {
+		t.Fatalf("retained tail = %q, want bounded redacted content", tail)
+	}
+}
+
 func TestRunFailureAddsBadDescriptorHint(t *testing.T) {
 	exe, err := os.Executable()
 	if err != nil {
@@ -489,6 +675,30 @@ func TestSubprocessHelper(t *testing.T) {
 	case "failbadfd":
 		_, _ = os.Stderr.WriteString("OSError: [Errno 9] Bad file descriptor\n")
 		os.Exit(120)
+	case "delayed-fail":
+		if err := os.WriteFile(os.Getenv("SUBPROCESS_HELPER_READY_PATH"), []byte("ready"), 0o600); err != nil {
+			os.Exit(4)
+		}
+		deadline := time.Now().Add(2 * time.Second)
+		for {
+			if _, err := os.Stat(os.Getenv("SUBPROCESS_HELPER_RELEASE_PATH")); err == nil {
+				break
+			} else if !errors.Is(err, os.ErrNotExist) || time.Now().After(deadline) {
+				os.Exit(5)
+			}
+			time.Sleep(10 * time.Millisecond)
+		}
+		_, _ = os.Stderr.WriteString(os.Getenv("SUBPROCESS_HELPER_STDERR"))
+		os.Exit(6)
+	case "splitsecret":
+		secret := os.Getenv("API_KEY")
+		split := len(secret) / 2
+		for _, stream := range []*os.File{os.Stdout, os.Stderr} {
+			_, _ = stream.WriteString(secret[:split])
+			time.Sleep(20 * time.Millisecond)
+			_, _ = stream.WriteString(secret[split:] + "\n")
+		}
+		os.Exit(7)
 	case "sleep":
 		time.Sleep(500 * time.Millisecond)
 		os.Exit(0)
@@ -546,3 +756,17 @@ func TestSubprocessHelper(t *testing.T) {
 		os.Exit(2)
 	}
 }
+
+func waitForHelperFile(t *testing.T, path string) {
+	t.Helper()
+	deadline := time.Now().Add(2 * time.Second)
+	for time.Now().Before(deadline) {
+		if _, err := os.Stat(path); err == nil {
+			return
+		} else if !errors.Is(err, os.ErrNotExist) {
+			t.Fatalf("Stat(%q) error = %v", path, err)
+		}
+		time.Sleep(10 * time.Millisecond)
+	}
+	t.Fatalf("helper file %q was not created", path)
+}

internal/fileops/confined.go

@@ -184,15 +184,21 @@ func syncOpenedDirectory(parent *os.Root) error {
 }
 
 // OpenFileConfined opens a file after verifying its parent hierarchy without
-// following symbolic links. Existing symbolic-link leaves are rejected.
+// following symbolic links. Existing symbolic-link and non-regular leaves are
+// rejected.
 func OpenFileConfined(path string, flags int, mode os.FileMode) (*os.File, error) {
 	parent, name, err := openConfinedParent(path, false, 0)
 	if err != nil {
 		return nil, err
 	}
 	defer func() { _ = parent.Close() }()
-	if info, err := parent.Lstat(name); err == nil && info.Mode()&os.ModeSymlink != 0 {
+	if info, err := parent.Lstat(name); err == nil {
-		return nil, fmt.Errorf("destination file %q is a symbolic link", name)
+		if info.Mode()&os.ModeSymlink != 0 {
+			return nil, fmt.Errorf("destination file %q is a symbolic link", name)
+		}
+		if !info.Mode().IsRegular() {
+			return nil, fmt.Errorf("destination file %q is not a regular file", name)
+		}
 	} else if err != nil && !errors.Is(err, os.ErrNotExist) {
 		return nil, fmt.Errorf("inspect destination file %q: %w", name, err)
 	}
@@ -206,7 +212,7 @@ func OpenFileConfined(path string, flags int, mode os.FileMode) (*os.File, error
 		return nil, fmt.Errorf("inspect opened destination file %q: %w", name, err)
 	}
 	current, err := parent.Lstat(name)
-	if err != nil || current.Mode()&os.ModeSymlink != 0 || !os.SameFile(opened, current) {
+	if err != nil || !opened.Mode().IsRegular() || current.Mode()&os.ModeSymlink != 0 || !current.Mode().IsRegular() || !os.SameFile(opened, current) {
 		_ = file.Close()
 		if err != nil {
 			return nil, fmt.Errorf("reinspect destination file %q: %w", name, err)

internal/fileops/confined_open_test.go

@@ -0,0 +1,56 @@
+package fileops
+
+import (
+	"os"
+	"path/filepath"
+	"runtime"
+	"strings"
+	"testing"
+)
+
+func TestOpenFileConfinedRejectsNonRegularLeaf(t *testing.T) {
+	leaf := filepath.Join(t.TempDir(), "diagnostic.log")
+	if err := os.Mkdir(leaf, 0o700); err != nil {
+		t.Fatalf("Mkdir() error = %v", err)
+	}
+	file, err := OpenFileConfined(leaf, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, WorkspaceFileMode)
+	if file != nil {
+		_ = file.Close()
+		t.Fatal("OpenFileConfined() returned a file for a directory")
+	}
+	if err == nil || !strings.Contains(err.Error(), "not a regular file") {
+		t.Fatalf("OpenFileConfined() error = %v, want regular-file rejection", err)
+	}
+}
+
+func TestOpenFileConfinedRejectsSymlinkWithoutTruncatingTarget(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("creating symlinks requires privileges that are not available on every Windows runner")
+	}
+	dir := t.TempDir()
+	target := filepath.Join(dir, "target.log")
+	const original = "outside content"
+	if err := os.WriteFile(target, []byte(original), 0o600); err != nil {
+		t.Fatalf("WriteFile(target) error = %v", err)
+	}
+	leaf := filepath.Join(dir, "diagnostic.log")
+	if err := os.Symlink(target, leaf); err != nil {
+		t.Fatalf("Symlink() error = %v", err)
+	}
+
+	file, err := OpenFileConfined(leaf, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, WorkspaceFileMode)
+	if file != nil {
+		_ = file.Close()
+		t.Fatal("OpenFileConfined() returned a file for a symbolic link")
+	}
+	if err == nil || !strings.Contains(err.Error(), "symbolic link") {
+		t.Fatalf("OpenFileConfined() error = %v, want symbolic-link rejection", err)
+	}
+	data, readErr := os.ReadFile(target)
+	if readErr != nil {
+		t.Fatalf("ReadFile(target) error = %v", readErr)
+	}
+	if string(data) != original {
+		t.Fatalf("target content = %q, want %q", data, original)
+	}
+}