Merge branch 'security-42616-clone-from-url-dont-follow-http-redirects' into 'master'

Do not follow redirect when cloning repo from URL

See merge request gitlab/gitaly!30

3 files changed, 88 insertions, 0 deletions

diff --git a/internal/service/repository/create_from_url.go b/internal/service/repository/create_from_url.go
index 9a67a8e67..7bfa59a2f 100644
--- a/internal/service/repository/create_from_url.go
+++ b/internal/service/repository/create_from_url.go
@@ -29,6 +29,8 @@ func (s *server) CreateRepositoryFromURL(ctx context.Context, req *gitalypb.Crea
 	}
 
 	args := []string{
+		"-c",
+		"http.followRedirects=false",
 		"clone",
 		"--bare",
 		"--",
diff --git a/internal/service/repository/create_from_url_test.go b/internal/service/repository/create_from_url_test.go
index 10d3431c5..b760926e6 100644
--- a/internal/service/repository/create_from_url_test.go
+++ b/internal/service/repository/create_from_url_test.go
@@ -104,3 +104,34 @@ func TestFailedCreateRepositoryFromURLRequestDueToExistingTarget(t *testing.T) {
 		})
 	}
 }
+
+func TestPreventingRedirect(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-imported.git",
+		StorageName:  testhelper.DefaultStorageName,
+	}
+
+	httpServerState, redirectingServer := StartRedirectingTestServer()
+	defer redirectingServer.Close()
+
+	req := &gitalypb.CreateRepositoryFromURLRequest{
+		Repository: importedRepo,
+		Url:        redirectingServer.URL,
+	}
+
+	_, err := client.CreateRepositoryFromURL(ctx, req)
+
+	require.True(t, httpServerState.serverVisited, "git command should make the initial HTTP request")
+	require.False(t, httpServerState.serverVisitedAfterRedirect, "git command should not follow HTTP redirection")
+
+	require.Error(t, err)
+}
diff --git a/internal/service/repository/redirecting_test_server_test.go b/internal/service/repository/redirecting_test_server_test.go
new file mode 100644
index 000000000..8369b835d
--- /dev/null
+++ b/internal/service/repository/redirecting_test_server_test.go
@@ -0,0 +1,55 @@
+package repository
+
+import (
+	"os/exec"
+	"testing"
+
+	"net/http"
+	"net/http/httptest"
+
+	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/command"
+	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
+)
+
+const redirectURL = "/redirect_url"
+
+// RedirectingTestServerState holds information about whether the server was visited and redirect was happened
+type RedirectingTestServerState struct {
+	serverVisited              bool
+	serverVisitedAfterRedirect bool
+}
+
+// StartRedirectingTestServer starts the test server with initial state
+func StartRedirectingTestServer() (*RedirectingTestServerState, *httptest.Server) {
+	state := &RedirectingTestServerState{}
+	server := httptest.NewServer(
+		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			if r.URL.Path == redirectURL {
+				state.serverVisitedAfterRedirect = true
+			} else {
+				state.serverVisited = true
+				http.Redirect(w, r, redirectURL, http.StatusMovedPermanently)
+			}
+		}),
+	)
+
+	return state, server
+}
+
+func TestRedirectingServerRedirects(t *testing.T) {
+	dir, cleanup := testhelper.TempDir(t, "", t.Name())
+	defer cleanup()
+
+	httpServerState, redirectingServer := StartRedirectingTestServer()
+
+	// we only test for redirection, this command can fail after that
+	cmd := exec.Command("git", "-c", "http.followRedirects=true", "clone", "--bare", redirectingServer.URL, dir)
+	cmd.Env = append(command.GitEnv, cmd.Env...)
+	cmd.Run()
+
+	redirectingServer.Close()
+
+	require.True(t, httpServerState.serverVisited, "git command should make the initial HTTP request")
+	require.True(t, httpServerState.serverVisitedAfterRedirect, "git command should follow the HTTP redirect")
+}