fix: add missing checks to several API and web handlers (#39501) (#39507)

Backport #39501 by @silverwind

Several handlers skipped checks that their sibling routes or settings
already enforce. This brings them in line.

- Push mirror API honors `DISABLE_NEW_PUSH` and checks the caller's
permission
- Media API serves small files with the usual content headers
- Issue attachment API ignores comment attachments
- Push-to-create respects `FORCE_PRIVATE`
- Profile feeds and follow actions respect `ENABLE_FEED` and owner
visibility
- Tag delete route refuses release tags
- Refresh token grant only accepts refresh tokens
- Gitea migrations bound the source's page size

Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: bircni <bircni@icloud.com>

routers/api/v1/repo/file.go

@@ -162,7 +162,7 @@ func GetRawFileOrLFS(ctx *context.APIContext) {
 
 	// if it's not a pointer, just serve the data directly
 	if !pointer.IsValid() {
-		_, _ = ctx.Resp.Write(lfsPointerBuf)
+		httplib.ServeUserContentByReader(ctx.Req, ctx.Resp, int64(len(lfsPointerBuf)), bytes.NewReader(lfsPointerBuf), httplib.ServeHeaderOptions{Filename: blob.Name()})
 		return
 	}
 
@@ -171,7 +171,7 @@ func GetRawFileOrLFS(ctx *context.APIContext) {
 
 	// If there isn't one, just serve the data directly
 	if errors.Is(err, git_model.ErrLFSObjectNotExist) {
-		_, _ = ctx.Resp.Write(lfsPointerBuf)
+		httplib.ServeUserContentByReader(ctx.Req, ctx.Resp, int64(len(lfsPointerBuf)), bytes.NewReader(lfsPointerBuf), httplib.ServeHeaderOptions{Filename: blob.Name()})
 		return
 	} else if err != nil {
 		ctx.APIErrorInternal(err)
@@ -198,7 +198,7 @@ func GetRawFileOrLFS(ctx *context.APIContext) {
 		return
 	}
 	defer lfsDataFile.Close()
-	httplib.ServeUserContentByFile(ctx.Base.Req, ctx.Base.Resp, lfsDataFile, httplib.ServeHeaderOptions{Filename: ctx.Repo.TreePath})
+	httplib.ServeUserContentByFile(ctx.Base.Req, ctx.Base.Resp, lfsDataFile, httplib.ServeHeaderOptions{Filename: blob.Name()})
 }
 
 func getBlobForEntry(ctx *context.APIContext) (blob *git.Blob, entry *git.TreeEntry, lastModified *time.Time) {

routers/api/v1/repo/issue_attachment.go

@@ -386,8 +386,8 @@ func attachmentBelongsToRepoOrIssue(ctx *context.APIContext, attachment *repo_mo
 		ctx.APIErrorNotFound("no such attachment in repo")
 		return false
 	}
-	if attachment.IssueID == 0 {
-		log.Debug("Requested attachment[%d] is not in an issue.", attachment.ID)
+	if attachment.IssueID == 0 || attachment.CommentID != 0 {
+		log.Debug("Requested attachment[%d] is not an issue attachment.", attachment.ID)
 		ctx.APIErrorNotFound("no such attachment in issue")
 		return false
 	} else if issue != nil && attachment.IssueID != issue.ID {

routers/api/v1/repo/mirror.go

@@ -291,6 +291,11 @@ func AddPushMirror(ctx *context.APIContext) {
 		return
 	}
 
+	if setting.Mirror.DisableNewPush {
+		ctx.APIError(http.StatusForbidden, "the site administrator has disabled the creation of new push mirrors")
+		return
+	}
+
 	pushMirror := web.GetForm[*api.CreatePushMirrorOption](ctx)
 	CreatePushMirror(ctx, pushMirror)
 }
@@ -356,7 +361,7 @@ func CreatePushMirror(ctx *context.APIContext, mirrorOption *api.CreatePushMirro
 
 	address, err := git.ParseRemoteAddr(mirrorOption.RemoteAddress, mirrorOption.RemoteUsername, mirrorOption.RemotePassword)
 	if err == nil {
-		err = migrations.IsMigrateURLAllowed(address, ctx.ContextUser)
+		err = migrations.IsMigrateURLAllowed(address, ctx.Doer)
 	}
 	if err != nil {
 		HandleRemoteAddressError(ctx, err)

routers/api/v1/repo/mirror_test.go

@@ -10,13 +10,36 @@ import (
 	"gitea.dev/models/db"
 	repo_model "gitea.dev/models/repo"
 	"gitea.dev/models/unittest"
+	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/setting"
+	api "gitea.dev/modules/structs"
 	"gitea.dev/modules/test"
 	"gitea.dev/services/contexttest"
 
 	"github.com/stretchr/testify/assert"
 )
 
+func TestCreatePushMirrorUsesCallerPermission(t *testing.T) {
+	defer test.MockVariableValue(&setting.ImportLocalPaths, true)()
+	ctx, resp := contexttest.MockAPIContext(t, "user2/repo1")
+	ctx.Doer = &user_model.User{}
+	ctx.ContextUser = &user_model.User{AllowImportLocal: true}
+
+	CreatePushMirror(ctx, &api.CreatePushMirrorOption{RemoteAddress: "local-mirror", Interval: "0"})
+
+	assert.Equal(t, http.StatusUnauthorized, resp.Code)
+}
+
+func TestAddPushMirrorDisabled(t *testing.T) {
+	defer test.MockVariableValue(&setting.Mirror.DisableNewPush, true)()
+	ctx, resp := contexttest.MockAPIContext(t, "user2/repo1")
+
+	AddPushMirror(ctx)
+
+	assert.Equal(t, http.StatusForbidden, resp.Code)
+	assert.Contains(t, resp.Body.String(), "the site administrator has disabled the creation of new push mirrors")
+}
+
 // TestPushMirrorSync verifies the endpoint attempts every push mirror instead
 // of aborting on the first failure, reporting all failed remotes with a 422.
 // Each remote name is not a configured git remote, so SyncPushMirror fails fast

routers/private/hook_post_receive.go

@@ -139,7 +139,8 @@ func hookPostReceiveUpdateRepoByOptions(ctx *gitea_context.PrivateContext, opts
 	// The repo is empty and being initialized by this push, so there is no
 	// dependent state (webhooks, notifications, visibility fan-out) to reconcile
 	// yet; setting the flags directly is sufficient in this push-to-create case.
-	if isPrivate.Has() && repo.IsPrivate != isPrivate.Value() {
+	if isPrivate.Has() && repo.IsPrivate != isPrivate.Value() &&
+		(isPrivate.Value() || !setting.Repository.ForcePrivate || ctx.Doer.IsAdmin) {
 		repo.IsPrivate = isPrivate.Value()
 		if err := repo_model.UpdateRepositoryColsNoAutoTime(ctx, repo, "is_private"); err != nil {
 			log.Error("failed to update repo is_private: %v", err)

routers/web/auth/oauth2_provider.go

@@ -576,7 +576,7 @@ func handleRefreshToken(ctx *context.Context, form forms.AccessTokenForm, server
 	}
 
 	token, err := oauth2_provider.ParseToken(form.RefreshToken, serverKey)
-	if err != nil {
+	if err != nil || token.Kind != oauth2_provider.KindRefreshToken {
 		handleAccessTokenError(ctx, oauth2_provider.AccessTokenError{
 			ErrorCode:        oauth2_provider.AccessTokenErrorCodeUnauthorizedClient,
 			ErrorDescription: "unable to parse refresh token",

routers/web/feed/profile.go

@@ -9,7 +9,9 @@ import (
 	activities_model "gitea.dev/models/activities"
 	"gitea.dev/models/organization"
 	"gitea.dev/models/renderhelper"
+	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/markup/markdown"
+	"gitea.dev/modules/setting"
 	"gitea.dev/services/context"
 	feed_service "gitea.dev/services/feed"
 
@@ -28,8 +30,15 @@ func ShowUserFeedAtom(ctx *context.Context) {
 
 // showUserFeed show user activity as RSS / Atom feed
 func showUserFeed(ctx *context.Context, formatType string) {
-	includePrivate := ctx.IsSigned && (ctx.Doer.IsAdmin || ctx.Doer.ID == ctx.ContextUser.ID)
 	isOrganisation := ctx.ContextUser.IsOrganization()
+	if !setting.Other.EnableFeed ||
+		isOrganisation && !organization.HasOrgOrUserVisible(ctx, ctx.ContextUser, ctx.Doer) ||
+		!isOrganisation && !user_model.IsUserVisibleToViewer(ctx, ctx.ContextUser, ctx.Doer) {
+		ctx.NotFound(nil)
+		return
+	}
+
+	includePrivate := ctx.IsSigned && (ctx.Doer.IsAdmin || ctx.Doer.ID == ctx.ContextUser.ID)
 	if ctx.IsSigned && isOrganisation && !includePrivate {
 		// When feed is requested by a member of the organization,
 		// include the private repo's the member has access to.

routers/web/repo/release.go

@@ -660,6 +660,11 @@ func deleteReleaseOrTag(ctx *context.Context, isDelTag bool) {
 		return
 	}
 
+	if isDelTag && !rel.IsTag {
+		ctx.HTTPError(http.StatusConflict, "a tag attached to a release cannot be deleted directly")
+		return
+	}
+
 	if err := release_service.DeleteReleaseByID(ctx, ctx.Repo.Repository, rel, ctx.Doer, isDelTag); err != nil {
 		if release_service.IsErrProtectedTagName(err) {
 			ctx.Flash.Error(ctx.Tr("repo.release.tag_name_protected"))

routers/web/repo/release_test.go

@@ -4,6 +4,7 @@
 package repo
 
 import (
+	"net/http"
 	"net/http/httptest"
 	"testing"
 
@@ -21,6 +22,21 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+func TestDeleteTagRetainsReleaseAndAttachments(t *testing.T) {
+	unittest.PrepareTestEnv(t)
+	ctx, resp := contexttest.MockContext(t, "POST user2/repo1/tags/delete?id=1")
+	contexttest.LoadUser(t, ctx, 2)
+	contexttest.LoadRepo(t, ctx, 1)
+	release := unittest.AssertExistsAndLoadBean(t, &repo_model.Release{ID: 1})
+	attachment := unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: 9, ReleaseID: 1})
+
+	DeleteTag(ctx)
+
+	assert.Equal(t, http.StatusConflict, resp.Code)
+	assert.Equal(t, release, unittest.AssertExistsAndLoadBean(t, &repo_model.Release{ID: 1}))
+	assert.Equal(t, attachment, unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: 9}))
+}
+
 func TestNewReleasePost(t *testing.T) {
 	unittest.PrepareTestEnv(t)
 

routers/web/user/home.go

@@ -734,18 +734,10 @@ func UsernameSubRoute(ctx *context.Context) {
 			ShowGPGKeys(ctx)
 		}
 	case strings.HasSuffix(username, ".rss"):
-		if !setting.Other.EnableFeed {
-			ctx.HTTPError(http.StatusNotFound)
-			return
-		}
 		if reloadParam(".rss") {
 			feed.ShowUserFeedRSS(ctx)
 		}
 	case strings.HasSuffix(username, ".atom"):
-		if !setting.Other.EnableFeed {
-			ctx.HTTPError(http.StatusNotFound)
-			return
-		}
 		if reloadParam(".atom") {
 			feed.ShowUserFeedAtom(ctx)
 		}

routers/web/user/profile.go

@@ -322,6 +322,13 @@ func prepareUserProfileTabData(ctx *context.Context, profileDbRepo *repo_model.R
 
 // ActionUserFollow is for follow/unfollow user request
 func ActionUserFollow(ctx *context.Context) {
+	isOrg := ctx.ContextUser.IsOrganization()
+	if isOrg && !organization.HasOrgOrUserVisible(ctx, ctx.ContextUser, ctx.Doer) ||
+		!isOrg && !user_model.IsUserVisibleToViewer(ctx, ctx.ContextUser, ctx.Doer) {
+		ctx.NotFound(nil)
+		return
+	}
+
 	var err error
 	switch ctx.FormString("action") {
 	case "follow":