Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include file extension checks in attachment API #32151

Merged
merged 5 commits into from
Nov 6, 2024
Merged

Include file extension checks in attachment API #32151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
35b44c2
Include file extension checks in attachment API
kemzeb Sep 29, 2024
d377e25
Add release attachment test and fix test typo
kemzeb Oct 26, 2024
f9880a5
Make release test actually pass
kemzeb Oct 26, 2024
78ba2c0
Merge branch 'main' into fix-api-attachments
GiteaBot Nov 6, 2024
864dc5a
Merge branch 'main' into fix-api-attachments
GiteaBot Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions routers/api/v1/repo/issue_attachment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -247,6 +247,8 @@ func EditIssueAttachment(ctx *context.APIContext) {
// "$ref": "#/responses/Attachment"
// "404":
// "$ref": "#/responses/error"
// "422":
// "$ref": "#/responses/validationError"
// "423":
// "$ref": "#/responses/repoArchivedError"

Expand All @@ -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))
Expand Down
13 changes: 10 additions & 3 deletions routers/api/v1/repo/issue_comment_attachment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand All @@ -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))
}
Expand Down
13 changes: 10 additions & 3 deletions routers/api/v1/repo/release_attachment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -291,6 +291,8 @@ func EditReleaseAttachment(ctx *context.APIContext) {
// responses:
// "201":
// "$ref": "#/responses/Attachment"
// "422":
// "$ref": "#/responses/validationError"
// "404":
// "$ref": "#/responses/notFound"

Expand Down Expand Up @@ -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))
}
Expand Down
9 changes: 9 additions & 0 deletions services/attachment/attachment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
23 changes: 17 additions & 6 deletions services/context/upload/upload.go
View file
Open in desktop
kemzeb marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
}
Comment on lines +70 to +72
Copy link
Contributor Author

@kemzeb kemzeb Sep 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One problem with this implementation is that if we have the following conditions:

  • The attachment is just an empty file
  • We have a file extension that is not allowed
  • text/plain is an allowed type

We would return an error here, even though we may expect it not to (since stdlib treats empty files as text/plain). Instead of changing Verify() too much I could just create a separate func called VerifyFileExtensionOnly().

Then again, as I look at my example above, the logic in Verify() is interesting seeing that if you have a potential attachment with the following:

  • A filename that uses an extension that is not allowed
  • A mime type that is allowed
  • (or vice-versa)

... this would pass the verification checks. The current implementation does not follow this behavior and just checks if the filename is allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently I see a few approaches to this problem:

  • Store the MIME type of the attachment so that we can use it to be consistent with how we currently perform verification checks (we don't seem to have this in the attachment table)
  • Keep the logic of upload.Verify() unchanged. But for subsequent edits to the attachment name verify that it uses an approved file extension
  • Modify the logic of upload.Verify() so that both the file extension and MIME type must be allowed in order for the attachment to be allowed. Any subsequent edits of the attachment name would have us verify that it uses an approved extension (i.e. it would be inline with the new behavior)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far I can think of the third approach may suits best in this situation
becuase it will handle both MIME type and file extension .
it can be implemented something like this

func Verify(buf []byte, fileName, allowedTypesStr string) error {
	allowedTypesStr = strings.ReplaceAll(allowedTypesStr, "|", ",") // compat for old config format

	allowedTypes := []string{}
	for _, entry := range strings.Split(allowedTypesStr, ",") {
		entry = strings.ToLower(strings.TrimSpace(entry))
		if entry != "" {
			allowedTypes = append(allowedTypes, entry)
		}
	}

	if len(allowedTypes) == 0 {
		return nil // everything is allowed
	}

	fullMimeType := http.DetectContentType(buf)
	mimeType, _, err := mime.ParseMediaType(fullMimeType)
	if err != nil {
		log.Warn("Detected attachment type could not be parsed %s", fullMimeType)
		return ErrFileTypeForbidden{Type: fullMimeType}
	}
	extension := strings.ToLower(path.Ext(fileName))
	isBufEmpty := len(buf) <= 1

	// Check both file extension and MIME type
	extensionAllowed := false
	mimeTypeAllowed := false

	for _, allowEntry := range allowedTypes {
		if allowEntry == "*/*" {
			extensionAllowed = true
			mimeTypeAllowed = true
			break
		}
		if strings.HasPrefix(allowEntry, ".") && allowEntry == extension {
			extensionAllowed = true
		}
		if isBufEmpty {
			continue // skip mime type checks if buffer is empty
		}
		if mimeType == allowEntry {
			mimeTypeAllowed = true
		}
		if wildcardTypeRe.MatchString(allowEntry) && strings.HasPrefix(mimeType, allowEntry[:len(allowEntry)-1]) {
			mimeTypeAllowed = true
		}
	}

	// Require both extension and MIME type to be allowed
	if extensionAllowed && (isBufEmpty || mimeTypeAllowed) {
		return nil
	}

	if !isBufEmpty {
		log.Info("Attachment with type %s blocked from upload", fullMimeType)
	}

	return ErrFileTypeForbidden{Type: fullMimeType}
}

.
.
.
please have a look into this implementation , let me hear your thoughts on this

thanks :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's acceptable to not allow uploading empty file in reality.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then it above fn will have a simple addon

// Disallow uploading empty files
	if len(buf) == 0 {
		return ErrFileTypeForbidden{Type: "empty file"}
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's acceptable to not allow uploading empty file in reality.

Yes, I believe it makes sense to disallow empty attachments.

Then again, as I look at my example above, the logic in Verify() is interesting seeing that if you have a potential attachment with the following:

  • A filename that uses an extension that is not allowed
  • A mime type that is allowed
  • (or vice-versa)

... this would pass the verification checks.

My current problem is I am not sure if we should follow the way Verify() works when users try to edit attachment names. Since we don't seem to store the MIME types of attachments when they are uploaded, it doesn't seem possible currently. The current snapshot just checks the file extension when users try to do an edit.

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}
}

Expand Down
9 changes: 9 additions & 0 deletions templates/swagger/v1_json.tmpl
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

23 changes: 22 additions & 1 deletion tests/integration/api_comment_attachment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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})
Expand All @@ -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)()

Expand Down
22 changes: 21 additions & 1 deletion tests/integration/api_issue_attachment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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})
Expand All @@ -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)()

Expand Down
40 changes: 40 additions & 0 deletions tests/integration/api_releases_attachment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)
}
Loading