From 2d36a0c9ff93004f8dabe8d137cd20d0f2263451 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sun, 19 Oct 2025 00:37:50 +0800 Subject: [PATCH 01/29] Fix various bugs (#35684) 1. Fix incorrect column in `applySubscribedCondition`, add a test 2. Fix debian version parsing, add more tests fix #35695 3. Fix log level for HTTP errors, fix #35651 4. Fix abused "panic" handler in API `Migrate` 5. Fix the redirection from PR to issue, add a test 6. Fix Actions variable & secret name validation, add more tests * envNameCIRegexMatch is unnecessary, removed * validating in "delete" function doesn't make sense, removed 7. Fix incorrect link in release email --------- Signed-off-by: wxiaoguang Co-authored-by: delvh --- models/issues/issue_search.go | 2 +- models/issues/issue_test.go | 6 +++ modules/packages/debian/metadata.go | 2 +- modules/packages/debian/metadata_test.go | 8 ++++ modules/web/routing/logger.go | 3 +- routers/api/v1/repo/migrate.go | 50 +++++++++++----------- routers/web/repo/pull.go | 2 +- services/actions/variables.go | 34 --------------- services/secrets/secrets.go | 4 -- services/secrets/validation.go | 24 +++++++---- services/secrets/validation_test.go | 29 +++++++++++++ templates/mail/repo/release.tmpl | 4 +- tests/integration/api_repo_secrets_test.go | 4 -- tests/integration/api_user_secrets_test.go | 4 -- tests/integration/issue_test.go | 8 +++- 15 files changed, 98 insertions(+), 86 deletions(-) create mode 100644 services/secrets/validation_test.go diff --git a/models/issues/issue_search.go b/models/issues/issue_search.go index 466e788d6c..049dcc7de8 100644 --- a/models/issues/issue_search.go +++ b/models/issues/issue_search.go @@ -476,7 +476,7 @@ func applySubscribedCondition(sess *xorm.Session, subscriberID int64) { ), builder.Eq{"issue.poster_id": subscriberID}, builder.In("issue.repo_id", builder. - Select("id"). + Select("repo_id"). From("watch"). Where(builder.And(builder.Eq{"user_id": subscriberID}, builder.In("mode", repo_model.WatchModeNormal, repo_model.WatchModeAuto))), diff --git a/models/issues/issue_test.go b/models/issues/issue_test.go index 09fd492667..55a90f50a1 100644 --- a/models/issues/issue_test.go +++ b/models/issues/issue_test.go @@ -197,6 +197,12 @@ func TestIssues(t *testing.T) { }, []int64{2}, }, + { + issues_model.IssuesOptions{ + SubscriberID: 11, + }, + []int64{11, 5, 9, 8, 3, 2, 1}, + }, } { issues, err := issues_model.Issues(t.Context(), &test.Opts) assert.NoError(t, err) diff --git a/modules/packages/debian/metadata.go b/modules/packages/debian/metadata.go index e76db63975..1cae46ecb2 100644 --- a/modules/packages/debian/metadata.go +++ b/modules/packages/debian/metadata.go @@ -46,7 +46,7 @@ var ( // https://www.debian.org/doc/debian-policy/ch-controlfields.html#source namePattern = regexp.MustCompile(`\A[a-z0-9][a-z0-9+-.]+\z`) // https://www.debian.org/doc/debian-policy/ch-controlfields.html#version - versionPattern = regexp.MustCompile(`\A(?:[0-9]:)?[a-zA-Z0-9.+~]+(?:-[a-zA-Z0-9.+-~]+)?\z`) + versionPattern = regexp.MustCompile(`\A(?:(0|[1-9][0-9]*):)?[a-zA-Z0-9.+~]+(?:-[a-zA-Z0-9.+-~]+)?\z`) ) type Package struct { diff --git a/modules/packages/debian/metadata_test.go b/modules/packages/debian/metadata_test.go index a56b131416..fedd276614 100644 --- a/modules/packages/debian/metadata_test.go +++ b/modules/packages/debian/metadata_test.go @@ -176,4 +176,12 @@ func TestParseControlFile(t *testing.T) { assert.Equal(t, []string{"a", "b"}, p.Metadata.Dependencies) assert.Equal(t, full, p.Control) }) + + t.Run("ValidVersions", func(t *testing.T) { + for _, version := range []string{"1.0", "0:1.2", "9:1.0", "10:1.0", "900:1a.2b-x-y_z~1+2"} { + p, err := ParseControlFile(buildContent("testpkg", version, "amd64")) + assert.NoError(t, err, "ParseControlFile with version %q", version) + assert.NotNil(t, p) + } + }) } diff --git a/modules/web/routing/logger.go b/modules/web/routing/logger.go index 3bca9b3420..a6a0e0d517 100644 --- a/modules/web/routing/logger.go +++ b/modules/web/routing/logger.go @@ -104,7 +104,8 @@ func logPrinter(logger log.Logger) func(trigger Event, record *requestRecord) { } logf := logInfo // lower the log level for some specific requests, in most cases these logs are not useful - if strings.HasPrefix(req.RequestURI, "/assets/") /* static assets */ || + if status > 0 && status < 400 && + strings.HasPrefix(req.RequestURI, "/assets/") /* static assets */ || req.RequestURI == "/user/events" /* Server-Sent Events (SSE) handler */ || req.RequestURI == "/api/actions/runner.v1.RunnerService/FetchTask" /* Actions Runner polling */ { logf = logTrace diff --git a/routers/api/v1/repo/migrate.go b/routers/api/v1/repo/migrate.go index c1e0b47d33..17259dc724 100644 --- a/routers/api/v1/repo/migrate.go +++ b/routers/api/v1/repo/migrate.go @@ -4,7 +4,7 @@ package repo import ( - "bytes" + gocontext "context" "errors" "fmt" "net/http" @@ -173,7 +173,7 @@ func Migrate(ctx *context.APIContext) { opts.AWSSecretAccessKey = form.AWSSecretAccessKey } - repo, err := repo_service.CreateRepositoryDirectly(ctx, ctx.Doer, repoOwner, repo_service.CreateRepoOptions{ + createdRepo, err := repo_service.CreateRepositoryDirectly(ctx, ctx.Doer, repoOwner, repo_service.CreateRepoOptions{ Name: opts.RepoName, Description: opts.Description, OriginalURL: form.CloneAddr, @@ -187,35 +187,37 @@ func Migrate(ctx *context.APIContext) { return } - opts.MigrateToRepoID = repo.ID + opts.MigrateToRepoID = createdRepo.ID - defer func() { - if e := recover(); e != nil { - var buf bytes.Buffer - fmt.Fprintf(&buf, "Handler crashed with error: %v", log.Stack(2)) - - err = errors.New(buf.String()) - } - - if err == nil { - notify_service.MigrateRepository(ctx, ctx.Doer, repoOwner, repo) - return - } - - if repo != nil { - if errDelete := repo_service.DeleteRepositoryDirectly(ctx, repo.ID); errDelete != nil { - log.Error("DeleteRepository: %v", errDelete) + doLongTimeMigrate := func(ctx gocontext.Context, doer *user_model.User) (migratedRepo *repo_model.Repository, retErr error) { + defer func() { + if e := recover(); e != nil { + log.Error("MigrateRepository panic: %v\n%s", e, log.Stack(2)) + if errDelete := repo_service.DeleteRepositoryDirectly(ctx, createdRepo.ID); errDelete != nil { + log.Error("Unable to delete repo after MigrateRepository panic: %v", errDelete) + } + retErr = errors.New("MigrateRepository panic") // no idea why it would happen, just legacy code } - } - }() + }() - if repo, err = migrations.MigrateRepository(graceful.GetManager().HammerContext(), ctx.Doer, repoOwner.Name, opts, nil); err != nil { + migratedRepo, err := migrations.MigrateRepository(ctx, doer, repoOwner.Name, opts, nil) + if err != nil { + return nil, err + } + notify_service.MigrateRepository(ctx, doer, repoOwner, migratedRepo) + return migratedRepo, nil + } + + // use a background context, don't cancel the migration even if the client goes away + // HammerContext doesn't seem right (from https://github.com/go-gitea/gitea/pull/9335/files) + // There are other abuses, maybe most HammerContext abuses should be fixed together in the future. + migratedRepo, err := doLongTimeMigrate(graceful.GetManager().HammerContext(), ctx.Doer) + if err != nil { handleMigrateError(ctx, repoOwner, err) return } - log.Trace("Repository migrated: %s/%s", repoOwner.Name, form.RepoName) - ctx.JSON(http.StatusCreated, convert.ToRepo(ctx, repo, access_model.Permission{AccessMode: perm.AccessModeAdmin})) + ctx.JSON(http.StatusCreated, convert.ToRepo(ctx, migratedRepo, access_model.Permission{AccessMode: perm.AccessModeAdmin})) } func handleMigrateError(ctx *context.APIContext, repoOwner *user_model.User, err error) { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index ea3e700b77..f61571231f 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -131,7 +131,7 @@ func getPullInfo(ctx *context.Context) (issue *issues_model.Issue, ok bool) { ctx.Data["Issue"] = issue if !issue.IsPull { - ctx.NotFound(nil) + ctx.Redirect(issue.Link()) return nil, false } diff --git a/services/actions/variables.go b/services/actions/variables.go index 2603f1d461..57e6af1d9b 100644 --- a/services/actions/variables.go +++ b/services/actions/variables.go @@ -5,10 +5,8 @@ package actions import ( "context" - "regexp" actions_model "code.gitea.io/gitea/models/actions" - "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/util" secret_service "code.gitea.io/gitea/services/secrets" ) @@ -18,10 +16,6 @@ func CreateVariable(ctx context.Context, ownerID, repoID int64, name, data, desc return nil, err } - if err := envNameCIRegexMatch(name); err != nil { - return nil, err - } - v, err := actions_model.InsertVariable(ctx, ownerID, repoID, name, util.ReserveLineBreakForTextarea(data), description) if err != nil { return nil, err @@ -35,10 +29,6 @@ func UpdateVariableNameData(ctx context.Context, variable *actions_model.ActionV return false, err } - if err := envNameCIRegexMatch(variable.Name); err != nil { - return false, err - } - variable.Data = util.ReserveLineBreakForTextarea(variable.Data) return actions_model.UpdateVariableCols(ctx, variable, "name", "data", "description") @@ -49,14 +39,6 @@ func DeleteVariableByID(ctx context.Context, variableID int64) error { } func DeleteVariableByName(ctx context.Context, ownerID, repoID int64, name string) error { - if err := secret_service.ValidateName(name); err != nil { - return err - } - - if err := envNameCIRegexMatch(name); err != nil { - return err - } - v, err := GetVariable(ctx, actions_model.FindVariablesOpts{ OwnerID: ownerID, RepoID: repoID, @@ -79,19 +61,3 @@ func GetVariable(ctx context.Context, opts actions_model.FindVariablesOpts) (*ac } return vars[0], nil } - -// some regular expression of `variables` and `secrets` -// reference to: -// https://docs.github.com/en/actions/learn-github-actions/variables#naming-conventions-for-configuration-variables -// https://docs.github.com/en/actions/security-guides/encrypted-secrets#naming-your-secrets -var ( - forbiddenEnvNameCIRx = regexp.MustCompile("(?i)^CI") -) - -func envNameCIRegexMatch(name string) error { - if forbiddenEnvNameCIRx.MatchString(name) { - log.Error("Env Name cannot be ci") - return util.NewInvalidArgumentErrorf("env name cannot be ci") - } - return nil -} diff --git a/services/secrets/secrets.go b/services/secrets/secrets.go index ec6a3cb062..1aec6e146d 100644 --- a/services/secrets/secrets.go +++ b/services/secrets/secrets.go @@ -56,10 +56,6 @@ func DeleteSecretByID(ctx context.Context, ownerID, repoID, secretID int64) erro } func DeleteSecretByName(ctx context.Context, ownerID, repoID int64, name string) error { - if err := ValidateName(name); err != nil { - return err - } - s, err := db.Find[secret_model.Secret](ctx, secret_model.FindSecretsOptions{ OwnerID: ownerID, RepoID: repoID, diff --git a/services/secrets/validation.go b/services/secrets/validation.go index 3db5b96452..d3636ded84 100644 --- a/services/secrets/validation.go +++ b/services/secrets/validation.go @@ -5,21 +5,29 @@ package secrets import ( "regexp" + "strings" + "sync" "code.gitea.io/gitea/modules/util" ) +// https://docs.github.com/en/actions/learn-github-actions/variables#naming-conventions-for-configuration-variables // https://docs.github.com/en/actions/security-guides/encrypted-secrets#naming-your-secrets -var ( - namePattern = regexp.MustCompile("(?i)^[A-Z_][A-Z0-9_]*$") - forbiddenPrefixPattern = regexp.MustCompile("(?i)^GIT(EA|HUB)_") - - ErrInvalidName = util.NewInvalidArgumentErrorf("invalid secret name") -) +var globalVars = sync.OnceValue(func() (ret struct { + namePattern, forbiddenPrefixPattern *regexp.Regexp +}, +) { + ret.namePattern = regexp.MustCompile("(?i)^[A-Z_][A-Z0-9_]*$") + ret.forbiddenPrefixPattern = regexp.MustCompile("(?i)^GIT(EA|HUB)_") + return ret +}) func ValidateName(name string) error { - if !namePattern.MatchString(name) || forbiddenPrefixPattern.MatchString(name) { - return ErrInvalidName + vars := globalVars() + if !vars.namePattern.MatchString(name) || + vars.forbiddenPrefixPattern.MatchString(name) || + strings.EqualFold(name, "CI") /* CI is always set to true in GitHub Actions*/ { + return util.NewInvalidArgumentErrorf("invalid variable or secret name") } return nil } diff --git a/services/secrets/validation_test.go b/services/secrets/validation_test.go new file mode 100644 index 0000000000..6ee47513dc --- /dev/null +++ b/services/secrets/validation_test.go @@ -0,0 +1,29 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package secrets + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestValidateName(t *testing.T) { + cases := []struct { + name string + valid bool + }{ + {"FOO", true}, + {"FOO1_BAR2", true}, + {"_FOO", true}, // really? why support this + {"1FOO", false}, + {"giteA_xx", false}, + {"githuB_xx", false}, + {"cI", false}, + } + for _, c := range cases { + err := ValidateName(c.name) + assert.Equal(t, c.valid, err == nil, "ValidateName(%q)", c.name) + } +} diff --git a/templates/mail/repo/release.tmpl b/templates/mail/repo/release.tmpl index 3172e6a19d..b306ddf5b9 100644 --- a/templates/mail/repo/release.tmpl +++ b/templates/mail/repo/release.tmpl @@ -32,10 +32,10 @@
    {{if not .DisableDownloadSourceArchives}}
  • - {{.locale.Tr "mail.release.download.zip"}} + {{.locale.Tr "mail.release.download.zip"}}
  • - {{.locale.Tr "mail.release.download.targz"}} + {{.locale.Tr "mail.release.download.targz"}}
    • {{end}} {{if .Release.Attachments}} diff --git a/tests/integration/api_repo_secrets_test.go b/tests/integration/api_repo_secrets_test.go index 2059aff484..dc37e2beb6 100644 --- a/tests/integration/api_repo_secrets_test.go +++ b/tests/integration/api_repo_secrets_test.go @@ -131,9 +131,5 @@ func TestAPIRepoSecrets(t *testing.T) { req = NewRequest(t, "DELETE", url). AddTokenAuth(token) MakeRequest(t, req, http.StatusNotFound) - - req = NewRequest(t, "DELETE", fmt.Sprintf("/api/v1/repos/%s/actions/secrets/000", repo.FullName())). - AddTokenAuth(token) - MakeRequest(t, req, http.StatusBadRequest) }) } diff --git a/tests/integration/api_user_secrets_test.go b/tests/integration/api_user_secrets_test.go index 10024ac090..0d2439c3cb 100644 --- a/tests/integration/api_user_secrets_test.go +++ b/tests/integration/api_user_secrets_test.go @@ -92,9 +92,5 @@ func TestAPIUserSecrets(t *testing.T) { req = NewRequest(t, "DELETE", url). AddTokenAuth(token) MakeRequest(t, req, http.StatusNotFound) - - req = NewRequest(t, "DELETE", "/api/v1/user/actions/secrets/000"). - AddTokenAuth(token) - MakeRequest(t, req, http.StatusBadRequest) }) } diff --git a/tests/integration/issue_test.go b/tests/integration/issue_test.go index 613353e55c..0da08796dc 100644 --- a/tests/integration/issue_test.go +++ b/tests/integration/issue_test.go @@ -497,9 +497,13 @@ func TestIssueRedirect(t *testing.T) { resp = session.MakeRequest(t, req, http.StatusSeeOther) assert.Equal(t, "/user2/repo1/pulls/2", test.RedirectURL(resp)) - repoUnit := unittest.AssertExistsAndLoadBean(t, &repo_model.RepoUnit{RepoID: 1, Type: unit.TypeIssues}) + // by the way, test PR redirection + req = NewRequest(t, "GET", "/user2/repo1/pulls/1") + resp = session.MakeRequest(t, req, http.StatusSeeOther) + assert.Equal(t, "/user2/repo1/issues/1", test.RedirectURL(resp)) - // disable issue unit, it will be reset + // disable issue unit + repoUnit := unittest.AssertExistsAndLoadBean(t, &repo_model.RepoUnit{RepoID: 1, Type: unit.TypeIssues}) _, err := db.DeleteByID[repo_model.RepoUnit](t.Context(), repoUnit.ID) assert.NoError(t, err) From c30d74d0f9d088e3ede41f9fd4b48a0c0b0af3f0 Mon Sep 17 00:00:00 2001 From: Bryan Mutai Date: Sun, 19 Oct 2025 13:19:12 +0300 Subject: [PATCH 02/29] feat(diff): Enable commenting on expanded lines in PR diffs (#35662) Fixes #32257 /claim #32257 Implemented commenting on unchanged lines in Pull Request diffs, lines are accessed by expanding the diff preview. Comments also appear in the "Files Changed" tab on the unchanged lines where they were placed. --------- Co-authored-by: wxiaoguang --- routers/web/repo/commit.go | 23 +- routers/web/repo/compare.go | 79 +++- routers/web/repo/compare_test.go | 40 ++ routers/web/repo/pull.go | 6 + services/gitdiff/gitdiff.go | 179 +++++++-- services/gitdiff/gitdiff_test.go | 343 ++++++++++++++++++ services/pull/review.go | 10 + templates/repo/diff/blob_excerpt.tmpl | 90 ++--- templates/repo/diff/section_split.tmpl | 24 +- templates/repo/diff/section_unified.tmpl | 26 +- .../repo/issue/view_content/conversation.tmpl | 5 + web_src/css/review.css | 21 +- web_src/js/features/repo-diff.ts | 45 ++- 13 files changed, 753 insertions(+), 138 deletions(-) create mode 100644 routers/web/repo/compare_test.go diff --git a/routers/web/repo/commit.go b/routers/web/repo/commit.go index 1a86a62fae..0383e4ca9e 100644 --- a/routers/web/repo/commit.go +++ b/routers/web/repo/commit.go @@ -276,20 +276,24 @@ func Diff(ctx *context.Context) { userName := ctx.Repo.Owner.Name repoName := ctx.Repo.Repository.Name commitID := ctx.PathParam("sha") - var ( - gitRepo *git.Repository - err error - ) + + diffBlobExcerptData := &gitdiff.DiffBlobExcerptData{ + BaseLink: ctx.Repo.RepoLink + "/blob_excerpt", + DiffStyle: ctx.FormString("style"), + AfterCommitID: commitID, + } + gitRepo := ctx.Repo.GitRepo + var gitRepoStore gitrepo.Repository = ctx.Repo.Repository if ctx.Data["PageIsWiki"] != nil { - gitRepo, err = gitrepo.OpenRepository(ctx, ctx.Repo.Repository.WikiStorageRepo()) + var err error + gitRepoStore = ctx.Repo.Repository.WikiStorageRepo() + gitRepo, err = gitrepo.RepositoryFromRequestContextOrOpen(ctx, gitRepoStore) if err != nil { ctx.ServerError("Repo.GitRepo.GetCommit", err) return } - defer gitRepo.Close() - } else { - gitRepo = ctx.Repo.GitRepo + diffBlobExcerptData.BaseLink = ctx.Repo.RepoLink + "/wiki/blob_excerpt" } commit, err := gitRepo.GetCommit(commitID) @@ -324,7 +328,7 @@ func Diff(ctx *context.Context) { ctx.NotFound(err) return } - diffShortStat, err := gitdiff.GetDiffShortStat(ctx, ctx.Repo.Repository, gitRepo, "", commitID) + diffShortStat, err := gitdiff.GetDiffShortStat(ctx, gitRepoStore, gitRepo, "", commitID) if err != nil { ctx.ServerError("GetDiffShortStat", err) return @@ -360,6 +364,7 @@ func Diff(ctx *context.Context) { ctx.Data["Title"] = commit.Summary() + " · " + base.ShortSha(commitID) ctx.Data["Commit"] = commit ctx.Data["Diff"] = diff + ctx.Data["DiffBlobExcerptData"] = diffBlobExcerptData if !fileOnly { diffTree, err := gitdiff.GetDiffTree(ctx, gitRepo, false, parentCommitID, commitID) diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 9262703078..08bd0a7e74 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -14,6 +14,7 @@ import ( "net/http" "net/url" "path/filepath" + "sort" "strings" "code.gitea.io/gitea/models/db" @@ -43,6 +44,7 @@ import ( "code.gitea.io/gitea/services/context/upload" "code.gitea.io/gitea/services/gitdiff" pull_service "code.gitea.io/gitea/services/pull" + user_service "code.gitea.io/gitea/services/user" ) const ( @@ -638,6 +640,11 @@ func PrepareCompareDiff( } ctx.Data["DiffShortStat"] = diffShortStat ctx.Data["Diff"] = diff + ctx.Data["DiffBlobExcerptData"] = &gitdiff.DiffBlobExcerptData{ + BaseLink: ci.HeadRepo.Link() + "/blob_excerpt", + DiffStyle: ctx.FormString("style"), + AfterCommitID: headCommitID, + } ctx.Data["DiffNotAvailable"] = diffShortStat.NumFiles == 0 if !fileOnly { @@ -865,6 +872,28 @@ func CompareDiff(ctx *context.Context) { ctx.HTML(http.StatusOK, tplCompare) } +// attachCommentsToLines attaches comments to their corresponding diff lines +func attachCommentsToLines(section *gitdiff.DiffSection, lineComments map[int64][]*issues_model.Comment) { + for _, line := range section.Lines { + if comments, ok := lineComments[int64(line.LeftIdx*-1)]; ok { + line.Comments = append(line.Comments, comments...) + } + if comments, ok := lineComments[int64(line.RightIdx)]; ok { + line.Comments = append(line.Comments, comments...) + } + sort.SliceStable(line.Comments, func(i, j int) bool { + return line.Comments[i].CreatedUnix < line.Comments[j].CreatedUnix + }) + } +} + +// attachHiddenCommentIDs calculates and attaches hidden comment IDs to expand buttons +func attachHiddenCommentIDs(section *gitdiff.DiffSection, lineComments map[int64][]*issues_model.Comment) { + for _, line := range section.Lines { + gitdiff.FillHiddenCommentIDsForDiffLine(line, lineComments) + } +} + // ExcerptBlob render blob excerpt contents func ExcerptBlob(ctx *context.Context) { commitID := ctx.PathParam("sha") @@ -874,19 +903,26 @@ func ExcerptBlob(ctx *context.Context) { idxRight := ctx.FormInt("right") leftHunkSize := ctx.FormInt("left_hunk_size") rightHunkSize := ctx.FormInt("right_hunk_size") - anchor := ctx.FormString("anchor") direction := ctx.FormString("direction") filePath := ctx.FormString("path") gitRepo := ctx.Repo.GitRepo + + diffBlobExcerptData := &gitdiff.DiffBlobExcerptData{ + BaseLink: ctx.Repo.RepoLink + "/blob_excerpt", + DiffStyle: ctx.FormString("style"), + AfterCommitID: commitID, + } + if ctx.Data["PageIsWiki"] == true { var err error - gitRepo, err = gitrepo.OpenRepository(ctx, ctx.Repo.Repository.WikiStorageRepo()) + gitRepo, err = gitrepo.RepositoryFromRequestContextOrOpen(ctx, ctx.Repo.Repository.WikiStorageRepo()) if err != nil { ctx.ServerError("OpenRepository", err) return } - defer gitRepo.Close() + diffBlobExcerptData.BaseLink = ctx.Repo.RepoLink + "/wiki/blob_excerpt" } + chunkSize := gitdiff.BlobExcerptChunkSize commit, err := gitRepo.GetCommit(commitID) if err != nil { @@ -947,10 +983,43 @@ func ExcerptBlob(ctx *context.Context) { section.Lines = append(section.Lines, lineSection) } } + + diffBlobExcerptData.PullIssueIndex = ctx.FormInt64("pull_issue_index") + if diffBlobExcerptData.PullIssueIndex > 0 { + if !ctx.Repo.CanRead(unit.TypePullRequests) { + ctx.NotFound(nil) + return + } + + issue, err := issues_model.GetIssueByIndex(ctx, ctx.Repo.Repository.ID, diffBlobExcerptData.PullIssueIndex) + if err != nil { + log.Error("GetIssueByIndex error: %v", err) + } else if issue.IsPull { + // FIXME: DIFF-CONVERSATION-DATA: the following data assignment is fragile + ctx.Data["Issue"] = issue + ctx.Data["CanBlockUser"] = func(blocker, blockee *user_model.User) bool { + return user_service.CanBlockUser(ctx, ctx.Doer, blocker, blockee) + } + // and "diff/comment_form.tmpl" (reply comment) needs them + ctx.Data["PageIsPullFiles"] = true + ctx.Data["AfterCommitID"] = diffBlobExcerptData.AfterCommitID + + allComments, err := issues_model.FetchCodeComments(ctx, issue, ctx.Doer, ctx.FormBool("show_outdated")) + if err != nil { + log.Error("FetchCodeComments error: %v", err) + } else { + if lineComments, ok := allComments[filePath]; ok { + attachCommentsToLines(section, lineComments) + attachHiddenCommentIDs(section, lineComments) + } + } + } + } + ctx.Data["section"] = section ctx.Data["FileNameHash"] = git.HashFilePathForWebUI(filePath) - ctx.Data["AfterCommitID"] = commitID - ctx.Data["Anchor"] = anchor + ctx.Data["DiffBlobExcerptData"] = diffBlobExcerptData + ctx.HTML(http.StatusOK, tplBlobExcerpt) } diff --git a/routers/web/repo/compare_test.go b/routers/web/repo/compare_test.go new file mode 100644 index 0000000000..61472dc71e --- /dev/null +++ b/routers/web/repo/compare_test.go @@ -0,0 +1,40 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package repo + +import ( + "testing" + + issues_model "code.gitea.io/gitea/models/issues" + "code.gitea.io/gitea/services/gitdiff" + + "github.com/stretchr/testify/assert" +) + +func TestAttachCommentsToLines(t *testing.T) { + section := &gitdiff.DiffSection{ + Lines: []*gitdiff.DiffLine{ + {LeftIdx: 5, RightIdx: 10}, + {LeftIdx: 6, RightIdx: 11}, + }, + } + + lineComments := map[int64][]*issues_model.Comment{ + -5: {{ID: 100, CreatedUnix: 1000}}, // left side comment + 10: {{ID: 200, CreatedUnix: 2000}}, // right side comment + 11: {{ID: 300, CreatedUnix: 1500}, {ID: 301, CreatedUnix: 2500}}, // multiple comments + } + + attachCommentsToLines(section, lineComments) + + // First line should have left and right comments + assert.Len(t, section.Lines[0].Comments, 2) + assert.Equal(t, int64(100), section.Lines[0].Comments[0].ID) + assert.Equal(t, int64(200), section.Lines[0].Comments[1].ID) + + // Second line should have two comments, sorted by creation time + assert.Len(t, section.Lines[1].Comments, 2) + assert.Equal(t, int64(300), section.Lines[1].Comments[0].ID) + assert.Equal(t, int64(301), section.Lines[1].Comments[1].ID) +} diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index f61571231f..cfe9a7ae02 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -827,6 +827,12 @@ func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) { } ctx.Data["Diff"] = diff + ctx.Data["DiffBlobExcerptData"] = &gitdiff.DiffBlobExcerptData{ + BaseLink: ctx.Repo.RepoLink + "/blob_excerpt", + PullIssueIndex: pull.Index, + DiffStyle: ctx.FormString("style"), + AfterCommitID: afterCommitID, + } ctx.Data["DiffNotAvailable"] = diffShortStat.NumFiles == 0 if ctx.IsSigned && ctx.Doer != nil { diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index db0f565b52..96aea8308c 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -22,19 +22,21 @@ import ( git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" pull_model "code.gitea.io/gitea/models/pull" - repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/analyze" + "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/charset" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/git/attribute" "code.gitea.io/gitea/modules/git/gitcmd" "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/highlight" + "code.gitea.io/gitea/modules/htmlutil" "code.gitea.io/gitea/modules/lfs" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/svg" "code.gitea.io/gitea/modules/translation" "code.gitea.io/gitea/modules/util" @@ -67,18 +69,6 @@ const ( DiffFileCopy ) -// DiffLineExpandDirection represents the DiffLineSection expand direction -type DiffLineExpandDirection uint8 - -// DiffLineExpandDirection possible values. -const ( - DiffLineExpandNone DiffLineExpandDirection = iota + 1 - DiffLineExpandSingle - DiffLineExpandUpDown - DiffLineExpandUp - DiffLineExpandDown -) - // DiffLine represents a line difference in a DiffSection. type DiffLine struct { LeftIdx int // line number, 1-based @@ -99,6 +89,8 @@ type DiffLineSectionInfo struct { RightIdx int LeftHunkSize int RightHunkSize int + + HiddenCommentIDs []int64 // IDs of hidden comments in this section } // DiffHTMLOperation is the HTML version of diffmatchpatch.Diff @@ -153,8 +145,7 @@ func (d *DiffLine) GetLineTypeMarker() string { return "" } -// GetBlobExcerptQuery builds query string to get blob excerpt -func (d *DiffLine) GetBlobExcerptQuery() string { +func (d *DiffLine) getBlobExcerptQuery() string { query := fmt.Sprintf( "last_left=%d&last_right=%d&"+ "left=%d&right=%d&"+ @@ -167,19 +158,88 @@ func (d *DiffLine) GetBlobExcerptQuery() string { return query } -// GetExpandDirection gets DiffLineExpandDirection -func (d *DiffLine) GetExpandDirection() DiffLineExpandDirection { +func (d *DiffLine) getExpandDirection() string { if d.Type != DiffLineSection || d.SectionInfo == nil || d.SectionInfo.LeftIdx-d.SectionInfo.LastLeftIdx <= 1 || d.SectionInfo.RightIdx-d.SectionInfo.LastRightIdx <= 1 { - return DiffLineExpandNone + return "" } if d.SectionInfo.LastLeftIdx <= 0 && d.SectionInfo.LastRightIdx <= 0 { - return DiffLineExpandUp + return "up" } else if d.SectionInfo.RightIdx-d.SectionInfo.LastRightIdx > BlobExcerptChunkSize && d.SectionInfo.RightHunkSize > 0 { - return DiffLineExpandUpDown + return "updown" } else if d.SectionInfo.LeftHunkSize <= 0 && d.SectionInfo.RightHunkSize <= 0 { - return DiffLineExpandDown + return "down" } - return DiffLineExpandSingle + return "single" +} + +type DiffBlobExcerptData struct { + BaseLink string + IsWikiRepo bool + PullIssueIndex int64 + DiffStyle string + AfterCommitID string +} + +func (d *DiffLine) RenderBlobExcerptButtons(fileNameHash string, data *DiffBlobExcerptData) template.HTML { + dataHiddenCommentIDs := strings.Join(base.Int64sToStrings(d.SectionInfo.HiddenCommentIDs), ",") + anchor := fmt.Sprintf("diff-%sK%d", fileNameHash, d.SectionInfo.RightIdx) + + makeButton := func(direction, svgName string) template.HTML { + style := util.IfZero(data.DiffStyle, "unified") + link := data.BaseLink + "/" + data.AfterCommitID + fmt.Sprintf("?style=%s&direction=%s&anchor=%s", url.QueryEscape(style), direction, url.QueryEscape(anchor)) + "&" + d.getBlobExcerptQuery() + if data.PullIssueIndex > 0 { + link += fmt.Sprintf("&pull_issue_index=%d", data.PullIssueIndex) + } + return htmlutil.HTMLFormat( + ``, + link, dataHiddenCommentIDs, svg.RenderHTML(svgName), + ) + } + var content template.HTML + + if len(d.SectionInfo.HiddenCommentIDs) > 0 { + tooltip := fmt.Sprintf("%d hidden comment(s)", len(d.SectionInfo.HiddenCommentIDs)) + content += htmlutil.HTMLFormat(`%d`, tooltip, len(d.SectionInfo.HiddenCommentIDs)) + } + + expandDirection := d.getExpandDirection() + if expandDirection == "up" || expandDirection == "updown" { + content += makeButton("up", "octicon-fold-up") + } + if expandDirection == "updown" || expandDirection == "down" { + content += makeButton("down", "octicon-fold-down") + } + if expandDirection == "single" { + content += makeButton("single", "octicon-fold") + } + return htmlutil.HTMLFormat(`
    %s
    `, expandDirection, content) +} + +// FillHiddenCommentIDsForDiffLine finds comment IDs that are in the hidden range of an expand button +func FillHiddenCommentIDsForDiffLine(line *DiffLine, lineComments map[int64][]*issues_model.Comment) { + if line.Type != DiffLineSection { + return + } + + var hiddenCommentIDs []int64 + for commentLineNum, comments := range lineComments { + if commentLineNum < 0 { + // ATTENTION: BLOB-EXCERPT-COMMENT-RIGHT: skip left-side, unchanged lines always use "right (proposed)" side for comments + continue + } + lineNum := int(commentLineNum) + isEndOfFileExpansion := line.SectionInfo.RightHunkSize == 0 + inRange := lineNum > line.SectionInfo.LastRightIdx && + (isEndOfFileExpansion && lineNum <= line.SectionInfo.RightIdx || + !isEndOfFileExpansion && lineNum < line.SectionInfo.RightIdx) + + if inRange { + for _, comment := range comments { + hiddenCommentIDs = append(hiddenCommentIDs, comment.ID) + } + } + } + line.SectionInfo.HiddenCommentIDs = hiddenCommentIDs } func getDiffLineSectionInfo(treePath, line string, lastLeftIdx, lastRightIdx int) *DiffLineSectionInfo { @@ -485,6 +545,8 @@ func (diff *Diff) LoadComments(ctx context.Context, issue *issues_model.Issue, c sort.SliceStable(line.Comments, func(i, j int) bool { return line.Comments[i].CreatedUnix < line.Comments[j].CreatedUnix }) + // Mark expand buttons that have comments in hidden lines + FillHiddenCommentIDsForDiffLine(line, lineCommits) } } } @@ -1281,7 +1343,7 @@ type DiffShortStat struct { NumFiles, TotalAddition, TotalDeletion int } -func GetDiffShortStat(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Repository, beforeCommitID, afterCommitID string) (*DiffShortStat, error) { +func GetDiffShortStat(ctx context.Context, repoStorage gitrepo.Repository, gitRepo *git.Repository, beforeCommitID, afterCommitID string) (*DiffShortStat, error) { afterCommit, err := gitRepo.GetCommit(afterCommitID) if err != nil { return nil, err @@ -1293,7 +1355,7 @@ func GetDiffShortStat(ctx context.Context, repo *repo_model.Repository, gitRepo } diff := &DiffShortStat{} - diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = gitrepo.GetDiffShortStatByCmdArgs(ctx, repo, nil, actualBeforeCommitID.String(), afterCommitID) + diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = gitrepo.GetDiffShortStatByCmdArgs(ctx, repoStorage, nil, actualBeforeCommitID.String(), afterCommitID) if err != nil { return nil, err } @@ -1386,6 +1448,75 @@ func CommentAsDiff(ctx context.Context, c *issues_model.Comment) (*Diff, error) return diff, nil } +// GeneratePatchForUnchangedLine creates a patch showing code context for an unchanged line +func GeneratePatchForUnchangedLine(gitRepo *git.Repository, commitID, treePath string, line int64, contextLines int) (string, error) { + commit, err := gitRepo.GetCommit(commitID) + if err != nil { + return "", fmt.Errorf("GetCommit: %w", err) + } + + entry, err := commit.GetTreeEntryByPath(treePath) + if err != nil { + return "", fmt.Errorf("GetTreeEntryByPath: %w", err) + } + + blob := entry.Blob() + dataRc, err := blob.DataAsync() + if err != nil { + return "", fmt.Errorf("DataAsync: %w", err) + } + defer dataRc.Close() + + return generatePatchForUnchangedLineFromReader(dataRc, treePath, line, contextLines) +} + +// generatePatchForUnchangedLineFromReader is the testable core logic that generates a patch from a reader +func generatePatchForUnchangedLineFromReader(reader io.Reader, treePath string, line int64, contextLines int) (string, error) { + // Calculate line range (commented line + lines above it) + commentLine := int(line) + if line < 0 { + commentLine = int(-line) + } + startLine := max(commentLine-contextLines, 1) + endLine := commentLine + + // Read only the needed lines efficiently + scanner := bufio.NewScanner(reader) + currentLine := 0 + var lines []string + for scanner.Scan() { + currentLine++ + if currentLine >= startLine && currentLine <= endLine { + lines = append(lines, scanner.Text()) + } + if currentLine > endLine { + break + } + } + if err := scanner.Err(); err != nil { + return "", fmt.Errorf("scanner error: %w", err) + } + + if len(lines) == 0 { + return "", fmt.Errorf("no lines found in range %d-%d", startLine, endLine) + } + + // Generate synthetic patch + var patchBuilder strings.Builder + patchBuilder.WriteString(fmt.Sprintf("diff --git a/%s b/%s\n", treePath, treePath)) + patchBuilder.WriteString(fmt.Sprintf("--- a/%s\n", treePath)) + patchBuilder.WriteString(fmt.Sprintf("+++ b/%s\n", treePath)) + patchBuilder.WriteString(fmt.Sprintf("@@ -%d,%d +%d,%d @@\n", startLine, len(lines), startLine, len(lines))) + + for _, lineContent := range lines { + patchBuilder.WriteString(" ") + patchBuilder.WriteString(lineContent) + patchBuilder.WriteString("\n") + } + + return patchBuilder.String(), nil +} + // CommentMustAsDiff executes AsDiff and logs the error instead of returning func CommentMustAsDiff(ctx context.Context, c *issues_model.Comment) *Diff { if c == nil { diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index 7b64b6b5f8..51fb9b58d6 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -640,3 +640,346 @@ func TestNoCrashes(t *testing.T) { ParsePatch(t.Context(), setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(testcase.gitdiff), "") } } + +func TestGeneratePatchForUnchangedLineFromReader(t *testing.T) { + tests := []struct { + name string + content string + treePath string + line int64 + contextLines int + want string + wantErr bool + }{ + { + name: "single line with context", + content: "line1\nline2\nline3\nline4\nline5\n", + treePath: "test.txt", + line: 3, + contextLines: 1, + want: `diff --git a/test.txt b/test.txt +--- a/test.txt ++++ b/test.txt +@@ -2,2 +2,2 @@ + line2 + line3 +`, + }, + { + name: "negative line number (left side)", + content: "line1\nline2\nline3\nline4\nline5\n", + treePath: "test.txt", + line: -3, + contextLines: 1, + want: `diff --git a/test.txt b/test.txt +--- a/test.txt ++++ b/test.txt +@@ -2,2 +2,2 @@ + line2 + line3 +`, + }, + { + name: "line near start of file", + content: "line1\nline2\nline3\n", + treePath: "test.txt", + line: 2, + contextLines: 5, + want: `diff --git a/test.txt b/test.txt +--- a/test.txt ++++ b/test.txt +@@ -1,2 +1,2 @@ + line1 + line2 +`, + }, + { + name: "first line with context", + content: "line1\nline2\nline3\n", + treePath: "test.txt", + line: 1, + contextLines: 3, + want: `diff --git a/test.txt b/test.txt +--- a/test.txt ++++ b/test.txt +@@ -1,1 +1,1 @@ + line1 +`, + }, + { + name: "zero context lines", + content: "line1\nline2\nline3\n", + treePath: "test.txt", + line: 2, + contextLines: 0, + want: `diff --git a/test.txt b/test.txt +--- a/test.txt ++++ b/test.txt +@@ -2,1 +2,1 @@ + line2 +`, + }, + { + name: "multi-line context", + content: "package main\n\nfunc main() {\n fmt.Println(\"Hello\")\n}\n", + treePath: "main.go", + line: 4, + contextLines: 2, + want: `diff --git a/main.go b/main.go +--- a/main.go ++++ b/main.go +@@ -2,3 +2,3 @@ + + func main() { + fmt.Println("Hello") +`, + }, + { + name: "empty file", + content: "", + treePath: "empty.txt", + line: 1, + contextLines: 1, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reader := strings.NewReader(tt.content) + got, err := generatePatchForUnchangedLineFromReader(reader, tt.treePath, tt.line, tt.contextLines) + if tt.wantErr { + assert.Error(t, err) + } else { + require.NoError(t, err) + assert.Equal(t, strings.ReplaceAll(tt.want, "", " "), got) + } + }) + } +} + +func TestCalculateHiddenCommentIDsForLine(t *testing.T) { + tests := []struct { + name string + line *DiffLine + lineComments map[int64][]*issues_model.Comment + expected []int64 + }{ + { + name: "comments in hidden range", + line: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + LastRightIdx: 10, + RightIdx: 20, + }, + }, + lineComments: map[int64][]*issues_model.Comment{ + 15: {{ID: 100}, {ID: 101}}, + 12: {{ID: 102}}, + }, + expected: []int64{100, 101, 102}, + }, + { + name: "comments outside hidden range", + line: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + LastRightIdx: 10, + RightIdx: 20, + }, + }, + lineComments: map[int64][]*issues_model.Comment{ + 5: {{ID: 100}}, + 25: {{ID: 101}}, + }, + expected: nil, + }, + { + name: "negative line numbers (left side)", + line: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + LastRightIdx: 10, + RightIdx: 20, + }, + }, + lineComments: map[int64][]*issues_model.Comment{ + -15: {{ID: 100}}, // Left-side comment, should NOT be counted + 15: {{ID: 101}}, // Right-side comment, should be counted + }, + expected: []int64{101}, // Only right-side comment + }, + { + name: "boundary conditions - normal expansion (both boundaries exclusive)", + line: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + LastRightIdx: 10, + RightIdx: 20, + RightHunkSize: 5, // Normal case: next section has content + }, + }, + lineComments: map[int64][]*issues_model.Comment{ + 10: {{ID: 100}}, // at LastRightIdx (visible line), should NOT be included + 20: {{ID: 101}}, // at RightIdx (visible line), should NOT be included + 11: {{ID: 102}}, // just inside range, should be included + 19: {{ID: 103}}, // just inside range, should be included + }, + expected: []int64{102, 103}, + }, + { + name: "boundary conditions - end of file expansion (RightIdx inclusive)", + line: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + LastRightIdx: 54, + RightIdx: 70, + RightHunkSize: 0, // End of file: no more content after + }, + }, + lineComments: map[int64][]*issues_model.Comment{ + 54: {{ID: 54}}, // at LastRightIdx (visible line), should NOT be included + 70: {{ID: 70}}, // at RightIdx (last hidden line), SHOULD be included + 60: {{ID: 60}}, // inside range, should be included + }, + expected: []int64{60, 70}, // Lines 60 and 70 are hidden + }, + { + name: "real-world scenario - start of file with hunk", + line: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + LastRightIdx: 0, // No previous visible section + RightIdx: 26, // Line 26 is first visible line of hunk + RightHunkSize: 9, // Normal hunk with content + }, + }, + lineComments: map[int64][]*issues_model.Comment{ + 1: {{ID: 1}}, // Line 1 is hidden + 26: {{ID: 26}}, // Line 26 is visible (hunk start) - should NOT be hidden + 10: {{ID: 10}}, // Line 10 is hidden + 15: {{ID: 15}}, // Line 15 is hidden + }, + expected: []int64{1, 10, 15}, // Lines 1, 10, 15 are hidden; line 26 is visible + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + FillHiddenCommentIDsForDiffLine(tt.line, tt.lineComments) + assert.ElementsMatch(t, tt.expected, tt.line.SectionInfo.HiddenCommentIDs) + }) + } +} + +func TestDiffLine_RenderBlobExcerptButtons(t *testing.T) { + tests := []struct { + name string + line *DiffLine + fileNameHash string + data *DiffBlobExcerptData + expectContains []string + expectNotContain []string + }{ + { + name: "expand up button with hidden comments", + line: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + LastRightIdx: 0, + RightIdx: 26, + LeftIdx: 26, + LastLeftIdx: 0, + LeftHunkSize: 0, + RightHunkSize: 0, + HiddenCommentIDs: []int64{100}, + }, + }, + fileNameHash: "abc123", + data: &DiffBlobExcerptData{ + BaseLink: "/repo/blob_excerpt", + AfterCommitID: "commit123", + DiffStyle: "unified", + }, + expectContains: []string{ + "octicon-fold-up", + "direction=up", + "code-comment-more", + "1 hidden comment(s)", + }, + }, + { + name: "expand up and down buttons with pull request", + line: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + LastRightIdx: 10, + RightIdx: 50, + LeftIdx: 10, + LastLeftIdx: 5, + LeftHunkSize: 5, + RightHunkSize: 5, + HiddenCommentIDs: []int64{200, 201}, + }, + }, + fileNameHash: "def456", + data: &DiffBlobExcerptData{ + BaseLink: "/repo/blob_excerpt", + AfterCommitID: "commit456", + DiffStyle: "split", + PullIssueIndex: 42, + }, + expectContains: []string{ + "octicon-fold-down", + "octicon-fold-up", + "direction=down", + "direction=up", + `data-hidden-comment-ids=",200,201,"`, // use leading and trailing commas to ensure exact match by CSS selector `attr*=",id,"` + "pull_issue_index=42", + "2 hidden comment(s)", + }, + }, + { + name: "no hidden comments", + line: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + LastRightIdx: 10, + RightIdx: 20, + LeftIdx: 10, + LastLeftIdx: 5, + LeftHunkSize: 5, + RightHunkSize: 5, + HiddenCommentIDs: nil, + }, + }, + fileNameHash: "ghi789", + data: &DiffBlobExcerptData{ + BaseLink: "/repo/blob_excerpt", + AfterCommitID: "commit789", + }, + expectContains: []string{ + "code-expander-button", + }, + expectNotContain: []string{ + "code-comment-more", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := tt.line.RenderBlobExcerptButtons(tt.fileNameHash, tt.data) + resultStr := string(result) + + for _, expected := range tt.expectContains { + assert.Contains(t, resultStr, expected, "Expected to contain: %s", expected) + } + + for _, notExpected := range tt.expectNotContain { + assert.NotContains(t, resultStr, notExpected, "Expected NOT to contain: %s", notExpected) + } + }) + } +} diff --git a/services/pull/review.go b/services/pull/review.go index 357a816593..3977e351ca 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -22,6 +22,7 @@ import ( "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" + "code.gitea.io/gitea/services/gitdiff" notify_service "code.gitea.io/gitea/services/notify" ) @@ -283,6 +284,15 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo log.Error("Error whilst generating patch: %v", err) return nil, err } + + // If patch is still empty (unchanged line), generate code context + if patch == "" && commitID != "" { + patch, err = gitdiff.GeneratePatchForUnchangedLine(gitRepo, commitID, treePath, line, setting.UI.CodeCommentLines) + if err != nil { + // Log the error but don't fail comment creation + log.Debug("Unable to generate patch for unchanged line (file=%s, line=%d, commit=%s): %v", treePath, line, commitID, err) + } + } } return issues_model.CreateComment(ctx, &issues_model.CreateCommentOptions{ Type: issues_model.CommentTypeCode, diff --git a/templates/repo/diff/blob_excerpt.tmpl b/templates/repo/diff/blob_excerpt.tmpl index 4089d8fb33..c9aac6d61d 100644 --- a/templates/repo/diff/blob_excerpt.tmpl +++ b/templates/repo/diff/blob_excerpt.tmpl @@ -1,28 +1,10 @@ -{{$blobExcerptLink := print $.RepoLink (Iif $.PageIsWiki "/wiki" "") "/blob_excerpt/" (PathEscape $.AfterCommitID) (QueryBuild "?" "anchor" $.Anchor)}} +{{$diffBlobExcerptData := .DiffBlobExcerptData}} +{{$canCreateComment := and ctx.RootData.SignedUserID $diffBlobExcerptData.PullIssueIndex}} {{if $.IsSplitStyle}} {{range $k, $line := $.section.Lines}} - + {{if eq .GetType 4}} - {{$expandDirection := $line.GetExpandDirection}} - -
    - {{if or (eq $expandDirection 3) (eq $expandDirection 5)}} - - {{end}} - {{if or (eq $expandDirection 3) (eq $expandDirection 4)}} - - {{end}} - {{if eq $expandDirection 2}} - - {{end}} -
    - + {{$line.RenderBlobExcerptButtons $.FileNameHash $diffBlobExcerptData}} {{- $inlineDiff := $.section.GetComputedInlineDiffFor $line ctx.Locale -}} {{- template "repo/diff/section_code" dict "diff" $inlineDiff -}} @@ -33,6 +15,12 @@ {{if and $line.LeftIdx $inlineDiff.EscapeStatus.Escaped}}{{end}} {{if $line.LeftIdx}}{{end}} + {{/* ATTENTION: BLOB-EXCERPT-COMMENT-RIGHT: here it intentially use "right" side to comment, because the backend code depends on the assumption that the comment only happens on right side*/}} + {{- if and $canCreateComment $line.RightIdx -}} + + {{- end -}} {{- if $line.LeftIdx -}} {{- template "repo/diff/section_code" dict "diff" $inlineDiff -}} {{- else -}} @@ -43,6 +31,11 @@ {{if and $line.RightIdx $inlineDiff.EscapeStatus.Escaped}}{{end}} {{if $line.RightIdx}}{{end}} + {{- if and $canCreateComment $line.RightIdx -}} + + {{- end -}} {{- if $line.RightIdx -}} {{- template "repo/diff/section_code" dict "diff" $inlineDiff -}} {{- else -}} @@ -51,31 +44,26 @@ {{end}} + {{if $line.Comments}} + + + {{if eq $line.GetCommentSide "previous"}} + {{template "repo/diff/conversation" dict "." $ "comments" $line.Comments}} + {{end}} + + + {{if eq $line.GetCommentSide "proposed"}} + {{template "repo/diff/conversation" dict "." $ "comments" $line.Comments}} + {{end}} + + + {{end}} {{end}} {{else}} {{range $k, $line := $.section.Lines}} - + {{if eq .GetType 4}} - {{$expandDirection := $line.GetExpandDirection}} - -
    - {{if or (eq $expandDirection 3) (eq $expandDirection 5)}} - - {{end}} - {{if or (eq $expandDirection 3) (eq $expandDirection 4)}} - - {{end}} - {{if eq $expandDirection 2}} - - {{end}} -
    - + {{$line.RenderBlobExcerptButtons $.FileNameHash $diffBlobExcerptData}} {{else}} @@ -83,7 +71,21 @@ {{$inlineDiff := $.section.GetComputedInlineDiffFor $line ctx.Locale}} {{if $inlineDiff.EscapeStatus.Escaped}}{{end}} - {{$inlineDiff.Content}} + + {{- if and $canCreateComment -}} + + {{- end -}} + {{$inlineDiff.Content}} + + {{if $line.Comments}} + + + {{template "repo/diff/conversation" dict "." $ "comments" $line.Comments}} + + + {{end}} {{end}} {{end}} diff --git a/templates/repo/diff/section_split.tmpl b/templates/repo/diff/section_split.tmpl index 9953db5eb2..ab23b1b934 100644 --- a/templates/repo/diff/section_split.tmpl +++ b/templates/repo/diff/section_split.tmpl @@ -1,5 +1,5 @@ {{$file := .file}} -{{$blobExcerptLink := print (or ctx.RootData.CommitRepoLink ctx.RootData.RepoLink) (Iif $.root.PageIsWiki "/wiki" "") "/blob_excerpt/" (PathEscape $.root.AfterCommitID) "?"}} +{{$diffBlobExcerptData := $.root.DiffBlobExcerptData}} @@ -16,26 +16,8 @@ {{if or (ne .GetType 2) (not $hasmatch)}} {{if eq .GetType 4}} - {{$expandDirection := $line.GetExpandDirection}} - -
    - {{if or (eq $expandDirection 3) (eq $expandDirection 5)}} - - {{end}} - {{if or (eq $expandDirection 3) (eq $expandDirection 4)}} - - {{end}} - {{if eq $expandDirection 2}} - - {{end}} -
    - {{$inlineDiff := $section.GetComputedInlineDiffFor $line ctx.Locale}} + {{$inlineDiff := $section.GetComputedInlineDiffFor $line ctx.Locale}} + {{$line.RenderBlobExcerptButtons $file.NameHash $diffBlobExcerptData}} {{if $inlineDiff.EscapeStatus.Escaped}}{{end}} {{template "repo/diff/section_code" dict "diff" $inlineDiff}} {{else if and (eq .GetType 3) $hasmatch}}{{/* DEL */}} diff --git a/templates/repo/diff/section_unified.tmpl b/templates/repo/diff/section_unified.tmpl index cb612bc27c..908b14656e 100644 --- a/templates/repo/diff/section_unified.tmpl +++ b/templates/repo/diff/section_unified.tmpl @@ -1,7 +1,6 @@ {{$file := .file}} -{{$repoLink := or ctx.RootData.CommitRepoLink ctx.RootData.RepoLink}} -{{$afterCommitID := or $.root.AfterCommitID "no-after-commit-id"}}{{/* this tmpl is also used by the PR Conversation page, so the "AfterCommitID" may not exist */}} -{{$blobExcerptLink := print $repoLink (Iif $.root.PageIsWiki "/wiki" "") "/blob_excerpt/" (PathEscape $afterCommitID) "?"}} +{{/* this tmpl is also used by the PR Conversation page, so the "AfterCommitID" and "DiffBlobExcerptData" may not exist */}} +{{$diffBlobExcerptData := $.root.DiffBlobExcerptData}} @@ -14,26 +13,7 @@ {{if eq .GetType 4}} {{if $.root.AfterCommitID}} - {{$expandDirection := $line.GetExpandDirection}} - -
    - {{if or (eq $expandDirection 3) (eq $expandDirection 5)}} - - {{end}} - {{if or (eq $expandDirection 3) (eq $expandDirection 4)}} - - {{end}} - {{if eq $expandDirection 2}} - - {{end}} -
    - + {{$line.RenderBlobExcerptButtons $file.NameHash $diffBlobExcerptData}} {{else}} {{/* for code file preview page or comment diffs on pull comment pages, do not show the expansion arrows */}} diff --git a/templates/repo/issue/view_content/conversation.tmpl b/templates/repo/issue/view_content/conversation.tmpl index 189b9d6259..01261b0a56 100644 --- a/templates/repo/issue/view_content/conversation.tmpl +++ b/templates/repo/issue/view_content/conversation.tmpl @@ -1,3 +1,8 @@ +{{/* FIXME: DIFF-CONVERSATION-DATA: in the future this template should be refactor to avoid called by {{... "." $}} +At the moment, two kinds of request handler call this template: +* ExcerptBlob -> blob_excerpt.tmpl -> this +* Other compare and diff pages -> ... -> {section_unified.tmpl|section_split.tmpl} -> this) +The variables in "ctx.Data" are different in each case, making this template fragile, hard to read and maintain. */}} {{if len .comments}} {{$comment := index .comments 0}} {{$invalid := $comment.Invalidated}} diff --git a/web_src/css/review.css b/web_src/css/review.css index 23383c051c..39916d1bd8 100644 --- a/web_src/css/review.css +++ b/web_src/css/review.css @@ -115,6 +115,23 @@ color: var(--color-text); } +.code-expander-buttons { + position: relative; +} + +.code-expander-buttons .code-comment-more { + position: absolute; + line-height: 12px; + padding: 2px 4px; + font-size: 10px; + background-color: var(--color-primary); + color: var(--color-primary-contrast); + border-radius: 8px !important; + left: -12px; + top: 6px; + text-align: center; +} + .code-expander-button { border: none; color: var(--color-text-light); @@ -127,8 +144,8 @@ flex: 1; } -/* expand direction 3 is both ways with two buttons */ -.code-expander-buttons[data-expand-direction="3"] .code-expander-button { +/* expand direction "updown" is both ways with two buttons */ +.code-expander-buttons[data-expand-direction="updown"] .code-expander-button { height: 18px; } diff --git a/web_src/js/features/repo-diff.ts b/web_src/js/features/repo-diff.ts index bde7ec0324..24d937a252 100644 --- a/web_src/js/features/repo-diff.ts +++ b/web_src/js/features/repo-diff.ts @@ -9,7 +9,7 @@ import {submitEventSubmitter, queryElemSiblings, hideElem, showElem, animateOnce import {POST, GET} from '../modules/fetch.ts'; import {createTippy} from '../modules/tippy.ts'; import {invertFileFolding} from './file-fold.ts'; -import {parseDom} from '../utils.ts'; +import {parseDom, sleep} from '../utils.ts'; import {registerGlobalSelectorFunc} from '../modules/observer.ts'; const {i18n} = window.config; @@ -218,21 +218,46 @@ function initRepoDiffShowMore() { }); } -async function loadUntilFound() { - const hashTargetSelector = window.location.hash; - if (!hashTargetSelector.startsWith('#diff-') && !hashTargetSelector.startsWith('#issuecomment-')) { - return; - } +async function onLocationHashChange() { + // try to scroll to the target element by the current hash + const currentHash = window.location.hash; + if (!currentHash.startsWith('#diff-') && !currentHash.startsWith('#issuecomment-')) return; - while (true) { + // avoid reentrance when we are changing the hash to scroll and trigger ":target" selection + const attrAutoScrollRunning = 'data-auto-scroll-running'; + if (document.body.hasAttribute(attrAutoScrollRunning)) return; + + const targetElementId = currentHash.substring(1); + while (currentHash === window.location.hash) { // use getElementById to avoid querySelector throws an error when the hash is invalid // eslint-disable-next-line unicorn/prefer-query-selector - const targetElement = document.getElementById(hashTargetSelector.substring(1)); + const targetElement = document.getElementById(targetElementId); if (targetElement) { + // need to change hash to re-trigger ":target" CSS selector, let's manually scroll to it targetElement.scrollIntoView(); + document.body.setAttribute(attrAutoScrollRunning, 'true'); + window.location.hash = ''; + window.location.hash = currentHash; + setTimeout(() => document.body.removeAttribute(attrAutoScrollRunning), 0); return; } + // If looking for a hidden comment, try to expand the section that contains it + const issueCommentPrefix = '#issuecomment-'; + if (currentHash.startsWith(issueCommentPrefix)) { + const commentId = currentHash.substring(issueCommentPrefix.length); + const expandButton = document.querySelector(`.code-expander-button[data-hidden-comment-ids*=",${commentId},"]`); + if (expandButton) { + // avoid infinite loop, do not re-click the button if already clicked + const attrAutoLoadClicked = 'data-auto-load-clicked'; + if (expandButton.hasAttribute(attrAutoLoadClicked)) return; + expandButton.setAttribute(attrAutoLoadClicked, 'true'); + expandButton.click(); + await sleep(500); // Wait for HTMX to load the content. FIXME: need to drop htmx in the future + continue; // Try again to find the element + } + } + // the button will be refreshed after each "load more", so query it every time const showMoreButton = document.querySelector('#diff-show-more-files'); if (!showMoreButton) { @@ -246,8 +271,8 @@ async function loadUntilFound() { } function initRepoDiffHashChangeListener() { - window.addEventListener('hashchange', loadUntilFound); - loadUntilFound(); + window.addEventListener('hashchange', onLocationHashChange); + onLocationHashChange(); } export function initRepoDiffView() { From 66ee8f3553c0a4a6620c244e248f906c749dc085 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 20 Oct 2025 04:06:45 +0800 Subject: [PATCH 03/29] Avoid emoji mismatch and allow to only enable chosen emojis (#35692) Fix #23635 --- custom/conf/app.example.ini | 4 ++ modules/emoji/emoji.go | 132 ++++++++++++++++++----------------- modules/emoji/emoji_test.go | 25 +++++-- modules/markup/html_emoji.go | 34 +++++---- modules/markup/html_test.go | 9 +-- modules/setting/ui.go | 3 + 6 files changed, 115 insertions(+), 92 deletions(-) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index aa2fcee765..4a6356ec50 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -1343,6 +1343,10 @@ LEVEL = Info ;; Dont mistake it for Reactions. ;CUSTOM_EMOJIS = gitea, codeberg, gitlab, git, github, gogs ;; +;; Comma separated list of enabled emojis, for example: smile, thumbsup, thumbsdown +;; Leave it empty to enable all emojis. +;ENABLED_EMOJIS = +;; ;; Whether the full name of the users should be shown where possible. If the full name isn't set, the username will be used. ;DEFAULT_SHOW_FULL_NAME = false ;; diff --git a/modules/emoji/emoji.go b/modules/emoji/emoji.go index 3d4ef8599b..891a0b9ab3 100644 --- a/modules/emoji/emoji.go +++ b/modules/emoji/emoji.go @@ -8,7 +8,9 @@ import ( "io" "sort" "strings" - "sync" + "sync/atomic" + + "code.gitea.io/gitea/modules/setting" ) // Gemoji is a set of emoji data. @@ -23,74 +25,78 @@ type Emoji struct { SkinTones bool } -var ( - // codeMap provides a map of the emoji unicode code to its emoji data. - codeMap map[string]int +type globalVarsStruct struct { + codeMap map[string]int // emoji unicode code to its emoji data. + aliasMap map[string]int // the alias to its emoji data. + emptyReplacer *strings.Replacer // string replacer for emoji codes, used for finding emoji positions. + codeReplacer *strings.Replacer // string replacer for emoji codes. + aliasReplacer *strings.Replacer // string replacer for emoji aliases. +} - // aliasMap provides a map of the alias to its emoji data. - aliasMap map[string]int +var globalVarsStore atomic.Pointer[globalVarsStruct] - // emptyReplacer is the string replacer for emoji codes. - emptyReplacer *strings.Replacer +func globalVars() *globalVarsStruct { + vars := globalVarsStore.Load() + if vars != nil { + return vars + } + // although there can be concurrent calls, the result should be the same, and there is no performance problem + vars = &globalVarsStruct{} + vars.codeMap = make(map[string]int, len(GemojiData)) + vars.aliasMap = make(map[string]int, len(GemojiData)) - // codeReplacer is the string replacer for emoji codes. - codeReplacer *strings.Replacer + // process emoji codes and aliases + codePairs := make([]string, 0) + emptyPairs := make([]string, 0) + aliasPairs := make([]string, 0) - // aliasReplacer is the string replacer for emoji aliases. - aliasReplacer *strings.Replacer + // sort from largest to small so we match combined emoji first + sort.Slice(GemojiData, func(i, j int) bool { + return len(GemojiData[i].Emoji) > len(GemojiData[j].Emoji) + }) - once sync.Once -) - -func loadMap() { - once.Do(func() { - // initialize - codeMap = make(map[string]int, len(GemojiData)) - aliasMap = make(map[string]int, len(GemojiData)) - - // process emoji codes and aliases - codePairs := make([]string, 0) - emptyPairs := make([]string, 0) - aliasPairs := make([]string, 0) - - // sort from largest to small so we match combined emoji first - sort.Slice(GemojiData, func(i, j int) bool { - return len(GemojiData[i].Emoji) > len(GemojiData[j].Emoji) - }) - - for i, e := range GemojiData { - if e.Emoji == "" || len(e.Aliases) == 0 { - continue - } - - // setup codes - codeMap[e.Emoji] = i - codePairs = append(codePairs, e.Emoji, ":"+e.Aliases[0]+":") - emptyPairs = append(emptyPairs, e.Emoji, e.Emoji) - - // setup aliases - for _, a := range e.Aliases { - if a == "" { - continue - } - - aliasMap[a] = i - aliasPairs = append(aliasPairs, ":"+a+":", e.Emoji) - } + for idx, emoji := range GemojiData { + if emoji.Emoji == "" || len(emoji.Aliases) == 0 { + continue } - // create replacers - emptyReplacer = strings.NewReplacer(emptyPairs...) - codeReplacer = strings.NewReplacer(codePairs...) - aliasReplacer = strings.NewReplacer(aliasPairs...) - }) + // process aliases + firstAlias := "" + for _, alias := range emoji.Aliases { + if alias == "" { + continue + } + enabled := len(setting.UI.EnabledEmojisSet) == 0 || setting.UI.EnabledEmojisSet.Contains(alias) + if !enabled { + continue + } + if firstAlias == "" { + firstAlias = alias + } + vars.aliasMap[alias] = idx + aliasPairs = append(aliasPairs, ":"+alias+":", emoji.Emoji) + } + + // process emoji code + if firstAlias != "" { + vars.codeMap[emoji.Emoji] = idx + codePairs = append(codePairs, emoji.Emoji, ":"+emoji.Aliases[0]+":") + emptyPairs = append(emptyPairs, emoji.Emoji, emoji.Emoji) + } + } + + // create replacers + vars.emptyReplacer = strings.NewReplacer(emptyPairs...) + vars.codeReplacer = strings.NewReplacer(codePairs...) + vars.aliasReplacer = strings.NewReplacer(aliasPairs...) + globalVarsStore.Store(vars) + return vars } // FromCode retrieves the emoji data based on the provided unicode code (ie, // "\u2618" will return the Gemoji data for "shamrock"). func FromCode(code string) *Emoji { - loadMap() - i, ok := codeMap[code] + i, ok := globalVars().codeMap[code] if !ok { return nil } @@ -102,12 +108,11 @@ func FromCode(code string) *Emoji { // "alias" or ":alias:" (ie, "shamrock" or ":shamrock:" will return the Gemoji // data for "shamrock"). func FromAlias(alias string) *Emoji { - loadMap() if strings.HasPrefix(alias, ":") && strings.HasSuffix(alias, ":") { alias = alias[1 : len(alias)-1] } - i, ok := aliasMap[alias] + i, ok := globalVars().aliasMap[alias] if !ok { return nil } @@ -119,15 +124,13 @@ func FromAlias(alias string) *Emoji { // alias (in the form of ":alias:") (ie, "\u2618" will be converted to // ":shamrock:"). func ReplaceCodes(s string) string { - loadMap() - return codeReplacer.Replace(s) + return globalVars().codeReplacer.Replace(s) } // ReplaceAliases replaces all aliases of the form ":alias:" with its // corresponding unicode value. func ReplaceAliases(s string) string { - loadMap() - return aliasReplacer.Replace(s) + return globalVars().aliasReplacer.Replace(s) } type rememberSecondWriteWriter struct { @@ -163,7 +166,6 @@ func (n *rememberSecondWriteWriter) WriteString(s string) (int, error) { // FindEmojiSubmatchIndex returns index pair of longest emoji in a string func FindEmojiSubmatchIndex(s string) []int { - loadMap() secondWriteWriter := rememberSecondWriteWriter{} // A faster and clean implementation would copy the trie tree formation in strings.NewReplacer but @@ -175,7 +177,7 @@ func FindEmojiSubmatchIndex(s string) []int { // Therefore we can simply take the index of the second write as our first emoji // // FIXME: just copy the trie implementation from strings.NewReplacer - _, _ = emptyReplacer.WriteString(&secondWriteWriter, s) + _, _ = globalVars().emptyReplacer.WriteString(&secondWriteWriter, s) // if we wrote less than twice then we never "replaced" if secondWriteWriter.writecount < 2 { diff --git a/modules/emoji/emoji_test.go b/modules/emoji/emoji_test.go index fbf80fe41a..607299cdc1 100644 --- a/modules/emoji/emoji_test.go +++ b/modules/emoji/emoji_test.go @@ -7,14 +7,13 @@ package emoji import ( "testing" + "code.gitea.io/gitea/modules/container" + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/test" + "github.com/stretchr/testify/assert" ) -func TestDumpInfo(t *testing.T) { - t.Logf("codes: %d", len(codeMap)) - t.Logf("aliases: %d", len(aliasMap)) -} - func TestLookup(t *testing.T) { a := FromCode("\U0001f37a") b := FromCode("🍺") @@ -24,7 +23,6 @@ func TestLookup(t *testing.T) { assert.Equal(t, a, b) assert.Equal(t, b, c) assert.Equal(t, c, d) - assert.Equal(t, a, d) m := FromCode("\U0001f44d") n := FromAlias(":thumbsup:") @@ -32,7 +30,20 @@ func TestLookup(t *testing.T) { assert.Equal(t, m, n) assert.Equal(t, m, o) - assert.Equal(t, n, o) + + defer test.MockVariableValue(&setting.UI.EnabledEmojisSet, container.SetOf("thumbsup"))() + defer globalVarsStore.Store(nil) + globalVarsStore.Store(nil) + a = FromCode("\U0001f37a") + c = FromAlias(":beer:") + m = FromCode("\U0001f44d") + n = FromAlias(":thumbsup:") + o = FromAlias("+1") + assert.Nil(t, a) + assert.Nil(t, c) + assert.NotNil(t, m) + assert.NotNil(t, n) + assert.Nil(t, o) } func TestReplacers(t *testing.T) { diff --git a/modules/markup/html_emoji.go b/modules/markup/html_emoji.go index c638065425..91ba26c676 100644 --- a/modules/markup/html_emoji.go +++ b/modules/markup/html_emoji.go @@ -5,6 +5,7 @@ package markup import ( "strings" + "unicode" "code.gitea.io/gitea/modules/emoji" "code.gitea.io/gitea/modules/setting" @@ -66,26 +67,31 @@ func emojiShortCodeProcessor(ctx *RenderContext, node *html.Node) { } m[0] += start m[1] += start - start = m[1] alias := node.Data[m[0]:m[1]] - alias = strings.ReplaceAll(alias, ":", "") - converted := emoji.FromAlias(alias) - if converted == nil { - // check if this is a custom reaction - if _, exist := setting.UI.CustomEmojisMap[alias]; exist { - replaceContent(node, m[0], m[1], createCustomEmoji(ctx, alias)) - node = node.NextSibling.NextSibling - start = 0 - continue - } + + var nextChar byte + if m[1] < len(node.Data) { + nextChar = node.Data[m[1]] + } + if nextChar == ':' || unicode.IsLetter(rune(nextChar)) || unicode.IsDigit(rune(nextChar)) { continue } - replaceContent(node, m[0], m[1], createEmoji(ctx, converted.Emoji, converted.Description)) - node = node.NextSibling.NextSibling - start = 0 + alias = strings.Trim(alias, ":") + converted := emoji.FromAlias(alias) + if converted != nil { + // standard emoji + replaceContent(node, m[0], m[1], createEmoji(ctx, converted.Emoji, converted.Description)) + node = node.NextSibling.NextSibling + start = 0 // restart searching start since node has changed + } else if _, exist := setting.UI.CustomEmojisMap[alias]; exist { + // custom reaction + replaceContent(node, m[0], m[1], createCustomEmoji(ctx, alias)) + node = node.NextSibling.NextSibling + start = 0 // restart searching start since node has changed + } } } diff --git a/modules/markup/html_test.go b/modules/markup/html_test.go index 5fdbf43f7c..08b050baae 100644 --- a/modules/markup/html_test.go +++ b/modules/markup/html_test.go @@ -357,12 +357,9 @@ func TestRender_emoji(t *testing.T) { `

    😎🤪🔐🤑❓

    `) // should match nothing - test( - "2001:0db8:85a3:0000:0000:8a2e:0370:7334", - `

    2001:0db8:85a3:0000:0000:8a2e:0370:7334

    `) - test( - ":not exist:", - `

    :not exist:

    `) + test(":100:200", `

    :100:200

    `) + test("std::thread::something", `

    std::thread::something

    `) + test(":not exist:", `

    :not exist:

    `) } func TestRender_ShortLinks(t *testing.T) { diff --git a/modules/setting/ui.go b/modules/setting/ui.go index 3d9c916bf7..13cb0f5c66 100644 --- a/modules/setting/ui.go +++ b/modules/setting/ui.go @@ -33,6 +33,8 @@ var UI = struct { ReactionsLookup container.Set[string] `ini:"-"` CustomEmojis []string CustomEmojisMap map[string]string `ini:"-"` + EnabledEmojis []string + EnabledEmojisSet container.Set[string] `ini:"-"` SearchRepoDescription bool OnlyShowRelevantRepos bool ExploreDefaultSort string `ini:"EXPLORE_PAGING_DEFAULT_SORT"` @@ -169,4 +171,5 @@ func loadUIFrom(rootCfg ConfigProvider) { for _, emoji := range UI.CustomEmojis { UI.CustomEmojisMap[emoji] = ":" + emoji + ":" } + UI.EnabledEmojisSet = container.SetOf(UI.EnabledEmojis...) } From 897e48dde331bf5896d345e5e902aa5ab398c5b1 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Mon, 20 Oct 2025 04:46:37 -0600 Subject: [PATCH 04/29] Add quick approve button on PR page (#35678) This PR adds a quick approve button on PR page to allow reviewers to approve all pending checks. Only users with write permission to the Actions unit can approve. --------- Signed-off-by: Zettat123 Co-authored-by: wxiaoguang --- models/git/commit_status.go | 58 +++++-- options/locale/locale_en-US.ini | 4 + routers/web/repo/actions/view.go | 92 ++++++++--- routers/web/repo/issue_view.go | 3 + routers/web/repo/pull.go | 32 +++- routers/web/web.go | 1 + services/actions/commit_status.go | 28 ++++ .../issue/view_content/pull_merge_box.tmpl | 3 +- templates/repo/pulls/status.tmpl | 39 +++-- tests/integration/actions_approve_test.go | 146 ++++++++++++++++++ 10 files changed, 358 insertions(+), 48 deletions(-) create mode 100644 tests/integration/actions_approve_test.go diff --git a/models/git/commit_status.go b/models/git/commit_status.go index e255bca5d0..2ae5937a3d 100644 --- a/models/git/commit_status.go +++ b/models/git/commit_status.go @@ -30,17 +30,21 @@ import ( // CommitStatus holds a single Status of a single Commit type CommitStatus struct { - ID int64 `xorm:"pk autoincr"` - Index int64 `xorm:"INDEX UNIQUE(repo_sha_index)"` - RepoID int64 `xorm:"INDEX UNIQUE(repo_sha_index)"` - Repo *repo_model.Repository `xorm:"-"` - State commitstatus.CommitStatusState `xorm:"VARCHAR(7) NOT NULL"` - SHA string `xorm:"VARCHAR(64) NOT NULL INDEX UNIQUE(repo_sha_index)"` - TargetURL string `xorm:"TEXT"` - Description string `xorm:"TEXT"` - ContextHash string `xorm:"VARCHAR(64) index"` - Context string `xorm:"TEXT"` - Creator *user_model.User `xorm:"-"` + ID int64 `xorm:"pk autoincr"` + Index int64 `xorm:"INDEX UNIQUE(repo_sha_index)"` + RepoID int64 `xorm:"INDEX UNIQUE(repo_sha_index)"` + Repo *repo_model.Repository `xorm:"-"` + State commitstatus.CommitStatusState `xorm:"VARCHAR(7) NOT NULL"` + SHA string `xorm:"VARCHAR(64) NOT NULL INDEX UNIQUE(repo_sha_index)"` + + // TargetURL points to the commit status page reported by a CI system + // If Gitea Actions is used, it is a relative link like "{RepoLink}/actions/runs/{RunID}/jobs{JobID}" + TargetURL string `xorm:"TEXT"` + + Description string `xorm:"TEXT"` + ContextHash string `xorm:"VARCHAR(64) index"` + Context string `xorm:"TEXT"` + Creator *user_model.User `xorm:"-"` CreatorID int64 CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` @@ -211,21 +215,45 @@ func (status *CommitStatus) LocaleString(lang translation.Locale) string { // HideActionsURL set `TargetURL` to an empty string if the status comes from Gitea Actions func (status *CommitStatus) HideActionsURL(ctx context.Context) { + if _, ok := status.cutTargetURLGiteaActionsPrefix(ctx); ok { + status.TargetURL = "" + } +} + +func (status *CommitStatus) cutTargetURLGiteaActionsPrefix(ctx context.Context) (string, bool) { if status.RepoID == 0 { - return + return "", false } if status.Repo == nil { if err := status.loadRepository(ctx); err != nil { log.Error("loadRepository: %v", err) - return + return "", false } } prefix := status.Repo.Link() + "/actions" - if strings.HasPrefix(status.TargetURL, prefix) { - status.TargetURL = "" + return strings.CutPrefix(status.TargetURL, prefix) +} + +// ParseGiteaActionsTargetURL parses the commit status target URL as Gitea Actions link +func (status *CommitStatus) ParseGiteaActionsTargetURL(ctx context.Context) (runID, jobID int64, ok bool) { + s, ok := status.cutTargetURLGiteaActionsPrefix(ctx) + if !ok { + return 0, 0, false } + + parts := strings.Split(s, "/") // expect: /runs/{runID}/jobs/{jobID} + if len(parts) < 5 || parts[1] != "runs" || parts[3] != "jobs" { + return 0, 0, false + } + + runID, err1 := strconv.ParseInt(parts[2], 10, 64) + jobID, err2 := strconv.ParseInt(parts[4], 10, 64) + if err1 != nil || err2 != nil { + return 0, 0, false + } + return runID, jobID, true } // CalcCommitStatus returns commit status state via some status, the commit statues should order by id desc diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 03017ce674..46fdf06022 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1969,6 +1969,9 @@ pulls.status_checks_requested = Required pulls.status_checks_details = Details pulls.status_checks_hide_all = Hide all checks pulls.status_checks_show_all = Show all checks +pulls.status_checks_approve_all = Approve all workflows +pulls.status_checks_need_approvals = %d workflow awaiting approval +pulls.status_checks_need_approvals_helper = The workflow will only run after approval from the repository maintainer. pulls.update_branch = Update branch by merge pulls.update_branch_rebase = Update branch by rebase pulls.update_branch_success = Branch update was successful @@ -3890,6 +3893,7 @@ workflow.has_workflow_dispatch = This workflow has a workflow_dispatch event tri workflow.has_no_workflow_dispatch = Workflow '%s' has no workflow_dispatch event trigger. need_approval_desc = Need approval to run workflows for fork pull request. +approve_all_success = All workflow runs are approved successfully. variables = Variables variables.management = Variables Management diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go index 4250e6ff77..013dab4acf 100644 --- a/routers/web/repo/actions/view.go +++ b/routers/web/repo/actions/view.go @@ -606,33 +606,53 @@ func Cancel(ctx *context_module.Context) { func Approve(ctx *context_module.Context) { runIndex := getRunIndex(ctx) - current, jobs := getRunJobs(ctx, runIndex, -1) + approveRuns(ctx, []int64{runIndex}) if ctx.Written() { return } - run := current.Run - doer := ctx.Doer - var updatedJobs []*actions_model.ActionRunJob + ctx.JSONOK() +} + +func approveRuns(ctx *context_module.Context, runIndexes []int64) { + doer := ctx.Doer + repo := ctx.Repo.Repository + + updatedJobs := make([]*actions_model.ActionRunJob, 0) + runMap := make(map[int64]*actions_model.ActionRun, len(runIndexes)) + runJobs := make(map[int64][]*actions_model.ActionRunJob, len(runIndexes)) err := db.WithTx(ctx, func(ctx context.Context) (err error) { - run.NeedApproval = false - run.ApprovedBy = doer.ID - if err := actions_model.UpdateRun(ctx, run, "need_approval", "approved_by"); err != nil { - return err - } - for _, job := range jobs { - job.Status, err = actions_service.PrepareToStartJobWithConcurrency(ctx, job) + for _, runIndex := range runIndexes { + run, err := actions_model.GetRunByIndex(ctx, repo.ID, runIndex) if err != nil { return err } - if job.Status == actions_model.StatusWaiting { - n, err := actions_model.UpdateRunJob(ctx, job, nil, "status") + runMap[run.ID] = run + run.Repo = repo + run.NeedApproval = false + run.ApprovedBy = doer.ID + if err := actions_model.UpdateRun(ctx, run, "need_approval", "approved_by"); err != nil { + return err + } + jobs, err := actions_model.GetRunJobsByRunID(ctx, run.ID) + if err != nil { + return err + } + runJobs[run.ID] = jobs + for _, job := range jobs { + job.Status, err = actions_service.PrepareToStartJobWithConcurrency(ctx, job) if err != nil { return err } - if n > 0 { - updatedJobs = append(updatedJobs, job) + if job.Status == actions_model.StatusWaiting { + n, err := actions_model.UpdateRunJob(ctx, job, nil, "status") + if err != nil { + return err + } + if n > 0 { + updatedJobs = append(updatedJobs, job) + } } } } @@ -643,7 +663,9 @@ func Approve(ctx *context_module.Context) { return } - actions_service.CreateCommitStatusForRunJobs(ctx, current.Run, jobs...) + for runID, run := range runMap { + actions_service.CreateCommitStatusForRunJobs(ctx, run, runJobs[runID]...) + } if len(updatedJobs) > 0 { job := updatedJobs[0] @@ -654,8 +676,6 @@ func Approve(ctx *context_module.Context) { _ = job.LoadAttributes(ctx) notify_service.WorkflowJobStatusUpdate(ctx, job.Run.Repo, job.Run.TriggerUser, job, nil) } - - ctx.JSONOK() } func Delete(ctx *context_module.Context) { @@ -818,6 +838,42 @@ func ArtifactsDownloadView(ctx *context_module.Context) { } } +func ApproveAllChecks(ctx *context_module.Context) { + repo := ctx.Repo.Repository + commitID := ctx.FormString("commit_id") + + commitStatuses, err := git_model.GetLatestCommitStatus(ctx, repo.ID, commitID, db.ListOptionsAll) + if err != nil { + ctx.ServerError("GetLatestCommitStatus", err) + return + } + runs, err := actions_service.GetRunsFromCommitStatuses(ctx, commitStatuses) + if err != nil { + ctx.ServerError("GetRunsFromCommitStatuses", err) + return + } + + runIndexes := make([]int64, 0, len(runs)) + for _, run := range runs { + if run.NeedApproval { + runIndexes = append(runIndexes, run.Index) + } + } + + if len(runIndexes) == 0 { + ctx.JSONOK() + return + } + + approveRuns(ctx, runIndexes) + if ctx.Written() { + return + } + + ctx.Flash.Success(ctx.Tr("actions.approve_all_success")) + ctx.JSONOK() +} + func DisableWorkflowFile(ctx *context_module.Context) { disableOrEnableWorkflowFile(ctx, false) } diff --git a/routers/web/repo/issue_view.go b/routers/web/repo/issue_view.go index d9f6c33e3f..5866ddc402 100644 --- a/routers/web/repo/issue_view.go +++ b/routers/web/repo/issue_view.go @@ -437,6 +437,9 @@ func ViewIssue(ctx *context.Context) { func ViewPullMergeBox(ctx *context.Context) { issue := prepareIssueViewLoad(ctx) + if ctx.Written() { + return + } if !issue.IsPull { ctx.NotFound(nil) return diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index cfe9a7ae02..c8da025b15 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -38,6 +38,7 @@ import ( "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/routers/utils" shared_user "code.gitea.io/gitea/routers/web/shared/user" + actions_service "code.gitea.io/gitea/services/actions" asymkey_service "code.gitea.io/gitea/services/asymkey" "code.gitea.io/gitea/services/automerge" "code.gitea.io/gitea/services/context" @@ -311,6 +312,14 @@ func prepareMergedViewPullInfo(ctx *context.Context, issue *issues_model.Issue) return compareInfo } +type pullCommitStatusCheckData struct { + MissingRequiredChecks []string // list of missing required checks + IsContextRequired func(string) bool // function to check whether a context is required + RequireApprovalRunCount int // number of workflow runs that require approval + CanApprove bool // whether the user can approve workflow runs + ApproveLink string // link to approve all checks +} + // prepareViewPullInfo show meta information for a pull request preview page func prepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *pull_service.CompareInfo { ctx.Data["PullRequestWorkInProgressPrefixes"] = setting.Repository.PullRequest.WorkInProgressPrefixes @@ -456,6 +465,11 @@ func prepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *pull_ return nil } + statusCheckData := &pullCommitStatusCheckData{ + ApproveLink: fmt.Sprintf("%s/actions/approve-all-checks?commit_id=%s", repo.Link(), sha), + } + ctx.Data["StatusCheckData"] = statusCheckData + commitStatuses, err := git_model.GetLatestCommitStatus(ctx, repo.ID, sha, db.ListOptionsAll) if err != nil { ctx.ServerError("GetLatestCommitStatus", err) @@ -465,6 +479,20 @@ func prepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *pull_ git_model.CommitStatusesHideActionsURL(ctx, commitStatuses) } + runs, err := actions_service.GetRunsFromCommitStatuses(ctx, commitStatuses) + if err != nil { + ctx.ServerError("GetRunsFromCommitStatuses", err) + return nil + } + for _, run := range runs { + if run.NeedApproval { + statusCheckData.RequireApprovalRunCount++ + } + } + if statusCheckData.RequireApprovalRunCount > 0 { + statusCheckData.CanApprove = ctx.Repo.CanWrite(unit.TypeActions) + } + if len(commitStatuses) > 0 { ctx.Data["LatestCommitStatuses"] = commitStatuses ctx.Data["LatestCommitStatus"] = git_model.CalcCommitStatus(commitStatuses) @@ -486,9 +514,9 @@ func prepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *pull_ missingRequiredChecks = append(missingRequiredChecks, requiredContext) } } - ctx.Data["MissingRequiredChecks"] = missingRequiredChecks + statusCheckData.MissingRequiredChecks = missingRequiredChecks - ctx.Data["is_context_required"] = func(context string) bool { + statusCheckData.IsContextRequired = func(context string) bool { for _, c := range pb.StatusCheckContexts { if c == context { return true diff --git a/routers/web/web.go b/routers/web/web.go index 5ee211b576..9b3cfb6d16 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1459,6 +1459,7 @@ func registerWebRoutes(m *web.Router) { m.Post("/enable", reqRepoAdmin, actions.EnableWorkflowFile) m.Post("/run", reqRepoActionsWriter, actions.Run) m.Get("/workflow-dispatch-inputs", reqRepoActionsWriter, actions.WorkflowDispatchInputs) + m.Post("/approve-all-checks", reqRepoActionsWriter, actions.ApproveAllChecks) m.Group("/runs/{run}", func() { m.Combo(""). diff --git a/services/actions/commit_status.go b/services/actions/commit_status.go index d3f2b0f3cc..089dfeb634 100644 --- a/services/actions/commit_status.go +++ b/services/actions/commit_status.go @@ -18,6 +18,7 @@ import ( actions_module "code.gitea.io/gitea/modules/actions" "code.gitea.io/gitea/modules/commitstatus" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/util" webhook_module "code.gitea.io/gitea/modules/webhook" commitstatus_service "code.gitea.io/gitea/services/repository/commitstatus" @@ -52,6 +53,33 @@ func CreateCommitStatusForRunJobs(ctx context.Context, run *actions_model.Action } } +func GetRunsFromCommitStatuses(ctx context.Context, statuses []*git_model.CommitStatus) ([]*actions_model.ActionRun, error) { + runMap := make(map[int64]*actions_model.ActionRun) + for _, status := range statuses { + runIndex, _, ok := status.ParseGiteaActionsTargetURL(ctx) + if !ok { + continue + } + _, ok = runMap[runIndex] + if !ok { + run, err := actions_model.GetRunByIndex(ctx, status.RepoID, runIndex) + if err != nil { + if errors.Is(err, util.ErrNotExist) { + // the run may be deleted manually, just skip it + continue + } + return nil, fmt.Errorf("GetRunByIndex: %w", err) + } + runMap[runIndex] = run + } + } + runs := make([]*actions_model.ActionRun, 0, len(runMap)) + for _, run := range runMap { + runs = append(runs, run) + } + return runs, nil +} + func getCommitStatusEventNameAndCommitID(run *actions_model.ActionRun) (event, commitID string, _ error) { switch run.Event { case webhook_module.HookEventPush: diff --git a/templates/repo/issue/view_content/pull_merge_box.tmpl b/templates/repo/issue/view_content/pull_merge_box.tmpl index b0ac24c9f6..c0d717e854 100644 --- a/templates/repo/issue/view_content/pull_merge_box.tmpl +++ b/templates/repo/issue/view_content/pull_merge_box.tmpl @@ -31,9 +31,8 @@ {{template "repo/pulls/status" (dict "CommitStatus" .LatestCommitStatus "CommitStatuses" .LatestCommitStatuses - "MissingRequiredChecks" .MissingRequiredChecks "ShowHideChecks" true - "is_context_required" .is_context_required + "StatusCheckData" .StatusCheckData )}} {{end}} diff --git a/templates/repo/pulls/status.tmpl b/templates/repo/pulls/status.tmpl index 96030f9422..f3c1973c2b 100644 --- a/templates/repo/pulls/status.tmpl +++ b/templates/repo/pulls/status.tmpl @@ -1,11 +1,10 @@ {{/* Template Attributes: * CommitStatus: summary of all commit status state * CommitStatuses: all commit status elements -* MissingRequiredChecks: commit check contexts that are required by branch protection but not present * ShowHideChecks: whether use a button to show/hide the checks -* is_context_required: Used in pull request commit status check table +* StatusCheckData: additional status check data, see backend pullCommitStatusCheckData struct */}} - +{{$statusCheckData := .StatusCheckData}} {{if .CommitStatus}}
    @@ -33,25 +32,43 @@ {{end}}
    + {{if and $statusCheckData $statusCheckData.RequireApprovalRunCount}} +
    +
    + + {{ctx.Locale.Tr "repo.pulls.status_checks_need_approvals" $statusCheckData.RequireApprovalRunCount}} + +

    {{ctx.Locale.Tr "repo.pulls.status_checks_need_approvals_helper"}}

    +
    + {{if $statusCheckData.CanApprove}} + + {{end}} +
    + {{end}} +
    {{range .CommitStatuses}}
    {{template "repo/commit_status" .}}
    {{.Context}} {{.Description}}
    - {{if $.is_context_required}} - {{if (call $.is_context_required .Context)}}
    {{ctx.Locale.Tr "repo.pulls.status_checks_requested"}}
    {{end}} + {{if and $statusCheckData $statusCheckData.IsContextRequired}} + {{if (call $statusCheckData.IsContextRequired .Context)}}
    {{ctx.Locale.Tr "repo.pulls.status_checks_requested"}}
    {{end}} {{end}} {{if .TargetURL}}{{ctx.Locale.Tr "repo.pulls.status_checks_details"}}{{end}}
    {{end}} - {{range .MissingRequiredChecks}} -
    - {{svg "octicon-dot-fill" 18 "commit-status icon text yellow"}} -
    {{.}}
    -
    {{ctx.Locale.Tr "repo.pulls.status_checks_requested"}}
    -
    + {{if $statusCheckData}} + {{range $statusCheckData.MissingRequiredChecks}} +
    + {{svg "octicon-dot-fill" 18 "commit-status icon text yellow"}} +
    {{.}}
    +
    {{ctx.Locale.Tr "repo.pulls.status_checks_requested"}}
    +
    + {{end}} {{end}}
    diff --git a/tests/integration/actions_approve_test.go b/tests/integration/actions_approve_test.go new file mode 100644 index 0000000000..04b8bcb715 --- /dev/null +++ b/tests/integration/actions_approve_test.go @@ -0,0 +1,146 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package integration + +import ( + "encoding/base64" + "fmt" + "net/http" + "net/url" + "testing" + "time" + + actions_model "code.gitea.io/gitea/models/actions" + auth_model "code.gitea.io/gitea/models/auth" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/util" + + "github.com/stretchr/testify/assert" +) + +func TestApproveAllRunsOnPullRequestPage(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + // user2 is the owner of the base repo + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + user2Session := loginUser(t, user2.Name) + user2Token := getTokenForLoggedInUser(t, user2Session, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser) + // user4 is the owner of the fork repo + user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + user4Session := loginUser(t, user4.Name) + user4Token := getTokenForLoggedInUser(t, loginUser(t, user4.Name), auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser) + + apiBaseRepo := createActionsTestRepo(t, user2Token, "approve-all-runs", false) + baseRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: apiBaseRepo.ID}) + user2APICtx := NewAPITestContext(t, baseRepo.OwnerName, baseRepo.Name, auth_model.AccessTokenScopeWriteRepository) + defer doAPIDeleteRepository(user2APICtx)(t) + + runner := newMockRunner() + runner.registerAsRepoRunner(t, baseRepo.OwnerName, baseRepo.Name, "mock-runner", []string{"ubuntu-latest"}, false) + + // init workflows + wf1TreePath := ".gitea/workflows/pull_1.yml" + wf1FileContent := `name: Pull 1 +on: pull_request +jobs: + unit-test: + runs-on: ubuntu-latest + steps: + - run: echo unit-test +` + opts1 := getWorkflowCreateFileOptions(user2, baseRepo.DefaultBranch, "create %s"+wf1TreePath, wf1FileContent) + createWorkflowFile(t, user2Token, baseRepo.OwnerName, baseRepo.Name, wf1TreePath, opts1) + wf2TreePath := ".gitea/workflows/pull_2.yml" + wf2FileContent := `name: Pull 2 +on: pull_request +jobs: + integration-test: + runs-on: ubuntu-latest + steps: + - run: echo integration-test +` + opts2 := getWorkflowCreateFileOptions(user2, baseRepo.DefaultBranch, "create %s"+wf2TreePath, wf2FileContent) + createWorkflowFile(t, user2Token, baseRepo.OwnerName, baseRepo.Name, wf2TreePath, opts2) + + // user4 forks the repo + req := NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/repos/%s/%s/forks", baseRepo.OwnerName, baseRepo.Name), + &api.CreateForkOption{ + Name: util.ToPointer("approve-all-runs-fork"), + }).AddTokenAuth(user4Token) + resp := MakeRequest(t, req, http.StatusAccepted) + var apiForkRepo api.Repository + DecodeJSON(t, resp, &apiForkRepo) + forkRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: apiForkRepo.ID}) + user4APICtx := NewAPITestContext(t, user4.Name, forkRepo.Name, auth_model.AccessTokenScopeWriteRepository) + defer doAPIDeleteRepository(user4APICtx)(t) + + // user4 creates a pull request from branch "bugfix/user4" + doAPICreateFile(user4APICtx, "user4-fix.txt", &api.CreateFileOptions{ + FileOptions: api.FileOptions{ + NewBranchName: "bugfix/user4", + Message: "create user4-fix.txt", + Author: api.Identity{ + Name: user4.Name, + Email: user4.Email, + }, + Committer: api.Identity{ + Name: user4.Name, + Email: user4.Email, + }, + Dates: api.CommitDateOptions{ + Author: time.Now(), + Committer: time.Now(), + }, + }, + ContentBase64: base64.StdEncoding.EncodeToString([]byte("user4-fix")), + })(t) + apiPull, err := doAPICreatePullRequest(user4APICtx, baseRepo.OwnerName, baseRepo.Name, baseRepo.DefaultBranch, user4.Name+":bugfix/user4")(t) + assert.NoError(t, err) + + // check runs + run1 := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, TriggerUserID: user4.ID, WorkflowID: "pull_1.yml"}) + assert.True(t, run1.NeedApproval) + assert.Equal(t, actions_model.StatusBlocked, run1.Status) + run2 := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, TriggerUserID: user4.ID, WorkflowID: "pull_2.yml"}) + assert.True(t, run2.NeedApproval) + assert.Equal(t, actions_model.StatusBlocked, run2.Status) + + // user4 cannot see the approve button + req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d", baseRepo.OwnerName, baseRepo.Name, apiPull.Index)) + resp = user4Session.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + assert.Zero(t, htmlDoc.doc.Find("#approve-status-checks button.link-action").Length()) + + // user2 can see the approve button + req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d", baseRepo.OwnerName, baseRepo.Name, apiPull.Index)) + resp = user2Session.MakeRequest(t, req, http.StatusOK) + htmlDoc = NewHTMLParser(t, resp.Body) + dataURL, exist := htmlDoc.doc.Find("#approve-status-checks button.link-action").Attr("data-url") + assert.True(t, exist) + assert.Equal(t, + fmt.Sprintf("%s/actions/approve-all-checks?commit_id=%s", + baseRepo.Link(), apiPull.Head.Sha), + dataURL, + ) + + // user2 approves all runs + req = NewRequestWithValues(t, "POST", dataURL, + map[string]string{ + "_csrf": GetUserCSRFToken(t, user2Session), + }) + user2Session.MakeRequest(t, req, http.StatusOK) + + // check runs + run1 = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: run1.ID}) + assert.False(t, run1.NeedApproval) + assert.Equal(t, user2.ID, run1.ApprovedBy) + assert.Equal(t, actions_model.StatusWaiting, run1.Status) + run2 = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: run2.ID}) + assert.False(t, run2.NeedApproval) + assert.Equal(t, user2.ID, run2.ApprovedBy) + assert.Equal(t, actions_model.StatusWaiting, run2.Status) + }) +} From b2ee5be52e2c4327cc86f1805460e12e44a1ca0d Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 21 Oct 2025 02:43:08 +0800 Subject: [PATCH 05/29] Refactor legacy code (#35708) And by the way, remove the legacy TODO, split large functions into small ones, and add more tests --- cmd/serv.go | 27 +-- models/asymkey/ssh_key.go | 7 - models/asymkey/ssh_key_authorized_keys.go | 73 ++++--- models/repo/upload.go | 11 +- modules/git/hook.go | 22 +- modules/setting/config_provider.go | 6 +- modules/util/path.go | 11 +- routers/private/key.go | 13 +- .../asymkey/ssh_key_authorized_principals.go | 14 +- services/doctor/authorizedkeys.go | 6 +- services/lfs/server.go | 33 ++- services/lfs/server_test.go | 51 +++++ services/repository/generate.go | 197 ++++++++++-------- services/repository/generate_test.go | 170 ++++++++++++--- tests/integration/cmd_keys_test.go | 2 +- 15 files changed, 407 insertions(+), 236 deletions(-) create mode 100644 services/lfs/server_test.go diff --git a/cmd/serv.go b/cmd/serv.go index 76d8c81544..72ca7c4a00 100644 --- a/cmd/serv.go +++ b/cmd/serv.go @@ -13,7 +13,6 @@ import ( "path/filepath" "strconv" "strings" - "time" "unicode" asymkey_model "code.gitea.io/gitea/models/asymkey" @@ -32,7 +31,6 @@ import ( "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/services/lfs" - "github.com/golang-jwt/jwt/v5" "github.com/kballard/go-shellquote" "github.com/urfave/cli/v3" ) @@ -133,27 +131,6 @@ func getAccessMode(verb, lfsVerb string) perm.AccessMode { return perm.AccessModeNone } -func getLFSAuthToken(ctx context.Context, lfsVerb string, results *private.ServCommandResults) (string, error) { - now := time.Now() - claims := lfs.Claims{ - RegisteredClaims: jwt.RegisteredClaims{ - ExpiresAt: jwt.NewNumericDate(now.Add(setting.LFS.HTTPAuthExpiry)), - NotBefore: jwt.NewNumericDate(now), - }, - RepoID: results.RepoID, - Op: lfsVerb, - UserID: results.UserID, - } - token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims) - - // Sign and get the complete encoded token as a string using the secret - tokenString, err := token.SignedString(setting.LFS.JWTSecretBytes) - if err != nil { - return "", fail(ctx, "Failed to sign JWT Token", "Failed to sign JWT token: %v", err) - } - return "Bearer " + tokenString, nil -} - func runServ(ctx context.Context, c *cli.Command) error { // FIXME: This needs to internationalised setup(ctx, c.Bool("debug")) @@ -283,7 +260,7 @@ func runServ(ctx context.Context, c *cli.Command) error { // LFS SSH protocol if verb == git.CmdVerbLfsTransfer { - token, err := getLFSAuthToken(ctx, lfsVerb, results) + token, err := lfs.GetLFSAuthTokenWithBearer(lfs.AuthTokenOptions{Op: lfsVerb, UserID: results.UserID, RepoID: results.RepoID}) if err != nil { return err } @@ -294,7 +271,7 @@ func runServ(ctx context.Context, c *cli.Command) error { if verb == git.CmdVerbLfsAuthenticate { url := fmt.Sprintf("%s%s/%s.git/info/lfs", setting.AppURL, url.PathEscape(results.OwnerName), url.PathEscape(results.RepoName)) - token, err := getLFSAuthToken(ctx, lfsVerb, results) + token, err := lfs.GetLFSAuthTokenWithBearer(lfs.AuthTokenOptions{Op: lfsVerb, UserID: results.UserID, RepoID: results.RepoID}) if err != nil { return err } diff --git a/models/asymkey/ssh_key.go b/models/asymkey/ssh_key.go index 87205f0651..d77b5d46a7 100644 --- a/models/asymkey/ssh_key.go +++ b/models/asymkey/ssh_key.go @@ -67,13 +67,6 @@ func (key *PublicKey) OmitEmail() string { return strings.Join(strings.Split(key.Content, " ")[:2], " ") } -// AuthorizedString returns formatted public key string for authorized_keys file. -// -// TODO: Consider dropping this function -func (key *PublicKey) AuthorizedString() string { - return AuthorizedStringForKey(key) -} - func addKey(ctx context.Context, key *PublicKey) (err error) { if len(key.Fingerprint) == 0 { key.Fingerprint, err = CalcFingerprint(key.Content) diff --git a/models/asymkey/ssh_key_authorized_keys.go b/models/asymkey/ssh_key_authorized_keys.go index 2e4cd62e5c..db4730f00a 100644 --- a/models/asymkey/ssh_key_authorized_keys.go +++ b/models/asymkey/ssh_key_authorized_keys.go @@ -17,29 +17,13 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" + + "golang.org/x/crypto/ssh" ) -// _____ __ .__ .__ .___ -// / _ \ __ ___/ |_| |__ ___________|__|_______ ____ __| _/ -// / /_\ \| | \ __\ | \ / _ \_ __ \ \___ // __ \ / __ | -// / | \ | /| | | Y ( <_> ) | \/ |/ /\ ___// /_/ | -// \____|__ /____/ |__| |___| /\____/|__| |__/_____ \\___ >____ | -// \/ \/ \/ \/ \/ -// ____ __. -// | |/ _|____ ___.__. ______ -// | <_/ __ < | |/ ___/ -// | | \ ___/\___ |\___ \ -// |____|__ \___ > ____/____ > -// \/ \/\/ \/ -// -// This file contains functions for creating authorized_keys files -// -// There is a dependence on the database within RegeneratePublicKeys however most of these functions probably belong in a module - -const ( - tplCommentPrefix = `# gitea public key` - tplPublicKey = tplCommentPrefix + "\n" + `command=%s,no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty,no-user-rc,restrict %s` + "\n" -) +// AuthorizedStringCommentPrefix is a magic tag +// some functions like RegeneratePublicKeys needs this tag to skip the keys generated by Gitea, while keep other keys +const AuthorizedStringCommentPrefix = `# gitea public key` var sshOpLocker sync.Mutex @@ -50,17 +34,45 @@ func WithSSHOpLocker(f func() error) error { } // AuthorizedStringForKey creates the authorized keys string appropriate for the provided key -func AuthorizedStringForKey(key *PublicKey) string { +func AuthorizedStringForKey(key *PublicKey) (string, error) { sb := &strings.Builder{} - _ = setting.SSH.AuthorizedKeysCommandTemplateTemplate.Execute(sb, map[string]any{ + _, err := writeAuthorizedStringForKey(key, sb) + return sb.String(), err +} + +// WriteAuthorizedStringForValidKey writes the authorized key for the provided key. If the key is invalid, it does nothing. +func WriteAuthorizedStringForValidKey(key *PublicKey, w io.Writer) error { + validKey, err := writeAuthorizedStringForKey(key, w) + if !validKey { + log.Debug("WriteAuthorizedStringForValidKey: key %s is not valid: %v", key, err) + return nil + } + return err +} + +func writeAuthorizedStringForKey(key *PublicKey, w io.Writer) (keyValid bool, err error) { + const tpl = AuthorizedStringCommentPrefix + "\n" + `command=%s,no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty,no-user-rc,restrict %s %s` + "\n" + pubKey, _, _, _, err := ssh.ParseAuthorizedKey([]byte(key.Content)) + if err != nil { + return false, err + } + // now the key is valid, the code below could only return template/IO related errors + sbCmd := &strings.Builder{} + err = setting.SSH.AuthorizedKeysCommandTemplateTemplate.Execute(sbCmd, map[string]any{ "AppPath": util.ShellEscape(setting.AppPath), "AppWorkPath": util.ShellEscape(setting.AppWorkPath), "CustomConf": util.ShellEscape(setting.CustomConf), "CustomPath": util.ShellEscape(setting.CustomPath), "Key": key, }) - - return fmt.Sprintf(tplPublicKey, util.ShellEscape(sb.String()), key.Content) + if err != nil { + return true, err + } + sshCommandEscaped := util.ShellEscape(sbCmd.String()) + sshKeyMarshalled := strings.TrimSpace(string(ssh.MarshalAuthorizedKey(pubKey))) + sshKeyComment := fmt.Sprintf("user-%d", key.OwnerID) + _, err = fmt.Fprintf(w, tpl, sshCommandEscaped, sshKeyMarshalled, sshKeyComment) + return true, err } // appendAuthorizedKeysToFile appends new SSH keys' content to authorized_keys file. @@ -112,7 +124,7 @@ func appendAuthorizedKeysToFile(keys ...*PublicKey) error { if key.Type == KeyTypePrincipal { continue } - if _, err = f.WriteString(key.AuthorizedString()); err != nil { + if err = WriteAuthorizedStringForValidKey(key, f); err != nil { return err } } @@ -120,10 +132,9 @@ func appendAuthorizedKeysToFile(keys ...*PublicKey) error { } // RegeneratePublicKeys regenerates the authorized_keys file -func RegeneratePublicKeys(ctx context.Context, t io.StringWriter) error { +func RegeneratePublicKeys(ctx context.Context, t io.Writer) error { if err := db.GetEngine(ctx).Where("type != ?", KeyTypePrincipal).Iterate(new(PublicKey), func(idx int, bean any) (err error) { - _, err = t.WriteString((bean.(*PublicKey)).AuthorizedString()) - return err + return WriteAuthorizedStringForValidKey(bean.(*PublicKey), t) }); err != nil { return err } @@ -144,11 +155,11 @@ func RegeneratePublicKeys(ctx context.Context, t io.StringWriter) error { scanner := bufio.NewScanner(f) for scanner.Scan() { line := scanner.Text() - if strings.HasPrefix(line, tplCommentPrefix) { + if strings.HasPrefix(line, AuthorizedStringCommentPrefix) { scanner.Scan() continue } - _, err = t.WriteString(line + "\n") + _, err = io.WriteString(t, line+"\n") if err != nil { return err } diff --git a/models/repo/upload.go b/models/repo/upload.go index f7d4749842..b9bda8fdbf 100644 --- a/models/repo/upload.go +++ b/models/repo/upload.go @@ -127,16 +127,9 @@ func DeleteUploads(ctx context.Context, uploads ...*Upload) (err error) { for _, upload := range uploads { localPath := upload.LocalPath() - isFile, err := util.IsFile(localPath) - if err != nil { - log.Error("Unable to check if %s is a file. Error: %v", localPath, err) - } - if !isFile { - continue - } - if err := util.Remove(localPath); err != nil { - return fmt.Errorf("remove upload: %w", err) + // just continue, don't fail the whole operation if a file is missing (removed by others) + log.Error("unable to remove upload file %s: %v", localPath, err) } } diff --git a/modules/git/hook.go b/modules/git/hook.go index 361aa53100..0e19387d97 100644 --- a/modules/git/hook.go +++ b/modules/git/hook.go @@ -47,30 +47,16 @@ func GetHook(repoPath, name string) (*Hook, error) { name: name, path: filepath.Join(repoPath, filepath.Join("hooks", name+".d", name)), } - isFile, err := util.IsFile(h.path) - if err != nil { - return nil, err - } - if isFile { - data, err := os.ReadFile(h.path) - if err != nil { - return nil, err - } + if data, err := os.ReadFile(h.path); err == nil { h.IsActive = true h.Content = string(data) return h, nil + } else if !os.IsNotExist(err) { + return nil, err } samplePath := filepath.Join(repoPath, "hooks", name+".sample") - isFile, err = util.IsFile(samplePath) - if err != nil { - return nil, err - } - if isFile { - data, err := os.ReadFile(samplePath) - if err != nil { - return nil, err - } + if data, err := os.ReadFile(samplePath); err == nil { h.Sample = string(data) } return h, nil diff --git a/modules/setting/config_provider.go b/modules/setting/config_provider.go index 09eaaefdaf..777885cb4f 100644 --- a/modules/setting/config_provider.go +++ b/modules/setting/config_provider.go @@ -202,11 +202,11 @@ func NewConfigProviderFromFile(file string) (ConfigProvider, error) { loadedFromEmpty := true if file != "" { - isFile, err := util.IsFile(file) + isExist, err := util.IsExist(file) if err != nil { - return nil, fmt.Errorf("unable to check if %q is a file. Error: %v", file, err) + return nil, fmt.Errorf("unable to check if %q exists: %v", file, err) } - if isFile { + if isExist { if err = cfg.Append(file); err != nil { return nil, fmt.Errorf("failed to load config file %q: %v", file, err) } diff --git a/modules/util/path.go b/modules/util/path.go index 0e56348978..0cb8ab7ece 100644 --- a/modules/util/path.go +++ b/modules/util/path.go @@ -115,15 +115,10 @@ func IsDir(dir string) (bool, error) { return false, err } -// IsFile returns true if given path is a file, -// or returns false when it's a directory or does not exist. -func IsFile(filePath string) (bool, error) { - f, err := os.Stat(filePath) +func IsRegularFile(filePath string) (bool, error) { + f, err := os.Lstat(filePath) if err == nil { - return !f.IsDir(), nil - } - if os.IsNotExist(err) { - return false, nil + return f.Mode().IsRegular(), nil } return false, err } diff --git a/routers/private/key.go b/routers/private/key.go index 9fd0a16c07..2be80477a9 100644 --- a/routers/private/key.go +++ b/routers/private/key.go @@ -45,7 +45,7 @@ func UpdatePublicKeyInRepo(ctx *context.PrivateContext) { ctx.PlainText(http.StatusOK, "success") } -// AuthorizedPublicKeyByContent searches content as prefix (leak e-mail part) +// AuthorizedPublicKeyByContent searches content as prefix (without comment part) // and returns public key found. func AuthorizedPublicKeyByContent(ctx *context.PrivateContext) { content := ctx.FormString("content") @@ -57,5 +57,14 @@ func AuthorizedPublicKeyByContent(ctx *context.PrivateContext) { }) return } - ctx.PlainText(http.StatusOK, publicKey.AuthorizedString()) + + authorizedString, err := asymkey_model.AuthorizedStringForKey(publicKey) + if err != nil { + ctx.JSON(http.StatusInternalServerError, private.Response{ + Err: err.Error(), + UserMsg: "invalid public key", + }) + return + } + ctx.PlainText(http.StatusOK, authorizedString) } diff --git a/services/asymkey/ssh_key_authorized_principals.go b/services/asymkey/ssh_key_authorized_principals.go index 2838bb5fc7..b06d8d2a40 100644 --- a/services/asymkey/ssh_key_authorized_principals.go +++ b/services/asymkey/ssh_key_authorized_principals.go @@ -25,10 +25,7 @@ import ( // There is a dependence on the database within RewriteAllPrincipalKeys & RegeneratePrincipalKeys // The sshOpLocker is used from ssh_key_authorized_keys.go -const ( - authorizedPrincipalsFile = "authorized_principals" - tplCommentPrefix = `# gitea public key` -) +const authorizedPrincipalsFile = "authorized_principals" // RewriteAllPrincipalKeys removes any authorized principal and rewrite all keys from database again. // Note: db.GetEngine(ctx).Iterate does not get latest data after insert/delete, so we have to call this function @@ -90,10 +87,9 @@ func rewriteAllPrincipalKeys(ctx context.Context) error { return util.Rename(tmpPath, fPath) } -func regeneratePrincipalKeys(ctx context.Context, t io.StringWriter) error { +func regeneratePrincipalKeys(ctx context.Context, t io.Writer) error { if err := db.GetEngine(ctx).Where("type = ?", asymkey_model.KeyTypePrincipal).Iterate(new(asymkey_model.PublicKey), func(idx int, bean any) (err error) { - _, err = t.WriteString((bean.(*asymkey_model.PublicKey)).AuthorizedString()) - return err + return asymkey_model.WriteAuthorizedStringForValidKey(bean.(*asymkey_model.PublicKey), t) }); err != nil { return err } @@ -114,11 +110,11 @@ func regeneratePrincipalKeys(ctx context.Context, t io.StringWriter) error { scanner := bufio.NewScanner(f) for scanner.Scan() { line := scanner.Text() - if strings.HasPrefix(line, tplCommentPrefix) { + if strings.HasPrefix(line, asymkey_model.AuthorizedStringCommentPrefix) { scanner.Scan() continue } - _, err = t.WriteString(line + "\n") + _, err = io.WriteString(t, line+"\n") if err != nil { return err } diff --git a/services/doctor/authorizedkeys.go b/services/doctor/authorizedkeys.go index 46e7099dce..fe1dc639e8 100644 --- a/services/doctor/authorizedkeys.go +++ b/services/doctor/authorizedkeys.go @@ -20,8 +20,6 @@ import ( asymkey_service "code.gitea.io/gitea/services/asymkey" ) -const tplCommentPrefix = `# gitea public key` - func checkAuthorizedKeys(ctx context.Context, logger log.Logger, autofix bool) error { if setting.SSH.StartBuiltinServer || !setting.SSH.CreateAuthorizedKeysFile { return nil @@ -47,7 +45,7 @@ func checkAuthorizedKeys(ctx context.Context, logger log.Logger, autofix bool) e scanner := bufio.NewScanner(f) for scanner.Scan() { line := scanner.Text() - if strings.HasPrefix(line, tplCommentPrefix) { + if strings.HasPrefix(line, asymkey_model.AuthorizedStringCommentPrefix) { continue } linesInAuthorizedKeys.Add(line) @@ -67,7 +65,7 @@ func checkAuthorizedKeys(ctx context.Context, logger log.Logger, autofix bool) e scanner = bufio.NewScanner(regenerated) for scanner.Scan() { line := scanner.Text() - if strings.HasPrefix(line, tplCommentPrefix) { + if strings.HasPrefix(line, asymkey_model.AuthorizedStringCommentPrefix) { continue } if linesInAuthorizedKeys.Contains(line) { diff --git a/services/lfs/server.go b/services/lfs/server.go index 9f2e532f23..4a1e0aaf7d 100644 --- a/services/lfs/server.go +++ b/services/lfs/server.go @@ -18,6 +18,7 @@ import ( "regexp" "strconv" "strings" + "time" actions_model "code.gitea.io/gitea/models/actions" auth_model "code.gitea.io/gitea/models/auth" @@ -54,6 +55,33 @@ type Claims struct { jwt.RegisteredClaims } +type AuthTokenOptions struct { + Op string + UserID int64 + RepoID int64 +} + +func GetLFSAuthTokenWithBearer(opts AuthTokenOptions) (string, error) { + now := time.Now() + claims := Claims{ + RegisteredClaims: jwt.RegisteredClaims{ + ExpiresAt: jwt.NewNumericDate(now.Add(setting.LFS.HTTPAuthExpiry)), + NotBefore: jwt.NewNumericDate(now), + }, + RepoID: opts.RepoID, + Op: opts.Op, + UserID: opts.UserID, + } + token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims) + + // Sign and get the complete encoded token as a string using the secret + tokenString, err := token.SignedString(setting.LFS.JWTSecretBytes) + if err != nil { + return "", fmt.Errorf("failed to sign LFS JWT token: %w", err) + } + return "Bearer " + tokenString, nil +} + // DownloadLink builds a URL to download the object. func (rc *requestContext) DownloadLink(p lfs_module.Pointer) string { return setting.AppURL + path.Join(url.PathEscape(rc.User), url.PathEscape(rc.Repo+".git"), "info/lfs/objects", url.PathEscape(p.Oid)) @@ -559,9 +587,6 @@ func authenticate(ctx *context.Context, repository *repo_model.Repository, autho } func handleLFSToken(ctx stdCtx.Context, tokenSHA string, target *repo_model.Repository, mode perm_model.AccessMode) (*user_model.User, error) { - if !strings.Contains(tokenSHA, ".") { - return nil, nil - } token, err := jwt.ParseWithClaims(tokenSHA, &Claims{}, func(t *jwt.Token) (any, error) { if _, ok := t.Method.(*jwt.SigningMethodHMAC); !ok { return nil, fmt.Errorf("unexpected signing method: %v", t.Header["alg"]) @@ -569,7 +594,7 @@ func handleLFSToken(ctx stdCtx.Context, tokenSHA string, target *repo_model.Repo return setting.LFS.JWTSecretBytes, nil }) if err != nil { - return nil, nil + return nil, errors.New("invalid token") } claims, claimsOk := token.Claims.(*Claims) diff --git a/services/lfs/server_test.go b/services/lfs/server_test.go new file mode 100644 index 0000000000..e66e48cee8 --- /dev/null +++ b/services/lfs/server_test.go @@ -0,0 +1,51 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package lfs + +import ( + "strings" + "testing" + + perm_model "code.gitea.io/gitea/models/perm" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unittest" + "code.gitea.io/gitea/services/contexttest" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMain(m *testing.M) { + unittest.MainTest(m) +} + +func TestAuthenticate(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + + token2, _ := GetLFSAuthTokenWithBearer(AuthTokenOptions{Op: "download", UserID: 2, RepoID: 1}) + _, token2, _ = strings.Cut(token2, " ") + ctx, _ := contexttest.MockContext(t, "/") + + t.Run("handleLFSToken", func(t *testing.T) { + u, err := handleLFSToken(ctx, "", repo1, perm_model.AccessModeRead) + require.Error(t, err) + assert.Nil(t, u) + + u, err = handleLFSToken(ctx, "invalid", repo1, perm_model.AccessModeRead) + require.Error(t, err) + assert.Nil(t, u) + + u, err = handleLFSToken(ctx, token2, repo1, perm_model.AccessModeRead) + require.NoError(t, err) + assert.EqualValues(t, 2, u.ID) + }) + + t.Run("authenticate", func(t *testing.T) { + const prefixBearer = "Bearer " + assert.False(t, authenticate(ctx, repo1, "", true, false)) + assert.False(t, authenticate(ctx, repo1, prefixBearer+"invalid", true, false)) + assert.True(t, authenticate(ctx, repo1, prefixBearer+token2, true, false)) + }) +} diff --git a/services/repository/generate.go b/services/repository/generate.go index 8e7f9a45b0..062c6f4fb1 100644 --- a/services/repository/generate.go +++ b/services/repository/generate.go @@ -13,6 +13,7 @@ import ( "regexp" "strconv" "strings" + "sync" "time" git_model "code.gitea.io/gitea/models/git" @@ -40,29 +41,41 @@ type expansion struct { Transformers []transformer } -var defaultTransformers = []transformer{ - {Name: "SNAKE", Transform: xstrings.ToSnakeCase}, - {Name: "KEBAB", Transform: xstrings.ToKebabCase}, - {Name: "CAMEL", Transform: xstrings.ToCamelCase}, - {Name: "PASCAL", Transform: xstrings.ToPascalCase}, - {Name: "LOWER", Transform: strings.ToLower}, - {Name: "UPPER", Transform: strings.ToUpper}, - {Name: "TITLE", Transform: util.ToTitleCase}, -} +var globalVars = sync.OnceValue(func() (ret struct { + defaultTransformers []transformer + fileNameSanitizeRegexp *regexp.Regexp +}, +) { + ret.defaultTransformers = []transformer{ + {Name: "SNAKE", Transform: xstrings.ToSnakeCase}, + {Name: "KEBAB", Transform: xstrings.ToKebabCase}, + {Name: "CAMEL", Transform: xstrings.ToCamelCase}, + {Name: "PASCAL", Transform: xstrings.ToPascalCase}, + {Name: "LOWER", Transform: strings.ToLower}, + {Name: "UPPER", Transform: strings.ToUpper}, + {Name: "TITLE", Transform: util.ToTitleCase}, + } -func generateExpansion(ctx context.Context, src string, templateRepo, generateRepo *repo_model.Repository, sanitizeFileName bool) string { + // invalid filename contents, based on https://github.com/sindresorhus/filename-reserved-regex + // "COM10" needs to be opened with UNC "\\.\COM10" on Windows, so itself is valid + ret.fileNameSanitizeRegexp = regexp.MustCompile(`(?i)[<>:"/\\|?*\x{0000}-\x{001F}]|^(con|prn|aux|nul|com\d|lpt\d)$`) + return ret +}) + +func generateExpansion(ctx context.Context, src string, templateRepo, generateRepo *repo_model.Repository) string { + transformers := globalVars().defaultTransformers year, month, day := time.Now().Date() expansions := []expansion{ {Name: "YEAR", Value: strconv.Itoa(year), Transformers: nil}, {Name: "MONTH", Value: fmt.Sprintf("%02d", int(month)), Transformers: nil}, - {Name: "MONTH_ENGLISH", Value: month.String(), Transformers: defaultTransformers}, + {Name: "MONTH_ENGLISH", Value: month.String(), Transformers: transformers}, {Name: "DAY", Value: fmt.Sprintf("%02d", day), Transformers: nil}, - {Name: "REPO_NAME", Value: generateRepo.Name, Transformers: defaultTransformers}, - {Name: "TEMPLATE_NAME", Value: templateRepo.Name, Transformers: defaultTransformers}, + {Name: "REPO_NAME", Value: generateRepo.Name, Transformers: transformers}, + {Name: "TEMPLATE_NAME", Value: templateRepo.Name, Transformers: transformers}, {Name: "REPO_DESCRIPTION", Value: generateRepo.Description, Transformers: nil}, {Name: "TEMPLATE_DESCRIPTION", Value: templateRepo.Description, Transformers: nil}, - {Name: "REPO_OWNER", Value: generateRepo.OwnerName, Transformers: defaultTransformers}, - {Name: "TEMPLATE_OWNER", Value: templateRepo.OwnerName, Transformers: defaultTransformers}, + {Name: "REPO_OWNER", Value: generateRepo.OwnerName, Transformers: transformers}, + {Name: "TEMPLATE_OWNER", Value: templateRepo.OwnerName, Transformers: transformers}, {Name: "REPO_LINK", Value: generateRepo.Link(), Transformers: nil}, {Name: "TEMPLATE_LINK", Value: templateRepo.Link(), Transformers: nil}, {Name: "REPO_HTTPS_URL", Value: generateRepo.CloneLinkGeneral(ctx).HTTPS, Transformers: nil}, @@ -80,32 +93,23 @@ func generateExpansion(ctx context.Context, src string, templateRepo, generateRe } return os.Expand(src, func(key string) string { - if expansion, ok := expansionMap[key]; ok { - if sanitizeFileName { - return fileNameSanitize(expansion) - } - return expansion + if val, ok := expansionMap[key]; ok { + return val } return key }) } -// GiteaTemplate holds information about a .gitea/template file -type GiteaTemplate struct { - Path string - Content []byte - - globs []glob.Glob +// giteaTemplateFileMatcher holds information about a .gitea/template file +type giteaTemplateFileMatcher struct { + LocalFullPath string + globs []glob.Glob } -// Globs parses the .gitea/template globs or returns them if they were already parsed -func (gt *GiteaTemplate) Globs() []glob.Glob { - if gt.globs != nil { - return gt.globs - } - +func newGiteaTemplateFileMatcher(fullPath string, content []byte) *giteaTemplateFileMatcher { + gt := &giteaTemplateFileMatcher{LocalFullPath: fullPath} gt.globs = make([]glob.Glob, 0) - scanner := bufio.NewScanner(bytes.NewReader(gt.Content)) + scanner := bufio.NewScanner(bytes.NewReader(content)) for scanner.Scan() { line := strings.TrimSpace(scanner.Text()) if line == "" || strings.HasPrefix(line, "#") { @@ -113,73 +117,91 @@ func (gt *GiteaTemplate) Globs() []glob.Glob { } g, err := glob.Compile(line, '/') if err != nil { - log.Info("Invalid glob expression '%s' (skipped): %v", line, err) + log.Debug("Invalid glob expression '%s' (skipped): %v", line, err) continue } gt.globs = append(gt.globs, g) } - return gt.globs + return gt } -func readGiteaTemplateFile(tmpDir string) (*GiteaTemplate, error) { - gtPath := filepath.Join(tmpDir, ".gitea", "template") - if _, err := os.Stat(gtPath); os.IsNotExist(err) { +func (gt *giteaTemplateFileMatcher) HasRules() bool { + return len(gt.globs) != 0 +} + +func (gt *giteaTemplateFileMatcher) Match(s string) bool { + for _, g := range gt.globs { + if g.Match(s) { + return true + } + } + return false +} + +func readGiteaTemplateFile(tmpDir string) (*giteaTemplateFileMatcher, error) { + localPath := filepath.Join(tmpDir, ".gitea", "template") + if _, err := os.Stat(localPath); os.IsNotExist(err) { return nil, nil } else if err != nil { return nil, err } - content, err := os.ReadFile(gtPath) + content, err := os.ReadFile(localPath) if err != nil { return nil, err } - return &GiteaTemplate{Path: gtPath, Content: content}, nil + return newGiteaTemplateFileMatcher(localPath, content), nil } -func processGiteaTemplateFile(ctx context.Context, tmpDir string, templateRepo, generateRepo *repo_model.Repository, giteaTemplateFile *GiteaTemplate) error { - if err := util.Remove(giteaTemplateFile.Path); err != nil { - return fmt.Errorf("remove .giteatemplate: %w", err) +func substGiteaTemplateFile(ctx context.Context, tmpDir, tmpDirSubPath string, templateRepo, generateRepo *repo_model.Repository) error { + tmpFullPath := filepath.Join(tmpDir, tmpDirSubPath) + if ok, err := util.IsRegularFile(tmpFullPath); !ok { + return err } - if len(giteaTemplateFile.Globs()) == 0 { + + content, err := os.ReadFile(tmpFullPath) + if err != nil { + return err + } + if err := util.Remove(tmpFullPath); err != nil { + return err + } + + generatedContent := generateExpansion(ctx, string(content), templateRepo, generateRepo) + substSubPath := filepath.Clean(filePathSanitize(generateExpansion(ctx, tmpDirSubPath, templateRepo, generateRepo))) + newLocalPath := filepath.Join(tmpDir, substSubPath) + regular, err := util.IsRegularFile(newLocalPath) + if canWrite := regular || os.IsNotExist(err); !canWrite { + return nil + } + if err := os.MkdirAll(filepath.Dir(newLocalPath), 0o755); err != nil { + return err + } + return os.WriteFile(newLocalPath, []byte(generatedContent), 0o644) +} + +func processGiteaTemplateFile(ctx context.Context, tmpDir string, templateRepo, generateRepo *repo_model.Repository, fileMatcher *giteaTemplateFileMatcher) error { + if err := util.Remove(fileMatcher.LocalFullPath); err != nil { + return fmt.Errorf("unable to remove .gitea/template: %w", err) + } + if !fileMatcher.HasRules() { return nil // Avoid walking tree if there are no globs } - tmpDirSlash := strings.TrimSuffix(filepath.ToSlash(tmpDir), "/") + "/" - return filepath.WalkDir(tmpDirSlash, func(path string, d os.DirEntry, walkErr error) error { + + return filepath.WalkDir(tmpDir, func(fullPath string, d os.DirEntry, walkErr error) error { if walkErr != nil { return walkErr } - if d.IsDir() { return nil } - - base := strings.TrimPrefix(filepath.ToSlash(path), tmpDirSlash) - for _, g := range giteaTemplateFile.Globs() { - if g.Match(base) { - content, err := os.ReadFile(path) - if err != nil { - return err - } - - generatedContent := []byte(generateExpansion(ctx, string(content), templateRepo, generateRepo, false)) - if err := os.WriteFile(path, generatedContent, 0o644); err != nil { - return err - } - - substPath := filepath.FromSlash(filepath.Join(tmpDirSlash, generateExpansion(ctx, base, templateRepo, generateRepo, true))) - - // Create parent subdirectories if needed or continue silently if it exists - if err = os.MkdirAll(filepath.Dir(substPath), 0o755); err != nil { - return err - } - - // Substitute filename variables - if err = os.Rename(path, substPath); err != nil { - return err - } - break - } + tmpDirSubPath, err := filepath.Rel(tmpDir, fullPath) + if err != nil { + return err + } + if fileMatcher.Match(filepath.ToSlash(tmpDirSubPath)) { + return substGiteaTemplateFile(ctx, tmpDir, tmpDirSubPath, templateRepo, generateRepo) } return nil }) // end: WalkDir @@ -219,13 +241,13 @@ func generateRepoCommit(ctx context.Context, repo, templateRepo, generateRepo *r } // Variable expansion - giteaTemplateFile, err := readGiteaTemplateFile(tmpDir) + fileMatcher, err := readGiteaTemplateFile(tmpDir) if err != nil { return fmt.Errorf("readGiteaTemplateFile: %w", err) } - if giteaTemplateFile != nil { - err = processGiteaTemplateFile(ctx, tmpDir, templateRepo, generateRepo, giteaTemplateFile) + if fileMatcher != nil { + err = processGiteaTemplateFile(ctx, tmpDir, templateRepo, generateRepo, fileMatcher) if err != nil { return err } @@ -317,12 +339,17 @@ func (gro GenerateRepoOptions) IsValid() bool { gro.IssueLabels || gro.ProtectedBranch // or other items as they are added } -var fileNameSanitizeRegexp = regexp.MustCompile(`(?i)\.\.|[<>:\"/\\|?*\x{0000}-\x{001F}]|^(con|prn|aux|nul|com\d|lpt\d)$`) - -// Sanitize user input to valid OS filenames -// -// Based on https://github.com/sindresorhus/filename-reserved-regex -// Adds ".." to prevent directory traversal -func fileNameSanitize(s string) string { - return strings.TrimSpace(fileNameSanitizeRegexp.ReplaceAllString(s, "_")) +func filePathSanitize(s string) string { + fields := strings.Split(filepath.ToSlash(s), "/") + for i, field := range fields { + field = strings.TrimSpace(strings.TrimSpace(globalVars().fileNameSanitizeRegexp.ReplaceAllString(field, "_"))) + if strings.HasPrefix(field, "..") { + field = "__" + field[2:] + } + if strings.EqualFold(field, ".git") { + field = "_" + field[1:] + } + fields[i] = field + } + return filepath.FromSlash(strings.Join(fields, "/")) } diff --git a/services/repository/generate_test.go b/services/repository/generate_test.go index 1163c392c9..19b84c7bde 100644 --- a/services/repository/generate_test.go +++ b/services/repository/generate_test.go @@ -4,13 +4,18 @@ package repository import ( + "os" + "path/filepath" "testing" + repo_model "code.gitea.io/gitea/models/repo" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -var giteaTemplate = []byte(` +func TestGiteaTemplate(t *testing.T) { + giteaTemplate := []byte(` # Header # All .go files @@ -23,48 +28,153 @@ text/*.txt **/modules/* `) -func TestGiteaTemplate(t *testing.T) { - gt := GiteaTemplate{Content: giteaTemplate} - assert.Len(t, gt.Globs(), 3) + gt := newGiteaTemplateFileMatcher("", giteaTemplate) + assert.Len(t, gt.globs, 3) tt := []struct { Path string Match bool }{ {Path: "main.go", Match: true}, - {Path: "a/b/c/d/e.go", Match: true}, - {Path: "main.txt", Match: false}, - {Path: "a/b.txt", Match: false}, + {Path: "sub/sub/foo.go", Match: true}, + + {Path: "a.txt", Match: false}, {Path: "text/a.txt", Match: true}, - {Path: "text/b.txt", Match: true}, - {Path: "text/c.json", Match: false}, + {Path: "sub/text/a.txt", Match: false}, + {Path: "text/a.json", Match: false}, + {Path: "a/b/c/modules/README.md", Match: true}, {Path: "a/b/c/modules/d/README.md", Match: false}, } for _, tc := range tt { - t.Run(tc.Path, func(t *testing.T) { - match := false - for _, g := range gt.Globs() { - if g.Match(tc.Path) { - match = true - break - } - } - assert.Equal(t, tc.Match, match) - }) + assert.Equal(t, tc.Match, gt.Match(tc.Path), "path: %s", tc.Path) } } -func TestFileNameSanitize(t *testing.T) { - assert.Equal(t, "test_CON", fileNameSanitize("test_CON")) - assert.Equal(t, "test CON", fileNameSanitize("test CON ")) - assert.Equal(t, "__traverse__", fileNameSanitize("../traverse/..")) - assert.Equal(t, "http___localhost_3003_user_test.git", fileNameSanitize("http://localhost:3003/user/test.git")) - assert.Equal(t, "_", fileNameSanitize("CON")) - assert.Equal(t, "_", fileNameSanitize("con")) - assert.Equal(t, "_", fileNameSanitize("\u0000")) - assert.Equal(t, "目标", fileNameSanitize("目标")) +func TestFilePathSanitize(t *testing.T) { + assert.Equal(t, "test_CON", filePathSanitize("test_CON")) + assert.Equal(t, "test CON", filePathSanitize("test CON ")) + assert.Equal(t, "__/traverse/__", filePathSanitize(".. /traverse/ ..")) + assert.Equal(t, "./__/a/_git/b_", filePathSanitize("./../a/.git/ b: ")) + assert.Equal(t, "_", filePathSanitize("CoN")) + assert.Equal(t, "_", filePathSanitize("LpT1")) + assert.Equal(t, "_", filePathSanitize("CoM1")) + assert.Equal(t, "_", filePathSanitize("\u0000")) + assert.Equal(t, "目标", filePathSanitize("目标")) + // unlike filepath.Clean, it only sanitizes, doesn't change the separator layout + assert.Equal(t, "", filePathSanitize("")) //nolint:testifylint // for easy reading + assert.Equal(t, ".", filePathSanitize(".")) + assert.Equal(t, "/", filePathSanitize("/")) +} + +func TestProcessGiteaTemplateFile(t *testing.T) { + tmpDir := filepath.Join(t.TempDir(), "gitea-template-test") + + assertFileContent := func(path, expected string) { + data, err := os.ReadFile(filepath.Join(tmpDir, path)) + if expected == "" { + assert.ErrorIs(t, err, os.ErrNotExist) + return + } + require.NoError(t, err) + assert.Equal(t, expected, string(data), "file content mismatch for %s", path) + } + + assertSymLink := func(path, expected string) { + link, err := os.Readlink(filepath.Join(tmpDir, path)) + if expected == "" { + assert.ErrorIs(t, err, os.ErrNotExist) + return + } + require.NoError(t, err) + assert.Equal(t, expected, link, "symlink target mismatch for %s", path) + } + + require.NoError(t, os.MkdirAll(tmpDir+"/.gitea", 0o755)) + require.NoError(t, os.WriteFile(tmpDir+"/.gitea/template", []byte("*\ninclude/**"), 0o644)) + require.NoError(t, os.MkdirAll(tmpDir+"/sub", 0o755)) + require.NoError(t, os.MkdirAll(tmpDir+"/include/foo/bar", 0o755)) + + require.NoError(t, os.WriteFile(tmpDir+"/sub/link-target", []byte("link target content from ${TEMPLATE_NAME}"), 0o644)) + require.NoError(t, os.WriteFile(tmpDir+"/include/foo/bar/test.txt", []byte("include subdir ${TEMPLATE_NAME}"), 0o644)) + + // case-1 + { + require.NoError(t, os.WriteFile(tmpDir+"/normal", []byte("normal content"), 0o644)) + require.NoError(t, os.WriteFile(tmpDir+"/template", []byte("template from ${TEMPLATE_NAME}"), 0o644)) + } + + // case-2 + { + require.NoError(t, os.Symlink(tmpDir+"/sub/link-target", tmpDir+"/link")) + } + + // case-3 + { + require.NoError(t, os.WriteFile(tmpDir+"/subst-${REPO_NAME}", []byte("dummy subst repo name"), 0o644)) + } + + // case-4 + assertSubstTemplateName := func(normalContent, toLinkContent, fromLinkContent string) { + assertFileContent("subst-${TEMPLATE_NAME}-normal", normalContent) + assertFileContent("subst-${TEMPLATE_NAME}-to-link", toLinkContent) + assertFileContent("subst-${TEMPLATE_NAME}-from-link", fromLinkContent) + } + { + // will succeed + require.NoError(t, os.WriteFile(tmpDir+"/subst-${TEMPLATE_NAME}-normal", []byte("dummy subst template name normal"), 0o644)) + // will skil if the path subst result is a link + require.NoError(t, os.WriteFile(tmpDir+"/subst-${TEMPLATE_NAME}-to-link", []byte("dummy subst template name to link"), 0o644)) + require.NoError(t, os.Symlink(tmpDir+"/sub/link-target", tmpDir+"/subst-TemplateRepoName-to-link")) + // will be skipped since the source is a symlink + require.NoError(t, os.Symlink(tmpDir+"/sub/link-target", tmpDir+"/subst-${TEMPLATE_NAME}-from-link")) + // pre-check + assertSubstTemplateName("dummy subst template name normal", "dummy subst template name to link", "link target content from ${TEMPLATE_NAME}") + } + + // process the template files + { + templateRepo := &repo_model.Repository{Name: "TemplateRepoName"} + generatedRepo := &repo_model.Repository{Name: "/../.gIt/name"} + fileMatcher, _ := readGiteaTemplateFile(tmpDir) + err := processGiteaTemplateFile(t.Context(), tmpDir, templateRepo, generatedRepo, fileMatcher) + require.NoError(t, err) + assertFileContent("include/foo/bar/test.txt", "include subdir TemplateRepoName") + } + + // the lin target should never be modified, and since it is in a subdirectory, it is not affected by the template either + assertFileContent("sub/link-target", "link target content from ${TEMPLATE_NAME}") + + // case-1 + { + assertFileContent("no-such", "") + assertFileContent("normal", "normal content") + assertFileContent("template", "template from TemplateRepoName") + } + + // case-2 + { + // symlink with templates should be preserved (not read or write) + assertSymLink("link", tmpDir+"/sub/link-target") + } + + // case-3 + { + assertFileContent("subst-${REPO_NAME}", "") + assertFileContent("subst-/__/_gIt/name", "dummy subst repo name") + } + + // case-4 + { + // the paths with templates should have been removed, subst to a regular file, succeed, the link is preserved + assertSubstTemplateName("", "", "link target content from ${TEMPLATE_NAME}") + assertFileContent("subst-TemplateRepoName-normal", "dummy subst template name normal") + // subst to a link, skip, and the target is unchanged + assertSymLink("subst-TemplateRepoName-to-link", tmpDir+"/sub/link-target") + // subst from a link, skip, and the target is unchanged + assertSymLink("subst-${TEMPLATE_NAME}-from-link", tmpDir+"/sub/link-target") + } } func TestTransformers(t *testing.T) { @@ -82,9 +192,9 @@ func TestTransformers(t *testing.T) { } input := "Abc_Def-XYZ" - assert.Len(t, defaultTransformers, len(cases)) + assert.Len(t, globalVars().defaultTransformers, len(cases)) for i, c := range cases { - tf := defaultTransformers[i] + tf := globalVars().defaultTransformers[i] require.Equal(t, c.name, tf.Name) assert.Equal(t, c.expected, tf.Transform(input), "case %s", c.name) } diff --git a/tests/integration/cmd_keys_test.go b/tests/integration/cmd_keys_test.go index 3878302ef0..be226a01ec 100644 --- a/tests/integration/cmd_keys_test.go +++ b/tests/integration/cmd_keys_test.go @@ -30,7 +30,7 @@ func Test_CmdKeys(t *testing.T) { "with_key", []string{"keys", "-e", "git", "-u", "git", "-t", "ssh-rsa", "-k", "AAAAB3NzaC1yc2EAAAADAQABAAABgQDWVj0fQ5N8wNc0LVNA41wDLYJ89ZIbejrPfg/avyj3u/ZohAKsQclxG4Ju0VirduBFF9EOiuxoiFBRr3xRpqzpsZtnMPkWVWb+akZwBFAx8p+jKdy4QXR/SZqbVobrGwip2UjSrri1CtBxpJikojRIZfCnDaMOyd9Jp6KkujvniFzUWdLmCPxUE9zhTaPu0JsEP7MW0m6yx7ZUhHyfss+NtqmFTaDO+QlMR7L2QkDliN2Jl3Xa3PhuWnKJfWhdAq1Cw4oraKUOmIgXLkuiuxVQ6mD3AiFupkmfqdHq6h+uHHmyQqv3gU+/sD8GbGAhf6ftqhTsXjnv1Aj4R8NoDf9BS6KRkzkeun5UisSzgtfQzjOMEiJtmrep2ZQrMGahrXa+q4VKr0aKJfm+KlLfwm/JztfsBcqQWNcTURiCFqz+fgZw0Ey/de0eyMzldYTdXXNRYCKjs9bvBK+6SSXRM7AhftfQ0ZuoW5+gtinPrnmoOaSCEJbAiEiTO/BzOHgowiM="}, false, - "# gitea public key\ncommand=\"" + setting.AppPath + " --config=" + util.ShellEscape(setting.CustomConf) + " serv key-1\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty,no-user-rc,restrict ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQDWVj0fQ5N8wNc0LVNA41wDLYJ89ZIbejrPfg/avyj3u/ZohAKsQclxG4Ju0VirduBFF9EOiuxoiFBRr3xRpqzpsZtnMPkWVWb+akZwBFAx8p+jKdy4QXR/SZqbVobrGwip2UjSrri1CtBxpJikojRIZfCnDaMOyd9Jp6KkujvniFzUWdLmCPxUE9zhTaPu0JsEP7MW0m6yx7ZUhHyfss+NtqmFTaDO+QlMR7L2QkDliN2Jl3Xa3PhuWnKJfWhdAq1Cw4oraKUOmIgXLkuiuxVQ6mD3AiFupkmfqdHq6h+uHHmyQqv3gU+/sD8GbGAhf6ftqhTsXjnv1Aj4R8NoDf9BS6KRkzkeun5UisSzgtfQzjOMEiJtmrep2ZQrMGahrXa+q4VKr0aKJfm+KlLfwm/JztfsBcqQWNcTURiCFqz+fgZw0Ey/de0eyMzldYTdXXNRYCKjs9bvBK+6SSXRM7AhftfQ0ZuoW5+gtinPrnmoOaSCEJbAiEiTO/BzOHgowiM= user2@localhost\n", + "# gitea public key\ncommand=\"" + setting.AppPath + " --config=" + util.ShellEscape(setting.CustomConf) + " serv key-1\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty,no-user-rc,restrict ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQDWVj0fQ5N8wNc0LVNA41wDLYJ89ZIbejrPfg/avyj3u/ZohAKsQclxG4Ju0VirduBFF9EOiuxoiFBRr3xRpqzpsZtnMPkWVWb+akZwBFAx8p+jKdy4QXR/SZqbVobrGwip2UjSrri1CtBxpJikojRIZfCnDaMOyd9Jp6KkujvniFzUWdLmCPxUE9zhTaPu0JsEP7MW0m6yx7ZUhHyfss+NtqmFTaDO+QlMR7L2QkDliN2Jl3Xa3PhuWnKJfWhdAq1Cw4oraKUOmIgXLkuiuxVQ6mD3AiFupkmfqdHq6h+uHHmyQqv3gU+/sD8GbGAhf6ftqhTsXjnv1Aj4R8NoDf9BS6KRkzkeun5UisSzgtfQzjOMEiJtmrep2ZQrMGahrXa+q4VKr0aKJfm+KlLfwm/JztfsBcqQWNcTURiCFqz+fgZw0Ey/de0eyMzldYTdXXNRYCKjs9bvBK+6SSXRM7AhftfQ0ZuoW5+gtinPrnmoOaSCEJbAiEiTO/BzOHgowiM= user-2\n", }, {"invalid", []string{"keys", "--not-a-flag=git"}, true, "Incorrect Usage: flag provided but not defined: -not-a-flag\n\n"}, } From a2eea2fb2ece46b9bba6a2c8c4ad9550eca47527 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 21 Oct 2025 13:19:29 +0800 Subject: [PATCH 06/29] Fix various trivial problems (#35714) --- services/automergequeue/automergequeue.go | 3 ++- web_src/css/repo.css | 12 ++++++++++-- web_src/js/components/RepoActionView.vue | 3 ++- web_src/js/components/RepoCodeFrequency.vue | 2 +- web_src/js/components/RepoContributors.vue | 2 +- web_src/js/components/RepoRecentCommits.vue | 2 +- 6 files changed, 17 insertions(+), 7 deletions(-) diff --git a/services/automergequeue/automergequeue.go b/services/automergequeue/automergequeue.go index fa9c04da87..e8cc4512a7 100644 --- a/services/automergequeue/automergequeue.go +++ b/services/automergequeue/automergequeue.go @@ -5,6 +5,7 @@ package automergequeue import ( "context" + "errors" "fmt" issues_model "code.gitea.io/gitea/models/issues" @@ -17,7 +18,7 @@ var AutoMergeQueue *queue.WorkerPoolQueue[string] var AddToQueue = func(pr *issues_model.PullRequest, sha string) { log.Trace("Adding pullID: %d to the pull requests patch checking queue with sha %s", pr.ID, sha) - if err := AutoMergeQueue.Push(fmt.Sprintf("%d_%s", pr.ID, sha)); err != nil { + if err := AutoMergeQueue.Push(fmt.Sprintf("%d_%s", pr.ID, sha)); err != nil && !errors.Is(err, queue.ErrAlreadyInQueue) { log.Error("Error adding pullID: %d to the pull requests patch checking queue %v", pr.ID, err) } } diff --git a/web_src/css/repo.css b/web_src/css/repo.css index c70937147a..070623d24e 100644 --- a/web_src/css/repo.css +++ b/web_src/css/repo.css @@ -532,6 +532,10 @@ td .commit-summary { color: var(--color-text-light); } +.repository.view.issue .comment-list .timeline-item .comment-text-line .ui.label { + line-height: 1.5; /* label has background, so it can't use parent's line-height */ +} + .repository.view.issue .comment-list .timeline-item .comment-text-line a { color: inherit; } @@ -654,8 +658,8 @@ td .commit-summary { .repository.view.issue .comment-list .code-comment .comment-header { background: transparent; - border-bottom: 0 !important; - padding: 0 !important; + border-bottom: 0; + padding: 0; } .repository.view.issue .comment-list .code-comment .comment-content { @@ -1311,6 +1315,10 @@ td .commit-summary { gap: 0.25em; } +.comment-header.avatar-content-left-arrow::after { + border-right-color: var(--color-box-header); +} + .comment-header.arrow-top::before, .comment-header.arrow-top::after { transform: rotate(90deg); diff --git a/web_src/js/components/RepoActionView.vue b/web_src/js/components/RepoActionView.vue index 6e6733c7d0..24c0f5300c 100644 --- a/web_src/js/components/RepoActionView.vue +++ b/web_src/js/components/RepoActionView.vue @@ -565,7 +565,7 @@ export default defineComponent({
    -
    @@ -667,6 +667,7 @@ export default defineComponent({ .action-commit-summary { display: flex; + align-items: center; flex-wrap: wrap; gap: 5px; margin-left: 28px; diff --git a/web_src/js/components/RepoCodeFrequency.vue b/web_src/js/components/RepoCodeFrequency.vue index f331a26fe9..9afacbbf24 100644 --- a/web_src/js/components/RepoCodeFrequency.vue +++ b/web_src/js/components/RepoCodeFrequency.vue @@ -148,7 +148,7 @@ const options: ChartOptions<'line'> = { {{ isLoading ? locale.loadingTitle : errorText ? locale.loadingTitleFailed: `Code frequency over the history of ${repoLink.slice(1)}` }}
    -
    +
    {{ locale.loadingInfo }} diff --git a/web_src/js/components/RepoContributors.vue b/web_src/js/components/RepoContributors.vue index 8a2c9cfed8..c3a32fe3fc 100644 --- a/web_src/js/components/RepoContributors.vue +++ b/web_src/js/components/RepoContributors.vue @@ -379,7 +379,7 @@ export default defineComponent({
    -
    +
    {{ locale.loadingInfo }} diff --git a/web_src/js/components/RepoRecentCommits.vue b/web_src/js/components/RepoRecentCommits.vue index 27aa27dfc3..8ca825993b 100644 --- a/web_src/js/components/RepoRecentCommits.vue +++ b/web_src/js/components/RepoRecentCommits.vue @@ -126,7 +126,7 @@ const options: ChartOptions<'bar'> = { {{ isLoading ? locale.loadingTitle : errorText ? locale.loadingTitleFailed: "Number of commits in the past year" }}
    -
    +
    {{ locale.loadingInfo }} From 3917d2746739d154cd927daa7d1d265f4bd764f0 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 21 Oct 2025 15:30:24 +0800 Subject: [PATCH 07/29] Make restricted users can access public repositories (#35693) Fix #35690 Change the "restricted user" behavior introduced by #6274. Now restricted user can also access public repositories when sign-in is not required. For required sign-in, the behavior isn't changed. --- models/organization/org.go | 4 ++ models/organization/org_test.go | 8 ++++ models/perm/access/access.go | 9 +++- models/perm/access/access_test.go | 10 +++- models/repo/repo_list.go | 25 ++++++---- models/repo/repo_list_test.go | 78 ++++++++++++++++++++++++++++--- 6 files changed, 116 insertions(+), 18 deletions(-) diff --git a/models/organization/org.go b/models/organization/org.go index 9ece044d6c..b4d28f5405 100644 --- a/models/organization/org.go +++ b/models/organization/org.go @@ -429,6 +429,10 @@ func HasOrgOrUserVisible(ctx context.Context, orgOrUser, user *user_model.User) return true } + if !setting.Service.RequireSignInViewStrict && orgOrUser.Visibility == structs.VisibleTypePublic { + return true + } + if (orgOrUser.Visibility == structs.VisibleTypePrivate || user.IsRestricted) && !OrgFromUser(orgOrUser).hasMemberWithUserID(ctx, user.ID) { return false } diff --git a/models/organization/org_test.go b/models/organization/org_test.go index e7c4d2f9f7..7a74c5f5fc 100644 --- a/models/organization/org_test.go +++ b/models/organization/org_test.go @@ -13,7 +13,9 @@ import ( repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -382,6 +384,12 @@ func TestHasOrgVisibleTypePublic(t *testing.T) { assert.True(t, test1) // owner of org assert.True(t, test2) // user not a part of org assert.True(t, test3) // logged out user + + restrictedUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 29, IsRestricted: true}) + require.True(t, restrictedUser.IsRestricted) + assert.True(t, organization.HasOrgOrUserVisible(t.Context(), org.AsUser(), restrictedUser)) + defer test.MockVariableValue(&setting.Service.RequireSignInViewStrict, true)() + assert.False(t, organization.HasOrgOrUserVisible(t.Context(), org.AsUser(), restrictedUser)) } func TestHasOrgVisibleTypeLimited(t *testing.T) { diff --git a/models/perm/access/access.go b/models/perm/access/access.go index 6a0a901f71..6433c4675c 100644 --- a/models/perm/access/access.go +++ b/models/perm/access/access.go @@ -13,6 +13,8 @@ import ( "code.gitea.io/gitea/models/perm" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/structs" "xorm.io/builder" ) @@ -41,7 +43,12 @@ func accessLevel(ctx context.Context, user *user_model.User, repo *repo_model.Re restricted = user.IsRestricted } - if !restricted && !repo.IsPrivate { + if err := repo.LoadOwner(ctx); err != nil { + return mode, err + } + + repoIsFullyPublic := !setting.Service.RequireSignInViewStrict && repo.Owner.Visibility == structs.VisibleTypePublic && !repo.IsPrivate + if (restricted && repoIsFullyPublic) || (!restricted && !repo.IsPrivate) { mode = perm.AccessModeRead } diff --git a/models/perm/access/access_test.go b/models/perm/access/access_test.go index f01993ab4e..15d18b368c 100644 --- a/models/perm/access/access_test.go +++ b/models/perm/access/access_test.go @@ -12,6 +12,7 @@ import ( repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/setting" "github.com/stretchr/testify/assert" ) @@ -51,7 +52,14 @@ func TestAccessLevel(t *testing.T) { assert.NoError(t, err) assert.Equal(t, perm_model.AccessModeNone, level) - // restricted user has no access to a public repo + // restricted user has default access to a public repo if no sign-in is required + setting.Service.RequireSignInViewStrict = false + level, err = access_model.AccessLevel(t.Context(), user29, repo1) + assert.NoError(t, err) + assert.Equal(t, perm_model.AccessModeRead, level) + + // restricted user has no access to a public repo if sign-in is required + setting.Service.RequireSignInViewStrict = true level, err = access_model.AccessLevel(t.Context(), user29, repo1) assert.NoError(t, err) assert.Equal(t, perm_model.AccessModeNone, level) diff --git a/models/repo/repo_list.go b/models/repo/repo_list.go index f2cdd2f284..811f83c999 100644 --- a/models/repo/repo_list.go +++ b/models/repo/repo_list.go @@ -642,6 +642,17 @@ func SearchRepositoryIDsByCondition(ctx context.Context, cond builder.Cond) ([]i Find(&repoIDs) } +func userAllPublicRepoCond(cond builder.Cond, orgVisibilityLimit []structs.VisibleType) builder.Cond { + return cond.Or(builder.And( + builder.Eq{"`repository`.is_private": false}, + // Aren't in a private organisation or limited organisation if we're not logged in + builder.NotIn("`repository`.owner_id", builder.Select("id").From("`user`").Where( + builder.And( + builder.Eq{"type": user_model.UserTypeOrganization}, + builder.In("visibility", orgVisibilityLimit)), + )))) +} + // AccessibleRepositoryCondition takes a user a returns a condition for checking if a repository is accessible func AccessibleRepositoryCondition(user *user_model.User, unitType unit.Type) builder.Cond { cond := builder.NewCond() @@ -651,15 +662,8 @@ func AccessibleRepositoryCondition(user *user_model.User, unitType unit.Type) bu if user == nil || user.ID <= 0 { orgVisibilityLimit = append(orgVisibilityLimit, structs.VisibleTypeLimited) } - // 1. Be able to see all non-private repositories that either: - cond = cond.Or(builder.And( - builder.Eq{"`repository`.is_private": false}, - // 2. Aren't in an private organisation or limited organisation if we're not logged in - builder.NotIn("`repository`.owner_id", builder.Select("id").From("`user`").Where( - builder.And( - builder.Eq{"type": user_model.UserTypeOrganization}, - builder.In("visibility", orgVisibilityLimit)), - )))) + // 1. Be able to see all non-private repositories + cond = userAllPublicRepoCond(cond, orgVisibilityLimit) } if user != nil { @@ -683,6 +687,9 @@ func AccessibleRepositoryCondition(user *user_model.User, unitType unit.Type) bu if !user.IsRestricted { // 5. Be able to see all public repos in private organizations that we are an org_user of cond = cond.Or(userOrgPublicRepoCond(user.ID)) + } else if !setting.Service.RequireSignInViewStrict { + orgVisibilityLimit := []structs.VisibleType{structs.VisibleTypePrivate, structs.VisibleTypeLimited} + cond = userAllPublicRepoCond(cond, orgVisibilityLimit) } } diff --git a/models/repo/repo_list_test.go b/models/repo/repo_list_test.go index 6cc0d3155c..943e0c5025 100644 --- a/models/repo/repo_list_test.go +++ b/models/repo/repo_list_test.go @@ -10,9 +10,14 @@ import ( "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/optional" + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/test" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func getTestCases() []struct { @@ -182,7 +187,16 @@ func getTestCases() []struct { func TestSearchRepository(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) + t.Run("SearchRepositoryPublic", testSearchRepositoryPublic) + t.Run("SearchRepositoryPublicRestricted", testSearchRepositoryRestricted) + t.Run("SearchRepositoryPrivate", testSearchRepositoryPrivate) + t.Run("SearchRepositoryNonExistingOwner", testSearchRepositoryNonExistingOwner) + t.Run("SearchRepositoryWithInDescription", testSearchRepositoryWithInDescription) + t.Run("SearchRepositoryNotInDescription", testSearchRepositoryNotInDescription) + t.Run("SearchRepositoryCases", testSearchRepositoryCases) +} +func testSearchRepositoryPublic(t *testing.T) { // test search public repository on explore page repos, count, err := repo_model.SearchRepositoryByName(t.Context(), repo_model.SearchRepoOptions{ ListOptions: db.ListOptions{ @@ -211,9 +225,54 @@ func TestSearchRepository(t *testing.T) { assert.NoError(t, err) assert.Equal(t, int64(2), count) assert.Len(t, repos, 2) +} +func testSearchRepositoryRestricted(t *testing.T) { + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + restrictedUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 29, IsRestricted: true}) + + performSearch := func(t *testing.T, user *user_model.User) (publicRepoIDs []int64) { + repos, count, err := repo_model.SearchRepositoryByName(t.Context(), repo_model.SearchRepoOptions{ + ListOptions: db.ListOptions{Page: 1, PageSize: 10000}, + Actor: user, + }) + require.NoError(t, err) + assert.Len(t, repos, int(count)) + for _, repo := range repos { + require.NoError(t, repo.LoadOwner(t.Context())) + if repo.Owner.Visibility == structs.VisibleTypePublic && !repo.IsPrivate { + publicRepoIDs = append(publicRepoIDs, repo.ID) + } + } + return publicRepoIDs + } + + normalPublicRepoIDs := performSearch(t, user2) + require.Greater(t, len(normalPublicRepoIDs), 10) // quite a lot + + t.Run("RestrictedUser-NoSignInRequirement", func(t *testing.T) { + // restricted user can also see public repositories if no "required sign-in" + repoIDs := performSearch(t, restrictedUser) + assert.ElementsMatch(t, normalPublicRepoIDs, repoIDs) + }) + + defer test.MockVariableValue(&setting.Service.RequireSignInViewStrict, true)() + + t.Run("NormalUser-RequiredSignIn", func(t *testing.T) { + // normal user can still see all public repos, not affected by "required sign-in" + repoIDs := performSearch(t, user2) + assert.ElementsMatch(t, normalPublicRepoIDs, repoIDs) + }) + t.Run("RestrictedUser-RequiredSignIn", func(t *testing.T) { + // restricted user can see only their own repo + repoIDs := performSearch(t, restrictedUser) + assert.Equal(t, []int64{4}, repoIDs) + }) +} + +func testSearchRepositoryPrivate(t *testing.T) { // test search private repository on explore page - repos, count, err = repo_model.SearchRepositoryByName(t.Context(), repo_model.SearchRepoOptions{ + repos, count, err := repo_model.SearchRepositoryByName(t.Context(), repo_model.SearchRepoOptions{ ListOptions: db.ListOptions{ Page: 1, PageSize: 10, @@ -242,16 +301,18 @@ func TestSearchRepository(t *testing.T) { assert.NoError(t, err) assert.Equal(t, int64(3), count) assert.Len(t, repos, 3) +} - // Test non existing owner - repos, count, err = repo_model.SearchRepositoryByName(t.Context(), repo_model.SearchRepoOptions{OwnerID: unittest.NonexistentID}) +func testSearchRepositoryNonExistingOwner(t *testing.T) { + repos, count, err := repo_model.SearchRepositoryByName(t.Context(), repo_model.SearchRepoOptions{OwnerID: unittest.NonexistentID}) assert.NoError(t, err) assert.Empty(t, repos) assert.Equal(t, int64(0), count) +} - // Test search within description - repos, count, err = repo_model.SearchRepository(t.Context(), repo_model.SearchRepoOptions{ +func testSearchRepositoryWithInDescription(t *testing.T) { + repos, count, err := repo_model.SearchRepository(t.Context(), repo_model.SearchRepoOptions{ ListOptions: db.ListOptions{ Page: 1, PageSize: 10, @@ -266,9 +327,10 @@ func TestSearchRepository(t *testing.T) { assert.Equal(t, "test_repo_14", repos[0].Name) } assert.Equal(t, int64(1), count) +} - // Test NOT search within description - repos, count, err = repo_model.SearchRepository(t.Context(), repo_model.SearchRepoOptions{ +func testSearchRepositoryNotInDescription(t *testing.T) { + repos, count, err := repo_model.SearchRepository(t.Context(), repo_model.SearchRepoOptions{ ListOptions: db.ListOptions{ Page: 1, PageSize: 10, @@ -281,7 +343,9 @@ func TestSearchRepository(t *testing.T) { assert.NoError(t, err) assert.Empty(t, repos) assert.Equal(t, int64(0), count) +} +func testSearchRepositoryCases(t *testing.T) { testCases := getTestCases() for _, testCase := range testCases { From a4e23b81d34837b4c0edb73862639e1c250573b9 Mon Sep 17 00:00:00 2001 From: a1012112796 <1012112796@qq.com> Date: Tue, 21 Oct 2025 23:07:11 +0800 Subject: [PATCH 08/29] fix attachment file size limit in server backend (#35519) fix #35512 --------- Co-authored-by: wxiaoguang --- modules/git/utils.go | 27 ------ modules/setting/attachment.go | 10 ++- modules/util/error.go | 1 + routers/api/v1/repo/issue_attachment.go | 9 +- .../api/v1/repo/issue_comment_attachment.go | 8 +- routers/api/v1/repo/release_attachment.go | 22 +++-- routers/web/repo/attachment.go | 3 +- routers/web/repo/editor_uploader.go | 2 + services/attachment/attachment.go | 44 ++++++++-- services/context/api.go | 3 +- services/context/base.go | 15 ++++ services/context/context.go | 3 +- services/mailer/incoming/incoming_handler.go | 10 ++- templates/swagger/v1_json.tmpl | 9 ++ .../api_comment_attachment_test.go | 7 +- .../integration/api_issue_attachment_test.go | 7 +- tests/integration/api_releases_test.go | 82 ++++++++++--------- tests/integration/attachment_test.go | 16 ++-- 18 files changed, 169 insertions(+), 109 deletions(-) diff --git a/modules/git/utils.go b/modules/git/utils.go index b5f188904a..e7d30ce9ee 100644 --- a/modules/git/utils.go +++ b/modules/git/utils.go @@ -6,7 +6,6 @@ package git import ( "crypto/sha1" "encoding/hex" - "io" "strconv" "strings" "sync" @@ -68,32 +67,6 @@ func ParseBool(value string) (result, valid bool) { return intValue != 0, true } -// LimitedReaderCloser is a limited reader closer -type LimitedReaderCloser struct { - R io.Reader - C io.Closer - N int64 -} - -// Read implements io.Reader -func (l *LimitedReaderCloser) Read(p []byte) (n int, err error) { - if l.N <= 0 { - _ = l.C.Close() - return 0, io.EOF - } - if int64(len(p)) > l.N { - p = p[0:l.N] - } - n, err = l.R.Read(p) - l.N -= int64(n) - return n, err -} - -// Close implements io.Closer -func (l *LimitedReaderCloser) Close() error { - return l.C.Close() -} - func HashFilePathForWebUI(s string) string { h := sha1.New() _, _ = h.Write([]byte(s)) diff --git a/modules/setting/attachment.go b/modules/setting/attachment.go index c11b0c478a..5d420c987c 100644 --- a/modules/setting/attachment.go +++ b/modules/setting/attachment.go @@ -16,9 +16,13 @@ var Attachment AttachmentSettingType func loadAttachmentFrom(rootCfg ConfigProvider) (err error) { Attachment = AttachmentSettingType{ AllowedTypes: ".avif,.cpuprofile,.csv,.dmp,.docx,.fodg,.fodp,.fods,.fodt,.gif,.gz,.jpeg,.jpg,.json,.jsonc,.log,.md,.mov,.mp4,.odf,.odg,.odp,.ods,.odt,.patch,.pdf,.png,.pptx,.svg,.tgz,.txt,.webm,.webp,.xls,.xlsx,.zip", - MaxSize: 2048, - MaxFiles: 5, - Enabled: true, + + // FIXME: this size is used for both "issue attachment" and "release attachment" + // The design is not right, these two should be different settings + MaxSize: 2048, + + MaxFiles: 5, + Enabled: true, } sec, _ := rootCfg.GetSection("attachment") if sec == nil { diff --git a/modules/util/error.go b/modules/util/error.go index 6b2721618e..24fa1ba151 100644 --- a/modules/util/error.go +++ b/modules/util/error.go @@ -16,6 +16,7 @@ var ( ErrPermissionDenied = errors.New("permission denied") // also implies HTTP 403 ErrNotExist = errors.New("resource does not exist") // also implies HTTP 404 ErrAlreadyExist = errors.New("resource already exists") // also implies HTTP 409 + ErrContentTooLarge = errors.New("content exceeds limit") // also implies HTTP 413 // ErrUnprocessableContent implies HTTP 422, the syntax of the request content is correct, // but the server is unable to process the contained instructions diff --git a/routers/api/v1/repo/issue_attachment.go b/routers/api/v1/repo/issue_attachment.go index 3f751a295c..bfe9c92f1c 100644 --- a/routers/api/v1/repo/issue_attachment.go +++ b/routers/api/v1/repo/issue_attachment.go @@ -4,6 +4,7 @@ package repo import ( + "errors" "net/http" issues_model "code.gitea.io/gitea/models/issues" @@ -11,6 +12,7 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web" attachment_service "code.gitea.io/gitea/services/attachment" "code.gitea.io/gitea/services/context" @@ -154,6 +156,8 @@ func CreateIssueAttachment(ctx *context.APIContext) { // "$ref": "#/responses/error" // "404": // "$ref": "#/responses/error" + // "413": + // "$ref": "#/responses/error" // "422": // "$ref": "#/responses/validationError" // "423": @@ -181,7 +185,8 @@ func CreateIssueAttachment(ctx *context.APIContext) { filename = query } - attachment, err := attachment_service.UploadAttachment(ctx, file, setting.Attachment.AllowedTypes, header.Size, &repo_model.Attachment{ + uploaderFile := attachment_service.NewLimitedUploaderKnownSize(file, header.Size) + attachment, err := attachment_service.UploadAttachmentGeneralSizeLimit(ctx, uploaderFile, setting.Attachment.AllowedTypes, &repo_model.Attachment{ Name: filename, UploaderID: ctx.Doer.ID, RepoID: ctx.Repo.Repository.ID, @@ -190,6 +195,8 @@ func CreateIssueAttachment(ctx *context.APIContext) { if err != nil { if upload.IsErrFileTypeForbidden(err) { ctx.APIError(http.StatusUnprocessableEntity, err) + } else if errors.Is(err, util.ErrContentTooLarge) { + ctx.APIError(http.StatusRequestEntityTooLarge, err) } else { ctx.APIErrorInternal(err) } diff --git a/routers/api/v1/repo/issue_comment_attachment.go b/routers/api/v1/repo/issue_comment_attachment.go index 5f660c5750..3227f5ddee 100644 --- a/routers/api/v1/repo/issue_comment_attachment.go +++ b/routers/api/v1/repo/issue_comment_attachment.go @@ -13,6 +13,7 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web" attachment_service "code.gitea.io/gitea/services/attachment" "code.gitea.io/gitea/services/context" @@ -161,6 +162,8 @@ func CreateIssueCommentAttachment(ctx *context.APIContext) { // "$ref": "#/responses/forbidden" // "404": // "$ref": "#/responses/error" + // "413": + // "$ref": "#/responses/error" // "422": // "$ref": "#/responses/validationError" // "423": @@ -189,7 +192,8 @@ func CreateIssueCommentAttachment(ctx *context.APIContext) { filename = query } - attachment, err := attachment_service.UploadAttachment(ctx, file, setting.Attachment.AllowedTypes, header.Size, &repo_model.Attachment{ + uploaderFile := attachment_service.NewLimitedUploaderKnownSize(file, header.Size) + attachment, err := attachment_service.UploadAttachmentGeneralSizeLimit(ctx, uploaderFile, setting.Attachment.AllowedTypes, &repo_model.Attachment{ Name: filename, UploaderID: ctx.Doer.ID, RepoID: ctx.Repo.Repository.ID, @@ -199,6 +203,8 @@ func CreateIssueCommentAttachment(ctx *context.APIContext) { if err != nil { if upload.IsErrFileTypeForbidden(err) { ctx.APIError(http.StatusUnprocessableEntity, err) + } else if errors.Is(err, util.ErrContentTooLarge) { + ctx.APIError(http.StatusRequestEntityTooLarge, err) } else { ctx.APIErrorInternal(err) } diff --git a/routers/api/v1/repo/release_attachment.go b/routers/api/v1/repo/release_attachment.go index defde81a1d..43e97beb27 100644 --- a/routers/api/v1/repo/release_attachment.go +++ b/routers/api/v1/repo/release_attachment.go @@ -4,7 +4,7 @@ package repo import ( - "io" + "errors" "net/http" "strings" @@ -12,6 +12,7 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web" attachment_service "code.gitea.io/gitea/services/attachment" "code.gitea.io/gitea/services/context" @@ -191,6 +192,8 @@ func CreateReleaseAttachment(ctx *context.APIContext) { // "$ref": "#/responses/error" // "404": // "$ref": "#/responses/notFound" + // "413": + // "$ref": "#/responses/error" // Check if attachments are enabled if !setting.Attachment.Enabled { @@ -205,10 +208,8 @@ func CreateReleaseAttachment(ctx *context.APIContext) { } // Get uploaded file from request - var content io.ReadCloser var filename string - var size int64 = -1 - + var uploaderFile *attachment_service.UploaderFile if strings.HasPrefix(strings.ToLower(ctx.Req.Header.Get("Content-Type")), "multipart/form-data") { file, header, err := ctx.Req.FormFile("attachment") if err != nil { @@ -217,15 +218,14 @@ func CreateReleaseAttachment(ctx *context.APIContext) { } defer file.Close() - content = file - size = header.Size filename = header.Filename if name := ctx.FormString("name"); name != "" { filename = name } + uploaderFile = attachment_service.NewLimitedUploaderKnownSize(file, header.Size) } else { - content = ctx.Req.Body filename = ctx.FormString("name") + uploaderFile = attachment_service.NewLimitedUploaderMaxBytesReader(ctx.Req.Body, ctx.Resp) } if filename == "" { @@ -234,7 +234,7 @@ func CreateReleaseAttachment(ctx *context.APIContext) { } // Create a new attachment and save the file - attach, err := attachment_service.UploadAttachment(ctx, content, setting.Repository.Release.AllowedTypes, size, &repo_model.Attachment{ + attach, err := attachment_service.UploadAttachmentGeneralSizeLimit(ctx, uploaderFile, setting.Repository.Release.AllowedTypes, &repo_model.Attachment{ Name: filename, UploaderID: ctx.Doer.ID, RepoID: ctx.Repo.Repository.ID, @@ -245,6 +245,12 @@ func CreateReleaseAttachment(ctx *context.APIContext) { ctx.APIError(http.StatusBadRequest, err) return } + + if errors.Is(err, util.ErrContentTooLarge) { + ctx.APIError(http.StatusRequestEntityTooLarge, err) + return + } + ctx.APIErrorInternal(err) return } diff --git a/routers/web/repo/attachment.go b/routers/web/repo/attachment.go index f696669196..54200d8de8 100644 --- a/routers/web/repo/attachment.go +++ b/routers/web/repo/attachment.go @@ -45,7 +45,8 @@ func uploadAttachment(ctx *context.Context, repoID int64, allowedTypes string) { } defer file.Close() - attach, err := attachment.UploadAttachment(ctx, file, allowedTypes, header.Size, &repo_model.Attachment{ + uploaderFile := attachment.NewLimitedUploaderKnownSize(file, header.Size) + attach, err := attachment.UploadAttachmentGeneralSizeLimit(ctx, uploaderFile, allowedTypes, &repo_model.Attachment{ Name: header.Filename, UploaderID: ctx.Doer.ID, RepoID: repoID, diff --git a/routers/web/repo/editor_uploader.go b/routers/web/repo/editor_uploader.go index 1ce9a1aca4..5ad07cda74 100644 --- a/routers/web/repo/editor_uploader.go +++ b/routers/web/repo/editor_uploader.go @@ -41,6 +41,8 @@ func UploadFileToServer(ctx *context.Context) { return } + // FIXME: need to check the file size according to setting.Repository.Upload.FileMaxSize + uploaded, err := repo_model.NewUpload(ctx, name, buf, file) if err != nil { ctx.ServerError("NewUpload", err) diff --git a/services/attachment/attachment.go b/services/attachment/attachment.go index ccb97c66c8..eb208a141c 100644 --- a/services/attachment/attachment.go +++ b/services/attachment/attachment.go @@ -6,11 +6,14 @@ package attachment import ( "bytes" "context" + "errors" "fmt" "io" + "net/http" "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/storage" "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/services/context/upload" @@ -28,27 +31,56 @@ func NewAttachment(ctx context.Context, attach *repo_model.Attachment, file io.R attach.UUID = uuid.New().String() size, err := storage.Attachments.Save(attach.RelativePath(), file, size) if err != nil { - return fmt.Errorf("Create: %w", err) + return fmt.Errorf("Attachments.Save: %w", err) } attach.Size = size - return db.Insert(ctx, attach) }) return attach, err } -// UploadAttachment upload new attachment into storage and update database -func UploadAttachment(ctx context.Context, file io.Reader, allowedTypes string, fileSize int64, attach *repo_model.Attachment) (*repo_model.Attachment, error) { +type UploaderFile struct { + rd io.ReadCloser + size int64 + respWriter http.ResponseWriter +} + +func NewLimitedUploaderKnownSize(r io.Reader, size int64) *UploaderFile { + return &UploaderFile{rd: io.NopCloser(r), size: size} +} + +func NewLimitedUploaderMaxBytesReader(r io.ReadCloser, w http.ResponseWriter) *UploaderFile { + return &UploaderFile{rd: r, size: -1, respWriter: w} +} + +func UploadAttachmentGeneralSizeLimit(ctx context.Context, file *UploaderFile, allowedTypes string, attach *repo_model.Attachment) (*repo_model.Attachment, error) { + return uploadAttachment(ctx, file, allowedTypes, setting.Attachment.MaxSize<<20, attach) +} + +func uploadAttachment(ctx context.Context, file *UploaderFile, allowedTypes string, maxFileSize int64, attach *repo_model.Attachment) (*repo_model.Attachment, error) { + src := file.rd + if file.size < 0 { + src = http.MaxBytesReader(file.respWriter, src, maxFileSize) + } buf := make([]byte, 1024) - n, _ := util.ReadAtMost(file, buf) + n, _ := util.ReadAtMost(src, buf) buf = buf[:n] if err := upload.Verify(buf, attach.Name, allowedTypes); err != nil { return nil, err } - return NewAttachment(ctx, attach, io.MultiReader(bytes.NewReader(buf), file), fileSize) + if maxFileSize >= 0 && file.size > maxFileSize { + return nil, util.ErrorWrap(util.ErrContentTooLarge, "attachment exceeds limit %d", maxFileSize) + } + + attach, err := NewAttachment(ctx, attach, io.MultiReader(bytes.NewReader(buf), src), file.size) + var maxBytesError *http.MaxBytesError + if errors.As(err, &maxBytesError) { + return nil, util.ErrorWrap(util.ErrContentTooLarge, "attachment exceeds limit %d", maxFileSize) + } + return attach, err } // UpdateAttachment updates an attachment, verifying that its name is among the allowed types. diff --git a/services/context/api.go b/services/context/api.go index ab50a360f4..d698b91163 100644 --- a/services/context/api.go +++ b/services/context/api.go @@ -229,8 +229,7 @@ func APIContexter() func(http.Handler) http.Handler { // If request sends files, parse them here otherwise the Query() can't be parsed and the CsrfToken will be invalid. if ctx.Req.Method == http.MethodPost && strings.Contains(ctx.Req.Header.Get("Content-Type"), "multipart/form-data") { - if err := ctx.Req.ParseMultipartForm(setting.Attachment.MaxSize << 20); err != nil && !strings.Contains(err.Error(), "EOF") { // 32MB max size - ctx.APIErrorInternal(err) + if !ctx.ParseMultipartForm() { return } } diff --git a/services/context/base.go b/services/context/base.go index 28d6656fd1..de839ede81 100644 --- a/services/context/base.go +++ b/services/context/base.go @@ -4,6 +4,7 @@ package context import ( + "errors" "fmt" "html/template" "io" @@ -42,6 +43,20 @@ type Base struct { Locale translation.Locale } +func (b *Base) ParseMultipartForm() bool { + err := b.Req.ParseMultipartForm(32 << 20) + if err != nil { + // TODO: all errors caused by client side should be ignored (connection closed). + if !errors.Is(err, io.EOF) && !errors.Is(err, io.ErrUnexpectedEOF) { + // Errors caused by server side (disk full) should be logged. + log.Error("Failed to parse request multipart form for %s: %v", b.Req.RequestURI, err) + } + b.HTTPError(http.StatusInternalServerError, "failed to parse request multipart form") + return false + } + return true +} + // AppendAccessControlExposeHeaders append headers by name to "Access-Control-Expose-Headers" header func (b *Base) AppendAccessControlExposeHeaders(names ...string) { val := b.RespHeader().Get("Access-Control-Expose-Headers") diff --git a/services/context/context.go b/services/context/context.go index 32ec260aab..4e83dee807 100644 --- a/services/context/context.go +++ b/services/context/context.go @@ -186,8 +186,7 @@ func Contexter() func(next http.Handler) http.Handler { // If request sends files, parse them here otherwise the Query() can't be parsed and the CsrfToken will be invalid. if ctx.Req.Method == http.MethodPost && strings.Contains(ctx.Req.Header.Get("Content-Type"), "multipart/form-data") { - if err := ctx.Req.ParseMultipartForm(setting.Attachment.MaxSize << 20); err != nil && !strings.Contains(err.Error(), "EOF") { // 32MB max size - ctx.ServerError("ParseMultipartForm", err) + if !ctx.ParseMultipartForm() { return } } diff --git a/services/mailer/incoming/incoming_handler.go b/services/mailer/incoming/incoming_handler.go index 38a234eac1..440b3a6b59 100644 --- a/services/mailer/incoming/incoming_handler.go +++ b/services/mailer/incoming/incoming_handler.go @@ -6,6 +6,7 @@ package incoming import ( "bytes" "context" + "errors" "fmt" issues_model "code.gitea.io/gitea/models/issues" @@ -85,7 +86,9 @@ func (h *ReplyHandler) Handle(ctx context.Context, content *MailContent, doer *u attachmentIDs := make([]string, 0, len(content.Attachments)) if setting.Attachment.Enabled { for _, attachment := range content.Attachments { - a, err := attachment_service.UploadAttachment(ctx, bytes.NewReader(attachment.Content), setting.Attachment.AllowedTypes, int64(len(attachment.Content)), &repo_model.Attachment{ + attachmentBuf := bytes.NewReader(attachment.Content) + uploaderFile := attachment_service.NewLimitedUploaderKnownSize(attachmentBuf, attachmentBuf.Size()) + a, err := attachment_service.UploadAttachmentGeneralSizeLimit(ctx, uploaderFile, setting.Attachment.AllowedTypes, &repo_model.Attachment{ Name: attachment.Name, UploaderID: doer.ID, RepoID: issue.Repo.ID, @@ -95,6 +98,11 @@ func (h *ReplyHandler) Handle(ctx context.Context, content *MailContent, doer *u log.Info("Skipping disallowed attachment type: %s", attachment.Name) continue } + if errors.Is(err, util.ErrContentTooLarge) { + log.Info("Skipping attachment exceeding size limit: %s", attachment.Name) + continue + } + return err } attachmentIDs = append(attachmentIDs, a.UUID) diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 325f0b78a0..966aff12f8 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -9569,6 +9569,9 @@ "404": { "$ref": "#/responses/error" }, + "413": { + "$ref": "#/responses/error" + }, "422": { "$ref": "#/responses/validationError" }, @@ -10194,6 +10197,9 @@ "404": { "$ref": "#/responses/error" }, + "413": { + "$ref": "#/responses/error" + }, "422": { "$ref": "#/responses/validationError" }, @@ -15510,6 +15516,9 @@ }, "404": { "$ref": "#/responses/notFound" + }, + "413": { + "$ref": "#/responses/error" } } } diff --git a/tests/integration/api_comment_attachment_test.go b/tests/integration/api_comment_attachment_test.go index cddae27d94..ae90331962 100644 --- a/tests/integration/api_comment_attachment_test.go +++ b/tests/integration/api_comment_attachment_test.go @@ -6,7 +6,6 @@ package integration import ( "bytes" "fmt" - "io" "mime/multipart" "net/http" "testing" @@ -95,15 +94,13 @@ func TestAPICreateCommentAttachment(t *testing.T) { session := loginUser(t, repoOwner.Name) token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteIssue) - filename := "image.png" - buff := generateImg() body := &bytes.Buffer{} // Setup multi-part writer := multipart.NewWriter(body) - part, err := writer.CreateFormFile("attachment", filename) + part, err := writer.CreateFormFile("attachment", "image.png") assert.NoError(t, err) - _, err = io.Copy(part, &buff) + _, err = part.Write(testGeneratePngBytes()) assert.NoError(t, err) err = writer.Close() assert.NoError(t, err) diff --git a/tests/integration/api_issue_attachment_test.go b/tests/integration/api_issue_attachment_test.go index 6806d27df2..12acaeae5d 100644 --- a/tests/integration/api_issue_attachment_test.go +++ b/tests/integration/api_issue_attachment_test.go @@ -6,7 +6,6 @@ package integration import ( "bytes" "fmt" - "io" "mime/multipart" "net/http" "testing" @@ -72,15 +71,13 @@ func TestAPICreateIssueAttachment(t *testing.T) { session := loginUser(t, repoOwner.Name) token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteIssue) - filename := "image.png" - buff := generateImg() body := &bytes.Buffer{} // Setup multi-part writer := multipart.NewWriter(body) - part, err := writer.CreateFormFile("attachment", filename) + part, err := writer.CreateFormFile("attachment", "image.png") assert.NoError(t, err) - _, err = io.Copy(part, &buff) + _, err = part.Write(testGeneratePngBytes()) assert.NoError(t, err) err = writer.Close() assert.NoError(t, err) diff --git a/tests/integration/api_releases_test.go b/tests/integration/api_releases_test.go index 1a04bec64e..53d37df8e0 100644 --- a/tests/integration/api_releases_test.go +++ b/tests/integration/api_releases_test.go @@ -9,6 +9,7 @@ import ( "io" "mime/multipart" "net/http" + "net/http/httptest" "net/url" "strings" "testing" @@ -18,7 +19,9 @@ import ( "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/gitrepo" + "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" @@ -294,67 +297,70 @@ func TestAPIDeleteReleaseByTagName(t *testing.T) { func TestAPIUploadAssetRelease(t *testing.T) { defer tests.PrepareTestEnv(t)() + defer test.MockVariableValue(&setting.Attachment.MaxSize, 1)() repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) session := loginUser(t, owner.LowerName) token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) - r := createNewReleaseUsingAPI(t, token, owner, repo, "release-tag", "", "Release Tag", "test") + bufImageBytes := testGeneratePngBytes() + bufLargeBytes := bytes.Repeat([]byte{' '}, 2*1024*1024) - filename := "image.png" - buff := generateImg() - - assetURL := fmt.Sprintf("/api/v1/repos/%s/%s/releases/%d/assets", owner.Name, repo.Name, r.ID) + release := createNewReleaseUsingAPI(t, token, owner, repo, "release-tag", "", "Release Tag", "test") + assetURL := fmt.Sprintf("/api/v1/repos/%s/%s/releases/%d/assets", owner.Name, repo.Name, release.ID) t.Run("multipart/form-data", func(t *testing.T) { defer tests.PrintCurrentTest(t)() + const filename = "image.png" - body := &bytes.Buffer{} + performUpload := func(t *testing.T, uploadURL string, buf []byte, expectedStatus int) *httptest.ResponseRecorder { + body := &bytes.Buffer{} + writer := multipart.NewWriter(body) + part, err := writer.CreateFormFile("attachment", filename) + assert.NoError(t, err) + _, err = io.Copy(part, bytes.NewReader(bufImageBytes)) + assert.NoError(t, err) + err = writer.Close() + assert.NoError(t, err) - writer := multipart.NewWriter(body) - part, err := writer.CreateFormFile("attachment", filename) - assert.NoError(t, err) - _, err = io.Copy(part, bytes.NewReader(buff.Bytes())) - assert.NoError(t, err) - err = writer.Close() - assert.NoError(t, err) + req := NewRequestWithBody(t, http.MethodPost, uploadURL, bytes.NewReader(body.Bytes())). + AddTokenAuth(token). + SetHeader("Content-Type", writer.FormDataContentType()) + return MakeRequest(t, req, http.StatusCreated) + } - req := NewRequestWithBody(t, http.MethodPost, assetURL, bytes.NewReader(body.Bytes())). - AddTokenAuth(token). - SetHeader("Content-Type", writer.FormDataContentType()) - resp := MakeRequest(t, req, http.StatusCreated) + performUpload(t, assetURL, bufLargeBytes, http.StatusRequestEntityTooLarge) - var attachment *api.Attachment - DecodeJSON(t, resp, &attachment) - - assert.Equal(t, filename, attachment.Name) - assert.EqualValues(t, 104, attachment.Size) - - req = NewRequestWithBody(t, http.MethodPost, assetURL+"?name=test-asset", bytes.NewReader(body.Bytes())). - AddTokenAuth(token). - SetHeader("Content-Type", writer.FormDataContentType()) - resp = MakeRequest(t, req, http.StatusCreated) - - var attachment2 *api.Attachment - DecodeJSON(t, resp, &attachment2) - - assert.Equal(t, "test-asset", attachment2.Name) - assert.EqualValues(t, 104, attachment2.Size) + t.Run("UploadDefaultName", func(t *testing.T) { + resp := performUpload(t, assetURL, bufImageBytes, http.StatusCreated) + var attachment api.Attachment + DecodeJSON(t, resp, &attachment) + assert.Equal(t, filename, attachment.Name) + assert.EqualValues(t, 104, attachment.Size) + }) + t.Run("UploadWithName", func(t *testing.T) { + resp := performUpload(t, assetURL+"?name=test-asset", bufImageBytes, http.StatusCreated) + var attachment api.Attachment + DecodeJSON(t, resp, &attachment) + assert.Equal(t, "test-asset", attachment.Name) + assert.EqualValues(t, 104, attachment.Size) + }) }) t.Run("application/octet-stream", func(t *testing.T) { defer tests.PrintCurrentTest(t)() - req := NewRequestWithBody(t, http.MethodPost, assetURL, bytes.NewReader(buff.Bytes())). - AddTokenAuth(token) + req := NewRequestWithBody(t, http.MethodPost, assetURL, bytes.NewReader(bufImageBytes)).AddTokenAuth(token) MakeRequest(t, req, http.StatusBadRequest) - req = NewRequestWithBody(t, http.MethodPost, assetURL+"?name=stream.bin", bytes.NewReader(buff.Bytes())). - AddTokenAuth(token) + req = NewRequestWithBody(t, http.MethodPost, assetURL+"?name=stream.bin", bytes.NewReader(bufLargeBytes)).AddTokenAuth(token) + MakeRequest(t, req, http.StatusRequestEntityTooLarge) + + req = NewRequestWithBody(t, http.MethodPost, assetURL+"?name=stream.bin", bytes.NewReader(bufImageBytes)).AddTokenAuth(token) resp := MakeRequest(t, req, http.StatusCreated) - var attachment *api.Attachment + var attachment api.Attachment DecodeJSON(t, resp, &attachment) assert.Equal(t, "stream.bin", attachment.Name) diff --git a/tests/integration/attachment_test.go b/tests/integration/attachment_test.go index 30c394e9b0..44aaee09f8 100644 --- a/tests/integration/attachment_test.go +++ b/tests/integration/attachment_test.go @@ -7,7 +7,6 @@ import ( "bytes" "image" "image/png" - "io" "mime/multipart" "net/http" "strings" @@ -21,22 +20,21 @@ import ( "github.com/stretchr/testify/assert" ) -func generateImg() bytes.Buffer { - // Generate image +func testGeneratePngBytes() []byte { myImage := image.NewRGBA(image.Rect(0, 0, 32, 32)) var buff bytes.Buffer - png.Encode(&buff, myImage) - return buff + _ = png.Encode(&buff, myImage) + return buff.Bytes() } -func createAttachment(t *testing.T, session *TestSession, csrf, repoURL, filename string, buff bytes.Buffer, expectedStatus int) string { +func testCreateIssueAttachment(t *testing.T, session *TestSession, csrf, repoURL, filename string, content []byte, 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) + _, err = part.Write(content) assert.NoError(t, err) err = writer.Close() assert.NoError(t, err) @@ -57,14 +55,14 @@ func createAttachment(t *testing.T, session *TestSession, csrf, repoURL, filenam func TestCreateAnonymousAttachment(t *testing.T) { defer tests.PrepareTestEnv(t)() session := emptyTestSession(t) - createAttachment(t, session, GetAnonymousCSRFToken(t, session), "user2/repo1", "image.png", generateImg(), http.StatusSeeOther) + testCreateIssueAttachment(t, session, GetAnonymousCSRFToken(t, session), "user2/repo1", "image.png", testGeneratePngBytes(), http.StatusSeeOther) } func TestCreateIssueAttachment(t *testing.T) { defer tests.PrepareTestEnv(t)() const repoURL = "user2/repo1" session := loginUser(t, "user2") - uuid := createAttachment(t, session, GetUserCSRFToken(t, session), repoURL, "image.png", generateImg(), http.StatusOK) + uuid := testCreateIssueAttachment(t, session, GetUserCSRFToken(t, session), repoURL, "image.png", testGeneratePngBytes(), http.StatusOK) req := NewRequest(t, "GET", repoURL+"/issues/new") resp := session.MakeRequest(t, req, http.StatusOK) From c28aab671479fc7c67a95953fb21621fa9be373b Mon Sep 17 00:00:00 2001 From: GiteaBot Date: Wed, 22 Oct 2025 00:36:55 +0000 Subject: [PATCH 09/29] [skip ci] Updated translations via Crowdin --- options/locale/locale_ga-IE.ini | 4 ++++ options/locale/locale_pt-PT.ini | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/options/locale/locale_ga-IE.ini b/options/locale/locale_ga-IE.ini index 776340db42..8173b92acc 100644 --- a/options/locale/locale_ga-IE.ini +++ b/options/locale/locale_ga-IE.ini @@ -1969,6 +1969,9 @@ pulls.status_checks_requested=Riachtanach pulls.status_checks_details=Sonraí pulls.status_checks_hide_all=Folaigh gach seiceáil pulls.status_checks_show_all=Taispeáin gach seiceáil +pulls.status_checks_approve_all=Ceadaigh gach sreabhadh oibre +pulls.status_checks_need_approvals=%d sreabhadh oibre ag fanacht le ceadú +pulls.status_checks_need_approvals_helper=Ní rithfidh an sreabhadh oibre ach amháin tar éis ceadú a fháil ó chothaitheoir an stórais. pulls.update_branch=Nuashonrú brainse trí chumasc pulls.update_branch_rebase=Nuashonraigh an bhrainse trí athbhunú pulls.update_branch_success=Bhí nuashonrú brainse rathúil @@ -3890,6 +3893,7 @@ workflow.has_workflow_dispatch=Tá comhoibriú ag an gcur i bhfeidhm seo le hime workflow.has_no_workflow_dispatch=Níl aon truicear teagmhais workflow_dispatch ag sreabhadh oibre '%s'. need_approval_desc=Teastaíonn faomhadh chun sreafaí oibre a rith le haghaidh iarratas tarraingt forc. +approve_all_success=Tá gach rith sreabha oibre ceadaithe go rathúil. variables=Athróga variables.management=Bainistíocht Athróg diff --git a/options/locale/locale_pt-PT.ini b/options/locale/locale_pt-PT.ini index eefe32a515..4497a589d4 100644 --- a/options/locale/locale_pt-PT.ini +++ b/options/locale/locale_pt-PT.ini @@ -1969,6 +1969,9 @@ pulls.status_checks_requested=Obrigatório pulls.status_checks_details=Detalhes pulls.status_checks_hide_all=Ocultar todas as verificações pulls.status_checks_show_all=Mostrar todas as verificações +pulls.status_checks_approve_all=Aprovar todas as sequências de trabalho +pulls.status_checks_need_approvals=%d sequências de trabalho aguardando aprovação +pulls.status_checks_need_approvals_helper=A sequência de trabalho só irá ser executada depois de aprovada pelo responsável do repositório. pulls.update_branch=Modificar ramo executando a integração pulls.update_branch_rebase=Modificar ramo mudando a base pulls.update_branch_success=A sincronização do ramo foi bem sucedida @@ -3890,6 +3893,7 @@ workflow.has_workflow_dispatch=Esta sequência de trabalho tem um despoletador d workflow.has_no_workflow_dispatch=A sequência de trabalho '%s' não tem nenhum despoletador de eventos workflow_dispatch. need_approval_desc=É necessária aprovação para executar sequências de trabalho para a derivação do pedido de integração. +approve_all_success=Todas as execuções de sequências de trabalho foram aprovadas com sucesso. variables=Variáveis variables.management=Gestão de variáveis From 5f0697243c38405765c2f80f90a8512994620494 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 22 Oct 2025 12:35:56 +0800 Subject: [PATCH 10/29] Don't block site admin's operation if SECRET_KEY is lost (#35721) Related: #24573 --- models/admin/task.go | 7 ++++--- models/auth/twofactor.go | 4 ++-- models/secret/secret.go | 4 ++-- services/auth/source/ldap/source.go | 6 +++++- 4 files changed, 13 insertions(+), 8 deletions(-) diff --git a/models/admin/task.go b/models/admin/task.go index 0541a8ec78..5d2b9bbff6 100644 --- a/models/admin/task.go +++ b/models/admin/task.go @@ -11,6 +11,7 @@ import ( repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/json" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/migration" "code.gitea.io/gitea/modules/secret" "code.gitea.io/gitea/modules/setting" @@ -123,17 +124,17 @@ func (task *Task) MigrateConfig() (*migration.MigrateOptions, error) { // decrypt credentials if opts.CloneAddrEncrypted != "" { if opts.CloneAddr, err = secret.DecryptSecret(setting.SecretKey, opts.CloneAddrEncrypted); err != nil { - return nil, err + log.Error("Unable to decrypt CloneAddr, maybe SECRET_KEY is wrong: %v", err) } } if opts.AuthPasswordEncrypted != "" { if opts.AuthPassword, err = secret.DecryptSecret(setting.SecretKey, opts.AuthPasswordEncrypted); err != nil { - return nil, err + log.Error("Unable to decrypt AuthPassword, maybe SECRET_KEY is wrong: %v", err) } } if opts.AuthTokenEncrypted != "" { if opts.AuthToken, err = secret.DecryptSecret(setting.SecretKey, opts.AuthTokenEncrypted); err != nil { - return nil, err + log.Error("Unable to decrypt AuthToken, maybe SECRET_KEY is wrong: %v", err) } } diff --git a/models/auth/twofactor.go b/models/auth/twofactor.go index 200ce7c7c0..4263495650 100644 --- a/models/auth/twofactor.go +++ b/models/auth/twofactor.go @@ -111,11 +111,11 @@ func (t *TwoFactor) SetSecret(secretString string) error { func (t *TwoFactor) ValidateTOTP(passcode string) (bool, error) { decodedStoredSecret, err := base64.StdEncoding.DecodeString(t.Secret) if err != nil { - return false, err + return false, fmt.Errorf("ValidateTOTP invalid base64: %w", err) } secretBytes, err := secret.AesDecrypt(t.getEncryptionKey(), decodedStoredSecret) if err != nil { - return false, err + return false, fmt.Errorf("ValidateTOTP unable to decrypt (maybe SECRET_KEY is wrong): %w", err) } secretStr := string(secretBytes) return totp.Validate(passcode, secretStr), nil diff --git a/models/secret/secret.go b/models/secret/secret.go index 10a0287dfd..a82a924c39 100644 --- a/models/secret/secret.go +++ b/models/secret/secret.go @@ -178,8 +178,8 @@ func GetSecretsOfTask(ctx context.Context, task *actions_model.ActionTask) (map[ for _, secret := range append(ownerSecrets, repoSecrets...) { v, err := secret_module.DecryptSecret(setting.SecretKey, secret.Data) if err != nil { - log.Error("decrypt secret %v %q: %v", secret.ID, secret.Name, err) - return nil, err + log.Error("Unable to decrypt Actions secret %v %q, maybe SECRET_KEY is wrong: %v", secret.ID, secret.Name, err) + continue } secrets[secret.Name] = v } diff --git a/services/auth/source/ldap/source.go b/services/auth/source/ldap/source.go index d49dbc45ce..2362cad8aa 100644 --- a/services/auth/source/ldap/source.go +++ b/services/auth/source/ldap/source.go @@ -8,6 +8,7 @@ import ( "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/modules/json" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/secret" "code.gitea.io/gitea/modules/setting" ) @@ -66,9 +67,12 @@ func (source *Source) FromDB(bs []byte) error { } if source.BindPasswordEncrypt != "" { source.BindPassword, err = secret.DecryptSecret(setting.SecretKey, source.BindPasswordEncrypt) + if err != nil { + log.Error("Unable to decrypt bind password for LDAP source, maybe SECRET_KEY is wrong: %v", err) + } source.BindPasswordEncrypt = "" } - return err + return nil } // ToDB exports a LDAPConfig to a serialized format. From a9f2ea720be96435129c1d551806bf369260aa50 Mon Sep 17 00:00:00 2001 From: Kemal Zebari <60799661+kemzeb@users.noreply.github.com> Date: Tue, 21 Oct 2025 22:06:56 -0700 Subject: [PATCH 11/29] Honor delete branch on merge repo setting when using merge API (#35488) Fix #35463. --------- Co-authored-by: wxiaoguang --- models/git/protected_branch.go | 3 +- modules/util/error.go | 53 +++--- modules/util/error_test.go | 29 +++ routers/api/v1/repo/pull.go | 56 ++---- routers/web/repo/actions/view.go | 4 +- routers/web/repo/editor_apply_patch.go | 2 +- routers/web/repo/editor_cherry_pick.go | 2 +- routers/web/repo/editor_error.go | 4 +- routers/web/repo/pull.go | 172 +++--------------- services/actions/workflow.go | 10 +- services/automerge/automerge.go | 21 +-- services/forms/repo_form.go | 2 +- services/pull/merge.go | 21 +++ services/repository/branch.go | 91 ++++++++- .../issue/view_content/pull_merge_box.tmpl | 4 +- tests/integration/api_issue_config_test.go | 2 +- tests/integration/api_pull_test.go | 74 ++++++++ .../integration/api_repo_file_create_test.go | 4 +- tests/integration/api_repo_file_helpers.go | 42 +++-- tests/integration/pull_merge_test.go | 2 +- 20 files changed, 334 insertions(+), 264 deletions(-) create mode 100644 modules/util/error_test.go diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go index 511f7563cf..13e1ced0e1 100644 --- a/models/git/protected_branch.go +++ b/models/git/protected_branch.go @@ -5,7 +5,6 @@ package git import ( "context" - "errors" "fmt" "slices" "strings" @@ -25,7 +24,7 @@ import ( "xorm.io/builder" ) -var ErrBranchIsProtected = errors.New("branch is protected") +var ErrBranchIsProtected = util.ErrorWrap(util.ErrPermissionDenied, "branch is protected") // ProtectedBranch struct type ProtectedBranch struct { diff --git a/modules/util/error.go b/modules/util/error.go index 24fa1ba151..4c2566a2a3 100644 --- a/modules/util/error.go +++ b/modules/util/error.go @@ -6,6 +6,7 @@ package util import ( "errors" "fmt" + "html/template" ) // Common Errors forming the base of our error system @@ -40,22 +41,6 @@ func (w errorWrapper) Unwrap() error { return w.Err } -type LocaleWrapper struct { - err error - TrKey string - TrArgs []any -} - -// Error returns the message -func (w LocaleWrapper) Error() string { - return w.err.Error() -} - -// Unwrap returns the underlying error -func (w LocaleWrapper) Unwrap() error { - return w.err -} - // ErrorWrap returns an error that formats as the given text but unwraps as the provided error func ErrorWrap(unwrap error, message string, args ...any) error { if len(args) == 0 { @@ -84,15 +69,39 @@ func NewNotExistErrorf(message string, args ...any) error { return ErrorWrap(ErrNotExist, message, args...) } -// ErrorWrapLocale wraps an err with a translation key and arguments -func ErrorWrapLocale(err error, trKey string, trArgs ...any) error { - return LocaleWrapper{err: err, TrKey: trKey, TrArgs: trArgs} +// ErrorTranslatable wraps an error with translation information +type ErrorTranslatable interface { + error + Unwrap() error + Translate(ErrorLocaleTranslator) template.HTML } -func ErrorAsLocale(err error) *LocaleWrapper { - var e LocaleWrapper +type errorTranslatableWrapper struct { + err error + trKey string + trArgs []any +} + +type ErrorLocaleTranslator interface { + Tr(key string, args ...any) template.HTML +} + +func (w *errorTranslatableWrapper) Error() string { return w.err.Error() } + +func (w *errorTranslatableWrapper) Unwrap() error { return w.err } + +func (w *errorTranslatableWrapper) Translate(t ErrorLocaleTranslator) template.HTML { + return t.Tr(w.trKey, w.trArgs...) +} + +func ErrorWrapTranslatable(err error, trKey string, trArgs ...any) ErrorTranslatable { + return &errorTranslatableWrapper{err: err, trKey: trKey, trArgs: trArgs} +} + +func ErrorAsTranslatable(err error) ErrorTranslatable { + var e *errorTranslatableWrapper if errors.As(err, &e) { - return &e + return e } return nil } diff --git a/modules/util/error_test.go b/modules/util/error_test.go new file mode 100644 index 0000000000..31d0c91260 --- /dev/null +++ b/modules/util/error_test.go @@ -0,0 +1,29 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package util + +import ( + "io" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestErrorTranslatable(t *testing.T) { + var err error + + err = ErrorWrapTranslatable(io.EOF, "key", 1) + assert.ErrorIs(t, err, io.EOF) + assert.Equal(t, "EOF", err.Error()) + assert.Equal(t, "key", err.(*errorTranslatableWrapper).trKey) + assert.Equal(t, []any{1}, err.(*errorTranslatableWrapper).trArgs) + + err = ErrorWrap(err, "new msg %d", 100) + assert.ErrorIs(t, err, io.EOF) + assert.Equal(t, "new msg 100", err.Error()) + + errTr := ErrorAsTranslatable(err) + assert.Equal(t, "EOF", errTr.Error()) + assert.Equal(t, "key", errTr.(*errorTranslatableWrapper).trKey) +} diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 22851ad2c5..2561ff3975 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -13,7 +13,6 @@ import ( "time" activities_model "code.gitea.io/gitea/models/activities" - git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" access_model "code.gitea.io/gitea/models/perm/access" pull_model "code.gitea.io/gitea/models/pull" @@ -938,7 +937,7 @@ func MergePullRequest(ctx *context.APIContext) { } else if errors.Is(err, pull_service.ErrNoPermissionToMerge) { ctx.APIError(http.StatusMethodNotAllowed, "User not allowed to merge PR") } else if errors.Is(err, pull_service.ErrHasMerged) { - ctx.APIError(http.StatusMethodNotAllowed, "") + ctx.APIError(http.StatusMethodNotAllowed, "The PR is already merged") } else if errors.Is(err, pull_service.ErrIsWorkInProgress) { ctx.APIError(http.StatusMethodNotAllowed, "Work in progress PRs cannot be merged") } else if errors.Is(err, pull_service.ErrNotMergeableState) { @@ -989,8 +988,14 @@ func MergePullRequest(ctx *context.APIContext) { message += "\n\n" + form.MergeMessageField } + deleteBranchAfterMerge, err := pull_service.ShouldDeleteBranchAfterMerge(ctx, form.DeleteBranchAfterMerge, ctx.Repo.Repository, pr) + if err != nil { + ctx.APIErrorInternal(err) + return + } + if form.MergeWhenChecksSucceed { - scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, form.DeleteBranchAfterMerge) + scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, deleteBranchAfterMerge) if err != nil { if pull_model.IsErrAlreadyScheduledToAutoMerge(err) { ctx.APIError(http.StatusConflict, err) @@ -1035,47 +1040,10 @@ func MergePullRequest(ctx *context.APIContext) { } log.Trace("Pull request merged: %d", pr.ID) - // for agit flow, we should not delete the agit reference after merge - if form.DeleteBranchAfterMerge && pr.Flow == issues_model.PullRequestFlowGithub { - // check permission even it has been checked in repo_service.DeleteBranch so that we don't need to - // do RetargetChildrenOnMerge - if err := repo_service.CanDeleteBranch(ctx, pr.HeadRepo, pr.HeadBranch, ctx.Doer); err == nil { - // Don't cleanup when there are other PR's that use this branch as head branch. - exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch) - if err != nil { - ctx.APIErrorInternal(err) - return - } - if exist { - ctx.Status(http.StatusOK) - return - } - - var headRepo *git.Repository - if ctx.Repo != nil && ctx.Repo.Repository != nil && ctx.Repo.Repository.ID == pr.HeadRepoID && ctx.Repo.GitRepo != nil { - headRepo = ctx.Repo.GitRepo - } else { - headRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo) - if err != nil { - ctx.APIErrorInternal(err) - return - } - defer headRepo.Close() - } - - if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, headRepo, pr.HeadBranch, pr); err != nil { - switch { - case git.IsErrBranchNotExist(err): - ctx.APIErrorNotFound(err) - case errors.Is(err, repo_service.ErrBranchIsDefault): - ctx.APIError(http.StatusForbidden, errors.New("can not delete default branch")) - case errors.Is(err, git_model.ErrBranchIsProtected): - ctx.APIError(http.StatusForbidden, errors.New("branch protected")) - default: - ctx.APIErrorInternal(err) - } - return - } + if deleteBranchAfterMerge { + if err = repo_service.DeleteBranchAfterMerge(ctx, ctx.Doer, pr.ID, nil); err != nil { + // no way to tell users that what error happens, and the PR has been merged, so ignore the error + log.Debug("DeleteBranchAfterMerge: pr %d, err: %v", pr.ID, err) } } diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go index 013dab4acf..f7e57c91a8 100644 --- a/routers/web/repo/actions/view.go +++ b/routers/web/repo/actions/view.go @@ -943,8 +943,8 @@ func Run(ctx *context_module.Context) { return nil }) if err != nil { - if errLocale := util.ErrorAsLocale(err); errLocale != nil { - ctx.Flash.Error(ctx.Tr(errLocale.TrKey, errLocale.TrArgs...)) + if errTr := util.ErrorAsTranslatable(err); errTr != nil { + ctx.Flash.Error(errTr.Translate(ctx.Locale)) ctx.Redirect(redirectURL) } else { ctx.ServerError("DispatchActionWorkflow", err) diff --git a/routers/web/repo/editor_apply_patch.go b/routers/web/repo/editor_apply_patch.go index bd2811cc5f..aad7b4129c 100644 --- a/routers/web/repo/editor_apply_patch.go +++ b/routers/web/repo/editor_apply_patch.go @@ -41,7 +41,7 @@ func NewDiffPatchPost(ctx *context.Context) { Committer: parsed.GitCommitter, }) if err != nil { - err = util.ErrorWrapLocale(err, "repo.editor.fail_to_apply_patch") + err = util.ErrorWrapTranslatable(err, "repo.editor.fail_to_apply_patch") } if err != nil { editorHandleFileOperationError(ctx, parsed.NewBranchName, err) diff --git a/routers/web/repo/editor_cherry_pick.go b/routers/web/repo/editor_cherry_pick.go index 10c2741b1c..099814a9fa 100644 --- a/routers/web/repo/editor_cherry_pick.go +++ b/routers/web/repo/editor_cherry_pick.go @@ -74,7 +74,7 @@ func CherryPickPost(ctx *context.Context) { opts.Content = buf.String() _, err = files.ApplyDiffPatch(ctx, ctx.Repo.Repository, ctx.Doer, opts) if err != nil { - err = util.ErrorWrapLocale(err, "repo.editor.fail_to_apply_patch") + err = util.ErrorWrapTranslatable(err, "repo.editor.fail_to_apply_patch") } } if err != nil { diff --git a/routers/web/repo/editor_error.go b/routers/web/repo/editor_error.go index 245226a039..e1473a34b3 100644 --- a/routers/web/repo/editor_error.go +++ b/routers/web/repo/editor_error.go @@ -38,8 +38,8 @@ func editorHandleFileOperationErrorRender(ctx *context_service.Context, message, } func editorHandleFileOperationError(ctx *context_service.Context, targetBranchName string, err error) { - if errAs := util.ErrorAsLocale(err); errAs != nil { - ctx.JSONError(ctx.Tr(errAs.TrKey, errAs.TrArgs...)) + if errAs := util.ErrorAsTranslatable(err); errAs != nil { + ctx.JSONError(errAs.Translate(ctx.Locale)) } else if errAs, ok := errorAs[git.ErrNotExist](err); ok { ctx.JSONError(ctx.Tr("repo.editor.file_modifying_no_longer_exists", errAs.RelPath)) } else if errAs, ok := errorAs[git_model.ErrLFSFileLocked](err); ok { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index c8da025b15..1d7e5be31f 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1130,11 +1130,17 @@ func MergePullRequest(ctx *context.Context) { message += "\n\n" + form.MergeMessageField } + deleteBranchAfterMerge, err := pull_service.ShouldDeleteBranchAfterMerge(ctx, form.DeleteBranchAfterMerge, ctx.Repo.Repository, pr) + if err != nil { + ctx.ServerError("ShouldDeleteBranchAfterMerge", err) + return + } + if form.MergeWhenChecksSucceed { // delete all scheduled auto merges _ = pull_model.DeleteScheduledAutoMerge(ctx, pr.ID) // schedule auto merge - scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, form.DeleteBranchAfterMerge) + scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, deleteBranchAfterMerge) if err != nil { ctx.ServerError("ScheduleAutoMerge", err) return @@ -1220,37 +1226,29 @@ func MergePullRequest(ctx *context.Context) { log.Trace("Pull request merged: %d", pr.ID) - if !form.DeleteBranchAfterMerge { - ctx.JSONRedirect(issue.Link()) - return - } - - // Don't cleanup when other pr use this branch as head branch - exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch) - if err != nil { - ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err) - return - } - if exist { - ctx.JSONRedirect(issue.Link()) - return - } - - var headRepo *git.Repository - if ctx.Repo != nil && ctx.Repo.Repository != nil && pr.HeadRepoID == ctx.Repo.Repository.ID && ctx.Repo.GitRepo != nil { - headRepo = ctx.Repo.GitRepo - } else { - headRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo) - if err != nil { - ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err) + if deleteBranchAfterMerge { + deleteBranchAfterMergeAndFlashMessage(ctx, pr.ID) + if ctx.Written() { return } - defer headRepo.Close() } - deleteBranch(ctx, pr, headRepo) ctx.JSONRedirect(issue.Link()) } +func deleteBranchAfterMergeAndFlashMessage(ctx *context.Context, prID int64) { + var fullBranchName string + err := repo_service.DeleteBranchAfterMerge(ctx, ctx.Doer, prID, &fullBranchName) + if errTr := util.ErrorAsTranslatable(err); errTr != nil { + ctx.Flash.Error(errTr.Translate(ctx.Locale)) + return + } else if err == nil { + ctx.Flash.Success(ctx.Tr("repo.branch.deletion_success", fullBranchName)) + return + } + // catch unknown errors + ctx.ServerError("DeleteBranchAfterMerge", err) +} + // CancelAutoMergePullRequest cancels a scheduled pr func CancelAutoMergePullRequest(ctx *context.Context) { issue, ok := getPullInfo(ctx) @@ -1437,131 +1435,17 @@ func CompareAndPullRequestPost(ctx *context.Context) { } // CleanUpPullRequest responses for delete merged branch when PR has been merged +// Used by "DeleteBranchLink" for "delete branch" button func CleanUpPullRequest(ctx *context.Context) { issue, ok := getPullInfo(ctx) if !ok { return } - - pr := issue.PullRequest - - // Don't cleanup unmerged and unclosed PRs and agit PRs - if !pr.HasMerged && !issue.IsClosed && pr.Flow != issues_model.PullRequestFlowGithub { - ctx.NotFound(nil) + deleteBranchAfterMergeAndFlashMessage(ctx, issue.PullRequest.ID) + if ctx.Written() { return } - - // Don't cleanup when there are other PR's that use this branch as head branch. - exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch) - if err != nil { - ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err) - return - } - if exist { - ctx.NotFound(nil) - return - } - - if err := pr.LoadHeadRepo(ctx); err != nil { - ctx.ServerError("LoadHeadRepo", err) - return - } else if pr.HeadRepo == nil { - // Forked repository has already been deleted - ctx.NotFound(nil) - return - } else if err = pr.LoadBaseRepo(ctx); err != nil { - ctx.ServerError("LoadBaseRepo", err) - return - } else if err = pr.HeadRepo.LoadOwner(ctx); err != nil { - ctx.ServerError("HeadRepo.LoadOwner", err) - return - } - - if err := repo_service.CanDeleteBranch(ctx, pr.HeadRepo, pr.HeadBranch, ctx.Doer); err != nil { - if errors.Is(err, util.ErrPermissionDenied) { - ctx.NotFound(nil) - } else { - ctx.ServerError("CanDeleteBranch", err) - } - return - } - - fullBranchName := pr.HeadRepo.Owner.Name + "/" + pr.HeadBranch - - var gitBaseRepo *git.Repository - - // Assume that the base repo is the current context (almost certainly) - if ctx.Repo != nil && ctx.Repo.Repository != nil && ctx.Repo.Repository.ID == pr.BaseRepoID && ctx.Repo.GitRepo != nil { - gitBaseRepo = ctx.Repo.GitRepo - } else { - // If not just open it - gitBaseRepo, err = gitrepo.OpenRepository(ctx, pr.BaseRepo) - if err != nil { - ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.BaseRepo.FullName()), err) - return - } - defer gitBaseRepo.Close() - } - - // Now assume that the head repo is the same as the base repo (reasonable chance) - gitRepo := gitBaseRepo - // But if not: is it the same as the context? - if pr.BaseRepoID != pr.HeadRepoID && ctx.Repo != nil && ctx.Repo.Repository != nil && ctx.Repo.Repository.ID == pr.HeadRepoID && ctx.Repo.GitRepo != nil { - gitRepo = ctx.Repo.GitRepo - } else if pr.BaseRepoID != pr.HeadRepoID { - // Otherwise just load it up - gitRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo) - if err != nil { - ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err) - return - } - defer gitRepo.Close() - } - - defer func() { - ctx.JSONRedirect(issue.Link()) - }() - - // Check if branch has no new commits - headCommitID, err := gitBaseRepo.GetRefCommitID(pr.GetGitHeadRefName()) - if err != nil { - log.Error("GetRefCommitID: %v", err) - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - return - } - branchCommitID, err := gitRepo.GetBranchCommitID(pr.HeadBranch) - if err != nil { - log.Error("GetBranchCommitID: %v", err) - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - return - } - if headCommitID != branchCommitID { - ctx.Flash.Error(ctx.Tr("repo.branch.delete_branch_has_new_commits", fullBranchName)) - return - } - - deleteBranch(ctx, pr, gitRepo) -} - -func deleteBranch(ctx *context.Context, pr *issues_model.PullRequest, gitRepo *git.Repository) { - fullBranchName := pr.HeadRepo.FullName() + ":" + pr.HeadBranch - - if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, gitRepo, pr.HeadBranch, pr); err != nil { - switch { - case git.IsErrBranchNotExist(err): - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - case errors.Is(err, repo_service.ErrBranchIsDefault): - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - case errors.Is(err, git_model.ErrBranchIsProtected): - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - default: - log.Error("DeleteBranch: %v", err) - ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - } - return - } - - ctx.Flash.Success(ctx.Tr("repo.branch.deletion_success", fullBranchName)) + ctx.JSONRedirect(issue.Link()) } // DownloadPullDiff render a pull's raw diff diff --git a/services/actions/workflow.go b/services/actions/workflow.go index 25801d6fa1..4f72aad112 100644 --- a/services/actions/workflow.go +++ b/services/actions/workflow.go @@ -46,14 +46,14 @@ func EnableOrDisableWorkflow(ctx *context.APIContext, workflowID string, isEnabl func DispatchActionWorkflow(ctx reqctx.RequestContext, doer *user_model.User, repo *repo_model.Repository, gitRepo *git.Repository, workflowID, ref string, processInputs func(model *model.WorkflowDispatch, inputs map[string]any) error) error { if workflowID == "" { - return util.ErrorWrapLocale( + return util.ErrorWrapTranslatable( util.NewNotExistErrorf("workflowID is empty"), "actions.workflow.not_found", workflowID, ) } if ref == "" { - return util.ErrorWrapLocale( + return util.ErrorWrapTranslatable( util.NewNotExistErrorf("ref is empty"), "form.target_ref_not_exist", ref, ) @@ -63,7 +63,7 @@ func DispatchActionWorkflow(ctx reqctx.RequestContext, doer *user_model.User, re cfgUnit := repo.MustGetUnit(ctx, unit.TypeActions) cfg := cfgUnit.ActionsConfig() if cfg.IsWorkflowDisabled(workflowID) { - return util.ErrorWrapLocale( + return util.ErrorWrapTranslatable( util.NewPermissionDeniedErrorf("workflow is disabled"), "actions.workflow.disabled", ) @@ -82,7 +82,7 @@ func DispatchActionWorkflow(ctx reqctx.RequestContext, doer *user_model.User, re runTargetCommit, err = gitRepo.GetBranchCommit(ref) } if err != nil { - return util.ErrorWrapLocale( + return util.ErrorWrapTranslatable( util.NewNotExistErrorf("ref %q doesn't exist", ref), "form.target_ref_not_exist", ref, ) @@ -122,7 +122,7 @@ func DispatchActionWorkflow(ctx reqctx.RequestContext, doer *user_model.User, re } if entry == nil { - return util.ErrorWrapLocale( + return util.ErrorWrapTranslatable( util.NewNotExistErrorf("workflow %q doesn't exist", workflowID), "actions.workflow.not_found", workflowID, ) diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index a60883b4cc..eabfeff143 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -205,18 +205,6 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { return } - var headGitRepo *git.Repository - if pr.BaseRepoID == pr.HeadRepoID { - headGitRepo = baseGitRepo - } else { - headGitRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo) - if err != nil { - log.Error("OpenRepository %-v: %v", pr.HeadRepo, err) - return - } - defer headGitRepo.Close() - } - switch pr.Flow { case issues_model.PullRequestFlowGithub: headBranchExist := pr.HeadRepo != nil && gitrepo.IsBranchExist(ctx, pr.HeadRepo, pr.HeadBranch) @@ -276,9 +264,12 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { return } - if pr.Flow == issues_model.PullRequestFlowGithub && scheduledPRM.DeleteBranchAfterMerge { - if err := repo_service.DeleteBranch(ctx, doer, pr.HeadRepo, headGitRepo, pr.HeadBranch, pr); err != nil { - log.Error("DeletePullRequestHeadBranch: %v", err) + deleteBranchAfterMerge, err := pull_service.ShouldDeleteBranchAfterMerge(ctx, &scheduledPRM.DeleteBranchAfterMerge, pr.BaseRepo, pr) + if err != nil { + log.Error("ShouldDeleteBranchAfterMerge: %v", err) + } else if deleteBranchAfterMerge { + if err = repo_service.DeleteBranchAfterMerge(ctx, doer, pr.ID, nil); err != nil { + log.Error("DeleteBranchAfterMerge: %v", err) } } } diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index cb267f891c..67f24c4cbe 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -540,7 +540,7 @@ type MergePullRequestForm struct { HeadCommitID string `json:"head_commit_id,omitempty"` ForceMerge bool `json:"force_merge,omitempty"` MergeWhenChecksSucceed bool `json:"merge_when_checks_succeed,omitempty"` - DeleteBranchAfterMerge bool `json:"delete_branch_after_merge,omitempty"` + DeleteBranchAfterMerge *bool `json:"delete_branch_after_merge,omitempty"` } // Validate validates the fields diff --git a/services/pull/merge.go b/services/pull/merge.go index 1e8e9d444b..43bb3dd235 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -730,3 +730,24 @@ func SetMerged(ctx context.Context, pr *issues_model.PullRequest, mergedCommitID return true, nil }) } + +func ShouldDeleteBranchAfterMerge(ctx context.Context, userOption *bool, repo *repo_model.Repository, pr *issues_model.PullRequest) (bool, error) { + if pr.Flow != issues_model.PullRequestFlowGithub { + // only support GitHub workflow (branch-based) + // for agit workflow, there is no branch, so nothing to delete + // TODO: maybe in the future, it should delete the "agit reference (refs/for/xxxx)"? + return false, nil + } + + // if user has set an option, respect it + if userOption != nil { + return *userOption, nil + } + + // otherwise, use repository default + prUnit, err := repo.GetUnit(ctx, unit.TypePullRequests) + if err != nil { + return false, err + } + return prUnit.PullRequestsConfig().DefaultDeleteBranchAfterMerge, nil +} diff --git a/services/repository/branch.go b/services/repository/branch.go index 5d8178375e..b49478422c 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -484,10 +484,7 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, doer *user_m return "", nil } -// enmuerates all branch related errors -var ( - ErrBranchIsDefault = errors.New("branch is default") -) +var ErrBranchIsDefault = util.ErrorWrap(util.ErrPermissionDenied, "branch is default") func CanDeleteBranch(ctx context.Context, repo *repo_model.Repository, branchName string, doer *user_model.User) error { if branchName == repo.DefaultBranch { @@ -745,3 +742,89 @@ func GetBranchDivergingInfo(ctx reqctx.RequestContext, baseRepo *repo_model.Repo info.BaseHasNewCommits = info.HeadCommitsBehind > 0 return info, nil } + +func DeleteBranchAfterMerge(ctx context.Context, doer *user_model.User, prID int64, outFullBranchName *string) error { + pr, err := issues_model.GetPullRequestByID(ctx, prID) + if err != nil { + return err + } + if err = pr.LoadIssue(ctx); err != nil { + return err + } + if err = pr.LoadBaseRepo(ctx); err != nil { + return err + } + if err := pr.LoadHeadRepo(ctx); err != nil { + return err + } + if pr.HeadRepo == nil { + // Forked repository has already been deleted + return util.ErrorWrapTranslatable(util.ErrNotExist, "repo.branch.deletion_failed", "(deleted-repo):"+pr.HeadBranch) + } + if err = pr.HeadRepo.LoadOwner(ctx); err != nil { + return err + } + + fullBranchName := pr.HeadRepo.FullName() + ":" + pr.HeadBranch + if outFullBranchName != nil { + *outFullBranchName = fullBranchName + } + + errFailedToDelete := func(err error) error { + return util.ErrorWrapTranslatable(err, "repo.branch.deletion_failed", fullBranchName) + } + + // Don't clean up unmerged and unclosed PRs and agit PRs + if !pr.HasMerged && !pr.Issue.IsClosed && pr.Flow != issues_model.PullRequestFlowGithub { + return errFailedToDelete(util.ErrUnprocessableContent) + } + + // Don't clean up when there are other PR's that use this branch as head branch. + exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch) + if err != nil { + return err + } + if exist { + return errFailedToDelete(util.ErrUnprocessableContent) + } + + if err := CanDeleteBranch(ctx, pr.HeadRepo, pr.HeadBranch, doer); err != nil { + if errors.Is(err, util.ErrPermissionDenied) { + return errFailedToDelete(err) + } + return err + } + + gitBaseRepo, gitBaseCloser, err := gitrepo.RepositoryFromContextOrOpen(ctx, pr.BaseRepo) + if err != nil { + return err + } + defer gitBaseCloser.Close() + + gitHeadRepo, gitHeadCloser, err := gitrepo.RepositoryFromContextOrOpen(ctx, pr.HeadRepo) + if err != nil { + return err + } + defer gitHeadCloser.Close() + + // Check if branch has no new commits + headCommitID, err := gitBaseRepo.GetRefCommitID(pr.GetGitHeadRefName()) + if err != nil { + log.Error("GetRefCommitID: %v", err) + return errFailedToDelete(err) + } + branchCommitID, err := gitHeadRepo.GetBranchCommitID(pr.HeadBranch) + if err != nil { + log.Error("GetBranchCommitID: %v", err) + return errFailedToDelete(err) + } + if headCommitID != branchCommitID { + return util.ErrorWrapTranslatable(util.ErrUnprocessableContent, "repo.branch.delete_branch_has_new_commits", fullBranchName) + } + + err = DeleteBranch(ctx, doer, pr.HeadRepo, gitHeadRepo, pr.HeadBranch, pr) + if errors.Is(err, util.ErrPermissionDenied) || errors.Is(err, util.ErrNotExist) { + return errFailedToDelete(err) + } + return err +} diff --git a/templates/repo/issue/view_content/pull_merge_box.tmpl b/templates/repo/issue/view_content/pull_merge_box.tmpl index c0d717e854..2b943d2069 100644 --- a/templates/repo/issue/view_content/pull_merge_box.tmpl +++ b/templates/repo/issue/view_content/pull_merge_box.tmpl @@ -50,7 +50,7 @@
    - +
    {{end}} @@ -68,7 +68,7 @@
    {{if and .IsPullBranchDeletable (not .IsPullRequestBroken)}}
    - +
    {{end}}
    diff --git a/tests/integration/api_issue_config_test.go b/tests/integration/api_issue_config_test.go index ad39965443..f6045e1a80 100644 --- a/tests/integration/api_issue_config_test.go +++ b/tests/integration/api_issue_config_test.go @@ -129,7 +129,7 @@ func TestAPIRepoIssueConfigPaths(t *testing.T) { configData, err := yaml.Marshal(configMap) assert.NoError(t, err) - _, err = createFileInBranch(owner, repo, fullPath, repo.DefaultBranch, string(configData)) + _, err = createFile(owner, repo, fullPath, string(configData)) assert.NoError(t, err) issueConfig := getIssueConfig(t, owner.Name, repo.Name) diff --git a/tests/integration/api_pull_test.go b/tests/integration/api_pull_test.go index 8376fd2717..433dce3d5e 100644 --- a/tests/integration/api_pull_test.go +++ b/tests/integration/api_pull_test.go @@ -17,19 +17,23 @@ import ( issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/perm" repo_model "code.gitea.io/gitea/models/repo" + unit_model "code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/services/convert" "code.gitea.io/gitea/services/forms" "code.gitea.io/gitea/services/gitdiff" issue_service "code.gitea.io/gitea/services/issue" pull_service "code.gitea.io/gitea/services/pull" + repo_service "code.gitea.io/gitea/services/repository" files_service "code.gitea.io/gitea/services/repository/files" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestAPIViewPulls(t *testing.T) { @@ -186,6 +190,76 @@ func TestAPIMergePullWIP(t *testing.T) { MakeRequest(t, req, http.StatusMethodNotAllowed) } +func TestAPIMergePull(t *testing.T) { + onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + apiCtx := NewAPITestContext(t, repo.OwnerName, repo.Name, auth_model.AccessTokenScopeWriteRepository) + + checkBranchExists := func(t *testing.T, branchName string, status int) { + req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/%s/branches/%s", owner.Name, repo.Name, branchName)).AddTokenAuth(apiCtx.Token) + MakeRequest(t, req, status) + } + + createTestBranchPR := func(t *testing.T, branchName string) *api.PullRequest { + testCreateFileInBranch(t, owner, repo, createFileInBranchOptions{NewBranch: branchName}, map[string]string{"a-new-file-" + branchName + ".txt": "dummy content"}) + prDTO, err := doAPICreatePullRequest(apiCtx, repo.OwnerName, repo.Name, repo.DefaultBranch, branchName)(t) + require.NoError(t, err) + return &prDTO + } + + performMerge := func(t *testing.T, prIndex int64, params map[string]any, optExpectedStatus ...int) { + req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prIndex), params).AddTokenAuth(apiCtx.Token) + expectedStatus := util.OptionalArg(optExpectedStatus, http.StatusOK) + MakeRequest(t, req, expectedStatus) + } + + t.Run("Normal", func(t *testing.T) { + newBranch := "test-pull-1" + prDTO := createTestBranchPR(t, newBranch) + performMerge(t, prDTO.Index, map[string]any{"do": "merge"}) + checkBranchExists(t, newBranch, http.StatusOK) + // try to merge again, make sure we cannot perform a merge on the same PR + performMerge(t, prDTO.Index, map[string]any{"do": "merge"}, http.StatusMethodNotAllowed) + }) + + t.Run("DeleteBranchAfterMergePassedByFormField", func(t *testing.T) { + newBranch := "test-pull-2" + prDTO := createTestBranchPR(t, newBranch) + performMerge(t, prDTO.Index, map[string]any{"do": "merge", "delete_branch_after_merge": true}) + checkBranchExists(t, newBranch, http.StatusNotFound) + }) + + updateRepoUnitDefaultDeleteBranchAfterMerge := func(t *testing.T, repo *repo_model.Repository, value bool) { + prUnit, err := repo.GetUnit(t.Context(), unit_model.TypePullRequests) + require.NoError(t, err) + + prUnit.PullRequestsConfig().DefaultDeleteBranchAfterMerge = value + require.NoError(t, repo_service.UpdateRepositoryUnits(t.Context(), repo, []repo_model.RepoUnit{{ + RepoID: repo.ID, + Type: unit_model.TypePullRequests, + Config: prUnit.PullRequestsConfig(), + }}, nil)) + } + + t.Run("DeleteBranchAfterMergePassedByRepoSettings", func(t *testing.T) { + newBranch := "test-pull-3" + prDTO := createTestBranchPR(t, newBranch) + updateRepoUnitDefaultDeleteBranchAfterMerge(t, repo, true) + performMerge(t, prDTO.Index, map[string]any{"do": "merge"}) + checkBranchExists(t, newBranch, http.StatusNotFound) + }) + + t.Run("DeleteBranchAfterMergeFormFieldIsSetButNotRepoSettings", func(t *testing.T) { + newBranch := "test-pull-4" + prDTO := createTestBranchPR(t, newBranch) + updateRepoUnitDefaultDeleteBranchAfterMerge(t, repo, false) + performMerge(t, prDTO.Index, map[string]any{"do": "merge", "delete_branch_after_merge": true}) + checkBranchExists(t, newBranch, http.StatusNotFound) + }) + }) +} + func TestAPICreatePullSuccess(t *testing.T) { defer tests.PrepareTestEnv(t)() repo10 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10}) diff --git a/tests/integration/api_repo_file_create_test.go b/tests/integration/api_repo_file_create_test.go index af3bc54680..7cf1083248 100644 --- a/tests/integration/api_repo_file_create_test.go +++ b/tests/integration/api_repo_file_create_test.go @@ -133,7 +133,7 @@ func BenchmarkAPICreateFileSmall(b *testing.B) { b.ResetTimer() for n := 0; b.Loop(); n++ { treePath := fmt.Sprintf("update/file%d.txt", n) - _, _ = createFileInBranch(user2, repo1, treePath, repo1.DefaultBranch, treePath) + _, _ = createFile(user2, repo1, treePath) } }) } @@ -149,7 +149,7 @@ func BenchmarkAPICreateFileMedium(b *testing.B) { for n := 0; b.Loop(); n++ { treePath := fmt.Sprintf("update/file%d.txt", n) copy(data, treePath) - _, _ = createFileInBranch(user2, repo1, treePath, repo1.DefaultBranch, treePath) + _, _ = createFile(user2, repo1, treePath) } }) } diff --git a/tests/integration/api_repo_file_helpers.go b/tests/integration/api_repo_file_helpers.go index 37962028bd..785416b0f5 100644 --- a/tests/integration/api_repo_file_helpers.go +++ b/tests/integration/api_repo_file_helpers.go @@ -6,26 +6,36 @@ package integration import ( "context" "strings" + "testing" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/util" files_service "code.gitea.io/gitea/services/repository/files" + + "github.com/stretchr/testify/require" ) -func createFileInBranch(user *user_model.User, repo *repo_model.Repository, treePath, branchName, content string) (*api.FilesResponse, error) { +type createFileInBranchOptions struct { + OldBranch, NewBranch string +} + +func testCreateFileInBranch(t *testing.T, user *user_model.User, repo *repo_model.Repository, createOpts createFileInBranchOptions, files map[string]string) *api.FilesResponse { + resp, err := createFileInBranch(user, repo, createOpts, files) + require.NoError(t, err) + return resp +} + +func createFileInBranch(user *user_model.User, repo *repo_model.Repository, createOpts createFileInBranchOptions, files map[string]string) (*api.FilesResponse, error) { ctx := context.TODO() - opts := &files_service.ChangeRepoFilesOptions{ - Files: []*files_service.ChangeRepoFile{ - { - Operation: "create", - TreePath: treePath, - ContentReader: strings.NewReader(content), - }, - }, - OldBranch: branchName, - Author: nil, - Committer: nil, + opts := &files_service.ChangeRepoFilesOptions{OldBranch: createOpts.OldBranch, NewBranch: createOpts.NewBranch} + for path, content := range files { + opts.Files = append(opts.Files, &files_service.ChangeRepoFile{ + Operation: "create", + TreePath: path, + ContentReader: strings.NewReader(content), + }) } return files_service.ChangeRepoFiles(ctx, repo, user, opts) } @@ -53,10 +63,12 @@ func createOrReplaceFileInBranch(user *user_model.User, repo *repo_model.Reposit return err } - _, err = createFileInBranch(user, repo, treePath, branchName, content) + _, err = createFileInBranch(user, repo, createFileInBranchOptions{OldBranch: branchName}, map[string]string{treePath: content}) return err } -func createFile(user *user_model.User, repo *repo_model.Repository, treePath string) (*api.FilesResponse, error) { - return createFileInBranch(user, repo, treePath, repo.DefaultBranch, "This is a NEW file") +// TODO: replace all usages of this function with testCreateFileInBranch or testCreateFile +func createFile(user *user_model.User, repo *repo_model.Repository, treePath string, optContent ...string) (*api.FilesResponse, error) { + content := util.OptionalArg(optContent, "This is a NEW file") // some tests need this default content because its SHA is hardcoded + return createFileInBranch(user, repo, createFileInBranchOptions{}, map[string]string{treePath: content}) } diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 3345216838..32ab741382 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -93,7 +93,7 @@ func testPullCleanUp(t *testing.T, session *TestSession, user, repo, pullnum str // Click the little button to create a pull htmlDoc := NewHTMLParser(t, resp.Body) - link, exists := htmlDoc.doc.Find(".timeline-item .delete-button").Attr("data-url") + link, exists := htmlDoc.doc.Find(".timeline-item .delete-branch-after-merge").Attr("data-url") assert.True(t, exists, "The template has changed, can not find delete button url") req = NewRequestWithValues(t, "POST", link, map[string]string{ "_csrf": htmlDoc.GetCSRF(), From 08b97769702bf426859e1a83625369b46b7e4ce1 Mon Sep 17 00:00:00 2001 From: ChristopherHX Date: Wed, 22 Oct 2025 13:12:31 +0200 Subject: [PATCH 12/29] Refactor Actions Token Access (#35688) * use a single function to do Action Tokens Permission checks * allows easier customization * add basic tests * lfs file locks should work now --------- Signed-off-by: ChristopherHX Co-authored-by: wxiaoguang --- models/git/lfs.go | 31 ------ models/git/lfs_lock.go | 30 ----- models/perm/access/repo_permission.go | 30 +++++ models/user/user.go | 7 +- models/user/user_system.go | 21 ++-- models/user/user_test.go | 39 ++++--- routers/api/v1/api.go | 19 +--- routers/api/v1/repo/repo.go | 3 + routers/web/repo/githttp.go | 25 +---- services/convert/convert.go | 2 +- services/lfs/locks.go | 14 --- services/lfs/server.go | 25 ++--- tests/integration/actions_job_token_test.go | 117 ++++++++++++++++++++ tests/integration/api_repo_file_get_test.go | 6 +- tests/integration/api_repo_lfs_test.go | 17 +-- 15 files changed, 218 insertions(+), 168 deletions(-) create mode 100644 tests/integration/actions_job_token_test.go diff --git a/models/git/lfs.go b/models/git/lfs.go index 8bba060ff9..a4ae3e7bee 100644 --- a/models/git/lfs.go +++ b/models/git/lfs.go @@ -8,7 +8,6 @@ import ( "fmt" "code.gitea.io/gitea/models/db" - "code.gitea.io/gitea/models/perm" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" @@ -42,30 +41,6 @@ func (err ErrLFSLockNotExist) Unwrap() error { return util.ErrNotExist } -// ErrLFSUnauthorizedAction represents a "LFSUnauthorizedAction" kind of error. -type ErrLFSUnauthorizedAction struct { - RepoID int64 - UserName string - Mode perm.AccessMode -} - -// IsErrLFSUnauthorizedAction checks if an error is a ErrLFSUnauthorizedAction. -func IsErrLFSUnauthorizedAction(err error) bool { - _, ok := err.(ErrLFSUnauthorizedAction) - return ok -} - -func (err ErrLFSUnauthorizedAction) Error() string { - if err.Mode == perm.AccessModeWrite { - return fmt.Sprintf("User %s doesn't have write access for lfs lock [rid: %d]", err.UserName, err.RepoID) - } - return fmt.Sprintf("User %s doesn't have read access for lfs lock [rid: %d]", err.UserName, err.RepoID) -} - -func (err ErrLFSUnauthorizedAction) Unwrap() error { - return util.ErrPermissionDenied -} - // ErrLFSLockAlreadyExist represents a "LFSLockAlreadyExist" kind of error. type ErrLFSLockAlreadyExist struct { RepoID int64 @@ -93,12 +68,6 @@ type ErrLFSFileLocked struct { UserName string } -// IsErrLFSFileLocked checks if an error is a ErrLFSFileLocked. -func IsErrLFSFileLocked(err error) bool { - _, ok := err.(ErrLFSFileLocked) - return ok -} - func (err ErrLFSFileLocked) Error() string { return fmt.Sprintf("File is lfs locked [repo: %d, locked by: %s, path: %s]", err.RepoID, err.UserName, err.Path) } diff --git a/models/git/lfs_lock.go b/models/git/lfs_lock.go index c5f9a4e6de..184e616915 100644 --- a/models/git/lfs_lock.go +++ b/models/git/lfs_lock.go @@ -11,10 +11,7 @@ import ( "time" "code.gitea.io/gitea/models/db" - "code.gitea.io/gitea/models/perm" - access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" - "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" @@ -71,10 +68,6 @@ func (l *LFSLock) LoadOwner(ctx context.Context) error { // CreateLFSLock creates a new lock. func CreateLFSLock(ctx context.Context, repo *repo_model.Repository, lock *LFSLock) (*LFSLock, error) { return db.WithTx2(ctx, func(ctx context.Context) (*LFSLock, error) { - if err := CheckLFSAccessForRepo(ctx, lock.OwnerID, repo, perm.AccessModeWrite); err != nil { - return nil, err - } - lock.Path = util.PathJoinRel(lock.Path) lock.RepoID = repo.ID @@ -165,10 +158,6 @@ func DeleteLFSLockByID(ctx context.Context, id int64, repo *repo_model.Repositor return nil, err } - if err := CheckLFSAccessForRepo(ctx, u.ID, repo, perm.AccessModeWrite); err != nil { - return nil, err - } - if !force && u.ID != lock.OwnerID { return nil, errors.New("user doesn't own lock and force flag is not set") } @@ -180,22 +169,3 @@ func DeleteLFSLockByID(ctx context.Context, id int64, repo *repo_model.Repositor return lock, nil }) } - -// CheckLFSAccessForRepo check needed access mode base on action -func CheckLFSAccessForRepo(ctx context.Context, ownerID int64, repo *repo_model.Repository, mode perm.AccessMode) error { - if ownerID == 0 { - return ErrLFSUnauthorizedAction{repo.ID, "undefined", mode} - } - u, err := user_model.GetUserByID(ctx, ownerID) - if err != nil { - return err - } - perm, err := access_model.GetUserRepoPermission(ctx, repo, u) - if err != nil { - return err - } - if !perm.CanAccess(mode, unit.TypeCode) { - return ErrLFSUnauthorizedAction{repo.ID, u.DisplayName(), mode} - } - return nil -} diff --git a/models/perm/access/repo_permission.go b/models/perm/access/repo_permission.go index 678b18442e..df96db8d5a 100644 --- a/models/perm/access/repo_permission.go +++ b/models/perm/access/repo_permission.go @@ -5,9 +5,11 @@ package access import ( "context" + "errors" "fmt" "slices" + actions_model "code.gitea.io/gitea/models/actions" "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/organization" perm_model "code.gitea.io/gitea/models/perm" @@ -253,6 +255,34 @@ func finalProcessRepoUnitPermission(user *user_model.User, perm *Permission) { } } +// GetActionsUserRepoPermission returns the actions user permissions to the repository +func GetActionsUserRepoPermission(ctx context.Context, repo *repo_model.Repository, actionsUser *user_model.User, taskID int64) (perm Permission, err error) { + if actionsUser.ID != user_model.ActionsUserID { + return perm, errors.New("api GetActionsUserRepoPermission can only be called by the actions user") + } + task, err := actions_model.GetTaskByID(ctx, taskID) + if err != nil { + return perm, err + } + if task.RepoID != repo.ID { + // FIXME allow public repo read access if tokenless pull is enabled + return perm, nil + } + + var accessMode perm_model.AccessMode + if task.IsForkPullRequest { + accessMode = perm_model.AccessModeRead + } else { + accessMode = perm_model.AccessModeWrite + } + + if err := repo.LoadUnits(ctx); err != nil { + return perm, err + } + perm.SetUnitsWithDefaultAccessMode(repo.Units, accessMode) + return perm, nil +} + // GetUserRepoPermission returns the user permissions to the repository func GetUserRepoPermission(ctx context.Context, repo *repo_model.Repository, user *user_model.User) (perm Permission, err error) { defer func() { diff --git a/models/user/user.go b/models/user/user.go index 80d5eb5ec4..3583694cf9 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -249,8 +249,13 @@ func (u *User) MaxCreationLimit() int { } // CanCreateRepoIn checks whether the doer(u) can create a repository in the owner -// NOTE: functions calling this assume a failure due to repository count limit; it ONLY checks the repo number LIMIT, if new checks are added, those functions should be revised +// NOTE: functions calling this assume a failure due to repository count limit, or the owner is not a real user. +// It ONLY checks the repo number LIMIT or whether owner user is real. If new checks are added, those functions should be revised. +// TODO: the callers can only return ErrReachLimitOfRepo, need to fine tune to support other error types in the future. func (u *User) CanCreateRepoIn(owner *User) bool { + if u.ID <= 0 || owner.ID <= 0 { + return false // fake user like Ghost or Actions user + } if u.IsAdmin { return true } diff --git a/models/user/user_system.go b/models/user/user_system.go index e07274d291..11008c77d4 100644 --- a/models/user/user_system.go +++ b/models/user/user_system.go @@ -48,17 +48,16 @@ func IsGiteaActionsUserName(name string) bool { // NewActionsUser creates and returns a fake user for running the actions. func NewActionsUser() *User { return &User{ - ID: ActionsUserID, - Name: ActionsUserName, - LowerName: ActionsUserName, - IsActive: true, - FullName: "Gitea Actions", - Email: ActionsUserEmail, - KeepEmailPrivate: true, - LoginName: ActionsUserName, - Type: UserTypeBot, - AllowCreateOrganization: true, - Visibility: structs.VisibleTypePublic, + ID: ActionsUserID, + Name: ActionsUserName, + LowerName: ActionsUserName, + IsActive: true, + FullName: "Gitea Actions", + Email: ActionsUserEmail, + KeepEmailPrivate: true, + LoginName: ActionsUserName, + Type: UserTypeBot, + Visibility: structs.VisibleTypePublic, } } diff --git a/models/user/user_test.go b/models/user/user_test.go index 4201ec4816..6a530553d7 100644 --- a/models/user/user_test.go +++ b/models/user/user_test.go @@ -648,33 +648,36 @@ func TestGetInactiveUsers(t *testing.T) { func TestCanCreateRepo(t *testing.T) { defer test.MockVariableValue(&setting.Repository.MaxCreationLimit)() const noLimit = -1 - doerNormal := &user_model.User{} - doerAdmin := &user_model.User{IsAdmin: true} + doerActions := user_model.NewActionsUser() + doerNormal := &user_model.User{ID: 2} + doerAdmin := &user_model.User{ID: 1, IsAdmin: true} t.Run("NoGlobalLimit", func(t *testing.T) { setting.Repository.MaxCreationLimit = noLimit - assert.True(t, doerNormal.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: noLimit})) - assert.False(t, doerNormal.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: 0})) - assert.True(t, doerNormal.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: 100})) + assert.False(t, doerNormal.CanCreateRepoIn(&user_model.User{ID: 2, NumRepos: 10, MaxRepoCreation: 0})) + assert.True(t, doerNormal.CanCreateRepoIn(&user_model.User{ID: 2, NumRepos: 10, MaxRepoCreation: 100})) + assert.True(t, doerNormal.CanCreateRepoIn(&user_model.User{ID: 2, NumRepos: 10, MaxRepoCreation: noLimit})) - assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: noLimit})) - assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: 0})) - assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: 100})) + assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{ID: 2, NumRepos: 10, MaxRepoCreation: 0})) + assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{ID: 2, NumRepos: 10, MaxRepoCreation: 100})) + assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{ID: 2, NumRepos: 10, MaxRepoCreation: noLimit})) + assert.False(t, doerActions.CanCreateRepoIn(&user_model.User{ID: 2, NumRepos: 10, MaxRepoCreation: noLimit})) + assert.False(t, doerAdmin.CanCreateRepoIn(doerActions)) }) t.Run("GlobalLimit50", func(t *testing.T) { setting.Repository.MaxCreationLimit = 50 - assert.True(t, doerNormal.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: noLimit})) - assert.False(t, doerNormal.CanCreateRepoIn(&user_model.User{NumRepos: 60, MaxRepoCreation: noLimit})) // limited by global limit - assert.False(t, doerNormal.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: 0})) - assert.True(t, doerNormal.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: 100})) - assert.True(t, doerNormal.CanCreateRepoIn(&user_model.User{NumRepos: 60, MaxRepoCreation: 100})) + assert.True(t, doerNormal.CanCreateRepoIn(&user_model.User{ID: 2, NumRepos: 10, MaxRepoCreation: noLimit})) + assert.False(t, doerNormal.CanCreateRepoIn(&user_model.User{ID: 2, NumRepos: 60, MaxRepoCreation: noLimit})) // limited by global limit + assert.False(t, doerNormal.CanCreateRepoIn(&user_model.User{ID: 2, NumRepos: 10, MaxRepoCreation: 0})) + assert.True(t, doerNormal.CanCreateRepoIn(&user_model.User{ID: 2, NumRepos: 10, MaxRepoCreation: 100})) + assert.True(t, doerNormal.CanCreateRepoIn(&user_model.User{ID: 2, NumRepos: 60, MaxRepoCreation: 100})) - assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: noLimit})) - assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{NumRepos: 60, MaxRepoCreation: noLimit})) - assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: 0})) - assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{NumRepos: 10, MaxRepoCreation: 100})) - assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{NumRepos: 60, MaxRepoCreation: 100})) + assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{ID: 2, NumRepos: 10, MaxRepoCreation: noLimit})) + assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{ID: 2, NumRepos: 60, MaxRepoCreation: noLimit})) + assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{ID: 2, NumRepos: 10, MaxRepoCreation: 0})) + assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{ID: 2, NumRepos: 10, MaxRepoCreation: 100})) + assert.True(t, doerAdmin.CanCreateRepoIn(&user_model.User{ID: 2, NumRepos: 60, MaxRepoCreation: 100})) }) } diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 5f52ee8a43..e6238acce0 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -70,7 +70,6 @@ import ( "net/http" "strings" - actions_model "code.gitea.io/gitea/models/actions" auth_model "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/perm" @@ -190,27 +189,11 @@ func repoAssignment() func(ctx *context.APIContext) { if ctx.Doer != nil && ctx.Doer.ID == user_model.ActionsUserID { taskID := ctx.Data["ActionsTaskID"].(int64) - task, err := actions_model.GetTaskByID(ctx, taskID) + ctx.Repo.Permission, err = access_model.GetActionsUserRepoPermission(ctx, repo, ctx.Doer, taskID) if err != nil { ctx.APIErrorInternal(err) return } - if task.RepoID != repo.ID { - ctx.APIErrorNotFound() - return - } - - if task.IsForkPullRequest { - ctx.Repo.Permission.AccessMode = perm.AccessModeRead - } else { - ctx.Repo.Permission.AccessMode = perm.AccessModeWrite - } - - if err := ctx.Repo.Repository.LoadUnits(ctx); err != nil { - ctx.APIErrorInternal(err) - return - } - ctx.Repo.Permission.SetUnitsWithDefaultAccessMode(ctx.Repo.Repository.Units, ctx.Repo.Permission.AccessMode) } else { needTwoFactor, err := doerNeedTwoFactorAuth(ctx, ctx.Doer) if err != nil { diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go index 2f5a969b6b..bb6bda587d 100644 --- a/routers/api/v1/repo/repo.go +++ b/routers/api/v1/repo/repo.go @@ -28,6 +28,7 @@ import ( repo_module "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/validation" "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/routers/api/v1/utils" @@ -270,6 +271,8 @@ func CreateUserRepo(ctx *context.APIContext, owner *user_model.User, opt api.Cre db.IsErrNamePatternNotAllowed(err) || label.IsErrTemplateLoad(err) { ctx.APIError(http.StatusUnprocessableEntity, err) + } else if errors.Is(err, util.ErrPermissionDenied) { + ctx.APIError(http.StatusForbidden, err) } else { ctx.APIErrorInternal(err) } diff --git a/routers/web/repo/githttp.go b/routers/web/repo/githttp.go index 69b93dd060..1b1c272a8d 100644 --- a/routers/web/repo/githttp.go +++ b/routers/web/repo/githttp.go @@ -18,7 +18,6 @@ import ( "sync" "time" - actions_model "code.gitea.io/gitea/models/actions" auth_model "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/perm" access_model "code.gitea.io/gitea/models/perm/access" @@ -190,29 +189,17 @@ func httpBase(ctx *context.Context) *serviceHandler { if ctx.Data["IsActionsToken"] == true { taskID := ctx.Data["ActionsTaskID"].(int64) - task, err := actions_model.GetTaskByID(ctx, taskID) + p, err := access_model.GetActionsUserRepoPermission(ctx, repo, ctx.Doer, taskID) if err != nil { - ctx.ServerError("GetTaskByID", err) - return nil - } - if task.RepoID != repo.ID { - ctx.PlainText(http.StatusForbidden, "User permission denied") + ctx.ServerError("GetUserRepoPermission", err) return nil } - if task.IsForkPullRequest { - if accessMode > perm.AccessModeRead { - ctx.PlainText(http.StatusForbidden, "User permission denied") - return nil - } - environ = append(environ, fmt.Sprintf("%s=%d", repo_module.EnvActionPerm, perm.AccessModeRead)) - } else { - if accessMode > perm.AccessModeWrite { - ctx.PlainText(http.StatusForbidden, "User permission denied") - return nil - } - environ = append(environ, fmt.Sprintf("%s=%d", repo_module.EnvActionPerm, perm.AccessModeWrite)) + if !p.CanAccess(accessMode, unitType) { + ctx.PlainText(http.StatusNotFound, "Repository not found") + return nil } + environ = append(environ, fmt.Sprintf("%s=%d", repo_module.EnvActionPerm, p.UnitAccessMode(unitType))) } else { p, err := access_model.GetUserRepoPermission(ctx, repo, ctx.Doer) if err != nil { diff --git a/services/convert/convert.go b/services/convert/convert.go index 0de3822140..9f8fff970c 100644 --- a/services/convert/convert.go +++ b/services/convert/convert.go @@ -771,7 +771,7 @@ func ToOAuth2Application(app *auth.OAuth2Application) *api.OAuth2Application { // ToLFSLock convert a LFSLock to api.LFSLock func ToLFSLock(ctx context.Context, l *git_model.LFSLock) *api.LFSLock { - u, err := user_model.GetUserByID(ctx, l.OwnerID) + u, err := user_model.GetPossibleUserByID(ctx, l.OwnerID) if err != nil { return nil } diff --git a/services/lfs/locks.go b/services/lfs/locks.go index 264001f0f9..5bc3f6b95a 100644 --- a/services/lfs/locks.go +++ b/services/lfs/locks.go @@ -187,13 +187,6 @@ func PostLockHandler(ctx *context.Context) { }) return } - if git_model.IsErrLFSUnauthorizedAction(err) { - ctx.Resp.Header().Set("WWW-Authenticate", `Basic realm="gitea-lfs"`) - ctx.JSON(http.StatusUnauthorized, api.LFSLockError{ - Message: "You must have push access to create locks : " + err.Error(), - }) - return - } log.Error("Unable to CreateLFSLock in repository %-v at %s for user %-v: Error: %v", repository, req.Path, ctx.Doer, err) ctx.JSON(http.StatusInternalServerError, api.LFSLockError{ Message: "internal server error : Internal Server Error", @@ -317,13 +310,6 @@ func UnLockHandler(ctx *context.Context) { lock, err := git_model.DeleteLFSLockByID(ctx, ctx.PathParamInt64("lid"), repository, ctx.Doer, req.Force) if err != nil { - if git_model.IsErrLFSUnauthorizedAction(err) { - ctx.Resp.Header().Set("WWW-Authenticate", `Basic realm="gitea-lfs"`) - ctx.JSON(http.StatusUnauthorized, api.LFSLockError{ - Message: "You must have push access to delete locks : " + err.Error(), - }) - return - } log.Error("Unable to DeleteLFSLockByID[%d] by user %-v with force %t: Error: %v", ctx.PathParamInt64("lid"), ctx.Doer, req.Force, err) ctx.JSON(http.StatusInternalServerError, api.LFSLockError{ Message: "unable to delete lock : Internal Server Error", diff --git a/services/lfs/server.go b/services/lfs/server.go index 4a1e0aaf7d..81991de434 100644 --- a/services/lfs/server.go +++ b/services/lfs/server.go @@ -20,7 +20,6 @@ import ( "strings" "time" - actions_model "code.gitea.io/gitea/models/actions" auth_model "code.gitea.io/gitea/models/auth" git_model "code.gitea.io/gitea/models/git" perm_model "code.gitea.io/gitea/models/perm" @@ -549,33 +548,31 @@ func authenticate(ctx *context.Context, repository *repo_model.Repository, autho if ctx.Data["IsActionsToken"] == true { taskID := ctx.Data["ActionsTaskID"].(int64) - task, err := actions_model.GetTaskByID(ctx, taskID) + perm, err := access_model.GetActionsUserRepoPermission(ctx, repository, ctx.Doer, taskID) if err != nil { - log.Error("Unable to GetTaskByID for task[%d] Error: %v", taskID, err) + log.Error("Unable to GetActionsUserRepoPermission for task[%d] Error: %v", taskID, err) return false } - if task.RepoID != repository.ID { - return false - } - - if task.IsForkPullRequest { - return accessMode <= perm_model.AccessModeRead - } - return accessMode <= perm_model.AccessModeWrite + return perm.CanAccess(accessMode, unit.TypeCode) } - // ctx.IsSigned is unnecessary here, this will be checked in perm.CanAccess + // it works for both anonymous request and signed-in user, then perm.CanAccess will do the permission check perm, err := access_model.GetUserRepoPermission(ctx, repository, ctx.Doer) if err != nil { log.Error("Unable to GetUserRepoPermission for user %-v in repo %-v Error: %v", ctx.Doer, repository, err) return false } - canRead := perm.CanAccess(accessMode, unit.TypeCode) - if canRead && (!requireSigned || ctx.IsSigned) { + canAccess := perm.CanAccess(accessMode, unit.TypeCode) + // if it doesn't require sign-in and anonymous user has access, return true + // if the user is already signed in (for example: by session auth method), and the doer can access, return true + if canAccess && (!requireSigned || ctx.IsSigned) { return true } + // now, either sign-in is required or the ctx.Doer cannot access, check the LFS token + // however, "ctx.Doer exists but cannot access then check LFS token" should not really happen: + // * why a request can be sent with both valid user session and valid LFS token then use LFS token to access? user, err := parseToken(ctx, authorization, repository, accessMode) if err != nil { // Most of these are Warn level - the true internal server errors are logged in parseToken already diff --git a/tests/integration/actions_job_token_test.go b/tests/integration/actions_job_token_test.go new file mode 100644 index 0000000000..c4e8e880eb --- /dev/null +++ b/tests/integration/actions_job_token_test.go @@ -0,0 +1,117 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package integration + +import ( + "encoding/base64" + "net/http" + "net/url" + "testing" + + actions_model "code.gitea.io/gitea/models/actions" + auth_model "code.gitea.io/gitea/models/auth" + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/unittest" + "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/util" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestActionsJobTokenAccess(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + t.Run("Write Access", testActionsJobTokenAccess(u, false)) + t.Run("Read Access", testActionsJobTokenAccess(u, true)) + }) +} + +func testActionsJobTokenAccess(u *url.URL, isFork bool) func(t *testing.T) { + return func(t *testing.T) { + task := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: 47}) + require.NoError(t, task.GenerateToken()) + task.Status = actions_model.StatusRunning + task.IsForkPullRequest = isFork + err := actions_model.UpdateTask(t.Context(), task, "token_hash", "token_salt", "token_last_eight", "status", "is_fork_pull_request") + require.NoError(t, err) + session := emptyTestSession(t) + context := APITestContext{ + Session: session, + Token: task.Token, + Username: "user5", + Reponame: "repo4", + } + dstPath := t.TempDir() + + u.Path = context.GitPath() + u.User = url.UserPassword("gitea-actions", task.Token) + + t.Run("Git Clone", doGitClone(dstPath, u)) + + t.Run("API Get Repository", doAPIGetRepository(context, func(t *testing.T, r structs.Repository) { + require.Equal(t, "repo4", r.Name) + require.Equal(t, "user5", r.Owner.UserName) + })) + + context.ExpectedCode = util.Iif(isFork, http.StatusForbidden, http.StatusCreated) + t.Run("API Create File", doAPICreateFile(context, "test.txt", &structs.CreateFileOptions{ + FileOptions: structs.FileOptions{ + NewBranchName: "new-branch", + Message: "Create File", + }, + ContentBase64: base64.StdEncoding.EncodeToString([]byte(`This is a test file created using job token.`)), + })) + + context.ExpectedCode = http.StatusForbidden + t.Run("Fail to Create Repository", doAPICreateRepository(context, true)) + + context.ExpectedCode = http.StatusForbidden + t.Run("Fail to Delete Repository", doAPIDeleteRepository(context)) + + t.Run("Fail to Create Organization", doAPICreateOrganization(context, &structs.CreateOrgOption{ + UserName: "actions", + FullName: "Gitea Actions", + })) + } +} + +func TestActionsJobTokenAccessLFS(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + httpContext := NewAPITestContext(t, "user2", "repo-lfs-test", auth_model.AccessTokenScopeWriteUser, auth_model.AccessTokenScopeWriteRepository) + t.Run("Create Repository", doAPICreateRepository(httpContext, false, func(t *testing.T, repository structs.Repository) { + task := &actions_model.ActionTask{} + require.NoError(t, task.GenerateToken()) + task.Status = actions_model.StatusRunning + task.IsForkPullRequest = false + task.RepoID = repository.ID + err := db.Insert(t.Context(), task) + require.NoError(t, err) + session := emptyTestSession(t) + httpContext := APITestContext{ + Session: session, + Token: task.Token, + Username: "user2", + Reponame: "repo-lfs-test", + } + + u.Path = httpContext.GitPath() + dstPath := t.TempDir() + + u.Path = httpContext.GitPath() + u.User = url.UserPassword("gitea-actions", task.Token) + + t.Run("Clone", doGitClone(dstPath, u)) + + dstPath2 := t.TempDir() + + t.Run("Partial Clone", doPartialGitClone(dstPath2, u)) + + lfs := lfsCommitAndPushTest(t, dstPath, testFileSizeSmall)[0] + + reqLFS := NewRequest(t, "GET", "/api/v1/repos/user2/repo-lfs-test/media/"+lfs).AddTokenAuth(task.Token) + respLFS := MakeRequestNilResponseRecorder(t, reqLFS, http.StatusOK) + assert.Equal(t, testFileSizeSmall, respLFS.Length) + })) + }) +} diff --git a/tests/integration/api_repo_file_get_test.go b/tests/integration/api_repo_file_get_test.go index 379851b689..ec50cf52f4 100644 --- a/tests/integration/api_repo_file_get_test.go +++ b/tests/integration/api_repo_file_get_test.go @@ -9,7 +9,6 @@ import ( "testing" auth_model "code.gitea.io/gitea/models/auth" - api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" @@ -25,8 +24,9 @@ func TestAPIGetRawFileOrLFS(t *testing.T) { // Test with LFS onGiteaRun(t, func(t *testing.T, u *url.URL) { + createLFSTestRepository(t, "repo-lfs-test") httpContext := NewAPITestContext(t, "user2", "repo-lfs-test", auth_model.AccessTokenScopeWriteRepository) - doAPICreateRepository(httpContext, false, func(t *testing.T, repository api.Repository) { + t.Run("repo-lfs-test", func(t *testing.T) { u.Path = httpContext.GitPath() dstPath := t.TempDir() @@ -41,7 +41,7 @@ func TestAPIGetRawFileOrLFS(t *testing.T) { lfs := lfsCommitAndPushTest(t, dstPath, testFileSizeSmall)[0] - reqLFS := NewRequest(t, "GET", "/api/v1/repos/user2/repo1/media/"+lfs) + reqLFS := NewRequest(t, "GET", "/api/v1/repos/user2/repo-lfs-test/media/"+lfs).AddTokenAuth(httpContext.Token) respLFS := MakeRequestNilResponseRecorder(t, reqLFS, http.StatusOK) assert.Equal(t, testFileSizeSmall, respLFS.Length) }) diff --git a/tests/integration/api_repo_lfs_test.go b/tests/integration/api_repo_lfs_test.go index da4c89a1b7..fb55d311cc 100644 --- a/tests/integration/api_repo_lfs_test.go +++ b/tests/integration/api_repo_lfs_test.go @@ -23,6 +23,7 @@ import ( "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestAPILFSNotStarted(t *testing.T) { @@ -59,12 +60,12 @@ func TestAPILFSMediaType(t *testing.T) { MakeRequest(t, req, http.StatusUnsupportedMediaType) } -func createLFSTestRepository(t *testing.T, name string) *repo_model.Repository { - ctx := NewAPITestContext(t, "user2", "lfs-"+name+"-repo", auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser) +func createLFSTestRepository(t *testing.T, repoName string) *repo_model.Repository { + ctx := NewAPITestContext(t, "user2", repoName, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser) t.Run("CreateRepo", doAPICreateRepository(ctx, false)) - repo, err := repo_model.GetRepositoryByOwnerAndName(t.Context(), "user2", "lfs-"+name+"-repo") - assert.NoError(t, err) + repo, err := repo_model.GetRepositoryByOwnerAndName(t.Context(), "user2", repoName) + require.NoError(t, err) return repo } @@ -74,7 +75,7 @@ func TestAPILFSBatch(t *testing.T) { setting.LFS.StartServer = true - repo := createLFSTestRepository(t, "batch") + repo := createLFSTestRepository(t, "lfs-batch-repo") content := []byte("dummy1") oid := storeObjectInRepo(t, repo.ID, &content) @@ -253,7 +254,7 @@ func TestAPILFSBatch(t *testing.T) { assert.NoError(t, err) assert.True(t, exist) - repo2 := createLFSTestRepository(t, "batch2") + repo2 := createLFSTestRepository(t, "lfs-batch2-repo") content := []byte("dummy0") storeObjectInRepo(t, repo2.ID, &content) @@ -329,7 +330,7 @@ func TestAPILFSUpload(t *testing.T) { setting.LFS.StartServer = true - repo := createLFSTestRepository(t, "upload") + repo := createLFSTestRepository(t, "lfs-upload-repo") content := []byte("dummy3") oid := storeObjectInRepo(t, repo.ID, &content) @@ -433,7 +434,7 @@ func TestAPILFSVerify(t *testing.T) { setting.LFS.StartServer = true - repo := createLFSTestRepository(t, "verify") + repo := createLFSTestRepository(t, "lfs-verify-repo") content := []byte("dummy3") oid := storeObjectInRepo(t, repo.ID, &content) From 195fc715ff6a8c67c1b3473316b52379bfca6d49 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 23 Oct 2025 07:41:38 +0800 Subject: [PATCH 13/29] Fix external render (#35727) Fix #35725 --------- Signed-off-by: wxiaoguang --- custom/conf/app.example.ini | 6 ++ modules/markup/external/external.go | 7 ++- modules/markup/render.go | 48 +++++++++----- modules/setting/markup.go | 4 +- routers/web/repo/view.go | 17 ++++- tests/integration/markup_external_test.go | 77 ++++++++++++++--------- tests/sqlite.ini.tmpl | 13 +++- 7 files changed, 120 insertions(+), 52 deletions(-) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index 4a6356ec50..b77da3ab79 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -2541,6 +2541,12 @@ LEVEL = Info ;; * no-sanitizer: Disable the sanitizer and render the content inside current page. It's **insecure** and may lead to XSS attack if the content contains malicious code. ;; * iframe: Render the content in a separate standalone page and embed it into current page by iframe. The iframe is in sandbox mode with same-origin disabled, and the JS code are safely isolated from parent page. ;RENDER_CONTENT_MODE=sanitized +;; +;; Whether post-process the rendered HTML content, including: +;; resolve relative links and image sources, recognizing issue/commit references, escaping invisible characters, +;; mentioning users, rendering permlink code blocks, replacing emoji shorthands, etc. +;; By default, this is true when RENDER_CONTENT_MODE is `sanitized`, otherwise false. +;NEED_POST_PROCESS=false ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; diff --git a/modules/markup/external/external.go b/modules/markup/external/external.go index 39861ade12..be40a50aad 100644 --- a/modules/markup/external/external.go +++ b/modules/markup/external/external.go @@ -15,6 +15,8 @@ import ( "code.gitea.io/gitea/modules/markup" "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/setting" + + "github.com/kballard/go-shellquote" ) // RegisterRenderers registers all supported third part renderers according settings @@ -81,7 +83,10 @@ func (p *Renderer) Render(ctx *markup.RenderContext, input io.Reader, output io. envMark("GITEA_PREFIX_SRC"), baseLinkSrc, envMark("GITEA_PREFIX_RAW"), baseLinkRaw, ).Replace(p.Command) - commands := strings.Fields(command) + commands, err := shellquote.Split(command) + if err != nil || len(commands) == 0 { + return fmt.Errorf("%s invalid command %q: %w", p.Name(), p.Command, err) + } args := commands[1:] if p.IsInputFile { diff --git a/modules/markup/render.go b/modules/markup/render.go index 79f1f473c2..4eff9734ae 100644 --- a/modules/markup/render.go +++ b/modules/markup/render.go @@ -120,31 +120,38 @@ func (ctx *RenderContext) WithHelper(helper RenderHelper) *RenderContext { return ctx } -// Render renders markup file to HTML with all specific handling stuff. -func Render(ctx *RenderContext, input io.Reader, output io.Writer) error { +// FindRendererByContext finds renderer by RenderContext +// TODO: it should be merged with other similar functions like GetRendererByFileName, DetectMarkupTypeByFileName, etc +func FindRendererByContext(ctx *RenderContext) (Renderer, error) { if ctx.RenderOptions.MarkupType == "" && ctx.RenderOptions.RelativePath != "" { ctx.RenderOptions.MarkupType = DetectMarkupTypeByFileName(ctx.RenderOptions.RelativePath) if ctx.RenderOptions.MarkupType == "" { - return util.NewInvalidArgumentErrorf("unsupported file to render: %q", ctx.RenderOptions.RelativePath) + return nil, util.NewInvalidArgumentErrorf("unsupported file to render: %q", ctx.RenderOptions.RelativePath) } } renderer := renderers[ctx.RenderOptions.MarkupType] if renderer == nil { - return util.NewInvalidArgumentErrorf("unsupported markup type: %q", ctx.RenderOptions.MarkupType) + return nil, util.NewNotExistErrorf("unsupported markup type: %q", ctx.RenderOptions.MarkupType) } - if ctx.RenderOptions.RelativePath != "" { - if externalRender, ok := renderer.(ExternalRenderer); ok && externalRender.DisplayInIFrame() { - if !ctx.RenderOptions.InStandalonePage { - // for an external "DisplayInIFrame" render, it could only output its content in a standalone page - // otherwise, a `, - setting.AppSubURL, +func renderIFrame(ctx *RenderContext, sandbox string, output io.Writer) error { + src := fmt.Sprintf("%s/%s/%s/render/%s/%s", setting.AppSubURL, url.PathEscape(ctx.RenderOptions.Metas["user"]), url.PathEscape(ctx.RenderOptions.Metas["repo"]), - ctx.RenderOptions.Metas["RefTypeNameSubURL"], - url.PathEscape(ctx.RenderOptions.RelativePath), - )) + util.PathEscapeSegments(ctx.RenderOptions.Metas["RefTypeNameSubURL"]), + util.PathEscapeSegments(ctx.RenderOptions.RelativePath), + ) + + var sandboxAttrValue template.HTML + if sandbox != "" { + sandboxAttrValue = htmlutil.HTMLFormat(`sandbox="%s"`, sandbox) + } + iframe := htmlutil.HTMLFormat(``, src, sandboxAttrValue) + _, err := io.WriteString(output, string(iframe)) return err } @@ -192,14 +190,26 @@ func pipes() (io.ReadCloser, io.WriteCloser, func()) { } } +func getExternalRendererOptions(renderer Renderer) (ret ExternalRendererOptions, _ bool) { + if externalRender, ok := renderer.(ExternalRenderer); ok { + return externalRender.GetExternalRendererOptions(), true + } + return ret, false +} + func RenderWithRenderer(ctx *RenderContext, renderer Renderer, input io.Reader, output io.Writer) error { - if externalRender, ok := renderer.(ExternalRenderer); ok && externalRender.DisplayInIFrame() { + var extraHeadHTML template.HTML + if extOpts, ok := getExternalRendererOptions(renderer); ok && extOpts.DisplayInIframe { if !ctx.RenderOptions.InStandalonePage { // for an external "DisplayInIFrame" render, it could only output its content in a standalone page // otherwise, a