From 8b2407371365fc123fc368bfd46b15f55ba8ae6a Mon Sep 17 00:00:00 2001 From: Antoine GIRARD Date: Sun, 5 Jan 2020 00:20:08 +0100 Subject: [PATCH] Only serve attachments when linked to issue/release and if accessible by user (#9340) * 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 | 88 -------------------- integrations/attachment_test.go | 137 +++++++++++++++++++++++++++++++ models/attachment.go | 20 +++++ models/attachment_test.go | 32 +++++++- models/fixtures/attachment.yml | 13 ++- models/fixtures/release.yml | 17 +++- models/fixtures/repo_unit.yml | 6 ++ routers/repo/attachment.go | 56 +++++++++++++ routers/routes/routes.go | 30 +------ routers/user/home_test.go | 4 +- 10 files changed, 279 insertions(+), 124 deletions(-) delete mode 100644 integrations/attachement_test.go create mode 100644 integrations/attachment_test.go diff --git a/integrations/attachement_test.go b/integrations/attachement_test.go deleted file mode 100644 index 8d709a376..000000000 --- a/integrations/attachement_test.go +++ /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 index 000000000..746256df9 --- /dev/null +++ b/integrations/attachment_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" + "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) + }) + } +} diff --git a/models/attachment.go b/models/attachment.go index 487ddd4ad..6cfa6cb64 100644 --- a/models/attachment.go +++ b/models/attachment.go @@ -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() diff --git a/models/attachment_test.go b/models/attachment_test.go index f38a5beee..ddb6abad3 100644 --- a/models/attachment_test.go +++ b/models/attachment_test.go @@ -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) + + }) + } +} diff --git a/models/fixtures/attachment.yml b/models/fixtures/attachment.yml index 289d4d0ef..2606d52b4 100644 --- a/models/fixtures/attachment.yml +++ b/models/fixtures/attachment.yml @@ -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 @@ -81,6 +81,15 @@ - id: 10 uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a20 + uploader_id: 8 name: attach1 download_count: 0 - created_unix: 946684800 \ No newline at end of file + created_unix: 946684800 + +- + id: 11 + uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a21 + release_id: 2 + name: attach1 + download_count: 0 + created_unix: 946684800 diff --git a/models/fixtures/release.yml b/models/fixtures/release.yml index db9a6b503..f95eb048b 100644 --- a/models/fixtures/release.yml +++ b/models/fixtures/release.yml @@ -11,4 +11,19 @@ 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 diff --git a/models/fixtures/repo_unit.yml b/models/fixtures/repo_unit.yml index f7522d303..5ced38b00 100644 --- a/models/fixtures/repo_unit.yml +++ b/models/fixtures/repo_unit.yml @@ -472,4 +472,10 @@ 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 diff --git a/routers/repo/attachment.go b/routers/repo/attachment.go index 0d496230e..96dc28a23 100644 --- a/routers/repo/attachment.go +++ b/routers/repo/attachment.go @@ -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 + } +} diff --git a/routers/routes/routes.go b/routers/routes/routes.go index c8351f312..888c92ac4 100644 --- a/routers/routes/routes.go +++ b/routers/routes/routes.go @@ -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() { diff --git a/routers/user/home_test.go b/routers/user/home_test.go index e5bbd0e98..39186d93e 100644 --- a/routers/user/home_test.go +++ b/routers/user/home_test.go @@ -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) {