diff --git a/apps/backend/internal/gitlab/service_task_mr_link.go b/apps/backend/internal/gitlab/service_task_mr_link.go index 5a3127186b..c00210dfa9 100644 --- a/apps/backend/internal/gitlab/service_task_mr_link.go +++ b/apps/backend/internal/gitlab/service_task_mr_link.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "net" "net/url" "strconv" "strings" @@ -36,8 +37,7 @@ func parseMRURLForHost(rawURL, configuredHost string) (string, int, error) { if err != nil { return "", 0, ErrInvalidMRURL } - expectedOrigin, err := normalizeHostOrigin(configuredHost) - if err != nil || !strings.EqualFold(origin, expectedOrigin) { + if !sameConfiguredOrigin(origin, configuredHost) { return "", 0, fmt.Errorf("%w: host does not match workspace connection", ErrInvalidMRURL) } const marker = "/-/merge_requests/" @@ -58,6 +58,103 @@ func parseMRURLForHost(rawURL, configuredHost string) (string, int, error) { return projectPath, iid, nil } +// parseGitLabRemoteURLIdentity extracts a normalized (host origin, project +// path) pair from a git remote URL, accepting HTTPS, ssh://, and scp-style +// (git@host:path) forms. SSH remotes are normalized to an HTTPS-style origin +// so they can be host-compared against a configured GitLab connection host, +// which is always stored as an HTTP(S) origin. Returns empty strings when the +// URL cannot be parsed as an owner/repo remote. +func parseGitLabRemoteURLIdentity(remoteURL string) (host, projectPath string) { + const sshScheme = "ssh" + remoteURL = strings.TrimSpace(remoteURL) + if remoteURL == "" { + return "", "" + } + scheme, hostname, path, ok := splitRemoteURLIdentity(remoteURL) + if !ok { + return "", "" + } + scheme = strings.ToLower(scheme) + if scheme != mentionHTTPScheme && scheme != mentionHTTPSScheme && scheme != sshScheme { + return "", "" + } + if scheme == sshScheme { + scheme = mentionHTTPSScheme + } + path = strings.TrimSuffix(strings.Trim(strings.TrimSpace(path), "/"), ".git") + if hostname == "" || path == "" || !strings.Contains(path, "/") { + return "", "" + } + return scheme + "://" + hostname, path +} + +// splitRemoteURLIdentity resolves the (scheme, hostname, path) triple for +// either a scp-style shorthand (git@host:path) or a URL-form remote +// (https://, ssh://, etc). Extracted from parseGitLabRemoteURLIdentity to +// keep its cyclomatic complexity within the linter budget. +func splitRemoteURLIdentity(remoteURL string) (scheme, hostname, path string, ok bool) { + const sshScheme = "ssh" + if strings.Contains(remoteURL, "@") && !strings.Contains(remoteURL, "://") { + // scp-style shorthand: git@host:group/sub/project.git (host may be a + // bracketed IPv6 literal, e.g. git@[::1]:group/project.git, whose + // embedded colons must not be mistaken for the host/path separator). + _, rest, _ := strings.Cut(remoteURL, "@") + h, p, cutOK := cutSCPHostPath(rest) + if !cutOK { + return "", "", "", false + } + return sshScheme, h, p, true + } + parsed, err := url.Parse(remoteURL) + if err != nil || parsed.Scheme == "" || parsed.Host == "" { + return "", "", "", false + } + hostname = remoteHost(parsed) + if strings.EqualFold(parsed.Scheme, sshScheme) { + // SSH transport ports (e.g. ssh://git@host:2222/...) are unrelated to + // the GitLab web origin's port, so compare hostname only. Bracket + // IPv6 literals so the synthesized origin stays a valid URL (an + // unbracketed "https://::1" is not). + hostname = bracketedHostname(parsed.Hostname()) + } + return parsed.Scheme, hostname, parsed.Path, true +} + +// cutSCPHostPath splits an scp-style remote's "host:path" segment (the part +// after "git@"), honoring bracketed IPv6 literals such as +// "[::1]:group/project.git" whose embedded colons would otherwise be +// mistaken for the host/path separator by a plain strings.Cut on ":". +func cutSCPHostPath(rest string) (host, path string, ok bool) { + if strings.HasPrefix(rest, "[") { + closeIdx := strings.Index(rest, "]") + if closeIdx < 0 || closeIdx+1 >= len(rest) || rest[closeIdx+1] != ':' { + return "", "", false + } + return rest[:closeIdx+1], rest[closeIdx+2:], true + } + return strings.Cut(rest, ":") +} + +func remoteHost(parsed *url.URL) string { + hostname := parsed.Hostname() + if hostname == "" { + return "" + } + if port := parsed.Port(); port != "" { + return net.JoinHostPort(hostname, port) + } + return bracketedHostname(hostname) +} + +// bracketedHostname wraps an IPv6 literal in brackets (as required for a +// valid URL host) and returns other hostnames unchanged. +func bracketedHostname(hostname string) string { + if hostname == "" || !strings.Contains(hostname, ":") { + return hostname + } + return "[" + hostname + "]" +} + // AssociateExistingMRByURL validates a workspace-owned task/repository pair, // fetches the configured-host MR, and idempotently persists its association. func (s *Service) AssociateExistingMRByURL( diff --git a/apps/backend/internal/gitlab/service_task_mr_link_test.go b/apps/backend/internal/gitlab/service_task_mr_link_test.go index c2b962bf2d..c13c953d9f 100644 --- a/apps/backend/internal/gitlab/service_task_mr_link_test.go +++ b/apps/backend/internal/gitlab/service_task_mr_link_test.go @@ -15,10 +15,11 @@ func seedTaskMRLinkFixture(t *testing.T, store *Store, workspaceID, taskID, repo if _, err := store.db.Exec(`CREATE TABLE IF NOT EXISTS repositories ( id TEXT PRIMARY KEY, workspace_id TEXT NOT NULL, - provider TEXT NOT NULL DEFAULT '', - provider_host TEXT NOT NULL DEFAULT '', - provider_owner TEXT NOT NULL DEFAULT '', - provider_name TEXT NOT NULL DEFAULT '' + provider TEXT DEFAULT '', + provider_host TEXT DEFAULT '', + provider_owner TEXT DEFAULT '', + provider_name TEXT DEFAULT '', + remote_url TEXT DEFAULT '' ); CREATE TABLE IF NOT EXISTS task_repositories ( id TEXT PRIMARY KEY, task_id TEXT NOT NULL, @@ -57,6 +58,30 @@ func setTaskMRRepositoryIdentity( } } +func setTaskMRRepositoryRemoteURL(t *testing.T, store *Store, repositoryID, remoteURL string) { + t.Helper() + if _, err := store.db.Exec( + `UPDATE repositories SET remote_url = ? WHERE id = ?`, remoteURL, repositoryID, + ); err != nil { + t.Fatalf("set repository remote_url: %v", err) + } +} + +// setTaskMRRepositoryIdentityColumnsNull sets every provider identity column +// (provider, provider_host, provider_owner, provider_name, remote_url) to +// SQL NULL, mirroring rows persisted before those columns existed or by code +// paths that never populated them. The production schema declares them +// nullable, so ValidateTaskMRRepositoryIdentity must tolerate NULL scans. +func setTaskMRRepositoryIdentityColumnsNull(t *testing.T, store *Store, repositoryID string) { + t.Helper() + if _, err := store.db.Exec(`UPDATE repositories + SET provider = NULL, provider_host = NULL, provider_owner = NULL, + provider_name = NULL, remote_url = NULL + WHERE id = ?`, repositoryID); err != nil { + t.Fatalf("null repository identity columns: %v", err) + } +} + func newTaskMRLinkService(t *testing.T, host string) (*Service, *Store, *MockClient) { t.Helper() store := newTestStore(t) @@ -103,6 +128,28 @@ func TestAssociateExistingMRByURLCreatesIdempotentWorkspaceScopedLink(t *testing } } +func TestAssociateExistingMRByURLAcceptsMRURLWithExplicitDefaultPort(t *testing.T) { + // The workspace's configured GitLab host and the incoming MR URL should + // match even when only one side spells out the scheme's implicit default + // port (":443" for https), since they identify the same web origin. + const configuredHost = "https://gitlab.acme.test:443" + const mrURLHost = "https://gitlab.acme.test" + svc, store, client := newTaskMRLinkService(t, configuredHost) + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + setTaskMRRepositoryIdentity(t, store, "repo-1", configuredHost, "acme/api") + client.SeedMR("acme/api", &MR{ + IID: 21, Title: "MR", WebURL: mrURLHost + "/acme/api/-/merge_requests/21", + State: "opened", CreatedAt: time.Now().UTC(), + }) + + if _, err := svc.AssociateExistingMRByURL( + context.Background(), "ws-1", "task-1", "repo-1", + mrURLHost+"/acme/api/-/merge_requests/21", + ); err != nil { + t.Fatalf("AssociateExistingMRByURL: %v", err) + } +} + func TestAssociateExistingMRByURLRejectsWrongHostAndCrossWorkspaceResources(t *testing.T) { const host = "http://gitlab.internal.test" svc, store, client := newTaskMRLinkService(t, host) @@ -151,7 +198,6 @@ func TestAssociateExistingMRByURLRejectsRepositoryIdentityMismatch(t *testing.T) repositoryHost string repositoryPath string }{ - {name: "same hostname with HTTPS origin", repositoryHost: "https://gitlab.internal.test:8080", repositoryPath: "group/subgroup/project"}, {name: "different GitLab host", repositoryHost: "http://other.internal.test:8080", repositoryPath: "group/subgroup/project"}, {name: "different subgroup project", repositoryHost: host, repositoryPath: "group/other/project"}, {name: "unknown legacy host", repositoryHost: "", repositoryPath: "group/subgroup/project"}, @@ -195,6 +241,197 @@ func TestAssociateExistingMRByURLAcceptsExactSelfManagedHTTPSRepositoryIdentity( } } +func TestAssociateExistingMRByURLAcceptsRepositoryIdentityWithExplicitDefaultPort(t *testing.T) { + const host = "https://gitlab.internal.test" + svc, store, client := newTaskMRLinkService(t, host) + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + // An explicit default HTTPS port (:443) must compare equal to the + // workspace host, which omits it. + setTaskMRRepositoryIdentity(t, store, "repo-1", "https://gitlab.internal.test:443", "group/subgroup/project") + client.SeedMR("group/subgroup/project", &MR{ + IID: 13, Title: "MR", WebURL: host + "/group/subgroup/project/-/merge_requests/13", + State: "opened", CreatedAt: time.Now().UTC(), + }) + + if _, err := svc.AssociateExistingMRByURL( + context.Background(), "ws-1", "task-1", "repo-1", + host+"/group/subgroup/project/-/merge_requests/13", + ); err != nil { + t.Fatalf("AssociateExistingMRByURL: %v", err) + } +} + +func TestAssociateExistingMRByURLAcceptsScpStyleRemoteWithBracketedIPv6Host(t *testing.T) { + const host = "https://[::1]" + svc, store, client := newTaskMRLinkService(t, host) + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + setTaskMRRepositoryRemoteURL(t, store, "repo-1", "git@[::1]:clients/socodevi/laravel/co-up.git") + client.SeedMR("clients/socodevi/laravel/co-up", &MR{ + IID: 92, Title: "MR", WebURL: host + "/clients/socodevi/laravel/co-up/-/merge_requests/92", + State: "opened", CreatedAt: time.Now().UTC(), + }) + + if _, err := svc.AssociateExistingMRByURL( + context.Background(), "ws-1", "task-1", "repo-1", + host+"/clients/socodevi/laravel/co-up/-/merge_requests/92", + ); err != nil { + t.Fatalf("AssociateExistingMRByURL: %v", err) + } +} + +func TestAssociateExistingMRByURLAcceptsSSHURLRemoteWithIPv6Host(t *testing.T) { + const host = "https://[::1]" + tests := []struct { + name string + remoteURL string + }{ + {name: "no port", remoteURL: "ssh://git@[::1]/clients/socodevi/laravel/co-up.git"}, + {name: "explicit non-web port", remoteURL: "ssh://git@[::1]:2222/clients/socodevi/laravel/co-up.git"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + svc, store, client := newTaskMRLinkService(t, host) + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + setTaskMRRepositoryRemoteURL(t, store, "repo-1", tt.remoteURL) + client.SeedMR("clients/socodevi/laravel/co-up", &MR{ + IID: 92, Title: "MR", WebURL: host + "/clients/socodevi/laravel/co-up/-/merge_requests/92", + State: "opened", CreatedAt: time.Now().UTC(), + }) + + if _, err := svc.AssociateExistingMRByURL( + context.Background(), "ws-1", "task-1", "repo-1", + host+"/clients/socodevi/laravel/co-up/-/merge_requests/92", + ); err != nil { + t.Fatalf("AssociateExistingMRByURL: %v", err) + } + }) + } +} + +func TestAssociateExistingMRByURLAcceptsRepositoryIdentityWithSameHostDifferentScheme(t *testing.T) { + const host = "http://gitlab.internal.test:8080" + svc, store, client := newTaskMRLinkService(t, host) + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + setTaskMRRepositoryIdentity(t, store, "repo-1", "https://gitlab.internal.test:8080", "group/subgroup/project") + client.SeedMR("group/subgroup/project", &MR{ + IID: 12, Title: "MR", WebURL: host + "/group/subgroup/project/-/merge_requests/12", + State: "opened", CreatedAt: time.Now().UTC(), + }) + + if _, err := svc.AssociateExistingMRByURL( + context.Background(), "ws-1", "task-1", "repo-1", + host+"/group/subgroup/project/-/merge_requests/12", + ); err != nil { + t.Fatalf("AssociateExistingMRByURL: %v", err) + } +} + +func TestAssociateExistingMRByURLAcceptsRemoteURLWhenDurableIdentityEmpty(t *testing.T) { + // Reproduces the reported bug: a self-hosted GitLab repository added as a + // local clone never gets a durable provider identity (only github.com and + // gitlab.com are tagged at discovery time), but its remote_url still + // clearly identifies the same GitLab host and nested-subgroup project. + const host = "https://gitlab.savoirfairelinux.com" + tests := []struct { + name string + remoteURL string + }{ + {name: "https remote", remoteURL: host + "/clients/socodevi/laravel/co-up.git"}, + {name: "https remote without .git suffix", remoteURL: host + "/clients/socodevi/laravel/co-up"}, + {name: "https remote with trailing slash", remoteURL: host + "/clients/socodevi/laravel/co-up/"}, + {name: "https remote with different case", remoteURL: "https://GitLab.SavoirFaireLinux.com/Clients/Socodevi/Laravel/Co-Up.git"}, + {name: "http remote for https workspace host", remoteURL: "http://gitlab.savoirfairelinux.com/clients/socodevi/laravel/co-up.git"}, + {name: "ssh url remote", remoteURL: "ssh://git@gitlab.savoirfairelinux.com/clients/socodevi/laravel/co-up.git"}, + {name: "scp-style remote", remoteURL: "git@gitlab.savoirfairelinux.com:clients/socodevi/laravel/co-up.git"}, + // SSH transport ports (used by hosts that put SSH on a non-standard + // port) are unrelated to the GitLab web origin's port and must not + // be compared against it. + {name: "ssh url remote with explicit non-web port", remoteURL: "ssh://git@gitlab.savoirfairelinux.com:2222/clients/socodevi/laravel/co-up.git"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + svc, store, client := newTaskMRLinkService(t, host) + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + setTaskMRRepositoryRemoteURL(t, store, "repo-1", tt.remoteURL) + client.SeedMR("clients/socodevi/laravel/co-up", &MR{ + IID: 92, Title: "MR", WebURL: host + "/clients/socodevi/laravel/co-up/-/merge_requests/92", + State: "opened", CreatedAt: time.Now().UTC(), + }) + + if _, err := svc.AssociateExistingMRByURL( + context.Background(), "ws-1", "task-1", "repo-1", + host+"/clients/socodevi/laravel/co-up/-/merge_requests/92", + ); err != nil { + t.Fatalf("AssociateExistingMRByURL: %v", err) + } + }) + } +} + +func TestAssociateExistingMRByURLRejectsRemoteURLPointingElsewhere(t *testing.T) { + const host = "https://gitlab.savoirfairelinux.com" + svc, store, client := newTaskMRLinkService(t, host) + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + // Durable identity empty (legacy/unresolved row) and remote_url points to + // a different project than the MR being linked. + setTaskMRRepositoryRemoteURL(t, store, "repo-1", host+"/clients/socodevi/laravel/other-project.git") + client.SeedMR("clients/socodevi/laravel/co-up", &MR{ + IID: 92, Title: "MR", WebURL: host + "/clients/socodevi/laravel/co-up/-/merge_requests/92", + State: "opened", CreatedAt: time.Now().UTC(), + }) + + _, err := svc.AssociateExistingMRByURL( + context.Background(), "ws-1", "task-1", "repo-1", + host+"/clients/socodevi/laravel/co-up/-/merge_requests/92", + ) + if !errors.Is(err, ErrTaskMRRepositoryMismatch) { + t.Fatalf("error = %v, want ErrTaskMRRepositoryMismatch", err) + } +} + +func TestAssociateExistingMRByURLRejectsRemoteURLOnDifferentHost(t *testing.T) { + const host = "https://gitlab.savoirfairelinux.com" + svc, store, client := newTaskMRLinkService(t, host) + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + setTaskMRRepositoryRemoteURL(t, store, "repo-1", "https://gitlab.other.test/clients/socodevi/laravel/co-up.git") + client.SeedMR("clients/socodevi/laravel/co-up", &MR{ + IID: 92, Title: "MR", WebURL: host + "/clients/socodevi/laravel/co-up/-/merge_requests/92", + State: "opened", CreatedAt: time.Now().UTC(), + }) + + _, err := svc.AssociateExistingMRByURL( + context.Background(), "ws-1", "task-1", "repo-1", + host+"/clients/socodevi/laravel/co-up/-/merge_requests/92", + ) + if !errors.Is(err, ErrTaskMRRepositoryMismatch) { + t.Fatalf("error = %v, want ErrTaskMRRepositoryMismatch", err) + } +} + +func TestAssociateExistingMRByURLRejectsRepositoryWithNullIdentityColumns(t *testing.T) { + // The production schema declares provider/provider_host/provider_owner/ + // provider_name/remote_url as nullable columns, so rows can legitimately + // have SQL NULL there. ValidateTaskMRRepositoryIdentity must coalesce + // those to empty strings and fail closed with the normal mismatch error + // instead of an internal scan error. + const host = "https://gitlab.savoirfairelinux.com" + svc, store, client := newTaskMRLinkService(t, host) + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + setTaskMRRepositoryIdentityColumnsNull(t, store, "repo-1") + client.SeedMR("clients/socodevi/laravel/co-up", &MR{ + IID: 92, Title: "MR", WebURL: host + "/clients/socodevi/laravel/co-up/-/merge_requests/92", + State: "opened", CreatedAt: time.Now().UTC(), + }) + + _, err := svc.AssociateExistingMRByURL( + context.Background(), "ws-1", "task-1", "repo-1", + host+"/clients/socodevi/laravel/co-up/-/merge_requests/92", + ) + if !errors.Is(err, ErrTaskMRRepositoryMismatch) { + t.Fatalf("error = %v, want ErrTaskMRRepositoryMismatch", err) + } +} + func TestAssociateExistingMRByURLInfersSoleRepositoryAndRejectsMultiRepoAmbiguity(t *testing.T) { const host = "https://gitlab.acme.test" svc, store, client := newTaskMRLinkService(t, host) diff --git a/apps/backend/internal/gitlab/store_task_mr_link.go b/apps/backend/internal/gitlab/store_task_mr_link.go index 47844b4003..c02efcd935 100644 --- a/apps/backend/internal/gitlab/store_task_mr_link.go +++ b/apps/backend/internal/gitlab/store_task_mr_link.go @@ -5,6 +5,7 @@ import ( "database/sql" "errors" "fmt" + "net/url" "strings" ) @@ -40,9 +41,13 @@ func (s *Store) ValidateTaskMRScope(ctx context.Context, workspaceID, taskID, re return nil } -// ValidateTaskMRRepositoryIdentity verifies the durable provider identity for -// the repository selected by the task. Legacy rows without a provider host -// fail closed because owner/name alone cannot distinguish GitLab instances. +// ValidateTaskMRRepositoryIdentity verifies the repository selected by the +// task identifies the same GitLab host and project as the MR. It accepts a +// match against either the repository's durable provider identity or its +// remote_url, since self-hosted GitLab repositories added as local clones +// never get a durable provider identity (only github.com/gitlab.com are +// tagged at discovery time) yet still carry a resolvable remote_url. Rows +// with neither signal resolving to a GitLab identity fail closed. func (s *Store) ValidateTaskMRRepositoryIdentity( ctx context.Context, workspaceID, taskID, repositoryID, configuredHost, projectPath string, @@ -51,13 +56,18 @@ func (s *Store) ValidateTaskMRRepositoryIdentity( return ErrTaskMRRepositoryMismatch } var identity struct { - Provider string `db:"provider"` - Host string `db:"provider_host"` - Owner string `db:"provider_owner"` - Name string `db:"provider_name"` + Provider string `db:"provider"` + Host string `db:"provider_host"` + Owner string `db:"provider_owner"` + Name string `db:"provider_name"` + RemoteURL string `db:"remote_url"` } err := s.ro.GetContext(ctx, &identity, ` - SELECT r.provider, r.provider_host, r.provider_owner, r.provider_name + SELECT COALESCE(r.provider, '') AS provider, + COALESCE(r.provider_host, '') AS provider_host, + COALESCE(r.provider_owner, '') AS provider_owner, + COALESCE(r.provider_name, '') AS provider_name, + COALESCE(r.remote_url, '') AS remote_url FROM task_repositories tr JOIN repositories r ON r.id = tr.repository_id JOIN tasks t ON t.id = tr.task_id @@ -74,18 +84,67 @@ func (s *Store) ValidateTaskMRRepositoryIdentity( if err != nil { return ErrTaskMRRepositoryMismatch } - gotHost, err := normalizeHostOrigin(identity.Host) - if err != nil { - return ErrTaskMRRepositoryMismatch - } - gotProject := strings.Trim(strings.TrimSpace(identity.Owner+"/"+identity.Name), "/") wantProject := strings.Trim(strings.TrimSpace(projectPath), "/") - if !strings.EqualFold(identity.Provider, "gitlab") || - !strings.EqualFold(gotHost, wantHost) || - !strings.EqualFold(gotProject, wantProject) { - return ErrTaskMRRepositoryMismatch + + type candidate struct{ host, project string } + var candidates []candidate + if strings.EqualFold(identity.Provider, "gitlab") { + if gotHost, err := normalizeHostOrigin(identity.Host); err == nil { + gotProject := strings.Trim(strings.TrimSpace(identity.Owner+"/"+identity.Name), "/") + candidates = append(candidates, candidate{host: gotHost, project: gotProject}) + } + } + if remoteURLHost, remoteProject := parseGitLabRemoteURLIdentity(identity.RemoteURL); remoteURLHost != "" { + if gotHost, err := normalizeHostOrigin(remoteURLHost); err == nil { + candidates = append(candidates, candidate{host: gotHost, project: remoteProject}) + } + } + for _, c := range candidates { + if sameGitLabHost(c.host, wantHost) && strings.EqualFold(c.project, wantProject) { + return nil + } + } + return ErrTaskMRRepositoryMismatch +} + +func sameGitLabHost(left, right string) bool { + leftURL, leftErr := validateHost(strings.TrimSpace(left)) + rightURL, rightErr := validateHost(strings.TrimSpace(right)) + if leftErr != nil || rightErr != nil { + return false + } + return strings.EqualFold(hostWithoutDefaultPort(leftURL), hostWithoutDefaultPort(rightURL)) +} + +// sameConfiguredOrigin reports whether candidate and configured identify the +// same GitLab web origin: same scheme, and hosts equal once each side's +// implicit default port is stripped. Used to validate an incoming MR URL's +// origin against the workspace's configured GitLab host, so an explicit +// "https://gitlab.example.com:443" on either side still matches the +// equivalent unported form. +func sameConfiguredOrigin(candidate, configured string) bool { + candidateURL, candidateErr := validateHost(strings.TrimSpace(candidate)) + configuredURL, configuredErr := validateHost(strings.TrimSpace(configured)) + if candidateErr != nil || configuredErr != nil { + return false + } + return strings.EqualFold(candidateURL.Scheme, configuredURL.Scheme) && + strings.EqualFold(hostWithoutDefaultPort(candidateURL), hostWithoutDefaultPort(configuredURL)) +} + +// hostWithoutDefaultPort strips a scheme's implicit default port (":443" for +// https, ":80" for http) so an explicitly-ported origin like +// "https://gitlab.example.com:443" compares equal to the equivalent +// "https://gitlab.example.com" without one. +func hostWithoutDefaultPort(u *url.URL) string { + switch u.Scheme { + case mentionHTTPSScheme: + return strings.TrimSuffix(u.Host, ":443") + case mentionHTTPScheme: + return strings.TrimSuffix(u.Host, ":80") + default: + return u.Host } - return nil } // ResolveTaskMRRepository validates an explicit repository or infers the sole diff --git a/apps/backend/internal/task/service/repository_discovery.go b/apps/backend/internal/task/service/repository_discovery.go index c2170ff249..7891472dba 100644 --- a/apps/backend/internal/task/service/repository_discovery.go +++ b/apps/backend/internal/task/service/repository_discovery.go @@ -917,7 +917,7 @@ func ParseGitRemoteIdentity(remoteURL string) (origin, projectPath string) { } if strings.Contains(remoteURL, "@") && !strings.Contains(remoteURL, "://") { _, remoteURL, _ = strings.Cut(remoteURL, "@") - host, path, ok := strings.Cut(remoteURL, ":") + host, path, ok := cutSCPHostPath(remoteURL) if !ok { return "", "" } @@ -930,6 +930,21 @@ func ParseGitRemoteIdentity(remoteURL string) (origin, projectPath string) { return normalizeRemoteIdentity(parsed.Scheme, parsed.Host, parsed.Path) } +// cutSCPHostPath splits an scp-style remote's "host:path" segment (the part +// after "git@"), honoring bracketed IPv6 literals such as +// "[::1]:group/project.git" whose embedded colons would otherwise be +// mistaken for the host/path separator by a plain strings.Cut on ":". +func cutSCPHostPath(rest string) (host, path string, ok bool) { + if strings.HasPrefix(rest, "[") { + closeIdx := strings.Index(rest, "]") + if closeIdx < 0 || closeIdx+1 >= len(rest) || rest[closeIdx+1] != ':' { + return "", "", false + } + return rest[:closeIdx+1], rest[closeIdx+2:], true + } + return strings.Cut(rest, ":") +} + func normalizeRemoteIdentity(scheme, host, path string) (string, string) { scheme = strings.ToLower(strings.TrimSpace(scheme)) host = strings.ToLower(strings.TrimSpace(host)) diff --git a/apps/backend/internal/task/service/repository_discovery_test.go b/apps/backend/internal/task/service/repository_discovery_test.go index 56c0ed8520..2c12a2421e 100644 --- a/apps/backend/internal/task/service/repository_discovery_test.go +++ b/apps/backend/internal/task/service/repository_discovery_test.go @@ -550,6 +550,7 @@ func TestParseGitRemoteIdentityPreservesOriginAndFullSubgroupPath(t *testing.T) {"self-managed HTTPS", "https://gitlab.internal/group/subgroup/project", "https://gitlab.internal", "group/subgroup/project"}, {"SSH URL", "ssh://git@gitlab.internal/group/subgroup/project.git", "ssh://gitlab.internal", "group/subgroup/project"}, {"SCP SSH", "git@gitlab.internal:group/subgroup/project.git", "ssh://gitlab.internal", "group/subgroup/project"}, + {"SCP SSH bracketed IPv6", "git@[::1]:group/subgroup/project.git", "ssh://[::1]", "group/subgroup/project"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/apps/backend/internal/task/service/service_resources.go b/apps/backend/internal/task/service/service_resources.go index ea89e07423..bd08dc6218 100644 --- a/apps/backend/internal/task/service/service_resources.go +++ b/apps/backend/internal/task/service/service_resources.go @@ -754,11 +754,6 @@ func (s *Service) createRepository( if resolveProvider { resolveRepositoryProviderIdentity(repository) } - if repository.LocalPath != "" && repository.RemoteURL == "" { - if origin := canonicalCloneOrigin(repository.LocalPath); origin != "" { - repository.RemoteURL = origin - } - } if err := s.repoEntities.CreateRepository(ctx, repository); err != nil { s.logger.Error("failed to create repository", zap.Error(err)) @@ -770,19 +765,42 @@ func (s *Service) createRepository( return repository, nil } -// resolveRepositoryProviderIdentity fills missing provider metadata from a local repository origin. +// resolveRepositoryProviderIdentity fills missing provider metadata from a +// local repository origin. RemoteURL is resolved independently of the +// provider/host/owner/name fields: those are only tagged for the well-known +// github.com/gitlab.com hosts (see ResolveGitRemoteProviderIdentity), but +// self-hosted GitLab/GitHub Enterprise instances still need a populated +// RemoteURL so downstream identity matching (e.g. GitLab MR-task linking) +// has something to compare against instead of failing closed. +// +// canonicalCloneOrigin is tried first because it produces the exact +// provider-canonical clone URL (e.g. the ".git"-suffixed GitHub/GitLab.com +// form) that other code, including test fixtures rewriting Git's clone +// transport via "insteadOf" config, matches against verbatim. The broader +// ResolveGitRemoteIdentity-based fallback only runs when canonicalCloneOrigin +// doesn't recognize the host (e.g. a self-hosted GitLab/GitHub Enterprise +// instance), since it doesn't guarantee a byte-identical canonical form. func resolveRepositoryProviderIdentity(repository *models.Repository) { - if repository.LocalPath == "" || (repository.Provider != "" && repository.ProviderHost != "") { + if repository.LocalPath == "" { return } - p, h, o, n := ResolveGitRemoteProviderIdentity(repository.LocalPath) - if repository.Provider == "" { - repository.Provider = p + if repository.Provider == "" || repository.ProviderHost == "" { + p, h, o, n := ResolveGitRemoteProviderIdentity(repository.LocalPath) + if repository.Provider == "" { + repository.Provider = p + } + if repository.Provider != "" && (strings.HasPrefix(h, "http://") || strings.HasPrefix(h, "https://")) { + repository.ProviderHost = h + repository.ProviderOwner = o + repository.ProviderName = n + } } - if repository.Provider != "" && (strings.HasPrefix(h, "http://") || strings.HasPrefix(h, "https://")) { - repository.ProviderHost = h - repository.ProviderOwner = o - repository.ProviderName = n + if repository.RemoteURL == "" { + if origin := canonicalCloneOrigin(repository.LocalPath); origin != "" { + repository.RemoteURL = origin + } else if origin, owner, name := ResolveGitRemoteIdentity(repository.LocalPath); origin != "" && owner != "" && name != "" { + repository.RemoteURL = origin + "/" + owner + "/" + name + } } } diff --git a/apps/backend/internal/task/service/service_resources_test.go b/apps/backend/internal/task/service/service_resources_test.go index 696414d416..b6c316bda7 100644 --- a/apps/backend/internal/task/service/service_resources_test.go +++ b/apps/backend/internal/task/service/service_resources_test.go @@ -131,6 +131,147 @@ func TestService_CreateRepositoryCanonicalizesExplicitLocalPath(t *testing.T) { } } +// TestService_CreateRepositoryResolvesRemoteURLForSelfHostedGitLabOrigin +// covers the bug where a local repository cloned from a self-hosted GitLab +// instance (e.g. gitlab.example.com, not gitlab.com) got no RemoteURL +// persisted at creation time: resolveRepositoryProviderIdentity only tagged +// Provider/ProviderHost/ProviderOwner/ProviderName for the well-known +// github.com/gitlab.com hosts and never populated RemoteURL at all, so +// downstream GitLab MR-task-link identity matching had nothing to compare +// against and rejected every attach attempt for self-hosted repositories. +func TestService_CreateRepositoryResolvesRemoteURLForSelfHostedGitLabOrigin(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + if err := repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "Workspace"}); err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + + repoPath := t.TempDir() + makeRepo(t, repoPath) + config := `[core] + repositoryformatversion = 0 +[remote "origin"] + url = git@gitlab.example.com:clients/socodevi/laravel/co-up.git + fetch = +refs/heads/*:refs/remotes/origin/* +` + if err := os.WriteFile(filepath.Join(repoPath, ".git", "config"), []byte(config), 0o644); err != nil { + t.Fatalf("write .git/config: %v", err) + } + + created, err := svc.CreateRepository(ctx, &CreateRepositoryRequest{ + WorkspaceID: "ws-1", + Name: "co-up", + SourceType: sourceTypeLocal, + LocalPath: repoPath, + }) + if err != nil { + t.Fatalf("CreateRepository: %v", err) + } + // ResolveGitRemoteIdentity normalizes scp-style remotes to an ssh:// + // origin; the GitLab MR-link identity matcher already treats ssh:// and + // https:// origins as equivalent for the same host+path. + const wantRemoteURL = "ssh://gitlab.example.com/clients/socodevi/laravel/co-up" + if created.RemoteURL != wantRemoteURL { + t.Fatalf("RemoteURL = %q, want %q", created.RemoteURL, wantRemoteURL) + } + // Self-hosted hosts are deliberately left untagged: only github.com and + // gitlab.com get a durable Provider/ProviderHost identity. + if created.Provider != "" { + t.Fatalf("Provider = %q, want empty for self-hosted host", created.Provider) + } + stored, err := repo.GetRepository(ctx, created.ID) + if err != nil { + t.Fatalf("GetRepository: %v", err) + } + if stored.RemoteURL != wantRemoteURL { + t.Fatalf("stored RemoteURL = %q, want %q", stored.RemoteURL, wantRemoteURL) + } +} + +// TestService_CreateRepositoryPrefersCanonicalCloneOriginForKnownProviders +// is a regression guard: for a local checkout whose origin is a well-known +// provider host (gitlab.com here), the RemoteURL fallback must resolve to +// canonicalCloneOrigin's exact ".git"-suffixed canonical form and not the +// broader ResolveGitRemoteIdentity-based form (which omits the suffix). +// Other code, including E2E test fixtures that rewrite Git's clone +// transport via an "insteadOf" config keyed on that exact canonical URL, +// depends on this byte-identical form to route the clone correctly. +func TestService_CreateRepositoryPrefersCanonicalCloneOriginForKnownProviders(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + if err := repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "Workspace"}); err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + + repoPath := t.TempDir() + makeRepo(t, repoPath) + config := `[core] + repositoryformatversion = 0 +[remote "origin"] + url = https://gitlab.com/fixture/docker-second-source.git + fetch = +refs/heads/*:refs/remotes/origin/* +` + if err := os.WriteFile(filepath.Join(repoPath, ".git", "config"), []byte(config), 0o644); err != nil { + t.Fatalf("write .git/config: %v", err) + } + + created, err := svc.CreateRepository(ctx, &CreateRepositoryRequest{ + WorkspaceID: "ws-1", + Name: "fixture/docker-second-source", + SourceType: sourceTypeLocal, + LocalPath: repoPath, + Provider: "gitlab", + ProviderHost: "https://gitlab.com", + ProviderOwner: "fixture", + ProviderName: "docker-second-source", + }) + if err != nil { + t.Fatalf("CreateRepository: %v", err) + } + const wantRemoteURL = "https://gitlab.com/fixture/docker-second-source.git" + if created.RemoteURL != wantRemoteURL { + t.Fatalf("RemoteURL = %q, want %q", created.RemoteURL, wantRemoteURL) + } +} + +// TestService_CreateRepositoryDoesNotOverwriteExplicitRemoteURL ensures the +// new local-discovery RemoteURL fallback only fills a gap and never clobbers +// a caller-supplied RemoteURL. +func TestService_CreateRepositoryDoesNotOverwriteExplicitRemoteURL(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + if err := repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "Workspace"}); err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + + repoPath := t.TempDir() + makeRepo(t, repoPath) + config := `[core] + repositoryformatversion = 0 +[remote "origin"] + url = git@gitlab.example.com:group/discovered-repo.git + fetch = +refs/heads/*:refs/remotes/origin/* +` + if err := os.WriteFile(filepath.Join(repoPath, ".git", "config"), []byte(config), 0o644); err != nil { + t.Fatalf("write .git/config: %v", err) + } + const explicitRemoteURL = "https://gitlab.example.com/group/explicit-repo" + + created, err := svc.CreateRepository(ctx, &CreateRepositoryRequest{ + WorkspaceID: "ws-1", + Name: "explicit-repo", + SourceType: sourceTypeLocal, + LocalPath: repoPath, + RemoteURL: explicitRemoteURL, + }) + if err != nil { + t.Fatalf("CreateRepository: %v", err) + } + if created.RemoteURL != explicitRemoteURL { + t.Fatalf("RemoteURL = %q, want unmodified explicit value %q", created.RemoteURL, explicitRemoteURL) + } +} + func TestService_CreateRepositoryRejectsInvalidLocalPathWithoutPersistence(t *testing.T) { svc, _, repo := createTestService(t) ctx := context.Background()