From 2961aaf72be6a3e657005cf2efa9c702bc868cd9 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Fri, 7 Jul 2023 13:10:16 +0200 Subject: [PATCH] Harden datastore (#1940) - only explicit when filters - write all more complex filter with builder - reuse builder --- server/store/datastore/config.go | 12 +++--- server/store/datastore/cron.go | 7 +--- server/store/datastore/permission.go | 23 +++++------ server/store/datastore/pipeline.go | 53 ++++++++++++------------- server/store/datastore/redirection.go | 8 +++- server/store/datastore/registry.go | 11 ++--- server/store/datastore/secret.go | 30 +++++++------- server/store/datastore/server_config.go | 7 +--- server/store/datastore/step.go | 24 +++++------ 9 files changed, 84 insertions(+), 91 deletions(-) diff --git a/server/store/datastore/config.go b/server/store/datastore/config.go index 8d81cfd87..b4e072bc1 100644 --- a/server/store/datastore/config.go +++ b/server/store/datastore/config.go @@ -30,11 +30,10 @@ func (s storage) ConfigsForPipeline(pipelineID int64) ([]*model.Config, error) { } func (s storage) ConfigFindIdentical(repoID int64, hash string) (*model.Config, error) { - conf := &model.Config{ - RepoID: repoID, - Hash: hash, - } - if err := wrapGet(s.engine.Get(conf)); err != nil { + conf := new(model.Config) + if err := wrapGet(s.engine.Where( + builder.Eq{"config_repo_id": repoID, "config_hash": hash}, + ).Get(conf)); err != nil { return nil, err } return conf, nil @@ -43,8 +42,7 @@ func (s storage) ConfigFindIdentical(repoID int64, hash string) (*model.Config, func (s storage) ConfigFindApproved(config *model.Config) (bool, error) { return s.engine.Table("pipelines").Select("pipelines.pipeline_id"). Join("INNER", "pipeline_config", "pipelines.pipeline_id = pipeline_config.pipeline_id"). - Where(builder.Eq{"pipelines.pipeline_repo_id": config.RepoID}. - And(builder.Eq{"pipeline_config.config_id": config.ID}). + Where(builder.Eq{"pipelines.pipeline_repo_id": config.RepoID, "pipeline_config.config_id": config.ID}. And(builder.NotIn("pipelines.pipeline_status", model.StatusBlocked, model.StatusPending))). Exist() } diff --git a/server/store/datastore/cron.go b/server/store/datastore/cron.go index 1910cafce..7f0a39b1c 100644 --- a/server/store/datastore/cron.go +++ b/server/store/datastore/cron.go @@ -29,11 +29,8 @@ func (s storage) CronCreate(cron *model.Cron) error { } func (s storage) CronFind(repo *model.Repo, id int64) (*model.Cron, error) { - cron := &model.Cron{ - RepoID: repo.ID, - ID: id, - } - return cron, wrapGet(s.engine.Get(cron)) + cron := new(model.Cron) + return cron, wrapGet(s.engine.ID(id).Where("repo_id = ?", repo.ID).Get(cron)) } func (s storage) CronList(repo *model.Repo, p *model.ListOptions) ([]*model.Cron, error) { diff --git a/server/store/datastore/permission.go b/server/store/datastore/permission.go index ce5dfd1f5..bfc340e20 100644 --- a/server/store/datastore/permission.go +++ b/server/store/datastore/permission.go @@ -24,11 +24,10 @@ import ( ) func (s storage) PermFind(user *model.User, repo *model.Repo) (*model.Perm, error) { - perm := &model.Perm{ - UserID: user.ID, - RepoID: repo.ID, - } - return perm, wrapGet(s.engine.Get(perm)) + perm := new(model.Perm) + return perm, wrapGet(s.engine. + Where(builder.Eq{"perm_user_id": user.ID, "perm_repo_id": repo.ID}). + Get(perm)) } func (s storage) PermUpsert(perm *model.Perm) error { @@ -59,15 +58,13 @@ func (s storage) permUpsert(sess *xorm.Session, perm *model.Perm) error { perm.RepoID = r.ID } - exist, err := sess.Where("perm_user_id = ? AND perm_repo_id = ?", perm.UserID, perm.RepoID). - Exist(new(model.Perm)) + exist, err := sess.Where(userIDAndRepoIDCond(perm)).Exist(new(model.Perm)) if err != nil { return err } if exist { - _, err = sess.Where("perm_user_id = ? AND perm_repo_id = ?", perm.UserID, perm.RepoID). - AllCols().Update(perm) + _, err = sess.Where(userIDAndRepoIDCond(perm)).AllCols().Update(perm) } else { // only Insert set auto created ID back to object _, err = sess.Insert(perm) @@ -76,9 +73,7 @@ func (s storage) permUpsert(sess *xorm.Session, perm *model.Perm) error { } func (s storage) PermDelete(perm *model.Perm) error { - return wrapDelete(s.engine. - Where("perm_user_id = ? AND perm_repo_id = ?", perm.UserID, perm.RepoID). - Delete(new(model.Perm))) + return wrapDelete(s.engine.Where(userIDAndRepoIDCond(perm)).Delete(new(model.Perm))) } func (s storage) PermFlush(user *model.User, before int64) error { @@ -95,3 +90,7 @@ func userPushOrAdminCondition(userID int64) builder.Cond { And(builder.Eq{"perms.perm_push": true}. Or(builder.Eq{"perms.perm_admin": true})) } + +func userIDAndRepoIDCond(perm *model.Perm) builder.Cond { + return builder.Eq{"perm_user_id": perm.UserID, "perm_repo_id": perm.RepoID} +} diff --git a/server/store/datastore/pipeline.go b/server/store/datastore/pipeline.go index 9751a865c..332308b8b 100644 --- a/server/store/datastore/pipeline.go +++ b/server/store/datastore/pipeline.go @@ -17,6 +17,7 @@ package datastore import ( "time" + "xorm.io/builder" "xorm.io/xorm" "github.com/woodpecker-ci/woodpecker/server/model" @@ -28,47 +29,40 @@ func (s storage) GetPipeline(id int64) (*model.Pipeline, error) { } func (s storage) GetPipelineNumber(repo *model.Repo, num int64) (*model.Pipeline, error) { - pipeline := &model.Pipeline{ - RepoID: repo.ID, - Number: num, - } - return pipeline, wrapGet(s.engine.Get(pipeline)) + pipeline := new(model.Pipeline) + return pipeline, wrapGet(s.engine.Where( + builder.Eq{"pipeline_repo_id": repo.ID, "pipeline_number": num}, + ).Get(pipeline)) } func (s storage) GetPipelineRef(repo *model.Repo, ref string) (*model.Pipeline, error) { - pipeline := &model.Pipeline{ - RepoID: repo.ID, - Ref: ref, - } - return pipeline, wrapGet(s.engine.Get(pipeline)) + pipeline := new(model.Pipeline) + return pipeline, wrapGet(s.engine.Where( + builder.Eq{"pipeline_repo_id": repo.ID, "pipeline_ref": ref}, + ).Get(pipeline)) } func (s storage) GetPipelineCommit(repo *model.Repo, sha, branch string) (*model.Pipeline, error) { - pipeline := &model.Pipeline{ - RepoID: repo.ID, - Branch: branch, - Commit: sha, - } - return pipeline, wrapGet(s.engine.Get(pipeline)) + pipeline := new(model.Pipeline) + return pipeline, wrapGet(s.engine.Where( + builder.Eq{"pipeline_repo_id": repo.ID, "pipeline_branch": branch, "pipeline_commit": sha}, + ).Get(pipeline)) } func (s storage) GetPipelineLast(repo *model.Repo, branch string) (*model.Pipeline, error) { - pipeline := &model.Pipeline{ - RepoID: repo.ID, - Branch: branch, - Event: model.EventPush, - } - return pipeline, wrapGet(s.engine.Desc("pipeline_number").Get(pipeline)) + pipeline := new(model.Pipeline) + return pipeline, wrapGet(s.engine. + Desc("pipeline_number"). + Where(builder.Eq{"pipeline_repo_id": repo.ID, "pipeline_branch": branch, "pipeline_event": model.EventPush}). + Get(pipeline)) } func (s storage) GetPipelineLastBefore(repo *model.Repo, branch string, num int64) (*model.Pipeline, error) { - pipeline := &model.Pipeline{ - RepoID: repo.ID, - Branch: branch, - } + pipeline := new(model.Pipeline) return pipeline, wrapGet(s.engine. Desc("pipeline_number"). - Where("pipeline_id < ?", num). + Where(builder.Lt{"pipeline_id": num}. + And(builder.Eq{"pipeline_repo_id": repo.ID, "pipeline_branch": branch})). Get(pipeline)) } @@ -111,7 +105,10 @@ func (s storage) CreatePipeline(pipeline *model.Pipeline, stepList ...*model.Ste // calc pipeline number var number int64 - if _, err := sess.SQL("SELECT MAX(pipeline_number) FROM `pipelines` WHERE pipeline_repo_id = ?", pipeline.RepoID).Get(&number); err != nil { + if _, err := sess.Select("MAX(pipeline_number)"). + Table(new(model.Pipeline)). + Where("pipeline_repo_id = ?", pipeline.RepoID). + Get(&number); err != nil { return err } pipeline.Number = number + 1 diff --git a/server/store/datastore/redirection.go b/server/store/datastore/redirection.go index 4f6cf8c05..1cee51d0c 100644 --- a/server/store/datastore/redirection.go +++ b/server/store/datastore/redirection.go @@ -15,8 +15,10 @@ package datastore import ( - "github.com/woodpecker-ci/woodpecker/server/model" + "xorm.io/builder" "xorm.io/xorm" + + "github.com/woodpecker-ci/woodpecker/server/model" ) func (s storage) GetRedirection(fullName string) (*model.Redirection, error) { @@ -43,5 +45,7 @@ func (s storage) createRedirection(e *xorm.Session, redirect *model.Redirection) } func (s storage) HasRedirectionForRepo(repoID int64, fullName string) (bool, error) { - return s.engine.Where("repo_id = ? ", repoID).And("repo_full_name = ?", fullName).Get(new(model.Redirection)) + return s.engine.Where( + builder.Eq{"repo_id": repoID, "repo_full_name": fullName}, + ).Exist(new(model.Redirection)) } diff --git a/server/store/datastore/registry.go b/server/store/datastore/registry.go index 916380b2d..b8fd719d3 100644 --- a/server/store/datastore/registry.go +++ b/server/store/datastore/registry.go @@ -15,15 +15,16 @@ package datastore import ( + "xorm.io/builder" + "github.com/woodpecker-ci/woodpecker/server/model" ) func (s storage) RegistryFind(repo *model.Repo, addr string) (*model.Registry, error) { - reg := &model.Registry{ - RepoID: repo.ID, - Address: addr, - } - return reg, wrapGet(s.engine.Get(reg)) + reg := new(model.Registry) + return reg, wrapGet(s.engine.Where( + builder.Eq{"registry_repo_id": repo.ID, "registry_addr": addr}, + ).Get(reg)) } func (s storage) RegistryList(repo *model.Repo, p *model.ListOptions) ([]*model.Registry, error) { diff --git a/server/store/datastore/secret.go b/server/store/datastore/secret.go index 7da0f6f9a..047901725 100644 --- a/server/store/datastore/secret.go +++ b/server/store/datastore/secret.go @@ -23,11 +23,10 @@ import ( const orderSecretsBy = "secret_name" func (s storage) SecretFind(repo *model.Repo, name string) (*model.Secret, error) { - secret := &model.Secret{ - RepoID: repo.ID, - Name: name, - } - return secret, wrapGet(s.engine.Get(secret)) + secret := new(model.Secret) + return secret, wrapGet(s.engine.Where( + builder.Eq{"secret_repo_id": repo.ID, "secret_name": name}, + ).Get(secret)) } func (s storage) SecretList(repo *model.Repo, includeGlobalAndOrgSecrets bool, p *model.ListOptions) ([]*model.Secret, error) { @@ -61,11 +60,10 @@ func (s storage) SecretDelete(secret *model.Secret) error { } func (s storage) OrgSecretFind(owner, name string) (*model.Secret, error) { - secret := &model.Secret{ - Owner: owner, - Name: name, - } - return secret, wrapGet(s.engine.Get(secret)) + secret := new(model.Secret) + return secret, wrapGet(s.engine.Where( + builder.Eq{"secret_owner": owner, "secret_name": name}, + ).Get(secret)) } func (s storage) OrgSecretList(owner string, p *model.ListOptions) ([]*model.Secret, error) { @@ -74,13 +72,15 @@ func (s storage) OrgSecretList(owner string, p *model.ListOptions) ([]*model.Sec } func (s storage) GlobalSecretFind(name string) (*model.Secret, error) { - secret := &model.Secret{ - Name: name, - } - return secret, wrapGet(s.engine.Where(builder.And(builder.Eq{"secret_owner": ""}, builder.Eq{"secret_repo_id": 0})).Get(secret)) + secret := new(model.Secret) + return secret, wrapGet(s.engine.Where( + builder.Eq{"secret_owner": "", "secret_repo_id": 0, "secret_name": name}, + ).Get(secret)) } func (s storage) GlobalSecretList(p *model.ListOptions) ([]*model.Secret, error) { secrets := make([]*model.Secret, 0) - return secrets, s.paginate(p).Where(builder.And(builder.Eq{"secret_owner": ""}, builder.Eq{"secret_repo_id": 0})).OrderBy(orderSecretsBy).Find(&secrets) + return secrets, s.paginate(p).Where( + builder.Eq{"secret_owner": "", "secret_repo_id": 0}, + ).OrderBy(orderSecretsBy).Find(&secrets) } diff --git a/server/store/datastore/server_config.go b/server/store/datastore/server_config.go index e96608420..0348c1239 100644 --- a/server/store/datastore/server_config.go +++ b/server/store/datastore/server_config.go @@ -3,11 +3,8 @@ package datastore import "github.com/woodpecker-ci/woodpecker/server/model" func (s storage) ServerConfigGet(key string) (string, error) { - config := &model.ServerConfig{ - Key: key, - } - - err := wrapGet(s.engine.Get(config)) + config := new(model.ServerConfig) + err := wrapGet(s.engine.ID(key).Get(config)) if err != nil { return "", err } diff --git a/server/store/datastore/step.go b/server/store/datastore/step.go index 44fbefa7b..6bb611f88 100644 --- a/server/store/datastore/step.go +++ b/server/store/datastore/step.go @@ -15,6 +15,7 @@ package datastore import ( + "xorm.io/builder" "xorm.io/xorm" "github.com/woodpecker-ci/woodpecker/server/model" @@ -26,25 +27,24 @@ func (s storage) StepLoad(id int64) (*model.Step, error) { } func (s storage) StepFind(pipeline *model.Pipeline, pid int) (*model.Step, error) { - step := &model.Step{ - PipelineID: pipeline.ID, - PID: pid, - } - return step, wrapGet(s.engine.Get(step)) + step := new(model.Step) + return step, wrapGet(s.engine.Where( + builder.Eq{"step_pipeline_id": pipeline.ID, "step_pid": pid}, + ).Get(step)) } func (s storage) StepByUUID(uuid string) (*model.Step, error) { step := new(model.Step) - return step, wrapGet(s.engine.Where("step_uuid = ?", uuid).Get(step)) + return step, wrapGet(s.engine.Where( + builder.Eq{"step_uuid": uuid}, + ).Get(step)) } func (s storage) StepChild(pipeline *model.Pipeline, ppid int, child string) (*model.Step, error) { - step := &model.Step{ - PipelineID: pipeline.ID, - PPID: ppid, - Name: child, - } - return step, wrapGet(s.engine.Get(step)) + step := new(model.Step) + return step, wrapGet(s.engine.Where( + builder.Eq{"step_pipeline_id": pipeline.ID, "step_ppid": ppid, "step_name": child}, + ).Get(step)) } func (s storage) StepList(pipeline *model.Pipeline) ([]*model.Step, error) {