Enable Uploading/Removing Attachments When Editing an Issue/Comment

[blueworrybear]

This pull request is to fix issue #3618 .
The update will allow user editing attachments with dropzone UI.
When user start editing issue or comment, the existed attachments will be added to dropzone below text area. Only when user submit his/her content the attachments will be uploaded/removed.

I also add a new template for showing attachments, which could update attachments in issue page without refreshing.

[codecov-io]

Codecov Report

Merging #8426 into master will decrease coverage by 0.07%.
The diff coverage is 16.21%.

@@            Coverage Diff             @@
##           master    #8426      +/-   ##
==========================================
- Coverage   41.72%   41.65%   -0.08%     
==========================================
  Files         514      514              
  Lines       67028    67172     +144     
==========================================
+ Hits        27966    27979      +13     
- Misses      35469    35593     +124     
- Partials     3593     3600       +7

Impacted Files	Coverage Δ
modules/util/compare.go	46.15% <0%> (-8.4%)	⬇️
models/issue_comment.go	48.26% <0%> (-1.28%)	⬇️
models/issue.go	49.88% <0%> (-0.6%)	⬇️
routers/repo/attachment.go	10.16% <0%> (-4.84%)	⬇️
routers/routes/routes.go	82.38% <100%> (+0.06%)	⬆️
routers/repo/issue.go	34.98% <22.35%> (-0.88%)	⬇️
models/unit.go	62.16% <0%> (-5.41%)	⬇️
modules/migrations/migrate.go	21.22% <0%> (-1.68%)	⬇️
modules/migrations/gitea.go	10.25% <0%> (-1.5%)	⬇️
routers/repo/view.go	40% <0%> (-0.96%)	⬇️
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7d348e...f8d15e6. Read the comment docs.

[guillep2k]

Perhaps it's worth having a common function and use it from Comment and Issue to avoid duplicating code? This function is not too short but also not too long, so it sits in-between.

[guillep2k]

Why we get repo attachments for update and delete, but issue/comment attachments for get? I understand that there's a special case for issues/comments under creation, but attachments can also be modified on edit.

By the way, this is a great opportunity to validate user permissions when updating/deleting an attachment associated to an existing issue/comment that they don't have access to. The criteria should be the same as for edit/delete the issue/comment.

Perhaps this is a matter for another PR.

[lafriks]

Yes, right checking for deleting attachment should definitely be done

[blueworrybear]

@guillep2k
I use [issue/comment]/attachments because I could get attachments information without inject information into view_content.tmpl. Otherwise I have to inject attachments name, uuid and size during rendering view_content.tmpl for initializing dropzone when first time edit button click.

However, as you said it's okay to modify attachments on edit. Do you think I should modify my codes? If there is anything I misunderstand, please let me know 😃

@lafriks
I think I could try to enable user permissions checking, but I think it's better to open another pull request.

[guillep2k]

@guillep2k
I use [issue/comment]/attachments because I could get attachments information without inject information into view_content.tmpl. Otherwise I have to inject attachments name, uuid and size during rendering view_content.tmpl for initializing dropzone when first time edit button click.

However, as you said it's okay to modify attachments on edit. Do you think I should modify my codes? If there is anything I misunderstand, please let me know 😃

@lafriks
I think I could try to enable user permissions checking, but I think it's better to open another pull request.

I think that changing the API is not required per se, but it would be needed when access permissions were to be checked, so it can wait for the next PR.

[blueworrybear]

@guillep2k
Ok, I'll finish refactoring UpdateCommentContent and UpdateIssueContent. Then moving on checking access permissions. 👍

[blueworrybear]

Hummm... I see the problem. I underestimate the effort to check attachment permission. No wonder Gitea does not actually delete file on server when removing it from dropzone.

So I guess the possible solution may be either:

1. Not to delete attachment on server actually during editing for short term. Since UpdateIssueContent and UpdateCommentContent will check login user, there is less security concern IMO.
2. Keep working on [WIP] Refactor attachement security #7956 to check attachment's uploader before deleting.

Correct me if I'm wrong. 😕

[guillep2k]

But isn't the same problem present in update as well? I think that only repo access is checked when you know the attachment GUID.

[blueworrybear]

Let me recap:

1. My /attachments/delete will cause security issue because I only check whether user is login with:

m.Group("/attachments", func() {
  m.Post("", repo.UploadAttachment)
  m.Post("/delete", repo.DeleteAttachment)
}, reqSignIn)

The way to fix it is either:

• Remove /attachments/delete route, attachments will not be deleted on server just as usual when creating new issue/comment
• At least check Uploader ID in repo.DeleteAttachment such as

  func DeleteAttachment(ctx *context.Context) {
    file := ctx.Query("file")
    attach, err := models.GetAttachmentByUUID(file)
    if attach.UploaderID != ctx.User.ID {
      ctx.Error(403)
    }
    ...
  }

  But I see there is a comment in:

  type Attachment struct {
    ID            int64  `xorm:"pk autoincr"`
    UUID          string `xorm:"uuid UNIQUE"`
    IssueID       int64  `xorm:"INDEX"`
    ReleaseID     int64  `xorm:"INDEX"`
    UploaderID    int64  `xorm:"INDEX DEFAULT 0"` // Notice: will be zero before this column added
    CommentID     int64
    Name          string
    DownloadCount int64              `xorm:"DEFAULT 0"`
    Size          int64              `xorm:"DEFAULT 0"`
    CreatedUnix   timeutil.TimeStamp `xorm:"created"`
  }

  So maybe UploaderID is not available yet?

  After trying, it seems like the UploaderID is not correctly set yet (using sqlite)

2. Issue.UpdateAttachments may also casue security issue(?)
   It seems to me that I use it only inside repo.UpdateIssueContent and
   user permission will be checked in the begin of repo.UpdateIssueContent. So
   maybe it's okay?
3. Other attachments' security issue requires finishing [WIP] Refactor attachement security #7956 to solve

Correct me if I'm wrong. Thank you~

[guillep2k]

You are correct 😉
The same problems delete introduces are already present with update. What can happen is the following:

User bob creates an issue with attachments abcd and efgh. Alice is a little mad with bob (reasons unknown), so she decides to play him a joke. She's skilled in Javascript, so she opens the issue, starts a new comment and POST content into abcd. Now the attachment is modified. 😢. The same can happen with the new delete option, as it copies the previous behavior.

Back to your options: I think we should decide whether solving this is a matter of this PR or another. Considering that update was already affected, perhaps we can treat this as a different problem. I forgot about @sapk PR. It's reasonable to expect this to be resolved there.

@lafriks What do you think? @sapk?

[blueworrybear]

After trying, it seems like the UploaderID is not correctly set yet (using sqlite)

I found I'm wrong, UploaderID is correctly set (using sqlite). It turns out it's my codes in index.js is wrong.

$.post($dropzone.data('remove-url'), {
    file: file.uuid,
    _csrf: $dropzone.data('csrf')
});

I used file.uuid for POST request, but it will not be set. filenameDict[file.name].uuid should be used instead.

So I think checking UploaderID before delete attachment could be a short-term workaround.
I'll update my codes later. 😓

[guillep2k]

Under some particular operations I've found that the upload area does not render properly:

What I I did was:

• Create an issue.
• Add a comment; add one attachment with a permitted extension (.pdf).
• Edit the attachment.
• Attempt uploading a file that is not allowed as attachment (e.g. .exe).
• Cancel de edit.
• Click edit again.

If I cancel again and click on edit, or if I reload the page, it renders properly. I think I can reproduce consistently.

[blueworrybear]

Under some particular operations I've found that the upload area does not render properly:

@guillep2k
Thank you for testing. I figure out the problem now.

this.on("removedfile", function (file) {
    if (file.name in filenameDict) {
        $('#' + filenameDict[file.name].uuid).remove();
    }
    if ($dropzone.data('remove-url') && $dropzone.data('csrf') && !filenameDict[file.name].submitted) {
        $.post($dropzone.data('remove-url'), {
            file: file.uuid,
            _csrf: $dropzone.data('csrf')
        });
    }
});

Invalid file upload will not trigger dropzone success event, and then the file won't be in filenameDict. It causes !filenameDict[file.name].submitted fail.

I'll push again later. Thanks again 😄

[lafriks]

Right checking could probably be improved to also allow repo admins to delete them not only uploader but for first iteration this is ok imho