dreampiler/kandev
1
0
Fork 0
Code Issues 43 Pull Requests Actions 6 Packages Projects Releases Wiki Activity

refactor(secrets): detect absent secret via ErrNotFound sentinel (#1770)

Browse Source 
* refactor(secrets): detect absent secret via ErrNotFound sentinel

Replace the "secret not found:" string-prefix matching with an exported
secrets.ErrNotFound sentinel, matched via errors.Is. The store(s) now
return fmt.Errorf("%w: %s", ErrNotFound, id); the rendered message is
byte-identical so logs are unchanged.

Consumers migrated off the prefix:
- internal/plugins/config.go isSecretNotFound (behind host GetSecret/
  DeleteSecret and service.go vault rollback)
- internal/integrations/secretadapter Exists (jira/linear/sentry inherit
  the change through the shared adapter)

Adds a non-Postgres-gated secrets test asserting errors.Is(err,
ErrNotFound) plus message-unchanged coverage; updates the affected test
fakes to wrap the sentinel.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* test(secrets): address review — eager t.Cleanup, faithful fake Delete

- sqlite_store_test: register sqlxDB close via t.Cleanup immediately
  after creation, before the NewMasterKeyProvider/Provide fatal paths
  (per the t.Cleanup-immediately convention).
- secretadapter_test: fakeStore.Delete now returns wrapped
  secrets.ErrNotFound for an absent id, mirroring the real store and
  the fakeSecretRevealer.Delete fake.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
João Salavisa
2026-07-18 01:08:29 +01:00
committed by GitHub
parent fa278e8dff
commit bb5a81a396
8 changed files with 113 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/backend/internal/integrations/secretadapter/secretadapter.go
+7 -7
View File
@@ -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
apps/backend/internal/integrations/secretadapter/secretadapter_test.go
+7 -3
View File
@@ -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
}
apps/backend/internal/plugins/config.go
+7 -9
View File
@@ -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
apps/backend/internal/plugins/service_test.go
+3 -2
View File
@@ -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
apps/backend/internal/secrets/postgres_store_test.go
+3 -3
View File
@@ -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)
}
}
apps/backend/internal/secrets/sqlite_store.go
+3 -3
View File
@@ -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
}
apps/backend/internal/secrets/sqlite_store_test.go
+73
View File
@@ -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: <id>" 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)
}
}
apps/backend/internal/secrets/store.go
+10 -1
View File
@@ -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.