]> source.dussan.org Git - gitea.git/commitdiff
Only serve attachments when linked to issue/release and if accessible by user (#9340)
authorAntoine GIRARD <sapk@users.noreply.github.com>
Sat, 4 Jan 2020 23:20:08 +0000 (00:20 +0100)
committerLauris BH <lauris@nix.lv>
Sat, 4 Jan 2020 23:20:08 +0000 (01:20 +0200)
* test: add current attachement responses

* refactor: check if attachement is linked and accessible by user

* chore: clean TODO

* fix: typo attachement -> attachment

* revert un-needed go.sum change

* refactor: move models logic to models

* fix TestCreateIssueAttachment which was wrongly successful

* fix unit tests with unittype added

* fix unit tests with changes

* use a valid uuid format for pgsql int. test

* test: add unit test TestLinkedRepository

* refactor: allow uploader to access unlinked attachement

* add missing blank line

* refactor: move to a separate function repo.GetAttachment

* typo

* test: remove err test return

* refactor: use repo perm for access checking generally + 404 for all reject

integrations/attachement_test.go [deleted file]
integrations/attachment_test.go [new file with mode: 0644]
models/attachment.go
models/attachment_test.go
models/fixtures/attachment.yml
models/fixtures/release.yml
models/fixtures/repo_unit.yml
routers/repo/attachment.go
routers/routes/routes.go
routers/user/home_test.go

diff --git a/integrations/attachement_test.go b/integrations/attachement_test.go
deleted file mode 100644 (file)
index 8d709a3..0000000
+++ /dev/null
@@ -1,88 +0,0 @@
-// 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, 32, 32))
-       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)
-       err = writer.Close()
-       assert.NoError(t, err)
-
-       csrf := GetCSRF(t, session, repoURL)
-
-       req := NewRequestWithBody(t, "POST", "/attachments", body)
-       req.Header.Add("X-Csrf-Token", csrf)
-       req.Header.Add("Content-Type", writer.FormDataContentType())
-       resp := session.MakeRequest(t, req, expectedStatus)
-
-       if expectedStatus != http.StatusOK {
-               return ""
-       }
-       var obj map[string]string
-       DecodeJSON(t, resp, &obj)
-       return obj["uuid"]
-}
-
-func TestCreateAnonymousAttachment(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
-
-       //Validate that attachement is available
-       req = NewRequest(t, "GET", "/attachments/"+uuid)
-       session.MakeRequest(t, req, http.StatusOK)
-}
diff --git a/integrations/attachment_test.go b/integrations/attachment_test.go
new file mode 100644 (file)
index 0000000..746256d
--- /dev/null
@@ -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"
+       "io/ioutil"
+       "mime/multipart"
+       "net/http"
+       "os"
+       "path"
+       "testing"
+
+       "code.gitea.io/gitea/models"
+       "code.gitea.io/gitea/modules/test"
+
+       "github.com/stretchr/testify/assert"
+)
+
+func generateImg() bytes.Buffer {
+       // Generate image
+       myImage := image.NewRGBA(image.Rect(0, 0, 32, 32))
+       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)
+       err = writer.Close()
+       assert.NoError(t, err)
+
+       csrf := GetCSRF(t, session, repoURL)
+
+       req := NewRequestWithBody(t, "POST", "/attachments", body)
+       req.Header.Add("X-Csrf-Token", csrf)
+       req.Header.Add("Content-Type", writer.FormDataContentType())
+       resp := session.MakeRequest(t, req, expectedStatus)
+
+       if expectedStatus != http.StatusOK {
+               return ""
+       }
+       var obj map[string]string
+       DecodeJSON(t, resp, &obj)
+       return obj["uuid"]
+}
+
+func TestCreateAnonymousAttachment(t *testing.T) {
+       prepareTestEnv(t)
+       session := emptyTestSession(t)
+       createAttachment(t, session, "user2/repo1", "image.png", generateImg(), http.StatusFound)
+}
+
+func TestCreateIssueAttachment(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 Attachment",
+               "content": "some content",
+               "files":   uuid,
+       }
+
+       req = NewRequestWithValues(t, "POST", link, postData)
+       resp = session.MakeRequest(t, req, http.StatusFound)
+       test.RedirectURL(resp) // check that redirect URL exists
+
+       //Validate that attachment is available
+       req = NewRequest(t, "GET", "/attachments/"+uuid)
+       session.MakeRequest(t, req, http.StatusOK)
+}
+
+func TestGetAttachment(t *testing.T) {
+       prepareTestEnv(t)
+       adminSession := loginUser(t, "user1")
+       user2Session := loginUser(t, "user2")
+       user8Session := loginUser(t, "user8")
+       emptySession := emptyTestSession(t)
+       testCases := []struct {
+               name       string
+               uuid       string
+               createFile bool
+               session    *TestSession
+               want       int
+       }{
+               {"LinkedIssueUUID", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11", true, user2Session, http.StatusOK},
+               {"LinkedCommentUUID", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a17", true, user2Session, http.StatusOK},
+               {"linked_release_uuid", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a19", true, user2Session, http.StatusOK},
+               {"NotExistingUUID", "b0eebc99-9c0b-4ef8-bb6d-6bb9bd380a18", false, user2Session, http.StatusNotFound},
+               {"FileMissing", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a18", false, user2Session, http.StatusInternalServerError},
+               {"NotLinked", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a20", true, user2Session, http.StatusNotFound},
+               {"NotLinkedAccessibleByUploader", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a20", true, user8Session, http.StatusOK},
+               {"PublicByNonLogged", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11", true, emptySession, http.StatusOK},
+               {"PrivateByNonLogged", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12", true, emptySession, http.StatusNotFound},
+               {"PrivateAccessibleByAdmin", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12", true, adminSession, http.StatusOK},
+               {"PrivateAccessibleByUser", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12", true, user2Session, http.StatusOK},
+               {"RepoNotAccessibleByUser", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12", true, user8Session, http.StatusNotFound},
+               {"OrgNotAccessibleByUser", "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a21", true, user8Session, http.StatusNotFound},
+       }
+       for _, tc := range testCases {
+               t.Run(tc.name, func(t *testing.T) {
+                       //Write empty file to be available for response
+                       if tc.createFile {
+                               localPath := models.AttachmentLocalPath(tc.uuid)
+                               err := os.MkdirAll(path.Dir(localPath), os.ModePerm)
+                               assert.NoError(t, err)
+                               err = ioutil.WriteFile(localPath, []byte("hello world"), 0644)
+                               assert.NoError(t, err)
+                       }
+                       //Actual test
+                       req := NewRequest(t, "GET", "/attachments/"+tc.uuid)
+                       tc.session.MakeRequest(t, req, tc.want)
+               })
+       }
+}
index 487ddd4ad5b4d63b5110e805f5a99acf8ea4a7c9..6cfa6cb64ec3c194626d8fb6e435474dfd1c645f 100644 (file)
@@ -71,6 +71,26 @@ func (a *Attachment) DownloadURL() string {
        return fmt.Sprintf("%sattachments/%s", setting.AppURL, a.UUID)
 }
 
+// LinkedRepository returns the linked repo if any
+func (a *Attachment) LinkedRepository() (*Repository, UnitType, error) {
+       if a.IssueID != 0 {
+               iss, err := GetIssueByID(a.IssueID)
+               if err != nil {
+                       return nil, UnitTypeIssues, err
+               }
+               repo, err := GetRepositoryByID(iss.RepoID)
+               return repo, UnitTypeIssues, err
+       } else if a.ReleaseID != 0 {
+               rel, err := GetReleaseByID(a.ReleaseID)
+               if err != nil {
+                       return nil, UnitTypeReleases, err
+               }
+               repo, err := GetRepositoryByID(rel.RepoID)
+               return repo, UnitTypeReleases, err
+       }
+       return nil, -1, nil
+}
+
 // NewAttachment creates a new attachment object.
 func NewAttachment(attach *Attachment, buf []byte, file io.Reader) (_ *Attachment, err error) {
        attach.UUID = gouuid.NewV4().String()
index f38a5beeee13d68e90555e7674cfbd59ba9a06d8..ddb6abad3212500a7343adf2bfef6ed25bf0288d 100644 (file)
@@ -61,7 +61,7 @@ func TestGetByCommentOrIssueID(t *testing.T) {
        // count of attachments from issue ID
        attachments, err := GetAttachmentsByIssueID(1)
        assert.NoError(t, err)
-       assert.Equal(t, 2, len(attachments))
+       assert.Equal(t, 1, len(attachments))
 
        attachments, err = GetAttachmentsByCommentID(1)
        assert.NoError(t, err)
@@ -73,7 +73,7 @@ func TestDeleteAttachments(t *testing.T) {
 
        count, err := DeleteAttachmentsByIssue(4, false)
        assert.NoError(t, err)
-       assert.Equal(t, 1, count)
+       assert.Equal(t, 2, count)
 
        count, err = DeleteAttachmentsByComment(2, false)
        assert.NoError(t, err)
@@ -128,3 +128,31 @@ func TestGetAttachmentsByUUIDs(t *testing.T) {
        assert.Equal(t, int64(1), attachList[0].IssueID)
        assert.Equal(t, int64(5), attachList[1].IssueID)
 }
+
+func TestLinkedRepository(t *testing.T) {
+       assert.NoError(t, PrepareTestDatabase())
+       testCases := []struct {
+               name             string
+               attachID         int64
+               expectedRepo     *Repository
+               expectedUnitType UnitType
+       }{
+               {"LinkedIssue", 1, &Repository{ID: 1}, UnitTypeIssues},
+               {"LinkedComment", 3, &Repository{ID: 1}, UnitTypeIssues},
+               {"LinkedRelease", 9, &Repository{ID: 1}, UnitTypeReleases},
+               {"Notlinked", 10, nil, -1},
+       }
+       for _, tc := range testCases {
+               t.Run(tc.name, func(t *testing.T) {
+                       attach, err := GetAttachmentByID(tc.attachID)
+                       assert.NoError(t, err)
+                       repo, unitType, err := attach.LinkedRepository()
+                       assert.NoError(t, err)
+                       if tc.expectedRepo != nil {
+                               assert.Equal(t, tc.expectedRepo.ID, repo.ID)
+                       }
+                       assert.Equal(t, tc.expectedUnitType, unitType)
+
+               })
+       }
+}
index 289d4d0efd6c9b7e1522767e05f726f17171abd5..2606d52b47166d0da28711ce908f8d2d44e5e01e 100644 (file)
@@ -10,7 +10,7 @@
 -
   id: 2
   uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12
-  issue_id: 1
+  issue_id: 4
   comment_id: 0
   name: attach2
   download_count: 1
 -
   id: 10
   uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a20
+  uploader_id: 8
+  name: attach1
+  download_count: 0
+  created_unix: 946684800
+
+-
+  id: 11
+  uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a21
+  release_id: 2
   name: attach1
   download_count: 0
-  created_unix: 946684800 
\ No newline at end of file
+  created_unix: 946684800
index db9a6b503d45e673dc3a7e01ba5f72faa94573af..f95eb048be38c0605b11732efcb91bf580310ea3 100644 (file)
   is_draft: false
   is_prerelease: false
   is_tag: false
-  created_unix: 946684800
\ No newline at end of file
+  created_unix: 946684800
+
+-
+  id: 2
+  repo_id: 40
+  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
index f7522d30311692f44b317dceb90a461fce751805..5ced38b003fefc32ec6d954454e6adb5d8d3675a 100644 (file)
   repo_id: 48
   type: 7
   config: "{\"ExternalTrackerURL\":\"https://tracker.com\",\"ExternalTrackerFormat\":\"https://tracker.com/{user}/{repo}/issues/{index}\",\"ExternalTrackerStyle\":\"alphanumeric\"}"
+  created_unix: 946684810
+-
+  id: 69
+  repo_id: 2
+  type: 2
+  config: "{}"
   created_unix: 946684810
\ No newline at end of file
index 0d496230e16a1cdfbff93c7feaf73e18cb1d2240..96dc28a23a9d970e3f0597220caa4fb8ebf788db 100644 (file)
@@ -6,6 +6,8 @@ package repo
 
 import (
        "fmt"
+       "net/http"
+       "os"
        "strings"
 
        "code.gitea.io/gitea/models"
@@ -85,3 +87,57 @@ func DeleteAttachment(ctx *context.Context) {
                "uuid": attach.UUID,
        })
 }
+
+// GetAttachment serve attachements
+func GetAttachment(ctx *context.Context) {
+       attach, err := models.GetAttachmentByUUID(ctx.Params(":uuid"))
+       if err != nil {
+               if models.IsErrAttachmentNotExist(err) {
+                       ctx.Error(404)
+               } else {
+                       ctx.ServerError("GetAttachmentByUUID", err)
+               }
+               return
+       }
+
+       repository, unitType, err := attach.LinkedRepository()
+       if err != nil {
+               ctx.ServerError("LinkedRepository", err)
+               return
+       }
+
+       if repository == nil { //If not linked
+               if !(ctx.IsSigned && attach.UploaderID == ctx.User.ID) { //We block if not the uploader
+                       ctx.Error(http.StatusNotFound)
+                       return
+               }
+       } else { //If we have the repository we check access
+               perm, err := models.GetUserRepoPermission(repository, ctx.User)
+               if err != nil {
+                       ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err.Error())
+                       return
+               }
+               if !perm.CanRead(unitType) {
+                       ctx.Error(http.StatusNotFound)
+                       return
+               }
+       }
+
+       //If we have matched and access to release or issue
+       fr, err := os.Open(attach.LocalPath())
+       if err != nil {
+               ctx.ServerError("Open", err)
+               return
+       }
+       defer fr.Close()
+
+       if err := attach.IncreaseDownloadCount(); err != nil {
+               ctx.ServerError("Update", err)
+               return
+       }
+
+       if err = ServeData(ctx, attach.Name, fr); err != nil {
+               ctx.ServerError("ServeData", err)
+               return
+       }
+}
index c8351f312bb3f8e7503bef31e6751ab9f4267193..888c92ac4aec807aad4afa9bcbda1e00a4d49147 100644 (file)
@@ -8,7 +8,6 @@ import (
        "bytes"
        "encoding/gob"
        "net/http"
-       "os"
        "path"
        "text/template"
        "time"
@@ -474,34 +473,7 @@ func RegisterRoutes(m *macaron.Macaron) {
                        m.Get("/following", user.Following)
                })
 
-               m.Get("/attachments/:uuid", func(ctx *context.Context) {
-                       attach, err := models.GetAttachmentByUUID(ctx.Params(":uuid"))
-                       if err != nil {
-                               if models.IsErrAttachmentNotExist(err) {
-                                       ctx.Error(404)
-                               } else {
-                                       ctx.ServerError("GetAttachmentByUUID", err)
-                               }
-                               return
-                       }
-
-                       fr, err := os.Open(attach.LocalPath())
-                       if err != nil {
-                               ctx.ServerError("Open", err)
-                               return
-                       }
-                       defer fr.Close()
-
-                       if err := attach.IncreaseDownloadCount(); err != nil {
-                               ctx.ServerError("Update", err)
-                               return
-                       }
-
-                       if err = repo.ServeData(ctx, attach.Name, fr); err != nil {
-                               ctx.ServerError("ServeData", err)
-                               return
-                       }
-               })
+               m.Get("/attachments/:uuid", repo.GetAttachment)
        }, ignSignIn)
 
        m.Group("/attachments", func() {
index e5bbd0e98e6422fa62c8bb51b9ae7c91b3ad0982..39186d93eeaa9f1b85742f2e599ea3024035f308 100644 (file)
@@ -26,10 +26,10 @@ func TestIssues(t *testing.T) {
        Issues(ctx)
        assert.EqualValues(t, http.StatusOK, ctx.Resp.Status())
 
-       assert.EqualValues(t, map[int64]int64{1: 1}, ctx.Data["Counts"])
+       assert.EqualValues(t, map[int64]int64{1: 1, 2: 1}, ctx.Data["Counts"])
        assert.EqualValues(t, true, ctx.Data["IsShowClosed"])
        assert.Len(t, ctx.Data["Issues"], 1)
-       assert.Len(t, ctx.Data["Repos"], 1)
+       assert.Len(t, ctx.Data["Repos"], 2)
 }
 
 func TestMilestones(t *testing.T) {