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

Jump to bottom

Include file extension checks in attachment API #32151

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+108 −17
Open

Include file extension checks in attachment API #32151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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"}
	}

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
kemzeb marked this conversation as resolved.
Show resolved Hide resolved
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{RepoID: 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
Loading