Skip to content

Commit 35b44c2

Browse files
kemzebkemzeb
committed
Include file extension checks in attachment API
1 parent 7cf611d commit 35b44c2

File tree

8 files changed

+108
-17
lines changed

8 files changed

+108
-17
lines changed

‎routers/api/v1/repo/issue_attachment.go‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import (
1212
"code.gitea.io/gitea/modules/setting"
1313
api "code.gitea.io/gitea/modules/structs"
1414
"code.gitea.io/gitea/modules/web"
15-
"code.gitea.io/gitea/services/attachment"
15+
attachment_service "code.gitea.io/gitea/services/attachment"
1616
"code.gitea.io/gitea/services/context"
1717
"code.gitea.io/gitea/services/context/upload"
1818
"code.gitea.io/gitea/services/convert"
@@ -181,7 +181,7 @@ func CreateIssueAttachment(ctx *context.APIContext) {
181181
filename = query
182182
}
183183

184-
attachment, err := attachment.UploadAttachment(ctx, file, setting.Attachment.AllowedTypes, header.Size, &repo_model.Attachment{
184+
attachment, err := attachment_service.UploadAttachment(ctx, file, setting.Attachment.AllowedTypes, header.Size, &repo_model.Attachment{
185185
Name: filename,
186186
UploaderID: ctx.Doer.ID,
187187
RepoID: ctx.Repo.Repository.ID,
@@ -247,6 +247,8 @@ func EditIssueAttachment(ctx *context.APIContext) {
247247
// "$ref": "#/responses/Attachment"
248248
// "404":
249249
// "$ref": "#/responses/error"
250+
// "422":
251+
// "$ref": "#/responses/validationError"
250252
// "423":
251253
// "$ref": "#/responses/repoArchivedError"
252254

@@ -261,8 +263,13 @@ func EditIssueAttachment(ctx *context.APIContext) {
261263
attachment.Name = form.Name
262264
}
263265

264-
if err := repo_model.UpdateAttachment(ctx, attachment); err != nil {
266+
if err := attachment_service.UpdateAttachment(ctx, setting.Attachment.AllowedTypes, attachment); err != nil {
267+
if upload.IsErrFileTypeForbidden(err) {
268+
ctx.Error(http.StatusUnprocessableEntity, "", err)
269+
return
270+
}
265271
ctx.Error(http.StatusInternalServerError, "UpdateAttachment", err)
272+
return
266273
}
267274

268275
ctx.JSON(http.StatusCreated, convert.ToAPIAttachment(ctx.Repo.Repository, attachment))

‎routers/api/v1/repo/issue_comment_attachment.go‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import (
1414
"code.gitea.io/gitea/modules/setting"
1515
api "code.gitea.io/gitea/modules/structs"
1616
"code.gitea.io/gitea/modules/web"
17-
"code.gitea.io/gitea/services/attachment"
17+
attachment_service "code.gitea.io/gitea/services/attachment"
1818
"code.gitea.io/gitea/services/context"
1919
"code.gitea.io/gitea/services/context/upload"
2020
"code.gitea.io/gitea/services/convert"
@@ -189,7 +189,7 @@ func CreateIssueCommentAttachment(ctx *context.APIContext) {
189189
filename = query
190190
}
191191

192-
attachment, err := attachment.UploadAttachment(ctx, file, setting.Attachment.AllowedTypes, header.Size, &repo_model.Attachment{
192+
attachment, err := attachment_service.UploadAttachment(ctx, file, setting.Attachment.AllowedTypes, header.Size, &repo_model.Attachment{
193193
Name: filename,
194194
UploaderID: ctx.Doer.ID,
195195
RepoID: ctx.Repo.Repository.ID,
@@ -263,6 +263,8 @@ func EditIssueCommentAttachment(ctx *context.APIContext) {
263263
// "$ref": "#/responses/Attachment"
264264
// "404":
265265
// "$ref": "#/responses/error"
266+
// "422":
267+
// "$ref": "#/responses/validationError"
266268
// "423":
267269
// "$ref": "#/responses/repoArchivedError"
268270
attach := getIssueCommentAttachmentSafeWrite(ctx)
@@ -275,8 +277,13 @@ func EditIssueCommentAttachment(ctx *context.APIContext) {
275277
attach.Name = form.Name
276278
}
277279

278-
if err := repo_model.UpdateAttachment(ctx, attach); err != nil {
280+
if err := attachment_service.UpdateAttachment(ctx, setting.Attachment.AllowedTypes, attach); err != nil {
281+
if upload.IsErrFileTypeForbidden(err) {
282+
ctx.Error(http.StatusUnprocessableEntity, "", err)
283+
return
284+
}
279285
ctx.Error(http.StatusInternalServerError, "UpdateAttachment", attach)
286+
return
280287
}
281288
ctx.JSON(http.StatusCreated, convert.ToAPIAttachment(ctx.Repo.Repository, attach))
282289
}

‎routers/api/v1/repo/release_attachment.go‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import (
1313
"code.gitea.io/gitea/modules/setting"
1414
api "code.gitea.io/gitea/modules/structs"
1515
"code.gitea.io/gitea/modules/web"
16-
"code.gitea.io/gitea/services/attachment"
16+
attachment_service "code.gitea.io/gitea/services/attachment"
1717
"code.gitea.io/gitea/services/context"
1818
"code.gitea.io/gitea/services/context/upload"
1919
"code.gitea.io/gitea/services/convert"
@@ -234,7 +234,7 @@ func CreateReleaseAttachment(ctx *context.APIContext) {
234234
}
235235

236236
// Create a new attachment and save the file
237-
attach, err := attachment.UploadAttachment(ctx, content, setting.Repository.Release.AllowedTypes, size, &repo_model.Attachment{
237+
attach, err := attachment_service.UploadAttachment(ctx, content, setting.Repository.Release.AllowedTypes, size, &repo_model.Attachment{
238238
Name: filename,
239239
UploaderID: ctx.Doer.ID,
240240
RepoID: ctx.Repo.Repository.ID,
@@ -291,6 +291,8 @@ func EditReleaseAttachment(ctx *context.APIContext) {
291291
// responses:
292292
// "201":
293293
// "$ref": "#/responses/Attachment"
294+
// "422":
295+
// "$ref": "#/responses/validationError"
294296
// "404":
295297
// "$ref": "#/responses/notFound"
296298

@@ -322,8 +324,13 @@ func EditReleaseAttachment(ctx *context.APIContext) {
322324
attach.Name = form.Name
323325
}
324326

325-
if err := repo_model.UpdateAttachment(ctx, attach); err != nil {
327+
if err := attachment_service.UpdateAttachment(ctx, setting.Repository.Release.AllowedTypes, attach); err != nil {
328+
if upload.IsErrFileTypeForbidden(err) {
329+
ctx.Error(http.StatusUnprocessableEntity, "", err)
330+
return
331+
}
326332
ctx.Error(http.StatusInternalServerError, "UpdateAttachment", attach)
333+
return
327334
}
328335
ctx.JSON(http.StatusCreated, convert.ToAPIAttachment(ctx.Repo.Repository, attach))
329336
}

‎services/attachment/attachment.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,3 +50,12 @@ func UploadAttachment(ctx context.Context, file io.Reader, allowedTypes string,
5050

5151
return NewAttachment(ctx, attach, io.MultiReader(bytes.NewReader(buf), file), fileSize)
5252
}
53+
54+
// UpdateAttachment updates an attachment, verifying that its name is among the allowed types.
55+
func UpdateAttachment(ctx context.Context, allowedTypes string, attach *repo_model.Attachment) error {
56+
if err := upload.Verify(nil, attach.Name, allowedTypes); err != nil {
57+
return err
58+
}
59+
60+
return repo_model.UpdateAttachment(ctx, attach)
61+
}

‎services/context/upload/upload.go‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,13 @@ func IsErrFileTypeForbidden(err error) bool {
2828
}
2929

3030
func (err ErrFileTypeForbidden) Error() string {
31-
return "This file extension or type is not allowed to be uploaded."
31+
return "This file cannot be uploaded or modified due to a forbidden file extension or type."
3232
}
3333

3434
var wildcardTypeRe = regexp.MustCompile(`^[a-z]+/\*$`)
3535

36-
// Verify validates whether a file is allowed to be uploaded.
36+
// Verify validates whether a file is allowed to be uploaded. If buf is empty, it will just check if the file
37+
// has an allowed file extension.
3738
func Verify(buf []byte, fileName, allowedTypesStr string) error {
3839
allowedTypesStr = strings.ReplaceAll(allowedTypesStr, "|", ",") // compat for old config format
3940

@@ -56,21 +57,31 @@ func Verify(buf []byte, fileName, allowedTypesStr string) error {
5657
return ErrFileTypeForbidden{Type: fullMimeType}
5758
}
5859
extension := strings.ToLower(path.Ext(fileName))
60+
isBufEmpty := len(buf) <= 1
5961

6062
// https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file#Unique_file_type_specifiers
6163
for _, allowEntry := range allowedTypes {
6264
if allowEntry == "*/*" {
6365
return nil // everything allowed
64-
} else if strings.HasPrefix(allowEntry, ".") && allowEntry == extension {
66+
}
67+
if strings.HasPrefix(allowEntry, ".") && allowEntry == extension {
6568
return nil // extension is allowed
66-
} else if mimeType == allowEntry {
69+
}
70+
if isBufEmpty {
71+
continue // skip mime type checks if buffer is empty
72+
}
73+
if mimeType == allowEntry {
6774
return nil // mime type is allowed
68-
} else if wildcardTypeRe.MatchString(allowEntry) && strings.HasPrefix(mimeType, allowEntry[:len(allowEntry)-1]) {
75+
}
76+
if wildcardTypeRe.MatchString(allowEntry) && strings.HasPrefix(mimeType, allowEntry[:len(allowEntry)-1]) {
6977
return nil // wildcard match, e.g. image/*
7078
}
7179
}
7280

73-
log.Info("Attachment with type %s blocked from upload", fullMimeType)
81+
if !isBufEmpty {
82+
log.Info("Attachment with type %s blocked from upload", fullMimeType)
83+
}
84+
7485
return ErrFileTypeForbidden{Type: fullMimeType}
7586
}
7687

‎templates/swagger/v1_json.tmpl‎

Lines changed: 9 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎tests/integration/api_comment_attachment_test.go‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ func TestAPICreateCommentAttachmentWithUnallowedFile(t *testing.T) {
151151
func TestAPIEditCommentAttachment(t *testing.T) {
152152
defer tests.PrepareTestEnv(t)()
153153

154-
const newAttachmentName = "newAttachmentName"
154+
const newAttachmentName = "newAttachmentName.txt"
155155

156156
attachment := unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: 6})
157157
comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: attachment.CommentID})
@@ -173,6 +173,27 @@ func TestAPIEditCommentAttachment(t *testing.T) {
173173
unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: apiAttachment.ID, CommentID: comment.ID, Name: apiAttachment.Name})
174174
}
175175

176+
func TestAPIEditCommentAttachmentWithUnallowedFile(t *testing.T) {
177+
defer tests.PrepareTestEnv(t)()
178+
179+
attachment := unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: 6})
180+
comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: attachment.CommentID})
181+
issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: comment.IssueID})
182+
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: issue.RepoID})
183+
repoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
184+
session := loginUser(t, repoOwner.Name)
185+
token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteIssue)
186+
187+
filename := "file.bad"
188+
urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/issues/comments/%d/assets/%d",
189+
repoOwner.Name, repo.Name, comment.ID, attachment.ID)
190+
req := NewRequestWithValues(t, "PATCH", urlStr, map[string]string{
191+
"name": filename,
192+
}).AddTokenAuth(token)
193+
194+
session.MakeRequest(t, req, http.StatusUnprocessableEntity)
195+
}
196+
176197
func TestAPIDeleteCommentAttachment(t *testing.T) {
177198
defer tests.PrepareTestEnv(t)()
178199

‎tests/integration/api_issue_attachment_test.go‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ func TestAPICreateIssueAttachmentWithUnallowedFile(t *testing.T) {
126126
func TestAPIEditIssueAttachment(t *testing.T) {
127127
defer tests.PrepareTestEnv(t)()
128128

129-
const newAttachmentName = "newAttachmentName"
129+
const newAttachmentName = "hello_world.txt"
130130

131131
attachment := unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: 1})
132132
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: attachment.RepoID})
@@ -147,6 +147,26 @@ func TestAPIEditIssueAttachment(t *testing.T) {
147147
unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: apiAttachment.ID, IssueID: issue.ID, Name: apiAttachment.Name})
148148
}
149149

150+
func TestAPIEditIssueAttachmentWithUnallowedFile(t *testing.T) {
151+
defer tests.PrepareTestEnv(t)()
152+
153+
attachment := unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: 1})
154+
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: attachment.RepoID})
155+
issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{RepoID: attachment.IssueID})
156+
repoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
157+
158+
session := loginUser(t, repoOwner.Name)
159+
token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteIssue)
160+
161+
filename := "file.bad"
162+
urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/issues/%d/assets/%d", repoOwner.Name, repo.Name, issue.Index, attachment.ID)
163+
req := NewRequestWithValues(t, "PATCH", urlStr, map[string]string{
164+
"name": filename,
165+
}).AddTokenAuth(token)
166+
167+
session.MakeRequest(t, req, http.StatusUnprocessableEntity)
168+
}
169+
150170
func TestAPIDeleteIssueAttachment(t *testing.T) {
151171
defer tests.PrepareTestEnv(t)()
152172

0 commit comments

Comments
 (0)