From 88c6018950f839fe2af7b9ca1b223496fa5ac91a Mon Sep 17 00:00:00 2001 From: Shengwen YU Date: Wed, 9 Aug 2023 10:22:54 +0800 Subject: [PATCH] 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 --- src/common/utils/utils.go | 16 ++++++ src/common/utils/utils_test.go | 50 +++++++++++++++++++ src/controller/p2p/preheat/controller.go | 12 +++++ src/controller/p2p/preheat/controllor_test.go | 8 +-- src/controller/retention/controller.go | 8 +++ src/controller/retention/controller_test.go | 4 +- src/pkg/p2p/preheat/models/policy/policy.go | 12 +++++ .../p2p/preheat/models/policy/policy_test.go | 24 +++++++++ src/pkg/retention/policy/models.go | 19 +++++++ src/pkg/retention/policy/models_test.go | 42 ++++++++++++++++ src/server/v2.0/handler/gc.go | 9 ++-- src/server/v2.0/handler/purge.go | 8 +-- src/server/v2.0/handler/scan_all.go | 5 ++ 13 files changed, 204 insertions(+), 13 deletions(-) diff --git a/src/common/utils/utils.go b/src/common/utils/utils.go index 789807dfd..22875014c 100644 --- a/src/common/utils/utils.go +++ b/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: // diff --git a/src/common/utils/utils_test.go b/src/common/utils/utils_test.go index 025806994..73d9fb28b 100644 --- a/src/common/utils/utils_test.go +++ b/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) + } + } + } +} diff --git a/src/controller/p2p/preheat/controller.go b/src/controller/p2p/preheat/controller.go index 36dec8660..bb9c375b6 100644 --- a/src/controller/p2p/preheat/controller.go +++ b/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 diff --git a/src/controller/p2p/preheat/controllor_test.go b/src/controller/p2p/preheat/controllor_test.go index df76cdd03..b06af2672 100644 --- a/src/controller/p2p/preheat/controllor_test.go +++ b/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, "") diff --git a/src/controller/retention/controller.go b/src/controller/retention/controller.go index 5f39d1de7..ef79c5b6a 100644 --- a/src/controller/retention/controller.go +++ b/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 diff --git a/src/controller/retention/controller_test.go b/src/controller/retention/controller_test.go index 07307b181..2088feb07 100644 --- a/src/controller/retention/controller_test.go +++ b/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{ diff --git a/src/pkg/p2p/preheat/models/policy/policy.go b/src/pkg/p2p/preheat/models/policy/policy.go index aab0e789d..1a59bdddf 100644 --- a/src/pkg/p2p/preheat/models/policy/policy.go +++ b/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 { diff --git a/src/pkg/p2p/preheat/models/policy/policy_test.go b/src/pkg/p2p/preheat/models/policy/policy_test.go index 6c39b42e1..e2310933a 100644 --- a/src/pkg/p2p/preheat/models/policy/policy_test.go +++ b/src/pkg/p2p/preheat/models/policy/policy_test.go @@ -37,6 +37,7 @@ 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. @@ -44,6 +45,29 @@ 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 diff --git a/src/pkg/retention/policy/models.go b/src/pkg/retention/policy/models.go index 42b4d20fb..14e7630f4 100644 --- a/src/pkg/retention/policy/models.go +++ b/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 { diff --git a/src/pkg/retention/policy/models_test.go b/src/pkg/retention/policy/models_test.go index 5ca55ab9e..78daa1d4e 100644 --- a/src/pkg/retention/policy/models_test.go +++ b/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", diff --git a/src/server/v2.0/handler/gc.go b/src/server/v2.0/handler/gc.go index 7be005b9d..88cf97cb1 100644 --- a/src/server/v2.0/handler/gc.go +++ b/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 } diff --git a/src/server/v2.0/handler/purge.go b/src/server/v2.0/handler/purge.go index 8a624feb4..564d94c6c 100644 --- a/src/server/v2.0/handler/purge.go +++ b/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 diff --git a/src/server/v2.0/handler/scan_all.go b/src/server/v2.0/handler/scan_all.go index e2dc51abe..1ae867ac8 100644 --- a/src/server/v2.0/handler/scan_all.go +++ b/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