From 6ccd0fcd0c098abf75d6b0271b2d37d3684ec999 Mon Sep 17 00:00:00 2001
From: Andrii Korotkov <andrii.korotkov@verkada.com>
Date: Wed, 27 Nov 2024 06:45:07 -0800
Subject: [PATCH] fix: Allow to delete repos with invalid urls (#20921)

Fixes #20921

Normalization of repo url is attempted during repo deletion before comparison with existing repos. Before this change, it'd fail on invalid urls and return "", resulting in permission denied. This ended up in a state where people can add repos with invalid urls but couldn't delete them. Return raw repo url if url parsing failed. Add a test to validate the deletion can be performed and make sure it fails without the main change.

This needs to be cherry-picked to v2.11-2.13

Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com>
---
 server/repository/repository_test.go | 33 ++++++++++++++++++++++++++++
 util/git/git.go                      | 20 ++++++++++++++---
 2 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/server/repository/repository_test.go b/server/repository/repository_test.go
index f4d80c3c434d8..91c2e016ab3d4 100644
--- a/server/repository/repository_test.go
+++ b/server/repository/repository_test.go
@@ -21,6 +21,7 @@ import (
 
 	"github.com/argoproj/argo-cd/v2/common"
 	"github.com/argoproj/argo-cd/v2/pkg/apiclient/repository"
+	repositorypkg "github.com/argoproj/argo-cd/v2/pkg/apiclient/repository"
 	appsv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
 	fakeapps "github.com/argoproj/argo-cd/v2/pkg/client/clientset/versioned/fake"
 	appinformer "github.com/argoproj/argo-cd/v2/pkg/client/informers/externalversions"
@@ -1119,3 +1120,35 @@ func TestGetRepository(t *testing.T) {
 		})
 	}
 }
+
+func TestDeleteRepository(t *testing.T) {
+	repositories := map[string]string{
+		"valid": "https://bitbucket.org/workspace/repo.git",
+		// Check a wrongly formatter repo as well, see https://github.com/argoproj/argo-cd/issues/20921
+		"invalid": "git clone https://bitbucket.org/workspace/repo.git",
+	}
+
+	kubeclientset := fake.NewSimpleClientset(&argocdCM, &argocdSecret)
+	settingsMgr := settings.NewSettingsManager(context.Background(), kubeclientset, testNamespace)
+
+	for name, repo := range repositories {
+		t.Run(name, func(t *testing.T) {
+			repoServerClient := mocks.RepoServerServiceClient{}
+			repoServerClient.On("TestRepository", mock.Anything, mock.Anything).Return(&apiclient.TestRepositoryResponse{}, nil)
+
+			repoServerClientset := mocks.Clientset{RepoServerServiceClient: &repoServerClient}
+			enforcer := newEnforcer(kubeclientset)
+
+			db := &dbmocks.ArgoDB{}
+			db.On("DeleteRepository", context.TODO(), repo, "default").Return(nil)
+			db.On("ListRepositories", context.TODO()).Return([]*appsv1.Repository{{Repo: repo, Project: "default"}}, nil)
+			db.On("GetRepository", context.TODO(), repo, "default").Return(&appsv1.Repository{Repo: repo, Project: "default"}, nil)
+			appLister, projLister := newAppAndProjLister(defaultProj)
+
+			s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, testNamespace, settingsMgr)
+			resp, err := s.DeleteRepository(context.TODO(), &repository.RepoQuery{Repo: repo, AppProject: "default"})
+			require.NoError(t, err)
+			assert.Equal(t, repositorypkg.RepoResponse{}, *resp)
+		})
+	}
+}
diff --git a/util/git/git.go b/util/git/git.go
index 3a087aeb00096..6ef47fecfb172 100644
--- a/util/git/git.go
+++ b/util/git/git.go
@@ -35,8 +35,8 @@ func IsTruncatedCommitSHA(sha string) bool {
 
 // SameURL returns whether or not the two repository URLs are equivalent in location
 func SameURL(leftRepo, rightRepo string) bool {
-	normalLeft := NormalizeGitURL(leftRepo)
-	normalRight := NormalizeGitURL(rightRepo)
+	normalLeft := NormalizeGitURLAllowInvalid(leftRepo)
+	normalRight := NormalizeGitURLAllowInvalid(rightRepo)
 	return normalLeft != "" && normalRight != "" && normalLeft == normalRight
 }
 
@@ -45,6 +45,16 @@ func SameURL(leftRepo, rightRepo string) bool {
 // Prefer using SameURL() over this function when possible. This algorithm may change over time
 // and should not be considered stable from release to release
 func NormalizeGitURL(repo string) string {
+	return normalizeGitURL(repo, false)
+}
+
+// Similar to NormalizeGitURL, except returning a not fully normalized string when the url is invalid.
+// Needed to allow a deletion of repos with invalid urls. See https://github.com/argoproj/argo-cd/issues/20921.
+func NormalizeGitURLAllowInvalid(repo string) string {
+	return normalizeGitURL(repo, true)
+}
+
+func normalizeGitURL(repo string, allowInvalid bool) string {
 	repo = strings.ToLower(strings.TrimSpace(repo))
 	if yes, _ := IsSSHURL(repo); yes {
 		if !strings.HasPrefix(repo, "ssh://") {
@@ -57,7 +67,11 @@ func NormalizeGitURL(repo string) string {
 	repo = strings.TrimSuffix(repo, ".git")
 	repoURL, err := url.Parse(repo)
 	if err != nil {
-		return ""
+		if allowInvalid {
+			return repo
+		} else {
+			return ""
+		}
 	}
 	normalized := repoURL.String()
 	return strings.TrimPrefix(normalized, "ssh://")