route: bypass require signin check for trigger repo tasks (#6079)

* route: bypass require signin check for trigger repo tasks

* CHANGELOG

* Fix lint errors

CHANGELOG.md

@@ -51,6 +51,7 @@ All notable changes to Gogs are documented in this file.
 - Enable Federated Avatar Lookup could cause server to crash. [#5848](https://github.com/gogs/gogs/issues/5848)
 - Private repositories are hidden in the organization's view. [#5869](https://github.com/gogs/gogs/issues/5869)
 - Server error when changing email address in user settings page. [#5899](https://github.com/gogs/gogs/issues/5899)
+- Webhooks are not fired after push when `[service] REQUIRE_SIGNIN_VIEW = true`.
 
 ### Removed
 

internal/cmd/hook.go

@@ -238,9 +238,10 @@ func runHookPostReceive(c *cli.Context) error {
 		reqURL := fmt.Sprintf("%s%s/%s/tasks/trigger?%s", conf.Server.LocalRootURL, options.RepoUserName, options.RepoName, q.Encode())
 		log.Trace("Trigger task: %s", reqURL)
 
-		resp, err := httplib.Head(reqURL).SetTLSClientConfig(&tls.Config{
-			InsecureSkipVerify: true,
-		}).Response()
+		resp, err := httplib.Get(reqURL).
+			SetTLSClientConfig(&tls.Config{
+				InsecureSkipVerify: true,
+			}).Response()
 		if err == nil {
 			_ = resp.Body.Close()
 			if resp.StatusCode/100 != 2 {

internal/cmd/web.go

@@ -614,7 +614,6 @@ func runWeb(c *cli.Context) error {
 			m.Get("", repo.Home)
 			m.Get("/stars", repo.Stars)
 			m.Get("/watchers", repo.Watchers)
-			m.Head("/tasks/trigger", repo.TriggerTask) // TODO: Without session and CSRF
 		}, ignSignIn, context.RepoAssignment(), context.RepoRef())
 		// ***** END: Repository *****
 
@@ -654,6 +653,8 @@ func runWeb(c *cli.Context) error {
 	// ***************************
 
 	m.Group("/:username/:reponame", func() {
+		m.Get("/tasks/trigger", repo.TriggerTask)
+
 		m.Group("/info/lfs", func() {
 			lfs.RegisterRoutes(m.Router)
 		})

internal/route/lfs/route.go

@@ -85,7 +85,7 @@ func authenticate() macaron.Handler {
 				// Once we found the token, we're supposed to find its related user,
 				// thus any error is unexpected.
 				internalServerError(c.Resp)
-				log.Error("Failed to get user: %v", err)
+				log.Error("Failed to get user [id: %d]: %v", token.UserID, err)
 				return
 			}
 		}

internal/route/repo/pull.go

@@ -19,7 +19,6 @@ import (
 	"gogs.io/gogs/internal/db"
 	"gogs.io/gogs/internal/form"
 	"gogs.io/gogs/internal/gitutil"
-	"gogs.io/gogs/internal/tool"
 )
 
 const (
@@ -744,51 +743,3 @@ func CompareAndPullRequestPost(c *context.Context, f form.NewIssue) {
 	log.Trace("Pull request created: %d/%d", repo.ID, pullIssue.ID)
 	c.Redirect(c.Repo.RepoLink + "/pulls/" + com.ToStr(pullIssue.Index))
 }
-
-func parseOwnerAndRepo(c *context.Context) (*db.User, *db.Repository) {
-	owner, err := db.GetUserByName(c.Params(":username"))
-	if err != nil {
-		c.NotFoundOrError(err, "get user by name")
-		return nil, nil
-	}
-
-	repo, err := db.GetRepositoryByName(owner.ID, c.Params(":reponame"))
-	if err != nil {
-		c.NotFoundOrError(err, "get repository by name")
-		return nil, nil
-	}
-
-	return owner, repo
-}
-
-func TriggerTask(c *context.Context) {
-	pusherID := c.QueryInt64("pusher")
-	branch := c.Query("branch")
-	secret := c.Query("secret")
-	if len(branch) == 0 || len(secret) == 0 || pusherID <= 0 {
-		c.NotFound()
-		log.Trace("TriggerTask: branch or secret is empty, or pusher ID is not valid")
-		return
-	}
-	owner, repo := parseOwnerAndRepo(c)
-	if c.Written() {
-		return
-	}
-	if secret != tool.MD5(owner.Salt) {
-		c.NotFound()
-		log.Trace("TriggerTask [%s/%s]: invalid secret", owner.Name, repo.Name)
-		return
-	}
-
-	pusher, err := db.GetUserByID(pusherID)
-	if err != nil {
-		c.NotFoundOrError(err, "get user by ID")
-		return
-	}
-
-	log.Trace("TriggerTask '%s/%s' by '%s'", repo.Name, branch, pusher.Name)
-
-	go db.HookQueue.Add(repo.ID)
-	go db.AddTestPullRequestTask(pusher, repo.ID, branch, true)
-	c.Status(202)
-}

internal/route/repo/tasks.go

@@ -0,0 +1,74 @@
+// Copyright 2020 The Gogs Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package repo
+
+import (
+	"net/http"
+
+	"gopkg.in/macaron.v1"
+	log "unknwon.dev/clog/v2"
+
+	"gogs.io/gogs/internal/db"
+	"gogs.io/gogs/internal/tool"
+)
+
+func TriggerTask(c *macaron.Context) {
+	branch := c.Query("branch")
+	pusherID := c.QueryInt64("pusher")
+	secret := c.Query("secret")
+	if branch == "" || pusherID <= 0 || secret == "" {
+		c.Error(http.StatusBadRequest, "Incomplete branch, pusher or secret")
+		return
+	}
+
+	username := c.Params(":username")
+	reponame := c.Params(":reponame")
+
+	owner, err := db.Users.GetByUsername(username)
+	if err != nil {
+		if db.IsErrUserNotExist(err) {
+			c.Error(http.StatusBadRequest, "Owner does not exist")
+		} else {
+			c.Status(http.StatusInternalServerError)
+			log.Error("Failed to get user [name: %s]: %v", username, err)
+		}
+		return
+	}
+
+	// 🚨 SECURITY: No need to check existence of the repository if the client
+	// can't even get the valid secret. Mostly likely not a legitimate request.
+	if secret != tool.MD5(owner.Salt) {
+		c.Error(http.StatusBadRequest, "Invalid secret")
+		return
+	}
+
+	repo, err := db.Repos.GetByName(owner.ID, reponame)
+	if err != nil {
+		if db.IsErrRepoNotExist(err) {
+			c.Error(http.StatusBadRequest, "Repository does not exist")
+		} else {
+			c.Status(http.StatusInternalServerError)
+			log.Error("Failed to get repository [owner_id: %d, name: %s]: %v", owner.ID, reponame, err)
+		}
+		return
+	}
+
+	pusher, err := db.Users.GetByID(pusherID)
+	if err != nil {
+		if db.IsErrUserNotExist(err) {
+			c.Error(http.StatusBadRequest, "Pusher does not exist")
+		} else {
+			c.Status(http.StatusInternalServerError)
+			log.Error("Failed to get user [id: %d]: %v", pusherID, err)
+		}
+		return
+	}
+
+	log.Trace("TriggerTask: %s/%s@%s by %q", owner.Name, repo.Name, branch, pusher.Name)
+
+	go db.HookQueue.Add(repo.ID)
+	go db.AddTestPullRequestTask(pusher, repo.ID, branch, true)
+	c.Status(http.StatusAccepted)
+}