From 3d5d8c785bf37d5fbb0e832001f54ecedc0a2476 Mon Sep 17 00:00:00 2001 From: sh0rez Date: Fri, 20 May 2022 12:45:18 -0400 Subject: [PATCH] pkg/web: restrict handler types (#48495) Makes `pkg/web` only accept handles from the following set: ```go handlerStd = func(http.ResponseWriter, *http.Request) handlerStdCtx = func(http.ResponseWriter, *http.Request, *web.Context) handlerStdReqCtx = func(http.ResponseWriter, *http.Request, *models.ReqContext) handlerReqCtx = func(*models.ReqContext) handlerReqCtxRes = func(*models.ReqContext) Response handlerCtx = func(*web.Context) ``` This is a first step to reducing above set to only `http.Handler`. --- Due to a cyclic import situation between `pkg/models` and `pkg/web`, parts of this PR were put into `pkg/api/response`, even though they definitely do not belong there. This however is _temporary_ until we untangle `models.ReqContext`. --- pkg/api/common_test.go | 4 ++ pkg/api/response/web_hack.go | 62 +++++++++++++++++++ pkg/infra/usagestats/service/api_test.go | 4 ++ pkg/services/accesscontrol/middleware_test.go | 4 ++ .../resourcepermissions/api_test.go | 4 ++ pkg/services/contexthandler/contexthandler.go | 5 +- pkg/services/contexthandler/ctxkey/key.go | 13 ++++ pkg/services/ldap/testing.go | 3 + pkg/services/multildap/multildap_test.go | 3 + pkg/services/serviceaccounts/api/api_test.go | 4 ++ pkg/web/macaron.go | 26 ++------ pkg/web/macaron.s | 2 + pkg/web/webtest/webtest.go | 4 ++ 13 files changed, 115 insertions(+), 23 deletions(-) create mode 100644 pkg/api/response/web_hack.go create mode 100644 pkg/services/contexthandler/ctxkey/key.go create mode 100644 pkg/web/macaron.s diff --git a/pkg/api/common_test.go b/pkg/api/common_test.go index bfd3d3836c3..012380ef0bf 100644 --- a/pkg/api/common_test.go +++ b/pkg/api/common_test.go @@ -27,6 +27,7 @@ import ( "github.com/grafana/grafana/pkg/services/auth" "github.com/grafana/grafana/pkg/services/contexthandler" "github.com/grafana/grafana/pkg/services/contexthandler/authproxy" + "github.com/grafana/grafana/pkg/services/contexthandler/ctxkey" "github.com/grafana/grafana/pkg/services/dashboards" dashboardsstore "github.com/grafana/grafana/pkg/services/dashboards/database" dashboardservice "github.com/grafana/grafana/pkg/services/dashboards/service" @@ -411,6 +412,9 @@ func setupHTTPServerWithCfgDb(t *testing.T, useFakeAccessControl, enableAccessCo initCtx.Context = c initCtx.Logger = log.New("api-test") c.Map(initCtx) + + c.Req = c.Req.WithContext(ctxkey.Set(c.Req.Context(), initCtx)) + c.Map(c.Req) }) m.Use(accesscontrol.LoadPermissionsMiddleware(hs.AccessControl)) diff --git a/pkg/api/response/web_hack.go b/pkg/api/response/web_hack.go new file mode 100644 index 00000000000..c8d6e6a2063 --- /dev/null +++ b/pkg/api/response/web_hack.go @@ -0,0 +1,62 @@ +//nolint:unused,deadcode +package response + +//NOTE: This file belongs into pkg/web, but due to cyclic imports that are hard to resolve at the current time, it temporarily lives here. + +import ( + "context" + "fmt" + "net/http" + + "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/contexthandler/ctxkey" + "github.com/grafana/grafana/pkg/web" +) + +type ( + handlerStd = func(http.ResponseWriter, *http.Request) + handlerStdCtx = func(http.ResponseWriter, *http.Request, *web.Context) + handlerStdReqCtx = func(http.ResponseWriter, *http.Request, *models.ReqContext) + handlerReqCtx = func(*models.ReqContext) + handlerReqCtxRes = func(*models.ReqContext) Response + handlerCtx = func(*web.Context) +) + +func wrap_handler(h web.Handler) http.HandlerFunc { + switch handle := h.(type) { + case handlerStd: + return handle + case handlerStdCtx: + return func(w http.ResponseWriter, r *http.Request) { + handle(w, r, web.FromContext(r.Context())) + } + case handlerStdReqCtx: + return func(w http.ResponseWriter, r *http.Request) { + handle(w, r, getReqCtx(r.Context())) + } + case handlerReqCtx: + return func(w http.ResponseWriter, r *http.Request) { + handle(getReqCtx(r.Context())) + } + case handlerReqCtxRes: + return func(w http.ResponseWriter, r *http.Request) { + ctx := getReqCtx(r.Context()) + res := handle(ctx) + res.WriteTo(ctx) + } + case handlerCtx: + return func(w http.ResponseWriter, r *http.Request) { + handle(web.FromContext(r.Context())) + } + } + + panic(fmt.Sprintf("unexpected handler type: %T", h)) +} + +func getReqCtx(ctx context.Context) *models.ReqContext { + reqCtx, ok := ctx.Value(ctxkey.Key{}).(*models.ReqContext) + if !ok { + panic("no *models.ReqContext found") + } + return reqCtx +} diff --git a/pkg/infra/usagestats/service/api_test.go b/pkg/infra/usagestats/service/api_test.go index 450bfb83c0d..ea67c868442 100644 --- a/pkg/infra/usagestats/service/api_test.go +++ b/pkg/infra/usagestats/service/api_test.go @@ -9,6 +9,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/contexthandler/ctxkey" "github.com/grafana/grafana/pkg/services/sqlstore/mockstore" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/web" @@ -103,5 +104,8 @@ func contextProvider(tc *testContext) web.Handler { Logger: log.New("test"), } c.Map(reqCtx) + + c.Req = c.Req.WithContext(ctxkey.Set(c.Req.Context(), reqCtx)) + c.Map(c.Req) } } diff --git a/pkg/services/accesscontrol/middleware_test.go b/pkg/services/accesscontrol/middleware_test.go index 5fd2018363b..887390f34b8 100644 --- a/pkg/services/accesscontrol/middleware_test.go +++ b/pkg/services/accesscontrol/middleware_test.go @@ -11,6 +11,7 @@ import ( "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/mock" + "github.com/grafana/grafana/pkg/services/contexthandler/ctxkey" "github.com/grafana/grafana/pkg/web" ) @@ -90,5 +91,8 @@ func contextProvider() web.Handler { SkipCache: true, } c.Map(reqCtx) + + c.Req = c.Req.WithContext(ctxkey.Set(c.Req.Context(), reqCtx)) + c.Map(c.Req) } } diff --git a/pkg/services/accesscontrol/resourcepermissions/api_test.go b/pkg/services/accesscontrol/resourcepermissions/api_test.go index 491cedbaa5e..c1786f435b5 100644 --- a/pkg/services/accesscontrol/resourcepermissions/api_test.go +++ b/pkg/services/accesscontrol/resourcepermissions/api_test.go @@ -18,6 +18,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/accesscontrol" + "github.com/grafana/grafana/pkg/services/contexthandler/ctxkey" "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/web" @@ -500,6 +501,9 @@ func contextProvider(tc *testContext) web.Handler { Logger: log.New("test"), } c.Map(reqCtx) + + c.Req = c.Req.WithContext(ctxkey.Set(c.Req.Context(), reqCtx)) + c.Map(c.Req) } } diff --git a/pkg/services/contexthandler/contexthandler.go b/pkg/services/contexthandler/contexthandler.go index d339ea5f09b..263bb20e0d4 100644 --- a/pkg/services/contexthandler/contexthandler.go +++ b/pkg/services/contexthandler/contexthandler.go @@ -19,6 +19,7 @@ import ( "github.com/grafana/grafana/pkg/middleware/cookies" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/contexthandler/authproxy" + "github.com/grafana/grafana/pkg/services/contexthandler/ctxkey" "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/services/rendering" "github.com/grafana/grafana/pkg/services/sqlstore" @@ -69,7 +70,7 @@ type ContextHandler struct { GetTime func() time.Time } -type reqContextKey struct{} +type reqContextKey = ctxkey.Key // FromContext returns the ReqContext value stored in a context.Context, if any. func FromContext(c context.Context) *models.ReqContext { @@ -94,7 +95,7 @@ func (h *ContextHandler) Middleware(mContext *web.Context) { } // Inject ReqContext into a request context and replace the request instance in the macaron context - mContext.Req = mContext.Req.WithContext(context.WithValue(mContext.Req.Context(), reqContextKey{}, reqContext)) + mContext.Req = mContext.Req.WithContext(ctxkey.Set(mContext.Req.Context(), reqContext)) mContext.Map(mContext.Req) traceID := tracing.TraceIDFromContext(mContext.Req.Context(), false) diff --git a/pkg/services/contexthandler/ctxkey/key.go b/pkg/services/contexthandler/ctxkey/key.go new file mode 100644 index 00000000000..42226b19ba5 --- /dev/null +++ b/pkg/services/contexthandler/ctxkey/key.go @@ -0,0 +1,13 @@ +package ctxkey + +import "context" + +type Key struct{} + +func Set(ctx context.Context, data interface{}) context.Context { + return context.WithValue(ctx, Key{}, data) +} + +func Get(ctx context.Context) interface{} { + return ctx.Value(Key{}) +} diff --git a/pkg/services/ldap/testing.go b/pkg/services/ldap/testing.go index cd9ff9184f4..d0d649185ac 100644 --- a/pkg/services/ldap/testing.go +++ b/pkg/services/ldap/testing.go @@ -4,6 +4,9 @@ import ( "crypto/tls" "gopkg.in/ldap.v3" + + //TODO(sh0rez): remove once import cycle resolved + _ "github.com/grafana/grafana/pkg/api/response" ) type searchFunc = func(request *ldap.SearchRequest) (*ldap.SearchResult, error) diff --git a/pkg/services/multildap/multildap_test.go b/pkg/services/multildap/multildap_test.go index f39f3123709..aa103c828ed 100644 --- a/pkg/services/multildap/multildap_test.go +++ b/pkg/services/multildap/multildap_test.go @@ -8,6 +8,9 @@ import ( "github.com/grafana/grafana/pkg/services/ldap" "github.com/stretchr/testify/require" + + //TODO(sh0rez): remove once import cycle resolved + _ "github.com/grafana/grafana/pkg/api/response" ) func TestMultiLDAP(t *testing.T) { diff --git a/pkg/services/serviceaccounts/api/api_test.go b/pkg/services/serviceaccounts/api/api_test.go index b60e5361da7..c6db4b27b23 100644 --- a/pkg/services/serviceaccounts/api/api_test.go +++ b/pkg/services/serviceaccounts/api/api_test.go @@ -15,6 +15,7 @@ import ( "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/accesscontrol" accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" + "github.com/grafana/grafana/pkg/services/contexthandler/ctxkey" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/serviceaccounts" "github.com/grafana/grafana/pkg/services/serviceaccounts/database" @@ -238,6 +239,9 @@ func setupTestServer(t *testing.T, svc *tests.ServiceAccountMock, Logger: log.New("serviceaccounts-test"), } c.Map(ctx) + + c.Req = c.Req.WithContext(ctxkey.Set(c.Req.Context(), ctx)) + c.Map(c.Req) }) a.RouterRegister.Register(m.Router) return m, a diff --git a/pkg/web/macaron.go b/pkg/web/macaron.go index f9b3084706d..071ea67e9c4 100644 --- a/pkg/web/macaron.go +++ b/pkg/web/macaron.go @@ -19,9 +19,10 @@ package web import ( + _ "unsafe" + "context" "net/http" - "reflect" "strings" ) @@ -47,31 +48,14 @@ func Version() string { // and panics if an argument could not be fulfilled via dependency injection. type Handler interface{} -// handlerFuncInvoker is an inject.FastInvoker wrapper of func(http.ResponseWriter, *http.Request). -type handlerFuncInvoker func(http.ResponseWriter, *http.Request) - -func (invoke handlerFuncInvoker) Invoke(params []interface{}) ([]reflect.Value, error) { - invoke(params[0].(http.ResponseWriter), params[1].(*http.Request)) - return nil, nil -} +//go:linkname hack_wrap github.com/grafana/grafana/pkg/api/response.wrap_handler +func hack_wrap(Handler) http.HandlerFunc // validateAndWrapHandler makes sure a handler is a callable function, it panics if not. // When the handler is also potential to be any built-in inject.FastInvoker, // it wraps the handler automatically to have some performance gain. func validateAndWrapHandler(h Handler) Handler { - if reflect.TypeOf(h).Kind() != reflect.Func { - panic("Macaron handler must be a callable function") - } - - if !IsFastInvoker(h) { - switch v := h.(type) { - case func(*Context): - return ContextInvoker(v) - case func(http.ResponseWriter, *http.Request): - return handlerFuncInvoker(v) - } - } - return h + return hack_wrap(h) } // validateAndWrapHandlers preforms validation and wrapping for each input handler. diff --git a/pkg/web/macaron.s b/pkg/web/macaron.s new file mode 100644 index 00000000000..9dbf545efc8 --- /dev/null +++ b/pkg/web/macaron.s @@ -0,0 +1,2 @@ +// ./hack/hack.go pushes its Wrap function as hack_wrap into this package. +// This .s file is required to allow hack_wrap to be defined without having a function body. diff --git a/pkg/web/webtest/webtest.go b/pkg/web/webtest/webtest.go index 7ac09e8e78e..8645914ebaf 100644 --- a/pkg/web/webtest/webtest.go +++ b/pkg/web/webtest/webtest.go @@ -11,6 +11,7 @@ import ( "github.com/grafana/grafana/pkg/api/routing" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/contexthandler/ctxkey" "github.com/grafana/grafana/pkg/web" ) @@ -33,6 +34,9 @@ func NewServer(t testing.TB, routeRegister routing.RouteRegister) *Server { initCtx.Context = c initCtx.Logger = log.New("api-test") c.Map(initCtx) + + c.Req = c.Req.WithContext(ctxkey.Set(c.Req.Context(), initCtx)) + c.Map(c.Req) }) m.Use(requestContextMiddleware())