From 86c618a6d62dcd2f33983fb0cecbad71781da8c3 Mon Sep 17 00:00:00 2001 From: Sven Kirschbaum Date: Fri, 16 Feb 2024 09:43:47 +0100 Subject: [PATCH] Alerting: Escape namespace and group path parameters (#80504) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Jean-Philippe Quéméner --- pkg/services/ngalert/api/lotex_ruler.go | 37 ++- pkg/services/ngalert/api/lotex_ruler_test.go | 263 +++++++++++++++++++ 2 files changed, 286 insertions(+), 14 deletions(-) diff --git a/pkg/services/ngalert/api/lotex_ruler.go b/pkg/services/ngalert/api/lotex_ruler.go index 4e0100304cc..bcffe81c63f 100644 --- a/pkg/services/ngalert/api/lotex_ruler.go +++ b/pkg/services/ngalert/api/lotex_ruler.go @@ -3,6 +3,7 @@ package api import ( "bytes" "fmt" + "io" "net/http" "net/url" @@ -43,15 +44,22 @@ var subtypeToPrefix = map[string]string{ Mimir: mimirPrefix, } +// The requester is primarily used for testing purposes, allowing us to inject a different implementation of withReq. +type requester interface { + withReq(ctx *contextmodel.ReqContext, method string, u *url.URL, body io.Reader, extractor func(*response.NormalResponse) (any, error), headers map[string]string) response.Response +} + type LotexRuler struct { log log.Logger *AlertingProxy + requester requester } func NewLotexRuler(proxy *AlertingProxy, log log.Logger) *LotexRuler { return &LotexRuler{ log: log, AlertingProxy: proxy, + requester: proxy, } } @@ -60,12 +68,12 @@ func (r *LotexRuler) RouteDeleteNamespaceRulesConfig(ctx *contextmodel.ReqContex if err != nil { return ErrResp(500, err, "") } - return r.withReq( + return r.requester.withReq( ctx, http.MethodDelete, withPath( *ctx.Req.URL, - fmt.Sprintf("%s/%s", legacyRulerPrefix, namespace), + fmt.Sprintf("%s/%s", legacyRulerPrefix, url.PathEscape(namespace)), ), nil, messageExtractor, @@ -78,7 +86,7 @@ func (r *LotexRuler) RouteDeleteRuleGroupConfig(ctx *contextmodel.ReqContext, na if err != nil { return ErrResp(500, err, "") } - return r.withReq( + return r.requester.withReq( ctx, http.MethodDelete, withPath( @@ -86,8 +94,8 @@ func (r *LotexRuler) RouteDeleteRuleGroupConfig(ctx *contextmodel.ReqContext, na fmt.Sprintf( "%s/%s/%s", legacyRulerPrefix, - namespace, - group, + url.PathEscape(namespace), + url.PathEscape(group), ), ), nil, @@ -101,7 +109,7 @@ func (r *LotexRuler) RouteGetNamespaceRulesConfig(ctx *contextmodel.ReqContext, if err != nil { return ErrResp(500, err, "") } - return r.withReq( + return r.requester.withReq( ctx, http.MethodGet, withPath( @@ -109,7 +117,7 @@ func (r *LotexRuler) RouteGetNamespaceRulesConfig(ctx *contextmodel.ReqContext, fmt.Sprintf( "%s/%s", legacyRulerPrefix, - namespace, + url.PathEscape(namespace), ), ), nil, @@ -123,7 +131,7 @@ func (r *LotexRuler) RouteGetRulegGroupConfig(ctx *contextmodel.ReqContext, name if err != nil { return ErrResp(500, err, "") } - return r.withReq( + return r.requester.withReq( ctx, http.MethodGet, withPath( @@ -131,8 +139,8 @@ func (r *LotexRuler) RouteGetRulegGroupConfig(ctx *contextmodel.ReqContext, name fmt.Sprintf( "%s/%s/%s", legacyRulerPrefix, - namespace, - group, + url.PathEscape(namespace), + url.PathEscape(group), ), ), nil, @@ -147,7 +155,7 @@ func (r *LotexRuler) RouteGetRulesConfig(ctx *contextmodel.ReqContext) response. return ErrResp(500, err, "") } - return r.withReq( + return r.requester.withReq( ctx, http.MethodGet, withPath( @@ -170,7 +178,7 @@ func (r *LotexRuler) RoutePostNameRulesConfig(ctx *contextmodel.ReqContext, conf return ErrResp(500, err, "Failed marshal rule group") } u := withPath(*ctx.Req.URL, fmt.Sprintf("%s/%s", legacyRulerPrefix, ns)) - return r.withReq(ctx, http.MethodPost, u, bytes.NewBuffer(yml), jsonExtractor(nil), nil) + return r.requester.withReq(ctx, http.MethodPost, u, bytes.NewBuffer(yml), jsonExtractor(nil), nil) } func (r *LotexRuler) validateAndGetPrefix(ctx *contextmodel.ReqContext) (string, error) { @@ -216,7 +224,8 @@ func (r *LotexRuler) validateAndGetPrefix(ctx *contextmodel.ReqContext) (string, } func withPath(u url.URL, newPath string) *url.URL { - // TODO: handle path escaping - u.Path = newPath + u.Path, _ = url.PathUnescape(newPath) + u.RawPath = newPath + return &u } diff --git a/pkg/services/ngalert/api/lotex_ruler_test.go b/pkg/services/ngalert/api/lotex_ruler_test.go index 661377d68f3..c54272e9953 100644 --- a/pkg/services/ngalert/api/lotex_ruler_test.go +++ b/pkg/services/ngalert/api/lotex_ruler_test.go @@ -3,11 +3,15 @@ package api import ( "context" "errors" + "io" "net/http" + "net/url" "testing" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/auth/identity" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" @@ -129,3 +133,262 @@ func (f fakeCacheService) GetDatasourceByUID(ctx context.Context, datasourceUID return f.datasource, nil } + +func TestLotexRuler_RouteDeleteNamespaceRulesConfig(t *testing.T) { + tc := []struct { + name string + namespace string + expected string + urlParams string + namedParams map[string]string + datasource *datasources.DataSource + }{ + { + name: "with a namespace that has to be escaped", + namespace: "namespace/with/slashes", + expected: "http://mimir.com/config/v1/rules/namespace%2Fwith%2Fslashes?subtype=mimir", + urlParams: "?subtype=mimir", + namedParams: map[string]string{":DatasourceUID": "d164"}, + datasource: &datasources.DataSource{URL: "http://mimir.com", Type: PrometheusDatasourceType}, + }, + { + name: "with a namespace that does not need to be escaped", + namespace: "namespace_without_slashes", + expected: "http://mimir.com/config/v1/rules/namespace_without_slashes?subtype=mimir", + urlParams: "?subtype=mimir", + namedParams: map[string]string{":DatasourceUID": "d164"}, + datasource: &datasources.DataSource{URL: "http://mimir.com", Type: PrometheusDatasourceType}, + }, + } + + for _, tt := range tc { + t.Run(tt.name, func(t *testing.T) { + requestMock := RequestMock{} + defer requestMock.AssertExpectations(t) + + requestMock.On( + "withReq", + mock.Anything, + mock.Anything, + mock.AnythingOfType("*url.URL"), + mock.Anything, + mock.Anything, + mock.Anything, + ).Return(response.Empty(200)).Run(func(args mock.Arguments) { + // Validate that the full url as string is equal to the expected value + require.Equal(t, tt.expected, args.Get(2).(*url.URL).String()) + }) + + // Setup Proxy. + proxy := &AlertingProxy{DataProxy: &datasourceproxy.DataSourceProxyService{DataSourceCache: fakeCacheService{datasource: tt.datasource}}} + ruler := &LotexRuler{AlertingProxy: proxy, log: log.NewNopLogger(), requester: &requestMock} + + // Setup request context. + httpReq, err := http.NewRequest(http.MethodGet, tt.datasource.URL+tt.urlParams, nil) + require.NoError(t, err) + ctx := &contextmodel.ReqContext{Context: &web.Context{Req: web.SetURLParams(httpReq, tt.namedParams)}} + + ruler.RouteDeleteNamespaceRulesConfig(ctx, tt.namespace) + }) + } +} + +func TestLotexRuler_RouteDeleteRuleGroupConfig(t *testing.T) { + tc := []struct { + name string + namespace string + group string + expected string + urlParams string + namedParams map[string]string + datasource *datasources.DataSource + }{ + { + name: "with a namespace that has to be escaped", + namespace: "namespace/with/slashes", + group: "group/with/slashes", + expected: "http://mimir.com/config/v1/rules/namespace%2Fwith%2Fslashes/group%2Fwith%2Fslashes?subtype=mimir", + urlParams: "?subtype=mimir", + namedParams: map[string]string{":DatasourceUID": "d164"}, + datasource: &datasources.DataSource{URL: "http://mimir.com", Type: PrometheusDatasourceType}, + }, + { + name: "with a namespace that does not need to be escaped", + namespace: "namespace_without_slashes", + group: "group_without_slashes", + expected: "http://mimir.com/config/v1/rules/namespace_without_slashes/group_without_slashes?subtype=mimir", + urlParams: "?subtype=mimir", + namedParams: map[string]string{":DatasourceUID": "d164"}, + datasource: &datasources.DataSource{URL: "http://mimir.com", Type: PrometheusDatasourceType}, + }, + } + + for _, tt := range tc { + t.Run(tt.name, func(t *testing.T) { + requestMock := RequestMock{} + defer requestMock.AssertExpectations(t) + + requestMock.On( + "withReq", + mock.Anything, + mock.Anything, + mock.AnythingOfType("*url.URL"), + mock.Anything, + mock.Anything, + mock.Anything, + ).Return(response.Empty(200)).Run(func(args mock.Arguments) { + // Validate that the full url as string is equal to the expected value + require.Equal(t, tt.expected, args.Get(2).(*url.URL).String()) + }) + + // Setup Proxy. + proxy := &AlertingProxy{DataProxy: &datasourceproxy.DataSourceProxyService{DataSourceCache: fakeCacheService{datasource: tt.datasource}}} + ruler := &LotexRuler{AlertingProxy: proxy, log: log.NewNopLogger(), requester: &requestMock} + + // Setup request context. + httpReq, err := http.NewRequest(http.MethodGet, tt.datasource.URL+tt.urlParams, nil) + require.NoError(t, err) + ctx := &contextmodel.ReqContext{Context: &web.Context{Req: web.SetURLParams(httpReq, tt.namedParams)}} + + ruler.RouteDeleteRuleGroupConfig(ctx, tt.namespace, tt.group) + }) + } +} + +func TestLotexRuler_RouteGetNamespaceRulesConfig(t *testing.T) { + tc := []struct { + name string + namespace string + group string + expected string + urlParams string + namedParams map[string]string + datasource *datasources.DataSource + }{ + { + name: "with a namespace that has to be escaped", + namespace: "namespace/with/slashes", + expected: "http://mimir.com/config/v1/rules/namespace%2Fwith%2Fslashes?subtype=mimir", + urlParams: "?subtype=mimir", + namedParams: map[string]string{":DatasourceUID": "d164"}, + datasource: &datasources.DataSource{URL: "http://mimir.com", Type: PrometheusDatasourceType}, + }, + { + name: "with a namespace that does not need to be escaped", + namespace: "namespace_without_slashes", + expected: "http://mimir.com/config/v1/rules/namespace_without_slashes?subtype=mimir", + urlParams: "?subtype=mimir", + namedParams: map[string]string{":DatasourceUID": "d164"}, + datasource: &datasources.DataSource{URL: "http://mimir.com", Type: PrometheusDatasourceType}, + }, + } + + for _, tt := range tc { + t.Run(tt.name, func(t *testing.T) { + requestMock := RequestMock{} + defer requestMock.AssertExpectations(t) + + requestMock.On( + "withReq", + mock.Anything, + mock.Anything, + mock.AnythingOfType("*url.URL"), + mock.Anything, + mock.Anything, + mock.Anything, + ).Return(response.Empty(200)).Run(func(args mock.Arguments) { + // Validate that the full url as string is equal to the expected value + require.Equal(t, tt.expected, args.Get(2).(*url.URL).String()) + }) + + // Setup Proxy. + proxy := &AlertingProxy{DataProxy: &datasourceproxy.DataSourceProxyService{DataSourceCache: fakeCacheService{datasource: tt.datasource}}} + ruler := &LotexRuler{AlertingProxy: proxy, log: log.NewNopLogger(), requester: &requestMock} + + // Setup request context. + httpReq, err := http.NewRequest(http.MethodGet, tt.datasource.URL+tt.urlParams, nil) + require.NoError(t, err) + ctx := &contextmodel.ReqContext{Context: &web.Context{Req: web.SetURLParams(httpReq, tt.namedParams)}} + + ruler.RouteGetNamespaceRulesConfig(ctx, tt.namespace) + }) + } +} + +func TestLotexRuler_RouteGetRulegGroupConfig(t *testing.T) { + tc := []struct { + name string + namespace string + group string + expected string + urlParams string + namedParams map[string]string + datasource *datasources.DataSource + }{ + { + name: "with a namespace that has to be escaped", + namespace: "namespace/with/slashes", + group: "group/with/slashes", + expected: "http://mimir.com/config/v1/rules/namespace%2Fwith%2Fslashes/group%2Fwith%2Fslashes?subtype=mimir", + urlParams: "?subtype=mimir", + namedParams: map[string]string{":DatasourceUID": "d164"}, + datasource: &datasources.DataSource{URL: "http://mimir.com", Type: PrometheusDatasourceType}, + }, + { + name: "with a namespace that does not need to be escaped", + namespace: "namespace_without_slashes", + group: "group_without_slashes", + expected: "http://mimir.com/config/v1/rules/namespace_without_slashes/group_without_slashes?subtype=mimir", + urlParams: "?subtype=mimir", + namedParams: map[string]string{":DatasourceUID": "d164"}, + datasource: &datasources.DataSource{URL: "http://mimir.com", Type: PrometheusDatasourceType}, + }, + } + + for _, tt := range tc { + t.Run(tt.name, func(t *testing.T) { + requestMock := RequestMock{} + defer requestMock.AssertExpectations(t) + + requestMock.On( + "withReq", + mock.Anything, + mock.Anything, + mock.AnythingOfType("*url.URL"), + mock.Anything, + mock.Anything, + mock.Anything, + ).Return(response.Empty(200)).Run(func(args mock.Arguments) { + // Validate that the full url as string is equal to the expected value + require.Equal(t, tt.expected, args.Get(2).(*url.URL).String()) + }) + + // Setup Proxy. + proxy := &AlertingProxy{DataProxy: &datasourceproxy.DataSourceProxyService{DataSourceCache: fakeCacheService{datasource: tt.datasource}}} + ruler := &LotexRuler{AlertingProxy: proxy, log: log.NewNopLogger(), requester: &requestMock} + + // Setup request context. + httpReq, err := http.NewRequest(http.MethodGet, tt.datasource.URL+tt.urlParams, nil) + require.NoError(t, err) + ctx := &contextmodel.ReqContext{Context: &web.Context{Req: web.SetURLParams(httpReq, tt.namedParams)}} + + ruler.RouteGetRulegGroupConfig(ctx, tt.namespace, tt.group) + }) + } +} + +type RequestMock struct { + mock.Mock +} + +func (a *RequestMock) withReq( + ctx *contextmodel.ReqContext, + method string, + u *url.URL, + body io.Reader, + extractor func(*response.NormalResponse) (any, error), + headers map[string]string, +) response.Response { + args := a.Called(ctx, method, u, body, extractor, headers) + return args.Get(0).(response.Response) +}