From 5c9fbcca00caaf676abbb475815a2a3a91107f1d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 10 Oct 2023 23:33:56 +0800 Subject: [PATCH] Fix attachment download bug (#27486) --- services/auth/auth.go | 12 ++++++++---- services/auth/auth_test.go | 14 +++++++++----- services/auth/basic.go | 2 +- services/auth/oauth2.go | 2 +- services/auth/reverseproxy.go | 2 +- services/convert/attachment.go | 10 +--------- 6 files changed, 21 insertions(+), 21 deletions(-) diff --git a/services/auth/auth.go b/services/auth/auth.go index 0c8acac61f..713463a3d4 100644 --- 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 f4e3cdf0d3..3adaa28664 100644 --- 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 bb9eb7a321..6c3fbf595e 100644 --- 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 38b705cc5b..08a2a05539 100644 --- 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 ad525f5c95..359c1f2473 100644 --- 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 ab36a1c577..4a8f10f7b0 100644 --- 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