From 3135e146f9c27d661f6ec8088eca13ba5ab57937 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 24 Nov 2024 17:56:50 -0800 Subject: [PATCH 1/6] Strict pagination check (#32548) (cherry picked from commit c363bd06e93986a564601527ade219d602c9d8dd) Conflicts: models/user/search.go change already done in 9b85f97835a9b2f8e6af97bbec27c59210c2c94e --- models/issues/comment.go | 2 +- models/issues/issue.go | 2 +- models/issues/issue_watch.go | 2 +- models/issues/label.go | 4 ++-- models/issues/reaction.go | 2 +- models/issues/stopwatch.go | 2 +- models/issues/tracked_time.go | 2 +- models/user/user.go | 4 ++-- 8 files changed, 10 insertions(+), 10 deletions(-) diff --git a/models/issues/comment.go b/models/issues/comment.go index 90c7122174..c955f02e98 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -1102,7 +1102,7 @@ func FindComments(ctx context.Context, opts *FindCommentsOptions) (CommentList, sess.Join("INNER", "issue", "issue.id = comment.issue_id") } - if opts.Page != 0 { + if opts.Page > 0 { sess = db.SetSessionPagination(sess, opts) } diff --git a/models/issues/issue.go b/models/issues/issue.go index 7d1a5ca407..17391ffe6c 100644 --- a/models/issues/issue.go +++ b/models/issues/issue.go @@ -663,7 +663,7 @@ func (issue *Issue) BlockedByDependencies(ctx context.Context, opts db.ListOptio Where("issue_id = ?", issue.ID). // sort by repo id then created date, with the issues of the same repo at the beginning of the list OrderBy("CASE WHEN issue.repo_id = ? THEN 0 ELSE issue.repo_id END, issue.created_unix DESC", issue.RepoID) - if opts.Page != 0 { + if opts.Page > 0 { sess = db.SetSessionPagination(sess, &opts) } err = sess.Find(&issueDeps) diff --git a/models/issues/issue_watch.go b/models/issues/issue_watch.go index 9e616a0eb1..560be17eb6 100644 --- a/models/issues/issue_watch.go +++ b/models/issues/issue_watch.go @@ -105,7 +105,7 @@ func GetIssueWatchers(ctx context.Context, issueID int64, listOptions db.ListOpt And("`user`.prohibit_login = ?", false). Join("INNER", "`user`", "`user`.id = `issue_watch`.user_id") - if listOptions.Page != 0 { + if listOptions.Page > 0 { sess = db.SetSessionPagination(sess, &listOptions) watches := make([]*IssueWatch, 0, listOptions.PageSize) return watches, sess.Find(&watches) diff --git a/models/issues/label.go b/models/issues/label.go index 61478e17ac..804a118e7a 100644 --- a/models/issues/label.go +++ b/models/issues/label.go @@ -394,7 +394,7 @@ func GetLabelsByRepoID(ctx context.Context, repoID int64, sortType string, listO sess.Asc("name") } - if listOptions.Page != 0 { + if listOptions.Page > 0 { sess = db.SetSessionPagination(sess, &listOptions) } @@ -466,7 +466,7 @@ func GetLabelsByOrgID(ctx context.Context, orgID int64, sortType string, listOpt sess.Asc("name") } - if listOptions.Page != 0 { + if listOptions.Page > 0 { sess = db.SetSessionPagination(sess, &listOptions) } diff --git a/models/issues/reaction.go b/models/issues/reaction.go index eb7faefc79..11b3c6be20 100644 --- a/models/issues/reaction.go +++ b/models/issues/reaction.go @@ -163,7 +163,7 @@ func FindReactions(ctx context.Context, opts FindReactionsOptions) (ReactionList Where(opts.toConds()). In("reaction.`type`", setting.UI.Reactions). Asc("reaction.issue_id", "reaction.comment_id", "reaction.created_unix", "reaction.id") - if opts.Page != 0 { + if opts.Page > 0 { sess = db.SetSessionPagination(sess, &opts) reactions := make([]*Reaction, 0, opts.PageSize) diff --git a/models/issues/stopwatch.go b/models/issues/stopwatch.go index 93eaf8845d..68e59d59f4 100644 --- a/models/issues/stopwatch.go +++ b/models/issues/stopwatch.go @@ -81,7 +81,7 @@ func GetUIDsAndStopwatch(ctx context.Context) (map[int64][]*Stopwatch, error) { func GetUserStopwatches(ctx context.Context, userID int64, listOptions db.ListOptions) ([]*Stopwatch, error) { sws := make([]*Stopwatch, 0, 8) sess := db.GetEngine(ctx).Where("stopwatch.user_id = ?", userID) - if listOptions.Page != 0 { + if listOptions.Page > 0 { sess = db.SetSessionPagination(sess, &listOptions) } diff --git a/models/issues/tracked_time.go b/models/issues/tracked_time.go index caa582a9fc..ea404d36cd 100644 --- a/models/issues/tracked_time.go +++ b/models/issues/tracked_time.go @@ -139,7 +139,7 @@ func (opts *FindTrackedTimesOptions) toSession(e db.Engine) db.Engine { sess = sess.Where(opts.ToConds()) - if opts.Page != 0 { + if opts.Page > 0 { sess = db.SetSessionPagination(sess, opts) } diff --git a/models/user/user.go b/models/user/user.go index 96f8c3f729..d49fbdd2fc 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -339,7 +339,7 @@ func GetUserFollowers(ctx context.Context, u, viewer *User, listOptions db.ListO And("`user`.type=?", UserTypeIndividual). And(isUserVisibleToViewerCond(viewer)) - if listOptions.Page != 0 { + if listOptions.Page > 0 { sess = db.SetSessionPagination(sess, &listOptions) users := make([]*User, 0, listOptions.PageSize) @@ -361,7 +361,7 @@ func GetUserFollowing(ctx context.Context, u, viewer *User, listOptions db.ListO And("`user`.type IN (?, ?)", UserTypeIndividual, UserTypeOrganization). And(isUserVisibleToViewerCond(viewer)) - if listOptions.Page != 0 { + if listOptions.Page > 0 { sess = db.SetSessionPagination(sess, &listOptions) users := make([]*User, 0, listOptions.PageSize) From 3973f1022d57a3134e8f775e1c1cc6d398681bb4 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 25 Nov 2024 11:35:49 -0800 Subject: [PATCH 2/6] Add github compatible tarball download API endpoints (#32572) Fix #29654 Fix #32481 (cherry picked from commit 703be6bf307ed19ce8dc8cd311d24aeb6e5b9861) Conflicts: routers/api/v1/repo/file.go routers/web/repo/repo.go services/repository/archiver/archiver.go services/repository/archiver/archiver_test.go trivial context conflicts add missing function PathParam skipped in a very large refactor --- routers/api/v1/api.go | 2 + routers/api/v1/repo/download.go | 53 +++++++++++++++++++ routers/api/v1/repo/file.go | 15 ++++-- routers/web/repo/repo.go | 14 ++++- services/repository/archiver/archiver.go | 34 +++++++----- services/repository/archiver/archiver_test.go | 25 ++++----- tests/integration/api_repo_archive_test.go | 40 ++++++++++++++ 7 files changed, 152 insertions(+), 31 deletions(-) create mode 100644 routers/api/v1/repo/download.go diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 4fe10d8a00..c028a5a32d 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -1352,6 +1352,8 @@ func Routes() *web.Route { m.Post("", bind(api.UpdateRepoAvatarOption{}), repo.UpdateAvatar) m.Delete("", repo.DeleteAvatar) }, reqAdmin(), reqToken()) + + m.Get("/{ball_type:tarball|zipball|bundle}/*", reqRepoReader(unit.TypeCode), repo.DownloadArchive) }, repoAssignment(), checkTokenPublicOnly()) }, tokenRequiresScopes(auth_model.AccessTokenScopeCategoryRepository)) diff --git a/routers/api/v1/repo/download.go b/routers/api/v1/repo/download.go new file mode 100644 index 0000000000..3a0401a5b0 --- /dev/null +++ b/routers/api/v1/repo/download.go @@ -0,0 +1,53 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package repo + +import ( + "fmt" + "net/http" + + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/gitrepo" + "code.gitea.io/gitea/services/context" + archiver_service "code.gitea.io/gitea/services/repository/archiver" +) + +func DownloadArchive(ctx *context.APIContext) { + var tp git.ArchiveType + switch ballType := ctx.Params("ball_type"); ballType { + case "tarball": + tp = git.TARGZ + case "zipball": + tp = git.ZIP + case "bundle": + tp = git.BUNDLE + default: + ctx.Error(http.StatusBadRequest, "", fmt.Sprintf("Unknown archive type: %s", ballType)) + return + } + + if ctx.Repo.GitRepo == nil { + gitRepo, err := gitrepo.OpenRepository(ctx, ctx.Repo.Repository) + if err != nil { + ctx.Error(http.StatusInternalServerError, "OpenRepository", err) + return + } + ctx.Repo.GitRepo = gitRepo + defer gitRepo.Close() + } + + r, err := archiver_service.NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, ctx.Params("*"), tp) + if err != nil { + ctx.ServerError("NewRequest", err) + return + } + + archive, err := r.Await(ctx) + if err != nil { + ctx.ServerError("archive.Await", err) + return + } + + download(ctx, r.GetArchiveName(), archive) +} diff --git a/routers/api/v1/repo/file.go b/routers/api/v1/repo/file.go index 50d2786ec8..2cea538b72 100644 --- a/routers/api/v1/repo/file.go +++ b/routers/api/v1/repo/file.go @@ -308,7 +308,13 @@ func GetArchive(ctx *context.APIContext) { func archiveDownload(ctx *context.APIContext) { uri := ctx.Params("*") - aReq, err := archiver_service.NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, uri) + ext, tp, err := archiver_service.ParseFileName(uri) + if err != nil { + ctx.Error(http.StatusBadRequest, "ParseFileName", err) + return + } + + aReq, err := archiver_service.NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, strings.TrimSuffix(uri, ext), tp) if err != nil { if errors.Is(err, archiver_service.ErrUnknownArchiveFormat{}) { ctx.Error(http.StatusBadRequest, "unknown archive format", err) @@ -334,9 +340,12 @@ func download(ctx *context.APIContext, archiveName string, archiver *repo_model. // Add nix format link header so tarballs lock correctly: // https://github.com/nixos/nix/blob/56763ff918eb308db23080e560ed2ea3e00c80a7/doc/manual/src/protocols/tarball-fetcher.md - ctx.Resp.Header().Add("Link", fmt.Sprintf("<%s/archive/%s.tar.gz?rev=%s>; rel=\"immutable\"", + ctx.Resp.Header().Add("Link", fmt.Sprintf(`<%s/archive/%s.%s?rev=%s>; rel="immutable"`, ctx.Repo.Repository.APIURL(), - archiver.CommitID, archiver.CommitID)) + archiver.CommitID, + archiver.Type.String(), + archiver.CommitID, + )) rPath := archiver.RelativePath() if setting.RepoArchive.Storage.MinioConfig.ServeDirect { diff --git a/routers/web/repo/repo.go b/routers/web/repo/repo.go index 8036bcae67..1c4fb39546 100644 --- a/routers/web/repo/repo.go +++ b/routers/web/repo/repo.go @@ -472,7 +472,12 @@ func RedirectDownload(ctx *context.Context) { // Download an archive of a repository func Download(ctx *context.Context) { uri := ctx.Params("*") - aReq, err := archiver_service.NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, uri) + ext, tp, err := archiver_service.ParseFileName(uri) + if err != nil { + ctx.ServerError("ParseFileName", err) + return + } + aReq, err := archiver_service.NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, strings.TrimSuffix(uri, ext), tp) if err != nil { if errors.Is(err, archiver_service.ErrUnknownArchiveFormat{}) { ctx.Error(http.StatusBadRequest, err.Error()) @@ -547,7 +552,12 @@ func download(ctx *context.Context, archiveName string, archiver *repo_model.Rep // kind of drop it on the floor if this is the case. func InitiateDownload(ctx *context.Context) { uri := ctx.Params("*") - aReq, err := archiver_service.NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, uri) + ext, tp, err := archiver_service.ParseFileName(uri) + if err != nil { + ctx.ServerError("ParseFileName", err) + return + } + aReq, err := archiver_service.NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, strings.TrimSuffix(uri, ext), tp) if err != nil { ctx.ServerError("archiver_service.NewRequest", err) return diff --git a/services/repository/archiver/archiver.go b/services/repository/archiver/archiver.go index 73dcb0795f..279067c002 100644 --- a/services/repository/archiver/archiver.go +++ b/services/repository/archiver/archiver.go @@ -68,30 +68,36 @@ func (e RepoRefNotFoundError) Is(err error) bool { return ok } -// NewRequest creates an archival request, based on the URI. The -// resulting ArchiveRequest is suitable for being passed to Await() -// if it's determined that the request still needs to be satisfied. -func NewRequest(ctx context.Context, repoID int64, repo *git.Repository, uri string) (*ArchiveRequest, error) { - r := &ArchiveRequest{ - RepoID: repoID, - } - - var ext string +func ParseFileName(uri string) (ext string, tp git.ArchiveType, err error) { switch { case strings.HasSuffix(uri, ".zip"): ext = ".zip" - r.Type = git.ZIP + tp = git.ZIP case strings.HasSuffix(uri, ".tar.gz"): ext = ".tar.gz" - r.Type = git.TARGZ + tp = git.TARGZ case strings.HasSuffix(uri, ".bundle"): ext = ".bundle" - r.Type = git.BUNDLE + tp = git.BUNDLE default: - return nil, ErrUnknownArchiveFormat{RequestFormat: uri} + return "", 0, ErrUnknownArchiveFormat{RequestFormat: uri} + } + return ext, tp, nil +} + +// NewRequest creates an archival request, based on the URI. The +// resulting ArchiveRequest is suitable for being passed to Await() +// if it's determined that the request still needs to be satisfied. +func NewRequest(ctx context.Context, repoID int64, repo *git.Repository, refName string, fileType git.ArchiveType) (*ArchiveRequest, error) { + if fileType < git.ZIP || fileType > git.BUNDLE { + return nil, ErrUnknownArchiveFormat{RequestFormat: fileType.String()} } - r.refName = strings.TrimSuffix(uri, ext) + r := &ArchiveRequest{ + RepoID: repoID, + refName: refName, + Type: fileType, + } // Get corresponding commit. commitID, err := repo.ConvertToGitID(r.refName) diff --git a/services/repository/archiver/archiver_test.go b/services/repository/archiver/archiver_test.go index 6d9224da9f..e7a2422e55 100644 --- a/services/repository/archiver/archiver_test.go +++ b/services/repository/archiver/archiver_test.go @@ -9,6 +9,7 @@ import ( "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/unittest" + "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/services/contexttest" _ "code.gitea.io/gitea/models/actions" @@ -32,47 +33,47 @@ func TestArchive_Basic(t *testing.T) { contexttest.LoadGitRepo(t, ctx) defer ctx.Repo.GitRepo.Close() - bogusReq, err := NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, firstCommit+".zip") + bogusReq, err := NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, firstCommit, git.ZIP) require.NoError(t, err) assert.NotNil(t, bogusReq) assert.EqualValues(t, firstCommit+".zip", bogusReq.GetArchiveName()) // Check a series of bogus requests. // Step 1, valid commit with a bad extension. - bogusReq, err = NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, firstCommit+".dilbert") + bogusReq, err = NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, firstCommit, 100) require.Error(t, err) assert.Nil(t, bogusReq) // Step 2, missing commit. - bogusReq, err = NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, "dbffff.zip") + bogusReq, err = NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, "dbffff", git.ZIP) require.Error(t, err) assert.Nil(t, bogusReq) // Step 3, doesn't look like branch/tag/commit. - bogusReq, err = NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, "db.zip") + bogusReq, err = NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, "db", git.ZIP) require.Error(t, err) assert.Nil(t, bogusReq) - bogusReq, err = NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, "master.zip") + bogusReq, err = NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, "master", git.ZIP) require.NoError(t, err) assert.NotNil(t, bogusReq) assert.EqualValues(t, "master.zip", bogusReq.GetArchiveName()) - bogusReq, err = NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, "test/archive.zip") + bogusReq, err = NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, "test/archive", git.ZIP) require.NoError(t, err) assert.NotNil(t, bogusReq) assert.EqualValues(t, "test-archive.zip", bogusReq.GetArchiveName()) // Now two valid requests, firstCommit with valid extensions. - zipReq, err := NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, firstCommit+".zip") + zipReq, err := NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, firstCommit, git.ZIP) require.NoError(t, err) assert.NotNil(t, zipReq) - tgzReq, err := NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, firstCommit+".tar.gz") + tgzReq, err := NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, firstCommit, git.TARGZ) require.NoError(t, err) assert.NotNil(t, tgzReq) - secondReq, err := NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, secondCommit+".zip") + secondReq, err := NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, secondCommit, git.ZIP) require.NoError(t, err) assert.NotNil(t, secondReq) @@ -92,7 +93,7 @@ func TestArchive_Basic(t *testing.T) { // Sleep two seconds to make sure the queue doesn't change. time.Sleep(2 * time.Second) - zipReq2, err := NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, firstCommit+".zip") + zipReq2, err := NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, firstCommit, git.ZIP) require.NoError(t, err) // This zipReq should match what's sitting in the queue, as we haven't // let it release yet. From the consumer's point of view, this looks like @@ -107,12 +108,12 @@ func TestArchive_Basic(t *testing.T) { // Now we'll submit a request and TimedWaitForCompletion twice, before and // after we release it. We should trigger both the timeout and non-timeout // cases. - timedReq, err := NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, secondCommit+".tar.gz") + timedReq, err := NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, secondCommit, git.TARGZ) require.NoError(t, err) assert.NotNil(t, timedReq) doArchive(db.DefaultContext, timedReq) - zipReq2, err = NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, firstCommit+".zip") + zipReq2, err = NewRequest(ctx, ctx.Repo.Repository.ID, ctx.Repo.GitRepo, firstCommit, git.ZIP) require.NoError(t, err) // Now, we're guaranteed to have released the original zipReq from the queue. // Ensure that we don't get handed back the released entry somehow, but they diff --git a/tests/integration/api_repo_archive_test.go b/tests/integration/api_repo_archive_test.go index a16136a39b..fab3dedfab 100644 --- a/tests/integration/api_repo_archive_test.go +++ b/tests/integration/api_repo_archive_test.go @@ -63,3 +63,43 @@ func TestAPIDownloadArchive(t *testing.T) { link, _ = url.Parse(fmt.Sprintf("/api/v1/repos/%s/%s/archive/master", user2.Name, repo.Name)) MakeRequest(t, NewRequest(t, "GET", link.String()).AddTokenAuth(token), http.StatusBadRequest) } + +func TestAPIDownloadArchive2(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + session := loginUser(t, user2.LowerName) + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadRepository) + + link, _ := url.Parse(fmt.Sprintf("/api/v1/repos/%s/%s/zipball/master", user2.Name, repo.Name)) + resp := MakeRequest(t, NewRequest(t, "GET", link.String()).AddTokenAuth(token), http.StatusOK) + bs, err := io.ReadAll(resp.Body) + require.NoError(t, err) + assert.Len(t, bs, 320) + + link, _ = url.Parse(fmt.Sprintf("/api/v1/repos/%s/%s/tarball/master", user2.Name, repo.Name)) + resp = MakeRequest(t, NewRequest(t, "GET", link.String()).AddTokenAuth(token), http.StatusOK) + bs, err = io.ReadAll(resp.Body) + require.NoError(t, err) + assert.Len(t, bs, 266) + + // Must return a link to a commit ID as the "immutable" archive link + linkHeaderRe := regexp.MustCompile(`^<(https?://.*/api/v1/repos/user2/repo1/archive/[a-f0-9]+\.tar\.gz.*)>; rel="immutable"$`) + m := linkHeaderRe.FindStringSubmatch(resp.Header().Get("Link")) + assert.NotEmpty(t, m[1]) + resp = MakeRequest(t, NewRequest(t, "GET", m[1]).AddTokenAuth(token), http.StatusOK) + bs2, err := io.ReadAll(resp.Body) + require.NoError(t, err) + // The locked URL should give the same bytes as the non-locked one + assert.EqualValues(t, bs, bs2) + + link, _ = url.Parse(fmt.Sprintf("/api/v1/repos/%s/%s/bundle/master", user2.Name, repo.Name)) + resp = MakeRequest(t, NewRequest(t, "GET", link.String()).AddTokenAuth(token), http.StatusOK) + bs, err = io.ReadAll(resp.Body) + require.NoError(t, err) + assert.Len(t, bs, 382) + + link, _ = url.Parse(fmt.Sprintf("/api/v1/repos/%s/%s/archive/master", user2.Name, repo.Name)) + MakeRequest(t, NewRequest(t, "GET", link.String()).AddTokenAuth(token), http.StatusBadRequest) +} From 2e00ae4cddff6ba04fb52adc44b21293857f4267 Mon Sep 17 00:00:00 2001 From: Rowan Bohde Date: Wed, 27 Nov 2024 20:50:27 -0600 Subject: [PATCH 3/6] Validate OAuth Redirect URIs (#32643) This fixes a TODO in the code to validate the RedirectURIs when adding or editing an OAuth application in user settings. This also includes a refactor of the user settings tests to only create the DB once per top-level test to avoid reloading fixtures. (cherry picked from commit 16a7d343d78807e39df124756e5d43a69a2203a3) Conflicts: services/forms/user_form.go tests/integration/user_settings_test.go simple conflicts --- modules/util/truncate.go | 2 + modules/validation/binding.go | 37 ++++- modules/validation/binding_test.go | 1 + modules/validation/validurllist_test.go | 157 ++++++++++++++++++++++ routers/web/user/setting/oauth2_common.go | 17 ++- services/forms/user_form.go | 2 +- 6 files changed, 209 insertions(+), 7 deletions(-) create mode 100644 modules/validation/validurllist_test.go diff --git a/modules/util/truncate.go b/modules/util/truncate.go index 77b116eeff..f2edbdc673 100644 --- a/modules/util/truncate.go +++ b/modules/util/truncate.go @@ -41,6 +41,8 @@ func SplitStringAtByteN(input string, n int) (left, right string) { // SplitTrimSpace splits the string at given separator and trims leading and trailing space func SplitTrimSpace(input, sep string) []string { + // Trim initial leading & trailing space + input = strings.TrimSpace(input) // replace CRLF with LF input = strings.ReplaceAll(input, "\r\n", "\n") diff --git a/modules/validation/binding.go b/modules/validation/binding.go index 1c81423ab3..006fbfafc1 100644 --- a/modules/validation/binding.go +++ b/modules/validation/binding.go @@ -10,6 +10,7 @@ import ( "code.gitea.io/gitea/modules/auth" "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/util" "code.forgejo.org/go-chi/binding" "github.com/gobwas/glob" @@ -33,6 +34,7 @@ const ( // AddBindingRules adds additional binding rules func AddBindingRules() { addGitRefNameBindingRule() + addValidURLListBindingRule() addValidURLBindingRule() addValidSiteURLBindingRule() addGlobPatternRule() @@ -47,7 +49,7 @@ func addGitRefNameBindingRule() { // Git refname validation rule binding.AddRule(&binding.Rule{ IsMatch: func(rule string) bool { - return strings.HasPrefix(rule, "GitRefName") + return rule == "GitRefName" }, IsValid: func(errs binding.Errors, name string, val any) (bool, binding.Errors) { str := fmt.Sprintf("%v", val) @@ -61,11 +63,38 @@ func addGitRefNameBindingRule() { }) } +func addValidURLListBindingRule() { + // URL validation rule + binding.AddRule(&binding.Rule{ + IsMatch: func(rule string) bool { + return rule == "ValidUrlList" + }, + IsValid: func(errs binding.Errors, name string, val any) (bool, binding.Errors) { + str := fmt.Sprintf("%v", val) + if len(str) == 0 { + errs.Add([]string{name}, binding.ERR_URL, "Url") + return false, errs + } + + ok := true + urls := util.SplitTrimSpace(str, "\n") + for _, u := range urls { + if !IsValidURL(u) { + ok = false + errs.Add([]string{name}, binding.ERR_URL, u) + } + } + + return ok, errs + }, + }) +} + func addValidURLBindingRule() { // URL validation rule binding.AddRule(&binding.Rule{ IsMatch: func(rule string) bool { - return strings.HasPrefix(rule, "ValidUrl") + return rule == "ValidUrl" }, IsValid: func(errs binding.Errors, name string, val any) (bool, binding.Errors) { str := fmt.Sprintf("%v", val) @@ -83,7 +112,7 @@ func addValidSiteURLBindingRule() { // URL validation rule binding.AddRule(&binding.Rule{ IsMatch: func(rule string) bool { - return strings.HasPrefix(rule, "ValidSiteUrl") + return rule == "ValidSiteUrl" }, IsValid: func(errs binding.Errors, name string, val any) (bool, binding.Errors) { str := fmt.Sprintf("%v", val) @@ -174,7 +203,7 @@ func addUsernamePatternRule() { func addValidGroupTeamMapRule() { binding.AddRule(&binding.Rule{ IsMatch: func(rule string) bool { - return strings.HasPrefix(rule, "ValidGroupTeamMap") + return rule == "ValidGroupTeamMap" }, IsValid: func(errs binding.Errors, name string, val any) (bool, binding.Errors) { _, err := auth.UnmarshalGroupTeamMapping(fmt.Sprintf("%v", val)) diff --git a/modules/validation/binding_test.go b/modules/validation/binding_test.go index 4f1031abf3..5adcdf0289 100644 --- a/modules/validation/binding_test.go +++ b/modules/validation/binding_test.go @@ -27,6 +27,7 @@ type ( TestForm struct { BranchName string `form:"BranchName" binding:"GitRefName"` URL string `form:"ValidUrl" binding:"ValidUrl"` + URLs string `form:"ValidUrls" binding:"ValidUrlList"` GlobPattern string `form:"GlobPattern" binding:"GlobPattern"` RegexPattern string `form:"RegexPattern" binding:"RegexPattern"` } diff --git a/modules/validation/validurllist_test.go b/modules/validation/validurllist_test.go new file mode 100644 index 0000000000..506f96da69 --- /dev/null +++ b/modules/validation/validurllist_test.go @@ -0,0 +1,157 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package validation + +import ( + "testing" + + "code.forgejo.org/go-chi/binding" +) + +// This is a copy of all the URL tests cases, plus additional ones to +// account for multiple URLs +var urlListValidationTestCases = []validationTestCase{ + { + description: "Empty URL", + data: TestForm{ + URLs: "", + }, + expectedErrors: binding.Errors{}, + }, + { + description: "URL without port", + data: TestForm{ + URLs: "http://test.lan/", + }, + expectedErrors: binding.Errors{}, + }, + { + description: "URL with port", + data: TestForm{ + URLs: "http://test.lan:3000/", + }, + expectedErrors: binding.Errors{}, + }, + { + description: "URL with IPv6 address without port", + data: TestForm{ + URLs: "http://[::1]/", + }, + expectedErrors: binding.Errors{}, + }, + { + description: "URL with IPv6 address with port", + data: TestForm{ + URLs: "http://[::1]:3000/", + }, + expectedErrors: binding.Errors{}, + }, + { + description: "Invalid URL", + data: TestForm{ + URLs: "http//test.lan/", + }, + expectedErrors: binding.Errors{ + binding.Error{ + FieldNames: []string{"URLs"}, + Classification: binding.ERR_URL, + Message: "http//test.lan/", + }, + }, + }, + { + description: "Invalid schema", + data: TestForm{ + URLs: "ftp://test.lan/", + }, + expectedErrors: binding.Errors{ + binding.Error{ + FieldNames: []string{"URLs"}, + Classification: binding.ERR_URL, + Message: "ftp://test.lan/", + }, + }, + }, + { + description: "Invalid port", + data: TestForm{ + URLs: "http://test.lan:3x4/", + }, + expectedErrors: binding.Errors{ + binding.Error{ + FieldNames: []string{"URLs"}, + Classification: binding.ERR_URL, + Message: "http://test.lan:3x4/", + }, + }, + }, + { + description: "Invalid port with IPv6 address", + data: TestForm{ + URLs: "http://[::1]:3x4/", + }, + expectedErrors: binding.Errors{ + binding.Error{ + FieldNames: []string{"URLs"}, + Classification: binding.ERR_URL, + Message: "http://[::1]:3x4/", + }, + }, + }, + { + description: "Multi URLs", + data: TestForm{ + URLs: "http://test.lan:3000/\nhttp://test.local/", + }, + expectedErrors: binding.Errors{}, + }, + { + description: "Multi URLs with newline", + data: TestForm{ + URLs: "http://test.lan:3000/\nhttp://test.local/\n", + }, + expectedErrors: binding.Errors{}, + }, + { + description: "List with invalid entry", + data: TestForm{ + URLs: "http://test.lan:3000/\nhttp://[::1]:3x4/", + }, + expectedErrors: binding.Errors{ + binding.Error{ + FieldNames: []string{"URLs"}, + Classification: binding.ERR_URL, + Message: "http://[::1]:3x4/", + }, + }, + }, + { + description: "List with two invalid entries", + data: TestForm{ + URLs: "ftp://test.lan:3000/\nhttp://[::1]:3x4/\n", + }, + expectedErrors: binding.Errors{ + binding.Error{ + FieldNames: []string{"URLs"}, + Classification: binding.ERR_URL, + Message: "ftp://test.lan:3000/", + }, + binding.Error{ + FieldNames: []string{"URLs"}, + Classification: binding.ERR_URL, + Message: "http://[::1]:3x4/", + }, + }, + }, +} + +func Test_ValidURLListValidation(t *testing.T) { + AddBindingRules() + + for _, testCase := range urlListValidationTestCases { + t.Run(testCase.description, func(t *testing.T) { + performValidationTest(t, testCase) + }) + } +} diff --git a/routers/web/user/setting/oauth2_common.go b/routers/web/user/setting/oauth2_common.go index 85d1e820a5..2132d127b8 100644 --- a/routers/web/user/setting/oauth2_common.go +++ b/routers/web/user/setting/oauth2_common.go @@ -47,7 +47,6 @@ func (oa *OAuth2CommonHandlers) AddApp(ctx *context.Context) { return } - // TODO validate redirect URI app, err := auth.CreateOAuth2Application(ctx, auth.CreateOAuth2ApplicationOptions{ Name: form.Name, RedirectURIs: util.SplitTrimSpace(form.RedirectURIs, "\n"), @@ -95,11 +94,25 @@ func (oa *OAuth2CommonHandlers) EditSave(ctx *context.Context) { form := web.GetForm(ctx).(*forms.EditOAuth2ApplicationForm) if ctx.HasError() { + app, err := auth.GetOAuth2ApplicationByID(ctx, ctx.ParamsInt64("id")) + if err != nil { + if auth.IsErrOAuthApplicationNotFound(err) { + ctx.NotFound("Application not found", err) + return + } + ctx.ServerError("GetOAuth2ApplicationByID", err) + return + } + if app.UID != oa.OwnerID { + ctx.NotFound("Application not found", nil) + return + } + ctx.Data["App"] = app + oa.renderEditPage(ctx) return } - // TODO validate redirect URI var err error if ctx.Data["App"], err = auth.UpdateOAuth2Application(ctx, auth.UpdateOAuth2ApplicationOptions{ ID: ctx.ParamsInt64("id"), diff --git a/services/forms/user_form.go b/services/forms/user_form.go index afd3a42982..3ba8724c92 100644 --- a/services/forms/user_form.go +++ b/services/forms/user_form.go @@ -388,7 +388,7 @@ func (f *NewAccessTokenForm) GetScope() (auth_model.AccessTokenScope, error) { // EditOAuth2ApplicationForm form for editing oauth2 applications type EditOAuth2ApplicationForm struct { Name string `binding:"Required;MaxSize(255)" form:"application_name"` - RedirectURIs string `binding:"Required" form:"redirect_uris"` + RedirectURIs string `binding:"Required;ValidUrlList" form:"redirect_uris"` ConfidentialClient bool `form:"confidential_client"` } From 7aec65b48aa114f516870a7dc4a78374e407672b Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Sat, 30 Nov 2024 04:32:10 +0800 Subject: [PATCH 4/6] Fix a bug in actions artifact test (#32672) This bug exists in `TestActionsArtifactDownload`. https://github.com/go-gitea/gitea/blob/a1f56f83bff56f86180e59742efd3748908b82c1/tests/integration/api_actions_artifact_test.go#L123-L134 We assert that `listResp.Count` is `2`, so `artifactIdx` could be `0` or `1`. https://github.com/go-gitea/gitea/blob/a1f56f83bff56f86180e59742efd3748908b82c1/tests/integration/api_actions_artifact_test.go#L144-L147 Then we assert that the length of `downloadResp.Value` is `1`. If `artifactIdx` is `1` at this point, the assertion on Line 147 will throw an `index out of range` error. (cherry picked from commit fd3aa5bedb07d295d48b1f550c19ad1b387ba83f) --- tests/integration/api_actions_artifact_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/integration/api_actions_artifact_test.go b/tests/integration/api_actions_artifact_test.go index 5b300b96b1..6393fc53cc 100644 --- a/tests/integration/api_actions_artifact_test.go +++ b/tests/integration/api_actions_artifact_test.go @@ -144,12 +144,12 @@ func TestActionsArtifactDownload(t *testing.T) { var downloadResp downloadArtifactResponse DecodeJSON(t, resp, &downloadResp) assert.Len(t, downloadResp.Value, 1) - assert.Equal(t, "artifact-download/abc.txt", downloadResp.Value[artifactIdx].Path) - assert.Equal(t, "file", downloadResp.Value[artifactIdx].ItemType) - assert.Contains(t, downloadResp.Value[artifactIdx].ContentLocation, "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts") + assert.Equal(t, "artifact-download/abc.txt", downloadResp.Value[0].Path) + assert.Equal(t, "file", downloadResp.Value[0].ItemType) + assert.Contains(t, downloadResp.Value[0].ContentLocation, "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts") - idx = strings.Index(downloadResp.Value[artifactIdx].ContentLocation, "/api/actions_pipeline/_apis/pipelines/") - url = downloadResp.Value[artifactIdx].ContentLocation[idx:] + idx = strings.Index(downloadResp.Value[0].ContentLocation, "/api/actions_pipeline/_apis/pipelines/") + url = downloadResp.Value[0].ContentLocation[idx:] req = NewRequest(t, "GET", url). AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") resp = MakeRequest(t, req, http.StatusOK) From 2faccf5374034e09bce5277d78e9328f9b10c2a5 Mon Sep 17 00:00:00 2001 From: william-allspice Date: Tue, 26 Nov 2024 00:37:24 -0600 Subject: [PATCH 5/6] Fix race condition in mermaid observer (#32599) This Pull Request addresses a race condition in the updateIframeHeight function where it is sometimes called when the iframe is not fully loaded or accessible resulting in an alarming error message for the user. To address this we: 1. Add defensive programming within the updateIframeHeight function 2. Delay instantiating the intersection observer until the iframe has loaded Co-authored-by: wxiaoguang (cherry picked from commit 88f5d33ab267f330ffaf02eb019e772ed06ed34f) --- web_src/js/markup/mermaid.js | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/web_src/js/markup/mermaid.js b/web_src/js/markup/mermaid.js index e420ff12a2..cf69739fee 100644 --- a/web_src/js/markup/mermaid.js +++ b/web_src/js/markup/mermaid.js @@ -57,16 +57,12 @@ export async function renderMermaid() { mermaidBlock.append(btn); const updateIframeHeight = () => { - iframe.style.height = `${iframe.contentWindow.document.body.clientHeight}px`; + const body = iframe.contentWindow?.document?.body; + if (body) { + iframe.style.height = `${body.clientHeight}px`; + } }; - // update height when element's visibility state changes, for example when the diagram is inside - // a
+ block and the
block becomes visible upon user interaction, it - // would initially set a incorrect height and the correct height is set during this callback. - (new IntersectionObserver(() => { - updateIframeHeight(); - }, {root: document.documentElement})).observe(iframe); - iframe.addEventListener('load', () => { pre.replaceWith(mermaidBlock); mermaidBlock.classList.remove('tw-hidden'); @@ -75,6 +71,13 @@ export async function renderMermaid() { mermaidBlock.classList.remove('is-loading'); iframe.classList.remove('tw-invisible'); }, 0); + + // update height when element's visibility state changes, for example when the diagram is inside + // a
+ block and the
block becomes visible upon user interaction, it + // would initially set a incorrect height and the correct height is set during this callback. + (new IntersectionObserver(() => { + updateIframeHeight(); + }, {root: document.documentElement})).observe(iframe); }); document.body.append(mermaidBlock); From df37e245d0c283e0b9970df7e362701cd8d15d01 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Tue, 3 Dec 2024 10:21:20 +0100 Subject: [PATCH 6/6] chore(release-notes): notes for the week 2024-49 weekly cherry pick --- release-notes/6110.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 release-notes/6110.md diff --git a/release-notes/6110.md b/release-notes/6110.md new file mode 100644 index 0000000000..b87ec44882 --- /dev/null +++ b/release-notes/6110.md @@ -0,0 +1 @@ +feat: [commit](https://codeberg.org/forgejo/forgejo/commit/3973f1022d57a3134e8f775e1c1cc6d398681bb4) Add github compatible tarball download API endpoints