From 5b30b7dc6f22d546d479c0911a9d81c7f1d6bfd3 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 25 Jan 2025 08:51:59 +0100 Subject: [PATCH 1/6] fix(sec): web route delete runner The web route to delete action runners did not check if the ID that was given belonged to the context it was requested in, this made it possible to delete every existing runner of a instance by a authenticated user. The code was reworked to ensure that the caller of the delete runner function retrieved the runner by ID and then checks if it belongs to the context it was requested in, although this is not an optimal solution it is consistent with the context checking of other code for runners. (cherry picked from commit 567765be03d56d6c8c36bb783c330c8ca70b1aca) Conflicts: models/actions/runner.go models/actions/runner_test.go conflicting UUID bug fix and associated tests do not exist --- models/actions/runner.go | 8 ++------ routers/web/repo/setting/runners.go | 2 +- routers/web/shared/actions/runners.go | 15 +++++++++++++-- 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/models/actions/runner.go b/models/actions/runner.go index 67f003387b..2381559c8d 100644 --- a/models/actions/runner.go +++ b/models/actions/runner.go @@ -252,12 +252,8 @@ func UpdateRunner(ctx context.Context, r *ActionRunner, cols ...string) error { } // DeleteRunner deletes a runner by given ID. -func DeleteRunner(ctx context.Context, id int64) error { - if _, err := GetRunnerByID(ctx, id); err != nil { - return err - } - - _, err := db.DeleteByID[ActionRunner](ctx, id) +func DeleteRunner(ctx context.Context, r *ActionRunner) error { + _, err := db.DeleteByID[ActionRunner](ctx, r.ID) return err } diff --git a/routers/web/repo/setting/runners.go b/routers/web/repo/setting/runners.go index a47d3b45e2..9dce5d13b7 100644 --- a/routers/web/repo/setting/runners.go +++ b/routers/web/repo/setting/runners.go @@ -179,7 +179,7 @@ func RunnerDeletePost(ctx *context.Context) { ctx.ServerError("getRunnersCtx", err) return } - actions_shared.RunnerDeletePost(ctx, ctx.ParamsInt64(":runnerid"), rCtx.RedirectLink, rCtx.RedirectLink+url.PathEscape(ctx.Params(":runnerid"))) + actions_shared.RunnerDeletePost(ctx, ctx.ParamsInt64(":runnerid"), rCtx.OwnerID, rCtx.RepoID, rCtx.RedirectLink, rCtx.RedirectLink+url.PathEscape(ctx.Params(":runnerid"))) } func RedirectToDefaultSetting(ctx *context.Context) { diff --git a/routers/web/shared/actions/runners.go b/routers/web/shared/actions/runners.go index 34b7969442..733406426b 100644 --- a/routers/web/shared/actions/runners.go +++ b/routers/web/shared/actions/runners.go @@ -143,10 +143,21 @@ func RunnerResetRegistrationToken(ctx *context.Context, ownerID, repoID int64, r } // RunnerDeletePost response for deleting a runner -func RunnerDeletePost(ctx *context.Context, runnerID int64, +func RunnerDeletePost(ctx *context.Context, runnerID, ownerID, repoID int64, successRedirectTo, failedRedirectTo string, ) { - if err := actions_model.DeleteRunner(ctx, runnerID); err != nil { + runner, err := actions_model.GetRunnerByID(ctx, runnerID) + if err != nil { + ctx.ServerError("GetRunnerByID", err) + return + } + + if !runner.Editable(ownerID, repoID) { + ctx.NotFound("Editable", util.NewPermissionDeniedErrorf("no permission to edit this runner")) + return + } + + if err := actions_model.DeleteRunner(ctx, runner); err != nil { log.Warn("DeleteRunnerPost.UpdateRunner failed: %v, url: %s", err, ctx.Req.URL) ctx.Flash.Warning(ctx.Tr("actions.runners.delete_runner_failed")) From 0e82cf121d6e6e4919030857567f0bc2b795f0c6 Mon Sep 17 00:00:00 2001 From: sillyguodong <33891828+sillyguodong@users.noreply.github.com> Date: Fri, 29 Mar 2024 04:40:35 +0800 Subject: [PATCH 2/6] chore(refactor): partial port of Add API for `Variables` (#29520) The commit has, in addition to the implementation of the API, a few function refactor that are useful in backports. --- close #27801 --------- Co-authored-by: silverwind (cherry picked from commit 62b073e6f31645e446c7e8d6b5a506f61b47924e) Conflicts: - modules/util/util.go Trivial resolution, only picking the newly introduced function - routers/api/v1/swagger/options.go Trivial resolution. We don't have UserBadges, don't pick that part. - templates/swagger/v1_json.tmpl Regenerated. (cherry picked from commit 16696a42f557dd65f335a44f55881d27a3247f97) --- models/actions/variable.go | 27 ++++--- modules/util/util.go | 9 +++ modules/util/util_test.go | 5 ++ routers/web/shared/actions/variables.go | 67 ++-------------- routers/web/shared/secrets/secrets.go | 4 +- services/actions/variables.go | 100 ++++++++++++++++++++++++ 6 files changed, 139 insertions(+), 73 deletions(-) create mode 100644 services/actions/variables.go diff --git a/models/actions/variable.go b/models/actions/variable.go index 14ded60fac..b0a455e675 100644 --- a/models/actions/variable.go +++ b/models/actions/variable.go @@ -6,13 +6,11 @@ package actions import ( "context" "errors" - "fmt" "strings" "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/timeutil" - "code.gitea.io/gitea/modules/util" "xorm.io/builder" ) @@ -55,24 +53,24 @@ type FindVariablesOpts struct { db.ListOptions OwnerID int64 RepoID int64 + Name string } func (opts FindVariablesOpts) ToConds() builder.Cond { cond := builder.NewCond() + // Since we now support instance-level variables, + // there is no need to check for null values for `owner_id` and `repo_id` cond = cond.And(builder.Eq{"owner_id": opts.OwnerID}) cond = cond.And(builder.Eq{"repo_id": opts.RepoID}) + + if opts.Name != "" { + cond = cond.And(builder.Eq{"name": strings.ToUpper(opts.Name)}) + } return cond } -func GetVariableByID(ctx context.Context, variableID int64) (*ActionVariable, error) { - var variable ActionVariable - has, err := db.GetEngine(ctx).Where("id=?", variableID).Get(&variable) - if err != nil { - return nil, err - } else if !has { - return nil, fmt.Errorf("variable with id %d: %w", variableID, util.ErrNotExist) - } - return &variable, nil +func FindVariables(ctx context.Context, opts FindVariablesOpts) ([]*ActionVariable, error) { + return db.Find[ActionVariable](ctx, opts) } func UpdateVariable(ctx context.Context, variable *ActionVariable) (bool, error) { @@ -84,6 +82,13 @@ func UpdateVariable(ctx context.Context, variable *ActionVariable) (bool, error) return count != 0, err } +func DeleteVariable(ctx context.Context, id int64) error { + if _, err := db.DeleteByID[ActionVariable](ctx, id); err != nil { + return err + } + return nil +} + func GetVariablesOfRun(ctx context.Context, run *ActionRun) (map[string]string, error) { variables := map[string]string{} diff --git a/modules/util/util.go b/modules/util/util.go index e4d658d7f8..b6ea283551 100644 --- a/modules/util/util.go +++ b/modules/util/util.go @@ -220,3 +220,12 @@ func Iif[T any](condition bool, trueVal, falseVal T) T { } return falseVal } + +func ReserveLineBreakForTextarea(input string) string { + // Since the content is from a form which is a textarea, the line endings are \r\n. + // It's a standard behavior of HTML. + // But we want to store them as \n like what GitHub does. + // And users are unlikely to really need to keep the \r. + // Other than this, we should respect the original content, even leading or trailing spaces. + return strings.ReplaceAll(input, "\r\n", "\n") +} diff --git a/modules/util/util_test.go b/modules/util/util_test.go index 46c8acd7ad..344f67bb7b 100644 --- a/modules/util/util_test.go +++ b/modules/util/util_test.go @@ -236,3 +236,8 @@ func TestToPointer(t *testing.T) { val123 := 123 assert.NotSame(t, &val123, ToPointer(val123)) } + +func TestReserveLineBreakForTextarea(t *testing.T) { + assert.Equal(t, "test\ndata", ReserveLineBreakForTextarea("test\r\ndata")) + assert.Equal(t, "test\ndata\n", ReserveLineBreakForTextarea("test\r\ndata\r\n")) +} diff --git a/routers/web/shared/actions/variables.go b/routers/web/shared/actions/variables.go index 0f705399c9..79c03e4e8c 100644 --- a/routers/web/shared/actions/variables.go +++ b/routers/web/shared/actions/variables.go @@ -4,17 +4,13 @@ package actions import ( - "errors" - "regexp" - "strings" - actions_model "code.gitea.io/gitea/models/actions" "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/web" + actions_service "code.gitea.io/gitea/services/actions" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/forms" - secret_service "code.gitea.io/gitea/services/secrets" ) func SetVariablesContext(ctx *context.Context, ownerID, repoID int64) { @@ -29,41 +25,16 @@ func SetVariablesContext(ctx *context.Context, ownerID, repoID int64) { ctx.Data["Variables"] = variables } -// some regular expression of `variables` and `secrets` -// reference to: -// https://docs.github.com/en/actions/learn-github-actions/variables#naming-conventions-for-configuration-variables -// https://docs.github.com/en/actions/security-guides/encrypted-secrets#naming-your-secrets -var ( - forbiddenEnvNameCIRx = regexp.MustCompile("(?i)^CI") -) - -func envNameCIRegexMatch(name string) error { - if forbiddenEnvNameCIRx.MatchString(name) { - log.Error("Env Name cannot be ci") - return errors.New("env name cannot be ci") - } - return nil -} - func CreateVariable(ctx *context.Context, ownerID, repoID int64, redirectURL string) { form := web.GetForm(ctx).(*forms.EditVariableForm) - if err := secret_service.ValidateName(form.Name); err != nil { - ctx.JSONError(err.Error()) - return - } - - if err := envNameCIRegexMatch(form.Name); err != nil { - ctx.JSONError(err.Error()) - return - } - - v, err := actions_model.InsertVariable(ctx, ownerID, repoID, form.Name, ReserveLineBreakForTextarea(form.Data)) + v, err := actions_service.CreateVariable(ctx, ownerID, repoID, form.Name, form.Data) if err != nil { - log.Error("InsertVariable error: %v", err) + log.Error("CreateVariable: %v", err) ctx.JSONError(ctx.Tr("actions.variables.creation.failed")) return } + ctx.Flash.Success(ctx.Tr("actions.variables.creation.success", v.Name)) ctx.JSONRedirect(redirectURL) } @@ -72,23 +43,8 @@ func UpdateVariable(ctx *context.Context, redirectURL string) { id := ctx.ParamsInt64(":variable_id") form := web.GetForm(ctx).(*forms.EditVariableForm) - if err := secret_service.ValidateName(form.Name); err != nil { - ctx.JSONError(err.Error()) - return - } - - if err := envNameCIRegexMatch(form.Name); err != nil { - ctx.JSONError(err.Error()) - return - } - - ok, err := actions_model.UpdateVariable(ctx, &actions_model.ActionVariable{ - ID: id, - Name: strings.ToUpper(form.Name), - Data: ReserveLineBreakForTextarea(form.Data), - }) - if err != nil || !ok { - log.Error("UpdateVariable error: %v", err) + if ok, err := actions_service.UpdateVariable(ctx, id, form.Name, form.Data); err != nil || !ok { + log.Error("UpdateVariable: %v", err) ctx.JSONError(ctx.Tr("actions.variables.update.failed")) return } @@ -99,7 +55,7 @@ func UpdateVariable(ctx *context.Context, redirectURL string) { func DeleteVariable(ctx *context.Context, redirectURL string) { id := ctx.ParamsInt64(":variable_id") - if _, err := db.DeleteByBean(ctx, &actions_model.ActionVariable{ID: id}); err != nil { + if err := actions_service.DeleteVariableByID(ctx, id); err != nil { log.Error("Delete variable [%d] failed: %v", id, err) ctx.JSONError(ctx.Tr("actions.variables.deletion.failed")) return @@ -107,12 +63,3 @@ func DeleteVariable(ctx *context.Context, redirectURL string) { ctx.Flash.Success(ctx.Tr("actions.variables.deletion.success")) ctx.JSONRedirect(redirectURL) } - -func ReserveLineBreakForTextarea(input string) string { - // Since the content is from a form which is a textarea, the line endings are \r\n. - // It's a standard behavior of HTML. - // But we want to store them as \n like what GitHub does. - // And users are unlikely to really need to keep the \r. - // Other than this, we should respect the original content, even leading or trailing spaces. - return strings.ReplaceAll(input, "\r\n", "\n") -} diff --git a/routers/web/shared/secrets/secrets.go b/routers/web/shared/secrets/secrets.go index 73505ec372..3bd421f86a 100644 --- a/routers/web/shared/secrets/secrets.go +++ b/routers/web/shared/secrets/secrets.go @@ -7,8 +7,8 @@ import ( "code.gitea.io/gitea/models/db" secret_model "code.gitea.io/gitea/models/secret" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web" - "code.gitea.io/gitea/routers/web/shared/actions" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/forms" secret_service "code.gitea.io/gitea/services/secrets" @@ -27,7 +27,7 @@ func SetSecretsContext(ctx *context.Context, ownerID, repoID int64) { func PerformSecretsPost(ctx *context.Context, ownerID, repoID int64, redirectURL string) { form := web.GetForm(ctx).(*forms.AddSecretForm) - s, _, err := secret_service.CreateOrUpdateSecret(ctx, ownerID, repoID, form.Name, actions.ReserveLineBreakForTextarea(form.Data)) + s, _, err := secret_service.CreateOrUpdateSecret(ctx, ownerID, repoID, form.Name, util.ReserveLineBreakForTextarea(form.Data)) if err != nil { log.Error("CreateOrUpdateSecret failed: %v", err) ctx.JSONError(ctx.Tr("secrets.creation.failed")) diff --git a/services/actions/variables.go b/services/actions/variables.go new file mode 100644 index 0000000000..8dde9c4af5 --- /dev/null +++ b/services/actions/variables.go @@ -0,0 +1,100 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package actions + +import ( + "context" + "regexp" + "strings" + + actions_model "code.gitea.io/gitea/models/actions" + "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/util" + secret_service "code.gitea.io/gitea/services/secrets" +) + +func CreateVariable(ctx context.Context, ownerID, repoID int64, name, data string) (*actions_model.ActionVariable, error) { + if err := secret_service.ValidateName(name); err != nil { + return nil, err + } + + if err := envNameCIRegexMatch(name); err != nil { + return nil, err + } + + v, err := actions_model.InsertVariable(ctx, ownerID, repoID, name, util.ReserveLineBreakForTextarea(data)) + if err != nil { + return nil, err + } + + return v, nil +} + +func UpdateVariable(ctx context.Context, variableID int64, name, data string) (bool, error) { + if err := secret_service.ValidateName(name); err != nil { + return false, err + } + + if err := envNameCIRegexMatch(name); err != nil { + return false, err + } + + return actions_model.UpdateVariable(ctx, &actions_model.ActionVariable{ + ID: variableID, + Name: strings.ToUpper(name), + Data: util.ReserveLineBreakForTextarea(data), + }) +} + +func DeleteVariableByID(ctx context.Context, variableID int64) error { + return actions_model.DeleteVariable(ctx, variableID) +} + +func DeleteVariableByName(ctx context.Context, ownerID, repoID int64, name string) error { + if err := secret_service.ValidateName(name); err != nil { + return err + } + + if err := envNameCIRegexMatch(name); err != nil { + return err + } + + v, err := GetVariable(ctx, actions_model.FindVariablesOpts{ + OwnerID: ownerID, + RepoID: repoID, + Name: name, + }) + if err != nil { + return err + } + + return actions_model.DeleteVariable(ctx, v.ID) +} + +func GetVariable(ctx context.Context, opts actions_model.FindVariablesOpts) (*actions_model.ActionVariable, error) { + vars, err := actions_model.FindVariables(ctx, opts) + if err != nil { + return nil, err + } + if len(vars) != 1 { + return nil, util.NewNotExistErrorf("variable not found") + } + return vars[0], nil +} + +// some regular expression of `variables` and `secrets` +// reference to: +// https://docs.github.com/en/actions/learn-github-actions/variables#naming-conventions-for-configuration-variables +// https://docs.github.com/en/actions/security-guides/encrypted-secrets#naming-your-secrets +var ( + forbiddenEnvNameCIRx = regexp.MustCompile("(?i)^CI") +) + +func envNameCIRegexMatch(name string) error { + if forbiddenEnvNameCIRx.MatchString(name) { + log.Error("Env Name cannot be ci") + return util.NewInvalidArgumentErrorf("env name cannot be ci") + } + return nil +} From 4c8c215b75bb480bcce7fbd31bfdb8bed6a4598a Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 25 Jan 2025 09:26:50 +0100 Subject: [PATCH 3/6] fix(sec): web route update and delete runner variables The web route to update and delete variables of runners did not check if the ID that was given belonged to the context it was requested in, this made it possible to update and delete every existing runner variable of a instance for any authenticated user. The code has been reworked to always take into account the context of the request (owner and repository ID). (cherry picked from commit 5cb8fdfc8b9213cc368cd074aac93a1327ea20b0) --- models/actions/variable.go | 10 ++++------ options/locale/locale_en-US.ini | 1 + routers/web/repo/setting/variables.go | 4 ++-- routers/web/shared/actions/variables.go | 24 ++++++++++++++++-------- services/actions/variables.go | 17 ++++++++--------- 5 files changed, 31 insertions(+), 25 deletions(-) diff --git a/models/actions/variable.go b/models/actions/variable.go index b0a455e675..4aab2db3ba 100644 --- a/models/actions/variable.go +++ b/models/actions/variable.go @@ -74,7 +74,7 @@ func FindVariables(ctx context.Context, opts FindVariablesOpts) ([]*ActionVariab } func UpdateVariable(ctx context.Context, variable *ActionVariable) (bool, error) { - count, err := db.GetEngine(ctx).ID(variable.ID).Cols("name", "data"). + count, err := db.GetEngine(ctx).ID(variable.ID).Where("owner_id = ? AND repo_id = ?", variable.OwnerID, variable.RepoID).Cols("name", "data"). Update(&ActionVariable{ Name: variable.Name, Data: variable.Data, @@ -82,11 +82,9 @@ func UpdateVariable(ctx context.Context, variable *ActionVariable) (bool, error) return count != 0, err } -func DeleteVariable(ctx context.Context, id int64) error { - if _, err := db.DeleteByID[ActionVariable](ctx, id); err != nil { - return err - } - return nil +func DeleteVariable(ctx context.Context, variableID, ownerID, repoID int64) (bool, error) { + count, err := db.GetEngine(ctx).Table("action_variable").Where("id = ? AND owner_id = ? AND repo_id = ?", variableID, ownerID, repoID).Delete() + return count != 0, err } func GetVariablesOfRun(ctx context.Context, run *ActionRun) (map[string]string, error) { diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 64566ce742..e19b09e165 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -3743,6 +3743,7 @@ variables.deletion.description = Removing a variable is permanent and cannot be variables.description = Variables will be passed to certain actions and cannot be read otherwise. variables.id_not_exist = Variable with ID %d does not exist. variables.edit = Edit Variable +variables.not_found = Failed to find the variable. variables.deletion.failed = Failed to remove variable. variables.deletion.success = The variable has been removed. variables.creation.failed = Failed to add variable. diff --git a/routers/web/repo/setting/variables.go b/routers/web/repo/setting/variables.go index 45b6c0f39a..4fb8c06e84 100644 --- a/routers/web/repo/setting/variables.go +++ b/routers/web/repo/setting/variables.go @@ -127,7 +127,7 @@ func VariableUpdate(ctx *context.Context) { return } - shared.UpdateVariable(ctx, vCtx.RedirectLink) + shared.UpdateVariable(ctx, vCtx.OwnerID, vCtx.RepoID, vCtx.RedirectLink) } func VariableDelete(ctx *context.Context) { @@ -136,5 +136,5 @@ func VariableDelete(ctx *context.Context) { ctx.ServerError("getVariablesCtx", err) return } - shared.DeleteVariable(ctx, vCtx.RedirectLink) + shared.DeleteVariable(ctx, vCtx.OwnerID, vCtx.RepoID, vCtx.RedirectLink) } diff --git a/routers/web/shared/actions/variables.go b/routers/web/shared/actions/variables.go index 79c03e4e8c..47f1176f46 100644 --- a/routers/web/shared/actions/variables.go +++ b/routers/web/shared/actions/variables.go @@ -39,25 +39,33 @@ func CreateVariable(ctx *context.Context, ownerID, repoID int64, redirectURL str ctx.JSONRedirect(redirectURL) } -func UpdateVariable(ctx *context.Context, redirectURL string) { +func UpdateVariable(ctx *context.Context, ownerID, repoID int64, redirectURL string) { id := ctx.ParamsInt64(":variable_id") form := web.GetForm(ctx).(*forms.EditVariableForm) - if ok, err := actions_service.UpdateVariable(ctx, id, form.Name, form.Data); err != nil || !ok { - log.Error("UpdateVariable: %v", err) - ctx.JSONError(ctx.Tr("actions.variables.update.failed")) + if ok, err := actions_service.UpdateVariable(ctx, id, ownerID, repoID, form.Name, form.Data); err != nil || !ok { + if !ok { + ctx.JSONError(ctx.Tr("actions.variables.not_found")) + } else { + log.Error("UpdateVariable: %v", err) + ctx.JSONError(ctx.Tr("actions.variables.update.failed")) + } return } ctx.Flash.Success(ctx.Tr("actions.variables.update.success")) ctx.JSONRedirect(redirectURL) } -func DeleteVariable(ctx *context.Context, redirectURL string) { +func DeleteVariable(ctx *context.Context, ownerID, repoID int64, redirectURL string) { id := ctx.ParamsInt64(":variable_id") - if err := actions_service.DeleteVariableByID(ctx, id); err != nil { - log.Error("Delete variable [%d] failed: %v", id, err) - ctx.JSONError(ctx.Tr("actions.variables.deletion.failed")) + if ok, err := actions_model.DeleteVariable(ctx, id, ownerID, repoID); err != nil || !ok { + if !ok { + ctx.JSONError(ctx.Tr("actions.variables.not_found")) + } else { + log.Error("Delete variable [%d] failed: %v", id, err) + ctx.JSONError(ctx.Tr("actions.variables.deletion.failed")) + } return } ctx.Flash.Success(ctx.Tr("actions.variables.deletion.success")) diff --git a/services/actions/variables.go b/services/actions/variables.go index 8dde9c4af5..a5703898ab 100644 --- a/services/actions/variables.go +++ b/services/actions/variables.go @@ -31,7 +31,7 @@ func CreateVariable(ctx context.Context, ownerID, repoID int64, name, data strin return v, nil } -func UpdateVariable(ctx context.Context, variableID int64, name, data string) (bool, error) { +func UpdateVariable(ctx context.Context, variableID, ownerID, repoID int64, name, data string) (bool, error) { if err := secret_service.ValidateName(name); err != nil { return false, err } @@ -41,16 +41,14 @@ func UpdateVariable(ctx context.Context, variableID int64, name, data string) (b } return actions_model.UpdateVariable(ctx, &actions_model.ActionVariable{ - ID: variableID, - Name: strings.ToUpper(name), - Data: util.ReserveLineBreakForTextarea(data), + ID: variableID, + Name: strings.ToUpper(name), + Data: util.ReserveLineBreakForTextarea(data), + OwnerID: ownerID, + RepoID: repoID, }) } -func DeleteVariableByID(ctx context.Context, variableID int64) error { - return actions_model.DeleteVariable(ctx, variableID) -} - func DeleteVariableByName(ctx context.Context, ownerID, repoID int64, name string) error { if err := secret_service.ValidateName(name); err != nil { return err @@ -69,7 +67,8 @@ func DeleteVariableByName(ctx context.Context, ownerID, repoID int64, name strin return err } - return actions_model.DeleteVariable(ctx, v.ID) + _, err = actions_model.DeleteVariable(ctx, v.ID, ownerID, repoID) + return err } func GetVariable(ctx context.Context, opts actions_model.FindVariablesOpts) (*actions_model.ActionVariable, error) { From 6e13dd44d61965e86fa25d8465cccd076461bb80 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 25 Jan 2025 12:16:04 +0100 Subject: [PATCH 4/6] fix(sec): add tests for web route delete runner Exhaustively test each combination of deleting and updating a action runner via the web route. Although updating an action runner was not impacted, its good to have a test nonetheless. (cherry picked from commit 4ace0e938e7c9efaa40cf17e9440b423ee572375) --- .../TestRunnerModification/action_runner.yml | 31 +++++ tests/integration/runner_test.go | 130 ++++++++++++++++++ 2 files changed, 161 insertions(+) create mode 100644 tests/integration/fixtures/TestRunnerModification/action_runner.yml create mode 100644 tests/integration/runner_test.go diff --git a/tests/integration/fixtures/TestRunnerModification/action_runner.yml b/tests/integration/fixtures/TestRunnerModification/action_runner.yml new file mode 100644 index 0000000000..95599b19bd --- /dev/null +++ b/tests/integration/fixtures/TestRunnerModification/action_runner.yml @@ -0,0 +1,31 @@ +- + id: 1001 + uuid: "43b5d4d3-401b-42f9-94df-a9d45b447b82" + name: "User runner" + owner_id: 2 + repo_id: 0 + deleted: 0 + +- + id: 1002 + uuid: "bdc77f4f-2b2b-442d-bd44-e808f4306347" + name: "Organisation runner" + owner_id: 3 + repo_id: 0 + deleted: 0 + +- + id: 1003 + uuid: "9268bc8c-efbf-4dbe-aeb5-945733cdd098" + name: "Repository runner" + owner_id: 0 + repo_id: 1 + deleted: 0 + +- + id: 1004 + uuid: "fb857e63-c0ce-4571-a6c9-fde26c128073" + name: "Global runner" + owner_id: 0 + repo_id: 0 + deleted: 0 diff --git a/tests/integration/runner_test.go b/tests/integration/runner_test.go new file mode 100644 index 0000000000..bab2a67230 --- /dev/null +++ b/tests/integration/runner_test.go @@ -0,0 +1,130 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package integration + +import ( + "fmt" + "net/http" + "testing" + + actions_model "code.gitea.io/gitea/models/actions" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + forgejo_context "code.gitea.io/gitea/services/context" + "code.gitea.io/gitea/tests" + + "github.com/stretchr/testify/assert" +) + +func TestRunnerModification(t *testing.T) { + defer tests.AddFixtures("tests/integration/fixtures/TestRunnerModification")() + defer tests.PrepareTestEnv(t)() + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + userRunner := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunner{ID: 1001, OwnerID: user.ID}) + userURL := "/user/settings/actions/runners" + org := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3, Type: user_model.UserTypeOrganization}) + orgRunner := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunner{ID: 1002, OwnerID: org.ID}) + orgURL := "/org/" + org.Name + "/settings/actions/runners" + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1, OwnerID: user.ID}) + repoRunner := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunner{ID: 1003, RepoID: repo.ID}) + repoURL := "/" + repo.FullName() + "/settings/actions/runners" + admin := unittest.AssertExistsAndLoadBean(t, &user_model.User{IsAdmin: true}) + globalRunner := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunner{ID: 1004}, "owner_id = 0 AND repo_id = 0") + adminURL := "/admin/actions/runners" + + adminSess := loginUser(t, admin.Name) + adminCSRF := GetCSRF(t, adminSess, "/") + sess := loginUser(t, user.Name) + csrf := GetCSRF(t, sess, "/") + + test := func(t *testing.T, fail bool, baseURL string, id int64) { + defer tests.PrintCurrentTest(t, 1)() + t.Helper() + + sess := sess + csrf := csrf + if baseURL == adminURL { + sess = adminSess + csrf = adminCSRF + } + + req := NewRequestWithValues(t, "POST", baseURL+fmt.Sprintf("/%d", id), map[string]string{ + "_csrf": csrf, + "description": "New Description", + }) + if fail { + sess.MakeRequest(t, req, http.StatusNotFound) + } else { + sess.MakeRequest(t, req, http.StatusSeeOther) + flashCookie := sess.GetCookie(forgejo_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.EqualValues(t, "success%3DRunner%2Bupdated%2Bsuccessfully", flashCookie.Value) + } + + req = NewRequestWithValues(t, "POST", baseURL+fmt.Sprintf("/%d/delete", id), map[string]string{ + "_csrf": csrf, + }) + if fail { + sess.MakeRequest(t, req, http.StatusNotFound) + } else { + sess.MakeRequest(t, req, http.StatusOK) + flashCookie := sess.GetCookie(forgejo_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.EqualValues(t, "success%3DRunner%2Bdeleted%2Bsuccessfully", flashCookie.Value) + } + } + + t.Run("User runner", func(t *testing.T) { + t.Run("Organisation", func(t *testing.T) { + test(t, true, orgURL, userRunner.ID) + }) + t.Run("Repository", func(t *testing.T) { + test(t, true, repoURL, userRunner.ID) + }) + t.Run("User", func(t *testing.T) { + test(t, false, userURL, userRunner.ID) + }) + }) + + t.Run("Organisation runner", func(t *testing.T) { + t.Run("Repository", func(t *testing.T) { + test(t, true, repoURL, orgRunner.ID) + }) + t.Run("User", func(t *testing.T) { + test(t, true, userURL, orgRunner.ID) + }) + t.Run("Organisation", func(t *testing.T) { + test(t, false, orgURL, orgRunner.ID) + }) + }) + + t.Run("Repository runner", func(t *testing.T) { + t.Run("Organisation", func(t *testing.T) { + test(t, true, orgURL, repoRunner.ID) + }) + t.Run("User", func(t *testing.T) { + test(t, true, userURL, repoRunner.ID) + }) + t.Run("Repository", func(t *testing.T) { + test(t, false, repoURL, repoRunner.ID) + }) + }) + + t.Run("Global runner", func(t *testing.T) { + t.Run("Organisation", func(t *testing.T) { + test(t, true, orgURL, globalRunner.ID) + }) + t.Run("User", func(t *testing.T) { + test(t, true, userURL, globalRunner.ID) + }) + t.Run("Repository", func(t *testing.T) { + test(t, true, repoURL, globalRunner.ID) + }) + t.Run("Admin", func(t *testing.T) { + test(t, false, adminURL, globalRunner.ID) + }) + }) +} From 4c3227eeed5b3f31c1174f6ccf834a277875bfd9 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 25 Jan 2025 13:03:49 +0100 Subject: [PATCH 5/6] fix(sec): web route test edit and delete variable Exhaustively test each combination of deleting and updating a action action variable via the web route. (cherry picked from commit cd0334f85ac46db7b1b42770c9b4e809ea6f4254) --- tests/integration/actions_variables_test.go | 150 ++++++++++++++++++ .../action_variable.yml | 31 ++++ 2 files changed, 181 insertions(+) create mode 100644 tests/integration/actions_variables_test.go create mode 100644 tests/integration/fixtures/TestActionVariablesModification/action_variable.yml diff --git a/tests/integration/actions_variables_test.go b/tests/integration/actions_variables_test.go new file mode 100644 index 0000000000..0179a543dc --- /dev/null +++ b/tests/integration/actions_variables_test.go @@ -0,0 +1,150 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package integration + +import ( + "fmt" + "net/http" + "testing" + + actions_model "code.gitea.io/gitea/models/actions" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + forgejo_context "code.gitea.io/gitea/services/context" + "code.gitea.io/gitea/tests" + + "github.com/stretchr/testify/assert" +) + +func TestActionVariablesModification(t *testing.T) { + defer tests.AddFixtures("tests/integration/fixtures/TestActionVariablesModification")() + defer tests.PrepareTestEnv(t)() + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + userVariable := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionVariable{ID: 1001, OwnerID: user.ID}) + userURL := "/user/settings/actions/variables" + org := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3, Type: user_model.UserTypeOrganization}) + orgVariable := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionVariable{ID: 1002, OwnerID: org.ID}) + orgURL := "/org/" + org.Name + "/settings/actions/variables" + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1, OwnerID: user.ID}) + repoVariable := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionVariable{ID: 1003, RepoID: repo.ID}) + repoURL := "/" + repo.FullName() + "/settings/actions/variables" + admin := unittest.AssertExistsAndLoadBean(t, &user_model.User{IsAdmin: true}) + globalVariable := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionVariable{ID: 1004}, "owner_id = 0 AND repo_id = 0") + adminURL := "/admin/actions/variables" + + adminSess := loginUser(t, admin.Name) + adminCSRF := GetCSRF(t, adminSess, "/") + sess := loginUser(t, user.Name) + csrf := GetCSRF(t, sess, "/") + + type errorJSON struct { + Error string `json:"errorMessage"` + } + + test := func(t *testing.T, fail bool, baseURL string, id int64) { + defer tests.PrintCurrentTest(t, 1)() + t.Helper() + + sess := sess + csrf := csrf + if baseURL == adminURL { + sess = adminSess + csrf = adminCSRF + } + + req := NewRequestWithValues(t, "POST", baseURL+fmt.Sprintf("/%d/edit", id), map[string]string{ + "_csrf": csrf, + "name": "glados_quote", + "data": "I'm fine. Two plus two is...ten, in base four, I'm fine!", + }) + if fail { + resp := sess.MakeRequest(t, req, http.StatusBadRequest) + var error errorJSON + DecodeJSON(t, resp, &error) + assert.EqualValues(t, "Failed to find the variable.", error.Error) + } else { + sess.MakeRequest(t, req, http.StatusOK) + flashCookie := sess.GetCookie(forgejo_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.EqualValues(t, "success%3DThe%2Bvariable%2Bhas%2Bbeen%2Bedited.", flashCookie.Value) + } + + req = NewRequestWithValues(t, "POST", baseURL+fmt.Sprintf("/%d/delete", id), map[string]string{ + "_csrf": csrf, + }) + if fail { + resp := sess.MakeRequest(t, req, http.StatusBadRequest) + var error errorJSON + DecodeJSON(t, resp, &error) + assert.EqualValues(t, "Failed to find the variable.", error.Error) + } else { + sess.MakeRequest(t, req, http.StatusOK) + flashCookie := sess.GetCookie(forgejo_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.EqualValues(t, "success%3DThe%2Bvariable%2Bhas%2Bbeen%2Bremoved.", flashCookie.Value) + } + } + + t.Run("User variable", func(t *testing.T) { + t.Run("Organisation", func(t *testing.T) { + test(t, true, orgURL, userVariable.ID) + }) + t.Run("Repository", func(t *testing.T) { + test(t, true, repoURL, userVariable.ID) + }) + t.Run("Admin", func(t *testing.T) { + test(t, true, adminURL, userVariable.ID) + }) + t.Run("User", func(t *testing.T) { + test(t, false, userURL, userVariable.ID) + }) + }) + + t.Run("Organisation variable", func(t *testing.T) { + t.Run("Repository", func(t *testing.T) { + test(t, true, repoURL, orgVariable.ID) + }) + t.Run("User", func(t *testing.T) { + test(t, true, userURL, orgVariable.ID) + }) + t.Run("Admin", func(t *testing.T) { + test(t, true, adminURL, userVariable.ID) + }) + t.Run("Organisation", func(t *testing.T) { + test(t, false, orgURL, orgVariable.ID) + }) + }) + + t.Run("Repository variable", func(t *testing.T) { + t.Run("Organisation", func(t *testing.T) { + test(t, true, orgURL, repoVariable.ID) + }) + t.Run("User", func(t *testing.T) { + test(t, true, userURL, repoVariable.ID) + }) + t.Run("Admin", func(t *testing.T) { + test(t, true, adminURL, userVariable.ID) + }) + t.Run("Repository", func(t *testing.T) { + test(t, false, repoURL, repoVariable.ID) + }) + }) + + t.Run("Global variable", func(t *testing.T) { + t.Run("Organisation", func(t *testing.T) { + test(t, true, orgURL, globalVariable.ID) + }) + t.Run("User", func(t *testing.T) { + test(t, true, userURL, globalVariable.ID) + }) + t.Run("Repository", func(t *testing.T) { + test(t, true, repoURL, globalVariable.ID) + }) + t.Run("Admin", func(t *testing.T) { + test(t, false, adminURL, globalVariable.ID) + }) + }) +} diff --git a/tests/integration/fixtures/TestActionVariablesModification/action_variable.yml b/tests/integration/fixtures/TestActionVariablesModification/action_variable.yml new file mode 100644 index 0000000000..925838d0f0 --- /dev/null +++ b/tests/integration/fixtures/TestActionVariablesModification/action_variable.yml @@ -0,0 +1,31 @@ +- + id: 1001 + name: GLADOS_QUOTE + owner_id: 2 + repo_id: 0 + data: "" + created_unix: 1737000000 + +- + id: 1002 + name: GLADOS_QUOTE + owner_id: 3 + repo_id: 0 + data: "" + created_unix: 1737000001 + +- + id: 1003 + name: GLADOS_QUOTE + owner_id: 0 + repo_id: 1 + data: "" + created_unix: 1737000002 + +- + id: 1004 + name: GLADOS_QUOTE + owner_id: 0 + repo_id: 0 + data: "" + created_unix: 1737000003 From c8293d0e3ccd04954a68a572279f477c62efb7a6 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sat, 25 Jan 2025 16:36:35 +0100 Subject: [PATCH 6/6] chore(refactor): remove deadcode from port of Add API for `Variables` (#29520) --- .deadcode-out | 1 - models/actions/variable.go | 4 ---- services/actions/variables.go | 33 --------------------------------- 3 files changed, 38 deletions(-) diff --git a/.deadcode-out b/.deadcode-out index f305839cb3..80359e7dbb 100644 --- a/.deadcode-out +++ b/.deadcode-out @@ -22,7 +22,6 @@ package "code.gitea.io/gitea/models/actions" func (ScheduleList).GetRepoIDs func (ScheduleList).LoadTriggerUser func (ScheduleList).LoadRepos - func GetVariableByID package "code.gitea.io/gitea/models/asymkey" func (ErrGPGKeyAccessDenied).Error diff --git a/models/actions/variable.go b/models/actions/variable.go index 4aab2db3ba..bf3e28abab 100644 --- a/models/actions/variable.go +++ b/models/actions/variable.go @@ -69,10 +69,6 @@ func (opts FindVariablesOpts) ToConds() builder.Cond { return cond } -func FindVariables(ctx context.Context, opts FindVariablesOpts) ([]*ActionVariable, error) { - return db.Find[ActionVariable](ctx, opts) -} - func UpdateVariable(ctx context.Context, variable *ActionVariable) (bool, error) { count, err := db.GetEngine(ctx).ID(variable.ID).Where("owner_id = ? AND repo_id = ?", variable.OwnerID, variable.RepoID).Cols("name", "data"). Update(&ActionVariable{ diff --git a/services/actions/variables.go b/services/actions/variables.go index a5703898ab..2824073336 100644 --- a/services/actions/variables.go +++ b/services/actions/variables.go @@ -49,39 +49,6 @@ func UpdateVariable(ctx context.Context, variableID, ownerID, repoID int64, name }) } -func DeleteVariableByName(ctx context.Context, ownerID, repoID int64, name string) error { - if err := secret_service.ValidateName(name); err != nil { - return err - } - - if err := envNameCIRegexMatch(name); err != nil { - return err - } - - v, err := GetVariable(ctx, actions_model.FindVariablesOpts{ - OwnerID: ownerID, - RepoID: repoID, - Name: name, - }) - if err != nil { - return err - } - - _, err = actions_model.DeleteVariable(ctx, v.ID, ownerID, repoID) - return err -} - -func GetVariable(ctx context.Context, opts actions_model.FindVariablesOpts) (*actions_model.ActionVariable, error) { - vars, err := actions_model.FindVariables(ctx, opts) - if err != nil { - return nil, err - } - if len(vars) != 1 { - return nil, util.NewNotExistErrorf("variable not found") - } - return vars[0], nil -} - // some regular expression of `variables` and `secrets` // reference to: // https://docs.github.com/en/actions/learn-github-actions/variables#naming-conventions-for-configuration-variables