From f359ebeea53d325e0c39cb58ed77e01792151e7d 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. --- models/actions/runner.go | 15 +++++---------- models/actions/runner_test.go | 2 +- routers/web/repo/setting/runners.go | 2 +- routers/web/shared/actions/runners.go | 15 +++++++++++++-- 4 files changed, 20 insertions(+), 14 deletions(-) diff --git a/models/actions/runner.go b/models/actions/runner.go index a679d7d989..b24950d014 100644 --- a/models/actions/runner.go +++ b/models/actions/runner.go @@ -282,27 +282,22 @@ 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 { - runner, err := GetRunnerByID(ctx, id) - if err != nil { - return err - } - +func DeleteRunner(ctx context.Context, r *ActionRunner) error { // Replace the UUID, which was either based on the secret's first 16 bytes or an UUIDv4, // with a sequence of 8 0xff bytes followed by the little-endian version of the record's // identifier. This will prevent the deleted record's identifier from colliding with any // new record. b := make([]byte, 8) - binary.LittleEndian.PutUint64(b, uint64(id)) - runner.UUID = fmt.Sprintf("ffffffff-ffff-ffff-%.2x%.2x-%.2x%.2x%.2x%.2x%.2x%.2x", + binary.LittleEndian.PutUint64(b, uint64(r.ID)) + r.UUID = fmt.Sprintf("ffffffff-ffff-ffff-%.2x%.2x-%.2x%.2x%.2x%.2x%.2x%.2x", b[0], b[1], b[2], b[3], b[4], b[5], b[6], b[7]) - err = UpdateRunner(ctx, runner, "UUID") + err := UpdateRunner(ctx, r, "UUID") if err != nil { return err } - _, err = db.DeleteByID[ActionRunner](ctx, id) + _, err = db.DeleteByID[ActionRunner](ctx, r.ID) return err } diff --git a/models/actions/runner_test.go b/models/actions/runner_test.go index 26ef4c44c6..2c8d430f94 100644 --- a/models/actions/runner_test.go +++ b/models/actions/runner_test.go @@ -34,7 +34,7 @@ func TestDeleteRunner(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) before := unittest.AssertExistsAndLoadBean(t, &ActionRunner{ID: recordID}) - err := DeleteRunner(db.DefaultContext, recordID) + err := DeleteRunner(db.DefaultContext, &ActionRunner{ID: recordID}) require.NoError(t, err) var after ActionRunner 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 7ed3f88f6c..66dce1412b 100644 --- a/routers/web/shared/actions/runners.go +++ b/routers/web/shared/actions/runners.go @@ -142,10 +142,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"))