Skip to content

Commit a5c2b1a

Browse files
authored
fix(secret): treat ErrNotFound on keyring delete as already deleted (#1758)
## Bug `KeyringProvider.Delete` treats every error from `keyring.Delete` as a keyring failure, including `keyring.ErrNotFound`. When the entry was already removed outside mcpproxy (Keychain Access, `security delete-generic-password`, Seahorse, Credential Manager), it returns before `removeFromRegistry`, so the name stays in the registry and `List` keeps showing it. Every later delete hits the same error. It also calls `markUnavailable()`, so for the rest of the process every keyring `Store`/`Delete` short-circuits with `ErrKeyringUnavailable`, even though the keyring just answered fine. ## Trigger 1. Store a secret `foo` through the UI or `mcpproxy secrets set`. 2. Delete the `mcpproxy`/`foo` item in Keychain Access. 3. Call `DELETE /api/v1/secrets/foo`. It returns `500 failed to delete secret foo from keyring: secret not found in keyring` on every retry, `foo` stays listed, and further secret writes fail as "keyring unavailable". ## Fix Treat `keyring.ErrNotFound` from the delete as success: the keyring answered and the entry is already gone. So the provider is not marked unavailable and execution continues to `removeFromRegistry`. Other errors behave as before. `IsAvailable` already uses the same convention, where `ErrNotFound` means "available, no secret". ## Test `TestKeyringProvider_Delete_AlreadyGoneCleansRegistry` uses the existing `withKeyringMocks` seam, with `Delete` returning `ErrNotFound`. It asserts that `Delete` returns nil, the registry drops `foo`, `List` returns only `bar`, and the provider is not marked unavailable. ## Commands run - `CGO_ENABLED=0 go test -count=1 ./internal/secret`: ok - With the fix reverted (`git stash` of `keyring_provider.go`), `-run AlreadyGone` fails with `Delete of an already-removed secret: failed to delete secret foo from keyring: secret not found in keyring`. - `gofmt -l` on the edited files: no output
1 parent 1f9911d commit a5c2b1a

2 files changed

Lines changed: 49 additions & 1 deletion

File tree

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
package secret
2+
3+
import (
4+
"context"
5+
"testing"
6+
7+
"github.com/zalando/go-keyring"
8+
)
9+
10+
// A secret removed outside mcpproxy (Keychain Access, `security
11+
// delete-generic-password`) makes keyring.Delete return ErrNotFound. That must
12+
// still clean the registry and must not mark the whole keyring unavailable.
13+
func TestKeyringProvider_Delete_AlreadyGoneCleansRegistry(t *testing.T) {
14+
clearHeadlessEnv(t)
15+
registry := "foo\nbar"
16+
withKeyringMocks(t,
17+
func(_, user string) (string, error) {
18+
if user == RegistryKey {
19+
return registry, nil
20+
}
21+
return "", keyring.ErrNotFound
22+
},
23+
func(_, user, password string) error {
24+
if user == RegistryKey {
25+
registry = password
26+
}
27+
return nil
28+
},
29+
func(_, _ string) error { return keyring.ErrNotFound },
30+
)
31+
32+
p := NewKeyringProvider()
33+
if err := p.Delete(context.Background(), Ref{Type: SecretTypeKeyring, Name: "foo"}); err != nil {
34+
t.Fatalf("Delete of an already-removed secret: %v", err)
35+
}
36+
if registry != "bar" {
37+
t.Errorf("registry = %q, want %q", registry, "bar")
38+
}
39+
if p.isKnownUnavailable() {
40+
t.Error("keyring marked unavailable after ErrNotFound on delete")
41+
}
42+
refs, err := p.List(context.Background())
43+
if err != nil || len(refs) != 1 || refs[0].Name != "bar" {
44+
t.Errorf("List = %v, %v; want only bar", refs, err)
45+
}
46+
}

‎internal/secret/keyring_provider.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,9 @@ func (p *KeyringProvider) Delete(_ context.Context, ref Ref) error {
280280
err := runWithTimeout(func() error {
281281
return keyringDelFn(p.serviceName, ref.Name)
282282
})
283-
if err != nil {
283+
// ErrNotFound means the keyring answered and the entry is already gone
284+
// (e.g. removed in Keychain Access); the registry still needs cleaning.
285+
if err != nil && !errors.Is(err, keyring.ErrNotFound) {
284286
p.markUnavailable()
285287
return fmt.Errorf("failed to delete secret %s from keyring: %w", ref.Name, err)
286288
}

0 commit comments

Comments
 (0)