From fbb94584dc7f7ffc138507937c1b4a7bd9a11a2f Mon Sep 17 00:00:00 2001 From: Anna Song Date: Wed, 14 Dec 2022 20:48:40 -0500 Subject: [PATCH 01/15] Refactor parseHostSpec Fix ssh parsing in issue 4847 --- api/internal/git/repospec.go | 112 +++++++++++++++++++++--------- api/internal/git/repospec_test.go | 37 +++++++--- 2 files changed, 105 insertions(+), 44 deletions(-) diff --git a/api/internal/git/repospec.go b/api/internal/git/repospec.go index cae95b4450..542010407d 100644 --- a/api/internal/git/repospec.go +++ b/api/internal/git/repospec.go @@ -228,46 +228,90 @@ func parsePath(n string) string { } 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 - } + // We used to use go-getter to handle our urls: https://github.com/hashicorp/go-getter. + // This prefix signaled go-getter to use the git protocol to fetch the url's contents. + // We still accept this prefix. + n, _ = trimPrefixIgnoreCase(n, "git::") + // We support gh: assuming authors use it as a github shorthand, specified in .gitconfig. + if rest, found := trimPrefixIgnoreCase(n, "gh:"); found { + return "gh:", rest + } + scheme, rest, _ := strings.Cut(n, "://") + switch scheme = strings.ToLower(scheme); scheme { + // no scheme + case strings.ToLower(n): + scheme = "" + rest = n + // The file protocol specifies an absolute path to a local git repo. There is no host. + case "file": + return "file://", rest + case "https", "http", "ssh": + scheme += "://" + // We either + // 1. do not support said scheme or + // 2. found a part of the path because there is no scheme. + // Instead of determining the case, we try to match the host as if under the 2nd case. + // If we are actually under the first, host matching will fail. + default: + scheme = "" + rest = n } - if host == "git@" { - i := strings.Index(n, "/") - if i > -1 { - host += n[:i+1] - n = n[i+1:] + return matchHost(scheme, rest) +} + +// trimPrefixIgnoreCase returns the rest of s and true if prefix, ignoring case, prefixes s. +// Otherwise, trimPrefixIgnoreCase returns s and false. +func trimPrefixIgnoreCase(s, prefix string) (string, bool) { + if len(prefix) <= len(s) && strings.ToLower(s[:len(prefix)]) == prefix { + return s[len(prefix):], true + } + return s, false +} + +// matchHost returns a host that kustomize recognizes and the rest of s given scheme and url s. +// scheme can be any of http;//, https://, ssh://, or empty. +func matchHost(scheme, s string) (host, rest string) { + var isSCP bool + if s, isSCP = trimPrefixIgnoreCase(s, "git@"); isSCP { + host = "git@" + } + const httpGithub = "https://github.com/" + const scpGithub = "git@github.com:" + var normalized, separator string + switch scheme { + case "": + if isSCP { + normalized = scpGithub + separator = ":" } else { - i = strings.Index(n, ":") - if i > -1 { - host += n[:i+1] - n = n[i+1:] - } + normalized = httpGithub } - return host, n + case "https://", "http://": + normalized = httpGithub + separator = "/" + case "ssh://": + normalized = scpGithub + separator = "/" } - - // If host is a http(s) or ssh URL, grab the domain part. - for _, p := range []string{ - "ssh://", "https://", "http://"} { - if strings.HasSuffix(host, p) { - i := strings.Index(n, "/") - if i > -1 { - host += n[0 : i+1] - n = n[i+1:] - } - break + for _, builtin := range []string{"github.com:", "github.com/"} { + rest, found := trimPrefixIgnoreCase(s, builtin) + if found { + return normalized, rest } } - - return normalizeGitHostSpec(host), n + i := strings.Index(s, separator) + // There is no host if the separator was not found or the separator delimits an empty + // host. Note that this will happen if the separator is empty. + if i <= 0 { + return "", s + } + if separator == ":" { + // The colon acts as a delimiter for scp protocol only if not prefixed by '/'. + if slashIndex := strings.Index(s, "/"); slashIndex != -1 && slashIndex < i { + return "", s + } + } + return scheme + host + s[:i+1], s[i+1:] } func normalizeGitHostSpec(host string) string { diff --git a/api/internal/git/repospec_test.go b/api/internal/git/repospec_test.go index 1d371b45a0..b6ff7983e0 100644 --- a/api/internal/git/repospec_test.go +++ b/api/internal/git/repospec_test.go @@ -89,6 +89,10 @@ func TestNewRepoSpecFromUrlErrors(t *testing.T) { "htxxxtp://github.com/", "url lacks host", }, + "bad_scp": { + "git@local/path:file/system", + "url lacks host", + }, "no_org_repo": { "ssh://git.example.com", "url lacks repoPath", @@ -191,27 +195,40 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { }, { name: "t6", - input: "git@gitlab2.sqtools.ru:10022/infra/kubernetes/thanos-base.git?ref=v0.1.0", - cloneSpec: "git@gitlab2.sqtools.ru:10022/infra/kubernetes/thanos-base.git", + input: "git@gitlab2.sqtools.ru:infra/kubernetes/thanos-base.git?ref=v0.1.0", + cloneSpec: "git@gitlab2.sqtools.ru:infra/kubernetes/thanos-base.git", absPath: notCloned.String(), repoSpec: RepoSpec{ - Host: "git@gitlab2.sqtools.ru:10022/", - RepoPath: "infra/kubernetes/thanos-base", + Host: "git@gitlab2.sqtools.ru:", + RepoPath: "infra/kubernetes/thanos-base", Ref: "v0.1.0", GitSuffix: ".git", }, }, { - name: "t7", + name: "non-github_scp", input: "git@bitbucket.org:company/project.git//path?ref=branch", cloneSpec: "git@bitbucket.org:company/project.git", absPath: notCloned.Join("path"), repoSpec: RepoSpec{ - Host: "git@bitbucket.org:company/", - RepoPath: "project", - KustRootPath: "/path", - Ref: "branch", - GitSuffix: ".git", + Host: "git@bitbucket.org:", + RepoPath: "company/project", + KustRootPath: "/path", + Ref: "branch", + GitSuffix: ".git", + }, + }, + { + name: "non-github_git-user_ssh", + input: "ssh://git@bitbucket.org/company/project.git//path?ref=branch", + cloneSpec: "ssh://git@bitbucket.org/company/project.git", + absPath: notCloned.Join("path"), + repoSpec: RepoSpec{ + Host: "ssh://git@bitbucket.org/", + RepoPath: "company/project", + KustRootPath: "/path", + Ref: "branch", + GitSuffix: ".git", }, }, { From dc05fa153f79e5ad2b04286cfe61878dbf493232 Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Tue, 3 Jan 2023 18:40:38 -0500 Subject: [PATCH 02/15] Additional repospec refactoring --- api/internal/git/repospec.go | 146 ++++++++++++++++-------------- api/internal/git/repospec_test.go | 32 ++++++- 2 files changed, 111 insertions(+), 67 deletions(-) diff --git a/api/internal/git/repospec.go b/api/internal/git/repospec.go index 542010407d..f261043338 100644 --- a/api/internal/git/repospec.go +++ b/api/internal/git/repospec.go @@ -232,31 +232,63 @@ func parseHostSpec(n string) (string, string) { // This prefix signaled go-getter to use the git protocol to fetch the url's contents. // We still accept this prefix. n, _ = trimPrefixIgnoreCase(n, "git::") - // We support gh: assuming authors use it as a github shorthand, specified in .gitconfig. - if rest, found := trimPrefixIgnoreCase(n, "gh:"); found { - return "gh:", rest + + scheme, n := parseScheme(n) + if scheme == "file://" || scheme == "gh:" { + // We support gh: assuming authors use it as a github shorthand, specified in .gitconfig. + // The file protocol specifies an absolute path to a local git repo. There is no host. + // In both cases, we return the scheme and the rest of the url. + return scheme, n + } + + username, n := parseUsername(n) + + if rest, isGithubURL := trimGithubHost(n); isGithubURL { + // we strictly normalize Github URLs; this may discard scheme and username components in some cases. + return normalizedGithubHost(scheme, username), rest + } + + sepIndex := findPathSeparator(n, username == gitUsername) + host, rest := n[:sepIndex+1], n[sepIndex+1:] + + if !validHostSpecParsed(scheme, username, host) { + return "", n + } + return scheme + username + host, rest +} + +func validHostSpecParsed(scheme, username, host string) bool { + isSCP := username == gitUsername + return len(host) > 0 && + (scheme != "" || isSCP) // scheme may be blank for URLs like git@github.com:org/repo +} + +func parseScheme(s string) (string, string) { + var scheme string + for _, prefix := range []string{"gh:", "ssh://", "https://", "http://", "file://"} { + if rest, found := trimPrefixIgnoreCase(s, prefix); found { + scheme = prefix + s = rest + break + } } - scheme, rest, _ := strings.Cut(n, "://") - switch scheme = strings.ToLower(scheme); scheme { - // no scheme - case strings.ToLower(n): - scheme = "" - rest = n - // The file protocol specifies an absolute path to a local git repo. There is no host. - case "file": - return "file://", rest - case "https", "http", "ssh": - scheme += "://" - // We either - // 1. do not support said scheme or - // 2. found a part of the path because there is no scheme. - // Instead of determining the case, we try to match the host as if under the 2nd case. - // If we are actually under the first, host matching will fail. - default: - scheme = "" - rest = n + return scheme, s +} + +func parseUsername(s string) (string, string) { + if trimmed, found := trimPrefixIgnoreCase(s, gitUsername); found { + return gitUsername, trimmed } - return matchHost(scheme, rest) + return "", s +} + +func trimGithubHost(s string) (string, bool) { + for _, prefix := range []string{"github.com/", "github.com:"} { + if trimmed, found := trimPrefixIgnoreCase(s, prefix); found { + return trimmed, true + } + } + return s, false } // trimPrefixIgnoreCase returns the rest of s and true if prefix, ignoring case, prefixes s. @@ -268,59 +300,41 @@ func trimPrefixIgnoreCase(s, prefix string) (string, bool) { return s, false } -// matchHost returns a host that kustomize recognizes and the rest of s given scheme and url s. -// scheme can be any of http;//, https://, ssh://, or empty. -func matchHost(scheme, s string) (host, rest string) { - var isSCP bool - if s, isSCP = trimPrefixIgnoreCase(s, "git@"); isSCP { - host = "git@" +func findPathSeparator(hostPath string, isSCP bool) int { + if strings.Contains(hostPath, "://") { + // This function assumes that the hostPath does not contain a scheme. + // If it does, we return -1 to indicate that the hostPath is invalid. + return -1 } - const httpGithub = "https://github.com/" - const scpGithub = "git@github.com:" - var normalized, separator string - switch scheme { - case "": - if isSCP { - normalized = scpGithub - separator = ":" - } else { - normalized = httpGithub - } - case "https://", "http://": - normalized = httpGithub - separator = "/" - case "ssh://": - normalized = scpGithub - separator = "/" - } - for _, builtin := range []string{"github.com:", "github.com/"} { - rest, found := trimPrefixIgnoreCase(s, builtin) - if found { - return normalized, rest - } - } - i := strings.Index(s, separator) - // There is no host if the separator was not found or the separator delimits an empty - // host. Note that this will happen if the separator is empty. - if i <= 0 { - return "", s - } - if separator == ":" { + + sepIndex := strings.Index(hostPath, "/") + if isSCP { // The colon acts as a delimiter for scp protocol only if not prefixed by '/'. - if slashIndex := strings.Index(s, "/"); slashIndex != -1 && slashIndex < i { - return "", s + if colonIndex := strings.Index(hostPath, ":"); colonIndex > 0 && colonIndex < sepIndex { + sepIndex = colonIndex } } - return scheme + host + s[:i+1], s[i+1:] + return sepIndex +} + +const normalizedHTTPGithub = "https://github.com/" +const gitUsername = "git@" +const normalizedSCPGithub = gitUsername + "github.com:" + +func normalizedGithubHost(scheme, username string) string { + if strings.HasPrefix(scheme, "ssh://") || username == gitUsername { + return normalizedSCPGithub + } + return normalizedHTTPGithub } func normalizeGitHostSpec(host string) string { s := strings.ToLower(host) if strings.Contains(s, "github.com") { - if strings.Contains(s, "git@") || strings.Contains(s, "ssh:") { - host = "git@github.com:" + if strings.Contains(s, gitUsername) || strings.Contains(s, "ssh:") { + host = normalizedSCPGithub } else { - host = "https://github.com/" + host = normalizedHTTPGithub } } if strings.HasPrefix(s, "git::") { diff --git a/api/internal/git/repospec_test.go b/api/internal/git/repospec_test.go index b6ff7983e0..603631758b 100644 --- a/api/internal/git/repospec_test.go +++ b/api/internal/git/repospec_test.go @@ -34,6 +34,7 @@ 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:"}, + {"git::git@github.com:", "git@github.com:"}, } var repoPaths = []string{"someOrg/someRepo", "kubernetes/website"} var pathNames = []string{"README.md", "foo/krusty.txt", ""} @@ -81,6 +82,10 @@ func TestNewRepoSpecFromUrlErrors(t *testing.T) { "/tmp", "uri looks like abs path", }, + "relative path": { + "../../tmp", + "url lacks host", + }, "no_slashes": { "iauhsdiuashduas", "url lacks repoPath", @@ -91,7 +96,7 @@ func TestNewRepoSpecFromUrlErrors(t *testing.T) { }, "bad_scp": { "git@local/path:file/system", - "url lacks host", + "url lacks orgRepo", }, "no_org_repo": { "ssh://git.example.com", @@ -109,6 +114,18 @@ func TestNewRepoSpecFromUrlErrors(t *testing.T) { "https://github.com/org/repo.git//path/../../exits/repo", "url path exits repo", }, + "bad github separator": { + "github.com!org/repo.git//path", + "url lacks host", + }, + "unsupported protocol after username": { + "git@scp://github.com/org/repo.git//path", + "url lacks host", + }, + "supported protocol after username": { + "git@https://github.com/org/repo.git//path", + "url lacks host", + }, } for name, testCase := range badData { @@ -476,6 +493,19 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { GitSuffix: ".git", }, }, + { + name: "non-git username with non-github host", + input: "ssh://myusername@bitbucket.org/ourteamname/ourrepositoryname.git//path?ref=branch", + cloneSpec: "ssh://myusername@bitbucket.org/ourteamname/ourrepositoryname.git", + absPath: notCloned.Join("path"), + repoSpec: RepoSpec{ + Host: "ssh://myusername@bitbucket.org/", + OrgRepo: "ourteamname/ourrepositoryname", + Path: "/path", + Ref: "branch", + GitSuffix: ".git", + }, + }, } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { From 7e6cc7aafb8f88c1a5e80bbc037f59ef967c98b8 Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Thu, 5 Jan 2023 17:17:41 -0500 Subject: [PATCH 03/15] Review feedback --- api/internal/git/repospec.go | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/api/internal/git/repospec.go b/api/internal/git/repospec.go index f261043338..c536b8c12d 100644 --- a/api/internal/git/repospec.go +++ b/api/internal/git/repospec.go @@ -128,7 +128,7 @@ func parseGitURL(n string) *RepoSpec { repoSpec.KustRootPath = parsePath(n[index+len(gitDelimiter)+len(repoSpec.RepoPath):]) return repoSpec } - repoSpec.Host, n = parseHostSpec(n) + repoSpec.Host, n = extractHost(n) isLocal := strings.HasPrefix(repoSpec.Host, "file://") if !isLocal { repoSpec.GitSuffix = gitSuffix @@ -227,13 +227,13 @@ func parsePath(n string) string { return parsed.Path } -func parseHostSpec(n string) (string, string) { +func extractHost(n string) (string, string) { // We used to use go-getter to handle our urls: https://github.com/hashicorp/go-getter. // This prefix signaled go-getter to use the git protocol to fetch the url's contents. // We still accept this prefix. n, _ = trimPrefixIgnoreCase(n, "git::") - scheme, n := parseScheme(n) + scheme, n := extractScheme(n) if scheme == "file://" || scheme == "gh:" { // We support gh: assuming authors use it as a github shorthand, specified in .gitconfig. // The file protocol specifies an absolute path to a local git repo. There is no host. @@ -241,7 +241,7 @@ func parseHostSpec(n string) (string, string) { return scheme, n } - username, n := parseUsername(n) + username, n := extractUsername(n) if rest, isGithubURL := trimGithubHost(n); isGithubURL { // we strictly normalize Github URLs; this may discard scheme and username components in some cases. @@ -263,19 +263,16 @@ func validHostSpecParsed(scheme, username, host string) bool { (scheme != "" || isSCP) // scheme may be blank for URLs like git@github.com:org/repo } -func parseScheme(s string) (string, string) { - var scheme string +func extractScheme(s string) (string, string) { for _, prefix := range []string{"gh:", "ssh://", "https://", "http://", "file://"} { if rest, found := trimPrefixIgnoreCase(s, prefix); found { - scheme = prefix - s = rest - break + return prefix, rest } } - return scheme, s + return "", s } -func parseUsername(s string) (string, string) { +func extractUsername(s string) (string, string) { if trimmed, found := trimPrefixIgnoreCase(s, gitUsername); found { return gitUsername, trimmed } From 4821259f010768c382b6ba555ff457b4adf094d0 Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Fri, 6 Jan 2023 16:53:52 -0500 Subject: [PATCH 04/15] Stop handling mysterious gh: prefix in remote URLs --- api/internal/git/repospec.go | 6 ++---- api/internal/git/repospec_test.go | 6 ++++-- api/internal/localizer/util_test.go | 4 ---- 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/api/internal/git/repospec.go b/api/internal/git/repospec.go index c536b8c12d..748be77243 100644 --- a/api/internal/git/repospec.go +++ b/api/internal/git/repospec.go @@ -234,10 +234,8 @@ func extractHost(n string) (string, string) { n, _ = trimPrefixIgnoreCase(n, "git::") scheme, n := extractScheme(n) - if scheme == "file://" || scheme == "gh:" { - // We support gh: assuming authors use it as a github shorthand, specified in .gitconfig. + if scheme == "file://" { // The file protocol specifies an absolute path to a local git repo. There is no host. - // In both cases, we return the scheme and the rest of the url. return scheme, n } @@ -264,7 +262,7 @@ func validHostSpecParsed(scheme, username, host string) bool { } func extractScheme(s string) (string, string) { - for _, prefix := range []string{"gh:", "ssh://", "https://", "http://", "file://"} { + for _, prefix := range []string{"ssh://", "https://", "http://", "file://"} { if rest, found := trimPrefixIgnoreCase(s, prefix); found { return prefix, rest } diff --git a/api/internal/git/repospec_test.go b/api/internal/git/repospec_test.go index 603631758b..310ae32b1f 100644 --- a/api/internal/git/repospec_test.go +++ b/api/internal/git/repospec_test.go @@ -19,8 +19,6 @@ func TestNewRepoSpecFromUrl_Permute(t *testing.T) { // we probably stil don't want to break backwards compatibility for things // that are unintentionally supported. var schemeAuthority = []struct{ raw, normalized string }{ - {"gh:", "gh:"}, - {"GH:", "gh:"}, {"gitHub.com/", "https://github.com/"}, {"github.com:", "https://github.com/"}, {"http://github.com/", "https://github.com/"}, @@ -126,6 +124,10 @@ func TestNewRepoSpecFromUrlErrors(t *testing.T) { "git@https://github.com/org/repo.git//path", "url lacks host", }, + "mysterious gh: prefix previously supported is no longer handled": { + "gh:org/repo", + "url lacks orgRepo", + }, } for name, testCase := range badData { diff --git a/api/internal/localizer/util_test.go b/api/internal/localizer/util_test.go index ce70b4c01d..91fca64ce9 100644 --- a/api/internal/localizer/util_test.go +++ b/api/internal/localizer/util_test.go @@ -226,10 +226,6 @@ func TestLocRootPath_URLComponents(t *testing.T) { urlf: "file:///var/run/repo//%s?ref=value", path: simpleJoin(t, FileSchemeDir, "var", "run", "repo", "value"), }, - "gh_shorthand": { - urlf: "gh:org/repo//%s?ref=value", - path: simpleJoin(t, "gh", "org", "repo", "value"), - }, "IPv6": { urlf: "https://[2001:4860:4860::8888]/org/repo//%s?ref=value", path: simpleJoin(t, "2001:4860:4860::8888", "org", "repo", "value"), From 43d96ba91b4cbe8f9b42039bddb63a9d34211d70 Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Fri, 6 Jan 2023 18:00:14 -0500 Subject: [PATCH 05/15] Address feedback on correctness of SCP/username validations --- api/internal/git/repospec.go | 95 ++++++++++++++++++++++--------- api/internal/git/repospec_test.go | 40 +++++++++++-- 2 files changed, 101 insertions(+), 34 deletions(-) diff --git a/api/internal/git/repospec.go b/api/internal/git/repospec.go index 748be77243..7365bbbf39 100644 --- a/api/internal/git/repospec.go +++ b/api/internal/git/repospec.go @@ -228,37 +228,82 @@ func parsePath(n string) string { } func extractHost(n string) (string, string) { - // We used to use go-getter to handle our urls: https://github.com/hashicorp/go-getter. - // This prefix signaled go-getter to use the git protocol to fetch the url's contents. - // We still accept this prefix. - n, _ = trimPrefixIgnoreCase(n, "git::") - + n = ignoreForcedGitProtocol(n) scheme, n := extractScheme(n) - if scheme == "file://" { - // The file protocol specifies an absolute path to a local git repo. There is no host. - return scheme, n - } - username, n := extractUsername(n) + n, isGithubURL := trimGithubHost(n) - if rest, isGithubURL := trimGithubHost(n); isGithubURL { - // we strictly normalize Github URLs; this may discard scheme and username components in some cases. - return normalizedGithubHost(scheme, username), rest + // Validate the username and scheme before attempting host/path parsing, because if the parsing + // so far has not succeeded, we will not be able to extract the host and path correctly. + if err := validateUsernameAndScheme(username, scheme, isGithubURL); err != nil { + // TODO: return this error instead. + return "", n + } + + // Github URLS get special handling because they are strictly normalized in a way that + // may discard scheme and username components in some cases. + if isGithubURL { + return normalizedGithubHost(scheme, username), n } - sepIndex := findPathSeparator(n, username == gitUsername) + // Now that we have extracted a valid scheme+username, we can parse host itself. + + // The file protocol specifies an absolute path to a local git repo. There is no host. + if scheme == "file://" { + return scheme, n + } + sepIndex := findPathSeparator(n, acceptSCPStyle(scheme, username)) host, rest := n[:sepIndex+1], n[sepIndex+1:] - if !validHostSpecParsed(scheme, username, host) { + // Host is required, so do not concat the scheme and username if we didn't find one. + if host == "" { + // TODO: This should return an error. return "", n } return scheme + username + host, rest } -func validHostSpecParsed(scheme, username, host string) bool { - isSCP := username == gitUsername - return len(host) > 0 && - (scheme != "" || isSCP) // scheme may be blank for URLs like git@github.com:org/repo +// ignoreForcedGitProtocol strips the "git::" prefix from URLs. +// We used to use go-getter to handle our urls: https://github.com/hashicorp/go-getter. +// The git:: prefix signaled go-getter to use the git protocol to fetch the url's contents. +// We silently strip this prefix to allow these go-getter-style urls to continue to work, +// although the git protocol (which is insecure and unsupported on many platforms, including Github) +// will not actually be used as intended. +func ignoreForcedGitProtocol(n string) string { + n, _ = trimPrefixIgnoreCase(n, "git::") + return n +} + +// acceptSCPStyle returns true if the scheme and username indicate potential use of an SCP-style URL. +// With this style, the scheme is not explicit and the path is delimited by a colon. +// Strictly speaking the username is optional in SCP-like syntax, but Kustomize has always required it. +// Example: user@host.xz:path/to/repo.git/ +func acceptSCPStyle(scheme, username string) bool { + return username != "" && scheme == "" +} + +func validateUsernameAndScheme(username, scheme string, isGithubURL bool) error { + // see https://git-scm.com/docs/git-fetch#_git_urls for info relevant to these validations + switch scheme { + case "": + // Empty scheme is only ok if it's a Github URL or if it looks like SCP-style syntax + if !acceptSCPStyle(scheme, username) && !isGithubURL { + return fmt.Errorf("no username or scheme found") + } + case "ssh://": + // usernames are optional for ssh protocol + return nil + case "https://", "http://", "file://": + // usernames are not supported by http or file protocols + if username != "" { + return fmt.Errorf("username %q specified, but %s does not support usernames", username, scheme) + } + default: + // At time of writing, we should never end up here because we do not parse out + // unsupported schemes to begin with. + return fmt.Errorf("unsupported scheme %q", scheme) + } + return nil } func extractScheme(s string) (string, string) { @@ -295,16 +340,10 @@ func trimPrefixIgnoreCase(s, prefix string) (string, bool) { return s, false } -func findPathSeparator(hostPath string, isSCP bool) int { - if strings.Contains(hostPath, "://") { - // This function assumes that the hostPath does not contain a scheme. - // If it does, we return -1 to indicate that the hostPath is invalid. - return -1 - } - +func findPathSeparator(hostPath string, acceptSCP bool) int { sepIndex := strings.Index(hostPath, "/") - if isSCP { - // The colon acts as a delimiter for scp protocol only if not prefixed by '/'. + if acceptSCP { + // The colon acts as a delimiter in scp-style ssh URLs only if not prefixed by '/'. if colonIndex := strings.Index(hostPath, ":"); colonIndex > 0 && colonIndex < sepIndex { sepIndex = colonIndex } diff --git a/api/internal/git/repospec_test.go b/api/internal/git/repospec_test.go index 310ae32b1f..d95f4cf402 100644 --- a/api/internal/git/repospec_test.go +++ b/api/internal/git/repospec_test.go @@ -116,16 +116,20 @@ func TestNewRepoSpecFromUrlErrors(t *testing.T) { "github.com!org/repo.git//path", "url lacks host", }, - "unsupported protocol after username": { - "git@scp://github.com/org/repo.git//path", + "mysterious gh: prefix previously supported is no longer handled": { + "gh:org/repo", "url lacks host", }, - "supported protocol after username": { - "git@https://github.com/org/repo.git//path", + "username unsupported with http": { + "http://git@foo.com/path/to/repo", "url lacks host", }, - "mysterious gh: prefix previously supported is no longer handled": { - "gh:org/repo", + "username unsupported with https": { + "https://git@foo.com/path/to/repo", + "url lacks host", + }, + "username unsupported with file": { + "file://git@/path/to/repo", "url lacks orgRepo", }, } @@ -508,6 +512,30 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { GitSuffix: ".git", }, }, + { + name: "unsupported protocol after username (invalid and will be rejected by git)", + input: "git@scp://github.com/org/repo.git//path", + cloneSpec: "git@scp://github.com/org/repo.git", + absPath: notCloned.Join("path"), + repoSpec: RepoSpec{ + Host: "git@scp:/", + OrgRepo: "/github.com/org/repo", + Path: "/path", + GitSuffix: ".git", + }, + }, + { + name: "supported protocol after username (invalid and will be rejected by git)", + input: "git@ssh://github.com/org/repo.git//path", + cloneSpec: "git@ssh://github.com/org/repo.git", + absPath: notCloned.Join("path"), + repoSpec: RepoSpec{ + Host: "git@ssh:/", + OrgRepo: "/github.com/org/repo", + Path: "/path", + GitSuffix: ".git", + }, + }, } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { From 150134758fe59528c470d19d0b38dd1b29ddb3b1 Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Fri, 6 Jan 2023 18:11:06 -0500 Subject: [PATCH 06/15] Show change vs master on accepted but invalid ssh-like urls --- api/internal/git/repospec_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/api/internal/git/repospec_test.go b/api/internal/git/repospec_test.go index d95f4cf402..fbff557c8c 100644 --- a/api/internal/git/repospec_test.go +++ b/api/internal/git/repospec_test.go @@ -518,8 +518,8 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { cloneSpec: "git@scp://github.com/org/repo.git", absPath: notCloned.Join("path"), repoSpec: RepoSpec{ - Host: "git@scp:/", - OrgRepo: "/github.com/org/repo", + Host: "git@scp:", + OrgRepo: "//github.com/org/repo", Path: "/path", GitSuffix: ".git", }, @@ -530,8 +530,8 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { cloneSpec: "git@ssh://github.com/org/repo.git", absPath: notCloned.Join("path"), repoSpec: RepoSpec{ - Host: "git@ssh:/", - OrgRepo: "/github.com/org/repo", + Host: "git@ssh:", + OrgRepo: "//github.com/org/repo", Path: "/path", GitSuffix: ".git", }, From 2591303430468b5e0e71b13f5c98faf2162300bf Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Fri, 6 Jan 2023 19:25:35 -0500 Subject: [PATCH 07/15] Add test case from github docs --- api/internal/git/repospec_test.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/api/internal/git/repospec_test.go b/api/internal/git/repospec_test.go index fbff557c8c..282b078c67 100644 --- a/api/internal/git/repospec_test.go +++ b/api/internal/git/repospec_test.go @@ -118,7 +118,7 @@ func TestNewRepoSpecFromUrlErrors(t *testing.T) { }, "mysterious gh: prefix previously supported is no longer handled": { "gh:org/repo", - "url lacks host", + "url lacks orgRepo", }, "username unsupported with http": { "http://git@foo.com/path/to/repo", @@ -536,6 +536,18 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { GitSuffix: ".git", }, }, + { + name: "complex github ssh url from docs", + input: "ssh://git@ssh.github.com:443/YOUR-USERNAME/YOUR-REPOSITORY.git", + cloneSpec: "ssh://git@ssh.github.com:443/YOUR-USERNAME/YOUR-REPOSITORY.git", + absPath: notCloned.String(), + repoSpec: RepoSpec{ + Host: "ssh://git@ssh.github.com:443/", + OrgRepo: "YOUR-USERNAME/YOUR-REPOSITORY", + Path: "", + GitSuffix: ".git", + }, + }, } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { From 06999462e77dc9476aeebb4a642311557db38b9a Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Fri, 6 Jan 2023 21:52:04 -0500 Subject: [PATCH 08/15] Make Github less special --- api/internal/git/repospec.go | 48 +++++++++++++++++------------------- 1 file changed, 23 insertions(+), 25 deletions(-) diff --git a/api/internal/git/repospec.go b/api/internal/git/repospec.go index 7365bbbf39..1ac8ac6a7d 100644 --- a/api/internal/git/repospec.go +++ b/api/internal/git/repospec.go @@ -231,30 +231,30 @@ func extractHost(n string) (string, string) { n = ignoreForcedGitProtocol(n) scheme, n := extractScheme(n) username, n := extractUsername(n) - n, isGithubURL := trimGithubHost(n) + stdGithub := isStandardGithubHost(n) + acceptSCP := acceptSCPStyle(scheme, username, stdGithub) // Validate the username and scheme before attempting host/path parsing, because if the parsing // so far has not succeeded, we will not be able to extract the host and path correctly. - if err := validateUsernameAndScheme(username, scheme, isGithubURL); err != nil { + if err := validateUsernameAndScheme(username, scheme, acceptSCP); err != nil { // TODO: return this error instead. return "", n } - // Github URLS get special handling because they are strictly normalized in a way that - // may discard scheme and username components in some cases. - if isGithubURL { - return normalizedGithubHost(scheme, username), n - } - // Now that we have extracted a valid scheme+username, we can parse host itself. // The file protocol specifies an absolute path to a local git repo. There is no host. if scheme == "file://" { return scheme, n } - sepIndex := findPathSeparator(n, acceptSCPStyle(scheme, username)) + sepIndex := findPathSeparator(n, acceptSCP) host, rest := n[:sepIndex+1], n[sepIndex+1:] + // Github URLs are strictly normalized in a way that may discard scheme and username components. + if stdGithub { + scheme, username, host = normalizeGithubHostParts(scheme, username) + } + // Host is required, so do not concat the scheme and username if we didn't find one. if host == "" { // TODO: This should return an error. @@ -276,18 +276,19 @@ func ignoreForcedGitProtocol(n string) string { // acceptSCPStyle returns true if the scheme and username indicate potential use of an SCP-style URL. // With this style, the scheme is not explicit and the path is delimited by a colon. -// Strictly speaking the username is optional in SCP-like syntax, but Kustomize has always required it. +// Strictly speaking the username is optional in SCP-like syntax, but Kustomize has always +// required it for non-Github URLs. // Example: user@host.xz:path/to/repo.git/ -func acceptSCPStyle(scheme, username string) bool { - return username != "" && scheme == "" +func acceptSCPStyle(scheme, username string, isGithubURL bool) bool { + return scheme == "" && (username != "" || isGithubURL) } -func validateUsernameAndScheme(username, scheme string, isGithubURL bool) error { +func validateUsernameAndScheme(username, scheme string, acceptSCPStyle bool) error { // see https://git-scm.com/docs/git-fetch#_git_urls for info relevant to these validations switch scheme { case "": // Empty scheme is only ok if it's a Github URL or if it looks like SCP-style syntax - if !acceptSCPStyle(scheme, username) && !isGithubURL { + if !acceptSCPStyle { return fmt.Errorf("no username or scheme found") } case "ssh://": @@ -322,13 +323,10 @@ func extractUsername(s string) (string, string) { return "", s } -func trimGithubHost(s string) (string, bool) { - for _, prefix := range []string{"github.com/", "github.com:"} { - if trimmed, found := trimPrefixIgnoreCase(s, prefix); found { - return trimmed, true - } - } - return s, false +func isStandardGithubHost(s string) bool { + lowerCased := strings.ToLower(s) + return strings.HasPrefix(lowerCased, "github.com/") || + strings.HasPrefix(lowerCased, "github.com:") } // trimPrefixIgnoreCase returns the rest of s and true if prefix, ignoring case, prefixes s. @@ -355,11 +353,11 @@ const normalizedHTTPGithub = "https://github.com/" const gitUsername = "git@" const normalizedSCPGithub = gitUsername + "github.com:" -func normalizedGithubHost(scheme, username string) string { - if strings.HasPrefix(scheme, "ssh://") || username == gitUsername { - return normalizedSCPGithub +func normalizeGithubHostParts(scheme, username string) (string, string, string) { + if strings.HasPrefix(scheme, "ssh://") || username != "" { + return "", username, "github.com:" } - return normalizedHTTPGithub + return "https://", "", "github.com/" } func normalizeGitHostSpec(host string) string { From 64d2366e87f787893bdcd0346d87f5285544bf77 Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Mon, 9 Jan 2023 13:15:26 -0500 Subject: [PATCH 09/15] Naming changes from rebase --- api/internal/git/repospec_test.go | 62 +++++++++++++++---------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/api/internal/git/repospec_test.go b/api/internal/git/repospec_test.go index 282b078c67..6f79e98226 100644 --- a/api/internal/git/repospec_test.go +++ b/api/internal/git/repospec_test.go @@ -94,7 +94,7 @@ func TestNewRepoSpecFromUrlErrors(t *testing.T) { }, "bad_scp": { "git@local/path:file/system", - "url lacks orgRepo", + "url lacks repoPath", }, "no_org_repo": { "ssh://git.example.com", @@ -118,7 +118,7 @@ func TestNewRepoSpecFromUrlErrors(t *testing.T) { }, "mysterious gh: prefix previously supported is no longer handled": { "gh:org/repo", - "url lacks orgRepo", + "url lacks repoPath", }, "username unsupported with http": { "http://git@foo.com/path/to/repo", @@ -130,7 +130,7 @@ func TestNewRepoSpecFromUrlErrors(t *testing.T) { }, "username unsupported with file": { "file://git@/path/to/repo", - "url lacks orgRepo", + "url lacks repoPath", }, } @@ -223,7 +223,7 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { absPath: notCloned.String(), repoSpec: RepoSpec{ Host: "git@gitlab2.sqtools.ru:", - RepoPath: "infra/kubernetes/thanos-base", + RepoPath: "infra/kubernetes/thanos-base", Ref: "v0.1.0", GitSuffix: ".git", }, @@ -234,11 +234,11 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { cloneSpec: "git@bitbucket.org:company/project.git", absPath: notCloned.Join("path"), repoSpec: RepoSpec{ - Host: "git@bitbucket.org:", - RepoPath: "company/project", - KustRootPath: "/path", - Ref: "branch", - GitSuffix: ".git", + Host: "git@bitbucket.org:", + RepoPath: "company/project", + KustRootPath: "/path", + Ref: "branch", + GitSuffix: ".git", }, }, { @@ -247,11 +247,11 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { cloneSpec: "ssh://git@bitbucket.org/company/project.git", absPath: notCloned.Join("path"), repoSpec: RepoSpec{ - Host: "ssh://git@bitbucket.org/", - RepoPath: "company/project", - KustRootPath: "/path", - Ref: "branch", - GitSuffix: ".git", + Host: "ssh://git@bitbucket.org/", + RepoPath: "company/project", + KustRootPath: "/path", + Ref: "branch", + GitSuffix: ".git", }, }, { @@ -505,11 +505,11 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { cloneSpec: "ssh://myusername@bitbucket.org/ourteamname/ourrepositoryname.git", absPath: notCloned.Join("path"), repoSpec: RepoSpec{ - Host: "ssh://myusername@bitbucket.org/", - OrgRepo: "ourteamname/ourrepositoryname", - Path: "/path", - Ref: "branch", - GitSuffix: ".git", + Host: "ssh://myusername@bitbucket.org/", + RepoPath: "ourteamname/ourrepositoryname", + KustRootPath: "/path", + Ref: "branch", + GitSuffix: ".git", }, }, { @@ -518,10 +518,10 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { cloneSpec: "git@scp://github.com/org/repo.git", absPath: notCloned.Join("path"), repoSpec: RepoSpec{ - Host: "git@scp:", - OrgRepo: "//github.com/org/repo", - Path: "/path", - GitSuffix: ".git", + Host: "git@scp:", + RepoPath: "//github.com/org/repo", + KustRootPath: "/path", + GitSuffix: ".git", }, }, { @@ -530,10 +530,10 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { cloneSpec: "git@ssh://github.com/org/repo.git", absPath: notCloned.Join("path"), repoSpec: RepoSpec{ - Host: "git@ssh:", - OrgRepo: "//github.com/org/repo", - Path: "/path", - GitSuffix: ".git", + Host: "git@ssh:", + RepoPath: "//github.com/org/repo", + KustRootPath: "/path", + GitSuffix: ".git", }, }, { @@ -542,10 +542,10 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { cloneSpec: "ssh://git@ssh.github.com:443/YOUR-USERNAME/YOUR-REPOSITORY.git", absPath: notCloned.String(), repoSpec: RepoSpec{ - Host: "ssh://git@ssh.github.com:443/", - OrgRepo: "YOUR-USERNAME/YOUR-REPOSITORY", - Path: "", - GitSuffix: ".git", + Host: "ssh://git@ssh.github.com:443/", + RepoPath: "YOUR-USERNAME/YOUR-REPOSITORY", + KustRootPath: "", + GitSuffix: ".git", }, }, } From ddf14ea6881564bfb6278ed4315fe013222e3c88 Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Mon, 9 Jan 2023 13:18:11 -0500 Subject: [PATCH 10/15] Deprecate git:: stripping --- api/internal/git/repospec.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/api/internal/git/repospec.go b/api/internal/git/repospec.go index 1ac8ac6a7d..d08d842e80 100644 --- a/api/internal/git/repospec.go +++ b/api/internal/git/repospec.go @@ -5,6 +5,7 @@ package git import ( "fmt" + "log" "net/url" "path/filepath" "strconv" @@ -270,7 +271,12 @@ func extractHost(n string) (string, string) { // although the git protocol (which is insecure and unsupported on many platforms, including Github) // will not actually be used as intended. func ignoreForcedGitProtocol(n string) string { - n, _ = trimPrefixIgnoreCase(n, "git::") + n, found := trimPrefixIgnoreCase(n, "git::") + if found { + log.Println("Warning: Forcing the git protocol using the 'git::' URL prefix is not supported. " + + "Kustomize currently strips this invalid prefix, but will stop doing so in a future release. " + + "Please remove the 'git::' prefix from your configuration.") + } return n } From 240282fc6fb8dba73302e688cfdbb181ed1a01ab Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Mon, 9 Jan 2023 17:15:40 -0500 Subject: [PATCH 11/15] Allow file paths that look like usernames --- api/internal/git/repospec.go | 13 +++++++++---- api/internal/git/repospec_test.go | 17 +++++++++++++---- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/api/internal/git/repospec.go b/api/internal/git/repospec.go index d08d842e80..b82c5f7d81 100644 --- a/api/internal/git/repospec.go +++ b/api/internal/git/repospec.go @@ -244,9 +244,10 @@ func extractHost(n string) (string, string) { // Now that we have extracted a valid scheme+username, we can parse host itself. - // The file protocol specifies an absolute path to a local git repo. There is no host. + // The file protocol specifies an absolute path to a local git repo. + // Everything after the scheme (including any 'username' we found) is actually part of that path. if scheme == "file://" { - return scheme, n + return scheme, username + n } sepIndex := findPathSeparator(n, acceptSCP) host, rest := n[:sepIndex+1], n[sepIndex+1:] @@ -300,8 +301,12 @@ func validateUsernameAndScheme(username, scheme string, acceptSCPStyle bool) err case "ssh://": // usernames are optional for ssh protocol return nil - case "https://", "http://", "file://": - // usernames are not supported by http or file protocols + case "file://": + // everything following the scheme in the file protocol is a path on the local filesystem, + // which may contain arbitrary characters (theoretically including `@`, which we'd mistake for a username) + return nil + case "https://", "http://": + // usernames are not supported by the http protocol if username != "" { return fmt.Errorf("username %q specified, but %s does not support usernames", username, scheme) } diff --git a/api/internal/git/repospec_test.go b/api/internal/git/repospec_test.go index 6f79e98226..a3f01e88c7 100644 --- a/api/internal/git/repospec_test.go +++ b/api/internal/git/repospec_test.go @@ -128,10 +128,6 @@ func TestNewRepoSpecFromUrlErrors(t *testing.T) { "https://git@foo.com/path/to/repo", "url lacks host", }, - "username unsupported with file": { - "file://git@/path/to/repo", - "url lacks repoPath", - }, } for name, testCase := range badData { @@ -512,6 +508,19 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { GitSuffix: ".git", }, }, + { + name: "username-like filepath with file protocol", + input: "file://git@home/path/to/repository.git//path?ref=branch", + cloneSpec: "file://git@home/path/to/repository.git", + absPath: notCloned.Join("path"), + repoSpec: RepoSpec{ + Host: "file://", + RepoPath: "git@home/path/to/repository", + KustRootPath: "/path", + Ref: "branch", + GitSuffix: ".git", + }, + }, { name: "unsupported protocol after username (invalid and will be rejected by git)", input: "git@scp://github.com/org/repo.git//path", From 1a201ab913b7ab0e914e832bc7374eaf951f27f3 Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Mon, 9 Jan 2023 17:24:40 -0500 Subject: [PATCH 12/15] do not validate against http+usernames after all --- api/internal/git/repospec.go | 14 ++---------- api/internal/git/repospec_test.go | 38 +++++++++++++++++++++++-------- 2 files changed, 30 insertions(+), 22 deletions(-) diff --git a/api/internal/git/repospec.go b/api/internal/git/repospec.go index b82c5f7d81..8b3ba499aa 100644 --- a/api/internal/git/repospec.go +++ b/api/internal/git/repospec.go @@ -298,18 +298,8 @@ func validateUsernameAndScheme(username, scheme string, acceptSCPStyle bool) err if !acceptSCPStyle { return fmt.Errorf("no username or scheme found") } - case "ssh://": - // usernames are optional for ssh protocol - return nil - case "file://": - // everything following the scheme in the file protocol is a path on the local filesystem, - // which may contain arbitrary characters (theoretically including `@`, which we'd mistake for a username) - return nil - case "https://", "http://": - // usernames are not supported by the http protocol - if username != "" { - return fmt.Errorf("username %q specified, but %s does not support usernames", username, scheme) - } + case "ssh://", "file://", "https://", "http://": + // These are all supported schemes default: // At time of writing, we should never end up here because we do not parse out // unsupported schemes to begin with. diff --git a/api/internal/git/repospec_test.go b/api/internal/git/repospec_test.go index a3f01e88c7..fb8ed0f752 100644 --- a/api/internal/git/repospec_test.go +++ b/api/internal/git/repospec_test.go @@ -120,14 +120,6 @@ func TestNewRepoSpecFromUrlErrors(t *testing.T) { "gh:org/repo", "url lacks repoPath", }, - "username unsupported with http": { - "http://git@foo.com/path/to/repo", - "url lacks host", - }, - "username unsupported with https": { - "https://git@foo.com/path/to/repo", - "url lacks host", - }, } for name, testCase := range badData { @@ -522,7 +514,33 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { }, }, { - name: "unsupported protocol after username (invalid and will be rejected by git)", + name: "username with http protocol (invalid but currently passed through to git)", + input: "http://git@home.com/path/to/repository.git//path?ref=branch", + cloneSpec: "http://git@home.com/path/to/repository.git", + absPath: notCloned.Join("path"), + repoSpec: RepoSpec{ + Host: "http://git@home.com/", + RepoPath: "path/to/repository", + KustRootPath: "/path", + Ref: "branch", + GitSuffix: ".git", + }, + }, + { + name: "username with https protocol (invalid but currently passed through to git)", + input: "https://git@home.com/path/to/repository.git//path?ref=branch", + cloneSpec: "https://git@home.com/path/to/repository.git", + absPath: notCloned.Join("path"), + repoSpec: RepoSpec{ + Host: "https://git@home.com/", + RepoPath: "path/to/repository", + KustRootPath: "/path", + Ref: "branch", + GitSuffix: ".git", + }, + }, + { + name: "unsupported protocol after username (invalid but currently passed through to git)", input: "git@scp://github.com/org/repo.git//path", cloneSpec: "git@scp://github.com/org/repo.git", absPath: notCloned.Join("path"), @@ -534,7 +552,7 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { }, }, { - name: "supported protocol after username (invalid and will be rejected by git)", + name: "supported protocol after username (invalid but currently passed through to git)", input: "git@ssh://github.com/org/repo.git//path", cloneSpec: "git@ssh://github.com/org/repo.git", absPath: notCloned.Join("path"), From a885ee12c614df4540f610b662ef9490bca54c7c Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Mon, 9 Jan 2023 17:32:14 -0500 Subject: [PATCH 13/15] Add test for behaviour of scp-like non-github that incorrectly uses slash --- api/internal/git/repospec_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/api/internal/git/repospec_test.go b/api/internal/git/repospec_test.go index fb8ed0f752..6d8c6568c4 100644 --- a/api/internal/git/repospec_test.go +++ b/api/internal/git/repospec_test.go @@ -229,6 +229,19 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { GitSuffix: ".git", }, }, + { + name: "non-github_scp incorrectly using slash (invalid but currently passed through to git)", + input: "git@bitbucket.org/company/project.git//path?ref=branch", + cloneSpec: "git@bitbucket.org/company/project.git", + absPath: notCloned.Join("path"), + repoSpec: RepoSpec{ + Host: "git@bitbucket.org/", + RepoPath: "company/project", + KustRootPath: "/path", + Ref: "branch", + GitSuffix: ".git", + }, + }, { name: "non-github_git-user_ssh", input: "ssh://git@bitbucket.org/company/project.git//path?ref=branch", From c2885642d6b726b9f4197bd1f6984a58d7257fe1 Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Tue, 10 Jan 2023 15:01:29 -0500 Subject: [PATCH 14/15] Remove unused param --- api/internal/git/repospec.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/internal/git/repospec.go b/api/internal/git/repospec.go index 8b3ba499aa..8f9c247776 100644 --- a/api/internal/git/repospec.go +++ b/api/internal/git/repospec.go @@ -237,7 +237,7 @@ func extractHost(n string) (string, string) { // Validate the username and scheme before attempting host/path parsing, because if the parsing // so far has not succeeded, we will not be able to extract the host and path correctly. - if err := validateUsernameAndScheme(username, scheme, acceptSCP); err != nil { + if err := validateScheme(scheme, acceptSCP); err != nil { // TODO: return this error instead. return "", n } @@ -290,13 +290,13 @@ func acceptSCPStyle(scheme, username string, isGithubURL bool) bool { return scheme == "" && (username != "" || isGithubURL) } -func validateUsernameAndScheme(username, scheme string, acceptSCPStyle bool) error { +func validateScheme(scheme string, acceptSCPStyle bool) error { // see https://git-scm.com/docs/git-fetch#_git_urls for info relevant to these validations switch scheme { case "": // Empty scheme is only ok if it's a Github URL or if it looks like SCP-style syntax if !acceptSCPStyle { - return fmt.Errorf("no username or scheme found") + return fmt.Errorf("failed to parse scheme") } case "ssh://", "file://", "https://", "http://": // These are all supported schemes From 3134e9b0c28cf0809395131a95957287ba008ad5 Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Tue, 10 Jan 2023 16:14:54 -0500 Subject: [PATCH 15/15] Better SCP colon-must-be-after-slash test --- api/internal/git/repospec_test.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/api/internal/git/repospec_test.go b/api/internal/git/repospec_test.go index 6d8c6568c4..84a878164a 100644 --- a/api/internal/git/repospec_test.go +++ b/api/internal/git/repospec_test.go @@ -92,10 +92,6 @@ func TestNewRepoSpecFromUrlErrors(t *testing.T) { "htxxxtp://github.com/", "url lacks host", }, - "bad_scp": { - "git@local/path:file/system", - "url lacks repoPath", - }, "no_org_repo": { "ssh://git.example.com", "url lacks repoPath", @@ -588,6 +584,18 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { GitSuffix: ".git", }, }, + { + name: "colon behind slash not scp delimiter", + input: "git@gitlab.com/user:name/YOUR-REPOSITORY.git/path", + cloneSpec: "git@gitlab.com/user:name/YOUR-REPOSITORY.git", + absPath: notCloned.Join("path"), + repoSpec: RepoSpec{ + Host: "git@gitlab.com/", + RepoPath: "user:name/YOUR-REPOSITORY", + KustRootPath: "path", + GitSuffix: ".git", + }, + }, } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) {