Catch and handle unallowed file type errors in issue attachment API (#30791) (#30834)

Backport #30791 by kemzeb

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.

Co-authored-by: Kemal Zebari <60799661+kemzeb@users.noreply.github.com>

diff --git a/routers/api/v1/repo/issue_attachment.go b/routers/api/v1/repo/issue_attachment.go
index 7a5c6d554da4f00ada6d491413ad361f98b207e2..f5a28e6fa6bb1d5029dec27765b58e767bedb29f 100644 (file)
--- 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
index 4096cbf07b02513e3d33309dbb8764f9de4b66c7..77aa7f04004991126faec47a5aad665d4c2c56ef 100644 (file)
--- 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
index 362a847332ad59741ed5cf2c65fbe80b4a0ab245..e10018bba7b6aa48250e9ce6abe4ea600812da12 100644 (file)
--- a/templates/swagger/v1_json.tmpl
+++ b/templates/swagger/v1_json.tmpl
@@ -7418,6 +7418,9 @@
           "404": {
             "$ref": "#/responses/error"
           },
+          "422": {
+            "$ref": "#/responses/validationError"
+          },
           "423": {
             "$ref": "#/responses/repoArchivedError"
           }
@@ -8037,6 +8040,9 @@
           "404": {
             "$ref": "#/responses/error"
           },
+          "422": {
+            "$ref": "#/responses/validationError"
+          },
           "423": {
             "$ref": "#/responses/repoArchivedError"
           }

diff --git a/tests/integration/api_comment_attachment_test.go b/tests/integration/api_comment_attachment_test.go
index 2d7587bbde9f6b1d616c048c771ebae11296a0e9..0ec950d4c2e0feda436b56a0cad2d20871918bb7 100644 (file)
--- 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
index 497dd0155e0b0fd7fd653e8507fd7eb3d20acc99..b4196ec6dbbbb5ca9813f4dd86b5201e7b5e688a 100644 (file)
--- 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)()