diff --git a/CHANGELOG.md b/CHANGELOG.md index 4db6fd594a..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) @@ -34,6 +38,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/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..366dc1315a --- /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.mokoconsulting.tech/MokoConsulting/MokoGitea/models/perm" + "code.mokoconsulting.tech/MokoConsulting/MokoGitea/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) + }) + } +} 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/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/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..959b01a513 --- /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.mokoconsulting.tech/MokoConsulting/MokoGitea/models/auth" + "code.mokoconsulting.tech/MokoConsulting/MokoGitea/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/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 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/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/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/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..5f75f4eba1 --- /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.mokoconsulting.tech/MokoConsulting/MokoGitea/modules/test" + "code.mokoconsulting.tech/MokoConsulting/MokoGitea/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 { 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/modules/hostmatcher/hostmatcher.go b/modules/hostmatcher/hostmatcher.go index 044dba679a..c82a57b7b9 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,61 @@ 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 +}) + +// isReservedIP reports whether ip falls in reserved special-purpose +// range (see reservedIPNets) that must not be considered a public/external destination. +func isReservedIP(ip net.IP) bool { + 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. @@ -93,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() && !ip.IsPrivate() { + // 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 ip.IsPrivate() { + // 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: @@ -135,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 } @@ -146,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 c781847471..464354ff41 100644 --- a/modules/hostmatcher/hostmatcher_test.go +++ b/modules/hostmatcher/hostmatcher_test.go @@ -159,3 +159,60 @@ 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 + "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.Falsef(t, private.MatchIPAddr(addr), "reserved ip %s should match private block-list", ip) + } +} 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") + }) } 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..ccb4d8f0f1 --- /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.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" +) + +// 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/oauth.go b/routers/web/auth/oauth.go index 362824646f..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 @@ -514,17 +526,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/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/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/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/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/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..e4edc5e5f6 --- /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.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" +) + +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/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/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/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/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/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 { 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/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) diff --git a/tests/integration/api_org_test.go b/tests/integration/api_org_test.go index bf210ea9f9..c32eb3c5eb 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" @@ -289,6 +290,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/auth_oauth2_test.go b/tests/integration/auth_oauth2_test.go index fc57088e3b..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, @@ -130,8 +132,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 +235,119 @@ 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) + } + })) + t.Cleanup(srv.Close) + 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) 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) + }) + } +} 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) +} 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) + }) }) }