From 0e0abe1565ba10cceb57fcb9abb936b446aed271 Mon Sep 17 00:00:00 2001
From: Adam Hegyi <ahegyi@gitlab.com>
Date: Mon, 2 Sep 2019 09:06:54 +0200
Subject: [PATCH] Adding .git suffix when clone from URL fails

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.
---
 .../add-git-suffix-when-cloning-from-url.yml  |  5 ++
 .../service/repository/create_from_url.go     | 52 +++++++++++++------
 .../repository/create_from_url_test.go        | 29 +++++++++++
 .../redirecting_test_server_test.go           |  2 +
 4 files changed, 72 insertions(+), 16 deletions(-)
 create mode 100644 changelogs/unreleased/add-git-suffix-when-cloning-from-url.yml

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 00000000000..b36ada7ffa2
--- /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 7bfa59a2f52..4b149275db2 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 b760926e6bf..ffe909a40b1 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 8369b835d24..2acc56bebb0 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 {
-- 
GitLab