From 872caa17c0a30d95f85ab75c068d606e07bd10b3 Mon Sep 17 00:00:00 2001 From: Kemal Zebari <60799661+kemzeb@users.noreply.github.com> Date: Thu, 2 May 2024 09:33:31 -0700 Subject: [PATCH] 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. --- routers/api/v1/repo/issue_attachment.go | 9 +++++- .../api/v1/repo/issue_comment_attachment.go | 10 ++++++- templates/swagger/v1_json.tmpl | 6 ++++ .../api_comment_attachment_test.go | 28 +++++++++++++++++++ .../integration/api_issue_attachment_test.go | 27 ++++++++++++++++++ 5 files changed, 78 insertions(+), 2 deletions(-) diff --git a/routers/api/v1/repo/issue_attachment.go b/routers/api/v1/repo/issue_attachment.go index 7a5c6d554..f5a28e6fa 100644 --- 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 4096cbf07..77aa7f040 100644 --- 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 0c5e5c974..5ca499e70 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -7478,6 +7478,9 @@ "404": { "$ref": "#/responses/error" }, + "422": { + "$ref": "#/responses/validationError" + }, "423": { "$ref": "#/responses/repoArchivedError" } @@ -8097,6 +8100,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 2d7587bbd..0ec950d4c 100644 --- 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 497dd0155..b4196ec6d 100644 --- 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)()