fix: Fix the panic when ssh remote lfs endpoint parsing failure (#38026)

Fix #38016

modules/lfs/client.go

@@ -5,9 +5,12 @@ package lfs
 
 import (
 	"context"
+	"fmt"
 	"io"
 	"net/http"
 	"net/url"
+
+	"gitea.dev/modules/util"
 )
 
 // DownloadCallback gets called for every requested LFS object to process its content
@@ -23,10 +26,23 @@ type Client interface {
 	Upload(ctx context.Context, objects []Pointer, callback UploadCallback) error
 }
 
-// NewClient creates a LFS client
+// newClient creates a LFS client
-func NewClient(endpoint *url.URL, httpTransport *http.Transport) Client {
+func newClient(endpoint *url.URL, httpTransport *http.Transport) Client {
 	if endpoint.Scheme == "file" {
 		return newFilesystemClient(endpoint)
 	}
 	return newHTTPClient(endpoint, httpTransport)
 }
+
+// NewClientFromEndpoint creates a LFS client after resolving its endpoint.
+func NewClientFromEndpoint(cloneurl, lfsurl string, httpTransport *http.Transport) (Client, error) {
+	endpoint := DetermineEndpoint(cloneurl, lfsurl)
+	if endpoint == nil {
+		source := cloneurl
+		if lfsurl != "" {
+			source = lfsurl
+		}
+		return nil, fmt.Errorf("unable to determine LFS endpoint from %q", util.SanitizeCredentialURLs(source))
+	}
+	return newClient(endpoint, httpTransport), nil
+}

modules/lfs/client_test.go

@@ -12,10 +12,21 @@ import (
 
 func TestNewClient(t *testing.T) {
 	u, _ := url.Parse("file:///test")
-	c := NewClient(u, nil)
+	c := newClient(u, nil)
 	assert.IsType(t, &FilesystemClient{}, c)
 
 	u, _ = url.Parse("https://test.com/lfs")
-	c = NewClient(u, nil)
+	c = newClient(u, nil)
 	assert.IsType(t, &HTTPClient{}, c)
 }
+
+func TestNewClientFromEndpoint(t *testing.T) {
+	client, err := NewClientFromEndpoint("ssh://git@example.com/owner/repo.git", "", nil)
+	assert.NoError(t, err)
+	assert.NotNil(t, client)
+
+	client, err = NewClientFromEndpoint("ftp://example.com/owner/repo.git", "", nil)
+	assert.Nil(t, client)
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "unable to determine LFS endpoint")
+}

modules/lfs/endpoint.go

@@ -10,6 +10,7 @@ import (
 	"path/filepath"
 	"strings"
 
+	giturl "gitea.dev/modules/git/url"
 	"gitea.dev/modules/log"
 	"gitea.dev/modules/util"
 )
@@ -44,15 +45,20 @@ func endpointFromCloneURL(rawurl string) *url.URL {
 }
 
 func endpointFromURL(rawurl string) *url.URL {
+	if rawurl == "" {
+		return nil
+	}
+
 	if strings.HasPrefix(rawurl, "/") {
 		return endpointFromLocalPath(rawurl)
 	}
 
-	u, err := url.Parse(rawurl)
+	gitURL, err := giturl.ParseGitURL(rawurl)
 	if err != nil {
 		log.Error("lfs.endpointFromUrl: %v", err)
 		return nil
 	}
+	u := gitURL.URL
 
 	switch u.Scheme {
 	case "http", "https":
@@ -60,6 +66,12 @@ func endpointFromURL(rawurl string) *url.URL {
 	case "git":
 		u.Scheme = "https"
 		return u
+	case "ssh", "git+ssh":
+		u.Scheme = "https"    // is it possible http?
+		u.Host = u.Hostname() // remove ssh port if any
+		u.Path = "/" + strings.TrimPrefix(u.Path, "/")
+		u.User = nil
+		return u
 	case "file":
 		return u
 	default:

modules/lfs/endpoint_test.go

@@ -64,6 +64,24 @@ func TestDetermineEndpoint(t *testing.T) {
 			lfsurl:   "git://gitlfs.com/repo",
 			expected: str2url("https://gitlfs.com/repo"),
 		},
+		// case 7
+		{
+			cloneurl: "ssh://git@git.com/owner/repo.git",
+			lfsurl:   "",
+			expected: str2url("https://git.com/owner/repo.git/info/lfs"),
+		},
+		// case 8
+		{
+			cloneurl: "git@git.com:owner/repo.git",
+			lfsurl:   "",
+			expected: str2url("https://git.com/owner/repo.git/info/lfs"),
+		},
+		// case 9
+		{
+			cloneurl: "",
+			lfsurl:   "ssh://git@gitlfs.com/owner/repo.git/info/lfs",
+			expected: str2url("https://gitlfs.com/owner/repo.git/info/lfs"),
+		},
 	}
 
 	for n, c := range cases {

services/mirror/mirror_pull.go

@@ -172,9 +172,10 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*repo_module.SyncResu
 
 	if m.LFS && setting.LFS.StartServer {
 		log.Trace("SyncMirrors [repo: %-v]: syncing LFS objects...", m.Repo)
-		endpoint := lfs.DetermineEndpoint(remoteURL.String(), m.LFSEndpoint)
+		lfsClient, err := lfs.NewClientFromEndpoint(remoteURL.String(), m.LFSEndpoint, migrations.NewMigrationHTTPTransport())
-		lfsClient := lfs.NewClient(endpoint, migrations.NewMigrationHTTPTransport())
+		if err != nil {
-		if err = repo_module.StoreMissingLfsObjectsInRepository(ctx, m.Repo, gitRepo, lfsClient); err != nil {
+			log.Error("SyncMirrors [repo: %-v]: failed to initialize LFS client: %v", m.Repo.FullName(), err)
+		} else if err = repo_module.StoreMissingLfsObjectsInRepository(ctx, m.Repo, gitRepo, lfsClient); err != nil {
 			log.Error("SyncMirrors [repo: %-v]: failed to synchronize LFS objects for repository: %v", m.Repo.FullName(), err)
 		}
 	}

services/mirror/mirror_push.go

@@ -144,8 +144,10 @@ func runPushSync(ctx context.Context, m *repo_model.PushMirror) error {
 			}
 			defer gitRepo.Close()
 
-			endpoint := lfs.DetermineEndpoint(remoteURL.String(), "")
+			lfsClient, err := lfs.NewClientFromEndpoint(remoteURL.String(), "", migrations.NewMigrationHTTPTransport())
-			lfsClient := lfs.NewClient(endpoint, migrations.NewMigrationHTTPTransport())
+			if err != nil {
+				return err
+			}
 			if err := pushAllLFSObjects(ctx, gitRepo, lfsClient); err != nil {
 				return util.SanitizeErrorCredentialURLs(err)
 			}

services/repository/migrate.go

@@ -159,8 +159,10 @@ func MigrateRepositoryGitData(ctx context.Context, u *user_model.User,
 		}
 
 		if opts.LFS {
-			endpoint := lfs.DetermineEndpoint(opts.CloneAddr, opts.LFSEndpoint)
+			lfsClient, err := lfs.NewClientFromEndpoint(opts.CloneAddr, opts.LFSEndpoint, httpTransport)
-			lfsClient := lfs.NewClient(endpoint, httpTransport)
+			if err != nil {
+				return repo, fmt.Errorf("NewClientFromEndpoint: %w", err)
+			}
 			if err = repo_module.StoreMissingLfsObjectsInRepository(ctx, repo, gitRepo, lfsClient); err != nil {
 				log.Error("Failed to store missing LFS objects for repository: %v", err)
 				return repo, fmt.Errorf("StoreMissingLfsObjectsInRepository: %w", err)