Fix edge cases, improve validation, and clean up orchestrator, config, and CLI workflows

cmd/proxsave/install.go

@@ -838,12 +838,6 @@ func isInstallAbortedError(err error) bool {
 	return false
 }
 
-// clearImmutableAttributes attempts to remove immutable flags (chattr -i) so deletion can proceed.
-// It logs warnings on failure but does not return an error, since removal will report issues later.
-func clearImmutableAttributes(target string, bootstrap *logging.BootstrapLogger) {
-	_ = clearImmutableAttributesWithContext(context.Background(), target, bootstrap)
-}
-
 func clearImmutableAttributesWithContext(ctx context.Context, target string, bootstrap *logging.BootstrapLogger) error {
 	if ctx == nil {
 		ctx = context.Background()

cmd/proxsave/install_existing_config.go

@@ -26,9 +26,16 @@ type existingConfigDecision struct {
 }
 
 func promptExistingConfigModeCLI(ctx context.Context, reader *bufio.Reader, configPath string) (existingConfigMode, error) {
+	if ctx == nil {
+		ctx = context.Background()
+	}
+
 	info, err := os.Stat(configPath)
 	if err != nil {
 		if os.IsNotExist(err) {
+			if err := ctx.Err(); err != nil {
+				return existingConfigCancel, err
+			}
 			return existingConfigOverwrite, nil
 		}
 		return existingConfigCancel, fmt.Errorf("failed to access configuration file: %w", err)
@@ -53,12 +60,24 @@ func promptExistingConfigModeCLI(ctx context.Context, reader *bufio.Reader, conf
 		case "":
 			fallthrough
 		case "3":
+			if err := ctx.Err(); err != nil {
+				return existingConfigCancel, err
+			}
 			return existingConfigKeepContinue, nil
 		case "1":
+			if err := ctx.Err(); err != nil {
+				return existingConfigCancel, err
+			}
 			return existingConfigOverwrite, nil
 		case "2":
+			if err := ctx.Err(); err != nil {
+				return existingConfigCancel, err
+			}
 			return existingConfigEdit, nil
 		case "0":
+			if err := ctx.Err(); err != nil {
+				return existingConfigCancel, err
+			}
 			return existingConfigCancel, nil
 		default:
 			fmt.Println("Please enter 1, 2, 3 or 0.")

cmd/proxsave/install_existing_config_test.go

@@ -21,6 +21,20 @@ func TestPromptExistingConfigModeCLIMissingFileDefaultsToOverwrite(t *testing.T)
 	}
 }
 
+func TestPromptExistingConfigModeCLIMissingFileRespectsCanceledContext(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel()
+
+	missing := filepath.Join(t.TempDir(), "missing.env")
+	mode, err := promptExistingConfigModeCLI(ctx, bufio.NewReader(strings.NewReader("")), missing)
+	if !errors.Is(err, context.Canceled) {
+		t.Fatalf("expected context canceled error, got %v", err)
+	}
+	if mode != existingConfigCancel {
+		t.Fatalf("expected cancel mode, got %v", mode)
+	}
+}
+
 func TestPromptExistingConfigModeCLIOptions(t *testing.T) {
 	cfgFile := createTempFile(t, "EXISTING=1\n")
 	tests := []struct {

cmd/proxsave/new_install.go

@@ -56,6 +56,7 @@ func formatNewInstallPreservedEntries(entries []string) string {
 	formatted := make([]string, 0, len(entries))
 	for _, entry := range entries {
 		trimmed := strings.TrimSpace(entry)
+		trimmed = strings.TrimRight(trimmed, "/")
 		if trimmed == "" {
 			continue
 		}

cmd/proxsave/new_install_test.go

@@ -120,6 +120,11 @@ func TestFormatNewInstallPreservedEntries(t *testing.T) {
 			entries: []string{"", " ", "\t"},
 			want:    "(none)",
 		},
+		{
+			name:    "normalizes trailing slashes",
+			entries: []string{"env/", "build//", " identity/// ", "/"},
+			want:    "env/ build/ identity/",
+		},
 	}
 
 	for _, tt := range tests {

docs/CONFIGURATION.md

@@ -846,6 +846,7 @@ If `EMAIL_ENABLED` is omitted, the default remains `false`. The legacy alias `EM
 - Allowed values for `EMAIL_DELIVERY_METHOD` are: `relay`, `sendmail`, `pmf` (invalid values will skip Email with a warning).
 - `EMAIL_FALLBACK_SENDMAIL` is a historical name (kept for compatibility). When `EMAIL_DELIVERY_METHOD=relay`, it enables fallback to **pmf** (it will not fall back to `/usr/sbin/sendmail`).
 - `relay` requires a real mailbox recipient and blocks `root@…` recipients; set `EMAIL_RECIPIENT` to a non-root mailbox if needed.
+- When logs say the relay "accepted request", it means the worker and upstream email API accepted the submission. It does **not** guarantee final inbox delivery (the message may still bounce, be deferred, or land in spam later).
 - If `EMAIL_RECIPIENT` is empty, ProxSave auto-detects the recipient from the `root@pam` user:
   - **PVE**: Proxmox API via `pvesh get /access/users/root@pam` → fallback to `pveum user list` → fallback to `/etc/pve/user.cfg`
   - **PBS**: `proxmox-backup-manager user list` → fallback to `/etc/proxmox-backup/user.cfg`

docs/TROUBLESHOOTING.md

@@ -571,6 +571,22 @@ If Email is enabled but you don't see it being dispatched, ensure `EMAIL_DELIVER
 - Relay blocks `root@…` recipients; use a real non-root mailbox for `EMAIL_RECIPIENT`.
 - If `EMAIL_FALLBACK_SENDMAIL=true`, ProxSave will fall back to `EMAIL_DELIVERY_METHOD=pmf` when the relay fails.
 - Check the proxsave logs for `email-relay` warnings/errors.
+- `Email relay accepted request ...` means the relay accepted the submission. It does **not** guarantee final inbox delivery; later provider-side failures/bounces are outside the ProxSave process.
+
+Common relay auth/forbidden errors:
+
+- `authentication failed (HTTP 401): missing bearer token`: the relay did not receive the `Authorization: Bearer ...` header.
+- `authentication failed (HTTP 401): missing signature`: the relay did not receive the `X-Signature` header.
+- `forbidden (HTTP 403): invalid token`: the bearer token is wrong or not allowed by the worker.
+- `forbidden (HTTP 403): HMAC signature validation failed`: the request body and `X-Signature` do not match the worker's `HMAC_SECRET`.
+- `forbidden (HTTP 403): missing or invalid script version`: the relay rejected `X-Script-Version` (it must be semantic-version-like, e.g. `1.2.3`).
+- `forbidden (HTTP 403): from address override not allowed`: the client attempted to override the worker-managed sender address.
+
+If you operate your own relay worker:
+
+- The worker-side env var `MAC_LIMIT_IP_WHITELIST` can bypass the per-server daily MAC quota for trusted source IPs.
+- Example: `MAC_LIMIT_IP_WHITELIST=86.56.17.99`
+- This bypass affects only the daily MAC quota. It does **not** disable bearer-token checks, HMAC validation, IP burst limits, or token limits.
 
 Quick checks for auto-detect:
 

internal/config/config_test.go

@@ -11,6 +11,24 @@ import (
 
 func setBaseDirEnv(t *testing.T, value string) {
 	t.Helper()
+	if value == "" {
+		original, hadOriginal := os.LookupEnv("BASE_DIR")
+		if err := os.Unsetenv("BASE_DIR"); err != nil {
+			t.Fatalf("Unsetenv(BASE_DIR) failed: %v", err)
+		}
+		t.Cleanup(func() {
+			if hadOriginal {
+				if err := os.Setenv("BASE_DIR", original); err != nil {
+					t.Fatalf("restore BASE_DIR failed: %v", err)
+				}
+				return
+			}
+			if err := os.Unsetenv("BASE_DIR"); err != nil {
+				t.Fatalf("cleanup Unsetenv(BASE_DIR) failed: %v", err)
+			}
+		})
+		return
+	}
 	t.Setenv("BASE_DIR", value)
 }
 

internal/config/env_mutation.go

@@ -1,19 +1,38 @@
 package config
 
 import (
+	"strconv"
 	"strings"
 
 	"github.com/tis24dev/proxsave/pkg/utils"
 )
 
+func encodeEnvValue(value string) string {
+	value = strings.TrimSpace(value)
+	if value == "" {
+		return ""
+	}
+
+	if !strings.ContainsAny(value, "# \t\r\n\"'") {
+		return value
+	}
+	if !strings.Contains(value, "'") {
+		return "'" + value + "'"
+	}
+	if !strings.Contains(value, `"`) {
+		return `"` + value + `"`
+	}
+	return strconv.Quote(value)
+}
+
 // ApplySecondaryStorageSettings writes the canonical secondary-storage state
 // into an env template. Disabled secondary storage always clears both
 // SECONDARY_PATH and SECONDARY_LOG_PATH so the saved config matches user intent.
 func ApplySecondaryStorageSettings(template string, enabled bool, secondaryPath string, secondaryLogPath string) string {
 	if enabled {
 		template = utils.SetEnvValue(template, "SECONDARY_ENABLED", "true")
-		template = utils.SetEnvValue(template, "SECONDARY_PATH", strings.TrimSpace(secondaryPath))
-		template = utils.SetEnvValue(template, "SECONDARY_LOG_PATH", strings.TrimSpace(secondaryLogPath))
+		template = utils.SetEnvValue(template, "SECONDARY_PATH", encodeEnvValue(secondaryPath))
+		template = utils.SetEnvValue(template, "SECONDARY_LOG_PATH", encodeEnvValue(secondaryLogPath))
 		return template
 	}
 

internal/config/env_mutation_test.go

@@ -1,74 +1,112 @@
 package config
 
 import (
+	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 )
 
-func TestApplySecondaryStorageSettingsEnabled(t *testing.T) {
-	template := "SECONDARY_ENABLED=false\nSECONDARY_PATH=\nSECONDARY_LOG_PATH=\n"
+func parseMutatedEnvTemplate(t *testing.T, template string) (map[string]string, map[string]int) {
+	t.Helper()
 
-	got := ApplySecondaryStorageSettings(template, true, " /mnt/secondary ", " /mnt/secondary/log ")
+	values := make(map[string]string)
+	counts := make(map[string]int)
+
+	for _, line := range strings.Split(template, "\n") {
+		trimmed := strings.TrimSpace(line)
+		if trimmed == "" || strings.HasPrefix(trimmed, "#") {
+			continue
+		}
 
-	for _, needle := range []string{
-		"SECONDARY_ENABLED=true",
-		"SECONDARY_PATH=/mnt/secondary",
-		"SECONDARY_LOG_PATH=/mnt/secondary/log",
-	} {
-		if !strings.Contains(got, needle) {
-			t.Fatalf("expected %q in template:\n%s", needle, got)
+		parts := strings.SplitN(line, "=", 2)
+		if len(parts) != 2 {
+			t.Fatalf("invalid env line %q in template:\n%s", line, template)
 		}
+
+		key := strings.TrimSpace(parts[0])
+		value := parts[1]
+		counts[key]++
+		values[key] = value
 	}
+
+	return values, counts
+}
+
+func assertMutatedEnvValue(t *testing.T, values map[string]string, counts map[string]int, key, want string) {
+	t.Helper()
+
+	if got := counts[key]; got != 1 {
+		t.Fatalf("%s occurrences = %d; want 1", key, got)
+	}
+	if got := values[key]; got != want {
+		t.Fatalf("%s = %q; want %q", key, got, want)
+	}
+}
+
+func TestApplySecondaryStorageSettingsEnabled(t *testing.T) {
+	template := "SECONDARY_ENABLED=false\nSECONDARY_PATH=\nSECONDARY_LOG_PATH=\n"
+
+	got := ApplySecondaryStorageSettings(template, true, " /mnt/secondary ", " /mnt/secondary/log ")
+	values, counts := parseMutatedEnvTemplate(t, got)
+	assertMutatedEnvValue(t, values, counts, "SECONDARY_ENABLED", "true")
+	assertMutatedEnvValue(t, values, counts, "SECONDARY_PATH", "/mnt/secondary")
+	assertMutatedEnvValue(t, values, counts, "SECONDARY_LOG_PATH", "/mnt/secondary/log")
 }
 
 func TestApplySecondaryStorageSettingsEnabledWithEmptyLogPath(t *testing.T) {
 	template := "SECONDARY_ENABLED=false\nSECONDARY_PATH=\nSECONDARY_LOG_PATH=/old/log\n"
 
 	got := ApplySecondaryStorageSettings(template, true, "/mnt/secondary", "")
-
-	for _, needle := range []string{
-		"SECONDARY_ENABLED=true",
-		"SECONDARY_PATH=/mnt/secondary",
-		"SECONDARY_LOG_PATH=",
-	} {
-		if !strings.Contains(got, needle) {
-			t.Fatalf("expected %q in template:\n%s", needle, got)
-		}
-	}
+	values, counts := parseMutatedEnvTemplate(t, got)
+	assertMutatedEnvValue(t, values, counts, "SECONDARY_ENABLED", "true")
+	assertMutatedEnvValue(t, values, counts, "SECONDARY_PATH", "/mnt/secondary")
+	assertMutatedEnvValue(t, values, counts, "SECONDARY_LOG_PATH", "")
 }
 
 func TestApplySecondaryStorageSettingsDisabledClearsValues(t *testing.T) {
 	template := "SECONDARY_ENABLED=true\nSECONDARY_PATH=/mnt/old-secondary\nSECONDARY_LOG_PATH=/mnt/old-secondary/logs\n"
 
 	got := ApplySecondaryStorageSettings(template, false, "/ignored", "/ignored/logs")
-
-	for _, needle := range []string{
-		"SECONDARY_ENABLED=false",
-		"SECONDARY_PATH=",
-		"SECONDARY_LOG_PATH=",
-	} {
-		if !strings.Contains(got, needle) {
-			t.Fatalf("expected %q in template:\n%s", needle, got)
-		}
-	}
-	if strings.Contains(got, "/mnt/old-secondary") {
-		t.Fatalf("expected old secondary values to be cleared:\n%s", got)
-	}
+	values, counts := parseMutatedEnvTemplate(t, got)
+	assertMutatedEnvValue(t, values, counts, "SECONDARY_ENABLED", "false")
+	assertMutatedEnvValue(t, values, counts, "SECONDARY_PATH", "")
+	assertMutatedEnvValue(t, values, counts, "SECONDARY_LOG_PATH", "")
 }
 
 func TestApplySecondaryStorageSettingsDisabledAppendsCanonicalState(t *testing.T) {
 	template := "BACKUP_ENABLED=true\n"
 
 	got := ApplySecondaryStorageSettings(template, false, "", "")
+	values, counts := parseMutatedEnvTemplate(t, got)
+	assertMutatedEnvValue(t, values, counts, "BACKUP_ENABLED", "true")
+	assertMutatedEnvValue(t, values, counts, "SECONDARY_ENABLED", "false")
+	assertMutatedEnvValue(t, values, counts, "SECONDARY_PATH", "")
+	assertMutatedEnvValue(t, values, counts, "SECONDARY_LOG_PATH", "")
+}
 
-	for _, needle := range []string{
-		"BACKUP_ENABLED=true",
-		"SECONDARY_ENABLED=false",
-		"SECONDARY_PATH=",
-		"SECONDARY_LOG_PATH=",
-	} {
-		if !strings.Contains(got, needle) {
-			t.Fatalf("expected %q in template:\n%s", needle, got)
-		}
+func TestApplySecondaryStorageSettingsQuotesUnsafePaths(t *testing.T) {
+	template := "SECONDARY_ENABLED=false\nSECONDARY_PATH=\nSECONDARY_LOG_PATH=\n"
+
+	got := ApplySecondaryStorageSettings(template, true, " /mnt/secondary #1 ", " /mnt/secondary/log dir ")
+	values, counts := parseMutatedEnvTemplate(t, got)
+	assertMutatedEnvValue(t, values, counts, "SECONDARY_ENABLED", "true")
+	assertMutatedEnvValue(t, values, counts, "SECONDARY_PATH", "'/mnt/secondary #1'")
+	assertMutatedEnvValue(t, values, counts, "SECONDARY_LOG_PATH", "'/mnt/secondary/log dir'")
+
+	configPath := filepath.Join(t.TempDir(), "backup.env")
+	if err := os.WriteFile(configPath, []byte(got), 0o600); err != nil {
+		t.Fatalf("write config: %v", err)
+	}
+
+	raw, err := parseEnvFile(configPath)
+	if err != nil {
+		t.Fatalf("parseEnvFile() error = %v", err)
+	}
+	if gotPath := raw["SECONDARY_PATH"]; gotPath != "/mnt/secondary #1" {
+		t.Fatalf("SECONDARY_PATH = %q; want %q", gotPath, "/mnt/secondary #1")
+	}
+	if gotLogPath := raw["SECONDARY_LOG_PATH"]; gotLogPath != "/mnt/secondary/log dir" {
+		t.Fatalf("SECONDARY_LOG_PATH = %q; want %q", gotLogPath, "/mnt/secondary/log dir")
 	}
 }