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.

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

Private attachments #23071

Open
delvh opened this issue Feb 22, 2023 · 8 comments
Open

Private attachments #23071

delvh opened this issue Feb 22, 2023 · 8 comments
Labels
type/enhancement An improvement of existing functionality type/proposal The new feature has not been accepted yet but needs to be discussed first.

Comments

@delvh
Copy link
Member

delvh commented Feb 22, 2023

Feature Description

At the moment, any attachment you upload on a gitea instance will be publicly available,
i.e. https://try.gitea.io/attachments/a35cb41a-1afe-4415-bb8c-6058e29e9e21.

This is not always a good idea, as sometimes attachments are files that should be hidden from the public, i.e. personal information or security concerns.
This is especially a requirement for private issues, as otherwise no attachments can be safely shared.

Proposal

In addition to the existing upload mechanism that uploads to /attachments/<UUID>, we should add a second mechanism that returns 404 if the user is not allowed to read this attachment.
I can think of two possible implementations for the backend:

  1. Add a separate path structure /attachments/private/<context>/UUID to store private attachments, where <context> can be for example <user>/<repo>/<comment-id>
  2. Add a proxy before returning the attachment that stores in the db the necessary information for private attachments, i.e.
type AttachmentInfo struct {
  ID int64
  AttachmentID string `xorm:"UNIQUE"`
  RepoID int64
  PosterID int64

Then, if no attachment info exists, the attachment is public, and otherwise the user must be the poster of this attachment, or have (at least) read access to issues (and PRs) on this repo.
The edge case attachmentInfo exists (=> attachment is private) && user is not logged in should probably still result in not showing the attachment, as a private attachment should always mean "only logged in users can see it", otherwise it could also be public.
The problem with this feature are especially two points:

  1. What UI would result in a good user experience for this?
  2. How to avoid performance problems? Every query for an attachment now needs a database query…
@delvh delvh added type/proposal The new feature has not been accepted yet but needs to be discussed first. type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels Feb 22, 2023
@KN4CK3R
Copy link
Member

KN4CK3R commented Feb 23, 2023

Why not use the existing attachment model? Could simply add one more check in repo.GetAttachment then.

@wxiaoguang
Copy link
Contributor

Just FYI, according to my test, GitHub's issue attachments are also open to public, even in a private repo. I just pasted an image into a private issue: https://user-images.githubusercontent.com/2114189/220891059-1d6e99c6-f906-4e21-9365-0e548ca67a18.png

@lunny
Copy link
Member

lunny commented May 31, 2023

I think this has already been implemented by #9340 ?

@lunny lunny added issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail and removed type/proposal The new feature has not been accepted yet but needs to be discussed first. type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels May 31, 2023
@delvh
Copy link
Member Author

delvh commented Jun 2, 2023

Partly.
As I've been able to confirm now for completely private repos, attachments won't be shown if the user has no access to them.
However, this issue talks about private issues in a public repo in particular.
You cannot simulate private issues with a private repo as then only people you've granted access can report something that should be kept private.
That's typically not how security sensitive issues work.

@lunny
Copy link
Member

lunny commented Jun 2, 2023

Then this should be a part of #3217 ?

@delvh
Copy link
Member Author

delvh commented Jun 2, 2023

Well… Yesn't.

This is especially a requirement for private issues, as otherwise no attachments can be safely shared.

I see it as a requirement for implementing private issues.
While it could be implemented in the same PR, I recommend a separate PR to keep the diffs manageable.
That's the whole point why I opened this issue, so that this one gets implemented first.

@GiteaBot
Copy link
Collaborator

GiteaBot commented Sep 8, 2023

We close issues that need feedback from the author if there were no new comments for a month. 🍵

@GiteaBot GiteaBot closed this as completed Sep 8, 2023
@KN4CK3R
Copy link
Member

KN4CK3R commented Sep 8, 2023

I think this should remain open.

@jolheiser jolheiser reopened this Sep 8, 2023
@jolheiser jolheiser added type/proposal The new feature has not been accepted yet but needs to be discussed first. type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/enhancement An improvement of existing functionality and removed issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement An improvement of existing functionality type/proposal The new feature has not been accepted yet but needs to be discussed first.
Projects
None yet
Development

No branches or pull requests

6 participants