From 178d33ed4dce734384caa61a04290b30ab955b21 Mon Sep 17 00:00:00 2001 From: bircni Date: Tue, 27 Oct 2026 21:40:42 +0100 Subject: [PATCH] fix: add missing access checks and size limits (#39942) Several handlers and parsers skipped checks that their siblings already perform. - Reject issue, comment, label, milestone and release mutations in archived repositories via the API with one guard per route group, matching the web UI - Hide timeline references and dependencies pointing at issues the doer cannot read, in the web UI and the API, including pull requests of the same repository without pull request access - Reject workflow runs with more than `MaxJobNumPerRun` jobs, including inline reusable workflow jobs, and show rejected pushes as invalid workflow runs - Bound how much of a Helm chart archive is inflated while looking for `Chart.yaml` --------- Co-authored-by: silverwind Co-authored-by: wxiaoguang --- models/actions/run_job.go | 1 - modules/packages/helm/metadata.go | 4 +- routers/api/v1/api.go | 48 +++++++++-------- routers/api/v1/repo/issue_comment.go | 29 ++-------- routers/web/repo/issue_view.go | 37 ++----------- services/actions/max_parallel_test.go | 29 ++++++++-- services/actions/notifier_helper.go | 6 +++ services/actions/rerun.go | 3 ++ services/actions/reusable_workflow.go | 9 +++- services/actions/run.go | 8 +++ services/issue/comments.go | 53 +++++++++++++++++++ services/issue/comments_test.go | 34 ++++++++++++ services/migrations/onedev.go | 13 ++--- services/migrations/onedev_test.go | 7 --- .../integration/api_issue_dependency_test.go | 30 +++++++++++ tests/integration/api_issue_test.go | 21 ++++++++ 16 files changed, 229 insertions(+), 103 deletions(-) create mode 100644 services/issue/comments_test.go diff --git a/models/actions/run_job.go b/models/actions/run_job.go index 97aa10de97d..0eb32666760 100644 --- a/models/actions/run_job.go +++ b/models/actions/run_job.go @@ -25,7 +25,6 @@ import ( // MaxJobNumPerRun is the maximum number of jobs in a single run. // https://docs.github.com/en/actions/reference/limits#existing-system-limits -// TODO: check this limit when creating jobs const MaxJobNumPerRun = 256 // ActionRunJob represents a job of a run diff --git a/modules/packages/helm/metadata.go b/modules/packages/helm/metadata.go index 6ec1619a0df..eb218329d25 100644 --- a/modules/packages/helm/metadata.go +++ b/modules/packages/helm/metadata.go @@ -70,6 +70,8 @@ type Dependency struct { Alias string `json:"alias,omitempty" yaml:"alias,omitempty"` } +const maxArchiveSize = 256 * 1024 * 1024 // tar members are inflated while looking for Chart.yaml + // ParseChartArchive parses the metadata of a Helm archive func ParseChartArchive(r io.Reader) (*Metadata, error) { gzr, err := gzip.NewReader(r) @@ -78,7 +80,7 @@ func ParseChartArchive(r io.Reader) (*Metadata, error) { } defer gzr.Close() - tr := tar.NewReader(gzr) + tr := tar.NewReader(packages.NewLimitedReader(gzr, maxArchiveSize)) for { hd, err := tr.Next() if err == io.EOF { diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index b496584e1ee..0d5b5271b4b 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -814,6 +814,10 @@ func mustEnableRepoProjects(ctx *context.APIContext) { // FIXME: for consistency, maybe most mustNotBeArchived checks should be replaced with mustEnableEditor func mustNotBeArchived(ctx *context.APIContext) { + switch ctx.Req.Method { + case http.MethodGet, http.MethodHead, http.MethodOptions: + return + } if ctx.Repo.Repository.IsArchived { ctx.APIError(http.StatusLocked, "repo is archived") return @@ -1426,7 +1430,7 @@ func Routes() *web.Router { Get(repo.GetReleaseByTag). Delete(reqToken(), reqRepoWriter(unit.TypeReleases), repo.DeleteReleaseByTag) }) - }, mustEnableUnit(unit.TypeReleases)) + }, mustEnableUnit(unit.TypeReleases), mustNotBeArchived) m.Post("/mirror-sync", reqToken(), reqRepoWriter(unit.TypeCode), mustNotBeArchived, repo.MirrorSync) m.Post("/push_mirrors-sync", reqAdmin(), reqToken(), mustNotBeArchived, repo.PushMirrorSync) m.Group("/push_mirrors", func() { @@ -1573,14 +1577,14 @@ func Routes() *web.Router { m.Group("/{username}/{reponame}", func() { m.Group("/issues", func() { m.Combo("").Get(repo.ListIssues). - Post(reqToken(), mustNotBeArchived, bind(api.CreateIssueOption{}), reqRepoReader(unit.TypeIssues), repo.CreateIssue) + Post(reqToken(), bind(api.CreateIssueOption{}), reqRepoReader(unit.TypeIssues), repo.CreateIssue) m.Get("/pinned", reqRepoReader(unit.TypeIssues), repo.ListPinnedIssues) m.Group("/comments", func() { m.Get("", repo.ListRepoIssueComments) m.Group("/{id}", func() { m.Combo(""). Get(repo.GetIssueComment). - Patch(mustNotBeArchived, reqToken(), bind(api.EditIssueCommentOption{}), repo.EditIssueComment). + Patch(reqToken(), bind(api.EditIssueCommentOption{}), repo.EditIssueComment). Delete(reqToken(), repo.DeleteIssueComment) m.Combo("/reactions"). Get(repo.GetIssueCommentReactions). @@ -1589,35 +1593,35 @@ func Routes() *web.Router { m.Group("/assets", func() { m.Combo(""). Get(repo.ListIssueCommentAttachments). - Post(reqToken(), mustNotBeArchived, repo.CreateIssueCommentAttachment) + Post(reqToken(), repo.CreateIssueCommentAttachment) m.Combo("/{attachment_id}"). Get(repo.GetIssueCommentAttachment). - Patch(reqToken(), mustNotBeArchived, bind(api.EditAttachmentOptions{}), repo.EditIssueCommentAttachment). - Delete(reqToken(), mustNotBeArchived, repo.DeleteIssueCommentAttachment) + Patch(reqToken(), bind(api.EditAttachmentOptions{}), repo.EditIssueCommentAttachment). + Delete(reqToken(), repo.DeleteIssueCommentAttachment) }, mustEnableAttachments) }) }) m.Group("/{index}", func() { m.Combo("").Get(repo.GetIssue). - Patch(reqToken(), mustNotBeArchived, bind(api.EditIssueOption{}), repo.EditIssue). - Delete(reqToken(), reqAdmin(), mustNotBeArchived, context.ReferencesGitRepo(), repo.DeleteIssue) + Patch(reqToken(), bind(api.EditIssueOption{}), repo.EditIssue). + Delete(reqToken(), reqAdmin(), context.ReferencesGitRepo(), repo.DeleteIssue) m.Combo("/assignees"). - Post(reqToken(), mustNotBeArchived, bind(api.IssueAssigneesOption{}), repo.AddIssueAssignees). - Delete(reqToken(), mustNotBeArchived, bind(api.IssueAssigneesOption{}), repo.DeleteIssueAssignees) + Post(reqToken(), bind(api.IssueAssigneesOption{}), repo.AddIssueAssignees). + Delete(reqToken(), bind(api.IssueAssigneesOption{}), repo.DeleteIssueAssignees) m.Get("/assignees/{assignee}", repo.CheckIssueAssignee) m.Group("/comments", func() { m.Combo("").Get(repo.ListIssueComments). - Post(reqToken(), mustNotBeArchived, bind(api.CreateIssueCommentOption{}), repo.CreateIssueComment) + Post(reqToken(), bind(api.CreateIssueCommentOption{}), repo.CreateIssueComment) m.Combo("/{id}", reqToken()).Patch(bind(api.EditIssueCommentOption{}), repo.EditIssueCommentDeprecated). Delete(repo.DeleteIssueCommentDeprecated) }) m.Get("/timeline", repo.ListIssueCommentsAndTimeline) m.Group("/labels", func() { m.Combo("").Get(repo.ListIssueLabels). - Post(reqToken(), mustNotBeArchived, bind(api.IssueLabelsOption{}), repo.AddIssueLabels). - Put(reqToken(), mustNotBeArchived, bind(api.IssueLabelsOption{}), repo.ReplaceIssueLabels). - Delete(reqToken(), mustNotBeArchived, repo.ClearIssueLabels) - m.Delete("/{id}", reqToken(), mustNotBeArchived, repo.DeleteIssueLabel) + Post(reqToken(), bind(api.IssueLabelsOption{}), repo.AddIssueLabels). + Put(reqToken(), bind(api.IssueLabelsOption{}), repo.ReplaceIssueLabels). + Delete(reqToken(), repo.ClearIssueLabels) + m.Delete("/{id}", reqToken(), repo.DeleteIssueLabel) }) m.Group("/times", func() { m.Combo(""). @@ -1645,16 +1649,16 @@ func Routes() *web.Router { m.Group("/assets", func() { m.Combo(""). Get(repo.ListIssueAttachments). - Post(reqToken(), mustNotBeArchived, repo.CreateIssueAttachment) + Post(reqToken(), repo.CreateIssueAttachment) m.Combo("/{attachment_id}"). Get(repo.GetIssueAttachment). - Patch(reqToken(), mustNotBeArchived, bind(api.EditAttachmentOptions{}), repo.EditIssueAttachment). - Delete(reqToken(), mustNotBeArchived, repo.DeleteIssueAttachment) + Patch(reqToken(), bind(api.EditAttachmentOptions{}), repo.EditIssueAttachment). + Delete(reqToken(), repo.DeleteIssueAttachment) }, mustEnableAttachments) m.Combo("/dependencies"). Get(repo.GetIssueDependencies). - Post(reqToken(), mustNotBeArchived, bind(api.IssueMeta{}), repo.CreateIssueDependency). - Delete(reqToken(), mustNotBeArchived, bind(api.IssueMeta{}), repo.RemoveIssueDependency) + Post(reqToken(), bind(api.IssueMeta{}), repo.CreateIssueDependency). + Delete(reqToken(), bind(api.IssueMeta{}), repo.RemoveIssueDependency) m.Combo("/blocks"). Get(repo.GetIssueBlocks). Post(reqToken(), bind(api.IssueMeta{}), repo.CreateIssueBlocking). @@ -1687,9 +1691,9 @@ func Routes() *web.Router { Delete(reqToken(), reqRepoWriter(unit.TypeIssues, unit.TypePullRequests), repo.DeleteMilestone) }) m.Group("/projects", func() { - addProjectRoutes(m, reqToken(), reqRepoWriter(unit.TypeProjects), mustNotBeArchived) + addProjectRoutes(m, reqToken(), reqRepoWriter(unit.TypeProjects)) }, reqRepoReader(unit.TypeProjects), mustEnableRepoProjects) - }, repoAssignment()) + }, repoAssignment(), mustNotBeArchived) }, tokenRequiresScopes(auth_model.AccessTokenScopeCategoryIssue)) // NOTE: these are Gitea package management API - see packages.CommonRoutes and packages.DockerContainerRoutes for endpoints that implement package manager APIs diff --git a/routers/api/v1/repo/issue_comment.go b/routers/api/v1/repo/issue_comment.go index 974d8872398..eb98ee43f56 100644 --- a/routers/api/v1/repo/issue_comment.go +++ b/routers/api/v1/repo/issue_comment.go @@ -5,14 +5,11 @@ package repo import ( - stdCtx "context" "errors" "net/http" issues_model "gitea.dev/models/issues" "gitea.dev/models/perm" - access_model "gitea.dev/models/perm/access" - repo_model "gitea.dev/models/repo" "gitea.dev/models/unit" user_model "gitea.dev/models/user" "gitea.dev/modules/optional" @@ -189,10 +186,14 @@ func ListIssueCommentsAndTimeline(ctx *context.APIContext) { ctx.APIErrorInternal(err) return } + if comments, err = issue_service.FilterReadableComments(ctx, ctx.Doer, ctx.Repo.Permission, issue, comments); err != nil { + ctx.APIErrorInternal(err) + return + } var apiComments []*api.TimelineComment for _, comment := range comments { - if comment.Type != issues_model.CommentTypeCode && isXRefCommentAccessible(ctx, ctx.Doer, comment, issue.RepoID) { + if comment.Type != issues_model.CommentTypeCode { comment.Issue = issue apiComments = append(apiComments, convert.ToTimelineComment(ctx, issue.Repo, comment, ctx.Doer)) } @@ -202,26 +203,6 @@ func ListIssueCommentsAndTimeline(ctx *context.APIContext) { ctx.JSON(http.StatusOK, &apiComments) } -func isXRefCommentAccessible(ctx stdCtx.Context, user *user_model.User, c *issues_model.Comment, issueRepoID int64) bool { - // Remove comments that the user has no permissions to see - if issues_model.CommentTypeIsRef(c.Type) && c.RefRepoID != issueRepoID && c.RefRepoID != 0 { - var err error - // Set RefRepo for description in template - c.RefRepo, err = repo_model.GetRepositoryByID(ctx, c.RefRepoID) - if err != nil { - return false - } - perm, err := access_model.GetDoerRepoPermission(ctx, c.RefRepo, user) - if err != nil { - return false - } - if !perm.CanReadIssuesOrPulls(c.RefIsPull) { - return false - } - } - return true -} - // ListRepoIssueComments returns all issue-comments for a repo func ListRepoIssueComments(ctx *context.APIContext) { // swagger:operation GET /repos/{owner}/{repo}/issues/comments issue issueGetRepoComments diff --git a/routers/web/repo/issue_view.go b/routers/web/repo/issue_view.go index 967f84645a9..75926782865 100644 --- a/routers/web/repo/issue_view.go +++ b/routers/web/repo/issue_view.go @@ -166,31 +166,6 @@ func addParticipant(poster *user_model.User, participants []*user_model.User) [] return append(participants, poster) } -func filterXRefComments(ctx *context.Context, issue *issues_model.Issue) error { - // Remove comments that the user has no permissions to see - for i := 0; i < len(issue.Comments); { - c := issue.Comments[i] - if issues_model.CommentTypeIsRef(c.Type) && c.RefRepoID != issue.RepoID && c.RefRepoID != 0 { - var err error - // Set RefRepo for description in template - c.RefRepo, err = repo_model.GetRepositoryByID(ctx, c.RefRepoID) - if err != nil { - return err - } - perm, err := access_model.GetDoerRepoPermission(ctx, c.RefRepo, ctx.Doer) - if err != nil { - return err - } - if !perm.CanReadIssuesOrPulls(c.RefIsPull) { - issue.Comments = append(issue.Comments[:i], issue.Comments[i+1:]...) - continue - } - } - i++ - } - return nil -} - // combineXRefComments keeps only the first reference from each issue, carrying over the action of later ones func combineXRefComments(issue *issues_model.Issue) { first := make(map[int64]*issues_model.Comment) @@ -376,8 +351,9 @@ func ViewIssue(ctx *context.Context) { return } - if err := filterXRefComments(ctx, issue); err != nil { - ctx.ServerError("filterXRefComments", err) + var err error + if issue.Comments, err = issue_service.FilterReadableComments(ctx, ctx.Doer, ctx.Repo.Permission, issue, issue.Comments); err != nil { + ctx.ServerError("FilterReadableComments", err) return } combineXRefComments(issue) @@ -760,13 +736,6 @@ func prepareIssueViewCommentsAndSidebarParticipants(ctx *context.Context, issue ctx.ServerError("LoadAssigneeUserAndTeam", err) return } - } else if comment.Type == issues_model.CommentTypeRemoveDependency || comment.Type == issues_model.CommentTypeAddDependency { - if err = comment.LoadDepIssueDetails(ctx); err != nil { - if !issues_model.IsErrIssueNotExist(err) { - ctx.ServerError("LoadDepIssueDetails", err) - return - } - } } else if comment.Type.HasContentSupport() { rctx := renderhelper.NewRenderContextRepoComment(ctx, issue.Repo, renderhelper.RepoCommentOptions{ FootnoteContextID: strconv.FormatInt(comment.ID, 10), diff --git a/services/actions/max_parallel_test.go b/services/actions/max_parallel_test.go index a39e7ce709e..23f5f390bbe 100644 --- a/services/actions/max_parallel_test.go +++ b/services/actions/max_parallel_test.go @@ -4,7 +4,9 @@ package actions import ( + "fmt" "slices" + "strings" "testing" actions_model "gitea.dev/models/actions" @@ -13,6 +15,7 @@ import ( "gitea.dev/models/unittest" user_model "gitea.dev/models/user" "gitea.dev/modules/test" + "gitea.dev/modules/util" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -53,9 +56,8 @@ jobs: - run: echo hi ` -func insertMaxParallelRun(t *testing.T, workflow string, needApproval bool) *actions_model.ActionRun { - t.Helper() - run := &actions_model.ActionRun{ +func newMaxParallelRun(needApproval bool) *actions_model.ActionRun { + return &actions_model.ActionRun{ Title: "max-parallel", RepoID: 4, OwnerID: 1, @@ -70,6 +72,11 @@ func insertMaxParallelRun(t *testing.T, workflow string, needApproval bool) *act WorkflowRepoID: 4, WorkflowCommitSHA: "c2d72f548424103f01ee1dc02889c1e2bff816b0", } +} + +func insertMaxParallelRun(t *testing.T, workflow string, needApproval bool) *actions_model.ActionRun { + t.Helper() + run := newMaxParallelRun(needApproval) require.NoError(t, PrepareRunAndInsert(t.Context(), []byte(workflow), run, nil)) return run } @@ -304,3 +311,19 @@ func insertConcurrencyHolder(t *testing.T, index int64, group string) *actions_m require.NoError(t, db.Insert(ctx, holder)) return holder } + +func TestPrepareRunAndInsert_MaxJobNumPerRun(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + workflow := func(jobCount int) []byte { + var sb strings.Builder + sb.WriteString("on: push\njobs:\n") + for i := range jobCount { + fmt.Fprintf(&sb, " job%d:\n runs-on: ubuntu-latest\n steps:\n - run: echo\n", i) + } + return []byte(sb.String()) + } + require.NoError(t, PrepareRunAndInsert(t.Context(), workflow(actions_model.MaxJobNumPerRun), newMaxParallelRun(false), nil)) + err := PrepareRunAndInsert(t.Context(), workflow(actions_model.MaxJobNumPerRun+1), newMaxParallelRun(false), nil) + assert.ErrorIs(t, err, util.ErrInvalidArgument) + assert.ErrorContains(t, err, "exceeds the maximum") +} diff --git a/services/actions/notifier_helper.go b/services/actions/notifier_helper.go index 2757ea49973..ea1ac5dbf79 100644 --- a/services/actions/notifier_helper.go +++ b/services/actions/notifier_helper.go @@ -5,6 +5,7 @@ package actions import ( "context" + "errors" "fmt" "slices" "strings" @@ -25,6 +26,7 @@ import ( "gitea.dev/modules/log" "gitea.dev/modules/setting" api "gitea.dev/modules/structs" + "gitea.dev/modules/util" webhook_module "gitea.dev/modules/webhook" "gitea.dev/services/convert" ) @@ -432,6 +434,10 @@ func buildApproveAndInsertRun( } if err := PrepareRunAndInsert(ctx, dwf.Content, run, nil); err != nil { + if errors.Is(err, util.ErrInvalidArgument) && !isScopedRun { + insertInvalidWorkflowRun(ctx, run, err) + return nil + } return fmt.Errorf("PrepareRunAndInsert: %w", err) } return nil diff --git a/services/actions/rerun.go b/services/actions/rerun.go index d5c53fe7510..d2cfec25d60 100644 --- a/services/actions/rerun.go +++ b/services/actions/rerun.go @@ -352,6 +352,9 @@ func execRerunPlan(ctx context.Context, plan *rerunPlan) (*actions_model.ActionR hasWaitingJobs = hasWaitingJobs || (newJob.Status == actions_model.StatusWaiting && !newJob.IsReusableCaller) newJobs = append(newJobs, newJob) } + if err := checkRunJobLimit(ctx, plan.run.ID, newAttempt.ID, 0); err != nil { // inline reusable expansions only counted the jobs inserted before them + return err + } // Refresh each ancestor's status from its now-fresh children. // `newJobs` is appended top-down (caller before its children), so we walk it in reverse to refresh the deepest ancestor first. diff --git a/services/actions/reusable_workflow.go b/services/actions/reusable_workflow.go index ef6dcea96ba..5a2d6c66145 100644 --- a/services/actions/reusable_workflow.go +++ b/services/actions/reusable_workflow.go @@ -37,7 +37,7 @@ import ( // MaxReusableCallLevels allows nine calls across ten workflows, including the top-level workflow. const MaxReusableCallLevels = 8 -// checkRunJobLimit rejects an expansion that would push the attempt over actions_model.MaxJobNumPerRun. +// checkRunJobLimit rejects adding jobs that would push the attempt over actions_model.MaxJobNumPerRun. // checkCallerChain bounds nesting *depth*, but a reusable graph also fans out in *breadth*: without a // cumulative cap a tiny set of files can drive exponential job-row insertion and exhaust the database. func checkRunJobLimit(ctx context.Context, runID, attemptID int64, adding int) error { @@ -46,11 +46,16 @@ func checkRunJobLimit(ctx context.Context, runID, attemptID int64, adding int) e return fmt.Errorf("count existing jobs of run %d attempt %d: %w", runID, attemptID, err) } if existing+int64(adding) > actions_model.MaxJobNumPerRun { - return fmt.Errorf("workflow run exceeds the maximum of %d jobs", actions_model.MaxJobNumPerRun) + return runJobLimitError() } return nil } +func runJobLimitError() error { + err := util.NewInvalidArgumentErrorf("workflow run exceeds the maximum of %d jobs", actions_model.MaxJobNumPerRun) + return util.ErrorWrapTranslatable(err, "actions.runs.invalid_workflow_helper", err.Error()) +} + type reusableWorkflowSource struct { content []byte repoID int64 diff --git a/services/actions/run.go b/services/actions/run.go index 701ccc9983d..8add589d8bc 100644 --- a/services/actions/run.go +++ b/services/actions/run.go @@ -63,6 +63,7 @@ func PrepareRunAndInsert(ctx context.Context, content []byte, run *actions_model func InsertRun(ctx context.Context, run *actions_model.ActionRun, content []byte, vars map[string]string, inputs map[string]any, wfRawConcurrency *act_model.RawConcurrency) error { var cancelledConcurrencyJobs []*actions_model.ActionRunJob var needPostCommitEmit bool + pending := *run if err := db.WithTx(ctx, func(ctx context.Context) error { index, err := db.GetNextResourceIndex(ctx, "action_run_index", run.RepoID) if err != nil { @@ -122,6 +123,9 @@ func InsertRun(ctx context.Context, run *actions_model.ActionRun, content []byte if err != nil { return fmt.Errorf("parse workflow: %w", err) } + if len(jobs) > actions_model.MaxJobNumPerRun { + return runJobLimitError() + } titleChanged := len(jobs) > 0 && jobs[0].RunName != "" if titleChanged { run.Title = util.EllipsisDisplayString(jobs[0].RunName, 255) @@ -151,6 +155,9 @@ func InsertRun(ctx context.Context, run *actions_model.ActionRun, content []byte hasWaitingJobs = hasWaitingJobs || (runJob.Status == actions_model.StatusWaiting && !runJob.IsReusableCaller) runJobs = append(runJobs, runJob) } + if err := checkRunJobLimit(ctx, run.ID, runAttempt.ID, 0); err != nil { // inline reusable expansions only counted the jobs inserted before them + return err + } runAttempt.Status = actions_model.AggregateJobStatus(runJobs) if err := actions_model.UpdateRunAttempt(ctx, runAttempt, "status"); err != nil { @@ -166,6 +173,7 @@ func InsertRun(ctx context.Context, run *actions_model.ActionRun, content []byte return nil }); err != nil { + *run = pending // drop the ID and index of the rolled back insert return err } diff --git a/services/issue/comments.go b/services/issue/comments.go index 649f4bfd180..325e50a67ae 100644 --- a/services/issue/comments.go +++ b/services/issue/comments.go @@ -13,6 +13,7 @@ import ( access_model "gitea.dev/models/perm/access" repo_model "gitea.dev/models/repo" user_model "gitea.dev/models/user" + "gitea.dev/modules/cache" "gitea.dev/modules/git" "gitea.dev/modules/json" "gitea.dev/modules/log" @@ -196,3 +197,55 @@ func LoadCommentPushCommits(ctx context.Context, c *issues_model.Comment) error return nil } + +// FilterReadableComments drops comments pointing at issues doer cannot read and loads RefRepo of the rest +func FilterReadableComments(ctx context.Context, doer *user_model.User, perm access_model.Permission, issue *issues_model.Issue, comments issues_model.CommentList) (issues_model.CommentList, error) { + type refTarget struct { + repo *repo_model.Repository + perm access_model.Permission + } + targets := cache.NewEphemeralCache() + targets.Put("ref-target", issue.RepoID, refTarget{issue.Repo, perm}) + getRepoPerm := func(refRepoID int64) (refTarget, error) { + return cache.GetWithEphemeralCache(ctx, targets, "ref-target", refRepoID, func(ctx context.Context, repoID int64) (refTarget, error) { + repo, err := repo_model.GetRepositoryByID(ctx, repoID) + if err != nil { + return refTarget{}, err + } + repoPerm, err := access_model.GetDoerRepoPermission(ctx, repo, doer) + return refTarget{repo, repoPerm}, err + }) + } + + readable := comments[:0] + for _, c := range comments { + var refRepoID int64 + var refIsPull bool + switch { + case issues_model.CommentTypeIsRef(c.Type): + refRepoID, refIsPull = c.RefRepoID, c.RefIsPull + case c.Type == issues_model.CommentTypeAddDependency || c.Type == issues_model.CommentTypeRemoveDependency: + if err := c.LoadDepIssueDetails(ctx); err != nil && !issues_model.IsErrIssueNotExist(err) { + return nil, err + } + if c.DependentIssue != nil { + refRepoID, refIsPull = c.DependentIssue.RepoID, c.DependentIssue.IsPull + } + } + if refRepoID != 0 { + target, err := getRepoPerm(refRepoID) + if err != nil { + return nil, err + } + if !target.perm.CanReadIssuesOrPulls(refIsPull) { + continue + } + c.RefRepo = target.repo + if c.DependentIssue != nil { + c.DependentIssue.Repo = target.repo + } + } + readable = append(readable, c) + } + return readable, nil +} diff --git a/services/issue/comments_test.go b/services/issue/comments_test.go new file mode 100644 index 00000000000..cadf3f92d57 --- /dev/null +++ b/services/issue/comments_test.go @@ -0,0 +1,34 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package issue + +import ( + "testing" + + issues_model "gitea.dev/models/issues" + perm_model "gitea.dev/models/perm" + access_model "gitea.dev/models/perm/access" + repo_model "gitea.dev/models/repo" + "gitea.dev/models/unit" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFilterReadableCommentsChecksUnitOfSameRepoTargets(t *testing.T) { + var perm access_model.Permission + perm.SetUnitsWithDefaultAccessMode([]*repo_model.RepoUnit{{Type: unit.TypeIssues}}, perm_model.AccessModeRead) + issueRef := &issues_model.Comment{Type: issues_model.CommentTypeIssueRef, RefRepoID: 1} + pullRef := &issues_model.Comment{Type: issues_model.CommentTypePullRef, RefRepoID: 1, RefIsPull: true} + issueDependency := &issues_model.Comment{Type: issues_model.CommentTypeAddDependency, DependentIssue: &issues_model.Issue{RepoID: 1}} + pullDependency := &issues_model.Comment{Type: issues_model.CommentTypeAddDependency, DependentIssue: &issues_model.Issue{RepoID: 1, IsPull: true}} + repo := &repo_model.Repository{ID: 1} + + comments, err := FilterReadableComments(t.Context(), nil, perm, &issues_model.Issue{RepoID: 1, Repo: repo}, issues_model.CommentList{issueRef, pullRef, issueDependency, pullDependency}) + require.NoError(t, err) + assert.Equal(t, issues_model.CommentList{issueRef, issueDependency}, comments) + assert.Same(t, repo, issueRef.RefRepo) + assert.Same(t, repo, issueDependency.RefRepo) + assert.Same(t, repo, issueDependency.DependentIssue.Repo) +} diff --git a/services/migrations/onedev.go b/services/migrations/onedev.go index 2aedcefa05e..6298c47f239 100644 --- a/services/migrations/onedev.go +++ b/services/migrations/onedev.go @@ -22,10 +22,7 @@ import ( "github.com/hashicorp/go-version" ) -const ( - OneDevRequiredVersion = "12.0.1" - maxOneDevVersionResponseSize = 1024 -) +const OneDevRequiredVersion = "12.0.1" var ( _ base.Downloader = &OneDevDownloader{} @@ -148,14 +145,12 @@ func (d *OneDevDownloader) getServerVersionByEndpoint(ctx context.Context, endpo if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("OneDev server version request failed: %s", resp.Status) } - // This patch doesn't change the situation, "decoder.Decode(&result)" in callAPI still reads the entire body - bytes, err := io.ReadAll(io.LimitReader(resp.Body, maxOneDevVersionResponseSize+1)) + // This patch doesn't change the situation, "decoder.Decode(&result)" in callAPI still reads the entire body. + // Also, the methods like GetMilestones can read plenty of responses into memory even if each single is small. + bytes, err := io.ReadAll(io.LimitReader(resp.Body, 1024)) if err != nil { return nil, err } - if len(bytes) > maxOneDevVersionResponseSize { - return nil, fmt.Errorf("OneDev server version response exceeds %d bytes", maxOneDevVersionResponseSize) - } ver, err := version.NewVersion(string(bytes)) if err != nil { return nil, fmt.Errorf("failed to parse OneDev server version %q: %w", string(bytes), err) diff --git a/services/migrations/onedev_test.go b/services/migrations/onedev_test.go index 581dfe37cd3..32d566b9560 100644 --- a/services/migrations/onedev_test.go +++ b/services/migrations/onedev_test.go @@ -11,7 +11,6 @@ import ( "os" "path/filepath" "runtime" - "strings" "sync/atomic" "testing" "time" @@ -40,12 +39,6 @@ func TestOneDevServerVersion(t *testing.T) { assert.NoError(t, err) downloader := func() *OneDevDownloader { return &OneDevDownloader{baseURL: baseURL, client: server.Client()} } - t.Run("LargeResponse", func(t *testing.T) { - respMap.Store(&respMapType{"/~api/version/server": {code: http.StatusOK, body: strings.Repeat("x", maxOneDevVersionResponseSize+1)}}) - _, err := downloader().getServerVersion(t.Context()) - assert.ErrorContains(t, err, "server version response exceeds") - }) - t.Run("LegacyEndpoint", func(t *testing.T) { respMap.Store(&respMapType{"/~api/version/server": {code: http.StatusOK, body: "12.0.1"}}) d := downloader() diff --git a/tests/integration/api_issue_dependency_test.go b/tests/integration/api_issue_dependency_test.go index 64b9bf5e490..aa4d5babdcf 100644 --- a/tests/integration/api_issue_dependency_test.go +++ b/tests/integration/api_issue_dependency_test.go @@ -6,6 +6,7 @@ package integration import ( "fmt" "net/http" + "slices" "strconv" "testing" @@ -139,3 +140,32 @@ func TestAPICreateIssueDependencyCrossRepoPermission(t *testing.T) { unittest.AssertNotExistsBean(t, &issues_model.IssueDependency{IssueID: targetIssue.ID, DependencyID: dependencyIssue.ID}) }) } + +func TestIssueTimelineDependencyCrossRepoPermission(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + targetIssue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{RepoID: 1, Index: 1}) + dependencyRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) + assert.True(t, dependencyRepo.IsPrivate) + dependencyIssue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{RepoID: dependencyRepo.ID, Index: 1}) + enableRepoDependencies(t, targetIssue.RepoID) + enableRepoDependencies(t, dependencyRepo.ID) + user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + assert.NoError(t, issues_model.CreateIssueDependency(t.Context(), user1, targetIssue, dependencyIssue)) + + session := loginUser(t, "user40") + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadIssue) + assertDependencyVisible := func(visible bool) { + t.Helper() + req := NewRequest(t, "GET", "/api/v1/repos/user2/repo1/issues/1/timeline").AddTokenAuth(token) + comments := DecodeJSON(t, MakeRequest(t, req, http.StatusOK), []*api.TimelineComment{}) + assert.Equal(t, visible, slices.ContainsFunc(comments, func(c *api.TimelineComment) bool { return c.Type == "add_dependency" })) + resp := session.MakeRequest(t, NewRequest(t, "GET", "/user2/repo1/issues/1"), http.StatusOK) + assert.Equal(t, visible, NewHTMLParser(t, resp.Body).Find(`.timeline-item a[href$="/org3/repo3/issues/1"]`).Length() > 0) + } + + assertDependencyVisible(false) + user40 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 40}) + assert.NoError(t, repo_service.AddOrUpdateCollaborator(t.Context(), dependencyRepo, user40, perm.AccessModeRead)) + assertDependencyVisible(true) +} diff --git a/tests/integration/api_issue_test.go b/tests/integration/api_issue_test.go index c6ffef19d4e..d10cd485da3 100644 --- a/tests/integration/api_issue_test.go +++ b/tests/integration/api_issue_test.go @@ -8,6 +8,7 @@ import ( "net/http" "net/url" "strconv" + "strings" "sync" "testing" "time" @@ -705,3 +706,23 @@ func testAPIIssueProjects(t *testing.T) { req = NewRequestWithJSON(t, "POST", urlStr, &api.CreateIssueOption{Title: "issue by a reader", Projects: []int64{1}}).AddTokenAuth(readerToken) assert.Empty(t, DecodeJSON(t, MakeRequest(t, req, http.StatusCreated), &api.Issue{}).Projects) } + +func TestAPIArchivedRepoMutations(t *testing.T) { + defer tests.PrepareTestEnv(t)() + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + assert.NoError(t, repo_model.SetArchiveRepoState(t.Context(), repo, true)) + token := getUserToken(t, "user2", auth_model.AccessTokenScopeWriteIssue, auth_model.AccessTokenScopeWriteRepository) + for route, status := range map[string]int{ + "GET /issues/1/reactions": http.StatusOK, + "POST /issues/1/reactions": http.StatusLocked, + "DELETE /issues/comments/1": http.StatusLocked, + "DELETE /labels/1": http.StatusLocked, + "GET /releases/1": http.StatusOK, + "DELETE /releases/1": http.StatusLocked, + } { + t.Run(route, func(t *testing.T) { + method, path, _ := strings.Cut(route, " ") + MakeRequest(t, NewRequest(t, method, "/api/v1/repos/user2/repo1"+path).AddTokenAuth(token), status) + }) + } +}