[WIP] Refactor attachement security

integrations/attachement_test.go

@@ -0,0 +1,137 @@
+// Copyright 2019 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package integrations
+
+import (
+	"bytes"
+	"image"
+	"image/png"
+	"io"
+	"mime/multipart"
+	"net/http"
+	"testing"
+
+	"code.gitea.io/gitea/modules/test"
+	"github.com/stretchr/testify/assert"
+)
+
+func generateImg() bytes.Buffer {
+	// Generate image
+	myImage := image.NewRGBA(image.Rect(0, 0, 1, 1))
+	var buff bytes.Buffer
+	png.Encode(&buff, myImage)
+	return buff
+}
+
+func createAttachment(t *testing.T, session *TestSession, repoURL, filename string, buff bytes.Buffer, expectedStatus int) string {
+	body := &bytes.Buffer{}
+
+	//Setup multi-part
+	writer := multipart.NewWriter(body)
+	part, err := writer.CreateFormFile("file", filename)
+	assert.NoError(t, err)
+	_, err = io.Copy(part, &buff)
+	assert.NoError(t, err)
+
+	req := NewRequest(t, "GET", repoURL)
+	resp := session.MakeRequest(t, req, http.StatusOK)
+	doc := NewHTMLParser(t, resp.Body)
+	err = writer.WriteField("_csrf", getCsrf(t, doc.doc))
+	assert.NoError(t, err)
+
+	err = writer.Close()
+	assert.NoError(t, err)
+
+	req = NewRequestWithBody(t, "POST", repoURL+"/attachments", body)
+	resp = session.MakeRequest(t, req, expectedStatus)
+
+	if expectedStatus != http.StatusOK {
+		return ""
+	}
+
+	var obj map[string]string
+	DecodeJSON(t, resp, &obj)
+	return obj["uuid"]
+}
+
+func TestCreateAnonymeAttachement(t *testing.T) {
+	prepareTestEnv(t)
+	session := emptyTestSession(t)
+	createAttachment(t, session, "user2/repo1", "image.png", generateImg(), http.StatusFound)
+}
+
+func TestCreateIssueAttachement(t *testing.T) {
+	prepareTestEnv(t)
+	const repoURL = "user2/repo1"
+	session := loginUser(t, "user2")
+	uuid := createAttachment(t, session, repoURL, "image.png", generateImg(), http.StatusOK)
+
+	req := NewRequest(t, "GET", repoURL+"/issues/new")
+	resp := session.MakeRequest(t, req, http.StatusOK)
+	htmlDoc := NewHTMLParser(t, resp.Body)
+
+	link, exists := htmlDoc.doc.Find("form").Attr("action")
+	assert.True(t, exists, "The template has changed")
+
+	postData := map[string]string{
+		"_csrf":    htmlDoc.GetCSRF(),
+		"title":    "New Issue With Attachement",
+		"content":  "some content",
+		"files[0]": uuid,
+	}
+
+	req = NewRequestWithValues(t, "POST", link, postData)
+	resp = session.MakeRequest(t, req, http.StatusFound)
+	test.RedirectURL(resp) // check that redirect URL exists
+
+	//TODO validate
+}
+
+/*
+
+
+func TestCreateDisaledAttachement(t *testing.T) {
+	prepareTestEnv(t)
+	session := loginUser(t, "user2")
+	//setting.AttachmentEnabled
+}
+
+func TestCreateNotAllowedAttachement(t *testing.T) {
+	prepareTestEnv(t)
+	session := loginUser(t, "user2")
+	//setting.AttachmentEnabled
+}
+
+func TestCreateIssueCommentAttachement(t *testing.T) {
+	prepareTestEnv(t)
+	session := loginUser(t, "user2")
+	//TODO upload attachement
+	//TODO create issue comment with attachement
+	//TODO validate
+}
+
+func TestCreateReleaseAttachement(t *testing.T) {
+	prepareTestEnv(t)
+	session := loginUser(t, "user2")
+	//TODO upload attachement
+	//TODO create release with attachement
+	createNewRelease(t, session, "/user2/repo1", "test-attachement", "test-attachement", false, true)
+	//TODO validate
+}
+
+func TestGetUnlinkedAttachement(t *testing.T) {
+	prepareTestEnv(t)
+	session := loginUser(t, "user2")
+	//TODO upload attachement
+	//TODO try to get attachement
+}
+
+func TestGetUnauthorizedAttachement(t *testing.T) {
+	prepareTestEnv(t)
+	session := loginUser(t, "user2")
+	//TODO upload attachement
+	//TODO try to get attachement from an unauthorized user
+}
+*/

integrations/release_test.go

@@ -16,6 +16,7 @@ import (
 )
 
 func createNewRelease(t *testing.T, session *TestSession, repoURL, tag, title string, preRelease, draft bool) {
+	//TODO allow attachement
 	req := NewRequest(t, "GET", repoURL+"/releases/new")
 	resp := session.MakeRequest(t, req, http.StatusOK)
 	htmlDoc := NewHTMLParser(t, resp.Body)
@@ -78,7 +79,7 @@ func TestCreateRelease(t *testing.T) {
 	session := loginUser(t, "user2")
 	createNewRelease(t, session, "/user2/repo1", "v0.0.1", "v0.0.1", false, false)
 
-	checkLatestReleaseAndCount(t, session, "/user2/repo1", "v0.0.1", i18n.Tr("en", "repo.release.stable"), 1)
+	checkLatestReleaseAndCount(t, session, "/user2/repo1", "v0.0.1", i18n.Tr("en", "repo.release.stable"), 2)
 }
 
 func TestCreateReleasePreRelease(t *testing.T) {
@@ -87,7 +88,7 @@ func TestCreateReleasePreRelease(t *testing.T) {
 	session := loginUser(t, "user2")
 	createNewRelease(t, session, "/user2/repo1", "v0.0.1", "v0.0.1", true, false)
 
-	checkLatestReleaseAndCount(t, session, "/user2/repo1", "v0.0.1", i18n.Tr("en", "repo.release.prerelease"), 1)
+	checkLatestReleaseAndCount(t, session, "/user2/repo1", "v0.0.1", i18n.Tr("en", "repo.release.prerelease"), 2)
 }
 
 func TestCreateReleaseDraft(t *testing.T) {
@@ -96,7 +97,7 @@ func TestCreateReleaseDraft(t *testing.T) {
 	session := loginUser(t, "user2")
 	createNewRelease(t, session, "/user2/repo1", "v0.0.1", "v0.0.1", false, true)
 
-	checkLatestReleaseAndCount(t, session, "/user2/repo1", "v0.0.1", i18n.Tr("en", "repo.release.draft"), 1)
+	checkLatestReleaseAndCount(t, session, "/user2/repo1", "v0.0.1", i18n.Tr("en", "repo.release.draft"), 2)
 }
 
 func TestCreateReleasePaging(t *testing.T) {

models/attachment.go

@@ -42,6 +42,11 @@ func (a *Attachment) IncreaseDownloadCount() error {
 	return nil
 }
 
+// IsLinked define is the attachement is linked to an issue or release
+func (a *Attachment) IsLinked() bool {
+	return a.ReleaseID != 0 || a.IssueID != 0
+}
+
 // APIFormat converts models.Attachment to api.Attachment
 func (a *Attachment) APIFormat() *api.Attachment {
 	return &api.Attachment{
@@ -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)

models/attachment_test.go

@@ -116,3 +116,24 @@ func TestUpdateAttachment(t *testing.T) {
 
 	AssertExistsAndLoadBean(t, &Attachment{Name: "new_name"})
 }
+
+func TestAttachment_IsLinked(t *testing.T) {
+	assert.NoError(t, PrepareTestDatabase())
+	tests := []struct {
+		name string
+		UUID string
+		want bool
+	}{
+		{"comment_linked", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a17", true},
+		{"issue_linked", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a18", true},
+		{"release_linked", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a19", true},
+		{"not_linked", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a20", false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			a, err := GetAttachmentByUUID(tt.UUID)
+			assert.NoError(t, err)
+			assert.Equal(t, tt.want, a.IsLinked())
+		})
+	}
+}

models/fixtures/attachment.yml

@@ -69,3 +69,18 @@
   name: attach1
   download_count: 0
   created_unix: 946684800
+
+-
+  id: 9
+  uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a19
+  release_id: 1
+  name: attach1
+  download_count: 0
+  created_unix: 946684800
+
+-
+  id: 10
+  uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a20
+  name: attach1
+  download_count: 0
+  created_unix: 946684800

models/fixtures/release.yml

@@ -1 +1,14 @@
-[] # empty
+-
+  id: 1
+  repo_id: 1
+  publisher_id: 2
+  tag_name: "v1.1"
+  lower_tag_name: "v1.1"
+  target: "master"
+  title: "testing-release"
+  sha1: "65f1bf27bc3bf70f64657658635e66094edbcb4d"
+  num_commits: 10
+  is_draft: false
+  is_prerelease: false
+  is_tag: false
+  created_unix: 946684800

models/issue.go

@@ -1187,6 +1187,10 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) {
 		}
 
 		for i := 0; i < len(attachments); i++ {
+			if attachments[i].IsLinked() {
+				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)

models/issue_comment.go

@@ -554,19 +554,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].IsLinked() {
+				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().

models/release.go

@@ -126,22 +126,19 @@ func UpdateRelease(rel *Release) error {
 	return err
 }
 
-// AddReleaseAttachments adds a release attachments
-func AddReleaseAttachments(releaseID int64, attachmentUUIDs []string) (err error) {
+// LinkReleaseAttachments adds a release attachments
+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].IsLinked() {
+			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 {

public/js/index.js

@@ -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);