Include file extension checks in attachment API (#32151)

From testing, I found that issue posters and users with repository write
access are able to edit attachment names in a way that circumvents the
instance-level file extension restrictions using the edit attachment
APIs. This snapshot adds checks for these endpoints.

Author: kemzeb

Diff for: routers/api/v1/repo/issue_attachment.go

@@ -12,7 +12,7 @@ import (
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/web"
-	"code.gitea.io/gitea/services/attachment"
+	attachment_service "code.gitea.io/gitea/services/attachment"
 	"code.gitea.io/gitea/services/context"
 	"code.gitea.io/gitea/services/context/upload"
 	"code.gitea.io/gitea/services/convert"
@@ -181,7 +181,7 @@ func CreateIssueAttachment(ctx *context.APIContext) {
 		filename = query
 	}
 
-	attachment, err := attachment.UploadAttachment(ctx, file, setting.Attachment.AllowedTypes, header.Size, &repo_model.Attachment{
+	attachment, err := attachment_service.UploadAttachment(ctx, file, setting.Attachment.AllowedTypes, header.Size, &repo_model.Attachment{
 		Name:       filename,
 		UploaderID: ctx.Doer.ID,
 		RepoID:     ctx.Repo.Repository.ID,
@@ -247,6 +247,8 @@ func EditIssueAttachment(ctx *context.APIContext) {
 	//     "$ref": "#/responses/Attachment"
 	//   "404":
 	//     "$ref": "#/responses/error"
+	//   "422":
+	//     "$ref": "#/responses/validationError"
 	//   "423":
 	//     "$ref": "#/responses/repoArchivedError"
 
@@ -261,8 +263,13 @@ func EditIssueAttachment(ctx *context.APIContext) {
 		attachment.Name = form.Name
 	}
 
-	if err := repo_model.UpdateAttachment(ctx, attachment); err != nil {
+	if err := attachment_service.UpdateAttachment(ctx, setting.Attachment.AllowedTypes, attachment); err != nil {
+		if upload.IsErrFileTypeForbidden(err) {
+			ctx.Error(http.StatusUnprocessableEntity, "", err)
+			return
+		}
 		ctx.Error(http.StatusInternalServerError, "UpdateAttachment", err)
+		return
 	}
 
 	ctx.JSON(http.StatusCreated, convert.ToAPIAttachment(ctx.Repo.Repository, attachment))

Diff for: routers/api/v1/repo/issue_comment_attachment.go

@@ -14,7 +14,7 @@ import (
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/web"
-	"code.gitea.io/gitea/services/attachment"
+	attachment_service "code.gitea.io/gitea/services/attachment"
 	"code.gitea.io/gitea/services/context"
 	"code.gitea.io/gitea/services/context/upload"
 	"code.gitea.io/gitea/services/convert"
@@ -189,7 +189,7 @@ func CreateIssueCommentAttachment(ctx *context.APIContext) {
 		filename = query
 	}
 
-	attachment, err := attachment.UploadAttachment(ctx, file, setting.Attachment.AllowedTypes, header.Size, &repo_model.Attachment{
+	attachment, err := attachment_service.UploadAttachment(ctx, file, setting.Attachment.AllowedTypes, header.Size, &repo_model.Attachment{
 		Name:       filename,
 		UploaderID: ctx.Doer.ID,
 		RepoID:     ctx.Repo.Repository.ID,
@@ -263,6 +263,8 @@ func EditIssueCommentAttachment(ctx *context.APIContext) {
 	//     "$ref": "#/responses/Attachment"
 	//   "404":
 	//     "$ref": "#/responses/error"
+	//   "422":
+	//     "$ref": "#/responses/validationError"
 	//   "423":
 	//     "$ref": "#/responses/repoArchivedError"
 	attach := getIssueCommentAttachmentSafeWrite(ctx)
@@ -275,8 +277,13 @@ func EditIssueCommentAttachment(ctx *context.APIContext) {
 		attach.Name = form.Name
 	}
 
-	if err := repo_model.UpdateAttachment(ctx, attach); err != nil {
+	if err := attachment_service.UpdateAttachment(ctx, setting.Attachment.AllowedTypes, attach); err != nil {
+		if upload.IsErrFileTypeForbidden(err) {
+			ctx.Error(http.StatusUnprocessableEntity, "", err)
+			return
+		}
 		ctx.Error(http.StatusInternalServerError, "UpdateAttachment", attach)
+		return
 	}
 	ctx.JSON(http.StatusCreated, convert.ToAPIAttachment(ctx.Repo.Repository, attach))
 }

Diff for: routers/api/v1/repo/release_attachment.go

@@ -13,7 +13,7 @@ import (
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/web"
-	"code.gitea.io/gitea/services/attachment"
+	attachment_service "code.gitea.io/gitea/services/attachment"
 	"code.gitea.io/gitea/services/context"
 	"code.gitea.io/gitea/services/context/upload"
 	"code.gitea.io/gitea/services/convert"
@@ -234,7 +234,7 @@ func CreateReleaseAttachment(ctx *context.APIContext) {
 	}
 
 	// Create a new attachment and save the file
-	attach, err := attachment.UploadAttachment(ctx, content, setting.Repository.Release.AllowedTypes, size, &repo_model.Attachment{
+	attach, err := attachment_service.UploadAttachment(ctx, content, setting.Repository.Release.AllowedTypes, size, &repo_model.Attachment{
 		Name:       filename,
 		UploaderID: ctx.Doer.ID,
 		RepoID:     ctx.Repo.Repository.ID,
@@ -291,6 +291,8 @@ func EditReleaseAttachment(ctx *context.APIContext) {
 	// responses:
 	//   "201":
 	//     "$ref": "#/responses/Attachment"
+	//   "422":
+	//     "$ref": "#/responses/validationError"
 	//   "404":
 	//     "$ref": "#/responses/notFound"
 
@@ -322,8 +324,13 @@ func EditReleaseAttachment(ctx *context.APIContext) {
 		attach.Name = form.Name
 	}
 
-	if err := repo_model.UpdateAttachment(ctx, attach); err != nil {
+	if err := attachment_service.UpdateAttachment(ctx, setting.Repository.Release.AllowedTypes, attach); err != nil {
+		if upload.IsErrFileTypeForbidden(err) {
+			ctx.Error(http.StatusUnprocessableEntity, "", err)
+			return
+		}
 		ctx.Error(http.StatusInternalServerError, "UpdateAttachment", attach)
+		return
 	}
 	ctx.JSON(http.StatusCreated, convert.ToAPIAttachment(ctx.Repo.Repository, attach))
 }

Diff for: services/attachment/attachment.go

@@ -50,3 +50,12 @@ func UploadAttachment(ctx context.Context, file io.Reader, allowedTypes string,
 
 	return NewAttachment(ctx, attach, io.MultiReader(bytes.NewReader(buf), file), fileSize)
 }
+
+// UpdateAttachment updates an attachment, verifying that its name is among the allowed types.
+func UpdateAttachment(ctx context.Context, allowedTypes string, attach *repo_model.Attachment) error {
+	if err := upload.Verify(nil, attach.Name, allowedTypes); err != nil {
+		return err
+	}
+
+	return repo_model.UpdateAttachment(ctx, attach)
+}

Diff for: services/context/upload/upload.go

@@ -28,12 +28,13 @@ func IsErrFileTypeForbidden(err error) bool {
 }
 
 func (err ErrFileTypeForbidden) Error() string {
-	return "This file extension or type is not allowed to be uploaded."
+	return "This file cannot be uploaded or modified due to a forbidden file extension or type."
 }
 
 var wildcardTypeRe = regexp.MustCompile(`^[a-z]+/\*$`)
 
-// Verify validates whether a file is allowed to be uploaded.
+// Verify validates whether a file is allowed to be uploaded. If buf is empty, it will just check if the file
+// has an allowed file extension.
 func Verify(buf []byte, fileName, allowedTypesStr string) error {
 	allowedTypesStr = strings.ReplaceAll(allowedTypesStr, "|", ",") // compat for old config format
 
@@ -56,21 +57,31 @@ func Verify(buf []byte, fileName, allowedTypesStr string) error {
 		return ErrFileTypeForbidden{Type: fullMimeType}
 	}
 	extension := strings.ToLower(path.Ext(fileName))
+	isBufEmpty := len(buf) <= 1
 
 	// https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file#Unique_file_type_specifiers
 	for _, allowEntry := range allowedTypes {
 		if allowEntry == "*/*" {
 			return nil // everything allowed
-		} else if strings.HasPrefix(allowEntry, ".") && allowEntry == extension {
+		}
+		if strings.HasPrefix(allowEntry, ".") && allowEntry == extension {
 			return nil // extension is allowed
-		} else if mimeType == allowEntry {
+		}
+		if isBufEmpty {
+			continue // skip mime type checks if buffer is empty
+		}
+		if mimeType == allowEntry {
 			return nil // mime type is allowed
-		} else if wildcardTypeRe.MatchString(allowEntry) && strings.HasPrefix(mimeType, allowEntry[:len(allowEntry)-1]) {
+		}
+		if wildcardTypeRe.MatchString(allowEntry) && strings.HasPrefix(mimeType, allowEntry[:len(allowEntry)-1]) {
 			return nil // wildcard match, e.g. image/*
 		}
 	}
 
-	log.Info("Attachment with type %s blocked from upload", fullMimeType)
+	if !isBufEmpty {
+		log.Info("Attachment with type %s blocked from upload", fullMimeType)
+	}
+
 	return ErrFileTypeForbidden{Type: fullMimeType}
 }
 

Diff for: templates/swagger/v1_json.tmpl

@@ -151,7 +151,7 @@ func TestAPICreateCommentAttachmentWithUnallowedFile(t *testing.T) {
 func TestAPIEditCommentAttachment(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 
-	const newAttachmentName = "newAttachmentName"
+	const newAttachmentName = "newAttachmentName.txt"
 
 	attachment := unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: 6})
 	comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: attachment.CommentID})
@@ -173,6 +173,27 @@ func TestAPIEditCommentAttachment(t *testing.T) {
 	unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: apiAttachment.ID, CommentID: comment.ID, Name: apiAttachment.Name})
 }
 
+func TestAPIEditCommentAttachmentWithUnallowedFile(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	attachment := unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: 6})
+	comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: attachment.CommentID})
+	issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: comment.IssueID})
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: issue.RepoID})
+	repoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
+	session := loginUser(t, repoOwner.Name)
+	token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteIssue)
+
+	filename := "file.bad"
+	urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/issues/comments/%d/assets/%d",
+		repoOwner.Name, repo.Name, comment.ID, attachment.ID)
+	req := NewRequestWithValues(t, "PATCH", urlStr, map[string]string{
+		"name": filename,
+	}).AddTokenAuth(token)
+
+	session.MakeRequest(t, req, http.StatusUnprocessableEntity)
+}
+
 func TestAPIDeleteCommentAttachment(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 

Diff for: tests/integration/api_comment_attachment_test.go

@@ -126,7 +126,7 @@ func TestAPICreateIssueAttachmentWithUnallowedFile(t *testing.T) {
 func TestAPIEditIssueAttachment(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 
-	const newAttachmentName = "newAttachmentName"
+	const newAttachmentName = "hello_world.txt"
 
 	attachment := unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: 1})
 	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: attachment.RepoID})
@@ -147,6 +147,26 @@ func TestAPIEditIssueAttachment(t *testing.T) {
 	unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: apiAttachment.ID, IssueID: issue.ID, Name: apiAttachment.Name})
 }
 
+func TestAPIEditIssueAttachmentWithUnallowedFile(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	attachment := unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: 1})
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: attachment.RepoID})
+	issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: attachment.IssueID})
+	repoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
+
+	session := loginUser(t, repoOwner.Name)
+	token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteIssue)
+
+	filename := "file.bad"
+	urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/issues/%d/assets/%d", repoOwner.Name, repo.Name, issue.Index, attachment.ID)
+	req := NewRequestWithValues(t, "PATCH", urlStr, map[string]string{
+		"name": filename,
+	}).AddTokenAuth(token)
+
+	session.MakeRequest(t, req, http.StatusUnprocessableEntity)
+}
+
 func TestAPIDeleteIssueAttachment(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 

Diff for: tests/integration/api_issue_attachment_test.go

@@ -0,0 +1,40 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package integration
+
+import (
+	"fmt"
+	"net/http"
+	"testing"
+
+	auth_model "code.gitea.io/gitea/models/auth"
+	repo_model "code.gitea.io/gitea/models/repo"
+	"code.gitea.io/gitea/models/unittest"
+	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/test"
+	"code.gitea.io/gitea/tests"
+)
+
+func TestAPIEditReleaseAttachmentWithUnallowedFile(t *testing.T) {
+	// Limit the allowed release types (since by default there is no restriction)
+	defer test.MockVariableValue(&setting.Repository.Release.AllowedTypes, ".exe")()
+	defer tests.PrepareTestEnv(t)()
+
+	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)
+	token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
+
+	filename := "file.bad"
+	urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/releases/%d/assets/%d", repoOwner.Name, repo.Name, release.ID, attachment.ID)
+	req := NewRequestWithValues(t, "PATCH", urlStr, map[string]string{
+		"name": filename,
+	}).AddTokenAuth(token)
+
+	session.MakeRequest(t, req, http.StatusUnprocessableEntity)
+}