fix: cron string validation (#19071)

fix: cron string validation (the 1st field of a cron string must be 0 when there are 6 fields)

Signed-off-by: Shengwen Yu <yshengwen@vmware.com>

src/common/utils/utils.go

@@ -301,6 +301,22 @@ func CronParser() cronlib.Parser {
 	return cronlib.NewParser(cronlib.Second | cronlib.Minute | cronlib.Hour | cronlib.Dom | cronlib.Month | cronlib.Dow)
 }
 
+// ValidateCronString check whether it is a valid cron string and whether the 1st field (indicating Seconds of time) of the cron string is a fixed value of 0 or not
+func ValidateCronString(cron string) error {
+	if len(cron) == 0 {
+		return fmt.Errorf("empty cron string is invalid")
+	}
+	_, err := CronParser().Parse(cron)
+	if err != nil {
+		return err
+	}
+	cronParts := strings.Split(cron, " ")
+	if len(cronParts) == 6 && cronParts[0] != "0" {
+		return fmt.Errorf("the 1st field (indicating Seconds of time) of the cron setting must be 0")
+	}
+	return nil
+}
+
 // MostMatchSorter is a sorter for the most match, usually invoked in sort Less function
 // usage:
 //

src/common/utils/utils_test.go

@@ -436,3 +436,53 @@ func Test_sortMostMatch(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateCronString(t *testing.T) {
+	testCases := []struct {
+		description string
+		input       string
+		hasErr      bool
+	}{
+		// empty cron string
+		{
+			description: "test case 1",
+			input:       "",
+			hasErr:      true,
+		},
+
+		// invalid cron format
+		{
+			description: "test case 2",
+			input:       "0 2 3",
+			hasErr:      true,
+		},
+
+		// the 1st field (indicating Seconds of time) of the cron setting must be 0
+		{
+			description: "test case 3",
+			input:       "1 0 0 1 1 0",
+			hasErr:      true,
+		},
+
+		// valid cron string
+		{
+			description: "test case 4",
+			input:       "0 1 2 1 1 *",
+			hasErr:      false,
+		},
+	}
+
+	for _, tc := range testCases {
+		err := ValidateCronString(tc.input)
+		if tc.hasErr {
+			if err == nil {
+				t.Errorf("%s, expect having error, while actual error returned is nil", tc.description)
+			}
+		} else {
+			// tc.hasErr == false
+			if err != nil {
+				t.Errorf("%s, expect having no error, while actual error returned is not nil, err=%v", tc.description, err)
+			}
+		}
+	}
+}

src/controller/p2p/preheat/controller.go

@@ -294,6 +294,12 @@ func (c *controller) CreatePolicy(ctx context.Context, schema *policyModels.Sche
 		return 0, err
 	}
 
+	// valid policy schema
+	err = schema.ValidatePreheatPolicy()
+	if err != nil {
+		return 0, err
+	}
+
 	id, err = c.pManager.Create(ctx, schema)
 	if err != nil {
 		return
@@ -360,6 +366,12 @@ func (c *controller) UpdatePolicy(ctx context.Context, schema *policyModels.Sche
 		return err
 	}
 
+	// valid policy schema
+	err = schema.ValidatePreheatPolicy()
+	if err != nil {
+		return err
+	}
+
 	var cron = schema.Trigger.Settings.Cron
 	var oldCron = s0.Trigger.Settings.Cron
 	var needUn bool

src/controller/p2p/preheat/controllor_test.go

@@ -240,7 +240,7 @@ func (s *preheatSuite) TestCreatePolicy() {
 	policy := &policy.Schema{
 		Name:       "test",
 		FiltersStr: `[{"type":"repository","value":"harbor*"},{"type":"tag","value":"2*"}]`,
-		TriggerStr: fmt.Sprintf(`{"type":"%s", "trigger_setting":{"cron":"* * * * */1"}}`, policy.TriggerTypeScheduled),
+		TriggerStr: fmt.Sprintf(`{"type":"%s", "trigger_setting":{"cron":"0 * * * * */1"}}`, policy.TriggerTypeScheduled),
 	}
 	s.fakeScheduler.On("Schedule", s.ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(int64(1), nil)
 	s.fakePolicyMgr.On("Create", s.ctx, policy).Return(int64(1), nil)
@@ -269,7 +269,7 @@ func (s *preheatSuite) TestGetPolicyByName() {
 
 func (s *preheatSuite) TestUpdatePolicy() {
 	var p0 = &policy.Schema{Name: "test", Trigger: &policy.Trigger{Type: policy.TriggerTypeScheduled}}
-	p0.Trigger.Settings.Cron = "* * * * */1"
+	p0.Trigger.Settings.Cron = "0 * * * * */1"
 	p0.Filters = []*policy.Filter{
 		{
 			Type:  policy.FilterTypeRepository,
@@ -281,6 +281,8 @@ func (s *preheatSuite) TestUpdatePolicy() {
 		},
 	}
 	s.fakePolicyMgr.On("Get", s.ctx, int64(1)).Return(p0, nil)
+	s.fakeScheduler.On("UnScheduleByVendor", s.ctx, mock.Anything, mock.Anything).Return(nil)
+	s.fakeScheduler.On("Schedule", s.ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(int64(1), nil)
 
 	// need change to schedule
 	p1 := &policy.Schema{
@@ -299,7 +301,7 @@ func (s *preheatSuite) TestUpdatePolicy() {
 		ID:         1,
 		Name:       "test",
 		FiltersStr: `[{"type":"repository","value":"harbor*"},{"type":"tag","value":"2*"}]`,
-		TriggerStr: fmt.Sprintf(`{"type":"%s", "trigger_setting":{"cron":"* * * * */2"}}`, policy.TriggerTypeScheduled),
+		TriggerStr: fmt.Sprintf(`{"type":"%s", "trigger_setting":{"cron":"0 * * * * */2"}}`, policy.TriggerTypeScheduled),
 	}
 	s.fakePolicyMgr.On("Update", s.ctx, p2, mock.Anything).Return(nil)
 	err = s.controller.UpdatePolicy(s.ctx, p2, "")

src/controller/retention/controller.go

@@ -100,6 +100,10 @@ func (r *defaultController) GetRetention(ctx context.Context, id int64) (*policy
 
 // CreateRetention Create Retention
 func (r *defaultController) CreateRetention(ctx context.Context, p *policy.Metadata) (int64, error) {
+	err := p.ValidateRetentionPolicy()
+	if err != nil {
+		return 0, err
+	}
 	id, err := r.manager.CreatePolicy(ctx, p)
 	if err != nil {
 		return 0, err
@@ -125,6 +129,10 @@ func (r *defaultController) CreateRetention(ctx context.Context, p *policy.Metad
 
 // UpdateRetention Update Retention
 func (r *defaultController) UpdateRetention(ctx context.Context, p *policy.Metadata) error {
+	err := p.ValidateRetentionPolicy()
+	if err != nil {
+		return err
+	}
 	p0, err := r.manager.GetPolicy(ctx, p.ID)
 	if err != nil {
 		return err

src/controller/retention/controller_test.go

@@ -160,7 +160,7 @@ func (s *ControllerTestSuite) TestPolicy() {
 		Trigger: &policy.Trigger{
 			Kind: "Schedule",
 			Settings: map[string]interface{}{
-				"cron": "* 22 11 * * *",
+				"cron": "0 22 11 * * *",
 			},
 		},
 		Scope: &policy.Scope{
@@ -271,7 +271,7 @@ func (s *ControllerTestSuite) TestExecution() {
 		Trigger: &policy.Trigger{
 			Kind: "Schedule",
 			Settings: map[string]interface{}{
-				"cron": "* 22 11 * * *",
+				"cron": "0 22 11 * * *",
 			},
 		},
 		Scope: &policy.Scope{

src/pkg/p2p/preheat/models/policy/policy.go

@@ -118,6 +118,18 @@ type Trigger struct {
 	} `json:"trigger_setting,omitempty"`
 }
 
+// ValidatePreheatPolicy validate preheat policy
+func (s *Schema) ValidatePreheatPolicy() error {
+	// currently only validate cron string of preheat policy
+	if s.Trigger != nil && s.Trigger.Type == TriggerTypeScheduled && len(s.Trigger.Settings.Cron) > 0 {
+		if err := utils.ValidateCronString(s.Trigger.Settings.Cron); err != nil {
+			return errors.New(nil).WithCode(errors.BadRequestCode).
+				WithMessage("invalid cron string for scheduled preheat: %s, error: %v", s.Trigger.Settings.Cron, err)
+		}
+	}
+	return nil
+}
+
 // Valid the policy
 func (s *Schema) Valid(v *validation.Validation) {
 	if len(s.Name) == 0 {

src/pkg/p2p/preheat/models/policy/policy_test.go

@@ -37,13 +37,37 @@ func TestPolicy(t *testing.T) {
 // SetupSuite prepares the env for PolicyTestSuite.
 func (p *PolicyTestSuite) SetupSuite() {
 	p.schema = &Schema{}
+	p.schema.Trigger = &Trigger{}
 }
 
 // TearDownSuite clears the env for PolicyTestSuite.
 func (p *PolicyTestSuite) TearDownSuite() {
 	p.schema = nil
 }
 
+// TestValidatePreheatPolicy tests the ValidatePreheatPolicy method
+func (p *PolicyTestSuite) TestValidatePreheatPolicy() {
+	// manual trigger
+	p.schema.Trigger.Type = TriggerTypeManual
+	p.NoError(p.schema.ValidatePreheatPolicy())
+
+	// event trigger
+	p.schema.Trigger.Type = TriggerTypeEventBased
+	p.NoError(p.schema.ValidatePreheatPolicy())
+
+	// scheduled trigger
+	p.schema.Trigger.Type = TriggerTypeScheduled
+	// cron string is empty
+	p.schema.Trigger.Settings.Cron = ""
+	p.NoError(p.schema.ValidatePreheatPolicy())
+	// the 1st field of cron string is not 0
+	p.schema.Trigger.Settings.Cron = "1 0 0 1 1 *"
+	p.Error(p.schema.ValidatePreheatPolicy())
+	// valid cron string
+	p.schema.Trigger.Settings.Cron = "0 0 0 1 1 *"
+	p.NoError(p.schema.ValidatePreheatPolicy())
+}
+
 // TestValid tests Valid method.
 func (p *PolicyTestSuite) TestValid() {
 	// policy name is empty, should return error

src/pkg/retention/policy/models.go

@@ -17,6 +17,8 @@ package policy
 import (
 	"github.com/beego/beego/v2/core/validation"
 
+	"github.com/goharbor/harbor/src/common/utils"
+	"github.com/goharbor/harbor/src/lib/errors"
 	"github.com/goharbor/harbor/src/lib/selector/selectors/doublestar"
 	"github.com/goharbor/harbor/src/pkg/retention/policy/rule"
 	"github.com/goharbor/harbor/src/pkg/retention/policy/rule/index"
@@ -58,6 +60,23 @@ type Metadata struct {
 	Scope *Scope `json:"scope" valid:"Required"`
 }
 
+// ValidateRetentionPolicy validate the retention policy
+func (m *Metadata) ValidateRetentionPolicy() error {
+	// currently only validate the cron string of retention policy
+	if m.Trigger != nil {
+		if m.Trigger.Kind == TriggerKindSchedule && m.Trigger.Settings != nil {
+			cronItem, ok := m.Trigger.Settings[TriggerSettingsCron]
+			if ok && len(cronItem.(string)) > 0 {
+				if err := utils.ValidateCronString(cronItem.(string)); err != nil {
+					return errors.New(nil).WithCode(errors.BadRequestCode).
+						WithMessage("invalid cron string for scheduled tag retention: %s, error: %v", cronItem.(string), err)
+				}
+			}
+		}
+	}
+	return nil
+}
+
 // Valid Valid
 func (m *Metadata) Valid(v *validation.Validation) {
 	if m.Trigger == nil {

src/pkg/retention/policy/models_test.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/beego/beego/v2/core/validation"
 	"github.com/stretchr/testify/require"
+	"github.com/stretchr/testify/suite"
 
 	"github.com/goharbor/harbor/src/pkg/retention/policy/rule"
 )
@@ -35,6 +36,47 @@ func TestAlgorithm(t *testing.T) {
 // 	}
 // }
 
+type PolicyTestSuite struct {
+	suite.Suite
+
+	policy *Metadata
+}
+
+// TestRetentionPolicy is the entry method of running PolicyTestSuite.
+func TestRetentionPolicy(t *testing.T) {
+	suite.Run(t, &PolicyTestSuite{})
+}
+
+// SetupSuite prepares the env for PolicyTestSuite.
+func (p *PolicyTestSuite) SetupSuite() {
+	p.policy = &Metadata{}
+	p.policy.Trigger = &Trigger{}
+}
+
+// TearDownSuite clears the env for PolicyTestSuite.
+func (p *PolicyTestSuite) TearDownSuite() {
+	p.policy = nil
+}
+
+func (p *PolicyTestSuite) TestValidateRetentionPolicy() {
+	p.policy.Trigger.Kind = TriggerKindSchedule
+
+	// cron is not in the map of trigger setting
+	p.NoError(p.policy.ValidateRetentionPolicy())
+
+	// cron value is an empty string
+	p.policy.Trigger.Settings = map[string]interface{}{"cron": ""}
+	p.NoError(p.policy.ValidateRetentionPolicy())
+
+	//  the 1st field of cron value is not 0
+	p.policy.Trigger.Settings = map[string]interface{}{"cron": "1 0 0 1 1 *"}
+	p.Error(p.policy.ValidateRetentionPolicy())
+
+	// valid cron value
+	p.policy.Trigger.Settings = map[string]interface{}{"cron": "0 0 0 1 1 *"}
+	p.NoError(p.policy.ValidateRetentionPolicy())
+}
+
 func TestRule(t *testing.T) {
 	p := &Metadata{
 		Algorithm: "or",

src/server/v2.0/handler/gc.go

@@ -24,6 +24,7 @@ import (
 	"github.com/go-openapi/runtime/middleware"
 
 	"github.com/goharbor/harbor/src/common/rbac"
+	"github.com/goharbor/harbor/src/common/utils"
 	"github.com/goharbor/harbor/src/controller/gc"
 	"github.com/goharbor/harbor/src/jobservice/job"
 	"github.com/goharbor/harbor/src/lib/config"
@@ -139,10 +140,6 @@ func (g *gcAPI) kick(ctx context.Context, scheType string, cron string, paramete
 }
 
 func (g *gcAPI) createSchedule(ctx context.Context, cronType, cron string, policy gc.Policy) error {
-	if cron == "" {
-		return errors.New(nil).WithCode(errors.BadRequestCode).
-			WithMessage("empty cron string for gc schedule")
-	}
 	_, err := g.gcCtr.CreateSchedule(ctx, cronType, cron, policy)
 	if err != nil {
 		return err
@@ -151,6 +148,10 @@ func (g *gcAPI) createSchedule(ctx context.Context, cronType, cron string, polic
 }
 
 func (g *gcAPI) updateSchedule(ctx context.Context, cronType, cron string, policy gc.Policy) error {
+	if err := utils.ValidateCronString(cron); err != nil {
+		return errors.New(nil).WithCode(errors.BadRequestCode).
+			WithMessage("invalid cron string for scheduled gc: %s, error: %v", cron, err)
+	}
 	if err := g.gcCtr.DeleteSchedule(ctx); err != nil {
 		return err
 	}

src/server/v2.0/handler/purge.go

@@ -141,6 +141,10 @@ func (p *purgeAPI) kick(ctx context.Context, vendorType string, scheType string,
 }
 
 func (p *purgeAPI) updateSchedule(ctx context.Context, vendorType, cronType, cron string, policy pg.JobPolicy, extraParams map[string]interface{}) error {
+	if err := utils.ValidateCronString(cron); err != nil {
+		return errors.New(nil).WithCode(errors.BadRequestCode).
+			WithMessage("invalid cron string for scheduled log rotation purge: %s, error: %v", cron, err)
+	}
 	if err := p.schedulerCtl.Delete(ctx, vendorType); err != nil {
 		return err
 	}
@@ -315,10 +319,6 @@ func verifyUpdateRequest(params purge.UpdatePurgeScheduleParams) error {
 }
 
 func (p *purgeAPI) createSchedule(ctx context.Context, vendorType string, cronType string, cron string, policy pg.JobPolicy, extraParam map[string]interface{}) error {
-	if cron == "" {
-		return errors.New(nil).WithCode(errors.BadRequestCode).
-			WithMessage("empty cron string for schedule")
-	}
 	_, err := p.schedulerCtl.Create(ctx, vendorType, cronType, cron, pg.SchedulerCallback, policy, extraParam)
 	if err != nil {
 		return err

src/server/v2.0/handler/scan_all.go

@@ -25,6 +25,7 @@ import (
 
 	"github.com/goharbor/harbor/src/common/rbac"
 	"github.com/goharbor/harbor/src/common/secret"
+	"github.com/goharbor/harbor/src/common/utils"
 	"github.com/goharbor/harbor/src/controller/scan"
 	"github.com/goharbor/harbor/src/controller/scanner"
 	"github.com/goharbor/harbor/src/jobservice/job"
@@ -194,6 +195,10 @@ func (s *scanAllAPI) GetLatestScheduledScanAllMetrics(ctx context.Context, param
 }
 
 func (s *scanAllAPI) createOrUpdateScanAllSchedule(ctx context.Context, cronType, cron string, previous *scheduler.Schedule) (int64, error) {
+	if err := utils.ValidateCronString(cron); err != nil {
+		return 0, errors.New(nil).WithCode(errors.BadRequestCode).
+			WithMessage("invalid cron string for scheduled scan all: %s, error: %v", cron, err)
+	}
 	if previous != nil {
 		if cronType == previous.CRONType && cron == previous.CRON {
 			return previous.ID, nil