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

[WIP] Refactor attachement security #7956

Closed
wants to merge 24 commits into from
Closed
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
10 changes: 10 additions & 0 deletions models/attachment.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ func (a *Attachment) IncreaseDownloadCount() error {
return nil
}

// IsNotAttached define is the attachement is linked to an issue or release
func (a *Attachment) IsNotAttached() bool {
return a.ReleaseID == 0 && a.IssueID == 0
}

// APIFormat converts models.Attachment to api.Attachment
func (a *Attachment) APIFormat() *api.Attachment {
return &api.Attachment{
Expand Down Expand Up @@ -148,6 +153,11 @@ func GetAttachmentByUUID(uuid string) (*Attachment, error) {
return getAttachmentByUUID(x, uuid)
}

// GetAttachmentsByUUIDs returns attachment by given UUID list.
func GetAttachmentsByUUIDs(uuids []string) ([]*Attachment, error) {
return getAttachmentsByUUIDs(x, uuids)
}

// GetAttachmentByReleaseIDFileName returns attachment by given releaseId and fileName.
func GetAttachmentByReleaseIDFileName(releaseID int64, fileName string) (*Attachment, error) {
return getAttachmentByReleaseIDFileName(x, releaseID, fileName)
Expand Down
4 changes: 4 additions & 0 deletions models/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -1171,6 +1171,10 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) {
}

for i := 0; i < len(attachments); i++ {
if !attachments[i].IsNotAttached() {
sapk marked this conversation as resolved.
Show resolved Hide resolved
log.Error("newIssue [%s]: skipping already linked attachement", attachments[i].UUID)
continue
}
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]: %v", attachments[i].ID, err)
Expand Down
17 changes: 7 additions & 10 deletions models/issue_comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -606,19 +606,16 @@ func sendCreateCommentAction(e *xorm.Session, opts *CreateCommentOptions, commen
}

// Check attachments
attachments := make([]*Attachment, 0, len(opts.Attachments))
for _, uuid := range opts.Attachments {
attach, err := getAttachmentByUUID(e, uuid)
if err != nil {
if IsErrAttachmentNotExist(err) {
continue
}
return fmt.Errorf("getAttachmentByUUID [%s]: %v", uuid, err)
}
attachments = append(attachments, attach)
attachments, err := getAttachmentsByUUIDs(e, opts.Attachments)
if err != nil {
return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %v", opts.Attachments, err)
}

for i := range attachments {
if !attachments[i].IsNotAttached() {
log.Error("sendCreateCommentAction [%s]: skipping already linked attachement", attachments[i].UUID)
continue
}
attachments[i].IssueID = opts.Issue.ID
attachments[i].CommentID = comment.ID
// No assign value could be 0, so ignore AllCols().
Expand Down
23 changes: 10 additions & 13 deletions models/release.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,21 +148,18 @@ func createTag(gitRepo *git.Repository, rel *Release) error {
return nil
}

func addReleaseAttachments(releaseID int64, attachmentUUIDs []string) (err error) {
func linkReleaseAttachments(releaseID int64, attachmentUUIDs []string) (err error) {
// Check attachments
var attachments = make([]*Attachment, 0)
for _, uuid := range attachmentUUIDs {
attach, err := getAttachmentByUUID(x, uuid)
if err != nil {
if IsErrAttachmentNotExist(err) {
continue
}
return fmt.Errorf("getAttachmentByUUID [%s]: %v", uuid, err)
}
attachments = append(attachments, attach)
attachments, err := GetAttachmentsByUUIDs(attachmentUUIDs)
if err != nil {
return fmt.Errorf("GetAttachmentsByUUIDs [uuids: %v]: %v", attachmentUUIDs, err)
}

for i := range attachments {
if !attachments[i].IsNotAttached() {
log.Error("linkReleaseAttachments [%s]: skipping already linked attachement", attachments[i].UUID)
continue
}
attachments[i].ReleaseID = releaseID
// No assign value could be 0, so ignore AllCols().
if _, err = x.ID(attachments[i].ID).Update(attachments[i]); err != nil {
Expand Down Expand Up @@ -192,7 +189,7 @@ func CreateRelease(gitRepo *git.Repository, rel *Release, attachmentUUIDs []stri
return err
}

err = addReleaseAttachments(rel.ID, attachmentUUIDs)
err = linkReleaseAttachments(rel.ID, attachmentUUIDs)
if err != nil {
return err
}
Expand Down Expand Up @@ -402,7 +399,7 @@ func UpdateRelease(doer *User, gitRepo *git.Repository, rel *Release, attachment
return err
}

err = addReleaseAttachments(rel.ID, attachmentUUIDs)
err = linkReleaseAttachments(rel.ID, attachmentUUIDs)

mode, _ := AccessLevel(doer, rel.Repo)
if err1 := PrepareWebhooks(rel.Repo, HookEventRelease, &api.ReleasePayload{
Expand Down
5 changes: 3 additions & 2 deletions public/js/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -351,8 +351,9 @@ function uploadFile(file, callback) {
callback(xhr.responseText);
}
};

xhr.open("post", suburl + "/attachments", true);
const contextPath = window.location.pathname.substr(suburl.length).replace(/^\/+$/g, '');
const contextPathArray = contextPath.split('/');
xhr.open("post", suburl + "/" + contextPathArray[0] + "/" + contextPathArray[1] + "/attachments", true);
xhr.setRequestHeader("X-Csrf-Token", csrf);
const formData = new FormData();
formData.append('file', file, file.name);
Expand Down
41 changes: 37 additions & 4 deletions routers/routes/routes.go
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,7 @@ func RegisterRoutes(m *macaron.Macaron) {
m.Get("/following", user.Following)
})

//Keeping this path to have backward compat
m.Get("/attachments/:uuid", func(ctx *context.Context) {
attach, err := models.GetAttachmentByUUID(ctx.Params(":uuid"))
if err != nil {
Expand All @@ -489,6 +490,38 @@ func RegisterRoutes(m *macaron.Macaron) {
return
}

//Attachement without issue or release attached should not be returned
if attach.IsNotAttached() {
sapk marked this conversation as resolved.
Show resolved Hide resolved
ctx.Error(404)
return
}
//Check issue access
if attach.IssueID != 0 {
iss, err := models.GetIssueByID(attach.IssueID)
if err != nil {
ctx.ServerError("GetAttachmentByUUID.GetIssueByID", err)
return
}
if !iss.Repo.CheckUnitUser(ctx.User.ID, ctx.User.IsAdmin, models.UnitTypeIssues) {
ctx.Error(403)
return
}
}

//Check release access
if attach.ReleaseID != 0 {
rel, err := models.GetReleaseByID(attach.ReleaseID)
if err != nil {
ctx.ServerError("GetAttachmentByUUID.GetReleaseByID", err)
return
}
if !rel.Repo.CheckUnitUser(ctx.User.ID, ctx.User.IsAdmin, models.UnitTypeReleases) {
ctx.Error(403)
return
}
}

//If we have matched a access release or issue
fr, err := os.Open(attach.LocalPath())
if err != nil {
ctx.ServerError("Open", err)
Expand All @@ -508,10 +541,6 @@ func RegisterRoutes(m *macaron.Macaron) {
})
}, ignSignIn)

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

m.Group("/:username", func() {
m.Get("/action/:action", user.Action)
}, reqSignIn)
Expand Down Expand Up @@ -679,6 +708,10 @@ func RegisterRoutes(m *macaron.Macaron) {
m.Combo("/new").Get(context.RepoRef(), repo.NewIssue).
Post(bindIgnErr(auth.CreateIssueForm{}), repo.NewIssuePost)
}, context.RepoMustNotBeArchived(), reqRepoIssueReader)

//Should be able to create issue (a user that can create release can create issue)
m.Post("/attachments", repo.UploadAttachment, context.RepoMustNotBeArchived(), reqRepoIssueReader)

// FIXME: should use different URLs but mostly same logic for comments of issue and pull reuqest.
// So they can apply their own enable/disable logic on routers.
m.Group("/issues", func() {
Expand Down