peezy-tech/jojo
2
0
Fork 0
mirror of https://codeberg.org/forgejo/forgejo.git synced 2026-05-12 22:10:25 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

fix: check that attachments belong to correct resource

Browse source 
It was possible to hijack attachments during update and create functions
to another owner as permissions to check they weren't already attached
to another resource and wasn't checked if it belonged to the repository
that was being operated on.
This commit is contained in:
Gusted 2026-02-11 08:16:51 +01:00 • committed by Mathieu Fenniak
parent 68b2930caa
commit ce0a376723
7 changed files with 93 additions and 79 deletions
Download patch file Download diff file Expand all files Collapse all files

29
models/issues/comment.go
View file

@ -611,9 +611,13 @@ func (c *Comment) UpdateAttachments(ctx context.Context, uuids []string) error {
}
defer committer.Close()
attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, uuids)
if err := c.LoadIssue(ctx); err != nil {
return fmt.Errorf("LoadIssue: %w", err)
}
attachments, err := repo_model.FindRepoAttachmentsByUUID(ctx, c.Issue.RepoID, uuids, repo_model.FindAttachmentOptions{})
if err != nil {
return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", uuids, err)
return fmt.Errorf("FindRepoAttachmentsByUUID[uuids=%q,repoID=%d]: %w", uuids, c.Issue.RepoID, err)
}
for i := 0; i < len(attachments); i++ {
attachments[i].IssueID = c.IssueID
@ -889,7 +893,7 @@ func updateCommentInfos(ctx context.Context, opts *CreateCommentOptions, comment
// Check comment type.
switch opts.Type {
case CommentTypeCode:
if err = updateAttachments(ctx, opts, comment); err != nil {
if err := comment.UpdateAttachments(ctx, opts.Attachments); err != nil {
return err
}
if comment.ReviewID != 0 {
@ -909,7 +913,7 @@ func updateCommentInfos(ctx context.Context, opts *CreateCommentOptions, comment
}
fallthrough
case CommentTypeReview:
if err = updateAttachments(ctx, opts, comment); err != nil {
if err := comment.UpdateAttachments(ctx, opts.Attachments); err != nil {
return err
}
case CommentTypeReopen, CommentTypeClose:
@ -921,23 +925,6 @@ func updateCommentInfos(ctx context.Context, opts *CreateCommentOptions, comment
return UpdateIssueCols(ctx, opts.Issue, "updated_unix")
}
func updateAttachments(ctx context.Context, opts *CreateCommentOptions, comment *Comment) error {
attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, opts.Attachments)
if err != nil {
return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", opts.Attachments, err)
}
for i := range attachments {
attachments[i].IssueID = opts.Issue.ID
attachments[i].CommentID = comment.ID
// No assign value could be 0, so ignore AllCols().
if _, err = db.GetEngine(ctx).ID(attachments[i].ID).Update(attachments[i]); err != nil {
return fmt.Errorf("update attachment [%d]: %w", attachments[i].ID, err)
}
}
comment.Attachments = attachments
return nil
}
func createDeadlineComment(ctx context.Context, doer *user_model.User, issue *Issue, newDeadlineUnix timeutil.TimeStamp) (*Comment, error) {
var content string
var commentType CommentType

22
models/issues/issue_update.go
View file

@ -234,18 +234,18 @@ func AddDeletePRBranchComment(ctx context.Context, doer *user_model.User, repo *
}
// UpdateIssueAttachments update attachments by UUIDs for the issue
func UpdateIssueAttachments(ctx context.Context, issueID int64, uuids []string) (err error) {
func UpdateIssueAttachments(ctx context.Context, issue *Issue, uuids []string) (err error) {
ctx, committer, err := db.TxContext(ctx)
if err != nil {
return err
}
defer committer.Close()
attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, uuids)
attachments, err := repo_model.FindRepoAttachmentsByUUID(ctx, issue.RepoID, uuids, repo_model.FindAttachmentOptions{})
if err != nil {
return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", uuids, err)
return fmt.Errorf("FindRepoAttachmentsByUUID[uuids=%q,repoID=%d]: %w", uuids, issue.RepoID, err)
}
for i := 0; i < len(attachments); i++ {
attachments[i].IssueID = issueID
attachments[i].IssueID = issue.ID
if err := repo_model.UpdateAttachment(ctx, attachments[i]); err != nil {
return fmt.Errorf("update attachment [id: %d]: %w", attachments[i].ID, err)
}
@ -394,18 +394,8 @@ func NewIssueWithIndex(ctx context.Context, doer *user_model.User, opts NewIssue
return err
}
if len(opts.Attachments) > 0 {
attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, opts.Attachments)
if err != nil {
return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", opts.Attachments, err)
}
for i := 0; i < len(attachments); i++ {
attachments[i].IssueID = opts.Issue.ID
if _, err = e.ID(attachments[i].ID).Update(attachments[i]); err != nil {
return fmt.Errorf("update attachment [id: %d]: %w", attachments[i].ID, err)
}
}
if err := UpdateIssueAttachments(ctx, opts.Issue, opts.Attachments); err != nil {
return fmt.Errorf("UpdateIssueAttachments: %w", err)
}
if err = opts.Issue.LoadAttributes(ctx); err != nil {
return err

64
models/repo/attachment.go
View file

@ -1,4 +1,5 @@
// Copyright 2017 The Gitea Authors. All rights reserved.
// Copyright 2026 The Forgejo Authors. All rights reserved.
// SPDX-License-Identifier: MIT
package repo
@ -16,17 +17,30 @@ import (
"forgejo.org/modules/timeutil"
"forgejo.org/modules/util"
"forgejo.org/modules/validation"
"xorm.io/builder"
)
// Attachment represent a attachment of issue/comment/release.
type Attachment struct {
ID int64 `xorm:"pk autoincr"`
UUID string `xorm:"uuid UNIQUE"`
RepoID int64 `xorm:"INDEX"` // this should not be zero
IssueID int64 `xorm:"INDEX"` // maybe zero when creating
ReleaseID int64 `xorm:"INDEX"` // maybe zero when creating
UploaderID int64 `xorm:"INDEX DEFAULT 0"` // Notice: will be zero before this column added
CommentID int64 `xorm:"INDEX"`
ID int64 `xorm:"pk autoincr"`
// UUID is the public identifier of the attachment, and is used during HTTP
// requests to refer to a specific attachment.
UUID string `xorm:"uuid UNIQUE"`
// UploaderID is always set and non-zero and refers to the user that has
// uploaded this attachment.
UploaderID int64 `xorm:"INDEX DEFAULT 0"`
// RepoID is always set and non-zero and refers to the repository where this
// attachment was uploaded to.
RepoID int64 `xorm:"INDEX"`
// IssueID, ReleaseID and CommentID have multiple possible states:
// - ReleaseID != 0 && IssueID == 0 && CommentID == 0: attached to release with id `ReleaseID`.
// - ReleaseID == 0 && IssueID != 0 && CommentID == 0: attached to the issue with id `IssueID`.
// - ReleaseID == 0 && IssueID != 0 && CommentID != 0: attached to comment with id `CommentID` that is in issue with id `IssueID`.
// All other states should be considered invalid.
IssueID int64 `xorm:"INDEX"`
ReleaseID int64 `xorm:"INDEX"`
CommentID int64 `xorm:"INDEX"`
Name string
DownloadCount int64 `xorm:"DEFAULT 0"`
Size int64 `xorm:"DEFAULT 0"`
@ -73,6 +87,12 @@ func (a *Attachment) DownloadURL() string {
return setting.AppURL + "attachments/" + url.PathEscape(a.UUID)
}
// IsAttachedToResource returns true if this attachment is attached to a release,
// issue or comment.
func (a *Attachment) IsAttachedToResource() bool {
return a.ReleaseID != 0 || a.IssueID != 0 || a.CommentID != 0
}
// ErrAttachmentNotExist represents a "AttachmentNotExist" kind of error.
type ErrAttachmentNotExist struct {
ID int64
@ -133,15 +153,37 @@ func GetAttachmentByUUID(ctx context.Context, uuid string) (*Attachment, error)
return attach, nil
}
// GetAttachmentsByUUIDs returns attachment by given UUID list.
func GetAttachmentsByUUIDs(ctx context.Context, uuids []string) ([]*Attachment, error) {
type FindAttachmentOptions struct {
ReleaseID int64
IssueID int64
CommentID int64
}
func (opts FindAttachmentOptions) ToConds() builder.Cond {
return builder.Eq{"release_id": opts.ReleaseID, "issue_id": opts.IssueID, "comment_id": opts.CommentID}
}
// FindRepoAttachmentsByUUID always returns attachment that has a UUID that is
// in the given `uuids` argument and is attached to the repository.
//
// The values in `opts` are always as a condition even if they are zero, this
// allows to search for attachments that are not yet attached to any resource by
// specifying a empty struct.
func FindRepoAttachmentsByUUID(ctx context.Context, repoID int64, uuids []string, opts FindAttachmentOptions) ([]*Attachment, error) {
// Nothing to match anyway.
if len(uuids) == 0 {
return []*Attachment{}, nil
}
// Silently drop invalid uuids.
// At maximum nothing is filtered and we get all attachments via the UUID.
attachments := make([]*Attachment, 0, len(uuids))
return attachments, db.GetEngine(ctx).In("uuid", uuids).Find(&attachments)
err := db.GetEngine(ctx).
Where("repo_id = ?", repoID).
In("uuid", uuids).
And(opts.ToConds()).
Find(&attachments)
return attachments, err
}
// ExistAttachmentsByUUID returns true if attachment exists with the given UUID

12
models/repo/release.go
View file

@ -224,18 +224,14 @@ func UpdateRelease(ctx context.Context, rel *Release) error {
}
// AddReleaseAttachments adds a release attachments
func AddReleaseAttachments(ctx context.Context, releaseID int64, attachmentUUIDs []string) (err error) {
// Check attachments
attachments, err := GetAttachmentsByUUIDs(ctx, attachmentUUIDs)
func AddReleaseAttachments(ctx context.Context, release *Release, attachmentUUIDs []string) (err error) {
attachments, err := FindRepoAttachmentsByUUID(ctx, release.RepoID, attachmentUUIDs, FindAttachmentOptions{})
if err != nil {
return fmt.Errorf("GetAttachmentsByUUIDs [uuids: %v]: %w", attachmentUUIDs, err)
return fmt.Errorf("FindRepoAttachmentsByUUID[uuids=%q,repoID=%d]: %w", attachmentUUIDs, release.RepoID, err)
}
for i := range attachments {
if attachments[i].ReleaseID != 0 {
return util.NewPermissionDeniedErrorf("release permission denied")
}
attachments[i].ReleaseID = releaseID
attachments[i].ReleaseID = release.ID
// No assign value could be 0, so ignore AllCols().
if _, err = db.GetEngine(ctx).ID(attachments[i].ID).Update(attachments[i]); err != nil {
return fmt.Errorf("update attachment [%d]: %w", attachments[i].ID, err)

2
routers/web/repo/issue.go
View file

@ -3682,7 +3682,7 @@ func updateAttachments(ctx *context.Context, item any, files []string) error {
if len(files) > 0 {
switch content := item.(type) {
case *issues_model.Issue:
err = issues_model.UpdateIssueAttachments(ctx, content.ID, files)
err = issues_model.UpdateIssueAttachments(ctx, content, files)
case *issues_model.Comment:
err = content.UpdateAttachments(ctx, files)
default:

6
services/attachment/attachment.go
View file

@ -24,6 +24,9 @@ func NewAttachment(ctx context.Context, attach *repo_model.Attachment, file io.R
if attach.RepoID == 0 {
return nil, fmt.Errorf("attachment %s should belong to a repository", attach.Name)
}
if attach.UploaderID == 0 {
return nil, fmt.Errorf("attachment %s should have a uploader", attach.Name)
}
err := db.WithTx(ctx, func(ctx context.Context) error {
attach.UUID = uuid.New().String()
@ -48,6 +51,9 @@ func NewExternalAttachment(ctx context.Context, attach *repo_model.Attachment) (
if attach.RepoID == 0 {
return nil, fmt.Errorf("attachment %s should belong to a repository", attach.Name)
}
if attach.UploaderID == 0 {
return nil, fmt.Errorf("attachment %s should have a uploader", attach.Name)
}
if attach.ExternalURL == "" {
return nil, fmt.Errorf("attachment %s should have a external url", attach.Name)
}

37
services/release/release.go
View file

@ -192,7 +192,7 @@ func CreateRelease(gitRepo *git.Repository, rel *repo_model.Release, msg string,
}
}
if err = repo_model.AddReleaseAttachments(gitRepo.Ctx, rel.ID, addAttachmentUUIDs.Values()); err != nil {
if err = repo_model.AddReleaseAttachments(gitRepo.Ctx, rel, addAttachmentUUIDs.Values()); err != nil {
return err
}
@ -314,44 +314,37 @@ func UpdateRelease(ctx context.Context, doer *user_model.User, gitRepo *git.Repo
}
}
if err = repo_model.AddReleaseAttachments(ctx, rel.ID, addAttachmentUUIDs.Values()); err != nil {
if err = repo_model.AddReleaseAttachments(ctx, rel, addAttachmentUUIDs.Values()); err != nil {
return fmt.Errorf("AddReleaseAttachments: %w", err)
}
deletedUUIDs := make(container.Set[string])
if len(delAttachmentUUIDs) > 0 {
// Check attachments
attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, delAttachmentUUIDs.Values())
// Check delAttachments
delAttachments, err := repo_model.FindRepoAttachmentsByUUID(ctx, rel.RepoID, delAttachmentUUIDs.Values(), repo_model.FindAttachmentOptions{ReleaseID: rel.ID})
if err != nil {
return fmt.Errorf("GetAttachmentsByUUIDs [uuids: %v]: %w", delAttachmentUUIDs, err)
return fmt.Errorf("FindRepoAttachmentsByUUID[uuids=%q,repoID=%d,releaseID=%d]: %w", delAttachmentUUIDs.Values(), rel.RepoID, rel.ID, err)
}
for _, attach := range attachments {
if attach.ReleaseID != rel.ID {
return util.SilentWrap{
Message: "delete attachment of release permission denied",
Err: util.ErrPermissionDenied,
}
}
for _, attach := range delAttachments {
deletedUUIDs.Add(attach.UUID)
}
if _, err := repo_model.DeleteAttachments(ctx, attachments, true); err != nil {
if _, err := repo_model.DeleteAttachments(ctx, delAttachments, true); err != nil {
return fmt.Errorf("DeleteAttachments [uuids: %v]: %w", delAttachmentUUIDs, err)
}
}
if len(updateAttachmentUUIDs) > 0 {
// Check attachments
attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, updateAttachmentUUIDs.Values())
// Check that attachments actually belong to repository and release.
attachments, err := repo_model.FindRepoAttachmentsByUUID(ctx, rel.RepoID, updateAttachmentUUIDs.Values(), repo_model.FindAttachmentOptions{ReleaseID: rel.ID})
if err != nil {
return fmt.Errorf("GetAttachmentsByUUIDs [uuids: %v]: %w", updateAttachmentUUIDs, err)
return fmt.Errorf("FindRepoAttachmentsByUUID[uuids=%q,repoID=%d,releaseID=%d]: %w", updateAttachmentUUIDs.Values(), rel.RepoID, rel.ID, err)
}
for _, attach := range attachments {
if attach.ReleaseID != rel.ID {
return util.SilentWrap{
Message: "update attachment of release permission denied",
Err: util.ErrPermissionDenied,
}
if len(attachments) != len(updateAttachments) {
return util.SilentWrap{
Message: "update attachment of release permission denied",
Err: util.ErrPermissionDenied,
}
}
}