From 5b30b7dc6f22d546d479c0911a9d81c7f1d6bfd3 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 25 Jan 2025 08:51:59 +0100 Subject: [PATCH] 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"))