diff --git a/cmd/commands/secrets_core_test.go b/cmd/commands/secrets_core_test.go index 0230d2b0..4738226d 100644 --- a/cmd/commands/secrets_core_test.go +++ b/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) } diff --git a/cmd/commands/secrets_edit_rotate_test.go b/cmd/commands/secrets_edit_rotate_test.go index d400a70c..f18537d4 100644 --- a/cmd/commands/secrets_edit_rotate_test.go +++ b/cmd/commands/secrets_edit_rotate_test.go @@ -31,7 +31,8 @@ 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) { @@ -39,21 +40,54 @@ func TestSecretsRotate_InvalidatesOldValue(t *testing.T) { 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,7 +117,7 @@ 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") } @@ -91,8 +125,8 @@ func TestSecretsRotateDualWindow_OldValueOnlyReachableViaPrevious(t *testing.T) 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. diff --git a/internal/secrets/secrets_store.go b/internal/secrets/secrets_store.go index e808e0e5..dcdd78fb 100644 --- a/internal/secrets/secrets_store.go +++ b/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 ')", key, hint, key) + } if hint != "" { slog.Info("rotation hint", "note", hint) } diff --git a/internal/secrets/secrets_validate.go b/internal/secrets/secrets_validate.go new file mode 100644 index 00000000..15b499ef --- /dev/null +++ b/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=` 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. "" or "". +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. \"\"), 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 +} diff --git a/internal/secrets/secrets_validate_test.go b/internal/secrets/secrets_validate_test.go new file mode 100644 index 00000000..863787fb --- /dev/null +++ b/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 "" 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", ""}, + {"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", "