Fix attachment download bug (#27486) (#27571)

Backport #27486 by @lunny

Fix #27204

This PR allows `/<username>/<reponame>/attachments/<uuid>` access with
personal access token and also changed attachments API download url to
it so it can be download correctly.

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

diff --git a/services/auth/auth.go b/services/auth/auth.go
index 0c8acac61f05375581cef16d7caa2980b34d0922..713463a3d47ed0c5d1528b578e9a8402ee11b066 100644 (file)
--- a/services/auth/auth.go
+++ b/services/auth/auth.go
@@ -36,12 +36,16 @@ func isContainerPath(req *http.Request) bool {
 }
 
 var (
-       gitRawReleasePathRe = regexp.MustCompile(`^/[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+/(?:(?:git-(?:(?:upload)|(?:receive))-pack$)|(?:info/refs$)|(?:HEAD$)|(?:objects/)|(?:raw/)|(?:releases/download/))`)
-       lfsPathRe           = regexp.MustCompile(`^/[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+/info/lfs/`)
+       gitRawOrAttachPathRe = regexp.MustCompile(`^/[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+/(?:(?:git-(?:(?:upload)|(?:receive))-pack$)|(?:info/refs$)|(?:HEAD$)|(?:objects/)|(?:raw/)|(?:releases/download/)|(?:attachments/))`)
+       lfsPathRe            = regexp.MustCompile(`^/[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+/info/lfs/`)
 )
 
-func isGitRawReleaseOrLFSPath(req *http.Request) bool {
-       if gitRawReleasePathRe.MatchString(req.URL.Path) {
+func isGitRawOrAttachPath(req *http.Request) bool {
+       return gitRawOrAttachPathRe.MatchString(req.URL.Path)
+}
+
+func isGitRawOrAttachOrLFSPath(req *http.Request) bool {
+       if isGitRawOrAttachPath(req) {
                return true
        }
        if setting.LFS.StartServer {

diff --git a/services/auth/auth_test.go b/services/auth/auth_test.go
index f4e3cdf0d32dc451a0031b397df2a4b61ecab73b..3adaa2866444ec7ff719dbf160970a44a6491257 100644 (file)
--- a/services/auth/auth_test.go
+++ b/services/auth/auth_test.go
@@ -85,6 +85,10 @@ func Test_isGitRawOrLFSPath(t *testing.T) {
                        "/owner/repo/releases/download/tag/repo.tar.gz",
                        true,
                },
+               {
+                       "/owner/repo/attachments/6d92a9ee-5d8b-4993-97c9-6181bdaa8955",
+                       true,
+               },
        }
        lfsTests := []string{
                "/owner/repo/info/lfs/",
@@ -104,11 +108,11 @@ func Test_isGitRawOrLFSPath(t *testing.T) {
                t.Run(tt.path, func(t *testing.T) {
                        req, _ := http.NewRequest("POST", "http://localhost"+tt.path, nil)
                        setting.LFS.StartServer = false
-                       if got := isGitRawReleaseOrLFSPath(req); got != tt.want {
+                       if got := isGitRawOrAttachOrLFSPath(req); got != tt.want {
                                t.Errorf("isGitOrLFSPath() = %v, want %v", got, tt.want)
                        }
                        setting.LFS.StartServer = true
-                       if got := isGitRawReleaseOrLFSPath(req); got != tt.want {
+                       if got := isGitRawOrAttachOrLFSPath(req); got != tt.want {
                                t.Errorf("isGitOrLFSPath() = %v, want %v", got, tt.want)
                        }
                })
@@ -117,11 +121,11 @@ func Test_isGitRawOrLFSPath(t *testing.T) {
                t.Run(tt, func(t *testing.T) {
                        req, _ := http.NewRequest("POST", tt, nil)
                        setting.LFS.StartServer = false
-                       if got := isGitRawReleaseOrLFSPath(req); got != setting.LFS.StartServer {
-                               t.Errorf("isGitOrLFSPath(%q) = %v, want %v, %v", tt, got, setting.LFS.StartServer, gitRawReleasePathRe.MatchString(tt))
+                       if got := isGitRawOrAttachOrLFSPath(req); got != setting.LFS.StartServer {
+                               t.Errorf("isGitOrLFSPath(%q) = %v, want %v, %v", tt, got, setting.LFS.StartServer, gitRawOrAttachPathRe.MatchString(tt))
                        }
                        setting.LFS.StartServer = true
-                       if got := isGitRawReleaseOrLFSPath(req); got != setting.LFS.StartServer {
+                       if got := isGitRawOrAttachOrLFSPath(req); got != setting.LFS.StartServer {
                                t.Errorf("isGitOrLFSPath(%q) = %v, want %v", tt, got, setting.LFS.StartServer)
                        }
                })

diff --git a/services/auth/basic.go b/services/auth/basic.go
index bb9eb7a3216368ddc4b6d625ca8e60b01c6c8331..6c3fbf595e44cd6ea3973831ae316f636882b767 100644 (file)
--- a/services/auth/basic.go
+++ b/services/auth/basic.go
@@ -42,7 +42,7 @@ func (b *Basic) Name() string {
 // Returns nil if header is empty or validation fails.
 func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error) {
        // Basic authentication should only fire on API, Download or on Git or LFSPaths
-       if !middleware.IsAPIPath(req) && !isContainerPath(req) && !isAttachmentDownload(req) && !isGitRawReleaseOrLFSPath(req) {
+       if !middleware.IsAPIPath(req) && !isContainerPath(req) && !isAttachmentDownload(req) && !isGitRawOrAttachOrLFSPath(req) {
                return nil, nil
        }
 

diff --git a/services/auth/oauth2.go b/services/auth/oauth2.go
index 38b705cc5b8f738724890c8678171391186c6866..08a2a05539b5abca9662c6f0d687e28b55d28b94 100644 (file)
--- a/services/auth/oauth2.go
+++ b/services/auth/oauth2.go
@@ -127,7 +127,7 @@ func (o *OAuth2) userIDFromToken(ctx context.Context, tokenSHA string, store Dat
 func (o *OAuth2) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error) {
        // These paths are not API paths, but we still want to check for tokens because they maybe in the API returned URLs
        if !middleware.IsAPIPath(req) && !isAttachmentDownload(req) && !isAuthenticatedTokenRequest(req) &&
-               !gitRawReleasePathRe.MatchString(req.URL.Path) {
+               !isGitRawOrAttachPath(req) {
                return nil, nil
        }
 

diff --git a/services/auth/reverseproxy.go b/services/auth/reverseproxy.go
index ad525f5c95fef475a814192dd11eebb51c18ddf8..359c1f24737848b2ad09ddd2420fbaaaad7c8f97 100644 (file)
--- a/services/auth/reverseproxy.go
+++ b/services/auth/reverseproxy.go
@@ -117,7 +117,7 @@ func (r *ReverseProxy) Verify(req *http.Request, w http.ResponseWriter, store Da
        }
 
        // Make sure requests to API paths, attachment downloads, git and LFS do not create a new session
-       if !middleware.IsAPIPath(req) && !isAttachmentDownload(req) && !isGitRawReleaseOrLFSPath(req) {
+       if !middleware.IsAPIPath(req) && !isAttachmentDownload(req) && !isGitRawOrAttachOrLFSPath(req) {
                if sess != nil && (sess.Get("uid") == nil || sess.Get("uid").(int64) != user.ID) {
                        handleSignIn(w, req, sess, user)
                }

diff --git a/services/convert/attachment.go b/services/convert/attachment.go
index ab36a1c577856ebd992db76620dc374149a8b8a1..4a8f10f7b02cddc2a73e7d6c4777b947c714edff 100644 (file)
--- a/services/convert/attachment.go
+++ b/services/convert/attachment.go
@@ -4,10 +4,7 @@
 package convert
 
 import (
-       "strconv"
-
        repo_model "code.gitea.io/gitea/models/repo"
-       "code.gitea.io/gitea/modules/setting"
        api "code.gitea.io/gitea/modules/structs"
 )
 
@@ -16,12 +13,7 @@ func WebAssetDownloadURL(repo *repo_model.Repository, attach *repo_model.Attachm
 }
 
 func APIAssetDownloadURL(repo *repo_model.Repository, attach *repo_model.Attachment) string {
-       if attach.CustomDownloadURL != "" {
-               return attach.CustomDownloadURL
-       }
-
-       // /repos/{owner}/{repo}/releases/{id}/assets/{attachment_id}
-       return setting.AppURL + "api/repos/" + repo.FullName() + "/releases/" + strconv.FormatInt(attach.ReleaseID, 10) + "/assets/" + strconv.FormatInt(attach.ID, 10)
+       return attach.DownloadURL()
 }
 
 // ToAttachment converts models.Attachment to api.Attachment for API usage