Browse Source

api: fix critical CSRF vulnerabilities on API routes (#5355)

By explicitly requires token authentication.
Unknwon 5 years ago
parent
commit
3db9b06a6e
5 changed files with 34 additions and 32 deletions
  1. 1 1
      gogs.go
  2. 28 27
      pkg/auth/auth.go
  3. 3 2
      pkg/context/context.go
  4. 1 1
      routes/api/v1/api.go
  5. 1 1
      templates/.VERSION

+ 1 - 1
gogs.go

@@ -16,7 +16,7 @@ import (
 	"github.com/gogs/gogs/pkg/setting"
 )
 
-const APP_VER = "0.11.70.1126"
+const APP_VER = "0.11.71.1128"
 
 func init() {
 	setting.AppVer = APP_VER

+ 28 - 27
pkg/auth/auth.go

@@ -23,10 +23,11 @@ func IsAPIPath(url string) bool {
 	return strings.HasPrefix(url, "/api/")
 }
 
-// SignedInID returns the id of signed in user.
-func SignedInID(c *macaron.Context, sess session.Store) int64 {
+// SignedInID returns the id of signed in user, along with one bool value which indicates whether user uses token
+// authentication.
+func SignedInID(c *macaron.Context, sess session.Store) (_ int64, isTokenAuth bool) {
 	if !models.HasEngine {
-		return 0
+		return 0, false
 	}
 
 	// Check access token.
@@ -53,40 +54,40 @@ func SignedInID(c *macaron.Context, sess session.Store) int64 {
 				if !models.IsErrAccessTokenNotExist(err) && !models.IsErrAccessTokenEmpty(err) {
 					log.Error(2, "GetAccessTokenBySHA: %v", err)
 				}
-				return 0
+				return 0, false
 			}
 			t.Updated = time.Now()
 			if err = models.UpdateAccessToken(t); err != nil {
 				log.Error(2, "UpdateAccessToken: %v", err)
 			}
-			return t.UID
+			return t.UID, true
 		}
 	}
 
 	uid := sess.Get("uid")
 	if uid == nil {
-		return 0
+		return 0, false
 	}
 	if id, ok := uid.(int64); ok {
 		if _, err := models.GetUserByID(id); err != nil {
 			if !errors.IsUserNotExist(err) {
 				log.Error(2, "GetUserByID: %v", err)
 			}
-			return 0
+			return 0, false
 		}
-		return id
+		return id, false
 	}
-	return 0
+	return 0, false
 }
 
-// SignedInUser returns the user object of signed user.
-// It returns a bool value to indicate whether user uses basic auth or not.
-func SignedInUser(ctx *macaron.Context, sess session.Store) (*models.User, bool) {
+// SignedInUser returns the user object of signed in user, along with two bool values,
+// which indicate whether user uses HTTP Basic Authentication or token authentication respectively.
+func SignedInUser(ctx *macaron.Context, sess session.Store) (_ *models.User, isBasicAuth bool, isTokenAuth bool) {
 	if !models.HasEngine {
-		return nil, false
+		return nil, false, false
 	}
 
-	uid := SignedInID(ctx, sess)
+	uid, isTokenAuth := SignedInID(ctx, sess)
 
 	if uid <= 0 {
 		if setting.Service.EnableReverseProxyAuth {
@@ -95,8 +96,8 @@ func SignedInUser(ctx *macaron.Context, sess session.Store) (*models.User, bool)
 				u, err := models.GetUserByName(webAuthUser)
 				if err != nil {
 					if !errors.IsUserNotExist(err) {
-						log.Error(4, "GetUserByName: %v", err)
-						return nil, false
+						log.Error(2, "GetUserByName: %v", err)
+						return nil, false, false
 					}
 
 					// Check if enabled auto-registration.
@@ -109,14 +110,14 @@ func SignedInUser(ctx *macaron.Context, sess session.Store) (*models.User, bool)
 						}
 						if err = models.CreateUser(u); err != nil {
 							// FIXME: should I create a system notice?
-							log.Error(4, "CreateUser: %v", err)
-							return nil, false
+							log.Error(2, "CreateUser: %v", err)
+							return nil, false, false
 						} else {
-							return u, false
+							return u, false, false
 						}
 					}
 				}
-				return u, false
+				return u, false, false
 			}
 		}
 
@@ -130,21 +131,21 @@ func SignedInUser(ctx *macaron.Context, sess session.Store) (*models.User, bool)
 				u, err := models.UserLogin(uname, passwd, -1)
 				if err != nil {
 					if !errors.IsUserNotExist(err) {
-						log.Error(4, "UserLogin: %v", err)
+						log.Error(2, "UserLogin: %v", err)
 					}
-					return nil, false
+					return nil, false, false
 				}
 
-				return u, true
+				return u, true, false
 			}
 		}
-		return nil, false
+		return nil, false, false
 	}
 
 	u, err := models.GetUserByID(uid)
 	if err != nil {
-		log.Error(4, "GetUserById: %v", err)
-		return nil, false
+		log.Error(2, "GetUserByID: %v", err)
+		return nil, false, false
 	}
-	return u, false
+	return u, false, isTokenAuth
 }

+ 3 - 2
pkg/context/context.go

@@ -40,6 +40,7 @@ type Context struct {
 	User        *models.User
 	IsLogged    bool
 	IsBasicAuth bool
+	IsTokenAuth bool
 
 	Repo *Repository
 	Org  *Organization
@@ -289,8 +290,8 @@ func Contexter() macaron.Handler {
 			c.Header().Set("Access-Control-Allow-Headers", "Content-Type, Access-Control-Allow-Headers, Authorization, X-Requested-With")
 		}
 
-		// Get user from session if logined.
-		c.User, c.IsBasicAuth = auth.SignedInUser(c.Context, c.Session)
+		// Get user from session or header when possible
+		c.User, c.IsBasicAuth, c.IsTokenAuth = auth.SignedInUser(c.Context, c.Session)
 
 		if c.User != nil {
 			c.IsLogged = true

+ 1 - 1
routes/api/v1/api.go

@@ -86,7 +86,7 @@ func repoAssignment() macaron.Handler {
 // Contexter middleware already checks token for user sign in process.
 func reqToken() macaron.Handler {
 	return func(c *context.Context) {
-		if !c.IsLogged {
+		if !c.IsTokenAuth {
 			c.Error(401)
 			return
 		}

+ 1 - 1
templates/.VERSION

@@ -1 +1 @@
-0.11.70.1126
+0.11.71.1128