From f64b121ddba80d57eb39c541754291ee807eac4a Mon Sep 17 00:00:00 2001 From: Adela Almasan <88068998+adela-almasan@users.noreply.github.com> Date: Tue, 10 Sep 2024 08:45:27 -0600 Subject: [PATCH] Canvas: Allow API calls to grafana origin (#91822) * allow post URL * check for config * allow relative paths * add allowed internal pattern; add checks for method * update defaults.ini * add custom header * update config comment * use globbing, switch to older middleware - deprecated call * add codeowner * update to use current api, add test * update fall through logic * Update pkg/middleware/validate_action_url.go Co-authored-by: Dan Cech * Update pkg/middleware/validate_action_url.go Co-authored-by: Dan Cech * add more tests * Update pkg/middleware/validate_action_url_test.go Co-authored-by: Dan Cech * fix request headers * add additional tests for all verbs * fix request headers++ * throw error when method is unknown --------- Co-authored-by: Ryan McKinley Co-authored-by: Brian Gann Co-authored-by: Brian Gann Co-authored-by: Dan Cech --- conf/defaults.ini | 3 + conf/sample.ini | 3 + pkg/api/http_server.go | 2 + pkg/middleware/middleware_test.go | 2 + pkg/middleware/validate_action_url.go | 119 +++++++ pkg/middleware/validate_action_url_test.go | 306 ++++++++++++++++++ pkg/setting/setting.go | 2 + .../panel/canvas/editor/element/utils.ts | 74 +++-- 8 files changed, 476 insertions(+), 35 deletions(-) create mode 100644 pkg/middleware/validate_action_url.go create mode 100644 pkg/middleware/validate_action_url_test.go diff --git a/conf/defaults.ini b/conf/defaults.ini index 49209a9ea69..c8a9077c2eb 100644 --- a/conf/defaults.ini +++ b/conf/defaults.ini @@ -406,6 +406,9 @@ csrf_always_check = false # Comma-separated list of plugins ids that won't be loaded inside the frontend sandbox disable_frontend_sandbox_for_plugins = grafana-incident-app +# Comma-separated list of paths for POST/PUT URL in actions. Empty will allow anything that is not on the same origin +actions_allow_post_url = + [security.encryption] # Defines the time-to-live (TTL) for decrypted data encryption keys stored in memory (cache). # Please note that small values may cause performance issues due to a high frequency decryption operations. diff --git a/conf/sample.ini b/conf/sample.ini index f94888ae679..3368d7e288e 100644 --- a/conf/sample.ini +++ b/conf/sample.ini @@ -411,6 +411,9 @@ # Comma-separated list of plugins ids that won't be loaded inside the frontend sandbox ;disable_frontend_sandbox_for_plugins = +# Comma-separated list of paths for POST/PUT URL in actions. Empty will allow anything that is not on the same origin +;actions_allow_post_url = + [security.encryption] # Defines the time-to-live (TTL) for decrypted data encryption keys stored in memory (cache). # Please note that small values may cause performance issues due to a high frequency decryption operations. diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index fa13e721a94..7dcaa3e1625 100644 --- a/pkg/api/http_server.go +++ b/pkg/api/http_server.go @@ -641,6 +641,8 @@ func (hs *HTTPServer) addMiddlewaresAndStaticRoutes() { if hs.Cfg.EnforceDomain { m.Use(middleware.ValidateHostHeader(hs.Cfg)) } + // handle action urls + m.UseMiddleware(middleware.ValidateActionUrl(hs.Cfg, hs.log)) m.Use(middleware.HandleNoCacheHeaders) diff --git a/pkg/middleware/middleware_test.go b/pkg/middleware/middleware_test.go index 06adf52a7a3..0dd9533a729 100644 --- a/pkg/middleware/middleware_test.go +++ b/pkg/middleware/middleware_test.go @@ -244,6 +244,8 @@ func middlewareScenario(t *testing.T, desc string, fn scenarioFunc, cbs ...func( ctxHdlr := getContextHandler(t, cfg, sc.authnService) sc.m.Use(ctxHdlr.Middleware) sc.m.Use(OrgRedirect(sc.cfg, sc.userService)) + // handle action urls + sc.m.Use(ValidateActionUrl(sc.cfg, logger)) sc.defaultHandler = func(c *contextmodel.ReqContext) { require.NotNil(t, c) diff --git a/pkg/middleware/validate_action_url.go b/pkg/middleware/validate_action_url.go new file mode 100644 index 00000000000..1b41293793d --- /dev/null +++ b/pkg/middleware/validate_action_url.go @@ -0,0 +1,119 @@ +package middleware + +import ( + "fmt" + "net/http" + + "github.com/gobwas/glob" + "github.com/grafana/grafana/pkg/cmd/grafana-cli/logger" + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/services/contexthandler" + contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" + "github.com/grafana/grafana/pkg/setting" + "github.com/grafana/grafana/pkg/util" +) + +var ( + errInvalidAllowedURL = func(url string) error { + return fmt.Errorf("action URL '%s' is invalid", url) + } +) + +type errorWithStatus struct { + Underlying error + HTTPStatus int +} + +func (e errorWithStatus) Error() string { + return e.Underlying.Error() +} + +func (e errorWithStatus) Unwrap() error { + return e.Underlying +} + +func ValidateActionUrl(cfg *setting.Cfg, logger log.Logger) func(http.Handler) http.Handler { + // get the urls allowed from server config + allGlobs, globErr := cacheGlobs(cfg.ActionsAllowPostURL) + if globErr != nil { + logger.Error("invalid glob settings in config section [security] actions_allow_post_url", "url", cfg.ActionsAllowPostURL) + } + + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ctx := contexthandler.FromContext(r.Context()) + // if no header + // return nil + // check if action header exists + action := ctx.Req.Header.Get("X-Grafana-Action") + if action == "" { + // header not found, this is not an action request + next.ServeHTTP(w, r) + return + } + if globErr != nil { + http.Error(w, "check server logs for glob configuration failure", http.StatusInternalServerError) + return + } + matchErr := check(ctx, allGlobs, logger) + if matchErr != nil { + http.Error(w, matchErr.Error(), http.StatusMethodNotAllowed) + return + } + // no errors fall through + next.ServeHTTP(w, r) + }) + } +} + +// check +// Detects header for action urls and compares to globbed pattern list +// returns true if allowed +func check(ctx *contextmodel.ReqContext, allGlobs *[]glob.Glob, logger log.Logger) *errorWithStatus { + // only process POST and PUT + if ctx.Req.Method != http.MethodPost && ctx.Req.Method != http.MethodPut { + return &errorWithStatus{ + Underlying: fmt.Errorf("method not allowed for path %s", ctx.Req.URL), + HTTPStatus: http.StatusMethodNotAllowed, + } + } + // for each split config + // if matches glob + // return nil + urlToCheck := ctx.Req.URL + if matchesAllowedPath(allGlobs, urlToCheck.Path) { + return nil + } + logger.Warn("POST/PUT to path not allowed", "url", urlToCheck) + // return some error + return &errorWithStatus{ + Underlying: fmt.Errorf("method POST/PUT not allowed for path %s", urlToCheck), + HTTPStatus: http.StatusMethodNotAllowed, + } +} + +func matchesAllowedPath(allGlobs *[]glob.Glob, pathToCheck string) bool { + logger.Debug("Checking url", "actions", pathToCheck) + for _, rule := range *allGlobs { + logger.Debug("Checking match", "actions", rule) + if rule.Match(pathToCheck) { + // allowed + logger.Debug("POST/PUT call matches allow configuration settings") + return true + } + } + return false +} + +func cacheGlobs(actionsAllowPostURL string) (*[]glob.Glob, error) { + allowedUrls := util.SplitString(actionsAllowPostURL) + allGlobs := make([]glob.Glob, 0) + for _, i := range allowedUrls { + g, err := glob.Compile(i) + if err != nil { + return nil, errInvalidAllowedURL(err.Error()) + } + allGlobs = append(allGlobs, g) + } + return &allGlobs, nil +} diff --git a/pkg/middleware/validate_action_url_test.go b/pkg/middleware/validate_action_url_test.go new file mode 100644 index 00000000000..ffee99c4793 --- /dev/null +++ b/pkg/middleware/validate_action_url_test.go @@ -0,0 +1,306 @@ +package middleware + +import ( + "fmt" + "net/http" + "testing" + + "github.com/grafana/grafana/pkg/setting" + "github.com/stretchr/testify/assert" +) + +func TestMiddlewareValidateActionUrl(t *testing.T) { + tests := []struct { + name string + method string + path string + actionsAllowPostURL string + addHeader bool + code int + }{ + { + name: "DELETE action with valid path", + method: "DELETE", + path: "/api/plugins/org-generic-app", + actionsAllowPostURL: "/api/plugins/*", + addHeader: true, + code: http.StatusMethodNotAllowed, + }, + { + name: "DELETE action with invalid path", + method: "DELETE", + path: "/api/notplugins/org-generic-app", + actionsAllowPostURL: "/api/plugins/*", + addHeader: true, + code: http.StatusMethodNotAllowed, + }, + { + name: "GET action with valid path", + method: "GET", + path: "/api/plugins/org-generic-app", + actionsAllowPostURL: "/api/plugins/*", + addHeader: true, + code: http.StatusMethodNotAllowed, + }, + { + name: "GET action with invalid path", + method: "GET", + path: "/api/notplugins/org-generic-app", + actionsAllowPostURL: "/api/plugins/*", + addHeader: true, + code: http.StatusMethodNotAllowed, + }, + { + name: "GET valid path without header", + method: "GET", + path: "/", // top-level get + actionsAllowPostURL: "", + addHeader: false, + code: http.StatusOK, + }, + { + name: "GET valid path with header", + method: "GET", + path: "/", // top-level get + actionsAllowPostURL: "", + addHeader: true, + code: http.StatusMethodNotAllowed, + }, + { + name: "HEAD request with header", + method: "HEAD", + path: "/", // top-level + actionsAllowPostURL: "", + addHeader: true, + code: http.StatusMethodNotAllowed, + }, + { + name: "OPTIONS request", + method: "OPTIONS", + path: "/", // top-level + actionsAllowPostURL: "", + addHeader: false, + code: http.StatusOK, + }, + { + name: "OPTIONS request with header", + method: "OPTIONS", + path: "/", // top-level + actionsAllowPostURL: "", + addHeader: true, + code: http.StatusMethodNotAllowed, + }, + { + name: "PATCH request with header", + method: "PATCH", + path: "/", // top-level + actionsAllowPostURL: "", + addHeader: true, + code: http.StatusMethodNotAllowed, + }, + { + name: "PATCH request without header", + method: "PATCH", + path: "/", // top-level + actionsAllowPostURL: "", + addHeader: false, + code: http.StatusOK, + }, + { + name: "POST without action header", + method: "POST", + path: "/api/plugins/org-generic-app", + actionsAllowPostURL: "", + addHeader: false, + code: http.StatusOK, + }, + { + name: "POST with action header, no paths defined", + method: "POST", + path: "/api/plugins/org-generic-app", + actionsAllowPostURL: "", + addHeader: true, + code: http.StatusMethodNotAllowed, + }, + { + name: "POST action with allowed path", + method: "POST", + path: "/api/plugins/org-generic-app", + actionsAllowPostURL: "/api/plugins/*", + addHeader: true, + code: http.StatusOK, + }, + { + name: "POST action with invalid path", + method: "POST", + path: "/api/notplugins/org-generic-app", + actionsAllowPostURL: "/api/plugins/*", + addHeader: true, + code: http.StatusMethodNotAllowed, + }, + { + name: "PUT action with valid path with header", + method: "PUT", + path: "/api/plugins/org-generic-app", + actionsAllowPostURL: "/api/plugins/*", + addHeader: true, + code: http.StatusOK, + }, + { + name: "PUT action with invalid path", + method: "PUT", + path: "/api/notplugins/org-generic-app", + actionsAllowPostURL: "/api/plugins/*", + addHeader: true, + code: http.StatusMethodNotAllowed, + }, + { + name: "PUT action with valid path without header", + method: "PUT", + path: "/api/plugins/org-generic-app", + actionsAllowPostURL: "/api/plugins/*", + addHeader: false, + code: http.StatusOK, + }, + { + name: "PUT action with invalid path without header", + method: "PUT", + path: "/api/notplugins/org-generic-app", + actionsAllowPostURL: "/api/plugins/*", + addHeader: false, + code: http.StatusOK, + }, + { + name: "CONNECT unknown verb with header", + method: "CONNECT", + path: "/api/notplugins/org-generic-app", + actionsAllowPostURL: "/api/plugins/*", + addHeader: true, + code: http.StatusMethodNotAllowed, + }, + { + name: "CONNECT unknown verb without header", + method: "CONNECT", + path: "/api/notplugins/org-generic-app", + actionsAllowPostURL: "/api/plugins/*", + addHeader: false, + code: http.StatusNotFound, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + middlewareScenario(t, tt.name, func(t *testing.T, sc *scenarioContext) { + switch tt.method { + case "DELETE": + sc.m.Delete(tt.path, sc.defaultHandler) + case "GET": + sc.m.Get(tt.path, sc.defaultHandler) + case "HEAD": + sc.m.Head(tt.path, sc.defaultHandler) + case "OPTIONS": + sc.m.Options(tt.path, sc.defaultHandler) + case "PATCH": + sc.m.Patch(tt.path, sc.defaultHandler) + case "POST": + sc.m.Post(tt.path, sc.defaultHandler) + case "PUT": + sc.m.Put(tt.path, sc.defaultHandler) + default: + // anything else is an error + anError := fmt.Errorf("unknown verb: %s", tt.method) + if assert.Errorf(t, anError, "unknown verb: %s", tt.method) { + assert.Contains(t, anError.Error(), "unknown verb") + } + } + sc.fakeReq(tt.method, tt.path) + if tt.addHeader { + sc.req.Header.Add("X-Grafana-Action", "1") + } + sc.exec() + resp := sc.resp.Result() + t.Cleanup(func() { + err := resp.Body.Close() + assert.NoError(t, err) + }) + // nolint:bodyclose + assert.Equal(t, tt.code, sc.resp.Result().StatusCode) + }, func(cfg *setting.Cfg) { + cfg.ActionsAllowPostURL = tt.actionsAllowPostURL + }) + }) + } +} + +func TestMatchesAllowedPath(t *testing.T) { + tests := []struct { + name string + aPath string + allowList string + matches bool + }{ + { + name: "single url with match", + allowList: "/api/plugins/*", + aPath: "/api/plugins/my-plugin", + matches: true, + }, + { + name: "single url no match", + allowList: "/api/plugins/*", + aPath: "/api/plugin/my-plugin", + matches: false, + }, + { + name: "multiple urls with match", + allowList: "/api/plugins/*, /api/other/**", + aPath: "/api/other/my-plugin", + matches: true, + }, + { + name: "multiple urls no match", + allowList: "/api/plugins/*, /api/other/**", + aPath: "/api/misc/my-plugin", + matches: false, + }, + } + + for _, tc := range tests { + tc := tc + + t.Run(tc.name, func(t *testing.T) { + allGlobs, err := cacheGlobs(tc.allowList) + matched := matchesAllowedPath(allGlobs, tc.aPath) + assert.NoError(t, err) + assert.Equal(t, matched, tc.matches) + }) + } +} + +func TestCacheGlobs(t *testing.T) { + tests := []struct { + name string + allowList string + expectedLength int + }{ + { + name: "single url", + allowList: "/api/plugins", + expectedLength: 1, + }, + { + name: "multiple urls", + allowList: "/api/plugins, /api/other/**", + expectedLength: 2, + }, + } + + for _, tc := range tests { + tc := tc + + t.Run(tc.name, func(t *testing.T) { + cache, err := cacheGlobs(tc.allowList) + assert.NoError(t, err) + assert.Equal(t, len(*cache), tc.expectedLength) + }) + } +} diff --git a/pkg/setting/setting.go b/pkg/setting/setting.go index 375c03ff6da..ef476357815 100644 --- a/pkg/setting/setting.go +++ b/pkg/setting/setting.go @@ -180,6 +180,7 @@ type Cfg struct { DisableFrontendSandboxForPlugins []string DisableGravatar bool DataProxyWhiteList map[string]bool + ActionsAllowPostURL string TempDataLifetime time.Duration @@ -1538,6 +1539,7 @@ func readSecuritySettings(iniFile *ini.File, cfg *Cfg) error { cfg.ContentTypeProtectionHeader = security.Key("x_content_type_options").MustBool(true) cfg.XSSProtectionHeader = security.Key("x_xss_protection").MustBool(true) + cfg.ActionsAllowPostURL = security.Key("actions_allow_post_url").MustString("") cfg.StrictTransportSecurity = security.Key("strict_transport_security").MustBool(false) cfg.StrictTransportSecurityMaxAge = security.Key("strict_transport_security_max_age_seconds").MustInt(86400) cfg.StrictTransportSecurityPreload = security.Key("strict_transport_security_preload").MustBool(false) diff --git a/public/app/plugins/panel/canvas/editor/element/utils.ts b/public/app/plugins/panel/canvas/editor/element/utils.ts index c99c844cc9f..e1ee7aa7da1 100644 --- a/public/app/plugins/panel/canvas/editor/element/utils.ts +++ b/public/app/plugins/panel/canvas/editor/element/utils.ts @@ -1,6 +1,7 @@ -import { AppEvents } from '@grafana/data'; +import { AppEvents, textUtil } from '@grafana/data'; import { BackendSrvRequest, getBackendSrv, getTemplateSrv } from '@grafana/runtime'; import { appEvents } from 'app/core/core'; +import { createAbsoluteUrl, RelativeUrl } from 'app/features/alerting/unified/utils/url'; import { getDashboardSrv } from 'app/features/dashboard/services/DashboardSrv'; import { HttpRequestMethod } from '../../panelcfg.gen'; @@ -10,30 +11,26 @@ import { APIEditorConfig } from './APIEditor'; type IsLoadingCallback = (loading: boolean) => void; export const callApi = (api: APIEditorConfig, updateLoadingStateCallback?: IsLoadingCallback) => { - if (api && api.endpoint) { - // If API endpoint origin matches Grafana origin, don't call it. - const interpolatedUrl = interpolateVariables(api.endpoint); - if (requestMatchesGrafanaOrigin(interpolatedUrl)) { - appEvents.emit(AppEvents.alertError, ['Cannot call API at Grafana origin.']); - updateLoadingStateCallback && updateLoadingStateCallback(false); - return; - } - const request = getRequest(api); - - getBackendSrv() - .fetch(request) - .subscribe({ - error: (error) => { - appEvents.emit(AppEvents.alertError, ['An error has occurred. Check console output for more details.']); - console.error('API call error: ', error); - updateLoadingStateCallback && updateLoadingStateCallback(false); - }, - complete: () => { - appEvents.emit(AppEvents.alertSuccess, ['API call was successful']); - updateLoadingStateCallback && updateLoadingStateCallback(false); - }, - }); + if (!api.endpoint) { + appEvents.emit(AppEvents.alertError, ['API endpoint is not defined.']); + return; } + + const request = getRequest(api); + + getBackendSrv() + .fetch(request) + .subscribe({ + error: (error) => { + appEvents.emit(AppEvents.alertError, ['An error has occurred. Check console output for more details.']); + console.error('API call error: ', error); + updateLoadingStateCallback && updateLoadingStateCallback(false); + }, + complete: () => { + appEvents.emit(AppEvents.alertSuccess, ['API call was successful']); + updateLoadingStateCallback && updateLoadingStateCallback(false); + }, + }); }; export const interpolateVariables = (text: string) => { @@ -42,9 +39,10 @@ export const interpolateVariables = (text: string) => { }; export const getRequest = (api: APIEditorConfig) => { - const requestHeaders: HeadersInit = []; + const endpoint = getEndpoint(interpolateVariables(api.endpoint)); + const url = new URL(endpoint); - const url = new URL(interpolateVariables(api.endpoint!)); + const requestHeaders: Record = {}; let request: BackendSrvRequest = { url: url.toString(), @@ -54,23 +52,24 @@ export const getRequest = (api: APIEditorConfig) => { }; if (api.headerParams) { - api.headerParams.forEach((param) => { - requestHeaders.push([interpolateVariables(param[0]), interpolateVariables(param[1])]); + api.headerParams.forEach(([name, value]) => { + requestHeaders[interpolateVariables(name)] = interpolateVariables(value); }); } if (api.queryParams) { - api.queryParams?.forEach((param) => { - url.searchParams.append(interpolateVariables(param[0]), interpolateVariables(param[1])); + api.queryParams?.forEach(([name, value]) => { + url.searchParams.append(interpolateVariables(name), interpolateVariables(value)); }); request.url = url.toString(); } if (api.method === HttpRequestMethod.POST) { - requestHeaders.push(['Content-Type', api.contentType!]); + requestHeaders['Content-Type'] = api.contentType!; } + requestHeaders['X-Grafana-Action'] = '1'; request.headers = requestHeaders; return request; @@ -85,8 +84,13 @@ const getData = (api: APIEditorConfig) => { return data; }; -const requestMatchesGrafanaOrigin = (requestEndpoint: string) => { - const requestURL = new URL(requestEndpoint); - const grafanaURL = new URL(window.location.origin); - return requestURL.origin === grafanaURL.origin; +const getEndpoint = (endpoint: string) => { + const isRelativeUrl = endpoint.startsWith('/'); + if (isRelativeUrl) { + // eslint-disable-next-line @typescript-eslint/consistent-type-assertions + const sanitizedRelativeURL = textUtil.sanitizeUrl(endpoint) as RelativeUrl; + endpoint = createAbsoluteUrl(sanitizedRelativeURL, []); + } + + return endpoint; };