fix(secrets): prevent duplicate DO Spaces key creation

cmd/wfctl/infra_bootstrap.go

@@ -713,19 +713,43 @@ var bootstrapSecrets = func(ctx context.Context, provider secrets.Provider, cfg
 					allowed[k] = true
 				}
 
-				// Capture access_key + created_at independently of storage so
-				// RotationResult always reports them on a force-rotate, even
-				// though access_key IS in the allowed set and created_at is
-				// NOT (per review #2 C5: stderr emission alone would skip
-				// access_key).
-				var newAccessKey, newCreatedAt string
-				for subKey, subVal := range subKeyMap {
-					if subKey == "access_key" {
-						newAccessKey = subVal
+				// Capture access_key + created_at BEFORE any Set call so the
+				// transactional rollback path has the upstream credential ID
+				// available even when the very first Set fails. Bug fix:
+				// previously these were extracted DURING iteration, so a
+				// Set("foo_access_key") failure left newAccessKey unset and
+				// the orphaned DO key invisible to the rollback path.
+				newAccessKey := subKeyMap["access_key"]
+				newCreatedAt := subKeyMap["created_at"]
+
+				// Transactional rollback: if ANY Set() inside the loop fails,
+				// revoke the just-created upstream credential before
+				// returning. The DO Spaces Keys API does NOT enforce name
+				// uniqueness, so a partially-applied generation without
+				// rollback leaves an orphaned key whose secret_key is
+				// permanently irrecoverable (see workflow#732 / SPEC V?).
+				//
+				// Rollback is best-effort — failure is logged but does not
+				// mask the original Set error. Operator can still find the
+				// orphan via the wfctl secrets list-orphans helper.
+				revokeOrphan := func(reason error) {
+					if newAccessKey == "" {
+						return
+					}
+					if credRevoker == nil {
+						fmt.Fprintf(os.Stderr, "warn: provider_credential %q minted access_key=%s but no revoker available; the upstream credential is now ORPHANED and unrecoverable. Run `wfctl secrets list-orphans --source %s --name %s` to clean up. Original error: %v\n",
+							gen.Key, newAccessKey, gen.Source, gen.Key, reason)
+						return
 					}
-					if subKey == "created_at" {
-						newCreatedAt = subVal
+					if revokeErr := credRevoker.RevokeProviderCredential(ctx, gen.Source, newAccessKey); revokeErr != nil {
+						fmt.Fprintf(os.Stderr, "warn: rollback-revoke just-minted credential %q (access_key=%s) failed: %v — manual cleanup required via `wfctl secrets list-orphans --source %s`\n",
+							gen.Key, newAccessKey, revokeErr, gen.Source)
+						return
 					}
+					fmt.Fprintf(os.Stderr, "wfctl: rolled back orphaned credential %q (access_key=%s) after Set failure\n", gen.Key, newAccessKey)
+				}
+
+				for subKey, subVal := range subKeyMap {
 					if !allowed[subKey] {
 						// Sidecar field — emit to stderr in machine-parseable form
 						// for operator observability + audit logs.
@@ -734,6 +758,7 @@ var bootstrapSecrets = func(ctx context.Context, provider secrets.Provider, cfg
 					}
 					fullKey := gen.Key + "_" + subKey
 					if setErr := provider.Set(ctx, fullKey, subVal); setErr != nil {
+						revokeOrphan(setErr)
 						return generated, rotations, fmt.Errorf("store secret %q: %w", fullKey, setErr)
 					}
 					generated[fullKey] = subVal
@@ -743,6 +768,7 @@ var bootstrapSecrets = func(ctx context.Context, provider secrets.Provider, cfg
 						fmt.Printf("  secret %q: created\n", fullKey)
 					}
 				}
+				_ = newCreatedAt // captured into RotationResult below
 				if forceRotate[gen.Key] {
 					fmt.Fprintf(os.Stderr, "wfctl: rotated provider_credential %s (replaced existing value at %s)\n", gen.Key, time.Now().UTC().Format(time.RFC3339))
 					// Record the rotation in-process so callers (rotate-and-prune)

cmd/wfctl/infra_bootstrap_secrets_test.go

@@ -265,3 +265,125 @@ func TestBootstrapSecrets_ProviderCredentialProbeIgnoresBareKey(t *testing.T) {
 		t.Fatalf("generator called %d times, want 1", generateCalls)
 	}
 }
+
+// failOnSetProvider triggers a Set() failure on the first matching key
+// — used to exercise the transactional rollback path when a
+// provider_credential creation succeeds upstream but the store-write
+// half fails (e.g. GH PAT lost permissions mid-bootstrap).
+type failOnSetProvider struct {
+	failKey  string
+	setCalls []string
+}
+
+func (p *failOnSetProvider) Name() string { return "fail-on-set" }
+func (p *failOnSetProvider) Get(_ context.Context, _ string) (string, error) {
+	return "", secrets.ErrUnsupported
+}
+func (p *failOnSetProvider) Set(_ context.Context, key, _ string) error {
+	p.setCalls = append(p.setCalls, key)
+	if key == p.failKey {
+		return errFakeStoreUnavailable
+	}
+	return nil
+}
+func (p *failOnSetProvider) Delete(_ context.Context, _ string) error { return nil }
+func (p *failOnSetProvider) List(_ context.Context) ([]string, error) {
+	return nil, secrets.ErrUnsupported
+}
+
+// recordingRevoker captures RevokeProviderCredential calls so the test
+// can assert rollback occurred. Implements interfaces.ProviderCredentialRevoker.
+type recordingRevoker struct {
+	calls []revokeCall
+}
+type revokeCall struct {
+	source       string
+	credentialID string
+}
+
+func (r *recordingRevoker) RevokeProviderCredential(_ context.Context, source, credentialID string) error {
+	r.calls = append(r.calls, revokeCall{source: source, credentialID: credentialID})
+	return nil
+}
+
+var errFakeStoreUnavailable = errFakeStore("store unavailable (simulated)")
+
+type errFakeStore string
+
+func (e errFakeStore) Error() string { return string(e) }
+
+// TestBootstrapSecrets_ProviderCredential_RollbackOnSetFailure is the
+// regression test for the orphan-key bug: when generateSecret returns a
+// fresh DO Spaces key but provider.Set fails to persist it, bootstrap
+// MUST revoke the just-minted upstream credential.
+//
+// Pre-fix behaviour: Set fails → return error → DO key remains in the
+// account with an unrecoverable secret_key. Every subsequent run mints
+// another orphan with the same name.
+//
+// Post-fix: Set failure triggers credRevoker.RevokeProviderCredential
+// with the access_key from the just-generated subKeyMap.
+func TestBootstrapSecrets_ProviderCredential_RollbackOnSetFailure(t *testing.T) {
+	withStubGenerator(t, func(_ context.Context, _ string, _ map[string]any) (string, error) {
+		out, _ := json.Marshal(map[string]string{
+			"access_key": "AK_ORPHAN",
+			"secret_key": "SK_DOOMED",
+		})
+		return string(out), nil
+	})
+
+	p := &failOnSetProvider{failKey: "SPACES_secret_key"}
+	rev := &recordingRevoker{}
+	cfg := &SecretsConfig{
+		Generate: []SecretGen{
+			{Key: "SPACES", Type: "provider_credential", Source: "digitalocean.spaces"},
+		},
+	}
+
+	_, _, err := bootstrapSecrets(context.Background(), p, cfg, nil, rev)
+	if err == nil {
+		t.Fatal("expected Set failure to surface as error")
+	}
+
+	if len(rev.calls) != 1 {
+		t.Fatalf("expected 1 rollback-revoke call; got %d", len(rev.calls))
+	}
+	if rev.calls[0].credentialID != "AK_ORPHAN" {
+		t.Errorf("rollback called with credentialID=%q want AK_ORPHAN", rev.calls[0].credentialID)
+	}
+	if rev.calls[0].source != "digitalocean.spaces" {
+		t.Errorf("rollback source=%q want digitalocean.spaces", rev.calls[0].source)
+	}
+}
+
+// TestBootstrapSecrets_ProviderCredential_RollbackOnFirstSetFailure
+// guards the most insidious shape of the bug: the very first Set call
+// fails (e.g. access_key write). The pre-fix code extracted
+// newAccessKey *during* the for-range loop, so a first-iteration
+// failure left newAccessKey empty even though the upstream key exists.
+// The fix extracts access_key BEFORE the loop.
+func TestBootstrapSecrets_ProviderCredential_RollbackOnFirstSetFailure(t *testing.T) {
+	withStubGenerator(t, func(_ context.Context, _ string, _ map[string]any) (string, error) {
+		out, _ := json.Marshal(map[string]string{
+			"access_key": "AK_FIRST",
+			"secret_key": "SK_FIRST",
+		})
+		return string(out), nil
+	})
+
+	p := &failOnSetProvider{failKey: "SPACES_access_key"}
+	rev := &recordingRevoker{}
+	cfg := &SecretsConfig{
+		Generate: []SecretGen{
+			{Key: "SPACES", Type: "provider_credential", Source: "digitalocean.spaces"},
+		},
+	}
+
+	_, _, err := bootstrapSecrets(context.Background(), p, cfg, nil, rev)
+	if err == nil {
+		t.Fatal("expected Set failure")
+	}
+	if len(rev.calls) != 1 || rev.calls[0].credentialID != "AK_FIRST" {
+		t.Errorf("rollback calls = %v; want one revoke of AK_FIRST", rev.calls)
+	}
+}

cmd/wfctl/plugin_marketplace_verify.go

@@ -85,6 +85,11 @@ Options:
 
 // ghAPICmd is the indirection point so tests can inject a fake gh binary.
 // Default is the real `gh api` CLI.
+//
+// #nosec G204 -- the binary is the fixed string "gh" and `endpoint` is
+// constructed from a literal-prefix + URL-escaped query inside this
+// package (urlQueryEscape sanitises). No user-controlled shell metachars
+// flow into the subprocess.
 var ghAPICmd = func(ctx context.Context, endpoint string) ([]byte, error) {
 	cmd := exec.CommandContext(ctx, "gh", "api", endpoint)
 	return cmd.Output()

cmd/wfctl/secrets.go

@@ -32,6 +32,8 @@ func runSecrets(args []string) error {
 		return runSecretsSync(args[1:])
 	case "setup":
 		return runSecretsSetup(args[1:])
+	case "list-orphans":
+		return runSecretsListOrphans(args[1:])
 	default:
 		return secretsUsage()
 	}
@@ -54,6 +56,7 @@ Actions:
   rotate    Trigger rotation of a secret
   sync      Copy secret structure between environments
   setup     Interactively set all secrets for an environment
+  list-orphans  List/delete duplicate-named upstream credentials (e.g. DO Spaces keys)
 
 Ad-hoc provider flags (set, get, list, delete, export):
   --provider  Override provider: keychain, env, aws
@@ -75,6 +78,8 @@ Examples:
   wfctl secrets sync --from staging --to production
   wfctl secrets setup --env local
   wfctl secrets setup --env production --auto-gen-keys
+  wfctl secrets list-orphans --source digitalocean.spaces --name workflow-spaces-key
+  wfctl secrets list-orphans --source digitalocean.spaces --name workflow-spaces-key --delete
 `)
 	return fmt.Errorf("missing or unknown action")
 }