From 668d3457d430d628ed83a1609dd6f76d2fee0036 Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Mon, 25 May 2026 14:44:58 -0600 Subject: [PATCH] chore: eliminate path-based authentication method enablement Authentication methods which identified if they should be enabled or not based upon req path have been moved into appropriate middleware application to the web routes instead. This keeps configuration for which auth method can be used in one place only -- the middleware -- rather than being split between middlewares and internal filtering. --- modules/web/middleware/request.go | 14 -- routers/api/packages/api.go | 19 +- routers/api/shared/middleware.go | 10 +- routers/web/web.go | 212 ++++++++++++------- services/auth/method/access_token.go | 23 +- services/auth/method/action_runtime_token.go | 6 - services/auth/method/action_task_token.go | 23 +- services/auth/method/auth.go | 39 ---- services/auth/method/auth_test.go | 198 ----------------- services/auth/method/basic.go | 6 - services/auth/method/oauth2.go | 10 - services/auth/method/reverseproxy.go | 9 +- 12 files changed, 178 insertions(+), 391 deletions(-) delete mode 100644 modules/web/middleware/request.go delete mode 100644 services/auth/method/auth_test.go diff --git a/modules/web/middleware/request.go b/modules/web/middleware/request.go deleted file mode 100644 index 0bb155df70..0000000000 --- a/modules/web/middleware/request.go +++ /dev/null @@ -1,14 +0,0 @@ -// Copyright 2020 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package middleware - -import ( - "net/http" - "strings" -) - -// IsAPIPath returns true if the specified URL is an API path -func IsAPIPath(req *http.Request) bool { - return strings.HasPrefix(req.URL.Path, "/api/") -} diff --git a/routers/api/packages/api.go b/routers/api/packages/api.go index 7106521e85..bc79669d51 100644 --- a/routers/api/packages/api.go +++ b/routers/api/packages/api.go @@ -193,9 +193,15 @@ func CommonRoutes() *web.Route { verifyAuth(r, []auth.Method{ &auth_method.OAuth2{}, &auth_method.Basic{}, - &auth_method.AccessToken{}, + &auth_method.AccessToken{ + PermitBasic: true, + PermitBearer: true, + }, &auth_method.ActionRuntimeToken{}, - &auth_method.ActionTaskToken{}, + &auth_method.ActionTaskToken{ + PermitBasic: true, + PermitBearer: true, + }, &nuget.Auth{}, &conan.Auth{}, &chef.Auth{}, @@ -842,9 +848,12 @@ func ContainerRoutes() *web.Route { verifyContainerAuth(r, []auth.Method{ &auth_method.Basic{}, - &auth_method.AccessToken{}, - &auth_method.ActionRuntimeToken{}, - &auth_method.ActionTaskToken{}, + &auth_method.AccessToken{ + PermitBasic: true, + }, + &auth_method.ActionTaskToken{ + PermitBasic: true, + }, &container.Auth{}, &auth_method.AuthorizedIntegration{ // `docker login` can't send a bearer token, so enable reading a token from the password field of diff --git a/routers/api/shared/middleware.go b/routers/api/shared/middleware.go index f6dd30f8b8..10802a89ed 100644 --- a/routers/api/shared/middleware.go +++ b/routers/api/shared/middleware.go @@ -50,9 +50,15 @@ func buildAuthGroup() *auth_method.Group { &auth_method.OAuth2{}, &auth_method.HTTPSign{}, &auth_method.Basic{}, // FIXME: this should be removed once we don't allow basic auth in API - &auth_method.AccessToken{}, + &auth_method.AccessToken{ + PermitBasic: true, + PermitBearer: true, + }, &auth_method.ActionRuntimeToken{}, - &auth_method.ActionTaskToken{}, + &auth_method.ActionTaskToken{ + PermitBasic: true, + PermitBearer: true, + }, &auth_method.AuthorizedIntegration{}, ) if setting.Service.EnableReverseProxyAuthAPI { diff --git a/routers/web/web.go b/routers/web/web.go index df97edd558..7d69f7f978 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -105,24 +105,35 @@ func optionsCorsHandler() func(next http.Handler) http.Handler { // earlier authentication success would prevent later authentication methods from being attempted. func buildAuthGroup() *auth_method.Group { group := auth_method.NewGroup() + if setting.Service.EnableReverseProxyAuth { + // reverseproxy should before Session, otherwise the header will be ignored if user has login + group.Add(&auth_method.ReverseProxy{ + CreateSession: true, + }) + } + group.Add(&auth_method.Session{}) + return group +} - // FIXME: OAuth2, Basic, AccessToken, ActionRuntimeToken, ActionTaskToken authentication methods all use path - // matching so that they only authenticate requests in specific endpoints -- in general these auth methods aren't - // permitted for web endpoints, and aren't supported because the security models don't match. (For example, they - // can have scoped permissions, which web UI handlers don't implement.) A clearer way to do this would be to apply - // specific authentication methods to their auth middlewares. buildGitLfsAuthGroup and buildGitAuthGroup are the - // start of transitioning to that model. Eventually these methods will be fully removed from buildAuthGroup: +// Authentication methods that are applied to "mixed" web URL routes. Mixed routes are those that are primarily +// designed for web browser access, but can also be accessed by API consumers. +func buildMixedAuthGroup() *auth_method.Group { + group := auth_method.NewGroup() group.Add(&auth_method.OAuth2{}) group.Add(&auth_method.Basic{}) - group.Add(&auth_method.AccessToken{}) + group.Add(&auth_method.AccessToken{ + PermitBasic: true, + PermitBearer: true, + }) group.Add(&auth_method.ActionRuntimeToken{}) - group.Add(&auth_method.ActionTaskToken{}) - + group.Add(&auth_method.ActionTaskToken{ + PermitBasic: true, + PermitBearer: true, + }) if setting.Service.EnableReverseProxyAuth { group.Add(&auth_method.ReverseProxy{}) // reverseproxy should before Session, otherwise the header will be ignored if user has login } group.Add(&auth_method.Session{}) - return group } @@ -132,8 +143,17 @@ func buildGitLfsAuthGroup() *auth_method.Group { group := auth_method.NewGroup() group.Add(&auth_method.LFSToken{}) group.Add(&auth_method.Basic{}) - group.Add(&auth_method.AccessToken{}) - group.Add(&auth_method.ActionTaskToken{}) + group.Add(&auth_method.AccessToken{ + PermitBasic: true, + // PermitBearer is left at default `false`. This behaviour is maintained from when one auth method performed + // all the "Basic ..." handling, and one performed all the "Bearer ..." handling, and access to LFS paths + // weren't permitted in the bearer codepath. There isn't a clear reason from Forgejo's perspective to deny + // usage in this case, it's just maintained because it previously worked that way. + }) + group.Add(&auth_method.ActionTaskToken{ + PermitBasic: true, + // PermitBearer is left at default `false` -- same explanation as above for AccessToken. + }) group.Add(&auth_method.AuthorizedIntegration{ // "Authorization: Basic ..." is easier to use for git operations, and already supported for other tokens, so it // is enabled for Authorized Integrations as well: @@ -148,9 +168,15 @@ func buildGitAuthGroup() *auth_method.Group { group := auth_method.NewGroup() group.Add(&auth_method.OAuth2{}) group.Add(&auth_method.Basic{}) - group.Add(&auth_method.AccessToken{}) + group.Add(&auth_method.AccessToken{ + PermitBasic: true, + PermitBearer: true, + }) group.Add(&auth_method.ActionRuntimeToken{}) - group.Add(&auth_method.ActionTaskToken{}) + group.Add(&auth_method.ActionTaskToken{ + PermitBasic: true, + PermitBearer: true, + }) group.Add(&auth_method.AuthorizedIntegration{ // "Authorization: Basic ..." is easier to use for git operations, and already supported for other tokens, so it // is enabled for Authorized Integrations as well: @@ -360,6 +386,11 @@ func Routes() *web.Route { // TODO: GetNotificationCount & GetActiveStopwatch really seem like things that could be folded into Contexter or as helper functions user.GetNotificationCount, repo.GetActiveStopwatch, goGet) + routes.Group("", + func() { + registerMixedRoutes(routes) + }, + gzipMid, common.Sessioner(), context.Contexter(), webAuth(buildMixedAuthGroup()), goGet) routes.Group("", func() { registerGitLFSRoutes(routes) @@ -369,6 +400,15 @@ func Routes() *web.Route { registerGitRoutes(routes) }, gzipMid, common.Sessioner(), context.Contexter(), webAuth(buildGitAuthGroup()), goGet) + // The only endpoint which can only be accessed with the OAuth2 authentication method is /userinfo, extracted here + // so that other auth methods can't be applied to it + routes.Methods( + "GET, POST, OPTIONS", + "/login/oauth/userinfo", + gzipMid, common.Sessioner(), context.Contexter(), + oauth2Enabled, optionsCorsHandler(), ignoreCSRF, webAuth(&auth_method.OAuth2{}), + auth.InfoOAuth) + routes.NotFound( gzipMid, common.Sessioner(), context.Contexter(), webAuth(buildAuthGroup()), // TODO: GetNotificationCount & GetActiveStopwatch really seem like things that could be folded into Contexter or as helper functions @@ -385,26 +425,39 @@ func Routes() *web.Route { return routes } -var ignoreCSRF = verifyAuthWithOptions(&common.VerifyOptions{DisableCSRF: true}) - -// registerRoutes register routes -func registerRoutes(m *web.Route) { - reqSignIn := verifyAuthWithOptions(&common.VerifyOptions{SignInRequired: true}) - reqSignOut := verifyAuthWithOptions(&common.VerifyOptions{SignOutRequired: true}) +var ( + ignoreCSRF = verifyAuthWithOptions(&common.VerifyOptions{DisableCSRF: true}) + reqSignIn = verifyAuthWithOptions(&common.VerifyOptions{SignInRequired: true}) + reqSignOut = verifyAuthWithOptions(&common.VerifyOptions{SignOutRequired: true}) // TODO: rename them to "optSignIn", which means that the "sign-in" could be optional, depends on the VerifyOptions (RequireSignInView) - ignSignIn := verifyAuthWithOptions(&common.VerifyOptions{SignInRequired: setting.Service.RequireSignInView}) - ignExploreSignIn := verifyAuthWithOptions(&common.VerifyOptions{SignInRequired: setting.Service.RequireSignInView || setting.Service.Explore.RequireSigninView}) + ignSignIn = verifyAuthWithOptions(&common.VerifyOptions{SignInRequired: setting.Service.RequireSignInView}) + ignExploreSignIn = verifyAuthWithOptions(&common.VerifyOptions{SignInRequired: setting.Service.RequireSignInView || setting.Service.Explore.RequireSigninView}) - validation.AddBindingRules() + reqRepoAdmin = context.RequireRepoAdmin() + reqRepoCodeWriter = context.RequireRepoWriter(unit.TypeCode) + canEnableEditor = context.CanEnableEditor() + reqRepoCodeReader = context.RequireRepoReader(unit.TypeCode) + reqRepoReleaseWriter = context.RequireRepoWriter(unit.TypeReleases) + reqRepoReleaseReader = context.RequireRepoReader(unit.TypeReleases) + reqRepoWikiWriter = context.RequireRepoWriter(unit.TypeWiki) + reqRepoIssueReader = context.RequireRepoReader(unit.TypeIssues) + reqRepoPullsReader = context.RequireRepoReader(unit.TypePullRequests) + reqRepoIssuesOrPullsWriter = context.RequireRepoWriterOr(unit.TypeIssues, unit.TypePullRequests) + reqRepoIssuesOrPullsReader = context.RequireRepoReaderOr(unit.TypeIssues, unit.TypePullRequests) + reqRepoProjectsReader = context.RequireRepoReader(unit.TypeProjects) + reqRepoProjectsWriter = context.RequireRepoWriter(unit.TypeProjects) + reqRepoActionsReader = context.RequireRepoReader(unit.TypeActions) + reqRepoActionsWriter = context.RequireRepoWriter(unit.TypeActions) + reqRepoDelegateActionTrust = context.RequireRepoDelegateActionTrust() - linkAccountEnabled := func(ctx *context.Context) { + linkAccountEnabled = func(ctx *context.Context) { if !setting.Service.EnableOpenIDSignIn && !setting.Service.EnableOpenIDSignUp && !setting.OAuth2.Enabled { ctx.Error(http.StatusForbidden) return } } - requiredTwoFactor := func(ctx *context.Context) { + requiredTwoFactor = func(ctx *context.Context) { if !ctx.Doer.MustHaveTwoFactor() { return } @@ -418,28 +471,28 @@ func registerRoutes(m *web.Route) { ctx.Data["HideNavbarLinks"] = !hasTwoFactor } - openIDSignInEnabled := func(ctx *context.Context) { + openIDSignInEnabled = func(ctx *context.Context) { if !setting.Service.EnableOpenIDSignIn { ctx.Error(http.StatusForbidden) return } } - openIDSignUpEnabled := func(ctx *context.Context) { + openIDSignUpEnabled = func(ctx *context.Context) { if !setting.Service.EnableOpenIDSignUp { ctx.Error(http.StatusForbidden) return } } - oauth2Enabled := func(ctx *context.Context) { + oauth2Enabled = func(ctx *context.Context) { if !setting.OAuth2.Enabled { ctx.Error(http.StatusForbidden) return } } - reqMilestonesDashboardPageEnabled := func(ctx *context.Context) { + reqMilestonesDashboardPageEnabled = func(ctx *context.Context) { if !setting.Service.ShowMilestonesDashboardPage { ctx.Error(http.StatusForbidden) return @@ -447,49 +500,42 @@ func registerRoutes(m *web.Route) { } // webhooksEnabled requires webhooks to be enabled by admin. - webhooksEnabled := func(ctx *context.Context) { + webhooksEnabled = func(ctx *context.Context) { if setting.DisableWebhooks { ctx.Error(http.StatusForbidden) return } } - federationEnabled := func(ctx *context.Context) { + federationEnabled = func(ctx *context.Context) { if !setting.Federation.Enabled { ctx.Error(http.StatusNotFound) return } } - dlSourceEnabled := func(ctx *context.Context) { - if setting.Repository.DisableDownloadSourceArchives { - ctx.Error(http.StatusNotFound) - return - } - } - - sitemapEnabled := func(ctx *context.Context) { + sitemapEnabled = func(ctx *context.Context) { if !setting.Other.EnableSitemap { ctx.Error(http.StatusNotFound) return } } - packagesEnabled := func(ctx *context.Context) { + packagesEnabled = func(ctx *context.Context) { if !setting.Packages.Enabled { ctx.Error(http.StatusForbidden) return } } - feedEnabled := func(ctx *context.Context) { + feedEnabled = func(ctx *context.Context) { if !setting.Other.EnableFeed { ctx.Error(http.StatusNotFound) return } } - reqUnitAccess := func(unitType unit.Type, accessMode perm.AccessMode, ignoreGlobal bool) func(ctx *context.Context) { + reqUnitAccess = func(unitType unit.Type, accessMode perm.AccessMode, ignoreGlobal bool) func(ctx *context.Context) { return func(ctx *context.Context) { // only check global disabled units when ignoreGlobal is false if !ignoreGlobal && unitType.UnitGlobalDisabled() { @@ -510,6 +556,11 @@ func registerRoutes(m *web.Route) { } } } +) + +// registerRoutes register routes +func registerRoutes(m *web.Route) { + validation.AddBindingRules() addSettingsVariablesRoutes := func() { m.Group("/variables", func() { @@ -640,7 +691,6 @@ func registerRoutes(m *web.Route) { }, reqSignIn) m.Group("", func() { - m.Methods("GET, POST, OPTIONS", "/userinfo", auth.InfoOAuth) m.Methods("POST, OPTIONS", "/access_token", web.Bind(forms.AccessTokenForm{}), auth.AccessTokenOAuth) m.Methods("GET, OPTIONS", "/keys", auth.OIDCKeys) m.Methods("POST, OPTIONS", "/introspect", web.Bind(forms.IntrospectTokenForm{}), auth.IntrospectOAuth) @@ -932,28 +982,10 @@ func registerRoutes(m *web.Route) { m.Group("", func() { m.Get("/{username}", user.UsernameSubRoute) - m.Methods("GET, OPTIONS", "/attachments/{uuid}", optionsCorsHandler(), repo.GetAttachment) }, ignSignIn) m.Post("/{username}", reqSignIn, context.UserAssignmentWeb(), user.Action) - reqRepoAdmin := context.RequireRepoAdmin() - reqRepoCodeWriter := context.RequireRepoWriter(unit.TypeCode) - canEnableEditor := context.CanEnableEditor() - reqRepoCodeReader := context.RequireRepoReader(unit.TypeCode) - reqRepoReleaseWriter := context.RequireRepoWriter(unit.TypeReleases) - reqRepoReleaseReader := context.RequireRepoReader(unit.TypeReleases) - reqRepoWikiWriter := context.RequireRepoWriter(unit.TypeWiki) - reqRepoIssueReader := context.RequireRepoReader(unit.TypeIssues) - reqRepoPullsReader := context.RequireRepoReader(unit.TypePullRequests) - reqRepoIssuesOrPullsWriter := context.RequireRepoWriterOr(unit.TypeIssues, unit.TypePullRequests) - reqRepoIssuesOrPullsReader := context.RequireRepoReaderOr(unit.TypeIssues, unit.TypePullRequests) - reqRepoProjectsReader := context.RequireRepoReader(unit.TypeProjects) - reqRepoProjectsWriter := context.RequireRepoWriter(unit.TypeProjects) - reqRepoActionsReader := context.RequireRepoReader(unit.TypeActions) - reqRepoActionsWriter := context.RequireRepoWriter(unit.TypeActions) - reqRepoDelegateActionTrust := context.RequireRepoDelegateActionTrust() - reqPackageAccess := func(accessMode perm.AccessMode) func(ctx *context.Context) { return func(ctx *context.Context) { if ctx.Package.AccessMode < accessMode && !ctx.IsUserSiteAdmin() { @@ -1478,7 +1510,6 @@ func registerRoutes(m *web.Route) { }, ctxDataSet("EnableFeed", setting.Other.EnableFeed), repo.MustBeNotEmpty, context.RepoRefByType(context.RepoRefTag, true)) m.Get("/releases/attachments/{uuid}", repo.MustBeNotEmpty, repo.GetAttachment) - m.Get("/releases/download/{vTag}/{fileName}", repo.MustBeNotEmpty, repo.RedirectDownload) m.Group("/releases", func() { m.Combo("/new", context.EnforceQuotaWeb(quota_model.LimitSubjectSizeReposAll, context.QuotaTargetRepo)). Get(repo.NewRelease). @@ -1493,11 +1524,6 @@ func registerRoutes(m *web.Route) { }, reqSignIn, repo.MustBeNotEmpty, context.RepoMustNotBeArchived(), reqRepoReleaseWriter, repo.CommitInfoCache, context.EnforceQuotaWeb(quota_model.LimitSubjectSizeReposAll, context.QuotaTargetRepo)) }, ignSignIn, context.RepoAssignment, context.UnitTypes(), reqRepoReleaseReader) - // to maintain compatibility with old attachments - m.Group("/{username}/{reponame}", func() { - m.Get("/attachments/{uuid}", repo.GetAttachment) - }, ignSignIn, context.RepoAssignment, context.UnitTypes()) - m.Group("/{username}/{reponame}", func() { m.Post("/topics", repo.TopicsPost) }, context.RepoAssignment, context.RepoMustNotBeArchived(), reqRepoAdmin) @@ -1646,11 +1672,6 @@ func registerRoutes(m *web.Route) { m.Get("/{period}", repo.ActivityAuthors) }, context.RepoRef(), repo.MustBeNotEmpty, context.RequireRepoReaderOr(unit.TypeCode)) - m.Group("/archive", func() { - m.Get("/*", repo.Download) - m.Post("/*", repo.InitiateDownload) - }, repo.MustBeNotEmpty, dlSourceEnabled, reqRepoCodeReader) - m.Group("/branches", func() { m.Get("/list", repo.GetBranchesList) m.Get("", repo.Branches) @@ -1721,15 +1742,6 @@ func registerRoutes(m *web.Route) { m.Get("/*", context.RepoRefByType(context.RepoRefLegacy), repo.SingleDownloadOrLFS) }, repo.MustBeNotEmpty, reqRepoCodeReader) - m.Group("/raw", func() { - m.Get("/branch/*", context.RepoRefByType(context.RepoRefBranch), repo.SingleDownload) - m.Get("/tag/*", context.RepoRefByType(context.RepoRefTag), repo.SingleDownload) - m.Get("/commit/*", context.RepoRefByType(context.RepoRefCommit), repo.SingleDownload) - m.Get("/blob/{sha}", context.RepoRefByType(context.RepoRefBlob), repo.DownloadByID) - // "/*" route is deprecated, and kept for backward compatibility - m.Get("/*", context.RepoRefByType(context.RepoRefLegacy), repo.SingleDownload) - }, repo.MustBeNotEmpty, reqRepoCodeReader) - m.Group("/render", func() { m.Get("/branch/*", context.RepoRefByType(context.RepoRefBranch), repo.RenderFile) m.Get("/tag/*", context.RepoRefByType(context.RepoRefTag), repo.RenderFile) @@ -1870,6 +1882,44 @@ func registerGitRoutes(m *web.Route) { }) } +func registerMixedRoutes(m *web.Route) { + dlSourceEnabled := func(ctx *context.Context) { + if setting.Repository.DisableDownloadSourceArchives { + ctx.Error(http.StatusNotFound) + return + } + } + + m.Group("/{username}/{reponame}", func() { + m.Group("/raw", func() { + m.Get("/branch/*", context.RepoRefByType(context.RepoRefBranch), repo.SingleDownload) + m.Get("/tag/*", context.RepoRefByType(context.RepoRefTag), repo.SingleDownload) + m.Get("/commit/*", context.RepoRefByType(context.RepoRefCommit), repo.SingleDownload) + m.Get("/blob/{sha}", context.RepoRefByType(context.RepoRefBlob), repo.DownloadByID) + // "/*" route is deprecated, and kept for backward compatibility + m.Get("/*", context.RepoRefByType(context.RepoRefLegacy), repo.SingleDownload) + }, repo.MustBeNotEmpty, reqRepoCodeReader) + + m.Group("/archive", func() { + m.Get("/*", repo.Download) + m.Post("/*", repo.InitiateDownload) + }, repo.MustBeNotEmpty, dlSourceEnabled, reqRepoCodeReader) + }, ignSignIn, context.RepoAssignment, context.UnitTypes()) + + m.Group("/{username}/{reponame}", func() { + m.Get("/releases/download/{vTag}/{fileName}", repo.MustBeNotEmpty, repo.RedirectDownload) + }, ignSignIn, context.RepoAssignment, context.UnitTypes(), reqRepoReleaseReader) + + // to maintain compatibility with old attachments + m.Group("/{username}/{reponame}", func() { + m.Get("/attachments/{uuid}", repo.GetAttachment) + }, ignSignIn, context.RepoAssignment, context.UnitTypes()) + + m.Group("", func() { + m.Methods("GET, OPTIONS", "/attachments/{uuid}", optionsCorsHandler(), repo.GetAttachment) + }, ignSignIn) +} + func BindUpload() http.HandlerFunc { return func(resp http.ResponseWriter, req *http.Request) { theObj := new(forms.UploadRepoFileForm) // create a new form obj for every request but not use obj directly diff --git a/services/auth/method/access_token.go b/services/auth/method/access_token.go index f1a5e8e766..7f8174336d 100644 --- a/services/auth/method/access_token.go +++ b/services/auth/method/access_token.go @@ -11,24 +11,21 @@ import ( user_model "forgejo.org/models/user" "forgejo.org/modules/log" "forgejo.org/modules/optional" - "forgejo.org/modules/web/middleware" "forgejo.org/services/auth" "forgejo.org/services/authz" ) var _ auth.Method = &AccessToken{} -type AccessToken struct{} +type AccessToken struct { + // Permit the use of `Authorization: Basic ...` to include an access token. + PermitBasic bool + // Permit the use of `Authorization: Bearer ...`, `Authorization: Token ...`, and form-based tokens. + PermitBearer bool +} func (a *AccessToken) Verify(req *http.Request, w http.ResponseWriter, _ auth.SessionStore) auth.MethodOutput { - // Authentication previously was performed in a single routine for `Authorization: Basic ...` and `Authorization: - // Bearer ...`, and both routines had separate URL exclusion lists onto which they wouldn't apply. That behaviour - // is maintained by cloning those conditions here and deciding whether to look at basic/bearer auth, or not. In the - // future this should be removed and migrated to route-specific middleware. - legacySkipBasic := !middleware.IsAPIPath(req) && !isContainerPath(req) && !isAttachmentDownload(req) && !isGitRawOrAttachOrLFSPath(req) - legacySkipFormAndBearer := !middleware.IsAPIPath(req) && !isAttachmentDownload(req) && !isAuthenticatedTokenRequest(req) && !isGitRawOrAttachPath(req) && !isArchivePath(req) - - maybeAuthToken := a.getTokenFromRequest(req, legacySkipBasic, legacySkipFormAndBearer) + maybeAuthToken := a.getTokenFromRequest(req) if !maybeAuthToken.Has() { return &auth.AuthenticationNotAttempted{} } @@ -60,8 +57,8 @@ func (a *AccessToken) Verify(req *http.Request, w http.ResponseWriter, _ auth.Se return &auth.AuthenticationSuccess{Result: &accessTokenAuthenticationResult{user: u, scope: token.Scope, reducer: reducer}} } -func (a *AccessToken) getTokenFromRequest(req *http.Request, skipBasic, skipFormAndBearer bool) optional.Option[string] { - if !skipFormAndBearer { +func (a *AccessToken) getTokenFromRequest(req *http.Request) optional.Option[string] { + if a.PermitBearer { if has, token := tokenFromForm(req).Get(); has { return optional.Some(token) } @@ -69,7 +66,7 @@ func (a *AccessToken) getTokenFromRequest(req *http.Request, skipBasic, skipForm return optional.Some(token) } } - if !skipBasic { + if a.PermitBasic { if has, token := tokenFromAuthorizationBasic(req).Get(); has { return optional.Some(token) } diff --git a/services/auth/method/action_runtime_token.go b/services/auth/method/action_runtime_token.go index 2d15476424..f58e9129a9 100644 --- a/services/auth/method/action_runtime_token.go +++ b/services/auth/method/action_runtime_token.go @@ -11,7 +11,6 @@ import ( actions_model "forgejo.org/models/actions" user_model "forgejo.org/models/user" "forgejo.org/modules/optional" - "forgejo.org/modules/web/middleware" "forgejo.org/services/actions" "forgejo.org/services/auth" ) @@ -21,11 +20,6 @@ var _ auth.Method = &ActionRuntimeToken{} type ActionRuntimeToken struct{} func (a *ActionRuntimeToken) Verify(req *http.Request, w http.ResponseWriter, _ auth.SessionStore) auth.MethodOutput { - // In the future this should be removed and migrated to route-specific middleware: - if !middleware.IsAPIPath(req) && !isAttachmentDownload(req) && !isAuthenticatedTokenRequest(req) && !isGitRawOrAttachPath(req) && !isArchivePath(req) { - return &auth.AuthenticationNotAttempted{} - } - maybeAuthToken := a.getTokenFromRequest(req) if !maybeAuthToken.Has() { return &auth.AuthenticationNotAttempted{} diff --git a/services/auth/method/action_task_token.go b/services/auth/method/action_task_token.go index b5e1ab4f59..d78fa5346d 100644 --- a/services/auth/method/action_task_token.go +++ b/services/auth/method/action_task_token.go @@ -13,23 +13,20 @@ import ( "forgejo.org/modules/log" "forgejo.org/modules/optional" "forgejo.org/modules/util" - "forgejo.org/modules/web/middleware" "forgejo.org/services/auth" ) var _ auth.Method = &ActionTaskToken{} -type ActionTaskToken struct{} +type ActionTaskToken struct { + // Permit the use of `Authorization: Basic ...` to include an access token. + PermitBasic bool + // Permit the use of `Authorization: Bearer ...`, `Authorization: Token ...`, and form-based tokens. + PermitBearer bool +} func (a *ActionTaskToken) Verify(req *http.Request, w http.ResponseWriter, _ auth.SessionStore) auth.MethodOutput { - // Authentication previously was performed in a single routine for `Authorization: Basic ...` and `Authorization: - // Bearer ...`, and both routines had separate URL exclusion lists onto which they wouldn't apply. That behaviour - // is maintained by cloning those conditions here and deciding whether to look at basic/bearer auth, or not. In the - // future this should be removed and migrated to route-specific middleware. - legacySkipBasic := !middleware.IsAPIPath(req) && !isContainerPath(req) && !isAttachmentDownload(req) && !isGitRawOrAttachOrLFSPath(req) - legacySkipFormAndBearer := !middleware.IsAPIPath(req) && !isAttachmentDownload(req) && !isAuthenticatedTokenRequest(req) && !isGitRawOrAttachPath(req) && !isArchivePath(req) - - maybeAuthToken := a.getTokenFromRequest(req, legacySkipBasic, legacySkipFormAndBearer) + maybeAuthToken := a.getTokenFromRequest(req) if !maybeAuthToken.Has() { return &auth.AuthenticationNotAttempted{} } @@ -49,8 +46,8 @@ func (a *ActionTaskToken) Verify(req *http.Request, w http.ResponseWriter, _ aut return &auth.AuthenticationSuccess{Result: &actionsTaskTokenAuthenticationResult{user: user_model.NewActionsUser(), taskID: task.ID}} } -func (a *ActionTaskToken) getTokenFromRequest(req *http.Request, skipBasic, skipFormAndBearer bool) optional.Option[string] { - if !skipFormAndBearer { +func (a *ActionTaskToken) getTokenFromRequest(req *http.Request) optional.Option[string] { + if a.PermitBearer { if has, token := tokenFromForm(req).Get(); has { return optional.Some(token) } @@ -58,7 +55,7 @@ func (a *ActionTaskToken) getTokenFromRequest(req *http.Request, skipBasic, skip return optional.Some(token) } } - if !skipBasic { + if a.PermitBasic { if has, token := tokenFromAuthorizationBasic(req).Get(); has { return optional.Some(token) } diff --git a/services/auth/method/auth.go b/services/auth/method/auth.go index b2e89d51e9..7d6a0d6722 100644 --- a/services/auth/method/auth.go +++ b/services/auth/method/auth.go @@ -7,15 +7,12 @@ package method import ( "fmt" "net/http" - "regexp" - "strings" user_model "forgejo.org/models/user" "forgejo.org/modules/auth/webauthn" "forgejo.org/modules/log" "forgejo.org/modules/optional" "forgejo.org/modules/session" - "forgejo.org/modules/setting" "forgejo.org/modules/web/middleware" "forgejo.org/services/auth" user_service "forgejo.org/services/user" @@ -27,42 +24,6 @@ func Init() { webauthn.Init() } -// isAttachmentDownload check if request is a file download (GET) with URL to an attachment -func isAttachmentDownload(req *http.Request) bool { - return strings.HasPrefix(req.URL.Path, "/attachments/") && req.Method == "GET" -} - -// isContainerPath checks if the request targets the container endpoint -func isContainerPath(req *http.Request) bool { - // Go's URL omits trailing slashes from `Path`. That means that `/v2/`, the top-level endpoint, appears as `/v2`. - // strings.HasPrefix(req.URL.Path, "/v2") would be inappropriate because it would match paths like `/v2-abcd`, too. - return req.URL.Path == "/v2" || strings.HasPrefix(req.URL.Path, "/v2/") -} - -var ( - gitRawOrAttachPathRe = regexp.MustCompile(`^/[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+/(?:(?:git-(?:(?:upload)|(?:receive))-pack$)|(?:info/refs$)|(?:HEAD$)|(?:objects/)|(?:raw/)|(?:releases/download/)|(?:attachments/))`) - lfsPathRe = regexp.MustCompile(`^/[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+/info/lfs/`) - archivePathRe = regexp.MustCompile(`^/[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+/archive/`) -) - -func isGitRawOrAttachPath(req *http.Request) bool { - return gitRawOrAttachPathRe.MatchString(req.URL.Path) -} - -func isGitRawOrAttachOrLFSPath(req *http.Request) bool { - if isGitRawOrAttachPath(req) { - return true - } - if setting.LFS.StartServer { - return lfsPathRe.MatchString(req.URL.Path) - } - return false -} - -func isArchivePath(req *http.Request) bool { - return archivePathRe.MatchString(req.URL.Path) -} - // handleSignIn clears existing session variables and stores new ones for the specified user object func handleSignIn(resp http.ResponseWriter, req *http.Request, sess auth.SessionStore, user *user_model.User) { // We need to regenerate the session... diff --git a/services/auth/method/auth_test.go b/services/auth/method/auth_test.go deleted file mode 100644 index 6e686feab8..0000000000 --- a/services/auth/method/auth_test.go +++ /dev/null @@ -1,198 +0,0 @@ -// Copyright 2014 The Gogs Authors. All rights reserved. -// Copyright 2019 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package method - -import ( - "net/http" - "net/url" - "testing" - - "forgejo.org/modules/setting" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func Test_isGitRawOrLFSPath(t *testing.T) { - tests := []struct { - path string - - want bool - }{ - { - "/owner/repo/git-upload-pack", - true, - }, - { - "/owner/repo/git-receive-pack", - true, - }, - { - "/owner/repo/info/refs", - true, - }, - { - "/owner/repo/HEAD", - true, - }, - { - "/owner/repo/objects/info/alternates", - true, - }, - { - "/owner/repo/objects/info/http-alternates", - true, - }, - { - "/owner/repo/objects/info/packs", - true, - }, - { - "/owner/repo/objects/info/blahahsdhsdkla", - true, - }, - { - "/owner/repo/objects/01/23456789abcdef0123456789abcdef01234567", - true, - }, - { - "/owner/repo/objects/pack/pack-123456789012345678921234567893124567894.pack", - true, - }, - { - "/owner/repo/objects/pack/pack-0123456789abcdef0123456789abcdef0123456.idx", - true, - }, - { - "/owner/repo/raw/branch/foo/fanaso", - true, - }, - { - "/owner/repo/stars", - false, - }, - { - "/notowner", - false, - }, - { - "/owner/repo", - false, - }, - { - "/owner/repo/commit/123456789012345678921234567893124567894", - false, - }, - { - "/owner/repo/releases/download/tag/repo.tar.gz", - true, - }, - { - "/owner/repo/attachments/6d92a9ee-5d8b-4993-97c9-6181bdaa8955", - true, - }, - } - lfsTests := []string{ - "/owner/repo/info/lfs/", - "/owner/repo/info/lfs/objects/batch", - "/owner/repo/info/lfs/objects/oid/filename", - "/owner/repo/info/lfs/objects/oid", - "/owner/repo/info/lfs/objects", - "/owner/repo/info/lfs/verify", - "/owner/repo/info/lfs/locks", - "/owner/repo/info/lfs/locks/verify", - "/owner/repo/info/lfs/locks/123/unlock", - } - - origLFSStartServer := setting.LFS.StartServer - - for _, tt := range tests { - t.Run(tt.path, func(t *testing.T) { - req, _ := http.NewRequest("POST", "http://localhost"+tt.path, nil) - setting.LFS.StartServer = false - if got := isGitRawOrAttachOrLFSPath(req); got != tt.want { - t.Errorf("isGitOrLFSPath() = %v, want %v", got, tt.want) - } - setting.LFS.StartServer = true - if got := isGitRawOrAttachOrLFSPath(req); got != tt.want { - t.Errorf("isGitOrLFSPath() = %v, want %v", got, tt.want) - } - }) - } - for _, tt := range lfsTests { - t.Run(tt, func(t *testing.T) { - req, _ := http.NewRequest("POST", tt, nil) - setting.LFS.StartServer = false - if got := isGitRawOrAttachOrLFSPath(req); got != setting.LFS.StartServer { - t.Errorf("isGitOrLFSPath(%q) = %v, want %v, %v", tt, got, setting.LFS.StartServer, gitRawOrAttachPathRe.MatchString(tt)) - } - setting.LFS.StartServer = true - if got := isGitRawOrAttachOrLFSPath(req); got != setting.LFS.StartServer { - t.Errorf("isGitOrLFSPath(%q) = %v, want %v", tt, got, setting.LFS.StartServer) - } - }) - } - setting.LFS.StartServer = origLFSStartServer -} - -func TestAuth_isContainerPath(t *testing.T) { - testCases := []struct { - name string - input string - isContainerPath bool - }{ - { - name: "without trailing slash", - input: "https://example.com/v2", - isContainerPath: true, - }, - { - name: "with trailing slash", - input: "https://example.com/v2/", - isContainerPath: true, - }, - { - name: "with additional path components", - input: "https://example.com/v2/example/blobs/uploads/", - isContainerPath: true, - }, - { - name: "without v2", - input: "https://example.com/", - isContainerPath: false, - }, - { - name: "v2 not at the beginning", - input: "https://example.com/something/v2/", - isContainerPath: false, - }, - { - name: "v2 with prefix", - input: "https://example.com/abcd-v2/", - isContainerPath: false, - }, - { - name: "v2 with suffix", - input: "https://example.com/v2-abcd/", - isContainerPath: false, - }, - { - name: "v1", - input: "https://example.com/v1/", - isContainerPath: false, - }, - } - - for _, testCase := range testCases { - t.Run(testCase.name, func(t *testing.T) { - inputURL, err := url.Parse(testCase.input) - require.NoError(t, err) - - request := http.Request{URL: inputURL} - - assert.Equal(t, testCase.isContainerPath, isContainerPath(&request)) - }) - } -} diff --git a/services/auth/method/basic.go b/services/auth/method/basic.go index 8385bea2fd..abc3387efe 100644 --- a/services/auth/method/basic.go +++ b/services/auth/method/basic.go @@ -16,7 +16,6 @@ import ( "forgejo.org/modules/log" "forgejo.org/modules/setting" "forgejo.org/modules/util" - "forgejo.org/modules/web/middleware" "forgejo.org/services/auth" "forgejo.org/services/auth/source/db" ) @@ -34,11 +33,6 @@ type Basic struct{} // Verify extracts and validates Basic data (username and password/token) from the "Authorization" header of the request // and returns the corresponding user object for that name/token on successful validation. func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, _ auth.SessionStore) auth.MethodOutput { - // Basic authentication should only fire on API, Download or on Git or LFSPaths - if !middleware.IsAPIPath(req) && !isContainerPath(req) && !isAttachmentDownload(req) && !isGitRawOrAttachOrLFSPath(req) { - return &auth.AuthenticationNotAttempted{} - } - baHead := req.Header.Get("Authorization") if len(baHead) == 0 { return &auth.AuthenticationNotAttempted{} diff --git a/services/auth/method/oauth2.go b/services/auth/method/oauth2.go index 9eeef6a6c1..acfd534fb7 100644 --- a/services/auth/method/oauth2.go +++ b/services/auth/method/oauth2.go @@ -17,7 +17,6 @@ import ( "forgejo.org/modules/log" "forgejo.org/modules/optional" "forgejo.org/modules/setting" - "forgejo.org/modules/web/middleware" "forgejo.org/services/auth" "forgejo.org/services/auth/source/oauth2" ) @@ -74,11 +73,6 @@ func (o *OAuth2) Verify(req *http.Request, w http.ResponseWriter, _ auth.Session if !setting.OAuth2.Enabled { return &auth.AuthenticationNotAttempted{} } - // These paths are not API paths, but we still want to check for tokens because they maybe in the API returned URLs - if !middleware.IsAPIPath(req) && !isAttachmentDownload(req) && !isAuthenticatedTokenRequest(req) && - !isGitRawOrAttachPath(req) && !isArchivePath(req) { - return &auth.AuthenticationNotAttempted{} - } maybeAuthToken := o.getTokenFromRequest(req) if !maybeAuthToken.Has() { @@ -145,7 +139,3 @@ func (*OAuth2) getTokenFromRequest(req *http.Request) optional.Option[string] { } return optional.None[string]() } - -func isAuthenticatedTokenRequest(req *http.Request) bool { - return req.URL.Path == "/login/oauth/userinfo" -} diff --git a/services/auth/method/reverseproxy.go b/services/auth/method/reverseproxy.go index 0fda466875..483f7ed724 100644 --- a/services/auth/method/reverseproxy.go +++ b/services/auth/method/reverseproxy.go @@ -15,7 +15,6 @@ import ( "forgejo.org/modules/optional" "forgejo.org/modules/setting" "forgejo.org/modules/util" - "forgejo.org/modules/web/middleware" "forgejo.org/services/auth" gouuid "github.com/google/uuid" @@ -34,7 +33,10 @@ const ReverseProxyMethodName = "reverse_proxy" // On successful authentication the proxy is expected to populate the username in the // "setting.ReverseProxyAuthUser" header. Optionally it can also populate the email of the // user in the "setting.ReverseProxyAuthEmail" header. -type ReverseProxy struct{} +type ReverseProxy struct { + // If true, create a session once a user authenticates. + CreateSession bool +} // getUserName extracts the username from the "setting.ReverseProxyAuthUser" header func (r *ReverseProxy) getUserName(req *http.Request) string { @@ -120,8 +122,7 @@ func (r *ReverseProxy) Verify(req *http.Request, w http.ResponseWriter, sess aut } } - // Make sure requests to API paths, attachment downloads, git and LFS do not create a new session - if !middleware.IsAPIPath(req) && !isAttachmentDownload(req) && !isGitRawOrAttachOrLFSPath(req) { + if r.CreateSession { if sess != nil && (sess.Get("uid") == nil || sess.Get("uid").(int64) != user.ID) { handleSignIn(w, req, sess, user) }