From 42513398c05ca6bdf71da76cb6f9baaebe8cb924 Mon Sep 17 00:00:00 2001 From: bircni Date: Sat, 6 Jun 2026 17:44:56 +0200 Subject: [PATCH 1/2] fix(lfs): reject unknown SSH LFS sub-verbs to prevent auth bypass (#38008) An authenticated SSH user could pass a malformed sub-verb (e.g. `git-lfs-authenticate badverb`) so getAccessMode falls through to AccessModeNone (0). The permission check in routers/private/serv.go then evaluates `userMode < 0` which is always false, granting a valid LFS JWT for any private repository. The HTTP LFS handler only validates the Op claim on writes, so the token works for downloads. Validate the sub-verb in runServ before calling getAccessMode and fail fast for anything other than upload/download. --- cmd/serv.go | 23 ++++++++++++-------- cmd/serv_test.go | 56 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 9 deletions(-) create mode 100644 cmd/serv_test.go diff --git a/cmd/serv.go b/cmd/serv.go index 93009cd32a3..b39076f6a78 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 00000000000..3bdcba1df37 --- /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" + + "gitea.dev/models/perm" + "gitea.dev/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) + }) + } +} From c43eb7c33a100ffc7b2367adf165f7085e0ccdc5 Mon Sep 17 00:00:00 2001 From: bircni Date: Sun, 7 Jun 2026 00:07:47 +0200 Subject: [PATCH 2/2] fix(auth): do not auto-reactivate disabled users on OAuth2 callback (#38009) The OAuth2 sign-in callback unconditionally set IsActive=true on the local user row whenever the IdP authenticated them, silently undoing an administrator's "Disable Account" action and granting the user a fresh session in the same response. Treat the local IsActive flag as an authoritative admin override: inactive users get a session and are routed through the existing activate / prohibit-login pages by verifyAuthWithOptions, matching the local-credentials sign-in path. Adds an integration regression test that disables a linked local user and asserts the row stays IsActive=false after a full OIDC callback. --------- Co-authored-by: wxiaoguang --- models/unittest/unit_tests.go | 2 +- routers/web/auth/oauth.go | 16 ++++++- services/auth/source/oauth2/source_sync.go | 4 +- tests/integration/auth_oauth2_test.go | 54 ++++++++++++++++++++++ 4 files changed, 71 insertions(+), 5 deletions(-) diff --git a/models/unittest/unit_tests.go b/models/unittest/unit_tests.go index 45ea0d175ea..c9895500214 100644 --- a/models/unittest/unit_tests.go +++ b/models/unittest/unit_tests.go @@ -47,7 +47,7 @@ func OrderBy(orderBy string) any { } func whereOrderConditions(e db.Engine, conditions []any) db.Engine { - orderBy := "id" // query must have the "ORDER BY", otherwise the result is not deterministic + orderBy := "id" // query must have the "ORDER BY", otherwise the result is not deterministic. FIXME: some tables do not have "id" column for _, condition := range conditions { switch cond := condition.(type) { case *testCond: diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index f7d9c3c34ac..06c1b20d578 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -364,9 +364,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) + } } if oauth2Source.GroupTeamMap != "" || oauth2Source.GroupTeamMapRemoval { diff --git a/services/auth/source/oauth2/source_sync.go b/services/auth/source/oauth2/source_sync.go index 8fc58f9ae10..3123e54bbb2 100644 --- a/services/auth/source/oauth2/source_sync.go +++ b/services/auth/source/oauth2/source_sync.go @@ -88,8 +88,8 @@ func (source *Source) refresh(ctx context.Context, provider goth.Provider, u *us } } - // Delete stored tokens, since they are invalid. This - // also provents us from checking this in subsequent runs. + // HINT: OAUTH-AUTO-SYNC-USER-ACTIVATION + // Delete stored tokens, since they are invalid. This also prevents us from checking this in subsequent runs. u.AccessToken = "" u.RefreshToken = "" u.ExpiresAt = time.Time{} diff --git a/tests/integration/auth_oauth2_test.go b/tests/integration/auth_oauth2_test.go index 3bb3d56e7bd..7978c3bd048 100644 --- a/tests/integration/auth_oauth2_test.go +++ b/tests/integration/auth_oauth2_test.go @@ -13,6 +13,7 @@ import ( "time" auth_model "gitea.dev/models/auth" + "gitea.dev/models/db" "gitea.dev/models/unittest" user_model "gitea.dev/models/user" "gitea.dev/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, @@ -191,6 +193,58 @@ func TestOIDCIgnoresStaleExternalLoginLinks(t *testing.T) { }) } +// 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