From 0b17346cff9a9c15f95501f519a47b4e971d2786 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 25 Jan 2025 09:26:50 +0100 Subject: [PATCH] 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). --- 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 d0f917d923..39cea95c4b 100644 --- a/models/actions/variable.go +++ b/models/actions/variable.go @@ -86,7 +86,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, @@ -94,11 +94,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 deff5f471e..f053a429f9 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -3903,6 +3903,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) {