Skip to content

fix: harden unauthenticated GitHub API fallbacks with context and 30s timeout #42521

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 3 commits into from
Jun 30, 2026
+30 −20
Merged

fix: harden unauthenticated GitHub API fallbacks with context and 30s timeout #42521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cb6d447
Initial plan
Copilot Jun 30, 2026
ed06cff
fix: harden unauthenticated GitHub API fallbacks with context and tim…
Copilot Jun 30, 2026
5483bc3
refactor: use package-level HTTP clients for unauthenticated API fall…
Copilot Jun 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion pkg/cli/update_workflows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ import (
var defaultBranchCache sync.Map
var branchCommitCache sync.Map

// updatePublicAPIClient is a shared HTTP client used for unauthenticated GitHub
// API fallback calls in the update workflow path. It carries a timeout to
// prevent indefinite hangs on slow or unresponsive hosts.
var updatePublicAPIClient = &http.Client{Timeout: constants.DefaultHTTPClientTimeout}

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate HTTP client declarations across two packages create a silent maintenance trap.

💡 Details

updatePublicAPIClient (here) and publicAPIClient (pkg/parser/remote_fetch.go:36) are structurally identical:

&http.Client{Timeout: constants.DefaultHTTPClientTimeout}

When the unauthenticated API path eventually needs tuning — custom Transport for proxy support, different timeout per environment, retry middleware — the change will be applied to one and silently missed in the other. A shared constructor (or a single exported client) in a common internal/httpclient or constants package would close this gap:

// e.g., in pkg/constants or pkg/internal/httpclient
func NewPublicAPIClient() *http.Client {
    return &http.Client{Timeout: DefaultHTTPClientTimeout}
}

Both call sites then call httpclient.NewPublicAPIClient(), giving a single place to evolve the configuration.

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/codebase-design] updatePublicAPIClient is a second package-level HTTP client with the same configuration as publicAPIClient in pkg/parser/remote_fetch.go. If the timeout value ever needs to change (e.g. for a shorter default), both declarations must be updated independently.

💡 Consider consolidating

Both clients are: &http.Client{Timeout: constants.DefaultHTTPClientTimeout}. The duplication is currently low-risk because constants.DefaultHTTPClientTimeout is the single source of truth for the value. However, the two clients could diverge in future (e.g. one gets headers, retries, or a transport).

A shared constructor or a single pkg/httputil package-level var would make the intent clearer and reduce the divergence risk. For now, a comment linking the two declarations would also help:

// See also publicAPIClient in pkg/parser/remote_fetch.go — same config.
var updatePublicAPIClient = &http.Client{Timeout: constants.DefaultHTTPClientTimeout}

@copilot please address this.


// repoBranchKey is a composite cache key for branch commit SHA lookups.
type repoBranchKey struct {
repo string
Expand Down Expand Up @@ -364,7 +369,7 @@ func fetchPublicGitHubAPI(ctx context.Context, endpoint string) ([]byte, error)
}
req.Header.Set("Accept", "application/vnd.github+json")

resp, err := http.DefaultClient.Do(req)
resp, err := updatePublicAPIClient.Do(req)
if err != nil {
return nil, err
}
Expand Down
43 changes: 24 additions & 19 deletions pkg/parser/remote_fetch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ import (

var remoteLog = logger.New("parser:remote_fetch")

// publicAPIClient is a shared HTTP client used for unauthenticated GitHub API
// fallback calls. It carries a timeout to prevent indefinite hangs on slow or
// unresponsive hosts.
var publicAPIClient = &http.Client{Timeout: constants.DefaultHTTPClientTimeout}

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mutable package-level var for the HTTP client enables test-state pollution.

💡 Details

publicAPIClient is a plain var, which means any in-package test can overwrite it:

// In a test file
func TestSomething(t *testing.T) {
    publicAPIClient = &http.Client{Transport: mockTransport}
    // if this test panics before cleanup, next test picks up mockTransport
}

If a test fails before restoring the original value (or uses t.Parallel()), subsequent tests silently use the wrong transport. The standard Go mitigation is to save and restore via t.Cleanup, but that requires discipline across every future test that needs to inject a mock. A safer pattern is a function-level or injected client:

// Unexported sentinel; set once at init, replaced in tests via a setter
var publicAPIClient = newPublicAPIClient()

func newPublicAPIClient() *http.Client {
    return &http.Client{Timeout: constants.DefaultHTTPClientTimeout}
}

Tests swap via the constructor rather than mutating the global, which makes the scope of the change explicit and t.Cleanup-friendly.

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs + /tdd] No regression test guards this fix — if a future change silently reverts to http.DefaultClient, the hang will come back undetected.

💡 Suggested test skeleton

A test using httptest.NewServer with a slow handler and a short-timeout client would confirm the fix:

func TestPublicAPIClientTimeout(t *testing.T) {
    slow := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        time.Sleep(2 * time.Second)
    }))
    defer slow.Close()

    orig := publicAPIClient
    publicAPIClient = &http.Client{Timeout: 100 * time.Millisecond}
    t.Cleanup(func() { publicAPIClient = orig })

    _, err := fetchPublicGitHubContentsAPI(context.Background(), "owner", "repo", "path", "ref")
    if err == nil {
        t.Fatal("expected timeout error, got nil")
    }
}

This acts as a living specification: any regression that swaps back to http.DefaultClient fails immediately.

@copilot please address this.


// gitListCloneCache is a process-lifetime cache of shallow clones used by
// git-based directory listing fallbacks to avoid repeated clone operations for
// the same repository/ref tuple. Entries are not explicitly cleaned up because
Expand Down Expand Up @@ -527,7 +532,7 @@ func resolveRefToSHA(owner, repo, ref, host string) (string, error) {
if gitErr != nil {
if host == "" || host == "github.com" {
remoteLog.Printf("Git fallback also failed, attempting unauthenticated API for %s/%s@%s", owner, repo, ref)
return resolveRefToSHAViaPublicAPI(owner, repo, ref)
return resolveRefToSHAViaPublicAPI(context.Background(), owner, repo, ref)

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The five fallback call sites are all forced to use context.Background() because the intermediate callers (resolveRefToSHA, downloadFileFromGitHubWithDepth, listWorkflowFilesForHost, listDirAllFilesForHost, listDirSubdirsForHost) have no ctx parameter. The 30 s client-level timeout is the real hang protection here — the ctx passed to http.NewRequestWithContext carries no deadline or cancellation signal from the outer call stack. As a follow-up, propagating ctx through those intermediate functions would allow callers to cancel in-flight requests via their own context deadlines. @copilot please address this.

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete context threading: context.Background() silently drops all caller cancellation signals at every public-API fallback site.

💡 Details and suggested fix

Five call sites in this file (resolveRefToSHA, downloadFileFromGitHubWithDepth, listWorkflowFilesForHost, listDirAllFilesForHost, listDirSubdirsForHost) pass context.Background() because the outer functions have no ctx parameter. This means:

  • Ctrl-C / OS signal → context cancelled at the CLI layer → the HTTP request in publicAPIClient.Do(req) continues for up to 30 s regardless.
  • Any caller with a short deadline (e.g., a test with t.Context()) has that deadline silently discarded.
  • The Timeout field on publicAPIClient is the only real protection; the new ctx parameters are scaffolding with no current effect on cancellation.

Complete the threading by adding ctx to the outer functions:

// was: func resolveRefToSHA(owner, repo, ref, host string) (string, error)
func resolveRefToSHA(ctx context.Context, owner, repo, ref, host string) (string, error) {
    // ...
    return resolveRefToSHAViaPublicAPI(ctx, owner, repo, ref) // real ctx, not context.Background()
}

Same pattern for the other four outer wrappers. Without this, the PR description overstates the change — context is not actually threaded, only a 30 s client-level timeout is added.

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] All five new context.Background() call sites forfeit context cancellation — if a caller later gains a deadline (e.g. a per-request context from a CLI command), the HTTP call will not respect it; the 30 s timeout is the only guard.

💡 Why this matters and what to consider

The parent functions (resolveRefToSHA, downloadFileFromGitHubWithDepth, listWorkflowFilesForHost, etc.) currently have no context.Context parameter, so context.Background() is the only viable option right now. This PR is correct as-is, but leaves a TODO in the door:

  • Short term: add a code comment at each call site, e.g. // TODO(context): pass caller ctx once parent accepts one
  • Longer term: thread ctx through the parent functions so cancellation signals from the CLI propagate end-to-end

Without this, a user cancelling a long-running gh aw compile will cancel everything except the last-resort public API fallbacks, which must wait for the full 30 s.

@copilot please address this.

}
return "", fmt.Errorf("failed to resolve ref via GitHub API (auth error) and git ls-remote: API error: %w, Git error: %w", err, gitErr)
}
Expand Down Expand Up @@ -559,17 +564,17 @@ func buildCommitLookupAPIPath(owner, repo, ref string) string {
// resolveRefToSHAViaPublicAPI resolves a git ref to its commit SHA using an
// unauthenticated call to the public GitHub API. Used as a last-resort fallback
// when both authenticated API and git ls-remote fail.
func resolveRefToSHAViaPublicAPI(owner, repo, ref string) (string, error) {
func resolveRefToSHAViaPublicAPI(ctx context.Context, owner, repo, ref string) (string, error) {
remoteLog.Printf("Attempting unauthenticated public API ref resolution for %s/%s@%s", owner, repo, ref)
apiURL := fmt.Sprintf("https://api.github.com/repos/%s/%s/commits/%s",
owner, repo, url.PathEscape(ref))
req, err := http.NewRequest(http.MethodGet, apiURL, nil)
req, err := http.NewRequestWithContext(ctx, http.MethodGet, apiURL, nil)
if err != nil {
return "", err
}
req.Header.Set("Accept", "application/vnd.github+json")

resp, err := http.DefaultClient.Do(req)
resp, err := publicAPIClient.Do(req)
if err != nil {
return "", err
}
Expand Down Expand Up @@ -922,7 +927,7 @@ func downloadFileFromGitHubWithDepth(owner, repo, path, ref string, symlinkDepth
if gitErr != nil {
if host == "" || host == "github.com" {
remoteLog.Printf("Git fallback also failed, attempting unauthenticated API for %s/%s/%s@%s", owner, repo, path, ref)
return downloadFileViaPublicAPI(owner, repo, path, ref)
return downloadFileViaPublicAPI(context.Background(), owner, repo, path, ref)
}
return nil, fmt.Errorf("failed to fetch file content via GitHub API (auth error) and git fallback: API error: %w, Git error: %w", err, gitErr)
}
Expand Down Expand Up @@ -979,9 +984,9 @@ func fetchRemoteFileContent(client *api.RESTClient, owner, repo, path, ref strin
// downloadFileViaPublicAPI downloads a file from a public GitHub repository
// using an unauthenticated API call. Used as a last-resort fallback when both
// authenticated API and git clone fail (e.g. enterprise SAML tokens).
func downloadFileViaPublicAPI(owner, repo, path, ref string) ([]byte, error) {
func downloadFileViaPublicAPI(ctx context.Context, owner, repo, path, ref string) ([]byte, error) {
remoteLog.Printf("Attempting unauthenticated public API download for %s/%s/%s@%s", owner, repo, path, ref)
body, err := fetchPublicGitHubContentsAPI(owner, repo, path, ref)
body, err := fetchPublicGitHubContentsAPI(ctx, owner, repo, path, ref)
if err != nil {
return nil, fmt.Errorf("unauthenticated public API also failed for %s/%s/%s@%s: %w", owner, repo, path, ref, err)
}
Expand Down Expand Up @@ -1062,7 +1067,7 @@ func listWorkflowFilesForHost(owner, repo, ref, workflowPath, host string) ([]st
if gitErr != nil {
if host == "" || host == "github.com" {
remoteLog.Printf("Git fallback also failed, attempting unauthenticated API for %s/%s@%s", owner, repo, ref)
return listWorkflowFilesViaPublicAPI(owner, repo, ref, workflowPath)
return listWorkflowFilesViaPublicAPI(context.Background(), owner, repo, ref, workflowPath)
}
return nil, fmt.Errorf("failed to list workflow files via GitHub API (auth error) and git fallback: API error: %w, Git error: %w", err, gitErr)
}
Expand Down Expand Up @@ -1116,7 +1121,7 @@ func listDirAllFilesForHost(owner, repo, ref, dirPath, host string) ([]string, e
if gitErr != nil {
if host == "" || host == "github.com" {
remoteLog.Printf("Git fallback also failed, attempting unauthenticated API for %s/%s@%s", owner, repo, ref)
return listDirAllFilesViaPublicAPI(owner, repo, ref, dirPath)
return listDirAllFilesViaPublicAPI(context.Background(), owner, repo, ref, dirPath)
}
return nil, fmt.Errorf("failed to list dir files via API (auth error) and git fallback: API error: %w, Git error: %w", err, gitErr)
}
Expand Down Expand Up @@ -1172,9 +1177,9 @@ func listDirAllFilesViaGitForHost(owner, repo, ref, dirPath, host string) ([]str
// listDirAllFilesViaPublicAPI lists files in a directory using an unauthenticated
// call to the public GitHub API. Used as a last-resort fallback when both
// authenticated API and git clone fail.
func listDirAllFilesViaPublicAPI(owner, repo, ref, dirPath string) ([]string, error) {
func listDirAllFilesViaPublicAPI(ctx context.Context, owner, repo, ref, dirPath string) ([]string, error) {
remoteLog.Printf("Attempting unauthenticated public API for listing dir files: %s/%s@%s (path: %s)", owner, repo, ref, dirPath)
body, err := fetchPublicGitHubContentsAPI(owner, repo, dirPath, ref)
body, err := fetchPublicGitHubContentsAPI(ctx, owner, repo, dirPath, ref)
if err != nil {
return nil, fmt.Errorf("unauthenticated public API also failed for %s/%s@%s (path: %s): %w", owner, repo, ref, dirPath, err)
}
Expand Down Expand Up @@ -1311,7 +1316,7 @@ func listDirAllFilesRecursivelyViaGitForHost(owner, repo, ref, dirPath, host str
// cannot access cross-organization public repositories and git clone also
// fails. Unauthenticated requests are subject to a lower rate limit
// (60 req/hour) but are sufficient for the handful of calls during update.
func fetchPublicGitHubContentsAPI(owner, repo, path, ref string) ([]byte, error) {
func fetchPublicGitHubContentsAPI(ctx context.Context, owner, repo, path, ref string) ([]byte, error) {
// Encode each path segment independently so that '/' separators are
// preserved — url.PathEscape would turn them into '%2F', breaking nested
// paths like '.github/workflows/shared/foo.md'.
Expand All @@ -1322,13 +1327,13 @@ func fetchPublicGitHubContentsAPI(owner, repo, path, ref string) ([]byte, error)
}
endpoint := fmt.Sprintf("https://api.github.com/repos/%s/%s/contents/%s?ref=%s",
owner, repo, strings.Join(encodedSegments, "/"), url.QueryEscape(ref))
req, err := http.NewRequest(http.MethodGet, endpoint, nil)
req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil)
if err != nil {
return nil, err
}
req.Header.Set("Accept", "application/vnd.github+json")

resp, err := http.DefaultClient.Do(req)
resp, err := publicAPIClient.Do(req)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1375,7 +1380,7 @@ func listDirSubdirsForHost(owner, repo, ref, dirPath, host string) ([]string, er
if gitErr != nil {
if host == "" || host == "github.com" {
remoteLog.Printf("Git fallback also failed, attempting unauthenticated API for %s/%s@%s", owner, repo, ref)
return listDirSubdirsViaPublicAPI(owner, repo, ref, dirPath)
return listDirSubdirsViaPublicAPI(context.Background(), owner, repo, ref, dirPath)
}
return nil, fmt.Errorf("failed to list subdirs via API (auth error) and git fallback: API error: %w, Git error: %w", err, gitErr)
}
Expand Down Expand Up @@ -1431,9 +1436,9 @@ func listDirSubdirsViaGitForHost(owner, repo, ref, dirPath, host string) ([]stri
// listDirSubdirsViaPublicAPI lists subdirectories using an unauthenticated call
// to the public GitHub API. Used as a last-resort fallback when both
// authenticated API and git clone fail (e.g. enterprise SAML tokens).
func listDirSubdirsViaPublicAPI(owner, repo, ref, dirPath string) ([]string, error) {
func listDirSubdirsViaPublicAPI(ctx context.Context, owner, repo, ref, dirPath string) ([]string, error) {
remoteLog.Printf("Attempting unauthenticated public API for listing subdirs: %s/%s@%s (path: %s)", owner, repo, ref, dirPath)
body, err := fetchPublicGitHubContentsAPI(owner, repo, dirPath, ref)
body, err := fetchPublicGitHubContentsAPI(ctx, owner, repo, dirPath, ref)
if err != nil {
return nil, fmt.Errorf("unauthenticated public API also failed for %s/%s@%s (path: %s): %w", owner, repo, ref, dirPath, err)
}
Expand Down Expand Up @@ -1515,9 +1520,9 @@ func listWorkflowFilesViaGitForHost(owner, repo, ref, workflowPath, host string)
// listWorkflowFilesViaPublicAPI lists workflow .md files using an unauthenticated
// call to the public GitHub API. Used as a last-resort fallback when both
// authenticated API and git clone fail.
func listWorkflowFilesViaPublicAPI(owner, repo, ref, workflowPath string) ([]string, error) {
func listWorkflowFilesViaPublicAPI(ctx context.Context, owner, repo, ref, workflowPath string) ([]string, error) {
remoteLog.Printf("Attempting unauthenticated public API for listing workflow files: %s/%s@%s (path: %s)", owner, repo, ref, workflowPath)
body, err := fetchPublicGitHubContentsAPI(owner, repo, workflowPath, ref)
body, err := fetchPublicGitHubContentsAPI(ctx, owner, repo, workflowPath, ref)
if err != nil {
return nil, fmt.Errorf("unauthenticated public API also failed for %s/%s@%s (path: %s): %w", owner, repo, ref, workflowPath, err)
}
Expand Down
Loading