test: pin managed credentials and persistence against SSH repository remotes (#2728)

Co-authored-by: Kandev Agent <agent@kandev.dev>

apps/backend/internal/orchestrator/executor/executor_credentials_ssh_scope_test.go

@@ -0,0 +1,165 @@
+package executor
+
+import (
+	"context"
+	"strings"
+	"testing"
+
+	"github.com/kandev/kandev/internal/gitcredentials"
+	"github.com/kandev/kandev/internal/task/models"
+)
+
+// sshRemoteScopeCase is one spelling a checkout cloned over SSH can leave in
+// repositories.remote_url. All four name the same repository, so all four must
+// resolve the same managed-credential scope. The `.git` suffix is deliberately
+// mixed: the persisted provider identity is authoritative, so the scope path is
+// the provider's canonical one regardless of how the remote was spelled. That
+// is what keeps the bare/suffixed split (#2675) out of the credential scope.
+type sshRemoteScopeCase struct {
+	name      string
+	remoteURL string
+}
+
+const sshRemoteScopePath = "/acme/widgets.git"
+
+func sshRemoteScopeCases() []sshRemoteScopeCase {
+	return []sshRemoteScopeCase{
+		{name: "scp style suffixed", remoteURL: "git@github.com:acme/widgets.git"},
+		{name: "scp style bare", remoteURL: "git@github.com:acme/widgets"},
+		{name: "ssh scheme suffixed", remoteURL: "ssh://git@github.com/acme/widgets.git"},
+		{name: "ssh scheme bare", remoteURL: "ssh://git@github.com/acme/widgets"},
+	}
+}
+
+// The regression in #2673 was not that a helper returned the wrong string: it
+// was that a repository whose remote_url holds an SSH URL, but which carries a
+// valid provider owner/name, stopped being issued a managed credential scope at
+// all. Assert the issued lease, not the URL rewrite, so a future refactor that
+// re-tightens the scheme check fails here rather than in production.
+func TestConfigureGitCredentialBrokerIssuesScopeForEverySSHRemoteSpelling(t *testing.T) {
+	for _, testCase := range sshRemoteScopeCases() {
+		t.Run(testCase.name, func(t *testing.T) {
+			issuer := &fakeGitHubCredentialLeaseIssuer{lease: gitcredentials.Lease{Token: "opaque-lease"}}
+			exec := newTestExecutor(t, &mockAgentManager{}, newMockRepository())
+			exec.SetGitHubCredentialBroker(issuer, "https://kandev.example/api/v1/github/credentials/resolve")
+			req := &LaunchAgentRequest{
+				TaskID: "task-1", WorkspaceID: "workspace-1", SessionID: "session-1",
+				ExecutorType: string(models.ExecutorTypeRemoteDocker), Env: map[string]string{},
+			}
+			info := &repoInfo{RepositoryID: "repo-1", Repository: &models.Repository{
+				SourceType: sourceTypeLocal, Provider: "github", ProviderOwner: "acme", ProviderName: "widgets",
+				RemoteURL: testCase.remoteURL,
+			}}
+
+			if err := exec.configureGitCredentialBrokerForRepositories(context.Background(), req, []*repoInfo{info}); err != nil {
+				t.Fatalf("configureGitCredentialBrokerForRepositories() error = %v", err)
+			}
+
+			if issuer.calls != 1 {
+				t.Fatalf("Issue calls = %d, want 1 lease issued for remote %q", issuer.calls, testCase.remoteURL)
+			}
+			if got := issuer.request; got.Host != "github.com" || got.Path != sshRemoteScopePath {
+				t.Fatalf("lease scope host/path = %q %q, want %q %q", got.Host, got.Path, "github.com", sshRemoteScopePath)
+			}
+			if got := req.Env[envGitHubCredentialOwner]; got != "acme" {
+				t.Fatalf("credential owner = %q, want acme", got)
+			}
+			if got := req.Env[envGitHubCredentialScopes]; !strings.Contains(got, `"path":"`+sshRemoteScopePath+`"`) {
+				t.Fatalf("credential scopes = %q, want HTTPS scope path %q", got, sshRemoteScopePath)
+			}
+		})
+	}
+}
+
+// The provider columns are the identity of record. #2117 moved this path onto
+// repositoryCloneURL, which reads remote_url first, and an SSH value there then
+// failed the HTTPS assertion. Pin that an SSH remote and its HTTPS twin produce
+// byte-identical scopes, so reading the transport-bearing column can never
+// again change the credential the launch receives.
+func TestGitCredentialScopeIsIndependentOfRemoteTransport(t *testing.T) {
+	scopeFor := func(t *testing.T, remoteURL string) string {
+		t.Helper()
+		issuer := &fakeGitHubCredentialLeaseIssuer{lease: gitcredentials.Lease{Token: "opaque-lease"}}
+		exec := newTestExecutor(t, &mockAgentManager{}, newMockRepository())
+		exec.SetGitHubCredentialBroker(issuer, "https://kandev.example/api/v1/github/credentials/resolve")
+		req := &LaunchAgentRequest{
+			TaskID: "task-1", WorkspaceID: "workspace-1", SessionID: "session-1",
+			ExecutorType: string(models.ExecutorTypeRemoteDocker), Env: map[string]string{},
+		}
+		info := &repoInfo{RepositoryID: "repo-1", Repository: &models.Repository{
+			SourceType: sourceTypeLocal, Provider: "github", ProviderOwner: "acme", ProviderName: "widgets",
+			RemoteURL: remoteURL,
+		}}
+		if err := exec.configureGitCredentialBrokerForRepositories(context.Background(), req, []*repoInfo{info}); err != nil {
+			t.Fatalf("configureGitCredentialBrokerForRepositories(%q) error = %v", remoteURL, err)
+		}
+		return req.Env[envGitHubCredentialScopes]
+	}
+
+	https := scopeFor(t, "https://github.com/acme/widgets.git")
+	// Driven from sshRemoteScopeCases() rather than a hand-listed subset. All
+	// four spellings agree today only because the provider identity wins; the
+	// fallback that rewrites the remote in place does not collapse the `.git`
+	// suffix, so a regression demoting the provider columns makes the bare forms
+	// diverge from the suffixed ones. A hand-listed pair of suffixed remotes
+	// cannot see that.
+	for _, testCase := range sshRemoteScopeCases() {
+		t.Run(testCase.name, func(t *testing.T) {
+			if got := scopeFor(t, testCase.remoteURL); got != https {
+				t.Fatalf("scopes for %q = %q, want the HTTPS-remote scopes %q", testCase.remoteURL, got, https)
+			}
+		})
+	}
+}
+
+// Accepting SSH remotes must not have widened which hosts a managed credential
+// can be minted for. A repository with no provider owner/name falls back to
+// rewriting its remote, and that rewrite is still checked against the provider
+// origin: an SSH remote pointing somewhere else is rejected before any lease is
+// issued, so a mis-backfilled remote_url cannot redirect a GitHub credential.
+func TestConfigureGitCredentialBrokerRejectsSSHRemoteFromForeignOrigin(t *testing.T) {
+	issuer := &fakeGitHubCredentialLeaseIssuer{lease: gitcredentials.Lease{Token: "opaque-lease"}}
+	exec := newTestExecutor(t, &mockAgentManager{}, newMockRepository())
+	exec.SetGitHubCredentialBroker(issuer, "https://kandev.example/api/v1/github/credentials/resolve")
+	req := &LaunchAgentRequest{
+		TaskID: "task-1", WorkspaceID: "workspace-1", SessionID: "session-1",
+		ExecutorType: string(models.ExecutorTypeRemoteDocker), Env: map[string]string{},
+	}
+	info := &repoInfo{RepositoryID: "repo-1", Repository: &models.Repository{
+		SourceType: sourceTypeLocal, Provider: "github",
+		RemoteURL: "git@evil.example:acme/widgets.git",
+	}}
+
+	err := exec.configureGitCredentialBrokerForRepositories(context.Background(), req, []*repoInfo{info})
+	if err == nil {
+		t.Fatalf("configureGitCredentialBrokerForRepositories() error = nil, want rejection of a foreign SSH origin")
+	}
+	if issuer.calls != 0 {
+		t.Fatalf("Issue calls = %d, want no lease issued for a foreign origin", issuer.calls)
+	}
+}
+
+// An SSH remote can carry a non-default SSH port, which names the SSH daemon
+// and not the HTTPS origin. The rewrite drops it, so a repository reachable at
+// ssh://…:2222 is still scoped to the provider's ordinary HTTPS origin rather
+// than to a port that would never match a credential helper.
+func TestConfigureGitCredentialBrokerDropsSSHPortFromScope(t *testing.T) {
+	issuer := &fakeGitHubCredentialLeaseIssuer{lease: gitcredentials.Lease{Token: "opaque-lease"}}
+	exec := newTestExecutor(t, &mockAgentManager{}, newMockRepository())
+	exec.SetGitHubCredentialBroker(issuer, "https://kandev.example/api/v1/github/credentials/resolve")
+	req := &LaunchAgentRequest{
+		TaskID: "task-1", WorkspaceID: "workspace-1", SessionID: "session-1",
+		ExecutorType: string(models.ExecutorTypeRemoteDocker), Env: map[string]string{},
+	}
+	info := &repoInfo{RepositoryID: "repo-1", Repository: &models.Repository{
+		SourceType: sourceTypeLocal, Provider: "github",
+		RemoteURL: "ssh://git@github.com:2222/acme/widgets.git",
+	}}
+
+	if err := exec.configureGitCredentialBrokerForRepositories(context.Background(), req, []*repoInfo{info}); err != nil {
+		t.Fatalf("configureGitCredentialBrokerForRepositories() error = %v", err)
+	}
+	if got := issuer.request; got.Host != "github.com" || got.Path != sshRemoteScopePath {
+		t.Fatalf("lease scope host/path = %q %q, want %q %q", got.Host, got.Path, "github.com", sshRemoteScopePath)
+	}
+}

apps/backend/internal/task/repository/sqlite/repository_ssh_remote_test.go

@@ -0,0 +1,160 @@
+package sqlite
+
+import (
+	"context"
+	"testing"
+
+	"github.com/kandev/kandev/internal/task/models"
+)
+
+// sshRemoteURLSpellings are the four shapes a checkout cloned over SSH leaves
+// in repositories.remote_url. Rows in this shape have existed in production for
+// months; every fixture feeding the credential and materialization paths was
+// HTTPS, which is how #2673 shipped. Persistence is where those rows come from,
+// so pin that the column survives the round trip verbatim.
+func sshRemoteURLSpellings() map[string]string {
+	return map[string]string{
+		"scp style suffixed":  "git@github.com:acme/widgets.git",
+		"scp style bare":      "git@github.com:acme/widgets",
+		"ssh scheme suffixed": "ssh://git@github.com/acme/widgets.git",
+		"ssh scheme bare":     "ssh://git@github.com/acme/widgets",
+	}
+}
+
+// An SSH remote_url must survive create -> get -> list unchanged. A rewrite or
+// a dropped column here would silently re-point every downstream consumer at a
+// different repository, or at none.
+func TestRepositorySSHRemoteURL_RoundTrip(t *testing.T) {
+	for name, remoteURL := range sshRemoteURLSpellings() {
+		t.Run(name, func(t *testing.T) {
+			repo := newRepoForEntityTests(t)
+			ctx := context.Background()
+			seedWorkspace(t, repo, "ws-ssh")
+
+			in := &models.Repository{
+				ID:            "repo-ssh",
+				WorkspaceID:   "ws-ssh",
+				Name:          "acme/widgets",
+				SourceType:    "provider",
+				Provider:      "github",
+				ProviderHost:  "https://github.com",
+				ProviderOwner: "acme",
+				ProviderName:  "widgets",
+				RemoteURL:     remoteURL,
+				DefaultBranch: "main",
+			}
+			if err := repo.CreateRepository(ctx, in); err != nil {
+				t.Fatalf("create repository: %v", err)
+			}
+
+			got, err := repo.GetRepository(ctx, in.ID)
+			if err != nil {
+				t.Fatalf("get repository: %v", err)
+			}
+			if got.RemoteURL != remoteURL {
+				t.Fatalf("GetRepository RemoteURL = %q, want %q", got.RemoteURL, remoteURL)
+			}
+			assertSSHRepositoryIdentityIntact(t, got, remoteURL)
+
+			list, err := repo.ListRepositories(ctx, "ws-ssh")
+			if err != nil {
+				t.Fatalf("list repositories: %v", err)
+			}
+			if len(list) != 1 {
+				t.Fatalf("ListRepositories returned %d rows, want 1", len(list))
+			}
+			if list[0].RemoteURL != remoteURL {
+				t.Fatalf("ListRepositories RemoteURL = %q, want %q", list[0].RemoteURL, remoteURL)
+			}
+			assertSSHRepositoryIdentityIntact(t, list[0], remoteURL)
+		})
+	}
+}
+
+// The provider columns are the identity a managed credential scope is derived
+// from, so they must still be readable alongside an SSH remote. This is the
+// exact combination #2673 proved was untested: SSH transport, valid provider
+// identity.
+func assertSSHRepositoryIdentityIntact(t *testing.T, got *models.Repository, remoteURL string) {
+	t.Helper()
+	if got.Provider != "github" || got.ProviderOwner != "acme" || got.ProviderName != "widgets" {
+		t.Fatalf("provider identity = %q %q/%q, want github acme/widgets (remote %q)",
+			got.Provider, got.ProviderOwner, got.ProviderName, remoteURL)
+	}
+	if got.ProviderHost != "https://github.com" {
+		t.Fatalf("provider host = %q, want https://github.com (remote %q)", got.ProviderHost, remoteURL)
+	}
+	if got.DefaultBranch != "main" {
+		t.Fatalf("default branch = %q, want main (remote %q)", got.DefaultBranch, remoteURL)
+	}
+}
+
+// Updating a repository must not drop or rewrite its SSH remote, and switching
+// transport in place must persist. Rows reach the SSH shape both ways: created
+// from an SSH checkout, or backfilled onto a row that started HTTPS.
+func TestRepositorySSHRemoteURL_UpdateRoundTrip(t *testing.T) {
+	repo := newRepoForEntityTests(t)
+	ctx := context.Background()
+	seedWorkspace(t, repo, "ws-ssh-update")
+
+	in := &models.Repository{
+		ID: "repo-ssh-update", WorkspaceID: "ws-ssh-update", Name: "acme/widgets",
+		SourceType: "provider", Provider: "github", ProviderHost: "https://github.com",
+		ProviderOwner: "acme", ProviderName: "widgets",
+		RemoteURL: "https://github.com/acme/widgets.git", DefaultBranch: "main",
+	}
+	if err := repo.CreateRepository(ctx, in); err != nil {
+		t.Fatalf("create repository: %v", err)
+	}
+
+	in.RemoteURL = "git@github.com:acme/widgets.git"
+	if err := repo.UpdateRepository(ctx, in); err != nil {
+		t.Fatalf("update repository: %v", err)
+	}
+
+	got, err := repo.GetRepository(ctx, in.ID)
+	if err != nil {
+		t.Fatalf("get repository: %v", err)
+	}
+	if got.RemoteURL != "git@github.com:acme/widgets.git" {
+		t.Fatalf("RemoteURL after update = %q, want the SSH remote", got.RemoteURL)
+	}
+	assertSSHRepositoryIdentityIntact(t, got, got.RemoteURL)
+}
+
+// Provider-identity lookup keys off the provider columns, never the remote. An
+// SSH-remote row must therefore still be found by the same identity as its
+// HTTPS twin, which is what lets find-or-create adopt it instead of creating a
+// duplicate every launch.
+func TestGetRepositoryByProviderIdentityFindsSSHRemoteRow(t *testing.T) {
+	for name, remoteURL := range sshRemoteURLSpellings() {
+		t.Run(name, func(t *testing.T) {
+			repo := newRepoForEntityTests(t)
+			ctx := context.Background()
+			seedWorkspace(t, repo, "ws-ssh-identity")
+
+			in := &models.Repository{
+				ID: "repo-ssh-identity", WorkspaceID: "ws-ssh-identity", Name: "acme/widgets",
+				SourceType: "provider", Provider: "github", ProviderHost: "https://github.com",
+				ProviderOwner: "acme", ProviderName: "widgets", RemoteURL: remoteURL,
+			}
+			if err := repo.CreateRepository(ctx, in); err != nil {
+				t.Fatalf("create repository: %v", err)
+			}
+
+			got, err := repo.GetRepositoryByProviderInfo(ctx, "ws-ssh-identity", "github", "https://github.com", "acme", "widgets")
+			if err != nil {
+				t.Fatalf("GetRepositoryByProviderInfo: %v", err)
+			}
+			if got == nil {
+				t.Fatalf("GetRepositoryByProviderInfo returned no row for SSH remote %q", remoteURL)
+			}
+			if got.ID != in.ID {
+				t.Fatalf("resolved repository = %q, want %q", got.ID, in.ID)
+			}
+			if got.RemoteURL != remoteURL {
+				t.Fatalf("resolved RemoteURL = %q, want %q", got.RemoteURL, remoteURL)
+			}
+		})
+	}
+}

apps/backend/internal/task/service/service_ssh_remote_test.go

@@ -0,0 +1,122 @@
+package service
+
+import (
+	"context"
+	"testing"
+
+	"github.com/kandev/kandev/internal/task/models"
+)
+
+// sshRemoteURLSpellings are the four shapes an SSH checkout leaves in
+// repositories.remote_url. Every fixture on this path was HTTPS before #2673,
+// so the service had never been driven with the data shape production holds.
+func sshRemoteURLSpellings() map[string]string {
+	return map[string]string{
+		"scp style suffixed":  "git@github.com:acme/widgets.git",
+		"scp style bare":      "git@github.com:acme/widgets",
+		"ssh scheme suffixed": "ssh://git@github.com/acme/widgets.git",
+		"ssh scheme bare":     "ssh://git@github.com/acme/widgets",
+	}
+}
+
+// Find-or-create must persist an SSH remote as given and, on a second call for
+// the same provider identity, adopt that row rather than create a duplicate.
+// Resolution keys off the provider columns, so the remote's transport must not
+// influence it.
+func TestService_FindOrCreateRepositoryPersistsAndAdoptsSSHRemote(t *testing.T) {
+	for name, remoteURL := range sshRemoteURLSpellings() {
+		t.Run(name, func(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)
+			}
+
+			created, wasCreated, err := svc.FindOrCreateRepository(ctx, &FindOrCreateRepositoryRequest{
+				WorkspaceID: "ws-1", Provider: "github", ProviderHost: "https://github.com",
+				ProviderOwner: "acme", ProviderName: "widgets", RemoteURL: remoteURL,
+			})
+			if err != nil || !wasCreated {
+				t.Fatalf("initial FindOrCreateRepository() = %+v, created=%t, err=%v", created, wasCreated, err)
+			}
+			if created.RemoteURL != remoteURL {
+				t.Fatalf("created RemoteURL = %q, want %q", created.RemoteURL, remoteURL)
+			}
+
+			// Deliberately the HTTPS spelling of the same repository. Resolution
+			// keys off the provider columns, so a later provider-driven request
+			// carrying the HTTPS URL must adopt this row rather than create a
+			// duplicate. Passing the SSH URL back would also pass against an
+			// implementation that matched on exact remote_url.
+			resolved, duplicateCreated, err := svc.FindOrCreateRepository(ctx, &FindOrCreateRepositoryRequest{
+				WorkspaceID: "ws-1", Provider: "github", ProviderHost: "https://github.com",
+				ProviderOwner: "acme", ProviderName: "widgets",
+				RemoteURL: "https://github.com/acme/widgets.git",
+			})
+			if err != nil {
+				t.Fatalf("second FindOrCreateRepository() error = %v", err)
+			}
+			if duplicateCreated || resolved.ID != created.ID {
+				t.Fatalf("second call = %q (created=%t), want existing %q", resolved.ID, duplicateCreated, created.ID)
+			}
+			// The row already had a remote, so the HTTPS spelling must not clobber
+			// the stored SSH one: backfill only fills an empty column.
+			if resolved.RemoteURL != remoteURL {
+				t.Fatalf("resolved RemoteURL = %q, want the stored SSH remote %q", resolved.RemoteURL, remoteURL)
+			}
+
+			stored, err := repo.GetRepository(ctx, created.ID)
+			if err != nil {
+				t.Fatalf("GetRepository: %v", err)
+			}
+			if stored.RemoteURL != remoteURL {
+				t.Fatalf("stored RemoteURL = %q, want %q", stored.RemoteURL, remoteURL)
+			}
+		})
+	}
+}
+
+// A row created without a remote is backfilled by a later call that carries
+// one. An SSH remote must be accepted by that backfill: this is precisely how
+// the production rows behind #2673 acquired their SSH remote_url.
+func TestService_FindOrCreateRepositoryBackfillsSSHRemoteOntoExistingRow(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)
+	}
+
+	created, err := svc.CreateRepository(ctx, &CreateRepositoryRequest{
+		WorkspaceID: "ws-1", Name: "acme/widgets", SourceType: "provider",
+		Provider: "github", ProviderHost: "https://github.com",
+		ProviderOwner: "acme", ProviderName: "widgets",
+	})
+	if err != nil {
+		t.Fatalf("CreateRepository: %v", err)
+	}
+	if created.RemoteURL != "" {
+		t.Fatalf("precondition: created RemoteURL = %q, want empty", created.RemoteURL)
+	}
+
+	resolved, wasCreated, err := svc.FindOrCreateRepository(ctx, &FindOrCreateRepositoryRequest{
+		WorkspaceID: "ws-1", Provider: "github", ProviderHost: "https://github.com",
+		ProviderOwner: "acme", ProviderName: "widgets", RemoteURL: "git@github.com:acme/widgets.git",
+	})
+	if err != nil {
+		t.Fatalf("FindOrCreateRepository: %v", err)
+	}
+	if wasCreated || resolved.ID != created.ID {
+		t.Fatalf("resolved = %q (created=%t), want existing %q", resolved.ID, wasCreated, created.ID)
+	}
+
+	stored, err := repo.GetRepository(ctx, created.ID)
+	if err != nil {
+		t.Fatalf("GetRepository: %v", err)
+	}
+	if stored.RemoteURL != "git@github.com:acme/widgets.git" {
+		t.Fatalf("backfilled RemoteURL = %q, want the SSH remote", stored.RemoteURL)
+	}
+	if stored.ProviderOwner != "acme" || stored.ProviderName != "widgets" {
+		t.Fatalf("provider identity after backfill = %q/%q, want acme/widgets", stored.ProviderOwner, stored.ProviderName)
+	}
+}