diff --git a/apps/backend/internal/integrations/secretadapter/secretadapter.go b/apps/backend/internal/integrations/secretadapter/secretadapter.go index bfdc3a63b4..00cbd960b5 100644 --- a/apps/backend/internal/integrations/secretadapter/secretadapter.go +++ b/apps/backend/internal/integrations/secretadapter/secretadapter.go @@ -1,7 +1,7 @@ // Package secretadapter wraps a global secrets.SecretStore so per-integration // services (jira, linear, future) can use a small, upsert-style API without // each one re-implementing the Create-or-Update fallback or the -// "secret not found:" string-prefix check. +// secrets.ErrNotFound absence check. // // A single Adapter satisfies any integration's local SecretStore interface // shaped as { Reveal, Set, Delete, Exists } — Go's structural typing means @@ -10,7 +10,7 @@ package secretadapter import ( "context" - "strings" + "errors" "github.com/kandev/kandev/internal/secrets" ) @@ -33,8 +33,8 @@ func (a *Adapter) Reveal(ctx context.Context, id string) (string, error) { // Set upserts the secret value: tries Update first, falls back to Create // when the secret does not yet exist. func (a *Adapter) Set(ctx context.Context, id, name, value string) error { - // Detect existence via Exists (which inspects the "secret not found:" - // prefix) instead of treating any Get error as "not found": a transient + // Detect existence via Exists (which matches secrets.ErrNotFound) + // instead of treating any Get error as "not found": a transient // DB error on an existing row would otherwise turn into a constraint- // violation Create that masks the real cause. exists, err := a.Exists(ctx, id) @@ -77,9 +77,9 @@ func (a *Adapter) ListIDs(ctx context.Context) ([]string, error) { func (a *Adapter) Exists(ctx context.Context, id string) (bool, error) { _, err := a.store.Get(ctx, id) if err != nil { - // secrets layer reports "not found" via fmt.Errorf with the - // "secret not found:" prefix; treat that as the absence case. - if strings.HasPrefix(err.Error(), "secret not found:") { + // secrets layer reports an absent entry via secrets.ErrNotFound; + // treat that as the absence case, any other error as a fault. + if errors.Is(err, secrets.ErrNotFound) { return false, nil } return false, err diff --git a/apps/backend/internal/integrations/secretadapter/secretadapter_test.go b/apps/backend/internal/integrations/secretadapter/secretadapter_test.go index 6408e6ce5b..6bec7fe8df 100644 --- a/apps/backend/internal/integrations/secretadapter/secretadapter_test.go +++ b/apps/backend/internal/integrations/secretadapter/secretadapter_test.go @@ -3,6 +3,7 @@ package secretadapter import ( "context" "errors" + "fmt" "testing" "github.com/kandev/kandev/internal/secrets" @@ -37,7 +38,7 @@ func (f *fakeStore) Get(_ context.Context, id string) (*secrets.Secret, error) { return nil, f.getErr } if _, ok := f.rows[id]; !ok { - return nil, errors.New("secret not found: " + id) + return nil, fmt.Errorf("%w: %s", secrets.ErrNotFound, id) } return &secrets.Secret{ID: id}, nil } @@ -45,7 +46,7 @@ func (f *fakeStore) Get(_ context.Context, id string) (*secrets.Secret, error) { func (f *fakeStore) Reveal(_ context.Context, id string) (string, error) { v, ok := f.rows[id] if !ok { - return "", errors.New("secret not found: " + id) + return "", fmt.Errorf("%w: %s", secrets.ErrNotFound, id) } return v, nil } @@ -55,7 +56,7 @@ func (f *fakeStore) Update(_ context.Context, id string, req *secrets.UpdateSecr return err } if _, ok := f.rows[id]; !ok { - return errors.New("secret not found: " + id) + return fmt.Errorf("%w: %s", secrets.ErrNotFound, id) } if req.Value != nil { f.rows[id] = *req.Value @@ -64,6 +65,9 @@ func (f *fakeStore) Update(_ context.Context, id string, req *secrets.UpdateSecr } func (f *fakeStore) Delete(_ context.Context, id string) error { + if _, ok := f.rows[id]; !ok { + return fmt.Errorf("%w: %s", secrets.ErrNotFound, id) + } delete(f.rows, id) return nil } diff --git a/apps/backend/internal/plugins/config.go b/apps/backend/internal/plugins/config.go index bcc10ba6c9..ef0f8b3ffc 100644 --- a/apps/backend/internal/plugins/config.go +++ b/apps/backend/internal/plugins/config.go @@ -19,6 +19,8 @@ import ( "reflect" "regexp" "strings" + + "github.com/kandev/kandev/internal/secrets" ) // configSecretMask is the placeholder returned in place of a secret config @@ -26,17 +28,13 @@ import ( // value unchanged". Deliberately implausible as a real credential. const configSecretMask = "********" -// secretNotFoundPrefix is how the secrets layer signals an absent entry -// (fmt.Errorf("secret not found: ...") in internal/secrets). Centralized -// here so the several call sites that must tell "absent" apart from a real -// backend error share one definition instead of each re-hardcoding the -// prefix (host.go's GetSecret/DeleteSecret, service.go's vault rollback). -const secretNotFoundPrefix = "secret not found:" - // isSecretNotFound reports whether err is the secrets layer's "absent entry" -// signal rather than a genuine backend failure. +// signal (secrets.ErrNotFound) rather than a genuine backend failure. Kept as +// one helper so the several call sites that must tell "absent" apart from a +// real backend error share one definition (host.go's GetSecret/DeleteSecret, +// service.go's vault rollback). func isSecretNotFound(err error) bool { - return err != nil && strings.HasPrefix(err.Error(), secretNotFoundPrefix) + return errors.Is(err, secrets.ErrNotFound) } // pluginVaultIDPrefix is the reserved id namespace every plugin-owned vault diff --git a/apps/backend/internal/plugins/service_test.go b/apps/backend/internal/plugins/service_test.go index b9ddfc990a..74860bf0d5 100644 --- a/apps/backend/internal/plugins/service_test.go +++ b/apps/backend/internal/plugins/service_test.go @@ -16,6 +16,7 @@ import ( "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/plugins/pkgtar/pkgtartest" "github.com/kandev/kandev/internal/plugins/store" + "github.com/kandev/kandev/internal/secrets" "github.com/kandev/kandev/pkg/pluginsdk" ) @@ -197,7 +198,7 @@ func (v *fakeSecretRevealer) Reveal(_ context.Context, ref string) (string, erro defer v.mu.Unlock() value, ok := v.values[ref] if !ok { - return "", fmt.Errorf("secret not found: %s", ref) + return "", fmt.Errorf("%w: %s", secrets.ErrNotFound, ref) } return value, nil } @@ -213,7 +214,7 @@ func (v *fakeSecretRevealer) Delete(_ context.Context, id string) error { v.mu.Lock() defer v.mu.Unlock() if _, ok := v.values[id]; !ok { - return fmt.Errorf("secret not found: %s", id) + return fmt.Errorf("%w: %s", secrets.ErrNotFound, id) } delete(v.values, id) return nil diff --git a/apps/backend/internal/secrets/postgres_store_test.go b/apps/backend/internal/secrets/postgres_store_test.go index cef2e85dda..5f47b517d4 100644 --- a/apps/backend/internal/secrets/postgres_store_test.go +++ b/apps/backend/internal/secrets/postgres_store_test.go @@ -2,7 +2,7 @@ package secrets import ( "context" - "strings" + "errors" "testing" "github.com/kandev/kandev/internal/testutil" @@ -86,7 +86,7 @@ func TestPostgresStoreRoundTrip(t *testing.T) { if err := store.Delete(ctx, secret.ID); err != nil { t.Fatalf("delete secret: %v", err) } - if _, err := store.Get(ctx, secret.ID); err == nil || !strings.Contains(err.Error(), "secret not found") { - t.Fatalf("get deleted secret error = %v, want not found", err) + if _, err := store.Get(ctx, secret.ID); err == nil || !errors.Is(err, ErrNotFound) { + t.Fatalf("get deleted secret error = %v, want secrets.ErrNotFound", err) } } diff --git a/apps/backend/internal/secrets/sqlite_store.go b/apps/backend/internal/secrets/sqlite_store.go index 82720edb11..054b170765 100644 --- a/apps/backend/internal/secrets/sqlite_store.go +++ b/apps/backend/internal/secrets/sqlite_store.go @@ -84,7 +84,7 @@ func (s *sqliteStore) Get(ctx context.Context, id string) (*Secret, error) { FROM secrets WHERE id = ?`), id) if err != nil { if err == sql.ErrNoRows { - return nil, fmt.Errorf("secret not found: %s", id) + return nil, fmt.Errorf("%w: %s", ErrNotFound, id) } return nil, fmt.Errorf("get secret: %w", err) } @@ -98,7 +98,7 @@ func (s *sqliteStore) Reveal(ctx context.Context, id string) (string, error) { Scan(&ciphertext, &nonce) if err != nil { if err == sql.ErrNoRows { - return "", fmt.Errorf("secret not found: %s", id) + return "", fmt.Errorf("%w: %s", ErrNotFound, id) } return "", fmt.Errorf("reveal secret: %w", err) } @@ -155,7 +155,7 @@ func (s *sqliteStore) Delete(ctx context.Context, id string) error { } rows, _ := result.RowsAffected() if rows == 0 { - return fmt.Errorf("secret not found: %s", id) + return fmt.Errorf("%w: %s", ErrNotFound, id) } return nil } diff --git a/apps/backend/internal/secrets/sqlite_store_test.go b/apps/backend/internal/secrets/sqlite_store_test.go new file mode 100644 index 0000000000..171ae35a1f --- /dev/null +++ b/apps/backend/internal/secrets/sqlite_store_test.go @@ -0,0 +1,73 @@ +package secrets + +import ( + "context" + "errors" + "path/filepath" + "testing" + + "github.com/jmoiron/sqlx" + + "github.com/kandev/kandev/internal/db" +) + +func newTestSQLiteStore(t *testing.T) *sqliteStore { + t.Helper() + dir := t.TempDir() + + conn, err := db.OpenSQLite(filepath.Join(dir, "secrets.db")) + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + sqlxDB := sqlx.NewDb(conn, "sqlite3") + t.Cleanup(func() { + _ = sqlxDB.Close() + }) + + crypto, err := NewMasterKeyProvider(dir) + if err != nil { + t.Fatalf("master key: %v", err) + } + + store, cleanup, err := Provide(sqlxDB, sqlxDB, crypto) + if err != nil { + t.Fatalf("provide store: %v", err) + } + t.Cleanup(func() { + _ = cleanup() + }) + return store +} + +// TestSQLiteStore_MissingID_IsErrNotFound proves the store signals an absent +// entry via the exported secrets.ErrNotFound sentinel (matchable with +// errors.Is), so consumers don't have to string-match the message. +func TestSQLiteStore_MissingID_IsErrNotFound(t *testing.T) { + store := newTestSQLiteStore(t) + ctx := context.Background() + + if _, err := store.Get(ctx, "does-not-exist"); !errors.Is(err, ErrNotFound) { + t.Errorf("Get(missing) error = %v, want errors.Is ErrNotFound", err) + } + if _, err := store.Reveal(ctx, "does-not-exist"); !errors.Is(err, ErrNotFound) { + t.Errorf("Reveal(missing) error = %v, want errors.Is ErrNotFound", err) + } + if err := store.Delete(ctx, "does-not-exist"); !errors.Is(err, ErrNotFound) { + t.Errorf("Delete(missing) error = %v, want errors.Is ErrNotFound", err) + } +} + +// TestSQLiteStore_MissingID_MessageUnchanged confirms the human-readable text +// is still "secret not found: " so existing logs are unchanged; only the +// detection mechanism (errors.Is) is new. +func TestSQLiteStore_MissingID_MessageUnchanged(t *testing.T) { + store := newTestSQLiteStore(t) + + _, err := store.Get(context.Background(), "abc") + if err == nil { + t.Fatal("Get(missing) returned nil error") + } + if got, want := err.Error(), "secret not found: abc"; got != want { + t.Errorf("error message = %q, want %q", got, want) + } +} diff --git a/apps/backend/internal/secrets/store.go b/apps/backend/internal/secrets/store.go index f99404e9bf..a89fe3d445 100644 --- a/apps/backend/internal/secrets/store.go +++ b/apps/backend/internal/secrets/store.go @@ -1,6 +1,15 @@ package secrets -import "context" +import ( + "context" + "errors" +) + +// ErrNotFound is the sentinel returned (wrapped) by store implementations when +// a secret id is absent. Consumers that must tell "absent entry" apart from a +// genuine backend fault should match with errors.Is(err, secrets.ErrNotFound) +// rather than string-matching the error text. +var ErrNotFound = errors.New("secret not found") // SecretStore abstracts secret storage. Implementations handle // encryption/decryption internally.