From 9c0fa84c952085673c0ea3f214f1dcbc349b4fd8 Mon Sep 17 00:00:00 2001 From: naiba Date: Tue, 26 May 2026 03:59:50 +0000 Subject: [PATCH] fix(cron): switch manual trigger to POST to defeat CSRF GHSA-8qhj-4f8c-j8qg: GET /api/v1/cron/:id/manual changed shared state on the agent stream, and the JWT cookie is SameSite=Lax so a victim's browser would send the cookie on a top-level cross-site GET. An attacker could trick a logged-in user into firing any of their own cron commands. Switch the route to POST: SameSite=Lax cookies are not sent on cross- site POST, closing the CSRF window without introducing a new token. Tests: - TestCronManualTriggerRejectsCrossSiteGET locks in that GET no longer resolves. - TestCronManualTriggerAcceptsSameSitePOST locks in the legitimate POST still works. Frontend (admin-frontend) updated in a follow-up commit. Co-authored-by: cloudcode --- cmd/dashboard/controller/controller.go | 2 +- cmd/dashboard/controller/cron.go | 2 +- .../controller/cron_manual_csrf_test.go | 105 ++++++++++++++++++ 3 files changed, 107 insertions(+), 2 deletions(-) create mode 100644 cmd/dashboard/controller/cron_manual_csrf_test.go diff --git a/cmd/dashboard/controller/controller.go b/cmd/dashboard/controller/controller.go index b771d861..7468ce4c 100644 --- a/cmd/dashboard/controller/controller.go +++ b/cmd/dashboard/controller/controller.go @@ -135,7 +135,7 @@ func routers(r *gin.Engine, frontendDist fs.FS) { auth.GET("/cron", listHandler(listCron)) auth.POST("/cron", commonHandler(createCron)) auth.PATCH("/cron/:id", commonHandler(updateCron)) - auth.GET("/cron/:id/manual", commonHandler(manualTriggerCron)) + auth.POST("/cron/:id/manual", commonHandler(manualTriggerCron)) auth.POST("/batch-delete/cron", commonHandler(batchDeleteCron)) auth.GET("/ddns", listHandler(listDDNS)) diff --git a/cmd/dashboard/controller/cron.go b/cmd/dashboard/controller/cron.go index abdf9a94..31027bcc 100644 --- a/cmd/dashboard/controller/cron.go +++ b/cmd/dashboard/controller/cron.go @@ -166,7 +166,7 @@ func updateCron(c *gin.Context) (any, error) { // @param id path uint true "Task ID" // @Produce json // @Success 200 {object} model.CommonResponse[any] -// @Router /cron/{id}/manual [get] +// @Router /cron/{id}/manual [post] func manualTriggerCron(c *gin.Context) (any, error) { idStr := c.Param("id") id, err := strconv.ParseUint(idStr, 10, 64) diff --git a/cmd/dashboard/controller/cron_manual_csrf_test.go b/cmd/dashboard/controller/cron_manual_csrf_test.go new file mode 100644 index 00000000..48dec831 --- /dev/null +++ b/cmd/dashboard/controller/cron_manual_csrf_test.go @@ -0,0 +1,105 @@ +package controller + +import ( + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/gin-gonic/gin" + "github.com/patrickmn/go-cache" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/driver/sqlite" + "gorm.io/gorm" + + "github.com/nezhahq/nezha/model" + "github.com/nezhahq/nezha/pkg/i18n" + "github.com/nezhahq/nezha/service/singleton" +) + +func setupCronManualTriggerFixture(t *testing.T) { + t.Helper() + + originalDB := singleton.DB + originalCache := singleton.Cache + originalLoc := singleton.Loc + originalLocalizer := singleton.Localizer + originalCron := singleton.CronShared + originalServer := singleton.ServerShared + originalUserInfo := singleton.UserInfoMap + db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) + require.NoError(t, err) + require.NoError(t, db.AutoMigrate(&model.Cron{}, &model.Server{}, &model.User{})) + + singleton.DB = db + singleton.Loc = time.UTC + singleton.Cache = cache.New(time.Minute, time.Minute) + singleton.Localizer = i18n.NewLocalizer("en_US", "nezha", "translations", i18n.Translations) + singleton.CronShared = singleton.NewCronClass() + singleton.ServerShared = singleton.NewServerClass() + singleton.UserLock.Lock() + singleton.UserInfoMap = map[uint64]model.UserInfo{100: {Role: model.RoleMember}} + singleton.UserLock.Unlock() + + cr := &model.Cron{ + Common: model.Common{ID: 7, UserID: 100}, + Name: "victim cron", + TaskType: model.CronTypeCronTask, + Command: "echo csrf-poc", + Cover: model.CronCoverIgnoreAll, + } + require.NoError(t, db.Create(cr).Error) + singleton.CronShared.Update(cr) + + t.Cleanup(func() { + singleton.DB = originalDB + singleton.Cache = originalCache + singleton.Loc = originalLoc + singleton.Localizer = originalLocalizer + singleton.CronShared = originalCron + singleton.ServerShared = originalServer + singleton.UserLock.Lock() + singleton.UserInfoMap = originalUserInfo + singleton.UserLock.Unlock() + }) +} + +func newCronManualRouter() *gin.Engine { + gin.SetMode(gin.TestMode) + r := gin.New() + r.Use(func(c *gin.Context) { + c.Set(model.CtxKeyAuthorizedUser, &model.User{ + Common: model.Common{ID: 100}, + Role: model.RoleMember, + }) + c.Next() + }) + r.POST("/api/v1/cron/:id/manual", commonHandler(manualTriggerCron)) + return r +} + +func TestCronManualTriggerRejectsCrossSiteGET(t *testing.T) { + setupCronManualTriggerFixture(t) + r := newCronManualRouter() + + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/api/v1/cron/7/manual", nil) + req.Header.Set("Origin", "https://attacker.example") + req.Header.Set("Sec-Fetch-Site", "cross-site") + r.ServeHTTP(w, req) + + assert.Equal(t, http.StatusNotFound, w.Code, "manual trigger must reject cross-site GET — the route is POST-only after the CSRF fix") +} + +func TestCronManualTriggerAcceptsSameSitePOST(t *testing.T) { + setupCronManualTriggerFixture(t) + r := newCronManualRouter() + + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/api/v1/cron/7/manual", nil) + r.ServeHTTP(w, req) + + success, errMsg := decodeCommonResponseError(t, w.Body.Bytes()) + assert.True(t, success, "owner POST must succeed: error=%q", errMsg) +}