From a95a0e1bd83c7457ccfd63816a6c7325f79bbbd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fiere?= Date: Thu, 13 Aug 2026 14:37:46 +0200 Subject: [PATCH] feat(secrets): copy and move secrets between global and workspace scopes (#2583) --- .../runtime/lifecycle/manager_auth_test.go | 18 + apps/backend/internal/backendapp/main.go | 55 +- .../internal/secrets/copy_move_test.go | 335 ++++++++++ apps/backend/internal/secrets/handlers.go | 172 ++++++ .../backend/internal/secrets/handlers_test.go | 580 ++++++++++++++++++ apps/backend/internal/secrets/models.go | 64 +- .../secrets/postgres_transfer_test.go | 390 ++++++++++++ apps/backend/internal/secrets/service.go | 139 ++++- apps/backend/internal/secrets/sqlite_store.go | 343 ++++++++++- apps/backend/internal/secrets/store.go | 57 ++ .../backend/internal/secrets/transfer_test.go | 450 ++++++++++++++ .../internal/secrets/user_visible_store.go | 54 +- apps/backend/pkg/websocket/actions.go | 2 + .../settings/copy-move-dialog-body.tsx | 339 ++++++++++ .../settings/copy-move-secret-dialog.test.tsx | 459 ++++++++++++++ .../settings/copy-move-secret-dialog.tsx | 424 +++++++++++++ apps/web/components/settings/secret-form.tsx | 83 +++ .../settings/secrets-delete-dialog.test.tsx | 2 +- .../settings/secrets-delete-dialog.tsx | 68 ++ .../settings/secrets-list-item-row.test.tsx | 65 ++ .../settings/secrets-list-item-row.tsx | 128 ++++ .../components/settings/secrets-settings.tsx | 517 +++++++--------- .../settings/mobile-secrets-copy-move.spec.ts | 167 +++++ .../tests/settings/secrets-copy-move.spec.ts | 185 ++++++ .../use-secret-destination-names.test.ts | 106 ++++ .../settings/use-secret-destination-names.ts | 90 +++ .../use-workspace-destinations.test.ts | 106 ++++ .../settings/use-workspace-destinations.ts | 59 ++ apps/web/lib/api/domains/secrets-api.test.ts | 57 +- apps/web/lib/api/domains/secrets-api.ts | 37 ++ apps/web/lib/types/http-secrets.ts | 16 + apps/web/src/locales/en/settings.json | 20 +- apps/web/src/locales/pseudo/settings.json | 20 +- apps/web/src/locales/pt-pt/settings.json | 21 +- apps/web/src/locales/zh-cn/settings.json | 26 +- docs/plans/secret-scope-transfer/plan.md | 407 ++++++++++++ .../task-01-backend-copy-move-service.md | 189 ++++++ .../task-02-backend-copy-move-handlers.md | 119 ++++ .../task-03-frontend-copy-move-dialog.md | 166 +++++ .../task-04-e2e-and-docs.md | 102 +++ docs/public/agents-and-profiles.md | 2 + docs/specs/INDEX.md | 1 + .../specs/workspaces/secret-scope-transfer.md | 336 ++++++++++ 43 files changed, 6653 insertions(+), 323 deletions(-) create mode 100644 apps/backend/internal/secrets/copy_move_test.go create mode 100644 apps/backend/internal/secrets/handlers_test.go create mode 100644 apps/backend/internal/secrets/postgres_transfer_test.go create mode 100644 apps/backend/internal/secrets/transfer_test.go create mode 100644 apps/web/components/settings/copy-move-dialog-body.tsx create mode 100644 apps/web/components/settings/copy-move-secret-dialog.test.tsx create mode 100644 apps/web/components/settings/copy-move-secret-dialog.tsx create mode 100644 apps/web/components/settings/secret-form.tsx create mode 100644 apps/web/components/settings/secrets-delete-dialog.tsx create mode 100644 apps/web/components/settings/secrets-list-item-row.test.tsx create mode 100644 apps/web/components/settings/secrets-list-item-row.tsx create mode 100644 apps/web/e2e/tests/settings/mobile-secrets-copy-move.spec.ts create mode 100644 apps/web/e2e/tests/settings/secrets-copy-move.spec.ts create mode 100644 apps/web/hooks/domains/settings/use-secret-destination-names.test.ts create mode 100644 apps/web/hooks/domains/settings/use-secret-destination-names.ts create mode 100644 apps/web/hooks/domains/settings/use-workspace-destinations.test.ts create mode 100644 apps/web/hooks/domains/settings/use-workspace-destinations.ts create mode 100644 docs/plans/secret-scope-transfer/plan.md create mode 100644 docs/plans/secret-scope-transfer/task-01-backend-copy-move-service.md create mode 100644 docs/plans/secret-scope-transfer/task-02-backend-copy-move-handlers.md create mode 100644 docs/plans/secret-scope-transfer/task-03-frontend-copy-move-dialog.md create mode 100644 docs/plans/secret-scope-transfer/task-04-e2e-and-docs.md create mode 100644 docs/specs/workspaces/secret-scope-transfer.md diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_auth_test.go b/apps/backend/internal/agent/runtime/lifecycle/manager_auth_test.go index c332ee7978..fda43056e5 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_auth_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_auth_test.go @@ -18,10 +18,12 @@ type inMemorySecretStore struct { var _ secrets.SecretStore = (*inMemorySecretStore)(nil) var _ secrets.ScopedSecretStore = (*inMemorySecretStore)(nil) +// newInMemorySecretStore returns an empty in-memory secret store for testing. func newInMemorySecretStore() *inMemorySecretStore { return &inMemorySecretStore{store: make(map[string]*secrets.SecretWithValue)} } +// Create stores the secret, returning the injected error when set. func (s *inMemorySecretStore) Create(_ context.Context, secret *secrets.SecretWithValue) error { if s.err != nil { return s.err @@ -33,6 +35,7 @@ func (s *inMemorySecretStore) Create(_ context.Context, secret *secrets.SecretWi return nil } +// Get returns the stored secret for the given ID, or an error when absent. func (s *inMemorySecretStore) Get(_ context.Context, id string) (*secrets.Secret, error) { if sw, ok := s.store[id]; ok { return &sw.Secret, nil @@ -40,6 +43,7 @@ func (s *inMemorySecretStore) Get(_ context.Context, id string) (*secrets.Secret return nil, fmt.Errorf("not found") } +// Reveal returns the plaintext value for the given ID, or an error when absent. func (s *inMemorySecretStore) Reveal(_ context.Context, id string) (string, error) { if sw, ok := s.store[id]; ok { return sw.Value, nil @@ -47,15 +51,23 @@ func (s *inMemorySecretStore) Reveal(_ context.Context, id string) (string, erro return "", fmt.Errorf("not found") } +// Update is a no-op for the in-memory store. func (s *inMemorySecretStore) Update(_ context.Context, _ string, _ *secrets.UpdateSecretRequest) error { return nil } + +// Delete is a no-op for the in-memory store. func (s *inMemorySecretStore) Delete(_ context.Context, _ string) error { return nil } + +// List returns no items for the in-memory store. func (s *inMemorySecretStore) List(_ context.Context) ([]*secrets.SecretListItem, error) { return nil, nil } + +// Close is a no-op for the in-memory store. func (s *inMemorySecretStore) Close() error { return nil } +// ListScoped returns stored secrets filtered by the requested scope and workspace. func (s *inMemorySecretStore) ListScoped(_ context.Context, opts secrets.SecretListOptions) ([]*secrets.SecretListItem, error) { items := make([]*secrets.SecretListItem, 0, len(s.store)) for _, stored := range s.store { @@ -77,6 +89,7 @@ func (s *inMemorySecretStore) ListScoped(_ context.Context, opts secrets.SecretL return items, nil } +// GetForWorkspace returns the secret when it is global or belongs to the given workspace. func (s *inMemorySecretStore) GetForWorkspace(_ context.Context, id, workspaceID string) (*secrets.Secret, error) { secret, err := s.Get(context.Background(), id) if err != nil { @@ -88,6 +101,7 @@ func (s *inMemorySecretStore) GetForWorkspace(_ context.Context, id, workspaceID return secret, nil } +// RevealGlobal reveals a global secret's value, rejecting workspace-scoped secrets. func (s *inMemorySecretStore) RevealGlobal(ctx context.Context, id string) (string, error) { secret, err := s.Get(ctx, id) if err != nil { @@ -99,6 +113,7 @@ func (s *inMemorySecretStore) RevealGlobal(ctx context.Context, id string) (stri return s.Reveal(ctx, id) } +// RevealForWorkspace reveals a secret's value after confirming workspace access. func (s *inMemorySecretStore) RevealForWorkspace(ctx context.Context, id, workspaceID string) (string, error) { if _, err := s.GetForWorkspace(ctx, id, workspaceID); err != nil { return "", err @@ -106,6 +121,7 @@ func (s *inMemorySecretStore) RevealForWorkspace(ctx context.Context, id, worksp return s.Reveal(ctx, id) } +// DeleteWorkspaceSecrets removes all secrets belonging to the given workspace. func (s *inMemorySecretStore) DeleteWorkspaceSecrets(_ context.Context, workspaceID string) error { for id, stored := range s.store { if stored.Scope == secrets.ScopeWorkspace && stored.WorkspaceID == workspaceID { @@ -115,6 +131,7 @@ func (s *inMemorySecretStore) DeleteWorkspaceSecrets(_ context.Context, workspac return nil } +// TestPersistAuthToken verifies persistAuthToken stores the instance auth token as a secret, records its ID in execution metadata, and no-ops when the token or store is absent. func TestPersistAuthToken(t *testing.T) { log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json"}) @@ -202,6 +219,7 @@ func TestPersistAuthToken(t *testing.T) { }) } +// TestPersistRuntimeSecrets verifies persistRuntimeSecrets stores the auth token and bootstrap nonce as secrets and records both IDs so they can be revealed later. func TestPersistRuntimeSecrets(t *testing.T) { log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json"}) store := newInMemorySecretStore() diff --git a/apps/backend/internal/backendapp/main.go b/apps/backend/internal/backendapp/main.go index f8b47636e4..732d9842ba 100644 --- a/apps/backend/internal/backendapp/main.go +++ b/apps/backend/internal/backendapp/main.go @@ -5,6 +5,7 @@ package backendapp import ( "context" + "errors" "flag" "fmt" "net" @@ -107,6 +108,7 @@ import ( workflowengine "github.com/kandev/kandev/internal/workflow/engine" taskhandlers "github.com/kandev/kandev/internal/task/handlers" + repoerrors "github.com/kandev/kandev/internal/task/repository/repoerrors" tasksqlite "github.com/kandev/kandev/internal/task/repository/sqlite" taskservice "github.com/kandev/kandev/internal/task/service" workflowservice "github.com/kandev/kandev/internal/workflow/service" @@ -150,6 +152,9 @@ type backendFlags struct { Version bool } +// parseBackendFlags parses the `kandev __backend` command-line flags into a +// backendFlags struct, returning the parsed flags, a usage printer, and any +// parse error. func parseBackendFlags(args []string) (backendFlags, func(), error) { flags := flag.NewFlagSet("kandev __backend", flag.ContinueOnError) flags.SetOutput(os.Stderr) @@ -269,6 +274,9 @@ func Run(args []string, build BuildInfo) int { return 0 } +// acquireRuntimeStateOwnership acquires the advisory ownership lock over the +// backend's runtime state (home dir and database), preventing a second backend +// instance from acting on the same state. func acquireRuntimeStateOwnership(cfg *config.Config) (*ownershiplock.Owner, error) { targets, err := ownershiplock.Targets(cfg.ResolvedHomeDir(), cfg.Database.Driver, cfg.Database.Path) if err != nil { @@ -277,6 +285,8 @@ func acquireRuntimeStateOwnership(cfg *config.Config) (*ownershiplock.Owner, err return ownershiplock.Acquire(targets) } +// setBuildInfo stamps the package-level build variables with the provided +// build metadata, skipping fields that were not populated. func setBuildInfo(build BuildInfo) { if build.Version != "" { Version = build.Version @@ -309,6 +319,9 @@ func run(cfg *config.Config, log *logger.Logger, cleanups *[]func() error, runCl return startServices(ctx, cfg, log, addCleanup, eventBus, runCleanups) } +// applyStartupRuntimeFlags resolves persisted runtime-flag overrides and +// applies them to the config, returning false if resolution fails. It is a +// no-op when no runtime-flag repository is configured. func applyStartupRuntimeFlags(ctx context.Context, cfg *config.Config, repos *Repositories, log *logger.Logger) bool { if repos.RuntimeFlags == nil { return true @@ -1004,6 +1017,8 @@ func startGatewayAndServe( return true } +// serverListenAddr formats the host and port into a listen address, binding +// all interfaces when host is empty. func serverListenAddr(host string, port int) string { host = strings.TrimSpace(host) if host == "" { @@ -1012,6 +1027,8 @@ func serverListenAddr(host string, port int) string { return net.JoinHostPort(host, fmt.Sprint(port)) } +// serverProbeAddr rewrites a listen address into a loopback address suitable +// for local probing, mapping wildcard hosts to 127.0.0.1 / ::1. func serverProbeAddr(listenAddr string) string { host, port, err := net.SplitHostPort(listenAddr) if err != nil { @@ -1163,6 +1180,8 @@ func initOfficeServices( return runProcessorSvc, true } +// newRunProcessorService constructs the office run-processor service, wiring +// its workspace, task, PR, and task-starter adapters to the backend services. func newRunProcessorService( cfg *config.Config, repos *Repositories, @@ -1284,12 +1303,14 @@ type officeRoutingRepoAdapter struct { repo *officesqlite.Repository } +// GetWorkspaceRouting returns the routing configuration for a workspace. func (a *officeRoutingRepoAdapter) GetWorkspaceRouting( ctx context.Context, workspaceID string, ) (*routing.WorkspaceConfig, error) { return a.repo.GetWorkspaceRouting(ctx, workspaceID) } +// ListProviderHealth returns the provider health statuses for a workspace. func (a *officeRoutingRepoAdapter) ListProviderHealth( ctx context.Context, workspaceID string, ) ([]officemodels.ProviderHealth, error) { @@ -1302,6 +1323,7 @@ type schedulerTaskStarterAdapter struct { orch *orchestrator.Service } +// StartTask starts a task on the orchestrator with the given launch parameters. func (a *schedulerTaskStarterAdapter) StartTask( ctx context.Context, taskID, agentProfileID, executorID, executorProfileID string, @@ -1314,6 +1336,8 @@ func (a *schedulerTaskStarterAdapter) StartTask( return err } +// StartTaskWithRoute starts a task on the orchestrator with an explicit launch +// context and route override. func (a *schedulerTaskStarterAdapter) StartTaskWithRoute( ctx context.Context, taskID, agentProfileID string, @@ -1472,6 +1496,8 @@ type runsServiceEngineAdapter struct { svc *runsservice.Service } +// QueueRun enqueues a run, translating the engine's QueueRunRequest into the +// runs-service request shape. func (a *runsServiceEngineAdapter) QueueRun(ctx context.Context, req workflowengine.QueueRunRequest) error { return a.svc.QueueRun(ctx, runsservice.QueueRunRequest{ AgentProfileID: req.AgentProfileID, @@ -1578,10 +1604,14 @@ type officeFailureInboxAdapter struct { svc *officeservice.Service } +// newOfficeFailureInboxAdapter wraps an office service in the dashboard +// failure-inbox adapter so its rows can be served through the dashboard API. func newOfficeFailureInboxAdapter(svc *officeservice.Service) *officeFailureInboxAdapter { return &officeFailureInboxAdapter{svc: svc} } +// ListFailedRunInboxRows returns the failed-run inbox rows for a workspace and +// user, mapped into the dashboard failure-inbox row type. func (a *officeFailureInboxAdapter) ListFailedRunInboxRows( ctx context.Context, workspaceID, userID string, ) ([]officedashboard.FailureInboxRow, error) { @@ -1604,6 +1634,8 @@ func (a *officeFailureInboxAdapter) ListFailedRunInboxRows( return out, nil } +// ListPausedAgentInboxRows returns the paused-agent inbox rows for a workspace +// and user, mapped into the dashboard failure-inbox row type. func (a *officeFailureInboxAdapter) ListPausedAgentInboxRows( ctx context.Context, workspaceID, userID string, ) ([]officedashboard.FailureInboxRow, error) { @@ -1855,6 +1887,8 @@ func buildOfficeDashboardService( // buildHTTPServer creates the HTTP server with all routes registered. var newInterimSettingsInterlockToken = httpmw.NewInterimSettingsInterlockToken +// resolvedHTTPPort returns the configured HTTP server port, falling back to +// the default backend port when unset. func resolvedHTTPPort(cfg *config.Config) int { if cfg.Server.Port != 0 { return cfg.Server.Port @@ -1862,6 +1896,8 @@ func resolvedHTTPPort(cfg *config.Config) int { return ports.Backend } +// buildHTTPServer creates the HTTP server with all middleware and routes +// registered against the gateway and service layer. func buildHTTPServer( cfg *config.Config, log *logger.Logger, @@ -1927,7 +1963,24 @@ func buildHTTPServer( router.Use(integrationWorkspaceScopeMiddleware(services.Auth, services.Task)) secretsSvc := secrets.NewService(userSecretStore, log) - secretsSvc.SetWorkspaceAuthorizer(services.Task.AuthorizeWorkspaceAccess) + // 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 + // unclassified (sanitized 500). + secretsSvc.SetWorkspaceAuthorizer(func(ctx context.Context, workspaceID string) error { + err := services.Task.AuthorizeWorkspaceAccess(ctx, workspaceID) + if errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + return secrets.ErrWorkspaceAccessDenied + } + return err + }) + secretsSvc.SetWorkspaceExistenceChecker(func(ctx context.Context, workspaceID string) error { + _, err := services.Task.GetWorkspace(ctx, workspaceID) + if errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + return secrets.ErrWorkspaceAccessDenied + } + return err + }) registerRoutes(routeParams{ router: router, gateway: gateway, diff --git a/apps/backend/internal/secrets/copy_move_test.go b/apps/backend/internal/secrets/copy_move_test.go new file mode 100644 index 0000000000..617a55a64b --- /dev/null +++ b/apps/backend/internal/secrets/copy_move_test.go @@ -0,0 +1,335 @@ +package secrets + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "testing" + + "github.com/kandev/kandev/internal/auth/authn" +) + +// testCopyMoveService builds a service over the user-visible SQLite store with +// an authorizer and existence checker that allow the listed workspaces. +func testCopyMoveService(t *testing.T, allowed map[string]bool) *Service { + t.Helper() + store := newTestSQLiteStore(t) + svc := NewService(NewUserVisibleStore(store), nil) + svc.SetWorkspaceAuthorizer(func(_ context.Context, workspaceID string) error { + if allowed[workspaceID] { + return nil + } + return ErrWorkspaceAccessDenied + }) + svc.SetWorkspaceExistenceChecker(func(_ context.Context, workspaceID string) error { + if allowed[workspaceID] { + return nil + } + return ErrWorkspaceAccessDenied + }) + return svc +} + +// decodeCopyRequest unmarshals a JSON copy/move request body for testing. +func decodeCopyRequest(t *testing.T, body string) *CopySecretRequest { + t.Helper() + var req CopySecretRequest + if err := json.Unmarshal([]byte(body), &req); err != nil { + t.Fatalf("decode %s: %v", body, err) + } + return &req +} + +// TestServiceCopy_GlobalToWorkspaceUsesSourceNameWhenOmitted verifies a global-to-workspace copy with no explicit name keeps the source name and value and retains the source. +func TestServiceCopy_GlobalToWorkspaceUsesSourceNameWhenOmitted(t *testing.T) { + svc := testCopyMoveService(t, map[string]bool{"workspace-a": true}) + ctx := context.Background() + if _, err := svc.Create(ctx, &CreateSecretRequest{Name: "API Key", Value: "v1"}); err != nil { + t.Fatalf("create: %v", err) + } + + item, err := svc.Copy(ctx, globalID(t, svc, "API Key"), "", decodeCopyRequest(t, `{"target_scope":"workspace","target_workspace_id":"workspace-a"}`)) + if err != nil { + t.Fatalf("Copy: %v", err) + } + if item.Scope != ScopeWorkspace || item.WorkspaceID != "workspace-a" || item.Name != "API Key" { + t.Fatalf("item = %+v", item) + } + if item.CreatedAt.IsZero() || item.UpdatedAt.IsZero() { + t.Fatalf("item timestamps zero: %+v", item) + } + value, err := svc.RevealForWorkspace(ctx, item.ID, "workspace-a") + if err != nil { + t.Fatalf("reveal: %v", err) + } + if value != "v1" { + t.Fatalf("value = %q, want v1", value) + } + // Source retained. + global, err := svc.List(ctx) + if err != nil || len(global) != 1 { + t.Fatalf("global list after copy = %+v, %v", global, err) + } +} + +// TestServiceCopy_WorkspaceToGlobalWithExplicitName verifies a workspace-to-global copy with an explicit name keeps the source intact. +func TestServiceCopy_WorkspaceToGlobalWithExplicitName(t *testing.T) { + svc := testCopyMoveService(t, map[string]bool{"workspace-a": true}) + ctx := context.Background() + w := mustCreateViaService(t, svc, "ws", "v2", ScopeWorkspace, "workspace-a") + + item, err := svc.Copy(ctx, w.ID, "workspace-a", decodeCopyRequest(t, `{"target_scope":"global","name":"renamed"}`)) + if err != nil { + t.Fatalf("Copy: %v", err) + } + if item.Scope != ScopeGlobal || item.Name != "renamed" { + t.Fatalf("item = %+v", item) + } + if _, err := svc.GetWorkspaceSecret(ctx, w.ID, "workspace-a"); err != nil { + t.Fatalf("source lost after copy: %v", err) + } +} + +// TestServiceCopy_WorkspaceToWorkspace verifies a workspace-to-workspace copy lands in the target workspace. +func TestServiceCopy_WorkspaceToWorkspace(t *testing.T) { + svc := testCopyMoveService(t, map[string]bool{"workspace-a": true, "workspace-b": true}) + ctx := context.Background() + w := mustCreateViaService(t, svc, "ws", "v3", ScopeWorkspace, "workspace-a") + + item, err := svc.Copy(ctx, w.ID, "workspace-a", decodeCopyRequest(t, `{"target_scope":"workspace","target_workspace_id":"workspace-b","name":"to-b"}`)) + if err != nil { + t.Fatalf("Copy: %v", err) + } + if item.WorkspaceID != "workspace-b" { + t.Fatalf("item workspace = %q", item.WorkspaceID) + } +} + +// TestServiceMove_WorkspaceToGlobalRemovesSource verifies moving a workspace secret to global removes the source. +func TestServiceMove_WorkspaceToGlobalRemovesSource(t *testing.T) { + svc := testCopyMoveService(t, map[string]bool{"workspace-a": true}) + ctx := context.Background() + w := mustCreateViaService(t, svc, "ws", "v4", ScopeWorkspace, "workspace-a") + + item, err := svc.Move(ctx, w.ID, "workspace-a", decodeCopyRequest(t, `{"target_scope":"global","name":"moved"}`)) + if err != nil { + t.Fatalf("Move: %v", err) + } + if item.Scope != ScopeGlobal { + t.Fatalf("item scope = %q", item.Scope) + } + if _, err := svc.GetWorkspaceSecret(ctx, w.ID, "workspace-a"); !errors.Is(err, ErrNotFound) { + t.Fatalf("source still present after move: %v", err) + } +} + +// TestServiceMove_GlobalToWorkspaceRemovesSource verifies moving a global secret into a workspace removes the source. +func TestServiceMove_GlobalToWorkspaceRemovesSource(t *testing.T) { + svc := testCopyMoveService(t, map[string]bool{"workspace-a": true}) + ctx := context.Background() + g := mustCreateViaService(t, svc, "g", "v5", ScopeGlobal, "") + + item, err := svc.Move(ctx, g.ID, "", decodeCopyRequest(t, `{"target_scope":"workspace","target_workspace_id":"workspace-a"}`)) + if err != nil { + t.Fatalf("Move: %v", err) + } + if item.WorkspaceID != "workspace-a" { + t.Fatalf("item workspace = %q", item.WorkspaceID) + } + if _, err := svc.Get(ctx, g.ID); !errors.Is(err, ErrNotFound) { + t.Fatalf("source still present after move: %v", err) + } +} + +// TestServiceCopy_NamePresenceSemantics verifies null, empty, whitespace, and over-long target names are rejected as ErrSecretValidation. +func TestServiceCopy_NamePresenceSemantics(t *testing.T) { + svc := testCopyMoveService(t, map[string]bool{"workspace-a": true}) + ctx := context.Background() + g := mustCreateViaService(t, svc, "API Key", "v", ScopeGlobal, "") + + t.Run("null is a validation error", func(t *testing.T) { + _, err := svc.Copy(ctx, g.ID, "", decodeCopyRequest(t, `{"target_scope":"workspace","target_workspace_id":"workspace-a","name":null}`)) + if !errors.Is(err, ErrSecretValidation) { + t.Fatalf("err = %v, want ErrSecretValidation", err) + } + }) + t.Run("empty string is a validation error", func(t *testing.T) { + _, err := svc.Copy(ctx, g.ID, "", decodeCopyRequest(t, `{"target_scope":"workspace","target_workspace_id":"workspace-a","name":""}`)) + if !errors.Is(err, ErrSecretValidation) { + t.Fatalf("err = %v, want ErrSecretValidation", err) + } + }) + t.Run("whitespace is a validation error", func(t *testing.T) { + _, err := svc.Copy(ctx, g.ID, "", decodeCopyRequest(t, `{"target_scope":"workspace","target_workspace_id":"workspace-a","name":" "}`)) + if !errors.Is(err, ErrSecretValidation) { + t.Fatalf("err = %v, want ErrSecretValidation", err) + } + }) + t.Run("over 100 UTF-8 bytes is a validation error", func(t *testing.T) { + long := "" + for len(long) <= 100 { + long += "é" // 2 bytes per code point + } + req := decodeCopyRequest(t, fmt.Sprintf(`{"target_scope":"workspace","target_workspace_id":"workspace-a","name":%q}`, long)) + _, err := svc.Copy(ctx, g.ID, "", req) + if !errors.Is(err, ErrSecretValidation) { + t.Fatalf("err = %v, want ErrSecretValidation", err) + } + }) +} + +// TestServiceCopy_SameScopeRejected verifies copying within the same scope returns ErrSecretValidation. +func TestServiceCopy_SameScopeRejected(t *testing.T) { + svc := testCopyMoveService(t, map[string]bool{"workspace-a": true}) + ctx := context.Background() + g := mustCreateViaService(t, svc, "g", "v", ScopeGlobal, "") + w := mustCreateViaService(t, svc, "w", "v", ScopeWorkspace, "workspace-a") + + if _, err := svc.Copy(ctx, g.ID, "", decodeCopyRequest(t, `{"target_scope":"global"}`)); !errors.Is(err, ErrSecretValidation) { + t.Fatalf("global->global err = %v, want ErrSecretValidation", err) + } + if _, err := svc.Copy(ctx, w.ID, "workspace-a", decodeCopyRequest(t, `{"target_scope":"workspace","target_workspace_id":"workspace-a"}`)); !errors.Is(err, ErrSecretValidation) { + t.Fatalf("workspace A->A err = %v, want ErrSecretValidation", err) + } +} + +// TestServiceCopy_NameConflict verifies a target-name collision in the destination scope returns ErrSecretNameConflict. +func TestServiceCopy_NameConflict(t *testing.T) { + svc := testCopyMoveService(t, map[string]bool{"workspace-a": true}) + ctx := context.Background() + // "taken" lives in the workspace target scope. + mustCreateViaService(t, svc, "taken", "other", ScopeWorkspace, "workspace-a") + g := mustCreateViaService(t, svc, "g", "v", ScopeGlobal, "") + + _, err := svc.Copy(ctx, g.ID, "", decodeCopyRequest(t, `{"target_scope":"workspace","target_workspace_id":"workspace-a","name":"taken"}`)) + if !errors.Is(err, ErrSecretNameConflict) { + t.Fatalf("err = %v, want ErrSecretNameConflict", err) + } +} + +// TestServiceCopy_MissingSourceIsNotFound verifies an unknown source ID returns ErrNotFound. +func TestServiceCopy_MissingSourceIsNotFound(t *testing.T) { + svc := testCopyMoveService(t, nil) + _, err := svc.Copy(context.Background(), "missing", "", decodeCopyRequest(t, `{"target_scope":"global"}`)) + if !errors.Is(err, ErrNotFound) { + t.Fatalf("err = %v, want ErrNotFound", err) + } +} + +// TestServiceCopy_UnauthorizedSourceIsDenied verifies revoking source-workspace access makes Copy fail with ErrWorkspaceAccessDenied. +func TestServiceCopy_UnauthorizedSourceIsDenied(t *testing.T) { + svc := testCopyMoveService(t, map[string]bool{"workspace-a": true}) + ctx := context.Background() + w := mustCreateViaService(t, svc, "w", "v", ScopeWorkspace, "workspace-a") + + // Revoke access to the source workspace after seeding the secret. + svc.SetWorkspaceAuthorizer(func(context.Context, string) error { return ErrWorkspaceAccessDenied }) + svc.SetWorkspaceExistenceChecker(func(context.Context, string) error { return ErrWorkspaceAccessDenied }) + + _, err := svc.Copy(ctx, w.ID, "workspace-a", decodeCopyRequest(t, `{"target_scope":"global"}`)) + if !errors.Is(err, ErrWorkspaceAccessDenied) { + t.Fatalf("err = %v, want ErrWorkspaceAccessDenied", err) + } +} + +// TestServiceCopy_UnauthorizedDestinationIsDenied verifies an unallowed destination workspace returns ErrWorkspaceAccessDenied. +func TestServiceCopy_UnauthorizedDestinationIsDenied(t *testing.T) { + svc := testCopyMoveService(t, map[string]bool{"workspace-a": true}) // workspace-b not allowed + ctx := context.Background() + g := mustCreateViaService(t, svc, "g", "v", ScopeGlobal, "") + + _, err := svc.Copy(ctx, g.ID, "", decodeCopyRequest(t, `{"target_scope":"workspace","target_workspace_id":"workspace-b"}`)) + if !errors.Is(err, ErrWorkspaceAccessDenied) { + t.Fatalf("err = %v, want ErrWorkspaceAccessDenied", err) + } +} + +// TestServiceCopy_NilExistenceCheckerFailsClosed verifies a nil existence checker denies workspace targets (fails closed). +func TestServiceCopy_NilExistenceCheckerFailsClosed(t *testing.T) { + store := newTestSQLiteStore(t) + svc := NewService(NewUserVisibleStore(store), nil) + svc.SetWorkspaceAuthorizer(func(context.Context, string) error { return nil }) + ctx := context.Background() + g := mustCreateViaService(t, svc, "g", "v", ScopeGlobal, "") + + _, err := svc.Copy(ctx, g.ID, "", decodeCopyRequest(t, `{"target_scope":"workspace","target_workspace_id":"any"}`)) + if !errors.Is(err, ErrWorkspaceAccessDenied) { + t.Fatalf("err = %v, want ErrWorkspaceAccessDenied (nil checker fails closed)", err) + } +} + +// TestServiceCopy_RawAuthorizerErrorPassesThroughUnclassified verifies a raw authorizer error is returned as-is rather than mapped to a sentinel. +func TestServiceCopy_RawAuthorizerErrorPassesThroughUnclassified(t *testing.T) { + store := newTestSQLiteStore(t) + svc := NewService(NewUserVisibleStore(store), nil) + rawErr := errors.New("database unreachable") + svc.SetWorkspaceAuthorizer(func(context.Context, string) error { return rawErr }) + svc.SetWorkspaceExistenceChecker(func(context.Context, string) error { return nil }) + ctx := context.Background() + g := mustCreateViaService(t, svc, "g", "v", ScopeGlobal, "") + + _, err := svc.Copy(ctx, g.ID, "", decodeCopyRequest(t, `{"target_scope":"workspace","target_workspace_id":"workspace-a"}`)) + if !errors.Is(err, rawErr) { + t.Fatalf("err = %v, want the raw authorizer error (unclassified)", err) + } + if errors.Is(err, ErrNotFound) || errors.Is(err, ErrSecretValidation) || errors.Is(err, ErrWorkspaceAccessDenied) { + t.Fatalf("raw error misclassified as a sentinel: %v", err) + } +} + +// TestServiceCopy_InternalSourceIDIsNotFound verifies internal (backend-owned) source IDs are rejected as ErrNotFound. +func TestServiceCopy_InternalSourceIDIsNotFound(t *testing.T) { + svc := testCopyMoveService(t, map[string]bool{"workspace-a": true}) + _, err := svc.Copy(context.Background(), "github:user:workspace:user:access", "", decodeCopyRequest(t, `{"target_scope":"workspace","target_workspace_id":"workspace-a"}`)) + if !errors.Is(err, ErrNotFound) { + t.Fatalf("err = %v, want ErrNotFound", err) + } +} + +// TestServiceMove_CrossUserOwnershipEnforced verifies a user cannot move another user's workspace secret (ErrNotFound) while the owner can. +func TestServiceMove_CrossUserOwnershipEnforced(t *testing.T) { + svc := testCopyMoveService(t, map[string]bool{"workspace-a": true}) + alice := authn.WithIdentity(context.Background(), authn.Identity{UserID: "user-a"}) + bob := authn.WithIdentity(context.Background(), authn.Identity{UserID: "user-b"}) + + // Alice creates a workspace secret. + created, err := svc.Create(alice, &CreateSecretRequest{Name: "alice-ws", Value: "v", Scope: ScopeWorkspace, WorkspaceID: "workspace-a"}) + if err != nil { + t.Fatalf("create as alice: %v", err) + } + // Bob cannot move it (store-level per-user scoping). + if _, err := svc.Move(bob, created.ID, "workspace-a", decodeCopyRequest(t, `{"target_scope":"global"}`)); !errors.Is(err, ErrNotFound) { + t.Fatalf("bob move err = %v, want ErrNotFound", err) + } + // Alice can. + if _, err := svc.Move(alice, created.ID, "workspace-a", decodeCopyRequest(t, `{"target_scope":"global","name":"alice-global"}`)); err != nil { + t.Fatalf("alice move: %v", err) + } +} + +// mustCreateViaService is a small helper that returns the created item. +func mustCreateViaService(t *testing.T, svc *Service, name, value string, scope SecretScope, workspaceID string) *SecretListItem { + t.Helper() + item, err := svc.Create(context.Background(), &CreateSecretRequest{Name: name, Value: value, Scope: scope, WorkspaceID: workspaceID}) + if err != nil { + t.Fatalf("create %s: %v", name, err) + } + return item +} + +// globalID resolves a global secret's ID by name via List. +func globalID(t *testing.T, svc *Service, name string) string { + t.Helper() + items, err := svc.List(context.Background()) + if err != nil { + t.Fatalf("list: %v", err) + } + for _, item := range items { + if item.Name == name { + return item.ID + } + } + t.Fatalf("no global secret named %s", name) + return "" +} diff --git a/apps/backend/internal/secrets/handlers.go b/apps/backend/internal/secrets/handlers.go index d2362f76ec..00d93983a7 100644 --- a/apps/backend/internal/secrets/handlers.go +++ b/apps/backend/internal/secrets/handlers.go @@ -2,6 +2,8 @@ package secrets import ( "context" + "encoding/json" + "errors" "net/http" "github.com/gin-gonic/gin" @@ -29,6 +31,7 @@ func RegisterRoutes(router *gin.Engine, dispatcher *ws.Dispatcher, svc *Service, h.registerWS(dispatcher) } +// registerHTTP registers the secrets HTTP routes under /api/v1. func (h *Handler) registerHTTP(router *gin.Engine) { api := router.Group("/api/v1") api.POST("/secrets", h.httpCreateSecret) @@ -37,18 +40,24 @@ func (h *Handler) registerHTTP(router *gin.Engine) { api.PUT("/secrets/:id", h.httpUpdateSecret) api.DELETE("/secrets/:id", h.httpDeleteSecret) api.POST("/secrets/:id/reveal", h.httpRevealSecret) + api.POST("/secrets/:id/copy", h.httpCopySecret) + api.POST("/secrets/:id/move", h.httpMoveSecret) } +// registerWS registers the secrets WebSocket actions on the dispatcher. func (h *Handler) registerWS(dispatcher *ws.Dispatcher) { dispatcher.RegisterFunc(ws.ActionSecretList, h.wsList) dispatcher.RegisterFunc(ws.ActionSecretCreate, h.wsCreate) dispatcher.RegisterFunc(ws.ActionSecretUpdate, h.wsUpdate) dispatcher.RegisterFunc(ws.ActionSecretDelete, h.wsDelete) dispatcher.RegisterFunc(ws.ActionSecretReveal, h.wsReveal) + dispatcher.RegisterFunc(ws.ActionSecretCopy, h.wsCopy) + dispatcher.RegisterFunc(ws.ActionSecretMove, h.wsMove) } // HTTP handlers +// httpCreateSecret handles POST /api/v1/secrets. func (h *Handler) httpCreateSecret(c *gin.Context) { var req CreateSecretRequest if err := c.ShouldBindJSON(&req); err != nil { @@ -65,6 +74,7 @@ func (h *Handler) httpCreateSecret(c *gin.Context) { c.JSON(http.StatusCreated, item) } +// httpListSecrets handles GET /api/v1/secrets. func (h *Handler) httpListSecrets(c *gin.Context) { opts := SecretListOptions{ Scope: SecretScope(c.Query("scope")), @@ -83,6 +93,7 @@ func (h *Handler) httpListSecrets(c *gin.Context) { c.JSON(http.StatusOK, items) } +// httpGetSecret handles GET /api/v1/secrets/:id. func (h *Handler) httpGetSecret(c *gin.Context) { id := c.Param("id") secret, err := h.getSecret(c, id) @@ -93,6 +104,7 @@ func (h *Handler) httpGetSecret(c *gin.Context) { c.JSON(http.StatusOK, secret) } +// httpUpdateSecret handles PUT /api/v1/secrets/:id. func (h *Handler) httpUpdateSecret(c *gin.Context) { id := c.Param("id") var req UpdateSecretRequest @@ -110,6 +122,7 @@ func (h *Handler) httpUpdateSecret(c *gin.Context) { c.JSON(http.StatusOK, item) } +// httpDeleteSecret handles DELETE /api/v1/secrets/:id. func (h *Handler) httpDeleteSecret(c *gin.Context) { id := c.Param("id") if err := h.deleteSecret(c, id); err != nil { @@ -119,6 +132,7 @@ func (h *Handler) httpDeleteSecret(c *gin.Context) { c.Status(http.StatusNoContent) } +// httpRevealSecret handles POST /api/v1/secrets/:id/reveal. func (h *Handler) httpRevealSecret(c *gin.Context) { id := c.Param("id") value, err := h.revealSecret(c, id) @@ -131,6 +145,7 @@ func (h *Handler) httpRevealSecret(c *gin.Context) { // WS handlers +// wsList handles the secrets.list WebSocket action. func (h *Handler) wsList(ctx context.Context, msg *ws.Message) (*ws.Message, error) { var req struct { Scope SecretScope `json:"scope"` @@ -152,6 +167,7 @@ func (h *Handler) wsList(ctx context.Context, msg *ws.Message) (*ws.Message, err return ws.NewResponse(msg.ID, msg.Action, items) } +// wsCreate handles the secrets.create WebSocket action. func (h *Handler) wsCreate(ctx context.Context, msg *ws.Message) (*ws.Message, error) { var req CreateSecretRequest if err := msg.ParsePayload(&req); err != nil { @@ -165,6 +181,7 @@ func (h *Handler) wsCreate(ctx context.Context, msg *ws.Message) (*ws.Message, e return ws.NewResponse(msg.ID, msg.Action, item) } +// wsUpdate handles the secrets.update WebSocket action. func (h *Handler) wsUpdate(ctx context.Context, msg *ws.Message) (*ws.Message, error) { var payload struct { ID string `json:"id"` @@ -182,6 +199,7 @@ func (h *Handler) wsUpdate(ctx context.Context, msg *ws.Message) (*ws.Message, e return ws.NewResponse(msg.ID, msg.Action, item) } +// wsDelete handles the secrets.delete WebSocket action. func (h *Handler) wsDelete(ctx context.Context, msg *ws.Message) (*ws.Message, error) { var payload struct { ID string `json:"id"` @@ -197,6 +215,7 @@ func (h *Handler) wsDelete(ctx context.Context, msg *ws.Message) (*ws.Message, e return ws.NewResponse(msg.ID, msg.Action, map[string]bool{"success": true}) } +// wsReveal handles the secrets.reveal WebSocket action. func (h *Handler) wsReveal(ctx context.Context, msg *ws.Message) (*ws.Message, error) { var payload struct { ID string `json:"id"` @@ -213,6 +232,8 @@ func (h *Handler) wsReveal(ctx context.Context, msg *ws.Message) (*ws.Message, e return ws.NewResponse(msg.ID, msg.Action, RevealSecretResponse{Value: value}) } +// getSecret fetches a secret by ID, resolving workspace-scoped access from +// the request's workspace_id query parameter. func (h *Handler) getSecret(c *gin.Context, id string) (*Secret, error) { if workspaceID := c.Query("workspace_id"); workspaceID != "" { return h.service.GetWorkspaceSecret(c.Request.Context(), id, workspaceID) @@ -220,6 +241,8 @@ func (h *Handler) getSecret(c *gin.Context, id string) (*Secret, error) { return h.service.Get(c.Request.Context(), id) } +// updateSecret updates a secret, resolving workspace-scoped access from the +// request's workspace_id query parameter. func (h *Handler) updateSecret(c *gin.Context, id string, req *UpdateSecretRequest) (*SecretListItem, error) { if workspaceID := c.Query("workspace_id"); workspaceID != "" { return h.service.UpdateWorkspaceSecret(c.Request.Context(), id, workspaceID, req) @@ -227,6 +250,8 @@ func (h *Handler) updateSecret(c *gin.Context, id string, req *UpdateSecretReque return h.service.Update(c.Request.Context(), id, req) } +// updateSecretForWorkspace updates a secret, targeting the workspace when +// workspaceID is non-empty and falling back to the global scope otherwise. func (h *Handler) updateSecretForWorkspace(ctx context.Context, id, workspaceID string, req *UpdateSecretRequest) (*SecretListItem, error) { if workspaceID != "" { return h.service.UpdateWorkspaceSecret(ctx, id, workspaceID, req) @@ -234,6 +259,8 @@ func (h *Handler) updateSecretForWorkspace(ctx context.Context, id, workspaceID return h.service.Update(ctx, id, req) } +// 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) @@ -241,6 +268,8 @@ func (h *Handler) deleteSecret(c *gin.Context, id string) error { return h.service.Delete(c.Request.Context(), id) } +// 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 { if workspaceID != "" { return h.service.DeleteWorkspaceSecret(ctx, id, workspaceID) @@ -248,6 +277,8 @@ func (h *Handler) deleteSecretForWorkspace(ctx context.Context, id, workspaceID return h.service.Delete(ctx, id) } +// revealSecret returns a secret's value, resolving workspace-scoped access +// from the request's workspace_id query parameter. func (h *Handler) revealSecret(c *gin.Context, id string) (string, error) { if workspaceID := c.Query("workspace_id"); workspaceID != "" { return h.service.RevealWorkspaceSecret(c.Request.Context(), id, workspaceID) @@ -255,9 +286,150 @@ func (h *Handler) revealSecret(c *gin.Context, id string) (string, error) { return h.service.Reveal(c.Request.Context(), id) } +// revealSecretForWorkspace returns a secret's value, targeting the workspace +// when workspaceID is non-empty and falling back to the global scope otherwise. func (h *Handler) revealSecretForWorkspace(ctx context.Context, id, workspaceID string) (string, error) { if workspaceID != "" { return h.service.RevealWorkspaceSecret(ctx, id, workspaceID) } return h.service.Reveal(ctx, id) } + +// HTTP copy/move handlers + +// httpCopySecret handles POST /api/v1/secrets/:id/copy. +func (h *Handler) httpCopySecret(c *gin.Context) { + h.httpTransferSecret(c, false) +} + +// httpMoveSecret handles POST /api/v1/secrets/:id/move. +func (h *Handler) httpMoveSecret(c *gin.Context) { + h.httpTransferSecret(c, true) +} + +// httpTransferSecret implements the shared HTTP copy/move flow for a secret; +// move selects move semantics, false selects copy. +func (h *Handler) httpTransferSecret(c *gin.Context, move bool) { + id := c.Param("id") + var req CopySecretRequest + if err := c.ShouldBindJSON(&req); err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": "invalid payload"}) + return + } + + item, err := h.transferSecret(c.Request.Context(), id, c.Query("workspace_id"), &req, move) + if err != nil { + status, message := h.transferError(err) + c.JSON(status, gin.H{"error": message}) + return + } + c.JSON(http.StatusCreated, item) +} + +// transferSecret dispatches copy/move using the existing source-scoping +// convention: a non-empty workspace_id selects the workspace-scoped source. +func (h *Handler) transferSecret(ctx context.Context, id, sourceWorkspaceID string, req *CopySecretRequest, move bool) (*SecretListItem, error) { + if move { + return h.service.Move(ctx, id, sourceWorkspaceID, req) + } + return h.service.Copy(ctx, id, sourceWorkspaceID, req) +} + +// transferError classifies a transfer error into an HTTP status and a safe +// message. Only the known sentinels map to 400/404/409; unexpected failures +// become a sanitized 500 with the details logged server-side. +func (h *Handler) transferError(err error) (int, string) { + switch { + case errors.Is(err, ErrSecretValidation): + return http.StatusBadRequest, err.Error() + case errors.Is(err, ErrNotFound), errors.Is(err, ErrWorkspaceAccessDenied): + return http.StatusNotFound, err.Error() + case errors.Is(err, ErrSecretNameConflict): + return http.StatusConflict, err.Error() + default: + h.logger.Error("secret transfer failed", zap.Error(err)) + return http.StatusInternalServerError, "internal error" + } +} + +// WS copy/move handlers + +// wsCopy handles the secrets.copy WebSocket action. +func (h *Handler) wsCopy(ctx context.Context, msg *ws.Message) (*ws.Message, error) { + return h.wsTransfer(ctx, msg, false) +} + +// wsMove handles the secrets.move WebSocket action. +func (h *Handler) wsMove(ctx context.Context, msg *ws.Message) (*ws.Message, error) { + return h.wsTransfer(ctx, msg, true) +} + +// wsTransferPayload is the WS envelope for secrets.copy / secrets.move. It +// decodes id/workspace_id separately, then delegates the transfer fields to +// the presence-aware CopySecretRequest decoder (a flat struct would silently +// collapse an explicit null name into "omitted"). +type wsTransferPayload struct { + ID string + WorkspaceID string + CopySecretRequest +} + +// UnmarshalJSON implements the presence-aware envelope decode. The +// CopySecretRequest decoder runs on the whole payload and resets every field, +// including its presence markers, so a reused envelope cannot leak state +// between messages. +func (p *wsTransferPayload) UnmarshalJSON(data []byte) error { + // Reset every field before parsing so a failed decode can never leave + // stale state from an earlier message on a reused envelope, and assign + // only after every sub-decode succeeds so a semantic failure cannot leave + // a partially populated receiver. + p.ID = "" + p.WorkspaceID = "" + p.CopySecretRequest = CopySecretRequest{} + var aux struct { + ID string `json:"id"` + WorkspaceID string `json:"workspace_id"` + } + if err := json.Unmarshal(data, &aux); err != nil { + return err + } + var req CopySecretRequest + if err := json.Unmarshal(data, &req); err != nil { + return err + } + p.ID = aux.ID + p.WorkspaceID = aux.WorkspaceID + p.CopySecretRequest = req + return nil +} + +// wsTransfer implements the shared WebSocket copy/move flow for a secret; +// move selects move semantics, false selects copy. +func (h *Handler) wsTransfer(ctx context.Context, msg *ws.Message, move bool) (*ws.Message, error) { + var payload wsTransferPayload + if err := msg.ParsePayload(&payload); err != nil { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, "invalid payload: "+err.Error(), nil) + } + + item, err := h.transferSecret(ctx, payload.ID, payload.WorkspaceID, &payload.CopySecretRequest, move) + if err != nil { + code, message := h.transferWSError(err) + return ws.NewError(msg.ID, msg.Action, code, message, nil) + } + return ws.NewResponse(msg.ID, msg.Action, item) +} + +// transferWSError maps a transfer error to its WebSocket payload and status. +func (h *Handler) transferWSError(err error) (string, string) { + switch { + case errors.Is(err, ErrSecretValidation): + return ws.ErrorCodeBadRequest, err.Error() + case errors.Is(err, ErrNotFound), errors.Is(err, ErrWorkspaceAccessDenied): + return ws.ErrorCodeNotFound, err.Error() + case errors.Is(err, ErrSecretNameConflict): + return ws.ErrorCodeConflict, err.Error() + default: + h.logger.Error("secret transfer failed", zap.Error(err)) + return ws.ErrorCodeInternalError, "internal error" + } +} diff --git a/apps/backend/internal/secrets/handlers_test.go b/apps/backend/internal/secrets/handlers_test.go new file mode 100644 index 0000000000..098b5acbff --- /dev/null +++ b/apps/backend/internal/secrets/handlers_test.go @@ -0,0 +1,580 @@ +package secrets + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gin-gonic/gin" + + "github.com/kandev/kandev/internal/common/logger" + ws "github.com/kandev/kandev/pkg/websocket" +) + +// testLogger returns an error-level JSON logger for tests. +func testLogger(t *testing.T) *logger.Logger { + t.Helper() + log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json"}) + if err != nil { + t.Fatalf("logger: %v", err) + } + return log +} + +// newSecretsHandlerService builds a service wired like main.go: the authorizer +// and existence checker classify missing/unauthorized workspaces as +// ErrWorkspaceAccessDenied, and injected raw errors pass through unclassified. +func newSecretsHandlerService(t *testing.T, allowed map[string]bool, authorizerErr, existenceErr error) *Service { + t.Helper() + store := newTestSQLiteStore(t) + svc := NewService(NewUserVisibleStore(store), nil) + svc.SetWorkspaceAuthorizer(func(_ context.Context, workspaceID string) error { + if authorizerErr != nil { + return authorizerErr + } + if allowed[workspaceID] { + return nil + } + return ErrWorkspaceAccessDenied + }) + svc.SetWorkspaceExistenceChecker(func(_ context.Context, workspaceID string) error { + if existenceErr != nil { + return existenceErr + } + if allowed[workspaceID] { + return nil + } + return ErrWorkspaceAccessDenied + }) + return svc +} + +// newSecretsHTTPRouter builds a gin engine with the secrets handler's HTTP routes registered. +func newSecretsHTTPRouter(t *testing.T, svc *Service) *gin.Engine { + t.Helper() + gin.SetMode(gin.TestMode) + router := gin.New() + NewHandler(svc, testLogger(t)).registerHTTP(router) + return router +} + +// doTransferHTTP performs an HTTP request against the router and returns the response recorder. +func doTransferHTTP(t *testing.T, router *gin.Engine, method, path, body string) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest(method, path, strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + return rec +} + +// decodeItem unmarshals a SecretListItem from the response body. +func decodeItem(t *testing.T, rec *httptest.ResponseRecorder) *SecretListItem { + t.Helper() + var item SecretListItem + if err := json.Unmarshal(rec.Body.Bytes(), &item); err != nil { + t.Fatalf("decode item from %d %s: %v", rec.Code, rec.Body.String(), err) + } + return &item +} + +// seedGlobalViaService creates a global secret via the service and returns its ID. +func seedGlobalViaService(t *testing.T, svc *Service, name, value string) string { + t.Helper() + item := mustCreateViaService(t, svc, name, value, ScopeGlobal, "") + return item.ID +} + +// TestHandlerCopy_GlobalToWorkspaceCreated verifies the copy endpoint returns 201 with the copied item and keeps the source. +func TestHandlerCopy_GlobalToWorkspaceCreated(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true}, nil, nil) + router := newSecretsHTTPRouter(t, svc) + id := seedGlobalViaService(t, svc, "API Key", "v") + + rec := doTransferHTTP(t, router, http.MethodPost, "/api/v1/secrets/"+id+"/copy", + `{"target_scope":"workspace","target_workspace_id":"workspace-a","name":"copied"}`) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, body = %s", rec.Code, rec.Body.String()) + } + item := decodeItem(t, rec) + if item.Scope != ScopeWorkspace || item.WorkspaceID != "workspace-a" || item.Name != "copied" { + t.Fatalf("item = %+v", item) + } + if _, err := svc.Get(context.Background(), id); err != nil { + t.Fatalf("source lost after copy: %v", err) + } +} + +// TestHandlerCopy_WorkspaceToWorkspace verifies the copy endpoint handles a +// workspace source (via ?workspace_id=) with a workspace target: 201 with the +// item scoped to the destination workspace, source intact. +func TestHandlerCopy_WorkspaceToWorkspace(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true, "workspace-b": true}, nil, nil) + router := newSecretsHTTPRouter(t, svc) + w := mustCreateViaService(t, svc, "ws-key", "v", ScopeWorkspace, "workspace-a") + + rec := doTransferHTTP(t, router, http.MethodPost, "/api/v1/secrets/"+w.ID+"/copy?workspace_id=workspace-a", + `{"target_scope":"workspace","target_workspace_id":"workspace-b","name":"copied"}`) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, body = %s", rec.Code, rec.Body.String()) + } + item := decodeItem(t, rec) + if item.Scope != ScopeWorkspace || item.WorkspaceID != "workspace-b" || item.Name != "copied" { + t.Fatalf("item = %+v", item) + } + if _, err := svc.GetForWorkspace(context.Background(), w.ID, "workspace-a"); err != nil { + t.Fatalf("source lost after copy: %v", err) + } +} + +// TestHandlerCopy_WorkspaceSourceRequiresWorkspaceID verifies a workspace source without workspace_id is rejected with 404. +func TestHandlerCopy_WorkspaceSourceRequiresWorkspaceID(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true}, nil, nil) + router := newSecretsHTTPRouter(t, svc) + w := mustCreateViaService(t, svc, "ws", "v", ScopeWorkspace, "workspace-a") + + rec := doTransferHTTP(t, router, http.MethodPost, "/api/v1/secrets/"+w.ID+"/copy", + `{"target_scope":"global","name":"x"}`) + if rec.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404 (workspace source without workspace_id)", rec.Code) + } +} + +// TestHandlerCopy_OmittedNameUsesSourceName verifies an omitted copy name falls back to the source name. +func TestHandlerCopy_OmittedNameUsesSourceName(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true}, nil, nil) + router := newSecretsHTTPRouter(t, svc) + id := seedGlobalViaService(t, svc, "API Key", "v") + + rec := doTransferHTTP(t, router, http.MethodPost, "/api/v1/secrets/"+id+"/copy", + `{"target_scope":"workspace","target_workspace_id":"workspace-a"}`) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, body = %s", rec.Code, rec.Body.String()) + } + if item := decodeItem(t, rec); item.Name != "API Key" { + t.Fatalf("name = %q, want source name", item.Name) + } +} + +// TestHandlerCopy_NullNameBadRequest verifies a null copy name is rejected with 400. +func TestHandlerCopy_NullNameBadRequest(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true}, nil, nil) + router := newSecretsHTTPRouter(t, svc) + id := seedGlobalViaService(t, svc, "g", "v") + + rec := doTransferHTTP(t, router, http.MethodPost, "/api/v1/secrets/"+id+"/copy", + `{"target_scope":"workspace","target_workspace_id":"workspace-a","name":null}`) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400 for null name", rec.Code) + } +} + +// TestHandlerMove_WorkspaceToGlobal verifies the move endpoint moves a workspace secret to global and removes the source. +func TestHandlerMove_WorkspaceToGlobal(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true}, nil, nil) + router := newSecretsHTTPRouter(t, svc) + w := mustCreateViaService(t, svc, "ws", "v", ScopeWorkspace, "workspace-a") + + rec := doTransferHTTP(t, router, http.MethodPost, "/api/v1/secrets/"+w.ID+"/move?workspace_id=workspace-a", + `{"target_scope":"global","name":"moved"}`) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, body = %s", rec.Code, rec.Body.String()) + } + item := decodeItem(t, rec) + if item.Scope != ScopeGlobal || item.Name != "moved" { + t.Fatalf("item = %+v", item) + } + if _, err := svc.GetWorkspaceSecret(context.Background(), w.ID, "workspace-a"); !errors.Is(err, ErrNotFound) { + t.Fatalf("source still present after move: %v", err) + } +} + +// TestHandlerCopy_SameScopeBadRequest verifies a same-scope copy is rejected with 400. +func TestHandlerCopy_SameScopeBadRequest(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true}, nil, nil) + router := newSecretsHTTPRouter(t, svc) + id := seedGlobalViaService(t, svc, "g", "v") + + rec := doTransferHTTP(t, router, http.MethodPost, "/api/v1/secrets/"+id+"/copy", + `{"target_scope":"global","name":"other"}`) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400 for same-scope", rec.Code) + } +} + +// TestHandlerCopy_NameConflict verifies a target name collision returns 409. +func TestHandlerCopy_NameConflict(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true}, nil, nil) + router := newSecretsHTTPRouter(t, svc) + mustCreateViaService(t, svc, "taken", "other", ScopeWorkspace, "workspace-a") + id := seedGlobalViaService(t, svc, "g", "v") + + rec := doTransferHTTP(t, router, http.MethodPost, "/api/v1/secrets/"+id+"/copy", + `{"target_scope":"workspace","target_workspace_id":"workspace-a","name":"taken"}`) + if rec.Code != http.StatusConflict { + t.Fatalf("status = %d, want 409", rec.Code) + } +} + +// TestHandlerCopy_MissingSourceNotFound verifies an unknown copy source returns 404. +func TestHandlerCopy_MissingSourceNotFound(t *testing.T) { + svc := newSecretsHandlerService(t, nil, nil, nil) + router := newSecretsHTTPRouter(t, svc) + rec := doTransferHTTP(t, router, http.MethodPost, "/api/v1/secrets/missing/copy", + `{"target_scope":"global","name":"x"}`) + if rec.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404", rec.Code) + } +} + +// TestHandlerCopy_UnauthorizedSourceNotFound verifies an unauthorized source workspace is reported as 404. +func TestHandlerCopy_UnauthorizedSourceNotFound(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true}, nil, nil) + router := newSecretsHTTPRouter(t, svc) + w := mustCreateViaService(t, svc, "ws", "v", ScopeWorkspace, "workspace-a") + svc.SetWorkspaceAuthorizer(func(context.Context, string) error { return ErrWorkspaceAccessDenied }) + svc.SetWorkspaceExistenceChecker(func(context.Context, string) error { return ErrWorkspaceAccessDenied }) + + rec := doTransferHTTP(t, router, http.MethodPost, "/api/v1/secrets/"+w.ID+"/copy?workspace_id=workspace-a", + `{"target_scope":"global","name":"x"}`) + if rec.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404 for unauthorized source", rec.Code) + } +} + +// TestHandlerCopy_UnauthorizedDestinationNotFound verifies an unauthorized destination workspace is reported as 404. +func TestHandlerCopy_UnauthorizedDestinationNotFound(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true}, nil, nil) + router := newSecretsHTTPRouter(t, svc) + id := seedGlobalViaService(t, svc, "g", "v") + + rec := doTransferHTTP(t, router, http.MethodPost, "/api/v1/secrets/"+id+"/copy", + `{"target_scope":"workspace","target_workspace_id":"workspace-b","name":"x"}`) + if rec.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404 for unauthorized destination", rec.Code) + } +} + +// TestHandlerCopy_NilExistenceCheckerNotFound verifies a nil existence checker fails closed with 404. +func TestHandlerCopy_NilExistenceCheckerNotFound(t *testing.T) { + store := newTestSQLiteStore(t) + svc := NewService(NewUserVisibleStore(store), nil) + svc.SetWorkspaceAuthorizer(func(context.Context, string) error { return nil }) + router := newSecretsHTTPRouter(t, svc) + id := seedGlobalViaService(t, svc, "g", "v") + + rec := doTransferHTTP(t, router, http.MethodPost, "/api/v1/secrets/"+id+"/copy", + `{"target_scope":"workspace","target_workspace_id":"any","name":"x"}`) + if rec.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404 (nil checker fails closed)", rec.Code) + } +} + +// TestHandlerCopy_InternalErrorSanitized verifies internal errors return 500 without leaking the error message. +func TestHandlerCopy_InternalErrorSanitized(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true}, errors.New("database unreachable"), nil) + router := newSecretsHTTPRouter(t, svc) + id := seedGlobalViaService(t, svc, "g", "v") + + rec := doTransferHTTP(t, router, http.MethodPost, "/api/v1/secrets/"+id+"/copy", + `{"target_scope":"workspace","target_workspace_id":"workspace-a","name":"x"}`) + if rec.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500", rec.Code) + } + if strings.Contains(rec.Body.String(), "database unreachable") { + t.Fatalf("500 body leaked internal error: %s", rec.Body.String()) + } +} + +// TestHandlerCopy_ValidationLikeInternalErrorStays500 verifies sentinel-based classification keeps a validation-sounding internal error at 500. +func TestHandlerCopy_ValidationLikeInternalErrorStays500(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true}, errors.New("name must be 1-100 characters"), nil) + router := newSecretsHTTPRouter(t, svc) + id := seedGlobalViaService(t, svc, "g", "v") + + rec := doTransferHTTP(t, router, http.MethodPost, "/api/v1/secrets/"+id+"/copy", + `{"target_scope":"workspace","target_workspace_id":"workspace-a","name":"x"}`) + if rec.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500 (classification is sentinel-based, not text-based)", rec.Code) + } +} + +// TestHandlerCopy_WhitespaceNameBadRequest verifies a whitespace-only name is rejected with 400. +func TestHandlerCopy_WhitespaceNameBadRequest(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true}, nil, nil) + router := newSecretsHTTPRouter(t, svc) + id := seedGlobalViaService(t, svc, "g", "v") + + rec := doTransferHTTP(t, router, http.MethodPost, "/api/v1/secrets/"+id+"/copy", + `{"target_scope":"workspace","target_workspace_id":"workspace-a","name":" "}`) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400 for whitespace name", rec.Code) + } +} + +// --- WebSocket handlers --- + +// newWSHandler builds a secrets Handler for WebSocket tests. +func newWSHandler(t *testing.T, svc *Service) *Handler { + t.Helper() + return NewHandler(svc, testLogger(t)) +} + +// wsTransfer dispatches a WebSocket copy/move message and returns the response message. +func wsTransfer(t *testing.T, h *Handler, action, payload string) (*ws.Message, error) { + t.Helper() + msg := &ws.Message{ID: "1", Action: action, Payload: json.RawMessage(payload)} + switch action { + case ws.ActionSecretCopy: + return h.wsCopy(context.Background(), msg) + case ws.ActionSecretMove: + return h.wsMove(context.Background(), msg) + } + t.Fatalf("unknown action %s", action) + return nil, nil +} + +type wsErrorPayload struct { + Code string `json:"code"` + Message string `json:"message"` +} + +// decodeWSError unmarshals a wsErrorPayload from a WebSocket response. +func decodeWSError(t *testing.T, resp *ws.Message) wsErrorPayload { + t.Helper() + var errPayload wsErrorPayload + if err := json.Unmarshal(resp.Payload, &errPayload); err != nil { + t.Fatalf("decode error payload %s: %v", string(resp.Payload), err) + } + return errPayload +} + +// decodeWSItem unmarshals a SecretListItem from a WebSocket response. +func decodeWSItem(t *testing.T, resp *ws.Message) *SecretListItem { + t.Helper() + var item SecretListItem + if err := json.Unmarshal(resp.Payload, &item); err != nil { + t.Fatalf("decode item payload %s: %v", string(resp.Payload), err) + } + return &item +} + +// TestWSHandler_CopySuccessAndShape verifies a WebSocket copy returns the copied item with the expected shape. +func TestWSHandler_CopySuccessAndShape(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true}, nil, nil) + h := newWSHandler(t, svc) + id := seedGlobalViaService(t, svc, "API Key", "v") + + resp, err := wsTransfer(t, h, ws.ActionSecretCopy, + `{"id":"`+id+`","target_scope":"workspace","target_workspace_id":"workspace-a","name":"copied"}`) + if err != nil { + t.Fatalf("wsCopy: %v", err) + } + item := decodeWSItem(t, resp) + if item.Scope != ScopeWorkspace || item.WorkspaceID != "workspace-a" || item.Name != "copied" { + t.Fatalf("item = %+v", item) + } +} + +// TestWSHandler_CopyNullNameBadRequest verifies a WebSocket copy with a null name returns a BAD_REQUEST error. +func TestWSHandler_CopyNullNameBadRequest(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true}, nil, nil) + h := newWSHandler(t, svc) + id := seedGlobalViaService(t, svc, "g", "v") + + resp, err := wsTransfer(t, h, ws.ActionSecretCopy, + `{"id":"`+id+`","target_scope":"workspace","target_workspace_id":"workspace-a","name":null}`) + if err != nil { + t.Fatalf("wsCopy: %v", err) + } + if got := decodeWSError(t, resp); got.Code != ws.ErrorCodeBadRequest { + t.Fatalf("code = %q, want BAD_REQUEST", got.Code) + } +} + +// TestWSHandler_CopyConflict verifies a WebSocket copy name collision returns a CONFLICT error. +func TestWSHandler_CopyConflict(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true}, nil, nil) + h := newWSHandler(t, svc) + mustCreateViaService(t, svc, "taken", "other", ScopeWorkspace, "workspace-a") + id := seedGlobalViaService(t, svc, "g", "v") + + resp, err := wsTransfer(t, h, ws.ActionSecretCopy, + `{"id":"`+id+`","target_scope":"workspace","target_workspace_id":"workspace-a","name":"taken"}`) + if err != nil { + t.Fatalf("wsCopy: %v", err) + } + if got := decodeWSError(t, resp); got.Code != ws.ErrorCodeConflict { + t.Fatalf("code = %q, want CONFLICT", got.Code) + } +} + +// TestWSHandler_CopyMissingSourceNotFound verifies a WebSocket copy of an unknown source returns NOT_FOUND. +func TestWSHandler_CopyMissingSourceNotFound(t *testing.T) { + svc := newSecretsHandlerService(t, nil, nil, nil) + h := newWSHandler(t, svc) + resp, err := wsTransfer(t, h, ws.ActionSecretCopy, + `{"id":"missing","target_scope":"global","name":"x"}`) + if err != nil { + t.Fatalf("wsCopy: %v", err) + } + if got := decodeWSError(t, resp); got.Code != ws.ErrorCodeNotFound { + t.Fatalf("code = %q, want NOT_FOUND", got.Code) + } +} + +// TestWSHandler_CopyUnauthorizedDestinationNotFound verifies an unauthorized WebSocket copy destination returns NOT_FOUND. +func TestWSHandler_CopyUnauthorizedDestinationNotFound(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true}, nil, nil) + h := newWSHandler(t, svc) + id := seedGlobalViaService(t, svc, "g", "v") + + resp, err := wsTransfer(t, h, ws.ActionSecretCopy, + `{"id":"`+id+`","target_scope":"workspace","target_workspace_id":"workspace-b","name":"x"}`) + if err != nil { + t.Fatalf("wsCopy: %v", err) + } + if got := decodeWSError(t, resp); got.Code != ws.ErrorCodeNotFound { + t.Fatalf("code = %q, want NOT_FOUND", got.Code) + } +} + +// TestWSHandler_CopyInternalErrorSanitized verifies WebSocket copy internal errors return INTERNAL_ERROR without leaking details. +func TestWSHandler_CopyInternalErrorSanitized(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true}, errors.New("database unreachable"), nil) + h := newWSHandler(t, svc) + id := seedGlobalViaService(t, svc, "g", "v") + + resp, err := wsTransfer(t, h, ws.ActionSecretCopy, + `{"id":"`+id+`","target_scope":"workspace","target_workspace_id":"workspace-a","name":"x"}`) + if err != nil { + t.Fatalf("wsCopy: %v", err) + } + got := decodeWSError(t, resp) + if got.Code != ws.ErrorCodeInternalError { + t.Fatalf("code = %q, want INTERNAL_ERROR", got.Code) + } + if strings.Contains(got.Message, "database unreachable") { + t.Fatalf("error message leaked internal detail: %q", got.Message) + } +} + +// TestWSHandler_WorkspaceSourceWithoutWorkspaceIDNotFound verifies a WebSocket copy of a workspace source without workspace_id returns NOT_FOUND. +func TestWSHandler_WorkspaceSourceWithoutWorkspaceIDNotFound(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true}, nil, nil) + h := newWSHandler(t, svc) + w := mustCreateViaService(t, svc, "ws", "v", ScopeWorkspace, "workspace-a") + + resp, err := wsTransfer(t, h, ws.ActionSecretCopy, + `{"id":"`+w.ID+`","target_scope":"global","name":"x"}`) + if err != nil { + t.Fatalf("wsCopy: %v", err) + } + if got := decodeWSError(t, resp); got.Code != ws.ErrorCodeNotFound { + t.Fatalf("code = %q, want NOT_FOUND (workspace source without workspace_id)", got.Code) + } +} + +// TestWSHandler_ReusedEnvelopeResetsAllFields verifies reusing a wsTransferPayload across decodes fully resets all fields, including after malformed and semantic decode failures. +func TestWSHandler_ReusedEnvelopeResetsAllFields(t *testing.T) { + svc := newSecretsHandlerService(t, map[string]bool{"workspace-a": true, "workspace-b": true}, nil, nil) + h := newWSHandler(t, svc) + wsSecretA := mustCreateViaService(t, svc, "ws-a", "va", ScopeWorkspace, "workspace-a") + global2 := seedGlobalViaService(t, svc, "g2", "vg") + + // First message: workspace source, workspace target, explicit null name. + first, err := wsTransfer(t, h, ws.ActionSecretCopy, + `{"id":"`+wsSecretA.ID+`","workspace_id":"workspace-a","target_scope":"workspace","target_workspace_id":"workspace-b","name":null}`) + if err != nil { + t.Fatalf("first message: %v", err) + } + if got := decodeWSError(t, first); got.Code != ws.ErrorCodeBadRequest { + t.Fatalf("first code = %q, want BAD_REQUEST", got.Code) + } + + // Second message: a different source (Global), workspace target A, omitted + // name, NO source workspace_id. If the decoder leaked the stale source + // workspace_id, the Global source would be mis-dispatched as a workspace + // source (404); a clean reset succeeds with the default source name. + second, err := wsTransfer(t, h, ws.ActionSecretCopy, + `{"id":"`+global2+`","target_scope":"workspace","target_workspace_id":"workspace-a"}`) + if err != nil { + t.Fatalf("second message: %v", err) + } + item := decodeWSItem(t, second) + if item.Scope != ScopeWorkspace || item.WorkspaceID != "workspace-a" || item.Name != "g2" { + t.Fatalf("second item = %+v (stale fields leaked between decodes)", item) + } + + // Decoder-level reuse: one wsTransferPayload instance decodes BOTH + // messages. The second decode must fully reset id, workspace_id, scope, + // and the name presence markers. + var payload wsTransferPayload + firstBody := `{"id":"` + wsSecretA.ID + `","workspace_id":"workspace-a","target_scope":"workspace","target_workspace_id":"workspace-b","name":null}` + if err := json.Unmarshal([]byte(firstBody), &payload); err != nil { + t.Fatalf("decode first into shared payload: %v", err) + } + // Local alias for the embedded request: promoted selectors for + // WorkspaceID would be ambiguous (the envelope's source workspace id + // shadows the embedded target workspace id). + target := &payload.CopySecretRequest + if !target.nameNull || payload.ID != wsSecretA.ID { + t.Fatalf("first shared decode = %+v", payload) + } + secondBody := `{"id":"` + global2 + `","target_scope":"workspace","target_workspace_id":"workspace-a"}` + if err := json.Unmarshal([]byte(secondBody), &payload); err != nil { + t.Fatalf("decode second into shared payload: %v", err) + } + if payload.ID != global2 || payload.WorkspaceID != "" || + target.Scope != ScopeWorkspace || target.WorkspaceID != "workspace-a" || + target.Name != nil || target.nameSet || target.nameNull { + t.Fatalf("shared payload after second decode = %+v (stale fields leaked)", payload) + } + + // A malformed second decode must leave the receiver fully reset, never + // carrying state from the first decode. + staleName := "stale" + bad := wsTransferPayload{ + ID: "stale-id", + WorkspaceID: "stale-ws", + CopySecretRequest: CopySecretRequest{ + Scope: ScopeWorkspace, + WorkspaceID: "stale-ws", + Name: &staleName, + nameSet: true, + }, + } + if err := bad.UnmarshalJSON([]byte(`{"id":`)); err == nil { + t.Fatal("malformed decode succeeded") + } + badTarget := &bad.CopySecretRequest + if bad.ID != "" || bad.WorkspaceID != "" || badTarget.Scope != "" || + badTarget.WorkspaceID != "" || badTarget.Name != nil || + badTarget.nameSet || badTarget.nameNull { + t.Fatalf("malformed decode left stale state: %+v", bad) + } + + // A SEMANTIC decode failure (invalid `name` type after valid top-level + // fields) must also leave the receiver fully reset, never partially + // populated with the already-parsed scope/workspace fields. + var semantic wsTransferPayload + if err := json.Unmarshal([]byte(firstBody), &semantic); err != nil { + t.Fatalf("seed semantic payload: %v", err) + } + if err := json.Unmarshal( + []byte(`{"id":"`+global2+`","target_scope":"workspace","target_workspace_id":"workspace-a","name":123}`), + &semantic, + ); err == nil { + t.Fatal("semantic decode with a numeric name succeeded") + } + semanticTarget := &semantic.CopySecretRequest + if semantic.ID != "" || semantic.WorkspaceID != "" || semanticTarget.Scope != "" || + semanticTarget.WorkspaceID != "" || semanticTarget.Name != nil || + semanticTarget.nameSet || semanticTarget.nameNull { + t.Fatalf("semantic decode left partially populated state: %+v", semantic) + } +} diff --git a/apps/backend/internal/secrets/models.go b/apps/backend/internal/secrets/models.go index d420587f61..6a8660af53 100644 --- a/apps/backend/internal/secrets/models.go +++ b/apps/backend/internal/secrets/models.go @@ -1,6 +1,9 @@ package secrets -import "time" +import ( + "encoding/json" + "time" +) // SecretScope identifies the ownership boundary for a secret. type SecretScope string @@ -65,3 +68,62 @@ type UpdateSecretRequest struct { type RevealSecretResponse struct { Value string `json:"value"` } + +// CopySecretRequest is the request body for copy/move operations. Name uses +// presence semantics: a JSON-omitted name means "use the source secret's +// name"; an explicit null, empty, or whitespace-only name is a validation +// error. +type CopySecretRequest struct { + Scope SecretScope `json:"target_scope"` + WorkspaceID string `json:"target_workspace_id"` + Name *string `json:"name,omitempty"` + + nameSet bool + nameNull bool +} + +// UnmarshalJSON implements presence-aware decoding so handlers can distinguish +// an omitted name (default to the source name) from an explicit null (a +// validation error). Go's standard decoding collapses both to a nil pointer, +// so this is the only way to honor the contract. Every field, including the +// presence markers, is reset on each decode so a reused request struct cannot +// leak state between messages. +func (r *CopySecretRequest) UnmarshalJSON(data []byte) error { + // Reset every field (including the presence markers) BEFORE parsing so a + // failed decode can never leave stale state from an earlier message, and + // assign the receiver only after EVERY sub-decode succeeds so a semantic + // failure (e.g. `name: 123`) cannot leave a partially populated receiver. + r.Scope = "" + r.WorkspaceID = "" + r.Name = nil + r.nameSet = false + r.nameNull = false + var aux struct { + Scope SecretScope `json:"target_scope"` + WorkspaceID string `json:"target_workspace_id"` + Name json.RawMessage `json:"name"` + } + if err := json.Unmarshal(data, &aux); err != nil { + return err + } + var name *string + nameSet := len(aux.Name) > 0 + nameNull := false + if nameSet { + if string(aux.Name) == "null" { + nameNull = true + } else { + var value string + if err := json.Unmarshal(aux.Name, &value); err != nil { + return err + } + name = &value + } + } + r.Scope = aux.Scope + r.WorkspaceID = aux.WorkspaceID + r.Name = name + r.nameSet = nameSet + r.nameNull = nameNull + return nil +} diff --git a/apps/backend/internal/secrets/postgres_transfer_test.go b/apps/backend/internal/secrets/postgres_transfer_test.go new file mode 100644 index 0000000000..e6e897aeb9 --- /dev/null +++ b/apps/backend/internal/secrets/postgres_transfer_test.go @@ -0,0 +1,390 @@ +package secrets + +import ( + "context" + "errors" + "strings" + "testing" + "time" + + "github.com/google/uuid" + "github.com/jmoiron/sqlx" + + "github.com/kandev/kandev/internal/testutil" +) + +// openPGWithConns opens an isolated Postgres schema with a pool of conns +// connections and search_path set per connection (via DSN options), so race +// tests can hold two real connections at once. A one-connection pool trivially +// serializes and proves nothing about the advisory locks. +func openPGWithConns(t *testing.T, conns int) *sqlx.DB { + t.Helper() + dsn := testutil.PostgresDSNFromEnv(t) + + schema := "kandev_test_" + strings.ReplaceAll(uuid.NewString(), "-", "") + admin, err := sqlx.Open("pgx", dsn) + if err != nil { + t.Fatalf("open postgres: %v", err) + } + if _, err := admin.Exec("CREATE SCHEMA " + schema); err != nil { + _ = admin.Close() + t.Fatalf("create schema: %v", err) + } + _ = admin.Close() + t.Cleanup(func() { + cleanup, cerr := sqlx.Open("pgx", dsn) + if cerr == nil { + _, _ = cleanup.Exec("DROP SCHEMA IF EXISTS " + schema + " CASCADE") + _ = cleanup.Close() + } + }) + + db, err := sqlx.Open("pgx", dsn+" options=-csearch_path="+schema) + if err != nil { + t.Fatalf("open postgres pool: %v", err) + } + db.SetMaxOpenConns(conns) + db.SetMaxIdleConns(conns) + t.Cleanup(func() { _ = db.Close() }) + return db +} + +// newPGStore builds a secrets store over the given Postgres pool with a fresh master key. +func newPGStore(t *testing.T, db *sqlx.DB) *sqliteStore { + t.Helper() + crypto, err := NewMasterKeyProvider(t.TempDir()) + if err != nil { + t.Fatalf("master key: %v", err) + } + store, cleanup, err := Provide(db, db, crypto) + if err != nil { + t.Fatalf("provide store: %v", err) + } + t.Cleanup(func() { _ = cleanup() }) + return store +} + +// TestPostgresTransfer_CopyMoveConflictAndOwnership verifies Postgres copy/move metadata, name conflicts (including legacy empty-scope globals), and source removal on move. +func TestPostgresTransfer_CopyMoveConflictAndOwnership(t *testing.T) { + db := openPGWithConns(t, 2) + store := newPGStore(t, db) + ctx := context.Background() + + mustCreate(t, store, &SecretWithValue{Secret: Secret{ID: "g1", Name: "orig"}, Value: "v"}) + + // Copy global -> workspace with target identity. + got, err := store.CopyScoped(ctx, "g1", "", ScopeWorkspace, "workspace-a", strPtr("copied"), nil) + if err != nil { + t.Fatalf("CopyScoped: %v", err) + } + if got.Scope != ScopeWorkspace || got.WorkspaceID != "workspace-a" || got.Name != "copied" { + t.Fatalf("copy metadata = %+v", got) + } + if got.CreatedAt.IsZero() || got.UpdatedAt.IsZero() { + t.Fatalf("copy timestamps zero: %+v", got) + } + if value, err := store.RevealForWorkspace(ctx, got.ID, "workspace-a"); err != nil || value != "v" { + t.Fatalf("reveal = %q, %v", value, err) + } + + // Conflict on a second copy of the same target name. + if _, err := store.CopyScoped(ctx, "g1", "", ScopeWorkspace, "workspace-a", strPtr("copied"), nil); !errors.Is(err, ErrSecretNameConflict) { + t.Fatalf("conflict err = %v", err) + } + + // Move workspace -> global removes the source. + moved, err := store.MoveScoped(ctx, "g1", "", ScopeGlobal, "", strPtr("global-copy"), nil) + if err != nil { + t.Fatalf("MoveScoped: %v", err) + } + if moved.Scope != ScopeGlobal { + t.Fatalf("moved scope = %q", moved.Scope) + } + if _, err := store.Get(ctx, "g1"); !errors.Is(err, ErrNotFound) { + t.Fatalf("source still present: %v", err) + } + + // Legacy empty-scope Global row conflicts with a Global target. + ciphertext, nonce, err := Encrypt([]byte("legacy"), store.crypto.Key()) + if err != nil { + t.Fatalf("encrypt: %v", err) + } + now := time.Now().UTC() + if _, err := db.ExecContext(ctx, db.Rebind(` + INSERT INTO secrets (id, name, user_id, scope, workspace_id, encrypted_value, nonce, created_at, updated_at) + VALUES (?, ?, '', '', '', ?, ?, ?, ?)`), + "legacy-1", "legacy-name", ciphertext, nonce, now, now); err != nil { + t.Fatalf("seed legacy row: %v", err) + } + mustCreate(t, store, &SecretWithValue{Secret: Secret{ID: "g2", Name: "src2"}, Value: "v2"}) + if _, err := store.CopyScoped(ctx, "g2", "", ScopeGlobal, "", strPtr("legacy-name"), nil); !errors.Is(err, ErrSecretNameConflict) { + t.Fatalf("legacy conflict err = %v", err) + } +} + +// TestPostgresTransfer_MoveRollbackAfterInsert verifies a failed source delete after insert rolls back the copy, leaving the source intact and no destination row. +func TestPostgresTransfer_MoveRollbackAfterInsert(t *testing.T) { + db := openPGWithConns(t, 2) + store := newPGStore(t, db) + ctx := context.Background() + mustCreate(t, store, &SecretWithValue{Secret: Secret{ID: "g1", Name: "orig"}, Value: "v"}) + + store.failAfterInsert = func() error { return errors.New("injected delete failure") } + if _, err := store.MoveScoped(ctx, "g1", "", ScopeWorkspace, "workspace-a", strPtr("copied"), nil); err == nil { + t.Fatal("MoveScoped succeeded despite failpoint") + } + if _, err := store.Get(ctx, "g1"); err != nil { + t.Fatalf("source lost after rollback: %v", err) + } + items, err := store.ListScoped(ctx, SecretListOptions{Scope: ScopeWorkspace, WorkspaceID: "workspace-a"}) + if err != nil || len(items) != 0 { + t.Fatalf("workspace rows = %+v, %v; want none", items, err) + } +} + +// TestPostgresTransfer_ConcurrentSameTarget proves exactly one winner for +// concurrent same-name transfers to a Global target, serialized by the Global +// advisory lock. +func TestPostgresTransfer_ConcurrentSameTarget(t *testing.T) { + db := openPGWithConns(t, 3) + storeA := newPGStore(t, db) + storeB := newPGStore(t, db) + ctx := context.Background() + mustCreate(t, storeA, &SecretWithValue{Secret: Secret{ID: "src", Name: "orig"}, Value: "v"}) + + locked := make(chan struct{}) + release := make(chan struct{}) + done := make(chan error, 1) + go func() { + _, err := storeA.CopyScoped(ctx, "src", "", ScopeGlobal, "", strPtr("dup"), func(context.Context) error { + close(locked) + <-release + return nil + }) + done <- err + }() + <-locked + + bErr := make(chan error, 1) + go func() { + _, err := storeB.CopyScoped(ctx, "src", "", ScopeGlobal, "", strPtr("dup"), nil) + bErr <- err + }() + select { + case err := <-bErr: + t.Fatalf("B completed while A held the advisory lock: %v", err) + case <-time.After(300 * time.Millisecond): + } + close(release) + if err := <-done; err != nil { + t.Fatalf("A: %v", err) + } + if err := <-bErr; !errors.Is(err, ErrSecretNameConflict) { + t.Fatalf("B error = %v, want ErrSecretNameConflict", err) + } +} + +// TestPostgresTransfer_DeleteVsTransferRace pins both interleavings of the +// shared workspace advisory lock: a transfer that inserts before a deletion +// commits is cleaned up by that deletion, and a deletion that commits first +// leaves nothing for a later transfer to attach to. +func TestPostgresTransfer_DeleteVsTransferRace(t *testing.T) { + db := openPGWithConns(t, 3) + store := newPGStore(t, db) + ctx := context.Background() + mustCreate(t, store, &SecretWithValue{Secret: Secret{ID: "src", Name: "orig"}, Value: "v"}) + + t.Run("transfer locked before deletion cleanup", func(t *testing.T) { + locked := make(chan struct{}) + release := make(chan struct{}) + transferDone := make(chan error, 1) + go func() { + _, err := store.CopyScoped(ctx, "src", "", ScopeWorkspace, "workspace-a", strPtr("copied"), func(context.Context) error { + close(locked) + <-release + return nil + }) + transferDone <- err + }() + <-locked + + delDone := make(chan error, 1) + go func() { + tx, err := db.BeginTxx(ctx, nil) + if err != nil { + delDone <- err + return + } + defer func() { _ = tx.Rollback() }() + if err := store.DeleteWorkspaceSecretsTx(ctx, tx, "workspace-a"); err != nil { + delDone <- err + return + } + delDone <- tx.Commit() + }() + select { + case err := <-delDone: + t.Fatalf("deletion completed while the transfer held the lock: %v", err) + case <-time.After(300 * time.Millisecond): + } + + close(release) + if err := <-transferDone; err != nil { + t.Fatalf("transfer: %v", err) + } + if err := <-delDone; err != nil { + t.Fatalf("deletion: %v", err) + } + // The deletion ran after the transfer inserted and removed the copy: + // no secret may remain attached to the deleted workspace. + items, err := store.ListScoped(ctx, SecretListOptions{Scope: ScopeWorkspace, WorkspaceID: "workspace-a"}) + if err != nil { + t.Fatalf("list: %v", err) + } + if len(items) != 0 { + t.Fatalf("workspace rows = %+v, want none after deletion", items) + } + }) + + t.Run("deletion committed before transfer existence check", func(t *testing.T) { + tx, err := db.BeginTxx(ctx, nil) + if err != nil { + t.Fatalf("begin deletion: %v", err) + } + if err := store.DeleteWorkspaceSecretsTx(ctx, tx, "workspace-b"); err != nil { + _ = tx.Rollback() + t.Fatalf("delete: %v", err) + } + if err := tx.Commit(); err != nil { + t.Fatalf("commit deletion: %v", err) + } + + // The transfer's existence check (the callback, run under the lock) + // denies a workspace that no longer exists; nothing is inserted. + denied := errors.New("workspace gone") + _, err = store.CopyScoped(ctx, "src", "", ScopeWorkspace, "workspace-b", strPtr("copied"), func(context.Context) error { + return denied + }) + if !errors.Is(err, denied) { + t.Fatalf("err = %v, want the denied callback error", err) + } + items, err := store.ListScoped(ctx, SecretListOptions{Scope: ScopeWorkspace, WorkspaceID: "workspace-b"}) + if err != nil || len(items) != 0 { + t.Fatalf("workspace rows = %+v, %v; want none", items, err) + } + }) +} + +// TestPostgresTransfer_MoveSerializesConcurrentUpdate proves the Move source +// row is locked with FOR UPDATE: a concurrent Update cannot interleave between +// Move's read and delete, so no newer value is lost. +func TestPostgresTransfer_MoveSerializesConcurrentUpdate(t *testing.T) { + db := openPGWithConns(t, 3) + store := newPGStore(t, db) + ctx := context.Background() + mustCreate(t, store, &SecretWithValue{Secret: Secret{ID: "src", Name: "orig"}, Value: "A"}) + + locked := make(chan struct{}) + release := make(chan struct{}) + moveDone := make(chan error, 1) + store.afterSourceLock = func() error { + close(locked) + <-release + return nil + } + go func() { + _, err := store.MoveScoped(ctx, "src", "", ScopeWorkspace, "workspace-a", strPtr("copied"), nil) + moveDone <- err + }() + + // Move holds the FOR UPDATE source row lock. + <-locked + + updateDone := make(chan error, 1) + go func() { + updateDone <- store.Update(ctx, "src", &UpdateSecretRequest{Value: stringPtr("B")}) + }() + + select { + case err := <-updateDone: + t.Fatalf("Update completed while Move held the source row lock: %v", err) + case <-time.After(300 * time.Millisecond): + // Blocked on the row lock, as intended. + } + + close(release) + if err := <-moveDone; err != nil { + t.Fatalf("move: %v", err) + } + // The Update completes after Move commits; the row is gone, so it touches + // nothing and the newer value B is not lost anywhere. + if err := <-updateDone; err != nil { + t.Fatalf("update: %v", err) + } + + if _, err := store.Get(ctx, "src"); !errors.Is(err, ErrNotFound) { + t.Fatalf("source still present after move: %v", err) + } + items, err := store.ListScoped(ctx, SecretListOptions{Scope: ScopeWorkspace, WorkspaceID: "workspace-a"}) + if err != nil { + t.Fatalf("list workspace: %v", err) + } + var moved *SecretListItem + for _, item := range items { + if item.Name == "copied" { + moved = item + } + } + if moved == nil { + t.Fatalf("moved copy not found in workspace-a: %+v", items) + } + value, err := store.RevealForWorkspace(ctx, moved.ID, "workspace-a") + if err != nil { + t.Fatalf("reveal moved: %v", err) + } + if value != "A" { + t.Fatalf("moved value = %q, want A (concurrent update must not be lost)", value) + } +} + +// TestPostgresTransfer_ConditionalInsertRejectsLateConflict verifies the +// transfer insert is guarded by an atomic NOT EXISTS: an ordinary create or +// rename (which does not take the transfer's advisory lock) committing the +// target name between the eager conflict check and the insert must produce a +// clean ErrSecretNameConflict, never a duplicate row. +func TestPostgresTransfer_ConditionalInsertRejectsLateConflict(t *testing.T) { + db := openPGWithConns(t, 2) + store := newPGStore(t, db) + ctx := context.Background() + + mustCreate(t, store, &SecretWithValue{Secret: Secret{ID: "g1", Name: "orig"}, Value: "v"}) + + // Commit the target name from a second connection at the exact point + // between the transfer's conflict check and its conditional insert. + store.beforeTransferInsert = func() error { + _, err := db.Exec(` + INSERT INTO secrets (id, name, user_id, scope, workspace_id, encrypted_value, nonce, created_at, updated_at) + VALUES ('late', 'copied', '', 'workspace', 'workspace-a', '\x01'::bytea, '\x01'::bytea, now(), now())`) + return err + } + _, err := store.CopyScoped(ctx, "g1", "", ScopeWorkspace, "workspace-a", strPtr("copied"), nil) + if !errors.Is(err, ErrSecretNameConflict) { + t.Fatalf("error = %v, want ErrSecretNameConflict (atomic NOT EXISTS must observe the late row)", err) + } + + // Exactly one 'copied' row in workspace-a: the late create won, the + // transfer did not insert a duplicate. + items, err := store.ListScoped(ctx, SecretListOptions{Scope: ScopeWorkspace, WorkspaceID: "workspace-a"}) + if err != nil { + t.Fatalf("list: %v", err) + } + count := 0 + for _, item := range items { + if item.Name == "copied" { + count++ + } + } + if count != 1 { + t.Fatalf("'copied' rows in workspace-a = %d, want 1 (no duplicate from the transfer)", count) + } +} diff --git a/apps/backend/internal/secrets/service.go b/apps/backend/internal/secrets/service.go index d235c91e81..3f05de23d3 100644 --- a/apps/backend/internal/secrets/service.go +++ b/apps/backend/internal/secrets/service.go @@ -13,6 +13,7 @@ type Service struct { store SecretStore logger *logger.Logger workspaceAuthorizer func(context.Context, string) error + workspaceExistence func(context.Context, string) error } // NewService creates a new secrets service. @@ -30,11 +31,21 @@ func (s *Service) SetWorkspaceAuthorizer(authorizer func(context.Context, string s.workspaceAuthorizer = authorizer } +// SetWorkspaceExistenceChecker wires a workspace-existence check that runs +// unconditionally for copy/move destinations, including auth-disabled +// contexts where the authorizer is a no-op. A nil checker fails closed: +// workspace destinations resolve as ErrWorkspaceAccessDenied. +func (s *Service) SetWorkspaceExistenceChecker(checker func(context.Context, string) error) { + s.workspaceExistence = checker +} + +// validateCreate validates a create request, trimming the name and checking +// length, scope, and workspace constraints. func (s *Service) validateCreate(req *CreateSecretRequest) error { req.Name = strings.TrimSpace(req.Name) if req.Name == "" || len(req.Name) > 100 { - return fmt.Errorf("name must be 1-100 characters") + return fmt.Errorf("name must be 1-100 bytes") } if req.Value == "" || len(req.Value) > 10000 { return fmt.Errorf("value must be 1-10000 characters") @@ -48,12 +59,14 @@ func (s *Service) validateCreate(req *CreateSecretRequest) error { return nil } +// validateUpdate validates an update request, trimming the name and checking +// length constraints on the fields that are present. func (s *Service) validateUpdate(req *UpdateSecretRequest) error { if req.Name != nil { name := strings.TrimSpace(*req.Name) req.Name = &name if name == "" || len(name) > 100 { - return fmt.Errorf("name must be 1-100 characters") + return fmt.Errorf("name must be 1-100 bytes") } } if req.Value != nil && (len(*req.Value) == 0 || len(*req.Value) > 10000) { @@ -154,6 +167,8 @@ func (s *Service) RevealForWorkspace(ctx context.Context, id, workspaceID string return s.store.Reveal(ctx, id) } +// RevealWorkspaceSecret returns the decrypted value of a Workspace secret +// after checking the caller's workspace access. func (s *Service) RevealWorkspaceSecret(ctx context.Context, id, workspaceID string) (string, error) { if _, err := s.GetWorkspaceSecret(ctx, id, workspaceID); err != nil { return "", err @@ -204,6 +219,8 @@ func (s *Service) UpdateForWorkspace(ctx context.Context, id, workspaceID string return secretListItem(secret), nil } +// UpdateWorkspaceSecret validates and updates a Workspace secret after +// checking the caller's workspace access. func (s *Service) UpdateWorkspaceSecret(ctx context.Context, id, workspaceID string, req *UpdateSecretRequest) (*SecretListItem, error) { if err := s.validateUpdate(req); err != nil { return nil, fmt.Errorf("validation: %w", err) @@ -237,6 +254,8 @@ func (s *Service) DeleteForWorkspace(ctx context.Context, id, workspaceID string return s.store.Delete(ctx, id) } +// DeleteWorkspaceSecret deletes a Workspace secret after checking the +// caller's workspace access. func (s *Service) DeleteWorkspaceSecret(ctx context.Context, id, workspaceID string) error { if _, err := s.GetWorkspaceSecret(ctx, id, workspaceID); err != nil { return err @@ -272,6 +291,8 @@ func (s *Service) ListScoped(ctx context.Context, opts SecretListOptions) ([]*Se return nil, fmt.Errorf("workspace-scoped secret storage is unavailable") } +// authorizeScope enforces workspace authorization for workspace-scoped +// requests; global scope requires no check. func (s *Service) authorizeScope(ctx context.Context, scope SecretScope, workspaceID string) error { if scope != ScopeWorkspace { return nil @@ -282,6 +303,8 @@ func (s *Service) authorizeScope(ctx context.Context, scope SecretScope, workspa return s.workspaceAuthorizer(ctx, workspaceID) } +// secretListItem converts a stored secret into a list item with a +// normalized scope. func secretListItem(secret *Secret) *SecretListItem { return &SecretListItem{ ID: secret.ID, @@ -293,3 +316,115 @@ func secretListItem(secret *Secret) *SecretListItem { UpdatedAt: secret.UpdatedAt, } } + +// Copy copies a secret into another scope/workspace without removing the +// source. Errors are classified with the package sentinels: validation +// failures wrap ErrSecretValidation, missing/unauthorized sources and +// destinations surface ErrNotFound or ErrWorkspaceAccessDenied, and raw +// lookup/storage failures pass through unclassified (sanitized 500 upstream). +func (s *Service) Copy(ctx context.Context, sourceID, sourceWorkspaceID string, req *CopySecretRequest) (*SecretListItem, error) { + return s.copyOrMove(ctx, sourceID, sourceWorkspaceID, req, false) +} + +// Move copies a secret into another scope/workspace and removes the source in +// one atomic store operation. +func (s *Service) Move(ctx context.Context, sourceID, sourceWorkspaceID string, req *CopySecretRequest) (*SecretListItem, error) { + return s.copyOrMove(ctx, sourceID, sourceWorkspaceID, req, true) +} + +// copyOrMove is the shared implementation behind Copy and Move: it validates +// the request, resolves the source secret and target name, and performs the +// copy or move in one store operation. +func (s *Service) copyOrMove(ctx context.Context, sourceID, sourceWorkspaceID string, req *CopySecretRequest, move bool) (*SecretListItem, error) { + if req == nil { + return nil, fmt.Errorf("%w: request is required", ErrSecretValidation) + } + if err := validateSecretScope(req.Scope, req.WorkspaceID); err != nil { + return nil, fmt.Errorf("%w: %v", ErrSecretValidation, err) + } + + var source *Secret + var err error + if strings.TrimSpace(sourceWorkspaceID) == "" { + source, err = s.Get(ctx, sourceID) + } else { + source, err = s.GetWorkspaceSecret(ctx, sourceID, sourceWorkspaceID) + } + if err != nil { + return nil, err + } + + // An explicit name (or an explicit null) is validated here; an omitted + // name is resolved from the source row inside the store's transaction so + // a concurrent rename can never split the copied name from its value. + if req.nameNull || req.Name != nil { + if _, err := resolveTransferName(req, ""); err != nil { + return nil, err + } + } + + if normalizeStoredScope(req.Scope) == normalizeStoredScope(source.Scope) && req.WorkspaceID == source.WorkspaceID { + return nil, fmt.Errorf("%w: source and destination scope are the same", ErrSecretValidation) + } + + transferStore, ok := s.store.(SecretTransferStore) + if !ok { + return nil, fmt.Errorf("secret transfer storage is unavailable") + } + + verifyDestination := s.destinationVerifier(ctx, req) + + var secret *Secret + if move { + secret, err = transferStore.MoveScoped(ctx, sourceID, source.WorkspaceID, req.Scope, req.WorkspaceID, req.Name, verifyDestination) + } else { + secret, err = transferStore.CopyScoped(ctx, sourceID, source.WorkspaceID, req.Scope, req.WorkspaceID, req.Name, verifyDestination) + } + if err != nil { + return nil, err + } + return secretListItem(secret), nil +} + +// resolveTransferName applies the name presence contract: an omitted name +// (and the absence of an explicit null) uses the source name; an explicit +// null, empty, whitespace-only, or over-length name is a validation error. +// The limit is 100 UTF-8 bytes, matching create/update validation. +func resolveTransferName(req *CopySecretRequest, sourceName string) (string, error) { + if req.nameNull { + return "", fmt.Errorf("%w: name must not be null", ErrSecretValidation) + } + if req.Name == nil { + return sourceName, nil + } + name := strings.TrimSpace(*req.Name) + if name == "" { + return "", fmt.Errorf("%w: name must not be empty", ErrSecretValidation) + } + if len(name) > 100 { + return "", fmt.Errorf("%w: name must be 1-100 bytes", ErrSecretValidation) + } + return name, nil +} + +// destinationVerifier returns the in-transaction callback the store invokes +// while holding the destination lock: workspace existence (unconditional, +// fail-closed when unconfigured) then workspace authorization. Global targets +// need no verification. +func (s *Service) destinationVerifier(ctx context.Context, req *CopySecretRequest) func(context.Context) error { + if normalizeStoredScope(req.Scope) != ScopeWorkspace { + return nil + } + // The callback ignores the store-supplied context and uses the outer + // request ctx: the auth callbacks run inside the store's transaction, + // whose context is not the one the request handlers set up. + return func(_ context.Context) error { + if s.workspaceExistence == nil { + return ErrWorkspaceAccessDenied + } + if err := s.workspaceExistence(ctx, req.WorkspaceID); err != nil { + return err + } + return s.authorizeScope(ctx, ScopeWorkspace, req.WorkspaceID) + } +} diff --git a/apps/backend/internal/secrets/sqlite_store.go b/apps/backend/internal/secrets/sqlite_store.go index 2c92ccb28e..172116840e 100644 --- a/apps/backend/internal/secrets/sqlite_store.go +++ b/apps/backend/internal/secrets/sqlite_store.go @@ -3,7 +3,9 @@ package secrets import ( "context" "database/sql" + "errors" "fmt" + "sort" "strings" "time" @@ -20,6 +22,22 @@ type sqliteStore struct { ro *sqlx.DB // reader crypto *MasterKeyProvider ownsDB bool + + // failAfterInsert is a test-only hook. When set, it is invoked after the + // transfer inserts the target row and before the Move source delete runs, + // proving Move is transactional (the inserted copy must roll back). + failAfterInsert func() error + + // afterSourceLock is a test-only hook invoked right after the source row + // is selected with FOR UPDATE (Move on PostgreSQL), giving tests a + // deterministic point where the source row lock is known to be held. + afterSourceLock func() error + + // beforeTransferInsert is a test-only hook invoked after the conflict + // check and before the conditional insert, letting tests commit a + // competing row at the exact point where the atomic NOT EXISTS guard must + // observe it (PostgreSQL: ordinary creates do not take the transfer lock). + beforeTransferInsert func() error } var _ ScopedSecretStore = (*sqliteStore)(nil) @@ -33,6 +51,7 @@ func Provide(writer, reader *sqlx.DB, crypto *MasterKeyProvider) (*sqliteStore, return store, store.Close, nil } +// initSchema creates the secrets table if needed and applies idempotent column migrations. func (s *sqliteStore) initSchema() error { binaryType := dialect.BlobType(s.db.DriverName()) schema := fmt.Sprintf(` @@ -81,6 +100,7 @@ func (s *sqliteStore) ClaimUnowned(ctx context.Context, ownerID string) error { return err } +// Close closes the underlying writer connection when the store owns it. func (s *sqliteStore) Close() error { if !s.ownsDB { return nil @@ -88,6 +108,7 @@ func (s *sqliteStore) Close() error { return s.db.Close() } +// Create validates the scope, encrypts the value, and inserts a new secret. func (s *sqliteStore) Create(ctx context.Context, secret *SecretWithValue) error { if secret == nil { return fmt.Errorf("secret is required") @@ -121,6 +142,7 @@ func (s *sqliteStore) Create(ctx context.Context, secret *SecretWithValue) error return nil } +// Get returns a secret's metadata by id, scoped to the caller; ErrNotFound if absent. func (s *sqliteStore) Get(ctx context.Context, id string) (*Secret, error) { var row secretRow err := s.ro.GetContext(ctx, &row, s.ro.Rebind(` @@ -136,6 +158,7 @@ func (s *sqliteStore) Get(ctx context.Context, id string) (*Secret, error) { return row.toSecret(), nil } +// Reveal returns the decrypted plaintext value of the secret with the given id. func (s *sqliteStore) Reveal(ctx context.Context, id string) (string, error) { var ciphertext, nonce []byte err := s.ro.QueryRowContext(ctx, s.ro.Rebind(` @@ -157,6 +180,7 @@ func (s *sqliteStore) Reveal(ctx context.Context, id string) (string, error) { return string(plaintext), nil } +// Update applies the requested name and/or value changes to the secret with the given id. func (s *sqliteStore) Update(ctx context.Context, id string, req *UpdateSecretRequest) error { existing, err := s.Get(ctx, id) if err != nil { @@ -195,6 +219,7 @@ func (s *sqliteStore) Update(ctx context.Context, id string, req *UpdateSecretRe return nil } +// Delete removes the secret with the given id, scoped to the caller; ErrNotFound if absent. func (s *sqliteStore) Delete(ctx context.Context, id string) error { result, err := s.db.ExecContext(ctx, s.db.Rebind(` DELETE FROM secrets WHERE id = ? AND (user_id = '' OR ? = '' OR user_id = ?)`), @@ -209,10 +234,12 @@ func (s *sqliteStore) Delete(ctx context.Context, id string) error { return nil } +// List returns all global secrets visible to the caller. func (s *sqliteStore) List(ctx context.Context) ([]*SecretListItem, error) { return s.ListScoped(ctx, SecretListOptions{Scope: ScopeGlobal}) } +// ListScoped returns secrets matching the given scope options, newest first. func (s *sqliteStore) ListScoped(ctx context.Context, opts SecretListOptions) ([]*SecretListItem, error) { if err := validateListOptions(opts); err != nil { return nil, err @@ -243,6 +270,7 @@ func (s *sqliteStore) ListScoped(ctx context.Context, opts SecretListOptions) ([ return toSecretListItems(rows), nil } +// GetForWorkspace returns the secret with id if it is global or belongs to the given workspace. func (s *sqliteStore) GetForWorkspace(ctx context.Context, id, workspaceID string) (*Secret, error) { if strings.TrimSpace(workspaceID) == "" { return nil, fmt.Errorf("workspace id is required") @@ -257,6 +285,7 @@ func (s *sqliteStore) GetForWorkspace(ctx context.Context, id, workspaceID strin return secret, nil } +// RevealGlobal returns the decrypted value of the global secret with the given id. func (s *sqliteStore) RevealGlobal(ctx context.Context, id string) (string, error) { secret, err := s.Get(ctx, id) if err != nil { @@ -268,6 +297,7 @@ func (s *sqliteStore) RevealGlobal(ctx context.Context, id string) (string, erro return s.Reveal(ctx, id) } +// RevealForWorkspace returns the decrypted value of a secret visible in the given workspace. func (s *sqliteStore) RevealForWorkspace(ctx context.Context, id, workspaceID string) (string, error) { if _, err := s.GetForWorkspace(ctx, id, workspaceID); err != nil { return "", err @@ -275,22 +305,43 @@ func (s *sqliteStore) RevealForWorkspace(ctx context.Context, id, workspaceID st return s.Reveal(ctx, id) } +// DeleteWorkspaceSecrets removes all workspace-owned secrets for the given workspace in a transaction. func (s *sqliteStore) DeleteWorkspaceSecrets(ctx context.Context, workspaceID string) error { if strings.TrimSpace(workspaceID) == "" { return fmt.Errorf("workspace id is required") } - return s.deleteWorkspaceSecrets(ctx, s.db, workspaceID) + // Run under a transaction so the PostgreSQL advisory lock is held to + // commit, keeping concurrent secret transfers from inserting into a + // workspace that is being deleted. + tx, err := s.db.BeginTxx(ctx, nil) + if err != nil { + return fmt.Errorf("begin workspace secret cleanup: %w", err) + } + defer func() { _ = tx.Rollback() }() + if err := s.DeleteWorkspaceSecretsTx(ctx, tx, workspaceID); err != nil { + return err + } + return tx.Commit() } // DeleteWorkspaceSecretsTx removes workspace-owned secrets on the supplied // transaction so workspace and secret deletion can commit or roll back as one. +// On PostgreSQL it first acquires the workspace's advisory lock (shared with +// secret transfers), guaranteeing a concurrent transfer cannot insert after +// this cleanup has run. func (s *sqliteStore) DeleteWorkspaceSecretsTx(ctx context.Context, tx *sqlx.Tx, workspaceID string) error { if tx == nil { return fmt.Errorf("transaction is required") } + if dialect.IsPostgres(s.db.DriverName()) { + if _, err := tx.ExecContext(ctx, s.db.Rebind("SELECT pg_advisory_xact_lock(?)"), WorkspaceLockKey(workspaceID)); err != nil { + return fmt.Errorf("acquire workspace lock: %w", err) + } + } return s.deleteWorkspaceSecrets(ctx, tx, workspaceID) } +// deleteWorkspaceSecrets runs the workspace-owned secret deletion on the supplied executor. func (s *sqliteStore) deleteWorkspaceSecrets(ctx context.Context, exec sqlx.ExtContext, workspaceID string) error { if strings.TrimSpace(workspaceID) == "" { return fmt.Errorf("workspace id is required") @@ -303,6 +354,7 @@ func (s *sqliteStore) deleteWorkspaceSecrets(ctx context.Context, exec sqlx.ExtC return nil } +// validateSecretScope rejects scope/workspace combinations that are not allowed. func validateSecretScope(scope SecretScope, workspaceID string) error { switch scope { case ScopeGlobal: @@ -319,6 +371,7 @@ func validateSecretScope(scope SecretScope, workspaceID string) error { return nil } +// validateListOptions rejects list options with an invalid scope or mismatched workspace. func validateListOptions(opts SecretListOptions) error { switch opts.Scope { case ScopeGlobal: @@ -345,6 +398,7 @@ type secretRow struct { UpdatedAt time.Time `db:"updated_at"` } +// toSecret converts the scanned row into a Secret with its stored scope normalized. func (r *secretRow) toSecret() *Secret { return &Secret{ ID: r.ID, @@ -367,6 +421,7 @@ type secretListRow struct { UpdatedAt time.Time `db:"updated_at"` } +// toSecretListItems converts scanned list rows into SecretListItems with normalized scopes. func toSecretListItems(rows []secretListRow) []*SecretListItem { items := make([]*SecretListItem, len(rows)) for i, r := range rows { @@ -383,9 +438,295 @@ func toSecretListItems(rows []secretListRow) []*SecretListItem { return items } +// normalizeStoredScope maps a legacy empty stored scope to Global. func normalizeStoredScope(scope SecretScope) SecretScope { if scope == "" { return ScopeGlobal } return scope } + +// transferExec is the minimal transaction-bound executor used by +// transferBody. Both *sql.Conn (SQLite BEGIN IMMEDIATE path) and *sqlx.Tx +// (PostgreSQL advisory-lock path) satisfy it, so every transfer statement +// runs on the same transaction and never on the store's reader handle. +type transferExec interface { + ExecContext(ctx context.Context, query string, args ...any) (sql.Result, error) + QueryRowContext(ctx context.Context, query string, args ...any) *sql.Row +} + +// CopyScoped copies a secret into the target scope under a new name and returns the copy. +func (s *sqliteStore) CopyScoped(ctx context.Context, sourceID, sourceWorkspaceID string, targetScope SecretScope, targetWorkspaceID string, requestedName *string, verifyDestination func(context.Context) error) (*Secret, error) { + return s.transferScoped(ctx, sourceID, sourceWorkspaceID, targetScope, targetWorkspaceID, requestedName, verifyDestination, false) +} + +// MoveScoped moves a secret into the target scope under a new name, deleting the source. +func (s *sqliteStore) MoveScoped(ctx context.Context, sourceID, sourceWorkspaceID string, targetScope SecretScope, targetWorkspaceID string, requestedName *string, verifyDestination func(context.Context) error) (*Secret, error) { + return s.transferScoped(ctx, sourceID, sourceWorkspaceID, targetScope, targetWorkspaceID, requestedName, verifyDestination, true) +} + +// transferScoped runs the atomic copy/move transfer: conflict check, insert, and optional source delete inside one transaction. +func (s *sqliteStore) transferScoped(ctx context.Context, sourceID, sourceWorkspaceID string, targetScope SecretScope, targetWorkspaceID string, requestedName *string, verifyDestination func(context.Context) error, deleteSource bool) (*Secret, error) { + if err := validateSecretScope(targetScope, targetWorkspaceID); err != nil { + return nil, fmt.Errorf("scope validation: %w", err) + } + if requestedName != nil && strings.TrimSpace(*requestedName) == "" { + return nil, fmt.Errorf("target name is required") + } + if dialect.IsPostgres(s.db.DriverName()) { + return s.transferPostgres(ctx, sourceID, sourceWorkspaceID, targetScope, targetWorkspaceID, requestedName, verifyDestination, deleteSource) + } + return s.transferSQLite(ctx, sourceID, targetScope, targetWorkspaceID, requestedName, verifyDestination, deleteSource) +} + +// transferSQLite runs the transfer on the single writer connection with +// BEGIN IMMEDIATE, taking the SQLite write lock up front so competing +// transfers serialize before any read. +func (s *sqliteStore) transferSQLite(ctx context.Context, sourceID string, targetScope SecretScope, targetWorkspaceID string, requestedName *string, verifyDestination func(context.Context) error, deleteSource bool) (*Secret, error) { + conn, err := s.db.Conn(ctx) + if err != nil { + return nil, fmt.Errorf("acquire writer connection: %w", err) + } + defer func() { _ = conn.Close() }() + + if _, err := conn.ExecContext(ctx, "BEGIN IMMEDIATE"); err != nil { + return nil, fmt.Errorf("begin immediate: %w", err) + } + committed := false + defer func() { + if !committed { + // The rollback must reach SQLite even when the caller canceled ctx: + // a canceled ROLLBACK would leave the BEGIN IMMEDIATE transaction + // and its write lock open on the pooled connection. + _, _ = conn.ExecContext(context.WithoutCancel(ctx), "ROLLBACK") + } + }() + + // SQLite's single writer serializes all writers (BEGIN IMMEDIATE), so the + // source row cannot change under us; no row lock is needed. + secret, err := s.transferBody(ctx, conn, func(q string) string { return q }, sourceID, targetScope, targetWorkspaceID, requestedName, verifyDestination, deleteSource, false) + if err != nil { + return nil, err + } + if _, err := conn.ExecContext(ctx, "COMMIT"); err != nil { + return nil, fmt.Errorf("commit transfer: %w", err) + } + committed = true + return secret, nil +} + +// transferPostgres runs the transfer in a transaction that acquires the +// destination's advisory lock (plus the source workspace's lock for Move) in +// sorted order before any read, serializing competing transfers and +// coordinating with workspace deletion under READ COMMITTED. +func (s *sqliteStore) transferPostgres(ctx context.Context, sourceID, sourceWorkspaceID string, targetScope SecretScope, targetWorkspaceID string, requestedName *string, verifyDestination func(context.Context) error, deleteSource bool) (*Secret, error) { + tx, err := s.db.BeginTxx(ctx, nil) + if err != nil { + return nil, fmt.Errorf("begin transfer: %w", err) + } + defer func() { _ = tx.Rollback() }() + + keys := []int64{transferTargetLockKey(targetScope, targetWorkspaceID)} + if deleteSource && strings.TrimSpace(sourceWorkspaceID) != "" { + keys = append(keys, WorkspaceLockKey(sourceWorkspaceID)) + } + sort.Slice(keys, func(i, j int) bool { return keys[i] < keys[j] }) + var prev int64 + first := true + for _, key := range keys { + if !first && key == prev { + continue + } + if _, err := tx.ExecContext(ctx, s.db.Rebind("SELECT pg_advisory_xact_lock(?)"), key); err != nil { + return nil, fmt.Errorf("acquire transfer lock: %w", err) + } + prev = key + first = false + } + + // A Move destroys the source, so the source row is locked with FOR UPDATE + // (deleteSource == true): a concurrent Update cannot commit a newer value + // between our read and delete, which would otherwise lose that value. + secret, err := s.transferBody(ctx, tx, s.db.Rebind, sourceID, targetScope, targetWorkspaceID, requestedName, verifyDestination, deleteSource, deleteSource) + if err != nil { + return nil, err + } + if err := tx.Commit(); err != nil { + return nil, fmt.Errorf("commit transfer: %w", err) + } + return secret, nil +} + +// transferTargetLockKey returns the advisory-lock key for a transfer target: +// the workspace key for Workspace targets, the Global constant otherwise. +func transferTargetLockKey(targetScope SecretScope, targetWorkspaceID string) int64 { + if normalizeStoredScope(targetScope) != ScopeWorkspace { + return GlobalSecretTransferLockKey + } + return WorkspaceLockKey(targetWorkspaceID) +} + +// transferBody executes the shared transfer steps on the transaction-bound +// executor: destination verification (while the lock is held), source select, +// normalized-scope conflict check, insert of the copy (reusing the source's +// encrypted value), optional source delete, and the returned-row read. Every +// statement uses the same executor and the per-user visibility predicate. +func (s *sqliteStore) transferBody(ctx context.Context, exec transferExec, rebind func(string) string, sourceID string, targetScope SecretScope, targetWorkspaceID string, requestedName *string, verifyDestination func(context.Context) error, deleteSource, lockSource bool) (*Secret, error) { + owner := scopeOwner(ctx) + + if verifyDestination != nil { + if err := verifyDestination(ctx); err != nil { + return nil, err + } + } + + src, err := s.transferSource(ctx, exec, rebind, sourceID, owner, lockSource) + if err != nil { + return nil, err + } + // An omitted name resolves from the transactional source row: a concurrent + // rename can never split the copied name from its value (or, for Move, + // delete the renamed row and recreate it under a stale name). + targetName := "" + if requestedName == nil { + targetName = src.Name + } else { + targetName = *requestedName + } + + if err := s.transferConflictCheck(ctx, exec, rebind, targetScope, targetWorkspaceID, targetName, owner); err != nil { + return nil, err + } + + now := time.Now().UTC() + storedScope := normalizeStoredScope(targetScope) + copyID := uuid.New().String() + if s.beforeTransferInsert != nil { + if err := s.beforeTransferInsert(); err != nil { + return nil, err + } + } + // The insert is guarded by the same conflict predicate atomically: on + // PostgreSQL, an ordinary create or rename does not take the transfer's + // advisory lock, so the eager check alone could race a late commit of the + // same name. NOT EXISTS turns that race into a clean conflict instead of a + // duplicate row. + predicate, predicateArgs := transferConflictPredicate(targetScope, targetWorkspaceID, targetName, owner) + result, err := exec.ExecContext(ctx, rebind(` + INSERT INTO secrets (id, name, user_id, scope, workspace_id, encrypted_value, nonce, created_at, updated_at) + SELECT ?, ?, ?, ?, ?, ?, ?, ?, ? + WHERE NOT EXISTS (SELECT 1 FROM secrets WHERE `+predicate+`)`), + append([]any{copyID, targetName, owner, storedScope, targetWorkspaceID, src.Ciphertext, src.Nonce, now, now}, predicateArgs...)...) + if err != nil { + return nil, fmt.Errorf("insert transferred secret: %w", err) + } + rows, _ := result.RowsAffected() + if rows == 0 { + return nil, ErrSecretNameConflict + } + + if deleteSource { + if err := s.deleteTransferredSource(ctx, exec, rebind, sourceID, owner); err != nil { + return nil, err + } + } + + var meta secretRow + if err := exec.QueryRowContext(ctx, rebind(` + SELECT id, name, scope, workspace_id, created_at, updated_at FROM secrets WHERE id = ?`), + copyID).Scan(&meta.ID, &meta.Name, &meta.Scope, &meta.WorkspaceID, &meta.CreatedAt, &meta.UpdatedAt); err != nil { + return nil, fmt.Errorf("read transferred secret: %w", err) + } + return meta.toSecret(), nil +} + +// deleteTransferredSource removes the source row using the same per-user +// visibility predicate as the source select. A missing row means the source +// vanished concurrently and the whole transfer rolls back. +func (s *sqliteStore) deleteTransferredSource(ctx context.Context, exec transferExec, rebind func(string) string, sourceID, owner string) error { + if s.failAfterInsert != nil { + if err := s.failAfterInsert(); err != nil { + return err + } + } + result, err := exec.ExecContext(ctx, rebind(` + DELETE FROM secrets WHERE id = ? AND (user_id = '' OR ? = '' OR user_id = ?)`), + sourceID, owner, owner) + if err != nil { + return fmt.Errorf("delete transferred source: %w", err) + } + rows, _ := result.RowsAffected() + if rows == 0 { + return fmt.Errorf("%w: %s", ErrNotFound, sourceID) + } + return nil +} + +// transferSourceRow is the decrypted-source scan target for transferBody. +type transferSourceRow struct { + ID string + Name string + Scope SecretScope + WorkspaceID string + Ciphertext []byte + Nonce []byte +} + +// transferSource selects the source row with the per-user visibility +// predicate. On PostgreSQL Moves (lockSource) it appends FOR UPDATE so a +// concurrent Update cannot interleave between the read and the delete; the +// afterSourceLock test hook fires once the row lock is known to be held. +func (s *sqliteStore) transferSource(ctx context.Context, exec transferExec, rebind func(string) string, sourceID, owner string, lockSource bool) (*transferSourceRow, error) { + query := ` + SELECT id, name, scope, workspace_id, encrypted_value, nonce + FROM secrets WHERE id = ? AND (user_id = '' OR ? = '' OR user_id = ?)` + if lockSource { + query += " FOR UPDATE" + } + var src transferSourceRow + err := exec.QueryRowContext(ctx, rebind(query), sourceID, owner, owner). + Scan(&src.ID, &src.Name, &src.Scope, &src.WorkspaceID, &src.Ciphertext, &src.Nonce) + if err != nil { + if errors.Is(err, sql.ErrNoRows) { + return nil, fmt.Errorf("%w: %s", ErrNotFound, sourceID) + } + return nil, fmt.Errorf("transfer source lookup: %w", err) + } + if lockSource && s.afterSourceLock != nil { + if err := s.afterSourceLock(); err != nil { + return nil, err + } + } + return &src, nil +} + +// transferConflictPredicate returns the WHERE predicate and arguments that +// identify an existing secret with the given name in the destination scope, +// comparing the NORMALIZED scope so a legacy empty stored scope (which reads +// as Global) can never be bypassed. Shared by the eager conflict check and the +// atomic NOT EXISTS guard on the transfer insert. +func transferConflictPredicate(targetScope SecretScope, targetWorkspaceID, targetName, owner string) (string, []any) { + if normalizeStoredScope(targetScope) != ScopeWorkspace { + return `name = ? AND (scope = ? OR scope = '') AND (user_id = '' OR ? = '' OR user_id = ?)`, + []any{targetName, ScopeGlobal, owner, owner} + } + return `name = ? AND scope = ? AND workspace_id = ? AND (user_id = '' OR ? = '' OR user_id = ?)`, + []any{targetName, ScopeWorkspace, targetWorkspaceID, owner, owner} +} + +// transferConflictCheck rejects a target name that already exists in the +// destination scope. It is the eager, pre-insert check; the insert itself is +// guarded by the same predicate atomically (see transferBody), so a competing +// create or rename that commits after this check can still never produce a +// duplicate name from the transfer. +func (s *sqliteStore) transferConflictCheck(ctx context.Context, exec transferExec, rebind func(string) string, targetScope SecretScope, targetWorkspaceID, targetName, owner string) error { + predicate, args := transferConflictPredicate(targetScope, targetWorkspaceID, targetName, owner) + query := `SELECT 1 FROM secrets WHERE ` + predicate + ` LIMIT 1` + var found int + if err := exec.QueryRowContext(ctx, rebind(query), args...).Scan(&found); err == nil { + return ErrSecretNameConflict + } else if !errors.Is(err, sql.ErrNoRows) { + return fmt.Errorf("transfer conflict check: %w", err) + } + return nil +} diff --git a/apps/backend/internal/secrets/store.go b/apps/backend/internal/secrets/store.go index a8b087331a..ca189409fa 100644 --- a/apps/backend/internal/secrets/store.go +++ b/apps/backend/internal/secrets/store.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "hash/fnv" "github.com/jmoiron/sqlx" ) @@ -14,6 +15,40 @@ import ( // rather than string-matching the error text. var ErrNotFound = errors.New("secret not found") +// ErrSecretNameConflict is returned when a copy/move target name already +// exists in the destination scope. Handlers map it to 409 Conflict. +var ErrSecretNameConflict = errors.New("a secret with this name already exists in the target scope") + +// ErrWorkspaceAccessDenied is returned for missing or unauthorized +// destination workspaces (and unconfigured workspace verification). Handlers +// map it to 404 Not Found so a nonexistent workspace is indistinguishable +// from an inaccessible one. +var ErrWorkspaceAccessDenied = errors.New("workspace access denied or workspace not found") + +// ErrSecretValidation marks validation failures on copy/move requests. +// Handlers map it to 400 Bad Request. +var ErrSecretValidation = errors.New("secret validation failed") + +// GlobalSecretTransferLockKey is the advisory-lock key used for transfers +// whose target scope is Global. It is a distinct stable constant, never +// derived from a workspace ID, so concurrent same-name Global transfers +// serialize and exactly one wins. +const GlobalSecretTransferLockKey int64 = 0x4B53544600000001 // "KSTF" + +const transferLockPrefix = "kandev-secret-transfer:" + +// WorkspaceLockKey returns the deterministic advisory-lock key for a +// workspace. The FNV-1a 64-bit hash is stable across restarts and +// collision-safe for UUID inputs (a theoretical collision only over-serializes +// transfers, never corrupts data). PostgreSQL uses these keys to coordinate +// secret transfers with workspace deletion; SQLite's single writer needs no +// advisory locks. +func WorkspaceLockKey(workspaceID string) int64 { + h := fnv.New64a() + _, _ = h.Write([]byte(transferLockPrefix + workspaceID)) + return int64(h.Sum64()) +} + // SecretStore abstracts secret storage. Implementations handle // encryption/decryption internally. type SecretStore interface { @@ -68,6 +103,28 @@ type ScopedSecretStore interface { DeleteWorkspaceSecrets(ctx context.Context, 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 +// lifecycle stores (and their test doubles) are not forced to implement +// transfer behavior. +type SecretTransferStore interface { + // CopyScoped atomically copies a secret into another scope/workspace and + // returns the new row's metadata. requestedName nil keeps the source + // name, resolved from the source row read inside the transaction (so a + // concurrent rename can never split the copied name from its value). + // verifyDestination runs inside the transaction while the destination + // lock is held, before the conflict check and insert; an error rolls back + // and surfaces. sourceWorkspaceID is the resolved source's workspace + // ("" for Global) and is locked alongside the destination for Move so + // concurrent same-source moves serialize without deadlock. + CopyScoped(ctx context.Context, sourceID, sourceWorkspaceID string, targetScope SecretScope, targetWorkspaceID string, requestedName *string, verifyDestination func(context.Context) error) (*Secret, error) + // MoveScoped atomically copies a secret into another scope/workspace and + // deletes the source in the same transaction; the name contract matches + // CopyScoped. + MoveScoped(ctx context.Context, sourceID, sourceWorkspaceID string, targetScope SecretScope, targetWorkspaceID string, requestedName *string, verifyDestination func(context.Context) error) (*Secret, error) +} + // WorkspaceSecretTransactionalDeleter is implemented by stores that can // remove workspace-owned secrets on a caller-owned SQL transaction. type WorkspaceSecretTransactionalDeleter interface { diff --git a/apps/backend/internal/secrets/transfer_test.go b/apps/backend/internal/secrets/transfer_test.go new file mode 100644 index 0000000000..daa8cc7eaf --- /dev/null +++ b/apps/backend/internal/secrets/transfer_test.go @@ -0,0 +1,450 @@ +package secrets + +import ( + "context" + "errors" + "path/filepath" + "testing" + "time" + + "github.com/jmoiron/sqlx" + + "github.com/kandev/kandev/internal/auth/authn" + "github.com/kandev/kandev/internal/db" +) + +// testTransferContext returns a context carrying the given user identity, or a bare background context when userID is empty. +func testTransferContext(userID string) context.Context { + if userID == "" { + return context.Background() + } + return authn.WithIdentity(context.Background(), authn.Identity{UserID: userID}) +} + +// mustCreate seeds a secret via the store, failing the test on error. +func mustCreate(t *testing.T, store SecretStore, secret *SecretWithValue) { + t.Helper() + if err := store.Create(context.Background(), secret); err != nil { + t.Fatalf("seed secret: %v", err) + } +} + +// strPtr returns a pointer to s for presence-aware transfer arguments. +func strPtr(s string) *string { return &s } + +// TestTransferStore_CopyGlobalToWorkspace verifies a global-to-workspace copy keeps the source, sets timestamps, and carries the value server-side. +func TestTransferStore_CopyGlobalToWorkspace(t *testing.T) { + store := newTestSQLiteStore(t) + ctx := context.Background() + mustCreate(t, store, &SecretWithValue{Secret: Secret{ID: "g1", Name: "orig"}, Value: "v1"}) + + got, err := store.CopyScoped(ctx, "g1", "", ScopeWorkspace, "workspace-a", strPtr("copied"), nil) + if err != nil { + t.Fatalf("CopyScoped: %v", err) + } + if got.Scope != ScopeWorkspace || got.WorkspaceID != "workspace-a" || got.Name != "copied" { + t.Fatalf("copy metadata = %+v", got) + } + if got.CreatedAt.IsZero() || got.UpdatedAt.IsZero() { + t.Fatalf("copy timestamps are zero: %+v", got) + } + // Source intact. + if _, err := store.Get(ctx, "g1"); err != nil { + t.Fatalf("source missing after copy: %v", err) + } + // Value carried server-side, verifiable only through reveal. + value, err := store.RevealForWorkspace(ctx, got.ID, "workspace-a") + if err != nil { + t.Fatalf("reveal copy: %v", err) + } + if value != "v1" { + t.Fatalf("copy value = %q, want v1", value) + } +} + +// TestTransferStore_CopyWorkspaceToGlobal verifies a workspace-to-global copy lands in the global scope with the value intact and the source kept. +func TestTransferStore_CopyWorkspaceToGlobal(t *testing.T) { + store := newTestSQLiteStore(t) + ctx := context.Background() + mustCreate(t, store, &SecretWithValue{Secret: Secret{ID: "w1", Name: "ws", Scope: ScopeWorkspace, WorkspaceID: "workspace-a"}, Value: "v2"}) + + got, err := store.CopyScoped(ctx, "w1", "workspace-a", ScopeGlobal, "", strPtr("global-copy"), nil) + if err != nil { + t.Fatalf("CopyScoped: %v", err) + } + if got.Scope != ScopeGlobal || got.WorkspaceID != "" || got.Name != "global-copy" { + t.Fatalf("copy metadata = %+v", got) + } + value, err := store.RevealGlobal(ctx, got.ID) + if err != nil { + t.Fatalf("reveal global copy: %v", err) + } + if value != "v2" { + t.Fatalf("copy value = %q, want v2", value) + } + if _, err := store.Get(ctx, "w1"); err != nil { + t.Fatalf("source missing after copy: %v", err) + } +} + +// TestTransferStore_CopyWorkspaceToWorkspace verifies a workspace-to-workspace copy lands in the target workspace with the value intact. +func TestTransferStore_CopyWorkspaceToWorkspace(t *testing.T) { + store := newTestSQLiteStore(t) + ctx := context.Background() + mustCreate(t, store, &SecretWithValue{Secret: Secret{ID: "w1", Name: "ws", Scope: ScopeWorkspace, WorkspaceID: "workspace-a"}, Value: "v3"}) + + got, err := store.CopyScoped(ctx, "w1", "workspace-a", ScopeWorkspace, "workspace-b", strPtr("moved"), nil) + if err != nil { + t.Fatalf("CopyScoped: %v", err) + } + if got.WorkspaceID != "workspace-b" { + t.Fatalf("copy workspace = %q, want workspace-b", got.WorkspaceID) + } + value, err := store.RevealForWorkspace(ctx, got.ID, "workspace-b") + if err != nil { + t.Fatalf("reveal copy: %v", err) + } + if value != "v3" { + t.Fatalf("copy value = %q, want v3", value) + } +} + +// TestTransferStore_MoveWorkspaceToGlobalRemovesSource verifies moving a workspace secret to global removes the source and preserves the value. +func TestTransferStore_MoveWorkspaceToGlobalRemovesSource(t *testing.T) { + store := newTestSQLiteStore(t) + ctx := context.Background() + mustCreate(t, store, &SecretWithValue{Secret: Secret{ID: "w1", Name: "ws", Scope: ScopeWorkspace, WorkspaceID: "workspace-a"}, Value: "v4"}) + + got, err := store.MoveScoped(ctx, "w1", "workspace-a", ScopeGlobal, "", strPtr("global-copy"), nil) + if err != nil { + t.Fatalf("MoveScoped: %v", err) + } + if got.Scope != ScopeGlobal { + t.Fatalf("moved metadata scope = %q", got.Scope) + } + if _, err := store.Get(ctx, "w1"); !errors.Is(err, ErrNotFound) { + t.Fatalf("source still present after move: %v", err) + } + value, err := store.RevealGlobal(ctx, got.ID) + if err != nil { + t.Fatalf("reveal moved copy: %v", err) + } + if value != "v4" { + t.Fatalf("moved value = %q, want v4", value) + } +} + +// TestTransferStore_MoveGlobalToWorkspaceRemovesSource verifies moving a global secret into a workspace removes the source. +func TestTransferStore_MoveGlobalToWorkspaceRemovesSource(t *testing.T) { + store := newTestSQLiteStore(t) + ctx := context.Background() + mustCreate(t, store, &SecretWithValue{Secret: Secret{ID: "g1", Name: "orig"}, Value: "v5"}) + + got, err := store.MoveScoped(ctx, "g1", "", ScopeWorkspace, "workspace-a", strPtr("copied"), nil) + if err != nil { + t.Fatalf("MoveScoped: %v", err) + } + if got.WorkspaceID != "workspace-a" { + t.Fatalf("moved workspace = %q", got.WorkspaceID) + } + if _, err := store.Get(ctx, "g1"); !errors.Is(err, ErrNotFound) { + t.Fatalf("source still present after move: %v", err) + } +} + +// TestTransferStore_ConflictGlobal verifies a Global target name collision returns ErrSecretNameConflict. +func TestTransferStore_ConflictGlobal(t *testing.T) { + store := newTestSQLiteStore(t) + ctx := context.Background() + mustCreate(t, store, &SecretWithValue{Secret: Secret{ID: "g1", Name: "orig"}, Value: "v"}) + mustCreate(t, store, &SecretWithValue{Secret: Secret{ID: "g2", Name: "taken"}, Value: "other"}) + + _, err := store.CopyScoped(ctx, "g1", "", ScopeGlobal, "", strPtr("taken"), nil) + if !errors.Is(err, ErrSecretNameConflict) { + t.Fatalf("error = %v, want ErrSecretNameConflict", err) + } +} + +// TestTransferStore_ConflictWorkspace verifies a workspace target name collision returns ErrSecretNameConflict. +func TestTransferStore_ConflictWorkspace(t *testing.T) { + store := newTestSQLiteStore(t) + ctx := context.Background() + mustCreate(t, store, &SecretWithValue{Secret: Secret{ID: "g1", Name: "orig"}, Value: "v"}) + mustCreate(t, store, &SecretWithValue{Secret: Secret{ID: "w1", Name: "taken", Scope: ScopeWorkspace, WorkspaceID: "workspace-a"}, Value: "other"}) + + _, err := store.CopyScoped(ctx, "g1", "", ScopeWorkspace, "workspace-a", strPtr("taken"), nil) + if !errors.Is(err, ErrSecretNameConflict) { + t.Fatalf("error = %v, want ErrSecretNameConflict", err) + } +} + +// TestTransferStore_ConflictLegacyEmptyScopeGlobal verifies a legacy empty-scope row conflicts with a Global target name. +func TestTransferStore_ConflictLegacyEmptyScopeGlobal(t *testing.T) { + store := newTestSQLiteStore(t) + ctx := context.Background() + mustCreate(t, store, &SecretWithValue{Secret: Secret{ID: "g1", Name: "orig"}, Value: "v"}) + + // Seed a legacy row whose stored scope is empty (pre-scope migration state). + ciphertext, nonce, err := Encrypt([]byte("legacy"), store.crypto.Key()) + if err != nil { + t.Fatalf("encrypt legacy: %v", err) + } + now := time.Now().UTC() + if _, err := store.db.ExecContext(ctx, store.db.Rebind(` + INSERT INTO secrets (id, name, user_id, scope, workspace_id, encrypted_value, nonce, created_at, updated_at) + VALUES (?, ?, '', '', '', ?, ?, ?, ?)`), + "legacy-1", "taken", ciphertext, nonce, now, now); err != nil { + t.Fatalf("seed legacy row: %v", err) + } + + _, err = store.CopyScoped(ctx, "g1", "", ScopeGlobal, "", strPtr("taken"), nil) + if !errors.Is(err, ErrSecretNameConflict) { + t.Fatalf("error = %v, want ErrSecretNameConflict (legacy empty scope is Global)", err) + } +} + +// TestTransferStore_SourceMissing verifies copying an unknown source returns ErrNotFound. +func TestTransferStore_SourceMissing(t *testing.T) { + store := newTestSQLiteStore(t) + _, err := store.CopyScoped(context.Background(), "nope", "", ScopeWorkspace, "workspace-a", strPtr("x"), nil) + if !errors.Is(err, ErrNotFound) { + t.Fatalf("error = %v, want ErrNotFound", err) + } +} + +// TestTransferStore_VerifyDestinationErrorRollsBack verifies a failing verifyDestination callback rolls back the copy, leaving no destination row and the source intact. +func TestTransferStore_VerifyDestinationErrorRollsBack(t *testing.T) { + store := newTestSQLiteStore(t) + ctx := context.Background() + mustCreate(t, store, &SecretWithValue{Secret: Secret{ID: "g1", Name: "orig"}, Value: "v"}) + + denied := errors.New("destination denied") + _, err := store.CopyScoped(ctx, "g1", "", ScopeWorkspace, "workspace-a", strPtr("copied"), func(context.Context) error { + return denied + }) + if !errors.Is(err, denied) { + t.Fatalf("error = %v, want the callback error", err) + } + items, err := store.ListScoped(ctx, SecretListOptions{Scope: ScopeWorkspace, WorkspaceID: "workspace-a"}) + if err != nil { + t.Fatalf("list: %v", err) + } + if len(items) != 0 { + t.Fatalf("destination rows = %d, want 0 after callback rollback", len(items)) + } + if _, err := store.Get(ctx, "g1"); err != nil { + t.Fatalf("source lost after callback rollback: %v", err) + } +} + +// TestTransferStore_MoveFailpointAfterInsertRollsBack verifies Move is transactional: an injected delete failure rolls back the inserted copy. +func TestTransferStore_MoveFailpointAfterInsertRollsBack(t *testing.T) { + store := newTestSQLiteStore(t) + ctx := context.Background() + mustCreate(t, store, &SecretWithValue{Secret: Secret{ID: "g1", Name: "orig"}, Value: "v"}) + + store.failAfterInsert = func() error { return errors.New("injected delete failure") } + _, err := store.MoveScoped(ctx, "g1", "", ScopeWorkspace, "workspace-a", strPtr("copied"), nil) + if err == nil { + t.Fatal("MoveScoped succeeded despite injected failpoint") + } + // Source intact, no destination row: Move is transactional, not copy-then-commit-delete. + if _, err := store.Get(ctx, "g1"); err != nil { + t.Fatalf("source lost after failpoint rollback: %v", err) + } + items, err := store.ListScoped(ctx, SecretListOptions{Scope: ScopeWorkspace, WorkspaceID: "workspace-a"}) + if err != nil { + t.Fatalf("list: %v", err) + } + if len(items) != 0 { + t.Fatalf("destination rows = %d, want 0 after failpoint rollback", len(items)) + } +} + +// TestTransferStore_RollbackSurvivesCanceledContext verifies the deferred +// ROLLBACK reaches SQLite even when the caller cancels the context mid-transfer: +// a canceled rollback would leave the BEGIN IMMEDIATE transaction and its write +// lock open on the pooled connection, breaking every later transfer. +func TestTransferStore_RollbackSurvivesCanceledContext(t *testing.T) { + store := newTestSQLiteStore(t) + // Pin the writer pool to one connection so the follow-up transfer MUST + // reuse the connection that ran the failed transfer: a leaked BEGIN + // IMMEDIATE transaction on it would break the next transfer. + store.db.SetMaxOpenConns(1) + ctx, cancel := context.WithCancel(context.Background()) + mustCreate(t, store, &SecretWithValue{Secret: Secret{ID: "g1", Name: "orig"}, Value: "v"}) + + // Cancel after the destination insert: the source delete then fails with + // context.Canceled, and the deferred rollback must still reach SQLite. + store.failAfterInsert = func() error { + cancel() + return nil + } + _, err := store.MoveScoped(ctx, "g1", "", ScopeWorkspace, "workspace-a", strPtr("copied"), nil) + if !errors.Is(err, context.Canceled) { + t.Fatalf("MoveScoped error = %v, want context.Canceled", err) + } + // The write lock must be released: a second transfer on the same pooled + // connection can BEGIN IMMEDIATE again. If the rollback leaked, this + // fails with "cannot start a transaction within a transaction". + store.failAfterInsert = nil + if _, err := store.MoveScoped(context.Background(), "g1", "", ScopeWorkspace, "workspace-a", strPtr("copied-again"), nil); err != nil { + t.Fatalf("second transfer failed: %v (leaked transaction/write lock?)", err) + } +} + +// TestTransferStore_OmittedNameUsesCurrentSourceName verifies an omitted name +// resolves from the source row read inside the transaction: a rename that +// lands before the transfer is reflected in the copy (and Move removes the +// renamed row), never a stale pre-transaction read. +func TestTransferStore_OmittedNameUsesCurrentSourceName(t *testing.T) { + store := newTestSQLiteStore(t) + ctx := context.Background() + mustCreate(t, store, &SecretWithValue{Secret: Secret{ID: "g1", Name: "orig"}, Value: "v"}) + // Simulate a rename committing between a stale pre-read and the transfer. + if err := store.Update(ctx, "g1", &UpdateSecretRequest{Name: strPtr("renamed")}); err != nil { + t.Fatalf("rename: %v", err) + } + got, err := store.CopyScoped(ctx, "g1", "", ScopeWorkspace, "workspace-a", nil, nil) + if err != nil { + t.Fatalf("CopyScoped: %v", err) + } + if got.Name != "renamed" { + t.Fatalf("copy name = %q, want the current source name %q", got.Name, "renamed") + } + // Move with an omitted name deletes the renamed source and carries its name. + got2, err := store.MoveScoped(ctx, "g1", "", ScopeWorkspace, "workspace-b", nil, nil) + if err != nil { + t.Fatalf("MoveScoped: %v", err) + } + if got2.Name != "renamed" { + t.Fatalf("moved name = %q, want %q", got2.Name, "renamed") + } + if _, err := store.Get(ctx, "g1"); err == nil { + t.Fatal("source still present after move") + } +} + +// TestTransferStore_PerUserScoping verifies secrets are per-user: another user cannot copy or read them, while the owner can move and read the copy. +func TestTransferStore_PerUserScoping(t *testing.T) { + store := newTestSQLiteStore(t) + alice := testTransferContext("user-a") + bob := testTransferContext("user-b") + + // Alice owns a workspace secret; Bob must not see or move it. + mustCreate(t, store, &SecretWithValue{Secret: Secret{ID: "w1", Name: "alice-ws", Scope: ScopeWorkspace, WorkspaceID: "workspace-a"}, Value: "v"}) + // The seed above ran unscoped (background ctx), so claim it for Alice by + // re-creating under her identity: delete the unowned row first. + if err := store.Delete(context.Background(), "w1"); err != nil { + t.Fatalf("cleanup seed: %v", err) + } + if err := store.Create(alice, &SecretWithValue{Secret: Secret{ID: "w1", Name: "alice-ws", Scope: ScopeWorkspace, WorkspaceID: "workspace-a"}, Value: "v"}); err != nil { + t.Fatalf("seed as alice: %v", err) + } + + if _, err := store.CopyScoped(bob, "w1", "workspace-a", ScopeGlobal, "", strPtr("x"), nil); !errors.Is(err, ErrNotFound) { + t.Fatalf("bob copying alice's secret: err = %v, want ErrNotFound", err) + } + + // Alice can move her own secret to Global. + got, err := store.MoveScoped(alice, "w1", "workspace-a", ScopeGlobal, "", strPtr("alice-global"), nil) + if err != nil { + t.Fatalf("alice move: %v", err) + } + // The moved copy is owned by Alice and invisible to Bob. + if _, err := store.Get(bob, got.ID); !errors.Is(err, ErrNotFound) { + t.Fatalf("bob reading alice's copy: err = %v, want ErrNotFound", err) + } + if _, err := store.Get(alice, got.ID); err != nil { + t.Fatalf("alice reading her copy: %v", err) + } +} + +// TestTransferStore_UserVisibleStoreRejectsInternalSource verifies the user-visible wrapper rejects copying internal backend-owned secrets as ErrNotFound. +func TestTransferStore_UserVisibleStoreRejectsInternalSource(t *testing.T) { + raw := newTestSQLiteStore(t) + // Seed an internal backend-owned row through the RAW store (the wrapper + // refuses to create it). + mustCreate(t, raw, &SecretWithValue{Secret: Secret{ID: "github:user:workspace:user:access", Name: "internal"}, Value: "token"}) + + wrapper, ok := NewUserVisibleStore(raw).(SecretTransferStore) + if !ok { + t.Fatal("wrapper is not a SecretTransferStore") + } + _, err := wrapper.CopyScoped(context.Background(), "github:user:workspace:user:access", "", ScopeGlobal, "", strPtr("x"), nil) + if !errors.Is(err, ErrNotFound) { + t.Fatalf("error = %v, want ErrNotFound for internal source", err) + } +} + +// TestTransferStore_ConcurrentSameTargetSQLite verifies concurrent same-name copies to a Global target serialize on the SQLite write lock with exactly one winner. +func TestTransferStore_ConcurrentSameTargetSQLite(t *testing.T) { + dir := t.TempDir() + dbPath := filepath.Join(dir, "secrets.db") + + conn1, err := db.OpenSQLite(dbPath) + if err != nil { + t.Fatalf("open db 1: %v", err) + } + t.Cleanup(func() { _ = conn1.Close() }) + conn2, err := db.OpenSQLite(dbPath) + if err != nil { + t.Fatalf("open db 2: %v", err) + } + t.Cleanup(func() { _ = conn2.Close() }) + + crypto, err := NewMasterKeyProvider(dir) + if err != nil { + t.Fatalf("master key: %v", err) + } + store1, cleanup1, err := Provide(sqlx.NewDb(conn1, "sqlite3"), sqlx.NewDb(conn1, "sqlite3"), crypto) + if err != nil { + t.Fatalf("provide store 1: %v", err) + } + t.Cleanup(func() { _ = cleanup1() }) + store2, cleanup2, err := Provide(sqlx.NewDb(conn2, "sqlite3"), sqlx.NewDb(conn2, "sqlite3"), crypto) + if err != nil { + t.Fatalf("provide store 2: %v", err) + } + t.Cleanup(func() { _ = cleanup2() }) + + ctx := context.Background() + mustCreate(t, store1, &SecretWithValue{Secret: Secret{ID: "src", Name: "orig"}, Value: "v"}) + + locked := make(chan struct{}) + release := make(chan struct{}) + done := make(chan error, 1) + go func() { + _, err := store1.CopyScoped(ctx, "src", "", ScopeGlobal, "", strPtr("dup"), func(context.Context) error { + close(locked) + <-release + return nil + }) + done <- err + }() + + // A holds the SQLite write lock (BEGIN IMMEDIATE + callback in-flight). + <-locked + + bErr := make(chan error, 1) + go func() { + _, err := store2.CopyScoped(ctx, "src", "", ScopeGlobal, "", strPtr("dup"), nil) + bErr <- err + }() + + select { + case err := <-bErr: + t.Fatalf("B completed while A held the lock: %v", err) + case <-time.After(300 * time.Millisecond): + // B is blocked at the SQLite transaction lock, not at a Go pool. + } + + close(release) + if err := <-done; err != nil { + t.Fatalf("A: %v", err) + } + err = <-bErr + if !errors.Is(err, ErrSecretNameConflict) { + t.Fatalf("B error = %v, want ErrSecretNameConflict", err) + } +} diff --git a/apps/backend/internal/secrets/user_visible_store.go b/apps/backend/internal/secrets/user_visible_store.go index 2c64cdded2..72e533ffde 100644 --- a/apps/backend/internal/secrets/user_visible_store.go +++ b/apps/backend/internal/secrets/user_visible_store.go @@ -20,18 +20,30 @@ func IsInternalID(id string) bool { // underlying store remains available to integration services for their // deterministic internal keys. type UserVisibleStore struct { - store SecretStore - scoped ScopedSecretStore + store SecretStore + // scoped carries both the scope-aware read/lifecycle surface and the + // transfer surface the wrapper exposes to the service. + scoped userVisibleScopedStore } +// userVisibleScopedStore is the store surface UserVisibleStore forwards: +// scope-aware reads/lifecycle plus atomic scope transfers. +type userVisibleScopedStore interface { + ScopedSecretStore + SecretTransferStore +} + +// NewUserVisibleStore wraps a SecretStore, returning a store that exposes +// only user-managed credentials; it returns nil when store is nil. func NewUserVisibleStore(store SecretStore) SecretStore { if store == nil { return nil } - scoped, _ := store.(ScopedSecretStore) + scoped, _ := store.(userVisibleScopedStore) return &UserVisibleStore{store: store, scoped: scoped} } +// Create creates a secret owned by the current user. func (s *UserVisibleStore) Create(ctx context.Context, secret *SecretWithValue) error { if secret != nil && IsInternalID(secret.ID) { return internalSecretNotFound(secret.ID) @@ -39,6 +51,8 @@ func (s *UserVisibleStore) Create(ctx context.Context, secret *SecretWithValue) return s.store.Create(ctx, secret) } +// Get returns the global user-visible secret with the given id, treating +// internal ids and non-global scopes as not found. func (s *UserVisibleStore) Get(ctx context.Context, id string) (*Secret, error) { if IsInternalID(id) { return nil, internalSecretNotFound(id) @@ -53,6 +67,7 @@ func (s *UserVisibleStore) Get(ctx context.Context, id string) (*Secret, error) return secret, nil } +// Reveal returns the plaintext value of the user-visible secret with the given id. func (s *UserVisibleStore) Reveal(ctx context.Context, id string) (string, error) { if IsInternalID(id) { return "", internalSecretNotFound(id) @@ -63,6 +78,7 @@ func (s *UserVisibleStore) Reveal(ctx context.Context, id string) (string, error return s.store.Reveal(ctx, id) } +// Update updates the user-visible secret with the given id. func (s *UserVisibleStore) Update(ctx context.Context, id string, req *UpdateSecretRequest) error { if IsInternalID(id) { return internalSecretNotFound(id) @@ -73,6 +89,7 @@ func (s *UserVisibleStore) Update(ctx context.Context, id string, req *UpdateSec return s.store.Update(ctx, id, req) } +// Delete deletes the user-visible secret with the given id. func (s *UserVisibleStore) Delete(ctx context.Context, id string) error { if IsInternalID(id) { return internalSecretNotFound(id) @@ -83,6 +100,7 @@ func (s *UserVisibleStore) Delete(ctx context.Context, id string) error { 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 { return s.ListScoped(ctx, SecretListOptions{Scope: ScopeGlobal}) @@ -100,6 +118,7 @@ func (s *UserVisibleStore) List(ctx context.Context) ([]*SecretListItem, error) return visible, nil } +// ListScoped returns user-visible secrets matching the given scope options. func (s *UserVisibleStore) ListScoped(ctx context.Context, opts SecretListOptions) ([]*SecretListItem, error) { if opts.Scope == "" { opts.Scope = ScopeGlobal @@ -117,6 +136,7 @@ func (s *UserVisibleStore) ListScoped(ctx context.Context, opts SecretListOption return filterInternalSecretItems(items), nil } +// GetForWorkspace returns the user-visible secret with the given id scoped to the workspace. func (s *UserVisibleStore) GetForWorkspace(ctx context.Context, id, workspaceID string) (*Secret, error) { if IsInternalID(id) { return nil, internalSecretNotFound(id) @@ -127,6 +147,7 @@ func (s *UserVisibleStore) GetForWorkspace(ctx context.Context, id, workspaceID return s.scoped.GetForWorkspace(ctx, id, workspaceID) } +// RevealGlobal returns the plaintext value of the global user-visible secret with the given id. func (s *UserVisibleStore) RevealGlobal(ctx context.Context, id string) (string, error) { if IsInternalID(id) { return "", internalSecretNotFound(id) @@ -137,6 +158,7 @@ func (s *UserVisibleStore) RevealGlobal(ctx context.Context, id string) (string, return s.store.Reveal(ctx, id) } +// RevealForWorkspace returns the plaintext value of the user-visible secret with the given id scoped to the workspace. func (s *UserVisibleStore) RevealForWorkspace(ctx context.Context, id, workspaceID string) (string, error) { if IsInternalID(id) { return "", internalSecretNotFound(id) @@ -150,6 +172,7 @@ func (s *UserVisibleStore) RevealForWorkspace(ctx context.Context, id, workspace return s.store.Reveal(ctx, id) } +// DeleteWorkspaceSecrets deletes all user-visible secrets scoped to the given workspace. func (s *UserVisibleStore) DeleteWorkspaceSecrets(ctx context.Context, workspaceID string) error { if s.scoped == nil { return fmt.Errorf("workspace-scoped secret storage is unavailable") @@ -157,6 +180,29 @@ func (s *UserVisibleStore) DeleteWorkspaceSecrets(ctx context.Context, workspace return s.scoped.DeleteWorkspaceSecrets(ctx, workspaceID) } +// CopyScoped copies the user-visible secret to the target scope after the destination verifier approves. +func (s *UserVisibleStore) CopyScoped(ctx context.Context, sourceID, sourceWorkspaceID string, targetScope SecretScope, targetWorkspaceID string, requestedName *string, verifyDestination func(context.Context) error) (*Secret, error) { + if IsInternalID(sourceID) { + return nil, internalSecretNotFound(sourceID) + } + if s.scoped == nil { + return nil, fmt.Errorf("workspace-scoped secret storage is unavailable") + } + return s.scoped.CopyScoped(ctx, sourceID, sourceWorkspaceID, targetScope, targetWorkspaceID, requestedName, verifyDestination) +} + +// MoveScoped moves the user-visible secret to the target scope after the destination verifier approves. +func (s *UserVisibleStore) MoveScoped(ctx context.Context, sourceID, sourceWorkspaceID string, targetScope SecretScope, targetWorkspaceID string, requestedName *string, verifyDestination func(context.Context) error) (*Secret, error) { + if IsInternalID(sourceID) { + return nil, internalSecretNotFound(sourceID) + } + if s.scoped == nil { + return nil, fmt.Errorf("workspace-scoped secret storage is unavailable") + } + return s.scoped.MoveScoped(ctx, sourceID, sourceWorkspaceID, targetScope, targetWorkspaceID, requestedName, verifyDestination) +} + +// DeleteWorkspaceSecretsTx deletes workspace-scoped user-visible secrets inside the given transaction. func (s *UserVisibleStore) DeleteWorkspaceSecretsTx(ctx context.Context, tx *sqlx.Tx, workspaceID string) error { if s.scoped == nil { return fmt.Errorf("workspace-scoped secret storage is unavailable") @@ -171,10 +217,12 @@ func (s *UserVisibleStore) DeleteWorkspaceSecretsTx(ctx context.Context, tx *sql // The wrapped store is owned and closed by the repository container. func (s *UserVisibleStore) Close() error { return nil } +// internalSecretNotFound returns the store's sentinel not-found error annotated with the given secret id. func internalSecretNotFound(id string) error { return fmt.Errorf("%w: %s", ErrNotFound, id) } +// filterInternalSecretItems returns only the user-managed items, dropping internal ones. func filterInternalSecretItems(items []*SecretListItem) []*SecretListItem { visible := make([]*SecretListItem, 0, len(items)) for _, item := range items { diff --git a/apps/backend/pkg/websocket/actions.go b/apps/backend/pkg/websocket/actions.go index 6a3777a52e..db96aad098 100644 --- a/apps/backend/pkg/websocket/actions.go +++ b/apps/backend/pkg/websocket/actions.go @@ -360,6 +360,8 @@ const ( ActionSecretUpdate = "secrets.update" ActionSecretDelete = "secrets.delete" ActionSecretReveal = "secrets.reveal" + ActionSecretCopy = "secrets.copy" + ActionSecretMove = "secrets.move" // Sprites actions ActionSpritesStatus = "sprites.status" diff --git a/apps/web/components/settings/copy-move-dialog-body.tsx b/apps/web/components/settings/copy-move-dialog-body.tsx new file mode 100644 index 0000000000..0c6c2abaee --- /dev/null +++ b/apps/web/components/settings/copy-move-dialog-body.tsx @@ -0,0 +1,339 @@ +"use client"; + +import { useTranslation } from "react-i18next"; +import { IconX } from "@tabler/icons-react"; +import { Button } from "@kandev/ui/button"; +import { + DialogClose, + DialogDescription, + DialogFooter, + DialogHeader, + DialogTitle, +} from "@kandev/ui/dialog"; +import { Input } from "@kandev/ui/input"; +import { Label } from "@kandev/ui/label"; +import { RadioGroup, RadioGroupItem } from "@kandev/ui/radio-group"; +import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from "@kandev/ui/select"; +import type { CopyMoveMode, SecretDestination } from "./copy-move-secret-dialog"; + +/** Renders the Copy/Move dialog body: mode selector, destination picker, target name, and footer actions. */ +export function CopyMoveDialogBody({ + secretName, + originToken, + mode, + onModeChange, + destination, + destinations, + workspaceNameById, + onDestinationChange, + destinationsLoading, + destinationsError, + onRetryDestinations, + name, + onNameChange, + nameError, + setNameError, + conflict, + nameTooLong, + formError, + canSubmit, + busy, + onSubmit, + onClose, +}: { + secretName: string; + originToken: string; + mode: CopyMoveMode; + onModeChange: (mode: CopyMoveMode) => void; + destination: SecretDestination | null; + destinations: SecretDestination[]; + workspaceNameById: Record; + onDestinationChange: (destination: SecretDestination) => void; + destinationsLoading: boolean; + destinationsError: string | null; + onRetryDestinations: () => void; + name: string; + onNameChange: (name: string) => void; + nameError: string | null; + setNameError: (error: string | null) => void; + conflict: boolean; + nameTooLong: boolean; + formError: string | null; + canSubmit: boolean; + busy: boolean; + onSubmit: () => void; + onClose: () => void; +}) { + const { t } = useTranslation(); + return ( + <> + + + {t("settings:copyMoveSecretNamed", { name: secretName })} + + {t("settings:copyMoveDialogHint")} + + + {/* The middle section scrolls on short viewports; the header and the + footer stay pinned so actions are never pushed off-screen. */} +
+ + + + + {formError !== null && ( +

+ {formError} +

+ )} +
+ + + ); +} + +/** Renders the dialog footer with cancel, submit, and close actions for a copy/move transfer. */ +function TransferDialogFooter({ + mode, + canSubmit, + busy, + onSubmit, + onClose, +}: { + mode: CopyMoveMode; + canSubmit: boolean; + busy: boolean; + onSubmit: () => void; + onClose: () => void; +}) { + const { t } = useTranslation(); + return ( + + + + + + + + ); +} + +/** Renders the target-name input with conflict and validation error display. */ +function TargetNameField({ + name, + onNameChange, + nameError, + setNameError, + conflict, + nameTooLong, + disabled, +}: { + name: string; + onNameChange: (name: string) => void; + nameError: string | null; + setNameError: (error: string | null) => void; + conflict: boolean; + nameTooLong: boolean; + disabled: boolean; +}) { + const { t } = useTranslation(); + const invalid = conflict || nameError !== null || nameTooLong; + return ( +
+ + { + onNameChange(e.target.value); + if (nameError !== null) { + setNameError(null); + } + }} + disabled={disabled} + aria-invalid={invalid} + aria-describedby={invalid ? "copy-move-name-error" : undefined} + className="min-h-11" + /> + {invalid && ( +

+ {nameError ?? + (nameTooLong + ? t("settings:secretNameTooLong") + : t("settings:secretNameConflictInDestination", { name: name.trim() }))} +

+ )} +
+ ); +} + +/** Renders the copy/move mode radio group, including the move warning. */ +function TransferModeField({ + mode, + onModeChange, + originToken, +}: { + mode: CopyMoveMode; + onModeChange: (mode: CopyMoveMode) => void; + originToken: string; +}) { + const { t } = useTranslation(); + return ( +
+ {t("settings:copyMoveMode")} + onModeChange(value as CopyMoveMode)} + className="flex flex-col gap-2" + > + + + +
+ ); +} + +/** Renders the destination picker with loading, error/retry, and empty states. */ +function DestinationField({ + destinations, + workspaceNameById, + destination, + onDestinationChange, + destinationsLoading, + destinationsError, + onRetryDestinations, + disabled, +}: { + destinations: SecretDestination[]; + workspaceNameById: Record; + destination: SecretDestination | null; + onDestinationChange: (destination: SecretDestination) => void; + destinationsLoading: boolean; + destinationsError: string | null; + onRetryDestinations: () => void; + disabled: boolean; +}) { + const { t } = useTranslation(); + const noDestinations = + destinations.length === 0 && !destinationsLoading && destinationsError === null; + + return ( +
+ + {destinationsLoading && ( +

{t("settings:destinationsLoading")}

+ )} + {destinationsError !== null && ( +
+

{t("settings:destinationsLoadFailed")}

+ +
+ )} + {noDestinations && ( +

{t("settings:noValidDestinations")}

+ )} + {destination !== null && ( + + )} +
+ ); +} diff --git a/apps/web/components/settings/copy-move-secret-dialog.test.tsx b/apps/web/components/settings/copy-move-secret-dialog.test.tsx new file mode 100644 index 0000000000..bd62b81c1f --- /dev/null +++ b/apps/web/components/settings/copy-move-secret-dialog.test.tsx @@ -0,0 +1,459 @@ +import { act, cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { ApiError } from "@/lib/api/client"; +import type { SecretListItem } from "@/lib/types/http-secrets"; +import { + buildDefaultTargetName, + CopyMoveSecretDialog, + truncateUtf8Bytes, +} from "./copy-move-secret-dialog"; + +const { mockCopySecret, mockMoveSecret, mockUseDestinationNames, storeState } = vi.hoisted(() => { + const storeState = { + workspaces: { + items: [ + { id: "ws-1", name: "Alpha" }, + { id: "ws-2", name: "Beta" }, + ], + activeId: null, + }, + setWorkspaces: vi.fn(), + }; + return { + mockCopySecret: vi.fn(), + mockMoveSecret: vi.fn(), + mockUseDestinationNames: vi.fn< + ( + scope: string, + workspaceId?: string, + ) => { names: string[]; loaded: boolean; conflict: (name: string) => boolean } + >(() => ({ names: [], loaded: true, conflict: () => false })), + storeState, + }; +}); + +vi.mock("@/components/state-provider", () => ({ + useAppStore: (selector: (state: typeof storeState) => unknown) => selector(storeState), +})); +vi.mock("@/hooks/domains/settings/use-workspace-destinations", () => ({ + useWorkspaceDestinations: () => ({ loading: false, error: null, retry: vi.fn() }), +})); +vi.mock("@/hooks/domains/settings/use-secret-destination-names", () => ({ + useSecretDestinationNames: (scope: string, workspaceId?: string) => + mockUseDestinationNames(scope, workspaceId), +})); +vi.mock("@/lib/api/domains/secrets-api", () => ({ + copySecret: (...args: unknown[]) => mockCopySecret(...args), + moveSecret: (...args: unknown[]) => mockMoveSecret(...args), +})); + +const DEFAULT_GLOBAL_NAME = "API Key (from Global)"; + +const globalSecret: SecretListItem = { + id: "secret-1", + name: "API Key", + scope: "global", + has_value: true, + created_at: "2026-01-01T00:00:00Z", + updated_at: "2026-01-01T00:00:00Z", +}; + +const workspaceSecret: SecretListItem = { + ...globalSecret, + id: "secret-2", + name: "WS Key", + scope: "workspace", + workspace_id: "ws-1", +}; + +/** Renders the dialog with test fakes and returns the spies. */ +function renderDialog(overrides: Partial[0]> = {}) { + const onCompleted = vi.fn(); + const onClose = vi.fn(); + const view = render( + , + ); + return { onCompleted, onClose, view }; +} + +afterEach(async () => { + cleanup(); + mockCopySecret.mockReset(); + mockMoveSecret.mockReset(); + mockUseDestinationNames.mockReset(); + mockUseDestinationNames.mockReturnValue({ names: [], loaded: true, conflict: () => false }); + storeState.workspaces.items = [ + { id: "ws-1", name: "Alpha" }, + { id: "ws-2", name: "Beta" }, + ]; +}); + +const TAKEN_NAME = "Taken"; +const TAKEN_CONFLICT_MESSAGE = "A secret named Taken already exists in this destination."; + +/** Whether the Copy/Move radio with the given name reports aria-checked. */ +function radioChecked(name: RegExp): boolean { + return screen.getByRole("radio", { name }).getAttribute("aria-checked") === "true"; +} + +/** Whether the target-name input reports aria-invalid. */ +function inputInvalid(): boolean { + return ( + (screen.getByLabelText("Name") as HTMLInputElement).getAttribute("aria-invalid") === "true" + ); +} + +describe("CopyMoveSecretDialog", () => { + it("pre-fills the name with the origin suffix and defaults to Copy", () => { + renderDialog(); + expect(screen.getByRole("heading", { name: `Copy or move ${globalSecret.name}` })).toBeTruthy(); + expect((screen.getByLabelText("Name") as HTMLInputElement).value).toBe(DEFAULT_GLOBAL_NAME); + expect(radioChecked(/^Copy/)).toBe(true); + expect(screen.getByRole("button", { name: "Copy" })).toBeTruthy(); + }); + + it("shows General as the default destination for a workspace source", () => { + renderDialog({ secret: workspaceSecret, originToken: "Alpha" }); + expect((screen.getByLabelText("Name") as HTMLInputElement).value).toBe("WS Key (from Alpha)"); + expect(screen.getByText("Global")).toBeTruthy(); + }); + + it("resets the form when a different secret is targeted while mounted", async () => { + // The dialog stays mounted between opens so Radix can restore focus to the + // trigger on close; targeting a new secret must not leak the previous + // mode/name/destination. + const { view } = renderDialog({ secret: workspaceSecret, originToken: "Alpha" }); + fireEvent.click(screen.getByRole("radio", { name: /^Move/ })); + fireEvent.change(screen.getByLabelText("Name"), { target: { value: "Custom name" } }); + fireEvent.click(screen.getByRole("combobox", { name: "Destination" })); + fireEvent.click(await screen.findByRole("option", { name: "Beta" })); + + view.rerender( + , + ); + + expect(radioChecked(/^Copy/)).toBe(true); + expect((screen.getByLabelText("Name") as HTMLInputElement).value).toBe(DEFAULT_GLOBAL_NAME); + // Destination re-defaults for the new source (Global source: first workspace). + expect(screen.getByRole("combobox", { name: "Destination" }).textContent).toContain("Alpha"); + }); + + it("clears the form again when the same secret is reopened", () => { + // Close then reopen the dialog for the same secret: the stale name/mode + // from the previous session must not leak into the reopened dialog. + const { view } = renderDialog(); + fireEvent.click(screen.getByRole("radio", { name: /^Move/ })); + fireEvent.change(screen.getByLabelText("Name"), { target: { value: "Custom name" } }); + + view.rerender( + , + ); + view.rerender( + , + ); + + expect(radioChecked(/^Copy/)).toBe(true); + expect((screen.getByLabelText("Name") as HTMLInputElement).value).toBe(DEFAULT_GLOBAL_NAME); + }); + + it("switches the submit label to Move and shows the warning", () => { + renderDialog(); + fireEvent.click(screen.getByRole("radio", { name: /^Move/ })); + expect(screen.getByRole("button", { name: "Move" })).toBeTruthy(); + expect(screen.getByText("The original secret will be removed from Global.")).toBeTruthy(); + }); + + it("blocks submit on a conflicting target name with an invalid field", () => { + mockUseDestinationNames.mockReturnValue({ + names: [TAKEN_NAME], + loaded: true, + conflict: (name: string) => name.trim() === TAKEN_NAME, + }); + renderDialog(); + fireEvent.change(screen.getByLabelText("Name"), { target: { value: TAKEN_NAME } }); + expect(inputInvalid()).toBe(true); + expect(screen.getByText(TAKEN_CONFLICT_MESSAGE)).toBeTruthy(); + expect((screen.getByRole("button", { name: "Copy" }) as HTMLButtonElement).disabled).toBe(true); + }); +}); + +describe("CopyMoveSecretDialog submit", () => { + it("submits a copy with the destination workspace payload and calls onCompleted", async () => { + mockCopySecret.mockResolvedValue({ id: "new-1", name: DEFAULT_GLOBAL_NAME }); + const { onCompleted } = renderDialog(); + fireEvent.click(screen.getByRole("button", { name: "Copy" })); + + await waitFor(() => expect(mockCopySecret).toHaveBeenCalledTimes(1)); + expect(mockCopySecret).toHaveBeenCalledWith( + "secret-1", + { + target_scope: "workspace", + target_workspace_id: "ws-1", + name: DEFAULT_GLOBAL_NAME, + }, + undefined, + ); + await waitFor(() => expect(onCompleted).toHaveBeenCalled()); + }); + + it("clears the workspace id when the destination switches to General", async () => { + mockMoveSecret.mockResolvedValue({ id: "new-1", name: "moved" }); + renderDialog({ secret: workspaceSecret, originToken: "Alpha" }); + + fireEvent.click(screen.getByRole("radio", { name: /^Move/ })); + fireEvent.click(screen.getByRole("button", { name: "Move" })); + + await waitFor(() => expect(mockMoveSecret).toHaveBeenCalledTimes(1)); + const payload = mockMoveSecret.mock.calls[0][1] as Record; + expect(payload).toEqual({ target_scope: "global", name: "WS Key (from Alpha)" }); + expect(JSON.stringify(payload)).not.toContain('"name":null'); + expect(JSON.stringify(payload)).not.toContain("target_workspace_id"); + // A workspace-scoped source must carry its workspace id in the query. + expect(mockMoveSecret.mock.calls[0][2]).toEqual({ workspaceId: "ws-1" }); + }); + + it("surfaces a 409 conflict on the name field", async () => { + mockCopySecret.mockRejectedValue(new ApiError("conflict", 409, {})); + const { onCompleted } = renderDialog(); + fireEvent.change(screen.getByLabelText("Name"), { target: { value: DEFAULT_GLOBAL_NAME } }); + fireEvent.click(screen.getByRole("button", { name: "Copy" })); + + await waitFor(() => + expect( + screen.getByText( + "A secret named API Key (from Global) already exists in this destination.", + ), + ).toBeTruthy(), + ); + expect(onCompleted).not.toHaveBeenCalled(); + }); +}); + +describe("CopyMoveSecretDialog conflict handling", () => { + it("blocks an overlong target name with a localized byte-limit error", () => { + renderDialog(); + const input = screen.getByLabelText("Name") as HTMLInputElement; + // 101 bytes: exceeds the 100-byte UTF-8 backend limit without submitting. + fireEvent.change(input, { target: { value: "x".repeat(101) } }); + expect(screen.getByText("Name must be at most 100 bytes.")).toBeTruthy(); + expect(inputInvalid()).toBe(true); + expect((screen.getByRole("button", { name: "Copy" }) as HTMLButtonElement).disabled).toBe(true); + + // A multibyte name is counted in bytes, not characters. + fireEvent.change(input, { target: { value: "é".repeat(51) } }); // 102 bytes + expect(inputInvalid()).toBe(true); + fireEvent.change(input, { target: { value: "é".repeat(50) } }); // 100 bytes: valid + expect(inputInvalid()).toBe(false); + expect((screen.getByRole("button", { name: "Copy" }) as HTMLButtonElement).disabled).toBe( + false, + ); + }); + + it("clears a 409 name conflict when the destination changes", async () => { + mockMoveSecret.mockRejectedValue(new ApiError("conflict", 409, {})); + renderDialog({ secret: workspaceSecret, originToken: "Alpha" }); + fireEvent.click(screen.getByRole("radio", { name: /^Move/ })); + fireEvent.click(screen.getByRole("button", { name: "Move" })); + + await waitFor(() => expect(screen.getByText(/already exists/)).toBeTruthy()); + expect((screen.getByRole("button", { name: "Move" }) as HTMLButtonElement).disabled).toBe(true); + + // The 409 is destination-specific: switching to a workspace where the name + // is free must clear the error and re-enable submission. + fireEvent.click(screen.getByRole("combobox", { name: "Destination" })); + fireEvent.click(await screen.findByRole("option", { name: "Beta" })); + + // Prove the destination actually changed: the trigger must now display + // Beta (not the initial General), otherwise the assertions below could + // pass while the selection stayed on the original destination. + await waitFor(() => + expect(screen.getByRole("combobox", { name: "Destination" }).textContent).toContain("Beta"), + ); + await waitFor(() => expect(screen.queryByText(/already exists/)).toBeNull()); + expect((screen.getByRole("button", { name: "Move" }) as HTMLButtonElement).disabled).toBe( + false, + ); + }); + + it("shows a generic failure on 404 and never removes the source row", async () => { + mockMoveSecret.mockRejectedValue(new ApiError("not found", 404, {})); + const { onCompleted, onClose } = renderDialog({ + secret: workspaceSecret, + originToken: "Alpha", + }); + fireEvent.click(screen.getByRole("radio", { name: /^Move/ })); + fireEvent.click(screen.getByRole("button", { name: "Move" })); + + await waitFor(() => + expect(screen.getByText("Could not copy or move the secret.")).toBeTruthy(), + ); + // A 404 is deliberately ambiguous (missing source OR missing/unauthorized + // destination): the dialog stays open, nothing is completed, and no row + // removal callback fires. + expect(onCompleted).not.toHaveBeenCalled(); + expect(onClose).not.toHaveBeenCalled(); + expect(screen.getByRole("dialog")).toBeTruthy(); + }); + + it("disables submit synchronously when the selected workspace vanishes from the list", () => { + const { view, onClose, onCompleted } = renderDialog(); + expect((screen.getByRole("button", { name: "Copy" }) as HTMLButtonElement).disabled).toBe( + false, + ); + + storeState.workspaces.items = []; + view.rerender( + , + ); + + // The validity check is synchronous: the stale workspace destination must + // not be submittable even before the cleanup effect runs. + expect((screen.getByRole("button", { name: "Copy" }) as HTMLButtonElement).disabled).toBe(true); + }); +}); + +describe("CopyMoveSecretDialog in-flight transfers", () => { + it("ignores duplicate clicks while a transfer is in flight", async () => { + const { promise, resolve } = Promise.withResolvers(); + mockCopySecret.mockReturnValue(promise); + renderDialog(); + const submit = screen.getByRole("button", { name: "Copy" }); + fireEvent.click(submit); + fireEvent.click(submit); + + expect(mockCopySecret).toHaveBeenCalledTimes(1); + await act(async () => { + resolve({ id: "new-1", name: DEFAULT_GLOBAL_NAME }); + }); + }); + + it("discards a stale transfer completion after switching to another secret", async () => { + // The dialog stays mounted between opens. Submit for secret A, then (while + // the request is in flight) reopen the dialog for secret B: A's completion + // must not fire onCompleted against B's dialog. + const { promise, resolve } = Promise.withResolvers(); + mockCopySecret.mockReturnValue(promise); + const { view, onCompleted } = renderDialog({ secret: workspaceSecret, originToken: "Alpha" }); + fireEvent.click(screen.getByRole("button", { name: "Copy" })); + expect(mockCopySecret).toHaveBeenCalledTimes(1); + + // The close affordance is disabled while a transfer is in flight. + expect((screen.getByRole("button", { name: "Close" }) as HTMLButtonElement).disabled).toBe( + true, + ); + + view.rerender( + , + ); + await act(async () => { + resolve({ id: "new-1", name: DEFAULT_GLOBAL_NAME }); + }); + + expect(onCompleted).not.toHaveBeenCalled(); + }); + + it("discards a stale completion when the same secret is reopened", async () => { + // Submit for a secret, close the dialog (the X is disabled while busy but + // Escape/overlay could still dismiss), then reopen the SAME secret: the + // first session's completion must not fire against the reopened dialog. + const { promise, resolve } = Promise.withResolvers(); + mockCopySecret.mockReturnValue(promise); + const { view, onCompleted } = renderDialog({ secret: workspaceSecret, originToken: "Alpha" }); + fireEvent.click(screen.getByRole("button", { name: "Copy" })); + + view.rerender( + , + ); + view.rerender( + , + ); + await act(async () => { + resolve({ id: "new-1", name: DEFAULT_GLOBAL_NAME }); + }); + + expect(onCompleted).not.toHaveBeenCalled(); + }); +}); + +describe("truncateUtf8Bytes", () => { + it("keeps ASCII under the limit unchanged", () => { + expect(truncateUtf8Bytes("short", 100)).toBe("short"); + }); + + it("truncates multibyte input to the byte limit", () => { + const long = "é".repeat(60); // 120 bytes + const truncated = truncateUtf8Bytes(long, 100); + expect(new TextEncoder().encode(truncated).length).toBe(100); + expect(truncated).toBe("é".repeat(50)); + }); + + it("never splits astral-plane surrogate pairs", () => { + const value = "🔐".repeat(40); // 160 bytes + const truncated = truncateUtf8Bytes(value, 100); + expect(truncated).toBe("🔐".repeat(25)); + expect(new TextEncoder().encode(truncated).length).toBe(100); + expect(truncated.includes("\uFFFD")).toBe(false); + }); +}); + +describe("buildDefaultTargetName", () => { + it("builds the locale-independent origin suffix within the byte limit", () => { + const name = buildDefaultTargetName("API Key", "Global"); + expect(name).toBe(DEFAULT_GLOBAL_NAME); + expect(new TextEncoder().encode(name).length).toBeLessThanOrEqual(100); + }); + + it("truncates an over-long source name so the copy can never fail validation", () => { + const name = buildDefaultTargetName("K".repeat(120), "general"); + expect(new TextEncoder().encode(name).length).toBeLessThanOrEqual(100); + }); +}); diff --git a/apps/web/components/settings/copy-move-secret-dialog.tsx b/apps/web/components/settings/copy-move-secret-dialog.tsx new file mode 100644 index 0000000000..ba3ada2685 --- /dev/null +++ b/apps/web/components/settings/copy-move-secret-dialog.tsx @@ -0,0 +1,424 @@ +"use client"; + +import { useCallback, useEffect, useMemo, useRef, useState } from "react"; +import { useTranslation } from "react-i18next"; +import { Dialog, DialogContent } from "@kandev/ui/dialog"; +import { useAppStore } from "@/components/state-provider"; +import { ApiError } from "@/lib/api/client"; +import { copySecret, moveSecret } from "@/lib/api/domains/secrets-api"; +import { useSecretDestinationNames } from "@/hooks/domains/settings/use-secret-destination-names"; +import { useWorkspaceDestinations } from "@/hooks/domains/settings/use-workspace-destinations"; +import type { SecretListItem } from "@/lib/types/http-secrets"; +import { CopyMoveDialogBody } from "./copy-move-dialog-body"; + +export const MAX_SECRET_NAME_BYTES = 100; + +export type CopyMoveMode = "copy" | "move"; + +export type SecretDestination = { scope: "global" } | { scope: "workspace"; workspaceId: string }; + +/** + * Truncates a string so its UTF-8 byte length is at most `limit`, iterating + * code points (never splitting surrogate pairs or producing replacement + * characters). The backend name limit is 100 UTF-8 bytes. + */ +export function truncateUtf8Bytes(value: string, limit: number): string { + const encoder = new TextEncoder(); + if (encoder.encode(value).length <= limit) { + return value; + } + let result = ""; + for (const char of value) { + const next = result + char; + if (encoder.encode(next).length > limit) { + break; + } + result = next; + } + return result; +} + +/** Default target name: ` (from )`, truncated to the byte limit. */ +export function buildDefaultTargetName(name: string, originToken: string): string { + return truncateUtf8Bytes(`${name} (from ${originToken})`, MAX_SECRET_NAME_BYTES); +} + +type CopyMoveSecretDialogProps = { + secret: SecretListItem; + /** `general` for a Global source, the workspace name otherwise (literal, locale-independent). */ + originToken: string; + /** Controlled visibility. The dialog stays mounted while closed so Radix can + * restore focus to the trigger on close; callers must keep `secret` set. */ + open: boolean; + onClose: () => void; + onCompleted: (item: SecretListItem, mode: CopyMoveMode) => void; +}; + +/** Renders the copy/move secret dialog, wiring form state, destinations, and submission. */ +export function CopyMoveSecretDialog({ + secret, + originToken, + open, + onClose, + onCompleted, +}: CopyMoveSecretDialogProps) { + const { + loading: destinationsLoading, + error: destinationsError, + retry: retryDestinations, + } = useWorkspaceDestinations(); + const workspaces = useAppStore((state) => state.workspaces.items); + const [mode, setMode] = useState("copy"); + const [destination, setDestination] = useState(null); + const [name, setName] = useState(() => buildDefaultTargetName(secret.name, originToken)); + const [nameError, setNameError] = useState(null); + const [destinationNamesKey, setDestinationNamesKey] = useState(0); + + const { destinations, workspaceNameById, destinationValid } = useDestinationOptions( + secret, + workspaces, + destination, + setDestination, + ); + const destinationNames = useSecretDestinationNames( + destination?.scope ?? "global", + destination?.scope === "workspace" ? destination.workspaceId : undefined, + destinationNamesKey, + ); + const conflict = nameError === null && destination !== null && destinationNames.conflict(name); + // The backend limit is 100 UTF-8 bytes; a user-edited name that exceeds it + // would otherwise fail server-side with a generic transfer error. + const nameTooLong = new TextEncoder().encode(name.trim()).length > MAX_SECRET_NAME_BYTES; + + const { busy, canSubmit, formError, clearError, run } = useTransferSubmit({ + secret, + mode, + destination, + destinationValid, + name, + nameError, + conflict, + nameTooLong, + destinationsLoading, + destinationsError: destinationsError !== null, + setNameError, + onCompleted, + open, + }); + const resetDestinationNames = useCallback(() => setDestinationNamesKey((key) => key + 1), []); + // Stable identity: the reset effect depends on this object, so it must not + // change every render (the guard inside useTransferFormReset would still + // keep behavior correct, but the effect would fire needlessly). + const transferSetters = useMemo( + () => ({ setMode, setDestination, setName, setNameError, clearError, resetDestinationNames }), + [setMode, setDestination, setName, setNameError, clearError, resetDestinationNames], + ); + useTransferFormReset(secret, originToken, open, transferSetters); + return ( + { + // While a transfer is in flight, no dismiss path (X, Escape, overlay) + // may close the dialog: a stale completion must not land in a + // reopened session for the same secret. + if (!next && !busy) { + onClose(); + } + }} + > + + { + // A 409 pins the name field and a non-409 failure pins the alert; + // both are destination-specific, so clear them on target change. + if (nameError !== null) { + setNameError(null); + } + clearError(); + setDestination(next); + }} + destinationsLoading={destinationsLoading} + destinationsError={destinationsError} + onRetryDestinations={retryDestinations} + name={name} + onNameChange={setName} + nameError={nameError} + nameTooLong={nameTooLong} + setNameError={setNameError} + conflict={conflict} + formError={formError} + canSubmit={canSubmit} + busy={busy} + onSubmit={run} + onClose={onClose} + /> + + + ); +} + +/** + * The dialog stays mounted between opens so Radix can restore focus to the + * trigger on close. This resets every piece of form state when a new secret is + * targeted or when the dialog is reopened, while skipping the mount render so + * the initial useState values (and the destination auto-selection) survive. + */ +function useTransferFormReset( + secret: SecretListItem, + originToken: string, + open: boolean, + setters: { + setMode: (mode: CopyMoveMode) => void; + setDestination: (destination: SecretDestination | null) => void; + setName: (name: string) => void; + setNameError: (error: string | null) => void; + clearError: () => void; + resetDestinationNames: () => void; + }, +) { + const lastSecretId = useRef(secret.id); + const prevOpen = useRef(open); + useEffect(() => { + const reopened = open && !prevOpen.current; + prevOpen.current = open; + if (!reopened && lastSecretId.current === secret.id) { + return; + } + lastSecretId.current = secret.id; + setters.setMode("copy"); + setters.setDestination(null); + setters.setName(buildDefaultTargetName(secret.name, originToken)); + setters.setNameError(null); + setters.clearError(); + setters.resetDestinationNames(); + }, [secret.id, originToken, open, setters]); +} + +type TransferCanSubmitArgs = { + isBusy: boolean; + destination: SecretDestination | null; + destinationValid: boolean; + name: string; + nameError: string | null; + conflict: boolean; + nameTooLong: boolean; + destinationsLoading: boolean; + destinationsError: boolean; +}; + +/** Returns whether the transfer can be submitted given the current form, destination, and request state. */ +function transferCanSubmit({ + isBusy, + destination, + destinationValid, + name, + nameError, + conflict, + nameTooLong, + destinationsLoading, + destinationsError, +}: TransferCanSubmitArgs): boolean { + return ( + !isBusy && + destination !== null && + destinationValid && + name.trim().length > 0 && + nameError === null && + !conflict && + !nameTooLong && + !destinationsLoading && + !destinationsError + ); +} + +type TransferSubmitArgs = { + secret: SecretListItem; + mode: CopyMoveMode; + destination: SecretDestination | null; + destinationValid: boolean; + name: string; + nameError: string | null; + conflict: boolean; + nameTooLong: boolean; + destinationsLoading: boolean; + destinationsError: boolean; + setNameError: (error: string | null) => void; + onCompleted: (item: SecretListItem, mode: CopyMoveMode) => void; + /** Controlled visibility; drives the session counter for stale-result guards. */ + open: boolean; +}; + +/** Runs the copy/move request, guarding against stale results across dialog sessions, and exposes busy/canSubmit/formError state. */ +function useTransferSubmit({ + secret, + mode, + destination, + destinationValid, + name, + nameError, + conflict, + nameTooLong, + destinationsLoading, + destinationsError, + setNameError, + onCompleted, + open, +}: TransferSubmitArgs) { + const { t } = useTranslation(); + const [isBusy, setIsBusy] = useState(false); + const [formError, setFormError] = useState(null); + // The dialog stays mounted across opens; a completion or error for a + // previously targeted secret must not mutate the dialog now showing another. + // The session counter also covers closing and reopening the SAME secret: + // a result from the previous session must not land in the reopened dialog. + const secretIdRef = useRef(secret.id); + const sessionRef = useRef(0); + useEffect(() => { + secretIdRef.current = secret.id; + }, [secret.id]); + useEffect(() => { + if (open) { + sessionRef.current += 1; + } + }, [open]); + + const canSubmit = transferCanSubmit({ + isBusy, + destination, + destinationValid, + name, + nameError, + conflict, + nameTooLong, + destinationsLoading, + destinationsError, + }); + + /** Runs the transfer request, guarding stale completions against dialog reopenings. */ + const run = async () => { + if (!canSubmit || destination === null) { + return; + } + const startedSecretId = secret.id; + const startedSession = sessionRef.current; + setIsBusy(true); + setFormError(null); + const trimmedName = name.trim(); + const payload = { + target_scope: destination.scope, + ...(destination.scope === "workspace" + ? { target_workspace_id: destination.workspaceId } + : {}), + name: trimmedName, + } as const; + try { + // A workspace-scoped source requires its workspace id in the query. + const sourceOptions = + secret.scope === "workspace" && secret.workspace_id + ? { workspaceId: secret.workspace_id } + : undefined; + const item = + mode === "copy" + ? await copySecret(secret.id, payload, sourceOptions) + : await moveSecret(secret.id, payload, sourceOptions); + if (secretIdRef.current === startedSecretId && sessionRef.current === startedSession) { + onCompleted(item, mode); + } + } catch (err) { + // A stale failure (user switched to another secret mid-flight, or closed + // and reopened the same one) must not paint onto the current dialog. + if (secretIdRef.current !== startedSecretId || sessionRef.current !== startedSession) { + return; + } + if (err instanceof ApiError && err.status === 409) { + setNameError(t("settings:secretNameConflictInDestination", { name: trimmedName })); + } else { + // 404 is deliberately ambiguous (missing source OR missing/unauthorized + // destination) to avoid disclosure; never remove the source row on it. + setFormError(t("settings:transferFailed")); + } + } finally { + setIsBusy(false); + } + }; + + const clearError = useCallback(() => setFormError(null), []); + + return { busy: isBusy, canSubmit, formError, clearError, run }; +} + +type WorkspaceOption = { id: string; name: string }; + +/** + * Computes the valid destinations for a secret (General plus every other + * workspace) and keeps the selection in sync: auto-selects the first option + * when nothing is chosen, and clears a selection that vanished from the list + * (deleted workspace or failed list) so submission can never target a stale + * destination. `null` keeps canSubmit false when nothing valid remains (a + * Global source must never fall back to a same-scope target). + */ +function useDestinationOptions( + secret: SecretListItem, + workspaces: WorkspaceOption[], + destination: SecretDestination | null, + onDestinationChange: (destination: SecretDestination | null) => void, +) { + const workspaceNameById = Object.fromEntries( + workspaces.map((workspace) => [workspace.id, workspace.name]), + ); + const destinations = useMemo(() => { + const list: SecretDestination[] = []; + if (secret.scope !== "global") { + list.push({ scope: "global" }); + } + for (const workspace of workspaces) { + if (secret.scope === "workspace" && workspace.id === secret.workspace_id) { + continue; + } + list.push({ scope: "workspace", workspaceId: workspace.id }); + } + return list; + }, [secret, workspaces]); + + // Synchronous validity: the selected destination must be present in the + // CURRENT list. canSubmit consumes this on the same render, so a workspace + // that vanished (deleted or failed list) can never be submitted even + // between renders; the effect below only performs state cleanup. + const destinationValid = + destination !== null && + destinations.some((option) => { + if (option.scope !== destination.scope) { + return false; + } + if (destination.scope === "workspace") { + return option.scope === "workspace" && option.workspaceId === destination.workspaceId; + } + return true; + }); + + useEffect(() => { + if (destination === null && destinations.length > 0) { + onDestinationChange(destinations[0]); + return; + } + if (destination !== null && !destinationValid) { + // The selected destination disappeared from the list: clear it so the + // UI cannot show a stale selection. null keeps canSubmit false when + // nothing valid remains (a Global source must never fall back to a + // same-scope target). + onDestinationChange(destinations[0] ?? null); + } + }, [destination, destinations, destinationValid, onDestinationChange]); + + return { destinations, workspaceNameById, destinationValid }; +} diff --git a/apps/web/components/settings/secret-form.tsx b/apps/web/components/settings/secret-form.tsx new file mode 100644 index 0000000000..1a17ebe150 --- /dev/null +++ b/apps/web/components/settings/secret-form.tsx @@ -0,0 +1,83 @@ +"use client"; + +import { useTranslation } from "react-i18next"; +import { Button } from "@kandev/ui/button"; +import { Input } from "@kandev/ui/input"; +import { Textarea } from "@kandev/ui/textarea"; + +export type SecretFormState = { + name: string; + value: string; +}; + +export const defaultFormState: SecretFormState = { + name: "", + value: "", +}; + +type SecretFormProps = { + title: string; + formState: SecretFormState; + onFormChange: (patch: Partial) => void; + onSubmit: () => void; + onCancel: () => void; + isValid: boolean; + isBusy: boolean; + submitLabel: string; + showSubmit?: boolean; + baselineState?: SecretFormState; +}; + +/** Renders the secret name/value form with submit and cancel actions. */ +export function SecretForm({ + title, + formState, + onFormChange, + onSubmit, + onCancel, + isValid, + isBusy, + submitLabel, + showSubmit = true, + baselineState, +}: SecretFormProps) { + const { t } = useTranslation(); + const nameIsDirty = Boolean(baselineState) && formState.name.trim() !== baselineState?.name; + const valueIsDirty = Boolean(baselineState) && formState.value !== baselineState?.value; + return ( +
+
{title}
+
+ onFormChange({ name: e.target.value })} + placeholder={t("settings:nameEGOpenaiProductionKey")} + disabled={isBusy} + data-settings-dirty={nameIsDirty} + /> +