fix(secrets): reject placeholder/reference values before they reach the store

cmd/commands/secrets_core_test.go

@@ -108,7 +108,7 @@ func TestSecretsList_ReflectsSetSecrets(t *testing.T) {
 			t.Fatalf("expected 0 secrets in a fresh store, got %d", len(keysBefore))
 		}
 
-		if err := secretsSetCmd.RunE(secretsSetCmd, []string{"JWT_SIGNING_KEY", "abc123"}); err != nil {
+		if err := secretsSetCmd.RunE(secretsSetCmd, []string{"JWT_SIGNING_KEY", "abc123def456ghi789jkl012"}); err != nil {
 			t.Fatalf("set: %v", err)
 		}
 

cmd/commands/secrets_edit_rotate_test.go

@@ -31,29 +31,63 @@ import (
 )
 
 // TestSecretsRotate_InvalidatesOldValue verifies plain (non-dual-window)
-// rotation actually replaces the value under the original key.
+// rotation actually replaces the value under the original key, for a
+// secret type rotate CAN auto-generate a replacement for (a password).
 func TestSecretsRotate_InvalidatesOldValue(t *testing.T) {
 	requireAge(t)
 	withProjectRoot(t, func(root string) {
 		initSecretsProject(t, root)
 		secretsEnvFlag = "dev"
 		defer func() { secretsEnvFlag = "dev" }()
 
-		if err := secretsSetCmd.RunE(secretsSetCmd, []string{"API_TOKEN", "old-compromised-value"}); err != nil {
+		if err := secretsSetCmd.RunE(secretsSetCmd, []string{"SERVICE_PASSWORD", "old-compromised-value-v1"}); err != nil {
 			t.Fatalf("set: %v", err)
 		}
 
 		_ = secretsRotateCmd.Flags().Set("dual-window", "false")
-		if err := secretsRotateCmd.RunE(secretsRotateCmd, []string{"API_TOKEN"}); err != nil {
+		if err := secretsRotateCmd.RunE(secretsRotateCmd, []string{"SERVICE_PASSWORD"}); err != nil {
 			t.Fatalf("rotate: %v", err)
 		}
 
-		newValue, err := secrets.Get(root, "dev", "API_TOKEN")
+		newValue, err := secrets.Get(root, "dev", "SERVICE_PASSWORD")
 		if err != nil {
 			t.Fatalf("get after rotate: %v", err)
 		}
-		if newValue == "old-compromised-value" {
-			t.Fatal("API_TOKEN still holds the pre-rotation value — rotation did not invalidate it")
+		if newValue == "old-compromised-value-v1" {
+			t.Fatal("SERVICE_PASSWORD still holds the pre-rotation value — rotation did not invalidate it")
+		}
+	})
+}
+
+// TestSecretsRotate_ManualRotationType_PreservesOldValueOnError verifies the
+// fix for the 2026-09-11/12 incident class: a secret type rotate cannot
+// auto-generate a replacement for (API keys/tokens, which must be rotated
+// through the provider's dashboard) must error out AND leave the existing
+// value completely untouched — never silently overwrite it with an empty
+// string, which is exactly as destructive as the incident's "${VAR}"
+// reference collapsing to empty on capture.
+func TestSecretsRotate_ManualRotationType_PreservesOldValueOnError(t *testing.T) {
+	requireAge(t)
+	withProjectRoot(t, func(root string) {
+		initSecretsProject(t, root)
+		secretsEnvFlag = "dev"
+		defer func() { secretsEnvFlag = "dev" }()
+
+		if err := secretsSetCmd.RunE(secretsSetCmd, []string{"API_TOKEN", "old-compromised-value-v1"}); err != nil {
+			t.Fatalf("set: %v", err)
+		}
+
+		_ = secretsRotateCmd.Flags().Set("dual-window", "false")
+		if err := secretsRotateCmd.RunE(secretsRotateCmd, []string{"API_TOKEN"}); err == nil {
+			t.Fatal("expected rotate on a manual-rotation-only key (API_TOKEN) to error, got nil")
+		}
+
+		stillThere, err := secrets.Get(root, "dev", "API_TOKEN")
+		if err != nil {
+			t.Fatalf("get after failed rotate: %v", err)
+		}
+		if stillThere != "old-compromised-value-v1" {
+			t.Fatalf("API_TOKEN value changed after a failed rotate — got %q, want the untouched original", stillThere)
 		}
 	})
 }
@@ -69,7 +103,7 @@ func TestSecretsRotateDualWindow_OldValueOnlyReachableViaPrevious(t *testing.T)
 		secretsEnvFlag = "dev"
 		defer func() { secretsEnvFlag = "dev" }()
 
-		if err := secretsSetCmd.RunE(secretsSetCmd, []string{"SESSION_SECRET", "old-value-v1"}); err != nil {
+		if err := secretsSetCmd.RunE(secretsSetCmd, []string{"SESSION_SECRET", "old-session-secret-original-v1"}); err != nil {
 			t.Fatalf("set: %v", err)
 		}
 
@@ -83,16 +117,16 @@ func TestSecretsRotateDualWindow_OldValueOnlyReachableViaPrevious(t *testing.T)
 		if err != nil {
 			t.Fatalf("get base after dual-window rotate: %v", err)
 		}
-		if base == "old-value-v1" {
+		if base == "old-session-secret-original-v1" {
 			t.Fatal("base key still returns the OLD value after dual-window rotate — new value never took effect")
 		}
 
 		prev, err := secrets.Get(root, "dev", "SESSION_SECRET_PREVIOUS")
 		if err != nil {
 			t.Fatalf("get _PREVIOUS: %v", err)
 		}
-		if prev != "old-value-v1" {
-			t.Errorf("SESSION_SECRET_PREVIOUS = %q, want the pre-rotation value %q", prev, "old-value-v1")
+		if prev != "old-session-secret-original-v1" {
+			t.Errorf("SESSION_SECRET_PREVIOUS = %q, want the pre-rotation value %q", prev, "old-session-secret-original-v1")
 		}
 
 		// Retire the old key window: _PREVIOUS must become entirely unreachable.

internal/secrets/secrets_store.go

@@ -101,8 +101,15 @@ func saveStore(projectRoot, env string, store *SecretStore) error {
 	return nil
 }
 
-// Set adds or updates a secret.
+// Set adds or updates a secret. The value is validated before it ever
+// touches the store — see ValidateSecretValue for the exact rules (empty
+// values, unexpanded shell references, bracketed placeholder tokens, and
+// implausibly short values are all rejected).
 func Set(projectRoot, env, key, value string) error {
+	if err := ValidateSecretValue(key, value); err != nil {
+		slog.Error("secrets_set_rejected", "key_name", key, "env", env, "error", err)
+		return err
+	}
 	store, err := loadStore(projectRoot, env)
 	if err != nil {
 		return err
@@ -162,6 +169,15 @@ func Rotate(projectRoot, env, key string) (string, error) {
 	}
 
 	newValue, hint := generateRotationValue(key)
+	if newValue == "" {
+		// generateRotationValue declines to auto-generate for API keys/
+		// tokens that must be rotated through a provider dashboard. Do NOT
+		// persist that empty value over the existing entry — that would
+		// silently blank a working credential, exactly the failure mode
+		// this whole validation layer exists to prevent. Leave the stored
+		// value untouched and tell the caller what to do instead.
+		return "", fmt.Errorf("secret %q requires manual rotation: %s (existing value left unchanged; use 'nself secrets set %s <new-value>')", key, hint, key)
+	}
 	if hint != "" {
 		slog.Info("rotation hint", "note", hint)
 	}

internal/secrets/secrets_validate.go

@@ -0,0 +1,81 @@
+package secrets
+
+// secrets_validate.go — refuses to store a value that is not a literal
+// secret before it ever reaches the encrypted store.
+//
+// Purpose: close the exact gap behind the 2026-09-11/12 production incident,
+// where an operator captured `grep | sed` output into a vault file and
+// assumed the right-hand side of every `KEY=` line was a literal value. It
+// often is not: `STRIPE_NSELF_SECRET_KEY=<set in .env.secrets>` is a
+// placeholder that reads as valid shell but is not a secret, and
+// `AUTH_JWT_SECRET=${PROD...}` is an unexpanded shell variable reference
+// that resolves to empty at the point it was captured. Both were written
+// over a working credential; the second also broke every line `source`
+// read after it. ValidateSecretValue is the fail-closed gate: called from
+// Set (and therefore from the `edit` re-save path, which calls Set for
+// every parsed line) so nothing reaches saveStore without passing it.
+// Inputs: the secret's key name and the candidate value, exactly as
+// supplied to `nself secrets set` / `nself secrets edit`.
+// Outputs: nil when the value is acceptable; a descriptive error naming
+// the specific rule that failed otherwise. Never logs or wraps the value
+// itself into the error message.
+// Constraints: pure, no I/O, no external calls — safe to unit test without
+// the `age` binary. Deliberately independent of, and stricter than,
+// internal/config's placeholder-secrets validator (which only screens a
+// small known-placeholder substring list at config-load time for
+// non-dev environments); this one runs unconditionally for every
+// environment because a corrupted vault/backup copy is exactly as
+// dangerous in dev as it is in prod.
+import (
+	"fmt"
+	"regexp"
+	"strings"
+)
+
+// referenceLikePattern matches a value that is entirely a bracketed
+// placeholder token, e.g. "<set in .env.secrets>" or "<CHANGE ME>".
+var referenceLikePattern = regexp.MustCompile(`^<.*>$`)
+
+// minLengthByKeySuffix mirrors the type classification generateRotationValue
+// already uses for rotation, reused here as a floor: a value shorter than
+// this for its key's shape is implausible for that secret type (e.g. an
+// 8-char "JWT secret" is not a rotated JWT signing key, it is someone's
+// typo or a truncated paste).
+func minLengthByKeySuffix(key string) int {
+	keyUpper := strings.ToUpper(key)
+	switch {
+	case strings.HasSuffix(keyUpper, "_PASSWORD") || strings.HasSuffix(keyUpper, "_PASS"):
+		return 12
+	case strings.Contains(keyUpper, "JWT") || strings.Contains(keyUpper, "SECRET"):
+		return 24
+	case strings.Contains(keyUpper, "API_KEY") || strings.Contains(keyUpper, "TOKEN") || strings.Contains(keyUpper, "_KEY"):
+		return 12
+	default:
+		return 6
+	}
+}
+
+// ValidateSecretValue rejects a candidate secret value that cannot be a
+// literal, working credential. It never returns the value in its error —
+// callers must not either.
+func ValidateSecretValue(key, value string) error {
+	if strings.TrimSpace(value) == "" {
+		return fmt.Errorf("secret %q: value is empty — refusing to store (this would silently blank a working credential)", key)
+	}
+	if strings.Contains(value, "${") {
+		return fmt.Errorf("secret %q: value contains an unexpanded shell variable reference (\"${...}\") — capture the literal value, not the reference", key)
+	}
+	if strings.Contains(value, "$(") {
+		return fmt.Errorf("secret %q: value contains an unexpanded shell command substitution (\"$(...)\") — capture the literal value, not the expression", key)
+	}
+	if strings.Contains(value, "<") || strings.Contains(value, ">") {
+		return fmt.Errorf("secret %q: value contains \"<\" or \">\" — this looks like a placeholder token (e.g. \"<set in .env.secrets>\"), not a literal secret", key)
+	}
+	if referenceLikePattern.MatchString(strings.TrimSpace(value)) {
+		return fmt.Errorf("secret %q: value is entirely a bracketed placeholder token — not a literal secret", key)
+	}
+	if min := minLengthByKeySuffix(key); len(value) < min {
+		return fmt.Errorf("secret %q: value is only %d characters, implausibly short for this secret type (expected at least %d) — refusing to store", key, len(value), min)
+	}
+	return nil
+}

internal/secrets/secrets_validate_test.go

@@ -0,0 +1,68 @@
+package secrets
+
+// secrets_validate_test.go covers ValidateSecretValue against the exact
+// incident shapes it exists to catch (2026-09-11/12 vault corruption:
+// a literal "<set in .env.secrets>" placeholder and an unexpanded
+// "${PROD...}" shell reference), plus the general empty/short-value cases.
+
+import (
+	"strings"
+	"testing"
+)
+
+func TestValidateSecretValue_RejectsIncidentShapes(t *testing.T) {
+	cases := []struct {
+		name  string
+		key   string
+		value string
+	}{
+		{"literal placeholder token", "STRIPE_NSELF_SECRET_KEY", "<set in .env.secrets>"},
+		{"unexpanded shell var reference", "AUTH_JWT_SECRET", "${PROD_AUTH_JWT_SECRET}"},
+		{"command substitution", "POSTGRES_PASSWORD", "$(cat /run/secrets/pg)"},
+		{"empty value", "HASURA_GRAPHQL_ADMIN_SECRET", ""},
+		{"whitespace-only value", "HASURA_GRAPHQL_ADMIN_SECRET", "   "},
+		{"bare angle bracket prefix", "REDIS_PASSWORD", "<partial"},
+		{"implausibly short jwt secret", "AUTH_JWT_SECRET", "abc123"},
+		{"implausibly short password", "POSTGRES_PASSWORD", "abc"},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			if err := ValidateSecretValue(c.key, c.value); err == nil {
+				t.Errorf("ValidateSecretValue(%q, %q) = nil, want rejection", c.key, c.value)
+			}
+		})
+	}
+}
+
+func TestValidateSecretValue_AcceptsPlausibleValues(t *testing.T) {
+	cases := []struct {
+		name  string
+		key   string
+		value string
+	}{
+		{"long random jwt secret", "AUTH_JWT_SECRET", "kR8f3nQ2vL9wZ4xM7pT1bY6cJ0hD5sA3eU8gN2rK4vW7qX1z"},
+		{"reasonable password", "POSTGRES_PASSWORD", "Tr0ub4dor&3xamplePW"},
+		{"reasonable api key", "STRIPE_NSELF_SECRET_KEY", "not-a-real-key-fixture-51AbCdEfGhIjKlMnOpQrStUv"},
+		{"generic short-but-allowed value", "NODE_ENV", "production"},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			if err := ValidateSecretValue(c.key, c.value); err != nil {
+				t.Errorf("ValidateSecretValue(%q, %q) = %v, want nil", c.key, c.value, err)
+			}
+		})
+	}
+}
+
+func TestValidateSecretValue_ErrorNeverEchoesTheRejectedValue(t *testing.T) {
+	// The error message must name the rule, never the offending value —
+	// a secret that fails validation must not leak into logs/output either.
+	secretValue := "${SOME_SUPER_SECRET_REFERENCE_VALUE}"
+	err := ValidateSecretValue("AUTH_JWT_SECRET", secretValue)
+	if err == nil {
+		t.Fatal("expected rejection")
+	}
+	if got := err.Error(); strings.Contains(got, secretValue) {
+		t.Errorf("error message echoed the rejected value: %q", got)
+	}
+}