fix(release): validate web attachment renames against allowed types (#38314)

This fixes the web release edit flow so renamed release attachments are
validated against `[repository.release] ALLOWED_TYPES`.

Previously, the API attachment edit endpoint already enforced release
attachment type restrictions, but the web release edit form passed
`attachment-edit-*` values into `release_service.UpdateRelease`, which
updated attachment names directly without validating the new filename
against `setting.Repository.Release.AllowedTypes`.

As a result, a user with repository write access could rename an
existing release attachment to a disallowed extension through the web
UI.

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

routers/web/repo/release.go

@@ -27,7 +27,6 @@ import (
 	"gitea.dev/modules/util"
 	"gitea.dev/modules/web"
 	"gitea.dev/routers/web/feed"
-	shared_user "gitea.dev/routers/web/shared/user"
 	"gitea.dev/services/context"
 	"gitea.dev/services/context/upload"
 	"gitea.dev/services/forms"
@@ -338,7 +337,6 @@ func LatestRelease(ctx *context.Context) {
 
 func newReleaseCommon(ctx *context.Context) {
 	ctx.Data["Title"] = ctx.Tr("repo.release.new_release")
-	ctx.Data["PageIsReleaseList"] = true
 
 	tags, err := repo_model.GetTagNamesByRepoID(ctx, ctx.Repo.Repository.ID)
 	if err != nil {
@@ -346,17 +344,8 @@ func newReleaseCommon(ctx *context.Context) {
 		return
 	}
 	ctx.Data["Tags"] = tags
-
 	ctx.Data["IsAttachmentEnabled"] = setting.Attachment.Enabled
-	assigneeUsers, err := repo_model.GetRepoAssignees(ctx, ctx.Repo.Repository)
-	if err != nil {
-		ctx.ServerError("GetRepoAssignees", err)
-		return
-	}
-	ctx.Data["Assignees"] = shared_user.MakeSelfOnTop(ctx.Doer, assigneeUsers)
-
 	upload.AddUploadContext(ctx, "release")
-
 	PrepareBranchList(ctx) // for New Release page
 }
 
@@ -425,8 +414,8 @@ func GenerateReleaseNotes(ctx *context.Context) {
 
 // NewReleasePost response for creating a release
 func NewReleasePost(ctx *context.Context) {
-	newReleaseCommon(ctx)
-	if ctx.Written() {
+	if ctx.HasError() {
+		ctx.JSONError(ctx.GetErrMsg())
 		return
 	}
 
@@ -450,35 +439,28 @@ func NewReleasePost(ctx *context.Context) {
 	// Or another choice is "always show the tag-only button" if error occurs.
 	ctx.Data["ShowCreateTagOnlyButton"] = form.TagOnly || rel == nil
 
-	// do some form checks
-	if ctx.HasError() {
-		ctx.HTML(http.StatusOK, tplReleaseNew)
-		return
-	}
-
 	form.Target = util.IfZero(form.Target, ctx.Repo.Repository.DefaultBranch)
 	if exist, _ := git_model.IsBranchExist(ctx, ctx.Repo.Repository.ID, form.Target); !exist {
-		ctx.RenderWithErrDeprecated(ctx.Tr("form.target_branch_not_exist"), tplReleaseNew, &form)
+		ctx.JSONError(ctx.Tr("form.target_branch_not_exist"))
 		return
 	}
 
 	if !form.TagOnly && form.Title == "" {
 		// if not "tag only", then the title of the release cannot be empty
-		ctx.RenderWithErrDeprecated(ctx.Tr("repo.release.title_empty"), tplReleaseNew, &form)
+		ctx.JSONError(ctx.Tr("repo.release.title_empty"))
 		return
 	}
 
 	handleTagReleaseError := func(err error) {
-		ctx.Data["Err_TagName"] = true
 		switch {
 		case release_service.IsErrTagAlreadyExists(err):
-			ctx.RenderWithErrDeprecated(ctx.Tr("repo.branch.tag_collision", form.TagName), tplReleaseNew, &form)
+			ctx.JSONError(ctx.Tr("repo.branch.tag_collision", form.TagName))
 		case repo_model.IsErrReleaseAlreadyExist(err):
-			ctx.RenderWithErrDeprecated(ctx.Tr("repo.release.tag_name_already_exist"), tplReleaseNew, &form)
+			ctx.JSONError(ctx.Tr("repo.release.tag_name_already_exist"))
 		case release_service.IsErrInvalidTagName(err):
-			ctx.RenderWithErrDeprecated(ctx.Tr("repo.release.tag_name_invalid"), tplReleaseNew, &form)
+			ctx.JSONError(ctx.Tr("repo.release.tag_name_invalid"))
 		case release_service.IsErrProtectedTagName(err):
-			ctx.RenderWithErrDeprecated(ctx.Tr("repo.release.tag_name_protected"), tplReleaseNew, &form)
+			ctx.JSONError(ctx.Tr("repo.release.tag_name_protected"))
 		default:
 			ctx.ServerError("handleTagReleaseError", err)
 		}
@@ -497,7 +479,7 @@ func NewReleasePost(ctx *context.Context) {
 			return
 		}
 		ctx.Flash.Success(ctx.Tr("repo.tag.create_success", form.TagName))
-		ctx.Redirect(ctx.Repo.RepoLink + "/src/tag/" + util.PathEscapeSegments(form.TagName))
+		ctx.JSONRedirect(ctx.Repo.RepoLink + "/src/tag/" + util.PathEscapeSegments(form.TagName))
 		return
 	}
 
@@ -522,7 +504,7 @@ func NewReleasePost(ctx *context.Context) {
 			handleTagReleaseError(err)
 			return
 		}
-		ctx.Redirect(ctx.Repo.RepoLink + "/releases")
+		ctx.JSONRedirect(ctx.Repo.RepoLink + "/releases")
 		return
 	}
 
@@ -530,8 +512,7 @@ func NewReleasePost(ctx *context.Context) {
 	// old logic: if the release is not a tag (it is a real release), do not update it on the "new release" page
 	// add new logic: if tag-only, do not convert the tag to a release
 	if form.TagOnly || !rel.IsTag {
-		ctx.Data["Err_TagName"] = true
-		ctx.RenderWithErrDeprecated(ctx.Tr("repo.release.tag_name_already_exist"), tplReleaseNew, &form)
+		ctx.JSONError(ctx.Tr("repo.release.tag_name_already_exist"))
 		return
 	}
 
@@ -547,7 +528,7 @@ func NewReleasePost(ctx *context.Context) {
 		handleTagReleaseError(err)
 		return
 	}
-	ctx.Redirect(ctx.Repo.RepoLink + "/releases")
+	ctx.JSONRedirect(ctx.Repo.RepoLink + "/releases")
 }
 
 // EditRelease render release edit page
@@ -589,55 +570,39 @@ func EditRelease(ctx *context.Context) {
 		return
 	}
 	ctx.Data["attachments"] = rel.Attachments
-
-	// Get assignees.
-	assigneeUsers, err := repo_model.GetRepoAssignees(ctx, rel.Repo)
-	if err != nil {
-		ctx.ServerError("GetRepoAssignees", err)
-		return
-	}
-	ctx.Data["Assignees"] = shared_user.MakeSelfOnTop(ctx.Doer, assigneeUsers)
-
 	ctx.HTML(http.StatusOK, tplReleaseNew)
 }
 
 // EditReleasePost response for edit release
 func EditReleasePost(ctx *context.Context) {
-	form := web.GetForm(ctx).(*forms.EditReleaseForm)
-
-	newReleaseCommon(ctx)
-	if ctx.Written() {
+	if ctx.HasError() {
+		ctx.JSONError(ctx.GetErrMsg())
 		return
 	}
 
-	ctx.Data["Title"] = ctx.Tr("repo.release.edit_release")
-	ctx.Data["PageIsEditRelease"] = true
+	form := web.GetForm(ctx).(*forms.EditReleaseForm)
 
 	tagName := ctx.PathParam("*")
 	rel, err := repo_model.GetRelease(ctx, ctx.Repo.Repository.ID, tagName)
 	if err != nil {
 		if repo_model.IsErrReleaseNotExist(err) {
-			ctx.NotFound(err)
+			ctx.JSONErrorNotFound(err.Error())
 		} else {
 			ctx.ServerError("GetRelease", err)
 		}
 		return
 	}
 	if rel.IsTag {
-		ctx.NotFound(err) // for a pure tag release, don't allow to edit it as a release
+		ctx.JSONErrorNotFound() // for a pure tag release, don't allow to edit it as a release
 		return
 	}
+
 	ctx.Data["tag_name"] = rel.TagName
 	ctx.Data["tag_target"] = util.IfZero(rel.Target, ctx.Repo.Repository.DefaultBranch)
 	ctx.Data["title"] = rel.Title
 	ctx.Data["content"] = rel.Note
 	ctx.Data["prerelease"] = rel.IsPrerelease
 
-	if ctx.HasError() {
-		ctx.HTML(http.StatusOK, tplReleaseNew)
-		return
-	}
-
 	const delPrefix = "attachment-del-"
 	const editPrefix = "attachment-edit-"
 	var addAttachmentUUIDs, delAttachmentUUIDs []string
@@ -659,10 +624,14 @@ func EditReleasePost(ctx *context.Context) {
 	rel.IsPrerelease = form.Prerelease
 	if err = release_service.UpdateRelease(ctx, ctx.Doer, ctx.Repo.GitRepo,
 		rel, addAttachmentUUIDs, delAttachmentUUIDs, editAttachments); err != nil {
-		ctx.ServerError("UpdateRelease", err)
+		if upload.IsErrFileTypeForbidden(err) {
+			ctx.JSONError(err.Error())
+		} else {
+			ctx.ServerError("UpdateRelease", err)
+		}
 		return
 	}
-	ctx.Redirect(ctx.Repo.RepoLink + "/releases")
+	ctx.JSONRedirect(ctx.Repo.RepoLink + "/releases")
 }
 
 // DeleteRelease deletes a release

routers/web/repo/release_test.go

@@ -4,12 +4,14 @@
 package repo
 
 import (
+	"net/http/httptest"
 	"testing"
 
 	"gitea.dev/models/db"
 	repo_model "gitea.dev/models/repo"
 	"gitea.dev/models/unit"
 	"gitea.dev/models/unittest"
+	"gitea.dev/modules/test"
 	"gitea.dev/modules/web"
 	"gitea.dev/services/context"
 	"gitea.dev/services/contexttest"
@@ -39,15 +41,15 @@ func TestNewReleasePost(t *testing.T) {
 		assert.NotEmpty(t, ctx.Data["ShowCreateTagOnlyButton"])
 	})
 
-	post := func(t *testing.T, form forms.NewReleaseForm) *context.Context {
-		ctx, _ := contexttest.MockContext(t, "user2/repo1/releases/new")
+	post := func(t *testing.T, form forms.NewReleaseForm) (*context.Context, *httptest.ResponseRecorder) {
+		ctx, resp := contexttest.MockContext(t, "user2/repo1/releases/new")
 		contexttest.LoadUser(t, ctx, 2)
 		contexttest.LoadRepo(t, ctx, 1)
 		contexttest.LoadGitRepo(t, ctx)
 		defer ctx.Repo.GitRepo.Close()
 		web.SetForm(ctx, &form)
 		NewReleasePost(ctx)
-		return ctx
+		return ctx, resp
 	}
 
 	loadRelease := func(t *testing.T, tagName string) *repo_model.Release {
@@ -70,7 +72,7 @@ func TestNewReleasePost(t *testing.T) {
 	})
 
 	t.Run("ReleaseExistsDoUpdate(non-tag)", func(t *testing.T) {
-		ctx := post(t, forms.NewReleaseForm{
+		_, resp := post(t, forms.NewReleaseForm{
 			TagName: "v1.1",
 			Target:  "master",
 			Title:   "updated-title",
@@ -80,11 +82,11 @@ func TestNewReleasePost(t *testing.T) {
 		require.NotNil(t, rel)
 		assert.False(t, rel.IsTag)
 		assert.Equal(t, "testing-release", rel.Title)
-		assert.NotEmpty(t, ctx.Flash.ErrorMsg)
+		assert.NotEmpty(t, test.ParseJSONError(resp.Body.Bytes()).ErrorMessage)
 	})
 
 	t.Run("ReleaseExistsDoUpdate(tag-only)", func(t *testing.T) {
-		ctx := post(t, forms.NewReleaseForm{
+		ctx, resp := post(t, forms.NewReleaseForm{
 			TagName: "delete-tag", // a strange name, but it is the only "is_tag=true" fixture
 			Target:  "master",
 			Title:   "updated-title",
@@ -95,12 +97,12 @@ func TestNewReleasePost(t *testing.T) {
 		require.NotNil(t, rel)
 		assert.True(t, rel.IsTag) // the record should not be updated because the request is "tag-only". TODO: need to improve the logic?
 		assert.Equal(t, "delete-tag", rel.Title)
-		assert.NotEmpty(t, ctx.Flash.ErrorMsg)
+		assert.NotEmpty(t, test.ParseJSONError(resp.Body.Bytes()).ErrorMessage)
 		assert.NotEmpty(t, ctx.Data["ShowCreateTagOnlyButton"]) // still show the "tag-only" button
 	})
 
 	t.Run("ReleaseExistsDoUpdate(tag-release)", func(t *testing.T) {
-		ctx := post(t, forms.NewReleaseForm{
+		ctx, _ := post(t, forms.NewReleaseForm{
 			TagName: "delete-tag", // a strange name, but it is the only "is_tag=true" fixture
 			Target:  "master",
 			Title:   "updated-title",
@@ -114,7 +116,7 @@ func TestNewReleasePost(t *testing.T) {
 	})
 
 	t.Run("TagOnly", func(t *testing.T) {
-		ctx := post(t, forms.NewReleaseForm{
+		ctx, _ := post(t, forms.NewReleaseForm{
 			TagName: "new-tag-only",
 			Target:  "master",
 			Title:   "title",
@@ -128,7 +130,7 @@ func TestNewReleasePost(t *testing.T) {
 	})
 
 	t.Run("TagOnlyConflict", func(t *testing.T) {
-		ctx := post(t, forms.NewReleaseForm{
+		_, resp := post(t, forms.NewReleaseForm{
 			TagName: "v1.1",
 			Target:  "master",
 			Title:   "title",
@@ -138,7 +140,7 @@ func TestNewReleasePost(t *testing.T) {
 		rel := loadRelease(t, "v1.1")
 		require.NotNil(t, rel)
 		assert.False(t, rel.IsTag)
-		assert.NotEmpty(t, ctx.Flash.ErrorMsg)
+		assert.NotEmpty(t, test.ParseJSONError(resp.Body.Bytes()).ErrorMessage)
 	})
 }
 

services/release/release.go

@@ -20,9 +20,11 @@ import (
 	"gitea.dev/modules/graceful"
 	"gitea.dev/modules/log"
 	"gitea.dev/modules/repository"
+	"gitea.dev/modules/setting"
 	"gitea.dev/modules/storage"
 	"gitea.dev/modules/timeutil"
 	"gitea.dev/modules/util"
+	"gitea.dev/services/context/upload"
 	notify_service "gitea.dev/services/notify"
 )
 
@@ -319,13 +321,17 @@ func UpdateRelease(ctx context.Context, doer *user_model.User, gitRepo *git.Repo
 			}
 
 			for uuid, newName := range editAttachments {
-				if !deletedUUIDs.Contains(uuid) {
-					if err = repo_model.UpdateAttachmentByUUID(ctx, &repo_model.Attachment{
-						UUID: uuid,
-						Name: newName,
-					}, "name"); err != nil {
-						return err
-					}
+				if deletedUUIDs.Contains(uuid) {
+					continue
+				}
+				if err = upload.Verify(nil, newName, setting.Repository.Release.AllowedTypes); err != nil {
+					return err
+				}
+				if err = repo_model.UpdateAttachmentByUUID(ctx, &repo_model.Attachment{
+					UUID: uuid,
+					Name: newName,
+				}, "name"); err != nil {
+					return err
 				}
 			}
 		}

services/release/release_test.go

@@ -12,8 +12,11 @@ import (
 	"gitea.dev/models/unittest"
 	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/gitrepo"
+	"gitea.dev/modules/setting"
+	"gitea.dev/modules/test"
 	"gitea.dev/modules/timeutil"
 	"gitea.dev/services/attachment"
+	"gitea.dev/services/context/upload"
 
 	_ "gitea.dev/models/actions"
 
@@ -270,6 +273,17 @@ func TestRelease_Update(t *testing.T) {
 	assert.Equal(t, release.ID, release.Attachments[0].ReleaseID)
 	assert.Equal(t, "test2.txt", release.Attachments[0].Name)
 
+	defer test.MockVariableValue(&setting.Repository.Release.AllowedTypes, ".zip")()
+	err = UpdateRelease(t.Context(), user, gitRepo, release, nil, nil, map[string]string{
+		attach.UUID: "test.exe",
+	})
+	assert.Error(t, err)
+	assert.True(t, upload.IsErrFileTypeForbidden(err))
+	release.Attachments = nil
+	assert.NoError(t, repo_model.GetReleaseAttachments(t.Context(), release))
+	assert.Len(t, release.Attachments, 1)
+	assert.Equal(t, "test2.txt", release.Attachments[0].Name)
+
 	// delete the attachment
 	assert.NoError(t, UpdateRelease(t.Context(), user, gitRepo, release, nil, []string{attach.UUID}, nil))
 	release.Attachments = nil

templates/repo/release/new.tmpl

@@ -13,7 +13,7 @@
 		</h2>
 		{{template "base/alert" .}}
 
-		<form class="ui form" action="{{.Link}}" method="post" data-global-init="initReleaseEditForm"
+		<form class="ui form form-fetch-action" action="{{.Link}}" method="post" data-global-init="initReleaseEditForm"
 			data-existing-tags="{{JsonUtils.EncodeToString .Tags}}"
 			data-tag-helper="{{ctx.Locale.Tr "repo.release.tag_helper"}}"
 			data-tag-helper-new="{{ctx.Locale.Tr "repo.release.tag_helper_new"}}"

tests/integration/release_test.go

@@ -10,6 +10,7 @@ import (
 
 	repo_model "gitea.dev/models/repo"
 	"gitea.dev/models/unittest"
+	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/setting"
 	"gitea.dev/modules/test"
 	"gitea.dev/modules/translation"
@@ -39,11 +40,10 @@ func createNewRelease(t *testing.T, session *TestSession, repoURL, tag, title st
 	if draft {
 		postData["draft"] = "1"
 	}
+
 	req = NewRequestWithValues(t, "POST", link, postData)
-
-	resp = session.MakeRequest(t, req, http.StatusSeeOther)
-
-	test.RedirectURL(resp) // check that redirect URL exists
+	resp = session.MakeRequest(t, req, http.StatusOK)
+	assert.NotEmpty(t, test.ParseJSONRedirect(resp.Body.Bytes()))
 }
 
 func checkLatestReleaseAndCount(t *testing.T, session *TestSession, repoURL, version, label string, count int) {
@@ -253,3 +253,27 @@ func TestDownloadReleaseAttachment(t *testing.T) {
 	session := loginUser(t, "user2")
 	session.MakeRequest(t, req, http.StatusOK)
 }
+
+func TestEditReleaseAttachmentRejectsForbiddenRename(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+	defer test.MockVariableValue(&setting.Repository.Release.AllowedTypes, ".zip")()
+
+	attachment := unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: 9})
+	release := unittest.AssertExistsAndLoadBean(t, &repo_model.Release{ID: attachment.ReleaseID})
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: attachment.RepoID})
+	repoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
+
+	session := loginUser(t, repoOwner.Name)
+	req := NewRequestWithValues(t, "POST", fmt.Sprintf("%s/releases/edit/%s", repo.Link(), release.TagName), map[string]string{
+		"title":                              release.Title,
+		"content":                            release.Note,
+		"attachment-edit-" + attachment.UUID: "evil.exe",
+	})
+
+	resp := session.MakeRequest(t, req, http.StatusBadRequest)
+	errMsg := test.ParseJSONError(resp.Body.Bytes()).ErrorMessage
+	assert.Equal(t, "This file cannot be uploaded or modified due to a forbidden file extension or type.", errMsg)
+
+	attachment = unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: attachment.ID})
+	assert.NotEqual(t, "evil.exe", attachment.Name)
+}