diff --git a/docs/adr/48318-harden-secret-delivery-and-include-write-boundaries.md b/docs/adr/48318-harden-secret-delivery-and-include-write-boundaries.md new file mode 100644 index 00000000000..6fe7fbaf61a --- /dev/null +++ b/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 `, 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//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 @`, 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//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.* diff --git a/pkg/cli/add_interactive_secrets.go b/pkg/cli/add_interactive_secrets.go index e35e345e2f9..35caabf0ffe 100644 --- a/pkg/cli/add_interactive_secrets.go +++ b/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)) } diff --git a/pkg/cli/add_interactive_secrets_test.go b/pkg/cli/add_interactive_secrets_test.go index a5eaf7f7cb8..730bfffede5 100644 --- a/pkg/cli/add_interactive_secrets_test.go +++ b/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", diff --git a/pkg/cli/engine_secrets.go b/pkg/cli/engine_secrets.go index 6b12c577a70..10d833dbe9d 100644 --- a/pkg/cli/engine_secrets.go +++ b/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,7 +289,7 @@ 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 } @@ -296,7 +297,7 @@ func ensureSecretAvailable(req SecretRequirement, config EngineSecretConfig) err 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)) } diff --git a/pkg/cli/engine_secrets_test.go b/pkg/cli/engine_secrets_test.go index 3d25361707e..64963e88ec2 100644 --- a/pkg/cli/engine_secrets_test.go +++ b/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 diff --git a/pkg/cli/includes.go b/pkg/cli/includes.go index 3f306056436..3aedb871723 100644 --- a/pkg/cli/includes.go +++ b/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))) } diff --git a/pkg/cli/remote_workflow_test.go b/pkg/cli/remote_workflow_test.go index a75db064223..d136d65935f 100644 --- a/pkg/cli/remote_workflow_test.go +++ b/pkg/cli/remote_workflow_test.go @@ -660,6 +660,58 @@ imports: } } +func TestFetchAndSaveRemoteIncludes_PathTraversalRejected(t *testing.T) { + mockFetch := func(_ context.Context, _ string, _ *WorkflowSpec, _ bool) ([]byte, string, error) { + return []byte("# include body\n"), "", nil + } + + tmpDir := t.TempDir() + targetDir := filepath.Join(tmpDir, ".github", "workflows") + require.NoError(t, os.MkdirAll(targetDir, 0o755)) + + spec := &WorkflowSpec{ + RepoSpec: RepoSpec{RepoSlug: "github/gh-aw", Version: "main"}, + } + err := fetchAndSaveRemoteIncludes(t.Context(), "@include ../secrets/evil.md\n", spec, targetDir, false, false, nil, mockFetch) + require.Error(t, err) + require.NoFileExists(t, filepath.Join(tmpDir, ".github", "secrets", "evil.md")) +} + +func TestFetchAndSaveRemoteIncludes_NestedTraversalRejected(t *testing.T) { + mockFetch := func(_ context.Context, _ string, _ *WorkflowSpec, _ bool) ([]byte, string, error) { + return []byte("# include body\n"), "", nil + } + + tmpDir := t.TempDir() + targetDir := filepath.Join(tmpDir, ".github", "workflows") + require.NoError(t, os.MkdirAll(targetDir, 0o755)) + + spec := &WorkflowSpec{ + RepoSpec: RepoSpec{RepoSlug: "github/gh-aw", Version: "main"}, + } + // A path that doesn't start with "../" but still escapes via a sub-directory component. + err := fetchAndSaveRemoteIncludes(t.Context(), "@include subdir/../../secrets/evil.md\n", spec, targetDir, false, false, nil, mockFetch) + require.Error(t, err) + require.NoFileExists(t, filepath.Join(tmpDir, ".github", "secrets", "evil.md")) +} + +func TestFetchAndSaveRemoteIncludes_SharedIncludeStaysUnderSharedDir(t *testing.T) { + mockFetch := func(_ context.Context, _ string, _ *WorkflowSpec, _ bool) ([]byte, string, error) { + return []byte("# include body\n"), "", nil + } + + tmpDir := t.TempDir() + targetDir := filepath.Join(tmpDir, ".github", "workflows") + require.NoError(t, os.MkdirAll(targetDir, 0o755)) + + spec := &WorkflowSpec{ + RepoSpec: RepoSpec{RepoSlug: "github/gh-aw", Version: "main"}, + } + err := fetchAndSaveRemoteIncludes(t.Context(), "@include shared/helper.md\n", spec, targetDir, false, false, nil, mockFetch) + require.NoError(t, err) + assert.FileExists(t, filepath.Join(tmpDir, ".github", "shared", "helper.md")) +} + // TestFetchAndSaveRemoteFrontmatterImports_InvalidRepoSlug verifies that an invalid // RepoSlug (not in owner/repo format) causes the function to return early without error. func TestFetchAndSaveRemoteFrontmatterImports_InvalidRepoSlug(t *testing.T) { diff --git a/pkg/fileutil/fileutil.go b/pkg/fileutil/fileutil.go index 5b22afdec12..07861226e66 100644 --- a/pkg/fileutil/fileutil.go +++ b/pkg/fileutil/fileutil.go @@ -59,6 +59,11 @@ func ValidateAbsolutePath(path string) (string, error) { // fallback when a path does not yet exist) before comparison, so neither ".." // components nor symlinks pointing outside base can be used to escape. // +// For candidate paths that do not yet exist on disk, the longest existing ancestor is +// resolved through EvalSymlinks before the non-existing suffix is re-appended. This +// prevents an in-base symlinked directory from being used to write outside the base +// even when the final file does not exist yet. +// // Returns an error when: // - Either path cannot be resolved to an absolute form. // - The resolved candidate path starts outside the resolved base directory. @@ -73,12 +78,9 @@ func ValidatePathWithinBase(base, candidate string) error { return fmt.Errorf("failed to resolve base path %q: %w", base, err) } } - absCand, err := filepath.EvalSymlinks(candidate) + absCand, err := resolveWithAncestorSymlinks(candidate) if err != nil { - absCand, err = filepath.Abs(candidate) - if err != nil { - return fmt.Errorf("failed to resolve candidate path %q: %w", candidate, err) - } + return fmt.Errorf("failed to resolve candidate path %q: %w", candidate, err) } rel, err := filepath.Rel(absBase, absCand) if err != nil || !filepath.IsLocal(rel) { @@ -89,6 +91,48 @@ func ValidatePathWithinBase(base, candidate string) error { return nil } +// resolveWithAncestorSymlinks resolves a path to its absolute real form, following +// symlinks for every existing component. For paths whose final component does not +// yet exist on disk, it walks up to the longest existing ancestor, resolves that +// through filepath.EvalSymlinks (catching any symlinked directories along the way), +// and then re-appends the non-existing suffix. This prevents a symlinked directory +// inside base from being used to escape the boundary when the target file is new. +func resolveWithAncestorSymlinks(p string) (string, error) { + // Fast path: path exists — EvalSymlinks fully resolves it. + if resolved, err := filepath.EvalSymlinks(p); err == nil { + return resolved, nil + } + // Path does not fully exist yet. Get a clean absolute path. + absp, err := filepath.Abs(p) + if err != nil { + return "", err + } + // Walk upward until we find the longest existing prefix and resolve it. + suffix := "" + cur := absp + for { + if resolved, err := filepath.EvalSymlinks(cur); err == nil { + if suffix == "" { + return resolved, nil + } + return filepath.Join(resolved, suffix), nil + } + parent := filepath.Dir(cur) + if parent == cur { + // Reached the filesystem root without finding an existing component. + // Fall back to the lexical absolute path. + return absp, nil + } + component := filepath.Base(cur) + if suffix == "" { + suffix = component + } else { + suffix = filepath.Join(component, suffix) + } + cur = parent + } +} + // EnsureParentDir ensures the parent directory for path exists, creating it recursively when needed. func EnsureParentDir(path string, perm os.FileMode) error { if path == "" { diff --git a/pkg/fileutil/fileutil_test.go b/pkg/fileutil/fileutil_test.go index 560457cc9d7..1b050fbe145 100644 --- a/pkg/fileutil/fileutil_test.go +++ b/pkg/fileutil/fileutil_test.go @@ -510,6 +510,29 @@ func TestValidatePathWithinBase(t *testing.T) { require.Error(t, err, "ValidatePathWithinBase should reject symlink that points outside base") require.ErrorContains(t, err, "escapes base directory", "Error should describe the symlink escape") }) + + t.Run("symlink directory ancestor with new file", func(t *testing.T) { + // Create a real directory outside the base to serve as the symlink target. + outsideDir, err := os.MkdirTemp("", "validatepathwithinbase-outsidedir-*") + require.NoError(t, err, "failed to create outside directory") + t.Cleanup(func() { _ = os.RemoveAll(outsideDir) }) + + // Place a symlinked directory inside base that points to the outside directory. + linkDir := filepath.Join(base, "link-dir") + if err := os.Symlink(outsideDir, linkDir); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + t.Cleanup(func() { _ = os.Remove(linkDir) }) + + // The target file does not exist yet — this is the bypass scenario: + // EvalSymlinks(linkDir/new.md) fails, and the old fallback filepath.Abs + // would return base/link-dir/new.md which lexically looks safe, while + // MkdirAll + WriteFile would follow the symlink and write outside base. + candidate := filepath.Join(linkDir, "new.md") + err = ValidatePathWithinBase(base, candidate) + require.Error(t, err, "ValidatePathWithinBase should reject write via symlinked directory to outside base") + require.ErrorContains(t, err, "escapes base directory", "Error should describe the symlink directory escape") + }) } func TestExtractFileFromTar_UnsafePaths(t *testing.T) {