Mitigate secret argv exposure and harden remote include write boundaries

docs/adr/48318-harden-secret-delivery-and-include-write-boundaries.md

@@ -0,0 +1,54 @@
+# ADR-48318: Harden Secret Delivery via Stdin and Enforce Include Write Boundaries
+
+**Date**: 2026-07-27
+**Status**: Draft
+**Deciders**: Unknown (generated from PR #48318 diff)
+
+---
+
+### Context
+
+VulnHunter flagged two credible security issues in the `gh-aw` CLI tool. First, calls to `gh secret set` passed secret values as a command-line argument via `--body <value>`, making credential material visible in process listings, shell history, and debug logs on the host machine. Second, the `fetchAndSaveRemoteIncludes` function allowed remote `@include` directives to specify paths that, after resolution, could escape the intended write directories (`.github/workflows` and `.github/shared`), enabling a path traversal attack that could overwrite arbitrary files on the local filesystem.
+
+Both issues affect the `pkg/cli` package and arise from missing or incorrect input validation at security boundaries: process argument handling and filesystem write operations.
+
+### Decision
+
+We will pass secret values to `gh secret set` exclusively via stdin using `RunGHInputContext` with `--body -`, eliminating argv exposure. We will also add a pre-write boundary check in `fetchAndSaveRemoteIncludes` using `fileutil.ValidatePathWithinBase` to reject any resolved target path that falls outside the allowed base directory for that include type (workflows directory for relative includes, shared directory for shared/workflowspec includes).
+
+### Alternatives Considered
+
+#### Alternative 1: Sanitize or Redact Secret Values Before Passing via Argv
+
+Secret values could be escaped, base64-encoded, or otherwise transformed before being passed as a command-line argument. This avoids changing the call interface but still places the transformed material in the process argument list, which remains readable in `/proc/<pid>/cmdline` and system audit logs during the brief execution window. It does not eliminate the exposure — it only obfuscates it — and any transformation must be reversed by the receiving process, adding complexity without a real security gain.
+
+#### Alternative 2: Write Secrets to a Temporary File and Pass the File Path
+
+The secret value could be written to a `mktemp`-created file, passed to `gh secret set` via `--body @<file>`, and then deleted. This keeps the secret out of argv but introduces new risks: the temp file may be readable by other users, the deletion may fail leaving the file on disk, and the lifecycle management adds code complexity. Stdin is simpler, ephemeral by nature, and the accepted standard for passing sensitive material to subprocesses.
+
+#### Alternative 3: Reject All Relative Include Paths (Only Allow Workflowspec Format)
+
+Relative `@include` paths (e.g., `@include helper.md`) could be entirely disallowed, requiring all includes to use fully-qualified workflowspec format. This eliminates the traversal surface for relative paths but breaks the existing documented feature of including workflow fragments co-located with the main workflow file, which is a common and legitimate use case. Restricting to a boundary check preserves the feature while blocking traversal.
+
+#### Alternative 4: Strip Path Traversal Sequences from the Include Path Before Resolution
+
+Normalize the include path by removing `..` components before computing the target path. Allowlist-style normalization is error-prone: character-encoding tricks (e.g., `%2e%2e`), symlink chains, or double-slash sequences can bypass naive stripping. Validating the fully-resolved target path against the base directory after all joins are computed is the canonical defense (TOCTOU-safe) and is already implemented by `fileutil.ValidatePathWithinBase`.
+
+### Consequences
+
+#### Positive
+- Secret values no longer appear in process argv (`/proc/<pid>/cmdline`), shell history for the parent process, or system audit logs at the point of the `gh` subprocess invocation.
+- Malicious or misconfigured remote `@include` paths can no longer cause writes outside `.github/workflows` or `.github/shared`, blocking the path traversal attack class.
+- Both fixes are covered by targeted regression tests that verify the security property directly (args log inspection for stdin delivery; `NoFileExists` assertion for traversal rejection).
+
+#### Negative
+- All `gh secret set` call sites must use the `RunGHInputContext` API variant, which requires a `context.Context` parameter; call sites without an available context must use `context.Background()` as a fallback, which is slightly less cancellation-friendly.
+- The write boundary check may reject edge-case include paths that were previously silently accepted (e.g., a path that resolves to a sibling of the target directory); authors of such includes will need to rewrite the path in a supported format.
+
+#### Neutral
+- The `fetchIncludeFromSource` package-level variable introduced to allow test injection is a dependency-inversion seam, consistent with how `downloadRemoteImportFile` is already structured; this is a test-enabling pattern, not a production-behavior change.
+- Both changes are backward-compatible with the external `gh` CLI interface — only the Go call sites inside `pkg/cli` are affected.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/cli/add_interactive_secrets.go

@@ -1,6 +1,7 @@
 package cli
 
 import (
+	"bytes"
 	"fmt"
 	"os"
 	"strings"
@@ -50,7 +51,7 @@ func (c *AddInteractiveConfig) checkExistingSecrets() error {
 
 // addRepositorySecret adds a secret to the repository
 func (c *AddInteractiveConfig) addRepositorySecret(name, value string) error {
-	output, err := workflow.RunGHCombined("Adding repository secret...", "secret", "set", name, "--repo", c.RepoOverride, "--body", value)
+	output, err := workflow.RunGHInputContext(c.Ctx, "Adding repository secret...", bytes.NewBufferString(value), "secret", "set", name, "--repo", c.RepoOverride)
 	if err != nil {
 		return fmt.Errorf("failed to set secret: %w (output: %s)", err, string(output))
 	}

pkg/cli/add_interactive_secrets_test.go

@@ -7,6 +7,8 @@ import (
 	"context"
 	"io"
 	"os"
+	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/github/gh-aw/pkg/console"
@@ -255,6 +257,37 @@ func TestParseSecretNames(t *testing.T) {
 	}
 }
 
+func TestAddInteractiveConfig_addRepositorySecret_UsesStdinForSecretValue(t *testing.T) {
+	fakeBinDir := t.TempDir()
+	fakeGH := filepath.Join(fakeBinDir, "gh")
+	argsLog := filepath.Join(fakeBinDir, "gh-args.log")
+	stdinLog := filepath.Join(fakeBinDir, "gh-stdin.log")
+	script := "#!/bin/sh\n" +
+		"printf '%s\\n' \"$*\" >> \"" + argsLog + "\"\n" +
+		"cat > \"" + stdinLog + "\"\n" +
+		"exit 0\n"
+	require.NoError(t, os.WriteFile(fakeGH, []byte(script), 0o755))
+	t.Setenv("PATH", fakeBinDir+string(os.PathListSeparator)+os.Getenv("PATH"))
+
+	config := &AddInteractiveConfig{
+		Ctx:          t.Context(),
+		RepoOverride: "owner/repo",
+	}
+	err := config.addRepositorySecret("TEST_SECRET", "super-secret-value")
+	require.NoError(t, err)
+
+	argsBytes, readArgsErr := os.ReadFile(argsLog)
+	require.NoError(t, readArgsErr)
+	args := string(argsBytes)
+	assert.Contains(t, args, "secret set TEST_SECRET --repo owner/repo")
+	assert.NotContains(t, args, "--body")
+	assert.NotContains(t, args, "super-secret-value")
+
+	stdinBytes, readStdinErr := os.ReadFile(stdinLog)
+	require.NoError(t, readStdinErr)
+	assert.Equal(t, "super-secret-value", strings.TrimSpace(string(stdinBytes)))
+}
+
 func TestAddInteractiveConfig_checkExistingSecrets(t *testing.T) {
 	config := &AddInteractiveConfig{
 		RepoOverride: "test-owner/test-repo",

pkg/cli/engine_secrets.go

@@ -1,6 +1,7 @@
 package cli
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"fmt"
@@ -27,8 +28,8 @@ var (
 	engineSecretsPromptFn = func(req SecretRequirement, config EngineSecretConfig) error {
 		return promptForSecret(req, config)
 	}
-	engineSecretsUploadFn = func(secretName, secretValue, repoSlug string, verbose bool, overwriteExisting bool) error {
-		return uploadSecretToRepo(secretName, secretValue, repoSlug, verbose, overwriteExisting)
+	engineSecretsUploadFn = func(ctx context.Context, secretName, secretValue, repoSlug string, verbose bool, overwriteExisting bool) error {
+		return uploadSecretToRepo(ctx, secretName, secretValue, repoSlug, verbose, overwriteExisting)
 	}
 )
 
@@ -288,15 +289,15 @@ func ensureSecretAvailable(req SecretRequirement, config EngineSecretConfig) err
 				console.PrintSuccessMessage(fmt.Sprintf("Found valid %s in environment", req.Name))
 				// Upload to repository if we have a repo slug
 				if config.RepoSlug != "" {
-					return engineSecretsUploadFn(req.Name, envValue, config.RepoSlug, config.Verbose, config.OverwriteExistingSecret)
+					return engineSecretsUploadFn(config.ctx(), req.Name, envValue, config.RepoSlug, config.Verbose, config.OverwriteExistingSecret)
 				}
 				return nil
 			}
 		} else {
 			console.PrintSuccessMessage(fmt.Sprintf("Found %s in environment", req.Name))
 			// Upload to repository if we have a repo slug
 			if config.RepoSlug != "" {
-				return engineSecretsUploadFn(req.Name, envValue, config.RepoSlug, config.Verbose, config.OverwriteExistingSecret)
+				return engineSecretsUploadFn(config.ctx(), req.Name, envValue, config.RepoSlug, config.Verbose, config.OverwriteExistingSecret)
 			}
 			return nil
 		}
@@ -391,7 +392,7 @@ func promptForCopilotPATUnified(req SecretRequirement, config EngineSecretConfig
 
 	// Upload to repository if we have a repo slug
 	if config.RepoSlug != "" {
-		return uploadSecretToRepo(req.Name, token, config.RepoSlug, config.Verbose, config.OverwriteExistingSecret)
+		return uploadSecretToRepo(config.ctx(), req.Name, token, config.RepoSlug, config.Verbose, config.OverwriteExistingSecret)
 	}
 
 	return nil
@@ -469,7 +470,7 @@ func promptForSystemTokenUnified(req SecretRequirement, config EngineSecretConfi
 
 	// Upload to repository if we have a repo slug
 	if config.RepoSlug != "" {
-		return uploadSecretToRepo(req.Name, token, config.RepoSlug, config.Verbose, config.OverwriteExistingSecret)
+		return uploadSecretToRepo(config.ctx(), req.Name, token, config.RepoSlug, config.Verbose, config.OverwriteExistingSecret)
 	}
 
 	return nil
@@ -521,7 +522,7 @@ func promptForGenericAPIKeyUnified(req SecretRequirement, config EngineSecretCon
 
 	// Upload to repository if we have a repo slug
 	if config.RepoSlug != "" {
-		return uploadSecretToRepo(req.Name, apiKey, config.RepoSlug, config.Verbose, config.OverwriteExistingSecret)
+		return uploadSecretToRepo(config.ctx(), req.Name, apiKey, config.RepoSlug, config.Verbose, config.OverwriteExistingSecret)
 	}
 
 	return nil
@@ -549,7 +550,7 @@ func checkOptionalSecret(req SecretRequirement, config EngineSecretConfig) error
 }
 
 // uploadSecretToRepo uploads a secret to the repository and can optionally replace an existing value.
-func uploadSecretToRepo(secretName, secretValue, repoSlug string, verbose bool, overwriteExisting bool) error {
+func uploadSecretToRepo(ctx context.Context, secretName, secretValue, repoSlug string, verbose bool, overwriteExisting bool) error {
 	engineSecretsLog.Printf("Uploading secret %s to %s", secretName, repoSlug)
 
 	// Check if secret already exists
@@ -571,7 +572,12 @@ func uploadSecretToRepo(secretName, secretValue, repoSlug string, verbose bool,
 		console.PrintInfoMessage(fmt.Sprintf("Uploading %s secret to repository", secretName))
 	}
 
-	output, err = workflow.RunGHCombined("Setting secret...", "secret", "set", secretName, "--repo", repoSlug, "--body", secretValue)
+	output, err = workflow.RunGHInputContext(
+		ctx,
+		"Setting secret...",
+		bytes.NewBufferString(secretValue),
+		"secret", "set", secretName, "--repo", repoSlug,
+	)
 	if err != nil {
 		return fmt.Errorf("failed to set %s secret: %w (output: %s)", secretName, err, string(output))
 	}

pkg/cli/engine_secrets_test.go

@@ -5,6 +5,8 @@ package cli
 import (
 	"net/url"
 	"os"
+	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -338,6 +340,39 @@ func TestStringContainsSecretName(t *testing.T) {
 	}
 }
 
+func TestUploadSecretToRepo_UsesStdinForSecretValue(t *testing.T) {
+	fakeBinDir := t.TempDir()
+	fakeGH := filepath.Join(fakeBinDir, "gh")
+	argsLog := filepath.Join(fakeBinDir, "gh-args.log")
+	stdinLog := filepath.Join(fakeBinDir, "gh-stdin.log")
+	script := "#!/bin/sh\n" +
+		"printf '%s\\n' \"$*\" >> \"" + argsLog + "\"\n" +
+		"if [ \"$1\" = \"secret\" ] && [ \"$2\" = \"list\" ]; then\n" +
+		"  exit 0\n" +
+		"fi\n" +
+		"if [ \"$1\" = \"secret\" ] && [ \"$2\" = \"set\" ]; then\n" +
+		"  cat > \"" + stdinLog + "\"\n" +
+		"  exit 0\n" +
+		"fi\n" +
+		"exit 1\n"
+	require.NoError(t, os.WriteFile(fakeGH, []byte(script), 0o755))
+	t.Setenv("PATH", fakeBinDir+string(os.PathListSeparator)+os.Getenv("PATH"))
+
+	err := uploadSecretToRepo(t.Context(), "TEST_SECRET", "super-secret-value", "owner/repo", false, true)
+	require.NoError(t, err)
+
+	argsBytes, readArgsErr := os.ReadFile(argsLog)
+	require.NoError(t, readArgsErr)
+	args := string(argsBytes)
+	assert.Contains(t, args, "secret set TEST_SECRET --repo owner/repo")
+	assert.NotContains(t, args, "--body")
+	assert.NotContains(t, args, "super-secret-value")
+
+	stdinBytes, readStdinErr := os.ReadFile(stdinLog)
+	require.NoError(t, readStdinErr)
+	assert.Equal(t, "super-secret-value", strings.TrimSpace(string(stdinBytes)))
+}
+
 func TestGetEngineSecretDescription(t *testing.T) {
 	tests := []struct {
 		name         string

pkg/cli/includes.go

@@ -23,6 +23,11 @@ import (
 var includeDirectivePattern = regexp.MustCompile(`^@include(\?)?\s+(.+)$`)
 var downloadRemoteImportFile = parser.DownloadFileFromGitHub
 
+// includesFetcher is the function type used by fetchAndSaveRemoteIncludes to retrieve
+// a single include file. Passing a non-nil value overrides the default FetchIncludeFromSource
+// implementation; this is used in tests to avoid real network calls.
+type includesFetcher func(ctx context.Context, includePath string, baseSpec *WorkflowSpec, verbose bool) ([]byte, string, error)
+
 // FetchIncludeFromSource fetches an include file from GitHub directly using a workflowspec format path.
 // The includePath should be in the format: owner/repo/path/to/file.md[@ref]
 // If the includePath is a relative path, it's resolved relative to the baseSpec.
@@ -435,9 +440,13 @@ func fetchFrontmatterImportsRecursive(ctx context.Context, content, currentBaseD
 	}
 }
 
-// fetchAndSaveRemoteIncludes parses the workflow content for @include directives and fetches them from the remote source
-func fetchAndSaveRemoteIncludes(ctx context.Context, content string, spec *WorkflowSpec, targetDir string, verbose bool, force bool, tracker *FileTracker) error {
+// fetchAndSaveRemoteIncludes parses the workflow content for @include directives and fetches them from the remote source.
+// The optional fetchFn parameter overrides the default FetchIncludeFromSource implementation; pass nil to use the default.
+func fetchAndSaveRemoteIncludes(ctx context.Context, content string, spec *WorkflowSpec, targetDir string, verbose bool, force bool, tracker *FileTracker, fetchFn includesFetcher) error {
 	remoteWorkflowLog.Printf("Fetching remote includes for workflow: %s", spec.String())
+	if fetchFn == nil {
+		fetchFn = FetchIncludeFromSource
+	}
 
 	// Parse the workflow content to find @include directives
 	scanner := bufio.NewScanner(strings.NewReader(content))
@@ -460,6 +469,11 @@ func fetchAndSaveRemoteIncludes(ctx context.Context, content string, spec *Workf
 			filePath = before
 		}
 
+		// Reject paths with traversal components before any further processing.
+		if strings.Contains(filePath, "..") {
+			return fmt.Errorf("include path %q contains illegal path traversal components", filePath)
+		}
+
 		// Skip if already processed
 		if setutil.Contains(seen, filePath) {
 			continue
@@ -468,7 +482,7 @@ func fetchAndSaveRemoteIncludes(ctx context.Context, content string, spec *Workf
 		}{}
 
 		// Fetch the include file
-		includeContent, _, err := FetchIncludeFromSource(ctx, includePath, spec, verbose)
+		includeContent, _, err := fetchFn(ctx, includePath, spec, verbose)
 		if err != nil {
 			if isOptional {
 				if verbose {
@@ -493,6 +507,13 @@ func fetchAndSaveRemoteIncludes(ctx context.Context, content string, spec *Workf
 			// Relative includes go alongside the workflow
 			targetPath = filepath.Join(targetDir, filePath)
 		}
+		writeBase := targetDir
+		if strings.HasPrefix(filePath, "shared/") || isWorkflowSpecFormat(filePath) {
+			writeBase = filepath.Join(filepath.Dir(targetDir), "shared")
+		}
+		if err := fileutil.ValidatePathWithinBase(writeBase, targetPath); err != nil {
+			return fmt.Errorf("refusing to write include outside allowed directory %s: %w", writeBase, err)
+		}
 
 		// Create target directory if needed
 		if err := os.MkdirAll(filepath.Dir(targetPath), constants.DirPermPublic); err != nil {
@@ -530,7 +551,7 @@ func fetchAndSaveRemoteIncludes(ctx context.Context, content string, spec *Workf
 		}
 
 		// Recursively fetch includes from the fetched file
-		if err := fetchAndSaveRemoteIncludes(ctx, string(includeContent), spec, targetDir, verbose, force, tracker); err != nil {
+		if err := fetchAndSaveRemoteIncludes(ctx, string(includeContent), spec, targetDir, verbose, force, tracker, fetchFn); err != nil {
 			if verbose {
 				fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to fetch nested includes from %s: %v", filePath, err)))
 			}
@@ -551,7 +572,7 @@ func fetchAndSaveRemoteIncludes(ctx context.Context, content string, spec *Workf
 func fetchAllRemoteDependencies(ctx context.Context, content string, spec *WorkflowSpec, targetDir string, verbose bool, force bool, tracker *FileTracker) error {
 	remoteWorkflowLog.Printf("Fetching all remote dependencies: spec=%s, targetDir=%s, force=%v", spec.String(), targetDir, force)
 	// Fetch and save @include directive dependencies (best-effort: errors are not fatal).
-	if err := fetchAndSaveRemoteIncludes(ctx, content, spec, targetDir, verbose, force, tracker); err != nil {
+	if err := fetchAndSaveRemoteIncludes(ctx, content, spec, targetDir, verbose, force, tracker, nil); err != nil {
 		if verbose {
 			fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to fetch include dependencies: %v", err)))
 		}