From 907ee913738cf54922554a22c24eccabb89db406 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Wed, 9 Sep 2026 07:50:57 +0100 Subject: [PATCH] fix: protect referenced secrets from deletion (#3500) (#3503) --- .../agent/runtime/environment/environment.go | 2 +- .../environment/secret_recovery_test.go | 31 +++ .../lifecycle/environment_resolution.go | 4 + .../runtime/lifecycle/secret_recovery_test.go | 40 +++ apps/backend/internal/backendapp/main.go | 4 + .../internal/backendapp/secret_references.go | 176 +++++++++++++ .../secret_references_integration_test.go | 85 +++++++ .../backendapp/secret_references_test.go | 128 ++++++++++ apps/backend/internal/secrets/delete.go | 74 ++++++ .../backend/internal/secrets/delete_errors.go | 21 ++ apps/backend/internal/secrets/delete_test.go | 238 ++++++++++++++++++ apps/backend/internal/secrets/handlers.go | 50 +++- apps/backend/internal/secrets/service.go | 34 ++- apps/backend/internal/secrets/store.go | 6 + .../internal/secrets/user_visible_store.go | 9 + .../settings/secret-delete-error.test.ts | 67 +++++ .../settings/secret-delete-error.ts | 57 +++++ .../settings/secrets-delete-dialog.tsx | 140 ++++++++++- .../settings/secrets-list-item-row.tsx | 40 ++- .../settings/secrets-settings.test.ts | 83 +++++- .../components/settings/secrets-settings.tsx | 112 +++++++-- apps/web/e2e/helpers/api-client.ts | 12 +- .../settings/mobile-secrets-delete.spec.ts | 72 ++++++ .../tests/settings/repository-secrets.spec.ts | 2 +- .../e2e/tests/settings/secrets-delete.spec.ts | 51 +++- apps/web/lib/api/domains/secrets-api.ts | 14 ++ apps/web/lib/types/http-secrets.ts | 13 + apps/web/src/locales/en/settings.json | 12 + apps/web/src/locales/pseudo/settings.json | 12 + apps/web/src/locales/pt-pt/settings.json | 12 + apps/web/src/locales/zh-cn/settings.json | 12 + apps/web/src/locales/zh-hk/settings.json | 12 + apps/web/src/locales/zh-tw/settings.json | 12 + .../plans/secret-reference-protection/plan.md | 44 ++++ .../task-01-protect-references.md | 107 ++++++++ docs/public/agents-and-profiles.md | 8 +- docs/public/websocket-api.md | 11 + docs/specs/workspaces/README.md | 1 + .../requirements/repository-secrets.md | 11 +- .../system-design/repository-secrets.md | 62 +++++ 40 files changed, 1809 insertions(+), 72 deletions(-) create mode 100644 apps/backend/internal/agent/runtime/environment/secret_recovery_test.go create mode 100644 apps/backend/internal/agent/runtime/lifecycle/secret_recovery_test.go create mode 100644 apps/backend/internal/backendapp/secret_references.go create mode 100644 apps/backend/internal/backendapp/secret_references_integration_test.go create mode 100644 apps/backend/internal/backendapp/secret_references_test.go create mode 100644 apps/backend/internal/secrets/delete.go create mode 100644 apps/backend/internal/secrets/delete_errors.go create mode 100644 apps/backend/internal/secrets/delete_test.go create mode 100644 apps/web/components/settings/secret-delete-error.test.ts create mode 100644 apps/web/components/settings/secret-delete-error.ts create mode 100644 docs/plans/secret-reference-protection/plan.md create mode 100644 docs/plans/secret-reference-protection/task-01-protect-references.md create mode 100644 docs/specs/workspaces/system-design/repository-secrets.md diff --git a/apps/backend/internal/agent/runtime/environment/environment.go b/apps/backend/internal/agent/runtime/environment/environment.go index be78a4e9a1..01d10e6067 100644 --- a/apps/backend/internal/agent/runtime/environment/environment.go +++ b/apps/backend/internal/agent/runtime/environment/environment.go @@ -43,7 +43,7 @@ type SecretError struct { } func (e *SecretError) Error() string { - return fmt.Sprintf("environment key %q from %s could not be resolved", e.Key, e.Origin) + return fmt.Sprintf("environment key %q from %s references an unavailable secret. Re-select the secret in the %s environment settings", e.Key, e.Origin, e.Origin) } func (e *SecretError) Unwrap() error { return e.err } diff --git a/apps/backend/internal/agent/runtime/environment/secret_recovery_test.go b/apps/backend/internal/agent/runtime/environment/secret_recovery_test.go new file mode 100644 index 0000000000..49353430cd --- /dev/null +++ b/apps/backend/internal/agent/runtime/environment/secret_recovery_test.go @@ -0,0 +1,31 @@ +package environment + +import ( + "context" + "errors" + "strings" + "testing" +) + +// @covers AC-WORKSPACES-REPOSITORY-SECRETS-001.10 +func TestSecretRecoveryNamesSourceAndRepair(t *testing.T) { + for _, origin := range []string{OriginAgentProfile, OriginExecutorProfile, RepositoryOrigin("app")} { + t.Run(origin, func(t *testing.T) { + cause := errors.New("missing private-secret-id with private-value") + env, records, err := Resolve(context.Background(), []Definition{ + {Key: "MY_TOKEN", SecretID: "private-secret-id", Origin: origin}, + }, func(context.Context, Definition) (string, error) { return "", cause }) + if err == nil || env != nil || records != nil || !errors.Is(err, cause) { + t.Fatalf("resolution = %v, %v, %v", env, records, err) + } + for _, want := range []string{"MY_TOKEN", origin, "Re-select"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not contain %q", err, want) + } + } + if strings.Contains(err.Error(), "private-") { + t.Fatalf("error leaked secret details: %v", err) + } + }) + } +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/environment_resolution.go b/apps/backend/internal/agent/runtime/lifecycle/environment_resolution.go index 92e890d44a..140631fc7a 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/environment_resolution.go +++ b/apps/backend/internal/agent/runtime/lifecycle/environment_resolution.go @@ -45,6 +45,10 @@ func (m *Manager) resolveStrictEnvironment( resolved, records, err := runtimeenv.Resolve(ctx, definitions, m.resolveEnvironmentDefinition) if err != nil { + var secretErr *runtimeenv.SecretError + if errors.As(err, &secretErr) && secretErr.Origin == runtimeenv.OriginAgentProfile && profileInfo != nil && profileInfo.ProfileName != "" { + err = fmt.Errorf("agent profile %q: %w", profileInfo.ProfileName, err) + } return nil, fmt.Errorf("resolve task environment: %w", err) } m.logEnvironmentOverrides(req, records) diff --git a/apps/backend/internal/agent/runtime/lifecycle/secret_recovery_test.go b/apps/backend/internal/agent/runtime/lifecycle/secret_recovery_test.go new file mode 100644 index 0000000000..182b01ddaa --- /dev/null +++ b/apps/backend/internal/agent/runtime/lifecycle/secret_recovery_test.go @@ -0,0 +1,40 @@ +package lifecycle + +import ( + "context" + "errors" + "strings" + "testing" + + runtimeenv "github.com/kandev/kandev/internal/agent/runtime/environment" + settingsmodels "github.com/kandev/kandev/internal/agent/settings/models" + "github.com/kandev/kandev/internal/secrets" +) + +// @covers AC-WORKSPACES-REPOSITORY-SECRETS-001.10 +func TestSecretRecoveryNamesAgentProfile(t *testing.T) { + mgr := newTestManager(t) + mgr.secretStore = newInMemorySecretStore() + // A same-named replacement must not repair a reference to the deleted ID. + if err := mgr.secretStore.Create(context.Background(), &secrets.SecretWithValue{ + Secret: secrets.Secret{ID: "replacement-id", Name: "MY_TOKEN"}, Value: "private-value", + }); err != nil { + t.Fatal(err) + } + profile := &AgentProfileInfo{ProfileName: "Claude review", EnvVars: []settingsmodels.ProfileEnvVar{{Key: "MY_TOKEN", SecretID: "deleted-id"}}} + env, err := mgr.buildEnvForExecution(context.Background(), "exec-1", &LaunchRequest{EnvironmentResolutionRequired: true}, nil, profile) + var secretErr *runtimeenv.SecretError + if env != nil || !errors.As(err, &secretErr) { + t.Fatalf("resolution = %v, %v", env, err) + } + for _, want := range []string{"Claude review", "MY_TOKEN", "Re-select", "agent profile"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q lacks %q", err, want) + } + } + for _, private := range []string{"deleted-id", "replacement-id", "private-value"} { + if strings.Contains(err.Error(), private) { + t.Errorf("error leaked %q: %v", private, err) + } + } +} diff --git a/apps/backend/internal/backendapp/main.go b/apps/backend/internal/backendapp/main.go index 20dfbbb20f..f636356358 100644 --- a/apps/backend/internal/backendapp/main.go +++ b/apps/backend/internal/backendapp/main.go @@ -2337,6 +2337,10 @@ func buildHTTPServer( router.Use(integrationWorkspaceScopeMiddleware(services.Auth, services.Task)) secretsSvc := secrets.NewService(userSecretStore, log) + secretsSvc.SetReferenceChecker(secretReferenceChecker{ + agents: repos.AgentSettings, tasks: repos.Task, + authorizeWorkspace: services.Task.AuthorizeWorkspaceAccess, + }.list) // Workspace classification happens here, at the wiring boundary, where both // packages are importable: the task service's not-found sentinel becomes // the secrets sentinel (404), while raw lookup/storage errors pass through diff --git a/apps/backend/internal/backendapp/secret_references.go b/apps/backend/internal/backendapp/secret_references.go new file mode 100644 index 0000000000..ad335944a4 --- /dev/null +++ b/apps/backend/internal/backendapp/secret_references.go @@ -0,0 +1,176 @@ +package backendapp + +import ( + "context" + "errors" + "fmt" + + settingsmodels "github.com/kandev/kandev/internal/agent/settings/models" + "github.com/kandev/kandev/internal/secrets" + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository/repoerrors" +) + +type secretAgentReferenceReader interface { + ListAgents(context.Context) ([]*settingsmodels.Agent, error) + ListAgentProfiles(context.Context, string) ([]*settingsmodels.AgentProfile, error) +} + +type secretTaskReferenceReader interface { + ListAllExecutorProfiles(context.Context) ([]*models.ExecutorProfile, error) + ListWorkspaces(context.Context) ([]*models.Workspace, error) + ListRepositories(context.Context, string) ([]*models.Repository, error) +} + +type secretReferenceChecker struct { + agents secretAgentReferenceReader + tasks secretTaskReferenceReader + authorizeWorkspace func(context.Context, string) error +} + +func (c secretReferenceChecker) list(ctx context.Context, id string) ([]secrets.Reference, error) { + refs, err := c.agentReferences(ctx, id) + if err != nil { + return nil, err + } + profiles, err := c.tasks.ListAllExecutorProfiles(ctx) + if err != nil { + return nil, fmt.Errorf("list executor profiles: %w", err) + } + for _, profile := range profiles { + refs = appendEnvironmentReferences(refs, id, "executor_profile", profile.ID, profile.Name, profile.EnvVars) + } + repositories, err := c.repositoryReferences(ctx, id) + if err != nil { + return nil, err + } + return append(refs, repositories...), nil +} + +func (c secretReferenceChecker) agentReferences(ctx context.Context, id string) ([]secrets.Reference, error) { + agents, err := c.agents.ListAgents(ctx) + if err != nil { + return nil, fmt.Errorf("list agents: %w", err) + } + var refs []secrets.Reference + workspaceAccess := make(map[string]error) + for _, agent := range agents { + profiles, err := c.agents.ListAgentProfiles(ctx, agent.ID) + if err != nil { + return nil, fmt.Errorf("list agent profiles: %w", err) + } + refs, err = c.appendAgentProfileReferences(ctx, refs, id, profiles, workspaceAccess) + if err != nil { + return nil, err + } + } + return refs, nil +} + +func (c secretReferenceChecker) appendAgentProfileReferences( + ctx context.Context, + refs []secrets.Reference, + secretID string, + profiles []*settingsmodels.AgentProfile, + workspaceAccess map[string]error, +) ([]secrets.Reference, error) { + for _, profile := range profiles { + if profile == nil || !hasSecretReference(profile.EnvVars, secretID) { + continue + } + redacted, err := c.agentProfileReferenceRedacted(ctx, profile.WorkspaceID, workspaceAccess) + if err != nil { + return nil, err + } + if redacted { + refs = appendRedactedEnvironmentReferences(refs, secretID, "agent_profile", profile.EnvVars) + continue + } + refs = appendEnvironmentReferences(refs, secretID, "agent_profile", profile.ID, profile.Name, profile.EnvVars) + } + return refs, nil +} + +func (c secretReferenceChecker) agentProfileReferenceRedacted( + ctx context.Context, + workspaceID string, + workspaceAccess map[string]error, +) (bool, error) { + if workspaceID == "" { + return false, nil + } + if c.authorizeWorkspace == nil { + return false, errors.New("workspace reference authorization is unavailable") + } + accessErr, cached := workspaceAccess[workspaceID] + if !cached { + accessErr = c.authorizeWorkspace(ctx, workspaceID) + workspaceAccess[workspaceID] = accessErr + } + if accessErr != nil && !errors.Is(accessErr, repoerrors.ErrWorkspaceNotFound) { + return false, accessErr + } + return accessErr != nil, nil +} + +func appendRedactedEnvironmentReferences(refs []secrets.Reference, secretID, kind string, env []models.ProfileEnvVar) []secrets.Reference { + for _, entry := range env { + if entry.SecretID == secretID { + refs = append(refs, secrets.Reference{Kind: kind}) + } + } + return refs +} + +func hasSecretReference(env []models.ProfileEnvVar, secretID string) bool { + for _, entry := range env { + if entry.SecretID == secretID { + return true + } + } + return false +} + +func appendEnvironmentReferences(refs []secrets.Reference, secretID, kind, id, name string, env []models.ProfileEnvVar) []secrets.Reference { + for _, entry := range env { + if entry.SecretID == secretID { + refs = append(refs, secrets.Reference{Kind: kind, ID: id, Name: name, Key: entry.Key}) + } + } + return refs +} + +func (c secretReferenceChecker) repositoryReferences(ctx context.Context, id string) ([]secrets.Reference, error) { + workspaces, err := c.tasks.ListWorkspaces(ctx) + if err != nil { + return nil, fmt.Errorf("list workspaces: %w", err) + } + var refs []secrets.Reference + for _, workspace := range workspaces { + if c.authorizeWorkspace == nil { + return nil, errors.New("workspace reference authorization is unavailable") + } + accessErr := c.authorizeWorkspace(ctx, workspace.ID) + if accessErr != nil && !errors.Is(accessErr, repoerrors.ErrWorkspaceNotFound) { + return nil, accessErr + } + // Read every workspace so a hidden reference still prevents deletion. + repositories, err := c.tasks.ListRepositories(ctx, workspace.ID) + if err != nil { + return nil, fmt.Errorf("list repository references: %w", err) + } + for _, repository := range repositories { + for _, binding := range repository.SecretBindings { + if binding.SecretID != id { + continue + } + ref := secrets.Reference{Kind: "repository"} + if accessErr == nil { + ref.ID, ref.Name, ref.Key = repository.ID, repository.Name, binding.Key + } + refs = append(refs, ref) + } + } + } + return refs, nil +} diff --git a/apps/backend/internal/backendapp/secret_references_integration_test.go b/apps/backend/internal/backendapp/secret_references_integration_test.go new file mode 100644 index 0000000000..7da1a3856e --- /dev/null +++ b/apps/backend/internal/backendapp/secret_references_integration_test.go @@ -0,0 +1,85 @@ +package backendapp + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/require" + + settingsmodels "github.com/kandev/kandev/internal/agent/settings/models" + settingsstore "github.com/kandev/kandev/internal/agent/settings/store" + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/secrets" + "github.com/kandev/kandev/internal/task/models" + ws "github.com/kandev/kandev/pkg/websocket" +) + +// @covers AC-WORKSPACES-REPOSITORY-SECRETS-001.9 +func TestSecretReferenceDeletionWithRealRepositories(t *testing.T) { + h := newBootStateTestHarness(t) + ctx := context.Background() + log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json"}) + require.NoError(t, err) + agents, closeAgents, err := settingsstore.Provide(h.db, h.db, log) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, closeAgents()) }) + crypto, err := secrets.NewMasterKeyProvider(t.TempDir()) + require.NoError(t, err) + store, closeSecrets, err := secrets.Provide(h.db, h.db, crypto) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, closeSecrets()) }) + svc := secrets.NewService(secrets.NewUserVisibleStore(store), log) + svc.SetReferenceChecker(secretReferenceChecker{agents: agents, tasks: h.taskRepo, authorizeWorkspace: h.taskSvc.AuthorizeWorkspaceAccess}.list) + secret, err := svc.Create(ctx, &secrets.CreateSecretRequest{Name: "MY_TOKEN", Value: "private-value"}) + require.NoError(t, err) + seedSecretReferences(t, h, agents, secret.ID) + router := gin.New() + secrets.RegisterRoutes(router, ws.NewDispatcher(), svc, log) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, httptest.NewRequest(http.MethodDelete, "/api/v1/secrets/"+secret.ID, nil)) + require.Equal(t, http.StatusConflict, rec.Code, rec.Body.String()) + var response struct { + References []secrets.Reference `json:"references"` + } + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &response)) + require.Len(t, response.References, 3) + value, err := svc.Reveal(ctx, secret.ID) + require.NoError(t, err) + require.Equal(t, "private-value", value) + + rec = httptest.NewRecorder() + router.ServeHTTP(rec, httptest.NewRequest(http.MethodDelete, "/api/v1/secrets/"+secret.ID+"?force=true", nil)) + require.Equal(t, http.StatusNoContent, rec.Code) + _, err = svc.Get(ctx, secret.ID) + require.ErrorIs(t, err, secrets.ErrNotFound) + refs, err := (secretReferenceChecker{agents: agents, tasks: h.taskRepo, authorizeWorkspace: h.taskSvc.AuthorizeWorkspaceAccess}).list(ctx, secret.ID) + require.NoError(t, err) + require.Len(t, refs, 3, "forced deletion must retain bindings for repair") +} + +func seedSecretReferences(t *testing.T, h bootStateTestHarness, agents settingsstore.Repository, secretID string) { + t.Helper() + ctx := context.Background() + agent := &settingsmodels.Agent{Name: "secret-reference-test"} + require.NoError(t, agents.CreateAgent(ctx, agent)) + require.NoError(t, agents.CreateAgentProfile(ctx, &settingsmodels.AgentProfile{ + ID: "secret-agent-profile", AgentID: agent.ID, Name: "Claude review", Enabled: false, + EnvVars: []settingsmodels.ProfileEnvVar{{Key: "MY_TOKEN", SecretID: secretID}}, + })) + require.NoError(t, h.taskRepo.CreateExecutor(ctx, &models.Executor{ + ID: "secret-executor", Name: "Secret executor", Type: models.ExecutorTypeLocal, Status: models.ExecutorStatusActive, + })) + require.NoError(t, h.taskRepo.CreateExecutorProfile(ctx, &models.ExecutorProfile{ + ID: "secret-executor-profile", ExecutorID: "secret-executor", Name: "Local", + EnvVars: []models.ProfileEnvVar{{Key: "EXEC_TOKEN", SecretID: secretID}}, + })) + workspace := &models.Workspace{ID: "secret-workspace", Name: "Secret workspace"} + require.NoError(t, h.taskRepo.CreateWorkspace(ctx, workspace)) + require.NoError(t, h.taskRepo.CreateRepositoryWithSecretBindings(ctx, &models.Repository{ + ID: "secret-repository", WorkspaceID: workspace.ID, Name: "App", SourceType: "local", + }, []models.RepositorySecretBinding{{Key: "REPO_TOKEN", SecretID: secretID}})) +} diff --git a/apps/backend/internal/backendapp/secret_references_test.go b/apps/backend/internal/backendapp/secret_references_test.go new file mode 100644 index 0000000000..b6ab49e2d8 --- /dev/null +++ b/apps/backend/internal/backendapp/secret_references_test.go @@ -0,0 +1,128 @@ +package backendapp + +import ( + "context" + "errors" + "reflect" + "testing" + + settingsmodels "github.com/kandev/kandev/internal/agent/settings/models" + "github.com/kandev/kandev/internal/secrets" + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository/repoerrors" +) + +type secretReferenceFixture struct { + agents []*settingsmodels.Agent + profiles []*settingsmodels.AgentProfile + executors []*models.ExecutorProfile + workspaces []*models.Workspace + repositories map[string][]*models.Repository + fail string +} + +var errReferenceRead = errors.New("reference storage unavailable") + +func (f *secretReferenceFixture) ListAgents(context.Context) ([]*settingsmodels.Agent, error) { + if f.fail == "agents" { + return nil, errReferenceRead + } + return f.agents, nil +} +func (f *secretReferenceFixture) ListAgentProfiles(context.Context, string) ([]*settingsmodels.AgentProfile, error) { + if f.fail == "profiles" { + return nil, errReferenceRead + } + return f.profiles, nil +} +func (f *secretReferenceFixture) ListAllExecutorProfiles(context.Context) ([]*models.ExecutorProfile, error) { + if f.fail == "executors" { + return nil, errReferenceRead + } + return f.executors, nil +} +func (f *secretReferenceFixture) ListWorkspaces(context.Context) ([]*models.Workspace, error) { + if f.fail == "workspaces" { + return nil, errReferenceRead + } + return f.workspaces, nil +} +func (f *secretReferenceFixture) ListRepositories(_ context.Context, workspaceID string) ([]*models.Repository, error) { + if f.fail == "repositories" { + return nil, errReferenceRead + } + return f.repositories[workspaceID], nil +} + +func newSecretReferenceFixture() *secretReferenceFixture { + return &secretReferenceFixture{ + agents: []*settingsmodels.Agent{{ID: "claude"}}, + profiles: []*settingsmodels.AgentProfile{ + {ID: "profile", Name: "Claude", EnvVars: []settingsmodels.ProfileEnvVar{ + {Key: "MY_TOKEN", SecretID: "secret"}, {Key: "LITERAL", Value: "secret"}, {Key: "OTHER", SecretID: "other"}, + }}, + {ID: "visible-profile", Name: "Visible", WorkspaceID: "visible", EnvVars: []settingsmodels.ProfileEnvVar{ + {Key: "VISIBLE_TOKEN", SecretID: "secret"}, + }}, + {ID: "private-profile", Name: "Private", WorkspaceID: "private", EnvVars: []settingsmodels.ProfileEnvVar{ + {Key: "PRIVATE_TOKEN", SecretID: "secret"}, + }}, + }, + executors: []*models.ExecutorProfile{{ID: "executor", Name: "Local", EnvVars: []models.ProfileEnvVar{{Key: "EXEC_TOKEN", SecretID: "secret"}}}}, + workspaces: []*models.Workspace{{ID: "visible"}, {ID: "private"}}, + repositories: map[string][]*models.Repository{ + "visible": {{ID: "repo", Name: "App", WorkspaceID: "visible", SecretBindings: []models.RepositorySecretBinding{{Key: "REPO_TOKEN", SecretID: "secret"}}}}, + "private": {{ID: "private-id", Name: "Private app", WorkspaceID: "private", SecretBindings: []models.RepositorySecretBinding{{Key: "PRIVATE_KEY", SecretID: "secret"}}}}, + }, + } +} + +// @covers AC-WORKSPACES-REPOSITORY-SECRETS-001.9 +func TestSecretReferenceDiscoveryAndRedaction(t *testing.T) { + f := newSecretReferenceFixture() + checker := secretReferenceChecker{agents: f, tasks: f, authorizeWorkspace: func(_ context.Context, id string) error { + if id == "private" { + return repoerrors.ErrWorkspaceNotFound + } + return nil + }} + refs, err := checker.list(context.Background(), "secret") + if err != nil { + t.Fatal(err) + } + want := []secrets.Reference{ + {Kind: "agent_profile", ID: "profile", Name: "Claude", Key: "MY_TOKEN"}, + {Kind: "agent_profile", ID: "visible-profile", Name: "Visible", Key: "VISIBLE_TOKEN"}, + {Kind: "agent_profile"}, + {Kind: "executor_profile", ID: "executor", Name: "Local", Key: "EXEC_TOKEN"}, + {Kind: "repository", ID: "repo", Name: "App", Key: "REPO_TOKEN"}, + {Kind: "repository"}, + } + if !reflect.DeepEqual(refs, want) { + t.Fatalf("references = %#v, want %#v", refs, want) + } + refs, err = checker.list(context.Background(), "absent") + if err != nil || len(refs) != 0 { + t.Fatalf("unreferenced = %v, %v", refs, err) + } +} + +// @covers AC-WORKSPACES-REPOSITORY-SECRETS-001.11 +func TestSecretReferenceReadFailure(t *testing.T) { + for _, stage := range []string{"agents", "profiles", "executors", "workspaces", "repositories", "authorization"} { + t.Run(stage, func(t *testing.T) { + f := newSecretReferenceFixture() + f.fail = stage + checker := secretReferenceChecker{agents: f, tasks: f, authorizeWorkspace: func(context.Context, string) error { + if stage == "authorization" { + return errReferenceRead + } + return nil + }} + refs, err := checker.list(context.Background(), "secret") + if !errors.Is(err, errReferenceRead) || refs != nil { + t.Fatalf("references = %v, error = %v", refs, err) + } + }) + } +} diff --git a/apps/backend/internal/secrets/delete.go b/apps/backend/internal/secrets/delete.go new file mode 100644 index 0000000000..6b783aae0d --- /dev/null +++ b/apps/backend/internal/secrets/delete.go @@ -0,0 +1,74 @@ +package secrets + +import ( + "context" + "fmt" + "strings" +) + +// Reference identifies a secret binding without carrying its value or secret ID. +// Inaccessible repositories expose only Kind. +type Reference struct { + Kind string `json:"kind"` + ID string `json:"id,omitempty"` + Name string `json:"name,omitempty"` + Key string `json:"key,omitempty"` +} + +// InUseError prevents deletion of a secret with existing environment bindings. +type InUseError struct { + References []Reference `json:"references"` +} + +func (e *InUseError) Error() string { + labels := make([]string, 0, len(e.References)) + for _, ref := range e.References { + label := strings.ReplaceAll(ref.Kind, "_", " ") + if ref.Name != "" { + label += fmt.Sprintf(" %q", ref.Name) + } + if ref.Key != "" { + label += fmt.Sprintf(" (%s)", ref.Key) + } + labels = append(labels, label) + } + return "secret is in use by " + strings.Join(labels, ", ") + ". Remove or replace these references before deleting it." +} + +// SetReferenceChecker wires reference discovery from the owning repositories. +// An unavailable checker blocks ordinary user-facing deletion. +func (s *Service) SetReferenceChecker(checker func(context.Context, string) ([]Reference, error)) { + s.referenceChecker = checker +} + +func (s *Service) deleteChecked(ctx context.Context, id, workspaceID string, force bool) error { + if force { + return s.deleteStored(ctx, id, workspaceID) + } + refs, err := s.listReferences(ctx, id) + if err != nil { + return err + } + if len(refs) > 0 { + return &InUseError{References: refs} + } + return s.deleteStored(ctx, id, workspaceID) +} + +func (s *Service) listReferences(ctx context.Context, id string) ([]Reference, error) { + if s.referenceChecker == nil { + return nil, fmt.Errorf("secret reference checking is unavailable") + } + refs, err := s.referenceChecker(ctx, id) + if err != nil { + return nil, fmt.Errorf("check secret references: %w", err) + } + return refs, nil +} + +func (s *Service) deleteStored(ctx context.Context, id, workspaceID string) error { + if scoped, ok := s.store.(WorkspaceSecretDeleter); ok && workspaceID != "" { + return scoped.DeleteForWorkspace(ctx, id, workspaceID) + } + return s.store.Delete(ctx, id) +} diff --git a/apps/backend/internal/secrets/delete_errors.go b/apps/backend/internal/secrets/delete_errors.go new file mode 100644 index 0000000000..10f65b4f9d --- /dev/null +++ b/apps/backend/internal/secrets/delete_errors.go @@ -0,0 +1,21 @@ +package secrets + +import ( + "errors" + "net/http" + + ws "github.com/kandev/kandev/pkg/websocket" +) + +func classifyDeleteError(err error) (int, string, string, map[string]any) { + var inUse *InUseError + if errors.As(err, &inUse) { + return http.StatusConflict, ws.ErrorCodeConflict, inUse.Error(), map[string]any{ + "code": "secret_in_use", "references": inUse.References, + } + } + if errors.Is(err, ErrNotFound) || errors.Is(err, ErrWorkspaceAccessDenied) { + return http.StatusNotFound, ws.ErrorCodeNotFound, "secret not found", map[string]any{} + } + return http.StatusInternalServerError, ws.ErrorCodeInternalError, "failed to delete secret", map[string]any{} +} diff --git a/apps/backend/internal/secrets/delete_test.go b/apps/backend/internal/secrets/delete_test.go new file mode 100644 index 0000000000..677cb6878b --- /dev/null +++ b/apps/backend/internal/secrets/delete_test.go @@ -0,0 +1,238 @@ +package secrets + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "strings" + "testing" + + "github.com/kandev/kandev/internal/auth/authn" + ws "github.com/kandev/kandev/pkg/websocket" +) + +// @covers AC-WORKSPACES-REPOSITORY-SECRETS-001.11 +func TestDeleteWithoutReferenceCheckerFailsClosed(t *testing.T) { + svc := newSecretsHandlerService(t, nil, nil, nil) + id := seedGlobalViaService(t, svc, "MY_TOKEN", "private-value") + rec := doTransferHTTP(t, newSecretsHTTPRouter(t, svc), http.MethodDelete, "/api/v1/secrets/"+id, "") + if rec.Code != http.StatusInternalServerError { + t.Fatalf("delete status = %d, want 500 when reference checking is unavailable", rec.Code) + } + if _, err := svc.Get(context.Background(), id); err != nil { + t.Fatalf("secret was removed: %v", err) + } +} + +// @covers AC-WORKSPACES-REPOSITORY-SECRETS-001.9 +func TestDeleteReferencesConflictAndForce(t *testing.T) { + for _, scope := range []SecretScope{ScopeGlobal, ScopeWorkspace} { + t.Run(string(scope), func(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true}, nil, nil) + workspaceID, query := "", "" + if scope == ScopeWorkspace { + workspaceID, query = "workspace-a", "?workspace_id=workspace-a" + } + item := mustCreateViaService(t, svc, "MY_TOKEN", "private-value", scope, workspaceID) + refs := []Reference{{Kind: "agent_profile", ID: "profile-1", Name: "Claude", Key: "MY_TOKEN"}} + svc.SetReferenceChecker(func(_ context.Context, id string) ([]Reference, error) { + if id != item.ID { + t.Fatalf("checked wrong secret %q", id) + } + return refs, nil + }) + router := newSecretsHTTPRouter(t, svc) + path := "/api/v1/secrets/" + item.ID + query + rec := doTransferHTTP(t, router, http.MethodDelete, path, "") + if rec.Code != http.StatusConflict { + t.Fatalf("status = %d, body = %s", rec.Code, rec.Body) + } + var conflict struct { + Code string `json:"code"` + References []Reference `json:"references"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &conflict); err != nil { + t.Fatal(err) + } + if conflict.Code != "secret_in_use" || len(conflict.References) != 1 || conflict.References[0] != refs[0] { + t.Fatalf("conflict = %+v", conflict) + } + if strings.Contains(rec.Body.String(), "private-value") || strings.Contains(rec.Body.String(), item.ID) { + t.Fatalf("secret leaked: %s", rec.Body) + } + get := func() (*Secret, error) { + if workspaceID != "" { + return svc.GetWorkspaceSecret(context.Background(), item.ID, workspaceID) + } + return svc.Get(context.Background(), item.ID) + } + if _, err := get(); err != nil { + t.Fatalf("secret removed: %v", err) + } + separator := "?" + if query != "" { + separator = "&" + } + for _, force := range []string{"false", "1", "yes"} { + rec = doTransferHTTP(t, router, http.MethodDelete, path+separator+"force="+force, "") + if rec.Code != http.StatusConflict { + t.Fatalf("force=%s status=%d", force, rec.Code) + } + } + rec = doTransferHTTP(t, router, http.MethodDelete, path+separator+"force=true", "") + if rec.Code != http.StatusNoContent { + t.Fatalf("forced delete = %d: %s", rec.Code, rec.Body) + } + if _, err := get(); !errors.Is(err, ErrNotFound) { + t.Fatalf("secret remains: %v", err) + } + }) + } +} + +// @covers AC-WORKSPACES-REPOSITORY-SECRETS-001.12 +func TestListReferencesReturnsConflictsWithoutDeleting(t *testing.T) { + svc := newSecretsHandlerService(t, nil, nil, nil) + id := seedGlobalViaService(t, svc, "MY_TOKEN", "private-value") + refs := []Reference{{Kind: "agent_profile", ID: "profile-1", Name: "Claude", Key: "MY_TOKEN"}} + svc.SetReferenceChecker(func(_ context.Context, checkedID string) ([]Reference, error) { + if checkedID != id { + t.Fatalf("checked wrong secret %q", checkedID) + } + return refs, nil + }) + + rec := doTransferHTTP(t, newSecretsHTTPRouter(t, svc), http.MethodGet, "/api/v1/secrets/"+id+"/references", "") + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, body = %s", rec.Code, rec.Body) + } + var response struct { + References []Reference `json:"references"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &response); err != nil { + t.Fatal(err) + } + if len(response.References) != 1 || response.References[0] != refs[0] { + t.Fatalf("references = %+v", response.References) + } + if strings.Contains(rec.Body.String(), "private-value") || strings.Contains(rec.Body.String(), id) { + t.Fatalf("secret leaked: %s", rec.Body) + } + if _, err := svc.Get(context.Background(), id); err != nil { + t.Fatalf("reference lookup removed secret: %v", err) + } +} + +func TestListReferencesReturnsEmptyArrayWhenUnused(t *testing.T) { + svc := newSecretsHandlerService(t, nil, nil, nil) + id := seedGlobalViaService(t, svc, "unused", "private-value") + svc.SetReferenceChecker(func(context.Context, string) ([]Reference, error) { return nil, nil }) + + rec := doTransferHTTP(t, newSecretsHTTPRouter(t, svc), http.MethodGet, "/api/v1/secrets/"+id+"/references", "") + if rec.Code != http.StatusOK || !strings.Contains(rec.Body.String(), `"references":[]`) { + t.Fatalf("response = %d, body = %s", rec.Code, rec.Body) + } +} + +func TestListReferencesPreservesScopeAuthorizationAndSanitizesFailures(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true}, nil, nil) + item := mustCreateViaService(t, svc, "workspace token", "private-value", ScopeWorkspace, "workspace-a") + svc.SetReferenceChecker(func(context.Context, string) ([]Reference, error) { + return nil, errors.New("private-database-error") + }) + router := newSecretsHTTPRouter(t, svc) + + missingScope := doTransferHTTP(t, router, http.MethodGet, "/api/v1/secrets/"+item.ID+"/references", "") + if missingScope.Code != http.StatusNotFound { + t.Fatalf("missing workspace status = %d, body = %s", missingScope.Code, missingScope.Body) + } + failure := doTransferHTTP(t, router, http.MethodGet, "/api/v1/secrets/"+item.ID+"/references?workspace_id=workspace-a", "") + if failure.Code != http.StatusInternalServerError || strings.Contains(failure.Body.String(), "private-") { + t.Fatalf("lookup failure = %d, body = %s", failure.Code, failure.Body) + } + if _, err := svc.GetWorkspaceSecret(context.Background(), item.ID, "workspace-a"); err != nil { + t.Fatalf("reference lookup removed secret: %v", err) + } +} + +func TestDeleteUnreferencedSecret(t *testing.T) { + svc := newSecretsHandlerService(t, nil, nil, nil) + id := seedGlobalViaService(t, svc, "unused", "value") + svc.SetReferenceChecker(func(context.Context, string) ([]Reference, error) { return nil, nil }) + if err := svc.Delete(context.Background(), id); err != nil { + t.Fatal(err) + } + if _, err := svc.Get(context.Background(), id); !errors.Is(err, ErrNotFound) { + t.Fatalf("secret remains: %v", err) + } +} + +// @covers AC-WORKSPACES-REPOSITORY-SECRETS-001.11 +func TestDeleteLookupFailureIsSanitized(t *testing.T) { + svc := newSecretsHandlerService(t, nil, nil, nil) + id := seedGlobalViaService(t, svc, "MY_TOKEN", "private-value") + svc.SetReferenceChecker(func(context.Context, string) ([]Reference, error) { return nil, errors.New("private-database-error") }) + rec := doTransferHTTP(t, newSecretsHTTPRouter(t, svc), http.MethodDelete, "/api/v1/secrets/"+id, "") + if rec.Code != http.StatusInternalServerError || strings.Contains(rec.Body.String(), "private-") { + t.Fatalf("delete = %d: %s", rec.Code, rec.Body) + } + if _, err := svc.Get(context.Background(), id); err != nil { + t.Fatalf("secret removed: %v", err) + } +} + +func TestDeleteForcePreservesAuthorization(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true}, nil, nil) + item := mustCreateViaService(t, svc, "workspace secret", "value", ScopeWorkspace, "workspace-a") + svc.SetReferenceChecker(func(context.Context, string) ([]Reference, error) { + t.Fatal("unauthorized reference lookup") + return nil, nil + }) + svc.SetWorkspaceAuthorizer(func(context.Context, string) error { return ErrWorkspaceAccessDenied }) + for _, force := range []bool{false, true} { + if err := svc.DeleteWorkspaceSecret(context.Background(), item.ID, "workspace-a", force); !errors.Is(err, ErrWorkspaceAccessDenied) { + t.Fatalf("force=%v: %v", force, err) + } + } + owner := authn.WithIdentity(context.Background(), authn.Identity{UserID: "owner", Role: authn.RoleMember}) + other := authn.WithIdentity(context.Background(), authn.Identity{UserID: "other", Role: authn.RoleMember}) + global, err := svc.Create(owner, &CreateSecretRequest{Name: "owned", Value: "private-value"}) + if err != nil { + t.Fatal(err) + } + if err := svc.Delete(other, global.ID, true); !errors.Is(err, ErrNotFound) { + t.Fatalf("foreign delete: %v", err) + } + if _, err := svc.Get(owner, global.ID); err != nil { + t.Fatal(err) + } +} + +func TestWSDeleteConflictAndForce(t *testing.T) { + svc := newSecretsHandlerService(t, nil, nil, nil) + id := seedGlobalViaService(t, svc, "MY_TOKEN", "private-value") + svc.SetReferenceChecker(func(context.Context, string) ([]Reference, error) { + return []Reference{{Kind: "executor_profile", ID: "exec", Name: "Local", Key: "MY_TOKEN"}}, nil + }) + h := newWSHandler(t, svc) + msg := &ws.Message{ID: "1", Action: ws.ActionSecretDelete, Payload: json.RawMessage(`{"id":"` + id + `"}`)} + resp, err := h.wsDelete(context.Background(), msg) + if err != nil { + t.Fatal(err) + } + if got := decodeWSError(t, resp); got.Code != ws.ErrorCodeConflict { + t.Fatalf("error = %+v", got) + } + if !strings.Contains(string(resp.Payload), `"references"`) { + t.Fatalf("missing references: %s", resp.Payload) + } + msg.Payload = json.RawMessage(`{"id":"` + id + `","force":true}`) + resp, err = h.wsDelete(context.Background(), msg) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(resp.Payload), `"success":true`) { + t.Fatalf("forced result = %s", resp.Payload) + } +} diff --git a/apps/backend/internal/secrets/handlers.go b/apps/backend/internal/secrets/handlers.go index 00d93983a7..b09fc039b3 100644 --- a/apps/backend/internal/secrets/handlers.go +++ b/apps/backend/internal/secrets/handlers.go @@ -37,6 +37,7 @@ func (h *Handler) registerHTTP(router *gin.Engine) { api.POST("/secrets", h.httpCreateSecret) api.GET("/secrets", h.httpListSecrets) api.GET("/secrets/:id", h.httpGetSecret) + api.GET("/secrets/:id/references", h.httpListSecretReferences) api.PUT("/secrets/:id", h.httpUpdateSecret) api.DELETE("/secrets/:id", h.httpDeleteSecret) api.POST("/secrets/:id/reveal", h.httpRevealSecret) @@ -104,6 +105,31 @@ func (h *Handler) httpGetSecret(c *gin.Context) { c.JSON(http.StatusOK, secret) } +// httpListSecretReferences handles GET /api/v1/secrets/:id/references. +func (h *Handler) httpListSecretReferences(c *gin.Context) { + id := c.Param("id") + var refs []Reference + var err error + if workspaceID := c.Query("workspace_id"); workspaceID != "" { + refs, err = h.service.WorkspaceSecretReferences(c.Request.Context(), id, workspaceID) + } else { + refs, err = h.service.References(c.Request.Context(), id) + } + if err != nil { + if errors.Is(err, ErrNotFound) || errors.Is(err, ErrWorkspaceAccessDenied) { + c.JSON(http.StatusNotFound, gin.H{"error": "secret not found"}) + return + } + h.logger.Error("failed to list secret references", zap.String("id", id), zap.Error(err)) + c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to list secret references"}) + return + } + if refs == nil { + refs = []Reference{} + } + c.JSON(http.StatusOK, gin.H{"references": refs}) +} + // httpUpdateSecret handles PUT /api/v1/secrets/:id. func (h *Handler) httpUpdateSecret(c *gin.Context) { id := c.Param("id") @@ -126,7 +152,12 @@ func (h *Handler) httpUpdateSecret(c *gin.Context) { func (h *Handler) httpDeleteSecret(c *gin.Context) { id := c.Param("id") if err := h.deleteSecret(c, id); err != nil { - c.JSON(http.StatusNotFound, gin.H{"error": err.Error()}) + status, _, message, details := classifyDeleteError(err) + if status == http.StatusInternalServerError { + h.logger.Error("failed to delete secret", zap.String("id", id), zap.Error(err)) + } + details["error"] = message + c.JSON(status, details) return } c.Status(http.StatusNoContent) @@ -204,13 +235,15 @@ func (h *Handler) wsDelete(ctx context.Context, msg *ws.Message) (*ws.Message, e var payload struct { ID string `json:"id"` WorkspaceID string `json:"workspace_id"` + Force bool `json:"force"` } if err := msg.ParsePayload(&payload); err != nil { return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, "invalid payload: "+err.Error(), nil) } - if err := h.deleteSecretForWorkspace(ctx, payload.ID, payload.WorkspaceID); err != nil { - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeNotFound, err.Error(), nil) + if err := h.deleteSecretForWorkspace(ctx, payload.ID, payload.WorkspaceID, payload.Force); err != nil { + _, code, message, details := classifyDeleteError(err) + return ws.NewError(msg.ID, msg.Action, code, message, details) } return ws.NewResponse(msg.ID, msg.Action, map[string]bool{"success": true}) } @@ -262,19 +295,16 @@ func (h *Handler) updateSecretForWorkspace(ctx context.Context, id, workspaceID // deleteSecret deletes a secret, resolving workspace-scoped access from the // request's workspace_id query parameter. func (h *Handler) deleteSecret(c *gin.Context, id string) error { - if workspaceID := c.Query("workspace_id"); workspaceID != "" { - return h.service.DeleteWorkspaceSecret(c.Request.Context(), id, workspaceID) - } - return h.service.Delete(c.Request.Context(), id) + return h.deleteSecretForWorkspace(c.Request.Context(), id, c.Query("workspace_id"), c.Query("force") == "true") } // deleteSecretForWorkspace deletes a secret, targeting the workspace when // workspaceID is non-empty and falling back to the global scope otherwise. -func (h *Handler) deleteSecretForWorkspace(ctx context.Context, id, workspaceID string) error { +func (h *Handler) deleteSecretForWorkspace(ctx context.Context, id, workspaceID string, force bool) error { if workspaceID != "" { - return h.service.DeleteWorkspaceSecret(ctx, id, workspaceID) + return h.service.DeleteWorkspaceSecret(ctx, id, workspaceID, force) } - return h.service.Delete(ctx, id) + return h.service.Delete(ctx, id, force) } // revealSecret returns a secret's value, resolving workspace-scoped access diff --git a/apps/backend/internal/secrets/service.go b/apps/backend/internal/secrets/service.go index 9559292de5..dbcd5f397c 100644 --- a/apps/backend/internal/secrets/service.go +++ b/apps/backend/internal/secrets/service.go @@ -14,6 +14,7 @@ type Service struct { logger *logger.Logger workspaceAuthorizer func(context.Context, string) error workspaceExistence func(context.Context, string) error + referenceChecker func(context.Context, string) ([]Reference, error) } // NewService creates a new secrets service. @@ -239,28 +240,49 @@ func (s *Service) UpdateWorkspaceSecret(ctx context.Context, id, workspaceID str } // Delete removes a secret. -func (s *Service) Delete(ctx context.Context, id string) error { +func (s *Service) Delete(ctx context.Context, id string, force ...bool) error { if _, err := s.Get(ctx, id); err != nil { return err } - return s.store.Delete(ctx, id) + forceDelete := len(force) > 0 && force[0] + return s.deleteChecked(ctx, id, "", forceDelete) } // DeleteForWorkspace deletes a Global or same-workspace secret. -func (s *Service) DeleteForWorkspace(ctx context.Context, id, workspaceID string) error { +func (s *Service) DeleteForWorkspace(ctx context.Context, id, workspaceID string, force ...bool) error { if _, err := s.GetForWorkspace(ctx, id, workspaceID); err != nil { return err } - return s.store.Delete(ctx, id) + forceDelete := len(force) > 0 && force[0] + return s.deleteChecked(ctx, id, workspaceID, forceDelete) } // DeleteWorkspaceSecret deletes a Workspace secret after checking the // caller's workspace access. -func (s *Service) DeleteWorkspaceSecret(ctx context.Context, id, workspaceID string) error { +func (s *Service) DeleteWorkspaceSecret(ctx context.Context, id, workspaceID string, force ...bool) error { if _, err := s.GetWorkspaceSecret(ctx, id, workspaceID); err != nil { return err } - return s.store.Delete(ctx, id) + forceDelete := len(force) > 0 && force[0] + return s.deleteChecked(ctx, id, workspaceID, forceDelete) +} + +// References returns the environment bindings for a Global secret without +// exposing the secret value or changing stored state. +func (s *Service) References(ctx context.Context, id string) ([]Reference, error) { + if _, err := s.Get(ctx, id); err != nil { + return nil, err + } + return s.listReferences(ctx, id) +} + +// WorkspaceSecretReferences returns the environment bindings for a Workspace +// secret after checking the caller's workspace access. +func (s *Service) WorkspaceSecretReferences(ctx context.Context, id, workspaceID string) ([]Reference, error) { + if _, err := s.GetWorkspaceSecret(ctx, id, workspaceID); err != nil { + return nil, err + } + return s.listReferences(ctx, id) } // List returns all secrets without values. diff --git a/apps/backend/internal/secrets/store.go b/apps/backend/internal/secrets/store.go index ca189409fa..f48ba20468 100644 --- a/apps/backend/internal/secrets/store.go +++ b/apps/backend/internal/secrets/store.go @@ -103,6 +103,12 @@ type ScopedSecretStore interface { DeleteWorkspaceSecrets(ctx context.Context, workspaceID string) error } +// WorkspaceSecretDeleter permits scoped deletion through stores whose default +// Delete method intentionally accepts Global secrets only. +type WorkspaceSecretDeleter interface { + DeleteForWorkspace(ctx context.Context, id, workspaceID string) error +} + // SecretTransferStore is implemented by stores that can atomically copy or // move a secret into another scope/workspace. It is a narrow optional // interface, separate from ScopedSecretStore, so scope-aware read and diff --git a/apps/backend/internal/secrets/user_visible_store.go b/apps/backend/internal/secrets/user_visible_store.go index b45d8473fc..a5a2f282b1 100644 --- a/apps/backend/internal/secrets/user_visible_store.go +++ b/apps/backend/internal/secrets/user_visible_store.go @@ -105,6 +105,15 @@ func (s *UserVisibleStore) Delete(ctx context.Context, id string) error { return s.store.Delete(ctx, id) } +// DeleteForWorkspace removes a visible Global or same-workspace secret after +// the service has authorized access to the requested workspace. +func (s *UserVisibleStore) DeleteForWorkspace(ctx context.Context, id, workspaceID string) error { + if _, err := s.GetForWorkspace(ctx, id, workspaceID); err != nil { + return err + } + return s.store.Delete(ctx, id) +} + // List returns all user-visible (non-internal) secrets. func (s *UserVisibleStore) List(ctx context.Context) ([]*SecretListItem, error) { if s.scoped != nil { diff --git a/apps/web/components/settings/secret-delete-error.test.ts b/apps/web/components/settings/secret-delete-error.test.ts new file mode 100644 index 0000000000..3b8227c49c --- /dev/null +++ b/apps/web/components/settings/secret-delete-error.test.ts @@ -0,0 +1,67 @@ +import { describe, expect, it } from "vitest"; +import { ApiError } from "@/lib/api/client"; +import { i18n } from "@/lib/i18n"; +import { secretDeleteErrorMessage, secretReferencesFromError } from "./secret-delete-error"; + +const t = i18n.getFixedT("en"); +const PRIVATE_DETAILS = "private details"; + +describe("secretDeleteErrorMessage", () => { + it("localizes known reference kinds and preserves hidden metadata", () => { + const error = new ApiError(PRIVATE_DETAILS, 409, { + code: "secret_in_use", + references: [ + { kind: "executor_profile", name: "Local", key: "TOKEN" }, + { kind: "repository", name: "App", key: "APP_TOKEN" }, + { kind: "repository" }, + ], + }); + expect(secretDeleteErrorMessage(error, t)).toBe( + 'This secret is in use by: Executor profile "Local" (TOKEN), Repository "App" (APP_TOKEN), A resource you cannot access. Remove or replace these references before deleting it.', + ); + }); + + it.each([null, {}, { references: [] }, { code: "different_conflict" }])( + "does not expose raw error text for an unknown response %j", + (body) => { + expect(secretDeleteErrorMessage(new ApiError(PRIVATE_DETAILS, 409, body), t)).toBe( + "Couldn't delete secret", + ); + }, + ); + + it("handles missing and malformed references without losing the conflict reason", () => { + for (const references of [ + undefined, + null, + "bad", + [null, {}, { kind: "unknown", name: "private", key: "x" }], + ]) { + const error = new ApiError(PRIVATE_DETAILS, 409, { code: "secret_in_use", references }); + expect(secretDeleteErrorMessage(error, t)).toBe( + "This secret is in use. Remove or replace its references before deleting it.", + ); + } + }); +}); + +describe("secretReferencesFromError", () => { + it("returns structured references from an in-use delete race", () => { + const references = [{ kind: "repository", id: "repo-1", name: "App", key: "DEPLOY_TOKEN" }]; + + expect( + secretReferencesFromError( + new ApiError(PRIVATE_DETAILS, 409, { code: "secret_in_use", references }), + ), + ).toEqual(references); + }); + + it("rejects malformed and unrelated errors", () => { + expect(secretReferencesFromError(new Error(PRIVATE_DETAILS))).toBeNull(); + expect( + secretReferencesFromError( + new ApiError(PRIVATE_DETAILS, 409, { code: "secret_in_use", references: "bad" }), + ), + ).toBeNull(); + }); +}); diff --git a/apps/web/components/settings/secret-delete-error.ts b/apps/web/components/settings/secret-delete-error.ts new file mode 100644 index 0000000000..051755ae81 --- /dev/null +++ b/apps/web/components/settings/secret-delete-error.ts @@ -0,0 +1,57 @@ +import type { TFunction } from "i18next"; +import { ApiError } from "@/lib/api/client"; +import type { SecretReference } from "@/lib/types/http-secrets"; + +export function secretReferencesFromError(error: unknown): SecretReference[] | null { + if (!(error instanceof ApiError) || error.status !== 409) return null; + const body = error.body; + if (!body || typeof body !== "object" || !("code" in body) || body.code !== "secret_in_use") { + return null; + } + if (!("references" in body) || !Array.isArray(body.references)) return null; + return body.references.filter(isSecretReference); +} + +/** Converts the structured conflict into localized copy without showing raw server errors. */ +export function secretDeleteErrorMessage(error: unknown, t: TFunction): string { + if (!isSecretInUseError(error)) return t("settings:secretDeleteFailed"); + const refs = secretReferencesFromError(error) ?? []; + const labels = refs.map((ref) => secretReferenceLabel(ref, t)); + return labels.length + ? t("settings:secretInUse", { references: labels.join(", ") }) + : t("settings:secretInUseUnknown"); +} + +export function secretReferenceLabel(ref: SecretReference, t: TFunction): string { + const name = ref.name ?? ""; + const key = ref.key ?? ""; + if (!name && !key) return t("settings:secretReferenceHidden"); + if (!name || !key) return t("settings:secretReferenceHidden"); + switch (ref.kind) { + case "agent_profile": + return t("settings:secretReferenceAgent", { name, key }); + case "executor_profile": + return t("settings:secretReferenceExecutor", { name, key }); + case "repository": + return t("settings:secretReferenceRepository", { name, key }); + default: + return t("settings:secretReferenceHidden"); + } +} + +function isSecretReference(value: unknown): value is SecretReference { + if (!value || typeof value !== "object") return false; + const ref = value as Record; + return ( + ref.kind === "agent_profile" || ref.kind === "executor_profile" || ref.kind === "repository" + ); +} + +function isSecretInUseError(error: unknown): error is ApiError { + const body = error instanceof ApiError ? error.body : null; + return ( + error instanceof ApiError && + error.status === 409 && + Boolean(body && typeof body === "object" && "code" in body && body.code === "secret_in_use") + ); +} diff --git a/apps/web/components/settings/secrets-delete-dialog.tsx b/apps/web/components/settings/secrets-delete-dialog.tsx index da903c5703..40f24f1b69 100644 --- a/apps/web/components/settings/secrets-delete-dialog.tsx +++ b/apps/web/components/settings/secrets-delete-dialog.tsx @@ -1,10 +1,23 @@ "use client"; import type { RefObject } from "react"; +import type { TFunction } from "i18next"; import { Trans, useTranslation } from "react-i18next"; +import { IconAlertTriangle, IconGitBranch, IconRobot, IconServer } from "@tabler/icons-react"; +import { Badge } from "@kandev/ui/badge"; +import { + AlertDialog, + AlertDialogCancel, + AlertDialogContent, + AlertDialogDescription, + AlertDialogFooter, + AlertDialogHeader, + AlertDialogTitle, +} from "@kandev/ui/alert-dialog"; import { ActionConfirmPopover } from "@/components/confirmation/action-confirm-popover"; -import type { SecretListItem } from "@/lib/types/http-secrets"; +import type { SecretListItem, SecretReference } from "@/lib/types/http-secrets"; +import { secretReferenceLabel } from "./secret-delete-error"; type SecretDeleteConfirmationProps = { secret: SecretListItem; @@ -13,6 +26,7 @@ type SecretDeleteConfirmationProps = { onOpenChange: (open: boolean) => void; onCancel: () => void; onConfirm: () => void | Promise; + loading?: boolean; }; /** Anchors secret deletion confirmation to its row action on fine pointers. */ @@ -23,6 +37,7 @@ export function SecretDeleteConfirmation({ onOpenChange, onCancel, onConfirm, + loading = false, }: SecretDeleteConfirmationProps) { const { t } = useTranslation(); @@ -32,16 +47,21 @@ export function SecretDeleteConfirmation({ anchorRef={anchorRef} title={t("settings:deleteSecret")} description={ - - This will permanently remove{" "} - {secret.name}. This action cannot be - undone. - + loading ? ( + t("settings:checkingSecretReferences") + ) : ( + + This will permanently remove{" "} + {secret.name}. This action cannot + be undone. + + ) } cancelLabel={t("settings:cancel")} confirmLabel={t("settings:deleteSecret")} confirmAriaLabel={t("settings:deleteSecretNamed", { name: secret.name })} confirmTestId="secret-delete-confirm" + confirmDisabled={loading} testId="secret-delete-confirm-popover" onOpenChange={onOpenChange} onCancel={onCancel} @@ -49,3 +69,111 @@ export function SecretDeleteConfirmation({ /> ); } + +type SecretDeleteConflictDialogProps = { + secret: SecretListItem | null; + references: SecretReference[]; + onClose: () => void; +}; + +/** Lists every visible blocker before an in-use secret can be deleted. */ +export function SecretDeleteConflictDialog({ + secret, + references, + onClose, +}: SecretDeleteConflictDialogProps) { + const { t } = useTranslation(); + return ( + !open && onClose()}> + + +
+ + + {t("settings:cannotDeleteSecret")} +
+ + +
+

{t("settings:secretInUseUnknown")}

+
    + {references.map((reference, index) => ( + + ))} +
+
+ + + + {t("common:close")} + + + + + ); +} + +function SecretReferenceCard({ reference }: { reference: SecretReference }) { + const { t } = useTranslation(); + const detailsVisible = Boolean(reference.name && reference.key); + const name = detailsVisible ? reference.name : secretReferenceLabel(reference, t); + + return ( +
  • + +
    +
    + + {name} + + {detailsVisible ? ( + + {secretReferenceTypeLabel(reference.kind, t)} + + ) : null} +
    + {detailsVisible ? ( + + {reference.key} + + ) : null} +
    +
    • + ); +} + +function SecretReferenceIcon({ kind }: { kind: SecretReference["kind"] }) { + let Icon = IconGitBranch; + if (kind === "agent_profile") Icon = IconRobot; + if (kind === "executor_profile") Icon = IconServer; + return ( + + + ); +} + +function secretReferenceTypeLabel(kind: SecretReference["kind"], t: TFunction) { + switch (kind) { + case "agent_profile": + return t("settings:secretReferenceTypeAgent"); + case "executor_profile": + return t("settings:secretReferenceTypeExecutor"); + case "repository": + return t("settings:secretReferenceTypeRepository"); + } +} diff --git a/apps/web/components/settings/secrets-list-item-row.tsx b/apps/web/components/settings/secrets-list-item-row.tsx index 436857820a..738b381bdd 100644 --- a/apps/web/components/settings/secrets-list-item-row.tsx +++ b/apps/web/components/settings/secrets-list-item-row.tsx @@ -23,6 +23,8 @@ type SecretListItemRowProps = { showCreate: boolean; isEditing: boolean; isDeleteConfirming: boolean; + isDeleteLoading?: boolean; + isDeleteBlocked?: boolean; }; type SecretListItemRowActionsProps = { @@ -37,6 +39,8 @@ type SecretListItemRowActionsProps = { showCreate: boolean; isEditing: boolean; isDeleteConfirming: boolean; + isDeleteLoading: boolean; + isDeleteBlocked: boolean; isFinePointer: boolean; revealed: boolean; revealing: boolean; @@ -50,6 +54,8 @@ type SecretListItemRowDeleteActionProps = { onDeleteConfirm: () => void | Promise; isBusy: boolean; isDeleteConfirming: boolean; + isDeleteLoading: boolean; + isDeleteBlocked: boolean; isFinePointer: boolean; deleteAnchorRef: RefObject; }; @@ -67,6 +73,8 @@ export function SecretListItemRow({ showCreate, isEditing, isDeleteConfirming, + isDeleteLoading = false, + isDeleteBlocked = false, }: SecretListItemRowProps) { const { t } = useTranslation(); const { isFinePointer } = useResponsiveBreakpoint(); @@ -124,6 +132,8 @@ export function SecretListItemRow({ showCreate={showCreate} isEditing={isEditing} isDeleteConfirming={isDeleteConfirming} + isDeleteLoading={isDeleteLoading} + isDeleteBlocked={isDeleteBlocked} isFinePointer={isFinePointer} revealed={revealed} revealing={revealing} @@ -151,6 +161,8 @@ function SecretListItemRowActions({ showCreate, isEditing, isDeleteConfirming, + isDeleteLoading, + isDeleteBlocked, isFinePointer, revealed, revealing, @@ -211,6 +223,8 @@ function SecretListItemRowActions({ onDeleteConfirm={onDeleteConfirm} isBusy={isBusy} isDeleteConfirming={isDeleteConfirming} + isDeleteLoading={isDeleteLoading} + isDeleteBlocked={isDeleteBlocked} isFinePointer={isFinePointer} deleteAnchorRef={deleteAnchorRef} /> @@ -226,6 +240,8 @@ function SecretListItemRowDeleteAction({ onDeleteConfirm, isBusy, isDeleteConfirming, + isDeleteLoading, + isDeleteBlocked, isFinePointer, deleteAnchorRef, }: SecretListItemRowDeleteActionProps) { @@ -239,19 +255,24 @@ function SecretListItemRowDeleteAction({ testId="secret-delete-inline-confirmation" ariaLabel={t("settings:deleteSecretNamed", { name: secret.name })} description={ - - This will permanently remove{" "} - {secret.name}. This action cannot - be undone. - + isDeleteLoading ? ( + t("settings:checkingSecretReferences") + ) : ( + + This will permanently remove{" "} + {secret.name}. This action + cannot be undone. + + ) } cancelLabel={t("settings:cancel")} confirmLabel={t("settings:deleteSecret")} confirmAriaLabel={t("settings:deleteSecretNamed", { name: secret.name })} confirmTestId="secret-delete-confirm" + confirmDisabled={isDeleteLoading} onCancel={onDeleteCancel} onClose={onDeleteCancel} onConfirm={onDeleteConfirm} @@ -269,7 +290,7 @@ function SecretListItemRowDeleteAction({ )} - {isFinePointer ? ( + {isFinePointer && !isDeleteBlocked ? ( ) : null} diff --git a/apps/web/components/settings/secrets-settings.test.ts b/apps/web/components/settings/secrets-settings.test.ts index bfaf2f249c..14111df174 100644 --- a/apps/web/components/settings/secrets-settings.test.ts +++ b/apps/web/components/settings/secrets-settings.test.ts @@ -2,9 +2,11 @@ import { cleanup, fireEvent, render, screen, waitFor, within } from "@testing-li import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { createElement, type ReactNode } from "react"; import type { SecretListItem } from "@/lib/types/http-secrets"; +import { ApiError } from "@/lib/api/client"; const mocks = vi.hoisted(() => ({ deleteSecret: vi.fn(), + listSecretReferences: vi.fn(), addSecret: vi.fn(), updateSecret: vi.fn(), removeSecret: vi.fn(), @@ -17,6 +19,7 @@ vi.mock("@/lib/api/domains/secrets-api", () => ({ createSecret: vi.fn(), updateSecret: mocks.updateSecret, deleteSecret: mocks.deleteSecret, + listSecretReferences: mocks.listSecretReferences, })); vi.mock("@/hooks/domains/settings/use-secrets", () => ({ useSecrets: () => ({ @@ -62,11 +65,21 @@ function renderSettings() { render(createElement(SecretsSettings, { initialItems: mocks.items })); } +async function openDeleteConfirmation() { + fireEvent.click(screen.getByRole("button", { name: DELETE_BUTTON })); + const popover = await screen.findByTestId(CONFIRM_POPOVER_TEST_ID); + const confirm = within(popover).getByTestId(CONFIRM_TEST_ID) as HTMLButtonElement; + await waitFor(() => expect(confirm.disabled).toBe(false)); + return popover; +} + beforeEach(() => { mocks.items = [secret]; mocks.responsive.isFinePointer = true; mocks.deleteSecret.mockReset(); mocks.deleteSecret.mockResolvedValue(undefined); + mocks.listSecretReferences.mockReset(); + mocks.listSecretReferences.mockResolvedValue([]); mocks.removeSecret.mockReset(); mocks.toast.mockReset(); }); @@ -91,12 +104,70 @@ describe("getSecretDraftMeta", () => { }); }); -describe("SecretsSettings deletion lifecycle", () => { - it("cancels locally without dispatching deletion", () => { +describe("SecretsSettings deletion preflight", () => { + it("keeps deletion disabled while checking and ignores a cancelled lookup", async () => { + let resolveReferences!: (references: unknown[]) => void; + mocks.listSecretReferences.mockReturnValue( + new Promise((resolve) => { + resolveReferences = resolve; + }), + ); renderSettings(); fireEvent.click(screen.getByRole("button", { name: DELETE_BUTTON })); - const popover = screen.getByTestId(CONFIRM_POPOVER_TEST_ID); + const popover = await screen.findByTestId(CONFIRM_POPOVER_TEST_ID); + expect(within(popover).getByText("Checking where this secret is used…")).toBeTruthy(); + expect((within(popover).getByTestId(CONFIRM_TEST_ID) as HTMLButtonElement).disabled).toBe(true); + + fireEvent.click(within(popover).getByRole("button", { name: "Cancel" })); + resolveReferences([{ kind: "repository", id: "repo-1", name: "App", key: "DEPLOY_TOKEN" }]); + + await waitFor(() => expect(screen.queryByTestId(CONFIRM_POPOVER_TEST_ID)).toBeNull()); + expect(screen.queryByTestId("secret-delete-conflict-dialog")).toBeNull(); + expect(mocks.deleteSecret).not.toHaveBeenCalled(); + }); + + it("shows references before deletion and does not offer an unsafe delete", async () => { + mocks.listSecretReferences.mockResolvedValue([ + { kind: "agent_profile", id: "profile-1", name: "Claude review", key: "MY_TOKEN" }, + ]); + renderSettings(); + + fireEvent.click(screen.getByRole("button", { name: DELETE_BUTTON })); + + const dialog = await screen.findByTestId("secret-delete-conflict-dialog"); + expect(within(dialog).getByText(/Claude review/)).toBeTruthy(); + expect(within(dialog).getByText(/MY_TOKEN/)).toBeTruthy(); + expect(within(dialog).queryByTestId(CONFIRM_TEST_ID)).toBeNull(); + expect(mocks.deleteSecret).not.toHaveBeenCalled(); + }); + + it("shows fresh references when deletion loses a race", async () => { + mocks.deleteSecret.mockRejectedValue( + new ApiError("private server details", 409, { + code: "secret_in_use", + references: [ + { kind: "agent_profile", id: "profile-1", name: "Claude review", key: "MY_TOKEN" }, + ], + }), + ); + renderSettings(); + await openDeleteConfirmation(); + fireEvent.click( + within(screen.getByTestId(CONFIRM_POPOVER_TEST_ID)).getByTestId(CONFIRM_TEST_ID), + ); + const dialog = await screen.findByTestId("secret-delete-conflict-dialog"); + expect(within(dialog).getByText(/Claude review/)).toBeTruthy(); + expect(mocks.removeSecret).not.toHaveBeenCalled(); + expect(document.body.textContent).not.toContain("private server details"); + }); +}); + +describe("SecretsSettings confirmed deletion", () => { + it("cancels locally without dispatching deletion", async () => { + renderSettings(); + + const popover = await openDeleteConfirmation(); fireEvent.click(within(popover).getByRole("button", { name: "Cancel" })); expect(mocks.deleteSecret).not.toHaveBeenCalled(); @@ -110,7 +181,7 @@ describe("SecretsSettings deletion lifecycle", () => { }); renderSettings(); - fireEvent.click(screen.getByRole("button", { name: DELETE_BUTTON })); + await openDeleteConfirmation(); fireEvent.click( within(screen.getByTestId(CONFIRM_POPOVER_TEST_ID)).getByTestId(CONFIRM_TEST_ID), ); @@ -129,7 +200,7 @@ describe("SecretsSettings deletion lifecycle", () => { ); renderSettings(); - fireEvent.click(screen.getByRole("button", { name: DELETE_BUTTON })); + await openDeleteConfirmation(); fireEvent.click( within(screen.getByTestId(CONFIRM_POPOVER_TEST_ID)).getByTestId(CONFIRM_TEST_ID), ); @@ -146,7 +217,7 @@ describe("SecretsSettings deletion lifecycle", () => { mocks.deleteSecret.mockRejectedValue(new Error("server details must stay out of copy")); renderSettings(); - fireEvent.click(screen.getByRole("button", { name: DELETE_BUTTON })); + await openDeleteConfirmation(); fireEvent.click( within(screen.getByTestId(CONFIRM_POPOVER_TEST_ID)).getByTestId(CONFIRM_TEST_ID), ); diff --git a/apps/web/components/settings/secrets-settings.tsx b/apps/web/components/settings/secrets-settings.tsx index 307e5a8bd1..f918beda23 100644 --- a/apps/web/components/settings/secrets-settings.tsx +++ b/apps/web/components/settings/secrets-settings.tsx @@ -10,11 +10,23 @@ import { useAppStore } from "@/components/state-provider"; import { useToast } from "@/components/toast-provider"; import { useSecrets } from "@/hooks/domains/settings/use-secrets"; import type { ApiRequestOptions } from "@/lib/api/client"; -import { createSecret, updateSecret, deleteSecret } from "@/lib/api/domains/secrets-api"; +import { + createSecret, + updateSecret, + deleteSecret, + listSecretReferences, +} from "@/lib/api/domains/secrets-api"; import { useRequest } from "@/lib/http/use-request"; -import type { SecretListItem, SecretScope, UpdateSecretRequest } from "@/lib/types/http-secrets"; +import type { + SecretListItem, + SecretReference, + SecretScope, + UpdateSecretRequest, +} from "@/lib/types/http-secrets"; import { SecretForm, defaultFormState, type SecretFormState } from "./secret-form"; import { SecretListItemRow } from "./secrets-list-item-row"; +import { secretDeleteErrorMessage, secretReferencesFromError } from "./secret-delete-error"; +import { SecretDeleteConflictDialog } from "./secrets-delete-dialog"; import { CopyMoveSecretDialog, type CopyMoveMode } from "./copy-move-secret-dialog"; /* ------------------------------------------------------------------ */ @@ -37,6 +49,7 @@ function useSecretsState( const [showCreate, setShowCreate] = useState(false); const [formState, setFormState] = useState(defaultFormState); const [deleteTarget, setDeleteTarget] = useState(null); + const [deleteReferences, setDeleteReferences] = useState(null); const [transferTarget, setTransferTarget] = useState(null); const [transferOpen, setTransferOpen] = useState(false); @@ -56,6 +69,8 @@ function useSecretsState( setFormState, deleteTarget, setDeleteTarget, + deleteReferences, + setDeleteReferences, transferTarget, setTransferTarget, transferOpen, @@ -134,10 +149,62 @@ function useSecretRequests( return { createRequest, updateRequest, deleteRequest }; } -/** Composes request runners and UI actions (create, edit, delete, transfer) from the secrets state. */ -function useSecretsActions(state: ReturnType) { +/** Owns reference preflight, cancellation ordering, and the final guarded deletion. */ +function useSecretDeleteActions( + state: ReturnType, + requestOptions: ApiRequestOptions, + deleteRequest: ReturnType["deleteRequest"], +) { const { t } = useTranslation(); const { toast } = useToast(); + const requestGeneration = useRef(0); + const { deleteTarget, deleteReferences, setDeleteTarget, setDeleteReferences } = state; + const closeDelete = () => { + requestGeneration.current += 1; + setDeleteTarget(null); + setDeleteReferences(null); + }; + const openDelete = async (secret: SecretListItem) => { + const generation = requestGeneration.current + 1; + requestGeneration.current = generation; + setDeleteTarget(secret); + setDeleteReferences(null); + try { + const references = await listSecretReferences(secret.id, requestOptions); + if (requestGeneration.current !== generation) return; + setDeleteReferences(references); + } catch { + if (requestGeneration.current !== generation) return; + setDeleteTarget(null); + setDeleteReferences(null); + toast({ description: t("settings:secretReferencesLoadFailed"), variant: "error" }); + } + }; + const confirmDelete = () => { + if (!deleteTarget || deleteReferences === null || deleteReferences.length > 0) return; + const target = deleteTarget; + setDeleteTarget(null); + setDeleteReferences(null); + return deleteRequest.run(target.id).catch((error: unknown) => { + const references = secretReferencesFromError(error); + if (references !== null) { + setDeleteTarget(target); + setDeleteReferences(references); + return; + } + toast({ description: secretDeleteErrorMessage(error, t), variant: "error" }); + }); + }; + return { + referencesLoading: deleteTarget !== null && deleteReferences === null, + openDelete: (secret: SecretListItem) => void openDelete(secret), + closeDelete, + confirmDelete, + }; +} + +/** Composes request runners and UI actions (create, edit, delete, transfer) from the secrets state. */ +function useSecretsActions(state: ReturnType) { const { rememberTransferTrigger, restoreTransferTrigger } = useTransferFocusRestore(); const { addSecret: addToStore, @@ -147,8 +214,6 @@ function useSecretsActions(state: ReturnType) { setEditingId, setShowCreate, setFormState, - setDeleteTarget, - deleteTarget, formState, scope, workspaceId, @@ -182,7 +247,12 @@ function useSecretsActions(state: ReturnType) { editingId, }, ); - const isBusy = createRequest.isLoading || updateRequest.isLoading || deleteRequest.isLoading; + const deletion = useSecretDeleteActions(state, requestOptions, deleteRequest); + const isBusy = + createRequest.isLoading || + updateRequest.isLoading || + deleteRequest.isLoading || + deletion.referencesLoading; return { resetForm, @@ -209,16 +279,9 @@ function useSecretsActions(state: ReturnType) { setShowCreate(true); setFormState(defaultFormState); }, - openDelete: (secret: SecretListItem) => setDeleteTarget(secret), - closeDelete: () => setDeleteTarget(null), - confirmDelete: () => { - if (!deleteTarget) return; - const id = deleteTarget.id; - setDeleteTarget(null); - return deleteRequest.run(id).catch(() => { - toast({ description: t("settings:secretDeleteFailed"), variant: "error" }); - }); - }, + openDelete: deletion.openDelete, + closeDelete: deletion.closeDelete, + confirmDelete: deletion.confirmDelete, openTransfer: (secret: SecretListItem) => { // Remember the focused Copy/Move button; close restores it because // Radix targets a stale node when the list re-renders mid-close. @@ -285,6 +348,8 @@ export type SecretsSettingsState = { setFormState: (state: SecretFormState | ((prev: SecretFormState) => SecretFormState)) => void; deleteTarget: SecretListItem | null; setDeleteTarget: (target: SecretListItem | null) => void; + deleteReferences: SecretReference[] | null; + setDeleteReferences: (references: SecretReference[] | null) => void; transferTarget: SecretListItem | null; setTransferTarget: (target: SecretListItem | null) => void; transferOpen: boolean; @@ -430,7 +495,13 @@ function SecretsSettingsBody({ isBusy={isBusy} showCreate={showCreate} isEditing={editingId === secret.id} - isDeleteConfirming={deleteTarget?.id === secret.id} + isDeleteConfirming={ + deleteTarget?.id === secret.id && (state.deleteReferences?.length ?? 0) === 0 + } + isDeleteLoading={deleteTarget?.id === secret.id && state.deleteReferences === null} + isDeleteBlocked={ + deleteTarget?.id === secret.id && Boolean(state.deleteReferences?.length) + } /> ))} @@ -489,6 +560,11 @@ function SecretsSettingsContent({ edit={edit} onFormChange={onFormChange} /> + ); } diff --git a/apps/web/e2e/helpers/api-client.ts b/apps/web/e2e/helpers/api-client.ts index 8041e34835..0fae87e467 100644 --- a/apps/web/e2e/helpers/api-client.ts +++ b/apps/web/e2e/helpers/api-client.ts @@ -981,8 +981,16 @@ export class ApiClient { return this.request("GET", `/api/v1/secrets${suffix ? `?${suffix}` : ""}`); } - async deleteSecret(secretId: string, workspaceId?: string): Promise { - const suffix = workspaceId ? `?workspace_id=${encodeURIComponent(workspaceId)}` : ""; + async deleteSecret( + secretId: string, + workspaceId?: string, + options?: { force?: boolean }, + ): Promise { + const query = new URLSearchParams(); + if (workspaceId) query.set("workspace_id", workspaceId); + if (options?.force) query.set("force", "true"); + const encodedQuery = query.toString(); + const suffix = encodedQuery ? `?${encodedQuery}` : ""; const response = await this.rawRequest("DELETE", `/api/v1/secrets/${secretId}${suffix}`); if (!response.ok) { throw new Error( diff --git a/apps/web/e2e/tests/settings/mobile-secrets-delete.spec.ts b/apps/web/e2e/tests/settings/mobile-secrets-delete.spec.ts index 25408ddaad..deeb6f548d 100644 --- a/apps/web/e2e/tests/settings/mobile-secrets-delete.spec.ts +++ b/apps/web/e2e/tests/settings/mobile-secrets-delete.spec.ts @@ -4,11 +4,20 @@ import { test, expect } from "../../fixtures/test-base"; import { assertNoDocumentHorizontalOverflow } from "../../helpers/layout-assertions"; const SECRET_VALUE = "e2e-mobile-secret-delete-redaction-value"; +const REFERENCE_KINDS = ["agent_profile", "executor_profile", "repository"] as const; function runToken() { return `${Date.now()}-${randomUUID().slice(0, 8)}`; } +function conflictReferences() { + return Array.from({ length: 16 }, (_, index) => ({ + kind: REFERENCE_KINDS[index % REFERENCE_KINDS.length], + name: index === 0 ? "E2E mobile review profile" : `E2E mobile resource ${index + 1}`, + key: index === 0 ? "E2E_MOBILE_TOKEN" : `E2E_MOBILE_TOKEN_${index + 1}`, + })); +} + test.describe("mobile-secrets-delete", () => { test("confirms inline at the target row without exposing the value", async ({ testPage, @@ -75,4 +84,67 @@ test.describe("mobile-secrets-delete", () => { await apiClient.deleteSecretIfPresent(secret.id).catch(() => undefined); } }); + + test("keeps the target row and explains an in-use conflict", async ({ + testPage, + apiClient, + prCapture, + }) => { + const name = `E2E Mobile Failed Delete Secret ${runToken()}`; + const secret = await apiClient.createSecret(name, SECRET_VALUE); + + try { + await expect + .poll(async () => (await apiClient.listSecrets()).some((item) => item.id === secret.id), { + timeout: 30_000, + }) + .toBe(true); + await testPage.goto("/settings/general/secrets"); + const row = testPage.getByTestId(`secret-row-${secret.id}`); + await testPage.route(`**/api/v1/secrets/${secret.id}/references`, async (route) => { + await route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify({ + references: conflictReferences(), + }), + }); + }); + + await row.getByRole("button", { name: `Delete secret ${name}` }).tap(); + const conflictDialog = testPage.getByTestId("secret-delete-conflict-dialog"); + await expect(conflictDialog).toBeVisible(); + const referenceList = conflictDialog.getByTestId("secret-delete-reference-list"); + const referenceCards = referenceList.getByTestId("secret-delete-reference"); + await expect(referenceCards).toHaveCount(16); + await expect(referenceCards.first()).toContainText("E2E mobile review profile"); + await expect(referenceCards.first()).toContainText("Agent profile"); + await expect(referenceCards.first()).toContainText("E2E_MOBILE_TOKEN"); + await expect(conflictDialog.getByTestId("secret-delete-confirm")).toHaveCount(0); + const listDimensions = await referenceList.evaluate((element) => ({ + clientHeight: element.clientHeight, + scrollHeight: element.scrollHeight, + })); + expect(listDimensions.scrollHeight).toBeGreaterThan(listDimensions.clientHeight); + const dialogBox = await conflictDialog.boundingBox(); + expect(dialogBox).not.toBeNull(); + expect(dialogBox!.x).toBeGreaterThanOrEqual(0); + expect(dialogBox!.x + dialogBox!.width).toBeLessThanOrEqual(testPage.viewportSize()!.width); + await assertNoDocumentHorizontalOverflow(testPage, "mobile secret deletion conflict"); + await prCapture.screenshot("mobile-secrets-delete-conflict", { + caption: "Mobile secret deletion conflict dialog", + }); + await referenceList.evaluate((element) => element.scrollTo({ top: element.scrollHeight })); + await expect(referenceCards.last()).toBeInViewport(); + const close = conflictDialog.getByRole("button", { name: "Close" }); + await expect(close).toBeVisible(); + expect((await close.boundingBox())!.height).toBeGreaterThanOrEqual(44); + await expect(row).toBeVisible(); + await expect(testPage.locator("body")).not.toContainText(SECRET_VALUE); + await expect(testPage.locator("body")).not.toContainText("secret_in_use"); + } finally { + await testPage.unroute(`**/api/v1/secrets/${secret.id}/references`); + await apiClient.deleteSecretIfPresent(secret.id).catch(() => undefined); + } + }); }); diff --git a/apps/web/e2e/tests/settings/repository-secrets.spec.ts b/apps/web/e2e/tests/settings/repository-secrets.spec.ts index 7d052589e9..a4932b7d28 100644 --- a/apps/web/e2e/tests/settings/repository-secrets.spec.ts +++ b/apps/web/e2e/tests/settings/repository-secrets.spec.ts @@ -227,7 +227,7 @@ test.describe("Repository secrets", () => { await apiClient.updateRepository(seedData.repositoryId, { secret_bindings: [{ key: "E2E_DELETED_TOKEN", secret_id: first.id }], }); - await apiClient.deleteSecret(first.id); + await apiClient.deleteSecret(first.id, undefined, { force: true }); const deletedTask = await apiClient.createTask( seedData.workspaceId, "Deleted repository secret", diff --git a/apps/web/e2e/tests/settings/secrets-delete.spec.ts b/apps/web/e2e/tests/settings/secrets-delete.spec.ts index 33ce78a538..7efa552eb9 100644 --- a/apps/web/e2e/tests/settings/secrets-delete.spec.ts +++ b/apps/web/e2e/tests/settings/secrets-delete.spec.ts @@ -3,11 +3,20 @@ import { randomUUID } from "node:crypto"; import { test, expect } from "../../fixtures/test-base"; const SECRET_VALUE = "e2e-secret-delete-redaction-value"; +const REFERENCE_KINDS = ["agent_profile", "executor_profile", "repository"] as const; function runToken() { return `${Date.now()}-${randomUUID().slice(0, 8)}`; } +function conflictReferences() { + return Array.from({ length: 18 }, (_, index) => ({ + kind: REFERENCE_KINDS[index % REFERENCE_KINDS.length], + name: index === 0 ? "E2E review profile" : `E2E resource ${index + 1}`, + key: index === 0 ? "E2E_TOKEN" : `E2E_TOKEN_${index + 1}`, + })); +} + test.describe("Secret deletion", () => { test("confirms at the target row, supports cancel, and removes the secret", async ({ testPage, @@ -55,7 +64,11 @@ test.describe("Secret deletion", () => { } }); - test("keeps the target row and reports localized failure", async ({ testPage, apiClient }) => { + test("keeps the target row and explains an in-use conflict", async ({ + testPage, + apiClient, + prCapture, + }) => { const name = `E2E Failed Delete Secret ${runToken()}`; const secret = await apiClient.createSecret(name, SECRET_VALUE); @@ -67,22 +80,42 @@ test.describe("Secret deletion", () => { .toBe(true); await testPage.goto("/settings/general/secrets"); const row = testPage.getByTestId(`secret-row-${secret.id}`); - await testPage.route(`**/api/v1/secrets/${secret.id}`, async (route) => { + await testPage.route(`**/api/v1/secrets/${secret.id}/references`, async (route) => { await route.fulfill({ - status: 500, + status: 200, contentType: "application/json", - body: JSON.stringify({ error: "synthetic delete failure" }), + body: JSON.stringify({ + references: conflictReferences(), + }), }); }); await row.getByRole("button", { name: `Delete secret ${name}` }).click(); - await testPage.getByTestId("secret-delete-confirm").click(); - - await expect(testPage.getByTestId("toast-message")).toContainText("Couldn't delete secret"); + const conflictDialog = testPage.getByTestId("secret-delete-conflict-dialog"); + await expect(conflictDialog).toBeVisible(); + await expect(testPage.getByTestId("secret-delete-confirm-popover")).toHaveCount(0); + const referenceList = conflictDialog.getByTestId("secret-delete-reference-list"); + const referenceCards = referenceList.getByTestId("secret-delete-reference"); + await expect(referenceCards).toHaveCount(18); + await expect(referenceCards.first()).toContainText("E2E review profile"); + await expect(referenceCards.first()).toContainText("Agent profile"); + await expect(referenceCards.first()).toContainText("E2E_TOKEN"); + await expect(conflictDialog.getByTestId("secret-delete-confirm")).toHaveCount(0); + const listDimensions = await referenceList.evaluate((element) => ({ + clientHeight: element.clientHeight, + scrollHeight: element.scrollHeight, + })); + expect(listDimensions.scrollHeight).toBeGreaterThan(listDimensions.clientHeight); + await prCapture.screenshot("desktop-secrets-delete-conflict", { + caption: "Desktop secret deletion conflict dialog", + }); + await referenceList.evaluate((element) => element.scrollTo({ top: element.scrollHeight })); + await expect(referenceCards.last()).toBeInViewport(); + await expect(conflictDialog.getByRole("button", { name: "Close" })).toBeVisible(); await expect(row).toBeVisible(); await expect(testPage.locator("body")).not.toContainText(SECRET_VALUE); - await expect(testPage.locator("body")).not.toContainText("synthetic delete failure"); + await expect(testPage.locator("body")).not.toContainText("secret_in_use"); } finally { - await testPage.unroute(`**/api/v1/secrets/${secret.id}`); + await testPage.unroute(`**/api/v1/secrets/${secret.id}/references`); await apiClient.deleteSecretIfPresent(secret.id); } }); diff --git a/apps/web/lib/api/domains/secrets-api.ts b/apps/web/lib/api/domains/secrets-api.ts index a1294c16a9..74ab12f386 100644 --- a/apps/web/lib/api/domains/secrets-api.ts +++ b/apps/web/lib/api/domains/secrets-api.ts @@ -6,6 +6,8 @@ import type { CopyMoveSecretRequest, RevealSecretResponse, SecretScope, + SecretReference, + SecretReferencesResponse, } from "@/lib/types/http-secrets"; /** Options for listing secrets: optional scope, workspace, and include-global filter. */ @@ -73,6 +75,18 @@ export async function deleteSecret(id: string, options?: SecretScopedOptions): P }); } +/** Lists configuration references without mutating the secret. */ +export async function listSecretReferences( + id: string, + options?: SecretScopedOptions, +): Promise { + const response = await fetchJson( + withSecretQuery(`/api/v1/secrets/${id}/references`, options), + options, + ); + return response.references ?? []; +} + /** Reveals the value of the secret with the given id. */ export async function revealSecret( id: string, diff --git a/apps/web/lib/types/http-secrets.ts b/apps/web/lib/types/http-secrets.ts index c64aa25114..7637e337b6 100644 --- a/apps/web/lib/types/http-secrets.ts +++ b/apps/web/lib/types/http-secrets.ts @@ -12,6 +12,19 @@ export interface SecretListItem { updated_at: string; } +/** A configuration resource that binds an environment key to a secret. */ +export interface SecretReference { + kind: "agent_profile" | "executor_profile" | "repository"; + id?: string; + name?: string; + key?: string; +} + +/** Response from the read-only secret-reference preflight endpoint. */ +export interface SecretReferencesResponse { + references: SecretReference[] | null; +} + /** Request payload for creating a secret. */ export interface CreateSecretRequest { name: string; diff --git a/apps/web/src/locales/en/settings.json b/apps/web/src/locales/en/settings.json index 27880c9f16..f3228db19a 100644 --- a/apps/web/src/locales/en/settings.json +++ b/apps/web/src/locales/en/settings.json @@ -127,7 +127,19 @@ "deleteLayoutProfile": "Delete layout profile", "deleteLayoutProfileNamed": "Delete {{name}}?", "deleteSecret": "Delete secret", + "cannotDeleteSecret": "Cannot delete secret", + "checkingSecretReferences": "Checking where this secret is used…", + "secretReferencesLoadFailed": "Couldn't check where this secret is used", "secretDeleteFailed": "Couldn't delete secret", + "secretInUse": "This secret is in use by: {{references}}. Remove or replace these references before deleting it.", + "secretInUseUnknown": "This secret is in use. Remove or replace its references before deleting it.", + "secretReferenceAgent": "Agent profile \"{{name}}\" ({{key}})", + "secretReferenceExecutor": "Executor profile \"{{name}}\" ({{key}})", + "secretReferenceRepository": "Repository \"{{name}}\" ({{key}})", + "secretReferenceTypeAgent": "Agent profile", + "secretReferenceTypeExecutor": "Executor profile", + "secretReferenceTypeRepository": "Repository", + "secretReferenceHidden": "A resource you cannot access", "deleteThisCustomLayoutAfterConfirmation": "Delete this custom layout after confirmation.", "desktopNotifications": "Desktop Notifications", "desktopNotificationsDescription": "Notify this device when a selected agent turn, question, or Office event occurs.", diff --git a/apps/web/src/locales/pseudo/settings.json b/apps/web/src/locales/pseudo/settings.json index 9377614a10..b035dae139 100644 --- a/apps/web/src/locales/pseudo/settings.json +++ b/apps/web/src/locales/pseudo/settings.json @@ -127,7 +127,19 @@ "deleteLayoutProfile": "Ďēĺēţē ĺàŷōũţ ƥŕōƒĩĺē", "deleteLayoutProfileNamed": "Ďēĺēţē {{name}}?", "deleteSecret": "Ďēĺēţē śēćŕēţ", + "cannotDeleteSecret": "Ćàńńōţ ďēĺēţē śēćŕēţ", + "checkingSecretReferences": "Ćĥēćķĩńĝ ŵĥēŕē ţĥĩś śēćŕēţ ĩś ũśēď…", + "secretReferencesLoadFailed": "Ćōũĺďń'ţ ćĥēćķ ŵĥēŕē ţĥĩś śēćŕēţ ĩś ũśēď", "secretDeleteFailed": "Ćōũĺďń'ţ ďēĺēţē śēćŕēţ", + "secretInUse": "Ţĥĩś śēćŕēţ ĩś ĩń ũśē ƀŷ: {{references}}. Ŕēḿōvē ōŕ ŕēƥĺàćē ţĥēśē ŕēƒēŕēńćēś ƀēƒōŕē ďēĺēţĩńĝ ĩţ.", + "secretInUseUnknown": "Ţĥĩś śēćŕēţ ĩś ĩń ũśē. Ŕēḿōvē ōŕ ŕēƥĺàćē ĩţś ŕēƒēŕēńćēś ƀēƒōŕē ďēĺēţĩńĝ ĩţ.", + "secretReferenceAgent": "Àĝēńţ ƥŕōƒĩĺē \"{{name}}\" ({{key}})", + "secretReferenceExecutor": "Ēxēćũţōŕ ƥŕōƒĩĺē \"{{name}}\" ({{key}})", + "secretReferenceRepository": "Ŕēƥōśĩţōŕŷ \"{{name}}\" ({{key}})", + "secretReferenceTypeAgent": "Àĝēńţ ƥŕōƒĩĺē", + "secretReferenceTypeExecutor": "Ēxēćũţōŕ ƥŕōƒĩĺē", + "secretReferenceTypeRepository": "Ŕēƥōśĩţōŕŷ", + "secretReferenceHidden": "À ŕēśōũŕćē ŷōũ ćàńńōţ àććēśś", "deleteThisCustomLayoutAfterConfirmation": "Ďēĺēţē ţĥĩś ćũśţōḿ ĺàŷōũţ àƒţēŕ ćōńƒĩŕḿàţĩōń.", "desktopNotifications": "Ďēśķţōƥ Ńōţĩƒĩćàţĩōńś", "desktopNotificationsDescription": "Ńōţĩƒŷ ţĥĩś ďēvĩćē ŵĥēń à śēĺēćţēď àĝēńţ ţũŕń, qũēśţĩōń, ōŕ Ōƒƒĩćē ēvēńţ ōććũŕś.", diff --git a/apps/web/src/locales/pt-pt/settings.json b/apps/web/src/locales/pt-pt/settings.json index 7b147fe714..2e608775c7 100644 --- a/apps/web/src/locales/pt-pt/settings.json +++ b/apps/web/src/locales/pt-pt/settings.json @@ -124,7 +124,19 @@ "deleteLayoutProfile": "Eliminar o perfil de esquema", "deleteLayoutProfileNamed": "Eliminar {{name}}?", "deleteSecret": "Eliminar segredo", + "cannotDeleteSecret": "Não é possível eliminar o segredo", + "checkingSecretReferences": "A verificar onde este segredo é utilizado…", + "secretReferencesLoadFailed": "Não foi possível verificar onde este segredo é utilizado", "secretDeleteFailed": "Não foi possível eliminar o segredo", + "secretInUse": "Este segredo está em uso por: {{references}}. Remova ou substitua estas referências antes de o eliminar.", + "secretInUseUnknown": "Este segredo está em uso. Remova ou substitua as suas referências antes de o eliminar.", + "secretReferenceAgent": "Perfil de agente \"{{name}}\" ({{key}})", + "secretReferenceExecutor": "Perfil de executor \"{{name}}\" ({{key}})", + "secretReferenceRepository": "Repositório \"{{name}}\" ({{key}})", + "secretReferenceTypeAgent": "Perfil de agente", + "secretReferenceTypeExecutor": "Perfil de executor", + "secretReferenceTypeRepository": "Repositório", + "secretReferenceHidden": "Um recurso ao qual não tem acesso", "deleteThisCustomLayoutAfterConfirmation": "Eliminar este esquema personalizado após confirmação.", "desktopNotifications": "Notificações no computador", "desktopNotificationsDescription": "Notificar este dispositivo quando ocorrer um turno de agente, uma pergunta ou um evento do Office selecionados.", diff --git a/apps/web/src/locales/zh-cn/settings.json b/apps/web/src/locales/zh-cn/settings.json index 5451e8f2b4..7eb79535fd 100644 --- a/apps/web/src/locales/zh-cn/settings.json +++ b/apps/web/src/locales/zh-cn/settings.json @@ -127,7 +127,19 @@ "deleteLayoutProfile": "删除布局配置", "deleteLayoutProfileNamed": "删除 {{name}}？", "deleteSecret": "删除密钥", + "cannotDeleteSecret": "无法删除密钥", + "checkingSecretReferences": "正在检查此密钥的使用位置…", + "secretReferencesLoadFailed": "无法检查此密钥的使用位置", "secretDeleteFailed": "无法删除密钥", + "secretInUse": "以下位置正在使用此密钥：{{references}}。删除密钥前，请移除或替换这些引用。", + "secretInUseUnknown": "此密钥正在使用中。删除密钥前，请移除或替换其引用。", + "secretReferenceAgent": "智能体配置“{{name}}”（{{key}}）", + "secretReferenceExecutor": "执行器配置“{{name}}”（{{key}}）", + "secretReferenceRepository": "仓库“{{name}}”（{{key}}）", + "secretReferenceTypeAgent": "智能体配置", + "secretReferenceTypeExecutor": "执行器配置", + "secretReferenceTypeRepository": "仓库", + "secretReferenceHidden": "您无权访问的资源", "deleteThisCustomLayoutAfterConfirmation": "确认后删除此自定义布局。", "desktopNotifications": "桌面通知", "desktopNotificationsDescription": "当选定的智能体回合、问题或办公室事件发生时通知此设备。", diff --git a/apps/web/src/locales/zh-hk/settings.json b/apps/web/src/locales/zh-hk/settings.json index 39c7b4ce8d..90226912d6 100644 --- a/apps/web/src/locales/zh-hk/settings.json +++ b/apps/web/src/locales/zh-hk/settings.json @@ -127,7 +127,19 @@ "deleteLayoutProfile": "刪除佈局設定", "deleteLayoutProfileNamed": "刪除 {{name}}？", "deleteSecret": "刪除密鑰", + "cannotDeleteSecret": "無法刪除密鑰", + "checkingSecretReferences": "正在檢查此密鑰的使用位置…", + "secretReferencesLoadFailed": "無法檢查此密鑰的使用位置", "secretDeleteFailed": "無法刪除密鑰", + "secretInUse": "以下位置正在使用此密鑰：{{references}}。刪除密鑰前，請移除或替換這些引用。", + "secretInUseUnknown": "此密鑰正在使用中。刪除密鑰前，請移除或替換其引用。", + "secretReferenceAgent": "代理程式設定檔「{{name}}」（{{key}}）", + "secretReferenceExecutor": "執行器設定檔「{{name}}」（{{key}}）", + "secretReferenceRepository": "儲存庫「{{name}}」（{{key}}）", + "secretReferenceTypeAgent": "代理程式設定檔", + "secretReferenceTypeExecutor": "執行器設定檔", + "secretReferenceTypeRepository": "儲存庫", + "secretReferenceHidden": "您無權存取的資源", "deleteThisCustomLayoutAfterConfirmation": "確認後刪除此自訂佈局。", "desktopNotifications": "桌面通知", "desktopNotificationsDescription": "當選定的代理程式回合、問題或辦公室事件發生時通知此設備。", diff --git a/apps/web/src/locales/zh-tw/settings.json b/apps/web/src/locales/zh-tw/settings.json index b18fa67d28..897f707096 100644 --- a/apps/web/src/locales/zh-tw/settings.json +++ b/apps/web/src/locales/zh-tw/settings.json @@ -127,7 +127,19 @@ "deleteLayoutProfile": "刪除佈局設定", "deleteLayoutProfileNamed": "刪除 {{name}}？", "deleteSecret": "刪除金鑰", + "cannotDeleteSecret": "無法刪除金鑰", + "checkingSecretReferences": "正在檢查此金鑰的使用位置…", + "secretReferencesLoadFailed": "無法檢查此金鑰的使用位置", "secretDeleteFailed": "無法刪除金鑰", + "secretInUse": "以下位置正在使用此金鑰：{{references}}。刪除金鑰前，請移除或替換這些引用。", + "secretInUseUnknown": "此金鑰正在使用中。刪除金鑰前，請移除或替換其引用。", + "secretReferenceAgent": "代理程式設定檔「{{name}}」（{{key}}）", + "secretReferenceExecutor": "執行器設定檔「{{name}}」（{{key}}）", + "secretReferenceRepository": "儲存庫「{{name}}」（{{key}}）", + "secretReferenceTypeAgent": "代理程式設定檔", + "secretReferenceTypeExecutor": "執行器設定檔", + "secretReferenceTypeRepository": "儲存庫", + "secretReferenceHidden": "您無權存取的資源", "deleteThisCustomLayoutAfterConfirmation": "確認後刪除此自訂佈局。", "desktopNotifications": "桌面通知", "desktopNotificationsDescription": "當選定的代理程式回合、問題或辦公室事件發生時通知此裝置。", diff --git a/docs/plans/secret-reference-protection/plan.md b/docs/plans/secret-reference-protection/plan.md new file mode 100644 index 0000000000..23c84d3fdf --- /dev/null +++ b/docs/plans/secret-reference-protection/plan.md @@ -0,0 +1,44 @@ +--- +created: 2026-09-08 +status: done +requirements: + - REQ-WORKSPACES-REPOSITORY-SECRETS-001 +system_design: + - ../../specs/workspaces/system-design/repository-secrets.md +legacy_specs: [] +--- + +# Secret reference protection + +## Overview + +Issue #3500 traces to unchecked secret deletion followed by strict runtime lookup of the retained secret ID. +The user authorized implementation after a confident diagnosis. The source trace confirms this path. + +## Scope and technical approach + +Protect user-facing secret deletion through `secrets.Service`, wire reference discovery in `backendapp`, and return structured HTTP/WS conflicts. +Preflight deletion through a read-only reference endpoint so the confirmation flow can show conflicts before issuing `DELETE`; retain the authoritative reference scan in the delete operation. +Add source-specific recovery guidance to runtime errors. Preserve scope checks and secret identity. +Automatic rebinding, scope-transfer changes, and the separate worktree recovery symptom are excluded. + +## Tests + +The work order owns targeted service/HTTP/WS, reference-discovery, runtime, UI, and responsive E2E regression tests for acceptance criteria .9 through .12. +Handler tests provide end-to-end API evidence, while the desktop and mobile browser tests verify the preflight conflict dialog. + +## Work orders + +- [x] [Task 01: Protect secret references](task-01-protect-references.md) + +## Verification results + +All work-order checks passed. The backend tests reported 157 passing secrets/environment tests, nine reference-checker tests, and three lifecycle tests. +The focused UI tests reported 24 passes, including preflight cancellation, conflicts, and a reference race during final deletion. TypeScript, targeted Go/frontend lint, localization, specification, and public-documentation checks passed. +The managed Chromium and Pixel 5 E2E runs passed two tests each and verified contained conflict dialogs with resource cards, list-only scrolling for long conflict sets, fixed Close actions, and no secret-value disclosure. +The first regression runs reproduced unsafe deletion and missing repair/profile information before the corresponding fixes. +Issue #3500 is assigned to `carlosflorencio`. The implementation is committed in PR #3503. + +## Risks + +The pre-delete check does not serialize concurrent profile saves. Forced deletion retains broken references by design. diff --git a/docs/plans/secret-reference-protection/task-01-protect-references.md b/docs/plans/secret-reference-protection/task-01-protect-references.md new file mode 100644 index 0000000000..9eee25a3f9 --- /dev/null +++ b/docs/plans/secret-reference-protection/task-01-protect-references.md @@ -0,0 +1,107 @@ +--- +id: "01-protect-references" +title: "Protect secret references" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-WORKSPACES-REPOSITORY-SECRETS-001 +acceptance_criteria: + - AC-WORKSPACES-REPOSITORY-SECRETS-001.9 + - AC-WORKSPACES-REPOSITORY-SECRETS-001.10 + - AC-WORKSPACES-REPOSITORY-SECRETS-001.11 + - AC-WORKSPACES-REPOSITORY-SECRETS-001.12 +system_design: + - ../../specs/workspaces/system-design/repository-secrets.md +--- + +# Task 01: Protect secret references + +## Summary + +Show secret-reference conflicts before deletion, reject unsafe deletion, and explain how to repair an existing missing reference. + +## In scope + +- Service guards, HTTP/WS conflict and force contracts, read-only reference preflight, responsive confirmation UI, runtime errors, and public documentation. + +## Out of scope + +- Automatic rebinding, scope transfers, concurrent save serialization, and worktree recovery. + +## Acceptance + +- Existing references block ordinary deletion. Force requires existing secret authorization. +- The delete flow loads direct references before confirmation and replaces destructive actions with a contained conflict dialog when any exist. +- The final delete operation repeats the reference scan so references created after preflight still block deletion. +- Lookup errors fail closed. Responses disclose no secret values or unauthorized repository details. +- Runtime errors identify the key and source and give a repair instruction. + +## Verification + +From `apps/backend`: + +```bash +go test ./internal/secrets ./internal/agent/runtime/environment +go test ./internal/backendapp -run TestSecretReference -count=1 +go test ./internal/agent/runtime/lifecycle -run 'TestSecretRecovery|TestResolveStrict' -count=1 +``` + +From the repository root: + +```bash +python3 scripts/lint-spec-files.py --all +node scripts/validate-public-docs.mjs +git diff --check +``` + +From `apps/web`: + +```bash +pnpm exec vitest run components/settings/secret-delete-error.test.ts components/settings/secrets-settings.test.ts components/settings/secrets-list-item-row.test.tsx +pnpm exec eslint components/settings/secret-delete-error.ts components/settings/secrets-delete-dialog.tsx components/settings/secrets-list-item-row.tsx components/settings/secrets-settings.tsx +pnpm run typecheck +pnpm run i18n:check +CAPTURE_PR_ASSETS=true pnpm e2e:run --host --project chromium e2e/tests/settings/secrets-delete.spec.ts +CAPTURE_PR_ASSETS=true pnpm e2e:run --host --project mobile-chrome e2e/tests/settings/mobile-secrets-delete.spec.ts +``` + +## Files likely touched + +- `apps/backend/internal/secrets/service.go`, `handlers.go`, `delete.go`, and deletion tests/helpers. +- `apps/backend/internal/backendapp/main.go` and new reference checker/tests. +- `apps/backend/internal/agent/runtime/environment/environment.go` and tests. +- `apps/backend/internal/agent/runtime/lifecycle/environment_resolution.go` and tests. +- `docs/public/agents-and-profiles.md` and `docs/public/websocket-api.md`. +- `apps/web/components/settings/secrets-settings.tsx`, `secrets-delete-dialog.tsx`, `secrets-list-item-row.tsx`, `secret-delete-error.ts`, their tests, the secret API/types, and the settings locale catalogs. + +## Dependencies + +None. + +## Risks + +Cross-owner reference reads must fail closed. Internal store deletion remains available for lifecycle cleanup. + +## Parallelism + +`sequential` + +## Inputs + +- Issue #3500 and the linked requirements and system design. +- Existing scoped secret handlers and dynamic-profile conflict responses. + +## Results + +- `go test ./internal/secrets ./internal/agent/runtime/environment`: passed, 157 tests. Eight PostgreSQL tests were skipped because no test DSN was configured. +- `go test ./internal/backendapp -run TestSecretReference -count=1`: passed, nine tests. The integration test uses real encrypted SQLite storage and all three reference owners. +- `go test ./internal/agent/runtime/lifecycle -run 'TestSecretRecovery|TestResolveStrict' -count=1`: passed, three tests. +- The listed Vitest command passed all 24 tests. TypeScript, ESLint, and `i18n:check` passed. +- The managed Chromium and Pixel 5 E2E runs passed two tests each and produced validated desktop/mobile conflict-dialog captures. Long mixed-resource lists scroll independently while the explanation and Close action remain visible. +- `golangci-lint run ./... --new-from-rev= --timeout=5m` passed with no issues. +- Specification lint, public-documentation validation, and `git diff --check` passed. +- The existing desktop and mobile secret-deletion E2E specs now cover the preflight conflict dialog, verify its resource-card presentation and long-list scrolling, confirm that no destructive action remains, and retain the secret row. +- No live instance was changed or started. Temporary databases use test cleanup. Unrelated generated translation changes were removed. +- No agents were delegated. The issue assignment was verified. The implementation is committed in PR #3503. diff --git a/docs/public/agents-and-profiles.md b/docs/public/agents-and-profiles.md index b9eba56a1a..5aab6ba901 100644 --- a/docs/public/agents-and-profiles.md +++ b/docs/public/agents-and-profiles.md @@ -357,7 +357,13 @@ Profile environment rules are: - `TASK_DESCRIPTION` and every `KANDEV_*` key are reserved; - an entry must use either a literal value or a secret reference, never both. -Secret references are resolved at process launch. A deleted, missing, or unreadable secret causes that environment entry to be omitted; Kandev does not fall back to an old value. Empty resolved values are also omitted. Profile values fill missing environment keys but do not overwrite environment supplied by the executor or Kandev runtime. +Kandev resolves secret references at process launch and cold resume. A deleted, missing, or unreadable secret blocks launch before the agent starts. The error identifies the environment key and its source. + +Secret deletion is blocked while an agent profile, executor profile, or repository environment references the secret. When you select Delete, Kandev checks first. If the secret is in use, a dialog lists the affected resources and does not offer a delete action. Remove or replace those references before deleting the secret. + +If a reference is already broken, open the named profile and select the replacement secret for the affected key, save, and retry. For a visible repository reference, open that repository's environment settings and replace the binding. A redacted `repository` reference means the repository is in a workspace that you cannot access; a workspace user with edit permission must locate and replace the binding in that workspace's repository settings. If no such user exists, ask a workspace owner or administrator to grant access or repair the binding. Creating a secret with the same name does not repair the reference because each secret has a separate ID. + +API clients can explicitly force deletion. This leaves broken references and blocks future launches until those references are repaired. See the [secret deletion API contract](websocket-api.md#settings-secrets-and-automations). Repositories can bind an environment key to a Global secret or to a Workspace secret from the same workspace under **Settings > Workspaces > _workspace_ > Repositories**. A task inherits bindings from every attached repository. Repository bindings are secret references, never values, and a repository binding to a deleted or unreadable secret blocks that task's launch. If two sources provide the same key, Kandev deduplicates an identical secret reference and rejects every other collision; repository order never chooses a winner. diff --git a/docs/public/websocket-api.md b/docs/public/websocket-api.md index cc3794ab2c..bba57d9978 100644 --- a/docs/public/websocket-api.md +++ b/docs/public/websocket-api.md @@ -516,6 +516,17 @@ automation.webhook.reveal_secret These are trusted local-administration operations. In particular, `secrets.reveal` and `automation.webhook.reveal_secret` make the lack of WebSocket authentication security-critical. +`secrets.delete` accepts `{ "id": "", "workspace_id": "", "force": false }`. +A Workspace secret requires its `workspace_id`. A referenced secret returns `CONFLICT` with `details.code: "secret_in_use"` and `details.references`. +References contain `kind` (`agent_profile`, `executor_profile`, or `repository`), `id`, `name`, and `key`. +An inaccessible workspace-scoped profile or repository exposes only its `kind`. Secret values and secret IDs never appear in conflict details. +The HTTP equivalent, `DELETE /api/v1/secrets/:id`, returns `409` with `code` and `references` at the top level. +`GET /api/v1/secrets/:id/references` performs the same authorized reference lookup without changing the secret and returns `{ "references": [...] }`. Add `?workspace_id=` for a Workspace secret. Settings uses this endpoint before it enables deletion; the later `DELETE` still repeats the check. + +With `force: true`, deletion preserves the broken bindings. Future launches fail until users repair those bindings. +The HTTP override is `?force=true`, combined with `workspace_id` for Workspace secrets. +Force does not bypass authorization. Reference lookup failures return `INTERNAL_ERROR` (HTTP `500`) and leave the secret intact. + `automation.run.stop` requires `automation_id` and `run_id`. It cancels the selected open run's exact task/session/turn binding and returns `{run_id, status}`. A stale or terminal binding returns not found; it never stops another diff --git a/docs/specs/workspaces/README.md b/docs/specs/workspaces/README.md index f248eb3ea7..fc510eef0b 100644 --- a/docs/specs/workspaces/README.md +++ b/docs/specs/workspaces/README.md @@ -49,6 +49,7 @@ Git state. ### System design +- [Secret reference protection](system-design/repository-secrets.md) - [Empty Remote Repositories](system-design/empty-remote-repositories.md) - [Improve Kandev](system-design/improve-kandev.md) - [Copy and Move Secrets Between Scopes](system-design/secret-scope-transfer.md) diff --git a/docs/specs/workspaces/requirements/repository-secrets.md b/docs/specs/workspaces/requirements/repository-secrets.md index d7650d76ab..c57ea82881 100644 --- a/docs/specs/workspaces/requirements/repository-secrets.md +++ b/docs/specs/workspaces/requirements/repository-secrets.md @@ -28,6 +28,11 @@ Repository setup and agent work often need credentials that are specific to a pr - **AC-WORKSPACES-REPOSITORY-SECRETS-001.7:** Repository bindings contain secret references only, never literal values. - **AC-WORKSPACES-REPOSITORY-SECRETS-001.8:** The effective repository environment is available to repository setup, the agent process, child shells, and terminal-panel terminals in every supported executor, including explicitly approved forwarding to SSH. +- **AC-WORKSPACES-REPOSITORY-SECRETS-001.9:** Secret deletion shall reject existing agent-profile, executor-profile, and repository environment references with a conflict response. The response identifies references without exposing secret values or unauthorized workspace-scoped reference metadata. An explicit force option permits deletion and preserves the broken bindings. +- **AC-WORKSPACES-REPOSITORY-SECRETS-001.10:** A missing secret shall block fresh launch and cold resume. The error names the environment key and source, identifies the agent profile when available, and directs the user to select the secret again. A replacement with the same name shall not repair the reference automatically. +- **AC-WORKSPACES-REPOSITORY-SECRETS-001.11:** Reference lookup failures shall block deletion with a sanitized internal error. Secret authorization shall run before reference disclosure or a forced deletion. +- **AC-WORKSPACES-REPOSITORY-SECRETS-001.12:** Settings shall check references before enabling secret deletion. Existing references shall open a contained conflict dialog that lists visible resources and offers no destructive action. The final deletion request shall repeat the authoritative reference check. + ## Migrated source detail ## Why @@ -107,8 +112,8 @@ updated_at: timestamp ``` The pair `(repository_id, key)` is unique. The repository foreign key cascades on repository or -workspace deletion. The secret reference deliberately permits a dangling ID so deleting a secret -leaves a broken binding that blocks future launch. +workspace deletion. The secret reference permits a dangling ID so forced deletion +leaves a broken binding that blocks future launch. Ordinary deletion rejects existing references. Keys follow profile environment rules: at most 100 bindings per repository, maximum key length 256, POSIX identifier syntax, no duplicates, and no `TASK_DESCRIPTION` or `KANDEV_*` keys. @@ -123,7 +128,7 @@ The existing HTTP and WebSocket secret CRUD contracts become scope-aware. - The default list returns Global secrets. A Workspace-filtered list returns that workspace's secrets, with an explicit option to include visible Global secrets for a repository selector. - Update changes name and/or value only; scope is immutable. -- Reveal and delete preserve existing response shapes while applying scope authorization. +- Reveal preserves its response shape while applying scope authorization. A read-only endpoint returns deletion references after applying the same scope authorization. Delete returns a conflict for an in-use secret unless explicitly forced. Repository create, get, list, update, and repository events include: diff --git a/docs/specs/workspaces/system-design/repository-secrets.md b/docs/specs/workspaces/system-design/repository-secrets.md new file mode 100644 index 0000000000..53e16bab8d --- /dev/null +++ b/docs/specs/workspaces/system-design/repository-secrets.md @@ -0,0 +1,62 @@ +--- +status: current +system: workspaces +requirements: + - REQ-WORKSPACES-REPOSITORY-SECRETS-001 +created: 2026-09-08 +owners: + - kandev +--- + +# Secret reference protection + +## Scope and requirement mapping + +This design covers AC-WORKSPACES-REPOSITORY-SECRETS-001.9 through .12. +The existing repository-secrets requirement retains the other runtime and storage contracts during specification migration. + +## Deletion boundary + +`secrets.Service` authorizes the secret, checks references, and delegates deletion to the existing store. +An injected reference checker keeps the secrets package independent of profile and task repositories. +`backendapp` wires the checker from the existing agent-settings and task repositories. +It reads active agent profiles, all executor profiles, and active repositories across workspaces. +Workspace-scoped profile and repository metadata is disclosed only after workspace access succeeds. +Inaccessible references still block deletion, with their metadata omitted. + +HTTP deletion returns `409` with `error`, `code: secret_in_use`, and `references`. +Each reference contains `kind`, `id`, `name`, and `key`. Hidden references contain only `kind`. +`GET /api/v1/secrets/:id/references` returns the same authorized reference projection without changing the secret. Workspace secrets require the existing `workspace_id` query parameter. +WebSocket deletion returns `CONFLICT` with equivalent details. +HTTP `?force=true` and the WebSocket boolean `force` bypass the reference check after authorization. +Missing or unauthorized secrets retain `404` behavior. Unexpected errors return sanitized `500` or `INTERNAL_ERROR` responses. + +The store remains available to internal credential cleanup and workspace cascades. +No schema or foreign-key migration is required. Existing broken references remain available for manual repair. +`UserVisibleStore.DeleteForWorkspace` preserves the authorized workspace scope through the final deletion boundary. +Its default `Delete` continues to accept Global secrets only. +The check protects references present during lookup. It does not serialize concurrent profile saves with deletion across repository owners. + +## Resolution and recovery + +`environment.SecretError` retains redacted errors and adds a repair instruction for the source environment. +Lifecycle error wrapping includes the selected agent profile name for agent-profile failures. +Origin classification and precedence remain unchanged. Secret IDs and underlying reveal errors stay out of rendered errors. + +Automatic name matching is excluded because a replacement name does not establish the original credential identity or authority. +Existing scope-transfer operations remain unchanged in this repair. + +## Settings feedback and mobile parity + +Opening a secret-delete confirmation starts the read-only reference request. While it is pending, the row-local desktop popover or mobile inline confirmation remains visible with its destructive action disabled. +An unreferenced secret keeps that local confirmation and requires a separate Delete action. +Existing references replace it with the same contained conflict-dialog pattern used by agent-profile deletion. The dialog presents each visible reference as a resource card with an icon, localized type badge, name, and environment key, and offers only Close. Only the resource list scrolls when it is long; the explanation and footer remain visible. +The dialog uses full-width touch actions below the small-screen breakpoint, remains within the dynamic viewport, and never exposes secret values or inaccessible resource metadata. +The final Delete request repeats the service reference check. A reference created after preflight reopens the conflict dialog from the structured `409`; unknown failures retain the generic localized toast. +Desktop and mobile Playwright coverage proves both the safe delete and preflight-conflict paths. + +## Evidence + +HTTP and WebSocket tests exercise the real service and encrypted SQLite store. +Reference-collector tests exercise profile and repository selection, redaction, and lookup failures. +Runtime tests cover actionable errors, profile identity, redaction, and all-or-nothing resolution.