From b3e8fafbc37546dfd879c08921706b5f68e0d486 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Mon, 17 Aug 2026 08:40:33 +0000 Subject: [PATCH] test: pin managed credentials and persistence against SSH repository remotes (#2728) Co-authored-by: Kandev Agent --- .../executor_credentials_ssh_scope_test.go | 165 ++++++++++++++++++ .../sqlite/repository_ssh_remote_test.go | 160 +++++++++++++++++ .../task/service/service_ssh_remote_test.go | 122 +++++++++++++ 3 files changed, 447 insertions(+) create mode 100644 apps/backend/internal/orchestrator/executor/executor_credentials_ssh_scope_test.go create mode 100644 apps/backend/internal/task/repository/sqlite/repository_ssh_remote_test.go create mode 100644 apps/backend/internal/task/service/service_ssh_remote_test.go diff --git a/apps/backend/internal/orchestrator/executor/executor_credentials_ssh_scope_test.go b/apps/backend/internal/orchestrator/executor/executor_credentials_ssh_scope_test.go new file mode 100644 index 0000000000..3352ac57e2 --- /dev/null +++ b/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) + } +} diff --git a/apps/backend/internal/task/repository/sqlite/repository_ssh_remote_test.go b/apps/backend/internal/task/repository/sqlite/repository_ssh_remote_test.go new file mode 100644 index 0000000000..bd64962c88 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/apps/backend/internal/task/service/service_ssh_remote_test.go b/apps/backend/internal/task/service/service_ssh_remote_test.go new file mode 100644 index 0000000000..c23b34bd2c --- /dev/null +++ b/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) + } +}