Fix various permission & login related bugs (#36002) (#36004)

lunny · wxiaoguang · web-flow · commit 20cf4b78493a · 2025-11-22T12:33:48.000Z
Backport #36002 Permission & protection check: - Fix Delete Release permission check - Fix Update Pull Request with rebase branch protection check - Fix Issue Dependency permission check - Fix Delete Comment History ID check Information leaking: - Show unified message for non-existing user and invalid password - Fix #35984 - Don't expose release draft to non-writer users. - Make API returns signature's email address instead of the user profile's. Auth & Login: - Avoid GCM OAuth2 attempt when OAuth2 is disabled - Fix #35510 --------- Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
diff --git a/.editorconfig b/.editorconfig
@@ -25,6 +25,10 @@ insert_final_newline = false
 [templates/user/auth/oidc_wellknown.tmpl]
 indent_style = space
 
+[templates/shared/actions/runner_badge_*.tmpl]
+# editconfig lint requires these XML-like files to have charset defined, but the files don't have.
+charset = unset
+
 [Makefile]
 indent_style = tab
 
diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go
@@ -82,6 +82,7 @@ import (
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/modules/web"
 	"code.gitea.io/gitea/routers/api/v1/activitypub"
 	"code.gitea.io/gitea/routers/api/v1/admin"
@@ -791,7 +792,9 @@ func apiAuth(authMethod auth.Method) func(*context.APIContext) {
 	return func(ctx *context.APIContext) {
 		ar, err := common.AuthShared(ctx.Base, nil, authMethod)
 		if err != nil {
-			ctx.APIError(http.StatusUnauthorized, err)
+			msg, ok := auth.ErrAsUserAuthMessage(err)
+			msg = util.Iif(ok, msg, "invalid username, password or token")
+			ctx.APIError(http.StatusUnauthorized, msg)
 			return
 		}
 		ctx.Doer = ar.Doer
diff --git a/routers/api/v1/repo/issue_dependency.go b/routers/api/v1/repo/issue_dependency.go
@@ -201,7 +201,7 @@ func CreateIssueDependency(ctx *context.APIContext) {
 		return
 	}
 
-	dependencyPerm := getPermissionForRepo(ctx, target.Repo)
+	dependencyPerm := getPermissionForRepo(ctx, dependency.Repo)
 	if ctx.Written() {
 		return
 	}
@@ -262,7 +262,7 @@ func RemoveIssueDependency(ctx *context.APIContext) {
 		return
 	}
 
-	dependencyPerm := getPermissionForRepo(ctx, target.Repo)
+	dependencyPerm := getPermissionForRepo(ctx, dependency.Repo)
 	if ctx.Written() {
 		return
 	}
diff --git a/routers/api/v1/repo/release_tags.go b/routers/api/v1/repo/release_tags.go
@@ -7,6 +7,7 @@ import (
 	"net/http"
 
 	repo_model "code.gitea.io/gitea/models/repo"
+	unit_model "code.gitea.io/gitea/models/unit"
 	"code.gitea.io/gitea/services/context"
 	"code.gitea.io/gitea/services/convert"
 	release_service "code.gitea.io/gitea/services/release"
@@ -58,6 +59,13 @@ func GetReleaseByTag(ctx *context.APIContext) {
 		return
 	}
 
+	if release.IsDraft { // only the users with write access can see draft releases
+		if !ctx.IsSigned || !ctx.Repo.CanWrite(unit_model.TypeReleases) {
+			ctx.APIErrorNotFound()
+			return
+		}
+	}
+
 	if err = release.LoadAttributes(ctx); err != nil {
 		ctx.APIErrorInternal(err)
 		return
diff --git a/routers/web/repo/githttp.go b/routers/web/repo/githttp.go
@@ -147,7 +147,13 @@ func httpBase(ctx *context.Context) *serviceHandler {
 		// rely on the results of Contexter
 		if !ctx.IsSigned {
 			// TODO: support digit auth - which would be Authorization header with digit
-			ctx.Resp.Header().Set("WWW-Authenticate", `Basic realm="Gitea"`)
+			if setting.OAuth2.Enabled {
+				// `Basic realm="Gitea"` tells the GCM to use builtin OAuth2 application: https://github.com/git-ecosystem/git-credential-manager/pull/1442
+				ctx.Resp.Header().Set("WWW-Authenticate", `Basic realm="Gitea"`)
+			} else {
+				// If OAuth2 is disabled, then use another realm to avoid GCM OAuth2 attempt
+				ctx.Resp.Header().Set("WWW-Authenticate", `Basic realm="Gitea (Basic Auth)"`)
+			}
 			ctx.HTTPError(http.StatusUnauthorized)
 			return nil
 		}
diff --git a/routers/web/repo/issue_content_history.go b/routers/web/repo/issue_content_history.go
@@ -206,12 +206,11 @@ func SoftDeleteContentHistory(ctx *context.Context) {
 		ctx.NotFound(issues_model.ErrCommentNotExist{})
 		return
 	}
+	if history.CommentID != commentID {
+		ctx.NotFound(issues_model.ErrCommentNotExist{})
+		return
+	}
 	if commentID != 0 {
-		if history.CommentID != commentID {
-			ctx.NotFound(issues_model.ErrCommentNotExist{})
-			return
-		}
-
 		if comment, err = issues_model.GetCommentByID(ctx, commentID); err != nil {
 			log.Error("can not get comment for issue content history %v. err=%v", historyID, err)
 			return
diff --git a/services/auth/auth.go b/services/auth/auth.go
@@ -5,6 +5,7 @@
 package auth
 
 import (
+	"errors"
 	"fmt"
 	"net/http"
 	"regexp"
@@ -40,6 +41,20 @@ var globalVars = sync.OnceValue(func() *globalVarsStruct {
 	}
 })
 
+type ErrUserAuthMessage string
+
+func (e ErrUserAuthMessage) Error() string {
+	return string(e)
+}
+
+func ErrAsUserAuthMessage(err error) (string, bool) {
+	var msg ErrUserAuthMessage
+	if errors.As(err, &msg) {
+		return msg.Error(), true
+	}
+	return "", false
+}
+
 // Init should be called exactly once when the application starts to allow plugins
 // to allocate necessary resources
 func Init() {
diff --git a/services/auth/basic.go b/services/auth/basic.go
@@ -5,7 +5,6 @@
 package auth
 
 import (
-	"errors"
 	"net/http"
 
 	actions_model "code.gitea.io/gitea/models/actions"
@@ -146,7 +145,7 @@ func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore
 			return nil, err
 		}
 		if hasWebAuthn {
-			return nil, errors.New("basic authorization is not allowed while WebAuthn enrolled")
+			return nil, ErrUserAuthMessage("basic authorization is not allowed while WebAuthn enrolled")
 		}
 
 		if err := validateTOTP(req, u); err != nil {
diff --git a/services/convert/convert.go b/services/convert/convert.go
@@ -542,8 +542,9 @@ func ToVerification(ctx context.Context, c *git.Commit) *api.PayloadCommitVerifi
 	}
 	if verif.SigningUser != nil {
 		commitVerification.Signer = &api.PayloadUser{
-			Name:  verif.SigningUser.Name,
-			Email: verif.SigningUser.Email,
+			UserName: verif.SigningUser.Name,
+			Name:     verif.SigningUser.DisplayName(),
+			Email:    verif.SigningEmail, // Use the email from the signature, not from the user profile
 		}
 	}
 	return commitVerification
diff --git a/services/pull/merge.go b/services/pull/merge.go
@@ -546,11 +546,15 @@ var escapedSymbols = regexp.MustCompile(`([*[?! \\])`)
 
 // IsUserAllowedToMerge check if user is allowed to merge PR with given permissions and branch protections
 func IsUserAllowedToMerge(ctx context.Context, pr *issues_model.PullRequest, p access_model.Permission, user *user_model.User) (bool, error) {
+	return isUserAllowedToMergeInRepoBranch(ctx, pr.BaseRepoID, pr.BaseBranch, p, user)
+}
+
+func isUserAllowedToMergeInRepoBranch(ctx context.Context, repoID int64, branch string, p access_model.Permission, user *user_model.User) (bool, error) {
 	if user == nil {
 		return false, nil
 	}
 
-	pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch)
+	pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, repoID, branch)
 	if err != nil {
 		return false, err
 	}
diff --git a/services/pull/update.go b/services/pull/update.go
@@ -97,11 +97,11 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.
 }
 
 // IsUserAllowedToUpdate check if user is allowed to update PR with given permissions and branch protections
+// update PR means send new commits to PR head branch from base branch
 func IsUserAllowedToUpdate(ctx context.Context, pull *issues_model.PullRequest, user *user_model.User) (mergeAllowed, rebaseAllowed bool, err error) {
 	if pull.Flow == issues_model.PullRequestFlowAGit {
 		return false, false, nil
 	}
-
 	if user == nil {
 		return false, false, nil
 	}
@@ -117,61 +117,56 @@ func IsUserAllowedToUpdate(ctx context.Context, pull *issues_model.PullRequest,
 		return false, false, err
 	}
 
-	pr := &issues_model.PullRequest{
-		HeadRepoID: pull.BaseRepoID,
-		HeadRepo:   pull.BaseRepo,
-		BaseRepoID: pull.HeadRepoID,
-		BaseRepo:   pull.HeadRepo,
-		HeadBranch: pull.BaseBranch,
-		BaseBranch: pull.HeadBranch,
-	}
-
-	pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch)
-	if err != nil {
-		return false, false, err
-	}
-
-	if err := pr.LoadBaseRepo(ctx); err != nil {
-		return false, false, err
-	}
-	prUnit, err := pr.BaseRepo.GetUnit(ctx, unit.TypePullRequests)
-	if err != nil {
+	// 1. check base repository's AllowRebaseUpdate configuration
+	// it is a config in base repo but controls the head (fork) repo's "Update" behavior
+	{
+		prBaseUnit, err := pull.BaseRepo.GetUnit(ctx, unit.TypePullRequests)
 		if repo_model.IsErrUnitTypeNotExist(err) {
-			return false, false, nil
+			return false, false, nil // the PR unit is disabled in base repo
+		} else if err != nil {
+			return false, false, fmt.Errorf("get base repo unit: %v", err)
 		}
-		log.Error("pr.BaseRepo.GetUnit(unit.TypePullRequests): %v", err)
-		return false, false, err
+		rebaseAllowed = prBaseUnit.PullRequestsConfig().AllowRebaseUpdate
 	}
 
-	rebaseAllowed = prUnit.PullRequestsConfig().AllowRebaseUpdate
-
-	// If branch protected, disable rebase unless user is whitelisted to force push (which extends regular push)
-	if pb != nil {
-		pb.Repo = pull.BaseRepo
-		if !pb.CanUserForcePush(ctx, user) {
-			rebaseAllowed = false
+	// 2. check head branch protection whether rebase is allowed, if pb not found then rebase depends on the above setting
+	{
+		pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pull.HeadRepoID, pull.HeadBranch)
+		if err != nil {
+			return false, false, err
+		}
+		// If branch protected, disable rebase unless user is whitelisted to force push (which extends regular push)
+		if pb != nil {
+			pb.Repo = pull.HeadRepo
+			rebaseAllowed = rebaseAllowed && pb.CanUserForcePush(ctx, user)
 		}
 	}
 
+	// 3. check whether user has write access to head branch
 	baseRepoPerm, err := access_model.GetUserRepoPermission(ctx, pull.BaseRepo, user)
 	if err != nil {
 		return false, false, err
 	}
 
-	mergeAllowed, err = IsUserAllowedToMerge(ctx, pr, headRepoPerm, user)
+	mergeAllowed, err = isUserAllowedToMergeInRepoBranch(ctx, pull.HeadRepoID, pull.HeadBranch, headRepoPerm, user)
 	if err != nil {
 		return false, false, err
 	}
 
+	// 4. if the pull creator allows maintainer to edit, it means the write permissions of the head branch has been
+	// granted to the user with write permission of the base repository
 	if pull.AllowMaintainerEdit {
-		mergeAllowedMaintainer, err := IsUserAllowedToMerge(ctx, pr, baseRepoPerm, user)
+		mergeAllowedMaintainer, err := isUserAllowedToMergeInRepoBranch(ctx, pull.BaseRepoID, pull.BaseBranch, baseRepoPerm, user)
 		if err != nil {
 			return false, false, err
 		}
 
 		mergeAllowed = mergeAllowed || mergeAllowedMaintainer
 	}
 
+	// if merge is not allowed, rebase is also not allowed
+	rebaseAllowed = rebaseAllowed && mergeAllowed
+
 	return mergeAllowed, rebaseAllowed, nil
 }
 
diff --git a/services/release/release.go b/services/release/release.go
@@ -361,7 +361,7 @@ func DeleteReleaseByID(ctx context.Context, repo *repo_model.Repository, rel *re
 		if err != nil {
 			return fmt.Errorf("GetProtectedTags: %w", err)
 		}
-		isAllowed, err := git_model.IsUserAllowedToControlTag(ctx, protectedTags, rel.TagName, rel.PublisherID)
+		isAllowed, err := git_model.IsUserAllowedToControlTag(ctx, protectedTags, rel.TagName, doer.ID)
 		if err != nil {
 			return err
 		}
diff --git a/tests/integration/api_auth_test.go b/tests/integration/api_auth_test.go
@@ -0,0 +1,32 @@
+// Copyright 2025 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package integration
+
+import (
+	"net/http"
+	"testing"
+
+	"code.gitea.io/gitea/tests"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestAPIAuth(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	req := NewRequestf(t, "GET", "/api/v1/user").AddBasicAuth("user2")
+	MakeRequest(t, req, http.StatusOK)
+
+	req = NewRequestf(t, "GET", "/api/v1/user").AddBasicAuth("user2", "wrong-password")
+	resp := MakeRequest(t, req, http.StatusUnauthorized)
+	assert.Contains(t, resp.Body.String(), `{"message":"invalid username, password or token"`)
+
+	req = NewRequestf(t, "GET", "/api/v1/user").AddBasicAuth("user-not-exist")
+	resp = MakeRequest(t, req, http.StatusUnauthorized)
+	assert.Contains(t, resp.Body.String(), `{"message":"invalid username, password or token"`)
+
+	req = NewRequestf(t, "GET", "/api/v1/users/user2/repos").AddTokenAuth("Bearer wrong_token")
+	resp = MakeRequest(t, req, http.StatusUnauthorized)
+	assert.Contains(t, resp.Body.String(), `{"message":"invalid username, password or token"`)
+}
diff --git a/tests/integration/api_issue_dependency_test.go b/tests/integration/api_issue_dependency_test.go
diff --git a/tests/integration/api_releases_test.go b/tests/integration/api_releases_test.go
diff --git a/tests/integration/api_repo_test.go b/tests/integration/api_repo_test.go
diff --git a/tests/integration/integration_test.go b/tests/integration/integration_test.go
diff --git a/tests/integration/pull_update_test.go b/tests/integration/pull_update_test.go
diff --git a/tests/integration/repo_tag_test.go b/tests/integration/repo_tag_test.go

Original file line number	Diff line number	Diff line change
`@@ -201,7 +201,7 @@ func CreateIssueDependency(ctx *context.APIContext) {`
`201`	`201`	`return`
`202`	`202`	`}`
`203`	`203`
`204`		`- dependencyPerm := getPermissionForRepo(ctx, target.Repo)`
	`204`	`+ dependencyPerm := getPermissionForRepo(ctx, dependency.Repo)`
`205`	`205`	`if ctx.Written() {`
`206`	`206`	`return`
`207`	`207`	`}`
`@@ -262,7 +262,7 @@ func RemoveIssueDependency(ctx *context.APIContext) {`
`262`	`262`	`return`
`263`	`263`	`}`
`264`	`264`
`265`		`- dependencyPerm := getPermissionForRepo(ctx, target.Repo)`
	`265`	`+ dependencyPerm := getPermissionForRepo(ctx, dependency.Repo)`
`266`	`266`	`if ctx.Written() {`
`267`	`267`	`return`
`268`	`268`	`}`
Original file line number	Diff line number	Diff line change
`@@ -5,7 +5,6 @@`
`5`	`5`	`package auth`
`6`	`6`
`7`	`7`	`import (`
`8`		`- "errors"`
`9`	`8`	`"net/http"`
`10`	`9`
`11`	`10`	`actions_model "code.gitea.io/gitea/models/actions"`
`@@ -146,7 +145,7 @@ func (b Basic) Verify(req http.Request, w http.ResponseWriter, store DataStore`
`146`	`145`	`return nil, err`
`147`	`146`	`}`
`148`	`147`	`if hasWebAuthn {`
`149`		`- return nil, errors.New("basic authorization is not allowed while WebAuthn enrolled")`
	`148`	`+ return nil, ErrUserAuthMessage("basic authorization is not allowed while WebAuthn enrolled")`
`150`	`149`	`}`
`151`	`150`
`152`	`151`	`if err := validateTOTP(req, u); err != nil {`
Original file line number	Diff line number	Diff line change
`@@ -542,8 +542,9 @@ func ToVerification(ctx context.Context, c git.Commit) api.PayloadCommitVerifi`
`542`	`542`	`}`
`543`	`543`	`if verif.SigningUser != nil {`
`544`	`544`	`commitVerification.Signer = &api.PayloadUser{`
`545`		`- Name: verif.SigningUser.Name,`
`546`		`- Email: verif.SigningUser.Email,`
	`545`	`+ UserName: verif.SigningUser.Name,`
	`546`	`+ Name: verif.SigningUser.DisplayName(),`
	`547`	`+ Email: verif.SigningEmail, // Use the email from the signature, not from the user profile`
`547`	`548`	`}`
`548`	`549`	`}`
`549`	`550`	`return commitVerification`