diff --git a/.codereview/agents/go/implementation-tests/prompt.md b/.codereview/agents/go/implementation-tests/prompt.md index 3d82c8c..dfc8496 100644 --- a/.codereview/agents/go/implementation-tests/prompt.md +++ b/.codereview/agents/go/implementation-tests/prompt.md @@ -1,4 +1,4 @@ -You are reviewing Go implementation quality and test adequacy for codereview-cli. +You are reviewing Go implementation quality and test adequacy for this repository. Optimize for high-signal findings. Return no findings when the Go code is idiomatic enough, the changed behavior is adequately tested for its risk, or a concern would require speculation. This is not a general policy, architecture, security, or formatting reviewer. diff --git a/credstore/filebackend_nocgo_unix.go b/credstore/filebackend_nocgo_unix.go index 93a87e6..9d77e0b 100644 --- a/credstore/filebackend_nocgo_unix.go +++ b/credstore/filebackend_nocgo_unix.go @@ -42,6 +42,20 @@ func (b *fileKeyringBackend) get(itemKey string) (keyringItem, error) { return decodeFileKeyringItem(string(bytes), b.password) } +func (b *fileKeyringBackend) metadata(itemKey string) (keyringItem, error) { + name, err := b.filename(itemKey) + if err != nil { + return keyringItem{}, err + } + if _, err := os.Stat(name); err != nil { + if os.IsNotExist(err) { + return keyringItem{}, errKeyringItemNotFound + } + return keyringItem{}, err + } + return keyringItem{key: itemKey}, nil +} + func (b *fileKeyringBackend) set(it keyringItem) error { if err := b.unlock(); err != nil { return err diff --git a/credstore/osbackend_byteness.go b/credstore/osbackend_byteness.go index fa32fbf..5f78a7a 100644 --- a/credstore/osbackend_byteness.go +++ b/credstore/osbackend_byteness.go @@ -65,6 +65,33 @@ func (b bytenessBackend) get(itemKey string) (keyringItem, error) { }, nil } +func (b bytenessBackend) metadata(itemKey string) (keyringItem, error) { + md, err := b.kr.GetMetadata(itemKey) + if err != nil { + switch { + case errors.Is(err, keyring.ErrKeyNotFound): + return keyringItem{}, errKeyringItemNotFound + case errors.Is(err, keyring.ErrMetadataNotSupported), errors.Is(err, keyring.ErrMetadataNeedsCredentials): + return keyringItem{}, errKeyringMetadataUnsupported + default: + return keyringItem{}, err + } + } + if md.Item == nil { + if md.ModificationTime.IsZero() { + return keyringItem{}, errKeyringMetadataUnsupported + } + return keyringItem{}, nil + } + return keyringItem{ + key: md.Key, + label: md.Label, + description: md.Description, + keychainNotTrustApplication: md.KeychainNotTrustApplication, + keychainNotSynchronizable: md.KeychainNotSynchronizable, + }, nil +} + func (b bytenessBackend) set(it keyringItem) error { return b.kr.Set(keyring.Item{ Key: it.key, diff --git a/credstore/osbackend_byteness_test.go b/credstore/osbackend_byteness_test.go index 5b36fbc..01e5a49 100644 --- a/credstore/osbackend_byteness_test.go +++ b/credstore/osbackend_byteness_test.go @@ -3,6 +3,7 @@ package credstore import ( + "errors" "testing" "time" @@ -10,18 +11,32 @@ import ( ) type captureBytenessKeyring struct { - setItem keyring.Item + setItem keyring.Item + getRet keyring.Item + getErr error + metadataRet keyring.Metadata + metadataErr error + getCalls, metadataCalls, setCalls int } func (c *captureBytenessKeyring) Get(string) (keyring.Item, error) { - return keyring.Item{}, keyring.ErrKeyNotFound + c.getCalls++ + if c.getErr != nil { + return keyring.Item{}, c.getErr + } + return c.getRet, nil } func (c *captureBytenessKeyring) GetMetadata(string) (keyring.Metadata, error) { - return keyring.Metadata{}, keyring.ErrMetadataNotSupported + c.metadataCalls++ + if c.metadataErr != nil { + return keyring.Metadata{}, c.metadataErr + } + return c.metadataRet, nil } func (c *captureBytenessKeyring) Set(item keyring.Item) error { + c.setCalls++ c.setItem = item return nil } @@ -63,6 +78,85 @@ func TestBytenessBackendSetPassesThroughMetadata(t *testing.T) { } } +func TestBytenessBackendMetadataUsesMetadataOnly(t *testing.T) { + kr := &captureBytenessKeyring{ + metadataRet: keyring.Metadata{Item: &keyring.Item{ + Key: "default/git_token", + Data: []byte("secret data must not be copied"), + Label: "label", + Description: "description", + KeychainNotTrustApplication: true, + KeychainNotSynchronizable: true, + }}, + } + be := bytenessBackend{kr: kr} + + it, err := be.metadata("default/git_token") + if err != nil { + t.Fatalf("metadata: %v", err) + } + if kr.metadataCalls != 1 { + t.Fatalf("GetMetadata calls = %d, want 1", kr.metadataCalls) + } + if kr.getCalls != 0 { + t.Fatalf("Get calls = %d, want 0", kr.getCalls) + } + if it.key != "default/git_token" || it.label != "label" || it.description != "description" { + t.Fatalf("metadata item = %+v", it) + } + if len(it.data) != 0 { + t.Fatalf("metadata copied secret data: %q", string(it.data)) + } + if !it.keychainNotTrustApplication { + t.Fatal("keychainNotTrustApplication = false, want true") + } + if !it.keychainNotSynchronizable { + t.Fatal("keychainNotSynchronizable = false, want true") + } +} + +func TestBytenessBackendEmptyMetadataFallsBackForExists(t *testing.T) { + kr := &captureBytenessKeyring{getErr: keyring.ErrKeyNotFound} + b := &osKeyringBackend{ + kr: bytenessBackend{kr: kr}, + backendKind: BackendPass, + } + + ok, err := b.exists("default/git_token") + if err != nil { + t.Fatalf("exists: %v", err) + } + if ok { + t.Fatal("exists = true, want false") + } + if kr.metadataCalls != 1 { + t.Fatalf("GetMetadata calls = %d, want 1", kr.metadataCalls) + } + if kr.getCalls != 1 { + t.Fatalf("Get calls = %d, want 1", kr.getCalls) + } +} + +func TestBytenessBackendMetadataErrorMapping(t *testing.T) { + for _, tc := range []struct { + name string + err error + want error + }{ + {"not found", keyring.ErrKeyNotFound, errKeyringItemNotFound}, + {"not supported", keyring.ErrMetadataNotSupported, errKeyringMetadataUnsupported}, + {"needs credentials", keyring.ErrMetadataNeedsCredentials, errKeyringMetadataUnsupported}, + } { + t.Run(tc.name, func(t *testing.T) { + be := bytenessBackend{kr: &captureBytenessKeyring{metadataErr: tc.err}} + _, err := be.metadata("default/git_token") + if !errors.Is(err, tc.want) { + t.Fatalf("metadata err = %v, want %v", err, tc.want) + } + }) + } +} + func TestKeyringConfigFromBackendConfigForwardsOnePasswordOptions(t *testing.T) { // #nosec G101 -- test fixture values are non-secret placeholders cfg := keyringConfigFromBackendConfig(backendConfig{ diff --git a/credstore/osbackend_core.go b/credstore/osbackend_core.go index d08827d..38b4888 100644 --- a/credstore/osbackend_core.go +++ b/credstore/osbackend_core.go @@ -17,6 +17,7 @@ import ( ) var errKeyringItemNotFound = errors.New("credstore: keyring item not found") +var errKeyringMetadataUnsupported = errors.New("credstore: keyring metadata unsupported") type promptFunc func(string) (string, error) @@ -31,6 +32,7 @@ type keyringItem struct { type keyringBackend interface { get(itemKey string) (keyringItem, error) + metadata(itemKey string) (keyringItem, error) set(keyringItem) error remove(itemKey string) error keys() ([]string, error) @@ -285,7 +287,13 @@ func (b *osKeyringBackend) delete(itemKey string) error { } func (b *osKeyringBackend) exists(itemKey string) (bool, error) { - if _, err := b.kr.get(itemKey); err != nil { + if _, err := b.kr.metadata(itemKey); err != nil { + if errors.Is(err, errKeyringMetadataUnsupported) { + _, err = b.kr.get(itemKey) + } + if err == nil { + return true, nil + } if errors.Is(err, errKeyringItemNotFound) { return false, nil } diff --git a/credstore/osbackend_test.go b/credstore/osbackend_test.go index 1534117..e2b5570 100644 --- a/credstore/osbackend_test.go +++ b/credstore/osbackend_test.go @@ -170,8 +170,8 @@ func TestFileBackendPassphraseFuncError(t *testing.T) { // used to exercise osKeyringBackend's error mapping/wrapping arms // deterministically without a real OS keyring. type fakeKeyring struct { - items map[string]keyringItem - getErr, setErr, delErr, keysErr error + items map[string]keyringItem + getErr, metadataErr, setErr, delErr, keysErr error } func newFakeKeyring() *fakeKeyring { return &fakeKeyring{items: map[string]keyringItem{}} } @@ -186,6 +186,17 @@ func (f *fakeKeyring) get(k string) (keyringItem, error) { } return it, nil } +func (f *fakeKeyring) metadata(k string) (keyringItem, error) { + if f.metadataErr != nil { + return keyringItem{}, f.metadataErr + } + it, ok := f.items[k] + if !ok { + return keyringItem{}, errKeyringItemNotFound + } + it.data = nil + return it, nil +} func (f *fakeKeyring) set(it keyringItem) error { if f.setErr != nil { return f.setErr @@ -248,7 +259,7 @@ func TestOSKeyringBackendErrorMapping(t *testing.T) { op string }{ {"get", func(b *osKeyringBackend) error { _, e := b.get("p/k"); return e }, func(f *fakeKeyring) { f.getErr = sentinel }, "get"}, - {"exists", func(b *osKeyringBackend) error { _, e := b.exists("p/k"); return e }, func(f *fakeKeyring) { f.getErr = sentinel }, "exists"}, + {"exists", func(b *osKeyringBackend) error { _, e := b.exists("p/k"); return e }, func(f *fakeKeyring) { f.metadataErr = sentinel }, "exists"}, {"set", func(b *osKeyringBackend) error { return b.set("p/k", "v", true) }, func(f *fakeKeyring) { f.setErr = sentinel }, "set"}, // !overwrite + a non-not-found Get error hits the pre-check // default arm (distinct from the overwrite=true write path above). @@ -274,6 +285,44 @@ func TestOSKeyringBackendErrorMapping(t *testing.T) { } } +func TestOSKeyringBackendExistsUsesMetadataWhenAvailable(t *testing.T) { + f := newFakeKeyring() + f.items["p/k"] = keyringItem{key: "p/k", data: []byte("secret"), label: "label", description: "desc"} + f.getErr = errors.New("get should not run when metadata succeeds") + b := &osKeyringBackend{kr: f, backendKind: BackendKeychain} + + ok, err := b.exists("p/k") + if err != nil { + t.Fatalf("exists: %v", err) + } + if !ok { + t.Fatal("exists = false, want true") + } +} + +func TestOSKeyringBackendExistsFallsBackToGetWhenMetadataUnsupported(t *testing.T) { + f := newFakeKeyring() + f.items["p/k"] = keyringItem{key: "p/k", data: []byte("secret")} + f.metadataErr = errKeyringMetadataUnsupported + b := &osKeyringBackend{kr: f, backendKind: BackendFile} + + ok, err := b.exists("p/k") + if err != nil { + t.Fatalf("exists: %v", err) + } + if !ok { + t.Fatal("exists = false, want true") + } + + ok, err = b.exists("p/missing") + if err != nil { + t.Fatalf("exists missing: %v", err) + } + if ok { + t.Fatal("exists missing = true, want false") + } +} + // TestOpenEnvSelectsFileBackend exercises the real // selectBackend→Open integration through os.Getenv (the file backend // runs in CI), proving env selection actually changes the constructed diff --git a/credstore/passbackend_nocgo_unix.go b/credstore/passbackend_nocgo_unix.go index be7a98b..be875e7 100644 --- a/credstore/passbackend_nocgo_unix.go +++ b/credstore/passbackend_nocgo_unix.go @@ -68,6 +68,13 @@ func (b *passKeyringBackend) get(itemKey string) (keyringItem, error) { return decodePassItemOutput(output) } +func (b *passKeyringBackend) metadata(itemKey string) (keyringItem, error) { + if err := b.checkItemExists(itemKey); err != nil { + return keyringItem{}, err + } + return keyringItem{key: itemKey}, nil +} + func (b *passKeyringBackend) set(it keyringItem) error { bytes, err := json.Marshal(persistedKeyringItem{Key: it.key, Data: it.data}) if err != nil { diff --git a/credstore/secretservice_core.go b/credstore/secretservice_core.go index 95d8eba..117c25f 100644 --- a/credstore/secretservice_core.go +++ b/credstore/secretservice_core.go @@ -80,6 +80,20 @@ func (b *secretServiceBackend) get(itemKey string) (keyringItem, error) { }) } +func (b *secretServiceBackend) metadata(itemKey string) (keyringItem, error) { + if err := b.openCollection(); err != nil { + return keyringItem{}, secretServiceNotFoundError(err) + } + items, err := b.collection.searchItems(itemKey) + if err != nil { + return keyringItem{}, err + } + if len(items) == 0 { + return keyringItem{}, errKeyringItemNotFound + } + return keyringItem{key: itemKey}, nil +} + func (b *secretServiceBackend) set(it keyringItem) error { if err := b.openSecrets(); err != nil { return err