Adding .git suffix when clone from URL failsadd-git-suffix-when-cloning-from-url

Due to a security fix, git command is no longer following redirects
which can cause failed imports when the '.git' suffix is missing from
the url. This change appends the '.git' suffix if the clone command
fails and tries cloning the repository again.

4 files changed, 72 insertions, 16 deletions

diff --git a/changelogs/unreleased/add-git-suffix-when-cloning-from-url.yml b/changelogs/unreleased/add-git-suffix-when-cloning-from-url.yml
new file mode 100644
index 000000000..b36ada7ff
--- /dev/null
+++ b/changelogs/unreleased/add-git-suffix-when-cloning-from-url.yml
@@ -0,0 +1,5 @@
+---
+title: Try adding .git suffix to the URL when cloning from remote fails
+merge_request: 1467
+author:
+type: changed
diff --git a/internal/service/repository/create_from_url.go b/internal/service/repository/create_from_url.go
index 7bfa59a2f..4b149275d 100644
--- a/internal/service/repository/create_from_url.go
+++ b/internal/service/repository/create_from_url.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"strings"
 
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
@@ -28,22 +29,14 @@ func (s *server) CreateRepositoryFromURL(ctx context.Context, req *gitalypb.Crea
 		return nil, status.Errorf(codes.InvalidArgument, "CreateRepositoryFromURL: dest dir exists")
 	}
 
-	args := []string{
-		"-c",
-		"http.followRedirects=false",
-		"clone",
-		"--bare",
-		"--",
-		req.Url,
-		repositoryFullPath,
-	}
-	cmd, err := git.CommandWithoutRepo(ctx, args...)
-	if err != nil {
-		return nil, status.Errorf(codes.Internal, "CreateRepositoryFromURL: clone cmd start: %v", err)
-	}
-	if err := cmd.Wait(); err != nil {
-		os.RemoveAll(repositoryFullPath)
-		return nil, status.Errorf(codes.Internal, "CreateRepositoryFromURL: clone cmd wait: %v", err)
+	if _, err = cloneRepositoryFromURL(ctx, req.Url, repositoryFullPath); err != nil {
+		if !strings.HasSuffix(strings.ToLower(req.Url), ".git") {
+			if _, err = cloneRepositoryFromURL(ctx, req.Url+".git", repositoryFullPath); err != nil {
+				return cloneErrorMessage(err)
+			}
+		} else {
+			return cloneErrorMessage(err)
+		}
 	}
 
 	// CreateRepository is harmless on existing repositories with the side effect that it creates the hook symlink.
@@ -69,3 +62,30 @@ func validateCreateRepositoryFromURLRequest(req *gitalypb.CreateRepositoryFromUR
 
 	return nil
 }
+
+func cloneRepositoryFromURL(ctx context.Context, url string, repositoryFullPath string) (*gitalypb.CreateRepositoryFromURLResponse, error) {
+	args := []string{
+		"-c",
+		"http.followRedirects=false",
+		"clone",
+		"--bare",
+		"--",
+		url,
+		repositoryFullPath,
+	}
+
+	cmd, err := git.CommandWithoutRepo(ctx, args...)
+	if err != nil {
+		return nil, status.Errorf(codes.Internal, "CreateRepositoryFromURL: clone cmd start: %v", err)
+	}
+
+	if err := cmd.Wait(); err != nil {
+		os.RemoveAll(repositoryFullPath)
+		return nil, status.Errorf(codes.Internal, "CreateRepositoryFromURL: clone cmd wait: %v", err)
+	}
+	return &gitalypb.CreateRepositoryFromURLResponse{}, nil
+}
+
+func cloneErrorMessage(err error) (*gitalypb.CreateRepositoryFromURLResponse, error) {
+	return nil, status.Errorf(codes.Internal, "CreateRepositoryFromURL: clone failed: %v", err)
+}
diff --git a/internal/service/repository/create_from_url_test.go b/internal/service/repository/create_from_url_test.go
index b760926e6..ffe909a40 100644
--- a/internal/service/repository/create_from_url_test.go
+++ b/internal/service/repository/create_from_url_test.go
@@ -135,3 +135,32 @@ func TestPreventingRedirect(t *testing.T) {
 
 	require.Error(t, err)
 }
+
+func TestAddingGitSuffix(t *testing.T) {
+	server, serverSocketPath := runRepoServer(t)
+	defer server.Stop()
+
+	client, conn := newRepositoryClient(t, serverSocketPath)
+	defer conn.Close()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	importedRepo := &gitalypb.Repository{
+		RelativePath: "imports/test-repo-import",
+		StorageName:  testhelper.DefaultStorageName,
+	}
+
+	httpServerState, redirectingServer := StartRedirectingTestServer()
+	defer redirectingServer.Close()
+
+	req := &gitalypb.CreateRepositoryFromURLRequest{
+		Repository: importedRepo,
+		Url:        redirectingServer.URL + "/repo",
+	}
+
+	_, err := client.CreateRepositoryFromURL(ctx, req)
+
+	require.Error(t, err)
+	require.Equal(t, httpServerState.requestedPaths, []string{"/repo/info/refs", "/repo.git/info/refs"})
+}
diff --git a/internal/service/repository/redirecting_test_server_test.go b/internal/service/repository/redirecting_test_server_test.go
index 8369b835d..2acc56beb 100644
--- a/internal/service/repository/redirecting_test_server_test.go
+++ b/internal/service/repository/redirecting_test_server_test.go
@@ -18,6 +18,7 @@ const redirectURL = "/redirect_url"
 type RedirectingTestServerState struct {
 	serverVisited              bool
 	serverVisitedAfterRedirect bool
+	requestedPaths             []string
 }
 
 // StartRedirectingTestServer starts the test server with initial state
@@ -25,6 +26,7 @@ func StartRedirectingTestServer() (*RedirectingTestServerState, *httptest.Server
 	state := &RedirectingTestServerState{}
 	server := httptest.NewServer(
 		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			state.requestedPaths = append(state.requestedPaths, r.URL.Path)
 			if r.URL.Path == redirectURL {
 				state.serverVisitedAfterRedirect = true
 			} else {