Catch and handle unallowed file type errors in issue attachment API (#…

…30791) Before, we would just throw 500 if a user passes an attachment that is not an allowed type. This commit catches this error and throws a 422 instead since this should be considered a validation error.
go-gitea · May 2, 2024 · 872caa1 · 872caa1
1 parent 677032d
commit 872caa1
Show file tree

Hide file tree

Showing 5 changed files with 78 additions and 2 deletions.
diff --git a/routers/api/v1/repo/issue_attachment.go b/routers/api/v1/repo/issue_attachment.go
@@ -14,6 +14,7 @@ import (
 	"code.gitea.io/gitea/modules/web"
 	"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"
 	issue_service "code.gitea.io/gitea/services/issue"
 )
@@ -153,6 +154,8 @@ func CreateIssueAttachment(ctx *context.APIContext) {
 	//     "$ref": "#/responses/error"
 	//   "404":
 	//     "$ref": "#/responses/error"
+	//   "422":
+	//     "$ref": "#/responses/validationError"
 	//   "423":
 	//     "$ref": "#/responses/repoArchivedError"
 
@@ -185,7 +188,11 @@ func CreateIssueAttachment(ctx *context.APIContext) {
 		IssueID:    issue.ID,
 	})
 	if err != nil {
-		ctx.Error(http.StatusInternalServerError, "UploadAttachment", err)
+		if upload.IsErrFileTypeForbidden(err) {
+			ctx.Error(http.StatusUnprocessableEntity, "", err)
+		} else {
+			ctx.Error(http.StatusInternalServerError, "UploadAttachment", err)
+		}
 		return
 	}
 

diff --git a/routers/api/v1/repo/issue_comment_attachment.go b/routers/api/v1/repo/issue_comment_attachment.go
@@ -16,6 +16,7 @@ import (
 	"code.gitea.io/gitea/modules/web"
 	"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"
 	issue_service "code.gitea.io/gitea/services/issue"
 )
@@ -160,6 +161,8 @@ func CreateIssueCommentAttachment(ctx *context.APIContext) {
 	//     "$ref": "#/responses/forbidden"
 	//   "404":
 	//     "$ref": "#/responses/error"
+	//   "422":
+	//     "$ref": "#/responses/validationError"
 	//   "423":
 	//     "$ref": "#/responses/repoArchivedError"
 
@@ -194,9 +197,14 @@ func CreateIssueCommentAttachment(ctx *context.APIContext) {
 		CommentID:  comment.ID,
 	})
 	if err != nil {
-		ctx.Error(http.StatusInternalServerError, "UploadAttachment", err)
+		if upload.IsErrFileTypeForbidden(err) {
+			ctx.Error(http.StatusUnprocessableEntity, "", err)
+		} else {
+			ctx.Error(http.StatusInternalServerError, "UploadAttachment", err)
+		}
 		return
 	}
+
 	if err := comment.LoadAttachments(ctx); err != nil {
 		ctx.Error(http.StatusInternalServerError, "LoadAttachments", err)
 		return

diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl
diff --git a/tests/integration/api_comment_attachment_test.go b/tests/integration/api_comment_attachment_test.go
@@ -120,6 +120,34 @@ func TestAPICreateCommentAttachment(t *testing.T) {
 	unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: apiAttachment.ID, CommentID: comment.ID})
 }
 
+func TestAPICreateCommentAttachmentWithUnallowedFile(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 2})
+	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"
+	body := &bytes.Buffer{}
+
+	// Setup multi-part.
+	writer := multipart.NewWriter(body)
+	_, err := writer.CreateFormFile("attachment", filename)
+	assert.NoError(t, err)
+	err = writer.Close()
+	assert.NoError(t, err)
+
+	req := NewRequestWithBody(t, "POST", fmt.Sprintf("/api/v1/repos/%s/%s/issues/comments/%d/assets", repoOwner.Name, repo.Name, comment.ID), body).
+		AddTokenAuth(token).
+		SetHeader("Content-Type", writer.FormDataContentType())
+
+	session.MakeRequest(t, req, http.StatusUnprocessableEntity)
+}
+
 func TestAPIEditCommentAttachment(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 

diff --git a/tests/integration/api_issue_attachment_test.go b/tests/integration/api_issue_attachment_test.go
@@ -96,6 +96,33 @@ func TestAPICreateIssueAttachment(t *testing.T) {
 	unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: apiAttachment.ID, IssueID: issue.ID})
 }
 
+func TestAPICreateIssueAttachmentWithUnallowedFile(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
+	issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{RepoID: repo.ID})
+	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"
+	body := &bytes.Buffer{}
+
+	// Setup multi-part.
+	writer := multipart.NewWriter(body)
+	_, err := writer.CreateFormFile("attachment", filename)
+	assert.NoError(t, err)
+	err = writer.Close()
+	assert.NoError(t, err)
+
+	req := NewRequestWithBody(t, "POST", fmt.Sprintf("/api/v1/repos/%s/%s/issues/%d/assets", repoOwner.Name, repo.Name, issue.Index), body).
+		AddTokenAuth(token)
+	req.Header.Add("Content-Type", writer.FormDataContentType())
+
+	session.MakeRequest(t, req, http.StatusUnprocessableEntity)
+}
+
 func TestAPIEditIssueAttachment(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()