From 656351d028994d8edd5b2b0cf061fa0b3ff42621 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 2 Nov 2024 15:06:27 +0100 Subject: [PATCH 1/9] fix: add ID check for updating push mirror interval - Ensure that the specified push mirror ID belongs to the requested repository, otherwise it is possible to modify the intervals of the push mirrors that do not belong to the requested repository. - Integration test added. (cherry picked from commit 786dfc7fb81ee76d4292ca5fcb33e6ea7bdccc29) --- routers/web/repo/setting/setting.go | 16 +++--- tests/integration/mirror_push_test.go | 82 +++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 9 deletions(-) diff --git a/routers/web/repo/setting/setting.go b/routers/web/repo/setting/setting.go index a7d4e75ff6..ad44c0515a 100644 --- a/routers/web/repo/setting/setting.go +++ b/routers/web/repo/setting/setting.go @@ -514,21 +514,19 @@ func SettingsPost(ctx *context.Context) { // as an error on the UI for this action ctx.Data["Err_RepoName"] = nil + m, err := selectPushMirrorByForm(ctx, form, repo) + if err != nil { + ctx.NotFound("", nil) + return + } + interval, err := time.ParseDuration(form.PushMirrorInterval) if err != nil || (interval != 0 && interval < setting.Mirror.MinInterval) { ctx.RenderWithErr(ctx.Tr("repo.mirror_interval_invalid"), tplSettingsOptions, &forms.RepoSettingForm{}) return } - id, err := strconv.ParseInt(form.PushMirrorID, 10, 64) - if err != nil { - ctx.ServerError("UpdatePushMirrorIntervalPushMirrorID", err) - return - } - m := &repo_model.PushMirror{ - ID: id, - Interval: interval, - } + m.Interval = interval if err := repo_model.UpdatePushMirrorInterval(ctx, m); err != nil { ctx.ServerError("UpdatePushMirrorInterval", err) return diff --git a/tests/integration/mirror_push_test.go b/tests/integration/mirror_push_test.go index a8c654f69e..586c8d7d7c 100644 --- a/tests/integration/mirror_push_test.go +++ b/tests/integration/mirror_push_test.go @@ -13,11 +13,14 @@ import ( "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" + "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/test" gitea_context "code.gitea.io/gitea/services/context" doctor "code.gitea.io/gitea/services/doctor" "code.gitea.io/gitea/services/migrations" @@ -146,3 +149,82 @@ func doRemovePushMirror(ctx APITestContext, address, username, password string, assert.Contains(t, flashCookie.Value, "success") } } + +func TestPushMirrorSettings(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + defer test.MockVariableValue(&setting.Migrations.AllowLocalNetworks, true)() + defer test.MockVariableValue(&setting.Mirror.Enabled, true)() + require.NoError(t, migrations.Init()) + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + srcRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) + srcRepo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) + assert.False(t, srcRepo.HasWiki()) + sess := loginUser(t, user.Name) + pushToRepo, _, f := CreateDeclarativeRepoWithOptions(t, user, DeclarativeRepoOptions{ + Name: optional.Some("push-mirror-test"), + AutoInit: optional.Some(false), + EnabledUnits: optional.Some([]unit.Type{unit.TypeCode}), + }) + defer f() + + t.Run("Adding", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/settings", srcRepo2.FullName()), map[string]string{ + "_csrf": GetCSRF(t, sess, fmt.Sprintf("/%s/settings", srcRepo2.FullName())), + "action": "push-mirror-add", + "push_mirror_address": u.String() + pushToRepo.FullName(), + "push_mirror_interval": "0", + }) + sess.MakeRequest(t, req, http.StatusSeeOther) + + req = NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/settings", srcRepo.FullName()), map[string]string{ + "_csrf": GetCSRF(t, sess, fmt.Sprintf("/%s/settings", srcRepo.FullName())), + "action": "push-mirror-add", + "push_mirror_address": u.String() + pushToRepo.FullName(), + "push_mirror_interval": "0", + }) + sess.MakeRequest(t, req, http.StatusSeeOther) + + flashCookie := sess.GetCookie(gitea_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.Contains(t, flashCookie.Value, "success") + }) + + mirrors, _, err := repo_model.GetPushMirrorsByRepoID(db.DefaultContext, srcRepo.ID, db.ListOptions{}) + require.NoError(t, err) + assert.Len(t, mirrors, 1) + mirrorID := mirrors[0].ID + + mirrors, _, err = repo_model.GetPushMirrorsByRepoID(db.DefaultContext, srcRepo2.ID, db.ListOptions{}) + require.NoError(t, err) + assert.Len(t, mirrors, 1) + + t.Run("Interval", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + unittest.AssertExistsAndLoadBean(t, &repo_model.PushMirror{ID: mirrorID - 1}) + + req := NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/settings", srcRepo.FullName()), map[string]string{ + "_csrf": GetCSRF(t, sess, fmt.Sprintf("/%s/settings", srcRepo.FullName())), + "action": "push-mirror-update", + "push_mirror_id": strconv.FormatInt(mirrorID-1, 10), + "push_mirror_interval": "10m0s", + }) + sess.MakeRequest(t, req, http.StatusNotFound) + + req = NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/settings", srcRepo.FullName()), map[string]string{ + "_csrf": GetCSRF(t, sess, fmt.Sprintf("/%s/settings", srcRepo.FullName())), + "action": "push-mirror-update", + "push_mirror_id": strconv.FormatInt(mirrorID, 10), + "push_mirror_interval": "10m0s", + }) + sess.MakeRequest(t, req, http.StatusSeeOther) + + flashCookie := sess.GetCookie(gitea_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.Contains(t, flashCookie.Value, "success") + }) + }) +} From 7480b6072f46dfabd01cd469c180baeb0b76c8be Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 2 Nov 2024 20:55:30 +0100 Subject: [PATCH 2/9] fix: don't show private forks in forks list - If a repository is forked to a private or limited user/organization, the fork should not be visible in the list of forks depending on the doer requesting the list of forks. - Added integration testing for web and API route. (cherry picked from commit 061abe60045212acf8c3f5c49b5cc758b4cbcde9) --- models/repo/fork.go | 10 ++++--- routers/api/v1/repo/fork.go | 4 +-- routers/web/repo/view.go | 10 +++---- tests/integration/api_fork_test.go | 43 +++++++++++++++++++++++++++++ tests/integration/repo_fork_test.go | 32 +++++++++++++++++++++ 5 files changed, 88 insertions(+), 11 deletions(-) diff --git a/models/repo/fork.go b/models/repo/fork.go index 07cd31c269..632e91c2bb 100644 --- a/models/repo/fork.go +++ b/models/repo/fork.go @@ -7,6 +7,7 @@ import ( "context" "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" "xorm.io/builder" @@ -54,9 +55,9 @@ func GetUserFork(ctx context.Context, repoID, userID int64) (*Repository, error) return &forkedRepo, nil } -// GetForks returns all the forks of the repository -func GetForks(ctx context.Context, repo *Repository, listOptions db.ListOptions) ([]*Repository, error) { - sess := db.GetEngine(ctx) +// GetForks returns all the forks of the repository that are visible to the user. +func GetForks(ctx context.Context, repo *Repository, user *user_model.User, listOptions db.ListOptions) ([]*Repository, int64, error) { + sess := db.GetEngine(ctx).Where(AccessibleRepositoryCondition(user, unit.TypeInvalid)) var forks []*Repository if listOptions.Page == 0 { @@ -66,7 +67,8 @@ func GetForks(ctx context.Context, repo *Repository, listOptions db.ListOptions) sess = db.SetSessionPagination(sess, &listOptions) } - return forks, sess.Find(&forks, &Repository{ForkID: repo.ID}) + count, err := sess.FindAndCount(&forks, &Repository{ForkID: repo.ID}) + return forks, count, err } // IncrementRepoForkNum increment repository fork number diff --git a/routers/api/v1/repo/fork.go b/routers/api/v1/repo/fork.go index 212cc7a93b..41bac96ce4 100644 --- a/routers/api/v1/repo/fork.go +++ b/routers/api/v1/repo/fork.go @@ -55,7 +55,7 @@ func ListForks(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - forks, err := repo_model.GetForks(ctx, ctx.Repo.Repository, utils.GetListOptions(ctx)) + forks, total, err := repo_model.GetForks(ctx, ctx.Repo.Repository, ctx.Doer, utils.GetListOptions(ctx)) if err != nil { ctx.Error(http.StatusInternalServerError, "GetForks", err) return @@ -70,7 +70,7 @@ func ListForks(ctx *context.APIContext) { apiForks[i] = convert.ToRepo(ctx, fork, permission) } - ctx.SetTotalCountHeader(int64(ctx.Repo.Repository.NumForks)) + ctx.SetTotalCountHeader(total) ctx.JSON(http.StatusOK, apiForks) } diff --git a/routers/web/repo/view.go b/routers/web/repo/view.go index e98cd46c4e..48a84bf432 100644 --- a/routers/web/repo/view.go +++ b/routers/web/repo/view.go @@ -1221,11 +1221,8 @@ func Forks(ctx *context.Context) { page = 1 } - pager := context.NewPagination(ctx.Repo.Repository.NumForks, setting.MaxForksPerPage, page, 5) - ctx.Data["Page"] = pager - - forks, err := repo_model.GetForks(ctx, ctx.Repo.Repository, db.ListOptions{ - Page: pager.Paginater.Current(), + forks, total, err := repo_model.GetForks(ctx, ctx.Repo.Repository, ctx.Doer, db.ListOptions{ + Page: page, PageSize: setting.MaxForksPerPage, }) if err != nil { @@ -1233,6 +1230,9 @@ func Forks(ctx *context.Context) { return } + pager := context.NewPagination(int(total), setting.MaxForksPerPage, page, 5) + ctx.Data["Page"] = pager + for _, fork := range forks { if err = fork.LoadOwner(ctx); err != nil { ctx.ServerError("LoadOwner", err) diff --git a/tests/integration/api_fork_test.go b/tests/integration/api_fork_test.go index b80b4c6645..15b60f8344 100644 --- a/tests/integration/api_fork_test.go +++ b/tests/integration/api_fork_test.go @@ -18,6 +18,8 @@ import ( "code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/routers" "code.gitea.io/gitea/tests" + + "github.com/stretchr/testify/assert" ) func TestAPIForkAsAdminIgnoringLimits(t *testing.T) { @@ -108,3 +110,44 @@ func TestAPIDisabledForkRepo(t *testing.T) { }) }) } + +func TestAPIForkListPrivateRepo(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + session := loginUser(t, "user5") + token := getTokenForLoggedInUser(t, session, + auth_model.AccessTokenScopeWriteRepository, + auth_model.AccessTokenScopeWriteOrganization) + org23 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 23, Visibility: api.VisibleTypePrivate}) + + req := NewRequestWithJSON(t, "POST", "/api/v1/repos/user2/repo1/forks", &api.CreateForkOption{ + Organization: &org23.Name, + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusAccepted) + + t.Run("Anomynous", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "GET", "/api/v1/repos/user2/repo1/forks") + resp := MakeRequest(t, req, http.StatusOK) + + var forks []*api.Repository + DecodeJSON(t, resp, &forks) + + assert.Empty(t, forks) + assert.EqualValues(t, "0", resp.Header().Get("X-Total-Count")) + }) + + t.Run("Logged in", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "GET", "/api/v1/repos/user2/repo1/forks").AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusOK) + + var forks []*api.Repository + DecodeJSON(t, resp, &forks) + + assert.Len(t, forks, 1) + assert.EqualValues(t, "1", resp.Header().Get("X-Total-Count")) + }) +} diff --git a/tests/integration/repo_fork_test.go b/tests/integration/repo_fork_test.go index e58e9fe201..c6984a12fb 100644 --- a/tests/integration/repo_fork_test.go +++ b/tests/integration/repo_fork_test.go @@ -17,6 +17,7 @@ import ( "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/routers" repo_service "code.gitea.io/gitea/services/repository" @@ -238,3 +239,34 @@ func TestRepoForkToOrg(t *testing.T) { }) }) } + +func TestForkListPrivateRepo(t *testing.T) { + forkItemSelector := ".tw-flex.tw-items-center.tw-py-2" + + onGiteaRun(t, func(t *testing.T, u *url.URL) { + session := loginUser(t, "user5") + org23 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 23, Visibility: structs.VisibleTypePrivate}) + + testRepoFork(t, session, "user2", "repo1", org23.Name, "repo1") + + t.Run("Anomynous", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "GET", "/user2/repo1/forks") + resp := MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + + htmlDoc.AssertElement(t, forkItemSelector, false) + }) + + t.Run("Logged in", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "GET", "/user2/repo1/forks") + resp := session.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + + htmlDoc.AssertElement(t, forkItemSelector, true) + }) + }) +} From eb066a472c30629c1d984db471373e332ce20700 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 2 Nov 2024 17:41:34 +0100 Subject: [PATCH 3/9] fix: require code permissions for branch feed - The RSS and atom feed for branches exposes details about the code, it therefore should be guarded by the requirement that the doer has access to the code of that repository. - Added integration testing. (cherry picked from commit 3e3ef76808100cb1c853378733d0f6a910324ac6) --- routers/web/web.go | 6 +- tests/integration/api_feed_user_test.go | 20 +++++ tests/integration/fixtures/TestFeed/team.yml | 21 +++++ .../fixtures/TestFeed/team_repo.yml | 11 +++ .../fixtures/TestFeed/team_unit.yml | 83 +++++++++++++++++++ .../fixtures/TestFeed/team_user.yml | 11 +++ 6 files changed, 150 insertions(+), 2 deletions(-) create mode 100644 tests/integration/fixtures/TestFeed/team.yml create mode 100644 tests/integration/fixtures/TestFeed/team_repo.yml create mode 100644 tests/integration/fixtures/TestFeed/team_unit.yml create mode 100644 tests/integration/fixtures/TestFeed/team_user.yml diff --git a/routers/web/web.go b/routers/web/web.go index ee9694f41c..081290fe08 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1547,8 +1547,10 @@ func registerRoutes(m *web.Route) { m.Get("/cherry-pick/{sha:([a-f0-9]{4,64})$}", repo.SetEditorconfigIfExists, repo.CherryPick) }, repo.MustBeNotEmpty, context.RepoRef(), reqRepoCodeReader) - m.Get("/rss/branch/*", repo.MustBeNotEmpty, context.RepoRefByType(context.RepoRefBranch), feedEnabled, feed.RenderBranchFeed("rss")) - m.Get("/atom/branch/*", repo.MustBeNotEmpty, context.RepoRefByType(context.RepoRefBranch), feedEnabled, feed.RenderBranchFeed("atom")) + m.Group("", func() { + m.Get("/rss/branch/*", feed.RenderBranchFeed("rss")) + m.Get("/atom/branch/*", feed.RenderBranchFeed("atom")) + }, repo.MustBeNotEmpty, context.RepoRefByType(context.RepoRefBranch), reqRepoCodeReader, feedEnabled) m.Group("/src", func() { m.Get("/branch/*", context.RepoRefByType(context.RepoRefBranch), repo.Home) diff --git a/tests/integration/api_feed_user_test.go b/tests/integration/api_feed_user_test.go index 7f3824963a..23b9e56e90 100644 --- a/tests/integration/api_feed_user_test.go +++ b/tests/integration/api_feed_user_test.go @@ -86,4 +86,24 @@ func TestFeed(t *testing.T) { }) }) }) + + t.Run("View permission", func(t *testing.T) { + t.Run("Anomynous", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + req := NewRequest(t, "GET", "/org3/repo3/rss/branch/master") + MakeRequest(t, req, http.StatusNotFound) + }) + t.Run("No code permission", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + session := loginUser(t, "user8") + req := NewRequest(t, "GET", "/org3/repo3/rss/branch/master") + session.MakeRequest(t, req, http.StatusNotFound) + }) + t.Run("With code permission", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + session := loginUser(t, "user9") + req := NewRequest(t, "GET", "/org3/repo3/rss/branch/master") + session.MakeRequest(t, req, http.StatusOK) + }) + }) } diff --git a/tests/integration/fixtures/TestFeed/team.yml b/tests/integration/fixtures/TestFeed/team.yml new file mode 100644 index 0000000000..da27ac7c0c --- /dev/null +++ b/tests/integration/fixtures/TestFeed/team.yml @@ -0,0 +1,21 @@ +- + id: 1001 + org_id: 3 + lower_name: no_code + name: no_code + authorize: 1 # read + num_repos: 1 + num_members: 1 + includes_all_repositories: false + can_create_org_repo: false + +- + id: 1002 + org_id: 3 + lower_name: read_code + name: no_code + authorize: 1 # read + num_repos: 1 + num_members: 1 + includes_all_repositories: false + can_create_org_repo: false diff --git a/tests/integration/fixtures/TestFeed/team_repo.yml b/tests/integration/fixtures/TestFeed/team_repo.yml new file mode 100644 index 0000000000..922d1ef51e --- /dev/null +++ b/tests/integration/fixtures/TestFeed/team_repo.yml @@ -0,0 +1,11 @@ +- + id: 1001 + org_id: 3 + team_id: 1001 + repo_id: 3 + +- + id: 1002 + org_id: 3 + team_id: 1002 + repo_id: 3 diff --git a/tests/integration/fixtures/TestFeed/team_unit.yml b/tests/integration/fixtures/TestFeed/team_unit.yml new file mode 100644 index 0000000000..9fcb4396dc --- /dev/null +++ b/tests/integration/fixtures/TestFeed/team_unit.yml @@ -0,0 +1,83 @@ +- + id: 1001 + team_id: 1001 + type: 1 + access_mode: 0 + +- + id: 1002 + team_id: 1001 + type: 2 + access_mode: 1 + +- + id: 1003 + team_id: 1001 + type: 3 + access_mode: 1 + +- + id: 1004 + team_id: 1001 + type: 4 + access_mode: 1 + +- + id: 1005 + team_id: 1001 + type: 5 + access_mode: 1 + +- + id: 1006 + team_id: 1001 + type: 6 + access_mode: 1 + +- + id: 1007 + team_id: 1001 + type: 7 + access_mode: 1 + +- + id: 1008 + team_id: 1002 + type: 1 + access_mode: 1 + +- + id: 1009 + team_id: 1002 + type: 2 + access_mode: 1 + +- + id: 1010 + team_id: 1002 + type: 3 + access_mode: 1 + +- + id: 1011 + team_id: 1002 + type: 4 + access_mode: 1 + +- + id: 1012 + team_id: 1002 + type: 5 + access_mode: 1 + +- + id: 1013 + team_id: 1002 + type: 6 + access_mode: 1 + +- + id: 1014 + team_id: 1002 + type: 7 + access_mode: 1 diff --git a/tests/integration/fixtures/TestFeed/team_user.yml b/tests/integration/fixtures/TestFeed/team_user.yml new file mode 100644 index 0000000000..15fa3ebb1d --- /dev/null +++ b/tests/integration/fixtures/TestFeed/team_user.yml @@ -0,0 +1,11 @@ +- + id: 1001 + org_id: 3 + team_id: 1001 + uid: 8 + +- + id: 1002 + org_id: 3 + team_id: 1002 + uid: 9 From c2284d871fa69d0ee3de464ebeacd1fb11fc413f Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 9 Nov 2024 23:47:39 +0100 Subject: [PATCH 4/9] fix: anomynous users code search for private/limited user's repository - Consider private/limited users in the `AccessibleRepositoryCondition` query, previously this only considered private/limited organization. This limits the ability for anomynous users to do code search on private/limited user's repository - Unit test added. (cherry picked from commit b70196653f9d7d3b9d4e72d114e5cc6f472988c4) --- .../repository.yml | 30 +++++++++++++ models/repo/repo_list.go | 7 +-- models/repo/repo_list_test.go | 45 +++++++++++++++++++ 3 files changed, 77 insertions(+), 5 deletions(-) create mode 100644 models/repo/TestSearchRepositoryIDsByCondition/repository.yml diff --git a/models/repo/TestSearchRepositoryIDsByCondition/repository.yml b/models/repo/TestSearchRepositoryIDsByCondition/repository.yml new file mode 100644 index 0000000000..9ce830783d --- /dev/null +++ b/models/repo/TestSearchRepositoryIDsByCondition/repository.yml @@ -0,0 +1,30 @@ +- + id: 1001 + owner_id: 33 + owner_name: user33 + lower_name: repo1001 + name: repo1001 + default_branch: main + num_watches: 0 + num_stars: 0 + num_forks: 0 + num_issues: 0 + num_closed_issues: 0 + num_pulls: 0 + num_closed_pulls: 0 + num_milestones: 0 + num_closed_milestones: 0 + num_projects: 0 + num_closed_projects: 0 + is_private: false + is_empty: false + is_archived: false + is_mirror: false + status: 0 + is_fork: false + fork_id: 0 + is_template: false + template_id: 0 + size: 0 + is_fsck_enabled: true + close_issues_via_commit_in_any_branch: false diff --git a/models/repo/repo_list.go b/models/repo/repo_list.go index cb7cd47a8d..9644cbd70c 100644 --- a/models/repo/repo_list.go +++ b/models/repo/repo_list.go @@ -665,12 +665,9 @@ func AccessibleRepositoryCondition(user *user_model.User, unitType unit.Type) bu // 1. Be able to see all non-private repositories that either: cond = cond.Or(builder.And( builder.Eq{"`repository`.is_private": false}, - // 2. Aren't in an private organisation or limited organisation if we're not logged in + // 2. Aren't in an private organisation/user or limited organisation/user if the doer is not logged in. builder.NotIn("`repository`.owner_id", builder.Select("id").From("`user`").Where( - builder.And( - builder.Eq{"type": user_model.UserTypeOrganization}, - builder.In("visibility", orgVisibilityLimit)), - )))) + builder.In("visibility", orgVisibilityLimit))))) } if user != nil { diff --git a/models/repo/repo_list_test.go b/models/repo/repo_list_test.go index 2da8920f80..cfc1174b2f 100644 --- a/models/repo/repo_list_test.go +++ b/models/repo/repo_list_test.go @@ -4,13 +4,18 @@ package repo_test import ( + "path/filepath" + "slices" "strings" "testing" "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" + "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/optional" + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/structs" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -403,3 +408,43 @@ func TestSearchRepositoryByTopicName(t *testing.T) { }) } } + +func TestSearchRepositoryIDsByCondition(t *testing.T) { + defer unittest.OverrideFixtures( + unittest.FixturesOptions{ + Dir: filepath.Join(setting.AppWorkPath, "models/fixtures/"), + Base: setting.AppWorkPath, + Dirs: []string{"models/repo/TestSearchRepositoryIDsByCondition/"}, + }, + )() + require.NoError(t, unittest.PrepareTestDatabase()) + // Sanity check of the database + limitedUser := unittest.AssertExistsAndLoadBean(t, &user.User{ID: 33, Visibility: structs.VisibleTypeLimited}) + unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1001, OwnerID: limitedUser.ID}) + + testCases := []struct { + user *user.User + repoIDs []int64 + }{ + { + user: nil, + repoIDs: []int64{1, 4, 8, 9, 10, 11, 12, 14, 17, 18, 21, 23, 25, 27, 29, 32, 33, 34, 35, 36, 37, 42, 44, 45, 46, 47, 48, 49, 50, 51, 53, 57, 58, 60, 61, 1059}, + }, + { + user: unittest.AssertExistsAndLoadBean(t, &user.User{ID: 4}), + repoIDs: []int64{1, 3, 4, 8, 9, 10, 11, 12, 14, 17, 18, 21, 23, 25, 27, 29, 32, 33, 34, 35, 36, 37, 38, 40, 42, 44, 45, 46, 47, 48, 49, 50, 51, 53, 57, 58, 60, 61, 1001, 1059}, + }, + { + user: unittest.AssertExistsAndLoadBean(t, &user.User{ID: 5}), + repoIDs: []int64{1, 4, 8, 9, 10, 11, 12, 14, 17, 18, 21, 23, 25, 27, 29, 32, 33, 34, 35, 36, 37, 38, 40, 42, 44, 45, 46, 47, 48, 49, 50, 51, 53, 57, 58, 60, 61, 1001, 1059}, + }, + } + + for _, testCase := range testCases { + repoIDs, err := repo_model.FindUserCodeAccessibleRepoIDs(db.DefaultContext, testCase.user) + require.NoError(t, err) + + slices.Sort(repoIDs) + assert.EqualValues(t, testCase.repoIDs, repoIDs) + } +} From 3a197f7de252793653bc535cd3e83fb79a377603 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 2 Nov 2024 14:08:04 +0100 Subject: [PATCH 5/9] fix: strict matching of allowed content for sanitizer - _Simply_ add `^$` to regexp that didn't had it yet, this avoids any content being allowed that simply had the allowed content as a substring. - Fix file-preview regex to have `$` instead of `*`. (cherry picked from commit 7067cc7da4f144cc8a2fd2ae6e5307e0465ace7f) v7: added fix for ref-issue, this is already fixed in forgejo branch but not backported as it was part of a feature. --- modules/markup/sanitizer.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/modules/markup/sanitizer.go b/modules/markup/sanitizer.go index d07bba3004..f9789bbb97 100644 --- a/modules/markup/sanitizer.go +++ b/modules/markup/sanitizer.go @@ -94,10 +94,10 @@ func createDefaultPolicy() *bluemonday.Policy { } // Allow classes for anchors - policy.AllowAttrs("class").Matching(regexp.MustCompile(`ref-issue( ref-external-issue)?`)).OnElements("a") + policy.AllowAttrs("class").Matching(regexp.MustCompile(`^ref-issue( ref-external-issue)?$`)).OnElements("a") // Allow classes for task lists - policy.AllowAttrs("class").Matching(regexp.MustCompile(`task-list-item`)).OnElements("li") + policy.AllowAttrs("class").Matching(regexp.MustCompile(`^task-list-item$`)).OnElements("li") // Allow classes for org mode list item status. policy.AllowAttrs("class").Matching(regexp.MustCompile(`^(unchecked|checked|indeterminate)$`)).OnElements("li") @@ -106,7 +106,7 @@ func createDefaultPolicy() *bluemonday.Policy { policy.AllowAttrs("class").Matching(regexp.MustCompile(`^icon(\s+[\p{L}\p{N}_-]+)+$`)).OnElements("i") // Allow classes for emojis - policy.AllowAttrs("class").Matching(regexp.MustCompile(`emoji`)).OnElements("img") + policy.AllowAttrs("class").Matching(regexp.MustCompile(`^emoji$`)).OnElements("img") // Allow icons, emojis, chroma syntax and keyword markup on span policy.AllowAttrs("class").Matching(regexp.MustCompile(`^((icon(\s+[\p{L}\p{N}_-]+)+)|(emoji)|(language-math display)|(language-math inline))$|^([a-z][a-z0-9]{0,2})$|^` + keywordClass + `$`)).OnElements("span") @@ -122,13 +122,13 @@ func createDefaultPolicy() *bluemonday.Policy { policy.AllowAttrs("class").Matching(regexp.MustCompile("^header$")).OnElements("div") policy.AllowAttrs("data-line-number").Matching(regexp.MustCompile("^[0-9]+$")).OnElements("span") policy.AllowAttrs("class").Matching(regexp.MustCompile("^text small grey$")).OnElements("span") - policy.AllowAttrs("class").Matching(regexp.MustCompile("^file-preview*")).OnElements("table") + policy.AllowAttrs("class").Matching(regexp.MustCompile("^file-preview$")).OnElements("table") policy.AllowAttrs("class").Matching(regexp.MustCompile("^lines-escape$")).OnElements("td") policy.AllowAttrs("class").Matching(regexp.MustCompile("^toggle-escape-button btn interact-bg$")).OnElements("button") policy.AllowAttrs("title").OnElements("button") policy.AllowAttrs("class").Matching(regexp.MustCompile("^ambiguous-code-point$")).OnElements("span") policy.AllowAttrs("data-tooltip-content").OnElements("span") - policy.AllowAttrs("class").Matching(regexp.MustCompile("muted|(text black)")).OnElements("a") + policy.AllowAttrs("class").Matching(regexp.MustCompile("^muted|(text black)$")).OnElements("a") policy.AllowAttrs("class").Matching(regexp.MustCompile("^ui warning message tw-text-left$")).OnElements("div") // Allow generally safe attributes From 5b53a150c0f3d6e0b75f9451c898553e2d739cb4 Mon Sep 17 00:00:00 2001 From: Gusted Date: Mon, 12 Aug 2024 20:01:09 +0200 Subject: [PATCH 6/9] Improve usage of HMAC output for mailer tokens - If the incoming mail feature is enabled, tokens are being sent with outgoing mails. These tokens contains information about what type of action is allow with such token (such as replying to a certain issue ID), to verify these tokens the code uses the HMAC-SHA256 construction. - The output of the HMAC is truncated to 80 bits, because this is recommended by RFC2104, but RFC2104 actually doesn't recommend this. It recommends, if truncation should need to take place, it should use max(80, hash_len/2) of the leftmost bits. For HMAC-SHA256 this works out to 128 bits instead of the currently used 80 bits. - Update to token version 2 and disallow any usage of token version 1, token version 2 are generated with 128 bits of HMAC output. - Add test to verify the deprecation of token version 1 and a general MAC check test. (cherry picked from commit 9508aa7713632ed40124a933d91d5766cf2369c2) --- services/mailer/token/token.go | 16 +++++++-- tests/integration/incoming_email_test.go | 46 ++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/services/mailer/token/token.go b/services/mailer/token/token.go index 8a5a762d6b..1a52bce803 100644 --- a/services/mailer/token/token.go +++ b/services/mailer/token/token.go @@ -22,9 +22,16 @@ import ( // // The payload is verifiable by the generated HMAC using the user secret. It contains: // | Timestamp | Action/Handler Type | Action/Handler Data | +// +// +// Version changelog +// +// v1 -> v2: +// Use 128 instead of 80 bits of the HMAC-SHA256 output. const ( tokenVersion1 byte = 1 + tokenVersion2 byte = 2 tokenLifetimeInYears int = 1 ) @@ -70,7 +77,7 @@ func CreateToken(ht HandlerType, user *user_model.User, data []byte) (string, er return "", err } - return encodingWithoutPadding.EncodeToString(append([]byte{tokenVersion1}, packagedData...)), nil + return encodingWithoutPadding.EncodeToString(append([]byte{tokenVersion2}, packagedData...)), nil } // ExtractToken extracts the action/user tuple from the token and verifies the content @@ -84,7 +91,7 @@ func ExtractToken(ctx context.Context, token string) (HandlerType, *user_model.U return UnknownHandlerType, nil, nil, &ErrToken{"no data"} } - if data[0] != tokenVersion1 { + if data[0] != tokenVersion2 { return UnknownHandlerType, nil, nil, &ErrToken{fmt.Sprintf("unsupported token version: %v", data[0])} } @@ -124,5 +131,8 @@ func generateHmac(secret, payload []byte) []byte { mac.Write(payload) hmac := mac.Sum(nil) - return hmac[:10] // RFC2104 recommends not using less then 80 bits + // RFC2104 section 5 recommends that if you do HMAC truncation, you should use + // the max(80, hash_len/2) of the leftmost bits. + // For SHA256 this works out to using 128 of the leftmost bits. + return hmac[:16] } diff --git a/tests/integration/incoming_email_test.go b/tests/integration/incoming_email_test.go index fdc0425b54..66f833b28d 100644 --- a/tests/integration/incoming_email_test.go +++ b/tests/integration/incoming_email_test.go @@ -4,6 +4,7 @@ package integration import ( + "encoding/base32" "io" "net" "net/smtp" @@ -75,6 +76,51 @@ func TestIncomingEmail(t *testing.T) { assert.Equal(t, payload, p) }) + tokenEncoding := base32.StdEncoding.WithPadding(base32.NoPadding) + t.Run("Deprecated token version", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + payload := []byte{1, 2, 3, 4, 5} + + token, err := token_service.CreateToken(token_service.ReplyHandlerType, user, payload) + require.NoError(t, err) + assert.NotEmpty(t, token) + + // Set the token to version 1. + unencodedToken, err := tokenEncoding.DecodeString(token) + require.NoError(t, err) + unencodedToken[0] = 1 + token = tokenEncoding.EncodeToString(unencodedToken) + + ht, u, p, err := token_service.ExtractToken(db.DefaultContext, token) + require.ErrorContains(t, err, "unsupported token version: 1") + assert.Equal(t, token_service.UnknownHandlerType, ht) + assert.Nil(t, u) + assert.Nil(t, p) + }) + + t.Run("MAC check", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + payload := []byte{1, 2, 3, 4, 5} + + token, err := token_service.CreateToken(token_service.ReplyHandlerType, user, payload) + require.NoError(t, err) + assert.NotEmpty(t, token) + + // Modify the MAC. + unencodedToken, err := tokenEncoding.DecodeString(token) + require.NoError(t, err) + unencodedToken[len(unencodedToken)-1] ^= 0x01 + token = tokenEncoding.EncodeToString(unencodedToken) + + ht, u, p, err := token_service.ExtractToken(db.DefaultContext, token) + require.ErrorContains(t, err, "verification failed") + assert.Equal(t, token_service.UnknownHandlerType, ht) + assert.Nil(t, u) + assert.Nil(t, p) + }) + t.Run("Handler", func(t *testing.T) { t.Run("Reply", func(t *testing.T) { checkReply := func(t *testing.T, payload []byte, issue *issues_model.Issue, commentType issues_model.CommentType) { From b770282d458d5ab7301e4a89512fb6563a4329c8 Mon Sep 17 00:00:00 2001 From: Gusted Date: Thu, 14 Nov 2024 13:31:02 +0100 Subject: [PATCH 7/9] fix: extend `forgejo_auth_token` table - Add a `purpose` column, this allows the `forgejo_auth_token` table to be used by other parts of Forgejo, while still enjoying the no-compromise architecture. - Remove the 'roll your own crypto' time limited code functions and migrate them to the `forgejo_auth_token` table. This migration ensures generated codes can only be used for their purpose and ensure they are invalidated after their usage by deleting it from the database, this also should help making auditing of the security code easier, as we're no longer trying to stuff a lot of data into a HMAC construction. -Helper functions are rewritten to ensure a safe-by-design approach to these tokens. - Add the `forgejo_auth_token` to dbconsistency doctor and add it to the `deleteUser` function. - TODO: Add cron job to delete expired authorization tokens. - Unit and integration tests added. (cherry picked from commit 1ce33aa38d1d258d14523ff2c7c2dbf339f22b74) v7: Removed migration - XORM can handle this case automatically without migration. assert.Equal(t, `doesnotexist@example.com`, msgs[0].To) in tests because v7 does not include the user name to the recipient. --- models/auth/auth_token.go | 26 +++- models/user/email_address.go | 20 --- models/user/user.go | 75 ++++++---- models/user/user_test.go | 68 ++++++++- modules/base/tool.go | 63 -------- modules/base/tool_test.go | 41 ----- routers/web/auth/auth.go | 100 ++++++------- routers/web/auth/password.go | 18 ++- routers/web/user/setting/account.go | 15 +- services/context/context_cookie.go | 2 +- services/doctor/dbconsistency.go | 3 + services/mailer/mail.go | 48 ++++-- services/user/delete.go | 1 + tests/integration/auth_token_test.go | 8 +- tests/integration/org_team_invite_test.go | 7 +- tests/integration/user_test.go | 174 ++++++++++++++++++++++ 16 files changed, 429 insertions(+), 240 deletions(-) diff --git a/models/auth/auth_token.go b/models/auth/auth_token.go index 2c3ca90734..c64af3e41f 100644 --- a/models/auth/auth_token.go +++ b/models/auth/auth_token.go @@ -15,12 +15,31 @@ import ( "code.gitea.io/gitea/modules/util" ) +type AuthorizationPurpose string + +var ( + // Used to store long term authorization tokens. + LongTermAuthorization AuthorizationPurpose = "long_term_authorization" + + // Used to activate a user account. + UserActivation AuthorizationPurpose = "user_activation" + + // Used to reset the password. + PasswordReset AuthorizationPurpose = "password_reset" +) + +// Used to activate the specified email address for a user. +func EmailActivation(email string) AuthorizationPurpose { + return AuthorizationPurpose("email_activation:" + email) +} + // AuthorizationToken represents a authorization token to a user. type AuthorizationToken struct { ID int64 `xorm:"pk autoincr"` UID int64 `xorm:"INDEX"` LookupKey string `xorm:"INDEX UNIQUE"` HashedValidator string + Purpose AuthorizationPurpose `xorm:"NOT NULL DEFAULT 'long_term_authorization'"` Expiry timeutil.TimeStamp } @@ -41,7 +60,7 @@ func (authToken *AuthorizationToken) IsExpired() bool { // GenerateAuthToken generates a new authentication token for the given user. // It returns the lookup key and validator values that should be passed to the // user via a long-term cookie. -func GenerateAuthToken(ctx context.Context, userID int64, expiry timeutil.TimeStamp) (lookupKey, validator string, err error) { +func GenerateAuthToken(ctx context.Context, userID int64, expiry timeutil.TimeStamp, purpose AuthorizationPurpose) (lookupKey, validator string, err error) { // Request 64 random bytes. The first 32 bytes will be used for the lookupKey // and the other 32 bytes will be used for the validator. rBytes, err := util.CryptoRandomBytes(64) @@ -56,14 +75,15 @@ func GenerateAuthToken(ctx context.Context, userID int64, expiry timeutil.TimeSt Expiry: expiry, LookupKey: lookupKey, HashedValidator: HashValidator(rBytes[32:]), + Purpose: purpose, }) return lookupKey, validator, err } // FindAuthToken will find a authorization token via the lookup key. -func FindAuthToken(ctx context.Context, lookupKey string) (*AuthorizationToken, error) { +func FindAuthToken(ctx context.Context, lookupKey string, purpose AuthorizationPurpose) (*AuthorizationToken, error) { var authToken AuthorizationToken - has, err := db.GetEngine(ctx).Where("lookup_key = ?", lookupKey).Get(&authToken) + has, err := db.GetEngine(ctx).Where("lookup_key = ? AND purpose = ?", lookupKey, purpose).Get(&authToken) if err != nil { return nil, err } else if !has { diff --git a/models/user/email_address.go b/models/user/email_address.go index 85824fcdcb..1359f36523 100644 --- a/models/user/email_address.go +++ b/models/user/email_address.go @@ -12,7 +12,6 @@ import ( "strings" "code.gitea.io/gitea/models/db" - "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" @@ -360,25 +359,6 @@ func MakeEmailPrimary(ctx context.Context, email *EmailAddress) error { return MakeEmailPrimaryWithUser(ctx, user, email) } -// VerifyActiveEmailCode verifies active email code when active account -func VerifyActiveEmailCode(ctx context.Context, code, email string) *EmailAddress { - minutes := setting.Service.ActiveCodeLives - - if user := GetVerifyUser(ctx, code); user != nil { - // time limit code - prefix := code[:base.TimeLimitCodeLength] - data := fmt.Sprintf("%d%s%s%s%s", user.ID, email, user.LowerName, user.Passwd, user.Rands) - - if base.VerifyTimeLimitCode(data, minutes, prefix) { - emailAddress := &EmailAddress{UID: user.ID, Email: email} - if has, _ := db.GetEngine(ctx).Get(emailAddress); has { - return emailAddress - } - } - } - return nil -} - // SearchEmailOrderBy is used to sort the results from SearchEmails() type SearchEmailOrderBy string diff --git a/models/user/user.go b/models/user/user.go index 3123a28e37..74e7c64d2f 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -6,7 +6,9 @@ package user import ( "context" + "crypto/subtle" "encoding/hex" + "errors" "fmt" "net/url" "path/filepath" @@ -308,15 +310,14 @@ func (u *User) OrganisationLink() string { return setting.AppSubURL + "/org/" + url.PathEscape(u.Name) } -// GenerateEmailActivateCode generates an activate code based on user information and given e-mail. -func (u *User) GenerateEmailActivateCode(email string) string { - code := base.CreateTimeLimitCode( - fmt.Sprintf("%d%s%s%s%s", u.ID, email, u.LowerName, u.Passwd, u.Rands), - setting.Service.ActiveCodeLives, nil) - - // Add tail hex username - code += hex.EncodeToString([]byte(u.LowerName)) - return code +// GenerateEmailAuthorizationCode generates an activation code based for the user for the specified purpose. +// The standard expiry is ActiveCodeLives minutes. +func (u *User) GenerateEmailAuthorizationCode(ctx context.Context, purpose auth.AuthorizationPurpose) (string, error) { + lookup, validator, err := auth.GenerateAuthToken(ctx, u.ID, timeutil.TimeStampNow().Add(int64(setting.Service.ActiveCodeLives)*60), purpose) + if err != nil { + return "", err + } + return lookup + ":" + validator, nil } // GetUserFollowers returns range of user's followers. @@ -786,38 +787,50 @@ func countUsers(ctx context.Context, opts *CountUserFilter) int64 { return count } -// GetVerifyUser get user by verify code -func GetVerifyUser(ctx context.Context, code string) (user *User) { - if len(code) <= base.TimeLimitCodeLength { - return nil +// VerifyUserActiveCode verifies that the code is valid for the given purpose for this user. +// If delete is specified, the token will be deleted. +func VerifyUserAuthorizationToken(ctx context.Context, code string, purpose auth.AuthorizationPurpose, delete bool) (*User, error) { + lookupKey, validator, found := strings.Cut(code, ":") + if !found { + return nil, nil } - // use tail hex username query user - hexStr := code[base.TimeLimitCodeLength:] - if b, err := hex.DecodeString(hexStr); err == nil { - if user, err = GetUserByName(ctx, string(b)); user != nil { - return user + authToken, err := auth.FindAuthToken(ctx, lookupKey, purpose) + if err != nil { + if errors.Is(err, util.ErrNotExist) { + return nil, nil } - log.Error("user.getVerifyUser: %v", err) + return nil, err } - return nil -} + if authToken.IsExpired() { + return nil, auth.DeleteAuthToken(ctx, authToken) + } -// VerifyUserActiveCode verifies active code when active account -func VerifyUserActiveCode(ctx context.Context, code string) (user *User) { - minutes := setting.Service.ActiveCodeLives + rawValidator, err := hex.DecodeString(validator) + if err != nil { + return nil, err + } - if user = GetVerifyUser(ctx, code); user != nil { - // time limit code - prefix := code[:base.TimeLimitCodeLength] - data := fmt.Sprintf("%d%s%s%s%s", user.ID, user.Email, user.LowerName, user.Passwd, user.Rands) + if subtle.ConstantTimeCompare([]byte(authToken.HashedValidator), []byte(auth.HashValidator(rawValidator))) == 0 { + return nil, errors.New("validator doesn't match") + } - if base.VerifyTimeLimitCode(data, minutes, prefix) { - return user + u, err := GetUserByID(ctx, authToken.UID) + if err != nil { + if IsErrUserNotExist(err) { + return nil, nil + } + return nil, err + } + + if delete { + if err := auth.DeleteAuthToken(ctx, authToken); err != nil { + return nil, err } } - return nil + + return u, nil } // ValidateUser check if user is valid to insert / update into database diff --git a/models/user/user_test.go b/models/user/user_test.go index 729fcd0263..1359e9e218 100644 --- a/models/user/user_test.go +++ b/models/user/user_test.go @@ -5,8 +5,9 @@ package user_test import ( "context" + "crypto/rand" + "encoding/hex" "fmt" - "math/rand" "strings" "testing" "time" @@ -19,7 +20,9 @@ import ( "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/modules/timeutil" + "code.gitea.io/gitea/modules/util" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -543,3 +546,66 @@ func Test_NormalizeUserFromEmail(t *testing.T) { } } } + +func TestGenerateEmailAuthorizationCode(t *testing.T) { + defer test.MockVariableValue(&setting.Service.ActiveCodeLives, 2)() + require.NoError(t, unittest.PrepareTestDatabase()) + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + code, err := user.GenerateEmailAuthorizationCode(db.DefaultContext, auth.UserActivation) + require.NoError(t, err) + + lookupKey, validator, ok := strings.Cut(code, ":") + assert.True(t, ok) + + rawValidator, err := hex.DecodeString(validator) + require.NoError(t, err) + + authToken, err := auth.FindAuthToken(db.DefaultContext, lookupKey, auth.UserActivation) + require.NoError(t, err) + assert.False(t, authToken.IsExpired()) + assert.EqualValues(t, authToken.HashedValidator, auth.HashValidator(rawValidator)) + + authToken.Expiry = authToken.Expiry.Add(-int64(setting.Service.ActiveCodeLives) * 60) + assert.True(t, authToken.IsExpired()) +} + +func TestVerifyUserAuthorizationToken(t *testing.T) { + defer test.MockVariableValue(&setting.Service.ActiveCodeLives, 2)() + require.NoError(t, unittest.PrepareTestDatabase()) + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + code, err := user.GenerateEmailAuthorizationCode(db.DefaultContext, auth.UserActivation) + require.NoError(t, err) + + lookupKey, _, ok := strings.Cut(code, ":") + assert.True(t, ok) + + t.Run("Wrong purpose", func(t *testing.T) { + u, err := user_model.VerifyUserAuthorizationToken(db.DefaultContext, code, auth.PasswordReset, false) + require.NoError(t, err) + assert.Nil(t, u) + }) + + t.Run("No delete", func(t *testing.T) { + u, err := user_model.VerifyUserAuthorizationToken(db.DefaultContext, code, auth.UserActivation, false) + require.NoError(t, err) + assert.EqualValues(t, user.ID, u.ID) + + authToken, err := auth.FindAuthToken(db.DefaultContext, lookupKey, auth.UserActivation) + require.NoError(t, err) + assert.NotNil(t, authToken) + }) + + t.Run("Delete", func(t *testing.T) { + u, err := user_model.VerifyUserAuthorizationToken(db.DefaultContext, code, auth.UserActivation, true) + require.NoError(t, err) + assert.EqualValues(t, user.ID, u.ID) + + authToken, err := auth.FindAuthToken(db.DefaultContext, lookupKey, auth.UserActivation) + require.ErrorIs(t, err, util.ErrNotExist) + assert.Nil(t, authToken) + }) +} diff --git a/modules/base/tool.go b/modules/base/tool.go index e4c3fb1818..4b5ae4d5da 100644 --- a/modules/base/tool.go +++ b/modules/base/tool.go @@ -15,12 +15,10 @@ import ( "runtime" "strconv" "strings" - "time" "unicode/utf8" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" - "code.gitea.io/gitea/modules/setting" "github.com/dustin/go-humanize" ) @@ -61,67 +59,6 @@ func BasicAuthDecode(encoded string) (string, string, error) { return auth[0], auth[1], nil } -// VerifyTimeLimitCode verify time limit code -func VerifyTimeLimitCode(data string, minutes int, code string) bool { - if len(code) <= 18 { - return false - } - - // split code - start := code[:12] - lives := code[12:18] - if d, err := strconv.ParseInt(lives, 10, 0); err == nil { - minutes = int(d) - } - - // right active code - retCode := CreateTimeLimitCode(data, minutes, start) - if retCode == code && minutes > 0 { - // check time is expired or not - before, _ := time.ParseInLocation("200601021504", start, time.Local) - now := time.Now() - if before.Add(time.Minute*time.Duration(minutes)).Unix() > now.Unix() { - return true - } - } - - return false -} - -// TimeLimitCodeLength default value for time limit code -const TimeLimitCodeLength = 12 + 6 + 40 - -// CreateTimeLimitCode create a time limit code -// code format: 12 length date time string + 6 minutes string + 40 sha1 encoded string -func CreateTimeLimitCode(data string, minutes int, startInf any) string { - format := "200601021504" - - var start, end time.Time - var startStr, endStr string - - if startInf == nil { - // Use now time create code - start = time.Now() - startStr = start.Format(format) - } else { - // use start string create code - startStr = startInf.(string) - start, _ = time.ParseInLocation(format, startStr, time.Local) - startStr = start.Format(format) - } - - end = start.Add(time.Minute * time.Duration(minutes)) - endStr = end.Format(format) - - // create sha1 encode string - sh := sha1.New() - _, _ = sh.Write([]byte(fmt.Sprintf("%s%s%s%s%d", data, hex.EncodeToString(setting.GetGeneralTokenSigningSecret()), startStr, endStr, minutes))) - encoded := hex.EncodeToString(sh.Sum(nil)) - - code := fmt.Sprintf("%s%06d%s", startStr, minutes, encoded) - return code -} - // FileSize calculates the file size and generate user-friendly string. func FileSize(s int64) string { return humanize.IBytes(uint64(s)) diff --git a/modules/base/tool_test.go b/modules/base/tool_test.go index 9b2b24bad0..5c09c51b15 100644 --- a/modules/base/tool_test.go +++ b/modules/base/tool_test.go @@ -5,7 +5,6 @@ package base import ( "testing" - "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -45,46 +44,6 @@ func TestBasicAuthDecode(t *testing.T) { require.Error(t, err) } -func TestVerifyTimeLimitCode(t *testing.T) { - tc := []struct { - data string - minutes int - code string - valid bool - }{{ - data: "data", - minutes: 2, - code: testCreateTimeLimitCode(t, "data", 2), - valid: true, - }, { - data: "abc123-ß", - minutes: 1, - code: testCreateTimeLimitCode(t, "abc123-ß", 1), - valid: true, - }, { - data: "data", - minutes: 2, - code: "2021012723240000005928251dac409d2c33a6eb82c63410aaad569bed", - valid: false, - }} - for _, test := range tc { - actualValid := VerifyTimeLimitCode(test.data, test.minutes, test.code) - assert.Equal(t, test.valid, actualValid, "data: '%s' code: '%s' should be valid: %t", test.data, test.code, test.valid) - } -} - -func testCreateTimeLimitCode(t *testing.T, data string, m int) string { - result0 := CreateTimeLimitCode(data, m, nil) - result1 := CreateTimeLimitCode(data, m, time.Now().Format("200601021504")) - result2 := CreateTimeLimitCode(data, m, time.Unix(time.Now().Unix()+int64(time.Minute)*int64(m), 0).Format("200601021504")) - - assert.Equal(t, result0, result1) - assert.NotEqual(t, result0, result2) - - assert.NotEmpty(t, result0) - return result0 -} - func TestFileSize(t *testing.T) { var size int64 = 512 assert.Equal(t, "512 B", FileSize(size)) diff --git a/routers/web/auth/auth.go b/routers/web/auth/auth.go index cb88560233..def5f5b8f7 100644 --- a/routers/web/auth/auth.go +++ b/routers/web/auth/auth.go @@ -5,8 +5,6 @@ package auth import ( - "crypto/subtle" - "encoding/hex" "errors" "fmt" "net/http" @@ -62,38 +60,11 @@ func autoSignIn(ctx *context.Context) (bool, error) { return false, nil } - lookupKey, validator, found := strings.Cut(authCookie, ":") - if !found { - return false, nil - } - - authToken, err := auth.FindAuthToken(ctx, lookupKey) + u, err := user_model.VerifyUserAuthorizationToken(ctx, authCookie, auth.LongTermAuthorization, false) if err != nil { - if errors.Is(err, util.ErrNotExist) { - return false, nil - } - return false, err + return false, fmt.Errorf("VerifyUserAuthorizationToken: %w", err) } - - if authToken.IsExpired() { - err = auth.DeleteAuthToken(ctx, authToken) - return false, err - } - - rawValidator, err := hex.DecodeString(validator) - if err != nil { - return false, err - } - - if subtle.ConstantTimeCompare([]byte(authToken.HashedValidator), []byte(auth.HashValidator(rawValidator))) == 0 { - return false, nil - } - - u, err := user_model.GetUserByID(ctx, authToken.UID) - if err != nil { - if !user_model.IsErrUserNotExist(err) { - return false, fmt.Errorf("GetUserByID: %w", err) - } + if u == nil { return false, nil } @@ -635,7 +606,10 @@ func handleUserCreated(ctx *context.Context, u *user_model.User, gothUser *goth. return false } - mailer.SendActivateAccountMail(ctx.Locale, u) + if err := mailer.SendActivateAccountMail(ctx, u); err != nil { + ctx.ServerError("SendActivateAccountMail", err) + return false + } ctx.Data["IsSendRegisterMail"] = true ctx.Data["Email"] = u.Email @@ -676,7 +650,10 @@ func Activate(ctx *context.Context) { ctx.Data["ResendLimited"] = true } else { ctx.Data["ActiveCodeLives"] = timeutil.MinutesToFriendly(setting.Service.ActiveCodeLives, ctx.Locale) - mailer.SendActivateAccountMail(ctx.Locale, ctx.Doer) + if err := mailer.SendActivateAccountMail(ctx, ctx.Doer); err != nil { + ctx.ServerError("SendActivateAccountMail", err) + return + } if err := ctx.Cache.Put(cacheKey+ctx.Doer.LowerName, ctx.Doer.LowerName, 180); err != nil { log.Error("Set cache(MailResendLimit) fail: %v", err) @@ -689,7 +666,12 @@ func Activate(ctx *context.Context) { return } - user := user_model.VerifyUserActiveCode(ctx, code) + user, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.UserActivation, false) + if err != nil { + ctx.ServerError("VerifyUserAuthorizationToken", err) + return + } + // if code is wrong if user == nil { ctx.Data["IsCodeInvalid"] = true @@ -753,7 +735,12 @@ func ActivatePost(ctx *context.Context) { return } - user := user_model.VerifyUserActiveCode(ctx, code) + user, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.UserActivation, true) + if err != nil { + ctx.ServerError("VerifyUserAuthorizationToken", err) + return + } + // if code is wrong if user == nil { ctx.Data["IsCodeInvalid"] = true @@ -837,23 +824,32 @@ func ActivateEmail(ctx *context.Context) { code := ctx.FormString("code") emailStr := ctx.FormString("email") - // Verify code. - if email := user_model.VerifyActiveEmailCode(ctx, code, emailStr); email != nil { - if err := user_model.ActivateEmail(ctx, email); err != nil { - ctx.ServerError("ActivateEmail", err) - return - } - - log.Trace("Email activated: %s", email.Email) - ctx.Flash.Success(ctx.Tr("settings.add_email_success")) - - if u, err := user_model.GetUserByID(ctx, email.UID); err != nil { - log.Warn("GetUserByID: %d", email.UID) - } else { - // Allow user to validate more emails - _ = ctx.Cache.Delete("MailResendLimit_" + u.LowerName) - } + u, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.EmailActivation(emailStr), true) + if err != nil { + ctx.ServerError("VerifyUserAuthorizationToken", err) + return } + if u == nil { + ctx.Redirect(setting.AppSubURL + "/user/settings/account") + return + } + + email, err := user_model.GetEmailAddressOfUser(ctx, emailStr, u.ID) + if err != nil { + ctx.ServerError("GetEmailAddressOfUser", err) + return + } + + if err := user_model.ActivateEmail(ctx, email); err != nil { + ctx.ServerError("ActivateEmail", err) + return + } + + log.Trace("Email activated: %s", email.Email) + ctx.Flash.Success(ctx.Tr("settings.add_email_success")) + + // Allow user to validate more emails + _ = ctx.Cache.Delete("MailResendLimit_" + u.LowerName) // FIXME: e-mail verification does not require the user to be logged in, // so this could be redirecting to the login page. diff --git a/routers/web/auth/password.go b/routers/web/auth/password.go index 53494df630..7a7f29b93b 100644 --- a/routers/web/auth/password.go +++ b/routers/web/auth/password.go @@ -87,7 +87,10 @@ func ForgotPasswdPost(ctx *context.Context) { return } - mailer.SendResetPasswordMail(u) + if err := mailer.SendResetPasswordMail(ctx, u); err != nil { + ctx.ServerError("SendResetPasswordMail", err) + return + } if err = ctx.Cache.Put("MailResendLimit_"+u.LowerName, u.LowerName, 180); err != nil { log.Error("Set cache(MailResendLimit) fail: %v", err) @@ -98,7 +101,7 @@ func ForgotPasswdPost(ctx *context.Context) { ctx.HTML(http.StatusOK, tplForgotPassword) } -func commonResetPassword(ctx *context.Context) (*user_model.User, *auth.TwoFactor) { +func commonResetPassword(ctx *context.Context, shouldDeleteToken bool) (*user_model.User, *auth.TwoFactor) { code := ctx.FormString("code") ctx.Data["Title"] = ctx.Tr("auth.reset_password") @@ -114,7 +117,12 @@ func commonResetPassword(ctx *context.Context) (*user_model.User, *auth.TwoFacto } // Fail early, don't frustrate the user - u := user_model.VerifyUserActiveCode(ctx, code) + u, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.PasswordReset, shouldDeleteToken) + if err != nil { + ctx.ServerError("VerifyUserAuthorizationToken", err) + return nil, nil + } + if u == nil { ctx.Flash.Error(ctx.Tr("auth.invalid_code_forgot_password", fmt.Sprintf("%s/user/forgot_password", setting.AppSubURL)), true) return nil, nil @@ -146,7 +154,7 @@ func commonResetPassword(ctx *context.Context) (*user_model.User, *auth.TwoFacto func ResetPasswd(ctx *context.Context) { ctx.Data["IsResetForm"] = true - commonResetPassword(ctx) + commonResetPassword(ctx, false) if ctx.Written() { return } @@ -156,7 +164,7 @@ func ResetPasswd(ctx *context.Context) { // ResetPasswdPost response from account recovery request func ResetPasswdPost(ctx *context.Context) { - u, twofa := commonResetPassword(ctx) + u, twofa := commonResetPassword(ctx, true) if ctx.Written() { return } diff --git a/routers/web/user/setting/account.go b/routers/web/user/setting/account.go index 6f412aed7f..dffe41cff0 100644 --- a/routers/web/user/setting/account.go +++ b/routers/web/user/setting/account.go @@ -146,9 +146,15 @@ func EmailPost(ctx *context.Context) { return } // Only fired when the primary email is inactive (Wrong state) - mailer.SendActivateAccountMail(ctx.Locale, ctx.Doer) + if err := mailer.SendActivateAccountMail(ctx, ctx.Doer); err != nil { + ctx.ServerError("SendActivateAccountMail", err) + return + } } else { - mailer.SendActivateEmailMail(ctx.Doer, email.Email) + if err := mailer.SendActivateEmailMail(ctx, ctx.Doer, email.Email); err != nil { + ctx.ServerError("SendActivateEmailMail", err) + return + } } address = email.Email @@ -209,7 +215,10 @@ func EmailPost(ctx *context.Context) { // Send confirmation email if setting.Service.RegisterEmailConfirm { - mailer.SendActivateEmailMail(ctx.Doer, form.Email) + if err := mailer.SendActivateEmailMail(ctx, ctx.Doer, form.Email); err != nil { + ctx.ServerError("SendActivateEmailMail", err) + return + } if err := ctx.Cache.Put("MailResendLimit_"+ctx.Doer.LowerName, ctx.Doer.LowerName, 180); err != nil { log.Error("Set cache(MailResendLimit) fail: %v", err) } diff --git a/services/context/context_cookie.go b/services/context/context_cookie.go index 39e3218d1b..3699f81071 100644 --- a/services/context/context_cookie.go +++ b/services/context/context_cookie.go @@ -47,7 +47,7 @@ func (ctx *Context) GetSiteCookie(name string) string { // SetLTACookie will generate a LTA token and add it as an cookie. func (ctx *Context) SetLTACookie(u *user_model.User) error { days := 86400 * setting.LogInRememberDays - lookup, validator, err := auth_model.GenerateAuthToken(ctx, u.ID, timeutil.TimeStampNow().Add(int64(days))) + lookup, validator, err := auth_model.GenerateAuthToken(ctx, u.ID, timeutil.TimeStampNow().Add(int64(days)), auth_model.LongTermAuthorization) if err != nil { return err } diff --git a/services/doctor/dbconsistency.go b/services/doctor/dbconsistency.go index 0903ecc2a6..c137e4ca27 100644 --- a/services/doctor/dbconsistency.go +++ b/services/doctor/dbconsistency.go @@ -227,6 +227,9 @@ func checkDBConsistency(ctx context.Context, logger log.Logger, autofix bool) er // find redirects without existing user. genericOrphanCheck("Orphaned Redirects without existing redirect user", "user_redirect", "user", "user_redirect.redirect_user_id=`user`.id"), + // find authorization tokens without existing user + genericOrphanCheck("Authorization token without existing User", + "forgejo_auth_token", "user", "forgejo_auth_token.uid=user.id"), ) for _, c := range consistencyChecks { diff --git a/services/mailer/mail.go b/services/mailer/mail.go index df8bd59a30..c20e2c483c 100644 --- a/services/mailer/mail.go +++ b/services/mailer/mail.go @@ -17,6 +17,7 @@ import ( "time" activities_model "code.gitea.io/gitea/models/activities" + auth_model "code.gitea.io/gitea/models/auth" issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" @@ -64,7 +65,7 @@ func SendTestMail(email string) error { } // sendUserMail sends a mail to the user -func sendUserMail(language string, u *user_model.User, tpl base.TplName, code, subject, info string) { +func sendUserMail(language string, u *user_model.User, tpl base.TplName, code, subject, info string) error { locale := translation.NewLocale(language) data := map[string]any{ "locale": locale, @@ -78,47 +79,66 @@ func sendUserMail(language string, u *user_model.User, tpl base.TplName, code, s var content bytes.Buffer if err := bodyTemplates.ExecuteTemplate(&content, string(tpl), data); err != nil { - log.Error("Template: %v", err) - return + return err } msg := NewMessage(u.Email, subject, content.String()) msg.Info = fmt.Sprintf("UID: %d, %s", u.ID, info) SendAsync(msg) + return nil } // SendActivateAccountMail sends an activation mail to the user (new user registration) -func SendActivateAccountMail(locale translation.Locale, u *user_model.User) { +func SendActivateAccountMail(ctx context.Context, u *user_model.User) error { if setting.MailService == nil { // No mail service configured - return + return nil } - sendUserMail(locale.Language(), u, mailAuthActivate, u.GenerateEmailActivateCode(u.Email), locale.TrString("mail.activate_account"), "activate account") + + locale := translation.NewLocale(u.Language) + code, err := u.GenerateEmailAuthorizationCode(ctx, auth_model.UserActivation) + if err != nil { + return err + } + + return sendUserMail(locale.Language(), u, mailAuthActivate, code, locale.TrString("mail.activate_account"), "activate account") } // SendResetPasswordMail sends a password reset mail to the user -func SendResetPasswordMail(u *user_model.User) { +func SendResetPasswordMail(ctx context.Context, u *user_model.User) error { if setting.MailService == nil { // No mail service configured - return + return nil } + locale := translation.NewLocale(u.Language) - sendUserMail(u.Language, u, mailAuthResetPassword, u.GenerateEmailActivateCode(u.Email), locale.TrString("mail.reset_password"), "recover account") + code, err := u.GenerateEmailAuthorizationCode(ctx, auth_model.PasswordReset) + if err != nil { + return err + } + + return sendUserMail(u.Language, u, mailAuthResetPassword, code, locale.TrString("mail.reset_password"), "recover account") } // SendActivateEmailMail sends confirmation email to confirm new email address -func SendActivateEmailMail(u *user_model.User, email string) { +func SendActivateEmailMail(ctx context.Context, u *user_model.User, email string) error { if setting.MailService == nil { // No mail service configured - return + return nil } + locale := translation.NewLocale(u.Language) + code, err := u.GenerateEmailAuthorizationCode(ctx, auth_model.EmailActivation(email)) + if err != nil { + return err + } + data := map[string]any{ "locale": locale, "DisplayName": u.DisplayName(), "ActiveCodeLives": timeutil.MinutesToFriendly(setting.Service.ActiveCodeLives, locale), - "Code": u.GenerateEmailActivateCode(email), + "Code": code, "Email": email, "Language": locale.Language(), } @@ -126,14 +146,14 @@ func SendActivateEmailMail(u *user_model.User, email string) { var content bytes.Buffer if err := bodyTemplates.ExecuteTemplate(&content, string(mailAuthActivateEmail), data); err != nil { - log.Error("Template: %v", err) - return + return err } msg := NewMessage(email, locale.TrString("mail.activate_email"), content.String()) msg.Info = fmt.Sprintf("UID: %d, activate email", u.ID) SendAsync(msg) + return nil } // SendRegisterNotifyMail triggers a notify e-mail by admin created a account. diff --git a/services/user/delete.go b/services/user/delete.go index e890990994..647ca2c8f6 100644 --- a/services/user/delete.go +++ b/services/user/delete.go @@ -96,6 +96,7 @@ func deleteUser(ctx context.Context, u *user_model.User, purge bool) (err error) &user_model.BlockedUser{BlockID: u.ID}, &user_model.BlockedUser{UserID: u.ID}, &actions_model.ActionRunnerToken{OwnerID: u.ID}, + &auth_model.AuthorizationToken{UID: u.ID}, ); err != nil { return fmt.Errorf("deleteBeans: %w", err) } diff --git a/tests/integration/auth_token_test.go b/tests/integration/auth_token_test.go index 2c39c87da2..d1fd5dda83 100644 --- a/tests/integration/auth_token_test.go +++ b/tests/integration/auth_token_test.go @@ -84,7 +84,7 @@ func TestLTACookie(t *testing.T) { assert.True(t, found) rawValidator, err := hex.DecodeString(validator) require.NoError(t, err) - unittest.AssertExistsAndLoadBean(t, &auth.AuthorizationToken{LookupKey: lookupKey, HashedValidator: auth.HashValidator(rawValidator), UID: user.ID}) + unittest.AssertExistsAndLoadBean(t, &auth.AuthorizationToken{LookupKey: lookupKey, HashedValidator: auth.HashValidator(rawValidator), UID: user.ID, Purpose: auth.LongTermAuthorization}) // Check if the LTA cookie it provides authentication. // If LTA cookie provides authentication /user/login shouldn't return status 200. @@ -143,7 +143,7 @@ func TestLTAExpiry(t *testing.T) { assert.True(t, found) // Ensure it's not expired. - lta := unittest.AssertExistsAndLoadBean(t, &auth.AuthorizationToken{UID: user.ID, LookupKey: lookupKey}) + lta := unittest.AssertExistsAndLoadBean(t, &auth.AuthorizationToken{UID: user.ID, LookupKey: lookupKey, Purpose: auth.LongTermAuthorization}) assert.False(t, lta.IsExpired()) // Manually stub LTA's expiry. @@ -151,7 +151,7 @@ func TestLTAExpiry(t *testing.T) { require.NoError(t, err) // Ensure it's expired. - lta = unittest.AssertExistsAndLoadBean(t, &auth.AuthorizationToken{UID: user.ID, LookupKey: lookupKey}) + lta = unittest.AssertExistsAndLoadBean(t, &auth.AuthorizationToken{UID: user.ID, LookupKey: lookupKey, Purpose: auth.LongTermAuthorization}) assert.True(t, lta.IsExpired()) // Should return 200 OK, because LTA doesn't provide authorization anymore. @@ -160,5 +160,5 @@ func TestLTAExpiry(t *testing.T) { session.MakeRequest(t, req, http.StatusOK) // Ensure it's deleted. - unittest.AssertNotExistsBean(t, &auth.AuthorizationToken{UID: user.ID, LookupKey: lookupKey}) + unittest.AssertNotExistsBean(t, &auth.AuthorizationToken{UID: user.ID, LookupKey: lookupKey, Purpose: auth.LongTermAuthorization}) } diff --git a/tests/integration/org_team_invite_test.go b/tests/integration/org_team_invite_test.go index d04199a2c1..2fe296e8c3 100644 --- a/tests/integration/org_team_invite_test.go +++ b/tests/integration/org_team_invite_test.go @@ -10,6 +10,7 @@ import ( "strings" "testing" + "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/unittest" @@ -293,8 +294,10 @@ func TestOrgTeamEmailInviteRedirectsNewUserWithActivation(t *testing.T) { require.NoError(t, err) session.jar.SetCookies(baseURL, cr.Cookies()) - activateURL := fmt.Sprintf("/user/activate?code=%s", user.GenerateEmailActivateCode("doesnotexist@example.com")) - req = NewRequestWithValues(t, "POST", activateURL, map[string]string{ + code, err := user.GenerateEmailAuthorizationCode(db.DefaultContext, auth.UserActivation) + require.NoError(t, err) + + req = NewRequestWithValues(t, "POST", "/user/activate?code="+url.QueryEscape(code), map[string]string{ "password": "examplePassword!1", }) diff --git a/tests/integration/user_test.go b/tests/integration/user_test.go index 349fe38fea..9009f7cf0b 100644 --- a/tests/integration/user_test.go +++ b/tests/integration/user_test.go @@ -5,12 +5,16 @@ package integration import ( + "bytes" + "encoding/hex" "fmt" "net/http" + "net/url" "strings" "testing" auth_model "code.gitea.io/gitea/models/auth" + "code.gitea.io/gitea/models/db" issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" unit_model "code.gitea.io/gitea/models/unit" @@ -20,9 +24,11 @@ import ( api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/modules/translation" + "code.gitea.io/gitea/services/mailer" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestViewUser(t *testing.T) { @@ -612,3 +618,171 @@ func TestUserPronouns(t *testing.T) { assert.EqualValues(t, "user2", userName) }) } + +func TestUserActivate(t *testing.T) { + defer tests.PrepareTestEnv(t)() + defer test.MockVariableValue(&setting.Service.RegisterEmailConfirm, true)() + + called := false + code := "" + defer test.MockVariableValue(&mailer.SendAsync, func(msgs ...*mailer.Message) { + called = true + assert.Len(t, msgs, 1) + assert.Equal(t, `doesnotexist@example.com`, msgs[0].To) + assert.EqualValues(t, translation.NewLocale("en-US").Tr("mail.activate_account"), msgs[0].Subject) + + messageDoc := NewHTMLParser(t, bytes.NewBuffer([]byte(msgs[0].Body))) + link, ok := messageDoc.Find("a").Attr("href") + assert.True(t, ok) + u, err := url.Parse(link) + require.NoError(t, err) + code = u.Query()["code"][0] + })() + + session := emptyTestSession(t) + req := NewRequestWithValues(t, "POST", "/user/sign_up", map[string]string{ + "_csrf": GetCSRF(t, session, "/user/sign_up"), + "user_name": "doesnotexist", + "email": "doesnotexist@example.com", + "password": "examplePassword!1", + "retype": "examplePassword!1", + }) + session.MakeRequest(t, req, http.StatusOK) + assert.True(t, called) + + queryCode, err := url.QueryUnescape(code) + require.NoError(t, err) + + lookupKey, validator, ok := strings.Cut(queryCode, ":") + assert.True(t, ok) + + rawValidator, err := hex.DecodeString(validator) + require.NoError(t, err) + + authToken, err := auth_model.FindAuthToken(db.DefaultContext, lookupKey, auth_model.UserActivation) + require.NoError(t, err) + assert.False(t, authToken.IsExpired()) + assert.EqualValues(t, authToken.HashedValidator, auth_model.HashValidator(rawValidator)) + + req = NewRequest(t, "POST", "/user/activate?code="+code) + session.MakeRequest(t, req, http.StatusOK) + + unittest.AssertNotExistsBean(t, &auth_model.AuthorizationToken{ID: authToken.ID}) + unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "doesnotexist", IsActive: true}) +} + +func TestUserPasswordReset(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + called := false + code := "" + defer test.MockVariableValue(&mailer.SendAsync, func(msgs ...*mailer.Message) { + if called { + return + } + called = true + + assert.Len(t, msgs, 1) + assert.Equal(t, user2.Email, msgs[0].To) + assert.EqualValues(t, translation.NewLocale("en-US").Tr("mail.reset_password"), msgs[0].Subject) + + messageDoc := NewHTMLParser(t, bytes.NewBuffer([]byte(msgs[0].Body))) + link, ok := messageDoc.Find("a").Attr("href") + assert.True(t, ok) + u, err := url.Parse(link) + require.NoError(t, err) + code = u.Query()["code"][0] + })() + + session := emptyTestSession(t) + req := NewRequestWithValues(t, "POST", "/user/forgot_password", map[string]string{ + "_csrf": GetCSRF(t, session, "/user/forgot_password"), + "email": user2.Email, + }) + session.MakeRequest(t, req, http.StatusOK) + assert.True(t, called) + + queryCode, err := url.QueryUnescape(code) + require.NoError(t, err) + + lookupKey, validator, ok := strings.Cut(queryCode, ":") + assert.True(t, ok) + + rawValidator, err := hex.DecodeString(validator) + require.NoError(t, err) + + authToken, err := auth_model.FindAuthToken(db.DefaultContext, lookupKey, auth_model.PasswordReset) + require.NoError(t, err) + assert.False(t, authToken.IsExpired()) + assert.EqualValues(t, authToken.HashedValidator, auth_model.HashValidator(rawValidator)) + + req = NewRequestWithValues(t, "POST", "/user/recover_account", map[string]string{ + "_csrf": GetCSRF(t, session, "/user/recover_account"), + "code": code, + "password": "new_password", + }) + session.MakeRequest(t, req, http.StatusSeeOther) + + unittest.AssertNotExistsBean(t, &auth_model.AuthorizationToken{ID: authToken.ID}) + assert.True(t, unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}).ValidatePassword("new_password")) +} + +func TestActivateEmailAddress(t *testing.T) { + defer tests.PrepareTestEnv(t)() + defer test.MockVariableValue(&setting.Service.RegisterEmailConfirm, true)() + + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + called := false + code := "" + defer test.MockVariableValue(&mailer.SendAsync, func(msgs ...*mailer.Message) { + if called { + return + } + called = true + + assert.Len(t, msgs, 1) + assert.Equal(t, "newemail@example.org", msgs[0].To) + assert.EqualValues(t, translation.NewLocale("en-US").Tr("mail.activate_email"), msgs[0].Subject) + + messageDoc := NewHTMLParser(t, bytes.NewBuffer([]byte(msgs[0].Body))) + link, ok := messageDoc.Find("a").Attr("href") + assert.True(t, ok) + u, err := url.Parse(link) + require.NoError(t, err) + code = u.Query()["code"][0] + })() + + session := loginUser(t, user2.Name) + req := NewRequestWithValues(t, "POST", "/user/settings/account/email", map[string]string{ + "_csrf": GetCSRF(t, session, "/user/settings"), + "email": "newemail@example.org", + }) + session.MakeRequest(t, req, http.StatusSeeOther) + assert.True(t, called) + + queryCode, err := url.QueryUnescape(code) + require.NoError(t, err) + + lookupKey, validator, ok := strings.Cut(queryCode, ":") + assert.True(t, ok) + + rawValidator, err := hex.DecodeString(validator) + require.NoError(t, err) + + authToken, err := auth_model.FindAuthToken(db.DefaultContext, lookupKey, auth_model.EmailActivation("newemail@example.org")) + require.NoError(t, err) + assert.False(t, authToken.IsExpired()) + assert.EqualValues(t, authToken.HashedValidator, auth_model.HashValidator(rawValidator)) + + req = NewRequestWithValues(t, "POST", "/user/activate_email", map[string]string{ + "code": code, + "email": "newemail@example.org", + }) + session.MakeRequest(t, req, http.StatusSeeOther) + + unittest.AssertNotExistsBean(t, &auth_model.AuthorizationToken{ID: authToken.ID}) + unittest.AssertExistsAndLoadBean(t, &user_model.EmailAddress{UID: user2.ID, IsActivated: true, Email: "newemail@example.org"}) +} From 16419b6fc3b554b2c4000169b62e17ab07da5c68 Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 29 Oct 2024 18:01:42 +0100 Subject: [PATCH 8/9] fix: disallow basic authorization when security keys are enrolled - This unifies the security behavior of enrolling security keys with enrolling TOTP as a 2FA method. When TOTP is enrolled, you cannot use basic authorization (user:password) to make API request on behalf of the user, this is now also the case when you enroll security keys. - The usage of access tokens are the only method to make API requests on behalf of the user when a 2FA method is enrolled for the user. - Integration test added. (cherry picked from commit e6bbecb02d47730d3cc630d419fe27ef2fb5cb39) --- services/auth/basic.go | 11 +++++++++++ tests/integration/api_twofa_test.go | 22 ++++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/services/auth/basic.go b/services/auth/basic.go index c8cb1735ee..eebdff2bad 100644 --- a/services/auth/basic.go +++ b/services/auth/basic.go @@ -5,6 +5,7 @@ package auth import ( + "errors" "net/http" "strings" @@ -132,6 +133,16 @@ func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore return nil, err } + hashWebAuthn, err := auth_model.HasWebAuthnRegistrationsByUID(req.Context(), u.ID) + if err != nil { + log.Error("HasWebAuthnRegistrationsByUID: %v", err) + return nil, err + } + + if hashWebAuthn { + return nil, errors.New("Basic authorization is not allowed while having security keys enrolled") + } + if skipper, ok := source.Cfg.(LocalTwoFASkipper); !ok || !skipper.IsSkipLocalTwoFA() { if err := validateTOTP(req, u); err != nil { return nil, err diff --git a/tests/integration/api_twofa_test.go b/tests/integration/api_twofa_test.go index 0bb20255a2..fb1d2badfc 100644 --- a/tests/integration/api_twofa_test.go +++ b/tests/integration/api_twofa_test.go @@ -15,6 +15,7 @@ import ( "code.gitea.io/gitea/tests" "github.com/pquerna/otp/totp" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -58,3 +59,24 @@ func TestAPITwoFactor(t *testing.T) { req.Header.Set("X-Forgejo-OTP", passcode) MakeRequest(t, req, http.StatusOK) } + +func TestAPIWebAuthn(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 32}) + unittest.AssertExistsAndLoadBean(t, &auth_model.WebAuthnCredential{UserID: user.ID}) + + req := NewRequest(t, "GET", "/api/v1/user") + req.SetBasicAuth(user.Name, "notpassword") + + resp := MakeRequest(t, req, http.StatusUnauthorized) + + type userResponse struct { + Message string `json:"message"` + } + var userParsed userResponse + + DecodeJSON(t, resp, &userParsed) + + assert.EqualValues(t, "Basic authorization is not allowed while having security keys enrolled", userParsed.Message) +} From 579560bd7237d4c0601994ee24dd4568b21cae10 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Fri, 15 Nov 2024 10:30:15 +0100 Subject: [PATCH 9/9] chore(release-notes): 15 November 2024 security fixes --- release-notes/5976.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 release-notes/5976.md diff --git a/release-notes/5976.md b/release-notes/5976.md new file mode 100644 index 0000000000..a6a2df1e9c --- /dev/null +++ b/release-notes/5976.md @@ -0,0 +1,8 @@ +fix: [commit](https://codeberg.org/forgejo/forgejo/commit/b770282d458d5ab7301e4a89512fb6563a4329c8) it was possible to use a token sent via email for secondary email validation to reset the password instead. In other words, a token sent for a given action (registration, password reset or secondary email validation) could be used to perform a different action. It is no longer possible to use a token for an action that is different from its original purpose. +fix: [commit](https://codeberg.org/forgejo/forgejo/commit/7480b6072f46dfabd01cd469c180baeb0b76c8be) a fork of a public repository would show in the list of forks, even if its owner was not a public user or organization. Such a fork is now hidden from the list of forks of the public repository. +fix: [commit](https://codeberg.org/forgejo/forgejo/commit/eb066a472c30629c1d984db471373e332ce20700) the members of an organization team with read access to a repository (e.g. to read issues) but no read access to the code could read the RSS or atom feeds which include the commit activity. Reading the RSS or atom feeds is now denied unless the team has read permissions on the code. +fix: [commit](https://codeberg.org/forgejo/forgejo/commit/5b53a150c0f3d6e0b75f9451c898553e2d739cb4) the tokens used when [replying by email to issues or pull requests](https://forgejo.org/docs/v9.0/user/incoming/) were weaker than the [rfc2104 recommendations](https://datatracker.ietf.org/doc/html/rfc2104#section-5). The tokens are now truncated to 128 bits instead of 80 bits. It is no longer possible to reply to emails sent before the upgrade because the weaker tokens are invalid. +fix: [commit](https://codeberg.org/forgejo/forgejo/commit/656351d028994d8edd5b2b0cf061fa0b3ff42621) a registered user could modify the update frequency of any push mirror (e.g. every 4h instead of every 8h). They are now only able to do that if they have administrative permissions on the repository. +fix: [commit](https://codeberg.org/forgejo/forgejo/commit/16419b6fc3b554b2c4000169b62e17ab07da5c68) it was possible to use basic authorization (i.e. user:password) for requests to the API even when security keys were enrolled for a user. It is no longer possible, an application token must be used instead. +fix: [commit](https://codeberg.org/forgejo/forgejo/commit/3a197f7de252793653bc535cd3e83fb79a377603) some markup sanitation rules were not as strong as they could be (e.g. allowing `emoji somethingelse` as well as `emoji`). The rules are now stricter and do not allow for such cases. +fix: [commit](https://codeberg.org/forgejo/forgejo/commit/c2284d871fa69d0ee3de464ebeacd1fb11fc413f) when Forgejo is configured to enable instance wide search (e.g. with [bleve](https://blevesearch.com/)), results found in the repositories of private or limited users were displayed to anonymous visitors. The results found in private or limited organizations were not displayed. The search results found in the repositories of private or limited user are no longer displayed to anonymous visitors.