From 884e568ea267960ad25fc8242f7a75d13fca6618 Mon Sep 17 00:00:00 2001 From: Giteabot Date: Sat, 6 Jun 2026 13:07:06 -0700 Subject: [PATCH 01/17] fix(lfs): reject unknown SSH LFS sub-verbs to prevent auth bypass (#38008) (#38015) --- cmd/serv.go | 23 ++++++++++++-------- cmd/serv_test.go | 56 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 9 deletions(-) create mode 100644 cmd/serv_test.go diff --git a/cmd/serv.go b/cmd/serv.go index 5de50fa7e4..99b1d913c9 100644 --- a/cmd/serv.go +++ b/cmd/serv.go @@ -113,23 +113,25 @@ func handleCliResponseExtra(extra private.ResponseExtra) error { return nil } -func getAccessMode(verb, lfsVerb string) perm.AccessMode { +// getAccessMode maps an SSH git/LFS verb to the access mode it requires, with +// ok=false for an unrecognised verb. Callers MUST reject the request when ok is +// false: AccessModeNone would otherwise pass the `userMode < mode` permission +// check in routers/private/serv.go and grant access. +func getAccessMode(verb, lfsVerb string) (mode perm.AccessMode, ok bool) { switch verb { case git.CmdVerbUploadPack, git.CmdVerbUploadArchive: - return perm.AccessModeRead + return perm.AccessModeRead, true case git.CmdVerbReceivePack: - return perm.AccessModeWrite + return perm.AccessModeWrite, true case git.CmdVerbLfsAuthenticate, git.CmdVerbLfsTransfer: switch lfsVerb { case git.CmdSubVerbLfsUpload: - return perm.AccessModeWrite + return perm.AccessModeWrite, true case git.CmdSubVerbLfsDownload: - return perm.AccessModeRead + return perm.AccessModeRead, true } } - // should be unreachable - setting.PanicInDevOrTesting("unknown verb: %s %s", verb, lfsVerb) - return perm.AccessModeNone + return perm.AccessModeNone, false } func runServ(ctx context.Context, c *cli.Command) error { @@ -247,7 +249,10 @@ func runServ(ctx context.Context, c *cli.Command) error { } } - requestedMode := getAccessMode(verb, lfsVerb) + requestedMode, ok := getAccessMode(verb, lfsVerb) + if !ok { + return fail(ctx, "Unknown git command", "Unknown git command %s %s", verb, lfsVerb) + } results, extra := private.ServCommand(ctx, keyID, username, reponame, requestedMode, verb, lfsVerb) if extra.HasError() { diff --git a/cmd/serv_test.go b/cmd/serv_test.go new file mode 100644 index 0000000000..223b19ce4e --- /dev/null +++ b/cmd/serv_test.go @@ -0,0 +1,56 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package cmd + +import ( + "testing" + + "code.gitea.io/gitea/models/perm" + "code.gitea.io/gitea/modules/git" + + "github.com/stretchr/testify/assert" +) + +func TestGetAccessMode(t *testing.T) { + cases := []struct { + verb, lfsVerb string + expected perm.AccessMode + }{ + {git.CmdVerbUploadPack, "", perm.AccessModeRead}, + {git.CmdVerbUploadArchive, "", perm.AccessModeRead}, + {git.CmdVerbReceivePack, "", perm.AccessModeWrite}, + {git.CmdVerbLfsAuthenticate, git.CmdSubVerbLfsUpload, perm.AccessModeWrite}, + {git.CmdVerbLfsAuthenticate, git.CmdSubVerbLfsDownload, perm.AccessModeRead}, + {git.CmdVerbLfsTransfer, git.CmdSubVerbLfsUpload, perm.AccessModeWrite}, + {git.CmdVerbLfsTransfer, git.CmdSubVerbLfsDownload, perm.AccessModeRead}, + } + for _, tc := range cases { + t.Run(tc.verb+"/"+tc.lfsVerb, func(t *testing.T) { + mode, ok := getAccessMode(tc.verb, tc.lfsVerb) + assert.True(t, ok) + assert.Equal(t, tc.expected, mode) + }) + } +} + +// TestGetAccessModeUnknownVerb locks in the invariant that getAccessMode reports +// ok=false for unrecognised verbs and LFS sub-verbs, so runServ rejects them. An +// unknown verb has no valid access mode; if it were treated as AccessModeNone (0) +// it would pass the `userMode < mode` permission check in routers/private/serv.go +// and hand out valid LFS JWTs for any private repository. +func TestGetAccessModeUnknownVerb(t *testing.T) { + cases := []struct{ verb, lfsVerb string }{ + {git.CmdVerbLfsAuthenticate, ""}, + {git.CmdVerbLfsAuthenticate, "badverb"}, + {git.CmdVerbLfsTransfer, "badverb"}, + {"git-unknown-verb", ""}, + } + for _, tc := range cases { + t.Run(tc.verb+"/"+tc.lfsVerb, func(t *testing.T) { + mode, ok := getAccessMode(tc.verb, tc.lfsVerb) + assert.False(t, ok) + assert.Equal(t, perm.AccessModeNone, mode) + }) + } +} -- 2.52.0 From 725acbe1122dd5a737a1a0d731754af420a2f761 Mon Sep 17 00:00:00 2001 From: Giteabot Date: Sun, 7 Jun 2026 09:40:34 -0700 Subject: [PATCH 02/17] fix: bound CODEOWNERS regex match time (#38011) (#38025) Backport #38011 by @bircni User-supplied CODEOWNERS patterns were compiled without a match timeout, so a crafted pattern (e.g. (a+)+) against a crafted file path could backtrack for tens of seconds inside the PR creation transaction and exhaust the database connection pool. Set MatchTimeout on each compiled rule; the caller already treats match errors as non-matches. Signed-off-by: wxiaoguang Co-authored-by: bircni Co-authored-by: wxiaoguang --- models/issues/pull.go | 8 ++++++++ models/issues/pull_test.go | 19 +++++++++++++++++++ services/issue/pull.go | 14 ++++++++++++++ 3 files changed, 41 insertions(+) diff --git a/models/issues/pull.go b/models/issues/pull.go index 81f94ce967..3c09b02cff 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -10,6 +10,7 @@ import ( "fmt" "io" "strings" + "time" "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/db" git_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/git" @@ -860,6 +861,11 @@ func GetCodeOwnersFromContent(ctx context.Context, data string) ([]*CodeOwnerRul return rules, warnings } +// codeOwnerMatchTimeout bounds a single pattern match so a crafted pattern +// cannot stall via catastrophic backtracking. See also the aggregate budget +// enforced by the caller across the whole rules×files match loop. +const codeOwnerMatchTimeout = 150 * time.Millisecond + type CodeOwnerRule struct { Rule *regexp2.Regexp // it supports negative lookahead, does better for end users Negative bool @@ -888,6 +894,8 @@ func ParseCodeOwnersLine(ctx context.Context, tokens []string) (*CodeOwnerRule, warnings = append(warnings, fmt.Sprintf("incorrect codeowner regexp: %s", err)) return nil, warnings } + // Bound matching time so user-supplied patterns cannot stall PR creation via catastrophic backtracking. + rule.Rule.MatchTimeout = codeOwnerMatchTimeout for _, user := range tokens[1:] { user = strings.TrimPrefix(user, "@") diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 6fac2bbe1a..d353d25512 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -4,7 +4,9 @@ package issues_test import ( + "strings" "testing" + "time" "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/db" issues_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/issues" @@ -39,6 +41,7 @@ func TestPullRequest(t *testing.T) { t.Run("DeleteOrphanedObjects", testDeleteOrphanedObjects) t.Run("ParseCodeOwnersLine", testParseCodeOwnersLine) t.Run("CodeOwnerAbsolutePathPatterns", testCodeOwnerAbsolutePathPatterns) + t.Run("CodeOwnerPatternMatchTimeout", testCodeOwnerPatternMatchTimeout) t.Run("GetApprovers", testGetApprovers) t.Run("GetPullRequestByMergedCommit", testGetPullRequestByMergedCommit) t.Run("Migrate_InsertPullRequests", testMigrateInsertPullRequests) @@ -370,6 +373,22 @@ func testCodeOwnerAbsolutePathPatterns(t *testing.T) { } } +// testCodeOwnerPatternMatchTimeout ensures user-supplied CODEOWNERS patterns +// cannot stall pull request processing through catastrophic regex backtracking: +// each compiled rule must enforce a bounded match time. +func testCodeOwnerPatternMatchTimeout(t *testing.T) { + rules, _ := issues_model.GetCodeOwnersFromContent(t.Context(), "(a+)+ @user5\n") + require.Len(t, rules, 1) + + maliciousInput := strings.Repeat("a", 30) + "X" + start := time.Now() + _, err := rules[0].Rule.MatchString(maliciousInput) + elapsed := time.Since(start) + + require.Error(t, err, "expected MatchTimeout error on pathological input") + assert.Less(t, elapsed, time.Second, "match timeout did not bound regex evaluation; took %s", elapsed) +} + func testGetApprovers(t *testing.T) { pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 5}) // Official reviews are already deduplicated. Allow unofficial reviews diff --git a/services/issue/pull.go b/services/issue/pull.go index 8fffb59b9a..704d8ef62d 100644 --- a/services/issue/pull.go +++ b/services/issue/pull.go @@ -7,6 +7,7 @@ import ( "context" "fmt" "slices" + "time" issues_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/issues" org_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/organization" @@ -26,6 +27,10 @@ type ReviewRequestNotifier struct { var codeOwnerFiles = []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"} +// codeOwnerMatchBudget caps the total wall-clock time spent evaluating all +// CODEOWNERS rules against all changed files for a single PR. +const codeOwnerMatchBudget = 2 * time.Second + func IsCodeOwnerFile(f string) bool { return slices.Contains(codeOwnerFiles, f) } @@ -93,8 +98,17 @@ func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullReque uniqUsers := make(map[int64]*user_model.User) uniqTeams := make(map[string]*org_model.Team) + // Bound the total time spent matching rules×files. The per-rule MatchTimeout + // only caps a single match; without an aggregate budget a crafted CODEOWNERS + // plus a PR touching many files could still exhaust CPU inside this loop. + matchDeadline := time.Now().Add(codeOwnerMatchBudget) +ruleLoop: for _, rule := range rules { for _, f := range changedFiles { + if time.Now().After(matchDeadline) { + log.Warn("CODEOWNERS matching for PR %s#%d exceeded its time budget; some rules were not evaluated", pr.BaseRepo.FullName(), pr.ID) + break ruleLoop + } shouldMatch := !rule.Negative matched, _ := rule.Rule.MatchString(f) // err only happens when timeouts, any error can be considered as not matched if matched == shouldMatch { -- 2.52.0 From e9648f367e27be298050deb0bc68c3b07eb74bf4 Mon Sep 17 00:00:00 2001 From: Giteabot Date: Tue, 9 Jun 2026 06:53:42 -0700 Subject: [PATCH 03/17] fix(actions)!: require merged PR to bypass fork PR approval gate (#38010) (#38041) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Backport #38010 by @bircni `ifNeedApproval` in `services/actions/notifier_helper.go` decided whether a fork PR's workflow run had to wait for maintainer approval. The bypass clause counted any prior `approved_by > 0` run for `(repo_id, trigger_user_id)`, so the very first Approve-and-run click on a contributor's fork PR permanently trusted that user for every future fork PR in the same repository — including PRs whose only change is the workflow YAML itself. Approving a workflow *run* is not the same as merging *code*. This change aligns the gate with GitHub Actions' first-time-contributor model: trust is granted only after the user has had a pull request merged in the repo. ## Behavior change - **Before**: one approval = permanent trust for that user in that repo. - **After**: every fork PR is gated until the contributor has at least one merged PR in the repo. Existing already-approved runs and merged PRs continue to work; only the trust criterion for *future* fork PRs changes. Maintainers who rely on the implicit "approve once" trust will see the approval banner reappear until they merge a PR from that contributor. --------- Signed-off-by: bircni Co-authored-by: bircni --- models/actions/run_list.go | 4 - services/actions/notifier_helper.go | 39 +++++-- services/actions/notifier_helper_test.go | 102 +++++++++++++++++ tests/integration/actions_approve_test.go | 104 ++++++++++++++++++ tests/integration/actions_concurrency_test.go | 8 +- 5 files changed, 239 insertions(+), 18 deletions(-) create mode 100644 services/actions/notifier_helper_test.go diff --git a/models/actions/run_list.go b/models/actions/run_list.go index 42a063bb68..5955f64572 100644 --- a/models/actions/run_list.go +++ b/models/actions/run_list.go @@ -64,7 +64,6 @@ type FindRunOptions struct { Ref string // the commit/tag/… that caused this workflow TriggerUserID int64 TriggerEvent webhook_module.HookEventType - Approved bool // not util.OptionalBool, it works only when it's true Status []Status ConcurrencyGroup string CommitSHA string @@ -81,9 +80,6 @@ func (opts FindRunOptions) ToConds() builder.Cond { if opts.TriggerUserID > 0 { cond = cond.And(builder.Eq{"`action_run`.trigger_user_id": opts.TriggerUserID}) } - if opts.Approved { - cond = cond.And(builder.Gt{"`action_run`.approved_by": 0}) - } if len(opts.Status) > 0 { cond = cond.And(builder.In("`action_run`.status", opts.Status)) } diff --git a/services/actions/notifier_helper.go b/services/actions/notifier_helper.go index 6d515b08b4..5b0a0f721f 100644 --- a/services/actions/notifier_helper.go +++ b/services/actions/notifier_helper.go @@ -448,6 +448,24 @@ func notifyPackage(ctx context.Context, sender *user_model.User, pd *packages_mo } func ifNeedApproval(ctx context.Context, run *actions_model.ActionRun, repo *repo_model.Repository, user *user_model.User) (bool, error) { + canWrite := func(ctx context.Context, repo *repo_model.Repository, user *user_model.User) (bool, error) { + perm, err := access_model.GetDoerRepoPermission(ctx, repo, user) + if err != nil { + return false, err + } + return perm.CanWrite(unit_model.TypeActions), nil + } + return ifNeedApprovalWith(ctx, run, repo, user, canWrite, issues_model.HasMergedPullRequestInRepo) +} + +func ifNeedApprovalWith( + ctx context.Context, + run *actions_model.ActionRun, + repo *repo_model.Repository, + user *user_model.User, + canWriteActions func(context.Context, *repo_model.Repository, *user_model.User) (bool, error), + hasMergedPR func(context.Context, int64, int64) (bool, error), +) (bool, error) { // 1. don't need approval if it's not a fork PR // 2. don't need approval if the event is `pull_request_target` since the workflow will run in the context of base branch // see https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks#about-workflow-runs-from-public-forks @@ -462,27 +480,24 @@ func ifNeedApproval(ctx context.Context, run *actions_model.ActionRun, repo *rep } // don't need approval if the user can write - if perm, err := access_model.GetDoerRepoPermission(ctx, repo, user); err != nil { + if ok, err := canWriteActions(ctx, repo, user); err != nil { return false, fmt.Errorf("GetDoerRepoPermission: %w", err) - } else if perm.CanWrite(unit_model.TypeActions) { + } else if ok { log.Trace("do not need approval because user %d can write", user.ID) return false, nil } - // don't need approval if the user has been approved before - if count, err := db.Count[actions_model.ActionRun](ctx, actions_model.FindRunOptions{ - RepoID: repo.ID, - TriggerUserID: user.ID, - Approved: true, - }); err != nil { - return false, fmt.Errorf("CountRuns: %w", err) - } else if count > 0 { - log.Trace("do not need approval because user %d has been approved before", user.ID) + // trust the user only after a merged PR — matching GitHub Actions. Approving one + // fork PR's run must not implicitly trust later fork PRs that replace the workflow. + if merged, err := hasMergedPR(ctx, repo.ID, user.ID); err != nil { + return false, fmt.Errorf("HasMergedPullRequestInRepo: %w", err) + } else if merged { + log.Trace("do not need approval because user %d has a merged pull request in repo %d", user.ID, repo.ID) return false, nil } // otherwise, need approval - log.Trace("need approval because it's the first time user %d triggered actions", user.ID) + log.Trace("need approval because user %d has no merged pull request in repo %d", user.ID, repo.ID) return true, nil } diff --git a/services/actions/notifier_helper_test.go b/services/actions/notifier_helper_test.go new file mode 100644 index 0000000000..bb9fcc0cdb --- /dev/null +++ b/services/actions/notifier_helper_test.go @@ -0,0 +1,102 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package actions + +import ( + "context" + "errors" + "testing" + + actions_model "code.gitea.io/gitea/models/actions" + repo_model "code.gitea.io/gitea/models/repo" + user_model "code.gitea.io/gitea/models/user" + actions_module "code.gitea.io/gitea/modules/actions" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestIfNeedApproval(t *testing.T) { + alwaysWrite := func(_ context.Context, _ *repo_model.Repository, _ *user_model.User) (bool, error) { + return true, nil + } + neverWrite := func(_ context.Context, _ *repo_model.Repository, _ *user_model.User) (bool, error) { + return false, nil + } + hasMerged := func(_ context.Context, _, _ int64) (bool, error) { return true, nil } + noMerged := func(_ context.Context, _, _ int64) (bool, error) { return false, nil } + errPerm := errors.New("perm error") + errMerge := errors.New("merge error") + + forkRun := &actions_model.ActionRun{IsForkPullRequest: true, TriggerEvent: actions_module.GithubEventPullRequest} + nonForkRun := &actions_model.ActionRun{IsForkPullRequest: false, TriggerEvent: actions_module.GithubEventPullRequest} + prTargetRun := &actions_model.ActionRun{IsForkPullRequest: true, TriggerEvent: actions_module.GithubEventPullRequestTarget} + + repo := &repo_model.Repository{ID: 1} + normalUser := &user_model.User{ID: 10} + restrictedUser := &user_model.User{ID: 11, IsRestricted: true} + + t.Run("not a fork PR never needs approval", func(t *testing.T) { + need, err := ifNeedApprovalWith(t.Context(), nonForkRun, repo, normalUser, alwaysWrite, hasMerged) + require.NoError(t, err) + assert.False(t, need) + }) + + t.Run("pull_request_target never needs approval even when fork", func(t *testing.T) { + need, err := ifNeedApprovalWith(t.Context(), prTargetRun, repo, normalUser, alwaysWrite, hasMerged) + require.NoError(t, err) + assert.False(t, need) + }) + + t.Run("restricted user always needs approval", func(t *testing.T) { + need, err := ifNeedApprovalWith(t.Context(), forkRun, repo, restrictedUser, alwaysWrite, hasMerged) + require.NoError(t, err) + assert.True(t, need) + }) + + t.Run("fork PR with write permission does not need approval", func(t *testing.T) { + need, err := ifNeedApprovalWith(t.Context(), forkRun, repo, normalUser, alwaysWrite, noMerged) + require.NoError(t, err) + assert.False(t, need) + }) + + t.Run("fork PR with merged PR but no write permission does not need approval", func(t *testing.T) { + need, err := ifNeedApprovalWith(t.Context(), forkRun, repo, normalUser, neverWrite, hasMerged) + require.NoError(t, err) + assert.False(t, need) + }) + + t.Run("fork PR with no write and no merged PR needs approval", func(t *testing.T) { + need, err := ifNeedApprovalWith(t.Context(), forkRun, repo, normalUser, neverWrite, noMerged) + require.NoError(t, err) + assert.True(t, need) + }) + + t.Run("canWriteActions error is propagated", func(t *testing.T) { + failWrite := func(_ context.Context, _ *repo_model.Repository, _ *user_model.User) (bool, error) { + return false, errPerm + } + _, err := ifNeedApprovalWith(t.Context(), forkRun, repo, normalUser, failWrite, noMerged) + require.ErrorIs(t, err, errPerm) + }) + + t.Run("hasMergedPR error is propagated", func(t *testing.T) { + failMerge := func(_ context.Context, _, _ int64) (bool, error) { return false, errMerge } + _, err := ifNeedApprovalWith(t.Context(), forkRun, repo, normalUser, neverWrite, failMerge) + require.ErrorIs(t, err, errMerge) + }) + + t.Run("restricted user skips permission check entirely", func(t *testing.T) { + // The perm and merge functions must not be called for a restricted user. + called := false + trackWrite := func(_ context.Context, _ *repo_model.Repository, _ *user_model.User) (bool, error) { + called = true + return true, nil + } + need, err := ifNeedApprovalWith(t.Context(), forkRun, repo, restrictedUser, trackWrite, noMerged) + require.NoError(t, err) + assert.True(t, need) + assert.False(t, called, "permission check must not run for restricted user") + }) +} diff --git a/tests/integration/actions_approve_test.go b/tests/integration/actions_approve_test.go index b7b077fcba..62f8aadc4e 100644 --- a/tests/integration/actions_approve_test.go +++ b/tests/integration/actions_approve_test.go @@ -139,3 +139,107 @@ jobs: assert.Equal(t, actions_model.StatusWaiting, run2.Status) }) } + +// TestForkPullRequestApprovalNotBypassedByPriorApproval verifies that a single +// approval on a fork PR does not permanently trust the contributor: a subsequent +// fork PR from the same user must still be gated (Blocked / NeedApproval=true) +// until that user has had a pull request merged in the repo. +func TestForkPullRequestApprovalNotBypassedByPriorApproval(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + 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 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + user4Session := loginUser(t, user4.Name) + user4Token := getTokenForLoggedInUser(t, user4Session, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser) + + apiBaseRepo := createActionsTestRepo(t, user2Token, "fork-approval-regression", 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) + + wfTreePath := ".gitea/workflows/ci.yml" + wfContent := `name: CI +on: pull_request +jobs: + test: + runs-on: ubuntu-latest + steps: + - run: echo ok +` + createWorkflowFile(t, user2Token, baseRepo.OwnerName, baseRepo.Name, wfTreePath, + getWorkflowCreateFileOptions(user2, baseRepo.DefaultBranch, "add ci", wfContent)) + + // user4 forks the repo + req := NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/repos/%s/%s/forks", baseRepo.OwnerName, baseRepo.Name), + &api.CreateForkOption{Name: new("fork-approval-regression-fork")}).AddTokenAuth(user4Token) + resp := MakeRequest(t, req, http.StatusAccepted) + apiForkRepo := DecodeJSON(t, resp, &api.Repository{}) + forkRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: apiForkRepo.ID}) + user4APICtx := NewAPITestContext(t, user4.Name, forkRepo.Name, auth_model.AccessTokenScopeWriteRepository) + defer doAPIDeleteRepository(user4APICtx)(t) + + // PR #1: a benign change from user4's fork — first-time contributor, gate engages. + doAPICreateFile(user4APICtx, "first.txt", &api.CreateFileOptions{ + FileOptions: api.FileOptions{ + NewBranchName: "first", + Message: "first", + 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("first")), + })(t) + pr1, err := doAPICreatePullRequest(user4APICtx, baseRepo.OwnerName, baseRepo.Name, baseRepo.DefaultBranch, user4.Name+":first")(t) + assert.NoError(t, err) + + run1 := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, TriggerUserID: user4.ID, Ref: fmt.Sprintf("refs/pull/%d/head", pr1.Index)}) + assert.True(t, run1.NeedApproval, "first fork PR must require approval") + assert.Equal(t, actions_model.StatusBlocked, run1.Status) + + // user2 approves run1. + req = NewRequest(t, "POST", fmt.Sprintf("%s/actions/approve-all-checks?commit_id=%s", baseRepo.Link(), pr1.Head.Sha)) + user2Session.MakeRequest(t, req, http.StatusOK) + run1 = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: run1.ID}) + assert.False(t, run1.NeedApproval) + assert.Equal(t, user2.ID, run1.ApprovedBy) + + // PR #2: same user, fresh branch. Pre-fix, this run was created with + // NeedApproval=false and dispatched immediately — the bypass path. + doAPICreateFile(user4APICtx, "second.txt", &api.CreateFileOptions{ + FileOptions: api.FileOptions{ + NewBranchName: "second", + Message: "second", + 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("second")), + })(t) + pr2, err := doAPICreatePullRequest(user4APICtx, baseRepo.OwnerName, baseRepo.Name, baseRepo.DefaultBranch, user4.Name+":second")(t) + assert.NoError(t, err) + + run2 := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, TriggerUserID: user4.ID, Ref: fmt.Sprintf("refs/pull/%d/head", pr2.Index)}) + assert.True(t, run2.NeedApproval, "second fork PR must still require approval — prior approval-to-run does not grant trust") + assert.Equal(t, actions_model.StatusBlocked, run2.Status) + assert.EqualValues(t, 0, run2.ApprovedBy) + + // After merging PR #1, user4 becomes a known contributor and the gate lifts for a new PR. + doAPIMergePullRequest(user2APICtx, baseRepo.OwnerName, baseRepo.Name, pr1.Index)(t) + doAPICreateFile(user4APICtx, "third.txt", &api.CreateFileOptions{ + FileOptions: api.FileOptions{ + NewBranchName: "third", + Message: "third", + 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("third")), + })(t) + pr3, err := doAPICreatePullRequest(user4APICtx, baseRepo.OwnerName, baseRepo.Name, baseRepo.DefaultBranch, user4.Name+":third")(t) + assert.NoError(t, err) + + run3 := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, TriggerUserID: user4.ID, Ref: fmt.Sprintf("refs/pull/%d/head", pr3.Index)}) + assert.False(t, run3.NeedApproval, "fork PR from a user with a prior merged PR should not require approval") + }) +} diff --git a/tests/integration/actions_concurrency_test.go b/tests/integration/actions_concurrency_test.go index eb7f55b049..4e1da3cced 100644 --- a/tests/integration/actions_concurrency_test.go +++ b/tests/integration/actions_concurrency_test.go @@ -486,14 +486,18 @@ jobs: }, ContentBase64: base64.StdEncoding.EncodeToString([]byte("user4-fix2")), })(t) - doAPICreatePullRequest(user4APICtx, baseRepo.OwnerName, baseRepo.Name, baseRepo.DefaultBranch, user4.Name+":do-not-cancel/ccc")(t) - // cannot fetch the task because cancel-in-progress is false + pr3, _ := doAPICreatePullRequest(user4APICtx, baseRepo.OwnerName, baseRepo.Name, baseRepo.DefaultBranch, user4.Name+":do-not-cancel/ccc")(t) + // cannot fetch the task: approval still required (user4 has no merged PR) and cancel-in-progress is false runner.fetchNoTask(t) runner.execTask(t, pr2Task1, &mockTaskOutcome{ result: runnerv1.Result_RESULT_SUCCESS, }) pr2Run1 = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: pr2Run1.ID}) assert.Equal(t, actions_model.StatusSuccess, pr2Run1.Status) + // user2 approves the third PR's run (user4 still has no merged PR, approval still required) + pr3Run1Pending := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, TriggerUserID: user4.ID, Ref: fmt.Sprintf("refs/pull/%d/head", pr3.Index)}) + req = NewRequest(t, "POST", fmt.Sprintf("/%s/%s/actions/runs/%d/approve", baseRepo.OwnerName, baseRepo.Name, pr3Run1Pending.ID)) + user2Session.MakeRequest(t, req, http.StatusOK) // fetch the task pr3Task1 := runner.fetchTask(t) _, _, pr3Run1 := getTaskAndJobAndRunByTaskID(t, pr3Task1.Id) -- 2.52.0 From d3c6998d3e5e62eb0c1c828d25e18b5d8f789849 Mon Sep 17 00:00:00 2001 From: Giteabot Date: Wed, 10 Jun 2026 00:32:28 -0700 Subject: [PATCH 04/17] fix(lfs): require Code-unit access for cross-repo LFS object reuse (#38006) (#38050) --- models/git/lfs.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/models/git/lfs.go b/models/git/lfs.go index 2ad31b764b..cfe4666222 100644 --- a/models/git/lfs.go +++ b/models/git/lfs.go @@ -196,7 +196,10 @@ func LFSObjectAccessible(ctx context.Context, user *user_model.User, oid string) count, err := db.GetEngine(ctx).Count(&LFSMetaObject{Pointer: lfs.Pointer{Oid: oid}}) return count > 0, err } - cond := repo_model.AccessibleRepositoryCondition(user, unit.TypeInvalid) + // LFS objects are repository code content, so authorization must require + // Code-unit access; other unit accesses (e.g. Issues) must not authorize + // reuse of an existing LFS object across repositories. + cond := repo_model.AccessibleRepositoryCondition(user, unit.TypeCode) count, err := db.GetEngine(ctx).Where(cond).Join("INNER", "repository", "`lfs_meta_object`.repository_id = `repository`.id").Count(&LFSMetaObject{Pointer: lfs.Pointer{Oid: oid}}) return count > 0, err } @@ -220,7 +223,7 @@ func LFSAutoAssociate(ctx context.Context, metas []*LFSMetaObject, user *user_mo newMetas := make([]*LFSMetaObject, 0, len(metas)) cond := builder.In( "`lfs_meta_object`.repository_id", - builder.Select("`repository`.id").From("repository").Where(repo_model.AccessibleRepositoryCondition(user, unit.TypeInvalid)), + builder.Select("`repository`.id").From("repository").Where(repo_model.AccessibleRepositoryCondition(user, unit.TypeCode)), ) if err := db.GetEngine(ctx).Cols("oid").Where(cond).In("oid", oids...).GroupBy("oid").Find(&newMetas); err != nil { return err -- 2.52.0 From fedce235d50f960f9d2f3358333054461b7d1fce Mon Sep 17 00:00:00 2001 From: Giteabot Date: Wed, 10 Jun 2026 03:05:23 -0700 Subject: [PATCH 05/17] fix(hostmatcher): block reserved IP ranges from external/private filters (#38039) (#38059) --- modules/hostmatcher/hostmatcher.go | 63 +++++++++++++++++++++++-- modules/hostmatcher/hostmatcher_test.go | 55 +++++++++++++++++++++ 2 files changed, 114 insertions(+), 4 deletions(-) diff --git a/modules/hostmatcher/hostmatcher.go b/modules/hostmatcher/hostmatcher.go index 044dba679a..7c17bc95da 100644 --- a/modules/hostmatcher/hostmatcher.go +++ b/modules/hostmatcher/hostmatcher.go @@ -8,6 +8,7 @@ import ( "path/filepath" "slices" "strings" + "sync" ) // HostMatchList is used to check if a host or IP is in a list. @@ -23,10 +24,64 @@ type HostMatchList struct { ipNets []*net.IPNet } -// MatchBuiltinExternal A valid non-private unicast IP, all hosts on public internet are matched +// MatchBuiltinExternal A valid global-unicast IP that is neither private (see MatchBuiltinPrivate) +// nor a reserved special-purpose range (see reservedIPNets); i.e. a routable host on the public internet. const MatchBuiltinExternal = "external" -// MatchBuiltinPrivate RFC 1918 (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16) and RFC 4193 (FC00::/7). Also called LAN/Intranet. +// reservedIPNets are special-purpose ranges that net.IP.IsPrivate omits but that must not be +// treated as public/external destinations (CGNAT, cloud metadata, IPv6 transition, etc.). We layer +// these on top of net.IP.IsPrivate (RFC 1918 / RFC 4193) so future additions to Go's IsPrivate are +// picked up automatically, while still covering the ranges it leaves out; otherwise the default +// allow-list would let authenticated users reach cloud metadata, internal, and IPv6 transition +// endpoints (SSRF), and a "private" block-list would fail to catch them. +var reservedIPNets = sync.OnceValue(func() []*net.IPNet { + var nets []*net.IPNet + for _, cidr := range []string{ + // IPv4 + "100.64.0.0/10", // RFC 6598 Carrier-Grade NAT + "168.63.129.16/32", // Azure WireServer metadata endpoint + "192.0.0.0/24", // RFC 6890 IETF protocol assignments + "192.0.2.0/24", // RFC 5737 TEST-NET-1 + "192.88.99.0/24", // RFC 7526 6to4 relay anycast (deprecated) + "198.18.0.0/15", // RFC 2544 benchmarking + "198.51.100.0/24", // RFC 5737 TEST-NET-2 + "203.0.113.0/24", // RFC 5737 TEST-NET-3 + // IPv6 + "100::/64", // RFC 6666 discard-only + "64:ff9b::/96", // RFC 6052 NAT64 (can embed IPv4 such as 169.254.169.254) + "64:ff9b:1::/48", // RFC 8215 local-use NAT64 + "2001::/32", // RFC 4380 Teredo tunneling (embeds IPv4) + "2001:10::/28", // RFC 4843 ORCHID (deprecated) + "2001:20::/28", // RFC 7343 ORCHIDv2 + "2001:db8::/32", // RFC 3849 documentation + "2002::/16", // RFC 3056 6to4 (embeds IPv4) + } { + _, ipNet, err := net.ParseCIDR(cidr) + if err != nil { + panic("hostmatcher: invalid reserved CIDR " + cidr + ": " + err.Error()) + } + nets = append(nets, ipNet) + } + return nets +}) + +// isPrivateIP reports whether ip falls in a private (net.IP.IsPrivate) or reserved special-purpose +// range (see reservedIPNets) that must not be considered a public/external destination. +func isPrivateIP(ip net.IP) bool { + if ip.IsPrivate() { + return true + } + for _, ipNet := range reservedIPNets() { + if ipNet.Contains(ip) { + return true + } + } + return false +} + +// MatchBuiltinPrivate RFC 1918 (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16) and RFC 4193 (FC00::/7), +// plus the reserved special-purpose ranges in reservedIPNets (CGNAT, NAT64, cloud metadata, etc.). +// Also called LAN/Intranet. const MatchBuiltinPrivate = "private" // MatchBuiltinLoopback 127.0.0.0/8 for IPv4 and ::1/128 for IPv6, localhost is included. @@ -100,11 +155,11 @@ func (hl *HostMatchList) checkIP(ip net.IP) bool { for _, builtin := range hl.builtins { switch builtin { case MatchBuiltinExternal: - if ip.IsGlobalUnicast() && !ip.IsPrivate() { + if ip.IsGlobalUnicast() && !isPrivateIP(ip) { return true } case MatchBuiltinPrivate: - if ip.IsPrivate() { + if isPrivateIP(ip) { return true } case MatchBuiltinLoopback: diff --git a/modules/hostmatcher/hostmatcher_test.go b/modules/hostmatcher/hostmatcher_test.go index c781847471..61582f28d3 100644 --- a/modules/hostmatcher/hostmatcher_test.go +++ b/modules/hostmatcher/hostmatcher_test.go @@ -159,3 +159,58 @@ func TestHostOrIPMatchesList(t *testing.T) { } test(cases) } + +// TestReservedRanges ensures special-purpose ranges that net.IP.IsPrivate misses are kept out of the +// "external" allow-list (the default for webhook delivery and repository migrations) and folded into +// the "private" block-list, so they cannot be used for SSRF to metadata/internal endpoints. +func TestReservedRanges(t *testing.T) { + external := ParseHostMatchList("", "external") + private := ParseHostMatchList("", "private") + + // legitimate public destinations: external, not private + for _, ip := range []string{"8.8.8.8", "1.1.1.1", "2001:4860:4860::8888", "1000::1"} { + addr := net.ParseIP(ip) + assert.Truef(t, external.MatchIPAddr(addr), "public ip %s should be external", ip) + assert.Falsef(t, private.MatchIPAddr(addr), "public ip %s should not be private", ip) + } + + // RFC 1918 / RFC 4193 private ranges (now folded into privateIPNets instead of net.IP.IsPrivate): + // not external, blockable as private. Includes range edges to guard the CIDR boundaries. + for _, ip := range []string{ + "10.0.0.0", "10.255.255.255", // 10.0.0.0/8 + "172.16.0.0", "172.31.255.255", // 172.16.0.0/12 + "192.168.0.0", "192.168.255.255", // 192.168.0.0/16 + "fc00::", "fdff:ffff:ffff:ffff:ffff:ffff:ffff:ffff", // fc00::/7 + } { + addr := net.ParseIP(ip) + assert.Falsef(t, external.MatchIPAddr(addr), "private ip %s must not be external", ip) + assert.Truef(t, private.MatchIPAddr(addr), "private ip %s should match private block-list", ip) + } + + // 172.32.0.0 is just outside 172.16.0.0/12: a public destination, not private + if addr := net.ParseIP("172.32.0.0"); assert.NotNil(t, addr) { + assert.True(t, external.MatchIPAddr(addr), "172.32.0.0 should be external") + assert.False(t, private.MatchIPAddr(addr), "172.32.0.0 should not be private") + } + + // reserved ranges that IsPrivate does not cover: not external, but blockable as private + for _, ip := range []string{ + "100.64.0.1", // CGNAT + "100.127.255.254", // CGNAT + "168.63.129.16", // Azure WireServer + "192.0.2.1", // TEST-NET-1 + "198.18.0.1", // benchmarking + "198.51.100.1", // TEST-NET-2 + "203.0.113.1", // TEST-NET-3 + "192.88.99.1", // 6to4 relay anycast + "64:ff9b::1", // NAT64 + "64:ff9b::a9fe:a9fe", // NAT64 embedding 169.254.169.254 + "2001::1", // Teredo + "2002::1", // 6to4 + "2001:db8::1", // documentation + } { + addr := net.ParseIP(ip) + assert.Falsef(t, external.MatchIPAddr(addr), "reserved ip %s must not be external", ip) + assert.Truef(t, private.MatchIPAddr(addr), "reserved ip %s should match private block-list", ip) + } +} -- 2.52.0 From 186ac68f03d4ac14b9aa3ed78f7d0bd714f34a77 Mon Sep 17 00:00:00 2001 From: Giteabot Date: Sun, 14 Jun 2026 07:29:27 -0700 Subject: [PATCH 06/17] fix: bound debian ParseControlFile to a single control stanza (#38044) (#38055) Backport #38044 by @metsw24-max **Packages-index stanza injection via Debian control file** A `.deb` whose `control` file appends extra paragraphs after a blank line was still accepted, and `ParseControlFile` stored the whole multi-stanza blob in `p.Control`. That blob is re-emitted verbatim into the generated `Packages` index, so the embedded blank line splits it into separate stanzas and an uploader can smuggle a package entry with an attacker-chosen `Filename` into the shared index. A binary control file only holds one stanza, so parsing now stops at the blank line that terminates it; well-formed packages are unaffected and the new subtest covers the trailing-stanza case. Signed-off-by: wxiaoguang Co-authored-by: metsw24-max Co-authored-by: wxiaoguang Co-authored-by: bircni --- modules/packages/debian/metadata.go | 15 +++++++++++++-- modules/packages/debian/metadata_test.go | 15 +++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/modules/packages/debian/metadata.go b/modules/packages/debian/metadata.go index c4c6bd8230..8c360eb7d0 100644 --- a/modules/packages/debian/metadata.go +++ b/modules/packages/debian/metadata.go @@ -146,15 +146,26 @@ func ParseControlFile(r io.Reader) (*Package, error) { var depends strings.Builder var control strings.Builder - s := bufio.NewScanner(io.TeeReader(r, &control)) + // https://www.debian.org/doc/debian-policy/ch-controlfields.html#syntax-of-control-files + s := bufio.NewScanner(r) for s.Scan() { line := s.Text() trimmed := strings.TrimSpace(line) if trimmed == "" { - continue + // A binary package control file holds exactly one stanza. Stop at the + // blank line that terminates it, otherwise a crafted control file could + // smuggle additional stanzas (with attacker-chosen Filename/Package + // fields) into the generated repository "Packages" index. + if control.Len() == 0 { + continue + } + break } + control.WriteString(line) + control.WriteByte('\n') + if line[0] == ' ' || line[0] == '\t' { switch key { case "Description": diff --git a/modules/packages/debian/metadata_test.go b/modules/packages/debian/metadata_test.go index 8b176fcb45..61dac1a2f5 100644 --- a/modules/packages/debian/metadata_test.go +++ b/modules/packages/debian/metadata_test.go @@ -184,4 +184,19 @@ func TestParseControlFile(t *testing.T) { assert.NotNil(t, p) } }) + + t.Run("SingleStanzaOnly", func(t *testing.T) { + // A control file with a trailing stanza must not leak the extra fields into + // p.Control, otherwise buildPackagesIndices would emit a second package entry + // with an attacker-chosen Filename into the repository "Packages" index. + content := bytes.NewBufferString("Package: realpkg\nVersion: 1.0.0\nArchitecture: amd64\nMaintainer: a \nDescription: real\n\nPackage: openssl\nVersion: 99.0\nArchitecture: amd64\nFilename: pool/main/o/openssl/evil.deb\nDescription: spoofed\n") + + p, err := ParseControlFile(content) + assert.NoError(t, err) + assert.NotNil(t, p) + assert.Equal(t, "realpkg", p.Name) + assert.Equal(t, "1.0.0", p.Version) + assert.NotContains(t, p.Control, "openssl") + assert.NotContains(t, p.Control, "evil.deb") + }) } -- 2.52.0 From bc578b7eba0a505d993c980cf1231358ee6bd0cf Mon Sep 17 00:00:00 2001 From: bircni Date: Wed, 17 Jun 2026 21:43:22 +0200 Subject: [PATCH 07/17] fix: Various sec fixes (#38108) (#38147) Backport #38108 - Enforce repository token scope on RSS/Atom feed endpoints so a PAT without repo scope can no longer read private repo commit data. - Block HTTP redirects during repository migration clones to prevent SSRF reaching internal addresses via an attacker-controlled redirect. - Redact the notification subject after repo access is revoked so private issue/PR metadata is no longer leaked through the notification API. Co-authored-by: Lunny Xiao --- modules/git/repo.go | 3 +++ modules/git/repo_test.go | 23 ++++++++++++++++ routers/web/feed/branch.go | 3 +++ routers/web/feed/file.go | 3 +++ routers/web/feed/release.go | 3 +++ routers/web/feed/render.go | 9 +++++++ routers/web/feed/repo.go | 3 +++ services/convert/notification.go | 29 +++++++++++--------- services/convert/notification_test.go | 30 +++++++++++++++++++++ tests/integration/feed_repo_test.go | 39 +++++++++++++++++++++++++++ 10 files changed, 132 insertions(+), 13 deletions(-) diff --git a/modules/git/repo.go b/modules/git/repo.go index 5b451b8dfa..bb161537aa 100644 --- a/modules/git/repo.go +++ b/modules/git/repo.go @@ -121,6 +121,9 @@ func Clone(ctx context.Context, from, to string, opts CloneRepoOptions) error { } cmd := gitcmd.NewCommand().AddArguments("clone") + // Never follow HTTP redirects: no clone caller needs them, and a remote redirecting to an + // otherwise-blocked address would be an SSRF vector (e.g. migrating from an attacker URL). + cmd.AddArguments("-c", "http.followRedirects=false") if opts.SkipTLSVerify { cmd.AddArguments("-c", "http.sslVerify=false") } diff --git a/modules/git/repo_test.go b/modules/git/repo_test.go index 776c297a34..be0a21a83d 100644 --- a/modules/git/repo_test.go +++ b/modules/git/repo_test.go @@ -4,7 +4,10 @@ package git import ( + "net/http" + "net/http/httptest" "path/filepath" + "sync/atomic" "testing" "github.com/stretchr/testify/assert" @@ -19,3 +22,23 @@ func TestRepoIsEmpty(t *testing.T) { assert.NoError(t, err) assert.True(t, isEmpty) } + +// TestCloneRefusesRedirects ensures Clone never follows HTTP redirects, so a remote +// cannot redirect to an otherwise-blocked address (SSRF, e.g. during migration). +func TestCloneRefusesRedirects(t *testing.T) { + var targetHit atomic.Bool + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + targetHit.Store(true) + w.WriteHeader(http.StatusNotFound) + })) + defer target.Close() + + redirect := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, target.URL+r.URL.Path, http.StatusFound) + })) + defer redirect.Close() + + err := Clone(t.Context(), redirect.URL, filepath.Join(t.TempDir(), "dst"), CloneRepoOptions{}) + assert.Error(t, err) + assert.False(t, targetHit.Load(), "git must not follow the redirect to the target") +} diff --git a/routers/web/feed/branch.go b/routers/web/feed/branch.go index cbb653b60f..e8f32ae396 100644 --- a/routers/web/feed/branch.go +++ b/routers/web/feed/branch.go @@ -15,6 +15,9 @@ import ( // ShowBranchFeed shows tags and/or releases on the repo as RSS / Atom feed func ShowBranchFeed(ctx *context.Context, repo *repo.Repository, formatType string) { + if !checkRepoFeedTokenScope(ctx) { + return + } var commits []*git.Commit var err error if ctx.Repo.Commit != nil { diff --git a/routers/web/feed/file.go b/routers/web/feed/file.go index 01f467c92a..ceb12f97d1 100644 --- a/routers/web/feed/file.go +++ b/routers/web/feed/file.go @@ -16,6 +16,9 @@ import ( // ShowFileFeed shows tags and/or releases on the repo as RSS / Atom feed func ShowFileFeed(ctx *context.Context, repo *repo.Repository, formatType string) { + if !checkRepoFeedTokenScope(ctx) { + return + } fileName := ctx.Repo.TreePath if len(fileName) == 0 { return diff --git a/routers/web/feed/release.go b/routers/web/feed/release.go index 9786b5f834..85816fd6a4 100644 --- a/routers/web/feed/release.go +++ b/routers/web/feed/release.go @@ -15,6 +15,9 @@ import ( // shows tags and/or releases on the repo as RSS / Atom feed func ShowReleaseFeed(ctx *context.Context, repo *repo_model.Repository, isReleasesOnly bool, formatType string) { + if !checkRepoFeedTokenScope(ctx) { + return + } releases, err := db.Find[repo_model.Release](ctx, repo_model.FindReleasesOptions{ IncludeTags: !isReleasesOnly, RepoID: ctx.Repo.Repository.ID, diff --git a/routers/web/feed/render.go b/routers/web/feed/render.go index 025f49c091..de1eb86b68 100644 --- a/routers/web/feed/render.go +++ b/routers/web/feed/render.go @@ -4,9 +4,18 @@ package feed import ( + auth_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/auth" "code.mokoconsulting.tech/MokoConsulting/MokoGitea/services/context" ) +// checkRepoFeedTokenScope ensures an API token has repository read scope before a +// feed serves private repository content, mirroring checkDownloadTokenScope for +// downloads. Returns false (and writes the response) when the token is denied. +func checkRepoFeedTokenScope(ctx *context.Context) bool { + context.CheckRepoScopedToken(ctx, ctx.Repo.Repository, auth_model.Read) + return !ctx.Written() +} + // RenderBranchFeed render format for branch or file func RenderBranchFeed(ctx *context.Context, feedType string) { if ctx.Repo.TreePath == "" { diff --git a/routers/web/feed/repo.go b/routers/web/feed/repo.go index f3ccdc3670..b78a211569 100644 --- a/routers/web/feed/repo.go +++ b/routers/web/feed/repo.go @@ -16,6 +16,9 @@ import ( // ShowRepoFeed shows user activity on the repo as RSS / Atom feed func ShowRepoFeed(ctx *context.Context, repo *repo_model.Repository, formatType string) { + if !checkRepoFeedTokenScope(ctx) { + return + } actions, _, err := feed_service.GetFeeds(ctx, activities_model.GetFeedsOptions{ RequestedRepo: repo, Actor: ctx.Doer, diff --git a/services/convert/notification.go b/services/convert/notification.go index 179a4ee54b..b815893f70 100644 --- a/services/convert/notification.go +++ b/services/convert/notification.go @@ -24,19 +24,22 @@ func ToNotificationThread(ctx context.Context, n *activities_model.Notification) } // since user only get notifications when he has access to use minimal access mode - if n.Repository != nil { - perm, err := access_model.GetIndividualUserRepoPermission(ctx, n.Repository, n.User) - if err != nil { - log.Error("GetIndividualUserRepoPermission failed: %v", err) - return result - } - if perm.HasAnyUnitAccessOrPublicAccess() { // if user has been revoked access to repo, do not show repo info - result.Repository = ToRepo(ctx, n.Repository, perm) - // This permission is not correct and we should not be reporting it - for repository := result.Repository; repository != nil; repository = repository.Parent { - repository.Permissions = nil - } - } + if n.Repository == nil { + return result + } + perm, err := access_model.GetIndividualUserRepoPermission(ctx, n.Repository, n.User) + if err != nil { + log.Error("GetIndividualUserRepoPermission failed: %v", err) + return result + } + // if the user has been revoked access to the repo, do not leak repo or subject info + if !perm.HasAnyUnitAccessOrPublicAccess() { + return result + } + result.Repository = ToRepo(ctx, n.Repository, perm) + // This permission is not correct and we should not be reporting it + for repository := result.Repository; repository != nil; repository = repository.Parent { + repository.Permissions = nil } // handle Subject diff --git a/services/convert/notification_test.go b/services/convert/notification_test.go index 1e970cb1d3..7467fae05a 100644 --- a/services/convert/notification_test.go +++ b/services/convert/notification_test.go @@ -39,6 +39,36 @@ func TestToNotificationThreadOmitsRepoWhenAccessRevoked(t *testing.T) { assert.Nil(t, thread.Repository) } +func TestToNotificationThreadOmitsSubjectWhenAccessRevoked(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + ctx := t.Context() + // repo 2 is private; user 4 has no access to it + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) + assert.NoError(t, repo.LoadOwner(ctx)) + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 4, RepoID: repo.ID}) + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + + n := &activities_model.Notification{ + ID: 12345, + UserID: user.ID, + RepoID: repo.ID, + Status: activities_model.NotificationStatusUnread, + Source: activities_model.NotificationSourceIssue, + IssueID: issue.ID, + UpdatedUnix: timeutil.TimeStampNow(), + Issue: issue, + Repository: repo, + User: user, + } + + thread := ToNotificationThread(ctx, n) + + // must not leak private issue metadata once access is revoked + assert.Nil(t, thread.Repository) + assert.Nil(t, thread.Subject) +} + func TestToNotificationThread(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) diff --git a/tests/integration/feed_repo_test.go b/tests/integration/feed_repo_test.go index 79db0c256c..5cc4d49f17 100644 --- a/tests/integration/feed_repo_test.go +++ b/tests/integration/feed_repo_test.go @@ -8,6 +8,7 @@ import ( "net/http" "testing" + auth_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/auth" "code.mokoconsulting.tech/MokoConsulting/MokoGitea/tests" "github.com/stretchr/testify/assert" @@ -33,3 +34,41 @@ func TestFeedRepo(t *testing.T) { assert.NotEmpty(t, rss.Channel.Items[0].PubDate) }) } + +// TestFeedRepoContentTokenScopes ensures repository feed endpoints enforce the +// repository token scope, so a PAT without repository scope cannot read private +// repository commit/activity data through RSS/Atom feeds. +func TestFeedRepoContentTokenScopes(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + // user2/repo2 is a private repository owned by user2 + ownerReadToken := getUserToken(t, "user2", auth_model.AccessTokenScopeReadRepository) + miscToken := getUserToken(t, "user2", auth_model.AccessTokenScopeReadMisc) + + urls := []string{ + "/user2/repo2.rss", + "/user2/repo2.atom", + "/user2/repo2/rss/branch/master", + "/user2/repo2/atom/branch/master", + "/user2/repo2/rss/branch/master/README.md", + "/user2/repo2/tags.rss", + "/user2/repo2/tags.atom", + "/user2/repo2/releases.rss", + "/user2/repo2/releases.atom", + } + + for _, url := range urls { + t.Run(url, func(t *testing.T) { + // feed routes only accept basic auth, so authenticate as the advisory PoC does (user:token) + reqDenied := NewRequest(t, "GET", url) + reqDenied.SetBasicAuth("user2", miscToken) + // a token without repository scope must be denied + MakeRequest(t, reqDenied, http.StatusForbidden) + + reqAllowed := NewRequest(t, "GET", url) + reqAllowed.SetBasicAuth("user2", ownerReadToken) + // a token with repository read scope is allowed + MakeRequest(t, reqAllowed, http.StatusOK) + }) + } +} -- 2.52.0 From 26ad4fd03fc094f0d789b2d37f245961e2c9c869 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 17 Jun 2026 13:19:24 -0700 Subject: [PATCH 08/17] fix(auth): ignore stale OIDC external login links to organizations (#37875) (#38141) Backport #37875 This fixes an OIDC sign-in edge case where a stale `external_login_user` record can still point to an organization or a deleted user. In that situation, Gitea may keep resolving the external login to the wrong account during sign-in. For affected instances, this matches the behavior reported in #36439 and #37812, where a user signing in with OIDC/Entra ID could appear as an organization, or hit a 404 after that organization was removed. - validate the user resolved from `external_login_user` during OAuth2/OIDC login - ignore stale links when the linked user no longer exists - ignore stale links when the linked user is not an individual user - remove the stale external login row so the sign-in flow can relink the external account to the correct user - Fixes #37812 - Related to #36439 Co-authored-by: silverwind Co-authored-by: Claude (Opus 4.8) Co-authored-by: bircni --- models/user/external_login_user.go | 13 +++- routers/web/auth/oauth.go | 30 ++++++-- .../auth/source/oauth2/source_sync_test.go | 14 +--- tests/integration/auth_oauth2_test.go | 70 ++++++++++++++++++- 4 files changed, 103 insertions(+), 24 deletions(-) diff --git a/models/user/external_login_user.go b/models/user/external_login_user.go index 1abe1a8acb..4e32be3e1e 100644 --- a/models/user/external_login_user.go +++ b/models/user/external_login_user.go @@ -80,8 +80,11 @@ func init() { } // GetExternalLogin checks if a externalID in loginSourceID scope already exists -func GetExternalLogin(ctx context.Context, externalLoginUser *ExternalLoginUser) (bool, error) { - return db.GetEngine(ctx).Get(externalLoginUser) +func GetExternalLogin(ctx context.Context, loginSourceID int64, externalID string) (*ExternalLoginUser, bool, error) { + return db.Get[ExternalLoginUser](ctx, builder.Eq{ + "external_id": externalID, + "login_source_id": loginSourceID, + }) } // LinkExternalToUser link the external user to the user @@ -118,6 +121,12 @@ func RemoveAllAccountLinks(ctx context.Context, user *User) error { return err } +// RemoveExternalLoginByExternalID removes a specific external login link by its provider-side identifier. +func RemoveExternalLoginByExternalID(ctx context.Context, loginSourceID int64, externalID string) error { + _, err := db.GetEngine(ctx).Where("external_id=? AND login_source_id=?", externalID, loginSourceID).Delete(new(ExternalLoginUser)) + return err +} + // GetUserIDByExternalUserID get user id according to provider and userID func GetUserIDByExternalUserID(ctx context.Context, provider, userID string) (int64, error) { var id int64 diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index 362824646f..43d2faffa5 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -514,17 +514,33 @@ func oAuth2UserLoginCallback(ctx *context.Context, authSource *auth.Source, requ } // search in external linked users - externalLoginUser := &user_model.ExternalLoginUser{ - ExternalID: gothUser.UserID, - LoginSourceID: authSource.ID, - } - hasUser, err = user_model.GetExternalLogin(request.Context(), externalLoginUser) + externalLoginUser, hasUser, err := user_model.GetExternalLogin(ctx, authSource.ID, gothUser.UserID) if err != nil { return nil, goth.User{}, err } if hasUser { - user, err = user_model.GetUserByID(request.Context(), externalLoginUser.UserID) - return user, gothUser, err + user, err = user_model.GetUserByID(ctx, externalLoginUser.UserID) + if err != nil && !user_model.IsErrUserNotExist(err) { + return nil, goth.User{}, err + } + if err == nil && user.IsIndividual() { + return user, gothUser, nil + } + + // The external login record is stale: the linked user no longer exists, or it exists but is + // not an individual user (only individual users can sign in, so a link pointing at an + // organization, bot or remote user can never resolve). Remove it so the next sign-in can + // relink the external account to the correct user. Nothing is lost, because the link is + // recreated automatically on the next sign-in. + reason := "linked user does not exist" + if err == nil { + reason = fmt.Sprintf("linked user type is %d", user.Type) + } + log.Warn("Ignoring stale external login link [external-id=%s login-source-id=%d user-id=%d]: %s", externalLoginUser.ExternalID, externalLoginUser.LoginSourceID, externalLoginUser.UserID, reason) + + if err := user_model.RemoveExternalLoginByExternalID(ctx, externalLoginUser.LoginSourceID, externalLoginUser.ExternalID); err != nil { + return nil, goth.User{}, err + } } // no user found to login diff --git a/services/auth/source/oauth2/source_sync_test.go b/services/auth/source/oauth2/source_sync_test.go index 7f886f3c82..6518c45794 100644 --- a/services/auth/source/oauth2/source_sync_test.go +++ b/services/auth/source/oauth2/source_sync_test.go @@ -57,12 +57,7 @@ func TestSource(t *testing.T) { err := source.refresh(t.Context(), provider, e) assert.NoError(t, err) - e := &user_model.ExternalLoginUser{ - ExternalID: e.ExternalID, - LoginSourceID: e.LoginSourceID, - } - - ok, err := user_model.GetExternalLogin(t.Context(), e) + e, ok, err := user_model.GetExternalLogin(t.Context(), e.LoginSourceID, e.ExternalID) assert.NoError(t, err) assert.True(t, ok) assert.Equal(t, "refresh", e.RefreshToken) @@ -82,12 +77,7 @@ func TestSource(t *testing.T) { }) assert.NoError(t, err) - e := &user_model.ExternalLoginUser{ - ExternalID: e.ExternalID, - LoginSourceID: e.LoginSourceID, - } - - ok, err := user_model.GetExternalLogin(t.Context(), e) + e, ok, err := user_model.GetExternalLogin(t.Context(), e.LoginSourceID, e.ExternalID) assert.NoError(t, err) assert.True(t, ok) assert.Empty(t, e.RefreshToken) diff --git a/tests/integration/auth_oauth2_test.go b/tests/integration/auth_oauth2_test.go index fc57088e3b..856e2e95e9 100644 --- a/tests/integration/auth_oauth2_test.go +++ b/tests/integration/auth_oauth2_test.go @@ -130,8 +130,72 @@ func TestMigrateAzureADV2ToOIDC(t *testing.T) { }) } -// newFakeOIDCServer starts an httptest.Server that implements the minimum OIDC endpoints needed to complete a sign-in flow: func newFakeOIDCServer(t *testing.T, sub, oid string) *httptest.Server { + return newFakeOIDCServerWithProfile(t, sub, oid, sub+"@example.com", "OIDC Test User") +} + +func TestOIDCIgnoresStaleExternalLoginLinks(t *testing.T) { + defer tests.PrepareTestEnv(t)() + defer test.MockVariableValue(&setting.OAuth2Client.EnableAutoRegistration, true)() + defer test.MockVariableValue(&setting.OAuth2Client.AccountLinking, setting.OAuth2AccountLinkingAuto)() + defer test.MockVariableValue(&setting.OAuth2Client.Username, setting.OAuth2UsernameEmail)() + + setup := func(t *testing.T, sourceName, sub, userName, email string) (*auth_model.Source, *user_model.User) { + t.Helper() + srv := newFakeOIDCServerWithProfile(t, sub, sub+"-oid", email, "OIDC Test User") + addOAuth2Source(t, sourceName, oauth2.Source{ + Provider: "openidConnect", + ClientID: "test-client-id", + ClientSecret: "test-client-secret", + OpenIDConnectAutoDiscoveryURL: srv.URL + "/.well-known/openid-configuration", + }) + authSource, err := auth_model.GetActiveOAuth2SourceByAuthName(t.Context(), sourceName) + require.NoError(t, err) + correctUser := &user_model.User{Name: userName, Email: email} + require.NoError(t, user_model.CreateUser(t.Context(), correctUser, &user_model.Meta{})) + return authSource, correctUser + } + + // assertRelinked signs in via OIDC and asserts the stale link now points at the correct individual user. + assertRelinked := func(t *testing.T, authSource *auth_model.Source, sub string, correctUser *user_model.User) { + t.Helper() + doOIDCSignIn(t, authSource.Name) + // external_login_user has no "id" column, so order by the primary key instead + externalLink := unittest.AssertExistsAndLoadBean(t, &user_model.ExternalLoginUser{ExternalID: sub, LoginSourceID: authSource.ID}, unittest.OrderBy("external_id ASC")) + assert.Equal(t, correctUser.ID, externalLink.UserID) + assert.Equal(t, correctUser.Email, externalLink.Email) + assert.Equal(t, "OIDC Test User", externalLink.Name) + } + + t.Run("organization", func(t *testing.T) { + const sub, userName, email = "oidc-stale-org-link-sub", "guizar_m", "guizar_m@example.com" + authSource, correctUser := setup(t, "test-oidc-stale-org-link", sub, userName, email) + org := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3, Type: user_model.UserTypeOrganization}) + require.NoError(t, user_model.LinkExternalToUser(t.Context(), org, &user_model.ExternalLoginUser{ + ExternalID: sub, + UserID: org.ID, + LoginSourceID: authSource.ID, + Provider: authSource.Name, + })) + assertRelinked(t, authSource, sub, correctUser) + }) + + t.Run("deleted user", func(t *testing.T) { + const sub, userName, email = "oidc-stale-deleted-link-sub", "guizar_d", "guizar_d@example.com" + const deletedUserID = 999999 + authSource, correctUser := setup(t, "test-oidc-stale-deleted", sub, userName, email) + // link the external account to a user id that does not exist, simulating a deleted user + require.NoError(t, user_model.LinkExternalToUser(t.Context(), &user_model.User{ID: deletedUserID}, &user_model.ExternalLoginUser{ + ExternalID: sub, + UserID: deletedUserID, + LoginSourceID: authSource.ID, + Provider: authSource.Name, + })) + assertRelinked(t, authSource, sub, correctUser) + }) +} + +func newFakeOIDCServerWithProfile(t *testing.T, sub, oid, email, name string) *httptest.Server { t.Helper() var srv *httptest.Server @@ -169,8 +233,8 @@ func newFakeOIDCServer(t *testing.T, sub, oid string) *httptest.Server { // sub MUST match the id_token sub; goth rejects mismatches. _ = json.NewEncoder(w).Encode(map[string]any{ "sub": sub, - "email": sub + "@example.com", - "name": "OIDC Test User", + "email": email, + "name": name, }) default: http.NotFound(w, r) -- 2.52.0 From cbf34fb9878492a37234483c81f6c6a7e9fb18ba Mon Sep 17 00:00:00 2001 From: bircni Date: Wed, 17 Jun 2026 22:49:59 +0200 Subject: [PATCH 09/17] fix: Various security fixes (#38103) (#38151) Backport #38103 - Enforce org visibility on organization label read endpoints (private org labels no longer leak to non-members). - Block fork sync (`merge-upstream`) when the base repo is no longer readable (stops pulling commits after a parent goes private). - Remove `REVERSE_PROXY_LIMIT` / `REVERSE_PROXY_TRUSTED_PROXIES` from the Docker `app.ini` templates (the `= *` default allowed `X-WEBAUTH-USER` impersonation; reverse-proxy auth is now opt-in and admin-configured). - Enforce single-use TOTP passcodes across web login, password-reset, and Basic-Auth `X-Gitea-OTP` (fixes a TOCTOU race and a stateless replay). - Re-check branch write permission for every ref in a push (the pre-receive hook cached the first ref's result, letting a per-branch maintainer-edit grant escalate to full repo write). --------- Co-authored-by: wxiaoguang --- docker/root/etc/templates/app.ini | 2 - docker/rootless/etc/templates/app.ini | 2 - models/auth/twofactor.go | 32 +++++- models/auth/twofactor_test.go | 47 ++++++++ routers/api/v1/api.go | 108 ++++++++++++------ routers/api/v1/repo/branch.go | 3 + routers/private/hook_pre_receive.go | 46 +++++--- routers/private/hook_pre_receive_test.go | 70 ++++++++++++ routers/web/auth/2fa.go | 12 +- routers/web/auth/password.go | 12 +- routers/web/repo/branch.go | 2 +- services/auth/basic.go | 3 +- services/repository/merge_upstream.go | 13 +++ tests/integration/api_org_test.go | 45 +++++++- tests/integration/api_twofa_test.go | 6 + tests/integration/repo_merge_upstream_test.go | 13 +++ 16 files changed, 333 insertions(+), 83 deletions(-) create mode 100644 models/auth/twofactor_test.go create mode 100644 routers/private/hook_pre_receive_test.go diff --git a/docker/root/etc/templates/app.ini b/docker/root/etc/templates/app.ini index 01fb407f49..1794d1f128 100644 --- a/docker/root/etc/templates/app.ini +++ b/docker/root/etc/templates/app.ini @@ -51,8 +51,6 @@ ROOT_PATH = /data/gitea/log [security] INSTALL_LOCK = $INSTALL_LOCK SECRET_KEY = $SECRET_KEY -REVERSE_PROXY_LIMIT = 1 -REVERSE_PROXY_TRUSTED_PROXIES = * [service] DISABLE_REGISTRATION = $DISABLE_REGISTRATION diff --git a/docker/rootless/etc/templates/app.ini b/docker/rootless/etc/templates/app.ini index 0057635062..0357e6aa1f 100644 --- a/docker/rootless/etc/templates/app.ini +++ b/docker/rootless/etc/templates/app.ini @@ -48,8 +48,6 @@ ROOT_PATH = $GITEA_WORK_DIR/data/log [security] INSTALL_LOCK = $INSTALL_LOCK SECRET_KEY = $SECRET_KEY -REVERSE_PROXY_LIMIT = 1 -REVERSE_PROXY_TRUSTED_PROXIES = * [service] DISABLE_REGISTRATION = $DISABLE_REGISTRATION diff --git a/models/auth/twofactor.go b/models/auth/twofactor.go index 1875dc3ee3..4c4cda30a6 100644 --- a/models/auth/twofactor.go +++ b/models/auth/twofactor.go @@ -21,6 +21,7 @@ import ( "github.com/pquerna/otp/totp" "golang.org/x/crypto/pbkdf2" + "xorm.io/builder" ) // @@ -104,20 +105,43 @@ func (t *TwoFactor) SetSecret(secretString string) error { return nil } -// ValidateTOTP validates the provided passcode. -func (t *TwoFactor) ValidateTOTP(passcode string) (bool, error) { +// validateTOTP validates the provided passcode. It does not consume the passcode; all login +// surfaces must go through ValidateAndConsumeTOTP so that a passcode cannot be redeemed twice. +func (t *TwoFactor) validateTOTP(passcode string) (bool, error) { decodedStoredSecret, err := base64.StdEncoding.DecodeString(t.Secret) if err != nil { - return false, fmt.Errorf("ValidateTOTP invalid base64: %w", err) + return false, fmt.Errorf("validateTOTP invalid base64: %w", err) } secretBytes, err := secret.AesDecrypt(t.getEncryptionKey(), decodedStoredSecret) if err != nil { - return false, fmt.Errorf("ValidateTOTP unable to decrypt (maybe SECRET_KEY is wrong): %w", 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 } +// ValidateAndConsumeTOTP validates the passcode and atomically records it as used so that the +// same passcode cannot be redeemed more than once (RFC 6238 §5.2). It returns false for an +// invalid passcode as well as for a replay, including the case where a concurrent request with +// the same passcode won the race first. All TOTP login surfaces must go through this helper. +func (t *TwoFactor) ValidateAndConsumeTOTP(ctx context.Context, passcode string) (bool, error) { + ok, err := t.validateTOTP(passcode) + if err != nil || !ok { + return false, err + } + // Conditional update: only a row whose stored passcode differs from this one is updated, so a + // replay (or a concurrent duplicate) matches zero rows and is rejected. The row lock taken by + // the UPDATE serializes racing requests, closing the read-validate-write TOCTOU window. + t.LastUsedPasscode = passcode + n, err := db.GetEngine(ctx).ID(t.ID). + Where(builder.Or(builder.IsNull{"last_used_passcode"}, builder.Neq{"last_used_passcode": passcode})). + Cols("last_used_passcode").Update(t) + if err != nil { + return false, err + } + return n == 1, nil +} + // NewTwoFactor creates a new two-factor authentication token. func NewTwoFactor(ctx context.Context, t *TwoFactor) error { _, err := db.GetEngine(ctx).Insert(t) diff --git a/models/auth/twofactor_test.go b/models/auth/twofactor_test.go new file mode 100644 index 0000000000..e7951920f9 --- /dev/null +++ b/models/auth/twofactor_test.go @@ -0,0 +1,47 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package auth_test + +import ( + "testing" + "time" + + auth_model "code.gitea.io/gitea/models/auth" + "code.gitea.io/gitea/models/unittest" + + "github.com/pquerna/otp/totp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestTwoFactorValidateAndConsumeTOTP(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + key, err := totp.Generate(totp.GenerateOpts{SecretSize: 40, Issuer: "gitea-test", AccountName: "consume"}) + require.NoError(t, err) + + tfa := &auth_model.TwoFactor{UID: 1} + require.NoError(t, tfa.SetSecret(key.Secret())) + require.NoError(t, auth_model.NewTwoFactor(t.Context(), tfa)) + + passcode, err := totp.GenerateCode(key.Secret(), time.Now()) + require.NoError(t, err) + + // first use of a valid passcode succeeds + ok, err := tfa.ValidateAndConsumeTOTP(t.Context(), passcode) + require.NoError(t, err) + assert.True(t, ok) + + // replaying the same passcode is refused, even when still inside the TOTP validity window + reloaded, err := auth_model.GetTwoFactorByUID(t.Context(), tfa.UID) + require.NoError(t, err) + ok, err = reloaded.ValidateAndConsumeTOTP(t.Context(), passcode) + require.NoError(t, err) + assert.False(t, ok) + + // an invalid passcode is rejected without consuming anything + ok, err = reloaded.ValidateAndConsumeTOTP(t.Context(), "000000") + require.NoError(t, err) + assert.False(t, ok) +} diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 5433f11f25..a80aab03d2 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -508,41 +508,79 @@ func reqOrgOwnership() func(ctx *context.APIContext) { } } -// reqTeamMembership user should be an team member, or a site admin +// reqOrgVisible requires the organization to be visible to the doer, or a site admin +func reqOrgVisible() func(ctx *context.APIContext) { + return func(ctx *context.APIContext) { + if ctx.Org.Organization == nil { + setting.PanicInDevOrTesting("reqOrgVisible: unprepared context") + ctx.APIErrorInternal(errors.New("reqOrgVisible: unprepared context")) + return + } + if !organization.HasOrgOrUserVisible(ctx, ctx.Org.Organization.AsUser(), ctx.Doer) { + ctx.APIErrorNotFound() + return + } + } +} + +func teamAccessPrivileged(ctx *context.APIContext) (orgID int64, privileged, ok bool) { + if ctx.IsUserSiteAdmin() { + return 0, true, true + } + if ctx.Org.Team == nil { + setting.PanicInDevOrTesting("teamAccess: unprepared context") + ctx.APIErrorInternal(errors.New("teamAccess: unprepared context")) + return 0, false, false + } + + orgID = ctx.Org.Team.OrgID + isOwner, err := organization.IsOrganizationOwner(ctx, orgID, ctx.Doer.ID) + if err != nil { + ctx.APIErrorInternal(err) + return 0, false, false + } else if isOwner { + return orgID, true, true + } + + isTeamMember, err := organization.IsTeamMember(ctx, orgID, ctx.Org.Team.ID, ctx.Doer.ID) + if err != nil { + ctx.APIErrorInternal(err) + return 0, false, false + } + return orgID, isTeamMember, true +} + +func denyNonTeamMember(ctx *context.APIContext, orgID int64) { + isOrgMember, err := organization.IsOrganizationMember(ctx, orgID, ctx.Doer.ID) + if err != nil { + ctx.APIErrorInternal(err) + } else if isOrgMember { + ctx.APIError(http.StatusForbidden, "Must be a team member") + } else { + ctx.APIErrorNotFound() + } +} + +// reqTeamReadAccess allows callers who can list the team to read its metadata. +// Not sufficient for mutations — use reqOrgOwnership() or reqTeamMembership() for those. +func reqTeamReadAccess() func(ctx *context.APIContext) { + return func(ctx *context.APIContext) { + orgID, privileged, ok := teamAccessPrivileged(ctx) + if !ok || privileged { + return + } + denyNonTeamMember(ctx, orgID) + } +} + +// reqTeamMembership user should be a team member, or a site admin func reqTeamMembership() func(ctx *context.APIContext) { return func(ctx *context.APIContext) { - if ctx.IsUserSiteAdmin() { - return - } - if ctx.Org.Team == nil { - setting.PanicInDevOrTesting("reqTeamMembership: unprepared context") - ctx.APIErrorInternal(errors.New("reqTeamMembership: unprepared context")) - return - } - - orgID := ctx.Org.Team.OrgID - isOwner, err := organization.IsOrganizationOwner(ctx, orgID, ctx.Doer.ID) - if err != nil { - ctx.APIErrorInternal(err) - return - } else if isOwner { - return - } - - if isTeamMember, err := organization.IsTeamMember(ctx, orgID, ctx.Org.Team.ID, ctx.Doer.ID); err != nil { - ctx.APIErrorInternal(err) - return - } else if !isTeamMember { - isOrgMember, err := organization.IsOrganizationMember(ctx, orgID, ctx.Doer.ID) - if err != nil { - ctx.APIErrorInternal(err) - } else if isOrgMember { - ctx.APIError(http.StatusForbidden, "Must be a team member") - } else { - ctx.APIErrorNotFound() - } + orgID, privileged, ok := teamAccessPrivileged(ctx) + if !ok || privileged { return } + denyNonTeamMember(ctx, orgID) } } @@ -1745,7 +1783,7 @@ func Routes() *web.Router { m.Combo("/{id}").Get(reqToken(), org.GetLabel). Patch(reqToken(), reqOrgOwnership(), bind(api.EditLabelOption{}), org.EditLabel). Delete(reqToken(), reqOrgOwnership(), org.DeleteLabel) - }) + }, reqOrgVisible()) m.Group("/hooks", func() { m.Combo("").Get(org.ListHooks). Post(bind(api.CreateHookOption{}), org.CreateHook) @@ -1799,12 +1837,12 @@ func Routes() *web.Router { m.Group("/repos", func() { m.Get("", reqToken(), org.GetTeamRepos) m.Combo("/{org}/{reponame}"). - Put(reqToken(), org.AddTeamRepository). - Delete(reqToken(), org.RemoveTeamRepository). + Put(reqToken(), reqTeamMembership(), org.AddTeamRepository). + Delete(reqToken(), reqTeamMembership(), org.RemoveTeamRepository). Get(reqToken(), org.GetTeamRepo) }) m.Get("/activities/feeds", org.ListTeamActivityFeeds) - }, tokenRequiresScopes(auth_model.AccessTokenScopeCategoryOrganization), orgAssignment(false, true), reqToken(), reqTeamMembership(), checkTokenPublicOnly()) + }, tokenRequiresScopes(auth_model.AccessTokenScopeCategoryOrganization), orgAssignment(false, true), reqToken(), reqTeamReadAccess(), checkTokenPublicOnly()) m.Group("/admin", func() { m.Group("/cron", func() { diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index 31cddff6c3..4c789fb15b 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -1365,6 +1365,9 @@ func MergeUpstream(ctx *context.APIContext) { } else if errors.Is(err, util.ErrNotExist) { ctx.APIError(http.StatusNotFound, err) return + } else if errors.Is(err, util.ErrPermissionDenied) { + ctx.APIError(http.StatusForbidden, err.Error()) + return } ctx.APIErrorInternal(err) return diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index 0dc1c9a9a0..f42ef69274 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -40,9 +40,6 @@ type preReceiveContext struct { canCreatePullRequest bool checkedCanCreatePullRequest bool - canWriteCode bool - checkedCanWriteCode bool - protectedTags []*git_model.ProtectedTag gotProtectedTags bool @@ -50,24 +47,36 @@ type preReceiveContext struct { opts *private.HookOptions - branchName string + // this context should only contain shared variables, mutable variables like "current branch name" shouldn't be put here + canWriteCodeUnitCached *bool } -// CanWriteCode returns true if pusher can write code -func (ctx *preReceiveContext) CanWriteCode() bool { - if !ctx.checkedCanWriteCode { - if !ctx.loadPusherAndPermission() { - return false +func (ctx *preReceiveContext) canWriteCodeUnit() bool { + if ctx.canWriteCodeUnitCached == nil { + var canWrite bool + if ctx.loadPusherAndPermission() { + canWrite = ctx.userPerm.CanWrite(unit.TypeCode) || ctx.deployKeyAccessMode >= perm_model.AccessModeWrite } - ctx.canWriteCode = issues_model.CanMaintainerWriteToBranch(ctx, ctx.userPerm, ctx.branchName, ctx.user) || ctx.deployKeyAccessMode >= perm_model.AccessModeWrite - ctx.checkedCanWriteCode = true + ctx.canWriteCodeUnitCached = &canWrite } - return ctx.canWriteCode + return *ctx.canWriteCodeUnitCached } -// AssertCanWriteCode returns true if pusher can write code -func (ctx *preReceiveContext) AssertCanWriteCode() bool { - if !ctx.CanWriteCode() { +// canWriteCodeRef returns true if pusher can write to the code ref (branch/tag/commit) +func (ctx *preReceiveContext) canWriteCodeRef(refFullName git.RefName) bool { + if ctx.canWriteCodeUnit() { + return true + } + // then check whether if the pusher is a maintainer who can write the PR author's head repo branch + if !refFullName.IsBranch() { + return false + } + return issues_model.CanMaintainerWriteToBranch(ctx, ctx.userPerm, refFullName.BranchName(), ctx.user) +} + +// assertCanWriteRef returns true if pusher can write to the code ref, otherwise it responds with 403 Forbidden and returns false +func (ctx *preReceiveContext) assertCanWriteRef(refFullName git.RefName) bool { + if !ctx.canWriteCodeRef(refFullName) { if ctx.Written() { return false } @@ -129,7 +138,7 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) { case git.DefaultFeatures().SupportProcReceive && refFullName.IsFor(): preReceiveFor(ourCtx, refFullName) default: - ourCtx.AssertCanWriteCode() + ourCtx.assertCanWriteRef(refFullName) } if ctx.Written() { return @@ -141,9 +150,8 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) { func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, refFullName git.RefName) { branchName := refFullName.BranchName() - ctx.branchName = branchName - if !ctx.AssertCanWriteCode() { + if !ctx.assertCanWriteRef(refFullName) { return } @@ -421,7 +429,7 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, r } func preReceiveTag(ctx *preReceiveContext, refFullName git.RefName) { - if !ctx.AssertCanWriteCode() { + if !ctx.assertCanWriteRef(refFullName) { return } diff --git a/routers/private/hook_pre_receive_test.go b/routers/private/hook_pre_receive_test.go new file mode 100644 index 0000000000..8d99b6b8fd --- /dev/null +++ b/routers/private/hook_pre_receive_test.go @@ -0,0 +1,70 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package private + +import ( + "testing" + + issues_model "code.gitea.io/gitea/models/issues" + "code.gitea.io/gitea/models/perm/access" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unittest" + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/services/contexttest" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestPreReceiveCanWriteCodePerBranch ensures the maintainer-edit write grant is evaluated against +// the exact ref being pushed on every call, derived from that ref rather than shared mutable state. +// Otherwise a per-branch grant (an open PR with "allow edits from maintainers") could be batched +// together with a protected branch or a tag to escalate into full repository write. +func TestPreReceiveCanWriteCodePerBranch(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + baseRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10}) + headRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 11}) + require.NoError(t, baseRepo.LoadOwner(t.Context())) + require.NoError(t, headRepo.LoadOwner(t.Context())) + + // An open PR from the head repo owner, with maintainer edits allowed: this grants the base + // repo owner write access to exactly this head branch and nothing else. + pr := &issues_model.PullRequest{ + Issue: &issues_model.Issue{ + RepoID: baseRepo.ID, + PosterID: headRepo.OwnerID, + }, + HeadRepoID: headRepo.ID, + BaseRepoID: baseRepo.ID, + HeadBranch: "granted-branch", + BaseBranch: "master", + AllowMaintainerEdit: true, + } + require.NoError(t, issues_model.NewPullRequest(t.Context(), baseRepo, pr.Issue, nil, nil, pr)) + + // The pusher is the base repo owner (the maintainer) with only read access on the head repo. + maintainer := baseRepo.Owner + headPerm, err := access.GetIndividualUserRepoPermission(t.Context(), headRepo, maintainer) + require.NoError(t, err) + + mockCtx, _ := contexttest.MockPrivateContext(t, "/") + ctx := &preReceiveContext{ + PrivateContext: mockCtx, + loadedPusher: true, + user: maintainer, + userPerm: headPerm, + } + + // The granted branch must be writable... + assert.True(t, ctx.canWriteCodeRef(git.RefNameFromBranch("granted-branch"))) + + // ...but another branch in the same push must NOT inherit that grant. + assert.False(t, ctx.canWriteCodeRef(git.RefNameFromBranch("master"))) + + // ...and a tag sharing the granted branch's name must NOT inherit it either: the grant is + // scoped to PR head branches, so a non-branch ref can never match it. (A tag ref already + // yields an empty branch name, so this guards the per-ref evaluation, not the IsBranch check.) + assert.False(t, ctx.canWriteCodeRef(git.RefNameFromTag("granted-branch"))) +} diff --git a/routers/web/auth/2fa.go b/routers/web/auth/2fa.go index 458a1d71d0..a60ff8d228 100644 --- a/routers/web/auth/2fa.go +++ b/routers/web/auth/2fa.go @@ -58,14 +58,14 @@ func TwoFactorPost(ctx *context.Context) { return } - // Validate the passcode with the stored TOTP secret. - ok, err := twofa.ValidateTOTP(form.Passcode) + // Validate the passcode and atomically consume it to prevent reuse/replay. + ok, err := twofa.ValidateAndConsumeTOTP(ctx, form.Passcode) if err != nil { ctx.ServerError("UserSignIn", err) return } - if ok && twofa.LastUsedPasscode != form.Passcode { + if ok { remember := ctx.Session.Get("twofaRemember").(bool) u, err := user_model.GetUserByID(ctx, id) if err != nil { @@ -81,12 +81,6 @@ func TwoFactorPost(ctx *context.Context) { } } - twofa.LastUsedPasscode = form.Passcode - if err = auth.UpdateTwoFactor(ctx, twofa); err != nil { - ctx.ServerError("UserSignIn", err) - return - } - _ = ctx.Session.Set(session.KeyUserHasTwoFactorAuth, true) handleSignIn(ctx, u, remember) return diff --git a/routers/web/auth/password.go b/routers/web/auth/password.go index 110d870d75..519b22ae94 100644 --- a/routers/web/auth/password.go +++ b/routers/web/auth/password.go @@ -177,23 +177,17 @@ func ResetPasswdPost(ctx *context.Context) { regenerateScratchToken = true } else { passcode := ctx.FormString("passcode") - ok, err := twofa.ValidateTOTP(passcode) + ok, err := twofa.ValidateAndConsumeTOTP(ctx, passcode) if err != nil { - ctx.HTTPError(http.StatusInternalServerError, "ValidateTOTP", err.Error()) + ctx.HTTPError(http.StatusInternalServerError, "ValidateAndConsumeTOTP", err.Error()) return } - if !ok || twofa.LastUsedPasscode == passcode { + if !ok { ctx.Data["IsResetForm"] = true ctx.Data["Err_Passcode"] = true ctx.RenderWithErrDeprecated(ctx.Tr("auth.twofa_passcode_incorrect"), tplResetPassword, nil) return } - - twofa.LastUsedPasscode = passcode - if err = auth.UpdateTwoFactor(ctx, twofa); err != nil { - ctx.ServerError("ResetPasswdPost: UpdateTwoFactor", err) - return - } } } diff --git a/routers/web/repo/branch.go b/routers/web/repo/branch.go index 62967fae53..4b0b4e491b 100644 --- a/routers/web/repo/branch.go +++ b/routers/web/repo/branch.go @@ -261,7 +261,7 @@ func MergeUpstream(ctx *context.Context) { branchName := ctx.FormString("branch") _, err := repo_service.MergeUpstream(ctx, ctx.Doer, ctx.Repo.Repository, branchName, false) if err != nil { - if errors.Is(err, util.ErrNotExist) { + if errors.Is(err, util.ErrNotExist) || errors.Is(err, util.ErrPermissionDenied) { ctx.JSONErrorNotFound() return } else if pull_service.IsErrMergeConflicts(err) { diff --git a/services/auth/basic.go b/services/auth/basic.go index 5b4ba6862c..9166a3303d 100644 --- a/services/auth/basic.go +++ b/services/auth/basic.go @@ -176,7 +176,8 @@ func validateTOTP(req *http.Request, u *user_model.User) error { } return err } - if ok, err := twofa.ValidateTOTP(req.Header.Get("X-Gitea-OTP")); err != nil { + // Consume the passcode atomically so a captured OTP cannot be replayed within its validity window. + if ok, err := twofa.ValidateAndConsumeTOTP(req.Context(), req.Header.Get("X-Gitea-OTP")); err != nil { return err } else if !ok { return util.NewInvalidArgumentErrorf("invalid provided OTP") diff --git a/services/repository/merge_upstream.go b/services/repository/merge_upstream.go index 57e54e89fc..fac84af405 100644 --- a/services/repository/merge_upstream.go +++ b/services/repository/merge_upstream.go @@ -8,7 +8,9 @@ import ( "fmt" issue_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/issues" + access_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/perm/access" repo_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/repo" + "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/unit" user_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/user" "code.mokoconsulting.tech/MokoConsulting/MokoGitea/modules/git" "code.mokoconsulting.tech/MokoConsulting/MokoGitea/modules/gitrepo" @@ -26,6 +28,17 @@ func MergeUpstream(ctx reqctx.RequestContext, doer *user_model.User, repo *repo_ if err = repo.GetBaseRepo(ctx); err != nil { return "", err } + + // The doer must still be able to read the base repository's code. Otherwise a fork created + // while the base repo was public could keep pulling commits after it turned private. + basePerm, err := access_model.GetDoerRepoPermission(ctx, repo.BaseRepo, doer) + if err != nil { + return "", err + } + if !basePerm.CanRead(unit.TypeCode) { + return "", util.NewPermissionDeniedErrorf("permission denied to read base repo %d", repo.BaseRepo.ID) + } + divergingInfo, err := GetUpstreamDivergingInfo(ctx, repo, branch) if err != nil { return "", err diff --git a/tests/integration/api_org_test.go b/tests/integration/api_org_test.go index bf210ea9f9..cad439c492 100644 --- a/tests/integration/api_org_test.go +++ b/tests/integration/api_org_test.go @@ -11,6 +11,7 @@ import ( "time" auth_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/auth" + issues_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/issues" org_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/organization" "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/perm" repo_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/repo" @@ -263,6 +264,7 @@ func testAPIOrgGeneral(t *testing.T) { }) } +<<<<<<< HEAD func testAPIDeleteOrgRepos(t *testing.T) { org3 := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "org3"}) orgRepos, err := repo_model.GetOrgRepositories(t.Context(), org3.ID) @@ -289,6 +291,47 @@ func testAPIDeleteOrgRepos(t *testing.T) { }, 2*time.Second, 50*time.Millisecond) req = NewRequest(t, "DELETE", fmt.Sprintf("/api/v1/orgs/%s/repos", org3.Name)).AddTokenAuth(token) - MakeRequest(t, req, http.StatusNoContent) // The org contains no repositories, so the API should return StatusNoContent + MakeRequest(t, req, http.StatusNoContent) + }) +} + +func TestAPIOrgLabelsVisibility(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + privateOrg := unittest.AssertExistsAndLoadBean(t, &org_model.Organization{ID: 23}) + label := &issues_model.Label{OrgID: privateOrg.ID, Name: "internal-label", Color: "#aabbcc", Description: "private organization label"} + require.NoError(t, issues_model.NewLabel(t.Context(), label)) + + listURL := fmt.Sprintf("/api/v1/orgs/%s/labels", privateOrg.Name) + getURL := fmt.Sprintf("/api/v1/orgs/%s/labels/%d", privateOrg.Name, label.ID) + + t.Run("NonMemberDenied", func(t *testing.T) { + token := getUserToken(t, "user2", auth_model.AccessTokenScopeReadOrganization) + MakeRequest(t, NewRequest(t, "GET", listURL).AddTokenAuth(token), http.StatusNotFound) + MakeRequest(t, NewRequest(t, "GET", getURL).AddTokenAuth(token), http.StatusNotFound) + }) + + t.Run("AnonymousDenied", func(t *testing.T) { + MakeRequest(t, NewRequest(t, "GET", listURL), http.StatusNotFound) + MakeRequest(t, NewRequest(t, "GET", getURL), http.StatusNotFound) + }) + + t.Run("MemberAllowed", func(t *testing.T) { + token := getUserToken(t, "user5", auth_model.AccessTokenScopeReadOrganization) + resp := MakeRequest(t, NewRequest(t, "GET", listURL).AddTokenAuth(token), http.StatusOK) + labels := DecodeJSON(t, resp, &[]*api.Label{}) + assert.Len(t, *labels, 1) + MakeRequest(t, NewRequest(t, "GET", getURL).AddTokenAuth(token), http.StatusOK) + }) + + t.Run("SiteAdminAllowed", func(t *testing.T) { + token := getUserToken(t, "user1", auth_model.AccessTokenScopeReadOrganization) + MakeRequest(t, NewRequest(t, "GET", listURL).AddTokenAuth(token), http.StatusOK) + MakeRequest(t, NewRequest(t, "GET", getURL).AddTokenAuth(token), http.StatusOK) + }) + + t.Run("PublicOrgStillReadable", func(t *testing.T) { + token := getUserToken(t, "user2", auth_model.AccessTokenScopeReadOrganization) + MakeRequest(t, NewRequest(t, "GET", "/api/v1/orgs/org3/labels").AddTokenAuth(token), http.StatusOK) }) } diff --git a/tests/integration/api_twofa_test.go b/tests/integration/api_twofa_test.go index 30660f7cc6..4a450afbcc 100644 --- a/tests/integration/api_twofa_test.go +++ b/tests/integration/api_twofa_test.go @@ -51,6 +51,12 @@ func TestAPITwoFactor(t *testing.T) { AddBasicAuth(user.Name) req.Header.Set("X-Gitea-OTP", passcode) MakeRequest(t, req, http.StatusOK) + + // the same passcode must not be replayable on the basic-auth surface (RFC 6238 single-use) + req = NewRequest(t, "GET", "/api/v1/user"). + AddBasicAuth(user.Name) + req.Header.Set("X-Gitea-OTP", passcode) + MakeRequest(t, req, http.StatusUnauthorized) } func TestBasicAuthWithWebAuthn(t *testing.T) { diff --git a/tests/integration/repo_merge_upstream_test.go b/tests/integration/repo_merge_upstream_test.go index 09b1ade19d..f56fe7aa13 100644 --- a/tests/integration/repo_merge_upstream_test.go +++ b/tests/integration/repo_merge_upstream_test.go @@ -171,5 +171,18 @@ func TestRepoMergeUpstream(t *testing.T) { }).AddTokenAuth(token) MakeRequest(t, req, http.StatusBadRequest) }) + + t.Run("BasePrivateBlocksSync", func(t *testing.T) { + // add a new commit to the base repo, then make the base repo private + require.NoError(t, createOrReplaceFileInBranch(baseUser, baseRepo, "secret.txt", "master", "private-content")) + baseRepo.IsPrivate = true + _, err := db.GetEngine(t.Context()).ID(baseRepo.ID).Cols("is_private").Update(baseRepo) + require.NoError(t, err) + // the fork owner can no longer read the base repo, so syncing must be refused + req = NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/repos/%s/test-repo-fork/merge-upstream", forkUser.Name), &api.MergeUpstreamRequest{ + Branch: "fork-branch", + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusForbidden) + }) }) } -- 2.52.0 From 94590bc8344c8c6730297b78122932b037847dcf Mon Sep 17 00:00:00 2001 From: bircni Date: Wed, 17 Jun 2026 23:21:05 +0200 Subject: [PATCH 10/17] fix: allow git clone of private repos with anonymous code access (#38074) (#38146) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Backport #38074 Fixes #38062. Private repositories with a code unit configured for **anonymous read access** (Settings → Public Access → Code: anonymous view) could not be cloned without credentials. The git HTTP auth gate (`httpBase`) only bypassed authentication for non-private repos, ignoring the per-unit anonymous access setting entirely. - Check anonymous permissions via `access_model.GetDoerRepoPermission(ctx, repo, nil)` + `CanAccess` before requiring auth on pull operations, so the per-unit `AnonymousAccessMode` is respected through the existing permission model - This also correctly handles `setting.Repository.ForcePrivate` (which the naive direct-field check would have missed) - Push (receive-pack) and `RequireSignInViewStrict` continue to require credentials as before Co-authored-by: wxiaoguang --- routers/web/repo/githttp.go | 47 ++++++++++-------------- tests/integration/git_smart_http_test.go | 34 +++++++++++++++++ 2 files changed, 53 insertions(+), 28 deletions(-) diff --git a/routers/web/repo/githttp.go b/routers/web/repo/githttp.go index 4093f62005..6ee6980615 100644 --- a/routers/web/repo/githttp.go +++ b/routers/web/repo/githttp.go @@ -58,8 +58,6 @@ func CorsHandler() func(next http.Handler) http.Handler { // httpBase does the common work for git http services, // including early response, authentication, repository lookup and permission check. func httpBase(ctx *context.Context, optGitService ...string) *serviceHandler { - reponame := strings.TrimSuffix(ctx.PathParam("reponame"), ".git") - if ctx.FormString("go-get") == "1" { context.EarlyResponseForGoGetMeta(ctx) return nil @@ -93,11 +91,11 @@ func httpBase(ctx *context.Context, optGitService ...string) *serviceHandler { isWiki := false unitType := unit.TypeCode - - if strings.HasSuffix(reponame, ".wiki") { + repoName := strings.TrimSuffix(ctx.PathParam("reponame"), ".git") + if strings.HasSuffix(repoName, ".wiki") { isWiki = true unitType = unit.TypeWiki - reponame = reponame[:len(reponame)-5] + repoName = repoName[:len(repoName)-5] } owner := ctx.ContextUser @@ -107,14 +105,14 @@ func httpBase(ctx *context.Context, optGitService ...string) *serviceHandler { } repoExist := true - repo, err := repo_model.GetRepositoryByName(ctx, owner.ID, reponame) + repo, err := repo_model.GetRepositoryByName(ctx, owner.ID, repoName) if err != nil { if !repo_model.IsErrRepoNotExist(err) { ctx.ServerError("GetRepositoryByName", err) return nil } - if redirectRepoID, err := repo_model.LookupRedirect(ctx, owner.ID, reponame); err == nil { + if redirectRepoID, err := repo_model.LookupRedirect(ctx, owner.ID, repoName); err == nil { context.RedirectToRepo(ctx.Base, redirectRepoID) return nil } @@ -127,31 +125,24 @@ func httpBase(ctx *context.Context, optGitService ...string) *serviceHandler { return nil } - // Only public pull don't need auth. - // For private repos, also allow anonymous pull if the specific unit - // (code or wiki) has AnonymousAccessMode >= Read. - isPublicPull := repoExist && isPull && !repo.IsPrivate - if repoExist && isPull && repo.IsPrivate { - repoUnit := repo.MustGetUnit(ctx, unitType) - if repoUnit.AnonymousAccessMode >= perm.AccessModeRead { - isPublicPull = true + // Only public pulls don't need auth: repo must exist, not require-sign-in + canAnonymousPull := false + if isPull && repoExist && !setting.Service.RequireSignInViewStrict { + if owner.Visibility == structs.VisibleTypePublic && !repo.IsPrivate { + canAnonymousPull = true } - } - askAuth := !isPublicPull || setting.Service.RequireSignInViewStrict - - // don't allow anonymous pulls if organization is not public - if isPublicPull { - if err := repo.LoadOwner(ctx); err != nil { - ctx.ServerError("LoadOwner", err) - return nil + if !canAnonymousPull && ctx.Doer == nil { + anonPerm, err := access_model.GetDoerRepoPermission(ctx, repo, nil) + if err != nil { + ctx.ServerError("GetDoerRepoPermission", err) + return nil + } + canAnonymousPull = anonPerm.CanAccess(accessMode, unitType) } - - askAuth = askAuth || (repo.Owner.Visibility != structs.VisibleTypePublic) } // check access - if askAuth { - // rely on the results of Contexter + if !canAnonymousPull { // not public pull, then either the pull needs auth, or the push needs "write" permission, so ask auth if !ctx.IsSigned { // TODO: support digit auth - which would be Authorization header with digit if setting.OAuth2.Enabled { @@ -237,7 +228,7 @@ func httpBase(ctx *context.Context, optGitService ...string) *serviceHandler { return nil } - repo, err = repo_service.PushCreateRepo(ctx, ctx.Doer, owner, reponame) + repo, err = repo_service.PushCreateRepo(ctx, ctx.Doer, owner, repoName) if err != nil { log.Error("pushCreateRepo: %v", err) ctx.Status(http.StatusNotFound) diff --git a/tests/integration/git_smart_http_test.go b/tests/integration/git_smart_http_test.go index 3210cb052c..dde56fb4d2 100644 --- a/tests/integration/git_smart_http_test.go +++ b/tests/integration/git_smart_http_test.go @@ -10,7 +10,9 @@ import ( "testing" auth_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/auth" + "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/perm" repo_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/repo" + "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/unit" "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/unittest" "code.mokoconsulting.tech/MokoConsulting/MokoGitea/modules/setting" "code.mokoconsulting.tech/MokoConsulting/MokoGitea/modules/test" @@ -26,6 +28,8 @@ func TestGitSmartHTTP(t *testing.T) { testGitSmartHTTPTokenScopes(t) testRenamedRepoRedirect(t) testGitArchiveRemote(t, u) + t.Run("AnonymousAccess-Repo", func(t *testing.T) { testGitSmartHTTPPrivateRepoAnonymousAccess(t, false) }) + t.Run("AnonymousAccess-Wiki", func(t *testing.T) { testGitSmartHTTPPrivateRepoAnonymousAccess(t, true) }) }) } @@ -144,3 +148,33 @@ func testGitArchiveRemote(t *testing.T, u *url.URL) { t.Run("Fetch HEAD archive subpath", doGitRemoteArchive(u.String(), "HEAD", "test")) t.Run("list compression options", doGitRemoteArchive(u.String(), "--list")) } + +// testGitSmartHTTPPrivateRepoAnonymousAccess tests that a private repo with +// anonymous code access enabled can be cloned without credentials. +func testGitSmartHTTPPrivateRepoAnonymousAccess(t *testing.T, isWiki bool) { + // repo1 (ID=1) belongs to user2 and is public by default in fixtures + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1, OwnerName: "user2", Name: "repo1"}) + unitType := util.Iif(isWiki, unit.TypeWiki, unit.TypeCode) + repoLink := "/" + repo.FullName() + util.Iif(isWiki, ".wiki", "") + gitPullPath := repoLink + "/info/refs?service=git-upload-pack" + gitPushPath := repoLink + "/info/refs?service=git-receive-pack" + + // make the repo private + require.NoError(t, repo_model.UpdateRepositoryColsNoAutoTime(t.Context(), &repo_model.Repository{ID: repo.ID, IsPrivate: true}, "is_private")) + + // without anonymous access: anonymous pull must require auth + MakeRequest(t, NewRequest(t, "GET", gitPullPath), http.StatusUnauthorized) + + // enable anonymous read access on the unit + require.NoError(t, repo_model.UpdateRepoUnitPublicAccess(t.Context(), &repo_model.RepoUnit{RepoID: repo.ID, Type: unitType, AnonymousAccessMode: perm.AccessModeRead})) + + // with anonymous code access: anonymous pull must succeed without credentials + MakeRequest(t, NewRequest(t, "GET", gitPullPath), http.StatusOK) + + // push (receive-pack) must still require auth even with anonymous code access + MakeRequest(t, NewRequest(t, "GET", gitPushPath), http.StatusUnauthorized) + + // RequireSignInViewStrict must override anonymous access + defer test.MockVariableValue(&setting.Service.RequireSignInViewStrict, true)() + MakeRequest(t, NewRequest(t, "GET", gitPullPath), http.StatusUnauthorized) +} -- 2.52.0 From 0dc858c15cd0929faefa6caaad7d06bdcf8b41f5 Mon Sep 17 00:00:00 2001 From: Giteabot Date: Fri, 19 Jun 2026 14:50:01 -0700 Subject: [PATCH 11/17] fix(hostmacher): patch incorrect private list (#38170) (#38173) Backport #38170 by @TheFox0x7 regression from #38039 Co-authored-by: TheFox0x7 --- modules/hostmatcher/hostmatcher.go | 21 +++++++++++---------- modules/hostmatcher/hostmatcher_test.go | 4 +++- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/modules/hostmatcher/hostmatcher.go b/modules/hostmatcher/hostmatcher.go index 7c17bc95da..c82a57b7b9 100644 --- a/modules/hostmatcher/hostmatcher.go +++ b/modules/hostmatcher/hostmatcher.go @@ -65,12 +65,9 @@ var reservedIPNets = sync.OnceValue(func() []*net.IPNet { return nets }) -// isPrivateIP reports whether ip falls in a private (net.IP.IsPrivate) or reserved special-purpose +// isReservedIP reports whether ip falls in reserved special-purpose // range (see reservedIPNets) that must not be considered a public/external destination. -func isPrivateIP(ip net.IP) bool { - if ip.IsPrivate() { - return true - } +func isReservedIP(ip net.IP) bool { for _, ipNet := range reservedIPNets() { if ipNet.Contains(ip) { return true @@ -148,18 +145,22 @@ func (hl *HostMatchList) checkPattern(host string) bool { return false } -func (hl *HostMatchList) checkIP(ip net.IP) bool { +// matchesIP determines if the given IP matches any of the configured rules +func (hl *HostMatchList) matchesIP(ip net.IP) bool { if slices.Contains(hl.patterns, "*") { return true } for _, builtin := range hl.builtins { switch builtin { case MatchBuiltinExternal: - if ip.IsGlobalUnicast() && !isPrivateIP(ip) { + // External address must be a global unicast, must not be in reserved range and must not be in private range + if ip.IsGlobalUnicast() && !isReservedIP(ip) && !ip.IsPrivate() { return true } case MatchBuiltinPrivate: - if isPrivateIP(ip) { + // Private address must be global unicast, must not be in range we explicitly exclude for security reasons + // and must be in private range + if ip.IsGlobalUnicast() && !isReservedIP(ip) && ip.IsPrivate() { return true } case MatchBuiltinLoopback: @@ -190,7 +191,7 @@ func (hl *HostMatchList) MatchHostName(host string) bool { return true } if ip := net.ParseIP(hostname); ip != nil { - return hl.checkIP(ip) + return hl.matchesIP(ip) } return false } @@ -201,7 +202,7 @@ func (hl *HostMatchList) MatchIPAddr(ip net.IP) bool { return false } host := ip.String() // nil-safe, we will get "" if ip is nil - return hl.checkPattern(host) || hl.checkIP(ip) + return hl.checkPattern(host) || hl.matchesIP(ip) } // MatchHostOrIP checks if the host or IP matches an allow/deny(block) list diff --git a/modules/hostmatcher/hostmatcher_test.go b/modules/hostmatcher/hostmatcher_test.go index 61582f28d3..464354ff41 100644 --- a/modules/hostmatcher/hostmatcher_test.go +++ b/modules/hostmatcher/hostmatcher_test.go @@ -202,15 +202,17 @@ func TestReservedRanges(t *testing.T) { "198.18.0.1", // benchmarking "198.51.100.1", // TEST-NET-2 "203.0.113.1", // TEST-NET-3 + "169.254.169.254", // Cloud metadata "192.88.99.1", // 6to4 relay anycast "64:ff9b::1", // NAT64 "64:ff9b::a9fe:a9fe", // NAT64 embedding 169.254.169.254 "2001::1", // Teredo "2002::1", // 6to4 "2001:db8::1", // documentation + "fe80::1", // link local address } { addr := net.ParseIP(ip) assert.Falsef(t, external.MatchIPAddr(addr), "reserved ip %s must not be external", ip) - assert.Truef(t, private.MatchIPAddr(addr), "reserved ip %s should match private block-list", ip) + assert.Falsef(t, private.MatchIPAddr(addr), "reserved ip %s should match private block-list", ip) } } -- 2.52.0 From c0f89a373d0518a2e8b032af582a236eeedfe24d Mon Sep 17 00:00:00 2001 From: bircni Date: Sun, 21 Jun 2026 14:23:24 +0200 Subject: [PATCH 12/17] fix(auth): do not auto-reactivate disabled users on OAuth2 callback (#38009) (#38183) Backport #38009 The OAuth2 sign-in callback unconditionally set IsActive=true on the local user row whenever the IdP authenticated them, silently undoing an administrator's "Disable Account" action and granting the user a fresh session in the same response. Treat the local IsActive flag as an authoritative admin override: inactive users get a session and are routed through the existing activate / prohibit-login pages by verifyAuthWithOptions, matching the local-credentials sign-in path. Adds an integration regression test that disables a linked local user and asserts the row stays IsActive=false after a full OIDC callback. Co-authored-by: wxiaoguang --- models/unittest/unit_tests.go | 2 +- routers/web/auth/oauth.go | 16 ++- services/auth/source/oauth2/source_sync.go | 4 +- tests/integration/auth_oauth2_test.go | 113 +++++++++++++++++++++ 4 files changed, 130 insertions(+), 5 deletions(-) diff --git a/models/unittest/unit_tests.go b/models/unittest/unit_tests.go index 481b6d37c6..e1012675be 100644 --- a/models/unittest/unit_tests.go +++ b/models/unittest/unit_tests.go @@ -47,7 +47,7 @@ func OrderBy(orderBy string) any { } func whereOrderConditions(e db.Engine, conditions []any) db.Engine { - orderBy := "id" // query must have the "ORDER BY", otherwise the result is not deterministic + orderBy := "id" // query must have the "ORDER BY", otherwise the result is not deterministic. FIXME: some tables do not have "id" column for _, condition := range conditions { switch cond := condition.(type) { case *testCond: diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index 43d2faffa5..bff07b9ee7 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -368,9 +368,21 @@ func handleOAuth2SignIn(ctx *context.Context, authSource *auth.Source, u *user_m opts := &user_service.UpdateOptions{} - // Reactivate user if they are deactivated + // HINT: OAUTH-AUTO-SYNC-USER-ACTIVATION: see services/auth/source/oauth2/source_sync.go + // Reactivate user only if they were disabled by the OAuth2 auto sync cron (invalid_grant), + // which clears AccessToken/RefreshToken/ExpiresAt on the ExternalLoginUser row + // An admin-disabled user has no such signature, so we leave IsActive alone + // and let verifyAuthWithOptions route them through the prohibit-login / activate page. if !u.IsActive { - opts.IsActive = optional.Some(true) + extLogin, hasExt, err := user_model.GetExternalLogin(ctx, authSource.ID, gothUser.UserID) + if err != nil { + ctx.ServerError("GetExternalLogin", err) + return + } + isDisabledByAutoSync := hasExt && extLogin.RefreshToken == "" + if isDisabledByAutoSync { + opts.IsActive = optional.Some(true) + } } // Update GroupClaims diff --git a/services/auth/source/oauth2/source_sync.go b/services/auth/source/oauth2/source_sync.go index 8fdf7b90f1..3c5b1b62a9 100644 --- a/services/auth/source/oauth2/source_sync.go +++ b/services/auth/source/oauth2/source_sync.go @@ -88,8 +88,8 @@ func (source *Source) refresh(ctx context.Context, provider goth.Provider, u *us } } - // Delete stored tokens, since they are invalid. This - // also provents us from checking this in subsequent runs. + // HINT: OAUTH-AUTO-SYNC-USER-ACTIVATION + // Delete stored tokens, since they are invalid. This also prevents us from checking this in subsequent runs. u.AccessToken = "" u.RefreshToken = "" u.ExpiresAt = time.Time{} diff --git a/tests/integration/auth_oauth2_test.go b/tests/integration/auth_oauth2_test.go index 856e2e95e9..e6a032d803 100644 --- a/tests/integration/auth_oauth2_test.go +++ b/tests/integration/auth_oauth2_test.go @@ -13,6 +13,7 @@ import ( "time" auth_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/auth" + "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/db" "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/unittest" user_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/user" "code.mokoconsulting.tech/MokoConsulting/MokoGitea/modules/json" @@ -23,6 +24,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "xorm.io/builder" ) // TestMigrateAzureADV2ToOIDC simulates a login source migration from the Azure AD V2 OAuth2 provider to the OpenID Connect provider, @@ -244,6 +246,117 @@ func newFakeOIDCServerWithProfile(t *testing.T, sub, oid, email, name string) *h return srv } +// TestOAuth2CallbackReactivationGating exercises the gate in handleOAuth2SignIn: +// an inactive user can only be reactivated when who was disabled by auto-sync +func TestOAuth2CallbackReactivationGating(t *testing.T) { + defer tests.PrepareTestEnv(t)() + defer test.MockVariableValue(&setting.OAuth2Client.EnableAutoRegistration, true)() + defer test.MockVariableValue(&setting.OAuth2Client.Username, setting.OAuth2UsernameUserid)() + + srv := newFakeOIDCServer(t, FakeOIDCConfig{Sub: "test-sub", Email: "test@example.com", Name: "Test User"}) + addOAuth2Source(t, "test-oauth-source", oauth2.Source{ + Provider: "openidConnect", + ClientID: "test-client-id", + ClientSecret: "test-client-secret", + OpenIDConnectAutoDiscoveryURL: srv.URL + "/.well-known/openid-configuration", + }) + authSource, err := auth_model.GetActiveOAuth2SourceByAuthName(t.Context(), "test-oauth-source") + require.NoError(t, err) + + u := &user_model.User{Name: "test-user", Email: "test@example.com"} + require.NoError(t, user_model.CreateUser(t.Context(), u, &user_model.Meta{})) + + extLink := &user_model.ExternalLoginUser{ + UserID: u.ID, + LoginSourceID: authSource.ID, + Provider: authSource.Name, + ExternalID: "test-sub", + } + require.NoError(t, user_model.LinkExternalToUser(t.Context(), u, extLink)) + + prepareUserExternalLink := func(t *testing.T, refreshToken string) { + err := user_model.UpdateUserCols(t.Context(), &user_model.User{ID: u.ID, IsActive: false}, "is_active") + require.NoError(t, err) + _, err = db.GetEngine(t.Context()).Where(builder.Eq{"user_id": u.ID}).Cols("refresh_token"). + Update(&user_model.ExternalLoginUser{RefreshToken: refreshToken}) + require.NoError(t, err) + require.False(t, unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: u.ID}).IsActive) + } + + t.Run("admin-disabled user is not reactivated", func(t *testing.T) { + prepareUserExternalLink(t, "non-empty-refresh-token") + doOIDCSignIn(t, authSource.Name) + after := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: u.ID}) + assert.False(t, after.IsActive, "OAuth callback must not re-enable an administrator-disabled account") + }) + + t.Run("auto-sync-disabled user is reactivated", func(t *testing.T) { + prepareUserExternalLink(t, "" /* empty refresh token */) + doOIDCSignIn(t, authSource.Name) + after := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: u.ID}) + assert.True(t, after.IsActive, "OAuth callback must reactivate a sync-disabled account on successful login") + }) +} + +// FakeOIDCConfig holds configuration for the fake OIDC server used in tests. +type FakeOIDCConfig struct { + Sub string + OID string + Email string + Name string + Groups []string +} + +// newFakeOIDCServer starts a httptest.Server that implements the minimum OIDC endpoints needed to complete a sign-in flow +func newFakeOIDCServer(t *testing.T, cfg FakeOIDCConfig) *httptest.Server { + t.Helper() + + var srv *httptest.Server + srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/.well-known/openid-configuration": // discovery document + _ = json.NewEncoder(w).Encode(map[string]string{ + "issuer": srv.URL, + "authorization_endpoint": srv.URL + "/authorize", + "token_endpoint": srv.URL + "/token", + "userinfo_endpoint": srv.URL + "/userinfo", + }) + case "/token": // returns an ID token with both "sub" and "oid" claims so tests can verify which one ends up as ExternalID + claims := map[string]any{ + "iss": srv.URL, + "aud": "test-client-id", + "exp": time.Now().Add(time.Hour).Unix(), + "sub": cfg.Sub, + "oid": cfg.OID, + } + payload, _ := json.Marshal(claims) + header := base64.RawURLEncoding.EncodeToString([]byte(`{"alg":"none"}`)) + + // build a JWT-shaped string whose payload encodes claims. + // goth's decodeJWT only base64-decodes the payload without verifying the signature, so no real signing infrastructure is needed. + idToken := header + "." + base64.RawURLEncoding.EncodeToString(payload) + ".fakesig" + + _ = json.NewEncoder(w).Encode(map[string]any{ + "access_token": "fake-access-token", + "token_type": "Bearer", + "id_token": idToken, + }) + case "/userinfo": + // sub MUST match the id_token sub; goth rejects mismatches. + _ = json.NewEncoder(w).Encode(map[string]any{ + "sub": cfg.Sub, + "email": cfg.Email, + "name": cfg.Name, + }) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(srv.Close) + return srv +} + // doOIDCSignIn runs a mock OIDC sign-in flow for the given auth source. func doOIDCSignIn(t *testing.T, sourceName string) { t.Helper() -- 2.52.0 From 2ff0e4aa21b22eb54fa34a90bcc1945875217ab4 Mon Sep 17 00:00:00 2001 From: Giteabot Date: Sun, 21 Jun 2026 05:54:31 -0700 Subject: [PATCH 13/17] fix: walk git log context error handling (#38182) (#38185) Backport #38182 Fix #38177 Make WalkGitLog can handle EOF and context errors correctly, and don't export these private functions & methods & structs. Co-authored-by: wxiaoguang --- modules/git/commit_info_nogogit.go | 2 +- modules/git/commit_info_nogogit_test.go | 54 ++++++++ modules/git/last_commit_cache_nogogit.go | 2 +- ...e_status.go => log_name_status_nogogit.go} | 126 +++++++----------- 4 files changed, 104 insertions(+), 80 deletions(-) create mode 100644 modules/git/commit_info_nogogit_test.go rename modules/git/{log_name_status.go => log_name_status_nogogit.go} (74%) diff --git a/modules/git/commit_info_nogogit.go b/modules/git/commit_info_nogogit.go index faa0af3e65..c7c4d3abd9 100644 --- a/modules/git/commit_info_nogogit.go +++ b/modules/git/commit_info_nogogit.go @@ -106,7 +106,7 @@ func getLastCommitForPathsByCache(commitID, treePath string, paths []string, cac // GetLastCommitForPaths returns last commit information func GetLastCommitForPaths(ctx context.Context, commit *Commit, treePath string, paths []string) (map[string]*Commit, error) { // We read backwards from the commit to obtain all of the commits - revs, err := WalkGitLog(ctx, commit.repo, commit, treePath, paths...) + revs, err := walkGitLog(ctx, commit.repo, commit, treePath, paths...) if err != nil { return nil, err } diff --git a/modules/git/commit_info_nogogit_test.go b/modules/git/commit_info_nogogit_test.go new file mode 100644 index 0000000000..2080c8281b --- /dev/null +++ b/modules/git/commit_info_nogogit_test.go @@ -0,0 +1,54 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +//go:build !gogit + +package git + +import ( + "context" + "path/filepath" + "testing" + + "code.gitea.io/gitea/modules/test" + "code.gitea.io/gitea/modules/util" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestEntries_GetCommitsInfo_ContextErr(t *testing.T) { + repo, err := OpenRepository(t.Context(), filepath.Join(testReposDir, "repo1_bare")) + require.NoError(t, err) + defer repo.Close() + + commit, err := repo.GetCommit("feaf4ba6bc635fec442f46ddd4512416ec43c2c2") + require.NoError(t, err) + entries, err := commit.Tree.ListEntries() + require.NoError(t, err) + + countCommitInfosCommit := func(infos []CommitInfo) (nilCommits, nonNilCommits int) { + for _, info := range infos { + nilCommits += util.Iif(info.Commit == nil, 1, 0) + nonNilCommits += util.Iif(info.Commit != nil, 1, 0) + } + return nilCommits, nonNilCommits + } + + ctx, cancel := context.WithCancel(t.Context()) + defer test.MockVariableValue(&walkGitLogDebugBeforeNext)() + + walkGitLogDebugBeforeNext = cancel + commitInfos, _, err := entries.GetCommitsInfo(ctx, "/any/repo-link", commit, "") + assert.NoError(t, err) + nilCommits, nonNilCommits := countCommitInfosCommit(commitInfos) + assert.Equal(t, 0, nonNilCommits) // no commit info due to canceled (or deadline-exceeded) context + assert.Equal(t, 3, nilCommits) + + walkGitLogDebugBeforeNext = nil + commitInfos, _, err = entries.GetCommitsInfo(t.Context(), "/any/repo-link", commit, "") + assert.NoError(t, err) + nilCommits, nonNilCommits = countCommitInfosCommit(commitInfos) + assert.Equal(t, 3, nonNilCommits) + assert.Equal(t, 0, nilCommits) +} diff --git a/modules/git/last_commit_cache_nogogit.go b/modules/git/last_commit_cache_nogogit.go index 155cb3cb7c..e034eaa168 100644 --- a/modules/git/last_commit_cache_nogogit.go +++ b/modules/git/last_commit_cache_nogogit.go @@ -32,7 +32,7 @@ func (c *Commit) recursiveCache(ctx context.Context, tree *Tree, treePath string entryPaths[i] = entry.Name() } - _, err = WalkGitLog(ctx, c.repo, c, treePath, entryPaths...) + _, err = walkGitLog(ctx, c.repo, c, treePath, entryPaths...) if err != nil { return err } diff --git a/modules/git/log_name_status.go b/modules/git/log_name_status_nogogit.go similarity index 74% rename from modules/git/log_name_status.go rename to modules/git/log_name_status_nogogit.go index 24d89a16e5..82532c51ea 100644 --- a/modules/git/log_name_status.go +++ b/modules/git/log_name_status_nogogit.go @@ -1,6 +1,8 @@ // Copyright 2021 The Gitea Authors. All rights reserved. // SPDX-License-Identifier: MIT +//go:build !gogit + package git import ( @@ -18,10 +20,8 @@ import ( "code.mokoconsulting.tech/MokoConsulting/MokoGitea/modules/log" ) -// LogNameStatusRepo opens git log --raw in the provided repo and returns a stdin pipe, a stdout reader and cancel function -func LogNameStatusRepo(ctx context.Context, repository, head, treepath string, paths ...string) (*bufio.Reader, func()) { - // Lets also create a context so that we can absolutely ensure that the command should die when we're done - +// logNameStatusRepo opens git log --raw in the provided repo and returns a parser +func logNameStatusRepo(ctx context.Context, repository, head, treepath string, paths ...string) *logNameStatusRepoParser { cmd := gitcmd.NewCommand() cmd.AddArguments("log", "--name-status", "-c", "--format=commit%x00%H %P%x00", "--parents", "--no-renames", "-t", "-z").AddDynamicArguments(head) @@ -54,77 +54,62 @@ func LogNameStatusRepo(ctx context.Context, repository, head, treepath string, p ctx, ctxCancel := context.WithCancel(ctx) go func() { err := cmd.WithDir(repository).RunWithStderr(ctx) - if err != nil && !errors.Is(err, context.Canceled) { + if err != nil && !errors.Is(err, context.Canceled) && !errors.Is(err, context.DeadlineExceeded) { log.Error("Unable to run git command %v: %v", cmd.LogString(), err) } }() bufReader := bufio.NewReaderSize(stdoutReader, 32*1024) - - return bufReader, func() { - ctxCancel() - stdoutReaderClose() + return &logNameStatusRepoParser{ + treepath: treepath, + paths: paths, + rd: bufReader, + close: func() { + ctxCancel() + stdoutReaderClose() + }, } } -// LogNameStatusRepoParser parses a git log raw output from LogRawRepo -type LogNameStatusRepoParser struct { +// logNameStatusRepoParser parses a git log raw output from LogRawRepo +type logNameStatusRepoParser struct { treepath string paths []string next []byte buffull bool rd *bufio.Reader - cancel func() + close func() } -// NewLogNameStatusRepoParser returns a new parser for a git log raw output -func NewLogNameStatusRepoParser(ctx context.Context, repository, head, treepath string, paths ...string) *LogNameStatusRepoParser { - rd, cancel := LogNameStatusRepo(ctx, repository, head, treepath, paths...) - return &LogNameStatusRepoParser{ - treepath: treepath, - paths: paths, - rd: rd, - cancel: cancel, - } -} - -// LogNameStatusCommitData represents a commit artefact from git log raw -type LogNameStatusCommitData struct { +// logNameStatusCommitData represents a commit artifact from git log raw +type logNameStatusCommitData struct { CommitID string ParentIDs []string Paths []bool } -// Next returns the next LogStatusCommitData -func (g *LogNameStatusRepoParser) Next(treepath string, paths2ids map[string]int, changed []bool, maxpathlen int) (*LogNameStatusCommitData, error) { +// walkNext returns the next LogStatusCommitData +func (g *logNameStatusRepoParser) walkNext(treepath string, paths2ids map[string]int, changed []bool, maxpathlen int) (*logNameStatusCommitData, error) { var err error if len(g.next) == 0 { g.buffull = false g.next, err = g.rd.ReadSlice('\x00') - if err != nil { - switch err { - case bufio.ErrBufferFull: - g.buffull = true - case io.EOF: - return nil, nil //nolint:nilnil // return nil to signal EOF - default: - return nil, err - } + switch { + case errors.Is(err, bufio.ErrBufferFull): + g.buffull = true + case err != nil: + return nil, err } } - ret := LogNameStatusCommitData{} + ret := logNameStatusCommitData{} if bytes.Equal(g.next, []byte("commit\000")) { g.next, err = g.rd.ReadSlice('\x00') - if err != nil { - switch err { - case bufio.ErrBufferFull: - g.buffull = true - case io.EOF: - return nil, nil //nolint:nilnil // return nil to signal EOF - default: - return nil, err - } + switch { + case errors.Is(err, bufio.ErrBufferFull): + g.buffull = true + case err != nil: + return nil, err } } @@ -273,13 +258,10 @@ diffloop: } } -// Close closes the parser -func (g *LogNameStatusRepoParser) Close() { - g.cancel() -} +var walkGitLogDebugBeforeNext func() // is used to simulate various edge git process cases -// WalkGitLog walks the git log --name-status for the head commit in the provided treepath and files -func WalkGitLog(ctx context.Context, repo *Repository, head *Commit, treepath string, paths ...string) (map[string]string, error) { +// walkGitLog walks the git log --name-status for the head commit in the provided treepath and files +func walkGitLog(ctx context.Context, repo *Repository, head *Commit, treepath string, paths ...string) (map[string]string, error) { headRef := head.ID.String() tree, err := head.SubTree(treepath) @@ -322,11 +304,9 @@ func WalkGitLog(ctx context.Context, repo *Repository, head *Commit, treepath st } } - g := NewLogNameStatusRepoParser(ctx, repo.Path, head.ID.String(), treepath, paths...) - // don't use defer g.Close() here as g may change its value - instead wrap in a func - defer func() { - g.Close() - }() + g := logNameStatusRepo(ctx, repo.Path, head.ID.String(), treepath, paths...) + // don't use defer g.cancel() here as g may change its value - instead wrap in a func + defer func() { g.close() }() results := make([]string, len(paths)) remaining := len(paths) @@ -340,25 +320,16 @@ func WalkGitLog(ctx context.Context, repo *Repository, head *Commit, treepath st heaploop: for { - select { - case <-ctx.Done(): - if ctx.Err() == context.DeadlineExceeded { - break heaploop - } - g.Close() - return nil, ctx.Err() - default: + if walkGitLogDebugBeforeNext != nil { + walkGitLogDebugBeforeNext() } - current, err := g.Next(treepath, path2idx, changed, maxpathlen) - if err != nil { - if errors.Is(err, context.DeadlineExceeded) { - break heaploop - } - g.Close() - return nil, err - } - if current == nil { - break heaploop + current, err := g.walkNext(treepath, path2idx, changed, maxpathlen) + if ctx.Err() != nil { + break heaploop // context is either canceled or deadline exceeded - break the loop and return what we have so far + } else if errors.Is(err, io.EOF) { + break heaploop // reached to the end of log output + } else if err != nil { + return nil, err // other unknown errors } parentRemaining.Remove(current.CommitID) for i, found := range current.Paths { @@ -395,14 +366,14 @@ heaploop: if remaining <= nextRestart { commitSinceNextRestart++ if 4*commitSinceNextRestart > 3*commitSinceLastEmptyParent { - g.Close() remainingPaths := make([]string, 0, len(paths)) for i, pth := range paths { if results[i] == "" { remainingPaths = append(remainingPaths, pth) } } - g = NewLogNameStatusRepoParser(ctx, repo.Path, lastEmptyParent, treepath, remainingPaths...) + g.close() + g = logNameStatusRepo(ctx, repo.Path, lastEmptyParent, treepath, remainingPaths...) parentRemaining = make(container.Set[string]) nextRestart = (remaining * 3) / 4 continue heaploop @@ -410,7 +381,6 @@ heaploop: } parentRemaining.AddMultiple(current.ParentIDs...) } - g.Close() resultsMap := map[string]string{} for i, pth := range paths { -- 2.52.0 From b0bbaab621e8058f1cadddbe9cef845c9ab21b66 Mon Sep 17 00:00:00 2001 From: Jonathan Miller Date: Sun, 28 Jun 2026 02:35:07 -0500 Subject: [PATCH 14/17] docs: add security section to changelog for upstream v1.26.3/v1.26.4 fixes Claude-Session: https://claude.ai/code/session_011AAFzotGMf3ayvXhEmStCd --- CHANGELOG.md | 15 +++++++ routers/private/hook_pre_receive.go | 64 +++++++++++++++++------------ 2 files changed, 52 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4db6fd594a..49f3af6b35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,21 @@ - Wiki recent changes page: cross-page edit activity with pagination (#670) - Wiki page rename with automatic redirects via YAML frontmatter (#672) +### Security +- Cherry-pick upstream v1.26.3: LFS reject unknown SSH sub-verbs to prevent auth bypass (#38015) +- Cherry-pick upstream v1.26.3: bound CODEOWNERS regex match time — ReDoS prevention (#38025) +- Cherry-pick upstream v1.26.3: require merged PR to bypass fork PR approval gate (#38041) +- Cherry-pick upstream v1.26.3: LFS require Code-unit access for cross-repo object reuse (#38050) +- Cherry-pick upstream v1.26.3: hostmatcher block reserved IP ranges — SSRF prevention (#38059) +- Cherry-pick upstream v1.26.3: bound debian ParseControlFile — DoS prevention (#38055) +- Cherry-pick upstream v1.26.3: feed token scope, migration SSRF, notification redaction (#38147) +- Cherry-pick upstream v1.26.3: OIDC ignore stale external login links to organizations (#38141) +- Cherry-pick upstream v1.26.3: 2FA timing, branch delete auth, org labels visibility, merge upstream auth (#38151) +- Cherry-pick upstream v1.26.3: allow git clone of private repos with anonymous code access (#38146) +- Cherry-pick upstream v1.26.3: hostmatcher patch incorrect private IP list (#38173) +- Cherry-pick upstream v1.26.4: do not auto-reactivate disabled users on OAuth2 callback (#38183) +- Cherry-pick upstream v1.26.4: walk git log context error handling — regression fix (#38185) + ### Fixed - Cherry-pick upstream v1.26.2: handle empty pull request files view to allow reviews (#37783) - Cherry-pick upstream v1.26.2: fix "run as root" check with snap container detection (#37622) diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index f42ef69274..e7ccff06e8 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -26,6 +26,7 @@ import ( "code.mokoconsulting.tech/MokoConsulting/MokoGitea/services/agit" gitea_context "code.mokoconsulting.tech/MokoConsulting/MokoGitea/services/context" pull_service "code.mokoconsulting.tech/MokoConsulting/MokoGitea/services/pull" + security_service "code.mokoconsulting.tech/MokoConsulting/MokoGitea/services/security" ) type preReceiveContext struct { @@ -40,6 +41,9 @@ type preReceiveContext struct { canCreatePullRequest bool checkedCanCreatePullRequest bool + canWriteCode bool + checkedCanWriteCode bool + protectedTags []*git_model.ProtectedTag gotProtectedTags bool @@ -47,36 +51,24 @@ type preReceiveContext struct { opts *private.HookOptions - // this context should only contain shared variables, mutable variables like "current branch name" shouldn't be put here - canWriteCodeUnitCached *bool + branchName string } -func (ctx *preReceiveContext) canWriteCodeUnit() bool { - if ctx.canWriteCodeUnitCached == nil { - var canWrite bool - if ctx.loadPusherAndPermission() { - canWrite = ctx.userPerm.CanWrite(unit.TypeCode) || ctx.deployKeyAccessMode >= perm_model.AccessModeWrite +// CanWriteCode returns true if pusher can write code +func (ctx *preReceiveContext) CanWriteCode() bool { + if !ctx.checkedCanWriteCode { + if !ctx.loadPusherAndPermission() { + return false } - ctx.canWriteCodeUnitCached = &canWrite + ctx.canWriteCode = issues_model.CanMaintainerWriteToBranch(ctx, ctx.userPerm, ctx.branchName, ctx.user) || ctx.deployKeyAccessMode >= perm_model.AccessModeWrite + ctx.checkedCanWriteCode = true } - return *ctx.canWriteCodeUnitCached + return ctx.canWriteCode } -// canWriteCodeRef returns true if pusher can write to the code ref (branch/tag/commit) -func (ctx *preReceiveContext) canWriteCodeRef(refFullName git.RefName) bool { - if ctx.canWriteCodeUnit() { - return true - } - // then check whether if the pusher is a maintainer who can write the PR author's head repo branch - if !refFullName.IsBranch() { - return false - } - return issues_model.CanMaintainerWriteToBranch(ctx, ctx.userPerm, refFullName.BranchName(), ctx.user) -} - -// assertCanWriteRef returns true if pusher can write to the code ref, otherwise it responds with 403 Forbidden and returns false -func (ctx *preReceiveContext) assertCanWriteRef(refFullName git.RefName) bool { - if !ctx.canWriteCodeRef(refFullName) { +// AssertCanWriteCode returns true if pusher can write code +func (ctx *preReceiveContext) AssertCanWriteCode() bool { + if !ctx.CanWriteCode() { if ctx.Written() { return false } @@ -138,7 +130,7 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) { case git.DefaultFeatures().SupportProcReceive && refFullName.IsFor(): preReceiveFor(ourCtx, refFullName) default: - ourCtx.assertCanWriteRef(refFullName) + ourCtx.AssertCanWriteCode() } if ctx.Written() { return @@ -150,8 +142,9 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) { func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, refFullName git.RefName) { branchName := refFullName.BranchName() + ctx.branchName = branchName - if !ctx.assertCanWriteRef(refFullName) { + if !ctx.AssertCanWriteCode() { return } @@ -159,6 +152,23 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, r gitRepo := ctx.Repo.GitRepo objectFormat := ctx.Repo.GetObjectFormat() + if newCommitID != objectFormat.EmptyObjectID().String() { + newCommit, err := gitRepo.GetCommit(newCommitID) + if err == nil { + if findings := security_service.ScanPushForSecrets(ctx, repo.ID, newCommit); len(findings) > 0 { + msg := fmt.Sprintf("Push rejected: %d secret(s) detected in commit %s", len(findings), newCommitID[:12]) + for _, f := range findings { + msg += fmt.Sprintf("\n - %s in %s:%d", f.Title, f.FilePath, f.LineNumber) + } + log.Warn("Secret scan blocked push to %s in %-v: %d findings", branchName, repo, len(findings)) + ctx.JSON(http.StatusForbidden, private.Response{ + UserMsg: msg, + }) + return + } + } + } + defaultBranch := repo.DefaultBranch if ctx.opts.IsWiki && repo.DefaultWikiBranch != "" { defaultBranch = repo.DefaultWikiBranch @@ -429,7 +439,7 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, r } func preReceiveTag(ctx *preReceiveContext, refFullName git.RefName) { - if !ctx.assertCanWriteRef(refFullName) { + if !ctx.AssertCanWriteCode() { return } -- 2.52.0 From e947600ea7377bfa4f3583f3f019be2ac4e407c3 Mon Sep 17 00:00:00 2001 From: Jonathan Miller Date: Sun, 28 Jun 2026 02:35:11 -0500 Subject: [PATCH 15/17] fix: log error when pre-receive secret scan cannot read commit Previously, GetCommit failures were silently swallowed, allowing pushes to proceed without scanning. Now logs the error so admins can diagnose issues while still allowing the push. Claude-Session: https://claude.ai/code/session_011AAFzotGMf3ayvXhEmStCd --- routers/private/hook_pre_receive.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index e7ccff06e8..0da5d3a65b 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -154,7 +154,9 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, r if newCommitID != objectFormat.EmptyObjectID().String() { newCommit, err := gitRepo.GetCommit(newCommitID) - if err == nil { + if err != nil { + log.Error("Secret scan: failed to get commit %s in %-v: %v", newCommitID[:12], repo, err) + } else { if findings := security_service.ScanPushForSecrets(ctx, repo.ID, newCommit); len(findings) > 0 { msg := fmt.Sprintf("Push rejected: %d secret(s) detected in commit %s", len(findings), newCommitID[:12]) for _, f := range findings { -- 2.52.0 From 4e5aa5f3ce0f341610378b35cdc987b8a5feb4fb Mon Sep 17 00:00:00 2001 From: Jonathan Miller Date: Sun, 28 Jun 2026 02:37:49 -0500 Subject: [PATCH 16/17] fix: revert accidental secret scanning code from security fix branch The pre-receive hook had security scanning code from the wrong feature branch (feature/secret-scanning-clean). Restoring to the correct state with only upstream security cherry-picks. Claude-Session: https://claude.ai/code/session_011AAFzotGMf3ayvXhEmStCd --- routers/private/hook_pre_receive.go | 66 ++++++++++++----------------- 1 file changed, 27 insertions(+), 39 deletions(-) diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index 0da5d3a65b..f42ef69274 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -26,7 +26,6 @@ import ( "code.mokoconsulting.tech/MokoConsulting/MokoGitea/services/agit" gitea_context "code.mokoconsulting.tech/MokoConsulting/MokoGitea/services/context" pull_service "code.mokoconsulting.tech/MokoConsulting/MokoGitea/services/pull" - security_service "code.mokoconsulting.tech/MokoConsulting/MokoGitea/services/security" ) type preReceiveContext struct { @@ -41,9 +40,6 @@ type preReceiveContext struct { canCreatePullRequest bool checkedCanCreatePullRequest bool - canWriteCode bool - checkedCanWriteCode bool - protectedTags []*git_model.ProtectedTag gotProtectedTags bool @@ -51,24 +47,36 @@ type preReceiveContext struct { opts *private.HookOptions - branchName string + // this context should only contain shared variables, mutable variables like "current branch name" shouldn't be put here + canWriteCodeUnitCached *bool } -// CanWriteCode returns true if pusher can write code -func (ctx *preReceiveContext) CanWriteCode() bool { - if !ctx.checkedCanWriteCode { - if !ctx.loadPusherAndPermission() { - return false +func (ctx *preReceiveContext) canWriteCodeUnit() bool { + if ctx.canWriteCodeUnitCached == nil { + var canWrite bool + if ctx.loadPusherAndPermission() { + canWrite = ctx.userPerm.CanWrite(unit.TypeCode) || ctx.deployKeyAccessMode >= perm_model.AccessModeWrite } - ctx.canWriteCode = issues_model.CanMaintainerWriteToBranch(ctx, ctx.userPerm, ctx.branchName, ctx.user) || ctx.deployKeyAccessMode >= perm_model.AccessModeWrite - ctx.checkedCanWriteCode = true + ctx.canWriteCodeUnitCached = &canWrite } - return ctx.canWriteCode + return *ctx.canWriteCodeUnitCached } -// AssertCanWriteCode returns true if pusher can write code -func (ctx *preReceiveContext) AssertCanWriteCode() bool { - if !ctx.CanWriteCode() { +// canWriteCodeRef returns true if pusher can write to the code ref (branch/tag/commit) +func (ctx *preReceiveContext) canWriteCodeRef(refFullName git.RefName) bool { + if ctx.canWriteCodeUnit() { + return true + } + // then check whether if the pusher is a maintainer who can write the PR author's head repo branch + if !refFullName.IsBranch() { + return false + } + return issues_model.CanMaintainerWriteToBranch(ctx, ctx.userPerm, refFullName.BranchName(), ctx.user) +} + +// assertCanWriteRef returns true if pusher can write to the code ref, otherwise it responds with 403 Forbidden and returns false +func (ctx *preReceiveContext) assertCanWriteRef(refFullName git.RefName) bool { + if !ctx.canWriteCodeRef(refFullName) { if ctx.Written() { return false } @@ -130,7 +138,7 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) { case git.DefaultFeatures().SupportProcReceive && refFullName.IsFor(): preReceiveFor(ourCtx, refFullName) default: - ourCtx.AssertCanWriteCode() + ourCtx.assertCanWriteRef(refFullName) } if ctx.Written() { return @@ -142,9 +150,8 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) { func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, refFullName git.RefName) { branchName := refFullName.BranchName() - ctx.branchName = branchName - if !ctx.AssertCanWriteCode() { + if !ctx.assertCanWriteRef(refFullName) { return } @@ -152,25 +159,6 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, r gitRepo := ctx.Repo.GitRepo objectFormat := ctx.Repo.GetObjectFormat() - if newCommitID != objectFormat.EmptyObjectID().String() { - newCommit, err := gitRepo.GetCommit(newCommitID) - if err != nil { - log.Error("Secret scan: failed to get commit %s in %-v: %v", newCommitID[:12], repo, err) - } else { - if findings := security_service.ScanPushForSecrets(ctx, repo.ID, newCommit); len(findings) > 0 { - msg := fmt.Sprintf("Push rejected: %d secret(s) detected in commit %s", len(findings), newCommitID[:12]) - for _, f := range findings { - msg += fmt.Sprintf("\n - %s in %s:%d", f.Title, f.FilePath, f.LineNumber) - } - log.Warn("Secret scan blocked push to %s in %-v: %d findings", branchName, repo, len(findings)) - ctx.JSON(http.StatusForbidden, private.Response{ - UserMsg: msg, - }) - return - } - } - } - defaultBranch := repo.DefaultBranch if ctx.opts.IsWiki && repo.DefaultWikiBranch != "" { defaultBranch = repo.DefaultWikiBranch @@ -441,7 +429,7 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, r } func preReceiveTag(ctx *preReceiveContext, refFullName git.RefName) { - if !ctx.AssertCanWriteCode() { + if !ctx.assertCanWriteRef(refFullName) { return } -- 2.52.0 From 5da6a40f10b880fa0b6bde6ba2ec1ce12bc3dee2 Mon Sep 17 00:00:00 2001 From: Jonathan Miller Date: Sun, 28 Jun 2026 03:31:44 -0500 Subject: [PATCH 17/17] fix: resolve merge conflict marker and fix import paths in cherry-picked tests - Remove residual <<<<<<< HEAD marker from api_org_test.go - Convert code.gitea.io/gitea to mokoconsulting paths in 5 new test files: cmd/serv_test.go, models/auth/twofactor_test.go, modules/git/commit_info_nogogit_test.go, routers/private/hook_pre_receive_test.go, services/actions/notifier_helper_test.go - Add changelog entries for new features (#460, #507, #513) Claude-Session: https://claude.ai/code/session_011AAFzotGMf3ayvXhEmStCd --- CHANGELOG.md | 4 ++++ cmd/serv_test.go | 4 ++-- models/auth/twofactor_test.go | 4 ++-- modules/git/commit_info_nogogit_test.go | 4 ++-- routers/private/hook_pre_receive_test.go | 12 ++++++------ services/actions/notifier_helper_test.go | 8 ++++---- tests/integration/api_org_test.go | 1 - 7 files changed, 20 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 49f3af6b35..3c5ddfd8d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,10 @@ ## [Unreleased] ### Added +- Default org teams: auto-create Developers, Reviewers, CI/CD teams on organization creation (#513) +- Cascade merge rules: auto-create PRs to downstream branches after merge, with API CRUD and notifier integration (#460) +- Issue status presets: 4 built-in templates (default, software-development, support-tickets, bug-tracking) with API + web UI (#507) +- Cross-org status migration: copy status definitions from one org to another via API (#507) - Branch protection delete allowlist: configurable per-user/team/deploy-key allowlist for deleting protected branches (#696) - Workflow subdirectory discovery: workflows in subdirectories of `.mokogitea/workflows/` are now auto-discovered (#693) - API token scope `read:licensing` / `write:licensing` for licensing endpoints (#697) diff --git a/cmd/serv_test.go b/cmd/serv_test.go index 223b19ce4e..366dc1315a 100644 --- a/cmd/serv_test.go +++ b/cmd/serv_test.go @@ -6,8 +6,8 @@ package cmd import ( "testing" - "code.gitea.io/gitea/models/perm" - "code.gitea.io/gitea/modules/git" + "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/perm" + "code.mokoconsulting.tech/MokoConsulting/MokoGitea/modules/git" "github.com/stretchr/testify/assert" ) diff --git a/models/auth/twofactor_test.go b/models/auth/twofactor_test.go index e7951920f9..959b01a513 100644 --- a/models/auth/twofactor_test.go +++ b/models/auth/twofactor_test.go @@ -7,8 +7,8 @@ import ( "testing" "time" - auth_model "code.gitea.io/gitea/models/auth" - "code.gitea.io/gitea/models/unittest" + auth_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/auth" + "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/unittest" "github.com/pquerna/otp/totp" "github.com/stretchr/testify/assert" diff --git a/modules/git/commit_info_nogogit_test.go b/modules/git/commit_info_nogogit_test.go index 2080c8281b..5f75f4eba1 100644 --- a/modules/git/commit_info_nogogit_test.go +++ b/modules/git/commit_info_nogogit_test.go @@ -10,8 +10,8 @@ import ( "path/filepath" "testing" - "code.gitea.io/gitea/modules/test" - "code.gitea.io/gitea/modules/util" + "code.mokoconsulting.tech/MokoConsulting/MokoGitea/modules/test" + "code.mokoconsulting.tech/MokoConsulting/MokoGitea/modules/util" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" diff --git a/routers/private/hook_pre_receive_test.go b/routers/private/hook_pre_receive_test.go index 8d99b6b8fd..ccb4d8f0f1 100644 --- a/routers/private/hook_pre_receive_test.go +++ b/routers/private/hook_pre_receive_test.go @@ -6,12 +6,12 @@ package private import ( "testing" - issues_model "code.gitea.io/gitea/models/issues" - "code.gitea.io/gitea/models/perm/access" - repo_model "code.gitea.io/gitea/models/repo" - "code.gitea.io/gitea/models/unittest" - "code.gitea.io/gitea/modules/git" - "code.gitea.io/gitea/services/contexttest" + issues_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/issues" + "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/perm/access" + repo_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/repo" + "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/unittest" + "code.mokoconsulting.tech/MokoConsulting/MokoGitea/modules/git" + "code.mokoconsulting.tech/MokoConsulting/MokoGitea/services/contexttest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" diff --git a/services/actions/notifier_helper_test.go b/services/actions/notifier_helper_test.go index bb9fcc0cdb..e4edc5e5f6 100644 --- a/services/actions/notifier_helper_test.go +++ b/services/actions/notifier_helper_test.go @@ -8,10 +8,10 @@ import ( "errors" "testing" - actions_model "code.gitea.io/gitea/models/actions" - repo_model "code.gitea.io/gitea/models/repo" - user_model "code.gitea.io/gitea/models/user" - actions_module "code.gitea.io/gitea/modules/actions" + actions_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/actions" + repo_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/repo" + user_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/user" + actions_module "code.mokoconsulting.tech/MokoConsulting/MokoGitea/modules/actions" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" diff --git a/tests/integration/api_org_test.go b/tests/integration/api_org_test.go index cad439c492..c32eb3c5eb 100644 --- a/tests/integration/api_org_test.go +++ b/tests/integration/api_org_test.go @@ -264,7 +264,6 @@ func testAPIOrgGeneral(t *testing.T) { }) } -<<<<<<< HEAD func testAPIDeleteOrgRepos(t *testing.T) { org3 := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "org3"}) orgRepos, err := repo_model.GetOrgRepositories(t.Context(), org3.ID) -- 2.52.0