Harden go-gh remote fetch callsites with escaped contents paths/refs and bounded REST clients (#41036)

* Initial plan

* Fix go-gh content ref escaping and REST client timeout options

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

* Address review feedback on path and host normalization

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

* Simplify secret API host normalization fallback

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: Peli de Halleux <pelikhan@users.noreply.github.com>

Author: Copilot

pkg/cli/secret_set_command.go

@@ -7,13 +7,16 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"net/url"
 	"os"
 	"strings"
 
 	"github.com/cli/go-gh/v2/pkg/api"
 	"github.com/github/gh-aw/pkg/console"
+	"github.com/github/gh-aw/pkg/constants"
 	"github.com/github/gh-aw/pkg/logger"
 	"github.com/github/gh-aw/pkg/repoutil"
+	"github.com/github/gh-aw/pkg/stringutil"
 	"github.com/github/gh-aw/pkg/tty"
 	"github.com/spf13/cobra"
 	"golang.org/x/crypto/nacl/box"
@@ -88,11 +91,7 @@ The secret value can be provided in three ways:
 			}
 
 			// Create GitHub REST client using go-gh
-			opts := api.ClientOptions{}
-			if flagAPIBase != "" {
-				opts.Host = strings.TrimPrefix(strings.TrimPrefix(flagAPIBase, "https://"), "http://")
-			}
-			client, err := api.NewRESTClient(opts)
+			client, err := api.NewRESTClient(secretSetClientOptions(flagAPIBase))
 			if err != nil {
 				return fmt.Errorf("cannot create GitHub client: %w", err)
 			}
@@ -123,6 +122,44 @@ The secret value can be provided in three ways:
 	return cmd
 }
 
+func secretSetClientOptions(apiBase string) api.ClientOptions {
+	opts := api.ClientOptions{
+		Timeout: constants.DefaultHTTPClientTimeout,
+	}
+	if apiBase != "" {
+		opts.Host = normalizeSecretSetAPIHost(apiBase)
+	}
+	return opts
+}
+
+func normalizeSecretSetAPIHost(apiBase string) string {
+	raw := strings.TrimSpace(apiBase)
+	if raw == "" {
+		return ""
+	}
+
+	candidates := []string{raw}
+	if !strings.Contains(raw, "://") {
+		candidates = append(candidates, "https://"+raw)
+	}
+	for _, candidate := range candidates {
+		parsed, err := url.Parse(candidate)
+		if err != nil || parsed.Hostname() == "" {
+			continue
+		}
+		if parsed.Hostname() == "api.github.com" {
+			return "github.com"
+		}
+		return parsed.Hostname()
+	}
+
+	legacy := stringutil.ExtractDomainFromURL(raw)
+	if legacy == "api.github.com" {
+		return "github.com"
+	}
+	return legacy
+}
+
 func resolveSecretValueForSet(fromEnv, fromFlag string) (string, error) {
 	if fromEnv != "" {
 		v := os.Getenv(fromEnv)

pkg/cli/secret_set_command_test.go

@@ -8,6 +8,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/github/gh-aw/pkg/constants"
 	"golang.org/x/crypto/nacl/box"
 )
 
@@ -165,3 +166,45 @@ func TestEncryptDecryptRoundTrip(t *testing.T) {
 		t.Errorf("decrypted = %q, want %q", string(decrypted), plaintext)
 	}
 }
+
+func TestSecretSetClientOptions(t *testing.T) {
+	t.Run("defaults include timeout", func(t *testing.T) {
+		opts := secretSetClientOptions("")
+		if opts.Host != "" {
+			t.Fatalf("expected empty host, got %q", opts.Host)
+		}
+		if opts.Timeout != constants.DefaultHTTPClientTimeout {
+			t.Fatalf("expected timeout %s, got %s", constants.DefaultHTTPClientTimeout, opts.Timeout)
+		}
+	})
+
+	t.Run("normalizes host when api-url is provided", func(t *testing.T) {
+		opts := secretSetClientOptions("https://ghe.example.com")
+		if opts.Host != "ghe.example.com" {
+			t.Fatalf("expected host ghe.example.com, got %q", opts.Host)
+		}
+		if opts.Timeout != constants.DefaultHTTPClientTimeout {
+			t.Fatalf("expected timeout %s, got %s", constants.DefaultHTTPClientTimeout, opts.Timeout)
+		}
+	})
+
+	t.Run("strips API path from api-url", func(t *testing.T) {
+		opts := secretSetClientOptions("https://ghe.example.com/api/v3")
+		if opts.Host != "ghe.example.com" {
+			t.Fatalf("expected host ghe.example.com, got %q", opts.Host)
+		}
+		if opts.Timeout != constants.DefaultHTTPClientTimeout {
+			t.Fatalf("expected timeout %s, got %s", constants.DefaultHTTPClientTimeout, opts.Timeout)
+		}
+	})
+
+	t.Run("maps api.github.com to github.com", func(t *testing.T) {
+		opts := secretSetClientOptions("https://api.github.com")
+		if opts.Host != "github.com" {
+			t.Fatalf("expected host github.com, got %q", opts.Host)
+		}
+		if opts.Timeout != constants.DefaultHTTPClientTimeout {
+			t.Fatalf("expected timeout %s, got %s", constants.DefaultHTTPClientTimeout, opts.Timeout)
+		}
+	})
+}

pkg/cli/update_check.go

@@ -239,7 +239,7 @@ func getLatestRelease(includePrereleases bool) (string, error) {
 	// Always target github.com explicitly: gh-aw is only published to github.com,
 	// and users in mixed-host environments (e.g. a GHE active auth host) must
 	// still reach the canonical registry to get the correct release metadata.
-	client, err := api.NewRESTClient(api.ClientOptions{Host: "github.com"})
+	client, err := api.NewRESTClient(gitHubDotComRESTClientOptions())
 	if err != nil {
 		return "", fmt.Errorf("failed to create GitHub client: %w", err)
 	}
@@ -274,6 +274,13 @@ func getLatestRelease(includePrereleases bool) (string, error) {
 	return release.TagName, nil
 }
 
+func gitHubDotComRESTClientOptions() api.ClientOptions {
+	return api.ClientOptions{
+		Host:    "github.com",
+		Timeout: constants.DefaultHTTPClientTimeout,
+	}
+}
+
 // findLatestPublishedReleaseTag returns the first non-draft release tag from the
 // releases API response, skipping entries without tag names.
 func findLatestPublishedReleaseTag(releases []Release) string {

pkg/cli/update_check_test.go

@@ -9,6 +9,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/github/gh-aw/pkg/constants"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -440,3 +441,9 @@ func TestIsCurrentVersionAtLeastLatest(t *testing.T) {
 		})
 	}
 }
+
+func TestGitHubDotComRESTClientOptions(t *testing.T) {
+	opts := gitHubDotComRESTClientOptions()
+	assert.Equal(t, "github.com", opts.Host)
+	assert.Equal(t, constants.DefaultHTTPClientTimeout, opts.Timeout)
+}

pkg/parser/remote_fetch.go

@@ -744,7 +744,7 @@ func downloadFileViaGitClone(owner, repo, path, ref, host string) ([]byte, error
 // Returns the symlink target and true if it is a symlink, or empty string and false otherwise.
 // A nil error with false means the path is not a symlink (e.g., it's a directory or file).
 func checkRemoteSymlink(client *api.RESTClient, owner, repo, dirPath, ref string) (string, bool, error) {
-	endpoint := fmt.Sprintf("repos/%s/%s/contents/%s?ref=%s", owner, repo, dirPath, ref)
+	endpoint := buildContentsAPIPath(owner, repo, dirPath, ref)
 	remoteLog.Printf("Checking if path component is symlink: %s/%s/%s@%s", owner, repo, dirPath, ref)
 
 	// The Contents API returns a JSON object for files/symlinks but a JSON array for directories.
@@ -951,14 +951,29 @@ func downloadFileFromGitHubWithDepth(owner, repo, path, ref string, symlinkDepth
 }
 
 func createRESTClientForHost(host string) (*api.RESTClient, error) {
+	opts := api.ClientOptions{Timeout: constants.DefaultHTTPClientTimeout}
 	if host != "" {
-		return api.NewRESTClient(api.ClientOptions{Host: host})
+		opts.Host = host
 	}
-	return api.DefaultRESTClient()
+	return api.NewRESTClient(opts)
+}
+
+func buildContentsAPIPath(owner, repo, path, ref string) string {
+	pathSegments := strings.Split(path, "/")
+	for i := range pathSegments {
+		pathSegments[i] = url.PathEscape(pathSegments[i])
+	}
+	return fmt.Sprintf(
+		"repos/%s/%s/contents/%s?ref=%s",
+		owner,
+		repo,
+		strings.Join(pathSegments, "/"),
+		url.QueryEscape(ref),
+	)
 }
 
 func fetchRemoteFileContent(client *api.RESTClient, owner, repo, path, ref string, fileContent any) error {
-	return client.Get(fmt.Sprintf("repos/%s/%s/contents/%s?ref=%s", owner, repo, path, ref), fileContent)
+	return client.Get(buildContentsAPIPath(owner, repo, path, ref), fileContent)
 }
 
 // downloadFileViaPublicAPI downloads a file from a public GitHub repository
@@ -1020,16 +1035,7 @@ func ListWorkflowFilesForHost(owner, repo, ref, workflowPath, host string) ([]st
 func listWorkflowFilesForHost(owner, repo, ref, workflowPath, host string) ([]string, error) {
 	remoteLog.Printf("Listing workflow files for %s/%s@%s (path: %s)", owner, repo, ref, workflowPath)
 
-	// Create REST client
-	var (
-		client *api.RESTClient
-		err    error
-	)
-	if host != "" {
-		client, err = api.NewRESTClient(api.ClientOptions{Host: host})
-	} else {
-		client, err = api.DefaultRESTClient()
-	}
+	client, err := createRESTClientForHost(host)
 	if err != nil {
 		remoteLog.Printf("Failed to create REST client, attempting git fallback: %v", err)
 		return listWorkflowFilesViaGitForHost(owner, repo, ref, workflowPath, host)
@@ -1043,7 +1049,7 @@ func listWorkflowFilesForHost(owner, repo, ref, workflowPath, host string) ([]st
 	}
 
 	// Fetch directory contents from GitHub API
-	endpoint := fmt.Sprintf("repos/%s/%s/contents/%s?ref=%s", owner, repo, workflowPath, ref)
+	endpoint := buildContentsAPIPath(owner, repo, workflowPath, ref)
 	err = client.Get(endpoint, &contents)
 	if err != nil {
 		errStr := err.Error()
@@ -1088,15 +1094,7 @@ func ListDirAllFilesForHost(owner, repo, ref, dirPath, host string) ([]string, e
 func listDirAllFilesForHost(owner, repo, ref, dirPath, host string) ([]string, error) {
 	remoteLog.Printf("Listing all files in dir for %s/%s@%s (path: %s)", owner, repo, ref, dirPath)
 
-	var (
-		client *api.RESTClient
-		err    error
-	)
-	if host != "" {
-		client, err = api.NewRESTClient(api.ClientOptions{Host: host})
-	} else {
-		client, err = api.DefaultRESTClient()
-	}
+	client, err := createRESTClientForHost(host)
 	if err != nil {
 		remoteLog.Printf("Failed to create REST client, attempting git fallback: %v", err)
 		return listDirAllFilesViaGitForHost(owner, repo, ref, dirPath, host)
@@ -1108,7 +1106,7 @@ func listDirAllFilesForHost(owner, repo, ref, dirPath, host string) ([]string, e
 		Type string `json:"type"`
 	}
 
-	endpoint := fmt.Sprintf("repos/%s/%s/contents/%s?ref=%s", owner, repo, dirPath, ref)
+	endpoint := buildContentsAPIPath(owner, repo, dirPath, ref)
 	err = client.Get(endpoint, &contents)
 	if err != nil {
 		errStr := err.Error()
@@ -1209,15 +1207,7 @@ func ListDirAllFilesRecursivelyForHost(owner, repo, ref, dirPath, host string) (
 func listDirAllFilesRecursivelyForHost(owner, repo, ref, dirPath, host string) ([]string, error) {
 	remoteLog.Printf("Listing all files recursively in dir for %s/%s@%s (path: %s)", owner, repo, ref, dirPath)
 
-	var (
-		client *api.RESTClient
-		err    error
-	)
-	if host != "" {
-		client, err = api.NewRESTClient(api.ClientOptions{Host: host})
-	} else {
-		client, err = api.DefaultRESTClient()
-	}
+	client, err := createRESTClientForHost(host)
 	if err != nil {
 		remoteLog.Printf("Failed to create REST client, attempting git fallback: %v", err)
 		return listDirAllFilesRecursivelyViaGitForHost(owner, repo, ref, dirPath, host)
@@ -1262,7 +1252,7 @@ func listContentsRecursivelyWithDepth(client *api.RESTClient, owner, repo, ref,
 		Type string `json:"type"`
 	}
 
-	endpoint := fmt.Sprintf("repos/%s/%s/contents/%s?ref=%s", owner, repo, dirPath, ref)
+	endpoint := buildContentsAPIPath(owner, repo, dirPath, ref)
 	if err := client.Get(endpoint, &contents); err != nil {
 		return nil, fmt.Errorf("failed to list dir files from %s/%s (path: %s): %w", owner, repo, dirPath, err)
 	}
@@ -1363,15 +1353,7 @@ func ListDirSubdirsForHost(owner, repo, ref, dirPath, host string) ([]string, er
 func listDirSubdirsForHost(owner, repo, ref, dirPath, host string) ([]string, error) {
 	remoteLog.Printf("Listing subdirs in %s/%s@%s (path: %s)", owner, repo, ref, dirPath)
 
-	var (
-		client *api.RESTClient
-		err    error
-	)
-	if host != "" {
-		client, err = api.NewRESTClient(api.ClientOptions{Host: host})
-	} else {
-		client, err = api.DefaultRESTClient()
-	}
+	client, err := createRESTClientForHost(host)
 	if err != nil {
 		remoteLog.Printf("Failed to create REST client, attempting git fallback: %v", err)
 		return listDirSubdirsViaGitForHost(owner, repo, ref, dirPath, host)
@@ -1383,7 +1365,7 @@ func listDirSubdirsForHost(owner, repo, ref, dirPath, host string) ([]string, er
 		Type string `json:"type"`
 	}
 
-	endpoint := fmt.Sprintf("repos/%s/%s/contents/%s?ref=%s", owner, repo, dirPath, ref)
+	endpoint := buildContentsAPIPath(owner, repo, dirPath, ref)
 	err = client.Get(endpoint, &contents)
 	if err != nil {
 		errStr := err.Error()

pkg/parser/remote_fetch_test.go

@@ -25,6 +25,32 @@ func TestBuildCommitLookupAPIPath(t *testing.T) {
 	})
 }
 
+func TestBuildContentsAPIPath(t *testing.T) {
+	t.Run("escapes refs with reserved query chars", func(t *testing.T) {
+		got := buildContentsAPIPath("owner", "repo", ".github/workflows/demo.md", "release+candidate#1")
+		want := "repos/owner/repo/contents/.github/workflows/demo.md?ref=release%2Bcandidate%231"
+		if got != want {
+			t.Fatalf("buildContentsAPIPath() = %q, want %q", got, want)
+		}
+	})
+
+	t.Run("keeps plain refs readable", func(t *testing.T) {
+		got := buildContentsAPIPath("owner", "repo", ".github/workflows/demo.md", "main")
+		want := "repos/owner/repo/contents/.github/workflows/demo.md?ref=main"
+		if got != want {
+			t.Fatalf("buildContentsAPIPath() = %q, want %q", got, want)
+		}
+	})
+
+	t.Run("escapes path segments with reserved chars", func(t *testing.T) {
+		got := buildContentsAPIPath("owner", "repo", "skills/path with spaces/file#100%.md", "main")
+		want := "repos/owner/repo/contents/skills/path%20with%20spaces/file%23100%25.md?ref=main"
+		if got != want {
+			t.Fatalf("buildContentsAPIPath() = %q, want %q", got, want)
+		}
+	})
+}
+
 func TestGitFallbackRequiresNonEmptyRef(t *testing.T) {
 	t.Run("all files fallback validates ref", func(t *testing.T) {
 		_, err := listDirAllFilesViaGitForHost("owner", "repo", "", "skills/demo", "")