From 9b99b7d1fdf0690c22165082a76a2471dd71cbfd Mon Sep 17 00:00:00 2001 From: Alassane <79718216+yattdev@users.noreply.github.com> Date: Wed, 19 Aug 2026 02:50:20 -0400 Subject: [PATCH] fix(backend): validate managed Git credential identity before session launch (#2787) Co-authored-by: ayattara Co-authored-by: Carlos Florencio --- .../internal/backendapp/git_credentials.go | 64 +--- .../internal/gitcredentials/identity.go | 170 +++++++++++ .../internal/gitcredentials/identity_test.go | 123 ++++++++ .../orchestrator/event_handlers_workflow.go | 76 ++++- ...andlers_workflow_profile_preflight_test.go | 280 ++++++++++++++++++ .../executor/executor_credentials.go | 151 ++++++---- .../executor_credentials_preflight_test.go | 259 ++++++++++++++++ .../orchestrator/executor/executor_execute.go | 10 + .../orchestrator/executor/executor_office.go | 5 + .../executor/executor_office_test.go | 51 ++++ ...ase_migrations_repository_identity_test.go | 45 +++ .../plan.md | 140 +++++++++ .../task-01-harden-repository-identity.md | 80 +++++ .../task-02-centralize-session-admission.md | 92 ++++++ .../task-03-remove-unsafe-origin-backfill.md | 79 +++++ .../integrations/github-authentication.md | 29 ++ 16 files changed, 1539 insertions(+), 115 deletions(-) create mode 100644 apps/backend/internal/gitcredentials/identity.go create mode 100644 apps/backend/internal/gitcredentials/identity_test.go create mode 100644 apps/backend/internal/orchestrator/event_handlers_workflow_profile_preflight_test.go create mode 100644 apps/backend/internal/orchestrator/executor/executor_credentials_preflight_test.go create mode 100644 apps/backend/internal/task/repository/sqlite/base_migrations_repository_identity_test.go create mode 100644 docs/plans/managed-git-credential-admission-repair/plan.md create mode 100644 docs/plans/managed-git-credential-admission-repair/task-01-harden-repository-identity.md create mode 100644 docs/plans/managed-git-credential-admission-repair/task-02-centralize-session-admission.md create mode 100644 docs/plans/managed-git-credential-admission-repair/task-03-remove-unsafe-origin-backfill.md diff --git a/apps/backend/internal/backendapp/git_credentials.go b/apps/backend/internal/backendapp/git_credentials.go index a20a1f8f70..1e43640d43 100644 --- a/apps/backend/internal/backendapp/git_credentials.go +++ b/apps/backend/internal/backendapp/git_credentials.go @@ -257,64 +257,12 @@ func repositoryHTTPSIdentity(repository *taskmodels.Repository) (string, string, if repository == nil { return "", "", fmt.Errorf("repository is required") } - remoteURL := repositoryHTTPSCloneURL(repository) - parsed, err := parseRepositoryHTTPSCloneURL(remoteURL) + identity, err := gitcredentials.ResolveRepositoryIdentity(gitcredentials.RepositoryIdentityInput{ + RepositoryID: repository.ID, Provider: repository.Provider, ProviderHost: repository.ProviderHost, + ProviderOwner: repository.ProviderOwner, ProviderName: repository.ProviderName, RemoteURL: repository.RemoteURL, + }) if err != nil { - return "", "", fmt.Errorf("repository HTTPS clone URL is unavailable") + return "", "", err } - providerHost := repositoryProviderOrigin(repository) - if err := repoclone.ValidateHTTPSCloneOrigin(remoteURL, providerHost); err != nil { - return "", "", fmt.Errorf("repository provider origin: %w", err) - } - return strings.ToLower(parsed.Host), parsed.Path, nil -} - -// repositoryHTTPSCloneURL mirrors the executor's credentialIdentityCloneURL: -// an HTTPS remote wins, then the persisted provider identity whose host carries -// the real HTTPS origin, and only then a rewrite of a non-HTTPS remote. The two -// must agree, because one issues the lease scope and the other authorizes it. -func repositoryHTTPSCloneURL(repository *taskmodels.Repository) string { - remoteURL := strings.TrimSpace(repository.RemoteURL) - if strings.HasPrefix(strings.ToLower(remoteURL), "https://") { - return remoteURL - } - if derived := providerHTTPSCloneURL(repository); derived != "" { - return derived - } - if converted := repoclone.CanonicalHTTPSCloneURL(remoteURL); converted != "" { - return converted - } - return remoteURL -} - -func providerHTTPSCloneURL(repository *taskmodels.Repository) string { - if !strings.EqualFold(repository.Provider, gitCredentialGitHubProviderID) || - repository.ProviderOwner == "" || repository.ProviderName == "" { - return "" - } - cloneURL, err := repoclone.CloneURLWithHost( - repository.Provider, repository.ProviderHost, - repository.ProviderOwner, repository.ProviderName, repoclone.ProtocolHTTPS, - ) - if err != nil { - return "" - } - return cloneURL -} - -func parseRepositoryHTTPSCloneURL(remoteURL string) (*url.URL, error) { - parsed, err := url.Parse(remoteURL) - if err != nil || parsed.Scheme != "https" || parsed.Host == "" || parsed.User != nil || - parsed.RawQuery != "" || parsed.Fragment != "" || parsed.Path == "" { - return nil, fmt.Errorf("invalid HTTPS clone URL") - } - return parsed, nil -} - -func repositoryProviderOrigin(repository *taskmodels.Repository) string { - providerHost := strings.TrimSpace(repository.ProviderHost) - if providerHost != "" || (repository.Provider != "" && !strings.EqualFold(repository.Provider, gitCredentialGitHubProviderID)) { - return providerHost - } - return "https://" + gitCredentialGitHubHost + return identity.Host, identity.Path, nil } diff --git a/apps/backend/internal/gitcredentials/identity.go b/apps/backend/internal/gitcredentials/identity.go new file mode 100644 index 0000000000..140d5d754e --- /dev/null +++ b/apps/backend/internal/gitcredentials/identity.go @@ -0,0 +1,170 @@ +package gitcredentials + +import ( + "fmt" + "net/url" + "strings" + + "github.com/kandev/kandev/internal/repoclone" +) + +const publicGitHubHost = "github.com" + +// RepositoryIdentityInput is the non-secret persisted repository metadata used +// to authorize a managed Git credential scope. Provider remains routing +// metadata; ProviderHost alone establishes the authorized HTTPS origin. +type RepositoryIdentityInput struct { + RepositoryID string + Provider string + ProviderHost string + ProviderOwner string + ProviderName string + RemoteURL string +} + +// RepositoryIdentity is the canonical HTTPS identity used by issuers and the +// credential broker. +type RepositoryIdentity struct { + Host string + Path string + Owner string + Repository string +} + +func ResolveRepositoryIdentity(input RepositoryIdentityInput) (RepositoryIdentity, error) { + remoteURL := strings.TrimSpace(input.RemoteURL) + if remoteURL == "" { + if input.ProviderOwner == "" || input.ProviderName == "" { + return RepositoryIdentity{}, repositoryIdentityError(input.RepositoryID, "has no clone URL") + } + origin, err := trustedOrigin(input, "") + if err != nil { + return RepositoryIdentity{}, err + } + remoteURL = origin + "/" + strings.Trim(input.ProviderOwner, "/") + "/" + strings.Trim(input.ProviderName, "/") + ".git" + } + + remote, err := parseRemote(remoteURL) + if err != nil { + return RepositoryIdentity{}, repositoryIdentityError(input.RepositoryID, "must use a credential-free HTTPS or SSH clone URL") + } + return resolveParsedRepositoryIdentity(input, remote) +} + +func resolveParsedRepositoryIdentity(input RepositoryIdentityInput, remote parsedRemote) (RepositoryIdentity, error) { + origin, err := trustedOrigin(input, remote.hostname) + if err != nil { + return RepositoryIdentity{}, err + } + originURL, _ := url.Parse(origin) + if remote.isHTTPS { + if err := repoclone.ValidateHTTPSCloneOrigin(remote.canonicalURL, origin); err != nil { + return RepositoryIdentity{}, repositoryIdentityError(input.RepositoryID, "remote host conflicts with its provider origin") + } + } else if !strings.EqualFold(remote.hostname, originURL.Hostname()) { + return RepositoryIdentity{}, repositoryIdentityError(input.RepositoryID, "remote host conflicts with its provider origin") + } + + canonicalPath, err := canonicalIdentityPath(input, remote.path) + if err != nil { + return RepositoryIdentity{}, err + } + owner, repository, err := identityPath(canonicalPath) + if err != nil { + return RepositoryIdentity{}, repositoryIdentityError(input.RepositoryID, "has an invalid clone path") + } + return RepositoryIdentity{Host: originURL.Host, Path: canonicalPath, Owner: owner, Repository: repository}, nil +} + +func canonicalIdentityPath(input RepositoryIdentityInput, remotePath string) (string, error) { + if !strings.EqualFold(strings.TrimSpace(input.Provider), "github") || input.ProviderOwner == "" || input.ProviderName == "" { + return remotePath, nil + } + remoteOwner, remoteRepository, err := identityPath(remotePath) + if err != nil || !strings.EqualFold(remoteOwner, strings.TrimSpace(input.ProviderOwner)) || + !strings.EqualFold(strings.TrimSuffix(remoteRepository, ".git"), strings.TrimSuffix(strings.TrimSpace(input.ProviderName), ".git")) { + return "", repositoryIdentityError(input.RepositoryID, "remote path conflicts with its provider identity") + } + return "/" + strings.Trim(input.ProviderOwner, "/") + "/" + strings.Trim(input.ProviderName, "/") + ".git", nil +} + +type parsedRemote struct { + hostname string + path string + canonicalURL string + isHTTPS bool +} + +func parseRemote(raw string) (parsedRemote, error) { + value := strings.TrimSpace(raw) + canonical := repoclone.CanonicalHTTPSCloneURL(value) + if canonical != "" { + if err := validateSSHRemote(value); err != nil { + return parsedRemote{}, err + } + parsed, err := url.Parse(canonical) + if err != nil || parsed.Host == "" || parsed.Path == "" { + return parsedRemote{}, fmt.Errorf("invalid SSH clone URL") + } + return parsedRemote{hostname: parsed.Hostname(), path: parsed.Path, canonicalURL: canonical}, nil + } + parsed, err := url.Parse(value) + if err != nil || !strings.EqualFold(parsed.Scheme, "https") || parsed.Host == "" || parsed.User != nil || + parsed.RawQuery != "" || parsed.ForceQuery || parsed.Fragment != "" || parsed.Path == "" { + return parsedRemote{}, fmt.Errorf("invalid HTTPS clone URL") + } + return parsedRemote{hostname: parsed.Hostname(), path: parsed.Path, canonicalURL: "https://" + strings.ToLower(parsed.Host) + parsed.Path, isHTTPS: true}, nil +} + +func validateSSHRemote(value string) error { + if !strings.Contains(value, "://") { + if strings.ContainsAny(value, "?#") { + return fmt.Errorf("invalid SSH clone URL") + } + return nil + } + parsed, err := url.Parse(value) + if err != nil || !strings.EqualFold(parsed.Scheme, "ssh") || parsed.Hostname() == "" || + parsed.Path == "" || parsed.Path == "/" || parsed.RawQuery != "" || parsed.ForceQuery || parsed.Fragment != "" { + return fmt.Errorf("invalid SSH clone URL") + } + if parsed.User != nil { + if _, hasPassword := parsed.User.Password(); hasPassword { + return fmt.Errorf("invalid SSH clone URL") + } + } + return nil +} + +func trustedOrigin(input RepositoryIdentityInput, remoteHost string) (string, error) { + if host := strings.TrimSpace(input.ProviderHost); host != "" { + origin, err := repoclone.HTTPSProviderOrigin(host) + if err != nil { + return "", repositoryIdentityError(input.RepositoryID, "has an invalid provider host") + } + return origin, nil + } + if strings.EqualFold(remoteHost, publicGitHubHost) { + return "https://" + publicGitHubHost, nil + } + if strings.EqualFold(strings.TrimSpace(input.Provider), "github") && remoteHost == "" { + return "https://" + publicGitHubHost, nil + } + return "", repositoryIdentityError(input.RepositoryID, "needs a provider host for managed credentials") +} + +func identityPath(path string) (string, string, error) { + trimmed := strings.TrimSuffix(strings.Trim(path, "/"), ".git") + owner, repository, found := strings.Cut(trimmed, "/") + if !found || owner == "" || repository == "" { + return "", "", fmt.Errorf("path needs owner and repository") + } + return owner, repository, nil +} + +func repositoryIdentityError(repositoryID, detail string) error { + if strings.TrimSpace(repositoryID) == "" { + return fmt.Errorf("repository identity %s", detail) + } + return fmt.Errorf("repository %q %s; repair its clone URL or provider host", repositoryID, detail) +} diff --git a/apps/backend/internal/gitcredentials/identity_test.go b/apps/backend/internal/gitcredentials/identity_test.go new file mode 100644 index 0000000000..86f5497a3c --- /dev/null +++ b/apps/backend/internal/gitcredentials/identity_test.go @@ -0,0 +1,123 @@ +package gitcredentials + +import ( + "strings" + "testing" +) + +func TestResolveRepositoryIdentityAcceptsGitHubTransportSpellingsWithoutProviderHost(t *testing.T) { + for _, provider := range []string{"", "plugin-provider", "kandev-plugin-tags"} { + for _, remoteURL := range []string{ + "git@github.com:acme/widgets.git", + "ssh://git@github.com/acme/widgets.git", + "https://github.com/acme/widgets.git", + } { + t.Run(provider+"/"+remoteURL, func(t *testing.T) { + identity, err := ResolveRepositoryIdentity(RepositoryIdentityInput{ + RepositoryID: "repo-1", Provider: provider, RemoteURL: remoteURL, + }) + if err != nil { + t.Fatalf("ResolveRepositoryIdentity() error = %v", err) + } + if identity.Host != "github.com" || identity.Path != "/acme/widgets.git" { + t.Fatalf("identity = %#v, want github.com /acme/widgets.git", identity) + } + }) + } + } +} + +func TestResolveRepositoryIdentityUsesExplicitProviderHostForHTTPS(t *testing.T) { + identity, err := ResolveRepositoryIdentity(RepositoryIdentityInput{ + RepositoryID: "repo-1", Provider: "acme-forge", ProviderHost: "https://forge.example:9443", + RemoteURL: "https://forge.example:9443/acme/widgets.git", + }) + if err != nil { + t.Fatalf("ResolveRepositoryIdentity() error = %v", err) + } + if identity.Host != "forge.example:9443" || identity.Path != "/acme/widgets.git" { + t.Fatalf("identity = %#v, want forge.example:9443 /acme/widgets.git", identity) + } +} + +// A custom host with no ProviderHost metadata must fail closed even when +// Provider names a would-be resolver and owner/name are populated: only an +// explicit provider host - or the well-known public github.com - is a +// trusted origin. +func TestResolveRepositoryIdentityRequiresExplicitProviderHostForNonGitHubProvider(t *testing.T) { + _, err := ResolveRepositoryIdentity(RepositoryIdentityInput{ + RepositoryID: "repo-1", Provider: "acme-forge", ProviderOwner: "acme", ProviderName: "widgets", + RemoteURL: "https://forge.example/acme/widgets.git", + }) + if err == nil { + t.Fatal("ResolveRepositoryIdentity() error = nil, want rejection of an untrusted custom host") + } +} + +// A GitHub row whose persisted owner/name conflict with its remote's actual +// path must be rejected even though the remote itself resolves to a trusted +// github.com origin - populated provider identity fields must not silently +// win over a mismatched remote. +func TestResolveRepositoryIdentityRejectsGitHubOwnerNameConflictWithRemotePath(t *testing.T) { + _, err := ResolveRepositoryIdentity(RepositoryIdentityInput{ + RepositoryID: "repo-1", Provider: "github", ProviderOwner: "acme", ProviderName: "widgets", + RemoteURL: "https://github.com/acme/other-repo.git", + }) + if err == nil { + t.Fatal("ResolveRepositoryIdentity() error = nil, want rejection of a provider/remote path conflict") + } +} + +// Credential-bearing and query/fragment-bearing URLs must be rejected, and +// the rejection must never echo the credential-bearing URL back - only the +// safe repository ID and a generic remediation. +func TestResolveRepositoryIdentityRejectsCredentialBearingAndMalformedURLsWithoutLeakingSecrets(t *testing.T) { + for name, remoteURL := range map[string]string{ + "https userinfo": "https://oauth2:sh_supersecret@github.com/acme/widgets.git", + "https query": "https://github.com/acme/widgets.git?token=sh_supersecret", + "https fragment": "https://github.com/acme/widgets.git#sh_supersecret", + "ssh password": "ssh://git:sh_supersecret@github.com/acme/widgets.git", + "ssh query": "ssh://git@github.com/acme/widgets.git?token=sh_supersecret", + "ssh fragment": "ssh://git@github.com/acme/widgets.git#sh_supersecret", + "scp-style query": "git@github.com:acme/widgets.git?token=sh_supersecret", + "scp-style fragment": "git@github.com:acme/widgets.git#sh_supersecret", + "malformed": "not-a-valid-url", + } { + t.Run(name, func(t *testing.T) { + _, err := ResolveRepositoryIdentity(RepositoryIdentityInput{RepositoryID: "repo-1", RemoteURL: remoteURL}) + if err == nil { + t.Fatalf("ResolveRepositoryIdentity(%q) error = nil, want rejection", remoteURL) + } + if strings.Contains(err.Error(), "sh_supersecret") { + t.Fatalf("error leaked the credential-bearing URL: %v", err) + } + if strings.Contains(err.Error(), remoteURL) { + t.Fatalf("error leaked the rejected remote: %v", err) + } + }) + } +} + +func TestResolveRepositoryIdentityUsesExplicitProviderHostForSSH(t *testing.T) { + identity, err := ResolveRepositoryIdentity(RepositoryIdentityInput{ + RepositoryID: "repo-1", ProviderHost: "https://ghe.example:8443", + RemoteURL: "ssh://git@ghe.example:2222/acme/widgets.git", + }) + if err != nil { + t.Fatalf("ResolveRepositoryIdentity() error = %v", err) + } + if identity.Host != "ghe.example:8443" || identity.Path != "/acme/widgets.git" { + t.Fatalf("identity = %#v, want ghe.example:8443 /acme/widgets.git", identity) + } +} + +func TestResolveRepositoryIdentityRejectsUntrustedOrConflictingOrigin(t *testing.T) { + for _, input := range []RepositoryIdentityInput{ + {RepositoryID: "repo-1", RemoteURL: "ssh://git@ghe.example/acme/widgets.git"}, + {RepositoryID: "repo-1", ProviderHost: "https://github.com", RemoteURL: "https://evil.example/acme/widgets.git"}, + } { + if _, err := ResolveRepositoryIdentity(input); err == nil { + t.Fatalf("ResolveRepositoryIdentity(%#v) error = nil, want rejection", input) + } + } +} diff --git a/apps/backend/internal/orchestrator/event_handlers_workflow.go b/apps/backend/internal/orchestrator/event_handlers_workflow.go index 54d7c08904..b879b6f752 100644 --- a/apps/backend/internal/orchestrator/event_handlers_workflow.go +++ b/apps/backend/internal/orchestrator/event_handlers_workflow.go @@ -1696,13 +1696,6 @@ func (s *Service) switchSessionForStep(ctx context.Context, taskID string, curre zap.String("current_session", currentSession.ID), zap.String("current_profile", currentSession.AgentProfileID), zap.String("new_profile", newAgentProfileID)) - - // Signal to the frontend that the task is preparing a new agent. - if err := s.taskRepo.UpdateTaskState(ctx, taskID, v1.TaskStateScheduling); err != nil { - s.logger.Warn("failed to set task SCHEDULING during agent switch", - zap.String("task_id", taskID), zap.Error(err)) - } - existing, lookupErr := s.findReusableSessionForProfile(ctx, taskID, newAgentProfileID, currentSession.ID) if lookupErr != nil { s.logger.Warn("failed to look up reusable session, falling through to create new", @@ -1710,6 +1703,35 @@ func (s *Service) switchSessionForStep(ctx context.Context, taskID string, curre zap.String("agent_profile_id", newAgentProfileID), zap.Error(lookupErr)) } + targetSession := currentSession + if existing != nil { + targetSession = existing + } + + // Validate managed-credential repository bindings before mutating either + // session's ownership. Reusing or replacing the current session only to + // discover at launch that a repository binding is irreparable would leave + // the task stuck on a doomed session with no recovery path back to the one + // that was still working. + dbTask, err := s.repo.GetTask(ctx, taskID) + if err != nil { + return nil, fmt.Errorf("failed to get task for session switch preflight: %w", err) + } + if dbTask == nil { + return nil, fmt.Errorf("task %s not found for session switch preflight", taskID) + } + if err := s.executor.PreflightManagedGitCredentials( + ctx, dbTask.WorkspaceID, taskID, targetSession.ExecutorID, targetSession.ExecutorProfileID, + ); err != nil { + return nil, err + } + + // Signal to the frontend that the task is preparing a new agent. + if err := s.taskRepo.UpdateTaskState(ctx, taskID, v1.TaskStateScheduling); err != nil { + s.logger.Warn("failed to set task SCHEDULING during agent switch", + zap.String("task_id", taskID), zap.Error(err)) + } + if existing != nil { return s.reuseSessionForStep(ctx, taskID, currentSession, existing) } @@ -1973,6 +1995,39 @@ func (s *Service) prepareWorkflowStepSession( return newSession, true, nil } +func (s *Service) preflightWorkflowStepCredentials( + ctx context.Context, + taskID string, + currentSession *models.TaskSession, + targetStep *wfmodels.WorkflowStep, +) error { + if currentSession == nil || targetStep == nil || s.agentManager.IsPassthroughSession(ctx, currentSession.ID) { + return nil + } + effectiveProfile := s.resolveStepAgentProfile(ctx, targetStep) + if effectiveProfile == "" || effectiveProfile == currentSession.AgentProfileID { + return nil + } + targetSession := currentSession + existing, err := s.findReusableSessionForProfile(ctx, taskID, effectiveProfile, currentSession.ID) + if err != nil { + return fmt.Errorf("find reusable session for credential preflight: %w", err) + } + if existing != nil { + targetSession = existing + } + task, err := s.repo.GetTask(ctx, taskID) + if err != nil { + return fmt.Errorf("get task for credential preflight: %w", err) + } + if task == nil { + return fmt.Errorf("task %s not found for credential preflight", taskID) + } + return s.executor.PreflightManagedGitCredentials( + ctx, task.WorkspaceID, taskID, targetSession.ExecutorID, targetSession.ExecutorProfileID, + ) +} + // maybySwitchSessionForProfile preserves the legacy processOnEnter failure // handling while sharing the error-returning step-entry preflight with direct // workflow-engine dispatch. @@ -3818,6 +3873,13 @@ func (s *Service) applyEngineTransition( return false } } + if err := s.preflightWorkflowStepCredentials(ctx, taskID, session, targetStep); err != nil { + s.logger.Warn("target profile credential preflight failed, skipping transition", + zap.String("task_id", taskID), + zap.String("step_id", result.ToStepID), + zap.Error(err)) + return false + } terminalTarget := s.workflowStepIsTerminal(ctx, targetStep.ID) diff --git a/apps/backend/internal/orchestrator/event_handlers_workflow_profile_preflight_test.go b/apps/backend/internal/orchestrator/event_handlers_workflow_profile_preflight_test.go new file mode 100644 index 0000000000..6747ec6931 --- /dev/null +++ b/apps/backend/internal/orchestrator/event_handlers_workflow_profile_preflight_test.go @@ -0,0 +1,280 @@ +package orchestrator + +import ( + "context" + "strings" + "testing" + "time" + + "github.com/kandev/kandev/internal/gitcredentials" + "github.com/kandev/kandev/internal/orchestrator/executor" + "github.com/kandev/kandev/internal/orchestrator/messagequeue" + "github.com/kandev/kandev/internal/orchestrator/queue" + "github.com/kandev/kandev/internal/orchestrator/scheduler" + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/workflow/engine" + wfmodels "github.com/kandev/kandev/internal/workflow/models" + v1 "github.com/kandev/kandev/pkg/api/v1" +) + +type fakeSwitchSessionCredentialIssuer struct{} + +func (fakeSwitchSessionCredentialIssuer) Issue( + context.Context, gitcredentials.Scope, +) (gitcredentials.Lease, error) { + return gitcredentials.Lease{Token: "opaque-lease"}, nil +} + +// TestSwitchSessionForStep_RejectsIrreparableManagedCredentialRepository +// covers AC-8/AC-9 at the profile-switch/re-entry seam: a task bound to a +// repository whose managed Git credential identity is irreparable (a custom +// host with no trustworthy provider host metadata) must reject the switch +// before mutating either session's ownership - not stop the still-working +// current session only to hand the task to a session that can never launch. +func TestSwitchSessionForStep_RejectsIrreparableManagedCredentialRepository(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + now := time.Now().UTC() + + ws := &models.Workspace{ID: "ws1", Name: "Test", CreatedAt: now, UpdatedAt: now} + if err := repo.CreateWorkspace(ctx, ws); err != nil { + t.Fatalf("create workspace: %v", err) + } + wf := &models.Workflow{ID: "wf1", WorkspaceID: "ws1", Name: "WF", CreatedAt: now, UpdatedAt: now} + if err := repo.CreateWorkflow(ctx, wf); err != nil { + t.Fatalf("create workflow: %v", err) + } + task := &models.Task{ + ID: "t1", WorkspaceID: "ws1", WorkflowID: "wf1", WorkflowStepID: "step2", + Title: "Test", Description: "Test", State: v1.TaskStateInProgress, + CreatedAt: now, UpdatedAt: now, + } + if err := repo.CreateTask(ctx, task); err != nil { + t.Fatalf("create task: %v", err) + } + + repository := &models.Repository{ + ID: "repo1", WorkspaceID: "ws1", Name: "widgets", SourceType: "local", + Provider: "acme-forge", RemoteURL: "https://forge.example/acme/widgets.git", + } + if err := repo.CreateRepository(ctx, repository); err != nil { + t.Fatalf("create repository: %v", err) + } + if err := repo.CreateTaskRepository(ctx, &models.TaskRepository{ + ID: "taskrepo1", TaskID: "t1", RepositoryID: "repo1", + }); err != nil { + t.Fatalf("create task repository: %v", err) + } + + session := &models.TaskSession{ + ID: "s1", TaskID: "t1", AgentProfileID: "profile-a", + ExecutorID: "exec-local", ExecutorProfileID: "ep1", AgentExecutionID: "ae1", + State: models.TaskSessionStateRunning, IsPrimary: true, + StartedAt: now, UpdatedAt: now, + } + if err := repo.CreateTaskSession(ctx, session); err != nil { + t.Fatalf("create task session: %v", err) + } + + taskRepo := newMockTaskRepo() + taskRepo.tasks["t1"] = &v1.Task{ + ID: "t1", WorkspaceID: "ws1", WorkflowID: "wf1", + Title: "Test", Description: "Test", State: v1.TaskStateInProgress, + } + + agentMgr := &mockAgentManager{repoForExecutionLookup: repo} + log := testLogger() + exec := executor.NewExecutor(agentMgr, repo, log, executor.ExecutorConfig{}) + exec.SetGitHubCredentialBroker(fakeSwitchSessionCredentialIssuer{}, "https://kandev.example/api/v1/github/credentials/resolve") + sched := scheduler.NewScheduler(queue.NewTaskQueue(100), exec, taskRepo, log, scheduler.SchedulerConfig{}) + svc := &Service{ + logger: log, + repo: repo, + workflowStepGetter: newMockStepGetter(), + taskRepo: taskRepo, + agentManager: agentMgr, + messageQueue: messagequeue.NewServiceMemory(log), + executor: exec, + scheduler: sched, + } + + _, err := svc.switchSessionForStep(ctx, "t1", session, "profile-b") + if err == nil { + t.Fatal("switchSessionForStep() error = nil, want rejection of the irreparable repository binding") + } + if !strings.Contains(err.Error(), "repo1") { + t.Fatalf("error %q does not name the safe repository id repo1", err.Error()) + } + + // The current session must be untouched: still RUNNING and still primary. + oldSession, getErr := repo.GetTaskSession(ctx, "s1") + if getErr != nil { + t.Fatalf("get old session: %v", getErr) + } + if oldSession.State != models.TaskSessionStateRunning { + t.Errorf("old session state = %s, want RUNNING (must not be touched by a rejected switch)", oldSession.State) + } + if !oldSession.IsPrimary { + t.Error("old session must still be primary after a rejected switch") + } + + // No replacement session for profile-b was created. + sessions, listErr := repo.ListTaskSessions(ctx, "t1") + if listErr != nil { + t.Fatalf("list task sessions: %v", listErr) + } + if len(sessions) != 1 { + t.Fatalf("task session count = %d, want 1 (no doomed replacement session)", len(sessions)) + } +} + +func TestApplyEngineTransitionRejectsTargetProfileBeforePersistingStep(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + now := time.Now().UTC() + + if err := repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws1", Name: "Test", CreatedAt: now, UpdatedAt: now}); err != nil { + t.Fatalf("create workspace: %v", err) + } + if err := repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf1", WorkspaceID: "ws1", Name: "WF", CreatedAt: now, UpdatedAt: now}); err != nil { + t.Fatalf("create workflow: %v", err) + } + if err := repo.CreateTask(ctx, &models.Task{ + ID: "t1", WorkspaceID: "ws1", WorkflowID: "wf1", WorkflowStepID: "step1", + Title: "Test", Description: "Test", State: v1.TaskStateInProgress, CreatedAt: now, UpdatedAt: now, + }); err != nil { + t.Fatalf("create task: %v", err) + } + if err := repo.CreateRepository(ctx, &models.Repository{ + ID: "repo1", WorkspaceID: "ws1", Name: "widgets", SourceType: "local", + Provider: "acme-forge", RemoteURL: "https://forge.example/acme/widgets.git", + }); err != nil { + t.Fatalf("create repository: %v", err) + } + if err := repo.CreateTaskRepository(ctx, &models.TaskRepository{ + ID: "taskrepo1", TaskID: "t1", RepositoryID: "repo1", + }); err != nil { + t.Fatalf("create task repository: %v", err) + } + session := &models.TaskSession{ + ID: "s1", TaskID: "t1", AgentProfileID: "profile-a", ExecutorID: "exec-local", + State: models.TaskSessionStateRunning, IsPrimary: true, StartedAt: now, UpdatedAt: now, + } + if err := repo.CreateTaskSession(ctx, session); err != nil { + t.Fatalf("create task session: %v", err) + } + + steps := newMockStepGetter() + steps.steps["step1"] = &wfmodels.WorkflowStep{ID: "step1", WorkflowID: "wf1", Position: 1} + steps.steps["step2"] = &wfmodels.WorkflowStep{ + ID: "step2", WorkflowID: "wf1", Position: 2, AgentProfileID: "profile-b", + } + taskRepo := newMockTaskRepo() + taskRepo.tasks["t1"] = &v1.Task{ + ID: "t1", WorkspaceID: "ws1", WorkflowID: "wf1", Title: "Test", State: v1.TaskStateInProgress, + } + agentMgr := &mockAgentManager{repoForExecutionLookup: repo} + log := testLogger() + exec := executor.NewExecutor(agentMgr, repo, log, executor.ExecutorConfig{}) + exec.SetGitHubCredentialBroker(fakeSwitchSessionCredentialIssuer{}, "https://kandev.example/api/v1/github/credentials/resolve") + svc := &Service{ + logger: log, repo: repo, workflowStepGetter: steps, taskRepo: taskRepo, agentManager: agentMgr, + messageQueue: messagequeue.NewServiceMemory(log), executor: exec, + workflowStore: newWorkflowStore(repo, steps, agentMgr, noopPublisher, log), + } + + applied := svc.applyEngineTransition(ctx, "t1", session, engine.HandleResult{ + Transitioned: true, FromStepID: "step1", ToStepID: "step2", + }, engine.TriggerOnTurnStart, "", false) + if applied { + t.Fatal("applyEngineTransition() = true, want credential admission rejection") + } + storedTask, err := repo.GetTask(ctx, "t1") + if err != nil { + t.Fatalf("get task: %v", err) + } + if storedTask.WorkflowStepID != "step1" { + t.Fatalf("workflow step = %q, want source step1", storedTask.WorkflowStepID) + } + storedSession, err := repo.GetTaskSession(ctx, "s1") + if err != nil { + t.Fatalf("get session: %v", err) + } + if storedSession.State != models.TaskSessionStateRunning || !storedSession.IsPrimary { + t.Fatalf("session after rejection = %#v, want running primary source session", storedSession) + } +} + +func TestSwitchSessionForStepUsesReusableSessionExecutorProfileForCredentialAdmission(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + now := time.Now().UTC() + + if err := repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws1", Name: "Test", CreatedAt: now, UpdatedAt: now}); err != nil { + t.Fatalf("create workspace: %v", err) + } + if err := repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf1", WorkspaceID: "ws1", Name: "WF", CreatedAt: now, UpdatedAt: now}); err != nil { + t.Fatalf("create workflow: %v", err) + } + if err := repo.CreateTask(ctx, &models.Task{ + ID: "t1", WorkspaceID: "ws1", WorkflowID: "wf1", WorkflowStepID: "step1", + Title: "Test", State: v1.TaskStateInProgress, CreatedAt: now, UpdatedAt: now, + }); err != nil { + t.Fatalf("create task: %v", err) + } + if err := repo.CreateRepository(ctx, &models.Repository{ + ID: "repo1", WorkspaceID: "ws1", Name: "widgets", Provider: "acme-forge", + RemoteURL: "https://forge.example/acme/widgets.git", + }); err != nil { + t.Fatalf("create repository: %v", err) + } + if err := repo.CreateTaskRepository(ctx, &models.TaskRepository{ + ID: "taskrepo1", TaskID: "t1", RepositoryID: "repo1", + }); err != nil { + t.Fatalf("create task repository: %v", err) + } + if err := repo.CreateExecutor(ctx, &models.Executor{ + ID: "exec-ssh", Name: "SSH", Type: models.ExecutorTypeSSH, Status: models.ExecutorStatusActive, + }); err != nil { + t.Fatalf("create executor: %v", err) + } + if err := repo.CreateExecutorProfile(ctx, &models.ExecutorProfile{ + ID: "ep-token", ExecutorID: "exec-ssh", Name: "Token profile", + Config: map[string]string{"remote_auth_secrets": `{"gh_cli_env":"secret-gh"}`}, + }); err != nil { + t.Fatalf("create executor profile: %v", err) + } + current := &models.TaskSession{ + ID: "s-current", TaskID: "t1", AgentProfileID: "profile-a", State: models.TaskSessionStateRunning, + IsPrimary: true, StartedAt: now, UpdatedAt: now, + } + target := &models.TaskSession{ + ID: "s-target", TaskID: "t1", AgentProfileID: "profile-b", ExecutorID: "exec-ssh", + ExecutorProfileID: "ep-token", State: models.TaskSessionStateCompleted, + StartedAt: now.Add(-time.Minute), UpdatedAt: now, + } + if err := repo.CreateTaskSession(ctx, current); err != nil { + t.Fatalf("create current session: %v", err) + } + if err := repo.CreateTaskSession(ctx, target); err != nil { + t.Fatalf("create target session: %v", err) + } + taskRepo := newMockTaskRepo() + taskRepo.tasks["t1"] = &v1.Task{ID: "t1", WorkspaceID: "ws1", WorkflowID: "wf1", State: v1.TaskStateInProgress} + agentMgr := &mockAgentManager{repoForExecutionLookup: repo} + log := testLogger() + exec := executor.NewExecutor(agentMgr, repo, log, executor.ExecutorConfig{}) + exec.SetGitHubCredentialBroker(fakeSwitchSessionCredentialIssuer{}, "https://kandev.example/api/v1/github/credentials/resolve") + svc := &Service{ + logger: log, repo: repo, taskRepo: taskRepo, agentManager: agentMgr, + messageQueue: messagequeue.NewServiceMemory(log), executor: exec, + } + + got, err := svc.switchSessionForStep(ctx, "t1", current, "profile-b") + if err != nil { + t.Fatalf("switchSessionForStep() error = %v, want reusable profile token override", err) + } + if got.ID != target.ID { + t.Fatalf("session = %q, want reusable target %q", got.ID, target.ID) + } +} diff --git a/apps/backend/internal/orchestrator/executor/executor_credentials.go b/apps/backend/internal/orchestrator/executor/executor_credentials.go index cad9fe5c02..66167238d5 100644 --- a/apps/backend/internal/orchestrator/executor/executor_credentials.go +++ b/apps/backend/internal/orchestrator/executor/executor_credentials.go @@ -16,7 +16,6 @@ import ( "github.com/kandev/kandev/internal/gitconfigenv" "github.com/kandev/kandev/internal/gitcredentials" "github.com/kandev/kandev/internal/githubauth" - "github.com/kandev/kandev/internal/repoclone" "github.com/kandev/kandev/internal/task/models" ) @@ -236,12 +235,104 @@ func (e *Executor) resolveManagedGitHubCredentials( return false, nil } +// PreflightManagedGitCredentials validates every task repository binding that +// would use a managed Git credential lease, without cloning or issuing one. +// It is a read-only check meant to run before a new session row is persisted +// (PrepareSession) or before a workflow profile switch tears down the +// current session (see switchSessionForStep), so an irreparable repository +// identity - a foreign/ambiguous provider host, a malformed clone URL - +// rejects the operation before anything is mutated, rather than surfacing as +// a doomed session created later at launch time. +func (e *Executor) PreflightManagedGitCredentials( + ctx context.Context, + workspaceID, taskID, executorID, executorProfileID string, +) error { + metadata := make(map[string]interface{}) + if executorProfileID != "" { + metadata["executor_profile_id"] = executorProfileID + } + execConfig := e.resolveExecutorConfig(ctx, executorID, workspaceID, metadata) + return e.preflightManagedGitCredentials(ctx, workspaceID, taskID, execConfig) +} + +func (e *Executor) preflightManagedGitCredentials( + ctx context.Context, + workspaceID, taskID string, + execConfig executorConfig, +) error { + if e.gitCredentialIssuer == nil { + // No broker configured: the managed-credential feature is inactive, so + // nothing here will ever attempt to resolve a repository's identity. + return nil + } + policy := TaskGitCredentialPolicy{Mode: taskGitCredentialsModeManaged} + if e.githubCredentialPolicyResolver != nil { + resolved, err := e.githubCredentialPolicyResolver.ResolveTaskGitCredentialPolicy(ctx, workspaceID) + if err != nil { + return fmt.Errorf("resolve task Git credential policy: %w", err) + } + policy = resolved + } + if policy.Mode == taskGitCredentialsModeExecutor { + return nil + } + if executorProfileHasGitHubToken(execConfig) { + return nil + } + taskRepos, err := e.repo.ListTaskRepositories(ctx, taskID) + if err != nil { + return fmt.Errorf("list task repositories: %w", err) + } + for _, taskRepo := range taskRepos { + if taskRepo == nil || taskRepo.RepositoryID == "" { + continue + } + repository, err := e.repo.GetRepository(ctx, taskRepo.RepositoryID) + if err != nil { + return fmt.Errorf("load repository %q: %w", taskRepo.RepositoryID, err) + } + if repository == nil || managedGitCredentialProvider(repository, true, nil) == "" { + continue + } + if _, _, _, _, err := gitCredentialCloneIdentity(repository, repository.ID); err != nil { + return err + } + } + return nil +} + +func executorProfileHasGitHubToken(execConfig executorConfig) bool { + if !executorNeedsResolvedCredentials(execConfig.ExecutorType) { + return false + } + authSecretsJSON, _ := execConfig.Metadata[profileKeyRemoteAuthSecrets].(string) + if authSecretsJSON == "" { + return false + } + var authSecrets map[string]string + if err := json.Unmarshal([]byte(authSecretsJSON), &authSecrets); err != nil { + return false + } + return strings.TrimSpace(authSecrets["gh_cli_env"]) != "" +} + func (e *Executor) issueGitCredentialScopes( ctx context.Context, req *LaunchAgentRequest, infos []*repoInfo, githubManaged bool, ) ([]githubCredentialScope, map[string]struct{}, error) { + // Validate every managed repository before issuing the first lease. This avoids + // leaving a partial lease set when a later task binding is invalid. + for _, info := range infos { + if info == nil || info.Repository == nil || managedGitCredentialProvider(info.Repository, githubManaged, req.Env) == "" { + continue + } + if _, _, _, _, err := gitCredentialCloneIdentity(info.Repository, info.RepositoryID); err != nil { + return nil, nil, err + } + } + scopes := make([]githubCredentialScope, 0, len(infos)*2) helpers := make(map[string]struct{}, len(infos)) for _, info := range infos { @@ -572,58 +663,18 @@ func managedGitCredentialProvider(repository *models.Repository, githubManaged b return providerID } -// credentialIdentityCloneURL returns the HTTPS clone URL a managed credential -// scope is derived from. A repository cloned over SSH stores an SSH remote URL; -// that is a transport choice, not a different repository, so it does not fail -// the launch. The persisted provider identity wins, because its host carries -// the real HTTPS origin including any non-default port that an SSH URL cannot -// express; rewriting the remote in place is the fallback when there is none. -func credentialIdentityCloneURL(repository *models.Repository) string { - cloneURL := repositoryCloneURL(repository) - if strings.HasPrefix(strings.ToLower(cloneURL), httpsScheme+"://") { - return cloneURL - } - if derived := providerHTTPSCloneURL(repository); derived != "" { - return derived - } - if converted := repoclone.CanonicalHTTPSCloneURL(cloneURL); converted != "" { - return converted - } - return cloneURL -} - func gitCredentialCloneIdentity(repository *models.Repository, repositoryID string) (string, string, string, string, error) { - cloneURL := credentialIdentityCloneURL(repository) - if cloneURL == "" { - return "", "", "", "", fmt.Errorf("repository %q has no HTTPS clone URL for managed credentials", repositoryID) + if repository == nil { + return "", "", "", "", fmt.Errorf("repository %q is required", repositoryID) } - parsed, err := url.Parse(cloneURL) - if err != nil || parsed.Scheme != httpsScheme || parsed.Host == "" || parsed.User != nil || parsed.RawQuery != "" || - parsed.Fragment != "" || parsed.Path == "" { - return "", "", "", "", fmt.Errorf("repository %q must use an HTTPS clone URL for managed credentials", repositoryID) + identity, err := gitcredentials.ResolveRepositoryIdentity(gitcredentials.RepositoryIdentityInput{ + RepositoryID: repositoryID, Provider: repository.Provider, ProviderHost: repository.ProviderHost, + ProviderOwner: repository.ProviderOwner, ProviderName: repository.ProviderName, RemoteURL: repository.RemoteURL, + }) + if err != nil { + return "", "", "", "", err } - providerHost := strings.TrimSpace(repository.ProviderHost) - providerID := strings.ToLower(strings.TrimSpace(repository.Provider)) - if providerHost == "" && (providerID == "" || providerID == gitHubProviderID) { - providerHost = "https://" + defaultGitHubHost - } - if err := repoclone.ValidateHTTPSCloneOrigin(cloneURL, providerHost); err != nil { - return "", "", "", "", fmt.Errorf("repository %q provider origin: %w", repositoryID, err) - } - owner, repo, err := credentialHelperPath(parsed.Path) - if err != nil || repositoryID == "" { - return "", "", "", "", fmt.Errorf("repository %q has an invalid HTTPS clone path", repositoryID) - } - return parsed.Host, parsed.Path, owner, repo, nil -} - -func credentialHelperPath(path string) (string, string, error) { - trimmed := strings.TrimSuffix(strings.Trim(path, "/"), ".git") - owner, repo, found := strings.Cut(trimmed, "/") - if !found || owner == "" || repo == "" { - return "", "", errors.New("repository path needs owner and name") - } - return owner, repo, nil + return identity.Host, identity.Path, identity.Owner, identity.Repository, nil } func validateGitHubCredentialBrokerURL(raw, executorType string) error { diff --git a/apps/backend/internal/orchestrator/executor/executor_credentials_preflight_test.go b/apps/backend/internal/orchestrator/executor/executor_credentials_preflight_test.go new file mode 100644 index 0000000000..3d2803ca5c --- /dev/null +++ b/apps/backend/internal/orchestrator/executor/executor_credentials_preflight_test.go @@ -0,0 +1,259 @@ +package executor + +import ( + "context" + "errors" + "os/exec" + "strings" + "testing" + + "github.com/kandev/kandev/internal/gitcredentials" + "github.com/kandev/kandev/internal/task/models" + v1 "github.com/kandev/kandev/pkg/api/v1" +) + +func newPreflightTestExecutor(t *testing.T, repo *mockRepository) *Executor { + t.Helper() + issuer := &fakeGitHubCredentialLeaseIssuer{lease: gitcredentials.Lease{Token: "opaque-lease"}} + exec := newTestExecutor(t, &mockAgentManager{}, repo) + exec.SetGitHubCredentialBroker(issuer, "https://kandev.example/api/v1/github/credentials/resolve") + return exec +} + +func seedPreflightTaskRepository(repo *mockRepository, taskID, repositoryID string, repository *models.Repository) { + repo.taskRepositories[taskID+"/"+repositoryID] = &models.TaskRepository{ + ID: taskID + "/" + repositoryID, TaskID: taskID, RepositoryID: repositoryID, + } + repo.repositories[repositoryID] = repository +} + +// TestPreflightManagedGitCredentialsAcceptsValidLegacyAndPluginGitHubRows +// covers AC-1/AC-2: legacy rows with either GitHub HTTPS/SSH spellings and an +// empty or plugin-specific provider must pass the preflight cleanly. +func TestPreflightManagedGitCredentialsAcceptsValidLegacyAndPluginGitHubRows(t *testing.T) { + for name, remoteURL := range map[string]string{ + "empty provider, ssh remote": "git@github.com:acme/widgets.git", + "empty provider, https remote": "https://github.com/acme/widgets.git", + } { + t.Run(name, func(t *testing.T) { + repo := newMockRepository() + seedPreflightTaskRepository(repo, "task-1", "repo-1", &models.Repository{ + ID: "repo-1", SourceType: sourceTypeLocal, RemoteURL: remoteURL, + }) + exec := newPreflightTestExecutor(t, repo) + + if err := exec.PreflightManagedGitCredentials(context.Background(), "workspace-1", "task-1", "", ""); err != nil { + t.Fatalf("PreflightManagedGitCredentials() error = %v", err) + } + }) + } + + t.Run("plugin provider, ssh remote", func(t *testing.T) { + repo := newMockRepository() + seedPreflightTaskRepository(repo, "task-1", "repo-1", &models.Repository{ + ID: "repo-1", SourceType: sourceTypeLocal, Provider: "kandev-plugin-tags", + RemoteURL: "ssh://git@github.com/acme/widgets.git", + }) + exec := newPreflightTestExecutor(t, repo) + + if err := exec.PreflightManagedGitCredentials(context.Background(), "workspace-1", "task-1", "", ""); err != nil { + t.Fatalf("PreflightManagedGitCredentials() error = %v", err) + } + }) +} + +// TestPreflightManagedGitCredentialsRejectsIrreparableIdentity covers AC-9: a +// repository whose remote host conflicts with an untrusted/custom provider +// (no explicit provider host) must fail the preflight with a safe, +// repository-scoped error, before any session row is ever persisted. +func TestPreflightManagedGitCredentialsRejectsIrreparableIdentity(t *testing.T) { + repo := newMockRepository() + seedPreflightTaskRepository(repo, "task-1", "repo-1", &models.Repository{ + ID: "repo-1", SourceType: sourceTypeLocal, Provider: "acme-forge", + RemoteURL: "https://forge.example/acme/widgets.git", + }) + exec := newPreflightTestExecutor(t, repo) + + err := exec.PreflightManagedGitCredentials(context.Background(), "workspace-1", "task-1", "", "") + if err == nil { + t.Fatal("PreflightManagedGitCredentials() error = nil, want rejection of an untrusted custom host") + } + if !strings.Contains(err.Error(), "repo-1") { + t.Fatalf("error %q does not name the safe repository id repo-1", err.Error()) + } +} + +func TestPreflightManagedGitCredentialsDoesNotAuthorizeFromLocalCheckoutOrigin(t *testing.T) { + localPath := t.TempDir() + if output, err := exec.Command("git", "init", localPath).CombinedOutput(); err != nil { + t.Fatalf("git init error = %v: %s", err, output) + } + if output, err := exec.Command( + "git", "-C", localPath, "remote", "add", "origin", "https://github.com/acme/widgets.git", + ).CombinedOutput(); err != nil { + t.Fatalf("git remote add error = %v: %s", err, output) + } + + repo := newMockRepository() + seedPreflightTaskRepository(repo, "task-1", "repo-1", &models.Repository{ + ID: "repo-1", SourceType: sourceTypeLocal, Provider: "github", LocalPath: localPath, + }) + exec := newPreflightTestExecutor(t, repo) + + err := exec.PreflightManagedGitCredentials(context.Background(), "workspace-1", "task-1", "", "") + if err == nil { + t.Fatal("PreflightManagedGitCredentials() error = nil, want incomplete persisted identity rejection") + } +} + +// TestPreflightManagedGitCredentialsValidatesEveryBindingInOrder covers the +// multi-repository requirement: a valid primary binding alongside an +// irreparable secondary binding must still fail, so a multi-repo task cannot +// slip one bad repository past the check. +func TestPreflightManagedGitCredentialsValidatesEveryBindingInOrder(t *testing.T) { + repo := newMockRepository() + seedPreflightTaskRepository(repo, "task-1", "repo-primary", &models.Repository{ + ID: "repo-primary", SourceType: sourceTypeLocal, RemoteURL: "https://github.com/acme/widgets.git", + }) + seedPreflightTaskRepository(repo, "task-1", "repo-secondary", &models.Repository{ + ID: "repo-secondary", SourceType: sourceTypeLocal, Provider: "acme-forge", + RemoteURL: "git@forge.example:acme/widgets2.git", + }) + exec := newPreflightTestExecutor(t, repo) + + err := exec.PreflightManagedGitCredentials(context.Background(), "workspace-1", "task-1", "", "") + if err == nil { + t.Fatal("PreflightManagedGitCredentials() error = nil, want rejection for the irreparable secondary binding") + } + if !strings.Contains(err.Error(), "repo-secondary") { + t.Fatalf("error %q does not name the failing secondary repository id", err.Error()) + } +} + +// TestPreflightManagedGitCredentialsSkipsWhenExecutorModePolicy covers the +// workspace policy applicability rule: when the resolved policy routes Git +// credentials through the executor rather than the managed broker, no +// repository identity is validated at all - matching +// configureGitCredentialBrokerForRepositories' own unconditional skip. +func TestPreflightManagedGitCredentialsSkipsWhenExecutorModePolicy(t *testing.T) { + repo := newMockRepository() + seedPreflightTaskRepository(repo, "task-1", "repo-1", &models.Repository{ + ID: "repo-1", SourceType: sourceTypeLocal, Provider: "acme-forge", + RemoteURL: "https://forge.example/acme/widgets.git", + }) + exec := newPreflightTestExecutor(t, repo) + exec.SetTaskGitCredentialPolicyResolver(fakeTaskGitCredentialPolicyResolver{ + policy: TaskGitCredentialPolicy{Mode: taskGitCredentialsModeExecutor}, + }) + + if err := exec.PreflightManagedGitCredentials(context.Background(), "workspace-1", "task-1", "", ""); err != nil { + t.Fatalf("PreflightManagedGitCredentials() error = %v, want skip under executor-mode policy", err) + } +} + +func TestPrepareSessionSkipsManagedIdentityForRemoteProfileGitHubToken(t *testing.T) { + repo := newMockRepository() + seedPreflightTaskRepository(repo, "task-1", "repo-1", &models.Repository{ + ID: "repo-1", SourceType: sourceTypeLocal, Provider: "acme-forge", + RemoteURL: "https://forge.example/acme/widgets.git", + }) + repo.executors["exec-ssh"] = &models.Executor{ID: "exec-ssh", Type: models.ExecutorTypeSSH} + repo.executorProfiles["profile-token"] = &models.ExecutorProfile{ + ID: "profile-token", ExecutorID: "exec-ssh", + Config: map[string]string{profileKeyRemoteAuthSecrets: `{"gh_cli_env":"secret-gh"}`}, + } + exec := newPreflightTestExecutor(t, repo) + + task := &v1.Task{ID: "task-1", WorkspaceID: "workspace-1"} + if _, err := exec.PrepareSession( + context.Background(), task, "agent-profile", "exec-ssh", "profile-token", "", + ); err != nil { + t.Fatalf("PrepareSession() error = %v, want profile token override", err) + } + if len(repo.createTaskSessionCalls) != 1 { + t.Fatalf("CreateTaskSession calls = %d, want 1", len(repo.createTaskSessionCalls)) + } +} + +// TestPreflightManagedGitCredentialsNoopWithoutBroker covers the feature-off +// case: with no credential broker configured, the preflight must not fail a +// launch that never would have attempted managed credential issuance anyway. +func TestPreflightManagedGitCredentialsNoopWithoutBroker(t *testing.T) { + repo := newMockRepository() + seedPreflightTaskRepository(repo, "task-1", "repo-1", &models.Repository{ID: "repo-1", SourceType: sourceTypeLocal}) + exec := newTestExecutor(t, &mockAgentManager{}, repo) + + if err := exec.PreflightManagedGitCredentials(context.Background(), "workspace-1", "task-1", "", ""); err != nil { + t.Fatalf("PreflightManagedGitCredentials() error = %v, want nil with no broker configured", err) + } +} + +// TestPreflightManagedGitCredentialsIsNonMutating asserts the preflight never +// touches the repository store: it must not create a session, issue a +// lease, or write anything, even when it rejects a binding. +func TestPreflightManagedGitCredentialsIsNonMutating(t *testing.T) { + repo := newMockRepository() + seedPreflightTaskRepository(repo, "task-1", "repo-1", &models.Repository{ + ID: "repo-1", SourceType: sourceTypeLocal, Provider: "acme-forge", + RemoteURL: "https://forge.example/acme/widgets.git", + }) + exec := newPreflightTestExecutor(t, repo) + issuer := exec.gitCredentialIssuer.(*fakeGitHubCredentialLeaseIssuer) + + if err := exec.PreflightManagedGitCredentials(context.Background(), "workspace-1", "task-1", "", ""); err == nil { + t.Fatal("expected rejection") + } + if issuer.calls != 0 { + t.Fatalf("Issue calls = %d, want 0 (preflight must never issue a lease)", issuer.calls) + } + if len(repo.createTaskSessionCalls) != 0 { + t.Fatalf("CreateTaskSession calls = %d, want 0 (preflight must never persist a session)", len(repo.createTaskSessionCalls)) + } +} + +// TestPrepareSessionRejectsIrreparableManagedCredentialRepositoryBeforePersisting +// covers AC-9 end to end at the PrepareSession seam: a task bound to a +// repository with an irreparable managed-credential identity must fail +// before a session row exists at all - not create one that only fails later +// when LaunchPreparedSession attempts credential issuance. +func TestPrepareSessionRejectsIrreparableManagedCredentialRepositoryBeforePersisting(t *testing.T) { + repo := newMockRepository() + repo.taskRepositories["task-repo-1"] = &models.TaskRepository{ + ID: "task-repo-1", TaskID: "task-123", RepositoryID: "repo-123", + } + repo.repositories["repo-123"] = &models.Repository{ + ID: "repo-123", SourceType: sourceTypeLocal, Provider: "acme-forge", + RemoteURL: "https://forge.example/acme/widgets.git", + } + exec := newPreflightTestExecutor(t, repo) + + task := &v1.Task{ID: "task-123", WorkspaceID: "workspace-123"} + _, err := exec.PrepareSession(context.Background(), task, "profile-123", "", "", "") + if err == nil { + t.Fatal("PrepareSession() error = nil, want rejection of the irreparable repository binding") + } + if !strings.Contains(err.Error(), "repo-123") { + t.Fatalf("error %q does not name the safe repository id repo-123", err.Error()) + } + if len(repo.createTaskSessionCalls) != 0 { + t.Fatalf("CreateTaskSession calls = %d, want 0 (no session row should be persisted)", len(repo.createTaskSessionCalls)) + } +} + +func TestPrepareSessionReturnsCredentialPolicyErrorBeforePersisting(t *testing.T) { + repo := newMockRepository() + seedPreflightTaskRepository(repo, "task-123", "repo-123", &models.Repository{ + ID: "repo-123", Provider: "github", RemoteURL: "https://github.com/acme/widgets.git", + }) + exec := newPreflightTestExecutor(t, repo) + exec.SetTaskGitCredentialPolicyResolver(fakeTaskGitCredentialPolicyResolver{err: errors.New("policy unavailable")}) + + task := &v1.Task{ID: "task-123", WorkspaceID: "workspace-123"} + _, err := exec.PrepareSession(context.Background(), task, "profile-123", "", "", "") + if err == nil || !strings.Contains(err.Error(), "policy unavailable") { + t.Fatalf("PrepareSession() error = %v, want credential policy error", err) + } + if len(repo.createTaskSessionCalls) != 0 { + t.Fatalf("CreateTaskSession calls = %d, want 0", len(repo.createTaskSessionCalls)) + } +} diff --git a/apps/backend/internal/orchestrator/executor/executor_execute.go b/apps/backend/internal/orchestrator/executor/executor_execute.go index 9e517d5d3e..63107cc98c 100644 --- a/apps/backend/internal/orchestrator/executor/executor_execute.go +++ b/apps/backend/internal/orchestrator/executor/executor_execute.go @@ -850,6 +850,16 @@ func (e *Executor) PrepareSession(ctx context.Context, task *v1.Task, agentProfi session.ExecutorID = execConfig.ExecutorID } + // Validate every managed-credential repository binding before persisting + // the session row. Doing this after the row exists would leave a + // zero-message session behind once launch fails at credential issuance. + if err := e.preflightManagedGitCredentials(ctx, task.WorkspaceID, task.ID, execConfig); err != nil { + e.logger.Error("managed Git credential preflight failed", + zap.String("task_id", task.ID), + zap.Error(err)) + return "", err + } + var createErr error if atomicCreator, ok := e.repo.(initialRuntimeSeedTaskSessionCreator); ok { createErr = atomicCreator.CreateTaskSessionWithInitialRuntimeSeed(ctx, session) diff --git a/apps/backend/internal/orchestrator/executor/executor_office.go b/apps/backend/internal/orchestrator/executor/executor_office.go index 228a292cb5..bc1295d434 100644 --- a/apps/backend/internal/orchestrator/executor/executor_office.go +++ b/apps/backend/internal/orchestrator/executor/executor_office.go @@ -58,6 +58,11 @@ func (e *Executor) EnsureSessionForAgentWithCreation( if agentProfileID == "" { return nil, false, ErrNoAgentProfileID } + if err := e.PreflightManagedGitCredentials( + ctx, task.WorkspaceID, task.ID, executorID, executorProfileID, + ); err != nil { + return nil, false, err + } existing, err := e.repo.GetTaskSessionByTaskAndAgent(ctx, task.ID, agentInstanceID) if err != nil { diff --git a/apps/backend/internal/orchestrator/executor/executor_office_test.go b/apps/backend/internal/orchestrator/executor/executor_office_test.go index c4b83e8d80..0ba8b38c40 100644 --- a/apps/backend/internal/orchestrator/executor/executor_office_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_office_test.go @@ -77,6 +77,57 @@ func TestEnsureSessionForAgent_CreatesWhenMissing(t *testing.T) { } } +func TestEnsureSessionForAgentRejectsManagedIdentityBeforeCreatingSession(t *testing.T) { + repo := newMockRepository() + seedPreflightTaskRepository(repo, "task-office", "repo-1", &models.Repository{ + ID: "repo-1", Provider: "acme-forge", RemoteURL: "https://forge.example/acme/widgets.git", + }) + exec := newPreflightTestExecutor(t, repo) + + got, err := exec.EnsureSessionForAgent( + context.Background(), officeTestTask(), "agent-1", "profile-1", "", "", + ) + if err == nil { + t.Fatal("EnsureSessionForAgent() error = nil, want managed identity rejection") + } + if got != nil { + t.Fatalf("session = %#v, want nil", got) + } + if len(repo.createTaskSessionCalls) != 0 { + t.Fatalf("CreateTaskSession calls = %d, want 0", len(repo.createTaskSessionCalls)) + } +} + +func TestEnsureSessionForAgentRejectsManagedIdentityBeforeRebindingIdleSession(t *testing.T) { + repo := newMockRepository() + seedPreflightTaskRepository(repo, "task-office", "repo-1", &models.Repository{ + ID: "repo-1", Provider: "acme-forge", RemoteURL: "https://forge.example/acme/widgets.git", + }) + existing := &models.TaskSession{ + ID: "sess-existing", TaskID: "task-office", AgentProfileID: "agent-1", + ExecutionProfileID: "profile-old", State: models.TaskSessionStateIdle, StartedAt: time.Now().UTC(), + } + repo.sessions[existing.ID] = existing + exec := newPreflightTestExecutor(t, repo) + + got, err := exec.EnsureSessionForAgent( + context.Background(), officeTestTask(), "agent-1", "profile-new", "", "", + ) + if err == nil { + t.Fatal("EnsureSessionForAgent() error = nil, want managed identity rejection") + } + if got != nil { + t.Fatalf("session = %#v, want nil", got) + } + stored := repo.sessions[existing.ID] + if stored.State != models.TaskSessionStateIdle { + t.Fatalf("session state = %q, want IDLE", stored.State) + } + if stored.ExecutionProfileID != "profile-old" { + t.Fatalf("execution profile = %q, want profile-old", stored.ExecutionProfileID) + } +} + func TestEnsureSessionForAgent_DoesNotMarkExistingTaskSessionAsOrigin(t *testing.T) { repo := newMockRepository() repo.sessions["sess-existing"] = &models.TaskSession{ diff --git a/apps/backend/internal/task/repository/sqlite/base_migrations_repository_identity_test.go b/apps/backend/internal/task/repository/sqlite/base_migrations_repository_identity_test.go new file mode 100644 index 0000000000..b59441e0cf --- /dev/null +++ b/apps/backend/internal/task/repository/sqlite/base_migrations_repository_identity_test.go @@ -0,0 +1,45 @@ +package sqlite + +import ( + "context" + "path/filepath" + "testing" + + "github.com/jmoiron/sqlx" + + "github.com/kandev/kandev/internal/db" + "github.com/kandev/kandev/internal/task/models" +) + +func TestBaseMigrationsDoNotInferProviderHostFromPartialRepositoryIdentity(t *testing.T) { + dbPath := filepath.Join(t.TempDir(), "repository-provider-identity.db") + dbConn, err := db.OpenSQLite(dbPath) + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + sqlxDB := sqlx.NewDb(dbConn, "sqlite3") + t.Cleanup(func() { _ = sqlxDB.Close() }) + repo, err := NewWithDB(sqlxDB, sqlxDB, nil) + if err != nil { + t.Fatalf("new repository: %v", err) + } + seedWorkspace(t, repo, "ws-provider-identity") + repository := &models.Repository{ + ID: "repo-plugin", WorkspaceID: "ws-provider-identity", Name: "widgets", SourceType: "local", + Provider: "kandev-plugin-tags", RemoteURL: "https://github.com/acme/widgets.git", + } + if err := repo.CreateRepository(context.Background(), repository); err != nil { + t.Fatalf("create repository: %v", err) + } + + if err := repo.runMigrations(); err != nil { + t.Fatalf("run migrations: %v", err) + } + stored, err := repo.GetRepository(context.Background(), repository.ID) + if err != nil { + t.Fatalf("get repository: %v", err) + } + if stored.ProviderHost != "" { + t.Fatalf("provider_host = %q, want no inferred persistent origin", stored.ProviderHost) + } +} diff --git a/docs/plans/managed-git-credential-admission-repair/plan.md b/docs/plans/managed-git-credential-admission-repair/plan.md new file mode 100644 index 0000000000..40c3fa9114 --- /dev/null +++ b/docs/plans/managed-git-credential-admission-repair/plan.md @@ -0,0 +1,140 @@ +--- +spec: docs/specs/integrations/github-authentication.md +created: 2026-08-18 +status: complete +--- + +# Fix Plan: Make Managed Git Credential Admission Atomic + +## Overview + +PR #2787 adds a shared repository identity resolver and checks managed Git credential identity +before selected session changes. The direction fits the provider-neutral broker architecture, but +four gaps can still authorize different identities or mutate task state before a later rejection. + +The repair keeps one provider-neutral resolver. It makes persisted repository metadata the only +authorization source, validates secret-bearing SSH syntax before normalization, and runs one +read-only admission gate before all normal, Office, and workflow session mutations. It also removes +the startup migration that infers persistent provider identity from an incomplete legacy tuple. + +## Confirmed Root Causes + +### SSH syntax is normalized before it is validated + +`gitcredentials.parseRemote` first calls `repoclone.CanonicalHTTPSCloneURL`. That conversion drops +an `ssh://` password, query, or fragment before the resolver checks the canonical result. SCP-style +input can also retain a query or fragment in the path. The resolver therefore accepts remote text +that violates its credential-free input contract. + +### Issuance and redemption use different identity sources + +Executor preflight calls `gitCredentialCloneIdentity`, which can inspect `repository.LocalPath` and +read the checkout's Git origin. Broker authorization uses persisted `RemoteURL`, `ProviderHost`, +`Owner`, and `Name`. A repository with only a usable local origin can pass preflight and then fail +when the broker issues a credential. + +### Admission is incomplete and occurs after some state changes + +`PreflightManagedGitCredentials` ignores a task-policy resolution error and cannot determine that +an explicit executor-profile token overrides the managed broker. Office session creation and reuse +bypass the normal `PrepareSession` gate. A workflow transition persists the destination step before +the later profile-switch preflight runs. These paths can reject the launch after a session or the +workflow state has changed. + +### The migration infers a semantic identity from partial metadata + +`repository_credential_origin_migration.go` writes `provider_host` when it sees only a provider and +remote URL. It does not have the full verified provider tuple, and nullable legacy fields can also +abort its scan into Go strings. The accepted repository-origin decision requires existing rows to +remain hostless when the database cannot infer their origin safely. Runtime resolution already +recognizes public `github.com` remotes without this write. + +## Repair Architecture + +### Shared repository identity + +- Validate the original HTTPS, `ssh://`, and SCP-style syntax before any canonical conversion. +- Reject URL userinfo passwords, queries, and fragments with bounded errors that do not echo input. +- Remove the unused `RepositoryIdentity.CanonicalURL` field. +- Make executor preflight pass persisted repository identity fields only. Do not inspect a local + checkout during credential authorization. +- Keep `internal/gitcredentials` provider-neutral. GitHub-specific policy stays in its callers. + +### Read-only credential admission + +- Make policy-resolution errors fail closed. +- Resolve the selected executor profile before managed identity validation. +- Treat a configured executor-profile `GITHUB_TOKEN` or `GH_TOKEN`, including a secret reference, + as the effective unmanaged source. Admission must not reveal the secret value. +- Put policy, profile-source, and repository checks behind one read-only executor admission method. +- Call that method before normal session preparation changes state and before Office code creates, + rebinds, or resumes a session. + +### Workflow transition boundary + +- Resolve the destination agent and effective executor profile without changing state. +- Run credential admission before `processOnExit` and before the workflow engine applies the + transition. +- Keep post-transition session switching as the execution step. A failed admission must leave the + source step, current session, and prompt routing unchanged. + +### Persistence + +- Remove the new repository credential-origin migration and its registration from base migrations. +- Add a regression test that runs startup migrations for incomplete legacy repository metadata and + verifies that `provider_host` remains empty. +- Do not add a replacement backfill. Repository discovery or import remains responsible for + persisting verified provider-origin identity. + +## Tests + +- Reject `ssh://` passwords, queries, fragments, and SCP-style query or fragment input without + leaking the supplied remote in the error. +- Reject a provider-backed repository that has only a local checkout origin in both executor + preflight and broker authorization. +- Return task-policy resolver errors before session mutation. +- Allow an invalid managed identity when an explicit executor-profile GitHub token is the selected + source. +- Prove that fresh and reused Office sessions remain unchanged after failed admission. +- Prove that a failed destination-profile admission leaves the workflow on its source step and + does not route the destination prompt. +- Prove that startup migration does not infer `provider_host` from partial legacy metadata. + +## Documentation + +- Update the durable GitHub authentication specification with the atomic admission, persisted + identity, SSH validation, and no-backfill rules. +- No public documentation or frontend copy changes are required. The repair makes failure behavior + match the existing task Git access contract. + +## Implementation Tasks + +- [x] [task-01-harden-repository-identity](task-01-harden-repository-identity.md) - done +- [x] [task-02-centralize-session-admission](task-02-centralize-session-admission.md) - done +- [x] [task-03-remove-unsafe-origin-backfill](task-03-remove-unsafe-origin-backfill.md) - done + +Execution is sequential in the primary conversation. Task 02 depends on the corrected identity +contract from Task 01. Task 03 then restores the persistence boundary and locks it with a startup +regression test. + +## Risks And Out Of Scope + +- Existing database rows that already have a verified `provider_host` remain unchanged. +- This repair does not discover provider identity from arbitrary local Git configuration. +- This repair does not change the workspace policy model, broker lease scope, or plugin API. +- Profile-token admission checks only whether the selected profile configures the override. It must + not resolve or log the token during the read-only gate. +- Workflow admission must use the same profile that the later switch will use. A duplicated profile + selection rule would recreate the time-of-check/time-of-use gap. + +## Recorded Results + +- The resolver now rejects secret-bearing SSH forms before canonicalization and returns only safe, + repository-scoped errors. +- Executor issuance and broker redemption use the same persisted repository fields. +- Policy, remote profile token, normal session, Office session, and workflow transition admission + now complete before the related state mutation. +- The repository credential-origin backfill was removed. Startup migration keeps partial legacy + provider identity unchanged. +- Targeted verification passed 25 resolver tests, 11 identity/admission tests, 41 executor + lifecycle tests, 13 race-enabled workflow transition tests, and 598 SQLite repository tests. diff --git a/docs/plans/managed-git-credential-admission-repair/task-01-harden-repository-identity.md b/docs/plans/managed-git-credential-admission-repair/task-01-harden-repository-identity.md new file mode 100644 index 0000000000..168ad77486 --- /dev/null +++ b/docs/plans/managed-git-credential-admission-repair/task-01-harden-repository-identity.md @@ -0,0 +1,80 @@ +--- +id: "01-harden-repository-identity" +title: "Harden repository credential identity" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/integrations/github-authentication.md" +--- + +# Task 01: Harden Repository Credential Identity + +## Acceptance + +- The shared resolver rejects HTTPS or SSH credentials, queries, and fragments before + canonicalization can discard them. +- Rejection errors do not include the supplied remote or a contained secret. +- Executor preflight and broker authorization derive identity from the same persisted repository + fields and never use a local checkout origin as authorization data. +- The identity type contains only fields consumed by authorization. +- The package remains provider-neutral and contains no plugin-specific contract. + +## TDD Sequence + +1. Add table-driven resolver cases for an `ssh://` password, query, and fragment and for SCP-style + query and fragment input. Assert that each error omits the input and marker secret. +2. Run the focused identity test and record RED cases that the current canonicalization accepts. +3. Add an executor/broker parity case for a GitHub provider row with a valid local origin but no + persisted remote or owner/name identity. Record the current preflight/broker disagreement. +4. Validate original remote syntax before conversion, remove the unused canonical URL result, and + make executor preflight use the persisted remote. +5. Run the focused packages and record GREEN results. + +## Verification + +```bash +cd apps/backend && go test ./internal/gitcredentials -run 'TestResolveRepositoryIdentity' -count=1 +cd apps/backend && go test ./internal/orchestrator/executor ./internal/backendapp -run 'Test.*GitCredential.*(Identity|Preflight|Authorization)' -count=1 +``` + +## Files Likely Touched + +- `apps/backend/internal/gitcredentials/identity.go` +- `apps/backend/internal/gitcredentials/identity_test.go` +- `apps/backend/internal/orchestrator/executor/executor_credentials.go` +- `apps/backend/internal/orchestrator/executor/executor_credentials_preflight_test.go` +- `apps/backend/internal/backendapp/git_credentials.go` +- focused broker authorization tests under `apps/backend/internal/backendapp/` +- this task file and `plan.md` + +## Dependencies + +None. + +## Parallelism + +Sequential. Task 02 consumes the corrected persisted-identity contract. + +## Inputs + +- The managed identity rules in the GitHub authentication specification. +- `gitcredentials.ResolveRepositoryIdentity`. +- `executor.gitCredentialCloneIdentity`. +- The broker authorizer configured in `backendapp.ConfigureGitCredentialBroker`. + +## Output Contract + +Report the exact RED failures, the accepted remote forms, the common persisted identity source, +the GREEN commands, files changed, and remaining risks. Update this task and the parent plan with +the recorded results. + +## Recorded Results + +- RED: five SSH password/query/fragment cases passed unsafe input through canonicalization. +- RED: executor preflight accepted a valid local checkout origin when persisted authorization + identity was incomplete. +- GREEN: `go test ./internal/gitcredentials -run TestResolveRepositoryIdentity -count=1` passed 25 + tests. +- GREEN: the focused executor/backend application identity and admission command passed 11 tests. +- `RepositoryIdentity.CanonicalURL` was removed because no authorization caller used it. diff --git a/docs/plans/managed-git-credential-admission-repair/task-02-centralize-session-admission.md b/docs/plans/managed-git-credential-admission-repair/task-02-centralize-session-admission.md new file mode 100644 index 0000000000..a9da76f6ad --- /dev/null +++ b/docs/plans/managed-git-credential-admission-repair/task-02-centralize-session-admission.md @@ -0,0 +1,92 @@ +--- +id: "02-centralize-session-admission" +title: "Centralize session credential admission" +status: done +wave: 2 +depends_on: ["01-harden-repository-identity"] +plan: "plan.md" +spec: "../../specs/integrations/github-authentication.md" +--- + +# Task 02: Centralize Session Credential Admission + +## Acceptance + +- A task-policy resolver error stops admission and reaches the caller. +- Admission resolves the effective executor profile and skips broker identity validation when that + profile configures `GITHUB_TOKEN` or `GH_TOKEN`. +- Normal and Office paths run admission before any session create, rebind, resume, or state change. +- Workflow target-profile admission runs before exit hooks and the persisted step transition. +- A failed workflow admission preserves the source step and current session and does not deliver + the destination prompt. +- The admission operation is read-only and does not reveal an executor-profile secret. + +## TDD Sequence + +1. Add a policy-error case that records session storage calls and proves the current preflight + ignores the error. Add a remote-profile `gh_cli_env` secret-reference case. +2. Add fresh-session and idle-session Office cases. Record RED results for creation, profile + rebind, or state change before a managed identity failure. +3. Extend the workflow profile-preflight test to assert source-step persistence, current-session + ownership, exit-hook behavior, and destination prompt routing. Record the current partial + transition as RED. +4. Introduce one read-only admission boundary that selects policy, profile override, and persisted + repository identity. Call it before normal and Office mutations. +5. Resolve the destination profile and effective executor profile before workflow exit processing. + Run admission, then apply the transition and switch the session only after success. +6. Run the focused executor and orchestrator tests and record GREEN results. + +## Verification + +```bash +cd apps/backend && go test ./internal/orchestrator/executor -run 'Test.*(PreflightManagedGitCredentials|PrepareSession|EnsureSessionForAgent)' -count=1 +cd apps/backend && go test ./internal/orchestrator -run 'Test.*Workflow.*(Profile|Credential|Preflight|Transition)' -count=1 +``` + +## Files Likely Touched + +- `apps/backend/internal/orchestrator/executor/executor_credentials.go` +- `apps/backend/internal/orchestrator/executor/executor_execute.go` +- `apps/backend/internal/orchestrator/executor/executor_office.go` +- `apps/backend/internal/orchestrator/executor/executor_credentials_preflight_test.go` +- focused Office executor tests under `apps/backend/internal/orchestrator/executor/` +- `apps/backend/internal/orchestrator/event_handlers_workflow.go` +- `apps/backend/internal/orchestrator/event_handlers_workflow_profile_preflight_test.go` +- this task file and `plan.md` + +## Dependencies + +- Task 01 must establish the common persisted repository identity contract. + +## Parallelism + +Sequential. The session and workflow paths must use the same admission operation and effective +profile selection. + +## Inputs + +- The atomic admission scenarios in the GitHub authentication specification. +- `executor.PreflightManagedGitCredentials` and executor profile environment metadata. +- `PrepareSession` and `EnsureSessionForAgentWithCreation`. +- The workflow transition and `switchSessionForStep` paths. + +## Output Contract + +Report each RED mutation, the final admission call sites, how profile overrides are detected +without secret access, the GREEN commands, files changed, and remaining risks. Update this task and +the parent plan with the recorded results. + +## Recorded Results + +- RED: a policy resolver error returned success and persisted a session. +- RED: a configured remote-profile GitHub token still failed managed repository identity + admission. +- RED: fresh and idle Office paths created, rebound, or resumed a session before rejection. +- RED: workflow rejection occurred after the destination step was stored and changed the source + session state. +- RED: a reusable destination session with a token profile was checked with the source session's + executor profile. +- GREEN: the focused executor lifecycle command passed 41 tests. +- GREEN: the race-enabled workflow transition command passed 13 tests. +- Admission detects the selected profile's non-secret `gh_cli_env` binding metadata. It does not + reveal the referenced token. diff --git a/docs/plans/managed-git-credential-admission-repair/task-03-remove-unsafe-origin-backfill.md b/docs/plans/managed-git-credential-admission-repair/task-03-remove-unsafe-origin-backfill.md new file mode 100644 index 0000000000..3de0de8ccc --- /dev/null +++ b/docs/plans/managed-git-credential-admission-repair/task-03-remove-unsafe-origin-backfill.md @@ -0,0 +1,79 @@ +--- +id: "03-remove-unsafe-origin-backfill" +title: "Remove unsafe origin backfill" +status: done +wave: 3 +depends_on: ["02-centralize-session-admission"] +plan: "plan.md" +spec: "../../specs/integrations/github-authentication.md" +--- + +# Task 03: Remove Unsafe Origin Backfill + +## Acceptance + +- Startup migrations do not infer or persist `provider_host` from only a provider and remote URL. +- Incomplete legacy rows, including rows with nullable identity fields, do not stop startup schema + replay. +- Runtime identity resolution can still infer public `github.com` in memory from a safe persisted + remote. +- No replacement migration or duplicate provider-specific inference path is added. + +## TDD Sequence + +1. Add a startup migration regression with incomplete legacy provider metadata and nullable fields. + Assert that startup succeeds and `provider_host` remains empty. +2. Run the focused SQLite repository test and record RED because the PR migration writes the host + or fails while it scans a nullable value. +3. Remove the credential-origin migration registration, implementation, and migration-specific + tests. Keep the startup regression at the base migration boundary. +4. Run the SQLite repository package test and record GREEN. +5. Confirm that the specification and accepted provider-origin decision describe the same + persistence boundary. + +## Verification + +```bash +cd apps/backend && go test ./internal/task/repository/sqlite -run 'Test.*Migration.*ProviderHost' -count=1 +cd apps/backend && go test ./internal/task/repository/sqlite -count=1 +``` + +## Files Likely Touched + +- `apps/backend/internal/task/repository/sqlite/base_migrations.go` +- `apps/backend/internal/task/repository/sqlite/repository_credential_origin_migration.go` +- `apps/backend/internal/task/repository/sqlite/repository_credential_origin_migration_test.go` +- `apps/backend/internal/task/repository/sqlite/repository_credential_origin_migration_postgres_test.go` +- a focused base migration regression test under + `apps/backend/internal/task/repository/sqlite/` +- this task file and `plan.md` + +## Dependencies + +- Task 02 must make runtime admission safe without relying on a persistent backfill. + +## Parallelism + +Sequential. This final task removes the temporary persistence mechanism after the runtime contract +is complete. + +## Inputs + +- ADR `docs/decisions/2026-07-20-repository-provider-origin-identity.md`. +- The runtime public-host inference in `gitcredentials.ResolveRepositoryIdentity`. +- `baseMigrations` and startup migration replay tests. + +## Output Contract + +Report the RED persistence change or scan failure, deleted migration files, retained startup +regression, GREEN commands, files changed, and remaining risks. Update this task and the parent plan +with the recorded results. + +## Recorded Results + +- RED: startup migration changed an empty plugin repository `provider_host` to public GitHub from + only its provider label and remote URL. +- The new credential-origin migration, registration, and provider-specific tests were removed. +- A base migration regression now preserves the empty provider host for partial legacy identity. +- GREEN: the focused regression passed once. +- GREEN: `go test ./internal/task/repository/sqlite -count=1` passed 598 tests. diff --git a/docs/specs/integrations/github-authentication.md b/docs/specs/integrations/github-authentication.md index abb6dddda2..e9f8206347 100644 --- a/docs/specs/integrations/github-authentication.md +++ b/docs/specs/integrations/github-authentication.md @@ -401,9 +401,19 @@ post-signature processing failures produce `failing`; a later valid successful d - `executor -> executor-visible credentials`, regardless of PAT/CLI/App automation method. - Initial launch and resume run the same resolution. A successful operation replaces the session snapshot; a failed operation leaves the previous snapshot unchanged. +- Managed admission resolves the task policy and the selected executor profile before it validates + repository identity. An explicit profile `GITHUB_TOKEN` or `GH_TOKEN` bypasses managed broker + admission because the profile token is the effective source. +- Managed admission completes before Kandev creates, rebinds, resumes, or changes a session, and + before a workflow transition changes the current step. A policy error or an invalid repository + identity leaves the task, workflow, and session state unchanged. - Each attached repository is resolved once for an individual launch or resume. The resolved repository set is shared by task configuration and credential-snapshot construction rather than triggering repeated materialization or origin mutation. +- Broker issuance and redemption use the same persisted repository identity fields. A local Git + checkout and its `origin` are materialization data, not authorization data. +- A public `github.com` remote can supply an in-memory origin when persisted provider-host metadata + is absent. Kandev does not rewrite repository provider identity from partial legacy metadata. - Changing the workspace policy affects new launches and the next resume, not an already-running process. @@ -425,6 +435,9 @@ post-signature processing failures produce `failing`; a later valid successful d - Public base URL validation requires a canonical HTTPS origin with no credentials, query, or fragment and rejects loopback, private, link-local, or non-globally-routable DNS results. Kandev does not fetch the supplied URL as validation. +- Repository identity accepts credential-free HTTPS and SSH remote forms. It rejects an SSH + password, query, or fragment before canonicalization, and an error never contains the rejected + secret-bearing remote. - App private keys, client secrets, webhook secrets, personal tokens, and live installation tokens never enter executor environments. Only brokered PAT/CLI tokens or repository-restricted installation tokens reach a managed trusted child operation; explicit executor-profile @@ -676,6 +689,20 @@ registration and never creates a global default. credential policy is selected, **THEN** Kandev leaves its configured `origin` unchanged. - **GIVEN** managed mode and an explicit executor-profile GitHub token, **WHEN** a task launches, **THEN** the profile token wins and the session disclosure labels its actor runtime-selected. +- **GIVEN** managed mode and an explicit executor-profile `GITHUB_TOKEN` or `GH_TOKEN`, **WHEN** an + attached repository has incomplete managed identity, **THEN** the profile token remains the + effective source and managed broker admission does not reject the task. +- **GIVEN** a managed repository remote with an SSH password, query, or fragment, **WHEN** Kandev + resolves its credential identity, **THEN** resolution fails without exposing the remote and no + task, workflow, or session state changes. +- **GIVEN** a provider-backed repository whose only usable identity is a local checkout origin, + **WHEN** a managed task launches or the broker authorizes a request, **THEN** both paths reject + the incomplete persisted identity before session state changes. +- **GIVEN** task Git credential policy resolution fails, **WHEN** a normal or Office task launches + or resumes, **THEN** Kandev returns the error and does not create, rebind, or resume a session. +- **GIVEN** a workflow transition selects an agent profile that cannot use managed credentials, + **WHEN** the source step exits, **THEN** Kandev keeps the source step and current session and does + not route the destination step prompt. - **GIVEN** a managed helper cannot execute or redeem its lease, **WHEN** Git requests GitHub HTTPS credentials, **THEN** the command fails without falling through to a personal helper or prompt. - **GIVEN** a broker-enabled managed task whose login profile replaces its inherited `PATH`, @@ -766,6 +793,8 @@ and the [system-service identity guardrails repair plan](../../plans/system-service-identity-guardrails/plan.md). The new-workspace default repair is tracked in [the new workspace GitHub access defaults plan](../../plans/new-workspace-github-access-defaults/plan.md). +Managed credential admission and repository identity corrections are tracked in +[the managed Git credential admission repair plan](../../plans/managed-git-credential-admission-repair/plan.md). ## Decision