From 05fe86fd76e0d84b28fbae57ced4e3ef1cb0553a Mon Sep 17 00:00:00 2001 From: Alassane <79718216+yattdev@users.noreply.github.com> Date: Fri, 21 Aug 2026 05:34:19 -0400 Subject: [PATCH] fix(gitcredentials): reject unsafe clone authorities (#2869) Co-authored-by: ayattara Co-authored-by: Carlos Florencio --- .../internal/gitcredentials/identity.go | 60 +++++++++++ .../gitcredentials/identity_hostile_test.go | 99 +++++++++++++++++++ apps/backend/internal/repoclone/origin.go | 57 ++++++++++- .../backend/internal/repoclone/origin_test.go | 51 ++++++++++ .../integrations/github-authentication.md | 5 +- 5 files changed, 269 insertions(+), 3 deletions(-) create mode 100644 apps/backend/internal/gitcredentials/identity_hostile_test.go diff --git a/apps/backend/internal/gitcredentials/identity.go b/apps/backend/internal/gitcredentials/identity.go index 140d5d754e..25886b8f98 100644 --- a/apps/backend/internal/gitcredentials/identity.go +++ b/apps/backend/internal/gitcredentials/identity.go @@ -97,6 +97,9 @@ type parsedRemote struct { func parseRemote(raw string) (parsedRemote, error) { value := strings.TrimSpace(raw) + if !hasSafeRemoteAuthority(value) { + return parsedRemote{}, fmt.Errorf("invalid clone URL authority") + } canonical := repoclone.CanonicalHTTPSCloneURL(value) if canonical != "" { if err := validateSSHRemote(value); err != nil { @@ -138,6 +141,9 @@ func validateSSHRemote(value string) error { func trustedOrigin(input RepositoryIdentityInput, remoteHost string) (string, error) { if host := strings.TrimSpace(input.ProviderHost); host != "" { + if !hasSafeProviderHostAuthority(host) { + return "", repositoryIdentityError(input.RepositoryID, "has an invalid provider host") + } origin, err := repoclone.HTTPSProviderOrigin(host) if err != nil { return "", repositoryIdentityError(input.RepositoryID, "has an invalid provider host") @@ -168,3 +174,57 @@ func repositoryIdentityError(repositoryID, detail string) error { } return fmt.Errorf("repository %q %s; repair its clone URL or provider host", repositoryID, detail) } + +func hasSafeRemoteAuthority(value string) bool { + authority, found := rawRemoteAuthority(value) + return found && repoclone.IsSafeRawAuthority(authority) +} + +func rawRemoteAuthority(value string) (string, bool) { + if schemeEnd := strings.Index(value, "://"); schemeEnd >= 0 && isURLScheme(value[:schemeEnd]) { + return authorityBeforePath(value[schemeEnd+len("://"):]) + } + user, hostAndPath, found := strings.Cut(value, "@") + if !found || user == "" { + return "", false + } + host, _, found := strings.Cut(hostAndPath, ":") + if !found || host == "" { + return "", false + } + return user + "@" + host, true +} + +func hasSafeProviderHostAuthority(value string) bool { + if schemeEnd := strings.Index(value, "://"); schemeEnd >= 0 { + if !isURLScheme(value[:schemeEnd]) { + return false + } + value = value[schemeEnd+len("://"):] + } + authority, found := authorityBeforePath(value) + return found && repoclone.IsSafeRawAuthority(authority) +} + +func authorityBeforePath(value string) (string, bool) { + if authorityEnd := strings.IndexAny(value, "/?#"); authorityEnd >= 0 { + value = value[:authorityEnd] + } + return value, value != "" +} + +func isURLScheme(value string) bool { + if value == "" || !isASCIILetter(value[0]) { + return false + } + for i := 1; i < len(value); i++ { + if !isASCIILetter(value[i]) && (value[i] < '0' || value[i] > '9') && value[i] != '+' && value[i] != '-' && value[i] != '.' { + return false + } + } + return true +} + +func isASCIILetter(value byte) bool { + return value >= 'a' && value <= 'z' || value >= 'A' && value <= 'Z' +} diff --git a/apps/backend/internal/gitcredentials/identity_hostile_test.go b/apps/backend/internal/gitcredentials/identity_hostile_test.go new file mode 100644 index 0000000000..f44c9f1cd7 --- /dev/null +++ b/apps/backend/internal/gitcredentials/identity_hostile_test.go @@ -0,0 +1,99 @@ +package gitcredentials + +import ( + "strings" + "testing" +) + +func TestResolveRepositoryIdentityRejectsUnsafeRemoteAuthorities(t *testing.T) { + t.Parallel() + remotes := []string{ + "git@gİthub.com:acme/widgets.git", + "ssh://git@gİthub.com/acme/widgets.git", + "https://gİthub.com/acme/widgets.git", + "git@g%C4%B0thub.com:acme/widgets.git", + "ssh://git@g%C4%B0thub.com/acme/widgets.git", + "https://g%C4%B0thub.com/acme/widgets.git", + "git@g%25C4%25B0thub.com:acme/widgets.git", + "ssh://git@g%25C4%25B0thub.com/acme/widgets.git", + "https://g%25C4%25B0thub.com/acme/widgets.git", + } + for _, providerHost := range []string{"", "https://github.com"} { + for _, remoteURL := range remotes { + t.Run(providerHost+"/"+remoteURL, func(t *testing.T) { + t.Parallel() + _, err := ResolveRepositoryIdentity(RepositoryIdentityInput{ + RepositoryID: "repo-1", ProviderHost: providerHost, RemoteURL: remoteURL, + }) + if err == nil { + t.Fatal("ResolveRepositoryIdentity() error = nil, want rejection") + } + if strings.Contains(err.Error(), remoteURL) || !strings.Contains(err.Error(), "repair its clone URL or provider host") { + t.Fatalf("error = %q, want generic remediation without hostile remote", err) + } + }) + } + } +} + +func TestParseRemoteRejectsUnsafeHTTPSAuthorities(t *testing.T) { + t.Parallel() + for _, remoteURL := range []string{ + "https://gİthub.com/acme/widgets.git", + "https://g%C4%B0thub.com/acme/widgets.git", + "https://g%25C4%25B0thub.com/acme/widgets.git", + } { + t.Run(remoteURL, func(t *testing.T) { + t.Parallel() + if _, err := parseRemote(remoteURL); err == nil { + t.Fatalf("parseRemote(%q) error = nil, want rejection", remoteURL) + } + }) + } +} + +func TestResolveRepositoryIdentityRejectsUnsafeProviderHost(t *testing.T) { + t.Parallel() + for _, providerHost := range []string{ + "gİthub.com", + "g%C4%B0thub.com", + "g%25C4%25B0thub.com", + } { + t.Run(providerHost, func(t *testing.T) { + t.Parallel() + _, err := ResolveRepositoryIdentity(RepositoryIdentityInput{ + RepositoryID: "repo-1", ProviderHost: providerHost, + RemoteURL: "https://github.com/acme/widgets.git", + }) + if err == nil { + t.Fatal("ResolveRepositoryIdentity() error = nil, want rejection") + } + if strings.Contains(err.Error(), providerHost) || !strings.Contains(err.Error(), "repair its clone URL or provider host") { + t.Fatalf("error = %q, want generic remediation without hostile provider host", err) + } + }) + } +} + +func TestResolveRepositoryIdentityPreservesUnicodeAndEncodedPaths(t *testing.T) { + t.Parallel() + for _, remoteURL := range []string{ + "git@github.com:acme/wídgets.git", + "ssh://git@github.com/acme/wídgets.git", + "https://github.com/acme/wídgets.git", + "git@github.com:acme/w%C3%ADdgets.git", + "ssh://git@github.com/acme/w%C3%ADdgets.git", + "https://github.com/acme/w%C3%ADdgets.git", + } { + t.Run(remoteURL, func(t *testing.T) { + t.Parallel() + identity, err := ResolveRepositoryIdentity(RepositoryIdentityInput{RepositoryID: "repo-1", RemoteURL: remoteURL}) + if err != nil { + t.Fatalf("ResolveRepositoryIdentity() error = %v", err) + } + if identity.Path != "/acme/wídgets.git" { + t.Fatalf("identity.Path = %q, want %q", identity.Path, "/acme/wídgets.git") + } + }) + } +} diff --git a/apps/backend/internal/repoclone/origin.go b/apps/backend/internal/repoclone/origin.go index b6a2a32c23..51829e7dc7 100644 --- a/apps/backend/internal/repoclone/origin.go +++ b/apps/backend/internal/repoclone/origin.go @@ -17,6 +17,9 @@ func HTTPSProviderOrigin(providerHost string) (string, error) { if !strings.Contains(value, "://") { value = ProtocolHTTPS + "://" + value } + if !hasSafeRawAuthority(value) { + return "", fmt.Errorf("provider host must be a credential-free HTTPS URL") + } parsed, err := url.Parse(value) if err != nil || !strings.EqualFold(parsed.Scheme, ProtocolHTTPS) || parsed.Host == "" || parsed.User != nil || parsed.RawQuery != "" || parsed.Fragment != "" { @@ -41,6 +44,9 @@ func CanonicalHTTPSCloneURL(raw string) string { if !strings.Contains(value, "://") { return scpStyleHTTPSCloneURL(value) } + if !hasSafeRawAuthority(value) { + return "" + } parsed, err := url.Parse(value) if err != nil || !strings.EqualFold(parsed.Scheme, ProtocolSSH) || parsed.Hostname() == "" || parsed.Path == "" || parsed.Path == "/" { @@ -60,6 +66,9 @@ func scpStyleHTTPSCloneURL(value string) string { if !found || host == "" || strings.Contains(host, "/") { return "" } + if !IsSafeRawAuthority(user + "@" + host) { + return "" + } path = strings.TrimPrefix(path, "/") if path == "" { return "" @@ -71,7 +80,11 @@ func scpStyleHTTPSCloneURL(value string) string { // and requires their origin to match the provider identity that authorized // credential use. func ValidateHTTPSCloneOrigin(cloneURL, providerHost string) error { - parsed, err := url.Parse(strings.TrimSpace(cloneURL)) + value := strings.TrimSpace(cloneURL) + if !hasSafeRawAuthority(value) { + return fmt.Errorf("clone URL must be a credential-free HTTPS URL") + } + parsed, err := url.Parse(value) if err != nil || !strings.EqualFold(parsed.Scheme, ProtocolHTTPS) || parsed.Host == "" || parsed.User != nil || parsed.Path == "" || parsed.RawQuery != "" || parsed.Fragment != "" { return fmt.Errorf("clone URL must be a credential-free HTTPS URL") @@ -86,3 +99,45 @@ func ValidateHTTPSCloneOrigin(cloneURL, providerHost string) error { } return nil } + +func hasSafeRawAuthority(value string) bool { + authority, found := rawAuthority(value) + return found && IsSafeRawAuthority(authority) +} + +func rawAuthority(value string) (string, bool) { + if schemeEnd := strings.Index(value, "://"); schemeEnd >= 0 && isURLScheme(value[:schemeEnd]) { + value = value[schemeEnd+len("://"):] + } + if authorityEnd := strings.IndexAny(value, "/?#"); authorityEnd >= 0 { + value = value[:authorityEnd] + } + return value, value != "" +} + +// IsSafeRawAuthority reports whether a raw authority stays ASCII-only and +// percent-free before any URL parsing or case folding can reinterpret it. +func IsSafeRawAuthority(authority string) bool { + for i := 0; i < len(authority); i++ { + if authority[i] > 0x7f || authority[i] == '%' { + return false + } + } + return true +} + +func isURLScheme(value string) bool { + if value == "" || !isASCIILetter(value[0]) { + return false + } + for i := 1; i < len(value); i++ { + if !isASCIILetter(value[i]) && (value[i] < '0' || value[i] > '9') && value[i] != '+' && value[i] != '-' && value[i] != '.' { + return false + } + } + return true +} + +func isASCIILetter(value byte) bool { + return value >= 'a' && value <= 'z' || value >= 'A' && value <= 'Z' +} diff --git a/apps/backend/internal/repoclone/origin_test.go b/apps/backend/internal/repoclone/origin_test.go index 37e68fc546..4f53ddd5ac 100644 --- a/apps/backend/internal/repoclone/origin_test.go +++ b/apps/backend/internal/repoclone/origin_test.go @@ -16,6 +16,14 @@ func TestCanonicalHTTPSCloneURL(t *testing.T) { {name: "ssh scheme", raw: "ssh://git@github.com/acme/widgets.git", want: "https://github.com/acme/widgets.git"}, {name: "ssh scheme with port", raw: "ssh://git@gitlab.example:2222/group/widgets.git", want: "https://gitlab.example/group/widgets.git"}, {name: "ssh scheme drops password", raw: "ssh://git:secret@github.com/acme/widgets.git", want: "https://github.com/acme/widgets.git"}, + {name: "scp style rejects Unicode authority", raw: "git@gİthub.com:acme/widgets.git"}, + {name: "ssh scheme rejects Unicode authority", raw: "ssh://git@gİthub.com/acme/widgets.git"}, + {name: "scp style rejects encoded authority", raw: "git@g%C4%B0thub.com:acme/widgets.git"}, + {name: "ssh scheme rejects encoded authority", raw: "ssh://git@g%C4%B0thub.com/acme/widgets.git"}, + {name: "scp style rejects double encoded authority", raw: "git@g%25C4%25B0thub.com:acme/widgets.git"}, + {name: "ssh scheme rejects double encoded authority", raw: "ssh://git@g%25C4%25B0thub.com/acme/widgets.git"}, + {name: "scp style preserves Unicode path", raw: "git@github.com:acme/wídgets.git", want: "https://github.com/acme/wídgets.git"}, + {name: "ssh scheme preserves Unicode path", raw: "ssh://git@github.com/acme/wídgets.git", want: "https://github.com/acme/wídgets.git"}, {name: "https passes through untouched", raw: "https://github.com/acme/widgets.git"}, {name: "file remote", raw: "file:///tmp/widgets.git"}, {name: "local path", raw: "/home/user/widgets"}, @@ -35,6 +43,25 @@ func TestCanonicalHTTPSCloneURL(t *testing.T) { } } +func TestHTTPSProviderOriginRejectsUnsafeAuthority(t *testing.T) { + t.Parallel() + for _, providerHost := range []string{ + "gİthub.com", + "g%C4%B0thub.com", + "g%25C4%25B0thub.com", + "https://gİthub.com", + "https://g%C4%B0thub.com", + "https://g%25C4%25B0thub.com", + } { + t.Run(providerHost, func(t *testing.T) { + t.Parallel() + if _, err := HTTPSProviderOrigin(providerHost); err == nil { + t.Fatalf("HTTPSProviderOrigin(%q) error = nil, want rejection", providerHost) + } + }) + } +} + func TestValidateHTTPSCloneOrigin(t *testing.T) { t.Parallel() tests := []struct { @@ -77,6 +104,30 @@ func TestValidateHTTPSCloneOrigin(t *testing.T) { providerHost: "https://bitbucket.example.test", wantErr: true, }, + { + name: "Unicode clone authority", + cloneURL: "https://gİthub.com/acme/widgets.git", + providerHost: "https://github.com", + wantErr: true, + }, + { + name: "encoded clone authority", + cloneURL: "https://g%C4%B0thub.com/acme/widgets.git", + providerHost: "https://github.com", + wantErr: true, + }, + { + name: "unsafe provider authority", + cloneURL: "https://github.com/acme/widgets.git", + providerHost: "https://gİthub.com", + wantErr: true, + }, + { + name: "bare Unicode provider authority", + cloneURL: "https://github.com/acme/widgets.git", + providerHost: "gİthub.com", + wantErr: true, + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { diff --git a/docs/specs/integrations/github-authentication.md b/docs/specs/integrations/github-authentication.md index e9f8206347..b57265fc3a 100644 --- a/docs/specs/integrations/github-authentication.md +++ b/docs/specs/integrations/github-authentication.md @@ -435,8 +435,9 @@ post-signature processing failures produce `failing`; a later valid successful d - Public base URL validation requires a canonical HTTPS origin with no credentials, query, or fragment and rejects loopback, private, link-local, or non-globally-routable DNS results. Kandev does not fetch the supplied URL as validation. -- Repository identity accepts credential-free HTTPS and SSH remote forms. It rejects an SSH - password, query, or fragment before canonicalization, and an error never contains the rejected +- Repository identity accepts credential-free HTTPS and SSH remote forms. Authorities must remain + ASCII-only and percent-free before URL parsing or case folding. It rejects an SSH password, + query, or fragment before canonicalization, and an error never contains the rejected secret-bearing remote. - App private keys, client secrets, webhook secrets, personal tokens, and live installation tokens never enter executor environments. Only brokered PAT/CLI tokens or repository-restricted