From 03740ac4cf2b6d1e4946b354d91984f68f4f2e64 Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 4 Sep 2026 14:15:28 +0200 Subject: [PATCH 1/2] Use API SSH URLs for repository clones Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 635c0b51-d384-47de-bca4-121793a65152 --- api/queries_repo.go | 1 + api/queries_repo_test.go | 6 ++ pkg/cmd/repo/clone/clone.go | 20 +++++- pkg/cmd/repo/clone/clone_test.go | 102 ++++++++++++++++++++++++++++--- 4 files changed, 119 insertions(+), 10 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 41e6780b97d..659c06f6018 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -364,6 +364,7 @@ func GitHubRepo(client *Client, repo ghrepo.Interface) (*Repository, error) { databaseId name owner { login } + sshUrl hasIssuesEnabled description hasWikiEnabled diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index 59b68ce16bd..6b99c686110 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -42,6 +42,7 @@ func TestGitHubRepo_success(t *testing.T) { "databaseId": 1234, "name": "REPO", "owner": {"login": "OWNER"}, + "sshUrl": "org-1234@github.com:OWNER/REPO.git", "hasIssuesEnabled": true, "description": "a cool repo", "hasWikiEnabled": true, @@ -61,6 +62,7 @@ func TestGitHubRepo_success(t *testing.T) { DatabaseID: 1234, Name: "REPO", Owner: RepositoryOwner{Login: "OWNER"}, + SSHURL: "org-1234@github.com:OWNER/REPO.git", HasIssuesEnabled: true, Description: "a cool repo", HasWikiEnabled: true, @@ -85,6 +87,7 @@ func TestGitHubRepo_withParent(t *testing.T) { "id": "REPOID", "name": "REPO", "owner": {"login": "OWNER"}, + "sshUrl": "git@github.com:OWNER/REPO.git", "hasIssuesEnabled": true, "description": "", "hasWikiEnabled": false, @@ -94,6 +97,7 @@ func TestGitHubRepo_withParent(t *testing.T) { "id": "PARENTID", "name": "PARENT-REPO", "owner": {"login": "PARENT-OWNER"}, + "sshUrl": "org-5678@github.com:PARENT-OWNER/PARENT-REPO.git", "hasIssuesEnabled": true, "description": "parent repo", "hasWikiEnabled": true, @@ -112,6 +116,7 @@ func TestGitHubRepo_withParent(t *testing.T) { ID: "PARENTID", Name: "PARENT-REPO", Owner: RepositoryOwner{Login: "PARENT-OWNER"}, + SSHURL: "org-5678@github.com:PARENT-OWNER/PARENT-REPO.git", HasIssuesEnabled: true, Description: "parent repo", HasWikiEnabled: true, @@ -123,6 +128,7 @@ func TestGitHubRepo_withParent(t *testing.T) { ID: "REPOID", Name: "REPO", Owner: RepositoryOwner{Login: "OWNER"}, + SSHURL: "git@github.com:OWNER/REPO.git", HasIssuesEnabled: true, ViewerPermission: "READ", DefaultBranchRef: BranchRef{Name: "main"}, diff --git a/pkg/cmd/repo/clone/clone.go b/pkg/cmd/repo/clone/clone.go index b29b25038b6..8e00791b0a1 100644 --- a/pkg/cmd/repo/clone/clone.go +++ b/pkg/cmd/repo/clone/clone.go @@ -175,7 +175,10 @@ func cloneRun(opts *CloneOptions) error { if err != nil { return err } - canonicalCloneURL := ghrepo.FormatRemoteURL(canonicalRepo, protocol) + canonicalCloneURL, err := remoteURL(canonicalRepo, protocol) + if err != nil { + return err + } // If repo HasWikiEnabled and wantsWiki is true then create a new clone URL if wantsWiki { @@ -203,7 +206,10 @@ func cloneRun(opts *CloneOptions) error { } } else { protocol := cfg.GitProtocol(canonicalRepo.Parent.RepoHost()).Value - upstreamURL := ghrepo.FormatRemoteURL(canonicalRepo.Parent, protocol) + upstreamURL, err := remoteURL(canonicalRepo.Parent, protocol) + if err != nil { + return err + } upstreamName := opts.UpstreamName if opts.UpstreamName == "@owner" { @@ -236,6 +242,16 @@ func cloneRun(opts *CloneOptions) error { return nil } +func remoteURL(repo *api.Repository, protocol string) (string, error) { + if protocol != "ssh" { + return ghrepo.FormatRemoteURL(repo, protocol), nil + } + if repo.SSHURL == "" { + return "", fmt.Errorf("invalid API response: repository %s is missing sshUrl", ghrepo.FullName(repo)) + } + return repo.SSHURL, nil +} + // simplifyURL strips given URL of extra parts like extra path segments (i.e., // anything beyond `/owner/repo`), query strings, or fragments. This function // never returns an error. diff --git a/pkg/cmd/repo/clone/clone_test.go b/pkg/cmd/repo/clone/clone_test.go index ea074242b7c..7e9a39da73a 100644 --- a/pkg/cmd/repo/clone/clone_test.go +++ b/pkg/cmd/repo/clone/clone_test.go @@ -112,6 +112,10 @@ func TestNewCmdClone(t *testing.T) { } func runCloneCommand(httpClient *http.Client, cli string) (*test.CmdOut, error) { + return runCloneCommandWithConfig(httpClient, config.NewMockConfig(), cli) +} + +func runCloneCommandWithConfig(httpClient *http.Client, cfg gh.Config, cli string) (*test.CmdOut, error) { ios, stdin, stdout, stderr := iostreams.Test() fac := &cmdutil.Factory{ IOStreams: ios, @@ -119,7 +123,7 @@ func runCloneCommand(httpClient *http.Client, cli string) (*test.CmdOut, error) return httpClient, nil }, Config: func() (gh.Config, error) { - return config.NewMockConfig(), nil + return cfg, nil }, GitClient: &git.Client{ GhPath: "some/path/gh", @@ -151,9 +155,11 @@ func runCloneCommand(httpClient *http.Client, cli string) (*test.CmdOut, error) func Test_RepoClone(t *testing.T) { tests := []struct { - name string - args string - want string + name string + args string + sshURL string + gitProtocol string + want string }{ { name: "shorthand", @@ -186,9 +192,23 @@ func Test_RepoClone(t *testing.T) { want: "git clone https://github.com/OWNER/REPO.git", }, { - name: "SSH URL", - args: "git@github.com:OWNER/REPO.git", - want: "git clone git@github.com:OWNER/REPO.git", + name: "SSH URL", + args: "git@github.com:OWNER/REPO.git", + sshURL: "git@github.com:OWNER/REPO.git", + want: "git clone git@github.com:OWNER/REPO.git", + }, + { + name: "GitHub.com SSH certificate authority URL", + args: "OWNER/REPO", + sshURL: "org-1234@github.com:OWNER/REPO.git", + gitProtocol: "ssh", + want: "git clone org-1234@github.com:OWNER/REPO.git", + }, + { + name: "GHE.com SSH certificate authority URL", + args: "test-prodweu01@test-prodweu01.ghe.com:OWNER/REPO.git", + sshURL: "test-prodweu01_1234@test-prodweu01.ghe.com:OWNER/REPO.git", + want: "git clone test-prodweu01_1234@test-prodweu01.ghe.com:OWNER/REPO.git", }, { name: "Non-canonical capitalization", @@ -200,6 +220,13 @@ func Test_RepoClone(t *testing.T) { args: "Owner/Repo.wiki", want: "git clone https://github.com/OWNER/REPO.wiki.git", }, + { + name: "clone wiki with SSH certificate authority URL", + args: "Owner/Repo.wiki", + sshURL: "org-1234@github.com:OWNER/REPO.git", + gitProtocol: "ssh", + want: "git clone org-1234@github.com:OWNER/REPO.wiki.git", + }, { name: "wiki URL", args: "https://github.com/owner/repo.wiki", @@ -223,6 +250,7 @@ func Test_RepoClone(t *testing.T) { "owner": { "login": "OWNER" }, + "sshUrl": "`+tt.sshURL+`", "hasWikiEnabled": true } } } `)) @@ -233,7 +261,11 @@ func Test_RepoClone(t *testing.T) { defer restore(t) cs.Register(tt.want, 0, "") - output, err := runCloneCommand(httpClient, tt.args) + cfg := config.NewMockConfig() + if tt.gitProtocol != "" { + cfg.Set("", "git_protocol", tt.gitProtocol) + } + output, err := runCloneCommandWithConfig(httpClient, cfg, tt.args) if err != nil { t.Fatalf("error running command `repo clone`: %v", err) } @@ -244,6 +276,25 @@ func Test_RepoClone(t *testing.T) { } } +func Test_RepoClone_missingSSHURL(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.GraphQL(`query RepositoryInfo\b`), + httpmock.StringResponse(` + { "data": { "repository": { + "name": "REPO", + "owner": {"login": "OWNER"} + } } } + `)) + + cfg := config.NewMockConfig() + cfg.Set("", "git_protocol", "ssh") + + _, err := runCloneCommandWithConfig(&http.Client{Transport: reg}, cfg, "OWNER/REPO") + require.EqualError(t, err, "invalid API response: repository OWNER/REPO is missing sshUrl") +} + func Test_RepoClone_hasParent(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) @@ -284,6 +335,41 @@ func Test_RepoClone_hasParent(t *testing.T) { } } +func Test_RepoClone_hasParentWithSSHCertificateAuthorityURLs(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.GraphQL(`query RepositoryInfo\b`), + httpmock.StringResponse(` + { "data": { "repository": { + "name": "REPO", + "owner": {"login": "OWNER"}, + "sshUrl": "org-1234@github.com:OWNER/REPO.git", + "parent": { + "name": "ORIG", + "owner": {"login": "hubot"}, + "sshUrl": "org-5678@github.com:hubot/ORIG.git", + "defaultBranchRef": {"name": "trunk"} + } + } } } + `)) + + cfg := config.NewMockConfig() + cfg.Set("", "git_protocol", "ssh") + httpClient := &http.Client{Transport: reg} + + cs, cmdTeardown := run.Stub() + defer cmdTeardown(t) + cs.Register(`git clone org-1234@github.com:OWNER/REPO.git`, 0, "") + cs.Register(`git -C REPO remote add -t trunk upstream org-5678@github.com:hubot/ORIG.git`, 0, "") + cs.Register(`git -C REPO fetch upstream`, 0, "") + cs.Register(`git -C REPO remote set-branches upstream *`, 0, "") + cs.Register(`git -C REPO config --add remote.upstream.gh-resolved base`, 0, "") + + _, err := runCloneCommandWithConfig(httpClient, cfg, "OWNER/REPO") + require.NoError(t, err) +} + func Test_RepoClone_hasParent_upstreamRemoteName(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) From 77e282ebe15ba810fcb213eadb2d3285e490573c Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 4 Sep 2026 15:38:11 +0200 Subject: [PATCH 2/2] Guard repository query selections Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ee65cc7a-8eaf-4f12-8c00-a94930274a0e --- api/queries_repo_test.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index 6b99c686110..dca9321d7be 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -140,21 +140,22 @@ func TestGitHubRepo_withParent(t *testing.T) { assert.False(t, repo.ViewerCanTriage()) } -// TestBaseRepoQueriesSelectDatabaseID guards the 3 queries that build the base -// repository. Each selection is written by hand, and a field left out of one -// of them is silently zero rather than an error, so the selection is asserted -// against the request instead of the response. -func TestBaseRepoQueriesSelectDatabaseID(t *testing.T) { +// TestBaseRepoQuerySelections guards fields that must be explicitly requested. +// A field left out of a query is silently zero rather than an error, so the +// selection is asserted against the request instead of the response. +func TestBaseRepoQuerySelections(t *testing.T) { tests := []struct { name string matcher httpmock.Matcher body string + fields []string call func(*Client) error }{ { name: "GitHubRepo", matcher: httpmock.GraphQL(`query RepositoryInfo\b`), body: `{ "data": { "repository": { "id": "REPOID", "databaseId": 1234 } } }`, + fields: []string{"databaseId", "sshUrl"}, call: func(client *Client) error { _, err := GitHubRepo(client, ghrepo.New("OWNER", "REPO")) return err @@ -164,6 +165,7 @@ func TestBaseRepoQueriesSelectDatabaseID(t *testing.T) { name: "RepoNetwork", matcher: httpmock.GraphQL(`query RepositoryNetwork\b`), body: `{ "data": { "repo_000": { "id": "REPOID", "databaseId": 1234 } } }`, + fields: []string{"databaseId"}, call: func(client *Client) error { _, err := RepoNetwork(client, []ghrepo.Interface{ghrepo.New("OWNER", "REPO")}) return err @@ -173,6 +175,7 @@ func TestBaseRepoQueriesSelectDatabaseID(t *testing.T) { name: "IssueRepoInfo", matcher: httpmock.GraphQL(`query IssueRepositoryInfo\b`), body: `{ "data": { "repository": { "id": "REPOID", "databaseId": 1234 } } }`, + fields: []string{"databaseId"}, call: func(client *Client) error { _, err := IssueRepoInfo(client, ghrepo.New("OWNER", "REPO")) return err @@ -191,7 +194,9 @@ func TestBaseRepoQueriesSelectDatabaseID(t *testing.T) { })) require.NoError(t, tt.call(newTestClient(httpReg))) - assert.Contains(t, query, "databaseId") + for _, field := range tt.fields { + assert.Contains(t, query, field) + } }) } }