From 1e77a88e4e7f53167a50de9b4448ba58936b5edc Mon Sep 17 00:00:00 2001 From: Yunchi Luo Date: Mon, 1 Aug 2022 16:16:50 -0400 Subject: [PATCH 1/9] repospec: support ssh urls with ssh certificates Organizations that use a custom SSH Certificate Authority with GitHub Enterprise Cloud will have a host that starts with `org-12345@` instead of `git@`. This removes the hard-coded `git@` in the repo spec parsing for a regexp match on a valid username instead. E.g. now it will be able to parse specs like `org-12345@github.com:someOrg/someRepo/README.md?ref=someBranch`. Docs [here](https://docs.github.com/en/enterprise-cloud@latest/organizations/managing-git-access-to-your-organizations-repositories/about-ssh-certificate-authorities#about-ssh-urls-with-ssh-certificates) --- api/internal/git/repospec.go | 33 +++++++++++++++++++++---------- api/internal/git/repospec_test.go | 2 ++ 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/api/internal/git/repospec.go b/api/internal/git/repospec.go index a1c29e6c71..ecb75a61a5 100644 --- a/api/internal/git/repospec.go +++ b/api/internal/git/repospec.go @@ -7,6 +7,7 @@ import ( "fmt" "net/url" "path/filepath" + "regexp" "strconv" "strings" "time" @@ -212,18 +213,26 @@ func peelQuery(arg string) (string, string, time.Duration, bool) { return parsed.Path, ref, duration, submodules } +var userRegex = regexp.MustCompile(`^[a-zA-Z][a-zA-Z0-9-]*@`) + func parseHostSpec(n string) (string, string) { var host string - // Start accumulating the host part. - for _, p := range []string{ - // Order matters here. - "git::", "gh:", "ssh://", "https://", "http://", "file://", - "git@", "github.com:", "github.com/"} { - if len(p) < len(n) && strings.ToLower(n[:len(p)]) == p { - n = n[len(p):] - host += p + consumeHostStrings := func(parts []string) { + for _, p := range parts { + if len(p) < len(n) && strings.ToLower(n[:len(p)]) == p { + n = n[len(p):] + host += p + } } } + // Start accumulating the host part. + // Order matters here. + consumeHostStrings([]string{"git::", "gh:", "ssh://", "https://", "http://", "file://"}) + if p := userRegex.FindString(n); p != "" { + n = n[len(p):] + host += p + } + consumeHostStrings([]string{"github.com:", "github.com/"}) if host == "git@" { i := strings.Index(n, "/") if i > -1 { @@ -257,10 +266,14 @@ func parseHostSpec(n string) (string, string) { func normalizeGitHostSpec(host string) string { s := strings.ToLower(host) + m := userRegex.FindString(host) if strings.Contains(s, "github.com") { - if strings.Contains(s, "git@") || strings.Contains(s, "ssh:") { + switch { + case m != "": + host = m + "github.com:" + case strings.Contains(s, "ssh:"): host = "git@github.com:" - } else { + default: host = "https://github.com/" } } diff --git a/api/internal/git/repospec_test.go b/api/internal/git/repospec_test.go index ad63b4aa02..9b71031496 100644 --- a/api/internal/git/repospec_test.go +++ b/api/internal/git/repospec_test.go @@ -35,6 +35,8 @@ func TestNewRepoSpecFromUrl_Permute(t *testing.T) { {"git::https://git.example.com/", "https://git.example.com/"}, {"git@github.com:", "git@github.com:"}, {"git@github.com/", "git@github.com:"}, + {"org-12345@github.com:", "org-12345@github.com:"}, + {"org-12345@github.com/", "org-12345@github.com:"}, } var orgRepos = []string{"someOrg/someRepo", "kubernetes/website"} var pathNames = []string{"README.md", "foo/krusty.txt", ""} From a6ca6a0f5fdb083af2df8371c5531cc1f9486895 Mon Sep 17 00:00:00 2001 From: Yunchi Luo Date: Mon, 26 Sep 2022 10:07:41 -0400 Subject: [PATCH 2/9] changes for code review --- api/internal/git/repospec.go | 35 ++++++++++++++------- api/internal/git/repospec_test.go | 51 ++++++++++++++++++++++++++----- 2 files changed, 68 insertions(+), 18 deletions(-) diff --git a/api/internal/git/repospec.go b/api/internal/git/repospec.go index ecb75a61a5..0a3639fd24 100644 --- a/api/internal/git/repospec.go +++ b/api/internal/git/repospec.go @@ -87,7 +87,10 @@ func NewRepoSpecFromURL(n string) (*RepoSpec, error) { if filepath.IsAbs(n) { return nil, fmt.Errorf("uri looks like abs path: %s", n) } - host, orgRepo, path, gitRef, gitSubmodules, suffix, gitTimeout := parseGitURL(n) + host, orgRepo, path, gitRef, gitSubmodules, suffix, gitTimeout, err := parseGitURL(n) + if err != nil { + return nil, err + } if orgRepo == "" { return nil, fmt.Errorf("url lacks orgRepo: %s", n) } @@ -110,16 +113,22 @@ const ( // https://github.com/someOrg/someRepo?ref=someHash, extract // the parts. func parseGitURL(n string) ( - host string, orgRepo string, path string, gitRef string, gitSubmodules bool, gitSuff string, gitTimeout time.Duration) { + host string, orgRepo string, path string, gitRef string, gitSubmodules bool, gitSuff string, gitTimeout time.Duration, err error) { if strings.Contains(n, gitDelimiter) { index := strings.Index(n, gitDelimiter) // Adding _git/ to host - host = normalizeGitHostSpec(n[:index+len(gitDelimiter)]) + host, err = normalizeGitHostSpec(n[:index+len(gitDelimiter)]) + if err != nil { + return + } orgRepo = strings.Split(strings.Split(n[index+len(gitDelimiter):], "/")[0], "?")[0] path, gitRef, gitTimeout, gitSubmodules = peelQuery(n[index+len(gitDelimiter)+len(orgRepo):]) return } - host, n = parseHostSpec(n) + host, n, err = parseHostSpec(n) + if err != nil { + return + } isLocal := strings.HasPrefix(host, "file://") if !isLocal { gitSuff = gitSuffix @@ -163,7 +172,7 @@ func parseGitURL(n string) ( } path = "" orgRepo, gitRef, gitTimeout, gitSubmodules = peelQuery(n) - return host, orgRepo, path, gitRef, gitSubmodules, gitSuff, gitTimeout + return host, orgRepo, path, gitRef, gitSubmodules, gitSuff, gitTimeout, nil } // Clone git submodules by default. @@ -213,9 +222,9 @@ func peelQuery(arg string) (string, string, time.Duration, bool) { return parsed.Path, ref, duration, submodules } -var userRegex = regexp.MustCompile(`^[a-zA-Z][a-zA-Z0-9-]*@`) +var userRegex = regexp.MustCompile(`[a-zA-Z][a-zA-Z0-9-]*@`) -func parseHostSpec(n string) (string, string) { +func parseHostSpec(n string) (string, string, error) { var host string consumeHostStrings := func(parts []string) { for _, p := range parts { @@ -245,7 +254,7 @@ func parseHostSpec(n string) (string, string) { n = n[i+1:] } } - return host, n + return host, n, nil } // If host is a http(s) or ssh URL, grab the domain part. @@ -261,15 +270,19 @@ func parseHostSpec(n string) (string, string) { } } - return normalizeGitHostSpec(host), n + host, err := normalizeGitHostSpec(host) + return host, n, err } -func normalizeGitHostSpec(host string) string { +func normalizeGitHostSpec(host string) (string, error) { s := strings.ToLower(host) m := userRegex.FindString(host) if strings.Contains(s, "github.com") { switch { case m != "": + if strings.HasPrefix(s, "git::") && m != "git" { + return "", fmt.Errorf("git protocol on github.com only allows git@ user") + } host = m + "github.com:" case strings.Contains(s, "ssh:"): host = "git@github.com:" @@ -280,7 +293,7 @@ func normalizeGitHostSpec(host string) string { if strings.HasPrefix(s, "git::") { host = strings.TrimPrefix(s, "git::") } - return host + return host, nil } // The format of Azure repo URL is documented diff --git a/api/internal/git/repospec_test.go b/api/internal/git/repospec_test.go index 9b71031496..9aceab379a 100644 --- a/api/internal/git/repospec_test.go +++ b/api/internal/git/repospec_test.go @@ -37,6 +37,7 @@ func TestNewRepoSpecFromUrl_Permute(t *testing.T) { {"git@github.com/", "git@github.com:"}, {"org-12345@github.com:", "org-12345@github.com:"}, {"org-12345@github.com/", "org-12345@github.com:"}, + {"git::git@github.com:", "git@github.com:"}, } var orgRepos = []string{"someOrg/someRepo", "kubernetes/website"} var pathNames = []string{"README.md", "foo/krusty.txt", ""} @@ -101,16 +102,19 @@ func TestNewRepoSpecFromUrlErrors(t *testing.T) { {"htxxxtp://github.com/", "url lacks host"}, {"ssh://git.example.com", "url lacks orgRepo"}, {"git::___", "url lacks orgRepo"}, + {"git::org-12345@github.com:kubernetes-sigs/kustomize", "git protocol on github.com only allows git@ user"}, } for _, testCase := range badData { - _, err := NewRepoSpecFromURL(testCase.url) - if err == nil { - t.Error("expected error") - } - if !strings.Contains(err.Error(), testCase.error) { - t.Errorf("unexpected error: %s", err) - } + t.Run(testCase.error, func(t *testing.T) { + _, err := NewRepoSpecFromURL(testCase.url) + if err == nil { + t.Fatal("expected error") + } + if !strings.Contains(err.Error(), testCase.error) { + t.Errorf("unexpected error: %s", err) + } + }) } } @@ -409,6 +413,39 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { GitSuffix: ".git", }, }, + { + name: "t25", + input: "https://org-12345@github.com/kubernetes-sigs/kustomize", + cloneSpec: "org-12345@github.com:kubernetes-sigs/kustomize.git", + absPath: notCloned.String(), + repoSpec: RepoSpec{ + Host: "org-12345@github.com:", + OrgRepo: "kubernetes-sigs/kustomize", + GitSuffix: ".git", + }, + }, + { + name: "t26", + input: "ssh://org-12345@github.com/kubernetes-sigs/kustomize", + cloneSpec: "org-12345@github.com:kubernetes-sigs/kustomize.git", + absPath: notCloned.String(), + repoSpec: RepoSpec{ + Host: "org-12345@github.com:", + OrgRepo: "kubernetes-sigs/kustomize", + GitSuffix: ".git", + }, + }, + { + name: "t27", + input: "org-12345@github.com/kubernetes-sigs/kustomize", + cloneSpec: "org-12345@github.com:kubernetes-sigs/kustomize.git", + absPath: notCloned.String(), + repoSpec: RepoSpec{ + Host: "org-12345@github.com:", + OrgRepo: "kubernetes-sigs/kustomize", + GitSuffix: ".git", + }, + }, } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { From 1841f0b94fc414115d88b3957ae01dd405a0c4ee Mon Sep 17 00:00:00 2001 From: Yunchi Luo Date: Mon, 26 Sep 2022 10:32:26 -0400 Subject: [PATCH 3/9] more tests --- api/internal/git/repospec.go | 12 ++++++------ api/internal/git/repospec_test.go | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/api/internal/git/repospec.go b/api/internal/git/repospec.go index 0a3639fd24..44f44eba4c 100644 --- a/api/internal/git/repospec.go +++ b/api/internal/git/repospec.go @@ -222,8 +222,6 @@ func peelQuery(arg string) (string, string, time.Duration, bool) { return parsed.Path, ref, duration, submodules } -var userRegex = regexp.MustCompile(`[a-zA-Z][a-zA-Z0-9-]*@`) - func parseHostSpec(n string) (string, string, error) { var host string consumeHostStrings := func(parts []string) { @@ -274,16 +272,18 @@ func parseHostSpec(n string) (string, string, error) { return host, n, err } +var userRegex = regexp.MustCompile(`^(?:ssh://|git::)?([a-zA-Z][a-zA-Z0-9-]*@)`) + func normalizeGitHostSpec(host string) (string, error) { s := strings.ToLower(host) - m := userRegex.FindString(host) + m := userRegex.FindStringSubmatch(host) if strings.Contains(s, "github.com") { switch { - case m != "": - if strings.HasPrefix(s, "git::") && m != "git" { + case len(m) > 0: + if strings.HasPrefix(s, "git::") && m[1] != "git@" { return "", fmt.Errorf("git protocol on github.com only allows git@ user") } - host = m + "github.com:" + host = m[1] + "github.com:" case strings.Contains(s, "ssh:"): host = "git@github.com:" default: diff --git a/api/internal/git/repospec_test.go b/api/internal/git/repospec_test.go index 9aceab379a..b2f399e993 100644 --- a/api/internal/git/repospec_test.go +++ b/api/internal/git/repospec_test.go @@ -416,10 +416,10 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { { name: "t25", input: "https://org-12345@github.com/kubernetes-sigs/kustomize", - cloneSpec: "org-12345@github.com:kubernetes-sigs/kustomize.git", + cloneSpec: "https://github.com/kubernetes-sigs/kustomize.git", absPath: notCloned.String(), repoSpec: RepoSpec{ - Host: "org-12345@github.com:", + Host: "https://github.com/", OrgRepo: "kubernetes-sigs/kustomize", GitSuffix: ".git", }, From b84d5971ff93538cddcbe2bb9e322ab89a1a5569 Mon Sep 17 00:00:00 2001 From: Yunchi Luo Date: Fri, 30 Sep 2022 14:35:13 -0400 Subject: [PATCH 4/9] remove switch/case --- api/internal/git/repospec.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/api/internal/git/repospec.go b/api/internal/git/repospec.go index 44f44eba4c..7c9c6dea80 100644 --- a/api/internal/git/repospec.go +++ b/api/internal/git/repospec.go @@ -278,15 +278,14 @@ func normalizeGitHostSpec(host string) (string, error) { s := strings.ToLower(host) m := userRegex.FindStringSubmatch(host) if strings.Contains(s, "github.com") { - switch { - case len(m) > 0: + if len(m) > 0 { if strings.HasPrefix(s, "git::") && m[1] != "git@" { return "", fmt.Errorf("git protocol on github.com only allows git@ user") } host = m[1] + "github.com:" - case strings.Contains(s, "ssh:"): + } else if strings.Contains(s, "ssh:") { host = "git@github.com:" - default: + } else { host = "https://github.com/" } } From 1827c6959eab6adea6cf965a58ec8135498222b5 Mon Sep 17 00:00:00 2001 From: Yunchi Luo Date: Fri, 30 Sep 2022 15:28:51 -0400 Subject: [PATCH 5/9] change parseGitURL to return a RepoSpec --- api/internal/git/repospec.go | 55 ++++++++++++++++++++++++------------ 1 file changed, 37 insertions(+), 18 deletions(-) diff --git a/api/internal/git/repospec.go b/api/internal/git/repospec.go index 7c9c6dea80..bbb12f9a18 100644 --- a/api/internal/git/repospec.go +++ b/api/internal/git/repospec.go @@ -87,20 +87,17 @@ func NewRepoSpecFromURL(n string) (*RepoSpec, error) { if filepath.IsAbs(n) { return nil, fmt.Errorf("uri looks like abs path: %s", n) } - host, orgRepo, path, gitRef, gitSubmodules, suffix, gitTimeout, err := parseGitURL(n) + rs, err := parseGitURL(n) if err != nil { return nil, err } - if orgRepo == "" { + if rs.OrgRepo == "" { return nil, fmt.Errorf("url lacks orgRepo: %s", n) } - if host == "" { + if rs.Host == "" { return nil, fmt.Errorf("url lacks host: %s", n) } - return &RepoSpec{ - raw: n, Host: host, OrgRepo: orgRepo, - Dir: notCloned, Path: path, Ref: gitRef, GitSuffix: suffix, - Submodules: gitSubmodules, Timeout: gitTimeout}, nil + return rs, nil } const ( @@ -112,22 +109,44 @@ const ( // From strings like git@github.com:someOrg/someRepo.git or // https://github.com/someOrg/someRepo?ref=someHash, extract // the parts. -func parseGitURL(n string) ( - host string, orgRepo string, path string, gitRef string, gitSubmodules bool, gitSuff string, gitTimeout time.Duration, err error) { +func parseGitURL(n string) (*RepoSpec, error) { + var ( + host string + orgRepo string + path string + gitRef string + gitSubmodules bool + gitSuff string + gitTimeout time.Duration + err error + ) + newSpec := func() *RepoSpec { + return &RepoSpec{ + raw: n, + Host: host, + OrgRepo: orgRepo, + Dir: notCloned, + Path: path, + Ref: gitRef, + GitSuffix: gitSuff, + Submodules: gitSubmodules, + Timeout: gitTimeout, + } + } if strings.Contains(n, gitDelimiter) { index := strings.Index(n, gitDelimiter) // Adding _git/ to host host, err = normalizeGitHostSpec(n[:index+len(gitDelimiter)]) if err != nil { - return + return nil, err } orgRepo = strings.Split(strings.Split(n[index+len(gitDelimiter):], "/")[0], "?")[0] path, gitRef, gitTimeout, gitSubmodules = peelQuery(n[index+len(gitDelimiter)+len(orgRepo):]) - return + return newSpec(), nil } host, n, err = parseHostSpec(n) if err != nil { - return + return nil, err } isLocal := strings.HasPrefix(host, "file://") if !isLocal { @@ -142,7 +161,7 @@ func parseGitURL(n string) ( n = n[1:] } path, gitRef, gitTimeout, gitSubmodules = peelQuery(n) - return + return newSpec(), nil } if isLocal { @@ -150,29 +169,29 @@ func parseGitURL(n string) ( orgRepo = n[:idx] n = n[idx+2:] path, gitRef, gitTimeout, gitSubmodules = peelQuery(n) - return + return newSpec(), nil } path, gitRef, gitTimeout, gitSubmodules = peelQuery(n) orgRepo = path path = "" - return + return newSpec(), nil } i := strings.Index(n, "/") if i < 1 { path, gitRef, gitTimeout, gitSubmodules = peelQuery(n) - return + return newSpec(), nil } j := strings.Index(n[i+1:], "/") if j >= 0 { j += i + 1 orgRepo = n[:j] path, gitRef, gitTimeout, gitSubmodules = peelQuery(n[j+1:]) - return + return newSpec(), nil } path = "" orgRepo, gitRef, gitTimeout, gitSubmodules = peelQuery(n) - return host, orgRepo, path, gitRef, gitSubmodules, gitSuff, gitTimeout, nil + return newSpec(), nil } // Clone git submodules by default. From 60b4a769292e422b46e7c8eac601b002273daa54 Mon Sep 17 00:00:00 2001 From: Yunchi Luo Date: Fri, 30 Sep 2022 17:34:37 -0400 Subject: [PATCH 6/9] Revert "remove switch/case" This reverts commit b84d5971ff93538cddcbe2bb9e322ab89a1a5569. --- api/internal/git/repospec.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/api/internal/git/repospec.go b/api/internal/git/repospec.go index bbb12f9a18..cb5218abca 100644 --- a/api/internal/git/repospec.go +++ b/api/internal/git/repospec.go @@ -297,14 +297,15 @@ func normalizeGitHostSpec(host string) (string, error) { s := strings.ToLower(host) m := userRegex.FindStringSubmatch(host) if strings.Contains(s, "github.com") { - if len(m) > 0 { + switch { + case len(m) > 0: if strings.HasPrefix(s, "git::") && m[1] != "git@" { return "", fmt.Errorf("git protocol on github.com only allows git@ user") } host = m[1] + "github.com:" - } else if strings.Contains(s, "ssh:") { + case strings.Contains(s, "ssh:"): host = "git@github.com:" - } else { + default: host = "https://github.com/" } } From 9200100f21daea9ece1a9cddfad8d8c7cba1eda5 Mon Sep 17 00:00:00 2001 From: Yunchi Luo Date: Fri, 30 Sep 2022 19:43:08 -0400 Subject: [PATCH 7/9] simplify parseGitURL returns --- api/internal/git/repospec.go | 77 +++++++++++++----------------------- 1 file changed, 28 insertions(+), 49 deletions(-) diff --git a/api/internal/git/repospec.go b/api/internal/git/repospec.go index cb5218abca..cc8bad5ee4 100644 --- a/api/internal/git/repospec.go +++ b/api/internal/git/repospec.go @@ -110,88 +110,67 @@ const ( // https://github.com/someOrg/someRepo?ref=someHash, extract // the parts. func parseGitURL(n string) (*RepoSpec, error) { - var ( - host string - orgRepo string - path string - gitRef string - gitSubmodules bool - gitSuff string - gitTimeout time.Duration - err error - ) - newSpec := func() *RepoSpec { - return &RepoSpec{ - raw: n, - Host: host, - OrgRepo: orgRepo, - Dir: notCloned, - Path: path, - Ref: gitRef, - GitSuffix: gitSuff, - Submodules: gitSubmodules, - Timeout: gitTimeout, - } - } + var err error + rs := &RepoSpec{raw: n, Dir: notCloned} if strings.Contains(n, gitDelimiter) { index := strings.Index(n, gitDelimiter) // Adding _git/ to host - host, err = normalizeGitHostSpec(n[:index+len(gitDelimiter)]) + rs.Host, err = normalizeGitHostSpec(n[:index+len(gitDelimiter)]) if err != nil { return nil, err } - orgRepo = strings.Split(strings.Split(n[index+len(gitDelimiter):], "/")[0], "?")[0] - path, gitRef, gitTimeout, gitSubmodules = peelQuery(n[index+len(gitDelimiter)+len(orgRepo):]) - return newSpec(), nil + rs.OrgRepo = strings.Split(strings.Split(n[index+len(gitDelimiter):], "/")[0], "?")[0] + rs.Path, rs.Ref, rs.Timeout, rs.Submodules = peelQuery(n[index+len(gitDelimiter)+len(rs.OrgRepo):]) + return rs, nil } - host, n, err = parseHostSpec(n) + rs.Host, n, err = parseHostSpec(n) if err != nil { return nil, err } - isLocal := strings.HasPrefix(host, "file://") + isLocal := strings.HasPrefix(rs.Host, "file://") if !isLocal { - gitSuff = gitSuffix + rs.GitSuffix = gitSuffix } if strings.Contains(n, gitSuffix) { - gitSuff = gitSuffix + rs.GitSuffix = gitSuffix index := strings.Index(n, gitSuffix) - orgRepo = n[0:index] + rs.OrgRepo = n[0:index] n = n[index+len(gitSuffix):] if len(n) > 0 && n[0] == '/' { n = n[1:] } - path, gitRef, gitTimeout, gitSubmodules = peelQuery(n) - return newSpec(), nil + rs.Path, rs.Ref, rs.Timeout, rs.Submodules = peelQuery(n) + return rs, nil } if isLocal { if idx := strings.Index(n, "//"); idx > 0 { - orgRepo = n[:idx] + rs.OrgRepo = n[:idx] n = n[idx+2:] - path, gitRef, gitTimeout, gitSubmodules = peelQuery(n) - return newSpec(), nil + rs.Path, rs.Ref, rs.Timeout, rs.Submodules = peelQuery(n) + return rs, nil } - path, gitRef, gitTimeout, gitSubmodules = peelQuery(n) - orgRepo = path - path = "" - return newSpec(), nil + rs.Path, rs.Ref, rs.Timeout, rs.Submodules = peelQuery(n) + rs.OrgRepo = rs.Path + rs.Path = "" + return rs, nil } i := strings.Index(n, "/") if i < 1 { - path, gitRef, gitTimeout, gitSubmodules = peelQuery(n) - return newSpec(), nil + rs.Path, rs.Ref, rs.Timeout, rs.Submodules = peelQuery(n) + return rs, nil } j := strings.Index(n[i+1:], "/") if j >= 0 { j += i + 1 - orgRepo = n[:j] - path, gitRef, gitTimeout, gitSubmodules = peelQuery(n[j+1:]) - return newSpec(), nil + rs.OrgRepo = n[:j] + rs.Path, rs.Ref, rs.Timeout, rs.Submodules = peelQuery(n[j+1:]) + return rs, nil } - path = "" - orgRepo, gitRef, gitTimeout, gitSubmodules = peelQuery(n) - return newSpec(), nil + rs.Path = "" + rs.OrgRepo, rs.Ref, rs.Timeout, rs.Submodules = peelQuery(n) + return rs, nil } // Clone git submodules by default. From 62c772ab5a3c8529f2038e5ea55dc551b7f9b2eb Mon Sep 17 00:00:00 2001 From: Yunchi Luo Date: Mon, 3 Oct 2022 17:45:38 -0400 Subject: [PATCH 8/9] add regression test for git::ssh:// --- api/internal/git/repospec_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/api/internal/git/repospec_test.go b/api/internal/git/repospec_test.go index b2f399e993..436f024bf1 100644 --- a/api/internal/git/repospec_test.go +++ b/api/internal/git/repospec_test.go @@ -446,6 +446,18 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { GitSuffix: ".git", }, }, + { + name: "t28", + input: "git::ssh://git@github.com/someorg/somerepo/somepath", + cloneSpec: "ssh://git@github.com/someorg/somerepo.git", + absPath: notCloned.Join("somepath"), + repoSpec: RepoSpec{ + Host: "ssh://git@github.com/", + OrgRepo: "someorg/somerepo", + Path: "somepath", + GitSuffix: ".git", + }, + }, } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { From 201ee234965e695a1d43af656be6d8da22515b75 Mon Sep 17 00:00:00 2001 From: Yunchi Luo Date: Tue, 4 Oct 2022 16:13:11 -0400 Subject: [PATCH 9/9] style fixes --- api/internal/git/repospec.go | 50 +++++++++++++++++++++---------- api/internal/git/repospec_test.go | 6 ++-- 2 files changed, 38 insertions(+), 18 deletions(-) diff --git a/api/internal/git/repospec.go b/api/internal/git/repospec.go index cc8bad5ee4..06b0552056 100644 --- a/api/internal/git/repospec.go +++ b/api/internal/git/repospec.go @@ -220,6 +220,8 @@ func peelQuery(arg string) (string, string, time.Duration, bool) { return parsed.Path, ref, duration, submodules } +var userRegexp = regexp.MustCompile(`^([a-zA-Z][a-zA-Z0-9-]*)@`) + func parseHostSpec(n string) (string, string, error) { var host string consumeHostStrings := func(parts []string) { @@ -233,7 +235,7 @@ func parseHostSpec(n string) (string, string, error) { // Start accumulating the host part. // Order matters here. consumeHostStrings([]string{"git::", "gh:", "ssh://", "https://", "http://", "file://"}) - if p := userRegex.FindString(n); p != "" { + if p := userRegexp.FindString(n); p != "" { n = n[len(p):] host += p } @@ -270,27 +272,45 @@ func parseHostSpec(n string) (string, string, error) { return host, n, err } -var userRegex = regexp.MustCompile(`^(?:ssh://|git::)?([a-zA-Z][a-zA-Z0-9-]*@)`) +var githubRegexp = regexp.MustCompile(`^(?:ssh://)?([a-zA-Z][a-zA-Z0-9-]*)@(github.com[:/]?)`) func normalizeGitHostSpec(host string) (string, error) { s := strings.ToLower(host) - m := userRegex.FindStringSubmatch(host) - if strings.Contains(s, "github.com") { + + // The git:: syntax is meant to force the Git protocol (separate from SSH + // and HTTPS), but we drop it here, to preserve past behavior. + isGitProtocol := strings.HasPrefix(s, "git::") + if isGitProtocol { + host = strings.TrimPrefix(s, "git::") + } + + // Special treatment for github.com + if strings.Contains(host, "github.com") { + m := githubRegexp.FindStringSubmatch(host) + if m == nil { + return "https://github.com/", nil + } + userName, realHost := m[1], m[2] + + if realHost == "github.com/" { + realHost = "github.com:" + } + + const gitUser = "git" + isGitUser := userName == gitUser || userName == "" + if userName == "" { + userName = gitUser + } + switch { - case len(m) > 0: - if strings.HasPrefix(s, "git::") && m[1] != "git@" { - return "", fmt.Errorf("git protocol on github.com only allows git@ user") - } - host = m[1] + "github.com:" - case strings.Contains(s, "ssh:"): - host = "git@github.com:" + case isGitProtocol && !isGitUser: + return "", fmt.Errorf("git protocol on github.com only allows git@ user") + case isGitProtocol: + return "git@github.com:", nil default: - host = "https://github.com/" + return fmt.Sprintf("%s@%s", userName, realHost), nil } } - if strings.HasPrefix(s, "git::") { - host = strings.TrimPrefix(s, "git::") - } return host, nil } diff --git a/api/internal/git/repospec_test.go b/api/internal/git/repospec_test.go index 436f024bf1..80c3b8a4b0 100644 --- a/api/internal/git/repospec_test.go +++ b/api/internal/git/repospec_test.go @@ -448,11 +448,11 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { }, { name: "t28", - input: "git::ssh://git@github.com/someorg/somerepo/somepath", - cloneSpec: "ssh://git@github.com/someorg/somerepo.git", + input: "git@github.com/someorg/somerepo/somepath", + cloneSpec: "git@github.com:someorg/somerepo.git", absPath: notCloned.Join("somepath"), repoSpec: RepoSpec{ - Host: "ssh://git@github.com/", + Host: "git@github.com:", OrgRepo: "someorg/somerepo", Path: "somepath", GitSuffix: ".git",