[WIP] Refactor attachement security

[sapk]

First, keep in mind that this PR is not perfect to be backward compatible (at least for viewing) but improve the current situation.

To be readable an attachment:

• Need to be linked to an Issue or a Release (will hide any previously uploaded anonymously since not linked later)
• The user need to have read access to the repo linked to issue or release

To be uploaded and later attached to an Issue or Release:

• The attachment need to not be linked by an other issue or release. (You can not rebound a uploaded attachment from an other repo to a repo you own to be able to read it later).
• You need to have read issue access to the current repo to upload an attachment. (Disable previous anonymous upload)

Way this PR could be improved later:

• Add and set at upload a repoid fields and check it when linking the attachment to an issue or release. This could improve the read access maybe
• Move the GET attachment under repo to use the access rule of the repo
• Add an admin action to clear old none linked attachement (file uploaded but not linked to an issue or attachment (not submitted).

TODO:

• Test it thoroughly manually
• Add tests

Related issue: #7908 #4721
Related PR: #7909 (on an other aspect but is compatible)

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@8ff6067). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #7956   +/-   ##
=========================================
  Coverage          ?   41.49%           
=========================================
  Files             ?      479           
  Lines             ?    63998           
  Branches          ?        0           
=========================================
  Hits              ?    26553           
  Misses            ?    33988           
  Partials          ?     3457

---

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 8ff6067...c05178a. Read the comment docs.

[guillep2k]

I'm always in favor of better security.

[lafriks]

How would you upload attachment/image when creating new issue?

[sapk]

@lafriks from userpoint, the same way. I just check that you can open an issue to the current repo to be able to upload. This is mostly to limit anonymous upload. Currently someone could use gitea as a storage to distribute file without even needing an account.

[lafriks]

Yes but when uploading attachment you are uploading it not attached to anything (there is no issue yet) so you have no rights to check for

[sapk]

@lafriks the post request is under repo path (and verification) now. if not linked later it will return 404 when get. The link is done later like previously when submitting the issue or release. I hesitate to add a repo id set at upload and check it at linking. It will add a little more flow check but isn't rely needed since it is the linking that allow the attachement to be available. The main reason is to force someone malicious to create an account to upload an attachement. And also for download I check the acces to the repo linked to issue or release.

[guillep2k]

@lafriks the post request is under repo path (and verification) now. if not linked later it will return 404 when get. The link is done later like previously when submitting the issue or release. I hesitate to add a repo id set at upload and check it at linking. It will add a little more flow check but isn't rely needed since it is the linking that allow the attachement to be available. The main reason is to force someone malicious to create an account to upload an attachement. And also for download I check the acces to the repo linked to issue or release.

I think that's a good start. If the admin limits the access by registration, they should be able to trust Gitea to cover their backs. But that leaves me thinking (outside the scope of this PR): for repositories with anonymous access, a DoS attack is a piece of cake. Flood attacks can be covered by fail2ban (if the proper logs are set), but uploading attachments can deplete the storage space while being invisible to fail2ban. Perhaps in the future some kind of throtling mechanism should be considered.

[sapk]

Yes, that why I indicate that this PR is definitively not the complete fix and that attachment would need more rewrite and breaking changes to be on best practice.

First, keep in mind that this PR is not perfect to be backward compatible (at least for viewing) but improve the current situation.

From my point of view, I see this PR as a frist step to learn how attachements works and lay down some bases (and tests) for next step. Also I try to keep this as non-beaking as possible to backport it.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

[sapk]

@Stale There are still that changes needed to be breakdown from this PR.

[lunny]

@sapk Should be replaced by #9340?

[sapk]

@lunny it still have a part for the upload of file that need to be extracted/improved in a separate PR.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

[stale]

This pull request has been automatically closed because of inactivity. You can re-open it if needed.