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

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
+	}
+}

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() {

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) {